fix(acp): stage file edits for native review instead of writing twice

packages/core/src/review-overlay.ts

@@ -0,0 +1,98 @@
+export * as ReviewOverlay from "./review-overlay"
+
+import { FSUtil } from "./fs-util"
+
+// In-memory staging area for ACP review mode. While enabled, file writes are
+// kept here instead of going to disk, and are later sent to the client so they
+// show up in the native review UI. `entries` holds the current staged content
+// (or a delete marker) per path; `pending` is the queue of writes still to send.
+export type Entry = { readonly content: string } | { readonly deleted: true }
+
+export type PendingWrite = { sessionID: string; path: string; content: string }
+
+let enabled = false
+let activeSession: string | undefined
+const entries = new Map<string, Entry>()
+const pending: PendingWrite[] = []
+// Tracks the exact content that was successfully delivered to the client per
+// path, so a later enqueueUnflushed() call in the same turn doesn't re-send a
+// write that already landed (entries isn't cleared until end of turn). Only
+// markFlushed() populates this — draining alone does not, so a write that was
+// drained but failed to send stays eligible for a retry.
+const flushedContent = new Map<string, string>()
+
+export function setEnabled(value: boolean) {
+  enabled = value
+}
+
+export function isEnabled() {
+  return enabled
+}
+
+export function setActiveSession(sessionID: string | undefined) {
+  activeSession = sessionID
+}
+
+export function stage(path: string, content: string) {
+  const key = FSUtil.resolve(path)
+  entries.set(key, { content })
+  if (activeSession) pending.push({ sessionID: activeSession, path: key, content })
+}
+
+export function markDeleted(path: string) {
+  entries.set(FSUtil.resolve(path), { deleted: true })
+}
+
+export function get(path: string) {
+  return entries.get(FSUtil.resolve(path))
+}
+
+export function has(path: string) {
+  return entries.has(FSUtil.resolve(path))
+}
+
+// Recover staged edits that were written before a session was active, so a late
+// flush still sends them. Skips paths already queued, delete markers, and
+// content that was successfully flushed to the client earlier in this turn.
+export function enqueueUnflushed(sessionID: string) {
+  const queued = new Set(pending.map((item) => item.path))
+  for (const [path, entry] of entries) {
+    if (!("content" in entry)) continue
+    if (queued.has(path)) continue
+    if (flushedContent.get(path) === entry.content) continue
+    pending.push({ sessionID, path, content: entry.content })
+    queued.add(path)
+  }
+}
+
+// Take the queued writes to send. Draining does not mark anything as flushed;
+// the caller must call markFlushed() for each write the client actually
+// accepted so a rejected write can still be retried.
+export function drainPendingWrites() {
+  const drained = [...pending]
+  pending.length = 0
+  return drained
+}
+
+// Record that a write was successfully delivered to the client. `path` is the
+// already-resolved key carried on the drained pending item.
+export function markFlushed(path: string, content: string) {
+  flushedContent.set(path, content)
+}
+
+// Drop staged state at the end of a turn but keep review mode enabled.
+export function clear() {
+  entries.clear()
+  pending.length = 0
+  flushedContent.clear()
+  activeSession = undefined
+}
+
+// Full reset, including disabling review mode. Used on shutdown and in tests.
+export function reset() {
+  enabled = false
+  activeSession = undefined
+  entries.clear()
+  pending.length = 0
+  flushedContent.clear()
+}

packages/core/test/review-overlay.test.ts

