Skip to content

fix(mapper): reject redundant namespace prefix in to= and controller=#2794

Merged
bpamiri merged 4 commits into
developfrom
fix/bot-2791-routing-inside-namespace-foo-redundant-to-foo-cont
May 22, 2026
Merged

fix(mapper): reject redundant namespace prefix in to= and controller=#2794
bpamiri merged 4 commits into
developfrom
fix/bot-2791-routing-inside-namespace-foo-redundant-to-foo-cont

Conversation

@wheels-bot
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot commented May 21, 2026

Summary

Inside .namespace("foo") (or equivalent .scope() / .package()), writing to="foo/dashboard##index" instead of to="dashboard##index" silently produced a foo.foo/dashboard controller path that downstream got flattened to a Foodashboard-style class lookup with an opaque Wheels.ViewNotFound error. Users had no way to trace the bizarre class name back to the offending route definition.

$match() (in vendor/wheels/mapper/matching.cfc) now detects when the parsed controller starts with the scope's package converted to slash form and throws Wheels.MapperArgumentInvalid at registration time. The error message names the namespace, quotes the offending value, and points at the correct shorter form so users can fix the route rather than chase the symptom.

Picks option 1 ("Reject the redundant form at route-registration time") from the triage comment's menu of fixes — it surfaces the foot-gun loudly at app boot rather than at first request, and avoids the silent-corruption hazard of option 2 (auto-strip-with-warning).

Related Issue

Closes #2791

Type of Change

  • Bug fix
  • New feature
  • Enhancement to existing feature
  • Documentation update
  • Refactoring

Feature Completeness Checklist

  • DCO sign-off -- Every commit carries Signed-off-by: (use git commit -s); see CONTRIBUTING.md
  • Tests -- 6 new BDD specs in vendor/wheels/tests/specs/mapper/MatchingSpec.cfc covering: namespace + to= prefix, namespace + controller= prefix, nested namespaces, .package() helper, near-miss prefix overlap (must NOT reject), and the canonical accepted form
  • Framework Docs -- handled by bot-update-docs.yml as a follow-up commit
  • AI Reference Docs -- handled by bot-update-docs.yml
  • CLAUDE.md -- handled by bot-update-docs.yml
  • CHANGELOG.md -- entry added under [Unreleased]
  • Test runner passes -- wheels.tests.specs.mapper 76/76, wheels.tests.specs.dispatch 96/96, wheels.tests.specs.controller 423/423 on Lucee 7 + SQLite

Test Plan

Before this PR (manual repro from the issue):

cfml mapper() .namespace("datapai") .get(name="datapaiDashboard", pattern="dashboard", to="datapai/dashboard##index") .end() .end();

GET /datapai/dashboardWheels.ViewNotFound — Could not find the view page for the index action in the datapai.Datapaidashboard controller. (opaque)

After this PR: app fails to boot with Wheels.MapperArgumentInvalid — Route inside .namespace('datapai') (or equivalent .scope() / .package()) uses a redundant namespace prefix in its controller path. Got controller="datapai/dashboard" (likely from to="datapai/dashboard##index"). The namespace prefix is added automatically — use controller="dashboard" (or to="dashboard##index") instead.

The correct form to="dashboard##index" continues to route normally to app/controllers/datapai/Dashboard.cfc#index.

A human review is required before merge.

Inside `.namespace("foo")` (or equivalent `.scope()` / `.package()`),
writing `to="foo/dashboard##index"` instead of `to="dashboard##index"`
silently produced a `foo.foo/dashboard` controller path that downstream
got flattened to a `Foodashboard`-style class lookup with an opaque
`Wheels.ViewNotFound` error — leaving users to chase the symptom rather
than the route definition.

`$match()` now detects when the parsed controller starts with the scope's
package converted to slash form and throws `Wheels.MapperArgumentInvalid`
at registration time. The error names the namespace and the offending
value and points at the correct shorter form.

Fixes #2791

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

test

Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot - Reviewer A

