From f24afe13f40ea886f46df2c731b3a75ffe44ccc3 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 14 May 2026 23:22:27 +0200 Subject: [PATCH] fix(read): added truncation tally markers and elision recovery footers - Updated shell minimizer line truncation to append `...[+N]` with the count of dropped Unicode scalars when truncation occurs. - Updated read summary rendering to track `elidedLines`, include them in tool details, and append a recovery footer for `:raw` or line-range access whenever elided spans are present. - Updated read-tool prompts/docs/tests to cover the new elision-footers and recovery guidance. Fixes #1046 --- crates/pi-shell/src/minimizer/pipeline.rs | 4 +- crates/pi-shell/src/minimizer/primitives.rs | 46 +++++++++- docs/tools/read.md | 4 +- packages/coding-agent/CHANGELOG.md | 9 ++ .../coding-agent/src/prompts/tools/read.md | 92 +++++++++++-------- packages/coding-agent/src/tools/read.ts | 31 ++++++- .../coding-agent/test/read-summary.test.ts | 37 ++++++++ 7 files changed, 176 insertions(+), 47 deletions(-) diff --git a/crates/pi-shell/src/minimizer/pipeline.rs b/crates/pi-shell/src/minimizer/pipeline.rs index 7e1cf7fbd..9a110247d 100644 --- a/crates/pi-shell/src/minimizer/pipeline.rs +++ b/crates/pi-shell/src/minimizer/pipeline.rs @@ -472,12 +472,12 @@ on_empty = "demo: ok" [[tests.demo]] name = "basic" input = "\u001b[31mfirst\u001b[0m\nDownloading foo\nA_really_long_line_indeed\n" -expected = "first\nA_really_l\u2026\n" +expected = "first\nA_really_l\u2026[+15]\n" "#; let pipeline = compile_one(src); let out = pipeline.apply("\u{1b}[31mfirst\u{1b}[0m\nDownloading foo\nA_really_long_line_indeed\n"); - assert_eq!(out.as_ref(), "first\nA_really_l\u{2026}\n"); + assert_eq!(out.as_ref(), "first\nA_really_l\u{2026}[+15]\n"); } #[test] diff --git a/crates/pi-shell/src/minimizer/primitives.rs b/crates/pi-shell/src/minimizer/primitives.rs index 5f45b1a12..713c9ff58 100644 --- a/crates/pi-shell/src/minimizer/primitives.rs +++ b/crates/pi-shell/src/minimizer/primitives.rs @@ -166,7 +166,17 @@ pub fn compact_listing(input: &str, max_lines: usize) -> String { } /// Truncate a single line to at most `max_chars` characters (Unicode scalars, -/// not bytes). Appends a single `…` marker when truncation happens. +/// not bytes). +/// +/// When truncation happens, appends a `…[+N]` marker where `N` is the number +/// of dropped Unicode scalars. The bracketed tally lets agents and humans +/// distinguish minimizer truncation from genuine `…` in the source data +/// (see issue #1046), and gives a concrete count so the agent can decide +/// whether the missing tail is recoverable inline or needs the +/// `artifact://` footer surfaced by the bash wrapper. +/// +/// `max_chars == 0` is treated as "drop the line"; no marker is emitted in +/// that case since the caller asked for an empty result. pub fn truncate_line(line: &str, max_chars: usize) -> String { if max_chars == 0 { return String::new(); @@ -179,8 +189,11 @@ pub fn truncate_line(line: &str, max_chars: usize) -> String { None => return out, } } - if chars.next().is_some() { - out.push('…'); + let dropped = chars.count(); + if dropped > 0 { + use std::fmt::Write as _; + // 5–6 bytes typical; this avoids pulling `itoa` for a marker tally. + let _ = write!(out, "…[+{dropped}]"); } out } @@ -290,4 +303,31 @@ mod tests { let out = group_by_file("src/a.ts:1:2 error one\nsrc/a.ts:2:3 error two\n", 10); assert_eq!(out, "src/a.ts:\n 1:2 error one\n 2:3 error two\n"); } + + #[test] + fn truncate_line_short_passes_through() { + assert_eq!(truncate_line("hi", 10), "hi"); + } + + #[test] + fn truncate_line_at_exact_length_emits_no_marker() { + assert_eq!(truncate_line("abcde", 5), "abcde"); + } + + #[test] + fn truncate_line_appends_dropped_char_tally() { + // "abcdefghij" (10 chars) capped at 4 drops 6 chars. + assert_eq!(truncate_line("abcdefghij", 4), "abcd\u{2026}[+6]"); + } + + #[test] + fn truncate_line_counts_unicode_scalars_not_bytes() { + // "aaaα" is 4 scalars, 5 bytes. Cap at 2 drops 2 scalars. + assert_eq!(truncate_line("aaaα", 2), "aa\u{2026}[+2]"); + } + + #[test] + fn truncate_line_max_zero_yields_empty() { + assert_eq!(truncate_line("anything", 0), ""); + } } diff --git a/docs/tools/read.md b/docs/tools/read.md index ad7fc8630..71ae882ca 100644 --- a/docs/tools/read.md +++ b/docs/tools/read.md @@ -55,7 +55,7 @@ URL selectors are parsed separately in `packages/coding-agent/src/tools/fetch.ts - URL fields: `url`, `finalUrl`, `contentType`, `method`, `notes` - `truncation` - `displayContent` (unprefixed text + starting line for TUI rendering) - - `summary` (`lines`, `elidedSpans`) for structural summaries + - `summary` (`lines`, `elidedSpans`, `elidedLines`) for structural summaries - `meta` from `packages/coding-agent/src/tools/output-meta.ts` - `details.meta.source` is set to the backing path, URL, or internal URL. - `details.meta.truncation` carries shown range, total lines/bytes, next offset, and optional `artifactId` for cached URL output. @@ -95,7 +95,7 @@ URL selectors are parsed separately in `packages/coding-agent/src/tools/fetch.ts ### Local text files - No selector: if summarization is enabled and the file is small enough, `#trySummarize()` calls `summarizeCode()`. - Guards: file size `<= 2 MiB` (`MAX_SUMMARY_BYTES`), line count `<= 20_000` (`MAX_SUMMARY_LINES`). - - Summary output keeps selected declarations and replaces elided spans with `...`. + - Summary output keeps selected declarations and replaces elided spans with `...`. When at least one span is elided, the text content ends with a footer like `[NN lines across MM elided regions; read :raw or a line range like :1-9999 for verbatim content]` so the agent has a concrete recovery selector instead of a bare marker. - When an elided block sits between matching brace lines, `#renderSummary()` may merge them into one anchored line rather than emitting separate opener/closer lines. - Explicit selector or summarization miss: streamed text read. - Default open-ended limit is `min(session setting read.defaultLimit, DEFAULT_MAX_LINES)`. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4b8fcf8a1..83f9d09d0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,15 @@ ## [Unreleased] +### Changed + +- Changed shell-minimizer per-line truncation marker from a bare `…` to `…[+N]`, where `N` is the count of dropped Unicode scalars. The bracketed tally disambiguates minimizer-driven cuts from genuine `…` characters in the source (paths, JSON, stack traces, etc.) and gives the agent an exact count so it can decide whether the missing tail is recoverable inline or warrants reading the `[raw output: artifact://]` footer the bash wrapper already emits when the minimizer rewrites output. Affects pipeline Stage 5 (`truncate_lines_at` in `defs/*.toml`) and the internal callers in `filters/git.rs`, `filters/listing.rs`, and `filters/lint.rs`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) + +### Fixed + +- Fixed summarized `read` output stalling agents on elided regions by appending an explicit footer like `[NN lines across MM elided regions; read :raw or a line range like :1-9999 for verbatim content]`. The footer fires whenever the structural summarizer elided at least one span, so the model gets a concrete recovery selector instead of having to guess from a bare `...` / `{ .. }` marker. Surfaces `elidedLines` on `ReadToolDetails.summary` alongside the existing `elidedSpans`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) +- Updated the `read` tool prompt to describe the new elision footer and instruct the model to follow `:raw` (or an explicit line range) when the elided body is actually needed, rather than guessing. + ## [15.0.1] - 2026-05-14 ### Breaking Changes diff --git a/packages/coding-agent/src/prompts/tools/read.md b/packages/coding-agent/src/prompts/tools/read.md index ae0b926e6..8712ff14a 100644 --- a/packages/coding-agent/src/prompts/tools/read.md +++ b/packages/coding-agent/src/prompts/tools/read.md @@ -1,46 +1,58 @@ -Reads the content at the specified path or URL. +Read files, directories, archives, SQLite databases, images, documents, internal resources, and web URLs through a single `path` string. -The `read` tool is multi-purpose and more capable than it looks — inspects files, directories, archives, SQLite databases, images, documents (PDF/DOCX/PPTX/XLSX/RTF/EPUB/ipynb), **and URLs**. -- You MUST parallelize reads when exploring related files -- For URLs, `read` fetches the page and returns clean extracted text/markdown by default (reader-mode). It handles HTML pages, GitHub issues/PRs, Stack Overflow, Wikipedia, Reddit, NPM, arXiv, RSS/Atom, JSON endpoints, PDFs, etc. You SHOULD reach for `read` — not a browser/puppeteer tool — for fetching and inspecting web content. +- One tool for filesystem, archives, SQLite, images, documents (PDF/DOCX/PPTX/XLSX/RTF/EPUB/ipynb), internal URIs, and web URLs (reader-mode by default). +- You SHOULD parallelize independent reads when exploring related files. +- You SHOULD reach for `read` — not a browser/puppeteer tool — for fetching web content. + ## Parameters -- `path` — file path or URL (required). Append `:` for line ranges or raw mode (for example `src/foo.ts:50-200` or `src/foo.ts:raw`). + +- `path` — required. Local path, internal URI (`skill://`, `agent://`, `artifact://`, `memory://`, `rule://`, `local://`, `mcp://`), or URL. Append `:` for line ranges, raw mode, or special modes (e.g. `src/foo.ts:50-200`, `src/foo.ts:raw`, `db.sqlite:users:42`). ## Selectors -|`path` suffix|Behavior| -|---|---| -|_(omitted)_|For parseable code files, return a structural summary. Otherwise read from the start (up to {{DEFAULT_LIMIT}} lines).| -|`:50`|Read from line 50 onward| -|`:50-200`|Read lines 50-200| -|`:50+150`|Read 150 lines starting at line 50| -|`:20+1`|Read exactly one line| -|`:5-16,960-973`|Read multiple ranges in one call (comma-separated; ranges sort and merge automatically)| -|`:raw`|Read verbatim text without anchors or summarization| -|`:conflicts`|Return a one-line-per-block index of every merge conflict in the file| +Append `:` to `path`. The bare path falls back to the default mode. -# Filesystem -- Reading a directory path returns a list of dirents. - {{#if IS_HL_MODE}} -- Reading a file with an explicit selector returns lines prefixed with anchors (line+hash): `41th|def alpha():` - {{else}} - {{#if IS_LINE_NUMBER_MODE}} -- Reading a file with an explicit selector returns lines prefixed with line numbers: `41|def alpha():` - {{/if}} - {{/if}} -- Reading a parseable code file without a selector returns a structural summary with signatures/declarations kept and large bodies collapsed to `…`. Use `:raw` or an explicit range such as `:1-9999` for verbatim content. +- _(none)_ — parseable code → structural summary (signatures kept, bodies elided); other files → read from the start (up to {{DEFAULT_LIMIT}} lines). +- `:50` — read from line 50 onward. +- `:50-200` — lines 50–200 inclusive. +- `:50+150` — 150 lines starting at line 50. +- `:20+1` — exactly one line. +- `:5-16,960-973` — multiple ranges in one call (sorted, overlaps merged). +- `:raw` — verbatim text; no anchors, no summary, no line prefixes. +- `:2-4:raw` or `:raw:2-4` — range AND verbatim; the two compose in either order. +- `:conflicts` — one-line-per-block index of every unresolved git merge conflict. -# Inspection +# Files -Extracts text from PDF, Word, PowerPoint, Excel, RTF, EPUB, and Jupyter notebook files. Notebooks are shown as editable `# %% [type] cell:N` text; edits to that text are applied back to the underlying `.ipynb` JSON while preserving notebook metadata where possible. Can inspect images. +- Reading a directory path returns a depth-limited dirent listing. +{{#if IS_HL_MODE}} +- Reading a file with an explicit selector returns lines prefixed with `line+hash` anchors: `41th|def alpha():`. The 2-char hash is a content fingerprint that `edit` / `apply_patch` consume — copy it verbatim, NEVER fabricate. +{{else}} +{{#if IS_LINE_NUMBER_MODE}} +- Reading a file with an explicit selector returns lines prefixed with line numbers: `41|def alpha():`. +{{/if}} +{{/if}} +- Parseable code without a selector returns a **structural summary**: declarations kept, large bodies collapsed to `..` (merged brace pair) or `…` (standalone). Summarized output ends with a footer of the form: -# Directories & Archives + `[NN lines across MM elided regions; read :raw or a line range like :1-9999 for verbatim content]` -Directories and archive roots return a list of entries. Supports `.tar`, `.tar.gz`, `.tgz`, `.zip`. Use `archive.ext:path/inside/archive` to read contents, and append a selector to the archive entry such as `archive.zip:dir/file.ts:50-60`. + If the elided body is what you actually need, re-issue the **exact selector the footer names**. NEVER guess what's inside `..` / `…` — those markers carry no content. -# SQLite Databases +# Documents & Notebooks + +Extracts text from PDF, Word, PowerPoint, Excel, RTF, and EPUB. Notebooks (`.ipynb`) are shown as editable `# %% [type] cell:N` text; edits round-trip back to the underlying JSON preserving notebook metadata. Add `:raw` to a notebook to bypass the converter and read the JSON directly. + +# Images + +Reading an image path returns metadata (mime, bytes, dimensions, channels, alpha). For actual visual analysis, call `inspect_image` with the path and a question describing what to inspect. + +# Archives + +Supports `.tar`, `.tar.gz`, `.tgz`, `.zip`. Use `archive.ext:path/inside/archive` to read a member, and append a normal selector to the inner path: `archive.zip:dir/file.ts:50-60`. + +# SQLite For `.sqlite`, `.sqlite3`, `.db`, `.db3`: - `file.db` — list tables with row counts @@ -52,13 +64,19 @@ For `.sqlite`, `.sqlite3`, `.db`, `.db3`: # URLs -Extracts content from web pages, GitHub issues/PRs, Stack Overflow, Wikipedia, Reddit, NPM, arXiv, RSS/Atom feeds, JSON endpoints, PDFs at URLs, and similar text-based resources. Returns clean reader-mode text/markdown — no browser required. Use a `:raw` suffix for untouched HTML. URL line selectors mirror the file form (`:50`, `:50-100`, `:50+150`, `:raw`). If a URL would otherwise look like `host:port`, add a trailing slash before the selector (e.g. `https://example.com/:80`). - +- Default reader-mode: HTML pages, GitHub issues/PRs, Stack Overflow, Wikipedia, Reddit, NPM, arXiv, RSS/Atom, JSON endpoints, PDFs → clean text/markdown. +- `:raw` returns untouched HTML; line selectors (`:50`, `:50-100`, `:50+150`) paginate the cached fetched output. +- Bare `host:port` URLs collide with the selector grammar — add a trailing slash before the selector: `https://example.com/:80`. + +# Internal URIs + +`skill://`, `agent://`, `artifact://`, `memory://root`, `rule://`, `local://.md`, `mcp://` resolve transparently and accept the same line selectors as filesystem paths. Use `artifact://` to recover full output that a previous bash/eval/tool result spilled or truncated. -- You MUST use `read` for every file, directory, archive, and URL read. `cat`, `head`, `tail`, `less`, `more`, `ls`, `tar`, `unzip`, `curl`, and `wget` are **FORBIDDEN** for inspection — any such Bash call is a bug, regardless of how short or convenient it looks. -- You MUST prefer `read` over a browser/puppeteer tool for fetching URL content; only use a browser if `read` fails to deliver reasonable content. -- You MUST always include the `path` parameter — never call `read` with an empty argument object `{}`. -- For specific line ranges, append the selector to `path` (e.g. `path="src/foo.ts:50-200"`, `path="src/foo.ts:50+150"`) — NEVER reach for `sed -n`, `awk NR`, or `head`/`tail` pipelines. -- You MAY use path suffix selectors with URL reads; the tool paginates cached fetched output. +- You MUST use `read` for every file, directory, archive, and URL inspection. `cat`, `head`, `tail`, `less`, `more`, `ls`, `tar`, `unzip`, `curl`, `wget` are FORBIDDEN — any such bash call is a bug, regardless of how short or convenient it looks. +- You MUST prefer `read` over a browser/puppeteer tool for URL content; only reach for a browser when `read` cannot deliver reasonable content. +- You MUST always include `path`. NEVER call `read` with `{}`. +- For line ranges, append the selector to `path` (`path="src/foo.ts:50-200"`, `path="src/foo.ts:50+150"`). NEVER substitute `sed -n`, `awk NR`, or `head`/`tail` pipelines. +- Summary footer says `read :raw …`? Re-issue the exact selector it names. NEVER guess what's inside `..` / `…` markers — they carry no content. +- You MAY combine selectors with URL reads and internal URIs; both paginate the cached resolved output. diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index c7a110ae0..9f89f611e 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -172,6 +172,19 @@ function countTextLines(text: string): number { if (text.length === 0) return 0; return text.split("\n").length; } + +/** + * Footer appended to summarized reads telling the model how to recover the + * elided body. Without this hint, agents either ignore the `...`/`{ .. }` + * markers or burn a turn guessing the right selector (see issue #1046). + */ +function formatSummaryElisionFooter(readPath: string, elidedSpans: number, elidedLines: number): string { + if (elidedSpans <= 0) return ""; + const spanWord = elidedSpans === 1 ? "region" : "regions"; + const lineWord = elidedLines === 1 ? "line" : "lines"; + const linePart = elidedLines > 0 ? `${elidedLines} ${lineWord} across ` : ""; + return `[${linePart}${elidedSpans} elided ${spanWord}; read ${readPath}:raw or a line range like ${readPath}:1-9999 for verbatim content]`; +} const READ_CHUNK_SIZE = 8 * 1024; /** @@ -484,7 +497,7 @@ export interface ReadToolDetails { * Mirrors the same lines the model receives but without hashline/line-number prefixes, * so the TUI can render the file content with its own gutter without re-parsing the formatted text. */ displayContent?: { text: string; startLine: number }; - summary?: { lines: number; elidedSpans: number }; + summary?: { lines: number; elidedSpans: number; elidedLines: number }; /** Number of unresolved git conflicts surfaced by this read (TUI uses for inline `⚠ N` badge). */ conflictCount?: number; } @@ -1317,6 +1330,7 @@ export class ReadTool implements AgentTool { text: string; displayText: string; elidedSpans: number; + elidedLines: number; } { const displayMode = resolveFileDisplayMode(this.session); const shouldAddHashLines = displayMode.hashLines; @@ -1377,11 +1391,13 @@ export class ReadTool implements AgentTool { const modelParts: string[] = []; const displayParts: string[] = []; let elidedSpans = 0; + let elidedLines = 0; for (const unit of units) { if (unit.kind === "elided") { modelParts.push("..."); displayParts.push("..."); elidedSpans++; + elidedLines += unit.endLine - unit.startLine + 1; continue; } if (unit.kind === "merged") { @@ -1396,13 +1412,15 @@ export class ReadTool implements AgentTool { modelParts.push(formatted.model); displayParts.push(formatted.display); elidedSpans++; + // Merged brace pair encloses (start+1)..(end-1) as elided. + elidedLines += Math.max(0, unit.endLine - unit.startLine - 1); continue; } modelParts.push(formatSingleLine(unit.line, unit.text, shouldAddHashLines, shouldAddLineNumbers)); displayParts.push(unit.text); } - return { text: modelParts.join("\n"), displayText: displayParts.join("\n"), elidedSpans }; + return { text: modelParts.join("\n"), displayText: displayParts.join("\n"), elidedSpans, elidedLines }; } async execute( @@ -1646,16 +1664,23 @@ export class ReadTool implements AgentTool { const summary = await this.#trySummarize(absolutePath, fileSize, signal); if (summary?.parsed && summary.elided) { const renderedSummary = this.#renderSummary(summary); + const footer = formatSummaryElisionFooter( + localReadPath, + renderedSummary.elidedSpans, + renderedSummary.elidedLines, + ); + const modelText = footer ? `${renderedSummary.text}\n\n${footer}` : renderedSummary.text; details = { displayContent: { text: renderedSummary.displayText, startLine: 1 }, summary: { lines: countTextLines(renderedSummary.text), elidedSpans: renderedSummary.elidedSpans, + elidedLines: renderedSummary.elidedLines, }, }; sourcePath = absolutePath; - content = [{ type: "text", text: renderedSummary.text }]; + content = [{ type: "text", text: modelText }]; } } diff --git a/packages/coding-agent/test/read-summary.test.ts b/packages/coding-agent/test/read-summary.test.ts index e339d73bc..13d6384b4 100644 --- a/packages/coding-agent/test/read-summary.test.ts +++ b/packages/coding-agent/test/read-summary.test.ts @@ -239,4 +239,41 @@ describe("read summary", () => { expect(text).toContain("\n...\n"); expect(text).not.toContain(" .. "); }); + + it("appends an elision footer that names the path and `:raw` recovery selector", async () => { + // Regression for issue #1046: summarized reads must tell the model how + // to recover the elided body so it does not stall on `...` / `{ .. }` + // markers and burn a turn guessing the selector. + const fixture = path.join(tmpDir, "footer.ts"); + await fs.writeFile( + fixture, + "export function alpha(value: string): string {\n\tconst clean = value.trim();\n\tconst label = clean || 'alpha';\n\treturn label.toUpperCase();\n}\n\nexport function beta(): number {\n\tconst one = 1;\n\tconst two = 2;\n\treturn one + two;\n}\n", + ); + + const tool = new ReadTool(createSession(tmpDir)); + const result = await tool.execute("read-summary-footer", { path: fixture }); + const text = textOutput(result); + + expect(result.details?.summary?.elidedSpans).toBe(2); + expect(result.details?.summary?.elidedLines).toBeGreaterThan(0); + expect(text).toContain("elided regions"); + expect(text).toContain(`${fixture}:raw`); + expect(text).toContain(`${fixture}:1-9999`); + // Footer must be the LAST block of output so the recovery hint sits + // next to the structural summary it describes. + expect(text.trimEnd().endsWith("for verbatim content]")).toBe(true); + }); + + it("does not append a footer when the file has no elision", async () => { + const fixture = path.join(tmpDir, "noelide.ts"); + await fs.writeFile(fixture, "export const x = 1;\n"); + + const tool = new ReadTool(createSession(tmpDir)); + const result = await tool.execute("read-summary-no-footer", { path: fixture }); + const text = textOutput(result); + + expect(text).not.toContain("elided regions"); + expect(text).not.toContain(":raw"); + expect(result.details?.summary).toBeUndefined(); + }); });