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).