diff --git a/internal/acp/agent.go b/internal/acp/agent.go index b3fafb32a..7b9ad9870 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -349,9 +349,11 @@ func (a *Agent) handleSetMode(_ context.Context, params json.RawMessage) (any, e if sess == nil { return nil, RPCError(codeInvalidParams, "unknown session: "+p.SessionID) } + sess.turnMu.Lock() + defer sess.turnMu.Unlock() mode := agent.PermissionMode(p.ModeID) switch mode { - case agent.PermissionModeAuto, agent.PermissionModeAsk: + case agent.PermissionModeAuto, agent.PermissionModeAsk, agent.PermissionModePlan: sess.setMode(mode) (¬ifier{conn: a.conn, sessionID: sess.id}).currentMode(string(mode)) return SetSessionModeResult{}, nil @@ -381,9 +383,13 @@ func (a *Agent) handleSetConfigOption(_ context.Context, params json.RawMessage) return nil, err } case configIDMode: + // Same turnMu as handleSetMode so the two advertised mode doors (session + // set_mode and set_config_option) serialize mode flips consistently. + sess.turnMu.Lock() + defer sess.turnMu.Unlock() mode := agent.PermissionMode(p.Value) switch mode { - case agent.PermissionModeAuto, agent.PermissionModeAsk: + case agent.PermissionModeAuto, agent.PermissionModeAsk, agent.PermissionModePlan: sess.setMode(mode) (¬ifier{conn: a.conn, sessionID: sess.id}).currentMode(string(mode)) case agent.PermissionModeUnsafe: @@ -439,13 +445,16 @@ func (a *Agent) handleCancel(_ context.Context, params json.RawMessage) { // ---- advertising helpers ---- func (a *Agent) modeState(s *acpSession) *SessionModeState { - // Only auto/ask are offered over ACP; Unsafe is gated to the operator (see - // handleSetMode) so a client can't grant itself no-prompt host access. + // auto/ask/plan are offered over ACP; Unsafe is gated to the operator (see + // handleSetMode) so a client can't grant itself no-prompt host access. Plan + // only narrows what a client can do (read-only, no write/shell tools), so + // unlike Unsafe there is no elevation risk in letting a client select it. return &SessionModeState{ CurrentModeID: string(s.currentMode()), AvailableModes: []SessionMode{ {ID: string(agent.PermissionModeAuto), Name: "Auto", Description: "Run safe tools automatically; ask before risky ones."}, {ID: string(agent.PermissionModeAsk), Name: "Ask", Description: "Ask before every tool that changes state."}, + {ID: string(agent.PermissionModePlan), Name: "Plan", Description: "Read-only planning; write and shell tools are hidden."}, }, } } @@ -516,6 +525,7 @@ func (a *Agent) configOptions(s *acpSession) []SessionConfigOption { Options: []SessionConfigOptionValue{ {Value: string(agent.PermissionModeAuto), Name: "Auto", Description: "Run safe tools automatically; ask before risky ones."}, {Value: string(agent.PermissionModeAsk), Name: "Ask", Description: "Ask before every tool that changes state."}, + {Value: string(agent.PermissionModePlan), Name: "Plan", Description: "Read-only planning; write and shell tools are hidden."}, }, }} } diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index a9e07eccc..4fa97a258 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -393,6 +393,32 @@ func TestACPSetModeUpdatesSession(t *testing.T) { if got := configured.ConfigOptions[1].CurrentValue; got != string(agent.PermissionModeAuto) { t.Fatalf("configured mode = %q", got) } + // Plan is accepted: it only narrows capability (read-only), so unlike Unsafe + // there is no elevation risk in letting a client select it. + if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModePlan)}, &SetSessionModeResult{}); err != nil { + t.Fatalf("set_mode plan: %v", err) + } + // Configuration path is a separate contract from MethodSessionSetMode: the + // mode option is advertised via configOptions and applied by handleSetConfigOption. + var planConfigured SetSessionConfigOptionResult + if err := h.client.Call(ctx, MethodSessionSetConfigOption, SetSessionConfigOptionParams{ + SessionID: newRes.SessionID, ConfigID: configIDMode, Value: string(agent.PermissionModePlan), + }, &planConfigured); err != nil { + t.Fatalf("set_config_option plan: %v", err) + } + if got := planConfigured.ConfigOptions[1].CurrentValue; got != string(agent.PermissionModePlan) { + t.Fatalf("configured mode after plan = %q, want plan", got) + } + hasPlanOption := false + for _, opt := range planConfigured.ConfigOptions[1].Options { + if opt.Value == string(agent.PermissionModePlan) { + hasPlanOption = true + break + } + } + if !hasPlanOption { + t.Fatalf("config mode options missing plan: %#v", planConfigured.ConfigOptions[1].Options) + } // Unsafe must be rejected over ACP — a client can't self-grant no-prompt host access. if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModeUnsafe)}, &SetSessionModeResult{}); err == nil { t.Fatal("expected Unsafe mode to be rejected over ACP") @@ -403,6 +429,37 @@ func TestACPSetModeUpdatesSession(t *testing.T) { } } +// TestACPPlanModeWiresPermissionModeIntoAgentOptions confirms selecting "plan" +// over ACP actually reaches agent.Options.PermissionMode for the next turn — +// the same gap this test's TUI counterpart covers for /plan on. +func TestACPPlanModeWiresPermissionModeIntoAgentOptions(t *testing.T) { + deps := testDeps(t) + var captured agent.Options + deps.RunAgent = func(_ context.Context, _ string, _ zeroruntime.Provider, opts agent.Options) (agent.Result, error) { + captured = opts + return agent.Result{FinalAnswer: "ok"}, nil + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var newRes NewSessionResult + if err := h.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: t.TempDir(), McpServers: []McpServer{}}, &newRes); err != nil { + t.Fatalf("session/new: %v", err) + } + if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModePlan)}, &SetSessionModeResult{}); err != nil { + t.Fatalf("set_mode plan: %v", err) + } + if err := h.client.Call(ctx, MethodSessionPrompt, PromptParams{SessionID: newRes.SessionID, Prompt: []ContentBlock{TextBlock("plan it out")}}, &PromptResult{}); err != nil { + t.Fatalf("session/prompt: %v", err) + } + if captured.PermissionMode != agent.PermissionModePlan { + t.Fatalf("agent.Options.PermissionMode = %q, want plan", captured.PermissionMode) + } +} + // TestACPRunTurnWiresSandboxAndScopedRegistry proves the sandbox engine and the // scoped registry from BuildWorkspace actually reach agent.Options — i.e. ACP // shell tools run confined, not unconfined on the host. diff --git a/internal/agent/deferred_loop_test.go b/internal/agent/deferred_loop_test.go index 895b4dd8f..6452e0ef9 100644 --- a/internal/agent/deferred_loop_test.go +++ b/internal/agent/deferred_loop_test.go @@ -797,3 +797,98 @@ func TestDisabledToolSearchFallsBackToEager(t *testing.T) { t.Fatalf("deferred tool must be callable under eager fallback, got status=%s output=%q", result.Status, result.Output) } } + +// 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 } + +func (t fakeDeferredMutatorTool) Name() string { return t.name } +func (t fakeDeferredMutatorTool) Description() string { return "mutates the workspace, deferred" } +func (t fakeDeferredMutatorTool) Parameters() tools.Schema { + return tools.Schema{Type: "object", AdditionalProperties: false} +} +func (t fakeDeferredMutatorTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionPrompt, Reason: "mutates"} +} +func (t fakeDeferredMutatorTool) Run(_ context.Context, _ map[string]any) tools.Result { + return tools.Result{Status: tools.StatusOK, Output: "mutated"} +} +func (t fakeDeferredMutatorTool) Deferred() bool { return true } + +// TestPlanModeToolSearchNeverLeaksDeferredMutatorSchema guards the concern +// raised in PR #642 review (jatmn, P2): that tool_search filters deferred +// candidates only by EnabledTools/DisabledTools, not by plan-mode visibility, +// so a plan-mode model could call `tool_search select:` +// and receive that tool's full schema even though a direct call to it is +// correctly denied the following turn. +// +// tool_search's own Safety is SideEffectNone, and ToolAdvertisedForPermissionMode (the +// same gate executeToolCall uses to deny a direct call) requires +// SideEffect==Read to advertise a tool in plan mode. That means tool_search +// itself is never advertised, never activates deferral (loaderUsable in +// partitionToolsCached requires ToolAdvertised(loader, permissionMode)), and +// is denied at dispatch like any other hidden tool if a stale/adversarial +// call reaches it anyway — so a deferred mutator's schema can never reach the +// model through tool_search while in plan mode. This test pins that +// end-to-end: tool_search is absent from the advertised tool list, and a +// forced call to it is denied before rendering any tool schema. +func TestPlanModeToolSearchNeverLeaksDeferredMutatorSchema(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + registry.Register(fakeDeferredMutatorTool{name: "mcp__srv__mutate"}) + registry.Register(fakeDeferredMutatorTool{name: "mcp__srv__mutate2"}) + registry.Register(tools.NewToolSearchTool(registry)) + + provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{ + { // turn 1: force a call to tool_search even though it should not be advertised. + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "c1", ToolName: tools.ToolSearchToolName}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "c1", ArgumentsFragment: `{"query":"select:mcp__srv__mutate"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "c1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { // turn 2: final answer. + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }} + + result, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + DeferThreshold: 2, // 2 deferred mutators registered => eligible for deferral. + MaxTurns: 2, + }) + if err != nil { + t.Fatal(err) + } + + // tool_search (and the deferred mutators) must not be advertised in plan + // mode's turn 1 tool list at all. + for _, def := range provider.requests[0].Tools { + if def.Name == tools.ToolSearchToolName { + t.Fatalf("plan mode must not advertise tool_search, got %#v", provider.requests[0].Tools) + } + if def.Name == "mcp__srv__mutate" || def.Name == "mcp__srv__mutate2" { + t.Fatalf("plan mode must not advertise a deferred mutator, got %#v", provider.requests[0].Tools) + } + } + + // The forced call must be denied outright, never a loaded-schema result: + // the tool result must not mention the mutator's name or carry a + // load_tools signal. + var toolMessage string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + toolMessage = message.Content + break + } + } + if !strings.Contains(toolMessage, "not available in plan mode") { + t.Fatalf("expected tool_search call denied in plan mode, got %q", toolMessage) + } + if strings.Contains(toolMessage, "mcp__srv__mutate") { + t.Fatalf("denial must not leak the deferred mutator's name/schema, got %q", toolMessage) + } +} diff --git a/internal/agent/loop.go b/internal/agent/loop.go index c6cd6092d..79d4965e4 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1095,12 +1095,13 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal }, nil } tool, toolFound := registry.Get(call.Name) - if permissionMode == PermissionModeSpecDraft && toolFound && !ToolAdvertised(tool, permissionMode) { + if (permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan) && toolFound && !ToolAdvertised(tool, permissionMode) { + modeName := string(permissionMode) return ToolResult{ ToolCallID: call.ID, Name: call.Name, Status: tools.StatusError, - Output: `Error: Tool "` + call.Name + `" is not available in spec-draft mode.`, + Output: `Error: Tool "` + call.Name + `" is not available in ` + modeName + ` mode.`, DenialReason: DenialFiltered, }, nil } @@ -1777,9 +1778,31 @@ func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolR } } +// hooksSuppressed reports whether advisory (non-veto) hooks must not run for +// this run's permission mode. Plan mode promises a read-only turn, but +// sessionStart/sessionEnd/afterTool hooks execute configured host commands +// outside the advertised-tool and sandbox gates, so dispatching them would let +// merely starting a plan session or finishing a read mutate the workspace. +// +// beforeTool is intentionally NOT suppressed: a non-zero exit is a deny gate, +// and skipping it fails open (operators who block secret-file reads via +// beforeTool would lose that protection under /plan on). See dispatchBeforeTool. +// +// Spec-draft keeps the existing trust-gated hook model: project hooks still +// fire when the workspace (or its worktree trust root) is trusted. That is +// intentional; trust inheritance for --use-spec --worktree is covered by +// TestExecSpecWorktreeInheritsTrustEndToEnd. +func hooksSuppressed(options Options) bool { + return options.PermissionMode == PermissionModePlan +} + // dispatchBeforeTool runs configured beforeTool hooks for a tool call. A hook // that exits non-zero vetoes the call: the returned bool is true and the tool // must not run. A nil dispatcher (no hooks wired) is a no-op. +// +// Unlike advisory hooks, beforeTool still runs under plan mode. Suppressing it +// would fail open: a project policy that blocks reads of secrets via +// beforeTool would silently stop applying the moment permission mode is plan. func dispatchBeforeTool(ctx context.Context, options Options, call ToolCall, args map[string]any) (hooks.DispatchOutcome, bool) { if options.Hooks == nil { return hooks.DispatchOutcome{}, false @@ -1804,7 +1827,7 @@ func dispatchBeforeTool(ctx context.Context, options Options, call ToolCall, arg // returns any advisory output (e.g. a formatter or vet result) to surface back // to the model. afterTool hooks never block. A nil dispatcher is a no-op. func dispatchAfterTool(ctx context.Context, options Options, call ToolCall, args map[string]any, result tools.Result) string { - if options.Hooks == nil { + if options.Hooks == nil || hooksSuppressed(options) { return "" } outcome := options.Hooks.Dispatch(ctx, hooks.DispatchInput{ @@ -1828,7 +1851,7 @@ func dispatchAfterTool(ctx context.Context, options Options, call ToolCall, args // model turn. Lifecycle hooks are advisory: dispatcher failures are audited but // never block the run. func dispatchSessionStart(ctx context.Context, options Options) { - if options.Hooks == nil { + if options.Hooks == nil || hooksSuppressed(options) { return } options.Hooks.Dispatch(ctx, hooks.DispatchInput{ @@ -1849,7 +1872,7 @@ func dispatchSessionStart(ctx context.Context, options Options) { // dispatchSessionEnd runs configured sessionEnd hooks once when the agent run // exits, including early error returns. Lifecycle hooks are advisory. func dispatchSessionEnd(ctx context.Context, options Options, result Result, runErr error) { - if options.Hooks == nil { + if options.Hooks == nil || hooksSuppressed(options) { return } payload := map[string]any{ @@ -2148,6 +2171,20 @@ type requestPermissionsArgs struct { } func executeRequestPermissions(ctx context.Context, call ToolCall, args map[string]any, permissionMode PermissionMode, options Options) (ToolResult, error) { + // request_permissions is dispatched by name above, before the registry-based + // ToolAdvertised gate runs, so a read-only mode's registry omitting this tool + // (rather than registering it as denied) must not fall through to a real + // grant. Deny it here unconditionally for spec-draft/plan, independent of + // whether the caller's registry happens to contain the tool. + if permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan { + return ToolResult{ + ToolCallID: call.ID, + Name: call.Name, + Status: tools.StatusError, + Output: `Error: Tool "` + call.Name + `" is not available in ` + string(permissionMode) + ` mode.`, + DenialReason: DenialFiltered, + }, nil + } parsed, err := parseRequestPermissionsArgs(args) if err != nil { return ToolResult{ @@ -3146,11 +3183,17 @@ func propertyToRuntimeMap(property tools.PropertySchema) map[string]any { } func ToolAdvertised(tool tools.Tool, permissionMode PermissionMode) bool { + // Denied tools are never advertised in any mode. Keep this short-circuit + // here so auto/member-auto/ask/unsafe all honor it before mode branches; + // tools.ToolAdvertisedForPermissionMode repeats it for the tool_search path + // which never enters this function. if tool.Safety().Permission == tools.PermissionDeny { return false } - if permissionMode == PermissionModeSpecDraft { - return toolAdvertisedInSpecDraft(tool) + // plan/spec-draft policy lives in tools so tool_search and the dispatch + // gate cannot drift. PermissionDeny was already checked above. + if permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan { + return tools.ToolAdvertisedForPermissionMode(tool, string(permissionMode)) } if permissionMode == PermissionModeAuto { return tool.Safety().Permission == tools.PermissionAllow || tool.Safety().AdvertiseInAuto @@ -3173,17 +3216,6 @@ func ToolAdvertised(tool tools.Tool, permissionMode PermissionMode) bool { return true } -func toolAdvertisedInSpecDraft(tool tools.Tool) bool { - switch tool.Name() { - case "ask_user", "submit_spec": - return true - case "update_plan": - return false - } - safety := tool.Safety() - return safety.SideEffect == tools.SideEffectRead && safety.Permission == tools.PermissionAllow -} - func stopReasonFromToolResult(result ToolResult) StopReason { if result.Meta == nil { return "" diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 1aeba7071..03ced456b 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -3391,6 +3391,378 @@ func TestSpecDraftDeniesBashToolCalls(t *testing.T) { } } +func TestPlanModeAdvertisesOnlySafeTools(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + for _, tool := range tools.CoreToolsScoped(root, nil) { + registry.Register(tool) + } + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{{ + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }}, + } + + _, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + }) + if err != nil { + t.Fatal(err) + } + names := map[string]bool{} + for _, definition := range provider.requests[0].Tools { + names[definition.Name] = true + } + for _, want := range []string{"read_file", "list_directory", "glob", "grep", "skill", "ask_user", "update_plan"} { + if !names[want] { + t.Fatalf("plan mode tools missing %q from %#v", want, names) + } + } + for _, denied := range []string{"write_file", "edit_file", "apply_patch", "bash", "web_fetch", "lsp_navigate"} { + if names[denied] { + t.Fatalf("plan mode advertised denied tool %q in %#v", denied, names) + } + } +} + +// spoofedSafetyTool lets a test register a tool under a name the plan allowlist +// historically treated specially (ask_user, update_plan) but with attacker-chosen +// Safety, simulating a caller that overwrites the real tool: Registry.Register +// keys purely on Name(), so nothing stops a re-registration under the same name. +type spoofedSafetyTool struct { + name string + safety tools.Safety + run func(ctx context.Context, args map[string]any) tools.Result +} + +func (tool spoofedSafetyTool) Name() string { return tool.name } +func (tool spoofedSafetyTool) Description() string { return "spoofed tool for test" } +func (tool spoofedSafetyTool) Parameters() tools.Schema { return tools.Schema{Type: "object"} } +func (tool spoofedSafetyTool) Safety() tools.Safety { return tool.safety } +func (tool spoofedSafetyTool) Run(ctx context.Context, args map[string]any) tools.Result { + return tool.run(ctx, args) +} + +// TestSpecDraftModeRejectsNameOnlySpoofedControlTools guards against +// tools.ToolAdvertisedForPermissionMode trusting the names "ask_user"/"submit_spec" +// alone: a re-registered tool with the wrong Safety shape must be neither +// advertised nor executed in spec-draft mode. +func TestSpecDraftModeRejectsNameOnlySpoofedControlTools(t *testing.T) { + cases := []struct { + name string + safety tools.Safety + }{ + {name: "ask_user", safety: tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionAllow, Reason: "spoof"}}, + {name: "submit_spec", safety: tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionAllow, Reason: "spoof"}}, + {name: "ask_user", safety: tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionDeny, Reason: "spoof"}}, + {name: "submit_spec", safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionDeny, Reason: "spoof"}}, + } + for _, tc := range cases { + t.Run(tc.name+"/"+string(tc.safety.SideEffect)+"/"+string(tc.safety.Permission), func(t *testing.T) { + written := filepath.Join(t.TempDir(), "spoofed.txt") + registry := tools.NewRegistry() + registry.Register(spoofedSafetyTool{ + name: tc.name, + safety: tc.safety, + 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"} + }, + }) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: tc.name}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + result, err := Run(context.Background(), "spec", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeSpecDraft, + MaxTurns: 2, + }) + if err != nil { + t.Fatal(err) + } + for _, definition := range provider.requests[0].Tools { + if definition.Name == tc.name { + t.Fatalf("spec-draft advertised spoofed %s with safety %+v", tc.name, tc.safety) + } + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available") { + t.Fatalf("expected spoofed %s denial, got %q", tc.name, denied) + } + if _, err := os.Stat(written); !os.IsNotExist(err) { + t.Fatalf("spoofed %s should not have run, stat err=%v", tc.name, err) + } + }) + } +} + +// TestPlanModeRejectsNameOnlySpoofedControlTools guards against +// tools.ToolAdvertisedForPermissionMode trusting the name "update_plan"/"ask_user" alone: a tool +// registered under either name with mutating Safety must be neither advertised +// nor executed in plan mode. +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"} + }, + }) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: name}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + + result, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + }) + if err != nil { + t.Fatal(err) + } + for _, definition := range provider.requests[0].Tools { + if definition.Name == name { + t.Fatalf("plan mode advertised a spoofed %s carrying mutating Safety", name) + } + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available in plan mode") { + t.Fatalf("expected spoofed %s denial, got %q", name, denied) + } + if _, err := os.Stat(written); !os.IsNotExist(err) { + t.Fatalf("spoofed %s should not have run, stat err=%v", name, err) + } + }) + } +} + +// TestPlanModeDeniesLSPNavigateToolCalls locks the process-spawning boundary: +// lsp_navigate is classified SideEffectRead but lazily starts a language server +// via exec. Even if the model still emits a call (e.g. from a prior turn's +// tool list), plan mode must deny it before Run can spawn anything. +func TestPlanModeDeniesLSPNavigateToolCalls(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewScopedLSPNavigateTool(root, nil)) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "lsp_navigate"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"op":"definition","path":"main.go","line":1,"character":1}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + + result, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + }) + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("expected final answer after denial, got %q", result.FinalAnswer) + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available in plan mode") { + t.Fatalf("expected plan mode lsp_navigate denial, got %q", denied) + } +} + +func TestPlanModeDeniesHiddenToolCalls(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewScopedWriteFileTool(root, nil)) + provider := providerCallingWriteFileThenAnswer("done") + + result, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + }) + + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("expected final answer after denial, got %q", result.FinalAnswer) + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available in plan mode") { + t.Fatalf("expected plan mode denial, got %q", denied) + } + if _, err := os.Stat(filepath.Join(root, "notes.txt")); !os.IsNotExist(err) { + t.Fatalf("write_file should not have written notes.txt, stat err=%v", err) + } +} + +func TestPlanModeDeniesBashToolCalls(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewBashTool(root)) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "bash"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"command":"printf ran > ran.txt"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + + result, err := Run(context.Background(), "plan", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + }) + + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("expected final answer after denial, got %q", result.FinalAnswer) + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available in plan mode") { + t.Fatalf("expected plan mode bash denial, got %q", denied) + } + if _, err := os.Stat(filepath.Join(root, "ran.txt")); !os.IsNotExist(err) { + t.Fatalf("bash should not have written ran.txt, stat err=%v", err) + } +} + +// TestReadOnlyModesDenyRequestPermissionsEvenWhenRegistryOmitsIt guards against +// the read-only gate's blind spot: request_permissions is dispatched by name in +// executeToolCall before the registry-based ToolAdvertised check runs, so a +// plan/spec-draft registry that simply omits the tool (rather than registering +// it as denied) must not let the call fall through to a real grant. +func TestReadOnlyModesDenyRequestPermissionsEvenWhenRegistryOmitsIt(t *testing.T) { + for _, mode := range []PermissionMode{PermissionModePlan, PermissionModeSpecDraft} { + t.Run(string(mode), func(t *testing.T) { + root := t.TempDir() + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "grant-1", ToolName: tools.RequestPermissionsToolName}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "grant-1", ArgumentsFragment: `{"reason":"need access","permissions":{"file_system":{"write":["/tmp/outside"]}}}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "grant-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + var promptCount int + + result, err := Run(context.Background(), "task", provider, Options{ + Registry: registry, + PermissionMode: mode, + MaxTurns: 2, + OnPermissionRequest: func(context.Context, PermissionRequest) (PermissionDecision, error) { + promptCount++ + return PermissionDecision{Action: PermissionDecisionAllow, Reason: "ok"}, nil + }, + }) + + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("expected final answer after denial, got %q", result.FinalAnswer) + } + if promptCount != 0 { + t.Fatalf("request_permissions must be denied before reaching OnPermissionRequest, got %d prompts", promptCount) + } + var denied string + for _, message := range result.Messages { + if message.Role == zeroruntime.MessageRoleTool { + denied = message.Content + break + } + } + if !strings.Contains(denied, "not available in "+string(mode)+" mode") { + t.Fatalf("expected %s mode denial for request_permissions, got %q", mode, denied) + } + }) + } +} + func TestRunStopsWhenSubmitSpecReturnsReviewControl(t *testing.T) { root := t.TempDir() registry := tools.NewRegistry() @@ -3756,3 +4128,171 @@ func TestRunNilTraceForwardsUsage(t *testing.T) { t.Fatal("OnUsage not forwarded when Trace is nil") } } + +// TestRunSuppressesAdvisoryHooksInPlanMode: plan mode promises a read-only +// turn for advisory hooks (sessionStart/sessionEnd/afterTool), which execute +// configured host commands outside the advertised-tool and sandbox gates. +// beforeTool is deliberately still dispatched so deny policies keep working; +// see TestPlanModeHonorsBeforeToolVeto. +func TestRunSuppressesAdvisoryHooksInPlanMode(t *testing.T) { + goBinary, err := exec.LookPath("go") + if err != nil { + goRoot := runtime.GOROOT() //nolint:staticcheck // Safe for this non-portable test binary. + goBinary = filepath.Join(goRoot, "bin", "go") + if runtime.GOOS == "windows" { + goBinary += ".exe" + } + if _, statErr := os.Stat(goBinary); statErr != nil { + t.Skipf("go binary unavailable on PATH or in GOROOT: %v", statErr) + } + } + audit, err := hooks.NewAuditStore(hooks.AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + sessionMarker := filepath.Join(t.TempDir(), "session-marker-dir") + afterToolMarker := filepath.Join(t.TempDir(), "after-tool-marker-dir") + // beforeTool allows the read (exit 0) so the tool still runs and afterTool + // would fire if it were not suppressed. + dispatcher := hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.session-start", Event: hooks.EventSessionStart, Command: goBinary, Args: []string{"mod", "init", "-modfile", filepath.Join(sessionMarker, "go.mod"), "marker"}, Enabled: true}, + {ID: "zero.session-end", Event: hooks.EventSessionEnd, Command: goBinary, Args: []string{"version"}, Enabled: true}, + {ID: "zero.before-tool", Event: hooks.EventBeforeTool, Matcher: "read_file", Command: goBinary, Args: []string{"version"}, Enabled: true}, + {ID: "zero.after-tool", Event: hooks.EventAfterTool, Matcher: "read_file", Command: goBinary, Args: []string{"mod", "init", "-modfile", filepath.Join(afterToolMarker, "go.mod"), "marker"}, Enabled: true}, + }, + }, + Audit: audit, + }) + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "notes.txt"), []byte("hello"), 0o644); err != nil { + t.Fatalf("write notes.txt: %v", err) + } + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "read_file"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"path":"notes.txt"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "plan drafted"}, + {Type: zeroruntime.StreamEventDone}, + }, + }} + + if _, err := Run(context.Background(), "plan something", provider, Options{ + SessionID: "session-plan", + Cwd: root, + Registry: registry, + ProviderName: "test-provider", + Model: "test-model", + Hooks: dispatcher, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + }); err != nil { + t.Fatalf("Run: %v", err) + } + + events, err := audit.ReadEvents() + if err != nil { + t.Fatalf("ReadEvents: %v", err) + } + sawBeforeTool := false + for _, event := range events { + if event.Type != "hook_execution_started" { + continue + } + switch event.Event { + case hooks.EventBeforeTool: + sawBeforeTool = true + case hooks.EventSessionStart, hooks.EventSessionEnd, hooks.EventAfterTool: + t.Fatalf("advisory hook %q executed during a plan-mode run", event.Event) + } + } + if !sawBeforeTool { + t.Fatal("expected beforeTool to still dispatch under plan mode (deny-gate must not fail open)") + } + for _, marker := range []string{sessionMarker, afterToolMarker} { + if _, statErr := os.Stat(marker); !os.IsNotExist(statErr) { + t.Fatalf("plan-mode run let advisory hook touch the filesystem via %q: %v", marker, statErr) + } + } +} + +// TestPlanModeHonorsBeforeToolVeto guards the fail-open hole where hooksSuppressed +// used to skip beforeTool under plan mode, so a deny-policy hook that blocks +// secret reads in auto mode would silently allow them under PermissionModePlan. +func TestPlanModeHonorsBeforeToolVeto(t *testing.T) { + goBinary, err := exec.LookPath("go") + if err != nil { + goRoot := runtime.GOROOT() //nolint:staticcheck // Safe for this non-portable test binary. + goBinary = filepath.Join(goRoot, "bin", "go") + if runtime.GOOS == "windows" { + goBinary += ".exe" + } + if _, statErr := os.Stat(goBinary); statErr != nil { + t.Skipf("go binary unavailable on PATH or in GOROOT: %v", statErr) + } + } + // A non-zero exit from beforeTool is a veto. "go definitely-not-a-subcommand" + // exits non-zero on every platform with a go toolchain. + dispatcher := hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.veto", Event: hooks.EventBeforeTool, Matcher: "read_file", Command: goBinary, Args: []string{"definitely-not-a-go-subcommand"}, Enabled: true}, + }, + }, + }) + root := t.TempDir() + secret := filepath.Join(root, "secret.txt") + if err := os.WriteFile(secret, []byte("SUPERSECRET"), 0o644); err != nil { + t.Fatalf("write secret.txt: %v", err) + } + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + var toolOutputs []string + provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "read_file"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"path":"secret.txt"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "blocked"}, + {Type: zeroruntime.StreamEventDone}, + }, + }} + + if _, err := Run(context.Background(), "read the secret", provider, Options{ + SessionID: "session-plan-veto", + Cwd: root, + Registry: registry, + ProviderName: "test-provider", + Model: "test-model", + Hooks: dispatcher, + PermissionMode: PermissionModePlan, + MaxTurns: 2, + OnToolResult: func(result ToolResult) { + toolOutputs = append(toolOutputs, result.Output) + }, + }); err != nil { + t.Fatalf("Run: %v", err) + } + if len(toolOutputs) == 0 { + t.Fatal("expected a tool result for the vetoed read_file call") + } + combined := strings.Join(toolOutputs, "\n") + if strings.Contains(combined, "SUPERSECRET") { + t.Fatalf("plan mode failed open: beforeTool veto was skipped and secret leaked: %q", combined) + } + if !strings.Contains(combined, "blocked") && !strings.Contains(combined, "zero.veto") && !strings.Contains(strings.ToLower(combined), "hook") { + t.Fatalf("expected tool result to mention the beforeTool veto, got %q", combined) + } +} diff --git a/internal/agent/system_prompt.go b/internal/agent/system_prompt.go index 2572370f4..4d3616eb8 100644 --- a/internal/agent/system_prompt.go +++ b/internal/agent/system_prompt.go @@ -118,6 +118,9 @@ func buildSystemPromptParts(options Options) systemPromptParts { if project != "" { sections = append(sections, project) } + if modeCtx := permissionModeContext(options); modeCtx != "" { + sections = append(sections, modeCtx) + } if delegation := specialistDelegationContext(options); delegation != "" { sections = append(sections, delegation) } @@ -679,3 +682,12 @@ func gitBranchForPrompt(cwd string) string { } return ref } + +func permissionModeContext(options Options) string { + switch options.PermissionMode { + case PermissionModePlan: + return "Plan mode is active on this session. Your role is read-only exploration and planning: inspect the workspace and shape the plan with update_plan, but do not make changes to files or execute commands." + default: + return "" + } +} diff --git a/internal/agent/types.go b/internal/agent/types.go index cfee5b20e..de5ec72bb 100644 --- a/internal/agent/types.go +++ b/internal/agent/types.go @@ -26,6 +26,15 @@ const ( PermissionModeAsk PermissionMode = "ask" PermissionModeUnsafe PermissionMode = "unsafe" PermissionModeSpecDraft PermissionMode = "spec-draft" + // PermissionModePlan is an interactive, read-only planning mode. It applies + // to the CURRENT session (unlike spec-draft, which drafts in a separate + // session): the agent may inspect the workspace and shape the plan with + // update_plan/ask_user, but no mutating tool is advertised, so it cannot + // write files, run shell, or implement while planning. Entry points: + // the TUI's /plan on (exit with /plan off, which restores whatever mode + // was active before), `zero exec --plan`, and the ACP session mode + // selector ("plan"). + PermissionModePlan PermissionMode = "plan" // PermissionModeMemberAuto is a headless mode for swarm/specialist MEMBERS: it // advertises the in-workspace mutators a member needs to build (write/edit + // shell) on top of the Auto set, while the sandbox engine still gates them at diff --git a/internal/cli/app.go b/internal/cli/app.go index 4d6b11aab..1c5640ad3 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -1361,6 +1361,10 @@ Flags: --spec-model Override the draft model when --use-spec is set --spec-reasoning-effort Override draft reasoning effort when --use-spec is set + --plan Read-only planning mode: write and shell tools are hidden + --permission-mode Set permission mode directly (plan, spec-draft, auto, member, + ask, unsafe). Outranks --auto; prefer --plan / --auto for + interactive use. Used by specialist/swarm child processes. --max-turns Override the maximum agent loop turns --exec-profile Apply an execution profile (balanced, fast, thorough): loop posture only (turn budget, effort, self-correction, escalation); diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 9c0326876..ca65799bb 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -24,7 +24,8 @@ var completionRoot = completionNode{ children: []completionNode{ {names: []string{"exec"}, flags: []string{ "-h", "--help", "-f", "--file", "--image", "--add-dir", "--mode", "-m", "--model", - "--use-spec", "--spec-model", "--spec-reasoning-effort", "--max-turns", "--exec-profile", + "--use-spec", "--spec-model", "--spec-reasoning-effort", "--plan", "--permission-mode", + "--max-turns", "--exec-profile", "--auto", "--enabled-tools", "--disabled-tools", "--list-tools", "--profile", "-r", "--reasoning-effort", "-C", "--cwd", "-w", "--worktree", "--worktree-dir", "-i", "--input-format", "-o", "--output-format", "--prompt", "--resume", "--fork", diff --git a/internal/cli/exec.go b/internal/cli/exec.go index 2d1fe542a..fe4ec32a5 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -78,11 +78,18 @@ type execOptions struct { // fill-only-if-unset rule, so precedence is explicit flag > mode > profile. // Distinct from the mode preset also named "fast" (which picks a model) and // from the legacy inert --profile above. See internal/execprofile. - execProfile string - reasoningEffort string - useSpec bool - specModel string - specReasoningEffort string + execProfile string + reasoningEffort string + useSpec bool + specModel string + specReasoningEffort string + // plan selects PermissionModePlan for this run: the same read-only, + // in-session planning mode the TUI enters with /plan on. Unlike --use-spec + // (a separate draft session with its own review flow), this only swaps the + // permission mode for the run already being made — ToolAdvertised gates + // write/shell tools generically for any mode, so no other exec plumbing + // needs to know about it. + plan bool maxTurns int cwd string inputFormat execInputFormat @@ -104,6 +111,7 @@ type execOptions struct { worktreeName string worktreeDir string skipPermissionsUnsafe bool + permissionMode string // allowEscalation opts the run into mid-run model escalation: it registers // the escalate_model tool and wires agent.Options.ModelSwitcher. Off by // default — a run without the flag is byte-identical to before (no tool, nil @@ -240,6 +248,9 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in if options.useSpec { permissionMode = agent.PermissionModeSpecDraft } + if options.plan { + permissionMode = agent.PermissionModePlan + } var mcpRuntime mcpToolRuntime var mcpSkip trustSkip // Make local plugins live for this run: register their declared tools into the @@ -306,11 +317,13 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in _, _ = fmt.Fprintln(stderr, "[zero] "+notice) } executionRunner.SetPreparer(sandboxEngine) - mcpRuntime, mcpSkip, err = registerMCPToolsForWorkspace(context.Background(), workspaceRoot, registry, deps, execMCPAutonomy(options), trustRoot, executionRunner) - if err != nil { - return writeExecProviderError(stdout, stderr, options.outputFormat, "mcp_error", err.Error()) + if permissionMode != agent.PermissionModePlan { + mcpRuntime, mcpSkip, err = registerMCPToolsForWorkspace(context.Background(), workspaceRoot, registry, deps, execMCPAutonomy(options), trustRoot, executionRunner) + if err != nil { + return writeExecProviderError(stdout, stderr, options.outputFormat, "mcp_error", err.Error()) + } + defer closeMCPRuntime(stderr, mcpRuntime) } - defer closeMCPRuntime(stderr, mcpRuntime) pluginActivation = activatePlugins(workspaceRoot, registry, deps, stderr, trustRoot, executionRunner) registerLocalControlTools(registry, workspaceRoot, resolved.LocalControl) if err := validateExecToolFilters(options, registry); err != nil { @@ -578,8 +591,12 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in // warned, so --auto high silently ran without notice. if permissionMode == agent.PermissionModeUnsafe { reason := "--auto high" - if options.skipPermissionsUnsafe { + switch { + case options.skipPermissionsUnsafe: reason = "--skip-permissions-unsafe" + case strings.EqualFold(strings.TrimSpace(options.permissionMode), "unsafe"), + strings.EqualFold(strings.TrimSpace(options.permissionMode), "high"): + reason = "--permission-mode unsafe" } writer.warning(fmt.Sprintf("Unsafe permissions are active for this run because %s was passed.", reason)) if writer.err != nil { diff --git a/internal/cli/exec_parse.go b/internal/cli/exec_parse.go index 8eb55223d..6198bae6f 100644 --- a/internal/cli/exec_parse.go +++ b/internal/cli/exec_parse.go @@ -20,6 +20,19 @@ func parseExecArgs(args []string) (execOptions, bool, error) { switch { case arg == "-h" || arg == "--help" || arg == "help": return options, true, nil + case arg == "--permission-mode": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return options, false, err + } + options.permissionMode = strings.TrimSpace(value) + index = next + case strings.HasPrefix(arg, "--permission-mode="): + value, err := requiredInlineFlagValue(arg, "--permission-mode") + if err != nil { + return options, false, err + } + options.permissionMode = value case arg == "--skip-permissions-unsafe": options.skipPermissionsUnsafe = true case arg == "--list-tools": @@ -171,6 +184,8 @@ func parseExecArgs(args []string) (execOptions, bool, error) { options.reasoningEffort = strings.TrimSpace(strings.TrimPrefix(arg, "--reasoning-effort=")) case arg == "--use-spec": options.useSpec = true + case arg == "--plan": + options.plan = true case arg == "--spec-model": value, next, err := nextFlagValue(args, index, arg) if err != nil { @@ -437,6 +452,27 @@ func parseExecArgs(args []string) (execOptions, bool, error) { if !options.useSpec && options.specReasoningEffort != "" { return options, false, execUsageError{"--spec-reasoning-effort requires --use-spec."} } + // Plan mode may be entered via --plan or --permission-mode plan. Combination + // guards must key off both: otherwise --permission-mode plan bypasses the + // worktree/unsafe/use-spec rejects that --plan alone enforces, and worktree + // prep mutates the filesystem before the read-only mode is assigned. + planMode := options.plan || strings.EqualFold(strings.TrimSpace(options.permissionMode), "plan") + if planMode && options.useSpec { + return options, false, execUsageError{"Use either --plan or --use-spec, not both."} + } + if planMode && options.skipPermissionsUnsafe { + return options, false, execUsageError{"Use either --plan or --skip-permissions-unsafe, not both."} + } + if planMode && options.worktree { + // Worktree prep (copying/branching the workspace) runs before the plan + // permission mode is assigned, so it would happen even under plan mode's + // read-only, no-side-effects promise. Reject the combination outright + // rather than let a mutation slip in ahead of the mode gate. + return options, false, execUsageError{"--plan cannot be combined with --worktree."} + } + if options.plan && options.permissionMode != "" && strings.ToLower(options.permissionMode) != "plan" { + return options, false, execUsageError{"--plan cannot be combined with --permission-mode=" + options.permissionMode + "."} + } if options.initSessionID != "" && (options.resume != "" || options.resumeLatest) { return options, false, execUsageError{"Use --init-session-id only when creating or forking a session."} } diff --git a/internal/cli/exec_plan_test.go b/internal/cli/exec_plan_test.go new file mode 100644 index 000000000..165aa57e1 --- /dev/null +++ b/internal/cli/exec_plan_test.go @@ -0,0 +1,177 @@ +package cli + +import ( + "bytes" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/config" +) + +func TestParseExecArgsRecognizesPlanFlag(t *testing.T) { + options, _, err := parseExecArgs([]string{"--plan", "draft a plan"}) + if err != nil { + t.Fatalf("parseExecArgs: %v", err) + } + if !options.plan { + t.Fatal("expected --plan to set options.plan") + } +} + +func TestParseExecArgsRejectsPlanWithUseSpec(t *testing.T) { + _, _, err := parseExecArgs([]string{"--plan", "--use-spec", "draft a plan"}) + if err == nil || !strings.Contains(err.Error(), "not both") { + t.Fatalf("expected --plan/--use-spec validation, got %v", err) + } +} + +func TestParseExecArgsRejectsPlanWithSkipPermissionsUnsafe(t *testing.T) { + _, _, err := parseExecArgs([]string{"--plan", "--skip-permissions-unsafe", "draft a plan"}) + if err == nil || !strings.Contains(err.Error(), "not both") { + t.Fatalf("expected --plan/--skip-permissions-unsafe validation, got %v", err) + } +} + +// TestParseExecArgsRejectsPlanWithWorktree guards against worktree +// preparation (a filesystem mutation) running ahead of the plan mode gate: +// options.worktree is processed in runExec before the plan permission mode is +// assigned, so the combination must be rejected during option validation, +// before any worktree prep can occur. +func TestParseExecArgsRejectsPlanWithWorktree(t *testing.T) { + _, _, err := parseExecArgs([]string{"--plan", "--worktree", "draft a plan"}) + if err == nil || !strings.Contains(err.Error(), "--worktree") { + t.Fatalf("expected --plan/--worktree validation, got %v", err) + } +} + +func TestParseExecArgsRejectsPlanWithNonPlanPermissionMode(t *testing.T) { + _, _, err := parseExecArgs([]string{"--plan", "--permission-mode=ask", "draft a plan"}) + if err == nil || !strings.Contains(err.Error(), "--permission-mode") { + t.Fatalf("expected --plan/--permission-mode validation, got %v", err) + } + + options, _, err := parseExecArgs([]string{"--plan", "--permission-mode=plan", "draft a plan"}) + if err != nil { + t.Fatalf("expected --plan with --permission-mode=plan to succeed, got %v", err) + } + if !options.plan || options.permissionMode != "plan" { + t.Fatalf("expected options.plan=true and permissionMode=plan, got plan=%v mode=%q", options.plan, options.permissionMode) + } +} + +// TestParseExecArgsPermissionModePlanSharesCombinationGuards ensures the +// combination rejects that key off --plan also fire for --permission-mode plan, +// which reaches PermissionModePlan without setting options.plan. +func TestParseExecArgsPermissionModePlanSharesCombinationGuards(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + {"worktree", []string{"--permission-mode", "plan", "--worktree", "draft a plan"}, "--worktree"}, + {"use-spec", []string{"--permission-mode=plan", "--use-spec", "draft a plan"}, "not both"}, + {"skip-permissions-unsafe", []string{"--permission-mode", "plan", "--skip-permissions-unsafe", "draft a plan"}, "not both"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, _, err := parseExecArgs(tc.args) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("expected --permission-mode plan / %s validation containing %q, got %v", tc.name, tc.want, err) + } + }) + } + + // Bare --permission-mode plan (no conflicting flags) still parses. + options, _, err := parseExecArgs([]string{"--permission-mode", "plan", "draft a plan"}) + if err != nil { + t.Fatalf("expected bare --permission-mode plan to succeed, got %v", err) + } + if options.permissionMode != "plan" || options.plan { + t.Fatalf("expected permissionMode=plan and plan=false, got mode=%q plan=%v", options.permissionMode, options.plan) + } +} + +// TestRunExecPlanHidesWriteAndShellToolsFromListing drives the real --plan +// flag through runExec (via --list-tools, so no provider is needed) and +// confirms write_file and bash — advertised under every other mode covered by +// TestRunExecListToolsAppliesModeBeforeListing-style tests — are hidden, +// mirroring the TUI /plan on gating end to end from the CLI entry point. +func TestRunExecPlanHidesWriteAndShellToolsFromListing(t *testing.T) { + cwd := t.TempDir() + var stdout bytes.Buffer + var stderr bytes.Buffer + + exitCode := runWithDeps([]string{"exec", "--plan", "--list-tools"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { + return cwd, nil + }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + listing := stdout.String() + if !strings.Contains(listing, "Tools visible to model") { + t.Fatalf("expected --plan --list-tools to list tools, got %q", listing) + } + if !strings.Contains(listing, "read_file") { + t.Fatalf("expected plan mode to still list read_file, got %q", listing) + } + for _, hidden := range []string{"write_file", "edit_file", "apply_patch", "bash"} { + if strings.Contains(listing, hidden) { + t.Fatalf("expected --plan to hide %q from the tool listing, got %q", hidden, listing) + } + } +} + +// TestRunExecPermissionModePlanHidesWriteAndShellToolsFromListing covers the +// --permission-mode plan entry path, which does not set options.plan and must +// still hide write and shell tools the same way --plan does. +func TestRunExecPermissionModePlanHidesWriteAndShellToolsFromListing(t *testing.T) { + cwd := t.TempDir() + var stdout bytes.Buffer + var stderr bytes.Buffer + + exitCode := runWithDeps([]string{"exec", "--permission-mode", "plan", "--list-tools"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { + return cwd, nil + }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + listing := stdout.String() + if !strings.Contains(listing, "Tools visible to model") { + t.Fatalf("expected --permission-mode plan --list-tools to list tools, got %q", listing) + } + if !strings.Contains(listing, "read_file") { + t.Fatalf("expected plan mode to still list read_file, got %q", listing) + } + for _, hidden := range []string{"write_file", "edit_file", "apply_patch", "bash"} { + if strings.Contains(listing, hidden) { + t.Fatalf("expected --permission-mode plan to hide %q from the tool listing, got %q", hidden, listing) + } + } +} + +func TestResolveExecPermissionModePlanOverride(t *testing.T) { + options := execOptions{autonomy: "low", plan: true} + mode, err := resolveExecPermissionMode(options) + if err != nil { + t.Fatalf("resolveExecPermissionMode: %v", err) + } + // resolveExecPermissionMode itself only resolves --auto; the --plan override + // is applied by the caller (runExec) afterward, same as --use-spec. This + // pins the precondition: --plan must not interfere with autonomy resolution. + if mode != agent.PermissionModeAuto { + t.Fatalf("resolveExecPermissionMode with --plan = %q, want auto (override applied by the caller)", mode) + } +} diff --git a/internal/cli/exec_tools.go b/internal/cli/exec_tools.go index d019a4250..2d5a15de6 100644 --- a/internal/cli/exec_tools.go +++ b/internal/cli/exec_tools.go @@ -73,6 +73,24 @@ func toolListContains(names []string, want string) bool { } func resolveExecPermissionMode(options execOptions) (agent.PermissionMode, error) { + if pm := strings.ToLower(strings.TrimSpace(options.permissionMode)); pm != "" { + switch pm { + case "plan": + return agent.PermissionModePlan, nil + case "spec-draft", "spec_draft": + return agent.PermissionModeSpecDraft, nil + case "auto": + return agent.PermissionModeAuto, nil + case "member", "member-auto", "member_auto": + return agent.PermissionModeMemberAuto, nil + case "ask": + return agent.PermissionModeAsk, nil + case "unsafe", "high": + return agent.PermissionModeUnsafe, nil + default: + return "", execUsageError{fmt.Sprintf("Invalid permission mode %q. Expected plan, spec-draft, auto, member, ask, or unsafe.", options.permissionMode)} + } + } // Validate --auto first, regardless of --skip-permissions-unsafe, so an // invalid autonomy value is always rejected. (Previously the unsafe path // short-circuited before validation, letting "--auto bogus" slip through diff --git a/internal/specialist/exec.go b/internal/specialist/exec.go index 702360543..2941b9568 100644 --- a/internal/specialist/exec.go +++ b/internal/specialist/exec.go @@ -148,6 +148,10 @@ func specialistAutonomy(permissionMode string) string { // read-only "low". An unsafe parent still yields "high" (full unsafe), and a // non-member (Task specialist) is unchanged. Authority stays sandbox-confined. func memberAwareAutonomy(permissionMode string, member bool) string { + pm := strings.TrimSpace(permissionMode) + if pm == "plan" || pm == "spec-draft" { + return "low" + } autonomy := specialistAutonomy(permissionMode) if member && autonomy == "low" { return "member" @@ -155,6 +159,20 @@ func memberAwareAutonomy(permissionMode string, member bool) string { return autonomy } +// childPermissionModeFlag returns the --permission-mode value to pass a child +// process, or "" when the child's tool set must be driven by --auto alone. +// Only plan and spec-draft require the flag: resolveExecPermissionMode prefers +// --permission-mode over --auto, so forwarding auto/ask/member/unsafe would +// discard the member rung and widen headless ask children. +func childPermissionModeFlag(permissionMode string) string { + switch strings.TrimSpace(permissionMode) { + case "plan", "spec-draft": + return strings.TrimSpace(permissionMode) + default: + return "" + } +} + // permissionModeUnsafe mirrors agent.PermissionModeUnsafe without importing the // agent package (which would create an import cycle): exec resolves "--auto high" // to this mode. @@ -263,6 +281,15 @@ func (executor Executor) BuildArgs(input BuildArgsInput) (BuildArgsResult, error args = append(args, promptArgs...) args = appendModelArgs(args, input.Manifest, input.ParentModel, input.ParentReasoningEffort) args = append(args, "--auto", memberAwareAutonomy(input.PermissionMode, input.MemberAutonomy), "--output-format", "stream-json") + // Only plan/spec-draft need an explicit --permission-mode on the child. + // resolveExecPermissionMode short-circuits on --permission-mode and ignores + // --auto, so propagating auto/ask/member/unsafe would strip member write + // tools (member + --permission-mode auto) or widen headless ask children + // past the read-only --auto low tool set. Plan/spec-draft cannot be + // expressed via --auto alone, so they still require the flag. + if permMode := childPermissionModeFlag(input.PermissionMode); permMode != "" { + args = append(args, "--permission-mode", permMode) + } toolAllowlist, err := resolvedToolAllowlist(input.Manifest) if err != nil { return BuildArgsResult{}, err @@ -315,6 +342,11 @@ func (executor Executor) BuildResumeArgs(input BuildResumeArgsInput) (BuildArgsR args := []string{"exec", "--resume", sessionID} args = append(args, promptArgs...) args = append(args, "--auto", specialistAutonomy(input.PermissionMode), "--output-format", "stream-json") + // See BuildArgs: only plan/spec-draft propagate --permission-mode so --auto + // remains the authority for member/auto/ask/unsafe child resolution. + if permMode := childPermissionModeFlag(input.PermissionMode); permMode != "" { + args = append(args, "--permission-mode", permMode) + } toolAllowlist, err := resolvedToolAllowlist(input.Manifest) if err != nil { return BuildArgsResult{}, err diff --git a/internal/specialist/exec_test.go b/internal/specialist/exec_test.go index b0de1b6a1..63f13bf8b 100644 --- a/internal/specialist/exec_test.go +++ b/internal/specialist/exec_test.go @@ -116,6 +116,11 @@ func TestBuildArgsMemberAutonomyEmitsMember(t *testing.T) { if !containsSequence(res.Args, []string{"--auto", "member"}) { t.Fatalf("non-unsafe member must yield --auto member, got %v", res.Args) } + // --permission-mode auto would short-circuit resolveExecPermissionMode and + // strip the member rung; member children must rely on --auto alone. + if containsSequence(res.Args, []string{"--permission-mode"}) { + t.Fatalf("non-plan member must not emit --permission-mode (would override --auto member), got %v", res.Args) + } // Without the member flag, the same parent stays --auto low (unchanged). plain, err := executor.BuildArgs(BuildArgsInput{Manifest: manifest, Prompt: "p", PermissionMode: "auto"}) @@ -125,6 +130,9 @@ func TestBuildArgsMemberAutonomyEmitsMember(t *testing.T) { if !containsSequence(plain.Args, []string{"--auto", "low"}) || containsSequence(plain.Args, []string{"--auto", "member"}) { t.Fatalf("a plain specialist must stay --auto low, got %v", plain.Args) } + if containsSequence(plain.Args, []string{"--permission-mode"}) { + t.Fatalf("auto parent must not emit --permission-mode, got %v", plain.Args) + } // An unsafe member still runs --auto high, never downgraded to member. unsafe, err := executor.BuildArgs(BuildArgsInput{Manifest: manifest, Prompt: "p", PermissionMode: "unsafe", MemberAutonomy: true}) @@ -134,6 +142,79 @@ func TestBuildArgsMemberAutonomyEmitsMember(t *testing.T) { if !containsSequence(unsafe.Args, []string{"--auto", "high"}) { t.Fatalf("unsafe member must yield --auto high, got %v", unsafe.Args) } + if containsSequence(unsafe.Args, []string{"--permission-mode"}) { + t.Fatalf("unsafe parent must not emit --permission-mode, got %v", unsafe.Args) + } +} + +// TestBuildArgsPropagatesPermissionModeOnlyForPlanAndSpecDraft pins that +// --permission-mode is emitted only when --auto cannot express the mode +// (plan/spec-draft). Emitting it for auto/ask/member would either strip swarm +// member write tools or widen headless ask children past --auto low. +func TestBuildArgsPropagatesPermissionModeOnlyForPlanAndSpecDraft(t *testing.T) { + executor := Executor{NewSessionID: func() (string, error) { return "child", nil }} + manifest := Manifest{Metadata: Metadata{Name: "worker"}, SystemPrompt: "x", ResolvedTools: []string{"read_file", "write_file", "bash"}} + + for _, mode := range []string{"auto", "ask", "unsafe", "member", ""} { + res, err := executor.BuildArgs(BuildArgsInput{Manifest: manifest, Prompt: "p", PermissionMode: mode}) + if err != nil { + t.Fatalf("BuildArgs(%q): %v", mode, err) + } + if containsSequence(res.Args, []string{"--permission-mode"}) { + t.Fatalf("mode %q must not emit --permission-mode, got %v", mode, res.Args) + } + } + + for _, mode := range []string{"plan", "spec-draft"} { + res, err := executor.BuildArgs(BuildArgsInput{Manifest: manifest, Prompt: "p", PermissionMode: mode}) + if err != nil { + t.Fatalf("BuildArgs(%q): %v", mode, err) + } + if !containsSequence(res.Args, []string{"--permission-mode", mode}) { + t.Fatalf("mode %q must emit --permission-mode %s, got %v", mode, mode, res.Args) + } + if !containsSequence(res.Args, []string{"--auto", "low"}) { + t.Fatalf("mode %q must still emit --auto low, got %v", mode, res.Args) + } + } + + // Resume path uses the same rule. + resume, err := executor.BuildResumeArgs(BuildResumeArgsInput{ + SessionID: "child_session", Prompt: "p", Manifest: manifest, PermissionMode: "ask", + }) + if err != nil { + t.Fatalf("BuildResumeArgs(ask): %v", err) + } + if containsSequence(resume.Args, []string{"--permission-mode"}) { + t.Fatalf("resume ask must not emit --permission-mode, got %v", resume.Args) + } + planResume, err := executor.BuildResumeArgs(BuildResumeArgsInput{ + SessionID: "child_session", Prompt: "p", Manifest: manifest, PermissionMode: "plan", + }) + if err != nil { + t.Fatalf("BuildResumeArgs(plan): %v", err) + } + if !containsSequence(planResume.Args, []string{"--permission-mode", "plan"}) { + t.Fatalf("resume plan must emit --permission-mode plan, got %v", planResume.Args) + } +} + +func TestChildPermissionModeFlag(t *testing.T) { + cases := map[string]string{ + "": "", + "auto": "", + "ask": "", + "unsafe": "", + "member": "", + "plan": "plan", + "spec-draft": "spec-draft", + " plan ": "plan", + } + for in, want := range cases { + if got := childPermissionModeFlag(in); got != want { + t.Errorf("childPermissionModeFlag(%q) = %q, want %q", in, got, want) + } + } } func TestBuildArgsAutonomyHonorsPermissionMode(t *testing.T) { diff --git a/internal/tools/tool_search.go b/internal/tools/tool_search.go index bc34449b5..4d4d55ea3 100644 --- a/internal/tools/tool_search.go +++ b/internal/tools/tool_search.go @@ -70,10 +70,16 @@ func (tool toolSearchTool) RunWithOptions(_ context.Context, args map[string]any } query = strings.TrimSpace(query) - // Honor the run's operator filters so an operator-hidden deferred tool is - // invisible to tool_search: it never resolves via select:, never ranks for a - // keyword query, and is omitted from the no-match listing. - deferred := tool.visibleDeferredTools(options.EnabledTools, options.DisabledTools) + // Honor the run's operator filters AND the run's permission-mode visibility + // so an operator-hidden OR mode-hidden deferred tool is invisible to + // tool_search: it never resolves via select:, never ranks for a keyword + // query, and is omitted from the no-match listing. The mode filter matters + // even though tool_search itself is denied at dispatch in plan/spec-draft + // today (its own Safety carries no side effect, so it fails the same + // advertisement gate) — it is defense-in-depth so a deferred write/mutator + // tool's name, description, and schema can never leak through this loader + // if that outer gate is ever loosened independently of this one. + deferred := tool.visibleDeferredTools(options.EnabledTools, options.DisabledTools, options.PermissionMode) var matches []Tool if rest, ok := strings.CutPrefix(query, "select:"); ok { @@ -87,7 +93,7 @@ func (tool toolSearchTool) RunWithOptions(_ context.Context, args map[string]any // update_plan) otherwise falls through to a misleading "no tools matched" — // a common confusion that leaves weaker models looping. Redirect the model to // call those tools directly instead. - alreadyAvailable := tool.eagerToolsForQuery(query, options.EnabledTools, options.DisabledTools) + alreadyAvailable := tool.eagerToolsForQuery(query, options.EnabledTools, options.DisabledTools, options.PermissionMode) if len(matches) == 0 { if len(alreadyAvailable) > 0 { @@ -112,12 +118,15 @@ func (tool toolSearchTool) RunWithOptions(_ context.Context, args map[string]any } // visibleDeferredTools returns the registry's deferred-eligible tools that pass -// the operator allow/deny filters, sorted by name so keyword ranking and -// listings stay deterministic. A nil/empty filter pair admits every deferred -// tool (the pre-filter behavior). The allow/deny semantics mirror the agent's -// ToolAllowedByFilters: denied if in disabled; if enabled is non-empty, the tool -// must be listed in it. -func (tool toolSearchTool) visibleDeferredTools(enabled []string, disabled []string) []Tool { +// the operator allow/deny filters and the run's permission-mode visibility, +// sorted by name so keyword ranking and listings stay deterministic. A +// nil/empty filter pair admits every deferred tool on the operator axis (the +// pre-filter behavior); permissionMode "" admits every deferred tool on the +// mode axis (matches every mode but plan/spec-draft, which never restricted +// tool_search's candidates before this filter existed). The allow/deny +// semantics mirror the agent's ToolAllowedByFilters: denied if in disabled; if +// enabled is non-empty, the tool must be listed in it. +func (tool toolSearchTool) visibleDeferredTools(enabled []string, disabled []string, permissionMode string) []Tool { var deferred []Tool if tool.registry != nil { for _, candidate := range tool.registry.All() { @@ -127,6 +136,9 @@ func (tool toolSearchTool) visibleDeferredTools(enabled []string, disabled []str if !toolAllowedByFilters(candidate.Name(), enabled, disabled) { continue } + if !toolAdvertisedForPermissionMode(candidate, permissionMode) { + continue + } deferred = append(deferred, candidate) } } @@ -137,9 +149,10 @@ func (tool toolSearchTool) visibleDeferredTools(enabled []string, disabled []str } // visibleEagerToolNames returns the names of tools the model ALREADY has in its -// list this run: registered, NOT deferred, passing the operator filters, and not -// tool_search itself. These never need (and never resolve through) tool_search. -func (tool toolSearchTool) visibleEagerToolNames(enabled []string, disabled []string) map[string]bool { +// list this run: registered, NOT deferred, passing the operator filters and the +// run's permission-mode visibility, and not tool_search itself. These never +// need (and never resolve through) tool_search. +func (tool toolSearchTool) visibleEagerToolNames(enabled []string, disabled []string, permissionMode string) map[string]bool { names := map[string]bool{} if tool.registry == nil { return names @@ -151,6 +164,9 @@ func (tool toolSearchTool) visibleEagerToolNames(enabled []string, disabled []st if !toolAllowedByFilters(candidate.Name(), enabled, disabled) { continue } + if !toolAdvertisedForPermissionMode(candidate, permissionMode) { + continue + } names[candidate.Name()] = true } return names @@ -160,8 +176,8 @@ func (tool toolSearchTool) visibleEagerToolNames(enabled []string, disabled []st // tool_search query refers to: exact names for a "select:" query, or eager tools // whose name matches a keyword otherwise. Used to steer the model back to calling // a tool it already has instead of fruitlessly searching for it. -func (tool toolSearchTool) eagerToolsForQuery(query string, enabled []string, disabled []string) []string { - eager := tool.visibleEagerToolNames(enabled, disabled) +func (tool toolSearchTool) eagerToolsForQuery(query string, enabled []string, disabled []string, permissionMode string) []string { + eager := tool.visibleEagerToolNames(enabled, disabled, permissionMode) if len(eager) == 0 { return nil } @@ -215,6 +231,59 @@ func toolAllowedByFilters(name string, enabled []string, disabled []string) bool return !containsName(disabled, name) } +// PlanMode and SpecDraftMode are the string values of agent.PermissionModePlan +// and agent.PermissionModeSpecDraft. RunOptions.PermissionMode is already a +// plain string (set from agent.PermissionMode via string(permissionMode)), so +// this package owns the literals rather than importing the agent type (which +// would create an import cycle). agent.ToolAdvertised delegates plan/spec-draft +// decisions here so tool_search and the dispatch gate stay identical. +const ( + PlanMode = "plan" + SpecDraftMode = "spec-draft" +) + +// ToolAdvertisedForPermissionMode is the single source of truth for whether a +// tool may be advertised under a given permission mode string. agent.ToolAdvertised +// calls this for plan/spec-draft after its own PermissionDeny short-circuit; +// tool_search applies it to deferred candidates so a mutator cannot leak through +// load_tools. Modes other than plan/spec-draft only apply the PermissionDeny +// gate here (auto/member-auto/ask/unsafe advertisement rules stay in agent). +func ToolAdvertisedForPermissionMode(tool Tool, permissionMode string) bool { + // A denied tool is never advertised in any mode (mirrors agent.ToolAdvertised). + if tool.Safety().Permission == PermissionDeny { + return false + } + switch permissionMode { + case PlanMode: + if tool.Name() == "lsp_navigate" { + return false + } + safety := tool.Safety() + return safety.SideEffect == SideEffectRead && safety.Permission == PermissionAllow + case SpecDraftMode: + // Never trust control-tool names alone (a re-registered spoof can carry + // arbitrary Safety). + safety := tool.Safety() + switch tool.Name() { + case "update_plan": + return false + case "ask_user": + return safety.SideEffect == SideEffectRead && safety.Permission == PermissionAllow + case "submit_spec": + return safety.SideEffect == SideEffectWrite && safety.Permission == PermissionAllow + } + return safety.SideEffect == SideEffectRead && safety.Permission == PermissionAllow + default: + return true + } +} + +// toolAdvertisedForPermissionMode is the unexported name used by tool_search +// internals; keep it as a thin alias so call sites stay short. +func toolAdvertisedForPermissionMode(tool Tool, permissionMode string) bool { + return ToolAdvertisedForPermissionMode(tool, permissionMode) +} + func containsName(names []string, name string) bool { for _, candidate := range names { if candidate == name { diff --git a/internal/tools/tool_search_test.go b/internal/tools/tool_search_test.go index 3d07481c7..c3abae365 100644 --- a/internal/tools/tool_search_test.go +++ b/internal/tools/tool_search_test.go @@ -25,6 +25,27 @@ func (t searchFakeTool) Run(context.Context, map[string]any) Result { } func (t searchFakeTool) Deferred() bool { return true } +// searchFakeMutatorTool is a deferred-eligible tool with mutating Safety +// (SideEffectWrite), standing in for a real write/mutator MCP tool that a +// plan/spec-draft run must never surface through tool_search. +type searchFakeMutatorTool struct { + name string + description string +} + +func (t searchFakeMutatorTool) Name() string { return t.name } +func (t searchFakeMutatorTool) Description() string { return t.description } +func (t searchFakeMutatorTool) Parameters() Schema { + return Schema{Type: "object", AdditionalProperties: false} +} +func (t searchFakeMutatorTool) Safety() Safety { + return Safety{SideEffect: SideEffectWrite, Permission: PermissionPrompt, Reason: "mutates"} +} +func (t searchFakeMutatorTool) Run(context.Context, map[string]any) Result { + return Result{Status: StatusOK} +} +func (t searchFakeMutatorTool) Deferred() bool { return true } + func newDeferredFixtureRegistry() *Registry { reg := NewRegistry() reg.Register(searchFakeTool{ @@ -320,3 +341,185 @@ func TestToolSearchHonorsEnabledAllowlist(t *testing.T) { t.Fatalf("excluded tool leaked into keyword no-match listing: %q", keywordResult.Output) } } + +// TestToolSearchExcludesDeferredMutatorInPlanMode guards PR #642 review +// finding (jatmn, P2): tool_search's deferred-candidate filter previously +// checked only EnabledTools/DisabledTools, never the run's permission-mode +// visibility, so a plan-mode run could resolve `select:` +// and receive that tool's full schema even though a direct call to it is +// denied by the agent's plan-mode dispatch gate. A deferred mutator (write +// SideEffect) must be invisible to tool_search in plan mode: absent from +// select: resolution, absent from keyword ranking, and absent from the +// no-match/listing text — mirroring ToolAdvertisedForPermissionMode applied +// to direct tool calls via agent.ToolAdvertised. +func TestToolSearchExcludesDeferredMutatorInPlanMode(t *testing.T) { + reg := NewRegistry() + reg.Register(searchFakeTool{name: "weather_lookup", description: "Look up weather."}) + reg.Register(searchFakeMutatorTool{name: "file_mutate", description: "Writes files to disk."}) + tool := NewToolSearchTool(reg).(optionsAwareTool) + plan := RunOptions{PermissionMode: "plan"} + + // select: an unrelated miss must NOT list the mutator among available tools. + // Query "select:does_not_exist" avoids echoing the mutator's name verbatim so + // the listing-omission assertion below is not confused by the query echo. + selectResult := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:does_not_exist"}, plan) + if _, present := selectResult.Meta["load_tools"]; present { + t.Fatalf("plan mode must not resolve a deferred mutator via select:, got load_tools=%q", selectResult.Meta["load_tools"]) + } + listing := selectResult.Output + if idx := strings.Index(listing, "Available tools: "); idx >= 0 { + listing = listing[idx:] + } + if strings.Contains(listing, "file_mutate") { + t.Fatalf("deferred mutator leaked into plan-mode no-match listing: %q", selectResult.Output) + } + if !strings.Contains(listing, "weather_lookup") { + t.Fatalf("plan-mode no-match listing must still name the visible read-only tool, got %q", selectResult.Output) + } + + // keyword: the mutator must NOT rank, and its schema/description must not leak. + keywordResult := tool.RunWithOptions(context.Background(), + map[string]any{"query": "mutate write"}, plan) + if got := keywordResult.Meta["load_tools"]; got != "" { + t.Fatalf("deferred mutator must not rank for a keyword query in plan mode, got load_tools=%q", got) + } + if strings.Contains(keywordResult.Output, "file_mutate") || strings.Contains(keywordResult.Output, "Writes files") { + t.Fatalf("deferred mutator leaked into plan-mode keyword output: %q", keywordResult.Output) + } + + // A direct select: of file_mutate must load nothing (exact resolution also + // filters by mode, not just the no-match listing/keyword paths). + exactResult := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:file_mutate"}, plan) + if got := exactResult.Meta["load_tools"]; got != "" { + t.Fatalf("select:file_mutate must not load anything in plan mode, got load_tools=%q", got) + } + if strings.Contains(exactResult.Output, "Writes files") { + t.Fatalf("deferred mutator schema/description leaked via exact select: %q", exactResult.Output) + } + + // A read-only deferred tool must remain visible in plan mode (this filter + // narrows by mode, it does not disable deferral discovery altogether). + readResult := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:weather_lookup"}, plan) + if got := readResult.Meta["load_tools"]; got != "weather_lookup" { + t.Fatalf("plan mode must still resolve a read-only deferred tool, got load_tools=%q output=%q", got, readResult.Output) + } +} + +// TestToolSearchExcludesDeferredMutatorInSpecDraftMode is the spec-draft analog +// of TestToolSearchExcludesDeferredMutatorInPlanMode: spec-draft is the other +// read-only mode whose direct-invocation dispatch gate restricts a tool to +// SideEffectRead+PermissionAllow (ToolAdvertisedForPermissionMode), so +// tool_search must apply the identical narrowing. +func TestToolSearchExcludesDeferredMutatorInSpecDraftMode(t *testing.T) { + reg := NewRegistry() + reg.Register(searchFakeMutatorTool{name: "file_mutate", description: "Writes files to disk."}) + tool := NewToolSearchTool(reg).(optionsAwareTool) + specDraft := RunOptions{PermissionMode: "spec-draft"} + + result := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:file_mutate"}, specDraft) + if got := result.Meta["load_tools"]; got != "" { + t.Fatalf("spec-draft mode must not resolve a deferred mutator via select:, got load_tools=%q", got) + } + if strings.Contains(result.Output, "Writes files") { + t.Fatalf("deferred mutator schema/description leaked in spec-draft mode: %q", result.Output) + } +} + +// TestToolSearchRejectsSpoofedSpecDraftControlToolsBySafety guards the +// name-only allowlist hole CodeRabbit flagged: a tool re-registered as +// ask_user or submit_spec with the wrong Safety shape must not be +// advertised or loadable via tool_search in spec-draft mode. +func TestToolSearchRejectsSpoofedSpecDraftControlToolsBySafety(t *testing.T) { + cases := []struct { + name string + safety Safety + }{ + // ask_user must be SideEffectRead+Allow; a shell spoof fails. + {name: "ask_user", safety: Safety{SideEffect: SideEffectShell, Permission: PermissionAllow, Reason: "spoof"}}, + // submit_spec must be SideEffectWrite+Allow; a shell spoof fails. + {name: "submit_spec", safety: Safety{SideEffect: SideEffectShell, Permission: PermissionAllow, Reason: "spoof"}}, + // Wrong permission also fails even if the side effect matches. + {name: "ask_user", safety: Safety{SideEffect: SideEffectRead, Permission: PermissionDeny, Reason: "spoof"}}, + {name: "submit_spec", safety: Safety{SideEffect: SideEffectWrite, Permission: PermissionDeny, Reason: "spoof"}}, + } + for _, tc := range cases { + t.Run(tc.name+"/"+string(tc.safety.SideEffect)+"/"+string(tc.safety.Permission), func(t *testing.T) { + reg := NewRegistry() + reg.Register(searchSpoofedSafetyTool{name: tc.name, safety: tc.safety, description: "spoofed control tool"}) + tool := NewToolSearchTool(reg).(optionsAwareTool) + result := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:" + tc.name}, RunOptions{PermissionMode: "spec-draft"}) + if got := result.Meta["load_tools"]; got != "" { + t.Fatalf("spec-draft must not load spoofed %s (safety=%+v), got load_tools=%q", tc.name, tc.safety, got) + } + if strings.Contains(result.Output, "spoofed control tool") { + t.Fatalf("spoofed %s description leaked: %q", tc.name, result.Output) + } + // The description check alone would still pass a regression that leaks + // the schema without the description, since Parameters() previously + // exposed no distinctive marker. spoofed_secret is a property unique to + // this schema, so its absence from Output proves the schema itself + // (not just the description string) never serialized into the result. + if strings.Contains(result.Output, "spoofed_secret") { + t.Fatalf("spoofed %s schema leaked: %q", tc.name, result.Output) + } + }) + } +} + +// TestToolSearchExcludesPermissionDenyInDefaultMode locks the PermissionDeny +// short-circuit that agent.ToolAdvertised applies in every mode: a deferred +// tool marked PermissionDeny must stay unresolvable through tool_search even +// under auto/empty permission mode (where plan/spec-draft narrowing does not +// apply). +func TestToolSearchExcludesPermissionDenyInDefaultMode(t *testing.T) { + reg := NewRegistry() + reg.Register(searchSpoofedSafetyTool{ + name: "denied_lookup", + description: "Should never resolve", + safety: Safety{SideEffect: SideEffectRead, Permission: PermissionDeny, Reason: "denied"}, + }) + tool := NewToolSearchTool(reg).(optionsAwareTool) + + for _, mode := range []string{"", "auto", "ask", "unsafe"} { + result := tool.RunWithOptions(context.Background(), + map[string]any{"query": "select:denied_lookup"}, RunOptions{PermissionMode: mode}) + if got := result.Meta["load_tools"]; got != "" { + t.Fatalf("mode %q must not resolve PermissionDeny deferred tool, got load_tools=%q", mode, got) + } + if strings.Contains(result.Output, "Should never resolve") || strings.Contains(result.Output, "spoofed_secret") { + t.Fatalf("PermissionDeny deferred tool leaked under mode %q: %q", mode, result.Output) + } + } +} + +// searchSpoofedSafetyTool is a deferred-eligible tool whose Name and Safety +// are under test control (used to re-register ask_user/submit_spec shapes). +type searchSpoofedSafetyTool struct { + name, description string + safety Safety +} + +func (t searchSpoofedSafetyTool) Name() string { return t.name } +func (t searchSpoofedSafetyTool) Description() string { return t.description } +func (t searchSpoofedSafetyTool) Parameters() Schema { + // spoofed_secret is a distinctive marker property with no other purpose: + // its presence or absence in a serialized result.Output is what proves + // whether the schema itself (not just the description) leaked. + return Schema{ + Type: "object", + Properties: map[string]PropertySchema{ + "spoofed_secret": {Type: "string", Description: "spoofed control tool marker property"}, + }, + AdditionalProperties: false, + } +} +func (t searchSpoofedSafetyTool) Safety() Safety { return t.safety } +func (t searchSpoofedSafetyTool) Deferred() bool { return true } +func (t searchSpoofedSafetyTool) Run(context.Context, map[string]any) Result { + return Result{Status: StatusOK, Output: "should not run"} +} diff --git a/internal/tui/commands.go b/internal/tui/commands.go index 137f7beb4..db1495b93 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -112,9 +112,9 @@ var commandDefinitions = []commandDefinition{ }, { name: "/plan", - usage: "/plan", + usage: "/plan [status|on|off]", group: commandGroupSession, - description: "Show planning mode status.", + description: "Show plan status, or enter/exit read-only planning mode.", kind: commandPlan, }, { diff --git a/internal/tui/commands_test.go b/internal/tui/commands_test.go index ea3a37323..eb2edeca6 100644 --- a/internal/tui/commands_test.go +++ b/internal/tui/commands_test.go @@ -49,7 +49,7 @@ func TestFormatCommandHelpLinesGroupsCommandsByStableOrder(t *testing.T) { " /model [list|id] - Show or switch the active model.", " /effort [list|low|medium|high|auto] - Show or set reasoning effort for supported models.", "session:", - " /plan - Show planning mode status.", + " /plan [status|on|off] - Show plan status, or enter/exit read-only planning mode.", "runtime:", " /permissions - Show the active permission mode and sandbox grants.", " /debug (/debug-mode) - Show debug mode status.", diff --git a/internal/tui/model.go b/internal/tui/model.go index 916b26221..3f019c960 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -126,8 +126,12 @@ type model struct { agentOptions agent.Options notifier *notify.Notifier permissionMode agent.PermissionMode - selfCorrectTests bool - reasoningEffort modelregistry.ReasoningEffort + // permissionModeBeforePlan holds whatever mode was active when /plan on + // entered PermissionModePlan, so /plan off can restore it exactly (mirrors + // the execProfile displaced/applied pattern below). + permissionModeBeforePlan agent.PermissionMode + selfCorrectTests bool + reasoningEffort modelregistry.ReasoningEffort // Active execution profile (set by /profile; applies to the NEXT run). // The displaced/applied pairs let a switch or /profile balanced restore // exactly what the profile replaced while leaving later manual overrides @@ -4372,6 +4376,13 @@ func (m model) dispatchCommand(command parsedCommand) (tea.Model, tea.Cmd) { }) return m, nil } + if m.permissionMode == agent.PermissionModePlan && planModeCommandUnavailable(command) { + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendSystem, + text: command.name + " is unavailable in plan mode — it mutates the workspace or spawns a process outside the read-only gate. Exit with /plan off first.", + }) + return m, nil + } switch command.kind { case commandEmpty: return m, nil @@ -4519,7 +4530,9 @@ func (m model) dispatchCommand(command parsedCommand) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.debugText()}) return m, nil case commandPlan: - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.planText()}) + text := "" + m, text = m.handlePlanCommand(command.text) + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) return m, nil case commandDoctor: return m.startDoctorCommand(command.text) diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index b7b66529b..221dcfced 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -193,6 +193,9 @@ func TestParseCommand(t *testing.T) { {input: "/resume", kind: commandResume}, {input: "/sessions", kind: commandResume}, {input: "/spec add review flow", kind: commandSpec, text: "add review flow"}, + {input: "/plan", kind: commandPlan}, + {input: "/plan on", kind: commandPlan, text: "on"}, + {input: "/plan off", kind: commandPlan, text: "off"}, {input: "/compact", kind: commandCompact}, {input: "/effort high", kind: commandEffort, text: "high"}, {input: "/style concise", kind: commandStyle, text: "concise"}, diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 8baf629da..cbe542c0d 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/Gitlawb/zero/internal/agent" "github.com/Gitlawb/zero/internal/tools" ) @@ -11,6 +12,66 @@ type currentPlanReader interface { CurrentPlan() []tools.PlanItem } +// handlePlanCommand drives /plan: bare or "status" just reports the current +// plan (pre-existing behavior); "on" and "off" are the entry/exit path into +// PermissionModePlan. Unlike /spec (which drafts in a separate, forked +// session), plan mode applies to the CURRENT session, so entering/exiting it +// is a direct m.permissionMode flip rather than a run-option override. +func (m model) handlePlanCommand(args string) (model, string) { + switch strings.ToLower(strings.TrimSpace(args)) { + case "", "status": + return m, m.planText() + case "on": + if m.pending { + return m, "Cannot change plan mode while a turn is active." + } + if m.permissionMode == agent.PermissionModePlan { + return m, "Plan mode\nAlready active. Write and shell tools stay hidden until /plan off." + } + m.permissionModeBeforePlan = m.permissionMode + m.permissionMode = agent.PermissionModePlan + return m, "Plan mode\nActive: read-only planning. Write and shell tools are hidden until /plan off." + case "off": + if m.pending { + return m, "Cannot change plan mode while a turn is active." + } + if m.permissionMode != agent.PermissionModePlan { + return m, "Plan mode\nNot currently active." + } + restored := m.permissionModeBeforePlan + if restored == "" { + restored = agent.PermissionModeAuto + } + m.permissionMode = restored + m.permissionModeBeforePlan = "" + return m, "Plan mode\nExited. Permission mode restored to " + string(restored) + "." + default: + return m, "Plan mode\nUsage: /plan [status|on|off]" + } +} + +// planModeCommandUnavailable reports whether a local (non-tool) TUI command +// must be blocked while plan mode is active. Plan mode's tool-advertisement +// gate only covers agent tool calls; these commands run entirely inside the +// TUI process and would mutate the workspace or spawn a host process outside +// that gate: /rewind restores files from a checkpoint, /export writes a +// transcript file to disk, /sandbox-setup runs native platform setup, +// /spec forks a drafting session, /mcp mutates server configuration, and +// /init's whole job is writing AGENTS.md (which plan mode then denies). +// Bare /mcp (empty text) only opens the read-only manager view, so it stays +// available. Modeled on btwCommandUnavailable's shape for the analogous BTW guard. +func planModeCommandUnavailable(command parsedCommand) bool { + switch command.kind { + case commandRewind, commandExport, commandSandboxSetup, commandSpec, commandInit: + return true + case commandMCP: + // Bare /mcp only opens the read-only manager view; subcommands mutate config. + return strings.TrimSpace(command.text) != "" + default: + return false + } +} + func (m model) planText() string { tool, ok := m.registry.Get("update_plan") if !ok { diff --git a/internal/tui/plan_mode_test.go b/internal/tui/plan_mode_test.go new file mode 100644 index 000000000..98ee8f1b8 --- /dev/null +++ b/internal/tui/plan_mode_test.go @@ -0,0 +1,391 @@ +package tui + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// TestPlanCommandEntersAndExitsPlanMode drives /plan on then /plan off and +// confirms m.permissionMode actually flips to PermissionModePlan and back — +// the entry/exit path the previous /plan (display-only) command was missing. +func TestPlanCommandEntersAndExitsPlanMode(t *testing.T) { + m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAuto}) + + updated, cmd := m.dispatchCommand(parseCommand("/plan on")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /plan on to be synchronous") + } + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("permissionMode after /plan on = %s, want plan", next.permissionMode) + } + if !transcriptContains(next.transcript, "read-only planning") { + t.Fatalf("expected activation notice in transcript, got %#v", next.transcript) + } + + updated, cmd = next.dispatchCommand(parseCommand("/plan off")) + next = updated.(model) + if cmd != nil { + t.Fatal("expected /plan off to be synchronous") + } + if next.permissionMode != agent.PermissionModeAuto { + t.Fatalf("permissionMode after /plan off = %s, want auto (restored)", next.permissionMode) + } + if !transcriptContains(next.transcript, "restored to auto") { + t.Fatalf("expected restore notice in transcript, got %#v", next.transcript) + } +} + +// TestPlanCommandRestoresPriorModeOnExit confirms /plan off restores whatever +// mode was active before /plan on, not a hardcoded default. +func TestPlanCommandRestoresPriorModeOnExit(t *testing.T) { + m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAsk}) + + updated, _ := m.dispatchCommand(parseCommand("/plan on")) + next := updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("permissionMode after /plan on = %s, want plan", next.permissionMode) + } + + updated, _ = next.dispatchCommand(parseCommand("/plan off")) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("permissionMode after /plan off = %s, want ask (restored)", next.permissionMode) + } +} + +// TestPlanCommandStatusDoesNotChangeMode is a regression guard for the +// pre-existing display-only behavior: bare /plan and /plan status must keep +// reporting the plan without touching the active permission mode. +func TestPlanCommandStatusDoesNotChangeMode(t *testing.T) { + m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAuto, Registry: tools.NewRegistry()}) + + updated, _ := m.dispatchCommand(parseCommand("/plan")) + next := updated.(model) + if next.permissionMode != agent.PermissionModeAuto { + t.Fatalf("bare /plan changed permissionMode to %s", next.permissionMode) + } + + updated, _ = next.dispatchCommand(parseCommand("/plan status")) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAuto { + t.Fatalf("/plan status changed permissionMode to %s", next.permissionMode) + } +} + +// TestPlanCommandOffWithoutActivePlanIsNoop confirms /plan off is a harmless +// no-op (not an error, not a mode change) when plan mode was never entered. +func TestPlanCommandOffWithoutActivePlanIsNoop(t *testing.T) { + m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAuto}) + + updated, _ := m.dispatchCommand(parseCommand("/plan off")) + next := updated.(model) + if next.permissionMode != agent.PermissionModeAuto { + t.Fatalf("permissionMode = %s, want unchanged auto", next.permissionMode) + } + if !transcriptContains(next.transcript, "Not currently active") { + t.Fatalf("expected not-active notice in transcript, got %#v", next.transcript) + } +} + +// TestPlanCommandOnTwiceDoesNotClobberSavedMode confirms a redundant /plan on +// doesn't overwrite the saved prior mode with Plan itself, which would strand +// /plan off unable to restore the real original mode. +func TestPlanCommandOnTwiceDoesNotClobberSavedMode(t *testing.T) { + m := newModel(context.Background(), Options{PermissionMode: agent.PermissionModeAsk}) + + updated, _ := m.dispatchCommand(parseCommand("/plan on")) + next := updated.(model) + updated, _ = next.dispatchCommand(parseCommand("/plan on")) + next = updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("permissionMode = %s, want plan", next.permissionMode) + } + + updated, _ = next.dispatchCommand(parseCommand("/plan off")) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("permissionMode after off = %s, want ask restored (double /plan on must not clobber it)", next.permissionMode) + } +} + +// TestNextPermissionModeLeavesPlanUntouched confirms the shift+tab Auto<->Ask +// toggle cannot silently exit plan mode: folding Plan to Ask would be a LESS +// strict landing (Ask still permits write/shell tools with a prompt), so the +// read-only guarantee must only be given up via the explicit /plan off exit. +func TestNextPermissionModeLeavesPlanUntouched(t *testing.T) { + if got := nextPermissionMode(agent.PermissionModePlan); got != agent.PermissionModePlan { + t.Fatalf("nextPermissionMode(Plan) = %s, want Plan unchanged", got) + } +} + +// TestPlanModeBlocksRewindMutation guards the coderabbitai finding that /plan +// on only flips the agent permission mode: local TUI commands bypass tool +// filtering entirely, so /rewind — which restores workspace files straight +// from a checkpoint — needed its own gate. This drives a real checkpoint and +// confirms the file on disk is untouched (and no rewind summary appears) +// while plan mode is active. +func TestPlanModeBlocksRewindMutation(t *testing.T) { + store := testSessionStore(t) + ws := t.TempDir() + session, err := store.Create(sessions.CreateInput{Title: "plan rewind", Cwd: ws, ModelID: "gpt-4.1", Provider: "openai"}) + if err != nil { + t.Fatalf("Create returned error: %v", err) + } + path := filepath.Join(ws, "a.txt") + writeTestFile(t, ws, "a.txt", "original") + if _, err := store.CaptureToolCheckpoint(session.SessionID, ws, "write_file", []string{"a.txt"}); err != nil { + t.Fatalf("CaptureToolCheckpoint: %v", err) + } + writeTestFile(t, ws, "a.txt", "changed while planning") + + m := newModel(context.Background(), Options{SessionStore: store, Cwd: ws, PermissionMode: agent.PermissionModePlan}) + m.activeSession = session + + updated, cmd := m.dispatchCommand(parseCommand("/rewind")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /rewind to be blocked synchronously in plan mode") + } + if !transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("expected plan-mode denial, got %#v", next.transcript) + } + if transcriptContains(next.transcript, "Rewound") { + t.Fatalf("rewind should not have run in plan mode, got %#v", next.transcript) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read file: %v", err) + } + if string(got) != "changed while planning" { + t.Fatalf("rewind mutated the workspace despite plan mode gate: %q", got) + } +} + +// TestPlanModeBlocksExportMutation guards the same finding for /export, which +// writes a transcript file to disk outside the agent tool gate. +func TestPlanModeBlocksExportMutation(t *testing.T) { + dir := t.TempDir() + m := newModel(context.Background(), Options{Cwd: dir, PermissionMode: agent.PermissionModePlan}) + + updated, cmd := m.dispatchCommand(parseCommand("/export out.txt")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /export to be blocked synchronously in plan mode") + } + if !transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("expected plan-mode denial, got %#v", next.transcript) + } + if _, err := os.Stat(filepath.Join(dir, "out.txt")); !os.IsNotExist(err) { + t.Fatalf("export should not have written a file in plan mode, stat err=%v", err) + } +} + +// TestPlanModeBlocksSandboxSetupProcess guards the same finding for +// /sandbox-setup, which spawns a native host process outside the agent tool +// gate: the injected SandboxSetupCommand must never run while plan mode is +// active, and the command must be handled synchronously (no async tea.Cmd). +func TestPlanModeBlocksSandboxSetupProcess(t *testing.T) { + called := false + m := newModel(context.Background(), Options{ + PermissionMode: agent.PermissionModePlan, + SandboxSetupCommand: func(context.Context) SandboxSetupCommandResult { + called = true + return SandboxSetupCommandResult{ExitCode: 0} + }, + }) + + updated, cmd := m.dispatchCommand(parseCommand("/sandbox-setup")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /sandbox-setup to be blocked synchronously in plan mode") + } + if called { + t.Fatal("sandbox setup process must not run in plan mode") + } + if !transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("expected plan-mode denial, got %#v", next.transcript) + } +} + +// TestPlanModeBlocksInitCommand: /init's sole job is writing AGENTS.md, which +// plan mode then denies at the tool gate. Block the command up front so the +// operator gets a clear "exit plan mode first" instead of a failed turn. +func TestPlanModeBlocksInitCommand(t *testing.T) { + m := newModel(context.Background(), Options{Cwd: t.TempDir(), PermissionMode: agent.PermissionModePlan}) + + updated, cmd := m.dispatchCommand(parseCommand("/init")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /init to be blocked synchronously in plan mode") + } + if !transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("expected plan-mode denial, got %#v", next.transcript) + } + if transcriptContains(next.transcript, "Generate an AGENTS.md") { + t.Fatalf("/init must not launch the bootstrap turn in plan mode, got %#v", next.transcript) + } +} + +// TestPlanModeAllowsBareMCPManagerView: bare /mcp only opens the read-only +// manager overlay and must stay available while planning. +func TestPlanModeAllowsBareMCPManagerView(t *testing.T) { + m := newModel(context.Background(), Options{Cwd: t.TempDir(), PermissionMode: agent.PermissionModePlan}) + + updated, cmd := m.dispatchCommand(parseCommand("/mcp")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected bare /mcp to be synchronous") + } + if transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("bare /mcp must not be blocked in plan mode, got %#v", next.transcript) + } + if next.mcpManager == nil { + t.Fatal("expected bare /mcp to open the MCP manager in plan mode") + } +} + +// TestPlanModeBlocksMCPSubcommands: /mcp mutates server configuration +// and must stay blocked while plan mode is active. +func TestPlanModeBlocksMCPSubcommands(t *testing.T) { + m := newModel(context.Background(), Options{Cwd: t.TempDir(), PermissionMode: agent.PermissionModePlan}) + + updated, cmd := m.dispatchCommand(parseCommand("/mcp disable docs")) + next := updated.(model) + if cmd != nil { + t.Fatal("expected /mcp subcommand to be blocked synchronously in plan mode") + } + if !transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("expected plan-mode denial for /mcp subcommand, got %#v", next.transcript) + } + if next.mcpManager != nil { + t.Fatal("/mcp subcommand must not open the manager when blocked") + } +} + +// TestPlanModeCommandGuardDoesNotBlockOutsideMode confirms the guard is +// scoped to plan mode: the same commands must behave normally (not be +// swallowed by the new check) once plan mode is off. +func TestPlanModeCommandGuardDoesNotBlockOutsideMode(t *testing.T) { + dir := t.TempDir() + m := newModel(context.Background(), Options{Cwd: dir, PermissionMode: agent.PermissionModeAuto}) + m.transcript = append(m.transcript, transcriptRow{kind: rowUser, text: "hello"}) + + updated, _ := m.dispatchCommand(parseCommand("/export out.txt")) + next := updated.(model) + if transcriptContains(next.transcript, "unavailable in plan mode") { + t.Fatalf("export should not be gated outside plan mode, got %#v", next.transcript) + } + if _, err := os.Stat(filepath.Join(dir, "out.txt")); err != nil { + t.Fatalf("expected export to write the file outside plan mode: %v", err) + } +} + +func newPlanModeTestModel(root string, provider zeroruntime.Provider) model { + registry := tools.NewRegistry() + for _, tool := range tools.CoreToolsScoped(root, nil) { + registry.Register(tool) + } + return newModel(context.Background(), Options{ + Cwd: root, + ProviderName: "openai", + ModelName: "gpt-4.1", + Provider: provider, + Registry: registry, + // Ask (not Auto) is the base mode here so the "write_file is advertised + // again after /plan off" check is unambiguous: ToolAdvertised only + // exposes prompt-permission tools like write_file/bash unconditionally + // under Ask (Auto hides them from advertisement entirely unless a tool + // opts into AdvertiseInAuto, which write_file does not). + PermissionMode: agent.PermissionModeAsk, + }) +} + +// TestPlanModeGatesWriteToolAndRestoresOnExit is the end-to-end integration +// test: it drives /plan on, submits a prompt whose (adversarial, since the +// tool isn't even advertised) provider response tries to call write_file +// directly, and confirms the call is denied and nothing is written to disk. +// Then it drives /plan off and confirms write_file is advertised again, +// proving the mode genuinely reverted rather than merely relabeling. +func TestPlanModeGatesWriteToolAndRestoresOnExit(t *testing.T) { + root := t.TempDir() + targetPath := filepath.Join(root, "notes.txt") + provider := &scriptedProvider{scripts: [][]zeroruntime.StreamEvent{ + // The write attempt is denied before it reaches the sandbox, so the loop + // makes a second request for the model's next move within THIS run — + // hence two scripts for run 1, matching the write/deny/react shape used + // elsewhere (see TestPromptSubmitPersistsPermissionSessionEvents). + writeFileToolScript("call_write", "notes.txt", "hello from plan mode"), + textScript("understood, staying read-only"), + textScript("nothing to report"), + }} + m := newPlanModeTestModel(root, provider) + + updated, _ := m.dispatchCommand(parseCommand("/plan on")) + m = updated.(model) + if m.permissionMode != agent.PermissionModePlan { + t.Fatalf("permissionMode after /plan on = %s, want plan", m.permissionMode) + } + + m.input.SetValue("please write the notes file") + updatedModel, cmd := m.Update(testKey(tea.KeyEnter)) + next := updatedModel.(model) + if cmd == nil { + t.Fatal("expected prompt submit to start an agent run") + } + updatedModel, _ = next.Update(execCmd(cmd)) + next = updatedModel.(model) + + if len(provider.requests) == 0 { + t.Fatal("expected at least one provider request") + } + if providerRequestIncludesTool(provider.requests[0], "write_file") { + t.Fatalf("write_file must not be advertised in plan mode: %#v", provider.requests[0].Tools) + } + if providerRequestIncludesTool(provider.requests[0], "bash") { + t.Fatalf("bash must not be advertised in plan mode: %#v", provider.requests[0].Tools) + } + + result, ok := findTranscriptRow(next.transcript, rowToolResult) + if !ok || result.tool != "write_file" || result.status != tools.StatusError { + t.Fatalf("expected a denied write_file tool result, got ok=%v row=%#v", ok, result) + } + if !strings.Contains(result.detail, "not available in plan mode") { + t.Fatalf("expected plan-mode denial message, got %q", result.detail) + } + if _, err := os.Stat(targetPath); !os.IsNotExist(err) { + t.Fatalf("write_file executed despite plan mode gating: stat err=%v", err) + } + + updated, _ = next.dispatchCommand(parseCommand("/plan off")) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("permissionMode after /plan off = %s, want ask (restored)", next.permissionMode) + } + + next.input.SetValue("anything else to check?") + updatedModel, cmd = next.Update(testKey(tea.KeyEnter)) + next = updatedModel.(model) + if cmd == nil { + t.Fatal("expected second prompt submit to start an agent run") + } + updatedModel, _ = next.Update(execCmd(cmd)) + next = updatedModel.(model) + + if len(provider.requests) != 3 { + t.Fatalf("expected three provider requests (2 in the plan-mode run, 1 after /plan off), got %d", len(provider.requests)) + } + if !providerRequestIncludesTool(provider.requests[2], "write_file") { + t.Fatalf("write_file must be advertised again after /plan off: %#v", provider.requests[2].Tools) + } +} diff --git a/internal/tui/theme.go b/internal/tui/theme.go index 9e708c3b3..f48649cfb 100644 --- a/internal/tui/theme.go +++ b/internal/tui/theme.go @@ -79,6 +79,7 @@ type tuiTheme struct { modeAuto lipgloss.Style modeAsk lipgloss.Style modeUnsafe lipgloss.Style + modePlan lipgloss.Style // Raw colors a few renderers paint/interpolate with directly (the streaming // fade interpolates accent→ink; panel-backed prompts paint on bgPanel), kept @@ -192,6 +193,7 @@ func buildTheme(p palette) tuiTheme { // tool is actually asking right now — a glance separates state from event. modeAsk: fg(p.amber), modeUnsafe: fg(p.red).Bold(true), + modePlan: fg(p.blue).Bold(true), accentColor: col(p.accent), inkColor: col(p.ink), diff --git a/internal/tui/view.go b/internal/tui/view.go index 44c9e3df1..cc9e2c242 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -312,12 +312,18 @@ func providerDisplayNameIsGenericCustom(name string) bool { // on it would let prompt-required tools run with no decision. Unsafe stays an // explicit opt-in (the launch/--skip-permissions-unsafe path), not a UI toggle. // Unsafe is folded back to Ask so the toggle always lands somewhere safe. +// Plan is left untouched: folding it to Ask would be a LESS strict landing +// (Ask allows write/shell tools with a prompt; Plan hides them entirely), so +// the read-only guarantee must only be given up through the explicit /plan +// off exit, never a stray shift+tab. func nextPermissionMode(mode agent.PermissionMode) agent.PermissionMode { switch mode { case agent.PermissionModeAuto: return agent.PermissionModeAsk case agent.PermissionModeAsk: return agent.PermissionModeAuto + case agent.PermissionModePlan: + return agent.PermissionModePlan default: // Anything else (incl. an externally-set Unsafe) folds to Ask — the stricter // landing, so toggling never makes an Unsafe session less strict. @@ -333,6 +339,8 @@ func (m model) modeLabel() (string, lipgloss.Style) { return "ask", zeroTheme.modeAsk case agent.PermissionModeUnsafe: return "unsafe", zeroTheme.modeUnsafe + case agent.PermissionModePlan: + return "plan", zeroTheme.modePlan default: mode := strings.TrimSpace(string(m.permissionMode)) if mode == "" {