Skip to content

build: WebKit → preview-pr-181-ebf11e28#29250

Closed
Jarred-Sumner wants to merge 4 commits intomainfrom
claude/webkit-preview-pr-181
Closed

build: WebKit → preview-pr-181-ebf11e28#29250
Jarred-Sumner wants to merge 4 commits intomainfrom
claude/webkit-preview-pr-181

Conversation

@Jarred-Sumner
Copy link
Copy Markdown
Collaborator

Points the WebKit prebuilt at oven-sh/WebKit#181's preview build (autobuild-preview-pr-181-e38dd2fc).

That PR fixes Windows resource exhaustion in libpas/WTF:

  • Scavenger thread HANDLE leak (one per ~10s idle cycle)
  • FLS index dropped → TLC destructor never fired
  • OSAllocator::decommit never released commit charge (MEM_RESETMEM_DECOMMIT)
  • pas_page_malloc decommit leaked commit on the do_mprotect=false path

Baseline artifacts are present in this preview release, so no build-system workaround needed.

@robobun
Copy link
Copy Markdown
Collaborator

robobun commented Apr 13, 2026

Updated 6:24 AM PT - Apr 13th, 2026

@Jarred-Sumner, your commit 7d5ca23 has 1 failures in Build #45484 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29250

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

bun-29250 --bun

@github-actions
Copy link
Copy Markdown
Contributor

Found 6 issues this PR may fix:

  1. webkit: re-add -DUSE_SYSTEM_MALLOC=ON to Windows prebuilts #28101 - Directly addresses bmalloc/libpas bugs on Windows that motivated the request to switch to system malloc
  2. Crash in bmalloc (pas_assertion_failed) on Windows x64 baseline in standalone executable #26982 - Fixes libpas assertion failures (pas_assertion_failed) in bmalloc on Windows x64 baseline builds
  3. Segfault after ~24h idle on Windows 11 with sleep/wake cycles #28175 - Scavenger HANDLE leak and decommit fix address the 5:1 commit-charge-to-RSS ratio and crash after ~24h idle on Windows 11
  4. bun --bun run dev allocates 10Gb of memory for empty Nuxt project #16898 - Decommit fix (MEM_RESET→MEM_DECOMMIT) explains inflated 10GB commit charge on Windows for trivial workloads
  5. Blob allocations cause excessive memory, and GC does not release all memory / Windows 10 / Blob,Uint8Array,ArrayBuffer #20544 - Decommit fixes explain why GC'd Blob memory was never released back to the OS on Windows
  6. Bun 1.3.11 (integrated in Claude Code) on Windows x64 OOM crash #28174 - Unreleased commit charge from decommit bug makes OOM more likely on memory-constrained Windows systems

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

Fixes #28101
Fixes #26982
Fixes #28175
Fixes #16898
Fixes #20544
Fixes #28174

🤖 Generated with Claude Code

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

Walkthrough

The WEBKIT_VERSION constant in scripts/build/deps/webkit.ts was changed from a specific commit hash to a preview identifier (preview-pr-181-ebf11e28), altering the default WebKit tag used to compute prebuilt URLs and extraction destination unless overridden via --webkit-version.

Changes

Cohort / File(s) Summary
WebKit Version Update
scripts/build/deps/webkit.ts
Updated exported WEBKIT_VERSION constant from commit hash 42f80a684c5df57121a97e20825a3bcab7a0741b to preview identifier preview-pr-181-ebf11e28. Default prebuilt tag/destination will target artifacts for this identifier; the --webkit-version=<hash> override remains unchanged.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: updating WebKit from a commit hash to a preview build identifier (preview-pr-181-ebf11e28).
Description check ✅ Passed The description covers both required template sections: it explains what the PR does (points WebKit prebuilt to preview build with details on fixes) and implies verification through the PR objectives that show test status, though formal verification steps are not explicitly detailed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The WEBKIT_VERSION constant is missing the upstream tag prefix,
causing the git checkout in the sync logic to fail; update either the constant
or the sync checkout to normalize the ref: either set WEBKIT_VERSION to the full
ref (include the "autobuild-" prefix) or change the checkout path in the sync
code that uses WEBKIT_VERSION (the git checkout call in the
sync-webkit-source.ts checkout logic) to prepend "autobuild-" when the value
doesn't already start with that prefix so both prebuiltUrl() and the source sync
use the same ref.
🪄 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: e64a77ab-69ee-4c37-b7c0-a43d5331c7c3

