Skip to content

fix(router): except= normalization, scope option passthrough, end() underflow guard - #2925

Merged
bpamiri merged 1 commit into
developfrom
peter/review-w2-review-mapper-resources-scoping
Jun 10, 2026
Merged

bpamiri merged 1 commit into
developfrom
peter/review-w2-review-mapper-resources-scoping

Conversation

@bpamiri

@bpamiri bpamiri commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes three mapper findings from the wave-2 framework review (package mapper-resources-scoping):

  • resources()/resource() only/except lists are now normalized (Trim + LCase per item, empties dropped) and except filtering uses array operations instead of the old case-sensitive ReReplace("\b...\b"), so except="Delete" or except="new, delete" can no longer silently leave an excluded destructive route registered. Unknown action names throw Wheels.InvalidResource in development (showErrorInformation) and are dropped otherwise.
  • namespace(), package(), and group() now forward all arguments to scope(), so options like middleware and binding are no longer silently dropped. api() and version() already delegate to group() and inherit the fix.
  • end() now throws a clear Wheels.InvalidRoute DSL error on scope-stack underflow (an extra end()) instead of a raw array-index engine error.

Findings addressed

  • routing:1 (R1 [High]) — resource()/resources() except= filtering is case-sensitive and untrimmed; excluded destructive routes stay registered @ vendor/wheels/mapper/resources.cfc:103-125 (normalized only/except + array-based filter) with new helper $normalizeRestActions() @ vendor/wheels/mapper/resources.cfc:268. The fix sits in the shared resource() path so both singular and plural are covered; the normalized list feeds the exact-match ListFind consumers in vendor/wheels/mapper/mapping.cfc.
  • routing:5 (R5 [Medium]) — namespace()/package()/group()/api()/version() silently drop middleware, binding, and other scope() options @ vendor/wheels/mapper/scoping.cfc:113 (namespace()), vendor/wheels/mapper/scoping.cfc:128 (package()), vendor/wheels/mapper/scoping.cfc:172-183 (group() copies every passed argument except callback); api()/version() forward argumentCollection to group() and inherit the fix.
  • routing:11 (R11 [Low]) — end() has no scope-stack underflow guard @ vendor/wheels/mapper/mapping.cfc:30-37 (ArrayIsEmpty guard throws Wheels.InvalidRoute, a pre-existing error type, before any scopeStack[1] access; $resetScopeStack seeds a root entry, so internal recursive end() calls never hit an empty stack).

Findings verified already-fixed

None — all three findings were verified live on origin/develop before fixing (the case-sensitive LCase+ReReplace block in resources.cfc, the subset-forwarding namespace()/package()/group() wrappers in scoping.cfc, and the unguarded scopeStack[1] access in mapping.cfc were all still present).

Source

Internal multi-agent framework review 2026-06-09, wave 2, package mapper-resources-scoping.

Tests

  • New spec: vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc — 12 tests covering all three behaviors (except/only normalization incl. casing + whitespace, dev-mode throw + prod-mode ignore for unknown actions, middleware/binding passthrough for namespace()/package()/group()/api()/version(), and the end() underflow error). 11 of 12 fail against the pre-fix code; the prod-ignore test is a pass-pre-fix regression guard.
  • Local verification: ran the spec bundle in the worktree via the prebuilt Lucee 7 docker image + SQLite (testBundles=wheels.tests.specs.mapper.MapperRobustnessSpec): 12 pass, 0 fail, 0 error (HTTP 200).
  • Full cross-engine coverage (Adobe 2023/2025, BoxLang, Lucee 6) deferred to the CI compat matrix, which should be confirmed green before merge — particularly the new spec bundle on Adobe 2023/2025 and BoxLang.

Cross-engine notes

  • New helper $normalizeRestActions() is public with a $ prefix (mixin-integration invariant 7).
  • No closures, no reserved-scope parameter names, no Left(str, 0), no attributeCollection = arguments in the framework changes; group()'s argument copy uses the StructKeyExists + IsNull guarded loop pattern (mirrors scope()'s BoxLang branch).
  • Spec escapes ## correctly, contains no inline closures as constructor named args and no local.X-in-catch patterns, and follows the NestedResourcesSpec construction and mapperModernSpec application-route snapshot/restore precedents.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

