feat(tui): add plan mode command and fix plan file editing - #854
feat(tui): add plan mode command and fix plan file editing#854euxaristia wants to merge 31 commits into
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
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 |
- 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.
…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.
- 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.
…an 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.
…xisting 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.
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.
Three review findings: - Unknown /plan subcommands (a typo like "openx", or "status") fell through the switch to the bare toggle and silently exited the read-only mode. They now return a usage error; only bare /plan toggles. - WritePlan opened the plan path with O_TRUNC, destroying the previous durable plan before the new content landed, and followed a symlink that resolves inside the workspace — a planted .zero/plans/<slug>.md symlink would redirect plan mode's one allowed write over an arbitrary workspace file. It now refuses symlinked targets and writes an owner-only O_EXCL temporary sibling renamed into place. - The update_plan result callback re-read the shared tool's CurrentPlan() after the call released its mutex, so a cancel plus /new or /resume in that window persisted the wrong session's plan (or an empty reset) under the old run's session ID. A successful call now carries its own plan snapshot in the result meta and the callback persists exactly that snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool.
… registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ctive Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entering plan mode replaced options.SystemPrompt wholesale with planmode.DraftSystemPrompt, discarding any embedder-configured system prompt for the whole duration of plan mode. Layer the plan-mode instructions onto the configured prompt instead, falling back to the plain draft prompt when nothing was configured. Also chmod the plan-edit staging directory unconditionally after MkdirAll, so a pre-existing, loosely permissioned directory no longer undermines the staging design's symlink-race protection.
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
update_plan is read-only and auto-allowed, but the TUI persisted every successful call into .zero/plans under the workspace. Store durable plans under the user config directory (scoped by workspace) so Ask mode and plan mode no longer create workspace files without a write grant. Also verify the editor staging directory is a plain owner-only dir after chmod (reject group/world-writable or symlink paths), cover the pre-existing permissive staging-dir case, and assert plan mode layers DraftSystemPrompt onto a configured agent system prompt rather than replacing it.
…ing checks os.UserConfigDir (what config.UserConfigDir defers to outside darwin) reads %AppData% on Windows and ignores XDG_CONFIG_HOME there, so tests that only set XDG_CONFIG_HOME silently fail to isolate plan storage on Windows and fall through to the runner's real profile directory. Set AppData too wherever a test overrides the config root. Also skip the new group/world-writable check in verifyPrivateDirectory on Windows: NTFS reports a directory's POSIX mode via ACLs rather than the bits os.Chmod sets, so the check rejected every staging directory unconditionally and made /plan open never launch $EDITOR on Windows, the same rationale already used to skip the file-mode assertion in TestWritePlanUsesRestrictivePermissions.
slugify alone maps distinct session/workspace IDs that differ only by separator (plan_a vs plan-a) onto the same path. pathKey appends a SHA-256 suffix of the exact original string so durable plans stay isolated across those collisions. Refs Gitlawb#643
…n mode completion, and continuation whitespace
…ool policy vetoes Reset plan mode when drafting or approving specs, preserve beforeTool policy vetoes during plan mode, reject plan storage in temp tree, and hash unmodified identifiers in pathKey. Refs Gitlawb#643
… by main Both were thin unscoped wrappers around the Scoped variants, deleted upstream in Gitlawb#706 since nothing else called them directly. Only this branch's tests still did; switch to the Scoped calls main's own tests already use.
0708379 to
a372f61
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The plan-mode core here is good. I drove the real advertised-tool gate against the full core registry and plan mode exposes exactly ask_user, glob, grep, list_directory, read_file, read_minified_file, skill, update_plan — every mutator, web_fetch, lsp_navigate, request_permissions, the Task/swarm spawners (SideEffectShell) and all MCP tools (SideEffectNetwork) are denied, at advertisement and at dispatch. The name-only spoofing guard on update_plan/ask_user is the right call, and so is denying request_permissions before the registry lookup rather than relying on the registry to omit it. tea.ExecProcess is used correctly: the m.pending || m.exiting gate keeps it off a live run, the staged copy plus defer cleanup() in the callback is right, and I couldn't find a terminal-state path that escapes bubbletea's release/restore. go build ./..., go vet, gofmt -l, and go test ./internal/tui ./internal/agent ./internal/planmode ./internal/tools are all clean here.
Four things before this goes in.
1. /btw is the session switch you missed. internal/tui/btw.go:94 does side.activeSession = fork without exitPlanMode() or resetPlanForSessionSwitch(). You guard the other four switch sites (session.go:70, session.go:241, spec_mode.go:38, spec_mode.go:203); this is the fifth. Driving the real path — enter plan mode from Ask with an update_plan draft in the tool, then /btw — gives:
side.permissionMode == "plan"andside.permissionModeBeforePlan == "ask", so the isolated side conversation is silently read-only and a/plan offinside it restores the main session's prior mode into the fork.side.planText()renders the main session's plan.side.plan.clear()at btw.go:144 only clears the sticky panel; the sharedupdate_plantool still holds the parent's items.- Worse, that leak is now durable:
/plan openinside the side conversation seeds the fork's plan file with the parent's plan. I gotplanmode.ReadPlan(cwd, side.activeSession.SessionID)returningexists=true, "1. [in_progress] MAIN SESSION SECRET STEP\n"for a session that never drafted it.
Add the same two calls at btw.go:94. Two more things while you're in there: leaveBTW (btw.go:164) restores the parent model wholesale but not the shared update_plan tool, which the side conversation may have replaced — it should re-hydrate from the parent session's plan file the way handleResumeCommand now does. And btwCommandUnavailable (btw.go:206) already blocks /new, /resume, /spec, /loop, /goal; /plan now mutates permission mode and writes durable per-session files, so it probably belongs on that list too.
2. internal/planmode drags testing into the shipped binary. planmode.go:12 imports "testing" for SetTempDirForTest (planmode.go:381). go list -deps ./cmd/zero | grep -cx testing is 0 on origin/main and 1 on this branch (it brings flag and regexp along too), and planmode.go is the only non-test file under internal/ that does this. Move the helper to an export_test.go in the package, or to a planmodetest subpackage, and keep tempDirFn unexported.
3. program *tea.Program at model.go:135 is dead. Nothing assigns or reads it — deleting the line and running go build ./internal/tui/ exits 0. The comment says it's "set right before Run", but run.go is untouched by this PR, and plan_command_test.go:210 already refers to it as "(now-removed)". Drop the field and fix that test's rationale comment.
4. hooksSuppressed's comment says something the code doesn't do. loop.go:1791 explains suppression as preventing "merely starting a plan session or calling read_file" from mutating the workspace or spawning processes — but dispatchBeforeTool is deliberately exempt, and beforeTool is precisely the hook that fires on every read_file. I ran a plan-mode Run with a beforeTool hook that shells out to go mod init -modfile <tmp>/go.mod: the audit store logged hook_execution_started/completed for beforeTool, read_file returned normally, and the file existed on disk afterwards. Keeping beforeTool for fail-closed policy vetoes is the right trade-off — just say so in the comment instead of claiming the opposite. Relatedly, TestRunSuppressesExecutableHooksInPlanMode asserts "no hook command at all" but only wires sessionStart/sessionEnd; either soften the wording or add a case that pins the beforeTool exemption, so a future change can't flip it silently.
Smaller things, none blocking:
exitPlanMode(plan_command.go:122) falls back to Auto whenpermissionModeBeforePlanis empty.nextPermissionModefolds unknown modes to Ask on purpose ("the stricter landing") and app.go:786 makes Ask the interactive default — Auto is the looser landing. Only reachable via an embedder starting in plan mode, but make it Ask.handleSpecCommand(spec_mode.go:38) clears plan state beforecreateSpecDraftSession; on a create failure the user loses plan mode and the in-memory plan with no session switch.handleResumeCommand's ordering (switch, then reset) is the shape to copy.result.Meta[plan_snapshot]lands verbatim in the session event log (model.go:5526), so everyupdate_planstores the plan twice on disk. It isn't replayed into model context, so it's disk-only, but stripping it fromtoolPayloadis cheap.- Plan files accumulate under
UserConfigDir/zero/plansforever, one per (workspace, session), with no pruning. Worth a retention story. - Entering plan mode doesn't pause an armed
/goalor/loop, so continuations keep firing turns that can't make progress. Safe, just wasteful.
One thing I checked and am happy with: I fuzzed formatPlanItems/parsePlanFileLines beyond your tests (empty content, leading whitespace on the first line, a first line reading "3. ...", a [weird] leading token, tab continuations, blank note lines, an empty first Notes line, CRLF). Every case is a fixed point under repeated open-and-save; the only losses are leading whitespace on an item's first content line and an empty first Notes line, both harmless. The escape/indent encoding holds up.
Same as the others today: this is everything in one pass, nothing queued behind it. And thanks for the turnaround on #849 — that one went from requested-changes to approved inside two hours, which is the loop I'd like these to run in.
Reset plan mode and in-memory plan state when entering a BTW side session so /btw matches the /new and /resume session-switch guards. Move SetTempDirForTest into export_test.go so cmd/zero no longer depends on testing. Drop the unused model.program field. Clarify that plan mode suppresses lifecycle and afterTool hooks only, while beforeTool still runs for fail-closed vetoes, and pin that behavior with a regression test.
|
Addressed review:
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving — all four are done, and I checked each one rather than going off the summary.
The /btw fix is the one I cared about. It now calls exitPlanMode and resetPlanForSessionSwitch like the other four switch sites, so it's no longer the odd one out. I commented out the exitPlanMode call and TestBTWExitsPlanModeOnSideAndPreservesParent fails with "BTW side kept plan mode: plan", so the guard is genuinely held in place rather than just present in the diff. Good that the test also pins the parent side surviving — that's the half that would have been easy to miss.
The testing import is properly gone: go list -deps ./cmd/zero | grep -cx testing is 0 on this head, where it was 1 before. Moving SetTempDirForTest into export_test.go was the cleaner of the two options I suggested.
Dead program field is gone, and the hooksSuppressed comment now says advisory-only with beforeTool still running for fail-closed vetoes — which matches what #853 actually does now, so the two PRs tell the same story. Worth something that they agree; a comment that drifts from its sibling PR is how the original confusion started.
Nothing else from me on this one.
2 participants
Summary
/plancommand and TUI wiring forPermissionModePlan(see the companion agent-side PR), including a command-palette entry, editor round-trip for the plan file, and status/notes preserved across editor exitexitPlanModeagainst clobbering an unrelated permission modebeforeToolpolicy vetoes while activeTest plan
go test ./internal/tui/... ./internal/planmode/...