📥 Commits

Reviewing files that changed from the base of the PR and between d35774f and 2c2b1ca.

📒 Files selected for processing (1)
  • scripts/build/deps/webkit.ts

* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-e38dd2fc";
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.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify which refs actually exist in oven-sh/WebKit
repo="https://github.com/oven-sh/WebKit"

echo "Checking candidate refs..."
git ls-remote --heads --tags "$repo" \
  "refs/heads/preview-pr-181-e38dd2fc" \
  "refs/tags/preview-pr-181-e38dd2fc" \
  "refs/heads/autobuild-preview-pr-181-e38dd2fc" \
  "refs/tags/autobuild-preview-pr-181-e38dd2fc"

echo
echo "Expected:"
echo "- At least one ref must match what sync-webkit-source.ts passes to 'git checkout'."
echo "- If only autobuild-* exists, current WEBKIT_VERSION may break local sync."

Repository: oven-sh/bun

Length of output: 335


🏁 Script executed:

cat -n scripts/sync-webkit-source.ts | head -30

Repository: oven-sh/bun

Length of output: 1177


🏁 Script executed:

rg -A 10 "function prebuiltUrl\|const prebuiltUrl" scripts/build/deps/webkit.ts

Repository: oven-sh/bun

Length of output: 37


🏁 Script executed:

rg -B 3 -A 3 "git checkout" scripts/sync-webkit-source.ts

Repository: oven-sh/bun

Length of output: 354


🏁 Script executed:

rg "prebuiltUrl" scripts/build/deps/webkit.ts -A 15

Repository: oven-sh/bun

Length of output: 1474


🏁 Script executed:

rg "export.*WEBKIT_VERSION\|autobuild" scripts/build/deps/webkit.ts | head -20

Repository: oven-sh/bun

Length of output: 37


git checkout in sync-webkit-source.ts will fail: only autobuild-preview-pr-181-e38dd2fc exists upstream.

