feat(js-sdk): run the template test suite on Bun and Deno - #1594
feat(js-sdk): run the template test suite on Bun and Deno#1594mishushakov wants to merge 1 commit into
Conversation
Adds --project template to test:bun and test:deno, and fixes the two runtime bugs the suite surfaced: - Bun: JavaScriptCore elides tail-call frames (including the `const r = f(); return r` form), so every template method that did `return this.runInNewStackTraceContext(...)` lost its own frame and the fixed-depth stack walk attributed build errors one frame past the user's call site. Methods now capture the caller frame in their own body and pass it down via runInStackTraceOverrideContext (the pattern copy()/copyItems() already used); runInNewStackTraceContext and the enable/disable toggle are gone. - Deno: fetch ignores an explicit Content-Length header on stream bodies and falls back to Transfer-Encoding: chunked, which S3-compatible presigned PUT URLs reject with 501 (the #1243 failure mode). Uploads now send the spooled archive as a file-backed Blob (openAsBlob), which every runtime sizes itself. Bun infers an unstrippable MIME type for such blobs and would send Content-Type, breaking the presigned signature (403) — typed blobs fall back to blob.stream() plus explicit Content-Length, which Bun honors. tarFileStream is now spoolTarArchive, returning the archive path, size, and a cleanup callback owned by the caller instead of a self-deleting read stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Under Bun, template builder helpers that delegated to Template context uploads no longer stream the tar with Reviewed by Cursor Bugbot for commit aaffc36. Bugbot is set up for automated code reviews on this repo. Configure here. |
🦋 Changeset detectedLatest commit: aaffc36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaffc36623
ℹ️ 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".
| body: blob.stream(), | ||
| headers: { 'Content-Length': blob.size.toString() }, |
There was a problem hiding this comment.
Avoid chunked uploads on Bun's stream branch
When this branch is taken for Bun's typed file-backed blobs, fetch still sends a Web ReadableStream request body with Transfer-Encoding: chunked instead of the supplied Content-Length (verified with the local Bun 1.2.14 runtime; Bun's docs also describe stream bodies as streamed directly). Since the same comment says presigned uploads reject chunked bodies, Bun template builds still fail to upload archives, and the newly expanded test:bun template suite will hit this path for context.tar.gz.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No bugs found, but this PR is worth a human look given the complexity of the two runtime-specific fixes it introduces (JSC tail-call frame elision affecting 12 builder methods' stack traces, and Bun/Deno blob-type feature detection in the upload path).
I reviewed the stack-trace capture change across all twelve runInNewStackTraceContext → runInStackTraceOverrideContext conversions (remove, rename, makeDir, makeSymlink, pipInstall, npmInstall, bunInstall, aptInstall, addMcpServer, gitClone, betaDevContainerPrebuild, betaSetDevContainerStart) — no leftover references to the removed helpers remain, and the pattern matches the pre-existing copy()/copyItems() implementation.
The uploadFile rewrite (file-backed Blob with a Bun-specific stream fallback keyed on blob.type) and the spoolTarArchive caller-owned cleanup are both covered by updated unit tests, and cleanup is now called unconditionally in a finally block.
Extended reasoning...
The PR extends the js-sdk Bun/Deno vitest runs to include the template project and fixes two runtime-specific bugs it surfaced: (1) under Bun, JavaScriptCore elides tail-call stack frames, so twelve builder methods that delegated to runCmd via a fixed-depth stack walk pointed build errors at the wrong frame — fixed by capturing the caller frame in the public method body and threading it through an override context; (2) Deno ignores Content-Length on streamed fetch bodies and falls back to chunked transfer encoding, which presigned S3 PUT URLs reject — fixed by sending a file-backed Blob so every runtime derives Content-Length from it, with a Bun-specific fallback since Bun infers a MIME type from the .tar.gz extension that would break the presigned signature.
No security-sensitive code (auth/crypto/permissions) is touched, and the upload URL is server-issued (presigned), so there is no new injection or auth-bypass surface. The main risk is correctness/reliability of the build/upload path across three JS runtimes, not exploitability.
Given that this rewires stack-trace bookkeeping across a dozen methods and the file-upload transport logic used by every template build, and relies on subtle, hard-to-independently-verify claims about JSC tail-call elision and Bun/Deno Blob MIME behavior, I think this warrants a human pass even though the bug-hunting system found nothing and the PR author reports real integration test runs (522/523 passing) against Bun and Deno in the description. Test coverage is decent (unit tests updated for both fixes, and the changeset documents the rationale), which is why I did not raise blocking concerns of my own — just flagging that the underlying reasoning is subtle enough to be worth a second set of eyes.
|
Split into #1595 (Deno: template suite + uploadFile Content-Length fix) and the Bun PR above (template suite + stack-trace fix) — the two changes are independent against main. |
|
Bun counterpart: #1596 |
## What Extends the Deno vitest run (#1585) with the `template` project and fixes the real runtime bug the suite surfaced. Split out of #1594 (Bun counterpart: #1596). ```jsonc // packages/js-sdk/package.json "test:deno": "deno run -A npm:vitest run --project unit --project connectionConfig --project template", ``` ## Bug — Deno: template uploads used chunked transfer encoding Deno's native `fetch` ignores an explicit `Content-Length` header on stream bodies and falls back to `Transfer-Encoding: chunked` — exactly the failure #1243 fixed for Node, since S3-compatible presigned PUT URLs reject chunked uploads with 501. `uploadFile` now streams the spooled archive through **undici's `fetch`** (via the existing `loadUndici()` helper — undici 8 where it imports, undici 7 on Bun, global `fetch` where undici isn't resolvable, e.g. bundled apps), which honors the `Content-Length` header on stream bodies on every runtime. One upload path, no runtime sniffing. Approaches rejected along the way, all verified empirically with 1GB uploads + RSS sampling: - **File-backed `Blob` body (`fs.openAsBlob`)** — lazy on Node/Bun, but Deno's shim reads the whole file into memory eagerly (denoland/deno#32316), and Bun infers an unstrippable MIME type from the extension whose `Content-Type` breaks presigned signatures (403 against production storage). - **`node:http(s)` on Deno** — works (and is memory-bounded), but can't be unified: Bun's `node:http` ignores abort signals, and it's a second code path. Known caveat: Deno's `Readable.toWeb` shim has no backpressure, so the archive is buffered in memory during upload on Deno (Node and Bun stream in lockstep with the socket). Filed upstream as denoland/deno#36275 — accepted as Deno's to fix rather than worked around here. As part of this, `tarFileStream` became `spoolTarArchive`, returning `{ path, size, cleanup }` with caller-owned cleanup instead of a self-deleting read stream. `tests/template/uploadFile.test.ts` also asserts no `Content-Type` header is sent. ## Python SDK parity Intentionally none: `upload_file` already sends a sized file body via httpx. ## Testing - Real template builds (`tests/template/build.test.ts`, against prod S3 presigned URLs) green under **Node, Deno, and Bun** - `uploadFile` + `spoolTarArchive` suites green under Node, Deno, and Bun - `tests/template/abortSignal.test.ts` green under Deno - `pnpm build`, `lint`, `typecheck`, `prettier --check` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Extends the Bun (#1584) and Deno (#1585) vitest runs with the
templateproject, and fixes the two real runtime bugs the suite surfaced. Full matrix now passes: Node 131 ✓, Bun 522 ✓, Deno 523 ✓ (remainder skipped).Bug 1 — Bun: build-error stack traces pointed at the wrong frame
Bun's JavaScriptCore performs proper tail calls in strict mode — and its bytecode generator also treats
const r = f(); return ras a tail call — so the frame of any method that returns a call result directly disappears fromnew Error().stack. Every builder method implemented asreturn this.runInNewStackTraceContext(() => this.runCmd(...))(remove,rename,makeDir,makeSymlink,pipInstall,npmInstall,bunInstall,aptInstall,addMcpServer,gitClone,betaDevContainerPrebuild,betaSetDevContainerStart) lost its own frame, and the fixed-depth walk landed one frame past the user's call site — under Bun, a failed build step's error pointed at test-runner internals instead of the user's.remove(...)line.Fixed by capturing
getCallerFrame(STACK_TRACE_DEPTH - 1)in the public method's own body (frames are still live there) and passing it down viarunInStackTraceOverrideContext— the patterncopy()/copyItems()already used, whose traces were correct under Bun all along.runInNewStackTraceContextand the enable/disable toggle are deleted.Bug 2 — Deno: template uploads used chunked transfer encoding
Deno's
fetchignores an explicitContent-Lengthheader on stream bodies and falls back toTransfer-Encoding: chunked— exactly the failure #1243 fixed for Node, since S3-compatible presigned PUT URLs reject chunked uploads with 501.uploadFilenow sends the spooled archive as a file-backedBlob(fs.openAsBlob, lazily read, so memory stays bounded); every runtime derivesContent-Lengthfrom the blob itself. One wrinkle: Bun infers a MIME type from the file extension (unstrippable — it survivesslice()and re-wrapping) and would send aContent-Typethe presigned signature doesn't cover (observed 403 Forbidden against production storage). Typed blobs therefore fall back toblob.stream()+ explicitContent-Length, which Bun honors. The choice is feature-detected viablob.type, not runtime-sniffed.tests/template/uploadFile.test.tsnow also asserts noContent-Typeheader is sent.As part of this,
tarFileStreambecamespoolTarArchive, returning{ path, size, cleanup }with caller-owned cleanup instead of a self-deleting read stream.Python SDK parity
Intentionally none: CPython never elides frames, so
_run_in_new_stack_trace_contextproduces correct traces, andupload_filealready sends a sized file body via httpx.Testing
vitest run --project template— Node 131 passed / 3 skippedpnpm test:bun— 522 passed / 35 skipped (real template builds + uploads against prod)pnpm test:deno— 523 passed / 34 skipped (real template builds + uploads against prod)pnpm build,lint,typecheck,prettier --checkclean🤖 Generated with Claude Code