Skip to content

Commit 000349c

Browse files
committed
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.
1 parent 4bbd78c commit 000349c

4 files changed

Lines changed: 68 additions & 39 deletions

File tree

‎src/plugins/file-inspection-diagnosis.test.ts‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@ describe("PDF sniffing", () => {
1717
).toBe("pdf");
1818
});
1919

20-
test("a .pdf name without magic is malformed, not a capability miss", () => {
20+
test("a .pdf name with NUL and no magic is malformed; a text .pdf is not refused", () => {
2121
expect(pathLooksLikePdf("report.PDF")).toBe(true);
2222
expect(
2323
inspectionKindFromFirstChunk("report.pdf", Buffer.from("not-a-pdf\0")),
2424
).toBe("pdf-malformed");
2525
expect(
2626
inspectionKindFromFirstChunk("report.pdf", Buffer.from("plain text")),
27-
).toBe("pdf-malformed");
27+
).toBeUndefined();
2828
});
2929

3030
test("NUL without PDF markers is ordinary binary", () => {
@@ -51,7 +51,7 @@ describe("diagnoseBlockedFileInspection", () => {
5151
expect(result.code).toBe("missing_extractor");
5252
expect(result.message).toContain("PDF");
5353
expect(result.message).toContain(PDF_EXTRACTOR_BIN);
54-
expect(result.message).toContain("missing");
54+
expect(result.message).toContain("Missing PDF extractor");
5555
expect(result.message.toLowerCase()).toContain("poppler");
5656
expect(result.message).toContain("not a malformed file");
5757
expect(result.message).not.toContain("permission boundary");
@@ -76,7 +76,7 @@ describe("diagnoseBlockedFileInspection", () => {
7676
});
7777
expect(result.code).toBe("permission_boundary");
7878
expect(result.message).toContain("installed");
79-
expect(result.message).toContain("permission boundary");
79+
expect(result.message).toContain("Permission boundary");
8080
expect(result.message).toContain("run_shell");
8181
expect(result.message).toContain("not a malformed file");
8282
expect(result.message.toLowerCase()).not.toContain("brew install");
@@ -102,7 +102,7 @@ describe("diagnoseBlockedFileInspection", () => {
102102
canExecuteHostCommands: false,
103103
});
104104
expect(result.code).toBe("malformed");
105-
expect(result.message).toContain("not a valid PDF");
105+
expect(result.message).toContain("Malformed PDF");
106106
expect(result.message).toContain("not a missing extractor");
107107
expect(result.message).not.toContain("brew install");
108108
expect(result.message).not.toContain("grant this worker");
@@ -116,7 +116,7 @@ describe("diagnoseBlockedFileInspection", () => {
116116
canExecuteHostCommands: true,
117117
});
118118
expect(result.code).toBe("unreadable");
119-
expect(result.message).toContain("unreadable");
119+
expect(result.message).toContain("Unreadable file");
120120
expect(result.message).toContain("filesystem permission");
121121
expect(result.message).not.toContain("grant this worker");
122122
expect(result.message.toLowerCase()).not.toContain("poppler");
@@ -131,6 +131,34 @@ describe("diagnoseBlockedFileInspection", () => {
131131
});
132132
expect(result.code).toBe("binary");
133133
expect(result.message).toContain("refusing to read binary file");
134-
expect(result.message).toContain("not a missing-tool");
134+
expect(result.message).toContain("Not a missing-tool");
135+
});
136+
137+
test("the cause leads so a 72-character collapsed preview still names the gap", () => {
138+
const missing = diagnoseBlockedFileInspection({
139+
path: "/very/long/workspace/path/to/a/nested/report.pdf",
140+
kind: "pdf",
141+
extractorAvailable: false,
142+
canExecuteHostCommands: true,
143+
});
144+
expect(missing.message.slice(0, 72).toLowerCase()).toContain("extractor");
145+
146+
const permission = diagnoseBlockedFileInspection({
147+
path: "/very/long/workspace/path/to/a/nested/report.pdf",
148+
kind: "pdf",
149+
extractorAvailable: true,
150+
canExecuteHostCommands: false,
151+
});
152+
expect(permission.message.slice(0, 72).toLowerCase()).toContain(
153+
"permission",
154+
);
155+
156+
const malformed = diagnoseBlockedFileInspection({
157+
path: "/very/long/workspace/path/to/a/nested/report.pdf",
158+
kind: "pdf-malformed",
159+
extractorAvailable: false,
160+
canExecuteHostCommands: false,
161+
});
162+
expect(malformed.message.slice(0, 72).toLowerCase()).toContain("malformed");
135163
});
136164
});

‎src/plugins/file-inspection-diagnosis.ts‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,18 @@ export function pathLooksLikePdf(filePath: string): boolean {
4545

4646
/**
4747
* Classify the first streamed chunk. PDF magic wins even without a .pdf
48-
* suffix. A .pdf name without magic is malformed, whether or not the chunk
49-
* looks binary. NUL without PDF markers is an ordinary binary file.
48+
* suffix. A .pdf name with NUL but no magic is malformed. A .pdf that is
49+
* otherwise valid UTF-8 is left as a normal text read so a misnamed text
50+
* file is not a new refusal.
5051
*/
5152
export function inspectionKindFromFirstChunk(
5253
filePath: string,
5354
chunk: Uint8Array,
5455
): FileInspectionKind | undefined {
5556
if (chunkLooksLikePdf(chunk)) return "pdf";
56-
if (pathLooksLikePdf(filePath)) return "pdf-malformed";
57-
if (chunk.includes(0)) return "binary";
57+
if (chunk.includes(0)) {
58+
return pathLooksLikePdf(filePath) ? "pdf-malformed" : "binary";
59+
}
5860
return undefined;
5961
}
6062

@@ -76,25 +78,25 @@ export function diagnoseBlockedFileInspection(
7678
return {
7779
code: "unreadable",
7880
message:
79-
`Cannot inspect ${path}: the file is unreadable (filesystem permission denied), not a missing extractor or worker-tool gap. ` +
80-
`Check ownership and mode, or copy the file into a readable workspace path.`,
81+
`Unreadable file (filesystem permission denied), not a missing extractor or worker-tool gap. ` +
82+
`Check ownership and mode, or copy it into a readable workspace path. File: ${path}`,
8183
};
8284
}
8385

8486
if (input.kind === "pdf-malformed") {
8587
return {
8688
code: "malformed",
8789
message:
88-
`Cannot inspect ${path}: the file is not a valid PDF (malformed or unreadable bytes), not a missing extractor or permission boundary. ` +
89-
`Open it in a PDF reader or re-export it, then retry.`,
90+
`Malformed PDF, not a missing extractor or permission boundary. ` +
91+
`Open it in a PDF reader or re-export it. File: ${path}`,
9092
};
9193
}
9294

9395
if (input.kind === "binary") {
9496
return {
9597
code: "binary",
9698
message:
97-
`refusing to read binary file: ${path}. This is a binary file, not a missing-tool or permission failure. ` +
99+
`refusing to read binary file: ${path}. Not a missing-tool or permission failure. ` +
98100
`Use a format-specific extractor if you need text from it.`,
99101
};
100102
}
@@ -104,24 +106,22 @@ export function diagnoseBlockedFileInspection(
104106
return {
105107
code: "missing_extractor",
106108
message:
107-
`Cannot inspect PDF ${path}: ${extractor} is not installed (missing PDF-extraction capability), not a malformed file. ` +
108-
`Install poppler so ${extractor} is on PATH (for example \`brew install poppler\` or \`apt-get install poppler-utils\`), then retry.`,
109+
`Missing PDF extractor: ${extractor} is not on PATH (capability gap), not a malformed file. ` +
110+
`Install poppler (\`brew install poppler\` or \`apt-get install poppler-utils\`), then retry. File: ${path}`,
109111
};
110112
}
111113

112114
if (!input.canExecuteHostCommands) {
113115
return {
114116
code: "permission_boundary",
115117
message:
116-
`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). ` +
117-
`Re-dispatch to a director that mounts run_shell, or grant this worker run_shell, then retry.`,
118+
`Permission boundary: ${extractor} is installed but this worker cannot execute it (run_shell is not mounted), not a malformed file. ` +
119+
`Re-dispatch to a director that mounts run_shell, or grant this worker run_shell. File: ${path}`,
118120
};
119121
}
120122

121123
return {
122124
code: "extractor_ready",
123-
message:
124-
`Cannot inspect PDF ${path} as text via read_file. ${extractor} is installed and this session can run host commands. ` +
125-
`Run \`${extractor} ${path} -\` via bash to extract text.`,
125+
message: `read_file cannot decode PDFs. ${extractor} is installed — run \`${extractor} ${path} -\` via bash.`,
126126
};
127127
}

‎src/plugins/read-file-guard-plugin.test.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ describe("readFileBounded", () => {
126126
);
127127
expect(isError).toBe(true);
128128
expect(content).toContain("binary");
129-
expect(content).toContain("not a missing-tool");
129+
expect(content).toContain("Not a missing-tool");
130130
});
131131

