Skip to content

feat(js-sdk): run the template test suite on Deno - #1595

Merged
mishushakov merged 5 commits into
mainfrom
deno-template-tests
Jul 23, 2026
Merged

feat(js-sdk): run the template test suite on Deno#1595
mishushakov merged 5 commits into
mainfrom
deno-template-tests

Conversation

@mishushakov

@mishushakov mishushakov commented Jul 23, 2026

Copy link
Copy Markdown
Member

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).

// 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 (node:fs openAsBlob should be streaming 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

Adds --project template to test:deno and fixes the runtime bug the
suite surfaced: Deno's 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. Runtimes whose blobs
carry an inferred MIME type (Bun) would send a Content-Type that breaks
the presigned signature, so typed blobs fall back to blob.stream() plus
explicit Content-Length.

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>
@cla-bot cla-bot Bot added the cla-signed label Jul 23, 2026
@cursor

cursor Bot commented Jul 23, 2026

Copy link
Copy Markdown

PR Summary

Overview
Unable to generate summary.

Reviewed by Cursor Bugbot for commit 119bdd8. Bugbot is set up for automated code reviews on this repo. Configure here.

@changeset-bot

changeset-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 119bdd8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
e2b Patch

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

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Package Artifacts

Built from 963ce58. Download artifacts from this workflow run.

JS SDK (e2b@2.35.4-deno-template-tests.0):

npm install ./e2b-2.35.4-deno-template-tests.0.tgz

CLI (@e2b/cli@2.15.1-deno-template-tests.0):

npm install ./e2b-cli-2.15.1-deno-template-tests.0.tgz

Python SDK (e2b==2.34.0+deno.template.tests):

pip install ./e2b-2.34.0+deno.template.tests-py3-none-any.whl

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97fa550dc4

ℹ️ 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".

Comment thread packages/js-sdk/src/template/buildApi.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found by me or the bug-hunting system, but I'd like a human to weigh in given the change touches the presigned-upload networking path used by real template builds. Beyond the reported findings, I checked whether the new typed-blob fallback branch (blob.stream() + explicit Content-Length, added for Bun's MIME-type inference) is exercised by any test in this diff — it isn't; uploadFile.test.ts only covers the untyped-blob (blob.type === '') path that Node/Deno take, since fs.openAsBlob returns a typeless blob on Node. The Bun branch's correctness currently rests on the PR description's account of manual production testing rather than an automated check.

Extended reasoning...

Overview

This PR extends the Deno vitest run with the template project and changes uploadFile in buildApi.ts to send the spooled tar archive as a file-backed Blob (via fs.openAsBlob) instead of a Node Readable stream with an explicit Content-Length. The reason is that Deno's fetch ignores Content-Length on stream bodies and falls back to chunked transfer encoding, which S3-compatible presigned PUT URLs reject. tarFileStream was also renamed to spoolTarArchive, returning { path, size, cleanup } with caller-owned cleanup instead of a self-deleting stream — cleaner ownership semantics, and the tests were updated accordingly.

Security risks

Low. No new user input handling, auth, or crypto — this is an internal upload path to a presigned S3-style URL. The main risk category is functional correctness: sending the wrong headers (extra Content-Type, wrong Content-Length, or chunked encoding) breaks presigned-URL signature validation and causes uploads to fail with 403/501, not a security exposure.

Level of scrutiny

This warrants more than a rubber stamp: it changes a production-critical upload code path (every template build goes through uploadFile) and introduces runtime-dependent branching (feature-detected via blob.type) whose correctness depends on subtle, differing behavior across Node, Deno, and Bun's fetch/Blob implementations. The Node and Deno paths are exercised by the updated unit test and the real test:deno run (523 passed per the PR description). However, the typed-blob fallback branch — added specifically to fix Bun's inferred-MIME-type behavior — is not covered by any test in this diff; test:bun in package.json still only runs unit/connectionConfig, not template (that's deferred to the sibling PR #1594). The PR description states the Bun 403 was "observed... against production storage" but there's no automated regression test in this PR that would catch a regression in that branch.

Other factors

The rest of the diff is low-risk: a workflow comment reword, a package.json script addition (--project template for Deno), and mechanical test-file updates to match the new spoolTarArchive API (stream-based extraction replaced with file-path-based extraction, cleanup assertions added). These are straightforward and correctly mirror the implementation change. No CODEOWNERS-restricted files are touched, and the bug-hunting system found nothing. My deferral is specifically about the untested Bun-specific branch in critical upload code, not about any confirmed defect.

Deno's native fetch ignores an explicit Content-Length header on stream
bodies and falls back to Transfer-Encoding: chunked, which S3 presigned
PUT URLs reject with 501 (#1243). A file-backed Blob body is not an
option either: Deno's openAsBlob shim reads the whole file into memory
eagerly (denoland/deno#32316), and Bun's blobs carry an inferred MIME
type that breaks presigned-URL signatures.

Upload the spooled archive through undici's fetch instead, which honors
the Content-Length header on stream bodies on every runtime, falling
back to the global fetch where undici isn't resolvable. Known caveat:
Deno's Readable.toWeb shim has no backpressure, so the archive is
buffered in memory on Deno until denoland/deno#36275 is fixed upstream.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dynamic imports guarded nothing: buildApi.ts already sits in a
module graph with top-level node:fs imports (template/utils.ts), so any
bundle including putFileStream pulls in node builtins statically either
way. The edge-compat bundle test still passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mishushakov
mishushakov enabled auto-merge (squash) July 23, 2026 17:54
mishushakov and others added 2 commits July 23, 2026 19:59
Named imports from node builtins (import { createReadStream } from
'node:fs') crash the vitest browser project at module evaluation: Vite
externalizes node:fs to a stub whose named bindings throw on access.
Default-form imports bind the stub object itself and defer the property
access to call time, which never happens in the browser — the same
reason the pre-existing top-level node: imports in template/utils.ts
were harmless.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mishushakov
mishushakov merged commit 9ee4414 into main Jul 23, 2026
46 of 48 checks passed
@mishushakov
mishushakov deleted the deno-template-tests branch July 23, 2026 18:11
mishushakov added a commit that referenced this pull request Jul 24, 2026
With frame selection now boundary-based, the fixed-depth stack walk
that Bun's tail-call frame elision used to break is gone, so the
template suite passes under Bun without runtime-specific workarounds.
Add --project template to test:bun, matching the Deno leg (#1595).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mishushakov added a commit that referenced this pull request Jul 24, 2026
## Description

Adds `--project template` to `test:bun` so the Bun CI leg runs the
template suite, matching the Deno leg (#1595).

No code changes are needed: the template suite previously failed under
Bun because Bun's JavaScriptCore elides tail-call frames and the
fixed-depth stack walk attributed build errors one frame past the user's
call site (the workaround attempt in #1596 was closed in favor of
#1599). With #1599's boundary-based frame selection (now merged), the
suite passes under Bun as-is.

The CI workflow already passes `E2B_API_KEY`/`E2B_DOMAIN` to the Bun
leg, and the matrix comment (updated in #1595) already covers Bun
re-running API-backed suites, so `package.json` is the only change.

## Testing

Full `test:bun` (unit + connectionConfig + template) green locally on
Bun 1.3.14 against the real API: 530 passed, 35 skipped, 0 failed —
including all 34 stack-trace/caller-directory tests that pin exact user
call-site line/columns, the frames Bun used to elide.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants