fix(dispatch): narrow binding catch, cache binding misses, unify route copy semantics

[bpamiri]

Summary

Fixes six findings in the dispatch/route pipeline: route model binding no longer swallows finder-time query errors or silently absorbs explicit-binding configuration errors; conventional binding misses are negative-cached (cleared on reload) instead of repeating the app-wide model lock + base-model bootstrap + DB metadata probe on every keyed request; the static fast path and regex fallback in $findMatchingRoute now share a single $copyRouteForRequest helper (shallow top-level copy with non-simple members duplicated) so the static path can no longer leak shared nested route state and the regex path stops deep-Duplicate-ing the whole route struct per request; the matched route regex executes once per request (stashed match reused by $mergeRoutePattern); and $translateDatePartSubmissions cleans up the orphaned field($ampm) param key.

Findings addressed

• R8 [Medium] $resolveRouteModelBinding swallows all exceptions from model().findByKey() as "model class doesn't exist" @ vendor/wheels/Dispatch.cfc:550-646 — model class resolved in its own try (Dispatch.cfc:607-630); findByKey() moved outside it (Dispatch.cfc:636) so query errors propagate; explicit binding="BlogPost" resolution failures rethrow; conventional misses log a dev-mode wheels log breadcrumb with e.type/e.message.
• R13 [High, perf] Route model binding miss path repeats exclusive lock + model bootstrap + DB metadata query on every request, silently @ vendor/wheels/Dispatch.cfc:593-604, 616-621 — negative cache application.wheels.unresolvableRouteBindings, written on conventional-binding misses, consulted before model(), cleared on reload (onapplicationstart.cfc resets application.$wheels). Explicit bindings never read or write the cache.
• R12 [Low] Static-route fast path returns a shallow StructCopy while the regex path deep-Duplicates — shared nested route state leaks to request code @ vendor/wheels/Dispatch.cfc:157, 163, 240-247 — both paths now call $copyRouteForRequest(), which duplicates non-simple members (constraints, middleware) so middleware reading request.wheels.currentRoute can never mutate the shared route table.
• R16 [Low, perf] $findMatchingRoute fallback deep-Duplicates the matched route per request while the static fast path proves a shallow copy suffices @ vendor/wheels/Dispatch.cfc:188; vendor/wheels/Mapper.cfc:149-152 — regex path switched to the same shallow-plus-duplicated-nested copy; Mapper comment synced to describe the new copy semantics.
• R17 [Low, perf] Matched route regex executed twice per request @ vendor/wheels/Dispatch.cfc:186-189, 510-514 — $findMatchingRoute runs ReFindNoCase(..., 1, true) once and stashes the sub-expression result as regexMatch on the per-request copy; $mergeRoutePattern reuses it, falling back to a fresh match when absent (static fast path / direct calls). pos[1] > 0 is equivalent to the old two-arg truthiness, including the root-path special case.
• DC5 [Low] $translateDatePartSubmissions never deletes the ($ampm) part key @ vendor/wheels/Dispatch.cfc:812 — StructDelete(local.rv, local.key & "($ampm)") added to the cleanup block; PM-to-24-hour conversion unchanged.

Findings verified already-fixed

None — all six findings in this package were live against origin/develop (reviewer spot-checked the pre-fix code: develop's cleanup deleted only the six year-through-second keys, and develop's catch wrapped the whole model().findByKey() chain in a single swallow-all).

Source

Internal multi-agent framework review 2026-06-09, wave 2 package dispatch-route-pipeline.

Tests

8 new WheelsTest BDD specs across the existing dispatch spec files, each written to fail against the pre-fix code where the behavior allows it:

• vendor/wheels/tests/specs/dispatch/createParamsSpec.cfc — ($ampm) key removed from params; PM time still converts to 24-hour.
• vendor/wheels/tests/specs/dispatch/findMatchingRouteSpec.cfc — static-path nested isolation (mutating constraints on the returned copy doesn't touch the route table), regex-path isolation, regexMatch stash present on regex-matched copies, $mergeRoutePattern fallback for routes without the stash.
• vendor/wheels/tests/specs/dispatch/routeModelBindingSpec.cfc — explicit-binding resolution failure throws; conventional miss writes the negative cache; cached miss is consulted (binding skipped without re-resolution); afterEach cleans every cache name the file can create.

Local verification (Lucee 7 + SQLite, prebuilt docker image, worktree dir-only mount): all three touched bundles green — createParamsSpec 13 pass / 0 fail / 0 error, findMatchingRouteSpec 34 pass / 0 fail / 0 error, routeModelBindingSpec 13 pass / 0 fail / 0 error. Full engine x DB coverage deferred to the CI compat-matrix (the real gate); worth watching the BoxLang and Adobe lanes for these three bundles.

Cross-engine notes

• $copyRouteForRequest is public with a $ prefix (mixin invariant 7 — private helpers are not integrated on Lucee/Adobe).
• No local.X writes inside catch bodies are read after the catch (BoxLang invariant 11) — the catch paths only write application-scope keys and return the pre-try local.rv; local.modelClass is assigned in the try body, which persists fine.
• rethrow in script, writeLog(file/type/text), and the 4-arg ReFindNoCase(..., 1, true) form all have in-file prior art on develop that already passes the CI matrix.
• No reserved-scope parameter names, no inline closures as constructor named args, no arguments-as-attributeCollection, no Left(str, 0).

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR fixes six legitimate dispatch/route-pipeline bugs (narrowed binding catch, negative-caching of binding misses, unified route-copy semantics, regex stash, ampm key cleanup) and the overall approach is sound. All five implementation paths are functionally correct and the cross-engine checklist the author ran through in the PR body is accurate. Commenting rather than approving on two minor points: the test for the R8 query-error-propagates guarantee is missing, and findMatchingRouteSpec.cfc is missing a trailing newline.

---

Correctness

Benign TOCTOU on negative-cache initialization (vendor/wheels/Dispatch.cfc, lines 618-621):

Two concurrent requests can both pass the StructKeyExists check before either writes the struct, then both assign {} to the key and both add modelName = true. The race outcome is idempotent — same key, same value — so no data is corrupted. The worst case is that both requests complete the model-bootstrap they were trying to avoid. Given the write is inside a catch block that only fires on the very first miss per model name, the window is narrow. Acceptable as-is, flagging for reviewers who see the unguarded application-scope write.

Root-path special case stashes a failed match (vendor/wheels/Dispatch.cfc, lines 186-189):

When arguments.path is empty and matched via the local.route.pattern == "/" special case, local.match.pos[1] is 0 (regex did not match the empty string) but is still stashed as regexMatch on the per-request copy. In $mergeRoutePattern this stash is consumed; ArrayLen(local.matches.pos) is 1 so the variable-extraction loop does not execute. Root routes have no capture groups so this is a no-op in practice. Correct as implemented.

---

Tests

R8 core guarantee — query errors propagate — has no direct test (vendor/wheels/tests/specs/dispatch/routeModelBindingSpec.cfc):

The structural change of R8 moves local.modelClass.findByKey(...) outside the try block so DB-time errors propagate instead of being swallowed as missing-class exceptions. The new specs cover explicit-binding resolution failures throwing, negative-cache population, and cache hits — but none exercises a findByKey error escaping to the caller. This is the most important behavioral contract introduced by this PR. Worth a follow-up tracking item so the regression surface is explicit.

Missing trailing newline (vendor/wheels/tests/specs/dispatch/findMatchingRouteSpec.cfc, final line): the diff ends with no newline at end of file. The original had one; restoring it keeps the repo consistent with all other spec files.

afterEach coverage is complete — the PR correctly anticipates that the pre-existing "skips silently when model class does not exist" test (controller nonexistentThings -> model NonexistentThing) will now populate the negative cache, and the added cleanup block covers Post, NonexistentThing, and NonexistentWidget. No gaps found.

---

Docs

No CHANGELOG.md [Unreleased] entry. The PR body acknowledges this ("Entry deliberately omitted; consolidated at campaign end"). Fine for a campaign-batch PR, worth confirming the consolidation step is tracked.

---

Commits

fix(dispatch): narrow binding catch, cache binding misses, unify route copy semantics — 84-character header, valid fix type, valid dispatch scope, sentence-case subject. Passes commitlint.

---

Security

The writeLog in the catch block (line 626) interpolates e.type and e.message into the log text. Model names are derived from controller names via routing, not raw user input, so the injection surface is narrow. Gated behind != "production". No action needed.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-reasoned. Both concrete findings (missing trailing newline, missing R8 query-error-propagation test) check out against the diff. A's two technical analyses (benign TOCTOU, root-path zero-pos stash) are correct. No sycophancy, no false positives, no missed issues beyond what A already raised. Aligning on changes-needed: the trailing newline is a trivial fix, and the R8 test gap — the most important behavioral contract in this PR — warrants either a concrete spec or a tracked follow-up issue.

Sycophancy

None detected. A issued COMMENTED, not an approval, and backed every claim with cited lines.

False positives

None detected.

• TOCTOU on unresolvableRouteBindings init: A's analysis is correct. The race window produces at most a redundant model bootstrap; no key is lost or corrupted. The worst case is two concurrent threads both creating a fresh {}, then both writing their respective model names — both entries land correctly in the final struct. Idempotent as stated.
• Root-path zero-pos stash: A's analysis is correct. When arguments.path is empty and the route matches only via the pattern == "/" special case, ReFindNoCase returns pos = [0]. $mergeRoutePattern then iterates for (i = 2; i <= ArrayLen(pos); ...) — with ArrayLen = 1, the loop body is skipped. Root routes have no capture groups, so foundVariables is empty and skipping the loop is correct. No variable extraction is lost.
• Commit header length: A says 84 chars; my count is 85. Immaterial — both are well under the 100-char limit and commitlint would catch a real violation.
• writeLog injection surface: A's assessment is correct. The model name is derived from the registered controller name via routing, not raw user input, and the catch path only fires on CFML component-resolution exceptions (not DB-layer messages). The != "production" guard is appropriate.

Missed issues

None detected beyond what A already flagged.

The cross-engine checklist holds up on inspection:

• $copyRouteForRequest is public with $ prefix (mixin invariant 7).
• No local.X writes inside catch bodies that are read after the catch (BoxLang invariant 11). Catch body writes go to application scope; the only local read is return local.rv, which was assigned before the try.
• No arguments-as-attributeCollection (invariant 10).
• No inline closures as constructor named args (invariant 5).
• for (key in struct) iteration while replacing values (not adding or removing keys) is safe on all supported engines.

The negative-cache-cleared-on-reload claim in the PR body (application[$appKey()] is reset by onapplicationstart.cfc) is plausible and consistent with framework reload semantics, though A does not explicitly verify it. Not flagging as a missed issue — the test cleanup pattern (manual StructDelete in afterEach) is correct regardless, and this is a standard framework invariant.

Verdict alignment

A's COMMENTED state is consistent with the findings: two concrete issues that are not merge-blockers on their own but represent unfinished work — a trivial newline omission and a missing regression test for the PR's most important behavioral change.

Convergence

Aligned. Both findings are real: the trailing newline on findMatchingRouteSpec.cfc should be restored (one-character fix), and the R8 query-error-propagation guarantee needs either a spec (mock findByKey to throw, or an integration scenario) or an explicit follow-up issue so the regression surface is tracked. Address-review should apply the newline fix and, if a lightweight spec is feasible, add it; otherwise open a tracking issue. Joint recommendation: apply the two changes and re-review on the new SHA.