Skip to content

fix(router): scope()/namespace() honor callback= and auto-close the scope - #3100

Merged
bpamiri merged 4 commits into
developfrom
fix/bot-3072-router-scope-namespace-silently-ignore-callback-ro
Jun 12, 2026
Merged

bpamiri merged 4 commits into
developfrom
fix/bot-3072-router-scope-namespace-silently-ignore-callback-ro

Conversation

@wheels-bot

@wheels-bot wheels-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

scope() in vendor/wheels/mapper/scoping.cfc declared no callback argument, so the documented .scope(path="/x", callback=function(map){ ... }) form — and namespace() / package() / controller(), which all forward to scope() — silently dropped it. The extra named argument landed in arguments, was prepended onto the scope stack, and was never invoked. Two failures resulted, both silent: (1) the callback never ran, so its routes were never registered (Wheels.RouteNotFound / 404), and (2) the scope was never closed, so every route declared afterward (resources, root, even wildcard) inherited the scope's path prefix and middleware — which is why misrouted URLs like /admin/posts would respond 200 while the intended public /posts 404'd, making the breakage very hard to diagnose.

This PR implements the issue's preferred direction: scope() now consumes callback exactly the way group() already does — it runs callback(this) to declare the nested routes and then auto-end()s the scope. Because namespace()/package()/controller() forward here, they all gain consistent callback support. The implementation reuses group()'s cross-engine-proven IsCustomFunction + callback(this) shape (single code path, no engine branching).

Related Issue

Refs #3072

(Scoped to the framework fix + specs. The five guide MDX call sites the issue lists, and the CLAUDE.md note, are handled separately by bot-update-docs.yml, so this uses Refs rather than Closes to avoid auto-closing the tracking issue before the docs follow-up lands.)

Type of Change

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

Feature Completeness Checklist

  • DCO sign-off -- commit carries Signed-off-by: (git commit -s)
  • Tests -- vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc adds a scope()/namespace()/package() callback form block: callback routes register under the scope path, the scope auto-closes (routes declared after are not swallowed), and scope middleware does not leak onto later routes. Written failing-first (5 fails reproducing the bug), then passing after the fix.
  • Framework Docs -- left for bot-update-docs.yml follow-up
  • AI Reference Docs -- left for bot-update-docs.yml follow-up
  • CLAUDE.md -- left for bot-update-docs.yml follow-up
  • Changelog fragment -- changelog.d/3072-scope-namespace-callback.fixed.md
  • Test runner passes -- see Test Plan

Test Plan

Local Lucee 7 (7.0.1.100) + SQLite, via the running matrix server:

  1. Failing-first — the 5 new specs failed exactly on the bug:
    • The key(s) [pattern] does not exist (callback never ran → no route registered)
    • Expected [/public-index] but received [/limited/public-index] (scope never closed → route after swallowed)
    • The key(s) [middleware] does not exist (scoped route never created)
  2. After the fix — MapperRobustnessSpec 17/17 pass; the whole wheels.tests.specs.mapper directory plus mapperSpec (64) and mapperModernSpec (35) are green.
  3. No regression — full core suite: 4444 specs, 0 fail, 0 error (18 pre-existing skips).

Adobe 2018/2021/2023/2025 and BoxLang are exercised by the PR's compat-matrix CI. The change adds no engine-specific path — it mirrors group()/api()/version(), whose callback handling already passes on every engine (the group()/api()/version() middleware specs in this same file).

…cope

scope() declared no callback argument, so the documented `.scope(path="/x", callback=function(map){...})` form (and namespace()/package()/controller(), which forward to scope()) silently dropped it: the callback never ran (its routes 404'd) and nothing closed the scope, so every route declared after inherited the scope's path prefix and middleware.

scope() now consumes callback the same way group() does — runs callback(this) then auto-end()s — using the cross-engine-proven IsCustomFunction guard. Adds MapperRobustnessSpec coverage for the scope/namespace/package callback forms plus the no-swallow and no-middleware-leak guarantees for routes declared after the block.

Refs #3072

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Three focused updates:
- Anti-pattern #3: note that scope()/namespace()/package()/controller()
  also accept callback= and auto-close the scope (not just resources())
- Middleware Quick Reference: use callback form for route-scoped .scope()
- Route Model Binding: use callback form for the .scope(binding=true) example

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
@wheels-bot

wheels-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Docs updated

Added a doc commit to this PR:

  • CLAUDE.md — Extended anti-pattern New master #3 to note that scope(), namespace(), package(), and controller() also accept callback= and auto-close the scope (not just resources()); updated the Middleware Quick Reference and Route Model Binding examples from the manual two-step .scope(...) ... .end() form to the preferred callback= form.

The five user-guide MDX pages (rate-limiting, cors, multi-tenancy, route-model-binding, routing) already use the correct callback= syntax and required no changes — the bug was in the framework, not the docs.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer

TL;DR — This PR makes scope() consume a callback= argument the same way group() already does (run callback(this), then auto-end()), so the documented .scope(path="/api", callback=...) form — and namespace()/package()/controller(), which forward to scope() — stop silently dropping the callback and leaking the scope's path/middleware onto every later route. The implementation is correct, mirrors proven prior art in the same file, and ships failing-first specs that assert the exact failure modes from #3072. Verdict: comment — no blocking findings, two minor nits below.

Correctness

Verified clean — no findings. Evidence checked:

  • Consume-before-prepend ordering is right. The callback is lifted into local and deleted from arguments at vendor/wheels/mapper/scoping.cfc:37-40, before ArrayPrepend(variables.scopeStack, arguments) at line 103, so the callback can no longer pollute the scope stack. Invocation + end() happen after the prepend (lines 109-112), so nested routes correctly see the new scope.
  • Cross-engine guard is proven prior art, not new risk. The StructKeyExists(...) && IsCustomFunction(...) shape is identical to group() at scoping.cfc:207 and resources() at vendor/wheels/mapper/resources.cfc:44, both of which already pass the full engine matrix — including no-callback call sites (vendor/wheels/tests/specs/mapperModernSpec.cfc:94 calls .group(path="admin") with no callback). The BoxLang null-argument branch at scoping.cfc:94-99 is untouched and unaffected, since callback is removed from arguments before that copy runs.
  • Wrapper forwarding works without declaring the param. namespace()/package()/controller() forward via argumentCollection = arguments (scoping.cfc:134/149/163), so an extra callback= named argument reaches scope()'s declared any callback on every engine; the specs confirm this for namespace() and package().
  • Behavior-change blast radius is nil. Anyone who previously wrote .scope(callback=fn).end() had completely broken routing (the callback's routes 404'd per #3072), so no working code depends on the old open-scope behavior; a now-redundant trailing .end() fails loudly with Wheels.InvalidRoute ("Unbalanced end() call", vendor/wheels/mapper/mapping.cfc:30-36) rather than silently misrouting.

One non-blocking observation: a non-function callback value (e.g. a string) is consumed and silently ignored, leaving the scope open. That exactly matches group()'s existing semantics (scoping.cfc:207), so it's consistent — but if you ever harden this, a dev-mode throw would fit the repo's recent "fail loudly" direction (cf. the end() underflow guard). Not requested here.

Tests

  • Coverage is good: callback-runs + path-prefix, auto-close (no route swallowing), middleware no-leak, namespace() package+path prefixing, and package() package-without-URL-prefix are all asserted in vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc:183-281, written failing-first per the PR body. The spec extends wheels.WheelsTest (BDD, not RocketUnit) and reuses the file's existing beforeAll/afterAll route-snapshot hygiene (lines 12-24).
  • Minor nit: controller() also gains callback support via the same forwarding path (scoping.cfc:163) and is named in the new CLAUDE.md line, but has no spec. It's deprecated per its docblock, so this is optional — a one-it() addition would close the gap if you want the documented surface fully pinned.

Docs

  • Changelog fragment changelog.d/3072-scope-namespace-callback.fixed.md is present, correctly named (<slug>.fixed.md), and is a complete bullet line — no direct CHANGELOG.md edit. Good.
  • Minor nit: the PR body's Feature Completeness Checklist still says CLAUDE.md — left for bot-update-docs.yml follow-up, but the second commit (5e2302f9, docs: update scope()/namespace() callback= examples in CLAUDE.md (#3072)) updates CLAUDE.md in this PR. The CLAUDE.md edits themselves are accurate against the implementation (the auto-close note and both example rewrites match scoping.cfc); just tick that checklist row so the body reflects the actual diff. The guides MDX call sites remain correctly deferred.

Commits

Both commits conform to commitlint.config.js: valid types (fix, docs), headers ≤ 100 chars, not ALL-CAPS, and bodies explain the why (silent drop → 404 + scope leak). Both carry Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> matching the commit author. The PR title is itself a valid conventional-commit header, which is what the squash-merge gate lints.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer

TL;DR — Re-review at head 5e2302f9 (the prior wheels-bot review was marked against 19acb5c3). This PR makes scope() consume callback= the way group() already does — run callback(this), then auto-end() — so the documented .scope(path="/api", callback=...) form, and the namespace()/package()/controller() wrappers that forward to scope(), stop silently dropping the callback and leaking the scope's path/middleware onto every later route. The implementation is correct, reuses cross-engine-proven prior art in the same file, ships failing-first specs, and the in-PR CLAUDE.md doc updates are accurate against the code. Verdict: comment — no blocking findings; the same two minor nits from the previous round remain open.

Correctness

Verified clean — no findings. Evidence checked at this head:

  • Consume-before-prepend ordering. callback is lifted into local and deleted from arguments at vendor/wheels/mapper/scoping.cfc:37-40, before ArrayPrepend(variables.scopeStack, arguments) at line 103, so it never pollutes the scope stack. Invocation + end() (lines 109-112) happen after the prepend, so nested routes see the new scope.
  • Wrapper forwarding. namespace()/package()/controller() forward via argumentCollection = arguments (scoping.cfc:134/149/163), so callback= reaches scope()'s declared any callback param; the new specs confirm this for namespace() and package().
  • No working code regresses. Anyone who previously wrote .scope(callback=fn) had broken routing (the callback never ran — that is the bug per #3072), so nothing depends on the old open-scope behavior. A now-redundant trailing .end() fails loudly via the underflow guard ("Unbalanced end() call", vendor/wheels/mapper/mapping.cfc:33) rather than silently misrouting.
  • Error path: a callback that throws leaves the scope open — identical to group()'s existing semantics (scoping.cfc:207-210), and routes-config exceptions abort app start anyway. Consistent, non-blocking.

Cross-engine

Verified clean. The one new shape I probed: on BoxLang, declared-but-unpassed params can sit in arguments (that's why the copy loop at scoping.cfc:94-99 checks isNull(arguments[local.k])), so I checked whether line 37's StructKeyExists(arguments, "callback") could fire on every no-callback scope() call there. Refuted by prior art: group() uses the identical StructKeyExists + IsCustomFunction guard (scoping.cfc:207), as does resources() (vendor/wheels/mapper/resources.cfc:44), and mapperModernSpec.cfc:94 exercises .group(path="admin") with no callback on the full engine matrix today. The guard demonstrably does not fire for unpassed callbacks on any engine, so the new local.callback = arguments.callback assignment is unreachable in that case. The local.callback(this) invocation is a scope-qualified call (not the Adobe-crashing obj["key"]() bracket form), and the inline-closure-as-named-arg shape in the specs is a method call, not a constructor — both already proven by the group()/api()/version() specs in the same file.

Tests

  • Coverage is good: callback-runs + path prefix, auto-close (no route swallowing), middleware no-leak, namespace() package+path prefixing, and package() package-without-URL-prefix, at vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc:183-281. BDD spec extending wheels.WheelsTest, reusing the file's existing route-snapshot hygiene (beforeAll/afterAll, lines 12-24). PR body documents the failing-first run.
  • Minor nit (carried over): controller() also gains callback support via the same forwarding path (scoping.cfc:163) and is named in the new CLAUDE.md line, but has no spec. It's deprecated per its docblock (scoping.cfc:153), so optional — one it() would pin the documented surface fully.

Docs

  • Changelog fragment changelog.d/3072-scope-namespace-callback.fixed.md is present, correctly named, a complete bullet line, no direct CHANGELOG.md edit. Good.
  • The CLAUDE.md edits (anti-pattern #3 note; route-model-binding and middleware examples switched to the callback form with the now-redundant .end() removed) are accurate against the implementation.
  • Minor nit (carried over): the PR body's Feature Completeness Checklist still says CLAUDE.md — left for bot-update-docs.yml follow-up, but commit 5e2302f9 updates CLAUDE.md in this PR. Please tick that row so the body reflects the actual diff. The guides MDX call sites remain correctly deferred.

Commits

Both commits (19acb5c3 fix(router), 5e2302f9 docs) conform to commitlint.config.js: valid types, headers ≤ 100 chars, not ALL-CAPS, and both carry Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> matching the commit author email.

Peter Amiri and others added 2 commits June 12, 2026 06:49
…sides now that scope() honors it

#3092 and #3095 landed caution asides on develop (routing.mdx, cors.mdx,
rate-limiting.mdx) stating scope()/namespace() silently ignore callback=,
tracked in #3072. This branch fixes exactly that, so merge develop in and
rewrite the three statements: document the now-supported callback= form
(auto-closing the scope), keep the unclosed-scope warning for the explicit
.end() form, and note that releases <= 4.0.3 still ignore callback=. Adds a
compile-tested namespace(callback=) example to routing.mdx, matching the
already-correct callback examples in route-model-binding.mdx and
multi-tenancy.mdx.

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

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer

TL;DR — Re-review at head 44b20820 (prior wheels-bot reviews covered 19acb5c3 and 5e2302f9, both comment-state). The delta since the last reviewed head is a merge of origin/develop plus one docs commit that flips the three "callback= silently ignored" guide asides (routing.mdx, cors.mdx, rate-limiting.mdx) to document the now-supported callback= form. The framework change itself (vendor/wheels/mapper/scoping.cfc) is byte-identical to what the previous two reviews verified — the merge did not touch it — and the new guide text is accurate against the implementation and the release history. Verdict: comment — no blocking findings; three minor nits below (one new, two carried over).

Correctness

Verified clean at this head — no findings.

  • The merge introduced no code drift. git show 44b20820:vendor/wheels/mapper/scoping.cfc shows the consume-before-prepend block (lines 37–40), the ArrayPrepend (line 103), and the IsCustomFunction + callback(this) + end() block (lines 105–112) exactly as reviewed at 19acb5c3/5e2302f9. The new specs in MapperRobustnessSpec.cfc:183-281 are likewise unchanged.
  • New guide claims are accurate. "runs the callback and closes the scope automatically when it returns" matches scoping.cfc:109-111 (local.callback(this); end();). The "(4.0.3 and earlier)" qualifier is correct: v4.0.3 is the latest release (cut 2026-06-10, per gh release list) and predates this fix, so every release ≤ 4.0.3 has the silently-ignored behavior — and the phrasing stays correct regardless of which version the fix ships in.
  • The flipped asides keep the true part. All three files retain the unclosed-scope warning for the explicit .end() form (e.g. routing.mdx:122), which is still real — only the "callback is silently ignored" claim was removed, version-scoped to pre-fix releases. A git grep sweep at this head finds no remaining stale "scope() does not take a callback" claims in the guides.
  • Coverage of issue #3072's five guide call sites is now complete in-PR. Three are edited here (rate-limiting.mdx:208, cors.mdx:117, routing.mdx "Namespaced sections"); the other two (multi-tenancy.mdx:271, route-model-binding.mdx:118) already teach the callback form and become correct as written by the framework fix itself — no edit needed, and neither carries a stale warning.

Cross-engine

Clean. The delta is docs-only. The one compiled artifact is the new {test:compile} snippet in routing.mdx:109-119: an inline closure passed as a method named argument (.namespace(name="admin", callback=function(map){...})), which is not the Adobe-crashing constructor-named-arg shape (Cross-Engine Invariant 5) and is the exact shape already shipped in multi-tenancy.mdx:271 and exercised by the new specs. {test:compile} is the established fence convention in this file (7 pre-existing uses).

Tests

  • No new tests needed for the docs delta; the framework specs from the earlier commits stand (MapperRobustnessSpec.cfc:183-281, failing-first per the PR body, plus the green "Bot PR TDD Gate" check at this head).
  • Minor nit (carried over from both prior reviews): controller() also gains callback support via the argumentCollection = arguments forwarding at scoping.cfc:163 and is named in the CLAUDE.md and routing.mdx text, but has no spec. It's deprecated per its docblock, so optional — one it() would pin the documented surface fully.

Docs

  • Changelog fragment changelog.d/3072-scope-namespace-callback.fixed.md present and correctly formed; no direct CHANGELOG.md edit. CLAUDE.md and the three guide edits are accurate against the implementation.
  • Minor nit (carried over, now broader): the PR body is stale against the diff. The Feature Completeness Checklist still marks Framework Docs and CLAUDE.md as "left for bot-update-docs.yml follow-up", and the Related Issue section says "the five guide MDX call sites the issue lists … are handled separately" — but commit 5e2302f9 updates CLAUDE.md and commit 44b20820 updates three of the five guides, with the remaining two correct as written post-fix. Please tick those checklist rows and refresh the Related Issue paragraph; with the docs now fully covered in-PR, Refs #3072 could arguably become Closes #3072 (author's call — the issue also tracks the extends="app.controllers.Controller" side note).

Commits

  • The PR title (fix(router): scope()/namespace() honor callback= and auto-close the scope, 75 chars) is a valid conventional-commit header, and that is what gates: .github/workflows/pr.yml:26 lints only $PR_TITLE, the "Validate Commit Messages" check is green at this head, and the repo squash-merges so the title becomes the landing subject.
  • Minor nit (new, non-blocking): the new commit's header docs(web/guides): flip the three #3072 'callback= silently ignored' asides now that scope() honors it measures 101 characters, one over commitlint.config.js's header-max-length: 100 (line 27). Nothing enforces this on branch commits and the message never lands (squash-merge), so no action is required — flagging it only for the record, and as a reminder that the prefix counts toward the limit when authoring.
  • Sign-offs: 44b20820 carries Signed-off-by: Peter Amiri <petera@pai.com> matching the commit author; the earlier two commits carry matching claude[bot] sign-offs. The merge commit 9da7dd22 has none, which is conventional — merge commits are exempt from both commitlint and DCO sign-off practice.

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