📭 fix: Deliver Attachment Text Until a File Tool Holds It - #16027
Conversation
A `none`-routed upload had its extracted text withheld whenever a file tool was enabled, on the assumption the tool would serve the file. A plain chat with the File Search toggle runs an ephemeral agent, so the upload is filed under no tool resource and never embedded: the text was withheld for a vector store that never received the file, and the attachment reached nothing. Withholding now requires the record to show the tool holds the file, which is the same evidence deferred provisioning reads before queueing it. `resources.ts` delegates to that shared predicate so the two cannot disagree again.
|
Ready for review at head That head carries the whole change: The interesting review question is the pairing: evidence for one tool must not qualify the other, and a tool that holds the file but cannot read the type must not count. Both are covered in Checks at this head: 91 data-provider, 511 packages/api (agents/files, resources, initialize, files/upload, files/provision), 148 |
|
New head Adds one test fixture fix on top of the previous head. Locally at this head: On CI, note that
|
|
@codex review the latest head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…reads (#16058) Since #15694 bounded each turn's attachments across history, a thread whose history counts more than `fileLimit` model-bound files is refused on every turn, including turns that attach nothing. Two kinds of file reached that count that never belonged in the prompt. Code outputs: priming clears an expired sandbox reference on the turn's copy of the record so the file is re-provisioned, and a route-less record without a reference was classified as prompt content, so it counted toward the limit and could be encoded as media. Code outputs now stay tool-owned regardless of reference liveness, through one predicate shared by admission, BaseClient delivery and the child run-file encoder. Tool-routed spreadsheets: with `textFallbackWithoutTools`, #16027 delivered the fallback text until a tool held a copy. Run Code receives its copy only on its first call, which a refused turn never makes, so the text counted on every later turn. An enabled Run Code is a reader again for the types it can read; File Search still reads only what its store holds.
Summary
Turning on the File Search toggle in a plain chat made an upload invisible to the model. Uploading a PDF with the toggle off worked, the extracted text was delivered; uploading the same PDF with the toggle on produced an answer as if no file were attached, with no error and nothing in the log. Enabling a file tool was strictly worse than leaving it off.
Two halves of the pipeline disagreed about what "this conversation has a file tool" means. At upload time the destination is chosen from the agent record, and a plain chat runs an ephemeral agent that has no record, so no tool claims the file and it is never embedded. At turn time the consumers are derived from the live tool set, which does include the toggle, so
resolveTurnLLMDeliveryPathsuppressed thetextFallbackWithoutToolstext on the assumption that retrieval would serve the file. The vector store had never received it, so nothing did: the upload path concluded "no file tool, do not file it" and the turn path concluded "there is a file tool, withhold the text".Withholding the text now requires the record to show that a tool this turn runs actually holds the file, which is the evidence deferred provisioning already reads before queueing one. An attachment no tool has a copy of is delivered as text, on the turn it arrives and on every later turn, and lazy provisioning still embeds it on the first search, so the file is both readable and searchable. A file the sandbox or the vector store does hold is untouched and stays with its tool.
Fixes #15970 (the regression half). The unembedded-upload and
.pptxitems stay with #15420, which describes them more precisely.How it works
The delivery decision asks the record, not the tool set, and shares one predicate with the provisioning queue:
The evidence is paired with the tool that can read the type, so vectors do not qualify a code-only turn and a sandbox pointer does not qualify a retrieval-only one.
packages/api/src/agents/resources.tsnow callshasToolResourceProvisioningin place of its own copy of that logic, which is what stops the two sides drifting apart again.Every reader of a turn route goes through the same resolver, so one change covers the first turn, the resend path, and a handoff child:
Change Type
Testing
Reproduction is the issue's: a custom OpenAI-compatible endpoint with
textFallbackWithoutTools: trueandapplication/pdfoverridden tonone, a new plain chat, File Search toggled on, a text-bearing PDF attached. The model now answers from the extracted text, and the file is embedded on the first search rather than never.Tests cover both sides of the disagreement, at each layer that resolves a route:
packages/data-providerresolve-llm-delivery-path.spec.tspackages/apiagents/files, resources, initialize, files/upload, files/provisionapiBaseClient.test.jsnpx tsc --noEmitinpackages/data-providerandpackages/apinpm run static-checks(staged)New cases: File Search enabled but the vector store never received the file delivers text; Run Code enabled with no sandbox reference delivers text; a tool that holds the file but cannot read the type delivers text; and the same for the historical resend path and a handoff child. Three existing tests asserted that an enabled tool keeps a file off the prompt using fixtures that carried no provisioning evidence; they now carry the reference or the embedding, which preserves what each was written to check.
Not run:
npm run lighthouse(no startup, config, or message-loading path changed) andapi/server/routes/files/files.test.js(upload route untouched by this change).Test Configuration:
Checklist