🚫 fix: Reject Binary Files in read_file Sandbox Fallback (No More Mojibake) (#12851)

* 🚫 fix: Reject Binary Files in read_file Sandbox Fallback (No More Mojibake)

`read_file("/mnt/data/simple_graph.png")` was shelling `cat` through codeapi
`/exec` and shipping the result back to the model. codeapi's transport is
JSON, so `stdout` containing PNG bytes round-tripped through lossy UTF-8
replacement (every non-ASCII byte became U+FFFD), got line-numbered by
`addLineNumbers`, and arrived in the model's context as a multi-KB blob of
`�PNG\r\n  2 | ...`. The bytes were unrecoverable — and the same
codeapi sandbox logged the base64-style mojibake too — so the goal is
fail-fast, not retrieval.

Two guards in `handleSandboxFileFallback`, both bypassed by the existing
text path:

  1. **Extension precheck** (BEFORE any network call) for known-binary
     types: images, documents (pdf/docx/xlsx/etc), archives, audio, video,
     native libs, fonts, and a few other byte-soup formats. The message
     for image extensions points at the existing chat attachment ("the
     image is already shown to the user, use bash_tool for programmatic
     processing"); other binaries get the generic "use bash" hint.

  2. **NUL-byte sniff** (AFTER the read) on the first 8KB for unknown
     extensions or no-extension paths. The codeapi `/exec` JSON encoding
     mangles most non-UTF-8 bytes but a NUL terminator from a magic
     header survives the round-trip, so this catches novel binary
     formats without an extension precheck.

`lowercaseExtension` uses the basename to avoid false-triggering on
directory-name dots (e.g. `proj.v1/notes` has no extension, not `.v1/notes`).

+6 tests:
  - Image rejected by extension, never calls readSandboxFile, message
    points at the existing attachment.
  - Non-image binary (.zip) rejected with a different (bash-only) message.
  - Case-insensitive extension match (.PNG vs .png).
  - NUL-byte sniff catches unknown-extension binary post-fetch.
  - Text files with binary-adjacent extensions (.txt) still readable.
  - Dotted directory names don't false-trigger the extension match.

38/38 handlers.spec.ts pass.

The companion bash "command not found" issue from the same conversation
is a separate LLM mistake (writing raw Python as the bash command without
`python3 -c` / heredoc wrapper). Not coded here — flagged to the user.

* 🖼️ fix: Allow SVG read_file (XML text, no mojibake risk) — codex review P2

`.svg` was bucketed with raster-image extensions in
`BINARY_EXTENSIONS_NEVER_READABLE`, which made `handleSandboxFileFallback`
reject every SVG before calling readSandboxFile. SVG is an XML text
format — there's no mojibake risk for normal content, and the model has
legitimate reasons to inspect or edit a generated SVG (tweaking colors,
paths, viewBox, etc.). Block was a regression for valid read_file usage.

Remove `.svg` from both `BINARY_EXTENSIONS_NEVER_READABLE` (so it routes
through the normal sandbox read path) and `IMAGE_EXTENSIONS_FOR_HINT`
(now-dead entry — only used by the rejection-message picker). The
post-fetch NUL-byte sniff still catches anything that turns out to be
binary despite a `.svg` extension.

+1 regression test that an SVG with valid XML content reads through
successfully (`<svg>...<circle/>...</svg>` → status: 'success', content
contains `<svg`/`viewBox`).

packages/api/src/agents/handlers.ts

@@ -147,6 +147,154 @@ function addLineNumbers(content: string): string {
   return lines.map((l, i) => `${String(i + 1).padStart(w, ' ')} | ${l}`).join('\n');
 }
 
+/**
+ * Extensions whose contents `read_file` must never serialize as text. `cat`
+ * on a PNG inside the sandbox returns the raw bytes as stdout, JSON-encoded
+ * by codeapi with lossy UTF-8 replacement and then line-numbered by us —
+ * the result is a multi-KB blob of mojibake that pollutes the LLM context
+ * and exposes the raw bytes anyway. Short-circuit before the network call.
+ *
+ * Image categories surface a "use the existing attachment" message because
+ * the file was already attached to the conversation as part of the
+ * code-execution artifact pipeline — re-attaching here would dup it.
+ */
+const BINARY_EXTENSIONS_NEVER_READABLE = new Set([
+  // Raster images (already attached as artifacts by the code-execution
+  // pipeline). `.svg` is intentionally NOT in this list — it's an XML
+  // text format with no mojibake risk, and there are legitimate reasons
+  // for the model to inspect or edit a generated SVG. The post-fetch
+  // NUL-byte sniff still catches anything that turns out to be binary
+  // despite a `.svg` extension.
+  '.png',
+  '.jpg',
+  '.jpeg',
+  '.gif',
+  '.webp',
+  '.bmp',
+  '.tiff',
+  '.tif',
+  '.ico',
+  '.heic',
+  '.heif',
+  '.avif',
+  // Documents (binary container formats)
+  '.pdf',
+  '.doc',
+  '.docx',
+  '.xls',
+  '.xlsx',
+  '.ppt',
+  '.pptx',
+  '.odt',
+  '.ods',
+  '.odp',
+  // Archives
+  '.zip',
+  '.tar',
+  '.gz',
+  '.tgz',
+  '.bz2',
+  '.xz',
+  '.7z',
+  '.rar',
+  '.lz4',
+  '.zst',
+  // Audio / video
+  '.mp3',
+  '.wav',
+  '.flac',
+  '.ogg',
+  '.m4a',
+  '.aac',
+  '.wma',
+  '.mp4',
+  '.mkv',
+  '.mov',
+  '.avi',
+  '.webm',
+  '.flv',
+  '.m4v',
+  // Executables / object files / native libs
+  '.exe',
+  '.dll',
+  '.so',
+  '.dylib',
+  '.o',
+  '.obj',
+  '.a',
+  '.lib',
+  '.bin',
+  '.class',
+  '.jar',
+  // Other byte-soup formats
+  '.parquet',
+  '.bson',
+  '.db',
+  '.sqlite',
+  '.sqlite3',
+  '.pyc',
+  '.pyo',
+  '.wasm',
+  '.ttf',
+  '.otf',
+  '.woff',
+  '.woff2',
+  '.eot',
+]);
+
+const IMAGE_EXTENSIONS_FOR_HINT = new Set([
+  '.png',
+  '.jpg',
+  '.jpeg',
+  '.gif',
+  '.webp',
+  '.bmp',
+  '.tiff',
+  '.tif',
+  '.ico',
+  '.heic',
+  '.heif',
+  '.avif',
+]);
+
+function lowercaseExtension(filePath: string): string {
+  const dot = filePath.lastIndexOf('.');
+  const slash = Math.max(filePath.lastIndexOf('/'), filePath.lastIndexOf('\\'));
+  if (dot < 0 || dot < slash) return '';
+  return filePath.slice(dot).toLowerCase();
+}
+
+/**
+ * Builds the model-visible error returned when `read_file` is invoked on
+ * a binary path. Phrasing is tuned for the LLM: states the fact (file is
+ * binary, can't be read as text), points at the correct affordance for
+ * each common case (image already in the chat; bash for everything else),
+ * and includes the path verbatim so the model can copy-paste into its
+ * next call.
+ */
+function buildBinaryFileError(filePath: string, ext: string): string {
+  if (IMAGE_EXTENSIONS_FOR_HINT.has(ext)) {
+    return `"${filePath}" is an image file (${ext}) and cannot be read as text. The image is already attached to the conversation and visible to the user. To process it programmatically, use \`bash_tool\` (e.g. \`file ${filePath}\` for metadata, or \`python3 -c '...'\` to operate on the bytes).`;
+  }
+  return `"${filePath}" is a binary file (${ext}) and cannot be read as text by \`read_file\`. Use \`bash_tool\` to process it (e.g. \`file ${filePath}\` for metadata, or a runtime-appropriate command for the format).`;
+}
+
+/**
+ * True when the first chunk of a string contains a NUL byte. Used as a
+ * post-fetch safety net for files whose extension didn't match the
+ * blocklist (no extension, novel format, etc.) — sniffs `cat` stdout to
+ * avoid ever forwarding mangled bytes to the LLM. 8KB is the same
+ * window the skill-file path uses; enough for any reasonable magic
+ * number while bounded enough to stay cheap.
+ */
+function looksBinary(content: string): boolean {
+  const limit = Math.min(content.length, 8192);
+  for (let i = 0; i < limit; i++) {
+    if (content.charCodeAt(i) === 0) return true;
+  }
+  return false;
+}
+
 /**
  * Routes a `read_file` call to the code-execution sandbox via the
  * host-provided `readSandboxFile` callback. The sandbox session id and
@@ -156,12 +304,30 @@ function addLineNumbers(content: string): string {
  * `ToolExecuteResult` with the file content (line-numbered) on success,
  * or an instructive error pointing the model at `bash_tool` when the
  * sandbox isn't reachable from this configuration.
+ *
+ * Two binary guards keep `cat`-on-a-PNG-style mojibake out of the LLM
+ * context: (1) an extension precheck that short-circuits known binary
+ * types BEFORE any network call, and (2) a NUL-byte content sniff after
+ * the read for unknown extensions. The codeapi `/exec` transport is JSON,
+ * which already lossily down-converts non-UTF-8 stdout to replacement
+ * characters — the bytes are unrecoverable here, so the goal is to fail
+ * fast with an instructive message rather than ship garbage.
  */
 async function handleSandboxFileFallback(
   tc: ToolCallRequest,
   filePath: string,
   options: ToolExecuteOptions,
 ): Promise<ToolExecuteResult> {
+  const ext = lowercaseExtension(filePath);
+  if (BINARY_EXTENSIONS_NEVER_READABLE.has(ext)) {
+    return {
+      toolCallId: tc.id,
+      status: 'error',
+      content: '',
+      errorMessage: buildBinaryFileError(filePath, ext),
+    };
+  }
+
   const { readSandboxFile } = options;
   if (!readSandboxFile) {
     return {
@@ -189,6 +355,14 @@ async function handleSandboxFileFallback(
         errorMessage: `Failed to read "${filePath}" from the code-execution sandbox. Try \`bash_tool\` (e.g. \`cat ${filePath}\`).`,
       };
     }
+    if (looksBinary(result.content)) {
+      return {
+        toolCallId: tc.id,
+        status: 'error',
+        content: '',
+        errorMessage: `"${filePath}" appears to be a binary file and cannot be read as text. Use \`bash_tool\` to process it (e.g. \`file ${filePath}\` for metadata).`,
+      };
+    }
     /**
      * Cap before line-numbering. `addLineNumbers` allocates a SECOND
      * full-size string with per-line prefixes, so a multi-MB log read