Runtime-core: multi-model tool tolerance, failure guard, rich TUI, subagents, plugins/skills#101
Closed
gnanam1990 wants to merge 36 commits into
Closed
Runtime-core: multi-model tool tolerance, failure guard, rich TUI, subagents, plugins/skills#101gnanam1990 wants to merge 36 commits into
gnanam1990 wants to merge 36 commits into
Conversation
Local development snapshot of one session's intertwined workstreams. Committed together because changes span shared files (provider.go, model.go, loop.go, types.go, streamjson) and can't be cleanly split without interactive hunk staging. Build + vet + full -race suite green. Zenline TUI skin: Zen home / Statusline chat / permission-modal surfaces, 5 themes (Phosphor/Cyan/Sage/Violet/Mono), boot splash, mouse support, and a headless --snapshot mode (internal/zenline, cli/zenline.go, tui/zenline_view.go). OpenAI provider: streaming tool-call accumulation hardening plus an idle-timeout watchdog (reader goroutine + context cancel) so a stalled upstream surfaces an error instead of hanging the agent forever. Agent reliability: update_plan schema makes id optional/auto-numbered with an advertised item schema; malformed (nameless) tool calls feed a retry to the model instead of silently ending the turn; secret scrubbing at the registry boundary (covers agent loop AND MCP); structured ToolResult (changedFiles/display/redacted) surfaced via stream-json. Session file checkpoints + safe rewind: before-mutation content snapshots (content-addressed, deduped, size-capped, 0600) captured on write/edit/apply_patch in TUI and headless; RestoreToSequence + ApplyRewind (path-traversal-guarded, session-locked); /rewind in TUI and 'zero sessions rewind' headless; stream-json checkpoint/restore events. Design specs under docs/superpowers/specs/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reasoning defaults Resolve()/ResolveWithFallback()/FallbackFor() + DefaultReasoningEffort/EffectiveReasoningEffort wired into CLI --model and TUI /model (deprecated models auto-redirect with a notice; unknown ids like minimax-m3 pass through unchanged). zero models renders an aligned table; catalog entries decorated with match patterns/default efforts/deprecation rules. Provider reasoning-effort propagation deferred (needs request-schema change). Build/vet/test green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…firmation policy Pre-exec interactive-command detection (editors/pagers/REPLs/ssh/git-rebase-i) hardened against absolute-path, quoted, and $()/backtick-substitution bypasses; destructive-pattern hardening (fork bombs, block-device writes, rm -rf root variants); structured violation annotation on blocked bash; CONFIRMATION_POLICY embedded + appended to the system prompt; 'zero sandbox policy --effective'. Full 4-action permission-engine / 5x3 autonomy matrix deferred. Build/vet/-race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stops a run after 3 consecutive empty turns (no text/tool calls) instead of burning to maxTurns; one-shot not-called reminder (by turn 3 if a multi-step task skipped update_plan) and stale reminder (after 10 tool calls since last update_plan). Dropped-call turns excluded from the empty counter. Build/vet/-race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New read-only ask_user tool (questions[] with options/multiSelect); the loop intercepts it like permissions and routes to Options.OnAskUser. TUI shows an async questionnaire (buffered channel + ctx.Done select, mirroring the permission flow — no deadlock); headless leaves OnAskUser nil so it degrades gracefully to 'proceed with assumptions'. Build/vet/-race green; 13 new tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Data-driven presets (smart/deep/fast/large/precise) bundling model + reasoning effort + max turns (+ optional tool filter), resolved through the registry. --mode seeds only unset exec options so explicit --model/-r/--max-turns still win; /mode lists/switches in the TUI. Build/vet/test green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g sessions Proactive compaction at 80% of the model context window + reactive recovery on context-limit errors: summarizes the oldest middle messages into one user-role summary, preserves system + recent suffix. Pure Compact() (injectable Summarizer), low-water-mark guard against churn, suffix walked back to an assistant boundary so no consecutive-user/dangling-tool-result sequences, recover() gated on enabled. Disabled (no-op) when ContextWindow==0 — wired from the resolved model's context window in CLI+TUI. Build/vet/-race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ask_user: coerce options from objects ({label|value|text|...})/scalars/newline-strings instead of erroring 'options must be an array of strings'; accept bare-string question items and prompt/text/q/title aliases for the question. write_file: read content from contents/text/body/data/file_content aliases (minimax mis-keys 'content'); present-but-non-string still rejected. Same weak-model tolerance already applied to update_plan. Tests added; build/vet/-race green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ively Removed tea.WithMouseCellMotion() from the zenline program: mouse cell-motion reporting routed clicks to the app and broke the terminal's native text selection + copy (and made paste flaky). The permission modal is keyboard-driven (a/y/d/Esc), so capturing the mouse cost core UX for little gain. Click-to-select, Cmd/Ctrl+C copy, and Cmd/Ctrl+V paste now behave like a normal terminal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…models internal/tools/argtolerance.go adds aliasedStringArg (alias-aware, type-strict, primary-key errors) and coerceStringSlice (best-effort list coercion). Every tool now accepts the key variants models emit: read_file/write_file/edit_file path->file/file_path/filename; content->contents/text/body/data; old_string->old/search/find; new_string->new/replace/replacement; apply_patch patch->diff; grep pattern->query/regex/search + path->dir/directory; glob pattern->glob/match/query + cwd->dir/path; list_directory path->directory/dir; bash command->cmd/script/shell. ask_user options/question coercion unified onto the shared helper. Type-strictness preserved (content:42 still errors). Serves the diverse multi-model userbase; build/vet/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
guardState tracks consecutive same-error failures per tool: after 2 identical failures it injects a one-shot corrective hint (the tool's JSON arg schema + the exact error) so the model self-corrects; after 4 it halts the run with a clear message instead of burning turns to maxTurns. A successful call resets that tool's streak; the hint takes priority over plan reminders. Complements the arg-tolerance layer: tolerance prevents most mis-shapes, the guard backstops any that remain (any model, any tool). Build/vet/-race/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bg card) Home/boot text used foreground-only styles, so content cells showed the TERMINAL's own background while lipgloss filled the surrounding whitespace with the theme bg — a visible two-tone 'card'. Added newCanvasStyles (foreground + theme background) for the full-bleed surfaces (home + boot), and set the theme bg on the input box and the centered-content wrappers, so the whole screen is one uniform themed background filling the terminal. Chat status bars keep the bg-free styles (they compose inside Panel-colored segments) so they're unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nce) boolArg accepts true/false/yes/no/on/off/1/0 (string or number); intArg accepts whole-number strings. Fixes minimax-m3's 'overwrite must be a boolean' and string-number args. Genuinely uncoercible values still error. Completes the arg-tolerance layer alongside string aliases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…diffs (PRD F4) Assistant messages now render through glamour (CommonMark + chroma-highlighted fenced code), themed to the active palette and sitting on the full-bleed bg. edit_file/apply_patch results render as colored, guttered unified diffs (green add / red del / dim context, 40-line cap). Rendered markdown is cached per (text,theme,width) with a 512-entry bound so RenderChat (called every spinner tick/keystroke) doesn't re-run glamour each frame. Streaming text stays plain (partial markdown renders badly). Adds glamour v1.0.0 + chroma v2 (offline from cache). Build/vet/-race/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Typing '/' surfaces a filtered, navigable command list (Tab/↑↓ cycle, Enter/Tab complete, Esc dismiss) in both default and zenline skins — purely additive UI state that never disturbs the permission/ask_user/zenline/submit key precedence. Bare /model, /effort, /mode (and /theme in zenline) open an interactive selector overlay that routes the choice through the existing handlers. Build/vet/-race/full-suite green; existing key tests unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
agent.Options gains Provider (auto-injected in Run) + Depth. A 'task' tool is intercepted in executeToolCall (like ask_user): it spawns agent.Run as a child with Depth+1, a registry filtered via new Registry.Without("task","ask_user") (no infinite nesting, no interactive prompts), inherited Sandbox/PermissionMode/Autonomy but all interactive callbacks nil, sharing ctx for cancellation; the child's final answer becomes the tool result. Depth guard (maxTaskDepth=2) refuses deeper nesting with a clear message. Synchronous; async task output/stop deferred. Build/vet/-race/full-suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Plugins: LoadedPlugin gains optional Author/License/Keywords/Homepage/Interface metadata (omitempty, non-breaking, best-effort parse) surfaced in 'zero plugins'. Skills: new internal/skills package loads */SKILL.md (hand-parsed frontmatter, no YAML dep) from ZERO_SKILLS_DIR or the XDG data dir; a read-only 'skill' tool lets the agent load a named skill's content on demand (registered in CoreReadOnlyTools); 'zero skills' lists them (+--json). Build/vet/-race/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Zero automated PR reviewVerdict: Changes requested Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughThe PR adds agent runtime features for compaction, guardrails, ask-user/task flows, session checkpointing/rewind, provider idle handling, and broad tool/CLI/TUI/Zenline updates. It also expands tests, dependency metadata, and ignore rules to support the new behaviors. ChangesRuntime, tools, sessions, CLI, and UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related issues
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/sessions.go (1)
197-204:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Add "rewind" to the command list.
The
isSessionsCommandfunction is missing "rewind" in the case list (line 199), but the command handler exists at lines 64-68 and help text documents it at line 468. Without this entry, the parser at line 175-181 will rejectzero sessions rewind <id>with "unknown sessions command" before the switch statement can route it.🐛 Proposed fix
func isSessionsCommand(command string) bool { switch command { - case "list", "children", "lineage", "tree", "rewind-plan", "compact-plan": + case "list", "children", "lineage", "tree", "rewind-plan", "rewind", "compact-plan": return true default: return false } }🤖 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/sessions.go` around lines 197 - 204, The isSessionsCommand function's switch is missing the "rewind" case so the CLI parser rejects "zero sessions rewind <id>"; update isSessionsCommand to include "rewind" alongside "rewind-plan", "list", "children", "lineage", and "tree" so it returns true and allows the existing sessions rewind handler to be reached.
🧹 Nitpick comments (16)
internal/tui/ask_user_test.go (1)
124-127: 💤 Low valueRemove the empty if block or add a linter suppression comment.
Static analysis flagged this empty
ifas SA9003. The comment documents the intended behavior, but the empty block triggers a lint violation. Either remove the if statement and keep the comment standalone, or add a//nolint:staticcheckdirective if you want to preserve the assertion structure.♻️ Option 1: Standalone comment
if len(answers) != 1 { t.Fatalf("expected the answer callback to fire on Esc, got %#v", answers) } - if next.pending { - // cancelRun is the normal Esc path; here we only cancel the prompt, the - // run continues with the degraded answers. - } + // cancelRun is the normal Esc path; here we only cancel the prompt, the + // run continues with the degraded answers (so next.pending should still be true).♻️ Option 2: Suppress lint
+ //nolint:staticcheck // SA9003: intentionally checking pending stays true if next.pending { // cancelRun is the normal Esc path; here we only cancel the prompt, the // run continues with the degraded answers. }🤖 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/tui/ask_user_test.go` around lines 124 - 127, The empty if block checking next.pending should be removed or suppressed to satisfy SA9003: either delete the "if next.pending { ... }" and leave the explanatory comment as a standalone line describing that cancelRun is the normal Esc path and here we only cancel the prompt, or keep the if for clarity but add a linter suppression (e.g., a "//nolint:staticcheck" comment immediately preceding the if) so the static checker ignores the empty-body intentional assertion; reference the conditional expression "next.pending" and the behavior note about "cancelRun" to locate the code to change.internal/tools/file_tools_test.go (1)
186-214: Add excluded-dir coverage for NewGlobTool tests
NewGlobToolalready honorsshouldSkipDirectory(it skips excluded dirs duringWalkDir), but current tests only cover grep’s excluded-dir behavior. Add a parallelTestGlobToolSkipsAlwaysExcludedDirectoriesthat writes files under.git/andnode_modules/plus akeep.txt, runsNewGlobTool(root)with a pattern that would match those paths (e.g.,"**/*"), and asserts output includeskeep.txtand does not contain.gitornode_modules.🤖 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/file_tools_test.go` around lines 186 - 214, Add a new test named TestGlobToolSkipsAlwaysExcludedDirectories that mirrors TestGrepSkipsAlwaysExcludedDirectories: use the same mustWrite helper to create keep.txt, .git/config, and node_modules/pkg/index.js under a temp root, call NewGlobTool(root).Run(context.Background(), map[string]any{"pattern":"**/*","output_mode":"paths"}), assert res.Status==StatusOK, assert res.Output contains "keep.txt" and does NOT contain ".git" or "node_modules". Ensure the test uses the same exclusion expectations (shouldSkipDirectory) so NewGlobTool’s WalkDir behavior is covered.internal/tools/glob.go (1)
45-48: Reconsider thepathalias forglob.cwkUX concern
glob’scwdparameter aliases includepath, and it is semantically consistent with the rest of the tools wherepathis the target directory/file (e.g.,read_file,edit_file,write_file,list_directory,grep). Also,pathis not used as an alias forpattern, so a model passingpathinstead of the glob pattern will still fail with “pattern is required.” The only real UX mismatch is naming consistency:globuses canonicalcwdwhile other tools use canonicalpath; consider aligning the canonical key if that matters for consumers.🤖 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/glob.go` around lines 45 - 48, The current aliasedStringArg call for cwd includes "path" as an alias which conflicts with other tools' semantics; remove "path" from the alias list in the aliasedStringArg invocation that sets cwd to avoid confusing callers (leave "cwd","dir","directory" etc.), and if you prefer cross-tool consistency instead rename the canonical parameter from cwd to path in the glob tool (update the aliasedStringArg call and the public API/usage for glob.cwk and any references to cwd) so that pattern remains required and callers aren't misled.internal/zenline/markdown.go (1)
70-70: 💤 Low valueTrim vs TrimRight: Comment mentions trailing newlines only.
Line 70 uses
strings.Trim(out, "\n")which removes newlines from both ends, but the comment on lines 45-46 specifically mentions "Trailing blank lines glamour appends are trimmed." If the intent is to preserve leading whitespace/newlines but remove trailing ones, usestrings.TrimRight(out, "\n")instead.📝 Clarification
- out = strings.Trim(out, "\n") + out = strings.TrimRight(out, "\n")If trimming both ends is intentional, update the comment to reflect that.
🤖 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/zenline/markdown.go` at line 70, The code currently calls strings.Trim(out, "\n") which removes newlines from both ends but the surrounding comment says only trailing blank lines are trimmed; update the implementation to use strings.TrimRight(out, "\n") to remove only trailing newlines (locate the call that assigns to variable out in internal/zenline/markdown.go) or, if bidirectional trimming was intentional, update the comment to state that both leading and trailing newlines are removed.internal/cli/command_center.go (1)
287-299: 💤 Low valueUnchecked error returns from tabwriter operations.
While
strings.Buildernever returns write errors (making these safe in practice), Go convention is to either check the error or explicitly ignore it with_ =. Consider adding explicit ignores to satisfy static analysis:- fmt.Fprintln(writer, "ID\tPROVIDER\tSTATUS\tCONTEXT\tREASONING\t$IN/1M\t$OUT/1M") + _, _ = fmt.Fprintln(writer, "ID\tPROVIDER\tSTATUS\tCONTEXT\tREASONING\t$IN/1M\t$OUT/1M")- fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + _, _ = fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n",- writer.Flush() + _ = writer.Flush()🤖 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/command_center.go` around lines 287 - 299, The tabwriter write calls in the loop and header (fmt.Fprintln, fmt.Fprintf) and the final writer.Flush currently ignore returned errors; update the code around the writer usage (the fmt.Fprintln call that writes the header, the fmt.Fprintf inside the for _, model range, and writer.Flush()) to explicitly handle or ignore the errors per Go convention—for example assign the returns to the blank identifier ( _ = fmt.Fprintln(...), _ = fmt.Fprintf(...), _ = writer.Flush() ) or check and handle the error; locate these calls by the symbols fmt.Fprintln, fmt.Fprintf and writer.Flush to make the change.docs/superpowers/specs/2026-06-06-session-file-checkpoints-design.md (1)
40-43: 💤 Low valueAdd language identifier to fenced code block.
The JSON payload example at line 41 should specify
jsonas the language for syntax highlighting and to satisfy markdown linters:📝 Proposed fix
- ``` + ```json { sequence, tool, files: [ { path, blob: "<sha256>"|"", absent: bool, skipped: bool, bytes: int } ] } - ``` + ```🤖 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/superpowers/specs/2026-06-06-session-file-checkpoints-design.md` around lines 40 - 43, Update the fenced code block showing the Store.AppendEvent payload to declare the language as json for proper syntax highlighting and linter compliance; locate the block containing the example payload for Store.AppendEvent (the line with "{ sequence, tool, files: [ { path, blob: "<sha256>"|"", absent: bool, skipped: bool, bytes: int } ] }") and change the opening triple-backticks to "```json" while leaving the closing triple-backticks unchanged.internal/zenline/render_test.go (1)
141-156: 💤 Low valuestripANSI only handles SGR sequences.
The current implementation only strips Select Graphic Rendition (SGR) escape sequences ending with
m. If the rendering layers emit cursor-positioning CSI codes (e.g.,\x1b[H,\x1b[2J) or other control sequences, they will leak into the test assertions. For the current tests this appears sufficient, but if test flakiness appears due to unexpected ANSI codes, consider using a more comprehensive stripper or a library likegithub.com/acarl005/stripansi.🤖 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/zenline/render_test.go` around lines 141 - 156, stripANSI currently only removes SGR sequences ending with 'm' and will miss other CSI/control sequences; update the stripANSI function to either (a) fully parse CSI sequences by, when seeing 0x1b followed by '[' (or other intermediates), consume bytes until a final byte in the CSI final-byte range (0x40–0x7E) before resuming writing runes, or (b) replace stripANSI with a call to a well‑tested library such as github.com/acarl005/stripansi; ensure you change references to stripANSI accordingly and add a unit test that includes a CSI sequence (e.g., "\x1b[H" or "\x1b[2J") to verify the new behavior.internal/providers/openai/provider.go (1)
132-144: ⚡ Quick winRetry loop returns stale errors when context is cancelled.
If
ctxis cancelled during the backoff wait (line 133 or 139), the retry loop exits and returns the network or 5xx error that triggered the retry, rather than a "context cancelled" error. This can produce misleading error messages in logs when users abort a run mid-retry.Consider checking
ctx.Err()after the retry loop and prioritizing context errors:🔍 Proposed fix
response = resp break } + if ctx.Err() != nil { + sendEvent(ctx, events, zeroruntime.StreamEvent{Type: zeroruntime.StreamEventError, Error: provider.redact("provider stream error: " + ctx.Err().Error())}) + return + } defer func() {🤖 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 132 - 144, The retry loop can return a stale network or 5xx error when ctx is cancelled during backoff; update the logic around the loop (the block that uses attempt, maxAttempts, backoff, resp and response) to check ctx.Err() after the loop completes and, if non-nil, send/return the context error (e.g., prefer ctx.Err() over the prior err/resp error) — ensure you still close resp.Body when required and use provider.redact/sendEvent with the context cancellation message instead of the original network/5xx error.internal/tools/args.go (1)
89-98: 💤 Low valueConsider potential precision loss for very large integers.
Line 94 parses strings as float64 before checking integrality. For integers larger than 2^53, this can lose precision. In practice, tool arguments won't reach this range, but for completeness you could parse large integers directly via
strconv.ParseIntbefore falling back to float parsing.This is a low-priority edge case and the current implementation handles realistic inputs correctly.
🤖 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/args.go` around lines 89 - 98, The string branch (case string) currently converts numeric strings by trying strconv.Atoi then strconv.ParseFloat which can lose precision for integers > 2^53; change the parsing order to attempt strconv.ParseInt(trimmed, 10, 64) (or ParseUint when appropriate) after Atoi fails, then fall back to ParseFloat only if ParseInt/ParseUint fail and you still need to accept integral floats; update the assignment to set number from the parsed int and preserve the existing error path (return fmt.Errorf("%s must be an integer", key)) if all parses fail so callers of this switch get the same behavior.internal/tools/ask_user.go (1)
184-202: ⚡ Quick winRemove unused dead code.
The
stringSliceArgfunction is unused and should be removed.🧹 Proposed fix to remove dead code
-func stringSliceArg(args map[string]any, key string) ([]string, error) { - value, ok := args[key] - if !ok || value == nil { - return nil, nil - } - items, ok := value.([]any) - if !ok { - return nil, fmt.Errorf("%s must be an array of strings", key) - } - result := make([]string, 0, len(items)) - for _, item := range items { - text, ok := item.(string) - if !ok { - return nil, fmt.Errorf("%s must be an array of strings", key) - } - result = append(result, text) - } - return result, 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/tools/ask_user.go` around lines 184 - 202, Remove the dead/unused helper function stringSliceArg from internal/tools/ask_user.go: locate the function declaration for stringSliceArg(...) and delete it entirely (including its signature and body) so there is no unused code left; run go vet/go build to ensure there are no remaining references and remove any imports that become unused as a result.internal/tools/task.go (1)
114-118: 💤 Low valueInconsistent trimming:
Promptis assigned untrimmed whileDescriptionandSubagentTypeare trimmed.Line 100 trims
promptfor the blank check, but line 116 assigns the raw value. Consider trimming consistently:♻️ Suggested fix
return TaskRequest{ Description: strings.TrimSpace(description), - Prompt: prompt, + Prompt: strings.TrimSpace(prompt), SubagentType: strings.TrimSpace(subagentType), }, 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/tools/task.go` around lines 114 - 118, The returned TaskRequest sets Prompt to the untrimmed variable while Description and SubagentType use strings.TrimSpace; change the assignment in the TaskRequest construction (Prompt field) to use a trimmed value (e.g., strings.TrimSpace(prompt) or reuse the trimmedPrompt used in the earlier blank check) so Prompt is consistently trimmed like Description and SubagentType.internal/zenline/render.go (1)
934-945: ⚡ Quick winRemove unused function
detailLines.Static analysis reports this function is never called. If it's not needed, remove it to reduce maintenance burden.
🤖 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/zenline/render.go` around lines 934 - 945, Remove the unused helper function detailLines (and its internal calls like strings.TrimRight, strings.Split usage) from internal/zenline/render.go; locate the function named detailLines and delete its entire definition to eliminate dead code and update any imports if they become unused after removal.internal/tui/zenline_view.go (1)
174-174: 💤 Low valueConsider applying De Morgan's law for clarity.
Static analysis suggests simplifying the boolean expression. Current:
Running: !(r.id != "" && resultIDs[r.id])De Morgan's law equivalent:
Running: r.id == "" || !resultIDs[r.id]Both are correct, but the second form is more direct.
🤖 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/tui/zenline_view.go` at line 174, The boolean expression for the Running field is negating a conjunction and should be simplified using De Morgan's law: replace the current expression that uses !(r.id != "" && resultIDs[r.id]) with the clearer equivalent r.id == "" || !resultIDs[r.id]; update the struct literal where Running is set (the place referencing r.id and resultIDs) to use this simplified expression for readability and maintainability.internal/zenline/theme.go (2)
60-76: 💤 Low valueExtract clamping logic to reduce duplication.
Both
ResolveandThemeNameduplicate the variant clamping logic (lines 61-63 and 72-74). Extract a helper to avoid drift:func clampVariant(variant int) int { if variant < 0 || variant >= len(Themes) { return 0 } return variant }Then use
variant = clampVariant(variant)in both functions.🤖 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/zenline/theme.go` around lines 60 - 76, Extract the repeated bounds-check into a helper named clampVariant that returns 0 for out-of-range variants and the original variant otherwise, and call it from both Resolve and ThemeName (e.g., variant = clampVariant(variant)); ensure clampVariant references the Themes slice length for its bounds and keep Resolve’s dark switch and ThemeName’s return unchanged aside from using the clamped variant.
31-57: 💤 Low valueConsider making
Themesimmutable or providing a getter.The exported
Themesvariable is a mutable slice. External callers could accidentally or maliciously modify it (e.g.,Themes[0].Name = "Hacked"orThemes = nil), breaking UI rendering for all users of the package.If the slice is meant to be read-only, either:
- Export a function
func GetThemes() []Themethat returns a copy, or- Document that Themes must not be modified.
🤖 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/zenline/theme.go` around lines 31 - 57, The exported Themes slice is mutable; make it read-only by replacing direct export with an accessor that returns a copy: add a new function GetThemes() []Theme that returns a copied slice of Themes (or an unexported internal variable like themes) and stop exporting the mutable Themes variable, ensuring references use GetThemes() instead; update any callers to call GetThemes() and keep the original Theme struct and mkPal usage unchanged.internal/tui/autocomplete.go (1)
60-62: ⚡ Quick winRedundant negative check.
Lines 57-59 already reset
suggestionIdxto 0 if it's out of range. The check on lines 60-62 for< 0is unreachable becausesuggestionIdxcannot become negative after being reset to 0 at line 58.♻️ Proposed cleanup
matches := matchCommandSuggestions(token) m.suggestions = matches if m.suggestionIdx >= len(matches) { m.suggestionIdx = 0 } - if m.suggestionIdx < 0 { - m.suggestionIdx = 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/tui/autocomplete.go` around lines 60 - 62, The check for m.suggestionIdx < 0 is redundant because earlier code already clamps suggestionIdx to 0 when out of range; remove the unreachable conditional block referencing m.suggestionIdx < 0 (in internal/tui/autocomplete.go) to simplify the logic and avoid dead code, ensuring the existing range-reset logic (the earlier check that sets m.suggestionIdx = 0) remains intact.
🤖 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/modelregistry/models.go`:
- Around line 155-157: The existing validation in models.go checks that
model.DefaultReasoningEffort is a valid constant via ValidReasoningEffort but
doesn't ensure the default is actually present in the model's ReasoningEfforts
slice; update the validation in the same block (around the
DefaultReasoningEffort check) to also verify membership in
model.ReasoningEfforts (e.g., iterate the slice or use an existing helper) and
return an error like "default reasoning effort %q not in ReasoningEfforts" if
it's missing, while keeping the current ValidReasoningEffort check.
In `@internal/modelregistry/resolve.go`:
- Around line 38-56: Add a validation pass in NewRegistry after all entries are
registered to ensure any non-empty Deprecation.FallbackID actually resolves:
iterate the registered entries (from models.go), and for each entry with
entry.Deprecation != nil and strings.TrimSpace(entry.Deprecation.FallbackID) !=
"" call registry.Get(entry.Deprecation.FallbackID) and return an error if Get
returns not ok; this prevents ResolveWithFallback from silently ignoring
misconfigured fallbacks and surfaces the problem at startup.
In `@internal/sessions/checkpoint_test.go`:
- Line 147: The test calls CaptureToolCheckpoint("s", ws, "write_file",
[]string{"new.txt"}) without checking its error return; update the test setup to
capture the returned error from captureToolCheckpoint (CaptureToolCheckpoint)
and assert/fail on it (e.g., use t.Fatal/t.Fatalf or your test helper like
require.NoError) so test failures show the underlying error; ensure the same
pattern used elsewhere (lines 125-127) is applied here to make diagnostics
clear.
- Line 213: The test calls store.AppendEvent("s", AppendEventInput{Type:
EventMessage, Payload: map[string]any{}}) without checking the returned error;
update the test setup to capture the returned error from store.AppendEvent and
assert/fail on it (e.g., using t.Fatalf, t.Fatal, or your test helper like
require.NoError) so failures in AppendEvent surface clearly; search for the
AppendEvent call in checkpoint_test.go and replace the unchecked invocation with
an error-checking assertion referencing the same function call.
- Line 195: Tests currently ignore the error returned by store.AppendEvent when
creating the malicious checkpoint event; capture the returned error from the
AppendEvent call (e.g. err := store.AppendEvent("s", AppendEventInput{...})) and
explicitly fail the test on error using the test harness (e.g.
require.NoError(t, err) or t.Fatalf("AppendEvent failed: %v", err)). Ensure you
reference the same AppendEvent call and AppendEventInput/CheckpointPayload used
in the setup so any unexpected failures are reported clearly.
- Around line 125-127: The test setup ignores errors returned by
CaptureToolCheckpoint which can mask setup failures; modify the calls to
CaptureToolCheckpoint in checkpoint_test.go to capture their returned error and
immediately fail the test on error (e.g., use t.Fatalf/t.Helper or a test helper
like require.NoError) so failures surface at the point of checkpoint capture
rather than later (update both calls to CaptureToolCheckpoint shown in the
diff).
In `@internal/sessions/rewind.go`:
- Around line 29-85: RestoreToSequence currently increments
FilesRestored/FilesDeleted every time a path is processed across checkpoints,
causing double-counting; replace the per-iteration counting with a single
final-op map: create a map (e.g., finalOps keyed by f.Path with values like
state ("restored"/"deleted"/"skipped") and a skipped flag), update finalOps
inside the existing loop (use resolveWithinWorkspace, store.readBlob and
store.writeFileAtomic as now), and after processing all checkpoints iterate
finalOps once to populate report.FilesRestored, report.FilesDeleted and
report.Skipped so each path is counted exactly once.
---
Outside diff comments:
In `@internal/cli/sessions.go`:
- Around line 197-204: The isSessionsCommand function's switch is missing the
"rewind" case so the CLI parser rejects "zero sessions rewind <id>"; update
isSessionsCommand to include "rewind" alongside "rewind-plan", "list",
"children", "lineage", and "tree" so it returns true and allows the existing
sessions rewind handler to be reached.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-06-06-session-file-checkpoints-design.md`:
- Around line 40-43: Update the fenced code block showing the Store.AppendEvent
payload to declare the language as json for proper syntax highlighting and
linter compliance; locate the block containing the example payload for
Store.AppendEvent (the line with "{ sequence, tool, files: [ { path, blob:
"<sha256>"|"", absent: bool, skipped: bool, bytes: int } ] }") and change the
opening triple-backticks to "```json" while leaving the closing triple-backticks
unchanged.
In `@internal/cli/command_center.go`:
- Around line 287-299: The tabwriter write calls in the loop and header
(fmt.Fprintln, fmt.Fprintf) and the final writer.Flush currently ignore returned
errors; update the code around the writer usage (the fmt.Fprintln call that
writes the header, the fmt.Fprintf inside the for _, model range, and
writer.Flush()) to explicitly handle or ignore the errors per Go convention—for
example assign the returns to the blank identifier ( _ = fmt.Fprintln(...), _ =
fmt.Fprintf(...), _ = writer.Flush() ) or check and handle the error; locate
these calls by the symbols fmt.Fprintln, fmt.Fprintf and writer.Flush to make
the change.
In `@internal/providers/openai/provider.go`:
- Around line 132-144: The retry loop can return a stale network or 5xx error
when ctx is cancelled during backoff; update the logic around the loop (the
block that uses attempt, maxAttempts, backoff, resp and response) to check
ctx.Err() after the loop completes and, if non-nil, send/return the context
error (e.g., prefer ctx.Err() over the prior err/resp error) — ensure you still
close resp.Body when required and use provider.redact/sendEvent with the context
cancellation message instead of the original network/5xx error.
In `@internal/tools/args.go`:
- Around line 89-98: The string branch (case string) currently converts numeric
strings by trying strconv.Atoi then strconv.ParseFloat which can lose precision
for integers > 2^53; change the parsing order to attempt
strconv.ParseInt(trimmed, 10, 64) (or ParseUint when appropriate) after Atoi
fails, then fall back to ParseFloat only if ParseInt/ParseUint fail and you
still need to accept integral floats; update the assignment to set number from
the parsed int and preserve the existing error path (return fmt.Errorf("%s must
be an integer", key)) if all parses fail so callers of this switch get the same
behavior.
In `@internal/tools/ask_user.go`:
- Around line 184-202: Remove the dead/unused helper function stringSliceArg
from internal/tools/ask_user.go: locate the function declaration for
stringSliceArg(...) and delete it entirely (including its signature and body) so
there is no unused code left; run go vet/go build to ensure there are no
remaining references and remove any imports that become unused as a result.
In `@internal/tools/file_tools_test.go`:
- Around line 186-214: Add a new test named
TestGlobToolSkipsAlwaysExcludedDirectories that mirrors
TestGrepSkipsAlwaysExcludedDirectories: use the same mustWrite helper to create
keep.txt, .git/config, and node_modules/pkg/index.js under a temp root, call
NewGlobTool(root).Run(context.Background(),
map[string]any{"pattern":"**/*","output_mode":"paths"}), assert
res.Status==StatusOK, assert res.Output contains "keep.txt" and does NOT contain
".git" or "node_modules". Ensure the test uses the same exclusion expectations
(shouldSkipDirectory) so NewGlobTool’s WalkDir behavior is covered.
In `@internal/tools/glob.go`:
- Around line 45-48: The current aliasedStringArg call for cwd includes "path"
as an alias which conflicts with other tools' semantics; remove "path" from the
alias list in the aliasedStringArg invocation that sets cwd to avoid confusing
callers (leave "cwd","dir","directory" etc.), and if you prefer cross-tool
consistency instead rename the canonical parameter from cwd to path in the glob
tool (update the aliasedStringArg call and the public API/usage for glob.cwk and
any references to cwd) so that pattern remains required and callers aren't
misled.
In `@internal/tools/task.go`:
- Around line 114-118: The returned TaskRequest sets Prompt to the untrimmed
variable while Description and SubagentType use strings.TrimSpace; change the
assignment in the TaskRequest construction (Prompt field) to use a trimmed value
(e.g., strings.TrimSpace(prompt) or reuse the trimmedPrompt used in the earlier
blank check) so Prompt is consistently trimmed like Description and
SubagentType.
In `@internal/tui/ask_user_test.go`:
- Around line 124-127: The empty if block checking next.pending should be
removed or suppressed to satisfy SA9003: either delete the "if next.pending {
... }" and leave the explanatory comment as a standalone line describing that
cancelRun is the normal Esc path and here we only cancel the prompt, or keep the
if for clarity but add a linter suppression (e.g., a "//nolint:staticcheck"
comment immediately preceding the if) so the static checker ignores the
empty-body intentional assertion; reference the conditional expression
"next.pending" and the behavior note about "cancelRun" to locate the code to
change.
In `@internal/tui/autocomplete.go`:
- Around line 60-62: The check for m.suggestionIdx < 0 is redundant because
earlier code already clamps suggestionIdx to 0 when out of range; remove the
unreachable conditional block referencing m.suggestionIdx < 0 (in
internal/tui/autocomplete.go) to simplify the logic and avoid dead code,
ensuring the existing range-reset logic (the earlier check that sets
m.suggestionIdx = 0) remains intact.
In `@internal/tui/zenline_view.go`:
- Line 174: The boolean expression for the Running field is negating a
conjunction and should be simplified using De Morgan's law: replace the current
expression that uses !(r.id != "" && resultIDs[r.id]) with the clearer
equivalent r.id == "" || !resultIDs[r.id]; update the struct literal where
Running is set (the place referencing r.id and resultIDs) to use this simplified
expression for readability and maintainability.
In `@internal/zenline/markdown.go`:
- Line 70: The code currently calls strings.Trim(out, "\n") which removes
newlines from both ends but the surrounding comment says only trailing blank
lines are trimmed; update the implementation to use strings.TrimRight(out, "\n")
to remove only trailing newlines (locate the call that assigns to variable out
in internal/zenline/markdown.go) or, if bidirectional trimming was intentional,
update the comment to state that both leading and trailing newlines are removed.
In `@internal/zenline/render_test.go`:
- Around line 141-156: stripANSI currently only removes SGR sequences ending
with 'm' and will miss other CSI/control sequences; update the stripANSI
function to either (a) fully parse CSI sequences by, when seeing 0x1b followed
by '[' (or other intermediates), consume bytes until a final byte in the CSI
final-byte range (0x40–0x7E) before resuming writing runes, or (b) replace
stripANSI with a call to a well‑tested library such as
github.com/acarl005/stripansi; ensure you change references to stripANSI
accordingly and add a unit test that includes a CSI sequence (e.g., "\x1b[H" or
"\x1b[2J") to verify the new behavior.
In `@internal/zenline/render.go`:
- Around line 934-945: Remove the unused helper function detailLines (and its
internal calls like strings.TrimRight, strings.Split usage) from
internal/zenline/render.go; locate the function named detailLines and delete its
entire definition to eliminate dead code and update any imports if they become
unused after removal.
In `@internal/zenline/theme.go`:
- Around line 60-76: Extract the repeated bounds-check into a helper named
clampVariant that returns 0 for out-of-range variants and the original variant
otherwise, and call it from both Resolve and ThemeName (e.g., variant =
clampVariant(variant)); ensure clampVariant references the Themes slice length
for its bounds and keep Resolve’s dark switch and ThemeName’s return unchanged
aside from using the clamped variant.
- Around line 31-57: The exported Themes slice is mutable; make it read-only by
replacing direct export with an accessor that returns a copy: add a new function
GetThemes() []Theme that returns a copied slice of Themes (or an unexported
internal variable like themes) and stop exporting the mutable Themes variable,
ensuring references use GetThemes() instead; update any callers to call
GetThemes() and keep the original Theme struct and mkPal usage unchanged.
🪄 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: ef10bdf6-9ae8-4760-8f43-984e9fa3a696
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (108)
.gitignoredocs/superpowers/specs/2026-06-06-reliability-batch-design.mddocs/superpowers/specs/2026-06-06-session-file-checkpoints-design.mdgo.modinternal/agent/ask_user_test.gointernal/agent/compaction.gointernal/agent/compaction_test.gointernal/agent/confirmation_policy.mdinternal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/task_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/app_test.gointernal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_sessions.gointernal/cli/exec_test.gointernal/cli/exec_writer.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/cli/sessions.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/cli/zenline.gointernal/modelregistry/catalog.gointernal/modelregistry/catalog_test.gointernal/modelregistry/models.gointernal/modelregistry/modes.gointernal/modelregistry/modes_test.gointernal/modelregistry/resolve.gointernal/modelregistry/resolve_test.gointernal/plugins/plugins.gointernal/plugins/plugins_test.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sessions/checkpoint.gointernal/sessions/checkpoint_test.gointernal/sessions/rewind.gointernal/sessions/store.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.gointernal/tools/apply_patch.gointernal/tools/args.gointernal/tools/argtolerance.gointernal/tools/argtolerance_test.gointernal/tools/ask_user.gointernal/tools/ask_user_test.gointernal/tools/bash.gointernal/tools/bash_tool_test.gointernal/tools/edit_file.gointernal/tools/file_tools_test.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/list_directory.gointernal/tools/mutation_targets.gointernal/tools/mutation_targets_test.gointernal/tools/plan_tool_test.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/skill.gointernal/tools/skill_test.gointernal/tools/task.gointernal/tools/task_test.gointernal/tools/types.gointernal/tools/update_plan.gointernal/tools/write_file.gointernal/tools/write_tools_test.gointernal/tui/ask_user_test.gointernal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/command_center.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_catalog.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/rendering.gointernal/tui/run.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/tui/session_test.gointernal/tui/transcript.gointernal/tui/view.gointernal/tui/zenline_view.gointernal/tui/zenline_view_test.gointernal/zenline/markdown.gointernal/zenline/markdown_test.gointernal/zenline/render.gointernal/zenline/render_test.gointernal/zenline/theme.gointernal/zerocommands/contracts.gointernal/zeroruntime/empty_toolcall_test.gointernal/zeroruntime/helpers.gointernal/zeroruntime/types.go
sandbox: Classify resolves command across command/cmd/script/shell (H4 destructive-gate alias bypass); rm -rf quoted/braced $HOME match (M6); firstProgram skip-list + sh -c/bash -c recursion + sudo/env option skipping (M7); piped-installer regex incl |sh/|zsh (low); chmod combined-flag/octal + rm -- (low). sessions: RestoreToSequence applies only the closest-to-target checkpoint per path (H2 + double-count low); resolveWithinWorkspace resolves symlinks to block in-workspace escape (H3); per-session flock across processes (M5); readBlob verifies sha256 (low); writeMetadata unique tmp suffix (low). TDD; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mcp: schema conversion recurses Items/Properties/Required (H6); client.request runs a reader goroutine + selects on ctx, no blocking under lock (H9); connect/initialize timeout + ctx-aware Serve reads (low). providers: Anthropic+Gemini get the OpenAI-style idle watchdog via providerio.ScanSSEDataWithContext (M4); Gemini emitDone pointer receiver (low); Anthropic emits ToolCallDropped for nameless tool_use (low); OpenAI delegates to providerio error/redact, 503/529 classified (low). zeroruntime: toolCallCollector emits in start order + doesn't merge distinct empty-id calls / clobber names (lows). TDD; build/vet/-race/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bound+reset the glamour renderer cache mdCache (M8); clip glamour output lines to frame width with ANSI-aware truncate so long URLs/code don't break the fixed-height layout (M9); PermLayout hit-test mirrors RenderChat's width/height clamps + overlay subtraction so mouse clicks land on the modal buttons (M10). TDD; full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MutationTargets resolves path/patch via the same alias lists the tools use, so aliased writes get checkpointed and /rewind works (H1); updatePlanTool guards currentPlan with a mutex against the agent/TUI goroutine race (M3); optional path/cwd args (grep/glob/list_directory) treat an explicit "" as the default instead of erroring (low); deleted dead stringSliceArg (low). TDD incl. -race; full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Redact intercepted ask_user/task outputs through the same boundary as the registry so secrets don't reach the transcript unredacted (M1); reactive mid-stream compaction retry drops OnText/OnUsage so text isn't double-streamed (M2); removed dead taskFallbackResult nil-provider branch (low); reactiveAttempted set only on a successful shrink so a no-op recover doesn't burn the one-shot budget (low); surface DroppedToolCalls even when a valid call is also present (low). TDD incl. -race; full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cancelled runs now flush in-flight session events incl. checkpoints so /rewind works + no orphaned blobs (H5); shift+tab cycles permission mode Auto/Ask/Unsafe (H7); interactive TUI defaults to Ask so write/edit/bash/apply_patch are advertised + gate through the permission prompt (H8 — VERIFY LIVE); zenline skin renders the ask_user questionnaire instead of a spinner when a prompt is pending (M11); applyExecMode runs before filter validation + --list-tools (low); mode-model deprecation notice surfaced via the shared --model path (low); zenline listed in --help (low). TDD incl. -race; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erred lows) The two remaining low findings (plugin manifest tools/hooks/prompts/skills parsed but unwired; plugin skill dirs not merged into the skill tool) are intentionally discovery-only for now — documented in code so the parsed-but-unconsumed fields aren't mistaken for dead code. Runtime wiring is tracked as a separate feature. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…afe-toggle guard, hitbox clamp loop.go: keep OnUsage on the reactive retry (drop only OnText) so token telemetry counts the retry. tui: flushRunID -> flushRunIDs set so a second cancel before the first goroutine returns doesn't lose the first run's checkpoint flush; shift+tab now toggles Auto<->Ask only (Unsafe is no longer reachable by a casual keypress since it disables permission prompts). zenline: PermLayout returns inactive geometry when the button row is clipped at small heights so clicks can't resolve to allow/deny on non-button rows. providerio_test: check pw.Close (errcheck). Tests added; build/vet/-race/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
User feedback: This branch incorporates audit fixes from 🧠 Learnings used✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/providers/openai/provider.go (1)
132-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn cancellation instead of the stale retry error.
If
ctxis canceled whilebackoffis sleeping, this loop falls through and emits the original network/5xx failure. That misreports an already-canceled run as a provider failure.Suggested fix
resp, err := provider.httpClient.Do(request) if err != nil { - if attempt < maxAttempts && ctx.Err() == nil && backoff(ctx, attempt) { - continue + if attempt < maxAttempts && ctx.Err() == nil { + if backoff(ctx, attempt) { + continue + } + if ctx.Err() != nil { + sendEvent(ctx, events, zeroruntime.StreamEvent{ + Type: zeroruntime.StreamEventError, + Error: provider.redact("provider stream error: " + ctx.Err().Error()), + }) + return + } } sendEvent(ctx, events, zeroruntime.StreamEvent{Type: zeroruntime.StreamEventError, Error: provider.redact("provider stream error: " + err.Error())}) return } - if resp.StatusCode >= http.StatusInternalServerError && attempt < maxAttempts && backoff(ctx, attempt) { - _ = resp.Body.Close() - continue + if resp.StatusCode >= http.StatusInternalServerError && attempt < maxAttempts { + if backoff(ctx, attempt) { + _ = resp.Body.Close() + continue + } + if ctx.Err() != nil { + _ = resp.Body.Close() + sendEvent(ctx, events, zeroruntime.StreamEvent{ + Type: zeroruntime.StreamEventError, + Error: provider.redact("provider stream error: " + ctx.Err().Error()), + }) + return + } }Based on learnings: Verify that the review calls out the risk surface and whether it introduces a blocker if the PR touches auth, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts.
🤖 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 132 - 145, The retry loop around provider.httpClient.Do should detect mid-backoff context cancellation and return that cancellation instead of emitting the original network/5xx error; after any backoff(ctx, attempt) call (both in the Do error branch and the 5xx branch) check ctx.Err() and if non-nil sendEvent with a cancellation StreamEvent (use events, zeroruntime.StreamEvent{Type: zeroruntime.StreamEventError, Error: provider.redact(ctx.Err().Error())}) and return, so the cancellation is reported instead of the stale provider error; update the logic around provider.httpClient.Do, backoff(ctx, attempt), sendEvent and response handling accordingly.internal/sandbox/risk.go (1)
181-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve tolerated path aliases in
requestPaths.This PR hardens command alias handling, but path classification still only looks at
path,cwd,file, anddir. With the new arg-tolerance layer accepting aliases likefile_path, an accepted tool invocation can skipabsolute_path,path_escape, and workspace-boundary checks.Suggested fix
func requestPaths(request Request) []string { paths := []string{} - for _, key := range []string{"path", "cwd", "file", "dir"} { + for _, key := range []string{"path", "file", "file_path", "cwd", "dir", "directory"} { if value := argString(request.Args, key); value != "" { paths = append(paths, value) } } return paths }Based on learnings: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
🤖 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/risk.go` around lines 181 - 189, requestPaths currently only checks "path","cwd","file","dir" so aliased args (e.g., "file_path","absolute_path","path_escape" and other workspace-related aliases) slip through; update requestPaths to include those tolerated path aliases when calling argString (or add a small alias-to-canonical list) so values from "file_path", "absolute_path", "path_escape", "workspace_path"/"working_dir" (or any aliases introduced by the arg-tolerance layer) are also appended; reference the requestPaths function and the argString call so the change consistently checks aliases and normalizes them into the returned paths slice.
🧹 Nitpick comments (1)
internal/agent/task_test.go (1)
131-174: ⚡ Quick winAdd assertion for redaction notice in scrubbed output.
The test verifies the secret is removed and
Redacted=true, but doesn't confirm a redaction notice was appended. For consistency withTestRunScrubsSecretsFromToolOutputinloop_test.go(lines 924-926), add:if !captured.Redacted { t.Error("expected Redacted=true when a secret was scrubbed from a sub-agent final answer") } + if !strings.Contains(strings.ToLower(captured.Output), "redacted") { + t.Errorf("expected a redaction reminder in task output, got %q", captured.Output) + }This ensures the parent agent receives a clear signal that content was scrubbed.
🤖 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/task_test.go` around lines 131 - 174, In TestRunRedactsSecretsInTaskChildFinalAnswer add an assertion that the scrubbed tool output contains the standard redaction notice (the same string asserted in TestRunScrubsSecretsFromToolOutput) so the parent agent sees a clear signal; update the test that uses captured (from Run) to assert captured.Output contains the canonical redaction notice in addition to the existing checks of strings.Contains(captured.Output, secret) and captured.Redacted.
🤖 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_test.go`:
- Around line 412-417: The test comment and assertion disagree: the code
currently builds joined := strings.Join(deltas, "") and asserts that
strings.Count(joined, "recovered") == 0 (no occurrences), but the comment says
"at most once"; update the comment above that block to state that the retried
turn "must NOT be re-streamed" (or "must appear zero times") so it matches the
assertion in the test (refer to variables joined, deltas and the Count check for
"recovered").
In `@internal/cli/app_test.go`:
- Line 104: The test change switches the default TUI permission mode from
PermissionModeAuto to PermissionModeAsk (see assertAgentOptions,
launchedOptions, PermissionModeAsk/PermissionModeAuto); update the PR to
explicitly document this behavioral change (changelog/PR description and commit
message), add a note requesting maintainer/SEC sign-off, and add or adjust tests
that verify which tool permissions are advertised under PermissionModeAsk (e.g.,
ensure PermissionPrompt tools are advertised and gated) so the
permission-surface impact is clearly covered and reviewed rather than hidden in
unrelated cleanup.
In `@internal/providers/providerio/providerio.go`:
- Around line 104-120: When idleTimeout <= 0 we must not call ScanSSEData (which
ignores ctx.Done) — keep ctx cancellation active; change the idleTimeout<=0
branch in ScanSSEDataWithContext to reuse the same scanner/forwarding goroutine
and consumer select as the timed path but simply omit the idle timer: still
select on ctx.Done (return ctx.Err()), the payload channel, and call cancel()
only where the timed path would; do not call ScanSSEData directly so ctx
cancellation and cancel remain honored.
In `@internal/sessions/rewind_test.go`:
- Around line 123-126: The call to store.AppendEvent in the test ignores its
error return; update the test to capture and assert the returned error from
store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload:
CheckpointPayload{...}}) and fail the test if non-nil (e.g., t.Fatalf or
require.NoError). Ensure you check the error immediately after the AppendEvent
call so the test setup reliably aborts on AppendEvent failures.
- Around line 173-176: The call to store.AppendEvent in the test (and other
occurrences in this file) ignores the returned error; capture its return value
(err := store.AppendEvent(...)) and fail the test on error—either use
t.Fatalf("AppendEvent failed: %v", err) or require.NoError(t, err) if testify is
available—so update each AppendEvent usage (and similar helper calls) to check
and handle the error instead of discarding it.
- Around line 74-81: The test setup calls store.AppendEvent twice
(store.AppendEvent in rewind_test.go) without checking returned errors; update
both calls to capture the error and assert success (e.g., err :=
store.AppendEvent(...); require.NoError(t, err) or if err != nil { t.Fatalf(...)
}) so the test fails immediately if appending events fails and does not continue
with a silent setup failure.
- Around line 27-30: The two calls to store.AppendEvent used to seed checkpoint
data are ignoring returned errors; capture the error return from each
store.AppendEvent(...) call and fail the test on error (e.g. use
require.NoError(t, err) or if err != nil { t.Fatalf(...) }) so the test setup
cannot silently succeed when AppendEvent fails—apply this change for both
AppendEvent invocations that create the CheckpointPayload entries.
---
Outside diff comments:
In `@internal/providers/openai/provider.go`:
- Around line 132-145: The retry loop around provider.httpClient.Do should
detect mid-backoff context cancellation and return that cancellation instead of
emitting the original network/5xx error; after any backoff(ctx, attempt) call
(both in the Do error branch and the 5xx branch) check ctx.Err() and if non-nil
sendEvent with a cancellation StreamEvent (use events,
zeroruntime.StreamEvent{Type: zeroruntime.StreamEventError, Error:
provider.redact(ctx.Err().Error())}) and return, so the cancellation is reported
instead of the stale provider error; update the logic around
provider.httpClient.Do, backoff(ctx, attempt), sendEvent and response handling
accordingly.
In `@internal/sandbox/risk.go`:
- Around line 181-189: requestPaths currently only checks
"path","cwd","file","dir" so aliased args (e.g.,
"file_path","absolute_path","path_escape" and other workspace-related aliases)
slip through; update requestPaths to include those tolerated path aliases when
calling argString (or add a small alias-to-canonical list) so values from
"file_path", "absolute_path", "path_escape", "workspace_path"/"working_dir" (or
any aliases introduced by the arg-tolerance layer) are also appended; reference
the requestPaths function and the argString call so the change consistently
checks aliases and normalizes them into the returned paths slice.
---
Nitpick comments:
In `@internal/agent/task_test.go`:
- Around line 131-174: In TestRunRedactsSecretsInTaskChildFinalAnswer add an
assertion that the scrubbed tool output contains the standard redaction notice
(the same string asserted in TestRunScrubsSecretsFromToolOutput) so the parent
agent sees a clear signal; update the test that uses captured (from Run) to
assert captured.Output contains the canonical redaction notice in addition to
the existing checks of strings.Contains(captured.Output, secret) and
captured.Redacted.
🪄 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: c107e844-76b1-4264-9716-abb4ab89881c
📒 Files selected for processing (62)
.gitignoredocs/audit-2026-06-07-deep-audit.mdgo.modinternal/agent/ask_user_test.gointernal/agent/compaction.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/task_test.gointernal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/mcp/client.gointernal/mcp/hang_test.gointernal/mcp/schema.gointernal/mcp/schema_test.gointernal/mcp/server.gointernal/plugins/plugins.gointernal/providers/anthropic/dropped_test.gointernal/providers/anthropic/idle_test.gointernal/providers/anthropic/provider.gointernal/providers/gemini/done_test.gointernal/providers/gemini/idle_test.gointernal/providers/gemini/provider.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/providerio/providerio.gointernal/providers/providerio/providerio_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sessions/checkpoint.gointernal/sessions/filelock_other.gointernal/sessions/filelock_unix.gointernal/sessions/rewind.gointernal/sessions/rewind_test.gointernal/sessions/store.gointernal/skills/skills.gointernal/tools/argtolerance_test.gointernal/tools/ask_user.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/list_directory.gointernal/tools/mutation_targets.gointernal/tools/mutation_targets_test.gointernal/tools/plan_tool_test.gointernal/tools/update_plan.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/session.gointernal/tui/session_test.gointernal/tui/view.gointernal/tui/zenline_view.gointernal/tui/zenline_view_test.gointernal/zenline/markdown.gointernal/zenline/markdown_test.gointernal/zenline/render.gointernal/zenline/render_test.gointernal/zeroruntime/helpers.gointernal/zeroruntime/order_test.go
💤 Files with no reviewable changes (1)
- internal/tools/ask_user.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- internal/zeroruntime/order_test.go
- docs/audit-2026-06-07-deep-audit.md
🚧 Files skipped from review as they are similar to previous changes (22)
- internal/tools/glob.go
- internal/tools/grep.go
- internal/tools/list_directory.go
- internal/zenline/markdown.go
- internal/tools/mutation_targets_test.go
- internal/providers/openai/provider_test.go
- internal/tui/zenline_view_test.go
- internal/tools/mutation_targets.go
- internal/plugins/plugins.go
- internal/sandbox/safe_command_test.go
- internal/skills/skills.go
- internal/zenline/render_test.go
- internal/tui/zenline_view.go
- internal/sandbox/safe_command.go
- internal/tools/argtolerance_test.go
- internal/cli/exec.go
- internal/agent/compaction.go
- internal/agent/loop.go
- internal/zenline/render.go
- internal/sessions/checkpoint.go
- internal/sessions/rewind.go
- internal/tui/model.go
Comment on lines
+412
to
+417
| // The retried turn's text must be streamed to OnText at most once. Before the | ||
| // fix it was emitted on both the original (mid-stream) collect AND the retry | ||
| // collect, double-emitting the retried response. | ||
| joined := strings.Join(deltas, "") | ||
| if got := strings.Count(joined, "recovered"); got != 0 { | ||
| t.Fatalf("retried-turn text must NOT be re-streamed to OnText, saw %d occurrences in %q", got, joined) |
There was a problem hiding this comment.
Clarify comment to match assertion.
The comment says "at most once" but the assertion checks for exactly zero occurrences. "At most once" typically means ≤1 (0 or 1 times), but got != 0 fails when count is 1 or greater, enforcing exactly 0.
Either update the comment to say "must not be re-streamed" or "must appear exactly zero times", or change the assertion to got > 1 if the intent is truly "at most once".
📝 Suggested comment fix
-// The retried turn's text must be streamed to OnText at most once. Before the
-// fix it was emitted on both the original (mid-stream) collect AND the retry
-// collect, double-emitting the retried response.
+// The retried turn's text must NOT be re-streamed to OnText. Before the fix it
+// was emitted on both the original (mid-stream) collect AND the retry collect,
+// double-emitting the retried response.
joined := strings.Join(deltas, "")
if got := strings.Count(joined, "recovered"); got != 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The retried turn's text must be streamed to OnText at most once. Before the | |
| // fix it was emitted on both the original (mid-stream) collect AND the retry | |
| // collect, double-emitting the retried response. | |
| joined := strings.Join(deltas, "") | |
| if got := strings.Count(joined, "recovered"); got != 0 { | |
| t.Fatalf("retried-turn text must NOT be re-streamed to OnText, saw %d occurrences in %q", got, joined) | |
| // The retried turn's text must NOT be re-streamed to OnText. Before the fix it | |
| // was emitted on both the original (mid-stream) collect AND the retry collect, | |
| // double-emitting the retried response. | |
| joined := strings.Join(deltas, "") | |
| if got := strings.Count(joined, "recovered"); got != 0 { | |
| t.Fatalf("retried-turn text must NOT be re-streamed to OnText, saw %d occurrences in %q", got, joined) |
🤖 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_test.go` around lines 412 - 417, The test comment
and assertion disagree: the code currently builds joined := strings.Join(deltas,
"") and asserts that strings.Count(joined, "recovered") == 0 (no occurrences),
but the comment says "at most once"; update the comment above that block to
state that the retried turn "must NOT be re-streamed" (or "must appear zero
times") so it matches the assertion in the test (refer to variables joined,
deltas and the Count check for "recovered").
Comment on lines
+104
to
+120
| // ScanSSEDataWithContext parses SSE data payloads while enforcing an idle | ||
| // timeout and honoring ctx cancellation. The blocking scan runs on a goroutine | ||
| // that forwards each completed payload over a buffered channel; this consumer | ||
| // selects on ctx.Done, the idle timer, and incoming payloads. When the upstream | ||
| // goes silent for idleTimeout, cancel is invoked to abort the in-flight request | ||
| // (unblocking the reader) and ErrStreamIdle is returned. On ctx cancellation | ||
| // ctx.Err() is returned. A non-positive idleTimeout disables the watchdog. | ||
| func ScanSSEDataWithContext( | ||
| ctx context.Context, | ||
| cancel context.CancelFunc, | ||
| reader io.Reader, | ||
| idleTimeout time.Duration, | ||
| handle func(data string) bool, | ||
| ) error { | ||
| if idleTimeout <= 0 { | ||
| return ScanSSEData(reader, handle) | ||
| } |
There was a problem hiding this comment.
Keep ctx cancellation active when the watchdog is disabled.
idleTimeout <= 0 currently drops into ScanSSEData, which never observes ctx.Done(). That means callers lose cancellation entirely on this code path, even though the docstring says only the idle watchdog is disabled.
Suggested direction
- if idleTimeout <= 0 {
- return ScanSSEData(reader, handle)
- }
-
scanner := bufio.NewScanner(reader)
scanner.Buffer(make([]byte, 0, 4096), maxSSELineBytes)
+ idleEnabled := idleTimeout > 0
...
- idle := time.NewTimer(idleTimeout)
- defer idle.Stop()
+ var idle *time.Timer
+ var idleC <-chan time.Time
+ if idleEnabled {
+ idle = time.NewTimer(idleTimeout)
+ defer idle.Stop()
+ idleC = idle.C
+ }
...
- case <-idle.C:
+ case <-idleC:Based on learnings: Verify that the review calls out the risk surface and whether it introduces a blocker if the PR touches auth, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts.
🤖 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/providerio/providerio.go` around lines 104 - 120, When
idleTimeout <= 0 we must not call ScanSSEData (which ignores ctx.Done) — keep
ctx cancellation active; change the idleTimeout<=0 branch in
ScanSSEDataWithContext to reuse the same scanner/forwarding goroutine and
consumer select as the timed path but simply omit the idle timer: still select
on ctx.Done (return ctx.Err()), the payload channel, and call cancel() only
where the timed path would; do not call ScanSSEData directly so ctx cancellation
and cancel remain honored.
Comment on lines
+27
to
+30
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | ||
| Tool: "edit_file", | ||
| Files: []CheckpointFile{{Path: "a.txt", Skipped: true}}, | ||
| }}) |
There was a problem hiding this comment.
Check error returns from store.AppendEvent.
The test seeds checkpoint data by calling store.AppendEvent at lines 27 and 32 without checking the returned error. If these calls fail silently, the test setup is invalid and test results are misleading.
🛡️ Proposed fix
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "edit_file",
Files: []CheckpointFile{{Path: "a.txt", Skipped: true}},
-}})
+}}); err != nil {
+ t.Fatal(err)
+}
// Newer checkpoint (further from target) has a real blob.
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "edit_file",
Files: []CheckpointFile{{Path: "a.txt", Blob: hash}},
-}})
+}}); err != nil {
+ t.Fatal(err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Skipped: true}}, | |
| }}) | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Skipped: true}}, | |
| }}); err != nil { | |
| t.Fatal(err) | |
| } | |
| // Newer checkpoint (further from target) has a real blob. | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: hash}}, | |
| }}); err != nil { | |
| t.Fatal(err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 27-27: Error return value of store.AppendEvent is not checked
(errcheck)
🤖 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/rewind_test.go` around lines 27 - 30, The two calls to
store.AppendEvent used to seed checkpoint data are ignoring returned errors;
capture the error return from each store.AppendEvent(...) call and fail the test
on error (e.g. use require.NoError(t, err) or if err != nil { t.Fatalf(...) })
so the test setup cannot silently succeed when AppendEvent fails—apply this
change for both AppendEvent invocations that create the CheckpointPayload
entries.
Comment on lines
+74
to
+81
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | ||
| Tool: "edit_file", | ||
| Files: []CheckpointFile{{Path: "a.txt", Blob: oldHash}}, | ||
| }}) | ||
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | ||
| Tool: "edit_file", | ||
| Files: []CheckpointFile{{Path: "a.txt", Blob: newHash}}, | ||
| }}) |
There was a problem hiding this comment.
Check error returns from store.AppendEvent.
Similar to the previous test, lines 74 and 78 call store.AppendEvent without checking errors, which can lead to misleading test results if the setup fails silently.
🛡️ Proposed fix
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "edit_file",
Files: []CheckpointFile{{Path: "a.txt", Blob: oldHash}},
-}})
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+}}); err != nil {
+ t.Fatal(err)
+}
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "edit_file",
Files: []CheckpointFile{{Path: "a.txt", Blob: newHash}},
-}})
+}}); err != nil {
+ t.Fatal(err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: oldHash}}, | |
| }}) | |
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: newHash}}, | |
| }}) | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: oldHash}}, | |
| }}); err != nil { | |
| t.Fatal(err) | |
| } | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: newHash}}, | |
| }}); err != nil { | |
| t.Fatal(err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 74-74: Error return value of store.AppendEvent is not checked
(errcheck)
🤖 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/rewind_test.go` around lines 74 - 81, The test setup calls
store.AppendEvent twice (store.AppendEvent in rewind_test.go) without checking
returned errors; update both calls to capture the error and assert success
(e.g., err := store.AppendEvent(...); require.NoError(t, err) or if err != nil {
t.Fatalf(...) }) so the test fails immediately if appending events fails and
does not continue with a silent setup failure.
Comment on lines
+123
to
+126
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | ||
| Tool: "edit_file", | ||
| Files: []CheckpointFile{{Path: "a.txt", Blob: hash}}, | ||
| }}) |
There was a problem hiding this comment.
Check error return from store.AppendEvent.
Line 123 calls store.AppendEvent without checking the error, which can invalidate test setup if the call fails.
🛡️ Proposed fix
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "edit_file",
Files: []CheckpointFile{{Path: "a.txt", Blob: hash}},
-}})
+}}); err != nil {
+ t.Fatal(err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: hash}}, | |
| }}) | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "edit_file", | |
| Files: []CheckpointFile{{Path: "a.txt", Blob: hash}}, | |
| }}); err != nil { | |
| t.Fatal(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/sessions/rewind_test.go` around lines 123 - 126, The call to
store.AppendEvent in the test ignores its error return; update the test to
capture and assert the returned error from store.AppendEvent("s",
AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{...}})
and fail the test if non-nil (e.g., t.Fatalf or require.NoError). Ensure you
check the error immediately after the AppendEvent call so the test setup
reliably aborts on AppendEvent failures.
Comment on lines
+173
to
+176
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | ||
| Tool: "write_file", | ||
| Files: []CheckpointFile{{Path: "link/secret.txt", Blob: hash}}, | ||
| }}) |
There was a problem hiding this comment.
Check error return from store.AppendEvent.
Line 173 calls store.AppendEvent without checking the error. This pattern is repeated throughout the file and should be fixed for test reliability.
🛡️ Proposed fix
-store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
+if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{
Tool: "write_file",
Files: []CheckpointFile{{Path: "link/secret.txt", Blob: hash}},
-}})
+}}); err != nil {
+ t.Fatal(err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "write_file", | |
| Files: []CheckpointFile{{Path: "link/secret.txt", Blob: hash}}, | |
| }}) | |
| if _, err := store.AppendEvent("s", AppendEventInput{Type: EventSessionCheckpoint, Payload: CheckpointPayload{ | |
| Tool: "write_file", | |
| Files: []CheckpointFile{{Path: "link/secret.txt", Blob: hash}}, | |
| }}); err != nil { | |
| t.Fatal(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/sessions/rewind_test.go` around lines 173 - 176, The call to
store.AppendEvent in the test (and other occurrences in this file) ignores the
returned error; capture its return value (err := store.AppendEvent(...)) and
fail the test on error—either use t.Fatalf("AppendEvent failed: %v", err) or
require.NoError(t, err) if testify is available—so update each AppendEvent usage
(and similar helper calls) to check and handle the error instead of discarding
it.
…102) docs/audit-2026-06-07-deep-audit.md was swept in by the same stray 'git add -A' as shop.html (18108b8). Its content is already tracked as GitHub issue #102, so it doesn't belong in the tree. Untracked (local copy kept) + gitignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
docs/superpowers/specs/*.md are local brainstorming/design process output, not shipped project docs (unlike PRD.md/INSTALL.md). Untrack (local copies kept) + gitignore docs/superpowers/. confirmation_policy.md is intentionally kept — it is go:embed'd into the agent loop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sandbox: quoted-root rm -rf bypass (HIGH); interactive-detector quote/escape-in-token + segment-boundary false positives; chmod single-file not destructive. sessions: Fork copies checkpoint blobs so fork rewind works (HIGH); ApplyRewind atomic under one lock; pruneOrphanBlobs+capture race fixed under lock (verified real); restore preserves file mode; size-cap int64 (no 32-bit overflow); sessionLocks growth documented. TDD incl -race; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
providers: OpenAI requests stream_options.include_usage (usage no longer 0); terminal finish/stop reason surfaced via CollectedStream.FinishReason+Truncated() across OpenAI/Anthropic/Gemini; Gemini emits ToolCallDropped for nameless functionCall; OpenAI max_completion_tokens wired; OpenAI uses providerio SSE (multi-line data join). runtime: empty-id delta-before-start no longer orphans args. zenline: overlay suppressed under permission prompt (PermLayout agreement) + overlay height capped; clip() display-width-aware; dead detailLines removed. TDD; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
grep no longer follows in-workspace symlinks to files outside the root (confinement) and computes clean workspace-relative match paths under a symlinked root (HIGH x2); apply_patch ChangedFiles + MutationTargets are workspace-relative when cwd!="." (HIGH); aliasedStringArg(allowEmpty) skips an empty primary so a populated alias wins — no more write_file content/path data loss (HIGH); registry.Without isolates a fresh update_plan for sub-agents so a task child can't clobber the parent's plan (MED). TDD incl -race; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
executeTask propagates OnUsage into the child run so sub-agent token usage is counted (cost telemetry; activity callbacks stay nil) (MED); repeated-failure guard Stop mid-turn now appends aborted-placeholder tool results for unexecuted calls so result.Messages stays structurally valid (every tool_use answered) (LOW). The 'dead AgentEventType' finding was rejected — it's an intended TUI/headless/sessions contract, not dead code. TDD incl -race; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tui: Ctrl+C now flushes the in-flight run's checkpoint session events before quitting (no orphaned blobs / broken rewind), mirroring Esc-cancel (MED); removed dead permission-modal MouseMsg branch (mouse capture is intentionally off) + its test (MED); /model//mode//effort//theme pickers refuse to open mid-run with a clear message instead of opening then refusing the selection (LOW). mcp: RegisterTools is atomic — stages all servers and commits only if all succeed, so a later server failure no longer leaves dead tools (LOW). skills: duplicate frontmatter names resolve deterministically (lexicographically-first dir wins) + Duplicates() surfaces collisions (LOW). TDD incl -race; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cli: --reasoning-effort notice evaluates the EFFECTIVE model (works without --model) (MED); unsafe warning fires for --auto high too (MED); --auto value validated even with --skip-permissions-unsafe (LOW); DefaultRegistry built once per exec, not 3x (LOW); --profile kept as intentional legacy-compat accept-and-ignore (contested finding resolved, doc-commented). modelregistry: removed dead FallbackFor. tools: removed dead TaskNonInteractiveMessage + dead OnSandboxDecision hook (no setter anywhere); fixed the now-stale loop.go comment. TDD where behavioral; build/vet/full-suite green. Refs #102 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review Pushed a second round of fixes (6 commits, |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect the key high-severity areas from the second round of fixes
echo "=== grep.go - symlink confinement + clean rel-paths ==="
cat internal/tools/grep.go
echo ""
echo "=== apply_patch.go - workspace-relative ChangedFiles ==="
cat internal/tools/apply_patch.go
echo ""
echo "=== argtolerance.go - empty-alias masking fix ==="
grep -n "allowEmpty\|aliasedStringArg\|empty" internal/tools/argtolerance.go | head -60
echo ""
echo "=== sandbox/risk.go - quoted-root rm -rf gate ==="
cat internal/sandbox/risk.go
echo ""
echo "=== sessions/checkpoint.go - Fork copies checkpoint blobs ==="
grep -n "fork\|Fork\|copy\|Clone\|blob\|CopyBlobs\|copyBlob" internal/sessions/checkpoint.go | head -40
echo ""
echo "=== sessions/rewind.go - symlink path guard ==="
grep -n "symlink\|Lstat\|Readlink\|resolveWithin\|EvalSymlinks" internal/sessions/rewind.go | head -30
echo ""
echo "=== git log of recent commits ==="
git log --oneline -10Length of output: 26669
Full review queued — the re-review will cover all 40 fixed findings plus the full PR surface. 🧠 Learnings used✅ Action performedFull review finished. |
gnanam1990
marked this pull request as draft
June 7, 2026 04:57
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sessions/store.go (1)
283-345:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLock the parent session while snapshotting a fork.
Forkstill readsevents.jsonland then copies checkpoint blobs from the parent without ever takinglockSession(parentSessionID). A concurrentApplyRewindininternal/sessions/rewind.goorCaptureToolCheckpoint/pruneOrphanBlobsininternal/sessions/checkpoint.gocan change the parent between those steps, so the child can inherit checkpoint events whose blobs were already pruned, or return afterCreatewith a half-populated fork. Snapshot the parent under its session lock before building the child, and use a fixed lock order if the child also needs locking.🤖 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 283 - 345, Fork currently reads parent events and copies blobs without holding the parent session lock; wrap the snapshot operation in lockSession(parentSessionID) so the parent is stable while you ReadEvents, compute last, and before you call Create. After creating the fork (fork := ... via store.Create), if you need to lock the child for appending/copying, acquire locks in a consistent order (always lockSession(parentSessionID) before lockSession(fork.SessionID)) to avoid deadlocks, then perform store.copyBlobs(parent.SessionID, fork.SessionID) and the subsequent store.AppendEvent calls while both locks are held; remember to defer unlocking in each case. Use the existing lockSession/unlockSession helpers and reference the Fork function, store.Create, store.ReadEvents, store.copyBlobs, and store.AppendEvent when making the changes.internal/sessions/checkpoint.go (1)
97-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't checkpoint escaped or unreadable paths as deletions.
SnapshotForCheckpointcurrently doesfilepath.Join(workspaceRoot, rel)and thenos.Stat/os.ReadFile, solink/secret.txtwill follow an in-workspace symlink and persist outside-workspace content intocheckpoints/blobsbefore rewind ever gets a chance to reject the path. On top of that, thestatErr != nilbranch marks every failure asAbsent, so anEPERM/EIOcase becomes a futureos.Removeduring rewind. Resolve each candidate with the same symlink-aware workspace guard used by restore, setAbsentonly onos.IsNotExist, and mark every other failure asSkipped.Suggested fix
for _, rel := range paths { entry := CheckpointFile{Path: rel} - abs := filepath.Join(workspaceRoot, rel) + abs, ok := resolveWithinWorkspace(workspaceRoot, rel) + if !ok { + entry.Skipped = true + files = append(files, entry) + continue + } info, statErr := os.Stat(abs) if statErr != nil { - // Treat anything we cannot stat as absent (new file) — restore deletes it. - entry.Absent = true + if os.IsNotExist(statErr) { + entry.Absent = true + } else { + entry.Skipped = true + } files = append(files, entry) continue }🤖 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/checkpoint.go` around lines 97 - 137, SnapshotForCheckpoint currently joins workspaceRoot+rel and blindly stats/reads it, which follows symlinks and treats all stat errors as Absent; fix it by first resolving rel with the same symlink-aware workspace-guard helper used by restore (use that helper to produce a safe resolved path or an error), use the resolved path for os.Stat/os.ReadFile, set entry.Absent = true only when os.IsNotExist(err), mark entry.Skipped = true for any other resolution/stat/read/write errors, and ensure you never call os.ReadFile or store.writeBlob for paths resolved outside the workspace (use the resolved path variable instead of the current abs).
🧹 Nitpick comments (2)
internal/zeroruntime/helpers.go (1)
156-190: ⚡ Quick winConsider adding inline scenario comments for pendingEmptyDelta edge cases.
The
pendingEmptyDeltalogic correctly handles empty-id deltas arriving before starts, but it's intricate. Adding inline comments documenting key scenarios (e.g., delta→start→end, delta→end→start) would help future maintainers avoid introducing bugs.🤖 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/zeroruntime/helpers.go` around lines 156 - 190, Add concise inline comments around the pendingEmptyDelta/openEmptyID handling in toolCallCollector.delta and toolCallCollector.end to document the key edge-case scenarios and why the code manipulates those fields: describe the delta→start→end path where a synthetic key is created, reserved in openEmptyID and adopted by a subsequent start (pendingEmptyDelta should remain set), the delta→end→start path where a delta-created synthetic key is closed before any start adopts it (pendingEmptyDelta must be cleared when closed), and mention why ensure/synthetic and calls[key].Arguments updates rely on this behavior; place these short scenario notes adjacent to the code that sets collector.pendingEmptyDelta, appends to collector.openEmptyID, and the branch that clears pendingEmptyDelta in end.internal/providers/gemini/provider.go (1)
471-482: Confirm Gemini finishReason mappings for PROHIBITED_CONTENT/BLOCKLIST/SPIIPROHIBITED_CONTENT, BLOCKLIST, and SPII are valid Gemini
finishReasonenum values for content-filtered halts, so mapping them tozeroruntime.FinishReasonContentFilteris appropriate. The inline doc comment just doesn’t mention these additional finishReason strings—consider expanding it beyond theSAFETYreference.🤖 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/gemini/provider.go` around lines 471 - 482, Update the doc comment for mapFinishReason to list all Gemini finishReason values that map to the runtime's content-filter terminal reason (include "SAFETY", "PROHIBITED_CONTENT", "BLOCKLIST", and "SPII"), and ensure the comment clearly states that these map to zeroruntime.FinishReasonContentFilter while noting that a normal stop ("STOP" or empty string) returns an empty string; modify the comment above func mapFinishReason to reflect these exact enum strings and the mapping to zeroruntime.FinishReasonContentFilter.
🤖 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 307-329: The program index lookup must use the same normalization
rule as firstProgram: change programIndex to normalize each candidate token with
normalizeProgramToken before comparing against the target executable so callers
like hasNonInteractiveFlag and shellDashCPayload see the same resolved program
(this ensures tokens like /usr/bin/python or "python" match); update the second
occurrence of this comparison (the similar logic referenced in the later block
around the other programIndex use) the same way so both lookup sites normalize
tokens prior to equality checks.
- Around line 131-145: The segment matching can miss commands with path or
quoting (e.g., "/usr/bin/git" or "\"git\"") because commandBody(...) returns the
raw first token; update the loop that iterates splitShellSegments(normalized) to
normalize the first real command token before comparing to interactiveSegments:
extract the first token from commandBody(strings.Fields(segment)), strip
surrounding quotes and environment/path prefixes (use something like basename
semantics to remove directories), lower-case it, then rebuild the normalized
segment body to check equality or prefix against seg.match; apply the same
normalization logic where interactiveSegments are checked (also the similar
block around lines 237-263) so InteractiveCommandResult detection covers
path-prefixed or quoted executables like git.
---
Outside diff comments:
In `@internal/sessions/checkpoint.go`:
- Around line 97-137: SnapshotForCheckpoint currently joins workspaceRoot+rel
and blindly stats/reads it, which follows symlinks and treats all stat errors as
Absent; fix it by first resolving rel with the same symlink-aware
workspace-guard helper used by restore (use that helper to produce a safe
resolved path or an error), use the resolved path for os.Stat/os.ReadFile, set
entry.Absent = true only when os.IsNotExist(err), mark entry.Skipped = true for
any other resolution/stat/read/write errors, and ensure you never call
os.ReadFile or store.writeBlob for paths resolved outside the workspace (use the
resolved path variable instead of the current abs).
In `@internal/sessions/store.go`:
- Around line 283-345: Fork currently reads parent events and copies blobs
without holding the parent session lock; wrap the snapshot operation in
lockSession(parentSessionID) so the parent is stable while you ReadEvents,
compute last, and before you call Create. After creating the fork (fork := ...
via store.Create), if you need to lock the child for appending/copying, acquire
locks in a consistent order (always lockSession(parentSessionID) before
lockSession(fork.SessionID)) to avoid deadlocks, then perform
store.copyBlobs(parent.SessionID, fork.SessionID) and the subsequent
store.AppendEvent calls while both locks are held; remember to defer unlocking
in each case. Use the existing lockSession/unlockSession helpers and reference
the Fork function, store.Create, store.ReadEvents, store.copyBlobs, and
store.AppendEvent when making the changes.
---
Nitpick comments:
In `@internal/providers/gemini/provider.go`:
- Around line 471-482: Update the doc comment for mapFinishReason to list all
Gemini finishReason values that map to the runtime's content-filter terminal
reason (include "SAFETY", "PROHIBITED_CONTENT", "BLOCKLIST", and "SPII"), and
ensure the comment clearly states that these map to
zeroruntime.FinishReasonContentFilter while noting that a normal stop ("STOP" or
empty string) returns an empty string; modify the comment above func
mapFinishReason to reflect these exact enum strings and the mapping to
zeroruntime.FinishReasonContentFilter.
In `@internal/zeroruntime/helpers.go`:
- Around line 156-190: Add concise inline comments around the
pendingEmptyDelta/openEmptyID handling in toolCallCollector.delta and
toolCallCollector.end to document the key edge-case scenarios and why the code
manipulates those fields: describe the delta→start→end path where a synthetic
key is created, reserved in openEmptyID and adopted by a subsequent start
(pendingEmptyDelta should remain set), the delta→end→start path where a
delta-created synthetic key is closed before any start adopts it
(pendingEmptyDelta must be cleared when closed), and mention why
ensure/synthetic and calls[key].Arguments updates rely on this behavior; place
these short scenario notes adjacent to the code that sets
collector.pendingEmptyDelta, appends to collector.openEmptyID, and the branch
that clears pendingEmptyDelta in end.
🪄 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: e9f24369-d3c7-4021-93e5-8ab8bc03f756
📒 Files selected for processing (50)
internal/agent/loop.gointernal/agent/loop_test.gointernal/cli/exec.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/mcp/registry.gointernal/mcp/registry_test.gointernal/modelregistry/resolve.gointernal/modelregistry/resolve_test.gointernal/providers/anthropic/finish_reason_test.gointernal/providers/anthropic/provider.gointernal/providers/factory.gointernal/providers/gemini/finish_reason_test.gointernal/providers/gemini/provider.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/tool_state.gointernal/providers/openai/types.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.gointernal/sessions/checkpoint.gointernal/sessions/rewind.gointernal/sessions/rewind_test.gointernal/sessions/store.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/tools/apply_patch.gointernal/tools/argtolerance.gointernal/tools/argtolerance_test.gointernal/tools/file_tools_test.gointernal/tools/grep.gointernal/tools/mutation_targets.gointernal/tools/mutation_targets_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/task.gointernal/tools/write_tools_test.gointernal/tui/model.gointernal/tui/picker_test.gointernal/tui/rendering.gointernal/tui/session_test.gointernal/tui/zenline_view_test.gointernal/zenline/render.gointernal/zenline/render_overlay_test.gointernal/zeroruntime/empty_toolcall_test.gointernal/zeroruntime/finish_reason_test.gointernal/zeroruntime/helpers.gointernal/zeroruntime/types.go
💤 Files with no reviewable changes (4)
- internal/modelregistry/resolve.go
- internal/tools/task.go
- internal/modelregistry/resolve_test.go
- internal/tui/zenline_view_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/tools/mutation_targets.go
- internal/sandbox/risk.go
- internal/zeroruntime/empty_toolcall_test.go
- internal/tools/write_tools_test.go
- internal/tools/argtolerance_test.go
- internal/tools/apply_patch.go
- internal/providers/anthropic/provider.go
- internal/tui/session_test.go
- internal/agent/loop.go
- internal/tools/registry_test.go
- internal/providers/openai/tool_state.go
- internal/sandbox/risk_hardening_test.go
- internal/sandbox/safe_command_test.go
- internal/tools/argtolerance.go
- internal/cli/exec.go
- internal/tui/model.go
- internal/zenline/render.go
Comment on lines
+131
to
+145
| // Multi-word interactive invocations (flags/subcommands) take priority so | ||
| // the more specific message wins. Match only at a real command boundary — | ||
| // the start of a shell segment (after leading env-assignments and wrapper | ||
| // prefixes) — so the segment text appearing inside a quoted argument (e.g. | ||
| // `echo "git rebase -i ..."`) is NOT a false positive. | ||
| for _, segment := range splitShellSegments(normalized) { | ||
| body := strings.ToLower(commandBody(strings.Fields(segment))) | ||
| for _, seg := range interactiveSegments { | ||
| if body == seg.match || strings.HasPrefix(body, seg.match+" ") { | ||
| return InteractiveCommandResult{ | ||
| Interactive: true, | ||
| Command: seg.command, | ||
| Reason: seg.reason, | ||
| Suggestion: seg.suggestion, | ||
| } |
There was a problem hiding this comment.
Path-prefixed git rebase -i now slips past the interactive guard.
The new anchored match compares commandBody(...) directly to git rebase -i. For /usr/bin/git rebase -i or "git" rebase -i, commandBody returns the prefixed/quoted token, so none of the interactiveSegments match and there is no single-program fallback for git. That lets an interactive rebase through and can hang the run. Normalize the first real command token before segment matching.
Suggested fix
- // First real command token: the body starts here.
- return strings.Join(fields[index:], " ")
+ // First real command token: normalize it the same way firstProgram does
+ // so /usr/bin/git rebase -i and "git" rebase -i still match.
+ body := append([]string{normalizeProgramToken(field)}, fields[index+1:]...)
+ return strings.Join(body, " ")Also applies to: 237-263
🤖 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.go` around lines 131 - 145, The segment
matching can miss commands with path or quoting (e.g., "/usr/bin/git" or
"\"git\"") because commandBody(...) returns the raw first token; update the loop
that iterates splitShellSegments(normalized) to normalize the first real command
token before comparing to interactiveSegments: extract the first token from
commandBody(strings.Fields(segment)), strip surrounding quotes and
environment/path prefixes (use something like basename semantics to remove
directories), lower-case it, then rebuild the normalized segment body to check
equality or prefix against seg.match; apply the same normalization logic where
interactiveSegments are checked (also the similar block around lines 237-263) so
InteractiveCommandResult detection covers path-prefixed or quoted executables
like git.
Comment on lines
+307
to
+329
| // normalizeProgramToken reduces a raw command token to a bare, lowercased program | ||
| // name: it strips shell quoting/escaping characters (", ', `, \) wherever they | ||
| // appear in the token (including embedded ones like `vi\m` or `v"i"m`), strips | ||
| // leading command-substitution markers, removes any directory prefix (so | ||
| // /usr/bin/vim and C:\tools\vim.exe match "vim"), and lowercases. This closes | ||
| // path/quote/substitution evasions of the detector. | ||
| func normalizeProgramToken(field string) string { | ||
| token := strings.TrimSpace(field) | ||
| token = strings.TrimLeft(token, "$(") | ||
| token = strings.TrimRight(token, ")") | ||
| // Strip shell quoting/escaping characters (", ', `, \) wherever they appear | ||
| // in the token — surrounding, embedded, or as a mid-word escape — so | ||
| // "vim", v"i"m, 'v'im, and vi\m all collapse to the program name. This is | ||
| // done BEFORE the directory-prefix trim so an escape can't masquerade as a | ||
| // path separator (e.g. vi\m must become vim, not m). | ||
| token = stripChars(token, "\"'`\\") | ||
| // Strip a directory prefix so /usr/bin/vim reduces to the basename. (A | ||
| // Windows-style backslash path separator is already removed above, so only | ||
| // the POSIX separator remains to split on.) | ||
| if i := strings.LastIndex(token, "/"); i >= 0 { | ||
| token = token[i+1:] | ||
| } | ||
| return strings.ToLower(token) |
There was a problem hiding this comment.
programIndex no longer agrees with firstProgram.
normalizeProgramToken now strips quotes and directory prefixes, so firstProgram can return python for /usr/bin/python or "python". hasNonInteractiveFlag and shellDashCPayload still locate the raw field through programIndex, which means /usr/bin/python -c 'print(1)' is treated as interactive because -c is never seen. Compare normalized tokens inside programIndex so the downstream flag/payload checks use the same executable resolution rule.
Suggested fix
func programIndex(program string, fields []string) int {
for index, field := range fields {
- if strings.ToLower(strings.TrimPrefix(field, "\\")) == program {
+ if normalizeProgramToken(field) == program {
return index
}
}
return -1
}Also applies to: 413-419
🤖 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.go` around lines 307 - 329, The program index
lookup must use the same normalization rule as firstProgram: change programIndex
to normalize each candidate token with normalizeProgramToken before comparing
against the target executable so callers like hasNonInteractiveFlag and
shellDashCPayload see the same resolved program (this ensures tokens like
/usr/bin/python or "python" match); update the second occurrence of this
comparison (the similar logic referenced in the later block around the other
programIndex use) the same way so both lookup sites normalize tokens prior to
equality checks.
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/tools/registry.go (1)
86-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEarly error returns bypass secret scrubbing.
RunWithOptionsredacts only post-execution returns. Sandbox deny/prompt and permission-denied branches return unsanitized output, so secret-shaped substrings in decision text can leak through.Patch sketch
if !ok { - return errorResult(`Error: Unknown tool "` + name + `".`) + return scrubResultSecrets(errorResult(`Error: Unknown tool "` + name + `".`)) } @@ if d.Action == sandbox.ActionDeny { res := errorResult(d.ErrorString()) res.SandboxDecision = sandboxDecision - return res + return scrubResultSecrets(res) } if d.Action == sandbox.ActionPrompt && !options.PermissionGranted { res := errorResult("Error: Sandbox approval required for " + name + ": " + d.Reason) res.SandboxDecision = sandboxDecision - return res + return scrubResultSecrets(res) } @@ if !options.PermissionGranted && !sandboxGrantAuthorized { res := errorResult("Error: Permission required for " + name + ": " + tool.Safety().Reason + ` The tool is marked "prompt" and was not executed.`) res.SandboxDecision = sandboxDecision - return res + return scrubResultSecrets(res) } default: res := errorResult("Error: Permission denied for " + name + ": " + tool.Safety().Reason) res.SandboxDecision = sandboxDecision - return res + return scrubResultSecrets(res) }🤖 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/registry.go` around lines 86 - 131, Early returns in Registry.RunWithOptions (sandbox deny/prompt and permission-denied branches) return unsanitized text from sandbox.Decision (d.ErrorString(), d.Reason) and tool.Safety().Reason; ensure those messages are redacted before building the Result. Update the branches that call errorResult to first pass any user-visible strings through the Registry's secret-scrubbing helper (e.g., registry.ScrubSecrets or registry.RedactSecrets) and use the redacted values when constructing res and setting res.SandboxDecision so no secret-shaped substrings leak in early returns.internal/cli/sessions.go (1)
197-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
rewindis unreachable because it is not recognized as a valid sessions subcommand.Line 199 omits
"rewind"fromisSessionsCommand, sozero sessions rewind <id>fails during argument parsing before the new dispatch case can run.💡 Suggested fix
func isSessionsCommand(command string) bool { switch command { - case "list", "children", "lineage", "tree", "rewind-plan", "compact-plan": + case "list", "children", "lineage", "tree", "rewind-plan", "rewind", "compact-plan": return true default: return false } }Also applies to: 64-68
🤖 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/sessions.go` around lines 197 - 200, The sessions subcommand checker is missing "rewind" so the CLI rejects "zero sessions rewind <id>"; update the isSessionsCommand switch (the function named isSessionsCommand shown around the sessions dispatch) to include "rewind" in the case list alongside "list", "children", "lineage", "tree", "rewind-plan", "compact-plan", and also apply the same addition to the other isSessionsCommand occurrence earlier in the file (the similar switch around lines 64-68) so both argument parsers recognize the "rewind" subcommand.
♻️ Duplicate comments (2)
internal/sessions/checkpoint_test.go (1)
59-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStop discarding errors in test setup/read paths.
Line 59, Line 114, Line 122, Line 145, Line 164, Line 177, and Line 217 still ignore returned errors. These paths are part of checkpoint/rewind assertions; swallowing errors makes failures harder to diagnose and can mask root cause.
Suggested tightening
- entries, _ := os.ReadDir(store.blobsDir("s")) + entries, err := os.ReadDir(store.blobsDir("s")) + if err != nil { + t.Fatal(err) + } - events, _ := store.ReadEvents("s") + events, err := store.ReadEvents("s") + if err != nil { + t.Fatal(err) + } - target, _ := store.AppendEvent("s", AppendEventInput{Type: EventMessage, Payload: map[string]any{}}) + target := mustAppend(t, store, AppendEventInput{Type: EventMessage, Payload: map[string]any{}})Also applies to: 114-115, 122-123, 145-146, 164-165, 177-178, 217-218
🤖 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/checkpoint_test.go` around lines 59 - 60, The test currently discards errors from os.ReadDir calls (e.g., entries, _ := os.ReadDir(store.blobsDir("s"))) and similar reads across checkpoint_test.go; change each to capture the error (entries, err := os.ReadDir(...)) and fail the test on error (use t.Fatalf or require.NoError(t, err)) so setup/read failures are surfaced immediately; update all occurrences referencing os.ReadDir and store.blobsDir in checkpoint_test.go (lines noted in the review) to check err and report a clear message including the directory path.internal/sandbox/safe_command.go (1)
237-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize executable tokens consistently in segment matching and index lookup.
commandBody(Line 262) returns the raw first token, andprogramIndex(Line 415) compares raw tokens. This lets quoted/path-prefixed forms bypass segment matches (/usr/bin/git rebase -i) and breaks non-interactive flag detection for normalized programs (/usr/bin/python -c ...).Suggested fix
func commandBody(fields []string) string { for index := 0; index < len(fields); index++ { field := fields[index] ... if wrapperPrograms[normalizeProgramToken(field)] { continue } - return strings.Join(fields[index:], " ") + body := append([]string{normalizeProgramToken(field)}, fields[index+1:]...) + return strings.Join(body, " ") } return "" } func programIndex(program string, fields []string) int { for index, field := range fields { - if strings.ToLower(strings.TrimPrefix(field, "\\")) == program { + if normalizeProgramToken(field) == program { return index } } return -1 }Also applies to: 413-419
🤖 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.go` around lines 237 - 263, The commandBody function returns the first raw command token which can differ from programIndex's lookup and from normalized forms (e.g. quoted paths or /usr/bin/git), so change commandBody to normalize the first real command token with normalizeProgramToken before joining/returning the body and ensure programIndex uses the same normalization when comparing tokens; specifically, locate commandBody and modify its return to normalize fields[index] (and any wrapped tokens) via normalizeProgramToken, and update programIndex to compare normalizeProgramToken(tokens[i]) against wrapperPrograms/lookup so both matching and flag detection behave consistently.
🟡 Minor comments (15)
internal/cli/command_center.go-287-299 (1)
287-299:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle tabwriter write/flush errors explicitly.
Line 287, Line 289, and Line 299 ignore write errors. This can silently return truncated output and triggers errcheck findings in changed code.
Suggested fix
writer := tabwriter.NewWriter(&builder, 0, 0, 2, ' ', 0) - fmt.Fprintln(writer, "ID\tPROVIDER\tSTATUS\tCONTEXT\tREASONING\t$IN/1M\t$OUT/1M") + if _, err := fmt.Fprintln(writer, "ID\tPROVIDER\tSTATUS\tCONTEXT\tREASONING\t$IN/1M\t$OUT/1M"); err != nil { + return "Models\n (render error)" + } for _, model := range models { - fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + if _, err := fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", model.ID, model.Provider, model.Status, formatTokenCount(model.ContextWindow), formatReasoningEfforts(model.ReasoningEfforts), formatRate(model.InputPerMillion), formatRate(model.OutputPerMillion), - ) + ); err != nil { + return "Models\n (render error)" + } } - writer.Flush() + if err := writer.Flush(); err != nil { + return "Models\n (render error)" + }🤖 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/command_center.go` around lines 287 - 299, The printing block currently ignores errors from writer writes and flushes (calls to fmt.Fprintln, fmt.Fprintf, and writer.Flush); modify the code around the loop that prints model rows (the writer variable and the loop iterating over models) to capture each error return and handle it appropriately (e.g., return the error to the caller or log and abort), checking the result of fmt.Fprintln(writer, ...), each fmt.Fprintf(writer, ...) call, and writer.Flush() after the loop so write/flush failures are not silently ignored; ensure you propagate or report the error from the enclosing function that calls this printing code.internal/cli/skills_test.go-102-104 (1)
102-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten the empty-directory assertion to avoid false positives.
The current check passes on any output containing
"no"(for example unrelated error text), so this test can succeed for the wrong reason.Suggested assertion hardening
- if !strings.Contains(strings.ToLower(stdout.String()), "no") { + out := strings.ToLower(stdout.String()) + if !(strings.Contains(out, "no") && strings.Contains(out, "skill")) { t.Fatalf("expected a no-skills message, got:\n%s", 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/skills_test.go` around lines 102 - 104, The test currently uses strings.Contains(strings.ToLower(stdout.String()), "no") which can match unrelated text; change the assertion in the test (in internal/cli/skills_test.go) to validate the precise expected output (e.g., trim stdout, then compare equality to the exact "no skills" message or use a case-insensitive regex anchored to the full message) instead of a substring match so the test only passes when the CLI actually prints the intended empty-directory message.internal/tools/task.go-89-93 (1)
89-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve prompt type errors instead of collapsing everything to “required”.
On Line 89-Line 93, any parse failure becomes
prompt is required, including wrong-type input. That hides actionable validation errors and weakens type-strict behavior.Suggested fix
- prompt, err := aliasedStringArg(args, []string{"prompt", "instructions", "task", "input"}, "", true, false) + prompt, err := aliasedStringArg(args, []string{"prompt", "instructions", "task", "input"}, "", false, true) if err != nil { - // Normalize the error to the canonical "prompt" key regardless of alias. - return TaskRequest{}, fmt.Errorf("prompt is required") + return TaskRequest{}, err } if strings.TrimSpace(prompt) == "" { return TaskRequest{}, fmt.Errorf("prompt is required") }🤖 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/task.go` around lines 89 - 93, The current handler calls aliasedStringArg and always returns fmt.Errorf("prompt is required") on any error, which hides type/parse errors; update the error handling after aliasedStringArg in the TaskRequest creation so that you only normalize to "prompt is required" when the underlying error indicates a missing value (e.g., a specific missing-key sentinel or empty value), otherwise propagate the original error from aliasedStringArg; modify the block around aliasedStringArg (and the TaskRequest{} return) to return the original err for non-missing errors and only return the normalized message for true missing prompts.internal/cli/exec.go-508-516 (1)
508-516:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrimmed model input is dropped on unresolved lookups.
Line 515 returns
inputinstead oftrimmed. That can pass whitespace-padded model IDs downstream when the registry doesn’t resolve a custom name.Proposed fix
func resolveSelectedModel(registry modelregistry.Registry, input string) (string, string) { trimmed := strings.TrimSpace(input) if trimmed == "" { - return input, "" + return "", "" } entry, notice, ok := registry.ResolveWithFallback(trimmed) if !ok { - return input, "" + return trimmed, "" } return entry.ID, notice }🤖 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.go` around lines 508 - 516, In resolveSelectedModel, when the registry lookup fails you currently return the original input which can preserve trailing/leading whitespace; change the failure return to use the trimmed value instead. Specifically, in the function resolveSelectedModel (and the branch handling registry.ResolveWithFallback returning !ok), return trimmed (not input) so downstream callers receive the sanitized model ID string.internal/cli/exec_test.go-245-268 (1)
245-268:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test does not actually verify the mode-to-shared-resolution path it claims.
The assertion uses a hard-coded deprecated model (
"gpt-4-turbo") instead of the mode-derived model flow, so the test can pass even if mode routing regresses. Either assert against the mode-seeded value through the same path, or rename/scope this test to only deprecation-resolution behavior.🤖 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_test.go` around lines 245 - 268, The test currently bypasses the mode-to-shared-resolution path by calling resolveSelectedModel with a hard-coded "gpt-4-turbo"; change it to use the model value produced by applyExecMode so the mode flow is actually exercised: after calling applyExecMode(&options) read the mode-seeded model (e.g., options.Model or options.model as set by applyExecMode) and pass that into resolveSelectedModel(registry, <mode-seeded-model>) and assert the returned resolved ID and notice (still expecting the deprecation redirect and notice). If the field name differs, locate it on execOptions and use that instead; alternatively, if you intend to only test deprecation resolution, rename the test to remove the mode claim.internal/tui/autocomplete_test.go-91-97 (1)
91-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the wrap-around assertion to catch negative indices.
The current modulo check can pass for out-of-range negative indices, so this test can miss a real selection bug.
Suggested test fix
- if m.suggestionIdx != m.suggestionIdx%len(m.suggestions) { - t.Fatal("selection index out of range after cycling") - } + if m.suggestionIdx < 0 || m.suggestionIdx >= len(m.suggestions) { + t.Fatalf("selection index out of range after cycling: %d", m.suggestionIdx) + }🤖 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/tui/autocomplete_test.go` around lines 91 - 97, The assertion allows negative indices to slip through because modulo comparison can match for negatives; change the test that cycles suggestions (using model, m.suggestions and Update with tea.KeyTab) to assert m.suggestionIdx is within the valid range by checking 0 <= m.suggestionIdx && m.suggestionIdx < len(m.suggestions) after the loop, ensuring wrap-around yields a non-negative index less than len(m.suggestions).internal/mcp/hang_test.go-88-88 (1)
88-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle the
Closeerror in test cleanup.Line 88 and Line 136 ignore
reader.Close(), anderrcheckis already flagging both sites. If lint is part of the merge gates, this file stays red until cleanup reports the close failure explicitly.Proposed fix
- reader := newBlockingReader() - defer reader.Close() + reader := newBlockingReader() + t.Cleanup(func() { + if err := reader.Close(); err != nil { + t.Errorf("close blockingReader: %v", err) + } + })- reader := newBlockingReader() - defer reader.Close() + reader := newBlockingReader() + t.Cleanup(func() { + if err := reader.Close(); err != nil { + t.Errorf("close blockingReader: %v", err) + } + })Also applies to: 136-136
🤖 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/hang_test.go` at line 88, The deferred calls to reader.Close() in the test ignore returned errors (variable name reader) and cause errcheck failures; change each defer reader.Close() to a deferred closure that captures and checks the error—e.g., defer func() { if err := reader.Close(); err != nil { t.Fatalf("reader.Close failed: %v", err) } }—so the test reports close failures explicitly (replace t.Fatalf with t.Errorf if non-fatal is preferred).internal/zerocommands/contracts.go-214-223 (1)
214-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRepresentative tier selection assumes ordering that isn’t validated.
Line 220 returns
cost.Tiers[0], but tier ordering is not enforced in validation. If tiers are unsorted, the exported “representative” rate can be wrong.💡 Proposed fix
func representativeRates(cost modelregistry.ModelCost) (float64, float64) { if cost.InputPerMillion > 0 || cost.OutputPerMillion > 0 { return cost.InputPerMillion, cost.OutputPerMillion } if len(cost.Tiers) > 0 { - return cost.Tiers[0].InputPerMillion, cost.Tiers[0].OutputPerMillion + best := cost.Tiers[0] + for _, tier := range cost.Tiers[1:] { + // Prefer the smallest positive ceiling tier; fallback tier (0) should not + // override a concrete bounded tier for representative pricing. + if best.UpToInputTokens == 0 || (tier.UpToInputTokens > 0 && tier.UpToInputTokens < best.UpToInputTokens) { + best = tier + } + } + return best.InputPerMillion, best.OutputPerMillion } return 0, 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/zerocommands/contracts.go` around lines 214 - 223, The function representativeRates assumes cost.Tiers is pre-sorted and returns cost.Tiers[0], which can be incorrect if tiers are unsorted; update representativeRates to scan cost.Tiers and pick the tier that represents the true base/lowest-threshold tier (e.g., the tier with the smallest minimum token/usage threshold) and return its InputPerMillion and OutputPerMillion; if your tier struct uses a field like MinTokens or Min, use that field to determine the smallest threshold, otherwise document and choose the conservative (lowest) rates by comparing InputPerMillion/OutputPerMillion across tiers.internal/providers/anthropic/finish_reason_test.go-46-52 (1)
46-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert done-event presence in the normal-stop test.
Line 47 currently only rejects non-empty finish reasons; if the done event disappears entirely, this test still passes and misses a regression.
✅ Proposed test hardening
events := collectProviderEvents(t, provider) + var sawDone bool for _, e := range events { - if e.Type == zeroruntime.StreamEventDone && e.FinishReason != "" { - t.Fatalf("normal stop leaked FinishReason %q", e.FinishReason) + if e.Type == zeroruntime.StreamEventDone { + sawDone = true + if e.FinishReason != "" { + t.Fatalf("normal stop leaked FinishReason %q", e.FinishReason) + } } } + if !sawDone { + t.Fatalf("no done event; events: %+v", events) + }🤖 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/anthropic/finish_reason_test.go` around lines 46 - 52, The test currently only checks that any StreamEventDone doesn't have a non-empty FinishReason but doesn't fail if no done event exists; modify the loop that iterates over events returned by collectProviderEvents(t, provider) to first assert that at least one event with Type == zeroruntime.StreamEventDone exists (fail the test if none found), and then for that done event(s) assert FinishReason == "" (fail with the existing message if non-empty) so the test catches both missing done events and leaked finish reasons.internal/zenline/markdown_test.go-183-186 (1)
183-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the streaming-plain assertion to avoid false positives.
Line 184 only checks
"partial"; markdown processing could still happen and this test would pass.✅ Proposed assertion update
if !strings.Contains(out, "partial") { t.Errorf("streaming text missing: %q", out) } + if !strings.Contains(out, "**incomplete") { + t.Errorf("streaming text should stay verbatim (markdown markers stripped unexpectedly): %q", out) + }🤖 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/zenline/markdown_test.go` around lines 183 - 186, The current assertion only checks for the substring "partial" which can pass even if markdown was rendered; update the test around the variable out to assert the literal markdown source is present (e.g., check for the exact token with its markdown markers such as "**partial**" or "`partial`" depending on the test input) and also assert that rendered output variants are absent (e.g., ensure out does not contain "<em>partial</em>", "<strong>partial</strong>" or other HTML/emphasis forms); modify the assertion near the existing if !strings.Contains(out, "partial") to check both presence of the raw markdown token and absence of rendered forms so streaming text is verified as verbatim.internal/streamjson/streamjson_test.go-188-188 (1)
188-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStop ignoring
json.Marshalerrors in these tests.Line 188, Line 202, and Line 210 currently discard marshal errors, which can mask serialization regressions and yield false-positive test passes.
Suggested fix
- bare, _ := json.Marshal(Event{SchemaVersion: 1, Type: EventText}) + bare, err := json.Marshal(Event{SchemaVersion: 1, Type: EventText}) + if err != nil { + t.Fatal(err) + } @@ - data, _ := json.Marshal(ev) + data, err := json.Marshal(ev) + if err != nil { + t.Fatal(err) + } @@ - bare, _ := json.Marshal(Event{SchemaVersion: 1, Type: EventText}) + bare, err := json.Marshal(Event{SchemaVersion: 1, Type: EventText}) + if err != nil { + t.Fatal(err) + }Also applies to: 202-202, 210-210
🤖 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/streamjson/streamjson_test.go` at line 188, Don't ignore json.Marshal errors in the tests: replace the bare, _ := json.Marshal(...) calls (e.g., the json.Marshal(Event{SchemaVersion: 1, Type: EventText}) invocation that assigns to bare) with a two-value assignment (bare, err := json.Marshal(...)) and immediately fail the test on error (e.g., if err != nil { t.Fatalf("json.Marshal failed: %v", err) } or use require.NoError). Apply the same change for the other json.Marshal calls noted in this file so marshal errors can't be silently ignored.internal/providers/gemini/finish_reason_test.go-41-45 (1)
41-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire
StreamEventDonein SAFETY/STOP finish-reason tests.Line 41 and Line 55 checks are conditional; both tests can pass even if no done event is emitted. Make done-event presence explicit.
Suggested fix
func TestStreamCompletionSurfacesSafetyFinishReason(t *testing.T) { @@ events := collectProviderEvents(t, provider) + var sawDone bool for _, e := range events { - if e.Type == zeroruntime.StreamEventDone && e.FinishReason != zeroruntime.FinishReasonContentFilter { - t.Fatalf("done FinishReason = %q, want %q", e.FinishReason, zeroruntime.FinishReasonContentFilter) + if e.Type == zeroruntime.StreamEventDone { + sawDone = true + if e.FinishReason != zeroruntime.FinishReasonContentFilter { + t.Fatalf("done FinishReason = %q, want %q", e.FinishReason, zeroruntime.FinishReasonContentFilter) + } } } + if !sawDone { + t.Fatalf("no done event; events: %+v", events) + } } @@ func TestStreamCompletionNormalFinishReasonHasNoReason(t *testing.T) { @@ events := collectProviderEvents(t, provider) + var sawDone bool for _, e := range events { - if e.Type == zeroruntime.StreamEventDone && e.FinishReason != "" { - t.Fatalf("normal finish leaked FinishReason %q", e.FinishReason) + if e.Type == zeroruntime.StreamEventDone { + sawDone = true + if e.FinishReason != "" { + t.Fatalf("normal finish leaked FinishReason %q", e.FinishReason) + } } } + if !sawDone { + t.Fatalf("no done event; events: %+v", events) + } }Also applies to: 55-59
🤖 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/gemini/finish_reason_test.go` around lines 41 - 45, The tests currently loop over events but never assert that a StreamEventDone was emitted, so they can pass incorrectly; update both checks to first assert that at least one event has Type == zeroruntime.StreamEventDone (e.g., find or count such events in the events slice), then validate that the Done event's FinishReason equals the expected constant (use zeroruntime.FinishReasonContentFilter for the safety test and zeroruntime.FinishReasonStopSequence for the stop test) using the existing t.Fatalf assertions; reference the events variable and zeroruntime.StreamEventDone / FinishReason constants when locating where to add the presence assertion.internal/zenline/markdown.go-67-71 (1)
67-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim only trailing newlines here.
Line 71 uses
strings.Trim(out, "\n"), which strips leading newlines too. The comment says we only want to drop glamour’s trailing padding, so this changes content more than intended.Proposed fix
- out = strings.Trim(out, "\n") + out = strings.TrimRight(out, "\n")🤖 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/zenline/markdown.go` around lines 67 - 71, The current code in markdown.go calls r.Render(text) and then uses strings.Trim(out, "\n"), which removes both leading and trailing newlines; change this to only remove trailing newlines so leading content is preserved—use a trailing-only trim (e.g., strings.TrimRight or equivalent) on the variable out after Render in the same block that handles the r.Render(text) result to strip only glamour’s trailing padding.internal/agent/compaction.go-125-136 (1)
125-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject whitespace-only summaries before dropping history.
Line 129 trims the summarizer output, but
Compactnever revalidates it. A caller that returns only whitespace will still replace the elided middle withsummaryLabel, silently losing history without a usable summary.Proposed fix
summary, err := opts.Summarize(middle) if err != nil { return nil, err } summary = strings.TrimSpace(summary) + if summary == "" { + return nil, errors.New("compaction summary must not be empty") + } compacted := make([]zeroruntime.Message, 0, systemEnd+1+(len(messages)-boundary))🤖 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 125 - 136, The Compact logic trims the summarizer output but doesn't revalidate it, so a whitespace-only summary can replace history; update the Compact implementation (the call site around opts.Summarize(middle) and the code that appends summaryLabel+summary) to check the trimmed summary string for emptiness and reject it (return an error or skip compaction) instead of appending an empty summary; ensure you reference the variables summary, opts.Summarize, summaryLabel and the Compact flow when adding the guard so history isn't silently lost.internal/cli/exec_writer.go-106-110 (1)
106-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle malformed checkpoint payloads instead of emitting an empty checkpoint.
Line 109 ignores
json.Unmarshalfailures and then still emits a checkpoint event with zero-valuetool/filesfields. That turns a corrupted or incompatible session record into apparently valid output for JSON consumers.Proposed fix
func (writer *execEventWriter) checkpoint(event sessions.Event) { var payload sessions.CheckpointPayload if len(event.Payload) > 0 { - _ = json.Unmarshal(event.Payload, &payload) + if err := json.Unmarshal(event.Payload, &payload); err != nil { + writer.warning("checkpoint payload decode failed: " + err.Error()) + 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/cli/exec_writer.go` around lines 106 - 110, The checkpoint method (execEventWriter.checkpoint) currently ignores json.Unmarshal errors and proceeds to emit a checkpoint with a zero-value sessions.CheckpointPayload; change it to capture the error from json.Unmarshal(event.Payload, &payload), and if unmarshal fails, do not emit the checkpoint—log or record the error via the writer (e.g., writer.logger or an existing error-reporting helper) and return early; ensure valid checkpoints are only emitted when the payload successfully unmarshals into sessions.CheckpointPayload.
🧹 Nitpick comments (15)
internal/providers/anthropic/dropped_test.go (1)
61-65: ⚡ Quick winAssert the valid tool-use path actually starts a tool call.
The test currently only checks “not dropped.” Add a
StreamEventToolCallStartassertion so a no-op stream doesn’t pass.Suggested enhancement
- for _, e := range events { - if e.Type == zeroruntime.StreamEventToolCallDropped { - t.Errorf("valid tool_use block must not be dropped; events: %+v", events) - } - } + var started int + for _, e := range events { + if e.Type == zeroruntime.StreamEventToolCallDropped { + t.Errorf("valid tool_use block must not be dropped; events: %+v", events) + } + if e.Type == zeroruntime.StreamEventToolCallStart { + started++ + } + } + if started != 1 { + t.Errorf("expected exactly one tool-call-start for valid tool_use, got %d; events: %+v", started, events) + }🤖 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/anthropic/dropped_test.go` around lines 61 - 65, The test currently only ensures no StreamEventToolCallDropped events occur but doesn't assert a tool call actually started; update the loop over events (the slice named events) to also check for a StreamEventToolCallStart event by scanning events for an entry with Type == zeroruntime.StreamEventToolCallStart and fail the test (t.Errorf or t.Fatalf) if none is found, while preserving the existing dropped-event check for Type == zeroruntime.StreamEventToolCallDropped; reference the event type symbol StreamEventToolCallStart in your assertion so a no-op stream cannot pass.internal/tools/task_test.go (1)
46-53: ⚡ Quick winAdd a non-string
promptcase to lock parser type-strictness.Current coverage checks missing/blank prompt, but not
promptpresent with wrong type (for example{"prompt": 123}), which is a key tolerance-path contract.Suggested test addition
func TestParseTaskRequestRequiresPrompt(t *testing.T) { if _, err := ParseTaskRequest(map[string]any{"description": "no prompt here"}); err == nil { t.Fatalf("expected error when prompt is missing") } if _, err := ParseTaskRequest(map[string]any{"prompt": " "}); err == nil { t.Fatalf("expected error when prompt is blank") } + if _, err := ParseTaskRequest(map[string]any{"prompt": 123}); err == nil { + t.Fatalf("expected error when prompt is non-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/tools/task_test.go` around lines 46 - 53, Add a test branch to TestParseTaskRequestRequiresPrompt that verifies ParseTaskRequest rejects a non-string prompt value: call ParseTaskRequest with a map containing "prompt": 123 (or another non-string) and assert that err is non-nil; update the test function TestParseTaskRequestRequiresPrompt to include this case so the parser enforces type-strictness for the prompt field.internal/agent/task_test.go (1)
271-303: ⚡ Quick winAdd child-run permission callback isolation coverage.
This verifies
OnAskUseris not inherited, but there’s no companion check forOnPermissionRequestwhen a child sees a mutating tool in ask mode. Add a regression to confirm no permission callback leakage/elevation across the task boundary.Based on learnings: Verify that the review calls out the risk surface and whether it introduces a blocker if the PR touches auth, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts.
🤖 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/task_test.go` around lines 271 - 303, Add a companion regression to TestSubAgentRunsWithoutInteractiveCallbacks that asserts a parent's OnPermissionRequest is not invoked by a child run: create a similar test or extend TestSubAgentRunsWithoutInteractiveCallbacks to register a mutating tool (e.g., tools.NewTaskTool or tools.NewReadFileTool in ask mode), use the existing routingProvider and Run(...) call, and supply an Options.OnPermissionRequest callback that increments a counter; after Run returns assert the counter is zero to ensure permission callbacks are not leaked to the child agent. Ensure you reference the same Run, routingProvider, and Options.OnPermissionRequest symbols so the test targets the same task boundary behavior.internal/tools/write_tools_test.go (1)
403-405: ⚡ Quick winNarrow skip conditions so regressions fail instead of silently skipping.
These branches currently
Skipfon any non-OK result, which can mask real behavior breaks. Skip only for known environment constraints (e.g., git unavailable), andFatalffor all other failures.Proposed test-hardening diff
- if res.Status != StatusOK { - t.Skipf("git apply unavailable or failed: %s", res.Output) - } + if res.Status != StatusOK { + lower := strings.ToLower(res.Output) + if strings.Contains(lower, "git apply unavailable") || + strings.Contains(lower, "executable file not found") { + t.Skipf("git apply unavailable: %s", res.Output) + } + t.Fatalf("expected patch ok, got %s: %s", res.Status, res.Output) + }Also applies to: 452-454
🤖 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/write_tools_test.go` around lines 403 - 405, The test currently calls t.Skipf for any non-StatusOK result (res.Status != StatusOK), which can hide regressions; change the logic in the test where res and StatusOK are checked (the branch with t.Skipf) to only call t.Skipf when the failure clearly indicates an environmental constraint (e.g., the git binary is missing or "git apply" is unavailable as determined from res.Output or the error string), and call t.Fatalf (or t.Fatalf with res.Output) for all other non-expected failures so test failures surface real regressions; apply the same change to the other occurrence around the block at lines 452-454 that also uses t.Skipf.internal/cli/exec.go (1)
229-239: Consider emitting the unsafe-mode source in structured run metadata.You already warn in text; adding the source (
--auto highvs--skip-permissions-unsafe) to stream/json metadata would make audit trails and incident triage stronger for headless executions.Based on learnings: Verify that the review calls out the risk surface and whether it introduces a blocker if the PR touches permissions or provider routing.
🤖 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.go` around lines 229 - 239, When permissionMode == agent.PermissionModeUnsafe the code only emits a text warning; also populate structured run metadata with the unsafe-mode source (distinguish "--auto high" vs "--skip-permissions-unsafe") so headless/JSON streams include this info for auditing. Update the block that computes reason (using permissionMode and options.skipPermissionsUnsafe) to also emit a metadata field like unsafe_permissions_source (or augment the existing writer stream/json metadata output) alongside the writer.warning call and before checking writer.err; use the same writer/stream mechanism used elsewhere for structured run metadata so the value is serialized in the run's JSON/stream output.internal/tui/view.go (1)
75-87: ⚡ Quick winAlign the permission-toggle comment with the implemented behavior.
The code currently folds
UnsafetoAuto, but the comment says it folds toAsk. This is a policy-sensitive path; keep docs exact.Comment-only fix
-// Unsafe is folded back to Ask so the toggle always lands somewhere safe. +// Unsafe is folded back to Auto so the toggle always lands somewhere safe.Based on learnings: verify that permission-policy/trust-model changes are explicitly called out and not hidden by unrelated edits.
🤖 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/tui/view.go` around lines 75 - 87, The comment above nextPermissionMode is inaccurate: it states Unsafe is folded back to Ask, but the implemented logic in nextPermissionMode returns Auto for any non-Auto value (so Unsafe → Auto). Update the comment to precisely describe the implemented behavior (that Unsafe is folded back to Auto, not Ask), referencing the nextPermissionMode function and agent.PermissionMode (including Unsafe/Auto/Ask) so the policy-sensitive behavior is documented exactly.internal/providers/gemini/provider.go (1)
136-145: Add counters for idle timeouts and dropped tool-calls.This path now defines key provider-failure signals; exposing both as metrics would make upstream regressions and model formatting drift visible quickly in production.
Based on learnings: verify that reviews call out risk surface and blocker status for provider routing/outbound network/background execution changes.
Also applies to: 208-229
🤖 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/gemini/provider.go` around lines 136 - 145, The code path handling SSE stream idle errors (providerio.ScanSSEDataWithContext -> errors.Is(err, providerio.ErrStreamIdle)) currently emits a StreamEvent but does not increment any metrics; add and increment a dedicated counter metric (e.g., providerIdleTimeoutCounter) when ErrStreamIdle is detected and similarly add a counter for dropped tool-calls where provider.emitPayload returns false (or where you detect a dropped tool invocation) so both idle timeouts and dropped tool-calls are recorded; update the same pattern in the other similar block (the providerio.ScanSSEDataWithContext/ErrStreamIdle handling later in the file) and ensure you use the existing symbols (provider.streamIdleTimeout, providerio.SendEvent, zeroruntime.StreamEvent, provider.redact) when adding logging + metric increments so the new counters are tied to this provider’s error path.internal/cli/zenline.go (1)
11-15: ⚡ Quick winAdd a contract test for zenline/default-shell permission-policy parity.
This comment states zenline reuses identical runtime wiring. A focused CLI test locking provider/sandbox/permission parity would prevent silent trust-model drift.
Based on learnings: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup.
🤖 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/zenline.go` around lines 11 - 15, Add a contract-style CLI test that asserts runZenline's runtime wiring matches the default `zero` shell (provider, tools/sandbox, permissions, sessions) to prevent silent trust-model drift: write a test that programmatically initializes the same startup path used by runZenline and the default shell, extract and compare their provider/sandbox/permission-policy/routing-default/telemetry-network configuration objects or fingerprints, and fail if any of product, trust-model, routing-default, telemetry/network, or permission-policy values differ; locate and exercise runZenline (and the default zero shell entry point) and compare the resulting permission-policy and sandbox/provider state rather than UI rendering.internal/zeroruntime/empty_toolcall_test.go (1)
22-34: ⚡ Quick winAlso assert dropped-call accounting in this nameless-call test.
Line 23–33 verifies leak prevention, but not
DroppedToolCalls. Adding that assertion makes the malformed-call contract explicit.Suggested assertion
if got.ToolCalls[0].Name != "read_file" { t.Errorf("kept call name = %q, want read_file", got.ToolCalls[0].Name) } + if got.DroppedToolCalls != 1 { + t.Fatalf("DroppedToolCalls = %d, want 1 for malformed call b", got.DroppedToolCalls) + } for _, c := range got.ToolCalls { if c.Name == "" { t.Error("a nameless tool call leaked to the agent") } }🤖 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/zeroruntime/empty_toolcall_test.go` around lines 22 - 34, The test currently checks that CollectStream returns one valid tool call (got.ToolCalls) and no nameless calls leaked, but doesn't assert dropped-call accounting; update the test that calls CollectStream(context.Background(), events) to also assert the dropped-malformed counter by checking got.DroppedToolCalls == 1 (or the expected dropped count) and fail the test with a clear message if it differs so the malformed-call contract is explicit; reference the CollectStream call and the got.DroppedToolCalls field when making this assertion.internal/streamjson/streamjson_test.go (1)
165-166: ⚡ Quick winAssert
Truncatedsurvives round-trip too.Line 165 sets
Truncated, but the test never verifies it after unmarshal. Add the check so this field is covered alongsideRedacted/Display/ChangedFiles.Suggested assertion
if back.Redacted == nil || !*back.Redacted { t.Error("redacted lost in round-trip") } + if back.Truncated == nil || *back.Truncated { + t.Errorf("truncated lost or changed: %+v", back.Truncated) + } if len(back.ChangedFiles) != 1 || back.ChangedFiles[0] != "f.go" { t.Errorf("changedFiles lost: %v", back.ChangedFiles) }Also applies to: 178-186
🤖 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/streamjson/streamjson_test.go` around lines 165 - 166, The test sets Truncated (a pointer bool) but never asserts it after JSON round-trip; update the test (the same place it asserts Redacted, Display, and ChangedFiles after unmarshal) to also assert that the unmarshaled Truncated matches the original value (nil vs non-nil and bool content). Locate the assertions that check Redacted/Display/ChangedFiles in the test and add an equivalent check for Truncated using the same comparison style (dereference when non-nil or check nil equality) so Truncated is covered by the round-trip verification.internal/sandbox/safe_command_test.go (1)
190-217: ⚡ Quick winAdd regression cases for normalized segment matching and option-value ssh forms.
Please add cases like
/usr/bin/git rebase -i HEAD~2,"/usr/bin/python" -c 'print(1)', andssh -i key hostto lock down the normalization and trailing-command heuristics that are easy to regress.🤖 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 190 - 217, Add regression test cases to TestDetectInteractiveBypasses to cover normalized-segment matching and option-value SSH forms: include "/usr/bin/git rebase -i HEAD~2" to ensure interactive flags in subcommands (git rebase -i) are detected, add "\"/usr/bin/python\" -c 'print(1)'" to ensure quoted interpreter paths with -c are treated non-interactive, and add "ssh -i key host" (and a variant like "ssh -i key user@host") to ensure ssh with a private-key option is recognized correctly by DetectInteractiveCommand; update the blocked or allowed slices as appropriate so these specific strings are asserted against DetectInteractiveCommand("linux") with expected Interactive boolean outcomes.internal/agent/loop.go (1)
458-496: Document the sub-agent trust boundary as an explicit policy decision before merge.This path now inherits sandbox/permission/autonomy into child runs while suppressing interactive callbacks. Please call out this risk surface and explicit maintainer alignment in PR/release notes (including blocker status) since it changes runtime trust behavior.
Based on learnings: verify that permission-policy/routing/background-execution changes have explicit risk-surface callouts and maintainer alignment.
🤖 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 458 - 496, Add an explicit policy note documenting the sub-agent trust boundary: update the PR/release notes and add a short in-code comment near childOptions / Options construction (referencing Depth, PermissionMode, Sandbox, Autonomy, Registry.Without("task","ask_user") and OnUsage) stating that permission mode, sandbox, and autonomy are intentionally inherited while interactive callbacks are suppressed, call out the resulting risk surface (background execution and permission routing) and mark this as an approved maintainer decision/blocker for merge so reviewers know this is an intentional behavior change needing alignment.internal/tools/registry.go (1)
158-193: Call out the trust-surface expansion in release/docs (non-blocker).Adding
ask_user/skillto read-only tools andtaskto core tools changes default capability/permission surface; this should stay explicitly documented for maintainer sign-off.Based on learnings: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup.
🤖 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/registry.go` around lines 158 - 193, The change expands the default trust surface by adding ask_user and skill to CoreReadOnlyTools and task to CoreTools; update the release notes and relevant documentation to explicitly call out this permission/capability change and request maintainer sign-off. Specifically, document the impact of NewAskUserTool, NewSkillTool (read-only set) and NewTaskTool (core tools) and ensure product, trust-model, routing-default, telemetry/network, and permission-policy docs and any routing or telemetry config are reviewed and amended so these changes aren’t hidden in unrelated cleanup; add a short checklist in the release/docs entry requiring reviewer/maintainer approval.internal/tools/skill_test.go (1)
52-87: ⚡ Quick winAdd a regression test for dual-key alias precedence (
nameempty,skillpopulated).Current tests cover
skillalias and missing-name, but not the mixed-input edge that previously regressed in arg tolerance paths.Suggested test addition
+func TestSkillToolNameEmptyButSkillAliasProvided(t *testing.T) { + dir := t.TempDir() + writeSkillFile(t, dir, "demo", "body of demo") + + tool := NewSkillTool(dir) + result := tool.Run(context.Background(), map[string]any{ + "name": "", + "skill": "demo", + }) + if result.Status != StatusOK { + t.Fatalf("Status = %s, want ok (output: %s)", result.Status, result.Output) + } +}🤖 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/skill_test.go` around lines 52 - 87, Add a regression test that verifies the SkillTool accepts the `skill` alias even when `name` is present but empty: create a temp dir, write a skill file (e.g., "demo"), instantiate the tool with NewSkillTool, call tool.Run with an args map containing {"name": "", "skill": "demo"}, and assert result.Status == StatusOK and result.Output contains the skill body; place this alongside TestSkillToolAcceptsSkillAlias to cover the dual-key alias precedence edge case.internal/modelregistry/modes_test.go (1)
41-49: ⚡ Quick winAssert deep-copy behavior for tool filter slices too.
This only proves
Nameis copied. A shallow-copy regression inEnabledTools/DisabledToolswould still pass here.Proposed test hardening
func TestModesReturnsIndependentCopies(t *testing.T) { + originalEnabled := defaultModes[0].EnabledTools + originalDisabled := defaultModes[0].DisabledTools + defaultModes[0].EnabledTools = []string{"read_file"} + defaultModes[0].DisabledTools = []string{"bash"} + t.Cleanup(func() { + defaultModes[0].EnabledTools = originalEnabled + defaultModes[0].DisabledTools = originalDisabled + }) + modes := Modes() if len(modes) == 0 { t.Fatal("Modes() returned no presets") } modes[0].Name = "mutated" - if again := Modes(); again[0].Name == "mutated" { + modes[0].EnabledTools[0] = "mutated-tool" + modes[0].DisabledTools[0] = "mutated-tool" + + if again := Modes(); again[0].Name == "mutated" { t.Fatal("Modes() should return defensive copies, not shared state") } + if again := Modes(); again[0].EnabledTools[0] == "mutated-tool" || again[0].DisabledTools[0] == "mutated-tool" { + t.Fatal("Modes() should deep-copy tool filter slices") + } }🤖 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/modelregistry/modes_test.go` around lines 41 - 49, Test currently only mutates Mode.Name; extend the test for Modes() to also verify that slices are deep-copied by mutating the returned preset's EnabledTools and DisabledTools (e.g., change/append an element on modes[0].EnabledTools and modes[0].DisabledTools) and then calling Modes() again to assert the corresponding slices on the fresh copy are unchanged; update TestModesReturnsIndependentCopies to include these slice mutations and assertions against again[0].EnabledTools and again[0].DisabledTools to prevent shallow-copy regressions.
Comment on lines
+337
to
+357
| func renderTranscript(messages []zeroruntime.Message) string { | ||
| lines := make([]string, 0, len(messages)) | ||
| for _, message := range messages { | ||
| switch message.Role { | ||
| case zeroruntime.MessageRoleAssistant: | ||
| line := "assistant: " + message.Content | ||
| if len(message.ToolCalls) > 0 { | ||
| calls := make([]string, 0, len(message.ToolCalls)) | ||
| for _, call := range message.ToolCalls { | ||
| calls = append(calls, call.Name+"("+call.Arguments+")") | ||
| } | ||
| line += "\n[tool calls: " + strings.Join(calls, "; ") + "]" | ||
| } | ||
| lines = append(lines, line) | ||
| case zeroruntime.MessageRoleTool: | ||
| lines = append(lines, "tool result: "+message.Content) | ||
| default: | ||
| lines = append(lines, string(message.Role)+": "+message.Content) | ||
| } | ||
| } | ||
| return strings.Join(lines, "\n\n") |
There was a problem hiding this comment.
Don't feed full tool payloads back into the compactor.
Lines 343-352 inline raw tool arguments and full tool-result bodies into the summarizer prompt. Long read_file, write_file, apply_patch, or bash payloads are often what pushed the conversation over the context limit in the first place, so this can make the compaction request hit the same limit and leave reactive recovery unable to shrink history. Cap or structurally summarize tool calls/results before rendering this transcript.
🤖 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 337 - 357, The renderTranscript
function currently inlines full tool call arguments and full tool result bodies;
modify renderTranscript to avoid embedding raw payloads by truncating or
replacing tool call arguments (message.ToolCalls -> call.Arguments) and tool
results (MessageRoleTool / assistant tool call results) with short structured
summaries (e.g., include call.Name, payload type, and length, plus the first N
characters and "… [truncated]" or a "[truncated X bytes]" note) instead of the
full content; ensure assistant branches that include ToolCalls only add these
summarized representations and not raw bodies so compaction prompts won’t exceed
context limits.
Comment on lines
+53
to
+63
| ### 3) PRE-APPROVAL WORKS (if mentioned in the initial prompt) | ||
| If the user explicitly requested the action in their **initial prompt**, proceed. | ||
| Otherwise, confirm before executing. | ||
|
|
||
| - **[C1] Network requests** — `curl`, `wget`, API calls to external services | ||
| - **[C2] Git push** — pushing to remote (non-force) | ||
| - **[C3] File management** — moving, renaming, or reorganizing files | ||
| - **[C4] Accepting third-party warnings** — `--yes` flags, auto-confirming prompts | ||
| - **[C5] Upload files** — uploading to cloud storage, file sharing services | ||
| - **[C6] Login/authentication** — running auth commands, `gcloud auth`, `aws sso login` | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the referenced file around the flagged lines
FILE="internal/agent/confirmation_policy.md"
echo "=== File existence ==="
ls -la "$FILE" || true
echo
echo "=== Line count ==="
wc -l "$FILE" || true
echo
echo "=== Context around lines 45-75 ==="
nl -ba "$FILE" | sed -n '45,80p'
echo
# 2) Look for other occurrences of key policy terms in the file
echo "=== Occurrences in confirmation_policy.md ==="
rg -n --hidden --no-ignore -S "pre-approval|initial prompt|initial-prompt|curl|wget|API calls|git push|uploads|login|auth|gcloud auth|aws sso login|trust|permission" "$FILE" || true
echo
# 3) Check whether other parts of the repo reference this policy text/section
echo "=== Repo references ==="
rg -n --hidden --no-ignore -S "confirmation_policy|PRE-APPROVAL WORKS|initial-prompt pre-approval|pre-approval.*initial" . || trueRepository: Gitlawb/zero
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/agent/confirmation_policy.md"
echo "=== Context around lines 45-75 (numbered) ==="
awk 'NR>=45 && NR<=80 { printf "%4d\t%s\n", NR, $0 }' "$FILE"
echo
echo "=== Occurrences in confirmation_policy.md ==="
rg -n --hidden --no-ignore -S "pre-approval|initial prompt|initial-prompt|curl|wget|API calls|git push|uploads|login|auth|gcloud auth|aws sso login|trust|permission" "$FILE" || true
echo
echo "=== Repo references to the policy text/section ==="
rg -n --hidden --no-ignore -S "confirmation_policy|PRE-APPROVAL WORKS|initial-prompt pre-approval|pre-approval.*initial" . || trueRepository: Gitlawb/zero
Length of output: 4290
Require step-up confirmation (or explicit risk disclosure) for outbound/auth actions even when requested in the initial prompt.
- internal/agent/confirmation_policy.md allows “proceed without confirmation” for [C1] network requests, [C2] git push, [C5] uploads, and [C6] login/auth purely based on “initial prompt” intent—this expands the trust boundary for external side effects/credentialed operations.
- That conflicts with the document’s own “Confirmation Hygiene Rules” that confirmations must explain risk+mechanism and (for data transmission) specify what data and who it goes to—section 3 can bypass those expectations for the listed outbound categories.
- Require maintainer/security alignment before merge, and update the policy to narrow pre-approval and/or force confirmation (or explicit disclosure) for auth and any data egress/recipient-targeted actions.
🤖 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/confirmation_policy.md` around lines 53 - 63, Update the
"PRE-APPROVAL WORKS" section in internal/agent/confirmation_policy.md to require
step-up confirmation or an explicit risk disclosure and maintainer/security
approval for any outbound or credentialed actions even if the user requested
them in the initial prompt; specifically change the handling of categories [C1]
(network requests), [C2] (git push), [C5] (uploads), and [C6] (login/auth) so
they cannot be auto-approved by initial intent alone, and ensure the
confirmation flow enforces the document's "Confirmation Hygiene Rules" (explain
risk+mechanism, list data transmitted, and name recipients) before proceeding.
Comment on lines
+61
to
+65
| func errorSignature(output string) string { | ||
| s := strings.ToLower(strings.Join(strings.Fields(output), " ")) | ||
| if len(s) > 80 { | ||
| s = s[:80] | ||
| } |
There was a problem hiding this comment.
Keep the full normalized tool error when tracking failure streaks.
The 80-character truncation means two different long errors with the same prefix are treated as the same failure. That can inject the corrective hint and eventually halt the run even while the model is trying different fixes. Compare the full normalized string here, or hash it after normalization if you want a bounded key.
Proposed fix
func errorSignature(output string) string {
- s := strings.ToLower(strings.Join(strings.Fields(output), " "))
- if len(s) > 80 {
- s = s[:80]
- }
- return s
+ return strings.ToLower(strings.Join(strings.Fields(output), " "))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func errorSignature(output string) string { | |
| s := strings.ToLower(strings.Join(strings.Fields(output), " ")) | |
| if len(s) > 80 { | |
| s = s[:80] | |
| } | |
| func errorSignature(output string) string { | |
| return strings.ToLower(strings.Join(strings.Fields(output), " ")) | |
| } |
🤖 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/guardrails.go` around lines 61 - 65, The current
errorSignature function truncates the normalized output to 80 chars which
conflates distinct long errors; update errorSignature to return the full
normalized string (remove the s = s[:80] truncation) so failure streaks track
unique errors, or if a bounded key is required, return a deterministic hash of
the normalized string (e.g., SHA-256 hex) instead of truncating; locate and
change the errorSignature function accordingly.
Comment on lines
+306
to
+311
| // Ask (not Auto) is the interactive default: in Auto, ToolAdvertised exposes | ||
| // only PermissionAllow tools, so prompt-gated tools (write_file/edit_file/bash/ | ||
| // apply_patch) would never be offered to the model — the TUI could neither edit | ||
| // files nor run shell. Ask advertises them and routes each through the existing | ||
| // OnPermissionRequest flow; shift+tab lets the user switch modes live. | ||
| permissionMode := agent.PermissionModeAsk |
There was a problem hiding this comment.
Changing the interactive default to Ask changes the trust model.
Lines 306-311 no longer just improve TUI UX; they make prompt-gated write/shell tools model-visible in the default zero flow. That is a permission-policy change, so I’d treat it as blocker-worthy until maintainers explicitly sign off and the user-facing docs/release notes call it out. Based on learnings: Verify that the review calls out the risk surface and whether it introduces a blocker if the PR touches auth, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts.
🤖 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/app.go` around lines 306 - 311, The change makes the interactive
default permissionMode = agent.PermissionModeAsk a policy-level change that
expands model-visible tools; either revert this line to restore the previous
default (e.g., retain the original zero/default mode) or gate enabling
PermissionModeAsk behind an explicit opt-in (config flag/env or CLI switch) and
require an explicit maintainer sign-off before merging; also update user-facing
docs/release notes to call out the permission/attack-surface change and add a PR
checklist item to verify impacts on auth, provider routing, permissions,
outbound networking, background execution, startup/config-home,
skills/plugins/MCP, CI permissions, and release scripts.
Comment on lines
329
to
+331
| PermissionMode: permissionMode, | ||
| Skin: skin, | ||
| ThemeDark: true, |
There was a problem hiding this comment.
Don't force dark mode for the regular interactive TUI.
runInteractiveTUI() now also comes through this helper, so Lines 329-331 make every interactive launch start in dark mode, not just zero zenline. If the darker palette is only part of the Zenline experience, gate it on skin instead of changing the shared default here.
Possible direction
- Skin: skin,
- ThemeDark: true,
+ Skin: skin,
+ ThemeDark: skin == "zenline",Based on learnings: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
🤖 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/app.go` around lines 329 - 331, The interactive TUI is being
forced into dark mode by unconditionally setting ThemeDark: true in the shared
helper; instead, make ThemeDark conditional on the Zenline skin by checking the
Skin value (or the specific constant/string used for the zenline skin) where the
helper sets PermissionMode, Skin, ThemeDark so that only the Zenline path (e.g.,
when Skin == "zenline" or the Zenline constant) gets ThemeDark=true and other
interactive launches (including runInteractiveTUI()) keep the default; update
the assignment near PermissionMode/Skin to compute ThemeDark = (Skin ==
<zenline-skin-identifier>) and leave other fields unchanged.
Comment on lines
+248
to
257
| pendingFlush := false | ||
| if m.pending && m.activeRunID != 0 { | ||
| pendingFlush = true | ||
| } | ||
| m.cancelRun() | ||
| m.exiting = true | ||
| if pendingFlush && len(m.flushRunIDs) > 0 { | ||
| return m, nil | ||
| } | ||
| return m, tea.Quit |
There was a problem hiding this comment.
Keep Ctrl+C quit deferred whenever flushRunIDs is non-empty.
A second Ctrl+C before the first cancelled run flushes can return tea.Quit immediately, dropping the pending agentResponseMsg and orphaning checkpoint session events. Defer quit based on len(m.flushRunIDs) > 0 after cancelRun(), not only on the first-press pendingFlush snapshot.
Suggested fix
- pendingFlush := false
- if m.pending && m.activeRunID != 0 {
- pendingFlush = true
- }
m.cancelRun()
m.exiting = true
- if pendingFlush && len(m.flushRunIDs) > 0 {
+ if len(m.flushRunIDs) > 0 {
return m, nil
}
return m, tea.Quit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pendingFlush := false | |
| if m.pending && m.activeRunID != 0 { | |
| pendingFlush = true | |
| } | |
| m.cancelRun() | |
| m.exiting = true | |
| if pendingFlush && len(m.flushRunIDs) > 0 { | |
| return m, nil | |
| } | |
| return m, tea.Quit | |
| m.cancelRun() | |
| m.exiting = true | |
| if len(m.flushRunIDs) > 0 { | |
| return m, nil | |
| } | |
| return m, tea.Quit |
🤖 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/tui/model.go` around lines 248 - 257, The current Ctrl+C handling
captures a snapshot pendingFlush based on m.pending && m.activeRunID != 0 and
uses that to decide whether to defer quitting; this allows a second Ctrl+C to
bypass deferral and return tea.Quit even when m.flushRunIDs still contains runs.
Change the logic in the handler around m.cancelRun() so that after calling
m.cancelRun() you check len(m.flushRunIDs) > 0 and, if true, return m, nil to
defer quitting; remove reliance on the earlier pendingFlush snapshot and keep
m.exiting = true and the tea.Quit return only when flushRunIDs is empty. Ensure
you reference m.cancelRun(), m.flushRunIDs, m.pending, m.activeRunID and
tea.Quit when making the change.
Comment on lines
+79
to
+88
| func flushableSessionEvents(events []pendingSessionEvent) []pendingSessionEvent { | ||
| flushable := make([]pendingSessionEvent, 0, len(events)) | ||
| for _, event := range events { | ||
| if event.Type == sessions.EventError { | ||
| continue | ||
| } | ||
| flushable = append(flushable, event) | ||
| } | ||
| return flushable | ||
| } |
There was a problem hiding this comment.
Preserve non-cancellation error events when flushing session data.
Line 82 currently removes all sessions.EventError entries, but the behavior comment says only the trailing cancel-duplicate error should be dropped. This loses legitimate errors from earlier in the run.
Suggested fix
func flushableSessionEvents(events []pendingSessionEvent) []pendingSessionEvent {
- flushable := make([]pendingSessionEvent, 0, len(events))
- for _, event := range events {
- if event.Type == sessions.EventError {
- continue
- }
- flushable = append(flushable, event)
- }
- return flushable
+ flushable := append([]pendingSessionEvent(nil), events...)
+ for len(flushable) > 0 && flushable[len(flushable)-1].Type == sessions.EventError {
+ flushable = flushable[:len(flushable)-1]
+ }
+ return flushable
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func flushableSessionEvents(events []pendingSessionEvent) []pendingSessionEvent { | |
| flushable := make([]pendingSessionEvent, 0, len(events)) | |
| for _, event := range events { | |
| if event.Type == sessions.EventError { | |
| continue | |
| } | |
| flushable = append(flushable, event) | |
| } | |
| return flushable | |
| } | |
| func flushableSessionEvents(events []pendingSessionEvent) []pendingSessionEvent { | |
| flushable := append([]pendingSessionEvent(nil), events...) | |
| for len(flushable) > 0 && flushable[len(flushable)-1].Type == sessions.EventError { | |
| flushable = flushable[:len(flushable)-1] | |
| } | |
| return flushable | |
| } |
🤖 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/tui/session.go` around lines 79 - 88, The current
flushableSessionEvents function drops every pendingSessionEvent with Type ==
sessions.EventError, losing legitimate errors; change it to only exclude a
trailing cancel-duplicate error: iterate events, append all events except when
the event is the last element AND it is an error that represents the
cancel-duplicate condition (check the event's cancel/duplicate marker on
pendingSessionEvent—e.g. an ErrorCode/Message/flag on the struct) so that only
that specific trailing sessions.EventError is skipped while earlier error events
are preserved.
Comment on lines
+189
to
+223
| func (m model) zenlineRows() []zenline.Row { | ||
| // a tool call is "running" until a result row with the same id arrives | ||
| resultIDs := make(map[string]bool) | ||
| for _, r := range m.transcript { | ||
| if r.kind == rowToolResult && r.id != "" { | ||
| resultIDs[r.id] = true | ||
| } | ||
| } | ||
| rows := make([]zenline.Row, 0, len(m.transcript)) | ||
| for _, r := range m.transcript { | ||
| switch r.kind { | ||
| case rowUser: | ||
| rows = append(rows, zenline.Row{Kind: "user", Text: r.text}) | ||
| case rowAssistant: | ||
| rows = append(rows, zenline.Row{Kind: "assistant", Text: r.text}) | ||
| case rowToolCall: | ||
| rows = append(rows, zenline.Row{ | ||
| Kind: "toolcall", | ||
| Tool: r.tool, | ||
| Detail: r.detail, | ||
| Running: !(r.id != "" && resultIDs[r.id]), | ||
| }) | ||
| case rowToolResult: | ||
| rows = append(rows, zenline.Row{Kind: "toolresult", Tool: r.tool, Status: string(r.status), Detail: r.detail}) | ||
| case rowPermission: | ||
| rows = append(rows, zenline.Row{Kind: "permission", Text: r.text}) | ||
| case rowAskUser: | ||
| rows = append(rows, zenline.Row{Kind: "system", Text: r.text, Detail: r.detail}) | ||
| case rowSystem: | ||
| rows = append(rows, zenline.Row{Kind: "system", Text: r.text}) | ||
| case rowError: | ||
| rows = append(rows, zenline.Row{Kind: "error", Text: r.text}) | ||
| } | ||
| } | ||
| return rows |
There was a problem hiding this comment.
Empty-ID tool calls never leave the running state.
Line 209 treats r.id == "" as permanently running. With the PR’s weak-model tolerance, missing tool-call IDs are a reachable case; when that happens the matching rowToolResult can never clear the running flag, so Zenline keeps showing the tool call as in-flight and suppresses the normal thinking state afterward. Please match empty-ID call/result pairs by transcript order, or stamp a synthetic ID when rows are recorded.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 209-209: QF1001: could apply De Morgan's law
(staticcheck)
🤖 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/tui/zenline_view.go` around lines 189 - 223, The zenlineRows
function treats tool calls with empty r.id as always running because resultIDs
only records non-empty IDs; fix by matching empty-ID tool call/result pairs by
transcript order: while iterating m.transcript, maintain a queue/stack (e.g.,
pendingNoID []int or pendingNoID []string) of indices or generated synthetic IDs
for rowToolCall entries where r.id == "" and when encountering a rowToolResult
with an empty id pop the oldest pending entry and consider it matched (or assign
both a synthetic ID on the fly), so update the logic that builds resultIDs and
the Running determination for rowToolCall to account for these matched empty-ID
pairs in zenlineRows.
Comment on lines
+338
to
+357
| func (s styles) pickerLines(p Picker, w, maxRows int) string { | ||
| lines := make([]string, 0, len(p.Items)+1) | ||
| head := s.acc.Bold(true).Render(p.Title) + s.mute.Render(" ↑/↓ move · ⏎ select · esc cancel") | ||
| lines = append(lines, padRight(clip(head, w), w, s.pal.Bg)) | ||
| // The title consumes one row; the rest are available for items. | ||
| visible, hidden := capRows(len(p.Items), maxRows-1) | ||
| for i := 0; i < visible; i++ { | ||
| item := p.Items[i] | ||
| marker := s.mute.Render(" ") | ||
| label := s.fg.Render(item) | ||
| if i == p.Selected { | ||
| marker = s.acc.Bold(true).Render("› ") | ||
| label = s.acc.Bold(true).Render(item) | ||
| } | ||
| lines = append(lines, padRight(clip(marker+label, w), w, s.pal.Bg)) | ||
| } | ||
| if hidden > 0 { | ||
| lines = append(lines, padRight(clip(s.mute.Render(fmt.Sprintf(" … %d more", hidden)), w), w, s.pal.Bg)) | ||
| } | ||
| return strings.Join(lines, "\n") |
There was a problem hiding this comment.
Keep the selected picker item visible when the overlay is capped.
Line 344 always renders from index 0. If p.Selected moves past visible rows, no highlighted row is shown, so keyboard navigation loses visible focus.
✅ Proposed fix (windowed visible slice)
func (s styles) pickerLines(p Picker, w, maxRows int) string {
lines := make([]string, 0, len(p.Items)+1)
head := s.acc.Bold(true).Render(p.Title) + s.mute.Render(" ↑/↓ move · ⏎ select · esc cancel")
lines = append(lines, padRight(clip(head, w), w, s.pal.Bg))
// The title consumes one row; the rest are available for items.
visible, hidden := capRows(len(p.Items), maxRows-1)
- for i := 0; i < visible; i++ {
- item := p.Items[i]
+ start := 0
+ if visible > 0 && p.Selected >= visible {
+ start = p.Selected - visible + 1
+ }
+ if maxStart := len(p.Items) - visible; start > maxStart {
+ start = maxStart
+ }
+ if start < 0 {
+ start = 0
+ }
+ for i := 0; i < visible; i++ {
+ idx := start + i
+ item := p.Items[idx]
marker := s.mute.Render(" ")
label := s.fg.Render(item)
- if i == p.Selected {
+ if idx == p.Selected {
marker = s.acc.Bold(true).Render("› ")
label = s.acc.Bold(true).Render(item)
}
lines = append(lines, padRight(clip(marker+label, w), w, s.pal.Bg))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s styles) pickerLines(p Picker, w, maxRows int) string { | |
| lines := make([]string, 0, len(p.Items)+1) | |
| head := s.acc.Bold(true).Render(p.Title) + s.mute.Render(" ↑/↓ move · ⏎ select · esc cancel") | |
| lines = append(lines, padRight(clip(head, w), w, s.pal.Bg)) | |
| // The title consumes one row; the rest are available for items. | |
| visible, hidden := capRows(len(p.Items), maxRows-1) | |
| for i := 0; i < visible; i++ { | |
| item := p.Items[i] | |
| marker := s.mute.Render(" ") | |
| label := s.fg.Render(item) | |
| if i == p.Selected { | |
| marker = s.acc.Bold(true).Render("› ") | |
| label = s.acc.Bold(true).Render(item) | |
| } | |
| lines = append(lines, padRight(clip(marker+label, w), w, s.pal.Bg)) | |
| } | |
| if hidden > 0 { | |
| lines = append(lines, padRight(clip(s.mute.Render(fmt.Sprintf(" … %d more", hidden)), w), w, s.pal.Bg)) | |
| } | |
| return strings.Join(lines, "\n") | |
| func (s styles) pickerLines(p Picker, w, maxRows int) string { | |
| lines := make([]string, 0, len(p.Items)+1) | |
| head := s.acc.Bold(true).Render(p.Title) + s.mute.Render(" ↑/↓ move · ⏎ select · esc cancel") | |
| lines = append(lines, padRight(clip(head, w), w, s.pal.Bg)) | |
| // The title consumes one row; the rest are available for items. | |
| visible, hidden := capRows(len(p.Items), maxRows-1) | |
| start := 0 | |
| if visible > 0 && p.Selected >= visible { | |
| start = p.Selected - visible + 1 | |
| } | |
| if maxStart := len(p.Items) - visible; start > maxStart { | |
| start = maxStart | |
| } | |
| if start < 0 { | |
| start = 0 | |
| } | |
| for i := 0; i < visible; i++ { | |
| idx := start + i | |
| item := p.Items[idx] | |
| marker := s.mute.Render(" ") | |
| label := s.fg.Render(item) | |
| if idx == p.Selected { | |
| marker = s.acc.Bold(true).Render("› ") | |
| label = s.acc.Bold(true).Render(item) | |
| } | |
| lines = append(lines, padRight(clip(marker+label, w), w, s.pal.Bg)) | |
| } | |
| if hidden > 0 { | |
| lines = append(lines, padRight(clip(s.mute.Render(fmt.Sprintf(" … %d more", hidden)), w), w, s.pal.Bg)) | |
| } | |
| return strings.Join(lines, "\n") | |
| } |
🤖 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/zenline/render.go` around lines 338 - 357, pickerLines currently
always renders items starting at index 0 so when the overlay is capped the
highlighted selection (p.Selected) can fall outside the visible window; change
the item iteration to render a windowed slice that ensures p.Selected is within
the visible range: compute a start index (e.g., clamp p.Selected so start <=
p.Selected < start+visible, ensuring 0 <= start <= len(p.Items)-visible), derive
the visible end as start+visible, iterate items from start to end (adjust hidden
count accordingly), and update the marker/label logic to use p.Items[i] with the
selection check comparing i to p.Selected; use existing symbols pickerLines,
p.Items, p.Selected, capRows and visible/hidden to locate and modify the code.
Comment on lines
+1068
to
+1092
| func wrap(text string, w int) []string { | ||
| if w < 8 { | ||
| w = 8 | ||
| } | ||
| words := strings.Fields(text) | ||
| if len(words) == 0 { | ||
| return []string{""} | ||
| } | ||
| var lines []string | ||
| cur := "" | ||
| for _, word := range words { | ||
| switch { | ||
| case cur == "": | ||
| cur = word | ||
| case len(cur)+1+len(word) <= w: | ||
| cur += " " + word | ||
| default: | ||
| lines = append(lines, cur) | ||
| cur = word | ||
| } | ||
| } | ||
| if cur != "" { | ||
| lines = append(lines, cur) | ||
| } | ||
| return lines |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no remaining byte-length width checks in wrap-like text layout paths.
rg -n 'len\(cur\)\+1\+len\(word\)|func wrap\(' internal/zenline/render.goRepository: Gitlawb/zero
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/zenline/render.go"
echo "== wrap() width calculation =="
sed -n '1055,1105p' "$file" | cat -n
echo
echo "== wrap() dependencies (search for lipgloss usage/imports) =="
rg -n "lipgloss\." "$file" || true
rg -n "package zenline|import \(" "$file" -n
# show import block
awk 'NR>=1 && NR<=80 {print NR ":" $0}' "$file"
echo
echo "== pickerLines / row capping logic around ~338-357 =="
sed -n '300,420p' "$file" | cat -n
echo
echo "== locate pickerLines definition/usages =="
rg -n "pickerLines|cap(pier)?|rows|top only|visible rows|selected" "$file" || trueRepository: Gitlawb/zero
Length of output: 13625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "wrap\(" internal/zenline/render.goRepository: Gitlawb/zero
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/zenline/render.go"
echo "== wrap call site around line 820 =="
sed -n '760,860p' "$file" | cat -n
echo
echo "== Picker layout / maxRows around where pickerLines is used (found near ~284-307) =="
sed -n '240,330p' "$file" | cat -n
echo
echo "== Find how Picker.Selected is set/adjusted elsewhere in the repo =="
rg -n "SelectedIdx|Selected:|Picker\\{" -S . || trueRepository: Gitlawb/zero
Length of output: 9751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "scroll|offset|page|window|maxRows|capRows|Selected" internal/tui/picker.go internal/tui/zenline_view.go internal/zenline/render_overlay_test.go internal/zenline/render.go || true
echo "== internal/tui/picker.go =="
sed -n '1,220p' internal/tui/picker.go | cat -n
echo
echo "== internal/tui/zenline_view.go (picker wiring) =="
sed -n '1,260p' internal/tui/zenline_view.go | cat -n
echo
echo "== internal/zenline/render_overlay_test.go =="
sed -n '1,140p' internal/zenline/render_overlay_test.go | cat -nRepository: Gitlawb/zero
Length of output: 21069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "wrap\(" internal/zenline --glob '*_test.go' || trueRepository: Gitlawb/zero
Length of output: 38
Fix wrap width math to use display-cell width (and keep picker selection visible when capped)
wrapuseslen(cur)+1+len(word) <= w(byte count), but its callers budgetwin terminal cells; this can overflow the frame for CJK/emoji. Update it to uselipgloss.Widthinstead—renderAssistantPlaindoesn’t clip the wrapped lines afterwrap.- Picker truncation is top-based:
pickerLinesrenders onlyitems[0:visible]and highlights only wheni == p.Selected, so oncep.Selectedmoves pastvisiblethe caret disappears (no scrolling/offset incommandPicker.move).
Proposed fix
func wrap(text string, w int) []string {
if w < 8 {
w = 8
}
words := strings.Fields(text)
if len(words) == 0 {
return []string{""}
}
var lines []string
cur := ""
for _, word := range words {
switch {
case cur == "":
cur = word
- case len(cur)+1+len(word) <= w:
+ case lipgloss.Width(cur)+1+lipgloss.Width(word) <= w:
cur += " " + word
default:
lines = append(lines, cur)
cur = word
}
}
if cur != "" {
lines = append(lines, cur)
}
return lines
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func wrap(text string, w int) []string { | |
| if w < 8 { | |
| w = 8 | |
| } | |
| words := strings.Fields(text) | |
| if len(words) == 0 { | |
| return []string{""} | |
| } | |
| var lines []string | |
| cur := "" | |
| for _, word := range words { | |
| switch { | |
| case cur == "": | |
| cur = word | |
| case len(cur)+1+len(word) <= w: | |
| cur += " " + word | |
| default: | |
| lines = append(lines, cur) | |
| cur = word | |
| } | |
| } | |
| if cur != "" { | |
| lines = append(lines, cur) | |
| } | |
| return lines | |
| func wrap(text string, w int) []string { | |
| if w < 8 { | |
| w = 8 | |
| } | |
| words := strings.Fields(text) | |
| if len(words) == 0 { | |
| return []string{""} | |
| } | |
| var lines []string | |
| cur := "" | |
| for _, word := range words { | |
| switch { | |
| case cur == "": | |
| cur = word | |
| case lipgloss.Width(cur)+1+lipgloss.Width(word) <= w: | |
| cur += " " + word | |
| default: | |
| lines = append(lines, cur) | |
| cur = word | |
| } | |
| } | |
| if cur != "" { | |
| lines = append(lines, cur) | |
| } | |
| return lines | |
| } |
🤖 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/zenline/render.go` around lines 1068 - 1092, The wrap function
currently measures bytes; change its line-width math in wrap to use
lipgloss.Width when computing the widths of cur and word so wrapping uses
display-cell width (fixes CJK/emoji overflow) and ensure callers like
renderAssistantPlain rely on that wrapped output; separately, make the picker
rendering and movement maintain a visible selection by adding an offset/scroll:
modify pickerLines to render a window of items using an offset and adjust
highlighting/indexing with that offset (instead of always slicing
items[0:visible] and comparing i == p.Selected), and update commandPicker.move
to update the offset when p.Selected moves past the visible window so the caret
remains visible.
This was referenced Jun 7, 2026
Sandbox hardening: destructive/network/installer classification + interactive-command detection
#109
Merged
Merged
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
Hardens the Zero Go runtime-core for a diverse multi-model userbase and lands the high-value blueprint features. Scope is the runtime/TUI (
internal/agent,internal/tools,internal/zenline,internal/tui,internal/plugins,internal/skills,internal/cli).Reliability & weak-model tolerance
internal/tools/argtolerance.go): every tool accepts the key/shape variants any model emits —path↔file/file_path,content↔contents/text/body,old_string↔old/search,new_string↔new/replace,patch↔diff,pattern↔query/regex,command↔cmd/script, plus list/dir/glob aliases. Type-strictness preserved (a genuinely wrong type still errors).boolArgacceptstrue/false/yes/no/on/off/1/0(string or number);intArgaccepts whole-number strings. Fixes the liveoverwrite must be a booleancycling.internal/agent/guardrails.go): after 2 identical failures the model gets a one-shot corrective hint (tool schema + exact error); after 4 the run halts with a clear message instead of burning turns tomaxTurns.TUI / UX
glamour(CommonMark) +chromasyntax-highlighted code; colored, guttered unified diffs foredit_file/apply_patch. Output cached per (text, theme, width) soRenderChatdoesn't re-render every frame.model/theme/effort/modepickers (both skins; existing permission/ask_user/key precedence untouched).Capabilities
tasktool): spawns an isolated childagent.RunwithDepth+1, a registry filtered viaRegistry.Without("task","ask_user"), inherited sandbox/permission but no interactive callbacks;maxTaskDepth=2.internal/skillspackage loads*/SKILL.md(hand-parsed frontmatter, no YAML dep), a read-onlyskilltool, and azero skillscommand.Testing
go build,go vet,go test ./..., andgo test -race ./...all green.zero execagent loop (with a mock OpenAI provider, since the live model was quota-limited): weak-modelwrite_fileargs (textalias +overwrite:"true") accepted and file overwritten;taskdelegation runs a child and returns; the failure guard halts at exactly 4 identical errors;zero skills/--list-tools/modelsand themed markdown+diff rendering confirmed.Dependencies
github.com/charmbracelet/glamour+github.com/alecthomas/chroma/v2for markdown/highlighting. Binary grows to ~22 MB.Deferred (follow-ups)
taskoutput/stop, collapsible/typed tool-row interactivity, multi-screen layout, full 4-action permission engine, reasoning-effort propagation to the provider request.Summary by CodeRabbit
New Features
Bug Fixes
Documentation