node:repl: stop aliasing globalThis as repl.context in the stub - #34286
node:repl: stop aliasing globalThis as repl.context in the stub#34286robobun wants to merge 4 commits into
Conversation
The stub's default export was shaped like a REPLServer instance with
context set to globalThis. Libraries that feature-detect a REPL by
probing repl.context and writing helpers into it (the standard
embedding pattern) would silently pollute the real global object,
with no error anywhere. Node's require('node:repl') has no context
property at the module level; that belongs to a REPLServer instance
whose default is a fresh vm context.
Drop the instance-shaped fields and export the module-level shape
Node actually has: start and REPLServer throw ERR_NOT_IMPLEMENTED,
Recoverable extends SyntaxError, REPL_MODE_SLOPPY and REPL_MODE_STRICT
are symbols, writer wraps util.inspect, and builtinModules plus
_builtinLibs remain.
|
Updated 7:50 PM PT - Jul 15th, 2026
❌ @robobun, your commit 9ba9410 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34286That installs a local version of the PR into your bun-34286 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe Node REPL stub
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/stubs.test.js`:
- Around line 180-189: Extend the “exposes Node’s module-level exports” test to
invoke repl.writer with a representative value and compare the result against
inspect(value, repl.writer.options). Also verify that repl.writer.options
initially matches inspect.replDefaults, while preserving the existing type and
export assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10785747-6966-4e81-a2b9-ab00cf9040c9
📒 Files selected for processing (2)
src/js/node/repl.tstest/js/node/stubs.test.js
|
Re: the issue-linker suggestion for #31470: this PR does not fix that. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: the duplicate detector: #28480 (+534 lines) and #31827 (+13226 lines) are full |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The stub reshape looks correct and the context: globalThis removal fixes a real hazard, but this changes the user-visible export shape of require("node:repl") while two full-implementation PRs (#28480, #31827) are open for the same file — a maintainer should make the call on landing the interim fix vs. waiting.
Checked: new export shape matches Node's module-level repl (start/REPLServer/Recoverable/writer/mode symbols); inspect.replDefaults exists in Bun's util.inspect; throwNotImplemented(feature, issue) signature is correct.
Checked: no in-tree consumers of the removed instance fields — src/runtime/cli/repl.rs hits are an unrelated Rust struct; test-repl-preview-without-inspector.js already fails on REPLServer either way.
Ruled out: eager top-level require("node:util") — only runs when node:repl is imported, and the module is a stub.
Extended reasoning...
Overview
Reshapes the node:repl stub in src/js/node/repl.ts from a fake REPLServer-instance-shaped object (with context: globalThis, terminal, useGlobal, Proxy-trapped input/output, etc.) to Node's actual module-level export shape: start()/REPLServer() that throw ERR_NOT_IMPLEMENTED pointing at #28478, a minimal Recoverable extends SyntaxError, REPL_MODE_SLOPPY/REPL_MODE_STRICT symbols, and writer() wrapping util.inspect with inspect.replDefaults. _builtinLibs/builtinModules are unchanged. Adds a 7-test describe block to test/js/node/stubs.test.js.
Security risks
None introduced. The change actually removes a hazard: the old context: globalThis let any library that feature-detects a REPL and writes helpers into repl.context silently pollute the real global object. Now that write throws (since repl.context is undefined), matching Node.
Level of scrutiny
Moderate. It's a ~35-line net change to an explicitly-labeled stub module, but it is a user-facing Node-compat surface change: packages that were (incorrectly) probing removed fields like repl.terminal or repl.useGlobal will now see undefined instead of true. That's more Node-compatible, but it's still a behavior change. I verified inspect.replDefaults exists (defined via getter in src/js/internal/util/inspect.js:767), throwNotImplemented accepts (feature, issue), and grepped for in-tree consumers of the removed fields — the only hits are the Rust CLI REPL's own repl.history struct field (unrelated) and a Node parallel test that already fails on REPLServer before and after.
Other factors
Two open PRs (#28480 at +534 lines, #31827 at +13226 lines) fully implement node:repl and would replace this file wholesale. The author's position — land this as a targeted safety fix now, trivial rebase for the full impls — is reasonable, but whether to take an interim stub change vs. push one of the full implementations across the line is a maintainer/product call I shouldn't make. Tests are solid (subprocess isolation for the pollution check with bunEnv and expect.any(String) for stderr, concurrent pipe draining, ERR_NOT_IMPLEMENTED code assertions), and the CodeRabbit nit about writer() behavior was addressed in 97d95b9.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/stubs.test.js (1)
124-143: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the module-shape assertions.
The test only checks property absence for
context; other instance fields could be reintroduced withundefinedvalues and still pass. Likewise, any arrays or symbols satisfy the export test. Assert every instance field is absent, compare both builtin lists against the expected values, and verify the two REPL mode symbols are distinct.Also applies to: 182-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/stubs.test.js` around lines 124 - 143, Strengthen the module-shape assertions in the REPL stub tests: verify each REPLServer instance field is absent from repl, not merely undefined, and tighten the export checks to compare both builtin lists with their expected values. Also assert that the two REPL mode symbols are distinct, updating the related assertions near the second referenced section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/stubs.test.js`:
- Line 194: Move the node:util inspect import from its current location into the
module-scope import block in stubs.test.js, keeping the existing inspect usage
unchanged and avoiding a runtime require() in the test body.
---
Outside diff comments:
In `@test/js/node/stubs.test.js`:
- Around line 124-143: Strengthen the module-shape assertions in the REPL stub
tests: verify each REPLServer instance field is absent from repl, not merely
undefined, and tighten the export checks to compare both builtin lists with
their expected values. Also assert that the two REPL mode symbols are distinct,
updating the related assertions near the second referenced section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73d1641e-fbc1-499d-8891-2183887446b2
📒 Files selected for processing (1)
test/js/node/stubs.test.js
…eld absence via 'in'
There was a problem hiding this comment.
LGTM — targeted safety fix that reshapes the node:repl stub to match Node's actual module-level export surface and removes the context: globalThis alias that could silently pollute the real global.
What was reviewed:
- Verified
throwNotImplemented(feature, issue)signature ininternal/shared.tsmatches the new call sites. - Confirmed
inspect.replDefaultsexists as a getter ininternal/util/inspect.js, so{ ...inspect.replDefaults }is well-defined. - Grepped for in-tree consumers of the removed instance-shaped fields (
repl.context,.input,.output, etc.) — none outside the tests being updated. - Subprocess test follows repo conventions:
bunEnv, concurrent pipe drain,stderr: expect.any(String), combined-object assertion;JSON.stringifydroppingpolluted: undefinedis fine under.toEqualsemantics.
Extended reasoning...
Overview
Two files touched: src/js/node/repl.ts (the stub itself, ~55 lines removed / ~35 added) and test/js/node/stubs.test.js (+87 lines of coverage). The stub previously exported an object shaped like a REPLServer instance — including context: globalThis — which is not what Node's require('node:repl') returns. The PR replaces it with the real module-level surface: start, REPLServer, Recoverable, REPL_MODE_SLOPPY/STRICT, writer, plus the pre-existing _builtinLibs/builtinModules. start() and REPLServer() throw ERR_NOT_IMPLEMENTED referencing issue 28478.
Security risks
The change is a net security/robustness improvement: it removes the one path by which importing node:repl and following the standard repl.context.helper = ... pattern would mutate globalThis in Bun. No new attack surface is introduced — writer is a thin wrapper over util.inspect, and everything else either throws or is a plain constant. No auth/crypto/permissions code involved.
Level of scrutiny
Low-to-moderate. This is a stub module explicitly marked "None of this is actually implemented yet"; the change makes its shape more honest (matches Node) and safer (no silent global aliasing). The only behavioral question is whether anything in-tree depended on the removed instance fields — a repo-wide grep for repl.context/repl.input/repl.output/repl.useGlobal/repl.terminal found only the new test and an upstream Node parallel test that operates on an actual REPLServer instance, not the module export. No packages/bun-types declarations describe these fields either.
Other factors
throwNotImplementedalready accepts(feature, issue?), so passing28478is correct.inspect.replDefaultsis a real getter on Bun'sutil.inspectreturning an options object, so the spread and thetoEqualassertion are sound.- Tests follow CLAUDE.md conventions:
bunEnv,await usingon the spawn, concurrentPromise.alldrain of stdout/stderr/exited, loose stderr match viaexpect.any(String), combined-object assertion. The PR description shows the suite fails on system Bun and passes on the debug build. - All CodeRabbit review threads are resolved; the author already clarified the relationship to #28480/#31827 (full implementations that would supersede this) and #31470 (not fully fixed here). No outstanding human reviewer comments.
|
CI status: the diff itself is green.
None of these import |
|
The node:repl stub this PR reshapes was replaced by the full implementation in #31827 (which closed #28478), so src/js/node/repl.ts no longer exists. On main, "context" is not a property of the node:repl module and assigning through repl.context throws, matching Node, so the bug this PR fixed is gone. Closing. |
1 participant
What
The
node:replstub's default export was shaped like aREPLServerinstance withcontext: globalThis. This is the one place the stub silently lied instead of throwing: libraries that feature-detect a REPL by probingrepl.contextand writing helpers into it (the standard REPL-embedding pattern) would silently pollute the real global object.Repro
In Node,
contextis aREPLServerinstance property (and defaults to a freshvmcontext sinceuseGlobaldefaults tofalse); it does not exist on the module export.Fix
src/js/node/repl.tsnow exports the module-level shape Node actually has instead of a fake instance:context,terminal,useGlobal,lines,history,historyIndex,cursor,historySize,removeHistoryDuplicates,crlfDelay,completer,_initialPrompt,input,output,line,eval,isCompletionEnabled,escapeCodeTimeout,tabSize,breakEvalOnSigint,underscoreAssigned,last,_domain,allowBlockingCompletions,useColorsstart()andREPLServer()now throwERR_NOT_IMPLEMENTEDpointing atrepl.startnot implemented in Bun #28478Recoverableis a minimalSyntaxErrorsubclass matching NodeREPL_MODE_SLOPPY/REPL_MODE_STRICTare theSymbol("repl-sloppy")/Symbol("repl-strict")symbolswriterwrapsutil.inspectwithinspect.replDefaults_builtinLibs/builtinModulesare unchangedVerification
Note: #28480 is a full
REPLServer/start()implementation; this PR only fixes the stub's shape so it is safe and honest until that lands.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The node:repl stub exported an object shaped like a REPLServer instance, including
context: globalThisand other instance fields, rather than the module-level surface Node actually returns fromrequire('node:repl'). Libraries that feature-detect a REPL by writing helpers torepl.contextwould therefore silently mutate the real global object. The fix replaces the stub with Node's true module-level exports (start, REPLServer, Recoverable, mode constants, writer, builtinModules) socontextis undefined and such writes fail instead of polluting globalThis.