feat(runtime): materialize verified native PDF inputs - #3266
Conversation
Gate PDF byte materialization on explicit first-party provider and wire contracts, then share the durable attachment path across current turns, replay, steering, and compaction. Add PDF and combined binary budgets plus exact provider-wire and fail-closed regression coverage. Generated-by: OpenAI Codex
MoonOld
left a comment
There was a problem hiding this comment.
One focused blocker before this PR closes #3164:
P2 — durable replay drops PDF attachments from steering messages. The live steering path materializes attachments through appendAttachmentParts, but materializeRuntimeReplayItem returns item.content directly whenever item.steering is set, even though the replay plan preserves item.attachments. A later Turn or restart therefore replays the steering envelope text without the PDF file part.
Please route this branch through appendAttachmentParts using the stable steering:${item.steering.eventId} decision key, preserve steeringProviderOptions, and extend the existing prior-turn steering replay test with a PDF assertion.
I rechecked the scope and narrowed #3164: base-URL allowlisting, the removed Headless path, and encrypted/page-limit/provider-rejection-specific recovery are not blockers for this first slice.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the provider evidence in #3164 and the wire-lowering tests here made the contract straightforward to check against the real adapters rather than against claims.
Reviewed exact head 303d20a3a295cfa54be20f7aaacaebe0a8707e87.
The provider/wire gate is the right minimal shape: resolveModelPdfInputContract fails closed for relays, subscriptions, and unknown models, an explicit provider inventory outranks generated modality metadata, and the three wire shapes are locked against the real SDK adapters rather than asserted. The turn budget is created once in TurnScope and the decision keys line up across current turn, RuntimeEvent replay, and steering. The implementation does not yet satisfy its bounded-work contract, so I recommend revising it before approval.
The concrete current-head findings are inline. At the architecture level the gap is that "bounded" is enforced on what reaches the provider payload, but not on the work done to produce and observe that payload: bytes are read again per step after a decision is already cached, and the always-on request telemetry expands them element-wise on every step. Both belong before materialization, not after.
@MoonOld's steering finding reproduces on this head, and it is pre-existing for images as well — materializeRuntimeReplayItem ignores item.attachments on the steering branch on current main too, while the replay plan does preserve them. A steered PDF replays as envelope text plus an <attachment> ref and zero file parts. Routing that branch through appendAttachmentParts with steering:${item.steering.eventId} fixes it and keeps dedupe aligned with the live drain and the stored-message sidecar.
Verification: I built this head in an isolated worktree and ran packages/runtime/dist/__tests__/ai-sdk-backend.test.js — 210/212 pass, the two failures being regressions I wrote for the steering gap and the per-step re-read. A 52-line change (cache-before-read, plus the steering branch) takes it to 212/212, including enforces the PDF subtype budget from bytes read, not attachment metadata, so the "charge actual bytes" semantics survive the fix.
Review disclosure: this review was prepared with Claude Code, which read the diff, traced the call paths, and executed the reproductions and measurements quoted inline against this head. The human contributor reviewed the findings and the measurements and chose to publish this COMMENT review. The P1 timings come from a single macOS machine and are order-of-magnitude evidence, not a benchmark. No CI checks are currently reported for this head.
| * 21.4 MiB, leaving headroom under Anthropic's 32 MiB whole-request limit for | ||
| * text, tool schemas, JSON framing, and other content. | ||
| */ | ||
| export const MAX_PROVIDER_PDF_REQUEST_BYTES = 16 * 1024 * 1024; |
There was a problem hiding this comment.
[P1] Bound the work the request telemetry does on these bytes before raising the cap that makes them routine. capturePreparedProviderRequest runs on every provider request — the tracker exists whenever model-call accounting does, so this is not gated on recordProviderRequestCapture — and canonicalize has no typed-array branch, so a Uint8Array is expanded element-wise through Object.keys().sort(). Measured against this repository's built packages/runtime/dist with capture persistence disabled: an 8 MiB attachment costs 3.5 s per step, 12 MiB (today's image cap) 7.5 s, and 16 MiB (this cap) 9.5 s per step with requestBytes reported as 222.4 MiB; across three steps that is 28.5 s during which a 10 ms timer scheduled beforehand never fires, and RSS grows 4.4 to 7.6 GiB. With capture persistence enabled the bytes are also verbatim in serializedRequest — I matched the %PDF- header inside it — which contradicts this PR's "without copying bytes into transcript text or diagnostics" and #3164's "PDF bytes must never enter transcript text, logs, RuntimeEvents, or diagnostics". This is not introduced here; images take the same path on main. But this slice raises the ceiling to 18 MiB combined and makes ten-plus-MiB attachments ordinary, because a 16 MiB PDF is a normal document and a 16 MiB PNG is not. Give request-shape.ts a binary branch that represents file data as { byteLength, sha256 } — substituting that summary drops the same call from 9.5 s to 0.4 ms and requestBytes from 222.4 MiB to 312 B — and add a regression asserting that a file part's bytes never appear in serializedRequest. Landing it as a separate prerequisite is fine; landing native PDF input without it is not.
| } | ||
| let read: Awaited<ReturnType<AttachmentByteReader>>; | ||
| try { | ||
| read = await this.input.readAttachmentBytes(attachment.ref); |
There was a problem hiding this comment.
[P2] Consult the decision cache before reading the bytes, not after. The cache is checked inside chargeAttachmentBudget, which runs once readAttachmentBytes has already returned, so an attachment whose omission was decided on the first step is read in full again on every later step; materializeToolResultOutput below already does this in the correct order. Reproduced on a durable turn with loadTurnRuntimeEvents wired, as Host composition does: reads equal steps + 1 for the omitted ref — 3 reads over 2 steps, 5 over 4. In production each read is a full readDurableAttachmentBinary with no maxBytes passed, so up to MAX_ATTACHMENT_BYTES (50 MiB), serialized on the ArtifactStore queue and blocking other artifact work in the session. Move the decisionKey computation and a non-keep early return above the read, and add a multi-step regression counting reads per ref.
| (attachment.kind === 'image' && this.input.supportsVision === true) || | ||
| (attachment.kind === 'pdf' && this.input.pdfInputContract !== undefined); | ||
| if (!isEligible) continue; | ||
| if (attachment.kind === 'pdf' && attachment.mimeType !== 'application/pdf') { |
There was a problem hiding this comment.
[P3] Degrade a mislabeled PDF locally instead of spending a provider round trip on it. This check compares declared metadata only, but readPreparedBinary already sniffs magic bytes and returns the true MIME — %PDF- is in the allowlist, so a file that is not any allowed binary type does fail closed with unsupported_mime. createAttachmentByteReader then discards result.mimeType, so a PNG named brief.pdf is uploaded as application/pdf and degrades through a provider 400 rather than the bounded local explanation #3164 asks for. Surface the sniffed MIME through the reader and compare it here; that is smaller than the preflight parsing deferred to #3284.
| * accept an AI SDK PDF file part. Provider identity is intentional: a relay | ||
| * using the same adapter or wire does not inherit first-party PDF support. | ||
| */ | ||
| export type ModelPdfInputContract = |
There was a problem hiding this comment.
[P3] Say provider identity, not endpoint, in this contract's wording, and give the union a consumer or drop it. The gate is providerType plus wire; the endpoint is not checked, and this PR's own test authorizes providerType: 'openai' with baseUrl: 'https://provider.invalid/v1'. Narrowing base-URL allowlisting out of this slice is reasonable and images are in the same position, but "verified first-party provider wire" reads as endpoint verification. Separately, nothing consumes providerType or wire — AiSdkBackend only tests the contract for presence — so either branch on the wire where it matters, such as per-wire caps, or let it be a boolean.
| const fallbackText = hasUnsupportedImages | ||
| ? appendNonVisionImageFallbackNotice(textContent) | ||
| : textContent; | ||
| const eligibleAttachments = binaryAttachments.filter( |
There was a problem hiding this comment.
[P3] Keep one copy of the eligibility rule. eligibleAttachments is computed only to decide the early return and the loop recomputes the same predicate per item, so the two must be kept in sync by hand. Iterating eligibleAttachments directly removes the duplicate; note the decision key indexes into binaryAttachments, so whichever array you iterate has to stay consistent with the key.
Summary
document, OpenAI Chatfile, and OpenAI Responsesinput_file, including a Host-to-provider streaming integration test.Fixes #3164
Verification
npm run build:test— all workspaces built successfully.npm run typecheck— all workspace typechecks passed.biome lint .— 2,451 files checked, passed.biome format .— 1,544 files checked, passed.git diff --checkand staged secret-pattern scan — passed.The full Runtime command was also attempted on Windows. The affected PDF/attachment tests passed, but the overall suite did not finish cleanly because unrelated platform fixtures failed or hung in SQLite temp-file cleanup (
EBUSY), process-tree, shell, and PTY paths. The 11 SQLite failures reproduced in isolation with the same locked-file cleanup error and do not touch changed code.Protected OpenAI/Anthropic live calls were not run because no provider credentials were available. Exact request bodies are covered against the locked SDK adapters, and the Host composition test exercises artifact reading, backend construction, streaming transport, and final OpenAI Chat wire serialization without an external credential.
Review focus
The provider/wire contract is intentionally narrower than SDK encoding support: a compatible relay does not receive raw PDF bytes merely because its adapter can serialize a file part. Budget accounting uses bytes actually read, does not charge failed reads, preserves the existing image limit, and applies one combined cap before Base64 expansion.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with issue and duplicate-PR analysis, implementation, tests, local verification, and PR drafting. @tianrking requested the contribution and remains the human contributor of record.
Checklist
Does this PR entail a change in behavior?