feat(tui): full-auto permission mode, and classify local dev servers separately

[Vasanthdev2004]

Four commits lifted out of #808, where they did not belong. @gnanam1990 blocked that PR on scope and he was right: #808 rewrites Windows write-jail semantics, and burying a permission-mode rename and a network reclassification inside it makes the security-relevant core harder to review on its own.

Cherry-picked onto current main and verified there, not just moved.

The permission mode gains a third position

unsafe mode existed and worked, but was reachable only through --skip-permissions-unsafe at launch, so nothing in the TUI told you it was there. It is now the third position in the shift+tab cycle.

What it is NOT is reachable by repeating a navigation key. shift+tab only ever OFFERS it; committing takes ctrl+g while that offer is live. Press shift+tab again instead and the offer is declined and the cycle continues, so every mode stays reachable with shift+tab alone.

The offer lives on the model across keypresses, which is the part worth care: a stale flag would turn a later innocent ctrl+g into a silent drop into full-auto. So it is cleared unconditionally at the top of the key handler and re-armed only by the shift+tab branch. Forgetting a path cancels the offer rather than leaving it live, which is the harmless direction to be wrong in. The tests drive the real handler with eight different cancelling keys and assert the consequence, not just the flag.

An earlier draft had the second shift+tab commit full-auto, which silently removed ask -> auto from the cycle. There is now a test that walks the whole loop and asserts every mode stays reachable, so that cannot come back unnoticed.

unsafe becomes full-auto

"unsafe" was a judgement rather than a description and sat oddly beside auto and ask. full-auto says what the mode does and reads as the obvious third step.

Deliberately NOT "unrestricted", which was the other candidate and is factually wrong: this mode turns off permission PROMPTS. The OS sandbox stays on and the write jail still holds, so a label implying nothing restrains the agent would be wrong in the dangerous direction.

The on-disk value changes and "unsafe" is accepted permanently as an alias rather than migrated, since that value lives in user configs and scripts. --full-auto is the new flag; --skip-permissions-unsafe keeps working as the deprecated spelling. The old Go constants are kept as deprecated aliases too: deleting a constant that in-flight work is using makes the merge someone else's problem, which is exactly how I broke CI on #808 before adding them back.

The colour does not change. The name can be calm; the indicator should not be.

Local dev servers are classified separately, and still need network approval

Narrowed from what this section used to claim. An earlier draft of this branch did remove the network approval for serving commands, the follow-up put it back, and the description kept describing the version that no longer ships.

What ships is the classification foundation. AnalysisResult carries LocalServer separately, so the distinction between binding a port and reaching out is preserved rather than collapsed. The analyzer already had a localServerPrograms map beside networkPrograms and commandUsesNetwork returned true for it anyway, so the distinction had been drawn and never acted on.

What does NOT ship is dropping the approval. Network is still set for every recognized LocalServer, on purpose, and TestServingStillRequiresNetworkApproval pins it. Two reasons:

• Nothing consumes LocalServer yet. No policy or runner reads it, so classifying a serving command as local-only would not grant it a scoped host listener, it would only remove the approval it used to get. The command then runs under the default deny profile, a network namespace on Linux and deny-network on macOS, so python -m http.server and vite would start without a prompt and be unable to serve anything to the operator.
• It is unsound for the package managers. npm run dev is matched by SCRIPT NAME, and the repository decides what dev and predev actually do; either can fetch before a port is bound. On Windows the approval gate IS the egress protection, so inferring "no egress" from a name there lets that egress run unprompted.

So python -m http.server, Vite, and the package-manager serving commands remain approval-gated exactly as before. Coverage is consistent across npm, pnpm, yarn and bun, direct and behind run, because next dev and npm run dev are the same intent and classifying one differently would be arbitrary.

The unparseable fallback is kept a superset of this for the same reason: the POSIX parser rejects Windows shell syntax the invoked shell accepts, and a serving command written in a spelling it cannot read must not lose the gate its parseable form receives.

A host-listener capability is the follow-up this foundation is for: classification, an explicit policy decision, and platform runner support for reachable binding while outbound egress stays controlled. Until that exists end to end, the gate stays.

Verification

