fix(mapper): reject redundant namespace prefix in to= and controller=#2794
Conversation
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>
There was a problem hiding this comment.
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.prefixis alwayspackageAsPath & "/", soLen(local.prefix) >= 1always. The Lucee 7Left(str, 0)crash (CLAUDE.md invariant 8) cannot trigger.- No
struct.map()member-function collision. - No
applicationscope 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 -- 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):
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 — 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. SycophancyNone detected. A gives a specific "comment" verdict backed by concrete findings rather than a blanket LGTM. False positivesNone detected. Every claim checks out against the actual diff:
Missed issuesMulti-line comment blocks violate CLAUDE.md style rules. CLAUDE.md: "Don't add error handling… multi-line comment blocks — one short line max."
Neither is a correctness issue, but both are concrete CLAUDE.md style violations A should have flagged. Verdict alignmentA's "comment" verdict (no blocking issues, one UX nit) is consistent with the findings. There is nothing in the diff that warrants ConvergenceAligned on verdict: both A and B agree this is "comment" territory — no blocking correctness, security, or cross-engine issues. The consensus changes are:
All three are minor, concrete, and safe to apply automatically. |
There was a problem hiding this comment.
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 theThrowmessage 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 theit()group should be one line or cut entirely. -
Empty-package edge case (correctness, not style). If
arguments.packageis ever set to an empty string,local.prefixbecomes"/"and any controller path that starts with"/"would trigger a spuriousWheels.MapperArgumentInvalidthrow. The outerStructKeyExistscheck does not guard against a zero-length value. A minimal fix isLen(arguments.package) > 0added to the outerifcondition, plus a matching spec asserting that.namespace("")(or direct$match()withpackage="") 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.prefixis alwayspackageAsPath & "/", soLen(local.prefix) >= 1always. The Lucee 7Left(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 — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 2):
Verification: ran Skipped (intentionally not in consensus):
The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
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) > 0added to outer guard condition. ✓ - New spec "Does not falsely reject when package is empty" added. ✓
- 7-line block comment in
matching.cfccollapsed to one line. ✓ - 4-line header comment above spec group collapsed to one line. ✓
local.fromTo/local.originalTosnapshotted before theto=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) > 0guaranteesLen(local.prefix) >= 2, so Lucee 7Left(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 — 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 SycophancyNone detected. A's "comment" verdict is backed by two concrete, specific findings rather than a blanket LGTM. False positivesNone detected.
Missed issuesNone detected.
Verdict alignmentA's "comment" verdict (not ConvergenceAligned 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 |
- 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 — Address Review (round 2)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1 on the round-1 commit):
Verification: ran the full core suite against Lucee 7 / SQLite via the local test server. 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 |
There was a problem hiding this comment.
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) > 0in outerif— preventslocal.prefix = "/"spuriously matching leading-slash controllers. ✓ local.fromTo/local.originalTosnapshotted before theto=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 — Reviewer B (round 1)A's round-3 review on SHA SycophancyNone detected. The "approve" verdict is backed by specific, cited line-level claims, not a blanket LGTM. False positivesNone detected.
Missed issuesNone detected. Re-scan found nothing A skipped:
Verdict alignmentA'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. ConvergenceAligned 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. |
…-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>
|
Merged Conflict was in
|
There was a problem hiding this comment.
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) > 0in outerifat line 319. ✓ local.fromTo/local.originalTosnapshotted before theto=parse block (lines 306–307); error detail correctly distinguishes the two call forms. ✓local.hh = "####"at line 328 evaluates to##at runtime, producingto="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 — 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 SycophancyNone detected. The "approve" verdict is backed by explicit, line-level verification of every previously-resolved finding — not a blanket LGTM. False positivesNone detected. All cited items are confirmed in the diff:
Missed issuesNone 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 alignmentA'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. ConvergenceAligned with A. The PR is review-clean on this SHA. No further changes needed — ready for human merge. |
Summary
Inside
.namespace("foo")(or equivalent.scope()/.package()), writingto="foo/dashboard##index"instead ofto="dashboard##index"silently produced afoo.foo/dashboardcontroller path that downstream got flattened to aFoodashboard-style class lookup with an opaqueWheels.ViewNotFounderror. Users had no way to trace the bizarre class name back to the offending route definition.$match()(invendor/wheels/mapper/matching.cfc) now detects when the parsed controller starts with the scope's package converted to slash form and throwsWheels.MapperArgumentInvalidat 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
Feature Completeness Checklist
Signed-off-by:(usegit commit -s); see CONTRIBUTING.mdvendor/wheels/tests/specs/mapper/MatchingSpec.cfccovering: namespace +to=prefix, namespace +controller=prefix, nested namespaces,.package()helper, near-miss prefix overlap (must NOT reject), and the canonical accepted formbot-update-docs.ymlas a follow-up commitbot-update-docs.ymlbot-update-docs.yml[Unreleased]wheels.tests.specs.mapper76/76,wheels.tests.specs.dispatch96/96,wheels.tests.specs.controller423/423 on Lucee 7 + SQLiteTest 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/dashboard→Wheels.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 toapp/controllers/datapai/Dashboard.cfc#index.A human review is required before merge.