diff --git a/packages/app/src/utils/tool-call-parsers.test.ts b/packages/app/src/utils/tool-call-parsers.test.ts index c02f957a07..16c2f57f5b 100644 --- a/packages/app/src/utils/tool-call-parsers.test.ts +++ b/packages/app/src/utils/tool-call-parsers.test.ts @@ -21,6 +21,36 @@ describe("tool-call-parsers", () => { expect(parsed.find((entry) => entry.type === "add")?.content).toBe("+new"); }); + it("keeps file headers in multi-file diffs", () => { + const parsed = parseUnifiedDiff( + [ + "diff --git a/alpha.txt b/alpha.txt", + "--- a/alpha.txt", + "+++ b/alpha.txt", + "@@ -1 +1 @@", + "-color = blue", + "+color = green", + "diff --git a/beta.txt b/beta.txt", + "--- a/beta.txt", + "+++ b/beta.txt", + "@@ -1 +1 @@", + "-size = small", + "+size = large", + ].join("\n"), + ); + + expect(parsed).toEqual([ + { type: "header", content: "diff --git a/alpha.txt b/alpha.txt" }, + { type: "header", content: "@@ -1 +1 @@" }, + { type: "remove", content: "-color = blue" }, + { type: "add", content: "+color = green" }, + { type: "header", content: "diff --git a/beta.txt b/beta.txt" }, + { type: "header", content: "@@ -1 +1 @@" }, + { type: "remove", content: "-size = small" }, + { type: "add", content: "+size = large" }, + ]); + }); + it("extracts TodoWrite task entries", () => { const tasks = extractTaskEntriesFromToolCall("TodoWrite", { todos: [ diff --git a/packages/app/src/utils/tool-call-parsers.ts b/packages/app/src/utils/tool-call-parsers.ts index 1e89e2b303..7980d3ee5f 100644 --- a/packages/app/src/utils/tool-call-parsers.ts +++ b/packages/app/src/utils/tool-call-parsers.ts @@ -210,6 +210,8 @@ export function parseUnifiedDiff(diffText?: string): DiffLine[] { } const lines = splitIntoLines(diffText); + const fileHeaders = lines.filter((line) => line.startsWith("diff --git ")); + const hasMultipleFiles = fileHeaders.length > 1; const diff: DiffLine[] = []; for (const line of lines) { @@ -218,7 +220,8 @@ export function parseUnifiedDiff(diffText?: string): DiffLine[] { continue; } - if (line.startsWith("@@")) { + const isFileHeader = hasMultipleFiles && line.startsWith("diff --git "); + if (line.startsWith("@@") || isFileHeader) { diff.push({ type: "header", content: line }); continue; } diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts index 9b5263e474..bf453ad651 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts @@ -3436,6 +3436,87 @@ describe("Codex app-server provider", () => { } }); + test.each([ + ["patch_apply_begin", undefined, "running", null], + ["patch_apply_end", true, "completed", null], + ["patch_apply_end", false, "failed", { message: "Cannot apply patch" }], + ] as const)( + "maps both files from legacy %s notifications (success: %s)", + async (type, success, status, error) => { + const appServer = createFakeCodexAppServer(); + const session = new CodexAppServerAgentSession( + createConfig({ cwd: "/tmp/repo" }), + null, + createTestLogger(), + async () => appServer.child, + ); + try { + await session.connect(); + const nextTimelineItem = waitForNextTimelineItem(session); + appServer.child.stdout.write( + `${JSON.stringify({ + method: `codex/event/${type}`, + params: { + conversationId: "thread-1", + msg: { + type, + call_id: "patch-array-alias", + changes: [ + { + path: "/tmp/repo/src/array-alias.ts", + kind: "modify", + unified_diff: "@@\n-old\n+new\n", + }, + { + path: "/tmp/repo/src/second.ts", + kind: "modify", + unified_diff: "@@\n-before\n+after\n", + }, + ], + success, + stderr: "Cannot apply patch", + }, + }, + })}\n`, + ); + + await expect(nextTimelineItem).resolves.toEqual({ + type: "timeline", + provider: "codex", + item: { + type: "tool_call", + callId: "patch-array-alias", + name: "apply_patch", + status, + error, + detail: { + type: "edit", + filePath: "src/array-alias.ts", + unifiedDiff: [ + "diff --git a/src/array-alias.ts b/src/array-alias.ts", + "--- a/src/array-alias.ts", + "+++ b/src/array-alias.ts", + "@@", + "-old", + "+new", + "", + "diff --git a/src/second.ts b/src/second.ts", + "--- a/src/second.ts", + "+++ b/src/second.ts", + "@@", + "-before", + "+after", + ].join("\n"), + }, + }, + }); + appServer.assertNoErrors(); + } finally { + await session.close(); + } + }, + ); + test("maps Codex plan markdown to a synthetic plan tool call", () => { const item = mapCodexPlanToToolCall({ callId: "plan-turn-1", @@ -7879,3 +7960,93 @@ test("publishes a materialized image before dispatching the following turn compl await session.close(); } }); + +describe("legacy multi-file diff budget", () => { + test.each([true, false])( + "retains later file diffs when a large patch is running: %s", + (running) => { + const item = mapCodexPatchNotificationToToolCall({ + callId: "large-legacy-patch", + cwd: "/repo", + running, + success: true, + changes: [ + { + path: "/repo/alpha.txt", + kind: "modify", + unified_diff: "@@ -1 +1 @@\n-old\n+" + "a".repeat(14000), + }, + { path: "/repo/beta.txt", kind: "modify", unified_diff: "@@ -1 +1 @@\n-small\n+large\n" }, + ], + }); + expect(item?.detail.type).toBe("edit"); + if (item?.detail.type !== "edit") throw new Error("Expected edit detail"); + expect(item.detail.unifiedDiff).toContain("diff --git a/beta.txt b/beta.txt"); + expect(item.detail.unifiedDiff).toContain("+large"); + expect(item.detail.unifiedDiff!.length).toBeLessThanOrEqual(12000); + }, + ); +}); + +describe("legacy multi-file move metadata", () => { + test.each([true, false])("preserves path-keyed moves while running: %s", (running) => { + const item = mapCodexPatchNotificationToToolCall({ + callId: "legacy-move", + cwd: "/repo", + running, + success: true, + changes: { + "/repo/old.txt": { + type: "update", + move_path: "/repo/new.txt", + unified_diff: "@@ -1 +1 @@\n-old\n+new\n", + }, + "/repo/other.txt": { type: "update", unified_diff: "@@ -1 +1 @@\n-before\n+after\n" }, + }, + }); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain("diff --git a/old.txt b/new.txt"); + expect(item.detail.unifiedDiff).toContain("+++ b/new.txt"); + }); +}); + +describe("legacy move-only entries", () => { + test.each([true, false])( + "keeps a move-only file alongside another edit while running: %s", + (running) => { + const item = mapCodexPatchNotificationToToolCall({ + callId: "legacy-move-only", + cwd: "/repo", + running, + success: true, + changes: { + "/repo/old.txt": { type: "update", move_path: "/repo/new.txt", unified_diff: "" }, + "/repo/second.txt": { type: "update", unified_diff: "@@ -1 +1 @@\n-small\n+large\n" }, + }, + }); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain("rename from old.txt\nrename to new.txt"); + expect(item.detail.unifiedDiff).toContain("diff --git a/second.txt b/second.txt"); + }, + ); +}); + +describe("legacy empty additions", () => { + test.each([true, false])("keeps an empty new file while running: %s", (running) => { + const item = mapCodexPatchNotificationToToolCall({ + callId: "legacy-empty", + cwd: "/repo", + running, + success: true, + changes: { + "/repo/empty.txt": { type: "add", content: "" }, + "/repo/second.txt": { type: "update", unified_diff: "@@ -1 +1 @@\n-old\n+new\n" }, + }, + }); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain( + "diff --git a/empty.txt b/empty.txt\n--- /dev/null\n+++ b/empty.txt", + ); + expect(item.detail.unifiedDiff).toContain("diff --git a/second.txt b/second.txt"); + }); +}); diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index f349978996..bf797949c7 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -55,11 +55,13 @@ import { renderPromptAttachmentAsText } from "../prompt-attachments.js"; import { composeSystemPromptParts } from "../system-prompt.js"; import { CodexAsyncQuestions, codexAsyncQuestionToTimeline } from "./codex/async-questions.js"; import { + fileChangesToPreview, mapCodexToolCallEnvelope, mapCodexToolCallFromThreadItem, normalizeCommandExecutionCommand, splitCodexMcpToolResultImages, } from "./codex/tool-call-mapper.js"; +import { normalizeCodexFilePath } from "./codex/tool-call-detail-parser.js"; import { checkProviderLaunchAvailable, createProviderEnv, @@ -1402,6 +1404,15 @@ interface CodexPatchFileChange { path: string; kind?: string; content?: string; + movePath?: string; +} + +function codexPatchMovePath(value: unknown): string | undefined { + const record = toObjectRecord(value); + if (!record) return undefined; + const kind = toObjectRecord(record.kind); + const destination = record.move_path ?? kind?.move_path; + return typeof destination === "string" ? destination : undefined; } function extractPatchLikeText(value: unknown): string | undefined { @@ -1534,6 +1545,7 @@ function parseCodexPatchChanges(changes: unknown): CodexPatchFileChange[] { (typeof record.type === "string" && record.type) || undefined, content: extractPatchLikeText(record), + movePath: codexPatchMovePath(record), }; }) .filter((entry): entry is CodexPatchFileChange => entry !== null); @@ -1553,6 +1565,7 @@ function parseCodexPatchChanges(changes: unknown): CodexPatchFileChange[] { (typeof recordChanges.type === "string" && recordChanges.type) || undefined, content: extractPatchLikeText(recordChanges), + movePath: codexPatchMovePath(recordChanges), }, ]; } @@ -1572,6 +1585,7 @@ function parseCodexPatchChanges(changes: unknown): CodexPatchFileChange[] { ? ((value as { type?: string }).type ?? undefined) : undefined, content: extractPatchLikeText(value), + movePath: codexPatchMovePath(value), }; }) .filter((entry): entry is CodexPatchFileChange => entry !== null); @@ -1680,6 +1694,31 @@ function mapCodexExecNotificationToToolCall(params: { return params.running ? toRunningToolCall(mapped) : mapped; } +function codexPatchPreview( + files: CodexPatchFileChange[], + cwd?: string | null, +): { + filePath: string | undefined; + patchText: string | undefined; +} { + const preview = + files.length > 1 + ? fileChangesToPreview( + files.map((file) => ({ + path: normalizeCodexFilePath(file.path, cwd) ?? file.path, + kind: file.kind, + diff: file.content, + movePath: file.movePath ? normalizeCodexFilePath(file.movePath, cwd) : undefined, + })), + ) + : undefined; + const firstPath = preview?.filePath ?? files[0]?.path; + const firstPatchText = files + .map((file) => file.content?.trim()) + .find((value): value is string => typeof value === "string" && value.length > 0); + return { filePath: firstPath, patchText: preview?.unifiedDiff ?? firstPatchText }; +} + export function mapCodexPatchNotificationToToolCall(params: { callId?: string | null; changes: unknown; @@ -1690,11 +1729,7 @@ export function mapCodexPatchNotificationToToolCall(params: { running: boolean; }): ToolCallTimelineItem | null { const files = parseCodexPatchChanges(params.changes); - const firstPath = files[0]?.path; - const firstPatchText = files - .map((file) => file.content?.trim()) - .find((value): value is string => typeof value === "string" && value.length > 0); - const patchText = firstPatchText; + const { filePath: firstPath, patchText } = codexPatchPreview(files, params.cwd); const patchFields = codexPatchTextFields(patchText); const mapped = mapCodexToolCallEnvelope({ callId: params.callId ?? null, diff --git a/packages/server/src/server/agent/providers/codex/tool-call-mapper.test.ts b/packages/server/src/server/agent/providers/codex/tool-call-mapper.test.ts index 566fd10083..423aa08c76 100644 --- a/packages/server/src/server/agent/providers/codex/tool-call-mapper.test.ts +++ b/packages/server/src/server/agent/providers/codex/tool-call-mapper.test.ts @@ -528,6 +528,165 @@ describe("codex tool-call mapper", () => { }); }); + it.each([ + ["inProgress", "running"], + ["completed", "completed"], + ])("keeps both fileChange diffs when the patch is %s", (status, expectedStatus) => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-two-files", + status, + changes: [ + { + path: "/tmp/repo/alpha.txt", + kind: { type: "update", move_path: null }, + diff: "@@ -1 +1 @@\n-color = blue\n+color = green\n", + }, + { + path: "/tmp/repo/beta.txt", + kind: { type: "update", move_path: null }, + diff: "@@ -1 +1 @@\n-size = small\n+size = large\n", + }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toEqual({ + type: "tool_call", + callId: "codex-two-files", + name: "apply_patch", + status: expectedStatus, + error: null, + detail: { + type: "edit", + filePath: "alpha.txt", + unifiedDiff: [ + "diff --git a/alpha.txt b/alpha.txt", + "--- a/alpha.txt", + "+++ b/alpha.txt", + "@@ -1 +1 @@", + "-color = blue", + "+color = green", + "", + "diff --git a/beta.txt b/beta.txt", + "--- a/beta.txt", + "+++ b/beta.txt", + "@@ -1 +1 @@", + "-size = small", + "+size = large", + ].join("\n"), + }, + }); + }); + + it("uses the move destination in generated multi-file diff headers", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-moved-file", + status: "completed", + changes: [ + { + path: "/tmp/repo/old.txt", + kind: { type: "update", move_path: "/tmp/repo/new.txt" }, + diff: "@@ -1 +1 @@\n-old\n+new\n", + }, + { + path: "/tmp/repo/other.txt", + kind: { type: "update", move_path: null }, + diff: "@@ -1 +1 @@\n-before\n+after\n", + }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toEqual({ + type: "tool_call", + callId: "codex-moved-file", + name: "apply_patch", + status: "completed", + error: null, + detail: { + type: "edit", + filePath: "old.txt", + unifiedDiff: [ + "diff --git a/old.txt b/new.txt", + "--- a/old.txt", + "+++ b/new.txt", + "@@ -1 +1 @@", + "-old", + "+new", + "", + "diff --git a/other.txt b/other.txt", + "--- a/other.txt", + "+++ b/other.txt", + "@@ -1 +1 @@", + "-before", + "+after", + ].join("\n"), + }, + }); + }); + + it("keeps update, add, and delete changes in one fileChange diff", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-mixed-files", + status: "completed", + changes: [ + { + path: "/tmp/repo/alpha.txt", + kind: { type: "update", move_path: null }, + diff: "--- a/alpha.txt\n+++ b/alpha.txt\n@@ -1 +1 @@\n-blue\n+green\n", + }, + { path: "/tmp/repo/new.txt", kind: { type: "add" }, diff: "new\n\nvalue\n" }, + { path: "/tmp/repo/old.txt", kind: { type: "delete" }, diff: "old\n\nvalue\n" }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toEqual({ + type: "tool_call", + callId: "codex-mixed-files", + name: "apply_patch", + status: "completed", + error: null, + detail: { + type: "edit", + filePath: "alpha.txt", + unifiedDiff: [ + "diff --git a/alpha.txt b/alpha.txt", + "--- a/alpha.txt", + "+++ b/alpha.txt", + "@@ -1 +1 @@", + "-blue", + "+green", + "", + "diff --git a/new.txt b/new.txt", + "--- /dev/null", + "+++ b/new.txt", + "@@ -0,0 +1,3 @@", + "+new", + "+", + "+value", + "", + "diff --git a/old.txt b/old.txt", + "--- a/old.txt", + "+++ /dev/null", + "@@ -1,3 +0,0 @@", + "-old", + "-", + "-value", + ].join("\n"), + }, + }); + }); + it("maps fileChange object-style change payloads keyed by path", () => { const item = expectMapped( mapCodexToolCallFromThreadItem( @@ -827,6 +986,44 @@ describe("codex tool-call mapper", () => { }); }); + it("maps multi-file path-only updates to unknown detail", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-multi-file-path-only", + status: "completed", + changes: [ + { path: "/tmp/repo/first.ts", kind: "modify" }, + { path: "/tmp/repo/second.ts", kind: { type: "update", move_path: null } }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toEqual({ + type: "tool_call", + callId: "codex-multi-file-path-only", + name: "apply_patch", + status: "completed", + error: null, + detail: { + type: "unknown", + input: { + files: [ + { path: "first.ts", kind: "modify" }, + { path: "second.ts", kind: "update" }, + ], + }, + output: { + files: [ + { path: "first.ts", kind: "modify" }, + { path: "second.ts", kind: "update" }, + ], + }, + }, + }); + }); + it("maps path-only apply_patch tool-call payloads to unknown detail instead of empty edit detail", () => { const item = expectMapped( mapCodexToolCallEnvelope({ @@ -1102,3 +1299,186 @@ describe("codex tool-call mapper", () => { `); }); }); + +describe("multi-file diff budget", () => { + it("retains a labeled diff for later files when the first diff exceeds the detail limit", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "large-multi-file", + status: "completed", + changes: [ + { + path: "/repo/alpha.txt", + kind: "modify", + diff: "@@ -1 +1 @@\n-old\n+" + "a".repeat(14000), + }, + { path: "/repo/beta.txt", kind: "modify", diff: "@@ -1 +1 @@\n-small\n+large\n" }, + ], + }, + { cwd: "/repo" }, + ); + expect(item?.detail.type).toBe("edit"); + if (item?.detail.type !== "edit") throw new Error("Expected edit detail"); + expect(item.detail.unifiedDiff).toContain("diff --git a/alpha.txt b/alpha.txt"); + expect(item.detail.unifiedDiff).toContain("diff --git a/beta.txt b/beta.txt"); + expect(item.detail.unifiedDiff).toContain("+large"); + expect(item.detail.unifiedDiff!.length).toBeLessThanOrEqual(12000); + }); +}); + +describe("multi-file preview metadata", () => { + it("preserves a path-keyed move destination", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "moved-keyed", + status: "completed", + changes: { + "/repo/old.txt": { + type: "update", + move_path: "/repo/new.txt", + unified_diff: "@@ -1 +1 @@\n-old\n+new\n", + }, + "/repo/other.txt": { type: "update", unified_diff: "@@ -1 +1 @@\n-before\n+after\n" }, + }, + }, + { cwd: "/repo" }, + ); + expect(item?.detail.type).toBe("edit"); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain("diff --git a/old.txt b/new.txt"); + expect(item.detail.unifiedDiff).toContain("+++ b/new.txt"); + }); + it("names the file whose diff is shown when the first entry has no content", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "partial-content", + status: "completed", + changes: [ + { path: "/repo/first.txt", kind: "modify" }, + { path: "/repo/second.txt", kind: "modify", diff: "@@ -1 +1 @@\n-old\n+new\n" }, + ], + }, + { cwd: "/repo" }, + ); + expect(item?.detail).toMatchObject({ type: "edit", filePath: "second.txt" }); + }); + it("keeps whole labels and marks omitted files when many long paths exceed the preview", () => { + const paths = Array.from( + { length: 50 }, + (_, index) => `src/${String(index).padStart(2, "0")}-${"x".repeat(108)}.txt`, + ); + const item = mapCodexToolCallFromThreadItem({ + type: "fileChange", + id: "many-files", + status: "completed", + changes: paths.map((path) => ({ path, kind: "modify", diff: "@@ -1 +1 @@\n-old\n+new\n" })), + }); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + const diff = item.detail.unifiedDiff!; + const headers = diff.split("\n").filter((line) => line.startsWith("diff --git ")); + expect(headers.length).toBeGreaterThan(0); + const expectedHeaders = paths.map((path) => `diff --git a/${path} b/${path}`); + expect(expectedHeaders).toEqual(expect.arrayContaining(headers)); + expect(diff).toContain("-old\n+new"); + expect(diff).toContain(`${paths.length - headers.length} more files omitted`); + expect(diff.length).toBeLessThanOrEqual(12000); + }); +}); + +describe("large multi-file additions", () => { + it("bounds a generated addition without throwing before later files are mapped", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "large-addition", + status: "completed", + changes: [ + { path: "/repo/generated.txt", kind: { type: "add" }, diff: "x\n".repeat(150000) }, + { path: "/repo/second.txt", kind: { type: "add" }, diff: "second\n" }, + ], + }, + { cwd: "/repo" }, + ); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.status).toBe("completed"); + expect(item.detail.unifiedDiff).toContain("diff --git a/generated.txt b/generated.txt"); + expect(item.detail.unifiedDiff).toContain("diff --git a/second.txt b/second.txt"); + expect(item.detail.unifiedDiff).toContain("+second"); + expect(item.detail.unifiedDiff!.length).toBeLessThanOrEqual(12000); + }); +}); + +describe("move-only entries in multi-file previews", () => { + it("shows a move without manufacturing added file contents from provider metadata", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "move-only", + status: "completed", + changes: [ + { + path: "/repo/old.txt", + kind: { type: "update", move_path: "/repo/new.txt" }, + diff: "\n\nMoved to: /repo/new.txt", + }, + { path: "/repo/second.txt", kind: "modify", diff: "@@ -1 +1 @@\n-small\n+large\n" }, + ], + }, + { cwd: "/repo" }, + ); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain("rename from old.txt\nrename to new.txt"); + expect(item.detail.unifiedDiff).toContain("diff --git a/second.txt b/second.txt"); + expect(item.detail.unifiedDiff).not.toContain("+Moved to:"); + expect(item.detail.unifiedDiff).not.toContain("@@ -0,0"); + }); +}); + +describe("single-file move compatibility", () => { + it("retains the destination outside the multi-file preview path", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "single-move", + status: "completed", + changes: [ + { + path: "/repo/old.txt", + kind: { type: "update", move_path: "/repo/new.txt" }, + diff: "\n\nMoved to: /repo/new.txt", + }, + ], + }, + { cwd: "/repo" }, + ); + expect(JSON.stringify(item)).toContain("new.txt"); + }); +}); + +describe("empty additions in multi-file previews", () => { + it.each(["inProgress", "completed"])( + "keeps an empty created file when the patch is %s", + (status) => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "empty-addition", + status, + changes: [ + { path: "/repo/empty.txt", kind: { type: "add" }, diff: "" }, + { path: "/repo/second.txt", kind: "modify", diff: "@@ -1 +1 @@\n-old\n+new\n" }, + ], + }, + { cwd: "/repo" }, + ); + if (item?.detail.type !== "edit") throw new Error("Expected edit"); + expect(item.detail.unifiedDiff).toContain( + "diff --git a/empty.txt b/empty.txt\n--- /dev/null\n+++ b/empty.txt", + ); + expect(item.detail.unifiedDiff).toContain("diff --git a/second.txt b/second.txt"); + }, + ); +}); diff --git a/packages/server/src/server/agent/providers/codex/tool-call-mapper.ts b/packages/server/src/server/agent/providers/codex/tool-call-mapper.ts index 4e77dc9367..8c0f8b9558 100644 --- a/packages/server/src/server/agent/providers/codex/tool-call-mapper.ts +++ b/packages/server/src/server/agent/providers/codex/tool-call-mapper.ts @@ -4,6 +4,7 @@ import type { ToolCallTimelineItem } from "../../agent-sdk-types.js"; import { extractCodexShellOutput, normalizeToolCallStatus, + TOOL_CALL_DIFF_MAX_CHARS, truncateDiffText, } from "../tool-call-mapper-utils.js"; import { deriveCodexToolDetail, normalizeCodexFilePath } from "./tool-call-detail-parser.js"; @@ -400,7 +401,7 @@ function contentToDeletionDiff(filePath: string, content: string): string { output.push(`diff --git a/${filePath} b/${filePath}`); output.push(`--- a/${filePath}`); output.push(`+++ /dev/null`); - const nonEmpty = lines.filter((l) => l.length > 0 || lines.indexOf(l) < lines.length - 1); + const nonEmpty = lines.slice(0, lines.at(-1) === "" ? -1 : undefined); if (nonEmpty.length > 0) { output.push(`@@ -1,${nonEmpty.length} +0,0 @@`); for (const line of nonEmpty) { @@ -713,6 +714,7 @@ interface CodexFileChangeEntry { path: string; kind?: string; diff?: string; + movePath?: string; } function parseFileChangePath( @@ -740,6 +742,9 @@ function parseFileChangePath( } function parseFileChangeKind(entry: Record): string | undefined { + if (isRecord(entry.kind) && typeof entry.kind.type === "string") { + return entry.kind.type; + } return ( (typeof entry.kind === "string" && entry.kind) || (typeof entry.type === "string" && entry.type) || @@ -767,10 +772,14 @@ function toFileChangeEntry( if (!path) { return null; } + const kind = isRecord(entry.kind) ? entry.kind : undefined; + const movePath = entry.move_path ?? kind?.move_path; return { path, kind: parseFileChangeKind(entry), diff: parseFileChangeDiff(entry), + movePath: + typeof movePath === "string" ? normalizeCodexFilePath(movePath, options?.cwd) : undefined, }; } @@ -839,6 +848,105 @@ function resolveFileChangeTextFields(file: CodexFileChangeEntry | undefined): { return asEditTextFields(file.diff); } +function fileChangeToUnifiedDiff(file: CodexFileChangeEntry): string { + // Codex formats a content-preserving move as a notice, not added file text. + const isMoveOnlyNotice = file.movePath && file.diff?.startsWith("\n\nMoved to: "); + const { unifiedDiff, newString } = isMoveOnlyNotice ? {} : resolveFileChangeTextFields(file); + if (!unifiedDiff && !newString) { + if (file.kind === "add") { + return [ + `diff --git a/${file.path} b/${file.path}`, + "--- /dev/null", + `+++ b/${file.path}`, + ].join("\n"); + } + return file.movePath + ? [ + `diff --git a/${file.path} b/${file.movePath}`, + `rename from ${file.path}`, + `rename to ${file.movePath}`, + ].join("\n") + : ""; + } + if (unifiedDiff?.startsWith("diff --git")) { + return unifiedDiff.replace(/\r?\n$/, ""); + } + const destination = file.movePath ?? file.path; + const header = `diff --git a/${file.path} b/${destination}`; + if (unifiedDiff?.startsWith("--- ")) { + return `${header}\n${unifiedDiff.replace(/\r?\n$/, "")}`; + } + const left = file.kind === "add" ? "/dev/null" : `a/${file.path}`; + const right = file.kind === "delete" ? "/dev/null" : `b/${destination}`; + const lines = [header, `--- ${left}`, `+++ ${right}`]; + if (unifiedDiff) { + lines.push(unifiedDiff.replace(/\r?\n$/, "")); + } else if (newString) { + const contentLines = newString.replace(/\r\n/g, "\n").replace(/\n$/, "").split("\n"); + lines.push(`@@ -0,0 +1,${contentLines.length} @@`); + for (const line of contentLines) lines.push(`+${line}`); + } + return lines.join("\n"); +} + +function splitFileDiffPreview(diff: string): { header: string; body: string } { + const hunk = diff.indexOf("\n@@"); + const bodyStart = hunk < 0 ? -1 : diff.indexOf("\n", hunk + 1); + return bodyStart < 0 + ? { header: diff, body: "" } + : { header: diff.slice(0, bodyStart + 1), body: diff.slice(bodyStart + 1) }; +} + +export function fileChangesToPreview(files: CodexFileChangeEntry[]): { + filePath: string | undefined; + unifiedDiff: string; +} { + const diffs = files + .map((file) => ({ filePath: file.path, diff: fileChangeToUnifiedDiff(file) })) + .filter((file) => file.diff.length > 0); + const filePath = diffs[0]?.filePath ?? files[0]?.path; + const combined = diffs.map((file) => file.diff).join("\n\n"); + if (combined.length <= TOOL_CALL_DIFF_MAX_CHARS) return { filePath, unifiedDiff: combined }; + + const marker = "\n...[truncated]"; + const omittedNotice = (count: number) => + count > 0 ? `\n\n...[${count} more files omitted]` : ""; + const previews = diffs.map((file) => splitFileDiffPreview(file.diff)); + const visible: typeof previews = []; + let minimumSize = 0; + for (const preview of previews) { + // Keep whole labels and a useful body excerpt, or explicitly omit the file. + const required = + preview.header.length + Math.max(marker.length, Math.min(80, preview.body.length)); + const nextSize = minimumSize + required + (visible.length > 0 ? 2 : 0); + if ( + nextSize + omittedNotice(previews.length - visible.length - 1).length > + TOOL_CALL_DIFF_MAX_CHARS + ) + break; + visible.push(preview); + minimumSize = nextSize; + } + const omitted = omittedNotice(previews.length - visible.length); + const headersSize = visible.reduce((size, preview) => size + preview.header.length, 0); + const bodyBudget = Math.floor( + (TOOL_CALL_DIFF_MAX_CHARS - + headersSize - + Math.max(0, visible.length - 1) * 2 - + omitted.length) / + Math.max(1, visible.length), + ); + const unifiedDiff = + visible + .map( + ({ header, body }) => + header + + (body.length <= bodyBudget ? body : body.slice(0, bodyBudget - marker.length) + marker), + ) + .join("\n\n") + omitted; + return { filePath, unifiedDiff }; +} + function mapFileChangeItem( item: z.infer, options?: CodexMapperOptions, @@ -873,17 +981,17 @@ function mapFileChangeItem( const error = item.error ?? null; const status = normalizeToolCallStatus(item.status, error, output); const firstFile = files[0]; - const firstTextFields = resolveFileChangeTextFields(firstFile); - const hasFirstTextFields = Object.keys(firstTextFields).length > 0; + const preview = files.length > 1 ? fileChangesToPreview(files) : undefined; + const filePath = preview?.filePath ?? firstFile?.path; + const textFields = preview + ? { unifiedDiff: preview.unifiedDiff } + : resolveFileChangeTextFields(firstFile); + const hasTextFields = Boolean(textFields.unifiedDiff || textFields.newString); const input = toNullableObject({ ...inputBase, - ...(firstFile?.path && hasFirstTextFields ? { path: firstFile.path } : {}), - ...(hasFirstTextFields && firstTextFields.unifiedDiff - ? { patch: firstTextFields.unifiedDiff } - : {}), - ...(hasFirstTextFields && firstTextFields.newString - ? { content: firstTextFields.newString } - : {}), + ...(filePath && hasTextFields ? { path: filePath } : {}), + ...(hasTextFields && textFields.unifiedDiff ? { patch: textFields.unifiedDiff } : {}), + ...(hasTextFields && textFields.newString ? { content: textFields.newString } : {}), }); return { diff --git a/packages/server/src/server/agent/providers/tool-call-mapper-utils.ts b/packages/server/src/server/agent/providers/tool-call-mapper-utils.ts index cd0e4776b7..704e2b5d32 100644 --- a/packages/server/src/server/agent/providers/tool-call-mapper-utils.ts +++ b/packages/server/src/server/agent/providers/tool-call-mapper-utils.ts @@ -224,9 +224,11 @@ export function stripReadLineNumberGutter( return { content: stripped.join("\n"), startLine }; } +export const TOOL_CALL_DIFF_MAX_CHARS = 12_000; + export function truncateDiffText( text: string | undefined, - maxChars: number = 12_000, + maxChars: number = TOOL_CALL_DIFF_MAX_CHARS, ): string | undefined { if (typeof text !== "string") { return undefined;