WEBKIT_VERSION = "preview-pr-181-e38dd2fc" works for prebuilt downloads (because prebuiltUrl() auto-prefixes autobuild-), but scripts/sync-webkit-source.ts line 24 checks out WEBKIT_VERSION directly without normalization. Since only the autobuild- prefixed tag exists in oven-sh/WebKit, the checkout will fail. Either store the full ref in WEBKIT_VERSION or teach sync logic to apply the same prefix normalization.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/build/deps/webkit.ts` at line 6, The WEBKIT_VERSION constant is
missing the upstream tag prefix, causing the git checkout in the sync logic to
fail; update either the constant or the sync checkout to normalize the ref:
either set WEBKIT_VERSION to the full ref (include the "autobuild-" prefix) or
change the checkout path in the sync code that uses WEBKIT_VERSION (the git
checkout call in the sync-webkit-source.ts checkout logic) to prepend
"autobuild-" when the value doesn't already start with that prefix so both
prebuiltUrl() and the source sync use the same ref.

@github-actions
Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(windows): bypass bmalloc/libpas to fix GC crashes under memory pressure #28230 - Also fixes Windows bmalloc/libpas memory issues, but by bypassing libpas with mimalloc instead of upgrading WebKit
  2. Bump WebKit #26661 - Also bumps the bundled WebKit version (same code area, different target version)
  3. Upgrade WebKit to 5b194ac887f2 #26352 - Also bumps the bundled WebKit version to a different upstream commit

Generated with Claude Code

Comment on lines 6 to 7
export const WEBKIT_VERSION = "preview-pr-181-e38dd2fc";

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.

🟡 The prebuiltDestDir() function uses cfg.webkitVersion.slice(0, 16) to build the cache directory name, but for the new preview-style version strings like "preview-pr-181-e38dd2fc" this yields "preview-pr-181-e" — capturing only the first character of the 8-char unique commit hash. Two preview builds of the same PR whose hashes share the same leading hex digit (e.g. preview-pr-181-e38dd2fc and preview-pr-181-eabcdef0) would collide on the same destDir, preventing them from coexisting in cache. Correctness is preserved by the identity field (full version string), but switching between such previews forces a re-download each time.

Extended reasoning...

What the bug is and how it manifests

prebuiltDestDir() at line 85 builds its directory name using cfg.webkitVersion.slice(0, 16). This was designed for 40-char Git SHAs (e.g. 42f80a684c5df57121a97e20825a3bcab7a0741b), where 16 hex characters provide ~64 bits of uniqueness — essentially no collision risk. The new preview version string "preview-pr-181-e38dd2fc" is only 24 characters long, and slice(0, 16) produces "preview-pr-181-e", which includes just the first character (e) of the 8-char commit hash that is the uniquely-differentiating portion.

The specific code path that triggers it

At webkit.ts:85: const version16 = cfg.webkitVersion.slice(0, 16); followed by return resolve(cfg.cacheDir, webkit-${version16}${prebuiltSuffix(cfg)});. For the version string "preview-pr-181-e38dd2fc", the resulting destDir is webkit-preview-pr-181-e (plus any suffix). Any other preview of PR 181 whose commit hash begins with e — approximately a 1-in-16 (~6%) chance for any given pair — gets the same destDir.

Why existing code doesn't prevent it

The identity field at line 179 uses the full version string (${cfg.webkitVersion}${prebuiltSuffix(cfg)}), so the stamp-based correctness check will detect a mismatch and trigger a re-download. This prevents silent ABI corruption. However, the re-download also overwrites the previously cached extraction, meaning the two preview versions can never coexist in cache simultaneously.

What the impact would be

On a shared cacheDir used across multiple machines or CI jobs, switching between two affected preview versions (same PR, hash starting with same character) always forces a fresh download — defeating the caching that destDir is meant to provide. The ~6% per-pair collision probability is low but non-trivial for active development workflows that may compare consecutive preview builds.

How to fix it

Use the full version string (or a hash of it) as the directory key instead of a fixed 16-char prefix: const version16 = cfg.webkitVersion.slice(0, 16) → e.g. const versionKey = cfg.webkitVersion.replace(/[^a-zA-Z0-9_-]/g, '_') or simply use the full string directly since it's already filesystem-safe.

Step-by-step proof

  1. Current PR sets WEBKIT_VERSION = "preview-pr-181-e38dd2fc" (24 chars).
  2. prebuiltDestDir() calls "preview-pr-181-e38dd2fc".slice(0, 16)"preview-pr-181-e".
  3. destDir becomes $CACHE/webkit-preview-pr-181-e (plus suffix).
  4. A subsequent preview at "preview-pr-181-eabcdef0" also calls "preview-pr-181-eabcdef0".slice(0, 16)"preview-pr-181-e".
  5. Same destDir → the identity stamp mismatch triggers re-download and overwrites the first extraction.
  6. Switching back to the first preview triggers another re-download — two previews can never coexist.

Comment on lines +3 to +6
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-e38dd2fc";
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.

🟡 The JSDoc comment at the top of webkit.ts says "WebKit commit" and --webkit-version=<hash>, but WEBKIT_VERSION is now "preview-pr-181-e38dd2fc" — a named preview tag, not a git commit SHA. The <hash> placeholder in the override hint is misleading since preview-style strings are equally valid.

Extended reasoning...

What the bug is: The JSDoc block at lines 1–4 of webkit.ts uses two commit-specific terms: (1) "WebKit commit — determines prebuilt download URL + what to checkout" and (2) --webkit-version=<hash>. Both phrases imply the value is always a 40-character hexadecimal git SHA.

The code path that triggers it: WEBKIT_VERSION = "preview-pr-181-e38dd2fc" is a human-readable preview build tag (the actual GitHub release tag is autobuild-preview-pr-181-e38dd2fc, constructed by prebuiltUrl()). It is unambiguously not a commit hash — it contains hyphens and a PR number, not hex digits.

Why existing code doesn't prevent it: The functional code in prebuiltUrl() already handles both formats correctly via the autobuild- prefix logic. Only the documentation is wrong. The comment was presumably written when only SHA values were ever used, and was not updated when the preview-tag format was introduced.

What the impact would be: A developer reading the JSDoc might wrongly conclude that --webkit-version only accepts commit SHAs and that passing a preview tag string like preview-pr-181-e38dd2fc is invalid. This could cause confusion when testing against preview builds or when reviewing future PRs of this type.

How to fix it: Update the JSDoc to say "WebKit version identifier" instead of "WebKit commit", and change <hash> to something like <version> or <hash-or-tag>. Example:

/**
 * WebKit version — determines prebuilt download URL + what to checkout
 * for local mode. Override via `--webkit-version=<version>` to test a branch.
 * From https://github.com/oven-sh/WebKit releases.
 */

Step-by-step proof:

  1. Before this PR: WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b" — a 40-char hex SHA. The comment "WebKit commit" and <hash> were accurate.
  2. This PR changes it to "preview-pr-181-e38dd2fc" — contains letters like 'p', 'r', '-' which are not valid hex digits; it is clearly a named tag, not a hash.
  3. The comment at line 1 still says "WebKit commit" — inaccurate.
  4. The comment at line 3 still says --webkit-version=<hash> — the <hash> placeholder no longer describes the actual current value or the valid input format.

@Jarred-Sumner Jarred-Sumner changed the title build: WebKit → preview-pr-181-e38dd2fc build: WebKit → preview-pr-181-68e2ebfa Apr 13, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The constant WEBKIT_VERSION should not default to the preview string;
revert it so the default is the stable release (or an empty/undefined value) and
ensure preview testing only occurs when an explicit flag is passed; modify the
exported WEBKIT_VERSION (symbol WEBKIT_VERSION) to remove the "preview-pr-..."
default and change consuming logic to require/accept the --webkit-version
override instead of relying on the preview as a fallback.
🪄 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: 15a26092-105f-4192-ac3b-8da2e2fe4008

📥 Commits

Reviewing files that changed from the base of the PR and between 2c2b1ca and 6f011fb.

📒 Files selected for processing (1)
  • scripts/build/deps/webkit.ts

* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-68e2ebfa";
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.

⚠️ Potential issue | 🔴 Critical

Blocker: defaulting to this preview is unsafe with current CI crash profile.

This bump is currently tied to 99 failures with broad Windows segfaults (multiple configs/tests). Please avoid setting it as the default WEBKIT_VERSION until that regression is resolved; keep preview testing behind explicit --webkit-version override.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/build/deps/webkit.ts` at line 6, The constant WEBKIT_VERSION should
not default to the preview string; revert it so the default is the stable
release (or an empty/undefined value) and ensure preview testing only occurs
when an explicit flag is passed; modify the exported WEBKIT_VERSION (symbol
WEBKIT_VERSION) to remove the "preview-pr-..." default and change consuming
logic to require/accept the --webkit-version override instead of relying on the
preview as a fallback.

* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-68e2ebfa";
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.

🔴 Changing WEBKIT_VERSION to a preview tag string breaks scripts/sync-webkit-source.ts in two ways: (1) the git checkout on line 24 uses the tag directly, but the actual ref in oven-sh/WebKit is autobuild-preview-pr-181-68e2ebfaprebuiltUrl() adds the autobuild- prefix for downloads but sync-webkit-source.ts has no such normalization, causing a hard pathspec did not match failure; (2) the already-synced early-exit on line 17 compares git rev-parse HEAD (a 40-char SHA) to the new tag string — these formats can never match, so every invocation unconditionally re-syncs even when already at the correct commit. Fix: normalize the ref in sync-webkit-source.ts (prepend autobuild- when not already present) and resolve the tag to its SHA for the equality check.

Extended reasoning...

What the bugs are

sync-webkit-source.ts uses WEBKIT_VERSION in two places that both break when the value is a named preview tag instead of a raw 40-char SHA.

Bug 1 — hard checkout failure (lines 15, 24)

sync-webkit-source.ts imports WEBKIT_VERSION as expectedCommit and passes it directly to git checkout ${expectedCommit}. With the old SHA value (42f80a684c5df57121a97e20825a3bcab7a0741b), git could check it out directly. With the new value preview-pr-181-68e2ebfa, git looks for a tag or branch of that exact name. However, prebuiltUrl() in webkit.ts shows that the actual release tag in oven-sh/WebKit is constructed as autobuild-${version} (line 79: const tag = version.startsWith("autobuild-") ? version : autobuild-${version}``). There is no tag named preview-pr-181-68e2ebfa — only `autobuild-preview-pr-181-68e2ebfa`. The checkout fails with: `error: pathspec 'preview-pr-181-68e2ebfa' did not match any file(s) known to git.`

Bug 2 — early-exit optimization permanently broken (lines 14–17)

Before the checkout, the script runs git rev-parse HEAD and compares the output (always a 40-char hex SHA, e.g. 68e2ebfa... expanded to 40 chars) to expectedCommit = "preview-pr-181-68e2ebfa". A SHA and a tag-name string can never be equal, so the already at commit fast path never fires. Every invocation falls through to git checkout main + git pull + git checkout, even on a repo already at the right commit. Even if bug 1 is fixed (checkout uses the autobuild- prefixed tag), git checks out the tag in detached-HEAD mode — subsequent git rev-parse HEAD still returns the SHA, not the tag name, so the early-exit remains broken.

