smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: revert read-repeat

Browse Source 
This reverts commit 9613076e96.
This commit is contained in:
can1357
2026-06-12 03:10:19 +02:00
parent 3fd0dc3b58
commit b69a2024d3
2 changed files with 1 additions and 194 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/tools/read.ts
+1 -57
View File
@@ -736,17 +736,6 @@ interface ResolvedSqliteReadPath {
/** Per-execute memo of suffix-glob lookups; `null` records a confirmed miss. */
type SuffixMatchCache = Map<string, { absolutePath: string; displayPath: string } | null>;
/**
* 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<typeof readSchema, ReadToolDetails> {
readonly #autoResizeImages: boolean;
readonly #defaultLimit: number;
readonly #inspectImageEnabled: boolean;
/** Successful file reads per resolved base path (selector stripped) this session. */
readonly #readCounts = new Map<string, number>();
constructor(private readonly session: ToolSession) {
const displayMode = resolveFileDisplayMode(session);
@@ -785,19 +772,6 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
});
}
/**
* 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<typeof readSchema, ReadToolDetails> {
ignoreResultLimits?: boolean;
raw?: boolean;
immutable?: boolean;
/** Trailing repeat-read nudge; appended at the very end of the text. */
repeatNotice?: string;
},
): AgentToolResult<ReadToolDetails> {
const displayMode = resolveFileDisplayMode(this.session, { raw: options.raw, immutable: options.immutable });
@@ -1120,9 +1092,6 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
: 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<typeof readSchema, ReadToolDetails> {
entityLabel: string;
raw?: boolean;
immutable?: boolean;
/** Trailing repeat-read nudge; appended at the very end of the text. */
repeatNotice?: string;
},
): AgentToolResult<ReadToolDetails> {
const displayMode = resolveFileDisplayMode(this.session, { raw: options.raw, immutable: options.immutable });
@@ -1210,11 +1177,8 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
}
} 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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
details: { resolvedPath: absolutePath },
sourcePath: absolutePath,
entityLabel: "document",
repeatNotice,
});
}
const { offset, limit } = selToOffsetLimit(parsed);
@@ -2039,7 +1995,6 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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<typeof readSchema, ReadToolDetails> {
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);
packages/coding-agent/test/tools/read-repeat-notice.test.ts
-137
View File
@@ -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<ReadToolDetails>): 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<InternalResource> {
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);
});
});