fix(sdk): apply the upload headers the API returns with a file upload link - #1870
Conversation
… link Azure Blob Storage requires the request header x-ms-blob-type on Put Blob, which a signed URL cannot carry, so every template build with a COPY instruction failed on Azure-backed clusters. The API now returns the headers alongside the upload URL; both SDKs send them verbatim on the PUT and keep their own Content-Length. GCS- and S3-backed clusters return no headers, so their presigned PUTs go out with the same header set as before — their signatures cover the header list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5085b81 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from e6f575b. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.49.2-feat-azure-upload-headers.0.tgzCLI ( npm install ./e2b-cli-2.19.1-feat-azure-upload-headers.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-feat-azure-upload-headers.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-feat-azure-upload-headers.0.tgzPython SDK ( pip install ./e2b-2.49.1+feat.azure.upload.headers-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.0+feat.azure.upload.headers-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.5.0+feat.azure.upload.headers-py3-none-any.whl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review of the SDK changes (generated clients, spec, tests and changesets excluded — only buildApi.ts, template/index.ts, template_{sync,async}/build_api.py, template_{sync,async}/main.py were judged).
Checked: parity (T-1, T-2), API shape / keyword-only optionals (T-3, T-3a), generated-type boundary (T-18), timeouts & signal threading (T-46), comments/docstrings (T-69).
2 violations, both in the Python SDK:
- T-2 / T-1 — sync and async
upload_filediverge. The async mirror merges the API headers under its ownContent-Length({**(headers or {}), "Content-Length": str(size)}) and the JS side has a test for exactly that, but the sync mirror passesheadersstraight through, so an API-returnedContent-Lengthwould win. Same method, same semantics is the rule; the sync variant needs the same override and a matchingtest_upload_file_keeps_its_own_content_lengthcase. - T-3a — new optional
headersis positional. It was inserted beforerequest_timeoutwithout a*, so it both re-orders an existing optional and lets callers bind it by position. Both call sites already pass it by keyword, so a bare*costs nothing here (sync + async).
Not tied to a diff line: JS uploadFile/putFileStream correctly keep Content-Length ours and thread signal (T-46) — no issues found there. file_info.headers.to_dict() if file_info.headers else None in both main.py files relies on Unset.__bool__; it's internal plumbing so T-18 doesn't strictly apply, but if not isinstance(file_info.headers, Unset) would be the explicit spelling used elsewhere in the generated client.
| response = client.put(url, content=tar_file) | ||
| # Headers the API asked for, applied as given (Azure's Put | ||
| # Blob requires x-ms-blob-type, which its SAS cannot carry). | ||
| response = client.put(url, content=tar_file, headers=headers) |
There was a problem hiding this comment.
T-2 (sync and async mirrors share the same semantics) / T-1: the async upload_file merges these headers under its own Content-Length and the JS side tests "keeps its own Content-Length when the API returns one", but here headers is passed through verbatim, so an API-returned Content-Length would override httpx's correct one. Mirror the async form:
| response = client.put(url, content=tar_file, headers=headers) | |
| size = os.fstat(tar_file.fileno()).st_size | |
| response = client.put( | |
| url, | |
| content=tar_file, | |
| headers={**(headers or {}), "Content-Length": str(size)}, | |
| ) |
(import os needed, as in the async module)
(and add the sync counterpart of the JS keeps its own Content-Length test).
There was a problem hiding this comment.
Fixed in 105242e — sync now merges API headers under its own Content-Length (os.fstat size, mirroring async) with a forcing test in both variants, and headers is keyword-only in both signatures. The Unset note applied too: explicit isinstance in both main.py loops.
| resolve_symlinks: bool, | ||
| gzip: bool, | ||
| stack_trace: Optional[TracebackType], | ||
| headers: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
T-3a (optionals are keyword-only, enforced by a bare *): a defaulted parameter without * is still positional — this both inserts a new positional before request_timeout and lets callers bind headers by position. Both call sites already pass it by keyword, so:
| headers: Optional[Dict[str, str]] = None, | |
| *, | |
| headers: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
Fixed in 105242e — sync now merges API headers under its own Content-Length (os.fstat size, mirroring async) with a forcing test in both variants, and headers is keyword-only in both signatures. The Unset note applied too: explicit isinstance in both main.py loops.
| resolve_symlinks: bool, | ||
| gzip: bool, | ||
| stack_trace: Optional[TracebackType], | ||
| headers: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
T-3a: same as the sync variant — make the new optional keyword-only.
| headers: Optional[Dict[str, str]] = None, | |
| *, | |
| headers: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
Fixed in 105242e — sync now merges API headers under its own Content-Length (os.fstat size, mirroring async) with a forcing test in both variants, and headers is keyword-only in both signatures. The Unset note applied too: explicit isinstance in both main.py loops.
|
CI attribution, for the reviewer — and the live suites raise the evidence bar on the part that matters here: The changed upload path passes end to end against the real API, in both SDKs.
|
|
Live validation of the header-merge path against a real Azure BYOC env (miso9): |
…headers keyword-only Review fixes: the sync upload_file now mirrors async — API-returned headers merge under our own Content-Length (forcing test added in both variants); headers is keyword-only in both signatures; the Unset check in main.py is the explicit isinstance spelling. Comments compressed to one line each. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… regen Mechanical: make codegen after the runtime branch merged its main (drags in the V1 template-build endpoint removal). Re-point at the merge commit before undrafting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d27b1f295
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| headers: { | ||
| ...headers, | ||
| 'Content-Length': size.toString(), |
There was a problem hiding this comment.
When the API returns this header with any casing other than exactly Content-Length (for example, content-length), the object retains both keys because JavaScript property names are case-sensitive, while Fetch header names are not. Undici combines the values (such as 1, 123), causing the streamed PUT to fail with a content-length mismatch; the added test only covers the exact-case spelling. Delete or replace API-provided Content-Length case-insensitively before setting the archive size.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfc487d — API-sent Content-Length is now stripped case-insensitively before ours is set, in JS and both Python variants (same hole); the forcing tests now pass lowercase content-length to pin the behavior.
…ting ours Review finding: fetch header names are case-insensitive but object/dict keys are not, so an API-returned lowercase content-length survived the spread and undici/httpx would send both values. Filter it out in JS and both Python variants; the keeps-its-own-Content-Length tests now use the lowercase spelling to force the path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and did not find any bugs. Because it touches build-upload networking logic across the JS and Python (sync + async) SDKs and regenerates the API client from a bumped runtime-spec pin, a human look would still be worthwhile.
What was reviewed:
- Header pass-through and Content-Length precedence in JS
putFileStreamand Python sync/asyncupload_file— API-supplied headers are merged first, thenContent-Lengthis forced, matching the PR's stated behavior. - The sync
build_api.pyfix (explicitos.fstatsize + merged headers) that appears to resolve the earlier inline review comments on that file and its async counterpart. - Did not independently re-verify the full
spec/openapi.ymlregeneration for byte-for-byte fidelity to amake codegenrun.
Extended reasoning...
Overview
This PR threads API-returned upload headers (needed for Azure Blob Storage's x-ms-blob-type requirement) through both the JS SDK (buildApi.ts, index.ts) and the Python SDK (sync and async build_api.py/main.py), and regenerates the OpenAPI client/spec from a bumped spec/runtime-ref pin, which also removes deprecated v1/v2 template endpoints and adds unrelated scheduling fields. Tests were added for header pass-through and Content-Length precedence in both languages, and two changesets are present per CLAUDE.md's requirement.
Security risks
The headers merged into the PUT request originate from the trusted first-party API response, not user input, so this isn't an injection vector in the traditional sense. The main risk class is subtler: header-name case handling and null-vs-Unset deserialization edge cases in the generated Python model, both of which were already surfaced as candidates and are not something I found new evidence to escalate beyond what was already considered.
Level of scrutiny
This warrants more than a rubber-stamp: it's a cross-language SDK change (JS + Python sync/async, properly mirrored) that alters a real network code path (the build-context upload PUT), plus a spec regeneration that alters generated client surface across several files. It's not a one-line config tweak, and prior reviewer feedback (devin-ai-integration inline comments on build_api.py) was substantive enough that the author pushed follow-up commits specifically to address it, which I confirmed by diffing before/after (the sync path now computes size via os.fstat and explicitly merges headers alongside a forced Content-Length, mirroring the async path).
Other factors
The bug-hunting run exited via dry_streak (ran until no new findings), and reported no bugs this run; the previously-flagged candidates (spec pin pointing at an unmerged branch head, header case-sensitivity in the JS/Python merge, and null-vs-Unset in the generated model) were investigated and not escalated as findings. The unmerged-branch-pin point is also already called out by the PR author themselves as a known pre-merge TODO, so repeating it adds nothing. Given the change's breadth across two SDKs and a regenerated client, but the absence of any new confirmed defect, a defer with a brief "no bugs found, human look still useful" note is the right level of signal here rather than an outright approval or silence.
This review covers commit 5d27b1f, which is no longer the latest commit on this pull request; later commits are not covered by it.
belt#3308 merged and synced out; the pin now references the contract on main (drags in the 429 declarations from the same window). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
We landed on the same fix independently in #1876 (now closed in favour of this one, which has the live Azure evidence). Three deltas from that branch, take or leave each:
|
|
End-to-end validation at this exact head (1cfbb7c) through the full production path — SDK → staging api
Both lanes exercised with the rebuilt SDK snapshot including the case-insensitive Content-Length strip. Nothing further blocks merge. |
…ders # Conflicts: # spec/runtime-ref
|
E2E re-validated at the merge commit (8f4c744 — SDK rebuilt from it, fresh install verified to carry the case-insensitive Content-Length strip), 2026-09-15 06:11–06:12 UTC against miso9 through staging api + edge:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
The API now returns request headers with a file-upload link (e2b-dev/belt#3308, moved from e2b-dev/runtime#3634 — Azure
Put Blobneedsx-ms-blob-type, which its SAS cannot carry); both SDKs apply them on the upload PUT. Header-less providers (S3/GCS/fs) get byte-identical requests, so nothing changes off Azure.getFileUploadLinkreturnsheaders;putFileStreammerges them under our ownContent-Length, stripping any API-sent Content-Length case-insensitively.upload_filetakes keyword-onlyheaders, merged the same way (sync mirrored to async per review).MissingRequiredHeader.spec/runtime-refpins runtimemain(756512ca8), which carries the merged contract (e2b-dev/belt#3308).Changesets: one patch per SDK.
Sponsor: @michalsuba-e2b
🤖 Generated with Claude Code