From 9be36d16062b02dd31b5114240bc14adf02cb927 Mon Sep 17 00:00:00 2001 From: Paco Preciado Mendoza Date: Mon, 8 Jun 2026 10:57:03 -0600 Subject: [PATCH 1/4] feat(core): add ReviewOverlay for ACP review staging 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. Co-authored-by: Cursor --- packages/core/src/review-overlay.ts | 79 +++++++++++++++++++++++ packages/core/test/review-overlay.test.ts | 56 ++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 packages/core/src/review-overlay.ts create mode 100644 packages/core/test/review-overlay.test.ts diff --git a/packages/core/src/review-overlay.ts b/packages/core/src/review-overlay.ts new file mode 100644 index 000000000000..4a55fb396c48 --- /dev/null +++ b/packages/core/src/review-overlay.ts @@ -0,0 +1,79 @@ +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() +const pending: PendingWrite[] = [] + +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 and delete markers. +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 + pending.push({ sessionID, path, content: entry.content }) + queued.add(path) + } +} + +export function drainPendingWrites() { + const drained = [...pending] + pending.length = 0 + return drained +} + +// Drop staged state at the end of a turn but keep review mode enabled. +export function clear() { + entries.clear() + pending.length = 0 + 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 +} diff --git a/packages/core/test/review-overlay.test.ts b/packages/core/test/review-overlay.test.ts new file mode 100644 index 000000000000..697383c5c383 --- /dev/null +++ b/packages/core/test/review-overlay.test.ts @@ -0,0 +1,56 @@ +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("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([]) + }) +}) From 40ebd05be1c4af663c4e590e0017d33f251077fb Mon Sep 17 00:00:00 2001 From: Paco Preciado Mendoza Date: Mon, 8 Jun 2026 10:57:07 -0600 Subject: [PATCH 2/4] feat(opencode): add review mode and ReviewFs layer 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. Co-authored-by: Cursor --- packages/opencode/src/acp/review-mode.ts | 37 +++++ packages/opencode/src/acp/review-staging.ts | 40 +++++ .../opencode/src/effect/review-fs-layer.ts | 140 ++++++++++++++++++ .../opencode/test/acp/review-mode.test.ts | 72 +++++++++ .../opencode/test/acp/review-staging.test.ts | 83 +++++++++++ .../test/effect/review-fs-layer.test.ts | 97 ++++++++++++ 6 files changed, 469 insertions(+) create mode 100644 packages/opencode/src/acp/review-mode.ts create mode 100644 packages/opencode/src/acp/review-staging.ts create mode 100644 packages/opencode/src/effect/review-fs-layer.ts create mode 100644 packages/opencode/test/acp/review-mode.test.ts create mode 100644 packages/opencode/test/acp/review-staging.test.ts create mode 100644 packages/opencode/test/effect/review-fs-layer.test.ts diff --git a/packages/opencode/src/acp/review-mode.ts b/packages/opencode/src/acp/review-mode.ts new file mode 100644 index 000000000000..543f5a836b1d --- /dev/null +++ b/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() +} diff --git a/packages/opencode/src/acp/review-staging.ts b/packages/opencode/src/acp/review-staging.ts new file mode 100644 index 000000000000..5783a549e48f --- /dev/null +++ b/packages/opencode/src/acp/review-staging.ts @@ -0,0 +1,40 @@ +export * as ACPReviewStaging from "./review-staging" + +import type { AgentSideConnection } from "@agentclientprotocol/sdk" +import * as Log from "@opencode-ai/core/util/log" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { isActive } from "./review-mode" + +const log = Log.create({ service: "acp-review-staging" }) + +type Connection = Partial> + +export async function flushPendingWrites(connection: Connection | undefined, sessionID?: string) { + if (!isActive()) return + if (!connection?.writeTextFile) return + if (sessionID) { + ReviewOverlay.setActiveSession(sessionID) + ReviewOverlay.enqueueUnflushed(sessionID) + } + + const pending = ReviewOverlay.drainPendingWrites() + if (pending.length > 0) { + log.info("flushing staged writes", { count: pending.length }) + } + + for (const item of pending) { + await connection + .writeTextFile({ + sessionId: item.sessionID, + path: item.path, + content: item.content, + }) + .catch((error: unknown) => { + log.error("failed to write staged edit through ACP", { + error, + path: item.path, + sessionID: item.sessionID, + }) + }) + } +} diff --git a/packages/opencode/src/effect/review-fs-layer.ts b/packages/opencode/src/effect/review-fs-layer.ts new file mode 100644 index 000000000000..2c3f14fe7cc6 --- /dev/null +++ b/packages/opencode/src/effect/review-fs-layer.ts @@ -0,0 +1,140 @@ +import { Effect, FileSystem, Layer, Option } from "effect" +import * as PlatformError from "effect/PlatformError" +import { FSUtil } from "@opencode-ai/core/fs-util" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" + +// FSUtil layer for ACP review mode. It wraps the real FSUtil: writes are sent to +// the ReviewOverlay (staged in memory) and reads return staged content, so the +// agent sees its own pending edits without touching disk. When review mode is +// off, every call passes straight through to the real FSUtil. + +const overlayNotFound = (method: string, path: string) => + PlatformError.systemError({ + _tag: "NotFound", + module: "FileSystem", + method, + pathOrDescriptor: path, + syscall: method, + }) + +function overlayFileStat(content: string): FileSystem.File.Info { + const now = Option.some(new Date()) + return { + type: "File", + mtime: now, + atime: now, + birthtime: now, + dev: 0, + rdev: Option.some(0), + ino: Option.none(), + mode: 0o644, + nlink: Option.some(1), + uid: Option.none(), + gid: Option.none(), + size: FileSystem.Size(new TextEncoder().encode(content).length), + blksize: Option.none(), + blocks: Option.none(), + } +} + +export const layer = Layer.effect( + FSUtil.Service, + Effect.gen(function* () { + const fs = yield* FSUtil.Service + + const readFileString = Effect.fn("ReviewFs.readFileString")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.readFileString(path) + const entry = ReviewOverlay.get(path) + if (entry) { + if ("deleted" in entry) return yield* Effect.fail(overlayNotFound("readFileString", path)) + return entry.content + } + return yield* fs.readFileString(path) + }) + + const readFile = Effect.fn("ReviewFs.readFile")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.readFile(path) + const entry = ReviewOverlay.get(path) + if (entry) { + if ("deleted" in entry) return yield* Effect.fail(overlayNotFound("readFile", path)) + return new TextEncoder().encode(entry.content) + } + return yield* fs.readFile(path) + }) + + const readFileStringSafe = Effect.fn("ReviewFs.readFileStringSafe")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.readFileStringSafe(path) + const entry = ReviewOverlay.get(path) + if (entry) { + if ("deleted" in entry) return undefined + return entry.content + } + return yield* fs.readFileStringSafe(path) + }) + + const exists = Effect.fn("ReviewFs.exists")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.exists(path) + const entry = ReviewOverlay.get(path) + if (entry) return !("deleted" in entry) + return yield* fs.exists(path) + }) + + const stat = Effect.fn("ReviewFs.stat")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.stat(path) + const entry = ReviewOverlay.get(path) + if (entry) { + if ("deleted" in entry) return yield* Effect.fail(overlayNotFound("stat", path)) + const disk = yield* fs.stat(path).pipe(Effect.catch(() => Effect.succeed(undefined))) + if (disk?.type === "File") { + return { ...disk, size: FileSystem.Size(new TextEncoder().encode(entry.content).length) } + } + if (disk) return disk + return overlayFileStat(entry.content) + } + return yield* fs.stat(path) + }) + + const isFile = Effect.fn("ReviewFs.isFile")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.isFile(path) + const entry = ReviewOverlay.get(path) + if (entry) return !("deleted" in entry) + return yield* fs.isFile(path) + }) + + const isDir = Effect.fn("ReviewFs.isDir")(function* (path: string) { + if (!ReviewOverlay.isEnabled()) return yield* fs.isDir(path) + const entry = ReviewOverlay.get(path) + if (entry) return false + return yield* fs.isDir(path) + }) + + const writeWithDirs = Effect.fn("ReviewFs.writeWithDirs")(function* ( + path: string, + content: string | Uint8Array, + mode?: number, + ) { + // Only text writes can be staged and shown in the review UI. Binary writes + // (and any write while review mode is off) go straight to disk. + if (!ReviewOverlay.isEnabled() || typeof content !== "string") { + return yield* fs.writeWithDirs(path, content, mode) + } + ReviewOverlay.stage(path, content) + }) + + return FSUtil.Service.of({ + ...fs, + readFileString, + readFile, + readFileStringSafe, + exists, + stat, + isFile, + isDir, + writeWithDirs, + }) + }), +).pipe(Layer.provide(FSUtil.defaultLayer)) + +export const defaultLayer = layer + +export * as ReviewFs from "./review-fs-layer" diff --git a/packages/opencode/test/acp/review-mode.test.ts b/packages/opencode/test/acp/review-mode.test.ts new file mode 100644 index 000000000000..281806c71fad --- /dev/null +++ b/packages/opencode/test/acp/review-mode.test.ts @@ -0,0 +1,72 @@ +import { afterEach, describe, expect, it } from "bun:test" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { forceEnableForAcp, isActive, reset, setClientWriteTextFileSupported, syncEnabled } from "@/acp/review-mode" + +describe("ACPReviewMode", () => { + afterEach(() => { + delete process.env.OPENCODE_ACP_REVIEW + delete process.env.OPENCODE_CLIENT + reset() + }) + + it("is inactive without flag even when capability is present", () => { + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(true) + syncEnabled() + expect(isActive()).toBe(false) + expect(ReviewOverlay.isEnabled()).toBe(false) + }) + + it("is inactive outside acp client", () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "cli" + setClientWriteTextFileSupported(true) + syncEnabled() + expect(isActive()).toBe(false) + }) + + it("is inactive when the client lacks writeTextFile capability, even with the env flag", () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(false) + syncEnabled() + expect(isActive()).toBe(false) + expect(ReviewOverlay.isEnabled()).toBe(false) + }) + + it("is inactive when forced but the client lacks writeTextFile capability", () => { + process.env.OPENCODE_CLIENT = "acp" + forceEnableForAcp() + setClientWriteTextFileSupported(false) + expect(isActive()).toBe(false) + expect(ReviewOverlay.isEnabled()).toBe(false) + }) + + it("is active when flag, client, and capability are set", () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(true) + syncEnabled() + expect(isActive()).toBe(true) + expect(ReviewOverlay.isEnabled()).toBe(true) + }) + + it("is active when forced and the client supports writeTextFile, without the env flag", () => { + process.env.OPENCODE_CLIENT = "acp" + forceEnableForAcp() + setClientWriteTextFileSupported(true) + expect(isActive()).toBe(true) + expect(ReviewOverlay.isEnabled()).toBe(true) + }) + + it("deactivates when capability is later reported missing", () => { + process.env.OPENCODE_CLIENT = "acp" + forceEnableForAcp() + setClientWriteTextFileSupported(true) + expect(isActive()).toBe(true) + + setClientWriteTextFileSupported(false) + expect(isActive()).toBe(false) + expect(ReviewOverlay.isEnabled()).toBe(false) + }) +}) diff --git a/packages/opencode/test/acp/review-staging.test.ts b/packages/opencode/test/acp/review-staging.test.ts new file mode 100644 index 000000000000..9d26c1da7247 --- /dev/null +++ b/packages/opencode/test/acp/review-staging.test.ts @@ -0,0 +1,83 @@ +import { afterEach, describe, expect, it } from "bun:test" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { flushPendingWrites } from "@/acp/review-staging" +import { reset, setClientWriteTextFileSupported, syncEnabled } from "@/acp/review-mode" + +describe("ACPReviewStaging", () => { + afterEach(() => { + delete process.env.OPENCODE_ACP_REVIEW + delete process.env.OPENCODE_CLIENT + reset() + }) + + it("awaits one writeTextFile per staged path", async () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(true) + syncEnabled() + + ReviewOverlay.setActiveSession("sess-1") + ReviewOverlay.stage("/tmp/a.ts", "a") + ReviewOverlay.stage("/tmp/b.ts", "b") + + const calls: Array<{ sessionId: string; path: string; content: string }> = [] + await flushPendingWrites({ + writeTextFile: async (input) => { + calls.push(input) + return {} + }, + }) + + expect(calls).toHaveLength(2) + expect(calls[0]?.sessionId).toBe("sess-1") + expect(calls[0]?.content).toBe("a") + expect(calls[1]?.content).toBe("b") + expect(ReviewOverlay.drainPendingWrites()).toEqual([]) + }) + + it("recovers staged files when session was set late", async () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(true) + syncEnabled() + + ReviewOverlay.stage("/tmp/late.ts", "late content") + + const calls: Array<{ sessionId: string; path: string; content: string }> = [] + await flushPendingWrites( + { + writeTextFile: async (input) => { + calls.push(input) + return {} + }, + }, + "sess-late", + ) + + expect(calls).toEqual([ + { + sessionId: "sess-late", + path: expect.stringContaining("late.ts"), + content: "late content", + }, + ]) + }) + + it("skips flush when review mode is inactive", async () => { + ReviewOverlay.setEnabled(true) + ReviewOverlay.setActiveSession("sess-1") + ReviewOverlay.stage("/tmp/a.ts", "a") + + let called = false + await flushPendingWrites({ + writeTextFile: async () => { + called = true + return {} + }, + }) + + expect(called).toBe(false) + expect(ReviewOverlay.drainPendingWrites()).toHaveLength(1) + ReviewOverlay.reset() + }) +}) diff --git a/packages/opencode/test/effect/review-fs-layer.test.ts b/packages/opencode/test/effect/review-fs-layer.test.ts new file mode 100644 index 000000000000..747d578c722e --- /dev/null +++ b/packages/opencode/test/effect/review-fs-layer.test.ts @@ -0,0 +1,97 @@ +import { afterEach, describe, expect } from "bun:test" +import fs from "fs/promises" +import path from "path" +import { Effect } from "effect" +import { FSUtil } from "@opencode-ai/core/fs-util" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { ReviewFs } from "@/effect/review-fs-layer" +import { TestInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +const it = testEffect(ReviewFs.defaultLayer) + +afterEach(() => { + ReviewOverlay.reset() +}) + +const readDisk = (filepath: string) => Effect.promise(() => fs.readFile(filepath, "utf-8").catch(() => "")) + +describe("ReviewFs overlay", () => { + it.instance("stages writes without mutating disk", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + ReviewOverlay.setEnabled(true) + ReviewOverlay.setActiveSession("sess-1") + + const filepath = path.join(test.directory, "staged.txt") + yield* afs.writeWithDirs(filepath, "staged content") + + expect(yield* readDisk(filepath)).toBe("") + expect(yield* afs.readFileString(filepath)).toBe("staged content") + expect(yield* afs.exists(filepath)).toBe(true) + expect(yield* afs.isFile(filepath)).toBe(true) + const info = yield* afs.stat(filepath) + expect(info.type).toBe("File") + }), + ) + + it.instance("read-after-write returns latest staged content", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + ReviewOverlay.setEnabled(true) + + const filepath = path.join(test.directory, "twice.txt") + yield* afs.writeWithDirs(filepath, "first") + yield* afs.writeWithDirs(filepath, "second") + expect(yield* afs.readFileString(filepath)).toBe("second") + expect(yield* readDisk(filepath)).toBe("") + }), + ) + + it.instance("readFile returns staged bytes for Bom.readFile", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + ReviewOverlay.setEnabled(true) + + const filepath = path.join(test.directory, "bytes.txt") + yield* afs.writeWithDirs(filepath, "utf8 body") + const bytes = yield* afs.readFile(filepath) + expect(new TextDecoder().decode(bytes)).toBe("utf8 body") + }), + ) + + it.instance("markDeleted hides overlay paths", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + ReviewOverlay.setEnabled(true) + + const filepath = path.join(test.directory, "gone.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "on disk", "utf-8")) + ReviewOverlay.markDeleted(filepath) + + expect(yield* afs.exists(filepath)).toBe(false) + expect(yield* afs.isFile(filepath)).toBe(false) + const stat = yield* afs.stat(filepath).pipe(Effect.catch(() => Effect.succeed(undefined))) + expect(stat).toBeUndefined() + }), + ) + + it.instance("updates see staged content from disk baseline", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + ReviewOverlay.setEnabled(true) + + const filepath = path.join(test.directory, "base.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "line1\nline2\n", "utf-8")) + yield* afs.writeWithDirs(filepath, "line1\nchanged\n") + + expect(yield* afs.readFileString(filepath)).toBe("line1\nchanged\n") + expect(yield* readDisk(filepath)).toBe("line1\nline2\n") + }), + ) +}) From b12937ef81e250ea91f9c2c8960907e627f8ec64 Mon Sep 17 00:00:00 2001 From: Paco Preciado Mendoza Date: Mon, 8 Jun 2026 10:57:10 -0600 Subject: [PATCH 3/4] feat(acp): stage file edits for native review in ACP clients 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. Co-authored-by: Cursor --- packages/opencode/src/acp/event.ts | 8 + packages/opencode/src/acp/permission.ts | 5 +- packages/opencode/src/acp/service.ts | 159 ++++++++------- packages/opencode/src/acp/tool.ts | 36 ++++ packages/opencode/src/cli/cmd/acp.ts | 5 + .../server/routes/instance/httpapi/server.ts | 3 +- packages/opencode/src/tool/apply_patch.ts | 66 ++++--- packages/opencode/src/tool/edit.ts | 54 +++-- packages/opencode/src/tool/registry.ts | 3 +- packages/opencode/src/tool/write.ts | 54 +++-- packages/opencode/test/acp/event.test.ts | 54 ++++- .../opencode/test/acp/service-session.test.ts | 59 ++++++ packages/opencode/test/acp/tool.test.ts | 64 ++++++ .../test/tool/review-mode-tools.test.ts | 187 ++++++++++++++++++ 14 files changed, 623 insertions(+), 134 deletions(-) create mode 100644 packages/opencode/test/tool/review-mode-tools.test.ts diff --git a/packages/opencode/src/acp/event.ts b/packages/opencode/src/acp/event.ts index 21ac68afb3a9..4cce68cb656a 100644 --- a/packages/opencode/src/acp/event.ts +++ b/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 & Partial> @@ -232,6 +234,7 @@ export class Subscription { } private async handleToolPart(sessionId: string, part: ToolPart) { + ReviewOverlay.setActiveSession(sessionId) await this.toolStart(sessionId, part) switch (part.state.status) { @@ -256,6 +259,11 @@ 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. + if (part.tool === "edit" || part.tool === "write" || part.tool === "apply_patch") { + await flushPendingWrites(this.input.connection, sessionId) + } return case "error": diff --git a/packages/opencode/src/acp/permission.ts b/packages/opencode/src/acp/permission.ts index 357754e093e9..418d752bb9f4 100644 --- a/packages/opencode/src/acp/permission.ts +++ b/packages/opencode/src/acp/permission.ts @@ -5,6 +5,7 @@ import { exists, readText } from "@/util/filesystem" import type { ACPSession } from "./session" import { toLocations, toToolKind, type ToolInput } from "./tool" import { Effect } from "effect" +import { isActive } from "./review-mode" type PermissionEvent = Extract type Reply = "once" | "always" | "reject" @@ -77,7 +78,9 @@ export class Handler { return } - if (permission.permission === "edit") { + // 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(() => {}) } diff --git a/packages/opencode/src/acp/service.ts b/packages/opencode/src/acp/service.ts index 36e8375f5cc4..74642e6395fb 100644 --- a/packages/opencode/src/acp/service.ts +++ b/packages/opencode/src/acp/service.ts @@ -40,6 +40,9 @@ import { ACPEvent } from "./event" import { ACPSession } from "./session" import { UsageService } from "./usage" import { ACPProfile } from "./profile" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { syncEnabled, setClientWriteTextFileSupported } from "./review-mode" +import { flushPendingWrites } from "./review-staging" import { ProviderV2 } from "@opencode-ai/core/provider" import { ModelV2 } from "@opencode-ai/core/model" import { Provider } from "@/provider/provider" @@ -106,6 +109,14 @@ export function make(input: { } } + // Review mode needs the client to accept staged edits via writeTextFile. + // Record the capability from initialize so review activates only when usable. + setClientWriteTextFileSupported(params.clientCapabilities?.fs?.writeTextFile === true) + syncEnabled() + if (ReviewOverlay.isEnabled()) { + log.info("ACP review-at-end active") + } + const response = { protocolVersion: 1, agentCapabilities: { @@ -340,6 +351,7 @@ export function make(input: { const removed = yield* session.remove(params.sessionId) registeredMcp.delete(params.sessionId) sessionSnapshots.delete(params.sessionId) + ReviewOverlay.clear() if (!removed) return {} yield* abortBackingSession(removed) @@ -348,6 +360,7 @@ export function make(input: { const cancel = Effect.fn("ACP.cancel")(function* (params: CancelNotification) { const current = yield* session.get(params.sessionId) + ReviewOverlay.clear() yield* abortBackingSession(current) }) @@ -491,79 +504,89 @@ export function make(input: { setSessionModel, prompt: Effect.fn("ACP.prompt")(function* (params: PromptRequest) { const current = yield* session.get(params.sessionId) - const snapshot = yield* directorySnapshot(current.cwd) - const selected = current.model ?? selectDefaultModel(snapshot) - if (!current.model) { - yield* session.setModel(params.sessionId, selected) - } - const variant = current.variant ?? selectVariant(snapshot, selected) - const modeId = current.modeId ?? (snapshot.availableModes.length > 0 ? snapshot.defaultModeID : undefined) - const parts = promptContentToParts(params.prompt) - const command = detectSlashCommand(parts) - - if (!command) { - const response = yield* request( - () => - input.sdk.session.prompt( - { - sessionID: current.id, - model: { + ReviewOverlay.setActiveSession(current.id) + syncEnabled() + // Always clear the staging overlay when the turn settles — success, + // failure, or interruption — so partial edits from a failed turn never + // leak into the next prompt. + return yield* Effect.gen(function* () { + const snapshot = yield* directorySnapshot(current.cwd) + const selected = current.model ?? selectDefaultModel(snapshot) + if (!current.model) { + yield* session.setModel(params.sessionId, selected) + } + const variant = current.variant ?? selectVariant(snapshot, selected) + const modeId = current.modeId ?? (snapshot.availableModes.length > 0 ? snapshot.defaultModeID : undefined) + const parts = promptContentToParts(params.prompt) + const command = detectSlashCommand(parts) + + if (!command) { + const response = yield* request( + () => + input.sdk.session.prompt( + { + sessionID: current.id, + model: { + providerID: selected.providerID, + modelID: selected.modelID, + }, + ...(variant ? { variant } : {}), + parts, + ...(modeId ? { agent: modeId } : {}), + directory: current.cwd, + }, + { throwOnError: true }, + ), + "session", + ) + yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd) + yield* Effect.promise(() => flushPendingWrites(input.connection, current.id)) + return promptResponse(response.info, params.messageId) + } + + const known = snapshot.availableCommands.find((item) => item.name === command.name) + if (known) { + const response = yield* request( + () => + input.sdk.session.command( + { + sessionID: current.id, + command: known.name, + arguments: command.args, + model: `${selected.providerID}/${selected.modelID}`, + ...(variant ? { variant } : {}), + ...(modeId ? { agent: modeId } : {}), + directory: current.cwd, + }, + { throwOnError: true }, + ), + "session", + ) + yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd) + yield* Effect.promise(() => flushPendingWrites(input.connection, current.id)) + return promptResponse(response.info, params.messageId) + } + + if (command.name === "compact") { + yield* request( + () => + input.sdk.session.summarize( + { + sessionID: current.id, + directory: current.cwd, providerID: selected.providerID, modelID: selected.modelID, }, - ...(variant ? { variant } : {}), - parts, - ...(modeId ? { agent: modeId } : {}), - directory: current.cwd, - }, - { throwOnError: true }, - ), - "session", - ) - yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd) - return promptResponse(response.info, params.messageId) - } + { throwOnError: true }, + ), + "session", + ) + } - const known = snapshot.availableCommands.find((item) => item.name === command.name) - if (known) { - const response = yield* request( - () => - input.sdk.session.command( - { - sessionID: current.id, - command: known.name, - arguments: command.args, - model: `${selected.providerID}/${selected.modelID}`, - ...(variant ? { variant } : {}), - ...(modeId ? { agent: modeId } : {}), - directory: current.cwd, - }, - { throwOnError: true }, - ), - "session", - ) yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd) - return promptResponse(response.info, params.messageId) - } - - if (command.name === "compact") { - yield* request( - () => - input.sdk.session.summarize( - { - sessionID: current.id, - directory: current.cwd, - providerID: selected.providerID, - modelID: selected.modelID, - }, - { throwOnError: true }, - ), - "session", - ) - } - - yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd) - return promptResponse(undefined, params.messageId) + yield* Effect.promise(() => flushPendingWrites(input.connection, current.id)) + return promptResponse(undefined, params.messageId) + }).pipe(Effect.ensuring(Effect.sync(() => ReviewOverlay.clear()))) }), cancel, } diff --git a/packages/opencode/src/acp/tool.ts b/packages/opencode/src/acp/tool.ts index 4c39c0eff1fd..72791740fce5 100644 --- a/packages/opencode/src/acp/tool.ts +++ b/packages/opencode/src/acp/tool.ts @@ -110,6 +110,10 @@ export function completedToolContent(toolName: string, state: CompletedToolState }, ] + if (toolName.toLocaleLowerCase() === "apply_patch") { + content.push(...applyPatchDiffContent(state.metadata)) + } + if (toToolKind(toolName) === "edit") { content.push(...diffContent(state.input)) } @@ -264,6 +268,14 @@ export const buildDuplicateRunningToolUpdate = duplicateRunningToolUpdate export const buildCompletedToolUpdate = completedToolUpdate export const buildErrorToolUpdate = errorToolUpdate +export function deleteDiffContent(path: string, oldText: string): ToolCallContent[] { + return [{ type: "diff", path, oldText, newText: "" }] +} + +export function moveDiffContent(dest: string, oldText: string, newText: string): ToolCallContent[] { + return [{ type: "diff", path: dest, oldText, newText }] +} + function locationFrom(...values: unknown[]): ToolCallLocation[] { return Array.from( new Set( @@ -279,6 +291,30 @@ function locationFrom(...values: unknown[]): ToolCallLocation[] { ) } +// apply_patch deletes and moves go straight to disk, so the client never gets a +// writeTextFile for them. Build diff content from the tool metadata so those +// changes still show up as a diff in the tool call. Metadata is untrusted, so +// every field is checked before use. +function applyPatchDiffContent(metadata: unknown): ToolCallContent[] { + if (!metadata || typeof metadata !== "object") return [] + const files = (metadata as Record).files + if (!Array.isArray(files)) return [] + + return files.flatMap((file): ToolCallContent[] => { + if (!file || typeof file !== "object") return [] + const info = file as Record + const type = stringValue(info.type) + const path = stringValue(info.diffPath) ?? stringValue(info.movePath) ?? stringValue(info.filePath) + const oldText = stringValue(info.oldText) + if (!path || oldText === undefined) return [] + const newText = stringValue(info.newText) ?? "" + + if (type === "delete") return deleteDiffContent(path, oldText) + if (type === "move") return moveDiffContent(path, oldText, newText) + return [] + }) +} + function diffContent(input: ToolInput): ToolCallContent[] { const oldText = stringValue(input.oldString) const newText = stringValue(input.newString) ?? stringValue(input.content) diff --git a/packages/opencode/src/cli/cmd/acp.ts b/packages/opencode/src/cli/cmd/acp.ts index da47d9579578..b5c067ba0783 100644 --- a/packages/opencode/src/cli/cmd/acp.ts +++ b/packages/opencode/src/cli/cmd/acp.ts @@ -5,6 +5,7 @@ import { ServerAuth } from "@/server/auth" import { createOpencodeClient } from "@opencode-ai/sdk/v2" import { withNetworkOptions, resolveNetworkOptions } from "../network" import { ACPProfile } from "@/acp/profile" +import { forceEnableForAcp } from "@/acp/review-mode" export const AcpCommand = effectCmd({ command: "acp", @@ -21,6 +22,10 @@ export const AcpCommand = effectCmd({ const { ACP } = yield* Effect.promise(() => import("@/acp/agent")) ACPProfile.mark("cli.acp.handler") process.env.OPENCODE_CLIENT = "acp" + // Opt this ACP session into review mode. It still only activates if the + // client reports the writeTextFile capability during initialize. + forceEnableForAcp() + log.info("ACP review-at-end enabled") const opts = yield* resolveNetworkOptions(args) const server = yield* Effect.promise(() => ACPProfile.measure("cli.acp.server.listen", () => Server.listen(opts))) diff --git a/packages/opencode/src/server/routes/instance/httpapi/server.ts b/packages/opencode/src/server/routes/instance/httpapi/server.ts index aa274142c15a..47a56966538c 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/server.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/server.ts @@ -10,6 +10,7 @@ import { } from "effect/unstable/http" import * as Socket from "effect/unstable/socket/Socket" import { FSUtil } from "@opencode-ai/core/fs-util" +import { ReviewFs } from "@/effect/review-fs-layer" import { Account } from "@/account/account" import { Agent } from "@/agent/agent" import { Auth } from "@/auth" @@ -258,7 +259,7 @@ export function createRoutes( Vcs.defaultLayer, Workspace.defaultLayer, Worktree.appLayer, - FSUtil.defaultLayer, + ReviewFs.defaultLayer, FetchHttpClient.layer, HttpServer.layerServices, ]), diff --git a/packages/opencode/src/tool/apply_patch.ts b/packages/opencode/src/tool/apply_patch.ts index f9201be8a7db..bf963b934883 100644 --- a/packages/opencode/src/tool/apply_patch.ts +++ b/packages/opencode/src/tool/apply_patch.ts @@ -14,6 +14,8 @@ import DESCRIPTION from "./apply_patch.txt" import { FileSystem } from "@opencode-ai/core/filesystem" import { Format } from "../format" import * as Bom from "@/util/bom" +import { isActive } from "@/acp/review-mode" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" export const Parameters = Schema.Struct({ patchText: Schema.String.annotate({ description: "The full patch text that describes all changes to be made" }), @@ -199,6 +201,9 @@ export const ApplyPatchTool = Tool.define( additions: change.additions, deletions: change.deletions, movePath: change.movePath, + diffPath: change.movePath ?? change.filePath, + oldText: change.oldContent, + newText: change.type === "delete" ? "" : change.newContent, })) // Check permissions if needed @@ -214,6 +219,11 @@ export const ApplyPatchTool = Tool.define( }, }) + // In review mode, add/update writes are staged in the overlay (see ReviewFs) + // instead of going to disk. Skip the disk-only steps below: formatting, + // file events, and LSP diagnostics. + const review = isActive() + // Apply the changes const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = [] @@ -221,8 +231,6 @@ export const ApplyPatchTool = Tool.define( const edited = change.type === "delete" ? undefined : (change.movePath ?? change.filePath) switch (change.type) { case "add": - // Create parent directories (recursive: true is safe on existing/root dirs) - yield* afs.writeWithDirs(change.filePath, Bom.join(change.newContent, change.bom)) updates.push({ file: change.filePath, event: "add" }) break @@ -234,10 +242,9 @@ export const ApplyPatchTool = Tool.define( case "move": if (change.movePath) { - // Create parent directories (recursive: true is safe on existing/root dirs) - yield* afs.writeWithDirs(change.movePath!, Bom.join(change.newContent, change.bom)) yield* afs.remove(change.filePath) + if (review) ReviewOverlay.markDeleted(change.filePath) updates.push({ file: change.filePath, event: "unlink" }) updates.push({ file: change.movePath, event: "add" }) } @@ -245,11 +252,12 @@ export const ApplyPatchTool = Tool.define( case "delete": yield* afs.remove(change.filePath) + if (review) ReviewOverlay.markDeleted(change.filePath) updates.push({ file: change.filePath, event: "unlink" }) break } - if (edited) { + if (edited && !review) { if (yield* format.file(edited)) { yield* Bom.syncFile(afs, edited, change.bom) } @@ -257,18 +265,22 @@ export const ApplyPatchTool = Tool.define( } } - // Publish file change events - for (const update of updates) { - yield* events.publish(Watcher.Event.Updated, update) + if (!review) { + for (const update of updates) { + yield* events.publish(Watcher.Event.Updated, update) + } } - // Notify LSP of file changes and collect diagnostics - for (const change of fileChanges) { - if (change.type === "delete") continue - const target = change.movePath ?? change.filePath - yield* lsp.touchFile(target, "document") - } - const diagnostics = yield* lsp.diagnostics() + const diagnostics = review + ? {} + : yield* Effect.gen(function* () { + for (const change of fileChanges) { + if (change.type === "delete") continue + const target = change.movePath ?? change.filePath + yield* lsp.touchFile(target, "document") + } + return yield* lsp.diagnostics() + }) // Generate output summary const summaryLines = fileChanges.map((change) => { @@ -283,13 +295,23 @@ export const ApplyPatchTool = Tool.define( }) let output = `Success. Updated the following files:\n${summaryLines.join("\n")}` - for (const change of fileChanges) { - if (change.type === "delete") continue - const target = change.movePath ?? change.filePath - const block = LSP.Diagnostic.report(target, diagnostics[FSUtil.normalizePath(target)] ?? []) - if (!block) continue - const rel = path.relative(instance.worktree, target).replaceAll("\\", "/") - output += `\n\nLSP errors detected in ${rel}, please fix:\n${block}` + // ACP has no deleteFile/renameFile, so deletions and moves are applied to + // disk immediately even in review mode (only add/update are staged for + // review). Surface this so a reviewer knows those changes are not undone + // by rejecting the staged edits. + if (review && fileChanges.some((change) => change.type === "delete" || change.type === "move")) { + output += `\n\nNote: deletions and moves were applied directly to disk and are not part of the staged review.` + } + + if (!review) { + for (const change of fileChanges) { + if (change.type === "delete") continue + const target = change.movePath ?? change.filePath + const block = LSP.Diagnostic.report(target, diagnostics[FSUtil.normalizePath(target)] ?? []) + if (!block) continue + const rel = path.relative(instance.worktree, target).replaceAll("\\", "/") + output += `\n\nLSP errors detected in ${rel}, please fix:\n${block}` + } } return { diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index a92e4720c0fc..5a3480fb3c73 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -18,6 +18,7 @@ import { Snapshot } from "@/snapshot" import { assertExternalDirectoryEffect } from "./external-directory" import { FSUtil } from "@opencode-ai/core/fs-util" import * as Bom from "@/util/bom" +import { isActive } from "@/acp/review-mode" function normalizeLineEndings(text: string): string { return text.replaceAll("\r\n", "\n") @@ -109,14 +110,18 @@ export const EditTool = Tool.define( }, }) yield* afs.writeWithDirs(filePath, Bom.join(contentNew, desiredBom)) - if (yield* format.file(filePath)) { - contentNew = yield* Bom.syncFile(afs, filePath, desiredBom) + // In review mode the write above is staged in the overlay, not on + // disk. Skip disk-only follow-ups (format + file events) here. + if (!isActive()) { + if (yield* format.file(filePath)) { + contentNew = yield* Bom.syncFile(afs, filePath, desiredBom) + } + yield* events.publish(FileSystem.Event.Edited, { file: filePath }) + yield* events.publish(Watcher.Event.Updated, { + file: filePath, + event: "add", + }) } - yield* events.publish(FileSystem.Event.Edited, { file: filePath }) - yield* events.publish(Watcher.Event.Updated, { - file: filePath, - event: "add", - }) return } @@ -153,14 +158,17 @@ export const EditTool = Tool.define( }) yield* afs.writeWithDirs(filePath, Bom.join(contentNew, desiredBom)) - if (yield* format.file(filePath)) { - contentNew = yield* Bom.syncFile(afs, filePath, desiredBom) + // Staged in review mode: skip format + file events (disk only). + if (!isActive()) { + if (yield* format.file(filePath)) { + contentNew = yield* Bom.syncFile(afs, filePath, desiredBom) + } + yield* events.publish(FileSystem.Event.Edited, { file: filePath }) + yield* events.publish(Watcher.Event.Updated, { + file: filePath, + event: "change", + }) } - yield* events.publish(FileSystem.Event.Edited, { file: filePath }) - yield* events.publish(Watcher.Event.Updated, { - file: filePath, - event: "change", - }) diff = trimDiff( createTwoFilesPatch( filePath, @@ -194,11 +202,19 @@ export const EditTool = Tool.define( }) let output = "Edit applied successfully." - yield* lsp.touchFile(filePath, "document") - const diagnostics = yield* lsp.diagnostics() - const normalizedFilePath = FSUtil.normalizePath(filePath) - const block = LSP.Diagnostic.report(filePath, diagnostics[normalizedFilePath] ?? []) - if (block) output += `\n\nLSP errors detected in this file, please fix:\n${block}` + // LSP needs the file on disk, so skip diagnostics while the edit is + // staged in review mode. + const diagnostics = isActive() + ? {} + : yield* Effect.gen(function* () { + yield* lsp.touchFile(filePath, "document") + return yield* lsp.diagnostics() + }) + if (!isActive()) { + const normalizedFilePath = FSUtil.normalizePath(filePath) + const block = LSP.Diagnostic.report(filePath, diagnostics[normalizedFilePath] ?? []) + if (block) output += `\n\nLSP errors detected in this file, please fix:\n${block}` + } return { metadata: { diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index 9d503414595b..b25bbbc39496 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -42,6 +42,7 @@ import { Todo } from "../session/todo" import { LSP } from "@/lsp/lsp" import { Instruction } from "../session/instruction" import { FSUtil } from "@opencode-ai/core/fs-util" +import { ReviewFs } from "@/effect/review-fs-layer" import { EventV2Bridge } from "@/event-v2-bridge" import { Agent } from "../agent/agent" import { Skill } from "../skill" @@ -353,7 +354,7 @@ export const defaultLayer = Layer.suspend(() => Layer.provide(Reference.defaultLayer), Layer.provide(LSP.defaultLayer), Layer.provide(Instruction.defaultLayer), - Layer.provide(FSUtil.defaultLayer), + Layer.provide(ReviewFs.defaultLayer), Layer.provide(EventV2Bridge.defaultLayer), Layer.provide(FetchHttpClient.layer), Layer.provide(Format.defaultLayer), diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 37be6d8c47bc..7211684f4d26 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -14,6 +14,7 @@ import { InstanceState } from "@/effect/instance-state" import { trimDiff } from "./edit" import { assertExternalDirectoryEffect } from "./external-directory" import * as Bom from "@/util/bom" +import { isActive } from "@/acp/review-mode" const MAX_PROJECT_DIAGNOSTICS_FILES = 5 @@ -62,31 +63,42 @@ export const WriteTool = Tool.define( }) yield* fs.writeWithDirs(filepath, Bom.join(contentNew, desiredBom)) - if (yield* format.file(filepath)) { - yield* Bom.syncFile(fs, filepath, desiredBom) + // In review mode the write above is staged in the overlay, not on disk. + // Skip disk-only follow-ups (format + file events) here. + if (!isActive()) { + if (yield* format.file(filepath)) { + yield* Bom.syncFile(fs, filepath, desiredBom) + } + yield* events.publish(FileSystem.Event.Edited, { file: filepath }) + yield* events.publish(Watcher.Event.Updated, { + file: filepath, + event: exists ? "change" : "add", + }) } - yield* events.publish(FileSystem.Event.Edited, { file: filepath }) - yield* events.publish(Watcher.Event.Updated, { - file: filepath, - event: exists ? "change" : "add", - }) let output = "Wrote file successfully." - yield* lsp.touchFile(filepath, "document") - const diagnostics = yield* lsp.diagnostics() - const normalizedFilepath = FSUtil.normalizePath(filepath) - let projectDiagnosticsCount = 0 - for (const [file, issues] of Object.entries(diagnostics)) { - const current = file === normalizedFilepath - if (!current && projectDiagnosticsCount >= MAX_PROJECT_DIAGNOSTICS_FILES) continue - const block = LSP.Diagnostic.report(current ? filepath : file, issues) - if (!block) continue - if (current) { - output += `\n\nLSP errors detected in this file, please fix:\n${block}` - continue + // LSP needs the file on disk, so skip diagnostics while staged. + const diagnostics = isActive() + ? {} + : yield* Effect.gen(function* () { + yield* lsp.touchFile(filepath, "document") + return yield* lsp.diagnostics() + }) + if (!isActive()) { + const normalizedFilepath = FSUtil.normalizePath(filepath) + let projectDiagnosticsCount = 0 + for (const [file, issues] of Object.entries(diagnostics)) { + const current = file === normalizedFilepath + if (!current && projectDiagnosticsCount >= MAX_PROJECT_DIAGNOSTICS_FILES) continue + const block = LSP.Diagnostic.report(current ? filepath : file, issues) + if (!block) continue + if (current) { + output += `\n\nLSP errors detected in this file, please fix:\n${block}` + continue + } + projectDiagnosticsCount++ + output += `\n\nLSP errors detected in other files:\n${block}` } - projectDiagnosticsCount++ - output += `\n\nLSP errors detected in other files:\n${block}` } return { diff --git a/packages/opencode/test/acp/event.test.ts b/packages/opencode/test/acp/event.test.ts index 4b20c7292d45..a2e559d48988 100644 --- a/packages/opencode/test/acp/event.test.ts +++ b/packages/opencode/test/acp/event.test.ts @@ -1,8 +1,10 @@ -import { describe, expect, it } from "bun:test" +import { afterEach, describe, expect, it } from "bun:test" import type { AgentSideConnection } from "@agentclientprotocol/sdk" import type { Event, Message, OpencodeClient, Part, SessionMessageResponse, ToolPart } from "@opencode-ai/sdk/v2" import { Effect, ManagedRuntime } from "effect" import { ACPEvent } from "@/acp/event" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { reset, setClientWriteTextFileSupported, syncEnabled } from "@/acp/review-mode" import * as ACPService from "@/acp/service" import { Directory } from "@/acp/directory" import { ACPSession } from "@/acp/session" @@ -740,4 +742,54 @@ describe("acp event routing", () => { ], ]) }) + + afterEach(() => { + delete process.env.OPENCODE_ACP_REVIEW + delete process.env.OPENCODE_CLIENT + reset() + }) + + it("flushes staged writes after completed edit tool", async () => { + process.env.OPENCODE_ACP_REVIEW = "1" + process.env.OPENCODE_CLIENT = "acp" + setClientWriteTextFileSupported(true) + syncEnabled() + + const writes: Array<{ sessionId: string; path: string; content: string }> = [] + const harness = createHarness() + const subscription = new ACPEvent.Subscription({ + sdk: harness.sdk, + connection: { + ...harness.connection, + writeTextFile: async (input) => { + writes.push(input) + return {} + }, + }, + session: harness.session, + }) + + ReviewOverlay.setActiveSession("ses_flush") + ReviewOverlay.stage("/workspace/file.ts", "staged body") + + await Effect.runPromise(harness.session.create({ id: "ses_flush", cwd: "/workspace" })) + await subscription.handle( + toolUpdated( + completedTool("ses_flush", "call_edit", "edited", [], { + tool: "edit", + input: { filePath: "/workspace/file.ts", oldString: "a", newString: "b" }, + }), + ), + ) + + expect(writes).toEqual([ + { + sessionId: "ses_flush", + path: expect.stringContaining("file.ts"), + content: "staged body", + }, + ]) + subscription.stop() + harness.events.close() + }) }) diff --git a/packages/opencode/test/acp/service-session.test.ts b/packages/opencode/test/acp/service-session.test.ts index 890719072abf..7cf2cd98d3e2 100644 --- a/packages/opencode/test/acp/service-session.test.ts +++ b/packages/opencode/test/acp/service-session.test.ts @@ -18,6 +18,8 @@ import * as ACPService from "@/acp/service" import * as ACPError from "@/acp/error" import { UsageService } from "@/acp/usage" import type { Provider } from "@/provider/provider" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { forceEnableForAcp, reset as resetReviewMode, setClientWriteTextFileSupported } from "@/acp/review-mode" const providerID = ProviderV2.ID.make("test") const modelID = ModelV2.ID.make("test-model") @@ -1143,6 +1145,63 @@ describe("ACP service sessions", () => { expect(error.code).toBe(-32000) }) + + it("clears the review overlay when a prompt fails mid-turn", async () => { + process.env.OPENCODE_CLIENT = "acp" + forceEnableForAcp() + setClientWriteTextFileSupported(true) + try { + const failing = ACPService.make({ + sdk: { + config: { + providers: () => Promise.resolve({ data: { providers: [provider], default: { test: modelID } } }), + get: () => Promise.resolve({ data: {} }), + }, + app: { + agents: () => Promise.resolve({ data: [{ name: "build", mode: "primary", permission: [], options: {} }] }), + skills: () => Promise.resolve({ data: [] }), + }, + command: { + list: () => Promise.resolve({ data: [] }), + }, + session: { + create: () => Promise.resolve({ data: { id: "ses_review_fail" } }), + list: () => Promise.resolve({ data: [] }), + prompt: () => Promise.reject(new Error("provider exploded")), + }, + mcp: { + add: () => Promise.resolve({ data: {} }), + }, + } as unknown as OpencodeClient, + connection: { + sessionUpdate: () => Promise.resolve(), + writeTextFile: () => Promise.resolve({}), + } as unknown as Pick, + usage: UsageService.Service.of({ + buildUsage: UsageService.buildUsage, + latestAssistantMessage: UsageService.latestAssistantMessage, + totalSessionCost: UsageService.totalSessionCost, + contextLimit: () => Effect.succeed(128000), + sendUpdate: () => Effect.void, + }), + }) + + const session = await Effect.runPromise(failing.newSession({ cwd: "/workspace", mcpServers: [] })) + ReviewOverlay.setActiveSession(session.sessionId) + ReviewOverlay.stage("/workspace/leftover.ts", "staged but turn fails") + expect(ReviewOverlay.get("/workspace/leftover.ts")).toBeDefined() + + const exit = await Effect.runPromiseExit( + failing.prompt({ sessionId: session.sessionId, prompt: [{ type: "text", text: "hello" }] }), + ) + expect(exit._tag).toBe("Failure") + + expect(ReviewOverlay.get("/workspace/leftover.ts")).toBeUndefined() + } finally { + delete process.env.OPENCODE_CLIENT + resetReviewMode() + } + }) }) function assistantInfo(tokens: UsageService.AssistantTokenCost["tokens"]): UsageService.AssistantMessage { diff --git a/packages/opencode/test/acp/tool.test.ts b/packages/opencode/test/acp/tool.test.ts index e7ad1c1b1638..b2557828e285 100644 --- a/packages/opencode/test/acp/tool.test.ts +++ b/packages/opencode/test/acp/tool.test.ts @@ -2,8 +2,10 @@ import { describe, expect, test } from "bun:test" import { completedToolContent, completedToolRawOutput, + deleteDiffContent, extractImageAttachments, imageContents, + moveDiffContent, shellOutputSnapshot, toLocations, toToolKind, @@ -207,4 +209,66 @@ describe("acp tool conversion", () => { expect(shellOutputSnapshot({ metadata: { output: 42 } })).toBeUndefined() expect(shellOutputSnapshot({ metadata: undefined })).toBeUndefined() }) + + test("builds Codex-style delete and move diff helpers", () => { + expect(deleteDiffContent("/tmp/remove.ts", "gone")).toEqual([ + { type: "diff", path: "/tmp/remove.ts", oldText: "gone", newText: "" }, + ]) + expect(moveDiffContent("/tmp/dest.ts", "old body", "new body")).toEqual([ + { type: "diff", path: "/tmp/dest.ts", oldText: "old body", newText: "new body" }, + ]) + }) + + test("builds apply_patch completed content with delete and move diffs from metadata", () => { + expect( + completedToolContent("apply_patch", { + status: "completed", + input: { patchText: "*** Begin Patch\n*** End Patch" }, + output: "Success.", + metadata: { + files: [ + { + type: "delete", + filePath: "/tmp/remove.ts", + diffPath: "/tmp/remove.ts", + oldText: "remove me", + newText: "", + }, + { + type: "move", + filePath: "/tmp/src.ts", + movePath: "/tmp/dest.ts", + diffPath: "/tmp/dest.ts", + oldText: "before move", + newText: "after move", + }, + { + type: "update", + filePath: "/tmp/keep.ts", + diffPath: "/tmp/keep.ts", + oldText: "old", + newText: "new", + }, + ], + }, + }), + ).toEqual([ + { + type: "content", + content: { type: "text", text: "Success." }, + }, + { + type: "diff", + path: "/tmp/remove.ts", + oldText: "remove me", + newText: "", + }, + { + type: "diff", + path: "/tmp/dest.ts", + oldText: "before move", + newText: "after move", + }, + ]) + }) }) diff --git a/packages/opencode/test/tool/review-mode-tools.test.ts b/packages/opencode/test/tool/review-mode-tools.test.ts new file mode 100644 index 000000000000..cf87bcb5dcdf --- /dev/null +++ b/packages/opencode/test/tool/review-mode-tools.test.ts @@ -0,0 +1,187 @@ +import { afterEach, describe, expect } from "bun:test" +import path from "path" +import fs from "fs/promises" +import { Effect, Layer } from "effect" +import { EditTool } from "../../src/tool/edit" +import { WriteTool } from "../../src/tool/write" +import { ApplyPatchTool } from "../../src/tool/apply_patch" +import { disposeAllInstances, TestInstance } from "../fixture/fixture" +import { LSP } from "@/lsp/lsp" +import { Format } from "../../src/format" +import { Agent } from "../../src/agent/agent" +import { EventV2Bridge } from "../../src/event-v2-bridge" +import { Truncate } from "@/tool/truncate" +import { SessionID, MessageID } from "../../src/session/schema" +import { testEffect } from "../lib/effect" +import { ReviewFs } from "@/effect/review-fs-layer" +import { ReviewOverlay } from "@opencode-ai/core/review-overlay" +import { forceEnableForAcp, reset, setClientWriteTextFileSupported } from "@/acp/review-mode" +import { completedToolContent } from "@/acp/tool" +import { ToolRegistry } from "@/tool/registry" +import { ProviderV2 } from "@opencode-ai/core/provider" +import { ModelV2 } from "@opencode-ai/core/model" + +const baseCtx = { + sessionID: SessionID.make("ses_review"), + messageID: MessageID.make("msg_review"), + callID: "", + agent: "build", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +} + +const layer = Layer.mergeAll( + LSP.defaultLayer, + ReviewFs.defaultLayer, + Format.defaultLayer, + EventV2Bridge.defaultLayer, + Truncate.defaultLayer, + Agent.defaultLayer, +) + +const it = testEffect(layer) + +afterEach(async () => { + delete process.env.OPENCODE_ACP_REVIEW + delete process.env.OPENCODE_CLIENT + reset() + await disposeAllInstances() +}) + +const enableReview = () => { + process.env.OPENCODE_CLIENT = "acp" + forceEnableForAcp() + setClientWriteTextFileSupported(true) + ReviewOverlay.setActiveSession("ses_review") +} + +const readDisk = (filepath: string) => Effect.promise(() => fs.readFile(filepath, "utf-8").catch(() => "")) + +describe("review mode tools", () => { + it.instance("edit stages without disk write", () => + Effect.gen(function* () { + enableReview() + const test = yield* TestInstance + const edit = yield* EditTool + const tool = yield* edit.init() + + const filepath = path.join(test.directory, "edit.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "before\n", "utf-8")) + + yield* tool.execute( + { + filePath: filepath, + oldString: "before", + newString: "after", + }, + baseCtx, + ) + + expect(yield* readDisk(filepath)).toBe("before\n") + const staged = ReviewOverlay.get(filepath) + expect(staged && "content" in staged ? staged.content : undefined).toBe("after\n") + }), + ) + + it.instance("write stages new files without disk write", () => + Effect.gen(function* () { + enableReview() + const test = yield* TestInstance + const write = yield* WriteTool + const tool = yield* write.init() + + const filepath = path.join(test.directory, "new.txt") + yield* tool.execute({ filePath: filepath, content: "created" }, baseCtx) + + expect(yield* readDisk(filepath)).toBe("") + expect(ReviewOverlay.get(filepath)).toEqual({ content: "created" }) + }), + ) + + it.instance("apply_patch stages add/update and deletes on disk", () => + Effect.gen(function* () { + enableReview() + const test = yield* TestInstance + const patch = yield* ApplyPatchTool + const tool = yield* patch.init() + + const updatePath = path.join(test.directory, "update.txt") + const deletePath = path.join(test.directory, "delete.txt") + yield* Effect.promise(() => fs.writeFile(updatePath, "old\n", "utf-8")) + yield* Effect.promise(() => fs.writeFile(deletePath, "gone\n", "utf-8")) + + const patchText = [ + "*** Begin Patch", + "*** Add File: added.txt", + "+added", + "*** Update File: update.txt", + "@@", + "-old", + "+new", + "*** Delete File: delete.txt", + "*** End Patch", + ].join("\n") + + const result = yield* tool.execute({ patchText }, baseCtx) + + expect(yield* readDisk(path.join(test.directory, "added.txt"))).toBe("") + expect(yield* readDisk(updatePath)).toBe("old\n") + expect(yield* readDisk(deletePath)).toBe("") + + expect(ReviewOverlay.get(path.join(test.directory, "added.txt"))).toEqual({ content: "added\n" }) + expect(ReviewOverlay.get(updatePath)).toEqual({ content: "new\n" }) + + // delete/move bypass staging (ACP has no deleteFile), so the output must + // warn that those changes already hit disk and won't be undone on reject. + expect(result.output).toContain("applied directly to disk") + + const content = completedToolContent("apply_patch", { + status: "completed", + input: { patchText }, + output: result.output, + metadata: result.metadata, + }) + expect(content.some((item) => item.type === "diff" && item.path === deletePath && item.newText === "")).toBe( + true, + ) + }), + ) +}) + +// Regression: tools resolved through the production ToolRegistry wiring must +// stage instead of writing to disk. The direct-tool tests above can pass even +// when ToolRegistry injects the plain FSUtil layer, so this exercises the real +// path that ACP prompts go through. +const registryIt = testEffect(Layer.mergeAll(ToolRegistry.defaultLayer, Agent.defaultLayer)) + +describe("review mode via ToolRegistry", () => { + registryIt.instance("edit staged through registry does not touch disk", () => + Effect.gen(function* () { + enableReview() + const test = yield* TestInstance + const agent = yield* Agent.Service + const build = yield* agent.get("build") + if (!build) throw new Error("build agent not found") + + const registry = yield* ToolRegistry.Service + const tools = yield* registry.tools({ + providerID: ProviderV2.ID.opencode, + modelID: ModelV2.ID.make("test"), + agent: build, + }) + const edit = tools.find((tool) => tool.id === "edit") + if (!edit) throw new Error("edit tool not registered") + + const filepath = path.join(test.directory, "registry-edit.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "before\n", "utf-8")) + + yield* edit.execute({ filePath: filepath, oldString: "before", newString: "after" }, baseCtx) + + expect(yield* readDisk(filepath)).toBe("before\n") + const staged = ReviewOverlay.get(filepath) + expect(staged && "content" in staged ? staged.content : undefined).toBe("after\n") + }), + ) +}) From b268f5056529685aeb6fc458b33aa615fe51b385 Mon Sep 17 00:00:00 2001 From: Paco Preciado Mendoza Date: Mon, 8 Jun 2026 14:44:09 -0600 Subject: [PATCH 4/4] fix(acp): replace removed log module with Effect logging Upstream dropped @opencode-ai/core/util/log; migrate ACP review paths to Effect.logInfo/logError so typecheck passes after the rebase. Co-authored-by: Cursor --- packages/opencode/src/acp/permission.ts | 10 ++++++++- packages/opencode/src/acp/review-staging.ts | 23 +++++++++------------ packages/opencode/src/acp/service.ts | 2 +- packages/opencode/src/cli/cmd/acp.ts | 2 +- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/packages/opencode/src/acp/permission.ts b/packages/opencode/src/acp/permission.ts index 418d752bb9f4..1545537d2a2f 100644 --- a/packages/opencode/src/acp/permission.ts +++ b/packages/opencode/src/acp/permission.ts @@ -81,7 +81,15 @@ export class Handler { // 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(() => {}) + 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) diff --git a/packages/opencode/src/acp/review-staging.ts b/packages/opencode/src/acp/review-staging.ts index 5783a549e48f..86c1d054874d 100644 --- a/packages/opencode/src/acp/review-staging.ts +++ b/packages/opencode/src/acp/review-staging.ts @@ -1,12 +1,10 @@ export * as ACPReviewStaging from "./review-staging" import type { AgentSideConnection } from "@agentclientprotocol/sdk" -import * as Log from "@opencode-ai/core/util/log" +import { Effect } from "effect" import { ReviewOverlay } from "@opencode-ai/core/review-overlay" import { isActive } from "./review-mode" -const log = Log.create({ service: "acp-review-staging" }) - type Connection = Partial> export async function flushPendingWrites(connection: Connection | undefined, sessionID?: string) { @@ -18,9 +16,6 @@ export async function flushPendingWrites(connection: Connection | undefined, ses } const pending = ReviewOverlay.drainPendingWrites() - if (pending.length > 0) { - log.info("flushing staged writes", { count: pending.length }) - } for (const item of pending) { await connection @@ -29,12 +24,14 @@ export async function flushPendingWrites(connection: Connection | undefined, ses path: item.path, content: item.content, }) - .catch((error: unknown) => { - log.error("failed to write staged edit through ACP", { - error, - path: item.path, - sessionID: item.sessionID, - }) - }) + .catch((error: unknown) => + Effect.runPromise( + Effect.logError("failed to write staged edit through ACP", { + error, + path: item.path, + sessionID: item.sessionID, + }), + ), + ) } } diff --git a/packages/opencode/src/acp/service.ts b/packages/opencode/src/acp/service.ts index 74642e6395fb..761e35ad38dc 100644 --- a/packages/opencode/src/acp/service.ts +++ b/packages/opencode/src/acp/service.ts @@ -114,7 +114,7 @@ export function make(input: { setClientWriteTextFileSupported(params.clientCapabilities?.fs?.writeTextFile === true) syncEnabled() if (ReviewOverlay.isEnabled()) { - log.info("ACP review-at-end active") + yield* Effect.logInfo("ACP review-at-end active") } const response = { diff --git a/packages/opencode/src/cli/cmd/acp.ts b/packages/opencode/src/cli/cmd/acp.ts index b5c067ba0783..93a02a91f39b 100644 --- a/packages/opencode/src/cli/cmd/acp.ts +++ b/packages/opencode/src/cli/cmd/acp.ts @@ -25,7 +25,7 @@ export const AcpCommand = effectCmd({ // Opt this ACP session into review mode. It still only activates if the // client reports the writeTextFile capability during initialize. forceEnableForAcp() - log.info("ACP review-at-end enabled") + yield* Effect.logInfo("ACP review-at-end enabled") const opts = yield* resolveNetworkOptions(args) const server = yield* Effect.promise(() => ACPProfile.measure("cli.acp.server.listen", () => Server.listen(opts)))