From ff397cf27a71a461900b3e012d4223c51b798ce9 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 3 Jul 2026 23:17:46 +0000 Subject: [PATCH] fix(read): kept large artifacts reachable from path-only resolvers Bash URL expansion and search/grep only need sourcePath; they now request pathOnly resolution so large artifacts stay usable for search/copy workflows while unbounded content materialization stays blocked. Fixes #4482 --- .../src/internal-urls/artifact-protocol.ts | 14 ++++ .../coding-agent/src/internal-urls/types.ts | 10 +++ .../coding-agent/src/tools/bash-skill-urls.ts | 6 +- packages/coding-agent/src/tools/grep.ts | 21 +++++- .../internal-urls/artifact-path-only.test.ts | 71 +++++++++++++++++++ 5 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 packages/coding-agent/test/internal-urls/artifact-path-only.test.ts diff --git a/packages/coding-agent/src/internal-urls/artifact-protocol.ts b/packages/coding-agent/src/internal-urls/artifact-protocol.ts index 0993fa7e1..c888612c1 100644 --- a/packages/coding-agent/src/internal-urls/artifact-protocol.ts +++ b/packages/coding-agent/src/internal-urls/artifact-protocol.ts @@ -101,6 +101,20 @@ export class ArtifactProtocolHandler implements ProtocolHandler { async resolve(url: InternalUrl, context?: ResolveContext): Promise { const artifact = await resolveArtifactFile(url, context); + + // Path-only callers (search/grep, bash URL expansion) never touch the + // artifact bytes. Return the resource shape so those flows keep working + // on artifacts of any size — only content materialization is gated. + if (context?.pathOnly) { + return { + url: url.href, + content: "", + contentType: "text/plain", + size: artifact.size, + sourcePath: artifact.path, + }; + } + 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}`, diff --git a/packages/coding-agent/src/internal-urls/types.ts b/packages/coding-agent/src/internal-urls/types.ts index b5b8dfaa9..59bcb311d 100644 --- a/packages/coding-agent/src/internal-urls/types.ts +++ b/packages/coding-agent/src/internal-urls/types.ts @@ -107,6 +107,16 @@ export interface ResolveContext { * reject directory resources, so they never need the listing. */ skipDirectoryListing?: boolean; + /** + * When set, handlers that would otherwise materialize expensive content + * (e.g. reading a multi-MiB artifact into memory just to expose its + * `sourcePath`) may return the resource shape without content. Callers + * that only need `sourcePath` — search/grep, bash URL expansion — pass + * this so a large `artifact://` still resolves to its backing file + * without OOM risk. Handlers that cannot separate path from content + * ignore the flag. + */ + pathOnly?: boolean; } /** diff --git a/packages/coding-agent/src/tools/bash-skill-urls.ts b/packages/coding-agent/src/tools/bash-skill-urls.ts index b856baa6e..82e9e5f8e 100644 --- a/packages/coding-agent/src/tools/bash-skill-urls.ts +++ b/packages/coding-agent/src/tools/bash-skill-urls.ts @@ -3,7 +3,7 @@ import * as path from "node:path"; import type { Skill } from "../extensibility/skills"; import { type LocalProtocolOptions, resolveLocalUrlToPath } from "../internal-urls"; import { validateRelativePath } from "../internal-urls/skill-protocol"; -import type { InternalResource } from "../internal-urls/types"; +import type { InternalResource, ResolveContext } from "../internal-urls/types"; import { normalizeLocalScheme } from "./path-utils"; import { ToolError } from "./tool-errors"; @@ -19,7 +19,7 @@ type SupportedInternalScheme = (typeof SUPPORTED_INTERNAL_SCHEMES)[number]; interface InternalUrlResolver { canHandle(input: string): boolean; - resolve(input: string): Promise; + resolve(input: string, context?: ResolveContext): Promise; } export interface InternalUrlExpansionOptions { @@ -184,7 +184,7 @@ async function resolveInternalUrlToPath( let resource: InternalResource; try { - resource = await internalRouter.resolve(url); + resource = await internalRouter.resolve(url, { pathOnly: true }); } catch (error) { const message = error instanceof Error ? error.message : String(error); throw new ToolError(`Failed to resolve ${scheme}:// URL in bash command: ${url}\n${message}`); diff --git a/packages/coding-agent/src/tools/grep.ts b/packages/coding-agent/src/tools/grep.ts index 6b1d6944e..dbc5f3371 100644 --- a/packages/coding-agent/src/tools/grep.ts +++ b/packages/coding-agent/src/tools/grep.ts @@ -750,6 +750,11 @@ async function resolveInternalSearchInputs(opts: { localProtocolOptions: opts.localProtocolOptions, skills: opts.skills, skipDirectoryListing: true, + // Try path-only first so large artifacts (and any other handler that + // separates path from content) resolve without materializing bytes. + // Handlers that ignore the flag still return content, and virtual + // resources without a sourcePath fall through to a second resolve. + pathOnly: true, }; for (let idx = 0; idx < paths.length; idx++) { @@ -764,7 +769,7 @@ async function resolveInternalSearchInputs(opts: { if (hasGlobPathChars(globTarget)) { throw new ToolError(`Glob patterns are not supported for internal URLs: ${rawPath}`); } - const resource = await internalRouter.resolve(rawPath, context); + let resource = await internalRouter.resolve(rawPath, context); // A directory listing with no backing local path (e.g. a remote ssh:// dir) // has no real contents to grep — searching its listing text would be // misleading. Local/skill/vault dir resources set `sourcePath` and skip this. @@ -781,8 +786,20 @@ async function resolveInternalSearchInputs(opts: { continue; } + // No sourcePath: this handler needs its content materialized so the + // virtual expansion can search it. Re-resolve without pathOnly. + if (context.pathOnly) { + resource = await internalRouter.resolve(rawPath, { ...context, pathOnly: false }); + } + const ranges = opts.pathSpecs[idx]?.ranges; - const expanded = await expandVirtualInternalResource(rawPath, resource, internalRouter, context, ranges); + const expanded = await expandVirtualInternalResource( + rawPath, + resource, + internalRouter, + { ...context, pathOnly: false }, + ranges, + ); virtualInputIndexes.add(idx); for (const virtual of expanded) { virtualResources.push(virtual); diff --git a/packages/coding-agent/test/internal-urls/artifact-path-only.test.ts b/packages/coding-agent/test/internal-urls/artifact-path-only.test.ts new file mode 100644 index 000000000..aaa129bb3 --- /dev/null +++ b/packages/coding-agent/test/internal-urls/artifact-path-only.test.ts @@ -0,0 +1,71 @@ +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 { ArtifactProtocolHandler } from "@oh-my-pi/pi-coding-agent/internal-urls/artifact-protocol"; +import { parseInternalUrl } from "@oh-my-pi/pi-coding-agent/internal-urls/parse"; +import { + registerArtifactsDir, + resetRegisteredArtifactDirsForTests, +} from "@oh-my-pi/pi-coding-agent/internal-urls/registry-helpers"; + +/** + * Path-only callers (search/grep, bash URL expansion) only need the artifact's + * filesystem path. Blocking them for large artifacts would break `search` + * against MCP results and `bash` commands that reference the file — the very + * workflows the read-tool guidance points users toward. + */ +describe("artifact:// path-only resolution", () => { + let testDir: string; + let artifactDir: string; + let unregister: (() => void) | undefined; + const handler = new ArtifactProtocolHandler(); + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), "artifact-path-only-")); + artifactDir = path.join(testDir, "session"); + await fs.mkdir(artifactDir, { recursive: true }); + // 9 MiB — larger than the 8 MiB inline cap so `pathOnly: false` refuses to + // materialize while `pathOnly: true` returns the shape unchanged. + const bytes = Buffer.alloc(9 * 1024 * 1024, 65); + await Bun.write(path.join(artifactDir, "0.mcp.log"), bytes); + resetRegisteredArtifactDirsForTests(); + unregister = registerArtifactsDir(artifactDir); + }); + + afterEach(async () => { + unregister?.(); + resetRegisteredArtifactDirsForTests(); + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it("returns the artifact source path for large artifacts under pathOnly without reading its bytes", async () => { + const url = parseInternalUrl("artifact://0"); + const resource = await handler.resolve(url, { pathOnly: true }); + + expect(resource.sourcePath).toBe(path.join(artifactDir, "0.mcp.log")); + expect(resource.size).toBe(9 * 1024 * 1024); + // Content must NOT be materialized — that is the whole point of pathOnly. + expect(resource.content).toBe(""); + }); + + it("still rejects full content resolution for large artifacts (existing OOM guard)", async () => { + const url = parseInternalUrl("artifact://0"); + await expect(handler.resolve(url)).rejects.toThrow(/full internal resolution is blocked/); + }); + + it("materializes small artifacts on ordinary resolution", async () => { + const smallArtifactDir = path.join(testDir, "small-session"); + await fs.mkdir(smallArtifactDir, { recursive: true }); + await Bun.write(path.join(smallArtifactDir, "9.mcp.log"), "hello world\n"); + const unregisterSmall = registerArtifactsDir(smallArtifactDir); + try { + const url = parseInternalUrl("artifact://9"); + const resource = await handler.resolve(url); + expect(resource.content).toBe("hello world\n"); + expect(resource.sourcePath).toBe(path.join(smallArtifactDir, "9.mcp.log")); + } finally { + unregisterSmall(); + } + }); +});