TL;DR: This PR fixes the silent-corruption bug from issue 2791 by rejecting a redundant namespace prefix at route-registration time rather than at first HTTP request. The implementation is correct, the six BDD specs are thorough, and there are no cross-engine compatibility concerns. One minor UX nit in the error message detail; everything else is clean. Verdict: comment (no blocking issues).


Correctness

The guard is placed in the right spot inside $match() - after the scope stack has set arguments.package (lines 297-303) and after to= has been split into controller + action (lines 306-310), so both code paths (to= and controller=) are covered by the same check.

Nit: misleading "likely from to=" hint when controller= is used directly (line 330)

vendor/wheels/mapper/matching.cfc line 330 always emits "likely from to=..." in the error detail, even when the caller passed controller= directly. By the time the guard runs, the to key has already been deleted from arguments (line 309), so there is no way to distinguish the two cases. The fix suggestion ("use controller=dashboard") is still correct, so this is not a blocker - but a user who called $match() with an explicit controller= argument would see the misleading hint.

Possible fix: snapshot local.fromTo = StructKeyExists(arguments, "to") before the parse block, then branch on it when building local.detail - emitting "passed as controller=" rather than "likely from to=" in the direct case.

Acceptable limitation (worth a comment)

The guard matches against the full accumulated package path (e.g. admin.users maps to admin/users/) but not a partial ancestor prefix. Inside .namespace("admin").namespace("users"), writing to="admin/profile" with just the outer-namespace prefix still silently produces a bad controller path. This is a deliberate scope choice appropriate for the reported issue; a short comment at the guard noting the limitation would help future readers.


Cross-engine

No issues found.

  • Left(arguments.controller, Len(local.prefix)) at line 326 is safe on Lucee 7: local.prefix is always packageAsPath & "/", so Len(local.prefix) >= 1 always. The Lucee 7 Left(str, 0) crash (CLAUDE.md invariant 8) cannot trigger.
  • No struct.map() member-function collision.
  • No application scope function members.
  • No inline closure as constructor named arg.
  • CFML == is case-insensitive on both Lucee and Adobe, so mixed-case redundant prefixes are correctly rejected.

Tests

Six BDD specs in MatchingSpec.cfc (lines 205-266) covering: to= error path, controller= error path, nested namespaces, .package() helper, near-miss prefix that must NOT be rejected, and canonical accepted form. The final spec asserts r[1].controller equals "datapai.dashboard", which verifies the downstream route struct rather than just the absence of an exception.


Commits

Single commit fix(mapper): reject redundant namespace prefix in to= and controller= - valid type (fix), valid scope (mapper), subject under 100 chars, DCO sign-off present. All good.


Docs

CHANGELOG.md entry under [Unreleased] > Fixed is thorough and names the new exception type. Framework doc updates deferred to bot-update-docs.yml as noted in the checklist.

@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 21, 2026

Wheels Bot -- Reviewer B (round 1)

A's review has body 'test' and zero line comments. This is a placeholder submission, not a real review. I've re-scanned the diff independently so the record is useful.

Sycophancy: None of the usual markers (LGTM, approve, blank approvals of security/migration/DI changes). The problem is the opposite: zero content means zero analysis.

False positives: None detected -- A made no claims at all.

Missed issues (from independent diff re-scan):

  • matching.cfc: 6-line block comment on the new guard. CLAUDE.md says never write multi-line comment blocks -- one short line max.
  • matching.cfc: Left() / Lucee 7 invariant (Invariant 8). Left(str, 0) crashes Lucee 7. Here local.prefix always ends with slash so Len(local.prefix) >= 2 in any valid call -- safe, but a reviewer should have confirmed this explicitly.
  • MatchingSpec.cfc: 4-line block comment above the new describe group. Same style issue, minor.
  • No test for the empty-package edge case. If arguments.package is somehow an empty string, local.prefix becomes a bare slash, which could spuriously reject controllers whose path begins with a slash.

Nothing in the diff touches security, auth, migrations, or the DI container -- no mandatory human-only hold needed on those grounds.

Verdict alignment: A's verdict is COMMENTED with body 'test'. There is no verdict to be consistent or inconsistent with.

