From b69a2024d3ce0f15bcb457b0ecc89f3aa9e64a5d Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 12 Jun 2026 03:10:19 +0200 Subject: [PATCH] fix: revert read-repeat This reverts commit 9613076e9637e88a23d3c7417ac8c79ecd8be707. --- packages/coding-agent/src/tools/read.ts | 58 +------- .../test/tools/read-repeat-notice.test.ts | 137 ------------------ 2 files changed, 1 insertion(+), 194 deletions(-) delete mode 100644 packages/coding-agent/test/tools/read-repeat-notice.test.ts diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 5918da327..285dbd09b 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -736,17 +736,6 @@ interface ResolvedSqliteReadPath { /** Per-execute memo of suffix-glob lookups; `null` records a confirmed miss. */ type SuffixMatchCache = Map; -/** - * Repeated whole-file reads of the same path pin stale copies in context. - * From this per-session read count onward, file reads carry a trailing nudge - * to prefer narrower re-reads. - */ -const REPEAT_READ_NOTICE_THRESHOLD = 3; - -function formatRepeatReadNotice(count: number): string { - return `[note: read #${count} of this file this session — after edits, prefer the context echoed in the edit result or a narrow range re-read]`; -} - /** * Read tool implementation. * @@ -765,8 +754,6 @@ export class ReadTool implements AgentTool { readonly #autoResizeImages: boolean; readonly #defaultLimit: number; readonly #inspectImageEnabled: boolean; - /** Successful file reads per resolved base path (selector stripped) this session. */ - readonly #readCounts = new Map(); constructor(private readonly session: ToolSession) { const displayMode = resolveFileDisplayMode(session); @@ -785,19 +772,6 @@ export class ReadTool implements AgentTool { }); } - /** - * Count a file read of `absolutePath` and return the repeat-read nudge once - * the per-session count reaches {@link REPEAT_READ_NOTICE_THRESHOLD}. - * Non-file sources (URLs, internal resources, directories, archives, - * SQLite, images) are never counted. - */ - #repeatReadNotice(absolutePath: string): string | undefined { - const count = (this.#readCounts.get(absolutePath) ?? 0) + 1; - this.#readCounts.set(absolutePath, count); - if (count < REPEAT_READ_NOTICE_THRESHOLD) return undefined; - return formatRepeatReadNotice(count); - } - async #tryReadDelimitedPaths( readPath: string, signal?: AbortSignal, @@ -974,8 +948,6 @@ export class ReadTool implements AgentTool { ignoreResultLimits?: boolean; raw?: boolean; immutable?: boolean; - /** Trailing repeat-read nudge; appended at the very end of the text. */ - repeatNotice?: string; }, ): AgentToolResult { const displayMode = resolveFileDisplayMode(this.session, { raw: options.raw, immutable: options.immutable }); @@ -1120,9 +1092,6 @@ export class ReadTool implements AgentTool { : formatLineEntries(buildLineEntries(endLine), startLineDisplay); } - if (options.repeatNotice) { - outputText += `\n${options.repeatNotice}`; - } resultBuilder.text(outputText); if (truncationInfo) { resultBuilder.truncation(truncationInfo.result, truncationInfo.options); @@ -1148,8 +1117,6 @@ export class ReadTool implements AgentTool { entityLabel: string; raw?: boolean; immutable?: boolean; - /** Trailing repeat-read nudge; appended at the very end of the text. */ - repeatNotice?: string; }, ): AgentToolResult { const displayMode = resolveFileDisplayMode(this.session, { raw: options.raw, immutable: options.immutable }); @@ -1210,11 +1177,8 @@ export class ReadTool implements AgentTool { const bound = range.endLine !== undefined ? `${range.startLine}-${range.endLine}` : `${range.startLine}`; notices.push(`[Range ${bound} is beyond end of ${options.entityLabel} (${totalLines} lines total); skipped]`); } - let finalText = + const finalText = notices.length > 0 ? (outputText ? `${outputText}\n${notices.join("\n")}` : notices.join("\n")) : outputText; - if (options.repeatNotice) { - finalText = finalText ? `${finalText}\n${options.repeatNotice}` : options.repeatNotice; - } resultBuilder.text(finalText); return resultBuilder.done(); } @@ -1232,7 +1196,6 @@ export class ReadTool implements AgentTool { parsed: ParsedSelector, displayMode: { hashLines: boolean; lineNumbers: boolean }, suffixResolution: { from: string; to: string } | undefined, - repeatNotice: string | undefined, signal: AbortSignal | undefined, ): Promise<{ outputText: string; @@ -1252,7 +1215,6 @@ export class ReadTool implements AgentTool { sourcePath: absolutePath, entityLabel: "file", raw: rawSelector, - repeatNotice, }); if (suffixResolution) { const notice = `[Path '${suffixResolution.from}' not found; resolved to '${suffixResolution.to}' via suffix match]`; @@ -1934,7 +1896,6 @@ export class ReadTool implements AgentTool { let details: ReadToolDetails = {}; let sourcePath: string | undefined; let columnTruncated = 0; - let repeatNotice: string | undefined; let truncationInfo: | { result: TruncationResult; options: { direction: "head"; startLine?: number; totalFileLines?: number } } | undefined; @@ -1999,13 +1960,11 @@ export class ReadTool implements AgentTool { } } else if (isNotebookPath(absolutePath) && !isRawSelector(parsed)) { const notebookText = await readEditableNotebookText(absolutePath, localReadPath); - repeatNotice = this.#repeatReadNotice(absolutePath); if (isMultiRange(parsed) && parsed.kind === "lines") { return this.#buildInMemoryMultiRangeResult(notebookText, parsed.ranges, { details: { resolvedPath: absolutePath }, sourcePath: absolutePath, entityLabel: "notebook", - repeatNotice, }); } const { offset, limit } = selToOffsetLimit(parsed); @@ -2013,13 +1972,11 @@ export class ReadTool implements AgentTool { details: { resolvedPath: absolutePath }, sourcePath: absolutePath, entityLabel: "notebook", - repeatNotice, }); } else if (shouldConvertWithMarkit) { // Convert document via markit. const result = await convertFileWithMarkit(absolutePath, signal); if (result.ok) { - repeatNotice = this.#repeatReadNotice(absolutePath); // Route the converted markdown through the in-memory text builder // so line-range selectors (`file.pdf:50-100`, `:5-16,40-80`) and // raw mode apply against the converted output. Without this, @@ -2030,7 +1987,6 @@ export class ReadTool implements AgentTool { details: { resolvedPath: absolutePath }, sourcePath: absolutePath, entityLabel: "document", - repeatNotice, }); } const { offset, limit } = selToOffsetLimit(parsed); @@ -2039,7 +1995,6 @@ export class ReadTool implements AgentTool { sourcePath: absolutePath, entityLabel: "document", raw: isRawSelector(parsed), - repeatNotice, }); } else if (result.error) { content = [{ type: "text", text: `[Cannot read ${ext} file: ${result.error || "conversion failed"}]` }]; @@ -2047,7 +2002,6 @@ export class ReadTool implements AgentTool { content = [{ type: "text", text: `[Cannot read ${ext} file: conversion failed]` }]; } } else { - repeatNotice = this.#repeatReadNotice(absolutePath); if ( parsed.kind === "none" && this.session.settings.get("read.summarize.enabled") && @@ -2089,7 +2043,6 @@ export class ReadTool implements AgentTool { parsed, displayMode, suffixResolution, - repeatNotice, undefined, // plain-file read: deterministic and fast, never abort mid-read ); if (multiResult.bridgeResult) return multiResult.bridgeResult; @@ -2113,7 +2066,6 @@ export class ReadTool implements AgentTool { sourcePath: absolutePath, entityLabel: "file", raw: isRawSelector(parsed), - repeatNotice, }); if (suffixResolution) { const notice = `[Path '${suffixResolution.from}' not found; resolved to '${suffixResolution.to}' via suffix match]`; @@ -2415,14 +2367,6 @@ export class ReadTool implements AgentTool { content = [{ type: "text", text: notice }, ...content]; } } - if (repeatNotice) { - // Trailing nudge goes at the very end of the textual result so it never - // disturbs hashline tag headers or inline notices. - const lastText = content.findLast((c): c is TextContent => c.type === "text"); - if (lastText) { - lastText.text = `${lastText.text}\n${repeatNotice}`; - } - } const resultBuilder = toolResult(details).content(content); if (sourcePath) { resultBuilder.sourcePath(sourcePath); diff --git a/packages/coding-agent/test/tools/read-repeat-notice.test.ts b/packages/coding-agent/test/tools/read-repeat-notice.test.ts deleted file mode 100644 index 91c5fc437..000000000 --- a/packages/coding-agent/test/tools/read-repeat-notice.test.ts +++ /dev/null @@ -1,137 +0,0 @@ -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 type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; -import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { - type InternalResource, - type InternalUrl, - InternalUrlRouter, - type ProtocolHandler, -} from "@oh-my-pi/pi-coding-agent/internal-urls"; -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"; - -const NOTICE_RE = - /\[note: read #(\d+) of this file this session — after edits, prefer the context echoed in the edit result or a narrow range re-read\]/; - -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(); - // Deterministic plain-file reads regardless of language heuristics. - settings.set("read.summarize.enabled", false); - // URL reads must never reach the network in tests. - settings.set("fetch.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, - }; -} - -function registerVirtualDoc(content: string): void { - const handler: ProtocolHandler = { - scheme: "virtual", - immutable: true, - async resolve(url: InternalUrl): Promise { - return { - url: url.href, - content, - contentType: "text/plain", - size: Buffer.byteLength(content, "utf-8"), - }; - }, - }; - InternalUrlRouter.instance().register(handler); -} - -function makeNumberedContent(lines: number): string { - return Array.from({ length: lines }, (_, i) => `line ${i + 1}`).join("\n"); -} - -describe("read tool repeat-read notice", () => { - let tmpDir: string; - - beforeEach(async () => { - tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "read-repeat-notice-test-")); - InternalUrlRouter.resetForTests(); - }); - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); - InternalUrlRouter.resetForTests(); - }); - - it("appends the notice on the third read of the same path, not on the first two", async () => { - const filePath = path.join(tmpDir, "sample.txt"); - await fs.writeFile(filePath, makeNumberedContent(10)); - const tool = new ReadTool(createSession(tmpDir)); - - const first = textOutput(await tool.execute("c1", { path: filePath })); - expect(first).toContain("line 1"); - expect(first).not.toMatch(NOTICE_RE); - - const second = textOutput(await tool.execute("c2", { path: filePath })); - expect(second).not.toMatch(NOTICE_RE); - - const third = textOutput(await tool.execute("c3", { path: filePath })); - expect(third).toContain("line 1"); - const match = third.match(NOTICE_RE); - expect(match?.[1]).toBe("3"); - // Appended at the very end of content, after the file body (never - // prepended, so hashline tag headers stay on the first line). - expect(third.trimEnd().endsWith("a narrow range re-read]")).toBe(true); - expect(third.indexOf("line 10")).toBeLessThan(third.search(NOTICE_RE)); - }); - - it("shares one counter across different selectors of the same file", async () => { - const filePath = path.join(tmpDir, "selectors.txt"); - await fs.writeFile(filePath, makeNumberedContent(20)); - const tool = new ReadTool(createSession(tmpDir)); - - const plain = textOutput(await tool.execute("s1", { path: filePath })); - expect(plain).not.toMatch(NOTICE_RE); - - const range = textOutput(await tool.execute("s2", { path: `${filePath}:2-4` })); - expect(range).not.toMatch(NOTICE_RE); - - const raw = textOutput(await tool.execute("s3", { path: `${filePath}:raw` })); - expect(raw.match(NOTICE_RE)?.[1]).toBe("3"); - - const multi = textOutput(await tool.execute("s4", { path: `${filePath}:1-2,5-6` })); - expect(multi.match(NOTICE_RE)?.[1]).toBe("4"); - }); - - it("never adds the notice for https:// or internal :// sources", async () => { - registerVirtualDoc(makeNumberedContent(5)); - const tool = new ReadTool(createSession(tmpDir)); - - for (let i = 1; i <= 4; i++) { - const text = textOutput(await tool.execute(`v${i}`, { path: "virtual://doc" })); - expect(text).toContain("line 1"); - expect(text).not.toMatch(NOTICE_RE); - } - - // https:// exits before any counting (fetch disabled in this session). - await expect(tool.execute("u1", { path: "https://example.com/page" })).rejects.toThrow("URL reads are disabled"); - - // The :// reads above never polluted the per-file counter: a real file - // still needs three reads of its own before the notice appears. - const filePath = path.join(tmpDir, "clean.txt"); - await fs.writeFile(filePath, makeNumberedContent(3)); - const first = textOutput(await tool.execute("f1", { path: filePath })); - expect(first).not.toMatch(NOTICE_RE); - }); -});