Skip to content

jsc: treat BunTranspiledModule as Module in JSC debugger/inspector switches - #35754

Closed
robobun wants to merge 8 commits into
mainfrom
farm/ee589aa8/jsc-buntranspiledmodule-debugger
Closed

jsc: treat BunTranspiledModule as Module in JSC debugger/inspector switches#35754
robobun wants to merge 8 commits into
mainfrom
farm/ee589aa8/jsc-buntranspiledmodule-debugger

Conversation

@robobun

@robobun robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

bun test --isolate (and bun build --compile standalone executables) attach a pre-computed module record to each ESM SourceProvider and tag it SourceProviderSourceType::BunTranspiledModule so JSC's JSModuleLoader can skip re-parsing. Four sourceType() branch sites in JSC only match ::Module, so a BunTranspiledModule provider falls through:

$ bun --inspect-wait=0 test --isolate x.test.ts   # + Debugger.enable over WebSocket
Debugger.scriptParsed: { url: ".../x.test.ts", module: false }
Debugger.setBreakpoint: { error: { message: "Could not resolve breakpoint" } }

The same file without --isolate (plain ::Module provider) reports module: true and the breakpoint resolves. #35605 would route every runtime ESM provider through this type and is blocked on this.

Cause

  • debugger/DebuggerParseData.cpp gatherDebuggerParseDataForSource: falls to default: return false, Debugger::resolveBreakpoint gets empty pause positions, Debugger.setBreakpoint replies "Could not resolve breakpoint" for every line and Debugger.setBreakpointByUrl silently returns locations: [].
  • inspector/agents/InspectorDebuggerAgent.cpp didParseSource: Debugger.scriptParsed sends module: false instead of true.
  • runtime/CachedTypes.cpp CachedSourceProvider::encode/decode: would RELEASE_ASSERT_NOT_REACHED() if JSC's bytecode cache ever encoded one of these providers.
  • runtime/Completion.cpp getSourceType: returns ScriptFetchParameters::Type::None instead of JavaScript.

Fix

oven-sh/WebKit#345 adds a USE(BUN_JSC_ADDITIONS)-gated case SourceProviderSourceType::BunTranspiledModule: at each site that falls through to the existing Module arm. These are the only four == ::Module / case ::Module: comparison sites on the enum; the remaining references construct providers of that type (jsc.cpp, JSScript.mm, WebCore) or already handle it (ModuleProgramExecutable.cpp, JSModuleLoader.cpp).

This PR bumps WEBKIT_VERSION to the autobuild-preview-pr-345-64b7e437 preview build and adds test/cli/inspect/debugger-buntranspiledmodule.test.ts that drives bun test --isolate under --inspect-wait, asserts Debugger.scriptParsed reports module: true, and asserts Debugger.setBreakpoint / Debugger.setBreakpointByUrl resolve on the user's ESM source. A paired no---isolate case pins the baseline.

Also in this range (549170099226..64b7e4374ab0)

  • oven-sh/WebKit#328 inspector: release throw scope before tail-calling impl in injected-script prototype host functions
  • oven-sh/WebKit#317 LiteralParser: throw RangeError on OOM when copying a JSON string value
  • oven-sh/WebKit#331 SignalsWin: fix VEH return value and register behind AddressSanitizer's handler
  • oven-sh/WebKit#332 Heap: make minEdenToOldGenerationRatio a JSC option (same default; behaviour unchanged)

Verification

$ USE_SYSTEM_BUN=1 bun test test/cli/inspect/debugger-buntranspiledmodule.test.ts
(fail) ... --isolate ...           # module:false, "Could not resolve breakpoint"
(pass) ... (no --isolate) ...

$ bun bd test test/cli/inspect/debugger-buntranspiledmodule.test.ts
(pass) ... --isolate ...
(pass) ... (no --isolate) ...

Also ran isolation.test.ts, test-reporter.test.ts, node/inspector/inspector.test.ts and regression/issue/21654 with no failures.

