From 301908560e6d6b4d79f6157b39a2b216b4bfcd83 Mon Sep 17 00:00:00 2001 From: Sunil Srivatsa Date: Mon, 17 Aug 2026 08:23:29 -0700 Subject: [PATCH 1/3] perf(read): materialize a local file once per read The local text read path opened the same file for every consumer. A ranged read of a file within the snapshot cap cost four opens and three decodes: an 8KiB binary sniff, a streaming scan for the rendered window, a whole-file read for bracket context, and another whole-file read to hash the snapshot. Whole-file reads under the structural summarizer paid a fifth. Two of those readers also ran normalizeToLF over the same bytes. Read the bytes once at or below SNAPSHOT_MAX_BYTES and derive every view from them: sniff the leading 8KiB of the buffer, slice the rendered window out of it under the identical line and byte budgets, index bracket context into its addressable lines, and hand the normalized text to the snapshot store and the summarizer. Past the cap nothing wants the whole file, so the streaming reader stays. Line byte lengths are walked out of the buffer rather than measured on the decoded strings, so reported byte counts and the truncation boundary stay exact for content that is not valid UTF-8. The buffered text is BOM-stripped for hashing, matching the decoder the patcher's live read uses. A whole-file read of a BOM file previously hashed its tag from BOM-bearing text, so the following edit only applied through stale-hash recovery and told the model the file had changed externally when it had not. Also stop rejoining lines into a fresh whole-file string on the way to tree-sitter when the caller still holds that text, and drop the unread selectedBytesTotal accounting from the streaming reader. Measured on 966 differential cases across CRLF, BOM, lone-CR, invalid-UTF-8, oversized-line, empty, no-trailing-newline, multi-range and raw shapes: the spurious recovery warning is the only behavioral difference. Raw reads, which skip the tree-sitter parse that dominates everything else, get 30-45% faster (2.7MB: 10.3ms -> 5.7ms); non-raw reads 1-3%. --- .../coding-agent/src/tools/read-format.ts | 18 +- .../coding-agent/src/tools/read-summary.ts | 13 +- packages/coding-agent/src/tools/read.ts | 366 ++++++++++++++---- .../coding-agent/src/utils/block-context.ts | 10 +- .../test/read-single-pass.test.ts | 175 +++++++++ packages/utils/src/binary.ts | 8 +- 6 files changed, 509 insertions(+), 81 deletions(-) create mode 100644 packages/coding-agent/test/read-single-pass.test.ts diff --git a/packages/coding-agent/src/tools/read-format.ts b/packages/coding-agent/src/tools/read-format.ts index 9255ccf58..d2fcd6b7c 100644 --- a/packages/coding-agent/src/tools/read-format.ts +++ b/packages/coding-agent/src/tools/read-format.ts @@ -72,7 +72,20 @@ export async function readHashlineHeaderContext( absolutePath: string, cwd: string, ): Promise { - const fullText = await Bun.file(absolutePath).text(); + return hashlineHeaderContextForText(session, absolutePath, cwd, await Bun.file(absolutePath).text()); +} + +/** + * {@link readHashlineHeaderContext} for a caller that already holds the file's + * full text, so the file is not reopened just to hash it. Line endings are + * normalized here, exactly as the reading variant does. + */ +export function hashlineHeaderContextForText( + session: ToolSession, + absolutePath: string, + cwd: string, + fullText: string, +): HashlineHeaderContext { const context = recordFullHashlineContext( session, absolutePath, @@ -392,6 +405,7 @@ export function buildInMemoryTextResult( const buildLineEntries = (endLineDisplay: number): LineEntry[] => buildLineEntriesWithBlockContext(allLines, [{ startLine: startLineDisplay, endLine: endLineDisplay }], { path: options.sourcePath, + text, }); let outputText: string; @@ -534,7 +548,7 @@ export function buildInMemoryMultiRangeResult( if (options.raw === true) { outputText = rawParts.length > 0 ? rawParts.join("\n\n…\n\n") : ""; } else if (visibleSpans.length > 0) { - const entries = buildLineEntriesWithBlockContext(allLines, visibleSpans, { path: options.sourcePath }); + const entries = buildLineEntriesWithBlockContext(allLines, visibleSpans, { path: options.sourcePath, text }); if (shouldAddHashLines) seenLines = lineNumbersFromEntries(entries); const firstLine = entries.find(entry => entry.kind === "line"); if (firstLine?.kind === "line") { diff --git a/packages/coding-agent/src/tools/read-summary.ts b/packages/coding-agent/src/tools/read-summary.ts index fcdcfd24b..753dcdfee 100644 --- a/packages/coding-agent/src/tools/read-summary.ts +++ b/packages/coding-agent/src/tools/read-summary.ts @@ -52,21 +52,26 @@ export function routeReadThroughBridge( if (!bridge?.capabilities.readTextFile || !bridge.readTextFile) return undefined; return bridge.readTextFile({ path: absolutePath, ...options }); } +/** + * Structural summary of `absolutePath`, or `null` when the file is too large, + * too short, or unparseable. `diskText` lets a caller that already read the file + * hand those bytes over instead of forcing a second read; an ACP bridge still + * wins, since the editor's buffer is the source of truth. + */ export async function trySummarize( session: ToolSession, absolutePath: string, fileSize: number, signal?: AbortSignal, + diskText?: string, ): Promise { if (fileSize > MAX_SUMMARY_BYTES) return null; try { throwIfAborted(signal); const bridgePromise = routeReadThroughBridge(session, absolutePath); - const code = - bridgePromise !== undefined - ? await bridgePromise.catch(() => Bun.file(absolutePath).text()) - : await Bun.file(absolutePath).text(); + const readDisk = async () => diskText ?? (await Bun.file(absolutePath).text()); + const code = bridgePromise !== undefined ? await bridgePromise.catch(readDisk) : await readDisk(); throwIfAborted(signal); const lineCount = countTextLines(code); if (lineCount > MAX_SUMMARY_LINES) return null; diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index dba46c9b9..66409ba0b 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -10,7 +10,15 @@ import type { ToolTier, } from "@oh-my-pi/pi-agent-core"; import type { ImageContent, TextContent } from "@oh-my-pi/pi-ai"; -import { type ImageMetadata, isProbablyBinary, logger, prompt, readImageMetadata } from "@oh-my-pi/pi-utils"; +import { + BINARY_SNIFF_BYTES, + type ImageMetadata, + isProbablyBinary, + isProbablyBinaryHeader, + logger, + prompt, + readImageMetadata, +} from "@oh-my-pi/pi-utils"; import { canonicalSnapshotKey, getFileSnapshotStore, @@ -85,6 +93,7 @@ import { formatTextWithMode, type HashlineHeaderContext, hashlineHeaderContext, + hashlineHeaderContextForText, lineNumbersFromSpans, markMarkdownContentType, prependHashlineHeader, @@ -117,13 +126,187 @@ export { readToolRenderer } from "./read-renderer"; const MAX_PROFILE_SUMMARY_BYTES = 32 * 1024 * 1024; const MAX_ARTIFACT_RAW_INLINE_BYTES = DEFAULT_MAX_BYTES; -async function readBracketContextFullLines(absolutePath: string, fileSize: number): Promise { - if (fileSize > SNAPSHOT_MAX_BYTES) return undefined; +/** LF byte, scanned natively to find line boundaries in a buffered file. */ +const LF_BYTE = 0x0a; + +/** + * Whole-file bytes plus every view the local text read path consumes, + * materialized exactly once. + * + * The binary sniff, the structural summary, the emitted line window, bracket + * context and the snapshot hash all want the same bytes. Each used to open the + * file for itself, so a single ranged read cost up to four opens, three UTF-8 + * decodes and two CRLF normalization passes over identical content. + * + * Only files at or below {@link SNAPSHOT_MAX_BYTES} are buffered: past that cap + * bracket context and the snapshot are skipped anyway, so streaming a window + * stays strictly cheaper than materializing the file. + */ +interface BufferedFileText { + /** File bytes, verbatim. */ + readonly bytes: Buffer; + /** Verbatim UTF-8 decode: a leading BOM and CRLF line endings both survive. */ + readonly rawText: string; + /** {@link rawText} split on LF, CR retained, so segments stay byte-faithful. */ + readonly rawSegments: readonly string[]; + /** BOM-stripped, CRLF-preserving text: what `Bun.file(path).text()` returns. */ + readonly strippedText: string; + /** {@link strippedText} normalized to LF — the exact text the snapshot store hashes. */ + readonly normalizedText: string; + /** Addressable lines of {@link normalizedText}; bracket context indexes these. */ + readonly addressableLines: readonly string[]; + /** Whether the final byte is LF. */ + readonly endsWithNewline: boolean; +} + +/** + * Read `absolutePath` once and derive every view of it the read path needs. + * Returns `undefined` when the bytes cannot be read, which drops the caller + * back to the streaming reader and reproduces today's error surface. + * + * `Bun.file(path).text()` strips a leading BOM while `Buffer.toString` keeps it, + * and the snapshot store plus the patcher's live-file read both go through the + * stripping decoder. {@link BufferedFileText.strippedText} therefore reproduces + * that decode for hashing while {@link BufferedFileText.rawText} stays verbatim + * for the emitted lines and their byte accounting. + */ +async function loadBufferedFileText(absolutePath: string): Promise { + let bytes: Buffer; try { - return splitAddressableFileLines(normalizeToLF(await Bun.file(absolutePath).text())); + bytes = await fs.readFile(absolutePath); } catch { return undefined; } + const rawText = bytes.toString("utf-8"); + const strippedText = rawText.charCodeAt(0) === 0xfeff ? rawText.slice(1) : rawText; + // `normalizeToLF` allocates a copy; skip it outright for the common LF file. + const normalizedText = strippedText.includes("\r") ? normalizeToLF(strippedText) : strippedText; + const rawSegments = rawText.split("\n"); + let addressableLines: readonly string[]; + if (normalizedText === rawText) { + // Nothing was rewritten, so display and bracket context share one array; + // the terminal newline sentinel is dropped exactly as + // `splitAddressableFileLines` does. + const last = rawSegments.length - 1; + addressableLines = last > 0 && rawSegments[last] === "" ? rawSegments.slice(0, last) : rawSegments; + } else { + addressableLines = splitAddressableFileLines(normalizedText); + } + return { + bytes, + rawText, + rawSegments, + strippedText, + normalizedText, + addressableLines, + endsWithNewline: bytes.length > 0 && bytes[bytes.length - 1] === LF_BYTE, + }; +} + +/** The line window a range read renders, with the budget accounting behind it. */ +interface ReadLineWindow { + lines: string[]; + totalFileLines: number; + collectedBytes: number; + stoppedByByteLimit: boolean; + firstLinePreview?: { text: string; bytes: number }; + firstLineByteLength?: number; + /** Whether the fully scanned source ended in a newline. */ + hasTrailingNewline: boolean; + /** False when `stopScanAfterCollect` cut the scan short — `totalFileLines` is then a lower bound. */ + reachedEof: boolean; +} + +/** + * Slice the window {@link streamLinesFromFile} would have collected out of an + * already-buffered file, under the identical line and byte budgets. + * + * Line byte lengths are walked out of the buffer rather than measured on the + * decoded strings: a file that is not valid UTF-8 decodes to U+FFFD, whose + * encoded length differs from the bytes on disk, and those lengths decide both + * the reported byte counts and where truncation lands. + */ +function collectLineWindowFromBuffer( + file: BufferedFileText, + startLine: number, + maxLinesToCollect: number, + maxBytes: number, + selectedLineLimit: number, + includeTerminalNewline: boolean, +): ReadLineWindow { + const { bytes, rawSegments, endsWithNewline } = file; + // A trailing LF closes the last line rather than opening an empty one, except + // in raw mode where that terminal sentinel is addressable. + const totalFileLines = + endsWithNewline && !includeTerminalNewline && rawSegments.length > 1 + ? rawSegments.length - 1 + : rawSegments.length; + const window: ReadLineWindow = { + lines: [], + totalFileLines, + collectedBytes: 0, + stoppedByByteLimit: false, + hasTrailingNewline: endsWithNewline, + reachedEof: true, + }; + if (startLine >= totalFileLines) return window; + + let lineStart = 0; + for (let index = 0; index < startLine; index++) { + const newlineAt = bytes.indexOf(LF_BYTE, lineStart); + if (newlineAt === -1) { + lineStart = bytes.length; + break; + } + lineStart = newlineAt + 1; + } + + let doneCollecting = false; + let selectedLinesSeen = 0; + for (let index = startLine; index < totalFileLines; index++) { + const newlineAt = bytes.indexOf(LF_BYTE, lineStart); + const lineEnd = newlineAt === -1 ? bytes.length : newlineAt; + const lineByteLength = lineEnd - lineStart; + + if (selectedLinesSeen < selectedLineLimit) selectedLinesSeen++; + // Preview covers the first selected line only, capped at the byte budget: + // the oversized-first-line branch renders it when no full line fits. + if (window.lines.length === 0 && window.firstLinePreview === undefined && lineByteLength > 0) { + const previewEnd = Math.min(lineEnd, lineStart + maxBytes); + const { text, bytes: previewBytes } = truncateHeadBytes(bytes.subarray(lineStart, previewEnd), maxBytes); + window.firstLinePreview = { text, bytes: previewBytes }; + } + + if (!doneCollecting) { + const separatorBytes = window.lines.length > 0 ? 1 : 0; + if (window.lines.length >= maxLinesToCollect) { + doneCollecting = true; + } else if (window.lines.length === 0 && lineByteLength > maxBytes) { + window.stoppedByByteLimit = true; + doneCollecting = true; + window.firstLineByteLength ??= lineByteLength; + } else if (window.lines.length > 0 && window.collectedBytes + separatorBytes + lineByteLength > maxBytes) { + window.stoppedByByteLimit = true; + doneCollecting = true; + } else { + window.lines.push(rawSegments[index] ?? ""); + window.collectedBytes += separatorBytes + lineByteLength; + window.firstLineByteLength ??= lineByteLength; + if (window.collectedBytes > maxBytes) { + window.stoppedByByteLimit = true; + doneCollecting = true; + } else if (window.lines.length >= maxLinesToCollect) { + doneCollecting = true; + } + } + } else if (window.firstLineByteLength === undefined) { + window.firstLineByteLength = lineByteLength; + } + + if (doneCollecting && selectedLinesSeen >= selectedLineLimit) break; + lineStart = lineEnd + 1; + } + return window; } interface StreamFileLinesOptions { @@ -139,19 +322,7 @@ async function streamLinesFromFile( selectedLineLimit: number | null, signal?: AbortSignal, options: StreamFileLinesOptions = {}, -): Promise<{ - lines: string[]; - totalFileLines: number; - collectedBytes: number; - stoppedByByteLimit: boolean; - firstLinePreview?: { text: string; bytes: number }; - firstLineByteLength?: number; - selectedBytesTotal: number; - /** Whether the fully scanned source ended in a newline. */ - hasTrailingNewline: boolean; - /** False when `stopScanAfterCollect` cut the scan short — `totalFileLines` is then a lower bound. */ - reachedEof: boolean; -}> { +): Promise { const { includeTerminalNewline = false, stopScanAfterCollect = false } = options; const bufferChunk = Buffer.allocUnsafe(READ_CHUNK_SIZE); const collectedLines: string[] = []; @@ -168,7 +339,6 @@ async function streamLinesFromFile( let firstLinePreviewBytes = 0; const firstLinePreviewChunks: Buffer[] = []; let firstLineByteLength: number | undefined; - let selectedBytesTotal = 0; let selectedLinesSeen = 0; let captureLine = false; let discardLineChunks = false; @@ -219,7 +389,6 @@ async function streamLinesFromFile( const finalizeLine = () => { if (lineIndex >= startLine && (selectedLineLimit === null || selectedLinesSeen < selectedLineLimit)) { - selectedBytesTotal += currentLineLength + (selectedLinesSeen > 0 ? 1 : 0); selectedLinesSeen++; } @@ -337,7 +506,6 @@ async function streamLinesFromFile( stoppedByByteLimit, firstLinePreview, firstLineByteLength, - selectedBytesTotal, reachedEof, hasTrailingNewline: reachedEof && endedWithNewline, }; @@ -659,15 +827,17 @@ export class ReadTool implements AgentTool { } /** - * Stream multiple non-contiguous ranges from a local file. ACP bridge takes - * priority when present (editor buffer is source of truth); otherwise each - * range is streamed independently with its own line/byte budget. Out-of-bounds - * ranges surface as inline notices rather than aborting the read. + * Render multiple non-contiguous ranges of a local file. ACP bridge takes + * priority when present (editor buffer is source of truth); otherwise ranges + * are sliced out of `buffered` when the caller already materialized the file, + * and streamed independently with their own line/byte budget when it did not. + * Out-of-bounds ranges surface as inline notices rather than aborting the read. */ async #readLocalFileMultiRange( absolutePath: string, ranges: readonly LineRange[], fileSize: number, + buffered: BufferedFileText | undefined, parsed: ParsedSelector, displayMode: { hashLines: boolean; lineNumbers: boolean }, suffixResolution: { from: string; to: string } | undefined, @@ -715,7 +885,7 @@ export class ReadTool implements AgentTool { const notices: string[] = []; const visibleSpans: Array<{ startLine: number; endLine: number }> = []; const displayLineByNumber = new Map(); - const fullLines = rawSelector ? undefined : await readBracketContextFullLines(absolutePath, fileSize); + const fullLines = rawSelector ? undefined : buffered?.addressableLines; let columnTruncated = 0; let displayContent: { text: string; startLine: number; lineNumbers?: Array } | undefined; @@ -724,27 +894,25 @@ export class ReadTool implements AgentTool { const requestedLength = range.endLine !== undefined ? range.endLine - range.startLine + 1 : this.#defaultLimit; const maxLines = Math.min(requestedLength, DEFAULT_MAX_LINES); - // When the full file is already in memory (the common case for files - // within the snapshot byte cap), slice ranges from it instead of - // re-streaming the file once per range. + // The file is already in memory for everything within the snapshot byte + // cap, so slice ranges out of it instead of re-streaming per range. Raw + // mode cannot use the addressable lines (it keeps CR bytes and the + // terminal newline sentinel) but still slices the same buffer. let collectedLines: string[]; let totalFileLines: number; + const maxBytesForRead = Math.max(DEFAULT_MAX_BYTES, maxLines * 512); if (fullLines) { totalFileLines = fullLines.length; collectedLines = fullLines.slice(rangeStart, rangeStart + maxLines); } else { - const maxBytesForRead = Math.max(DEFAULT_MAX_BYTES, maxLines * 512); - const streamResult = await streamLinesFromFile( - absolutePath, - rangeStart, - maxLines, - maxBytesForRead, - maxLines, - signal, - { includeTerminalNewline: rawSelector, stopScanAfterCollect: fileSize > SNAPSHOT_MAX_BYTES }, - ); - totalFileLines = streamResult.totalFileLines; - collectedLines = streamResult.lines; + const window = buffered + ? collectLineWindowFromBuffer(buffered, rangeStart, maxLines, maxBytesForRead, maxLines, rawSelector) + : await streamLinesFromFile(absolutePath, rangeStart, maxLines, maxBytesForRead, maxLines, signal, { + includeTerminalNewline: rawSelector, + stopScanAfterCollect: fileSize > SNAPSHOT_MAX_BYTES, + }); + totalFileLines = window.totalFileLines; + collectedLines = window.lines; } if (rangeStart >= totalFileLines) { @@ -786,7 +954,7 @@ export class ReadTool implements AgentTool { const entries = buildLineEntriesWithBlockContext( fullLines, visibleSpans, - { path: absolutePath }, + { path: absolutePath, text: buffered?.normalizedText }, { lineText: (lineNumber, sourceText) => { const visibleText = displayLineByNumber.get(lineNumber); @@ -811,14 +979,26 @@ export class ReadTool implements AgentTool { outputText = blocks.join("\n\n…\n\n"); } if (shouldAddHashLines && outputText) { - const tag = await recordFileSnapshot(this.session, absolutePath); + const tag = buffered + ? getFileSnapshotStore(this.session).record(canonicalSnapshotKey(absolutePath), buffered.normalizedText) + : await recordFileSnapshot(this.session, absolutePath); if (tag) { recordSeenLinesFromBody(this.session, absolutePath, tag, outputText); outputText = `${formatReadHashlineHeader(formatPathRelativeToCwd(absolutePath, this.session.cwd), tag)}\n${outputText}`; } } else if (rawSelector && visibleSpans.length > 0) { const rawSeenLines = lineNumbersFromSpans(visibleSpans); - if (rawSeenLines.length > 0) await recordFileSnapshot(this.session, absolutePath, rawSeenLines); + if (rawSeenLines.length > 0) { + if (buffered) { + getFileSnapshotStore(this.session).record( + canonicalSnapshotKey(absolutePath), + buffered.normalizedText, + rawSeenLines, + ); + } else { + await recordFileSnapshot(this.session, absolutePath, rawSeenLines); + } + } } if (notices.length > 0) { outputText = outputText ? `${outputText}\n${notices.join("\n")}` : notices.join("\n"); @@ -1161,6 +1341,12 @@ export class ReadTool implements AgentTool { content = [{ type: "text", text: `[Cannot read ${ext} file: conversion failed]` }]; } } else { + // One read for every consumer below. The sniff, the structural summary, + // the rendered window, bracket context and the snapshot hash all want + // the same bytes; past the snapshot cap nothing wants the whole file, + // so the streaming reader keeps that case cheap. + const buffered = fileSize <= SNAPSHOT_MAX_BYTES ? await loadBufferedFileText(absolutePath) : undefined; + // 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, @@ -1168,7 +1354,12 @@ export class ReadTool implements AgentTool { // 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))) { + const looksBinary = + !isRawSelector(parsed) && + (buffered + ? isProbablyBinaryHeader(buffered.bytes.subarray(0, BINARY_SNIFF_BYTES)) + : await isProbablyBinary(absolutePath)); + if (looksBinary) { return toolResult({ resolvedPath: absolutePath, suffixResolution }) .text( prependSuffixResolutionNotice( @@ -1185,7 +1376,7 @@ export class ReadTool implements AgentTool { this.session.settings.get("read.summarize.enabled") && (this.session.settings.get("read.summarize.prose") || !isProseSummaryPath(absolutePath)) ) { - const summary = await trySummarize(this.session, absolutePath, fileSize, signal); + const summary = await trySummarize(this.session, absolutePath, fileSize, signal, buffered?.strippedText); if (summary?.parsed && summary.elided) { const renderedSummary = renderSummary(this.session, summary); const footer = formatSummaryElisionFooter( @@ -1194,7 +1385,14 @@ export class ReadTool implements AgentTool { renderedSummary.elidedLines, ); const summaryHashContext = displayMode.hashLines - ? await readHashlineHeaderContext(this.session, absolutePath, this.session.cwd) + ? buffered + ? hashlineHeaderContextForText( + this.session, + absolutePath, + this.session.cwd, + buffered.normalizedText, + ) + : await readHashlineHeaderContext(this.session, absolutePath, this.session.cwd) : undefined; const bodyText = footer ? `${renderedSummary.text}\n\n${footer}` : renderedSummary.text; const modelText = prependHashlineHeader(bodyText, summaryHashContext); @@ -1221,6 +1419,7 @@ export class ReadTool implements AgentTool { absolutePath, parsed.ranges, fileSize, + buffered, parsed, displayMode, suffixResolution, @@ -1285,15 +1484,24 @@ export class ReadTool implements AgentTool { // Assume ~512 bytes/line average; never go below the shared default. const maxBytesForRead = Math.max(DEFAULT_MAX_BYTES, maxLinesToCollect * 512); - const streamResult = await streamLinesFromFile( - absolutePath, - startLine, - maxLinesToCollect, - maxBytesForRead, - selectedLineLimit, - undefined, // plain-file read: deterministic and fast, never abort mid-read - { includeTerminalNewline: rawSelector, stopScanAfterCollect: fileSize > SNAPSHOT_MAX_BYTES }, - ); + const lineWindow = buffered + ? collectLineWindowFromBuffer( + buffered, + startLine, + maxLinesToCollect, + maxBytesForRead, + selectedLineLimit, + rawSelector, + ) + : await streamLinesFromFile( + absolutePath, + startLine, + maxLinesToCollect, + maxBytesForRead, + selectedLineLimit, + undefined, // plain-file read: deterministic and fast, never abort mid-read + { includeTerminalNewline: rawSelector, stopScanAfterCollect: fileSize > SNAPSHOT_MAX_BYTES }, + ); const { lines: collectedLines, @@ -1304,7 +1512,7 @@ export class ReadTool implements AgentTool { firstLineByteLength, reachedEof, hasTrailingNewline, - } = streamResult; + } = lineWindow; // Check if offset is out of bounds - return graceful message instead of throwing if (requestedStart >= totalFileLines) { @@ -1347,9 +1555,7 @@ export class ReadTool implements AgentTool { for (let i = 0; i < displayLines.length; i++) { displayLineByNumber.set(startLineDisplay + i, displayLines[i] ?? ""); } - const bracketContextFullLines = rawSelector - ? undefined - : await readBracketContextFullLines(absolutePath, fileSize); + const bracketContextFullLines = rawSelector ? undefined : buffered?.addressableLines; const displayedEndLine = startLineDisplay + Math.max(0, displayLines.length - 1); const selectedContent = displayLines.join("\n"); @@ -1376,17 +1582,22 @@ export class ReadTool implements AgentTool { const shouldAddLineNumbers = rawSelector ? false : shouldAddHashLines ? false : displayMode.lineNumbers; let hashContext: HashlineHeaderContext | undefined; if (shouldAddHashLines && collectedLines.length > 0 && !firstLineExceedsLimit) { - // The tag is a content hash of the WHOLE file. A whole-file read - // already holds every line in memory; a range read re-reads the - // file (bounded by SNAPSHOT_MAX_BYTES) so the tag fingerprints the - // full file and any anchor validates while the file is unchanged. + // The tag is a content hash of the WHOLE file, so any anchor the + // model returns validates while the live file is unchanged. The + // buffered text is that whole file; above the snapshot cap only a + // non-truncated whole-file window can supply it. const isWholeFile = offset === undefined && limit === undefined && !wasTruncated; - const tag = isWholeFile + const tag = buffered ? getFileSnapshotStore(this.session).record( canonicalSnapshotKey(absolutePath), - normalizeToLF(`${collectedLines.join("\n")}${hasTrailingNewline ? "\n" : ""}`), + buffered.normalizedText, ) - : await recordFileSnapshot(this.session, absolutePath); + : isWholeFile + ? getFileSnapshotStore(this.session).record( + canonicalSnapshotKey(absolutePath), + normalizeToLF(`${collectedLines.join("\n")}${hasTrailingNewline ? "\n" : ""}`), + ) + : await recordFileSnapshot(this.session, absolutePath); if (tag) { hashContext = hashlineHeaderContext(formatPathRelativeToCwd(absolutePath, this.session.cwd), tag); } @@ -1413,7 +1624,7 @@ export class ReadTool implements AgentTool { const entries = buildLineEntriesWithBlockContext( bracketContextFullLines, [{ startLine: startLineDisplay, endLine: displayedEndLine }], - { path: absolutePath }, + { path: absolutePath, text: buffered?.normalizedText }, { lineText: (lineNumber, sourceText) => { const visibleText = displayLineByNumber.get(lineNumber); @@ -1500,11 +1711,18 @@ export class ReadTool implements AgentTool { recordSeenLinesFromBody(this.session, absolutePath, hashContext.tag, outputText); } if (rawSelector && !firstLineExceedsLimit && collectedLines.length > 0) { - await recordFileSnapshot( - this.session, - absolutePath, - contiguousLineNumbers(startLineDisplay, collectedLines.length), - ); + // A raw read emits no header, but recording the range it displayed + // lets a same-content hashline tag inherit its provenance. + const seenLines = contiguousLineNumbers(startLineDisplay, collectedLines.length); + if (buffered) { + getFileSnapshotStore(this.session).record( + canonicalSnapshotKey(absolutePath), + buffered.normalizedText, + seenLines, + ); + } else { + await recordFileSnapshot(this.session, absolutePath, seenLines); + } } if (capturedDisplayContent) { @@ -1685,10 +1903,14 @@ export class ReadTool implements AgentTool { const rawSelector = isRawSelector(parsedSel); const displayMode = resolveFileDisplayMode(this.session, { raw: rawSelector, immutable: true }); if (isMultiRange(parsedSel) && parsedSel.kind === "lines") { + // Bracket context and per-range slicing both want the whole artifact, so + // materialize it once exactly as the plain-file path does. + const buffered = artifact.size <= SNAPSHOT_MAX_BYTES ? await loadBufferedFileText(artifact.path) : undefined; const read = await this.#readLocalFileMultiRange( artifact.path, parsedSel.ranges, artifact.size, + buffered, parsedSel, displayMode, undefined, diff --git a/packages/coding-agent/src/utils/block-context.ts b/packages/coding-agent/src/utils/block-context.ts index 5b450cf97..910c4d288 100644 --- a/packages/coding-agent/src/utils/block-context.ts +++ b/packages/coding-agent/src/utils/block-context.ts @@ -22,6 +22,14 @@ export interface LineSpan { export interface BlockContextSource { path?: string; lang?: string; + /** + * The whole source `fullLines` was split from, when the caller still holds it. + * Supplying it skips re-joining every line into a fresh whole-file string on + * the way to the parser. It MUST be the same content as `fullLines`; a + * differing trailing newline is the only tolerated variation, since it moves + * no node's line number. + */ + text?: string; } export type LineEntry = { kind: "line"; lineNumber: number; text: string; context: boolean } | { kind: "ellipsis" }; @@ -105,7 +113,7 @@ function nativeBlockContext( let boundaries: number[] | null; try { boundaries = enclosingBlockBoundaries({ - code: fullLines.join("\n"), + code: source.text ?? fullLines.join("\n"), path: source.path, lang: source.lang, ranges, diff --git a/packages/coding-agent/test/read-single-pass.test.ts b/packages/coding-agent/test/read-single-pass.test.ts new file mode 100644 index 000000000..3d4150632 --- /dev/null +++ b/packages/coding-agent/test/read-single-pass.test.ts @@ -0,0 +1,175 @@ +/** + * The local text read path materializes a file once and derives every view from + * those bytes: the binary sniff, the rendered window and its byte accounting, + * bracket context, and the whole-file snapshot hash. These tests pin the parts + * of that contract a plausible rewrite would silently break — the decode the + * snapshot tag is hashed from, exact on-disk byte counts, the raw terminal + * newline sentinel, and the absence of a second whole-file read. + */ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Patch, Patcher } from "@oh-my-pi/hashline"; +import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { getFileSnapshotStore } from "@oh-my-pi/pi-coding-agent/edit/file-snapshot-store"; +import { HashlineFilesystem } from "@oh-my-pi/pi-coding-agent/edit/hashline/filesystem"; +import { writethroughNoop } from "@oh-my-pi/pi-coding-agent/lsp"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import type { ReadToolDetails } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { formatBytes } from "@oh-my-pi/pi-coding-agent/tools/render-utils"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; + +function textOutput(result: AgentToolResult): string { + return result.content + .filter(c => c.type === "text") + .map(c => c.text) + .join("\n"); +} + +function createSession(cwd: string): ToolSession { + const settings = Settings.isolated(); + // Structural summarization would answer whole-file reads from the summarizer + // instead of the range path under test. + settings.set("read.summarize.enabled", false); + return { + cwd, + hasUI: false, + getSessionFile: () => path.join(cwd, "session.jsonl"), + getSessionSpawns: () => "*", + getArtifactsDir: () => path.join(cwd, "artifacts"), + allocateOutputArtifact: async () => ({ id: "artifact-1", path: path.join(cwd, "artifact-1.log") }), + settings, + } as ToolSession; +} + +describe("read tool single-pass file access", () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "read-single-pass-")); + }); + + afterEach(async () => { + await removeWithRetries(tmpDir); + }); + + it("hashes the snapshot from BOM-stripped text so a whole-file tag validates without recovery", async () => { + // `Bun.file().text()` strips a leading BOM and the patcher's live read goes + // through it, so a tag hashed from BOM-bearing text only ever applies via + // stale-hash recovery — which tells the model the file changed externally + // when nothing changed. + const filePath = path.join(tmpDir, "bom.ts"); + await fs.writeFile(filePath, Buffer.from("\uFEFFexport const a = 1;\nexport const b = 2;\n", "utf-8")); + + const session = createSession(tmpDir); + const header = textOutput(await new ReadTool(session).execute("bom-read", { path: filePath })).split("\n")[0]; + expect(header).toMatch(/^\[bom\.ts#[0-9A-F]{4}\]$/); + + const patcher = new Patcher({ + fs: new HashlineFilesystem({ + session, + writethrough: writethroughNoop, + beginDeferredDiagnosticsForPath: () => { + throw new Error("deferred diagnostics are unused"); + }, + }), + snapshots: getFileSnapshotStore(session), + }); + const applied = await patcher.apply(Patch.parse(`${header}\nPUT 2.=2:\n+export const b = 22;`, { cwd: tmpDir })); + + expect(applied.sections[0]?.warnings).toEqual([]); + // The BOM survives the write; only the addressed line changed. + expect(await fs.readFile(filePath, "utf8")).toBe("\uFEFFexport const a = 1;\nexport const b = 22;\n"); + }); + + it("reports on-disk byte lengths for a line that is not valid UTF-8", async () => { + // Decoding replaces each stray byte with U+FFFD, which re-encodes to three + // bytes. Measuring the decoded string instead of the buffer would inflate + // every reported length by two bytes per stray byte. + const strayBytes = 512; + const lineBytes = 60 * 1024; + const filePath = path.join(tmpDir, "invalid-utf8.txt"); + await fs.writeFile( + filePath, + Buffer.concat([ + // Stray bytes sit past the 8KiB binary sniff window, so the file still + // reads as text and reaches the oversized-line notice. + Buffer.from("z".repeat(lineBytes - strayBytes), "utf-8"), + Buffer.from(Array.from({ length: strayBytes }, () => 0xff)), + Buffer.from("\ntail\n", "utf-8"), + ]), + ); + + // `:1-1` keeps the byte budget at its 50KB floor, which the 60KB line exceeds. + const text = textOutput(await new ReadTool(createSession(tmpDir)).execute("bytes", { path: `${filePath}:1-1` })); + + expect(text).toContain(`[Line 1 is ${formatBytes(lineBytes)}, exceeds ${formatBytes(50 * 1024)} limit`); + expect(text).not.toContain(formatBytes(lineBytes + strayBytes * 2)); + }); + + it("counts the terminal newline as an addressable line only in raw mode", async () => { + const filePath = path.join(tmpDir, "trailing.txt"); + await fs.writeFile(filePath, "alpha\nbeta\n"); + const tool = new ReadTool(createSession(tmpDir)); + + // Non-raw: the trailing LF closes line 2 rather than opening line 3. + expect(textOutput(await tool.execute("beyond", { path: `${filePath}:3` }))).toBe( + "Line 3 is beyond end of file (2 lines total). Use :1 to read from the start, or :2 to read the last line.", + ); + // Raw: the sentinel is addressable, so line 3 exists and is empty. + expect(textOutput(await tool.execute("raw-sentinel", { path: `${filePath}:raw:3` }))).toBe(""); + expect(textOutput(await tool.execute("raw-beyond", { path: `${filePath}:raw:4` }))).toContain( + "beyond end of file (3 lines total)", + ); + }); + + it("does not re-read a file it already materialized", async () => { + // Bracket context and the snapshot hash each used to pull the whole file + // through `Bun.file(path).text()`, so a ranged read of one file opened it + // four times. Any reintroduced whole-file re-read trips this counter. + const filePath = path.join(tmpDir, "counted.ts"); + await fs.writeFile( + filePath, + `${Array.from({ length: 400 }, (_, i) => `export const value${i} = ${i};`).join("\n")}\n`, + ); + + type FileFactory = typeof Bun.file; + const originalFile: FileFactory = Bun.file; + const bunNamespace = Bun as unknown as { file: FileFactory }; + let wholeFileReads = 0; + bunNamespace.file = ((target: Parameters[0], ...rest: unknown[]) => { + const factory = originalFile as unknown as (t: unknown, ...r: unknown[]) => Bun.BunFile; + const handle = factory(target, ...rest); + if (typeof target !== "string" || target !== filePath) return handle; + return new Proxy(handle, { + // Receiver must be the real BunFile: native accessors such as `size` + // throw when `this` is the proxy. + get(obj, prop) { + const value = Reflect.get(obj, prop); + if (prop === "text" || prop === "bytes" || prop === "arrayBuffer") { + const reader = value as (...a: unknown[]) => Promise; + return (...args: unknown[]) => { + wholeFileReads++; + return reader.apply(obj, args); + }; + } + return typeof value === "function" ? value.bind(obj) : value; + }, + }); + }) as FileFactory; + + try { + const tool = new ReadTool(createSession(tmpDir)); + const text = textOutput(await tool.execute("counted", { path: `${filePath}:100-120` })); + expect(text).toContain("export const value100 = 100;"); + expect(text).toMatch(/^\[counted\.ts#[0-9A-F]{4}\]$/m); + } finally { + bunNamespace.file = originalFile; + } + + expect(wholeFileReads).toBe(0); + }); +}); diff --git a/packages/utils/src/binary.ts b/packages/utils/src/binary.ts index 1245bd062..e96e8da63 100644 --- a/packages/utils/src/binary.ts +++ b/packages/utils/src/binary.ts @@ -14,8 +14,12 @@ */ import { peekFile, peekFileSync } from "./peek-file"; -/** Header window sniffed for the binary heuristic; mirrors git's 8000-byte scan. */ -const BINARY_SNIFF_BYTES = 8192; +/** + * Header window sniffed for the binary heuristic; mirrors git's 8000-byte scan. + * Exported so callers that already hold the whole file in memory can sniff the + * identical prefix through {@link isProbablyBinaryHeader} instead of reopening. + */ +export const BINARY_SNIFF_BYTES = 8192; /** * Classify an in-memory byte header as binary (non-UTF-8-text). From bce0b35e44cd718d7dddd2960e5ebd194c6e617f Mon Sep 17 00:00:00 2001 From: Sunil Srivatsa Date: Mon, 17 Aug 2026 08:27:56 -0700 Subject: [PATCH 2/3] docs(changelog): record the read single-pass change --- packages/coding-agent/CHANGELOG.md | 7 +++++++ packages/utils/CHANGELOG.md | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2f7b500ff..1d9949caf 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -7,6 +7,13 @@ ### Changed - Send the `omp/` User-Agent on xAI chat (`xai` and `xai-oauth`) unless the request already set its own ([#8840](https://github.com/can1357/oh-my-pi/pull/8840) by [@Jaaneek](https://github.com/Jaaneek)). +### Changed + +- The `read` tool now materializes a local text file once per invocation instead of once per consumer. A ranged read of a file within the snapshot cap previously cost four opens and three UTF-8 decodes — an 8KiB binary sniff, a streaming scan for the rendered window, a whole-file read for bracket context, and another whole-file read to hash the snapshot — with two of those readers separately normalizing line endings; whole-file reads under the structural summarizer paid a fifth read. Byte counts and truncation boundaries are now measured on the buffered bytes, so they stay exact for content that is not valid UTF-8. Files above the snapshot cap keep streaming, since nothing on that path wants the whole file. Raw reads, which skip the tree-sitter parse that dominates other reads, are 30-45% faster (2.7MB file: 10.3ms to 5.7ms). + +### Fixed + +- Fixed a whole-file `read` of a file with a UTF-8 BOM minting a hashline tag hashed from BOM-bearing text. Because the patcher's live read strips the BOM, the next edit to that file only applied through stale-hash recovery and reported that the file had changed externally when it had not. ## [17.3.6] - 2026-08-17 diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index a1ca33818..8d67d0fec 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Exported `BINARY_SNIFF_BYTES`, the header window `isProbablyBinary` sniffs, so a caller holding the whole file in memory can classify the identical prefix through `isProbablyBinaryHeader` instead of reopening the file. + ## [17.3.5] - 2026-08-16 ### Fixed From 5e4757f187df32163dbdc099366254a68813ba50 Mon Sep 17 00:00:00 2001 From: Sunil Srivatsa Date: Mon, 17 Aug 2026 14:44:02 -0700 Subject: [PATCH 3/3] refactor(read): sniff the bytes before decoding them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review noted that a file at or below the snapshot cap with a text-like extension but binary content was fully decoded into three string views and a line array before the sniff rejected it — roughly three times the file size in transient allocations for output that is thrown away. Split the loader: read the bytes, sniff those bytes, and derive the views only for what survives. A refused 4MiB binary now costs one read and no decode. No observable change: the 966-case differential against the base commit is byte-identical to the run before this split, still differing only in the four intended BOM snapshot tags. Also name in BlockContextSource.text the one shape that would violate its same-content contract — a source object reused across two different line arrays — since the contract is documented rather than enforced. --- packages/coding-agent/src/tools/read.ts | 40 ++++++++++++------- .../coding-agent/src/utils/block-context.ts | 6 +++ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 66409ba0b..1d9352da4 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -160,9 +160,24 @@ interface BufferedFileText { } /** - * Read `absolutePath` once and derive every view of it the read path needs. - * Returns `undefined` when the bytes cannot be read, which drops the caller - * back to the streaming reader and reproduces today's error surface. + * Read the whole file, or `undefined` when the bytes cannot be read — which + * drops the caller back to the streaming reader and reproduces today's error + * surface. + * + * Kept separate from {@link deriveBufferedFileText} so the binary sniff can run + * on the bytes first: a file that decodes to mojibake is refused, and building + * three string views of it before finding that out would be pure waste. + */ +async function readWholeFile(absolutePath: string): Promise { + try { + return await fs.readFile(absolutePath); + } catch { + return undefined; + } +} + +/** + * Derive every view of `bytes` the read path needs, decoding exactly once. * * `Bun.file(path).text()` strips a leading BOM while `Buffer.toString` keeps it, * and the snapshot store plus the patcher's live-file read both go through the @@ -170,13 +185,7 @@ interface BufferedFileText { * that decode for hashing while {@link BufferedFileText.rawText} stays verbatim * for the emitted lines and their byte accounting. */ -async function loadBufferedFileText(absolutePath: string): Promise { - let bytes: Buffer; - try { - bytes = await fs.readFile(absolutePath); - } catch { - return undefined; - } +function deriveBufferedFileText(bytes: Buffer): BufferedFileText { const rawText = bytes.toString("utf-8"); const strippedText = rawText.charCodeAt(0) === 0xfeff ? rawText.slice(1) : rawText; // `normalizeToLF` allocates a copy; skip it outright for the common LF file. @@ -1345,7 +1354,7 @@ export class ReadTool implements AgentTool { // the rendered window, bracket context and the snapshot hash all want // the same bytes; past the snapshot cap nothing wants the whole file, // so the streaming reader keeps that case cheap. - const buffered = fileSize <= SNAPSHOT_MAX_BYTES ? await loadBufferedFileText(absolutePath) : undefined; + const wholeFileBytes = fileSize <= SNAPSHOT_MAX_BYTES ? await readWholeFile(absolutePath) : undefined; // Binary sniff before any UTF-8 text materialization. A binary file // (font, object, archive, packed blob) decodes to NUL/control bytes and @@ -1356,8 +1365,8 @@ export class ReadTool implements AgentTool { // covers both the multi-range and single-range disk paths below. const looksBinary = !isRawSelector(parsed) && - (buffered - ? isProbablyBinaryHeader(buffered.bytes.subarray(0, BINARY_SNIFF_BYTES)) + (wholeFileBytes + ? isProbablyBinaryHeader(wholeFileBytes.subarray(0, BINARY_SNIFF_BYTES)) : await isProbablyBinary(absolutePath)); if (looksBinary) { return toolResult({ resolvedPath: absolutePath, suffixResolution }) @@ -1370,6 +1379,8 @@ export class ReadTool implements AgentTool { .sourcePath(absolutePath) .done(); } + // Decode only what survived the sniff. + const buffered = wholeFileBytes ? deriveBufferedFileText(wholeFileBytes) : undefined; if ( parsed.kind === "none" && @@ -1905,7 +1916,8 @@ export class ReadTool implements AgentTool { if (isMultiRange(parsedSel) && parsedSel.kind === "lines") { // Bracket context and per-range slicing both want the whole artifact, so // materialize it once exactly as the plain-file path does. - const buffered = artifact.size <= SNAPSHOT_MAX_BYTES ? await loadBufferedFileText(artifact.path) : undefined; + const artifactBytes = artifact.size <= SNAPSHOT_MAX_BYTES ? await readWholeFile(artifact.path) : undefined; + const buffered = artifactBytes ? deriveBufferedFileText(artifactBytes) : undefined; const read = await this.#readLocalFileMultiRange( artifact.path, parsedSel.ranges, diff --git a/packages/coding-agent/src/utils/block-context.ts b/packages/coding-agent/src/utils/block-context.ts index 910c4d288..5949ece5b 100644 --- a/packages/coding-agent/src/utils/block-context.ts +++ b/packages/coding-agent/src/utils/block-context.ts @@ -28,6 +28,12 @@ export interface BlockContextSource { * the way to the parser. It MUST be the same content as `fullLines`; a * differing trailing newline is the only tolerated variation, since it moves * no node's line number. + * + * Every current supplier derives both from one buffer in the same breath, so + * the two cannot drift. Do NOT set it on a source object that is reused + * across two different line arrays — a before/after diff pair, say — because + * the boundary lines tree-sitter reports would then be indexed into the wrong + * array and surface off-by-N context rows. */ text?: string; }