[hussainph]

Problem (VC-801)

read called env.readBinaryFile (readFile, no stat, no cap) and decoded, split and re-joined the whole file before its 2,000-line / 50 KB cut. All of that runs in Electron main.

• A large log or JSON fixture: hundreds of MB of buffer, string and line array, plus seconds of synchronous CPU that freeze every window.
• A symlink to /dev/zero or to a FIFO: readFile reads to an EOF that never comes, or blocks waiting for a writer. Verified on main: the symlinked-FIFO test hangs. Named directly, both were already refused, because resolveReadToolPath's exists → lstat rejects their file type. The ticket's direct-path sequences 2 and 3 are only reachable through a link.
• Non-image binaries were decoded as UTF-8, so up to 50 KB of U+FFFD/NUL went to the model as text. Verified on main.

Fix

• FileSystem.openFileReader (new; vendored types.ts, env/nodejs.ts) opens with O_RDONLY | O_NONBLOCK | O_NOCTTY and judges the file with fstat on the open descriptor:
  • A FIFO can't hold the open, and a terminal can't become the controlling terminal of hostd on a Linux host.
  • There is no pathname check/use window for a swap to slip a device through.
  • Symlinks are followed. Directories → is_directory. FIFOs and devices → invalid ("Not a regular file …").
  • A socket can't be opened at all, and its errno differs by platform: ENXIO on Linux, and EOPNOTSUPP on macOS, which libuv leaves unnamed. So when an open fails, the path is stated afterwards, only to choose the same "Not a regular file" words.
  • It returns a positional read(position, length, context, into?) and never throws.
• ScopedExecutionEnv guards and delegates it like readBinaryFile. refusingCredentialReads adds it to the refused reads.
• read opens the reader and sniffs the first 8 KiB:
  • An image signature reads the whole file into the unchanged image path, so the processor still gets a Node Buffer.
  • A NUL byte answers [Binary file (N), not shown as text. Use bash: file <path>, or xxd <path> | head -c 51200]. If the file also starts with a UTF-16 BOM (FF FE / FE FF), it answers [UTF-16 text (N), not shown as text. Use bash: iconv -f UTF-16 -t UTF-8 <path> | head -c 51200] instead.
  • Anything else streams in 256 KiB reads into one reused buffer. Lines are found by their LF byte, and only the prefix the cut can show is decoded. That prefix then goes through the existing truncateHead, with only its totals replaced.
• Result text, continuation offsets, truncation wording, schema, description, image handling and markingSavedOutput are untouched.

Decisions

• Interface over openTextLineReader. That reader buffers a whole line, so one 500 MB minified line would still be held in full, and ScopedExecutionEnv refuses it. A positional byte reader bounds both cases. It also lets the type check happen on the open descriptor, which avoids a TOCTOU race and a blocking FIFO open.
• Every byte is still read. The continuation hint says "of N lines", and that count needs the whole file. Bytes are only scanned for LF, never decoded or kept, in 256 KiB reads into one buffer.
• No stat before open. Review asked for a stat before open, keeping fstat as the swap guard. I pushed that as 5a7d5a9. CodeQL then raised a new high js/file-system-race alert on it ("The file may have changed since it was checked"). docs/development/codeql-triage.md prescribes exactly the descriptor-only design: "Checking and consuming the same descriptor closes pathname check/use races … reject FIFO/directory targets through nonblocking open and fstat." So the descriptor check stays the only check. O_NOCTTY (from the review) covers the tty side effect of opening a device, and the socket refusal is kept by the post-failure stat. The guarantee is about the file type only. ScopedExecutionEnv's workspace lstat walk followed by the open is still racy, as it was before this change.
• Raw-byte totals. details.truncation.totalBytes and an oversized first line's size are counted in the file's own bytes, less a leading BOM. For valid UTF-8 that is the same number. Decoding everything counted each invalid byte as a 3-byte U+FFFD; matching that would mean decoding the whole file, which is the allocation churn this removes. No caller reads totalBytes for read.
• Negative limit selects no lines. It used to count from the end, an accident of Array.prototype.slice. The surrounding hint numbers are computed exactly as before.
• Images are still read whole once their signature matches. This is unchanged on purpose, because the ticket says image handling stays as it is. An animated PNG (acTL past the head) falls through to the binary notice, since PNG headers always contain NUL bytes.
• One existing test changed: pi-vendor-tools-coverage.test.ts "propagates read failures" now injects its failure on openFileReader instead of readBinaryFile, because read no longer calls readBinaryFile. Its assertions are unchanged. The other read tests (truncation messages, offset/limit, images, saved-output marking) pass without edits.
• Vendor divergence 7 is recorded in vendor/pi-harness/README.md, with a note in UPSTREAM.md.