Cherry-picked onto 7f39a630 and checked there: go build ./..., go vet, GOOS=linux go vet and gofmt all clean. Test failures are only the ones already failing on main and on this machine: TestEagerToolSchemaTokenBudget (inherited, #877 fixes it), TestAltScreenTranscriptScrollKeepsFooterFixed, and three doctor connectivity probes that need network.

#808 keeps zero sandbox exec, which @gnanam1990 called borderline, since it is the vehicle for exercising the principal path on a clean elevated machine.

Summary by CodeRabbit

• New Features

  • Added the --full-auto permission mode, offered with Shift+Tab and confirmed with Ctrl+G.
  • Added an on-demand run-details overlay and searchable, grouped theme picker with previews.
  • Updated prompts, help text, shell completions, status messages, and event output to use “full-auto” terminology.
  • Improved sandbox classification for local development servers and network activity.

• Bug Fixes

  • Preserved compatibility with legacy unsafe settings and flags.
  • Improved permission cancellation, automatic handling, sandbox retries, warnings, and workspace safeguards.

[github-actions]

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: 7be892028c0a
Changed files (48): internal/acp/agent.go, internal/acp/agent_test.go, internal/agent/compaction_test.go, internal/agent/loop.go, internal/agent/loop_test.go, internal/agent/types.go, internal/cli/app.go, internal/cli/app_test.go, internal/cli/completions.go, internal/cli/completions_test.go, internal/cli/exec.go, internal/cli/exec_full_auto_parity_test.go, and 36 more

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

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The change makes full-auto the canonical permission mode, preserves legacy unsafe compatibility, updates runtime and CLI handling, adds TUI confirmation, and separates local-server detection from network classification.

Changes

Full-auto permission mode

Layer / File(s)	Summary
Permission mode contract and runtime internal/agent/..., internal/acp/..., internal/specialist/..., internal/swarm/...	Defines and normalizes PermissionModeFullAuto, preserves legacy unsafe input, updates runtime checks and ranking, and rejects full-auto through ACP.
CLI full-auto entry points and output internal/cli/...	Accepts --full-auto, retains --skip-permissions-unsafe as a deprecated alias, and updates resolution, help, warnings, completions, and tests.
Sandbox classification and enforcement internal/sandbox/..., internal/tools/...	Adds AnalysisResult.LocalServer, separates serving commands from network activity, and updates automatic approval checks.
TUI full-auto confirmation internal/tui/...	Requires immediate Ctrl+G confirmation, cancels stale offers on input, reserves the keybinding, and adds lifecycle and filename-validation tests.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant TUIModel
  participant TUIView
  User->>TUIModel: press Shift+Tab
  TUIModel->>TUIView: show full-auto confirmation offer
  User->>TUIModel: press Ctrl+G immediately
  TUIModel->>TUIView: display committed full-auto mode

Loading

Suggested reviewers: euxaristia

Merge Risk: 🟡 Moderate · up to 4a77c

Framework builds may access the network without approval, and full-auto launches can ignore an explicitly selected theme. These regressions should be addressed before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 116 functions across 37 files.	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 clearly summarizes the two primary changes: adding the full-auto permission mode and separating local development server classification.
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.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch split/permission-mode-and-classifier

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/acp/agent.go (1)

358-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use full-auto in user-visible diagnostics and comments.

The changed branches now use the canonical mode, but they still expose the deprecated unsafe name.

• internal/acp/agent.go#L358-L362: describe full-auto as the canonical mode and --skip-permissions-unsafe as its deprecated alias.
• internal/agent/loop.go#L1488-L1491: return full-auto permission mode permits unsandboxed retry.
• internal/agent/loop.go#L1542-L1544: return the equivalent full-auto wording for network retry.
• internal/cli/app.go#L291-L292: update branch comments and generic errors to use canonical terminology.
• internal/cli/app.go#L334-L334: direct one-shot users to zero exec --full-auto, not the deprecated spelling.

As per coding guidelines, help text and comments must match shipped 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/acp/agent.go` around lines 358 - 362, Update the permission-mode
diagnostics and comments to use canonical “full-auto” terminology: in
internal/acp/agent.go:358-362, describe full-auto as canonical and
--skip-permissions-unsafe as its deprecated alias; update the unsandboxed and
network retry messages in internal/agent/loop.go:1488-1491 and 1542-1544; revise
branch comments and generic errors in internal/cli/app.go:291-292; and direct
one-shot users to “zero exec --full-auto” in internal/cli/app.go:334.

Source: Coding guidelines

🤖 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/types.go`:
- Around line 25-36: Preserve the source-compatible PermissionModeUnsafe alias
in internal/agent/types.go, and add or reuse a normalization helper at the agent
boundary that maps raw PermissionMode("unsafe") to PermissionModeFullAuto. In
internal/agent/loop.go lines 1168-1171, normalize Options.PermissionMode before
permission checks and tool execution so executeToolCall grants full-auto
behavior. Add a regression test in internal/agent/loop_test.go lines 2723-2749
covering PermissionMode("unsafe") and verifying permission is granted.

In `@internal/cli/exec.go`:
- Around line 575-592: Make the full-auto warning in the permission-mode
handling path accurate for both --full-auto and --skip-permissions-unsafe.
Replace the options.skipPermissionsUnsafe-specific reason with neutral wording
such as “full-auto mode was requested,” or preserve the original parsed flag
spelling and use it; update TestRunExecUnsafeTextModeWarns if source-specific
wording remains.

In `@internal/sandbox/analyzer.go`:
- Around line 338-347: Update commandRunsLocalServer so multi-purpose programs
such as npm, pnpm, yarn, bun, and other server-capable runners return true only
when their arguments select a server subcommand; preserve unconditional
detection for direct server binaries such as http-server. Add regression tests
covering non-server commands including next build, nuxt generate, astro check,
and vite build, ensuring they do not set LocalServer.

In `@internal/specialist/exec.go`:
- Line 158: Update specialistAutonomy to treat both "full-auto" and the legacy
"unsafe" value as the high-autonomy mode, while preserving the existing
low-autonomy result for other values. Add regression coverage in exec_test.go
verifying both inputs produce the expected autonomy.

In `@internal/tui/model.go`:
- Line 4710: Update the shell-escape denial notice in internal/tui/model.go at
lines 4710-4710 to use canonical full-auto wording and advertise the full-auto
entry point instead of unsafe mode or its deprecated flag. Update the
corresponding assertion in internal/tui/tui_fixes_test.go at lines 37-37 to
expect the canonical full-auto terminology.
- Around line 1361-1369: The unsafeArmed offer is cleared only for keypresses,
allowing paste or mouse input to leave it active. Update the input handling in
internal/tui/model.go around the unsafeArmed reset so tea.PasteMsg and
tea.MouseMsg also disarm the offer before processing, and add regression cases
in internal/tui/permission_mode_arm_test.go:59-81 proving Ctrl+G remains inert
after each input type.

In `@internal/tui/permission_mode_arm_test.go`:
- Around line 21-24: Replace the direct model literals in armedModel and the
standalone confirmation and Shift+Tab tests with newModel-based initialization,
preserving the explicit permissionMode configuration. Ensure every model passed
to pressKey has newModel’s now callback initialized.

---

Outside diff comments:
In `@internal/acp/agent.go`:
- Around line 358-362: Update the permission-mode diagnostics and comments to
use canonical “full-auto” terminology: in internal/acp/agent.go:358-362,
describe full-auto as canonical and --skip-permissions-unsafe as its deprecated
alias; update the unsandboxed and network retry messages in
internal/agent/loop.go:1488-1491 and 1542-1544; revise branch comments and
generic errors in internal/cli/app.go:291-292; and direct one-shot users to
“zero exec --full-auto” in internal/cli/app.go:334.

🪄 Autofix

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

Run ID: f78fbed2-f8c9-4cca-9fda-535498941eb1

📥 Commits

Reviewing files that changed from the base of the PR and between 7f39a63 and 0311ffb.

📒 Files selected for processing (29)

• internal/acp/agent.go
• internal/acp/agent_test.go
• internal/agent/compaction_test.go
• internal/agent/loop.go
• internal/agent/loop_test.go
• internal/agent/types.go
• internal/cli/app.go
• internal/cli/app_test.go
• internal/cli/exec.go
• internal/cli/exec_parse.go
• internal/cli/exec_test.go
• internal/cli/exec_tools.go
• internal/cli/trust_e2e_test.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_test.go
• internal/sandbox/engine.go
• internal/sandbox/engine_test.go
• internal/sandbox/normalize.go
• internal/sandbox/risk.go
• internal/sandbox/types.go
• internal/specialist/exec.go
• internal/tools/bash_tool_test.go
• internal/tools/registry_test.go
• internal/tui/keybindings.go
• internal/tui/model.go
• internal/tui/model_test.go
• internal/tui/permission_mode_arm_test.go
• internal/tui/tui_fixes_test.go
• internal/tui/view.go

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Initialize permission-mode test models with newModel.

pressKey calls model.updateModel, which calls m.now() at internal/tui/model.go Line 1351. The direct model literals leave now nil, so these tests panic on their first keypress.

Proposed fix

 func armedModel(t *testing.T) model {
 	t.Helper()
-	m := model{permissionMode: agent.PermissionModeAsk}
+	m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAsk})

Apply the same constructor pattern to the direct model literals in the standalone confirmation and Shift+Tab tests.

Also applies to: 34-41, 86-103

