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
This commit is contained in:
@@ -101,6 +101,20 @@ export class ArtifactProtocolHandler implements ProtocolHandler {
|
||||
|
||||
async resolve(url: InternalUrl, context?: ResolveContext): Promise<InternalResource> {
|
||||
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}`,
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<InternalResource>;
|
||||
resolve(input: string, context?: ResolveContext): Promise<InternalResource>;
|
||||
}
|
||||
|
||||
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}`);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user