Skip to content

TUI: live right-column (Plan + Code cards) + network/sandbox/provider fixes - #228

Closed
gnanam1990 wants to merge 9 commits into
mainfrom
feat/tui-ide-panels-and-fixes
Closed

TUI: live right-column (Plan + Code cards) + network/sandbox/provider fixes#228
gnanam1990 wants to merge 9 commits into
mainfrom
feat/tui-ide-panels-and-fixes

Conversation

@gnanam1990

@gnanam1990 gnanam1990 commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an IDE-style right column to the full-screen chat and bundles several independent fixes that were sitting on local branches. Fully gated (gofmt, vet, host+linux+windows build, go test -race, staticcheck).

Right-column panel (Plan + Code cards)

During an active run, a compact column floats over the top-right of the chat:

  • Plan card — the live update_plan steps with a spinner, activity word (Planning / Building / Scanning…) and elapsed time. Shown only when the run actually produced a plan, so a trivial "hi" never shows a stale one.
  • Code card — the live unified diff of the file being edited right now: green additions / red removals, with the path and a +adds/−dels tally.

While the Code card is showing a run's diff, inline edit cards in the chat collapse to a one-line record (full diff one click away, and back inline once the run ends) so the change lives in the card, not the middle of the chat. The column is content-sized and reserves the bottom rows, so it can never bury the conversation or the composer. update_plan's tool call/result are kept out of the chat (they live in the panel and /plan).

Network resilience

  • Auto-retry transient provider-stream failures (TLS handshake timeout, i/o timeout, connection reset, etc.) with bounded backoff; the chat shows a "retrying…" note instead of a frozen terminal.
  • After retries are exhausted with no output, surface an actionable message that this is a connectivity problem (check network/VPN or switch providers), not a model failure.

Sandbox

  • Scoped destructive commands (e.g. rm -rf <subdir>) now prompt for approval instead of being hard-blocked, while catastrophic ones stay blocked.

Providers

  • Drop two built-in presets whose endpoints no longer resolve, so a fresh open-source setup only lists providers that actually work.

Testing

  • gofmt -l clean; go vet clean
  • go build host + linux/amd64 + windows/amd64
  • go test ./internal/tui/ ./internal/agent/ ./internal/sandbox/ ./internal/providercatalog/ ./internal/providermodelcatalog/ -race — all pass
  • staticcheck — no new findings (pre-existing main nits unchanged)

Notes

  • Paste-into-wizard is already fixed on main, so it is intentionally not included here.
  • The selectable/clickable permission popup and right-click paste are deferred: main refactored the mouse/selection layer beneath them, so they need a proper re-port (plus interactive testing) rather than a blind merge.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added transparent retries for transient network failures during streaming/provider calls, including exponential backoff and user-facing retry notifications.
    • Added Plan panel to show live run activity and steps.
    • Added Code panel to render real-time unified diffs for edits.
  • Improvements

    • Strengthened destructive command safety with a new catastrophic destructive classification: catastrophic operations stay blocked; scoped destructive deletes now require confirmation.
    • Refined TUI layout and rendering (better chat sizing, floating plan/code overlay behavior, and compact live edit diffs).
  • Chores

    • Removed unsupported providers and updated related model/provider catalog mappings.

…blocking

`rm -rf <subdir>` was classified "destructive" by the AST analyzer and
hard-denied by the sandbox before any permission prompt — so deleting a
directory the agent had created was impossible, forcing an ask_user
workaround. The deny also fired before the unsafe-mode allow, so bypass
mode couldn't run it either.