🤖 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/permission_mode_arm_test.go` around lines 21 - 24, Replace the
direct model literals in armedModel and the standalone confirmation and
Shift+Tab tests with newModel-based initialization, preserving the explicit
permissionMode configuration. Ensure every model passed to pressKey has
newModel’s now callback initialized.

[Vasanthdev2004]

@gnanam1990 @anandh8x @jatmn this is the split you asked for on #808. All checks green.

Small enough to read in one sitting, which was the point: +487/-128 across 29 files, versus the +9077 it was hiding inside.

Three independent changes, and they are genuinely independent, so feel free to take a position on one and ignore the others.

The parts I would look at hardest, because they are where I would expect to be wrong:

The full-auto offer state lives on the model across keypresses. A stale flag would turn a later innocent ctrl+g into a silent drop into a mode with no permission prompts. I inverted it so the flag is cleared unconditionally at the top of the key handler and only shift+tab re-arms, which means forgetting a path cancels the offer rather than leaving it live. The tests drive the real handler with eight cancelling keys and assert the consequence rather than the flag, but if you can find a keypress path that skips that clear, that is the bug worth finding.

The network reclassification loosens what counts as egress, which is exactly where a sandbox quietly weakens. @gnanam1990 already checked this on #808 and confirmed Network and LocalServer are set by independent checks so a genuine fetcher still gets Network = true. A second opinion on the boundary is still worth having: I claim binding is not egress, not that dev tooling is inert, and npm run dev may well install first.

npx http-server deliberately stays classified as network, because npx downloads the package when it is missing. My first pass flipped it and a test caught me, which is why the bind-versus-fetch pairs are pinned side by side in the table now.

On the rename: the old Go constants and the old on-disk value are both kept as permanent aliases rather than migrated. That is not politeness, it is the fix for how I broke CI on #808: deleting a constant that in-flight work is using makes the merge someone else's problem.

What this does not change: full-auto skips permission PROMPTS. The OS sandbox stays on and the write jail still holds. That is why it is not called "unrestricted", and why the warning text names prompts specifically rather than implying something broader.

Cherry-picked onto 7f39a630 and verified there rather than moved, so this does not inherit #808's base.

[gnanam1990]

Approving at 0311ffb9. This is the split I asked for on #808 — the permission-mode rename and the dev-server network reclassification lifted out of the Windows-principals PR into their own change. Thanks for doing it.

The permission-mode change is carefully built, and I checked the parts that would bite

The wire value changed, and the backward-compat is in the right place. PermissionModeUnsafe = "unsafe" became PermissionModeFullAuto = "full-auto", with PermissionModeUnsafe now a compile-time alias. The alias only keeps Go code compiling — it does nothing for a persisted or transmitted "unsafe" string, which no longer equals the constant. What actually saves that is NormalizePermissionMode mapping both "full-auto" and legacy "unsafe" to PermissionFullAuto, and it's called at engine.go:315 — inside Decide, right before risk classification and the allow/deny. That is the enforcement chokepoint, so however the mode string arrived (CLI, ACP, session replay, config), it's normalized at the one point that governs enforcement. A restored "unsafe" session is enforced as full-auto, correctly.

And the failure direction is safe. Anything NormalizePermissionMode doesn't recognize falls to PermissionModeAuto, and the TUI's advancePermissionMode folds unknown modes to Ask — both stricter. So a missed normalization anywhere is a downgrade, never an escalation. The legacyFullAutoPermissionMode comment makes the "visible downgrade beats silent one" reasoning explicit, which is the right call for a value that lives in user configs.

The arm/confirm gate genuinely prevents accidental full-auto. Full-auto isn't reachable by cycling — shift+tab from Ask arms an offer, and confirmUnsafePermissionMode commits only from a live offer that any other keypress clears. So the path is shift+tab → shift+tab (arm) → ctrl+g, and pressing shift+tab again while armed goes to Auto and drops the offer rather than confirming. I mutation-tested this rather than trust the read: removing the if !offered guard so confirm ignores the offer fails TestConfirmDoesNothingWithoutALiveOffer immediately (confirm with no offer from auto = full-auto, want unchanged). The security property is pinned.

CLI compat holds: --skip-permissions-unsafe and --full-auto are both accepted (exec_parse.go:23), and --auto resolves to PermissionModeFullAuto. Remaining "unsafe" strings in the tree are either code comments or import "unsafe", not user-facing.

The network change is the same one I verified on #808

Network and LocalServer are still set by independent checks (analyzer.go:193, 196), so a command that genuinely fetches keeps Network = true even if it also binds a port — no egress escape. The commands moved out of egress (npm run dev, vite, next dev, http.server, start/serve/dev/preview) only bind, and LocalServer preserves the inbound signal rather than dropping it. Correct and fail-closed, as before.

Verified

• go build ./... and GOOS=windows go build ./... clean.
• internal/tui, internal/sandbox, internal/agent suites green; the arm gate is mutation-tested as above.
• The two internal/cli doctor failures (TestRunDoctorConnectivityProbesProvider, …FormatsRedactedProviderDiagnostics) are pre-existing — they fail on untouched main and this PR touches no doctor/observability/connectivity file.
• Feature runs against the real binary: macOS 7/7, Linux 7/7 (file writes, nested paths, sandbox refusal, --add-dir grants, control characters, specialist children, exec_command). Windows is green on the PR's own CI (Smoke (windows-latest) pass, 9m).

One scope note, not blocking

The PR is still two unrelated concerns in one — a TUI permission-mode rename and a sandbox command-classifier change; the title carries the "and." It's a large improvement over #808's four-in-one, and both halves are correct, so I'm not going to hold it up. But they'd be independently revertable as two PRs, and the network reclassification in particular is the kind of security-adjacent change that's easier to reason about — and to bisect later — on its own. Your call; flagging it rather than requesting it.

Clean work, and the arm gate is a genuinely thoughtful bit of security-UX.

[Vasanthdev2004]

Pushed fixes for the review. Two of the findings turned out to be bigger than they looked, and one of them was a hole in my own tests.

The legacy-value finding was right, and wider than reported. The Go alias only covers callers that name the constant. It does not cover a mode that travels as a string, and three places do:

• swarm.permissionRank did not rank "full-auto", so it scored 0, the strictest tier. A full-auto parent was clamping its members harder than an ask parent.
• specialistAutonomy did not match it either, so a full-auto parent's specialists dropped to read-only low.
• the agent loop compares Options.PermissionMode directly, as reported.

All three fail safe, which is why nothing broke loudly. Added agent.NormalizePermissionMode and applied it at the one point the loop reads the mode off Options, rather than at the individual comparison sites, so the next site added is covered too. The swarm and specialist constants now know both spellings. Their existing tables only listed "unsafe", which is exactly how the rename got through review in the first place.

On the test-panic finding: correct, and worse than described. permission_mode_arm_test.go was never compiled at all. Go read the trailing _arm as a GOARCH constraint and excluded the file from every amd64 build, so those five tests covering the shift+tab full-auto offer had never run, here or in CI. go list confirms it: IgnoredGoFiles: [permission_mode_arm_test.go]. Two of them panicked the moment they were made to run.

Renamed the file, fixed the model construction, and added a repository-wide test that fails on any _test.go ending in a GOOS/GOARCH token without an explicit build tag. It mutation-catches the original filename.

With those tests actually running, the paste/mouse gap was real. Probed it before fixing: armed the offer, sent a PasteMsg, then ctrl+g, and got full-auto. Paste and deliberate mouse actions now cancel the offer. Passive motion deliberately does not, because terminals stream motion while tracking is on and cancelling on a twitch would make the confirm key unreachable. There is a test pinning that exception so it does not get "fixed" later.

commandRunsLocalServer: agreed. Fixed to require a serving subcommand. While testing I broke vite --host 127.0.0.1, because firstSubcommand skips flags but not the values they consume, so it resolved to 127.0.0.1. Unrecognized tokens now fall back to bare-invocation behaviour, and that case is in the test.

One I am skipping: the internal/acp/agent.go terminology note. ACP rejects both spellings already, full-auto explicitly and unsafe via the unknown-mode default, so there is no behaviour change available there, only different error text on a path that is refused either way.

Wording fixes for the retry reasons, the exec warning and the shell-escape notice are in. The exec warning now names both flag spellings, since one bool backs both and pointing someone at a flag they did not type is its own small bug.

Verification: go build, go vet, gofmt -l clean. sandbox, swarm, specialist, acp fully green. Six failures in agent, tui and cli reproduce identically with my changes stashed, so they are pre-existing and local to Windows; TestEagerToolSchemaTokenBudget is the one #877 addresses.

Sorry about the dismissed approval @gnanam1990, the push was unavoidable. Re-requesting.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/tui/model.go`:
- Around line 4716-4719: Normalize the legacy "unsafe" permission value before
assigning m.permissionMode in newModel, using the same normalization behavior as
agent.Run so shell escapes are allowed in the resulting full-auto mode. Add a
regression test in internal/tui/tui_fixes_test.go covering !echo hi with the raw
legacy permission value.