Once oven-sh/WebKit#345 merges, WEBKIT_VERSION should be repointed at the merged main sha (the preview release is deleted at that point).

Unblocks #35605.


[decide:webkit] gate passed · iteration 6 · 2 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/cli/inspect/debugger-buntranspiledmodule.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/cli/inspect/debugger-buntranspiledmodule.test.ts
bun test v1.4.0 (0197616f0)

test/cli/inspect/debugger-buntranspiledmodule.test.ts:
(pass) bun test (no --isolate): Debugger.scriptParsed reports module and breakpoints resolve [1503.21ms]
(pass) bun test --isolate: Debugger.scriptParsed reports module and breakpoints resolve [1605.48ms]

 2 pass
 0 fail
 6 expect() calls
Ran 2 tests across 1 file. [3.98s]
Exit: 0
diff hotspot
scripts/build/deps/webkit.ts                       |   4 +-
 .../inspect/debugger-buntranspiledmodule.test.ts   | 255 +++++++++++++++++++++
 2 files changed, 258 insertions(+), 1 deletion(-)

gate history · 6 passed · 1 rejected · iteration 6

evidence per changed file
file                                                   reads  edits  tests
scripts/build/deps/webkit.ts                               1      1      0
test/cli/inspect/debugger-buntranspiledmodule.test.ts      4     12      0

…itches

