Skip to content

Commit 7240830

Browse files
steipetesteipete
committed
perf(pdf): extract input validation seam
1 parent e2f0ea4 commit 7240830

4 files changed

Lines changed: 64 additions & 49 deletions

File tree

‎src/agents/tools/pdf-tool.helpers.test.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
import type { OpenClawConfig } from "../../config/config.js";
21
import { describe, expect, it } from "vitest";
2+
import type { OpenClawConfig } from "../../config/config.js";
33
import {
44
coercePdfAssistantText,
55
coercePdfModelConfig,
66
parsePageRange,
77
providerSupportsNativePdf,
8+
resolvePdfInputs,
89
resolvePdfToolMaxTokens,
910
} from "./pdf-tool.helpers.js";
1011

@@ -76,6 +77,19 @@ describe("providerSupportsNativePdf", () => {
7677
});
7778

7879
describe("pdf-tool.helpers", () => {
80+
it("resolvePdfInputs requires at least one pdf reference", () => {
81+
expect(() => resolvePdfInputs({ prompt: "test" })).toThrow("pdf required");
82+
});
83+
84+
it("resolvePdfInputs deduplicates pdf and pdfs entries", () => {
85+
expect(
86+
resolvePdfInputs({
87+
pdf: " /tmp/nonexistent.pdf ",
88+
pdfs: ["/tmp/nonexistent.pdf", " ", "/tmp/other.pdf"],
89+
}),
90+
).toEqual(["/tmp/nonexistent.pdf", "/tmp/other.pdf"]);
91+
});
92+
7993
it("resolvePdfToolMaxTokens respects model limit", () => {
8094
expect(resolvePdfToolMaxTokens(2048, 4096)).toBe(2048);
8195
expect(resolvePdfToolMaxTokens(8192, 4096)).toBe(4096);

‎src/agents/tools/pdf-tool.helpers.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,31 @@ import { extractAssistantText } from "../pi-embedded-utils.js";
99

1010
export type PdfModelConfig = { primary?: string; fallbacks?: string[] };
1111

12+
export function resolvePdfInputs(record: Record<string, unknown>): string[] {
13+
const pdfCandidates: string[] = [];
14+
if (typeof record.pdf === "string") {
15+
pdfCandidates.push(record.pdf);
16+
}
17+
if (Array.isArray(record.pdfs)) {
18+
pdfCandidates.push(...record.pdfs.filter((v): v is string => typeof v === "string"));
19+
}
20+
21+
const seenPdfs = new Set<string>();
22+
const pdfInputs: string[] = [];
23+
for (const candidate of pdfCandidates) {
24+
const trimmed = candidate.trim();
25+
if (!trimmed || seenPdfs.has(trimmed)) {
26+
continue;
27+
}
28+
seenPdfs.add(trimmed);
29+
pdfInputs.push(trimmed);
30+
}
31+
if (pdfInputs.length === 0) {
32+
throw new Error("pdf required: provide a path or URL to a PDF document");
33+
}
34+
return pdfInputs;
35+
}
36+
1237
/**
1338
* Check whether a provider supports native PDF document input.
1439
*/

‎src/agents/tools/pdf-tool.test.ts‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,11 @@ const FAKE_PDF_MEDIA = {
4848
fileName: "doc.pdf",
4949
} as const;
5050

51-
function requirePdfTool(tool: Awaited<ReturnType<typeof loadCreatePdfTool>> extends (...args: any[]) => infer R ? R : never) {
51+
function requirePdfTool(
52+
tool: Awaited<ReturnType<typeof loadCreatePdfTool>> extends (...args: any[]) => infer R
53+
? R
54+
: never,
55+
) {
5256
expect(tool).not.toBeNull();
5357
if (!tool) {
5458
throw new Error("expected pdf tool");
@@ -69,6 +73,16 @@ async function withAnthropicPdfTool(
6973
});
7074
}
7175

76+
async function withConfiguredPdfTool(
77+
run: (tool: PdfToolInstance, agentDir: string) => Promise<void>,
78+
) {
79+
await withTempAgentDir(async (agentDir) => {
80+
const cfg = withPdfModel(ANTHROPIC_PDF_MODEL);
81+
const tool = requirePdfTool((await loadCreatePdfTool())({ config: cfg, agentDir }));
82+
await run(tool, agentDir);
83+
});
84+
}
85+
7286
function resetAuthEnv() {
7387
vi.stubEnv("OPENAI_API_KEY", "");
7488
vi.stubEnv("ANTHROPIC_API_KEY", "");
@@ -154,22 +168,22 @@ describe("createPdfTool", () => {
154168
expect(() => createTool({ config: cfg })).toThrow("requires agentDir");
155169
});
156170

157-
it("creates tool when auth is available", async () => {
158-
await withAnthropicPdfTool(async (tool) => {
171+
it("creates tool when a PDF model is configured", async () => {
172+
await withConfiguredPdfTool(async (tool) => {
159173
expect(tool.name).toBe("pdf");
160174
expect(tool.label).toBe("PDF");
161175
expect(tool.description).toContain("PDF documents");
162176
});
163177
});
164178

165179
it("rejects when no pdf input provided", async () => {
166-
await withAnthropicPdfTool(async (tool) => {
180+
await withConfiguredPdfTool(async (tool) => {
167181
await expect(tool.execute("t1", { prompt: "test" })).rejects.toThrow("pdf required");
168182
});
169183
});
170184

171185
it("rejects too many PDFs", async () => {
172-
await withAnthropicPdfTool(async (tool) => {
186+
await withConfiguredPdfTool(async (tool) => {
173187
const manyPdfs = Array.from({ length: 15 }, (_, i) => `/tmp/doc${i}.pdf`);
174188
const result = await tool.execute("t1", { prompt: "test", pdfs: manyPdfs });
175189
expect(result).toMatchObject({
@@ -180,11 +194,10 @@ describe("createPdfTool", () => {
180194

181195
it("respects fsPolicy.workspaceOnly for non-sandbox pdf paths", async () => {
182196
await withTempAgentDir(async (agentDir) => {
183-
vi.stubEnv("ANTHROPIC_API_KEY", "anthropic-test");
184197
const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pdf-ws-"));
185198
const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pdf-out-"));
186199
try {
187-
const cfg = withDefaultModel(ANTHROPIC_PDF_MODEL);
200+
const cfg = withPdfModel(ANTHROPIC_PDF_MODEL);
188201
const tool = requirePdfTool(
189202
(await loadCreatePdfTool())({
190203
config: cfg,
@@ -208,7 +221,7 @@ describe("createPdfTool", () => {
208221
});
209222

210223
it("rejects unsupported scheme references", async () => {
211-
await withAnthropicPdfTool(async (tool) => {
224+
await withConfiguredPdfTool(async (tool) => {
212225
const result = await tool.execute("t1", {
213226
prompt: "test",
214227
pdf: "ftp://example.com/doc.pdf",
@@ -219,24 +232,6 @@ describe("createPdfTool", () => {
219232
});
220233
});
221234

222-
it("deduplicates pdf inputs before loading", async () => {
223-
await withTempAgentDir(async (agentDir) => {
224-
const { loadSpy } = await stubPdfToolInfra(agentDir, { modelFound: false });
225-
const cfg = withPdfModel(ANTHROPIC_PDF_MODEL);
226-
const tool = requirePdfTool((await loadCreatePdfTool())({ config: cfg, agentDir }));
227-
228-
await expect(
229-
tool.execute("t1", {
230-
prompt: "test",
231-
pdf: "/tmp/nonexistent.pdf",
232-
pdfs: ["/tmp/nonexistent.pdf"],
233-
}),
234-
).rejects.toThrow("Unknown model");
235-
236-
expect(loadSpy).toHaveBeenCalledTimes(1);
237-
});
238-
});
239-
240235
it("uses native PDF path without eager extraction", async () => {
241236
await withTempAgentDir(async (agentDir) => {
242237
await stubPdfToolInfra(agentDir, { provider: "anthropic", input: ["text", "document"] });

‎src/agents/tools/pdf-tool.ts‎

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import { loadWebMediaRaw } from "../../media/web-media.js";
66
import { normalizeOptionalString } from "../../shared/string-coerce.js";
77
import { resolveUserPath } from "../../utils.js";
88
import { type ImageModelConfig } from "./image-tool.helpers.js";
9-
import { resolvePdfModelConfigForTool } from "./pdf-tool.model-config.js";
109
import {
1110
applyImageModelConfigDefaults,
1211
buildTextToolResult,
@@ -21,8 +20,10 @@ import {
2120
coercePdfModelConfig,
2221
parsePageRange,
2322
providerSupportsNativePdf,
23+
resolvePdfInputs,
2424
resolvePdfToolMaxTokens,
2525
} from "./pdf-tool.helpers.js";
26+
import { resolvePdfModelConfigForTool } from "./pdf-tool.model-config.js";
2627
import {
2728
createSandboxBridgeReadFile,
2829
discoverAuthStorage,
@@ -277,27 +278,7 @@ export function createPdfTool(options?: {
277278
const record = args && typeof args === "object" ? (args as Record<string, unknown>) : {};
278279

279280
// MARK: - Normalize pdf + pdfs input
280-
const pdfCandidates: string[] = [];
281-
if (typeof record.pdf === "string") {
282-
pdfCandidates.push(record.pdf);
283-
}
284-
if (Array.isArray(record.pdfs)) {
285-
pdfCandidates.push(...record.pdfs.filter((v): v is string => typeof v === "string"));
286-
}
287-
288-
const seenPdfs = new Set<string>();
289-
const pdfInputs: string[] = [];
290-
for (const candidate of pdfCandidates) {
291-
const trimmed = candidate.trim();
292-
if (!trimmed || seenPdfs.has(trimmed)) {
293-
continue;
294-
}
295-
seenPdfs.add(trimmed);
296-
pdfInputs.push(trimmed);
297-
}
298-
if (pdfInputs.length === 0) {
299-
throw new Error("pdf required: provide a path or URL to a PDF document");
300-
}
281+
const pdfInputs = resolvePdfInputs(record);
301282

302283
// Enforce max PDFs cap
303284
if (pdfInputs.length > DEFAULT_MAX_PDFS) {

0 commit comments

Comments
 (0)