Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 13 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "cb61607f1a4bae79d7701965062634dee9efb349"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-517-73543b07"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-517-73543b07, an ephemeral preview build for an unmerged WebKit PR. Per the repo's dependency rules (.claude/docs/landing-prs.md § Dependencies & vendoring: "Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA"), this must not merge as-is — once oven-sh/WebKit#517 lands, swap this to the merged SHA and verify prebuilt artifacts exist for every platform × flavor.
Extended reasoning...
What the issue is
WEBKIT_VERSION is changed from a 40-hex commit SHA to autobuild-preview-pr-517-73543b07 — a preview-build tag that oven-sh/WebKit's CI publishes for an open, unmerged PR (oven-sh/WebKit#517). The PR description acknowledges this is temporary ("After oven-sh/WebKit#517 merges, the pin should move to the merged sha"), but as written the PR would land the ephemeral pin.
Why this is merge-blocking
The repo's own review rules are explicit on this. REVIEW.md directs dependency bumps to .claude/docs/landing-prs.md § Dependencies & vendoring, which states verbatim at line 47:
Version bumps are repo-wide, verified operations. Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge.
autobuild-preview-pr-517-73543b07 is precisely the case that rule names: a preview tag for an unmerged-PR build. Repository-specific instructions can raise severity, and here they explicitly say "never merge" — so this is blocking, not a nit.
Concrete failure mode (step-by-step)
- This PR merges to
mainwithWEBKIT_VERSION = "autobuild-preview-pr-517-73543b07". prebuiltUrl()inscripts/build/deps/webkit.ts:76sees the value already starts withautobuild-and constructshttps://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-517-73543b07/bun-webkit-<os>-<arch><suffix>.tar.gz.- Yarr: reject non-ASCII identity escapes in unicode patterns WebKit#517 is merged (or closed, or force-pushed). The fork's CI garbage-collects the
autobuild-preview-pr-517-*release, or never published every platform × flavor combination in the first place. - Any fresh clone of Bun at this commit — a new contributor, a clean CI runner, a bisect landing here — runs
bun bd. The prebuilt fetch 404s and the build fails. There is no fallback SHA to try.
Nothing in the existing code prevents this: prebuiltUrl() and prebuiltDestDir() were both updated (in a prior PR) to accept autobuild--prefixed tags specifically so branches can test preview builds via --webkit-version=<tag>, but that mechanism is for local overrides, not for the checked-in default.
How to fix
Wait for oven-sh/WebKit#517 to merge, then replace this line with the resulting 40-hex commit SHA on oven-sh/WebKit main (e.g. the merge commit or squashed SHA), and confirm the corresponding autobuild-<sha> release has artifacts for every {linux,macos,windows,freebsd} × {amd64,arm64} × {debug,lto,asan,musl,android} combination prebuiltSuffix() can produce. Until then this PR should stay open (or in draft).
There was a problem hiding this comment.
Acknowledged. The preview pin is deliberate so CI can exercise the engine fix, and it must not merge. I will swap to the merged oven-sh/WebKit sha once oven-sh/WebKit#517 lands, and keep this thread open until then.
|
Correct, the preview pin is temporary and this PR must not merge as-is. The pin points at the preview build of oven-sh/WebKit#517 so CI can exercise the engine fix on every platform. When that PR merges, I will move |
c9b4c72 to
eec7cf6
Compare
|
Rebased on main. Main moved the WebKit pin to 1cb96a7b (#40417), so I rebased the engine branch in oven-sh/WebKit#517 onto that commit and repointed the pin at its new preview build ( |
eec7cf6 to
adb983c
Compare
|
Rebased on main again. Main's WebKit pin moved to 76882271, so the engine branch in oven-sh/WebKit#517 was rebased onto that commit and the pin here now points at its preview build ( |
|
Updated 6:45 AM PT - Aug 28th, 2026
❌ @robobun, your commit 3acba8a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40459That installs a local version of the PR into your bun-40459 --bun |
adb983c to
6fa4ff8
Compare
|
Rebased on main. Main's WebKit pin moved to 2da33d53 (#40570), so the engine branch in oven-sh/WebKit#517 was rebased onto it and the pin here now points at the new preview build ( |
6fa4ff8 to
7f38225
Compare
|
Rebased on main. Main's WebKit pin moved to 72597399, so the engine branch in oven-sh/WebKit#517 was rebased onto it and the pin here points at the new preview build ( The engine PR also picked up a CI fix: the windows-11-arm preview lane was failing fork-wide because the scoop installer aborts without an error on the 20260823 runner image. The lane now uses the preinstalled ninja and 7-Zip instead of scoop. Still draft until oven-sh/WebKit#517 merges. |
7f38225 to
54dcc85
Compare
|
Rebased on main. Main's WebKit pin moved to 0bb01ed5, so the engine branch in oven-sh/WebKit#517 was rebased onto it and the pin here points at the new preview build ( |
54dcc85 to
08fbdf7
Compare
|
Rebased on main. No conflicts this time: the pin and diff are unchanged ( |
08fbdf7 to
09829e7
Compare
|
Rebased on main. Main moved the WebKit pin to 1817c3c3, so the engine branch in oven-sh/WebKit#517 was rebased onto it and the pin here points at the new preview build ( |
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
09829e7 to
3acba8a
Compare
|
Rebased on main. Main moved the WebKit pin to c4ddc0cf, so the engine branch in oven-sh/WebKit#517 was rebased onto it and the pin here points at the new preview build ( |
…0459 to the RegExp identity-escape test
|
Closing in favour of #41767. Both PRs fix the same check in |
Problem
new RegExp("\\Ç", "u")compiles, and/\Ç/umatchesÇwith the Annex B meaning. ECMA-262 requires a SyntaxError: IdentityEscape under theuorvflag allows only SyntaxCharacter or/, plus ClassSetReservedPunctuator insidev-mode class sets. All of those are ASCII. V8 and SpiderMonkey throw. Fixesu/v-mode identity-escape validation is ASCII-only #40441.isIdentityEscapeAnErrorin Yarr (Source/JavaScriptCore/yarr/YarrParser.h:860in oven-sh/WebKit). The error condition is gated onisASCII(ch)becausestrchronly handles bytes, so any non-ASCII escape skips validation.Fix
WEBKIT_VERSIONto that PR's preview build (autobuild-preview-pr-517-f390a25a) and addstest/js/bun/jsc/webkit-upgrade-f390a25a.test.ts. After Yarr: reject non-ASCII identity escapes in unicode patterns WebKit#517 merges, the pin should move to the merged sha.\Ç,\é,\字, astral escapes, in and out of classes, withuandv) and passes with the bump. Also rantest/js/bun/jsc/and the regexp jsc-stress fixtures.Background
WEBKIT_VERSIONinscripts/build/deps/webkit.ts.\followed by a character that stands for itself. Annex B lets non-unicode patterns escape almost anything. Theuandvflags removed that laxness so escapes stay forward-compatible.regexp-unicode-identity-escape-non-ascii.js) that covers the same matrix inside the fork's own test suite.Notes
scripts/build/deps/webkit.ts. Asrc/-only stash keeps the bump, so a mechanical fail-before run of the test still passes. The fail-before proof is:USE_SYSTEM_BUN=1 bun test test/js/bun/jsc/webkit-upgrade-f390a25a.test.tsfails 1 of 3 on 1.4.1, and the same file passes underbun bdwith this branch.test/js/bun/jsc/domjit.test.tsshows 10 timeout failures locally in the debug ASAN build on a capped-core container (the same tests pass earlier in the same file in 1 to 4 s, the JIT-warmup repeat pass exceeds the 5 s budget). The Yarr change only runs at RegExp compile time and does not touch those paths.isIdentityEscapeAnErrorpasses an ASCII literal, so only the IdentityEscape default case inparseEscapechanges behavior.\-inside a class is special-cased before the check and stays valid.[decide:webkit] gate passed · iteration 6 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 8 passed · 0 rejected · iteration 6
evidence per changed file
root cause · written by the author bot
In Yarr's regular expression parser, the unicode-mode check that rejects invalid identity escapes was gated behind an ASCII test, so any non-ASCII character following a backslash bypassed validation and fell through to the lenient Annex B behavior where the escaped character matches itself. The fix removes the ASCII gate so the validation applies to all code points, meaning an identity escape under the u or v flag is only accepted for the syntax characters the specification permits and anything else raises a SyntaxError at compile time. This brings behavior in line with ECMA-262 and with V8…