Tests

New src/pi/read-bounded.test.ts (37 tests):

• Virtual 500 MB log (26 M lines): a fake FileByteReader serves a repeating pattern, so nothing that size touches disk or memory. The test checks:
  • the exact first 2,000 lines plus [Showing lines 1-2000 of 26214401. Use offset=2001 to continue.];
  • that no read exceeds the 256 KiB chunk, and every read after the head reuses one buffer;
  • that the read reaches EOF;
  • that truncateHead (wrapped through vi.mock) is called once, on at most 51,201 + 262,144 characters;
  • an offset/limit read from the middle of a separate 16 MB virtual file.
• Virtual 500 MB single line: the exact [Line 1 is 500.0MB …] message and totals, under the same bounds.
• This replaces the on-disk 128 MB / 64 MB fixtures and the RSS sampling, which were non-deterministic.
• Equivalence: the old whole-file algorithm, kept verbatim in the test as an oracle, must give identical results across 14 contents × 8 offsets × 7 limits. Cases include BOM, CRLF, multibyte characters split at 8 KiB and 256 KiB, the byte and line cuts, and fractional values. A separate check covers invalid UTF-8 split across chunks.
• Non-regular files: /dev/zero and FIFOs, directly and through a symlink, plus a socket behind a symlink, directories and a symlink to a regular file.
• Binary detection: a NUL in the first 8 KiB gives the notice, and a NUL past 8 KiB is still decoded. Also an animated PNG, UTF-16LE/BE versus binaries that start with FF/FE.
• openFileReader itself: positional and short reads, close, cancellation at each stage, injected stat/read/close failures, the descriptor check, the open flags (O_NOCTTY), opens that fail for other reasons, scoped guard and credential refusal.

Fails on main (one run of the file, -t-filtered): the symlinked-FIFO test times out "still waiting", the binary test receives decoded AAAA…, and the directory test gets Node's EISDIR message. The /dev/zero-symlink test was deliberately not run against main, because the old code would allocate up to 2 GB.

Run locally, one file at a time with --maxWorkers=1, all green:

• read-bounded
• pi-vendor-tools-coverage
• vendor-tools-edge
• tools-vendor
• tool-path
• credential-env
• scoped-execution-env
• pi-vendor-env-coverage
• tools

Also vp check on the touched files. Typecheck and coverage are left to CI.

Overlap: git merge-tree is clean against origin/main and against #909, #919, #920, #921 and #922. #651, an older open PR touching scoped-execution-env.ts, conflicts with this branch on exactly the 56 files it already conflicts on with main, so nothing new. Targets main.

Follow-ups (not in this PR)

• The text stream reads to EOF, not to the size seen at open, so a fast-growing log is chased. If this gets capped, cap only when size > 0, because procfs-style files report 0.
• The image path still reads the whole file once its signature matches. The ticket said to leave images alone.
• A zero or negative offset shows up as-is in the hint numbers. Clamp it in the schema later.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 13e90f3b-e319-40ba-bda5-27c2aa96d491

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}