fix(core): reject malformed patch hunks (#38188)

This commit is contained in:
Aiden Cline 2026-07-22 13:11:51 -05:00 committed by GitHub
parent 89e3141079
commit 532292b5f3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 411 additions and 61 deletions

View file

@ -22,6 +22,30 @@ describe("Patch", () => {
])
})
test("parses an empty patch", () => {
expect(parse("*** Begin Patch\n*** End Patch")).toEqual([])
})
test("ignores a Codex environment preamble", () => {
expect(
parse("*** Begin Patch\n*** Environment ID: remote\n*** Add File: file.txt\n+content\n*** End Patch"),
).toEqual([{ type: "add", path: "file.txt", contents: "content" }])
})
test("parses an update followed by an add", () => {
expect(
parse("*** Begin Patch\n*** Update File: update.txt\n@@\n+line\n*** Add File: add.txt\n+content\n*** End Patch"),
).toEqual([
{
type: "update",
path: "update.txt",
movePath: undefined,
chunks: [{ oldLines: [], newLines: ["line"], changeContext: undefined }],
},
{ type: "add", path: "add.txt", contents: "content" },
])
})
test("parses a file move", () => {
expect(
parse("*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n@@\n-old\n+new\n*** End Patch"),
@ -66,6 +90,20 @@ describe("Patch", () => {
])
})
test("strips quoted heredoc wrappers", () => {
const patch = "*** Begin Patch\n*** Add File: add.txt\n+added\n*** End Patch"
expect(parse(`<<'EOF'\n${patch}\nEOF`)).toEqual([{ type: "add", path: "add.txt", contents: "added" }])
expect(parse(`<<\"EOF\"\n${patch}\nEOF`)).toEqual([{ type: "add", path: "add.txt", contents: "added" }])
})
test("rejects malformed heredoc wrappers", () => {
const patch = "*** Begin Patch\n*** Add File: add.txt\n+added\n*** End Patch"
expect(() => parse(`<<\"EOF'\n${patch}\nEOF`)).toThrow("The first line of the patch must be '*** Begin Patch'")
expect(() => parse("<<EOF\n*** Begin Patch\n*** Add File: add.txt\n+added\nEOF")).toThrow(
"The last line of the patch must be '*** End Patch'",
)
})
test("parses a whitespace-padded hunk header", () => {
expect(parse("*** Begin Patch\n *** Update File: foo.txt\n@@\n-old\n+new\n*** End Patch")).toEqual([
{
@ -99,6 +137,23 @@ describe("Patch", () => {
])
})
test("parses relative and absolute hunk paths", () => {
expect(
parse(
"*** Begin Patch\n*** Add File: relative.txt\n+content\n*** Delete File: /tmp/delete.txt\n*** Update File: /tmp/update.txt\n@@\n-old\n+new\n*** End Patch",
),
).toEqual([
{ type: "add", path: "relative.txt", contents: "content" },
{ type: "delete", path: "/tmp/delete.txt" },
{
type: "update",
path: "/tmp/update.txt",
movePath: undefined,
chunks: [{ oldLines: ["old"], newLines: ["new"], changeContext: undefined }],
},
])
})
test("strips one carriage return from CRLF patch lines", () => {
expect(parse("*** Begin Patch\r\n*** Update File: file.txt\r\n@@\r\n-old\r\n+new\r\n*** End Patch\r\n")).toEqual([
{
@ -136,6 +191,42 @@ describe("Patch", () => {
])
})
test("allows an end-of-file marker before an explicit chunk", () => {
expect(
parse("*** Begin Patch\n*** Update File: file.txt\n*** End of File\n@@\n-old\n+new\n*** End Patch"),
).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [{ oldLines: ["old"], newLines: ["new"], changeContext: undefined }],
},
])
})
test("allows an end-of-file marker before an implicit chunk and move", () => {
expect(parse("*** Begin Patch\n*** Update File: file.txt\n*** End of File\n-old\n+new\n*** End Patch")).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [{ oldLines: ["old"], newLines: ["new"] }],
},
])
expect(
parse(
"*** Begin Patch\n*** Update File: old.txt\n*** End of File\n*** Move to: new.txt\n@@\n-old\n+new\n*** End Patch",
),
).toEqual([
{
type: "update",
path: "old.txt",
movePath: "new.txt",
chunks: [{ oldLines: ["old"], newLines: ["new"], changeContext: undefined }],
},
])
})
test("derives fuzzy line updates while preserving BOM", () => {
const update = Patch.derive("update.txt", [{ oldLines: [" old "], newLines: ["new"] }], "\uFEFFold\n")
expect(update).toEqual({ content: "new\n", bom: true })
@ -236,15 +327,157 @@ describe("Patch", () => {
).toThrow("Failed to find expected lines")
})
test("matches V1 lenient parsing of malformed hunk bodies", () => {
expect(parse("*** Begin Patch\n*** Add File: add.txt\nmissing plus\n*** End Patch")).toEqual([
{ type: "add", path: "add.txt", contents: "" },
])
expect(parse("*** Begin Patch\n*** Update File: update.txt\n*** End Patch")).toEqual([
{ type: "update", path: "update.txt", movePath: undefined, chunks: [] },
])
expect(parse("*** Begin Patch\n*** Delete File: delete.txt\nunexpected body\n*** End Patch")).toEqual([
{ type: "delete", path: "delete.txt" },
test("parses an update without an explicit first chunk header", () => {
expect(parse("*** Begin Patch\n*** Update File: file.txt\n import foo\n+bar\n*** End Patch")).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [{ oldLines: ["import foo"], newLines: ["import foo", "bar"] }],
},
])
})
test("keeps indented update markers as context lines", () => {
expect(
parse(
"*** Begin Patch\n*** Update File: a.txt\n@@\n-old a\n+new a\n *** Update File: b.txt\n@@\n-old b\n+new b\n*** End Patch",
),
).toEqual([
{
type: "update",
path: "a.txt",
movePath: undefined,
chunks: [
{
oldLines: ["old a", "*** Update File: b.txt"],
newLines: ["new a", "*** Update File: b.txt"],
changeContext: undefined,
},
{ oldLines: ["old b"], newLines: ["new b"], changeContext: undefined },
],
},
])
})
test("keeps indented move and EOF markers as context lines", () => {
expect(
parse(
"*** Begin Patch\n*** Update File: file.txt\n@@\n before\n *** Move to: moved.txt\n *** End of File\n*** End Patch",
),
).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [
{
oldLines: ["before", "*** Move to: moved.txt", "*** End of File"],
newLines: ["before", "*** Move to: moved.txt", "*** End of File"],
changeContext: undefined,
},
],
},
])
})
test("preserves update context indentation", () => {
expect(parse("*** Begin Patch\n*** Update File: file.txt\n@@ section\n-old\n+new\n*** End Patch")).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [{ oldLines: ["old"], newLines: ["new"], changeContext: " section" }],
},
])
})
test("preserves bare empty update lines as context", () => {
expect(
parse("*** Begin Patch\n*** Update File: file.txt\n@@\n context before\n\n context after\n*** End Patch"),
).toEqual([
{
type: "update",
path: "file.txt",
movePath: undefined,
chunks: [
{
oldLines: ["context before", "", "context after"],
newLines: ["context before", "", "context after"],
changeContext: undefined,
},
],
},
])
})
test("rejects invalid add and delete lines", () => {
expect(() => parse("*** Begin Patch\n*** Add File: file.txt\nbad\n*** End Patch")).toThrow(
"Invalid hunk at line 3: 'bad' is not a valid hunk header",
)
expect(() => parse("*** Begin Patch\n*** Delete File: file.txt\nbad\n*** End Patch")).toThrow(
"Invalid hunk at line 3: 'bad' is not a valid hunk header",
)
})
test("rejects an empty update hunk", () => {
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n*** End Patch")).toThrow(
"Invalid hunk at line 2: Update file hunk for path 'file.txt' is empty",
)
expect(() =>
parse(
"*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n*** Delete File: other.txt\n*** End Patch",
),
).toThrow("Invalid hunk at line 2: Update file hunk for path 'old.txt' is empty")
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n*** End of File\n*** End Patch")).toThrow(
"Invalid hunk at line 2: Update file hunk for path 'file.txt' is empty",
)
})
test("rejects an empty update chunk", () => {
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\n*** End Patch")).toThrow(
"Invalid hunk at line 4: Update hunk does not contain any lines",
)
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\n*** End of File\n*** End Patch")).toThrow(
"Invalid hunk at line 4: Update hunk does not contain any lines",
)
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\n@@\n-old\n+new\n*** End Patch")).toThrow(
"Invalid hunk at line 4: Unexpected line found in update hunk: '@@'",
)
expect(() =>
parse("*** Begin Patch\n*** Update File: file.txt\n@@\n*** Update File: other.txt\n@@\n-old\n+new\n*** End Patch"),
).toThrow("Invalid hunk at line 4: Unexpected line found in update hunk: '*** Update File: other.txt'")
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\nbad\n*** End Patch")).toThrow(
"Invalid hunk at line 4: Unexpected line found in update hunk: 'bad'",
)
})
test("rejects an invalid update line", () => {
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\n-old\nbad\n*** End Patch")).toThrow(
"Invalid hunk at line 5: Expected update hunk to start with a @@ context marker, got: 'bad'",
)
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@foo\n*** End Patch")).toThrow(
"Invalid hunk at line 3: Unexpected line found in update hunk: '@@foo'",
)
expect(() => parse("*** Begin Patch\n*** Update File: file.txt\n@@\n-old\n*** Frobnicate File: foo\n*** End Patch")).toThrow(
"Invalid hunk at line 5: Expected update hunk to start with a @@ context marker, got: '*** Frobnicate File: foo'",
)
})
test("rejects invalid and pathless hunk headers", () => {
expect(() => parse("*** Begin Patch\n*** Frobnicate File: foo\n*** End Patch")).toThrow(
"Invalid hunk at line 2: '*** Frobnicate File: foo' is not a valid hunk header",
)
expect(() => parse("*** Begin Patch\n*** Add File:\n*** End Patch")).toThrow(
"Invalid hunk at line 2: '*** Add File:' is not a valid hunk header",
)
for (const header of ["*** Add File: ", "*** Delete File: ", "*** Update File: "]) {
expect(() => parse(`*** Begin Patch\n${header}\n*** End Patch`)).toThrow(
`Invalid hunk at line 2: '${header.trim()}' is not a valid hunk header`,
)
}
expect(() =>
parse("*** Begin Patch\n*** Update File: old.txt\n*** Move to: \n@@\n-old\n+new\n*** End Patch"),
).toThrow("Invalid hunk at line 3: '*** Move to:' is not a valid hunk header")
})
})

