node:inspector: derive scriptParsed isModule and scriptLanguage from JSC's scriptType - #39051
Conversation
…JSC's scriptType The CDP adapter read a `module` parameter that JSC's Debugger.scriptParsed does not have, so every script was reported with isModule: false. JSC carries this as scriptType (program, module or webassembly); use it for isModule and for scriptLanguage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 1 remains after this review. WalkthroughThe inspector adapter derives ChangesScript metadata translation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on the released bun with the new test in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line adapter fix that reads JSC's actual scriptType field instead of a nonexistent module field.
What was reviewed:
- Confirmed the mapping (
scriptType === "module"→isModule,"webassembly"→scriptLanguage: "WebAssembly") matches V8's CDP shape; undefinedscriptTypefalls back to the old behavior (false/"JavaScript"). - Noted
packages/bun-inspector-protocol/src/protocol/jsc/protocol.jsonstill lists the oldmodule: boolean— that file is a stale copy for the VS Code extension's typegen, not what the runtime backend sends; robobun's repro confirms the live backend emitsscriptType. - Test covers all three script kinds, follows the neighboring fixture pattern, and uses an ordered command reply as the flush barrier so the event ordering assertion is deterministic.
Extended reasoning...
Overview
Two files touched. In src/js/internal/inspector/cdp.ts, the Debugger.scriptParsed translation now reads params.scriptType (which JSC's backend actually sends) instead of params.module (which it doesn't), and derives both isModule and scriptLanguage from it. The test file gains one new test that spawns a fixture acting as its own CDP client, loads an .mjs, a .cjs, and an empty WebAssembly.Module, and asserts the three scriptParsed events carry the right isModule/scriptLanguage.
Security risks
None. This is a read-only field mapping in the JSC→CDP protocol translator for the DevTools inspector. No user input is parsed, no auth/crypto/filesystem paths are touched.
Level of scrutiny
Low. The runtime change is two field expressions. If scriptType were somehow absent, both comparisons yield false and the output is identical to the pre-PR behavior (isModule: false, scriptLanguage: "JavaScript"), so there's no regression risk. I cross-checked the in-repo packages/bun-inspector-protocol/src/protocol/jsc/protocol.json, which still lists the legacy module: boolean field — but that's a stale snapshot used for the VS Code extension's type generation, not the live protocol; robobun's status comment confirms empirically that the released binary produces isModule: false for an .mjs (proving params.module is undefined at runtime) and this branch produces isModule: true (proving params.scriptType === "module" is present).
Other factors
The test mirrors the file's established fixture-as-its-own-CDP-client pattern (same tempDir/Bun.spawn/concurrent-drain shape as the neighboring tests), uses port: 0, and synchronizes on the ordered backend reply queue rather than sleeping. It asserts an ordered array, which is safe because each import is awaited sequentially and the trailing setBreakpointsActive reply proves all prior events have been delivered through the same ordered channel. The URL filter excludes the fixture itself and any replayed internal scripts from Debugger.enable. No CODEOWNERS cover these paths, no outstanding review comments, and the bug-hunting system found nothing.
|
Updated 1:50 AM PT - Aug 17th, 2026
✅ @robobun, your commit 94d4d58b7b03ac250d10dcd31e629b43fb0f6596 passed in 🧪 To try this PR locally: bunx bun-pr 39051That installs a local version of the PR into your bun-39051 --bun |
… pinned WebKit (#39110) ### Problem - `packages/bun-inspector-protocol/src/protocol/jsc/protocol.json` and the `index.d.ts` generated from it were last regenerated in November 2024 (#15261) and no longer describe the protocol the WebKit pinned in `scripts/build/deps/webkit.ts` speaks. - The visible symptom: `JSC.Debugger.ScriptParsedEvent` has `module?: boolean`, but bun sends `executionContextId` and `scriptType` (`"program" | "module" | "webassembly"`, the `Debugger.ScriptType` type the snapshot lacks) and never sends `module`. Code written against the types reads `params.module` and always gets `undefined` (the `node:inspector` side of that is fixed separately in #39051, which does not touch this package). - Diffing the snapshot against the pinned WebKit's protocol also shows: `LifecycleReporter.getModuleGraph` missing, the `BunFrontendDevServer` and `HTTPServer` domains (both served by bun, see `src/jsc/bindings/BunDebugger.cpp`) missing entirely, `Console.ChannelSource` out of date, plus description and `targetTypes` metadata changes. `TestReporter` was already current (#20645 updated it by hand). - Nothing regenerates the snapshot on a WebKit bump and no test compares it with what bun sends, which is how it drifted. ### Fix - Regenerated `jsc/protocol.json` and `jsc/index.d.ts` with `scripts/generate-protocol.ts` from the `CombinedDomains.json` shipped in the prebuilt `bun-webkit` tarball for `WEBKIT_VERSION` (`f0f60fd2`). The only removed lines in `index.d.ts` are `module`, the `appcache` channel and four description strings; everything else is additions. The snapshot is the protocol the linked WebKit was built from, which is the definition of what bun can send, so it is correct by construction; the new test checks that against the running binary. - `generate-protocol.ts`: locates the pinned WebKit's `CombinedDomains.json` in the build cache when no path is given (so `bun packages/bun-inspector-protocol/scripts/generate-protocol.ts` after a `bun bd` is the whole regeneration), refreshes the V8 snapshot only with `--v8` (needs network, and nothing in the repo imports it, so it is left untouched here), removes the dead `getJSC()`, and skips the `File` and `Process` domains: the WebKit fork declares them as JavaScript-debuggable, but bun registers no agent for them and answers `'Process' domain was not found`, so types for them would be as misleading as the stale `module` was. The test pins both sides of that list. - `schema.d.ts`: `Domain.types` and `Event.parameters` are optional in the actual JSON (`Audit` has no types, `Debugger.resumed` has no parameters); `debuggableTypes` is what the generator filters on. - `bun-debug-adapter-protocol/adapter.ts`: `DebugAdapterEventMap` was `InspectorEventMap & ...`, so with the new domains it would advertise `HTTPServer.*` / `BunFrontendDevServer.*` events the adapter never re-emits (since #31559 it only forwards an allowlist of domains). It is now derived from that allowlist; `isInspectorEvent` became the matching type guard, which also removes the `as keyof JSC.EventMap` cast. No runtime change. Type-checking `bun-debug-adapter-protocol`, `bun-inspector-protocol` and the `bun-vscode` files that import `JSC` types gives the same set of pre-existing errors before and after this change. - `.gitattributes`: the `linguist-generated` patterns still pointed at the pre-#4345 `protocol/` layout, so they matched nothing; they now cover `src/protocol/*/`, which is what collapses the two generated files in this PR's diff. - Test: `test/cli/inspect/bun-inspector-protocol.test.ts` starts `bun --inspect-wait` on an ESM + CJS fixture, enables every domain in the snapshot, pauses on a `debugger` statement, evaluates, and validates every event and response it receives (recursively, through `$ref`s and enums) against `protocol.json`; it also asserts the exact domain list and that `File.enable` / `Process.enable` are still rejected, so removing or adding a domain in the generator shows up here. Against the old snapshot it fails with: ``` "Debugger.scriptParsed: property executionContextId is not in the snapshot", "Debugger.scriptParsed: property scriptType is not in the snapshot", "LifecycleReporter.getModuleGraph: command is not in the snapshot", ``` and passes with the regenerated one (`bun bd test test/cli/inspect/bun-inspector-protocol.test.ts`). The rest of `test/cli/inspect/` passes as before (the `localhost` cases of `inspect.test.ts` fail in this container with the released bun as well; unrelated). - Also verified: the generator is idempotent, its output equals the per-domain JSON files in `vendor/WebKit/Source/JavaScriptCore/inspector/protocol/` at the pinned commit, and `--v8` and an explicit path argument still work. ### Background - Inspector protocol: the JSON-RPC-style protocol spoken over `bun --inspect`, organized into domains (`Debugger`, `Runtime`, ...) with commands, events and types. WebKit defines each domain in `inspector/protocol/<Domain>.json`; its build concatenates them into `CombinedDomains.json` and generates the C++ dispatchers from that, so a bun binary can only send what those files declare. Bun's fork adds domains such as `LifecycleReporter`, `TestReporter`, `HTTPServer`, `BunFrontendDevServer`, and bun registers an agent for each in `BunDebugger.cpp`; a domain with no agent is rejected with `domain was not found`. - `bun-inspector-protocol` keeps a copy of the JavaScript-debuggable subset of those definitions (`protocol.json`) and generates TypeScript types from it (`JSC.<Domain>.<Name>`, plus `EventMap` / `RequestMap` / `ResponseMap`). `bun-debug-adapter-protocol` (the VS Code debugger) and `bun-vscode` are its consumers. - `debuggableTypes` is the per-domain list in WebKit's JSON of which kinds of targets a domain applies to; the generator keeps the domains tagged `"javascript"`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line adapter fix reading the correct JSC protocol field, with a test covering all three scriptType values.
What was reviewed:
- Verified against
packages/bun-inspector-protocol/src/protocol/jsc/protocol.json:ScriptTypeis"program" | "module" | "webassembly"andscriptParsedcarriesscriptType(the oldmodulefield is gone after #39110), so the mapping is exhaustive. - CDP output values match V8's
Debugger.ScriptLanguageenum ("JavaScript"/"WebAssembly") andisModulesemantics (ES modules only; CJS is a program in both engines). - Test follows the file's existing fixture pattern, drains pipes concurrently, awaits an ordered command reply as the barrier instead of sleeping, and is hermetic.
Extended reasoning...
Overview
Two files touched. src/js/internal/inspector/cdp.ts changes two field values in the Debugger.scriptParsed translation: isModule now reads scriptType === "module" (previously !!params.module, a field JSC's protocol no longer emits, so it was always false), and scriptLanguage now reports "WebAssembly" when scriptType === "webassembly" (previously hardcoded "JavaScript"). test/js/node/inspector/inspector.test.ts adds one test that opens the inspector, connects as a CDP client, loads an .mjs, a .cjs, and an empty WebAssembly.Module, and asserts the three emitted events.
Security risks
None. This is metadata in debugger protocol events sent to an already-connected DevTools frontend; it does not affect execution, parsing, permissions, or any security boundary.
Level of scrutiny
Low. The production change is three lines (one destructure + two field expressions) in a protocol-translation adapter, and the mapping is mechanically derivable from the two protocol definitions. I confirmed the JSC side against the regenerated protocol snapshot at packages/bun-inspector-protocol/src/protocol/jsc/protocol.json:607-609 — ScriptType is exactly ["program", "module", "webassembly"], so === "module" and === "webassembly" cover the enum with "program" (and any undefined) falling through to isModule: false / "JavaScript", which is the correct default.
Other factors
The test is well-constructed per the repo's review criteria: it uses tempDir/bunEnv/bunExe, port: 0, drains stdout/stderr/exited concurrently, and awaits a command reply on the same ordered backend queue as the barrier for event arrival rather than sleeping. It exercises all three scriptType variants. The PR description states it fails on the released bun and passes with the change (USE_SYSTEM_BUN=1 verified by robobun). The comment-cop inline comment was addressed in 94d4d58 and the thread is resolved. No outstanding reviewer feedback.
2 participants
Problem
Debugger.scriptParsedevent that theinspector.open()DevTools server sends hasisModule: false, ES modules included, and aWebAssembly.Moduleis announced withscriptLanguage: "JavaScript".src/js/internal/inspector/cdp.ts:583builds the event fromparams.module, a parameter JSC'sDebugger.scriptParseddoes not have, so it is always undefined.modulecomes from an older WebKit protocol;packages/bun-inspector-protocol's JSC protocol snapshot still lists it, but the WebKit Bun builds replaced it. Regenerating that snapshot is a separate change; nothing else in the repo readsmodule.scriptType, one of"program","module"or"webassembly"(vendor/WebKit/Source/JavaScriptCore/inspector/protocol/Debugger.json;scriptTypeForScriptinInspectorDebuggerAgent.cppmaps Bun'sModuleandBunTranspiledModulesource providers to"module"and CommonJS files, which Bun evaluates as programs, to"program").Fix
scriptType:isModuleisscriptType === "module",scriptLanguageis"WebAssembly"for"webassembly"and"JavaScript"otherwise.isModule: trueonly for ES modules (CommonJS files are programs in both engines), andisModule: falsewithscriptLanguage: "WebAssembly"for wasm modules. The adapter forwards JSC's classification instead of reading a field its protocol never carries.test/js/node/inspector/inspector.test.ts, "Debugger.scriptParsed reports isModule and scriptLanguage from JSC's scriptType". The fixture is its own CDP client: it enables the Debugger domain, imports an.mjsand a.cjsfile, compiles an emptyWebAssembly.Module, and the test checks the three events. On the current release it fails withisModule: falseforesm.mjsandscriptLanguage: "JavaScript"for the wasm module; it passes with this change.bun bd test test/js/node/inspector/inspector.test.ts: 24 pass.Background
inspector.open()serves the V8 Chrome DevTools Protocol (CDP). Bun's debugger backend speaks JSC's inspector protocol, andsrc/js/internal/inspector/cdp.tstranslates between the two for each connection.Debugger.scriptParsedexists in both protocols and is emitted once per script the engine compiles. CDP clients readisModuleto know a script is an ES module (DevTools labels it as one and evaluates console input in module terms) andscriptLanguageto tell JavaScript from WebAssembly.WebAssembly.Moduleas a script (Debugger::sourceParsed(JSGlobalObject*, JSWebAssemblyModule*)), naming it<n>.wasmwhen the module has no source URL. That is the wasm event the test checks.inspector.Sessiongoes through the same adapter but discards events, so only theinspector.open()WebSocket path observes this.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The CDP adapter was not translating JSC's script metadata into the fields V8-style clients expect, so Debugger.scriptParsed events misreported ES modules and WebAssembly scripts as plain JavaScript programs. JSC describes a script with a single scriptType value of "program", "module", or "webassembly", while the CDP protocol splits that same information into isModule and scriptLanguage. The fix reads scriptType and maps it accordingly, setting isModule only for module scripts and reporting scriptLanguage as "WebAssembly" for wasm scripts, with integration tests covering ESM, CommonJS, and W…