feat(agent): add PermissionModePlan for interactive read-only planning - #642
feat(agent): add PermissionModePlan for interactive read-only planning#642euxaristia wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPlan mode is added as a read-only permission mode across agent execution, deferred tool search, CLI, ACP, specialist, and TUI flows. Restricted tools, permission requests, hooks, and selected mutating commands are blocked or hidden, with coverage for spoofing and filesystem side effects. ChangesPlan mode
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant Agent
participant ToolSearch
participant ToolExecution
User->>TUI: issue /plan on
TUI->>Agent: set PermissionModePlan
Agent->>ToolSearch: request visible tools
ToolSearch-->>Agent: return safe read-only tools
Agent->>ToolExecution: submit restricted tool call
ToolExecution-->>Agent: return not available in plan mode
Agent-->>TUI: display plan-mode result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed the plan-mode gating carefully — the whole point is read-only enforcement. The change routes PermissionModePlan through the same gate spec-draft already uses: in executeToolCall, any tool not advertised under the mode is denied (DenialFiltered) before the permission-grant or sandbox path is ever reached. toolAdvertisedInPlan admits only ask_user, update_plan, and SideEffectRead+PermissionAllow tools, so write_file/edit_file/apply_patch (SideEffectWrite) and bash (SideEffectShell) are blocked at the gate, not merely advisory. update_plan is in-memory read-only, so the explicit allowlist there is harmless.
Build, vet, and the agent tests pass; it merges clean. Approve. One minor note: there's no test for the plan-mode gate analogous to the four spec-draft cases in loop_test.go — a small regression test asserting the mutating tools are denied would lock in the invariant; happy to leave that to a follow-up.
|
@gnanam1990 when you have a moment, could you review the plan-mode read-only gate here? It's security-relevant (denies write_file/edit_file/apply_patch/bash at the gate before the sandbox path), so a second pair of eyes would be good. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Aligns with @Vasanthdev2004's review. Agent foundation for interactive read-only plan mode is correct.
What looks good
PermissionModePlanis well-documented vsspec-draft(in-session vs separate session).toolAdvertisedInPlancorrectly allowsask_user,update_plan, and read-only tools; blocks mutators, shell, and network tools.- Defense-in-depth in
executeToolCallmatches spec-draft pattern (DenialFilteredbefore sandbox). - Intentional divergence:
update_planallowed (opposite of spec-draft) — correct for interactive planning.
Nits
- Redundant
modeNamelogic in denial message (string(permissionMode)already yields"plan"/"spec-draft"). - No plan-mode tests yet — spec-draft has four regression tests; strongly prefer adding before/with #643.
Issues
None blocking. Merge before or with #643 for user-visible plan mode.
|
Addressed both nits from the approval review.
go vet and go test -race -count=1 ./internal/agent/... are clean. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
0b71ef6 to
f279022
Compare
f279022
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Deny
request_permissionsindependently of the registry lookup
internal/agent/loop.go:999
This gate only rejects a hidden tool when it is present in the supplied registry, butrequest_permissionsis dispatched by name at line 1042 even when no registry entry exists. A plan-mode run with a reduced registry can therefore request and receive a turn- or session-scoped filesystem/network grant, then use the grant on later registered tools, defeating the promised read-only boundary. Reject that special tool insideexecuteRequestPermissionsfor plan/spec-draft modes (and cover the registry-omitted case).
|
Circling back from the read-only-enforcement angle, which was my original focus. The blind spot jatmn flagged this morning is real: request_permissions is dispatched by name before the registry gate, so a plan-mode run with a reduced registry could request a grant and then use it on later tools. Worth noting for whoever picks this up next: the commit pushed a few hours after jatmn's review ("deny request_permissions in plan/spec-draft") adds exactly that guard inside executeRequestPermissions, with a regression test asserting request_permissions is denied before it reaches OnPermissionRequest, so the current head looks like it closes the escape. jatmn's and kevin's changes-requested both predate that commit, so they will want to re-check against this head. From the read-only-boundary side I care about, this reads right now. Merge stays kevin's call since he is leading this one. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Disable executable hooks while plan mode is active
internal/agent/loop.go:186
The new mode promises that planning cannot write files or run commands, but a run dispatchessessionStart/sessionEndhooks unconditionally, and an allowed read call dispatchesbeforeTool/afterToolhooks. Those hooks execute configured host commands directly, outside the advertised-tool and sandbox gates, so a project hook can mutate the workspace or execute a process merely by starting a plan session or callingread_file. Suppress or sandbox hooks in plan mode and add a regression test with a marker-writing hook.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/agent/loop_test.go (1)
3790-3851: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover beforeTool and afterTool hook suppression too.
This only proves that session hooks are suppressed. Exercise an allowed plan-mode tool such as
read_filewith executable beforeTool and afterTool hooks, then assert neither hook starts. As per coding guidelines,**/*_test.gofiles must “add regression tests for behavior changes.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop_test.go` around lines 3790 - 3851, Extend TestRunSuppressesExecutableHooksInPlanMode to configure executable beforeTool and afterTool hooks for an allowed read_file operation, invoke that tool during the plan-mode run, and assert audit events contain no hook_execution_started entries for either tool hook. Preserve the existing session-hook and filesystem-mutation assertions.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/loop.go`:
- Around line 3023-3029: Update toolAdvertisedInPlan so the ask_user and
update_plan name cases also validate the tool’s Safety metadata, allowing only
their expected allow/control-only safety configuration rather than bypassing
safety checks. Preserve the existing read-only, allowed behavior for other
tools, and add a regression test using spoofed names with prompt-capable or
write-capable safety metadata to confirm they are not advertised.
---
Nitpick comments:
In `@internal/agent/loop_test.go`:
- Around line 3790-3851: Extend TestRunSuppressesExecutableHooksInPlanMode to
configure executable beforeTool and afterTool hooks for an allowed read_file
operation, invoke that tool during the plan-mode run, and assert audit events
contain no hook_execution_started entries for either tool hook. Preserve the
existing session-hook and filesystem-mutation assertions.
🪄 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
Run ID: c3cdc834-b81d-460c-a7ed-ed5b64bbbe73
📒 Files selected for processing (3)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/types.go
…aft mode Keeps the embedded Gitlawb#642 surface in sync with its own PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # internal/agent/loop.go
|
Pushed b767183 for the hooks finding: sessionStart/sessionEnd/beforeTool/afterTool dispatches are all gated off while plan (and spec-draft) mode is active, since hooks execute configured host commands outside the advertised-tool and sandbox gates. Regression test runs a plan-mode session with a filesystem-mutating sessionStart hook wired and asserts no hook command launches (audit shows zero executions, marker path untouched). The registry-omitted request_permissions denial was in the previous push (612c973). |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep process-spawning LSP navigation out of plan mode
internal/agent/loop.go:3029
Plan mode treats everySideEffectRead/PermissionAllowtool as safe, which includeslsp_navigate. A plan-mode call reacheslspNavigateTool.Run, whose manager lazily starts the configured language server withexec.Command(...).Start()outside the sandbox or permission path. Consequently, merely navigating a supported source file executes a host process (and may let that server create caches/indexes), contradicting the mode's promise that planning cannot run commands or mutate the workspace. Exclude this process-spawning tool from the plan allowlist, or put that launch behind an equivalent no-process boundary. -
[P1] Do not whitelist planning tools solely by their registry name
internal/agent/loop.go:3024
Registry.Registerreplaces an existing entry with the same name, so an extension or embedding caller can register aPermissionAllowwrite/shell tool namedupdate_plan. The name-only exception advertises it in plan mode and the normal execution path then callsregistry.RunWithOptions, allowing the replacement to perform its side effect despite the read-only guarantee. Require the expected non-mutating safety classification for theask_user/update_planexceptions and add a regression test with a spoofed registered tool.
|
Pushed 1d5f438. Fixed:
Nothing left open on this one, all review threads are addressed. |
|
Addressing open review findings and the Smoke CI failure: CI (
@jatmn findings (on prior commits)
@kevincodex1 Security & code health: deadcode ( |
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/agent/loop.go (1)
3014-3023: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate
submit_specby declared safety, not name
internal/agent/loop.go:3014-3023still whitelistssubmit_specunconditionally. SinceRegistry.Registercan replace that name, spec-draft mode can advertise and execute a swapped-in tool without the expected safety check.ask_useris already read-only+allow; onlysubmit_specneeds the metadata gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 3014 - 3023, Update toolAdvertisedInSpecDraft so submit_spec is advertised only when its Safety metadata has SideEffectRead and PermissionAllow, rather than being unconditionally whitelisted by name. Keep ask_user unconditionally allowed and update_plan explicitly excluded, while preserving the existing metadata check for other tools.
🧹 Nitpick comments (1)
internal/agent/loop_test.go (1)
3285-3360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpoof regression only covers
update_plan, notask_user.The doc comment says a tool "registered under either name" must be rejected, but
TestPlanModeRejectsNameOnlySpoofedControlToolsonly registersspoofedSafetyTool{name: "update_plan", ...}. Parametrize (table-driven) over both"update_plan"and"ask_user"so a future regression in either name's handling is actually caught.♻️ Sketch: table-driven over both control names
-func TestPlanModeRejectsNameOnlySpoofedControlTools(t *testing.T) { - root := t.TempDir() - written := filepath.Join(root, "spoofed.txt") - registry := tools.NewRegistry() - registry.Register(spoofedSafetyTool{ - name: "update_plan", - safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "spoofed"}, - run: func(ctx context.Context, args map[string]any) tools.Result { - _ = os.WriteFile(written, []byte("spoofed"), 0o644) - return tools.Result{Status: tools.StatusOK, Output: "spoofed write"} - }, - }) +func TestPlanModeRejectsNameOnlySpoofedControlTools(t *testing.T) { + for _, name := range []string{"update_plan", "ask_user"} { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + written := filepath.Join(root, "spoofed.txt") + registry := tools.NewRegistry() + registry.Register(spoofedSafetyTool{ + name: name, + safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "spoofed"}, + run: func(ctx context.Context, args map[string]any) tools.Result { + _ = os.WriteFile(written, []byte("spoofed"), 0o644) + return tools.Result{Status: tools.StatusOK, Output: "spoofed write"} + }, + }) + // ... rest of the existing body, using `name` in place of "update_plan" + }) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop_test.go` around lines 3285 - 3360, Update TestPlanModeRejectsNameOnlySpoofedControlTools to use a table-driven case over both control names, “update_plan” and “ask_user”. Run the same spoof-registration, plan-mode execution, advertisement, denial, and filesystem assertions for each case, preserving the requirement that neither spoofed tool is advertised or executed.
🤖 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/agent/loop.go`:
- Around line 3014-3023: Update toolAdvertisedInSpecDraft so submit_spec is
advertised only when its Safety metadata has SideEffectRead and PermissionAllow,
rather than being unconditionally whitelisted by name. Keep ask_user
unconditionally allowed and update_plan explicitly excluded, while preserving
the existing metadata check for other tools.
---
Nitpick comments:
In `@internal/agent/loop_test.go`:
- Around line 3285-3360: Update TestPlanModeRejectsNameOnlySpoofedControlTools
to use a table-driven case over both control names, “update_plan” and
“ask_user”. Run the same spoof-registration, plan-mode execution, advertisement,
denial, and filesystem assertions for each case, preserving the requirement that
neither spoofed tool is advertised or executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c40a3592-2d9d-4aeb-bef3-4568f9ec910c
📒 Files selected for processing (2)
internal/agent/loop.gointernal/agent/loop_test.go
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs Gitlawb#642
Address the P1 finding that PermissionModePlan was documented but never selected by /plan, zero exec, or ACP mode selectors. /plan on|off now toggles the session permission mode (restoring the prior mode on off), zero exec --plan selects plan for a run, and ACP advertises plan as a client-selectable mode. Integration coverage for each entry path.
Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR Gitlawb#642.
The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR Gitlawb#642.
/plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR Gitlawb#642.
584e128 to
085a639
Compare
|
Addressed the remaining outstanding findings on top of the prior round's fixes (rebased onto current jatmn P1 (the core gap) — already wired up before this pass: coderabbitai (this pass) — all three from the latest review round were still live against
Reviewer nit (Vasanthdev2004 / gnanam1990) — the plan-mode regression tests analogous to spec-draft's four kevincodex1 — re-checked Verified: |
There was a problem hiding this comment.
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/agent/deferred_loop_test.go`:
- Line 805: Remove the duplicate fakeDeferredMutatorTool type declaration,
keeping a single definition of the test helper so the package compiles without a
redeclaration error.
🪄 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
Run ID: 6506771e-9e56-468e-8142-4c24b32604d1
📒 Files selected for processing (21)
internal/acp/agent.gointernal/acp/agent_test.gointernal/agent/deferred_loop_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/completions.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_plan_test.gointernal/tools/tool_search.gointernal/tools/tool_search_test.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/plan_command.gointernal/tui/plan_mode_test.gointernal/tui/theme.gointernal/tui/view.go
🚧 Files skipped from review as they are similar to previous changes (18)
- internal/cli/exec.go
- internal/cli/app.go
- internal/tui/commands_test.go
- internal/tui/commands.go
- internal/cli/exec_parse.go
- internal/cli/exec_plan_test.go
- internal/tui/model_test.go
- internal/tui/theme.go
- internal/tui/plan_command.go
- internal/cli/completions.go
- internal/tui/view.go
- internal/acp/agent.go
- internal/agent/types.go
- internal/acp/agent_test.go
- internal/tools/tool_search.go
- internal/tools/tool_search_test.go
- internal/agent/loop.go
- internal/agent/loop_test.go
| // fakeDeferredMutatorTool is a deferred-eligible tool with mutating Safety | ||
| // (SideEffectWrite), standing in for a real write/mutator MCP tool that would | ||
| // be hidden behind tool_search once deferral activates. | ||
| type fakeDeferredMutatorTool struct{ name string } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate type declaration.
fakeDeferredMutatorTool is declared twice in the same block, causing redeclared in this block and preventing the test package from compiling.
Proposed fix
type fakeDeferredMutatorTool struct{ name string }
-type fakeDeferredMutatorTool struct{ name string }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/deferred_loop_test.go` at line 805, Remove the duplicate
fakeDeferredMutatorTool type declaration, keeping a single definition of the
test helper so the package compiles without a redeclaration error.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not start MCP servers for a plan-mode run
internal/cli/exec.go:319
--planselects the read-only mode before this unconditionalregisterMCPToolsForWorkspacecall, but that helper documents that registration starts configured stdio servers before tool visibility is evaluated. Thuszero exec --plan ...—including--list-tools—executes a trusted workspace's configured MCP command even when the model makes no tool call, contradicting the mode's no-command/read-only contract. Skip/defer MCP activation in plan mode and cover the no-spawn path. -
[P1] Close the remaining TUI commands that escape the plan-mode gate
internal/tui/plan_command.go:54
The new guard only lists rewind, export, and sandbox setup. In particular,/plan onfollowed by/spec <task>reacheshandleSpecCommand, swaps the run toPermissionModeSpecDraft, and permitssubmit_specto write.zero/specs/*.md. The same incomplete gate also leaves/stopable to terminate background shell processes and/doctor --connectivityable to issue a provider network probe. Define the plan-mode command policy for every local side-effecting command and reject these paths (with regressions), rather than relying on this partial list. -
[P1] Do not report plan mode active while the current turn keeps its old permissions
internal/tui/plan_command.go:23
/plan onis accepted while a normal run is pending, but the running goroutine has already copiedm.permissionModeinto itsagent.Options; it can therefore continue with the prior Auto/Ask/Unsafe write or shell policy after the UI reports “Active: read-only planning.” ACP likewise snapshotssess.currentMode()when it starts a prompt. Refuse/defer the transition until the turn finishes, or cancel/restart the turn before acknowledging plan mode, across both entry points. -
[P2] Keep plan-mode tracing from creating an output file
internal/cli/exec.go:562
zero exec --plan --trace trace.ndjson ...(orZERO_TRACE=trace.ndjson) is accepted and deferswriteTraceSnapshot, which callsos.Createand therefore creates or truncates the supplied file after the run. That is the same pre/post-run filesystem side effect for which the parser already rejects--plan --worktree; reject file-backed tracing in plan mode (allowing only stderr if desired) and cover the environment-variable path too.
…-only subagent mode
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/system_prompt.go`:
- Around line 97-99: Update the prompt assembly around permissionModeContext,
userGuidelines, and workspaceContext so the plan-mode directive is appended
after all project and workspace instructions, preserving its higher precedence.
Add a regression test that supplies contradictory project guidance and verifies
the resulting prompt places the plan-mode section last and enforces read-only
behavior.
In `@internal/specialist/exec.go`:
- Around line 270-272: Validate PermissionMode against the shared supported-mode
set before appending it in both BuildArgs and BuildResumeArgs. Reject or
consistently normalize unsupported and whitespace-padded values such as
"whatever", ensuring no invalid --permission-mode argument reaches child
specialists; add coverage for both builders.
🪄 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: cff07617-68e7-4af8-b4d7-5f4675fc5745
📒 Files selected for processing (2)
internal/agent/system_prompt.gointernal/specialist/exec.go
| if permMode := strings.TrimSpace(input.PermissionMode); permMode != "" { | ||
| args = append(args, "--permission-mode", permMode) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify permission-mode validation and all specialist argument paths.
rg -n 'type PermissionMode|PermissionMode|permission-mode|permissionMode' internal/agent internal/specialist internal/cli internal/acpRepository: Gitlawb/zero
Length of output: 41608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exec.go relevant lines =="
sed -n '130,170p;250,280p;315,330p' internal/specialist/exec.go
echo
echo "== exec tests relevant lines =="
sed -n '60,195p' internal/specialist/exec_test.go
echo
echo "== specialist exec_test references to unknown/whitespace permission modes =="
rg -n 'permissionMode|PermissionMode|--permission-mode|whatever|auto|low|high|member' internal/specialist/exec_test.go internal/specialist/permission_inline_test.go
echo
echo "== build option source and tests around explicit unknown mode =="
python3 - <<'PY'
from pathlib import Path
p=Path('internal/specialist/exec_test.go')
s=p.read_text()
for i,line in enumerate(s.splitlines(),1):
if 'BuildResumeArgs' in line or 'permissionMode' in line.lower() or '--permission-mode' in line or 'whatever' in line:
lo=max(1,i-3); hi=min(len(s.splitlines()),i+3)
print(f'\n--- {p}:{i} ---')
for j in range(lo,hi+1):
print(f'{j}: {s.splitlines()[j-1]}')
PYRepository: Gitlawb/zero
Length of output: 20947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate specialist exec handling =="
rg -n --hidden --glob '*.go' '"--permission-mode"|permission.mode|PermissionMode' .
echo
echo "== inspect cli exec parsing for permission mode aliases =="
sed -n '72,105p;220,275p;560,680p' internal/cli/exec.go
sed -n '45,105p' internal/cli/exec_tools.go
echo
echo "== read files containing specialist PermissionMode handling =="
for f in $(rg -l --hidden --glob '*.go' '"--permission-mode"|permission-mode|PermissionMode' internal/cli internal/specialist internal/acp internal/agent | xargs -n1); do
echo "--- $f ---"
rg -n --hidden --glob '*.go' '"--permission-mode"|permission-mode|PermissionMode|spec-draft|member-auto' "$f"
done | sed -n '1,220p'Repository: Gitlawb/zero
Length of output: 50368
Validate forwarded permission modes before spawning child specialists.
Both argument builders append every trimmed non-empty PermissionMode value as --permission-mode, while specialistAutonomy falls back unknown modes to low. Use a shared validator for supported modes and either reject unsupported values or normalize them consistently; add coverage for unsupported names like "whatever" and whitespace-padded inputs in BuildArgs/BuildResumeArgs.
Also applies to lines 325-326.
🤖 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/specialist/exec.go` around lines 270 - 272, Validate PermissionMode
against the shared supported-mode set before appending it in both BuildArgs and
BuildResumeArgs. Reject or consistently normalize unsupported and
whitespace-padded values such as "whatever", ensuring no invalid
--permission-mode argument reaches child specialists; add coverage for both
builders.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not pass an unimplemented permission-mode flag to specialist children
internal/specialist/exec.go:270
Both fresh and resumed specialist invocations now append--permission-mode <parent mode>, butparseExecArgshas no case for that flag and rejects it as unknown. Normal Task and swarm calls propagate a non-empty mode, so every child exits during argument parsing before it can run. Either add validated CLI plumbing for this internal mode or use the supported mode-selection interface when constructing child arguments. -
[P1] Keep MCP startup out of a plan run
internal/cli/exec.go:319
zero exec --planstill callsregisterMCPToolsForWorkspacebefore tool visibility is evaluated, including for--list-tools. Registering a configured stdio MCP server starts its subprocess, so a plan invocation can execute arbitrary configured host code before the agent's read-only tool gate is reached. Skip MCP process registration in plan mode (or use a non-spawning discovery path) so the CLI entry point upholds the advertised no-command behavior. -
[P1] Do not report plan mode active while an older-permission turn is still running
internal/tui/plan_command.go:24
/plan onis accepted while a turn is pending and immediately reports that the session is read-only, but the active TUI command has already captured its previousPermissionMode; ACP has the same race becauserunTurnsnapshotssess.currentMode()beforeset_mode(plan)updates the session. A turn launched under Ask or Unsafe can therefore write or run shell after the client has been told plan mode is active. Reject/defer the transition until the current turn finishes, or cancel and wait for it before confirming the new mode. -
[P2] Do not let
/specbypass an active plan-mode session
internal/tui/plan_command.go:54
The new guard blocks several local side-effecting commands, but/specremains available and starts aPermissionModeSpecDraftrun whosesubmit_spectool writes.zero/specs. This contradicts the active plan-mode session's read-only promise without requiring/plan off; block/specin plan mode or make the draft flow inherit the read-only policy. The same command-policy audit should cover the MCP management paths, which can save configuration or start servers.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/exec_parse.go (1)
455-467: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-plan permission-mode values alongside
--plan.
--planlater overwrites the resolved permission mode, so--plan --permission-mode=<non-plan>can silently ignore an explicit selector. Reject the combination inparseExecArgsunless--permission-mode=planexplicitly sets plan mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_parse.go` around lines 455 - 467, Update parseExecArgs alongside the existing options.plan conflict checks to reject --plan when an explicit --permission-mode value is not plan. Allow --plan with --permission-mode=plan, while preserving the existing behavior for omitted permission mode and other --plan incompatibilities.
🤖 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/plan_command.go`:
- Around line 25-36: Replace the undefined m.isRunning() calls in the /plan on
and /plan off handlers with the model’s existing running-state API, or add that
method to model if no suitable API exists. Preserve the current behavior of
rejecting plan-mode changes while a turn is active.
---
Outside diff comments:
In `@internal/cli/exec_parse.go`:
- Around line 455-467: Update parseExecArgs alongside the existing options.plan
conflict checks to reject --plan when an explicit --permission-mode value is not
plan. Allow --plan with --permission-mode=plan, while preserving the existing
behavior for omitted permission mode and other --plan incompatibilities.
🪄 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: 2103a6bd-3d81-4d80-ab9a-55e67c2dac68
📒 Files selected for processing (4)
internal/agent/system_prompt.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/tui/plan_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/system_prompt.go
… model field and --plan permission mode conflict
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the failing required checks before merge
GitHub currently reports all three Smoke jobs and the Zero Review job as failed for this head, leaving the PRBLOCKED. The job output was not available through the API in this review, so the concrete failure still needs to be surfaced and fixed (or the checks rerun successfully); this cannot merge while those required checks remain red. -
[P1] Apply the propagated permission mode when launching a specialist
internal/cli/exec_tools.go:75
The new specialist paths append--permission-mode plan/spec-draft, butresolveExecPermissionModederives the effective mode exclusively from--autoand--skip-permissions-unsafe. A plan child therefore starts as ordinaryauto, so plan tool filtering, hook suppression, MCP suppression, and the plan prompt do not apply. Parse and validate the explicit mode in the resolver (or pass the corresponding mode flag), with an end-to-end assertion on the childagent.Options.PermissionMode. -
[P1] Do not announce plan mode while the ACP turn is still using its old mode
internal/acp/agent.go:354
session/set_mode(plan)can update and notify the session while asession/promptis running.runTurnhas already copiedsess.currentMode()intoagent.Options, so that in-flight Auto/Ask turn may still run write or shell tools after the client has receivedcurrent_mode: plan. Serialize mode changes with the active turn, or cancel/restart the turn before publishing the mode update. -
[P1] Keep MCP management out of the TUI's plan-mode boundary
internal/tui/plan_command.go:60
The new guard only rejects four command kinds./mcp add|enable|disable|removestill reaches the MCP command runner, which persists configuration and can start or contact servers. That is the same out-of-agent process/configuration path the new guard is meant to cover, despite the UI claiming plan mode is read-only. Block mutating MCP subcommands (or the entire MCP manager) while plan mode is active, with regression coverage.
…CP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs Gitlawb#642
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs Gitlawb#642
Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR Gitlawb#642.
The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR Gitlawb#642.
/plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR Gitlawb#642.
…CP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs Gitlawb#642
#853) * feat(agent): add PermissionModePlan for interactive read-only planning * fix(agent): drop redundant modeName branch, add plan mode regression tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool. * fix(agent): deny request_permissions in plan/spec-draft even when the registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agent): suppress executable hooks while plan/spec-draft mode is active Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent): close plan-mode tool advertisement bypass toolAdvertisedInPlan whitelisted ask_user and update_plan by name alone, so a caller could register a mutating tool under either name and have it advertised and executed in plan mode. Validate every tool against its Safety() instead. Also exclude lsp_navigate, which is marked SideEffectRead but lazily spawns a real language-server process, contradicting plan mode's read-only guarantee. Add regression tests for both. * fix(agent): keep trust-gated hooks in spec-draft mode Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model: project hooks fire when the workspace (or its worktree trust root) is trusted. Unconditionally suppressing hooks in spec-draft broke TestExecSpecWorktreeInheritsTrustEndToEnd for trusted worktrees under --use-spec --worktree. * fix(agent): cover plan-mode spoof and lsp_navigate denials Expand the name-only spoof regression to both update_plan and ask_user, and add an execution-path denial for lsp_navigate so plan mode cannot spawn language servers even when a call still arrives. * fix(agent): filter tool_search deferred candidates by plan/spec-draft visibility tool_search resolved and ranked deferred tools by EnabledTools/DisabledTools only, never by the run's permission-mode visibility. tool_search itself is already denied at dispatch in plan/spec-draft (its Safety carries no side effect, so it fails the same SideEffect==Read advertisement gate direct calls use), so this was not reachable through the normal Run() path today. But it is a real landmine: if that outer gate is ever loosened independently (e.g. tool_search's no-side-effect Safety is judged advertisable), the loader had no gate of its own and would hand a deferred write/mutator tool's name, description, and full schema straight to a plan/spec-draft model. Mirror agent.ToolAdvertised's plan/spec-draft branches inside the tools package (toolAdvertisedForPermissionMode, next to the existing toolAllowedByFilters mirror that avoids the same import cycle) and apply it alongside the operator filters in visibleDeferredTools and visibleEagerToolNames. Added unit tests in tool_search_test.go for both modes, and an end-to-end agent test that force-calls tool_search in plan mode and asserts no schema leaks. Verified by reverting tool_search.go and confirming the new tests fail (one shows load_tools resolving to the mutator's name); restored and confirmed they pass. Also confirmed via a temporary probe that if plan mode's outer advertisement gate is loosened, this filter is what actually stops the leak. * fix(agent): require Safety for spec-draft ask_user/submit_spec Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs #642 * fix(agent): wire plan mode into TUI, CLI, and ACP entry points Address the P1 finding that PermissionModePlan was documented but never selected by /plan, zero exec, or ACP mode selectors. /plan on|off now toggles the session permission mode (restoring the prior mode on off), zero exec --plan selects plan for a run, and ACP advertises plan as a client-selectable mode. Integration coverage for each entry path. * fix(cli): reject --plan combined with --worktree Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR #642. * test(tools): assert spoofed tool schema doesn't leak via tool_search The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR #642. * fix(tui): gate local mutating commands behind plan mode /plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR #642. * fix(agent,specialist): layer plan mode system prompt and enforce read-only subagent mode * Fix jatmn review findings for PR 642 * fix(agent,cli,tui): resolve CodeRabbit review comments on active turn model field and --plan permission mode conflict * fix(agent): propagate permission mode to exec options and serialize ACP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs #642 * fix(agent,tui): update tests off removed tools.CoreTools/NewWriteFileTool/NewLSPNavigateTool wrappers Those were thin unscoped wrappers around the Scoped variants, deleted upstream in #706 since nothing else called them directly. Only these tests still did; switch to the Scoped calls main's own tests already use. * fix(agent): fail-closed beforeTool vetoes and permission-mode plan guards Keep beforeTool deny gates active under plan mode so hooksSuppressed no longer fails open, and stop propagating --permission-mode for auto/ask/member children so swarm members keep write tools. Apply --plan combination rejects to --permission-mode plan as well. Refs #853 * fix(agent): address CodeRabbit findings for plan-mode advertisement and entry paths Unify plan/spec-draft tool advertisement in tools.ToolAdvertisedForPermissionMode (with PermissionDeny short-circuit), cover ACP config and --permission-mode plan list-tools paths, and allow bare /mcp while blocking mutating MCP subcommands. Refs #853 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: euxaristia <euxaristia@users.noreply.github.com>
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
5 participants
This PR adds the core agent support for PermissionModePlan. In this mode, the agent available tools are restricted to read-only capabilities, update_plan, and ask_user. This ensures the agent cannot mutate the workspace, execute commands, or run code changes while planning.
Summary by CodeRabbit
--planand TUI/plan [status|on|off], restoring the prior mode on exit; also supported in session mode switching.execbehavior/tool listings to reflect plan visibility.request_permissionsis blocked; plan runs suppress executable hook activity; denial messages now reference the active mode.tool_searchrespects permission mode (including deferred-tool filtering) and prevents schema/name leakage.tool_searchfiltering.