Bump WebKit to 3997b59485da - #37352
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe build pins a newer WebKit commit. Inspector tests now cover Bun-transpiled module metadata and breakpoint resolution in isolated and non-isolated runs. Existing inspector fixtures use ChangesWebKit and debugger validation
Suggested reviewers: 🚥 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: 2
🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: Add an automated debugger regression test alongside the WEBKIT_VERSION
update, covering runtime ESM modules with prebuilt module records. Use
Debugger.setBreakpointByUrl to set a line breakpoint and verify execution pauses
at that breakpoint without including or relying on a debugger; statement; keep
the test independently runnable in this change.
- Line 6: Add the WebKit pin to the expected versions object in the
process.versions regression assertion within process.test.js, using the
WEBKIT_VERSION value exactly so process.versions.webkit is checked against the
configured pin and future drift is detected.
🪄 Autofix
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: 87974565-f593-4284-bdf5-111fc58baddb
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — routine WebKit version bump.
What was reviewed:
- Single-line hash change to
WEBKIT_VERSION; no other build config touched. - Description scopes the fork commit to adding
BunTranspiledModulearms in JSC debugger/cache switches — no upstream merge riding along. - Test coverage is deferred to #35605 per the description; the bump itself has no independently testable surface.
Extended reasoning...
Overview
This PR changes one line in scripts/build/deps/webkit.ts, updating the WEBKIT_VERSION constant from 447082ab6897... to 723cea6c8c6c.... Per the description, that delta is a single commit on oven-sh/WebKit (#405) that adds a BunTranspiledModule case alongside Module in JSC's DebuggerParseData, CachedTypes, and Completion switches, so line breakpoints resolve for prebuilt-module-record ESM. No other build machinery, flags, or ABI-affecting knobs in webkit.ts are touched.
Security risks
None. This is a version pin for a vendored dependency built from Bun's own fork. The change does not touch auth, crypto, input parsing, or any user-reachable code path in this repo; the actual behavioral change lives in the WebKit fork and was reviewed there.
Level of scrutiny
Low. Version bumps of this shape are routine in this repo (two recent commits on main are WebKit bumps of the same form). The constant feeds the prebuilt tarball URL and local-checkout ref; if the hash were wrong or the release artifact missing, CI would fail loudly at the fetch/build step rather than silently misbehave. There is nothing in the one-line diff itself that can be independently wrong beyond a typo in the hash, which CI validates.
Other factors
The PR intentionally ships without a test — the description explains the inspector regression test lands with #35605, which depends on this bump landing first. That's a reasonable split for a dependency bump whose behavioral effect isn't reachable until the follow-up wires every bun run module through the new source type. The bug hunting system found nothing, and there are no prior reviewer comments or outstanding threads on the PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/inspect/debugger-buntranspiledmodule.test.ts`:
- Around line 56-81: Update the stderr reader around the async stream handler to
process the final stderrLineBuf after the loop ends, using the same trimming and
WebSocket URL detection as completed lines, before rejecting when urlFound is
still false. Preserve the existing rejection behavior when neither completed
lines nor the final partial line contains an inspector URL.
- Around line 156-159: Update the Inspector.initialized handling near
pausedPromise to route command rejections through failAll instead of swallowing
them with an empty catch, ensuring the pending Debugger.paused waiter rejects
with the original error and every failure path completes the test operation.
🪄 Autofix
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: ac01e2de-74a2-40d7-a586-546c98a3d041
📒 Files selected for processing (1)
test/cli/inspect/debugger-buntranspiledmodule.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/inspect/debugger-buntranspiledmodule.test.ts (1)
29-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert subprocess completion and propagate resume failures.
runDebuggerProbedrops both drain promises, never awaitsproc.exited, and suppressesDebugger.resumefailures. A fixture can fail afterdebugger;, exit abnormally, or remain paused while the test passes. Retain the drain promises, handle the resume result, then await stdout, stderr, andproc.exitedtogether. Assert the exit code after the diagnostics.🤖 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/cli/inspect/debugger-buntranspiledmodule.test.ts` around lines 29 - 41, Update runDebuggerProbe to retain the stdout and stderr drain promises, check and propagate failures from Debugger.resume, then await both drains together with proc.exited before completing. After diagnostics have been collected, assert that the subprocess exits with the expected code.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/cli/inspect/debugger-buntranspiledmodule.test.ts`:
- Around line 29-41: Update runDebuggerProbe to retain the stdout and stderr
drain promises, check and propagate failures from Debugger.resume, then await
both drains together with proc.exited before completing. After diagnostics have
been collected, assert that the subprocess exits with the expected code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5ff46d8-3620-48fa-b967-f50b750ff795
📒 Files selected for processing (1)
test/cli/inspect/debugger-buntranspiledmodule.test.ts
There was a problem hiding this comment.
Thanks for addressing the earlier feedback — the skipIf(isCI && isASAN) gate is gone, the header comment now correctly points at test/no-validate-exceptions.txt, and the CodeRabbit notes (trailing stderr line, Inspector.initialized → failAll) are incorporated. I didn't find any bugs in this revision. Since this bumps the JS engine itself (the actual behavioral change lives in oven-sh/WebKit#405, which I can't inspect from here) and ships a ~250-line inspector-protocol harness, a maintainer glance would still be worthwhile.
What was reviewed:
- Confirmed
isolatedModuleCacheSourceTypeexists inbun:internal-for-testingand is used bytest/cli/test/isolation.test.ts, so the fixture's premise self-check is wired to a real export. - Checked the test against REVIEW.md test rules:
tempDir/bunEnv/bunExe,port: 0via the127.0.0.1:0inspect URL, concurrent stdout/stderr drain,failAllwired to ws error/close so every pending waiter rejects,await usingfor the subprocess,test.concurrent, full-objecttoEqualon the CDP replies. - Verified the
no-validate-exceptions.txtreference in the new header comment resolves (line 144).
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts moves WEBKIT_VERSION from 447082ab6897 to 723cea6c8c6c (one commit on oven-sh/WebKit, PR #405, per the description: adds a BunTranspiledModule arm next to Module in JSC's DebuggerParseData/CachedTypes/Completion switches). test/cli/inspect/debugger-buntranspiledmodule.test.ts is a new 251-line regression test that spawns bun test --isolate under --inspect-wait, connects over the WebSocket inspector transport, and asserts that Debugger.scriptParsed reports scriptType: "module" and that both Debugger.setBreakpoint and Debugger.setBreakpointByUrl resolve to concrete locations. A second non---isolate case pins the baseline.
Security risks
None identified. The version pin is a commit hash on Bun's own WebKit fork; the build script only consumes it to construct a GitHub Releases download URL and checkout ref. The test spawns a local subprocess bound to 127.0.0.1:0 and connects to it — no external network, no untrusted input.
Level of scrutiny
High, on the dependency side. WebKit/JavaScriptCore is the engine — any bump is a core-runtime change, and REVIEW.md's situational guidance explicitly calls out "Dependencies & vendoring" for extra care. The description says the delta is a single narrowly-scoped commit, and recent main history shows WebKit bumps land routinely, but the actual C++ diff lives in a separate repository that this review cannot inspect. The test itself is moderate scrutiny: it's new code, but it's test-only and follows the harness conventions closely.
Other factors
All four prior inline threads (three CodeRabbit, one from my earlier pass) are resolved and the current diff reflects each fix: the regression test is present in this PR; the trailing partial stderr line is now scanned; Inspector.initialized rejections route through failAll; the skipIf(isCI && isASAN) gate and its stale expectations.txt citation are removed and the header comment now correctly cites test/no-validate-exceptions.txt. The test includes a premise self-check via isolatedModuleCacheSourceType (which I confirmed is exported from src/js/internal-for-testing.ts and already used by test/cli/test/isolation.test.ts), so the --isolate case can't silently degrade into testing the same provider type as the baseline. Given the engine-level dependency change, deferring to a human for the final sign-off rather than auto-approving.
There was a problem hiding this comment.
I re-reviewed after the latest push and didn't find any bugs — the earlier feedback (the skipIf(isCI && isASAN) justification, the trailing-stderr-line scan, and routing Inspector.initialized rejection through failAll) has all been addressed. Since this bumps the JavaScriptCore pin and adds a ~250-line inspector-protocol test, a human sign-off is still worthwhile.
What was reviewed:
- Confirmed
isolatedModuleCacheSourceTypeexists inbun:internal-for-testingand is already used bytest/cli/test/isolation.test.ts, so the fixture's premise self-check is wired to a real export. - Checked the test for the usual harness pitfalls: stdout/stderr are both drained concurrently,
error/closereject every pending waiter viafailAll, cleanup isawait using+finally, andport: 0is used. - Verified the header comment now points at
test/no-validate-exceptions.txtand the unjustifiedskipIf(isCI && isASAN)gate is gone.
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION one commit forward on the oven-sh/WebKit fork (oven-sh/WebKit#405 adds a BunTranspiledModule arm next to Module in JSC's DebuggerParseData/CachedTypes/Completion switches), and test/cli/inspect/debugger-buntranspiledmodule.test.ts is a new 251-line regression test that spawns bun test --isolate under --inspect-wait, connects over WebSocket, drives the CDP protocol, and asserts Debugger.setBreakpoint / Debugger.setBreakpointByUrl resolve and scriptParsed reports scriptType: "module".
Security risks
None identified. The version pin is to a commit on Bun's own WebKit fork; the test spawns a local subprocess bound to 127.0.0.1:0 and connects to it — no external network, no auth/crypto surface.
Level of scrutiny
Medium-high. The one-line pin change is mechanically trivial, but it swaps the JS engine build, so the actual behavioral surface is whatever oven-sh/WebKit#405 changed — that's outside this repo and I can't diff it here. The new test is substantial (~250 lines of hand-rolled CDP client) and touches an area (inspector protocol over WebSocket) that has historically been flaky enough to warrant quarantine entries. Both of those argue for a human maintainer to confirm the WebKit-side change is exactly what the description says and that CI is green across lanes.
Other factors
All three CodeRabbit inline comments and my prior inline finding are resolved in the current revision: the test now scans the trailing partial stderr line, Inspector.initialized rejection flows into failAll, and the file-header comment correctly cites test/no-validate-exceptions.txt with the skipIf(isCI && isASAN) gate removed. The test follows harness conventions well (tempDir, bunEnv/bunExe, await using for the subprocess, concurrent pipe draining, test.concurrent, full-object toEqual assertions so a CDP error reply shows in the diff, and a premise self-check that --isolate actually produces a BunTranspiledModule provider). The bug-hunting pass found nothing this run. I'm deferring rather than approving only because engine-dependency bumps are the kind of change a maintainer should sign off on directly.
|
Updated 4:45 PM PT - Aug 11th, 2026
❌ @Jarred-Sumner, your commit c3db033 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 37352That installs a local version of the PR into your bun-37352 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/cli/inspect/debugger-buntranspiledmodule.test.ts (2)
125-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate CDP parsing and protocol errors.
If
JSON.parsethrows in the message listener, the error does not reachfailAll, so pendingsend()andwaitForEvent()promises remain unresolved. Also,w.resolve(msg)treats a CDP response with anerrorfield as success. Therefore, setup failures andInspector.initializedfailures can bypass the catches and leave the test waiting forDebugger.paused.Wrap parsing and dispatch in
try/catchand route failures throughfailAll. Reject or explicitly fail on CDPerrorresponses while preserving the original response as error context.As per coding guidelines, “Propagate actual errors through typed channels” and “Every error, abort, and timeout path must complete the operation.”
Also applies to: 161-170
🤖 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/cli/inspect/debugger-buntranspiledmodule.test.ts` around lines 125 - 154, Update the WebSocket message listener and pending-response handling around send, eventWaiters, and failAll to catch JSON parsing or dispatch exceptions and route them through failAll so every pending promise settles. Treat CDP responses containing an error field as failures by rejecting the corresponding pending request while preserving the original response as error context, including setup and Inspector.initialized failures.Source: Coding guidelines
39-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert successful inspectee completion.
The helper starts stdout and stderr readers but does not retain or await their promises. It also never awaits or asserts
proc.exited.await using procprovides cleanup, not a success assertion. A failure afterDebugger.resumecan therefore leave both regression tests green. The blanket catch onDebugger.resumehides another post-assertion failure.Retain the drain promises, propagate reader errors, await
Debugger.resume, then await the readers and assert the child exit result last.As per coding guidelines, “Subprocess tests must drain stdout, stderr, and process exit concurrently and assert the combined result and ordered stage outputs” and “Never swallow failures or signal success after failure.”
Also applies to: 91-97, 244-244
🤖 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/cli/inspect/debugger-buntranspiledmodule.test.ts` around lines 39 - 51, Update the inspectee subprocess flow around the spawn call and its related reader setup to retain stdout/stderr drain promises, propagate reader failures, and await Debugger.resume without swallowing errors. Await stdout, stderr, and proc.exited concurrently, then assert the combined successful exit result and expected ordered stage outputs last; apply the same change to the other referenced subprocess paths.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/cli/inspect/debugger-buntranspiledmodule.test.ts`:
- Around line 125-154: Update the WebSocket message listener and
pending-response handling around send, eventWaiters, and failAll to catch JSON
parsing or dispatch exceptions and route them through failAll so every pending
promise settles. Treat CDP responses containing an error field as failures by
rejecting the corresponding pending request while preserving the original
response as error context, including setup and Inspector.initialized failures.
- Around line 39-51: Update the inspectee subprocess flow around the spawn call
and its related reader setup to retain stdout/stderr drain promises, propagate
reader failures, and await Debugger.resume without swallowing errors. Await
stdout, stderr, and proc.exited concurrently, then assert the combined
successful exit result and expected ordered stage outputs last; apply the same
change to the other referenced subprocess paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7dfc497-2774-4633-8c84-ba3b4a689248
📒 Files selected for processing (1)
test/cli/inspect/debugger-buntranspiledmodule.test.ts
There was a problem hiding this comment.
LGTM — the earlier note about the skipIf(isCI && isASAN) / stale expectations.txt reference is addressed: the skip is gone, the child strips BUN_JSC_validateExceptionChecks instead, and the header comment now points at test/no-validate-exceptions.txt. CodeRabbit's two test nits (final-partial stderr line, Inspector.initialized error routing) are also fixed.
What was reviewed
WEBKIT_VERSIONbump: single fork commit (oven-sh/WebKit#405) addingBunTranspiledModulearms; no other pin references to update.- New test:
inspecteeEnvcorrectly strips the validate-exception env vars via rest-destructure ofbunEnv(which spreadsprocess.env);isolatedModuleCacheSourceTypeexists ininternal-for-testing.ts. - Failure paths: WebSocket
error/closeandInspector.initializedrejection all route throughfailAll, so pending waiters reject rather than hang.
Extended reasoning...
Overview
Two-file change: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from 447082ab6897 to 723cea6c8c6c (one commit on oven-sh/WebKit adding a BunTranspiledModule arm next to Module in DebuggerParseData, CachedTypes, and Completion), and a new 261-line regression test at test/cli/inspect/debugger-buntranspiledmodule.test.ts that drives bun test --isolate under --inspect-wait, connects over WebSocket, and asserts Debugger.scriptParsed reports scriptType: "module" and both Debugger.setBreakpoint / setBreakpointByUrl resolve. A non---isolate baseline runs alongside.
Deltas since the prior review
My earlier inline comment flagged that the skipIf(isCI && isASAN) gate cited a nonexistent test/expectations.txt entry. Commit 13d78dda removed the skip entirely and instead strips BUN_JSC_validateExceptionChecks / BUN_JSC_dumpSimulatedThrows from the spawned child's env — the same treatment test/no-validate-exceptions.txt gives inspect.test.ts, applied at the child level so the outer test file still runs with full validation. The header comment now correctly references no-validate-exceptions.txt and states the actual mechanism (InjectedScript's unchecked exception scopes abort the child under validateExceptionChecks). The two CodeRabbit nits (scan the final unterminated stderr line; route Inspector.initialized rejection through failAll) are also applied. All four prior review threads are resolved.
Security risks
None. The runtime change is on the WebKit side (a switch-arm addition in the fork); the Bun-side change is a version-pin string plus test code. The test binds the inspector to 127.0.0.1:0 and connects locally; no external network, no auth/crypto/permissions surface.
Level of scrutiny
WebKit bumps warrant care, but this one is a single controlled commit on Bun's own fork with a narrowly-scoped, testable behavioral fix, and the PR ships the regression test that fails on the old pin. The test is additive — no existing coverage is weakened. I verified isolatedModuleCacheSourceType is exported from src/js/internal-for-testing.ts (backed by IsolatedModuleCache.cpp), that bunEnv spreads process.env so the destructure actually strips the ASAN-lane env vars, and that the no-validate-exceptions.txt reference in the new comment resolves.
Other factors
The test follows harness conventions well: tempDir + await using proc, test.concurrent for two independent subprocess cases, stderr/stdout drained concurrently so the child can't back-pressure, every failure event wired to reject the awaited promise, and the premise self-checked via bun:internal-for-testing so the --isolate case can't silently degrade to Module-vs-Module. The full CDP reply objects are asserted with toEqual, so an error field or wasThrown: true would surface in the diff rather than collapse to a vague mismatch.
There was a problem hiding this comment.
Re-reviewed after the latest push — no issues found. The skipIf(isCI && isASAN) gate and stale expectations.txt reference I flagged earlier are gone, and the CodeRabbit points (trailing stderr line, Inspector.initialized → failAll) are addressed. Since this is a WebKit prebuilt bump plus removal of two validateExceptionChecks workarounds, a maintainer should still confirm the ASAN/cross-platform CI lanes before merging.
What was reviewed:
- New test's CDP flow: every awaited promise (URL scan,
send,waitForEvent) rejects on socket error/close viafailAll; both stdout and stderr are drained so the--inspect-waitchild can't back-pressure. isolatedModuleCacheSourceTypefrombun:internal-for-testingexists (src/js/internal-for-testing.ts,IsolatedModuleCache.cpp), so the fixture import resolves.inspector.test.tsreverts to plainbunEnvandinspect.test.tsis dropped fromno-validate-exceptions.txt— consistent with oven-sh/WebKit#406 fixing the underlying unchecked-exception scopes.
Extended reasoning...
Overview
The PR bumps WEBKIT_VERSION from 447082ab6897 to 3997b59485da (two fork commits: BunTranspiledModule handling in JSC's debugger switches, and exception-check fixes on inspector pause/evaluate paths). Alongside the pin it adds a 246-line regression test driving bun test --isolate under --inspect-wait over the CDP WebSocket, restores bunEnv for four child spawns in test/js/node/inspector/inspector.test.ts (removing the injectedScriptChildEnv that stripped BUN_JSC_validateExceptionChecks), and removes test/cli/inspect/inspect.test.ts from test/no-validate-exceptions.txt.
Changes since my prior review
My earlier inline comment flagged that the new test's skipIf(isCI && isASAN) was justified by a nonexistent test/expectations.txt entry. The current revision drops both the skip and the stale comment entirely — the test now runs on all lanes. It also picks up the CodeRabbit fixes (final partial stderr line is scanned before rejecting; Inspector.initialized failures route through failAll) and bumps WebKit again to 3997b59485da so those workarounds could be removed rather than re-justified.
Security risks
None identified. The pin change fetches a prebuilt from oven-sh/WebKit releases (same channel as before). The new test binds the inspector to 127.0.0.1:0 and talks to it over a loopback WebSocket; no external network, no auth/crypto surface touched.
Level of scrutiny
High. The WebKit hash is the JS engine for every platform build. Even a two-commit fork bump changes the prebuilt tarball on every os/arch/asan/debug combination, and the PR simultaneously removes two validateExceptionChecks suppressions on the strength of oven-sh/WebKit#406. If that fix is incomplete on any platform, the ASAN lane will regress. That's exactly the kind of thing a maintainer should eyeball on the BuildKite matrix before merging — not something an automated pass can certify.
Other factors
The new test is well-constructed: it self-checks its own premise via isolatedModuleCacheSourceType (so a future refactor that stops tagging the entrypoint BunTranspiledModule would fail the --isolate case rather than silently comparing Module-vs-Module), asserts full CDP reply objects so a protocol error field is visible in the diff, wires every failure event to reject the awaited promise, and drains both pipes concurrently. I confirmed isolatedModuleCacheSourceType is exported from src/js/internal-for-testing.ts and implemented in src/jsc/bindings/IsolatedModuleCache.cpp. The bug-hunting pass found nothing this run. Deferring solely because dependency bumps to the engine itself fall outside the auto-approval bar.
There was a problem hiding this comment.
I reviewed this revision and found no bugs — the earlier note about the stale skipIf(isCI && isASAN) / test/expectations.txt reference has been addressed (the gate and comment are gone), and the CodeRabbit points on the trailing stderr line and Inspector.initialized error routing are also fixed. Leaving final sign-off to a human since this is a WebKit engine bump.
What was reviewed:
- New
debugger-buntranspiledmodule.test.ts: error/close wired intofailAll, both pipes drained,await usingcleanup, port 0,isolatedModuleCacheSourceTypeexists inbun:internal-for-testing. ObjectPrototypeInlines.hinclude swap matches the describedALWAYS_INLINEchange forobjectPrototypeToString.injectedScriptChildEnvremoval and theno-validate-exceptions.txtdeletion are consistent with the WebKit-side exception-check fix.
Extended reasoning...
Overview
Five files: the WEBKIT_VERSION pin in scripts/build/deps/webkit.ts moves three commits forward on oven-sh/WebKit; NodeUtilTypesModule.cpp swaps ObjectPrototype.h → ObjectPrototypeInlines.h so objectPrototypeToString (now ALWAYS_INLINE again upstream) is available at its two call sites in this TU; a new 246-line inspector regression test drives bun test --isolate under --inspect-wait and asserts Debugger.scriptParsed reports scriptType: "module" and both setBreakpoint/setBreakpointByUrl resolve for a BunTranspiledModule provider; inspector.test.ts drops the injectedScriptChildEnv workaround (four spawns revert to plain bunEnv); and inspect.test.ts is removed from no-validate-exceptions.txt.
Security risks
None identified. No auth, crypto, or permission surface is touched. The new test only spawns the debug binary against a temp dir over a loopback WebSocket.
Level of scrutiny
High. A WebKit pin bump changes the JS engine underneath every code path in Bun, and the correctness signal is CI across all platform/ABI lanes rather than diff inspection. The Bun-side edits themselves are small and mechanical, but the dependency change is not something an automated pass should approve on its own — a human should confirm the full CI matrix (build #92424 was still running at the time of the last robobun update) before merging.
Other factors
All three prior inline threads are resolved in the current diff: my earlier note about the stale test/expectations.txt citation no longer applies (no skipIf/isASAN remains in the new test), the trailing partial-line URL scan is present, and Inspector.initialized rejections now route through failAll. The new test follows harness conventions (tempDir, bunEnv, await using proc, concurrent pipe drains, test.concurrent, port 0) and self-checks its premise via isolatedModuleCacheSourceType so a future refactor that stops tagging the entrypoint BunTranspiledModule would visibly fail rather than silently pass. I confirmed isolatedModuleCacheSourceType is exported from src/js/internal-for-testing.ts and implemented in IsolatedModuleCache.cpp.
There was a problem hiding this comment.
Thanks for addressing the earlier feedback — the skipIf(isCI && isASAN) gate and stale expectations.txt reference are gone, and the CodeRabbit points (final-line stderr scan, Inspector.initialized failure routing) are handled. I didn't find any issues in this revision. Since this is a WebKit engine bump that changes debugger/inspector behavior and removes two validateExceptionChecks workarounds, a human should still confirm the cross-platform CI results before landing.
What was reviewed:
- New
debugger-buntranspiledmodule.test.ts: failure paths wire intofailAll/urlReject, both pipes drained, self-checks the BunTranspiledModule premise viabun:internal-for-testing. ObjectPrototypeInlines.hinclude swap — only out-of-lineobjectPrototypeToStringcaller insrc/.- Removal of
injectedScriptChildEnvand theno-validate-exceptions.txtentry match the described WebKit#406 fix.
Extended reasoning...
Overview
Bumps the pinned oven-sh/WebKit commit from 447082ab6897 to 3997b59485da (three fork commits: BunTranspiledModule debugger switch arms, inspector exception checks, and ALWAYS_INLINE restoration). Bun-side changes: the version pin, one header include (ObjectPrototype.h → ObjectPrototypeInlines.h), a new ~250-line inspector regression test, removal of the injectedScriptChildEnv workaround in inspector.test.ts, and removal of inspect.test.ts from no-validate-exceptions.txt.
Security risks
None identified. The changes are debugger/inspector-facing and a build-level include; no auth, crypto, or untrusted-input parsing is touched on the Bun side.
Level of scrutiny
High. WebKit is the JS engine — any bump can affect every execution path. The three fork PRs were presumably reviewed in oven-sh/WebKit, but the combined effect (particularly removing validateExceptionChecks workarounds and changing how the ASAN lane runs inspector tests) should be verified against full cross-platform CI by a human before merge.
Other factors
All prior review feedback on this PR is resolved: my earlier note about the stale test/expectations.txt citation and the unjustified skipIf(isCI && isASAN) was addressed by dropping both; CodeRabbit's points about scanning the trailing stderr line and routing the Inspector.initialized rejection through failAll are both implemented. The new test is well-structured (uses tempDir, bunEnv, drains both pipes concurrently, wires every socket/process failure to reject pending waiters, and self-checks its premise via isolatedModuleCacheSourceType). I confirmed isolatedModuleCacheSourceType exists in src/js/internal-for-testing.ts and IsolatedModuleCache.cpp. Build #92547 is still running per robobun; the outcome there is the main remaining signal.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. My earlier note about the stale skipIf(isCI && isASAN) / expectations.txt reference has been addressed — thanks. Because this is a WebKit engine bump (and the pin has moved to 09e4777447, past the 3997b59485da the description covers), a human look is still worthwhile.
What was reviewed:
- The new
debugger-buntranspiledmodule.test.ts— failure paths (WS close/error,Inspector.initializedrejection, trailing stderr line) all route tofailAll/urlReject; both stdout and stderr are drained concurrently. - The
ObjectPrototypeInlines.hinclude swap —objectPrototypeToStringis used at two call sites in this file; no other out-of-line users elsewhere insrc/. - Removal of the
injectedScriptChildEnvworkaround and theno-validate-exceptions.txtentry line up with the WebKit#406 fix described.
Extended reasoning...
Overview
This PR bumps the pinned WebKit commit in scripts/build/deps/webkit.ts, adds a one-line include change in NodeUtilTypesModule.cpp (needed because WebKit#403 restored __always_inline__ and objectPrototypeToString now lives in the Inlines header), adds a new inspector regression test for BunTranspiledModule breakpoint resolution, and removes two workarounds (injectedScriptChildEnv in inspector.test.ts and the inspect.test.ts entry in no-validate-exceptions.txt) that WebKit#406's exception-check fixes make unnecessary.
Since my previous inline comment, the author dropped the skipIf(isCI && isASAN) gate and the stale test/expectations.txt citation from the new test file — that concern is resolved. The CodeRabbit suggestions (scan the trailing stderr line, route Inspector.initialized rejection to failAll, add the regression test in this PR) were also all applied.
Security risks
None identified. This is a dependency version bump plus test/workaround changes; no auth, crypto, or input-parsing surface is touched on the bun side.
Level of scrutiny
High — WebKit is the JavaScript engine, so any bump is production-critical and gated primarily by CI. The bun-side edits are small and mechanical, but the PR title/description reference 3997b59485da while the actual pin landed at 09e477744721 after later commits ("bump webkit again", "Update webkit.ts"). A human should confirm what the additional fork commits between those two hashes contain and that CI is green across all lanes (including the ASAN/validateExceptionChecks lane the workaround removals rely on).
Other factors
The new test is well-structured: it self-checks its premise via isolatedModuleCacheSourceType so it can't silently go vacuous, uses test.concurrent, drains both pipes, wires every WS/process failure into promise rejections, and asserts full CDP reply objects rather than cherry-picked fields. The inspector.test.ts change is a pure revert to bunEnv. Given the engine-level scope and the pin drift past the described range, deferring to a human reviewer.
3 participants
447082ab6897→3997b59485dais three commits on the fork:DebuggerParseData,CachedTypesandCompletionswitches get aBunTranspiledModulearm next toModule. Runtime ESM that Bun hands to JSC with a prebuilt module record (bun test --isolate/--parallel,bun build --compileoutput) uses that source type, andgatherDebuggerParseDataForSourcereturned false for it, soDebugger.setBreakpointreplied "Could not resolve breakpoint" andDebugger.setBreakpointByUrlreturned no locations for those files.evaluateOnCallFrame/Runtime.evaluatepaths (JSJavaScriptCallFrame,JSInjectedScriptHost,jsToInspectorValue). UndervalidateExceptionChecks(the ASAN lane) a paused inspectee used to abort atJSJavaScriptCallFrame::scopeChain, which is whyinspector.test.tsstripped the flag for its children andinspect.test.tssat inno-validate-exceptions.txt. Both workarounds are removed here; all oftest/cli/inspect/*andtest/js/node/inspector/inspector.test.tspass locally with the flag forced on for every child.ALWAYS_INLINEis__always_inline__again in release builds, soNodeUtilTypesModule.cppnow includesObjectPrototypeInlines.hforobjectPrototypeToString(the only out-of-line use that relied on the old stopgap; release links locally against the new tarball).test/cli/inspect/debugger-buntranspiledmodule.test.ts(from #35754, updated for thescriptTypeparam the Aug 2 upgrade introduced) drivesbun test --isolateunder--inspect-waitand asserts both breakpoint calls resolve; it fails on447082aband passes here. #35605 makes everybun runmodule take theBunTranspiledModulepath; splitting the bump out so it can land first. Supersedes #35754.