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 39 commits into
Conversation
WalkthroughThis change adds read-only plan mode, secure durable plan storage, editor workflows, ChangesPlan mode and durable plan workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant PlanCommand
participant PlanMode
participant UpdatePlan
participant AgentLoop
User->>TUI: invoke /plan
TUI->>PlanCommand: handlePlanCommand
PlanCommand->>PlanMode: read or write durable plan
PlanCommand->>UpdatePlan: load plan state
TUI->>AgentLoop: run with plan prompt
AgentLoop->>UpdatePlan: update plan
UpdatePlan-->>TUI: return plan snapshot metadata
TUI->>PlanMode: persist successful snapshot
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- restrict plan file permissions to owner only (0o700 dir, 0o600 file) - surface editor failures from /plan open in the transcript - simplify fileExists to return a bool - collapse redundant plan path resolution in planText
/plan open was non-functional because run.go assigned the live program to a copy of the model after tea.NewProgram had already captured it by value; the field is removed and tea.ExecProcess is used directly. Shift+Tab no longer silently drops plan mode, planmode.DraftSystemPrompt is wired into plan-mode runs, plan file paths reject symlink escapes, read errors are no longer swallowed, opening a new plan file seeds it from the agent's draft instead of leaving it blank and shadowing that draft, /plan off restores the prior permission mode instead of forcing Auto, and the session slug is stable when no session ID exists yet.
…g, and persistence Make a bare /plan toggle off when already active instead of only reprinting the plan. Scope plan mode to the session that entered it: /new and /resume to a different session now exit plan mode instead of leaking a stale grant or restore-mode across sessions. Create the active session before naming its plan file so a fresh TUI no longer collides on a shared plan.md. Persist every update_plan call to the plan file so it is the durable source of truth instead of an in-memory snapshot. Replace the preflight Lstat symlink check with os.Root, closing the check/use race via descriptor-relative operations. Skip the plan-file permission assertions on Windows, where POSIX mode bits aren't meaningful.
…odes exitPlanMode() unconditionally reset permissionMode to Auto before restoring permissionModeBeforePlan, so /new and /resume to a different session dropped an explicit Ask/Auto choice made outside plan mode. Only touch permissionMode when actually leaving PermissionModePlan.
…path string(permissionMode) already yields "plan" and "spec-draft" for the two modes this branch handles, so the if/else was reassigning identical values.
…tus and notes /plan open let the user edit the plan file in $EDITOR, but the edit was never synced back into the in-memory update_plan, so it kept driving execution off the stale pre-edit draft. reloadPlanFromFile() now parses the saved file and pushes it back into update_plan via a new SetPlan method. The first version of that parser discarded each item's [status] bracket (resetting everything to pending on reload) and mis-parsed a "Notes: ..." continuation line as its own bogus plan step. Both are fixed: status is parsed back through the tool's existing normalization, and a Notes line folds into the preceding item instead of becoming a new one.
The palette showed "/plan - Show planning mode status" but /plan actually toggles plan mode and supports open/off subcommands.
…and session reset - executeRequestPermissions now denies plan/spec-draft mode unconditionally, instead of relying on the registry-based ToolAdvertised gate, which only fires when the tool happens to be present in whatever registry the caller passed in. - /new and /resume now clear the shared update_plan state and sticky plan panel on a session switch, not just the permission mode. - A successful $EDITOR exit from /plan open now always emits planEditorFinishedMsg, so edited plan content actually reloads instead of being silently dropped. - /plan open now blocks while a run is active, matching the bare /plan toggle's guard. - parsePlanFileLines now folds multi-line Notes blocks instead of treating continuation lines as bogus new steps.
…ext, other findings - /plan open now stages the plan file for $EDITOR in config.UserConfigDir() instead of handing it a workspace-relative path: ReadPlan/WritePlan resolve through os.Root and can't be redirected, but the external editor process opens its argument path with ordinary I/O, so a sandboxed tool invocation could previously replace the plan file with a symlink between our protected write and the editor's open. The OS temp directory doesn't avoid this since the sandbox's default write scope explicitly includes it. - A user-edited plan now gets recorded as a session event on reload, so it actually reaches the model's context instead of only updating the update_plan tool's in-memory state, which the model has no way to observe on its own. - /resume now hydrates the destination session's own persisted plan file after a session switch, instead of leaving update_plan and the sticky panel empty until the next update_plan call risks overwriting it. - formatPlanItems/parsePlanFileLines now indent multi-line Content continuations the same way Notes continuations already were, so agent-authored multi-line plan steps survive a round-trip through $EDITOR instead of shattering into bogus new pending steps. - WritePlan now Chmods the plan directory and file unconditionally after MkdirAll/OpenFile, since those only apply their mode at creation and would otherwise leave a pre-existing, more permissive dir/file broadly readable. - /plan open now checks plan mode is active before ensureActiveSession instead of after, so an invalid invocation doesn't leave a persistent empty session behind in /resume.
- StageForEditor rejects a staging directory that XDG_CONFIG_HOME has redirected into the sandbox's default-writable roots (the workspace or the OS temp directory) instead of silently staging somewhere a sandboxed process could symlink-swap. - The staged file is created per invocation via os.CreateTemp: a random, unpredictable name opened with O_EXCL, so a planted path is refused rather than followed, and two Zero instances editing the same resumed session no longer overwrite each other's staged draft. Cleanup removes only the file this invocation created. - Clearing every line in the editor now records an explicit plan-cleared user event in the session context, so the next run does not replay the discarded plan from the earlier update_plan call. - $VISUAL/$EDITOR values are parsed with POSIX shell word-splitting (mvdan.cc/sh/v3/shell, already a dependency) instead of strings.Fields, so quoted executable paths with spaces and flags launch correctly. - The /plan palette description says the literal "off" subcommand, and the help expectation matches.
…an state, lossless plan encoding
- The editor staging containment check now judges physical paths: the
staging directory is created first, resolved with EvalSymlinks, checked
against the symlink-resolved workspace and temp roots, and the staging
itself is anchored on the resolved path. An XDG_CONFIG_HOME symlinked
into a sandbox-writable root no longer passes on its lexical spelling.
- update_plan refuses to apply once its run context is cancelled, with the
check sharing the mutex that guards SetPlan, so a cancelled run's late
call can no longer repopulate the plan the UI just reset for a new
session; the UI-side file sync also runs only on successful results, so
a refused call cannot rewrite the old session's plan file either.
- The plan file encoding round-trips losslessly: indentation is decided
before content (a continuation reading "2. validate" stays a
continuation), continuations whose text would read as structure
("Notes:" or a leading backslash) are escaped, and whitespace-only
indented lines survive as blank continuation lines. Round-trip tests
cover the adversarial cases and assert a fixed point on the second pass.
…xisting ancestor The macOS and Windows CI runners spell temp paths through symlinks (/var -> /private/var) and 8.3 short names (RUNNER~1): a staging directory that does not exist yet kept its lexical spelling while the existing roots resolved to physical form, so the containment comparison silently missed. physicalPath now resolves the deepest existing ancestor and rejoins the remainder, giving both sides the same spelling.
The planEditorFinishedMsg handler reloaded the edited plan file into both the update_plan tool and the sticky panel, but emitted no visible confirmation, so a bare /plan open with no other change looked like a no-op. Append a system message noting the reload (or a clear when the edited file is empty), and cover the full Update message path with a test asserting the tool state, panel, and transcript are all updated.
Three review findings: - Unknown /plan subcommands (a typo like "openx", or "status") fell through the switch to the bare toggle and silently exited the read-only mode. They now return a usage error; only bare /plan toggles. - WritePlan opened the plan path with O_TRUNC, destroying the previous durable plan before the new content landed, and followed a symlink that resolves inside the workspace — a planted .zero/plans/<slug>.md symlink would redirect plan mode's one allowed write over an arbitrary workspace file. It now refuses symlinked targets and writes an owner-only O_EXCL temporary sibling renamed into place. - The update_plan result callback re-read the shared tool's CurrentPlan() after the call released its mutex, so a cancel plus /new or /resume in that window persisted the wrong session's plan (or an empty reset) under the old run's session ID. A successful call now carries its own plan snapshot in the result meta and the callback persists exactly that snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool.
… registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ctive Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entering plan mode replaced options.SystemPrompt wholesale with planmode.DraftSystemPrompt, discarding any embedder-configured system prompt for the whole duration of plan mode. Layer the plan-mode instructions onto the configured prompt instead, falling back to the plain draft prompt when nothing was configured. Also chmod the plan-edit staging directory unconditionally after MkdirAll, so a pre-existing, loosely permissioned directory no longer undermines the staging design's symlink-race protection.
Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model so trusted worktrees inherit trust under --use-spec --worktree (TestExecSpecWorktreeInheritsTrustEndToEnd). Also close two plan-mode advertisement gaps that Gitlawb#642 already fixed: exclude process-spawning lsp_navigate, and require Safety metadata for tools instead of a name-only ask_user/update_plan allowlist, with a spoofed-name regression test.
update_plan is read-only and auto-allowed, but the TUI persisted every successful call into .zero/plans under the workspace. Store durable plans under the user config directory (scoped by workspace) so Ask mode and plan mode no longer create workspace files without a write grant. Also verify the editor staging directory is a plain owner-only dir after chmod (reject group/world-writable or symlink paths), cover the pre-existing permissive staging-dir case, and assert plan mode layers DraftSystemPrompt onto a configured agent system prompt rather than replacing it.
…ing checks os.UserConfigDir (what config.UserConfigDir defers to outside darwin) reads %AppData% on Windows and ignores XDG_CONFIG_HOME there, so tests that only set XDG_CONFIG_HOME silently fail to isolate plan storage on Windows and fall through to the runner's real profile directory. Set AppData too wherever a test overrides the config root. Also skip the new group/world-writable check in verifyPrivateDirectory on Windows: NTFS reports a directory's POSIX mode via ACLs rather than the bits os.Chmod sets, so the check rejected every staging directory unconditionally and made /plan open never launch $EDITOR on Windows, the same rationale already used to skip the file-mode assertion in TestWritePlanUsesRestrictivePermissions.
slugify alone maps distinct session/workspace IDs that differ only by separator (plan_a vs plan-a) onto the same path. pathKey appends a SHA-256 suffix of the exact original string so durable plans stay isolated across those collisions. Refs Gitlawb#643
…n mode completion, and continuation whitespace
…ool policy vetoes Reset plan mode when drafting or approving specs, preserve beforeTool policy vetoes during plan mode, reject plan storage in temp tree, and hash unmodified identifiers in pathKey. Refs Gitlawb#643
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
internal/planmode/read_windows.go (1)
185-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConfirm the missing-plan path on Windows still returns "no plan", not an error.
mapWindowsOpenErrmaps onlySTATUS_OBJECT_NAME_NOT_FOUNDandSTATUS_OBJECT_PATH_NOT_FOUNDtoos.ErrNotExist.NtCreateFilecan also returnSTATUS_NO_SUCH_FILEwhen the parent directory exists but the final name does not. That status falls to thedefaultbranch and returnsst.Errno().
RtlNtStatusToDosErrorprobably convertsSTATUS_NO_SUCH_FILEtoERROR_FILE_NOT_FOUND, whichos.IsNotExistaccepts, soReadPlanwould still return("", false, nil). Confirm that conversion before relying on it. If it does not hold, a first-time/planon Windows reports a read error instead of an empty plan.Does RtlNtStatusToDosError map STATUS_NO_SUCH_FILE to ERROR_FILE_NOT_FOUND?🤖 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_windows.go` around lines 185 - 204, Verify that STATUS_NO_SUCH_FILE is converted by RtlNtStatusToDosError to ERROR_FILE_NOT_FOUND and remains recognized by os.IsNotExist in the ReadPlan missing-plan path. If not, update mapWindowsOpenErr to map windows.STATUS_NO_SUCH_FILE to os.ErrNotExist while preserving the existing mappings.
🧹 Nitpick comments (5)
internal/planmode/read_other.go (1)
10-36: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider failing closed on platforms without no-follow primitives.
This fallback keeps the
Lstat/Openrace the Unix and Windows walkers were written to remove.os.Rootbounds the escape to the storage tree, so the residual risk is following an in-root symlink planted between the two calls. That risk is small, but the code path contradicts the fail-closed rule the rest of the package follows.Two options: return an explicit "unsupported platform" error, or state in the comment why in-root symlink following is accepted here. The current comment describes the race but does not justify accepting it.
As per coding guidelines: "Fail closed on ownership, lease, and permission checks."
🤖 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_other.go` around lines 10 - 36, Update openPlanUnderBase to fail closed on platforms lacking no-follow primitives by returning an explicit unsupported-platform error instead of performing the racy Lstat/Open fallback. Remove or revise the fallback logic and comments so they no longer imply that in-root symlink following is accepted.Source: Coding guidelines
internal/planmode/planmode_test.go (1)
441-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-regular-file case for
readPlanFile.All three platform implementations reject a non-regular final component:
S_IFREGon Unix,FILE_ATTRIBUTE_DIRECTORYon Windows, andMode().IsRegular()in the fallback. No test in this package exercises that branch, so a regression in any one implementation would pass CI.Add a case that creates a directory at the plan path and asserts
readPlanFilereturns the "is not a regular file" error. That case is hermetic and runs on every platform, unlike the symlink tests which skip when symlinks are unavailable.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."
🧪 Proposed test
func TestReadPlanFileRejectsNonRegularFile(t *testing.T) { base := t.TempDir() dir := filepath.Join(base, "ws-key") path := filepath.Join(dir, "session.md") if err := os.MkdirAll(path, 0o700); err != nil { t.Fatalf("mkdir plan path: %v", err) } data, err := readPlanFile(base, path) if err == nil { t.Fatalf("expected a directory at the plan path to be refused, got %q", data) } if !strings.Contains(err.Error(), "not a regular file") { t.Fatalf("expected the non-regular-file refusal, 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 441 - 459, Add a TestReadPlanFileRejectsNonRegularFile test alongside TestReadPlanFileRoundtripPlainFile. Create the plan path as a directory, call readPlanFile, assert an error is returned, and verify its message contains "not a regular file"; retain the existing hermetic setup and avoid relying on symlinks.Source: Coding guidelines
internal/tui/plan_command_test.go (2)
332-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCanonicalize both paths before the containment assertion.
cwdcomes fromt.TempDir(). On macOS that returns a/var/folders/...path that is a symlink to/private/var/.... The durable plan path is derived from a resolved base. The prefix comparison therefore compares two different spellings of the same tree.The assertion is negative, so a spelling mismatch makes it pass without proving anything on macOS. Resolve both sides first so the check keeps its value on every platform.
🛠️ Proposed fix
- if strings.HasPrefix(path, cwd+string(os.PathSeparator)) || path == cwd { + resolvedCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + t.Fatalf("EvalSymlinks(cwd): %v", err) + } + resolvedPath, err := filepath.EvalSymlinks(filepath.Dir(path)) + if err != nil { + t.Fatalf("EvalSymlinks(plan dir): %v", err) + } + if strings.HasPrefix(resolvedPath, resolvedCwd+string(os.PathSeparator)) || resolvedPath == resolvedCwd { t.Fatalf("durable plan path %q must not live under the workspace %q", path, cwd) }As per coding guidelines: "canonicalize paths before comparison and avoid asserting raw temporary-directory spellings".
🤖 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 332 - 337, Canonicalize both cwd and the durable plan path before the containment assertion in the update_plan test. Resolve the temporary workspace path and the generated path using the existing filepath resolution APIs, then compare the canonical paths while preserving the assertion that the plan is outside the workspace; avoid relying on raw TempDir spellings.Source: Coding guidelines
20-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep plan config fixtures under
t.TempDir()instead of$HOME.
isolatePlanConfigcallsos.RemoveAllandos.MkdirAllunderos.UserHomeDir(), so every affected test write/delete path lands in the real developer home and fails when$HOMEis unavailable. This hook is shared by tui tests; use a workspace-relative config staging root and redirectplanmode.SetTempDirForTestto preventStageForEditorfrom rejecting it. The file already importsinternal/planmode, so this does not require an extra exported helper.🤖 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 20 - 54, The isolatePlanConfig helper must stop creating and deleting fixtures beneath os.UserHomeDir. Use t.TempDir() for the staging root, set the platform-specific config environment variables to that root, and redirect planmode.SetTempDirForTest to the same test workspace so StageForEditor accepts it. Preserve the existing cleanup and cross-platform environment setup.Source: Coding guidelines
internal/tui/spec_mode_test.go (1)
286-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the restored mode, not just "not plan".
Line 293 seeds
permissionModeBeforePlanwithagent.PermissionModeAuto. Line 298 only checks that the mode is no longerPermissionModePlan. The test therefore passes if/specrestoresAsk, or any other mode, instead of the recorded prior mode.
exitPlanModepromises to restore the recorded mode. Assert that directly so a regression in the restore logic fails this test.🛠️ Proposed fix
- if next.permissionMode == agent.PermissionModePlan { - t.Fatalf("expected /spec to exit plan mode, got %s", next.permissionMode) + if next.permissionMode != agent.PermissionModeAuto { + t.Fatalf("expected /spec to exit plan mode and restore Auto, got %s", next.permissionMode) + } + if next.permissionModeBeforePlan != "" { + t.Fatalf("expected permissionModeBeforePlan cleared on exit, got %q", next.permissionModeBeforePlan) }🤖 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/spec_mode_test.go` around lines 286 - 301, Update TestSpecCommandExitsPlanMode to assert that next.permissionMode equals the recorded agent.PermissionModeAuto value, rather than merely checking that it is not agent.PermissionModePlan. Keep the existing test setup and failure reporting focused on verifying restoration of permissionModeBeforePlan.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/planmode/planmode.go`:
- Around line 420-437: Update pathKey to cap the slugify(id) prefix so the
resulting filesystem component remains within NAME_MAX while retaining the
SHA-256 suffix for uniqueness; preserve the existing fallback and suffix
behavior. Add a regression test covering a workspace path longer than 255
characters that verifies plan path creation and read/write operations succeed.
In `@internal/planmode/read_unix.go`:
- Around line 48-55: Update the final open flags in readPlanFile around
openatRetry to include unix.O_NONBLOCK alongside O_RDONLY, O_NOFOLLOW, and
O_CLOEXEC, while preserving the existing symlink-error and general-error
handling and subsequent regular-file validation.
In `@internal/tui/btw.go`:
- Around line 212-219: In the reload error branch of the parent leave flow,
clear the copied parent plan panel/state before appending the transcript error
so stale plan data is not returned when reloadPlanFromFile fails. Update
TestBTWLeaveReportsPlanReloadError to assert that the returned parent panel is
empty, while preserving the existing successful reload behavior.
In `@internal/tui/plan_command.go`:
- Around line 54-61: Update both plan-mode exit paths in the "off", "exit"
handling and bare toggle-off branch to check m.pending before changing
m.permissionMode. When a run is in flight, preserve plan mode and follow the
same guarded behavior used by the plan-entry path; only call exitPlanMode and
append the successful exit message when no run is pending.
- Around line 195-206: Update the editor command parsing around shell.Fields in
the plan command flow to recognize Windows-style executable paths and preserve
backslashes instead of applying POSIX escape processing, while retaining POSIX
quoting and environment-variable expansion for applicable Unix values. Add a
focused Windows path case to the existing plan command tests in the test suite.
In `@internal/tui/session_test.go`:
- Around line 884-967: Add a cross-session resume regression test alongside
TestResumeDifferentSessionReportsPlanReloadError that writes a valid plan for
the destination session, invokes handleResumeCommand, and verifies
reloadPlanFromFile restores both the sticky m.plan panel and the shared
update_plan tool state. Assert restored items preserve Content, Status, and
Notes, while confirming the destination session becomes active.
In `@internal/tui/spec_mode.go`:
- Around line 38-48: Add regression tests for the spec-session transition around
createSpecDraftSession: cover its failure path and assert the existing plan mode
and plan state remain unchanged, then cover success and assert the new session
exits plan mode while clearing the plan panel and shared update_plan state.
Reuse the existing TUI test helpers and fixtures for constructing sessions and
simulating createSpecDraftSession outcomes.
In `@internal/tui/view.go`:
- Around line 321-325: Add a regression test for nextPermissionMode covering
agent.PermissionModePlan, and assert that passing Plan mode returns
agent.PermissionModePlan unchanged, preserving the read-only boundary during
Shift+Tab toggling.
---
Duplicate comments:
In `@internal/planmode/read_windows.go`:
- Around line 185-204: Verify that STATUS_NO_SUCH_FILE is converted by
RtlNtStatusToDosError to ERROR_FILE_NOT_FOUND and remains recognized by
os.IsNotExist in the ReadPlan missing-plan path. If not, update
mapWindowsOpenErr to map windows.STATUS_NO_SUCH_FILE to os.ErrNotExist while
preserving the existing mappings.
---
Nitpick comments:
In `@internal/planmode/planmode_test.go`:
- Around line 441-459: Add a TestReadPlanFileRejectsNonRegularFile test
alongside TestReadPlanFileRoundtripPlainFile. Create the plan path as a
directory, call readPlanFile, assert an error is returned, and verify its
message contains "not a regular file"; retain the existing hermetic setup and
avoid relying on symlinks.
In `@internal/planmode/read_other.go`:
- Around line 10-36: Update openPlanUnderBase to fail closed on platforms
lacking no-follow primitives by returning an explicit unsupported-platform error
instead of performing the racy Lstat/Open fallback. Remove or revise the
fallback logic and comments so they no longer imply that in-root symlink
following is accepted.
In `@internal/tui/plan_command_test.go`:
- Around line 332-337: Canonicalize both cwd and the durable plan path before
the containment assertion in the update_plan test. Resolve the temporary
workspace path and the generated path using the existing filepath resolution
APIs, then compare the canonical paths while preserving the assertion that the
plan is outside the workspace; avoid relying on raw TempDir spellings.
- Around line 20-54: The isolatePlanConfig helper must stop creating and
deleting fixtures beneath os.UserHomeDir. Use t.TempDir() for the staging root,
set the platform-specific config environment variables to that root, and
redirect planmode.SetTempDirForTest to the same test workspace so StageForEditor
accepts it. Preserve the existing cleanup and cross-platform environment setup.
In `@internal/tui/spec_mode_test.go`:
- Around line 286-301: Update TestSpecCommandExitsPlanMode to assert that
next.permissionMode equals the recorded agent.PermissionModeAuto value, rather
than merely checking that it is not agent.PermissionModePlan. Keep the existing
test setup and failure reporting focused on verifying restoration of
permissionModeBeforePlan.
🪄 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: 98c3dd3c-34dd-4264-b8fe-2da6f4ac6678
📒 Files selected for processing (28)
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/planmode/read.gointernal/planmode/read_other.gointernal/planmode/read_unix.gointernal/planmode/read_windows.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
Map missing-file NTSTATUS to os.ErrNotExist, cap pathKey slug length under NAME_MAX, open final plan files O_NONBLOCK on Unix, clear the sticky plan panel when BTW reload fails, block /plan exit while a run is pending, and parse unquoted Windows $EDITOR paths without POSIX backslash escapes. Add regression coverage for long workspaces, destination resume reload, editor splitting, and successful /spec plan reset. Refs Gitlawb#854
|
Addressed the open CodeRabbit findings on Fixes
Validation
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
internal/tui/plan_command.go (1)
55-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated plan-exit path.
The
off/exitbranch and the bare toggle-off branch repeat the same three steps: them.pending || m.exitinggate,exitPlanMode, and the identical "Exited plan mode." transcript note. A future change to the exit rules must be applied in two places.Extract a
func (m model) exitPlanModeWithNotice() (model, bool)helper and call it from both branches.Also applies to: 101-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 55 - 69, Extract the shared plan-mode exit logic into model method exitPlanModeWithNotice() (model, bool), including the m.pending || m.exiting guard, exitPlanMode call, and identical exit transcript notice; return false when exit is blocked and true after exiting. Replace both the "off"/"exit" branch and bare toggle-off branch with this helper while preserving their existing return behavior.internal/tui/model.go (1)
1276-1294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRecord the session message only when the plan actually changed.
This handler runs on every successful
/plan open, including a look-and-quit with no edit. It always appends ausermessage that claims "I edited the plan file directly". That injects a false statement into the next turn's context and grows the session log on every open.Capture the plan items before launching the editor, or compare
formatPlanItems(items)against the pre-edit content, and skip the transcript note plus the session event when they match.🤖 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/model.go` around lines 1276 - 1294, The /plan open handler currently records an edit event even when the editor makes no changes. In reloadPlanFromFile, capture the plan state before launching the editor and compare it with the post-edit items (using formatPlanItems or equivalent); only build the “I edited…” content and call appendSessionEvent when the plans differ, while preserving normal return and error handling for unchanged plans.internal/planmode/read_unix.go (1)
83-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare errnos with
errors.Isinstead of==.
openatRetryandisNoFollowErrrely onunix.Openatreturning a baresyscall.Errno. That holds today. It breaks silently if the error is ever wrapped, and a brokenisNoFollowErrdegrades a hard symlink refusal into a generic error thatReadPlanreports asread plan file: ...rather than a refusal. Fail-closed behavior would still hold, but the message and the classification would change.♻️ 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 83 - 98, Import the standard errors package and update openatRetry and isNoFollowErr to use errors.Is when matching syscall.EINTR, syscall.ELOOP, and syscall.EMLINK, preserving the existing retry and no-follow classification behavior when errors are wrapped.internal/planmode/read_windows.go (1)
168-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe third branch is unreachable.
Line 172 already compares
erragainstwindows.STATUS_REPARSE_POINT_ENCOUNTERED. Line 179 type-asserts towindows.NTStatusand performs the same comparison.windows.STATUS_REPARSE_POINT_ENCOUNTEREDis itself anNTStatus, so the interface comparison on Line 172 already succeeds for exactly the values Line 179 matches. Delete the redundant branch, or replace both witherrors.Is.🤖 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_windows.go` around lines 168 - 183, The type-assertion branch in isWindowsSymlinkErr redundantly checks windows.STATUS_REPARSE_POINT_ENCOUNTERED after the direct comparison already handles it. Remove the `windows.NTStatus` assertion branch while preserving the nil, direct status, and mapped errno checks.internal/planmode/planmode.go (1)
83-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the substring match on the error text with a sentinel error.
Line 88 classifies the refusal with
strings.Contains(err.Error(), "is a symlink"). That couplesReadPlanto the exact wording oferrPlanSymlinkinread.go. Any rewording of that message silently changesReadPlanfrom returning the refusal to wrapping it asread plan file: ..., and no test would catch the drift.Define a sentinel and match with
errors.Is.♻️ Proposed refactor
In
internal/planmode/read.go:var errIsSymlink = errors.New("is a symlink") func errPlanSymlink(path string) error { return fmt.Errorf("plan file %s %w; refusing to read through it", path, errIsSymlink) }Then here:
- // Symlink refusals from the reader are already fully formed. - if strings.Contains(err.Error(), "is a symlink") { + // Symlink refusals from the reader are already fully formed. + if errors.Is(err, errIsSymlink) { return "", false, 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.go` around lines 83 - 92, Define an `errIsSymlink` sentinel in `errPlanSymlink` within `read.go`, wrap it when constructing symlink refusal errors, and update `ReadPlan` to use `errors.Is(err, errIsSymlink)` instead of matching `"is a symlink"` in `err.Error()`. Preserve the existing return behavior for symlink refusals and other read errors.internal/agent/loop.go (1)
3227-3248: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse an explicit plan-mode safety property.
lsp_navigateis the only production read/allow tool that starts a process;web_fetchandweb_searchare correctly classified as network tools. However,Registry.Registeraccepts plugin, MCP, and custom tools. A future tool can still be incorrectly classified as read/allow and bypass this denylist. Gate plan advertisement on an explicit no-process/no-network property and add regression tests for external-effect tools.🤖 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 - 3248, Update toolAdvertisedInPlan to require an explicit safety property indicating the tool performs neither process execution nor network access, rather than relying on the lsp_navigate name denylist and SideEffectRead/PermissionAllow alone. Preserve the existing permission and side-effect checks, define the property through the tool safety metadata, and add regression coverage for tools with process or network effects, including custom or registered tools.
🤖 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 4035-4042: The test hook command in the session-start setup must
successfully create the filesystem marker so the subsequent os.Stat assertion
can detect execution. Update the marker path and corresponding go mod init
arguments around the hook definitions and related assertion near the alternate
lines, using a file path such as marker-go.mod rather than a nonexistent parent
directory; preserve the audit-event assertion.
In `@internal/planmode/planmode_test.go`:
- Around line 495-525: Add a regression test named TestReadPlanFileRejectsFifo
alongside the existing readPlanFile tests to create a FIFO, invoke readPlanFile
asynchronously, and assert it promptly returns a “not a regular file” error. Use
Unix-only handling or skip Windows, include a timeout to detect blocking when
O_NONBLOCK is absent, and add only the required imports.
In `@internal/planmode/planmode.go`:
- Around line 105-157: Make WritePlan handle-bound like ReadPlan by reusing the
platform-specific component-walk helpers to obtain a directory handle for
filepath.Dir(path), rather than relying on ensurePlanPathContained, MkdirAll,
Lstat, and pathname-based rename checks. Create the temporary file and replace
the plan through handle-relative, symlink-resistant operations on Unix and
Windows. Apply the same treatment to CommitStagedEdit’s staged-file access so
reads and writes remain traversal-resistant.
In `@internal/planmode/read_windows.go`:
- Around line 86-92: Update openWindowsBaseDir to detect UNC absBase values and
construct the NT object path using the \??\UNC\server\share form, while
preserving the existing \??\ drive-letter path handling. Add the strings import
as needed and introduce a unit test for NT path construction that covers the UNC
branch without requiring a real network share.
In `@internal/tui/btw.go`:
- Around line 206-223: The BTW restore flow around parent.reloadPlanFromFile
must handle the missing-file result (ok == false, err == nil) explicitly: define
the intended ReadPlan contract and reset or otherwise synchronize the parent
panel and shared update_plan state instead of skipping updates. Update leaveBTW
accordingly and add a regression test covering a missing plan file, verifying
the restored panel and tool state remain consistent.
In `@internal/tui/plan_command_test.go`:
- Around line 405-410: Update the containment assertion around
planmode.PlanFilePath to canonicalize both path and cwd with
filepath.EvalSymlinks before comparing them, handling any resolution errors
appropriately. Use the canonical values for the workspace-prefix and equality
checks while preserving the existing .zero absence assertion.
---
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 3227-3248: Update toolAdvertisedInPlan to require an explicit
safety property indicating the tool performs neither process execution nor
network access, rather than relying on the lsp_navigate name denylist and
SideEffectRead/PermissionAllow alone. Preserve the existing permission and
side-effect checks, define the property through the tool safety metadata, and
add regression coverage for tools with process or network effects, including
custom or registered tools.
In `@internal/planmode/planmode.go`:
- Around line 83-92: Define an `errIsSymlink` sentinel in `errPlanSymlink`
within `read.go`, wrap it when constructing symlink refusal errors, and update
`ReadPlan` to use `errors.Is(err, errIsSymlink)` instead of matching `"is a
symlink"` in `err.Error()`. Preserve the existing return behavior for symlink
refusals and other read errors.
In `@internal/planmode/read_unix.go`:
- Around line 83-98: Import the standard errors package and update openatRetry
and isNoFollowErr to use errors.Is when matching syscall.EINTR, syscall.ELOOP,
and syscall.EMLINK, preserving the existing retry and no-follow classification
behavior when errors are wrapped.
In `@internal/planmode/read_windows.go`:
- Around line 168-183: The type-assertion branch in isWindowsSymlinkErr
redundantly checks windows.STATUS_REPARSE_POINT_ENCOUNTERED after the direct
comparison already handles it. Remove the `windows.NTStatus` assertion branch
while preserving the nil, direct status, and mapped errno checks.
In `@internal/tui/model.go`:
- Around line 1276-1294: The /plan open handler currently records an edit event
even when the editor makes no changes. In reloadPlanFromFile, capture the plan
state before launching the editor and compare it with the post-edit items (using
formatPlanItems or equivalent); only build the “I edited…” content and call
appendSessionEvent when the plans differ, while preserving normal return and
error handling for unchanged plans.
In `@internal/tui/plan_command.go`:
- Around line 55-69: Extract the shared plan-mode exit logic into model method
exitPlanModeWithNotice() (model, bool), including the m.pending || m.exiting
guard, exitPlanMode call, and identical exit transcript notice; return false
when exit is blocked and true after exiting. Replace both the "off"/"exit"
branch and bare toggle-off branch with this helper while preserving their
existing return behavior.
🪄 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: 6d7e1a6a-3914-4c4a-9288-f9967e999bcd
📒 Files selected for processing (28)
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/planmode/read.gointernal/planmode/read_other.gointernal/planmode/read_unix.gointernal/planmode/read_windows.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
Bind WritePlan create/rename to a rooted no-follow walk, fix UNC NT paths, and tighten plan-mode hook/BTW/path regression tests so panel and tool stay consistent.
CodeRabbit follow-up (dc76c2f)Closed all 6 open findings from the last review on
Verification: Skipped none. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/planmode/write_windows.go (1)
125-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
==error comparisons witherrors.Is.These sites compare errors by identity:
err != os.ErrNotExist,err == os.ErrNotExist,err == syscall.EISDIR,err == syscall.EEXIST, andst == windows.STATUS_OBJECT_NAME_COLLISION. They only match whenmapWindowsOpenErrreturns the exact sentinel unwrapped. If that helper ever wraps withfmt.Errorf("%w", ...)or returns a*PathError, the comparison silently returns false. ThenrefuseSymlinkAtWindowssurfaces a directory as a hard failure, andensureDirNoFollowWindowstreats a benign collision as fatal. The write path fails closed but for the wrong reason, and the error text will not point at the cause.♻️ Proposed change
- if err != os.ErrNotExist && !os.IsNotExist(err) { + if !errors.Is(err, os.ErrNotExist) { return 0, err } @@ - if err == os.ErrNotExist || os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return nil } @@ - if err == syscall.EISDIR { + if errors.Is(err, syscall.EISDIR) { return nil } @@ - if err == syscall.EEXIST { + if errors.Is(err, syscall.EEXIST) { return true } - if st, ok := err.(windows.NTStatus); ok && st == windows.STATUS_OBJECT_NAME_COLLISION { + var st windows.NTStatus + if errors.As(err, &st) && st == windows.STATUS_OBJECT_NAME_COLLISION { return true }Add
"errors"to the import block. The sameerr.(windows.NTStatus)type assertion at line 271 has the identical fragility.Also applies to: 213-213, 219-219, 329-332
🤖 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/write_windows.go` at line 125, Update the error handling in refuseSymlinkAtWindows and ensureDirNoFollowWindows to use errors.Is for os.ErrNotExist, syscall.EISDIR, and syscall.EEXIST comparisons, adding the errors import. Replace the fragile windows.NTStatus identity assertion near the status-handling logic with errors.As so wrapped NT status errors are recognized, while preserving the existing outcomes for missing paths, directories, and benign collisions.internal/planmode/write_unix.go (1)
91-103: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo fsync on the Unix path, so the "durable write" claim does not hold.
write.godocuments an atomic durable write, andwrite_windows.gocallsfile.Sync()before the rename. The Unix path writes and closes without any fsync.renameatgives atomicity of the name change only. After a crash or power loss the plan file can exist with zero length or with stale content, because the data blocks and the parent directory entry were never flushed.Sync the file before the rename, and sync the parent directory after it.
🛡️ Proposed fix
if _, err := file.WriteString(content); err != nil { _ = file.Close() return fmt.Errorf("write plan file: %w", err) } + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } if err := file.Close(); err != nil { return fmt.Errorf("write plan file: %w", err) } if err := renameatRetry(dirfd, tmpName, dirfd, final); err != nil { return fmt.Errorf("replace plan file: %w", err) } written = true + // Flush the directory entry so the rename survives a crash. + _ = unix.Fsync(dirfd) return nil }🤖 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/write_unix.go` around lines 91 - 103, Update the Unix write flow around the file.WriteString and renameatRetry calls to sync the temporary file with file.Sync() before closing and renaming it, then sync the parent directory via dirfd after the rename succeeds. Preserve the existing error wrapping and ensure failures from either sync are returned instead of marking the write successful.internal/planmode/read_windows_test.go (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
strings.HasPrefixfor both checks.
hasPrefixduplicates the standard library and adds an unnecessary package-level helper. This also avoids future name collisions in the Windows-only package.🤖 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_windows_test.go` around lines 33 - 35, Remove the package-level hasPrefix helper and replace both of its call sites with strings.HasPrefix, adding the strings import if needed. Preserve the existing prefix-check behavior.
🤖 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/write_other.go`:
- Around line 11-14: Update the doc comment for writePlanUnderBase to remove the
inaccurate Root.MkdirAll symlink-following statement and describe the actual
per-component root.Mkdir creation behavior, while preserving the existing
fallback and rooted create/rename details.
In `@internal/planmode/write_windows.go`:
- Around line 259-269: Update the rename payload in the NtSetInformationFile
call to match windows.FileRenameInformation: set
fileRenameInformation.ReplaceIfExists to the legacy value 1 and remove
windows.FILE_RENAME_POSIX_SEMANTICS. If POSIX semantics are required instead,
use information class 65 (FileRenameInformationEx) with its matching flag
layout, defining the class locally because x/sys v0.47.0 does not expose it.
---
Nitpick comments:
In `@internal/planmode/read_windows_test.go`:
- Around line 33-35: Remove the package-level hasPrefix helper and replace both
of its call sites with strings.HasPrefix, adding the strings import if needed.
Preserve the existing prefix-check behavior.
In `@internal/planmode/write_unix.go`:
- Around line 91-103: Update the Unix write flow around the file.WriteString and
renameatRetry calls to sync the temporary file with file.Sync() before closing
and renaming it, then sync the parent directory via dirfd after the rename
succeeds. Preserve the existing error wrapping and ensure failures from either
sync are returned instead of marking the write successful.
In `@internal/planmode/write_windows.go`:
- Line 125: Update the error handling in refuseSymlinkAtWindows and
ensureDirNoFollowWindows to use errors.Is for os.ErrNotExist, syscall.EISDIR,
and syscall.EEXIST comparisons, adding the errors import. Replace the fragile
windows.NTStatus identity assertion near the status-handling logic with
errors.As so wrapped NT status errors are recognized, while preserving the
existing outcomes for missing paths, directories, and benign collisions.
🪄 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: 534e5305-0806-4e9b-a9f0-6809fa6d10fe
📒 Files selected for processing (14)
internal/agent/loop_test.gointernal/planmode/fifo_other_test.gointernal/planmode/fifo_unix_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/planmode/read_windows.gointernal/planmode/read_windows_test.gointernal/planmode/write.gointernal/planmode/write_other.gointernal/planmode/write_unix.gointernal/planmode/write_windows.gointernal/tui/btw.gointernal/tui/btw_test.gointernal/tui/plan_command_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tui/btw.go
- internal/planmode/read_windows.go
- internal/agent/loop_test.go
- internal/tui/plan_command_test.go
- internal/planmode/planmode.go
- internal/planmode/planmode_test.go
| var dummy fileRenameInformation | ||
| bufferSize := int(unsafe.Offsetof(dummy.FileName)) + fileNameLen | ||
| buffer := make([]byte, bufferSize) | ||
| info := (*fileRenameInformation)(unsafe.Pointer(&buffer[0])) | ||
| info.ReplaceIfExists = windows.FILE_RENAME_REPLACE_IF_EXISTS | windows.FILE_RENAME_POSIX_SEMANTICS | ||
| info.RootDirectory = newdirfd | ||
| info.FileNameLength = uint32(fileNameLen) | ||
| copy((*[windows.MAX_LONG_PATH]uint16)(unsafe.Pointer(&info.FileName[0]))[:fileNameLen/2:fileNameLen/2], newNameUTF16) | ||
|
|
||
| var iosb windows.IO_STATUS_BLOCK | ||
| err = windows.NtSetInformationFile(h, &iosb, &buffer[0], uint32(bufferSize), windows.FileRenameInformation) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f '^go\.(mod|sum)$' -x rg -n 'golang.org/x/sys' {}
rg -nP 'FILE_RENAME_POSIX_SEMANTICS|FileRenameInformationEx|FILE_RENAME_REPLACE_IF_EXISTS' --glob '**/golang.org/x/sys/**'Repository: Gitlawb/zero
Length of output: 338
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target outline ---'
ast-grep outline internal/planmode/write_windows.go
printf '%s\n' '--- target context ---'
sed -n '210,285p' internal/planmode/write_windows.go
printf '%s\n' '--- related symbols in repository ---'
rg -n -C 3 'fileRenameInformation|FILE_RENAME_POSIX_SEMANTICS|FILE_RENAME_REPLACE_IF_EXISTS|FileRenameInformationEx|FileRenameInformation|NtSetInformationFile' .
printf '%s\n' '--- module metadata ---'
sed -n '1,90p' go.modRepository: Gitlawb/zero
Length of output: 8834
🌐 Web query:
golang.org/x/sys v0.47.0 windows FILE_RENAME_POSIX_SEMANTICS FileRenameInformationEx source
💡 Result:
In the context of the Go golang.org/x/sys/windows package (and related internal syscall packages), FILE_RENAME_POSIX_SEMANTICS is a flag constant used with the FILE_RENAME_INFORMATION_EX structure when performing file rename operations via the NtSetInformationFile system call [1][2][3]. Technical Details and Usage: 1. Functionality: When combined with FILE_RENAME_REPLACE_IF_EXISTS, the FILE_RENAME_POSIX_SEMANTICS flag (value 0x00000002) instructs the Windows kernel to allow a rename operation to succeed even if the destination file exists and has open handles [4][5]. Under these semantics, existing handles to the replaced file remain valid for operations such as reading or writing, while any subsequent attempts to open that path will access the newly renamed file [4][5]. 2. Implementation in Go: Go uses this in low-level file operations (such as in atomic file writers) to achieve atomic-like replacement behavior on Windows, which traditionally struggles with file renames when files are in use [1][3]. The standard approach in Go code typically involves: - Preparing a FILE_RENAME_INFORMATION_EX structure [1][3]. - Setting Flags to FILE_RENAME_REPLACE_IF_EXISTS | FILE_RENAME_POSIX_SEMANTICS [1][3]. - Invoking NtSetInformationFile with the FileRenameInformationEx information class [1][3]. - Implementing a fallback mechanism to the legacy FILE_RENAME_INFORMATION class for older filesystems or Windows versions that do not support the extended information class [1][3]. 3. Context in golang.org/x/sys: While golang.org/x/sys provides the necessary low-level constants and structures for NtSetInformationFile [3][6][7], usage often involves the internal/syscall/windows package or direct interaction with ntdll functions via syscall.SyscallN [8][1][3]. The flag is part of the broader Windows driver-level API exposed for fine-grained file system control [5][6].
Citations:
- 1: https://go.dev/src/internal/syscall/windows/at_windows.go?m=text
- 2: https://go.dev/src/internal/syscall/windows/types_windows.go
- 3: Add retry for os.rename in atomicFileWriter moby/sys#219
- 4: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/4217551b-d2c0-42cb-9dc1-69a716cf6d0c
- 5: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
- 6: https://groups.google.com/g/golang-codereviews/c/z-3fMktzFV4
- 7: https://groups.google.com/g/golang-checkins/c/-_b0r0a4AWA
- 8: golang/sys@v0.30.0...v0.47.0
🏁 Script executed:
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/golang/sys/v0.47.0/windows'
printf '%s\n' '--- x/sys v0.47.0 rename symbols ---'
for file in zerrors_windows.go syscall_windows.go types_windows.go; do
printf '%s\n' "--- $file ---"
curl -fsSL "$base/$file" | rg -n -C 3 'FILE_RENAME_(REPLACE_IF_EXISTS|POSIX_SEMANTICS)|FileRenameInformation(Ex)?|NtSetInformationFile' || true
done
printf '%s\n' '--- Go standard-library reference implementation ---'
curl -fsSL 'https://raw.githubusercontent.com/golang/go/master/src/internal/syscall/windows/at_windows.go' |
rg -n -C 12 'FILE_RENAME|FileRenameInformation|POSIX_SEMANTICS' || true
printf '%s\n' '--- Microsoft structure documentation excerpt ---'
curl -fsSL 'https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information' |
rg -n -C 2 'ReplaceIfExists|FILE_RENAME_INFORMATION|BOOLEAN' | head -80 || trueRepository: Gitlawb/zero
Length of output: 11068
🌐 Web query:
Microsoft FILE_RENAME_INFORMATION_EX ReplaceIfExists BOOLEAN FileRenameInformationEx information class layout
💡 Result:
The FileRenameInformationEx information class uses a structure that incorporates flags to control rename operations, replacing the simple BOOLEAN ReplaceIfExists field found in the standard FileRenameInformation class [1][2]. In modern Windows development (starting with Windows 10, version 1607/RS1), the FILE_RENAME_INFORMATION structure is defined with a union to maintain backward compatibility, allowing it to handle both the legacy BOOLEAN ReplaceIfExists field and the newer ULONG Flags field used by FileRenameInformationEx [1][3]. The structure layout for FileRenameInformationEx is effectively represented as [1][4][5]: typedef struct _FILE_RENAME_INFORMATION_EX { ULONG Flags; HANDLE RootDirectory; ULONG FileNameLength; WCHAR FileName[1]; } FILE_RENAME_INFORMATION_EX, *PFILE_RENAME_INFORMATION_EX; When using FileRenameInformationEx, the Flags field replaces the BOOLEAN ReplaceIfExists field [1]. The available flags include [1][6]: FILE_RENAME_REPLACE_IF_EXISTS (0x00000001): If set, behaves like the legacy ReplaceIfExists = TRUE; if not set, the operation fails if the target exists [1][6]. FILE_RENAME_POSIX_SEMANTICS (0x00000002): When used with FILE_RENAME_REPLACE_IF_EXISTS, allows replacing a file even if handles are open to it [1][6]. FILE_RENAME_SUPPRESS_PIN_STATE_INHERITANCE (0x00000004): Suppresses inheritance of specific pinned attributes [1][6]. FILE_RENAME_SUPPRESS_STORAGE_RESERVE_INHERITANCE (0x00000008): Suppresses inheritance of storage reserve attributes [1]. FILE_RENAME_FORCE_RESIZE_SR (0x00000180): Forces resize of storage reserves [6]. For programming purposes, Microsoft documentation often uses a union named DUMMYUNIONNAME containing both ReplaceIfExists (BOOLEAN) and Flags (ULONG) to support both information classes within the same structure definition [1][3].
Citations:
- 1: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
- 2: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ne-wdm-_file_information_class
- 3: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info
- 4: https://github.com/MicrosoftDocs/windows-driver-docs-ddi/blob/staging/wdk-ddi-src/content/ntifs/ns-ntifs-_file_rename_information.md
- 5: https://ntdoc.m417z.com/file_rename_information_ex
- 6: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/4217551b-d2c0-42cb-9dc1-69a716cf6d0c
Match the rename payload to the information class.
FileRenameInformation interprets the first field as the legacy BOOLEAN ReplaceIfExists, so FILE_RENAME_POSIX_SEMANTICS is not applied. Use information class 65 (FileRenameInformationEx) with the flag layout when POSIX semantics are required; golang.org/x/sys v0.47.0 does not expose that class constant. Otherwise, set ReplaceIfExists to 1 and remove the POSIX flag.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 264-264: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(fileNameLen)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 268-268: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(bufferSize)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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/write_windows.go` around lines 259 - 269, Update the rename
payload in the NtSetInformationFile call to match windows.FileRenameInformation:
set fileRenameInformation.ReplaceIfExists to the legacy value 1 and remove
windows.FILE_RENAME_POSIX_SEMANTICS. If POSIX semantics are required instead,
use information class 65 (FileRenameInformationEx) with its matching flag
layout, defining the class locally because x/sys v0.47.0 does not expose it.
…n for intermediate symlinks Refs Gitlawb#854
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/planmode/planmode.go (1)
118-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op symlink branch in
WritePlan.Both branches return
"", errunchanged. The comment implies special handling that does not exist.ReadPlanneeds its equivalent branch because the fallback wraps the error;WritePlandoes not wrap.♻️ Proposed simplification
if err := writePlanFile(base, path, body); err != nil { - // Symlink refusals from the writer are already fully formed. - if strings.Contains(err.Error(), "is a symlink") { - return "", err - } return "", 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.go` around lines 118 - 124, Remove the redundant symlink-specific conditional in WritePlan and return the writePlanFile error directly when it fails. Preserve the existing behavior of returning an empty plan path alongside the unchanged error, without retaining the misleading comment or strings.Contains check.internal/planmode/planmode_test.go (1)
665-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
CommitStagedEditround-trip test.The staging half is covered here, but no test in this package calls
CommitStagedEdit. That function is the write-back boundary: it reads the staged file by pathname and pushes the content into durable storage. A regression that drops the write-back, or that stops trimming/normalising the body, would pass this suite.Add a test that stages a plan, rewrites the staged file, calls
CommitStagedEdit, and assertsReadPlanreturns the edited body. Add a failure case for a missing staged path.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."
🤖 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 665 - 702, Add a round-trip test alongside TestStageForEditorWritesUnderConfigStagingDir that writes a plan, calls StageForEditor, rewrites the staged file, invokes CommitStagedEdit, and verifies ReadPlan returns the edited, normalized body. Also assert CommitStagedEdit returns an error for a missing staged pathname, covering the write-back failure path.Source: Coding guidelines
internal/planmode/write_unix.go (1)
91-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnix writes skip
Sync; Windows writes do not.
write_windows.gocallsfile.Sync()before the rename so a crash cannot leave a partial durable plan. The Unix path closes and renames without a flush. After a crash the rename can be visible while the data blocks are not, which leaves an empty or truncated plan file. The atomic-replace guarantee is only as strong as the weaker platform.🛡️ Proposed fix
if _, err := file.WriteString(content); err != nil { _ = file.Close() return fmt.Errorf("write plan file: %w", err) } + // Flush before the rename: an unflushed rename can expose the new name + // with no data after a crash. + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("write plan file: %w", err) + } if err := file.Close(); err != nil { return fmt.Errorf("write plan file: %w", err) }As per coding guidelines: "Write complete temporary files and atomically replace destinations so concurrent readers never observe partial writes".
🤖 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/write_unix.go` around lines 91 - 101, Update the Unix write path around the temporary file’s WriteString and Close operations to call file.Sync() successfully before closing and renaming it, propagating any sync error with the existing plan-file error context and preserving cleanup. Keep renameatRetry unchanged, ensuring the temporary file is fully flushed before atomic replacement.Source: Coding guidelines
internal/tui/plan_command_test.go (1)
468-510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a failure-path case for the plan reload error.
reloadPlanFromFilenow returns aReadPlanerror, andinternal/tui/model.goLine 1261 renders "plan reload error: …". No test exercises that branch. Add a case whereReadPlanfails (for example, replace the plan file with a symlink, or make the plan directory unreadable) and assert the transcript reports the reload error.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths".
🤖 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 468 - 510, Add a failure-path test alongside TestPlanEditorFinishedMsgReloadsPanelAndConfirms that makes ReadPlan fail after setup, invokes Update with planEditorFinishedMsg, and asserts the resulting transcript contains the “plan reload error:” message. Reuse the existing model/session setup and ensure the test targets the reload path rather than calling reloadPlanFromFile directly.Source: Coding guidelines
internal/tui/plan_command.go (1)
252-265: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueWindows values that do not start with a drive or UNC path still go through POSIX escaping.
isUnquotedWindowsEditorPathonly matches when the whole value begins withC:\or\\. On Windows, a value such asnotepad -c C:\tmp\vimrcfails that test, soshell.Fieldsruns and removes the backslashes in the argument. Consider usingwindowsEditorFieldsfor every value whengoos == "windows"and the value contains a backslash, or documenting the quoting requirement in the error text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 252 - 265, Update splitEditorCommandFor so Windows commands containing backslashes use windowsEditorFields rather than shell.Fields, including values that do not begin with a drive or UNC path; retain the existing POSIX parsing path for non-Windows commands and Windows values without backslashes.
🤖 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_other.go`:
- Around line 25-35: Update the fallback file-opening flow around root.Lstat and
root.Open to return an unsupported-platform error instead of opening the
validated path. Preserve the existing validation behavior where applicable, but
fail closed on non-Unix/non-Windows platforms until os.Root.Open safely rejects
final-component symlinks and the walker rejects intermediate symlinks.
In `@internal/tui/model.go`:
- Around line 1276-1294: Update the reloadPlanFromFile editor flow around
formatPlanItems and appendSessionEvent to capture the plan state before editing
and compare it with the post-edit items. Only construct the “I edited the plan”
content, append the transcript note, and call appendSessionEvent when the plan
actually changed; otherwise return without recording a session event.
---
Nitpick comments:
In `@internal/planmode/planmode_test.go`:
- Around line 665-702: Add a round-trip test alongside
TestStageForEditorWritesUnderConfigStagingDir that writes a plan, calls
StageForEditor, rewrites the staged file, invokes CommitStagedEdit, and verifies
ReadPlan returns the edited, normalized body. Also assert CommitStagedEdit
returns an error for a missing staged pathname, covering the write-back failure
path.
In `@internal/planmode/planmode.go`:
- Around line 118-124: Remove the redundant symlink-specific conditional in
WritePlan and return the writePlanFile error directly when it fails. Preserve
the existing behavior of returning an empty plan path alongside the unchanged
error, without retaining the misleading comment or strings.Contains check.
In `@internal/planmode/write_unix.go`:
- Around line 91-101: Update the Unix write path around the temporary file’s
WriteString and Close operations to call file.Sync() successfully before closing
and renaming it, propagating any sync error with the existing plan-file error
context and preserving cleanup. Keep renameatRetry unchanged, ensuring the
temporary file is fully flushed before atomic replacement.
In `@internal/tui/plan_command_test.go`:
- Around line 468-510: Add a failure-path test alongside
TestPlanEditorFinishedMsgReloadsPanelAndConfirms that makes ReadPlan fail after
setup, invokes Update with planEditorFinishedMsg, and asserts the resulting
transcript contains the “plan reload error:” message. Reuse the existing
model/session setup and ensure the test targets the reload path rather than
calling reloadPlanFromFile directly.
In `@internal/tui/plan_command.go`:
- Around line 252-265: Update splitEditorCommandFor so Windows commands
containing backslashes use windowsEditorFields rather than shell.Fields,
including values that do not begin with a drive or UNC path; retain the existing
POSIX parsing path for non-Windows commands and Windows values without
backslashes.
🪄 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: 9ab23a70-8969-4da3-9157-24076aeb1414
📒 Files selected for processing (35)
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/fifo_other_test.gointernal/planmode/fifo_unix_test.gointernal/planmode/planmode.gointernal/planmode/planmode_test.gointernal/planmode/read.gointernal/planmode/read_other.gointernal/planmode/read_unix.gointernal/planmode/read_windows.gointernal/planmode/read_windows_test.gointernal/planmode/write.gointernal/planmode/write_other.gointernal/planmode/write_unix.gointernal/planmode/write_windows.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
| info, err := root.Lstat(rel) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if info.Mode()&os.ModeSymlink != 0 { | ||
| return nil, errPlanSymlink(displayPath) | ||
| } | ||
| if !info.Mode().IsRegular() { | ||
| return nil, fmt.Errorf("plan file %s is not a regular file", displayPath) | ||
| } | ||
| return root.Open(rel) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
git ls-files | rg '(^|/)go\.mod$|(^|/)read_other\.go$|(^|/)root\.go$|(^|/)os\.go$|(^|/)fs\.go$|(^|/)planmode/'
echo
echo "== read_other.go =="
fd -a 'read_other\.go$' . | while read -r f; do
echo "--- $f"
wc -l "$f"
sed -n '1,140p' "$f"
done
echo
echo "== Search Root/Open/Lstat definitions/usages =="
rg -n "type Root|func .*Root|func \(.*Root\)|Lstat|Open\(|ModeSymlink|IsRegular|errPlanSymlink|PlanSymlink" -g '*.go' .Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ReadPlan call sites and ReadPlan implementation =="
rg -n "ReadPlan|openPlanUnderBase|readPlan|open\(|OpenRoot|errPlanSymlink" internal/planmode -g '*.go'
echo
echo "== planmode/read.go =="
sed -n '1,90p' internal/planmode/read.go
echo
echo "== planmode/read_unix.go =="
sed -n '1,110p' internal/planmode/read_unix.go
echo
echo "== planmode/read_windows.go =="
sed -n '1,110p' internal/planmode/read_windows.go
echo
echo "== planmode tests relevant to fallback =="
sed -n '400,490p' internal/planmode/planmode_test.goRepository: Gitlawb/zero
Length of output: 20374
Fail closed on fallback platforms.
root.Lstat(rel) and root.Open(rel) still leave a time-of-check to time-of-use gap on non-Unix, non-Windows targets: a validated regular file can be replaced with an in-root symlink before root.Open executes. The current comment admits the race, but the implementation still returns the open file. Replace this fallback with an unsupported-platform error until os.Root.Open cannot follow final-component symlinks and the walker refuses intermediate components.
🤖 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_other.go` around lines 25 - 35, Update the fallback
file-opening flow around root.Lstat and root.Open to return an
unsupported-platform error instead of opening the validated path. Preserve the
existing validation behavior where applicable, but fail closed on
non-Unix/non-Windows platforms until os.Root.Open safely rejects final-component
symlinks and the walker rejects intermediate symlinks.
Source: Coding guidelines
| // SetPlan (inside reloadPlanFromFile) only changes the update_plan | ||
| // tool's in-memory state; the model has no way to observe that on its | ||
| // own. Record it as a session event too, so a user-authored edit | ||
| // actually reaches the next turn's context — whether that turn is | ||
| // more planning or, after /plan off, the implementation run the | ||
| // feature is supposed to drive. | ||
| content := "I edited the plan file directly and cleared the plan." | ||
| if plan := formatPlanItems(items); plan != "" { | ||
| content = "I edited the plan file directly. Updated plan:\n\n" + plan | ||
| } | ||
| var err error | ||
| m, err = m.appendSessionEvent(sessions.EventMessage, map[string]any{ | ||
| "role": "user", | ||
| "content": content, | ||
| }) | ||
| if err != nil { | ||
| m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "session record error: " + err.Error()}) | ||
| } | ||
| return m, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record the session message only when the plan file actually changed.
The handler appends a user message on every successful editor exit. If the user opens the plan with /plan open, makes no change, and quits the editor, the session still receives "I edited the plan file directly. Updated plan: …". The next turn then sees a user statement that is false, and repeated opens duplicate the whole plan body in the session log.
Capture the plan content before the editor runs (or compare items against the pre-edit CurrentPlan()), and skip the transcript note plus the session event when nothing changed.
🤖 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/model.go` around lines 1276 - 1294, Update the
reloadPlanFromFile editor flow around formatPlanItems and appendSessionEvent to
capture the plan state before editing and compare it with the post-edit items.
Only construct the “I edited the plan” content, append the transcript note, and
call appendSessionEvent when the plan actually changed; otherwise return without
recording a session event.
2 participants
Summary
/plancommand and TUI wiring forPermissionModePlan(see the companion agent-side PR), including a command-palette entry, editor round-trip for the plan file, and status/notes preserved across editor exitexitPlanModeagainst clobbering an unrelated permission modebeforeToolpolicy vetoes while activeTest plan
go test ./internal/tui/... ./internal/planmode/...Summary by CodeRabbit
New Features
/plan open,/plan off, and/plan exitcommands.Bug Fixes