Fix all critical & high deep-audit findings - #167
Merged
Conversation
- protocol.go read() rejected no upper bound on Content-Length, so a hostile peer could drive make([]byte, n) with n up to ~9.2e18. Cap every framed message at 64 MiB (both the Content-Length and newline-delimited paths). - The MCP stdio transport is newline-delimited JSON, not LSP Content-Length framing. write() now emits one JSON message per line; read() accepts newline-delimited frames and still tolerates Content-Length for back-compat, so Zero interoperates with spec-compliant MCP servers and clients.
- grep matched the glob against the workspace-relative path, so "*.go" with path="subdir" found nothing while "**/*.go" matched subdir/a.go. Match the glob relative to the search directory (ripgrep semantics); a single explicit file is matched by its base name. - apply_patch parsed file paths per-line, so a hunk-body line removing content that starts with "-- " (appearing as "--- ...") was mistaken for a file header and a valid patch was rejected. Parse headers with hunk line-counting, matching how git apply delimits hunks.
The sk- and bearer patterns were unanchored, so "sk-" inside "task-list" and the word after a bare "bearer" were replaced with [REDACTED], corrupting legitimate output. Anchor sk- on a word boundary and require a credential-length body for both sk- and bearer tokens; real keys/tokens are still redacted.
ReadEvents returned an error on any malformed JSONL line, so a single interrupted append (truncated final line) made resume hard-fail and lose the whole session. Drop only a malformed final line (a torn tail); malformed lines earlier in the file are real corruption and still error.
- `cron add "<expr>" --recipe X` silently dropped the explicit schedule because the recipe set expr before the positional arg was consumed. Consume the positional expr first so it overrides the recipe default. - cron_run persisted the job copy read at tick start, so a pause or removal that happened during the run was clobbered back to active. Re-read before persisting and preserve an external pause/removal (best-effort; full atomicity needs a lock).
A specialist run with no description set --session-title to just the description (empty), leaving AgentName empty on resume and making the session unresumable. Always prefix the title with the specialist name and fall back to the bare name when no description is given.
…children On timeout the context cancelled only the shell, leaving backgrounded children (e.g. 'sleep 3 & wait') orphaned. Worse, those children inherited the stdout/stderr pipes, so Run() blocked until they exited — far past the timeout. Set Setpgid and a Cancel that SIGKILLs the whole process group on POSIX, plus a WaitDelay backstop so a lingering child can't hang Wait. On Windows, set WaitDelay only (no process groups).
The default probe used http.DefaultClient, which (1) followed redirects without revalidating the target, so a 3xx to an internal address bypassed the pre-flight endpoint check, and (2) re-resolved DNS at dial time, leaving a rebinding TOCTOU window after validateEndpoint. Build a dedicated client that revalidates every redirect hop (capped at 5) and validates + pins the resolved IP at dial time, dialing the IP literal so the kernel cannot re-resolve to an unsafe address.
The macOS sandbox-exec profile made only the workspace writable, so ubiquitous operations failed with 'Operation not permitted' — `> /dev/null` and mktemp among them. The bubblewrap backend already grants these via --dev /dev and --tmpfs /tmp; bring the sandbox-exec profile to parity by allowing the standard device nodes and the system temp trees. The workspace remains the only writable project location.
Providers already normalized finish_reason into collected.FinishReason, but the agent loop never read it, so a final answer cut off at the output token cap (or by a content filter) was silently presented as complete. Carry the turn's finish reason into agent.Result, add Truncated()/TruncationNotice(), and emit a warning from the exec writer and a system row in the TUI when the response was truncated.
…quest zeroruntime.CompletionRequest had no effort field, so agent.Options.Reasoning Effort never reached the provider (cli/exec.go even noted it was 'not yet forwarded'). Add CompletionRequest.ReasoningEffort, thread it through every request the agent loop builds, and map it to OpenAI's reasoning_effort. The CLI gates the forwarded value against the resolved model: known non-reasoning models send nothing (matching the existing 'ignoring' advisory), known reasoning models send their effective level, and unknown custom endpoints forward the requested value as-is. Anthropic/Gemini mapping is handled separately.
Map a requested reasoning effort to an Anthropic thinking budget and enable extended thinking, raising max_tokens so the budget plus a response both fit. Crucially, capture the streamed thinking / redacted_thinking blocks (text + signature) and preserve them on the assistant message so they are replayed verbatim at the start of the next turn — without this, Anthropic rejects every tool-using follow-up. Adds zeroruntime carriers (Message.Reasoning, ToolCall. Signature, ReasoningBlock) shared with the upcoming Gemini work. No effort = no thinking, so default runs are unchanged.
Map a requested reasoning effort to a Gemini thinkingBudget (capped at the lowest per-model ceiling). Capture the thoughtSignature each functionCall part carries and preserve it on the tool call (via a new StreamEvent/ToolCall signature field), then replay it on the functionCall part in later turns so multi-turn function calling with thinking is not rejected. Thought-summary parts are skipped so reasoning never leaks into the answer. No effort = no thinking config, so default runs are unchanged.
The torn-tail tolerance dropped any malformed final line, including a complete-but-corrupt line ending in a newline — silently swallowing real corruption. A genuine torn tail is an incomplete write with no trailing newline; gate the tolerance on that so a fully-flushed corrupt line fails loudly again, while a truncated final line is still recovered.
The package had all the primitives (Select, ConfigStore, AuditStore) but no caller ever ran a hook. Add a Dispatcher that selects the hooks for a lifecycle event, runs each command (no shell) with the event payload on stdin under a timeout, and records started/completed audit events. A beforeTool hook that exits non-zero vetoes the tool and stops the chain; other events are advisory. A hook that cannot be launched is recorded as an error but never blocks. The command runner is injectable for testing.
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. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds end-to-end reasoning/replay (ReasoningEffort, ReasoningBlocks, tool-call signatures, FinishReason) across runtime, providers, agent/CLI/TUI; introduces a hook dispatcher with blocking before-tool semantics and audits; hardens MCP framing and provider connectivity; and updates sandbox, bash process lifetime, apply_patch parsing, grep globing, cron/store behavior, session JSONL robustness, redaction regexes, and many tests. ChangesExtended Thinking and Reasoning Support
Hook Dispatch System
MCP Protocol and Provider Health Hardening
Tool and Process Hardening
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 10
🧹 Nitpick comments (3)
internal/mcp/protocol_test.go (1)
27-36: ⚡ Quick winAdd a regression test for EOF-terminated final JSON line.
messageReader.readhas special logic for a final message without trailing\n, but that path is not explicitly tested. Add one focused test to prevent regressions in tail handling.Proposed test addition
func TestMessageReaderAcceptsNewlineDelimited(t *testing.T) { in := strings.NewReader(`{"jsonrpc":"2.0","method":"tools/list"}` + "\n") msg, err := newMessageReader(in).read() if err != nil { t.Fatalf("read: %v", err) } if msg.Method != "tools/list" { t.Fatalf("method=%q want tools/list", msg.Method) } } + +func TestMessageReaderAcceptsFinalLineWithoutTrailingNewline(t *testing.T) { + in := strings.NewReader(`{"jsonrpc":"2.0","method":"tools/list"}`) + msg, err := newMessageReader(in).read() + if err != nil { + t.Fatalf("read: %v", err) + } + if msg.Method != "tools/list" { + t.Fatalf("method=%q want tools/list", msg.Method) + } +}🤖 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/mcp/protocol_test.go` around lines 27 - 36, Add a regression test that verifies newMessageReader.read correctly handles a final JSON-RPC message that ends at EOF without a trailing newline: create a TestMessageReaderAcceptsEOFTerminated (or similar) in internal/mcp/protocol_test.go that constructs an io.Reader from a JSON-RPC message string without a trailing "\n", calls newMessageReader(...).read(), asserts no error, and checks msg.Method == "tools/list" (matching the existing newline test's assertions) to ensure the EOF-terminated path in read() is exercised.internal/tools/bash_tool_test.go (1)
254-288: ⚡ Quick winUse the test hook for
bashWaitDelayso this regression doesn’t add 3.5s to every run.This test can stay deterministic with a much shorter child sleep by temporarily lowering
bashWaitDelayinside the test. That keeps the regression coverage without baking a multi-second delay into the suite.Proposed test tightening
func TestBashToolTimeoutKillsBackgroundChildren(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("process-group kill is POSIX-only") } + oldWaitDelay := bashWaitDelay + bashWaitDelay = 100 * time.Millisecond + defer func() { bashWaitDelay = oldWaitDelay }() + root := t.TempDir() sentinel := filepath.Join(root, "leaked") @@ - command := fmt.Sprintf("(sleep 3; touch %s) & wait", shellQuote(sentinel)) + command := fmt.Sprintf("(sleep 1; touch %s) & wait", shellQuote(sentinel)) @@ - time.Sleep(3500 * time.Millisecond) + time.Sleep(1200 * time.Millisecond) if _, err := os.Stat(sentinel); err == nil { t.Fatalf("background child survived the timeout and wrote %s", sentinel) } else if !os.IsNotExist(err) { t.Fatalf("unexpected error stat-ing sentinel: %v", err) } }🤖 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/tools/bash_tool_test.go` around lines 254 - 288, The test should use the test hook for bashWaitDelay to avoid multi-second sleeps: inside TestBashToolTimeoutKillsBackgroundChildren save the original := bashWaitDelay, set bashWaitDelay = 10 * time.Millisecond (or similar small duration) before constructing command, defer restoring the original, and shorten the child sleep and final verification sleep accordingly (e.g., change "sleep 3" to "sleep 30ms" in command and the final time.Sleep(3500 * time.Millisecond) to ~100ms) so the test remains deterministic without long delays; keep references to TestBashToolTimeoutKillsBackgroundChildren and the package-level variable bashWaitDelay when making the change.internal/providers/openai/provider.go (1)
350-360: ⚡ Quick winRemove redundant normalization in the return statement.
Line 356 calls
strings.ToLower(strings.TrimSpace(requested))again, butrequestedwas already normalized at line 354. Return the pre-normalized value directly.♻️ Proposed fix
func openAIReasoningEffort(requested string) string { - switch strings.ToLower(strings.TrimSpace(requested)) { + normalized := strings.ToLower(strings.TrimSpace(requested)) + switch normalized { case "minimal", "low", "medium", "high": - return strings.ToLower(strings.TrimSpace(requested)) + return normalized default: 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/providers/openai/provider.go` around lines 350 - 360, The function openAIReasoningEffort re-normalizes requested twice; capture the normalized value once (e.g., assign norm := strings.ToLower(strings.TrimSpace(requested))) and use that for the switch and as the return value for the allowed cases instead of calling strings.ToLower(strings.TrimSpace(requested)) again; update the switch in openAIReasoningEffort to return the pre-normalized variable for "minimal","low","medium","high" and keep default returning "".
🤖 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/cli/cron_run.go`:
- Around line 175-177: Introduce a sentinel not-found error in the cron store
(e.g. var ErrNotFound = errors.New("cron job not found")) and change Store.Get
to return errors.Is-compatible wrapping: return ErrNotFound (or fmt.Errorf("%w:
%v", ErrNotFound, err)) only when the underlying failure is os.IsNotExist; other
IO/parsing errors must be returned unchanged. In cron_run (the code path that
calls Store.Get), only take the “job removed during run” early-return when
errors.Is(err, store.ErrNotFound); for any other Get error, do not swallow it —
log or surface it and continue to record the run via store.AppendRun and
store.Update (or fail loudly per desired policy). Add unit tests for Store.Get
that assert ErrNotFound for missing metadata and a non-ErrNotFound for
corrupt/unreadable metadata, and tests for cron_run behavior for both cases
(removed vs other error).
In `@internal/cli/cron_test.go`:
- Around line 111-113: The current assertion only checks jobs[0].Prompt is
non-empty which can pass via a fallback; update the test to assert that the job
prompt equals (or contains) the recipe's explicit prompt value to ensure
propagation. Locate the test's recipe setup (the recipe instance or
expectedPrompt variable used to create the job) and replace the empty-check on
jobs[0].Prompt with a strict comparison to that expected prompt (e.g., compare
to recipe.Prompt or expectedPrompt) and include a helpful failure message
referencing jobs and the expected value.
In `@internal/hooks/dispatch.go`:
- Around line 159-178: classifyResult currently maps any result.Err to
AuditError which lets timed-out/cancelled beforeTool hooks (from
execCommandRunner) fail-open; change classifyResult to treat
context.DeadlineExceeded and context.Canceled (use errors.Is(result.Err,
context.DeadlineExceeded) / errors.Is(result.Err, context.Canceled)) as vetoes
by returning (AuditBlocked, true) and ensure the blocking reason is surfaced by
using blockReason(result) (or include result.Err.Error() if no output) so
timeouts show as a block reason; update tests by adding a regression next to the
launch-failure test to assert that a timed-out beforeTool hook results in
AuditBlocked and includes the timeout/cancel reason.
In `@internal/providerhealth/providerhealth.go`:
- Around line 455-467: safeDialContext validates all DNS answers but only tries
addrs[0], causing false failures; change the dial step so after
resolver.LookupNetIP and the blockedAddrReason checks you iterate over addrs and
attempt dialer.DialContext for each net.JoinHostPort(addr.String(), port) until
one succeeds, returning that connection immediately; if none succeed, return an
endpointSafetyError (or the last dial error) so callers get failure only after
every validated address was tried; reference resolver.LookupNetIP,
blockedAddrReason, dialer.DialContext, network, port, and addrs in your changes.
In `@internal/providers/anthropic/provider.go`:
- Around line 437-441: The replay logic in the switch handling block.Type inside
provider.go omits the signature for the "redacted_thinking" case, causing
replayed request shapes to diverge; update the "redacted_thinking" branch (the
switch that appends to blocks) to include the "signature" field from
block.Signature alongside "data" (mirroring the "thinking" case) so replayed
artifacts retain both data and signature when building the next Anthropic
request.
- Around line 336-340: emitDone currently calls state.closeOpen() but doesn't
finalize open entries in state.thinking before sending the StreamEventDone via
providerio.SendEvent, so any pending thinking deltas are dropped; fix by
ensuring you finalize/flush open thinking buffers (e.g., call a new or existing
method like state.finalizeThinking() or state.flushThinkingBuffers that
moves/merges entries from state.thinking into state.reasoningBlocks) immediately
after state.closeOpen() and before providerio.SendEvent, and if such a helper
doesn't exist implement it to convert partial thinking buffers into completed
reasoning block entries so the final StreamEventDone contains consistent
reasoningBlocks.
In `@internal/providers/gemini/types.go`:
- Around line 15-20: Update the comment for type thinkingConfig to reflect
actual behavior: note that mapped.GenerationConfig.ThinkingConfig is only
assigned when thinkingBudgetForEffort(request.ReasoningEffort) > 0 (so a zero
budget is not sent to Gemini), and remove the incorrect reference to an
IncludeThoughts request field; also mention that thought summaries are
suppressed by skipping streamed parts where part.Thought is true rather than via
an IncludeThoughts flag. Keep references to thinkingConfig,
mapped.GenerationConfig.ThinkingConfig, thinkingBudgetForEffort, and
part.Thought so reviewers can locate the relevant code.
In `@internal/sessions/store_test.go`:
- Line 106: The call to file.Close() in the test is ignoring its returned error;
update the test in internal/sessions/store_test.go to capture and assert the
close succeeded (e.g., err := file.Close(); require.NoError(t, err) or t.Fatalf
on error) so the file is flushed/closed before reading it back; reference the
existing file variable and its Close() call to locate where to add the check.
- Line 135: The call to file.Close() in the test ignores its returned error;
change it to check and fail the test on error (e.g. capture err := file.Close()
and call t.Fatalf or use require.NoError(t, err)) so any flush/close failure is
surfaced before the test reads the file back; update the invocation of
file.Close() accordingly in the test in store_test.go.
In `@internal/tools/apply_patch.go`:
- Around line 196-206: parseHunkCounts currently scans the whole line and picks
up any +/- tokens after the second "@@", allowing an attacker to append e.g.
"+1,999" to override counts; restrict parsing to only the hunk header section by
extracting the substring between the first "@@" and the second "@@" (or to the
end if second "@@" missing), then run strings.Fields on that segment and pass
the numeric parts to hunkCount (referencing parseHunkCounts and hunkCount); this
prevents tokens outside the hunk header from influencing old/next counts and
stops patchHeaderPaths / validatePatchPaths from being confused by injected
tokens.
---
Nitpick comments:
In `@internal/mcp/protocol_test.go`:
- Around line 27-36: Add a regression test that verifies newMessageReader.read
correctly handles a final JSON-RPC message that ends at EOF without a trailing
newline: create a TestMessageReaderAcceptsEOFTerminated (or similar) in
internal/mcp/protocol_test.go that constructs an io.Reader from a JSON-RPC
message string without a trailing "\n", calls newMessageReader(...).read(),
asserts no error, and checks msg.Method == "tools/list" (matching the existing
newline test's assertions) to ensure the EOF-terminated path in read() is
exercised.
In `@internal/providers/openai/provider.go`:
- Around line 350-360: The function openAIReasoningEffort re-normalizes
requested twice; capture the normalized value once (e.g., assign norm :=
strings.ToLower(strings.TrimSpace(requested))) and use that for the switch and
as the return value for the allowed cases instead of calling
strings.ToLower(strings.TrimSpace(requested)) again; update the switch in
openAIReasoningEffort to return the pre-normalized variable for
"minimal","low","medium","high" and keep default returning "".
In `@internal/tools/bash_tool_test.go`:
- Around line 254-288: The test should use the test hook for bashWaitDelay to
avoid multi-second sleeps: inside TestBashToolTimeoutKillsBackgroundChildren
save the original := bashWaitDelay, set bashWaitDelay = 10 * time.Millisecond
(or similar small duration) before constructing command, defer restoring the
original, and shorten the child sleep and final verification sleep accordingly
(e.g., change "sleep 3" to "sleep 30ms" in command and the final time.Sleep(3500
* time.Millisecond) to ~100ms) so the test remains deterministic without long
delays; keep references to TestBashToolTimeoutKillsBackgroundChildren and the
package-level variable bashWaitDelay when making the change.
🪄 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: 391b0400-df03-4455-9e5f-6c5f17eb27d6
📒 Files selected for processing (42)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/cron.gointernal/cli/cron_run.gointernal/cli/cron_test.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/hooks/dispatch.gointernal/hooks/dispatch_test.gointernal/mcp/protocol.gointernal/mcp/protocol_test.gointernal/providerhealth/connectivity_client_test.gointernal/providerhealth/providerhealth.gointernal/providers/anthropic/provider.gointernal/providers/anthropic/provider_test.gointernal/providers/anthropic/types.gointernal/providers/gemini/provider.gointernal/providers/gemini/provider_test.gointernal/providers/gemini/types.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sessions/store.gointernal/sessions/store_test.gointernal/specialist/exec.gointernal/specialist/exec_test.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.gointernal/tools/apply_patch.gointernal/tools/bash.gointernal/tools/bash_proc_unix.gointernal/tools/bash_proc_windows.gointernal/tools/bash_tool_test.gointernal/tools/file_tools_test.gointernal/tools/grep.gointernal/tools/write_tools_test.gointernal/tui/model.gointernal/zeroruntime/helpers.gointernal/zeroruntime/types.go
Fixes from the code review on the critical/high audit branch: - tools/apply_patch: parseHunkCounts now reads only the range section between the opening and closing "@@". A crafted section heading like "@@ -1,1 +1,1 @@ +1,999999" could otherwise overwrite the line count, wedge the parser in hunk mode, and swallow later file headers so they escaped validatePatchPaths / recheckPatchWriteTargets — a workspace-confinement bypass. - providerhealth: the SSRF-hardened dial now tries every validated address in order instead of pinning addrs[0], so a dual-stack/multi-record provider isn't failed by a single dead record. Extracted dialValidatedAddrs for coverage. - hooks/dispatch: a beforeTool hook killed by its deadline now fails CLOSED (vetoes the tool) instead of fail-open. A launch failure (missing binary) still fails open so a misconfigured hook can't wedge every tool call. - cron: Store.Get returns a distinct ErrJobNotFound sentinel; cron_run only treats that as "removed during run" and still records/persists on a transient read error instead of silently dropping the run. - providers/anthropic: closeOpen finalizes thinking buffers still open at stream end (SSE ended after thinking_delta/signature_delta but before content_block_stop), so they survive in the done event's ReasoningBlocks for replay. Finalized in index order. - providers/gemini: corrected the stale thinkingConfig comment (no IncludeThoughts field; thought summaries are suppressed by skipping part.Thought parts). Tests: regression coverage for each fix, plus the requested nitpicks — EOF-terminated MCP line, recipe-prompt propagation assertion, checked file.Close() in session store tests, and a shortened bash process-group timeout test via the bashWaitDelay hook. Note: declined the suggestion to replay a "signature" on redacted_thinking blocks — Anthropic's redacted_thinking carries only an opaque "data" field; the signature is for regular thinking blocks, and the stream never populates one for redacted blocks.
Collaborator
Author
|
Addressed the review findings (head Fixed
Declined (with reasoning)
@coderabbitai review |
|
On the declined item: the reasoning is sound. Anthropic's Triggering a fresh review of the updated head. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/providerhealth/connectivity_client_test.go (1)
37-45: ⚡ Quick winAssert the “last error” contract explicitly.
At Line 42, this test only checks that an error exists, so it would still pass if
dialValidatedAddrsreturned the first failure instead of the last. Use distinct per-attempt errors and asserterrors.Is(err, lastErr)(and optionally assert attempt count) to lock in the behavior this test name promises.Proposed test tightening
func TestDialValidatedAddrsReturnsLastErrorWhenAllFail(t *testing.T) { addrs := []netip.Addr{netip.MustParseAddr("203.0.113.1"), netip.MustParseAddr("203.0.113.2")} + firstErr := errors.New("first unreachable") + lastErr := errors.New("last unreachable") + attempt := 0 dial := func(_ context.Context, _ string) (net.Conn, error) { - return nil, errors.New("unreachable") + attempt++ + if attempt == 1 { + return nil, firstErr + } + return nil, lastErr } - if _, err := dialValidatedAddrs(context.Background(), addrs, "443", dial); err == nil { + _, err := dialValidatedAddrs(context.Background(), addrs, "443", dial) + if err == nil { t.Fatal("expected an error when every address fails to dial") } + if !errors.Is(err, lastErr) { + t.Fatalf("expected last error, got %v", err) + } }🤖 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/providerhealth/connectivity_client_test.go` around lines 37 - 45, TestDialValidatedAddrsReturnsLastErrorWhenAllFail currently only checks for any error; update it to use distinct per-attempt errors and assert that the returned error matches the last attempt's error (use errors.Is(err, lastErr)) to enforce the “last error” contract for dialValidatedAddrs, and optionally verify the dial function was invoked for each address (attempt count) to ensure all addresses were tried.
🤖 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/providerhealth/connectivity_client_test.go`:
- Around line 37-45: TestDialValidatedAddrsReturnsLastErrorWhenAllFail currently
only checks for any error; update it to use distinct per-attempt errors and
assert that the returned error matches the last attempt's error (use
errors.Is(err, lastErr)) to enforce the “last error” contract for
dialValidatedAddrs, and optionally verify the dial function was invoked for each
address (attempt count) to ensure all addresses were tried.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34fbde2e-2646-4614-913d-d195dac25362
📒 Files selected for processing (16)
internal/cli/cron_run.gointernal/cli/cron_test.gointernal/cron/store.gointernal/cron/store_test.gointernal/hooks/dispatch.gointernal/hooks/dispatch_test.gointernal/mcp/protocol_test.gointernal/providerhealth/connectivity_client_test.gointernal/providerhealth/providerhealth.gointernal/providers/anthropic/provider.gointernal/providers/anthropic/provider_test.gointernal/providers/gemini/types.gointernal/sessions/store_test.gointernal/tools/apply_patch.gointernal/tools/bash_tool_test.gointernal/tools/write_tools_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/cli/cron_test.go
- internal/providers/gemini/types.go
- internal/mcp/protocol_test.go
- internal/tools/apply_patch.go
- internal/tools/bash_tool_test.go
- internal/providers/anthropic/provider_test.go
- internal/providerhealth/providerhealth.go
- internal/tools/write_tools_test.go
- internal/sessions/store_test.go
- internal/providers/anthropic/provider.go
- internal/hooks/dispatch.go
- internal/hooks/dispatch_test.go
…test Use distinct per-attempt errors and assert errors.Is(err, lastErr) plus the attempt count, so a regression returning the first failure (or short-circuiting) is caught instead of slipping past a bare non-nil check.
Collaborator
Author
|
Pushed
The other two open inline threads are already resolved:
@coderabbitai review |
|
The two inline threads are confirmed resolved:
Triggering a fresh review of the updated head. ✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List open review comments on PR `#167`
gh pr view 167 --repo Gitlawb/zero --json reviewDecision,reviews,state
echo "---"
# Get review threads
gh api repos/Gitlawb/zero/pulls/167/comments --jq '[.[] | {id: .id, path: .path, line: .line, body: .body[0:120], resolved: false}]' 2>/dev/null | head -100
echo "---"
# Check review thread resolution status via GraphQL
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 167) {
reviewThreads(first: 20) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
path
line
}
}
}
}
}
}
}' 2>/dev/nullLength of output: 58487 🏁 Script executed: #!/bin/bash
# Resolve the cron_run.go outdated thread via GraphQL
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs5Zv7s5"}) {
thread { isResolved }
}
}' 2>/dev/null || true
# List threads to get the node IDs for targeted resolution
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 167) {
reviewThreads(first: 20) {
nodes {
id
isResolved
isOutdated
comments(first: 1) {
nodes {
path
line
body
}
}
}
}
}
}
}' 2>/dev/null | python3 -c "
import json, sys
data = json.load(sys.stdin)
threads = data['data']['repository']['pullRequest']['reviewThreads']['nodes']
for t in threads:
comment = t['comments']['nodes'][0] if t['comments']['nodes'] else {}
print(f\"ID={t['id']} resolved={t['isResolved']} outdated={t['isOutdated']} path={comment.get('path','')} line={comment.get('line','')}\")"Length of output: 2325 🏁 Script executed: #!/bin/bash
# Resolve the cron_run.go outdated thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs6Ivr0W"}) {
thread { id isResolved }
}
}' 2>/dev/null
# Resolve the redacted_thinking thread (withdrawn — reasoning is correct per API contract)
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs6Ivr1C"}) {
thread { id isResolved }
}
}' 2>/dev/nullLength of output: 680
All critical and high findings have been addressed with regression coverage. Approving the PR. [approve] |
Collaborator
Author
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
Vasanthdev2004
approved these changes
Jun 11, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE. I reviewed the critical/high audit fixes around provider reasoning/finish reasons, MCP message bounds/framing, provider health safe dialing, hook dispatch timeout behavior, bash process lifetime hardening, cron/session edge cases, and the tool parsing fixes. I did not find a blocking issue. Validation: go test ./... passed locally in D:\tmp\zero-review-167.
This was referenced Jul 5, 2026
This was referenced Jul 18, 2026
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
2 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.
Summary
Resolves every critical and high finding from the deep audit. Each commit is scoped to a single finding and ships with a regression test; the full suite,
go vet, andgofmtare clean.make([]byte, contentLength)allocation.Setpgid+ groupSIGKILL+WaitDelay) so backgrounded children can't outlive the command or hangRun().CompletionRequest.ReasoningEffortthrough the agent loop and map it per provider: OpenAIreasoning_effort, Anthropic extended thinking (with thinking-block replay), Gemini thinking budget.task-list,bearer of bad news) is no longer over-redacted.finish_reason) to the user.Partial (tracked in
docs/audit/2026-06-10-deep-audit-status.md)Dispatcherthat runs and audits configured hooks is added and tested, but it is not yet wired into thebeforeTool/afterToollifecycle and has no CLIdispatch/editsurface (behavioral change deferred to a design pass).internal/cron/store.go) remains, with single-scheduler as the supported model.Test Plan
go build ./...go test ./...(full suite green)go vet ./...gofmt -lcleanSummary by CodeRabbit
New Features
Bug Fixes
Improvements