diff --git a/src/agent/tool-aliases.ts b/src/agent/tool-aliases.ts index 5bed3650e..3c731441c 100644 --- a/src/agent/tool-aliases.ts +++ b/src/agent/tool-aliases.ts @@ -141,7 +141,7 @@ export function withAuthzParityDefinitions< const SHELL_WRAPPERS = new Set(["bash", "sh", "zsh"]); -function shellQuote(arg: string): string { +export function shellQuote(arg: string): string { if (/^[A-Za-z0-9_\-./:=@%]+$/.test(arg)) return arg; return `'${arg.replace(/'/g, `'\\''`)}'`; } diff --git a/src/plugins/file-inspection-diagnosis.test.ts b/src/plugins/file-inspection-diagnosis.test.ts index d2d6cd177..6ad89381e 100644 --- a/src/plugins/file-inspection-diagnosis.test.ts +++ b/src/plugins/file-inspection-diagnosis.test.ts @@ -17,14 +17,17 @@ describe("PDF sniffing", () => { ).toBe("pdf"); }); - test("a .pdf name without magic is malformed, not a capability miss", () => { + test("a .pdf name with NUL and no magic is malformed; a text .pdf is not refused", () => { expect(pathLooksLikePdf("report.PDF")).toBe(true); expect( inspectionKindFromFirstChunk("report.pdf", Buffer.from("not-a-pdf\0")), ).toBe("pdf-malformed"); + }); + + test("plain UTF-8 named .pdf without magic is not an inspection block", () => { expect( inspectionKindFromFirstChunk("report.pdf", Buffer.from("plain text")), - ).toBe("pdf-malformed"); + ).toBeUndefined(); }); test("NUL without PDF markers is ordinary binary", () => { @@ -51,7 +54,7 @@ describe("diagnoseBlockedFileInspection", () => { expect(result.code).toBe("missing_extractor"); expect(result.message).toContain("PDF"); expect(result.message).toContain(PDF_EXTRACTOR_BIN); - expect(result.message).toContain("missing"); + expect(result.message).toContain("Missing PDF extractor"); expect(result.message.toLowerCase()).toContain("poppler"); expect(result.message).toContain("not a malformed file"); expect(result.message).not.toContain("permission boundary"); @@ -76,7 +79,7 @@ describe("diagnoseBlockedFileInspection", () => { }); expect(result.code).toBe("permission_boundary"); expect(result.message).toContain("installed"); - expect(result.message).toContain("permission boundary"); + expect(result.message).toContain("Permission boundary"); expect(result.message).toContain("run_shell"); expect(result.message).toContain("not a malformed file"); expect(result.message.toLowerCase()).not.toContain("brew install"); @@ -94,6 +97,18 @@ describe("diagnoseBlockedFileInspection", () => { expect(result.message).toContain(`${PDF_EXTRACTOR_BIN} ${PATH} -`); }); + test("extractor_ready quotes a path that would break a pasted bash command", () => { + const spaced = "/tmp/my report.pdf"; + const result = diagnoseBlockedFileInspection({ + path: spaced, + kind: "pdf", + extractorAvailable: true, + canExecuteHostCommands: true, + }); + expect(result.code).toBe("extractor_ready"); + expect(result.message).toContain(`${PDF_EXTRACTOR_BIN} '${spaced}' -`); + }); + test("malformed PDF is not blamed on tooling or permission", () => { const result = diagnoseBlockedFileInspection({ path: PATH, @@ -102,7 +117,7 @@ describe("diagnoseBlockedFileInspection", () => { canExecuteHostCommands: false, }); expect(result.code).toBe("malformed"); - expect(result.message).toContain("not a valid PDF"); + expect(result.message).toContain("Malformed PDF"); expect(result.message).toContain("not a missing extractor"); expect(result.message).not.toContain("brew install"); expect(result.message).not.toContain("grant this worker"); @@ -116,7 +131,7 @@ describe("diagnoseBlockedFileInspection", () => { canExecuteHostCommands: true, }); expect(result.code).toBe("unreadable"); - expect(result.message).toContain("unreadable"); + expect(result.message).toContain("Unreadable file"); expect(result.message).toContain("filesystem permission"); expect(result.message).not.toContain("grant this worker"); expect(result.message.toLowerCase()).not.toContain("poppler"); @@ -131,6 +146,34 @@ describe("diagnoseBlockedFileInspection", () => { }); expect(result.code).toBe("binary"); expect(result.message).toContain("refusing to read binary file"); - expect(result.message).toContain("not a missing-tool"); + expect(result.message).toContain("Not a missing-tool"); + }); + + test("the cause leads so a 72-character collapsed preview still names the gap", () => { + const missing = diagnoseBlockedFileInspection({ + path: "/very/long/workspace/path/to/a/nested/report.pdf", + kind: "pdf", + extractorAvailable: false, + canExecuteHostCommands: true, + }); + expect(missing.message.slice(0, 72).toLowerCase()).toContain("extractor"); + + const permission = diagnoseBlockedFileInspection({ + path: "/very/long/workspace/path/to/a/nested/report.pdf", + kind: "pdf", + extractorAvailable: true, + canExecuteHostCommands: false, + }); + expect(permission.message.slice(0, 72).toLowerCase()).toContain( + "permission", + ); + + const malformed = diagnoseBlockedFileInspection({ + path: "/very/long/workspace/path/to/a/nested/report.pdf", + kind: "pdf-malformed", + extractorAvailable: false, + canExecuteHostCommands: false, + }); + expect(malformed.message.slice(0, 72).toLowerCase()).toContain("malformed"); }); }); diff --git a/src/plugins/file-inspection-diagnosis.ts b/src/plugins/file-inspection-diagnosis.ts index 4d4336a75..a07239b2d 100644 --- a/src/plugins/file-inspection-diagnosis.ts +++ b/src/plugins/file-inspection-diagnosis.ts @@ -1,4 +1,5 @@ import { extname } from "node:path"; +import { shellQuote } from "../agent/tool-aliases.js"; /** Host binary that extracts text from a PDF (poppler). */ export const PDF_EXTRACTOR_BIN = "pdftotext"; @@ -45,16 +46,18 @@ export function pathLooksLikePdf(filePath: string): boolean { /** * Classify the first streamed chunk. PDF magic wins even without a .pdf - * suffix. A .pdf name without magic is malformed, whether or not the chunk - * looks binary. NUL without PDF markers is an ordinary binary file. + * suffix. A .pdf name with NUL but no magic is malformed. A .pdf that is + * otherwise valid UTF-8 is left as a normal text read so a misnamed text + * file is not a new refusal. */ export function inspectionKindFromFirstChunk( filePath: string, chunk: Uint8Array, ): FileInspectionKind | undefined { if (chunkLooksLikePdf(chunk)) return "pdf"; - if (pathLooksLikePdf(filePath)) return "pdf-malformed"; - if (chunk.includes(0)) return "binary"; + if (chunk.includes(0)) { + return pathLooksLikePdf(filePath) ? "pdf-malformed" : "binary"; + } return undefined; } @@ -76,8 +79,8 @@ export function diagnoseBlockedFileInspection( return { code: "unreadable", message: - `Cannot inspect ${path}: the file is unreadable (filesystem permission denied), not a missing extractor or worker-tool gap. ` + - `Check ownership and mode, or copy the file into a readable workspace path.`, + `Unreadable file (filesystem permission denied), not a missing extractor or worker-tool gap. ` + + `Check ownership and mode, or copy it into a readable workspace path. File: ${path}`, }; } @@ -85,8 +88,8 @@ export function diagnoseBlockedFileInspection( return { code: "malformed", message: - `Cannot inspect ${path}: the file is not a valid PDF (malformed or unreadable bytes), not a missing extractor or permission boundary. ` + - `Open it in a PDF reader or re-export it, then retry.`, + `Malformed PDF, not a missing extractor or permission boundary. ` + + `Open it in a PDF reader or re-export it. File: ${path}`, }; } @@ -94,7 +97,7 @@ export function diagnoseBlockedFileInspection( return { code: "binary", message: - `refusing to read binary file: ${path}. This is a binary file, not a missing-tool or permission failure. ` + + `refusing to read binary file: ${path}. Not a missing-tool or permission failure. ` + `Use a format-specific extractor if you need text from it.`, }; } @@ -104,8 +107,8 @@ export function diagnoseBlockedFileInspection( return { code: "missing_extractor", message: - `Cannot inspect PDF ${path}: ${extractor} is not installed (missing PDF-extraction capability), not a malformed file. ` + - `Install poppler so ${extractor} is on PATH (for example \`brew install poppler\` or \`apt-get install poppler-utils\`), then retry.`, + `Missing PDF extractor: ${extractor} is not on PATH (capability gap), not a malformed file. ` + + `Install poppler (\`brew install poppler\` or \`apt-get install poppler-utils\`), then retry. File: ${path}`, }; } @@ -113,15 +116,13 @@ export function diagnoseBlockedFileInspection( return { code: "permission_boundary", message: - `Cannot inspect PDF ${path}: ${extractor} is installed, but this worker cannot execute it (run_shell is not mounted — a permission boundary, not a malformed file). ` + - `Re-dispatch to a director that mounts run_shell, or grant this worker run_shell, then retry.`, + `Permission boundary: ${extractor} is installed but this worker cannot execute it (run_shell is not mounted), not a malformed file. ` + + `Re-dispatch to a director that mounts run_shell, or grant this worker run_shell. File: ${path}`, }; } return { code: "extractor_ready", - message: - `Cannot inspect PDF ${path} as text via read_file. ${extractor} is installed and this session can run host commands. ` + - `Run \`${extractor} ${path} -\` via bash to extract text.`, + message: `read_file cannot decode PDFs. ${extractor} is installed — run \`${extractor} ${shellQuote(path)} -\` via bash.`, }; } diff --git a/src/plugins/read-file-guard-plugin.test.ts b/src/plugins/read-file-guard-plugin.test.ts index 7e5c5e215..2410bfd43 100644 --- a/src/plugins/read-file-guard-plugin.test.ts +++ b/src/plugins/read-file-guard-plugin.test.ts @@ -126,7 +126,7 @@ describe("readFileBounded", () => { ); expect(isError).toBe(true); expect(content).toContain("binary"); - expect(content).toContain("not a missing-tool"); + expect(content).toContain("Not a missing-tool"); }); test("a PDF without pdftotext names the missing extractor, not a malformed file", async () => { @@ -140,7 +140,7 @@ describe("readFileBounded", () => { ); expect(isError).toBe(true); expect(content).toContain("pdftotext"); - expect(content).toContain("missing"); + expect(content).toContain("Missing PDF extractor"); expect(content).toContain("not a malformed file"); expect(content).not.toContain("permission boundary"); }); @@ -159,7 +159,7 @@ describe("readFileBounded", () => { ); expect(isError).toBe(true); expect(content).toContain("installed"); - expect(content).toContain("permission boundary"); + expect(content).toContain("Permission boundary"); expect(content).toContain("run_shell"); expect(content).toContain("not a malformed file"); }); @@ -174,11 +174,27 @@ describe("readFileBounded", () => { { whichExtractor: () => null, canExecuteHostCommands: () => false }, ); expect(isError).toBe(true); - expect(content).toContain("not a valid PDF"); + expect(content).toContain("Malformed PDF"); expect(content).toContain("not a missing extractor"); expect(content).not.toContain("brew install"); }); + test("UTF-8 .pdf without magic still reads as text", async () => { + const p = await fixture("utf8.pdf", "plain utf-8 notes\nsecond line"); + const { content, isError } = await readFileBounded( + p, + 0, + 2000, + neverAbort(), + { whichExtractor: () => null, canExecuteHostCommands: () => false }, + ); + expect(isError).toBeUndefined(); + expect(content).toContain("plain utf-8 notes"); + expect(content).toContain("second line"); + expect(content).not.toContain("not a valid PDF"); + expect(content).not.toContain("malformed"); + }); + test("a NUL deep in an otherwise-valid file does not discard streamed content", async () => { // Enough valid text (>64KB) to guarantee the NUL lands in a later chunk. const head = Array.from( @@ -807,8 +823,9 @@ describe("readFileGuardPlugin", () => { ); expect(missingResult.isError).toBe(true); expect(String(missingResult.content)).toContain("pdftotext"); - expect(String(missingResult.content)).toContain("missing"); + expect(String(missingResult.content)).toContain("Missing PDF extractor"); expect(String(missingResult.content)).toContain("not a malformed file"); + expect(String(missingResult.content)).toContain("flow.pdf"); const blocked = readFileGuardPlugin(dir, { whichExtractor: () => "/opt/homebrew/bin/pdftotext", @@ -820,10 +837,41 @@ describe("readFileGuardPlugin", () => { ); expect(blockedResult.isError).toBe(true); expect(String(blockedResult.content)).toContain("installed"); - expect(String(blockedResult.content)).toContain("permission boundary"); + expect(String(blockedResult.content)).toContain("Permission boundary"); expect(String(blockedResult.content)).toContain("run_shell"); expect(String(blockedResult.content)).not.toContain("brew install"); }); + + test("UTF-8 .pdf without magic still reads as text through the plugin", async () => { + await fixture("notes.pdf", "hello from a misnamed text file"); + const result = await run({ + id: "utf8-pdf", + name: "read_file", + arguments: { path: "notes.pdf" }, + }); + expect(result.isError).toBeFalsy(); + expect(result.content).toContain("hello from a misnamed text file"); + expect(String(result.content)).not.toContain("not a valid PDF"); + expect(String(result.content)).not.toContain("malformed"); + }); + + test("extractor_ready quotes a PDF path that contains spaces", async () => { + await fixture("my report.pdf", Buffer.from("%PDF-1.4\n")); + const plugin = readFileGuardPlugin(dir, { + whichExtractor: () => "/usr/bin/pdftotext", + canExecuteHostCommands: () => true, + }); + const result = await defined(plugin.middleware)(fallback)( + { + id: "quoted-pdf", + name: "read_file", + arguments: { path: "my report.pdf" }, + }, + neverAbort(), + ); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain(`pdftotext 'my report.pdf' -`); + }); }); describe("CL-8980 single-way path+offset resume", () => { diff --git a/src/plugins/read-file-guard-plugin.ts b/src/plugins/read-file-guard-plugin.ts index d9278390c..737a86e29 100644 --- a/src/plugins/read-file-guard-plugin.ts +++ b/src/plugins/read-file-guard-plugin.ts @@ -60,6 +60,8 @@ export interface ReadFileGuardPluginOptions { * Workers flip this after the capability filter; omitted means yes. */ canExecuteHostCommands?: () => boolean; + /** Model-facing path for diagnosis text (the call argument, not the absolute). */ + displayPath?: string; } // A truncated read tells the model to continue with the same path and the @@ -366,9 +368,10 @@ export function readFileBounded( signal: AbortSignal, inspection?: Pick< ReadFileGuardPluginOptions, - "whichExtractor" | "canExecuteHostCommands" + "whichExtractor" | "canExecuteHostCommands" | "displayPath" >, ): Promise { + const labeledPath = inspection?.displayPath ?? absolutePath; return readStreamBounded( createReadStream(absolutePath), absolutePath, @@ -376,13 +379,13 @@ export function readFileBounded( limit, signal, { - mapStreamError: (err) => mapFilesystemStreamError(absolutePath, err), + mapStreamError: (err) => mapFilesystemStreamError(labeledPath, err), windowHugeLines: true, diagnoseFirstChunk: (chunk) => { const kind = inspectionKindFromFirstChunk(absolutePath, chunk); if (kind === undefined) return undefined; return diagnoseBlockedFileInspection({ - path: absolutePath, + path: labeledPath, kind, extractorAvailable: resolveExtractorProbe(inspection?.whichExtractor), canExecuteHostCommands: @@ -537,7 +540,7 @@ export function readFileGuardPlugin( return { callId: call.id, content: diagnoseBlockedFileInspection({ - path: rawPath, + path: absolutePath, kind: "unreadable", extractorAvailable: false, canExecuteHostCommands: false, @@ -549,13 +552,10 @@ export function readFileGuardPlugin( } try { - const res = await readFileBounded( - absolutePath, - offset, - limit, - signal, - inspection, - ); + const res = await readFileBounded(absolutePath, offset, limit, signal, { + ...inspection, + displayPath: rawPath, + }); return res.isError ? { callId: call.id, content: res.content, isError: true } : { callId: call.id, content: res.content };