From bc80702a4668f4431f047702cbbaa2e06b034979 Mon Sep 17 00:00:00 2001 From: huangyihang Date: Tue, 4 Aug 2026 01:01:30 +0800 Subject: [PATCH] fix: write binary skill other-files without UTF-8 corruption Skill other files (e.g. images inside a skill directory) were read and written through a UTF-8 text path: the buffer was decoded with toString('utf-8') and written back with writeFileContent, so invalid byte sequences were replaced with U+FFFD and binary files were silently corrupted on both import and generate. Route binary other files through a buffer path instead: - isBinaryBuffer() detects content that a UTF-8 round-trip would corrupt - change detection compares bytes via Buffer.equals (readFileBufferOrNull) - writes go through writeFileBuffer, keeping text files on the existing text path (trailing-newline normalization + structured equivalence) --- src/types/dir-feature-processor.test.ts | 55 +++++++++++++++++++++++++ src/types/dir-feature-processor.ts | 53 +++++++++++++++++------- src/utils/file.ts | 23 +++++++++++ 3 files changed, 115 insertions(+), 16 deletions(-) diff --git a/src/types/dir-feature-processor.test.ts b/src/types/dir-feature-processor.test.ts index 1f8869b39..9a14c17ca 100644 --- a/src/types/dir-feature-processor.test.ts +++ b/src/types/dir-feature-processor.test.ts @@ -4,8 +4,10 @@ import { createMockLogger } from "../test-utils/mock-logger.js"; import { setupTestDirectory } from "../test-utils/test-directories.js"; import { ensureDir, + readFileBufferOrNull, readFileContentOrNull, removeDirectory, + writeFileBuffer, writeFileContent, } from "../utils/file.js"; import { AiDir, AiDirFile } from "./ai-dir.js"; @@ -16,9 +18,11 @@ vi.mock("../utils/file.js", async () => { return { ...actual, readFileContentOrNull: vi.fn().mockResolvedValue(null), + readFileBufferOrNull: vi.fn().mockResolvedValue(null), removeDirectory: vi.fn(), ensureDir: vi.fn(), writeFileContent: vi.fn(), + writeFileBuffer: vi.fn(), }; }); @@ -218,6 +222,57 @@ describe("DirFeatureProcessor", () => { expect(writeFileContent).toHaveBeenCalledTimes(1); }); + it("should write binary other files via the buffer path", async () => { + const binaryBuffer = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46]); + const processor = new TestDirProcessor({ logger: createMockLogger(), outputRoot: testDir }); + + const otherFile: AiDirFile = { + relativeFilePathToDirPath: "image.jpg", + fileBuffer: binaryBuffer, + }; + const dirs = [createMockDirWithFiles({ dirPath: "/path/to/dir1", otherFiles: [otherFile] })]; + + const result = await processor.writeAiDirs(dirs); + + expect(result).toEqual({ count: 1, paths: ["/path/to/dir1/image.jpg"] }); + expect(writeFileBuffer).toHaveBeenCalledWith("/path/to/dir1/image.jpg", binaryBuffer); + expect(writeFileContent).not.toHaveBeenCalled(); + }); + + it("should skip unchanged binary other files", async () => { + const binaryBuffer = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46]); + vi.mocked(readFileBufferOrNull).mockResolvedValue(binaryBuffer); + const processor = new TestDirProcessor({ logger: createMockLogger(), outputRoot: testDir }); + + const otherFile: AiDirFile = { + relativeFilePathToDirPath: "image.jpg", + fileBuffer: binaryBuffer, + }; + const dirs = [createMockDirWithFiles({ dirPath: "/path/to/dir1", otherFiles: [otherFile] })]; + + const result = await processor.writeAiDirs(dirs); + + expect(result).toEqual({ count: 0, paths: [] }); + expect(writeFileBuffer).not.toHaveBeenCalled(); + }); + + it("should detect changes in binary other files", async () => { + const binaryBuffer = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46]); + vi.mocked(readFileBufferOrNull).mockResolvedValue(Buffer.from([0x00, 0x01, 0x02])); + const processor = new TestDirProcessor({ logger: createMockLogger(), outputRoot: testDir }); + + const otherFile: AiDirFile = { + relativeFilePathToDirPath: "image.jpg", + fileBuffer: binaryBuffer, + }; + const dirs = [createMockDirWithFiles({ dirPath: "/path/to/dir1", otherFiles: [otherFile] })]; + + const result = await processor.writeAiDirs(dirs); + + expect(result).toEqual({ count: 1, paths: ["/path/to/dir1/image.jpg"] }); + expect(writeFileBuffer).toHaveBeenCalledTimes(1); + }); + it("should return changed count without writing in dry-run mode", async () => { vi.mocked(readFileContentOrNull).mockResolvedValue(null); const processor = new TestDirProcessor({ diff --git a/src/types/dir-feature-processor.ts b/src/types/dir-feature-processor.ts index a882fb012..904f7b5de 100644 --- a/src/types/dir-feature-processor.ts +++ b/src/types/dir-feature-processor.ts @@ -4,8 +4,11 @@ import { fileContentsEquivalent } from "../utils/content-equivalence.js"; import { addTrailingNewline, ensureDir, + isBinaryBuffer, + readFileBufferOrNull, readFileContentOrNull, removeDirectory, + writeFileBuffer, writeFileContent, } from "../utils/file.js"; import { stringifyFrontmatter } from "../utils/frontmatter.js"; @@ -96,23 +99,37 @@ export abstract class DirFeatureProcessor { } } - // Compute content for other files + // Compute content for other files. Text files are compared and written + // through the text path (trailing-newline normalization + structured + // equivalence); binary files (e.g. images in a skill dir) go through the + // buffer path so their bytes are never corrupted by a UTF-8 round-trip. const otherFiles: AiDirFile[] = aiDir.getOtherFiles(); - const otherFileContents: string[] = []; + const otherFileContents: (string | Buffer)[] = []; for (const file of otherFiles) { - const contentWithNewline = addTrailingNewline(file.fileBuffer.toString("utf-8")); - otherFileContents.push(contentWithNewline); - if (!dirHasChanges) { - const filePath = join(dirPath, file.relativeFilePathToDirPath); - const existingContent = await readFileContentOrNull(filePath); - if ( - !fileContentsEquivalent({ - filePath, - expected: contentWithNewline, - existing: existingContent, - }) - ) { - dirHasChanges = true; + if (isBinaryBuffer(file.fileBuffer)) { + otherFileContents.push(file.fileBuffer); + if (!dirHasChanges) { + const filePath = join(dirPath, file.relativeFilePathToDirPath); + const existingBuffer = await readFileBufferOrNull(filePath); + if (!existingBuffer || !existingBuffer.equals(file.fileBuffer)) { + dirHasChanges = true; + } + } + } else { + const contentWithNewline = addTrailingNewline(file.fileBuffer.toString("utf-8")); + otherFileContents.push(contentWithNewline); + if (!dirHasChanges) { + const filePath = join(dirPath, file.relativeFilePathToDirPath); + const existingContent = await readFileContentOrNull(filePath); + if ( + !fileContentsEquivalent({ + filePath, + expected: contentWithNewline, + existing: existingContent, + }) + ) { + dirHasChanges = true; + } } } } @@ -155,7 +172,11 @@ export abstract class DirFeatureProcessor { "This indicates a synchronization issue between otherFiles and otherFileContents arrays.", ); } - await writeFileContent(filePath, content); + if (typeof content === "string") { + await writeFileContent(filePath, content); + } else { + await writeFileBuffer(filePath, content); + } changedPaths.push(join(relativeDir, file.relativeFilePathToDirPath)); } } diff --git a/src/utils/file.ts b/src/utils/file.ts index cbd97ba66..0080a3845 100644 --- a/src/utils/file.ts +++ b/src/utils/file.ts @@ -301,6 +301,29 @@ export async function readFileBuffer(filepath: string): Promise { return readFile(filepath); } +/** + * Read file as a buffer if it exists, otherwise return null. + */ +export async function readFileBufferOrNull(filepath: string): Promise { + if (await fileExists(filepath)) { + return readFileBuffer(filepath); + } + return null; +} + +/** + * Whether a UTF-8 text round-trip would corrupt the buffer. + * + * Used to decide between the text and binary write paths for skill + * "other files": valid UTF-8 text (including CJK) survives a + * `toString("utf-8")` + `Buffer.from(_, "utf-8")` round-trip unchanged, + * while binary content such as JPEG/GIF does not (invalid byte sequences + * are replaced with U+FFFD). + */ +export function isBinaryBuffer(buffer: Buffer): boolean { + return !Buffer.from(buffer.toString("utf-8"), "utf-8").equals(buffer); +} + /** * Normalizes text to LF line endings and adds exactly one trailing newline. * Removes any existing trailing whitespace and appends a single newline.