@@ -0,0 +1,109 @@
+import { describe, expect, it } from "bun:test"
+import { ReviewOverlay } from "../src/review-overlay"
+
+describe("ReviewOverlay", () => {
+  ReviewOverlay.reset()
+
+  it("stage and get", () => {
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.setActiveSession("sess-1")
+    ReviewOverlay.stage("/tmp/foo.ts", "hello")
+    expect(ReviewOverlay.get("/tmp/foo.ts")).toEqual({ content: "hello" })
+    expect(ReviewOverlay.has("/tmp/foo.ts")).toBe(true)
+  })
+
+  it("drainPendingWrites", () => {
+    ReviewOverlay.reset()
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.setActiveSession("sess-1")
+    ReviewOverlay.stage("/tmp/a.ts", "a")
+    ReviewOverlay.stage("/tmp/b.ts", "b")
+    const drained = ReviewOverlay.drainPendingWrites()
+    expect(drained).toEqual([
+      { sessionID: "sess-1", path: expect.stringContaining("a.ts"), content: "a" },
+      { sessionID: "sess-1", path: expect.stringContaining("b.ts"), content: "b" },
+    ])
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([])
+  })
+
+  it("markDeleted", () => {
+    ReviewOverlay.reset()
+    ReviewOverlay.stage("/tmp/gone.ts", "soon deleted")
+    ReviewOverlay.markDeleted("/tmp/gone.ts")
+    expect(ReviewOverlay.get("/tmp/gone.ts")).toEqual({ deleted: true })
+    expect(ReviewOverlay.has("/tmp/gone.ts")).toBe(true)
+  })
+
+  it("enqueueUnflushed recovers staged content without pending queue", () => {
+    ReviewOverlay.reset()
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.stage("/tmp/recover.ts", "staged")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([])
+    ReviewOverlay.enqueueUnflushed("sess-2")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([
+      { sessionID: "sess-2", path: expect.stringContaining("recover.ts"), content: "staged" },
+    ])
+  })
+
+  it("enqueueUnflushed does not resend content already flushed this turn", () => {
+    // Reproduces the double fs/write_text_file bug: flushPendingWrites runs
+    // once on tool completion and again at end-of-turn, both calling
+    // enqueueUnflushed. Once a write is confirmed via markFlushed, the second
+    // call must not resend unchanged content.
+    ReviewOverlay.reset()
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.setActiveSession("sess-3")
+    ReviewOverlay.stage("/tmp/once.ts", "v1")
+    const drained = ReviewOverlay.drainPendingWrites()
+    expect(drained).toEqual([{ sessionID: "sess-3", path: expect.stringContaining("once.ts"), content: "v1" }])
+    for (const item of drained) ReviewOverlay.markFlushed(item.path, item.content)
+
+    // End-of-turn flush also calls enqueueUnflushed; entries still holds the
+    // staged content since only clear()/reset() remove it.
+    ReviewOverlay.enqueueUnflushed("sess-3")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([])
+  })
+
+  it("enqueueUnflushed retries content that was drained but never flushed", () => {
+    // A drained write whose client RPC was rejected must not be treated as
+    // delivered. Without markFlushed, the end-of-turn enqueueUnflushed has to
+    // re-queue it so the edit is not silently lost.
+    ReviewOverlay.reset()
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.setActiveSession("sess-5")
+    ReviewOverlay.stage("/tmp/rejected.ts", "v1")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([
+      { sessionID: "sess-5", path: expect.stringContaining("rejected.ts"), content: "v1" },
+    ])
+
+    ReviewOverlay.enqueueUnflushed("sess-5")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([
+      { sessionID: "sess-5", path: expect.stringContaining("rejected.ts"), content: "v1" },
+    ])
+  })
+
+  it("enqueueUnflushed still resends a path after it changes again", () => {
+    ReviewOverlay.reset()
+    ReviewOverlay.setEnabled(true)
+    ReviewOverlay.setActiveSession("sess-4")
+    ReviewOverlay.stage("/tmp/twice.ts", "v1")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([
+      { sessionID: "sess-4", path: expect.stringContaining("twice.ts"), content: "v1" },
+    ])
+
+    ReviewOverlay.stage("/tmp/twice.ts", "v2")
+    ReviewOverlay.enqueueUnflushed("sess-4")
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([
+      { sessionID: "sess-4", path: expect.stringContaining("twice.ts"), content: "v2" },
+    ])
+  })
+
+  it("clear", () => {
+    ReviewOverlay.reset()
+    ReviewOverlay.setActiveSession("sess-1")
+    ReviewOverlay.stage("/tmp/x.ts", "x")
+    ReviewOverlay.clear()
+    expect(ReviewOverlay.has("/tmp/x.ts")).toBe(false)
+    expect(ReviewOverlay.drainPendingWrites()).toEqual([])
+  })
+})

