Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions src/plugins/file-inspection-diagnosis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
});
});
32 changes: 16 additions & 16 deletions src/plugins/file-inspection-diagnosis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -76,25 +78,25 @@ 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}`,
};
}

if (input.kind === "pdf-malformed") {
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}`,
};
}

if (input.kind === "binary") {
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.`,
};
}
Expand All @@ -104,24 +106,22 @@ 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}`,
};
}

if (!input.canExecuteHostCommands) {
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.`,
};
}
13 changes: 7 additions & 6 deletions src/plugins/read-file-guard-plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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");
});
Expand All @@ -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");
});
Expand All @@ -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");
});
Expand Down Expand Up @@ -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",
Expand All @@ -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");
});
Expand Down
20 changes: 10 additions & 10 deletions src/plugins/read-file-guard-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -366,23 +368,24 @@ export function readFileBounded(
signal: AbortSignal,
inspection?: Pick<
ReadFileGuardPluginOptions,
"whichExtractor" | "canExecuteHostCommands"
"whichExtractor" | "canExecuteHostCommands" | "displayPath"
>,
): Promise<BoundedRead> {
const labeledPath = inspection?.displayPath ?? absolutePath;
return readStreamBounded(
createReadStream(absolutePath),
absolutePath,
offset,
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:
Expand Down Expand Up @@ -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 };
Expand Down
Loading