top-level await

[whoiskatrin]

top level await, i played a lot with just wrapping all code and tbh, i wasn't happy with it, felt hacky

so now we parse code with acord, then we check if last AST node is an ExpressionStatement, if the answer is yes: wrap code with return on last expression, if no: wrap code without return (returns undefined)

[changeset-bot]

🦋 Changeset detected

Latest commit: c81afe9

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

This PR includes changesets to release 1 package

Name	Type
@cloudflare/sandbox	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

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@261

commit: c81afe9

[github-actions]

🐳 Docker Images Published

Default (no Python):

FROM cloudflare/sandbox:0.0.0-pr-261-9476f46

With Python:

FROM cloudflare/sandbox:0.0.0-pr-261-9476f46-python

Version: 0.0.0-pr-261-9476f46

Use the -python variant if you need Python code execution.

[claude]

Claude Code Review

The type safety and architecture issues from previous reviews are resolved. Implementation is solid with excellent test coverage.

Critical blocker: Missing changeset file. This adds a new feature (top-level await) but has no changeset. Create .changeset/top-level-await.md:

---
'@cloudflare/sandbox': minor
---

Add top-level await support for JavaScript code execution

JavaScript code can now use await at the top level without wrapping in async IIFE.

Use minor (not patch) - this is a new feature, not a bug fix.

Previous feedback status:

• Silent catch block (code-transformer.ts:39) - acceptable as-is. Node executor is a standalone process without logger infrastructure, and the fallback behavior is correct
• Documentation could be enhanced but current comments are adequate
• Test coverage is strong, though adding arrow function/generator tests would be nice

Code quality:

• AST-based approach using acorn is much more robust than regex wrapping
• String slicing with AST positions is safe (parses trimmed string, slices same string)
• Semicolon regex /;$/ is safe (only matches end-of-string)
• No security issues identified
• Clean separation: transformer → executor → VM

Verdict: Excellent implementation. Add the changeset file and this is ready to merge.

[ghostwriternr]

Thanks a lot for this change! Overall, this approach makes sense but I think our solution is still incomplete/incorrect (mainly based on the e2e test change) - I wonder if we should just duplicate the logic Node's REPL uses (repl/await.js) with all the additions/logic they handle but cleanly extracted out as a util like you. This logic is not very frequently updated anyway and seems to be super stable.

[claude]

Claude Code Review

RECOMMENDATION: REQUEST CHANGES - BLOCKING ISSUE IDENTIFIED

Critical: Variable Persistence Regression

The IIFE wrapping pattern breaks variable persistence across code executions. Each execution creates a new function scope, so variables declared with const/let/var don't persist between calls.

Evidence: Six E2E tests were modified to use globalThis instead of normal variable declarations (lines 750, 820, 846, 895, 907, 951). This proves the implementation broke existing functionality.

Real-world failure:

// Execution 1
const data = await fetch('https://api.example.com').then(r => r.json());

// Execution 2
console.log(data);  // ❌ ReferenceError: data is not defined

This happens because each execution wraps code in (async () => { ... })(), creating isolated function scope. The persistent vm context is still there, but variables are scoped to each IIFE.

Why this matters: LLMs generate idiomatic JavaScript using const/let/var, not globalThis assignments. Multi-step workflows (fetch → process → display) will fail.

Required fixes:

1. Fix variable persistence without requiring globalThis
2. Add E2E tests verifying persistence across separate execute() calls
3. Revert the globalThis test modifications

Possible solutions: conditional wrapping (only when await present), variable hoisting to global scope, or AsyncFunction constructor instead of IIFE.

Important: Timeout Only Applies to Sync Phase

The timeout option (line 85) only covers vm.runInContext(), not the promise resolution. The IIFE returns instantly, so timeout can't catch infinite async loops:

// This times out (sync)
while(true) {}

// This does NOT timeout (async)
await new Promise(resolve => {
  setInterval(() => {}, 1000);  // Never resolves
});

Recommend adding timeout wrapper for promise resolution or documenting this limitation.

Minor: Parse Error Line Numbers

When acorn parsing fails (line 39), code is wrapped anyway and runtime errors report wrong line numbers (off by 1 due to IIFE wrapper).

What Works Well

• ✅ Type safety: @types/acorn properly added, no implicit any
• ✅ AST-based transformation more robust than regex
• ✅ Comprehensive unit tests (162 lines) cover edge cases
• ✅ Clean code organization following established patterns
• ✅ Proper error handling with try-catch
• ✅ Thenable type guard correctly implemented

Test Coverage Gap

All 4 new E2E tests verify single-execution behavior only. None test variable persistence across multiple executions - the primary use case for code interpreters.

Missing scenarios from reviewer request:

1. Variables defined with await persisting across executions
2. Promise resolution without await keyword
3. Multi-step async workflows

The variable persistence bug is blocking. The implementation needs a different approach that supports both top-level await AND variable persistence without non-idiomatic workarounds.

[claude]

Claude Code Review

The variable hoisting implementation is correct - script-level let declarations in vm.runInContext() do persist across executions as intended. The timeout handling now properly covers both sync and async execution paths.

Critical issue remaining: Anonymous function/class declarations will crash at runtime (code-transformer.ts:136, 154).

To fix before merge:

1. Add null checks for decl.id in processFunctionDeclaration and processClassDeclaration
2. Handle edge case of declarations without initializers creating empty body (code-transformer.ts:60)
3. Add E2E test for the new persistence feature (currently only transformation is tested)

Nice to have:

• Validate AST nodes have required position properties before slicing (defensive coding)
• Consider using var instead of let for hoisted variables to avoid TDZ edge cases

Overall: Good implementation of a complex feature with comprehensive unit tests. The architecture is sound once the crash bugs are fixed.

[claude]

Claude Code Review

The implementation is solid. All critical issues from the previous review have been addressed:

• ✅ Anonymous function/class null checks are present (lines 136, 158)
• ✅ Empty body handling for let x; works correctly
• ✅ vm.Context integration tests added (lines 299-378)
• ✅ Timeout now applies to both sync and async execution
• ✅ Type safety excellent - @types/acorn properly used

The hoisting pattern correctly enables REPL-style variable persistence. AST parsing with acorn is the right choice for this use case.

One minor improvement:

Type the catch clause at code-transformer.ts:307 for consistency:

} catch (error: unknown) {
  // Parse error - defer to V8 for better error messages
  return `(async () => {\n${trimmed}\n})()`;
}

This is non-blocking. The PR is ready to merge.

issues have been fixed