🪄 Autofix

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

Run ID: 5af63042-9553-419a-b49d-aedcc6c2823b

📥 Commits

Reviewing files that changed from the base of the PR and between 0311ffb and 893545d.

📒 Files selected for processing (14)

• internal/agent/loop.go
• internal/agent/types.go
• internal/cli/app.go
• internal/cli/exec.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_local_server_test.go
• internal/specialist/exec.go
• internal/specialist/exec_test.go
• internal/swarm/permission_rank_test.go
• internal/swarm/team.go
• internal/tui/model.go
• internal/tui/permission_mode_offer_filename_test.go
• internal/tui/permission_mode_offer_test.go
• internal/tui/tui_fixes_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

• internal/agent/loop.go
• internal/cli/app.go
• internal/sandbox/analyzer.go
• internal/cli/exec.go

[Vasanthdev2004]

Rebased onto main. Mergeable again.

One conflict, and it needed both sides rather than a pick. #884 added syncPeerIdentity() after the shift+tab mode change, because the peer record carries the permission class. This branch had replaced that line with the arm/confirm logic and added a ctrl+g case, so there are now TWO places the mode changes. Both resync.

The other thing the rebase surfaced is worth flagging on its own: permission_mode_arm_test.go is created by an earlier commit on this branch and renamed by a later one, and the rename matters. Go read the trailing _arm as a GOARCH constraint and excluded the file from every amd64 build, so those five tests covering the shift+tab full-auto offer never ran, here or in CI. Two of them panicked once they did. That is all in the branch already; the rebase just replays it in order.

go build, go vet, gofmt -l clean. internal/sandbox, internal/specialist and internal/swarm green. The one internal/tui failure is TestAltScreenTranscriptScrollKeepsFooterFixed, which reproduces on a clean tree on this machine.

@gnanam1990 your approval was dismissed by the earlier push, sorry; re-requesting. CodeRabbit is the remaining blocker.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve a host-reachable path for local preview servers
  internal/sandbox/analyzer.go:208
  Reclassifying python -m http.server, Vite, and package-manager dev commands as non-network suppresses the only permission path that grants NetworkAllow, but LocalServer has no execution-policy consumer. The default Linux sandbox consequently adds --unshare-net, so the listener is private to the sandbox namespace and the user's browser cannot reach it; macOS's (deny network*) rejects the bind outright. A normal request to start a preview now runs without a prompt but cannot provide the advertised local server. Keep a scoped approval/runner path for host-visible listening (or consume LocalServer with a suitable policy) and cover the real accessibility behavior.

• [P1] Do not trust package-script names as proof of no network access
  internal/sandbox/analyzer.go:268
npm run dev (and the pnpm/yarn/bun equivalents) executes arbitrary workspace predev/script/postdev hooks, yet this change clears its network classification solely from the requested script name. For example, a repository can make predev upload secrets with curl; on unelevated Windows the documented network isolation is only the per-command approval gate, so this now runs with real network access and without the former prompt. Keep package-manager script dispatch network-gated unless the resolved script can be safely analyzed; only direct known listener binaries should be eligible for the relaxed classification.

• [P2] Normalize the legacy mode before the TUI consumes it
  internal/tui/model.go:865
  The compatibility conversion happens in agent.Run, but newModel copies a persisted/raw PermissionMode("unsafe") unchanged. Since the deprecated Go constant now has the value "full-auto", this makes the TUI reject ! shell escapes, publish a prompting peer identity, and select lower self-correction autonomy even though the ensuing agent run normalizes to full-auto. Normalize at the TUI boundary as well and add the regression case requested by the existing CodeRabbit review.

• [P2] Include the canonical flag in generated shell completions
  internal/cli/completions.go:23
  Both root and exec completion inventories retain only the deprecated --skip-permissions-unsafe spelling. The new documented and parsed --full-auto flag therefore cannot be suggested or completed in bash, zsh, fish, PowerShell, or elvish. Add the canonical spelling to both inventories while retaining the alias.

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (1)

internal/tui/plan_mode_test.go (1)

51-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cover PermissionModeFullAuto restoration.

This test says it restores any prior mode, but it only tests PermissionModeAsk. Add a case that starts in agent.PermissionModeFullAuto, enters plan mode, and asserts that /plan off restores PermissionModeFullAuto.

As per coding guidelines, "Every behavior or security-boundary change requires a regression test."

Proposed test

+func TestPlanCommandRestoresFullAutoOnExit(t *testing.T) {
+	m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeFullAuto})
+
+	updated, _ := m.dispatchCommand(parseCommand("/plan on"))
+	next := updated.(model)
+	updated, _ = next.dispatchCommand(parseCommand("/plan off"))
+	next = updated.(model)
+
+	if next.permissionMode != agent.PermissionModeFullAuto {
+		t.Fatalf("permissionMode after /plan off = %s, want full-auto", next.permissionMode)
+	}
+}

🤖 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/plan_mode_test.go` around lines 51 - 65, Extend
TestPlanCommandRestoresPriorModeOnExit to cover an initial
agent.PermissionModeFullAuto state: enter plan mode with /plan on, then exit
with /plan off, and assert that permissionMode is restored to
agent.PermissionModeFullAuto. Preserve the existing PermissionModeAsk coverage.

Source: Coding guidelines

🤖 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/tui/plan_mode_test.go`:
- Around line 51-65: Extend TestPlanCommandRestoresPriorModeOnExit to cover an
initial agent.PermissionModeFullAuto state: enter plan mode with /plan on, then
exit with /plan off, and assert that permissionMode is restored to
agent.PermissionModeFullAuto. Preserve the existing PermissionModeAsk coverage.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 06eecd36-b0d6-4cf8-86db-adc590b94142

📥 Commits

Reviewing files that changed from the base of the PR and between e954fde and 0dd1a5b.

📒 Files selected for processing (17)

• internal/acp/agent.go
• internal/acp/agent_test.go
• internal/agent/loop.go
• internal/agent/loop_test.go
• internal/agent/types.go
• internal/cli/app.go
• internal/cli/app_test.go
• internal/cli/exec.go
• internal/cli/exec_parse.go
• internal/cli/exec_tools.go
• internal/specialist/exec.go
• internal/specialist/exec_test.go
• internal/tui/model.go
• internal/tui/model_test.go
• internal/tui/peer_messages.go
• internal/tui/plan_mode_test.go
• internal/tui/view.go

🚧 Files skipped from review as they are similar to previous changes (15)

• internal/cli/app_test.go
• internal/agent/loop_test.go
• internal/acp/agent_test.go
• internal/cli/exec.go
• internal/cli/exec_tools.go
• internal/tui/model.go
• internal/acp/agent.go
• internal/cli/exec_parse.go
• internal/specialist/exec_test.go
• internal/cli/app.go
• internal/specialist/exec.go
• internal/agent/loop.go
• internal/agent/types.go
• internal/tui/view.go
• internal/tui/model_test.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve a host-reachable sandbox path for local preview servers
  internal/sandbox/analyzer.go:192
  The new classification sets LocalServer but removes Network, and no policy or runner code consumes LocalServer. Consequently the existing network approval path is skipped and the command is executed with the default NetworkDeny profile: Linux isolates it in a network namespace and macOS applies (deny network*). A normal python -m http.server or vite request now runs without a prompt but cannot expose a preview to the user's browser. Keep a scoped host-listener approval/profile path (and cover runner-level accessibility), or retain the network approval path.

• [P1] Do not infer no egress from a package-script name
  internal/sandbox/analyzer.go:257
npm/pnpm/yarn/bun run dev (and the other serving names) are now classified as non-network without resolving the package script or its lifecycle hooks. A repository can make predev or dev execute curl before it starts a listener; on Windows, where the approval gate supplies the effective network protection, this change lets that host egress run without approval. Keep package-manager script dispatch network-gated unless the resolved hook chain can be safely analyzed.

• [P2] Normalize the legacy mode before the TUI applies mode-specific behavior
  internal/tui/model.go:870
newModel copies a raw PermissionMode("unsafe") option unchanged, while the TUI now checks only PermissionModeFullAuto. Any TUI entry point supplied with the accepted legacy value therefore rejects ! shell escapes, publishes a prompting peer identity, and selects low self-correction, even though agent.Run later normalizes the same run to full-auto. Normalize at the TUI boundary and add a regression case for the raw legacy value.

• [P2] Accept the canonical mode through the explicit exec-mode interface
  internal/cli/exec_tools.go:88
  The PR establishes full-auto as the canonical raw/on-disk value, but resolveExecPermissionMode accepts only the deprecated unsafe spelling (plus high). Thus zero exec --permission-mode full-auto ... exits with an invalid-mode usage error before NormalizePermissionMode is reached, while the warning code already contains an unreachable full-auto branch. Accept full-auto while retaining unsafe as the alias, and update the usage/test coverage.

• [P2] Add the canonical flag to generated shell completions
  internal/cli/completions.go:23
  Both completion inventories still list only --skip-permissions-unsafe; neither root nor exec completion can suggest the new documented --full-auto flag in any generated shell script. Include the canonical flag in both lists while preserving the deprecated alias.

[euxaristia]

Thanks for the review. I've addressed the key feedback points:

1. Fixed the CommandRunsLocalServer issue - the firstSubcommand now correctly skips flags but not values they consume. The vite --host 127.0.0.1 case is now handled correctly.

2. Updated the exec warning to name both flag spellings (--full-auto and --skip-permissions-unsafe), since one bool backs both.

3. Fixed the retry reasons wording to mention both flag spellings.

4. Fixed the shell-escape notice to include both spellings.

5. Updated the permission_mode_arm_test.go file - renamed it to include a GOARCH constraint that prevents it from being excluded from amd64 builds. This fixes the panic when the file is created by an earlier commit.

6. The TestEagerToolSchemaTokenBudget test failure is pre-existing and was also addressed in a related PR (test(agent): raise the eager tool schema ceiling to 3650 #877). I've noted it as an open item.

Verified: go build, go vet, gofmt -l clean. All sandbox, specialist, and swarm tests pass.

[Vasanthdev2004]

All five fixed at d32f271c, @jatmn.

Both P1s were the same mistake in two places, which is worth saying plainly: LocalServer was treated as a replacement for Network when nothing consumes it. I checked, and no policy or runner code reads that field, so the classification granted no host listener at all. It only removed the approval the command used to get, leaving it under the default deny profile: a network namespace on Linux, (deny network*) on macOS. So a dev server started with no prompt and could not serve a preview to the operator, which is the opposite of what the classification was for.

Your package-manager half is the sharper one and I had no answer to it. npm run dev is matched by script NAME, and the repository decides what dev and predev do. Either can curl before anything binds. On Windows, where the approval gate is the network protection, that is unprompted egress.

The fix keeps the classification, because a scoped host-listener path will want it, and keeps the approval until that path exists. One line, since both findings flow through the same point.

I rewrote the PRs own tests rather than flipping their assertions. TestNeitherServingNorBuildingCountsAsNetworkEgress asserted exactly the premise you refuted, so keeping it and changing the code would have been incoherent. It is now two tests: building still counts as no egress, and serving asserts it keeps BOTH the LocalServer classification and the network approval, with the package-manager and python cases included. Nine table rows moved with it.

The three P2s are done as described: resolveExecPermissionMode accepts full-auto with unsafe retained as the alias and the usage text updated, newModel normalizes at the TUI boundary so the legacy value cannot mean one thing to the TUI and another to agent.Run, and both completion inventories carry --full-auto alongside the deprecated flag.

Build, vet, gofmt clean and cross-compiled for windows, linux and darwin. internal/sandbox and internal/agent green. The one internal/tui failure is the pre-existing Windows-local TestAltScreenTranscriptScrollKeepsFooterFixed.

One thing still open from your #866 review that I have not done: the P3 about Varied claiming an error pattern anyErrorCount does not establish. That one is a wording and record-keeping change and I would rather do it deliberately than tack it on here.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/cli/completions.go`:
- Line 23: Update the completion regression expectations in assertCandidates
within completions_test.go for both the root and exec candidate lists to include
--full-auto, preserving the existing candidate ordering and coverage for the
newly added completion entry.

🪄 Autofix

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

Run ID: b170ffb0-0ab3-47ce-9f9e-9f9bbdbf1ec3

📥 Commits

Reviewing files that changed from the base of the PR and between 0dd1a5b and d32f271.

📒 Files selected for processing (6)

• internal/cli/completions.go
• internal/cli/exec_tools.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_local_server_test.go
• internal/sandbox/analyzer_test.go
• internal/tui/model.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/tui/model.go
• internal/cli/exec_tools.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P3] Cover the new canonical completion flags
  internal/cli/completions_test.go:156
  The completion-tree test still asserts only the deprecated --skip-permissions-unsafe spelling for both the root and exec contexts. Removing either newly added --full-auto entry from completionRoot would therefore leave all completion tests green, even though the advertised flag would no longer be suggested. Assert --full-auto in both contexts alongside the compatibility alias.

• [P3] Report accepted full_auto permission-mode input accurately
  internal/cli/exec.go:631
resolveExecPermissionMode accepts --permission-mode full_auto and resolves it to full-auto, but this warning-source switch does not recognize that accepted spelling. It falls through to --auto high, so the safety warning claims a flag was passed that the user did not supply. Include full_auto here (and preserve the supplied spelling), or use a generic --permission-mode reason for all accepted aliases.

[Vasanthdev2004]

@coderabbitai full review

main is merged in at e891146d. The only conflict was one hunk in internal/tui/model.go where both sides added struct fields; kept both. Re-reviewing against the current head.

[coderabbitai]

@Vasanthdev2004 I will review the full PR against the current head, including the resolved internal/tui/model.go conflict.

---

🧠 Learnings used

Learnt from: CR
Repo: Gitlawb/zero PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T16:02:16.338Z
Learning: Applies to **/* : Ensure PR descriptions, help text, and comments match shipped behavior; wire advertised entry points or narrow the claim, and do not widen security allowlists by name alone without checking classification and side effects.

---

Learnt from: CR
Repo: Gitlawb/zero PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T16:02:16.338Z
Learning: Applies to **/* : Rebase onto the current PR base before review; resolve conflicts by preserving upstream behavior unless the PR intentionally changes it.

---

Learnt from: gnanam1990
Repo: Gitlawb/zero PR: 138
File: internal/tui/model.go:860-880
Timestamp: 2026-06-08T04:46:15.695Z
Learning: In the Gitlawb/zero repository (internal/tui/model.go), the `!cmd` shell escape (commandBash) was intentionally gated behind `agent.PermissionModeUnsafe`. The rationale discussed was that while `!cmd` is human-initiated (analogous to `:!` in vim), the final implementation chose to require explicit unsafe mode to prevent sandbox bypass. TestBashEscapeGatedByPermissionMode covers this behavior.

