Skip to content

TUI scrollback re-architecture, Lime chat surface, and deep-audit fixes#158

Merged
gnanam1990 merged 15 commits into
mainfrom
tui-scrollback-and-deep-audit
Jun 10, 2026
Merged

TUI scrollback re-architecture, Lime chat surface, and deep-audit fixes#158
gnanam1990 merged 15 commits into
mainfrom
tui-scrollback-and-deep-audit

Conversation

@gnanam1990

@gnanam1990 gnanam1990 commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR lands the Lime TUI work rebased onto current main, plus a full deep audit of the codebase with fixes for the highest-impact findings.

TUI: real terminal scrollback (settled-row flush frontier)

The chat surface previously rendered the whole transcript inside Bubble Tea's managed View — the inline renderer clips anything taller than the terminal, so history above one screen was unrecoverable (scrolling up showed pre-launch screen contents). Now:

  • Settled transcript rows are printed once to native terminal scrollback via tea.Println (strictly ordered, ack-serialized); View() renders only the live tail (running tool cards, undecided prompts, streaming text, composer, status). Scroll-up, selection, and copy of full history work natively.
  • Flushed cards use a deep 400-line body cap, so full diffs of edited code are reviewable in history; the live region keeps the tidy 16-line cap.
  • Diff paths, file-tool targets, and grep hits are OSC 8 file:// hyperlinks — cmd/ctrl+click opens the edited file (URLs percent-encoded; ANSI truncation is OSC-aware and re-closes open links).
  • Streamed interim text is preserved as non-final assistant rows instead of vanishing when a tool card lands; Esc/Ctrl+C leave a visible "Run cancelled." marker plus the partial answer.
  • Composer: width tracks the terminal (no more invisible clipping) and ↑/↓ recalls previously submitted inputs.
  • /resume and /rewind flush rehydrated history to scrollback in one batch; ask_user questions and answers persist and rehydrate.

