From a3da6694fc0725fa8c8262b12557af1da29401d0 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 29 Sep 2026 16:30:47 -0700 Subject: [PATCH] fix(read): lead file-inspection diagnosis with the cause Collapsed tool rows clip to 72 characters, so a long path buried the gap. The first sentence now names missing extractor, permission boundary, or malformed file, and the message uses the call path. --- src/plugins/file-inspection-diagnosis.test.ts | 42 +++++++++++++++---- src/plugins/file-inspection-diagnosis.ts | 32 +++++++------- src/plugins/read-file-guard-plugin.test.ts | 13 +++--- src/plugins/read-file-guard-plugin.ts | 20 ++++----- 4 files changed, 68 insertions(+), 39 deletions(-) diff --git a/src/plugins/file-inspection-diagnosis.test.ts b/src/plugins/file-inspection-diagnosis.test.ts index d2d6cd177..6ba29f69a 100644 --- a/src/plugins/file-inspection-diagnosis.test.ts +++ b/src/plugins/file-inspection-diagnosis.test.ts @@ -17,14 +17,14 @@ 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"); expect( inspectionKindFromFirstChunk("report.pdf", Buffer.from("plain text")), - ).toBe("pdf-malformed"); + ).toBeUndefined(); }); test("NUL without PDF markers is ordinary binary", () => { @@ -51,7 +51,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 +76,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"); @@ -102,7 +102,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 +116,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 +131,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..7d9831606 100644 --- a/src/plugins/file-inspection-diagnosis.ts +++ b/src/plugins/file-inspection-diagnosis.ts @@ -45,16 +45,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 +78,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 +87,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 +96,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 +106,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 +115,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} ${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..ccd8bee10 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,7 +174,7 @@ 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"); }); @@ -807,8 +807,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,7 +821,7 @@ 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"); }); diff --git a/src/plugins/read-file-guard-plugin.ts b/src/plugins/read-file-guard-plugin.ts index d9278390c..29c3fd6c4 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: @@ -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 };