feat: AX audit + Codex review — polish pass

[Snider]

Summary

• 7 AX audit rounds: 133 violations fixed — eloquent Result literals, semantic naming, no abbreviations, consistent error propagation
• 4 Codex (GPT-5.4) review rounds: 18 → 7 → 9 → 8 findings. Fixed race conditions, path traversal, panic recovery, lifecycle bugs
• Lifecycle context: Core.Context() for cooperative task shutdown
• Result patterns: Result{x, true} positional, Result{}.Result(os.Open(p)) only for Go tuples

AX Changes

• Option.K/V → Key/Value, Err.Op/Msg/Err → Operation/Message/Cause
• ErrCode → ErrorCode, LogPan → LogPanic, Ipc → IPC
• ID → Identifier, BaseDir → BaseDirectory, Opts → Options
• Options.Get, Config.Get, Embed.ReadDir, Translator.Translate → return Result
• Sprint()/Sprintf() wrappers — removed fmt from all non-exempt files

Security Fixes

• embed.Extract: path traversal validation on rendered template paths
• embed.path: rejects .. traversal with Result error
• fs.validatePath: constructs error on sandbox escape (was nil)
• GetAsset: holds RLock through assets map read (race fix)

Lifecycle Fixes

• Core.Context() — cancelled on shutdown, recreated on startup
• ServiceShutdown drains tasks before stopping services
• PerformAsync: panic recovery wraps task execution
• shutdown flag cleared on ServiceStartup (restart support)

Test plan

• 253 tests passing, 78.8% coverage
• go build ./pkg/core/ clean
• go vet ./pkg/core/ clean
• CodeRabbit review

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

Summary by CodeRabbit

• New Features

  • Result-based unified return type across the API; new command registry, Data and Drive subsystems, expanded string/utilities and options primitives; simplified CLI surface.

• Bug Fixes

  • Safer path handling and sandbox checks for embeds/extraction and filesystem operations to prevent directory traversal and unsafe writes.

• Refactor

  • Broad API signature and naming standardisation; consolidated error/logging model and lifecycle/service/runtime simplifications.

• Tests

  • Major test refactor: many focused tests added, several legacy suites removed.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR introduces a universal Result/Options primitive, converts many APIs to return Result, refactors the CLI/command model to a registry-driven design, adds Data and Drive registries, restructures Core internals and service APIs, tightens embed/fs safety, renames error/log types, and updates tests accordingly.

Changes

Cohort / File(s)	Summary
Core primitives & utils pkg/core/options.go, pkg/core/utils.go, pkg/core/string.go	Introduce Result, Option/Options, typed accessors and helpers (Get/Has/String/Int/Bool); add argument, flag and path helpers (FilterArgs, ParseFlag, IsFlag, JoinPath) and string utility wrappers.
Command & CLI refactor pkg/core/command.go, pkg/core/cli.go	Replace tree/reflection CLI with registry-style Command DTO and thread-safe registry on Core; Cli simplified to delegate to Core commands; Run and command execution/lifecycle now use Result.
Data & Drive registries pkg/core/data.go, pkg/core/drive.go	Add Data for mounting/reading embedded filesystems and Drive for named transport handles; both expose New/Get/... methods returning Result.
Result-ification of subsystems pkg/core/embed.go, pkg/core/fs.go, pkg/core/ipc.go, pkg/core/task.go, pkg/core/config.go, pkg/core/i18n.go	Widespread API changes: functions/methods now return Result instead of multi-value (value, ok, error) patterns; control flow updated to propagate Result and handle OK/Value semantics.
Core internals & accessors pkg/core/core.go, pkg/core/contract.go, pkg/core/lock.go, pkg/core/runtime.go	Refactor Core fields and accessors (Options/Data/Drive/Context), remove older Contract/Option factories, rename lock/lifecycle fields, and make lifecycle/startup/shutdown return Result.
Service registry & lifecycle pkg/core/service.go, pkg/core/lock.go	Replace untyped variadic service registry with typed Service(name string, ...) Result; Service holds lifecycle callbacks; add Services() listing and change Startables/Stoppables to Result.
Array, App and helpers pkg/core/array.go, pkg/core/app.go	Array.Filter returns Result; AsSlice returns a defensive copy; Find returns Result instead of *App.
Errors & logging pkg/core/error.go, pkg/core/log.go	Rename/reshape error and logging types and fields (Err→Err fields → Operation/Message/Cause, ErrSink→ErrorSink, ErrLog→ErrorLog etc.); logging and panic-recovery APIs adjusted to Result semantics.
Embed & asset handling pkg/core/embed.go, scanned package types	Embed APIs unified to Result returns; add path-safety and extraction containment checks; rename BaseDir→BaseDirectory; pack/scan functions return Result.
Tests added/removed tests/..., tests/testdata/...	Add extensive new tests aligned to Result-based APIs and new subsystems; remove numerous legacy tests/benchmarks that relied on prior multi-value/error semantics.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as Cli
    participant Core as Core
    participant Reg as CommandRegistry
    participant Cmd as Command
    participant Action as Command.Action

    CLI->>Core: FilterArgs(args)
    CLI->>Core: Core.Commands() / find longest path
    Core->>Reg: lookup(joinedPath)
    Reg-->>Core: *Command or not found
    alt command found
        CLI->>CLI: build Options from remaining args (ParseFlag / _arg)
        CLI->>Cmd: Run(Options)
        Cmd->>Action: invoke Action(opts)
        Action-->>Cmd: Result
        Cmd-->>CLI: Result
    else no command
        CLI->>CLI: PrintHelp() / Banner()
        CLI-->>Caller: Result{OK:false}
    end

Loading

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.81% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The pull request title 'feat: AX audit + Codex review — polish pass' is vague and does not clearly summarise the main changes. It uses non-descriptive terms like 'polish pass' that obscure the actual scope of the work.	Use a more specific title that highlights the primary changes, such as 'refactor: standardise Result-based error handling and API patterns across core package' or similar, to better convey the scope to developers reviewing history.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

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: 16

🧹 Nitpick comments (14)

pkg/core/drive.go (1)

104-112: Consider returning an empty slice instead of nil for consistency.

When handles is nil, Names() returns nil. Returning []string{} would be more consistent with the pattern where "no items" is an empty collection rather than nil, avoiding potential nil-check burden on callers.