…nderflow guard

Addresses three wave-2 review findings in the mapper (routing:1, routing:5,
routing:11):

- resources()/resource() only/except lists are now normalized (Trim + LCase
  per item) and except filtering uses array operations instead of a
  case-sensitive ReReplace, so except="Delete" or except="new, delete" can
  no longer silently leave excluded destructive routes registered. Unknown
  action names throw Wheels.InvalidResource when showErrorInformation is
  enabled (development) and are dropped otherwise.
- namespace(), package(), and group() now forward all arguments to scope(),
  so options like middleware and binding are no longer silently dropped.
  api() and version() delegate to group() and inherit the fix.
- end() now throws a clear Wheels.InvalidRoute error on scope-stack
  underflow (an extra end()) instead of a raw array-index engine error.

Specs: vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc covers all
three behaviors and fails against the pre-fix code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

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 correctly fixes three mapper bugs (case-sensitive except= filtering, silent argument-drop in namespace()/package()/group(), and an unguarded end() underflow). The implementations are sound, the cross-engine review passes all checks, and the new spec follows established test infrastructure patterns. Two minor nits below — neither blocks merge.


Correctness

The three fixes all check out:

R1 — except= normalization (vendor/wheels/mapper/resources.cfc:100-123):
The old code called LCase(arguments.only) on the whole string, so only="Index, Show" produced "index, show" — a list with a leading space on the second element — causing ListFind's exact-match to miss "show" and silently skip that route. The new $normalizeRestActions() trims each element individually and produces a clean lowercase list. Correct.

R5 — scope wrapper passthrough (vendor/wheels/mapper/scoping.cfc):
namespace() and package() now use scope(argumentCollection = arguments, $call = "namespace/package"). The explicit named $call argument overrides the argumentCollection correctly (CFML named args beat argumentCollection). group()'s forward loop excludes only callback, then forces $call = "group" below the loop — so even if a caller accidentally passes $call, the post-loop assignment always wins. Correct.

R11 — end() underflow guard (vendor/wheels/mapper/mapping.cfc:30-36):
$resetScopeStack() (Mapper.cfc:275-279) seeds exactly one root {"$call": "$draw"} entry. The normal flow pops it when the outer end() fires. The guard only triggers when the stack is genuinely empty. Recursive end() calls inside the resources processing path always find at least the root entry below them, so they are unaffected. Correct.

Minor concern — $normalizeRestActions() does not distinguish singular vs. plural resource:
vendor/wheels/mapper/resources.cfc:269: local.validActions = "index,new,create,show,edit,update,delete" includes index, but a singular resource() call has a default action set of new,create,show,edit,update,delete (no index). Passing only="index" to resource() (singular) produces 0 routes with no error in dev or prod mode — index passes the validation guard but the resource generator never emits an index route. This was already the case before this PR (the old LCase path passed it through silently too), so it is not a regression. But the new dev-mode validation misses this class of user error. A follow-up could scope validActions to the caller's default action set rather than the combined list.


Tests

Coverage gap — singular resource() path not exercised:
vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc tests all three normalization behaviors against resources() (plural). Both singular and plural call through to the same $normalizeRestActions() helper, so the code path is the same — but a single resource(name="profile", except="Delete") spec would serve as a regression guard if the dispatch ever diverges. Low-priority follow-up.

Shallow copy vs. deep copy in beforeAll():
MapperRobustnessSpec.cfc:16-18:

_originalStaticRoutes = StructKeyExists(application.wheels, "staticRoutes") ? StructCopy(
    application.wheels.staticRoutes
) : {};

_originalRoutes uses Duplicate() (deep copy) but _originalStaticRoutes uses StructCopy() (shallow copy). If a future spec were to mutate a nested struct inside an existing named-route entry, the shallow copy would fail to restore it. Not a problem for the current tests — just worth aligning to Duplicate for consistency.

application.wheels.showErrorInformation mutation pattern (lines 78-97):
Confirmed consistent with the established pattern in validationsSpec.cfc, primaryKeySpec.cfc, and paginationHelpersSpec.cfc. $get() reads from application[$appKey()]; application.$wheels and application.wheels are the same struct reference in a live app so setting either one is sufficient. No issue.

No local.X inside catch (BoxLang anti-pattern):
Both showErrorInformation restore blocks declare var originalSetting before try/finally, and the restore assignment in finally reads that pre-declared var — not a local.X write inside a catch body. Clean.


Cross-Engine

  • $normalizeRestActions() declared public with $ prefix — mixin-integration invariant (CLAUDE.md #7) satisfied.
  • No closures, no Left(str, 0), no attributeCollection = arguments, no bracket-notation calls inside closures anywhere in the changed files.
  • The for (local.key in arguments) / StructKeyExists + !IsNull guard pattern in group() mirrors the BoxLang-safe loop already used in scope()'s BoxLang branch.
  • Inline closures passed as named args to m.group(), m.api(), m.version() in the spec are method calls, not new Component(callback = ...) constructor calls — Adobe CF anti-pattern #5 does not apply.

Commits

Single commit f134e956c:

  • Header: fix(router): except= normalization, scope option passthrough, end() underflow guard — 83 chars, valid type, unrestricted scope, not ALL-CAPS.
  • DCO: Signed-off-by: Peter Amiri <peter@alurium.com> present.
  • Body explains the "why" (case-sensitivity left excluded destructive routes registered; silent arg-drop; raw engine error on underflow).

Docs

CHANGELOG.md omission explicitly deferred to campaign end — understood, not a blocker.

@wheels-bot

wheels-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot -- Reviewer B (round 1)

A's review is thorough and checks out against the actual diff. No sycophancy, no false positives, no missed issues of substance. I agree the PR is merge-ready with the two minor follow-up nits A flagged.

Sycophancy

None detected. A backed every claim with specific file+line citations and verified all three fixes against the diff before endorsing them. The COMMENTED state is correct per bot rails.

False positives

None detected. I re-read every cited code section:

  • R1: LCase(arguments.only) on the whole string leaving a leading-space on the second list element, and the case-sensitive ReReplace on except, both confirmed in the diff exactly as A described.
  • R5: old namespace() / package() only forwarded named-arg subsets; old group() only copied name, path, constraints. New code uses argumentCollection = arguments forwarding plus a post-loop local.args.$call = "group" override. Named args beat argumentCollection in CFML; post-loop assignment unconditionally wins. Correct.
  • R11: ArrayIsEmpty(variables.scopeStack) guard is in the right place; the test calling m.$draw().end() then m.end() exercises the path correctly.
  • Anti-pattern 5: A correctly distinguished inline closures passed to method calls (.group(), .api(), .version()) from the constructor-named-arg form (new Foo(callback = function(){})) that causes Adobe CF compilation failures. The spec has no constructor calls with inline closure args.
  • Cross-engine: $normalizeRestActions() is public with $ prefix (mixin invariant 7 satisfied); no Left(str, 0), no attributeCollection = arguments on built-in tags, no bracket-notation calls inside closures. All confirmed.

Missed issues

None detected. Re-scan findings:

  • argumentCollection = arguments on CFC method calls (scope(argumentCollection = arguments, $call = "namespace")) is not anti-pattern 10, which applies to built-in CFML tags only.
  • $get("showErrorInformation") in mapper context: $get() is a Global.cfc mixin mixed into the mapper at startup. A's local run (12 pass, 0 fail, 0 error) confirms it resolves.
  • ## escaping in the spec: all to = "controller##action" strings are correctly doubled. No bare # in string literals.
  • for (local.key in arguments) in group(): standard struct iteration, no closure, no scope-capture issue.

A's two nits (singular resource() path not exercised; StructCopy vs Duplicate inconsistency in beforeAll()) are both legitimate and correctly labeled non-blocking.

Verdict alignment

COMMENTED with "neither blocks merge" is consistent with the findings. Correct.

Convergence

Aligned. A's analysis is accurate, the diff is clean, and the two open items are follow-up candidates not blockers. Joint recommendation: merge as-is.

@bpamiri
bpamiri merged commit 053185e into develop Jun 10, 2026
7 checks passed
@bpamiri
bpamiri deleted the peter/review-w2-review-mapper-resources-scoping branch June 10, 2026 07:31
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.

1 participant