✅ Action performed

Full review finished.

[Vasanthdev2004]

Rebased onto main at 677b3f0. All the conflicts were with #1019, which landed overnight: the root-flag allowlist now names --full-auto next to --skip-permissions-unsafe, the TUI launch keeps the escalation argument under the renamed mode, and the help and completion lists carry both flags. Build, vet and the cli, agent, sandbox and acp packages are green here; linux and darwin cross-builds pass. The push will have dismissed your approval at 4a77c20 @jatmn, so one more look when you have a moment.

[gnanam1990]

I found one remaining security blocker on the current head.

Verdict

Changes requested. Reviewed exact base 6937a309cf00 and head 677b3f0c4f4d. The branch is mergeable and CI is green, but parseable framework builds lose the network-specific approval gate that main currently applies.

Finding

• [P1] Preserve network approval independently of the local-server label
  internal/sandbox/analyzer.go:253
internal/sandbox/analyzer_local_server_test.go:46

  The branch correctly makes LocalServer describe binding rather than building, but it also removes localServerPrograms from commandUsesNetwork. frameworkSubcommandRunsLocalServer returns false for build, generate, and check, and the additive result.Network = true runs only when LocalServer is true. Consequently next build, vite build, nuxt generate, and astro check all produce Network=false; TestBuildingDoesNotCountAsNetworkEgress currently pins that unsafe result.

  These CLIs execute repository configuration, plugins, and application code during builds and can perform egress. The analyzer cannot prove otherwise from the subcommand name. On platforms where the approval gate is the network boundary, a caller that already granted the ordinary shell prompt now runs the build without the separate ReasonNetworkBlocked decision.

  Keep LocalServer precise, but restore an independent conservative Network classification for the framework CLI inventory. Add analyzer and Engine regressions asserting build-like commands are Network=true, LocalServer=false and still receive ReasonNetworkBlocked after an ordinary shell grant; serving commands should remain Network=true, LocalServer=true. Cover option-bearing build forms too so parsing the action more precisely cannot remove the egress gate.

Validation

• Existing focused analyzer, unparseable fallback, ACP mode-boundary, and full-auto CLI tests — pass.
• Disposable production-path regression for the four build commands — fails on all four: each reports Network=false, and Engine.Evaluate returns allow after an ordinary shell grant instead of a network prompt.
• Current GitHub checks — all green; they do not cover this invariant.
• git diff --check — pass.
• No dependency or external-integration change in the PR delta.

The current ACP test already exercises the separate set_config_option rejection path through the legacy unsafe alias, which normalizes to full-auto; I am not treating the additional canonical-spelling fixture as a merge blocker.

[Vasanthdev2004]

Fixed at 32c3527. You were right, and it is the same mistake I have a note about from #685: narrowing a classification and losing the consumer that read the old breadth.

commandUsesNetwork has its localServerPrograms rule back, so the whole framework inventory is conservative for Network whatever the subcommand; LocalServer alone carries the serve/build distinction. The test that pinned Network=false for builds is replaced by one asserting build-like and option-bearing forms are Network=true and LocalServer=false while serving forms are both, and an Engine regression pins the consumer: with a bash allow grant, next build, vite build --mode production, nuxt generate and astro check all still return the network prompt with the grant not matched, the same as curl. Removing the restored line fails both. Adding the option-bearing forms also caught a precision defect: vite --config vite.prod.ts build read the option value as the subcommand and fell through to the bare-vite rule, so the framework classifier now walks the positions a subcommand can occupy, the same helpers the package managers use.

Sandbox package green on Windows here, linux and darwin cross-builds and vet pass. @gnanam1990 this is the only change since your review. @jatmn your approval at 4a77c20 was dismissed by the rebase this morning and again by this push; one more look when you have a moment.

[Vasanthdev2004]

@gnanam1990 your P1 is fixed and you were right about it. commandUsesNetwork has localServerPrograms back, with a comment saying why: binding a port is not egress, but next, vite, nuxt and astro run the repository's own config, plugins and application code during a build, and the subcommand name proves nothing about whether that code makes requests. LocalServer alone now carries the serve versus build distinction.

TestBuildingDoesNotCountAsNetworkEgress, which pinned the unsafe result, is gone. TestFrameworkBuildsStayNetworkWithoutBeingServers replaces it: build-like invocations including vite --config vite.prod.ts build come back Network=true, LocalServer=false, serving comes back true for both. There is also an engine-level test proving a plain bash allow grant does not run next build without the separate network decision, since that is the consequence you named.

Head is f87a0afa, which also merges current main, so it is level with c1937dfa and conflict-free. CI 12 of 12.

[Vasanthdev2004]

@jatmn both P1s from 03f33e4d are closed. That review is three weeks old and was dismissed by a push, so it is worth saying where they went.

The dictation one was a real hole and your diagnosis of the root cause was the useful part: the offer was cancelled on selected deferred-input message types rather than on any user-visible input transition. sttPartialMsg now clears unsafeArmed on the same footing as the final transcription, and the reason is written down at the call site, that the offer can be armed after dictation is already running and active dictation is not a blocking modal, so an ordinary confirm still landed on a composer the user had moved past.

The rebase is done and then some: head f87a0afa merges current main, so it is level with c1937dfa rather than merely rebased onto an older target, and the merge is conflict-free. The two files you named as conflicting, internal/acp/agent.go and internal/agent/loop.go, both merged cleanly this time; the only conflict was in the ACP test file, which I rebuilt from main's version plus this branch's hunks and checked by set difference that nothing from either side was dropped.

Also worth knowing since it postdates your review: @gnanam1990 found that this branch had removed localServerPrograms from commandUsesNetwork, so framework builds lost the network approval gate. That is fixed and the test that pinned the unsafe result is gone.

CI 12 of 12 at this head.

[jatmn]

I found two P2 issues that need to be addressed before this is ready. Please address the underlying contracts and their affected paths together so the next revision closes these issues completely.

Findings

[P2] Keep the full-auto offer visible before accepting confirmation

internal/tui/model.go:1991 — arming; confirmation at internal/tui/model.go:2005; display consumers at internal/tui/view.go:215 and internal/tui/view.go:238–245.

Reproduction and impact

1. Start in Ask mode while dictation is recording or transcribing.
2. Press Shift+Tab. The handler arms the full-auto offer and keeps the current mode at Ask.
3. Inspect the footer: it displays REC or “transcribing…” instead of “full-auto? ctrl+g to confirm.”
4. Press Ctrl+G before another transcription delivery. The mode becomes full-auto, even though the offer and its confirmation instruction were never displayed.

The recording/transcribing sequence reproduces at 40, 96 and 160 columns. The resulting mode indicator also remains hidden while dictation owns that footer space. At normal widths, the model-download status is another branch that replaces the permission chip.

A separate, immediate Ctrl+G is still required. This is a defect in presenting the new confirmation offer, not a bypass of the confirmation key or the OS sandbox. The PR makes this interaction newly relevant: the base's Shift+Tab cycle cannot enter full-auto through this sequence.

Root cause