Suggested improvement

 func (d *Drive) Names() []string {
 	d.mu.RLock()
 	defer d.mu.RUnlock()
-	var names []string
+	names := make([]string, 0, len(d.handles))
 	for k := range d.handles {
 		names = append(names, k)
 	}
 	return names
 }

🤖 Prompt for AI Agents

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

In `@pkg/core/drive.go` around lines 104 - 112, The Names method can return nil
when d.handles is nil; change it to always return an empty slice by initializing
names before the loop (e.g., names := make([]string, 0, len(d.handles))) so
callers get []string{} instead of nil; update the Names function in type Drive
(which uses d.mu and d.handles) to allocate the names slice up-front and then
append as now, retaining the RLock/RUnlock.

pkg/core/string.go (1)

68-77: Join implementation note: quadratic for many parts.

The current implementation calls Concat iteratively, which is acceptable for typical use cases (small number of path segments). For scenarios with many parts, consider using strings.Join internally. However, given the documented "hook point" purpose of Concat, this trade-off seems intentional.

🤖 Prompt for AI Agents

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

In `@pkg/core/string.go` around lines 68 - 77, Replace the O(n^2) loop in Join by
delegating to strings.Join for better performance: import the standard "strings"
package and return strings.Join(parts, sep) (while keeping Concat available
elsewhere); update the Join function (currently calling Concat in the loop) to
use strings.Join(parts, sep) to avoid repeated allocations for many parts.

tests/testdata/cli_clir.go.bak (1)

1-11: Clarify the purpose of this .bak file.

This backup file contains compilation errors (type mismatches, undefined functions). If it's meant as reference implementation for the new CLI, consider either fixing the errors or removing it to avoid confusion.

🤖 Prompt for AI Agents

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

In `@tests/testdata/cli_clir.go.bak` around lines 1 - 11, This .bak file
(cli_clir.go.bak, package core) appears to be a broken backup with compilation
errors and should not live as a .go file; either remove it from the repo or
convert it to a non-Go reference (rename to .txt or move to a docs/tests
directory) so Go tooling won’t attempt to compile it, or if you intend it as a
working reference fix the issues by running go build/go vet on cli_clir.go.bak,
correct the type mismatches and undefined functions inside this file (fix any
symbols referenced there) and rename it back to .go when it compiles cleanly.

tests/cli_test.go (1)

65-69: Consider renaming to TestCli_Run_NoCommand_Bad for consistency.

This test verifies that Run() without arguments returns OK=false, which is an expected error/failure condition. Per coding guidelines, _Bad suffix is for expected error conditions.

Proposed rename