Why existing code doesn't prevent it

prebuiltUrl() correctly handles the prefix for artifact downloads, but sync-webkit-source.ts is a separate script that was written when WEBKIT_VERSION was always a raw SHA and has no normalization logic. The two code paths diverged silently.

Step-by-step proof

  1. This PR sets WEBKIT_VERSION = "preview-pr-181-68e2ebfa".
  2. Developer runs sync-webkit-source.ts to sync their local WebKit clone.
  3. Line 14: checkedOutCommit = await $(git rev-parse HEAD) → e.g. "abc123...40chars".
  4. Line 17: "abc123...40chars" == "preview-pr-181-68e2ebfa"false — no early exit.
  5. Script falls through; line 24: git checkout preview-pr-181-68e2ebfa.
  6. Git finds no tag/branch named preview-pr-181-68e2ebfapathspec did not match error.
  7. Script exits with failure; local WebKit sync is broken for all developers using local mode.

How to fix it

In sync-webkit-source.ts: (a) for the checkout, normalize the ref — prepend autobuild- when the version string does not already start with it; (b) for the equality check, resolve the expected tag to its SHA via git rev-parse autobuild-${version} or git rev-parse refs/tags/autobuild-${version}, then compare that SHA against git rev-parse HEAD.

🔬 also observed by 3071289855

@Jarred-Sumner Jarred-Sumner changed the title build: WebKit → preview-pr-181-68e2ebfa build: WebKit → preview-pr-181-4596a6f6 Apr 13, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)

6-6: ⚠️ Potential issue | 🔴 Critical

Blocker: don’t make this preview the default WebKit version.

Line 6 sets all default builds to a preview artifact currently associated with widespread Windows segfault failures in this PR’s CI context. Keep the stable default and require explicit --webkit-version=preview-pr-181-4596a6f6 for opt-in testing.

Suggested change
-export const WEBKIT_VERSION = "preview-pr-181-4596a6f6";
+export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/build/deps/webkit.ts` at line 6, The current default WEBKIT_VERSION
constant points to a preview build and must not be the default; change export
const WEBKIT_VERSION to a stable default (e.g., "stable" or the project's
canonical stable identifier) instead of "preview-pr-181-4596a6f6" and ensure any
build invocation that wants the preview must pass the explicit
--webkit-version=preview-pr-181-4596a6f6 flag so the preview is opt-in; update
code that reads WEBKIT_VERSION (the build argument/flag parsing logic) to allow
the CLI override while keeping the stable default value in the WEBKIT_VERSION
export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The current default WEBKIT_VERSION constant points to a preview build
and must not be the default; change export const WEBKIT_VERSION to a stable
default (e.g., "stable" or the project's canonical stable identifier) instead of
"preview-pr-181-4596a6f6" and ensure any build invocation that wants the preview
must pass the explicit --webkit-version=preview-pr-181-4596a6f6 flag so the
preview is opt-in; update code that reads WEBKIT_VERSION (the build
argument/flag parsing logic) to allow the CLI override while keeping the stable
default value in the WEBKIT_VERSION export.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fd286e30-b870-4fdd-98a9-bcd3dcd9d3e4

📥 Commits

Reviewing files that changed from the base of the PR and between 6f011fb and c63706b.

📒 Files selected for processing (1)
  • scripts/build/deps/webkit.ts

@Jarred-Sumner Jarred-Sumner changed the title build: WebKit → preview-pr-181-4596a6f6 build: WebKit → preview-pr-181-ebf11e28 Apr 13, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)

6-6: ⚠️ Potential issue | 🔴 Critical

Blocker: do not default to this preview until Windows crash regressions are cleared.

Line 6 makes the preview build the global default. PR discussion shows broad Windows SIGSEGV instability tied to this bump, so this should stay opt-in (--webkit-version) until those failures are green.

Suggested change
-export const WEBKIT_VERSION = "preview-pr-181-ebf11e28";
+export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
#!/bin/bash
set -euo pipefail
# Verify current CI signal before making preview default.
gh pr checks 29250 --repo oven-sh/bun --watch=false
gh pr checks 29250 --repo oven-sh/bun --json name,state,link \
  | jq -r '.[] | select(.state!="SUCCESS") | [.name,.state,.link] | `@tsv`'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/build/deps/webkit.ts` at line 6, The export WEBKIT_VERSION currently
