Skip to content

runtime: keep the stack of a REPL module-resolution error - #797

Merged
colinhacks merged 2 commits into
mainfrom
repl-resolution-stack
Aug 28, 2026
Merged

colinhacks merged 2 commits into
mainfrom
repl-resolution-stack

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

The nub REPL printed a failed require() with no stack frames, where node prints 8:

> require("./bar")
Uncaught [Error: Cannot find module './bar' ...] { code: 'MODULE_NOT_FOUND' }

The REPL cuts a trace at the last null-named frame — normally its own eval frame. nub's wrappers reach the saved Module._resolveFilename / Module._load through .call(), which nulls those frames' names, and nub's added resolve frames push the REPL's own frame past the default Error.stackTraceLimit of 10. The delegated original was then the only null-named frame left.

Naming the originals also restores at Module._resolveFilename / at Module._load, which printed as at Module.<anonymous>. No frame added or removed.

The CJS wrappers reach the saved `Module._resolveFilename` / `Module._load`
through `.call()`. V8 then finds no property of the receiver holding that
function, so the frame prints as `Module.<anonymous>` instead of its own name
and `CallSite.getFunctionName()` returns null — these internals carry no own
name for it to fall back on.

That null is what breaks the REPL. node:repl truncates an uncaught error's
trace at the LAST null-named frame, which is normally its own `REPL1:1` eval
frame. The frames nub adds to a CJS resolve push that frame past the default
`Error.stackTraceLimit` of 10, leaving the delegated original as the only
null-named frame in the captured array — so the REPL cut the whole trace and
`require("./missing")` printed as a bare `[Error: Cannot find module …]` where
node prints 8 frames.

Name the saved originals before wrapping them. The REPL's cut goes back where
Node puts it, and `at Module._resolveFilename` / `at Module._load` print with
Node's exact text again in ordinary stacks too. No frame is added or removed.
Copilot AI lite review requested due to automatic review settings August 27, 2026 09:11
@vercel

vercel Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 27, 2026 9:28am

Request Review

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ The diagnosis and the fix both hold up — I reproduced them independently. Two informational notes and a comment-accuracy nit.

Reviewed changes — the full diff at 8d1881c (2 files), plus the surrounding wrap sites in runtime/preload-common.cjs and the node:repl truncation logic they interact with.

  • nameInternalFrame helper — sets .name on a saved Node internal via Object.defineProperty before nub wraps it, inside a try/catch that leaves frozen/exotic functions verbatim.
  • Applied at two sites — module_._resolveFilename in installCjsRequireHooks, and module_._load in armChildProcessCompileCacheWrap.
  • New integration test — crates/nub-cli/tests/repl_error_stack.rs drives nub -i and node -i with a failing require, asserting both report Cannot find module and print at least one at frame, with node as the positive control.

I verified the mechanism rather than taking it from the PR body. On Node v24.18.0, Module._resolveFilename.name is "", and V8 only prints that frame correctly because CallSite.getMethodName() finds the function on its receiver — which stops working the moment nub's wrapper occupies the property. Delegating through .call() then yields a frame with both getFunctionName() and getMethodName() null, which is exactly what lib/repl.js cuts on. Preloading a shim that mimics nub's shape (a _resolveFilename wrapper plus a module.registerHooks resolve hook) into a real node -i reproduces it end-to-end: unwrapped prints 8 frames, wrapped-and-unnamed prints 0, wrapped-and-named prints 9. The fix is correct, and it has the pleasant property of being a no-op when the REPL's own frame is still inside the capture window.

The test can genuinely fail — nub -i reaches the node passthrough path, spawn_node injects the preload whenever !compat_mode, and a non-embed-runtime cargo test build resolves the preload to the in-tree runtime/. So the REPL under test really does have the CJS hooks installed.

ℹ️ A third .call()-delegated wrap site still prints <anonymous>

installCjsRequireHooks also saves and wraps module_._extensions[".js"] — nativeJs at preload-common.cjs:1039 and origExtension at :1138, both delegated via .call(module_._extensions, …). That original has .name === "" too, so it takes the same frame-text degradation this PR fixes at the other two sites.

Worth being precise about severity: this one is cosmetic only. That frame is already null-named in plain node, so wrapping it moves no node:repl cut point and cannot swallow a trace. It is a scope call, not a defect.

Technical details
# `module_._extensions[".js"]` keeps the `Module.<anonymous>` frame text

## Affected sites
- `runtime/preload-common.cjs:1039` — `const nativeJs = module_._extensions[".js"];`, delegated at `:1042`
- `runtime/preload-common.cjs:1138` — `const origExtension = module_._extensions[ext] || nativeJs;`, delegated at `:1145` and `:1152`

## Evidence
Probed on Node v24.18.0 against a module that throws at top level:

```
plain node:   at Object..js          (node:internal/modules/cjs/loader:2002:10)
wrapped:      at Object.<anonymous>  (node:internal/modules/cjs/loader:2002:10)
```

`node -e "console.log(JSON.stringify(require('module')._extensions['.js'].name))"` prints `""`.

Frame-level: plain node reports `getFunctionName() === null, getMethodName() === ".js"`;
after wrapping, both are null. Since `getFunctionName()` is null either way, the frame is a
`node:repl` cut point before and after — no truncation behavior changes.

## Required outcome
Decide whether frame-text fidelity at this site is in scope for this PR. If yes, it is the
same one-line `nameInternalFrame` call; if no, no action.

## Open questions for the human
- Is restoring `at Object..js` worth it, given the awkward name V8 would need? Setting
  `.name = "_extensions..js"` yields `Object._extensions..js`, which is close to but not
  identical to node's `Object..js`. Leaving it alone may be the better call.

ℹ️ Nitpicks

  • The _load half of the change has no test coverage — repl_error_stack.rs only exercises the _resolveFilename path.
  • repl_require_missing does not isolate XDG_CACHE_HOME, unlike 30 of the 45 files in crates/nub-cli/tests/. Nothing cache-keyed fires on this path (the scratch dir has no package.json), so this is convention rather than consequence.
  • No timeout around wait_with_output(). It cannot hang today — dropping ChildStdin sends EOF regardless of .exit, and provisioning/consent/verify-deps all short-circuit with no manifest present — so a future regression would surface as the 60-minute job timeout rather than a fast failure.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏

Comment thread runtime/preload-common.cjs Outdated
@colinhacks
colinhacks merged commit 57aa0aa into main Aug 28, 2026
74 checks passed
@colinhacks
colinhacks deleted the repl-resolution-stack branch August 28, 2026 21:37
@colinhacks

Copy link
Copy Markdown
Contributor Author

This branch was successfully deployed

1 active deployment
Preview — 421d35cc Deployed Aug 27, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants