From 65d2a4e00ce219989885bf19a39ca1dced89d1fd Mon Sep 17 00:00:00 2001 From: Aiden Cline <63023139+rekram1-node@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:58:37 -0500 Subject: [PATCH] feat(core): improve read tool parity (#39126) --- packages/cli/src/acp/tool.ts | 1 + packages/cli/test/acp/tool.test.ts | 11 ++-- packages/core/src/location-mutation.ts | 2 + packages/core/src/tool/plugin/read.ts | 17 ++++-- packages/core/src/tool/read-filesystem.ts | 7 ++- .../core/test/tool-read-filesystem.test.ts | 17 ++++++ packages/core/test/tool-read.test.ts | 55 +++++++++++++++++++ 7 files changed, 97 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/acp/tool.ts b/packages/cli/src/acp/tool.ts index 8937712f11b..041226068df 100644 --- a/packages/cli/src/acp/tool.ts +++ b/packages/cli/src/acp/tool.ts @@ -44,6 +44,7 @@ export function toLocations(toolName: string, input: ToolInput, cwd?: string): T return workdir ? [{ path: workdir }] : [] } case "read": + return locationFrom(input.path) case "edit": case "write": case "patch": diff --git a/packages/cli/test/acp/tool.test.ts b/packages/cli/test/acp/tool.test.ts index 0095b57ce1c..69908e0bb59 100644 --- a/packages/cli/test/acp/tool.test.ts +++ b/packages/cli/test/acp/tool.test.ts @@ -28,7 +28,7 @@ describe("acp tools", () => { }) test("extracts file locations from tool input", () => { - expect(toLocations("read", { filePath: "/tmp/a.ts" })).toEqual([{ path: "/tmp/a.ts" }]) + expect(toLocations("read", { path: "/tmp/a.ts" })).toEqual([{ path: "/tmp/a.ts" }]) expect(toLocations("edit", { filePath: "/tmp/b.ts" })).toEqual([{ path: "/tmp/b.ts" }]) expect(toLocations("write", { filePath: "/tmp/c.ts" })).toEqual([{ path: "/tmp/c.ts" }]) expect(toLocations("grep", { path: "/repo/src" })).toEqual([{ path: "/repo/src" }]) @@ -43,7 +43,7 @@ describe("acp tools", () => { ]) expect(toLocations("bash", { command: "pwd", workdir: "/abs/dir" }, "/workspace")).toEqual([{ path: "/abs/dir" }]) expect(toLocations("bash", { command: "printf hello" })).toEqual([]) - expect(toLocations("read", { path: "/tmp/missing-file-path.ts" })).toEqual([]) + expect(toLocations("read", { path: "/tmp/missing-file-path.ts" })).toEqual([{ path: "/tmp/missing-file-path.ts" }]) }) test("builds completed content with text and image attachments", () => { @@ -205,12 +205,13 @@ describe("acp tools", () => { runningToolUpdate({ toolCallId: "call", toolName: "read", - state: { input: { filePath: "/tmp/a" } }, + state: { input: { path: "/tmp/a" } }, content: [{ type: "text", text: "done" }], }), ).toMatchObject({ toolCallId: "call", status: "in_progress", + locations: [{ path: "/tmp/a" }], content: [{ type: "content", content: { type: "text", text: "done" } }], }) }) @@ -273,7 +274,7 @@ describe("acp tools", () => { errorToolUpdate({ toolCallId: "call", toolName: "read", - input: { filePath: "/tmp/a" }, + input: { path: "/tmp/a" }, content: [{ type: "text", text: "partial output" }], metadata: { path: "/tmp/a" }, error: "failed", @@ -284,7 +285,7 @@ describe("acp tools", () => { kind: "read", title: "read", locations: [{ path: "/tmp/a" }], - rawInput: { filePath: "/tmp/a" }, + rawInput: { path: "/tmp/a" }, content: [ { type: "content", content: { type: "text", text: "partial output" } }, { type: "content", content: { type: "text", text: "failed" } }, diff --git a/packages/core/src/location-mutation.ts b/packages/core/src/location-mutation.ts index 313b4dc79ef..43520c0f255 100644 --- a/packages/core/src/location-mutation.ts +++ b/packages/core/src/location-mutation.ts @@ -120,6 +120,8 @@ const layer = Layer.effect( const resolve = Effect.fn("LocationMutation.resolve")(function* (input: ResolveInput) { const absolute = path.resolve(location.directory, input.path) + // External access follows the requested path boundary. Symlinks reached through an + // internal path intentionally retain internal permission semantics after canonicalization. const lexicallyInternal = FSUtil.contains(location.directory, absolute) const resolved = yield* resolvePath(absolute) diff --git a/packages/core/src/tool/plugin/read.ts b/packages/core/src/tool/plugin/read.ts index e750d1f8dab..a3a0e1957a7 100644 --- a/packages/core/src/tool/plugin/read.ts +++ b/packages/core/src/tool/plugin/read.ts @@ -14,7 +14,7 @@ import { ReadToolFileSystem } from "../read-filesystem" export const name = "read" const FILENAME = "AGENTS.md" -const SUPPORTED_IMAGE_MIMES = new Set(["image/jpeg", "image/png", "image/gif", "image/webp"]) +const SUPPORTED_MEDIA_MIMES = new Set(["image/jpeg", "image/png", "image/gif", "image/webp", "application/pdf"]) const LocationInput = Schema.Struct({ path: Schema.String, offset: ReadToolFileSystem.PageInput.fields.offset.annotate({ @@ -110,7 +110,7 @@ export const Plugin = { Effect.catch(() => Effect.void), Effect.catchDefect(() => Effect.void), ) - if (content.type === "file" && content.encoding === "base64" && !SUPPORTED_IMAGE_MIMES.has(content.mime)) + if (content.type === "file" && content.encoding === "base64" && !SUPPORTED_MEDIA_MIMES.has(content.mime)) return yield* Effect.fail(new ReadToolFileSystem.BinaryFileError({ resource })) return content }).pipe( @@ -119,9 +119,13 @@ export const Plugin = { // unresized copy in model text. const content = output.type === "file" && output.encoding === "base64" - ? SUPPORTED_IMAGE_MIMES.has(output.mime) + ? SUPPORTED_MEDIA_MIMES.has(output.mime) ? ([ - { type: "text", text: "Image read successfully" }, + { + type: "text", + text: + output.mime === "application/pdf" ? "PDF read successfully" : "Image read successfully", + }, { type: "file", uri: `data:${output.mime};base64,${output.content}`, @@ -136,7 +140,10 @@ export const Plugin = { Effect.mapError((error) => { const message = error instanceof ReadToolFileSystem.BinaryFileError || - error instanceof ReadToolFileSystem.MediaIngestLimitError + error instanceof ReadToolFileSystem.MediaIngestLimitError || + error instanceof ReadToolFileSystem.MalformedUtf8Error || + error instanceof ReadToolFileSystem.OffsetOutOfRangeError || + error instanceof ReadToolFileSystem.PathKindError ? error.message : `Unable to read ${input.path}` return new ToolFailure({ message, error }) diff --git a/packages/core/src/tool/read-filesystem.ts b/packages/core/src/tool/read-filesystem.ts index bf85368e984..bafecd706e9 100644 --- a/packages/core/src/tool/read-filesystem.ts +++ b/packages/core/src/tool/read-filesystem.ts @@ -140,12 +140,13 @@ const extensions = new Set([ ".pyo", ]) const startsWith = (bytes: Uint8Array, prefix: number[]) => prefix.every((value, index) => bytes[index] === value) -const imageMime = (bytes: Uint8Array) => { +const mediaMime = (bytes: Uint8Array) => { if (startsWith(bytes, [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a])) return "image/png" if (startsWith(bytes, [0xff, 0xd8, 0xff])) return "image/jpeg" if (startsWith(bytes, [0x47, 0x49, 0x46, 0x38])) return "image/gif" if (startsWith(bytes, [0x52, 0x49, 0x46, 0x46]) && startsWith(bytes.subarray(8), [0x57, 0x45, 0x42, 0x50])) return "image/webp" + if (startsWith(bytes, [0x25, 0x50, 0x44, 0x46, 0x2d])) return "application/pdf" } const binary = (resource: string, bytes: Uint8Array) => { if (extensions.has(path.extname(resource).toLowerCase())) return true @@ -191,7 +192,7 @@ export const read = Effect.fn("ReadTool.read")(function* ( yield* file.readAlloc(Math.min(64 * 1024, Number(info.size) || 4 * 1024)), () => new Uint8Array(), ) - const mime = imageMime(first) + const mime = mediaMime(first) if (mime) { if (info.size > MAX_MEDIA_INGEST_BYTES) return yield* Effect.fail(new MediaIngestLimitError({ resource, maximumBytes: MAX_MEDIA_INGEST_BYTES })) @@ -217,7 +218,7 @@ export const read = Effect.fn("ReadTool.read")(function* ( mime, } } - if (startsWith(first, [0x25, 0x50, 0x44, 0x46]) || extensions.has(path.extname(resource).toLowerCase())) + if (extensions.has(path.extname(resource).toLowerCase())) return yield* Effect.fail(new BinaryFileError({ resource })) const paged = info.size > MAX_READ_BYTES || page.offset !== undefined || page.limit !== undefined if (!paged) { diff --git a/packages/core/test/tool-read-filesystem.test.ts b/packages/core/test/tool-read-filesystem.test.ts index 92c4f173972..96b227a21ae 100644 --- a/packages/core/test/tool-read-filesystem.test.ts +++ b/packages/core/test/tool-read-filesystem.test.ts @@ -115,4 +115,21 @@ describe("ReadToolFileSystem", () => { ) }), ) + + it.effect("reads PDFs as bounded media", () => + Effect.gen(function* () { + const { fs, files, directory } = yield* fixture + const file = path.join(directory, "document.pdf") + yield* files.writeFileString(file, "%PDF-1.7\ncontent") + + const result = yield* ReadToolFileSystem.read(fs, file, "document.pdf") + + expect(result).toMatchObject({ + type: "file", + content: Buffer.from("%PDF-1.7\ncontent").toString("base64"), + encoding: "base64", + mime: "application/pdf", + }) + }), + ) }) diff --git a/packages/core/test/tool-read.test.ts b/packages/core/test/tool-read.test.ts index c0bc9a2aa75..086e22aef73 100644 --- a/packages/core/test/tool-read.test.ts +++ b/packages/core/test/tool-read.test.ts @@ -540,6 +540,35 @@ describe("ReadTool", () => { }), ) + it.effect("returns PDFs as native media", () => + Effect.gen(function* () { + const pdf = "JVBERi0xLjcK" + readResult = { + type: "file", + uri: "file:///document.pdf", + name: "document.pdf", + content: pdf, + encoding: "base64", + mime: "application/pdf", + } + const registry = yield* Tool.Service + + expect( + yield* executeTool(registry, { + sessionID, + ...toolIdentity, + call: { type: "tool-call", id: "call-pdf", name: "read", input: { path: "document.pdf" } }, + }), + ).toMatchObject({ + status: "completed", + content: [ + { type: "text", text: "PDF read successfully" }, + { type: "file", uri: `data:application/pdf;base64,${pdf}`, mime: "application/pdf", name: "document.pdf" }, + ], + }) + }), + ) + it.effect("returns expected filesystem failures to the model", () => Effect.gen(function* () { readFailure = new ReadToolFileSystem.BinaryFileError({ resource: "archive.dat" }) @@ -563,6 +592,32 @@ describe("ReadTool", () => { }), ) + it.effect("preserves actionable read failure messages", () => + Effect.gen(function* () { + const registry = yield* Tool.Service + for (const [error, message] of [ + [ + new ReadToolFileSystem.MalformedUtf8Error({ resource: "invalid.txt" }), + "File is not valid UTF-8: invalid.txt", + ], + [new ReadToolFileSystem.OffsetOutOfRangeError({ offset: 10 }), "Offset 10 is out of range"], + [ + new ReadToolFileSystem.PathKindError({ resource: "socket", expected: "a file" }), + "Path is not a file: socket", + ], + ] as const) { + readFailure = error + expect( + yield* executeTool(registry, { + sessionID, + ...toolIdentity, + call: { type: "tool-call", id: `call-${error._tag}`, name: "read", input: { path: "target" } }, + }), + ).toEqual({ status: "error", error: { type: "unknown", message } }) + } + }), + ) + it.effect("preserves unexpected filesystem defects", () => Effect.gen(function* () { resolveFailure = new Error("unexpected")