From a9bb4c7d59a6ece4742a8d5f7cd87c278e4ec2e4 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 3 Jul 2026 23:08:18 +0000 Subject: [PATCH] fix(read): guarded large artifact raw reads Resolved artifact:// reads to backing files before selector handling, streamed bounded reads, and blocked unbounded raw reads for large artifacts with recovery guidance. Fixes #4482 --- packages/coding-agent/CHANGELOG.md | 4 + .../src/internal-urls/artifact-protocol.ts | 146 +++++++------ .../coding-agent/src/prompts/tools/read.md | 2 +- packages/coding-agent/src/tools/read.ts | 197 +++++++++++++++++- .../test/tools/read-artifact-large.test.ts | 93 +++++++++ 5 files changed, 382 insertions(+), 60 deletions(-) create mode 100644 packages/coding-agent/test/tools/read-artifact-large.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 860266c53..bea609876 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed large `artifact://` reads materializing entire MCP/tool artifacts before selector paging, preventing OOM crashes on unbounded raw reads and surfacing bounded read/search/copy guidance ([#4482](https://github.com/can1357/oh-my-pi/issues/4482)). + ## [16.3.4] - 2026-07-03 ### Fixed diff --git a/packages/coding-agent/src/internal-urls/artifact-protocol.ts b/packages/coding-agent/src/internal-urls/artifact-protocol.ts index cfe2536ae..0993fa7e1 100644 --- a/packages/coding-agent/src/internal-urls/artifact-protocol.ts +++ b/packages/coding-agent/src/internal-urls/artifact-protocol.ts @@ -15,75 +15,105 @@ import { isEnoent } from "@oh-my-pi/pi-utils"; import { artifactsDirsFromRegistry } from "./registry-helpers"; import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext, UrlCompletion } from "./types"; +const MAX_INLINE_ARTIFACT_BYTES = 8 * 1024 * 1024; + +/** Filesystem location for a session artifact, resolved without materializing its content. */ +export interface ResolvedArtifactFile { + id: string; + path: string; + size: number; +} + +function parseArtifactId(url: InternalUrl): string { + const id = url.rawHost || url.hostname; + if (!id) { + throw new Error("artifact:// URL requires a numeric ID: artifact://0"); + } + if (!/^\d+$/.test(id)) { + throw new Error(`artifact:// ID must be numeric, got: ${id}`); + } + return id; +} + +/** Resolve an `artifact://` URL to its backing file without reading artifact bytes. */ +export async function resolveArtifactFile(url: InternalUrl, context?: ResolveContext): Promise { + const id = parseArtifactId(url); + + // Artifact ids are per-session counters; in multi-session hosts the same + // id exists in several dirs. Pin resolution to the calling session's + // artifacts dir first so `artifact://3` means *this* session's #3. + const dirs = artifactsDirsFromRegistry(); + const pinnedDir = context?.localProtocolOptions?.getArtifactsDir?.() ?? null; + if (pinnedDir) { + const pinnedIndex = dirs.indexOf(pinnedDir); + if (pinnedIndex >= 0) dirs.splice(pinnedIndex, 1); + dirs.unshift(pinnedDir); + } + + if (dirs.length === 0) { + throw new Error("No session - artifacts unavailable"); + } + + let foundPath: string | undefined; + let anyDirExists = false; + const availableIds = new Set(); + + for (const dir of dirs) { + let files: string[]; + try { + files = await fs.readdir(dir); + anyDirExists = true; + } catch (err) { + if (isEnoent(err)) continue; + throw err; + } + const match = files.find(f => f.startsWith(`${id}.`)); + if (match) { + foundPath = path.join(dir, match); + break; + } + for (const f of files) { + const m = f.match(/^(\d+)\./); + if (m) availableIds.add(m[1]); + } + } + + if (!anyDirExists) { + throw new Error("No artifacts directory found"); + } + + if (!foundPath) { + const sorted = [...availableIds].sort((a, b) => Number(a) - Number(b)); + const availableStr = sorted.length > 0 ? sorted.join(", ") : "none"; + throw new Error(`Artifact ${id} not found. Available: ${availableStr}`); + } + + const stat = await Bun.file(foundPath).stat(); + if (stat.isDirectory()) { + throw new Error(`Artifact ${id} resolved to a directory, not a file`); + } + return { id, path: foundPath, size: stat.size }; +} + export class ArtifactProtocolHandler implements ProtocolHandler { readonly scheme = "artifact"; readonly immutable = true; async resolve(url: InternalUrl, context?: ResolveContext): Promise { - const id = url.rawHost || url.hostname; - if (!id) { - throw new Error("artifact:// URL requires a numeric ID: artifact://0"); - } - if (!/^\d+$/.test(id)) { - throw new Error(`artifact:// ID must be numeric, got: ${id}`); + const artifact = await resolveArtifactFile(url, context); + if (artifact.size > MAX_INLINE_ARTIFACT_BYTES) { + throw new Error( + `Artifact ${artifact.id} is ${artifact.size} bytes; full internal resolution is blocked. Use read selectors such as artifact://${artifact.id}:1-3000 or artifact://${artifact.id}:raw:1-3000, and use the artifact file path for search/copy workflows: ${artifact.path}`, + ); } - // Artifact ids are per-session counters; in multi-session hosts the same - // id exists in several dirs. Pin resolution to the calling session's - // artifacts dir first so `artifact://3` means *this* session's #3. - const dirs = artifactsDirsFromRegistry(); - const pinnedDir = context?.localProtocolOptions?.getArtifactsDir?.() ?? null; - if (pinnedDir) { - const pinnedIndex = dirs.indexOf(pinnedDir); - if (pinnedIndex >= 0) dirs.splice(pinnedIndex, 1); - dirs.unshift(pinnedDir); - } - - if (dirs.length === 0) { - throw new Error("No session - artifacts unavailable"); - } - - let foundPath: string | undefined; - let anyDirExists = false; - const availableIds = new Set(); - - for (const dir of dirs) { - let files: string[]; - try { - files = await fs.readdir(dir); - anyDirExists = true; - } catch (err) { - if (isEnoent(err)) continue; - throw err; - } - const match = files.find(f => f.startsWith(`${id}.`)); - if (match) { - foundPath = path.join(dir, match); - break; - } - for (const f of files) { - const m = f.match(/^(\d+)\./); - if (m) availableIds.add(m[1]); - } - } - - if (!anyDirExists) { - throw new Error("No artifacts directory found"); - } - - if (!foundPath) { - const sorted = [...availableIds].sort((a, b) => Number(a) - Number(b)); - const availableStr = sorted.length > 0 ? sorted.join(", ") : "none"; - throw new Error(`Artifact ${id} not found. Available: ${availableStr}`); - } - - const content = await Bun.file(foundPath).text(); + const content = await Bun.file(artifact.path).text(); return { url: url.href, content, contentType: "text/plain", - size: Buffer.byteLength(content, "utf-8"), - sourcePath: foundPath, + size: artifact.size, + sourcePath: artifact.path, }; } diff --git a/packages/coding-agent/src/prompts/tools/read.md b/packages/coding-agent/src/prompts/tools/read.md index ea3424e07..554dc4cab 100644 --- a/packages/coding-agent/src/prompts/tools/read.md +++ b/packages/coding-agent/src/prompts/tools/read.md @@ -67,7 +67,7 @@ For `.sqlite`, `.sqlite3`, `.db`, `.db3`: # Internal URIs -All URI schemes take the same line selectors. `artifact://` recovers full output a bash/eval/tool result spilled or truncated. +All URI schemes take the same line selectors. `artifact://` recovers spilled output; large artifacts block unbounded `:raw`, so page with `artifact://:N-M` / `artifact://:raw:N-M` and use the reported artifact file path for search/copy workflows. `ssh://host/` reads a remote text file (UTF-8, ≤1 MiB) or lists a directory one level deep, on a pre-configured SSH host or `~/.ssh/config` alias; `ssh://host/` lists the remote root and bare `ssh://` lists the configured hosts. Files are also writable via `write` and searchable via `search`; a directory only lists (`search` refuses a directory, `write` refuses to overwrite one). A literal `:`, `?`, or `#` in the remote path must be percent-encoded (`%3A`/`%3F`/`%23`) — a trailing `:sel` is read as a line selector, and `?`/`#` start a URL query/fragment. Requires a POSIX login shell (`sh`/`bash`/`zsh`); a Windows host or a non-POSIX shell (fish, csh/tcsh) is rejected — use the `ssh` tool there. diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 721767724..1c878f96e 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -37,6 +37,7 @@ import { normalizeToLF } from "../edit/normalize"; import { isNotebookPath, readEditableNotebookText } from "../edit/notebook"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { InternalUrlRouter, resolveLocalUrlToFile } from "../internal-urls"; +import { type ResolvedArtifactFile, resolveArtifactFile } from "../internal-urls/artifact-protocol"; import { parseInternalUrl } from "../internal-urls/parse"; import type { InternalUrl } from "../internal-urls/types"; import { getLanguageFromPath, type Theme } from "../modes/theme/theme"; @@ -151,6 +152,7 @@ const CONVERTIBLE_EXTENSIONS = new Set([".pdf", ".doc", ".docx", ".ppt", ".pptx" const MAX_SUMMARY_BYTES = 2 * 1024 * 1024; const MAX_SUMMARY_LINES = 20_000; +const MAX_ARTIFACT_RAW_INLINE_BYTES = DEFAULT_MAX_BYTES; /** * Per-line column cap for file reads. Lines wider than the value of * `tools.outputMaxColumns` are ellipsis-truncated at display time; the file @@ -1519,6 +1521,7 @@ export class ReadTool implements AgentTool { displayMode: { hashLines: boolean; lineNumbers: boolean }, suffixResolution: { from: string; to: string } | undefined, signal: AbortSignal | undefined, + allowBridge = true, ): Promise<{ outputText: string; columnTruncated: number; @@ -1528,7 +1531,7 @@ export class ReadTool implements AgentTool { const rawSelector = isRawSelector(parsed); // ACP bridge first — the editor's in-memory buffer is source of truth. - const bridgePromise = this.#routeReadThroughBridge(absolutePath); + const bridgePromise = allowBridge ? this.#routeReadThroughBridge(absolutePath) : undefined; if (bridgePromise !== undefined) { try { const bridgeText = await bridgePromise; @@ -2802,6 +2805,195 @@ export class ReadTool implements AgentTool { return toolResult(details).text(summary).sourcePath(absolutePath).done(); } + #formatArtifactWorkflowNotice(artifact: ResolvedArtifactFile, artifactUrl: string): string { + return `Artifact storage: ${artifact.path} (${formatBytes(artifact.size)}). Use ${artifactUrl}:N-M to page, ${artifactUrl}:raw:N-M for verbatim chunks, and the artifact file path for search/copy workflows.`; + } + + #formatRawArtifactBlockedNotice(artifact: ResolvedArtifactFile, artifactUrl: string): string { + return `Unbounded raw read blocked for ${artifactUrl} (${formatBytes( + artifact.size, + )}). Reading the whole artifact verbatim can exhaust memory. Use ${artifactUrl}:raw:1-3000 for bounded verbatim chunks, ${artifactUrl}:1-3000 for numbered exploration, and the artifact file path for search/copy workflows: ${artifact.path}`; + } + + async #readArtifactFile( + url: InternalUrl, + parsedSel: ParsedSelector, + signal?: AbortSignal, + ): Promise> { + const artifact = await resolveArtifactFile(url, { + cwd: this.session.cwd, + settings: this.session.settings, + signal, + localProtocolOptions: this.session.localProtocolOptions, + skills: this.session.skills, + }); + const artifactUrl = `artifact://${artifact.id}`; + const details: ReadToolDetails = { + resolvedPath: artifact.path, + contentType: "text/plain", + }; + + if (parsedSel.kind === "raw" && artifact.size > MAX_ARTIFACT_RAW_INLINE_BYTES) { + return toolResult(details) + .text(this.#formatRawArtifactBlockedNotice(artifact, artifactUrl)) + .sourcePath(artifact.path) + .sourceInternal(url.href) + .done(); + } + + const rawSelector = isRawSelector(parsedSel); + const displayMode = resolveFileDisplayMode(this.session, { raw: rawSelector, immutable: true }); + if (isMultiRange(parsedSel) && parsedSel.kind === "lines") { + const read = await this.#readLocalFileMultiRange( + artifact.path, + parsedSel.ranges, + artifact.size, + parsedSel, + displayMode, + undefined, + signal, + false, + ); + if (read.bridgeResult) return read.bridgeResult; + if (read.displayContent) details.displayContent = read.displayContent; + let text = read.outputText; + if (artifact.size > MAX_ARTIFACT_RAW_INLINE_BYTES) { + text = text + ? `${text}\n\n[${this.#formatArtifactWorkflowNotice(artifact, artifactUrl)}]` + : this.#formatArtifactWorkflowNotice(artifact, artifactUrl); + } + const resultBuilder = toolResult(details) + .text(text) + .sourcePath(artifact.path) + .sourceInternal(url.href); + if (read.columnTruncated > 0) resultBuilder.limits({ columnMax: read.columnTruncated }); + return resultBuilder.done(); + } + + const { offset, limit } = selToOffsetLimit(parsedSel); + const requestedStart = offset ? Math.max(0, offset - 1) : 0; + const expandStart = offset !== undefined && offset > 1; + const expandEnd = limit !== undefined; + const leadingContext = expandStart ? Math.min(requestedStart, RANGE_LEADING_CONTEXT_LINES) : 0; + const trailingContext = expandEnd ? RANGE_TRAILING_CONTEXT_LINES : 0; + const startLine = requestedStart - leadingContext; + const startLineDisplay = startLine + 1; + const effectiveLimit = limit ?? this.#defaultLimit; + const maxLinesToCollect = Math.min(effectiveLimit + leadingContext + trailingContext, DEFAULT_MAX_LINES); + const selectedLineLimit = effectiveLimit + leadingContext + trailingContext; + const maxBytesForRead = Math.max(DEFAULT_MAX_BYTES, maxLinesToCollect * 512); + const streamResult = await streamLinesFromFile( + artifact.path, + startLine, + maxLinesToCollect, + maxBytesForRead, + selectedLineLimit, + signal, + artifact.size > SNAPSHOT_MAX_BYTES, + ); + const { + lines: collectedLines, + totalFileLines, + collectedBytes, + stoppedByByteLimit, + firstLinePreview, + firstLineByteLength, + reachedEof, + } = streamResult; + + if (requestedStart >= totalFileLines) { + const suggestion = + totalFileLines === 0 + ? "The artifact is empty." + : `Use ${artifactUrl}:1 to read from the start, or ${artifactUrl}:${totalFileLines} to read the last line.`; + return toolResult(details) + .text(`Line ${requestedStart + 1} is beyond end of artifact (${totalFileLines} lines total). ${suggestion}`) + .sourcePath(artifact.path) + .sourceInternal(url.href) + .done(); + } + + const shouldAddLineNumbers = rawSelector ? false : displayMode.hashLines ? false : displayMode.lineNumbers; + const selectedContent = collectedLines.join("\n"); + const totalSelectedLines = totalFileLines - startLine; + const wasTruncated = collectedLines.length < totalSelectedLines || stoppedByByteLimit; + const firstLineExceedsLimit = firstLineByteLength !== undefined && firstLineByteLength > maxBytesForRead; + const truncation: TruncationResult = { + content: selectedContent, + truncated: wasTruncated, + truncatedBy: stoppedByByteLimit ? "bytes" : wasTruncated ? "lines" : undefined, + totalLines: totalSelectedLines, + totalBytes: collectedBytes, + outputLines: collectedLines.length, + outputBytes: collectedBytes, + lastLinePartial: false, + firstLineExceedsLimit, + }; + + let displayContent: { text: string; startLine: number; lineNumbers?: Array } | undefined; + const formatText = (text: string, startNum: number): string => { + const lineCount = countTextLines(text); + displayContent = { + text, + startLine: startNum, + lineNumbers: Array.from({ length: lineCount }, (_, i) => startNum + i), + }; + return formatTextWithMode(text, startNum, false, shouldAddLineNumbers); + }; + + let outputText: string; + let truncationInfo: + | { result: TruncationResult; options: { direction: "head"; startLine?: number; totalFileLines?: number } } + | undefined; + if (truncation.firstLineExceedsLimit) { + const firstLineBytes = firstLineByteLength ?? 0; + const snippet = firstLinePreview ?? { text: "", bytes: 0 }; + outputText = + snippet.text.length > 0 + ? formatText(snippet.text, startLineDisplay) + : `[Line ${startLineDisplay} is ${formatBytes( + firstLineBytes, + )}, exceeds ${formatBytes(maxBytesForRead)} limit. Unable to display a valid UTF-8 snippet.]`; + truncationInfo = { + result: truncation, + options: { + direction: "head", + startLine: startLineDisplay, + totalFileLines: reachedEof ? totalFileLines : undefined, + }, + }; + } else { + outputText = formatText(truncation.content, startLineDisplay); + if (truncation.truncated) { + truncationInfo = { + result: truncation, + options: { + direction: "head", + startLine: startLineDisplay, + totalFileLines: reachedEof ? totalFileLines : undefined, + }, + }; + } else if (startLine + collectedLines.length < totalFileLines || !reachedEof) { + const nextOffset = startLine + collectedLines.length + 1; + outputText += reachedEof + ? `\n\n[${totalFileLines - (startLine + collectedLines.length)} more lines in artifact. Use ${artifactUrl}:${nextOffset} to continue]` + : `\n\n[More lines in artifact (${formatBytes(artifact.size)} total; not scanned to EOF). Use ${artifactUrl}:${nextOffset} to continue]`; + } + } + + if (artifact.size > MAX_ARTIFACT_RAW_INLINE_BYTES) { + outputText += `\n\n[${this.#formatArtifactWorkflowNotice(artifact, artifactUrl)}]`; + } + if (displayContent) details.displayContent = displayContent; + if (truncationInfo) details.truncation = truncationInfo.result; + const resultBuilder = toolResult(details) + .text(outputText) + .sourcePath(artifact.path) + .sourceInternal(url.href); + if (truncationInfo) resultBuilder.truncation(truncationInfo.result, truncationInfo.options); + return resultBuilder.done(); + } + /** * Handle internal URLs (agent://, artifact://, memory://, skill://, rule://, local://, mcp://). * Supports pagination via offset/limit but rejects them when query extraction is used. @@ -2829,6 +3021,9 @@ export class ReadTool implements AgentTool { const hasQueryExtraction = queryParam !== null && queryParam !== ""; hasExtraction = hasPathExtraction || hasQueryExtraction; } + if (scheme === "artifact") { + return this.#readArtifactFile(urlMeta, parsedSel, signal); + } // local:// files are real on-disk paths. Detect image files and emit a // decoded image block before the text-only resource contract UTF-8 diff --git a/packages/coding-agent/test/tools/read-artifact-large.test.ts b/packages/coding-agent/test/tools/read-artifact-large.test.ts new file mode 100644 index 000000000..6a66cbcbd --- /dev/null +++ b/packages/coding-agent/test/tools/read-artifact-large.test.ts @@ -0,0 +1,93 @@ +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 { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { + registerArtifactsDir, + resetRegisteredArtifactDirsForTests, +} from "@oh-my-pi/pi-coding-agent/internal-urls/registry-helpers"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; + +function getTextOutput(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content + .filter(c => c.type === "text" && typeof c.text === "string") + .map(c => c.text as string) + .join("\n"); +} + +function makeSession(cwd: string): ToolSession { + return { + cwd, + hasUI: false, + getSessionFile: () => path.join(cwd, "session.jsonl"), + getSessionSpawns: () => "*", + getArtifactsDir: () => path.join(cwd, "session"), + allocateOutputArtifact: async (toolType: string) => ({ + id: "a1", + path: path.join(cwd, "session", `a1.${toolType}.log`), + }), + settings: Settings.isolated(), + }; +} + +function largeArtifactText(): string { + return Array.from( + { length: 400 }, + (_, index) => `line-${String(index + 1).padStart(3, "0")} ${"x".repeat(256)}`, + ).join("\n"); +} + +describe("read tool large artifact handling", () => { + let testDir: string; + let artifactDir: string; + let unregisterArtifactsDir: (() => void) | undefined; + let tool: ReadTool; + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), "read-artifact-large-")); + artifactDir = path.join(testDir, "session"); + await fs.mkdir(artifactDir, { recursive: true }); + await Bun.write(path.join(artifactDir, "0.mcp.log"), largeArtifactText()); + resetRegisteredArtifactDirsForTests(); + unregisterArtifactsDir = registerArtifactsDir(artifactDir); + tool = new ReadTool(makeSession(testDir)); + }); + + afterEach(async () => { + unregisterArtifactsDir?.(); + resetRegisteredArtifactDirsForTests(); + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it("blocks unbounded raw reads and points to bounded artifact workflows", async () => { + const result = await tool.execute("call-raw", { path: "artifact://0:raw" }); + const output = getTextOutput(result); + + expect(output).toContain("Unbounded raw read blocked for artifact://0"); + expect(output).toContain("artifact://0:raw:1-3000"); + expect(output).toContain(artifactDir); + expect(output).not.toContain("line-001"); + }); + + it("streams bounded artifact reads without materializing the whole artifact", async () => { + const result = await tool.execute("call-range", { path: "artifact://0:1-3" }); + const output = getTextOutput(result); + + expect(output).toContain("line-001"); + expect(output).toContain("line-003"); + expect(output).toContain("Artifact storage:"); + expect(output).toContain("artifact://0:raw:N-M"); + expect(output).not.toContain("line-400"); + }); + + it("allows bounded raw artifact chunks", async () => { + const result = await tool.execute("call-raw-range", { path: "artifact://0:raw:1-2" }); + const output = getTextOutput(result); + + expect(output).toStartWith("line-001"); + expect(output).toContain("line-002"); + expect(output).not.toContain("line-400"); + }); +});