From 55360163259ff09ecd0c91e6f6f98d418da886b0 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 10:54:11 -0400 Subject: [PATCH 01/17] feat(tui): add plan mode command and fix plan file editing --- internal/tui/model.go | 6 +- internal/tui/plan_command.go | 130 +++++++++++++++++++++++++++++++++-- internal/tui/run.go | 1 + 3 files changed, 130 insertions(+), 7 deletions(-) diff --git a/internal/tui/model.go b/internal/tui/model.go index eceebfc2d..d477b3eb5 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -126,6 +126,9 @@ type model struct { agentOptions agent.Options notifier *notify.Notifier permissionMode agent.PermissionMode + // program is the live Bubble Tea program, set right before Run so /plan open + // can suspend the TUI, launch $EDITOR, and resume on exit. + program *tea.Program selfCorrectTests bool reasoningEffort modelregistry.ReasoningEffort responseStyle string @@ -4212,8 +4215,7 @@ func (m model) handleSubmit() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.debugText()}) return m, nil case commandPlan: - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.planText()}) - return m, nil + return m.handlePlanCommand(command.text) case commandDoctor: return m.startDoctorCommand(command.text) case commandSearch: diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 8baf629da..5d4dd5b83 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -2,8 +2,14 @@ package tui import ( "fmt" + "os" + "os/exec" "strings" + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/tools" ) @@ -11,20 +17,126 @@ type currentPlanReader interface { CurrentPlan() []tools.PlanItem } +// handlePlanCommand toggles plan mode on the current session, in the style of +// openclaude's /plan: +// +// /plan toggle plan mode on/off; when on, show the current plan +// /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 agent.toolAdvertisedInPlan) +// 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) { + if _, ok := m.registry.Get("update_plan"); !ok { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "No plan is active."}) + return m, nil + } + + arg := strings.ToLower(strings.TrimSpace(text)) + switch arg { + case "off", "exit": + if m.permissionMode != agent.PermissionModePlan { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode is not active."}) + return m, nil + } + m.permissionMode = agent.PermissionModeAuto + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Exited plan mode. The agent can now implement."}) + return m, nil + case "open": + return m.openPlanInEditor() + } + + // No subcommand: toggle plan mode, then surface the current plan. + if m.permissionMode == agent.PermissionModePlan { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.planText()}) + return m, nil + } + 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 + } + m.permissionMode = agent.PermissionModePlan + textToShow := planEnterText(m) + "\n\n" + m.planText() + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: textToShow}) + return m, nil +} + +// 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) 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 + } + if _, ok := fileExists(path); !ok { + if _, err := planmode.WritePlan(m.cwd, m.activeSession.SessionID, ""); 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 + } + if m.program == nil { + // No live program (e.g. under test): just report the path. + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan file: " + path}) + return m, nil + } + parts := strings.Fields(editor) + cmd := exec.Command(parts[0], append(parts[1:], path)...) //nolint:gosec // editor path from $VISUAL/$EDITOR + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return m, tea.ExecProcess(cmd, func(err error) tea.Msg { + return nil + }) +} + +func planEnterText(m model) string { + path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID) + planNote := "" + if err == nil && path != "" { + planNote = "\nPlan file: " + path + } + return "Entered plan mode. The agent can inspect the workspace and shape the plan with update_plan, but cannot edit files or run commands until you exit.\n" + + "Use /plan to view the plan, /plan open to edit it, or /plan off to implement." + planNote +} + func (m model) planText() string { + // Prefer the session plan file when present. + if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil && path != "" { + if content, exists, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID); err == nil && exists { + header := "Current Plan (plan mode)" + if path != "" { + header += "\n" + path + } + return header + "\n" + strings.TrimRight(content, "\n") + } + } + + // Fall back to the update_plan list the agent has been building. tool, ok := m.registry.Get("update_plan") if !ok { - return "No plan is active." + return "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." } - reader, ok := tool.(currentPlanReader) if !ok { - return "No plan is active." + return "Plan mode is active. No plan written yet." } - plan := reader.CurrentPlan() if len(plan) == 0 { - return "No plan is active." + return "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." } lines := make([]string, 0, len(plan)+1) @@ -38,3 +150,11 @@ func (m model) planText() string { } return strings.Join(lines, "\n") } + +func fileExists(path string) (struct{}, bool) { + _, err := os.Stat(path) + if err != nil { + return struct{}{}, false + } + return struct{}{}, true +} diff --git a/internal/tui/run.go b/internal/tui/run.go index f2925dea2..ccde3964d 100644 --- a/internal/tui/run.go +++ b/internal/tui/run.go @@ -57,6 +57,7 @@ func Run(ctx context.Context, options Options) int { initialModel.mouseCapture = true } program = tea.NewProgram(initialModel, programOpts...) + initialModel.program = program if _, err := program.Run(); err != nil { // Surface the failure: exiting 1 with zero diagnostics left users From 69731f921d55140632c50c5bd6cc2b3e9ee11f01 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:20:24 -0400 Subject: [PATCH 02/17] style(tui): gofmt internal/tui/model.go to fix CI lint --- internal/tui/model.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/tui/model.go b/internal/tui/model.go index d477b3eb5..fa58544cf 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -128,23 +128,23 @@ type model struct { permissionMode agent.PermissionMode // program is the live Bubble Tea program, set right before Run so /plan open // can suspend the TUI, launch $EDITOR, and resume on exit. - program *tea.Program - selfCorrectTests bool - reasoningEffort modelregistry.ReasoningEffort - responseStyle string - keyBindings keyBindings - themeMode themeMode // palette preference: auto (default), dark, light - hasDarkBg bool // last terminal background-detection result (auto mode) - userAgent string - compactRequests int - compactInFlight bool - compactFrame int - lastCompactResult *CompactResult - lastCompactError string - unpricedRequests int - unpricedTokens int - lastUsage usage.Normalized - lastUsageSeen bool + program *tea.Program + selfCorrectTests bool + reasoningEffort modelregistry.ReasoningEffort + responseStyle string + keyBindings keyBindings + themeMode themeMode // palette preference: auto (default), dark, light + hasDarkBg bool // last terminal background-detection result (auto mode) + userAgent string + compactRequests int + compactInFlight bool + compactFrame int + lastCompactResult *CompactResult + lastCompactError string + unpricedRequests int + unpricedTokens int + lastUsage usage.Normalized + lastUsageSeen bool // turnLatencySum / turnLatencyCount accumulate completed-run wall time so // /context can show a rolling average turn latency (the "is it slow?" signal). // Reset by /new. From 7b7eb49c89e8a3afb7fca66de306f0c30625e14e Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:13:14 -0400 Subject: [PATCH 03/17] feat(tui): add missing internal/planmode package --- internal/planmode/planmode.go | 131 ++++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 internal/planmode/planmode.go diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go new file mode 100644 index 000000000..4ee26447b --- /dev/null +++ b/internal/planmode/planmode.go @@ -0,0 +1,131 @@ +package planmode + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +// PlanDirName is the workspace-relative directory where /plan plan files live, +// mirroring the spec-draft convention under .zero (see specmode.SpecDirName). +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 plan file path for a session under the +// workspace .zero/plans directory. The session ID is slugified so the file name +// is stable across re-entering plan mode within the same session. +func PlanFilePath(workspaceRoot, sessionID string) (string, error) { + root := strings.TrimSpace(workspaceRoot) + if root == "" { + return "", fmt.Errorf("workspace root is required") + } + absoluteRoot, err := filepath.Abs(root) + if err != nil { + return "", fmt.Errorf("resolve workspace root: %w", err) + } + id := slugify(sessionID) + relativePath := filepath.ToSlash(filepath.Join(PlanDirName, id+".md")) + path := filepath.Join(absoluteRoot, filepath.FromSlash(relativePath)) + if err := ensurePlanPathContained(absoluteRoot, path); err != nil { + return "", err + } + return path, nil +} + +// ReadPlan reads the plan file for a session. The bool reports whether a plan +// file exists; a missing file is not an error. +func ReadPlan(workspaceRoot, sessionID string) (string, bool, error) { + path, err := PlanFilePath(workspaceRoot, sessionID) + if err != nil { + return "", false, err + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return "", false, nil + } + 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. +func WritePlan(workspaceRoot, sessionID, content string) (string, error) { + path, err := PlanFilePath(workspaceRoot, sessionID) + if err != nil { + return "", err + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return "", fmt.Errorf("create plan directory: %w", err) + } + if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o644); err != nil { + return "", fmt.Errorf("write plan file: %w", err) + } + return path, nil +} + +func ensurePlanPathContained(workspaceRoot, path string) error { + relative, err := filepath.Rel(filepath.Clean(workspaceRoot), filepath.Clean(path)) + if err != nil { + return fmt.Errorf("resolve plan file path: %w", err) + } + if relative == "." || relative == ".." || filepath.IsAbs(relative) || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return fmt.Errorf("plan file path escapes workspace root") + } + return nil +} + +// slugify turns an arbitrary session identifier into a filesystem-safe slug. +func slugify(id string) string { + id = strings.TrimSpace(id) + if id == "" { + id = fmt.Sprintf("%d", time.Now().UnixNano()) + } + 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 +} From aef8b43c99256cc45d4a64a0e721918db96e9d8a Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:40:21 -0400 Subject: [PATCH 04/17] fix(tui): address review findings on plan mode - restrict plan file permissions to owner only (0o700 dir, 0o600 file) - surface editor failures from /plan open in the transcript - simplify fileExists to return a bool - collapse redundant plan path resolution in planText --- internal/planmode/planmode.go | 4 ++-- internal/tui/model.go | 5 +++++ internal/tui/plan_command.go | 28 +++++++++++++++------------- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index 4ee26447b..7803c2c01 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -78,10 +78,10 @@ func WritePlan(workspaceRoot, sessionID, content string) (string, error) { if err != nil { return "", err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return "", fmt.Errorf("create plan directory: %w", err) } - if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o644); err != nil { + if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o600); err != nil { return "", fmt.Errorf("write plan file: %w", err) } return path, nil diff --git a/internal/tui/model.go b/internal/tui/model.go index fa58544cf..101dea48a 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1111,6 +1111,11 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { m.copyStatus = "" } 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 case exitConfirmExpiredMsg: if msg.seq == m.exitConfirmSeq { m.exitConfirmActive = false diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 5d4dd5b83..67bf2b365 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -74,7 +74,7 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan path error: " + err.Error()}) return m, nil } - if _, ok := fileExists(path); !ok { + if !fileExists(path) { if _, err := planmode.WritePlan(m.cwd, m.activeSession.SessionID, ""); err != nil { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan write error: " + err.Error()}) return m, nil @@ -99,10 +99,19 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr return m, tea.ExecProcess(cmd, func(err error) tea.Msg { + if err != nil { + return planEditorFinishedMsg{err: err} + } return nil }) } +// planEditorFinishedMsg reports a failed $VISUAL/$EDITOR run launched by +// /plan open so the transcript can surface it. +type planEditorFinishedMsg struct { + err error +} + func planEnterText(m model) string { path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID) planNote := "" @@ -115,13 +124,9 @@ func planEnterText(m model) string { func (m model) planText() string { // Prefer the session plan file when present. - if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil && path != "" { - if content, exists, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID); err == nil && exists { - header := "Current Plan (plan mode)" - if path != "" { - header += "\n" + path - } - return header + "\n" + strings.TrimRight(content, "\n") + if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { + if content, err := os.ReadFile(path); err == nil { + return "Current Plan (plan mode)\n" + path + "\n" + strings.TrimRight(string(content), "\n") } } @@ -151,10 +156,7 @@ func (m model) planText() string { return strings.Join(lines, "\n") } -func fileExists(path string) (struct{}, bool) { +func fileExists(path string) bool { _, err := os.Stat(path) - if err != nil { - return struct{}{}, false - } - return struct{}{}, true + return err == nil } From 7eaf4f844089fa395f7d0b1a5f4e22ec3f233650 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 10:52:19 -0400 Subject: [PATCH 05/17] feat(agent): add PermissionModePlan for interactive read-only planning --- internal/agent/loop.go | 25 +++++++++++++++++++++++-- internal/agent/types.go | 6 ++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/internal/agent/loop.go b/internal/agent/loop.go index 69c87ecfc..3d9dae9d3 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -972,12 +972,18 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal }, nil } tool, toolFound := registry.Get(call.Name) - if permissionMode == PermissionModeSpecDraft && toolFound && !ToolAdvertised(tool, permissionMode) { + if (permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan) && toolFound && !ToolAdvertised(tool, permissionMode) { + modeName := string(permissionMode) + if permissionMode == PermissionModePlan { + modeName = "plan" + } else { + modeName = "spec-draft" + } return ToolResult{ ToolCallID: call.ID, Name: call.Name, Status: tools.StatusError, - Output: `Error: Tool "` + call.Name + `" is not available in spec-draft mode.`, + Output: `Error: Tool "` + call.Name + `" is not available in ` + modeName + ` mode.`, DenialReason: DenialFiltered, }, nil } @@ -2934,6 +2940,9 @@ func ToolAdvertised(tool tools.Tool, permissionMode PermissionMode) bool { if permissionMode == PermissionModeSpecDraft { return toolAdvertisedInSpecDraft(tool) } + if permissionMode == PermissionModePlan { + return toolAdvertisedInPlan(tool) + } if permissionMode == PermissionModeAuto { return tool.Safety().Permission == tools.PermissionAllow || tool.Safety().AdvertiseInAuto } @@ -2966,6 +2975,18 @@ func toolAdvertisedInSpecDraft(tool tools.Tool) bool { return safety.SideEffect == tools.SideEffectRead && safety.Permission == tools.PermissionAllow } +// toolAdvertisedInPlan mirrors toolAdvertisedInSpecDraft: the agent may only +// read the workspace, ask the user, and shape the plan with update_plan. No +// mutating tool is advertised, so plan mode stays strictly read-only. +func toolAdvertisedInPlan(tool tools.Tool) bool { + switch tool.Name() { + case "ask_user", "update_plan": + return true + } + safety := tool.Safety() + return safety.SideEffect == tools.SideEffectRead && safety.Permission == tools.PermissionAllow +} + func stopReasonFromToolResult(result ToolResult) StopReason { if result.Meta == nil { return "" diff --git a/internal/agent/types.go b/internal/agent/types.go index 465d4a0fd..d9f272be7 100644 --- a/internal/agent/types.go +++ b/internal/agent/types.go @@ -25,6 +25,12 @@ const ( PermissionModeAsk PermissionMode = "ask" PermissionModeUnsafe PermissionMode = "unsafe" PermissionModeSpecDraft PermissionMode = "spec-draft" + // PermissionModePlan is an interactive, read-only planning mode toggled from + // the TUI with /plan. It applies to the CURRENT session (unlike spec-draft, + // which drafts in a separate session): the agent may inspect the workspace + // and shape the plan with update_plan/ask_user, but no mutating tool is + // advertised, so it cannot write files, run shell, or implement while planning. + PermissionModePlan PermissionMode = "plan" // PermissionModeMemberAuto is a headless mode for swarm/specialist MEMBERS: it // advertises the in-workspace mutators a member needs to build (write/edit + // shell) on top of the Auto set, while the sandbox engine still gates them at From bef346143cf8c649d16e4f18cc0d3fd493b08e72 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 04:44:09 -0400 Subject: [PATCH 06/17] fix(tui): address review findings on plan mode editor and safety /plan open was non-functional because run.go assigned the live program to a copy of the model after tea.NewProgram had already captured it by value; the field is removed and tea.ExecProcess is used directly. Shift+Tab no longer silently drops plan mode, planmode.DraftSystemPrompt is wired into plan-mode runs, plan file paths reject symlink escapes, read errors are no longer swallowed, opening a new plan file seeds it from the agent's draft instead of leaving it blank and shadowing that draft, /plan off restores the prior permission mode instead of forcing Auto, and the session slug is stable when no session ID exists yet. --- internal/planmode/planmode.go | 34 ++++++- internal/planmode/planmode_test.go | 128 +++++++++++++++++++++++ internal/tui/model.go | 48 +++++---- internal/tui/model_test.go | 5 + internal/tui/plan_command.go | 52 ++++++---- internal/tui/plan_command_test.go | 157 +++++++++++++++++++++++++++++ internal/tui/run.go | 1 - internal/tui/view.go | 5 + 8 files changed, 390 insertions(+), 40 deletions(-) create mode 100644 internal/planmode/planmode_test.go create mode 100644 internal/tui/plan_command_test.go diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index 7803c2c01..80aee2d23 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -5,7 +5,6 @@ import ( "os" "path/filepath" "strings" - "time" ) // PlanDirName is the workspace-relative directory where /plan plan files live, @@ -95,6 +94,33 @@ func ensurePlanPathContained(workspaceRoot, path string) error { if relative == "." || relative == ".." || filepath.IsAbs(relative) || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { return fmt.Errorf("plan file path escapes workspace root") } + return rejectSymlinkEscape(workspaceRoot, path) +} + +// rejectSymlinkEscape walks path's ancestors up to (but not including) +// workspaceRoot and refuses to proceed if any existing component - the plan +// file itself, the plans directory, or .zero - is a symlink. The lexical +// filepath.Rel check above only guards against ".." traversal in the +// constructed path; a pre-planted symlink at any of those locations would +// still let os.MkdirAll/os.WriteFile/os.ReadFile follow it outside the +// workspace despite the path string looking contained. +func rejectSymlinkEscape(workspaceRoot, path string) error { + root := filepath.Clean(workspaceRoot) + for current := filepath.Clean(path); current != root; current = filepath.Dir(current) { + parent := filepath.Dir(current) + if parent == current { + // Reached the filesystem root without hitting workspaceRoot; the + // filepath.Rel check above already rejects this case. + return nil + } + info, err := os.Lstat(current) + switch { + case err == nil && info.Mode()&os.ModeSymlink != 0: + return fmt.Errorf("plan file path %s contains a symlink", current) + case err != nil && !os.IsNotExist(err): + return fmt.Errorf("check plan file path: %w", err) + } + } return nil } @@ -102,7 +128,11 @@ func ensurePlanPathContained(workspaceRoot, path string) error { func slugify(id string) string { id = strings.TrimSpace(id) if id == "" { - id = fmt.Sprintf("%d", time.Now().UnixNano()) + // 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. + id = "plan" } var b strings.Builder prevDash := false diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go new file mode 100644 index 000000000..aaadda92d --- /dev/null +++ b/internal/planmode/planmode_test.go @@ -0,0 +1,128 @@ +package planmode + +import ( + "os" + "path/filepath" + "testing" +) + +func TestPlanFilePathIsStableAcrossCalls(t *testing.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). + 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 TestWritePlanUsesRestrictivePermissions(t *testing.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 TestReadWritePlanRoundtrip(t *testing.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) { + 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") + } +} + +func TestPlanFilePathRejectsSymlinkedPlansDir(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".zero"), 0o700); err != nil { + t.Fatalf("mkdir .zero: %v", err) + } + // Plant a symlink at .zero/plans pointing outside the workspace, as if an + // attacker (or a stale state) had redirected it before /plan open ran. + if err := os.Symlink(outside, filepath.Join(root, ".zero", "plans")); err != nil { + t.Fatalf("symlink .zero/plans: %v", err) + } + + if _, err := PlanFilePath(root, "session-1"); err == nil { + t.Fatal("expected PlanFilePath to reject a symlinked plans directory") + } +} + +func TestPlanFilePathRejectsSymlinkedPlanFile(t *testing.T) { + root := t.TempDir() + outsideFile := filepath.Join(t.TempDir(), "exfil.md") + if err := os.WriteFile(outsideFile, []byte("secret"), 0o600); err != nil { + t.Fatalf("write outside file: %v", err) + } + plansDir := filepath.Join(root, ".zero", "plans") + if err := os.MkdirAll(plansDir, 0o700); err != nil { + t.Fatalf("mkdir plans: %v", err) + } + id := slugify("session-1") + if err := os.Symlink(outsideFile, filepath.Join(plansDir, id+".md")); err != nil { + t.Fatalf("symlink plan file: %v", err) + } + + if _, err := PlanFilePath(root, "session-1"); err == nil { + t.Fatal("expected PlanFilePath to reject a symlinked plan file") + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 101dea48a..cc5549551 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -24,6 +24,7 @@ import ( internalmcp "github.com/Gitlawb/zero/internal/mcp" "github.com/Gitlawb/zero/internal/modelregistry" "github.com/Gitlawb/zero/internal/notify" + "github.com/Gitlawb/zero/internal/planmode" "github.com/Gitlawb/zero/internal/providerhealth" "github.com/Gitlawb/zero/internal/providermodeldiscovery" "github.com/Gitlawb/zero/internal/providers/providerio" @@ -126,25 +127,25 @@ type model struct { agentOptions agent.Options notifier *notify.Notifier permissionMode agent.PermissionMode - // program is the live Bubble Tea program, set right before Run so /plan open - // can suspend the TUI, launch $EDITOR, and resume on exit. - program *tea.Program - selfCorrectTests bool - reasoningEffort modelregistry.ReasoningEffort - responseStyle string - keyBindings keyBindings - themeMode themeMode // palette preference: auto (default), dark, light - hasDarkBg bool // last terminal background-detection result (auto mode) - userAgent string - compactRequests int - compactInFlight bool - compactFrame int - lastCompactResult *CompactResult - lastCompactError string - unpricedRequests int - unpricedTokens int - lastUsage usage.Normalized - lastUsageSeen bool + // permissionModeBeforePlan is the mode active before /plan entered plan + // mode, restored on /plan off instead of hardcoding Auto. + permissionModeBeforePlan agent.PermissionMode + selfCorrectTests bool + reasoningEffort modelregistry.ReasoningEffort + responseStyle string + keyBindings keyBindings + themeMode themeMode // palette preference: auto (default), dark, light + hasDarkBg bool // last terminal background-detection result (auto mode) + userAgent string + compactRequests int + compactInFlight bool + compactFrame int + lastCompactResult *CompactResult + lastCompactError string + unpricedRequests int + unpricedTokens int + lastUsage usage.Normalized + lastUsageSeen bool // turnLatencySum / turnLatencyCount accumulate completed-run wall time so // /context can show a rolling average turn latency (the "is it slow?" signal). // Reset by /new. @@ -4737,8 +4738,15 @@ 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. + options.SystemPrompt = planmode.DraftSystemPrompt } options.SessionID = m.activeSession.SessionID options.ProviderName = m.providerName diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index e9fd5de21..7515ddcec 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2616,6 +2616,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 67bf2b365..14c275f44 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -17,8 +17,7 @@ type currentPlanReader interface { CurrentPlan() []tools.PlanItem } -// handlePlanCommand toggles plan mode on the current session, in the style of -// openclaude's /plan: +// handlePlanCommand toggles plan mode on the current session: // // /plan toggle plan mode on/off; when on, show the current plan // /plan open open the session's plan file in $VISUAL/$EDITOR @@ -41,6 +40,10 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { return m, nil } m.permissionMode = agent.PermissionModeAuto + if m.permissionModeBeforePlan != "" { + m.permissionMode = m.permissionModeBeforePlan + } + m.permissionModeBeforePlan = "" m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Exited plan mode. The agent can now implement."}) return m, nil case "open": @@ -56,6 +59,7 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "Cannot enter plan mode while a run is active."}) return m, nil } + m.permissionModeBeforePlan = m.permissionMode m.permissionMode = agent.PermissionModePlan textToShow := planEnterText(m) + "\n\n" + m.planText() m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: textToShow}) @@ -75,7 +79,11 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { return m, nil } if !fileExists(path) { - if _, err := planmode.WritePlan(m.cwd, m.activeSession.SessionID, ""); err != nil { + // 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 } @@ -88,11 +96,6 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Set $VISUAL or $EDITOR to open the plan file:\n" + path}) return m, nil } - if m.program == nil { - // No live program (e.g. under test): just report the path. - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan file: " + path}) - return m, nil - } parts := strings.Fields(editor) cmd := exec.Command(parts[0], append(parts[1:], path)...) //nolint:gosec // editor path from $VISUAL/$EDITOR cmd.Stdin = os.Stdin @@ -113,9 +116,8 @@ type planEditorFinishedMsg struct { } func planEnterText(m model) string { - path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID) planNote := "" - if err == nil && path != "" { + if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { planNote = "\nPlan file: " + path } return "Entered plan mode. The agent can inspect the workspace and shape the plan with update_plan, but cannot edit files or run commands until you exit.\n" + @@ -125,27 +127,43 @@ func planEnterText(m model) string { func (m model) planText() string { // Prefer the session plan file when present. if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { - if content, err := os.ReadFile(path); err == nil { + content, readErr := os.ReadFile(path) + switch { + case readErr == nil: return "Current Plan (plan mode)\n" + path + "\n" + strings.TrimRight(string(content), "\n") + case !os.IsNotExist(readErr): + // 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() } } // Fall back to the update_plan list the agent has been building. + if draft := m.formatPlanDraft(); draft != "" { + return "Current Plan\n" + draft + } + return "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." +} + +// 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 "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." + return "" } reader, ok := tool.(currentPlanReader) if !ok { - return "Plan mode is active. No plan written yet." + return "" } plan := reader.CurrentPlan() if len(plan) == 0 { - return "Plan mode is active. No plan written yet. Use update_plan to outline steps, or /plan open to draft the plan file." + return "" } - - lines := make([]string, 0, len(plan)+1) - lines = append(lines, "Current Plan") + lines := make([]string, 0, len(plan)) for index, item := range plan { line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, item.Content) if item.Notes != "" { diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go new file mode 100644 index 000000000..1a7d36bc9 --- /dev/null +++ b/internal/tui/plan_command_test.go @@ -0,0 +1,157 @@ +package tui + +import ( + "context" + "os" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/planmode" + "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +func newPlanModeTestModel(t *testing.T, cwd string, permissionMode agent.PermissionMode) model { + t.Helper() + 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 TestShiftTabDoesNotExitPlanMode(t *testing.T) { + m := newPlanModeTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan") + 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 := newPlanModeTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan") + 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 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 := newPlanModeTestModel(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) { + 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 TestPlanModeWiresDraftSystemPrompt(t *testing.T) { + provider := &fakeProvider{events: []zeroruntime.StreamEvent{ + {Type: zeroruntime.StreamEventText, Content: "planning"}, + {Type: zeroruntime.StreamEventDone}, + }} + m := newPlanModeTestModel(t, t.TempDir(), agent.PermissionModePlan) + 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) + } +} diff --git a/internal/tui/run.go b/internal/tui/run.go index ccde3964d..f2925dea2 100644 --- a/internal/tui/run.go +++ b/internal/tui/run.go @@ -57,7 +57,6 @@ func Run(ctx context.Context, options Options) int { initialModel.mouseCapture = true } program = tea.NewProgram(initialModel, programOpts...) - initialModel.program = program if _, err := program.Run(); err != nil { // Surface the failure: exiting 1 with zero diagnostics left users diff --git a/internal/tui/view.go b/internal/tui/view.go index e0a68246f..776844acd 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -308,6 +308,11 @@ func nextPermissionMode(mode agent.PermissionMode) agent.PermissionMode { return agent.PermissionModeAsk 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 // landing, so toggling never makes an Unsafe session less strict. From e7d9e5a0c48791f6d0831365ef716ca886f78de5 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:51:32 -0400 Subject: [PATCH 07/17] fix(tui): address plan-mode review findings on toggle, session scoping, and persistence Make a bare /plan toggle off when already active instead of only reprinting the plan. Scope plan mode to the session that entered it: /new and /resume to a different session now exit plan mode instead of leaking a stale grant or restore-mode across sessions. Create the active session before naming its plan file so a fresh TUI no longer collides on a shared plan.md. Persist every update_plan call to the plan file so it is the durable source of truth instead of an in-memory snapshot. Replace the preflight Lstat symlink check with os.Root, closing the check/use race via descriptor-relative operations. Skip the plan-file permission assertions on Windows, where POSIX mode bits aren't meaningful. --- internal/agent/plan_mode_advertised_test.go | 73 +++++++++++++ internal/planmode/planmode.go | 91 ++++++++-------- internal/planmode/planmode_test.go | 31 ++++-- internal/tui/model.go | 14 ++- internal/tui/plan_command.go | 97 +++++++++++------ internal/tui/plan_command_test.go | 114 ++++++++++++++++++++ internal/tui/session.go | 12 +++ internal/tui/session_test.go | 69 ++++++++++++ 8 files changed, 414 insertions(+), 87 deletions(-) create mode 100644 internal/agent/plan_mode_advertised_test.go diff --git a/internal/agent/plan_mode_advertised_test.go b/internal/agent/plan_mode_advertised_test.go new file mode 100644 index 000000000..c8783047e --- /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 toolAdvertisedInPlan's fallback requires +// SideEffect == SideEffectRead, so SideEffectNone tools must be named +// explicitly (ask_user, update_plan) to be advertised. request_permissions is +// not named, so it is excluded — this test pins that down. +func TestToolAdvertisedInPlanExcludesRequestPermissions(t *testing.T) { + if toolAdvertisedInPlan(tools.NewRequestPermissionsTool()) { + 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/planmode/planmode.go b/internal/planmode/planmode.go index 80aee2d23..ea6d502db 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -32,9 +32,13 @@ 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 plan file path for a session under the -// workspace .zero/plans directory. The session ID is slugified so the file name -// is stable across re-entering plan mode within the same session. +// PlanFilePath returns the deterministic, absolute plan file path for a +// session under the workspace .zero/plans directory, for display and for +// handing to an external editor process. It performs no filesystem access and +// gives no containment guarantee by itself: ReadPlan and WritePlan are the +// safe way to actually read or write plan content, since they resolve paths +// through os.Root and cannot be redirected outside the workspace even by a +// symlink planted between this call and theirs. func PlanFilePath(workspaceRoot, sessionID string) (string, error) { root := strings.TrimSpace(workspaceRoot) if root == "" { @@ -44,23 +48,18 @@ func PlanFilePath(workspaceRoot, sessionID string) (string, error) { if err != nil { return "", fmt.Errorf("resolve workspace root: %w", err) } - id := slugify(sessionID) - relativePath := filepath.ToSlash(filepath.Join(PlanDirName, id+".md")) - path := filepath.Join(absoluteRoot, filepath.FromSlash(relativePath)) - if err := ensurePlanPathContained(absoluteRoot, path); err != nil { - return "", err - } - return path, nil + return filepath.Join(absoluteRoot, planRelativePath(sessionID)), nil } // ReadPlan reads the plan file for a session. The bool reports whether a plan // file exists; a missing file is not an error. func ReadPlan(workspaceRoot, sessionID string) (string, bool, error) { - path, err := PlanFilePath(workspaceRoot, sessionID) + root, err := openWorkspaceRoot(workspaceRoot) if err != nil { return "", false, err } - data, err := os.ReadFile(path) + defer root.Close() + data, err := root.ReadFile(planRelativePath(sessionID)) if err != nil { if os.IsNotExist(err) { return "", false, nil @@ -73,55 +72,51 @@ func ReadPlan(workspaceRoot, sessionID string) (string, bool, error) { // WritePlan writes (creating the directory as needed) the plan file for a // session and returns its path. func WritePlan(workspaceRoot, sessionID, content string) (string, error) { - path, err := PlanFilePath(workspaceRoot, sessionID) + root, err := openWorkspaceRoot(workspaceRoot) if err != nil { return "", err } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + defer root.Close() + + if err := root.MkdirAll(filepath.FromSlash(PlanDirName), 0o700); err != nil { return "", fmt.Errorf("create plan directory: %w", err) } - if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o600); err != nil { + file, err := root.OpenFile(planRelativePath(sessionID), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { return "", fmt.Errorf("write plan file: %w", err) } - return path, nil + defer file.Close() + if _, err := file.WriteString(strings.TrimRight(content, "\n") + "\n"); err != nil { + return "", fmt.Errorf("write plan file: %w", err) + } + return PlanFilePath(workspaceRoot, sessionID) } -func ensurePlanPathContained(workspaceRoot, path string) error { - relative, err := filepath.Rel(filepath.Clean(workspaceRoot), filepath.Clean(path)) - if err != nil { - return fmt.Errorf("resolve plan file path: %w", err) +// openWorkspaceRoot opens the workspace directory as an os.Root, which the +// Go runtime resolves relative to using descriptor-relative (openat-style) +// operations: every subsequent Root method call re-walks the path from that +// descriptor and refuses to follow a symlink referencing a location outside +// it. That closes the check/use race a separate Lstat-then-open preflight +// would leave open (a symlink planted at .zero, .zero/plans, or the plan file +// itself between the check and the later open could otherwise redirect the +// read/write outside the workspace). +func openWorkspaceRoot(workspaceRoot string) (*os.Root, error) { + root := strings.TrimSpace(workspaceRoot) + if root == "" { + return nil, fmt.Errorf("workspace root is required") } - if relative == "." || relative == ".." || filepath.IsAbs(relative) || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { - return fmt.Errorf("plan file path escapes workspace root") + r, err := os.OpenRoot(root) + if err != nil { + return nil, fmt.Errorf("open workspace root: %w", err) } - return rejectSymlinkEscape(workspaceRoot, path) + return r, nil } -// rejectSymlinkEscape walks path's ancestors up to (but not including) -// workspaceRoot and refuses to proceed if any existing component - the plan -// file itself, the plans directory, or .zero - is a symlink. The lexical -// filepath.Rel check above only guards against ".." traversal in the -// constructed path; a pre-planted symlink at any of those locations would -// still let os.MkdirAll/os.WriteFile/os.ReadFile follow it outside the -// workspace despite the path string looking contained. -func rejectSymlinkEscape(workspaceRoot, path string) error { - root := filepath.Clean(workspaceRoot) - for current := filepath.Clean(path); current != root; current = filepath.Dir(current) { - parent := filepath.Dir(current) - if parent == current { - // Reached the filesystem root without hitting workspaceRoot; the - // filepath.Rel check above already rejects this case. - return nil - } - info, err := os.Lstat(current) - switch { - case err == nil && info.Mode()&os.ModeSymlink != 0: - return fmt.Errorf("plan file path %s contains a symlink", current) - case err != nil && !os.IsNotExist(err): - return fmt.Errorf("check plan file path: %w", err) - } - } - return nil +// planRelativePath returns the workspace-relative plan file path for a +// session. The session ID is slugified to a filesystem-safe alphabet (see +// slugify), so the result can never contain ".." or an absolute path. +func planRelativePath(sessionID string) string { + return filepath.Join(filepath.FromSlash(PlanDirName), slugify(sessionID)+".md") } // slugify turns an arbitrary session identifier into a filesystem-safe slug. diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go index aaadda92d..fe8a1a9be 100644 --- a/internal/planmode/planmode_test.go +++ b/internal/planmode/planmode_test.go @@ -3,6 +3,7 @@ package planmode import ( "os" "path/filepath" + "runtime" "testing" ) @@ -41,6 +42,14 @@ func TestPlanFilePathEmptySessionIsStable(t *testing.T) { } 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 the workspace-scoped os.Root + // resolution in WritePlan/ReadPlan instead, not on file permissions. + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } root := t.TempDir() path, err := WritePlan(root, "session-1", "notes") if err != nil { @@ -90,24 +99,29 @@ func TestReadPlanMissingFileIsNotAnError(t *testing.T) { } } -func TestPlanFilePathRejectsSymlinkedPlansDir(t *testing.T) { +func TestWritePlanRejectsSymlinkedPlansDir(t *testing.T) { root := t.TempDir() outside := t.TempDir() if err := os.MkdirAll(filepath.Join(root, ".zero"), 0o700); err != nil { t.Fatalf("mkdir .zero: %v", err) } // Plant a symlink at .zero/plans pointing outside the workspace, as if an - // attacker (or a stale state) had redirected it before /plan open ran. + // attacker (or stale state) had redirected it before WritePlan ran. Unlike + // a preflight Lstat check, os.Root re-resolves this on every call, so + // planting the symlink right before the call still gets caught. if err := os.Symlink(outside, filepath.Join(root, ".zero", "plans")); err != nil { t.Fatalf("symlink .zero/plans: %v", err) } - if _, err := PlanFilePath(root, "session-1"); err == nil { - t.Fatal("expected PlanFilePath to reject a symlinked plans directory") + if _, err := WritePlan(root, "session-1", "notes"); err == nil { + t.Fatal("expected WritePlan to reject a symlinked plans directory") + } + if _, _, err := ReadPlan(root, "session-1"); err == nil { + t.Fatal("expected ReadPlan to reject a symlinked plans directory") } } -func TestPlanFilePathRejectsSymlinkedPlanFile(t *testing.T) { +func TestWritePlanRejectsSymlinkedPlanFile(t *testing.T) { root := t.TempDir() outsideFile := filepath.Join(t.TempDir(), "exfil.md") if err := os.WriteFile(outsideFile, []byte("secret"), 0o600); err != nil { @@ -122,7 +136,10 @@ func TestPlanFilePathRejectsSymlinkedPlanFile(t *testing.T) { t.Fatalf("symlink plan file: %v", err) } - if _, err := PlanFilePath(root, "session-1"); err == nil { - t.Fatal("expected PlanFilePath to reject a symlinked plan file") + 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") } } diff --git a/internal/tui/model.go b/internal/tui/model.go index cc5549551..42f42d8fb 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -5055,8 +5055,20 @@ func (m model) runAgentWithOptions(runID int, runCtx context.Context, prompt str 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 { + items := reader.CurrentPlan() if m.runtimeMessageSink != nil { - m.runtimeMessageSink(planUpdateMsg{runID: runID, items: reader.CurrentPlan()}) + m.runtimeMessageSink(planUpdateMsg{runID: runID, items: items}) + } + // Persist every update_plan call to the session's plan file: it + // is the single durable source of truth /plan reads from, so a + // plan built entirely through update_plan (the user never ran + // /plan open) still survives a restart/resume, and one seeded by + // /plan open keeps reflecting later agent updates instead of + // showing that first snapshot forever. + 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()}) + } } } } diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 14c275f44..24c11aadc 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -19,7 +19,7 @@ type currentPlanReader interface { // handlePlanCommand toggles plan mode on the current session: // -// /plan toggle plan mode on/off; when on, show the current plan +// /plan toggle plan mode on/off; entering shows the current plan // /plan open open the session's plan file in $VISUAL/$EDITOR // /plan off exit plan mode (alias: /plan exit) // @@ -39,26 +39,36 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Plan mode is not active."}) return m, nil } - m.permissionMode = agent.PermissionModeAuto - if m.permissionModeBeforePlan != "" { - m.permissionMode = m.permissionModeBeforePlan - } - m.permissionModeBeforePlan = "" + m = m.exitPlanMode() m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Exited plan mode. The agent can now implement."}) return m, nil case "open": - return m.openPlanInEditor() + 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() } - // No subcommand: toggle plan mode, then surface the current plan. + // No subcommand: toggle plan mode. A bare /plan while already in plan mode + // exits it (matching the advertised on/off toggle); entering it shows the + // plan that was just seeded. if m.permissionMode == agent.PermissionModePlan { - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.planText()}) + m = m.exitPlanMode() + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Exited plan mode. The agent can now implement."}) return m, nil } 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 } + 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 textToShow := planEnterText(m) + "\n\n" + m.planText() @@ -66,6 +76,19 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { return m, nil } +// 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. +func (m model) exitPlanMode() model { + m.permissionMode = agent.PermissionModeAuto + if m.permissionModeBeforePlan != "" { + m.permissionMode = m.permissionModeBeforePlan + } + m.permissionModeBeforePlan = "" + 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) { @@ -78,7 +101,12 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan path error: " + err.Error()}) return m, nil } - if !fileExists(path) { + _, 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 @@ -121,22 +149,28 @@ func planEnterText(m model) string { planNote = "\nPlan file: " + path } return "Entered plan mode. The agent can inspect the workspace and shape the plan with update_plan, but cannot edit files or run commands until you exit.\n" + - "Use /plan to view the plan, /plan open to edit it, or /plan off to implement." + planNote + "Use /plan open to edit the plan, or /plan (again) / /plan off to implement." + planNote } func (m model) planText() string { - // Prefer the session plan file when present. - if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { - content, readErr := os.ReadFile(path) - switch { - case readErr == nil: - return "Current Plan (plan mode)\n" + path + "\n" + strings.TrimRight(string(content), "\n") - case !os.IsNotExist(readErr): - // 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() + // Prefer the session plan file when present. update_plan persists to this + // file on every call (see model.go's OnToolResult hook), so it is the + // durable 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) + switch { + case 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() + case exists: + header := "Current Plan (plan mode)" + 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. @@ -159,12 +193,18 @@ func (m model) formatPlanDraft() string { if !ok { return "" } - plan := reader.CurrentPlan() - if len(plan) == 0 { + return formatPlanItems(reader.CurrentPlan()) +} + +// 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. +func formatPlanItems(items []tools.PlanItem) string { + if len(items) == 0 { return "" } - lines := make([]string, 0, len(plan)) - for index, item := range plan { + lines := make([]string, 0, len(items)) + for index, item := range items { line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, item.Content) if item.Notes != "" { line += "\n Notes: " + item.Notes @@ -173,8 +213,3 @@ func (m model) formatPlanDraft() string { } return strings.Join(lines, "\n") } - -func fileExists(path string) bool { - _, err := os.Stat(path) - return err == nil -} diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index 1a7d36bc9..ec7c546c9 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -64,6 +64,65 @@ func TestPlanOffRestoresPreviousPermissionMode(t *testing.T) { } } +func TestBarePlanTogglesOff(t *testing.T) { + // Regression: a second bare /plan used to just re-print the current plan + // and leave PermissionModePlan active, contradicting the advertised + // on/off toggle and stranding the user in read-only mode until they + // discovered /plan off. + m := newPlanModeTestModel(t, t.TempDir(), agent.PermissionModeAsk) + m.input.SetValue("/plan") + 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") + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + if next.permissionMode != agent.PermissionModeAsk { + t.Fatalf("expected a second bare /plan to toggle plan mode off, got %s", next.permissionMode) + } + if !transcriptContains(next.transcript, "Exited plan mode") { + t.Fatalf("expected an exit notice in the transcript, got %#v", next.transcript) + } +} + +func TestPlanCommandCreatesSessionBeforeWritingPlanFile(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 used to write the + // plan file under the empty-session slug ("plan.md"), which every other + // fresh session would also reuse and which orphaned its content once the + // real session ID appeared. Entering plan mode must create the session + // first so the plan file is named for it from the start. + 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") + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + + if next.activeSession.SessionID == "" { + t.Fatal("expected /plan 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-enter text to reference the real session's plan file %q, got %#v", path, next.transcript) + } +} + 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 @@ -127,6 +186,61 @@ func TestPlanOpenSeedsFileFromDraft(t *testing.T) { } } +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. + 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) + } +} + func TestPlanModeWiresDraftSystemPrompt(t *testing.T) { provider := &fakeProvider{events: []zeroruntime.StreamEvent{ {Type: zeroruntime.StreamEventText, Content: "planning"}, diff --git a/internal/tui/session.go b/internal/tui/session.go index c5bd62663..dadf8acf6 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -53,6 +53,12 @@ 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. + m = m.exitPlanMode() + m.activeSession = sessions.Metadata{} m.sessionEvents = nil @@ -215,6 +221,12 @@ 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.activeSession = *session m.sessionEvents = append([]sessions.Event{}, events...) if m.providerName == "" { diff --git a/internal/tui/session_test.go b/internal/tui/session_test.go index 96821794f..a2d3ffb14 100644 --- a/internal/tui/session_test.go +++ b/internal/tui/session_test.go @@ -745,6 +745,75 @@ 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) { + 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) + } +} + +func TestResumeDifferentSessionExitsPlanMode(t *testing.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) + } +} + +// 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) { + 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) + } +} + func TestResumePickerExcludesSubRunSessions(t *testing.T) { store := testSessionStore(t) if _, err := store.Create(sessions.CreateInput{Title: "Real Conversation"}); err != nil { From a8a1f96f8c5c60d2c32036b37ccf162c18d1cd8c Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:12:40 -0400 Subject: [PATCH 08/17] fix(tui): guard exitPlanMode against clobbering non-plan permission modes exitPlanMode() unconditionally reset permissionMode to Auto before restoring permissionModeBeforePlan, so /new and /resume to a different session dropped an explicit Ask/Auto choice made outside plan mode. Only touch permissionMode when actually leaving PermissionModePlan. --- internal/tui/plan_command.go | 8 +++++--- internal/tui/session_test.go | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 24c11aadc..5c86818e3 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -81,9 +81,11 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { // 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. func (m model) exitPlanMode() model { - m.permissionMode = agent.PermissionModeAuto - if m.permissionModeBeforePlan != "" { - m.permissionMode = m.permissionModeBeforePlan + if m.permissionMode == agent.PermissionModePlan { + m.permissionMode = agent.PermissionModeAuto + if m.permissionModeBeforePlan != "" { + m.permissionMode = m.permissionModeBeforePlan + } } m.permissionModeBeforePlan = "" return m diff --git a/internal/tui/session_test.go b/internal/tui/session_test.go index a2d3ffb14..b1e0249ae 100644 --- a/internal/tui/session_test.go +++ b/internal/tui/session_test.go @@ -793,6 +793,43 @@ func TestResumeDifferentSessionExitsPlanMode(t *testing.T) { } } +// 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) { + 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) { + 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. From 6fbdf2211e0049d1a931cace655a4a35b102d114 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:12:46 -0400 Subject: [PATCH 09/17] refactor(agent): drop redundant modeName reassignment in tool-denial path string(permissionMode) already yields "plan" and "spec-draft" for the two modes this branch handles, so the if/else was reassigning identical values. --- internal/agent/loop.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/agent/loop.go b/internal/agent/loop.go index 3d9dae9d3..f7e4d4e93 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -974,11 +974,6 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal tool, toolFound := registry.Get(call.Name) if (permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan) && toolFound && !ToolAdvertised(tool, permissionMode) { modeName := string(permissionMode) - if permissionMode == PermissionModePlan { - modeName = "plan" - } else { - modeName = "spec-draft" - } return ToolResult{ ToolCallID: call.ID, Name: call.Name, From d1111e7731b531c35ee0154d90c97b35ac461d4e Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:03:04 -0400 Subject: [PATCH 10/17] fix(tui): reload plan file into memory on editor exit, preserving status and notes /plan open let the user edit the plan file in $EDITOR, but the edit was never synced back into the in-memory update_plan, so it kept driving execution off the stale pre-edit draft. reloadPlanFromFile() now parses the saved file and pushes it back into update_plan via a new SetPlan method. The first version of that parser discarded each item's [status] bracket (resetting everything to pending on reload) and mis-parsed a "Notes: ..." continuation line as its own bogus plan step. Both are fixed: status is parsed back through the tool's existing normalization, and a Notes line folds into the preceding item instead of becoming a new one. --- internal/tools/update_plan.go | 17 +++++-- internal/tui/model.go | 4 ++ internal/tui/plan_command.go | 62 ++++++++++++++++++++++++ internal/tui/plan_command_test.go | 78 +++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 3 deletions(-) diff --git a/internal/tools/update_plan.go b/internal/tools/update_plan.go index 6c1d67cc6..2d04398b1 100644 --- a/internal/tools/update_plan.go +++ b/internal/tools/update_plan.go @@ -85,6 +85,17 @@ 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. +func (tool *updatePlanTool) SetPlan(plan []PlanItem) { + plan = enforceSingleInProgress(plan) + tool.mu.Lock() + tool.currentPlan = plan + tool.mu.Unlock() +} + func (tool *updatePlanTool) ClearPlan() { tool.mu.Lock() tool.currentPlan = nil @@ -125,7 +136,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()) @@ -141,10 +152,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/tui/model.go b/internal/tui/model.go index 42f42d8fb..e61be1f8c 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1115,7 +1115,11 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { case planEditorFinishedMsg: if msg.err != nil { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan editor error: " + msg.err.Error()}) + return m, nil } + // 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. + m.reloadPlanFromFile() return m, nil case exitConfirmExpiredMsg: if msg.seq == m.exitConfirmSeq { diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 5c86818e3..49347d3da 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "os/exec" + "regexp" "strings" tea "charm.land/bubbletea/v2" @@ -13,10 +14,22 @@ import ( "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 } +// 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 toggles plan mode on the current session: // // /plan toggle plan mode on/off; entering shows the current plan @@ -145,6 +158,55 @@ type planEditorFinishedMsg struct { err error } +// 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 or unreadable file +// is left as-is (the in-memory plan remains authoritative). +func (m model) reloadPlanFromFile() { + content, ok, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID) + if err != nil || !ok { + return + } + items := parsePlanFileLines(content) + if writer, ok := m.registry.Get("update_plan"); ok { + if reloader, ok := writer.(planFileReloader); ok { + reloader.SetPlan(items) + } + } +} + +// parsePlanFileLines converts the plain-text plan file the user edits in +// $EDITOR back into plan items. Each numbered line is a 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. A "Notes: ..." line folds into the preceding item's +// Notes field rather than becoming a step of its own. Blank lines are dropped. +func parsePlanFileLines(content string) []tools.PlanItem { + items := make([]tools.PlanItem, 0) + for _, raw := range strings.Split(content, "\n") { + line := strings.TrimSpace(raw) + if line == "" { + continue + } + if notes, ok := strings.CutPrefix(line, "Notes:"); ok { + if len(items) > 0 { + items[len(items)-1].Notes = strings.TrimSpace(notes) + } + continue + } + status := "pending" + if match := numberedStatusRe.FindStringSubmatch(line); match != nil { + if match[1] != "" { + status = tools.NormalizePlanStatus(match[1]) + } + line = strings.TrimSpace(line[len(match[0]):]) + } + items = append(items, tools.PlanItem{Content: line, Status: status}) + } + return items +} + func planEnterText(m model) string { planNote := "" if path, err := planmode.PlanFilePath(m.cwd, m.activeSession.SessionID); err == nil { diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index ec7c546c9..1142aa70c 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -241,6 +241,84 @@ func TestUpdatePlanPersistsToPlanFile(t *testing.T) { } } +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. + 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) + } + m.reloadPlanFromFile() + + 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 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. + 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) + } + + m.reloadPlanFromFile() + + 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 TestPlanModeWiresDraftSystemPrompt(t *testing.T) { provider := &fakeProvider{events: []zeroruntime.StreamEvent{ {Type: zeroruntime.StreamEventText, Content: "planning"}, From 8ef7d1a3fc22f22b8b77a2f6c14ded4e603d33a4 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 22:38:47 -0400 Subject: [PATCH 11/17] fix(tui): correct /plan command palette description The palette showed "/plan - Show planning mode status" but /plan actually toggles plan mode and supports open/off subcommands. --- internal/tui/commands.go | 4 ++-- internal/tui/commands_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/tui/commands.go b/internal/tui/commands.go index db1fad80f..e69f969fc 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -109,9 +109,9 @@ var commandDefinitions = []commandDefinition{ }, { name: "/plan", - usage: "/plan", + usage: "/plan [open|off]", group: commandGroupSession, - description: "Show planning mode status.", + description: "Toggle plan mode, or open the plan file / exit.", kind: commandPlan, }, { diff --git a/internal/tui/commands_test.go b/internal/tui/commands_test.go index 2d937aad2..288f3cb76 100644 --- a/internal/tui/commands_test.go +++ b/internal/tui/commands_test.go @@ -49,7 +49,7 @@ func TestFormatCommandHelpLinesGroupsCommandsByStableOrder(t *testing.T) { " /model [list|id] - Show or switch the active model.", " /effort [list|low|medium|high|auto] - Show or set reasoning effort for supported models.", "session:", - " /plan - Show planning mode status.", + " /plan [open|off] - Toggle plan mode, or open the plan file / exit.", "runtime:", " /permissions - Show the active permission mode and sandbox grants.", " /debug (/debug-mode) - Show debug mode status.", From 4d0afbd6c06ef057af470d278e68e14cf4be5933 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 22:56:00 -0400 Subject: [PATCH 12/17] fix(tui,agent): address plan-mode review findings on gating, reload, and session reset - executeRequestPermissions now denies plan/spec-draft mode unconditionally, instead of relying on the registry-based ToolAdvertised gate, which only fires when the tool happens to be present in whatever registry the caller passed in. - /new and /resume now clear the shared update_plan state and sticky plan panel on a session switch, not just the permission mode. - A successful $EDITOR exit from /plan open now always emits planEditorFinishedMsg, so edited plan content actually reloads instead of being silently dropped. - /plan open now blocks while a run is active, matching the bare /plan toggle's guard. - parsePlanFileLines now folds multi-line Notes blocks instead of treating continuation lines as bogus new steps. --- internal/agent/loop.go | 13 ++++ internal/agent/request_permissions_test.go | 31 ++++++++ internal/tui/model.go | 7 +- internal/tui/plan_command.go | 83 ++++++++++++++++------ internal/tui/plan_command_test.go | 42 +++++++++++ internal/tui/session.go | 4 ++ internal/tui/session_test.go | 24 +++++++ 7 files changed, 181 insertions(+), 23 deletions(-) diff --git a/internal/agent/loop.go b/internal/agent/loop.go index f7e4d4e93..83bbf144d 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1971,6 +1971,19 @@ type requestPermissionsArgs struct { } func executeRequestPermissions(ctx context.Context, call ToolCall, args map[string]any, permissionMode PermissionMode, options Options) (ToolResult, error) { + // request_permissions is dispatched by name above, before the registry-based + // ToolAdvertised gate (which only applies when the tool is found in whatever + // registry the caller passed in). Check the mode here too, unconditionally, + // so a reduced registry that omits this tool can't let a plan/spec-draft turn + // obtain a real, outliving sandbox grant through the back door. + if permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan { + return ToolResult{ + ToolCallID: call.ID, + Name: call.Name, + Status: tools.StatusError, + Output: `Error: Tool "` + call.Name + `" is not available in ` + string(permissionMode) + ` mode.`, + }, nil + } parsed, err := parseRequestPermissionsArgs(args) if err != nil { return ToolResult{ diff --git a/internal/agent/request_permissions_test.go b/internal/agent/request_permissions_test.go index fb12d6e73..6f44905af 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/tui/model.go b/internal/tui/model.go index e61be1f8c..3de2f542b 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1118,8 +1118,11 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } // 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. - m.reloadPlanFromFile() + // the in-memory update_plan so the edited plan drives execution, and + // refresh the sticky plan panel to match. + if items, ok := m.reloadPlanFromFile(); ok { + m.plan.updateFromItems(items, m.now()) + } return m, nil case exitConfirmExpiredMsg: if msg.seq == m.exitConfirmSeq { diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 49347d3da..8dd23fb05 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -56,6 +56,10 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Exited plan mode. The agent can now implement."}) return m, nil 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 + } updated, err := m.ensureActiveSession("") if err != nil { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session error: " + err.Error()}) @@ -104,6 +108,22 @@ func (m model) exitPlanMode() model { return m } +// 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) { @@ -145,10 +165,7 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr return m, tea.ExecProcess(cmd, func(err error) tea.Msg { - if err != nil { - return planEditorFinishedMsg{err: err} - } - return nil + return planEditorFinishedMsg{err: err} }) } @@ -162,11 +179,13 @@ type planEditorFinishedMsg struct { // 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 or unreadable file -// is left as-is (the in-memory plan remains authoritative). -func (m model) reloadPlanFromFile() { +// is left as-is (the in-memory plan remains authoritative). 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) { content, ok, err := planmode.ReadPlan(m.cwd, m.activeSession.SessionID) if err != nil || !ok { - return + return nil, false } items := parsePlanFileLines(content) if writer, ok := m.registry.Get("update_plan"); ok { @@ -174,35 +193,57 @@ func (m model) reloadPlanFromFile() { reloader.SetPlan(items) } } + return items, true } // parsePlanFileLines converts the plain-text plan file the user edits in -// $EDITOR back into plan items. Each numbered line is a 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. A "Notes: ..." line folds into the preceding item's -// Notes field rather than becoming a step of its own. Blank lines are dropped. +// $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. A "Notes: ..." line, and any further +// non-numbered lines that follow it, fold into the preceding item's Notes +// field (joined by newline) rather than becoming steps of their own, so a +// multi-line notes block survives a round-trip through $EDITOR instead of +// shattering into bogus new pending steps. A non-numbered line that does NOT +// follow a "Notes:" line is instead treated as a freeform new step (e.g. a +// line the user added without bothering to number it), matching how earlier +// versions of this parser treated any non-blank line. Blank lines are +// dropped. func parsePlanFileLines(content string) []tools.PlanItem { items := make([]tools.PlanItem, 0) + inNotes := false for _, raw := range strings.Split(content, "\n") { line := strings.TrimSpace(raw) if line == "" { continue } - if notes, ok := strings.CutPrefix(line, "Notes:"); ok { - if len(items) > 0 { - items[len(items)-1].Notes = strings.TrimSpace(notes) - } - continue - } - status := "pending" if match := numberedStatusRe.FindStringSubmatch(line); match != nil { + status := "pending" if match[1] != "" { status = tools.NormalizePlanStatus(match[1]) } - line = strings.TrimSpace(line[len(match[0]):]) + items = append(items, tools.PlanItem{ + Content: strings.TrimSpace(line[len(match[0]):]), + Status: status, + }) + inNotes = false + continue + } + if notes, ok := strings.CutPrefix(line, "Notes:"); ok && len(items) > 0 { + items[len(items)-1].Notes = strings.TrimSpace(notes) + inNotes = true + continue + } + if inNotes && len(items) > 0 { + last := &items[len(items)-1] + if last.Notes == "" { + last.Notes = line + } else { + last.Notes += "\n" + line + } + continue } - items = append(items, tools.PlanItem{Content: line, Status: status}) + items = append(items, tools.PlanItem{Content: line, Status: "pending"}) } return items } diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index 1142aa70c..fa8990463 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -64,6 +64,23 @@ func TestPlanOffRestoresPreviousPermissionMode(t *testing.T) { } } +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 := newPlanModeTestModel(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 TestBarePlanTogglesOff(t *testing.T) { // Regression: a second bare /plan used to just re-print the current plan // and leave PermissionModePlan active, contradicting the advertised @@ -319,6 +336,31 @@ func TestPlanOpenEditorReloadPreservesStatusAndNotes(t *testing.T) { } } +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"}, diff --git a/internal/tui/session.go b/internal/tui/session.go index dadf8acf6..f1f8e547a 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -57,7 +57,10 @@ func (m model) startNewSession() model { // 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.sessionEvents = nil @@ -226,6 +229,7 @@ func (m model) handleResumeCommand(args string) (model, string) { // 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.sessionEvents = append([]sessions.Event{}, events...) diff --git a/internal/tui/session_test.go b/internal/tui/session_test.go index b1e0249ae..c545b498b 100644 --- a/internal/tui/session_test.go +++ b/internal/tui/session_test.go @@ -765,6 +765,30 @@ func TestNewSessionExitsPlanMode(t *testing.T) { } } +// 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) { + 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) { store := testSessionStore(t) active, err := store.Create(sessions.CreateInput{Title: "Active"}) From fbd0261fd70f68518e68a9077ca007a80d60ce32 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 23:41:40 -0400 Subject: [PATCH 13/17] fix(tui,planmode): close editor symlink race, propagate edits to context, other findings - /plan open now stages the plan file for $EDITOR in config.UserConfigDir() instead of handing it a workspace-relative path: ReadPlan/WritePlan resolve through os.Root and can't be redirected, but the external editor process opens its argument path with ordinary I/O, so a sandboxed tool invocation could previously replace the plan file with a symlink between our protected write and the editor's open. The OS temp directory doesn't avoid this since the sandbox's default write scope explicitly includes it. - A user-edited plan now gets recorded as a session event on reload, so it actually reaches the model's context instead of only updating the update_plan tool's in-memory state, which the model has no way to observe on its own. - /resume now hydrates the destination session's own persisted plan file after a session switch, instead of leaving update_plan and the sticky panel empty until the next update_plan call risks overwriting it. - formatPlanItems/parsePlanFileLines now indent multi-line Content continuations the same way Notes continuations already were, so agent-authored multi-line plan steps survive a round-trip through $EDITOR instead of shattering into bogus new pending steps. - WritePlan now Chmods the plan directory and file unconditionally after MkdirAll/OpenFile, since those only apply their mode at creation and would otherwise leave a pre-existing, more permissive dir/file broadly readable. - /plan open now checks plan mode is active before ensureActiveSession instead of after, so an invalid invocation doesn't leave a persistent empty session behind in /resume. --- internal/planmode/planmode.go | 82 +++++++++++++++++++++++++- internal/planmode/planmode_test.go | 39 ++++++++++++ internal/tui/model.go | 22 ++++++- internal/tui/plan_command.go | 95 ++++++++++++++++++++++-------- internal/tui/plan_command_test.go | 49 +++++++++++++++ internal/tui/session.go | 10 ++++ 6 files changed, 269 insertions(+), 28 deletions(-) diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index ea6d502db..b6b88a842 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/Gitlawb/zero/internal/config" ) // PlanDirName is the workspace-relative directory where /plan plan files live, @@ -78,20 +80,96 @@ func WritePlan(workspaceRoot, sessionID, content string) (string, error) { } defer root.Close() - if err := root.MkdirAll(filepath.FromSlash(PlanDirName), 0o700); err != nil { + dirRelPath := filepath.FromSlash(PlanDirName) + if err := root.MkdirAll(dirRelPath, 0o700); err != nil { return "", fmt.Errorf("create plan directory: %w", err) } - file, err := root.OpenFile(planRelativePath(sessionID), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + // MkdirAll's mode only applies at creation: it does not tighten an + // already-existing, more permissive directory (e.g. one predating this + // restriction, or created some other way). Chmod unconditionally so a + // pre-existing 0755 directory is brought back to owner-only on every + // write, matching the storage contract. + if err := root.Chmod(dirRelPath, 0o700); err != nil { + return "", fmt.Errorf("restrict plan directory permissions: %w", err) + } + fileRelPath := planRelativePath(sessionID) + file, err := root.OpenFile(fileRelPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) if err != nil { return "", fmt.Errorf("write plan file: %w", err) } defer file.Close() + // Same reasoning as the directory Chmod above: OpenFile's mode only + // applies when it creates the file, so a pre-existing 0644 plan file + // would otherwise stay group/other-readable. + if err := root.Chmod(fileRelPath, 0o600); err != nil { + return "", fmt.Errorf("restrict plan file permissions: %w", err) + } if _, err := file.WriteString(strings.TrimRight(content, "\n") + "\n"); err != nil { return "", fmt.Errorf("write plan file: %w", err) } return PlanFilePath(workspaceRoot, sessionID) } +// 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 inside the workspace itself would leave a +// symlink-swap race: ReadPlan/WritePlan resolve descriptor-relative through +// os.Root and cannot be redirected, but the external editor process opens +// its argument path with its own ordinary (non-Root) I/O, so a sandboxed +// tool invocation could replace the plan file with a symlink between our +// protected write and the editor's open, causing the editor (which runs +// unsandboxed, under the real user) to follow it and edit an arbitrary +// user-writable target. The OS temp directory does not avoid this: 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 not +// part of that default scope, so a sandboxed process cannot pre-stage a +// symlink at the path this creates. +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 + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return "", nil, fmt.Errorf("create plan editor staging directory: %w", err) + } + path := filepath.Join(dir, slugify(sessionID)+".md") + if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o600); err != nil { + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + return path, func() { _ = os.Remove(path) }, nil +} + +// CommitStagedEdit reads a file staged by StageForEditor (now edited by the +// user's $EDITOR) and writes its content back into the workspace via +// WritePlan, which is the safe, descriptor-relative path back through +// os.Root. +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 +} + // openWorkspaceRoot opens the workspace directory as an os.Root, which the // Go runtime resolves relative to using descriptor-relative (openat-style) // operations: every subsequent Root method call re-walks the path from that diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go index fe8a1a9be..97158190b 100644 --- a/internal/planmode/planmode_test.go +++ b/internal/planmode/planmode_test.go @@ -71,6 +71,45 @@ func TestWritePlanUsesRestrictivePermissions(t *testing.T) { } } +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") + } + root := t.TempDir() + planDir := filepath.Join(root, PlanDirName) + if err := os.MkdirAll(planDir, 0o755); err != nil { + t.Fatalf("pre-create loose plan dir: %v", err) + } + planFile := filepath.Join(planDir, "session-1.md") + if err := os.WriteFile(planFile, []byte("stale"), 0o644); err != nil { + t.Fatalf("pre-create loose plan file: %v", err) + } + + 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 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 TestReadWritePlanRoundtrip(t *testing.T) { root := t.TempDir() if _, err := WritePlan(root, "session-1", "# Draft\n\nStep one."); err != nil { diff --git a/internal/tui/model.go b/internal/tui/model.go index 3de2f542b..ecf2a131d 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1120,8 +1120,26 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { // 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. - if items, ok := m.reloadPlanFromFile(); ok { - m.plan.updateFromItems(items, m.now()) + items, ok := m.reloadPlanFromFile() + if !ok { + return m, nil + } + m.plan.updateFromItems(items, m.now()) + // 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. + if plan := formatPlanItems(items); plan != "" { + var err error + m, err = m.appendSessionEvent(sessions.EventMessage, map[string]any{ + "role": "user", + "content": "I edited the plan file directly. Updated plan:\n\n" + plan, + }) + if err != nil { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session record error: " + err.Error()}) + } } return m, nil case exitConfirmExpiredMsg: diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 8dd23fb05..f46137681 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -60,6 +60,14 @@ func (m model) handlePlanCommand(text string) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "Cannot open the plan file while a run is active."}) return m, nil } + // Validate plan mode is active before ensureActiveSession, not after: + // openPlanInEditor rejects this same condition, but by then a session + // would already have been created for what should be a pure no-op + // error, leaving a persistent empty session behind in /resume. + if m.permissionMode != agent.PermissionModePlan { + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Enter plan mode (/plan) 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()}) @@ -159,13 +167,30 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { 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 + } parts := strings.Fields(editor) - cmd := exec.Command(parts[0], append(parts[1:], path)...) //nolint:gosec // editor path from $VISUAL/$EDITOR + 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 { - return planEditorFinishedMsg{err: err} + 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} }) } @@ -200,50 +225,59 @@ func (m model) reloadPlanFromFile() ([]tools.PlanItem, bool) { // $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. A "Notes: ..." line, and any further -// non-numbered lines that follow it, fold into the preceding item's Notes -// field (joined by newline) rather than becoming steps of their own, so a -// multi-line notes block survives a round-trip through $EDITOR instead of -// shattering into bogus new pending steps. A non-numbered line that does NOT -// follow a "Notes:" line is instead treated as a freeform new step (e.g. a -// line the user added without bothering to number it), matching how earlier -// versions of this parser treated any non-blank line. Blank lines are -// dropped. +// edit instead of resetting to pending. +// +// An indented line (formatPlanItems writes continuations with a leading +// " ") folds into the current item instead of becoming a step of its own: +// before a "Notes: ..." line it extends the item's (possibly multi-line) +// Content, and a "Notes: ..." line plus any indented lines after it extend +// the item's Notes. This lets both multi-line Content and multi-line Notes +// round-trip through $EDITOR instead of shattering into bogus new pending +// steps. A non-numbered line with NO leading indentation is instead treated +// as a freeform new step (e.g. one the user typed without bothering to +// number or indent it), matching how earlier versions of this parser treated +// any non-blank line. Blank lines are dropped. func parsePlanFileLines(content string) []tools.PlanItem { items := make([]tools.PlanItem, 0) inNotes := false for _, raw := range strings.Split(content, "\n") { - line := strings.TrimSpace(raw) - if line == "" { + trimmed := strings.TrimSpace(raw) + if trimmed == "" { continue } - if match := numberedStatusRe.FindStringSubmatch(line); match != nil { + 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(line[len(match[0]):]), + Content: strings.TrimSpace(trimmed[len(match[0]):]), Status: status, }) inNotes = false continue } - if notes, ok := strings.CutPrefix(line, "Notes:"); ok && len(items) > 0 { - items[len(items)-1].Notes = strings.TrimSpace(notes) + indented := raw != trimmed + if !indented || len(items) == 0 { + items = append(items, tools.PlanItem{Content: trimmed, Status: "pending"}) + inNotes = false + continue + } + last := &items[len(items)-1] + if notes, ok := strings.CutPrefix(trimmed, "Notes:"); ok { + last.Notes = strings.TrimSpace(notes) inNotes = true continue } - if inNotes && len(items) > 0 { - last := &items[len(items)-1] + if inNotes { if last.Notes == "" { - last.Notes = line + last.Notes = trimmed } else { - last.Notes += "\n" + line + last.Notes += "\n" + trimmed } continue } - items = append(items, tools.PlanItem{Content: line, Status: "pending"}) + last.Content += "\n" + trimmed } return items } @@ -304,15 +338,28 @@ func (m model) formatPlanDraft() string { // 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(items)) for index, item := range items { - line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, item.Content) + contentLines := strings.Split(item.Content, "\n") + line := fmt.Sprintf("%d. [%s] %s", index+1, item.Status, contentLines[0]) + for _, cont := range contentLines[1:] { + line += "\n " + 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 " + cont + } } lines = append(lines, line) } diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index fa8990463..470cf6085 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -64,6 +64,31 @@ func TestPlanOffRestoresPreviousPermissionMode(t *testing.T) { } } +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) 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 @@ -336,6 +361,30 @@ func TestPlanOpenEditorReloadPreservesStatusAndNotes(t *testing.T) { } } +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 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 diff --git a/internal/tui/session.go b/internal/tui/session.go index f1f8e547a..fee4cc59e 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -232,6 +232,16 @@ func (m model) handleResumeCommand(args string) (model, string) { m = m.resetPlanForSessionSwitch() } m.activeSession = *session + 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. + if items, ok := m.reloadPlanFromFile(); ok { + m.plan.updateFromItems(items, m.now()) + } + } m.sessionEvents = append([]sessions.Event{}, events...) if m.providerName == "" { m.providerName = session.Provider From 5cb34106a0eae8b84b28ca3c8226fa169f81745e Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:49:50 -0400 Subject: [PATCH 14/17] fix(planmode,tui): harden editor staging and record cleared plans - StageForEditor rejects a staging directory that XDG_CONFIG_HOME has redirected into the sandbox's default-writable roots (the workspace or the OS temp directory) instead of silently staging somewhere a sandboxed process could symlink-swap. - The staged file is created per invocation via os.CreateTemp: a random, unpredictable name opened with O_EXCL, so a planted path is refused rather than followed, and two Zero instances editing the same resumed session no longer overwrite each other's staged draft. Cleanup removes only the file this invocation created. - Clearing every line in the editor now records an explicit plan-cleared user event in the session context, so the next run does not replay the discarded plan from the earlier update_plan call. - $VISUAL/$EDITOR values are parsed with POSIX shell word-splitting (mvdan.cc/sh/v3/shell, already a dependency) instead of strings.Fields, so quoted executable paths with spaces and flags launch correctly. - The /plan palette description says the literal "off" subcommand, and the help expectation matches. --- internal/planmode/planmode.go | 67 +++++++++++++++++++-- internal/planmode/planmode_test.go | 96 ++++++++++++++++++++++++++++++ internal/tui/commands.go | 2 +- internal/tui/commands_test.go | 2 +- internal/tui/model.go | 18 +++--- internal/tui/plan_command.go | 13 +++- 6 files changed, 182 insertions(+), 16 deletions(-) diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index b6b88a842..18a580f3c 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -124,9 +124,21 @@ func WritePlan(workspaceRoot, sessionID, content string) (string, error) { // user-writable target. The OS temp directory does not avoid this: 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 not -// part of that default scope, so a sandboxed process cannot pre-stage a -// symlink at the path this creates. +// 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 { @@ -136,16 +148,61 @@ func StageForEditor(workspaceRoot, sessionID string) (stagedPath string, cleanup if err != nil { return "", nil, err } + if !editorStagingDirIsPrivate(dir, workspaceRoot) { + return "", nil, fmt.Errorf("plan editor staging directory %s is inside a default sandbox-writable root (the workspace or the OS temp directory); check XDG_CONFIG_HOME", dir) + } + return stageContentForEditor(dir, sessionID, content) +} + +// stageContentForEditor creates a fresh, uniquely-named file under dir +// holding content, for StageForEditor to hand to $EDITOR. Split out from +// StageForEditor so the staging mechanics (CreateTemp, O_EXCL) are testable +// against an arbitrary directory without needing to fake config.UserConfigDir +// or XDG_CONFIG_HOME; the privacy check above is StageForEditor's job, not +// this function's. +func stageContentForEditor(dir, sessionID, content string) (stagedPath string, cleanup func(), err error) { if err := os.MkdirAll(dir, 0o700); err != nil { return "", nil, fmt.Errorf("create plan editor staging directory: %w", err) } - path := filepath.Join(dir, slugify(sessionID)+".md") - if err := os.WriteFile(path, []byte(strings.TrimRight(content, "\n")+"\n"), 0o600); err != nil { + file, err := os.CreateTemp(dir, slugify(sessionID)+"-*.md") + if err != nil { + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + path := file.Name() + if _, err := file.WriteString(strings.TrimRight(content, "\n") + "\n"); err != nil { + _ = file.Close() + _ = os.Remove(path) + return "", nil, fmt.Errorf("stage plan file for editor: %w", err) + } + if err := file.Close(); err != nil { + _ = os.Remove(path) return "", nil, fmt.Errorf("stage plan file for editor: %w", err) } return path, func() { _ = os.Remove(path) }, nil } +// editorStagingDirIsPrivate reports whether dir avoids the sandbox's default +// writable roots (the OS temp directory and the workspace itself), which are +// writable from inside the sandbox by default regardless of any extra grant. +func editorStagingDirIsPrivate(dir, workspaceRoot string) bool { + if isUnderOrEqual(dir, os.TempDir()) { + return false + } + if absRoot, err := filepath.Abs(workspaceRoot); err == nil && isUnderOrEqual(dir, absRoot) { + return false + } + return true +} + +// 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 workspace via // WritePlan, which is the safe, descriptor-relative path back through diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go index 97158190b..6639b304e 100644 --- a/internal/planmode/planmode_test.go +++ b/internal/planmode/planmode_test.go @@ -182,3 +182,99 @@ func TestWritePlanRejectsSymlinkedPlanFile(t *testing.T) { t.Fatal("expected ReadPlan to reject a symlinked plan file") } } + +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) { + 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) { + 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) { + 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) { + t.Fatalf("expected %q to be accepted as private", dir) + } +} + +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) + } +} diff --git a/internal/tui/commands.go b/internal/tui/commands.go index e69f969fc..9f9dafb52 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -111,7 +111,7 @@ var commandDefinitions = []commandDefinition{ name: "/plan", usage: "/plan [open|off]", group: commandGroupSession, - description: "Toggle plan mode, or open the plan file / exit.", + description: "Toggle plan mode, open the plan file, or turn plan mode off.", kind: commandPlan, }, { diff --git a/internal/tui/commands_test.go b/internal/tui/commands_test.go index 288f3cb76..145c2307f 100644 --- a/internal/tui/commands_test.go +++ b/internal/tui/commands_test.go @@ -49,7 +49,7 @@ func TestFormatCommandHelpLinesGroupsCommandsByStableOrder(t *testing.T) { " /model [list|id] - Show or switch the active model.", " /effort [list|low|medium|high|auto] - Show or set reasoning effort for supported models.", "session:", - " /plan [open|off] - Toggle plan mode, or open the plan file / exit.", + " /plan [open|off] - Toggle plan mode, open the plan file, or turn plan mode off.", "runtime:", " /permissions - Show the active permission mode and sandbox grants.", " /debug (/debug-mode) - Show debug mode status.", diff --git a/internal/tui/model.go b/internal/tui/model.go index ecf2a131d..72eec54a2 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1131,15 +1131,17 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { // 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 != "" { - var err error - m, err = m.appendSessionEvent(sessions.EventMessage, map[string]any{ - "role": "user", - "content": "I edited the plan file directly. Updated plan:\n\n" + plan, - }) - if err != nil { - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session record error: " + err.Error()}) - } + 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: diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index f46137681..27176291c 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -8,6 +8,7 @@ import ( "strings" tea "charm.land/bubbletea/v2" + "mvdan.cc/sh/v3/shell" "github.com/Gitlawb/zero/internal/agent" "github.com/Gitlawb/zero/internal/planmode" @@ -175,7 +176,17 @@ func (m model) openPlanInEditor() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "plan stage error: " + err.Error()}) return m, nil } - parts := strings.Fields(editor) + // $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. + parts, err := shell.Fields(editor, os.Getenv) + 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 From 6ab244aa9f76afa7fa48981764d2c3b626ee5947 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:40:00 -0400 Subject: [PATCH 15/17] fix(planmode,tui,tools): physical staging containment, cancel-safe plan state, lossless plan encoding - The editor staging containment check now judges physical paths: the staging directory is created first, resolved with EvalSymlinks, checked against the symlink-resolved workspace and temp roots, and the staging itself is anchored on the resolved path. An XDG_CONFIG_HOME symlinked into a sandbox-writable root no longer passes on its lexical spelling. - update_plan refuses to apply once its run context is cancelled, with the check sharing the mutex that guards SetPlan, so a cancelled run's late call can no longer repopulate the plan the UI just reset for a new session; the UI-side file sync also runs only on successful results, so a refused call cannot rewrite the old session's plan file either. - The plan file encoding round-trips losslessly: indentation is decided before content (a continuation reading "2. validate" stays a continuation), continuations whose text would read as structure ("Notes:" or a leading backslash) are escaped, and whitespace-only indented lines survive as blank continuation lines. Round-trip tests cover the adversarial cases and assert a fixed point on the second pass. --- internal/planmode/planmode.go | 47 +++++++++++--- internal/planmode/planmode_test.go | 72 ++++++++++++++++++++-- internal/tools/update_plan.go | 14 ++++- internal/tools/update_plan_test.go | 28 +++++++++ internal/tui/model.go | 8 ++- internal/tui/plan_command.go | 98 +++++++++++++++++++----------- internal/tui/plan_command_test.go | 35 +++++++++++ 7 files changed, 251 insertions(+), 51 deletions(-) create mode 100644 internal/tools/update_plan_test.go diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index 18a580f3c..9def0a49c 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -148,10 +148,25 @@ func StageForEditor(workspaceRoot, sessionID string) (stagedPath string, cleanup if err != nil { return "", nil, err } - if !editorStagingDirIsPrivate(dir, workspaceRoot) { - return "", nil, fmt.Errorf("plan editor staging directory %s is inside a default sandbox-writable root (the workspace or the OS temp directory); check XDG_CONFIG_HOME", dir) + // 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 := filepath.EvalSymlinks(dir) + if err != nil { + return "", nil, fmt.Errorf("resolve plan editor staging directory: %w", err) } - return stageContentForEditor(dir, sessionID, content) + if !editorStagingDirIsPrivate(resolvedDir, workspaceRoot, os.TempDir()) { + 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) + } + return stageContentForEditor(resolvedDir, sessionID, content) } // stageContentForEditor creates a fresh, uniquely-named file under dir @@ -182,18 +197,34 @@ func stageContentForEditor(dir, sessionID, content string) (stagedPath string, c } // editorStagingDirIsPrivate reports whether dir avoids the sandbox's default -// writable roots (the OS temp directory and the workspace itself), which are -// writable from inside the sandbox by default regardless of any extra grant. -func editorStagingDirIsPrivate(dir, workspaceRoot string) bool { - if isUnderOrEqual(dir, os.TempDir()) { +// 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 } - if absRoot, err := filepath.Abs(workspaceRoot); err == nil && isUnderOrEqual(dir, absRoot) { + if absRoot, err := filepath.Abs(workspaceRoot); err == nil && isUnderOrEqual(dir, physicalPath(absRoot)) { return false } return true } +// physicalPath resolves symlinks best-effort: a path that cannot be resolved +// (not existing yet) is compared as spelled. +func physicalPath(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + return path +} + // isUnderOrEqual reports whether path is root itself or a descendant of it. func isUnderOrEqual(path, root string) bool { rel, err := filepath.Rel(root, path) diff --git a/internal/planmode/planmode_test.go b/internal/planmode/planmode_test.go index 6639b304e..d3f1d500d 100644 --- a/internal/planmode/planmode_test.go +++ b/internal/planmode/planmode_test.go @@ -189,7 +189,7 @@ func TestEditorStagingDirIsPrivateRejectsOSTempDir(t *testing.T) { // 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) { + if editorStagingDirIsPrivate(dir, workspaceRoot, os.TempDir()) { t.Fatalf("expected %q (under the OS temp dir) to be rejected", dir) } } @@ -197,11 +197,11 @@ func TestEditorStagingDirIsPrivateRejectsOSTempDir(t *testing.T) { func TestEditorStagingDirIsPrivateRejectsWorkspaceDir(t *testing.T) { workspaceRoot := t.TempDir() dir := filepath.Join(workspaceRoot, ".config", "zero", "plan-edit") - if editorStagingDirIsPrivate(dir, workspaceRoot) { + 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) { + if editorStagingDirIsPrivate(workspaceRoot, workspaceRoot, os.TempDir()) { t.Fatal("expected the workspace root itself to be rejected") } } @@ -214,11 +214,75 @@ func TestEditorStagingDirIsPrivateAcceptsElsewhere(t *testing.T) { 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) { + 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.") diff --git a/internal/tools/update_plan.go b/internal/tools/update_plan.go index 2d04398b1..be1f95043 100644 --- a/internal/tools/update_plan.go +++ b/internal/tools/update_plan.go @@ -67,15 +67,25 @@ 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)) } diff --git a/internal/tools/update_plan_test.go b/internal/tools/update_plan_test.go new file mode 100644 index 000000000..bff71dc1c --- /dev/null +++ b/internal/tools/update_plan_test.go @@ -0,0 +1,28 @@ +package tools + +import ( + "context" + "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()) + if result := tool.Run(ctx, map[string]any{"plan": []any{map[string]any{"content": "live"}}}); result.Status != StatusOK { + t.Fatalf("live run: %+v", result) + } + + 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 items := tool.CurrentPlan(); len(items) != 0 { + t.Fatalf("cancelled run repopulated the shared plan: %+v", items) + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 72eec54a2..2a6f49e4b 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -5078,8 +5078,12 @@ func (m model) runAgentWithOptions(runID int, runCtx context.Context, prompt str rows = append(rows, row) m.sendAgentRow(runID, row) } - // Sync the sticky plan panel when update_plan runs. - if result.Name == "update_plan" && m.registry != nil { + // Sync the sticky plan panel when update_plan runs. 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 && m.registry != nil { if planTool, ok := m.registry.Get("update_plan"); ok { if reader, ok := planTool.(interface{ CurrentPlan() []tools.PlanItem }); ok { items := reader.CurrentPlan() diff --git a/internal/tui/plan_command.go b/internal/tui/plan_command.go index 27176291c..ec8dbaff6 100644 --- a/internal/tui/plan_command.go +++ b/internal/tui/plan_command.go @@ -238,61 +238,86 @@ func (m model) reloadPlanFromFile() ([]tools.PlanItem, bool) { // Status (matching formatPlanItems) so completed/in-progress steps survive an // edit instead of resetting to pending. // -// An indented line (formatPlanItems writes continuations with a leading -// " ") folds into the current item instead of becoming a step of its own: -// before a "Notes: ..." line it extends the item's (possibly multi-line) -// Content, and a "Notes: ..." line plus any indented lines after it extend -// the item's Notes. This lets both multi-line Content and multi-line Notes -// round-trip through $EDITOR instead of shattering into bogus new pending -// steps. A non-numbered line with NO leading indentation is instead treated -// as a freeform new step (e.g. one the user typed without bothering to -// number or indent it), matching how earlier versions of this parser treated -// any non-blank line. Blank lines are dropped. +// 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) - 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 - } - indented := raw != trimmed + 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 } last := &items[len(items)-1] - if notes, ok := strings.CutPrefix(trimmed, "Notes:"); ok { - last.Notes = strings.TrimSpace(notes) - inNotes = true - continue + if !inNotes { + if notes, ok := strings.CutPrefix(trimmed, "Notes:"); ok { + last.Notes = strings.TrimSpace(notes) + inNotes = true + continue + } } + line := unescapePlanContinuation(trimmed) if inNotes { if last.Notes == "" { - last.Notes = trimmed + last.Notes = line } else { - last.Notes += "\n" + trimmed + last.Notes += "\n" + line } continue } - last.Content += "\n" + trimmed + 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 { @@ -362,14 +387,17 @@ func formatPlanItems(items []tools.PlanItem) string { 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 " + cont + line += "\n " + escapePlanContinuation(cont) } if item.Notes != "" { noteLines := strings.Split(item.Notes, "\n") line += "\n Notes: " + noteLines[0] for _, cont := range noteLines[1:] { - line += "\n " + cont + line += "\n " + escapePlanContinuation(cont) } } lines = append(lines, line) diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index 470cf6085..b92cbdd21 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -385,6 +385,41 @@ func TestPlanItemsRoundTripMultilineContent(t *testing.T) { } } +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 From b4250204fd775f1242e1411a831f32dbb915164a Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:08:20 -0400 Subject: [PATCH 16/17] fix(planmode): resolve not-yet-existing paths through their deepest existing ancestor The macOS and Windows CI runners spell temp paths through symlinks (/var -> /private/var) and 8.3 short names (RUNNER~1): a staging directory that does not exist yet kept its lexical spelling while the existing roots resolved to physical form, so the containment comparison silently missed. physicalPath now resolves the deepest existing ancestor and rejoins the remainder, giving both sides the same spelling. --- internal/planmode/planmode.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/internal/planmode/planmode.go b/internal/planmode/planmode.go index 9def0a49c..3da476ebb 100644 --- a/internal/planmode/planmode.go +++ b/internal/planmode/planmode.go @@ -216,13 +216,23 @@ func editorStagingDirIsPrivate(dir, workspaceRoot, tempDir string) bool { return true } -// physicalPath resolves symlinks best-effort: a path that cannot be resolved -// (not existing yet) is compared as spelled. +// 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. func physicalPath(path string) string { if resolved, err := filepath.EvalSymlinks(path); err == nil { return resolved } - return path + 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. From 2e6101c80ce80218c80c0cb3c21af4b1ea1b333d Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:22:02 -0400 Subject: [PATCH 17/17] fix(tui): confirm plan reload in transcript after editor exit The planEditorFinishedMsg handler reloaded the edited plan file into both the update_plan tool and the sticky panel, but emitted no visible confirmation, so a bare /plan open with no other change looked like a no-op. Append a system message noting the reload (or a clear when the edited file is empty), and cover the full Update message path with a test asserting the tool state, panel, and transcript are all updated. --- internal/tui/model.go | 8 ++++++ internal/tui/plan_command_test.go | 43 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/internal/tui/model.go b/internal/tui/model.go index 2a6f49e4b..2a7c0d590 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1125,6 +1125,14 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { 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 diff --git a/internal/tui/plan_command_test.go b/internal/tui/plan_command_test.go index b92cbdd21..ece61b053 100644 --- a/internal/tui/plan_command_test.go +++ b/internal/tui/plan_command_test.go @@ -322,6 +322,49 @@ func TestPlanOpenEditorExitReloadsFileIntoPlan(t *testing.T) { } } +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. + 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) + } +} + func TestPlanOpenEditorReloadPreservesStatusAndNotes(t *testing.T) { // Regression: parsePlanFileLines used to discard the "[status]" bracket // (resetting every reloaded item to "pending") and treat a "Notes: ..."