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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)"
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<typeof readSchema, ReadToolDetails> {
|
||||
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<ReadToolDetails>({ 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<typeof readSchema, ReadToolDetails> {
|
||||
// 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<ReadToolDetails>({ 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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Added
|
||||
|
||||
- Added utility to detect binary files based on content sniffing
|
||||
|
||||
## [16.2.6] - 2026-06-29
|
||||
|
||||
### Added
|
||||
|
||||
@@ -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<boolean> {
|
||||
return peekFile(filePath, maxBytes, isProbablyBinaryHeader);
|
||||
}
|
||||
|
||||
/** Synchronous {@link isProbablyBinary}. */
|
||||
export function isProbablyBinarySync(filePath: string, maxBytes = BINARY_SNIFF_BYTES): boolean {
|
||||
return peekFileSync(filePath, maxBytes, isProbablyBinaryHeader);
|
||||
}
|
||||
@@ -1,5 +1,6 @@
|
||||
export { once, untilAborted } from "./abortable";
|
||||
export * from "./async";
|
||||
export * from "./binary";
|
||||
export * from "./color";
|
||||
export * from "./dirs";
|
||||
export * from "./env";
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user