Repository navigation
fix(router): normalize Java named capture groups in route constraints to non-capturing - #2982
wheels-bot[bot] wants to merge 3 commits into
Conversation
… to non-capturing Extends $nonCapturingConstraint in vendor/wheels/Mapper.cfc to rewrite `(?<name>...)` to `(?:...)` so a named capture group inside a route constraint no longer shifts the position of every subsequent route variable when $mergeRoutePattern extracts values by group position. The previous fix (#2944) bounded the extraction loop so an extra group could no longer crash param assembly, but a named group inside a constraint was still position-shifting because the scanner skipped every `(?...)` flavor without distinguishing the named-capturing form from lookbehinds. The scanner now disambiguates: `(?<=...)` / `(?<!...)` lookbehinds are left untouched (non-capturing already), while `(?<name>...)` consumes the `<name>` token and emits `(?:` in its place. Bare `(...)`, explicit `(?:...)`, lookaheads, and parens inside character classes continue to behave as before. Covered by a new spec at vendor/wheels/tests/specs/mapper/NonCapturingConstraintSpec.cfc that asserts both the unit-level normalization and the $patternToRegex integration shape, plus regression cases for the existing lookbehind / lookahead / non-capturing / bare-capture paths. Fixes #2976 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…r-java-named-capture-groups-in-route-constrai Signed-off-by: Peter Amiri <peter@alurium.com> # Conflicts: # CHANGELOG.md # vendor/wheels/Mapper.cfc
…p superseded duplicate spec PR #2989 (develop commit 1c9915e) already landed the named-capture-group normalization for issue #2976 with reviewed tests in mapperModernSpec.cfc. The only case in this branch's NonCapturingConstraintSpec.cfc not covered there was nested named groups, which exercises the scanner resuming right after the consumed `(?<name>` opener. Port that single expectation into the canonical spec and delete the otherwise-duplicative spec file. Signed-off-by: Peter Amiri <peter@alurium.com>
Conflict resolution + supersession cleanupThis branch was fully superseded before it could merge: issue #2976 was fixed on Conflicts resolved (merge commit
|
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR. The net diff is +5 lines of test code in mapperModernSpec.cfc — a single nested-named-group assertion that fills the last gap in the $nonCapturingConstraint unit coverage. The logic is correct, the placement is right, and there are no cross-engine concerns. Two documentation nits prevent an approve: the PR body still describes the full implementation (Mapper.cfc edits, the now-deleted NonCapturingConstraintSpec.cfc) as though it were being landed here, and the changelog checklist entry implies a direct [Unreleased] edit rather than the fragment approach that's actually in place. Neither is blocking; marking as comment.
Correctness
The new expectation is logically sound. Tracing $nonCapturingConstraint (Mapper.cfc:136–189) against the input (?<outer>(?<inner>\d{2})\-\d{2}):
i=1→(, next char is?;ReFind("^\(\?<[A-Za-z][A-Za-z0-9]*>", ...)matches(?<outer>(9 chars); emits(?:, advancesi += 8→i=9.- For-loop increments →
i=10→((start of(?<inner>); same branch fires; emits(?:, advancesi += 8→ for-loop leavesi=19. - Remainder
\d{2})\-\d{2})passes through unchanged.
Assembled result: (?:(?:\d{2})\-\d{2}) — matches the .toBe(...) argument exactly. ✓
The comment in the test is also accurate: "the scanner must resume right after the outer > and re-detect the inner opener at the very next character." After consuming (?<outer>, position 10 is the ( that opens (?<inner>.
Docs
1. PR body is stale (vendor/wheels/tests/specs/mapperModernSpec.cfc, net diff lines 397–400)
The summary describes the full implementation — edits to vendor/wheels/Mapper.cfc, creation of vendor/wheels/tests/specs/mapper/NonCapturingConstraintSpec.cfc with 10 assertions, a 4-of-10 pre/post fix count, and a test plan that refers to the NonCapturingConstraintSpec describe block — but none of that is in the actual net diff. The commit message for 39a589e correctly explains the situation ("PR #2989 (develop commit 1c9915e) already landed the named-capture-group normalization"), but the PR body was never updated to match. A reviewer reading only the PR description would expect Mapper.cfc changes that don't appear in the diff.
Suggested fix: replace the Summary and Test Plan sections with a short note that the implementation landed on develop via #2989 and this PR contributes the single missing nested-groups coverage case.
2. Changelog checklist wording (PR description, Feature Completeness Checklist)
- [x] **CHANGELOG.md** -- Entry added under `[Unreleased]` then Fixed
This implies a direct edit to the [Unreleased] section, which CLAUDE.md § "Changelog entries" explicitly forbids ("a direct CHANGELOG.md [Unreleased] edit is a finding — it recreates the same-anchor merge conflicts the fragment system removes"). The actual state is correct — changelog.d/router-named-capture-groups.fixed.md exists and contains the right entry — but the checklist description is misleading.
Suggested fix: update the line to [x] **CHANGELOG.md** -- Fragment file added under changelog.d/router-named-capture-groups.fixed.md per CLAUDE.md conventions.
Commits
Both commits are within the commitlint rules:
fix(router): normalize Java named capture groups in route constraints to non-capturing— 86 chars, typefix, DCO signed byclaude[bot]. ✓test(router): fold nested named-group case into mapperModernSpec, drop superseded duplicate spec— 96 chars, typetest, DCO signed by Peter Amiri with matching identity. ✓
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-reasoned. The correctness trace checks out, the two doc nits are real, and COMMENT is a defensible verdict for a 5-line test-only diff whose only issues are in PR metadata. I'm aligned. SycophancyNone detected. A provided a step-by-step positional trace to justify the correctness verdict rather than approving on vibes. False positivesNone detected.
Missed issuesNone detected. This is test code only; no cross-engine concerns apply. No security surface. The PR title is 78 chars, under the 100-char gate. Verdict alignmentCOMMENT is consistent with the findings: two actionable doc nits (stale PR body, misleading checklist entry) with no code issues — neither rises to request-changes, but together they are enough to hold off an outright approve. ConvergenceAligned. The code change is correct and complete; the two open items are both PR description updates:
Joint recommendation: apply both PR body updates and approve. |
Wheels Bot — Address Review (round 1)Applied the consensus findings from Reviewer A and Reviewer B's convergence (round 1 of the review loop):
No code commit was produced — both consensus findings were PR-metadata edits only. The repo diff (5 test lines + 1 spec deletion) is unchanged from Findings skippedNone — both converged findings were addressed. |
Summary
The
$nonCapturingConstraint()fix for #2976 landed ondevelopvia #2989 (commit1c9915e69, merged 2026-06-10) before this branch could merge. That PR'sReFind-based rewrite is strictly more complete than this branch's originalMid()-based implementation (it also fail-fast errors on\k<name>backreferences via$compileRegex), so the conflict-resolution merge took develop's version wholesale.What remains on this branch is a single nested-named-group test case in
vendor/wheels/tests/specs/mapperModernSpec.cfc(5 lines) that exercises the scanner resuming immediately after a consumed(?<name>opener and re-detecting the inner opener at the very next character. The duplicatevendor/wheels/tests/specs/mapper/NonCapturingConstraintSpec.cfcwas folded into the canonical spec and deleted, since the other assertions were 1:1 duplicates of coverage already merged in #2989.Related Issue
Closes #2976 (the underlying bug — fix landed via #2989)
Type of Change
Feature Completeness Checklist
Signed-off-by:(usegit commit -s); see CONTRIBUTING.mdvendor/wheels/tests/specs/mapperModernSpec.cfc(the existing$nonCapturingConstraintcoverage merged in fix(router): normalize Java named capture groups in route constraints #2989 lacked this specific case). Local:wheels.tests.specs.mapperModernSpec— 35/35 pass; fullwheels.tests.specs.mapperdirectory — 94/94 pass on Lucee 7 + SQLite.changelog.d/router-named-capture-groups.fixed.mdadded per CLAUDE.md conventions (carried in from fix(router): normalize Java named capture groups in route constraints #2989's merge into this branch).Test Plan
mapperModernSpec.cfccovers the nested-groups case:(?<outer>(?<inner>\d{2})\-\d{2})→(?:(?:\d{2})\-\d{2}).