[regression test] /[a-]/v should be a SyntaxError (#29003)#29006
[regression test] /[a-]/v should be a SyntaxError (#29003)#29006
Conversation
See oven-sh/WebKit#180 for the JSC-side parser fix. In UnicodeSets mode, - is a ClassSetSyntaxCharacter that is only legal as part of a full ClassSetRange. A dangling - (e.g. /[a-]/v, /[\d-]/v, /[\w-]/v, /[a-z\d-]/v) must throw a SyntaxError, matching V8/SpiderMonkey and the spec. The fix lives in vendor/WebKit (not tracked in this repo) in yarr/YarrParser.h. The companion WebKit PR raises ErrorCode::InvalidClassSetCharacter in flushCachedCharacterIfNeeded() and end() when the parser is still in a CachedCharacterHyphen or AfterCharacterClassHyphen state at a class-set transition point.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded a regression test that exercises /v (UnicodeSets) character-class parsing around dangling hyphens: it asserts specific forms throw Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@test/regression/issue/29003.test.ts`:
- Around line 5-8: In the regression test file issue/29003.test.ts remove the
extra multi-line explanatory comments and leave only the single-line GitHub
issue URL comment; specifically trim the multi-line bug-context prose around the
UnicodeSets note (the comment shown in the diff) and the other comment blocks
referenced (lines noted in the review) so the test body follows the repo's
single-line issue URL style; keep test code and assertion names intact (e.g.,
the test that asserts that trailing/dangling `-` in /v mode throws SyntaxError)
and ensure only one-line issue URL comments remain adjacent to those tests.
🪄 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: 33d1b7b2-229f-4523-9d7a-139428cd2114
📒 Files selected for processing (1)
test/regression/issue/29003.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Drop the multi-line prose comments in favor of the single-line issue URL (coderabbit nit). - Split the already-passing regression guards ([-a], [-]) into their own test() block so they keep running even if the pending /v fix lands late. - Wrap the not-yet-landed assertions (/[a-]/v, /[\d-]/v, /[\w-]/v, /[a-z\d-]/v) in test.todo(). These need oven-sh/WebKit#180 to merge and a WEBKIT_VERSION bump; the todos get promoted to test() in the same commit as the bump.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
My prior feedback was addressed — the structure is now correct.
Extended reasoning...
Overview
This PR adds a single regression test file () for issue #29003. No production code is modified.
Security risks
None. This is a test-only change.
Level of scrutiny
Low. The change is a test file only, with no impact on runtime behaviour. The structure is straightforward and follows established patterns in the regression test suite.
Resolution of prior feedback
My previous review flagged that the not-yet-fixed assertions would break CI and that the regression guards were structurally ineffective. Commit c49d5c7 addressed both issues: already-passing guards are now in a live test() block, and the pending-WebKit assertions are wrapped in test.todo(). The author verified 2 pass, 2 todo, 0 fail with USE_SYSTEM_BUN=1 bun test.
There was a problem hiding this comment.
LGTM — feedback addressed: live guards split out, pending assertions properly wrapped in test.todo().
Extended reasoning...
Overview
Single new test file (test/regression/issue/29003.test.ts, 35 lines). Adds regression coverage for issue #29003 — dangling hyphens in UnicodeSets (/v) mode. Two live test() blocks cover already-correct behavior (rejected forms and valid patterns); two test.todo() blocks document the pending WebKit parser fix.
Security risks
None. This is a test-only change with no production code paths touched.
Level of scrutiny
Low. Test-only addition following established repo patterns. No logic changes, no CODEOWNER-owned files, no security-sensitive code.
Other factors
My prior review flagged that the original commit had live assertions for not-yet-fixed behavior that would break CI. The author addressed both issues raised: split the already-passing guards into their own test() block and wrapped the unimplemented assertions in test.todo(). The author verified USE_SYSTEM_BUN=1 bun test gives 2 pass, 2 todo, 0 fail — exactly correct.
|
CI failure on This PR only adds From the failing job log: All four retries SIGILL in the same JSC spot under ASan. This is a JSC |
There was a problem hiding this comment.
LGTM — my previous concerns about CI-breaking live assertions were fully addressed in c49d5c7.
Extended reasoning...
Overview
Single new test file covering dangling-hyphen behaviour in UnicodeSets () mode.
What changed since my last review
My previous review flagged that several assertions would fail CI because the underlying WebKit fix hasn't landed yet. The author addressed this in commit c49d5c7:
- Already-rejected patterns (, ) are in a live block, keeping regression guards active.
- Not-yet-fixed patterns (, , , ) are wrapped in , so they don't break CI.
- Valid patterns are verified in a separate live .
Security risks
None — this is a pure test addition with no production code changes.
Level of scrutiny
Low. Single test file, no logic changes, well-structured with appropriate usage matching repo conventions.
Other factors
The CI failure noted in the timeline is a pre-existing JSC flake in an unrelated azure-service-bus test, not caused by this PR. All inline review comments are resolved.
|
CI build #44476 failed on all 11 test lanes (alpine, darwin, debian, ubuntu) with exit 2 — the common root cause is From the asan lane (019d6dce-6711): (Different flake than the azure-service-bus SIGILL on build #44466 but same pattern: unrelated infra failure.) Pushed empty rekick |
There was a problem hiding this comment.
LGTM — my prior feedback was addressed in c49d5c7.
Extended reasoning...
Overview
Single new test file for issue #29003 (dangling hyphen in UnicodeSets mode). No production code is touched.
Previous review
My prior review flagged that the original test structure would break CI because assertions for not-yet-landed patterns were in live blocks. The author split the already-passing guards into their own , moved the not-yet-fixed forms into , and confirmed bun test v1.3.11 (af24e28)
bun install v1.3.11 (af24e28)
- node-gyp@11.2.0
90 packages installed [1024.00ms]
bun install v1.3.11 (af24e28)
- node-gyp@11.2.0
90 packages installed [36.00ms]
bun install v1.3.11 (af24e28)
- node-gyp@11.2.0
90 packages installed [26.00ms]
bun install v1.3.11 (af24e28)
- node-gyp@11.2.0 (v12.2.0 available)
90 packages installed [103.00ms] gives 2 pass / 2 todo / 0 fail.
Security risks
None — this is a pure test addition with no runtime or library code changes.
Level of scrutiny
Low. Test-only, single file, follows repo conventions ( for pending upstream fixes), and CI failures in the timeline are unrelated infrastructure flakes (JSC ASan SIGILL, Valkey timeout).
Other factors
No bugs found by the bug hunting system. All inline comments from other reviewers are resolved. The test structure is now correct and will not block CI.
|
CI build #44505 failed on Nothing in this PR touches |
There was a problem hiding this comment.
LGTM — my earlier feedback was fully addressed in c49d5c7: live regression guards are in their own block and the pending WebKit assertions are correctly wrapped in .
Extended reasoning...
Overview
Single new test file () documenting issue #29003 — dangling hyphens in (UnicodeSets) mode should throw per ECMA-262 but currently don't due to a JSC/WebKit parser bug.
Security risks
None. This is a pure test addition with no production code changes.
Level of scrutiny
Low. The change is a single test file with no logic changes. The test structure is mechanically straightforward and follows existing repo conventions.
Other factors
My previous review flagged that originally-failing assertions were not wrapped in , which would have broken CI. The author addressed both issues (split the live guards into their own block, wrapped pending assertions in ), and verified the result yields 2 pass / 2 todo / 0 fail. CI failures on recent builds are pre-existing unrelated infra flakes (JSC ASan assertion, Valkey connection timeout, BroadcastChannel worker GC) that the author has documented and rekicked.
|
Build #44514 failed on I'm going to stop rekicking. This PR is test-only (2 live regression guards + 2
Either action is outside a test-only PR's scope. The live regression guards already cover |
1 participant
In UnicodeSets mode (
/v),-is aClassSetSyntaxCharacterper ECMA-262 and is only legal between twoClassSetCharacters as part of aClassSetRange. Bun currently parses/[a-]/vsuccessfully (and also/[\d-]/v,/[\w-]/v,/[a-z\d-]/v). Node, Deno, and the spec all reject these.Root cause
In
vendor/WebKit/Source/JavaScriptCore/yarr/YarrParser.h,ClassSetParserDelegate::flushCachedCharacterIfNeeded()only flushes theCachedCharacterstate.end()silently acceptsCachedCharacterHyphenby emitting both the cached character and a literal-, and falls throughAfterCharacterClassHyphenwithout any error. Both states represent an incompleteClassSetRange(a-with no RHS yet) and must be errors in/v.Fix
WebKit PR: oven-sh/WebKit#180
The WebKit fix raises
ErrorCode::InvalidClassSetCharacterin bothflushCachedCharacterIfNeeded()andend()when either incomplete-range state is hit. The valid-range path (a-z) is untouched because it goes straight fromCachedCharacterHypheninto the range-completion branch ofatomPatternCharacter()without routing through either helper.Once the WebKit PR merges and a new autobuild tarball exists, a follow-up
WEBKIT_VERSIONbump incmake/tools/SetupWebKit.cmakewill make this test pass.Patterns covered by the test
Newly rejected (will throw after WebKit fix):
/[a-]/v(the reported case)/[\d-]/v/[\w-]/v/[a-z\d-]/vAlready rejected (regression guard):
/[-a]/v/[-]/vMust still parse (regression guard):
/[a-z]/v,/[a\-]/v,/[\-a]/v,/[a--b]/v,/[a&&b]/v,/[\w--\d]/vFixes #29003.