Repository navigation
fix(ast): count UTF-8 bytes without Node's Buffer - #92
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
💤 Files with no reviewable changes (4)
Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe AST now uses a shared UTF-8 byte counter for input sizing and string extraction. Browser demo and e2e bundles no longer inject a Buffer shim. Tests cover sizing without a global Buffer and Unicode output in the browser. ChangesBrowser-compatible UTF-8 sizing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No confirmed issue prevents merging. The browser validation test covers running a Unicode-returning script without a Buffer global. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
@enclave-vm/browser could not validate any script in a plain browser: the pre-scanner's input size check and the string extraction transform called Buffer.byteLength, so run() failed with "Buffer is not defined". The browser e2e suite and the demo app hid it with an injected Buffer shim. Add an allocation-free utf8ByteLength helper (same counts as Buffer.byteLength, lone surrogates included), use it in both places, and drop the shims so the e2e suite runs the package as a browser would. Fixes #87 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
94f68ab to
6f04820
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation correctly preserves UTF-8 sizing semantics and includes focused unit and browser regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Replaces Node-specific UTF-8 sizing in the AST package so browser validation works without a Buffer global.
Changes:
- Adds allocation-free UTF-8 byte counting and adopts it across AST size checks and string extraction.
- Removes browser/demo
Buffershims. - Adds unit and browser regression coverage.
| File | Description |
|---|---|
libs/ast/src/utils/utf8-byte-length.ts |
Implements UTF-8 byte counting. |
libs/ast/src/pre-scanner/checks/size-check.ts |
Uses the browser-compatible utility. |
libs/ast/src/transforms/string-extraction.transform.ts |
Replaces Buffer.byteLength calls. |
libs/ast/src/__tests__/utf8-byte-length.spec.ts |
Tests parity and Buffer-free operation. |
libs/browser/e2e/validation.spec.ts |
Adds browser regression coverage. |
libs/browser/e2e/build-test-bundle.mjs |
Stops injecting the test shim. |
libs/browser/e2e/buffer-shim.mjs |
Removes the obsolete test shim. |
apps/browser-demo/scripts/build-enclave-bundle.mjs |
Stops injecting the demo shim. |
apps/browser-demo/scripts/buffer-shim.mjs |
Removes the obsolete demo shim. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Stack 1/10:
main← this ← #93 ← #94 ← #95 ← #96 ← #98 ← #99 ← #100 ← #101 ← #102Fixes #87.
Validation
Reproduced. In a plain browser,
BrowserEnclave.run('return 1;')failed withReferenceError: Buffer is not defined. The cause isBuffer.byteLengthin@enclave-vm/ast:libs/ast/src/pre-scanner/checks/size-check.ts:21andlibs/ast/src/transforms/string-extraction.transform.ts(3 call sites). The browser e2e suite and the browser demo app hid it with an injectedBuffershim.Fix
libs/ast/src/utils/utf8-byte-length.ts. It counts UTF-8 bytes by walking the string, with the same counts asBuffer.byteLength(a lone surrogate is 3 bytes). It doesn't allocate a copy: the pre-scanner measures untrusted input before any other check, sonew TextEncoder().encode(source)would allocate a buffer as big as the attack input.Buffershims fromlibs/browser/e2e/build-test-bundle.mjsandapps/browser-demo/scripts/build-enclave-bundle.mjs, so the e2e suite runs the package the way a browser does.Red → green
libs/ast/src/__tests__/utf8-byte-length.spec.ts(new: parity withBuffer.byteLengthon 10 samples, plus pre-scan, validation and extraction withglobalThis.Bufferdeleted)ReferenceError: Buffer is not defined), 10 passedvalidates without a Node.js Buffer global)Verification
npx jest -c libs/ast/jest.config.ts: 25 suites, 814 passednpx nx run browser:test-e2e: 64 passedyarn lint,yarn format:check,yarn build: pass (pre-existing warnings only)🤖 Generated with Claude Code
Summary by CodeRabbit
Buffer, including when returning Unicode strings.