Deep-audit remediation + sandbox hardening (batches 1–35)#189
Merged
Conversation
…paction estimate - secrets.Redact: replace longest matches first so a containing PRIVATE KEY block is redacted before a nested shorter match corrupts its exact string (audit medium internal/secrets/scanner.go:81) + regression test. - agent compaction estimateTokens: add a flat per-image token cost so an image-heavy context trends toward compaction instead of reading as ~0 (audit medium internal/agent/compaction.go:58) + test. - ledger: re-verify against current main; mark scanner.go:35 and redaction.go:362 already-fixed-on-main, record this batch's fixes.
Capture git stdout and stderr into separate buffers instead of CombinedOutput, so parsed rev-parse output is not polluted by git warnings and CommandResult.Stderr carries the real error text (audit medium internal/worktrees/worktrees.go:247) + test.
Independent agent per file re-checked each open/partial finding against the actual current code (the 06-11 ledger was verified on a since-diverged branch), with an adversarial second pass on every already_fixed verdict. Authoritative result: 78 genuinely pending (66 still_open + 12 partial); 9 rows were already fixed on main and are reclassified. Full table in 2026-06-13-reverification.md.
…x, compaction usage, dedup - loop.go: check ctx.Err() before collected.Error so a cancelled run returns context.Canceled (errors.Is holds) instead of a plain stringified error. - loop.go: 'Always allow' with no sandbox engine now allows THIS call instead of denying it (persistence is skipped when there's nowhere to persist). - loop.go: remove a duplicate schema["items"] assignment in propertyToRuntimeMap. - compaction.go: forward OnUsage (token accounting) through the summarizer while still omitting OnText, so compaction's token cost is counted in usage/budgets. - tests for all three behavioral fixes (each fails pre-fix).
…, surface session-record failures - exec --list-tools -o json now emits a JSON tool listing instead of falling through to the human-readable text (audit medium exec.go:182) + test. - interrupted run with -o json now emits a terminal error+done on stdout instead of only printing to stderr, so a JSON consumer sees a clean end (medium exec.go:500) + test. - execSessionRecorder: surface a latched session-append failure to stderr once at run end (deferred) instead of silently dropping it (medium exec_sessions.go:114/115) + test. - left app.go trailing-arg ignore as-is: documented-intentional, TUI has no argv prompt.
…hs); document env-drop on plain Runner - parseStatus: switch `git status` to `--porcelain -z` and parse the NUL-delimited form (audit medium zerogit.go:212). Previously the default --short output was split on newlines, so a rename `R old -> new` was kept as one bogus path and a non-ASCII/whitespace filename arrived C-quoted+escaped (e.g. `"caf\303\251.txt"`). The -z form never quotes paths and emits a rename/copy as `XY <dest>\0<src>`, so we now record the destination verbatim and consume the source field. + TestParseStatusZHandlesRenamesAndSpecialPaths. - resolveRunners: document that the plain-Runner EnvRunner adapter intentionally drops env (audit low zerogit.go:417). The branch is reached only when a caller supplies a custom Runner without a RunGitEnv — in practice fake-Runner tests. Production leaves both nil and gets defaultRunGit/defaultRunGitEnv, which honor GIT_INDEX_FILE, so snapshot-diff isolation holds. Routing env calls to real git instead would break the fake-Runner test pattern and fix nothing in production. - update Inspect/Commit mock tests to the -z status format and the new command.
…gap helpers - FileConfig.UnmarshalJSON now rejects a negative maxTurns instead of letting the `MaxTurns > 0` merge gates silently drop it and fall back to the default, hiding the misconfiguration (audit medium resolver.go:136). 0 is left as the "unset" sentinel (omitempty) and still falls back to the default; the CLI --max-turns flag continues to reject 0 too since there it is distinguishable from absent. + TestResolveRejectsNegativeMaxTurns / ...ZeroFallsBackToDefault. - remove dead config/contracts.go (ContractGap, DefaultContractGaps, FindContractGapsByMilestone, ContractOwnerRuntime) and its test — referenced only by that test, never by production (audit low contracts.go:12). - NOT changed: ResolveMCP intentionally does not run the provider command (TestResolveMCPDoesNotRunProviderCommand guards this), so the "ZERO_PROVIDER_COMMAND MCP servers dropped" row is by-design, not a defect.
connectStdio attached a plain bytes.Buffer as cmd.Stderr that is only ever read on initialize failure, yet it kept growing for the entire process lifetime as a long-lived server logged to stderr (audit medium client.go:98). Replace it with a concurrency-safe boundedBuffer capped at 64 KiB that keeps the head (enough for init-error diagnostics) and discards the rest, reporting full write lengths so os/exec never sees a short write. + TestBoundedBufferCapsRetainedBytes. Left as-is (not defects worth the risk in the MCP client): - client.go writer.write under client.mu: already designed to release the lock before the unbounded response wait; the residual concern is a server that stops draining stdin, an involved async rewrite for a theoretical deadlock. - readLoop not answering server-initiated requests (ping): responding safely needs re-locking around the shared writer; no configured server relies on it.
…ad provider branch - read_file: when a max_lines cut occurs, append an explicit "[truncated: N more line(s)... set start_line=X to continue]" marker to the output. Previously only the Result.Truncated flag was set, which is invisible in the rendered text, so the model could not distinguish a cut read from a complete one (audit low read_file.go:89). + TestReadFileToolMarksTruncation. - providers/factory: simplify `if !explicitProvider || isImplicitOpenAI(...)` to `if !explicitProvider` and delete the now-unused isImplicitOpenAI. The clause was dead: explicitProvider==true implies ProviderKind or Provider is set, but isImplicitOpenAI required both empty, so it could never contribute a case (audit low factory.go:118). Behavior-preserving; providers tests still green.
…no N+1 disk scans)
Store.Tree recursed via ListChildren per node, and each ListChildren ran a full
store.List() that re-read every metadata.json from disk — O(nodes * total-sessions)
reads for one tree (audit medium lineage.go:140). Snapshot all sessions once, index
children by parent in memory, and recurse over that. Child ordering is factored into
sortChildSessions so ListChildren and Tree stay identical; path-scoped cycle
detection (delete(seen,...)) is preserved. Existing tree/lineage tests still pass.
Left as-is: store.go timestamp() second precision (RFC3339) — switching to
RFC3339Nano would break lexical UpdatedAt ordering against existing second-precision
records ("...00.5Z" sorts before "...00Z"), so it is not a safe one-line change.
recordSpecialistStop and appendSpecialistUsageRollup checked for an existing event (specialistEventExists → ReadEvents) and then appended under SEPARATE store locks, so two concurrent finishers — a foreground onExit racing a TaskOutput poll, or a background reaper — could both pass the dedup check and write duplicate stop/usage events (audit medium accounting.go:72). Guard each check-then-append pair with a package-level accountingMu (Executor is used by value, so a struct field would not be shared). + TestRecordSpecialistStopDedupesUnderConcurrency (passes under -race).
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
…ached-token counts - zeroruntime CollectStreamWithOptions accumulated assistant text with `collected.Text += event.Content`, reallocating the whole string on every chunk (O(n^2) over a long streamed response). Accumulate in a strings.Builder on the collector and materialize it once in flush, the single finalization point hit before every return (audit low helpers.go:104). Existing collect tests still pass. - gemini decoded usageMetadata without cachedContentTokenCount, so a cached-prompt response reported zero cached input tokens while the other providers report them (audit medium gemini/types.go:88). Decode the field and thread it through streamState into NormalizeUsage as CachedInputTokens. + extended TestStreamCompletionEmitsTextUsageAndReasoningTokens.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughLarge coordinated update: token/accounting and summarization plumbing, agent loop/tool wiring, sandbox policy/enforcement (seccomp, denial monitoring, runner), CLI output and JSON protocols, background process-group termination, NUL git status parsing, sessions durability, web tool sandbox gating, secrets redaction, and extensive tests/docs. ChangesIntegrated runtime, sandbox, CLI, and tooling updates
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/audit/2026-06-13-reverification.md (1)
22-24: ⚡ Quick winConsolidate or label the repeated
cron_run.gorows.These three lines describe the same fixed root cause. If the
9total is meant to mean unique findings, the duplicates are misleading; if it means ledger rows, say that explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/audit/2026-06-13-reverification.md` around lines 22 - 24, Consolidate the three duplicate audit rows that all describe the same fix (cronTruncate now using cutRuneBoundary which ensures UTF-8 rune boundaries) into a single row referencing cronTruncate and cutRuneBoundary (and optionally noting promptExcerpt) and adjust the ledger/counting language to clarify whether the total “9” is unique findings or ledger entries; alternatively, keep the three rows but add a duplicate/alias label on the two repeats to indicate they reference the same root cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audit/2026-06-10-deep-audit-status.md`:
- Line 1: Update the stale heading line "# Deep-audit status ledger
(critical/high re-verified against HEAD on branch fix/audit-critical-high,
2026-06-11)" so it matches the body: change the date to 2026-06-13 and the
branch to fix/audit-batch (i.e., "# Deep-audit status ledger (critical/high
re-verified against HEAD on branch fix/audit-batch, 2026-06-13)"), leaving the
rest of the file unchanged.
In `@internal/config/types.go`:
- Around line 211-213: The validation error message for raw.MaxTurns is
inconsistent with the check: keep the existing condition that rejects only
values < 0 (i.e., allow 0) but update the error string in the function that
returns fmt.Errorf(...) to say "must be >= 0" (or equivalent) instead of "must
be greater than 0", referencing the raw.MaxTurns check in
internal/config/types.go so users aren't misled.
---
Nitpick comments:
In `@docs/audit/2026-06-13-reverification.md`:
- Around line 22-24: Consolidate the three duplicate audit rows that all
describe the same fix (cronTruncate now using cutRuneBoundary which ensures
UTF-8 rune boundaries) into a single row referencing cronTruncate and
cutRuneBoundary (and optionally noting promptExcerpt) and adjust the
ledger/counting language to clarify whether the total “9” is unique findings or
ledger entries; alternatively, keep the three rows but add a duplicate/alias
label on the two repeats to indicate they reference the same root cause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cca5870a-c523-4f94-bafb-9ff6b83aec31
📒 Files selected for processing (32)
docs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/providers/factory.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.go
💤 Files with no reviewable changes (2)
- internal/config/contracts.go
- internal/config/contracts_test.go
… cap; drop dead helper - ParseStream used a bufio.Scanner capped at 1 MiB, so one large stream-json line from a child (a big tool result or final answer) hit bufio.ErrTooLong and aborted the entire specialist run (audit medium streamer.go:43). Switch to a bufio.Reader with ReadString, which has no per-line limit; the child is our own trusted subprocess, so its output length is the legitimate bound. Line numbering and blank-line/EOF handling are preserved. + TestParseStreamHandlesLineLargerThan1MiB. - remove dead formatTaskOutput (output_tool.go:244): zero callers and zero tests; it only forwarded to formatTaskOutputSummary, which is the one actually used.
…ocess group Background children were launched in the parent's process group and terminated by signalling only the single PID, so any process the task forked was orphaned and outlived a cancel; the signal also risked hitting a recycled PID after the child was reaped (audit medium process_posix.go:38/50). - ConfigureChildProcessGroup (Setpgid on POSIX, no-op on Windows where taskkill /T already tree-kills) is applied at launch so the child leads its own group. - terminateProcess now SIGTERM/SIGKILLs the negative PID (the whole group), with a pid>1 guard so a bogus 0/1 can never expand to "our own group" or "everything", and treats ESRCH as already-gone. Liveness polls the group. Targeting a group (rarely recycled vs. a bare PID) also shrinks the reaped-then-recycled window. - specialist launchBackgroundProcess opts every child into its own group; on a SetPID failure it now kills the child + group, marks the task errored, and records the stop (deduped) instead of leaking an untracked orphan. - tests launch as group leaders; new TestTerminateProcessKillsForkedChildren proves a forked grandchild dies with the group.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/background/process_posix.go`:
- Around line 66-70: The code currently sends SIGKILL with syscall.Kill(-pid,
syscall.SIGKILL) but returns immediately; update the helper to re-run the same
liveness polling used earlier (call processGroupAlive(pid) in a loop with the
same deadline/interval) after sending SIGKILL and only return nil once
processGroupAlive(pid) is false, or return a descriptive error if the group
still exists after the second deadline; keep the existing error handling with
processGoneError(err) for the Kill call but treat a successful Kill as just the
escalation step, not success, until liveness polling confirms termination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d41bf0a2-1625-473b-a669-dbee19021a5b
📒 Files selected for processing (5)
internal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/specialist/exec.go
✅ Files skipped from review due to trivial changes (1)
- internal/background/process_windows.go
splitShellSegments used a quote-unaware strings.Replacer, so a shell operator inside quotes was treated as a separator: `git commit -m "use top | less"` split into a `less` segment and `echo "a; vim b"` into a `vim` segment, both falsely flagged as interactive (audit medium safe_command.go:138). Replace it with a quote-aware scanner that mirrors the shell: single quotes make everything literal; double quotes keep $(...)/`...` substitution active but treat |, ;, && as literal; unquoted text splits on the same operator set as before (;, |, ||, &&) plus substitution boundaries. Real unquoted operators still split, so an interactive program behind a separator — or inside a live `"$(...)"` substitution — is still caught (no new false negatives, the security-relevant direction). + TestDetectInteractiveQuoteAwareSeparators covering both directions.
gnanam1990
marked this pull request as draft
June 13, 2026 08:29
On a fall-back day the clock repeats an hour (e.g. 01:30 EDT then 01:30 EST), so
Next("30 1 * * *", <01:30 EDT>) returned the same-day 01:30 EST — firing a daily
job twice (audit medium schedule.go:171, which previously guarded only the
spring-forward gap).
Next now skips a match whose local wall-clock minute equals `after`'s but whose
absolute time is later (the repeated-hour duplicate) and keeps searching. Normal
forward search always advances the wall-clock, so this only triggers on the
fall-back repeat; when the scheduler passes the last fire time as `after`, the
repeated hour collapses to a single fire. + TestNextDSTFallBackDoesNotRepeatHour
(spring-forward termination test still passes).
Note: Next is stateless, so it dedupes relative to `after`; a caller that passes
an arbitrary mid-hour "now" instead of the last fire time is out of scope here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/safe_command.go`:
- Around line 486-492: The parser currently treats ')' as a split point
unconditionally; add tracking of active `$(` nesting by introducing an integer
depth counter (e.g., dollarParenDepth) that is incremented where you detect `c
== '$' && i+1 < len(runes) && runes[i+1] == '('` (after you consume the '(') and
decremented when you see `c == ')'`; only call flush() and treat `)` as a
boundary when the counter is > 0 (and decrement it), otherwise treat `)` as a
literal character (append to the current buffer) so things like `echo "a) less"`
are not split into fake segments; update the two switch branches that reference
runes, i, and flush() to use this depth logic and ensure the counter is
initialized to 0 in the surrounding parsing function/state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6af37025-dde1-472c-b3d9-966bb85f8ead
📒 Files selected for processing (2)
internal/sandbox/safe_command.gointernal/sandbox/safe_command_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cron/schedule.go`:
- Around line 191-203: The DST repeat guard in Next is over-filtering: change
the condition so we only skip the candidate repeated-hour time when it's not
strictly after `after`. Concretely, in the block that calls
sameWallClockMinute(t, after) use the combined check "sameWallClockMinute(t,
after) && !t.After(after)" to advance t by one minute; otherwise return t. This
preserves the strict-`after` semantics while still collapsing the duplicate
repeated hour when the candidate isn't a later instant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92a6e73b-3633-4e5d-8144-3266184bc426
📒 Files selected for processing (2)
internal/cron/next_test.gointernal/cron/schedule.go
…ger SSE buffer - decodeSSERPCMessage returned the first non-empty `message` event unconditionally, so a server-initiated notification/request arriving before the response made the caller see an id mismatch and fail (audit medium network_client.go:621). It now skips any message carrying a method (notifications/requests have one; a response does not) and returns the first real response; the caller's rpcIDMatches still validates the id. + TestDecodeSSERPCMessageSkipsNotifications. - raise the per-event SSE scanner cap from 1 MiB to a bounded 8 MiB so a large but legitimate MCP message no longer hits bufio.ErrTooLong and fails the request (audit medium network_client.go:637); still bounded against an unbounded server. Out of scope (kept): full reconnect of a persistent SSE stream after a fatal read error — a larger change than this remediation.
…ename writeMetadata wrote the temp file with os.WriteFile (no sync) and renamed it into place, so a crash right after the rename could expose a metadata file whose bytes were never flushed — a torn or empty file that corrupts the whole session (audit medium store.go:632). Write+fsync the temp via a new writeFileSync helper before renaming, closing that window. + TestWriteFileSyncRoundTrips. Deliberately scoped: - Event-log appends (appendEventLocked) are NOT fsynced per append: that would add a disk flush to every event (every tool call), and a hard-crash loss of the log's unsynced tail is acceptable, whereas torn METADATA is not. Metadata is the integrity-critical part and is what this syncs. - Directory fsync is omitted for cross-platform simplicity (it errors on Windows); the file fsync already prevents the torn-contents failure, the serious case. Left for follow-up (in this bucket but not safe/clear wins here): - hooks append sequence still re-reads the file per append; the in-memory cache is cross-process-unsafe and the safe last-line read is fiddly for a low-volume log. - Fork event re-append/de-dup is nuanced usage-accounting and needs its own change.
gnanam1990
changed the title
… deny an always-allow on persist failure - maybeCompact's estimate omitted the tool definitions, which ride on every request, so the real input could exceed the model limit while the message-only estimate stayed under threshold and compaction never fired (audit medium compaction.go:58). partitionTools is now computed before maybeCompact (it does not depend on the messages) and the exposed tools' estimated tokens are added to both the threshold check and the post-compaction shrink check. + estimateToolDefTokens and TestEstimateToolDefTokensCountsDefinitions. - PermissionDecisionAlwaysAllow denied the call when persistPermissionGrant failed, overriding what the user explicitly allowed (audit medium loop.go:468). Persisting is now best-effort: a failure (or no sandbox engine) just means the grant is not remembered and the user is re-prompted next time, not denied now. This also drops the dead requestEvent.GrantMatched/Grant writes (audit low loop.go:473) — the emitted event derives them from the sandbox decision via buildPermissionEvent. Left intentional (documented, not defects): the reactive-retry path omits OnText to avoid duplicating mid-stream text (loop.go:183); OnContext/MeasureContext is a complete, tested context-breakdown scaffold awaiting a consumer (types.go:183).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/agent/compaction.go (1)
375-388: 💤 Low value
lowWaterMarkinrecoverexcludes tool tokens, inconsistent withmaybeCompact.
recoversetslowWaterMark = estimateTokens(result)without tool tokens (line 386), butmaybeCompactcomparessize = estimateTokens(messages) + toolTokensagainstlowWaterMark(line 318). After a reactive compaction, the next turn's proactive check may seesize > lowWaterMarksimply becausesizeincludes tools andlowWaterMarkdoesn't—potentially triggering a redundant compaction attempt.Practical impact is low since reactive compaction fires at most once per run and the worst case is one extra no-op compaction call, but the estimate should stay consistent.
Suggested fix
Have
recoveraccepttools []zeroruntime.ToolDefinitionand include tool tokens:func (state *compactionState) recover( ctx context.Context, provider Provider, messages []zeroruntime.Message, + tools []zeroruntime.ToolDefinition, errorMessage string, ) (compacted []zeroruntime.Message, retried bool, err error) { ... state.reactiveAttempted = true - state.lowWaterMark = estimateTokens(result) + state.lowWaterMark = estimateTokens(result) + estimateToolDefTokens(tools) return result, true, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/compaction.go` around lines 375 - 388, The reactive recovery path in recover currently sets state.lowWaterMark = estimateTokens(result) without accounting for toolTokens, which is inconsistent with maybeCompact (which compares size = estimateTokens(messages) + toolTokens against lowWaterMark) and can cause a redundant proactive compaction; fix by changing recover to accept tools []zeroruntime.ToolDefinition (or equivalent source of tool token counts) and set state.lowWaterMark = estimateTokens(result) + computeToolTokens(tools) so lowWaterMark and maybeCompact use the same token accounting, and update all callers of recover to pass the same tools slice (or propagate a toolTokens int) accordingly.internal/agent/loop.go (1)
574-586: Keep “Always allow” persistence best-effort (never gate the approved call)In
internal/agent/loop.go,PermissionDecisionAlwaysAllowsetspermissionGranted=true, callspersistPermissionGrantonly whenoptions.Sandbox != nil, and discards any error (_, _ = ...). So a persistence failure can only affect whether FUTURE calls skip the prompt; it won’t deny the user’s approved action for THIS call.Optional: consider emitting debug/telemetry when
persistPermissionGrantfails, since users wouldn’t otherwise see why they got re-prompted later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 574 - 586, The "Always allow" branch currently honours the permission for this call but swallows persistence errors; keep the behavior (set permissionGranted = true and only call persistPermissionGrant when options.Sandbox != nil) but don’t silently discard failures — capture the error returned by persistPermissionGrant(call.Name, args, decisionReason, options) and emit a debug/telemetry/log entry (using the existing logger/telemetry facility in this package) that includes the call.Name, args summary and the error so persistence failures are visible while still not blocking the approved call; leave buildPermissionEvent/requestEvent logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/agent/compaction.go`:
- Around line 375-388: The reactive recovery path in recover currently sets
state.lowWaterMark = estimateTokens(result) without accounting for toolTokens,
which is inconsistent with maybeCompact (which compares size =
estimateTokens(messages) + toolTokens against lowWaterMark) and can cause a
redundant proactive compaction; fix by changing recover to accept tools
[]zeroruntime.ToolDefinition (or equivalent source of tool token counts) and set
state.lowWaterMark = estimateTokens(result) + computeToolTokens(tools) so
lowWaterMark and maybeCompact use the same token accounting, and update all
callers of recover to pass the same tools slice (or propagate a toolTokens int)
accordingly.
In `@internal/agent/loop.go`:
- Around line 574-586: The "Always allow" branch currently honours the
permission for this call but swallows persistence errors; keep the behavior (set
permissionGranted = true and only call persistPermissionGrant when
options.Sandbox != nil) but don’t silently discard failures — capture the error
returned by persistPermissionGrant(call.Name, args, decisionReason, options) and
emit a debug/telemetry/log entry (using the existing logger/telemetry facility
in this package) that includes the call.Name, args summary and the error so
persistence failures are visible while still not blocking the approved call;
leave buildPermissionEvent/requestEvent logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b8f2f38-793d-476e-b9b2-009255931678
📒 Files selected for processing (3)
internal/agent/compaction.gointernal/agent/compaction_test.gointernal/agent/loop.go
exec persists payload["model"] on --allow-escalation runs (the model can change mid-run), but BuildReport priced every event from the session's Metadata.ModelID and never read the payload, so usage from an escalated model was mis-priced at the base model (audit medium report.go:17). usageEventPayload now carries Model and BuildReport prefers it, falling back to the session model when absent (older events stay byte-identical and behave as before). + TestBuildReportPricesFromEventModelWhenPresent.
…uble-count) Fork re-appended every parent event, including provider_usage, into the child, so a usage report aggregating the parent and the fork double-counted the parent's usage (audit medium store.go:387). Skip EventUsage when copying — it already counted against the parent and is not part of the conversation history a fork replays — and record the actual copied count in the fork marker. + TestStoreForkSkipsUsageEvents.
|
✅ Action performedFull review finished. |
Vasanthdev2004
approved these changes
Jun 13, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Manual review verdict: approved. I rechecked the latest head (6289a37) across the high-risk sandbox, tool execution, MCP, usage, and compaction paths and did not find a PR-blocking issue. Focused local validation passed for sandbox, tools, usage, agent, MCP, background, cron, sessions, and zerogit. Note: internal/cli timed out locally because this machine has configured MCP servers and that test path is not fully hermetic here; GitHub CI smoke is green on Windows/Ubuntu/macOS, so I am not treating it as a blocker for this PR.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/agent/compaction.go (1)
375-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same token basis for reactive low-water mark as proactive compaction.
Reactive compaction stores
state.lowWaterMarkusing message-only tokens, but proactive compaction compares message+tool-definition tokens. That mismatch can retrigger compaction repeatedly after a reactive recovery even when history hasn’t meaningfully grown.Suggested fix
diff --git a/internal/agent/compaction.go b/internal/agent/compaction.go @@ func (state *compactionState) recover( ctx context.Context, provider Provider, messages []zeroruntime.Message, + tools []zeroruntime.ToolDefinition, errorMessage string, ) (compacted []zeroruntime.Message, retried bool, err error) { @@ + toolTokens := estimateToolDefTokens(tools) + before := estimateTokens(messages) + toolTokens + after := estimateTokens(result) + toolTokens - if estimateTokens(result) >= estimateTokens(messages) { + if after >= before { return messages, false, nil } @@ - state.lowWaterMark = estimateTokens(result) + state.lowWaterMark = after return result, true, nil }Also pass
exposedat both recover call sites ininternal/agent/loop.go:- if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, err.Error()); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, err.Error()); retried { - if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, collected.Error); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, collected.Error); retried {internal/cli/exec_spec.go (1)
169-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd JSON interruption terminal events in spec-draft mode.
runExecSpecDraftstill routes-o jsoninterruptions to stderr text (Interrupted.) instead of emitting terminal JSONerror+done. That breaks protocol parity withrunExecand can leave JSON clients waiting for a terminal record.💡 Suggested fix
if err != nil { if errors.Is(err, context.Canceled) || runCtx.Err() != nil { sessionRecorder.append(sessions.EventError, map[string]any{"message": "interrupted"}) - if run.options.outputFormat == execOutputStreamJSON { + switch run.options.outputFormat { + case execOutputStreamJSON: writer.errorEvent("interrupted", "run cancelled by signal", false) writer.runEnd("interrupted", exitInterrupted) if writer.err != nil { return exitCrash } - } else { + case execOutputJSON: + writer.errorEvent("interrupted", "run cancelled by signal", false) + writer.writeJSON(map[string]any{ + "type": "done", + "exit_code": exitInterrupted, + }) + if writer.err != nil { + return exitCrash + } + default: fmt.Fprintln(run.stderr, "Interrupted.") } return exitInterrupted }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_spec.go` around lines 169 - 179, The JSON output branch in the cancellation handling of runExecSpecDraft is missing the terminal JSON events; modify the cancellation block (the code checking errors.Is(err, context.Canceled) || runCtx.Err() != nil) so that when run.options.outputFormat == execOutputStreamJSON you emit the same sequence used by runExec: call writer.errorEvent("interrupted", "run cancelled by signal", false) then writer.runEnd("interrupted", exitInterrupted) and handle writer.err (return exitCrash on error); keep the existing sessionRecorder.append(sessions.EventError, ...) and the non-JSON fallback that prints "Interrupted." unchanged.internal/sessions/store.go (1)
702-713:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync the metadata directory after rename to fully harden crash durability.
writeFileSyncflushes file bytes, but afteros.Renamethe directory entry is still not guaranteed durable until the parent directory is synced. A crash can still lose or roll back the renamedmetadata.json.Suggested fix
func (store *Store) writeMetadata(session Metadata) error { data, err := json.MarshalIndent(session, "", " ") if err != nil { return fmt.Errorf("encode zero session metadata: %w", err) } path := store.metadataPath(session.SessionID) tmp := fmt.Sprintf("%s.tmp-%d", path, store.idCounter.Add(1)) if err := writeFileSync(tmp, append(data, '\n'), 0o600); err != nil { return fmt.Errorf("write zero session metadata: %w", err) } if err := os.Rename(tmp, path); err != nil { _ = os.Remove(tmp) return fmt.Errorf("replace zero session metadata: %w", err) } + dir, err := os.Open(filepath.Dir(path)) + if err != nil { + return fmt.Errorf("open zero session directory for sync: %w", err) + } + defer dir.Close() + if err := dir.Sync(); err != nil { + return fmt.Errorf("sync zero session directory: %w", err) + } return nil }Also applies to: 716-732
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store.go` around lines 702 - 713, The code flushes the file with writeFileSync and atomically renames tmp->path but doesn't fsync the parent directory; after the successful os.Rename(tmp, path) open the parent directory (e.g., via os.Open(filepath.Dir(path))), call dirFile.Sync(), close it, and propagate any Sync error (wrap with context like "sync metadata dir") while still removing tmp on errors; apply the same directory-sync fix to the other occurrence in this file that also uses writeFileSync followed by os.Rename.internal/cli/sandbox.go (1)
285-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate usage errors to document the new
--pathflag.Line [285] and Line [341] still print usage strings without
--path, while help text now advertises it. This makes CLI recovery instructions inconsistent.Suggested fix
- return writeExecUsageError(stderr, "usage: zero sandbox grants "+command+" <tool> [--auto low|medium|high] [--reason text] [--json]") + return writeExecUsageError(stderr, "usage: zero sandbox grants "+command+" <tool> [--auto low|medium|high] [--reason text] [--path <path>] [--json]") @@ - return writeExecUsageError(stderr, "usage: zero sandbox grants revoke <tool> [--json]") + return writeExecUsageError(stderr, "usage: zero sandbox grants revoke <tool> [--path <path>] [--json]")Also applies to: 341-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sandbox.go` around lines 285 - 286, Update the usage error strings that are passed to writeExecUsageError to include the new --path flag; locate the calls that construct "usage: zero sandbox grants "+command+" <tool> [--auto low|medium|high] [--reason text] [--json]" (two occurrences) and change them to include "[--path <path>]" in the bracketed options so the printed recovery/help text matches the advertised --path flag.
🧹 Nitpick comments (3)
internal/sandbox/safe_command_test.go (1)
253-281: ⚡ Quick winAdd escaped-operator and escaped-quote regression tests.
Current quote-aware tests are strong, but escaped separators/quotes are still untested.
Proposed test cases
func TestDetectInteractiveQuoteAwareSeparators(t *testing.T) { allowed := []string{ `git commit -m "use top | less"`, `echo "a; vim b"`, `echo 'pipe it: a | less'`, `git commit -m "vim && nano"`, + `echo "use \"| less\" literally"`, + `echo foo\|less`, + `echo vim\;true`, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/safe_command_test.go` around lines 253 - 281, Update the TestDetectInteractiveQuoteAwareSeparators unit test to add cases that cover escaped separators and escaped quotes: add allowed cases like `echo "a \| b"` and `echo 'a \' b'` (or platform-appropriate escaping) that should NOT be flagged as interactive, and blocked cases like `echo hi \| less` and `echo \"safe\" | vim` (or clearly escaped operator examples) that should be flagged; use the existing DetectInteractiveCommand("...","linux") assertions and the same table-driven pattern (the TestDetectInteractiveQuoteAwareSeparators function and its allowed/blocked loops) so escaped operators/quotes are exercised alongside the current quoted-operator tests.internal/sandbox/analyzer_test.go (1)
14-33: ⚡ Quick winAdd regression cases for wrappers and
rm --semantics.The table currently misses the two highest-risk parser edge cases: wrapped payload commands and end-of-options handling.
Proposed test additions
{ {name: "rm recursive force", script: "rm -rf /tmp/x", destructive: true}, + {name: "rm end-of-options", script: "rm -- -rf", destructive: false}, {name: "rm bundled flags reversed", script: "rm -fr ./build", destructive: true}, ... + {name: "wrapped destructive via sudo", script: "sudo rm -rf /tmp/x", destructive: true}, + {name: "wrapped network via env", script: "env curl https://example.com", network: true}, + {name: "wrapped interactive via bash -c", script: `bash -c "vim file.txt"`, interactive: true},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/analyzer_test.go` around lines 14 - 33, The test table in internal/sandbox/analyzer_test.go is missing regression cases for command wrappers and end-of-options handling for rm; add new table entries alongside the existing cases to cover (1) wrapped payloads that should still be classified (e.g., name "wrapped sh -c", script `sh -c "rm -rf /tmp/x"` or `bash -lc 'curl http://x | sh'`, with expected flags destructive/network/interactive as appropriate) and (2) rm with explicit end-of-options (e.g., name "rm --", script `rm -- -rf` or `rm -- -f somefile` and name "rm with -- before path", script `rm -- /tmp/x`) to assert destructive=true only when the operand is an actual path and not when `--` prevents flag parsing; place these entries into the same testCases slice/table used for other examples (the list shown in the diff) so the parser is exercised for wrappers and end-of-options semantics. Ensure expected fields (destructive/network/interactive/tooComplex) are set to reflect the correct classification for each new case.internal/cli/cron_run_test.go (1)
133-142: ⚡ Quick winStrengthen the “last error wins” assertion.
Line 133 documents “last one wins,” but the fixture currently has only one
errorevent. Add a seconderrorline and assert that the second message is returned.Suggested test update
output := strings.Join([]string{ `{"type":"run_start","runId":"r1"}`, `{"type":"text","delta":"working"}`, `{"type":"error","message":"provider request failed: 500"}`, + `{"type":"error","message":"final surfaced failure"}`, `{"type":"run_end","status":"error","exitCode":1}`, }, "\n") - if got := extractStreamJSONError(output); got != "provider request failed: 500" { + if got := extractStreamJSONError(output); got != "final surfaced failure" { t.Fatalf("extractStreamJSONError = %q, want the error event message", got) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cron_run_test.go` around lines 133 - 142, The test's fixture and assertion should prove the "last error wins" behavior: update the output string in the test to include a second `{"type":"error",...}` line after the existing error (e.g., `{"type":"error","message":"second failure"}`), and change the expected value passed to extractStreamJSONError to the second error's message (e.g., assert it returns "second failure"); this verifies extractStreamJSONError returns the last error event message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/background/process_posix.go`:
- Around line 46-58: terminateProcess assumes the input pid is the process group
ID and calls syscall.Kill(-pid,...), which can falsely succeed for non-leader
PIDs; first call syscall.Getpgid(pid) (or equivalent) to resolve the actual pgid
and use that pgid for the group signal and for processGroupAlive checks, falling
back to return the Getpgid error if it fails; update calls in terminateProcess
(and the duplicate block around lines 67-83) to use the resolved pgid when
calling syscall.Kill and when polling liveness via processGroupAlive, and add a
regression test that creates a process group with a leader and a non-leader
child, calls terminateProcess with the child's pid, and asserts the whole group
is terminated.
In `@internal/cli/sandbox.go`:
- Around line 301-310: The code currently treats an explicit empty --path as
"not provided" and widens scope; change the handling so explicit empty values
fail instead of falling back: in the blocks that handle options.path (the branch
that calls filepath.Abs and sets input.Scope and input.ScopeKind to
zeroSandbox.ScopeFile, and the similar logic in revoke and the other locations),
add a check that if options.path == "" then return writeExecUsageError(stderr,
"empty --path is not allowed") (or equivalent usage error) and otherwise proceed
to compute filepath.Abs and set input.Scope/input.ScopeKind; if your options
parsing supports a "wasProvided" flag prefer checking that to distinguish
omitted vs explicit empty, but if not available, treat empty string as an
explicit error. Ensure the same change is applied at the other affected sites so
explicit --path="" never widens scope.
In `@internal/cli/skills.go`:
- Around line 69-76: The duplicate-detection error is currently ignored; update
the skills.Duplicates call handling so when derr != nil you emit a redacted
warning to stderr instead of dropping the error: after calling
skills.Duplicates(dir) check if derr != nil and call fmt.Fprintf(stderr,
"warning: failed to detect duplicate skills: %v\n",
redaction.RedactString(derr.Error(), redaction.Options{})) (or similar redacted
message), otherwise keep the existing loop that prints each duplicate using
redaction.RedactString(dup.Name/dup.Winner/dup.Loser); ensure you continue using
the same stderr variable and do not change the existing duplicate-reporting
branch.
---
Outside diff comments:
In `@internal/cli/exec_spec.go`:
- Around line 169-179: The JSON output branch in the cancellation handling of
runExecSpecDraft is missing the terminal JSON events; modify the cancellation
block (the code checking errors.Is(err, context.Canceled) || runCtx.Err() !=
nil) so that when run.options.outputFormat == execOutputStreamJSON you emit the
same sequence used by runExec: call writer.errorEvent("interrupted", "run
cancelled by signal", false) then writer.runEnd("interrupted", exitInterrupted)
and handle writer.err (return exitCrash on error); keep the existing
sessionRecorder.append(sessions.EventError, ...) and the non-JSON fallback that
prints "Interrupted." unchanged.
In `@internal/cli/sandbox.go`:
- Around line 285-286: Update the usage error strings that are passed to
writeExecUsageError to include the new --path flag; locate the calls that
construct "usage: zero sandbox grants "+command+" <tool> [--auto
low|medium|high] [--reason text] [--json]" (two occurrences) and change them to
include "[--path <path>]" in the bracketed options so the printed recovery/help
text matches the advertised --path flag.
In `@internal/sessions/store.go`:
- Around line 702-713: The code flushes the file with writeFileSync and
atomically renames tmp->path but doesn't fsync the parent directory; after the
successful os.Rename(tmp, path) open the parent directory (e.g., via
os.Open(filepath.Dir(path))), call dirFile.Sync(), close it, and propagate any
Sync error (wrap with context like "sync metadata dir") while still removing tmp
on errors; apply the same directory-sync fix to the other occurrence in this
file that also uses writeFileSync followed by os.Rename.
---
Nitpick comments:
In `@internal/cli/cron_run_test.go`:
- Around line 133-142: The test's fixture and assertion should prove the "last
error wins" behavior: update the output string in the test to include a second
`{"type":"error",...}` line after the existing error (e.g.,
`{"type":"error","message":"second failure"}`), and change the expected value
passed to extractStreamJSONError to the second error's message (e.g., assert it
returns "second failure"); this verifies extractStreamJSONError returns the last
error event message.
In `@internal/sandbox/analyzer_test.go`:
- Around line 14-33: The test table in internal/sandbox/analyzer_test.go is
missing regression cases for command wrappers and end-of-options handling for
rm; add new table entries alongside the existing cases to cover (1) wrapped
payloads that should still be classified (e.g., name "wrapped sh -c", script `sh
-c "rm -rf /tmp/x"` or `bash -lc 'curl http://x | sh'`, with expected flags
destructive/network/interactive as appropriate) and (2) rm with explicit
end-of-options (e.g., name "rm --", script `rm -- -rf` or `rm -- -f somefile`
and name "rm with -- before path", script `rm -- /tmp/x`) to assert
destructive=true only when the operand is an actual path and not when `--`
prevents flag parsing; place these entries into the same testCases slice/table
used for other examples (the list shown in the diff) so the parser is exercised
for wrappers and end-of-options semantics. Ensure expected fields
(destructive/network/interactive/tooComplex) are set to reflect the correct
classification for each new case.
In `@internal/sandbox/safe_command_test.go`:
- Around line 253-281: Update the TestDetectInteractiveQuoteAwareSeparators unit
test to add cases that cover escaped separators and escaped quotes: add allowed
cases like `echo "a \| b"` and `echo 'a \' b'` (or platform-appropriate
escaping) that should NOT be flagged as interactive, and blocked cases like
`echo hi \| less` and `echo \"safe\" | vim` (or clearly escaped operator
examples) that should be flagged; use the existing
DetectInteractiveCommand("...","linux") assertions and the same table-driven
pattern (the TestDetectInteractiveQuoteAwareSeparators function and its
allowed/blocked loops) so escaped operators/quotes are exercised alongside the
current quoted-operator tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: da793473-7644-4a13-b6db-28021cd371bf
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/config/contracts.go
- internal/config/contracts_test.go
- internal/specialist/output_tool.go
- internal/tools/registry.go
Findings on the audit code from batches 21-36; each behavioural fix ships with a regression test (several confirmed to fail without the change). sandbox/analyzer (AnalyzeCommand): - Unwrap launcher prefixes (sudo/env/nice/bash -c/...) and recurse into `sh -c` payloads so `sudo rm -rf`, `env curl …`, `bash -c 'vim x'` classify on the real command, not the launcher. Mirrors DetectInteractiveCommand. - Honor rm's `--` end-of-options marker so `rm -- -rf` (delete a file named -rf) is not flagged destructive. sandbox/engine: NetworkHostAllowed now shares one backend-aware effective-mode helper with Evaluate, so scoped collapses to deny when the backend can't enforce scoped egress — the per-tool gate can no longer permit traffic the engine-level decision would fail closed. sandbox/egress: cache domain-prompt decisions by host:port, not host, so one "allow" can't authorize other ports on the same host. sandbox/safe_command: the segment splitter is now escape-aware (\| , \" ) so an escaped operator/quote can't manufacture a fake interactive segment. sandbox/log_monitor (darwin): Wait() the `log stream` child after draining stdout so the cancelled process isn't leaked as a zombie. tools/web_search: confine the search backend to same-host redirects (with a limit) so a redirect chain can't egress to a host the scoped/deny policy never authorized. cli/sandbox: an explicit empty --path (`--path=` / `--path ""`) now fails closed instead of silently widening an allow to tool-wide or a revoke to all-grants. cli/skills: warn on stderr when duplicate-skill detection fails instead of swallowing the error. specialist/exec: don't mask a failed orphan-kill — join the TerminateProcess error with the SetPID error so a surviving process is visible. background/process_posix: resolve the PGID and only group-signal when pid leads its own group; otherwise signal the individual PID. Avoids both a false-success on a non-leader ESRCH and the hazard of signalling an unrelated (possibly our own) group. Verified: gofmt + go vet clean; host + linux + windows builds; full go test ./... green.
Collaborator
Author
|
Addressed the re-review findings in
One deliberate deviation — CI green on all four Smoke jobs (ubuntu/macos/windows) + vet, host/linux/windows builds. @coderabbitai full review |
|
Quick notes before the review lands:
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/agent/compaction.go (1)
347-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
lowWaterMarkon the same token basis after reactive compaction.Line 386 stores a message-only baseline, but proactive checks use
messages + tool definitions(sizeat Line 311). That mismatch can make unchanged history look “grown” after a reactive recover, causing redundant re-compactions and extra summarizer cost.💡 Proposed fix
diff --git a/internal/agent/compaction.go b/internal/agent/compaction.go @@ func (state *compactionState) recover( ctx context.Context, provider Provider, messages []zeroruntime.Message, + tools []zeroruntime.ToolDefinition, errorMessage string, ) (compacted []zeroruntime.Message, retried bool, err error) { @@ + toolTokens := estimateToolDefTokens(tools) @@ - state.lowWaterMark = estimateTokens(result) + state.lowWaterMark = estimateTokens(result) + toolTokens return result, true, nil }diff --git a/internal/agent/loop.go b/internal/agent/loop.go @@ - if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, err.Error()); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, err.Error()); retried { @@ - if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, collected.Error); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, collected.Error); retried {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/compaction.go` around lines 347 - 387, The reactive recover currently sets state.lowWaterMark = estimateTokens(result), but proactive checks compare a different "size" metric (estimateTokens(messages) plus the tool-definitions overhead), causing mismatched baselines; change recover (in the recover method of compactionState) to compute and store the same composite size used by the proactive path (i.e., the identical "size" calculation referenced at the proactive check around Line 311) and assign that to state.lowWaterMark (reuse the same helper or code that builds that size so both paths use the exact same token-basis), leaving reactiveAttempted/estimateTokens/result logic unchanged.internal/specialist/accounting.go (1)
49-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
usageRolledUpreflect persisted state under concurrent finishers.On Line 51,
usageRolledUpis written from caller-local state, which can be stale under contention. If one goroutine appends usage and another wins the stop append withrolledUp=false, the persistedspecialist_stoppayload becomes inconsistent with the actualusageevent history.Proposed fix
payload["status"] = specialistStopStatus(status, exitCode, runErr) payload["exitCode"] = exitCode + if !usageRolledUp && specialistEventExists(store, input.ParentSessionID, sessions.EventUsage, input.ChildSessionID, summary.RunID) { + usageRolledUp = true + } payload["usageRolledUp"] = usageRolledUp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/accounting.go` around lines 49 - 52, The payload currently sets "usageRolledUp" from the caller-local variable usageRolledUp which can be stale under concurrent finishers; update the code that builds the specialist_stop payload to read the persisted rolledUp value returned by the append/stop operation (or explicitly re-read the latest persisted usage/stop record) and set payload["usageRolledUp"] from that persisted value (e.g., use the append result's RolledUp flag or a DB read) instead of the local usageRolledUp so the specialist_stop payload always reflects the persisted state.internal/cli/exec_spec.go (1)
169-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit terminal JSON interruption events in spec-draft mode, not stderr text.
runExecSpecDraftstill routes interrupted-o jsonruns through the text fallback, so JSON consumers do not receive terminalerror+doneevents like the mainrunExecpath.💡 Suggested fix
if errors.Is(err, context.Canceled) || runCtx.Err() != nil { sessionRecorder.append(sessions.EventError, map[string]any{"message": "interrupted"}) - if run.options.outputFormat == execOutputStreamJSON { + switch run.options.outputFormat { + case execOutputStreamJSON: writer.errorEvent("interrupted", "run cancelled by signal", false) writer.runEnd("interrupted", exitInterrupted) if writer.err != nil { return exitCrash } - } else { + case execOutputJSON: + writer.errorEvent("interrupted", "run cancelled by signal", false) + writer.writeJSON(map[string]any{"type": "done", "exit_code": exitInterrupted}) + if writer.err != nil { + return exitCrash + } + default: fmt.Fprintln(run.stderr, "Interrupted.") } return exitInterrupted }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_spec.go` around lines 169 - 179, The spec-draft interrupt handling in runExecSpecDraft should emit terminal JSON events when run.options.outputFormat == execOutputStreamJSON instead of falling back to stderr text; modify the branch in runExecSpecDraft that currently does fmt.Fprintln(run.stderr, "Interrupted.") so that when outputFormat == execOutputStreamJSON it calls writer.errorEvent("interrupted", "run cancelled by signal", false) and writer.runEnd("interrupted", exitInterrupted) (and returns exitCrash if writer.err != nil), otherwise keep the existing stderr text path; ensure sessionRecorder.append(sessions.EventError, ...) remains unchanged.internal/cli/sandbox.go (1)
284-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate usage errors to include
--pathfor grant set/revoke commands.The command parser accepts
--path, but the usage strings shown on argument-count failures still omit it, which can mislead users during recovery.Suggested patch
- return writeExecUsageError(stderr, "usage: zero sandbox grants "+command+" <tool> [--auto low|medium|high] [--reason text] [--json]") + return writeExecUsageError(stderr, "usage: zero sandbox grants "+command+" <tool> [--auto low|medium|high] [--reason text] [--path <path>] [--json]") @@ - return writeExecUsageError(stderr, "usage: zero sandbox grants revoke <tool> [--json]") + return writeExecUsageError(stderr, "usage: zero sandbox grants revoke <tool> [--path <path>] [--json]")Also applies to: 340-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sandbox.go` around lines 284 - 286, The usage error strings for the grants subcommands omit the accepted --path flag; update the message passed to writeExecUsageError (the string built with "usage: zero sandbox grants "+command+" <tool> ...") to include [--path] (e.g. add " [--path <path>]") so users see the flag; make this change for both occurrences where the positional length check returns the usage error (the blocks using len(positional) != 1 and calling writeExecUsageError with the grants usage string) so set/revoke both display the corrected usage.
🧹 Nitpick comments (2)
internal/cli/sandbox_test.go (1)
167-169: ⚡ Quick winAssert
exitUsageexplicitly for rejected empty--pathcases.These cases are specifically usage errors; checking only “non-success” can hide accidental crash/error-path regressions.
Suggested patch
- if exit := run(args...); exit == exitSuccess { - t.Fatalf("%v: expected a usage error for an empty --path, got success", args) + if exit := run(args...); exit != exitUsage { + t.Fatalf("%v: expected usage exit (%d) for empty --path, got %d", args, exitUsage, exit) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sandbox_test.go` around lines 167 - 169, The test currently checks that run(args...) != exitSuccess for empty --path cases; change it to assert the exact usage exit code by replacing the condition to if exit := run(args...); exit != exitUsage { t.Fatalf("%v: expected exitUsage for an empty --path, got %v", args, exit) } so the test uses the run function and the exitUsage and exitSuccess symbols to explicitly require an exitUsage result rather than any non-success code.internal/sandbox/egress_test.go (1)
301-344: ⚡ Quick winAdd a regression assertion that cache decisions are isolated per port.
This test currently proves same-host same-port caching, but not cross-port isolation. Add a second authorize call on the same host with a different port and assert an additional prompt invocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/egress_test.go` around lines 301 - 344, Add a cross-port assertion to TestEgressProxyAuthorizeDomainPrompt to ensure cache decisions are per-port: after the existing cached allow checks, call proxy.authorize("ask-allow.test", 80) (different port) and assert it returns true, then check mu-guarded calls["ask-allow.test"] == 2 to verify the prompt was invoked again for the different port; reference the existing egressProxy.domainPrompt callback and authorize method when making the insertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/analyzer.go`:
- Around line 142-146: The parser currently uses a global
wrapperValueOptions[text] check which can skip payload tokens regardless of
which wrapper is active; change the parsing loop to track the active wrapper
token (e.g., set activeWrapper when you detect a wrapper like "sudo" or other
wrapper tokens) and replace the global lookup with a per-wrapper map lookup
(e.g., wrapperValueOptions[activeWrapper][text]) so only options for the current
wrapper consume the next token via index++; update the loop that references
text, args, and index to consult the active-wrapper-specific map and clear
activeWrapper once the wrapper's option parsing is complete; add regression
tests covering cases like "sudo -n rm -rf /tmp/x" and "sudo -n curl
https://x.test" to ensure the payload command is not skipped.
In `@internal/sandbox/egress.go`:
- Around line 256-273: The promptForHost function can still trigger concurrent
runDomainPrompt calls for the same host:port; add an in-flight deduplication so
only one caller runs runDomainPrompt and others wait for its result. Introduce
an inFlight map keyed by the same key (e.g., map[string]chan bool) guarded by
proxy.cacheMu (or a separate mutex), and in promptForHost: after checking
decisionCache, if no decision exists check if a channel exists in inFlight — if
so, unlock and wait to receive the bool result from that channel and return it;
if not, create a channel, store it in inFlight, unlock, call
proxy.runDomainPrompt(host, port), then lock, write the result into
decisionCache, remove the channel from inFlight, close the channel (or send the
result then close), unlock and return; ensure waiting goroutines receive the
same result and that the inFlight entry is removed after completion to avoid
leaks. Use the existing symbols promptForHost, runDomainPrompt, decisionCache
and cacheMu when making changes.
In `@internal/sandbox/safe_command.go`:
- Around line 504-507: The parser currently treats ')' as closing a substitution
regardless of quote state; modify the handler that checks "case c == ')' &&
substitutionDepth > 0" so it only decrements substitutionDepth and calls flush()
when the ')' is not inside double quotes (i.e., check the parser's double-quote
state / inDoubleQuotes flag and require !inDoubleQuotes before closing the
substitution). Update the logic that tracks quotes accordingly if needed so
quoted ')' inside $(...) are ignored. Add a regression test/assertion that
parses a command like echo $(printf "a) less") (or uses the existing
Parse/Tokenize function) and asserts that the quoted ')' inside the $() payload
does not terminate the substitution and does not create a spurious segment.
---
Outside diff comments:
In `@internal/agent/compaction.go`:
- Around line 347-387: The reactive recover currently sets state.lowWaterMark =
estimateTokens(result), but proactive checks compare a different "size" metric
(estimateTokens(messages) plus the tool-definitions overhead), causing
mismatched baselines; change recover (in the recover method of compactionState)
to compute and store the same composite size used by the proactive path (i.e.,
the identical "size" calculation referenced at the proactive check around Line
311) and assign that to state.lowWaterMark (reuse the same helper or code that
builds that size so both paths use the exact same token-basis), leaving
reactiveAttempted/estimateTokens/result logic unchanged.
In `@internal/cli/exec_spec.go`:
- Around line 169-179: The spec-draft interrupt handling in runExecSpecDraft
should emit terminal JSON events when run.options.outputFormat ==
execOutputStreamJSON instead of falling back to stderr text; modify the branch
in runExecSpecDraft that currently does fmt.Fprintln(run.stderr, "Interrupted.")
so that when outputFormat == execOutputStreamJSON it calls
writer.errorEvent("interrupted", "run cancelled by signal", false) and
writer.runEnd("interrupted", exitInterrupted) (and returns exitCrash if
writer.err != nil), otherwise keep the existing stderr text path; ensure
sessionRecorder.append(sessions.EventError, ...) remains unchanged.
In `@internal/cli/sandbox.go`:
- Around line 284-286: The usage error strings for the grants subcommands omit
the accepted --path flag; update the message passed to writeExecUsageError (the
string built with "usage: zero sandbox grants "+command+" <tool> ...") to
include [--path] (e.g. add " [--path <path>]") so users see the flag; make this
change for both occurrences where the positional length check returns the usage
error (the blocks using len(positional) != 1 and calling writeExecUsageError
with the grants usage string) so set/revoke both display the corrected usage.
In `@internal/specialist/accounting.go`:
- Around line 49-52: The payload currently sets "usageRolledUp" from the
caller-local variable usageRolledUp which can be stale under concurrent
finishers; update the code that builds the specialist_stop payload to read the
persisted rolledUp value returned by the append/stop operation (or explicitly
re-read the latest persisted usage/stop record) and set payload["usageRolledUp"]
from that persisted value (e.g., use the append result's RolledUp flag or a DB
read) instead of the local usageRolledUp so the specialist_stop payload always
reflects the persisted state.
---
Nitpick comments:
In `@internal/cli/sandbox_test.go`:
- Around line 167-169: The test currently checks that run(args...) !=
exitSuccess for empty --path cases; change it to assert the exact usage exit
code by replacing the condition to if exit := run(args...); exit != exitUsage {
t.Fatalf("%v: expected exitUsage for an empty --path, got %v", args, exit) } so
the test uses the run function and the exitUsage and exitSuccess symbols to
explicitly require an exitUsage result rather than any non-success code.
In `@internal/sandbox/egress_test.go`:
- Around line 301-344: Add a cross-port assertion to
TestEgressProxyAuthorizeDomainPrompt to ensure cache decisions are per-port:
after the existing cached allow checks, call proxy.authorize("ask-allow.test",
80) (different port) and assert it returns true, then check mu-guarded
calls["ask-allow.test"] == 2 to verify the prompt was invoked again for the
different port; reference the existing egressProxy.domainPrompt callback and
authorize method when making the insertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5818346-4e64-483f-86cc-82f62600de00
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/config/contracts.go
- internal/config/contracts_test.go
- internal/specialist/output_tool.go
- internal/tools/registry.go
Follow-ups to batch 37, each with a regression test:
- safe_command (splitter): a ')' is now only a substitution boundary when it
closes an active, UNQUOTED $(...). A quote-state stack saved at each '$(' lets a
substitution run in a fresh quoting context and restores the outer quoting on
its matching ')'. Fixes `echo $(printf "a) less")` splitting into a fake `less`
segment, while still isolating the inner command of `echo "$(vim x)"`.
- egress: deduplicate in-flight domain prompts per host:port. Concurrent requests
for the same unknown target now wait on the first prompt and reuse its cached
decision instead of each firing a prompt (a prompt storm) and racing to write
the final decision. Race-tested with 25 concurrent authorizers → one prompt.
- analyzer + safe_command: wrapper option-value consumption is now wrapper-specific
(wrapperValueOptionsByProg) instead of a global option set. A valueless flag like
`sudo -n` no longer swallows the real payload, so `sudo -n rm -rf` /
`sudo -n curl …` are correctly classified destructive/network; `sudo -u root vim`
still consumes its value. Shared by the AST analyzer and the regex detector so
they can't diverge.
Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and
-race on sandbox/background all green.
Collaborator
Author
|
Addressed re-review round 2 in
CI green; @coderabbitai full review |
|
Quick notes on the three fixes:
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/agent/compaction.go (1)
365-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a consistent token basis for
lowWaterMarkafter reactive compaction.Line 386 writes
lowWaterMarkusing message-only tokens, butmaybeCompactcompares withmessages + tool-definition tokens. After reactive compaction, this mismatch can re-trigger compaction on the next turn even when history didn’t materially grow, causing unnecessary summarization churn.Suggested fix
--- a/internal/agent/compaction.go +++ b/internal/agent/compaction.go @@ func (state *compactionState) recover( ctx context.Context, provider Provider, messages []zeroruntime.Message, + tools []zeroruntime.ToolDefinition, errorMessage string, ) (compacted []zeroruntime.Message, retried bool, err error) { + toolTokens := estimateToolDefTokens(tools) @@ - if estimateTokens(result) >= estimateTokens(messages) { + if estimateTokens(result)+toolTokens >= estimateTokens(messages)+toolTokens { return messages, false, nil } @@ - state.lowWaterMark = estimateTokens(result) + state.lowWaterMark = estimateTokens(result) + toolTokens return result, true, nil }--- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ - if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, err.Error()); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, err.Error()); retried { @@ - if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, collected.Error); retried { + if compacted, retried, retryErr := compactor.recover(ctx, provider, messages, exposed, collected.Error); retried {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/compaction.go` around lines 365 - 387, The reactive compaction sets state.lowWaterMark using message-only tokens which mismatches maybeCompact's comparison basis (messages + tool-definition tokens); update the assignment so lowWaterMark is computed using the same token basis maybeCompact uses (i.e., include the tool-definition/token overhead when calling estimateTokens) to avoid spurious re-compactions—change the lowWaterMark update after successful compaction in maybeCompact (the block that sets state.reactiveAttempted and state.lowWaterMark using estimateTokens(result)) to compute lowWaterMark with the full-history/token-basis used by maybeCompact (use the same helper or formula that maybeCompact uses when comparing messages + tool-definition tokens).internal/sessions/store.go (1)
702-731:⚠️ Potential issue | 🟠 MajorMake event writes durable before fsync/renaming metadata (and fsync the directory after rename).
In
internal/sessions/store.go,appendEventLockedappends toevents.jsonland onlyClose()s the file (nofile.Sync()), thenwriteMetadatapersistently fsyncs/renamesmetadata.json. A power loss between those steps can leaveEventCount/LastEventTypeadvanced inmetadata.jsonwhile the last JSONL record is not on disk, making the session internally inconsistent for subsequent appends/rewinds.Also,
writeMetadatadoesos.Renamewithout fsyncing the containing directory afterward, so the rename itself may not be crash-durable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store.go` around lines 702 - 731, appendEventLocked currently writes to events.jsonl and only Close()s the file—ensure the event record is fsynced before updating metadata: after the file.Write in appendEventLocked call file.Sync() (and handle/return errors) before closing so the JSONL record is durably on disk. For metadata persistence, writeMetadata (which uses writeFileSync then os.Rename) must fsync the containing directory after the atomic rename to make the rename durable: after os.Rename(pathTmp, path) open the parent directory (os.Open on filepath.Dir(path)), call dir.Sync(), close it and propagate errors. You may keep writeFileSync as-is for file data fsync but add the directory fsync in writeMetadata; update error handling to remove tmp on failures where appropriate.
♻️ Duplicate comments (1)
internal/zerogit/zerogit.go (1)
235-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the extra rename/copy source field when
R/Cappears in either status column.At Line 238, only
code[0]is checked. Unstaged rename/copy entries (code[1] == 'R'/'C') leave the source path unconsumed and it gets misparsed as its own file entry.Proposed fix
- // A rename/copy (R or C in the staged column) is followed by a separate + // A rename/copy (R or C in either column) is followed by a separate // NUL-terminated field holding the original path; consume it so it is not // parsed as its own entry. This entry's own path is the destination. - if code[0] == 'R' || code[0] == 'C' { + if code[0] == 'R' || code[0] == 'C' || code[1] == 'R' || code[1] == 'C' { i++ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zerogit/zerogit.go` around lines 235 - 240, The current logic only checks code[0] when deciding to consume the extra NUL-terminated source path for renames/copies; update the check so that if either code[0] == 'R' || code[0] == 'C' OR code[1] == 'R' || code[1] == 'C' is true you increment i to consume the extra field (i++), ensuring unstaged rename/copy entries (when code[1] is R/C) are also handled; locate the check around the comment about "A rename/copy (R or C in the staged column)..." that inspects code and modify that conditional accordingly.
🧹 Nitpick comments (5)
internal/cli/cron_run_test.go (1)
131-146: ⚡ Quick winAdd an explicit “last error wins” assertion in the new helper test.
The helper is documented to return the last
errorevent message, but this test only exercises a singleerrorevent. Add a two-error case to lock the contract.Proposed test addition
func TestExtractStreamJSONError(t *testing.T) { @@ if got := extractStreamJSONError(`{"type":"run_end","status":"success"}`); got != "" { t.Fatalf("no error event should yield empty, got %q", got) } + if got := extractStreamJSONError(strings.Join([]string{ + `{"type":"error","message":"first error"}`, + `{"type":"error","message":"second error"}`, + }, "\n")); got != "second error" { + t.Fatalf("extractStreamJSONError should return last error message, got %q", got) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cron_run_test.go` around lines 131 - 146, TestExtractStreamJSONError currently only checks a single error event; add an assertion that verifies the helper extractStreamJSONError returns the last error when multiple error events are present. Update the test (TestExtractStreamJSONError) to include an output string with at least two `{"type":"error","message":"..."}` lines (different messages) plus other events, call extractStreamJSONError on that string, and assert the returned value equals the last error message to lock the "last error wins" contract.internal/cli/exec_protocol_test.go (1)
632-648: ⚡ Quick winAssert stderr remains silent for
-o jsoninterruption.The test narrative says JSON mode should not emit
"Interrupted."on stderr, but that behavior is not currently asserted.Suggested assertion
if exitCode != exitInterrupted { t.Fatalf("expected interrupted exit %d, got %d: %s", exitInterrupted, exitCode, stderr.String()) } + if stderr.Len() != 0 { + t.Fatalf("expected empty stderr for -o json interruption, got %q", stderr.String()) + } // -o json must end the stream with a terminal error+done, not just print // "Interrupted." to stderr (which the stream-json path already avoided). events := decodeJSONLines(t, stdout.String())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_protocol_test.go` around lines 632 - 648, The test currently checks the JSON output events but doesn't assert that stderr is silent for -o json interruptions; add an assertion after the exitCode check (where exitInterrupted, stderr and stdout are available) to ensure stderr.String() is empty or does not contain "Interrupted." (e.g., t.Fatalf if stderr.String() != "" or strings.Contains(stderr.String(), "Interrupted.")). Keep the new assertion just before calling decodeJSONLines and use the existing stderr variable so the test verifies JSON mode produces no stderr output.internal/cli/deferred_wiring_test.go (1)
196-215: ⚡ Quick winValidate the full tool-entry JSON contract (all entries), not only index 0.
This currently checks only
payload.Tools[0]; a malformed later entry (or missingside_effect) would still pass.Suggested test tightening
- if payload.Tools[0].Name == "" || payload.Tools[0].Permission == "" { - t.Fatalf("tool entries must carry name + permission: %#v", payload.Tools[0]) - } + for i, tool := range payload.Tools { + if tool.Name == "" || tool.Permission == "" || tool.SideEffect == "" { + t.Fatalf("tool[%d] must carry name + permission + side_effect: %#v", i, tool) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/deferred_wiring_test.go` around lines 196 - 215, The test currently only validates payload.Tools[0]; instead iterate over all entries in payload.Tools (for i, tool := range payload.Tools) and for each assert tool.Name != "" && tool.Permission != "" && tool.SideEffect != "" (use t.Fatalf with the index to show which entry failed, e.g. t.Fatalf("tool entry %d must carry name+permission+side_effect: %#v", i, tool)). Keep the existing Type and non-zero length checks but replace the single-index assertions with this loop so every tool JSON entry is validated.internal/sessions/store_test.go (1)
196-216: ⚡ Quick winAssert the fork-marker payload too.
This proves usage events are filtered, but it does not pin the other behavior changed in
Fork:copiedEventCountnow excludes usage events. If that payload regresses back to the parent’s full event count, this test still passes. Decode the trailingEventSessionForkpayload and assertcopiedEventCount == 1(and ideallyparentEventCount == 2).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store_test.go` around lines 196 - 216, Update the test that calls store.Fork(...) and ReadEvents(...) to also decode and assert the payload of the trailing EventSessionFork event: locate the last event whose Type == EventSessionFork, decode its payload fields parentEventCount and copiedEventCount (the fork marker payload produced by Fork) and add assertions that copiedEventCount == 1 and parentEventCount == 2; keep the existing checks that no EventUsage was copied and that the event type sequence equals []EventType{EventMessage, EventSessionFork}.internal/usage/report_test.go (1)
113-151: ⚡ Quick winMake the precedence check conflict with session metadata.
meta[0].ModelID == ""only proves the event model is used when there is no fallback. This still passes ifBuildReportincorrectly prefers a non-empty session model. Set the session metadata to a different valid model and keep the expectation on"gpt-4.1"so the test actually locks in event-model precedence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/usage/report_test.go` around lines 113 - 151, The test currently sets meta[0].ModelID == "" which doesn't prove precedence; change the session metadata to a different valid model id (e.g. meta[0].ModelID = "gpt-4") while leaving the event payload model as "gpt-4.1" and keeping the expected cost derived from "gpt-4.1". Update the meta slice in the test so BuildReport, events, and the registry (variable registry) show a conflicting non-empty session model to force the code path that must prefer the event's payload model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/background/process_posix_test.go`:
- Around line 153-155: The child liveness probe currently treats any non-nil
error from syscall.Kill(childPID, 0) as the child having exited; change that to
only treat syscall.ESRCH as “child no longer exists” and fail the test for any
other error. Concretely, in the test code around the child liveness probe where
syscall.Kill(childPID, 0) is called (using childPID), capture the returned error
into a variable, if err == nil continue, if err == syscall.ESRCH break,
otherwise call t.Fatalf (or t.Fatalf-like helper) reporting the unexpected Kill
error so other errors don’t get masked.
In `@internal/background/terminate.go`:
- Around line 3-6: Update the TerminateProcess docstring to accurately describe
POSIX behavior: state that it attempts to signal the whole process group but
will fall back to signaling the single PID when the PID is not a group leader,
and explicitly mention ConfigureChildProcessGroup as the helper to ensure a
child is in its own group if callers require whole-group termination; do not
change runtime behavior here, only correct the documentation in the
TerminateProcess comment to avoid implying guaranteed group cleanup.
In `@internal/cli/sandbox.go`:
- Around line 463-482: The grant command usage text is stale and omits the
newly-supported --path flag; update the usage/error strings shown for the allow,
deny and revoke grant subcommands so they list --path alongside the other flags
(the places that return execUsageError for those subcommands and any constant
usage/help strings shown to users). Search in sandbox.go for the grant-related
usage text and the execUsageError messages used by the allow|deny|revoke
handling and add "--path" to the displayed flag list so usage errors and help
output include the new --path option.
In `@internal/mcp/network_client.go`:
- Around line 654-663: The scanner currently only enforces a per-line cap
(maxSSEEventBytes) so an event assembled from many `data:` lines (dataLines) can
grow without bound; update scanSSEEvents to track an aggregate byte count for
the current SSE event (e.g., introduce maxSSEEventTotalBytes) that is
incremented as each `data:` line is appended and reset when an event is
dispatched, and if the aggregate exceeds the cap return a clear error and abort
processing; reference and update the scanner usage in scanSSEEvents and ensure
existing per-line buffering via maxSSEEventBytes remains in place.
In `@internal/sandbox/analyzer.go`:
- Around line 136-139: The code currently returns early when
wordText(args[index]) is empty, which aborts unwrapping for expansion-only
wrapper args; modify the logic so that in the function where text :=
wordText(args[index]) is computed you only return on empty text if wrapper is
not yet set (e.g., if wrapper == "" then return "", nil), otherwise continue
scanning (skip this arg and keep looking for the actual program token) so
wrapper unwrapping still completes for cases like env "$opts" curl ...; update
the surrounding loop/control flow to continue rather than return when wrapper is
already set and add regression tests covering wrapper scenarios such as `env
"$opts" curl https://x.test` and `sudo "$maybeFlag" rm -rf /tmp/x` to ensure the
literal payload is detected later in args.
In `@internal/sandbox/egress.go`:
- Around line 299-317: The current runDomainPrompt spawns domainPrompt in a
goroutine and only times out waiting, leaking the underlying callback; change
runDomainPrompt to create a cancellable context with context.WithTimeout(ctx,
timeout) and pass that context into domainPrompt (i.e., change signature from
domainPrompt(host,int) to domainPrompt(ctx context.Context, host string, port
int) and update callers), start the goroutine with that ctx so domainPrompt can
observe cancellation and return early, and when the select picks the timeout
case call cancel() (or let the context timeout) and ensure the goroutine's done
value is still consumed or the function returns only after canceling so no
lingering goroutine or late side-effects occur; update any call sites to
propagate the new context-based contract.
In `@internal/sandbox/runner.go`:
- Around line 509-513: The current global const sandboxDenialLogTag should be
replaced with a per-plan unique tag: add a denialLogTag string field to
CommandPlan (or set it when building a CommandPlan), generate a unique value
(e.g., uuid4 or timestamp+pid) when the plan is created, and use that tag
wherever the profile text is constructed instead of the global
sandboxDenialLogTag; update StartDenialMonitor to filter by the
CommandPlan.denialLogTag for that run (and ensure any code that previously
referenced the global sandboxDenialLogTag now takes the tag from the plan or its
profile), leaving a sensible fallback only if a plan tag is missing.
- Around line 359-363: findSeccompHelper currently returns a sibling
"zero-seccomp" next to the binary if it exists but doesn't check execute
permission; change the sibling-path branch in findSeccompHelper to verify the
candidate is a regular file and is executable (e.g. using
info.Mode().IsRegular() and info.Mode()&0111 != 0 on the FileMode for the
candidate) before returning it, otherwise fall back to the existing PATH lookup
(exec.LookPath) as the PATH branch already does.
In `@internal/sessions/lineage.go`:
- Around line 131-150: Before calling store.List() in Tree, call
store.Get(rootSessionID) and return any error (so metadata-read failures
surface); then proceed to list and build byID/childrenByParent but ensure the
retrieved root Metadata is inserted into byID (and into childrenByParent if it
meets SessionKind/ParentSessionID criteria) so the root isn't lost if
store.List() skipped it. Update Tree to perform the store.Get(rootSessionID)
error check first, add the returned Metadata into the byID map, and then
continue with the existing listing, sorting, and the call to
store.treeFrom(rootSessionID, byID, childrenByParent, ...).
In `@internal/specialist/exec.go`:
- Around line 355-368: The stop path in exec.go records a stop with an empty
runId which later allows onExit to append a second stop when it has a non-empty
runId because the dedupe only matches exact run IDs; change the dedupe logic in
recordSpecialistStop to treat a stored/previously recorded empty runId as a
wildcard (match any subsequent non-empty runId) for stop-event deduplication
only, leaving actual stored runId values unchanged elsewhere; update
recordSpecialistStop's comparison/matching code so that when existingEntry.runId
== "" it considers it equal to any incoming non-empty runId (but not vice-versa
for other semantics), and add a small unit test exercising the
immediate-stop-then-onExit sequence to ensure duplicate stops are prevented.
In `@internal/tools/web_search.go`:
- Around line 187-193: The redirect policy in sameHostRedirectPolicy currently
only compares hostnames and thus allows HTTPS→HTTP downgrades; update
sameHostRedirectPolicy to require the scheme to match exactly (compare
req.URL.Scheme to via[0].URL.Scheme) and return an error refusing redirects that
change scheme (specifically reject an origin with "https" redirecting to
"http"); add a test that exercises a redirect from https://... to http://... to
assert the policy rejects the downgrade.
---
Outside diff comments:
In `@internal/agent/compaction.go`:
- Around line 365-387: The reactive compaction sets state.lowWaterMark using
message-only tokens which mismatches maybeCompact's comparison basis (messages +
tool-definition tokens); update the assignment so lowWaterMark is computed using
the same token basis maybeCompact uses (i.e., include the tool-definition/token
overhead when calling estimateTokens) to avoid spurious re-compactions—change
the lowWaterMark update after successful compaction in maybeCompact (the block
that sets state.reactiveAttempted and state.lowWaterMark using
estimateTokens(result)) to compute lowWaterMark with the
full-history/token-basis used by maybeCompact (use the same helper or formula
that maybeCompact uses when comparing messages + tool-definition tokens).
In `@internal/sessions/store.go`:
- Around line 702-731: appendEventLocked currently writes to events.jsonl and
only Close()s the file—ensure the event record is fsynced before updating
metadata: after the file.Write in appendEventLocked call file.Sync() (and
handle/return errors) before closing so the JSONL record is durably on disk. For
metadata persistence, writeMetadata (which uses writeFileSync then os.Rename)
must fsync the containing directory after the atomic rename to make the rename
durable: after os.Rename(pathTmp, path) open the parent directory (os.Open on
filepath.Dir(path)), call dir.Sync(), close it and propagate errors. You may
keep writeFileSync as-is for file data fsync but add the directory fsync in
writeMetadata; update error handling to remove tmp on failures where
appropriate.
---
Duplicate comments:
In `@internal/zerogit/zerogit.go`:
- Around line 235-240: The current logic only checks code[0] when deciding to
consume the extra NUL-terminated source path for renames/copies; update the
check so that if either code[0] == 'R' || code[0] == 'C' OR code[1] == 'R' ||
code[1] == 'C' is true you increment i to consume the extra field (i++),
ensuring unstaged rename/copy entries (when code[1] is R/C) are also handled;
locate the check around the comment about "A rename/copy (R or C in the staged
column)..." that inspects code and modify that conditional accordingly.
---
Nitpick comments:
In `@internal/cli/cron_run_test.go`:
- Around line 131-146: TestExtractStreamJSONError currently only checks a single
error event; add an assertion that verifies the helper extractStreamJSONError
returns the last error when multiple error events are present. Update the test
(TestExtractStreamJSONError) to include an output string with at least two
`{"type":"error","message":"..."}` lines (different messages) plus other events,
call extractStreamJSONError on that string, and assert the returned value equals
the last error message to lock the "last error wins" contract.
In `@internal/cli/deferred_wiring_test.go`:
- Around line 196-215: The test currently only validates payload.Tools[0];
instead iterate over all entries in payload.Tools (for i, tool := range
payload.Tools) and for each assert tool.Name != "" && tool.Permission != "" &&
tool.SideEffect != "" (use t.Fatalf with the index to show which entry failed,
e.g. t.Fatalf("tool entry %d must carry name+permission+side_effect: %#v", i,
tool)). Keep the existing Type and non-zero length checks but replace the
single-index assertions with this loop so every tool JSON entry is validated.
In `@internal/cli/exec_protocol_test.go`:
- Around line 632-648: The test currently checks the JSON output events but
doesn't assert that stderr is silent for -o json interruptions; add an assertion
after the exitCode check (where exitInterrupted, stderr and stdout are
available) to ensure stderr.String() is empty or does not contain "Interrupted."
(e.g., t.Fatalf if stderr.String() != "" or strings.Contains(stderr.String(),
"Interrupted.")). Keep the new assertion just before calling decodeJSONLines and
use the existing stderr variable so the test verifies JSON mode produces no
stderr output.
In `@internal/sessions/store_test.go`:
- Around line 196-216: Update the test that calls store.Fork(...) and
ReadEvents(...) to also decode and assert the payload of the trailing
EventSessionFork event: locate the last event whose Type == EventSessionFork,
decode its payload fields parentEventCount and copiedEventCount (the fork marker
payload produced by Fork) and add assertions that copiedEventCount == 1 and
parentEventCount == 2; keep the existing checks that no EventUsage was copied
and that the event type sequence equals []EventType{EventMessage,
EventSessionFork}.
In `@internal/usage/report_test.go`:
- Around line 113-151: The test currently sets meta[0].ModelID == "" which
doesn't prove precedence; change the session metadata to a different valid model
id (e.g. meta[0].ModelID = "gpt-4") while leaving the event payload model as
"gpt-4.1" and keeping the expected cost derived from "gpt-4.1". Update the meta
slice in the test so BuildReport, events, and the registry (variable registry)
show a conflicting non-empty session model to force the code path that must
prefer the event's payload model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f33d840a-4fd0-4f73-bce8-5ee818141921
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/tools/registry.go
- internal/config/contracts_test.go
- internal/config/contracts.go
- internal/specialist/output_tool.go
All verified against current code; each behavioural fix has a regression test. Security / correctness: - tools/web_search: redirect policy now also refuses a scheme change, blocking a same-host https→http downgrade that would send the query + bearer token over plaintext. - sandbox/safe_command: a ')' is only a substitution boundary when it closes an active UNQUOTED $(...), and backticks now get the same quote-state save/restore as $( — so `echo "`a | less`"` and `echo "$(true | less)"` no longer hide the inner pager from interactive detection. - sandbox/analyzer + safe_command: wrapper option-value consumption stays wrapper-specific, and unwrapping no longer aborts on a dynamic ($x) wrapper arg once a wrapper is active, so `env "$opts" curl` / `sudo "$x" rm -rf` are still classified. - sandbox/egress: in-flight prompts deduped per host:port (carried from batch 38); the prompt callback is now cancellable (context cancelled on timeout) so a stuck prompt can abort instead of leaking; signature threaded through. - sandbox/runner: per-plan unique denial tag (pid+counter) so concurrent monitored sandbox-exec runs can't ingest each other's denials; helper selection now requires an executable regular file so a non-exec sibling degrades gracefully. - mcp/network_client: aggregate SSE event-size cap (not just per-line) so a server can't grow memory with many unterminated data: lines. - specialist/accounting: a previously-recorded stop/usage with an empty runId is a catch-all and now suppresses a later runId-bearing duplicate. - sessions/lineage: Tree fetches the root via Get() first so a corrupt root surfaces its real error instead of degrading to "not found". Docs/tests: - background/terminate: docstring now states the leader-only group-kill (PID-only fallback for non-leaders); test child-liveness probe uses ESRCH specifically. - cli/sandbox: grant usage strings include --path. Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and -race on sandbox/background all green.
Collaborator
Author
|
Addressed re-review round 3 in
CI green; @coderabbitai full review |
|
Quick notes on the key fixes:
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sessions/store.go (1)
709-713:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the metadata directory entry after rename for full crash durability.
Syncing the temp file is good, but rename durability is not guaranteed until the parent directory entry is synced. A crash after rename can still lose or roll back the metadata update.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store.go` around lines 709 - 713, The rename of tmp to path uses os.Rename but doesn't fsync the parent directory, so after a crash the rename may be lost; after the successful os.Rename(tmp, path) call, open the parent directory (use filepath.Dir(path)), call Sync() on that directory file (and Close it), and if the Sync fails return an error (preserving the existing error handling); keep the existing tmp removal on pre-rename error paths and ensure you propagate any directory-sync error so callers know the persist failed.
🧹 Nitpick comments (5)
internal/cli/cron_run_test.go (1)
132-145: ⚡ Quick winExpand this test to verify the “last error wins” contract.
The fixture currently has only one
errorevent, so a regression returning the first error would still pass. Add a seconderrorevent and assert the second message is returned.Proposed test update
output := strings.Join([]string{ `{"type":"run_start","runId":"r1"}`, `{"type":"text","delta":"working"}`, - `{"type":"error","message":"provider request failed: 500"}`, + `{"type":"error","message":"transient upstream error"}`, + `{"type":"error","message":"provider request failed: 500"}`, `{"type":"run_end","status":"error","exitCode":1}`, }, "\n") if got := extractStreamJSONError(output); got != "provider request failed: 500" { t.Fatalf("extractStreamJSONError = %q, want the error event message", got) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cron_run_test.go` around lines 132 - 145, The test should be expanded to assert the “last error wins” behavior: in the test block that builds output and calls extractStreamJSONError, add a second `{"type":"error","message":"..."}` event after the first (e.g. `{"type":"error","message":"provider request failed: 502"}`) and change the assertion to expect the second error message (e.g. "provider request failed: 502"); keep the existing assertion that a run_end with no error yields an empty string. Update the concrete string built in the test (the slice passed to strings.Join) and the expected value in the t.Fatalf checks to verify extractStreamJSONError returns the last error message.internal/sandbox/seccomp_linux.go (1)
22-39: 💤 Low valuePotential panic if
unixSocketBlockFilter()returns an empty slice.Line 33 dereferences
&kernelFilters[0]which will panic if the slice is empty. WhileunixSocketBlockFilter()presumably returns a static non-empty filter, a defensive check or documentation of this invariant would make the contract explicit.🛡️ Optional: Add defensive check
func ApplyUnixSocketBlock() error { if err := unix.Prctl(unix.PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); err != nil { return fmt.Errorf("seccomp: set no_new_privs: %w", err) } filters := unixSocketBlockFilter() + if len(filters) == 0 { + return fmt.Errorf("seccomp: empty filter program") + } kernelFilters := make([]unix.SockFilter, len(filters))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/seccomp_linux.go` around lines 22 - 39, The function ApplyUnixSocketBlock currently dereferences &kernelFilters[0] which will panic if unixSocketBlockFilter() returns an empty slice; add a defensive check after building kernelFilters to handle zero-length filters: if len(kernelFilters) == 0 return a descriptive error (e.g., fmt.Errorf("seccomp: empty filter from unixSocketBlockFilter")) and avoid taking &kernelFilters[0]; alternatively, set prog.Filter to nil when length is zero and only assign &kernelFilters[0] when len>0 so the call to unix.Prctl never dereferences an empty slice; update ApplyUnixSocketBlock accordingly and keep unixSocketBlockFilter() usage the same.internal/sandbox/egress.go (1)
326-340: 💤 Low valueStatic analysis:
net.LookupPortflagged bynoctxlinter.The linter flags this because
net.LookupPorthas a context-aware counterpart(*net.Resolver).LookupPort. However, for TCP with a numeric port string,LookupPortis effectively a pure parse operation with no I/O. If you want to silence the linter while keeping the same behavior, you can usestrconv.Atoidirectly or add a nolint directive.🔧 Optional: Silence lint with strconv
+ "strconv" ... func (proxy *egressProxy) authorizeTarget(target string, defaultPort int) bool { host := hostnameOnly(target) if host == "" { return false } port := defaultPort if _, portStr, err := net.SplitHostPort(target); err == nil { - if parsed, perr := net.LookupPort("tcp", portStr); perr == nil { + if parsed, perr := strconv.Atoi(portStr); perr == nil && parsed > 0 && parsed <= 65535 { port = parsed } } return proxy.authorize(host, port) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/egress.go` around lines 326 - 340, authorizeTarget uses net.LookupPort which the linter flags; replace the LookupPort call with a safe parse of the numeric port string to avoid context-aware resolver use. In authorizeTarget (and the inner logic that currently calls net.LookupPort), when net.SplitHostPort succeeds, parse portStr with strconv.Atoi (or strconv.ParseInt) and, on successful parse and valid port range, set port = parsed; otherwise leave defaultPort and proceed to call proxy.authorize(host, port). Keep hostnameOnly, net.SplitHostPort and proxy.authorize unchanged.Source: Linters/SAST tools
internal/sandbox/runner.go (1)
562-568: 💤 Low valueVerify that
denialTagis safe for seatbelt profile string interpolation.The
denialTagis interpolated directly into the seatbelt profile'sdeny default (with message "...")clause. WhilenextSandboxDenialTag()generates tags likezero-sandbox-denied-v1-<pid>-<seq>which are safe, consider passing it throughsandboxProfileString()for defense-in-depth against future changes to the tag format.🛡️ Optional hardening
if denialTag != "" { // Tag denials so the runtime log monitor can attribute them to THIS run; the // message is emitted to the unified log on every deny and StartDenialMonitor // filters `log stream` for this exact (per-plan) tag. - denyDefault = `(deny default (with message "` + denialTag + `"))` + denyDefault = `(deny default (with message "` + sandboxProfileString(denialTag) + `"))` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/runner.go` around lines 562 - 568, The denyDefault string currently interpolates denialTag directly; change the code that builds denyDefault to pass denialTag through sandboxProfileString() (e.g., sandboxProfileString(denialTag)) so the clause becomes `(deny default (with message " + sandboxProfileString(denialTag) + "))` (and keep the existing empty-case behavior), ensuring denialTag is safely escaped for the seatbelt profile; update the expression that sets denyDefault and any analogous interpolation sites to use sandboxProfileString() instead of raw denialTag.internal/zerogit/zerogit_test.go (1)
487-523: ⚡ Quick winAdd a
code[1]rename case to this parser regression test.Current coverage exercises staged rename (
R) only. Add a worktree rename (R) fixture so both rename columns are protected by tests.Suggested test extension
status := strings.Join([]string{ " M internal/a.go", // modified in worktree only "R new name.go", // staged rename; next field is the source "old name.go", // rename SOURCE — must be consumed, not its own entry + " R wt-new.go", // worktree rename; next field is the source + "wt-old.go", // rename SOURCE — must be consumed, not its own entry "A café.go", // staged add, non-ASCII path (no octal escaping) "?? un tracked.txt", // untracked, embedded space "", // trailing empty field after the final NUL }, "\x00") files := parseStatus(status) - if len(files) != 4 { - t.Fatalf("expected 4 entries (rename source consumed), got %d: %#v", len(files), files) + if len(files) != 5 { + t.Fatalf("expected 5 entries (rename sources consumed), got %d: %#v", len(files), files) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zerogit/zerogit_test.go` around lines 487 - 523, The test TestParseStatusZHandlesRenamesAndSpecialPaths only covers a staged rename (X='R') — add a worktree rename case (Y='R') by inserting an entry like " R worktree dest.txt" followed by its source "worktree src.txt" into the NUL-delimited status fixture passed to parseStatus, then update the expected count (len(files)) and assertions: assert the new destination entry appears (Path == "worktree dest.txt") with Unstaged true and Staged false, and keep the existing check that no rename source (e.g. "worktree src.txt") appears as its own entry; reference the test function TestParseStatusZHandlesRenamesAndSpecialPaths and the parseStatus call when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/compaction.go`:
- Around line 311-312: The lowWaterMark is being stored in a different token
domain than proactive checks: proactive checks compare
estimateTokens(messages)+toolTokens (variable size) against state.threshold,
while the reactive lowWaterMark is saved as a message-only token count; make
them consistent by changing the lowWaterMark assignment to use the same combined
token domain (estimateTokens(messages) + toolTokens) and ensure any comparisons
(the size <= state.threshold checks) all use that same combined value; locate
and update the assignments/conditions that reference size,
estimateTokens(messages), toolTokens, state.threshold, and lowWaterMark so both
proactive (lines around the size calculation) and reactive (where lowWaterMark
is set) use the identical combined token count.
In `@internal/mcp/network_client.go`:
- Around line 702-708: The SSE aggregate-size counter in the SSE handling block
currently does dataBytes += len(value) + 1 for every `data:` line which wrongly
counts a newline for the first line and can cause premature rejections; change
the accounting so you only add the joining newline when the aggregate already
has content (e.g., if dataBytes > 0 then add 1 for the newline, then add
len(value)). Update the calculation that compares dataBytes to maxSSEEventBytes
(the error message remains fine) — look for the variables/data flow around
dataBytes, value, and maxSSEEventBytes in the SSE parsing code in
network_client.go and adjust that increment logic accordingly.
In `@internal/sessions/lineage.go`:
- Around line 135-138: After calling store.Get(rootSessionID) check whether root
is nil before dereferencing it: if root == nil return a not-found error
(matching the package's existing not-found error type or creating a clear
errors.New("session not found") if none exists) instead of proceeding; apply the
same nil-guard at the second occurrence around the other store.Get call so both
spots avoid panics when a session is missing (refer to the
store.Get(rootSessionID) call and the variable root/err in lineage.go).
---
Outside diff comments:
In `@internal/sessions/store.go`:
- Around line 709-713: The rename of tmp to path uses os.Rename but doesn't
fsync the parent directory, so after a crash the rename may be lost; after the
successful os.Rename(tmp, path) call, open the parent directory (use
filepath.Dir(path)), call Sync() on that directory file (and Close it), and if
the Sync fails return an error (preserving the existing error handling); keep
the existing tmp removal on pre-rename error paths and ensure you propagate any
directory-sync error so callers know the persist failed.
---
Nitpick comments:
In `@internal/cli/cron_run_test.go`:
- Around line 132-145: The test should be expanded to assert the “last error
wins” behavior: in the test block that builds output and calls
extractStreamJSONError, add a second `{"type":"error","message":"..."}` event
after the first (e.g. `{"type":"error","message":"provider request failed:
502"}`) and change the assertion to expect the second error message (e.g.
"provider request failed: 502"); keep the existing assertion that a run_end with
no error yields an empty string. Update the concrete string built in the test
(the slice passed to strings.Join) and the expected value in the t.Fatalf checks
to verify extractStreamJSONError returns the last error message.
In `@internal/sandbox/egress.go`:
- Around line 326-340: authorizeTarget uses net.LookupPort which the linter
flags; replace the LookupPort call with a safe parse of the numeric port string
to avoid context-aware resolver use. In authorizeTarget (and the inner logic
that currently calls net.LookupPort), when net.SplitHostPort succeeds, parse
portStr with strconv.Atoi (or strconv.ParseInt) and, on successful parse and
valid port range, set port = parsed; otherwise leave defaultPort and proceed to
call proxy.authorize(host, port). Keep hostnameOnly, net.SplitHostPort and
proxy.authorize unchanged.
In `@internal/sandbox/runner.go`:
- Around line 562-568: The denyDefault string currently interpolates denialTag
directly; change the code that builds denyDefault to pass denialTag through
sandboxProfileString() (e.g., sandboxProfileString(denialTag)) so the clause
becomes `(deny default (with message " + sandboxProfileString(denialTag) + "))`
(and keep the existing empty-case behavior), ensuring denialTag is safely
escaped for the seatbelt profile; update the expression that sets denyDefault
and any analogous interpolation sites to use sandboxProfileString() instead of
raw denialTag.
In `@internal/sandbox/seccomp_linux.go`:
- Around line 22-39: The function ApplyUnixSocketBlock currently dereferences
&kernelFilters[0] which will panic if unixSocketBlockFilter() returns an empty
slice; add a defensive check after building kernelFilters to handle zero-length
filters: if len(kernelFilters) == 0 return a descriptive error (e.g.,
fmt.Errorf("seccomp: empty filter from unixSocketBlockFilter")) and avoid taking
&kernelFilters[0]; alternatively, set prog.Filter to nil when length is zero and
only assign &kernelFilters[0] when len>0 so the call to unix.Prctl never
dereferences an empty slice; update ApplyUnixSocketBlock accordingly and keep
unixSocketBlockFilter() usage the same.
In `@internal/zerogit/zerogit_test.go`:
- Around line 487-523: The test TestParseStatusZHandlesRenamesAndSpecialPaths
only covers a staged rename (X='R') — add a worktree rename case (Y='R') by
inserting an entry like " R worktree dest.txt" followed by its source "worktree
src.txt" into the NUL-delimited status fixture passed to parseStatus, then
update the expected count (len(files)) and assertions: assert the new
destination entry appears (Path == "worktree dest.txt") with Unstaged true and
Staged false, and keep the existing check that no rename source (e.g. "worktree
src.txt") appears as its own entry; reference the test function
TestParseStatusZHandlesRenamesAndSpecialPaths and the parseStatus call when
making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63779558-b828-4444-a241-ea0edc9a94b5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/config/contracts.go
- internal/specialist/output_tool.go
- internal/config/contracts_test.go
- internal/tools/registry.go
…ped) Verified each against current code. Fixed: - sessions/lineage: PANIC FIX — store.Get returns (nil, nil) for a missing session, so the batch-39 root prefetch could nil-deref. Guard root == nil and return a not-found error. Regression test added (was a panic). - agent/compaction: the reactive recover() now records lowWaterMark in the SAME combined (messages + tool-defs) token domain maybeCompact uses, so the proactive shrink-guard compares like with like. recover() takes tools; callers pass request.Tools. - mcp/network_client: SSE aggregate counter no longer counts a joining newline for the first data line (only when prior content exists), avoiding premature rejects. - sessions/store: fsync the parent directory after the metadata rename so the rename is durable across a crash (no-op on Windows, which can't fsync a dir). - sandbox/egress: parse the target port with strconv.Atoi instead of net.LookupPort (no context-unaware resolver use). - sandbox/runner: escape the per-plan denial tag through sandboxProfileString when embedding it in the seatbelt profile. - sandbox/seccomp_linux: guard against an empty filter program before taking &kernelFilters[0] (defensive; the program is fixed non-empty today). - cli/cron_run_test: assert last-error-wins with two error events. - background/terminate, cli/sandbox usage, process_posix_test ESRCH (carried). Skipped (with reason): - zerogit worktree-rename (Y='R') test: git porcelain v1 -z only emits the rename-source field when the INDEX column is R/C; the worktree column never carries 'R' with a source field in v1, so the fixture would test output git doesn't produce, and making parseStatus consume on code[1]=='R' would risk mis-consuming the following entry for a plain ` R` status. Left as-is. Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and -race on sandbox/background all green.
Collaborator
Author
|
Addressed re-review round 4 in Fixed
Skipped (with reason)
CI green; @coderabbitai full review |
|
Quick notes on the fixes:
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/egress.go (1)
200-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthorize absolute-form HTTPS requests with default port 443 (not 80).
Line 200 always calls
authorizeTarget(target, 80), but Line 210 preservesoutbound.URL.Scheme; forhttps://...absolute-form proxy requests, transport dials 443 while policy/prompt is evaluated for 80. This breaks port-scoped prompt/cache semantics and can allow 443 traffic under an 80 decision.Suggested fix
func (proxy *egressProxy) handleHTTP(w http.ResponseWriter, r *http.Request) { @@ - if !proxy.authorizeTarget(target, 80) { + defaultPort := 80 + if strings.EqualFold(r.URL.Scheme, "https") { + defaultPort = 443 + } + if !proxy.authorizeTarget(target, defaultPort) { proxy.logDecision("deny", r.Method, requestURLString(r)) http.Error(w, "scoped egress: host not allowed", http.StatusForbidden) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/egress.go` around lines 200 - 214, The code currently calls proxy.authorizeTarget(target, 80) before honoring the outbound URL scheme, which causes HTTPS absolute-form requests to be authorized against port 80; fix by deciding the default port from the request's scheme and using that when calling authorizeTarget: after creating outbound (r.Clone) and ensuring outbound.URL.Scheme is populated (if empty, set to "http"), compute defaultPort := 443 if outbound.URL.Scheme == "https" else 80, then call proxy.authorizeTarget(target, defaultPort) (and keep outbound.RequestURI = "" and setting outbound.URL.Host as-is).
♻️ Duplicate comments (1)
internal/zerogit/zerogit.go (1)
235-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume rename/copy source fields when
R/Cappears in either status column.Line 238 only checks
code[0]. For--porcelain -z, rename/copy can also be reported incode[1]; when that happens, the source-path field is left unconsumed and then misparsed as a new entry.Proposed fix
- // A rename/copy (R or C in the staged column) is followed by a separate + // A rename/copy (R or C in either status column) is followed by a separate // NUL-terminated field holding the original path; consume it so it is not // parsed as its own entry. This entry's own path is the destination. - if code[0] == 'R' || code[0] == 'C' { + if code[0] == 'R' || code[0] == 'C' || code[1] == 'R' || code[1] == 'C' { i++ }Please also add a regression case in
TestParseStatusZHandlesRenamesAndSpecialPathsfor an unstaged rename (" R <dest>\x00<src>\x00"), so this path stays covered.For `git status --porcelain -z` (v1), when `R` or `C` appears in the second status column (Y), does the entry still include an extra NUL-delimited source path field (`XY <dest>\0<src>\0`)?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zerogit/zerogit.go` around lines 235 - 240, The parser currently only consumes the extra NUL-delimited source path when code[0] is 'R' or 'C'; update the logic in the status parsing function in internal/zerogit/zerogit.go (the block that examines code) to check both code[0] and code[1] and consume the extra field if either is 'R' or 'C' (i.e., treat a rename/copy present in either status column the same); then add a regression case to TestParseStatusZHandlesRenamesAndSpecialPaths asserting that an unstaged rename entry like " R <dest>\x00<src>\x00" is parsed correctly so the extra src field is not treated as a separate entry.
🧹 Nitpick comments (1)
internal/sessions/store.go (1)
723-742: 💤 Low valueStatic analyzer flags swallowed error, but the best-effort design is intentional.
The linter flags
return nilat line 734 whenerr != nil. This is intentional per the docstring: dir sync is best-effort for durability. The file data is already synced and the rename succeeded; this just adds crash safety for the directory entry. Silencing the linter with a comment would clarify intent:d, err := os.Open(dir) if err != nil { - return nil + return nil //nolint:nilerr // best-effort: rename completed, dir sync optional }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store.go` around lines 723 - 742, In syncDir, the intentional early return on os.Open error (the err != nil branch) needs an explanatory comment and a linter suppression so the analyzer doesn't treat the swallowed error as a bug; update the err != nil branch in function syncDir to add a short comment like "// best-effort: directory sync not required or not supported on this platform; ignore open error" and add a nolint directive for the static analyzer that flags swallowed errors (for example "//nolint:staticcheck" or the project's preferred linter name) on that return so the intent is clear and the linter is silenced.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audit/2026-06-10-deep-audit-status.md`:
- Line 1: The narrative and the status table disagree on how the 78 re-verified
items split (narrative: 66 still_open + 12 partial; table: open: 64 + partial:
14), and the text naming three branch fixes (scanner.go:81, compaction.go:58,
worktrees.go:247) doesn't explain the redistribution; update the document so
they match by either (A) marking the narrative counts as pre-branch-fix and the
table counts as post-fix and adding one sentence that shows how the three fixes
moved 2 items from open to partial (or whatever the exact redistribution is), or
(B) if the original narrative was meant to be the final numbers, correct the
table to 66/12; ensure the chosen fix explicitly states which counts are pre- vs
post-fix, shows the arithmetic that sums to 78, and notes the effect of the
three branch fixes by name (scanner.go:81, compaction.go:58, worktrees.go:247).
In `@docs/audit/2026-06-13-reverification.md`:
- Around line 8-15: The counts in the reverification doc (still_open: 66,
partial: 12 → total 78) disagree with the related status doc (open: 64, partial:
14 → total 78); fix by reconciling and making the intent explicit: either (A)
update both documents (docs/audit/2026-06-13-reverification.md and
docs/audit/2026-06-10-deep-audit-status.md) to show both the "re-verification
baseline" and the "post-fix final" counts with clear labels (e.g.,
"reverification: still_open=66, partial=12" and "post-fix: open=64, partial=14")
and ensure the pending-by-severity numbers match the chosen breakdown, or (B)
pick a single canonical breakdown and update both files to that single set of
counts and adjust the pending-by-severity line accordingly; ensure any
explanatory note references the branch fixes (scanner.go:81, compaction.go:58,
worktrees.go:247) so readers understand why two numbers may exist.
In `@internal/agent/compaction.go`:
- Around line 435-440: The reduce pass error handling in summarizeMessagesOnce
needs to stop swallowing non-context errors: replace the current unconditional
"if reduceErr != nil { return combined, nil }" with a conditional that checks
whether reduceErr is a context cancellation/timeout (context.Canceled or
context.DeadlineExceeded) and only in that case return (combined, nil); for any
other reduceErr return the error upward (e.g., return "", reduceErr) so
auth/network/provider failures surface; reference the summarizeMessagesOnce call
and the reduceErr variable and add an errors/context check accordingly (import
errors/context if not already).
In `@internal/sandbox/safe_command.go`:
- Around line 198-222: The wrapper value-option logic only recognizes short
flags, so long flags like "--user" are treated as program tokens; update
wrapperValueOptionsByProg and wrapperConsumesValue to handle long-form options
(e.g., store/compare both short and long names or normalize by trimming leading
'-' and matching canonical names) so scanning skips the following token when a
long flag consumes a value; update wrapperConsumesValue to canonicalize option
(strip leading dashes and check both the original and canonical keys) and extend
wrapperValueOptionsByProg entries where needed for wrappers that have long
equivalents (e.g., map "--user"/"user" for sudo), and add regression tests
asserting DetectInteractiveCommand("sudo --user root vim file.txt", "linux")
blocks on vim and AnalyzeCommand("sudo --user root rm -rf /tmp/x") marks
destructive.
---
Outside diff comments:
In `@internal/sandbox/egress.go`:
- Around line 200-214: The code currently calls proxy.authorizeTarget(target,
80) before honoring the outbound URL scheme, which causes HTTPS absolute-form
requests to be authorized against port 80; fix by deciding the default port from
the request's scheme and using that when calling authorizeTarget: after creating
outbound (r.Clone) and ensuring outbound.URL.Scheme is populated (if empty, set
to "http"), compute defaultPort := 443 if outbound.URL.Scheme == "https" else
80, then call proxy.authorizeTarget(target, defaultPort) (and keep
outbound.RequestURI = "" and setting outbound.URL.Host as-is).
---
Duplicate comments:
In `@internal/zerogit/zerogit.go`:
- Around line 235-240: The parser currently only consumes the extra
NUL-delimited source path when code[0] is 'R' or 'C'; update the logic in the
status parsing function in internal/zerogit/zerogit.go (the block that examines
code) to check both code[0] and code[1] and consume the extra field if either is
'R' or 'C' (i.e., treat a rename/copy present in either status column the same);
then add a regression case to TestParseStatusZHandlesRenamesAndSpecialPaths
asserting that an unstaged rename entry like " R <dest>\x00<src>\x00" is parsed
correctly so the extra src field is not treated as a separate entry.
---
Nitpick comments:
In `@internal/sessions/store.go`:
- Around line 723-742: In syncDir, the intentional early return on os.Open error
(the err != nil branch) needs an explanatory comment and a linter suppression so
the analyzer doesn't treat the swallowed error as a bug; update the err != nil
branch in function syncDir to add a short comment like "// best-effort:
directory sync not required or not supported on this platform; ignore open
error" and add a nolint directive for the static analyzer that flags swallowed
errors (for example "//nolint:staticcheck" or the project's preferred linter
name) on that return so the intent is clear and the linter is silenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e31cd7f3-defa-4535-8a99-399d5f137b10
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/config/contracts.go
- internal/config/contracts_test.go
- internal/tools/registry.go
- internal/specialist/output_tool.go
All verified against current code; code fixes have regression tests. - agent/compaction: summarizeWithFallback's reduce pass no longer swallows ALL errors. A context-limit error still falls back to the joined partial summaries (documented "extreme" case), but a non-context failure (auth/network/provider) now surfaces unchanged, per the function's contract. Two tests (propagate vs. joined-fallback); the propagate test fails without the fix. - sandbox/safe_command + analyzer: wrapper value-consuming options now include the long spellings (sudo --user root, env --unset NAME, timeout --signal …), so a long flag's value is no longer mis-read as the program (interactive/destructive/ network detection bypass). A `--flag=value` token carries its own value and never consumes the next token. Tests for space and = forms in both the AST analyzer and the regex detector. - docs/audit: the status-ledger summary table's open/partial cells (64/14) were the lone outlier — the ledger's own narrative and 2026-06-13-reverification.md both state 66/12. Corrected to 66/12 (66+12+97 = 175); derivation: body 69 open + 18 partial minus the 9 already-fixed-on-main (3 open + 6 partial) = 66/12 = 78 pending, fixed 88+9 = 97. Added a comment documenting the summary-vs-body relationship. The reverification doc already states 66/12, so both now agree. Verified: gofmt + go vet clean; host + linux + windows builds; full go test ./... green.
Collaborator
Author
|
Addressed re-review round 5 in
CI green; @coderabbitai full review |
|
Quick notes on the four fixes:
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/cli/sandbox_test.go (1)
159-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert store immutability after each rejected empty
--pathcase.Line [179]-Line [185] validates store state only once at the end. A mutation in one rejected call could be masked by later calls and still end at zero grants. Capture
store.List()before each invocation and compare immediately after each failed call.Suggested patch
import ( "bytes" "encoding/json" "errors" "fmt" "path/filepath" + "reflect" "strings" "testing" ) @@ for _, args := range [][]string{ {"sandbox", "grants", "allow", "write_file", "--path", ""}, {"sandbox", "grants", "allow", "write_file", "--path="}, {"sandbox", "grants", "revoke", "write_file", "--path", ""}, {"sandbox", "grants", "revoke", "write_file", "--path="}, } { + before, err := store.List() + if err != nil { + t.Fatalf("%v: List before call: %v", args, err) + } if exit := run(args...); exit == exitSuccess { t.Fatalf("%v: expected a usage error for an empty --path, got success", args) } @@ if !strings.Contains(stderr.String(), "path") { t.Fatalf("%v: stderr should explain the empty --path, got %q", args, stderr.String()) } + after, err := store.List() + if err != nil { + t.Fatalf("%v: List after call: %v", args, err) + } + if !reflect.DeepEqual(after, before) { + t.Fatalf("%v: rejected call mutated grants; before=%#v after=%#v", args, before, after) + } } - - // No grant should have been created or removed by any of the rejected calls. - grants, err := store.List() - if err != nil { - t.Fatalf("List: %v", err) - } - if len(grants) != 0 { - t.Fatalf("expected no grant mutation from rejected --path calls, got %#v", grants) - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sandbox_test.go` around lines 159 - 185, The test currently checks store.List() only once after iterating several rejected empty --path cases, which can mask per-call mutations; update the loop around run(args...) to call store.List() before invoking run (e.g., prevGrants, _ := store.List()), then after each rejected run call immediately call store.List() again and assert equality with prevGrants (using the same error handling and t.Fatalf messages currently used), referencing the existing run(...) invocation and store.List() usage so each rejected "--path" case verifies the store is unchanged in isolation.internal/zerogit/zerogit.go (1)
238-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume rename/copy source fields when
R/Cis present in either status column.On Line [238], the parser only consumes the extra
\x00<src>field whencode[0]isR/C. In porcelain-z, rename/copy can be signaled incode[1]too, which misaligns subsequent parsing.Proposed fix
- // A rename/copy (R or C in the staged column) is followed by a separate + // A rename/copy (R or C in either status column) is followed by a separate // NUL-terminated field holding the original path; consume it so it is not // parsed as its own entry. This entry's own path is the destination. - if code[0] == 'R' || code[0] == 'C' { + if code[0] == 'R' || code[0] == 'C' || code[1] == 'R' || code[1] == 'C' { i++ }In `git status --porcelain -z` (v1), does a rename/copy entry include an extra source-path field when either status column (X or Y) is `R` or `C`?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zerogit/zerogit.go` around lines 238 - 240, The parser currently only consumes the extra "\x00<src>" field when code[0] is 'R' or 'C'; update the condition to consume that source field if either status column indicates rename/copy by checking both code[0] and code[1] (e.g., if code[0]=='R'||code[0]=='C'||code[1]=='R'||code[1]=='C'), ensuring you handle the expected two-character status string in variable code, and then increment i (the index that skips the extra field) as before so subsequent parsing stays aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/cli/sandbox_test.go`:
- Around line 159-185: The test currently checks store.List() only once after
iterating several rejected empty --path cases, which can mask per-call
mutations; update the loop around run(args...) to call store.List() before
invoking run (e.g., prevGrants, _ := store.List()), then after each rejected run
call immediately call store.List() again and assert equality with prevGrants
(using the same error handling and t.Fatalf messages currently used),
referencing the existing run(...) invocation and store.List() usage so each
rejected "--path" case verifies the store is unchanged in isolation.
In `@internal/zerogit/zerogit.go`:
- Around line 238-240: The parser currently only consumes the extra "\x00<src>"
field when code[0] is 'R' or 'C'; update the condition to consume that source
field if either status column indicates rename/copy by checking both code[0] and
code[1] (e.g., if code[0]=='R'||code[0]=='C'||code[1]=='R'||code[1]=='C'),
ensuring you handle the expected two-character status string in variable code,
and then increment i (the index that skips the extra field) as before so
subsequent parsing stays aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 245490b2-1bed-49a9-a2b3-2c5c8aaf2360
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
README.mdcmd/zero-seccomp/main.godocs/audit/2026-06-10-deep-audit-status.mddocs/audit/2026-06-13-reverification.mdgo.modinternal/agent/compaction.gointernal/agent/compaction_summarizer_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/terminate.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_sessions.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/config/contracts.gointernal/config/contracts_test.gointernal/config/resolver_test.gointernal/config/types.gointernal/cron/next_test.gointernal/cron/schedule.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/network_client.gointernal/mcp/network_client_test.gointernal/providers/factory.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/egress.gointernal/sandbox/egress_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/log_monitor_darwin.gointernal/sandbox/log_monitor_darwin_test.gointernal/sandbox/log_monitor_other.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/types.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/sessions/lineage.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/accounting.gointernal/specialist/accounting_test.gointernal/specialist/exec.gointernal/specialist/output_tool.gointernal/specialist/streamer.gointernal/specialist/streamer_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/web_fetch.gointernal/tools/web_fetch_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/usage/report.gointernal/usage/report_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.gointernal/zeroruntime/helpers.go
💤 Files with no reviewable changes (4)
- internal/config/contracts_test.go
- internal/specialist/output_tool.go
- internal/config/contracts.go
- internal/tools/registry.go
… w/ proof) CodeRabbit re-surfaced two "duplicate" refinements; CI was already green (its 6th pass was a COMMENT with no new findings). - cli/sandbox_test (TestRunSandboxGrantsRejectsEmptyPath): now seeds a tool-wide grant and asserts grant-store immutability after EACH rejected empty-`--path` call (reflect.DeepEqual before/after), so a mutation in one call can't be masked by a later one — and a buggy "revoke all" is actually observable (it's a no-op on an empty store). Stronger than the suggested patch. - zerogit.parseStatus: KEPT consuming the rename source on the index column only (code[0]), with an explanatory comment. Verified empirically that git porcelain v1 -z reports a rename/copy (and emits the extra source field) only in the index column: `git mv` → "R new\0old\0"; a worktree-only rename → " D old\0?? new\0" (delete + untracked, never "R" in code[1]). Consuming on code[1]=='R'/'C' would match no real git output and would only risk mis-consuming the next entry on malformed input, so the suggestion is declined as incorrect for v1. Verified: gofmt + go vet clean; host + linux + windows builds; affected suites green.
Collaborator
Author
|
Round 6 (
gofmt + vet clean; host/linux/windows builds; suites green. @coderabbitai review |
|
Kicking off the full re-review now. ✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Remediation of findings from the 2026-06-10 deep audit, plus a focused sandbox-hardening pass. Every finding was re-verified against current
mainbefore fixing — the ledger had drifted, so ~11 rows were already fixed and several were intentional/test-guarded (not defects). Every behavioural change ships with a test;gofmt,go vet ./..., host +GOOS=linux+GOOS=windowsbuilds, and the full suite are green on CI (all four Smoke jobs: ubuntu, macos, windows, performance). TUI findings are excluded (owned by a separate feature branch).Scope: 89 files, ~+3.4k/−0.3k, 36 commits, no behavioural change to any default path unless a new opt-in flag is set.
Sandbox hardening (batches 23–34)
The largest part of this PR. It deepens the existing macOS seatbelt / Linux bubblewrap sandbox and closes the gap where non-shell tools bypassed the network policy. Every new enforcement feature is opt-in and off by default, so existing behaviour is unchanged unless explicitly enabled.
Network policy now covers the built-in network tools (the real gap)
web_fetchandweb_searchhonour the scoped/deny policy (batches 33–34). Previously the egress proxy only constrained sandboxed shell commands;web_fetch/web_searchopened their own connections and could reach any public host even underdeny/scoped. A new shared gateEngine.NetworkHostAllowed(host)reuses the existing egress domain matcher as the single source of truth (allow → all; deny → none; scoped → allowlist minus denylist; empty allowlist collapses to deny).web_fetchchecks the host before the request and on every redirect, failing closed before any dial; the no-sandbox path is byte-for-byte unchanged.web_searchchecks the configured backend's endpoint host before the query leaves the machine.sandbox-execbackend (the only one that can route scoped egress); other backends collapsescopedtodenyfor these tools, which the engine already enforced at evaluation time.RunWithOptions → Evaluate → RunWithSandbox → per-host allowlist blocks an unlisted host before any dial.Backend isolation depth
com.apple.*mach-lookupallowlist and(allow signal (target self) (target pgrp)), so common toolchains work under the profile while the default-deny posture and the workspace write-boundary still hold (verified live on the host).AF_UNIXblock (batches 30, 32): a classic-BPF seccomp program that denies Unix-socket creation (the channel bubblewrap's filesystem/network isolation leaves open), delivered as azero-seccompexec-wrapper and wired into the bubblewrap command viaPolicy.BlockUnixSockets. Discovery finds the helper next to the binary or onPATHand degrades gracefully (runs without the filter) when absent.Policy.MonitorDenials, the seatbelt profile tags its default-deny and the bash path tails the unified log for that tag, surfacing blocked operations back to the agent as a<sandbox_violations>block on stderr.Permission / egress model
--pathon the sandbox allow/revoke CLI), not just tool-wide.Engine.Precheck(batch 26): report the violations that would block a request before it runs, so callers can fail fast with the reason.AnalyzeCommandparses commands with a real shell grammar instead of string heuristics."a | b","a; vim b") no longer split, so quoted program names aren't falsely flagged while real unquoted operators still do.ProxyEnv(batch 29); remove the deadOnSandboxDecisionhook (batch 24).Config surface
Both new hardening flags are reachable from the
sandboxconfig block and documented in the README —sandbox.blockUnixSockets(Linux) andsandbox.monitorDenials(macOS) — and map onto the policy opt-in only (config can turn a flag on, never silently off).Correctness / robustness (batches 5, 6, 13, 15, 16, 18, 22)
git status --porcelain -z: renames report the destination path; non-ASCII/whitespace filenames arrive verbatim, not C-quoted.maxTurnsinstead of silently dropping it; remove dead contract-gap code.Setpgid+ negative-PID signal + bounded wait) so forked children can't outlive a cancel; on aSetPIDfailure, kill + record stop instead of orphaning.stream-jsonstdout instead of a generic failure.Resource / durability / perf (batches 7, 9, 10, 11, 12, 17)
Treefrom oneList()snapshot, killing an N+1 disk scan.Usage / accounting (batches 19, 20)
Tooling (batches 4, 8, 21)
list-tools -o json; interrupted-o jsonruns emit a terminal event; surface session-record failures.read_fileemits a truncation marker on amax_linescut; remove a provably-dead provider-resolution branch.CI / portability (batch 35)
TestWriteFileSyncRoundTripsasserted an exact Unix file mode (0o600), which Windows doesn't honour (Go reports0o666). The exact-perm check is now guarded to the platforms that enforce it; the write/read-back round-trip is still asserted everywhere.Testing & verification
gofmt -lclean ·go vet ./...clean.GOOS=linux go build ./...(incl. thezero-seccompwrapper), andGOOS=windows go build ./...all pass.go test ./...green; all four CI Smoke jobs (ubuntu / macos / windows / performance) pass.Deliberately left (documented in commits)
Items that are intentional/test-guarded (e.g.
ResolveMCPnot running the provider command), perf-only with cross-process-unsafe shortcuts, nuanced usage-accounting edge cases, or low-value/forward-looking dead code. The seccomp/denial-monitor runtime validation is the only known follow-up and is gated behind default-off flags.Summary by CodeRabbit
New Features
Configuration
Bug Fixes