Split destructive into two tiers:
- "destructive_catastrophic" — the irrecoverable system-level forms the
  catastrophic regex already isolates (rm -rf / or $HOME/~/*, mkfs, dd to a
  raw device, fork bomb, chmod 777 on a system tree, chown -R). These stay a
  hard block, even in unsafe mode.
- everything else flagged destructive (e.g. rm -rf <subdir>, shred <file>) is
  now a Prompt in ask mode — surfaced to the (clickable) permission popup so
  the user can approve — and an Allow in unsafe mode.

risk.go tags the catastrophic set with the new category; engine.go branches
deny / fall-through-to-allow / prompt accordingly. Adds tests pinning the
split (scoped → prompt/allow, catastrophic → deny even in unsafe). Existing
sandbox/agent/cli/tools tests stay green; gofmt/vet/build(host+linux+windows)/
staticcheck clean.
Long tasks looked stuck — the agent would say "I'll build…" then work
silently. Dock a panel on the right of the chat (alt-screen) that shows the
update_plan steps with status glyphs (○ pending, ◐ in-progress, ✓ done) and
an animated header with the current activity — Planning / Building / Scanning
/ Running / Responding / Thinking — plus the spinner and elapsed time. The
activity is derived from the running tool (or streaming state), so the panel
is a live "it's not stuck" signal.

Width is a single source of truth: m.chatAreaWidth() = chatWidth - the panel
column when docked, else chatWidth, so every renderer AND mouse hit-test uses
the same narrowed width and clicks never desync from what's drawn. The panel
shows only when alt-screen + not setup + wide enough (>= 52-col chat) + a plan
or run is live; otherwise chatAreaWidth == chatWidth and every path is byte-
for-byte unchanged. composeWithPlanPanel docks the panel row-by-row in View().

Adds runStartedAt (for elapsed) and plan_panel.go; ~20 chatWidth(m.width)
call-sites moved to m.chatAreaWidth(). Tests cover activation/width-narrowing,
hidden cases (narrow/setup/inline), side-by-side compose, the activity label
(incl. from a running tool), glyphs, and elapsed formatting. gofmt/vet/build
(host+linux+windows)/full -race/staticcheck green.
…ream

A one-off TLS handshake / dial timeout (or connection reset) reaching the
provider killed the whole turn — the user saw a red "provider stream error:
… TLS handshake timeout" and had to resend. These are connection-level blips
where the request never reached the model, so they're safe to retry.

After collecting a turn's stream, if it errored with NO output produced and
the error looks transient (TLS handshake / i/o / dial timeout, connection
reset/refused, unexpected EOF, transient DNS), re-send the SAME request after
an exponential backoff (1s, 2s; up to 2 retries). A failure that already
produced text/tool calls is NOT retried (would duplicate output); a
cancellation, an HTTP status error, or a context-length error is left to the
normal handling (the latter still triggers reactive compaction). The backoff
respects context cancellation.

internal/agent/stream_retry.go classifies the error and backs off;
Options.OnNetworkRetry surfaces each retry, which the TUI shows as a "network
issue reaching the provider — retrying (attempt N)…" row so the pause isn't
silent. Tests: classification (transient vs auth/rate-limit/context), backoff,
retry-to-success, no-retry-on-terminal, no-retry-after-partial-output.
gofmt/vet/build(host+linux+windows)/full -race/staticcheck green.
…nned run

The panel was a full-height "long box" that also showed a STALE plan for
trivial follow-ups (a plain "hi" still displayed the previous task's plan),
and long steps were cut mid-word.

- It now FLOATS over the top-right corner (composeWithPlanPanel overlays only
  its own rows), so the transcript stays full width — no reserved column, no
  full-height box. chatAreaWidth is now just the full chat width.
- It shows ONLY while a run is in flight AND that run actually called
  update_plan (currentRunHasPlan), so "hi" / a delete task never shows a stale
  plan, and it disappears the moment the run finishes.
- Steps truncate with an ellipsis (cutRunesEllipsis) instead of a hard break,
  and the box is wider (40 cols).

Tests rewritten for the gate + overlay (shows only during a planned run; hidden
for trivial/narrow/setup/inline; floats top-right full width). Gate green.
…retries

A network failure that survives every retry (e.g. a provider whose host is
unreachable from the user's network) surfaced as a cryptic "TLS handshake
timeout". Annotate it once retries are exhausted: it's a network/connectivity
problem, not the model — check the connection/VPN or switch providers with
/provider. Test covers the annotation after the retries fail.
The agent re-dumped the entire "Current Plan: 1. [completed] …" on every
update_plan call, cluttering the transcript — and the plan is already shown
live in the right-side plan panel (and on demand via /plan). Skip update_plan's
tool call and result rows from the chat so the transcript stays focused on the
actual work (reads, edits, answers). Test added.
Adds the bottom half of the IDE-style right column: a "Code" card stacked
under the Plan/Progress card, showing the live unified diff of the file the
active run is editing — green additions / red removals, the same palette as
the inline diff cards. The card carries the path and a +adds/−dels tally and
tracks the most recent edit in the run.

While the Code card is showing a run's diff, the inline edit cards in the
chat collapse to a one-line record (full diff is one click away, and returns
inline once the run ends), so the change lives in the card instead of being
duplicated in the middle of the conversation. This is live-region only — the
detailed view (Ctrl+O) and inline scrollback keep the full diffs.

The right column is gated to an active run that actually planned or edited
(never a trivial "hi"), is content-sized, and reserves the bottom chat rows
so a tall plan+diff column can never bury the conversation or the composer.

- code_panel.go: currentEditDiff/diffPath/diffCounts, renderCodeCard,
  codeDiffLines/codeBand, codePanelActive
- plan_panel.go: rightColumnBase gate, renderRightColumn stacks Plan+Code,
  composeWithPlanPanel overlays the column with a bottom reserve
- rendering.go/render_cache.go: compactEdit option collapses edit cards in
  the live region, with a cache key + stability fix so it never serves stale
- tests for the diff parsing, gating, stacking order, collapse, and reserve
…ng endpoint

Removes two built-in openAI-compatible presets whose endpoints no longer
resolve, so a fresh open-source setup only lists providers that actually
work. Curated model lists and the registry mapping are pruned to match.
@github-actions

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: 196b9afd2896
Changed files (27): internal/agent/loop.go, internal/agent/stream_retry.go, internal/agent/stream_retry_test.go, internal/agent/types.go, internal/providercatalog/catalog.go, internal/providercatalog/catalog_test.go, internal/providermodelcatalog/catalog.go, internal/providermodelcatalog/remote.go, internal/providermodelcatalog/remote_test.go, internal/sandbox/destructive_test.go, internal/sandbox/engine.go, internal/sandbox/risk.go, and 15 more

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

@coderabbitai

coderabbitai Bot commented Jun 16, 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: 3fac5b2f-efb8-46f0-acbf-9dc4f218da61

📥 Commits

Reviewing files that changed from the base of the PR and between 196b9af and 809b0d3.

📒 Files selected for processing (9)
  • internal/agent/loop.go
  • internal/agent/stream_retry.go
  • internal/agent/stream_retry_test.go
  • internal/sandbox/destructive_test.go
  • internal/sandbox/risk.go
  • internal/tui/code_panel_test.go
  • internal/tui/plan_panel.go
  • internal/tui/plan_panel_test.go
  • internal/tui/rendering.go
🚧 Files skipped from review as they are similar to previous changes (6)
  • internal/agent/loop.go
  • internal/tui/code_panel_test.go
  • internal/sandbox/destructive_test.go
  • internal/agent/stream_retry.go
  • internal/tui/plan_panel_test.go
  • internal/tui/plan_panel.go

Walkthrough

Adds transient network retry logic with exponential backoff to the agent loop (OnNetworkRetry callback, error annotation). Splits sandbox destructive command evaluation into a catastrophic hard-deny vs. scoped prompt path. Introduces floating Plan and Code right-column panels in the TUI alt-screen view, refactors all layout width calculations to chatAreaWidth(), and suppresses plan tool rows from inline chat. Removes xiaomi-mimo and atomic-chat from provider and model catalogs.

Changes

Agent Transient Network Retry

Layer / File(s) Summary
Retry constants, classifier, and backoff helpers
internal/agent/types.go, internal/agent/stream_retry.go
Options.OnNetworkRetry callback added; maxNetworkRetries, transientNetworkSignals, isTransientNetworkError, streamProducedNoOutput, defaultNetworkRetryBackoff (1s/2s/4s/8s cap), annotateUnreachableProvider, and context-aware sleepWithContext defined.
Agent loop retry wiring
internal/agent/loop.go
CollectOptions prebuilt; loop retries StreamCompletion up to maxNetworkRetries on transient errors with no output, fires OnNetworkRetry per attempt, annotates exhausted transient failure with actionable message.
Retry tests
internal/agent/stream_retry_test.go
Covers: annotation after retry exhaustion (in-stream and initial-call), isTransientNetworkError classification table, backoff durations, instant-backoff helper, retry-then-succeed integration with OnNetworkRetry indexing, no-retry on terminal/auth/rate-limit errors, no-retry after partial text or reasoning/dropped-tool output.

Sandbox Destructive Command Hardening

Layer / File(s) Summary
Catastrophic pattern detection and risk classification
internal/sandbox/risk.go
Adds regex patterns for catastrophic destructive forms (root/home delete, mkfs, dd, fork bombs, recursive chmod/chown, path traversals); matchesCatastrophic and commandHasPathTraversal helpers; logic to upgrade destructive commands to destructive_catastrophic with RiskCritical.
Engine conditional evaluation and test
internal/sandbox/engine.go, internal/sandbox/destructive_test.go
engine.go switches from unconditional deny to: catastrophic commands blocked, scoped destructive prompts unless granted/unsafe. Test asserts ActionPrompt for scoped deletes (site, build/output, *) in ask mode, ActionAllow in unsafe, with destructive risk but not catastrophic. Catastrophic commands (/, $HOME, ~, traversals) yield ActionDeny with ViolationDestructiveCommand even in unsafe.

TUI Floating Plan/Code Panels and Layout Refactor

Layer / File(s) Summary
Model struct and chatAreaWidth() refactor
internal/tui/model.go, internal/tui/spec_mode.go, internal/tui/flush.go, internal/tui/composer.go, internal/tui/command_views.go, internal/tui/onboarding.go, internal/tui/transcript_view.go, internal/tui/model_test.go
runStartedAt field added; View() always applies composeWithPlanPanel; all width calculations migrated from chatWidth() to chatAreaWidth(); launchPrompt and spec-mode paths stamp runStartedAt; model test gains one extra now() call.
Plan panel implementation
internal/tui/plan_panel.go
Constants, gating, activity labeling, plan-fetching, tool-categorization, status-glyph rendering, truncation/formatting helpers, panel rendering, framing, column stacking, and overlay composition logic.
Code panel and inline card collapse
internal/tui/code_panel.go, internal/tui/rendering.go, internal/tui/render_cache.go
codePanelActive, currentEditDiff, diffPath, diffCounts, renderCodeCard, codeDiffLines, codeBand functions; compactEdit flag collapses always-expand diff cards when Code panel active; render cache marks active-run always-expand rows unstable and keys on compactEdit.
Plan row suppression and OnNetworkRetry wiring
internal/tui/rendering.go, internal/tui/model.go, internal/tui/plan_suppress_test.go
rowContext.skip suppresses update_plan tool call/result rows from chat; runAgentWithOptions wraps OnNetworkRetry to emit system transcript row; test verifies suppression behavior.
Plan and Code panel tests
internal/tui/plan_panel_test.go, internal/tui/code_panel_test.go
Full visibility gating, overlay geometry, activity labels, tool-kind mapping, status glyphs, truncation/elapsed formatting, diff extraction/counts, Code card rendering, right-column stacking, overflow protection, and inline card collapse behavior.

Provider Catalog Cleanup

Layer / File(s) Summary
Remove xiaomi-mimo and atomic-chat
internal/providercatalog/catalog.go, internal/providermodelcatalog/catalog.go, internal/providermodelcatalog/remote.go, internal/providercatalog/catalog_test.go, internal/providermodelcatalog/remote_test.go
Two provider descriptors and their curated model blocks removed; ModelsDevProviderID removes xiaomi-mimo mapping, adds nvidia-nim→nvidia; all test fixtures updated for new catalog order and alias tables.

Sequence Diagram(s)

sequenceDiagram
    rect rgba(255, 200, 100, 0.5)
        Note over AgentLoop,Provider: Transient Network Retry Loop
    end
    participant AgentLoop
    participant isTransientNetworkError
    participant sleepWithContext
    participant Provider
    participant OnNetworkRetry

    AgentLoop->>Provider: StreamCompletion (attempt 0)
    Provider-->>AgentLoop: error (no text/tool calls/reasoning)
    AgentLoop->>isTransientNetworkError: classify reason
    isTransientNetworkError-->>AgentLoop: true (if transient signal match)
    loop up to maxNetworkRetries
        AgentLoop->>OnNetworkRetry: attempt N, redacted reason
        AgentLoop->>sleepWithContext: exponential backoff (1s→2s→4s→8s cap)
        AgentLoop->>Provider: StreamCompletion (retry N)
        Provider-->>AgentLoop: success or error
    end
    AgentLoop->>AgentLoop: annotateUnreachableProvider if still transient/no-output
Loading
sequenceDiagram
    rect rgba(100, 180, 255, 0.5)
        Note over TUIView,renderRightColumn: Plan/Code Floating Panel Composition
    end
    participant TUIView as View()
    participant composeWithPlanPanel
    participant renderRightColumn
    participant renderPlanPanel
    participant renderCodeCard
    participant rendering as renderRowMode

    TUIView->>composeWithPlanPanel: chat content string
    composeWithPlanPanel->>renderRightColumn: model state
    renderRightColumn->>renderPlanPanel: currentPlanItems + activity label
    renderPlanPanel-->>renderRightColumn: plan card lines
    renderRightColumn->>renderCodeCard: currentEditDiff path+text
    renderCodeCard-->>renderRightColumn: code card (styled diff)
    renderRightColumn-->>composeWithPlanPanel: stacked column
    composeWithPlanPanel-->>TUIView: chat with top-right overlay (reserved rows protected)
    TUIView->>rendering: renderRowMode (live region, compactEdit=true)
    rendering-->>TUIView: update_plan rows suppressed, diff cards collapsed
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • Gitlawb/zero#125: Overlaps in agent loop's handling of "no visible output" during streaming — main PR adds transient retry gating on no-output, while this PR adds guardrail logic for empty turns.
  • Gitlawb/zero#160: Both PRs modify internal/tui/model.go agent wiring and launch flow — main adds runStartedAt and OnNetworkRetry handler, while this PR refactors queued/detailed transcript launch.
  • Gitlawb/zero#196: Both PRs modify internal/sandbox/risk.go command-risk classification — main adds destructive_catastrophic detection, while this PR wires AST-based analyzer for network/destructive/unparseable categories.

Suggested reviewers

  • Vasanthdev2004
  • anandh8x
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.60% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main components of the PR: right-column UI (Plan + Code cards) and three independent fixes (network resilience, sandbox destructive classification, provider updates).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ 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 feat/tui-ide-panels-and-fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

@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: 5

🧹 Nitpick comments (1)
internal/sandbox/destructive_test.go (1)

45-53: ⚡ Quick win

Add a regression case for rm -rf * to lock the scoped-vs-catastrophic boundary.

Given this PR’s behavior split, add an assertion that rm -rf * in ask mode prompts (and in unsafe mode allows), unless your policy intentionally treats it as catastrophic.

🤖 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/destructive_test.go` around lines 45 - 53, The test file
does not have regression coverage for the `rm -rf *` command to verify the
boundary between scoped-vs-catastrophic command handling. Add test assertions
after the catastrophic commands list that verify `rm -rf *` behaves as a scoped
command: it should prompt the user when run in ask mode and be allowed when run
in unsafe mode. This ensures the command is not incorrectly treated as
catastrophic like the other destructive commands in the catastrophic slice.
🤖 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/loop.go`:
- Around line 162-165: The retry condition in the for loop at the "No output"
retry guard is incomplete and allows retries even when reasoning or dropped
tool-call output has been emitted, which can duplicate surfaced output. Extend
the retry condition to also exclude collected.Reasoning and
collected.DroppedToolCalls (or equivalent field names) by adding checks that
these are empty/zero-valued, similar to how collected.Text and
collected.ToolCalls are checked. This same fix must be applied at both the
primary location (lines 162-165 in the for loop condition) and the sibling
location (lines 182-183, also applies to: 182-183) to ensure the "no output"
contract is properly enforced across all retry guards.
- Around line 150-178: The initial provider.StreamCompletion(ctx, request) call
that produces the stream variable is not protected by retry logic, so transient
network errors during that first call will return immediately without retrying.
Refactor the code to wrap the initial StreamCompletion call and the subsequent
retry loop into a unified retry mechanism that handles transient network
failures consistently for both the initial attempt and any retries, ensuring
that all transient network errors (those with no collected text or tool calls)
are subject to the maxNetworkRetries limit and backoff behavior.

In `@internal/sandbox/risk.go`:
- Around line 105-110: The current code in the `add("destructive_catastrophic",
RiskCritical)` assignment is reusing the broad `matchesDestructive` matcher,
which causes all destructive command matches (including scoped ones like `rm -rf
<subdir>`) to be treated as system-level catastrophic actions that get
hard-denied, rather than being promptable scoped-destructive actions. Create a
separate, stricter matcher function that only matches truly catastrophic
system-level operations (such as `rm -rf /`, `$HOME/~/*`, `mkfs`, `dd` to raw
device, fork bomb, `chmod 777` on system tree, and `chown -R`) and use this new
matcher exclusively for the `destructive_catastrophic` risk assignment, while
keeping the existing broader `matchesDestructive` matcher for the scoped
destructive detection that gets downgraded to prompts in the engine.

In `@internal/tui/plan_panel.go`:
- Around line 35-45: The rightColumnBase() function currently returns true
without verifying whether there is sufficient drawable height available for the
plan panel. Add a height-capacity check to this function to gate activation,
similar to the condition `height - planPanelReserveRows <= 0` that is used
downstream. This check should be added alongside the existing guards to ensure
that rightColumnBase() only returns true when the panel can actually be rendered
on short terminals. The same height-capacity gate should also be applied to any
related activation logic that currently lacks this check.

In `@internal/tui/rendering.go`:
- Around line 966-968: The condition for setting collapsedFooter in the
rendering logic around line 966-968 does not properly force collapsing of edit
tool cards in compact mode when they are short. Currently, the OR condition with
opts.compactEdit only partially addresses this. You need to modify the logic so
that when opts.compactEdit is enabled, edit tools (identified by
toolCardAlwaysExpands(name) returning true) always receive a collapsed footer
via collapsedToolFooter(row.detail), regardless of the body length. Also check
the similar logic around lines 996-1006 and apply the same fix there to ensure
consistent behavior across all locations where edit tools are rendered in
compact mode.

---

Nitpick comments:
In `@internal/sandbox/destructive_test.go`:
- Around line 45-53: The test file does not have regression coverage for the `rm
-rf *` command to verify the boundary between scoped-vs-catastrophic command
handling. Add test assertions after the catastrophic commands list that verify
`rm -rf *` behaves as a scoped command: it should prompt the user when run in
ask mode and be allowed when run in unsafe mode. This ensures the command is not
incorrectly treated as catastrophic like the other destructive commands in the
catastrophic slice.
🪄 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: 6e5b7d74-38c0-4f4b-9a14-0d31a7da812e

📥 Commits

Reviewing files that changed from the base of the PR and between 724167e and 196b9af.

📒 Files selected for processing (27)
  • internal/agent/loop.go
  • internal/agent/stream_retry.go
  • internal/agent/stream_retry_test.go
  • internal/agent/types.go
  • internal/providercatalog/catalog.go
  • internal/providercatalog/catalog_test.go
  • internal/providermodelcatalog/catalog.go
  • internal/providermodelcatalog/remote.go
  • internal/providermodelcatalog/remote_test.go
  • internal/sandbox/destructive_test.go
  • internal/sandbox/engine.go
  • internal/sandbox/risk.go
  • internal/tui/code_panel.go
  • internal/tui/code_panel_test.go
  • internal/tui/command_views.go
  • internal/tui/composer.go
  • internal/tui/flush.go
  • internal/tui/model.go
  • internal/tui/model_test.go
  • internal/tui/onboarding.go
  • internal/tui/plan_panel.go
  • internal/tui/plan_panel_test.go
  • internal/tui/plan_suppress_test.go
  • internal/tui/render_cache.go
  • internal/tui/rendering.go
  • internal/tui/spec_mode.go
  • internal/tui/transcript_view.go
💤 Files with no reviewable changes (4)
  • internal/providercatalog/catalog.go
  • internal/providermodelcatalog/remote.go
  • internal/providermodelcatalog/remote_test.go
  • internal/providermodelcatalog/catalog.go

Comment thread internal/agent/loop.go
Comment thread internal/agent/loop.go
Comment thread internal/sandbox/risk.go Outdated
Comment thread internal/tui/plan_panel.go
Comment thread internal/tui/rendering.go

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One P1 finding: the scoped-destructive allow path in unsafe mode can be bypassed with path-traversal targets (
m -rf ../../) because destructive_catastrophic is only tagged by matchesDestructive (regex-based), which does not recognize ../ as a catastrophic target. The network retry, provider catalog cleanup, and TUI plan/code panels look clean.

Comment thread internal/sandbox/engine.go

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes: one P1 sandbox-escape regression. The scoped-destructive allow path in unsafe mode can be bypassed with path-traversal targets (
m -rf ../../) because destructive_catastrophic is only tagged by matchesDestructive (regex-based), which does not recognize ../ as a catastrophic target. The network retry, provider catalog cleanup, and TUI plan/code panels look clean.

Comment thread internal/sandbox/engine.go
@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Parity check against the reference Droid (D), supplementing the P1 sandbox finding in the review above.

Sandbox (destructive split): D's DroidSandboxManager does NOT split destructive into scoped vs catastrophic tiers. It has a single deny path for destructive commands with an allowAlways persistence layer. Zero's two-tier split (catastrophic = hard block, scoped = prompt/allow) is a Zero-specific enhancement over D. The P1 path-traversal escape (rm -rf ../../ misclassified as scoped) is the gap to fix. D avoids this class of issue because it denies ALL destructive commands uniformly.

Network retry: D's retry lives at the LLM client layer (src/utils/retryPolicy.ts -> getRetryConfig) with error-type-specific delays: throttling (5s-30s with server retry-after), capacity/503 (500ms + provider rotation), timeout (3s base, 1.5x, 15s cap), and default jittered backoff. Zero's stream_retry.go retries at the agent-loop level (wrapping the provider stream). Key difference: D retries at the HTTP client level (before any streaming output is produced), so there is no risk of duplicating partial output. Zero retries only when no output was produced (confirmed correct in review), which avoids the same risk. However, D's per-error-type backoff is more sophisticated than a single backoff curve. Consider adding throttling-specific delays with Retry-After header support.

Plan panel: D uses PinnedTodoDisplay (a left-bordered box showing todo items with scroll anchoring) rendered inline in the chat, not a docked right column. Zero's right-docked panel is a different UX choice. D's approach keeps the chat width stable; Zero's reserves columns from the chat area. Both are valid.

Code panel: D does not have a live diff panel. It renders diffs inline in the chat via DiffRenderer/UnifiedDiffRenderer. Zero's floating Code card that collapses inline edits is a Zero innovation not present in D.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Fix guide: path-traversal escape in the scoped-destructive split

The case granted: fall-through at engine.go:307 lets rm -rf ../../ run in unsafe mode because destructive_catastrophic is only tagged inside if matchesDestructive(command) in risk.go. The regex-based matchesDestructive matches targets /, $HOME, ~, * but NOT ../, so the command gets destructive from the AST analyzer (hasRecursiveForce) but never destructive_catastrophic. The requestPaths path-escape check only inspects path/file/cwd arg keys, not command, so nothing else catches it.

Fix (two parts)

1. Tag path-escape rm as catastrophic in risk.go

After the existing AST block where analysis.Destructive adds destructive, add a check that upgrades to destructive_catastrophic when the AST sees a recursive rm whose target resolves outside the workspace:

// In classifyWithScope, after the `if analysis.Destructive { add("destructive", RiskCritical) }` block:

// A recursive rm targeting a parent path (../, absolute path outside the
// workspace) is catastrophic — it escapes the workspace boundary.
if analysis.Destructive && commandIsRecursiveRmEscape(command, request.WorkspaceRoot) {
    add("destructive_catastrophic", RiskCritical)
}

Add a helper that parses the rm target and checks for traversal:

// commandIsRecursiveRmEscape reports whether command is a recursive rm whose
// target resolves above the workspace root (e.g. rm -rf ../../, rm -rf /etc).
// It re-uses the AST walk so glob/expansion doesn't blind-side the check.
func commandIsRecursiveRmEscape(command string, workspaceRoot string) bool {
    parsed, err := syntax.Parse(strings.NewReader(command), "", syntax.PosixStrict)
    if err != nil {
        return false // unparseable — let the unparseable_command category handle it
    }
    for _, stmt := range parsed.Stmts {
        call, ok := stmt.Call.(*syntax.Call)
        if !ok || len(call.Args) == 0 {
            continue
        }
        prog := wordText(call.Args[0])
        if prog != "rm" {
            continue
        }
        if !hasRecursiveForce(call.Args[1:]) {
            continue
        }
        for _, arg := range call.Args[1:] {
            text := wordText(arg)
            if strings.HasPrefix(text, "-") || text == "--" {
                continue
            }
            // Target is a relative parent path or an absolute path outside root.
            cleaned := filepath.ToSlash(filepath.Clean(text))
            if cleaned == ".." || strings.HasPrefix(cleaned, "../") {
                return true
            }
            if filepath.IsAbs(cleaned) && workspaceRoot != "" {
                if abs, err := filepath.Abs(workspaceRoot); err == nil {
                    if !strings.HasPrefix(cleaned, abs+"/") && cleaned != abs {
                        return true
                    }
                }
            }
        }
    }
    return false
}

syntax is from mvdan.cc/sh/v3/syntax (already a dep). wordText and hasRecursiveForce already exist in analyzer.go; expose them or duplicate the small helpers.

2. Add test cases to destructive_test.go

Append to the catastrophic slice (or add a dedicated sub-test):

// Path-traversal targets are catastrophic — never allowed even in unsafe mode.
escapeCases := []string{
    "rm -rf ../../",
    "rm -rf ..",
    "rm -rf ../../../etc",
    "rm -rf /etc",
}
for _, command := range escapeCases {
    d := engine.Evaluate(context.Background(), shellReq(command, PermissionUnsafe))
    if d.Action != ActionDeny {
        t.Fatalf("escape %q in unsafe = %#v, want ActionDeny", command, d)
    }
    if !HasRiskCategory(d.Risk, "destructive_catastrophic") {
        t.Fatalf("escape %q categories = %v, want destructive_catastrophic", command, d.Risk.Categories)
    }
}

Why this approach

  • Keeps the scoped tier working: rm -rf site and rm -rf build/output still get only destructive (no ../ and not absolute-outside-root), so they stay prompt/allow as the PR intends.
  • Catastrophic stays a hard block: rm -rf ../../, rm -rf .., rm -rf /etc all get destructive_catastrophic and hit the case HasRiskCategory(risk, "destructive_catastrophic"): deny branch, even in unsafe mode.
  • Doesn't touch requestPaths: the command arg is still not inspected by the path-escape check (which is correct — requestPaths is for file-arg tools, not shell commands). The new check lives in the command-analysis path where destructive rm targets are already parsed.
  • No false positives on rm -rf ./subdir: filepath.Clean("./subdir") is "subdir" (no ../ prefix), so it stays scoped.

Verify

gofmt -w internal/sandbox/risk.go internal/sandbox/destructive_test.go
go vet ./internal/sandbox/
go test ./internal/sandbox/ -run TestScopedDestructive -race -v
go test ./internal/sandbox/ -race
go build ./...

@gnanam1990
gnanam1990 marked this pull request as draft June 17, 2026 05:54
@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Improvement notes for future work, supplementing the P1 sandbox finding in the review above.

Sandbox (destructive split): A simpler sandbox model is to deny ALL destructive commands uniformly and let the user allow them via a persistent settings layer (allow-always list). That single-deny approach avoids the need for a scoped-vs-catastrophic tier and the path-traversal gap that comes with it. The two-tier split in this PR is more nuanced, but it introduces the misclassification risk that the P1 finding calls out.

Network retry: A more sophisticated retry layer uses per-error-type backoff: throttling errors (5s-30s, respect server Retry-After header), provider capacity/503 errors (500ms + provider rotation), timeout errors (3s base, 1.5x growth, 15s cap), and a default jittered backoff for everything else. Right now stream_retry.go uses a single backoff curve. The good news is the loop-level retry correctly only fires when no output was produced, so partial-output duplication isn't a risk. Consider adding Retry-After support for throttling responses and shorter delays on capacity errors so provider rotation kicks in faster.

Plan panel: An alternative UI is to keep the plan inline in the chat as a left-bordered box with scroll anchoring (using update_plan steps). That keeps the chat width stable — no column reservation, no chat-width narrowing. Right now the right-docked panel takes columns from the chat area, which is visible at narrow terminal widths. Both approaches are valid; the inline approach is simpler to maintain and degrades more gracefully on small terminals.

Code panel: A live floating diff card that collapses inline edits is an interesting UX choice not commonly seen. Worth keeping, but verify it works well on long-running sessions where the diff card might grow large.

…els)

Agent loop (internal/agent):
- Route INITIAL StreamCompletion transient failures through the network-retry
  flow too — a TLS/dial timeout on the call itself was returned immediately,
  skipping retries (only the collect phase was covered). Annotate after retries.
- Exclude reasoning blocks and dropped tool-call signals from the "no output"
  retry/annotation guard via a shared streamProducedNoOutput helper, so a stream
  that surfaced reasoning is not re-sent (which would duplicate output).

Sandbox risk/engine (internal/sandbox):
- Split catastrophic detection from the broad destructive matcher. `rm -rf *`
  (and other workspace-local deletes) are now destructive-but-promptable instead
  of hard-denied; only system-level targets (/, $HOME, ~), mkfs/dd/fork-bomb/
  chmod-system/chown-R stay catastrophic.
- Close the path-traversal escape: a destructive command containing a `..`
  component (e.g. `rm -rf ../../`) is tagged destructive_catastrophic via both a
  regex target and an AST + traversal backstop, so it stays denied even in unsafe
  mode rather than escaping the workspace.

TUI (internal/tui):
- Gate rightColumnBase by drawable height (height > planPanelReserveRows) so
  activation matches composeWithPlanPanel, which overlays zero rows on a short
  terminal.
- Force-collapse short edit diffs in compact-edit mode so a live Code card does
  not duplicate the diff inline.

Tests cover each: initial-call retry + exhaustion, reasoning/dropped no-retry,
the scoped/catastrophic/traversal split, the height gate, and the short-diff
compact collapse.
@gnanam1990 gnanam1990 closed this Jun 17, 2026
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