Add Go M0 write tools#48
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds write-capable tools ( ChangesWrite-capable and planning tools
Sequence Diagram(s)sequenceDiagram
participant Client
participant Registry
participant WriteFileTool
participant FS
Client->>Registry: RunWithOptions(write_file, args, PermissionGranted=true)
Registry->>WriteFileTool: Run(ctx, args)
WriteFileTool->>WriteFileTool: validate args, resolve path
WriteFileTool->>FS: ensure parent dirs (MkdirAll)
WriteFileTool->>FS: write file (WriteFile)
WriteFileTool-->>Registry: okResult
Registry-->>Client: okResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/tools/plan_tool_test.go (1)
9-58: ⚡ Quick winAdd a direct test for
ClearPlan()state reset behavior.The suite validates store/format and status errors, but not reset semantics on this stateful tool.
Suggested test addition
func TestUpdatePlanToolRejectsInvalidStatus(t *testing.T) { @@ } + +func TestUpdatePlanToolClearPlanResetsState(t *testing.T) { + tool := NewUpdatePlanTool() + _ = tool.Run(context.Background(), map[string]any{ + "plan": []any{ + map[string]any{"id": "1", "content": "Step", "status": "pending"}, + }, + }) + + tool.ClearPlan() + if got := tool.CurrentPlan(); len(got) != 0 { + t.Fatalf("expected empty plan after ClearPlan, got %d items", len(got)) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/plan_tool_test.go` around lines 9 - 58, Add a new unit test that verifies ClearPlan() resets internal state: create an UpdatePlanTool via NewUpdatePlanTool(), call Run(...) with a multi-item "plan" and assert CurrentPlan() length is >0, then call tool.ClearPlan() and assert tool.CurrentPlan() returns an empty slice (len == 0) and that subsequent calls to Run or formatting reflect the cleared state; reference NewUpdatePlanTool, Run, CurrentPlan, and ClearPlan in the test to locate the code to exercise and assert reset semantics.
🤖 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/tools/apply_patch.go`:
- Around line 60-63: The deferred os.Remove(patchPath) and the tempFile.Close()
call inside the WriteString error branch currently ignore returned errors;
update internal/tools/apply_patch.go so the defer explicitly handles the remove
error (e.g., defer func() { _ = os.Remove(patchPath) }()) and ensure
tempFile.Close() return value is handled or discarded (e.g., if err2 :=
tempFile.Close(); err2 != nil { /* log or ignore: _ = err2 */ }) in the
WriteString error branch where tempFile is closed before returning from the
errorResult("Error applying patch: "+err.Error()) path; keep the existing
errorResult return behavior but do not leave Close/Remove errors unhandled.
In `@internal/tools/args.go`:
- Around line 21-24: The current check mixes type and emptiness so when
allowEmpty==true a non-string still returns "must be a non-empty string"; change
the logic in the function in internal/tools/args.go that performs the text, ok
:= value.(string) assertion to first return a clear type error (e.g., "%s must
be a string") if ok is false, then only perform the emptiness check if
allowEmpty is false and return "%s must be a non-empty string" when text == "";
reference the variables text, ok, key and allowEmpty to locate and update the
two separate validation branches.
In `@internal/tools/workspace.go`:
- Around line 95-131: The current resolution code builds a safe-looking path
(using variables existing, missingSegments and filepath.EvalSymlinks) but can be
racy if missing parents are created or swapped to symlinks later; fix by adding
a second containment/symlink check immediately before any final write/apply
operation (i.e., in the callers that perform writes) that re-evaluates symlinks
(call filepath.EvalSymlinks on the final path or perform an os.Lstat walk
refusing ModeSymlink) and verifies the resulting path is still inside root (same
logic that checks relative against root), or alternatively change this resolver
to return a secure descriptor/walk that refuses symlinks for each path segment
so callers can safely write without a TOCTOU escape. //
---
Nitpick comments:
In `@internal/tools/plan_tool_test.go`:
- Around line 9-58: Add a new unit test that verifies ClearPlan() resets
internal state: create an UpdatePlanTool via NewUpdatePlanTool(), call Run(...)
with a multi-item "plan" and assert CurrentPlan() length is >0, then call
tool.ClearPlan() and assert tool.CurrentPlan() returns an empty slice (len == 0)
and that subsequent calls to Run or formatting reflect the cleared state;
reference NewUpdatePlanTool, Run, CurrentPlan, and ClearPlan in the test to
locate the code to exercise and assert reset semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 792d9e70-2d98-493a-a99d-752b34c7411e
📒 Files selected for processing (10)
internal/tools/apply_patch.gointernal/tools/args.gointernal/tools/edit_file.gointernal/tools/plan_tool_test.gointernal/tools/registry.gointernal/tools/types.gointernal/tools/update_plan.gointernal/tools/workspace.gointernal/tools/write_file.gointernal/tools/write_tools_test.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- Defense-in-depth on writes: every write tool resolves via
resolveWorkspaceTargetPath(internal/tools/workspace.go:72-126) for non-existent targets, and then runsrecheckWorkspaceWriteTarget(internal/tools/workspace.go:128-191) immediately before the actual write. The recheck walks the path segment by segment and refuses any segment whoseLstatshowsos.ModeSymlink, so a symlink parent is rejected at the latest possible moment beforeos.WriteFileruns. This is the TOCTOU mitigation CodeRabbit pushed for, and it's now in place acrosswrite_file,edit_file, andapply_patch. apply_patchdoes the recheck twice — once duringvalidatePatchPathsand again right beforeexec.CommandContext("git", "apply", ...)— so thegit applysubprocess only ever sees a path tree that has just been re-validated (internal/tools/apply_patch.go:69-72). The temp patch file isdefer-cleaned (internal/tools/apply_patch.go:55-56) and both theWriteStringandCloseerrors are checked.Registry.RunWithOptions(internal/tools/registry.go:36-54) is a clean three-way switch:PermissionAllowruns unconditionally,PermissionPromptrequiresoptions.PermissionGranted, anything else (incl.PermissionDeny) is refused. DefaultingRuntoRunWithOptions(ctx, name, args, RunOptions{})keeps existing call sites safe by default while letting the TUI / agent loop pass a grant flag.edit_fileenforces uniqueness by default and only allows non-unique edits behind an explicitreplace_all: true, with a clear error message that quotes the occurrence count. Emptynew_stringis allowed and works as a real deletion (tested).write_filedefaults to refusing overwrites and requiresoverwrite: trueto replace.stringArgWithEmpty(internal/tools/args.go:9-23) now separates the type check from the emptiness check, so a caller passingcontent: 42getscontent must be a stringinstead of the misleadingcontent must be a non-empty string— CodeRabbit's earlier finding, fixed.update_planuses a pointer receiver, returns a defensive copy fromCurrentPlan()(internal/tools/update_plan.go:63-65), and the test suite verifies that mutating the returned slice doesn't mutate internal state. The follow-up commitdb22668addsTestUpdatePlanToolClearPlanResetsStatefor the reset semantics CodeRabbit flagged.- Test coverage: prompt blocking without grant, prompt running with grant, create/overwrite/refuse, empty content, type errors, workspace rejection, symlink-parent rejection, edit uniqueness,
replace_all, fullgit applyround-trip, and symlink rejection inside patch paths. CI is green on all four matrices and CodeRabbit reports SUCCESS on the latest SHA.
Observations (non-blocking)
-
TOCTOU between the recheck and the actual write.
write_file.go:64-67andedit_file.go:78-83dorecheckWorkspaceWriteTarget→os.MkdirAll/os.WriteFilenon-atomically. The recheck catches an existing symlink, but if<root>/parentdoesn't exist yet, an attacker (or another process) who creates it as a symlink to e.g./etcbetween the recheck and theMkdirAllcall would have the write land outside the workspace. The canonical fix is the same write-temp-then-renamepattern PR #40'sZeroMcpPermissionStoreuses: write to<dir>/.zero.tmp.<rand>,fsync, thenrenameonto the target. On POSIX this is atomic; on Windows the rename fails if the destination exists (which is the desired behaviour). Adopting it forwrite_filewould close this window for free; the same pattern would letedit_filebuild the new contents in a temp file and rename over the original, which would also give you a clean "the file changed under us" detection. -
apply_patchparses patch paths line-by-line, not via a real parser.patchPathsFromLine(internal/tools/apply_patch.go:121-135) recognisesdiff --git,---, and+++prefixes and stripsa//b/. A unified diff with quoted filenames containing spaces (e.g.+++ "b/path with spaces.txt") or with backslash-escaped paths won't be parsed correctly and the workspace check would either miss paths or fail to recognise that a path is unsafe. A test using agit diffoutput on a path with spaces would cover this; if you want to keep the manual parser, escaping-aware splitting on the diff header would be the next step. (Note: this isn't a vulnerability per se —git applyitself enforces the workspace boundary viacwd— but the pre-flight check could be bypassed, which is more of a correctness issue for the diagnostic.) -
Hard dependency on
git applyis not documented.apply_patchshells out togit apply --whitespace=nowarn. If the runtime doesn't have git on PATH (or the user disables it), the tool errors withError applying patch: exec: "git": executable file not found in $PATH— informative but unexplained. A line in the tool description ("Requiresgiton PATH.") would set expectations; an explicitexec.LookPath("git")check inNewApplyPatchTool(or at the top ofRun) with a friendlier error would be even better. -
update_planallows duplicate IDs across plan items.parsePlanItems(internal/tools/update_plan.go:60-93) doesn't check foriduniqueness, so two items withid: "1"are accepted and rendered. If the TUI or downstream consumers key onid, this would silently break them. A uniqueness assertion (or a doc comment saying IDs may repeat) would prevent surprises. -
RunvsRunWithOptionsasymmetry is a footgun. The defaultRunnever grants permissions, so anyPermissionPrompttool fails by default; the caller has to know to switch toRunWithOptionswithPermissionGranted: true. This is the right safe default, but it's easy to write a TUI that callsRunfor everything and then is surprised when write tools never work. A short note in theRundoc comment (or a lint-style test that asserts everyRuncall site is paired with a permission check upstream) would help. -
recheckPatchWriteTargetsdoesn't validategit apply's actual output paths against the workspace. The pre-checks look at the--- a/xand+++ b/xheaders, butgit applycan create new files, modify existing ones, and delete files. A patch that includesrename from/rename toorcopy from/copy tolines isn't enumerated bypatchPathsFromLineand therefore won't be rechecked. Same caveat as #2 —git applyitself respectscommand.Dir = applyRoot, so the actual writes land inside the workspace — but the pre-flight check is incomplete for non-trivial patches.
No blockers. The TOCTOU window in #1 is the one I'd most want to see closed in a follow-up; the rest are correctness / documentation polish.
2 participants
Summary
Tests
Local blockers
Summary by CodeRabbit
New Features
Security & Reliability
Tests