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 58 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughPlan mode now restricts tool execution, permission requests, executable hooks, and automatic continuations. Plans use secure durable storage and editor staging. The TUI synchronizes plan state across updates, editing, sessions, BTW conversations, and spec transitions. ChangesPlan mode and storage
TUI plan workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to This change adds plan mode and plan-file editing, but unresolved platform-specific plan I/O failures, stale permission state for peers, and loops that can remain paused after switching modes create concrete correctness and availability risks. The PR should not merge until these issues are fixed or explicitly accepted by the owners. Sequence Diagram(s)sequenceDiagram
participant Agent
participant UpdatePlanTool
participant TUI
participant PlanStorage
Agent->>UpdatePlanTool: submit plan update
UpdatePlanTool->>TUI: return plan snapshot metadata
TUI->>PlanStorage: persist plan
TUI->>TUI: refresh plan state and panel
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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.
|
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.
Block /plan inside /btw, re-sync parent plan on leaveBTW, fall back to Ask when exitPlanMode has no prior mode, clear plan only after successful /spec session create, and omit plan_snapshot from session tool events. Refs Gitlawb#854
|
Addressed the remaining plan-mode edge cases on tip
Regression tests cover each item; they fail on the previous tip and pass here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
internal/tools/update_plan.go (1)
108-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCopy the slice in
SetPlanto matchCurrentPlan.
SetPlanstores the caller's slice directly.enforceSingleInProgressalso mutates that slice in place when more than one item has statusin_progress. Two consequences follow:
- The caller's slice is modified as a side effect of calling
SetPlan.- The tool and the caller then share one backing array, so a later caller mutation changes tool state without the mutex.
CurrentPlanalready returns a copy, so the boundary is inconsistent. Callers do retain the slice:internal/tui/btw_test.gopassesitemstoSetPlanand then reusesitemsfor the plan panel.♻️ Proposed fix
func (tool *updatePlanTool) SetPlan(plan []PlanItem) { - plan = enforceSingleInProgress(plan) + // Copy before normalizing: enforceSingleInProgress mutates in place, and + // the tool must not share a backing array with the caller (CurrentPlan + // returns a copy for the same reason). + plan = enforceSingleInProgress(append([]PlanItem(nil), plan...)) tool.mu.Lock() tool.currentPlan = plan tool.mu.Unlock() }🤖 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/tools/update_plan.go` around lines 108 - 117, Update updatePlanTool.SetPlan to copy the incoming plan slice before enforcing statuses and storing it, ensuring the tool owns its backing array and caller mutations cannot affect currentPlan. Preserve the existing enforceSingleInProgress behavior while making the stored plan consistent with CurrentPlan’s copy-on-boundary behavior.internal/planmode/planmode.go (1)
204-218: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse handle-relative staging for
StageForEditor.
StageForEditorstill resolves withfilepath.EvalSymlinks, validatesresolvedDir, then opens withstageContentForEditor(resolvedDir, ...). That is pre-open resolution followed by open, which the code guidelines reject. With the declared Go toolchain, open the staging parent withos.OpenRootand use theos.Rootmethods forChmodandCreateTempso containment is bound at open/use time.🤖 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/planmode/planmode.go` around lines 204 - 218, Update StageForEditor to avoid filepath.EvalSymlinks and path-based staging; open the staging parent with os.OpenRoot, then use the resulting os.Root methods for Chmod and CreateTemp so validation and file creation remain handle-relative. Adapt stageContentForEditor to accept and use the root handle, while preserving the existing privacy checks and error behavior.Source: Coding guidelines
internal/agent/request_permissions_test.go (1)
149-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the denial category.
executeRequestPermissionssetsDenialReason: DenialFilteredon the plan-mode denial. Surfaces branch on that category instead of parsingOutput. Pin it here so a future change cannot drop the field while keeping the message.💚 Proposed assertion
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) } + if result.DenialReason != DenialFiltered { + t.Fatalf("DenialReason = %q, want %q", result.DenialReason, DenialFiltered) + } }🤖 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/request_permissions_test.go` around lines 149 - 151, Update the assertion for executeRequestPermissions’ plan-mode denial to also require the result’s DenialReason to equal DenialFiltered, while preserving the existing status and output checks.internal/agent/loop_test.go (2)
3448-3463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment claims
ask_usercoverage, but onlyupdate_planis exercised.Loop over both names, or narrow the comment to
update_plan. A table subtest keeps the guard honest if someone later reintroduces a name-based allowlist forask_user.As per coding guidelines: "Ensure PR descriptions, help text, and comments match shipped behavior".♻️ Table-driven variant
-func TestPlanModeRejectsNameOnlySpoofedControlTools(t *testing.T) { - root := t.TempDir() - written := filepath.Join(root, "spoofed.txt") - registry := tools.NewRegistry() - registry.Register(spoofedSafetyTool{ - name: "update_plan", +func TestPlanModeRejectsNameOnlySpoofedControlTools(t *testing.T) { + for _, spoofed := range []string{"update_plan", "ask_user"} { + t.Run(spoofed, func(t *testing.T) { + runSpoofedControlToolCase(t, spoofed) + }) + } +} + +func runSpoofedControlToolCase(t *testing.T, toolName string) { + t.Helper() + root := t.TempDir() + written := filepath.Join(root, "spoofed.txt") + registry := tools.NewRegistry() + registry.Register(spoofedSafetyTool{ + name: toolName, safety: tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "spoofed"},Then thread
toolNamethrough the provider events and the advertisement assertion.🤖 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_test.go` around lines 3448 - 3463, Update TestPlanModeRejectsNameOnlySpoofedControlTools to cover both “update_plan” and “ask_user” as claimed, preferably with table-driven subtests, and thread each toolName through provider events and advertisement assertions. Alternatively, narrow the test comment to describe only the currently exercised “update_plan” case.Source: Coding guidelines
4035-4074: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a filesystem state change that actually covers the hook, not the failure path.
go mod init -modfile <marker>/go.mod markerexits whenmarkerdoes not exist and does not createmarker, so theos.Stat(marker)check only guards the failed command. Use a temp directory that exists and have the hook create a file in it if executed.🤖 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_test.go` around lines 4035 - 4074, Update the test around the dispatcher and marker setup so the hook’s command operates on an already-created temporary directory and creates a file inside it when executed. Change the final filesystem assertion to check that file remains absent, ensuring the test detects actual hook execution rather than only a failed go command.Source: Coding guidelines
internal/tui/plan_command_test.go (2)
148-170: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for the unknown-
/plan-subcommand guard.
handlePlanCommandtreats an unrecognized subcommand as a hard error specifically so it cannot fall through to the bare toggle. The comment atinternal/tui/plan_command.goLines 55-58 states the reason: falling through "would silently exit the read-only boundary and re-enable implementation."That is a security-boundary behavior with no test here.
TestBarePlanTogglesOffcovers the toggle, but nothing covers/plan openxor/plan statuswhile plan mode is active.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."
🧪 Proposed test
func TestUnknownPlanSubcommandDoesNotExitPlanMode(t *testing.T) { // Regression: an unrecognized subcommand must not fall through to the // bare /plan toggle, which would silently drop the read-only boundary. m := newPlanModeTestModel(t, t.TempDir(), agent.PermissionModePlan) m.permissionModeBeforePlan = agent.PermissionModeAsk for _, arg := range []string{"openx", "status", "on"} { updated, cmd := m.handlePlanCommand(arg) next := updated.(model) if cmd != nil { t.Fatalf("%q: expected no command", arg) } if next.permissionMode != agent.PermissionModePlan { t.Fatalf("%q: expected plan mode preserved, got %s", arg, next.permissionMode) } if !transcriptContains(next.transcript, "Unknown /plan subcommand") { t.Fatalf("%q: expected an unknown-subcommand error, got %#v", arg, next.transcript) } } }🤖 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_test.go` around lines 148 - 170, Add a regression test alongside TestBarePlanTogglesOff for unknown handlePlanCommand subcommands such as “openx”, “status”, and “on” while PermissionModePlan is active. Assert no command is returned, plan mode remains active, and the transcript contains the “Unknown /plan subcommand” error, ensuring invalid input cannot fall through to the bare toggle.Source: Coding guidelines
328-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCanonicalize both paths before the containment assertion.
Line 332 compares the raw
pathandcwdspellings. On macOSt.TempDir()returns a/var/folders/...path that is a symlink to/private/var/folders/.... If the durable plan path ever resolved through the other spelling, this prefix check would pass while the file actually sits inside the workspace. The assertion is guarding a security boundary, so it must not be defeatable by a path-spelling difference.As per coding guidelines: "canonicalize paths before comparison and avoid asserting raw temporary-directory spellings."
🧭 Proposed fix: resolve symlinks before comparing
path, err := planmode.PlanFilePath(cwd, next.activeSession.SessionID) if err != nil { t.Fatalf("PlanFilePath: %v", err) } - if strings.HasPrefix(path, cwd+string(os.PathSeparator)) || path == cwd { - t.Fatalf("durable plan path %q must not live under the workspace %q", path, cwd) + resolvedCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + t.Fatalf("EvalSymlinks(cwd): %v", err) + } + // The plan file's parent exists even when the leaf may not; resolve the dir. + resolvedPlanDir, err := filepath.EvalSymlinks(filepath.Dir(path)) + if err != nil { + t.Fatalf("EvalSymlinks(plan dir): %v", err) + } + resolvedPlan := filepath.Join(resolvedPlanDir, filepath.Base(path)) + if rel, err := filepath.Rel(resolvedCwd, resolvedPlan); err == nil && + rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + t.Fatalf("durable plan path %q must not live under the workspace %q", resolvedPlan, resolvedCwd) }🤖 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_test.go` around lines 328 - 337, Canonicalize both path values before the containment assertion in the plan path test: resolve symlinks for cwd and the value returned by planmode.PlanFilePath, handle resolution errors through the test, then perform the existing workspace-prefix and equality checks on the canonical paths. Keep the .zero absence assertion unchanged.Source: Coding guidelines
🤖 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_test.go`:
- Around line 496-520: Extend the planmode tests with direct StageForEditor
coverage: configure the user config root to the workspace and assert it returns
the containment error, then add a success case verifying the staged file is
created under the resolved config staging directory. Reuse the existing test
setup and symbols such as StageForEditor and the config-root mechanism, while
retaining the platform-specific permission skips where applicable.
- Around line 295-304: Update TestWritePlanRejectsStorageInsideWorkspace to
override the plan storage temp-directory provider via SetTempDirForTest with an
unrelated directory, preventing the global temp-directory containment check from
triggering. Keep the workspace as the configured user config root, and assert
that WritePlan returns the expected workspace-containment error text so the test
specifically validates that rule.
In `@internal/planmode/planmode.go`:
- Around line 260-269: Update editorStagingDirIsPrivate so a
filepath.Abs(workspaceRoot) error immediately returns false instead of skipping
the workspace containment check and returning true; preserve the existing
rejection for directories under the resolved workspace root and temp directory.
- Around line 125-131: Update the comment above tmpPath in the plan-writing flow
to remove the inaccurate “random suffix” claim and describe the
PID/timestamp-based name accurately; retain the explanation that O_EXCL rejects
existing or pre-planted paths. Do not change the temporary-file implementation
unless needed to keep the comment consistent with shipped behavior.
- Around line 402-417: The blank-ID fallback in pathKey collides with the real
ID "plan", violating injective plan-path mapping. Replace the rawID fallback
with a reserved sentinel that cannot collide with valid session IDs, while
preserving stable results across calls; add a regression test verifying
PlanFilePath(root, "") and PlanFilePath(root, "plan") return different paths.
In `@internal/tools/update_plan_test.go`:
- Around line 12-28: Extend TestUpdatePlanRefusesCancelledRun to decode the
successful result’s PlanSnapshotMeta with encoding/json and assert it contains
the installed “live” plan, while asserting the cancelled result has no snapshot
metadata. Add a separate concurrent test that invokes Run and SetPlan(nil) from
different goroutines, waits for both to finish, and verifies CurrentPlan is
either empty or exactly the new session’s state; ensure the test is suitable for
execution with the race detector.
In `@internal/tui/plan_command.go`:
- Around line 237-249: Update reloadPlanFromFile to return the ReadPlan error
separately from the missing-plan false result, preserving the existing item
reload behavior. Adjust the /plan enter call site to discard the new error
value, and update the planEditorFinishedMsg handler to append a transcript error
containing the read failure before returning; retain the existing silent return
only when no plan exists.
---
Nitpick comments:
In `@internal/agent/loop_test.go`:
- Around line 3448-3463: Update TestPlanModeRejectsNameOnlySpoofedControlTools
to cover both “update_plan” and “ask_user” as claimed, preferably with
table-driven subtests, and thread each toolName through provider events and
advertisement assertions. Alternatively, narrow the test comment to describe
only the currently exercised “update_plan” case.
- Around line 4035-4074: Update the test around the dispatcher and marker setup
so the hook’s command operates on an already-created temporary directory and
creates a file inside it when executed. Change the final filesystem assertion to
check that file remains absent, ensuring the test detects actual hook execution
rather than only a failed go command.
In `@internal/agent/request_permissions_test.go`:
- Around line 149-151: Update the assertion for executeRequestPermissions’
plan-mode denial to also require the result’s DenialReason to equal
DenialFiltered, while preserving the existing status and output checks.
In `@internal/planmode/planmode.go`:
- Around line 204-218: Update StageForEditor to avoid filepath.EvalSymlinks and
path-based staging; open the staging parent with os.OpenRoot, then use the
resulting os.Root methods for Chmod and CreateTemp so validation and file
creation remain handle-relative. Adapt stageContentForEditor to accept and use
the root handle, while preserving the existing privacy checks and error
behavior.
In `@internal/tools/update_plan.go`:
- Around line 108-117: Update updatePlanTool.SetPlan to copy the incoming plan
slice before enforcing statuses and storing it, ensuring the tool owns its
backing array and caller mutations cannot affect currentPlan. Preserve the
existing enforceSingleInProgress behavior while making the stored plan
consistent with CurrentPlan’s copy-on-boundary behavior.
In `@internal/tui/plan_command_test.go`:
- Around line 148-170: Add a regression test alongside TestBarePlanTogglesOff
for unknown handlePlanCommand subcommands such as “openx”, “status”, and “on”
while PermissionModePlan is active. Assert no command is returned, plan mode
remains active, and the transcript contains the “Unknown /plan subcommand”
error, ensuring invalid input cannot fall through to the bare toggle.
- Around line 328-337: Canonicalize both path values before the containment
assertion in the plan path test: resolve symlinks for cwd and the value returned
by planmode.PlanFilePath, handle resolution errors through the test, then
perform the existing workspace-prefix and equality checks on the canonical
paths. Keep the .zero absence assertion unchanged.
🪄 Autofix
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 Plus
Run ID: 8db6760e-11f3-4350-9967-841e55455887
📒 Files selected for processing (24)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/plan_mode_advertised_test.gointernal/agent/request_permissions_test.gointernal/agent/types.gointernal/planmode/export_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/tools/types.gointernal/tools/update_plan.gointernal/tools/update_plan_test.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/plan_command.gointernal/tui/plan_command_test.gointernal/tui/session.gointernal/tui/session_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.gointernal/tui/view.go
…load Fail closed when the workspace root cannot be resolved for editor staging, use a non-colliding blank-session pathKey sentinel, copy on SetPlan so enforceSingleInProgress cannot mutate the caller, surface plan-file read errors from the editor reload path, and tighten regression coverage for workspace containment, StageForEditor, and plan_snapshot metadata. Refs Gitlawb#854
|
Addressed the latest CodeRabbit review on tip
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
internal/planmode/planmode_test.go (1)
344-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten this assertion.
The condition accepts either substring.
StageForEditorreturns exactly one message for this case, so assert that message. A weaker error path would still pass today.🧪 Proposed change
- if !strings.Contains(err.Error(), "sandbox-writable") && !strings.Contains(err.Error(), "workspace") { - t.Fatalf("expected workspace/staging containment error, got: %v", err) + if !strings.Contains(err.Error(), "sandbox-writable") { + t.Fatalf("expected staging containment error, got: %v", err) }🤖 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/planmode/planmode_test.go` around lines 344 - 346, In the StageForEditor test assertion, replace the OR-based substring check with an exact assertion against the expected error message returned for this case. Preserve the existing failure output while ensuring weaker alternative error messages cannot satisfy the test.internal/agent/loop_test.go (1)
4077-4134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated
gobinary lookup into a test helper.Lines 4081-4091 repeat Lines 4020-4030 verbatim. A shared helper keeps the skip condition and the Windows suffix logic in one place.
♻️ Suggested helper
// testGoBinary resolves the go binary for hook tests that need a real // executable, skipping when the toolchain is not reachable. func testGoBinary(t *testing.T) string { t.Helper() if goBinary, err := exec.LookPath("go"); err == nil { return goBinary } goBinary := filepath.Join(runtime.GOROOT(), "bin", "go") //nolint:staticcheck // Safe for this non-portable test binary. if runtime.GOOS == "windows" { goBinary += ".exe" } if _, err := os.Stat(goBinary); err != nil { t.Skipf("go binary unavailable on PATH or in GOROOT: %v", err) } return goBinary }Then both tests reduce to:
- goBinary, err := exec.LookPath("go") - if err != nil { - goRoot := runtime.GOROOT() //nolint:staticcheck // Safe for this non-portable test binary. - goBinary = filepath.Join(goRoot, "bin", "go") - if runtime.GOOS == "windows" { - goBinary += ".exe" - } - if _, statErr := os.Stat(goBinary); statErr != nil { - t.Skipf("go binary unavailable on PATH or in GOROOT: %v", statErr) - } - } + goBinary := testGoBinary(t)🤖 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_test.go` around lines 4077 - 4134, Extract the duplicated Go executable lookup from TestBeforeToolStillRunsInPlanMode and the nearby hook test into a shared testGoBinary helper. Preserve PATH lookup, GOROOT fallback, Windows suffix handling, missing-binary skip behavior, and mark the helper with t.Helper(); update both tests to call it.internal/agent/loop.go (1)
3227-3249: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a
Safetyclassification for host-process spawning.The current built-in tools have only
lsp_navigatewithSideEffectRead + PermissionAllowthat starts a process. A classification-based exclusion prevents this allowlist from becoming stale when another tool gains the same behavior.🤖 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 3227 - 3249, Add a dedicated Safety side-effect classification for tools that spawn host processes, apply it to lsp_navigate, and update toolAdvertisedInPlan to exclude that classification instead of checking the tool name. Preserve the existing read-only and permission checks for other tools.Source: Coding guidelines
🤖 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/agent/loop_test.go`:
- Around line 4011-4075: Add a regression test named
TestAfterToolSuppressedInPlanMode alongside the existing plan-mode hook tests.
Configure an EventAfterTool hook matching read_file, invoke dispatchAfterTool
with PermissionModePlan and a successful ToolCall, then assert no feedback is
returned and the audit contains no hook_execution_started event.
In `@internal/planmode/planmode.go`:
- Around line 73-84: Update ReadPlan to open the plan file through a handle with
syscall.O_NOFOLLOW on Linux, then read from that handle and close it, preserving
the existing not-found and wrapped-read-error behavior. Keep the Lstat-based
symlink check only as the Windows fallback, ensuring the file is not reopened by
name after validation.
- Around line 209-211: Update StageForEditor’s staging privacy check in
internal/planmode/planmode.go:209-211 to pass effectiveTempDir() instead of
os.TempDir(), matching ensurePlanPathContained’s test seam. In
internal/planmode/planmode_test.go:349-361, set a throwaway override with
SetTempDirForTest and construct configDir beneath t.TempDir() rather than beside
os.TempDir(), preserving cross-platform test behavior.
In `@internal/tui/btw.go`:
- Around line 210-212: Handle the error returned by reloadPlanFromFile in
internal/tui/btw.go lines 210-212 by reporting reload failures and synchronizing
both the restored panel and shared update_plan state; apply the equivalent fix
in internal/tui/session.go lines 256-258 for /resume, keeping destination plan
state consistent. Add regression tests covering unreadable and malformed plan
files in both flows.
In `@internal/tui/plan_command.go`:
- Around line 302-309: Remove the dead initial assignment to lineBody in the
surrounding parsing logic; declare it without initializing it, then retain the
existing branch assignments for the three whitespace cases so ineffassign passes
without changing behavior.
- Around line 50-61: Reorder the switch clauses in the /plan argument handling
so the default clause is last, after the case "off", "exit" and case "open"
blocks. Preserve the existing unknown-subcommand error message and return
behavior while satisfying ST1015 lint requirements.
---
Nitpick comments:
In `@internal/agent/loop_test.go`:
- Around line 4077-4134: Extract the duplicated Go executable lookup from
TestBeforeToolStillRunsInPlanMode and the nearby hook test into a shared
testGoBinary helper. Preserve PATH lookup, GOROOT fallback, Windows suffix
handling, missing-binary skip behavior, and mark the helper with t.Helper();
update both tests to call it.
In `@internal/agent/loop.go`:
- Around line 3227-3249: Add a dedicated Safety side-effect classification for
tools that spawn host processes, apply it to lsp_navigate, and update
toolAdvertisedInPlan to exclude that classification instead of checking the tool
name. Preserve the existing read-only and permission checks for other tools.
In `@internal/planmode/planmode_test.go`:
- Around line 344-346: In the StageForEditor test assertion, replace the
OR-based substring check with an exact assertion against the expected error
message returned for this case. Preserve the existing failure output while
ensuring weaker alternative error messages cannot satisfy the test.
🪄 Autofix
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 Plus
Run ID: 8330ef1d-b5c4-40ed-a140-1e5978adcfbc
📒 Files selected for processing (24)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/plan_mode_advertised_test.gointernal/agent/request_permissions_test.gointernal/agent/types.gointernal/planmode/export_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/tools/types.gointernal/tools/update_plan.gointernal/tools/update_plan_test.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/plan_command.gointernal/tui/plan_command_test.gointernal/tui/session.gointernal/tui/session_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.gointernal/tui/view.go
…mode Bind plan reads at open with O_NOFOLLOW on Unix, route StageForEditor through the temp-dir test seam so CI staging privacy checks pass, surface durable plan reload failures from /btw return and /resume, fix plan_command switch/lint nits that fail CI, and pin afterTool suppression in plan mode. Refs Gitlawb#854
|
Addressed the open CodeRabbit findings on ac88fb9:
Verified: @coderabbitai full review |
Rate Limit Exceeded
|
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/read_unix.go`:
- Around line 16-25: The plan-file readers must bind containment to trusted
directory handles instead of relying on pathname traversal. In
internal/planmode/read_unix.go, update readPlanFile to open each directory
component with no-follow protection and open the final file relative to that
handle; in internal/planmode/read_other.go, use traversal-resistant
handle-relative APIs for non-Unix platforms or fail closed when unavailable,
rather than calling os.ReadFile by pathname. Add regression coverage for
intermediate-component symlinks and Windows reparse points.
🪄 Autofix
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 Plus
Run ID: 298611ca-0d23-4fbe-aaa3-b1a4800c258b
📒 Files selected for processing (10)
internal/agent/loop_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/planmode/read_other.gointernal/planmode/read_unix.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/plan_command.gointernal/tui/session.gointernal/tui/session_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tui/btw.go
- internal/tui/session.go
- internal/agent/loop_test.go
- internal/planmode/planmode_test.go
- internal/planmode/planmode.go
- internal/tui/plan_command.go
Final-component O_NOFOLLOW left intermediate directory swaps able to redirect plan reads outside the storage tree. Open the plans base as os.Root and read relative to that handle so traversal cannot escape, and refuse a symlink final component. Add intermediate-symlink and plain-file regression coverage. Refs Gitlawb#854
CodeRabbit follow-up (finding 3738089757)SHA: What changed
Verification
No |
Rate Limit Exceeded
|
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/read.go`:
- Around line 33-40: Update the file-opening flow around root.Lstat and
root.Open to atomically refuse final-component symlinks: use a no-follow open
that also protects against Windows reparse points, then verify the opened handle
identifies a regular file before reading. Preserve the existing symlink refusal
error behavior where applicable, and add a regression test that replaces the
requested file with a symlink between path inspection and opening.
🪄 Autofix
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 Plus
Run ID: b2d1d138-3cc7-45ed-9e59-ba06772c08cd
📒 Files selected for processing (3)
internal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/planmode/read.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/planmode/planmode_test.go
- internal/planmode/planmode.go
os.Root.Open follows in-root symlinks after O_NOFOLLOW fails, so a root.Lstat then root.Open sequence could race and read a swapped target. Walk with true no-follow opens (openat O_NOFOLLOW / OBJ_DONT_REPARSE), verify a regular file, and cover the in-root replace-with-symlink case. Refs Gitlawb#854
CodeRabbit major (3738164693): TOCTOU on plan read fixedSHA:
Fix
Tests
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/planmode/read.go (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a sentinel error instead of substring matching.
ReadPlanininternal/planmode/planmode.godetects this refusal withstrings.Contains(err.Error(), "is a symlink"). That couples the caller to the message text. A wrapped sentinel keeps the same user-facing text and makes the check explicit.♻️ Proposed refactor
+// ErrPlanSymlink marks a refused symlink / reparse-point component. +var ErrPlanSymlink = errors.New("is a symlink; refusing to read through it") + func errPlanSymlink(path string) error { - return fmt.Errorf("plan file %s is a symlink; refusing to read through it", path) + return fmt.Errorf("plan file %s %w", path, ErrPlanSymlink) }Then
ReadPlanuseserrors.Is(err, ErrPlanSymlink).🤖 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/planmode/read.go` around lines 36 - 40, Define an exported sentinel error such as ErrPlanSymlink and have errPlanSymlink wrap it while preserving the existing user-facing message. Update ReadPlan to detect this condition with errors.Is(err, ErrPlanSymlink) instead of matching the error string, and remove the substring-based check.internal/planmode/planmode_test.go (1)
378-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWindows reparse-point coverage silently disappears here.
Both tests call
t.Skipfwhenos.Symlinkfails. On Windows without Developer Mode orSeCreateSymbolicLinkPrivilege, that is exactly what happens, so the entireread_windows.gowalker ships with zero executed assertions. The skip is correct behavior for a symlink test; the gap is that nothing else covers the Windows path.Add one Windows-only test that creates a directory junction with
mklink /J(junctions need no special privilege) and asserts the walker refuses it. That exercisesOBJ_DONT_REPARSEandisWindowsSymlinkErron the platform they exist for.As per coding guidelines: "path-sensitive logic must include a non-Linux case or a hermetic equivalent exercising the same normalization."
Also applies to: 425-427
🤖 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/planmode/planmode_test.go` around lines 378 - 380, Add a Windows-only test alongside the symlink tests that creates a directory junction via `mklink /J` without relying on `os.Symlink`, then invokes the walker and asserts the junction is rejected. Exercise the Windows-specific `read_windows.go` behavior, including `OBJ_DONT_REPARSE` and `isWindowsSymlinkErr`, while leaving the existing privilege-dependent symlink tests’ skip behavior unchanged.Source: Coding guidelines
internal/planmode/read_unix.go (1)
81-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Isfor the errno comparisons.
unix.Openatreturns asyscall.Errno, so==works today. It breaks silently if the error is ever wrapped, and the failure mode is bad: a wrappedELOOPwould stop being reported as a symlink refusal and would surface as a raw errno instead.errors.Iskeeps the same semantics and survives wrapping.♻️ Proposed refactor
func openatRetry(dirfd int, path string, flags int, mode uint32) (int, error) { for { fd, err := unix.Openat(dirfd, path, flags, mode) - if err == syscall.EINTR { + if errors.Is(err, syscall.EINTR) { continue } return fd, err } } func isNoFollowErr(err error) bool { - return err == syscall.ELOOP || err == syscall.EMLINK + return errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EMLINK) }Add
"errors"to the imports.🤖 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/planmode/read_unix.go` around lines 81 - 96, Update isNoFollowErr to use errors.Is when comparing err against syscall.ELOOP and syscall.EMLINK, and add the errors import. Preserve recognition of both platform-specific errno values while allowing wrapped errors to match.
🤖 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/read_windows.go`:
- Around line 185-204: Update mapWindowsOpenErr to map
windows.STATUS_NO_SUCH_FILE and the relevant intermediate-path-missing NTSTATUS
from the NtCreateFile walk to os.ErrNotExist, preserving the existing mappings.
Add Windows-specific coverage for ReadPlan when the session plan is missing
while the storage base exists, asserting it returns "", false, nil.
---
Nitpick comments:
In `@internal/planmode/planmode_test.go`:
- Around line 378-380: Add a Windows-only test alongside the symlink tests that
creates a directory junction via `mklink /J` without relying on `os.Symlink`,
then invokes the walker and asserts the junction is rejected. Exercise the
Windows-specific `read_windows.go` behavior, including `OBJ_DONT_REPARSE` and
`isWindowsSymlinkErr`, while leaving the existing privilege-dependent symlink
tests’ skip behavior unchanged.
In `@internal/planmode/read_unix.go`:
- Around line 81-96: Update isNoFollowErr to use errors.Is when comparing err
against syscall.ELOOP and syscall.EMLINK, and add the errors import. Preserve
recognition of both platform-specific errno values while allowing wrapped errors
to match.
In `@internal/planmode/read.go`:
- Around line 36-40: Define an exported sentinel error such as ErrPlanSymlink
and have errPlanSymlink wrap it while preserving the existing user-facing
message. Update ReadPlan to detect this condition with errors.Is(err,
ErrPlanSymlink) instead of matching the error string, and remove the
substring-based check.
🪄 Autofix
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 Plus
Run ID: 33871054-a167-4253-896b-05285317b085
📒 Files selected for processing (5)
internal/planmode/planmode_test.gointernal/planmode/read.gointernal/planmode/read_other.gointernal/planmode/read_unix.gointernal/planmode/read_windows.go
|
@coderabbitai full review |
…n for intermediate symlinks Refs Gitlawb#854
Add file.Sync() before the atomic rename on the Unix write path so the temp file is fully flushed before replacing the plan. Drop the redundant symlink conditional in WritePlan, which masked the underlying error. Cover the full editor staging -> commit -> read roundtrip and the plan-reload failure path with tests, and fix splitEditorCommandFor so unquoted Windows editor values containing backslashes keep separators literal regardless of whether they begin with a drive or UNC path. Refs Gitlawb#854 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
Keep the plan editor and durable file workflow while adopting main's explicit /plan on, /plan status, /plan off contract. Preserve terminal companion commands and the live Bubble Tea program field that /plan open needs after rebasing onto main. Refs Gitlawb#854 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
Keep plan-mode state accurate after file reload failures, report the mode actually restored by exitPlanMode, align help text with the explicit command contract, and cover staged editor write-back. Remove the unused model program reference. Refs Gitlawb#854 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
…or parsing Use an errors.Is sentinel for symlink refusals, chmod only the resolved staging directory after privacy validation, align Windows rename and delete information classes with their payloads, drop the duplicated reparse check and local prefix helper, detect unterminated Windows editor quotes, make test config roots unique, and assert the saved restore mode survives a same-session resume. Refs Gitlawb#854 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
…mode Delete the dead toolAdvertisedInSpecDraft/toolAdvertisedInPlan duplicates in loop.go that were failing the unused-func lint gate; the real advertisement gate already delegates to tools.ToolAdvertisedForPermissionMode. Wire planEnterText into /plan on instead of leaving it dead, and drop the ineffectual parts reassignment in the editor-quote test that make lint-static was failing on. Fix ntObjectPath to stop treating \?\ (extended-length) and \.\ (device) path prefixes as UNC, which produced a malformed NT path and failed every plan read for a user whose %AppData% resolves through one. Make the non-Unix/non-Windows fallback reader fail closed instead of opening a file through a validate-then-open symlink race it cannot close. Sweep staged plan files left behind when a Bubble Tea shutdown drops the tea.ExecProcess command before its cleanup callback runs. Fix two tests that didn't reach the behavior they claimed to guard: TestWritePlanRefusesIntermediateSymlink only ever hit the outer containment pre-check, never the handle-relative writer's own symlink refusal, and TestStageForEditorRejectsStagingInsideWorkspace's setup broke plan storage before StageForEditor could reach the staging-specific check (plan storage and staging both resolve through the same UserConfigDir, so pointing config at the workspace fails ReadPlan first — verified by running the review's own suggested fix, which still failed). Add isolatePlanConfig to the session-switch test that touches the real machine's plan directory, and align /plan help text with the parser's status|on|open|off subcommands. Refs Gitlawb#854
…mlink openat(..., O_DIRECTORY|O_NOFOLLOW) reports ENOTDIR, not ELOOP, when the named component is a symlink on Linux and Darwin: the kernel never dereferences it to see the O_DIRECTORY mismatch it would otherwise report. isNoFollowErr only recognized ELOOP/EMLINK, so the no-follow walkers in both openPlanUnderBase (read) and writePlanFile's writer fell through to a generic, unclassified error on those platforms instead of the intended symlink refusal. The write path's refusal still failed closed (no write occurred), just under the wrong error text, which is what surfaced this: the prior commit's tightened TestWritePlanRefusesIntermediateSymlink assertion failed on the ubuntu-latest and macos-latest smoke jobs. Add isSymlinkDisguisedAsENOTDIR, shared by both walkers, which disambiguates ENOTDIR with a no-follow stat so a genuine non-symlink, non-directory component (a plain file blocking the path) still reports its real error instead of a false symlink claim. Refs Gitlawb#854
…an file On a fresh TUI, or after /new, the session ID stays empty until the first prompt lazily creates it, and PlanFilePath maps an empty ID onto a single shared no-session slug. Plan-mode entry therefore has to create the session before it reports anything about the plan file, or the banner points every fresh session at the same shared path. TestPlanOpenCreatesSessionBeforeWritingPlanFile only reaches this through the /plan open that follows entry, so entry on its own was untested, including the banner now naming the session's plan file. Assert both that /plan on creates the session and that the banner carries that session's own path and not the no-session fallback. Recovered from an abandoned worktree, then adapted: the original drove entry with a bare /plan, which now reports status instead of entering plan mode. Refs Gitlawb#854
The planEditorFinishedMsg handler appended a session event on every successful editor exit. Opening the plan with /plan open, reading it, and quitting without saving therefore wrote "I edited the plan file directly. Updated plan: ..." into the session. That event is phrased as the user's own words, so the next turn saw a statement the user never made, and each repeated open restated the whole plan body into the session log again. Capture the plan before reloadPlanFromFile replaces it, compare it with the reloaded items, and return early when they match, skipping both the transcript note and the session event. planItemsEqual compares content, status, and notes but not ID: parsePlanFileLines rebuilds items from the file text without preserving in-memory IDs, so comparing IDs would report every reload as a change. TestPlanEditorFinishedMsgNoOpEditRecordsNothing fails without the guard with "an unchanged plan file must not record a session event: before=0 after=1". Refs Gitlawb#854
…base
Every component under the storage base was opened no-follow, but the base
itself was opened by path and followed links. ensurePlanPathContained
resolves the base and the plan path through the same link, so a link at
${UserConfigDir}/zero/plans passed containment unless its target happened to
be the workspace or the temp directory. The handle-relative walk was then
simply rooted inside the target, so every read, create, and rename landed
there while each individual component check still passed.
Open the base with O_NOFOLLOW on Unix and OBJ_DONT_REPARSE on Windows, and
report it through errPlanBaseSymlink, which wraps the existing
errPlanSymlinkRefusal sentinel so ReadPlan surfaces it like any other symlink
refusal. O_NOFOLLOW applies to the final component only, so a legitimately
symlinked ~/.config above the storage root still works. The Windows change is
one attribute on the shared openWindowsBaseDir, which both walkers already
use, and it matches the flags every component-level open there already sets.
TestPlanStorageBaseSymlinkRefused replaces the storage root with a link and
requires both ReadPlan and WritePlan to refuse and the target to stay empty.
Without the fix it fails on Linux with "expected ReadPlan to refuse a
symlinked plan storage root", verified in a container.
Refs Gitlawb#854
…st isolation Address review comments: - Document CommitStagedEdit trust contract for stagedPath. - Remove redundant chmod from stageContentForEditor. - Use errors.Is for errno checks in read_unix.go. - Use unsafe.Slice in write_windows.go for UTF-16 rename path. - Isolate plan config in TestNewSessionClearsPreviousPlan. Refs Gitlawb#854
… isolation Address review feedback: - Tighten staging dir permissions in stageContentForEditor. - Remove redundant pathname os.Chmod on base from writePlanFile. - Preserve in-memory plan state on /plan on reload failure with regression test. - Extend spoofed control-tool test to cover ask_user in loop_test.go. - Isolate plan config in session and spec mode switch tests. - Verify pending and activeRunID directly in spec mode create failure test. Refs Gitlawb#854
Unsafe sessions were still advertised as bypass after /plan on because Shift+Tab was the only path that called syncPeerIdentity. Enter and exit now republish the current permission class. Refs Gitlawb#854 Co-Authored-By: cairn-code <cairn-code@users.noreply.github.com>
Directory-symlink creation is privileged on many Windows runners, so TestPlanStorageBaseSymlinkRefused skips there. A junction is an unprivileged reparse point and exercises openWindowsBaseDir's OBJ_DONT_REPARSE mapping through WritePlan. Refs Gitlawb#854
Automatic /loop ticks and /goal continuations cannot make progress in plan mode, so entering /plan holds them and /plan off resumes them instead of spending turns that cannot implement the plan.
Grants FILE_TRAVERSE on Windows directory handles used as RootDirectory for NtCreateFile, since relative opens fail with STATUS_ACCESS_DENIED without SeChangeNotifyPrivilege. Fails the non-Unix/non-Windows write fallback closed to match the read side, since the prior os.Root-based path had a check-to-use race and wrote plans that could never be read back. Wraps errPlanSymlinkWrite around the shared errPlanSymlinkRefusal sentinel so callers can detect write-side refusals with errors.Is like the read side. Fixes stale test comments referencing a function that was never shipped, pins the chmod ordering in the staging-privacy test, and asserts the error from reloadPlanFromFile instead of discarding it.
Co-Authored-By: cairn-code <cairn-code@users.noreply.github.com>
Co-Authored-By: cairn-code <cairn-code@users.noreply.github.com>
…ment editorStagingDirIsPrivate compares physical paths so a staging directory that resolves into the workspace or the OS temp dir is refused, but physicalPath resolved through filepath.EvalSymlinks, which hands a junction straight back: os.Lstat maps one to ModeIrregular rather than ModeSymlink. A junction needs no SeCreateSymbolicLinkPrivilege, so it is the reparse point an unprivileged process can actually plant, and the check the function documents did not hold on the one platform where that matters. Resolve through GetFinalPathNameByHandle on Windows instead, which asks the filesystem what the handle resolved to and so accounts for every reparse type at once; VOLUME_NAME_DOS also returns long names, subsuming the 8.3 short-name normalization the comparison already needed. verifyPrivateDirectory now rejects a reparse point explicitly rather than relying on its !IsDir test firing by accident, which is why a junctioned staging directory was refused with "is not a directory". The Windows staging tests skip wherever directory-symlink creation is privileged, which is why this went unnoticed; the new ones use the junction helper the storage tests already rely on. Verified on NTFS: both containment tests fail before this change and pass after it. Refs Gitlawb#854
Prevent queued messages from auto-launching on turn completion while plan mode is active, requiring explicit exit or submission before running. Refs Gitlawb#854
abf29ca to
7230d33
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Every blocking item from both reviews is closed, and the junction fix is exactly right. Still requesting changes, for one thing: four separate exits now drop plan mode's read-only gate silently.
Closed
The Windows staging containment (mine). resolvePhysical replaces filepath.EvalSymlinks at planmode.go:170 and :330, and the new physical_windows.go uses CreateFile(FILE_FLAG_BACKUP_SEMANTICS) plus GetFinalPathNameByHandle, with the extended-length prefix trimmed back to Win32 form. Driven with a real cmd /c mklink /J: both halves now return false and physicalPath(junction) == physicalPath(target), where before the fix they returned true. Reverting to EvalSymlinks fails three committed tests by name. This is the right API and it is the one I could not give another contributor correctly, so thank you for getting it right.
The backstop and the coverage (mine). pathIsReparsePoint is in verifyPrivateDirectory and the doc comment now matches. The six new junction tests actually RUN on an unelevated box rather than skipping, which was the whole problem last time. I also want to credit the comment in TestPlanFlowRefusesJunctionedConfigRootIntoWorkspace saying "this one passes before the fix as well, and that is worth recording rather than hiding". That is the right instinct and I confirmed it is true.
Queued prompts (@jatmn's P1). model.go:5313 now carries || m.planModeBlocksContinuations(), the same predicate goal.go:269 and loop.go:341 use. Driven end to end: held across repeated completions, released exactly once on /plan off, not replayed by the next completion, and Up still returns the text to the composer intact. Deleting the guard fails the test at plan_command_test.go:1077, so it is genuinely pinned. The rebase is done too, behind_by 0.
Plan mode is dropped by four exits, and base keeps it
/spec is the clearest. Base:
case commandRewind, commandExport, commandSandboxSetup, commandSpec, commandInit:Head, plan_command.go:150, with commandSpec removed:
case commandRewind, commandExport, commandSandboxSetup, commandInit:So /plan on then /spec leaves permissionMode="auto", and spec_mode.go:209-219 launches the approved implementation run with no mode override. Base blocked that command outright.
The other three behave the same way. btw.go:151 calls side.exitPlanMode(), so a side conversation gets "unsafe" or "ask" where base keeps "plan"; under plan the bash tool is not advertised and under unsafe it is. session.go:77 and :249 take /new and /resume <other> from plan to "unsafe". Nothing in the transcript says the mode changed in any of the four, and /btw is not blocked inside plan mode while /plan is blocked inside BTW, so it is one way.
None of the four is pinned by a test, all four diverge from base toward less restriction, and view.go:328-333 in this same PR argues the opposite case in a comment. That is what makes this the blocker rather than a note.
Three more, same round
The plan reload hands raw items to the caller and enforced items to the tool. plan_command.go:360-366 returns parsePlanFileLines(content) to the caller and passes the same slice to SetPlan, which applies enforceSingleInProgress. Driven through a real /plan on with a file carrying two [doing] steps: update_plan, which this code calls the execution source of truth, records step 1 as completed while the sticky panel and the file both still say in_progress. The user is never told.
A no-op editor session claims the user edited the plan. The guard at model.go:1481-1512 compares with planItemsEqual, but formatPlanItems to parsePlanFileLines is not the identity for ordinary model-authored text: leading and trailing whitespace, tabs and CR are all stripped. Five of seven realistic agent-written shapes fail the round trip, so opening the editor and quitting without a keystroke writes "I edited the plan file directly. Updated plan: ..." into the session as the user. That is verbatim what the comment above the guard forbids. The same round trip also mutates the agent's plan on a plain /plan on with no editor involved.
On Windows a benign reparse point above the plans root kills /plan entirely. read_windows.go:116-158 opens the whole absolute path with OBJ_DONT_REPARSE, so the refusal fires on any ancestor rather than only the storage root. That contradicts the contract read_unix.go:25-27 states, that "a legitimately symlinked ~/.config above the storage root is still fine". Driven with a junction two and four levels above the root, and with a subst drive where pathIsReparsePoint is false on every component while the error still says "is a symlink; refusing to open through it". On an affected machine /plan on and /plan status error, and model.go:5891-5893 then emits an error row on every successful update_plan call in every mode. It fails closed, so this is availability plus a false diagnostic rather than a containment hole, but base worked. Resolving the parent physically and no-following only the final component is the shape that fixes it.
Notes, not blocking
The reparse guard at planmode.go:304 is not actually pinned: stub pathIsReparsePoint to false and TestVerifyPrivateDirectoryRejectsJunction still passes, because os.Lstat maps a junction to ModeIrregular so !info.IsDir() fires first. The outcome is refusal either way, so the direction is safe. Just do not let that test name be read later as coverage of the guard.
Seven of the no-follow-walk tests still skip on Windows. I drove the gap by hand with an intermediate junction and a junction at the final .md name; both were refused and nothing escaped, but the refusal came from ensurePlanPathContained, not from the OBJ_DONT_REPARSE walk, so the walk's own component refusal remains unexercised here.
Peer-message turns at peer_messages.go:237 and :249 are not held by plan mode, unlike /loop, /goal and now the queued prompt. Such a turn runs with the plan toolset, so head is stricter than base and this is a note only.
Checked and correct
Build, vet, gofmt, GOOS=linux and GOOS=darwin all clean; internal/planmode passes; the only internal/tui failure is TestHandleAddDirCommand, identical on base. The loop paused flag has only plan-mode writers, so resumeLoopsAfterPlan clearing it unconditionally cannot clobber another pause source, and every exitPlanMode path without a resume also calls clearLoopsForSessionSwitch. /new discarding a queued message is base behaviour, untouched by your diff, so not yours. The scroll_test.go Cwd hunk is test hygiene, not a papered-over failure. Build tags on the new test files are right and no untagged test asserts a platform-only contract.
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.
These are not ten unrelated edge cases. Most come from four shared design gaps, and fixing only the individual symptom at each cited line is likely to produce another review round.
Overall guidance
-
Make plan mode a single lifecycle/state-machine contract. Permission mode, the mode to restore, session ownership, queued/automatic continuations, the sticky panel, and user-visible status are currently changed by separate command handlers. Define one transition boundary for enter, explicit exit, session switch, and isolated side/spec surfaces. Each transition should atomically decide whether the read-only gate is preserved or exited, update every dependent field, and emit truthful user-visible state.
/spec,/new,/resume, and/btwshould not each hand-assemble part of that transition. -
Choose one canonical typed plan snapshot and derive every representation from it. The update tool, result metadata, sticky panel, editable text file, and durable file currently normalize or redact at different stages. Normalize once when accepting or parsing a plan, then pass one immutable typed snapshot to internal consumers. Render text from that snapshot, parse edits back into the same canonical form, and apply secret scrubbing only to transcript/display data—not to the internal persistence payload. The tool, panel, durable file, and next-turn context must never represent different plans.
-
Bind filesystem security and cleanup to the object/owner, not a checked pathname or elapsed time. Once a staging directory passes containment and privacy checks, keep a handle to that directory and perform creation relative to it with no-follow/reparse protections. Likewise, reclaim an edit only when Zero can prove that exact staged file belongs to an abandoned invocation through an ownership marker plus a lease/lock or equivalent liveness mechanism. A resolved path and an mtime are not durable security or ownership evidence.
-
Test the lifecycle matrix hermetically instead of adding one regression per review comment. Inject all config/cache/staging roots. Then cover mode transition × plan source × persistence result × platform: enter/exit,
/new,/resume,/btw,/spec, editor no-op/edit, write failure, missing file, restart, multiplein_progressrows, secret-shaped content, concurrent staging, and Unix/Windows path redirection. For every row, assert permission mode, prior-mode state, canonical tool plan, panel plan, durable bytes, transcript claim, and cleanup outcome. This should catch a whole class of state drift rather than pinning only the latest symptom.
Findings
-
[P1] Keep the read-only gate until the user explicitly exits plan mode
internal/tui/spec_mode.go:49Failure path: Start in a non-Plan permission mode, run
/plan on, and then invoke/spec,/new,/resume <other>, or/btw. Each path callsexitPlanModewithout/plan offor a warning, restoring the previous permission mode./specis the clearest impact:mainblocks it while plan mode is active, but this head removes that guard, restores the prior non-Plan mode, and the approval path starts an implementation run with no plan override.Root cause: Plan mode is represented partly as a global permission value and partly as session-owned state, but session transitions independently call low-level reset/exit helpers. There is no single contract deciding whether a transition preserves the read-only gate, blocks the command, or requires explicit user confirmation.
Required outcome: Route every plan/session transition through one state transition boundary. Relaxing the read-only gate must be explicit and visible; switching conversation state must not silently widen tool authority. Preserve the current behavior of explicit
/plan off, including restoration of the prior permission choice.Regression coverage: Exercise all four commands from Plan entered from Auto, Ask, and Unsafe. Assert the permission/toolset before and after the transition, the transcript notice, and the eventual spec implementation run's permission mode.
-
[P1] Return the same normalized plan that execution stores
internal/tui/plan_command.go:360Failure path: Put two
[in_progress]/[doing]rows in the plan file and reload it.reloadPlanFromFilereturns the raw parsed slice to the caller and sticky panel.SetPlancopies that slice and appliesenforceSingleInProgress, demoting one item only inupdate_plan.CurrentPlan(). The displayed/file plan and the plan described as the execution source of truth now disagree, without telling the user.Root cause: Normalization is hidden inside one consumer (
SetPlan) instead of being part of the parse/accept boundary. Multiple downstream consumers receive different representations of the same accepted input.Required outcome: Normalize once before publishing the snapshot, and use that exact canonical value for the tool, panel, durable representation, and session event. A useful API shape is for the accepting layer to return the canonical snapshot rather than letting a setter privately transform a copy, but the important contract is representation identity, not a specific API.
Regression coverage: Reload a file with multiple active rows and assert field-for-field agreement among
CurrentPlan, the sticky panel, formatted durable content, and the next-turn session context. -
[P2] Compare editor contents in their serialized form before recording an edit
internal/tui/model.go:1487Failure path: An agent-authored plan item can contain leading/trailing whitespace, tabs, carriage returns, or continuation shapes preserved by staging.
parsePlanFileLinestrims or normalizes those bytes. Opening the editor and quitting without a keystroke can therefore makeplanItemsEqual(beforeEdit, items)false, mutate the plan, and append “I edited the plan file directly” as a user message.Root cause: The no-op decision compares an in-memory pre-serialization structure with a post-serialization/post-parse structure. The comparison crosses a lossy boundary, so inequality does not prove the editor changed anything.
Required outcome: Determine whether the editor changed the staged bytes before applying lossy parsing, or canonicalize both the before and after values through exactly the same round trip before comparison. Only create the user-authored session event when the editor actually changed the canonical plan. Keep plan reload behavior for genuine edits and clears.
Regression coverage: Table-test whitespace, tabs, CRLF, multiline content, notes, escaped
Notes:lines, and an empty plan. For every unchanged staged file, assert no transcript/session event and no plan mutation; for a one-byte real edit, assert exactly one update event. -
[P1] Allow redirected ancestors above the protected Windows plan root
internal/planmode/read_windows.go:131Failure path:
openWindowsBaseDirpasses the full absolute storage-base path toNtCreateFilewithOBJ_DONT_REPARSE. Windows rejects reparse processing anywhere in that path, so a benign redirected profile/config ancestor, junction, orsubstdrive makes/plan on,/plan status, and every successfulupdate_planpersistence attempt fail. The diagnostic also incorrectly describes the protected storage root as a symlink.Root cause: The code conflates an ancestor needed to reach the user's legitimate config location with the protected plans root or a descendant being redirected after containment was established. Applying the no-reparse rule to the complete absolute path is stricter than the documented boundary and unlike the Unix contract that permits a legitimately redirected
~/.configancestor.Required outcome: Resolve/open the legitimate parent location first, bind it to a directory handle, and then open the final storage-root component and all descendants relative to that handle with Windows no-reparse protections. Continue refusing redirection at the protected root and below; do not weaken containment to regain availability.
Regression coverage: On Windows, cover junctions two and four levels above the plans root and a
substpath as accepted ancestors, plus junction/reparse replacement of the final root and intermediate plan components as refused cases for both read and write. -
[P2] Keep plan tests out of the real user home
internal/tui/plan_command_test.go:41Failure path:
isolatePlanConfigcallsos.UserHomeDir()and creates~/.cache/zero-planmode-test. The new tests fail in a valid read-only-home environment and leave a shared parent directory on developer/CI machines. On macOS and Windows, setting only XDG/AppData variables also does not redirect every root that standard-library config/cache helpers may resolve.Root cause: Production staging rejects the workspace and OS temp directory, so the tests work around that restriction by borrowing the developer's home instead of injecting a test-owned storage/staging base. The test seam does not cover every platform-specific root.
Required outcome: Add a narrow storage/staging-root injection seam and point it at a test-owned directory that is neither the workspace nor the default sandbox temp root. Redirect HOME, config, cache, AppData, and LocalAppData where platform APIs require them, or preferably pass the root directly so tests do not depend on ambient user-directory resolution.
Regression coverage: Run the affected packages with a read-only HOME and with real config/cache paths sentinel-protected. Assert the tests create nothing outside
t.TempDir()or the injected root on Linux, macOS, and Windows. -
[P1] Do not scrub the internal plan snapshot used for persistence
internal/tools/update_plan.go:97Failure path: A successful
update_plancall stores the canonical plan inupdatePlanTool.currentPlanand serializes another copy intoPlanSnapshotMeta. Registry result processing then runsscrubResultSecretsover every metadata value beforeOnToolResultdecodes that snapshot for the panel and durable file. A plan step containing an actual or false-positive secret-shaped string becomes[REDACTED]on disk and in the panel while tool memory retains the original. Resume later loads the altered file and makes that different plan authoritative.Root cause: Internal control data and user-visible/transcript metadata share the same mutable
Metatransport. A presentation security boundary correctly scrubs transcript data but unintentionally mutates the state-transfer payload.Required outcome: Carry the accepted plan through a typed internal snapshot field or callback payload that is immutable and excluded from transcript/session serialization. Continue scrubbing
Output, display fields, and ordinary metadata before they reach logs or UI. Persist exactly the canonical plan accepted by the tool; do not recover it by rereading shared mutable tool state.Regression coverage: Use secret-shaped fixtures and false-positive identifiers. Assert visible output/meta is redacted, while the internal snapshot, tool memory, panel plan, durable file, and resume result remain identical to the accepted canonical input.
-
[P2] Do not delete staged edits based only on age
internal/planmode/planmode.go:224Failure path: Every non-directory entry in the shared
plan-editdirectory whose mtime is older than six hours is removed. A second Zero instance can sweep the staged file of a first editor session that is legitimately still open; on Unix the editor may retain an unlinked descriptor and the eventual commit fails because the pathname is gone. The sweep has no filename or ownership filter, so any old non-directory entry is eligible.Root cause: Age is being used as proof of both ownership and abandonment. It proves neither: interactive edits can exceed the threshold, mtimes need not reflect editor liveness, and a shared directory can contain files from another process or version.
Required outcome: Give each staged edit explicit ownership and liveness state—for example a run/edit ID plus a held lock or lease—and reclaim only files proven to be Zero-owned and abandoned. If reliable proof is unavailable, leave the file for explicit cleanup rather than risking active user work. A filename prefix alone is an ownership filter, not sufficient liveness evidence.
Regression coverage: Hold one staged edit open past the threshold, start a second staging operation, and verify the first survives. Separately verify that a process-death/expired-lease case is reclaimed and that unrelated old files are never touched.
-
[P1] Preserve an in-memory parent plan across BTW
internal/tui/btw.go:213Failure path: The BTW side model is a value copy, but it shares the parent's registry pointer.
resetPlanForSessionSwitchon entry clears the sharedupdate_plantool, thereby clearing the parent's only in-memory plan too. Return restores from disk only; when a prior plan-file write failed or the durable file is missing, the error orok == falsebranches clear the parent's sticky panel, permanently losing the usable plan.Root cause: Session isolation is implemented as a shallow model copy around mutable shared tool state, then disk is treated as the only restoration mechanism even on paths where persistence is known to be fallible. The parent snapshot is never captured independently before the side reset.
Required outcome: Make plan state session-owned rather than registry-global, or capture an immutable parent snapshot before entering BTW and restore that exact snapshot when durable reload cannot provide a newer valid value. A failed reload should surface the error without destroying the last known-good parent plan. Preserve the isolation rule that the side conversation must not inherit or overwrite the parent's plan.
Regression coverage: Cover BTW entry/return with successful persistence, a forced write failure, a missing file, a read error, and a side plan update. Assert the parent canonical plan and panel survive failure, a valid newer durable plan wins when appropriate, and side state never leaks into the parent.
-
[P2] Make
/plan statusreflect the actual permission mode
internal/tui/plan_command.go:487Failure path:
planTextnever checksm.permissionMode. With a durable plan it prints “Current Plan (plan mode),” and without one it prints “Plan mode is active,” even after/plan offor a session transition restored a non-Plan mode. The command therefore gives a false read-only safety signal.Root cause: The presence or content of a saved plan is conflated with the active permission mode. Those are independent facts: a session may retain a plan after leaving plan mode, and plan mode may be active before any plan is written.
Required outcome: Build status from two explicit values: whether
PermissionModePlanis active and whether a saved/draft plan exists. Render both truthfully, such as “mode inactive; saved plan available” versus “mode active; no plan written.” Reuse the same status source for command output, view badges, and peer identity so UI surfaces cannot drift.Regression coverage: Test on/off with no plan, an in-memory draft, and a durable plan; then repeat after
/new,/resume,/btw, and/spectransitions. Assert both the permission state and exact status classification. -
[P1] Bind editor staging to the directory that passed validation
internal/planmode/planmode.go:234Failure path:
StageForEditorresolvesdir, verifies the resolved directory is outside sandbox-writable roots, tightens permissions, and checks it is not a symlink or reparse point. It then hands only the pathname tostageContentForEditor, which performs freshMkdirAll,Chmod, andCreateTemppath traversals. A concurrent rename/replacement after verification can redirect creation into a different directory, reopening the check-to-use race the staging hardening is meant to close.Root cause: Containment is proven for a pathname at one instant instead of being bound to the filesystem object used for creation. Repeating pathname operations after validation discards the identity that was checked. Random filenames and
O_EXCLprevent pre-planting the final leaf but do not protect traversal of replaced parent components.Required outcome: Open the validated staging directory once, verify that handle/object, and create the random leaf relative to it using handle-relative no-follow or reparse-safe APIs on Unix and Windows. Avoid re-running
MkdirAllandChmodby pathname inside the creation helper. Keep the random exclusive leaf and restrictive permissions as additional layers, not substitutes for traversal binding.Regression coverage: Add a deterministic seam that replaces or renames the directory after validation but before create, and assert creation either remains under the originally checked handle or fails closed. Exercise symlink and junction replacements on every supported platform and verify no file appears in the attacker-selected target.
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.
Most of these are symptoms of four shared design gaps. Fixing only the cited branch is likely to leave the same inconsistency reachable through another lifecycle path.
Why this PR keeps producing follow-up findings
The repeated review rounds are not happening because the feature needs an endless collection of isolated edge-case patches. They are happening because the implementation does not yet have one authoritative contract for permission lifecycle, plan state, persistence concurrency, or test isolation. Each fix has generally hardened the specific path named in the previous review, while a sibling producer or consumer continues to implement a slightly different rule.
The plan currently exists in several independently transformed forms:
- mutable state inside
update_plan; - a typed snapshot attached to a tool result;
- the sticky TUI panel;
- editable serialized text;
- the durable per-session file;
- reconstructed session/next-turn context;
- entry-time fallback snapshots used by session and BTW transitions.
Those forms do not share a single acceptance and publication boundary. Some normalize statuses, some trim text, some infer presence from length, some are updated asynchronously, and some are restored only after storage succeeds. That is why fixing secret scrubbing with typed transport exposed the empty-snapshot presence bug, fixing editor no-op reporting still left a lossy comparison, and fixing BTW restoration with an entry snapshot still left the hidden-parent update race.
The same pattern exists in permission handling. Plan mode is represented by permissionMode, permissionModeBeforePlan, session identity, paused continuations, the advertised toolset, and visible status, but /plan, /spec, /new, /resume, and /btw each assemble their own subset of the transition. A local fix can make one command correct while another still widens authority or restores stale state.
Filesystem hardening has likewise been applied at individual pathname operations. Security and correctness here depend on longer-lived identity: which directory object was validated, whether a file was created by Zero, whether its owner is still alive, and which durable version an editor staged. A suffix, resolved path, mtime, or final atomic rename answers only one part of that contract.
Finally, the tests have mostly accumulated one regression per reported symptom. They do not yet assert the complete state vector across every transition, so a patch can satisfy the new test while moving the inconsistency to the panel, durable bytes, next-turn context, another platform, or another session path. Broad edits to shared test files have also removed unrelated live-target protections, reducing the suite's ability to catch that movement.
The findings map to those causes as follows:
| Root contract | Findings it explains | Shared invariant that is missing |
|---|---|---|
| Permission/session lifecycle | read-only exit; BTW fallback | One transition decides authority, ownership, continuation state, visible status, and restoration together. |
| Canonical plan publication | noncanonical reload; false editor edit; empty clear; BTW fallback | One accepted typed snapshot—with explicit presence and provenance—is published unchanged to every internal consumer. |
| Persistence identity and concurrency | Windows ancestors; stale-file ownership; editor overwrite | Operations remain bound to the validated directory/object, owned staging invocation, and baseline durable version. |
| Hermetic regression protection | agent-loop test rollback; real-home writes; missing lifecycle combinations | Tests inject every external root and assert all representations/effects for the full transition matrix. |
Overall guidance
- Make plan mode a single lifecycle contract. Entry, explicit exit, session changes, spec mode, BTW, queued work, the prior permission mode, and user-visible status should go through one transition boundary. That boundary should decide whether the read-only gate remains active, requires an explicit exit, or blocks the transition, and should update all dependent state atomically.
- Accept and publish one canonical typed plan snapshot. Normalize once at the tool/file input boundary, then use the same immutable value for tool memory, the sticky panel, durable storage, and next-turn context. Text formatting and parsing should be representation boundaries, not additional hidden normalization stages.
- Bind persistence to identity, ownership, and version. A staged editor file needs positive Zero ownership, liveness, and the durable version it was based on. Cleanup must prove ownership and abandonment; commit must detect a newer durable value instead of blindly replacing it.
- Test the lifecycle matrix hermetically. Inject session/config/cache/staging roots, then cover enter/exit, session/spec/BTW transitions, empty and noncanonical plans, editor no-op/edit/conflict, persistence failures, restart, and Unix/Windows redirection. For each case, assert permission mode, canonical tool state, panel state, durable bytes, session context, and cleanup effects.
Suggested repair sequence
- Write down the state invariants before changing more branches. At minimum: Plan authority never widens implicitly; one session owns one canonical plan; every successful update, including an empty clear, carries an immutable present snapshot; tool, panel, disk, and context agree after every completed transition; a failed restore never replaces newer known-good state; cleanup deletes only proven-owned abandoned files; and an editor cannot silently overwrite a newer durable version.
- Introduce the canonical acceptance boundary. Parse and normalize tool/file input once, retain explicit snapshot presence and session/run provenance, and return the accepted immutable snapshot from the accepting operation. Make panel, persistence, context, and fallback state consume that value rather than re-reading shared tool state or privately normalizing another copy.
- Centralize lifecycle transitions. Give plan entry, explicit exit, session switch, spec, and BTW one transition API that receives the intended event and atomically produces the new permission mode, prior-mode state, session ownership, continuation state, plan snapshot, and user-visible notice. Command handlers should request a transition, not manually combine
exitPlanMode, reset, reload, and resume helpers. - Add versioned editor semantics and owned cleanup. Stage the canonical bytes together with their baseline durable identity/version and a positively identifiable Zero invocation. On return, distinguish unchanged, edited-without-conflict, and edited-after-concurrent-update cases before writing. Reclaim only an owned invocation whose liveness is conclusively gone.
- Bind platform storage to handles at the intended trust boundary. Permit legitimate config ancestors, then open the protected plan root relative to the accepted parent and no-follow every protected component. Use the same object-bound principle for staging creation and reclamation on Unix and Windows.
- Build table-driven lifecycle tests before applying the final fixes. Drive each event from each relevant starting mode and plan source, inject read/write/missing/conflict outcomes, and assert the entire state vector. Run the same storage contract with redirected ancestors and protected-root replacement on Windows, plus symlink paths on Unix. Keep all session/config/cache roots test-owned.
- Restore unrelated mainline assertions and then make production changes. This prevents the repair from trading plan-mode correctness for regressions in spec safety, tool-result history, or trace integrity.
Completion invariants
This should be ready for another review when the implementation and tests demonstrate all of the following, rather than only the ten examples below:
- Leaving or switching a conversation cannot widen Plan authority without an explicit visible user action.
- A plan accepted from either
update_planor the editor has exactly one canonical typed representation. - Successful nonempty updates and successful empty clears are both present snapshots and propagate identically.
- Tool memory, sticky panel, durable bytes, resume state, and next-turn context agree after update, edit, clear, restart, session switch, BTW, and persistence failure.
- Opening and closing an unchanged editor is observationally a no-op.
- A concurrent newer plan is never silently replaced by an older unchanged editor snapshot or stale transition fallback.
- Benign user-profile/config redirection remains supported, while replacement at the protected storage root or below fails closed.
- Cleanup touches only files proven to belong to an abandoned Zero staging invocation.
- Tests write only to injected review-owned roots and preserve unrelated mainline security/protocol assertions.
Findings
-
[P1] Keep the read-only gate until an explicit visible exit
internal/tui/plan_command.go:147Failure path: Enter plan mode from Auto, Ask, or Unsafe, then invoke
/spec,/new, cross-session/resume, or/btw. The current head removes/specfromplanModeCommandUnavailable, and these paths callexitPlanModewithout/plan offor confirmation. The visible surface can therefore regain implementation tools even though the user never explicitly relaxed the read-only gate.Root cause: Plan mode is split between a global permission value, a saved prior mode, and independently implemented session/side-surface transitions. Those transitions call low-level reset and exit helpers without a single policy deciding whether authority may widen.
Required outcome: Route every plan/session/spec/BTW transition through one explicit transition boundary. Preserve the read-only gate unless the user visibly exits or confirms the transition, while retaining
/plan offrestoration and conversation isolation.Regression coverage: Exercise all four commands after entering Plan from Auto, Ask, and Unsafe. Assert the resulting permission mode, advertised toolset, transcript notice, and any later implementation run's mode.
-
[P1] Publish the same canonical plan that execution stores
internal/tui/plan_command.go:359Failure path: Reload a plan file containing two
[in_progress]/[doing]rows.SetPlan(items)copies the slice and privately demotes the earlier active item, whilereloadPlanFromFilereturns the raw two-active slice.CurrentPlan()then disagrees with the sticky panel and editor-authored session event, and the durable bytes remain noncanonical.Root cause: Normalization is hidden inside one consumer instead of being part of the parse/accept boundary, so downstream consumers receive different representations of the same accepted input.
Required outcome: Normalize once before publishing the reload and distribute that exact snapshot to tool memory, panel, durable file, and session context. Whether
SetPlanreturns the accepted value or the parser normalizes first is an implementation choice; representation identity is the required contract.Regression coverage: Reload files with multiple active rows and status aliases, then assert field-for-field agreement among
CurrentPlan, panel items, formatted durable content, and next-turn context. -
[P2] Determine editor no-ops before the lossy parse boundary
internal/tui/model.go:1463Failure path: An agent plan contains leading/trailing whitespace, tabs, CRLF, or another shape normalized by
parsePlanFileLines. The user opens the editor and exits without changing a byte.beforeEditis the pre-serialization structure, whileitemsis the trimmed post-parse structure, soplanItemsEqualreports a change, mutates the plan, and appends the false user-authored “I edited the plan file directly” event.Root cause: The no-op comparison crosses a lossy serialization boundary. Structural inequality after parsing does not prove that the editor changed the staged file.
Required outcome: Compare the staged bytes with their pre-editor baseline before parsing, or canonicalize both before and after through exactly the same round trip. Preserve genuine edits and clears, but emit no mutation or user event for unchanged canonical content.
Regression coverage: Table-test unchanged and one-byte-edited content containing whitespace, tabs, CRLF, multiline values, notes, escaped
Notes:lines, and an empty plan. Assert exactly zero or one edit event as appropriate. -
[P1] Allow redirected Windows ancestors above the protected plan root
internal/planmode/read_windows.go:116Failure path: The user's profile, AppData, or another legitimate config ancestor is a junction, redirected folder, or
substpath.openWindowsBaseDirpasses the complete absolute storage-base path toNtCreateFilewithOBJ_DONT_REPARSE, so Windows rejects the ancestor traversal before the protectedplansroot is opened. Plan reads, writes, staging,/plan status, and successful update persistence then fail for that supported layout.Root cause: The Windows implementation applies the no-reparse boundary to every ancestor needed to reach the user's legitimate config location, rather than to the protected storage root and descendants. This is stricter than the Unix contract and conflates benign ancestor redirection with redirection of an owned security boundary.
Required outcome: Resolve/open the legitimate parent first, bind it to a directory handle, and no-follow the final protected root plus every descendant relative to that handle. Continue refusing reparse replacement at the protected root and below.
Regression coverage: Accept junctions at multiple ancestors and a
substpath, while refusing a reparse point at the final plan root and intermediate descendants for both read and write. -
[P2] Prove staged-file ownership before reclaiming Markdown files
internal/planmode/planmode.go:207Failure path: Put an old unrelated
notes.mdinzero/plan-editwith no companion lock, then invoke/plan open. The sweep treats the.mdsuffix as sufficient ownership evidence; the platform reclaimer treats the missing/open-failed lock as abandonment and deletes the file. The regression test protectsnotes.txt, so it does not exercise this path.Root cause: Filename extension and age are being used as proof of both ownership and abandonment. Neither identifies a Zero staging invocation, and a missing lock is not positive ownership evidence.
Required outcome: Reclaim only entries positively identified as Zero-owned staged edits and proven abandoned through a generated-name/ownership marker plus a lock, lease, or equivalent liveness mechanism. Preserve active edits and leave unrelated files untouched.
Regression coverage: Verify that old unrelated
.mdfiles and active staged edits survive, while a positively identified abandoned Zero staging file is reclaimed on Unix and Windows. -
[P1] Preserve presence when a successful update clears the plan
internal/tui/plan_command.go:565Failure path: After a nonempty plan, call
update_planwithplan: []. The tool accepts the call, clears its current plan, and returns a non-nil emptyPlanSnapshot.planSnapshotFromResultuseslen > 0as the presence test, so the callback skips the panel update andWritePlan. Tool memory says empty while the panel and durable file retain the old plan, which a later resume can resurrect.Root cause: Moving the state-transfer payload from encoded metadata to a typed slice lost the distinction between “successful empty snapshot” and “no snapshot because the call failed or was cancelled.” Length is being used as a presence bit.
Required outcome: Represent snapshot presence explicitly, or preserve nil versus non-nil semantics, and propagate a successful empty snapshot through panel, durable storage, and session consumers. Do not recover it by rereading the shared mutable tool.
Regression coverage: Cover nonempty update, empty clear, invalid arguments, cancellation, and a session switch between tool completion and the result callback. Assert the callback publishes only the snapshot carried by that result.
-
[P2] Restore unrelated mainline agent-loop regression coverage
internal/agent/loop_test.go:3448Failure path: This branch removes or weakens the live target's tests for spec-draft name-spoofed control tools, structured tool-error history, aborted-result
IsError, and trace context/prefix propagation. Equivalent integration assertions are absent at head, so those security and protocol regressions can return without failing this package's tests.Root cause: The feature branch rewrites a broad shared test file while its production agent change only adds typed
PlanSnapshottransport. Unrelated mainline assertions were dropped during that rewrite instead of being preserved or deliberately relocated.Required outcome: Restore equivalent end-to-end assertions for each removed contract without redesigning unrelated production code. If coverage is relocated, keep the same integration boundary rather than replacing it only with lower-level unit tests.
Regression coverage: Retain the spoofed spec-tool execution denial, tool-result error propagation, aborted-placeholder error status, and
OnContext/trace prefix-hash agreement assertions present at the live target. -
[P2] Detect a newer durable plan before committing an editor snapshot
internal/planmode/planmode.go:334Failure path: Instance A stages plan A. Instance B writes newer plan B for the same workspace/session. A exits its editor without changing the staged bytes.
CommitStagedEditunconditionally writes A over B before the TUI performs its no-op comparison, so the newer plan is lost and the completion path may report no edit.Root cause: The staged edit records content but not the durable version/content it was based on. Commit is an unconditional last-writer-wins operation, and no-op detection occurs after the destructive write.
Required outcome: Bind staging to a baseline version or content hash and compare before commit. An unchanged stale editor must not overwrite a newer durable snapshot; an intentional edit concurrent with another update should follow an explicit conflict policy rather than silently losing either value.
Regression coverage: Stage A, persist B from another instance, then cover both unchanged and intentionally edited A. Assert unchanged A preserves B and concurrent real edits produce the chosen explicit conflict behavior.
-
[P2] Restore the latest hidden-parent plan on BTW fallback
internal/tui/btw.go:155Failure path: BTW captures
parentPlanItemsat entry. While the side surface is visible, a pending hidden-parent run completes and its routedplanUpdateMsgupdates the parent model. If the durable plan is missing or reload fails on return,leaveBTWinstalls the entry-time slice into the shared tool and panel, overwriting the newer hidden-parent result.Root cause: The fallback snapshot is detached from the hidden parent after entry, even though routed messages continue mutating that parent. Durable storage is treated as the only newer source, so its failure exposes the stale capture.
Required outcome: Keep the fallback synchronized with routed parent plan updates or derive it from the latest hidden-parent state at return. Preserve BTW side isolation and let a successfully loaded valid durable plan continue to take precedence.
Regression coverage: Enter BTW with a parent plan, route a newer parent
planUpdateMsg, and return under successful reload, missing-file, and read-error conditions. Assert the latest valid parent plan survives and side state never leaks. -
[P2] Isolate
/plan ontests from the real session store
internal/tui/plan_mode_test.go:21Failure path: The new
/plan onpath callsensureActiveSession, but these tests constructnewModelwithout a testSessionStoreor complete data-root redirection. They write persistent sessions under the developer's real user-data directory and fail before entering plan mode when HOME/XDG data is read-only.Root cause: A new production persistence dependency was added to a previously in-memory command path without updating legacy tests to inject that dependency. The tests therefore depend on ambient platform-specific home/data resolution.
Required outcome: Give every affected model a test-owned session store and redirect any remaining config/data roots across Linux, macOS, and Windows. Preserve production session creation on
/plan on.Regression coverage: Run the focused plan-entry tests with a read-only real home and sentinel-protected user-data locations. Assert all writes remain inside the injected test root.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified at 8c25c35. The three commits since my last pass do not touch the paths below. Three of my items are closed and four stay open; jatmn raised all four open ones at this head and I agree with him on each, so this stays at request changes.
Closed:
- Windows staging containment through resolvePhysical. Reverting both call sites to filepath.EvalSymlinks fails the two junction tests on their property lines (planmode_windows_test.go:33, :43, :65). Junction chains, a junction at the parent and an upper-cased root are all refused. Correction to my earlier wording: two tests with three assertions, not three tests.
- The reparse backstop and its coverage: the junction tests run on an unelevated box; the skips are the symlink-privilege ones.
- Queued prompts are held in plan mode. Removing the planModeBlocksContinuations clause fails plan_command_test.go:1059, and every launch path goes through launchQueuedMessageIfReady.
Open, driven on both trees from Auto and then /plan on:
- /spec, /btw, /new and /resume still drop plan mode. On this head each returns the session to the pre-plan mode with only the footer label changing and nothing in the transcript; base keeps the read-only gate on all four and refuses /spec with "is unavailable in plan mode". The base gate list has commandSpec and this one does not, and the exits are pinned by name in your own tests, so I read it as a deliberate choice. It still widens authority on a session switch, which is jatmn's P1 at plan_command.go:147: five call sites assemble their own exit and there is no single visible transition.
- Reload hands raw items to the caller and panel and enforced items to the tool. A file with two [doing] steps gives the tool [completed in_progress pending] and the panel [in_progress in_progress pending], with no demotion notice. That is jatmn's canonical-plan P1.
- A no-op editor session claims a user edit. Driven end to end through update_plan, WritePlan, StageForEditor and CommitStagedEdit with no keystroke, six of seven whitespace shapes append a role=user "I edited the plan file directly" event and the "Reloaded the edited plan." row. Correction to my earlier "five of seven". jatmn has this as P2; I keep it in this round because it writes a false user-authored event into the session.
- A benign reparse point above the plans root kills plan I/O on Windows. A junction one or three levels above AppData, or a subst drive, makes WritePlan, ReadPlan and StageForEditor all refuse with "plan storage root ... is a symlink", so /plan on enters with a reload error and every successful update_plan logs a write error. OBJ_DONT_REPARSE is whole-path; read_unix.go:25 says a symlinked ~/.config above the root is fine, and Windows should match it by resolving the legitimate parent and no-following only from the root down. That is jatmn's P1 at read_windows.go:116.
Note only: the reparse guard at planmode.go:286 is unpinned. Stubbing pathIsReparsePoint to false leaves TestVerifyPrivateDirectoryRejectsJunction passing, because the refusal comes from the IsDir check.
3 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/...Summary by CodeRabbit
New Features
$VISUALor$EDITOR./plancommands to view, open, enable, disable, and exit plan mode.Bug Fixes