bun-inspector-protocol: carry the types other domains refer to into the JSC snapshot - #39475
bun-inspector-protocol: carry the types other domains refer to into the JSC snapshot#39475robobun wants to merge 3 commits into
Conversation
…he JSC snapshot generate-protocol.ts kept only the domains whose debuggableTypes list "javascript", but Console, Debugger and Runtime $ref Network.RequestId, Network.FrameId and GenericTypes.SearchMatch, so the generated jsc/index.d.ts referred to two namespaces it never declared. Every consumer compiles with skipLibCheck, so those four properties silently typed as the error type instead of string / SearchMatch[]. The generator now keeps domains that declare no debuggableTypes (GenericTypes), which WebKit's frontend activates for every debuggable, and emits a types-only copy of any other domain whose types the selected domains refer to, transitively (Network with FrameId and RequestId). A $ref may also name a primitive (Runtime.PropertyDescriptor.isPrivate is a $ref to boolean); those are left alone. The same closure is applied to the V8 snapshot when it is regenerated with --v8; its committed files are not regenerated here. Namespaces are closed with "}" rather than "};" so the output is a valid .d.ts before prettier runs. The script's CLI moved under import.meta.main so the test can import formatProtocol and selectJscDomains. The test now checks that every $ref in protocol.json resolves, that the generator carries along exactly the referenced types for a small fixture, that the committed index.d.ts (and the fixture's) type-checks without skipLibCheck, and that bun has no agent for the types-only domains.
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: fix pushed, waiting for CI. Reproduced with 805c341 (after self-review): the b6a9f47: the debugging-session test in this file has been flaky since #39110 (the inspectee exits with code 1 right after |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — tooling and generated-type-declaration change, no runtime code touched.
Checked: withReferencedTypes closure walk covers types/commands/events, recurses through arrays/objects, is cycle-safe via the ids.has(id) guard, and cannot emit a duplicate domain (early return on selectedNames); primitive $refs are skipped rather than thrown on. Regenerated index.d.ts/protocol.json are purely additive (two new namespaces), and the }; → } change is a no-op on committed output since prettier already stripped the semicolons. typesOnlyDomains derivation is pinned by the test to exactly GenericTypes/Network, so it cannot accidentally sweep up an existing domain.
Extended reasoning...
Overview
The PR fixes four dangling namespace references in packages/bun-inspector-protocol/src/protocol/jsc/index.d.ts (Network.RequestId, Network.FrameId, GenericTypes.SearchMatch) that were silently typing as the error type under skipLibCheck. It does so by extending the generator script to (a) keep domains that declare no debuggableTypes (matching WebKit's own InspectorBackend.activateDomain semantics) and (b) compute the transitive $ref closure over unselected domains and emit types-only copies of them. The generator's CLI body moves under import.meta.main so tests can import its pure functions. Regenerated protocol.json and index.d.ts gain exactly the GenericTypes and Network entries; nothing else moves. schema.d.ts gains an optional description field and a doc comment on $ref. test/tsconfig.json adds the generator script to its include list (same pattern as scripts/build/error.ts).
Security risks
None. No runtime/native code, no user-facing API surface, no untrusted input handling. The generator script is developer tooling that reads a WebKit build artifact and writes checked-in files; it is not part of the shipped binary.
Level of scrutiny
Low-to-medium. This is entirely in the packages/bun-inspector-protocol tooling layer plus its generated snapshot, consumed only by bun-debug-adapter-protocol and bun-vscode (both under skipLibCheck, so the four affected properties go from error-type to their real types with no downstream breakage). No CODEOWNER covers this path. The generated-file diff is additive-only.
Other factors
The new tests are thorough: one asserts the committed snapshot is closed under $ref (fails on the old snapshot naming the four dangling references), one unit-tests selectJscDomains on a fixture that exercises transitive references, bare same-domain references, primitive $refs, and exclusion of unreferenced domains, and one type-checks the committed .d.ts with skipLibCheck off (skipped in debug builds for speed, with the JSON-level tests covering the same invariant there). The existing debugging-session test is tightened to fail on unresolvable $refs and to verify bun still has no agent for the types-only domains. I traced the visit recursion for cycle safety (guarded by the per-domain ids set, cross-domain cycles terminate via the same set on re-entry) and for duplicate-domain emission (impossible: referenced is only populated for domains outside selectedNames). The allByName.get(name)! non-null assertion is sound because name only enters referenced after a successful lookup.
… guard Nothing exports or imports src/protocol/v8 and its committed files are not regenerated here, so downloadV8 goes back to what it is on main. The fixture's Page.Frame now refers to itself, so the walk's already-visited check is what keeps the generator test from recursing forever.
The fixture's reportError makes bun exit with code 1 as soon as the entry module finishes evaluating, which happens right after Debugger.resume. That exit raced the delivery of the resume response and the Debugger.resumed event, so the test failed with "WebSocket closed (1006) (inspectee exit: 1)" in roughly one run in eight under load (20 of 160 locally with 16 running at once; flagged flaky in 8 of the 9 main builds since it landed). The fixture now has a second debugger statement, so after resuming, the inspectee pauses again before the module can finish: everything it sent in between is delivered while it waits there, and the session is closed while it is still paused. 0 of 160 under the same load afterwards.
There was a problem hiding this comment.
LGTM. The stale --v8 description bullet from my earlier note has been updated; thread resolved.
What was reviewed:
withReferencedTypeswalk: cycle guard viaids.has(id)before recursing, primitive-$ref short-circuit, bare refs resolved against the referring domain when recursing into pulled-in types — all exercised by the fixture.- Regenerated
jsc/index.d.ts/protocol.jsonare purely additive (GenericTypes,Networkstubs); no consumer reads the four previously-erroring properties, all compile withskipLibCheck. - Debugging-session test's
setInterval→ seconddebugger;swap: comment explains thereportError-exit vs. last-message race it closes;await using procstill tears the paused inspectee down.
Extended reasoning...
Overview
Tooling-only change to packages/bun-inspector-protocol: the generator script gains selectJscDomains / withReferencedTypes to close the JSC protocol snapshot over cross-domain $refs, plus primitiveTypes and an import.meta.main guard so tests can import it. The regenerated jsc/index.d.ts and jsc/protocol.json add exactly two namespaces (GenericTypes whole, Network with FrameId/RequestId). schema.d.ts gets a description field and a $ref doc comment. test/cli/inspect/bun-inspector-protocol.test.ts adds three tests and tightens the existing debugging-session test; test/tsconfig.json lists the generator script.
Security risks
None. No native code, no runtime code paths, no network/auth/crypto. The generator is manually invoked; the only shipped artifact that changes is a .d.ts with two additive namespaces.
Level of scrutiny
Low-to-medium. This is a dev-tooling package whose consumers (bun-debug-adapter-protocol, bun-vscode) all compile with skipLibCheck and don't read the four affected properties, so the .d.ts addition cannot break them. The withReferencedTypes logic is ~50 lines of straightforward graph walking; I traced the cycle guard (ids.has(id) before visit), the primitive-$ref path (checked only when no domain declares the id, which is correct because a primitive $ref inside a selected domain returns early via selectedNames.has), and bare-ref resolution during transitive visits (visit(type, refDomain) passes the target domain so bare refs inside pulled-in types resolve there). The fixture in the test covers every one of those branches, including a self-referencing type and a two-hop chain through a third domain.
Other factors
- No CODEOWNER for the touched paths (
/packages/bun-types/is the onlypackages/entry). - My prior comment about the stale
--v8description was addressed by the author and the thread is resolved; the description now correctly says that path is deliberately left alone. - The debugging-session test change (fixture
setInterval→ seconddebugger;, awaitpausedAgainbeforews.close()) is a de-flake for a race betweenreportError-driven exit(1) and delivery of the last inspector messages, well explained in the inline comment.await using procstill kills the paused inspectee on test exit. - Test coverage is thorough: snapshot-closure check, generator unit test on a fixture exercising every branch, and a
tsc-based check of the committed.d.ts(skipped in debug builds with a stated reason). The}-vs-};change makes raw generator output valid without prettier and does not change the committed files. - The bug hunting system found nothing on this revision.
1 participant
Problem
packages/bun-inspector-protocol/src/protocol/jsc/index.d.tsrefers to two namespaces it does not declare.tsc --stricton the file alone reportserror TS2503: Cannot find namespace 'Network'at lines 313 (Console.ConsoleMessage.networkRequestId), 753 (Debugger.ScriptParsedEvent.requestId) and 2324 (Runtime.ExecutionContextDescription.frameId), andCannot find namespace 'GenericTypes'at line 1153 (Debugger.SearchInContentResponse.result).bun-debug-adapter-protocol,bun-vscode) compiles withskipLibCheck, which suppresses errors inside.d.tsfiles, so nothing fails: those four properties silently have the error type (behaves likeany) instead ofstringandGenericTypes.SearchMatch[]. None of the current consumers reads those four properties, so nothing is wrong for a user today; this matters for code that is about to be typed against the file (node:inspector: type the CDP adapter's JSC side against the protocol snapshot and typecheck it in CI #39471 typessrc/js/internal/inspector/cdp.tswith it) and for every future regeneration after a WebKit bump, since any new cross-domain reference WebKit adds would dangle the same way and nothing would report it.scripts/generate-protocol.tskeeps only the domains of WebKit'sCombinedDomains.jsonwhosedebuggableTypesinclude"javascript"(thejscobject at the bottom of the script) andformatPropertyemits every$refverbatim.Networkis a web page domain andGenericTypesdeclares nodebuggableTypesat all, so both are dropped, whileConsole,DebuggerandRuntimekeep referring to their types. Three of the four references predate bun-inspector-protocol: regenerate the JSC protocol snapshot from the pinned WebKit #39110 (which regenerated the snapshot);Debugger.ScriptParsedEvent.requestIdarrived with it.Fix
selectJscDomainskeeps a domain that declares nodebuggableTypes, which is how WebKit's own frontend treats one (InspectorBackend.activateDomainactivates such a domain for every debuggable type);GenericTypesis the only such domain in the pinned WebKit, and it comes in whole.withReferencedTypeswalks every$refof the selected domains (types, command parameters and returns, event parameters, through arrays and nested objects) and, for each domain that is not selected, emits a copy holding only the referenced types, following references inside those types in turn. For the pinned WebKit that isNetworkwithFrameIdandRequestId. The copy keeps the domain'sdebuggableTypesand gets a description saying what it is, soprotocol.jsondocuments why a domain with no commands is in it.index.d.tsis a pure function ofprotocol.jsonand each$refbecomes a name in it, so aprotocol.jsonthat is closed under$refgives a.d.tsthat declares every name it uses, with the referenced types keeping their protocol names (JSC.Network.RequestId) rather than being inlined; nothing claimed by the snapshot changes, since the copies carry no commands or events and bun has no agent for either domain (the test checks that it still answers'Network' domain was not found).$refmay also name a primitive:Runtime.PropertyDescriptor.isPrivateis{"$ref": "boolean"}, which WebKit's generator resolves through the primitives it predeclares (models.py,resolve_types). The walk leaves those alone instead of failing on them (primitiveTypes, documented on$refinschema.d.ts;schema.d.tsalso gains thedescriptionfield the JSON already has on domains).--v8path of the generator and the committedv8/files are left as they are: nothing exports or imports them (src/protocol/index.d.tsexports onlyJSC), they have not been regenerated since 2024, andv8/index.d.tshas the same kind of unresolved references (83 of them) by construction of its domain filter. Extending that path without being able to regenerate or test its output would add an unverified change to dead code; removing the path is the better follow-up, and is independent of this PR. The new tests cover the JSC snapshot only.}instead of};, so the generator's raw output is a valid.d.ts(a stray;is a statement, which ambient contexts reject); prettier was already removing them, so the committed files do not change because of this.import.meta.mainso the test can importformatProtocol/selectJscDomains/primitiveTypeswithout side effects (test/tsconfig.jsonlists the file for the same reason it listsscripts/build/error.ts). Running the script is unchanged.jsc/protocol.jsonandjsc/index.d.ts; the diff of both is exactly the addedGenericTypesandNetworkentries. The generator is idempotent on the result, and the pinned WebKit'sCombinedDomains.jsonis byte-identical to the one bun-inspector-protocol: regenerate the JSC protocol snapshot from the pinned WebKit #39110 used, so nothing else moved.test/cli/inspect/bun-inspector-protocol.test.ts:$refinprotocol.jsonresolves to a type in it; against the old snapshot it fails naming the four properties above (output below);selectJscDomainson a smallCombinedDomains.jsonkeeps adebuggableTypes-less domain whole, a referenced web page domain and a referenced no-agent domain as just their referenced types (including a type reached only through a bare same-domain reference and one reached through a second domain), leaves an unreferenced domain out, tolerates a primitive$ref, and terminates on a type that refers to itself (without the already-visited check in the walk this test dies withMaximum call stack size exceeded; checked by removing it);index.d.ts, and the.d.tsgenerated from that fixture, type-check withskipLibCheckoff, while the fixture's JavaScript domains on their own (what the generator used to emit) produce theCannot find namespaceerrors; skipped in debug builds, where loadingtypescriptalone takes about 25s;$refas a problem instead of skipping it, pins the domain list with the two new entries, and checks that bun has no agent for the domains the snapshot holds only the types of.mainbuilds since, and once on this PR): the fixture'sreportErrormakes bun exit with code 1 as soon as the entry module finishes evaluating, i.e. right afterDebugger.resume, and that exit raced the delivery of the resume response and theDebugger.resumedevent (WebSocket closed (1006) (inspectee exit: 1)). The fixture now has a seconddebuggerstatement, so the inspectee pauses again right after resuming and the session is closed while it is paused; 20 of 160 runs failed before under 16-way parallel load, 0 of 160 after.bun bd test test/cli/inspect/bun-inspector-protocol.test.tspasses (3 pass, 1 skip);USE_SYSTEM_BUN=1 bun teston it passes all 4; withpackages/stashed the file fails;./node_modules/.bin/tsc --ignoreConfig --noEmit --strict --target esnext packages/bun-inspector-protocol/src/protocol/jsc/index.d.tsexits 0 (4 errors before);tsconbun-inspector-protocol,bun-debug-adapter-protocolandbun-vscodereports the same pre-existing errors before and after (1, 5 and 1), and no consumer uses the four affected properties;cd test && tsc --noEmithas one error fewer than before (theTS6307that importing the script would otherwise add) and none in the touched files.Background
bun --inspect, split into domains (Debugger,Runtime, ...) that declare types, commands and events. WebKit defines each domain inSource/JavaScriptCore/inspector/protocol/<Domain>.jsonand its build concatenates them intoCombinedDomains.json, which is what the generator reads (from the prebuilt WebKit in the build cache afterbun bd).debuggableTypes: the per-domain list of the kinds of target a domain applies to (javascriptfor a JSContext such as a bun process,web-page,service-worker, ...). The generator uses it to pick bun's domains. A domain may omit it, which WebKit reads as "all of them".$ref: how a property in those JSON files names a declared type instead of spelling out a primitive: bare (RemoteObject, a type of the same domain), qualified (Network.RequestId), or, rarely, a primitive name. Domains reference each other freely because WebKit's C++ generator always builds all domains together; only this package selects a subset.bun-inspector-protocolsnapshot:protocol.jsonis the selected subset ofCombinedDomains.json;index.d.tsis generated from it as one namespace per domain (JSC.Console.ConsoleMessage, ...), with each$refemitted as written, so a reference to a domain outside the subset becomes a reference to a namespace that does not exist.skipLibCheck: a tsc option that skips type-checking the bodies of.d.tsfiles. With it on, an undeclared name inside a.d.tsis not reported; the property just gets the error type, which is assignable to and from anything.New tests against the old generated files
With the old generator script as well, the file fails to load:
Export named 'formatProtocol' not found.[review] gate passed · iteration 0 · 6 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