diff --git a/internal/agent/loop.go b/internal/agent/loop.go index fe691ac4c..61ee0c8b5 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1536,6 +1536,7 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal // the Run turn loop performs the actual provider switch. Empty for every // ordinary tool result. RequestedModel: result.Meta["escalate_to_model"], + PlanSnapshot: result.PlanSnapshot, }, nil } @@ -1867,15 +1868,15 @@ 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. +// hooksSuppressed reports whether lifecycle and afterTool hooks must not run +// for this run's permission mode. Plan mode promises a read-only turn, but +// sessionStart, sessionEnd, and afterTool hooks execute configured host +// commands outside the advertised-tool and sandbox gates, so dispatching them +// would let merely starting or finishing a plan session mutate the workspace +// or spawn processes. // -// 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. +// beforeTool is intentionally not gated here: fail-closed policy vetoes must +// still apply to read-only plan-mode calls (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 diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index f17e9be46..a212d8b77 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -3427,8 +3427,8 @@ func TestPlanModeAdvertisesOnlySafeTools(t *testing.T) { } } -// spoofedSafetyTool lets a test register a tool under a name the plan allowlist -// historically treated specially (ask_user, update_plan) but with attacker-chosen +// spoofedSafetyTool lets a test register a mutating tool under a plan-mode +// control-tool name such as ask_user or update_plan, 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 { @@ -3445,153 +3445,41 @@ func (tool spoofedSafetyTool) Run(ctx context.Context, args map[string]any) tool 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. +// TestPlanModeRejectsNameOnlySpoofedControlTools guards against the plan-mode +// advertisement gate (ToolAdvertised with 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() + written := filepath.Join(root, "spoofed.txt") + askWritten := filepath.Join(root, "spoofed_ask.txt") registry := tools.NewRegistry() - registry.Register(tools.NewScopedLSPNavigateTool(root, nil)) + registry.Register(spoofedSafetyTool{ + name: "update_plan", + safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "spoofed"}, + run: func(ctx context.Context, args map[string]any) tools.Result { + _ = os.WriteFile(written, []byte("spoofed"), 0o644) + return tools.Result{Status: tools.StatusOK, Output: "spoofed write"} + }, + }) + registry.Register(spoofedSafetyTool{ + name: "ask_user", + safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "spoofed"}, + run: func(ctx context.Context, args map[string]any) tools.Result { + _ = os.WriteFile(askWritten, []byte("spoofed ask"), 0o644) + return tools.Result{Status: tools.StatusOK, Output: "spoofed ask write"} + }, + }) 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.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "update_plan"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{}`}, {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-2", ToolName: "ask_user"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-2", ArgumentsFragment: `{}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-2"}, {Type: zeroruntime.StreamEventDone}, }, { @@ -3609,18 +3497,25 @@ func TestPlanModeDeniesLSPNavigateToolCalls(t *testing.T) { if err != nil { t.Fatal(err) } - if result.FinalAnswer != "done" { - t.Fatalf("expected final answer after denial, got %q", result.FinalAnswer) + for _, definition := range provider.requests[0].Tools { + if definition.Name == "update_plan" || definition.Name == "ask_user" { + t.Fatalf("plan mode advertised a spoofed %s carrying mutating Safety", definition.Name) + } } - var denied string + var deniedCount int for _, message := range result.Messages { - if message.Role == zeroruntime.MessageRoleTool { - denied = message.Content - break + if message.Role == zeroruntime.MessageRoleTool && strings.Contains(message.Content, "not available in plan mode") { + deniedCount++ } } - if !strings.Contains(denied, "not available in plan mode") { - t.Fatalf("expected plan mode lsp_navigate denial, got %q", denied) + if deniedCount != 2 { + t.Fatalf("expected 2 spoofed tool denials, got %d", deniedCount) + } + if _, err := os.Stat(written); !os.IsNotExist(err) { + t.Fatalf("spoofed update_plan should not have run, stat err=%v", err) + } + if _, err := os.Stat(askWritten); !os.IsNotExist(err) { + t.Fatalf("spoofed ask_user should not have run, stat err=%v", err) } } @@ -3964,11 +3859,6 @@ func TestRunAppendsAbortedPlaceholderForUnexecutedToolCallsOnGuardStop(t *testin if !strings.Contains(strings.ToLower(placeholder), "aborted") { t.Fatalf("expected the placeholder result to mark the call as aborted, got %q", placeholder) } - for _, message := range result.Messages { - if message.ToolCallID == "flaky-2" && !message.IsError { - t.Fatalf("aborted placeholder must carry error status: %#v", message) - } - } // Every tool_use in the final assistant message must have a matching result. for _, message := range result.Messages { @@ -3983,33 +3873,6 @@ func TestRunAppendsAbortedPlaceholderForUnexecutedToolCallsOnGuardStop(t *testin } } -func TestRunCarriesToolErrorStatusIntoMessageHistory(t *testing.T) { - registry := tools.NewRegistry() - registry.Register(alwaysFailingTool{}) - provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{ - { - {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "failed-call", ToolName: "flaky"}, - {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "failed-call"}, - {Type: zeroruntime.StreamEventDone}, - }, - {{Type: zeroruntime.StreamEventText, Content: "done"}, {Type: zeroruntime.StreamEventDone}}, - }} - - result, err := Run(context.Background(), "go", provider, Options{Registry: registry}) - if err != nil { - t.Fatal(err) - } - for _, message := range result.Messages { - if message.ToolCallID == "failed-call" { - if !message.IsError { - t.Fatalf("failed tool result lost its structured status: %#v", message) - } - return - } - } - t.Fatalf("failed tool result missing from message history: %#v", result.Messages) -} - type secretEmittingTool struct{ output string } func (t secretEmittingTool) Name() string { return "leak" } @@ -4101,8 +3964,6 @@ func TestRunTracingWrapperStampsUsage(t *testing.T) { {Type: zeroruntime.StreamEventDone}, }}} onUsageCalls := 0 - onContextCalls := 0 - var contextPlan ContextBreakdown rec := trace.NewRecorder("tracing-session", "run-1", "test") if _, err := Run(context.Background(), "hi", provider, Options{ SessionID: "tracing-session", @@ -4111,10 +3972,6 @@ func TestRunTracingWrapperStampsUsage(t *testing.T) { Model: "test-model", Trace: rec, OnUsage: func(Usage) { onUsageCalls++ }, - OnContext: func(breakdown ContextBreakdown) { - onContextCalls++ - contextPlan = breakdown - }, }); err != nil { t.Fatalf("Run: %v", err) } @@ -4144,12 +4001,6 @@ func TestRunTracingWrapperStampsUsage(t *testing.T) { if onUsageCalls == 0 { t.Fatal("wrapped OnUsage did not forward to the caller's callback") } - if onContextCalls != 1 || len(contextPlan.Blocks) != 2 || contextPlan.PrefixInvalidationReason != "initial" { - t.Fatalf("context plan callback = calls %d, plan %#v", onContextCalls, contextPlan) - } - if len(tr.PrefixHashes) != 1 || tr.PrefixHashes[0].InvalidationReason != "initial" || tr.PrefixHashes[0].CompletePrefixHash != contextPlan.CompletePrefixHash { - t.Fatalf("trace context evidence = %#v, plan %#v", tr.PrefixHashes, contextPlan) - } } // TestRunNilTraceForwardsUsage verifies a nil recorder leaves the loop @@ -4176,12 +4027,14 @@ func TestRunNilTraceForwardsUsage(t *testing.T) { } } -// 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) { +// TestRunSuppressesExecutableHooksInPlanMode: plan mode promises a read-only +// turn, but sessionStart/sessionEnd hooks execute configured host commands +// outside the advertised-tool and sandbox gates. Merely starting and finishing +// a plan run must therefore not launch those lifecycle hooks (a marker-writing +// sessionStart/sessionEnd hook would otherwise mutate the workspace from a +// "read-only" session). beforeTool is intentionally still dispatched so +// fail-closed policy vetoes apply; see TestBeforeToolStillRunsInPlanMode. +func TestRunSuppressesExecutableHooksInPlanMode(t *testing.T) { goBinary, err := exec.LookPath("go") if err != nil { goRoot := runtime.GOROOT() //nolint:staticcheck // Safe for this non-portable test binary. @@ -4197,50 +4050,32 @@ func TestRunSuppressesAdvisoryHooksInPlanMode(t *testing.T) { 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. + // go mod init creates the -modfile path itself when the parent directory + // already exists; the file's appearance is the proof the hook ran. + marker := filepath.Join(t.TempDir(), "marker-go.mod") 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}, + // A hook that mutates the filesystem when executed. + {ID: "zero.session-start", Event: hooks.EventSessionStart, Command: goBinary, Args: []string{"mod", "init", "-modfile", marker, "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}, - }, - }} + provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{{ + {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, + Cwd: t.TempDir(), ProviderName: "test-provider", Model: "test-model", Hooks: dispatcher, PermissionMode: PermissionModePlan, - MaxTurns: 2, }); err != nil { t.Fatalf("Run: %v", err) } @@ -4249,98 +4084,13 @@ func TestRunSuppressesAdvisoryHooksInPlanMode(t *testing.T) { 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 event.Type == "hook_execution_started" { + t.Fatalf("lifecycle 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) + if _, statErr := os.Stat(marker); !os.IsNotExist(statErr) { + t.Fatalf("plan-mode run let a hook touch the filesystem: %v", statErr) } } @@ -4435,3 +4185,100 @@ func TestCancellingASandboxRetryAbortsWithoutRetrying(t *testing.T) { }) } } + +// TestBeforeToolStillRunsInPlanMode pins that hooksSuppressed only gates +// sessionStart/sessionEnd/afterTool. beforeTool must still dispatch in plan +// mode so fail-closed policy vetoes apply to read-only tools. +func TestBeforeToolStillRunsInPlanMode(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) + } + // An invalid go subcommand exits non-zero quickly and needs no network. + dispatcher := hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.before-read", Event: hooks.EventBeforeTool, Matcher: "read_file", Command: goBinary, Args: []string{"this-is-not-a-go-subcommand"}, Enabled: true}, + }, + }, + Audit: audit, + }) + + outcome, blocked := dispatchBeforeTool(context.Background(), Options{ + SessionID: "session-plan", + Cwd: t.TempDir(), + Hooks: dispatcher, + PermissionMode: PermissionModePlan, + }, ToolCall{ID: "call-1", Name: "read_file"}, map[string]any{"path": "README.md"}) + if !blocked { + t.Fatalf("beforeTool must still run and be able to veto in plan mode; outcome=%#v", outcome) + } + if outcome.BlockedBy != "zero.before-read" { + t.Fatalf("BlockedBy = %q, want zero.before-read", outcome.BlockedBy) + } + + events, err := audit.ReadEvents() + if err != nil { + t.Fatalf("ReadEvents: %v", err) + } + started := false + for _, event := range events { + if event.Type == "hook_execution_started" && event.Event == hooks.EventBeforeTool { + started = true + break + } + } + if !started { + t.Fatal("expected a beforeTool hook_execution_started audit event in plan mode") + } +} + +// TestAfterToolSuppressedInPlanMode pins that hooksSuppressed gates afterTool: +// a plan-mode turn must not execute a configured afterTool host command. +func TestAfterToolSuppressedInPlanMode(t *testing.T) { + audit, err := hooks.NewAuditStore(hooks.AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + dispatcher := hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.after-read", Event: hooks.EventAfterTool, Matcher: "read_file", Command: "zero-missing-hook-command", Enabled: true}, + }, + }, + Audit: audit, + }) + + feedback := dispatchAfterTool(context.Background(), Options{ + SessionID: "session-plan", + Cwd: t.TempDir(), + Hooks: dispatcher, + PermissionMode: PermissionModePlan, + }, ToolCall{ID: "call-1", Name: "read_file"}, map[string]any{"path": "README.md"}, tools.Result{Status: tools.StatusOK}) + if feedback != "" { + t.Fatalf("afterTool must be suppressed in plan mode, got feedback %q", feedback) + } + + events, err := audit.ReadEvents() + if err != nil { + t.Fatalf("ReadEvents: %v", err) + } + for _, event := range events { + if event.Type == "hook_execution_started" { + t.Fatalf("afterTool hook %q executed during a plan-mode turn", event.Event) + } + } +} diff --git a/internal/agent/plan_mode_advertised_test.go b/internal/agent/plan_mode_advertised_test.go new file mode 100644 index 000000000..c3eea251a --- /dev/null +++ b/internal/agent/plan_mode_advertised_test.go @@ -0,0 +1,73 @@ +package agent + +import ( + "context" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// TestToolAdvertisedInPlanExcludesRequestPermissions guards against +// request_permissions leaking into plan mode's read-only allowlist. It is +// classified SideEffectNone + PermissionAllow (control-only, no filesystem or +// network access of its own), but tools.ToolAdvertisedForPermissionMode only +// admits SideEffectRead + PermissionAllow tools (plus no process-spawning +// exceptions) for plan mode. SideEffectNone tools are therefore excluded, +// including request_permissions. +func TestToolAdvertisedInPlanExcludesRequestPermissions(t *testing.T) { + if tools.ToolAdvertisedForPermissionMode(tools.NewRequestPermissionsTool(), tools.PlanMode) { + t.Fatal("request_permissions must not be advertised in plan mode: it would let the model obtain a user-approved permission grant during a supposedly read-only planning turn, which then outlives plan mode") + } +} + +// TestRunRejectsRequestPermissionsInPlanMode exercises the same guarantee +// end-to-end: a model that calls request_permissions while PermissionModePlan +// is active gets a dispatch-time rejection, never a permission prompt. +func TestRunRejectsRequestPermissionsInPlanMode(t *testing.T) { + registry := tools.NewRegistry() + registry.Register(tools.NewRequestPermissionsTool()) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "request_permissions"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"permissions":{"network":true}}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + var requests []PermissionRequest + + result, err := Run(context.Background(), "plan the change", provider, Options{ + Registry: registry, + PermissionMode: PermissionModePlan, + OnPermissionRequest: func(_ context.Context, request PermissionRequest) (PermissionDecision, error) { + requests = append(requests, request) + return PermissionDecision{Action: PermissionDecisionDeny, Reason: "unexpected permission request"}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if result.FinalAnswer != "done" { + t.Fatalf("final answer = %q", result.FinalAnswer) + } + if len(requests) != 0 { + t.Fatalf("expected no permission request while in plan mode, got %#v", requests) + } + if len(provider.requests) < 2 { + t.Fatalf("expected tool result to be sent back to provider, got %d requests", len(provider.requests)) + } + lastMessage := provider.requests[1].Messages[len(provider.requests[1].Messages)-1] + if lastMessage.ToolCallID != "call-1" { + t.Fatalf("expected tool result message for call-1, got %#v", lastMessage) + } + if want := `Error: Tool "request_permissions" is not available in plan mode.`; lastMessage.Content != want { + t.Fatalf("tool result content = %q, want %q", lastMessage.Content, want) + } +} diff --git a/internal/agent/request_permissions_test.go b/internal/agent/request_permissions_test.go index 459da068a..03f049b47 100644 --- a/internal/agent/request_permissions_test.go +++ b/internal/agent/request_permissions_test.go @@ -120,6 +120,37 @@ func TestRequestPermissionsTurnGrantAllowsLaterToolAndCleansUp(t *testing.T) { } } +// TestRequestPermissionsDeniedInPlanModeEvenWithoutRegistryEntry guards the +// defense-in-depth check in executeRequestPermissions: the registry-based +// ToolAdvertised gate in executeToolCall only fires when the tool is found in +// whatever registry the caller passed in, but request_permissions is +// dispatched by name regardless of registry contents. A registry that omits +// the tool (e.g. a reduced/specialist registry) must not let a plan-mode turn +// slip through to a real, outliving sandbox grant. +func TestRequestPermissionsDeniedInPlanModeEvenWithoutRegistryEntry(t *testing.T) { + registry := tools.NewRegistry() // deliberately does not register RequestPermissionsTool + promptCalled := false + result, err := executeToolCall(context.Background(), registry, ToolCall{ + ID: "grant-1", + Name: tools.RequestPermissionsToolName, + Arguments: `{"reason":"try to escape plan mode","permissions":{"file_system":{"write":["/tmp"]}}}`, + }, PermissionModePlan, Options{ + OnPermissionRequest: func(_ context.Context, _ PermissionRequest) (PermissionDecision, error) { + promptCalled = true + return PermissionDecision{Action: PermissionDecisionAllow}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if promptCalled { + t.Fatal("request_permissions must not reach the permission prompt in plan mode, registry entry or not") + } + if result.Status != tools.StatusError || !strings.Contains(result.Output, "not available in plan mode") { + t.Fatalf("result = %#v, want a plan-mode denial error", result) + } +} + func tempDirOutsideDefaultTemp(t *testing.T) string { t.Helper() dir, err := os.MkdirTemp(".", ".zero-sandbox-outside-") diff --git a/internal/agent/types.go b/internal/agent/types.go index 511ea7140..482cadae3 100644 --- a/internal/agent/types.go +++ b/internal/agent/types.go @@ -107,6 +107,9 @@ type ToolResult struct { // for every normal tool result; the Run loop performs the switch when it is // set and Options.ModelSwitcher is wired. RequestedModel string + // PlanSnapshot carries the typed, immutable snapshot of the []PlanItem + // accepted by a successful update_plan call, untampered by transcript scrubbing. + PlanSnapshot []tools.PlanItem `json:"-"` } // ModelOutput returns the bounded provider-facing result while preserving diff --git a/internal/planmode/export_test.go b/internal/planmode/export_test.go new file mode 100644 index 000000000..28f3018fd --- /dev/null +++ b/internal/planmode/export_test.go @@ -0,0 +1,12 @@ +package planmode + +import "testing" + +// SetTempDirForTest overrides the temp dir func for unit tests. Kept in +// export_test.go so the production planmode package (and therefore cmd/zero) +// does not import testing. +func SetTempDirForTest(t *testing.T, tempDir string) { + t.Helper() + restore := SetEffectiveTempDirForTest(tempDir) + t.Cleanup(restore) +} diff --git a/internal/planmode/fifo_other_test.go b/internal/planmode/fifo_other_test.go new file mode 100644 index 000000000..44c398c58 --- /dev/null +++ b/internal/planmode/fifo_other_test.go @@ -0,0 +1,9 @@ +//go:build !unix + +package planmode + +import "fmt" + +func mkfifoForTest(path string) error { + return fmt.Errorf("mkfifo not available on this platform") +} diff --git a/internal/planmode/fifo_unix_test.go b/internal/planmode/fifo_unix_test.go new file mode 100644 index 000000000..69c8fd484 --- /dev/null +++ b/internal/planmode/fifo_unix_test.go @@ -0,0 +1,9 @@ +//go:build unix + +package planmode + +import "syscall" + +func mkfifoForTest(path string) error { + return syscall.Mkfifo(path, 0o600) +} diff --git a/internal/planmode/physical_other.go b/internal/planmode/physical_other.go new file mode 100644 index 000000000..e55edc44e --- /dev/null +++ b/internal/planmode/physical_other.go @@ -0,0 +1,15 @@ +//go:build !windows + +package planmode + +import "path/filepath" + +// resolvePhysical returns path with every symlink component resolved. On +// non-Windows systems filepath.EvalSymlinks resolves every link type the +// platform has, so it is the whole implementation. +func resolvePhysical(path string) (string, error) { + return filepath.EvalSymlinks(path) +} + +// pathIsReparsePoint is a Windows concept; nothing here reports one. +func pathIsReparsePoint(string) bool { return false } diff --git a/internal/planmode/physical_windows.go b/internal/planmode/physical_windows.go new file mode 100644 index 000000000..db325f890 --- /dev/null +++ b/internal/planmode/physical_windows.go @@ -0,0 +1,120 @@ +//go:build windows + +package planmode + +import ( + "errors" + "fmt" + "path/filepath" + "strings" + + "golang.org/x/sys/windows" +) + +// resolvePhysical returns path in its canonical physical spelling. +// +// filepath.EvalSymlinks cannot do this alone on Windows. It resolves name +// surrogates (directory symlinks) but not junctions, which os.Lstat reports as +// os.ModeIrregular rather than os.ModeSymlink, so EvalSymlinks hands a junction +// straight back. A junction needs no SeCreateSymbolicLinkPrivilege, so it is +// the reparse point an unprivileged process can actually plant, and treating +// one as its own physical path lets a staging directory that really lands in +// the workspace or the OS temp directory compare as though it sits outside +// both. +// +// GetFinalPathNameByHandle asks the filesystem what the open handle resolved +// to, which is the only answer that accounts for every reparse type at once. +// VOLUME_NAME_DOS also returns long names, so it subsumes the 8.3 short-name +// normalization (RUNNER~1) the caller needs anyway. +func resolvePhysical(path string) (string, error) { + absolute, err := filepath.Abs(path) + if err != nil { + return "", err + } + pathUTF16, err := windows.UTF16PtrFromString(absolute) + if err != nil { + return "", err + } + // FILE_FLAG_BACKUP_SEMANTICS is required to open a directory handle, and + // no reparse flag is passed precisely so the open follows to the target + // this call is asking about. + handle, err := windows.CreateFile( + pathUTF16, + 0, // Query the name only; no read or write access is needed. + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS, + 0, + ) + if err != nil { + return "", err + } + defer windows.CloseHandle(handle) + + return finalPathName(handle) +} + +// fileNameNormalized|volumeNameDOS is the GetFinalPathNameByHandle flag pair +// that asks for the normalized (long-name) path with a drive letter. Both are +// zero, and x/sys/windows does not export either, so they are named here +// rather than left as a bare literal. +const ( + fileNameNormalized = 0x0 + volumeNameDOS = 0x0 +) + +// finalPathName reads the resolved path off an open handle, growing the buffer +// if the path is longer than MAX_PATH (a resolved path can be, which is why +// the API reports the size it needs). +func finalPathName(handle windows.Handle) (string, error) { + buf := make([]uint16, windows.MAX_PATH) + for range 2 { + // On success n excludes the terminating NUL; when the buffer is too + // small n is the required size INCLUDING it, so n >= len(buf) is the + // signal to grow rather than a result. + n, err := windows.GetFinalPathNameByHandle(handle, &buf[0], uint32(len(buf)), fileNameNormalized|volumeNameDOS) + if err != nil { + return "", err + } + if n < uint32(len(buf)) { + return trimExtendedLengthPrefix(windows.UTF16ToString(buf[:n])), nil + } + if n > windows.MAX_LONG_PATH { + return "", fmt.Errorf("resolved path needs %d UTF-16 units, over the %d limit", n, windows.MAX_LONG_PATH) + } + buf = make([]uint16, n) + } + return "", errors.New("resolved path length kept growing between calls") +} + +// trimExtendedLengthPrefix converts the extended-length spelling +// GetFinalPathNameByHandle returns back to the ordinary Win32 form, so the +// result compares against paths spelled the way the rest of the process +// spells them. `\\?\UNC\server\share` is a UNC path, not a drive path, and +// has to become `\\server\share` rather than `UNC\server\share`. +func trimExtendedLengthPrefix(path string) string { + if rest, ok := strings.CutPrefix(path, `\\?\UNC\`); ok { + return `\\` + rest + } + if rest, ok := strings.CutPrefix(path, `\\?\`); ok { + return rest + } + return path +} + +// pathIsReparsePoint reports whether path itself is a reparse point of any +// kind, junctions included. os.Lstat cannot answer this: it maps a junction to +// os.ModeIrregular, which is indistinguishable from other irregular files, so +// verifyPrivateDirectory's os.ModeSymlink test never fires for one. +func pathIsReparsePoint(path string) bool { + pathUTF16, err := windows.UTF16PtrFromString(path) + if err != nil { + return false + } + attrs, err := windows.GetFileAttributes(pathUTF16) + if err != nil { + return false + } + return attrs&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 +} diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go new file mode 100644 index 000000000..3dcf066ad --- /dev/null +++ b/internal/planmode/planmode.go @@ -0,0 +1,486 @@ +package planmode + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "time" + + "github.com/Gitlawb/zero/internal/config" +) + +// PlanDirName is the config-relative directory (under UserConfigDir) where +// durable /plan files live. Plans are kept outside the workspace so the +// auto-allowed, read-only update_plan tool can persist without a write grant +// and without mutating the workspace. +const PlanDirName = "zero/plans" + +// DraftSystemPrompt is the system prompt the TUI runs while /plan mode is active +// on the current session. It is read-only: the agent inspects the workspace and +// shapes the plan, but must not mutate anything until plan mode is exited. +const DraftSystemPrompt = `Plan mode is active on this session. + +You are planning an implementation, not changing files. + +Use read-only tools to inspect the workspace. You may use ask_user only when a +decision is genuinely blocking and cannot be resolved from the workspace or a +reasonable safe assumption. + +Do not write files, edit files, apply patches, run shell commands, spawn +specialists, or implement the requested change while in plan mode. + +Capture the plan with update_plan as you work. When the user is ready to +implement, they exit plan mode and you continue normally. + +The plan should converge on one concrete approach. Do not leave unresolved +choices such as "Option A" and "Option B". If something remains uncertain, make +the safest reasonable assumption and state it clearly.` + +// PlanFilePath returns the deterministic, absolute plan file path for a +// session under the per-user config plans directory, scoped by workspace so +// two workspaces never share a plan file. It performs no filesystem access; +// ReadPlan and WritePlan are the safe way to actually read or write plan +// content. +// +// Directory and file names are collision-resistant: slugify alone would map +// distinct IDs such as "plan_a" and "plan-a" (or workspaces "foo_bar" and +// "foo-bar") onto the same path, so a durable plan for one session/workspace +// could be read or overwritten by another. pathKey appends a content hash of +// the exact original string so the mapping is injective. +func PlanFilePath(workspaceRoot, sessionID string) (string, error) { + base, absWorkspace, err := planStorageBase(workspaceRoot) + if err != nil { + return "", err + } + return filepath.Join(base, pathKey(absWorkspace), pathKey(sessionID)+".md"), nil +} + +// ReadPlan reads the plan file for a session. The bool reports whether a plan +// file exists; a missing file is not an error. +// +// Containment is bound at open time via a rooted, handle-relative open under +// the plan storage base (see readPlanFile). Pre-open path checks alone are a +// check-to-use race: an intermediate directory can be replaced with a symlink +// or reparse point between resolve and open. +func ReadPlan(workspaceRoot, sessionID string) (string, bool, error) { + path, err := PlanFilePath(workspaceRoot, sessionID) + if err != nil { + return "", false, err + } + if err := ensurePlanPathContained(workspaceRoot, path); err != nil { + return "", false, err + } + base, _, err := planStorageBase(workspaceRoot) + if err != nil { + return "", false, err + } + data, err := readPlanFile(base, path) + if err != nil { + if os.IsNotExist(err) { + return "", false, nil + } + // Symlink refusals from the reader are already fully formed. + if errors.Is(err, errPlanSymlinkRefusal) { + return "", false, err + } + return "", false, fmt.Errorf("read plan file: %w", err) + } + return string(data), true, nil +} + +// WritePlan writes (creating the directory as needed) the plan file for a +// session and returns its path. The file is stored under the user config +// directory, never inside the workspace, so an auto-allowed read-only tool +// can persist without a workspace write grant. +// +// Containment is bound at create/rename time via a rooted, handle-relative +// no-follow walk under the plan storage base (see writePlanFile). Pre-open +// path checks alone are a check-to-use race: an intermediate directory can +// be replaced with a symlink between resolve and create, and pathname +// MkdirAll/OpenFile/Rename would then land outside the storage tree. +func WritePlan(workspaceRoot, sessionID, content string) (string, error) { + path, err := PlanFilePath(workspaceRoot, sessionID) + if err != nil { + return "", err + } + if err := ensurePlanPathContained(workspaceRoot, path); err != nil { + return "", err + } + base, _, err := planStorageBase(workspaceRoot) + if err != nil { + return "", err + } + body := strings.TrimRight(content, "\n") + "\n" + if err := writePlanFile(base, path, body); err != nil { + return "", err + } + return path, nil +} + +// StageForEditor copies a session's current plan content (read safely via +// ReadPlan) into a fresh file outside the workspace, for handing to an +// external $EDITOR process launched by /plan open. +// +// Handing $EDITOR a path at the durable plan location would leave a +// symlink-swap race between our protected write and the editor's open. The +// OS temp directory does not avoid this either: the sandbox's default write +// scope explicitly includes it (see defaultTempWriteRootCandidates in +// internal/sandbox), so a sandboxed process could plant the same symlink +// there. config.UserConfigDir() is usually outside that default scope, but +// it honors XDG_CONFIG_HOME (on macOS explicitly here, on Linux via +// os.UserConfigDir itself), so a misconfigured or sandboxed-process +// environment pointing that at the workspace or the OS temp dir would put +// the staging directory right back in a default-writable root. +// editorStagingDirIsPrivate rejects that case instead of silently staging +// somewhere unsafe. +// +// Two more layers close the remaining gap even when the directory itself is +// private: the filename includes a random, per-invocation suffix (os.CreateTemp) +// so a sandboxed process can't pre-plant a symlink at a path it can't predict, +// and CreateTemp opens with O_EXCL, so even a guessed or colliding path is +// refused rather than followed if something is already there. The random +// suffix also means two Zero instances editing the same resumed session no +// longer collide on the same staged file. +func StageForEditor(workspaceRoot, sessionID string) (stagedPath string, cleanup func(), err error) { + content, _, err := ReadPlan(workspaceRoot, sessionID) + if err != nil { + return "", nil, err + } + dir, err := editorStagingDir() + if err != nil { + return "", nil, err + } + // Create the directory before judging it, then judge (and use) its + // PHYSICAL path: a lexical check would pass an XDG_CONFIG_HOME that is + // itself a symlink into the workspace or the OS temp directory, while + // MkdirAll/CreateTemp followed the link and staged the file somewhere a + // sandboxed process can write. Resolving after MkdirAll also covers a + // pre-existing staging directory that was replaced with a symlink, and + // anchoring the staging on the resolved path means the file is created + // where it was checked, not wherever the link points next. + if err := os.MkdirAll(dir, 0o700); err != nil { + return "", nil, fmt.Errorf("create plan editor staging directory: %w", err) + } + resolvedDir, err := resolvePhysical(dir) + if err != nil { + return "", nil, fmt.Errorf("resolve plan editor staging directory: %w", err) + } + // Use effectiveTempDir (not os.TempDir) so SetTempDirForTest can redirect + // the privacy check the same way ensurePlanPathContained does. + if !editorStagingDirIsPrivate(resolvedDir, workspaceRoot, effectiveTempDir()) { + return "", nil, fmt.Errorf("plan editor staging directory %s resolves into a default sandbox-writable root (the workspace or the OS temp directory); check XDG_CONFIG_HOME", dir) + } + // MkdirAll's mode only applies at creation: tighten an existing directory + // only after validating its resolved target is safe to use. + if err := os.Chmod(resolvedDir, 0o700); err != nil { + return "", nil, fmt.Errorf("restrict plan editor staging directory permissions: %w", err) + } + // Verify the resolved directory after chmod: refuse anything that is not + // a plain directory or that is still group/world-writable. A pre-existing + // sticky or ACL-permissive directory that chmod could not fully lock down + // must not host a closed staged file the unsandboxed editor will reopen. + if err := verifyPrivateDirectory(resolvedDir); err != nil { + return "", nil, fmt.Errorf("plan editor staging directory: %w", err) + } + // tea.ExecProcess's cleanup closure only runs if the caller's Bubble Tea + // program lives long enough to invoke it: a shutdown that drops the + // pending command (e.g. the terminal or parent process dying while the + // editor is open) skips the callback, and the staged file it would have + // removed leaks. Sweep those abandoned files on the next stage instead of + // relying on every shutdown path to run cleanup. + sweepStaleStagedFiles(resolvedDir) + return stageContentForEditor(resolvedDir, sessionID, content) +} + +// staleStagedEditThreshold bounds how long an abandoned staged plan file can +// linger before sweepStaleStagedFiles reclaims it. The window must comfortably +// outlast any real interactive edit so a slow user never loses the file out +// from under their open editor. +const staleStagedEditThreshold = 6 * time.Hour + +// sweepStaleStagedFiles removes staged plan files in dir whose mtime is older +// than staleStagedEditThreshold and whose lock is proven to be abandoned. +// Best-effort: errors are ignored, since a failed sweep must not block staging +// a new file. Unrelated files (not matching the plan format) are never touched. +func sweepStaleStagedFiles(dir string) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + cutoff := time.Now().Add(-staleStagedEditThreshold) + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if !strings.HasSuffix(name, ".md") { + continue + } + info, err := entry.Info() + if err != nil || info.ModTime().After(cutoff) { + continue + } + tryReclaimStaleStagedFile(dir, name) + } +} + +// stageContentForEditor creates a fresh, uniquely-named file under dir +// holding content, for StageForEditor to hand to $EDITOR. It binds creation to +// the opened directory handle rather than repeating pathname traversals. +func stageContentForEditor(dir, sessionID, content string) (stagedPath string, cleanup func(), err error) { + return stageContentUnderBase(dir, sessionID, content) +} + +// editorStagingDirIsPrivate reports whether dir avoids the sandbox's default +// writable roots (tempDir, normally os.TempDir(), and the workspace itself), +// which are writable from inside the sandbox by default regardless of any +// extra grant. All three paths are compared in physical form: dir or either +// root may be reached through symlinks (an XDG_CONFIG_HOME symlinked into +// the workspace, macOS's /var -> /private/var), and a lexical comparison of +// unlike spellings would wave a staging directory through a boundary it +// actually sits inside. tempDir is a parameter so tests can exercise the +// symlink cases without needing to plant links outside the real temp dir. +func editorStagingDirIsPrivate(dir, workspaceRoot, tempDir string) bool { + dir = physicalPath(dir) + if isUnderOrEqual(dir, physicalPath(tempDir)) { + return false + } + // Fail closed if the workspace root cannot be resolved (e.g. deleted + // CWD makes filepath.Abs fail): do not treat an unresolvable workspace + // as "private" and skip the containment check. + absRoot, err := filepath.Abs(workspaceRoot) + if err != nil { + return false + } + if isUnderOrEqual(dir, physicalPath(absRoot)) { + return false + } + return true +} + +// verifyPrivateDirectory reports an error when path is not a plain directory +// or is still group/world-writable after the caller tightened it. Symlinks +// are rejected via Lstat so a TOCTOU swap of the directory for a link cannot +// host a staged file that $EDITOR will follow. Windows junctions are rejected +// separately: os.Lstat maps one to os.ModeIrregular, not os.ModeSymlink, so +// the check above cannot see it. The permission-bit check is skipped on +// Windows: NTFS reports a directory's POSIX mode via ACLs rather than the bits +// os.Chmod sets, so it does not reflect what os.Chmod(0o700) actually +// restricted (see the same rationale on the file-mode check in +// TestWritePlanUsesRestrictivePermissions) — containment there rests on +// editorStagingDirIsPrivate and on the reparse-point rejection here. +func verifyPrivateDirectory(path string) error { + info, err := os.Lstat(path) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("%s is a symlink; refusing to stage through it", path) + } + if pathIsReparsePoint(path) { + return fmt.Errorf("%s is a reparse point; refusing to stage through it", path) + } + if !info.IsDir() { + return fmt.Errorf("%s is not a directory", path) + } + if runtime.GOOS == "windows" { + return nil + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + return fmt.Errorf("%s is group/world-writable (mode %o) after restriction", path, perm) + } + return nil +} + +// physicalPath resolves symlinks best-effort. A path that does not exist yet +// is resolved through its deepest existing ancestor with the remainder +// rejoined, so a not-yet-created staging directory still compares in the +// same physical spelling as the (existing, resolved) roots: without this, +// macOS's /var vs /private/var and Windows's 8.3 short names (RUNNER~1) +// would make the containment comparison silently miss. +// +// Resolution goes through resolvePhysical rather than filepath.EvalSymlinks +// directly because EvalSymlinks does not traverse a Windows junction, and a +// junction is the one reparse point an unprivileged process can plant. See +// resolvePhysical in physical_windows.go. +func physicalPath(path string) string { + if resolved, err := resolvePhysical(path); err == nil { + return resolved + } + cleaned := filepath.Clean(path) + parent := filepath.Dir(cleaned) + if parent == cleaned { + // Reached a filesystem root that itself cannot be resolved. + return cleaned + } + return filepath.Join(physicalPath(parent), filepath.Base(cleaned)) +} + +// isUnderOrEqual reports whether path is root itself or a descendant of it. +func isUnderOrEqual(path, root string) bool { + rel, err := filepath.Rel(root, path) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} + +// CommitStagedEdit reads a file staged by StageForEditor (now edited by the +// user's $EDITOR) and writes its content back into the durable plan store +// via WritePlan. stagedPath must be a path produced by StageForEditor. +func CommitStagedEdit(workspaceRoot, sessionID, stagedPath string) error { + data, err := os.ReadFile(stagedPath) + if err != nil { + return fmt.Errorf("read staged plan file: %w", err) + } + _, err = WritePlan(workspaceRoot, sessionID, string(data)) + return err +} + +// editorStagingDir is where plan files are staged for external $EDITOR +// access. See StageForEditor for why this location, not the OS temp +// directory, is what actually closes the containment race. +func editorStagingDir() (string, error) { + dir, err := config.UserConfigDir() + if err != nil { + return "", fmt.Errorf("resolve editor staging directory: %w", err) + } + return filepath.Join(dir, "zero", "plan-edit"), nil +} + +// planStorageBase returns the absolute user-config plans root and the +// absolute workspace path used to scope per-workspace plan files. +func planStorageBase(workspaceRoot string) (base string, absWorkspace string, err error) { + if strings.TrimSpace(workspaceRoot) == "" { + return "", "", fmt.Errorf("workspace root is required") + } + absWorkspace, err = filepath.Abs(workspaceRoot) + if err != nil { + return "", "", fmt.Errorf("resolve workspace root: %w", err) + } + cfg, err := config.UserConfigDir() + if err != nil { + return "", "", fmt.Errorf("resolve plan storage directory: %w", err) + } + return filepath.Join(cfg, filepath.FromSlash(PlanDirName)), absWorkspace, nil +} + +var ( + tempDirMu sync.RWMutex + tempDirFn = os.TempDir +) + +func effectiveTempDir() string { + tempDirMu.RLock() + defer tempDirMu.RUnlock() + return tempDirFn() +} + +// SetEffectiveTempDirForTest overrides the temp dir func during tests, returning +// a restore function to reset it. +func SetEffectiveTempDirForTest(tempDir string) func() { + tempDirMu.Lock() + old := tempDirFn + tempDirFn = func() string { return tempDir } + tempDirMu.Unlock() + return func() { + tempDirMu.Lock() + tempDirFn = old + tempDirMu.Unlock() + } +} + +// ensurePlanPathContained verifies that path stays under the config plans +// root and does not resolve into the workspace or OS temp directory. A mis-set XDG_CONFIG_HOME +// pointing at the workspace or temp tree would otherwise turn every update_plan +// persistence into a silent workspace or sandbox-writable write. +func ensurePlanPathContained(workspaceRoot, path string) error { + base, absWorkspace, err := planStorageBase(workspaceRoot) + if err != nil { + return err + } + physPath := physicalPath(path) + physBase := physicalPath(base) + if !isUnderOrEqual(physPath, physBase) { + return fmt.Errorf("plan path %s escapes plan storage root %s", path, base) + } + if isUnderOrEqual(physPath, physicalPath(absWorkspace)) { + return fmt.Errorf("plan storage %s resolves into the workspace; check XDG_CONFIG_HOME", path) + } + if physTemp := physicalPath(effectiveTempDir()); physTemp != "" && isUnderOrEqual(physPath, physTemp) { + return fmt.Errorf("plan storage %s resolves into temp directory %s; check XDG_CONFIG_HOME", path, physTemp) + } + return nil +} + +// maxPathKeySlug is the max length of the human-readable slug prefix in a +// pathKey component. The SHA-256 suffix (32 hex chars) plus separator keep the +// full component well under NAME_MAX (255) even for very deep workspace paths. +const maxPathKeySlug = 64 + +// pathKey builds a filesystem-safe, collision-resistant directory or file +// stem from an arbitrary workspace path or session ID. The human-readable +// slug prefix is for operator convenience only; the SHA-256 suffix makes the +// key injective so distinct inputs never share a plan path. +func pathKey(id string) string { + rawID := id + if strings.TrimSpace(rawID) == "" { + // A stable fallback, not a per-call timestamp: PlanFilePath is called + // independently from several sites (planEnterText, planText, + // openPlanInEditor) before a session ID may exist, and they must all + // resolve to the same file rather than a fresh one each time. The + // sentinel is namespaced so it cannot collide with a session whose + // ID is literally "plan" (which would break injectivity of the map). + rawID = "\x00no-session" + } + sum := sha256.Sum256([]byte(rawID)) + // Truncate the slug so a deep workspace path cannot produce a single + // directory component over NAME_MAX. The hash keeps the key injective. + slug := slugify(id) + if len(slug) > maxPathKeySlug { + slug = strings.Trim(slug[:maxPathKeySlug], "-") + if slug == "" { + slug = "plan" + } + } + return slug + "-" + hex.EncodeToString(sum[:16]) +} + +// slugify turns an arbitrary session identifier into a filesystem-safe slug. +// It is lossy (see pathKey): do not use it alone as a durable storage key. +func slugify(id string) string { + id = strings.TrimSpace(id) + if id == "" { + id = "plan" + } + var b strings.Builder + prevDash := false + for _, r := range strings.ToLower(id) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + prevDash = false + case r == '-' || r == '_' || r == '/': + if !prevDash && b.Len() > 0 { + b.WriteRune('-') + prevDash = true + } + default: + if !prevDash && b.Len() > 0 { + b.WriteRune('-') + prevDash = true + } + } + } + out := strings.Trim(b.String(), "-") + if out == "" { + out = "plan" + } + return out +} diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go new file mode 100644 index 000000000..ae1f148ed --- /dev/null +++ b/internal/planmode/planmode_test.go @@ -0,0 +1,1217 @@ +package planmode + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +// setUserConfigHomeEnv points config.UserConfigDir and related user directories at dir. +// It redirects HOME, USERPROFILE, XDG_CONFIG_HOME, XDG_CACHE_HOME, AppData, and LocalAppData +// so tests are fully hermetic across Linux, macOS, and Windows. +func setUserConfigHomeEnv(t *testing.T, dir string) { + t.Helper() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + t.Setenv("XDG_CONFIG_HOME", dir) + t.Setenv("XDG_CACHE_HOME", filepath.Join(dir, "cache")) + t.Setenv("AppData", dir) + t.Setenv("LocalAppData", filepath.Join(dir, "local")) +} + +// isolatePlanStorage redirects the user config root so plan files land under a +// throwaway directory rather than the real ~/.config. Durable plans live under +// UserConfigDir (not the workspace), so every planmode test must isolate it. +func isolatePlanStorage(t *testing.T) string { + t.Helper() + root := t.TempDir() + configDir := filepath.Join(root, "config") + tempDir := filepath.Join(root, "tmp") + _ = os.MkdirAll(configDir, 0o700) + _ = os.MkdirAll(tempDir, 0o700) + setUserConfigHomeEnv(t, configDir) + SetTempDirForTest(t, tempDir) + return configDir +} + +func TestPlanFilePathSeparatesSlugCollisions(t *testing.T) { + // slugify alone maps '_' and '-' to the same dash form, so plan_a and + // plan-a (and workspaces foo_bar / foo-bar) must not share a path. + isolatePlanStorage(t) + root := t.TempDir() + a, err := PlanFilePath(root, "plan_a") + if err != nil { + t.Fatalf("PlanFilePath plan_a: %v", err) + } + b, err := PlanFilePath(root, "plan-a") + if err != nil { + t.Fatalf("PlanFilePath plan-a: %v", err) + } + if a == b { + t.Fatalf("slug-colliding session IDs must not share a plan path, both %q", a) + } + + wsA := filepath.Join(root, "foo_bar") + wsB := filepath.Join(root, "foo-bar") + if err := os.MkdirAll(wsA, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(wsB, 0o700); err != nil { + t.Fatal(err) + } + pathA, err := PlanFilePath(wsA, "session-1") + if err != nil { + t.Fatalf("PlanFilePath wsA: %v", err) + } + pathB, err := PlanFilePath(wsB, "session-1") + if err != nil { + t.Fatalf("PlanFilePath wsB: %v", err) + } + if pathA == pathB { + t.Fatalf("slug-colliding workspaces must not share a plan path, both %q", pathA) + } + if filepath.Dir(pathA) == filepath.Dir(pathB) { + t.Fatalf("workspace path keys collided: %q and %q share dir", pathA, pathB) + } +} + +func TestPlanFilePathIsStableAcrossCalls(t *testing.T) { + isolatePlanStorage(t) + root := t.TempDir() + first, err := PlanFilePath(root, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + second, err := PlanFilePath(root, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if first != second { + t.Fatalf("expected stable path for the same session, got %q then %q", first, second) + } +} + +func TestPlanFilePathEmptySessionIsStable(t *testing.T) { + // PlanFilePath(root, "") is called independently from several TUI call + // sites before a session ID may exist (planEnterText, planText, + // openPlanInEditor); they must all resolve to the same file rather than a + // fresh one each call (regression for the old time.Now().UnixNano() slug). + isolatePlanStorage(t) + root := t.TempDir() + first, err := PlanFilePath(root, "") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + second, err := PlanFilePath(root, "") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if first != second { + t.Fatalf("expected stable path for an empty session id, got %q then %q", first, second) + } +} + +func TestPlanFilePathLivesOutsideWorkspace(t *testing.T) { + // Regression for the update_plan auto-persist write: durable plan state + // must not land under the workspace, or a read-only auto-allowed tool + // would create/overwrite workspace files without a write grant. + cfg := isolatePlanStorage(t) + workspace := t.TempDir() + path, err := PlanFilePath(workspace, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if isUnderOrEqual(path, workspace) { + t.Fatalf("plan path %q must not live under the workspace %q", path, workspace) + } + if !isUnderOrEqual(path, cfg) { + t.Fatalf("plan path %q must live under the user config root %q", path, cfg) + } + if !strings.Contains(path, filepath.FromSlash(PlanDirName)) { + t.Fatalf("plan path %q must include %q", path, PlanDirName) + } +} + +func TestWritePlanUsesRestrictivePermissions(t *testing.T) { + // Windows reports 0666 for a plan file regardless of the mode passed to + // OpenFile - NTFS permissions are governed by ACLs, not the POSIX mode + // bits Go maps them to. Assert the mode bits only where they mean + // something; Windows containment relies on path isolation instead. + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + isolatePlanStorage(t) + root := t.TempDir() + path, err := WritePlan(root, "session-1", "notes") + if err != nil { + t.Fatalf("WritePlan: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat plan file: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("expected plan file mode 0600, got %o", perm) + } + dirInfo, err := os.Stat(filepath.Dir(path)) + if err != nil { + t.Fatalf("stat plan dir: %v", err) + } + if perm := dirInfo.Mode().Perm(); perm != 0o700 { + t.Fatalf("expected plan dir mode 0700, got %o", perm) + } +} + +func TestWritePlanTightensPreExistingLoosePermissions(t *testing.T) { + // Regression: MkdirAll/OpenFile's mode argument only applies at creation + // time, so a pre-existing 0755 plan directory or 0644 plan file (e.g. + // predating this restriction, or created some other way) stayed + // group/other-readable forever after, contrary to the owner-only + // storage contract WritePlan is supposed to enforce on every write. + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + isolatePlanStorage(t) + root := t.TempDir() + path, err := PlanFilePath(root, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + planDir := filepath.Dir(path) + if err := os.MkdirAll(planDir, 0o755); err != nil { + t.Fatalf("pre-create loose plan dir: %v", err) + } + if err := os.WriteFile(path, []byte("stale"), 0o644); err != nil { + t.Fatalf("pre-create loose plan file: %v", err) + } + + written, err := WritePlan(root, "session-1", "notes") + if err != nil { + t.Fatalf("WritePlan: %v", err) + } + info, err := os.Stat(written) + if err != nil { + t.Fatalf("stat plan file: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("expected pre-existing plan file tightened to mode 0600, got %o", perm) + } + dirInfo, err := os.Stat(planDir) + if err != nil { + t.Fatalf("stat plan dir: %v", err) + } + if perm := dirInfo.Mode().Perm(); perm != 0o700 { + t.Fatalf("expected pre-existing plan dir tightened to mode 0700, got %o", perm) + } +} + +func TestWritePlanDoesNotTouchWorkspace(t *testing.T) { + // Core P1 regression: persisting a plan must not create anything under + // the workspace, even via .zero/plans (the previous location). + isolatePlanStorage(t) + workspace := t.TempDir() + if _, err := WritePlan(workspace, "session-1", "notes"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + if _, err := os.Stat(filepath.Join(workspace, ".zero")); !os.IsNotExist(err) { + t.Fatalf("WritePlan must not create .zero under the workspace, stat err=%v", err) + } + entries, err := os.ReadDir(workspace) + if err != nil { + t.Fatalf("ReadDir workspace: %v", err) + } + if len(entries) != 0 { + t.Fatalf("expected empty workspace after WritePlan, got %v", entries) + } +} + +func TestReadWritePlanRoundtrip(t *testing.T) { + isolatePlanStorage(t) + root := t.TempDir() + if _, err := WritePlan(root, "session-1", "# Draft\n\nStep one."); err != nil { + t.Fatalf("WritePlan: %v", err) + } + content, ok, err := ReadPlan(root, "session-1") + if err != nil { + t.Fatalf("ReadPlan: %v", err) + } + if !ok { + t.Fatal("expected ReadPlan to report the file exists") + } + if content != "# Draft\n\nStep one.\n" { + t.Fatalf("unexpected plan content: %q", content) + } +} + +func TestReadPlanMissingFileIsNotAnError(t *testing.T) { + isolatePlanStorage(t) + root := t.TempDir() + _, ok, err := ReadPlan(root, "no-such-session") + if err != nil { + t.Fatalf("ReadPlan: %v", err) + } + if ok { + t.Fatal("expected ReadPlan to report no file for a session that never opened one") + } +} + +// TestReadPlanMissingSessionWithBasePresent covers the NtCreateFile / +// openat walk when the plan storage base exists (another session already +// wrote a plan) but this session's path is absent. Missing-file NTSTATUS +// values must map to os.ErrNotExist so ReadPlan returns ("", false, nil). +func TestReadPlanMissingSessionWithBasePresent(t *testing.T) { + isolatePlanStorage(t) + root := t.TempDir() + if _, err := WritePlan(root, "other-session", "notes"); err != nil { + t.Fatalf("WritePlan other: %v", err) + } + content, ok, err := ReadPlan(root, "no-such-session") + if err != nil { + t.Fatalf("ReadPlan missing session: %v", err) + } + if ok { + t.Fatal("expected missing session plan to report ok=false") + } + if content != "" { + t.Fatalf("expected empty content for missing plan, got %q", content) + } +} + +func TestPathKeyLongWorkspaceWithinNameMax(t *testing.T) { + // slugify keeps one output character per path character, so a workspace + // path longer than NAME_MAX would produce an oversized directory component + // without the pathKey slug cap. The hash suffix keeps injectivity. + isolatePlanStorage(t) + longSeg := strings.Repeat("deepseg", 40) // 280 chars + root := filepath.Join(t.TempDir(), longSeg, longSeg) + if len(root) <= 255 { + t.Fatalf("setup: expected workspace path >255 chars, got %d", len(root)) + } + path, err := PlanFilePath(root, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + for _, part := range strings.Split(path, string(os.PathSeparator)) { + if part == "" { + continue + } + if len(part) > 255 { + t.Fatalf("path component %q exceeds NAME_MAX (255), len=%d", part, len(part)) + } + } + // Write/read must succeed: MkdirAll would ENAMETOOLONG without the cap. + if _, err := WritePlan(root, "session-1", "long path plan"); err != nil { + t.Fatalf("WritePlan long workspace: %v", err) + } + content, ok, err := ReadPlan(root, "session-1") + if err != nil { + t.Fatalf("ReadPlan long workspace: %v", err) + } + if !ok || content != "long path plan\n" { + t.Fatalf("unexpected plan after long-workspace write: ok=%v content=%q", ok, content) + } + // Distinct long workspaces still get distinct keys (hash injectivity). + other := root + "-other" + pathOther, err := PlanFilePath(other, "session-1") + if err != nil { + t.Fatalf("PlanFilePath other: %v", err) + } + if path == pathOther { + t.Fatalf("long workspaces must not share a plan path: %q", path) + } +} + +func TestWritePlanRejectsSymlinkedPlanFile(t *testing.T) { + isolatePlanStorage(t) + root := t.TempDir() + path, err := PlanFilePath(root, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir plan dir: %v", err) + } + outsideFile := filepath.Join(t.TempDir(), "exfil.md") + if err := os.WriteFile(outsideFile, []byte("secret"), 0o600); err != nil { + t.Fatalf("write outside file: %v", err) + } + if err := os.Symlink(outsideFile, path); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if _, err := WritePlan(root, "session-1", "notes"); err == nil { + t.Fatal("expected WritePlan to reject a symlinked plan file") + } + if _, _, err := ReadPlan(root, "session-1"); err == nil { + t.Fatal("expected ReadPlan to reject a symlinked plan file") + } + // Victim content must be untouched. + data, err := os.ReadFile(outsideFile) + if err != nil { + t.Fatalf("read outside file: %v", err) + } + if string(data) != "secret" { + t.Fatalf("symlinked victim was modified: %q", data) + } +} + +// TestReadPlanFileRejectsIntermediateSymlink covers the bind-at-open property +// that final-component-only O_NOFOLLOW does not provide: a parent component +// replaced with a symlink (or Windows reparse point) to a directory outside +// the plan storage base must not yield the outside file's contents. +// +// Called against readPlanFile directly so the pre-open EvalSymlinks check in +// ensurePlanPathContained cannot mask a weak open. On Windows, os.Symlink for +// a directory creates a reparse point when the privilege is available. +func TestReadPlanFileRejectsIntermediateSymlink(t *testing.T) { + base := t.TempDir() + outside := t.TempDir() + secret := []byte("outside-secret\n") + if err := os.WriteFile(filepath.Join(outside, "plan.md"), secret, 0o600); err != nil { + t.Fatalf("write outside plan: %v", err) + } + parentLink := filepath.Join(base, "ws-key") + if err := os.Symlink(outside, parentLink); err != nil { + t.Skipf("directory symlinks/reparse points unavailable: %v", err) + } + path := filepath.Join(parentLink, "plan.md") + + data, err := readPlanFile(base, path) + if err == nil { + t.Fatalf("expected intermediate symlink to be refused, got content %q", data) + } + if len(data) > 0 { + t.Fatalf("refused read must not return bytes, got %q", data) + } + // Victim outside the base must be untouched and must not have been + // returned as a successful plan read. + got, err := os.ReadFile(filepath.Join(outside, "plan.md")) + if err != nil { + t.Fatalf("read outside plan: %v", err) + } + if string(got) != string(secret) { + t.Fatalf("outside plan was modified: %q", got) + } +} + +func TestReadPlanFileRejectsFinalSymlink(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "ws-key") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + outsideFile := filepath.Join(t.TempDir(), "exfil.md") + if err := os.WriteFile(outsideFile, []byte("secret"), 0o600); err != nil { + t.Fatalf("write outside: %v", err) + } + path := filepath.Join(dir, "session.md") + if err := os.Symlink(outsideFile, path); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + data, err := readPlanFile(base, path) + if err == nil { + t.Fatalf("expected final symlink to be refused, got %q", data) + } + if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected symlink refusal, got: %v", err) + } +} + +// TestReadPlanFileRejectsInRootFinalSymlink covers the os.Root.Open race that +// root.Lstat-then-root.Open cannot close: when the final name is replaced with +// a symlink whose target remains inside the storage base, os.Root.Open follows +// it via checkSymlink after O_NOFOLLOW fails. The no-follow walker must refuse +// without returning the in-root target's contents. +// +// This is the sequential stand-in for the Lstat/Open TOCTOU: plant the final +// symlink before open and prove we never follow it, even in-root. +func TestReadPlanFileRejectsInRootFinalSymlink(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "ws-key") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + other := filepath.Join(dir, "other-plan.md") + if err := os.WriteFile(other, []byte("other-plan\n"), 0o600); err != nil { + t.Fatalf("write other plan: %v", err) + } + path := filepath.Join(dir, "requested.md") + // Relative target stays inside the base, which is exactly the case + // os.Root.Open would follow. + if err := os.Symlink("other-plan.md", path); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + data, err := readPlanFile(base, path) + if err == nil { + t.Fatalf("expected in-root final symlink to be refused, got %q", data) + } + if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected symlink refusal, got: %v", err) + } + if len(data) > 0 { + t.Fatalf("refused read must not return bytes, got %q", data) + } + // Victim in-root target must be untouched and must not have been returned. + got, err := os.ReadFile(other) + if err != nil { + t.Fatalf("read other plan: %v", err) + } + if string(got) != "other-plan\n" { + t.Fatalf("other plan was modified: %q", got) + } +} + +// TestReadPlanFileRefusesAfterReplaceWithSymlink simulates the Lstat/Open +// replace-with-symlink race: a regular plan is swapped for an in-root symlink +// before readPlanFile runs. The open must refuse rather than follow. +func TestReadPlanFileRefusesAfterReplaceWithSymlink(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "ws-key") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + path := filepath.Join(dir, "session.md") + if err := os.WriteFile(path, []byte("requested-plan\n"), 0o600); err != nil { + t.Fatalf("write plan: %v", err) + } + other := filepath.Join(dir, "other.md") + if err := os.WriteFile(other, []byte("other-plan\n"), 0o600); err != nil { + t.Fatalf("write other: %v", err) + } + + // Sequential stand-in for the race window: remove the regular file and + // plant an in-root symlink at the same name before the open. + if err := os.Remove(path); err != nil { + t.Fatalf("remove plan: %v", err) + } + if err := os.Symlink("other.md", path); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + data, err := readPlanFile(base, path) + if err == nil { + t.Fatalf("expected replaced-with-symlink plan to be refused, got %q", data) + } + if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected symlink refusal, got: %v", err) + } + if string(data) == "other-plan\n" { + t.Fatal("open followed the in-root symlink planted after the regular file existed") + } +} + +func TestReadPlanFileRoundtripPlainFile(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "ws-key") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + path := filepath.Join(dir, "session.md") + want := "# plan\n\nstep one\n" + if err := os.WriteFile(path, []byte(want), 0o600); err != nil { + t.Fatalf("write plan: %v", err) + } + got, err := readPlanFile(base, path) + if err != nil { + t.Fatalf("readPlanFile: %v", err) + } + if string(got) != want { + t.Fatalf("content = %q, want %q", got, want) + } +} + +// TestReadPlanFileRejectsNonRegularFile pins the non-regular refusal: +// Unix: a planted FIFO must not hang open (O_NONBLOCK) and must be refused +// as "not a regular file". Windows: a directory at the plan path is refused +// the same way (no FIFO create API). +func TestReadPlanFileRejectsNonRegularFile(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "ws-key") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + path := filepath.Join(dir, "session.md") + + if runtime.GOOS == "windows" { + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("mkdir plan path: %v", err) + } + } else { + if err := mkfifoForTest(path); err != nil { + t.Skipf("mkfifo unavailable: %v", err) + } + } + + done := make(chan error, 1) + go func() { + _, err := readPlanFile(base, path) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("expected a non-regular plan path to be refused") + } + if !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("expected the regular-file refusal, got: %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("readPlanFile blocked on a non-regular target; O_NONBLOCK (or equivalent) is missing from the final open") + } +} + +// TestPlanStorageBaseSymlinkRefused covers a symlink or reparse point at the +// plan storage root itself, as opposed to a component under it. +// ensurePlanPathContained resolves the base and the plan path through the same +// link, so containment passes unless the target happens to be the workspace or +// temp directory. Before the base was opened no-follow, the handle-relative +// walk was simply rooted inside the link's target, so every read, create, and +// rename landed there while each individual component check still passed. +func TestPlanStorageBaseSymlinkRefused(t *testing.T) { + if runtime.GOOS == "windows" { + // Directory symlink creation is privileged on many Windows runners. + t.Skip("directory symlink creation is privileged on Windows CI") + } + cfg := isolatePlanStorage(t) + workspace := t.TempDir() + + // Seed a real plan so the read path has something to find if it followed + // the link, rather than failing for an unrelated missing-file reason. + if _, err := WritePlan(workspace, "session-1", "1. [pending] real step\n"); err != nil { + t.Fatalf("WritePlan (seed): %v", err) + } + plansRoot := filepath.Join(cfg, filepath.FromSlash(PlanDirName)) + elsewhere := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(elsewhere, 0o700); err != nil { + t.Fatalf("mkdir elsewhere: %v", err) + } + // Move the real storage aside and replace the root with a link, the shape + // an attacker or a bad restore leaves behind. + if err := os.RemoveAll(plansRoot); err != nil { + t.Fatalf("remove plans root: %v", err) + } + if err := os.Symlink(elsewhere, plansRoot); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if _, _, err := ReadPlan(workspace, "session-1"); err == nil { + t.Fatal("expected ReadPlan to refuse a symlinked plan storage root") + } else if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected a symlink refusal from ReadPlan, got: %v", err) + } + + if _, err := WritePlan(workspace, "session-1", "1. [pending] redirected\n"); err == nil { + t.Fatal("expected WritePlan to refuse a symlinked plan storage root") + } else if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected a symlink refusal from WritePlan, got: %v", err) + } + + // Nothing may have been written through the link. + if entries, _ := os.ReadDir(elsewhere); len(entries) != 0 { + t.Fatalf("write escaped through the storage-root symlink into %s: %v", elsewhere, entries) + } +} + +// TestWritePlanRefusesIntermediateSymlink pins that WritePlan's handle-bound +// walk refuses an intermediate directory that is a symlink rather than +// following it with pathname MkdirAll/OpenFile/Rename. +func TestWritePlanRefusesIntermediateSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + // Creating directory symlinks requires elevated privileges on many + // Windows runners; skip rather than flake. + t.Skip("directory symlink creation is privileged on Windows CI") + } + cfg := isolatePlanStorage(t) + workspace := t.TempDir() + path, err := PlanFilePath(workspace, "session-1") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + // Create the plans root, then plant the workspace-key component as a + // symlink that points outside the storage tree. + plansRoot := filepath.Join(cfg, filepath.FromSlash(PlanDirName)) + if err := os.MkdirAll(plansRoot, 0o700); err != nil { + t.Fatalf("mkdir plans root: %v", err) + } + outside := filepath.Join(t.TempDir(), "outside") + if err := os.MkdirAll(outside, 0o700); err != nil { + t.Fatalf("mkdir outside: %v", err) + } + wsKeyDir := filepath.Dir(path) + if err := os.Symlink(outside, wsKeyDir); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + // WritePlan resolves the planted symlink in ensurePlanPathContained and + // refuses before writePlanFile's own handle-relative walk ever runs, so + // this only pins the outer containment check. + _, err = WritePlan(workspace, "session-1", "notes") + if err == nil { + t.Fatal("expected WritePlan to refuse intermediate symlink") + } + if !strings.Contains(err.Error(), "escapes plan storage root") { + t.Fatalf("expected the containment refusal, got: %v", err) + } + // Nothing should have been written through the link. + if entries, _ := os.ReadDir(outside); len(entries) != 0 { + t.Fatalf("write escaped through intermediate symlink into %s: %v", outside, entries) + } + + // Exercise the handle-relative writer directly, bypassing the containment + // pre-check above, so the no-follow walk's own symlink refusal is what + // this test actually pins. + if err := writePlanFile(plansRoot, path, "notes\n"); err == nil { + t.Fatal("expected writePlanFile to refuse the intermediate symlink") + } else if !strings.Contains(err.Error(), "is a symlink") { + t.Fatalf("expected symlink refusal from the handle-relative writer, got: %v", err) + } + if entries, _ := os.ReadDir(outside); len(entries) != 0 { + t.Fatalf("write escaped through intermediate symlink into %s: %v", outside, entries) + } +} + +func TestWritePlanRejectsStorageInsideWorkspace(t *testing.T) { + // If the user config root is pointed at the workspace, plan storage would + // become a silent workspace write. Refuse rather than undermine the + // read-only / no-write-grant contract. + workspace := t.TempDir() + // Point the temp root elsewhere so the temp-directory rule cannot mask a + // missing workspace check (t.TempDir() lives under the real os.TempDir()). + SetTempDirForTest(t, filepath.Join(t.TempDir(), "unrelated-temp")) + setUserConfigHomeEnv(t, workspace) + _, err := WritePlan(workspace, "session-1", "notes") + if err == nil { + t.Fatal("expected WritePlan to reject plan storage inside the workspace") + } + if !strings.Contains(err.Error(), "resolves into the workspace") { + t.Fatalf("expected the workspace containment error, got: %v", err) + } +} + +func TestPlanFilePathBlankIDDiffersFromLiteralPlan(t *testing.T) { + // pathKey must stay injective: the no-session fallback must not collide + // with a session whose ID is literally "plan". + isolatePlanStorage(t) + root := t.TempDir() + blank, err := PlanFilePath(root, "") + if err != nil { + t.Fatalf("PlanFilePath blank: %v", err) + } + named, err := PlanFilePath(root, "plan") + if err != nil { + t.Fatalf("PlanFilePath plan: %v", err) + } + if blank == named { + t.Fatalf("blank session ID and \"plan\" must not share a plan path: %q", blank) + } +} + +func TestStageForEditorRejectsStagingInsideWorkspace(t *testing.T) { + // StageForEditor must refuse when the staging directory itself resolves + // into the workspace, even with plan storage otherwise valid. + // + // Pointing XDG_CONFIG_HOME/AppData at the workspace does not isolate this: + // plan storage and the staging directory both derive from the same + // UserConfigDir, so that setup makes ReadPlan's own workspace-containment + // check fire first (same error as TestWritePlanRejectsStorageInsideWorkspace) + // and StageForEditor never reaches editorStagingDirIsPrivate at all. Keep + // storage isolated and valid, and instead swap the staging leaf itself for + // a symlink into the workspace, so only the staging-specific check fires. + if runtime.GOOS == "windows" { + t.Skip("directory symlink creation is privileged on Windows CI") + } + cfg := isolatePlanStorage(t) + workspace := t.TempDir() + if _, err := WritePlan(workspace, "session-1", "notes"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + insideWorkspace := filepath.Join(workspace, "staged") + if err := os.MkdirAll(insideWorkspace, 0o700); err != nil { + t.Fatalf("mkdir inside workspace: %v", err) + } + // Distinctive mode: the refusal must happen before any chmod, so the + // symlink target's permissions must survive unchanged. + if err := os.Chmod(insideWorkspace, 0o755); err != nil { + t.Fatalf("chmod inside workspace: %v", err) + } + stagingLink := filepath.Join(cfg, "zero", "plan-edit") + if err := os.MkdirAll(filepath.Dir(stagingLink), 0o700); err != nil { + t.Fatalf("mkdir staging parent: %v", err) + } + if err := os.Symlink(insideWorkspace, stagingLink); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + _, cleanup, err := StageForEditor(workspace, "session-1") + if cleanup != nil { + cleanup() + } + if err == nil { + t.Fatal("expected StageForEditor to reject staging inside the workspace") + } + if !strings.Contains(err.Error(), "sandbox-writable") { + t.Fatalf("expected the staging-privacy error, got: %v", err) + } + info, statErr := os.Stat(insideWorkspace) + if statErr != nil { + t.Fatalf("stat symlink target: %v", statErr) + } + if perm := info.Mode().Perm(); perm != 0o755 { + t.Fatalf("rejected staging must not chmod the symlink target, mode = %o", perm) + } +} + +func TestStageForEditorWritesUnderConfigStagingDir(t *testing.T) { + // Config and the privacy-check temp root must both be redirectable. Build + // them under t.TempDir() and point SetTempDirForTest at a sibling so + // StageForEditor's effectiveTempDir() seam (and WritePlan containment) + // agree without planting a config root beside the real OS temp dir + // (which fails with permission denied on Linux CI and on Windows drive roots). + configDir := isolatePlanStorage(t) + + workspace := t.TempDir() + if _, err := WritePlan(workspace, "session-1", "1. [pending] draft step\n"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + staged, cleanup, err := StageForEditor(workspace, "session-1") + if err != nil { + t.Fatalf("StageForEditor: %v", err) + } + t.Cleanup(cleanup) + wantRoot := filepath.Join(configDir, "zero", "plan-edit") + physStaged := staged + if resolved, err := filepath.EvalSymlinks(staged); err == nil { + physStaged = resolved + } + physWant, err := filepath.EvalSymlinks(wantRoot) + if err != nil { + physWant = wantRoot + } + rel, err := filepath.Rel(physWant, physStaged) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("staged path %q not under config staging dir %q", staged, wantRoot) + } + data, err := os.ReadFile(staged) + if err != nil { + t.Fatalf("read staged: %v", err) + } + if !strings.Contains(string(data), "draft step") { + t.Fatalf("staged content missing plan body: %q", data) + } +} + +// Regression: tea.ExecProcess's cleanup callback only runs if the caller's +// Bubble Tea program lives long enough to invoke it, so an abrupt shutdown +// while the editor is open leaks the staged file (see the sweep call in +// StageForEditor). The next StageForEditor call must reclaim it. +func TestStageForEditorSweepsAbandonedStagedFiles(t *testing.T) { + isolatePlanStorage(t) + workspace := t.TempDir() + if _, err := WritePlan(workspace, "session-1", "1. [pending] draft step\n"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + // Simulate a staged file abandoned by a prior dead process: create an old + // staged file and companion lockfile under the config staging directory. + stagingDir, err := editorStagingDir() + if err != nil { + t.Fatalf("editorStagingDir: %v", err) + } + if err := os.MkdirAll(stagingDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + abandoned := filepath.Join(stagingDir, "session_1-1234-5678.md") + if err := os.WriteFile(abandoned, []byte("old draft\n"), 0o600); err != nil { + t.Fatalf("WriteFile abandoned: %v", err) + } + _ = os.WriteFile(abandoned+".lock", nil, 0o600) + + old := time.Now().Add(-staleStagedEditThreshold - time.Hour) + if err := os.Chtimes(abandoned, old, old); err != nil { + t.Fatalf("Chtimes: %v", err) + } + + fresh, cleanup, err := StageForEditor(workspace, "session-1") + if err != nil { + t.Fatalf("StageForEditor (fresh): %v", err) + } + t.Cleanup(cleanup) + + if _, err := os.Stat(abandoned); !os.IsNotExist(err) { + t.Fatalf("expected abandoned staged file to be swept, stat err = %v", err) + } + if _, err := os.Stat(fresh); err != nil { + t.Fatalf("expected fresh staged file to survive the sweep: %v", err) + } +} + +func TestCommitStagedEditWritesBackEditedPlan(t *testing.T) { + isolatePlanStorage(t) + workspace := t.TempDir() + if _, err := WritePlan(workspace, "session-1", "1. [pending] draft step\n"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + staged, cleanup, err := StageForEditor(workspace, "session-1") + if err != nil { + t.Fatalf("StageForEditor: %v", err) + } + t.Cleanup(cleanup) + if err := os.WriteFile(staged, []byte("1. [completed] edited step\n\n"), 0o600); err != nil { + t.Fatalf("rewrite staged plan: %v", err) + } + if err := CommitStagedEdit(workspace, "session-1", staged); err != nil { + t.Fatalf("CommitStagedEdit: %v", err) + } + content, exists, err := ReadPlan(workspace, "session-1") + if err != nil { + t.Fatalf("ReadPlan: %v", err) + } + if !exists || content != "1. [completed] edited step\n" { + t.Fatalf("ReadPlan = (%q, %t), want edited normalized plan", content, exists) + } + if err := CommitStagedEdit(workspace, "session-1", filepath.Join(t.TempDir(), "missing")); err == nil { + t.Fatal("expected CommitStagedEdit to reject a missing staged path") + } +} + +func TestEditorStagingDirIsPrivateRejectsOSTempDir(t *testing.T) { + workspaceRoot := t.TempDir() + // t.TempDir() itself lives under os.TempDir(), so it doubles as a stand-in + // for what config.UserConfigDir() would resolve to if XDG_CONFIG_HOME were + // pointed at the OS temp directory. + dir := t.TempDir() + if editorStagingDirIsPrivate(dir, workspaceRoot, os.TempDir()) { + t.Fatalf("expected %q (under the OS temp dir) to be rejected", dir) + } +} + +func TestEditorStagingDirIsPrivateRejectsWorkspaceDir(t *testing.T) { + workspaceRoot := t.TempDir() + dir := filepath.Join(workspaceRoot, ".config", "zero", "plan-edit") + if editorStagingDirIsPrivate(dir, workspaceRoot, os.TempDir()) { + t.Fatalf("expected %q (inside the workspace) to be rejected", dir) + } + // The workspace root itself, not just a descendant, must also be rejected. + if editorStagingDirIsPrivate(workspaceRoot, workspaceRoot, os.TempDir()) { + t.Fatal("expected the workspace root itself to be rejected") + } +} + +func TestEditorStagingDirIsPrivateAcceptsElsewhere(t *testing.T) { + // workspaceRoot (via t.TempDir()) and a naive "sibling of workspaceRoot" + // both live under os.TempDir(), so the stand-in for a real XDG config + // directory has to be built as a sibling of the OS temp dir itself, + // not of the workspace, to land genuinely outside both. + workspaceRoot := t.TempDir() + tempDir := filepath.Clean(os.TempDir()) + dir := filepath.Join(filepath.Dir(tempDir), "not-temp-not-workspace", "zero", "plan-edit") + if !editorStagingDirIsPrivate(dir, workspaceRoot, os.TempDir()) { + t.Fatalf("expected %q to be accepted as private", dir) + } +} + +func TestEditorStagingDirIsPrivateResolvesSymlinkedDir(t *testing.T) { + // An XDG config path that is lexically outside both roots but is a + // symlink INTO the workspace (or temp) must be rejected: MkdirAll and + // CreateTemp follow the link, so judging the spelled path would stage + // the file somewhere sandbox-writable. The fake temp root keeps the + // scenario constructible portably (everything a test may create lives + // under the real temp dir, which would otherwise mask the workspace case). + base := t.TempDir() + fakeTemp := filepath.Join(base, "faketemp") + workspaceRoot := filepath.Join(base, "workspace") + target := filepath.Join(workspaceRoot, "hidden-staging") + if err := os.MkdirAll(fakeTemp, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(base, "looks-private") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if editorStagingDirIsPrivate(link, workspaceRoot, fakeTemp) { + t.Fatal("expected a staging dir symlinked into the workspace to be rejected") + } + + // Same for a link into the temp root. + tempTarget := filepath.Join(fakeTemp, "hidden-staging") + if err := os.MkdirAll(tempTarget, 0o700); err != nil { + t.Fatal(err) + } + tempLink := filepath.Join(base, "looks-private-too") + if err := os.Symlink(tempTarget, tempLink); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if editorStagingDirIsPrivate(tempLink, workspaceRoot, fakeTemp) { + t.Fatal("expected a staging dir symlinked into the temp root to be rejected") + } +} + +func TestEditorStagingDirIsPrivateResolvesSymlinkedRoots(t *testing.T) { + // The inverse direction: the WORKSPACE itself is reached through a + // symlink, so a staging dir spelled via the physical workspace path does + // not lexically sit under the symlinked spelling. Physical comparison + // must still reject it. + base := t.TempDir() + fakeTemp := filepath.Join(base, "faketemp") + realWorkspace := filepath.Join(base, "real-workspace") + if err := os.MkdirAll(fakeTemp, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(realWorkspace, "cfg"), 0o700); err != nil { + t.Fatal(err) + } + workspaceLink := filepath.Join(base, "workspace-link") + if err := os.Symlink(realWorkspace, workspaceLink); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if editorStagingDirIsPrivate(filepath.Join(realWorkspace, "cfg"), workspaceLink, fakeTemp) { + t.Fatal("expected a staging dir inside the physical workspace to be rejected when the workspace is addressed through a symlink") + } +} + +func TestStageContentForEditorRoundTrip(t *testing.T) { + dir := t.TempDir() + path, cleanup, err := stageContentForEditor(dir, "session-1", "# Draft\n\nStep one.") + if err != nil { + t.Fatalf("stageContentForEditor: %v", err) + } + defer cleanup() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read staged file: %v", err) + } + if string(data) != "# Draft\n\nStep one.\n" { + t.Fatalf("staged content = %q", string(data)) + } + + cleanup() + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("expected cleanup to remove the staged file, stat err=%v", err) + } +} + +func TestStageContentForEditorGeneratesUniquePathsPerCall(t *testing.T) { + // Two concurrent invocations for the same session (e.g. two Zero + // instances editing a resumed session) must not collide on one shared + // deterministic path. + dir := t.TempDir() + pathA, cleanupA, err := stageContentForEditor(dir, "session-1", "draft A") + if err != nil { + t.Fatalf("stageContentForEditor (A): %v", err) + } + defer cleanupA() + pathB, cleanupB, err := stageContentForEditor(dir, "session-1", "draft B") + if err != nil { + t.Fatalf("stageContentForEditor (B): %v", err) + } + defer cleanupB() + + if pathA == pathB { + t.Fatalf("expected distinct staged paths, both were %q", pathA) + } + dataA, err := os.ReadFile(pathA) + if err != nil { + t.Fatalf("read A: %v", err) + } + dataB, err := os.ReadFile(pathB) + if err != nil { + t.Fatalf("read B: %v", err) + } + if string(dataA) != "draft A\n" || string(dataB) != "draft B\n" { + t.Fatalf("cross-contaminated staged files: A=%q B=%q", dataA, dataB) + } + + // cleanupA must not touch B's file, and vice versa. + cleanupA() + if _, err := os.Stat(pathB); err != nil { + t.Fatalf("cleanupA should not have removed B's staged file: %v", err) + } +} + +func TestStageContentForEditorTightensPreExistingLoosePermissions(t *testing.T) { + // Regression: MkdirAll(0700) does not change an existing group/world- + // writable plan-edit directory. stageContentForEditor must chmod before + // CreateTemp so a closed staged file is not writable by another local + // user before $EDITOR reopens it. + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + dir := t.TempDir() + if err := os.Chmod(dir, 0o777); err != nil { + t.Fatalf("chmod loose staging dir: %v", err) + } + path, cleanup, err := stageContentForEditor(dir, "session-1", "draft") + if err != nil { + t.Fatalf("stageContentForEditor: %v", err) + } + defer cleanup() + + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat staging dir: %v", err) + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + t.Fatalf("expected staging dir tightened away from group/world write, got %o", perm) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("staged file missing: %v", err) + } +} + +func TestVerifyPrivateDirectoryRejectsGroupWritable(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + dir := t.TempDir() + if err := os.Chmod(dir, 0o770); err != nil { + t.Fatalf("chmod: %v", err) + } + if err := verifyPrivateDirectory(dir); err == nil { + t.Fatal("expected verifyPrivateDirectory to reject a group-writable directory") + } +} + +func TestVerifyPrivateDirectoryAcceptsOwnerOnly(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + dir := t.TempDir() + if err := os.Chmod(dir, 0o700); err != nil { + t.Fatalf("chmod: %v", err) + } + if err := verifyPrivateDirectory(dir); err != nil { + t.Fatalf("verifyPrivateDirectory: %v", err) + } +} + +func TestStageForEditorCommitStagedEditReadPlanRoundTrip(t *testing.T) { + // End-to-end test covering the full editor round-trip: + // 1. Write a plan to durable storage + // 2. Stage it for editor (copies to private staging dir) + // 3. Rewrite the staged file (simulate user editing in $EDITOR) + // 4. Commit the staged edit back to durable storage + // 5. Read the plan back and verify it matches the edited content + isolatePlanStorage(t) + + workspace := t.TempDir() + sessionID := "e2e-roundtrip-session" + + // Step 1: Write initial plan + initial := "1. [pending] original step\n" + if _, err := WritePlan(workspace, sessionID, initial); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + // Step 2: Stage for editor + stagedPath, cleanup, err := StageForEditor(workspace, sessionID) + if err != nil { + t.Fatalf("StageForEditor: %v", err) + } + defer cleanup() + + // Step 3: Simulate user editing the staged file + editedContent := "1. [completed] edited step one\n2. [in_progress] edited step two\n Notes: from editor\n" + if err := os.WriteFile(stagedPath, []byte(editedContent), 0o600); err != nil { + t.Fatalf("rewrite staged file: %v", err) + } + + // Step 4: Commit staged edit back to durable storage + if err := CommitStagedEdit(workspace, sessionID, stagedPath); err != nil { + t.Fatalf("CommitStagedEdit: %v", err) + } + + // Step 5: Read plan back and verify it matches edited content (normalized with trailing newline) + content, ok, err := ReadPlan(workspace, sessionID) + if err != nil { + t.Fatalf("ReadPlan: %v", err) + } + if !ok { + t.Fatal("expected plan file to exist after commit") + } + // WritePlan normalizes to single trailing newline + expected := strings.TrimRight(editedContent, "\n") + "\n" + if content != expected { + t.Fatalf("round-trip content mismatch:\ngot: %q\nexpected: %q", content, expected) + } +} + +func TestCommitStagedEditReturnsErrorForMissingStagedFile(t *testing.T) { + // Cover the write-back failure path: CommitStagedEdit must return an error + // when the staged pathname does not exist. + isolatePlanStorage(t) + workspace := t.TempDir() + sessionID := "missing-staged" + + // Point to a non-existent staged file path (under a temp dir we control) + stagedPath := filepath.Join(t.TempDir(), "does-not-exist.md") + err := CommitStagedEdit(workspace, sessionID, stagedPath) + if err == nil { + t.Fatal("expected CommitStagedEdit to error for missing staged file") + } + if !strings.Contains(err.Error(), "read staged plan file") { + t.Fatalf("expected read error context, got: %v", err) + } +} + +// TestSweepStaleStagedFilesSkipsLockedAndUnrelatedFiles is the regression for P2: +// sweepStaleStagedFiles must not delete active staged files held by an editor, +// and must never delete unrelated non-plan files in the staging directory. +func TestSweepStaleStagedFilesSkipsLockedAndUnrelatedFiles(t *testing.T) { + isolatePlanStorage(t) + dir := t.TempDir() + + // 1. Unrelated non-plan file with old mtime must NOT be deleted + unrelatedFile := filepath.Join(dir, "notes.txt") + if err := os.WriteFile(unrelatedFile, []byte("important note"), 0o600); err != nil { + t.Fatalf("write unrelated: %v", err) + } + oldTime := time.Now().Add(-10 * time.Hour) + _ = os.Chtimes(unrelatedFile, oldTime, oldTime) + + // 2. Staged file with active lock (open editor) must NOT be deleted even if old + stagedPath, cleanup, err := stageContentForEditor(dir, "session-locked", "draft") + if err != nil { + t.Fatalf("stageContentForEditor: %v", err) + } + defer cleanup() + _ = os.Chtimes(stagedPath, oldTime, oldTime) + + // 3. Staged file with old mtime whose lock is released (abandoned editor) SHOULD be deleted + abandonedPath, abandonedCleanup, err := stageContentForEditor(dir, "session-abandoned", "old draft") + if err != nil { + t.Fatalf("stageContentForEditor: %v", err) + } + // Simulate editor crash/close by releasing lock but leaving file + abandonedCleanup() + _ = os.WriteFile(abandonedPath, []byte("abandoned content"), 0o600) + _ = os.Chtimes(abandonedPath, oldTime, oldTime) + + // Run sweep + sweepStaleStagedFiles(dir) + + // Verify unrelated file survived + if _, err := os.Stat(unrelatedFile); err != nil { + t.Fatalf("unrelated file was deleted by sweep: %v", err) + } + + // Verify locked staged file survived + if _, err := os.Stat(stagedPath); err != nil { + t.Fatalf("active locked staged file was deleted by sweep: %v", err) + } + + // Verify abandoned file was cleaned up + if _, err := os.Stat(abandonedPath); !os.IsNotExist(err) { + t.Fatalf("abandoned file was not cleaned up by sweep, stat err: %v", err) + } +} diff --git a/internal/planmode/planmode_windows_test.go b/internal/planmode/planmode_windows_test.go new file mode 100644 index 000000000..4a08206cc --- /dev/null +++ b/internal/planmode/planmode_windows_test.go @@ -0,0 +1,180 @@ +//go:build windows + +package planmode + +import ( + "os" + "path/filepath" + "testing" +) + +// TestEditorStagingDirIsPrivateRejectsJunctionIntoWorkspace is the Windows +// counterpart of TestEditorStagingDirIsPrivateResolvesSymlinkedDir. That test +// skips here whenever directory-symlink creation is privileged, which left the +// staging containment check with no Windows coverage at all — and it was inert +// on exactly this platform, because filepath.EvalSymlinks hands a junction +// back unresolved while MkdirAll and CreateTemp follow it. A junction needs no +// privilege to create, so it is the reparse point that actually matters here. +func TestEditorStagingDirIsPrivateRejectsJunctionIntoWorkspace(t *testing.T) { + base := t.TempDir() + fakeTemp := filepath.Join(base, "faketemp") + workspaceRoot := filepath.Join(base, "workspace") + target := filepath.Join(workspaceRoot, "hidden-staging") + if err := os.MkdirAll(fakeTemp, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + + link := filepath.Join(base, "looks-private") + createWindowsDirReparse(t, link, target) + if editorStagingDirIsPrivate(link, workspaceRoot, fakeTemp) { + t.Error("a staging dir junctioned into the workspace was accepted as private") + } + + tempTarget := filepath.Join(fakeTemp, "hidden-staging") + if err := os.MkdirAll(tempTarget, 0o700); err != nil { + t.Fatal(err) + } + tempLink := filepath.Join(base, "looks-private-too") + createWindowsDirReparse(t, tempLink, tempTarget) + if editorStagingDirIsPrivate(tempLink, workspaceRoot, fakeTemp) { + t.Error("a staging dir junctioned into the temp root was accepted as private") + } +} + +// TestEditorStagingDirIsPrivateResolvesJunctionedRoots is the inverse +// direction: the workspace is reached through a junction, so a staging dir +// spelled with the physical workspace path does not lexically sit under the +// junctioned spelling. Physical comparison must still reject it. +func TestEditorStagingDirIsPrivateResolvesJunctionedRoots(t *testing.T) { + base := t.TempDir() + fakeTemp := filepath.Join(base, "faketemp") + realWorkspace := filepath.Join(base, "real-workspace") + if err := os.MkdirAll(fakeTemp, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(realWorkspace, "cfg"), 0o700); err != nil { + t.Fatal(err) + } + workspaceLink := filepath.Join(base, "workspace-link") + createWindowsDirReparse(t, workspaceLink, realWorkspace) + + if editorStagingDirIsPrivate(filepath.Join(realWorkspace, "cfg"), workspaceLink, fakeTemp) { + t.Error("a staging dir inside the physical workspace was accepted when the workspace is addressed through a junction") + } +} + +// TestResolvePhysicalTraversesJunction pins the primitive the containment check +// rests on, so a future change back to filepath.EvalSymlinks fails here with a +// clear cause rather than only as a containment miss two layers up. +func TestResolvePhysicalTraversesJunction(t *testing.T) { + base := t.TempDir() + target := filepath.Join(base, "target") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(base, "link") + createWindowsDirReparse(t, link, target) + + resolved, err := resolvePhysical(link) + if err != nil { + t.Fatalf("resolvePhysical(junction): %v", err) + } + wantPhysical, err := resolvePhysical(target) + if err != nil { + t.Fatalf("resolvePhysical(target): %v", err) + } + if resolved != wantPhysical { + t.Errorf("resolvePhysical(junction) = %q, want the junction target %q", resolved, wantPhysical) + } + if resolved == filepath.Clean(link) { + t.Error("resolvePhysical returned the junction itself, so the reparse point was not traversed") + } +} + +// TestVerifyPrivateDirectoryRejectsJunction covers the backstop. os.Lstat maps +// a junction to os.ModeIrregular, so the os.ModeSymlink test cannot see one and +// verifyPrivateDirectory used to accept it. +func TestVerifyPrivateDirectoryRejectsJunction(t *testing.T) { + base := t.TempDir() + target := filepath.Join(base, "target") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(base, "link") + createWindowsDirReparse(t, link, target) + + if err := verifyPrivateDirectory(link); err == nil { + t.Error("verifyPrivateDirectory accepted a junction") + } + if err := verifyPrivateDirectory(target); err != nil { + t.Errorf("verifyPrivateDirectory(plain directory) = %v, want nil", err) + } +} + +// TestPlanFlowRefusesJunctionedConfigRootIntoWorkspace pins the end-to-end +// outcome: a junction at %AppData% puts both the plan storage root and the +// editor staging directory physically inside the workspace while every +// component of their spelling looks ordinary. +// +// This one passes before the fix as well, and that is worth recording rather +// than hiding. Two other gates already refuse this route: the no-follow +// storage walk (OBJ_DONT_REPARSE) rejects a junctioned plans root, and +// verifyPrivateDirectory rejects a junctioned staging directory through its +// !IsDir test, because os.Lstat maps a junction to os.ModeIrregular. So the +// inert containment check was a boundary that did not hold, not a reachable +// path to a staged file. The test guards the outcome against a future change +// to either of those gates. +func TestPlanFlowRefusesJunctionedConfigRootIntoWorkspace(t *testing.T) { + base := t.TempDir() + // The scenario is about the workspace root, so point the temp check at an + // unrelated directory: base itself lives under the real temp dir, which + // would otherwise reject the paths for the wrong reason. + fakeTemp := filepath.Join(base, "faketemp") + workspace := filepath.Join(base, "workspace") + insideWorkspace := filepath.Join(workspace, "sandbox-writable") + for _, dir := range []string{fakeTemp, insideWorkspace} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + SetTempDirForTest(t, fakeTemp) + + appData := filepath.Join(base, "appdata") + createWindowsDirReparse(t, appData, insideWorkspace) + t.Setenv("AppData", appData) + + const sessionID = "sess-junction" + refused := false + if _, err := WritePlan(workspace, sessionID, "# plan\n"); err != nil { + refused = true + } + if !refused { + staged, cleanup, err := StageForEditor(workspace, sessionID) + if cleanup != nil { + cleanup() + } + if err != nil { + refused = true + } else if physical, perr := resolvePhysical(staged); perr == nil && !isUnderOrEqual(physical, physicalPath(workspace)) { + // Staged outside the workspace after all: not the failure case. + refused = true + } + } + if !refused { + t.Fatal("a junctioned config root let the plan flow write and stage inside the workspace") + } + + var strays []string + _ = filepath.WalkDir(insideWorkspace, func(path string, d os.DirEntry, err error) error { + if err == nil && !d.IsDir() { + strays = append(strays, path) + } + return nil + }) + if len(strays) != 0 { + t.Errorf("plan content landed inside the workspace: %v", strays) + } +} diff --git a/internal/planmode/read.go b/internal/planmode/read.go new file mode 100644 index 000000000..f561dba02 --- /dev/null +++ b/internal/planmode/read.go @@ -0,0 +1,112 @@ +package planmode + +import ( + "errors" + "fmt" + "io" + "path/filepath" + "strings" +) + +// readPlanFile reads path by walking components under the plan storage base +// with a true no-follow open on every component (openat(O_NOFOLLOW) on Unix, +// NtCreateFile with OBJ_DONT_REPARSE on Windows). Intermediate directories and +// the final name are opened relative to the previous handle, so a concurrent +// symlink or reparse-point swap cannot redirect the read outside the storage +// tree and cannot replace a regular plan file with an in-root symlink between +// a pre-open Lstat and Open. +// +// A symlink final component is refused even when its target would stay inside +// the base: durable plan files are plain files, and reading through a link +// would re-introduce a replace-with-symlink race against the intended path. +// os.Root.Open is intentionally not used: it follows in-root symlinks after +// O_NOFOLLOW fails (checkSymlink), which is exactly the race we refuse. +func readPlanFile(base, path string) ([]byte, error) { + rel, err := relWithinBase(base, path) + if err != nil { + return nil, err + } + file, err := openPlanUnderBase(base, rel, path) + if err != nil { + return nil, err + } + defer file.Close() + return io.ReadAll(file) +} + +var errPlanSymlinkRefusal = errors.New("is a symlink") + +// errPlanSymlink is the stable refusal message for final and intermediate +// symlink / reparse-point components. ReadPlan matches on "is a symlink". +func errPlanSymlink(path string) error { + return fmt.Errorf("plan file %s %w; refusing to read through it", path, errPlanSymlinkRefusal) +} + +// errPlanBaseSymlink refuses a symlink or reparse point at the storage root +// itself, the directory the no-follow walk is rooted in. ensurePlanPathContained +// resolves the base and the plan path through the same links, so a link there +// passes containment (it only fails when the target is the workspace or the +// temp directory). Opening through it would anchor the whole walk inside the +// link's target, so every later handle-relative read, create, and rename would +// land there. Shared by the read and write walkers. +func errPlanBaseSymlink(base string) error { + return fmt.Errorf("plan storage root %s %w; refusing to open through it", base, errPlanSymlinkRefusal) +} + +// relWithinBase returns path relative to base after both are cleaned to +// absolute form, rejecting any lexical escape. The relative name is what the +// no-follow walk opens; absolute pathname open is intentionally not used. +func relWithinBase(base, path string) (string, error) { + absBase, err := filepath.Abs(base) + if err != nil { + return "", err + } + absPath, err := filepath.Abs(path) + if err != nil { + return "", err + } + rel, err := filepath.Rel(absBase, absPath) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("plan path %s escapes plan storage root %s", path, base) + } + if rel == "." { + return "", fmt.Errorf("plan path %s is the storage root, not a file", path) + } + return rel, nil +} + +// relComponents splits a storage-relative path into single-component names for +// a handle-relative openat/NtCreateFile walk. ".." and absolute forms are +// rejected even though relWithinBase already filters them, so the walker stays +// closed under a malicious or miscomputed relative name. +func relComponents(rel string) ([]string, error) { + rel = filepath.Clean(rel) + if rel == "." { + return nil, fmt.Errorf("plan path is the storage root, not a file") + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("plan path escapes plan storage root") + } + if filepath.IsAbs(rel) { + return nil, fmt.Errorf("plan path must be relative to the storage root") + } + slash := filepath.ToSlash(rel) + raw := strings.Split(slash, "/") + parts := make([]string, 0, len(raw)) + for _, p := range raw { + if p == "" || p == "." { + continue + } + if p == ".." { + return nil, fmt.Errorf("plan path escapes plan storage root") + } + parts = append(parts, p) + } + if len(parts) == 0 { + return nil, fmt.Errorf("plan path is the storage root, not a file") + } + return parts, nil +} diff --git a/internal/planmode/read_other.go b/internal/planmode/read_other.go new file mode 100644 index 000000000..e8f73a12f --- /dev/null +++ b/internal/planmode/read_other.go @@ -0,0 +1,19 @@ +//go:build !unix && !windows + +package planmode + +import ( + "fmt" + "os" +) + +// openPlanUnderBase fails closed on platforms without openat / OBJ_DONT_REPARSE +// primitives. A validate-then-open sequence (Lstat then Open) leaves a +// time-of-check to time-of-use gap: a validated regular file can be replaced +// with an in-root symlink before Open runs. Zero's supported targets are Unix +// and Windows, which use the true no-follow walkers in read_unix.go and +// read_windows.go; this fallback refuses instead of returning a file opened +// through a race it cannot close. +func openPlanUnderBase(_, _, displayPath string) (*os.File, error) { + return nil, fmt.Errorf("plan file %s: reading plan files is not supported on this platform", displayPath) +} diff --git a/internal/planmode/read_unix.go b/internal/planmode/read_unix.go new file mode 100644 index 000000000..e6229270e --- /dev/null +++ b/internal/planmode/read_unix.go @@ -0,0 +1,124 @@ +//go:build unix + +package planmode + +import ( + "errors" + "fmt" + "os" + "syscall" + + "golang.org/x/sys/unix" +) + +// openPlanUnderBase opens rel under base with a true no-follow walk: +// openat(O_NOFOLLOW|O_DIRECTORY) for every intermediate component and +// openat(O_NOFOLLOW|O_RDONLY) for the final name. Unlike os.Root.Open, a +// final-component O_NOFOLLOW failure is mapped to a hard refusal rather than +// followed via checkSymlink when the target remains inside the base. +func openPlanUnderBase(base, rel, displayPath string) (*os.File, error) { + parts, err := relComponents(rel) + if err != nil { + return nil, err + } + + // O_NOFOLLOW on the base as well as on every component under it: see + // errPlanBaseSymlink for why a link here defeats the whole walk. It applies + // to the final component only, so a legitimately symlinked ~/.config above + // the storage root is still fine. + dirfd, err := openatRetry(unix.AT_FDCWD, base, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + if isNoFollowErr(err) || isSymlinkDisguisedAsENOTDIR(unix.AT_FDCWD, base, err) { + return nil, errPlanBaseSymlink(base) + } + return nil, err + } + // Own dirfd until the final file is successfully handed to os.NewFile. + // Intermediate replacements close the previous fd. + defer func() { + if dirfd >= 0 { + _ = unix.Close(dirfd) + } + }() + + for i := 0; i < len(parts)-1; i++ { + next, err := openatRetry(dirfd, parts[i], unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + if isNoFollowErr(err) || isSymlinkDisguisedAsENOTDIR(dirfd, parts[i], err) { + return nil, errPlanSymlink(displayPath) + } + return nil, err + } + _ = unix.Close(dirfd) + dirfd = next + } + + final := parts[len(parts)-1] + // O_NONBLOCK so a planted FIFO cannot hang the open; the regular-file + // check below still rejects non-regular targets after open succeeds. + fd, err := openatRetry(dirfd, final, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC|unix.O_NONBLOCK, 0) + if err != nil { + if isNoFollowErr(err) { + return nil, errPlanSymlink(displayPath) + } + return nil, err + } + + var st unix.Stat_t + if err := unix.Fstat(fd, &st); err != nil { + _ = unix.Close(fd) + return nil, err + } + if st.Mode&unix.S_IFMT == unix.S_IFLNK { + _ = unix.Close(fd) + return nil, errPlanSymlink(displayPath) + } + if st.Mode&unix.S_IFMT != unix.S_IFREG { + _ = unix.Close(fd) + return nil, fmt.Errorf("plan file %s is not a regular file", displayPath) + } + + // Transfer ownership of fd to *os.File; prevent deferred Close of dirfd + // from touching it. dirfd is still closed by the deferred cleanup. + f := os.NewFile(uintptr(fd), displayPath) + if f == nil { + _ = unix.Close(fd) + return nil, fmt.Errorf("plan file %s: invalid file descriptor", displayPath) + } + return f, nil +} + +func openatRetry(dirfd int, path string, flags int, mode uint32) (int, error) { + for { + fd, err := unix.Openat(dirfd, path, flags, mode) + if errors.Is(err, syscall.EINTR) { + continue + } + return fd, err + } +} + +// isNoFollowErr reports whether err is the platform-specific errno returned +// when openat(..., O_NOFOLLOW) hits a symlink (ELOOP on most Unix, EMLINK on +// FreeBSD/Dragonfly). +func isNoFollowErr(err error) bool { + return errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EMLINK) +} + +// isSymlinkDisguisedAsENOTDIR reports whether err is the ENOTDIR that +// openat(..., O_DIRECTORY|O_NOFOLLOW) returns on Linux and Darwin when name +// is actually a symlink: the kernel never dereferences the symlink to see +// the O_DIRECTORY mismatch it would otherwise report as ELOOP/EMLINK (what +// isNoFollowErr checks). A genuine non-symlink, non-directory component (a +// plain file blocking the path) also returns ENOTDIR, so this disambiguates +// with a no-follow stat instead of trusting the errno alone. +func isSymlinkDisguisedAsENOTDIR(dirfd int, name string, err error) bool { + if !errors.Is(err, syscall.ENOTDIR) { + return false + } + var st unix.Stat_t + if statErr := unix.Fstatat(dirfd, name, &st, unix.AT_SYMLINK_NOFOLLOW); statErr != nil { + return false + } + return st.Mode&unix.S_IFMT == unix.S_IFLNK +} diff --git a/internal/planmode/read_windows.go b/internal/planmode/read_windows.go new file mode 100644 index 000000000..844c8c674 --- /dev/null +++ b/internal/planmode/read_windows.go @@ -0,0 +1,251 @@ +//go:build windows + +package planmode + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "unsafe" + + "golang.org/x/sys/windows" +) + +// openPlanUnderBase opens rel under base with a true no-follow walk using +// NtCreateFile and OBJ_DONT_REPARSE on every component (the same primitive +// behind Go's os.Root / O_NOFOLLOW_ANY). A reparse point at any component is +// refused rather than followed, including in-root final-component swaps that +// os.Root.Open would otherwise accept via checkSymlink. +func openPlanUnderBase(base, rel, displayPath string) (*os.File, error) { + parts, err := relComponents(rel) + if err != nil { + return nil, err + } + + absBase, err := filepath.Abs(base) + if err != nil { + return nil, err + } + + // parent is the current directory handle in the walk. On success the final + // file handle is transferred to *os.File; parent stays owned here and is + // closed by the deferred cleanup. The closure must re-read parent so + // intermediate reassignment is not leaked (defer args are evaluated now). + parent, err := openWindowsBaseDir(absBase) + if err != nil { + return nil, err + } + defer func() { _ = windows.CloseHandle(parent) }() + + for i := 0; i < len(parts)-1; i++ { + next, err := openatNoFollow(parent, parts[i], true) + if err != nil { + if isWindowsSymlinkErr(err) { + return nil, errPlanSymlink(displayPath) + } + return nil, err + } + _ = windows.CloseHandle(parent) + parent = next + } + + final := parts[len(parts)-1] + h, err := openatNoFollow(parent, final, false) + if err != nil { + if isWindowsSymlinkErr(err) { + return nil, errPlanSymlink(displayPath) + } + // FILE_NON_DIRECTORY_FILE fails with EISDIR when the final name is a + // directory; map it to the same regular-file refusal the attribute + // check below uses so callers see one stable message. + if err == syscall.EISDIR { + return nil, fmt.Errorf("plan file %s is not a regular file", displayPath) + } + return nil, err + } + + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(h, &info); err != nil { + _ = windows.CloseHandle(h) + return nil, err + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + _ = windows.CloseHandle(h) + return nil, errPlanSymlink(displayPath) + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 { + _ = windows.CloseHandle(h) + return nil, fmt.Errorf("plan file %s is not a regular file", displayPath) + } + + f := os.NewFile(uintptr(h), displayPath) + if f == nil { + _ = windows.CloseHandle(h) + return nil, fmt.Errorf("plan file %s: invalid file handle", displayPath) + } + return f, nil +} + +// ntObjectPath builds the NT object manager path for an absolute Win32 path. +// Drive-letter paths become `\??\C:\...`. UNC paths (`\\server\share\...`) +// must go through the UNC device: `\??\UNC\server\share\...`. Concatenating +// `\??\` alone yields `\??\\\server\...`, which NtCreateFile rejects. A +// roaming %AppData% (plan storage base) can legitimately be a UNC path. +// +// The extended-length (`\\?\C:\...`) and device (`\\.\...`) prefixes also +// begin with two backslashes but are not UNC. `\\?\` is stripped because +// `\??\` is its NT equivalent; `\\?\UNC\` is already UNC-qualified. +func ntObjectPath(absPath string) string { + if rest, ok := strings.CutPrefix(absPath, `\\?\`); ok { + // `\\?\UNC\server\share` -> `\??\UNC\server\share`. + return `\??\` + rest + } + if rest, ok := strings.CutPrefix(absPath, `\\.\`); ok { + return `\??\` + rest + } + if rest, ok := strings.CutPrefix(absPath, `\\`); ok { + return `\??\UNC\` + rest + } + return `\??\` + absPath +} + +// openWindowsBaseDir opens the storage base as a directory handle that can be +// used as RootDirectory for subsequent relative NtCreateFile calls. +func openWindowsBaseDir(absBase string) (windows.Handle, error) { + path := ntObjectPath(absBase) + objName, err := windows.NewNTUnicodeString(path) + if err != nil { + return 0, err + } + oa := &windows.OBJECT_ATTRIBUTES{ + ObjectName: objName, + // OBJ_DONT_REPARSE on the base too, not just on the components walked + // under it. ensurePlanPathContained resolves the base and the plan path + // through the same links, so a reparse point at the plans root passes + // containment (it only fails when the target is the workspace or temp + // directory). Following it here would root the whole no-follow walk in + // the target directory, which is precisely the redirection the walk + // exists to prevent. + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + oa.Length = uint32(unsafe.Sizeof(*oa)) + + var h windows.Handle + var iosb windows.IO_STATUS_BLOCK + err = windows.NtCreateFile( + &h, + windows.FILE_GENERIC_READ|windows.FILE_TRAVERSE|windows.SYNCHRONIZE, + oa, + &iosb, + nil, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windows.FILE_OPEN, + windows.FILE_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_FOR_BACKUP_INTENT, + 0, + 0, + ) + if err != nil { + mapped := mapWindowsOpenErr(err) + if isWindowsSymlinkErr(mapped) { + return 0, errPlanBaseSymlink(absBase) + } + return 0, mapped + } + return h, nil +} + +// openatNoFollow opens name relative to dirfd without following reparse points. +// When directory is true the target must be a directory; otherwise it must not +// be a directory. +func openatNoFollow(dirfd windows.Handle, name string, directory bool) (windows.Handle, error) { + objName, err := windows.NewNTUnicodeString(name) + if err != nil { + return 0, err + } + oa := &windows.OBJECT_ATTRIBUTES{ + RootDirectory: dirfd, + ObjectName: objName, + // OBJ_DONT_REPARSE is the O_NOFOLLOW_ANY equivalent used by Go's Root: + // any reparse point fails with STATUS_REPARSE_POINT_ENCOUNTERED rather + // than being followed. + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + oa.Length = uint32(unsafe.Sizeof(*oa)) + + access := uint32(windows.FILE_GENERIC_READ | windows.SYNCHRONIZE) + options := uint32(windows.FILE_SYNCHRONOUS_IO_NONALERT | windows.FILE_OPEN_FOR_BACKUP_INTENT) + if directory { + options |= windows.FILE_DIRECTORY_FILE + // FILE_TRAVERSE: this handle becomes the RootDirectory for the next + // component's NtCreateFile call, which requires it without + // SeChangeNotifyPrivilege. + access |= windows.FILE_LIST_DIRECTORY | windows.FILE_TRAVERSE + } else { + options |= windows.FILE_NON_DIRECTORY_FILE + } + + var h windows.Handle + var iosb windows.IO_STATUS_BLOCK + err = windows.NtCreateFile( + &h, + access, + oa, + &iosb, + nil, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windows.FILE_OPEN, + options, + 0, + 0, + ) + if err != nil { + return 0, mapWindowsOpenErr(err) + } + return h, nil +} + +func isWindowsSymlinkErr(err error) bool { + if err == nil { + return false + } + // Some paths surface the mapped errno instead of the raw NT status. + if err == syscall.ELOOP || err == windows.ERROR_CANT_RESOLVE_FILENAME { + return true + } + if st, ok := err.(windows.NTStatus); ok && st == windows.STATUS_REPARSE_POINT_ENCOUNTERED { + return true + } + return false +} + +func mapWindowsOpenErr(err error) error { + if err == nil { + return nil + } + if st, ok := err.(windows.NTStatus); ok { + switch st { + // Missing final name, missing intermediate component, and the + // filesystem "file not found" status all mean the plan is absent. + // ReadPlan maps os.ErrNotExist to ("", false, nil). + case windows.STATUS_OBJECT_NAME_NOT_FOUND, + windows.STATUS_OBJECT_PATH_NOT_FOUND, + windows.STATUS_NO_SUCH_FILE: + return os.ErrNotExist + case windows.STATUS_OBJECT_NAME_COLLISION: + return syscall.EEXIST + case windows.STATUS_REPARSE_POINT_ENCOUNTERED: + return st + case windows.STATUS_FILE_IS_A_DIRECTORY: + return syscall.EISDIR + case windows.STATUS_NOT_A_DIRECTORY: + return syscall.ENOTDIR + default: + return st.Errno() + } + } + return err +} diff --git a/internal/planmode/read_windows_test.go b/internal/planmode/read_windows_test.go new file mode 100644 index 000000000..0b1842797 --- /dev/null +++ b/internal/planmode/read_windows_test.go @@ -0,0 +1,56 @@ +//go:build windows + +package planmode + +import ( + "strings" + "testing" +) + +func TestNtObjectPathDriveAndUNC(t *testing.T) { + // Drive-letter form: `\??\` + absolute path. + got := ntObjectPath(`C:\Users\example\AppData\Roaming`) + want := `\??\C:\Users\example\AppData\Roaming` + if got != want { + t.Fatalf("drive path = %q, want %q", got, want) + } + + // UNC form must go through the UNC device, not `\??\\\server\...`. + got = ntObjectPath(`\\server\share\AppData\Roaming`) + want = `\??\UNC\server\share\AppData\Roaming` + if got != want { + t.Fatalf("UNC path = %q, want %q", got, want) + } + + // Already-trimmed leading slashes must not produce a double UNC prefix + // when only one leading pair is present. + got = ntObjectPath(`\\fileserver\profiles\user`) + if !strings.HasPrefix(got, `\??\UNC\`) { + t.Fatalf("UNC path missing UNC device prefix: %q", got) + } + if strings.HasPrefix(got, `\??\UNC\\`) { + t.Fatalf("UNC path has doubled separators: %q", got) + } + + // Extended-length prefix is not UNC: it must map to `\??\`, not + // `\??\UNC\?\...`. + got = ntObjectPath(`\\?\C:\Users\example\AppData\Roaming`) + want = `\??\C:\Users\example\AppData\Roaming` + if got != want { + t.Fatalf("extended-length path = %q, want %q", got, want) + } + + // Extended-length UNC is already UNC-qualified after stripping `\\?\`. + got = ntObjectPath(`\\?\UNC\server\share\AppData\Roaming`) + want = `\??\UNC\server\share\AppData\Roaming` + if got != want { + t.Fatalf("extended-length UNC path = %q, want %q", got, want) + } + + // Device prefix must map to `\??\`, not `\??\UNC\.\...`. + got = ntObjectPath(`\\.\C:\Users\example\AppData\Roaming`) + want = `\??\C:\Users\example\AppData\Roaming` + if got != want { + t.Fatalf("device path = %q, want %q", got, want) + } +} diff --git a/internal/planmode/write.go b/internal/planmode/write.go new file mode 100644 index 000000000..191ce4d32 --- /dev/null +++ b/internal/planmode/write.go @@ -0,0 +1,45 @@ +package planmode + +import ( + "fmt" + "os" + "time" +) + +// writePlanFile creates intermediate directories and replaces path under base +// with content using a true no-follow, handle-relative walk (openat/mkdirat/ +// renameat on Unix; NtCreateFile with OBJ_DONT_REPARSE on Windows). A +// concurrent intermediate symlink or reparse-point swap cannot redirect the +// create or rename outside the storage tree. +// +// The storage base itself is created with pathname MkdirAll: it is the walk +// root, not a component under attacker control inside the plans tree. Every +// component under base is then created/opened handle-relative with no-follow. +// +// The durable write is atomic temp+rename relative to the parent directory +// handle. The temp name is PID plus nanoseconds (predictable); O_EXCL / +// FILE_CREATE refuses a colliding or pre-planted path at the final component. +func writePlanFile(base, path, content string) error { + if err := os.MkdirAll(base, 0o700); err != nil { + return fmt.Errorf("create plan directory: %w", err) + } + rel, err := relWithinBase(base, path) + if err != nil { + return err + } + return writePlanUnderBase(base, rel, path, content) +} + +// errPlanSymlinkWrite is the stable refusal for final and intermediate +// symlink / reparse-point components on the write path. WritePlan matches on +// "is a symlink". +func errPlanSymlinkWrite(path string) error { + return fmt.Errorf("plan file %s %w; refusing to write through it", path, errPlanSymlinkRefusal) +} + +// planTempName returns a sibling temp leaf name for atomic replace. The +// suffix is PID plus nanoseconds (predictable, not random); exclusivity of +// the create is what refuses a colliding or pre-planted path. +func planTempName(finalName string) string { + return fmt.Sprintf("%s.tmp-%d-%d", finalName, os.Getpid(), time.Now().UnixNano()) +} diff --git a/internal/planmode/write_other.go b/internal/planmode/write_other.go new file mode 100644 index 000000000..262b5d7e3 --- /dev/null +++ b/internal/planmode/write_other.go @@ -0,0 +1,25 @@ +//go:build !unix && !windows + +package planmode + +import "fmt" + +// writePlanUnderBase fails closed on platforms without openat / +// OBJ_DONT_REPARSE primitives, matching openPlanUnderBase in read_other.go. +// os.Root resolves in-root symlinks and a Lstat-then-open sequence is a +// check-to-use race, so containment cannot be bound at create/rename time. A +// plan written by a weaker fallback could also never be read back, since +// openPlanUnderBase always refuses on these platforms. Zero's supported +// targets are Unix and Windows, which use the true no-follow walkers in +// write_unix.go and write_windows.go. +func writePlanUnderBase(_, _, displayPath, _ string) error { + return fmt.Errorf("plan file %s: writing plan files is not supported on this platform", displayPath) +} + +func stageContentUnderBase(_, _, _ string) (string, func(), error) { + return "", nil, fmt.Errorf("stage plan file: staging is not supported on this platform") +} + +func tryReclaimStaleStagedFile(_, _ string) bool { + return false +} diff --git a/internal/planmode/write_unix.go b/internal/planmode/write_unix.go new file mode 100644 index 000000000..57e43168a --- /dev/null +++ b/internal/planmode/write_unix.go @@ -0,0 +1,301 @@ +//go:build unix + +package planmode + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "time" + + "golang.org/x/sys/unix" +) + +// writePlanUnderBase creates missing intermediate directories under base with +// mkdirat and openat(O_NOFOLLOW|O_DIRECTORY), then writes content into a +// temporary sibling of the final name and renameat's it into place. Every +// component is opened relative to the previous handle with O_NOFOLLOW, so an +// intermediate symlink swap cannot redirect create/rename outside base. +func writePlanUnderBase(base, rel, displayPath, content string) error { + parts, err := relComponents(rel) + if err != nil { + return err + } + + // O_NOFOLLOW on the base as well as on every component under it: see + // errPlanBaseSymlink. MkdirAll above happily accepts a base whose final + // component is a symlink to a directory, so without this the writer would + // create and rename inside the link's target. + dirfd, err := openatRetry(unix.AT_FDCWD, base, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + if isNoFollowErr(err) || isSymlinkDisguisedAsENOTDIR(unix.AT_FDCWD, base, err) { + return errPlanBaseSymlink(base) + } + return fmt.Errorf("create plan directory: %w", err) + } + defer func() { + if dirfd >= 0 { + _ = unix.Close(dirfd) + } + }() + + // Ensure every intermediate component exists as a real directory and is + // not a symlink. Create missing components with mkdirat (which does not + // follow a final-component symlink on the create itself); refuse EEXIST + // targets that are not plain directories by retrying open with O_NOFOLLOW. + for i := 0; i < len(parts)-1; i++ { + next, err := ensureDirNoFollow(dirfd, parts[i]) + if err != nil { + if isNoFollowErr(err) { + return errPlanSymlinkWrite(displayPath) + } + return fmt.Errorf("create plan directory: %w", err) + } + _ = unix.Close(dirfd) + dirfd = next + } + + // Owner-only on the immediate parent directory. fchmod acts on the open + // handle so a rename race cannot point chmod at a different path. + if err := unix.Fchmod(dirfd, 0o700); err != nil { + return fmt.Errorf("restrict plan directory permissions: %w", err) + } + + final := parts[len(parts)-1] + // Refuse a final-component symlink: rename would replace the name itself + // on Unix, but the durable plan contract is a plain file, not a link. + if err := refuseSymlinkAt(dirfd, final, displayPath); err != nil { + return err + } + + tmpName := planTempName(final) + fd, err := openatRetry(dirfd, tmpName, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0o600) + if err != nil { + if isNoFollowErr(err) { + return errPlanSymlinkWrite(displayPath) + } + return fmt.Errorf("write plan file: %w", err) + } + + // written gates cleanup: on failure close the raw fd (if still ours) and + // unlink the temp leaf. os.NewFile takes ownership of fd, so after a + // successful handoff only Unlinkat remains our job. + written := false + defer func() { + if !written { + if fd >= 0 { + _ = unix.Close(fd) + } + _ = unix.Unlinkat(dirfd, tmpName, 0) + } + }() + + // Stream content through the fd via os.File so short writes are handled. + file := os.NewFile(uintptr(fd), displayPath+" (tmp)") + if file == nil { + return fmt.Errorf("write plan file: invalid file descriptor") + } + fd = -1 + if _, err := file.WriteString(content); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } + if err := file.Close(); err != nil { + return fmt.Errorf("write plan file: %w", err) + } + + if err := renameatRetry(dirfd, tmpName, dirfd, final); err != nil { + return fmt.Errorf("replace plan file: %w", err) + } + _ = unix.Fsync(dirfd) + written = true + return nil +} + +// ensureDirNoFollow opens name under dirfd as a directory without following +// symlinks. If it is missing, mkdirat creates it, then openat is retried. +// Concurrent creators are handled by treating EEXIST as a successful create +// and reopening. +func ensureDirNoFollow(dirfd int, name string) (int, error) { + for try := 0; try < 2; try++ { + next, err := openatRetry(dirfd, name, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err == nil { + return next, nil + } + if isNoFollowErr(err) { + return -1, err + } + if isSymlinkDisguisedAsENOTDIR(dirfd, name, err) { + // Translate to ELOOP so the caller's isNoFollowErr check reaches + // the same refusal every other platform's symlink hit gives. + return -1, syscall.ELOOP + } + if err != syscall.ENOENT && !os.IsNotExist(err) { + // EEXIST without open succeeding means a non-directory is present. + return -1, err + } + if mkdirErr := unix.Mkdirat(dirfd, name, 0o700); mkdirErr != nil && mkdirErr != syscall.EEXIST { + return -1, mkdirErr + } + } + return -1, fmt.Errorf("create plan directory %s: exhausted retries", name) +} + +// refuseSymlinkAt fails when name under dirfd is a symlink. Missing names are +// fine (the subsequent O_EXCL create will introduce the file). +func refuseSymlinkAt(dirfd int, name, displayPath string) error { + var st unix.Stat_t + err := unix.Fstatat(dirfd, name, &st, unix.AT_SYMLINK_NOFOLLOW) + if err != nil { + if err == syscall.ENOENT || os.IsNotExist(err) { + return nil + } + return err + } + if st.Mode&unix.S_IFMT == unix.S_IFLNK { + return errPlanSymlinkWrite(displayPath) + } + return nil +} + +func renameatRetry(olddirfd int, oldpath string, newdirfd int, newpath string) error { + for { + err := unix.Renameat(olddirfd, oldpath, newdirfd, newpath) + if err == syscall.EINTR { + continue + } + return err + } +} + +// stageContentUnderBase opens the validated dir descriptor with O_NOFOLLOW and +// creates a temporary staged plan file plus an exclusive companion lock file +// relative to that descriptor, ensuring containment cannot be bypassed by +// intermediate path swaps. +func stageContentUnderBase(dir, sessionID, content string) (string, func(), error) { + dirfd, err := openatRetry(unix.AT_FDCWD, dir, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return "", nil, fmt.Errorf("open plan editor staging directory: %w", err) + } + defer func() { + if dirfd >= 0 { + _ = unix.Close(dirfd) + } + }() + + var st unix.Stat_t + if err := unix.Fstat(dirfd, &st); err != nil { + return "", nil, fmt.Errorf("stat plan editor staging directory: %w", err) + } + if (st.Mode & unix.S_IFMT) != unix.S_IFDIR { + return "", nil, fmt.Errorf("plan editor staging directory is not a directory") + } + if err := unix.Fchmod(dirfd, 0o700); err != nil { + return "", nil, fmt.Errorf("restrict plan editor staging directory permissions: %w", err) + } + + slug := slugify(sessionID) + var leafName string + var fd int = -1 + var lockFd int = -1 + for try := 0; try < 100; try++ { + candidate := fmt.Sprintf("%s-%d-%d.md", slug, os.Getpid(), time.Now().UnixNano()) + lockCandidate := candidate + ".lock" + + cLockFd, err := openatRetry(dirfd, lockCandidate, unix.O_RDWR|unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0o600) + if err != nil { + continue + } + if err := unix.Flock(cLockFd, unix.LOCK_EX|unix.LOCK_NB); err != nil { + _ = unix.Close(cLockFd) + _ = unix.Unlinkat(dirfd, lockCandidate, 0) + continue + } + + cFd, err := openatRetry(dirfd, candidate, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0o600) + if err != nil { + _ = unix.Flock(cLockFd, unix.LOCK_UN) + _ = unix.Close(cLockFd) + _ = unix.Unlinkat(dirfd, lockCandidate, 0) + continue + } + + leafName = candidate + fd = cFd + lockFd = cLockFd + break + } + if fd < 0 { + return "", nil, fmt.Errorf("stage plan file for editor: failed to create unique temporary file") + } + + stagedPath := filepath.Join(dir, leafName) + lockPath := stagedPath + ".lock" + + file := os.NewFile(uintptr(fd), stagedPath) + if file == nil { + _ = unix.Close(fd) + _ = unix.Flock(lockFd, unix.LOCK_UN) + _ = unix.Close(lockFd) + _ = os.Remove(stagedPath) + _ = os.Remove(lockPath) + return "", nil, fmt.Errorf("stage plan file for editor: invalid descriptor") + } + if _, err := file.WriteString(strings.TrimRight(content, "\n") + "\n"); err != nil { + _ = file.Close() + _ = unix.Flock(lockFd, unix.LOCK_UN) + _ = unix.Close(lockFd) + _ = os.Remove(stagedPath) + _ = os.Remove(lockPath) + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + if err := file.Close(); err != nil { + _ = unix.Flock(lockFd, unix.LOCK_UN) + _ = unix.Close(lockFd) + _ = os.Remove(stagedPath) + _ = os.Remove(lockPath) + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + + cleanup := func() { + _ = unix.Flock(lockFd, unix.LOCK_UN) + _ = unix.Close(lockFd) + _ = os.Remove(stagedPath) + _ = os.Remove(lockPath) + } + return stagedPath, cleanup, nil +} + +// tryReclaimStaleStagedFile attempts to reclaim an abandoned staged plan file. +// It verifies the filename matches the Zero staged format, opens the companion +// .lock file and attempts non-blocking exclusive flock. If the lock cannot be +// acquired (an editor is actively open), the file is preserved. +func tryReclaimStaleStagedFile(dir, leafName string) bool { + if !strings.HasSuffix(leafName, ".md") { + return false + } + dirfd, err := openatRetry(unix.AT_FDCWD, dir, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return false + } + defer func() { _ = unix.Close(dirfd) }() + + lockName := leafName + ".lock" + lockFd, err := openatRetry(dirfd, lockName, unix.O_RDWR|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err == nil { + defer func() { _ = unix.Close(lockFd) }() + if err := unix.Flock(lockFd, unix.LOCK_EX|unix.LOCK_NB); err != nil { + return false + } + defer func() { _ = unix.Flock(lockFd, unix.LOCK_UN) }() + } + _ = unix.Unlinkat(dirfd, leafName, 0) + _ = unix.Unlinkat(dirfd, lockName, 0) + return true +} diff --git a/internal/planmode/write_windows.go b/internal/planmode/write_windows.go new file mode 100644 index 000000000..31b0a3705 --- /dev/null +++ b/internal/planmode/write_windows.go @@ -0,0 +1,464 @@ +//go:build windows + +package planmode + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "time" + "unsafe" + + "golang.org/x/sys/windows" +) + +// writePlanUnderBase creates missing intermediate directories under base with +// NtCreateFile(OBJ_DONT_REPARSE) and replaces the final name via a handle- +// relative temp create + FileRenameInformation rename. Intermediate reparse +// points are refused rather than followed, matching openPlanUnderBase. +func writePlanUnderBase(base, rel, displayPath, content string) error { + parts, err := relComponents(rel) + if err != nil { + return err + } + + absBase, err := filepath.Abs(base) + if err != nil { + return err + } + + parent, err := openWindowsBaseDir(absBase) + if err != nil { + return fmt.Errorf("create plan directory: %w", err) + } + defer func() { _ = windows.CloseHandle(parent) }() + + for i := 0; i < len(parts)-1; i++ { + next, err := ensureDirNoFollowWindows(parent, parts[i]) + if err != nil { + if isWindowsSymlinkErr(err) { + return errPlanSymlinkWrite(displayPath) + } + return fmt.Errorf("create plan directory: %w", err) + } + _ = windows.CloseHandle(parent) + parent = next + } + + final := parts[len(parts)-1] + if err := refuseSymlinkAtWindows(parent, final, displayPath); err != nil { + return err + } + + tmpName := planTempName(final) + h, err := createFileNoFollow(parent, tmpName) + if err != nil { + if isWindowsSymlinkErr(err) { + return errPlanSymlinkWrite(displayPath) + } + return fmt.Errorf("write plan file: %w", err) + } + + written := false + defer func() { + if !written { + _ = windows.CloseHandle(h) + _ = deleteAtWindows(parent, tmpName) + } + }() + + file := os.NewFile(uintptr(h), displayPath+" (tmp)") + if file == nil { + return fmt.Errorf("write plan file: invalid file handle") + } + // os.NewFile owns h; clear so the failure path does not double-close. + // Keep a copy for rename (must reopen by name after Close, or rename + // before Close). Prefer rename while still holding the handle. + owned := h + h = windows.InvalidHandle + + if _, err := file.WriteString(content); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } + // Flush data before rename so a crash mid-write cannot leave a partial + // durable plan. Close is not enough on Windows without FlushFileBuffers + // for some media; WriteString + Close is the same contract as the prior + // pathname path, so keep that shape. + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } + + // Rename while the write handle is still open (needs DELETE access, which + // createFileNoFollow requested). Closing first would force a reopen race. + if err := renameatWindows(owned, parent, final); err != nil { + _ = file.Close() + return fmt.Errorf("replace plan file: %w", err) + } + if err := file.Close(); err != nil { + // Rename already landed; surface close error but do not unlink the + // durable name. + written = true + return fmt.Errorf("write plan file: %w", err) + } + written = true + return nil +} + +// ensureDirNoFollowWindows opens name under parent as a directory without +// following reparse points, creating it when missing. +func ensureDirNoFollowWindows(parent windows.Handle, name string) (windows.Handle, error) { + for try := 0; try < 2; try++ { + next, err := openatNoFollow(parent, name, true) + if err == nil { + return next, nil + } + if isWindowsSymlinkErr(err) { + return 0, err + } + if try > 0 { + return 0, err + } + // Missing: create then reopen. EEXIST means a concurrent creator won; + // loop back to open. Other create errors are fatal. + if !errors.Is(err, os.ErrNotExist) && !os.IsNotExist(err) { + return 0, err + } + if mkdirErr := mkdiratNoFollow(parent, name); mkdirErr != nil && !isWindowsExistErr(mkdirErr) { + return 0, mkdirErr + } + } + return 0, fmt.Errorf("create plan directory %s: exhausted retries", name) +} + +func mkdiratNoFollow(dirfd windows.Handle, name string) error { + objName, err := windows.NewNTUnicodeString(name) + if err != nil { + return err + } + oa := &windows.OBJECT_ATTRIBUTES{ + RootDirectory: dirfd, + ObjectName: objName, + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + oa.Length = uint32(unsafe.Sizeof(*oa)) + + var h windows.Handle + var iosb windows.IO_STATUS_BLOCK + err = windows.NtCreateFile( + &h, + windows.FILE_GENERIC_READ|windows.SYNCHRONIZE, + oa, + &iosb, + nil, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windows.FILE_CREATE, + windows.FILE_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_FOR_BACKUP_INTENT, + 0, + 0, + ) + if err != nil { + return mapWindowsOpenErr(err) + } + _ = windows.CloseHandle(h) + return nil +} + +// createFileNoFollow creates name under dirfd exclusively without following +// reparse points. DELETE access is requested so the handle can be renamed +// via FileRenameInformation without a reopen race. +func createFileNoFollow(dirfd windows.Handle, name string) (windows.Handle, error) { + objName, err := windows.NewNTUnicodeString(name) + if err != nil { + return 0, err + } + oa := &windows.OBJECT_ATTRIBUTES{ + RootDirectory: dirfd, + ObjectName: objName, + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + oa.Length = uint32(unsafe.Sizeof(*oa)) + + access := uint32(windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE | windows.DELETE | windows.SYNCHRONIZE) + options := uint32(windows.FILE_NON_DIRECTORY_FILE | windows.FILE_SYNCHRONOUS_IO_NONALERT | windows.FILE_OPEN_FOR_BACKUP_INTENT) + + var h windows.Handle + var iosb windows.IO_STATUS_BLOCK + err = windows.NtCreateFile( + &h, + access, + oa, + &iosb, + nil, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windows.FILE_CREATE, + options, + 0, + 0, + ) + if err != nil { + return 0, mapWindowsOpenErr(err) + } + return h, nil +} + +// refuseSymlinkAtWindows fails when name under dirfd is a reparse point. +// Missing names are fine. +func refuseSymlinkAtWindows(dirfd windows.Handle, name, displayPath string) error { + h, err := openatNoFollow(dirfd, name, false) + if err != nil { + if err == os.ErrNotExist || os.IsNotExist(err) { + return nil + } + // A directory at the final name is not a symlink; the subsequent + // FILE_NON_DIRECTORY_FILE create of the temp is fine, and rename + // will fail clearly if the final name is a directory. + if err == syscall.EISDIR { + return nil + } + if isWindowsSymlinkErr(err) { + return errPlanSymlinkWrite(displayPath) + } + // STATUS_OBJECT_NAME_NOT_FOUND already mapped; other open failures + // (access denied on a planted reparse) surface as-is. + return err + } + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(h, &info); err != nil { + _ = windows.CloseHandle(h) + return err + } + _ = windows.CloseHandle(h) + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return errPlanSymlinkWrite(displayPath) + } + return nil +} + +// renameatWindows renames the open handle h into newname under newdirfd, +// replacing any existing regular file at the destination. +func renameatWindows(h windows.Handle, newdirfd windows.Handle, newname string) error { + newNameUTF16, err := windows.UTF16FromString(newname) + if err != nil { + return err + } + fileNameLen := len(newNameUTF16)*2 - 2 // drop trailing NUL bytes from length + if fileNameLen < 0 { + return syscall.EINVAL + } + + type fileRenameInformation struct { + ReplaceIfExists uint32 + RootDirectory windows.Handle + FileNameLength uint32 + FileName [1]uint16 + } + var dummy fileRenameInformation + bufferSize := int(unsafe.Offsetof(dummy.FileName)) + fileNameLen + buffer := make([]byte, bufferSize) + info := (*fileRenameInformation)(unsafe.Pointer(&buffer[0])) + info.ReplaceIfExists = 1 // BOOLEAN + padding under FileRenameInformation + info.RootDirectory = newdirfd + info.FileNameLength = uint32(fileNameLen) + copy(unsafe.Slice(&info.FileName[0], fileNameLen/2), newNameUTF16) + + var iosb windows.IO_STATUS_BLOCK + err = windows.NtSetInformationFile(h, &iosb, &buffer[0], uint32(bufferSize), windows.FileRenameInformation) + if err != nil { + if st, ok := err.(windows.NTStatus); ok { + return st.Errno() + } + return err + } + return nil +} + +func deleteAtWindows(dirfd windows.Handle, name string) error { + h, err := openForDelete(dirfd, name) + if err != nil { + return err + } + defer windows.CloseHandle(h) + var iosb windows.IO_STATUS_BLOCK + // FileDispositionInformation = 13: mark handle for delete-on-close. + type dispositionInfo struct{ DeleteFile uint8 } + disp := dispositionInfo{DeleteFile: 1} + return windows.NtSetInformationFile(h, &iosb, (*byte)(unsafe.Pointer(&disp)), uint32(unsafe.Sizeof(disp)), windows.FileDispositionInformation) +} + +func openForDelete(dirfd windows.Handle, name string) (windows.Handle, error) { + objName, err := windows.NewNTUnicodeString(name) + if err != nil { + return 0, err + } + oa := &windows.OBJECT_ATTRIBUTES{ + RootDirectory: dirfd, + ObjectName: objName, + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + oa.Length = uint32(unsafe.Sizeof(*oa)) + + var h windows.Handle + var iosb windows.IO_STATUS_BLOCK + err = windows.NtCreateFile( + &h, + windows.DELETE|windows.SYNCHRONIZE|windows.FILE_READ_ATTRIBUTES, + oa, + &iosb, + nil, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windows.FILE_OPEN, + windows.FILE_NON_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_FOR_BACKUP_INTENT|windows.FILE_OPEN_REPARSE_POINT, + 0, + 0, + ) + if err != nil { + return 0, mapWindowsOpenErr(err) + } + return h, nil +} + +func isWindowsExistErr(err error) bool { + if err == nil { + return false + } + if err == syscall.EEXIST { + return true + } + if st, ok := err.(windows.NTStatus); ok && st == windows.STATUS_OBJECT_NAME_COLLISION { + return true + } + return false +} + +// stageContentUnderBase opens the validated dir handle with OBJ_DONT_REPARSE +// and creates a temporary staged plan file plus an exclusive companion lock file +// relative to that handle, ensuring containment cannot be bypassed by +// intermediate path swaps. +func stageContentUnderBase(dir, sessionID, content string) (string, func(), error) { + parent, err := openWindowsBaseDir(dir) + if err != nil { + return "", nil, fmt.Errorf("open plan editor staging directory: %w", err) + } + defer func() { _ = windows.CloseHandle(parent) }() + + slug := slugify(sessionID) + var leafName string + var h windows.Handle = windows.InvalidHandle + var lockH windows.Handle = windows.InvalidHandle + + for try := 0; try < 100; try++ { + candidate := fmt.Sprintf("%s-%d-%d.md", slug, os.Getpid(), time.Now().UnixNano()) + lockCandidate := candidate + ".lock" + + cLockH, err := createFileNoFollow(parent, lockCandidate) + if err != nil { + continue + } + // Lock the file exclusively with LockFileEx + var overlapped windows.Overlapped + if err := windows.LockFileEx(cLockH, windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &overlapped); err != nil { + _ = windows.CloseHandle(cLockH) + _ = deleteAtWindows(parent, lockCandidate) + continue + } + + cH, err := createFileNoFollow(parent, candidate) + if err != nil { + _ = windows.UnlockFileEx(cLockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(cLockH) + _ = deleteAtWindows(parent, lockCandidate) + continue + } + + leafName = candidate + h = cH + lockH = cLockH + break + } + if h == windows.InvalidHandle { + return "", nil, fmt.Errorf("stage plan file for editor: failed to create unique temporary file") + } + + stagedPath := filepath.Join(dir, leafName) + lockPath := stagedPath + ".lock" + + file := os.NewFile(uintptr(h), stagedPath) + if file == nil { + _ = windows.CloseHandle(h) + var overlapped windows.Overlapped + _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(lockH) + _ = deleteAtWindows(parent, leafName) + _ = deleteAtWindows(parent, leafName+".lock") + return "", nil, fmt.Errorf("stage plan file for editor: invalid handle") + } + if _, err := file.WriteString(strings.TrimRight(content, "\n") + "\n"); err != nil { + _ = file.Close() + var overlapped windows.Overlapped + _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(lockH) + _ = deleteAtWindows(parent, leafName) + _ = deleteAtWindows(parent, leafName+".lock") + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + if err := file.Sync(); err != nil { + _ = file.Close() + var overlapped windows.Overlapped + _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(lockH) + _ = deleteAtWindows(parent, leafName) + _ = deleteAtWindows(parent, leafName+".lock") + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + if err := file.Close(); err != nil { + var overlapped windows.Overlapped + _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(lockH) + _ = deleteAtWindows(parent, leafName) + _ = deleteAtWindows(parent, leafName+".lock") + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + + cleanup := func() { + var overlapped windows.Overlapped + _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) + _ = windows.CloseHandle(lockH) + _ = os.Remove(stagedPath) + _ = os.Remove(lockPath) + } + return stagedPath, cleanup, nil +} + +// tryReclaimStaleStagedFile attempts to reclaim an abandoned staged plan file on Windows. +func tryReclaimStaleStagedFile(dir, leafName string) bool { + if !strings.HasSuffix(leafName, ".md") { + return false + } + parent, err := openWindowsBaseDir(dir) + if err != nil { + return false + } + defer func() { _ = windows.CloseHandle(parent) }() + + lockName := leafName + ".lock" + lockH, err := openatNoFollow(parent, lockName, false) + if err == nil { + defer func() { _ = windows.CloseHandle(lockH) }() + var overlapped windows.Overlapped + if err := windows.LockFileEx(lockH, windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &overlapped); err != nil { + return false + } + defer func() { _ = windows.UnlockFileEx(lockH, 0, 1, 0, &overlapped) }() + } + _ = deleteAtWindows(parent, leafName) + _ = deleteAtWindows(parent, lockName) + return true +} diff --git a/internal/planmode/write_windows_test.go b/internal/planmode/write_windows_test.go new file mode 100644 index 000000000..0b67c3d6b --- /dev/null +++ b/internal/planmode/write_windows_test.go @@ -0,0 +1,77 @@ +//go:build windows + +package planmode + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +// TestWritePlanRefusesStorageRootReparsePoint is the Windows counterpart of +// TestPlanStorageBaseSymlinkRefused. Directory-symlink creation is privileged +// on many runners, so that test skips here. A junction is an unprivileged +// directory reparse point and is exactly what openWindowsBaseDir's +// OBJ_DONT_REPARSE must refuse. +func TestWritePlanRefusesStorageRootReparsePoint(t *testing.T) { + cfg := isolatePlanStorage(t) + workspace := t.TempDir() + + if _, err := WritePlan(workspace, "session-1", "1. [pending] real step\n"); err != nil { + t.Fatalf("WritePlan (seed): %v", err) + } + plansRoot := filepath.Join(cfg, filepath.FromSlash(PlanDirName)) + elsewhere := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(elsewhere, 0o700); err != nil { + t.Fatalf("mkdir elsewhere: %v", err) + } + if err := os.RemoveAll(plansRoot); err != nil { + t.Fatalf("remove plans root: %v", err) + } + createWindowsDirReparse(t, plansRoot, elsewhere) + + absBase, err := filepath.Abs(plansRoot) + if err != nil { + t.Fatalf("Abs plans root: %v", err) + } + handle, err := openWindowsBaseDir(absBase) + if err == nil { + _ = windows.CloseHandle(handle) + t.Fatal("openWindowsBaseDir accepted a reparse-point storage root") + } + if !errors.Is(err, errPlanSymlinkRefusal) { + t.Fatalf("openWindowsBaseDir err = %v, want errPlanSymlinkRefusal", err) + } + + if _, err := WritePlan(workspace, "session-1", "1. [pending] redirected\n"); err == nil { + t.Fatal("expected WritePlan to refuse a reparse-point plan storage root") + } else if !errors.Is(err, errPlanSymlinkRefusal) || !strings.Contains(err.Error(), "plan storage root") { + t.Fatalf("expected WritePlan to propagate errPlanSymlinkRefusal, got: %v", err) + } + + if content, ok, err := ReadPlan(workspace, "session-1"); err == nil { + t.Fatalf("expected ReadPlan to refuse a reparse-point plan storage root, got ok=%t content=%q", ok, content) + } else if !errors.Is(err, errPlanSymlinkRefusal) { + t.Fatalf("expected ReadPlan to propagate errPlanSymlinkRefusal, got: %v", err) + } + + if entries, _ := os.ReadDir(elsewhere); len(entries) != 0 { + t.Fatalf("write escaped through the storage-root reparse point into %s: %v", elsewhere, entries) + } +} + +func createWindowsDirReparse(t *testing.T, link, target string) { + t.Helper() + // Prefer a junction: unlike a directory symlink it needs no + // SeCreateSymbolicLinkPrivilege / Developer Mode. + if out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput(); err != nil { + if serr := os.Symlink(target, link); serr != nil { + t.Skipf("cannot create a reparse point (junction: %v %q; symlink: %v)", err, strings.TrimSpace(string(out)), serr) + } + } +} diff --git a/internal/tools/types.go b/internal/tools/types.go index 27755d8d4..1477da9a4 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -52,6 +52,11 @@ const ( SandboxDenialKindMeta = "sandbox_denial_kind" SandboxDenialReasonMeta = "sandbox_denial_reason" SandboxDenialKeywordMeta = "sandbox_denial_keyword" + // PlanSnapshotMeta carries the JSON-encoded []PlanItem a successful + // update_plan call installed, so consumers persist exactly that call's + // plan instead of re-reading the shared tool later (by which time a + // session switch may have cleared or replaced it). + PlanSnapshotMeta = "plan_snapshot" ) const ( @@ -131,6 +136,12 @@ type Result struct { // consumers; Output, Display, and spill metadata remain synchronized for // compatibility with direct tool callers and persisted sessions. Outcome ToolOutcome + // PlanSnapshot carries the typed, immutable snapshot of the []PlanItem + // accepted by a successful update_plan call. This internal control field + // is excluded from transcript/session serialization and secret scrubbing, + // ensuring durable plan persistence and UI panels retain the exact + // canonical plan without risk of redaction mutating secret-shaped step text. + PlanSnapshot []PlanItem `json:"-"` // pendingFileObservation is proposed by read_file and committed only after // the final model-visible output boundary confirms the exact content survived. pendingFileObservation *pendingFileObservation diff --git a/internal/tools/update_plan.go b/internal/tools/update_plan.go index 7f39635d9..71fd96909 100644 --- a/internal/tools/update_plan.go +++ b/internal/tools/update_plan.go @@ -69,16 +69,33 @@ func NewUpdatePlanTool() *updatePlanTool { } } -func (tool *updatePlanTool) Run(_ context.Context, args map[string]any) Result { +func (tool *updatePlanTool) Run(ctx context.Context, args map[string]any) Result { plan, err := parsePlanItems(args["plan"]) if err != nil { return errorResult("Error: Invalid arguments for update_plan: " + err.Error()) } plan = enforceSingleInProgress(plan) tool.mu.Lock() + defer tool.mu.Unlock() + // The context check shares the mutex with SetPlan/ClearPlan: a cancelled + // run's goroutine can reach this point after the UI has already reset the + // shared plan for a new session (its loop only checks cancellation + // between calls), and a late write here would repopulate the next + // session's plan with the cancelled run's state. Refusing under the lock + // means either this write lands before the reset (and the reset clears + // it) or the cancellation is visible here and nothing is written. + if ctx.Err() != nil { + return errorResult("Error: update_plan skipped: the run was cancelled.") + } tool.currentPlan = plan - tool.mu.Unlock() - return okResult(formatPlan(plan)) + result := okResult(formatPlan(plan)) + // Carry this call's plan with its typed result snapshot: the TUI persists + // the plan from the result callback, which runs after Run releases the + // mutex, so re-reading CurrentPlan there could observe a later session's state. + // We use the typed PlanSnapshot field rather than transcript metadata so + // downstream scrubbing cannot mutate secret-shaped plan step text. + result.PlanSnapshot = append([]PlanItem{}, plan...) + return result } func (tool *updatePlanTool) CurrentPlan() []PlanItem { @@ -87,6 +104,19 @@ func (tool *updatePlanTool) CurrentPlan() []PlanItem { return append([]PlanItem{}, tool.currentPlan...) } +// SetPlan replaces the in-memory plan with already-parsed items. It is used to +// sync a user-edited plan file (opened via /plan open) back into the agent's +// source of truth; the file is only ever the seed/target, the in-memory plan +// drives execution. The caller's slice is copied so enforceSingleInProgress +// cannot mutate the caller's storage when demoting extra in_progress items. +func (tool *updatePlanTool) SetPlan(plan []PlanItem) { + plan = append([]PlanItem{}, plan...) + plan = enforceSingleInProgress(plan) + tool.mu.Lock() + tool.currentPlan = plan + tool.mu.Unlock() +} + func (tool *updatePlanTool) ClearPlan() { tool.mu.Lock() tool.currentPlan = nil @@ -127,7 +157,7 @@ func parsePlanItems(value any) ([]PlanItem, error) { if err != nil { return nil, fmt.Errorf("plan item %d %s", index+1, err.Error()) } - status = normalizePlanStatus(status) + status = NormalizePlanStatus(status) notes, err := stringArgWithEmpty(object, "notes", "", false, true) if err != nil { return nil, fmt.Errorf("plan item %d %s", index+1, err.Error()) @@ -143,10 +173,10 @@ func parsePlanItems(value any) ([]PlanItem, error) { return plan, nil } -// normalizePlanStatus coerces a free-form status into one of the four canonical +// NormalizePlanStatus coerces a free-form status into one of the four canonical // values. Unknown/empty input maps to "pending" so a weak model's stray status // never fails the whole update_plan call (which would freeze the plan panel). -func normalizePlanStatus(status string) string { +func NormalizePlanStatus(status string) string { switch strings.ToLower(strings.TrimSpace(status)) { case "completed", "complete", "done", "finished", "resolved", "✓", "x", "[x]": return "completed" diff --git a/internal/tools/update_plan_test.go b/internal/tools/update_plan_test.go new file mode 100644 index 000000000..976775b05 --- /dev/null +++ b/internal/tools/update_plan_test.go @@ -0,0 +1,140 @@ +package tools + +import ( + "context" + "strings" + "sync" + "testing" +) + +// TestUpdatePlanRefusesCancelledRun pins the guard against a cancelled run's +// late update_plan call repopulating the shared plan after the UI has reset +// it for a new session: the agent loop only checks cancellation between +// calls, so the tool itself must refuse the write once its context is dead. +func TestUpdatePlanRefusesCancelledRun(t *testing.T) { + tool := NewUpdatePlanTool() + ctx, cancel := context.WithCancel(context.Background()) + result := tool.Run(ctx, map[string]any{"plan": []any{map[string]any{"content": "live", "status": "pending"}}}) + if result.Status != StatusOK { + t.Fatalf("live run: %+v", result) + } + if len(result.PlanSnapshot) != 1 || result.PlanSnapshot[0].Content != "live" { + t.Fatalf("snapshot did not match installed plan: %+v", result.PlanSnapshot) + } + + tool.SetPlan(nil) // the UI reset for a new session + cancel() + result = tool.Run(ctx, map[string]any{"plan": []any{map[string]any{"content": "stale"}}}) + if result.Status != StatusError { + t.Fatalf("cancelled run must be refused, got %+v", result) + } + if len(result.PlanSnapshot) != 0 { + t.Fatalf("cancelled run must not attach PlanSnapshot, got %#v", result.PlanSnapshot) + } + if items := tool.CurrentPlan(); len(items) != 0 { + t.Fatalf("cancelled run repopulated the shared plan: %+v", items) + } +} + +// TestUpdatePlanPreservesSecretShapedPlanStepsAcrossScrubbing is the regression +// for P1: plan steps containing secret-shaped strings or false-positive tokens +// must be scrubbed from transcript Output and Meta, but the typed PlanSnapshot +// and in-memory tool plan must remain identical to the accepted canonical input. +func TestUpdatePlanPreservesSecretShapedPlanStepsAcrossScrubbing(t *testing.T) { + tool := NewUpdatePlanTool() + secretToken := "ghp_123456789012345678901234567890123456" + stepContent := "Configure API with secret key " + secretToken + " and verify" + + result := tool.Run(context.Background(), map[string]any{ + "plan": []any{ + map[string]any{ + "content": stepContent, + "status": "in_progress", + "notes": "Key value: " + secretToken, + }, + }, + }) + if result.Status != StatusOK { + t.Fatalf("Run failed: %+v", result) + } + + // Verify pre-scrub snapshot holds exact unredacted secret + if len(result.PlanSnapshot) != 1 || result.PlanSnapshot[0].Content != stepContent { + t.Fatalf("PlanSnapshot mismatch before scrubbing: %+v", result.PlanSnapshot) + } + + // Run registry secret scrubbing boundary + scrubbed := scrubResultSecrets(result) + + // Output must be redacted + if strings.Contains(scrubbed.Output, secretToken) { + t.Fatalf("Output was not redacted by scrubResultSecrets: %q", scrubbed.Output) + } + + // PlanSnapshot must NOT be scrubbed/mutated + if len(scrubbed.PlanSnapshot) != 1 { + t.Fatalf("PlanSnapshot missing or corrupted after scrubbing: %+v", scrubbed.PlanSnapshot) + } + if scrubbed.PlanSnapshot[0].Content != stepContent { + t.Fatalf("PlanSnapshot content was mutated: got %q, want %q", scrubbed.PlanSnapshot[0].Content, stepContent) + } + if scrubbed.PlanSnapshot[0].Notes != "Key value: "+secretToken { + t.Fatalf("PlanSnapshot notes were mutated: got %q, want %q", scrubbed.PlanSnapshot[0].Notes, "Key value: "+secretToken) + } + + // Tool currentPlan must also retain exact unredacted secret + stored := tool.CurrentPlan() + if len(stored) != 1 || stored[0].Content != stepContent || stored[0].Notes != "Key value: "+secretToken { + t.Fatalf("tool.CurrentPlan() corrupted: %+v", stored) + } +} + +// TestUpdatePlanSetPlanDoesNotMutateCallerSlice pins that enforceSingleInProgress +// demotions cannot rewrite the caller's storage through SetPlan. +func TestUpdatePlanSetPlanDoesNotMutateCallerSlice(t *testing.T) { + tool := NewUpdatePlanTool() + caller := []PlanItem{ + {Content: "a", Status: "in_progress"}, + {Content: "b", Status: "in_progress"}, + } + tool.SetPlan(caller) + if caller[0].Status != "in_progress" || caller[1].Status != "in_progress" { + t.Fatalf("SetPlan mutated caller slice: %+v", caller) + } + got := tool.CurrentPlan() + if len(got) != 2 || got[0].Status != "completed" || got[1].Status != "in_progress" { + t.Fatalf("SetPlan did not enforce single in_progress on stored plan: %+v", got) + } +} + +// TestUpdatePlanConcurrentCancelAndReset races a late Run against SetPlan(nil) +// the way a cancelled agent goroutine can race a UI session switch. Under +// -race, either empty or the successful write is fine; a torn mix is not. +func TestUpdatePlanConcurrentCancelAndReset(t *testing.T) { + tool := NewUpdatePlanTool() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + _ = tool.Run(ctx, map[string]any{"plan": []any{map[string]any{"content": "stale", "status": "pending"}}}) + }() + go func() { + defer wg.Done() + tool.SetPlan(nil) + }() + wg.Wait() + + // After a cancelled Run and a clear, the plan must not hold the stale + // cancelled payload. Empty is the expected stable outcome; a concurrent + // non-cancelled write is out of scope for this test. + if items := tool.CurrentPlan(); len(items) != 0 { + // Cancelled Run may have lost the race before cancel was visible only + // if ctx was live; here ctx is already cancelled, so refuse must win + // or SetPlan(nil) cleared after. Non-empty means the cancelled path + // wrote, which the mutex ordering forbids. + t.Fatalf("concurrent cancel/reset left unexpected plan: %+v", items) + } +} diff --git a/internal/tui/btw.go b/internal/tui/btw.go index 109b76bf6..f4e267e6b 100644 --- a/internal/tui/btw.go +++ b/internal/tui/btw.go @@ -7,6 +7,7 @@ import ( tea "charm.land/bubbletea/v2" "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/usage" ) @@ -29,6 +30,7 @@ Nothing from this side conversation will be merged into the main session.` type btwState struct { active bool parent *model + parentPlanItems []tools.PlanItem sideRunIDBase int parentNeedsInput bool } @@ -143,7 +145,21 @@ func (m model) handleBTWCommand(question string) (model, tea.Cmd) { side.activeLoopID = "" side.loopTicking = false side.specialists.clear() - side.plan.clear() + // Plan mode (and the in-memory plan) belongs to the parent session. A + // side surface that inherited it would stay read-only, or leak the + // parent's draft into a conversation that never drafted it. Match + // /new and /resume: exit plan mode and clear plan state on the side + // only. Capture the parent's current in-memory plan snapshot before + // resetting so returnFromBTW can restore it even if durable persistence + // was unavailable or reload encounters an error. + var parentPlanItems []tools.PlanItem + if reader, ok := parent.registry.Get("update_plan"); ok { + if r, ok := reader.(currentPlanReader); ok { + parentPlanItems = r.CurrentPlan() + } + } + side = side.exitPlanMode() + side = side.resetPlanForSessionSwitch() side.planDetailGen++ side.streamingText = nil side.streamingReasoning = "" @@ -151,9 +167,10 @@ func (m model) handleBTWCommand(question string) (model, tea.Cmd) { side.clearStreamingToolCall() side.resetStreamingFade() side.btw = btwState{ - active: true, - parent: &parent, - sideRunIDBase: side.runID, + active: true, + parent: &parent, + parentPlanItems: parentPlanItems, + sideRunIDBase: side.runID, } if question == "" { @@ -177,6 +194,7 @@ func (m model) leaveBTW() (model, tea.Cmd) { } m, _ = m.clearLoopsForSessionSwitch() parent := *m.btw.parent + savedParentPlan := m.btw.parentPlanItems parent.goalContinuationsSuspended = false parent.btwRunIDSeq = maxInt(parent.btwRunIDSeq, m.runID) parent.btw = btwState{} @@ -198,7 +216,47 @@ func (m model) leaveBTW() (model, tea.Cmd) { kind: actionAppendSystem, text: "Returned from the isolated BTW conversation. Its messages were not added to this session.", }) + // Entering BTW clears (or the side conversation may replace) the shared + // update_plan tool state. Re-sync from the parent session's plan file the + // same way /resume does after a session switch, so the restored surface + // matches the durable plan and not whatever the side conversation left. + // Surface I/O/parse failures so the restored panel and shared update_plan + // state are not silently left out of sync with the durable file, while + // restoring the captured parent plan snapshot if reload fails or is missing. + if items, ok, err := parent.reloadPlanFromFile(); err != nil { + // Durable reload failed: surface the error, but restore the saved + // parent plan snapshot into both the update_plan tool and the panel so + // an existing usable plan is not destroyed. + if reloader, ok := parent.registry.Get("update_plan"); ok { + if r, ok := reloader.(planFileReloader); ok { + r.SetPlan(savedParentPlan) + } + } + if len(savedParentPlan) > 0 { + parent.plan.updateFromItems(savedParentPlan, parent.now()) + } + parent.transcript = reduceTranscript(parent.transcript, transcriptAction{ + kind: actionAppendError, + text: "plan reload error: " + err.Error(), + }) + } else if ok { + parent.plan.updateFromItems(items, parent.now()) + } else { + // Missing durable plan (ok=false, err=nil): restore the parent's + // captured in-memory plan draft and sticky panel. + if reloader, ok := parent.registry.Get("update_plan"); ok { + if r, ok := reloader.(planFileReloader); ok { + r.SetPlan(savedParentPlan) + } + } + if len(savedParentPlan) > 0 { + parent.plan.updateFromItems(savedParentPlan, parent.now()) + } else { + parent.plan.clear() + } + } parent.resetFlushFrontier("· returned from btw ·") + parent = parent.syncPeerIdentity() var goalCmd tea.Cmd parent, goalCmd = parent.launchGoalContinuationIfReady() return parent, batchCommands(sweepCmd, spinnerCmd, goalCmd) @@ -207,7 +265,7 @@ func (m model) leaveBTW() (model, tea.Cmd) { func btwCommandUnavailable(command parsedCommand) bool { arg := strings.ToLower(strings.TrimSpace(command.text)) switch command.kind { - case commandNew, commandResume, commandRename, commandSpec, commandLoop, commandGoal, + case commandNew, commandResume, commandRename, commandSpec, commandPlan, commandLoop, commandGoal, commandRewind, commandCompact, commandSTTModel, commandMCP: return true case commandModel: diff --git a/internal/tui/btw_test.go b/internal/tui/btw_test.go index 0d2b958a7..55aeb08e7 100644 --- a/internal/tui/btw_test.go +++ b/internal/tui/btw_test.go @@ -9,7 +9,11 @@ import ( tea "charm.land/bubbletea/v2" + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/peermsg" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" ) func newBTWTestModel(t *testing.T) model { @@ -473,3 +477,278 @@ func TestBTWCtrlCDuringRunDoesNotClearDraft(t *testing.T) { t.Fatalf("missing in-flight return guidance: %#v", got.transcript) } } + +// Regression: entering plan mode then /btw used to copy permissionMode and the +// shared update_plan state onto the side surface. Match /new and /resume: the +// side conversation must exit plan mode and clear plan state, while the hidden +// parent keeps plan mode for restore. +func TestBTWExitsPlanModeOnSideAndPreservesParent(t *testing.T) { + isolatePlanConfig(t) + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "draft step", Status: "pending"}}) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newBTWTestModel(t) + m.cwd = t.TempDir() + m.registry = registry + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + + side, _ := m.handleBTWCommand("") + if side.permissionMode == agent.PermissionModePlan { + t.Fatalf("BTW side kept plan mode: %s", side.permissionMode) + } + if side.permissionMode != agent.PermissionModeAsk { + t.Fatalf("BTW side permission mode = %s, want restored Ask", side.permissionMode) + } + if side.permissionModeBeforePlan != "" { + t.Fatalf("BTW side left permissionModeBeforePlan set: %q", side.permissionModeBeforePlan) + } + if !side.plan.isEmpty() { + t.Fatalf("BTW side leaked the parent plan panel: %+v", side.plan) + } + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("BTW side left shared update_plan state: %+v", planTool.CurrentPlan()) + } + if side.btw.parent == nil { + t.Fatal("expected saved parent after /btw") + } + if side.btw.parent.permissionMode != agent.PermissionModePlan { + t.Fatalf("hidden parent lost plan mode: %s", side.btw.parent.permissionMode) + } + if side.btw.parent.permissionModeBeforePlan != agent.PermissionModeAsk { + t.Fatalf("hidden parent lost permissionModeBeforePlan: %q", side.btw.parent.permissionModeBeforePlan) + } + if side.btw.parent.plan.isEmpty() { + t.Fatal("hidden parent lost its sticky plan panel") + } + + returned, _ := side.leaveBTW() + if returned.permissionMode != agent.PermissionModePlan { + t.Fatalf("returning from BTW lost parent plan mode: %s", returned.permissionMode) + } + if returned.permissionModeBeforePlan != agent.PermissionModeAsk { + t.Fatalf("returning from BTW lost permissionModeBeforePlan: %q", returned.permissionModeBeforePlan) + } + // The captured parent in-memory plan is restored on return even when no + // durable plan file exists. + if returned.plan.isEmpty() { + t.Fatal("returning from BTW unexpectedly cleared parent's sticky plan panel") + } + if len(planTool.CurrentPlan()) != 1 || planTool.CurrentPlan()[0].Content != "draft step" { + t.Fatalf("expected shared update_plan restored from parent snapshot, got %+v", planTool.CurrentPlan()) + } +} + +func TestBTWLeaveRestoresParentPeerIdentity(t *testing.T) { + isolatePlanConfig(t) + svc, err := peermsg.New(peermsg.Options{ + RootDir: t.TempDir(), + Identity: peermsg.Identity{ + Name: "zero", + Cwd: t.TempDir(), + PermissionClass: peermsg.PermissionBypass, + }, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := svc.Start(func(peermsg.InboundMessage) bool { return true }); err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { _ = svc.Close() }) + + m := newBTWTestModel(t) + m.cwd = t.TempDir() + m.permissionMode = agent.PermissionModeUnsafe + m.peerService = svc + + updated, _ := m.handlePlanCommand("on") + parent := updated.(model) + if got := svc.Self().PermissionClass; got != peermsg.PermissionPrompting { + t.Fatalf("after /plan on PermissionClass = %q, want %q", got, peermsg.PermissionPrompting) + } + + side, _ := parent.handleBTWCommand("") + if got := svc.Self().PermissionClass; got != peermsg.PermissionBypass { + t.Fatalf("inside /btw PermissionClass = %q, want %q", got, peermsg.PermissionBypass) + } + + returned, _ := side.leaveBTW() + if returned.permissionMode != agent.PermissionModePlan { + t.Fatalf("returning from BTW lost parent plan mode: %s", returned.permissionMode) + } + if got := svc.Self().PermissionClass; got != peermsg.PermissionPrompting { + t.Fatalf("after returning from /btw PermissionClass = %q, want %q", got, peermsg.PermissionPrompting) + } +} + +func TestBTWCommandUnavailableBlocksPlan(t *testing.T) { + if !btwCommandUnavailable(parsedCommand{kind: commandPlan, name: "/plan"}) { + t.Fatal("expected /plan to be unavailable inside a BTW conversation") + } + // Sanity: help stays available so the blocklist is not total. + if btwCommandUnavailable(parsedCommand{kind: commandHelp, name: "/help"}) { + t.Fatal("expected /help to remain available in BTW") + } +} + +// Regression: enterBTW clears shared update_plan; leaveBTW must re-hydrate it +// from the parent session plan file the way /resume does after a switch. +func TestBTWLeaveResyncsSharedPlanFromParentFile(t *testing.T) { + isolatePlanConfig(t) + cwd := t.TempDir() + planTool := tools.NewUpdatePlanTool() + items := []tools.PlanItem{{Content: "draft step", Status: "pending"}} + planTool.SetPlan(items) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newBTWTestModel(t) + m.cwd = cwd + m.registry = registry + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(items, m.now()) + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, formatPlanItems(items)); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + side, _ := m.handleBTWCommand("") + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("BTW side left shared update_plan state: %+v", planTool.CurrentPlan()) + } + + returned, _ := side.leaveBTW() + got := planTool.CurrentPlan() + if len(got) != 1 || got[0].Content != "draft step" { + t.Fatalf("leaveBTW did not re-sync shared update_plan from parent plan file: %+v", got) + } + if returned.plan.isEmpty() { + t.Fatal("leaveBTW left sticky plan panel empty after re-sync") + } +} + +// Regression: when the durable plan file is gone (ok=false, err=nil), leaveBTW +// Regression: when the durable plan file is missing (ok=false, err=nil), leaveBTW +// restores the captured parent in-memory plan draft so the parent's draft and panel +// are preserved rather than lost. +func TestBTWLeavePreservesInMemPlanWhenPlanFileMissing(t *testing.T) { + isolatePlanConfig(t) + cwd := t.TempDir() + planTool := tools.NewUpdatePlanTool() + items := []tools.PlanItem{{Content: "draft step", Status: "pending"}} + planTool.SetPlan(items) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newBTWTestModel(t) + m.cwd = cwd + m.registry = registry + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(items, m.now()) + + side, _ := m.handleBTWCommand("") + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("BTW side left shared update_plan state: %+v", planTool.CurrentPlan()) + } + + returned, _ := side.leaveBTW() + got := planTool.CurrentPlan() + if len(got) != 1 || got[0].Content != "draft step" { + t.Fatalf("leaveBTW did not restore parent in-memory plan when file was missing, got: %+v", got) + } + if returned.plan.isEmpty() { + t.Fatal("expected sticky plan panel preserved when plan file is missing") + } +} + +// Regression: leaveBTW must surface a durable plan reload failure while preserving +// the parent's captured in-memory plan so an error does not destroy usable state. +func TestBTWLeaveReportsPlanReloadErrorAndPreservesParentPlan(t *testing.T) { + isolatePlanConfig(t) + cwd := t.TempDir() + planTool := tools.NewUpdatePlanTool() + items := []tools.PlanItem{{Content: "draft step", Status: "pending"}} + planTool.SetPlan(items) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newBTWTestModel(t) + m.cwd = cwd + m.registry = registry + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(items, m.now()) + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, formatPlanItems(items)); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + // Replace the plan file with a directory so ReadPlan fails with a real + // I/O error (missing file is not an error). + path, err := planmode.PlanFilePath(cwd, m.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if err := os.Remove(path); err != nil { + t.Fatalf("Remove plan file: %v", err) + } + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("Mkdir over plan path: %v", err) + } + + side, _ := m.handleBTWCommand("") + returned, _ := side.leaveBTW() + if !transcriptContains(returned.transcript, "plan reload error:") { + t.Fatalf("leaveBTW did not surface plan reload failure: %#v", returned.transcript) + } + got := planTool.CurrentPlan() + if len(got) != 1 || got[0].Content != "draft step" { + t.Fatalf("expected parent in-memory plan preserved after failed reload, got %+v", got) + } + if returned.plan.isEmpty() { + t.Fatal("expected sticky plan panel preserved after failed reload") + } +} + +// TestBTWLeaveSidePlanUpdateDoesNotLeakToParent verifies that plan updates made +// inside a BTW conversation stay isolated and never overwrite or leak into the parent. +func TestBTWLeaveSidePlanUpdateDoesNotLeakToParent(t *testing.T) { + isolatePlanConfig(t) + cwd := t.TempDir() + planTool := tools.NewUpdatePlanTool() + parentItems := []tools.PlanItem{{Content: "parent step", Status: "in_progress"}} + planTool.SetPlan(parentItems) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newBTWTestModel(t) + m.cwd = cwd + m.registry = registry + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(parentItems, m.now()) + + side, _ := m.handleBTWCommand("") + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("expected side update_plan initially empty, got: %+v", planTool.CurrentPlan()) + } + + // Side conversation updates its own plan + sideItems := []tools.PlanItem{{Content: "side step", Status: "pending"}} + planTool.SetPlan(sideItems) + side.plan.updateFromItems(sideItems, side.now()) + + // Return to parent + returned, _ := side.leaveBTW() + got := planTool.CurrentPlan() + if len(got) != 1 || got[0].Content != "parent step" { + t.Fatalf("side plan leaked into parent session: got %+v, want parent step", got) + } + if returned.plan.isEmpty() { + t.Fatal("parent sticky plan panel was lost after returning from BTW") + } +} diff --git a/internal/tui/commands.go b/internal/tui/commands.go index 5eea59d21..6076a9ff8 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -114,9 +114,9 @@ var commandDefinitions = []commandDefinition{ }, { name: "/plan", - usage: "/plan [status|on|off]", + usage: "/plan [status|on|open|off]", group: commandGroupSession, - description: "Show plan status, or enter/exit read-only planning mode.", + description: "Show plan status, enter plan mode, open the plan file, or exit plan mode.", kind: commandPlan, }, { diff --git a/internal/tui/commands_test.go b/internal/tui/commands_test.go index 2b2da10bf..973be11f1 100644 --- a/internal/tui/commands_test.go +++ b/internal/tui/commands_test.go @@ -50,7 +50,7 @@ func TestFormatCommandHelpLinesGroupsCommandsByStableOrder(t *testing.T) { " /effort [list|level|auto] - Show or set reasoning effort for supported models.", " /fast - Toggle fast mode for supported ChatGPT subscription models.", "session:", - " /plan [status|on|off] - Show plan status, or enter/exit read-only planning mode.", + " /plan [status|on|open|off] - Show plan status, enter plan mode, open the plan file, or exit plan mode.", "runtime:", " /permissions - Show the active permission mode and sandbox grants.", " /debug (/debug-mode) - Show debug mode status.", diff --git a/internal/tui/goal.go b/internal/tui/goal.go index 4b5ac83fa..305d9a496 100644 --- a/internal/tui/goal.go +++ b/internal/tui/goal.go @@ -258,11 +258,15 @@ func (m model) goalSystemPrompt(base string) string { return base + "\n\n" + instruction } +func (m model) hasArmedGoalContinuation() bool { + return m.activeSession.Goal != nil && m.activeSession.Goal.Status == sessions.GoalStatusActive +} + func (m model) launchGoalContinuationIfReady() (model, tea.Cmd) { goal := m.activeSession.Goal if goal == nil || goal.Status != sessions.GoalStatusActive || m.pending || m.compactInFlight || m.exiting || m.provider == nil || - m.goalContinuationsSuspended { + m.goalContinuationsSuspended || m.planModeBlocksContinuations() { return m, nil } updated, event, reserved, err := m.sessionStore.ReserveGoalContinuation(m.activeSession.SessionID) diff --git a/internal/tui/goal_test.go b/internal/tui/goal_test.go index 5f76d6c79..fbdf1352f 100644 --- a/internal/tui/goal_test.go +++ b/internal/tui/goal_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/Gitlawb/zero/internal/agent" "github.com/Gitlawb/zero/internal/sessions" "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/zeroruntime" @@ -263,6 +264,35 @@ func TestActiveGoalLaunchesContinuation(t *testing.T) { } } +func TestGoalContinuationSkippedInPlanMode(t *testing.T) { + // Regression: an armed /goal must not launch automatic turns while plan + // mode is active — those turns cannot make implementation progress. + store := testSessionStore(t) + session, err := store.Create(sessions.CreateInput{SessionID: "goal_plan"}) + if err != nil { + t.Fatal(err) + } + session, _, err = store.CreateGoal(session.SessionID, "Keep going", 0) + if err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + Provider: &scriptedProvider{}, + Registry: tools.NewRegistry(), + SessionStore: store, + PermissionMode: agent.PermissionModePlan, + }) + m.activeSession = session + + next, cmd := m.launchGoalContinuationIfReady() + if cmd != nil || next.pending { + t.Fatal("an armed goal must not launch a continuation while plan mode is active") + } + if next.activeSession.Goal == nil || next.activeSession.Goal.Status != sessions.GoalStatusActive { + t.Fatalf("plan mode must leave the goal armed, got %#v", next.activeSession.Goal) + } +} + func TestGoalContinuationChainStopsAtPersistedLimit(t *testing.T) { store := testSessionStore(t) session, err := store.Create(sessions.CreateInput{SessionID: "goal_hard_stop"}) diff --git a/internal/tui/loop.go b/internal/tui/loop.go index 03fb5803f..26faddb8c 100644 --- a/internal/tui/loop.go +++ b/internal/tui/loop.go @@ -291,6 +291,7 @@ func (m model) startLoop(cmd loopCommand) (model, tea.Cmd) { interval: cmd.interval, createdAt: m.now(), nextRunAt: m.now(), // fire the first iteration on the next idle tick + paused: m.planModeBlocksContinuations(), } m.loops = append(m.loops, loop) note := "" @@ -334,10 +335,10 @@ func (m model) stopAllLoops() (model, tea.Cmd) { } // fireDueLoopIfIdle fires the earliest due loop when the session is idle. Called -// from the poll tick; a no-op while a turn, modal, or queued user message is -// pending (the loop simply waits for the next idle tick). +// from the poll tick; a no-op while a turn, modal, queued user message, or plan +// mode is pending (the loop simply waits for the next idle tick). func (m model) fireDueLoopIfIdle() (model, tea.Cmd) { - if m.loopBusy() || len(m.loops) == 0 { + if m.loopBusy() || len(m.loops) == 0 || m.planModeBlocksContinuations() { return m, nil } now := m.now() @@ -640,6 +641,33 @@ func (m model) validateLoopTarget(prompt string) (string, bool) { // the session that created them; carrying them across /new or /resume would fire the // old session's prompt into an unrelated conversation. Returns the count cleared so // the caller can note it. Pure state reset — no transcript writes. +// pauseLoopsForPlan marks every active loop paused so the idle ticker cannot +// fire implementation turns while plan mode is read-only. Returns how many +// loops were newly paused. +func (m model) pauseLoopsForPlan() (model, int) { + n := 0 + for _, l := range m.loops { + if l == nil || l.paused { + continue + } + l.paused = true + n++ + } + return m, n +} + +// resumeLoopsAfterPlan unpauses loops that were held while plan mode was +// active so the next idle tick can fire them again. +func (m model) resumeLoopsAfterPlan() model { + for _, l := range m.loops { + if l == nil { + continue + } + l.paused = false + } + return m +} + func (m model) clearLoopsForSessionSwitch() (model, int) { n := len(m.loops) if n == 0 { diff --git a/internal/tui/loop_controller_test.go b/internal/tui/loop_controller_test.go index 500f97a66..7a7d14301 100644 --- a/internal/tui/loop_controller_test.go +++ b/internal/tui/loop_controller_test.go @@ -9,6 +9,7 @@ import ( tea "charm.land/bubbletea/v2" + "github.com/Gitlawb/zero/internal/agent" "github.com/Gitlawb/zero/internal/usercommands" ) @@ -24,6 +25,19 @@ func startFixedLoop(m model, prompt string, interval time.Duration) model { return m } +func TestStartLoopPausesWhilePlanModeActive(t *testing.T) { + now := time.Date(2026, 7, 5, 12, 0, 0, 0, time.UTC) + m := loopTestModel(t, now) + m.permissionMode = agent.PermissionModePlan + m = startFixedLoop(m, "check the build", 5*time.Minute) + if len(m.loops) != 1 || !m.loops[0].paused { + t.Fatalf("a loop started in plan mode should be paused, got %+v", m.loops) + } + if m.loops[0].due(now) { + t.Fatal("a loop started in plan mode must not be due") + } +} + func TestStartLoopRegistersAndSchedules(t *testing.T) { now := time.Date(2026, 7, 5, 12, 0, 0, 0, time.UTC) m := loopTestModel(t, now) @@ -172,6 +186,22 @@ func TestStopLoopByID(t *testing.T) { } } +func TestFireDueLoopSkipsInPlanMode(t *testing.T) { + // Regression: a due loop must stay armed and not fire while plan mode is + // read-only. Implementation turns cannot make progress there. + now := time.Date(2026, 7, 5, 12, 0, 0, 0, time.UTC) + m := loopTestModel(t, now) + m = startFixedLoop(m, "x", time.Minute) + m.permissionMode = agent.PermissionModePlan + got, cmd := m.fireDueLoopIfIdle() + if got.activeLoopID != "" || cmd != nil { + t.Fatal("a due loop must not fire while plan mode is active") + } + if got.loops[0].nextRunAt.IsZero() { + t.Fatal("a loop skipped in plan mode must stay scheduled") + } +} + func TestFireDueLoopSkipsWhenBusy(t *testing.T) { now := time.Date(2026, 7, 5, 12, 0, 0, 0, time.UTC) m := loopTestModel(t, now) diff --git a/internal/tui/model.go b/internal/tui/model.go index 9473de06a..e4d885795 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -28,6 +28,7 @@ import ( "github.com/Gitlawb/zero/internal/modelregistry" "github.com/Gitlawb/zero/internal/notify" "github.com/Gitlawb/zero/internal/peermsg" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/providerhealth" "github.com/Gitlawb/zero/internal/providermodeldiscovery" "github.com/Gitlawb/zero/internal/providers" @@ -1454,6 +1455,66 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { m.transientNotice = transientNotice{} } return m, nil + case planEditorFinishedMsg: + if msg.err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan editor error: " + msg.err.Error()}) + return m, nil + } + // Capture what the editor started from, before reloadPlanFromFile + // replaces it, so an editor session that changed nothing (open, read, + // quit) can be told apart from a real edit below. + var beforeEdit []tools.PlanItem + if tool, found := m.registry.Get("update_plan"); found { + if reader, isReader := tool.(currentPlanReader); isReader { + beforeEdit = reader.CurrentPlan() + } + } + // The user may have edited the plan file in $EDITOR; sync it back into + // the in-memory update_plan so the edited plan drives execution, and + // refresh the sticky plan panel to match. + items, ok, reloadErr := m.reloadPlanFromFile() + if reloadErr != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan reload error: " + reloadErr.Error()}) + return m, nil + } + if !ok { + return m, nil + } + // Quitting the editor without touching anything must not claim an edit + // happened. The session event below is written as the user's own words, + // so recording it unchanged would put a false statement into the next + // turn's context, and repeated opens would each restate the whole plan. + if planItemsEqual(beforeEdit, items) { + return m, nil + } + m.plan.updateFromItems(items, m.now()) + // The sticky-panel refresh above is the only visible sign the edit was + // taken up; a /plan open with no other output would otherwise look like + // nothing happened. Confirm the reload (or a clear) in the transcript. + reloadNote := "Reloaded the edited plan." + if len(items) == 0 { + reloadNote = "Cleared the plan (the edited plan file is empty)." + } + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: reloadNote}) + // SetPlan (inside reloadPlanFromFile) only changes the update_plan + // tool's in-memory state; the model has no way to observe that on its + // own. Record it as a session event too, so a user-authored edit + // actually reaches the next turn's context — whether that turn is + // more planning or, after /plan off, the implementation run the + // feature is supposed to drive. + content := "I edited the plan file directly and cleared the plan." + if plan := formatPlanItems(items); plan != "" { + content = "I edited the plan file directly. Updated plan:\n\n" + plan + } + var err error + m, err = m.appendSessionEvent(sessions.EventMessage, map[string]any{ + "role": "user", + "content": content, + }) + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session record error: " + err.Error()}) + } + return m, nil case exitConfirmExpiredMsg: if msg.seq == m.exitConfirmSeq { m.exitConfirmActive = false @@ -2619,7 +2680,8 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { // BEFORE the reset below clears them, and skip spec-draft reviews — those // are legitimate mid-plan err==nil yields where the plan is NOT done. if msg.err == nil && msg.specReview == nil && - m.pendingAskUser == nil && m.pendingPermission == nil { + m.pendingAskUser == nil && m.pendingPermission == nil && + m.permissionMode != agent.PermissionModePlan { m.plan.completeRemaining(m.now()) } m.pendingPermission = nil @@ -4780,10 +4842,7 @@ 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: - text := "" - m, text = m.handlePlanCommand(command.text) - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) - return m, nil + return m.handlePlanCommand(command.text) case commandDoctor: return m.startDoctorCommand(command.text) case commandSearch: @@ -5251,7 +5310,7 @@ func (m *model) ensureSpinnerTick() tea.Cmd { } func (m model) launchQueuedMessageIfReady() (model, tea.Cmd) { - if !m.hasQueuedMessage() || m.pending || m.exiting || m.pendingPermission != nil || m.pendingAskUser != nil || m.pendingSpecReview != nil { + if !m.hasQueuedMessage() || m.pending || m.exiting || m.pendingPermission != nil || m.pendingAskUser != nil || m.pendingSpecReview != nil || m.planModeBlocksContinuations() { return m, nil } prompt := m.queuedMessage @@ -5454,8 +5513,22 @@ func (m model) runAgentWithOptions(runID int, runCtx context.Context, prompt str if runOptions.permissionMode != "" { options.PermissionMode = runOptions.permissionMode } - if runOptions.systemPrompt != "" { + switch { + case runOptions.systemPrompt != "": options.SystemPrompt = runOptions.systemPrompt + case options.PermissionMode == agent.PermissionModePlan: + // Plan mode is toggled via /plan on the normal submit path (not a + // dedicated run-launch command like /spec), so there is no call site + // to pass planmode.DraftSystemPrompt through runOptions: set it here + // from the active permission mode instead. Layer it onto (rather + // than replace) any configured options.SystemPrompt: an embedder's + // system prompt encodes product policy that must still apply while + // planning, not just on ordinary turns. + if configured := strings.TrimSpace(options.SystemPrompt); configured != "" { + options.SystemPrompt = configured + "\n\n" + planmode.DraftSystemPrompt + } else { + options.SystemPrompt = planmode.DraftSystemPrompt + } } if runOptions.transientSystemPrompt != "" { options.TransientSystemPrompt = runOptions.transientSystemPrompt @@ -5792,12 +5865,34 @@ func (m model) runAgentWithOptions(runID int, runCtx context.Context, prompt str rows = append(rows, row) m.sendAgentRow(runID, row) } - // Keep the latest plan state in sync for run details and step drill-in. - if result.Name == "update_plan" && m.registry != nil { - if planTool, ok := m.registry.Get("update_plan"); ok { - if reader, ok := planTool.(interface{ CurrentPlan() []tools.PlanItem }); ok { - if m.runtimeMessageSink != nil { - m.runtimeMessageSink(planUpdateMsg{runID: runID, items: reader.CurrentPlan()}) + // Keep the latest plan state in sync for run details and step + // drill-in. Only on a successful result: an errored call + // (including one refused because its run was already cancelled) + // must not re-read the shared plan and write it into this run's + // session file, which could clobber that file with a later + // session's state. + if result.Name == "update_plan" && result.Status == tools.StatusOK { + // Use the plan snapshot the successful call carried with its + // result, never a fresh CurrentPlan() read: this callback runs + // after update_plan released its mutex, so a cancel plus + // /new or /resume in that window can clear or hydrate the + // shared tool, and re-reading it here would persist the wrong + // session's plan (or an empty reset) under this run's session. + if items, ok := planSnapshotFromResult(result); ok { + if m.runtimeMessageSink != nil { + m.runtimeMessageSink(planUpdateMsg{runID: runID, items: items}) + } + // Persist every update_plan call to the durable plan store + // (under the user config directory, outside the workspace): + // it is the single source of truth /plan reads from, so a + // plan built entirely through update_plan still survives a + // restart/resume, and one seeded by /plan open keeps + // reflecting later agent updates. Storing outside the + // workspace keeps the tool's read-only / auto-allow + // contract honest: no workspace write grant is required. + if m.activeSession.SessionID != "" { + if _, err := planmode.WritePlan(m.cwd, m.activeSession.SessionID, formatPlanItems(items)); err != nil { + m.sendAgentRow(runID, transcriptRow{kind: rowError, text: "plan file write error: " + err.Error()}) } } } @@ -6043,8 +6138,11 @@ func toolResultSessionPayload(result agent.ToolResult) map[string]any { if result.Redacted { payload["redacted"] = true } - if len(result.Meta) > 0 { - payload["meta"] = result.Meta + // Strip plan_snapshot from session event meta: WritePlan (or the durable + // plan file) is the plan source of truth; embedding the full snapshot + // again would store the plan twice on disk. + if meta := sessionToolResultMeta(result.Meta); len(meta) > 0 { + payload["meta"] = meta } if len(result.ChangedFiles) > 0 { payload["changedFiles"] = result.ChangedFiles diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index b81f3a6cc..4efbaa47f 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2904,6 +2904,11 @@ func TestNextPermissionModeFoldsUnsafeToAsk(t *testing.T) { if got := nextPermissionMode(agent.PermissionModeUnsafe); got != agent.PermissionModeAsk { t.Fatalf("Unsafe -> %s, want Ask", got) } + // Plan mode is a deliberate read-only gate entered via /plan; a casual + // shift+tab toggle must be a no-op, not silently drop back to Ask. + if got := nextPermissionMode(agent.PermissionModePlan); got != agent.PermissionModePlan { + t.Fatalf("Plan -> %s, want Plan (no-op)", got) + } } func TestModelNotifierFocusAndCompletion(t *testing.T) { diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index cbe542c0d..ea603b8a6 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -2,100 +2,589 @@ package tui import ( "fmt" + "os" + "os/exec" + "regexp" + "runtime" "strings" + tea "charm.land/bubbletea/v2" + "mvdan.cc/sh/v3/shell" + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/tools" ) +// numberedStatusRe matches the "N. [status] " prefix that formatPlanItems +// writes, capturing the status so a user-edited plan file (seeded from that +// format) can be re-parsed back into plan items without losing progress. +var numberedStatusRe = regexp.MustCompile(`^\d+\.\s*(?:\[([^\]]*)\]\s*)?`) + 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)) { +// planItemsEqual reports whether two plan snapshots carry the same content. +// ID is deliberately excluded: parsePlanFileLines rebuilds items from the file +// text and does not preserve the in-memory IDs, so comparing them would report +// every reload as a change even when the user edited nothing. +func planItemsEqual(left, right []tools.PlanItem) bool { + if len(left) != len(right) { + return false + } + for index := range left { + if left[index].Content != right[index].Content || + left[index].Status != right[index].Status || + left[index].Notes != right[index].Notes { + return false + } + } + return true +} + +// planFileReloader syncs a user-edited plan file back into the in-memory plan. +// The in-memory update_plan is the execution source of truth; the file is its +// seed and on-disk target, so after /plan open the edited file is reloaded here. +type planFileReloader interface { + SetPlan([]tools.PlanItem) +} + +// handlePlanCommand manages the current session's plan mode: +// +// /plan show the current plan status +// /plan on enter read-only plan mode +// /plan open open the session's plan file in $VISUAL/$EDITOR +// /plan off exit plan mode (alias: /plan exit) +// +// Plan mode is read-only: tool advertisement (see +// tools.ToolAdvertisedForPermissionMode) only exposes read tools, +// update_plan, and ask_user, so the agent cannot mutate the workspace while +// planning. +func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { + arg := strings.ToLower(strings.TrimSpace(text)) + switch arg { case "", "status": - return m, m.planText() + if _, ok := m.registry.Get("update_plan"); !ok { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "No plan is active."}) + return m, nil + } + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.planText()}) + return m, nil case "on": - if m.pending { - return m, "Cannot change plan mode while a turn is active." + if m.pending || m.exiting { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "Cannot enter plan mode while a run is active."}) + return m, nil } if m.permissionMode == agent.PermissionModePlan { - return m, "Plan mode\nAlready active. Write and shell tools stay hidden until /plan off." + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode\nAlready active. Write and shell tools stay hidden until /plan off."}) + return m, nil } + updated, err := m.ensureActiveSession("") + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session error: " + err.Error()}) + return m, nil + } + m = updated 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." + reloadWarning := "" + if items, ok, reloadErr := m.reloadPlanFromFile(); reloadErr != nil { + reloadWarning = "\nplan reload error: " + reloadErr.Error() + } else if ok { + m.plan.updateFromItems(items, m.now()) + } + // Armed /loop ticks and /goal continuations cannot make progress + // while tools are read-only. Pause them here so the idle ticker and + // end-of-turn launcher do not spend tokens on no-op turns. + pausedLoops := 0 + m, pausedLoops = m.pauseLoopsForPlan() + if pausedLoops > 0 || m.hasArmedGoalContinuation() { + reloadWarning += "\nAutomatic /loop and /goal continuations are paused until /plan off." } + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode\n" + planEnterText(m) + reloadWarning}) + return m.syncPeerIdentity(), nil + case "off", "exit": if m.permissionMode != agent.PermissionModePlan { - return m, "Plan mode\nNot currently active." + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode\nNot currently active."}) + return m, nil + } + if m.pending || m.exiting { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "Cannot exit plan mode while a run is active. Press Esc to cancel it first."}) + return m, nil } - restored := m.permissionModeBeforePlan - if restored == "" { - restored = agent.PermissionModeAuto + m = m.exitPlanMode() + m = m.resumeLoopsAfterPlan() + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode\nExited. Permission mode restored to " + string(m.permissionMode) + "."}) + m, queuedCmd := m.launchQueuedMessageIfReady() + if queuedCmd != nil { + return m, queuedCmd } - m.permissionMode = restored - m.permissionModeBeforePlan = "" - return m, "Plan mode\nExited. Permission mode restored to " + string(restored) + "." + return m.launchGoalContinuationIfReady() + case "open": + if m.pending || m.exiting { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "Cannot open the plan file while a run is active."}) + return m, nil + } + if m.permissionMode != agent.PermissionModePlan { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Enter plan mode (/plan on) before opening the plan file."}) + return m, nil + } + updated, err := m.ensureActiveSession("") + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session error: " + err.Error()}) + return m, nil + } + return updated.openPlanInEditor() default: - return m, "Plan mode\nUsage: /plan [status|on|off]" + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: fmt.Sprintf("Unknown /plan subcommand %q. Usage: /plan [status|on|open|off]", arg)}) + return m, nil } } -// 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. +// planModeCommandUnavailable reports whether a local TUI command would mutate +// the workspace or start a host process outside the plan-mode tool gate. func planModeCommandUnavailable(command parsedCommand) bool { switch command.kind { - case commandRewind, commandExport, commandSandboxSetup, commandSpec, commandInit: + case commandRewind, commandExport, commandSandboxSetup, 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 } } +// planModeBlocksContinuations reports whether automatic /loop ticks and +// /goal continuations must stay idle. Plan mode cannot run implementation +// turns, so firing them would only burn tokens. +func (m model) planModeBlocksContinuations() bool { + return m.permissionMode == agent.PermissionModePlan +} + +// exitPlanMode restores the permission mode that was active before /plan +// entered plan mode. Shared by /plan off, the bare-/plan toggle, and session +// switches (/new, /resume), which must not leave a stale plan-mode grant (or a +// stale "restore to" mode) attached to a session other than the one that set it. +// When no prior mode was recorded (legacy / incomplete state), fall back to Ask +// rather than Auto so exit does not silently re-enable unrestricted tools. +func (m model) exitPlanMode() model { + if m.permissionMode == agent.PermissionModePlan { + if m.permissionModeBeforePlan != "" { + m.permissionMode = m.permissionModeBeforePlan + } else { + m.permissionMode = agent.PermissionModeAsk + } + } + m.permissionModeBeforePlan = "" + return m.syncPeerIdentity() +} + +// resetPlanForSessionSwitch clears the in-memory plan (both the update_plan +// tool's state and the sticky plan panel) so a session switch doesn't leak +// the previous session's plan into a session that never drafted it. Callers +// must also call exitPlanMode; unlike that call, a plain /plan off/toggle +// within the same session must NOT go through this path, since exiting plan +// mode there is exactly the hand-off into implementing the plan just drafted. +func (m model) resetPlanForSessionSwitch() model { + if writer, ok := m.registry.Get("update_plan"); ok { + if reloader, ok := writer.(planFileReloader); ok { + reloader.SetPlan(nil) + } + } + m.plan.clear() + return m +} + +// openPlanInEditor writes the session plan file (if missing) and suspends the +// TUI to launch $VISUAL/$EDITOR on it, resuming on exit. +func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { + if m.permissionMode != agent.PermissionModePlan { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Enter plan mode (/plan on) before opening the plan file."}) + return m, nil + } + path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID) + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan path error: " + err.Error()}) + return m, nil + } + _, exists, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID) + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan read error: " + err.Error()}) + return m, nil + } + if !exists { + // Seed the file with the agent's in-memory update_plan draft (if any) + // rather than leaving it blank: once the file exists, planText prefers + // it over the draft, so starting empty would shadow real plan content + // the agent already captured. + if _, err := planmode.WritePlan(m.cwd, m.activeSession.SessionID, m.formatPlanDraft()); err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan write error: " + err.Error()}) + return m, nil + } + } + editor := strings.TrimSpace(os.Getenv("VISUAL")) + if editor == "" { + editor = strings.TrimSpace(os.Getenv("EDITOR")) + } + if editor == "" { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Set $VISUAL or $EDITOR to open the plan file:\n" + path}) + return m, nil + } + // The editor is launched on a staged copy outside the workspace, not on + // path directly: see planmode.StageForEditor for why handing $EDITOR a + // workspace-relative path would leave a symlink-swap containment race. + stagedPath, cleanup, err := planmode.StageForEditor(m.cwd, m.activeSession.SessionID) + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan stage error: " + err.Error()}) + return m, nil + } + // $VISUAL/$EDITOR commonly quote an executable path containing spaces + // (e.g. `"/Applications/Visual Studio Code.app/.../code" --wait`); + // strings.Fields would split that mid-path. shell.Fields applies POSIX + // shell word-splitting, so quoted segments and any $VAR references in the + // value are handled the way a shell would. Unquoted Windows paths such as + // C:\Windows\notepad.exe must not go through POSIX escapes (backslash + // would drop path separators); see splitEditorCommand. + parts, err := splitEditorCommand(editor) + if err != nil || len(parts) == 0 { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "invalid $VISUAL/$EDITOR value: " + editor}) + cleanup() + return m, nil + } + cmd := exec.Command(parts[0], append(parts[1:], stagedPath)...) //nolint:gosec // editor path from $VISUAL/$EDITOR + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + workspaceRoot := m.cwd + sessionID := m.activeSession.SessionID + return m, tea.ExecProcess(cmd, func(err error) tea.Msg { + defer cleanup() + if err != nil { + return planEditorFinishedMsg{err: err} + } + if commitErr := planmode.CommitStagedEdit(workspaceRoot, sessionID, stagedPath); commitErr != nil { + return planEditorFinishedMsg{err: commitErr} + } + return planEditorFinishedMsg{err: nil} + }) +} + +// planEditorFinishedMsg reports a failed $VISUAL/$EDITOR run launched by +// /plan open so the transcript can surface it. +type planEditorFinishedMsg struct { + err error +} + +// splitEditorCommand parses $VISUAL/$EDITOR into argv. Quoted values and +// backslash-free values use POSIX shell.Fields (spaces inside quotes, $VAR +// expansion). Unquoted Windows commands containing backslashes keep the +// separators literal via windowsEditorFields so `C:\Windows\notepad.exe` (or a +// relative `.\tools\editor.exe`) is not mangled by POSIX escape processing. +func splitEditorCommand(editor string) ([]string, error) { + return splitEditorCommandFor(runtime.GOOS, editor) +} + +func splitEditorCommandFor(goos, editor string) ([]string, error) { + editor = strings.TrimSpace(editor) + if editor == "" { + return nil, fmt.Errorf("empty editor") + } + if goos == "windows" && strings.Contains(editor, `\`) && !isQuoteWrapped(editor) { + parts, err := windowsEditorFields(editor) + if err != nil { + return nil, err + } + if len(parts) == 0 { + return nil, fmt.Errorf("empty editor") + } + return parts, nil + } + return shell.Fields(editor, os.Getenv) +} + +// isQuoteWrapped reports whether the value is wrapped in a leading quote, in +// which case POSIX shell.Fields owns parsing (single quotes keep everything +// literal; double quotes preserve backslashes before ordinary characters). +func isQuoteWrapped(s string) bool { + return len(s) > 0 && (s[0] == '"' || s[0] == '\'') +} + +// windowsEditorFields splits a Windows command line with literal backslashes. +// Double-quoted segments keep internal spaces; outside quotes, whitespace splits. +func windowsEditorFields(s string) ([]string, error) { + var parts []string + var b strings.Builder + inQuote := false + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case c == '"': + inQuote = !inQuote + case (c == ' ' || c == '\t') && !inQuote: + if b.Len() > 0 { + parts = append(parts, b.String()) + b.Reset() + } + default: + b.WriteByte(c) + } + } + if inQuote { + return nil, fmt.Errorf("unterminated double quote") + } + if b.Len() > 0 { + parts = append(parts, b.String()) + } + return parts, nil +} + +// reloadPlanFromFile reads the session plan file (if any) and syncs its +// content into the in-memory update_plan, so edits the user makes in $EDITOR +// become the plan that drives execution. The file is only the on-disk target; +// the in-memory plan stays the source of truth. A missing file returns +// ok=false with a nil error (in-memory plan remains authoritative). A real +// ReadPlan failure (I/O, symlink refusal) returns the error so the editor +// round-trip can surface it instead of going silent. Returns the parsed items +// and true on success, so the caller can also refresh the sticky plan panel, +// which reloadPlanFromFile cannot do itself as a value-receiver method. +func (m model) reloadPlanFromFile() ([]tools.PlanItem, bool, error) { + content, ok, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID) + if err != nil { + return nil, false, err + } + if !ok { + return nil, false, nil + } + items := parsePlanFileLines(content) + if writer, ok := m.registry.Get("update_plan"); ok { + if reloader, ok := writer.(planFileReloader); ok { + reloader.SetPlan(items) + } + } + return items, true, nil +} + +// parsePlanFileLines converts the plain-text plan file the user edits in +// $EDITOR back into plan items. A numbered line ("N. [status] ...") starts a +// new step; an optional leading "[status]" is parsed back into the item's +// Status (matching formatPlanItems) so completed/in-progress steps survive an +// edit instead of resetting to pending. +// +// Indentation is authoritative and is decided BEFORE anything else: an +// indented line (formatPlanItems writes every continuation with a leading +// " ") always folds into the current item, even when its text happens to +// look like a numbered step ("2. validate") — deciding by content first +// would shatter such a continuation into a bogus new pending step. Within an +// item, the first indented "Notes: ..." line switches from Content to Notes; +// an indented continuation whose text itself begins with "Notes:" (or a +// backslash) is escaped by formatPlanItems with a leading backslash, which +// this parser strips, so real content is distinguishable from the notes +// delimiter. A whitespace-only indented line is a preserved blank +// continuation line; a fully blank line is a separator and is dropped. A +// non-numbered line with NO leading indentation is a freeform new step (e.g. +// one the user typed without bothering to number or indent it). +func parsePlanFileLines(content string) []tools.PlanItem { + items := make([]tools.PlanItem, 0) + inNotes := false + for _, raw := range strings.Split(content, "\n") { + raw = strings.TrimRight(raw, "\r") + trimmed := strings.TrimSpace(raw) + indented := len(raw) > 0 && (raw[0] == ' ' || raw[0] == '\t') + if !indented || len(items) == 0 { + if trimmed == "" { + continue + } + if match := numberedStatusRe.FindStringSubmatch(trimmed); match != nil { + status := "pending" + if match[1] != "" { + status = tools.NormalizePlanStatus(match[1]) + } + items = append(items, tools.PlanItem{ + Content: strings.TrimSpace(trimmed[len(match[0]):]), + Status: status, + }) + inNotes = false + continue + } + items = append(items, tools.PlanItem{Content: trimmed, Status: "pending"}) + inNotes = false + continue + } + var lineBody string + switch { + case strings.HasPrefix(raw, " "): + lineBody = raw[3:] + case strings.HasPrefix(raw, "\t"): + lineBody = raw[1:] + default: + lineBody = strings.TrimLeft(raw, " \t") + } + + last := &items[len(items)-1] + if !inNotes { + if notes, ok := strings.CutPrefix(strings.TrimSpace(lineBody), "Notes:"); ok { + last.Notes = strings.TrimSpace(notes) + inNotes = true + continue + } + } + line := unescapePlanContinuation(lineBody) + if inNotes { + if last.Notes == "" { + last.Notes = line + } else { + last.Notes += "\n" + line + } + continue + } + last.Content += "\n" + line + } + return items +} + +// escapePlanContinuation guards a continuation line whose literal text would +// otherwise be parsed as structure: a line beginning with "Notes:" (the notes +// delimiter) or with a backslash (the escape itself) gets one leading +// backslash, which unescapePlanContinuation strips on reload. +func escapePlanContinuation(line string) string { + if strings.HasPrefix(strings.TrimSpace(line), "Notes:") || strings.HasPrefix(line, `\`) { + return `\` + line + } + return line +} + +func unescapePlanContinuation(line string) string { + if strings.HasPrefix(line, `\`) { + return line[1:] + } + return line +} + +func planEnterText(m model) string { + planNote := "" + if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { + planNote = "\nPlan file: " + path + } + return "Active: read-only planning. Write and shell tools are hidden until /plan off." + planNote +} + func (m model) planText() string { + // Prefer the durable plan file when present. update_plan persists to the + // per-user plan store on every call (see model.go's OnToolResult hook), so + // it is the source of truth once anything has been captured; the in-memory + // draft below is only a fallback for a plan that predates any write. + path, pathErr := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID) + content, exists, readErr := planmode.ReadPlan(m.cwd, m.activeSession.SessionID) + if readErr != nil { + // A real I/O/permission failure, not just a not-yet-created file: + // surface it instead of silently falling back to the in-memory draft, + // which would hide the failure entirely. + return "plan file read error: " + readErr.Error() + } + + modeLabel := "inactive" + if m.permissionMode == agent.PermissionModePlan { + modeLabel = "active" + } + + if exists && strings.TrimSpace(content) != "" { + header := fmt.Sprintf("Current Plan (plan mode %s)", modeLabel) + if pathErr == nil { + header += "\n" + path + } + return header + "\n" + strings.TrimRight(content, "\n") + } + + // Fall back to the update_plan list the agent has been building. + if draft := m.formatPlanDraft(); strings.TrimSpace(draft) != "" { + return fmt.Sprintf("Current Plan (plan mode %s; draft in memory)\n%s", modeLabel, draft) + } + + if m.permissionMode == agent.PermissionModePlan { + return "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." + } + return "Plan mode is inactive. No plan written. Use /plan on to enter plan mode." +} + +// formatPlanDraft renders the agent's in-memory update_plan items as plain +// text, or "" if nothing has been captured yet. Shared by planText's fallback +// and openPlanInEditor's file-seeding so a newly created plan file starts from +// the agent's real draft instead of blank. +func (m model) formatPlanDraft() string { tool, ok := m.registry.Get("update_plan") if !ok { - return "No plan is active." + return "" } - reader, ok := tool.(currentPlanReader) if !ok { - return "No plan is active." + return "" } + return formatPlanItems(reader.CurrentPlan()) +} - plan := reader.CurrentPlan() - if len(plan) == 0 { - return "No plan is active." +// formatPlanItems renders update_plan items as plain text, or "" if there are +// none. Shared by formatPlanDraft (in-memory fallback for display) and the +// OnToolResult hook in model.go that persists every update_plan call to disk. +// +// A multi-line Content or Notes is rendered with each continuation line +// indented (" "), matching what parsePlanFileLines expects: it is the +// indentation, not just the "Notes:" marker, that tells a reload apart a +// continuation of the current item from a freeform new step. +func formatPlanItems(items []tools.PlanItem) string { + if len(items) == 0 { + return "" } - - lines := make([]string, 0, len(plan)+1) - lines = append(lines, "Current Plan") - for index, item := range plan { - line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, item.Content) + lines := make([]string, 0, len(items)) + for index, item := range items { + contentLines := strings.Split(item.Content, "\n") + line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, contentLines[0]) + // Continuations are indented (which is what makes them continuations + // to parsePlanFileLines, even when the text looks like "2. validate") + // and escaped where their literal text would read as structure. + for _, cont := range contentLines[1:] { + line += "\n " + escapePlanContinuation(cont) + } if item.Notes != "" { - line += "\n Notes: " + item.Notes + noteLines := strings.Split(item.Notes, "\n") + line += "\n Notes: " + noteLines[0] + for _, cont := range noteLines[1:] { + line += "\n " + escapePlanContinuation(cont) + } } lines = append(lines, line) } return strings.Join(lines, "\n") } + +// planSnapshotFromResult extracts the immutable plan items a successful update_plan +// call carried in its typed PlanSnapshot field. ok=false when the snapshot is +// absent or empty — the caller then skips panel/file updates rather than re-reading +// the shared tool, whose state may already belong to another session by the time +// the result callback runs. +func planSnapshotFromResult(result agent.ToolResult) ([]tools.PlanItem, bool) { + if len(result.PlanSnapshot) > 0 { + return append([]tools.PlanItem{}, result.PlanSnapshot...), true + } + return nil, false +} + +// sessionToolResultMeta copies result.Meta for session event logging, omitting +// PlanSnapshotMeta if present so the plan body is not persisted twice (durable plan file +// plus event log). +func sessionToolResultMeta(meta map[string]string) map[string]string { + if len(meta) == 0 { + return nil + } + out := make(map[string]string, len(meta)) + for k, v := range meta { + if k == tools.PlanSnapshotMeta { + continue + } + out[k] = v + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go new file mode 100644 index 000000000..be1fb0eb6 --- /dev/null +++ b/internal/tui/plan_command_test.go @@ -0,0 +1,1199 @@ +package tui + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/peermsg" + "github.com/Gitlawb/zero/internal/planmode" + "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// isolatePlanConfig redirects user config/cache/profile directories so durable +// plan files and editor staging land under a throwaway directory. +func isolatePlanConfig(t *testing.T) { + t.Helper() + root := t.TempDir() + configDir := filepath.Join(root, "config") + tempDir := filepath.Join(root, "tmp") + _ = os.MkdirAll(configDir, 0o700) + _ = os.MkdirAll(tempDir, 0o700) + + t.Setenv("HOME", root) + t.Setenv("USERPROFILE", root) + t.Setenv("XDG_CONFIG_HOME", configDir) + t.Setenv("XDG_CACHE_HOME", filepath.Join(root, "cache")) + t.Setenv("AppData", configDir) + t.Setenv("LocalAppData", filepath.Join(root, "local")) + restore := planmode.SetEffectiveTempDirForTest(tempDir) + t.Cleanup(restore) +} + +func newPlanCommandTestModel(t *testing.T, cwd string, permissionMode agent.PermissionMode) model { + t.Helper() + isolatePlanConfig(t) + registry := tools.NewRegistry() + registry.Register(tools.NewUpdatePlanTool()) + m := newModel(context.Background(), Options{ + Cwd: cwd, + ProviderName: "openai", + ModelName: "gpt-4.1", + Provider: &fakeProvider{}, + Registry: registry, + PermissionMode: permissionMode, + }) + m.activeSession = sessions.Metadata{SessionID: "plan-test-session"} + return m +} + +func TestHandlePlanCommandSyncsPeerIdentityOnEnterAndExit(t *testing.T) { + isolatePlanConfig(t) + svc, err := peermsg.New(peermsg.Options{ + RootDir: t.TempDir(), + Identity: peermsg.Identity{ + Name: "zero", + Cwd: t.TempDir(), + PermissionClass: peermsg.PermissionBypass, + }, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := svc.Start(func(peermsg.InboundMessage) bool { return true }); err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { _ = svc.Close() }) + + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModeUnsafe) + m.peerService = svc + + updated, _ := m.handlePlanCommand("on") + next := updated.(model) + if got := next.peerService.Self().PermissionClass; got != peermsg.PermissionPrompting { + t.Fatalf("after /plan on PermissionClass = %q, want %q", got, peermsg.PermissionPrompting) + } + + updated, _ = next.handlePlanCommand("off") + next = updated.(model) + if got := next.peerService.Self().PermissionClass; got != peermsg.PermissionBypass { + t.Fatalf("after /plan off PermissionClass = %q, want %q", got, peermsg.PermissionBypass) + } +} + +func TestShiftTabDoesNotExitPlanMode(t *testing.T) { + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected /plan to enter plan mode, got %s", next.permissionMode) + } + + updated, _ = next.Update(testKeyShift(tea.KeyTab)) + next = updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected shift+tab to leave plan mode untouched, got %s", next.permissionMode) + } +} + +func TestPlanOffRestoresPreviousPermissionMode(t *testing.T) { + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected /plan to enter plan mode, got %s", next.permissionMode) + } + + next.input.SetValue("/plan off") + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected /plan off to restore the prior Ask mode, got %s", next.permissionMode) + } +} + +func TestPlanOpenOutsidePlanModeDoesNotCreateSession(t *testing.T) { + // Regression: /plan open when plan mode is inactive used to call + // ensureActiveSession before openPlanInEditor's own guard rejected the + // command, leaving a persistent empty session behind in /resume for what + // should have been a pure no-op error. + store := testSessionStore(t) + m := newModel(context.Background(), Options{ + Cwd: t.TempDir(), + SessionStore: store, + PermissionMode: agent.PermissionModeAsk, + }) + registry := tools.NewRegistry() + registry.Register(tools.NewUpdatePlanTool()) + m.registry = registry + + updated, _ := m.handlePlanCommand("open") + next := updated.(model) + if next.activeSession.SessionID != "" { + t.Fatalf("expected no session to be created for an invalid /plan open, got %+v", next.activeSession) + } + if !transcriptContains(next.transcript, "Enter plan mode (/plan on) before opening the plan file.") { + t.Fatalf("expected a plan-mode-required notice in the transcript, got %#v", next.transcript) + } +} + +func TestPlanOpenBlockedWhileRunActive(t *testing.T) { + // Regression: the bare /plan toggle refused to run while m.pending (a run + // in flight), but "/plan open" had no such guard, letting it race a live + // run to suspend the TUI into $EDITOR. + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModePlan) + m.pending = true + + updated, cmd := m.handlePlanCommand("open") + next := updated.(model) + if cmd != nil { + t.Fatal("expected /plan open to return no command while a run is active") + } + if !transcriptContains(next.transcript, "Cannot open the plan file while a run is active") { + t.Fatalf("expected a blocked-run notice in the transcript, got %#v", next.transcript) + } +} + +func TestPlanOffBlockedWhileRunActive(t *testing.T) { + // Mid-run /plan off would flip permissionMode before agentResponseMsg, + // so completeRemaining would mark every plan step completed for a + // planning turn. Exit must wait for the run to finish (or cancel). + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModePlan) + m.pending = true + + updated, cmd := m.handlePlanCommand("off") + next := updated.(model) + if cmd != nil { + t.Fatal("expected /plan off to return no command while a run is active") + } + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected plan mode preserved while run pending, got %s", next.permissionMode) + } + if !transcriptContains(next.transcript, "Cannot exit plan mode while a run is active") { + t.Fatalf("expected a blocked-exit notice in the transcript, got %#v", next.transcript) + } + +} + +func TestSplitEditorCommandWindowsPaths(t *testing.T) { + // shell.Fields treats unquoted backslash as a POSIX escape, so + // C:\Windows\notepad.exe becomes C:Windowsnotepad.exe. Windows-style + // absolute paths must keep separators literal. + parts, err := splitEditorCommandFor("windows", `C:\Windows\System32\notepad.exe`) + if err != nil { + t.Fatalf("split unquoted drive path: %v", err) + } + if len(parts) != 1 || parts[0] != `C:\Windows\System32\notepad.exe` { + t.Fatalf("unquoted Windows path: got %#v", parts) + } + + parts, err = splitEditorCommandFor("windows", `"C:\Program Files\Git\bin\vim.exe" --wait`) + if err != nil { + t.Fatalf("split quoted Windows path: %v", err) + } + if len(parts) != 2 || parts[0] != `C:\Program Files\Git\bin\vim.exe` || parts[1] != "--wait" { + t.Fatalf("quoted Windows path with args: got %#v", parts) + } + + // Quoted Unix paths still use POSIX shell.Fields (spaces preserved). + parts, err = splitEditorCommandFor("linux", `"/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code" --wait`) + if err != nil { + t.Fatalf("split quoted Unix path: %v", err) + } + if len(parts) != 2 || !strings.Contains(parts[0], "Visual Studio Code") || parts[1] != "--wait" { + t.Fatalf("quoted Unix path: got %#v", parts) + } + + // Unquoted simple command on any OS. + parts, err = splitEditorCommandFor("linux", "code --wait") + if err != nil { + t.Fatalf("split simple command: %v", err) + } + if len(parts) != 2 || parts[0] != "code" || parts[1] != "--wait" { + t.Fatalf("simple command: got %#v", parts) + } + + // Regression: a Windows command containing backslashes but not beginning + // with a drive or UNC path (e.g. a relative .\tools\editor.exe) used to + // fall through to shell.Fields, which drops the separators as POSIX + // escapes. It must keep backslashes literal too. + parts, err = splitEditorCommandFor("windows", `.\tools\editor.exe --wait`) + if err != nil { + t.Fatalf("split relative Windows path: %v", err) + } + if len(parts) != 2 || parts[0] != `.\tools\editor.exe` || parts[1] != "--wait" { + t.Fatalf("relative Windows path: got %#v", parts) + } + + _, err = splitEditorCommandFor("windows", `"C:\Program Files\editor.exe --wait`) + if err == nil { + t.Fatal("expected unterminated Windows quote to fail") + } + + // Single-quoted values still go through POSIX shell.Fields (literal + // content, backslashes preserved), matching the quoted-path contract. + parts, err = splitEditorCommandFor("windows", `'C:\Program Files\editor.exe' --wait`) + if err != nil { + t.Fatalf("split single-quoted Windows path: %v", err) + } + if len(parts) != 2 || parts[0] != `C:\Program Files\editor.exe` || parts[1] != "--wait" { + t.Fatalf("single-quoted Windows path: got %#v", parts) + } +} + +func TestBarePlanReportsStatusWithoutExiting(t *testing.T) { + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected /plan on to enter plan mode, got %s", next.permissionMode) + } + + next.input.SetValue("/plan") + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected bare /plan to preserve plan mode, got %s", next.permissionMode) + } +} +func TestPlanOpenCreatesSessionBeforeWritingPlanFile(t *testing.T) { + // Regression: on a fresh TUI (or after /new) the session ID is empty + // until the first prompt lazily creates it. /plan open must create the + // session before writing its plan file so fresh sessions do not share the + // empty-session plan path. + isolatePlanConfig(t) + registry := tools.NewRegistry() + registry.Register(tools.NewUpdatePlanTool()) + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: testSessionStore(t), + Registry: registry, + }) + if m.activeSession.SessionID != "" { + t.Fatal("setup: expected a fresh model to have no active session") + } + + t.Setenv("VISUAL", "") + t.Setenv("EDITOR", "") + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + next.input.SetValue("/plan open") + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + + if next.activeSession.SessionID == "" { + t.Fatal("expected /plan open to create a session before writing the plan file") + } + path, err := planmode.PlanFilePath(cwd, next.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("expected plan file for the active session: %v", err) + } +} + +// TestPlanOnCreatesSessionAndNamesItsPlanFile covers plan-mode entry on its +// own, without the /plan open that follows it in the test above. On a fresh +// TUI (or after /new) the session ID is empty until the first prompt lazily +// creates it, and PlanFilePath maps an empty ID onto a single shared +// no-session slug. Entering plan mode must create the session first, so the +// banner names that session's own plan file rather than the shared fallback +// that every other fresh session would also resolve to. +func TestPlanOnCreatesSessionAndNamesItsPlanFile(t *testing.T) { + isolatePlanConfig(t) + registry := tools.NewRegistry() + registry.Register(tools.NewUpdatePlanTool()) + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: testSessionStore(t), + Registry: registry, + }) + if m.activeSession.SessionID != "" { + t.Fatal("setup: expected a fresh model to have no active session") + } + + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + + if next.activeSession.SessionID == "" { + t.Fatal("expected /plan on to create a session before entering plan mode") + } + path, err := planmode.PlanFilePath(cwd, next.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if !transcriptContains(next.transcript, path) { + t.Fatalf("expected the plan-entry banner to name the real session's plan file %q, got %#v", path, next.transcript) + } + // The shared no-session path must never be what the user is pointed at. + fallback, err := planmode.PlanFilePath(cwd, "") + if err != nil { + t.Fatalf("PlanFilePath(empty): %v", err) + } + if transcriptContains(next.transcript, fallback) { + t.Fatalf("plan-entry banner named the shared no-session plan file %q", fallback) + } +} + +func TestPlanOpenLaunchesEditorCommand(t *testing.T) { + // Regression for the model being copied by value into tea.NewProgram + // before the (now-removed) m.program field was assigned in run.go: /plan + // open always took the "no live program" fallback and never actually + // suspended the TUI to run $EDITOR. + t.Setenv("EDITOR", "true") + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModePlan) + + m.input.SetValue("/plan open") + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + + if cmd == nil { + t.Fatal("expected /plan open to return a command that launches $EDITOR") + } + if transcriptContains(next.transcript, "Plan file:") { + t.Fatalf("expected the editor to be launched instead of just reporting the path: %#v", next.transcript) + } +} + +func TestPlanOpenSeedsFileFromDraft(t *testing.T) { + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + result := planTool.Run(context.Background(), map[string]any{ + "plan": []any{ + map[string]any{"content": "Wire model catalog", "status": "completed"}, + }, + }) + if result.Status != tools.StatusOK { + t.Fatalf("update_plan setup failed: %#v", result) + } + registry.Register(planTool) + + // File seeding happens before the $VISUAL/$EDITOR check, so it must not + // depend on an editor being configured; unset both explicitly so this test + // doesn't depend on (or shell out to) whatever the host environment has set. + t.Setenv("VISUAL", "") + t.Setenv("EDITOR", "") + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m.activeSession = sessions.Metadata{SessionID: "plan-test-session"} + + m.input.SetValue("/plan open") + m.Update(testKey(tea.KeyEnter)) + + path, err := planmode.PlanFilePath(cwd, "plan-test-session") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("expected the plan file to be created, got: %v", err) + } + if !strings.Contains(string(content), "Wire model catalog") { + t.Fatalf("expected the new plan file to be seeded with the update_plan draft, got: %q", content) + } +} + +func TestUpdatePlanPersistsToPlanFile(t *testing.T) { + // Regression: update_plan only updated the in-memory tool, so a plan built + // entirely through the agent's prescribed workflow (the user never ran + // /plan open) disappeared on restart/resume, and a plan file seeded once + // by /plan open never reflected later update_plan calls. The plan file + // must be the durable source of truth, refreshed on every update_plan call. + // It must also stay outside the workspace so the read-only auto-allow + // contract remains honest. + isolatePlanConfig(t) + store := testSessionStore(t) + cwd := t.TempDir() + provider := &scriptedProvider{scripts: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call_1", ToolName: "update_plan"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call_1", ArgumentsFragment: `{"plan":[{"content":"Wire model catalog","status":"in_progress"}]}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call_1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "planned"}, + {Type: zeroruntime.StreamEventDone}, + }, + }} + registry := tools.NewRegistry() + registry.Register(tools.NewUpdatePlanTool()) + m := newModel(context.Background(), Options{ + Cwd: cwd, + ProviderName: "openai", + ModelName: "gpt-4.1", + Provider: provider, + Registry: registry, + SessionStore: store, + }) + m.input.SetValue("outline the approach") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd == nil { + t.Fatal("expected prompt submit to start an agent run") + } + updated, _ = next.Update(execCmd(cmd)) + next = updated.(model) + + if next.activeSession.SessionID == "" { + t.Fatal("expected the run to create a session") + } + content, ok, err := planmode.ReadPlan(cwd, next.activeSession.SessionID) + if err != nil { + t.Fatalf("ReadPlan: %v", err) + } + if !ok { + t.Fatal("expected update_plan to persist a plan file") + } + if !strings.Contains(content, "Wire model catalog") { + t.Fatalf("expected the persisted plan file to reflect the update_plan call, got: %q", content) + } + path, err := planmode.PlanFilePath(cwd, next.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + // Canonicalize both sides: on macOS t.TempDir() is under /var while + // resolved paths live under /private/var, so a raw HasPrefix check can + // pass even when the plan file is inside the workspace. + resolvedCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + t.Fatalf("EvalSymlinks cwd: %v", err) + } + // Plan path itself may not exist yet on a pure path check; resolve the + // deepest existing ancestor (the plans root or its parent) via Dir. + resolvedPlanDir, err := filepath.EvalSymlinks(filepath.Dir(path)) + if err != nil { + // Fall back to physicalPath-style resolve of the parent only when the + // plan dir was never created (ReadPlan above already confirmed it exists). + t.Fatalf("EvalSymlinks plan dir: %v", err) + } + if resolvedPlanDir == resolvedCwd || strings.HasPrefix(resolvedPlanDir, resolvedCwd+string(os.PathSeparator)) { + t.Fatalf("durable plan path %q must not live under the workspace %q", path, cwd) + } + if _, err := os.Stat(filepath.Join(cwd, ".zero")); !os.IsNotExist(err) { + t.Fatalf("update_plan must not create .zero under the workspace, stat err=%v", err) + } +} + +func TestPlanOpenEditorExitReloadsFileIntoPlan(t *testing.T) { + // After /plan open edits the plan file in $EDITOR, the edited content + // must be reloaded into the in-memory update_plan so it drives + // execution, rather than being shadowed. + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + registry.Register(planTool) + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m.activeSession = sessions.Metadata{SessionID: "plan-test-session"} + + path, err := planmode.PlanFilePath(cwd, "plan-test-session") + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if _, err := planmode.WritePlan(cwd, "plan-test-session", "1. [pending] original step"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + // Simulate the editor exiting after the user rewrote the file. + if err := os.WriteFile(path, []byte("edited first step\nedited second step\n"), 0o600); err != nil { + t.Fatalf("rewrite plan file: %v", err) + } + if _, _, err := m.reloadPlanFromFile(); err != nil { + t.Fatalf("reloadPlanFromFile: %v", err) + } + + got := planTool.CurrentPlan() + if len(got) != 2 { + t.Fatalf("expected 2 reloaded plan items, got %d: %+v", len(got), got) + } + if got[0].Content != "edited first step" || got[1].Content != "edited second step" { + t.Fatalf("expected edited contents reloaded, got %+v", got) + } +} + +func TestPlanEditorFinishedMsgReloadsPanelAndConfirms(t *testing.T) { + // The editor-completion path must run through the real planEditorFinishedMsg + // case in Update (not just reloadPlanFromFile, which tests can call + // directly): it reloads the edited file into BOTH the update_plan tool (the + // execution source of truth) and the sticky panel, and confirms the reload + // in the transcript so a bare /plan open doesn't look like a silent no-op. + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + registry.Register(planTool) + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: testSessionStore(t), + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m, err := m.ensureActiveSession("plan editor completion") + if err != nil { + t.Fatalf("ensureActiveSession: %v", err) + } + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, "1. [in_progress] edited step\n Notes: from editor"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + updated, _ := m.Update(planEditorFinishedMsg{err: nil}) + next := updated.(model) + + // update_plan (what drives execution) reflects the edited file. + got := planTool.CurrentPlan() + if len(got) != 1 || got[0].Content != "edited step" || got[0].Status != "in_progress" { + t.Fatalf("expected update_plan reloaded from the edited file, got %+v", got) + } + // The sticky panel was refreshed too, not just the tool state. + if next.plan.isEmpty() { + t.Fatal("expected the sticky plan panel to be refreshed from the reloaded file") + } + // A completion message reaches the transcript. + if !transcriptContains(next.transcript, "Reloaded the edited plan.") { + t.Fatalf("expected an editor-reload completion message, got %#v", next.transcript) + } +} + +// TestPlanEditorFinishedMsgNoOpEditRecordsNothing covers quitting $EDITOR +// without changing anything. The session event the handler writes is phrased as +// the user's own words ("I edited the plan file directly"), so recording it for +// an untouched file puts a false statement into the next turn's context, and +// repeated opens would each restate the whole plan into the session log. +func TestPlanEditorFinishedMsgNoOpEditRecordsNothing(t *testing.T) { + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + registry.Register(planTool) + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: testSessionStore(t), + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m, err := m.ensureActiveSession("plan editor no-op") + if err != nil { + t.Fatalf("ensureActiveSession: %v", err) + } + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, "1. [in_progress] untouched step\n Notes: keep"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + // Load that plan in, so the tool state already matches the file exactly: + // the editor opened it and quit without saving a change. + if _, _, err := m.reloadPlanFromFile(); err != nil { + t.Fatalf("reloadPlanFromFile: %v", err) + } + eventsBefore := len(m.sessionEvents) + + updated, _ := m.Update(planEditorFinishedMsg{err: nil}) + next := updated.(model) + + if len(next.sessionEvents) != eventsBefore { + t.Fatalf("an unchanged plan file must not record a session event: before=%d after=%d", eventsBefore, len(next.sessionEvents)) + } + if transcriptContains(next.transcript, "Reloaded the edited plan.") { + t.Fatalf("an unchanged plan file must not claim a reload, got %#v", next.transcript) + } + // The plan itself must survive untouched. + if got := planTool.CurrentPlan(); len(got) != 1 || got[0].Content != "untouched step" || got[0].Status != "in_progress" { + t.Fatalf("no-op edit changed the plan: %+v", got) + } +} + +func TestPlanEditorFinishedMsgReloadErrorSurfaces(t *testing.T) { + // Failure path: if ReadPlan fails after the editor exits (e.g. the durable + // plan file was deleted or became unreadable), the reload error must surface + // in the transcript instead of failing silently. + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + registry.Register(planTool) + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: testSessionStore(t), + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m, err := m.ensureActiveSession("plan editor completion failure") + if err != nil { + t.Fatalf("ensureActiveSession: %v", err) + } + // Write a plan file, then replace it with a directory at the same path so + // ReadPlan fails (refused as a non-regular file) between editor exit and + // reload. A plain deletion would not do: ReadPlan treats a missing file as + // ok=false, not an error, so the reload would silently no-op instead of + // surfacing a failure. + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, "1. [in_progress] step"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + path, err := planmode.PlanFilePath(cwd, m.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if err := os.Remove(path); err != nil { + t.Fatalf("remove plan file: %v", err) + } + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("replace plan file with directory: %v", err) + } + + // Simulate editor completion with the plan file now missing + updated, _ := m.Update(planEditorFinishedMsg{err: nil}) + next := updated.(model) + + // The reload error should appear in the transcript + if !transcriptContains(next.transcript, "plan reload error:") { + t.Fatalf("expected a plan reload error message in transcript, got %#v", next.transcript) + } +} + +func TestPlanOnReloadErrorPreservesExistingPlan(t *testing.T) { + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "in-memory step", Status: "pending"}}) + registry.Register(planTool) + + cwd := t.TempDir() + store := testSessionStore(t) + m := newModel(context.Background(), Options{ + Cwd: cwd, + SessionStore: store, + Registry: registry, + }) + m, err := m.ensureActiveSession("plan reload failure test") + if err != nil { + t.Fatalf("ensureActiveSession: %v", err) + } + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + + if _, err := planmode.WritePlan(cwd, m.activeSession.SessionID, "1. [pending] on disk"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + path, err := planmode.PlanFilePath(cwd, m.activeSession.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if err := os.Remove(path); err != nil { + t.Fatalf("remove plan file: %v", err) + } + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("replace plan file with directory: %v", err) + } + + updated, _ := m.handlePlanCommand("on") + next := updated.(model) + if len(planTool.CurrentPlan()) != 1 || planTool.CurrentPlan()[0].Content != "in-memory step" { + t.Fatalf("expected in-memory plan preserved after /plan on reload error, got %+v", planTool.CurrentPlan()) + } + if next.plan.isEmpty() { + t.Fatal("expected sticky plan panel preserved after /plan on reload error") + } + if !transcriptContains(next.transcript, "plan reload error:") { + t.Fatalf("expected a plan reload error message in transcript, got %#v", next.transcript) + } +} + +func TestPlanOpenEditorReloadPreservesStatusAndNotes(t *testing.T) { + // Regression: parsePlanFileLines used to discard the "[status]" bracket + // (resetting every reloaded item to "pending") and treat a "Notes: ..." + // continuation line as its own bogus plan item instead of folding it + // into the preceding step. + isolatePlanConfig(t) + registry := tools.NewRegistry() + planTool := tools.NewUpdatePlanTool() + registry.Register(planTool) + + cwd := t.TempDir() + m := newModel(context.Background(), Options{ + Cwd: cwd, + Registry: registry, + PermissionMode: agent.PermissionModePlan, + }) + m.activeSession = sessions.Metadata{SessionID: "plan-test-session"} + + content := "1. [completed] step one\n2. [in_progress] step two\n Notes: half done\n3. [pending] step three" + if _, err := planmode.WritePlan(cwd, "plan-test-session", content); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + if _, _, err := m.reloadPlanFromFile(); err != nil { + t.Fatalf("reloadPlanFromFile: %v", err) + } + + got := planTool.CurrentPlan() + if len(got) != 3 { + t.Fatalf("expected 3 plan items (no bogus 'Notes' item), got %d: %+v", len(got), got) + } + if got[0].Status != "completed" { + t.Fatalf("expected step one to stay completed, got %q", got[0].Status) + } + if got[1].Status != "in_progress" || got[1].Notes != "half done" { + t.Fatalf("expected step two to stay in_progress with notes preserved, got status=%q notes=%q", got[1].Status, got[1].Notes) + } + if got[2].Status != "pending" || got[2].Content != "step three" { + t.Fatalf("expected step three unchanged, got %+v", got[2]) + } +} + +func TestPlanItemsRoundTripMultilineContent(t *testing.T) { + // Regression: a multi-line PlanItem.Content (e.g. from an agent-authored + // update_plan call) used to be written verbatim by formatPlanItems, and + // its continuation lines then reloaded as bogus new freeform pending + // steps instead of staying part of the original item's Content. + items := []tools.PlanItem{ + {Content: "first line\nsecond line\nthird line", Status: "in_progress", Notes: "a note\nsecond note line"}, + {Content: "step two", Status: "pending"}, + } + reloaded := parsePlanFileLines(formatPlanItems(items)) + if len(reloaded) != 2 { + t.Fatalf("expected 2 items after round-trip, got %d: %+v", len(reloaded), reloaded) + } + if reloaded[0].Content != items[0].Content { + t.Fatalf("expected multi-line content preserved, got %q", reloaded[0].Content) + } + if reloaded[0].Status != "in_progress" || reloaded[0].Notes != items[0].Notes { + t.Fatalf("expected status/notes preserved, got %+v", reloaded[0]) + } + if reloaded[1].Content != "step two" { + t.Fatalf("expected step two unaffected, got %+v", reloaded[1]) + } +} + +func TestPlanItemsRoundTripAmbiguousContinuations(t *testing.T) { + // Regression for the encoding ambiguities that silently rewrote plans on + // an open-and-save: a continuation that looks like a numbered step used + // to shatter into a new item, a continuation beginning "Notes:" used to + // become the notes delimiter, and blank continuation lines vanished. + items := []tools.PlanItem{ + {Content: "Investigate\n2. validate", Status: "pending"}, + {Content: "Header\nNotes: literal content line", Status: "pending", Notes: "real note"}, + {Content: "before\n\nafter", Status: "pending"}, + {Content: "escape\n\\Notes: already escaped", Status: "pending"}, + } + reloaded := parsePlanFileLines(formatPlanItems(items)) + if len(reloaded) != len(items) { + t.Fatalf("expected %d items after round-trip, got %d: %+v", len(items), len(reloaded), reloaded) + } + for index := range items { + if reloaded[index].Content != items[index].Content { + t.Fatalf("item %d content changed on round-trip: %q -> %q", index, items[index].Content, reloaded[index].Content) + } + if reloaded[index].Notes != items[index].Notes { + t.Fatalf("item %d notes changed on round-trip: %q -> %q", index, items[index].Notes, reloaded[index].Notes) + } + } + // A second pass must be a fixed point: open-and-save twice changes nothing. + again := parsePlanFileLines(formatPlanItems(reloaded)) + if len(again) != len(reloaded) { + t.Fatalf("second round-trip changed item count: %d -> %d", len(reloaded), len(again)) + } + for index := range reloaded { + if again[index] != reloaded[index] { + t.Fatalf("second round-trip changed item %d: %+v -> %+v", index, reloaded[index], again[index]) + } + } +} + +func TestParsePlanFileLinesFoldsMultilineNotes(t *testing.T) { + // Regression: a "Notes: ..." block spanning more than one line used to + // have its continuation lines treated as bogus new pending steps instead + // of folding into the preceding item's Notes. + content := "1. [in_progress] step one\n" + + " Notes: first line\n" + + " second line continuation\n" + + "2. [pending] step two\n" + + "a freeform unnumbered line" + + items := parsePlanFileLines(content) + if len(items) != 3 { + t.Fatalf("expected 3 items (2 numbered steps + 1 freeform), got %d: %+v", len(items), items) + } + if items[0].Notes != "first line\nsecond line continuation" { + t.Fatalf("expected multi-line notes folded, got %q", items[0].Notes) + } + if items[1].Content != "step two" || items[1].Notes != "" { + t.Fatalf("expected step two unaffected, got %+v", items[1]) + } + if items[2].Content != "a freeform unnumbered line" || items[2].Status != "pending" { + t.Fatalf("expected a trailing unnumbered line to become its own step, got %+v", items[2]) + } +} + +func TestPlanModeWiresDraftSystemPrompt(t *testing.T) { + provider := &fakeProvider{events: []zeroruntime.StreamEvent{ + {Type: zeroruntime.StreamEventText, Content: "planning"}, + {Type: zeroruntime.StreamEventDone}, + }} + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModePlan) + // Embedders set product policy via agentOptions.SystemPrompt. Plan mode + // must layer its restriction onto that prompt rather than replace it. + const configuredPrompt = "Custom product policy for this embedder." + m.agentOptions.SystemPrompt = configuredPrompt + m.provider = provider + m.input.SetValue("outline the approach") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd == nil { + t.Fatal("expected submitting a prompt in plan mode to start an agent run") + } + updated, _ = next.Update(execCmd(cmd)) + _ = updated.(model) + + if len(provider.requests) != 1 { + t.Fatalf("expected one provider request, got %d", len(provider.requests)) + } + if len(provider.requests[0].Messages) == 0 { + t.Fatal("expected provider request to include a system message") + } + systemPrompt := provider.requests[0].Messages[0].Content + if !strings.Contains(systemPrompt, "Plan mode is active on this session") { + t.Fatalf("expected planmode.DraftSystemPrompt to be wired in, got:\n%s", systemPrompt) + } + if !strings.Contains(systemPrompt, configuredPrompt) { + t.Fatalf("expected configured SystemPrompt to be preserved under plan mode, got:\n%s", systemPrompt) + } + if !strings.HasPrefix(systemPrompt, configuredPrompt) { + t.Fatalf("expected plan-mode layer to follow the configured prompt, got:\n%s", systemPrompt) + } +} + +// Regression: when permissionModeBeforePlan is empty (legacy/incomplete state), +// exitPlanMode must fall back to Ask, not Auto, so leaving plan mode does not +// silently re-enable unrestricted tools. +func TestExitPlanModeFallsBackToAsk(t *testing.T) { + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModePlan) + m.permissionModeBeforePlan = "" + + next := m.exitPlanMode() + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected empty permissionModeBeforePlan to fall back to Ask, got %s", next.permissionMode) + } + if next.permissionModeBeforePlan != "" { + t.Fatalf("expected permissionModeBeforePlan to be cleared, got %q", next.permissionModeBeforePlan) + } +} + +func TestSessionToolResultMetaStripsPlanSnapshot(t *testing.T) { + meta := map[string]string{ + tools.PlanSnapshotMeta: `[{"content":"step","status":"pending"}]`, + "other": "keep", + } + got := sessionToolResultMeta(meta) + if _, ok := got[tools.PlanSnapshotMeta]; ok { + t.Fatalf("expected plan_snapshot stripped from session meta, got %#v", got) + } + if got["other"] != "keep" { + t.Fatalf("expected other meta keys preserved, got %#v", got) + } + if sessionToolResultMeta(map[string]string{tools.PlanSnapshotMeta: "x"}) != nil { + t.Fatal("expected nil when only plan_snapshot was present") + } + if sessionToolResultMeta(nil) != nil { + t.Fatal("expected nil for empty meta") + } +} + +// Regression: entering plan mode must pause armed /loop ticks and /goal +// continuations so they do not fire read-only turns that cannot make +// progress. /plan off unpauses loops and may resume an active goal. +func TestPlanCommandPausesArmedContinuations(t *testing.T) { + store := testSessionStore(t) + session, err := store.Create(sessions.CreateInput{SessionID: "plan_pause", Title: "plan pause", Cwd: t.TempDir()}) + if err != nil { + t.Fatal(err) + } + session, _, err = store.CreateGoal(session.SessionID, "Keep shipping", 0) + if err != nil { + t.Fatal(err) + } + m := newPlanCommandTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.sessionStore = store + m.activeSession = session + m.provider = &scriptedProvider{} + m = startFixedLoop(m, "keep shipping", time.Minute) + + updated, cmd := m.handlePlanCommand("on") + next := updated.(model) + if cmd != nil { + t.Fatal("expected /plan on to be synchronous") + } + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected plan mode, got %s", next.permissionMode) + } + if len(next.loops) != 1 || !next.loops[0].paused { + t.Fatalf("expected the armed loop to be paused in plan mode, got %+v", next.loops) + } + if !transcriptContains(next.transcript, "Automatic /loop and /goal continuations are paused") { + t.Fatalf("expected a pause notice, got %#v", next.transcript) + } + idle, fireCmd := next.fireDueLoopIfIdle() + if fireCmd != nil || idle.activeLoopID != "" { + t.Fatal("paused loop must not fire while plan mode is active") + } + idle, goalCmd := idle.launchGoalContinuationIfReady() + if goalCmd != nil || idle.pending { + t.Fatal("armed goal must not continue while plan mode is active") + } + + updated, cmd = idle.handlePlanCommand("off") + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected Ask restored, got %s", next.permissionMode) + } + if len(next.loops) != 1 || next.loops[0].paused { + t.Fatalf("expected the loop to resume after /plan off, got %+v", next.loops) + } + if cmd == nil || !next.pending { + t.Fatal("expected /plan off to resume the armed goal continuation") + } + if !transcriptContains(next.transcript, "Continuing goal: Keep shipping") { + t.Fatalf("expected goal continuation after /plan off, got %#v", next.transcript) + } +} + +func TestReenteringPlanModePreservesExistingPlanFile(t *testing.T) { + dir := t.TempDir() + m := newPlanCommandTestModel(t, dir, agent.PermissionModeAsk) + const initialPlan = "1. [pending] Step one from disk\n2. [completed] Step two from disk" + if _, err := planmode.WritePlan(dir, m.activeSession.SessionID, initialPlan); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + m.input.SetValue("/plan on") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected /plan on to enter plan mode, got %s", next.permissionMode) + } + if len(next.plan.steps) != 2 { + t.Fatalf("expected 2 plan items reloaded from disk, got %d", len(next.plan.steps)) + } + if next.plan.steps[0].content != "Step one from disk" || next.plan.steps[1].status != "completed" { + t.Fatalf("unexpected plan steps: %+v", next.plan.steps) + } +} + +func TestParsePlanFileLinesPreservesContinuationWhitespace(t *testing.T) { + content := "1. [pending] Step with indented code\n" + + " ```go\n" + + " func hello() {}\n" + + " ```\n" + + " Notes:\n" + + " - note line 1\n" + + " - note line 2 " + + items := parsePlanFileLines(content) + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + expectedContent := "Step with indented code\n```go\n func hello() {}\n```" + if items[0].Content != expectedContent { + t.Fatalf("content = %q, want %q", items[0].Content, expectedContent) + } + expectedNotes := " - note line 1\n - note line 2 " + if items[0].Notes != expectedNotes { + t.Fatalf("notes = %q, want %q", items[0].Notes, expectedNotes) + } +} + +func TestPlanModeHoldsQueuedMessageUntilExitOrDeliberateSubmission(t *testing.T) { + isolatePlanConfig(t) + dir := t.TempDir() + m := newPlanCommandTestModel(t, dir, agent.PermissionModeAsk) + + // User has a queued prompt. + m.queuedMessage = "implement feature X" + + // Enter plan mode. + updated, _ := m.handlePlanCommand("on") + m = updated.(model) + + if m.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected permissionMode to be plan, got %s", m.permissionMode) + } + if !m.hasQueuedMessage() { + t.Fatal("expected queuedMessage to stay preserved when entering plan mode") + } + + // Trigger turn completion / idle transition. + updated, _ = m.Update(agentResponseMsg{runID: m.activeRunID}) + next := updated.(model) + + // The queued message must NOT have auto-launched under plan mode. + if next.pending { + t.Fatal("expected queued message not to auto-launch while plan mode is active") + } + if !next.hasQueuedMessage() || next.queuedMessage != "implement feature X" { + t.Fatalf("expected queued message to remain pending, got %q", next.queuedMessage) + } + + // Exiting plan mode should now allow the queued prompt to launch. + updatedAfterExit, exitCmd := next.handlePlanCommand("off") + resumed := updatedAfterExit.(model) + + if resumed.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected permission mode restored to ask, got %s", resumed.permissionMode) + } + if exitCmd == nil { + t.Fatal("expected /plan off to trigger launch of the pending queued prompt") + } + if resumed.hasQueuedMessage() { + t.Fatalf("expected queued message to be consumed on launch, still queued: %q", resumed.queuedMessage) + } + if !resumed.pending { + t.Fatal("expected model to transition to pending after queued message launched") + } +} + +// TestPlanCommandStatusReflectsActualPermissionMode is the regression for P2: +// /plan status (planText) must truthfully distinguish whether PermissionModePlan +// is active and whether a durable or draft plan exists, across mode transitions +// and session operations. +func TestPlanCommandStatusReflectsActualPermissionMode(t *testing.T) { + isolatePlanConfig(t) + dir := t.TempDir() + m := newPlanCommandTestModel(t, dir, agent.PermissionModeAsk) + + // 1. Inactive mode with no plan + status := m.planText() + if !strings.Contains(status, "Plan mode is inactive. No plan written.") { + t.Fatalf("expected inactive notice with no plan, got: %q", status) + } + + // 2. Active mode with no plan + m.permissionMode = agent.PermissionModePlan + status = m.planText() + if !strings.Contains(status, "Plan mode is active. No plan written yet.") { + t.Fatalf("expected active notice with no plan, got: %q", status) + } + + // 3. Active mode with in-memory draft + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "in-memory step", Status: "pending"}}) + m.registry.Register(planTool) + status = m.planText() + if !strings.Contains(status, "Current Plan (plan mode active; draft in memory)") || !strings.Contains(status, "in-memory step") { + t.Fatalf("expected active status with draft in memory, got: %q", status) + } + + // 4. Inactive mode with in-memory draft (e.g. after /plan off before disk save) + m.permissionMode = agent.PermissionModeAsk + status = m.planText() + if !strings.Contains(status, "Current Plan (plan mode inactive; draft in memory)") || !strings.Contains(status, "in-memory step") { + t.Fatalf("expected inactive status with draft in memory, got: %q", status) + } + + // 5. Active mode with durable plan file + if _, err := planmode.WritePlan(dir, m.activeSession.SessionID, "1. [pending] durable step"); err != nil { + t.Fatalf("WritePlan: %v", err) + } + m.permissionMode = agent.PermissionModePlan + status = m.planText() + if !strings.Contains(status, "Current Plan (plan mode active)") || !strings.Contains(status, "durable step") { + t.Fatalf("expected active status with durable plan, got: %q", status) + } + + // 6. Inactive mode with durable plan file (after /plan off) + m.permissionMode = agent.PermissionModeAsk + status = m.planText() + if !strings.Contains(status, "Current Plan (plan mode inactive)") || !strings.Contains(status, "durable step") { + t.Fatalf("expected inactive status with durable plan, got: %q", status) + } +} + +// TestPlanCommandPreservesSecretShapedPlanStepsInPanelAndFile is the regression +// for P1: plan steps containing secret tokens must remain unredacted on disk, +// in the UI panel, and across resume, while transcript Output was scrubbed. +func TestPlanCommandPreservesSecretShapedPlanStepsInPanelAndFile(t *testing.T) { + isolatePlanConfig(t) + dir := t.TempDir() + m := newPlanCommandTestModel(t, dir, agent.PermissionModePlan) + + secretToken := "ghp_123456789012345678901234567890123456" + stepContent := "Deploy with token " + secretToken + + // Run update_plan tool via registry + res := m.registry.Run(context.Background(), "update_plan", map[string]any{ + "plan": []any{ + map[string]any{ + "content": stepContent, + "status": "in_progress", + "notes": "Key: " + secretToken, + }, + }, + }) + if res.Status != tools.StatusOK { + t.Fatalf("registry.Run failed: %+v", res) + } + + // Output was redacted at registry boundary + if strings.Contains(res.Output, secretToken) { + t.Fatalf("res.Output leaked secretToken: %q", res.Output) + } + + // Typed PlanSnapshot carries exact secret + items, ok := planSnapshotFromResult(agent.ToolResult{ + Status: res.Status, + Output: res.Output, + PlanSnapshot: res.PlanSnapshot, + }) + if !ok || len(items) != 1 || items[0].Content != stepContent { + t.Fatalf("planSnapshotFromResult returned invalid snapshot: ok=%v, items=%+v", ok, items) + } + + // Update sticky panel and persist durable file + m.plan.updateFromItems(items, m.now()) + if _, err := planmode.WritePlan(dir, m.activeSession.SessionID, formatPlanItems(items)); err != nil { + t.Fatalf("WritePlan: %v", err) + } + + // Verify durable file has exact secret + content, exists, err := planmode.ReadPlan(dir, m.activeSession.SessionID) + if err != nil || !exists { + t.Fatalf("ReadPlan failed: exists=%v, err=%v", exists, err) + } + if !strings.Contains(content, secretToken) { + t.Fatalf("durable plan file was improperly redacted: %q", content) + } + + // Verify reload rehydrates exact secret + reloaded, reloadedOk, err := m.reloadPlanFromFile() + if err != nil || !reloadedOk || len(reloaded) != 1 || reloaded[0].Content != stepContent { + t.Fatalf("reloadPlanFromFile failed: ok=%v, err=%v, reloaded=%+v", reloadedOk, err, reloaded) + } +} diff --git a/internal/tui/scroll_test.go b/internal/tui/scroll_test.go index 2a94729c3..e19959849 100644 --- a/internal/tui/scroll_test.go +++ b/internal/tui/scroll_test.go @@ -112,7 +112,7 @@ func TestMouseWheelOnClippedFooterStatusDoesNotMoveComposerCursor(t *testing.T) } func TestAltScreenTranscriptScrollKeepsFooterFixed(t *testing.T) { - m := newModel(context.Background(), Options{AltScreen: true, ProviderName: "openai", ModelName: "gpt-4.1"}) + m := newModel(context.Background(), Options{Cwd: "/workspace", AltScreen: true, ProviderName: "openai", ModelName: "gpt-4.1"}) m.width = 90 m.height = 10 m.gitBranch = "feat/pinned-header" diff --git a/internal/tui/session.go b/internal/tui/session.go index 4aec00e58..614778982 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -68,6 +68,15 @@ func (m model) ensureActiveSession(prompt string) (model, error) { func (m model) startNewSession() model { previousID := m.activeSession.SessionID + // Plan mode (and the mode /plan off would restore) belongs to the session + // that entered it — carrying it into a fresh session would silently make + // the new session read-only, or later restore the old session's mode into + // it. Exit it here rather than leaving it to a same-session-only /plan off. + // The plan itself belongs to the old session too, so clear it rather than + // leaking it into a session that never drafted it. + m = m.exitPlanMode() + m = m.resetPlanForSessionSwitch() + m.activeSession = sessions.Metadata{} m.pendingSessionTitle = "" m.sessionEvents = nil @@ -233,8 +242,29 @@ func (m model) handleResumeCommand(args string) (model, string) { // on a real change — `/resume latest` or `/resume ` can resolve to // the already-active session, whose loops belong to it, not a "previous" one. previousID := m.activeSession.SessionID + if session.SessionID != previousID { + // Plan mode (and the mode /plan off would restore) belongs to the + // session that entered it, not to whatever session becomes active — + // see the matching guard in startNewSession. + m = m.exitPlanMode() + m = m.resetPlanForSessionSwitch() + } m.activeSession = *session m.pendingSessionTitle = "" + var planReloadErr error + if session.SessionID != previousID { + // resetPlanForSessionSwitch cleared the previous session's plan; now + // hydrate the destination session's own persisted plan file (if any), + // so the sticky panel and update_plan reflect what THIS session had + // saved instead of starting empty and risking an overwrite on the + // next update_plan call. Surface I/O failures so a broken plan file + // does not leave the destination session silently plan-empty. + if items, ok, err := m.reloadPlanFromFile(); err != nil { + planReloadErr = err + } else if ok { + m.plan.updateFromItems(items, m.now()) + } + } m.sessionEvents = append([]sessions.Event{}, events...) if m.providerName == "" { m.providerName = session.Provider @@ -252,6 +282,9 @@ func (m model) handleResumeCommand(args string) (model, string) { if loopsCleared > 0 { rows = appendRow(rows, rowSystem, fmt.Sprintf("Stopped %d loop(s) tied to the previous session.", loopsCleared)) } + if planReloadErr != nil { + rows = appendRow(rows, rowError, "plan reload error: "+planReloadErr.Error()) + } rows = appendTranscriptRowsDedup(rows, transcriptRowsFromSessionEvents(events)) m.transcript = rows // Every rehydrated row is settled by construction, so resetting the flush diff --git a/internal/tui/session_test.go b/internal/tui/session_test.go index 37477396d..2fd51cb11 100644 --- a/internal/tui/session_test.go +++ b/internal/tui/session_test.go @@ -13,6 +13,7 @@ import ( tea "charm.land/bubbletea/v2" "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/sandbox" "github.com/Gitlawb/zero/internal/sessions" "github.com/Gitlawb/zero/internal/tools" @@ -836,6 +837,259 @@ func TestResumeCommandIsBlockedWhileRunPending(t *testing.T) { } } +// Regression: plan mode (and the permission mode /plan off would restore) +// used to live only on the TUI model, so /new left it attached across the +// session switch — silently making the fresh session read-only, and letting +// its eventual /plan off restore the OLD session's permission mode into it. +func TestNewSessionExitsPlanMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + m := newModel(context.Background(), Options{SessionStore: store}) + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + + m = m.startNewSession() + + if m.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected /new to exit plan mode and restore Ask, got %s", m.permissionMode) + } + if m.permissionModeBeforePlan != "" { + t.Fatalf("expected permissionModeBeforePlan to be cleared, got %q", m.permissionModeBeforePlan) + } +} + +// Regression: exitPlanMode only restored the permission mode, not the plan +// itself. A session switch left the previous session's plan in the shared +// update_plan tool state and sticky panel, leaking it into a session that +// never drafted it. +func TestNewSessionClearsPreviousPlan(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "leftover step", Status: "pending"}}) + registry := tools.NewRegistry() + registry.Register(planTool) + m := newModel(context.Background(), Options{SessionStore: store, Registry: registry}) + m.permissionMode = agent.PermissionModePlan + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + + m = m.startNewSession() + + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("expected /new to clear the shared update_plan state, got %+v", planTool.CurrentPlan()) + } + if !m.plan.isEmpty() { + t.Fatalf("expected /new to clear the sticky plan panel, got %+v", m.plan) + } +} + +func TestResumeDifferentSessionExitsPlanMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + active, err := store.Create(sessions.CreateInput{Title: "Active"}) + if err != nil { + t.Fatalf("Create active: %v", err) + } + other, err := store.Create(sessions.CreateInput{Title: "Other"}) + if err != nil { + t.Fatalf("Create other: %v", err) + } + m := newModel(context.Background(), Options{SessionStore: store}) + m.activeSession = active + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + + m, _ = m.handleResumeCommand(other.SessionID) + + if m.activeSession.SessionID != other.SessionID { + t.Fatalf("expected to resume the other session, got %#v", m.activeSession) + } + if m.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected /resume to a different session to exit plan mode and restore Ask, got %s", m.permissionMode) + } + if m.permissionModeBeforePlan != "" { + t.Fatalf("expected permissionModeBeforePlan to be cleared, got %q", m.permissionModeBeforePlan) + } +} + +// Regression: /resume must surface a durable plan reload failure for the +// destination session instead of leaving sticky/shared plan state silently empty. +func TestResumeDifferentSessionReportsPlanReloadError(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + active, err := store.Create(sessions.CreateInput{Title: "Active"}) + if err != nil { + t.Fatalf("Create active: %v", err) + } + other, err := store.Create(sessions.CreateInput{Title: "Other"}) + if err != nil { + t.Fatalf("Create other: %v", err) + } + + cwd := t.TempDir() + // Plant an unreadable plan path for the destination session: a directory + // where the plan file should be so ReadPlan returns a real I/O error. + path, err := planmode.PlanFilePath(cwd, other.SessionID) + if err != nil { + t.Fatalf("PlanFilePath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("MkdirAll plan dir: %v", err) + } + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("Mkdir over plan path: %v", err) + } + + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "stale step", Status: "pending"}}) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newModel(context.Background(), Options{SessionStore: store, Cwd: cwd, Registry: registry}) + m.activeSession = active + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + + m, _ = m.handleResumeCommand(other.SessionID) + + if m.activeSession.SessionID != other.SessionID { + t.Fatalf("expected to resume the other session, got %#v", m.activeSession) + } + if !transcriptContains(m.transcript, "plan reload error:") { + t.Fatalf("resume did not surface plan reload failure: %#v", m.transcript) + } + // Destination plan state stays empty after the switch + failed reload + // (shared tool cleared by resetPlanForSessionSwitch, panel not rehydrated). + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("expected shared update_plan cleared after failed resume reload, got %+v", planTool.CurrentPlan()) + } + if !m.plan.isEmpty() { + t.Fatalf("expected sticky plan panel empty after failed resume reload, got %+v", m.plan) + } +} + +// Regression: /resume into a session that has a durable plan must restore +// the sticky panel and shared update_plan (including Status and Notes). +func TestResumeDifferentSessionReloadsDestinationPlan(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + active, err := store.Create(sessions.CreateInput{Title: "Active"}) + if err != nil { + t.Fatalf("Create active: %v", err) + } + other, err := store.Create(sessions.CreateInput{Title: "Other"}) + if err != nil { + t.Fatalf("Create other: %v", err) + } + + cwd := t.TempDir() + destItems := []tools.PlanItem{ + {Content: "wire catalog", Status: "completed", Notes: "done in review"}, + {Content: "ship it", Status: "pending", Notes: "wait for CI"}, + } + if _, err := planmode.WritePlan(cwd, other.SessionID, formatPlanItems(destItems)); err != nil { + t.Fatalf("WritePlan destination: %v", err) + } + + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "stale step", Status: "pending"}}) + registry := tools.NewRegistry() + registry.Register(planTool) + + m := newModel(context.Background(), Options{SessionStore: store, Cwd: cwd, Registry: registry}) + m.activeSession = active + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + + m, _ = m.handleResumeCommand(other.SessionID) + + if m.activeSession.SessionID != other.SessionID { + t.Fatalf("expected to resume the other session, got %#v", m.activeSession) + } + got := planTool.CurrentPlan() + if len(got) != 2 { + t.Fatalf("expected 2 restored update_plan items, got %+v", got) + } + if got[0].Content != "wire catalog" || got[0].Status != "completed" || got[0].Notes != "done in review" { + t.Fatalf("first restored item mismatch: %+v", got[0]) + } + if got[1].Content != "ship it" || got[1].Status != "pending" || got[1].Notes != "wait for CI" { + t.Fatalf("second restored item mismatch: %+v", got[1]) + } + if m.plan.isEmpty() { + t.Fatal("expected sticky plan panel restored after destination reload") + } + if len(m.plan.steps) != 2 || m.plan.steps[0].content != "wire catalog" || m.plan.steps[0].status != "completed" || m.plan.steps[0].notes != "done in review" { + t.Fatalf("sticky panel mismatch: %+v", m.plan.steps) + } +} + +// A session that never entered plan mode has an explicit, non-Plan +// permissionMode with no permissionModeBeforePlan to restore. /new and +// /resume must not reset that choice to Auto just because they +// unconditionally call exitPlanMode on every session switch. +func TestNewSessionPreservesNonPlanPermissionMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + m := newModel(context.Background(), Options{SessionStore: store}) + m.permissionMode = agent.PermissionModeAsk + + m = m.startNewSession() + + if m.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected /new to preserve the explicit Ask permission mode, got %s", m.permissionMode) + } +} + +func TestResumeDifferentSessionPreservesNonPlanPermissionMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + active, err := store.Create(sessions.CreateInput{Title: "Active"}) + if err != nil { + t.Fatalf("Create active: %v", err) + } + other, err := store.Create(sessions.CreateInput{Title: "Other"}) + if err != nil { + t.Fatalf("Create other: %v", err) + } + m := newModel(context.Background(), Options{SessionStore: store}) + m.activeSession = active + m.permissionMode = agent.PermissionModeAsk + + m, _ = m.handleResumeCommand(other.SessionID) + + if m.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected /resume to a different session to preserve the explicit Ask permission mode, got %s", m.permissionMode) + } +} + +// Resuming the session that is already active (e.g. `/resume latest` or +// `/resume `) is not a switch, so it must leave plan mode alone — +// matching the existing loopsCleared guard just below. +func TestResumeSameSessionKeepsPlanMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + active, err := store.Create(sessions.CreateInput{Title: "Active"}) + if err != nil { + t.Fatalf("Create active: %v", err) + } + m := newModel(context.Background(), Options{SessionStore: store}) + m.activeSession = active + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + + m, _ = m.handleResumeCommand(active.SessionID) + + if m.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected resuming the same session to leave plan mode active, got %s", m.permissionMode) + } + if m.permissionModeBeforePlan != agent.PermissionModeAsk { + t.Fatalf("expected the saved restore mode preserved on a same-session resume, got %q", m.permissionModeBeforePlan) + } +} + func TestResumePickerExcludesSubRunSessions(t *testing.T) { store := testSessionStore(t) if _, err := store.Create(sessions.CreateInput{Title: "Real Conversation"}); err != nil { diff --git a/internal/tui/spec_mode.go b/internal/tui/spec_mode.go index c91c51396..cb85272aa 100644 --- a/internal/tui/spec_mode.go +++ b/internal/tui/spec_mode.go @@ -35,6 +35,9 @@ func (m model) handleSpecCommand(task string) (tea.Model, tea.Cmd) { return m, nil } + // Match /resume ordering: switch/create the destination session first, then + // clear plan state that belonged to the previous session. Clearing before a + // failed create would drop plan mode on a session that never left. m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendUser, text: "/spec " + task}) var err error m, err = m.createSpecDraftSession(task) @@ -42,6 +45,8 @@ func (m model) handleSpecCommand(task string) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session create error: " + err.Error()}) return m, nil } + m, _ = m.clearLoopsForSessionSwitch() + m = m.resetPlanForSessionSwitch().exitPlanMode() m, err = m.appendSessionEvent(sessions.EventMessage, map[string]any{ "role": "user", "content": task, @@ -201,7 +206,9 @@ func (m model) approveSpecReview() (tea.Model, tea.Cmd) { m.pendingSpecReview = nil m.activeSession = impl m.sessionEvents = append([]sessions.Event{}, events...) + m, _ = m.clearLoopsForSessionSwitch() m = m.syncPeerIdentity() + m = m.resetPlanForSessionSwitch().exitPlanMode() m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Spec approved. Starting implementation session " + impl.SessionID + "."}) runCtx, cancel := context.WithCancel(m.ctx) m = m.beginRun(cancel) diff --git a/internal/tui/spec_mode_test.go b/internal/tui/spec_mode_test.go index 16dcae805..59c549c98 100644 --- a/internal/tui/spec_mode_test.go +++ b/internal/tui/spec_mode_test.go @@ -3,6 +3,8 @@ package tui import ( "context" "encoding/json" + "os" + "path/filepath" "strings" "testing" "time" @@ -67,6 +69,10 @@ func TestSpecApproveStartsImplementationSession(t *testing.T) { if review == nil { t.Fatal("expected pending review before approval") } + next = startFixedLoop(next, "draft-session loop", time.Minute) + next.permissionMode = agent.PermissionModePlan + next.permissionModeBeforePlan = agent.PermissionModeAsk + next, _ = next.pauseLoopsForPlan() updated, cmd = next.Update(testKeyText("a")) next = updated.(model) @@ -79,6 +85,9 @@ func TestSpecApproveStartsImplementationSession(t *testing.T) { if next.activeSession.SessionKind != sessions.SessionKindSpecImpl { t.Fatalf("expected active implementation session, got %#v", next.activeSession) } + if len(next.loops) != 0 { + t.Fatalf("expected approval to clear loops from the draft session, got %+v", next.loops) + } updated, _ = next.Update(execCmd(cmd)) next = updated.(model) @@ -280,3 +289,91 @@ func TestSpecLaunchesSeedElapsedClock(t *testing.T) { t.Fatal("impl launch did not seed turnStartedAt (elapsed clock would not render)") } } + +func TestSpecCommandExitsPlanMode(t *testing.T) { + isolatePlanConfig(t) + store := testSessionStore(t) + provider := &scriptedProvider{scripts: [][]zeroruntime.StreamEvent{ + submitSpecScript("call-1", "Review Flow", "# Goal\n\nAdd review flow."), + }} + m := newSpecModeTestModel(t.TempDir(), provider, store) + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "prior draft", Status: "pending"}}) + m.registry.Register(planTool) + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + var err error + m, err = m.ensureActiveSession("") + if err != nil { + t.Fatalf("ensureActiveSession: %v", err) + } + m = startFixedLoop(m, "previous-session loop", time.Minute) + updated, _ := m.handlePlanCommand("on") + m = updated.(model) + if len(m.loops) != 1 || !m.loops[0].paused { + t.Fatalf("expected /plan on to pause the existing loop, got %+v", m.loops) + } + m.input.SetValue("/spec add review flow") + + updated, _ = m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.permissionMode == agent.PermissionModePlan { + t.Fatalf("expected /spec to exit plan mode, got %s", next.permissionMode) + } + if next.permissionModeBeforePlan != "" { + t.Fatalf("expected permissionModeBeforePlan cleared after /spec, got %q", next.permissionModeBeforePlan) + } + if len(planTool.CurrentPlan()) != 0 { + t.Fatalf("expected shared update_plan cleared after successful /spec, got %+v", planTool.CurrentPlan()) + } + if !next.plan.isEmpty() { + t.Fatalf("expected sticky plan panel cleared after successful /spec, got %+v", next.plan) + } + if len(next.loops) != 0 { + t.Fatalf("expected /spec to clear loops from the previous session, got %+v", next.loops) + } +} + +// Regression: /spec used to clear plan mode before createSpecDraftSession. +// On create failure the user stayed on the original session with plan mode +// already wiped. Create first; only reset plan state after success. +func TestSpecCommandCreateFailurePreservesPlanMode(t *testing.T) { + root := t.TempDir() + // Point the session store root at a regular file so Create fails on MkdirAll. + badRoot := filepath.Join(root, "not-a-dir") + if err := os.WriteFile(badRoot, []byte("x"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + store := sessions.NewStore(sessions.StoreOptions{RootDir: badRoot}) + provider := &scriptedProvider{scripts: [][]zeroruntime.StreamEvent{ + submitSpecScript("call-1", "Review Flow", "# Goal\n\nAdd review flow."), + }} + m := newSpecModeTestModel(root, provider, store) + planTool := tools.NewUpdatePlanTool() + planTool.SetPlan([]tools.PlanItem{{Content: "keep me", Status: "pending"}}) + m.registry.Register(planTool) + m.permissionMode = agent.PermissionModePlan + m.permissionModeBeforePlan = agent.PermissionModeAsk + m.plan.updateFromItems(planTool.CurrentPlan(), m.now()) + m.input.SetValue("/spec add review flow") + + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.pending || next.activeRunID != 0 { + t.Fatalf("expected no agent run when session create fails, pending=%v activeRunID=%d", next.pending, next.activeRunID) + } + if next.permissionMode != agent.PermissionModePlan { + t.Fatalf("expected plan mode preserved after failed /spec create, got %s", next.permissionMode) + } + if next.permissionModeBeforePlan != agent.PermissionModeAsk { + t.Fatalf("expected permissionModeBeforePlan preserved, got %q", next.permissionModeBeforePlan) + } + if len(planTool.CurrentPlan()) != 1 || planTool.CurrentPlan()[0].Content != "keep me" { + t.Fatalf("expected shared plan preserved after failed /spec create, got %+v", planTool.CurrentPlan()) + } + if next.plan.isEmpty() { + t.Fatal("expected sticky plan panel preserved after failed /spec create") + } + if !transcriptContains(next.transcript, "session create error") { + t.Fatalf("expected session create error in transcript, got %#v", next.transcript) + } +} diff --git a/internal/tui/view.go b/internal/tui/view.go index c32f3b93c..4c88d6649 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -328,6 +328,9 @@ func nextPermissionMode(mode agent.PermissionMode) agent.PermissionMode { case agent.PermissionModeAsk: return agent.PermissionModeAuto case agent.PermissionModePlan: + // Plan mode is a deliberate read-only gate entered via /plan; a casual + // shift+tab must not silently drop it (that would re-enable file/command + // tools without the user ever choosing to exit plan mode). return agent.PermissionModePlan default: // Anything else (incl. an externally-set Unsafe) folds to Ask — the stricter