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"); } }