From e8090bb48ae228d306647b9abf466e40a4d70f68 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 30 Jun 2026 02:58:47 +0200 Subject: [PATCH] feat: introduced binary file detection to prevent encoding corruption - Introduced `isProbablyBinary` utility to sniff file headers for NUL bytes or invalid UTF-8 sequences. - Updated `ReadTool` to use the binary sniffer, preventing mojibake corruption in output when reading non-text files. - Refined `file-mentions` auto-reads to skip binary files and mark them as `binary` in the message transcript. - Added comprehensive unit tests for binary detection logic, covering NUL bytes, truncated multibyte characters, and path-based file sniffing. --- packages/coding-agent/CHANGELOG.md | 3 + .../modes/utils/transcript-render-helpers.ts | 4 +- packages/coding-agent/src/session/messages.ts | 2 +- packages/coding-agent/src/tools/read.ts | 56 ++++++++--------- .../coding-agent/src/utils/file-mentions.ts | 11 +++- .../coding-agent/test/file-mentions.test.ts | 22 +++++++ packages/coding-agent/test/tools.test.ts | 26 +++++--- packages/utils/CHANGELOG.md | 4 ++ packages/utils/src/binary.ts | 50 +++++++++++++++ packages/utils/src/index.ts | 1 + packages/utils/test/binary.test.ts | 61 +++++++++++++++++++ 11 files changed, 201 insertions(+), 39 deletions(-) create mode 100644 packages/utils/src/binary.ts create mode 100644 packages/utils/test/binary.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 122258be0..a3f375afd 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,9 @@ ### Changed +- Improved binary file detection to prevent terminal corruption from non-UTF8 content +- Updated file mention summaries to explicitly note skipped binary files + - Enabled contextual snapcompact shape resolution based on rendered text content ### Fixed diff --git a/packages/coding-agent/src/modes/utils/transcript-render-helpers.ts b/packages/coding-agent/src/modes/utils/transcript-render-helpers.ts index d66a6a030..74a70cd23 100644 --- a/packages/coding-agent/src/modes/utils/transcript-render-helpers.ts +++ b/packages/coding-agent/src/modes/utils/transcript-render-helpers.ts @@ -99,9 +99,9 @@ export function buildFileMentionBlock(files: FileMentionMessage["files"], indent const block = new TranscriptBlock(); for (const file of files) { let suffix: string; - if (file.skippedReason === "tooLarge") { + if (file.skippedReason === "tooLarge" || file.skippedReason === "binary") { const size = typeof file.byteSize === "number" ? formatBytes(file.byteSize) : "unknown size"; - suffix = `(skipped: ${size})`; + suffix = file.skippedReason === "binary" ? `(skipped: binary, ${size})` : `(skipped: ${size})`; } else { suffix = file.image ? "(image)" diff --git a/packages/coding-agent/src/session/messages.ts b/packages/coding-agent/src/session/messages.ts index b5eb34f2b..f72c829f5 100644 --- a/packages/coding-agent/src/session/messages.ts +++ b/packages/coding-agent/src/session/messages.ts @@ -488,7 +488,7 @@ export interface FileMentionMessage { /** File size in bytes, if known. */ byteSize?: number; /** Why the file contents were omitted from auto-read. */ - skippedReason?: "tooLarge"; + skippedReason?: "tooLarge" | "binary"; image?: ImageContent; }>; timestamp: number; diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 429fb9ff5..e57369238 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -14,7 +14,15 @@ import type { ImageContent, TextContent } from "@oh-my-pi/pi-ai"; import { glob, type SummaryResult, summarizeCode } from "@oh-my-pi/pi-natives"; import type { Component } from "@oh-my-pi/pi-tui"; import { Text } from "@oh-my-pi/pi-tui"; -import { getRemoteDir, type ImageMetadata, logger, prompt, readImageMetadata, untilAborted } from "@oh-my-pi/pi-utils"; +import { + getRemoteDir, + type ImageMetadata, + isProbablyBinary, + logger, + prompt, + readImageMetadata, + untilAborted, +} from "@oh-my-pi/pi-utils"; import { type } from "arktype"; import { LRUCache } from "lru-cache/raw"; import { @@ -2314,6 +2322,25 @@ export class ReadTool implements AgentTool { content = [{ type: "text", text: `[Cannot read ${ext} file: conversion failed]` }]; } } else { + // Binary sniff before any UTF-8 text materialization. A binary file + // (font, object, archive, packed blob) decodes to NUL/control bytes and + // U+FFFD mojibake that corrupts the terminal and burns context. Images, + // notebooks, and markit-convertible documents were already routed above; + // everything reaching here is meant to be plain text. `:raw` stays the + // explicit escape hatch for reading bytes verbatim. This single guard + // covers both the multi-range and single-range disk paths below. + if (!isRawSelector(parsed) && (await isProbablyBinary(absolutePath))) { + return toolResult({ resolvedPath: absolutePath, suffixResolution }) + .text( + prependSuffixResolutionNotice( + `[Cannot read binary file '${formatPathRelativeToCwd(absolutePath, this.session.cwd)}' (${formatBytes(fileSize)}); not valid UTF-8 text. Use ':raw' to read bytes verbatim.]`, + suffixResolution, + ), + ) + .sourcePath(absolutePath) + .done(); + } + if ( parsed.kind === "none" && this.session.settings.get("read.summarize.enabled") && @@ -2449,33 +2476,6 @@ export class ReadTool implements AgentTool { // counts in `truncation` keep reflecting the source, not the trimmed // view — column truncation surfaces separately via `.limits()`. const rawSelector = isRawSelector(parsed); - // Binary sniff: NUL bytes mean the file is not displayable text - // (binary, or UTF-16 which has NULs in the ASCII range) — emit a - // notice instead of mojibake filling the line budget. `:raw` - // stays an explicit escape hatch. - // - // `collectedLines` covers the common case where at least one - // physical line terminates within the byte budget. Binary blobs - // without newlines (videos, archives, packed JSON) leave it - // empty; their bytes only land in `firstLinePreview`, which the - // `firstLineExceedsLimit` branch below would otherwise emit - // verbatim. Sniffing the preview here keeps the refusal uniform. - if (!rawSelector) { - const hasNul = (text: string): boolean => text.includes("\u0000"); - const binaryDetected = - collectedLines.some(hasNul) || (firstLinePreview !== undefined && hasNul(firstLinePreview.text)); - if (binaryDetected) { - return toolResult({ resolvedPath: absolutePath, suffixResolution }) - .text( - prependSuffixResolutionNotice( - `[Cannot read binary file '${formatPathRelativeToCwd(absolutePath, this.session.cwd)}' (${formatBytes(fileSize)}); content contains NUL bytes (binary or UTF-16 encoded)]`, - suffixResolution, - ), - ) - .sourcePath(absolutePath) - .done(); - } - } const maxColumns = resolveOutputMaxColumns(this.session.settings); // Column truncation is display-only. `collectedLines` MUST stay // byte-for-byte with the on-disk content so the snapshot recorded diff --git a/packages/coding-agent/src/utils/file-mentions.ts b/packages/coding-agent/src/utils/file-mentions.ts index 87ad2bfa1..b7e5a0d19 100644 --- a/packages/coding-agent/src/utils/file-mentions.ts +++ b/packages/coding-agent/src/utils/file-mentions.ts @@ -10,7 +10,7 @@ import path from "node:path"; import { formatHashlineHeader, formatNumberedLines, type SnapshotStore } from "@oh-my-pi/hashline"; import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import type { ImageContent } from "@oh-my-pi/pi-ai"; -import { formatAge, formatBytes, readImageMetadata } from "@oh-my-pi/pi-utils"; +import { formatAge, formatBytes, isProbablyBinary, readImageMetadata } from "@oh-my-pi/pi-utils"; import { canonicalSnapshotKey } from "../edit/file-snapshot-store"; import { normalizeToLF } from "../edit/normalize"; import type { FileMentionMessage } from "../session/messages"; @@ -257,6 +257,15 @@ export async function generateFileMentionMessages( }); continue; } + if (await isProbablyBinary(absolutePath)) { + files.push({ + path: resolvedPath, + content: `(skipped auto-read: binary file, ${formatBytes(stat.size)})`, + byteSize: stat.size, + skippedReason: "binary", + }); + continue; + } const content = await Bun.file(absolutePath).text(); const snapshotStore = options?.useHashLines ? options.snapshotStore : undefined; diff --git a/packages/coding-agent/test/file-mentions.test.ts b/packages/coding-agent/test/file-mentions.test.ts index a49b56c5d..3de5a1525 100644 --- a/packages/coding-agent/test/file-mentions.test.ts +++ b/packages/coding-agent/test/file-mentions.test.ts @@ -98,4 +98,26 @@ describe("generateFileMentionMessages path resolution", () => { expect(message.files).toHaveLength(1); expect(message.files[0]?.path).toBe("My Folder/my file.png"); }); + + test("skips auto-reading a binary file instead of injecting raw bytes", async () => { + const cwd = await createTempDir(); + // TTF header begins with a NUL run; auto-reading it as text would leak + // control bytes into the conversation (the reported bug). + await Bun.write(path.join(cwd, "Silver.ttf"), Buffer.from([0x00, 0x01, 0x00, 0x00, 0x00, 0x0c, 0x4f, 0x53])); + // A non-NUL invalid-UTF8 blob must be refused too, not just NUL-bearing files. + await Bun.write(path.join(cwd, "blob.bin"), Buffer.from([0x4d, 0x5a, 0xff, 0xfe, 0xc0, 0xc0])); + + const messages = await generateFileMentionMessages(["Silver.ttf", "blob.bin"], cwd); + expect(messages).toHaveLength(1); + const message = messages[0]; + if (message?.role !== "fileMention") { + throw new Error("expected file mention message"); + } + expect(message.files).toHaveLength(2); + for (const file of message.files) { + expect(file.skippedReason).toBe("binary"); + expect(file.content).toContain("binary file"); + expect(file.content).not.toContain("\u0000"); + } + }); }); diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index 94a4e8eac..e9e222ce8 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -578,15 +578,27 @@ describe("Coding Agent Tools", () => { expect(output).toContain("Use :1 to read from the start, or :3 to read the last line."); }); - it("should emit a binary notice instead of mojibake for files with NUL bytes", async () => { - const testFile = path.join(testDir, "blob.bin"); - fs.writeFileSync(testFile, Buffer.from([0x61, 0x62, 0x63, 0x00, 0xff, 0xfe, 0x64, 0x65])); + it("should refuse binary files (NUL or invalid UTF-8) instead of emitting mojibake", async () => { + const nulFile = path.join(testDir, "blob.bin"); + fs.writeFileSync(nulFile, Buffer.from([0x61, 0x62, 0x63, 0x00, 0xff, 0xfe, 0x64, 0x65])); + // A header with no NUL but invalid UTF-8 (lone 0xFF/0xC0) must also refuse. + const invalidUtf8File = path.join(testDir, "font.ttfish"); + fs.writeFileSync(invalidUtf8File, Buffer.from([0x4d, 0x5a, 0xff, 0xfe, 0xc0, 0xc0, 0x90, 0x91])); - const result = await readTool.execute("test-call-binary-nul", { path: testFile }); - const output = getTextOutput(result); + for (const file of [nulFile, invalidUtf8File]) { + const output = getTextOutput(await readTool.execute("test-call-binary", { path: file })); + expect(output).toContain("Cannot read binary file"); + expect(output).not.toContain("\u0000"); + expect(output).not.toContain("\uFFFD"); + } + }); - expect(output).toContain("Cannot read binary file"); - expect(output).toContain("NUL bytes"); + it("reads a binary file verbatim when :raw is requested", async () => { + const testFile = path.join(testDir, "raw-blob.bin"); + fs.writeFileSync(testFile, Buffer.from([0x61, 0x62, 0x63, 0x00, 0x64, 0x65])); + + const output = getTextOutput(await readTool.execute("test-call-binary-raw", { path: `${testFile}:raw` })); + expect(output).not.toContain("Cannot read binary file"); }); it("should reject malformed internal-URL selectors instead of dumping the whole resource", async () => { diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index 43275e441..9b6dd0d44 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Added utility to detect binary files based on content sniffing + ## [16.2.6] - 2026-06-29 ### Added diff --git a/packages/utils/src/binary.ts b/packages/utils/src/binary.ts new file mode 100644 index 000000000..1245bd062 --- /dev/null +++ b/packages/utils/src/binary.ts @@ -0,0 +1,50 @@ +/** + * Content-based binary/text classification for files that are about to be + * decoded as UTF-8 text and shown to a model or user. + * + * The read tool and `@file` auto-read both materialize file bytes as UTF-8 + * strings. For a binary file (font, object, archive, packed blob) that decode + * is lossy: NUL bytes and invalid sequences survive as control characters and + * U+FFFD replacements, which corrupt terminal rendering and waste the context + * window with mojibake. Sniff the header first and refuse instead. + * + * @example + * if (await isProbablyBinary(path)) return "[binary file omitted]"; + * const text = await Bun.file(path).text(); + */ +import { peekFile, peekFileSync } from "./peek-file"; + +/** Header window sniffed for the binary heuristic; mirrors git's 8000-byte scan. */ +const BINARY_SNIFF_BYTES = 8192; + +/** + * Classify an in-memory byte header as binary (non-UTF-8-text). + * + * Binary when the header contains a NUL byte (true binary, plus UTF-16/UTF-32 + * text whose ASCII range is NUL-padded) or when it is not valid UTF-8. The + * decode runs in streaming mode so a multibyte sequence truncated at the header + * boundary is tolerated, while any genuinely invalid byte still fails — matching + * the strict `fatal` decode the `local://`/`ssh://` read paths already use. + */ +export function isProbablyBinaryHeader(header: Uint8Array): boolean { + if (header.indexOf(0) !== -1) return true; + try { + new TextDecoder("utf-8", { fatal: true }).decode(header, { stream: true }); + return false; + } catch { + return true; + } +} + +/** + * Sniff the first {@link BINARY_SNIFF_BYTES} of `filePath` and report whether it + * is binary (non-UTF-8-text). See {@link isProbablyBinaryHeader} for the rule. + */ +export function isProbablyBinary(filePath: string, maxBytes = BINARY_SNIFF_BYTES): Promise { + return peekFile(filePath, maxBytes, isProbablyBinaryHeader); +} + +/** Synchronous {@link isProbablyBinary}. */ +export function isProbablyBinarySync(filePath: string, maxBytes = BINARY_SNIFF_BYTES): boolean { + return peekFileSync(filePath, maxBytes, isProbablyBinaryHeader); +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 23dc2bc35..3fcfe829e 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,5 +1,6 @@ export { once, untilAborted } from "./abortable"; export * from "./async"; +export * from "./binary"; export * from "./color"; export * from "./dirs"; export * from "./env"; diff --git a/packages/utils/test/binary.test.ts b/packages/utils/test/binary.test.ts new file mode 100644 index 000000000..de204b26f --- /dev/null +++ b/packages/utils/test/binary.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { isProbablyBinary, isProbablyBinaryHeader, isProbablyBinarySync } from "@oh-my-pi/pi-utils/binary"; + +describe("isProbablyBinaryHeader", () => { + it("treats empty input as text", () => { + expect(isProbablyBinaryHeader(new Uint8Array(0))).toBe(false); + }); + + it("flags a NUL byte as binary", () => { + // TTF/OTF, WASM, ELF, UTF-16 text all carry NUL in their first bytes. + expect(isProbablyBinaryHeader(Buffer.from([0x00, 0x01, 0x00, 0x00]))).toBe(true); + }); + + it("flags invalid UTF-8 without a NUL as binary", () => { + // 0xFF/0xFE never appear in valid UTF-8; a font/object header with no + // early NUL still fails the fatal decode. + expect(isProbablyBinaryHeader(Buffer.from([0x4d, 0x5a, 0xff, 0xfe, 0xc0, 0xc0]))).toBe(true); + }); + + it("accepts plain ASCII text", () => { + expect(isProbablyBinaryHeader(Buffer.from("export const x = 1;\n", "utf-8"))).toBe(false); + }); + + it("accepts multibyte UTF-8 text", () => { + expect(isProbablyBinaryHeader(Buffer.from("héllo — 日本語 🚀\n", "utf-8"))).toBe(false); + }); + + it("tolerates a multibyte sequence truncated at the header boundary", () => { + // "😀" is 4 bytes (F0 9F 98 80); a header cut after the first 2 bytes is a + // valid-but-incomplete sequence, not corruption — streaming decode allows it. + const full = Buffer.from("ok 😀", "utf-8"); + const truncated = full.subarray(0, full.length - 2); + expect(truncated.indexOf(0)).toBe(-1); + expect(isProbablyBinaryHeader(truncated)).toBe(false); + }); +}); + +describe("isProbablyBinary / isProbablyBinarySync", () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-binary-")); + + function writeFile(name: string, bytes: Uint8Array | string): string { + const filePath = path.join(tempDir, name); + fs.writeFileSync(filePath, bytes); + return filePath; + } + + it("classifies a binary file from disk (async + sync agree)", async () => { + const filePath = writeFile("font.ttf", Buffer.from([0x00, 0x01, 0x00, 0x00, 0x00, 0x0c])); + expect(await isProbablyBinary(filePath)).toBe(true); + expect(isProbablyBinarySync(filePath)).toBe(true); + }); + + it("classifies a UTF-8 text file from disk as text", async () => { + const filePath = writeFile("notes.md", "# Title\n\nbody text\n"); + expect(await isProbablyBinary(filePath)).toBe(false); + expect(isProbablyBinarySync(filePath)).toBe(false); + }); +});