The handler and renderer disagree about whether an offer is available to the operator. noBlockingModal() permits arming and confirmation during dictation. modeLabel() correctly produces the offer text, but its caller, statusLine(), subsequently replaces that text with another status. The tiny-width branch returns the dictation chip directly; the wider branch overwrites the left chip. An assertion against modeLabel() alone therefore passes while the actual footer omits the offer.

Requested outcome

Keep offer eligibility, presentation and confirmation consistent: an offer that the UI cannot display must not remain actionable. Either preserve the offer in the final footer or decline arming/confirmation while its display is unavailable. Please account for both footer width branches and the existing statuses that replace this chip, rather than special-casing only the recording example.

The implementation choice is yours. Preserve the existing deliberate Ctrl+G confirmation, ordinary mode cycle, plan-mode restrictions and dictation behavior. This does not require a general UI rewrite or a new confirmation mechanism.

Focused validation

• Drive the real key handler and inspect the final footer for idle, recording, transcribing and model-download states, at narrow and normal widths.
• For each supported state, prove either that the offer is displayed before confirmation succeeds, or that an unavailable offer cannot change the mode. If a status transition can hide an already-armed offer, cover that transition too.
• Retain the positive idle confirmation test and the existing cycle, modal and cancellation regressions. A fix that makes confirmation unreachable everywhere would satisfy only the negative half of the contract.

[P2] Isolate the new CLI parity helper from personal state

internal/cli/exec_full_auto_parity_test.go:13–17 — execListToolsFor.

Failure path and impact

The new helper supplies a temporary working directory and a fake resolveConfig, then calls the real CLI entry point. The remaining dependencies and user-directory settings retain their production behavior. Before --list-tools returns:

• runExec starts RefreshModelsDevCache in a goroutine (internal/cli/exec.go:176). A missing or stale personal cache can cause a models.dev request and a cache update.
• Startup reaches the sandbox grant store and user MCP configuration/registration (internal/cli/exec.go:342–357). The fresh working directory does not isolate the user configuration layer, so personally configured MCP servers can participate in startup.

The parity test invokes this helper seven times: once for the baseline and once for each of six alternative spellings. A clean CI environment can pass while a developer's configured environment performs unrelated network/process activity or changes the test's behavior.

The background request is reproducible with emulated user directories and an in-process HTTP transport, without contacting an external service. Isolating the helper suppresses that request. The MCP startup consequence follows the production dependency path; it does not require assuming that every developer has servers configured.

Root cause

The fixture isolates the project directory and one configuration function, but CLI startup resolves additional user state independently. --list-tools is an early exit from agent execution, not from all startup work. The fake resolveConfig does not replace the models refresh, MCP resolver or permission stores.

This is attributable to the newly added test caller. Production CLI startup already had these behaviors; changing production startup is not the requested fix. The repository's hermetic-test rule applies to the new helper.

Requested outcome

Establish isolation inside execListToolsFor, before calling runWithDeps, so every invocation inherits it. The existing isolateCLIUserState(t) is the natural starting point: it redirects user roots, gives the models cache a fixture path, disables background model fetching and avoids the host keyring. Complete dependency substitution is also acceptable. Check the remaining defaults reached by this helper so that faking the main config is not mistaken for isolating the whole invocation.

Keep the current baseline and all six alias cases, including the swarm_spawn assertion. The fix belongs in test setup; production tool listing should retain its intended integration behavior.

Focused validation

• Run the parity test against an emulated populated user environment, with all fixture files and any fake services confined to temporary test directories.
• Verify that the helper neither starts an unrelated user MCP fixture nor makes a models refresh request, and leaves the emulated personal files unchanged. Exercise the missing/stale-cache condition so a warm cache cannot hide an unisolated refresh.
• Use the repository's cross-platform user-root helper rather than redirecting only the working directory or one XDG variable. Run the parity test alongside the existing CLI isolation tests to catch dependence on another test having configured the environment first.

Guidance for closing this revision

These two findings share a concrete testing gap: the assertions stop before the downstream behavior that determines whether the feature works. The offer text exists in modeLabel, but the final renderer removes it. The parity helper receives a fake config, but startup still resolves other production dependencies. Passing those local assertions does not establish the user-visible or side-effect contract.

That is the pattern worth addressing here. It explains why these two defects can survive otherwise useful tests; it is not enough evidence to assign one cause to every earlier review round.

For this revision, please close each contract through its actual consumers:

1. For the offer, follow input → armed state → final display → confirmation or cancellation. Test the real display and resulting mode together.
2. For the parity fixture, follow helper setup → CLI startup → configuration/cache/integration dependencies → return and cleanup. Establish isolation before startup can schedule background work.
3. For each fix, show that its regression test fails on the current code for the named reason and passes with the fix. Keep the positive behavior checks so refusing everything or skipping the intended assertions cannot make the test pass.
4. In the next update, identify the affected paths covered, the focused and neighboring tests run, and any concrete validation limitation. This will make it possible to assess the complete fix together rather than uncover another consumer after reviewing a single patched branch.

The requested work is bounded to these two findings and the directly affected paths above. It does not expand this PR into a sandbox redesign, a new host-listener capability, a general CLI startup refactor or a repository-wide test cleanup. Keep unrelated pre-existing behavior outside the patch. A complete fix here should align the existing contracts and prove them at their observable boundaries.

[jatmn]

I found two P2 issues that need to be addressed before this is ready, plus a smaller P3 confirmation-display issue. These concern the new interactive full-auto transition and its regression coverage.

Findings

[P2] Avoid blocking the event loop when full-auto releases held peer messages

internal/tui/model.go:2006

Trigger and impact. Start in ask mode with an active run. A message from a bypass-mode peer is held because the permission classes differ. openNextPeerApproval defers opening the approval prompt while m.pending, so the message can be queued without a blocking modal. Shift+Tab followed by Ctrl+G then enters full-auto and synchronizes the peer identity. If that makes the held message eligible for release, the UI freezes during confirmation.

Root cause. This identity update has a synchronous delivery effect:

1. The Ctrl+G handler calls syncPeerIdentity from inside Bubble Tea's Update.
2. Service.UpdateIdentity updates the peer record, reevaluates held messages under the parity policy, and synchronously invokes the held-release handler for newly compatible messages.
3. Run wires that handler through forward to program.Send.
4. program.Send waits on Bubble Tea's unbuffered message channel. The event loop that must receive the message is already executing the mode change and cannot return until this send completes.

The callback design already exists on the base branch. This PR newly reaches it through the prompting-to-bypass keyboard transition: on the base, Ask cycles back to Auto and these keys cannot enter bypass mode. The finding is about the new transition exposing that existing callback hazard.

A reproduction using the peer service and a real Bubble Tea event loop blocks inside the release callback until the program context is cancelled. The TUI route into that callback is established by the production call chain. The existing peer-release unit test uses a buffered callback channel, which verifies release eligibility but does not exercise this event-loop dependency.

Requested outcome. Make peer release safe when identity synchronization originates inside Update. Choose the appropriate delivery mechanism at that boundary so the event loop can return and subsequently process the released message. Preserve identity publication, the parity decision, and delivery of the queued message. Please account for ordering and shutdown when arranging deferred delivery; simply making the callback return is insufficient if it loses messages or leaves delivery blocked after shutdown.

Regression coverage. Exercise a held bypass-peer message while the receiver is prompting and an active run keeps its approval queued. Drive the new confirmation path with the production-style event-loop callback. Verify that confirmation returns without cancelling the program, the UI processes a subsequent message, and the released peer message reaches the existing inbox/delivery path once. This tests the consumer that the current buffered callback test misses.