TUI correctness fixes

  • Run-scoped transcript dedup keys + per-run ordinal suffixes: providers with repeating synthesized tool-call ids (Gemini gemini_tool_N) no longer silently drop tool cards, live or on /resume.
  • /exit mid-run and Ctrl+C-after-Esc no longer orphan rewind checkpoint blobs (quit deferred until the cancelled run's session events flush); /spec gated while exiting.
  • A cancelled run's late event flush is written to the session that was active at cancel time, so /resume-ing another session can't be contaminated; /rewind is blocked while a flush is outstanding; cancelled-run usage is recorded.
  • O(n²) transcript append removed; per-frame render cost no longer grows with session length; CancelFunc leak per prompt fixed; indentation-preserving prose wrap (code blocks survive); looksLikeDiff no longer hijacks output containing --- separators; rune-safe truncation of titles and tool output; dead footer/actions/theme code removed.

Cross-module audit fixes

  • zero search: panic + invalid-UTF-8 context fixed (lowered-text offsets mapped back to the original); unknown --session-id errors instead of silently returning 0 hits; corrupt sessions skipped, not fatal.
  • zero usage report: corrupt sessions skipped; works outside a git repository.
  • Providers: HTTP 529 retried; SSE comment keep-alives now reset the idle watchdog (heartbeating upstreams no longer killed).
  • Model registry: gpt-4.1 max output 16,384 → 32,768; claude-haiku-3.5 vision flag removed (model has no image input).
  • zero update --check works on dev/source builds.
  • MCP: server answers ping per spec; zero mcp list --json structurally redacts env/header values.
  • New sandbox.network: allow|deny config knob — the engine's NetworkDeny default was previously unreachable from any config surface, making "Sandbox violation [network]" impossible to opt out of.
  • zero doctor reports apiKey presence instead of an unconditional [REDACTED]; atomic config writes; C1 control bytes stripped from OSC-9 notifications; imageinput surfaces real open/read errors; rune-safe truncation across exec/stream-json/cron/sessions/zerogit/agent system prompt.
  • Hygiene: repo gofmt'ed, CI gates on gofmt + go vet, dead glamour dependency dropped.

Audit report

docs/audit/2026-06-10-deep-audit.md — 175 findings across all 44 packages with evidence and suggested fixes; the "Fixed in this pass" section maps to this PR, the rest are open for follow-up (top items: MCP stdio framing, hooks never dispatched, reasoning effort never serialized, specialist child permissions, bash timeout kill semantics, macOS sandbox-exec profile).

Test plan

  • go test ./... green (new regression tests for the flush frontier, dedup keys, input history, UTF-8 cuts, OSC truncation, wrap indentation, diff detection)
  • go vet ./... and gofmt -l clean; both now enforced in CI
  • Manual pty smoke test of the TUI (title bar to scrollback, empty state, composer, status line)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Lime TUI: refreshed near‑black lime chat UI with tool cards, starter suggestions (1–3), responsive width tiers, session cards on resume, improved composer/status hints, and richer permission/ask-user flows.
    • Sandbox network option to allow or deny network‑touching commands.
  • Bug Fixes

    • UTF‑8‑safe truncation to avoid corrupted characters.
    • Safer config file writes to prevent truncated/corrupt files.
    • Cleaner CLI/help and clearer TUI startup error reporting.
  • Documentation

    • README updated to document the Lime TUI controls and behavior.

gnanam1990 and others added 9 commits June 10, 2026 13:25
One design remains in the tree ahead of the Lime rebuild: the zeroline
package, its TUI view/skin plumbing (Options.Skin/ThemeVariant/ThemeDark,
model skin branches, theme picker), and the zeroline CLI subcommand are
gone. /theme keeps its shell-only message.
theme.go is rebuilt around the Lime design tokens (docs/design/
zero_tui_lime.html): near-black panels, one lime accent, solid tint
stand-ins for the prototype's rgba overlays. Existing render sites map
onto the new role styles (ink/line/userPrompt/toolName/toolArg) without
layout changes; no hex exists outside theme.go.
Title bar with brand badge + context window, empty state replacing the
deleted splash (block-art 0, tagline, 1-3 starter suggestions), stream
blocks (user prompt gutter, muted interim with cursor, final-answer
accent rail, done line, bordered notes), tool cards with diff/read/
bash/grep bodies and MiniDot run spinner, borderless composer with
run-state hints, and the grouped status line. Final answers are marked
at append time; resolved tool calls collapse into their result cards.
Adversarial review of the chat-surface commit confirmed 15 issues, all
fixed here:
- wrapPlainText sliced wide runes by rune count: CJK/emoji prose could
  panic the TUI or emit lines ~2x the measure; split at width instead
- a cancelled run's streamingText leaked into the next turn's interim
  block; cancelRun now clears it
- orphaned tool-call cards (cancelled/errored turns) re-animated with
  the spinner on any later run; the glyph is now scoped to the call's
  own runID
- starter-suggestion digit capture was active while the chips were off
  screen (/clear mid-run); the guard now mirrors the render condition
- prompt submits while a deferred Ctrl+C quit is draining could start a
  run the quit would orphan; submission is blocked while exiting
- the running placeholder advertised ctrl+c (which quits) as the
  interrupt key; it now says esc
- read-card parser expected 'N: text' but read_file emits 'N | text',
  so the gutter and L-range never rendered on real output
- toolCard borders were one column short of the body rows
- diff tint bands now span the full row; NEW FILE renders green on
  addBg; preamble/no-newline lines are unnumbered meta and no longer
  shift hunk numbering
- card bodies and system notes paint the panel surface; the head gains
  the separate faintest arg column (grep target + pattern)
- title-bar overflow now truncates the minimal candidate instead of
  rendering the deleted design's 'ZERO … READY' line
- untracked the stray snake-game.html (left on disk, ignored)
Permission prompts render as the amber permission card (PERMISSION
badge, risk readout, key chips); resolved prompts collapse to one line
(allowed once/always/denied · tool) and unprompted allows fold into the
tool card's [auto] tag. Ask-user and spec-review prompts share the card
language with line borders. Autocomplete and picker overlays move onto
the panel/selection tints with the accent ❯ marker; model-picker rows
gain the provider dot and ctx · KEY_ENV metadata from the registry and
provider catalog. /resume renders its list as stacked session cards
(id + age, title, meta). Key handling is untouched everywhere.
Width tiers re-evaluate from the live width: ≥100 renders the full
spec; 80–99 drops the tool-arg column, header ctx, and the status
'interactive' group; 58–79 drops the diff/read gutters, bares the
badge, and keeps provider+tokens+mode; <58 renders a single-segment
header, rail-less cards, and a mode-only status line. Table-driven
tests cover {58,70,80,100,120} plus a frame-wide invariant that no
emitted line exceeds the terminal at any tier. Card bodies expand tabs
so width math holds. Dead splash-chip machinery (startupCommandNames,
startupOrder) is gone; README's TUI section now describes Lime.
A second adversarial review confirmed 14 issues, all addressed:
- permission collapse and tool-card pairing are now run-scoped (rcKey):
  providers with repeating synthetic ToolCallIDs (Gemini gemini_tool_N)
  could attribute a decision or result to a different run's call
- rehydrated always-grants no longer mislabel as 'allowed once'
  (GrantMatched fallback)
- the width invariant now actually holds everywhere: empty-state
  tagline/hint, ask-user rows, permission detail blobs, pending image
  chips, and the sessions trailing hint are wrapped or fitted; the
  invariant test renders the empty state and all of those rows at
  widths 24-120
- every card (permission, ask-user, spec-review, sessions, notes)
  drops side borders on tiny terminals, not just tool cards
- /resume card fields are sanitized against separator bytes from
  user-controlled titles
- the picker's selected row paints its gap so the selBg band is solid
- dead splash helpers (normalizedStartupWidth, countLines, nonEmpty,
  unreachable rowWelcome branch) and stale /theme comments are gone
- README no longer claims transcript scrolling keys or Tab-accept
  behavior that never existed; chatWidth documents its 24-col floor

Tag tui-lime-s4 moves to this commit.
…odule fixes

TUI: settled-row flush frontier prints history to native terminal
scrollback (tea.Println, ordered + ack-serialized); View() renders only
the live tail. Full diffs (400-line flush cap) and OSC 8 file:// links
land in history; streamed interim text, cancellation markers, ask_user
exchanges, and composer input history are preserved; run-scoped dedup
keys fix Gemini's repeating tool-call ids; /exit, Ctrl+C-after-Esc, and
cross-session late flushes no longer orphan or contaminate checkpoints.

Cross-module: zero search panic + offset mapping, usage report
robustness, 529 retry, SSE keep-alive watchdog, registry corrections,
update --check on dev builds, MCP ping + mcp list --json secret
redaction, sandbox.network config knob, doctor apiKey presence, atomic
config writes, C1-safe notifications, rune-safe truncation everywhere,
gofmt + vet CI gates, dead glamour dependency dropped.

Full audit report: docs/audit/2026-06-10-deep-audit.md (175 findings).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: abd17f51-6235-4e3f-b7cc-f7c863d83d10

📥 Commits

Reviewing files that changed from the base of the PR and between 768d7b5 and edf4fb8.

📒 Files selected for processing (4)
  • internal/agent/guardrails.go
  • internal/agent/guardrails_test.go
  • internal/tools/bash_tool_test.go
  • internal/tools/shell_runtime.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/agent/guardrails.go
  • internal/tools/bash_tool_test.go
  • internal/tools/shell_runtime.go

Walkthrough

Adds a Lime single-column chat design and interactive prototype; removes the legacy zeroline renderer; refactors TUI model, rendering, flush, and session resume flows; enforces UTF‑8-safe truncation across modules; tightens tool schemas and agent max-turn behavior; adds gofmt/go vet CI checks and extensive Lime rendering/width tests.

Changes

Lime TUI rebuild

Layer / File(s) Summary
Design, prototype, README and CI
docs/design/*, docs/design/zero_tui_lime.html, README.md, .github/workflows/ci.yml, go.mod, .gitignore
Adds Lime design spec and interactive HTML prototype, updates README to describe Lime TUI, enables gofmt and go vet CI steps on ubuntu, updates go.mod, and ignores /snake-game.html.
CLI, config, sandbox, and runtime helpers
internal/cli/*, internal/config/*, internal/config/writer.go, internal/update/*
Removes zeroline CLI path, inlines TUI startup wiring, validates Sandbox.Network, atomically writes config files (temp+rename), increases default MaxTurns, and prints TUI runtime errors.
Provider/session/search/IO fixes
internal/providers/*, internal/search/*, internal/sessions/*, internal/cli/usage.go
Adds SSE comment/heartbeat keep-alives, treats HTTP 529 as retryable, tolerates unreadable session event logs (skips), makes search Unicode-safe via offset mapping and rune-boundary context slicing, and makes session replay truncation UTF‑8 safe.
TUI core: model, transcript, flush, session
internal/tui/*
Refactors model/update loop, introduces run-scoped transcript ids and metadata, implements flush-frontier scrollback printing, reworks session resume/rehydration and ask_user persistence, converts many tests to use execCmd, and adjusts the TUI Options surface.
Renderer, cards, wrapping, hyperlinks, theme
internal/tui/rendering.go, internal/tui/theme.go, internal/tui/view.go
Centralizes card-based rendering for tool results (diff/read/bash/grep/generic), permission and ask_user modals, hyperlink-safe truncation, rune-width wrapping, adaptive width tiers, and a lime tokenized theme with new tests.
Tools, agent, and schemas
internal/tools/*, internal/agent/*
Tightens ask_user JSON schema constraints, injects OS-specific shell guidance into Bash tool and system prompt, sanitizes tool-call arguments in history, requests a final concise answer after max turns, and adds shell-issue detection heuristics.
Tests & removals
internal/tui/*_test.go, internal/zeroline/*, internal/tools/*_test.go
Adds extensive Lime rendering, flush and width-tier tests; tightens ask_user schema tests; removes legacy zeroline renderer and its tests; updates many tests to use execCmd and new rendering APIs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45–90 minutes

Possibly related PRs

Suggested reviewers

  • Vasanthdev2004
  • anandh8x
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tui-scrollback-and-deep-audit

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
internal/zerogit/zerogit.go (1)

201-211: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistent byte vs. rune limits between ValidateMessage and truncateSubject.

ValidateMessage rejects subjects when len(firstLine) > 72 (bytes), but truncateSubject now counts and truncates on rune boundaries (line 467: runes := []rune(value)). This means:

  • A 72-byte ASCII subject passes validation and truncateSubject leaves it intact.
  • A 60-byte subject with multi-byte Unicode (say, 50 runes) passes validation but truncateSubject still leaves it intact.
  • A generated 72-rune subject with multi-byte chars (say, 100 bytes) would fail validation even though truncateSubject produced it.

The comment at line 465 says the old 72-byte limit "rejected valid non-ASCII subjects," which is correct. But now validation still uses the byte limit while generation uses runes. Consider updating ValidateMessage to count runes (or at least document the intentional divergence if byte-length matters for git tooling).

Also applies to: 464-472

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 201 - 211, ValidateMessage
currently enforces a 72-byte limit using len(firstLine) but truncateSubject
operates on rune boundaries, causing inconsistent behavior for non-ASCII input;
update ValidateMessage to count runes (e.g., use utf8.RuneCountInString or
convert to []rune) and enforce a 72-rune limit on firstLine so it matches
truncateSubject (or alternatively add a clear comment in ValidateMessage
explaining a deliberate byte-based limit), ensuring the change references the
ValidateMessage function and the existing truncateSubject behavior.
🧹 Nitpick comments (2)
internal/imageinput/imageinput.go (1)

62-62: 💤 Low value

Unchecked file.Close return value (linter hint).

For a read-only file, ignoring the Close error is defensible since the data was already read and there's no write buffer to flush. If you want to silence the linter, you can assign to a named return or use defer func() { _ = file.Close() }().

🤖 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/imageinput/imageinput.go` at line 62, The defer currently calls
defer file.Close() with its return value ignored; update the defer in the
function that opens the file (where file.Close() is used) to explicitly handle
or discard the error to satisfy the linter — e.g., replace defer file.Close()
with defer func() { _ = file.Close() }() or capture and log the error from
file.Close() inside that deferred wrapper so the linter is satisfied while
preserving current behavior.

Source: Linters/SAST tools

internal/tui/command_center.go (1)

106-122: ⚡ Quick win

Consider sanitizing additional control characters in card fields.

sanitizeCardField currently strips only \x1f (field separator), \n, and \x00 (prefix marker). User-controlled fields like session.Title can legally contain tabs, carriage returns, or other whitespace/control characters that might disrupt card rendering. Consider normalizing all control characters (e.g., r < 0x20 || r == 0x7f) to spaces, similar to the approach in internal/notify/notify.go lines 134-139.

🤖 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/command_center.go` around lines 106 - 122, sanitizeCardField
currently only strips sessionsCardFieldSep, newline, and NUL but should
normalize all ASCII control characters to avoid rendering issues; update
sanitizeCardField to iterate runes in value and replace any r where r < 0x20 ||
r == 0x7f with a space (preserving other characters), and then also remove the
sessionsCardFieldSep and NUL as before—use the same control-character
normalization approach as in internal/notify/notify.go (lines ~134-139) and keep
the function name sanitizeCardField and constants
sessionsCardFieldSep/sessionsCardsPrefix unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/modelregistry/catalog.go`:
- Line 36: The catalog entry for the deprecated Anthropic model in
anthropicModel("claude-haiku-3.5", "Claude Haiku 3.5",
"claude-3-5-haiku-20241022", ...) incorrectly omits ModelCapabilityVision even
though claude-3-5-haiku-20241022 is multimodal; update the capabilities slice
for that entry to include ModelCapabilityVision (i.e., add ModelCapabilityVision
alongside ModelCapabilityPromptCache) or alternatively add an explicit
comment/documentation in the same entry explaining why this model should be
treated as text-only if that is intentional.

In `@internal/update/update.go`:
- Around line 137-142: The currentVersion fallback in internal/update/update.go
indiscriminately coerces any parse error to "0.0.0"; instead, in the error
handling where currentVersion is set after a failed parse (the block that
currently sets currentVersion = "0.0.0"), restrict the fallback to known dev
markers only (e.g., if the original version string is "dev", "devel", or empty)
and for all other parse errors return or propagate an error (do not silently
coerce). Update the code around the currentVersion assignment and the parsing
error path so that non-dev malformed versions cause the function to return an
error, while dev markers still map to "0.0.0" to preserve --check behavior.

---

Outside diff comments:
In `@internal/zerogit/zerogit.go`:
- Around line 201-211: ValidateMessage currently enforces a 72-byte limit using
len(firstLine) but truncateSubject operates on rune boundaries, causing
inconsistent behavior for non-ASCII input; update ValidateMessage to count runes
(e.g., use utf8.RuneCountInString or convert to []rune) and enforce a 72-rune
limit on firstLine so it matches truncateSubject (or alternatively add a clear
comment in ValidateMessage explaining a deliberate byte-based limit), ensuring
the change references the ValidateMessage function and the existing
truncateSubject behavior.

---

Nitpick comments:
In `@internal/imageinput/imageinput.go`:
- Line 62: The defer currently calls defer file.Close() with its return value
ignored; update the defer in the function that opens the file (where
file.Close() is used) to explicitly handle or discard the error to satisfy the
linter — e.g., replace defer file.Close() with defer func() { _ = file.Close()
}() or capture and log the error from file.Close() inside that deferred wrapper
so the linter is satisfied while preserving current behavior.

In `@internal/tui/command_center.go`:
- Around line 106-122: sanitizeCardField currently only strips
sessionsCardFieldSep, newline, and NUL but should normalize all ASCII control
characters to avoid rendering issues; update sanitizeCardField to iterate runes
in value and replace any r where r < 0x20 || r == 0x7f with a space (preserving
other characters), and then also remove the sessionsCardFieldSep and NUL as
before—use the same control-character normalization approach as in
internal/notify/notify.go (lines ~134-139) and keep the function name
sanitizeCardField and constants sessionsCardFieldSep/sessionsCardsPrefix
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: c11a0439-31f6-41e7-bd0a-2983509154bf

📥 Commits

Reviewing files that changed from the base of the PR and between cf6b1c1 and b144cf9.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (76)
  • .github/workflows/ci.yml
  • .gitignore
  • README.md
  • docs/audit/2026-06-10-deep-audit.md
  • docs/design/ZERO_LIME_TUI_REBUILD_PROMPT.md
  • docs/design/zero_tui_lime.html
  • go.mod
  • internal/agent/system_prompt.go
  • internal/cli/app.go
  • internal/cli/app_test.go
  • internal/cli/cron.go
  • internal/cli/exec.go
  • internal/cli/exec_sessions.go
  • internal/cli/exec_writer.go
  • internal/cli/extensions.go
  • internal/cli/sandbox.go
  • internal/cli/usage.go
  • internal/cli/zeroline.go
  • internal/config/resolver.go
  • internal/config/types.go
  • internal/config/writer.go
  • internal/doctor/doctor.go
  • internal/imageinput/imageinput.go
  • internal/mcp/server.go
  • internal/modelregistry/catalog.go
  • internal/notify/notify.go
  • internal/providers/providerio/providerio.go
  • internal/providers/providerio/retry.go
  • internal/sandbox/safe_command_test.go
  • internal/search/search.go
  • internal/sessions/replay.go
  • internal/tui/ask_user_test.go
  • internal/tui/autocomplete_test.go
  • internal/tui/command_center.go
  • internal/tui/commands.go
  • internal/tui/commands_test.go
  • internal/tui/flush.go
  • internal/tui/flush_test.go
  • internal/tui/image_attach.go
  • internal/tui/image_attach_test.go
  • internal/tui/model.go
  • internal/tui/model_test.go
  • internal/tui/options.go
  • internal/tui/picker.go
  • internal/tui/picker_test.go
  • internal/tui/rendering.go
  • internal/tui/rendering_lime_test.go
  • internal/tui/run.go
  • internal/tui/session.go
  • internal/tui/session_controls.go
  • internal/tui/session_controls_test.go
  • internal/tui/session_test.go
  • internal/tui/spec_mode.go
  • internal/tui/spec_mode_test.go
  • internal/tui/startup.go
  • internal/tui/startup_test.go
  • internal/tui/theme.go
  • internal/tui/transcript.go
  • internal/tui/tui_fixes_test.go
  • internal/tui/view.go
  • internal/tui/width_tiers_test.go
  • internal/tui/zeroline_view.go
  • internal/tui/zeroline_view_test.go
  • internal/update/update.go
  • internal/zerocommands/backend_snapshots.go
  • internal/zerocommands/backend_snapshots_test.go
  • internal/zerocommands/sandbox_snapshots.go
  • internal/zerogit/zerogit.go
  • internal/zerogit/zerogit_test.go
  • internal/zeroline/markdown.go
  • internal/zeroline/markdown_test.go
  • internal/zeroline/render.go
  • internal/zeroline/render_overlay_test.go
  • internal/zeroline/render_test.go
  • internal/zeroline/theme.go
  • internal/zeroline/tokens_test.go
💤 Files with no reviewable changes (14)
  • internal/zeroline/tokens_test.go
  • internal/zeroline/theme.go
  • internal/tui/zeroline_view_test.go
  • internal/zeroline/render_test.go
  • internal/tui/commands_test.go
  • internal/zeroline/markdown_test.go
  • internal/tui/options.go
  • internal/zeroline/render_overlay_test.go
  • internal/cli/zeroline.go
  • internal/tui/zeroline_view.go
  • internal/zeroline/markdown.go
  • internal/cli/app_test.go
  • internal/zeroline/render.go
  • internal/tui/tui_fixes_test.go

Comment thread internal/modelregistry/catalog.go
Comment thread internal/update/update.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/tools/ask_user_test.go (1)

61-72: ⚡ Quick win

Refactor test to check actual field values instead of JSON string matching.

The string-based verification doesn't confirm that minItems and minLength are on the correct fields—it only checks that these strings appear somewhere in the marshaled JSON. A more robust test would directly inspect the schema structure.

♻️ Proposed refactor to check schema fields directly
 func TestAskUserToolAdvertisesNonEmptyQuestionSchema(t *testing.T) {
-	data, err := json.Marshal(NewAskUserTool().Parameters())
-	if err != nil {
-		t.Fatalf("marshal schema: %v", err)
-	}
-	schema := string(data)
-	for _, want := range []string{`"minItems":1`, `"minLength":1`} {
-		if !strings.Contains(schema, want) {
-			t.Fatalf("ask_user schema missing %s: %s", want, schema)
-		}
+	schema := NewAskUserTool().Parameters()
+	
+	// Verify questions array has minItems constraint
+	questions := schema.Properties["questions"]
+	if questions.MinItems == nil || *questions.MinItems != 1 {
+		t.Fatal("questions array should have minItems:1")
+	}
+	
+	// Verify question field has minLength constraint
+	questionField := questions.Items.Properties["question"]
+	if questionField.MinLength == nil || *questionField.MinLength != 1 {
+		t.Fatal("question field should have minLength:1")
 	}
 }
🤖 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_test.go` around lines 61 - 72, The test
TestAskUserToolAdvertisesNonEmptyQuestionSchema currently checks for substrings
in the marshaled JSON; instead unmarshal NewAskUserTool().Parameters() into a
map[string]interface{} (or typed struct), navigate to the schema fields under
"properties" (e.g., locate properties["question"]["minLength"] and
properties["choices"]["minItems"] or similar names the tool uses) and assert
their numeric values equal 1; update assertions to fail with clear messages if
those specific fields are missing or have the wrong value. Ensure you use
NewAskUserTool().Parameters() as the source and reference the exact property
keys when checking minLength and minItems.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/tools/ask_user_test.go`:
- Around line 61-72: The test TestAskUserToolAdvertisesNonEmptyQuestionSchema
currently checks for substrings in the marshaled JSON; instead unmarshal
NewAskUserTool().Parameters() into a map[string]interface{} (or typed struct),
navigate to the schema fields under "properties" (e.g., locate
properties["question"]["minLength"] and properties["choices"]["minItems"] or
similar names the tool uses) and assert their numeric values equal 1; update
assertions to fail with clear messages if those specific fields are missing or
have the wrong value. Ensure you use NewAskUserTool().Parameters() as the source
and reference the exact property keys when checking minLength and minItems.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f2611a7f-e6ce-4bc2-bbdc-76a349ca0bd0

📥 Commits

Reviewing files that changed from the base of the PR and between b144cf9 and e142340.

📒 Files selected for processing (5)
  • internal/tools/ask_user.go
  • internal/tools/ask_user_test.go
  • internal/tools/types.go
  • internal/tui/rendering.go
  • internal/tui/rendering_lime_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/tui/rendering_lime_test.go
  • internal/tui/rendering.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/config/resolver.go (1)

70-76: ⚠️ Potential issue | 🟠 Major

Add resolver tests for sandbox.network allow/deny validation

  • internal/config/resolver_test.go has no sandbox.network/NetworkAllow/NetworkDeny cases and no test asserts the resolver error "invalid sandbox.network %q: expected allow or deny".
  • Existing network: deny assertions are for CLI effective-policy output/snapshots and don’t exercise the resolver’s invalid-value branch.

Add resolver tests for both the success paths (allow/deny) and the fail-loud invalid path (matching the TestResolveSandboxMaxAutonomyInvalidFailsLoud pattern).

🤖 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/config/resolver.go` around lines 70 - 76, Add unit tests in
internal/config/resolver_test.go that exercise the sandbox.network validation:
create resolver test cases that set cfg.Sandbox.Network to "allow" and "deny"
and assert the resolver returns success (no error) and produced ResolvedConfig,
and add a failing case (following the
TestResolveSandboxMaxAutonomyInvalidFailsLoud pattern) that sets
cfg.Sandbox.Network to an invalid value and asserts the resolver returns an
error matching "invalid sandbox.network %q: expected allow or deny". Reference
the cfg.Sandbox.Network field, the sandbox.NetworkAllow/sandbox.NetworkDeny enum
values, and the resolver code path that returns ResolvedConfig{} on error to
locate where to call into the resolver and assert the expected outcomes.
🤖 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.

Outside diff comments:
In `@internal/config/resolver.go`:
- Around line 70-76: Add unit tests in internal/config/resolver_test.go that
exercise the sandbox.network validation: create resolver test cases that set
cfg.Sandbox.Network to "allow" and "deny" and assert the resolver returns
success (no error) and produced ResolvedConfig, and add a failing case
(following the TestResolveSandboxMaxAutonomyInvalidFailsLoud pattern) that sets
cfg.Sandbox.Network to an invalid value and asserts the resolver returns an
error matching "invalid sandbox.network %q: expected allow or deny". Reference
the cfg.Sandbox.Network field, the sandbox.NetworkAllow/sandbox.NetworkDeny enum
values, and the resolver code path that returns ResolvedConfig{} on error to
locate where to call into the resolver and assert the expected outcomes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8acc413e-a509-4a5b-b503-fe2aec3c57ff

📥 Commits

Reviewing files that changed from the base of the PR and between f225d48 and 9b4d0d0.

📒 Files selected for processing (4)
  • internal/agent/loop.go
  • internal/agent/loop_test.go
  • internal/config/resolver.go
  • internal/config/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/agent/loop_test.go

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Zero automated PR review

Verdict: No blockers found

Blockers

  • None found.

Validation

  • [pass] Diff hygiene: git diff --check
  • [pass] Tests: go test ./...
  • [pass] Build: go run ./cmd/zero-release build
  • [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: edf4fb8d8a8f
Changed files (88): .github/workflows/ci.yml, .gitignore, README.md, docs/audit/2026-06-10-deep-audit.md, docs/design/ZERO_LIME_TUI_REBUILD_PROMPT.md, docs/design/zero_tui_lime.html, go.mod, go.sum, internal/agent/guardrails.go, internal/agent/guardrails_test.go, internal/agent/loop.go, internal/agent/loop_test.go, and 76 more

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/guardrails.go`:
- Around line 197-202: The code only resets
state.toolOnlyTurns/state.toolOnlyReminderSent when hasVisibleText is true, so
empty non-tool turns keep the tool-only streak; change the control flow around
the hasToolCalls and hasVisibleText checks so any non-tool-only turn resets the
streak: treat "not (hasToolCalls && !hasVisibleText)" as the reset case and in
that branch set state.toolOnlyTurns = 0 and state.toolOnlyReminderSent = false;
update the block around hasToolCalls/hasVisibleText in guardrails.go (the code
that references state.toolOnlyTurns and state.toolOnlyReminderSent) accordingly.

In `@internal/tools/shell_runtime.go`:
- Around line 20-21: The windowsBashStyleCDPattern is too permissive and matches
cmd.exe's valid "cd /d ..." usage, and the " ls -" substring check in
internal/tools/bash.go is too broad; tighten the regex in
windowsBashStyleCDPattern to reject the special "/d" switch (e.g. use a negative
lookahead: cd\s+/(?!d\b)[a-z0-9_./~-]+ with (?i) for case-insensitivity) and
replace the naive substring check for " ls -" in the pre-execution blocker with
a word-boundary or whitespace-aware regex (e.g. (?i)(^|\s)ls\b\s+-) so only
actual "ls" commands with flags are matched; update references in
internal/tools/shell_runtime.go (windowsBashStyleCDPattern) and
internal/tools/bash.go (the pre-execution check) accordingly.
🪄 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: 38d6661b-8f49-48a5-b649-829831714890

📥 Commits

Reviewing files that changed from the base of the PR and between 9b4d0d0 and 6fe180b.

📒 Files selected for processing (8)
  • internal/agent/guardrails.go
  • internal/agent/guardrails_test.go
  • internal/agent/loop.go
  • internal/tools/bash.go
  • internal/tools/bash_tool_test.go
  • internal/tools/shell_runtime.go
  • internal/tui/rendering.go
  • internal/tui/rendering_lime_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/tools/bash_tool_test.go
  • internal/agent/loop.go
  • internal/tui/rendering.go

Comment thread internal/agent/guardrails.go
Comment thread internal/tools/shell_runtime.go Outdated
@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@gnanam1990
gnanam1990 merged commit 6fb0231 into main Jun 10, 2026
6 checks passed
@Vasanthdev2004
Vasanthdev2004 deleted the tui-scrollback-and-deep-audit branch June 28, 2026 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants