From f4b9153f7834f1d8a5ece287610f1901ea5f291b Mon Sep 17 00:00:00 2001 From: Michael <50274907+saleweaver@users.noreply.github.com> Date: Fri, 18 Apr 2025 02:00:30 +0200 Subject: [PATCH] =?UTF-8?q?chore:=20consolidate=20patch=20prefix=20constan?= =?UTF-8?q?ts=20in=20apply=E2=80=91patch.ts=20(#274)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR replaces all hard‑coded patch markers in apply‑patch.ts with the corresponding constants (now) exported from parse‑apply‑patch.ts. Changes • Import PATCH_PREFIX, PATCH_SUFFIX, ADD_FILE_PREFIX, DELETE_FILE_PREFIX, UPDATE_FILE_PREFIX, MOVE_FILE_TO_PREFIX, END_OF_FILE_PREFIX, and HUNK_ADD_LINE_PREFIX from parse‑apply‑patch.ts. • Remove duplicate string literals for patch markers in apply‑patch.ts. • Changed is_done() to trim the input to account for the slight difference between the variables. Why • DRY & Consistency: Ensures a single source of truth for patch prefixes. • Maintainability: Simplifies future updates to prefix values by centralizing them. • Readability: Makes the code more declarative and self‑documenting. All tests are passing, lint and format was ran. --- codex-cli/src/parse-apply-patch.ts | 15 +++-- codex-cli/src/utils/agent/apply-patch.ts | 82 ++++++++++++++---------- 2 files changed, 55 insertions(+), 42 deletions(-) diff --git a/codex-cli/src/parse-apply-patch.ts b/codex-cli/src/parse-apply-patch.ts index 60b7e611..acadf20d 100644 --- a/codex-cli/src/parse-apply-patch.ts +++ b/codex-cli/src/parse-apply-patch.ts @@ -22,13 +22,14 @@ export type ApplyPatchOp = | ApplyPatchDeleteFileOp | ApplyPatchUpdateFileOp; -const PATCH_PREFIX = "*** Begin Patch\n"; -const PATCH_SUFFIX = "\n*** End Patch"; -const ADD_FILE_PREFIX = "*** Add File: "; -const DELETE_FILE_PREFIX = "*** Delete File: "; -const UPDATE_FILE_PREFIX = "*** Update File: "; -const END_OF_FILE_PREFIX = "*** End of File"; -const HUNK_ADD_LINE_PREFIX = "+"; +export const PATCH_PREFIX = "*** Begin Patch\n"; +export const PATCH_SUFFIX = "\n*** End Patch"; +export const ADD_FILE_PREFIX = "*** Add File: "; +export const DELETE_FILE_PREFIX = "*** Delete File: "; +export const UPDATE_FILE_PREFIX = "*** Update File: "; +export const MOVE_FILE_TO_PREFIX = "*** Move to: "; +export const END_OF_FILE_PREFIX = "*** End of File"; +export const HUNK_ADD_LINE_PREFIX = "+"; /** * @returns null when the patch is invalid diff --git a/codex-cli/src/utils/agent/apply-patch.ts b/codex-cli/src/utils/agent/apply-patch.ts index 96443272..c0b1eaf2 100644 --- a/codex-cli/src/utils/agent/apply-patch.ts +++ b/codex-cli/src/utils/agent/apply-patch.ts @@ -3,6 +3,16 @@ import fs from "fs"; import path from "path"; +import { + ADD_FILE_PREFIX, + DELETE_FILE_PREFIX, + END_OF_FILE_PREFIX, + MOVE_FILE_TO_PREFIX, + PATCH_SUFFIX, + UPDATE_FILE_PREFIX, + HUNK_ADD_LINE_PREFIX, + PATCH_PREFIX, +} from "src/parse-apply-patch"; // ----------------------------------------------------------------------------- // Types & Models @@ -103,7 +113,7 @@ class Parser { } if ( prefixes && - prefixes.some((p) => this.lines[this.index]!.startsWith(p)) + prefixes.some((p) => this.lines[this.index]!.startsWith(p.trim())) ) { return true; } @@ -130,13 +140,13 @@ class Parser { } parse(): void { - while (!this.is_done(["*** End Patch"])) { - let path = this.read_str("*** Update File: "); + while (!this.is_done([PATCH_SUFFIX])) { + let path = this.read_str(UPDATE_FILE_PREFIX); if (path) { if (this.patch.actions[path]) { throw new DiffError(`Update File Error: Duplicate Path: ${path}`); } - const moveTo = this.read_str("*** Move to: "); + const moveTo = this.read_str(MOVE_FILE_TO_PREFIX); if (!(path in this.current_files)) { throw new DiffError(`Update File Error: Missing File: ${path}`); } @@ -146,7 +156,7 @@ class Parser { this.patch.actions[path] = action; continue; } - path = this.read_str("*** Delete File: "); + path = this.read_str(DELETE_FILE_PREFIX); if (path) { if (this.patch.actions[path]) { throw new DiffError(`Delete File Error: Duplicate Path: ${path}`); @@ -157,7 +167,7 @@ class Parser { this.patch.actions[path] = { type: ActionType.DELETE, chunks: [] }; continue; } - path = this.read_str("*** Add File: "); + path = this.read_str(ADD_FILE_PREFIX); if (path) { if (this.patch.actions[path]) { throw new DiffError(`Add File Error: Duplicate Path: ${path}`); @@ -170,7 +180,7 @@ class Parser { } throw new DiffError(`Unknown Line: ${this.lines[this.index]}`); } - if (!this.startswith("*** End Patch")) { + if (!this.startswith(PATCH_SUFFIX.trim())) { throw new DiffError("Missing End Patch"); } this.index += 1; @@ -183,11 +193,11 @@ class Parser { while ( !this.is_done([ - "*** End Patch", - "*** Update File:", - "*** Delete File:", - "*** Add File:", - "*** End of File", + PATCH_SUFFIX, + UPDATE_FILE_PREFIX, + DELETE_FILE_PREFIX, + ADD_FILE_PREFIX, + END_OF_FILE_PREFIX, ]) ) { const defStr = this.read_str("@@ "); @@ -258,14 +268,14 @@ class Parser { const lines: Array = []; while ( !this.is_done([ - "*** End Patch", - "*** Update File:", - "*** Delete File:", - "*** Add File:", + PATCH_SUFFIX, + UPDATE_FILE_PREFIX, + DELETE_FILE_PREFIX, + ADD_FILE_PREFIX, ]) ) { const s = this.read_str(); - if (!s.startsWith("+")) { + if (!s.startsWith(HUNK_ADD_LINE_PREFIX)) { throw new DiffError(`Invalid Add File Line: ${s}`); } lines.push(s.slice(1)); @@ -349,12 +359,14 @@ function peek_next_section( while (index < lines.length) { const s = lines[index]!; if ( - s.startsWith("@@") || - s.startsWith("*** End Patch") || - s.startsWith("*** Update File:") || - s.startsWith("*** Delete File:") || - s.startsWith("*** Add File:") || - s.startsWith("*** End of File") + [ + "@@", + PATCH_SUFFIX, + UPDATE_FILE_PREFIX, + DELETE_FILE_PREFIX, + ADD_FILE_PREFIX, + END_OF_FILE_PREFIX, + ].some((p) => s.startsWith(p.trim())) ) { break; } @@ -367,7 +379,7 @@ function peek_next_section( index += 1; const lastMode: "keep" | "add" | "delete" = mode; let line = s; - if (line[0] === "+") { + if (line[0] === HUNK_ADD_LINE_PREFIX) { mode = "add"; } else if (line[0] === "-") { mode = "delete"; @@ -412,7 +424,7 @@ function peek_next_section( ins_lines: insLines, }); } - if (index < lines.length && lines[index] === "*** End of File") { + if (index < lines.length && lines[index] === END_OF_FILE_PREFIX) { index += 1; return [old, chunks, index, true]; } @@ -430,8 +442,8 @@ export function text_to_patch( const lines = text.trim().split("\n"); if ( lines.length < 2 || - !(lines[0] ?? "").startsWith("*** Begin Patch") || - lines[lines.length - 1] !== "*** End Patch" + !(lines[0] ?? "").startsWith(PATCH_PREFIX.trim()) || + lines[lines.length - 1] !== PATCH_SUFFIX.trim() ) { throw new DiffError("Invalid patch text"); } @@ -445,11 +457,11 @@ export function identify_files_needed(text: string): Array { const lines = text.trim().split("\n"); const result = new Set(); for (const line of lines) { - if (line.startsWith("*** Update File: ")) { - result.add(line.slice("*** Update File: ".length)); + if (line.startsWith(UPDATE_FILE_PREFIX)) { + result.add(line.slice(UPDATE_FILE_PREFIX.length)); } - if (line.startsWith("*** Delete File: ")) { - result.add(line.slice("*** Delete File: ".length)); + if (line.startsWith(DELETE_FILE_PREFIX)) { + result.add(line.slice(DELETE_FILE_PREFIX.length)); } } return [...result]; @@ -459,8 +471,8 @@ export function identify_files_added(text: string): Array { const lines = text.trim().split("\n"); const result = new Set(); for (const line of lines) { - if (line.startsWith("*** Add File: ")) { - result.add(line.slice("*** Add File: ".length)); + if (line.startsWith(ADD_FILE_PREFIX)) { + result.add(line.slice(ADD_FILE_PREFIX.length)); } } return [...result]; @@ -581,8 +593,8 @@ export function process_patch( writeFn: (p: string, c: string) => void, removeFn: (p: string) => void, ): string { - if (!text.startsWith("*** Begin Patch")) { - throw new DiffError("Patch must start with *** Begin Patch"); + if (!text.startsWith(PATCH_PREFIX)) { + throw new DiffError("Patch must start with *** Begin Patch\\n"); } const paths = identify_files_needed(text); const orig = load_files(paths, openFn);