SourceProviderSourceType::BunTranspiledModule (added for #15758)
is missing from four sourceType() switch/compare sites in JSC, so a provider
with that type (today: bun test --isolate and bun build --compile output)
falls through:

- gatherDebuggerParseDataForSource returns false, Debugger.setBreakpoint
  replies "Could not resolve breakpoint" for every line in the script
- InspectorDebuggerAgent::didParseSource sends module:false in
  Debugger.scriptParsed
- CachedSourceProvider encode/decode would RELEASE_ASSERT_NOT_REACHED()
- Completion.cpp getSourceType returns Type::None instead of JavaScript

Bumps WEBKIT_VERSION to the oven-sh/WebKit#345 preview, which adds a
BUN_JSC_ADDITIONS-gated fall-through to the Module arm at each site, and adds
a test/cli/inspect/inspect.test.ts case that drives bun test --isolate under
--inspect-wait, asserts Debugger.scriptParsed reports module:true, and asserts
Debugger.setBreakpoint / setBreakpointByUrl resolve on the user's ESM source.

Also pulled in by this bump (549170099226..64b7e4374ab0):
- oven-sh/WebKit#328 inspector: release throw scope before tail-calling impl in injected-script prototype host functions
- oven-sh/WebKit#317 LiteralParser: throw RangeError on OOM when copying a JSON string value
- oven-sh/WebKit#331 SignalsWin: fix VEH return value and register behind AddressSanitizer's handler
- oven-sh/WebKit#332 Heap: make minEdenToOldGenerationRatio a JSC option

Unblocks #35605.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

WebKit preview dependency

Layer / File(s) Summary
Update WebKit preview version
scripts/build/deps/webkit.ts
Changes WEBKIT_VERSION to autobuild-preview-pr-345-64b7e437, updating derived download and cache identifiers.

Debugger inspector regression coverage

Layer / File(s) Summary
Add debugger inspector probe and regression cases
test/cli/inspect/debugger-buntranspiledmodule.test.ts
Adds an inspector-driven probe and concurrent tests comparing BunTranspiledModule behavior under --isolate with baseline Module behavior.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately reflects the JSC behavior change targeted by the WebKit version bump.
Description check ✅ Passed The description is detailed and includes the change and verification, though it uses different headings than the template.

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. Breakpoints inside Vite SSR modules aren't triggered #17366 - Breakpoints in Vite SSR modules use BunTranspiledModule source providers; the debugger didn't recognize them as modules, so breakpoints couldn't resolve
  2. bun build / single file executable - remote debugger #20253 - Standalone executables (bun build --compile) use BunTranspiledModule source providers, making the debugger unable to establish working breakpoints

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #17366
Fixes #20253

🤖 Generated with Claude Code

@claude claude 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.

I didn't find any bugs, but this is a WebKit version bump that pulls in five upstream JSC changes and is pinned to a preview release tag (autobuild-preview-pr-345-64b7e437) that the description notes will be deleted once oven-sh/WebKit#345 merges — a maintainer should sign off on landing against a preview tag vs. waiting for the merged sha.

What was reviewed:

  • prebuiltUrl/prebuiltDestDir handle the autobuild- prefix already, so the non-hex tag resolves correctly.
  • New inspect.test.ts cases: stderr/stdout drained concurrently, error/close wired to reject pending waiters, --inspect-wait on port 0, baseline no---isolate case pins the control.
  • Checked that Debugger.scriptParsed for mod.test.ts must arrive before Debugger.paused (script is parsed before the debugger; statement executes), so userScript is populated by the time it's read.
Extended reasoning...

Overview

Two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from a main-branch sha to autobuild-preview-pr-345-64b7e437, and test/cli/inspect/inspect.test.ts gains a ~200-line describe block driving bun test --isolate under --inspect-wait over a WebSocket to assert Debugger.scriptParsed reports module: true and that Debugger.setBreakpoint/setBreakpointByUrl resolve for a BunTranspiledModule source provider.

The actual fix lives in oven-sh/WebKit#345 (four case SourceProviderSourceType::BunTranspiledModule: fall-throughs added under USE(BUN_JSC_ADDITIONS)). The bump also picks up four other WebKit PRs in the range (#328 throw-scope release, #317 LiteralParser OOM RangeError, #331 SignalsWin VEH, #332 Heap option).

Security risks

None specific to this diff. The test spawns a local subprocess with --inspect-wait on 127.0.0.1:0 and connects to it — no external network, no new attack surface exposed. The WebKit changes themselves are in a separate repo and not reviewable here.

Level of scrutiny

High. A WebKit version bump changes the JS engine binary for every platform/ABI, and the range includes five distinct upstream changes (not just the one this PR is about). The pin is to a preview release tag rather than a merged main commit, and the PR description explicitly says the preview release is deleted once the WebKit PR merges — so merging this creates a window where a fresh clone can't fetch the pinned WebKit until someone repoints. That's a maintainer call, not something to auto-approve.

Other factors

The test itself looks solid against the repo's review checklist: it uses tempDir, bunEnv, port 0, drains both pipes in the background so the child can't back-pressure, buffers stderr to line boundaries before URL-parsing, wires error/close into every pending promise via failAll, and includes a no---isolate baseline so a regression in the plain Module path would also be caught. The send("Inspector.initialized").catch(() => {}) fire-and-forget is intentional — the response may race with the pause event and failAll on close is expected there. Verification section shows the --isolate case fails under USE_SYSTEM_BUN=1 and passes on the debug build.

Given the dep-bump scope and the temporary preview tag, deferring to a human.

…le provider

The debugger probe only asserted module:true and breakpoint resolution, both of
which also hold for a plain Module provider. If a refactor stopped attaching
module_info to the --isolate entrypoint, both cases would become
Module-vs-Module and the WebKit#345 regression guard would silently evaporate.

The fixture now stashes isolatedModuleCacheSourceType(import.meta.path) on
globalThis before the debugger statement (evaluateOnCallFrame parses its
expression as a Program, so import.meta cannot be evaluated directly), and the
driver asserts it is "BunTranspiledModule" for the --isolate case and null for
the no-isolate baseline.
@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:25 AM PT - Jul 26th, 2026

@robobun, your commit 0197616 has 2 failures in Build #82245 (All Failures):

  • test/js/bun/s3/s3.test.ts - crash reported on 🐧 3.23 x64
  • test/js/bun/s3/s3.test.ts - crash reported on 🐧 3.23 aarch64
  • 📦 Binary size — 12 over 0.50 MB
  • targetthis build canary: main #79916
    sizeΔ
    bun-darwin-aarch6458.13 MB57.58 MB+564.9 KB
    bun-darwin-x6463.48 MB62.95 MB+544.5 KB
    bun-linux-aarch6470.98 MB70.42 MB+576.0 KB
    bun-linux-x6472.47 MB71.95 MB+528.0 KB
    bun-linux-aarch64-musl64.88 MB64.32 MB+576.0 KB
    bun-linux-x64-musl66.98 MB66.45 MB+544.0 KB
    bun-linux-aarch64-android78.47 MB77.97 MB+512.0 KB
    bun-linux-x64-android80.62 MB80.10 MB+529.2 KB
    bun-freebsd-x6483.07 MB82.56 MB+528.0 KB
    bun-freebsd-aarch6484.84 MB84.31 MB+544.0 KB
    bun-windows-x6480.26 MB79.70 MB+571.0 KB
    bun-windows-aarch6470.86 MB70.34 MB+533.5 KB

    Add [skip size check] to the commit message if this increase is intentional.


🧪   To try this PR locally:

bunx bun-pr 35754

That installs a local version of the PR into your bun-35754 executable, so you can run:

bun-35754 --bun

@claude claude 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.

I didn't find issues with the test (it drains pipes, wires close/error to reject, self-checks the provider type via bun:internal-for-testing, and pins a no---isolate baseline). Deferring because this bumps WEBKIT_VERSION to a temporary autobuild-preview-pr-345-* tag — the description notes the preview release is deleted once oven-sh/WebKit#345 merges, and the range also pulls in four unrelated WebKit changes (#328/#317/#331/#332). A maintainer should decide whether to land on the preview tag now or wait for the merged sha.

What was reviewed

  • prebuiltUrl / prebuiltDestDir in webkit.ts already handle autobuild--prefixed versions, so the tag resolves correctly.
  • Test: stdout/stderr drained concurrently, --inspect-wait on port 0, failure paths reject the awaited promise, Debugger.paused waiter registered before Inspector.initialized.
  • providerSourceType self-check guards against the --isolate case silently degrading to a plain Module provider.
Extended reasoning...

Overview

This PR changes two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from the pinned main sha 549170099226… to the preview tag autobuild-preview-pr-345-64b7e437, and test/cli/inspect/inspect.test.ts gains a ~220-line describe block that drives bun test --isolate under --inspect-wait, connects over WebSocket, and asserts Debugger.scriptParsed.module === true plus that Debugger.setBreakpoint / setBreakpointByUrl resolve. The actual behavioral fix lives in oven-sh/WebKit#345 (four case SourceProviderSourceType::BunTranspiledModule: fall-throughs); this repo change is the version bump + regression test.

Security risks

None identified. The test spawns a local inspector on 127.0.0.1:0, uses tempDir, and does not touch auth/crypto/permissions. The WebKit bump range includes a Windows VEH-handler change (#331) and a LiteralParser OOM change (#317) which are behavior-affecting but not security-sensitive in the injection/auth sense.

Level of scrutiny

High — WebKit is the JS engine and WEBKIT_VERSION gates every prebuilt download across all platforms. The bump is not a single-fix cherry-pick: it pulls in four other WebKit PRs, and per the description the preview release tag will be deleted once #345 merges to WebKit main. Merging this as-is means main transiently points at an artifact that will disappear, breaking fresh clones/CI cache misses until someone repoints it. That's a process/release decision a maintainer should make explicitly (land now on the preview tag vs. wait for the merged sha), not something to auto-approve.

Other factors

  • The test is well-constructed against the repo's review rules: it awaits observable conditions (URL line on stderr, Debugger.paused event), wires error/close to reject every pending waiter, drains both stdout and stderr concurrently to avoid pipe back-pressure, uses port: 0, and cleans up via using/await using + try/finally around the WebSocket.
  • The isolatedModuleCacheSourceType self-check (asserting "BunTranspiledModule" under --isolate and null without) is a good guard against the test becoming vacuous if a future refactor stops routing --isolate entrypoints through BunTranspiledModule.
  • I confirmed prebuiltUrl already special-cases autobuild- prefixes (uses the value verbatim as the release tag) and prebuiltDestDir slices past autobuild- for the cache key, so the non-sha version string works with the existing download machinery.
  • No prior reviews on the PR; nothing outstanding to address.

inspect.test.ts has pre-existing localhost-based websocket cases that are
environment-sensitive (and the file is already [ASAN] [TIMEOUT] in
test/expectations.txt); running it end to end fails on those unrelated cases.
The new cases live in debugger-buntranspiledmodule.test.ts so the regression
guard runs clean on its own.
Comment thread test/cli/inspect/debugger-buntranspiledmodule.test.ts Outdated

@claude claude 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.

No bugs found, but this is a WebKit version bump — it points at a temporary autobuild-preview-pr-345-* release (which the description notes will be deleted once oven-sh/WebKit#345 merges) and pulls in four other WebKit PRs (#317, #328, #331, #332) alongside the debugger fix. Worth a human sign-off before landing.

What was reviewed

  • The new inspector test: port-0, tempDir, pipes drained, error/close wired to reject pending requests, test.concurrent, and the isolatedModuleCacheSourceType self-check that pins the --isolate case to an actual BunTranspiledModule provider — all look solid.
  • prebuiltUrl/prebuiltDestDir in scripts/build/deps/webkit.ts already handle autobuild-* tags, so the preview version string resolves correctly.
Extended reasoning...

Overview

Two files: scripts/build/deps/webkit.ts (repoints WEBKIT_VERSION from a main-branch sha to the preview tag autobuild-preview-pr-345-64b7e437) and a new ~225-line test at test/cli/inspect/debugger-buntranspiledmodule.test.ts. The behavioral fix itself lives in oven-sh/WebKit#345 — this PR is the version bump plus the regression test that proves it.

Security risks

None identified. The change is a JSC debugger/inspector switch-case fallthrough fix in the WebKit fork; the test drives a local 127.0.0.1:0 inspector WebSocket against a spawned child. No auth, crypto, or untrusted-input parsing is touched in this repo.

Level of scrutiny

High. WEBKIT_VERSION controls which JavaScriptCore build every Bun binary links against. The bump range (549170099226..64b7e4374ab0) carries four unrelated WebKit changes (LiteralParser OOM, injected-script throw-scope, Windows VEH/ASAN, a new Heap JSC option) in addition to the targeted fix — those need a maintainer's eye even if each is small. The version is also explicitly temporary: the PR description says the preview release is deleted once the upstream PR merges, so a human should decide whether to land against the preview tag now or wait for the merged sha.

Other factors

The test is well-constructed for this repo's conventions: it awaits real observable conditions (stderr line for the inspector URL, Debugger.paused event), rejects all pending waiters on WebSocket error/close, drains stdout to avoid backpressure, self-checks that --isolate actually produced a BunTranspiledModule provider via bun:internal-for-testing, and pins the no---isolate baseline alongside. My earlier test.concurrent nit was applied in 639852a. I don't see anything blocking in the test itself; the deferral is purely because engine bumps and temporary dependency pins are outside the auto-approve envelope.

@claude claude 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.

No bugs found in the test itself, but this is a WebKit version bump — it repoints the JS engine to an ephemeral autobuild-preview-pr-345-* tag (the description notes it must be repointed to a main sha before/after oven-sh/WebKit#345 merges, since the preview release is deleted) and pulls in four additional WebKit changes (#328, #317, #331, #332) beyond the one under test. That plus the unresolved CI status on Build #81417 warrants a maintainer sign-off.

What was reviewed:

  • The new inspector probe test — port-0 inspector, stderr/stdout drained concurrently, error/close wired to reject, self-check that --isolate actually yields a BunTranspiledModule provider, exact-shape assertions on setBreakpoint/setBreakpointByUrl. The earlier test.concurrent nit was applied in 639852a.
  • prebuiltUrl/prebuiltDestDir in scripts/build/deps/webkit.ts — both already handle autobuild-* version strings, so the preview tag resolves and caches correctly.
  • Checked that isolatedModuleCacheSourceType is exported from bun:internal-for-testing so the fixture import resolves.
Extended reasoning...

Overview

This PR is the bun-side half of a fix that lives in oven-sh/WebKit#345: four sourceType() switch sites in JSC (DebuggerParseData, InspectorDebuggerAgent, CachedTypes, Completion) don't recognize SourceProviderSourceType::BunTranspiledModule, so ESM sources loaded via bun test --isolate or bun build --compile report module: false in Debugger.scriptParsed and reject all breakpoints. The bun repo change is (a) a one-line WEBKIT_VERSION bump to a preview build of that WebKit PR, and (b) a ~225-line inspector-protocol regression test that drives bun test --isolate under --inspect-wait and asserts the fixed behavior.

Security risks

None identified. The test binds the inspector to 127.0.0.1:0, spawns only the local debug bun, and touches no auth/crypto/permissions paths. The WebKit-side change adds fall-through case arms behind USE(BUN_JSC_ADDITIONS).

Level of scrutiny

High — not because the diff in this repo is complex, but because WEBKIT_VERSION controls which JavaScriptCore build every bun binary links against. Per the repo's own guidance, dependency bumps (especially WebKit) are a situational section in .claude/docs/landing-prs.md and warrant maintainer review. This particular bump:

  1. Points at an ephemeral preview tag (autobuild-preview-pr-345-64b7e437). The PR description itself says the preview release is deleted once oven-sh/WebKit#345 merges and WEBKIT_VERSION must then be repointed at the merged main sha. Merging this to bun main before that repoint would leave main referencing a release that can vanish.
  2. Bundles four additional WebKit PRs in the range 549170099226..64b7e4374ab0 (#328 injected-script throw-scope, #317 LiteralParser OOM RangeError, #331 SignalsWin VEH/ASAN ordering, #332 minEdenToOldGenerationRatio as a JSC option). These are listed but not exercised by this PR's test, and a maintainer should confirm they're intended to ride along.

Other factors

  • Test quality: the new test is well-constructed against REVIEW.md's checklist — tempDir/bunEnv/bunExe, port 0, concurrent stdout+stderr drain to avoid pipe back-pressure, every WebSocket error/close wired to reject pending waiters, a providerSourceType self-check so the premise (that --isolate yields a BunTranspiledModule provider) can't silently evaporate, and a paired non---isolate baseline. The prior test.concurrent nit was applied.
  • CI: robobun flagged failures in Build #81417 on 639852a; the latest commit (92f2315) is a CI retrigger with no green status posted in the timeline yet.
  • Build tooling: verified prebuiltUrl() already special-cases version.startsWith("autobuild-") and prebuiltDestDir() uses the full tag for non-sha versions, so the preview tag won't collide in the cache or produce a malformed download URL.

Given the engine-level dependency bump to a temporary tag and the outstanding CI status, deferring to a human.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

CI status (build #82245, finished 193/196 passed): the diff is green.

  • test/cli/inspect/debugger-buntranspiledmodule.test.ts (the new regression test) is not in any failure; it passed on every test lane and skips on the CI ASAN lane per the isCI && isASAN gate.
  • binary-size check: ~550 KB growth across targets vs canary #79916. Expected from the WebKit bump (5 upstream commits in the range); advisory for dep bumps.
  • test/js/bun/s3/s3.test.ts: crashed on alpine x64/aarch64 with S3Error: ServiceUnavailable from Cloudflare R2 (external service outage) plus a follow-on crash in the reporter. Unrelated to this diff; also flaky on ubuntu/debian in the same build and main builds #81770/#81444 are red. Reported for main-break triage.
  • filter-workspace.test.ts / s3.leak.test.ts / require-cache.test.ts / 20144.test.ts: passed on retry.

Earlier builds #81159/#81417/#82091 hit CI agent congestion (*-build-cpp stuck scheduled until *-build-bun timed out waiting); that has cleared.

All review threads resolved. Ready for review; WEBKIT_VERSION should be repointed at the merged oven-sh/WebKit main sha once oven-sh/WebKit#345 lands (the preview release is deleted at that point).

…s unreliable

debian-13 x64-asan in build 82091 hit "WebSocket closed (1006)" before
Debugger.paused. This is the same WebSocket-inspector-under-ASAN flakiness
that test/expectations.txt quarantines inspect.test.ts for and that
test/regression/issue/21654 skips on. The JSC switch-arm fix being tested is in
C++ and behaves identically with or without ASAN; every release lane still runs
it.

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🤖 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-59: Update the stderr decoding loop around the TextDecoder in
the debugger test to call decode with streaming enabled for each chunk,
preserving incomplete multibyte sequences across chunk boundaries before
appending to stderrBuf.
- Around line 210-212: Update the setBreakpointByUrl assertion to validate the
full CDP reply message, including its error details, instead of only
result.locations. Follow the existing setBreakpoint assertion convention nearby
and preserve the expected breakpoint location within the complete response.
- Around line 171-183: Strengthen the debugger evaluation assertions around
sourceTypeEval: require paused.callFrames[0].callFrameId before calling
Debugger.evaluateOnCallFrame, verify the reply has no CDP error and wasThrown is
false, and assert the returned result shape directly instead of collapsing
failures with ?? null. Update the combined expectation so the baseline and
isolate cases can fail when evaluation is unavailable or unsuccessful.
🪄 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: 7f893a55-a207-43cc-8209-b79f97822a00

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and e1ad695.

📒 Files selected for processing (2)
  • scripts/build/deps/webkit.ts
  • test/cli/inspect/debugger-buntranspiledmodule.test.ts

Comment thread test/cli/inspect/debugger-buntranspiledmodule.test.ts
Comment thread test/cli/inspect/debugger-buntranspiledmodule.test.ts Outdated
Comment thread test/cli/inspect/debugger-buntranspiledmodule.test.ts Outdated
- TextDecoder.decode({stream: true}) so multi-byte sequences split across
  stderr chunks don't corrupt the diagnostic buffer.
- Assert the full evaluateOnCallFrame / setBreakpoint / setBreakpointByUrl
  reply objects instead of projecting into result.value / result.locations.
  The no-isolate baseline expects the null-value CDP shape (type:object,
  subtype:null, wasThrown:false), so a CDP error or thrown evaluation can no
  longer collapse to the same null the baseline expects.
- Require paused.callFrames[0].callFrameId before evaluating.
Comment thread test/cli/inspect/debugger-buntranspiledmodule.test.ts Outdated
skipIf(isASAN) also skips under a local `bun bd` (debug profile defaults ASAN
on Linux/arm64-macOS), so the regression guard was invisible to the default
local verification workflow even though it passes there. Gate on
isCI && isASAN instead, matching test/cli/hot/watch-many-dirs.test.ts and
test/js/bun/spawn/spawn-pipe-leak.test.ts.

@claude claude 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.

I didn't find any bugs, but this is a WebKit version bump — deferring to a maintainer.

The WEBKIT_VERSION pin is to a preview release tag (autobuild-preview-pr-345-64b7e437) that, per the PR description, is deleted once oven-sh/WebKit#345 merges — so a maintainer should decide whether to land this now and track the repoint, or wait for #345 to merge first and pin to the resulting main sha. The range also carries four unrelated WebKit changes (#328 throw-scope release in injected-script host functions, #317 LiteralParser OOM RangeError, #331 Windows VEH/ASAN handler ordering, #332 minEdenToOldGenerationRatio as a JSC option) that a human should sign off on.

What was reviewed

  • The new test: awaits real inspector events (URL from stderr, Debugger.paused), wires WS error/close to reject all pending waiters, drains both pipes, uses port 0 / tempDir, and asserts full CDP reply objects so error/wasThrown surface — the earlier vacuous-baseline concern is fixed.
  • .skipIf(isCI && isASAN) matches the watch-many-dirs.test.ts / spawn-pipe-leak.test.ts pattern; local bun bd still runs it.
  • webkit.ts: only the version constant + a TODO comment changed; prebuiltUrl/prebuiltDestDir already handle autobuild-* tags.
Extended reasoning...

Overview

Two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from a main-branch sha to the preview tag autobuild-preview-pr-345-64b7e437, and test/cli/inspect/debugger-buntranspiledmodule.test.ts is a new 255-line regression test that drives bun test --isolate under --inspect-wait, connects over the WebSocket inspector protocol, and asserts that a BunTranspiledModule source provider reports module: true in Debugger.scriptParsed and that Debugger.setBreakpoint / Debugger.setBreakpointByUrl resolve. The actual behavioural fix lives in oven-sh/WebKit#345 (four USE(BUN_JSC_ADDITIONS)-gated case ::BunTranspiledModule: fall-throughs in JSC C++), which this PR only pulls in via the version bump.

Security risks

None identified. The test is hermetic (127.0.0.1, port 0, tempDir, no external hosts). The WebKit range does touch a Windows VEH handler (#331) and a JSON-parse OOM path (#317), but those are upstream C++ changes I cannot audit from this repo — which is part of why a human should look.

Level of scrutiny

High. This is a vendored-dependency bump of the JavaScript engine itself, and the repo's own guidance calls out "Dependencies & vendoring" as a section requiring extra care. Beyond the targeted fix, the bumped range carries four other WebKit PRs (#328, #317, #331, #332) whose correctness I cannot verify here. More importantly, the pin is to a preview release that the PR body says will be deleted once oven-sh/WebKit#345 merges; landing this as-is commits main to a follow-up repoint before the preview artifact disappears (otherwise fresh clones would 404 on the WebKit download). That is a release-management call for a maintainer, not something to auto-approve.

Other factors

The test file has been through two rounds of bot review (my test.concurrent nit and the over-broad isASAN gate; CodeRabbit's streaming-decode, vacuous-baseline, and full-reply-assertion comments) and all threads are resolved in the current diff. I re-checked each: the sourceTypeEval assertion now matches the full {id, result: {result, wasThrown: false}} shape so the no---isolate baseline can no longer pass on a failed evaluation; setBreakpointByUrl asserts the whole reply; TextDecoder uses {stream: true}; the skip is isCI && isASAN. The test wires WebSocket error/close into failAll so no waiter can hang, drains both stdout and stderr concurrently to avoid pipe backpressure, and uses await using proc / using dir for cleanup. The webkit.ts change is just the constant plus a comment; prebuiltUrl() already special-cases autobuild- tags and prebuiltDestDir() slices the prefix, so the preview tag flows through the existing machinery. CI is reported green on the lanes that ran; the ASAN-lane WS flake is documented and gated.

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

For anyone landing here: the WebKit side of this went in as oven-sh/WebKit#405 (merged, 723cea6c). The WEBKIT_VERSION bump is in #37352 and the full change that needs it is #35605, which also covers the plain bun run case; once those land this PR is redundant (its --isolate test would still be a valid addition on top, adjusted for the scriptType protocol field).

@alii

alii commented Aug 10, 2026

Copy link
Copy Markdown
Member

Superseded by #37352 (same test, WebKit change landed as oven-sh/WebKit#405).

@alii alii closed this Aug 10, 2026
@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed: oven-sh/WebKit#405 landed the same four switch-arm additions and #37352 carries the test from this branch against the merged sha. Nothing further needed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants