fix(providers): route 6-10MB attachments to the provider large-file path - #6232
Merged
Conversation
The inline base64 cap was 10 MB of raw bytes, but the execution payload store refuses a single value above 8 MiB and base64 inflates by 4/3. Every raw file over 6 MiB therefore produced a base64 string the store rejected — and the rejection came from the base64 *cache* write, which threw and failed the run with "Execution memory limit exceeded" even though the bytes had already been read successfully. Because shouldUseLargeFilePath only fires above the inline cap, 6-10 MB attachments had no path at all on any provider: they never reached the OpenAI or Gemini Files API upload they were supposed to take. Derive the cap from the payload-store ceiling instead of hardcoding it, and degrade a refused cache write to "not cached" rather than failing a request whose bytes are in hand. Every other size guard in the chain compares raw bytes against maxBytes; only the Redis write sees the encoded size, which is why this went unnoticed — and why it failed only where Redis is configured. Also correct the provider ceilings against the vendors' current documentation: - openai: 50 MiB -> 50,000,000. The gate is `size > maxBytes`, so 50 MiB admitted 52,428,800 bytes; the docs say each file must be *under* 50 MB. - bedrock: had no entry and inherited the inline cap, which is above what Converse accepts (3.75 MB per image, 4.5 MB per document). - groq: 20 MiB -> 20,000,000, and modelled as the request cap the docs actually describe rather than a per-file MiB ceiling. - fireworks: had no entry; its 10 MB budget is on the base64 total, so the raw-byte equivalent is 7.5 MB. Add perRequestMaxBytes for the combined ceilings, enforced before any upload spend, and cover the OpenAI upload path end to end — it had no test at all.
… 0.115.0
@google/genai 2.x reworks the Interactions API, which the Gemini deep-research
provider is built on. Migrate it:
- `Interaction.outputs` (a flat content array) is now `steps`, a discriminated
timeline; the report text lives in the `model_output` steps' text content,
alongside thought and tool steps we skip.
- `Usage.total_reasoning_tokens` is now `total_thought_tokens`. The old code
already fell back to that name through a cast, so this just makes the field
the SDK actually returns the typed one.
- SSE events renamed: `content.delta` -> `step.delta`, `interaction.start` ->
`interaction.created`, `interaction.complete` -> `interaction.completed`.
The new event types are discriminated, so the payload casts are gone.
Both `interactions.create` calls also stop annotating their params with
`Interactions.CreateAgentInteractionParams{,Non}Streaming`. In 2.13.0 those
namespace aliases resolve to `CreateAgentInteraction`, whose `stream` is a
plain `boolean` rather than a literal — annotating with them erases the
discriminant and the call resolves to the union-returning overload, so the
result is typed as `Interaction | Stream` at every use. An inline
`stream: true as const` keeps the correct overload.
Neither upgrade required a `minimum-release-age` waiver: 2.15.0 and 0.115.0
were checked and 2.13.0 is the newest genai release clearing the 7-day window.
v5 is the only major with real breaking changes for us; v6 widened a Responses output type and v7 only raised the Node floor to 22, which apps/sim already requires. Three things needed fixing: `ChatCompletionMessageToolCall` became a union of function and custom tool calls, and the custom variant has no `function` field — 43 unguarded `.function` accesses across the OpenAI-compatible providers. Narrow once at each `message.tool_calls` read through a shared `isFunctionToolCall` guard rather than casting at every use. That guard deliberately tests for the `function` payload instead of `type === 'function'`. Many OpenAI-compatible vendors omit `type` on tool calls entirely — our own fixtures do — so discriminating on it type-checks perfectly and then silently drops every tool call those providers return. `ChatCompletionCreateParams.verbosity` narrowed from `string` to a literal union, and the Responses API's output and input item unions now diverge on members Sim never emits (computer-use call outputs, whose `status` admits `failed`, and the `AdditionalTools` escape hatch). Echoing output back as input is what a tool loop is supposed to do, so that conversion is asserted once in convertResponseOutputToInputItems and the streaming loop now routes through it instead of pushing raw output items. The hand-rolled multipart upload in file-attachments.server.ts can now be replaced with the SDK's typed `expires_after` — left for a follow-up so this commit stays a pure upgrade.
… changes
The mechanical rewrite that added `isFunctionToolCall` to every `tool_calls`
read also rewrote three truthiness guards, where the filtered array was
computed, discarded, and the unfiltered value used in the body. Filter once and
use that value. The helper also landed between `trackForcedToolUsage`'s TSDoc
block and its declaration, leaving that block documenting the wrong function.
Raise the Bedrock ceiling from 3.75 MB to 4.5 MB. Converse caps an image at
3.75 MB and a document at 4.5 MB, and a single `maxBytes` cannot express both.
Taking the lower bound looked conservative but regressed 3.75-4.5 MB documents,
which Converse accepts and which work today. At the document bound every size
that works now still works, and only genuinely-too-large files are rejected
early; oversized images in that band keep surfacing as a Bedrock API error,
exactly as they do without the entry.
Both limits re-verified verbatim against the primary docs: Converse's Message
reference ("Each image's size ... no more than 3.75 MB", "Each document's size
must be no more than 4.5 MB") and Fireworks' vision guide ("Total base64-encoded
images must be less than 10MB").
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Attachment routing introduces Base64 cache no longer throws on Redis budget or single-key size limits after a successful read—it logs and skips the cache write so runs still return base64. Errors and limits use SDK upgrades ( Tests cover the reported ~9.6 MB CSV path, cache skip on oversized values, attachment size formatting, and large-file lifecycle. Reviewed by Cursor Bugbot for commit 0356912. Configure here. |
Contributor
Greptile SummaryThe PR separates the inline attachment ceiling from the provider-upload crossover so attachments that are unsafe to cache as base64 can use provider large-file delivery.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/handlers/agent/agent-handler.ts | Selects a provider- and deployment-aware hydration ceiling and validates that each attachment retains either inline bytes or a reachable large-file path. |
| apps/sim/providers/attachments.ts | Introduces strategy-specific crossover decisions, corrected size formatting, and provider attachment routing helpers. |
| apps/sim/providers/file-attachments.server.ts | Adds storage-aware large-file eligibility and provider upload routing while preserving inline fallback when cloud storage is unavailable. |
| apps/sim/lib/uploads/utils/user-file-base64.server.ts | Converts execution-budget cache refusals into non-fatal skipped writes after attachment bytes have been materialized. |
| apps/sim/providers/gemini/core.ts | Migrates deep-research handling to the Interactions v2 step and event schema. |
| apps/sim/providers/openai/utils.ts | Updates OpenAI response and tool-call handling for the upgraded SDK unions. |
| apps/sim/package.json | Upgrades the OpenAI, Google GenAI, and Anthropic SDK dependencies. |
| apps/sim/providers/file-attachments.server.test.ts | Adds end-to-end coverage for the large-file upload lifecycle and crossover behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent attachment] --> B[Resolve provider strategy and storage availability]
B --> C{Files API reachable and file above crossover?}
C -- Yes --> D[Skip base64 hydration]
D --> E[Create trusted signed storage URL]
E --> F[Upload to provider Files API]
F --> G[Build provider message with file ID or URI]
C -- No --> H{Within inline ceiling?}
H -- Yes --> I[Read bytes and encode base64]
I --> J{Cache write accepted?}
J -- Yes --> K[Cache base64]
J -- No --> L[Continue without cache]
K --> M[Build inline provider message]
L --> M
H -- No --> N[Return actionable attachment error]
Reviews (8): Last reviewed commit: "fix(providers): order the attachment fai..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Auditing the routing change for backwards compatibility turned up two bands it silently broke. Lowering the single inline cap made the upload path mandatory above ~6 MB, but every large-file path reads its bytes back out of cloud object storage. A deployment without it — local dev, any disk-backed self-host — inlines those files as base64 today and would have started failing outright with "requires cloud file storage". Split the one number in two: the inline ceiling stays at 10 MiB, and a separate threshold marks where an upload becomes *preferable* because the base64 copy no longer fits the payload store. Where no upload path is reachable, base64 hydration now runs to the inline ceiling as before, and a missing cloud-storage backend leaves the file for the inline path instead of throwing. The two strategies also cross over at different sizes now. `files-api` carries every type the provider already accepts, so it takes over at the lower threshold. `remote-url` only fetches images and PDFs, so switching early would have started rejecting 6-10 MB text documents that inline fine today; it takes over only once inlining is genuinely impossible. Revert the Groq ceilings. Its published "20MB" governs a request carrying an image URL, and on this path the body holds only the URL, so it cannot bind on the files maxBytes guards. Groq documents no limit on the image it fetches, so tightening the per-file cap to 20,000,000 and summing raw bytes against the request cap would both reject uploads that work today on no documented basis.
Collaborator
Author
Collaborator
Author
|
@cursor review |
… findings A six-agent line-by-line audit against the vendors' live docs found the `models.ts` ceiling work was not the strict improvement it was written as, so all of it is reverted: - bedrock's 4.5 MB cap broke video. Converse takes image, document AND video blocks, and video is allowed 25 MB base64 — a single `maxBytes` cannot express three content classes, and every 4.5-10 MiB `.mp4` that works today would have started failing. - openai's combined 50 MB cap is the FILE-input limit. Image inputs are governed separately at 512 MB / 1500 images, so summing every attachment rejected eight 8 MB PNGs that OpenAI documents as legal. - fireworks' per-file ceiling was unreachable behind the request budget, while the upload picker went on advertising it — a size the UI accepts and execution always rejects. - The whole `perRequestMaxBytes` feature goes with them: it summed raw bytes against caps that are variously on encoded bytes, on one content class, or on a body that carries only URLs, and it double-counted a file referenced from several messages even though the uploader dedupes by key. Only openai's per-file `maxBytes` stays corrected, to decimal 50,000,000 — the one number a vendor states unambiguously and writes no MiB against. Also fixed, all found by the same audit: The hydration cap stopped short of where `remote-url` actually switches over, so 6-10 MiB attachments on anthropic/openrouter/xai/groq/together/baseten/vllm had neither base64 nor a handle and failed outright — the very band this branch exists to fix. Both decisions now come from one function so they cannot drift. Eight more sites where the mechanical rewrite computed a filtered array and then read the unfiltered one (deepseek, sakana, nvidia, kimi), leaving those providers without the narrowing they appear to have. `isFunctionToolCall` threw on a null or primitive `tool_calls` entry, because `in` requires an object — reachable exactly on the self-hosted gateways this filter was added for. It is now total, and all 32 test mocks match it rather than being quietly more permissive. `checkForForcedToolUsage` in utils/litellm/mistral evaluated the response before the `tool_choice` test, turning a tolerated malformed body into a TypeError on a path that never used to touch it. Gemini: `satisfies` restores the excess-property checking the dropped annotations removed, the poll loop recognises the terminal statuses v2 added instead of spinning for an hour and reporting a timeout, and the streaming doc block no longer names five events that were renamed six lines below it.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ade82d4. Configure here.
The size ceilings are decimal MB — that is how OpenAI, AWS and Fireworks all write them — but the error messages divided by 1024², so OpenAI's 50 MB cap was reported to the user as "48MB". Someone shrinking a 49 MB file to get under it was chasing a limit that does not exist. One formatter, used by all three messages, so the file size and the ceiling in the same sentence are always in the same unit.
Collaborator
Author
Collaborator
Author
|
@cursor review |
The previous commit fixed OpenAI's "48MB" by dividing every ceiling by 10⁶ — which broke the other seven. Only OpenAI's constant is decimal; anthropic, google, together and openrouter are 50 MiB, baseten and vllm 25 MiB, groq and xai 20 MiB. Rendering those as decimal MB overstated each by ~5%, so a 21 MB file on groq was rejected with "(21MB) exceeds the 21MB limit" — a sentence that contradicts itself and sends the user to shrink a file to a size that is still over. Same class of bug as the one being fixed, sign flipped. Both figures now render through one unit taken from the ceiling, so the number a user is told is the number the vendor publishes and the two sizes in a sentence are always comparable. Tested against every ceiling in the registry rather than only the values that happened to round cleanly. Two more from the same audit: A file with a missing or zero declared size was stranded on a files-api provider: hydration bailed on the real byte length while `shouldUseLargeFilePath` saw `0 > threshold` as false, so it got neither base64 nor a handle and failed as "may no longer be accessible" — a size failure wearing an access failure's message. Uploads read the real bytes and enforce the ceiling themselves, so an unknown size now routes to one. The oversized-attachment error blamed the provider for a deployment problem: a files-api provider on a host without cloud storage reported that the provider "has no large-file upload path", which is not true of the provider. `isFunctionToolCall` only proves `function` is present, never that it is well formed, so the trace enricher is defensive again about a hollow payload without giving up the compile-time gate. The 32 test mocks now match production exactly.
Collaborator
Author
Collaborator
Author
|
@cursor review |
Deriving the unit from the ceiling fixed the 5% error but left the precision fixed at two decimals, so a file one byte over a 20 MiB cap still printed "(20.00MB) exceeds the 20MB agent attachment limit" — the same self-contradicting sentence, now in a ~5 KB band above every ceiling in the registry. The size rounds up and the ceiling rounds down, so the two can no longer collide. The test that was supposed to guard this asserted a file 0.03MB over and an OpenAI file that was under the limit — neither anywhere near the band — so it passed while the bug was live. It now walks `limit + 1` for every ceiling, and goes red against the old rounding. The reason clause added last commit also claimed a deployment had no cloud file storage whenever the strategy was not inline. A generated document on a remote-url provider reaches that same error with storage fully configured, because a signed URL points at the generation source rather than the rendered artifact — so it was told something false about its own deployment. That case now names itself.
Collaborator
Author
Collaborator
Author
|
@cursor review |
Collaborator
Author
Collaborator
Author
|
@cursor review |
…e cause is The generated-document arm was checked first, so it won over both other causes and told users two things that were not true. On an inline-strategy provider — bedrock, mistral, ollama, fireworks, litellm, vertex, kimi — there is no upload path for any file, generated or not, but the message blamed the document format and implied a plain PDF would go through. On openai or google with cloud storage unconfigured it was simply false: a generated document does take the Files API path there, and that exact file uploads fine once storage exists. The one actionable fix was hidden from the operator. A provider with no upload path cannot be helped by changing the file, and a deployment with no object storage cannot reach any upload path whatever the file is, so both now outrank the format-specific case — which is left saying only what is true of it: a signed URL points at the generation source rather than the rendered file. The formatter is unchanged. It was brute-forced over every real ceiling and three million random pairs with no collision or inversion, but the test's six ceilings all divide to exact integers, so floor, round and ceil are indistinguishable on them and the limit-side rounding was unpinned. A ceiling with a fractional remainder now covers it.
waleedlatif1
force-pushed
the
fix/provider-file-upload-lifecycle
branch
from
August 4, 2026 01:31
394ee8b to
0356912
Compare
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0356912. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Execution memory limit exceeded. The inline base64 cap was 10 MB of raw bytes, but the execution payload store refuses a single value over 8 MiB and base64 inflates by 4/3 — so the base64 cache write threw and killed the run after the bytes had already been read successfully.shouldUseLargeFilePathonly fired above the inline cap, so files in that band never reached the OpenAI/Gemini Files API upload they were supposed to take.files-apicrosses over at the lower threshold (it carries every type viafile_id);remote-urlonly once inlining is genuinely impossible, since it fetches images and PDFs only. Where no upload path is reachable — an inline-only provider, or any deployment without cloud storage — base64 runs to the inline ceiling exactly as before.50_000_000. The gate issize > maxBytes, so 50 MiB admitted 52,428,800 bytes while the docs say each file must be under 50 MB.openai4.104.0 → 7.0.0,@google/genai1.34.0 → 2.13.0,@anthropic-ai/sdk0.114.0 → 0.115.0, with the Interactions v2 and tool-call-union migrations they require.Type of Change
Testing
Reproduced the original failure against unmodified code — a 9 MB hydration throws
ExecutionResourceLimitError: Execution memory limit exceeded, the exact error reported. Added an end-to-end test over the realattachLargeFileRemoteUrls→uploadLargeFilesToProvider→buildOpenAIMessageContentchain at the reported file's exact size (9,591,617 bytes), asserting thePOST /v1/filesmultipart shape and the resultinginput_file+file_id; that path previously had no test at all.Audited line by line by six parallel agents against the vendors' live docs. That pass reverted an earlier round of provider-ceiling changes that were not backwards compatible (a Bedrock cap that broke video attachments, an OpenAI combined cap that is a file-input limit being applied to image inputs, an unreachable Fireworks per-file cap) and caught a band where
remote-urlproviders had neither base64 nor a handle. It also found eight sites where the tool-call narrowing was computed and discarded, a guard that threw on a malformedtool_callsentry, and a lost short-circuit that turned a tolerated bad response body into a TypeError.Full suite green (18,239 passing; the one failure is a pre-existing missing
rgbinary that fails identically on staging),tscclean, lint clean,check:api-validationpasses.Not live-verified against real provider APIs. Deep research (Gemini Interactions) has no test coverage — pre-existing, and worth closing separately.
Checklist