feat: CoreGO v2 — unified struct, DTO pattern, zero constructors

[Snider]

Summary

• Complete architectural overhaul of pkg/core (17 files, ~4300 lines, zero deps)
• All subsystem types as DTOs, all methods on *Core
• Pure struct literal init in New() — no constructors
• Service() unified get/register with named mutex map
• Error system: Err/ErrLog/ErrPan + Log/LogErr/LogPan with shared ErrSink interface
• CoreCommand with optional description (i18n resolves from command path)
• Tests moved to tests/ directory (black-box)
• Removed: ServiceFor/MustServiceFor, global instance, Display/Workspace/Crypt

Test plan

• go build ./pkg/core/ passes
• go vet ./pkg/core/ passes
• Gemini security review — clean
• Codex refactoring audit — clean
• core-agent binary builds and dispatches agents
• CodeRabbit review

🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io

Summary by CodeRabbit

• New Features

  • CLI framework (nested commands, flags, actions), IPC message/query/task bus, async background tasks, embedded asset management with template extraction, sandboxed filesystem, internationalisation, config system with typed accessors, service registry and lifecycle/runtime composition.

• Improvements

  • Structured error handling, crash reporting and richer logging with redaction; additional runtime and startup/shutdown helpers.

• Architecture Changes

  • Core public surface reorganised and tests moved to external test packages.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 90ea845b-af4d-4245-af87-1522f25afe49

📥 Commits

Reviewing files that changed from the base of the PR and between 4fa90a8 and 7c7a257.

📒 Files selected for processing (1)

• pkg/core/error.go

---

📝 Walkthrough

Walkthrough

Introduces a comprehensive Core framework: new CLI, service registry, IPC (actions/queries/tasks) and async tasking, structured error/logging/crash reporting, asset/embed and sandboxed filesystem, config and i18n subsystems, utilities; removes several legacy internals and migrates tests to the new public API surface.

Changes