packages/opencode/src/acp/event.ts

@@ -12,6 +12,7 @@ import { Effect } from "effect"
 import { ACPSession } from "./session"
 import { ACPPermission } from "./permission"
 import { partsToContentChunks, type ReplayPart } from "./content"
+import { ReviewOverlay } from "@opencode-ai/core/review-overlay"
 import {
   duplicateRunningToolUpdate,
   errorToolUpdate,
@@ -20,6 +21,7 @@ import {
   shellOutputSnapshot,
   completedToolUpdate,
 } from "./tool"
+import { flushPendingWrites } from "./review-staging"
 
 type Connection = Pick<AgentSideConnection, "sessionUpdate"> &
   Partial<Pick<AgentSideConnection, "requestPermission" | "writeTextFile">>
@@ -233,6 +235,7 @@ export class Subscription {
   }
 
   private async handleToolPart(sessionId: string, part: ToolPart, cwd: string) {
+    ReviewOverlay.setActiveSession(sessionId)
     await this.toolStart(sessionId, part, cwd)
 
     switch (part.state.status) {
@@ -258,6 +261,16 @@ export class Subscription {
             }),
           },
         })
+        // These tools may have staged edits in the overlay. Send them to the
+        // client now so they show up in the native review UI as the tool
+        // finishes. This is best-effort: a rejected write is left unflushed so
+        // the end-of-turn flush retries it, and that flush is the one that fails
+        // the turn if the client still rejects.
+        if (part.tool === "edit" || part.tool === "write" || part.tool === "apply_patch") {
+          await flushPendingWrites(this.input.connection, sessionId).catch((error: unknown) =>
+            Effect.runPromise(Effect.logError("failed to flush staged edits on tool completion", { error })),
+          )
+        }
         return
 
       case "error":

packages/opencode/src/acp/permission.ts

@@ -12,6 +12,7 @@ import { exists, readText } from "@/util/filesystem"
 import type { ACPSession } from "./session"
 import { pendingToolCall, toLocations, type ToolInput } from "./tool"
 import { Effect } from "effect"
+import { isActive } from "./review-mode"
 
 type PermissionEvent = Extract<Event, { type: "permission.asked" }>
 type Reply = "once" | "always" | "reject"
@@ -81,8 +82,18 @@ export class Handler {
       return
     }
 
-    if (permission.permission === "edit") {
-      await this.writeProposedEdit(session.id, permission.metadata).catch(() => {})
+    // In review mode the overlay already sends edits to the client, so skip the
+    // proposed-edit message to avoid showing the same change twice.
+    if (permission.permission === "edit" && !isActive()) {
+      await this.writeProposedEdit(session.id, permission.metadata).catch((error: unknown) =>
+        Effect.runPromise(
+          Effect.logError("failed to write proposed edit through ACP", {
+            error,
+            permissionID: permission.id,
+            sessionID: permission.sessionID,
+          }),
+        ),
+      )
     }
 
     await this.reply(permission.id, reply, session.cwd)

packages/opencode/src/acp/review-mode.ts

@@ -0,0 +1,37 @@
+export * as ACPReviewMode from "./review-mode"
+
+import { Flag, truthy } from "@opencode-ai/core/flag/flag"
+import { ReviewOverlay } from "@opencode-ai/core/review-overlay"
+
+let clientWriteTextFile = false
+let forcedForAcp = false
+
+export function forceEnableForAcp() {
+  forcedForAcp = true
+  syncEnabled()
+}
+
+export function setClientWriteTextFileSupported(supported: boolean) {
+  clientWriteTextFile = supported
+  syncEnabled()
+}
+
+// Review staging only works when the ACP client can receive staged edits via
+// `fs/write_text_file`. Without that capability we must fall through to normal
+// disk writes; otherwise edits would be staged in-memory and silently dropped
+// (never written to disk nor sent to the client).
+export function isActive() {
+  if (Flag.OPENCODE_CLIENT !== "acp") return false
+  if (!clientWriteTextFile) return false
+  return forcedForAcp || truthy("OPENCODE_ACP_REVIEW")
+}
+
+export function syncEnabled() {
+  ReviewOverlay.setEnabled(isActive())
+}
+
+export function reset() {
+  clientWriteTextFile = false
+  forcedForAcp = false
+  ReviewOverlay.reset()
+}

packages/opencode/src/acp/review-staging.ts

@@ -0,0 +1,64 @@
+export * as ACPReviewStaging from "./review-staging"
+
+import type { AgentSideConnection } from "@agentclientprotocol/sdk"
+import { Effect } from "effect"
+import { ReviewOverlay } from "@opencode-ai/core/review-overlay"
+import { isActive } from "./review-mode"
+
+type Connection = Partial<Pick<AgentSideConnection, "writeTextFile">>
+
+// The per-tool flush (from the ACP event loop) and the end-of-turn flush (from
+// the prompt handler) run on independent async contexts but share the overlay.
+// Serialize them so each drain -> send -> markFlushed cycle is atomic: without
+// this, a slow writeTextFile still in flight when the end-of-turn flush drains
+// would re-send the same not-yet-flushed edit and produce a duplicate write.
+let flushChain: Promise<void> = Promise.resolve()
+
+export function flushPendingWrites(connection: Connection | undefined, sessionID?: string) {
+  const next = flushChain.then(() => flushOnce(connection, sessionID))
+  // Keep the chain going even when a flush rejects, so one failed flush does not
+  // poison later flushes; callers still observe the rejection via `next`.
+  flushChain = next.then(
+    () => undefined,
+    () => undefined,
+  )
+  return next
+}
+
+async function flushOnce(connection: Connection | undefined, sessionID?: string) {
+  if (!isActive()) return
+  if (!connection?.writeTextFile) return
+  if (sessionID) {
+    ReviewOverlay.setActiveSession(sessionID)
+    ReviewOverlay.enqueueUnflushed(sessionID)
+  }
+
+  const pending = ReviewOverlay.drainPendingWrites()
+
+  // Only mark a write flushed once the client accepts it. If the client rejects
+  // a write we must not treat it as delivered: the edit never reached the review
+  // UI, and in review mode opencode never writes it to disk either, so silently
+  // dropping it would surface a failed edit as a success. Unflushed entries stay
+  // eligible for the end-of-turn retry, and a rejection fails the flush so the
+  // turn can report the failure instead of pretending it worked.
+  const failed: { path: string; error: unknown }[] = []
+  for (const item of pending) {
+    const error = await connection
+      .writeTextFile({
+        sessionId: item.sessionID,
+        path: item.path,
+        content: item.content,
+      })
+      .then(() => undefined)
+      .catch((cause: unknown) => cause ?? new Error("writeTextFile rejected"))
+    if (error !== undefined) {
+      failed.push({ path: item.path, error })
+      continue
+    }
+    ReviewOverlay.markFlushed(item.path, item.content)
+  }
+
+  if (failed.length === 0) return
+  await Effect.runPromise(Effect.logError("client rejected staged edits", { failed }))
+  throw new Error(`client rejected ${failed.length} staged edit(s): ${failed.map((item) => item.path).join(", ")}`)
+}