From 33d66643d36c1ffcd4f600eada698430ebe9acfe Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 17 Jul 2026 07:45:06 +0000 Subject: [PATCH] fix(read): refreshed URL responses on each invocation Removed process-local URL response reuse so read and URL-backed search paths fetch current content. Added regressions for repeated reads and searches. Fixes #5803 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/tools/fetch.ts | 122 ++++-------------- packages/coding-agent/src/tools/read.ts | 35 ++--- .../test/tools/fetch-raw-mode.test.ts | 24 ++++ .../test/tools/search-url-paths.test.ts | 31 ++++- 5 files changed, 95 insertions(+), 121 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 493bd5332..c5d92704e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed repeated URL reads and URL-backed searches returning stale same-session responses instead of refetching the resource ([#5803](https://github.com/can1357/oh-my-pi/issues/5803)). + ## [17.0.2] - 2026-07-17 ### Added diff --git a/packages/coding-agent/src/tools/fetch.ts b/packages/coding-agent/src/tools/fetch.ts index 174afdb77..9ac6c9e9c 100644 --- a/packages/coding-agent/src/tools/fetch.ts +++ b/packages/coding-agent/src/tools/fetch.ts @@ -7,7 +7,6 @@ import type { FetchImpl, ImageContent, TextContent } from "@oh-my-pi/pi-ai"; import { htmlToMarkdown } from "@oh-my-pi/pi-natives"; import { type Component, Text } from "@oh-my-pi/pi-tui"; import { $which, ptree, truncate } from "@oh-my-pi/pi-utils"; -import { LRUCache } from "lru-cache/raw"; import type { Settings } from "../config/settings"; import { readEditableNotebookText } from "../edit/notebook"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; @@ -1553,24 +1552,15 @@ export interface ReadUrlToolDetails { meta?: OutputMeta; } -interface ReadUrlCacheEntry { +interface ReadUrlEntry { artifactId?: string; artifactPath?: string; - contentPath?: string; details: ReadUrlToolDetails; image?: FetchImagePayload; output: string; content: string; } -const READ_URL_CACHE_MAX_ENTRIES = 100; -const readUrlCache = new LRUCache({ max: READ_URL_CACHE_MAX_ENTRIES }); - -function getReadUrlCacheKey(session: ToolSession, requestedUrl: string, raw: boolean): string { - const scope = session.getSessionFile() ?? session.cwd; - return `${scope}::${raw ? "raw" : "rendered"}::${normalizeUrl(requestedUrl)}`; -} - async function findArtifactPath(session: ToolSession, artifactId: string): Promise { const artifactsDir = session.getArtifactsDir?.(); if (!artifactsDir) return null; @@ -1584,25 +1574,6 @@ async function findArtifactPath(session: ToolSession, artifactId: string): Promi } } -async function readArtifactOutput(session: ToolSession, artifactId: string): Promise { - const artifactPath = await findArtifactPath(session, artifactId); - return artifactPath ? await Bun.file(artifactPath).text() : null; -} - -async function materializeReadUrlCacheEntry( - session: ToolSession, - entry: ReadUrlCacheEntry, -): Promise { - if (entry.artifactId) { - const artifactOutput = await readArtifactOutput(session, entry.artifactId); - if (artifactOutput !== null) { - return { ...entry, output: artifactOutput }; - } - } - - return entry.output.length > 0 ? entry : null; -} - async function persistReadUrlArtifact( session: ToolSession, output: string, @@ -1613,7 +1584,7 @@ async function persistReadUrlArtifact( return artifact; } -async function ensureReadUrlCacheArtifact(session: ToolSession, entry: ReadUrlCacheEntry): Promise { +async function ensureReadUrlArtifact(session: ToolSession, entry: ReadUrlEntry): Promise { if (entry.artifactId && entry.artifactPath) return entry; if (entry.artifactId) { const artifactPath = await findArtifactPath(session, entry.artifactId); @@ -1632,19 +1603,7 @@ function readUrlContentExtension(finalUrl: string): string { } } -async function ensureReadUrlContentFile( - session: ToolSession, - entry: ReadUrlCacheEntry, - raw: boolean, -): Promise { - if (entry.contentPath) { - try { - await Bun.file(entry.contentPath).stat(); - return entry; - } catch { - // Recreate below when the cached scratch file was removed. - } - } +async function materializeReadUrlContent(session: ToolSession, entry: ReadUrlEntry, raw: boolean): Promise { const root = session.getArtifactsDir?.(); if (!root) { throw new ToolError("Cannot search URL output because this session cannot materialize read artifacts."); @@ -1654,20 +1613,16 @@ async function ensureReadUrlContentFile( const hash = Bun.hash(`${raw ? "raw" : "rendered"}:${entry.details.finalUrl}`).toString(36); const contentPath = path.join(dir, `${hash}${readUrlContentExtension(entry.details.finalUrl)}`); await Bun.write(contentPath, entry.content); - return { ...entry, contentPath }; + return contentPath; } -function cacheReadUrlEntry(session: ToolSession, requestedUrl: string, raw: boolean, entry: ReadUrlCacheEntry): void { - readUrlCache.set(getReadUrlCacheKey(session, requestedUrl, raw), entry); - readUrlCache.set(getReadUrlCacheKey(session, entry.details.finalUrl, raw), entry); -} - -async function buildReadUrlCacheEntry( +/** Fetch and render a URL for a read or search operation. */ +export async function fetchReadUrl( session: ToolSession, params: { path: string; raw?: boolean }, signal?: AbortSignal, options?: { ensureArtifact?: boolean }, -): Promise { +): Promise { const { path: url, raw = false } = params; const effectiveTimeout = clampTimeout("fetch", 30); @@ -1708,30 +1663,6 @@ async function buildReadUrlCacheEntry( }; } -export async function loadReadUrlCacheEntry( - session: ToolSession, - params: { path: string; raw?: boolean }, - signal?: AbortSignal, - options?: { ensureArtifact?: boolean; preferCached?: boolean }, -): Promise { - const raw = params.raw ?? false; - const cached = readUrlCache.get(getReadUrlCacheKey(session, params.path, raw)); - if (options?.preferCached && cached) { - const prepared = options.ensureArtifact ? await ensureReadUrlCacheArtifact(session, cached) : cached; - const materialized = await materializeReadUrlCacheEntry(session, prepared); - if (materialized) { - cacheReadUrlEntry(session, params.path, raw, materialized); - return materialized; - } - } - - const fresh = await buildReadUrlCacheEntry(session, params, signal, { - ensureArtifact: options?.ensureArtifact, - }); - cacheReadUrlEntry(session, params.path, raw, fresh); - return fresh; -} - /** Materialize rendered URL body text to a local file for tools that require filesystem paths. */ export async function materializeReadUrlToFile( session: ToolSession, @@ -1741,13 +1672,9 @@ export async function materializeReadUrlToFile( if (!session.settings.get("fetch.enabled")) { throw new ToolError("URL reads are disabled by settings."); } - const cacheEntry = await loadReadUrlCacheEntry(session, params, signal, { preferCached: true }); - const materialized = await ensureReadUrlContentFile(session, cacheEntry, params.raw ?? false); - cacheReadUrlEntry(session, params.path, params.raw ?? false, materialized); - if (!materialized.contentPath) { - throw new ToolError("Cannot search URL output because this session cannot materialize read artifacts."); - } - return { path: materialized.contentPath, details: materialized.details }; + const entry = await fetchReadUrl(session, params, signal); + const contentPath = await materializeReadUrlContent(session, entry, params.raw ?? false); + return { path: contentPath, details: entry.details }; } function buildUrlReadOutput(result: FetchRenderResult, content: string): string { @@ -1768,36 +1695,35 @@ export async function executeReadUrl( params: { path: string; raw?: boolean }, signal?: AbortSignal, ): Promise> { - let cacheEntry = await loadReadUrlCacheEntry(session, params, signal, { preferCached: true }); - const truncation = truncateHead(cacheEntry.output, { + let entry = await fetchReadUrl(session, params, signal); + const truncation = truncateHead(entry.output, { maxBytes: DEFAULT_MAX_BYTES, maxLines: FETCH_DEFAULT_MAX_LINES, }); const needsArtifact = truncation.truncated; - if (needsArtifact && !cacheEntry.artifactId) { - cacheEntry = await ensureReadUrlCacheArtifact(session, cacheEntry); - cacheReadUrlEntry(session, params.path, params.raw ?? false, cacheEntry); + if (needsArtifact && !entry.artifactId) { + entry = await ensureReadUrlArtifact(session, entry); } - const output = needsArtifact ? truncation.content : cacheEntry.output; + const output = needsArtifact ? truncation.content : entry.output; const details: ReadUrlToolDetails = { - ...cacheEntry.details, - truncated: Boolean(cacheEntry.details.truncated || needsArtifact), + ...entry.details, + truncated: Boolean(entry.details.truncated || needsArtifact), }; const contentBlocks: Array = [{ type: "text", text: output }]; - if (cacheEntry.image) { - contentBlocks.push({ type: "image", data: cacheEntry.image.data, mimeType: cacheEntry.image.mimeType }); + if (entry.image) { + contentBlocks.push({ type: "image", data: entry.image.data, mimeType: entry.image.mimeType }); } const resultBuilder = toolResult(details).content(contentBlocks).sourceUrl(details.finalUrl); if (needsArtifact) { - resultBuilder.truncation(truncation, { direction: "head", artifactId: cacheEntry.artifactId }); - } else if (cacheEntry.details.truncated) { - const outputLines = cacheEntry.output.split("\n").length; - const outputBytes = Buffer.byteLength(cacheEntry.output, "utf-8"); + resultBuilder.truncation(truncation, { direction: "head", artifactId: entry.artifactId }); + } else if (entry.details.truncated) { + const outputLines = entry.output.split("\n").length; + const outputBytes = Buffer.byteLength(entry.output, "utf-8"); const totalBytes = Math.max(outputBytes + 1, MAX_OUTPUT_CHARS + 1); const totalLines = outputLines + 1; - resultBuilder.truncationFromText(cacheEntry.output, { + resultBuilder.truncationFromText(entry.output, { direction: "tail", totalLines, totalBytes, diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 172cc110e..2a403d21b 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -78,7 +78,7 @@ import { } from "./conflict-detect"; import { executeReadUrl, - loadReadUrlCacheEntry, + fetchReadUrl, parseReadUrlTarget, type ReadUrlToolDetails, renderReadUrlCall, @@ -2167,15 +2167,12 @@ export class ReadTool implements AgentTool { const urlRaw = parsedUrlTarget.raw; const urlRanges = parsedUrlTarget.ranges; if (urlRanges !== undefined && urlRanges.length > 1) { - const cached = await loadReadUrlCacheEntry( - this.session, - { path: parsedUrlTarget.path, raw: urlRaw }, - signal, - { ensureArtifact: true, preferCached: true }, - ); - return this.#buildInMemoryMultiRangeResult(cached.output, urlRanges, { - details: { ...cached.details }, - sourceUrl: cached.details.finalUrl, + const entry = await fetchReadUrl(this.session, { path: parsedUrlTarget.path, raw: urlRaw }, signal, { + ensureArtifact: true, + }); + return this.#buildInMemoryMultiRangeResult(entry.output, urlRanges, { + details: { ...entry.details }, + sourceUrl: entry.details.finalUrl, entityLabel: "URL output", raw: urlRaw, immutable: true, @@ -2184,18 +2181,12 @@ export class ReadTool implements AgentTool { const urlOffset = parsedUrlTarget.offset; const urlLimit = parsedUrlTarget.limit; if (urlOffset !== undefined || urlLimit !== undefined) { - const cached = await loadReadUrlCacheEntry( - this.session, - { path: parsedUrlTarget.path, raw: urlRaw }, - signal, - { - ensureArtifact: true, - preferCached: true, - }, - ); - return this.#buildInMemoryTextResult(cached.output, urlOffset, urlLimit, { - details: { ...cached.details }, - sourceUrl: cached.details.finalUrl, + const entry = await fetchReadUrl(this.session, { path: parsedUrlTarget.path, raw: urlRaw }, signal, { + ensureArtifact: true, + }); + return this.#buildInMemoryTextResult(entry.output, urlOffset, urlLimit, { + details: { ...entry.details }, + sourceUrl: entry.details.finalUrl, entityLabel: "URL output", raw: urlRaw, immutable: true, diff --git a/packages/coding-agent/test/tools/fetch-raw-mode.test.ts b/packages/coding-agent/test/tools/fetch-raw-mode.test.ts index 1f7eb5fa5..6310de169 100644 --- a/packages/coding-agent/test/tools/fetch-raw-mode.test.ts +++ b/packages/coding-agent/test/tools/fetch-raw-mode.test.ts @@ -99,6 +99,30 @@ describe("read URL with :raw selector (regression: JSON/feed parsers ignored raw expect(textBlock?.text).toContain('"alpha": 1'); }); + it("refetches the same URL on subsequent reads", async () => { + const session = makeSession(testDir); + const tool = new ReadTool(session); + let body = "v1"; + const loadPage = vi.spyOn(scrapers, "loadPage").mockImplementation(async (requestedUrl: string) => ({ + ok: true, + status: 200, + finalUrl: requestedUrl, + contentType: "text/plain", + content: body, + })); + + const first = await tool.execute("first", { path: "https://example.com/live.txt:raw" }); + body = "v2"; + const second = await tool.execute("second", { path: "https://example.com/live.txt:raw" }); + const firstText = first.content.find(entry => entry.type === "text"); + const secondText = second.content.find(entry => entry.type === "text"); + + expect(firstText?.text).toContain("v1"); + expect(secondText?.text).toContain("v2"); + expect(secondText?.text).not.toContain("v1"); + expect(loadPage).toHaveBeenCalledTimes(2); + }); + it("returns slices of raw content when :raw is combined with a range", async () => { const session = makeSession(testDir); const tool = new ReadTool(session); diff --git a/packages/coding-agent/test/tools/search-url-paths.test.ts b/packages/coding-agent/test/tools/search-url-paths.test.ts index ca81898e0..a09fc2757 100644 --- a/packages/coding-agent/test/tools/search-url-paths.test.ts +++ b/packages/coding-agent/test/tools/search-url-paths.test.ts @@ -61,7 +61,7 @@ describe("search tools with external URL paths", () => { await removeWithRetries(testDir); }); - it("search fetches a URL through the read cache and greps the rendered text", async () => { + it("search fetches a URL and greps the rendered text", async () => { stubLoadPage("alpha\nremote needle\nomega\n", "text/plain"); const tools = await createTools(createSession(testDir)); const tool = tools.find(entry => entry.name === "grep"); @@ -77,6 +77,35 @@ describe("search tools with external URL paths", () => { expect(text).not.toContain("Cannot search external URL"); }); + it("refetches the same URL before each search", async () => { + let body = "first needle\n"; + const loadPage = vi.spyOn(scrapers, "loadPage").mockImplementation(async requestedUrl => ({ + ok: true, + status: 200, + finalUrl: requestedUrl, + contentType: "text/plain", + content: body, + })); + const tools = await createTools(createSession(testDir)); + const tool = tools.find(entry => entry.name === "grep"); + expect(tool).toBeDefined(); + + const first = await tool!.execute("search-url-first", { + pattern: "first|second", + path: "https://example.com/live.txt", + }); + body = "second needle\n"; + const second = await tool!.execute("search-url-second", { + pattern: "first|second", + path: "https://example.com/live.txt", + }); + + expect(resultText(first)).toContain("first needle"); + expect(resultText(second)).toContain("second needle"); + expect(resultText(second)).not.toContain("first needle"); + expect(loadPage).toHaveBeenCalledTimes(2); + }); + it("search applies URL line-range selectors after materialization", async () => { stubLoadPage("outside before\nremote needle\noutside after\n", "text/plain"); const tools = await createTools(createSession(testDir));