hardcodes the preview build and makes it the global default; change the export
so the preview is not the default (e.g., set WEBKIT_VERSION to an empty string
or the prior stable value instead of "preview-pr-181-ebf11e28") and ensure
consumers only use this value when an explicit --webkit-version flag is
provided; update any code that relies on WEBKIT_VERSION as the implicit default
(search for WEBKIT_VERSION usage) so the flag parsing (the --webkit-version
path) supplies the preview string and nothing in the codebase flips to this
preview implicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The export WEBKIT_VERSION currently hardcodes the preview build and
makes it the global default; change the export so the preview is not the default
(e.g., set WEBKIT_VERSION to an empty string or the prior stable value instead
of "preview-pr-181-ebf11e28") and ensure consumers only use this value when an
explicit --webkit-version flag is provided; update any code that relies on
WEBKIT_VERSION as the implicit default (search for WEBKIT_VERSION usage) so the
flag parsing (the --webkit-version path) supplies the preview string and nothing
in the codebase flips to this preview implicitly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f234c040-6fca-418f-a718-91c0368b7e69

📥 Commits

Reviewing files that changed from the base of the PR and between c63706b and 7d5ca23.

📒 Files selected for processing (1)
  • scripts/build/deps/webkit.ts

* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-ebf11e28";
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.

🟡 The build diagnostic in config.ts uses cfg.webkitVersion.slice(0, 10) to emit a feature tag, but for the new preview-format version string "preview-pr-181-ebf11e28" this always produces "preview-pr" — identical for every preview PR build regardless of PR number or commit hash. The unique portion (PR number + 8-char hash) only starts at position 12+, so the 'forgot to revert my WebKit test branch' warning is now useless for preview builds; consider using the full version string or slicing from the end to capture the commit hash.

Extended reasoning...

What the bug is and how it manifests

The build diagnostic in config.ts (line 797) pushes a feature tag via features.push(webkit-version:${cfg.webkitVersion.slice(0, 10)}). For the previous 40-char SHA format (e.g. 42f80a684c5df57121a97e20825a3bcab7a0741b), slice(0, 10) yielded 42f80a684c — 10 hex digits providing enough uniqueness to tell builds apart at a glance. Now with the new preview-style version string "preview-pr-181-ebf11e28", the same slice(0, 10) always yields "preview-pr" — independent of PR number or commit hash.

The specific code path that triggers it

config.ts:797 is guarded by cfg.webkitVersion \!== versionDefaults.webkitVersion, so it only fires when a developer overrides via --webkit-version=<value>. Any override using a preview-format string (e.g. --webkit-version=preview-pr-181-ebf11e28 or --webkit-version=preview-pr-182-abc12345) will push the identical tag "webkit-version:preview-pr" into the build features list.

Why existing code doesn't prevent it

The slice(0, 10) was designed assuming a 40-char hex SHA as input, where the first 10 hex chars captured ~40 bits of unique entropy. The preview format preview-pr-NNN-XXXXXXXX has its unique payload (PR number at position 11-13, 8-char commit hash at positions 16-23) entirely beyond the 10-char window. There is no validation or normalization at the point where the slice is taken.

What the impact would be

Developers who accidentally leave WEBKIT_VERSION set to a preview build cannot be warned by the usual mechanism — all preview builds show the same webkit-version:preview-pr tag in build summaries. The intent of this diagnostic (per the comment: 'show a short hash so you catch forgot to revert my WebKit test branch') is completely defeated. Two different preview builds from different PRs or different commits are indistinguishable in CI build output.

How to fix it

Replace the fixed slice(0, 10) with something that captures the unique suffix regardless of format. Options include: (a) use the full version string if short enough, (b) take the last N characters (e.g. slice(-16) captures the PR number and full commit hash for preview strings while still giving 16 chars of a SHA), or (c) detect the preview format and handle it separately.

Step-by-step proof

  1. This PR sets WEBKIT_VERSION = "preview-pr-181-ebf11e28".
  2. A developer overrides with --webkit-version=preview-pr-181-ebf11e28 (or any other preview-pr-* value).
  3. config.ts:797: "preview-pr-181-ebf11e28".slice(0, 10)"preview-pr".
  4. Feature tag pushed: "webkit-version:preview-pr".
  5. A different developer uses --webkit-version=preview-pr-999-ffffffff.
  6. "preview-pr-999-ffffffff".slice(0, 10) → also "preview-pr".
  7. Both builds show identical webkit-version:preview-pr tags — zero diagnostic signal.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants