fix(acp): stage file edits for native review instead of writing twice#38198
fix(acp): stage file edits for native review instead of writing twice#38198anthony-furman wants to merge 15 commits into
Conversation
In-memory overlay that holds staged file content during an ACP turn so add/update writes can be sent to the client for native review instead of hitting disk immediately. (cherry picked from commit e8e0244)
Gate review staging on the client writeTextFile capability, flush staged edits through ACP, and wrap FSUtil so tool writes land in the overlay instead of disk while review mode is active. (cherry picked from commit 8573277)
Wire review mode into ACP session lifecycle, tool registry, and edit/write/ apply_patch tools so add/update changes reach the client review UI via writeTextFile. Delete/move still hit disk and show as tool_call diffs. (cherry picked from commit f71e111)
Upstream dropped @opencode-ai/core/util/log; migrate ACP review paths to Effect.logInfo/logError so typecheck passes after the rebase. (cherry picked from commit 4e58f07)
ToolRegistry.defaultLayer already provides Agent; align the registry test layer with skill.test and use an inline build agent to satisfy Effect types. (cherry picked from commit b6d4823)
… migration Re-add ReviewFs to ToolRegistry.node and createRoutes so tool writes stage in memory again. Remove misplaced ReviewFs from AppRuntime, which ACP does not use. (cherry picked from commit 13fb2e6)
Verify ToolRegistry.node with ReviewFs stages edits through the same layer wiring used by the HTTP server after the LayerNode migration. (cherry picked from commit 44506c9)
Adapt ReviewFs.node and the HTTP review regression test to the new LayerNode.make object form and compile() after upstream rebase. (cherry picked from commit 2b32aaf)
Replace removed FSUtil.defaultLayer usage with compile-based ReviewFs nodes and global FSUtil replacement in the HTTP app graph so ACP review staging keeps working on the current dev stack. (cherry picked from commit 1c91651)
|
would be great if this fix is merged |
flushPendingWrites runs twice per turn (once on tool completion, once at end-of-turn), and enqueueUnflushed() had no memory of what it had already drained. Since entries isn't cleared until end-of-turn, the second flush re-queued unchanged content and sent a duplicate fs/write_text_file for the same edit. Track flushed content per path so unchanged entries aren't resent, while genuine re-edits still are. Adds a regression test in review-overlay.test.ts reproducing the double-flush directly, plus an end-to-end ACP test asserting the client sees the tool_call_update before exactly one write, and that opencode never writes the file itself.
|
tested and noticed a duplication of the acp log for fs write, fixed |
|
I think a rejected
Would it make sense to acknowledge/remove each item only after the RPC succeeds, and make rejection fail the tool/turn, with a regression test for a rejecting client? |
|
Thank you so much for rescuing this, @anthony-furman ! I didn't have time to rebase it these last few weeks, so I'm really happy you picked it up. Great job finding and fixing the double-flush bug too, and thanks for keeping the original commit authorship 🚀 |
|
Good catch @3ugen . I made it fail the turn, since it's not really the tool that failed in this case, and I'm not sure how different ACP clients would handle resending an already completed tool call as failed. |
4 participants
Issue for this PR
Closes #38196
Related to #4240 (previously tracked this, closed when the ACP implementation was refactored in #29929, but the underlying gap was never actually fixed by that refactor)
Type of change
What does this PR do?
This rebases @PacoDw's closed PR #31392 (
feat/acp-review-at-end) onto currentdev. That PR was the most complete fix for #4240, but got auto-closed by the repo's inactivity-cleanup bot (>1 month old, <2 reactions) rather than on its merits, and had drifted fromdevsince the ACP-next promotion (#29929).The core problem: when an ACP client (e.g. Zed) advertises the
fs.writeTextFilecapability, OpenCode still writes edits to disk itself via its ownafs.writeWithDirs(...), andacp/permission.tsalso unconditionally fires awriteTextFileRPC to the client with the same final content once the edit permission is approved —initializenever storedclientCapabilities.fs.writeTextFileanywhere, so nothing could gate on it. Two effects: (1) the client's native "Keep/Reject" review UI never appears, since the file is already mutated by the time the client would show it, and (2) two separate write signals go out for what's logically one edit.The fix adds a
ReviewOverlay/ReviewFsstaging layer (packages/core/src/review-overlay.ts,packages/opencode/src/effect/review-fs-layer.ts):service.tsnow readsparams.clientCapabilities.fs.writeTextFileatinitializeand stores it (review-mode.ts).opencode acp),edit/write/apply_patchwrite into an in-memory overlay instead of real disk; the staged content is sent to the client via a singlewriteTextFilecall, and the client's own write/review decision becomes the only mutation.permission.tsskips its old unconditional mirror write in this mode (!isActive()guard) so the change isn't reflected twice.apply_patchstill go straight to disk (ACP has no delete/rename primitive), clearly called out in tool output.writeTextFileRPC.What I did on top of the original PR: cherry-picked the 9 substantive commits (skipping the branch's various
Merge branch 'dev'/chore: generatecommits, which only carried unrelated doc/i18n churn and caused hundreds of spurious conflicts on a straight rebase) onto currentdev, preserving original authorship. All 9 applied with zero conflicts.How did you verify your code works?
bun run typecheck(viatsgo --noEmit) — clean, no errors.bun test test/acp test/effect/review-fs-layer.test.ts test/effect/httpapi-review-fs.test.ts test/tool/review-mode-tools.test.ts— 151 pass, 0 fail (includes the newreview-mode,review-staging,review-fs-layersuites).bun test test/tool/edit.test.ts test/tool/write.test.ts test/tool/apply_patch.test.ts(legacy, no-capability path) — 71 pass, 0 fail, confirming no regression for clients withoutfs.writeTextFile.bun test test/review-overlay.test.ts(packages/core) — 5 pass, 0 fail.The original PR includes recordings of a fresh end-to-end session against Zed/Devin (linked below), and this rebase didn't touch the ACP wire format, only adapted the wiring to the current
devstructure. If deemed necessary, I would be willing to run it again and update the PR.Screenshots / recordings
See the original PR for recordings against Zed and Devin: #31392
Checklist