feat(tui): add plan mode command and fix plan file editing#643
feat(tui): add plan mode command and fix plan file editing#643euxaristia wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPlan mode adds a session-scoped permission mode, secure ChangesPlan mode workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant planmode
participant Editor
participant Agent
User->>TUI: submit /plan open
TUI->>planmode: seed or read session plan
TUI->>Editor: launch editor with plan path
Editor-->>TUI: return completion
User->>TUI: submit prompt
TUI->>Agent: run with plan-mode prompt
Agent-->>TUI: return update_plan result
TUI->>planmode: persist updated plan
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/tui/plan_command.go (3)
154-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fileExistsreturning(struct{}, bool)is dead weight.The
struct{}value is never used at the call site (Line 77). Return a plainbool. Also noteos.Staterrors other than "not exist" (e.g. permission) are collapsed into "missing", which would then trigger an unnecessaryWritePlan; considererrors.Is(err, os.ErrNotExist)if that distinction matters.♻️ Simplify
-func fileExists(path string) (struct{}, bool) { - _, err := os.Stat(path) - if err != nil { - return struct{}{}, false - } - return struct{}{}, true -} +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +}- if _, ok := fileExists(path); !ok { + if !fileExists(path) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 154 - 160, Update fileExists to return only a bool, and adjust its call site to use the simplified result. Use errors.Is(err, os.ErrNotExist) to distinguish a missing file from other os.Stat failures, preserving non-not-found errors instead of treating them as absent.
116-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant path resolution in
planText.
PlanFilePathis called to obtainpath, thenReadPlanre-resolves it internally; theif path != ""at Line 121 is also already implied by the Line 118 guard. Minor, but the double resolution can be collapsed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 116 - 126, Refactor model.planText to avoid resolving the plan path twice: use a single planmode.PlanFilePath result to determine and read the plan, or update the read flow to pass the already-resolved path where supported. Remove the redundant if path != "" check inside the successful branch while preserving the existing header and content behavior.
101-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEditor failures are silently swallowed.
The
tea.ExecProcesscallback discardserr, so a missing/misconfigured editor or a non-zero exit leaves the user with no feedback — the TUI just resumes as if nothing happened. Surface it via a transcript message.♻️ Report editor errors back to the transcript
- return m, tea.ExecProcess(cmd, func(err error) tea.Msg { - return nil - }) + return m, tea.ExecProcess(cmd, func(err error) tea.Msg { + if err != nil { + return editorFinishedMsg{err: err} + } + return nil + })Handle
editorFinishedMsginUpdateto append an error row. Please also confirm thetea.ExecProcess(cmd, func(error) tea.Msg)signature for bubbletea v2.0.7.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 101 - 103, Editor failures are discarded in the tea.ExecProcess callback. In the editor-launching method, return the received error as an editorFinishedMsg (confirming Bubble Tea v2.0.7’s callback signature), then handle editorFinishedMsg in Update by appending a transcript error row; preserve successful completion without adding an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/plan_command.go`:
- Line 39: Add the missing PermissionModePlan constant to the agent package
alongside PermissionModeAuto and PermissionModeUnsafe, using the existing
permission-mode type and value conventions; verify all references in
plan_command.go compile successfully.
- Line 12: The import of internal/planmode in plan_command.go cannot resolve
because the package is missing or has an invalid declaration. Ensure the
planmode package files are included and declare a valid package name matching
the import, then verify references from plan_command.go compile successfully.
---
Nitpick comments:
In `@internal/tui/plan_command.go`:
- Around line 154-160: Update fileExists to return only a bool, and adjust its
call site to use the simplified result. Use errors.Is(err, os.ErrNotExist) to
distinguish a missing file from other os.Stat failures, preserving non-not-found
errors instead of treating them as absent.
- Around line 116-126: Refactor model.planText to avoid resolving the plan path
twice: use a single planmode.PlanFilePath result to determine and read the plan,
or update the read flow to pass the already-resolved path where supported.
Remove the redundant if path != "" check inside the successful branch while
preserving the existing header and content behavior.
- Around line 101-103: Editor failures are discarded in the tea.ExecProcess
callback. In the editor-launching method, return the received error as an
editorFinishedMsg (confirming Bubble Tea v2.0.7’s callback signature), then
handle editorFinishedMsg in Update by appending a transcript error row; preserve
successful completion without adding an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 328b2d08-881f-4c6a-ba61-2065b0525cc0
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/plan_command.gointernal/tui/run.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/planmode/planmode.go`:
- Around line 81-84: Use owner-only permissions for plan storage: change the
directory mode in the plan-writing function to 0o700 and the file mode passed to
os.WriteFile to 0o600, ensuring existing plan paths are also not left broadly
accessible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 403fa48e-d25a-4818-9899-41686bac7419
📒 Files selected for processing (1)
internal/planmode/planmode.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The scoping on plan-file editing is sound — /plan open only opens the single deterministic .zero/plans/<session>.md (path contained via filepath.Rel checks, never clobbered if it exists) as a user-initiated editor session, not an agent write grant, so it doesn't punch a hole in the read-only gate. The on/off toggle and the run-active guard look right.
The blocker: this PR doesn't build against main. plan_command.go references agent.PermissionModePlan (lines 39/51/59/68), which is introduced by the still-open #642 and isn't in this diff — go build ./... and the tui tests fail with undefined: agent.PermissionModePlan. Rebase onto #642's branch (or wait for #642 to land) and I'll take another pass.
Minor: the doc comment on handlePlanCommand credits an external tool as the design source — worth dropping so the behavior stands on its own.
|
@gnanam1990 flagging this one — it depends on #642 landing first (it doesn't build against main until PermissionModePlan exists). No rush, but once #642 merges a review here would help move it forward. |
|
CI was failing because this branch uses agent.PermissionModePlan, which is introduced in #642. Merged that branch in (d2204c0) so this compiles standalone. The diff temporarily includes #642's commit and will shrink once #642 merges. Retargeting the base onto the #642 branch is not possible because it only exists on my fork. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Detailed findings
🔴 1. /plan open is dead — program is always nil (Critical)
run.go does:
program = tea.NewProgram(initialModel, programOpts...)
initialModel.program = programBubble Tea v2 stores the model by value at construction: NewProgram sets initialModel: model (tea.go:597) and Run uses model := p.initialModel (tea.go:1099). The later initialModel.program = program mutates a copy that Bubble Tea never sees, so inside every Update/handlePlanCommand/openPlanInEditor invocation m.program is nil. openPlanInEditor therefore always takes the if m.program == nil branch and only prints the path. The entire editor-suspend feature is non-functional in a real session.
Fix: assign the program before constructing the program, e.g.
initialModel := newModel(ctx, options)
initialModel.program = tea.NewProgram(initialModel, programOpts...) // WRONG — still a copyThe correct fix is to make the program reachable from the live model instance. Simplest robust approach: capture the program in a field set after Run returns is too late; instead, since Bubble Tea lets you obtain the program inside Update via msg/tea helpers, or store the *tea.Program in a location the live model references. The idiomatic Bubble Tea v2 pattern is to call tea.ExecProcess and have the program available — typically by assigning the result of tea.NewProgram to a variable that is also stored on the model before NewProgram receives it is impossible by value. The right pattern is to keep the program in a pointer the model holds, or use tea.NewProgram(initialModel, opts...) and then set program.program = program is still a copy. The clean fix: store the program in a package-level or closure field, or make model hold a *tea.Program that is set and the initial model already references it. Concretely: create the program, then set the field on the same model value that Bubble Tea will evolve — which requires constructing the model with the program, i.e. initialModel.program = tea.NewProgram(initialModel, ...) — but that recurses. The supported approach in Bubble Tea v2 is to launch ExecProcess from within an update using a program obtained via the tea.WindowSizeMsg/context, or to capture the program reference through a small indirection (e.g., a programRef *tea.Program field set right after NewProgram, combined with Bubble Tea v2's guarantee that the initial model passed to NewProgram is the one evolved — so set the field on that exact variable before calling NewProgram). Reorder to:
initialModel := newModel(ctx, options)
if initialModel.wantsMouseCapture() { initialModel.mouseCapture = true }
initialModel.program = tea.NewProgram(initialModel, programOpts...) // still copies initialModel by value!This still copies. The only correct options are (a) make model carry the program via a pointer that Bubble Tea stores (the initial value copy still holds the same pointer), i.e. initialModel.programPtr = &prog where prog is assigned after — but the pointer target must exist; or (b) the documented Bubble Tea v2 way: obtain the program inside a command via func() tea.Msg { return ... } is not it either. Recommended: verify against Bubble Tea v2 docs, but the core requirement is that the model instance Bubble Tea mutates has program set. Since Bubble Tea mutates the value it was given, set the field on that exact variable before passing it to NewProgram — which is impossible because NewProgram needs the model to create the program. Use a forward declaration:
var program *tea.Program
initialModel.program = ... // cannot, program not yet createdPragmatic, correct fix: store the program in a field that is a pointer to a holder, or have openPlanInEditor receive the program through a closure/package variable set once. The least-invasive correct fix is to set the field on the model after program.Run() returns — no. The truly correct fix is to not rely on a model field at all: Bubble Tea v2 provides the program to commands via the tea.Program accessible in Update through msg only for specific messages. Given the complexity, the cleanest is to assign the program to the model and then start the program, accepting that Bubble Tea copied it — so instead set the program on the model returned by Update is moot.
Bottom line for the PR author: the current placement is wrong; the field must be set on the model instance that Bubble Tea will actually evolve. A reliable pattern is to capture the program in a pointer field: declare program *tea.Program, construct it, then set initialModel.program = program and ensure Bubble Tea uses that same value. Because Bubble Tea copies the initial model, the copy also contains the same pointer if the field is a pointer — but here the field is a pointer (*tea.Program), yet it is nil at copy time and only filled afterward, so the copy keeps nil. Therefore set the field before NewProgram by using a temporary holder:
prog := &tea.Program{} // placeholder; replaced below
initialModel.program = prog
program = tea.NewProgram(initialModel, programOpts...)
*prog = *program // copy the real program over the placeholder the model points toNow the model's pointer (copied by value into Bubble Tea) still points at prog, which now holds the real program. This works because the model stores a pointer and we mutate the pointed-to struct. Add a regression test that /plan open returns a tea.ExecProcess command (or that m.program != nil on the live model).
🟠 2. Shift+Tab silently exits plan mode (High)
nextPermissionMode (view.go:305) switches only on Auto/Ask and its default folds any other mode → Ask. model.go:1490 calls it on Shift+Tab with no guard for plan mode:
if m.noBlockingModal() {
m.permissionMode = nextPermissionMode(m.permissionMode)
return m, nil
}So pressing Shift+Tab while planning silently changes plan → Ask. Because the agent run reads m.permissionMode (model.go:4728, options.PermissionMode = m.permissionMode), the next run can mutate the workspace — read-only protection is gone without any message. (Note: spec mode presumably has its own guard; plan mode has none.)
Fix: add a case agent.PermissionModePlan: return agent.PermissionModePlan (no-op) to nextPermissionMode, or guard if m.permissionMode != agent.PermissionModePlan && m.noBlockingModal() at the call site. Optionally surface a notice when the mode is attempted.
🟡 3. Plan-file source-of-truth mismatch (Medium)
openPlanInEditor creates the plan file with empty content (planmode.WritePlan(m.cwd, id, "") at plan_command.go:78), and planText prefers the file (plan_command.go:126-131). Consequences:
- After the first
/plan open, the file exists and is empty, so/plan(which showsplanText) displays an empty plan instead of the in-memoryupdate_plandraft the agent built. - Edits the user makes in
$EDITORare never loaded back intoupdate_plan, so the agent's actual plan (what drives execution) ignores the file.
The PR claims plan mode "persists drafts in a workspace-backed plan file and displays saved plan content when available, with a fallback to the in-memory draft" — but the file and the in-memory plan are disconnected, and the fallback is permanently shadowed once the file is created.
Fix: seed the file from the current update_plan content on first open (so the user edits the real draft), and reload the file into the plan on editor exit (or treat the file as authoritative and sync it into update_plan). Pick one source of truth consistently.
🟡 4. DraftSystemPrompt is dead code (Medium)
grep confirms planmode.DraftSystemPrompt is only defined, never referenced. The run path sets a system prompt only when runOptions.systemPrompt != "" (model.go:4732), which is never the case for plan mode. Contrast spec_mode.go:75 which wires specmode.DraftSystemPrompt. So the agent is never explicitly told "you are in plan mode, do not implement." Read-only still holds via tool gating (see Positives), but the plan-specific guidance — and parity with spec mode — is missing.
Fix: wire planmode.DraftSystemPrompt into the plan-mode run (mirror spec_mode.go:75).
🟡 5. Symlink escape in plan-file path (Medium)
ensurePlanPathContained (planmode.go) validates the path with filepath.Rel — a lexical check. PlanFilePath then returns the path and WritePlan/planText call os.MkdirAll/os.WriteFile/os.ReadFile, which follow symlinks. A pre-existing symlink at .zero/plans/<slug>.md (or .zero/plans) would redirect writes/reads outside the workspace. The lexical check rejects .. traversal but not symlink-based escape.
Fix: resolve the final path with filepath.EvalSymlinks (after creation) and re-validate containment, or use os.Open with O_NOFOLLOW/O_EXCL semantics and reject symlinked components.
🟡 6. planText swallows non-IsNotExist read errors (Medium)
if content, err := os.ReadFile(path); err == nil {
return "Current Plan (plan mode)\n" + path + "\n" + strings.TrimRight(string(content), "\n")
}Any error other than success falls through to the in-memory plan with no signal, so a permission error or I/O failure is silently hidden.
Fix: distinguish os.IsNotExist (expected → fall back) from other errors (surface to the transcript).
🟢 Low / 🔵 Info (7–12)
- #7
/plan offhardcodesPermissionModeAuto(plan_command.go:43) — if the user was in a non-default mode before planning, it is lost. Consider restoring a previously-saved mode. - #8 Empty
SessionID→slugifyreturnstime.Now().UnixNano()per call (planmode.go:182), so the three call sites compute different plan files before a session exists. Use a stable fallback (e.g."plan"with a single resolved path cached per session). - #9
strings.Fields($VISUAL/$EDITOR)(plan_command.go:96) breaks paths with spaces; the//nolint:gosecis still justified (no shell). Trim/parse more carefully if editors-with-spaces must be supported. - #10 No timeout/abort on a hung editor (TUI frozen until the process exits). Acceptable for interactive use but worth a note.
- #11
planmode.ReadPlanis unused;slugifyis duplicated acrossplanmodeandspecmode(divergent — no length cap). Consolidate. - #12 No tests for plan mode; add unit tests for
handlePlanCommand,openPlanInEditor(editor-launch path /m.programwiring),nextPermissionModewithplan, andplanmodepath safety.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/loop.go (1)
901-907: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
modeNameif/else block.
string(permissionMode)already yields"plan"forPermissionModePlanand"spec-draft"forPermissionModeSpecDraft", so the if/else re-assigns identical values. Removing it also makes the code more maintainable: if a new mode is added to the outer condition,string(permissionMode)produces the correct name automatically, while theelsebranch would incorrectly emit"spec-draft".♻️ Proposed fix
if (permissionMode == PermissionModeSpecDraft || permissionMode == PermissionModePlan) && toolFound && !ToolAdvertised(tool, permissionMode) { - modeName := string(permissionMode) - if permissionMode == PermissionModePlan { - modeName = "plan" - } else { - modeName = "spec-draft" - } + modeName := string(permissionMode) return ToolResult{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 901 - 907, Remove the redundant PermissionModePlan conditional reassignment in the tool-advertisement error path. In the permissionMode/toolFound/ToolAdvertised condition, retain modeName initialized from string(permissionMode) and use it directly, preserving the existing mode names while allowing future modes to propagate automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 901-907: Remove the redundant PermissionModePlan conditional
reassignment in the tool-advertisement error path. In the
permissionMode/toolFound/ToolAdvertised condition, retain modeName initialized
from string(permissionMode) and use it directly, preserving the existing mode
names while allowing future modes to propagate automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a815ad6-71f3-4b1a-8b74-d6db93cd7dbf
📒 Files selected for processing (9)
internal/agent/loop.gointernal/agent/types.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/plan_command.gointernal/tui/plan_command_test.gointernal/tui/view.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/planmode/planmode.go
- internal/tui/plan_command.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the new permission test portable to Windows
internal/planmode/planmode_test.go:53
The required Windows smoke check currently fails in this test: Windows reports the plan file as mode0666even thoughWritePlanwas passed0600, sogo test ./...exits nonzero before the binary is built. Assert POSIX mode bits only on platforms where they are meaningful, or test the Windows ACL/security contract separately. -
[P2] Make bare
/planactually toggle plan mode off
internal/tui/plan_command.go:53
The command and PR describe/planas a toggle, but a second bare/planonly prints the current plan and leavesPermissionModePlanactive. A user following the advertised toggle workflow remains unable to write or run commands until they discover/plan off; either exit on the second invocation or stop describing it as a toggle. -
[P2] Do not expose
request_permissionsin read-only plan mode
internal/agent/loop.go:2904
request_permissionsis classified as side-effect-free and auto-allowed, so this predicate advertises it even though plan mode promises only reads,ask_user, andupdate_plan. The runtime then accepts a user-approved session grant; after/plan off, that grant can authorize filesystem or network operations the planning turn was not meant to request. Explicitly exclude it (and similarly privileged control tools) from the plan-mode allowlist. -
[P2] Keep plan mode scoped to the session that entered it
internal/tui/plan_command.go:62
The mode and its saved previous mode are held on the TUI model, while/newclears the session and/resumereplaces it without clearing either value. Entering plan mode in session A and then creating or resuming B silently makes B read-only;/plan offin B can also restore A's old permission mode. Clear/restore this state at session switches or persist it with the intended session. -
[P2] Create the session before assigning its plan-file name
internal/tui/plan_command.go:76
On a fresh TUI (or after/new),/plan openuses the empty session ID and writes.zero/plans/plan.md. The first planning prompt only creates the real session afterwards, so later/plancalls look for<session-id>.mdand silently orphan the edited plan; another fresh session also reusesplan.md. Ensure a session exists before opening/writing its plan, or migrate the provisional file when one is created. -
[P2] Persist and reconcile plans produced by
update_plan
internal/tui/plan_command.go:81
The plan file is written only once by/plan open; normalupdate_plancalls update an in-memory tool and neither save the plan nor refresh an existing file. Consequently a plan created through the prescribed agent workflow disappears after restart/resume, while opening the editor first permanently makes/plandisplay its stale snapshot instead of later agent updates. Define a single durable source of truth and update it whenever the plan changes. -
[P2] Close the symlink check/use race for plan files
internal/planmode/planmode.go:75
PlanFilePathchecks ancestors withLstat, butWritePlanand reads open those paths later. A process that replaces.zeroorplanswith a symlink between those operations can make/plan openwrite outside the workspace or make/planread an outside file, defeating the newly claimed symlink protection. Use descriptor-relative no-follow operations (or another atomic containment mechanism) rather than a preflight-only check.
|
Addressed in 2cd289e.
On |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/session_test.go (1)
748-815: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for non-plan permission mode preservation across session switches.
The three new tests all start from
PermissionModePlan. A test starting fromPermissionModeAsk(withpermissionModeBeforePlanempty) would catch the issue whereexitPlanMode()unconditionally resetspermissionModetoAuto— see the major issue flagged onsession.golines 56-61 and 224-229.🧪 Suggested test
func TestNewSessionPreservesNonPlanPermissionMode(t *testing.T) { store := testSessionStore(t) m := newModel(context.Background(), Options{SessionStore: store}) m.permissionMode = agent.PermissionModeAsk m.permissionModeBeforePlan = "" m = m.startNewSession() if m.permissionMode != agent.PermissionModeAsk { t.Fatalf("expected /new to preserve Ask mode when not in plan mode, got %s", m.permissionMode) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/session_test.go` around lines 748 - 815, Add coverage for switching sessions while already in PermissionModeAsk, with permissionModeBeforePlan empty, using the startNewSession flow. Assert that permissionMode remains PermissionModeAsk after the switch, ensuring exitPlanMode does not reset non-plan modes to Auto; keep the existing plan-mode tests unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/session.go`:
- Around line 56-61: Guard the exitPlanMode() call in startNewSession so it runs
only when permissionMode is PermissionModePlan or permissionModeBeforePlan is
non-empty; apply the same guard in handleResumeCommand. Preserve explicit
non-plan permission modes such as PermissionModeAsk at both session transitions.
---
Nitpick comments:
In `@internal/tui/session_test.go`:
- Around line 748-815: Add coverage for switching sessions while already in
PermissionModeAsk, with permissionModeBeforePlan empty, using the
startNewSession flow. Assert that permissionMode remains PermissionModeAsk after
the switch, ensuring exitPlanMode does not reset non-plan modes to Auto; keep
the existing plan-mode tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db69e896-a349-4507-9188-3aa41f7d6497
📒 Files selected for processing (8)
internal/agent/plan_mode_advertised_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/tui/model.gointernal/tui/plan_command.gointernal/tui/plan_command_test.gointernal/tui/session.gointernal/tui/session_test.go
|
Pushed two fixup commits addressing the latest coderabbitai feedback:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Reject
request_permissionsbefore the special dispatcher in plan mode
internal/agent/loop.go:901
The new denial only runs when the reserved tool is registered.executeToolCallthen dispatches therequest_permissionsname unconditionally at line 944, so a caller using a legitimate minimal/custom registry withoutNewRequestPermissionsToolcan still prompt for—and install—a session-scoped filesystem or network grant during/plan. That grant survives leaving plan mode, defeating its read-only guarantee. Apply the plan/spec-mode rejection by tool name before this special-case dispatch, and cover the unregistered-registry path. -
[P1] Reset or hydrate the in-memory plan on a real session switch
internal/tui/session.go:60
update_planis a registry-wide mutable tool, but/newand/resumeonly exit the permission mode; neither clears nor reloads itsCurrentPlan(or the sticky panel). When the destination session has no plan file,/planfalls back to that previous session's draft, and/plan openpersists it under the new session ID. This exposes one conversation's plan in another and corrupts the claimed per-session plan state. -
[P1] Do not overwrite editor changes from the agent result callback
internal/tui/model.go:5047
/plan openlets the user edit the file, but editor completion never reconciles those contents withupdate_plan's in-memory state. The next callback unconditionally truncates the file with that stale state; the same race is possible while/plan openis allowed during a pending run, and even an error result rewrites the file. A user's saved plan can therefore be silently lost. Serialize or reconcile editor and agent updates, and persist only successful, current plan updates. -
[P1] Keep the editor handoff inside the symlink-safe plan boundary
internal/tui/plan_command.go:130
ReadPlan/WritePlanuseos.Root, but the editor receives the separately computed ordinary pathname. A workspace process can replace.zero/plansor the plan file with an outside symlink after the protected operation and before the editor opens it, causing the editor to read or write outside the workspace. Use a descriptor-safe handoff (or otherwise remove this race) rather than passing the raw path after claiming containment. -
[P2] Fix permissions for pre-existing plan files and directories
internal/planmode/planmode.go:81
MkdirAll(..., 0700)andOpenFile(..., 0600)apply those modes only when they create the target. A pre-existing.zero/plansdirectory or session file with0755/0644stays broadly readable afterWritePlan, exposing persisted plan contents despite the owner-only storage contract. Safely chmod or reject pre-existing insecure paths and add a regression test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/plan_command.go`:
- Around line 179-208: Update parsePlanFileLines to track whether parsing is
inside a Notes block and preserve multiline content: numbered lines start new
PlanItems, while unnumbered lines append to the preceding item’s Content or
Notes according to the current context instead of creating pending items.
Accumulate repeated and multiline Notes text rather than overwriting it, and
initialize each parsed item’s ID consistently with the tool’s existing JSON
parser behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b55852bc-d5da-427c-8d16-52136a21c159
📒 Files selected for processing (4)
internal/tools/update_plan.gointernal/tui/model.gointernal/tui/plan_command.gointernal/tui/plan_command_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/model.go
- internal/tui/plan_command_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase this branch onto the current
mainbefore merging
internal/sandbox/profile.go:68
The PR is based ataa73a76, while its GitHub base isfa3052a; the 14 interveningmaincommits are absent from the head and the PR diff actively deletes their implementations and regression tests. This is not harmless merge-base churn: for example, this line restores a blanket OS-level deny on the whole.gittree, so a permitted sandboxedgit add,commit,fetch, orpullcannot update the index, objects, refs, orFETCH_HEAD. The same stale merge rolls back the strict-compatible-providerprompt_cache_keyguard, config typo validation, before-tool hook fail-closed behavior, durable secret/credential syncing, plugin-relative commands, queued-message preservation, Windows lock handling, and the npm packaging changes. Rebase onto currentmainand retain only the plan-mode changes, then re-run the affected tests; otherwise merging this PR regresses already-shipped behavior across those surfaces. -
[P1] Reject
request_permissionsby name before its special dispatcher in plan mode
internal/agent/loop.go:901
The plan-mode gate only fires when the reserved tool is present in the registry, butrequest_permissionsis dispatched unconditionally by name at line 944. A legitimate minimal/custom registry that omitsNewRequestPermissionsTooltherefore skips the gate, reachesexecuteRequestPermissions, and can install a session permission grant that remains usable after/plan off. The new test registers the tool, so it misses precisely this bypass. Reject this reserved call before the special dispatch and cover the unregistered-registry path. -
[P1] Reset or load
update_planwhen switching sessions
internal/tui/session.go:53
/newand/resumeonly leave the permission mode; the registry-wideupdate_planobject is neither cleared nor hydrated from the destination session. Create a plan in session A, then start/resume session B with no plan file:/planfalls back to A's in-memory draft, and/plan openwrites that draft under B's session ID. This leaks and corrupts per-session plans. Clear it on the switch or reload the destination's persisted plan before rendering/opening it. -
[P1] Emit a completion message after a successful plan-editor exit
internal/tui/plan_command.go:147
The callback returnsnilfor a successful$EDITORprocess, whilereloadPlanFromFileis reachable only fromcase planEditorFinishedMsginmodel.go. Consequently ordinary edits are never reloaded and the agent continues from the old in-memory plan; the added test callsreloadPlanFromFiledirectly and does not exercise the Bubble Tea callback. Return a completion message for both success and failure. -
[P1] Do not allow the editor to race a live plan-mode run
internal/tui/plan_command.go:109
Unlike entering plan mode,/plan openhas no pending-run guard or serialization. While an agent run is active, the user can edit the plan file and anupdate_planresult callback can concurrently truncate it with the tool's old/current state. Editor completion then reloads that overwritten file, silently discarding the user's edit. Block editor use while a run is active, or serialize/reconcile the two writers. -
[P1] Keep the external editor inside the plan-file containment boundary
internal/tui/plan_command.go:143
ReadPlanandWritePlanuseos.Root, but the editor receives a separately computed ordinary pathname. A workspace process can replace.zero/plansor the target after those protected operations and before the editor opens it; the editor then follows the new symlink and reads or writes an arbitrary user-writable file outside the workspace.PlanFilePathitself documents that it provides no containment guarantee. Use a descriptor-safe handoff or remove the raw-path time-of-check/time-of-use gap. -
[P2] Tighten permissions on existing plan storage
internal/planmode/planmode.go:81
MkdirAll(..., 0700)andOpenFile(..., 0600)apply their modes only when creating a target. A pre-existing.zero/plansdirectory or session file with0755/0644remains broadly readable afterWritePlan, exposing persisted plan contents despite the owner-only storage contract. Safely chmod or reject insecure existing paths and add a regression test. -
[P2] Preserve multiline content when parsing an edited plan
internal/tui/plan_command.go:185
Only the firstNotes:line is attached to the preceding item; subsequent unnumbered lines—and multiline step text—are parsed as new pending steps. For example,Notes: firstfollowed bysecondbecomes an extra item, so normal editor formatting corrupts the plan on reload. Track the current item/notes block, append continuation lines, and add multiline coverage. -
[P2] Refresh the visible plan panel after editor reload
internal/tui/plan_command.go:166
Reloading callsSetPlanbut never updatesm.plan; the sticky panel changes only whenplanUpdateMsgis sent from an agentupdate_planresult. After editing or deleting steps, execution uses the reloaded plan while the TUI continues displaying the old steps and statuses until a later agent update. Update the panel from the parsed items (or emit the corresponding message) during reload.
|
Pushed 2258ecf: fixed the /plan command palette description. It read "Show planning mode status" but /plan actually toggles plan mode and supports open/off subcommands; updated the usage string and description to match, plus the corresponding help-text test. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/commands.go`:
- Around line 112-114: Update the plan command description associated with the
usage "/plan [open|off]" to explicitly say “off” rather than implying an “exit”
subcommand, using wording that explains turning plan mode off; update the
corresponding help expectation to match the revised description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed90499a-210f-4975-8d4a-672df1d0e00e
📒 Files selected for processing (2)
internal/tui/commands.gointernal/tui/commands_test.go
| usage: "/plan [open|off]", | ||
| group: commandGroupSession, | ||
| description: "Show planning mode status.", | ||
| description: "Toggle plan mode, or open the plan file / exit.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the literal off subcommand in the description.
The usage exposes open|off, but “/ exit” may lead users to try /plan exit. Prefer wording such as: “Toggle plan mode, open the plan file, or turn plan mode off.” Update the corresponding help expectation as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/commands.go` around lines 112 - 114, Update the plan command
description associated with the usage "/plan [open|off]" to explicitly say “off”
rather than implying an “exit” subcommand, using wording that explains turning
plan mode off; update the corresponding help expectation to match the revised
description.
|
Pushed 21aaf17 addressing jatmn's latest round:
Added regression tests for each. Not addressed in this push: this branch was ~14 commits behind main. |
- 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
/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.
…g, 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.
…odes 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.
…path string(permissionMode) already yields "plan" and "spec-draft" for the two modes this branch handles, so the if/else was reassigning identical values.
…tus 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.
The palette showed "/plan - Show planning mode status" but /plan actually toggles plan mode and supports open/off subcommands.
…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.
euxaristia
force-pushed
the
feat/tui-plan-mode
branch
from
21aaf17 to
66e2652
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make editor handoff preserve the plan-file containment boundary
internal/tui/plan_command.go:162
ReadPlanandWritePlanuseos.Root, but/plan openlater passes the ordinaryPlanFilePathstring to$EDITOR; that helper explicitly has no containment guarantee. A workspace process can replace.zero/plansor the plan file with an outside symlink between the protected I/O and exec, causing the editor to follow it and modify an arbitrary user-writable target. Use a descriptor-safe handoff or otherwise eliminate this pathname race. -
[P1] Carry editor-authored plans into the implementation run
internal/tui/plan_command.go:185
Reloading an edited plan only callsSetPlan; it neither records a session event nor includes the plan in the next prompt. The normal run builds context fromsessionEvents, andbeginRunclears the rendered plan, so after/plan offthe implementation agent cannot see the user-authored editor plan despite the feature saying it drives execution. Persist/inject the plan into implementation context. -
[P2] Hydrate the destination plan on
/resume
internal/tui/session.go:227
A session switch clearsupdate_planand the sticky panel but never reloads the resumed session's persisted plan file. The next plan update starts from empty state and can overwrite the durable plan. Reload the target plan and refresh the panel after assigningactiveSession. -
[P2] Preserve multiline step content when reloading an edited plan
internal/tui/plan_command.go:246
OnlyNotes:continuations are folded. A newline in a normalPlanItem.Contentis formatted verbatim and then reloads as an extra pending item, silently changing the plan's structure. Track wrapped step continuations as well. -
[P2] Enforce restrictive modes on pre-existing plan storage
internal/planmode/planmode.go:81
MkdirAll(0700)andOpenFile(0600)do not tighten modes on existing directories/files. A pre-created0755plan directory or0644file remains broadly readable afterWritePlan, contrary to the owner-only storage contract. -
[P2] Avoid creating a session for an invalid
/plan open
internal/tui/plan_command.go:58
/plan openwhen plan mode is inactive callsensureActiveSessionbeforeopenPlanInEditorrejects the command. A command that should only show an error therefore leaves a persistent empty session in/resume. Validate the mode before creating a session.
…ext, 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.
|
Pushed 32715e2 for jatmn's review at #643 (review):
Added 5 regression tests. Full tui/planmode/agent suites pass. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep editor staging outside the sandbox for every supported config location
internal/planmode/planmode.go:135
The containment argument assumesconfig.UserConfigDir()is outside the sandbox's write roots, but that function honorsXDG_CONFIG_HOME. Starting Zero withXDG_CONFIG_HOMEin the workspace or/tmpputszero/plan-editback in a default writable sandbox root. A sandboxed command can then race or replace the deterministic staged path with a symlink, and the ordinaryos.WriteFile/external editor handoff follows it outside the intended boundary. Choose a staging directory that is actually private for the active sandbox profile (or reject writable roots) and create the staging file with no-follow, exclusive semantics. -
[P2] Record an empty editor result in the session context
internal/tui/model.go:1134
Clearing every line in the editor correctly callsSetPlan(nil), but this conditional skips the replacement user event whenformatPlanItems(items)is empty. The next run therefore replays the earlierupdate_plancall fromsessionEventsand has no indication that the user deliberately cleared it, so it can continue from the discarded plan. Persist an explicit "plan cleared" edit event (or otherwise replace the prior plan context) on every successful editor reload. -
[P2] Do not use a shared deterministic staging file for editor sessions
internal/planmode/planmode.go:142
StageForEditorpromises a fresh file but always writes<config>/zero/plan-edit/<session-slug>.md, and every caller's cleanup removes that same pathname. Two Zero instances editing a resumed session can overwrite one another's staged content; the first completion can commit the other editor's draft and remove its file, causing the other completion to lose or fail its edit. Use a unique, exclusively created staging file per invocation and clean up only that file. -
[P2] Parse quoted editor commands instead of splitting on whitespace
internal/tui/plan_command.go:178
$EDITOR/$VISUALvalues commonly include a quoted executable path plus flags, for example"/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code" --wait.strings.Fieldsturns that into an executable named"/Applications/Visual, so/plan opencannot launch it. Use a shell-word parser appropriate to the supported platforms, or define and document a command configuration format that preserves quoted paths and arguments.
3 participants
This PR adds the /plan slash command to toggle read-only plan mode in the TUI, view the plan, and edit the plan file in /, and migrates editor spawning to tea.ExecProcess under Bubbletea v2 to resolve compilation and runner hangs.
Summary by CodeRabbit
/plan,/plan open,/plan off(and/plan exit) to edit your plan in your editor./newand/resume.Notes:blocks.