View file

@ -13,8 +13,10 @@ export class BoundaryError extends Schema.TaggedErrorClass<BoundaryError>()("Pat
export class InvalidHunkError extends Schema.TaggedErrorClass<InvalidHunkError>()("Patch.InvalidHunkError", {
line: Schema.String,
lineNumber: Schema.Number,
reason: Schema.optional(Schema.String),
}) {
override get message() {
if (this.reason) return `Invalid hunk at line ${this.lineNumber}: ${this.reason}`
return `Invalid hunk at line ${this.lineNumber}: '${this.line}' is not a valid hunk header. Valid hunk headers: '*** Add File: {path}', '*** Delete File: {path}', '*** Update File: {path}'`
}
}
@ -57,51 +59,48 @@ export function parse(patchText: string): Result.Result<ReadonlyArray<Hunk>, Par
while (index < end) {
const line = lines[index]!
const header = line.trim()
if (header.startsWith("*** Add File:")) {
const path = header.slice("*** Add File:".length).trim()
if (!path) {
index++
continue
}
if (
index === begin + 1 &&
header.startsWith("*** Environment ID:") &&
header.slice("*** Environment ID:".length).trim()
) {
index++
continue
}
if (header.startsWith("*** Add File: ")) {
const path = header.slice("*** Add File: ".length).trim()
const parsed = parseAdd(lines, index + 1, end)
if ("error" in parsed) return Result.fail(parsed.error)
hunks.push({ type: "add", path, contents: parsed.content })
index = parsed.next
continue
}
if (header.startsWith("*** Delete File:")) {
const path = header.slice("*** Delete File:".length).trim()
if (!path) {
index++
continue
}
if (header.startsWith("*** Delete File: ")) {
const path = header.slice("*** Delete File: ".length).trim()
hunks.push({ type: "delete", path })
index++
continue
}
if (header.startsWith("*** Update File:")) {
const path = header.slice("*** Update File:".length).trim()
if (!path) {
index++
continue
}
if (header.startsWith("*** Update File: ")) {
const path = header.slice("*** Update File: ".length).trim()
let next = index + 1
let movePath: string | undefined
if (lines[next]?.startsWith("*** Move to:")) {
movePath = lines[next]!.slice("*** Move to:".length).trim()
while (lines[next]?.trimEnd() === "*** End of File") next++
const move = lines[next]?.trimEnd()
if (move === "*** Move to:" || move?.startsWith("*** Move to: ")) {
movePath = move.slice("*** Move to: ".length).trim()
if (!movePath) {
return Result.fail(new InvalidHunkError({ line: lines[next]!.trim(), lineNumber: next + 1 }))
}
next++
}
const parsed = parseUpdate(lines, next, end)
const parsed = parseUpdate(lines, next, end, path, index)
if ("error" in parsed) return Result.fail(parsed.error)
hunks.push({ type: "update", path, movePath, chunks: parsed.chunks })
index = parsed.next
continue
}
index++
}
if (hunks.length === 0) {
const invalid = lines.findIndex((line, index) => index > begin && index < end && line.trim() !== "")
if (invalid !== -1) {
return Result.fail(new InvalidHunkError({ line: lines[invalid]!.trim(), lineNumber: invalid + 1 }))
}
return Result.fail(new InvalidHunkError({ line: header, lineNumber: index + 1 }))
}
return Result.succeed(hunks)
}
@ -123,47 +122,166 @@ export function joinBom(text: string, bom: boolean) {
return bom ? `\uFEFF${stripped}` : stripped
}
function parseAdd(lines: ReadonlyArray<string>, start: number, end: number) {
function parseAdd(
lines: ReadonlyArray<string>,
start: number,
end: number,
): { content: string; next: number } | { error: InvalidHunkError } {
const content: string[] = []
let index = start
while (index < end && !lines[index]!.startsWith("***")) {
if (lines[index]!.startsWith("+")) content.push(lines[index]!.slice(1))
while (index < end && !isBoundary(lines[index]!.trim())) {
if (!lines[index]!.startsWith("+")) {
return { error: new InvalidHunkError({ line: lines[index]!.trim(), lineNumber: index + 1 }) }
}
content.push(lines[index]!.slice(1))
index++
}
return { content: content.join("\n"), next: index }
}
function parseUpdate(lines: ReadonlyArray<string>, start: number, end: number) {
const chunks: UpdateFileChunk[] = []
function parseUpdate(
lines: ReadonlyArray<string>,
start: number,
end: number,
path: string,
hunk: number,
): { chunks: ReadonlyArray<UpdateFileChunk>; next: number } | { error: InvalidHunkError } {
const chunks: Array<{
oldLines: string[]
newLines: string[]
changeContext?: string
endOfFile?: boolean
}> = []
let index = start
while (index < end && !lines[index]!.startsWith("***")) {
if (!lines[index]!.startsWith("@@")) {
let afterEndOfFile = false
while (index < end) {
const line = lines[index]!
const updateLine = line.trimEnd()
if (afterEndOfFile) {
if (updateLine === "") {
index++
continue
}
if (updateLine === "@@" || updateLine.startsWith("@@ ")) afterEndOfFile = false
else if (isBoundary(updateLine)) break
else {
return {
error: new InvalidHunkError({
line,
lineNumber: index + 1,
reason: `Expected update hunk to start with a @@ context marker, got: '${line}'`,
}),
}
}
}
if (updateLine === "*** End of File") {
const chunk = chunks.at(-1)
if (chunk && chunk.oldLines.length === 0 && chunk.newLines.length === 0) {
return {
error: new InvalidHunkError({
line: updateLine,
lineNumber: index + 1,
reason: "Update hunk does not contain any lines",
}),
}
}
if (chunk) {
chunk.endOfFile = true
afterEndOfFile = true
}
index++
continue
}
const changeContext = lines[index]!.slice(2).trim() || undefined
const oldLines: string[] = []
const newLines: string[] = []
let endOfFile = false
index++
while (index < end && !lines[index]!.startsWith("@@") && !lines[index]!.startsWith("***")) {
const line = lines[index]!
if (line.startsWith(" ")) {
oldLines.push(line.slice(1))
newLines.push(line.slice(1))
} else if (line.startsWith("-")) oldLines.push(line.slice(1))
else if (line.startsWith("+")) newLines.push(line.slice(1))
if (isBoundary(updateLine)) break
if (updateLine === "@@" || updateLine.startsWith("@@ ")) {
const previous = chunks.at(-1)
if (previous && previous.oldLines.length === 0 && previous.newLines.length === 0) {
return {
error: new InvalidHunkError({
line,
lineNumber: index + 1,
reason: `Unexpected line found in update hunk: '${line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)`,
}),
}
}
chunks.push({
oldLines: [],
newLines: [],
changeContext: updateLine === "@@" ? undefined : updateLine.slice("@@ ".length),
})
index++
continue
}
if (lines[index]?.trim() === "*** End of File") {
endOfFile = true
if (chunks.length === 0) chunks.push({ oldLines: [], newLines: [] })
const chunk = chunks.at(-1)!
if (line === "") {
chunk.oldLines.push("")
chunk.newLines.push("")
index++
continue
}
if (line.startsWith(" ")) {
chunk.oldLines.push(line.slice(1))
chunk.newLines.push(line.slice(1))
index++
continue
}
if (line.startsWith("-")) {
chunk.oldLines.push(line.slice(1))
index++
continue
}
if (line.startsWith("+")) {
chunk.newLines.push(line.slice(1))
index++
continue
}
const populated = chunk.oldLines.length > 0 || chunk.newLines.length > 0
return {
error: new InvalidHunkError({
line,
lineNumber: index + 1,
reason: populated
? `Expected update hunk to start with a @@ context marker, got: '${line}'`
: `Unexpected line found in update hunk: '${line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)`,
}),
}
}
if (chunks.length === 0) {
return {
error: new InvalidHunkError({
line: lines[hunk]!.trim(),
lineNumber: hunk + 1,
reason: `Update file hunk for path '${path}' is empty`,
}),
}
}
const last = chunks.at(-1)!
if (last.oldLines.length === 0 && last.newLines.length === 0) {
const line = lines[index]!.trim()
return {
error: new InvalidHunkError({
line,
lineNumber: index + 1,
reason:
line === "*** End Patch"
? "Update hunk does not contain any lines"
: `Unexpected line found in update hunk: '${line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)`,
}),
}
chunks.push({ oldLines, newLines, changeContext, endOfFile: endOfFile || undefined })
}
return { chunks, next: index }
}
function isBoundary(line: string) {
return (
line === "*** End Patch" ||
line.startsWith("*** Add File: ") ||
line.startsWith("*** Delete File: ") ||
line.startsWith("*** Update File: ")
)
}
function computeReplacements(lines: ReadonlyArray<string>, path: string, chunks: ReadonlyArray<UpdateFileChunk>) {
const replacements: Array<readonly [start: number, remove: number, insert: ReadonlyArray<string>]> = []
let lineIndex = 0
@ -231,5 +349,4 @@ const normalize = (value: string) =>
.replace(/[\u00A0\u2002-\u200A\u202F\u205F\u3000]/g, " ")
const splitBom = (text: string) =>
text.startsWith("\uFEFF") ? { bom: true, text: text.slice(1) } : { bom: false, text }
const stripHeredoc = (input: string) =>
input.match(/^(?:cat\s+)?<<['"]?(\w+)['"]?\s*\n([\s\S]*?)\n\1\s*$/)?.[2] ?? input
const stripHeredoc = (input: string) => input.match(/^(?:cat\s+)?<<(['"]?)(\w+)\1\s*\n([\s\S]*?)\n\2\s*$/)?.[3] ?? input