From a2aac3797f4a231881a9cfbce28731f97f83fd2e Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 26 Mar 2026 17:53:57 +0700 Subject: [PATCH] fix(server): translate Codex file deletions as removed lines instead of added lines Codex delete operations were displayed as edits with green (added) lines because the translation pipeline didn't distinguish deletes from edits. Now detects kind="delete" and *** Delete File directives, producing proper unified diffs with removed lines and +++ /dev/null headers. --- .../providers/codex/tool-call-mapper.test.ts | 123 ++++++++++++++++++ .../agent/providers/codex/tool-call-mapper.ts | 53 +++++++- 2 files changed, 169 insertions(+), 7 deletions(-) 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 98d5d53c9..65c4144d0 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 @@ -579,4 +579,127 @@ describe("codex tool-call mapper", () => { expect(item).toBeNull(); }); + + it("maps apply_patch with Delete File directive into edit detail with removed lines", () => { + const patch = [ + "*** Begin Patch", + "*** Delete File: /tmp/repo/src/dead-module.ts", + "*** End Patch", + ].join("\n"); + const item = expectMapped( + mapCodexRolloutToolCall({ + callId: "codex-delete-rollout", + name: "apply_patch", + input: patch, + output: + '{"output":"Success. Updated the following files:\\nD /tmp/repo/src/dead-module.ts\\n"}', + cwd: "/tmp/repo", + }), + ); + + expect(item.status).toBe("completed"); + expect(item.detail.type).toBe("edit"); + if (item.detail.type === "edit") { + expect(item.detail.filePath).toBe("src/dead-module.ts"); + expect(item.detail.unifiedDiff).toContain("/dev/null"); + } + }); + + it("maps multi-file apply_patch with update + delete into edit detail referencing the deleted file", () => { + // Exact data shape from real Codex rollout: update one file, delete another + const patch = [ + "*** Begin Patch", + "*** Update File: /tmp/repo/src/app/index.tsx", + "@@", + ' import { useEffect } from "react";', + '-import { WELCOME_ROUTE } from "@/app-support/index-startup";', + "+", + '+const WELCOME_ROUTE = "/welcome";', + "*** Delete File: /tmp/repo/src/app-support/index-startup.ts", + "*** End Patch", + ].join("\n"); + + const item = expectMapped( + mapCodexRolloutToolCall({ + callId: "codex-delete-multi", + name: "apply_patch", + input: patch, + output: JSON.stringify({ + output: + "Success. Updated the following files:\nM /tmp/repo/src/app/index.tsx\nD /tmp/repo/src/app-support/index-startup.ts\n", + metadata: { exit_code: 0, duration_seconds: 0.0 }, + }), + cwd: "/tmp/repo", + }), + ); + + expect(item.detail.type).toBe("edit"); + if (item.detail.type === "edit") { + // The unified diff should contain both file sections + const diff = item.detail.unifiedDiff ?? ""; + // The update file section should have normal diff lines + expect(diff).toContain("-import"); + expect(diff).toContain("+const WELCOME_ROUTE"); + // The delete file section should reference /dev/null + expect(diff).toContain("/dev/null"); + } + }); + + it("maps fileChange delete with content as removed lines, not added lines", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-file-delete-with-content", + status: "completed", + changes: [ + { + path: "/tmp/repo/src/dead-module.ts", + kind: "delete", + content: 'export const FOO = "bar";\nexport function hello() {}\n', + }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toBeTruthy(); + expect(item?.detail.type).toBe("edit"); + if (item?.detail.type === "edit") { + expect(item.detail.filePath).toBe("src/dead-module.ts"); + const diff = item.detail.unifiedDiff ?? ""; + // For a deletion, the content should appear as REMOVED lines (-) + // not as ADDED lines (+). This is the core bug. + expect(diff).toContain("/dev/null"); + expect(diff).toContain("-export const FOO"); + expect(diff).toContain("-export function hello"); + // The content must NOT appear as added lines + expect(diff).not.toContain("+export const FOO"); + expect(diff).not.toContain("+export function hello"); + } + }); + + it("maps fileChange delete without content to edit detail with /dev/null marker", () => { + const item = mapCodexToolCallFromThreadItem( + { + type: "fileChange", + id: "codex-file-delete-no-content", + status: "completed", + changes: [ + { + path: "/tmp/repo/src/dead-module.ts", + kind: "delete", + }, + ], + }, + { cwd: "/tmp/repo" }, + ); + + expect(item).toBeTruthy(); + // A delete without content should still produce a meaningful detail + if (item?.detail.type === "edit") { + expect(item.detail.filePath).toBe("src/dead-module.ts"); + const diff = item.detail.unifiedDiff ?? ""; + expect(diff).toContain("/dev/null"); + } + }); }); 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 acfe447c3..a5fd872f2 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 @@ -347,7 +347,7 @@ function normalizeDiffHeaderPath(rawPath: string): string { function codexApplyPatchToUnifiedDiff(text: string): string { const lines = text.replace(/\r\n/g, "\n").split("\n"); const output: string[] = []; - let sawDiffBody = false; + let sawDiffContent = false; for (const line of lines) { const directive = parseCodexApplyPatchDirective(line); @@ -362,6 +362,7 @@ function codexApplyPatchToUnifiedDiff(text: string): string { output.push(`diff --git a/${path} b/${path}`); output.push(`--- ${left}`); output.push(`+++ ${right}`); + sawDiffContent = true; } continue; } @@ -378,22 +379,22 @@ function codexApplyPatchToUnifiedDiff(text: string): string { if (line.startsWith("@@")) { output.push(line); - sawDiffBody = true; + sawDiffContent = true; continue; } if (line.startsWith("+") || line.startsWith("-") || line.startsWith(" ")) { output.push(line); - sawDiffBody = true; + sawDiffContent = true; continue; } if (line.startsWith("\\ No newline at end of file")) { output.push(line); - sawDiffBody = true; + sawDiffContent = true; continue; } } - if (!sawDiffBody) { + if (!sawDiffContent) { return text; } @@ -401,6 +402,22 @@ function codexApplyPatchToUnifiedDiff(text: string): string { return normalized.length > 0 ? normalized : text; } +function contentToDeletionDiff(filePath: string, content: string): string { + const lines = content.replace(/\r\n/g, "\n").split("\n"); + const output: 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); + if (nonEmpty.length > 0) { + output.push(`@@ -1,${nonEmpty.length} +0,0 @@`); + for (const line of nonEmpty) { + output.push(`-${line}`); + } + } + return output.join("\n"); +} + function classifyDiffLikeText( text: string, ): { isDiff: true; text: string } | { isDiff: false; text: string } { @@ -716,6 +733,26 @@ function parseFileChangeEntries( .filter((entry): entry is CodexFileChangeEntry => entry !== null); } +function resolveFileChangeTextFields( + file: CodexFileChangeEntry | undefined, +): { unifiedDiff?: string; newString?: string } { + if (!file) { + return {}; + } + const isDelete = file.kind === "delete"; + if (isDelete && file.diff) { + const classified = classifyDiffLikeText(file.diff); + if (classified.isDiff) { + return { unifiedDiff: truncateDiffText(classified.text) }; + } + return { unifiedDiff: truncateDiffText(contentToDeletionDiff(file.path, file.diff)) }; + } + if (isDelete && !file.diff) { + return { unifiedDiff: contentToDeletionDiff(file.path, "") }; + } + return asEditTextFields(file.diff); +} + function mapFileChangeItem( item: z.infer, options?: CodexMapperOptions, @@ -739,7 +776,9 @@ function mapFileChangeItem( files: files.map((file) => ({ path: file.path, ...(file.kind !== undefined ? { kind: file.kind } : {}), - ...asEditFileOutputFields(file.diff), + ...(file.kind === "delete" + ? { patch: resolveFileChangeTextFields(file).unifiedDiff } + : asEditFileOutputFields(file.diff)), })), } : {}), @@ -749,7 +788,7 @@ function mapFileChangeItem( const error = item.error ?? null; const status = resolveStatus(item.status, error, output); const firstFile = files[0]; - const firstTextFields = asEditTextFields(firstFile?.diff); + const firstTextFields = resolveFileChangeTextFields(firstFile); const hasFirstTextFields = Object.keys(firstTextFields).length > 0; const input = toNullableObject({ ...inputBase,