132132
test("a PDF without pdftotext names the missing extractor, not a malformed file", async () => {
@@ -140,7 +140,7 @@ describe("readFileBounded", () => {
140140
);
141141
expect(isError).toBe(true);
142142
expect(content).toContain("pdftotext");
143-
expect(content).toContain("missing");
143+
expect(content).toContain("Missing PDF extractor");
144144
expect(content).toContain("not a malformed file");
145145
expect(content).not.toContain("permission boundary");
146146
});
@@ -159,7 +159,7 @@ describe("readFileBounded", () => {
159159
);
160160
expect(isError).toBe(true);
161161
expect(content).toContain("installed");
162-
expect(content).toContain("permission boundary");
162+
expect(content).toContain("Permission boundary");
163163
expect(content).toContain("run_shell");
164164
expect(content).toContain("not a malformed file");
165165
});
@@ -174,7 +174,7 @@ describe("readFileBounded", () => {
174174
{ whichExtractor: () => null, canExecuteHostCommands: () => false },
175175
);
176176
expect(isError).toBe(true);
177-
expect(content).toContain("not a valid PDF");
177+
expect(content).toContain("Malformed PDF");
178178
expect(content).toContain("not a missing extractor");
179179
expect(content).not.toContain("brew install");
180180
});
@@ -807,8 +807,9 @@ describe("readFileGuardPlugin", () => {
807807
);
808808
expect(missingResult.isError).toBe(true);
809809
expect(String(missingResult.content)).toContain("pdftotext");
810-
expect(String(missingResult.content)).toContain("missing");
810+
expect(String(missingResult.content)).toContain("Missing PDF extractor");
811811
expect(String(missingResult.content)).toContain("not a malformed file");
812+
expect(String(missingResult.content)).toContain("flow.pdf");
812813

813814
const blocked = readFileGuardPlugin(dir, {
814815
whichExtractor: () => "/opt/homebrew/bin/pdftotext",
@@ -820,7 +821,7 @@ describe("readFileGuardPlugin", () => {
820821
);
821822
expect(blockedResult.isError).toBe(true);
822823
expect(String(blockedResult.content)).toContain("installed");
823-
expect(String(blockedResult.content)).toContain("permission boundary");
824+
expect(String(blockedResult.content)).toContain("Permission boundary");
824825
expect(String(blockedResult.content)).toContain("run_shell");
825826
expect(String(blockedResult.content)).not.toContain("brew install");
826827
});

‎src/plugins/read-file-guard-plugin.ts‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ export interface ReadFileGuardPluginOptions {
6060
* Workers flip this after the capability filter; omitted means yes.
6161
*/
6262
canExecuteHostCommands?: () => boolean;
63+
/** Model-facing path for diagnosis text (the call argument, not the absolute). */
64+
displayPath?: string;
6365
}
6466

6567
// A truncated read tells the model to continue with the same path and the
@@ -366,23 +368,24 @@ export function readFileBounded(
366368
signal: AbortSignal,
367369
inspection?: Pick<
368370
ReadFileGuardPluginOptions,
369-
"whichExtractor" | "canExecuteHostCommands"
371+
"whichExtractor" | "canExecuteHostCommands" | "displayPath"
370372
>,
371373
): Promise<BoundedRead> {
374+
const labeledPath = inspection?.displayPath ?? absolutePath;
372375
return readStreamBounded(
373376
createReadStream(absolutePath),
374377
absolutePath,
375378
offset,
376379
limit,
377380
signal,
378381
{
379-
mapStreamError: (err) => mapFilesystemStreamError(absolutePath, err),
382+
mapStreamError: (err) => mapFilesystemStreamError(labeledPath, err),
380383
windowHugeLines: true,
381384
diagnoseFirstChunk: (chunk) => {
382385
const kind = inspectionKindFromFirstChunk(absolutePath, chunk);
383386
if (kind === undefined) return undefined;
384387
return diagnoseBlockedFileInspection({
385-
path: absolutePath,
388+
path: labeledPath,
386389
kind,
387390
extractorAvailable: resolveExtractorProbe(inspection?.whichExtractor),
388391
canExecuteHostCommands:
@@ -549,13 +552,10 @@ export function readFileGuardPlugin(
549552
}
550553

551554
try {
552-
const res = await readFileBounded(
553-
absolutePath,
554-
offset,
555-
limit,
556-
signal,
557-
inspection,
558-
);
555+
const res = await readFileBounded(absolutePath, offset, limit, signal, {
556+
...inspection,
557+
displayPath: rawPath,
558+
});
559559
return res.isError
560560
? { callId: call.id, content: res.content, isError: true }
561561
: { callId: call.id, content: res.content };

0 commit comments

Comments
 (0)