Conversation
The API module loader hashed route source with `new Uint8Array(digest).toHex()`.
That method is native and unflagged in Deno, and absent from every Node release
before 25 -- it is the Uint8Array base64/hex proposal, still behind V8's
--js-base-64 flag there and not settable via NODE_OPTIONS. So on the supported
Node floor, `veryfront dev` returned 500 for *every* API route in every app:
Failed to load API handler: (intermediate value).toHex is not a function
Reuse the hand-rolled hex encoder that already exists two files over in
hash-utils.ts, which is what the rest of the codebase hashes with.
The clean-room npm smoke test already ran on Node 24, which lacks the method,
and stayed green because it only ever requested `/`. Step 8 now drops an API
route into the packed starter and requires a 200, which is the path that was
broken.
That step strips the proposal methods explicitly rather than trusting the
runner's Node version: Node 25 ships them unflagged, so a version-dependent
check would go quietly vacuous the moment CI moves off Node 24. It asserts the
strip worked before relying on it.
Verified against the real build:
with the fix steps 1-8 pass, exit 0
without the fix steps 1-7 pass, step 8 fails with
"API route returned 500, expected 200 (issue #3968)"
Step 7 passes in both, which is the coverage gap this closes.
Fixes #3968
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe module loader now uses shared hashing utilities for prepared module bytes and source strings. The npm smoke test adds API-route coverage under a runtime without Uint8Array base64/hex proposal methods. ChangesNode API compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR replaces an unsupported runtime API used to load API routes and adds regression coverage for that path; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing as superseded by #3970, which is already merged. #3970 applies the existing cross-runtime computeHash implementation at all three API module-loader hash sites and adds a packed npm smoke that starts the Node server and requires a real API route to return HTTP 200 with the expected JSON body. The published RC (veryfront@0.1.1250-rc.13757) has also been verified against the original reporter repository on stock Node 22 and Node 24. The useful additional concerns from this PR are preserved separately:
The stable 0.1.1250 promotion is proceeding in #3973, so merging this conflicting duplicate would add risk without adding loader coverage. |
Fixes #3968.
The bug
src/routing/api/module-loader/loader.tshashed route module source withnew Uint8Array(digest).toHex(). That is the Uint8Array base64/hex proposal — native andunflagged in Deno, and absent from every Node release before 25, where it sits behind V8's
--js-base-64and cannot be enabled throughNODE_OPTIONS.On the supported Node floor,
veryfront devtherefore returned500for every API route inevery app, not just workflow routes:
Reproduction
Minimal app — a
package.jsonand oneapp/api/hello/route.tsreturningResponse.json({ ok: true })— against published
veryfront@0.1.1248:Uint8Array.prototype.toHexGET /api/hello500—toHex is not a function500200 {"ok":true}500— identical failureSame app, same package, one variable.
The fix
Reuse the hand-rolled hex encoder already present in
src/utils/hash-utils.ts, which is how therest of the codebase hashes. Net −5/+3 lines in
loader.ts.Why this shipped
tests (npm install smoke)already does a clean-room install of the built npm packages and runs areal dev server on Node 24 — which lacks the method. It stayed green because step 7 only ever
requests
/. Page rendering never touches the API module loader, so a total API outage wasinvisible to the one job positioned to catch it.
Step 8 closes that: it drops an API route into the packed starter and requires
200 {"ok":true}.It strips the proposal methods explicitly instead of trusting the runner's Node version. Node 25
ships them unflagged, so a version-dependent check would go quietly vacuous the moment CI moves off
Node 24 — the same way step 7 was vacuous for this bug. The step asserts the strip actually worked
before relying on it, so a preload that silently stopped working can't leave the step passing for
the wrong reason.
Verification against the real build
deno task build:npmthenbash scripts/test/npm-install-smoke.shon Node 24:.toHex()restored in the built artifact)API route returned 500, expected 200 (issue #3968)Step 7 passes in both rows. That is the coverage gap, demonstrated rather than asserted.
Also run:
deno task fmt:check,deno task lint,deno task typecheck, anddeno task test src/routing/api/module-loader/ src/routing/api/handler.test.ts src/routing/api/route-executor.test.ts(11 passed, 264 steps, 0 failed).
Deliberately not in this PR
Two more production call sites use the same proposal and are not fixed here:
src/security/sandbox/worker-generation.ts:74—digest.toHex()src/security/sandbox/worker-script.ts:159-160— captures bothtoBase64andtoHexoffNativeUint8Array.prototypeat module loadStep 8 does not exercise those paths — I confirmed that from the run, rather than assuming the
class is closed. They are the same latent break on the same Node floor and deserve their own change
with their own reproduction; folding them in here on a guess would ship untested edits to the
sandbox. Worth a follow-up issue.
The
.toHex()calls in test fixtures (handler.test.ts,route-executor.test.ts,worker-script.test.ts) are left alone — they only run under Deno.Summary by CodeRabbit
Bug Fixes
Tests