[P2] Isolate user state in the new CLI parity helper

internal/cli/exec_full_auto_parity_test.go:13

Trigger and impact. Running TestFullAutoEntryPointsListTheSameSpecialistTools on a developer machine invokes execListToolsFor seven times. The helper supplies a temporary cwd and a mocked provider configuration, but runWithDeps fills its other dependencies with production defaults. Before --list-tools returns, startup can refresh the user's models cache, load and start configured MCP servers, and migrate sandbox grants or consume a pending migration notice. The test can therefore perform unrelated process/network activity, modify personal state, or fail because of the machine's configuration.

Root cause. The fixture isolates workspace configuration, while this entry point also consumes user-scoped configuration and starts background work. --list-tools returns after those startup effects have begun. Passing a temporary cwd does not redirect those user roots. The earlier test-isolation request remains unaddressed in this helper.

An emulated-user test with intercepted HTTP observes an attempted request to models.dev. Adding the existing isolateCLIUserState(t) fixture prevents that request. The MCP and grant effects follow from the production startup dependencies reached before the listing return; the HTTP reproduction does not claim to have exercised every possible personal integration.

Requested outcome. Apply the established user-state isolation fixture inside this helper before invoking runWithDeps. Ensure its background services and integrations consume only fixture-owned configuration. The repository already provides redirection of platform user roots and model-cache storage, disables background model fetching, and selects fixture-backed credential storage. Keep the real mode resolution and tool-registration behavior that this regression is intended to test.

Regression coverage. Add this helper to the existing helper-isolation coverage. Seed an emulated personal configuration outside the helper's fixture and verify that the helper redirects away from it, leaves it unchanged, and does not attempt the background model fetch. Keep the baseline plus all six alias cases and their swarm_spawn assertions. Calling the helper in isolation should be safe; it should not depend on another test having already configured the environment.

[P3] Keep the confirmation usable after the pet reserves footer space

internal/tui/view.go:210

Trigger and impact. With a docked pet, footerStatusLine reserves 11 columns before calling statusLine. At a supported 24-column terminal, the offer renders as ● full-aut…; at 30 columns it renders as ● full-auto? ctr…. The complete View omits the confirmation key in both cases, but Ctrl+G still enables full-auto. This is a narrow-layout defect; I would not block merging solely on this P3 item.

Root cause. offerConfirmable shares the armed/exit/cancel state between rendering and input handling, but that state does not establish that the final layout can display the confirmation. The footer can win the status-priority decision and subsequently be truncated by its effective width. Tests that call statusLine with the terminal width miss the width reduction made by its caller.

The full-View reproduction covers both the display and the subsequent keypress. Wider layouts and layouts without a pet provide controls where the key remains visible. The base has the same space reservation, but has no actionable full-auto offer, so the same key sequence cannot enable bypass there.

Requested outcome. Keep a readable offer and confirmation key visible at the final available footer width, or make an offer that cannot be displayed non-confirmable. Base the decision on the effective layout, including pet reservation. Preserve the existing offer cancellation and status-priority behavior.

Regression coverage. Drive Shift+Tab, inspect the complete View, then drive Ctrl+G at 24 and 30 columns with a docked pet. Assert the contract: either the user can see the offer and confirmation key and explicitly confirm it, or the hidden offer cannot enable full-auto. Include a wider or pet-free control so a fix cannot pass by disabling confirmation everywhere.

Guidance for closing this round

The common pattern in these three findings is that a locally correct change is being checked before its final consumer has finished applying it. The permission mode changes correctly, but its peer callback cannot complete on the UI event loop. The tool list is correct, but startup has already reached personal state. The offer wins the footer's priority decision, but the final layout removes its confirmation instruction.

That explains how these particular gaps can coexist with passing tests. The peer test substitutes a buffered delivery channel, the CLI test checks output without isolating all startup inputs, and the rendering test checks a component before its caller reserves space. The useful next step is to extend each regression through the corresponding production boundary and assert the resulting behavior.

Please address these three as one cohesive follow-up, with each fix and its regression tied to the failure path above. For each changed boundary, check the immediately adjacent caller and consumer so the fix preserves message delivery, tool-registration parity, and usable confirmation. Keep the remediation within those contracts; the existing peer service, CLI isolation fixture, and footer layout provide the relevant places to resolve them.

In the follow-up, please identify the underlying cause addressed, the test that fails without the fix, and the behavior it protects. That gives us concrete closure criteria for this round and reduces the chance of another example of the same underlying issue appearing in the next review. These findings establish specific gaps; they do not establish that every earlier review round had the same cause or that unrelated subsystems need redesign.

[Vasanthdev2004]

@jatmn all three at 7be89202, one commit each, plus main merged again. For each: the cause, the test that fails without the fix, and what it protects.

Event loop. The cause is exactly the chain you traced: UpdateIdentity runs the held-release handler synchronously, the ctrl+g and shift+tab paths call it from inside Update, and the handler ended in program.Send, which waits on the one channel the loop reads while the loop is inside that Update. Peer callbacks now go through a small forwarder in peer_forward.go: the callback appends to a FIFO and returns; one goroutine drains it into program.Send in order. That keeps identity publication synchronous and the parity decision where it was, keeps peer events ordered because there is exactly one drainer, and after the program has exited Send returns on its cancelled context so a late callback cannot wedge its caller either. The wiring moved out of Run into wirePeerService, which takes the four handler setters as an interface so the regression can stand in a fake.

TestFullAutoConfirmationReleasesAHeldPeerMessageWithoutBlockingTheLoop runs a real Bubble Tea program with the real wiring and a model whose Update invokes the installed release handler synchronously, the production shape reduced to its mechanism. It asserts the loop processes the next message, the released message reaches the peerHeldReleasedMsg path exactly once, the program exits, and a release after exit returns. With the forwarder delivering synchronously it fails on the loop never processed the message after the release, which is the freeze. The route from ctrl+g to that handler through syncPeerIdentity is the production call chain you established; I did not build a two-service transport fixture to drive it end to end, and say so rather than claim it.

CLI parity helper. execListToolsFor now calls isolateCLIUserState before runWithDeps, so user roots, the models cache, the background fetch and credential storage all land in the fixture, and the helper is a row in TestCLILaunchHelpersLeaveUserStateAlone, which seeds an emulated personal config outside the fixture and checks it is untouched, the config path resolves elsewhere, and the fetch is disabled. Without the isolation call that row fails on user config still resolves inside the seeded root. The baseline and all six alias cases with their swarm_spawn assertions are unchanged.

Footer width. offerConfirmable now also requires offerKeyVisible, which renders the exact offer chip, truncates it with the same fitStyledLine the footer uses at the width left after petComposerReservedColumns, and checks the key survived. An offer the footer cannot show is not raised by shift+tab, and a live one is withdrawn on a resize that hides its key. TestOfferConfirmsOnlyWhereTheFooterShowsItsKey drives shift+tab, reads the complete View, then drives ctrl+g at 24 and 30 columns with a docked pet, where the chip reads ● full-aut… and ● full-auto? ctr… as you saw, and at 30 without a pet and 96 with one as controls; the contract it asserts is yours, the key is visible and ctrl+g confirms, or it is not and ctrl+g cannot. With the visibility check stubbed to true it fails on ctrl+g entered full-auto on an offer the footer never showed the key for. Cancellation and status priority are untouched and their tests still pass.

Locally internal/tui and internal/cli pass apart from the transcript-scroll test that fails on main on this machine; CI is 12 of 12 at 7be89202.