-func TestCli_Run_NoCommand_Good(t *testing.T) {
+func TestCli_Run_NoCommand_Bad(t *testing.T) {

🤖 Prompt for AI Agents

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

In `@tests/cli_test.go` around lines 65 - 69, Rename the test function
TestCli_Run_NoCommand_Good to TestCli_Run_NoCommand_Bad to reflect that it
asserts an expected failure (Run() returns OK=false); update the function
declaration and any references or test runner calls that mention
TestCli_Run_NoCommand_Good so the test continues to compile and run, e.g.,
change the function name in tests/cli_test.go and any other places importing or
referencing that symbol.

tests/config_test.go (1)

69-74: Consider adding _Good suffix for naming consistency.

Per coding guidelines, tests should use _Good, _Bad, or _Ugly suffixes.

Proposed rename

-func TestConfig_Features_CaseSensitive(t *testing.T) {
+func TestConfig_Features_CaseSensitive_Good(t *testing.T) {

🤖 Prompt for AI Agents

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

In `@tests/config_test.go` around lines 69 - 74, Rename the test function
TestConfig_Features_CaseSensitive to follow the project naming convention by
appending the "_Good" suffix (e.g., TestConfig_Features_CaseSensitive_Good);
update any references to the test name if present, and keep the body unchanged
(it should still call New(), c.Config().Enable("Feature"), and assert
c.Config().Enabled("Feature") true and c.Config().Enabled("feature") false).

pkg/core/lock.go (4)

41-43: Consider caching the Lock reference to avoid redundant map lookups.

c.Lock(n) is called twice (once for Lock(), once for defer Unlock()), each triggering a map lookup and allocation of a new Lock struct. While functionally correct (the underlying mutex is shared), caching the reference is more efficient.

Proposed fix

 func (c *Core) LockEnable(name ...string) {
 	n := "srv"
 	if len(name) > 0 {
 		n = name[0]
 	}
-	c.Lock(n).Mutex.Lock()
-	defer c.Lock(n).Mutex.Unlock()
+	lk := c.Lock(n)
+	lk.Mutex.Lock()
+	defer lk.Mutex.Unlock()
 	c.services.lockEnabled = true
 }

🤖 Prompt for AI Agents

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

In `@pkg/core/lock.go` around lines 41 - 43, Cache the result of c.Lock(n) into a
local variable (e.g., l := c.Lock(n)) and use that cached reference for both
l.Mutex.Lock() and defer l.Mutex.Unlock() instead of calling c.Lock(n) twice;
this avoids redundant map lookups and allocations while preserving the same
locking behavior and still sets c.services.lockEnabled = true after acquiring
the lock.

---

52-56: Same optimisation opportunity in LockApply.

Proposed fix

 func (c *Core) LockApply(name ...string) {
 	n := "srv"
 	if len(name) > 0 {
 		n = name[0]
 	}
-	c.Lock(n).Mutex.Lock()
-	defer c.Lock(n).Mutex.Unlock()
+	lk := c.Lock(n)
+	lk.Mutex.Lock()
+	defer lk.Mutex.Unlock()
 	if c.services.lockEnabled {
 		c.services.locked = true
 	}
 }

🤖 Prompt for AI Agents

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

In `@pkg/core/lock.go` around lines 52 - 56, The LockApply code currently calls
c.Lock(n) twice (once for Lock() and again for Unlock() via defer) and
redundantly accesses c.Lock(n).Mutex; store the result of c.Lock(n) in a local
variable (e.g., l := c.Lock(n)), call l.Mutex.Lock(), and use defer
l.Mutex.Unlock() so the same lock instance is used; also set c.services.locked =
true conditionally after acquiring the lock (as done with lockEnabled) to match
the existing pattern and avoid repeated lookups.

---

64-65: Same optimisation opportunity in Startables.

Proposed fix

-	c.Lock("srv").Mutex.RLock()
-	defer c.Lock("srv").Mutex.RUnlock()
+	lk := c.Lock("srv")
+	lk.Mutex.RLock()
+	defer lk.Mutex.RUnlock()

🤖 Prompt for AI Agents

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

In `@pkg/core/lock.go` around lines 64 - 65, The current code calls c.Lock("srv")
twice (once to RLock and once to RUnlock) causing redundant map lookups; change
both occurrences to capture the lock in a local variable (e.g., lock :=
c.Lock("srv")) and then call lock.Mutex.RLock() with defer lock.Mutex.RUnlock();
apply the same change in the Startables usage so both call sites use a single
local lock variable instead of repeated c.Lock("srv") lookups.

---

80-81: Same optimisation opportunity in Stoppables.

Proposed fix

-	c.Lock("srv").Mutex.RLock()
-	defer c.Lock("srv").Mutex.RUnlock()
+	lk := c.Lock("srv")
+	lk.Mutex.RLock()
+	defer lk.Mutex.RUnlock()

🤖 Prompt for AI Agents

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

In `@pkg/core/lock.go` around lines 80 - 81, The code repeatedly calls
c.Lock("srv").Mutex.RLock()/RUnlock(), which re-fetches the lock and can be
optimized in the Stoppables code path; instead fetch the lock (e.g., l :=
c.Lock("srv")), call l.Mutex.RLock(), and defer l.Mutex.RUnlock() so the same
Mutex instance is used; update the Stoppables usage to follow this pattern
(replace direct c.Lock("srv").Mutex.RLock()/defer c.Lock("srv").Mutex.RUnlock()
with a single held variable).

tests/core_test.go (1)

23-27: Test naming inconsistent with behaviour.

The test is named _Bad but the comment states "should still create a valid Core" — this is a successful outcome, not an error condition. Per the coding guidelines, _Bad should be used for expected error conditions.

🔧 Rename to follow convention

-func TestNew_WithOptions_Bad(t *testing.T) {
-	// Empty options — should still create a valid Core
+func TestNew_WithEmptyOptions_Good(t *testing.T) {
+	// Empty options — should still create a valid Core
 	c := New(Options{})
 	assert.NotNil(t, c)
 }

As per coding guidelines: "Use test naming convention with _Good, _Bad, _Ugly suffixes: _Good for happy path tests, _Bad for expected error conditions".

🤖 Prompt for AI Agents

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

In `@tests/core_test.go` around lines 23 - 27, Rename the test function
TestNew_WithOptions_Bad to TestNew_WithOptions_Good and update its comment to
reflect a happy-path assertion (e.g., "Empty options — should still create a
valid Core"); adjust any references to TestNew_WithOptions_Bad (callers or test
filters) so they point to TestNew_WithOptions_Good to comply with the
`_Good`/`_Bad` naming convention used for New and Options-related tests.

tests/i18n_test.go (1)

24-26: Improve test robustness by asserting setup success.

If c.Data().New(...) fails, the test silently skips adding locales but then asserts Len(..., 1), which would produce a confusing failure message. Consider failing fast if setup fails.

🔧 Suggested improvement

-	if r.OK {
-		c.I18n().AddLocales(r.Value.(*Embed))
-	}
+	assert.True(t, r.OK, "Data().New should succeed")
+	c.I18n().AddLocales(r.Value.(*Embed))

🤖 Prompt for AI Agents

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

In `@tests/i18n_test.go` around lines 24 - 26, The test currently continues when
c.Data().New(...) fails (checking only r.OK before using r.Value), so add a
fail-fast assertion: if r.OK is false, call t.Fatalf (or require.Truef) with the
failure details (include r.Error or similar) immediately after calling
c.Data().New to stop the test and surface the setup error; only call
c.I18n().AddLocales(r.Value.(*Embed)) when r.OK is true. Ensure you reference
the result variable r and the setup call c.Data().New (and the subsequent use in
c.I18n().AddLocales) in your change.

tests/error_test.go (2)

215-229: Test has unused variables and unclear purpose.

The test creates a path variable that is never used meaningfully, and the assertions test default behaviour (no crash file configured) rather than actual crash file functionality.

🔧 Clean up or clarify the test

 func TestErrorPanic_CrashFile_Good(t *testing.T) {
-	dir := t.TempDir()
-	path := dir + "/crashes.json"
-
-	// Create Core, trigger a panic through SafeGo, check crash file
-	// ErrorPanic.filePath is unexported — but we can test via the package-level
-	// error handling that writes crash reports
-
-	// For now, test that Reports handles missing file gracefully
 	c := New()
+	// Test that Reports handles missing/unconfigured crash file gracefully
 	r := c.Error().Reports(5)
 	assert.False(t, r.OK)
 	assert.Nil(t, r.Value)
-	_ = path
 }

🤖 Prompt for AI Agents

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

In `@tests/error_test.go` around lines 215 - 229, TestErrorPanic_CrashFile_Good
contains an unused variable `path` and doesn't exercise crash-file behavior;
either remove `path` and adjust assertions to reflect you're testing default
behavior, or update the test to configure and exercise the crash file by setting
the crash filepath (via the package's error configuration or by manipulating
ErrorPanic.filePath through exported API), then trigger a panic with SafeGo and
call New().Error().Reports(5) to assert the report is written and returned
(check r.OK/r.Value accordingly); locate this logic around the
TestErrorPanic_CrashFile_Good function and the New(), Error(), Reports methods
to implement the cleanup or the actual crash-file assertions.

---

200-211: Test is a stub with no meaningful assertions.

This test creates a Core instance and declares a path but doesn't actually test crash reporting behaviour. The comments indicate this is intentional due to unexported fields, but the test should either be completed or removed to avoid giving false coverage confidence.

🔧 Consider completing or removing the stub

Either implement the test properly by triggering a crash via SafeGo and reading reports, or remove the test until the functionality can be tested:

 func TestErrorPanic_Reports_Good(t *testing.T) {
-	dir := t.TempDir()
-	path := dir + "/crashes.json"
-
-	// Create ErrorPanic with file output
-	c := New()
-	// Access internals via a crash that writes to file
-	// Since ErrorPanic fields are unexported, we test via Recover
-	_ = c
-	_ = path
-	// Crash reporting needs ErrorPanic configured with filePath — tested indirectly
+	t.Skip("Crash reporting requires ErrorPanic configuration - tested indirectly via integration tests")
 }

🤖 Prompt for AI Agents

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

In `@tests/error_test.go` around lines 200 - 211, The test
TestErrorPanic_Reports_Good is a stub and should be completed or removed; either
implement it to exercise ErrorPanic crash reporting by configuring a real file
path (path "/crashes.json"), creating the crash reporter via New(), launching a
goroutine that panics using SafeGo (or call Recover directly around a panic),
wait for reporting to flush, then read and assert the contents of the file for
expected crash metadata, or delete the test entirely if coverage for
ErrorPanic/Recover cannot be done reliably; locate symbols New(), ErrorPanic,
Recover, SafeGo and the path variable to make the change.

pkg/core/data.go (1)

99-113: Consider documenting that single-segment paths are unsupported.

The resolve method requires paths in "mount/relative" format and returns (nil, "") for single-segment paths. This is a valid design choice, but callers might expect "brain" to resolve to the root of the brain mount.

💡 Optional: Add clarifying comment

 // resolve splits a path like "brain/coding.md" into mount name + relative path.
+// Single-segment paths (e.g., "brain") are not supported; use Get() to access the mount directly.
 func (d *Data) resolve(path string) (*Embed, string) {

🤖 Prompt for AI Agents

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

In `@pkg/core/data.go` around lines 99 - 113, Document that Data.resolve expects
"mount/relative" paths and intentionally returns (nil, "") for single-segment
inputs; update the comment above the resolve method (function Data.resolve) to
state that paths must contain a mount and a relative path, that single-segment
names like "brain" do not resolve to a mount root, and mention the returned
values (nil, "") on invalid input so callers know to handle that case.

🤖 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/cli.go`:
- Around line 133-144: The help loop is printing registry placeholder commands
that aren't executable; update the loop over cl.core.commands.commands to skip
entries where the command has no executable behavior by checking both cmd.Action
and cmd.Lifecycle (i.e., if cmd.Action == nil && cmd.Lifecycle == nil then
continue), in addition to the existing Hidden check and still use
cmd.I18nKey()/tr.Value as before.
- Around line 97-103: The loop currently converts an explicit empty value
"--name=" into a boolean true; change the logic to preserve explicit empty
values by checking whether the original arg contained "=". In the block that
handles valid ParseFlag(arg) results, if strings.Contains(arg, "=") then append
Option{Key: key, Value: val} (even when val == ""), otherwise keep the current
behavior (if val != "" append the value, else append true). Update imports to
include the strings package if needed and reference ParseFlag, opts, and arg
when making the change.

In `@pkg/core/command.go`:
- Around line 73-78: The Run method on Command currently returns an empty Result
when cmd.Action is nil which silently hides placeholder parent commands; change
Command.Run to fail fast by returning a Result containing an error (with a
descriptive message including the command identifier such as cmd.Name or
cmd.Path if present) when cmd.Action == nil, instead of Result{}; apply the same
change to the other Run implementation referenced (the one at lines ~169-181) so
both places return an error Result for nil Action, and update any callers/tests
to expect an error Result rather than a no-op.
- Around line 147-149: The duplicate-registration guard only checks
existing.Action, so a command that has existing.Lifecycle set can be
overwritten; update the check in the registration logic (the branch using
c.commands.commands[path] and existing) to treat any non-nil existing.Action OR
non-nil existing.Lifecycle as “already registered” (e.g., replace the condition
existing.Action != nil with (existing.Action != nil || existing.Lifecycle !=
nil)) so lifecycle-backed commands are protected from being re-registered.

In `@pkg/core/embed.go`:
- Around line 241-250: The code uses TrimPrefix(file, groupPath+"/") which fails
on Windows due to mixed path separators; replace that TrimPrefix call by
computing the relative path via filepath.Rel (e.g., rel, err :=
filepath.Rel(groupPath, file)) and then normalize both relGroup and the computed
local path to forward slashes with filepath.ToSlash before writing the packed
asset key in the b.WriteString line that calls core.AddAsset; keep the existing
error handling around compressFile and filepath.Rel(pkg.BaseDirectory,
groupPath) but remove any manual "/" concatenation when producing localPath.
- Around line 163-170: Replace the manual TrimPrefix/TrimSuffix handling of
ast.BasicLit.Value in pkg/core/embed.go with proper Go string literal decoding
using strconv.Unquote: for the code paths that extract path and group from
call.Args (the blocks using call.Args[len(call.Args)-1] and call.Args[0]
respectively), call strconv.Unquote on the BasicLit.Value (handle the returned
string and error) and use the unquoted result as the path/group; apply the same
change to the other occurrence around the later block (the second BasicLit
handling mentioned in the comment). Ensure strconv is imported and handle/unwrap
the error (e.g. fallback to the original Value sans quotes if Unquote fails)
before passing values into ScanAssets.
- Around line 369-375: The path builder Embed.path currently joins the input
then checks for traversal, which can be bypassed since filepath.Join normalizes
".."; instead validate the relative path before joining by using
fs.ValidPath(name) (reject when it returns false, which covers "..", "/" and
null bytes), returning the same Result error (E("embed.path", ...)) if invalid;
only after fs.ValidPath passes should you compute joined :=
filepath.ToSlash(filepath.Join(s.basedir, name)) and return it.

In `@pkg/core/error.go`:
- Around line 349-362: The Reports method in ErrorPanic returns raw
os.ReadFile/json.Unmarshal errors without file context; update
ErrorPanic.Reports to wrap both I/O and JSON errors with the E() error helper
(from e.go) and include the crash file path and the operation (“read crash file”
/ “unmarshal crash file”) in the error metadata so callers know which file and
step failed (use h.filePath and reference the functions ErrorPanic.Reports,
os.ReadFile, and json.Unmarshal when constructing the E(...) wrapped errors
before returning Result).
- Around line 139-144: AllOperations (and the helper functions StackTrace and
FormatStackTrace) only traverse linear error chains with errors.Unwrap, so they
miss branches created by ErrorJoin (which uses errors.Join); update their
traversal to detect the multiple-unwrap form: check for the Unwrap() []error
method (e.g., via a local interface type like interface{ Unwrap() []error })
and, when present, iterate each branch recursively to collect operations/frames,
otherwise fall back to the single-error errors.Unwrap loop; keep ErrorJoin as-is
but ensure these functions perform a depth-first traversal over both single
unwraps and join branches so all joined errors are reachable by StackTrace and
FormatStackTrace.

In `@pkg/core/fs.go`:
- Around line 264-266: The DeleteAll function currently silently returns an
empty Result when the resolved path (variable full) is "/" or equals
os.Getenv("HOME"); change it to mirror Delete by returning an explicit error
instead of Result{} for protected paths. Update DeleteAll to detect full == "/"
|| full == os.Getenv("HOME") and return an error value (with contextual message
like "refusing to delete root or home directory") alongside the appropriate
Result or error type used by the function signature so callers can handle the
protected-path failure; reference the DeleteAll function and the same
protected-path check already used in Delete to implement this behavior.
- Around line 248-250: The current check that returns an empty Result{} when
full == "/" or full == os.Getenv("HOME") should instead return a Result that
contains an explicit error (e.g., Result{Err: fmt.Errorf("refusing to delete
protected path %q", full)}), so callers get diagnostics; update the deletion
function where the variable full is inspected to return that error value and add
the fmt/errors import if needed.

In `@pkg/core/runtime.go`:
- Around line 72-90: The shutdown loop currently only treats OnStop failures as
errors when r.Value implements error, which ignores non-error payloads; update
the loop that iterates stoppables := c.Stoppables() and handles s.OnStop() so
that any Result with !r.OK is recorded into firstErr: if r.Value is an error use
it, otherwise wrap the payload (including nil) into a new error (e.g. via
fmt.Errorf with descriptive context like "OnStop failed: %v") and assign that to
firstErr; ensure subsequent logic still returns Result{firstErr, false} when
firstErr is set.
- Around line 102-130: The factories currently take no arguments and are assumed
to return a concrete Service, which removes Core injection and prevents
pointer/typed service initialization; change the ServiceFactory signature to
accept the Core (e.g. type ServiceFactory func(*Core) Result) and in
NewWithFactories call factory(c) (instead of factory()) so factories can
initialize ServiceRuntime[T] and read config; then, after a successful Result,
register the returned instance directly via c.Service(name, r.Value) without
casting to Service, and preserve the existing error handling (use r.Value as the
instance and return failures when r.OK is false).

In `@tests/testdata/cli_clir.go.bak`:
- Around line 234-239: The NewChildCommand method calls an undefined NewCommand;
replace that call with the correct factory newClirCommand so it constructs the
child correctly. In clirCommand.NewChildCommand, change the instantiation line
to use newClirCommand(name, description...) (preserving the variadic description
handling), keep the subsequent c.AddCommand(result) and return result so
behavior is unchanged.
- Around line 39-52: The function newClirCommand is constructing a value of type
Command but its signature expects *clirCommand, causing a type mismatch; update
the constructor to return a *clirCommand (e.g., build and return &clirCommand{
name: name, shortdescription: desc, subCommandsMap:
make(map[string]*clirCommand), hidden: false, positionalArgsMap:
make(map[string]reflect.Value), sliceSeparator: make(map[string]string) }) or
change the function signature to return *Command so the types align; ensure you
still call result.setParentCommandPath("") on the returned clirCommand instance
and return that value.

In `@tests/testdata/scantest/sample.go`:
- Around line 5-7: The code incorrectly unpacks two return values from
core.GetAsset but GetAsset returns a single Result; change the call in function
example to capture the single return (e.g., res := core.GetAsset("mygroup",
"myfile.txt")) and then access the Result's fields or methods (or check its
error/status) instead of expecting a separate error return; update usage in
example() to work with the Result type returned by GetAsset.

---

Nitpick comments:
In `@pkg/core/data.go`:
- Around line 99-113: Document that Data.resolve expects "mount/relative" paths
and intentionally returns (nil, "") for single-segment inputs; update the
comment above the resolve method (function Data.resolve) to state that paths
must contain a mount and a relative path, that single-segment names like "brain"
do not resolve to a mount root, and mention the returned values (nil, "") on
invalid input so callers know to handle that case.

In `@pkg/core/drive.go`:
- Around line 104-112: The Names method can return nil when d.handles is nil;
change it to always return an empty slice by initializing names before the loop
(e.g., names := make([]string, 0, len(d.handles))) so callers get []string{}
instead of nil; update the Names function in type Drive (which uses d.mu and
d.handles) to allocate the names slice up-front and then append as now,
retaining the RLock/RUnlock.

In `@pkg/core/lock.go`:
- Around line 41-43: Cache the result of c.Lock(n) into a local variable (e.g.,
l := c.Lock(n)) and use that cached reference for both l.Mutex.Lock() and defer
l.Mutex.Unlock() instead of calling c.Lock(n) twice; this avoids redundant map
lookups and allocations while preserving the same locking behavior and still
sets c.services.lockEnabled = true after acquiring the lock.
- Around line 52-56: The LockApply code currently calls c.Lock(n) twice (once
for Lock() and again for Unlock() via defer) and redundantly accesses
c.Lock(n).Mutex; store the result of c.Lock(n) in a local variable (e.g., l :=
c.Lock(n)), call l.Mutex.Lock(), and use defer l.Mutex.Unlock() so the same lock
instance is used; also set c.services.locked = true conditionally after
acquiring the lock (as done with lockEnabled) to match the existing pattern and
avoid repeated lookups.
- Around line 64-65: The current code calls c.Lock("srv") twice (once to RLock
and once to RUnlock) causing redundant map lookups; change both occurrences to
capture the lock in a local variable (e.g., lock := c.Lock("srv")) and then call
lock.Mutex.RLock() with defer lock.Mutex.RUnlock(); apply the same change in the
Startables usage so both call sites use a single local lock variable instead of
repeated c.Lock("srv") lookups.
- Around line 80-81: The code repeatedly calls
c.Lock("srv").Mutex.RLock()/RUnlock(), which re-fetches the lock and can be
optimized in the Stoppables code path; instead fetch the lock (e.g., l :=
c.Lock("srv")), call l.Mutex.RLock(), and defer l.Mutex.RUnlock() so the same
Mutex instance is used; update the Stoppables usage to follow this pattern
(replace direct c.Lock("srv").Mutex.RLock()/defer c.Lock("srv").Mutex.RUnlock()
with a single held variable).

In `@pkg/core/string.go`:
- Around line 68-77: Replace the O(n^2) loop in Join by delegating to
strings.Join for better performance: import the standard "strings" package and
return strings.Join(parts, sep) (while keeping Concat available elsewhere);
update the Join function (currently calling Concat in the loop) to use
strings.Join(parts, sep) to avoid repeated allocations for many parts.

In `@tests/cli_test.go`:
- Around line 65-69: Rename the test function TestCli_Run_NoCommand_Good to
TestCli_Run_NoCommand_Bad to reflect that it asserts an expected failure (Run()
returns OK=false); update the function declaration and any references or test
runner calls that mention TestCli_Run_NoCommand_Good so the test continues to
compile and run, e.g., change the function name in tests/cli_test.go and any
other places importing or referencing that symbol.

In `@tests/config_test.go`:
- Around line 69-74: Rename the test function TestConfig_Features_CaseSensitive
to follow the project naming convention by appending the "_Good" suffix (e.g.,
TestConfig_Features_CaseSensitive_Good); update any references to the test name
if present, and keep the body unchanged (it should still call New(),
c.Config().Enable("Feature"), and assert c.Config().Enabled("Feature") true and
c.Config().Enabled("feature") false).

In `@tests/core_test.go`:
- Around line 23-27: Rename the test function TestNew_WithOptions_Bad to
TestNew_WithOptions_Good and update its comment to reflect a happy-path
assertion (e.g., "Empty options — should still create a valid Core"); adjust any
references to TestNew_WithOptions_Bad (callers or test filters) so they point to
TestNew_WithOptions_Good to comply with the `_Good`/`_Bad` naming convention
used for New and Options-related tests.

In `@tests/error_test.go`:
- Around line 215-229: TestErrorPanic_CrashFile_Good contains an unused variable
`path` and doesn't exercise crash-file behavior; either remove `path` and adjust
assertions to reflect you're testing default behavior, or update the test to
configure and exercise the crash file by setting the crash filepath (via the
package's error configuration or by manipulating ErrorPanic.filePath through
exported API), then trigger a panic with SafeGo and call
New().Error().Reports(5) to assert the report is written and returned (check
r.OK/r.Value accordingly); locate this logic around the
TestErrorPanic_CrashFile_Good function and the New(), Error(), Reports methods
to implement the cleanup or the actual crash-file assertions.
- Around line 200-211: The test TestErrorPanic_Reports_Good is a stub and should
be completed or removed; either implement it to exercise ErrorPanic crash
reporting by configuring a real file path (path "/crashes.json"), creating the
crash reporter via New(), launching a goroutine that panics using SafeGo (or
call Recover directly around a panic), wait for reporting to flush, then read
and assert the contents of the file for expected crash metadata, or delete the
test entirely if coverage for ErrorPanic/Recover cannot be done reliably; locate
symbols New(), ErrorPanic, Recover, SafeGo and the path variable to make the
change.

In `@tests/i18n_test.go`:
- Around line 24-26: The test currently continues when c.Data().New(...) fails
(checking only r.OK before using r.Value), so add a fail-fast assertion: if r.OK
is false, call t.Fatalf (or require.Truef) with the failure details (include
r.Error or similar) immediately after calling c.Data().New to stop the test and
surface the setup error; only call c.I18n().AddLocales(r.Value.(*Embed)) when
r.OK is true. Ensure you reference the result variable r and the setup call
c.Data().New (and the subsequent use in c.I18n().AddLocales) in your change.

In `@tests/testdata/cli_clir.go.bak`:
- Around line 1-11: This .bak file (cli_clir.go.bak, package core) appears to be
a broken backup with compilation errors and should not live as a .go file;
either remove it from the repo or convert it to a non-Go reference (rename to
.txt or move to a docs/tests directory) so Go tooling won’t attempt to compile
it, or if you intend it as a working reference fix the issues by running go
build/go vet on cli_clir.go.bak, correct the type mismatches and undefined
functions inside this file (fix any symbols referenced there) and rename it back
to .go when it compiles cleanly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d18986b5-6c50-4a4b-8de3-43d5a9cd44ea

📥 Commits

Reviewing files that changed from the base of the PR and between 3ee5857 and e17217a.

📒 Files selected for processing (56)

• 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/data.go
• pkg/core/drive.go
• pkg/core/embed.go
• pkg/core/error.go
• pkg/core/fs.go
• pkg/core/i18n.go
• pkg/core/ipc.go
• pkg/core/lock.go
• pkg/core/log.go
• pkg/core/options.go
• pkg/core/runtime.go
• pkg/core/service.go
• pkg/core/string.go
• pkg/core/task.go
• pkg/core/utils.go
• tests/app_test.go
• tests/array_test.go
• tests/async_test.go
• tests/bench_test.go
• tests/cli_test.go
• tests/command_test.go
• tests/config_test.go
• tests/core_extra_test.go
• tests/core_lifecycle_test.go
• tests/core_test.go
• tests/data_test.go
• tests/drive_test.go
• tests/e_test.go
• tests/embed_test.go
• tests/error_test.go
• tests/fs_test.go
• tests/fuzz_test.go
• tests/i18n_test.go
• tests/ipc_test.go
• tests/lock_test.go
• tests/log_test.go
• tests/message_bus_test.go
• tests/options_test.go
• tests/query_test.go
• tests/runtime_pkg_extra_test.go
• tests/runtime_pkg_test.go
• tests/runtime_test.go
• tests/service_manager_test.go
• tests/service_test.go
• tests/string_test.go
• tests/task_test.go
• tests/testdata/cli_clir.go.bak
• tests/testdata/scantest/sample.go
• tests/utils_test.go

💤 Files with no reviewable changes (11)

• tests/runtime_pkg_extra_test.go
• tests/service_manager_test.go
• tests/bench_test.go
• tests/core_lifecycle_test.go
• tests/message_bus_test.go
• tests/query_test.go
• tests/e_test.go
• tests/runtime_pkg_test.go
• tests/async_test.go
• tests/core_extra_test.go
• tests/fuzz_test.go

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

pkg/core/error.go (2)

349-367: ⚠️ Potential issue | 🟡 Minor

Wrap crash-report I/O errors with file context.

Returning the raw os.ReadFile/json.Unmarshal error provides no context about which crash report file failed or at which step. As per coding guidelines, "All errors MUST use E() from go-log".

🛠️ Suggested fix

 func (h *ErrorPanic) Reports(n int) Result {
 	if h.filePath == "" {
 		return Result{}
 	}
 	crashMu.Lock()
 	defer crashMu.Unlock()
 	data, err := os.ReadFile(h.filePath)
 	if err != nil {
-		return Result{err, false}
+		return Result{E("core.ErrorPanic.Reports", Concat("read crash reports from ", h.filePath), err), false}
 	}
 	var reports []CrashReport
 	if err := json.Unmarshal(data, &reports); err != nil {
-		return Result{err, false}
+		return Result{E("core.ErrorPanic.Reports", Concat("decode crash reports from ", h.filePath), err), false}
 	}

,

🤖 Prompt for AI Agents

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

In `@pkg/core/error.go` around lines 349 - 367, The Reports method on ErrorPanic
returns raw errors from os.ReadFile and json.Unmarshal; wrap these with go-log's
E() and include operation and file context (h.filePath) before returning so
callers get actionable messages. Update ErrorPanic.Reports to replace direct err
returns from the read and unmarshal steps with wrapped errors via E(err,
"reading crash report", "file", h.filePath) and E(err, "parsing crash report
JSON", "file", h.filePath) (or similar), keeping the same Result shape and
preserving crashMu locking and existing control flow.

---

196-211: ⚠️ Potential issue | 🟠 Major

Traverse joined error branches in AllOperations.

ErrorJoin creates multi-branch error trees via errors.Join, but AllOperations uses only linear errors.Unwrap traversal. When an error is joined (e.g., ErrorJoin(err1, err2, err3)), the joined branches are unreachable to StackTrace and FormatStackTrace.

Consider checking for the Unwrap() []error interface to handle multi-branch traversal:

🛠️ Suggested fix

 func AllOperations(err error) iter.Seq[string] {
 	return func(yield func(string) bool) {
-		for err != nil {
-			if e, ok := err.(*Err); ok {
-				if e.Operation != "" {
-					if !yield(e.Operation) {
-						return
-					}
-				}
-			}
-			err = errors.Unwrap(err)
-		}
+		var walk func(error) bool
+		walk = func(err error) bool {
+			if err == nil {
+				return true
+			}
+			if e, ok := err.(*Err); ok && e.Operation != "" {
+				if !yield(e.Operation) {
+					return false
+				}
+			}
+			switch u := err.(type) {
+			case interface{ Unwrap() []error }:
+				for _, child := range u.Unwrap() {
+					if !walk(child) {
+						return false
+					}
+				}
+			case interface{ Unwrap() error }:
+				return walk(u.Unwrap())
+			}
+			return true
+		}
+		_ = walk(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 196 - 211, AllOperations currently walks the
error chain using only errors.Unwrap (linear path) so branches produced by
ErrorJoin are missed; update AllOperations to detect and handle multi-error
unwrapping by checking for the interface Unwrap() []error (or using errors.As to
a type exposing []error), and recursively traverse each branch collecting
operations from *Err nodes (same check for e.Operation != "") while preserving
early-yield stop behavior; ensure this unified traversal is used by StackTrace
and FormatStackTrace call sites or returns the same sequence shape they expect.

🧹 Nitpick comments (2)

pkg/core/fs.go (2)

68-70: Consider wrapping symlink evaluation errors with context.

Line 69 returns the raw filepath.EvalSymlinks error without wrapping it using E(). For consistency with the coding guidelines and to provide better context to callers, consider wrapping this error.

💡 Suggested improvement

 		if err != nil {
 			if os.IsNotExist(err) {
 				// Part doesn't exist, we can't follow symlinks anymore.
 				// Since the path is already Cleaned and current is safe,
 				// appending a component to current will not escape.
 				current = next
 				continue
 			}
-			return Result{err, false}
+			return Result{E("fs.validatePath", Concat("symlink evaluation failed: ", next), err), false}
 		}

🤖 Prompt for AI Agents

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

In `@pkg/core/fs.go` around lines 68 - 70, The returned error from
filepath.EvalSymlinks is being returned raw as Result{err, false}; update this
to wrap the error using the project's error helper E() (e.g., E(err, "evaluating
symlink" or similar) ) so callers receive contextualized errors; locate the
return that constructs Result with the raw err (near the EvalSymlinks call) and
replace the raw err with the wrapped error while keeping the Result boolean
unchanged.

---

94-104: Type assertions are safe but rely on internal contract.

The code consistently uses vp.Value.(string) after checking vp.OK. This is safe given validatePath always returns a string in Value when OK is true. Consider whether a typed helper method on Result (e.g., String()) might improve safety if the contract changes in future.

🤖 Prompt for AI Agents

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

In `@pkg/core/fs.go` around lines 94 - 104, The Read method on Fs currently uses a
type assertion vp.Value.(string) after validatePath returns OK; to harden
against future contract changes add a typed accessor on Result (e.g., func (r
Result) String() (string, bool) or (string, error)) and update Fs.Read to call
that accessor instead of a direct assertion; modify validatePath to continue
populating Result.Value as before but callers like Fs.Read should use
Result.String() (and adjust other callers of validatePath/Result) so the
conversion logic and safety live on the Result type rather than scattered
assertions.

🤖 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/cli.go`:
- Line 109: Replace the direct call to cmd.Run(opts) with cmd.Start(opts) so
lifecycle-backed commands are executed via the lifecycle path; update the return
in the function where cmd.Run is currently called to return cmd.Start(opts)
(Start will still call Run for plain commands but will properly handle commands
that implement Lifecycle).

In `@pkg/core/command.go`:
- Around line 140-172: The registration currently only rejects an entirely empty
path but allows paths with empty segments (e.g. "/deploy", "deploy/",
"deploy//to"), which results in blank Command.Name and broken CLI resolution; in
the command registration function (around the code that calls Split(path, "/")
and sets cmd.Name = pathName(path)), validate the incoming path by splitting on
"/" and returning a Result error (use E("core.Command", ...)) if any segment is
empty or if the path has leading/trailing slashes or consecutive slashes;
perform this check before creating cmd and before assigning cmd.Name so only
well-formed segments reach the parent-chain logic and c.commands.commands map
insertion.

In `@pkg/core/error.go`:
- Around line 139-144: The doc comment for ErrorJoin has a typo reference to
core.CauseorJoin; update the comment to reference the correct symbol
core.ErrorJoin (or simply ErrorJoin) so the example and comment match the
function name ErrorJoin in this file; ensure the comment line above the
ErrorJoin function is corrected accordingly.

---

Duplicate comments:
In `@pkg/core/error.go`:
- Around line 349-367: The Reports method on ErrorPanic returns raw errors from
os.ReadFile and json.Unmarshal; wrap these with go-log's E() and include
operation and file context (h.filePath) before returning so callers get
actionable messages. Update ErrorPanic.Reports to replace direct err returns
from the read and unmarshal steps with wrapped errors via E(err, "reading crash
report", "file", h.filePath) and E(err, "parsing crash report JSON", "file",
h.filePath) (or similar), keeping the same Result shape and preserving crashMu
locking and existing control flow.
- Around line 196-211: AllOperations currently walks the error chain using only
errors.Unwrap (linear path) so branches produced by ErrorJoin are missed; update
AllOperations to detect and handle multi-error unwrapping by checking for the
interface Unwrap() []error (or using errors.As to a type exposing []error), and
recursively traverse each branch collecting operations from *Err nodes (same
check for e.Operation != "") while preserving early-yield stop behavior; ensure
this unified traversal is used by StackTrace and FormatStackTrace call sites or
returns the same sequence shape they expect.

---

Nitpick comments:
In `@pkg/core/fs.go`:
- Around line 68-70: The returned error from filepath.EvalSymlinks is being
returned raw as Result{err, false}; update this to wrap the error using the
project's error helper E() (e.g., E(err, "evaluating symlink" or similar) ) so
callers receive contextualized errors; locate the return that constructs Result
with the raw err (near the EvalSymlinks call) and replace the raw err with the
wrapped error while keeping the Result boolean unchanged.
- Around line 94-104: The Read method on Fs currently uses a type assertion
vp.Value.(string) after validatePath returns OK; to harden against future
contract changes add a typed accessor on Result (e.g., func (r Result) String()
(string, bool) or (string, error)) and update Fs.Read to call that accessor
instead of a direct assertion; modify validatePath to continue populating
Result.Value as before but callers like Fs.Read should use Result.String() (and
adjust other callers of validatePath/Result) so the conversion logic and safety
live on the Result type rather than scattered assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 42c125d5-45fa-49f6-bc3d-c375aa044200

📥 Commits

Reviewing files that changed from the base of the PR and between e17217a and af6b618.

📒 Files selected for processing (5)

• pkg/core/cli.go
• pkg/core/command.go
• pkg/core/error.go
• pkg/core/fs.go
• pkg/core/runtime.go

[coderabbitai]

♻️ Duplicate comments (1)

pkg/core/error.go (1)

349-367: ⚠️ Potential issue | 🟡 Minor

Wrap crash-report I/O errors with file context.

The Reports method returns raw os.ReadFile and json.Unmarshal errors without wrapping them with E(). This omits crucial context about which file failed and at which step.

🛠️ Suggested fix

 data, err := os.ReadFile(h.filePath)
 if err != nil {
-    return Result{err, false}
+    return Result{E("core.ErrorPanic.Reports", Concat("read crash reports from ", h.filePath), err), false}
 }
 var reports []CrashReport
 if err := json.Unmarshal(data, &reports); err != nil {
-    return Result{err, false}
+    return Result{E("core.ErrorPanic.Reports", Concat("decode crash reports from ", h.filePath), err), false}
 }

As per coding guidelines, "All errors MUST use E() from go-log (re-exported in e.go), never use fmt.Errorf".

🤖 Prompt for AI Agents

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

In `@pkg/core/error.go` around lines 349 - 367, The Reports method in ErrorPanic
should wrap I/O and JSON errors with the project's E() helper and include file
context; change the two error returns in ErrorPanic.Reports (the os.ReadFile
error and the json.Unmarshal error) to return Result{E(err, "reading crash
file", "file", h.filePath), false} and Result{E(err, "parsing crash reports",
"file", h.filePath), false} respectively so callers get E()-wrapped errors with
the file path and step included while keeping the same Result shape.

🧹 Nitpick comments (1)

pkg/core/error.go (1)

383-394: Consider structured logging for crash report errors.

The error logging uses string concatenation. For better observability and log analysis, consider using the key-value logging pattern that's already used elsewhere in the codebase.

♻️ Suggested improvement

 data, err := json.MarshalIndent(reports, "", "  ")
 if err != nil {
-    Default().Error(Concat("crash report marshal failed: ", err.Error()))
+    Default().Error("crash report marshal failed", "path", h.filePath, "err", err)
     return
 }
 if err := os.MkdirAll(filepath.Dir(h.filePath), 0755); err != nil {
-    Default().Error(Concat("crash report dir failed: ", err.Error()))
+    Default().Error("crash report dir creation failed", "path", filepath.Dir(h.filePath), "err", err)
     return
 }
 if err := os.WriteFile(h.filePath, data, 0600); err != nil {
-    Default().Error(Concat("crash report write failed: ", err.Error()))
+    Default().Error("crash report write failed", "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 383 - 394, The current crash-report error
handling uses string concatenation via Default().Error(Concat(...)); update each
error log after json.MarshalIndent(reports,...),
os.MkdirAll(filepath.Dir(h.filePath), ...), and os.WriteFile(h.filePath, data,
...) to use the repository's key-value structured logging pattern instead of
Concat—include the error value (err) and relevant context keys like "filePath":
h.filePath and "reportsCount": len(reports) (or similar) in the log call so
errors are emitted as structured fields rather than concatenated strings.

🤖 Prompt for all review comments with AI agents

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

Duplicate comments:
In `@pkg/core/error.go`:
- Around line 349-367: The Reports method in ErrorPanic should wrap I/O and JSON
errors with the project's E() helper and include file context; change the two
error returns in ErrorPanic.Reports (the os.ReadFile error and the
json.Unmarshal error) to return Result{E(err, "reading crash file", "file",
h.filePath), false} and Result{E(err, "parsing crash reports", "file",
h.filePath), false} respectively so callers get E()-wrapped errors with the file
path and step included while keeping the same Result shape.

---

Nitpick comments:
In `@pkg/core/error.go`:
- Around line 383-394: The current crash-report error handling uses string
concatenation via Default().Error(Concat(...)); update each error log after
json.MarshalIndent(reports,...), os.MkdirAll(filepath.Dir(h.filePath), ...), and
os.WriteFile(h.filePath, data, ...) to use the repository's key-value structured
logging pattern instead of Concat—include the error value (err) and relevant
context keys like "filePath": h.filePath and "reportsCount": len(reports) (or
similar) in the log call so errors are emitted as structured fields rather than
concatenated strings.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 07371f7f-a094-443e-9aba-34563543ea30

📥 Commits

Reviewing files that changed from the base of the PR and between af6b618 and 73eed89.

📒 Files selected for processing (3)

• pkg/core/cli.go
• pkg/core/command.go
• pkg/core/error.go

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

• pkg/core/cli.go
• pkg/core/command.go