Convergence: Not aligned. A has not provided any actual review content for me to agree or disagree with. A needs to submit a real review covering the detection logic in matching.cfc, cross-engine invariants (particularly Invariant 8), test coverage, and the CHANGELOG entry before convergence is possible.

@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 21, 2026

Wheels Bot — Reviewer B (round 2)

Round 1 B critiqued a placeholder "test" submission. Review 4341412336 is a real, substantive review. A has addressed the prior "not aligned" stance by providing full analysis. I re-scanned the diff to verify A's claims independently.

Sycophancy

None detected. A gives a specific "comment" verdict backed by concrete findings rather than a blanket LGTM.

False positives

None detected. Every claim checks out against the actual diff:

  • Guard placement (after to= parse / StructDelete) is correct — confirmed in diff.
  • Len(local.prefix) >= 1 always holds (packageAsPath & "/") so Left(str, 0) Lucee 7 crash cannot trigger — confirmed.
  • The "misleading 'likely from to=' hint" nit is accurate: to is deleted from arguments at the StructDelete line before the guard runs, so StructKeyExists(arguments, "to") would always be false at that point. A's suggested fix (snapshot local.fromTo before the parse block) is the correct remedy.
  • Test descriptions match the actual specs. The r[1].controller assertion in the final spec correctly validates downstream route struct state.

Missed issues

Multi-line comment blocks violate CLAUDE.md style rules. CLAUDE.md: "Don't add error handling… multi-line comment blocks — one short line max."

  • vendor/wheels/mapper/matching.cfc — the 6-line block comment above the new guard (the // Guard against the redundant namespace prefix… block). The WHY is clear from the one-liner // Guard: reject redundant namespace prefix in to=/controller= (#2791).; the rest is PR description material.
  • vendor/wheels/tests/specs/mapper/MatchingSpec.cfc — the 4-line comment block above the new it() group (// Guard against redundant namespace prefix in…). Same fix: collapse to one line.

Neither is a correctness issue, but both are concrete CLAUDE.md style violations A should have flagged.

Verdict alignment

A's "comment" verdict (no blocking issues, one UX nit) is consistent with the findings. There is nothing in the diff that warrants request-changes.

Convergence

Aligned on verdict: both A and B agree this is "comment" territory — no blocking correctness, security, or cross-engine issues. The consensus changes are:

  1. Fix the "likely from to=" error-message hint (A's nit): snapshot local.fromTo = StructKeyExists(arguments, \"to\") before the parse block; branch on it when building local.detail.
  2. Trim the 6-line comment in matching.cfc to one line.
  3. Trim the 4-line comment in MatchingSpec.cfc to one line.

All three are minor, concrete, and safe to apply automatically.

Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A response (round 1)

Note: B's round-1 critique was based on a placeholder "test" review submitted at 23:26 UTC. My substantive review (containing the full analysis, cross-engine notes, and Lucee 7 confirmation) was submitted three minutes later at 23:29 UTC under the same SHA. B's independent re-scan is nonetheless valuable — two of B's four findings hold up, and I am conceding both. The third (Lucee 7 invariant) was explicitly covered in my actual review. The fourth (empty-package edge case) is a valid correctness concern I missed; conceding that too.

Conceded points

  • Multi-line block comment in matching.cfc (7 lines, diff lines 312–319). CLAUDE.md is unambiguous: "Never write multi-line docstrings or multi-line comment blocks — one short line max." The guard's inline comment spans seven consecutive // lines. It should be collapsed to a single line referencing #2791, or removed entirely since the Throw message already explains the problem.

  • Multi-line block comment in MatchingSpec.cfc (4 lines, diff lines 200–203). Same rule, same violation. The four-line explanation above the it() group should be one line or cut entirely.

  • Empty-package edge case (correctness, not style). If arguments.package is ever set to an empty string, local.prefix becomes "/" and any controller path that starts with "/" would trigger a spurious Wheels.MapperArgumentInvalid throw. The outer StructKeyExists check does not guard against a zero-length value. A minimal fix is Len(arguments.package) > 0 added to the outer if condition, plus a matching spec asserting that .namespace("") (or direct $match() with package="") is not falsely rejected.

Defended points

  • Lucee 7 Left(str, 0) invariant (CLAUDE.md invariant 8). B noted this should have been confirmed explicitly. It was — in my actual review (PRR_kwDOABMUH88AAAABAsSx8A): "local.prefix is always packageAsPath & "/", so Len(local.prefix) >= 1 always. The Lucee 7 Left(str, 0) crash cannot trigger." B's critique was written against the placeholder; the analysis was already in the record.

Updated verdict

Request-changes: the two multi-line comment blocks are CLAUDE.md violations that should be trimmed before merge, and the empty-package edge case (Len(arguments.package) > 0 guard missing) is a correctness gap that warrants a fix and a spec.

- Snapshot `local.fromTo` / `local.originalTo` before the `to=` parse
  block so the error detail can distinguish `to=` vs direct `controller=`
  callers (Reviewer A nit).
- Add `Len(arguments.package) > 0` to the guard's outer condition so an
  empty package does not yield `prefix = "/"` and spuriously reject
  controllers whose path starts with a slash (Reviewer A response,
  Reviewer B round-1 missed-issue).
- Collapse multi-line block comments above the guard in `matching.cfc`
  and above the new `it()` group in `MatchingSpec.cfc` to one-liners
  to comply with CLAUDE.md style (both reviewers).
- Add a spec asserting `$match()` with `package = ""` and a controller
  starting with `/` is not falsely rejected.

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 21, 2026

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 2):

  • vendor/wheels/mapper/matching.cfc lines 305–340 — snapshot local.fromTo and local.originalTo before the to= parse block so the error detail can branch on whether the caller used to= or passed controller= directly. Replaces the misleading unconditional "likely from to=" hint that Reviewer A flagged (and Reviewer B verified) with two clean branches.
  • vendor/wheels/mapper/matching.cfc line 314 — added Len(arguments.package) > 0 to the guard's outer condition. Without it, an empty package yields local.prefix = "/" and any controller path starting with / would trigger a spurious Wheels.MapperArgumentInvalid throw (Reviewer A response, Reviewer B round-1 missed-issue).
  • vendor/wheels/mapper/matching.cfc lines 312–318 (old) — collapsed the 7-line block comment above the guard to one line referencing #2791, per CLAUDE.md "no multi-line comment blocks" rule (both reviewers).
  • vendor/wheels/tests/specs/mapper/MatchingSpec.cfc lines 201–204 (old) — collapsed the 4-line block comment above the new it() group to one line (both reviewers).
  • vendor/wheels/tests/specs/mapper/MatchingSpec.cfc after line 266 — added spec "Does not falsely reject when package is empty" calling $match() with package = "" and controller = "/users" to verify the empty-package fix.

Verification: ran wheels.tests.specs.mapper via the local SQLite test server. MatchingSpec reports 23 pass / 0 fail / 0 error; all 7 specs in the redundant-prefix describe group (including the new empty-package spec) pass.

Skipped (intentionally not in consensus):

  • Reviewer A's "Acceptable limitation (worth a comment)" about partial ancestor prefixes not being matched — Reviewer B did not echo this and adding a comment-only note would itself rub up against the multi-line-comment rule both reviewers just enforced.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: This is the updated round-1 state of the mapper redundant-prefix guard (PR for #2791). The second commit (fa83fc7e1) correctly addresses all three consensus findings: the empty-package edge case is now guarded, the multi-line block comment in matching.cfc is collapsed to one line, and the error detail now correctly distinguishes to= from direct controller= callers by snapshotting local.originalTo before the parse. Two minor items remain before merge: one 2-line comment block in the spec file that was not trimmed in round 1, and a local.hh value that produces a single # in the suggestion text where users need to type ##. Neither is a correctness bug; verdict: comment.


Conventions

1. 2-line comment block still present inside it() body — MatchingSpec.cfc lines 241–242

Round 1 correctly collapsed the 4-line header comment above the it() group, but the 2-line comment inside the "Allows controllers…" spec body was not touched:

it("Allows controllers whose name only happens to share a prefix with the namespace", function(){
    // "foobar/dashboard" inside .namespace("foo") is NOT a redundant prefix
    // — "foobar" != "foo". Must not be rejected.

CLAUDE.md: "Never write multi-line docstrings or multi-line comment blocks — one short line max." Collapse to one line, e.g.:

// "foobar/dashboard" inside .namespace("foo") is not a redundant prefix — "foobar" != "foo".

Correctness (UX nit)

2. local.hh = "##" produces a single # in the error suggestion — matching.cfc line 328

In a CFML string literal "##" evaluates to a single # character. The error detail therefore reads:

use to="dashboard#index" instead.

But users must type to="dashboard##index" in their CFML code to pass a string value containing a single #. The suggestion matches the runtime value, not the source text, so it could mislead a user who copies it verbatim and gets a CFML syntax error or a stray expression-delimiter warning.

Fix: use "####" (evaluates to ##) so the suggestion echoes exactly what to type:

local.hh = "####";
// detail now contains: use to="dashboard##index" instead.

Confirmed resolved (round 1 → round 2)

  • Empty-package edge case: Len(arguments.package) > 0 added to outer guard condition. ✓
  • New spec "Does not falsely reject when package is empty" added. ✓
  • 7-line block comment in matching.cfc collapsed to one line. ✓
  • 4-line header comment above spec group collapsed to one line. ✓
  • local.fromTo / local.originalTo snapshotted before the to= parse block; error detail now distinguishes the two call forms. ✓

Cross-engine

No new concerns introduced by the round-1 commit.

  • Left(arguments.controller, Len(local.prefix)) at line 325: Len(arguments.package) > 0 guarantees Len(local.prefix) >= 2, so Lucee 7 Left(str, 0) crash (CLAUDE.md invariant 8) cannot trigger. ✓
  • Case-insensitive == on both Lucee and Adobe handles mixed-case redundant prefixes correctly. ✓

Commits

Both commits carry DCO sign-off and conform to commitlint.config.js (type fix, scope mapper, subject ≤ 100 chars). ✓

@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 21, 2026

Wheels Bot — Reviewer B (round 1)

A's review on the updated SHA is substantive and accurate. Both findings are verified against the diff. Converging with A on comment -> changes needed.

Sycophancy

None detected. A's "comment" verdict is backed by two concrete, specific findings rather than a blanket LGTM.

False positives

None detected.

  • Finding 1 (2-line comment block): The diff for MatchingSpec.cfc does show two comment lines inside the "Allows controllers..." it() body. CLAUDE.md mandates one short line max. A's cite is accurate.
  • Finding 2 (local.hh = "##"): In a CFML double-quoted string, ## is the escape sequence for a single #. local.hh therefore holds the one-character string #, so the error suggestion renders as to="dashboard#index" -- which, if copied verbatim into source, either causes a CFML expression-evaluation error or a stray-delimiter warning rather than routing to the intended action. A's fix ("####" evaluates to ##) is correct and produces the source-accurate to="dashboard##index" hint.

Missed issues

None detected.

  • Len(arguments.package) > 0 correctly guards the Left(str, Len(local.prefix)) call so the Lucee 7 invariant-8 crash (Left(str, 0)) cannot trigger. A confirmed this; verified.
  • Commit subjects conform to commitlint.config.js; DCO sign-offs present on both commits.
  • No security, migration, DI, or deploy-path code touched.
  • CHANGELOG entry is accurate and under [Unreleased].

Verdict alignment

A's "comment" verdict (not request-changes) is consistent with the findings: neither issue is a correctness or cross-engine bug. Appropriate.

Convergence

Aligned with A. Both findings are real, verified, and concrete enough for address-review: collapse the 2-line comment inside the "Allows controllers..." spec to one line, and change local.hh = "##" to local.hh = "####".

- vendor/wheels/mapper/matching.cfc:328 — change local.hh = "##" to
  local.hh = "####" so the error-suggestion detail renders as
  to="dashboard##index" (source-correct CFML), not to="dashboard#index"
  (Reviewer A finding, Reviewer B verified).
- vendor/wheels/tests/specs/mapper/MatchingSpec.cfc:241–242 — collapse
  the 2-line comment inside the "Allows controllers..." spec body to a
  single line per CLAUDE.md "one short line max" rule (both reviewers).

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 21, 2026

Wheels Bot — Address Review (round 2)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1 on the round-1 commit):

  • vendor/wheels/mapper/matching.cfc line 328 — changed local.hh = "##" to local.hh = "####". In a CFML double-quoted string "##" evaluates to a single #, so the error suggestion was rendering as to="dashboard#index" — not the source-correct to="dashboard##index" users need to type. "####" evaluates to ##, so the suggestion now echoes exactly what to paste. (Reviewer A finding; Reviewer B verified.)
  • vendor/wheels/tests/specs/mapper/MatchingSpec.cfc lines 241–242 — collapsed the 2-line comment inside the "Allows controllers whose name only happens to share a prefix with the namespace" spec body to a single line, per CLAUDE.md "Never write multi-line docstrings or multi-line comment blocks — one short line max." (Reviewer A finding; Reviewer B verified — this was missed in the round-1 sweep that fixed the 4-line header above the group.)

Verification: ran the full core suite against Lucee 7 / SQLite via the local test server. wheels.tests.specs.mapper.MatchingSpec reports 23 pass / 0 fail / 0 error / 0 skipped (23 specs total, including the empty-package guard spec added in round 1). Global suite: 3698 pass / 0 fail / 0 error (16 skipped, all pre-existing browser specs).

No findings were skipped — both consensus items were applied.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: Round-2 review of the mapper redundant-prefix guard (fixes #2791). Both findings from the previous round — local.hh = "##" producing a single # in the error suggestion, and the 2-line comment block inside the "Allows controllers…" spec body — are correctly resolved in commit 092bde188. The three-commit history is now clean, all previous findings are confirmed closed, and no new issues were introduced. Verdict: approve (submitted as comment because GitHub blocks self-review on bot-authored PRs).


Confirmed resolved (round 2 → round 3)

1. local.hh = "##"local.hh = "####"matching.cfc line 328

"####" evaluates to ## at runtime, so the error-detail suggestion now renders as:

use to="dashboard##index" instead.

which is exactly the CFML source text the user must type (a string literal "dashboard##index" where ## escapes the separator #). The previous "##" evaluated to #, which would have produced to="dashboard#index" — syntactically valid CFML but carrying a stray expression-delimiter that would silently drop the action name at eval time. Fix is correct. ✓

2. 2-line comment collapsed — MatchingSpec.cfc lines 241–242

The two-line comment:

// "foobar/dashboard" inside .namespace("foo") is NOT a redundant prefix
// — "foobar" != "foo". Must not be rejected.

is now:

// "foobar/dashboard" inside .namespace("foo") is not a redundant prefix — "foobar" != "foo".

CLAUDE.md: "Never write multi-line docstrings or multi-line comment blocks — one short line max." Compliant. ✓


Residual checks (carried from rounds 1–2, still clean)

  • Empty-package guard: Len(arguments.package) > 0 in outer if — prevents local.prefix = "/" spuriously matching leading-slash controllers. ✓
  • local.fromTo / local.originalTo snapshotted before the to= parse block — error detail correctly distinguishes the two call forms. ✓
  • Single-line comment above guard in matching.cfc. ✓
  • Single-line comment above new spec group in MatchingSpec.cfc. ✓
  • 7 BDD specs covering: to= rejection, controller= rejection, nested namespaces, .package(), near-miss prefix (must NOT reject), canonical accepted form, and empty-package false-positive. ✓

Cross-engine

No new concerns. Len(arguments.package) > 0 guarantees Len(local.prefix) >= 2, so the Left(arguments.controller, Len(local.prefix)) call cannot hit the Lucee 7 Left(str, 0) crash (CLAUDE.md invariant 8). CFML case-insensitive == correctly handles mixed-case redundant prefixes on both Lucee and Adobe. ✓


Commits

All three commits carry Signed-off-by: claude[bot], use type fix, scope mapper, subjects ≤ 100 chars, and are not ALL-CAPS. Commit bodies describe the why, not just the what. ✓


Docs

CHANGELOG.md entry under [Unreleased] > Fixed is thorough and names the new exception type. Framework docs deferred to bot-update-docs.yml as noted in the checklist. ✓

@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 22, 2026

Wheels Bot — Reviewer B (round 1)

A's round-3 review on SHA 092bde188 is accurate and complete. All prior findings are confirmed closed; the two items resolved in this commit (the local.hh = "####" escaping fix and the 2-line comment collapse) were independently verified against the diff. No sycophancy, no false positives, no missed issues. Converging on approve.

Sycophancy

None detected. The "approve" verdict is backed by specific, cited line-level claims, not a blanket LGTM.

False positives

None detected.

  • local.hh = "####" evaluates to ##: Correct. In a CFML double-quoted string ## is the escape sequence for a single #, so "####" produces the two-character string ## at runtime. The error suggestion now renders as to="dashboard##index" (source-correct). A's analysis of the previous bug ("##" → single #to="dashboard#index") is accurate.
  • 2-line comment collapsed: Confirmed in diff — the spec body now has one line: // "foobar/dashboard" inside .namespace("foo") is not a redundant prefix — "foobar" != "foo".
  • local.fromTo / local.originalTo snapshot: Confirmed at diff lines +21 to +25; both locals are set before the parse block, after which arguments.to is deleted.
  • Len(arguments.package) > 0 prevents Left(str, 0): Confirmed. When Len(arguments.package) > 0, local.packageAsPath is at least 1 char and local.prefix is at least 2 chars, so Left(arguments.controller, Len(local.prefix)) never calls Left(str, 0). Lucee 7 invariant 8 cannot trigger.

Missed issues

None detected. Re-scan found nothing A skipped:

  • All 7 BDD specs are present and cover the described cases (verified by counting specs in diff).
  • Commit subjects conform to commitlint.config.js (type fix, scope mapper, not ALL-CAPS, all under 100 chars).
  • All 3 commits carry Signed-off-by: claude[bot] matching the configured git author identity.
  • No security, auth, migration, DI, or deploy-path code touched.
  • CHANGELOG entry is under [Unreleased] > Fixed, accurate, and names the new exception type.

Verdict alignment

A's "approve" (submitted as comment due to GitHub's self-review block) is consistent with the findings: no open correctness, cross-engine, or policy issues remain.

Convergence

Aligned with A. All prior consensus changes were applied correctly. No further changes are needed from this review cycle. The PR is ready for human merge.

@bpamiri bpamiri marked this pull request as ready for review May 22, 2026 12:48
…-redundant-to-foo-cont

Resolves a CHANGELOG.md conflict: both branches added entries under
[Unreleased] > Fixed. Kept all entries — the PR's #2791 mapper-prefix
guard sits at the top of the section above the develop-side entries
for #2790 (WheelsTest auto-bind), the model SELECT identifier quoting
fix, the packages-install dispatch fix, the BrowserTest NotWired
guard, and #2779 (BrowserTest base URL resolution).

vendor/wheels/mapper/matching.cfc and the MatchingSpec are untouched
by the merge.

Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri
Copy link
Copy Markdown
Collaborator

bpamiri commented May 22, 2026

Merged develop into the PR branch to clear the conflict.

Conflict was in CHANGELOG.md only — both branches added entries under [Unreleased] > Fixed. Resolution kept all entries: the #2791 mapper-prefix guard at the top, followed by the develop-side entries for #2790 (WheelsTest auto-bind), model SELECT identifier quoting, packages-install dispatch alias, BrowserTest NotWired guard, and #2779 (BrowserTest base URL resolution).

vendor/wheels/mapper/matching.cfc and vendor/wheels/tests/specs/mapper/MatchingSpec.cfc are byte-identical to the previously-approved 092bde188 — develop didn't touch them. Merge commit: b0f22ad.

Copy link
Copy Markdown
Contributor Author

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: The current head (b0f22adf5) is a merge commit that brings develop into the PR branch, resolving only a CHANGELOG.md conflict. The mapper guard code in matching.cfc and all seven BDD specs in MatchingSpec.cfc are explicitly noted in the commit message as untouched by the merge, and verified so by direct inspection. Every finding from rounds 1–3 remains resolved. No new issues were introduced. Verdict: approve (submitted as comment because GitHub blocks self-review on bot-authored PRs).


Confirmed merge resolution

The CHANGELOG.md conflict was resolved correctly: the PR's #2791 mapper-prefix guard entry sits at the top of [Unreleased] > Fixed, above the develop-side entries for #2790 (WheelsTest auto-bind), the model SELECT identifier quoting fix, the packages-install dispatch fix, the BrowserTest NotWired guard, and #2779 (BrowserTest base URL resolution). No entries were lost or reordered.


Confirmed resolved (rounds 1–3, still clean)

  • Empty-package guard: Len(arguments.package) > 0 in outer if at line 319. ✓
  • local.fromTo / local.originalTo snapshotted before the to= parse block (lines 306–307); error detail correctly distinguishes the two call forms. ✓
  • local.hh = "####" at line 328 evaluates to ## at runtime, producing to="dashboard##index" in the suggestion text — exactly what the user must type. ✓
  • Single-line comment above guard in matching.cfc (line 316). ✓
  • Single-line comment inside the "Allows controllers…" spec body in MatchingSpec.cfc (line 241). ✓
  • 7 BDD specs covering: to= rejection, controller= rejection, nested namespaces, .package(), near-miss prefix (must NOT reject), canonical accepted form, and empty-package false-positive. ✓

Cross-engine

No new concerns from the merge. Len(arguments.package) > 0 guarantees Len(local.prefix) >= 2, so Left(arguments.controller, Len(local.prefix)) cannot hit the Lucee 7 Left(str, 0) crash (CLAUDE.md invariant 8). ✓


Commits

The merge commit carries Signed-off-by: Peter Amiri <peter@alurium.com> — DCO present. The three bot-authored commits below it (42089da71, fa83fc7e1, 092bde188) all carry Signed-off-by: claude[bot], use type fix, scope mapper, subjects ≤ 100 chars, and are not ALL-CAPS. ✓

@wheels-bot
Copy link
Copy Markdown
Contributor Author

wheels-bot Bot commented May 22, 2026

Wheels Bot — Reviewer B (round 1)

A's review on the merge commit is accurate and complete. The diff is a single CHANGELOG.md entry resolving a conflict; all code in matching.cfc and MatchingSpec.cfc is unchanged from the previously-approved 092bde188. Every claim A made checks out against the diff. Converging on approve.

Sycophancy

None detected. The "approve" verdict is backed by explicit, line-level verification of every previously-resolved finding — not a blanket LGTM.

False positives

None detected. All cited items are confirmed in the diff:

  • local.fromTo = false; local.originalTo = "" snapshot before the parse block: present.
  • local.hh = "####": present; correctly evaluates to ## at runtime.
  • Single-line guard comment in matching.cfc: confirmed one line.
  • 7 BDD specs (counted): all present, including the empty-package spec added in round 1.
  • Len(arguments.package) > 0 in the outer guard condition: present; guarantees Len(local.prefix) >= 2, so the Lucee 7 Left(str, 0) crash cannot trigger.
  • CHANGELOG entry: accurate, positioned at the top of [Unreleased] > Fixed.

Missed issues

None detected. The only new change is the CHANGELOG.md merge-conflict resolution, which A verified correctly. No security, migration, DI, or deploy-path code was touched. Commit DCO sign-offs and subject format are clean.

Verdict alignment

A's "approve" verdict is consistent with the findings: a merge-only commit whose sole change is an accurate CHANGELOG entry, with all previously-approved code intact. Appropriate.

Convergence

Aligned with A. The PR is review-clean on this SHA. No further changes needed — ready for human merge.

@bpamiri bpamiri merged commit 950a7f8 into develop May 22, 2026
6 checks passed
@bpamiri bpamiri deleted the fix/bot-2791-routing-inside-namespace-foo-redundant-to-foo-cont branch May 22, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routing: inside .namespace("foo"), redundant to="foo/controller##action" produces opaque Foocontroller lookup; should reject or strip the prefix

1 participant