Conversation
…Babel transform support
… levels, quickstart, and worker pool
📝 WalkthroughWalkthroughAdds sandboxed Babel single- and multi-file transforms with import-rewrite, extends ast-guard exports, wires Babel into the Enclave runtime and types, adds extensive tests and benchmarks, reorganizes docs under an enclave/ namespace, and replaces Codex-driven CI with deterministic Nx-based release/publish workflows and a version script. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Enclave as Enclave VM
participant Babel as Restricted Babel Sandbox
participant AST as ast-guard (Import Rewriter)
participant CDN as CDN Builder
Client->>Enclave: request transform (files/options, preset=babel)
Enclave->>Babel: createRestrictedBabel() -> transform or transformMultiple(files, opts)
note right of Babel: run transform(s) inside isolated VM\nenforce time/size limits, return code
Babel-->>Enclave: transformed code per file
Enclave->>AST: rewriteImports(transformed code, config) [optional]
AST->>CDN: validate/build CDN URLs
CDN-->>AST: CDN URLs
AST-->>Enclave: rewritten code + metadata (rewrittenImports)
Enclave-->>Client: final transformed files, dependencies, warnings, rewrittenImports
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/enclave/api-reference/enclavejs-runtime.mdx (1)
2-6: Verify related links use the new enclave/ namespace.Since the title was updated for the enclave/ reorg, please confirm the “Related” links at the bottom now point to the enclave‑prefixed paths to avoid broken navigation.
libs/ast-guard/src/index.ts (1)
100-143: Import rewrite APIs need documentation. The Babel preset exports (createBabelPreset,getBabelConfig,BABEL_SECURITY_CONFIGS) are documented indocs/enclave/core-libraries/ast-guard/babel-preset.mdx. However, the import rewrite APIs (rewriteImports,isValidPackageName,isValidSubpath,ImportRewriteConfig,ImportRewriteResult) lack documentation. Add an entry indocs/enclave/core-libraries/ast-guard/explaining their purpose, usage, and configuration options.
🤖 Fix all issues with AI agents
In @.github/workflows/create-release-branch.yml:
- Around line 119-152: Add a fail-fast guard after computing PROJECTS: if the
environment variable/step output PROJECTS (from steps.projects.outputs.projects)
is empty or whitespace, print an error and exit non‑zero before calling npx nx
release version; this prevents calling npx nx release version with --projects=""
(which matches no projects) and ensures the workflow aborts early when no
publishable projects were discovered.
In @.github/workflows/publish-release.yml:
- Around line 184-252: The workflow step id "ai_changelog" currently hardcodes
the deprecated model string 'gpt-4-turbo-preview' inside the script's fetch
body; update this to use a current default model (e.g., 'gpt-4.1' or 'gpt-5')
and make the model configurable via an env/input (add e.g., OPENAI_MODEL or
inputs.openai_model) so the script reads process.env.OPENAI_MODEL or a provided
input instead of the literal 'gpt-4-turbo-preview', and update any
documentation/comments referencing the old model; ensure the fetch body uses
that variable for the "model" field and the workflow sets a sensible default for
OPENAI_MODEL.
- Around line 264-274: The "Update package versions" step should be skipped when
there are no matched projects just like downstream steps; modify the step guard
for the step named "Update package versions" to include a check that
steps.projects.outputs.projects is non-empty in addition to inputs.dry_run !=
true so the npx nx release version invocation (using VERSION and PROJECTS) only
runs when PROJECTS contains a value.
In `@docs/enclave/api-reference/enclave-vm.mdx`:
- Around line 2-6: Update the "Related" links in the docs page for the
enclave-vm package so they use the new enclave‑prefixed paths; open the
enclave-vm MDX (title: 'enclave-vm') and replace any old links in the "Related"
section with the new enclave/... routes (e.g., change references like /vm or
/other to /enclave-vm or /enclave-... as appropriate) ensuring all related items
point to the enclave/ namespace and run the link checker to verify no stale
links remain.
In `@docs/enclave/api-reference/enclavejs-stream.mdx`:
- Around line 1-7: Update the "Related" links at the bottom of the MDX so they
point to the new enclave-prefixed paths (e.g., change any links like /stream/...
or ../stream to /enclave/stream or the corresponding enclave/* path), ensuring
all hrefs and any internal reference identifiers in this file's "Related"
section use the enclave/ namespace; search for the "Related" heading or link
list in this document and replace non-enclave paths with the new
enclave-prefixed routes consistently.
In `@libs/ast-guard/src/transforms/import-rewrite.transform.ts`:
- Around line 242-293: The import rewrite currently only handles
ImportDeclaration nodes, so re-exports (ExportNamedDeclaration and
ExportAllDeclaration) that include a source are not validated or rewritten;
extend the AST walk where it checks node.type to also detect
ExportNamedDeclaration and ExportAllDeclaration, and for nodes with a non-null
source run the same flow you use for ImportDeclaration: use
parseImportSource(source), check allowedPackages.has(packageName), call
validatePackageName(packageName) and validateSubpath(subpath) if present,
resolve version from config.packageVersions, build cdnUrl the same way, then
update (node.source as Literal).value and .raw, and push the mapping into
rewrittenImports (and respect skipLocalImports/skippedImports and isLocalImport
logic). Ensure you reference the same variables/functions (parseImportSource,
validatePackageName, validateSubpath, allowedPackages, config, skippedImports,
rewrittenImports, isLocalImport) so behavior matches imports exactly.
- Around line 228-237: The docstring claims TypeScript support but the code uses
acorn.parse (no TS plugins) which will fail on TypeScript syntax; update the
transform to use a TypeScript-aware parser or change the docstring. To keep
TS/JSX support, replace acorn.parse usage (the call that produces ast: Program
from code) with a parser that supports TypeScript/JSX (e.g., `@babel/parser` with
the "typescript" and optional "jsx" plugins) and ensure downstream code that
expects an ESTree Program still works (or adapt to `@babel/parser` output), or
alternatively change the function docstring to explicitly state "JavaScript
only" and keep acorn.parse as-is. Reference the acorn.parse call and the ast
variable in import-rewrite.transform.ts when making the change.
In `@libs/enclave-vm/src/__tests__/babel-examples.spec.ts`:
- Around line 141-170: Tests assume classic JSX output but the preset isn't
explicit; update the JSX preset to force classic runtime so babel.transform(...)
yields React.createElement consistently. In the test calls that use
babel.transform and the shared wrapper config, change the react preset entry
from 'react' to ['react', { runtime: 'classic' }] (e.g., presets: ['typescript',
['react', { runtime: 'classic' }], filename: ...]) or alternatively broaden the
assertions around BABEL_EXAMPLES to accept either classic output
(React.createElement) or the automatic runtime helpers (e.g., _jsx/_jsxs/jsxDEV)
by checking for either pattern.
In `@libs/enclave-vm/src/babel/multi-file-transform.ts`:
- Around line 101-135: The filename validator rejects .mts/.cts even though
EXTENSION_MAP includes them; update VALID_FILENAME_REGEX (or generate it
dynamically) to allow .mts and .cts extensions (or all keys from EXTENSION_MAP)
so validateFilename accepts those module extensions; change the regex pattern
used by VALID_FILENAME_REGEX or build it from Object.keys(EXTENSION_MAP) and
ensure validateFilename uses that updated pattern (symbols:
VALID_FILENAME_REGEX, EXTENSION_MAP, validateFilename, transformMultiple).
- Around line 154-176: The current extractImports function uses regex
(ReDoS-prone) and only captures static import statements, missing export ...
from, export * from, and dynamic import() sources; replace the regex approach
with an AST-based solution (e.g., `@babel/parser` to parse the code into an AST
and `@babel/traverse` or acorn + walk) to reliably collect module specifiers from
nodes: ImportDeclaration, ExportNamedDeclaration/ExportAllDeclaration (when
node.source exists), and dynamic CallExpression nodes where callee.type ===
'Import' (and literal string argument), and likewise update any corresponding
path-rewriting logic elsewhere in this file to use the same AST traversal so all
specifiers are found and rewritten safely without regex.
In `@libs/enclave-vm/src/types.ts`:
- Around line 39-44: Update the public API documentation to include the new
'babel' AstPreset: add 'babel' to the preset list in the CreateEnclaveOptions
JSDoc (look for the CreateEnclaveOptions type/interface and its JSDoc comment in
libs/enclave-vm/src/types.ts) and update the corresponding consumer-facing docs
under docs/draft/docs/** to reflect the new option (do not edit docs/docs/**
directly), ensuring the JSDoc and draft docs both list 'babel' alongside
'agentscript','strict','secure','standard','permissive'.
In `@package.json`:
- Around line 17-18: The "docs" npm script in package.json calls "mint dev" but
the Mintlify CLI isn't declared as a project dependency, so add `@mintlify/cli` to
devDependencies and update the "docs" script to use the local CLI (or switch the
script to use the npx wrapper); specifically, add "@mintlify/cli" to
devDependencies and change the "docs" script (the "docs" entry in package.json)
to either "npx mintlify dev --port 4300" or the equivalent local invocation so
developers without a global mint install won't get a command-not-found error.
In `@scripts/compute-next-patch.mjs`:
- Around line 52-63: The code currently swallows all errors from the execSync
call that computes git tags; instead let git failures surface so the script
fails fast. Remove the surrounding try/catch around the execSync(`git tag --list
"v${releaseLine}.*" ...`) invocation, call execSync directly to get output, then
set tags = output.trim().split('\n').filter(Boolean) and if output is empty
result in tags = []; do not catch and ignore exceptions from execSync so
permission/missing-git/non-git errors propagate.
🧹 Nitpick comments (5)
libs/enclave-vm/src/__tests__/perf/babel.perf.spec.ts (1)
475-522: Minor edge case:lastIndexOfcould return -1 if base template is modified.The
lastIndexOf('</main>')on line 520 assumes the base template always contains</main>. If the base template is ever modified to not include this tag,insertPointwould be -1, causingslice(0, -1)to exclude the last character unexpectedly.This is unlikely to cause issues given the current hardcoded base template, but adding a guard would improve robustness.
💡 Optional defensive check
// Insert additional elements before closing main const insertPoint = base.lastIndexOf('</main>'); + if (insertPoint === -1) { + return base + additionalElements.join('\n'); + } return base.slice(0, insertPoint) + additionalElements.join('\n') + base.slice(insertPoint);libs/enclave-vm/src/__tests__/babel-preset-comprehensive.spec.ts (1)
732-748: Move timing assertion to perf test location.
This duration check is performance‑oriented; consider relocating it underlibs/enclave-vm/src/__tests__/perf/**/*.perf.spec.tsto match conventions.As per coding guidelines, "
**/src/__tests__/**/*.spec.ts: Place test files undersrc/__tests__/**/*.spec.tswith performance tests underlibs/enclave-vm/src/__tests__/perf/**/*.perf.spec.ts."libs/ast-guard/src/presets/babel.preset.ts (1)
169-171: Avoid exposing mutable config references.
Returning the shared object lets callers mutate global defaults; consider returning a shallow copy (includingallowedPresets) instead. Please confirm whether callers are expected to mutate the returned config.♻️ Suggested change
export function getBabelConfig(securityLevel: SecurityLevel = 'STANDARD'): BabelSecurityConfig { - return BABEL_SECURITY_CONFIGS[securityLevel]; + const config = BABEL_SECURITY_CONFIGS[securityLevel]; + return { ...config, allowedPresets: [...config.allowedPresets] }; }libs/enclave-vm/src/__tests__/multi-file-babel.spec.ts (1)
299-341: Ensure Enclave.dispose always runs (even on failures).If an assertion throws,
dispose()won’t execute, which can leak VM resources and make the test runner hang. Consider wrapping the body intry/finally(apply to all Enclave tests).♻️ Suggested pattern
- const result = await enclave.run<{ - fileCount: number; - hasTheme: boolean; - hasApp: boolean; - warnings: string[]; - }>(code); - - expect(result.success).toBe(true); - expect(result.value?.fileCount).toBe(2); - expect(result.value?.hasTheme).toBe(true); - expect(result.value?.hasApp).toBe(true); - - enclave.dispose(); + try { + const result = await enclave.run<{ + fileCount: number; + hasTheme: boolean; + hasApp: boolean; + warnings: string[]; + }>(code); + + expect(result.success).toBe(true); + expect(result.value?.fileCount).toBe(2); + expect(result.value?.hasTheme).toBe(true); + expect(result.value?.hasApp).toBe(true); + } finally { + enclave.dispose(); + }libs/enclave-vm/src/babel/babel-wrapper.ts (1)
264-288: Consider loading Babel inside the VM context for defense-in-depth, though current approach is validated secure.While
__babel_transform__is a host-bound function that executes in the host realm (a valid technical concern),@babel/standaloneis pure JavaScript with no access to Node.js APIs. The isolated context explicitly blocks all dangerous globals (process,require,fs, etc.), and comprehensive security testing (1255 tests, 100% pass rate including sandbox escape prevention) confirms no exploitation paths. For additional defense-in-depth, consider evaluating whether to load the Babel standalone bundle inside the VM context itself rather than binding a host function, though this may have performance implications.
…er handling of exports
…er handling of exports
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/enclave-vm/package.json (1)
39-49: Upgrade@babel/standaloneto 7.26.10+ due to security vulnerability CVE-2025-27789.
@babel/standalone@7.26.0is affected by a moderate ReDoS vulnerability (GHSA-968p-4wvh-cqc8 / CVE-2025-27789) in the generated.replace()polyfill when transpiling RegExp named capturing groups. Upgrade to@babel/standalone@7.26.10or later. Note: recompiling output is required to remove the vulnerable generated code; upgrading the package alone is insufficient.Also:
vectoriadb@2.1.3does not appear in public npm registry (latest public version is 2.0.2). Clarify if this version exists in a private registry or if the version constraint should be adjusted.
🤖 Fix all issues with AI agents
In @.github/workflows/publish-release.yml:
- Around line 306-389: The tag/release steps run even when no publishable
projects exist—guard "Create and push git tag" and "Create GitHub Release" (and
optionally "Prepare release body") by checking steps.projects.outputs.projects
so they only run when non-empty; update their if conditions to combine the
existing dry_run check with a non-empty projects check (e.g., use the expression
involving inputs.dry_run and steps.projects.outputs.projects) so tagging and
release creation is skipped when no projects are publishable.
- Around line 184-263: The ai_changelog step currently throws on any OpenAI
failure and can block releases; wrap the chat/completion logic in a try/catch
and early-skip when env.OPENAI_API_KEY is missing so failures degrade to a
warning. Concretely, in the actions/github-script script around the
fetch/response parsing and JSON.parse(result) block (the code that calls fetch,
reads data.choices[0].message.content and calls core.setOutput), add a check if
(!process.env.OPENAI_API_KEY) { core.warning('OPENAI_API_KEY missing; skipping
AI changelog'); core.setOutput('changelog','');
core.setOutput('has_card_mdx','false'); return; } and wrap the
fetch/JSON.parse/writeFileSync/core.setOutput sequence in try { ... } catch
(err) { core.warning(`AI changelog skipped: ${err.message}`);
core.setOutput('changelog',''); core.setOutput('has_card_mdx','false'); } so the
step logs a warning and sets empty outputs instead of throwing (optionally also
set the workflow step to continue-on-error: true for extra resilience).
In `@libs/enclave-vm/src/__tests__/babel-preset-comprehensive.spec.ts`:
- Around line 468-557: The test "should support complete UI component
transformation workflow" creates an Enclave instance (new Enclave({...})) but
calls enclave.dispose() after assertions, so failures leak resources; modify the
test to create the Enclave, then run the enclave.run(...) and all assertions
inside a try block and call enclave.dispose() inside a finally block to
guarantee cleanup, and apply the same try/finally pattern to other tests in this
file that instantiate Enclave (look for new Enclave(...), enclave.run, and
enclave.dispose references).
In `@libs/enclave-vm/src/__tests__/perf/babel.perf.spec.ts`:
- Around line 22-89: The perf assertions in the 'Babel Transform Performance'
suite (the describe block and the it.each test that uses COMPLEXITY_LEVELS,
getLatencyThreshold, recordMetrics and the expect checks for p50/p95) must be
gated so they don't run on shared CI by default; add an opt-in flag (e.g.
process.env.RUN_PERF_TESTS) and skip or mark the whole suite/test as skipped
unless the flag is set, or alternatively move this file to a perf-only job.
Modify the top-level describe or the it.each invocation to check the env flag
and call describe.skip / it.skip (or return early) when the flag is not present,
ensuring all uses of babel.transform, recordMetrics and the latency asserts are
only executed when the opt-in flag is enabled.
In `@libs/enclave-vm/src/babel/multi-file-transform.ts`:
- Around line 220-233: resolveLocalImport and updateLocalImportPaths are
inconsistent: resolveLocalImport's extensions list omits .mts/.cts and
updateLocalImportPaths blindly appends .js for extensionless local imports,
breaking .mjs/.cjs outputs. Fix by including .mts and .cts in the extensions
array (and indexCandidate checks) used in resolveLocalImport (look for
variable/extensions loop around relativePath and availableFiles) and change
updateLocalImportPaths to derive the appended extension from the same
input→output mapping used elsewhere (e.g., reuse the existing function or
mapping that maps source extensions to output extensions) so both resolution and
emitted import paths use the same .js/.mjs/.cjs chosen for the source file.
🧹 Nitpick comments (3)
.github/workflows/publish-release.yml (1)
347-354: Resolve package roots via Nx metadata instead of assuminglibs/<project>.Line 347-353 assumes publishable libs live under
libs/, which can drift from actual Nx project roots. This can silently drop packages from the release body. Prefer resolving each project’s root via Nx and reading itspackage.jsonthere.♻️ Suggested refactor
- for lib in "${LIBS[@]}"; do - # Get npm package name from package.json - if [ -f "libs/$lib/package.json" ]; then - NPM_NAME=$(node -p "require('./libs/$lib/package.json').name") - echo "- [\`${NPM_NAME}@${VERSION}\`](https://www.npmjs.com/package/${NPM_NAME}/v/${VERSION})" >> /tmp/release-body.md - fi - done + for lib in "${LIBS[@]}"; do + ROOT=$(node -e "const { execSync } = require('child_process'); const p = JSON.parse(execSync('npx nx show project ${lib} --json', { encoding: 'utf8' })); process.stdout.write(p.root || '')") + if [ -n "$ROOT" ] && [ -f "$ROOT/package.json" ]; then + NPM_NAME=$(node -p "require('./${ROOT}/package.json').name") + echo "- [\`${NPM_NAME}@${VERSION}\`](https://www.npmjs.com/package/${NPM_NAME}/v/${VERSION})" >> /tmp/release-body.md + fi + donelibs/enclave-vm/src/__tests__/babel-preset-comprehensive.spec.ts (1)
8-11: Consolidate imports from the same module.Multiple import statements from
'../babel'can be merged into a single statement for clarity.♻️ Suggested refactor
import { Enclave } from '../enclave'; -import { transformMultiple, type MultiFileInput, type MultiFileLimits } from '../babel'; -import { createRestrictedBabel, type BabelWrapperConfig, resetBabelContext } from '../babel'; +import { + transformMultiple, + createRestrictedBabel, + resetBabelContext, + type MultiFileInput, + type MultiFileLimits, + type BabelWrapperConfig, +} from '../babel'; import { rewriteImports, type ImportRewriteConfig } from 'ast-guard';libs/enclave-vm/src/__tests__/multi-file-babel.spec.ts (1)
7-11: Consolidate imports from the same module.Three separate import statements from
'../babel'can be merged into a single statement for clarity.♻️ Suggested refactor
import { Enclave } from '../enclave'; -import { transformMultiple, type MultiFileInput, type MultiFileLimits } from '../babel'; -import { createRestrictedBabel, type BabelWrapperConfig } from '../babel'; -import { resetBabelContext } from '../babel'; +import { + transformMultiple, + createRestrictedBabel, + resetBabelContext, + type MultiFileInput, + type MultiFileLimits, + type BabelWrapperConfig, +} from '../babel';
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/publish-release.yml:
- Around line 89-98: The current PROJECTS assignment wrapper silences real Nx
failures by catching all errors in the node -e script; remove the try-catch so
errors from the 'npx nx show projects -p tag:scope:publishable --type lib
--json' execSync call propagate and cause the workflow to fail fast. Keep the
JSON parse and arr.join(',') logic (so an empty array still yields an empty
string), but delete the surrounding try { ... } catch (e) {
process.stdout.write(''); } block in the node script that sets PROJECTS.
In `@libs/enclave-vm/package.json`:
- Line 39: The dependency entry "@babel/standalone": "^7.26.10" introduces a new
runtime public dependency and either should be bumped to the current stable
"^7.28.6" or you must document and justify why pinning to 7.26.10 is
intentional; update the libs' package.json dependency accordingly (change the
version string for "@babel/standalone") and also add the required docs update
under the repo's docs/draft/docs/** to record this public-API change and
rationale so the SDK publish guidelines are satisfied.
- Line 49: Update package.json to either restore a wider peerDependency range
for "vectoriadb" or document why the narrower "^2.1.3" constraint is required:
inspect changelogs/commits for vectoriadb to confirm breaking changes, and if
the bump is intentional update the package's changelog/docs under
docs/draft/docs/** to state the rationale and add a note about
peerDependencies/peerDependenciesMeta; also decide whether to bump the
enclave-vm package version (currently 2.7.0) to reflect the dependency change
and include that version bump in the package.json and release notes if required.
🧹 Nitpick comments (2)
libs/enclave-vm/src/__tests__/babel-preset-comprehensive.spec.ts (1)
756-761: Performance assertion may be flaky in resource-constrained CI environments.The 5-second threshold is generous, but wall-clock timing tests can still be flaky under heavy CI load. Consider either:
- Increasing the threshold with a safety margin
- Skipping performance assertions in CI (via environment variable check)
- Using a more relative measure or simply ensuring the test completes without timeout
♻️ Optional: Add CI tolerance
const duration = Date.now() - startTime; expect(Object.keys(result.files)).toHaveLength(20); - expect(duration).toBeLessThan(5000); // Should complete in under 5 seconds + // Allow more time in CI environments where resources may be constrained + const maxDuration = process.env.CI ? 10000 : 5000; + expect(duration).toBeLessThan(maxDuration);libs/enclave-vm/src/__tests__/perf/babel.perf.spec.ts (1)
21-21: Remove unused importsbenchmarkandbenchmarkSync.These utilities are imported but never used in the file—all timing is done manually with
performance.now().🧹 Proposed fix
-import { benchmark, benchmarkSync } from './utils/perf-utils';
…release.yml script
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 by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.