From e1d0d090c9de800a757812121ccc5bfcd29f88e7 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Thu, 24 Sep 2026 23:13:59 -0700 Subject: [PATCH] fix(server): show diffs for ACP edits that send oldText/newText ACP v1 tool-call diff content is { type: "diff", path, oldText, newText }; v2 replaced it with { changes, patch }. The ACP adapter read only the v2 patch.text, so file changes from agents that negotiate v1 (Grok answers initialize with protocolVersion 1) reached the timeline with no diff. Build the patch from oldText/newText with the server's existing jsdiff dependency (/dev/null for a new file), keeping v2 patch text as sent. The Grok read-only on-request replay now requires the file_change to carry what it wrote. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Adapters/AcpAdapterV2.test.ts | 52 +++++++++++++++++++ .../orchestration-v2/Adapters/AcpAdapterV2.ts | 46 +++++++++++++--- .../tool_call_read_only_on_request/output.ts | 6 +-- 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts index 644c68b38d6d..7226f0a30e89 100644 --- a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts +++ b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts @@ -82,6 +82,7 @@ import { acpPostSettleWakeEvidence, acpPostSettleWakeShouldBuffer, acpProjectedCommandExitCode, + acpToolCallDiffPatch, acpTurnStartShouldPreserveContinuation, makeAcpAdapterV2, type AcpAdapterV2ExtensionContext, @@ -122,6 +123,57 @@ describe("acpProjectedCommandExitCode", () => { }); }); +describe("acpToolCallDiffPatch", () => { + it("builds a patch per file from ACP v1 oldText/newText, with /dev/null for a new file", () => { + assert.equal( + acpToolCallDiffPatch([ + { type: "diff", path: "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/repo/new.txt", oldText: null, newText: "hello\n" }, + { type: "diff", path: "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/repo/a.ts", oldText: "a\nb\nc\n", newText: "a\nB\nc\n" }, + { type: "diff", path: "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/repo/same.ts", oldText: "x\n", newText: "x\n" }, + ]), + [ + "--- /dev/null", + "+++ /repo/new.txt", + "@@ -0,0 +1,1 @@", + "+hello", + "", + "--- /repo/a.ts", + "+++ /repo/a.ts", + "@@ -1,3 +1,3 @@", + " a", + "-b", + "+B", + " c", + "", + ].join("\n"), + ); + }); + + it("keeps the ACP v2 patch text as sent", () => { + const text = "diff --git a/repo/a.ts b/repo/a.ts\n"; + assert.equal( + acpToolCallDiffPatch([ + { + type: "diff", + changes: [{ operation: "modify", path: "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/repo/a.ts" }], + patch: { format: "git_patch", text }, + }, + ]), + text, + ); + }); + + it("drops the patch for a rewrite too large to diff cheaply", () => { + const lines = (prefix: string) => + Array.from({ length: 2_000 }, (_, index) => `${prefix} ${index}`).join("\n"); + assert.isUndefined( + acpToolCallDiffPatch([ + { type: "diff", path: "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/repo/big.ts", oldText: lines("old"), newText: lines("new") }, + ]), + ); + }); +}); + describe("ACP continuation ownership", () => { it("preserves a continuation offered during non-buffered carryover handling", () => { assert.isFalse( diff --git a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts index 14571cc3b98e..017f8f2b827d 100644 --- a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts +++ b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts @@ -33,6 +33,7 @@ import { } from "@t3tools/contracts"; import { modelSelectionsEqual } from "@t3tools/shared/model"; import { type SelfInvocation, selfInvocationArgs } from "@t3tools/shared/nodeRuntime"; +import { FILE_HEADERS_ONLY, formatPatch, structuredPatch } from "diff"; import * as Cause from "effect/Cause"; import * as Crypto from "effect/Crypto"; import * as DateTime from "effect/DateTime"; @@ -872,15 +873,45 @@ function structuredFileChanges(toolCall: AcpToolCallState) { }); } -function structuredDiffPatch(toolCall: AcpToolCallState): string | undefined { - const content = toolCall.data.content; +// Past this edit distance an edit keeps no patch text, so projecting a large +// rewrite cannot stall the event loop in the diff search. A created or emptied +// file has one empty side and needs no search, so it is never capped. +const ACP_V1_DIFF_MAX_EDITS = 1_000; + +/** + * Patch text for a tool call's diff content. ACP v2 diffs carry it as + * `patch.text`. ACP v1 diffs carry `oldText`/`newText` instead (`oldText` + * null or absent for a new file), and agents that negotiate v1 still send + * that shape, so the patch is built from the two sides. + */ +export function acpToolCallDiffPatch(content: unknown): string | undefined { if (!Array.isArray(content)) return undefined; - for (const entry of content) { + const diffs = content.flatMap((entry) => { const diff = unknownRecord(entry); - const patch = unknownRecord(diff?.patch); - if (diff?.type === "diff" && typeof patch?.text === "string") return patch.text; + return diff?.type === "diff" ? [diff] : []; + }); + for (const diff of diffs) { + const patch = unknownRecord(diff.patch); + if (typeof patch?.text === "string") return patch.text; } - return undefined; + const v1Patches = diffs.flatMap((diff) => { + if (typeof diff.path !== "string" || typeof diff.newText !== "string") return []; + const oldText = typeof diff.oldText === "string" ? diff.oldText : undefined; + const patch = structuredPatch( + oldText === undefined ? "/dev/null" : diff.path, + diff.path, + oldText ?? "", + diff.newText, + undefined, + undefined, + { + context: 3, + maxEditLength: oldText && diff.newText ? ACP_V1_DIFF_MAX_EDITS : Number.POSITIVE_INFINITY, + }, + ); + return patch === undefined || patch.hunks.length === 0 ? [] : [patch]; + }); + return v1Patches.length === 0 ? undefined : formatPatch(v1Patches, FILE_HEADERS_ONLY); } function pathFromToolCall(toolCall: AcpToolCallState): string | undefined { @@ -3070,7 +3101,8 @@ export function makeAcpAdapterV2(options: AcpAdapterV2Options): ProviderAdapterV const rawOutput = toolCall.data.rawOutput ?? toolCall.data.content; const changes = structuredFileChanges(toolCall); const path = changes[0]?.path ?? pathFromToolCall(toolCall); - const diffText = structuredDiffPatch(toolCall) ?? textFromUnknown(rawOutput); + const diffText = + acpToolCallDiffPatch(toolCall.data.content) ?? textFromUnknown(rawOutput); const rawInputRecord = unknownRecord(rawInput); const inputVariant = typeof rawInputRecord?.variant === "string" diff --git a/apps/server/src/orchestration-v2/testkit/fixtures/tool_call_read_only_on_request/output.ts b/apps/server/src/orchestration-v2/testkit/fixtures/tool_call_read_only_on_request/output.ts index 7da74cc401c2..e6d580004d1b 100644 --- a/apps/server/src/orchestration-v2/testkit/fixtures/tool_call_read_only_on_request/output.ts +++ b/apps/server/src/orchestration-v2/testkit/fixtures/tool_call_read_only_on_request/output.ts @@ -56,11 +56,11 @@ export function assertToolCallReadOnlyOnRequestOutput( writes.some((item) => item.status === "completed"), "the approved write must complete", ); - // A file_change projected from an ACP v1 diff ({ oldText, newText }) carries - // no content today; the adapter reads only the v2 patch form. + // Grok's write reports an ACP v1 diff ({ path, oldText, newText }); the + // file_change must still carry it, like the v2 patch form. for (const item of writes) { const content = writtenContent(item); - if (content === undefined) continue; + assert.isDefined(content, `the approved ${item.type} must carry what it wrote`); assert.include(content, PROBE_CONTENT, "the approved write must carry the requested content"); } }