Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,18 @@ async function readExistingFileForPreview(absolutePath: string): Promise<string>
}
}

async function readTextFileRejectingBinary(absolutePath: string, filePath: string): Promise<string> {
// Sniff for null bytes before decoding: decoding binary as UTF-8 mangles
// bytes into replacement characters, so a patch could either match
// garbage or silently corrupt the file on write. Reject early with a
// distinct code instead of letting apply_patch guess at binary content.
const raw = await readFile(absolutePath);
if (raw.includes(0)) {
throw Object.assign(new Error(`Refusing to patch binary file: ${filePath}`), { code: "EBINARY" });
}
return raw.toString("utf-8");
}

function formatLineCountSummary(added: number, removed: number): string {
return `(+${added} -${removed})`;
}
Expand Down Expand Up @@ -916,7 +928,7 @@ async function createPatchPreview(cwd: string, hunks: ParsedPatch[]): Promise<Ap
continue;
}

const oldContent = await readFile(absolutePath, "utf-8");
const oldContent = await readTextFileRejectingBinary(absolutePath, hunk.filePath);
const newContent =
hunk.chunks.length === 0 ? oldContent : replaceChunks(oldContent, hunk.filePath, hunk.chunks).content;
if (hunk.movePath) {
Comment on lines -919 to 922

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (broader_impact): Binary delete operations still read the file with readFile(..., "utf-8") in the delete branch, so the pending and final tool results include replacement-character text previews for binary contents even though binary update previews are suppressed. This produces the misleading preview the change is intended to prevent.

Triggers: When apply_patch deletes a binary file through the tool interface.

Suggested fix: Read binary deletes as bytes and omit the preview, or detect the null byte in the delete preview path and fall back to the progress-only result.

Expand Down Expand Up @@ -1194,7 +1206,7 @@ async function prepareOperation(cwd: string, hunk: ParsedPatch): Promise<Prepare
if (hunk.type === "delete") return { hunk, absolutePath, destination, fuzz: 0 };
const preservedMode = sourceStat.mode & 0o777;
if (hunk.movePath !== undefined) await requireAbsent(destination);
const currentContent = await readFile(absolutePath, "utf-8");
const currentContent = await readTextFileRejectingBinary(absolutePath, hunk.filePath);
const result =
hunk.chunks.length === 0
? { content: currentContent, fuzz: 0 }
Expand Down
135 changes: 135 additions & 0 deletions test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it } from "vitest";
import {
APPLY_PATCH_DESCRIPTION,
APPLY_PATCH_LARK_GRAMMAR,
ApplyPatchError,
type ApplyPatchExtensionAPI,
applyPatch,
applyPatchDetailed,
Expand Down Expand Up @@ -227,6 +228,140 @@ describe("pi-apply-patch", () => {
expect(await readFile(path.join(directory, "sample.txt"), "utf-8")).toBe("after\n");
});

it("#given binary file with null byte #when applying patch #then rejects with EBINARY and preserves file", async () => {
// given
const directory = await createTempDirectory();
const binaryPath = path.join(directory, "binary.bin");
await writeFile(binaryPath, Buffer.from([0x00, 0x01, 0x02, 0xff]));

// when
let caught: unknown;
try {
await applyPatch(
directory,
`*** Begin Patch
*** Update File: binary.bin
@@
-some
+changed
*** End Patch`,
);
} catch (error) {
caught = error;
}

// then
if (!(caught instanceof ApplyPatchError)) {
throw new Error("Expected apply_patch to reject with ApplyPatchError");
}
const failure = caught.failures[0];
expect(failure).toMatchObject({
filePath: "binary.bin",
operation: "update",
code: "EBINARY",
});
expect(failure?.message ?? "").toMatch(/binary|non-text|not text/);
expect(await readFile(binaryPath)).toEqual(Buffer.from([0x00, 0x01, 0x02, 0xff]));
});

it("#given binary file with null byte #when moving with apply_patch then rejects with EBINARY and leaves both paths unchanged", async () => {
// given
const directory = await createTempDirectory();
const sourcePath = path.join(directory, "binary.bin");
const destinationPath = path.join(directory, "moved.bin");
const original = Buffer.from("before\0after\n");
await writeFile(sourcePath, original);
const patch = `*** Begin Patch
*** Update File: binary.bin
*** Move to: moved.bin
@@
-before\0after
+changed
*** End Patch`;

// when
let caught: unknown;
try {
await applyPatch(directory, patch);
} catch (error) {
caught = error;
}

// then
if (!(caught instanceof ApplyPatchError)) {
throw new Error("Expected apply_patch to reject with ApplyPatchError");
}
expect(caught.failures[0]).toMatchObject({
filePath: "binary.bin",
operation: "update",
code: "EBINARY",
});
expect(await readFile(sourcePath)).toEqual(original);
await expect(readFile(destinationPath)).rejects.toMatchObject({ code: "ENOENT" });
});

it("#given binary file with null byte #when deleting with apply_patch then deletes it", async () => {
// given
const directory = await createTempDirectory();
const binaryPath = path.join(directory, "binary.bin");
await writeFile(binaryPath, Buffer.from([0x00, 0x01, 0x02, 0xff]));
const patch = `*** Begin Patch
*** Delete File: binary.bin
*** End Patch`;

// when
await applyPatch(directory, patch);

// then
await expect(readFile(binaryPath)).rejects.toMatchObject({ code: "ENOENT" });
});

it("#given binary file with null byte #when apply_patch tool previews then omits text preview and reports EBINARY", async () => {
// given
const directory = await createTempDirectory();
const binaryPath = path.join(directory, "binary.bin");
const original = Buffer.from("before\0after\n");
await writeFile(binaryPath, original);
const patch = `*** Begin Patch
*** Update File: binary.bin
@@
-before\0after
+changed
*** End Patch`;
const updates: Array<{ text: string; update: ApplyPatchUpdate }> = [];

// when
const result = await createApplyPatchTool().execute(
"binary-preview-test",
{ input: patch },
undefined,
(update) => {
const text = update.content.find((block) => block.type === "text")?.text;
if (text) updates.push({ text, update });
},
{ cwd: directory } as never,
);

// then
const initialUpdate = updates[0];
if (!initialUpdate) {
throw new Error("apply_patch did not emit an initial update");
}
expect(initialUpdate.text).toBe("Applying patch (0/1)...");
expect(initialUpdate.update.details?.preview).toBeUndefined();
expect(initialUpdate.text).not.toContain("before\0after");
expect(result.details?.preview).toBeUndefined();
expect(result.details?.result?.failures[0]).toMatchObject({
filePath: "binary.bin",
operation: "update",
code: "EBINARY",
});
const resultText = result.content.find((block) => block.type === "text")?.text ?? "";
expect(resultText).toContain("Refusing to patch binary file: binary.bin");
expect(resultText).not.toContain("MUST read");
expect(await readFile(binaryPath)).toEqual(original);
});

it("#given parent traversal path #when applying patch #then rejects outside cwd", async () => {
// given
const directory = await createTempDirectory();
Expand Down