Cohort / File(s)	Summary
Repo config & docs \.claude/settings.json, \.mcp.json, CLAUDE.md	Add Claude + MCP config files and update CLAUDE.md (session context, error guidance, Go version note).
Module deps go.mod	Adjusts module requires/indirect dependencies and test dependency versions.
CLI framework pkg/core/cli.go, pkg/core/command.go	New CLI/Command framework: nested commands, flags (many types + slices), positional args, hooks, help and dynamic action binding.
Core bootstrap & API surface pkg/core/core.go, pkg/core/contract.go, pkg/core/interfaces.go (removed)	Introduce a new Core struct and option pattern; remove legacy global singletons and the old interfaces file; reorganise public accessors.
Service registry & lifecycle pkg/core/service.go, pkg/core/service_manager.go (removed), tests/service_manager_test.go (added)	Add exported Service container and Core.Service registration/retrieval; remove internal serviceManager implementation; add new public tests.
IPC / message bus & tasking pkg/core/ipc.go, pkg/core/message_bus.go (removed), pkg/core/task.go, tests/message_bus_test.go (added)	Add new Ipc type and Core Action/Query/QueryAll/RegisterQuery APIs; remove old messageBus; add async tasking and related tests.
Error handling & crash reporting pkg/core/error.go, pkg/core/e.go (removed), tests/e_test.go	Replace legacy error type with structured Err (codes, wrapping, stack), logging helpers and crash reporting (ErrPan); remove old e.go.
Logging & rotation pkg/core/log.go, pkg/log/*.go (removed), pkg/log/rotation.go (removed)	Add structured logging in core (levels, redact, hooks) and remove previous pkg/log re-exports and rotating-writer implementation/tests.
Filesystem & assets pkg/core/fs.go, pkg/core/embed.go	Add sandboxed Fs with symlink/path validation and a comprehensive embed/asset registry, scanning and pack-generation plus template extraction utilities.
Configuration & utilities pkg/core/config.go, pkg/core/array.go	Add generic Config with typed getters and feature flags, ConfigVar; add generic Array[T] collection utilities.
I18n pkg/core/i18n.go	Add Translator interface and I18n struct for locale mounts, translator registration and language management (thread-safe).
Runtime wiring & service runtime pkg/core/runtime.go, pkg/core/runtime_pkg.go (removed)	Add runtime helpers and ServiceRuntime/factory wiring; remove older duplicate runtime file.
Locking & sync helpers pkg/core/lock.go	Add named Lock wrapper and Core lock helpers for service locking and startable/stoppable snapshotting.
Public API removals & reorganisation pkg/core/interfaces.go (removed), multiple deleted internals	Remove many legacy public types/methods and tests; reorganise public surface around Core, Service, IPC, Config, log and embed APIs.
Tests: migration & updates tests/*_test.go (many modified), pkg/core/*_test.go (many removed)	Migrate tests to external core_test (dot-import), update tests to new public APIs; add tests for IPC and new service manager; remove tests tied to deleted internals.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Caller
participant Core
participant Ipc
participant ServiceHandler

Caller->>Core: Action/Query/Task request
Core->>Ipc: dispatch Action/Query/Task
Ipc->>ServiceHandler: invoke registered handlers (concurrent/snapshot)
alt Query first-handler
ServiceHandler-->>Ipc: (result, handled=true, err)
Ipc-->>Core: return first handled result
else QueryAll / Action
ServiceHandler-->>Ipc: (result?, handled?, err)
Ipc-->>Core: aggregate results/errors
end
Core-->>Caller: return result / aggregated errors

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: CoreGO v2 — unified struct, DTO pattern, zero constructors' clearly and specifically summarises the main architectural changes: a major version upgrade with structural refactoring, pattern changes, and removal of constructor functions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.3)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 19

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (11)

pkg/core/fs.go-77-79 (1)

77-79: ⚠️ Potential issue | 🟡 Minor

Tighten the escape check to path segments.

strings.HasPrefix(rel, "..") also rejects valid in-sandbox names such as ..cache or ..config. Check rel == ".." or strings.HasPrefix(rel, ".."+string(os.PathSeparator)) instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/fs.go` around lines 77 - 79, Tighten the sandbox-escape check around
filepath.Rel: in the code that computes rel := filepath.Rel(m.root, realNext)
replace the unsafe strings.HasPrefix(rel, "..") test with a strict check that
only rejects parent-directory escapes, e.g. check rel == ".." ||
strings.HasPrefix(rel, ".."+string(os.PathSeparator)); ensure this change is
applied where m.root, realNext and rel are used and keep imports (os, strings)
available.

CLAUDE.md-75-75 (1)

75-75: ⚠️ Potential issue | 🟡 Minor

Update this guidance to the new core surface.

These lines still point contributors at runtime_pkg.go, go-log, and e.go, but the PR removes those pieces and folds error/logging into pkg/core. Leaving this here will send follow-up changes towards deleted files and stale APIs.

Also applies to: 84-89

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` at line 75, Update the documentation under "ServiceRuntime Generic
Helper (`runtime_pkg.go`)" to remove references to removed files
(`runtime_pkg.go`, `go-log`, `e.go`) and instead point contributors to the new
core surface in pkg/core; explicitly mention the new error/logging helpers in
pkg/core (the consolidated error and logging APIs) and update the guidance text
where similar references appear (lines ~84-89) so contributors are directed to
use the pkg/core error and logging utilities instead of the deleted files.

tests/fuzz_test.go-54-54 (1)

54-54: ⚠️ Potential issue | 🟡 Minor

Check the constructor error in the harness.

Ignoring New() failures turns setup problems into unrelated fuzz crashes and makes reproductions noisier than they need to be.

Also applies to: 88-88

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/fuzz_test.go` at line 54, The test harness currently ignores the error
return from New() (c, _ := New()), which can mask setup failures; update both
occurrences to capture and check the error (e.g., c, err := New()) and fail the
test immediately on error (use t.Fatalf or panic in the fuzz function) so
constructor failures surface as setup errors rather than downstream fuzz
crashes; ensure both instances that used the blank error are updated to the same
pattern.

pkg/core/error.go-23-30 (1)

23-30: ⚠️ Potential issue | 🟡 Minor

The exported ErrSink contract does not match the advertised implementers.

ErrSink expects Error/Warn(msg string, keyvals ...any), but neither ErrLog nor ErrPan has that shape; only *Log satisfies the interface here. Anyone following the comment will hit a compile-time mismatch.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 23 - 30, ErrSink's method signatures
(Error/Warn(msg string, keyvals ...any)) don't match the actual exported
implementers mentioned in the comment (ErrLog, ErrPan) and only *Log currently
satisfies it; update the contract so it matches the real types or update the
types to match the contract: locate ErrSink, ErrLog, ErrPan and Log and either
change ErrSink's method signatures to the exact signatures implemented by ErrLog
and ErrPan or refactor ErrLog/ErrPan to implement Error(msg string, keyvals
...any) and Warn(msg string, keyvals ...any); also fix or remove the misleading
comment and adjust the type assertion var _ ErrSink = (*Log)(nil) to reference
the correct implementing types (e.g., ErrLog/ErrPan) so compile-time checks
reflect the intended implementers.

CLAUDE.md-108-108 (1)

108-108: ⚠️ Potential issue | 🟡 Minor

Avoid hard-coding a workstation-specific workspace path.

~/Code/go.work only exists on one machine. Keep the Go version / go work steps, but drop the personal path so contributors do not assume the same local layout.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` at line 108, The README line currently hard-codes a personal
workspace path (`~/Code/go.work`); remove that path and instead state the Go
version and generic go work instruction. Replace the sentence "Uses Go 1.26
workspaces. This module is part of the workspace at `~/Code/go.work`." with
something like "Uses Go 1.26 workspaces; add this module to your go.work (no
specific local path required)" so it keeps the Go 1.26/work guidance but drops
the workstation-specific `~/Code/go.work` reference.

tests/runtime_pkg_test.go-124-125 (1)

124-125: ⚠️ Potential issue | 🟡 Minor

Update the comment to match the method name.

The note mentions sr.Cfg(), but the assertion below is exercising sr.Config(). The stale name makes this block look only partially renamed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/runtime_pkg_test.go` around lines 124 - 125, Comment references the
stale method name sr.Cfg() while the test exercises sr.Config(); update the
comment to mention sr.Config() so it matches the assertion and avoids confusion.
Locate the comment near the test that asserts sr.Config() (search for "We can't
directly test" or the use of sr.Config()) and replace "sr.Cfg()" with
"sr.Config()" and adjust wording if necessary to reflect that sr.Config() will
panic without a registered config service.

pkg/core/service.go-41-52 (1)

41-52: ⚠️ Potential issue | 🟡 Minor

Same type assertion issue in the registration branch.

Line 42 has the same silent failure pattern. A non-string first argument would result in an empty-name error, but this masks the actual programming error (wrong argument type).

Proposed fix

 	default:
-		name, _ := args[0].(string)
+		name, ok := args[0].(string)
+		if !ok {
+			return E("core.Service", "service name must be a string", nil)
+		}
 		if name == "" {
 			return E("core.Service", "service name cannot be empty", nil)
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/service.go` around lines 41 - 52, In the registration branch of the
default case, replace the silent type assertion "name, _ := args[0].(string)"
with a checked assertion (e.g., "name, ok := args[0].(string)" and if !ok return
an error) so that non-string first arguments produce a clear type error instead
of being treated as an empty name; update the logic around
c.Lock("srv").Mu.Lock() / c.srv.Services checks to use the validated name after
confirming it's a string and non-empty.

pkg/core/service.go-32-40 (1)

32-40: ⚠️ Potential issue | 🟡 Minor

Address silent type assertion failure in lookup operation.

The type assertion args[0].(string) on line 33 uses the blank identifier, causing non-string arguments to silently produce an empty string lookup instead of an error. Check the type explicitly:

 	case 1:
-		name, _ := args[0].(string)
+		name, ok := args[0].(string)
+		if !ok {
+			return nil
+		}
 		c.Lock("srv").Mu.RLock()
 		v, ok := c.srv.Services[name]
 		c.Lock("srv").Mu.RUnlock()

(Note: The redundant c.Lock("srv") calls are safe — the Lock method returns the same mutex instance for the same name, so no synchronisation issue exists.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/service.go` around lines 32 - 40, The lookup currently uses a silent
type assertion (args[0].(string) with blank identifier) which yields an empty
string on wrong types; change it to perform an explicit checked assertion like
name, ok := args[0].(string) and if !ok return nil (or an appropriate error)
before accessing c.srv.Services; keep the existing
c.Lock("srv").Mu.RLock()/RUnlock() usage around the map access (v, ok :=
c.srv.Services[name]) unchanged.

pkg/core/cli.go-97-101 (1)

97-101: ⚠️ Potential issue | 🟡 Minor

Potential nil pointer dereference in PrintBanner().

If bannerFunction is nil, line 99 will panic. Consider adding a nil check or ensuring bannerFunction is always initialised (e.g., defaulting to defaultBannerFunction).

🛡️ Proposed fix with nil guard

 func (c *Cli) PrintBanner() {
+	if c.bannerFunction == nil {
+		fmt.Println(defaultBannerFunction(c))
+	} else {
-	fmt.Println(c.bannerFunction(c))
+		fmt.Println(c.bannerFunction(c))
+	}
 	fmt.Println("")
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/cli.go` around lines 97 - 101, PrintBanner may dereference a nil
bannerFunction on Cli; update PrintBanner to guard against nil by checking
c.bannerFunction and falling back to a default (e.g., defaultBannerFunction)
before calling it, or ensure Cli construction always sets bannerFunction;
reference the Cli type and the PrintBanner method and use defaultBannerFunction
as the fallback to avoid the nil pointer panic.

pkg/core/cli.go-192-194 (1)

192-194: ⚠️ Potential issue | 🟡 Minor

Potential nil pointer dereference in OtherArgs().

OtherArgs() accesses c.rootCommand.flags.Args() without nil checks. If rootCommand or flags is nil (e.g., Cli not fully initialised), this will panic.

🛡️ Proposed fix with nil guards

 func (c *Cli) OtherArgs() []string {
+	if c.rootCommand == nil || c.rootCommand.flags == nil {
+		return nil
+	}
 	return c.rootCommand.flags.Args()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/cli.go` around lines 192 - 194, OtherArgs currently calls
c.rootCommand.flags.Args() without nil checks which can panic; update OtherArgs
(method on type Cli) to guard against nil c, nil c.rootCommand, and nil
c.rootCommand.flags and return an empty slice (or nil) when any are nil,
otherwise return c.rootCommand.flags.Args(); ensure you reference the
Cli.OtherArgs method and the rootCommand and flags fields when adding the nil
checks.

pkg/core/config.go-101-106 (1)

101-106: ⚠️ Potential issue | 🟡 Minor

Missing nil check in Enabled() — potential nil pointer dereference.

Unlike Get() which checks e.ConfigOpts == nil at line 63, Enabled() directly accesses e.Features[feature] without verifying that ConfigOpts or Features is initialised. This could panic if Config is used before any feature is enabled/disabled.

🛡️ Proposed fix to add nil check

 func (e *Config) Enabled(feature string) bool {
 	e.mu.RLock()
+	defer e.mu.RUnlock()
+	if e.ConfigOpts == nil || e.Features == nil {
+		return false
+	}
 	v := e.Features[feature]
-	e.mu.RUnlock()
 	return v
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/config.go` around lines 101 - 106, The Enabled method can panic if
ConfigOpts or Features is nil; update Config.Enabled to guard against nil by
checking e.ConfigOpts and e.ConfigOpts.Features (or e.Features) before accessing
the map: acquire the read lock (e.mu.RLock()), return false immediately if the
feature map is nil, otherwise read v := e.Features[feature], then release the
lock and return v; reference the Enabled method and the Config.ConfigOpts /
Config.Features fields when making the change.

🧹 Nitpick comments (11)

.mcp.json (1)

3-7: Config is solid; consider documenting the PATH requirement.

This will work as-is, but a short setup note that core-agent must be discoverable on PATH would reduce onboarding friction.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.mcp.json around lines 3 - 7, Add a short setup note to the repository
README or a contributing/setup doc that explains the "core" config expects the
"core-agent" executable to be discoverable on the system PATH (since "command":
"core-agent" is used with "type": "stdio" and "args": ["mcp"]); include
suggested verification steps (e.g., run `which core-agent` or `core-agent
--version`) and mention updating PATH or installing the agent if not found.

pkg/core/array.go (1)

14-15: Consider cloning at the API boundary.

NewArray keeps the caller's backing slice, and AsSlice returns the internal one unchanged. That makes external mutation able to bypass AddUnique/Deduplicate and creates surprising aliasing when the input came from someSlice....

Also applies to: 93-95

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/array.go` around lines 14 - 15, NewArray currently stores the
caller's backing slice and AsSlice returns the internal slice directly, allowing
external mutation to bypass protections like AddUnique and Deduplicate; to fix,
copy the input variadic slice into a new slice inside NewArray (and any other
factory/constructor that takes a slice, e.g., the code around the referenced
lines) so the Array has its own backing storage, and change AsSlice to return a
copied slice (not the internal slice pointer) so callers cannot mutate internal
state; update tests if any rely on aliasing.

pkg/core/i18n.go (1)

94-103: Consider documenting the silent no-op behaviour of SetLanguage when no translator is registered.

Currently, SetLanguage returns nil (success) even when no translator is registered. This could be surprising to callers who expect an error indicating the operation had no effect. If this is intentional (e.g., to allow deferred translator registration), a brief doc note would help.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/i18n.go` around lines 94 - 103, Update the doc comment for the
SetLanguage method on the I18n type to explicitly state that it is a no-op and
returns nil when no translator is registered; reference the method name
SetLanguage and the field translator so reviewers can find the implementation,
and optionally note intended behavior (e.g., allows deferred translator
registration) or suggest callers check for a registered translator if they need
an error instead.

pkg/core/app.go (1)

38-52: Consider documenting that Find only populates a subset of App fields.

The function returns an App with only Name, Filename, and Path populated, leaving Version, Description, and Runtime at their zero values. This is fine for a finder utility, but callers might expect a fully populated struct from the name.

Consider either:

• Renaming to FindPath or LocateApp to clarify intent, or
• Adding a brief doc note that other fields remain unpopulated

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/app.go` around lines 38 - 52, The Find function only fills Name,
Filename and Path on the App struct; document this by adding a concise doc
comment above Find stating that Version, Description, and Runtime are left
unpopulated (zero values), or alternatively rename the function to a clearer
name like FindPath or LocateApp and update any callers accordingly so intent is
explicit; reference the Find function and the App struct when making the change.

pkg/core/ipc.go (1)

41-53: Query returns error even when handled — verify this is intentional.

On line 49, if a handler returns handled=true with a non-nil error, both the result and error are returned to the caller. This differs from the QueryAll approach where errors are aggregated separately.

If a handled query should be considered successful regardless of error, you may want to return nil for the error. Otherwise, document that a handled result can still carry an error.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/ipc.go` around lines 41 - 53, The current Core.Query method returns
the handler's error even when handled==true; decide whether a "handled" response
should suppress handler errors or propagate them. If you want handled queries
considered successful, update Core.Query (the Query method on type Core) to
return (result, true, nil) whenever a handler returns handled==true, discarding
err; otherwise add clear documentation to the Query method signature/docstring
stating that a handled result may still include a non-nil error and keep the
current behavior. Ensure the chosen behavior is consistent with QueryAll's error
aggregation.

pkg/core/embed.go (2)

522-530: File close error not checked after template execution.

At line 530, f.Close() is called but its error is ignored. If the close fails (e.g., due to filesystem issues), data may be lost. Consider using defer with error capture or checking the close error.

♻️ Proposed fix with proper error handling

 		f, err := os.Create(targetFile)
 		if err != nil {
 			return err
 		}
-		if err := tmpl.Execute(f, data); err != nil {
-			f.Close()
+		execErr := tmpl.Execute(f, data)
+		closeErr := f.Close()
+		if execErr != nil {
-			return err
+			return execErr
 		}
-		f.Close()
+		if closeErr != nil {
+			return closeErr
+		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/embed.go` around lines 522 - 530, The file currently ignores the
error from f.Close() after writing the template; update the write-and-close
sequence (the os.Create call that returns f and the tmpl.Execute(f, data) usage)
to capture and handle close errors: use a deferred close that captures the error
(e.g., defer func(){ if cerr := f.Close(); err == nil { err = cerr } }()) or
check and return the close error after tmpl.Execute so any filesystem/flush
error is returned (ensure you preserve and wrap the tmpl.Execute error if both
occur).

---

584-591: io.Copy error could leave destination file in inconsistent state.

If io.Copy fails partway through, the destination file will contain partial data. The deferred d.Close() will still run, but the partial file remains. Consider removing the partial file on error or documenting this behaviour.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/embed.go` around lines 584 - 591, The current write flow in
pkg/core/embed.go uses io.Copy to write to the destination file (variable d) and
returns the io.Copy error, but if io.Copy fails the partial file at target
remains; update the logic around io.Copy(d, s) so that if io.Copy returns a
non-nil error you close d (if not already closed) and remove the partially
written file (os.Remove(target)) before returning the error; reference the
variables/function names d, io.Copy and target to locate and change this
behavior (alternatively, explicitly document that partial files may remain if
you choose not to remove them).

pkg/core/config.go (2)

17-24: ConfigVar methods are not thread-safe.

The Set, Get, IsSet, and Unset methods on ConfigVar[T] have no synchronisation. If instances are shared across goroutines, this could lead to data races.

If ConfigVar is intended for single-threaded use or external synchronisation, consider documenting this constraint. Otherwise, consider adding a mutex.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/config.go` around lines 17 - 24, The ConfigVar methods (ConfigVar,
Get, Set, IsSet, Unset) are not concurrency-safe; add a sync.RWMutex field to
the ConfigVar[T] struct and use it to protect access: use r.Lock()/r.Unlock()
for Set and Unset, and r.RLock()/r.RUnlock() for Get and IsSet so reads can
happen concurrently while writes are exclusive; update methods to acquire the
appropriate lock before touching v.val or v.set (or alternatively, if you intend
single-threaded use, add documentation on the non-concurrent requirement instead
of adding a mutex).

---

75-83: Silent type assertion failure returns zero value.

ConfigGet silently returns the zero value if the type assertion fails (line 81). This could mask configuration errors where a value exists but has an unexpected type.

Consider logging or returning an error when the type doesn't match, or document this behaviour clearly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/config.go` around lines 75 - 83, ConfigGet currently silently
returns the zero value when the stored value exists but fails the type
assertion; change ConfigGet to return an error on mismatch instead of hiding it.
Update the signature of ConfigGet[T any] to return (T, error), use e.Get(key) to
distinguish missing vs present, and when val.(T) fails produce and return a
descriptive error (include the key and the expected vs actual types using
reflect) so callers can handle or propagate the error; update any callers of
ConfigGet to handle the new error return.

pkg/core/core.go (1)

57-69: Potential nil pointer dereference in log wrapper methods.

LogError, LogWarn, and Must all call methods on c.log without checking if it's nil. If Core is partially initialised, these will panic.

Consider adding nil guards or ensuring c.log is always initialised (which appears to be the case in New() from the relevant snippets).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/core.go` around lines 57 - 69, The wrapper methods Core.LogError,
Core.LogWarn, and Core.Must call c.log without guarding against a nil c.log; add
a nil-check in each method (or ensure Core.New() always sets c.log) so they
won't panic when Core is partially initialized—e.g., in LogError and LogWarn
return a sensible fallback (return the original err or a wrapped error) if c.log
== nil, and in Must do a no-op panic fallback or call panic with the error if
c.log is nil; update these methods to check c.log before invoking c.log.Error,
c.log.Warn, and c.log.Must respectively (referencing Core, c.log, LogError,
LogWarn, Must, and New()).

tests/core_test.go (1)

78-84: Test uses global state — ensure proper isolation.

This test manipulates the global instance via SetInstance/GetInstance. The defer SetInstance(originalInstance) pattern is good for cleanup. However, if tests run in parallel, this could cause race conditions.

Consider whether this test should be marked with t.Parallel() exclusion or if the global instance pattern should be avoided in favour of dependency injection.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/core_test.go` around lines 78 - 84, This test mutates global state via
GetInstance/SetInstance and must not run concurrently; remove any t.Parallel()
from this test (or ensure it's not added) and protect the
SetInstance/GetInstance sequence with a test-level mutex (e.g.,
globalInstanceMu.Lock()/Unlock()) so the code in the test that does
originalInstance := GetInstance(); SetInstance(c); defer
SetInstance(originalInstance); and assert.Equal(t, app, CoreGUI()) executes
serially and restores the original instance safely.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/core/command.go`:
- Around line 34-46: NewCommand currently leaves c.flags and c.app nil which
lets callers like NewCommand(...).StringFlag(...) or PrintHelp() panic and lets
detached subtrees retain stale commandPath/app; fix by initializing flags (e.g.,
make(map[string]*Flag) or appropriate structure) and setting app to a sensible
default in NewCommand so flags/app are always non-nil, update
NewChildCommandInheritFlags to defensively ensure c.flags exists before using
it, and change AddCommand (and the child-attachment logic inside methods that
set commandPath) to recursively rewire the entire descendant subtree so each
descendant's commandPath and app are updated when a command is attached to a
parent. Ensure you reference NewCommand, NewChildCommandInheritFlags,
AddCommand, StringFlag, PrintHelp, commandPath and app when applying these
fixes.
- Around line 121-136: The current logic checks subcommands before parsing
parent flags which makes flags like "app --verbose child" stay on the parent;
update the dispatch so parent flags are parsed first (call c.parseFlags on
args), then inspect the leftover/remaining args for a subcommand in
c.subCommandsMap and, if present, call subcommand.run with the remaining args;
if parseFlags returns an error keep the existing error handling path
(app.errorHandler / E), and ensure you use the same symbols: c.parseFlags,
c.subCommandsMap, and subcommand.run when rearranging the order so
inherited/global flags are order-insensitive.
- Around line 327-416: The switch cases handling numeric flag default parsing
(e.g., the reflect.Int8/Int16/Int32/Int64 and
reflect.Uint/Uint8/Uint16/Uint32/Uint64 branches in pkg/core/command.go)
currently use strconv.Atoi and then cast, which permits silent
wrapping/out-of-range and rejects large uint64 on 32-bit; update each branch to
use strconv.ParseInt(defaultValue, 10, bitSize) for signed types and
strconv.ParseUint(defaultValue, 10, bitSize) for unsigned types (bitSize =
8/16/32/64 as appropriate), check and return the parse error instead of
panicking, then call field.SetInt or field.SetUint with the parsed value
(converted to int64/uint64). For extra safety you may also validate with
reflect.Value.OverflowInt/OverflowUint before setting; ensure the Int/Uint flag
registration calls (c.Int8Flag, c.Uint64Flag, etc.) remain unchanged.

In `@pkg/core/contract.go`:
- Around line 190-196: WithServiceLock currently calls c.LockEnable() and
c.LockApply() immediately, so its effect depends on option ordering; change it
to set a declarative flag on Core (e.g., c.lockAfterInit = true or similar)
instead of applying locks directly, and update New to apply all Option functions
first and then, after successful option application, check that flag and call
c.LockEnable() and c.LockApply() once; reference WithServiceLock, New, Option,
Core, LockEnable, and LockApply when making this change.
- Around line 115-123: Reject nil callbacks and detect typed-nil service returns
up front in WithService (and mirror the same fix in WithName): first validate
the factory (or callback) argument itself is not nil and return
E("core.WithService", "nil factory callback", nil) (or analogous for WithName);
after calling factory(c) check the returned serviceInstance for both literal nil
and typed-nil using reflection (e.g. use reflect.ValueOf(serviceInstance) and
consider kinds that can be nil: Ptr, Interface, Map, Slice, Chan, Func) and if
nil return a structured error like E("core.WithService", "service factory
returned nil instance", nil). Ensure the same nil-callback and typed-nil checks
and error returns are applied to the WithName implementation (lines referenced
155-160).
- Around line 136-149: In WithService, register the service with the core before
wiring IPC handlers: call the explicit registration API (use c.Service in its
proper error-returning form or replace with c.RegisterService) and handle its
error return immediately instead of storing it in result and type-asserting
any->error; only after successful registration reflect on serviceInstance for
HandleIPCEvents and then call c.RegisterAction(handler). Update the code paths
that currently do the reflect+RegisterAction prior to checking c.Service (the
block using handlerMethod, handler, c.RegisterAction and the subsequent result
:= c.Service(...) type-assert) so you prevent orphaned handlers on
duplicate/locked registrations and avoid misclassifying legitimate service
values that implement error.

In `@pkg/core/embed.go`:
- Around line 68-80: GetAsset has a TOCTOU race because assetGroupsMu.RUnlock()
is called before accessing g.assets; either keep the read lock while reading the
map or copy the byte slice under the lock and then release before calling
decompress. Update GetAsset to RLock, lookup g := assetGroups[group] and data :=
g.assets[name] while holding assetGroupsMu (or copy the data into a new slice),
then RUnlock and call decompress(data); keep error handling via E unchanged.

In `@pkg/core/error.go`:
- Around line 375-391: The Reports method on ErrPan should validate the input n:
explicitly reject negative n (return nil and a descriptive error) and treat n==0
as "no reports requested" by returning an empty []CrashReport and nil error;
implement this at the start of ErrPan.Reports (before reading h.filePath or
slicing), keep the existing behavior for h.filePath=="" and for n greater than
len(reports), and ensure error messages reference ErrPan.Reports and CrashReport
to make diagnostics clear.
- Around line 396-410: appendReport currently can silently drop the crash report
when the target directory doesn't exist and ignores write errors; update
ErrPan.appendReport to ensure the parent directory exists by calling
os.MkdirAll(filepath.Dir(h.filePath), 0o700) before attempting to write, and
stop ignoring the result of os.WriteFile by handling its error (either return an
error from appendReport or log/report it via the existing error-reporting
facility) so failures are visible; make the minimal changes inside
ErrPan.appendReport (use h.filePath, CrashReport, crashMu) to create the dir and
propagate or log write errors.

In `@pkg/core/fs.go`:
- Around line 240-255: The DeleteAll guard must also refuse to delete the
configured sandbox root: after resolving the target path with validatePath (used
in DeleteAll and Delete), compare the resolved full path against m.root (or the
absolute/resolved sandbox root equivalent) and return an error like the existing
E("core.DeleteAll", ...) when they match; keep the existing protections for "/"
and os.Getenv("HOME") intact and apply the same check in Delete if it isn't
already present so the sandbox root cannot be removed via Delete or DeleteAll.
- Around line 49-53: The early return in validatePath when Fs.root == "/"
disables sandbox checks; remove that special-case so validatePath always runs
confinement and symlink resolution logic (use the existing validatePath function
to resolve symlinks and check that the resolved path is within the intended
sandbox), but preserve m.path(p) as the base path resolution helper; ensure
validatePath still enforces the same containment checks even when m.root is "/"
(so update the branch that currently returns m.path(p) to instead call the
normal validation flow and return any error if the resolved path escapes
Fs.root).
- Around line 80-87: Replace the direct stderr write in the sandbox-escape
branch (the fmt.Fprintf(os.Stderr, ...) call and the local username lookup) with
a call into the core security logger so events are emitted as structured logs;
gather the same fields (m.root, p, realNext and the username from
user.Current()) and emit them as named fields/tags (timestamp is handled by the
logger) via the security logger API used elsewhere in the module (use the
project's core security logger instance rather than os.Stderr), then return
os.ErrPermission as before.
- Around line 117-125: The path validation in Fs.validatePath is racy for
mutating ops (WriteMode, Create, Append, Delete, DeleteAll, Rename): you
validate a resolved path and then perform filesystem mutations later, allowing a
TOCTOU symlink swap; fix by performing no-follow, dirfd/openat-style traversal
at the time of the mutation instead of relying on prior validation. Concretely,
update Fs.WriteMode (and the methods Create, Append, Delete, DeleteAll, Rename)
to traverse components using openat/dirfd (or syscall.Open with
O_NOFOLLOW/O_DIRECTORY flags on each parent), create target files with
O_NOFOLLOW|O_CREAT|O_WRONLY via an fd-based syscall, and use
fchown/fchmod/fstat/ftruncate/write on the returned file descriptor (or
atomically verify the final path via fstat), thereby eliminating the window
between validatePath and the actual mutation. Ensure directory creation (current
MkdirAll usage) is performed by creating directories via openat on parent dirfds
without following symlinks, and propagate errors clearly from these fd-based
operations.

In `@pkg/core/lock.go`:
- Around line 13-33: The current Lock function uses package-global lockMu and
lockMap, causing unrelated Core instances to share the same mutex map; move the
mutex map and its guard onto the Core instance and update Core.Lock to use them
instead. Add fields to Core (e.g., lockMu sync.Mutex and lockMap
map[string]*sync.RWMutex), initialize lockMap lazily if nil, then in Core.Lock
use c.lockMu.Lock()/Unlock() and c.lockMap[name] to create/lookup the
*sync.RWMutex and return &Lock{Name: name, Mu: m}; remove or stop using the
package-level lockMu and lockMap globals (refer to symbols: Core.Lock, lockMu,
lockMap, Lock struct).
- Around line 37-57: LockEnable and LockApply accept a lock name but still
toggle the single c.srv.lockEnabled / c.srv.locked flags, which is misleading;
change them to set and check the flags on the specific lock object returned by
c.Lock(name) instead. Concretely, in LockEnable(name ...string) call c.Lock(n)
and set c.Lock(n).lockEnabled = true (not c.srv.lockEnabled), and in
LockApply(name ...string) check c.Lock(n).lockEnabled and set c.Lock(n).locked =
true when enabled; keep the existing Mu.Lock()/Mu.Unlock() usage and the n
selection logic. Ensure you reference the lock instance returned by c.Lock(n)
for both enabling and applying so state is keyed per lock name.

In `@pkg/core/log.go`:
- Around line 237-246: The main log message (msg) is written verbatim which
allows injection via newline/carriage returns; before the final fmt.Fprintf call
that writes "%s %s %s%s\n" (using timestamp, prefix, msg, kvStr) sanitize or
escape msg similarly to the key/value handling (e.g., remove or encode '\n' and
'\r' and other control chars or quote the string) so that msg cannot inject
extra log lines—modify the code around the fmt.Fprintf usage to replace msg with
a safeMsg variable that has control characters escaped or quoted while
preserving intended readable content.
- Around line 303-352: The helpers are dereferencing a stale cached variable
defaultLog (initialized before init()) instead of loading the atomic pointer,
causing nil/logging panics; remove the package-level cached defaultLog or stop
using it and change Default(), SetLevel, SetRedactKeys and all helper functions
(Debug, Info, Warn, Error, Security) to call defaultLogPtr.Load() (or Default()
that itself returns defaultLogPtr.Load()) so they always operate on the current
logger; also update callers that constructed ErrLog/ErrOpts with &ErrOpts{Log:
defaultLog} (e.g., pkg/core/contract.go usage of ErrLog/ErrOpts) to use
Default() or defaultLogPtr.Load() so they don't capture the stale nil pointer.

In `@tests/core_test.go`:
- Line 37: The test calls a non-existent Get method on the Service struct via
c.Service().Get("core"); replace that pattern with the direct lookup call
c.Service("core") to obtain the core service; update the assignment to use svc
:= c.Service("core") (or the existing variable name) and remove the .Get
reference so the test uses the Service() accessor that accepts the service name
rather than calling Get().

---

Minor comments:
In `@CLAUDE.md`:
- Line 75: Update the documentation under "ServiceRuntime Generic Helper
(`runtime_pkg.go`)" to remove references to removed files (`runtime_pkg.go`,
`go-log`, `e.go`) and instead point contributors to the new core surface in
pkg/core; explicitly mention the new error/logging helpers in pkg/core (the
consolidated error and logging APIs) and update the guidance text where similar
references appear (lines ~84-89) so contributors are directed to use the
pkg/core error and logging utilities instead of the deleted files.
- Line 108: The README line currently hard-codes a personal workspace path
(`~/Code/go.work`); remove that path and instead state the Go version and
generic go work instruction. Replace the sentence "Uses Go 1.26 workspaces. This
module is part of the workspace at `~/Code/go.work`." with something like "Uses
Go 1.26 workspaces; add this module to your go.work (no specific local path
required)" so it keeps the Go 1.26/work guidance but drops the
workstation-specific `~/Code/go.work` reference.

In `@pkg/core/cli.go`:
- Around line 97-101: PrintBanner may dereference a nil bannerFunction on Cli;
update PrintBanner to guard against nil by checking c.bannerFunction and falling
back to a default (e.g., defaultBannerFunction) before calling it, or ensure Cli
construction always sets bannerFunction; reference the Cli type and the
PrintBanner method and use defaultBannerFunction as the fallback to avoid the
nil pointer panic.
- Around line 192-194: OtherArgs currently calls c.rootCommand.flags.Args()
without nil checks which can panic; update OtherArgs (method on type Cli) to
guard against nil c, nil c.rootCommand, and nil c.rootCommand.flags and return
an empty slice (or nil) when any are nil, otherwise return
c.rootCommand.flags.Args(); ensure you reference the Cli.OtherArgs method and
the rootCommand and flags fields when adding the nil checks.

In `@pkg/core/config.go`:
- Around line 101-106: The Enabled method can panic if ConfigOpts or Features is
nil; update Config.Enabled to guard against nil by checking e.ConfigOpts and
e.ConfigOpts.Features (or e.Features) before accessing the map: acquire the read
lock (e.mu.RLock()), return false immediately if the feature map is nil,
otherwise read v := e.Features[feature], then release the lock and return v;
reference the Enabled method and the Config.ConfigOpts / Config.Features fields
when making the change.

In `@pkg/core/error.go`:
- Around line 23-30: ErrSink's method signatures (Error/Warn(msg string, keyvals
...any)) don't match the actual exported implementers mentioned in the comment
(ErrLog, ErrPan) and only *Log currently satisfies it; update the contract so it
matches the real types or update the types to match the contract: locate
ErrSink, ErrLog, ErrPan and Log and either change ErrSink's method signatures to
the exact signatures implemented by ErrLog and ErrPan or refactor ErrLog/ErrPan
to implement Error(msg string, keyvals ...any) and Warn(msg string, keyvals
...any); also fix or remove the misleading comment and adjust the type assertion
var _ ErrSink = (*Log)(nil) to reference the correct implementing types (e.g.,
ErrLog/ErrPan) so compile-time checks reflect the intended implementers.

In `@pkg/core/fs.go`:
- Around line 77-79: Tighten the sandbox-escape check around filepath.Rel: in
the code that computes rel := filepath.Rel(m.root, realNext) replace the unsafe
strings.HasPrefix(rel, "..") test with a strict check that only rejects
parent-directory escapes, e.g. check rel == ".." || strings.HasPrefix(rel,
".."+string(os.PathSeparator)); ensure this change is applied where m.root,
realNext and rel are used and keep imports (os, strings) available.

In `@pkg/core/service.go`:
- Around line 41-52: In the registration branch of the default case, replace the
silent type assertion "name, _ := args[0].(string)" with a checked assertion
(e.g., "name, ok := args[0].(string)" and if !ok return an error) so that
non-string first arguments produce a clear type error instead of being treated
as an empty name; update the logic around c.Lock("srv").Mu.Lock() /
c.srv.Services checks to use the validated name after confirming it's a string
and non-empty.
- Around line 32-40: The lookup currently uses a silent type assertion
(args[0].(string) with blank identifier) which yields an empty string on wrong
types; change it to perform an explicit checked assertion like name, ok :=
args[0].(string) and if !ok return nil (or an appropriate error) before
accessing c.srv.Services; keep the existing c.Lock("srv").Mu.RLock()/RUnlock()
usage around the map access (v, ok := c.srv.Services[name]) unchanged.

In `@tests/fuzz_test.go`:
- Line 54: The test harness currently ignores the error return from New() (c, _
:= New()), which can mask setup failures; update both occurrences to capture and
check the error (e.g., c, err := New()) and fail the test immediately on error
(use t.Fatalf or panic in the fuzz function) so constructor failures surface as
setup errors rather than downstream fuzz crashes; ensure both instances that
used the blank error are updated to the same pattern.

In `@tests/runtime_pkg_test.go`:
- Around line 124-125: Comment references the stale method name sr.Cfg() while
the test exercises sr.Config(); update the comment to mention sr.Config() so it
matches the assertion and avoids confusion. Locate the comment near the test
that asserts sr.Config() (search for "We can't directly test" or the use of
sr.Config()) and replace "sr.Cfg()" with "sr.Config()" and adjust wording if
necessary to reflect that sr.Config() will panic without a registered config
service.

---

Nitpick comments:
In @.mcp.json:
- Around line 3-7: Add a short setup note to the repository README or a
contributing/setup doc that explains the "core" config expects the "core-agent"
executable to be discoverable on the system PATH (since "command": "core-agent"
is used with "type": "stdio" and "args": ["mcp"]); include suggested
verification steps (e.g., run `which core-agent` or `core-agent --version`) and
mention updating PATH or installing the agent if not found.

In `@pkg/core/app.go`:
- Around line 38-52: The Find function only fills Name, Filename and Path on the
App struct; document this by adding a concise doc comment above Find stating
that Version, Description, and Runtime are left unpopulated (zero values), or
alternatively rename the function to a clearer name like FindPath or LocateApp
and update any callers accordingly so intent is explicit; reference the Find
function and the App struct when making the change.

In `@pkg/core/array.go`:
- Around line 14-15: NewArray currently stores the caller's backing slice and
AsSlice returns the internal slice directly, allowing external mutation to
bypass protections like AddUnique and Deduplicate; to fix, copy the input
variadic slice into a new slice inside NewArray (and any other
factory/constructor that takes a slice, e.g., the code around the referenced
lines) so the Array has its own backing storage, and change AsSlice to return a
copied slice (not the internal slice pointer) so callers cannot mutate internal
state; update tests if any rely on aliasing.

In `@pkg/core/config.go`:
- Around line 17-24: The ConfigVar methods (ConfigVar, Get, Set, IsSet, Unset)
are not concurrency-safe; add a sync.RWMutex field to the ConfigVar[T] struct
and use it to protect access: use r.Lock()/r.Unlock() for Set and Unset, and
r.RLock()/r.RUnlock() for Get and IsSet so reads can happen concurrently while
writes are exclusive; update methods to acquire the appropriate lock before
touching v.val or v.set (or alternatively, if you intend single-threaded use,
add documentation on the non-concurrent requirement instead of adding a mutex).
- Around line 75-83: ConfigGet currently silently returns the zero value when
the stored value exists but fails the type assertion; change ConfigGet to return
an error on mismatch instead of hiding it. Update the signature of ConfigGet[T
any] to return (T, error), use e.Get(key) to distinguish missing vs present, and
when val.(T) fails produce and return a descriptive error (include the key and
the expected vs actual types using reflect) so callers can handle or propagate
the error; update any callers of ConfigGet to handle the new error return.

In `@pkg/core/core.go`:
- Around line 57-69: The wrapper methods Core.LogError, Core.LogWarn, and
Core.Must call c.log without guarding against a nil c.log; add a nil-check in
each method (or ensure Core.New() always sets c.log) so they won't panic when
Core is partially initialized—e.g., in LogError and LogWarn return a sensible
fallback (return the original err or a wrapped error) if c.log == nil, and in
Must do a no-op panic fallback or call panic with the error if c.log is nil;
update these methods to check c.log before invoking c.log.Error, c.log.Warn, and
c.log.Must respectively (referencing Core, c.log, LogError, LogWarn, Must, and
New()).

In `@pkg/core/embed.go`:
- Around line 522-530: The file currently ignores the error from f.Close() after
writing the template; update the write-and-close sequence (the os.Create call
that returns f and the tmpl.Execute(f, data) usage) to capture and handle close
errors: use a deferred close that captures the error (e.g., defer func(){ if
cerr := f.Close(); err == nil { err = cerr } }()) or check and return the close
error after tmpl.Execute so any filesystem/flush error is returned (ensure you
preserve and wrap the tmpl.Execute error if both occur).
- Around line 584-591: The current write flow in pkg/core/embed.go uses io.Copy
to write to the destination file (variable d) and returns the io.Copy error, but
if io.Copy fails the partial file at target remains; update the logic around
io.Copy(d, s) so that if io.Copy returns a non-nil error you close d (if not
already closed) and remove the partially written file (os.Remove(target)) before
returning the error; reference the variables/function names d, io.Copy and
target to locate and change this behavior (alternatively, explicitly document
that partial files may remain if you choose not to remove them).

In `@pkg/core/i18n.go`:
- Around line 94-103: Update the doc comment for the SetLanguage method on the
I18n type to explicitly state that it is a no-op and returns nil when no
translator is registered; reference the method name SetLanguage and the field
translator so reviewers can find the implementation, and optionally note
intended behavior (e.g., allows deferred translator registration) or suggest
callers check for a registered translator if they need an error instead.

In `@pkg/core/ipc.go`:
- Around line 41-53: The current Core.Query method returns the handler's error
even when handled==true; decide whether a "handled" response should suppress
handler errors or propagate them. If you want handled queries considered
successful, update Core.Query (the Query method on type Core) to return (result,
true, nil) whenever a handler returns handled==true, discarding err; otherwise
add clear documentation to the Query method signature/docstring stating that a
handled result may still include a non-nil error and keep the current behavior.
Ensure the chosen behavior is consistent with QueryAll's error aggregation.

In `@tests/core_test.go`:
- Around line 78-84: This test mutates global state via GetInstance/SetInstance
and must not run concurrently; remove any t.Parallel() from this test (or ensure
it's not added) and protect the SetInstance/GetInstance sequence with a
test-level mutex (e.g., globalInstanceMu.Lock()/Unlock()) so the code in the
test that does originalInstance := GetInstance(); SetInstance(c); defer
SetInstance(originalInstance); and assert.Equal(t, app, CoreGUI()) executes
serially and restores the original instance safely.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5badcdef-40ed-476b-92a4-142923f8a6a4

📥 Commits

Reviewing files that changed from the base of the PR and between 5e7c0e9 and 2406e81.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (46)

• .claude/settings.json
• .mcp.json
• CLAUDE.md
• go.mod
• pkg/core/app.go
• pkg/core/array.go
• pkg/core/cli.go
• pkg/core/command.go
• pkg/core/config.go
• pkg/core/contract.go
• pkg/core/core.go
• pkg/core/e.go
• pkg/core/embed.go
• pkg/core/error.go
• pkg/core/fs.go
• pkg/core/i18n.go
• pkg/core/interfaces.go
• pkg/core/ipc.go
• pkg/core/lock.go
• pkg/core/log.go
• pkg/core/message_bus.go
• pkg/core/message_bus_test.go
• pkg/core/runtime.go
• pkg/core/runtime_pkg.go
• pkg/core/service.go
• pkg/core/service_manager.go
• pkg/core/service_manager_test.go
• pkg/core/task.go
• pkg/log/log.go
• pkg/log/rotation.go
• pkg/log/rotation_test.go
• pkg/log/service.go
• tests/async_test.go
• tests/bench_test.go
• tests/core_extra_test.go
• tests/core_lifecycle_test.go
• tests/core_test.go
• tests/e_test.go
• tests/fuzz_test.go
• tests/ipc_test.go
• tests/message_bus_test.go
• tests/query_test.go
• tests/runtime_pkg_extra_test.go
• tests/runtime_pkg_test.go
• tests/service_manager_test.go
• tests/testdata/test.txt

💤 Files with no reviewable changes (11)

• pkg/log/rotation_test.go
• pkg/core/e.go
• pkg/core/runtime_pkg.go
• pkg/core/service_manager_test.go
• pkg/core/message_bus_test.go
• pkg/core/message_bus.go
• pkg/core/service_manager.go
• pkg/log/rotation.go
• pkg/log/service.go
• pkg/log/log.go
• pkg/core/interfaces.go

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

pkg/core/error.go (2)

376-393: Document behaviour when n <= 0.

The function returns all reports when n <= 0, which may surprise callers expecting an empty slice or an error. Consider clarifying this in the doc comment.

📝 Proposed documentation update

-// Reports returns the last n crash reports from the file.
+// Reports returns the last n crash reports from the file.
+// If n <= 0 or n exceeds the total count, all reports are returned.
 func (h *ErrPan) Reports(n int) ([]CrashReport, error) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 376 - 393, Update the Reports method comment
on ErrPan to explicitly document that passing n <= 0 returns all available
reports (i.e., the full slice) rather than an empty slice or an error; mention
the function's return behavior when h.filePath is empty (nil, nil) and when
len(reports) <= n so callers know edge cases for Reports(n int).

---

347-361: Consider cloning h.meta when building CrashReport.

Line 356 assigns h.meta directly to report.Meta. If the onCrash callback modifies report.Meta, it will mutate the shared h.meta map, affecting subsequent crash reports.

♻️ Proposed fix to clone meta

 	report := CrashReport{
 		Timestamp: time.Now(),
 		Error:     err.Error(),
 		Stack:     string(debug.Stack()),
 		System: CrashSystem{
 			OS:      runtime.GOOS,
 			Arch:    runtime.GOARCH,
 			Version: runtime.Version(),
 		},
-		Meta: h.meta,
+		Meta: maps.Clone(h.meta),
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 347 - 361, CrashReport is being built with
report.Meta pointing directly to h.meta, which allows onCrash (h.onCrash) to
mutate the shared h.meta; fix by cloning h.meta into a new map before assigning
to report.Meta (create a new map with the same type as h.meta and copy each
key/value from h.meta) so report.Meta is independent; update the construction
around CrashReport (where report.Meta = h.meta is set) to use the cloned map and
leave h.meta unchanged when h.onCrash modifies report.Meta.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/core/error.go`:
- Around line 254-280: The ErrLog methods (Error, Warn, Must) can nil-deref
el.Log when created via NewErrLog(nil) or NewErrLog(&ErrOpts{}); update either
NewErrLog to ensure ErrOpts.Log is set to a safe default logger when nil
(reference ErrOpts.Log and NewErrLog) or add a quick nil-check in ErrLog.Error,
ErrLog.Warn and ErrLog.Must that falls back to doing only Wrap/return or uses a
default no-op logger before calling el.Log.Error/el.Log.Warn; modify the
implementation of those methods or NewErrLog accordingly so they never call
methods on a nil el.Log.

---

Nitpick comments:
In `@pkg/core/error.go`:
- Around line 376-393: Update the Reports method comment on ErrPan to explicitly
document that passing n <= 0 returns all available reports (i.e., the full
slice) rather than an empty slice or an error; mention the function's return
behavior when h.filePath is empty (nil, nil) and when len(reports) <= n so
callers know edge cases for Reports(n int).
- Around line 347-361: CrashReport is being built with report.Meta pointing
directly to h.meta, which allows onCrash (h.onCrash) to mutate the shared
h.meta; fix by cloning h.meta into a new map before assigning to report.Meta
(create a new map with the same type as h.meta and copy each key/value from
h.meta) so report.Meta is independent; update the construction around
CrashReport (where report.Meta = h.meta is set) to use the cloned map and leave
h.meta unchanged when h.onCrash modifies report.Meta.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3758b154-3000-4876-849d-66ea5343c9af

📥 Commits

Reviewing files that changed from the base of the PR and between 2406e81 and ead9ea0.

📒 Files selected for processing (3)

• pkg/core/contract.go
• pkg/core/error.go
• pkg/core/log.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/core/log.go

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (3)

pkg/core/error.go (3)

383-399: ⚠️ Potential issue | 🟠 Major

Reports(n) should not treat n <= 0 as “return all”.

Line 396 currently returns all CrashReport entries for invalid input. Please make n < 0 an error and n == 0 an empty result.

🔧 Suggested fix

 func (h *ErrPan) Reports(n int) ([]CrashReport, error) {
 	if h.filePath == "" {
 		return nil, nil
 	}
+	if n < 0 {
+		return nil, fmt.Errorf("ErrPan.Reports: n must be >= 0 for CrashReport retrieval")
+	}
+	if n == 0 {
+		return []CrashReport{}, nil
+	}
 	data, err := os.ReadFile(h.filePath)
 	if err != nil {
 		return nil, err
 	}
@@
-	if n <= 0 || len(reports) <= n {
+	if len(reports) <= n {
 		return reports, nil
 	}
 	return reports[len(reports)-n:], nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 383 - 399, The Reports method on ErrPan
should treat negative n as an error and n==0 as returning an empty slice; update
the Reports(n int) function so that after unmarshalling it returns an error when
n < 0, returns an empty []CrashReport when n == 0, returns all reports when n >=
len(reports), and otherwise returns the last n reports (use the Reports function
name and ErrPan receiver to locate the code).

---

416-419: ⚠️ Potential issue | 🟠 Major

appendReport still suppresses crash persistence failures.

Line 417 and Line 418 ignore filesystem errors. If write fails, recovery appears successful but no report is stored.

🔧 Suggested fix

 	reports = append(reports, report)
 	if data, err := json.MarshalIndent(reports, "", "  "); err == nil {
-		_ = os.MkdirAll(filepath.Dir(h.filePath), 0755)
-		_ = os.WriteFile(h.filePath, data, 0600)
+		if err := os.MkdirAll(filepath.Dir(h.filePath), 0o700); err != nil {
+			Error("ErrPan.appendReport: failed to create crash report directory", "path", filepath.Dir(h.filePath), "err", err)
+			return
+		}
+		if err := os.WriteFile(h.filePath, data, 0o600); err != nil {
+			Error("ErrPan.appendReport: failed to write crash report", "path", h.filePath, "err", err)
+		}
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 416 - 419, The appendReport function
currently swallows filesystem errors when persisting reports; update it to check
and propagate/log errors from os.MkdirAll and os.WriteFile (and handle
json.MarshalIndent errors) instead of ignoring them. Specifically, in
appendReport replace the current if-block that calls json.MarshalIndent(...),
os.MkdirAll(filepath.Dir(h.filePath), 0755) and os.WriteFile(h.filePath, data,
0600) with logic that captures and returns or logs the errors from
json.MarshalIndent, os.MkdirAll and os.WriteFile (referencing
json.MarshalIndent, os.MkdirAll, os.WriteFile, and h.filePath) so callers can
detect persistence failures. Ensure the function signature and callers are
adjusted to handle the returned error if necessary.

---

184-212: ⚠️ Potential issue | 🟠 Major

Root and AllOps do not traverse errors.Join branches.

Line 189 and Line 209 only follow single-error unwrapping, so joined children are skipped and operation traces become incomplete.

In Go's standard library, does errors.Unwrap traverse errors created with errors.Join, or must Unwrap() []error be handled separately?

🔧 Suggested fix

+func unwrapChildren(err error) []error {
+	if err == nil {
+		return nil
+	}
+	if multi, ok := err.(interface{ Unwrap() []error }); ok {
+		return multi.Unwrap()
+	}
+	if one := errors.Unwrap(err); one != nil {
+		return []error{one}
+	}
+	return nil
+}
+
 func Root(err error) error {
 	if err == nil {
 		return nil
 	}
-	for {
-		unwrapped := errors.Unwrap(err)
-		if unwrapped == nil {
-			return err
-		}
-		err = unwrapped
-	}
+	children := unwrapChildren(err)
+	if len(children) == 0 {
+		return err
+	}
+	return Root(children[0])
 }
 
 func AllOps(err error) iter.Seq[string] {
 	return func(yield func(string) bool) {
-		for err != nil {
-			if e, ok := err.(*Err); ok {
-				if e.Op != "" {
-					if !yield(e.Op) {
-						return
-					}
-				}
-			}
-			err = errors.Unwrap(err)
-		}
+		stack := []error{err}
+		for len(stack) > 0 {
+			e := stack[len(stack)-1]
+			stack = stack[:len(stack)-1]
+			if e == nil {
+				continue
+			}
+			if ce, ok := e.(*Err); ok && ce.Op != "" {
+				if !yield(ce.Op) {
+					return
+				}
+			}
+			children := unwrapChildren(e)
+			for i := len(children) - 1; i >= 0; i-- {
+				stack = append(stack, children[i])
+			}
+		}
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/error.go` around lines 184 - 212, Root and AllOps currently only
follow single-error chains via errors.Unwrap and thus miss branches created by
errors.Join; update both to handle multi-child unwrapping by detecting the
Unwrap() []error form (use a local type like interface{ Unwrap() []error } or
similar) and traverse the error tree (depth-first or breadth-first) over all
children, e.g. maintain a stack/queue of errors to visit and for Root return the
first leaf encountered from that traversal, and for AllOps walk the whole tree
yielding every Err.Op found; ensure you still respect nil checks and stop
conditions and avoid infinite loops by not revisiting the same error instance if
necessary.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/core/error.go`:
- Around line 388-399: The Reports function must synchronise with appendReport
to avoid concurrent read/write JSON corruption: acquire the same mutex (crashMu)
before reading and unmarshalling the file, e.g. use crashMu.RLock() and defer
crashMu.RUnlock() around the os.ReadFile + json.Unmarshal block in Reports so
reads are protected the same way appendReport is; ensure you still return the
sliced reports as before after releasing the lock.
- Around line 354-364: The CrashReport is being assigned the shared map h.meta
(used as Meta) which can be mutated by callbacks; instead, create a shallow copy
of h.meta for each report and assign that copy to CrashReport.Meta so each
report has its own map instance (handle nil h.meta safely by leaving Meta nil or
creating an empty map). Locate where report is constructed (the CrashReport
literal using h.meta and CrashSystem) and replace the direct assignment with a
cloned map built by iterating h.meta and copying key/value pairs into a new map
before setting report.Meta.

---

Duplicate comments:
In `@pkg/core/error.go`:
- Around line 383-399: The Reports method on ErrPan should treat negative n as
an error and n==0 as returning an empty slice; update the Reports(n int)
function so that after unmarshalling it returns an error when n < 0, returns an
empty []CrashReport when n == 0, returns all reports when n >= len(reports), and
otherwise returns the last n reports (use the Reports function name and ErrPan
receiver to locate the code).
- Around line 416-419: The appendReport function currently swallows filesystem
errors when persisting reports; update it to check and propagate/log errors from
os.MkdirAll and os.WriteFile (and handle json.MarshalIndent errors) instead of
ignoring them. Specifically, in appendReport replace the current if-block that
calls json.MarshalIndent(...), os.MkdirAll(filepath.Dir(h.filePath), 0755) and
os.WriteFile(h.filePath, data, 0600) with logic that captures and returns or
logs the errors from json.MarshalIndent, os.MkdirAll and os.WriteFile
(referencing json.MarshalIndent, os.MkdirAll, os.WriteFile, and h.filePath) so
callers can detect persistence failures. Ensure the function signature and
callers are adjusted to handle the returned error if necessary.
- Around line 184-212: Root and AllOps currently only follow single-error chains
via errors.Unwrap and thus miss branches created by errors.Join; update both to
handle multi-child unwrapping by detecting the Unwrap() []error form (use a
local type like interface{ Unwrap() []error } or similar) and traverse the error
tree (depth-first or breadth-first) over all children, e.g. maintain a
stack/queue of errors to visit and for Root return the first leaf encountered
from that traversal, and for AllOps walk the whole tree yielding every Err.Op
found; ensure you still respect nil checks and stop conditions and avoid
infinite loops by not revisiting the same error instance if necessary.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 17df75ab-c004-479b-ae43-3200ccc1b7b1

📥 Commits

Reviewing files that changed from the base of the PR and between ead9ea0 and 4fa90a8.

📒 Files selected for processing (1)

• pkg/core/error.go