Skip to content

inspector: bump WebKit so Runtime.evaluate survives validateExceptionChecks - #35333

Open
robobun wants to merge 4 commits into
mainfrom
farm/cc5c187d/fix-inspector-injected-script-exception-scope
Open

inspector: bump WebKit so Runtime.evaluate survives validateExceptionChecks#35333
robobun wants to merge 4 commits into
mainfrom
farm/cc5c187d/fix-inspector-injected-script-exception-scope

inspector: bump WebKit so Runtime.evaluate survives validateException…

de8e181
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 23, 2026 in 14m 12s

Code review found 1 important issue

Found 5 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important scripts/build/deps/webkit.ts:13-16 WEBKIT_VERSION pinned to ephemeral preview tag
🟡 Nit test/cli/inspect/inspect.test.ts:362-365 SIGABRT stderr diagnostic is unreachable on the regression path
🟡 Nit test/cli/inspect/inspect.test.ts:323-336 urlPromise never rejects — test hangs to timeout on early inspectee exit

Annotations

Check failure on line 16 in scripts/build/deps/webkit.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

WEBKIT_VERSION pinned to ephemeral preview tag

`WEBKIT_VERSION` is pinned to `autobuild-preview-pr-328-31913c3e`, an ephemeral preview tag that GitHub deletes as soon as oven-sh/WebKit#328 merges or closes — at which point every fresh build of bun `main` 404s downloading WebKit. Before this lands, swap the pin to the merged 40-hex `oven-sh/WebKit` main sha (and drop the "Preview of…" comment).

Check warning on line 365 in test/cli/inspect/inspect.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

SIGABRT stderr diagnostic is unreachable on the regression path

The SIGABRT-stderr block at the bottom is unreachable on the regression path it targets: when the inspectee aborts, `reply` is `{closed:{code:1006,...}}`, so `expect(reply).toMatchObject(...)` throws inside the `try`, the `finally` runs, and the assertion error propagates straight out — lines 362–365 never run and the captured `stderr` (with the JSC "Unchecked JS exception ... @ JSInjectedScriptHostPrototype.cpp:NNN" pointer) is discarded. The PR's own fail-before transcript shows exactly this (

Check warning on line 336 in test/cli/inspect/inspect.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

urlPromise never rejects — test hangs to timeout on early inspectee exit

`urlPromise` is only ever resolved — `reject` is never destructured from `withResolvers()`. If the inspectee exits before printing the `ws://` line (plausible here since `BUN_JSC_validateExceptionChecks=1` is set and any unrelated startup-path exception-scope bug will SIGABRT before the inspector binds), the `for await` drain loop ends and `await urlPromise` blocks until the test-runner timeout, discarding the captured `stderr`. Destructure `reject` too and call it after the loop with the accumu