fix(middleware): short-circuit OPTIONS preflight in dispatch when CORS middleware is registered - #2728
Conversation
…S middleware is registered The new middleware pipeline ran AFTER route matching, so an OPTIONS preflight against a path that only declared POST/PUT/PATCH/DELETE 404'd in `$findMatchingRoute()` before `wheels.middleware.Cors`'s preflight branch could fire. The legacy `set(allowCorsRequests=true)` path aborted OPTIONS in `EventMethods.cfc` before dispatch, so the new middleware was strictly less capable than the 3.x setting it replaced. `Dispatch.$request()` now checks the verb up front and, if it is OPTIONS and the global pipeline contains a `wheels.middleware.Cors` instance, runs the pipeline against a no-op core handler so the CORS middleware can set headers and return without touching the route table. Behavior for OPTIONS without CORS middleware (still 404s) and for non-OPTIONS verbs (routed normally) is unchanged. Fixes #2703. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…global Cors registration Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The core fix is surgically correct — Dispatch.$request() now detects a global Cors instance and short-circuits OPTIONS before route matching, preserving the legacy set(allowCorsRequests=true) contract. Implementation and commit messages are clean. I'm leaving a --comment (not requesting changes) because the findings are test-coverage and documentation gaps rather than correctness or cross-engine failures.
Tests
CorsSpec.cfc has no test for the Cors middleware's own OPTIONS short-circuit branch.
vendor/wheels/tests/specs/middleware/CorsSpec.cfc has eight tests covering origin matching, wildcard validation, and passthrough but never exercises the if (local.requestMethod == "OPTIONS") { return ""; } branch in Cors.cfc:89. That branch is the unit of logic this PR defends. A test equivalent to:
it("returns empty string on OPTIONS without calling next", () => {
var nextCalled = false;
local.cors = new wheels.middleware.Cors(allowOrigins = "https://example.com");
// Pass request_method via the struct key Cors reads (cgi.request_method)
local.reqCtx = {cgi = {request_method = "OPTIONS", http_origin = "https://example.com"}};
local.result = local.cors.handle(
request = local.reqCtx,
next = function(required struct request) {
nextCalled = true;
return "should-not-reach";
}
);
expect(local.result).toBe("");
expect(nextCalled).toBeFalse();
});belongs in CorsSpec.cfc. Without it, the OPTIONS branch in Cors.cfc is dead from a unit-test perspective.
CorsPreflightDispatchSpec.cfc first case passes through an unexpected code path.
vendor/wheels/tests/specs/middleware/CorsPreflightDispatchSpec.cfc:57-58
expect(threw).toBeFalse();
expect(result).toBe("");The first spec sets request.cgi["request_method"] = "OPTIONS" (the custom request-scope struct), which is what Dispatch.$request() and $getRequestMethod() read. However, Cors.cfc:85 reads from the real CGI scope:
local.requestMethod = cgi.request_method; // real CGI scope, not request.cgiIn the test runner context, cgi.request_method is determined by the engine from the HTTP request that hit the test URL — almost certainly "GET", not "OPTIONS". So Cors.handle() falls through to return arguments.next(arguments.request), and the no-op local.preflightHandler returns "".
The test correctly verifies "dispatch doesn't 404" (the primary regression fix), but the assertion result == "" is satisfied by the no-op handler rather than the Cors middleware's short-circuit. An explicit assertion that CORS response headers were written (e.g. checking getPageContext().getResponse().getHeader("Access-Control-Allow-Origin")) or a note in the spec explaining why the CGI scope limitation makes end-to-end header assertion impractical here would prevent a future reader from believing the test covers more than it does.
beforeEach mixes deep and shallow copy for saved state.
vendor/wheels/tests/specs/middleware/CorsPreflightDispatchSpec.cfc:18-21
_savedRoutes = Duplicate(application.wheels.routes); // deep copy
_savedStaticRoutes = StructCopy(application.wheels.staticRoutes); // shallow copy_savedRoutes uses Duplicate (deep copy), but _savedStaticRoutes uses StructCopy (shallow). If staticRoutes contains nested arrays the restoration in afterEach would put back a shallow copy, potentially leaving mutated nested data visible across tests. Use Duplicate for both, or note why shallow is intentional.
Docs
v4-0-0 stable docs are missing the new Aside warning.
web/sites/guides/src/content/docs/v4-0-0/digging-deeper/cors.mdx (lines around the scope example, ~L88-L112) contains the route-scoped Cors example without any caveat that route-scoped Cors does not benefit from the preflight short-circuit. The PR adds a well-placed <Aside type="caution"> to v4-0-1-snapshot but omits the same note from the released stable docs. Since the limitation exists for v4.0.0 users today, the stable docs carry incorrect implied guidance ("scope-level middleware composes with global middleware — both run" implies full parity, which is untrue for the preflight path).
Conventions
Empty catch block silently discards errors.
vendor/wheels/Dispatch.cfc:268-271
try {
local.preflightMethod = $getRequestMethod();
} catch (any e) {
}An empty catch block is functionally safe here (fail-closed: no preflight detection, routing continues normally), but it will invisibly swallow any unexpected exception from $getRequestMethod() in test or future-maintenance scenarios. Even a single $log() call or a comment like // request.cgi not yet populated (test context) would make the intent explicit and aid diagnostics.
Security
No security concerns. The IsInstanceOf(local.mw, "wheels.middleware.Cors") guard in $hasPreflightCapableMiddleware() is appropriately tight — the short-circuit only fires when a genuine wheels.middleware.Cors instance is in the global pipeline, not on any middleware, and not on a string path alias.
Cross-engine
IsInstanceOf(local.mw, "wheels.middleware.Cors") with a dotted-path string is supported on Lucee, Adobe CF, and BoxLang. No closures-as-constructor-args in new code. local.preflightHandler is hoisted into a local var before being passed to $middlewarePipeline.run(), which avoids the Adobe CF ArrayStoreException trap. No client scope issues, no bracket-notation calls inside closures. Cross-engine hygiene is clean.
Commits
Both commits are well-formed and DCO-signed:
fix(middleware): short-circuit OPTIONS preflight in dispatch when CORS middleware is registered— valid type, scope, ≤ 100 chars.docs(web/guides): note that OPTIONS preflight short-circuit requires global Cors registration— valid type, scope, ≤ 100 chars.
The fix commit body explains the "why" (legacy contract, middleware-after-routing ordering) clearly.
The dispatch-layer fix is correct and the three regression guards (OPTIONS + CORS no 404, OPTIONS without CORS still 404s, non-OPTIONS still routes normally) cover the intended behavioral contract. Adding an OPTIONS unit test to CorsSpec.cfc and syncing the stable v4-0-0 docs would close the gaps flagged above.
Wheels Bot — Reviewer B (round 1)A's review is substantively accurate with one minor clarification needed on the proposed remediation for finding #2. All five findings are valid, the cross-engine and security analysis holds up against the diff, and the COMMENTED verdict is proportionate. I'm aligned and calling SycophancyNone detected. A calls the fix "surgically correct" and that holds: the False positivesNone. I verified each finding against the actual source:
Missed issuesNone. A's cross-engine and security passes were complete. Verdict alignmentCOMMENTED (not requesting changes) is consistent with findings that are test-quality and documentation gaps, not correctness or cross-engine failures. Appropriate. ConvergenceAligned. Five concrete, verifiable gaps — missing |
- vendor/wheels/middleware/Cors.cfc: read request_method from arguments.request.cgi first (fall back to engine CGI scope) so the middleware respects the per-request context the pipeline passes in. Mirrors the RateLimiter pattern; required because a bare `request` reference inside a function resolves to the engine REQUEST scope, not the function argument. No production behavior change — engine CGI remains the fallback. - vendor/wheels/tests/specs/middleware/CorsSpec.cfc: add a unit test for the OPTIONS short-circuit branch that was previously dead from a unit-test perspective. - vendor/wheels/tests/specs/middleware/CorsPreflightDispatchSpec.cfc: switch _savedStaticRoutes from StructCopy (shallow) to Duplicate (deep) to match _savedRoutes; clarify in the first spec why the empty-string assertion is satisfied by Dispatch's no-op handler rather than Cors's own OPTIONS branch. - vendor/wheels/Dispatch.cfc: document the intent of the empty catch block around $getRequestMethod() (fail-closed: skip the short-circuit and let normal routing proceed). - web/sites/guides/src/content/docs/v4-0-0/digging-deeper/cors.mdx: mirror the v4-0-1-snapshot Aside warning that the preflight short-circuit requires global Cors registration; route-scoped Cors does not benefit because route matching runs first. Middleware suite: 3557 pass / 0 fail / 0 error (Lucee 7 + SQLite), including the new CorsSpec test and existing CorsPreflightDispatchSpec. 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 1):
Tests
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: The round-1 fixup commit (4b7036f58) directly addresses all four findings from the previous review: the CorsSpec OPTIONS unit test is now present, _savedStaticRoutes is deep-copied with Duplicate, the empty catch block in Dispatch.cfc carries an explanatory comment, the stable v4-0-0 guide now has the Aside caution, and Cors.cfc reads request_method from arguments.request.cgi before falling back to the engine CGI scope. No correctness, cross-engine, or security blockers remain. Three minor nits are noted below — none warrant requesting changes.
Conventions
Case-sensitivity inconsistency between Dispatch.cfc and Cors.cfc.
Dispatch.cfc:276
if (UCase(local.preflightMethod) == "OPTIONS" && $hasPreflightCapableMiddleware()) {Cors.cfc:100
if (local.requestMethod == "OPTIONS") {Dispatch normalises the verb with UCase(); Cors does not. The HTTP spec requires uppercase method tokens, so this is not a practical bug — no browser sends options in lowercase. But the two places that gate on this verb should handle it consistently. Consider adding UCase() at Cors.cfc:100.
Comment accuracy: bare request inside handle().
Cors.cfc:86-89
// Inside a function, a bare `request` reference resolves to the engine REQUEST
// scope, not the function argument — `arguments.request` is required to address
// the passed struct.In standard CFML, the arguments scope has a higher lookup priority than the engine REQUEST scope, so bare request inside handle() should resolve to arguments.request. That would make the comment incorrect — and it would mean that line 50 already reads origin from the passed struct (not the engine scope), consistent with the rest of the pattern. The code is correct regardless (the preflightContext carries no cgi key, so both line 50 and line 91 fall through to the engine scope in production), but the comment may mislead future readers. Worth double-checking on Lucee/Adobe and then either removing the comment or correcting it to describe the actual resolution order.
Unused method field in preflightContext.
Dispatch.cfc:278-283
local.preflightContext = {
params = {},
route = {},
pathInfo = arguments.pathInfo,
method = local.preflightMethod // ← set here
};Cors.handle() never reads arguments.request.method; it checks arguments.request.cgi.request_method (line 91) and then the engine CGI scope. The method field is silently ignored by the Cors middleware. This is not a bug — method follows the same convention as the regular requestContext built later in $request(), and a future middleware could read it — but it may be worth a brief comment noting that Cors reads from cgi.request_method rather than this field, so future authors know it isn't redundant.
Tests
The three specs in CorsPreflightDispatchSpec.cfc are well-structured. The inline comment in the first spec (lines 38-43) explaining why the empty-string assertion is satisfied by the no-op handler rather than the Cors OPTIONS branch is a good addition — it prevents someone from misreading the test as end-to-end preflight header coverage. No gaps.
Cross-engine
IsInstanceOf(local.mw, "wheels.middleware.Cors") with a dotted-path string is supported on Lucee, Adobe CF, and BoxLang. The local.preflightHandler closure is hoisted into a local variable before being passed to $middlewarePipeline.run(), which avoids the Adobe CF ArrayStoreException: ASTcffunction constructor-arg trap. The new next = function(required struct request) { ... } in CorsSpec.cfc:94-97 is passed as a named argument to a regular method call (not a constructor), so the Adobe crash does not apply. No cross-engine concerns.
Commits
fix(middleware): address Reviewer A/B consensus findings (round 1) — valid type, scope, ≤ 100 chars, not all-caps, DCO-signed. The commit body enumerates each finding with the file and change, which is unusually good commit hygiene.
All Reviewer A's previous findings are resolved. The three items above are genuine nits — feel free to address them in a follow-up or as part of the docs pass; they should not block merge.
Wheels Bot — Reviewer B (round 1)A's round-2 review is accurate and proportionate. All five findings from the prior convergence are confirmed addressed; the three new nits are genuine and well-evidenced. No sycophancy, no false positives, one narrow precision point on nit #2. Converging on approve. SycophancyNone detected. A chose COMMENTED rather than APPROVED despite a positive opening, which is the right call when three open nits remain. The "surgically correct" characterisation of the fix is accurate and bounded — not a blanket LGTM. False positivesNone. I verified each nit against the source:
Missed issuesNone. A's cross-engine check ( Verdict alignmentCOMMENTED (not requesting changes) is consistent with three findings that are all documentation or cosmetic nits. None involves correctness, cross-engine failure, or a security gap. The verdict is appropriate. ConvergenceAligned. A's and my joint view: the core fix is sound, all prior round-1 findings are closed, and the three remaining nits do not block merge. They are candidates for the follow-up docs pass. The PR is review-clean on this SHA. |
Re-commits the auto-generated blog visual baseline PNG that was first produced by the refresh-visual-baselines.yml workflow run on this PR branch. The original commit body exceeded the 100-char limit enforced by commitlint (the branch name expanded inline), failing the Validate Commit Messages check; this re-commit carries the same binary content with a wrapped message. Run the visual-baselines workflow when an intentional content/layout change makes the visual-regression check fail. The new PNG(s) under web/tests/visual-baselines/ are now the expected rendering; re-run the failing visual-regression job to flip the check green. Signed-off-by: Peter Amiri <peter@alurium.com>
- Cors.cfc: normalise local.requestMethod with UCase() before comparing to "OPTIONS", matching the gating site in Dispatch.cfc and removing the case-sensitivity inconsistency. - Cors.cfc: correct the inaccurate comment about bare `request` resolving to the engine REQUEST scope. In CFML the arguments scope has higher lookup priority than named scopes (REQUEST, CGI), so bare `request` inside handle() resolves to arguments.request. The actual reason for the arguments.request.cgi-first lookup is that the engine CGI scope is read-only on Lucee 7, blocking unit tests from injecting OPTIONS via cgi.request_method. - Dispatch.cfc: drop the unused `method = local.preflightMethod` field from preflightContext. Cors.handle() reads the verb from arguments.request.cgi.request_method, not arguments.request.method, so the field is dead. Replace with a brief comment noting why the short-circuit context omits it (only Cors runs from this code path). No behaviour change. Existing CorsSpec and CorsPreflightDispatchSpec coverage continues to gate the OPTIONS short-circuit. Signed-off-by: Peter Amiri <peter@alurium.com>
0e956ba to
901bae8
Compare
There was a problem hiding this comment.
Wheels Bot - Reviewer A (round 3)
TL;DR: Round-2 nit commit (901bae884) correctly addresses all three findings from the previous review: UCase() normalisation added to Cors.cfc:100 to match the gating site in Dispatch.cfc:276, the inaccurate comment about bare request resolving to the engine REQUEST scope corrected to explain the real Lucee-7-read-only constraint, and the unused method field dropped from preflightContext. No correctness, cross-engine, or security blockers remain. This review is submitted as COMMENT rather than APPROVE because GitHub prevents the PR author account from approving their own PR; the verdict is approve.
Conventions
Comment at vendor/wheels/Dispatch.cfc:280 is technically imprecise.
The comment reads: only Cors is run from this short-circuit. However the pipeline invocation at line 289 (variables.$middlewarePipeline.run(...)) runs ALL global middleware against preflightContext, not only Cors. Middleware registered before Cors (e.g. RequestId, SecurityHeaders) executes against preflightContext normally; Cors then short-circuits and never calls next, so middleware registered after it does not run. A clearer phrasing: Cors is the only middleware that gates on this path; once it short-circuits, middleware registered after it does not run. Purely cosmetic - the code is correct. Feel free to leave it as-is or amend on a future pass.
Commits
All five commits are DCO-signed with the correct identity for their respective authors (claude[bot] for the bot-authored initial commits, Peter Amiri for the round-1/round-2 fixups). Commit types are valid and subjects are within the 100-char limit.
One unrelated commit slipped in: 45b3f8c91 chore(web): refresh visual baseline(s) (blog). It carries no CORS-related changes and is authored correctly. Not a blocker.
Cross-engine
UCase() normalisation in Cors.cfc:100 now matches Dispatch.cfc:276. The arguments.request.cgi-first lookup with engine-CGI fallback is cross-engine safe. IsInstanceOf(local.mw, "wheels.middleware.Cors") with a dotted-path string is supported on Lucee, Adobe CF, and BoxLang. No new cross-engine concerns.
All prior findings across three rounds are resolved. This is ready to merge once the PR is marked ready for review.
Reviewer A round-3 noted that the comment "only Cors is run from this short-circuit" is technically imprecise — `$middlewarePipeline.run()` still executes any middleware registered before Cors. Cors then short-circuits without calling next, so middleware registered after it does not run. Rewrite the comment to describe the actual pipeline behaviour. No code change. Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)Review 4301991302 is a test placeholder with no analysis. A submitted "hello test from bot" and COMMENTED state, with no line comments. I verified the three round-2 nit fixes (UCase consistency, comment accuracy, unused SycophancyDetected. A submitted a review body of "hello test from bot" with no line comments and COMMENTED state. No findings were stated, no analysis was performed, and no reasoning was given for the COMMENTED verdict. The review provides no signal at all — it is equivalent to a silent non-objection with no evidence. False positivesNone. A made no claims, so there is nothing to dispute. Missed issuesA should have verified that the three nits from round 2 were correctly addressed in SHA
No new issues in SHA Verdict alignmentCOMMENTED without any analysis is not a verdict I would issue for a diff that is cosmetic-only nit fixes on a previously approved core change. APPROVED is the appropriate call when three confirmed-clean cosmetic nits close the last open findings. A's COMMENTED state is formally non-blocking, but it carries no information. ConvergenceAligned on approval. The three round-2 nits are all correctly implemented. There are no missed issues in the diff. The prior SHA ( |
Wheels Bot — Reviewer B (round 1)Review 4301992286 contains no substantive content — its body is the literal string "test stdin body" with zero line comments. There is nothing to align on. This is not sycophancy in the usual sense; it is an empty placeholder that was submitted to the PR instead of an actual review. A needs to re-run against SHA 901bae8 and produce a real review before convergence can be assessed. SycophancyThe review body is "test stdin body\n". A made no claims, issued no findings, and provided no verdict reasoning. The COMMENTED state is structurally meaningless without content. This is functionally worse than an unsupported LGTM — at least that implies someone looked at the diff. False positivesNone. A made no claims, so there is nothing to evaluate. Missed issuesAll of them. Commit 901bae8 changed two files with three concrete nit fixes that were explicitly called out by B in the previous round:
A examined none of these. A valid review would confirm: (1) UCase normalisation is now symmetric with the Dispatch gating site; (2) the corrected comment is factually accurate (CFML arguments scope does have higher lookup priority than named scopes); (3) dropping the dead Verdict alignmentCannot be assessed. A's COMMENTED verdict carries no content, so there is no verdict to evaluate for consistency. ConvergenceNot aligned. A submitted a placeholder review with no findings, no analysis, and no verdict reasoning on a SHA that contains three small but explicitly-tracked nit fixes. The loop cannot converge until A produces a real review. The three changes are cosmetic/documentation only (no behaviour change per commit message) and the prior |
Wheels Bot - Reviewer B (round 1)A's round-3 review is accurate and terse. All three nits from the prior round are confirmed addressed, the one remaining cosmetic finding is real, and the approve verdict is proportionate. Aligned - converging on approve. SycophancyNone detected. A chose COMMENTED rather than APPROVED, correctly attributing that to GitHub's self-approval restriction rather than review hesitation. The positive framing is backed by per-finding verification, not a blanket LGTM. False positivesNone. I verified A's single finding against the actual source:
A's confirmation that the three round-2 nits are closed also checks out:
Missed issuesNone. A caught the unrelated Verdict alignmentCOMMENTED (with stated approve) is consistent with a single cosmetic, explicitly non-blocking finding. Appropriate. ConvergenceAligned on approve. The fix is sound, all prior round-1 findings are closed, and the one remaining note (comment imprecision at |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The round-4 commit (a7b3ecbc8) is a comment-only change that directly addresses the single nit from round 3: the imprecise phrase "only Cors is run from this short-circuit" has been replaced with accurate prose describing the actual pipeline execution order. No code change, no new issues. This PR is clean and ready to merge.
Commits
docs(middleware): clarify preflight-context comment in Dispatch.cfc — valid type (docs is correct for a comment-only change), valid scope, subject ≤ 100 chars, not ALL-CAPS, DCO-signed (Signed-off-by: Peter Amiri). Commit body explains the "why" (round-3 finding) and confirms no code change.
Comment accuracy — confirmed correct
vendor/wheels/Dispatch.cfc:278–282
// Cors.handle() reads the verb from arguments.request.cgi.request_method
// rather than arguments.request.method, so we don't carry the method
// field on this context. Cors is the only middleware that gates on
// this path; once it short-circuits, middleware registered after it
// does not run. Middleware registered before Cors still executes.This accurately describes $middlewarePipeline.run() behaviour: all middleware in registration order runs against preflightContext; Cors is the one that returns early without calling next() for OPTIONS, so anything registered after it in the array is skipped. Previously "only Cors is run" would have incorrectly implied that earlier middleware (e.g. RequestId, SecurityHeaders) is bypassed.
All findings across four review rounds are resolved. Verdict is --comment rather than --approve because the PR author and reviewer share a bot account — GitHub blocks self-approval. A human maintainer review is sufficient to merge.
…s-middleware-cors-cannot-short-circuit-option # Conflicts: # CHANGELOG.md
Wheels Bot — Reviewer B (round 1)Review 4301991659 is a third consecutive placeholder stub on SHA `901bae884` — body is literally "line one\nline two\nline three" with no line comments, no findings, and no rationale for the COMMENTED verdict. The PR head has since moved to `a7b3ecbc` (a human commit by @bpamiri that rewrites the comment in `Dispatch.cfc` for precision). I am verifying the current diff state independently to decide convergence. SycophancyDetected. Three consecutive Reviewer A runs on SHA `901bae884` submitted placeholder bodies with COMMENTED state and zero analysis. A placeholder COMMENTED is effectively a null verdict — it neither approves nor requests changes, and provides no information. This is worse than an unsupported LGTM: at least that implies the diff was opened. False positivesNone. A made no claims. Missed issuesA should have:
Verdict alignmentCOMMENTED with no content cannot be evaluated for consistency with findings that don't exist. A valid verdict requires a body. ConvergenceAligned on approve. The review loop has been running across three SHAs (`ee2579f60`, `4b7036f58`, `901bae884`) with consistent conclusions: the CORS preflight short-circuit fix is correct, all genuine findings have been addressed, and no cross-engine, security, or correctness issues remain open. The `a7b3ecbc` commit adds only an accurate comment rewrite — no code path changes, no new test requirements. There is nothing left to block merge. The placeholder A reviews on `901bae884` did not uncover new issues because there are none. The PR is review-clean on the current head. |
Wheels Bot — Reviewer B (round 1)A's review of the round-8 delta is accurate and proportionate. The only change since round 7 is the develop merge commit that brought in PR #2734 (documentation-only). No sycophancy, no false positives, no missed issues. Converging on approve. SycophancyNone detected. A chose COMMENTED rather than APPROVED, correctly attributing that to GitHub's self-approval restriction rather than any open concern. The positive framing is grounded in per-file verification of the merge delta, not a blanket LGTM. False positivesNone. A's characterisation of the delta is accurate. I verified:
Missed issuesNone. The develop merge introduces no code changes to the CORS path. All cross-engine hygiene confirmed in prior rounds is unchanged. The documentation additions are engine-neutral and factually correct. Verdict alignmentCOMMENTED is consistent with the situation: no open findings, but the bot cannot self-approve. Appropriate. ConvergenceAligned on approve. All findings across rounds 1-7 have been resolved. The round-8 delta is documentation-only, clean, and correctly characterised by A. No correctness, cross-engine, or security blockers remain. The PR is review-clean for this SHA. |
Re-commits the auto-generated blog visual baseline PNG produced by the refresh-visual-baselines.yml workflow run on this PR branch. The bot's default commit body expanded the branch name inline, producing a line that exceeded the 100-char limit enforced by commitlint; this re-commit carries the same binary content with a wrapped message body. Run the visual-baselines workflow when an intentional content/layout change makes the visual-regression check fail. The new PNG(s) under web/tests/visual-baselines/ are now the expected rendering; re-run the failing visual-regression job to flip the check green. Signed-off-by: Peter Amiri <peter@alurium.com>
3123a38 to
03ce081
Compare
…s-middleware-cors-cannot-short-circuit-option # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The CORS preflight short-circuit fix is correct and stable. This is round 9 of review; the authored code in Dispatch.cfc, Cors.cfc, both test specs, and both MDX docs has been unchanged since round 4. The new head commit (65c46fe) is a routine merge-of-develop (CHANGELOG.md conflict resolution, a CI workflow update, and an unrelated CLI packages-help fix). No new issues in the merge contents. Leaving a --comment for one previously-unraised API contract nit; no blocking issues.
Correctness
The fix's logic holds:
-
$getRequestMethod()is called in a try/catch: ifrequest.cgiis not yet populated (test contexts, unusual dispatch paths),preflightMethodstays empty and the guard below is skipped, so normal routing proceeds. Fail-closed is the right default. -
$hasPreflightCapableMiddleware()iterates the global pipeline withIsObject() && IsInstanceOf(..., "wheels.middleware.Cors"). An empty pipeline returns false safely; a non-CFC entry returns false fromIsObject()— no NPE risk. -
When the guard fires,
request.wheels.params = {}is set before the early return, matching the assignment on the normal path (line 300). Request scope consistency preserved. -
The
preflightHandlerclosure is assigned to a local variable before being passed to$middlewarePipeline.run()as a named argument — correct Adobe CF safe pattern (hoisted, not inline in therun()call).
Conventions
No mixed positional+named arguments, no query/array confusion, no route ordering issues. Nothing to flag.
Cross-engine
IsObject(),IsInstanceOf(),StructKeyExists(),UCase()— all standard CFML BIFs consistent across Lucee/Adobe/BoxLang.new wheels.middleware.Cors(allowOrigins = "...")in test specs passes a string, not an inline closure — the Adobe CFArrayStoreExceptiontrap does not fire.local.preflightHandler = function(...) { ... };is hoisted to a local variable before being passed ascoreHandler. Correct defensive form per the cross-engine guide.- No
clientscope, noapplicationscope function members, no bracket-notation function calls inside closures. Clean.
One nit not previously raised — preflightContext is missing the method key:
Dispatch.cfc:283-287 constructs the preflight context without a method field:
local.preflightContext = {
params = {},
route = {},
pathInfo = arguments.pathInfo
// no method field
};The normal requestContext built on lines 330-335 carries method = $getRequestMethod(). Any custom middleware that accesses arguments.request.method (as distinct from arguments.request.cgi.request_method) will encounter an undefined-key error on OPTIONS preflights after this fix ships. No built-in middleware is affected — RateLimiter and SecurityHeaders both read from arguments.request.cgi.* keys — but the API contract is inconsistent. Adding method = UCase(local.preflightMethod) to preflightContext would close the gap without any behavior change on the working path:
local.preflightContext = {
params = {},
route = {},
pathInfo = arguments.pathInfo,
method = UCase(local.preflightMethod)
};This is a follow-up candidate rather than a blocker — the MiddlewareInterface docblock does not enumerate method as a required key, so custom middleware accessing it without a StructKeyExists guard is defensively incomplete on its own terms. Worth addressing before broader adoption.
Tests
Both additions are correct:
-
CorsSpec.cfc(newitblock at line 83): injectsrequest_method = "OPTIONS"vialocal.reqCtx.cgi, confirmingCors.handle()prefersarguments.request.cgi.request_methodover the engine CGI scope and short-circuits with"". Thenextclosure returns"should-not-reach"so any fall-through surfaces as an assertion failure — functionally equivalent to anextCalledflag. -
CorsPreflightDispatchSpec.cfc: Three cases cover OPTIONS+CORS (no 404, returns""), OPTIONS+no-CORS (still 404s), and GET+CORS (normal dispatch). The comment on lines 35-43 honestly documents that the first test'sresult == ""is satisfied by the no-oppreflightHandler, not byCors.handle()'s own OPTIONS branch — the round-trip integration of those two paths is covered by the two specs together. Acceptable design given test-environment CGI scope constraints.
Docs
- Both
v4-0-0andv4-0-1-snapshotcors.mdxfiles carry the<Aside type="caution">warning that the preflight short-circuit requires globalCorsregistration. Correct and consistent. CHANGELOG.md[Unreleased] > Fixedentry added. The merge-commit CHANGELOG conflict was resolved cleanly — the new entry is present in the current diff.
Commits
All PR-authored commits conform to commitlint: valid types (fix, docs, chore), optional scopes, subjects under 100 chars, not ALL-CAPS. The latest merge commit (65c46fe) is a routine integrate-develop; its constituent changes (bot-tdd-gate.yml, cli/lucli/Module.cfc, PackagesCommandSpec.cfc) are unrelated to this fix and were reviewed on their own PRs.
Security
No user input reaches SQL strings. No unencoded output to views. No CSRF surface (OPTIONS is not a state-changing verb). No secrets in fixtures or test data.
Wheels Bot — Reviewer B (round 1)A's review is thorough and correct. One legitimate non-blocking nit; no sycophancy, no false positives, no missed issues. Converging as approve. SycophancyNone detected. A explicitly notes this is round 9 of the overall loop, that the authored code has been unchanged since round 4, and raises a specific nit with concrete code — it is not a blanket LGTM. False positivesNone detected. I verified each of A's claims against the diff:
Missed issuesNone detected. I re-scanned the diff for the usual gaps:
Verdict alignmentA's ConvergenceAligned. A and B agree: the fix is correct, cross-engine clean, and well-tested. The one open item (adding |
…s-middleware-cors-cannot-short-circuit-option # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a well-targeted fix. The dispatch-layer short-circuit for OPTIONS preflight is architecturally sound, all shipped middleware gracefully handle the context the new path produces, and the three cross-engine traps that have bitten similar pipeline work are cleanly avoided. I'm leaving a --comment (not requesting changes) for two residual coverage observations that aren't blocking.
Correctness
Dispatch short-circuit path is correct.
Dispatch.$request() in vendor/wheels/Dispatch.cfc:267-294 reads the verb via $getRequestMethod() before route matching, then gates on $hasPreflightCapableMiddleware(). The fallback try/catch (lines 268-274) fails closed — leaving preflightMethod = "" — so the guard is never accidentally satisfied in test contexts where request.cgi is not yet populated. The no-op preflightHandler closure is pre-assigned to a local variable before being passed to Pipeline.run():
local.preflightHandler = function(required struct request) {
return "";
};
return variables.$middlewarePipeline.run(
request = local.preflightContext,
coreHandler = local.preflightHandler
);This avoids the Adobe CF "inline closure as constructor named arg" crash (ArrayStoreException: ASTcffunction) because the literal does not appear directly as a named argument at a new call site.
Cors.handle() OPTIONS detection correctly prefers injected context.
The refactored lookup in Cors.cfc:127-134 checks arguments.request.cgi.request_method first (guarded with StructKeyExists) and falls back to the engine cgi scope. In production, preflightContext has no cgi key, so Cors reads from the engine cgi scope — which is "OPTIONS" during a real preflight. This is correct.
preflightContext missing cgi key is consistent with the normal context.
The normal requestContext (lines 331-336) also omits a cgi key. Every shipped middleware (RateLimiter, TenantResolver, Cors) guards access with StructKeyExists(arguments.request, "cgi") before reading it, so they fall back gracefully to the engine CGI scope on OPTIONS preflight — consistent behaviour.
Observation — user-authored middleware could be surprised. A developer who writes custom middleware and assumes request.cgi.remote_addr is always present would silently get a key-not-found or unexpected fallback on OPTIONS preflight. This is the same contract as normal requests (where requestContext also lacks cgi), but it is not called out in the middleware authoring guide. Adding one sentence to "Writing custom middleware" would eliminate future confusion. Not blocking, but worth a follow-up doc update.
Tests
Residual integration-test gap.
CorsPreflightDispatchSpec.cfc:34-68 (the first it()) asserts result == "" and threw == false. As the comment at lines 37-43 accurately documents, the "" return value comes from the no-op preflightHandler, not from Cors.handle() firing its OPTIONS branch — because the test runner's actual HTTP request is GET, so the engine cgi.request_method that Cors falls back to is "GET", not "OPTIONS".
The test validates the important thing: OPTIONS with CORS middleware registered reaches the pipeline instead of 404ing. But no test proves the production chain end-to-end: preflightContext (no cgi key) → Cors reads engine cgi.request_method = "OPTIONS" → Cors fires its OPTIONS branch. The Lucee 7 read-only CGI scope makes this difficult to exercise in unit tests, and the comment is honest about the gap. The new CorsSpec.cfc test (lines 82-100 of the diff) validates the Cors OPTIONS branch via injected context; the fallback-to-engine-scope path remains untested at unit level.
This is acceptable given the constraints and worth tracking. If future test infrastructure enables CGI mocking on Lucee 7, this is the first test to write.
Coverage for the three scenarios is otherwise solid — OPTIONS+CORS no longer 404s, OPTIONS without CORS still 404s, GET with CORS still routes normally. The BDD spec is well-structured and the beforeEach/afterEach save/restore pattern matches existing dispatch specs (requestSpec.cfc).
Cross-engine
No issues found.
IsInstanceOf(local.mw, "wheels.middleware.Cors")— standard BIF, works on Lucee, Adobe, and BoxLang.for (local.mw in variables.$middlewarePipeline.getMiddleware())— array iteration, cross-engine safe.UCase(local.preflightMethod) == "OPTIONS"— case-insensitive guard covers engines that normalise the verb differently.- No
clientscope usage, no bracket-notation-in-closure, nostruct.map()collision, noapplicationscope function members.
Docs
Both v4-0-0/digging-deeper/cors.mdx and v4-0-1-snapshot/digging-deeper/cors.mdx carry the new <Aside type="caution"> warning that route-scoped Cors does not benefit from the preflight short-circuit. This is the most likely developer surprise and surfacing it prominently is the right call.
Commits
All commits conform to commitlint.config.js: types are fix, docs, and chore; subjects are sentence-case and under 100 chars. No DCO issues visible.
Re-commits the auto-generated blog visual baseline PNG produced by the refresh-visual-baselines.yml workflow on this PR branch. The bot's default commit body expanded the branch name inline, producing a line that exceeded the 100-char commitlint limit; this re-commit carries the same binary content with a wrapped message body. Triggered after develop merged PR #2735, which added a new blog post (wheels + claude stdio MCP setup) that shifted the blog index render. Signed-off-by: Peter Amiri <peter@alurium.com>
1246379 to
279bb93
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes the OPTIONS preflight 404 regression by inserting a dispatch-layer short-circuit before $findMatchingRoute(). The mechanism is sound — IsInstanceOf gating on wheels.middleware.Cors, a fail-safe try/catch around $getRequestMethod(), and the correct "hoist closure" pattern for preflightHandler. No correctness, cross-engine, or security issues found. Leaving as --comment for two minor test-design observations and a small documentation gap.
Tests
CorsPreflightDispatchSpec.cfc test 1 passes for a subtly different reason than its comment implies — worth a clarifying note.
vendor/wheels/tests/specs/middleware/CorsPreflightDispatchSpec.cfc:34-68
The comment correctly identifies that Cors.handle() reads cgi.request_method from the engine CGI scope (the test runner's GET), not from request.cgi.request_method (the OPTIONS set by the test). So Cors.handle() falls through to next() and the no-op preflightHandler returns "".
This is actually correct test design for the dispatch layer concern: the test validates that Dispatch.$request() gates on $getRequestMethod() returning "OPTIONS" and runs the pipeline early, not that Cors.handle() itself short-circuits. CorsSpec.cfc covers the latter.
The confusion point for a future reader: the test sets request.cgi["request_method"] = "OPTIONS" and $getRequestMethod() reads request.cgi.request_method, so the dispatch-layer gate fires. But Cors.cfc::handle() calls StructKeyExists(arguments.request, "cgi") first — preflightContext has no cgi key, so Cors.handle() falls back to the engine CGI scope, which is GET. The test result "" is produced by the no-op handler, not by Cors.handle() short-circuiting.
This is fine as-is — the comment acknowledges it — but if a future author wants full path coverage at the dispatch layer, they could inject cgi = {request_method = "OPTIONS", http_origin = ""} into preflightContext in Dispatch.cfc. That would also let Cors.handle() exercise its OPTIONS branch in the real dispatch path. Not required for this fix; noting for the record.
CorsSpec.cfc new test correctly exercises Cors.handle() OPTIONS branch via injected request.cgi.
vendor/wheels/tests/specs/middleware/CorsSpec.cfc:85-100
local.reqCtx = {cgi = {request_method = "OPTIONS", http_origin = "https://example.com"}};
local.result = local.cors.handle(
request = local.reqCtx,
next = function(required struct request) {
return "should-not-reach";
}
);
expect(local.result).toBe("");This is the unit test that was requested by the previous Reviewer A round and it now exists. The inline next = function(...) {...} is a method-call argument (not a new Component(callback = function(){...}) constructor call), so the Adobe CF ArrayStoreException gotcha from .ai/wheels/cross-engine-compatibility.md does not apply here. Clean.
Docs
<Aside> note should clarify that IsInstanceOf detects subclasses but NOT fully independent CORS implementations.
web/sites/guides/src/content/docs/v4-0-1-snapshot/digging-deeper/cors.mdx (and the v4-0-0 copy):
The preflight short-circuit that prevents unmatched `OPTIONS` requests from reaching the route table applies only when `Cors` is registered in the **global** pipeline via `config/settings.cfm`.This is accurate for the route-scoped case. A second sentence worth adding: because $hasPreflightCapableMiddleware() in Dispatch.cfc uses IsInstanceOf(mw, "wheels.middleware.Cors"), custom subclasses of wheels.middleware.Cors are also detected and benefit from the short-circuit. However, a completely independent CORS middleware implementation (not extending wheels.middleware.Cors) would not trigger the dispatch gate — such users should register wheels.middleware.Cors globally or extend it.
Not a blocker; the current note is accurate, just incomplete for the custom-middleware case.
Commits
chore(web): refresh visual baseline(s) (blog) at 279bb9373 is the PR head and is unrelated to the middleware fix. This appears to be an automated visual regression baseline refresh triggered by a develop merge. It does not affect any functional code and commitlint passes (chore + lower-case subject). No action needed.
All other commits in the PR-specific chain conform to commitlint.config.js (type from the allowed list, subject ≤ 100 chars, not ALL-CAPS).
Wheels Bot -- Reviewer B (round 1)A's review is accurate and thorough. No sycophancy, no false positives, no missed issues on the diff I re-scanned. Aligning on approve. SycophancyNone detected. A explicitly left the review as COMMENTED rather than APPROVED, and all findings are supported by specific code references. False positivesNone detected. I verified each of A's claims:
Missed issuesNone detected. Cross-engine scan found nothing actionable:
Verdict alignmentA's COMMENTED verdict (no blockers, informational notes only) is consistent with the findings. No issues requiring changes were identified. ConvergenceAligned. A found no correctness, cross-engine, or security issues, and my independent re-scan of the diff confirms the same. The fix is targeted and correct: |
…t-short-circuit-option
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is round 10. The PR's own CORS dispatch fix has been unchanged since round 4 and all prior findings are fully resolved. The only new functional code is the whereIn/whereNotIn empty-array fix from PR #2736, pulled in via the develop merge commit (5a4a29923). That fix is correct and well-tested. Three minor observations are noted below; none warrant requesting changes. Verdict: --comment.
What changed since round 9
Two new commits arrived:
5a4a29923--feat(web/blog,model): beyond findAll post + whereIn empty-array fix (#2736): addsvariables.$alwaysEmptyflag toQueryBuilder.cfc, short-circuits all eight terminal methods for emptywhereIn(), makeswhereNotIn(empty)a no-op, adds 14 new specs, and updates the methods table in both doc versions.1438257d7-- routine merge-of-develop commit. No code changes to the CORS path.
Correctness
whereIn/whereNotIn empty-array fix is correct.
The $alwaysEmpty flag approach avoids the WHERE-parser trap cleanly. A "1 = 0" literal injection would trip the WHERE-clause parser's property-extraction regex in vendor/wheels/model/sql.cfc; the flag short-circuits before the SQL layer ever sees the clause. The eight terminal short-circuits (findAll, findOne/first, count, exists, updateAll, deleteAll, findEach, findInBatches) are all covered.
Thread safety is preserved: each model("author").whereIn(...) call triggers onMissingMethod in vendor/wheels/model/onmissingmethod.cfc:57, which creates a fresh QueryBuilder instance (new wheels.model.query.QueryBuilder(modelReference = this)) before delegating. variables.$alwaysEmpty is instance state, not class state.
findAll() short-circuit creates a varchar-typed empty query.
vendor/wheels/model/query/QueryBuilder.cfc:335-338
if (variables.$alwaysEmpty) {
return QueryNew(variables.modelReference.$classData().columnList);
}QueryNew(columnList) without a type list defaults all columns to varchar. A normal findAll() result carries proper column types (integer for IDs, date for timestamps). If a caller inspects column metadata type-sensitively (e.g. IsNumeric(q.id) on an empty result) it would receive "" rather than a numeric zero. In practice, zero-row results are guarded with q.recordcount > 0 before row access, so the practical severity is low. Worth noting for any future caller that introspects types rather than values.
The test only asserts ListLen(result.columnList) > 0, not column-type parity with a real findAll() result -- acceptable for this fix but a future improvement opportunity if type-sensitive callers emerge.
Cross-engine
No new cross-engine concerns.
IsArray(),ListToArray(),ArrayLen()-- standard BIFs, consistent across Lucee, Adobe CF, and BoxLang.ListToArray("")returns[](zero elements) on all three engines, sowhereIn("id", "")correctly sets the flag.- Closures in the new test specs (
findEach(callback=function(row){...}),findInBatches(callback=function(batch){...})) are method-call arguments, not constructor arguments -- the Adobe CFArrayStoreException: ASTcffunctiontrap applies only tonew Foo(callback = function(){...})at object construction time. - The tests use
var state = {invoked: 0}and accessstate.invokedinside closures, matching the CLAUDE.md closure-capture guideline (shared struct, not outerlocalvar). QueryNew(columnList)-- standard CFML, cross-engine safe.
Tests
Fourteen new specs in vendor/wheels/tests/specs/model/queryBuilderSpec.cfc cover:
- Empty array and empty list variants for both
whereInandwhereNotIn - Composition with other
where()clauses - Every terminal method:
count,findAll,first/findOne,exists,updateAll,deleteAll,findEach,findInBatches - The documented
select()-ignored-on-short-circuit caveat
Minor: get() terminal not explicitly tested. get() is an alias for findAll() (delegates via return findAll(argumentCollection = arguments)) and is therefore covered transitively. No action needed, but noting for completeness.
Docs
Both v4-0-0/basics/query-builder-and-scopes.mdx and v4-0-1-snapshot/basics/query-builder-and-scopes.mdx now carry the short-circuit annotation in the methods table. Both doc versions updated consistently. CHANGELOG [Unreleased] > Fixed entry is present and accurate.
The existing <Aside type="caution"> warning for the CORS preflight limitation (route-scoped vs global registration) remains correct and unchanged in both doc versions.
Commits
feat(web/blog,model): beyond findAll post + whereIn empty-array fix (#2736) -- valid type, multi-value scope (web/blog,model) is allowed since scope is unrestricted, subject within 100 chars, not ALL-CAPS. Merge commit (1438257d7) carries no commitlint obligation.
Security
No user input reaches SQL strings in the new code. $validatePropertyName() is called before the empty-array guard in both whereIn and whereNotIn, so the property argument is validated before any short-circuit. No XSS, CSRF, or secret-leakage concerns.
All findings across rounds 1-9 remain resolved. The new whereIn/whereNotIn fix is clean. Ready to merge once a human maintainer approves -- the bot cannot self-approve.
Re-commits the auto-generated blog visual baseline PNG produced by the refresh-visual-baselines.yml workflow on this PR branch. The bot's default commit body expanded the branch name inline, producing a line that exceeded the 100-char commitlint limit; this re-commit carries the same binary content with a wrapped message body. Triggered after develop merged PR #2736, which added a new blog post (beyond findAll) that shifted the blog index render. Same workflow body-length bug previously hit on #2734 and #2735 baseline refreshes in this PR. Signed-off-by: Peter Amiri <peter@alurium.com>
0e478b3 to
13bae60
Compare
Wheels Bot -- Reviewer B (round 1)A's round-10 review is accurate. The CORS dispatch fix has been stable since round 4, all prior findings remain resolved, and A's assessment of the incoming develop changes is sound. One procedural note on scope; no false positives; no missed issues. Converging on approve. SycophancyNone detected. A chose COMMENTED over APPROVED and cited specific code-level evidence for every finding. The positive framing is grounded in per-finding verification, not a blanket LGTM. False positivesNone. I verified A's key claims against the diff:
Missed issuesNone in the actual PR diff. The CORS changes (Dispatch.cfc, Cors.cfc, both test specs, both cors.mdx docs) are unchanged since round 4 and have been thoroughly vetted across prior rounds. One procedural observation that does not affect the verdict: A devotes the bulk of round 10 to analysing the Verdict alignmentCOMMENTED with no blocking changes is consistent with findings that are all informational (varchar column typing, a transitive test gap). No correctness, cross-engine, or security issues remain open. Appropriate. ConvergenceAligned on approve. The PR's own CORS dispatch fix is correct and well-tested. All findings across rounds 1-9 are resolved. The one observational note in round 10 (varchar-typed empty query) is low-severity, already-in-develop code that does not affect what this PR merges. No changes are required before merge; a human maintainer approval is all that stands between this and |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a targeted, well-understood fix for a genuine regression: the new middleware pipeline ran after route matching, so wheels.middleware.Cors could never intercept a browser OPTIONS preflight against an unregistered verb path. The implementation is correct, the limitation (global-pipeline-only) is documented in both the guides and the code, and test coverage is adequate. No correctness, cross-engine, or security issues. Submitting --comment with one observation about test scope and one design note.
Correctness
Dispatch short-circuit reads request.cgi.request_method; preflightContext intentionally omits cgi.
Dispatch.$request() gates the short-circuit on $getRequestMethod() which reads request.cgi.request_method (Dispatch.cfc:804). The preflightContext struct passed to $middlewarePipeline.run() does not carry a cgi key:
// Dispatch.cfc:283-287
local.preflightContext = {
params = {},
route = {},
pathInfo = arguments.pathInfo
};So in production Cors.handle() falls back to the engine cgi scope (which IS OPTIONS during a real browser preflight) and correctly short-circuits. The omission of cgi is internally consistent with the normal requestContext at lines 331-337, which also lacks a cgi key and relies on the same fallback path.
The first integration test validates dispatch routing, not CORS header emission.
CorsPreflightDispatchSpec.cfc:34-68 asserts result == "" but that result comes from Dispatch's no-op preflightHandler closure, not from Cors's OPTIONS branch. In the test context, request.cgi["request_method"] is set to "OPTIONS" (triggering the dispatch short-circuit) but cgi.request_method in the engine scope is still "GET" (the test runner's request). Since preflightContext has no cgi key, Cors.handle() reads the engine CGI scope ("GET"), does not enter the OPTIONS branch, and falls through to next() — which is the no-op handler.
The test comment (lines 38-43) documents this explicitly, and the newly added unit test in CorsSpec.cfc:85-100 covers the Cors OPTIONS branch directly via injected request.cgi. Coverage is adequate; the split is an acknowledged test-environment constraint, not an oversight.
request.wheels.params = {} before the short-circuit is correct.
Setting an empty params struct at Dispatch.cfc:277 before entering the pipeline prevents a key-not-found error in any debug or logging code that reads request.wheels.params outside the controller context. The normal path sets this at line 300 post-$paramParser. No issue.
Tests
Test assertions are narrower than the behaviour they claim to guard — but this is documented and accepted.
The integration test proves Wheels.RouteNotFound is NOT thrown; it does not verify CORS response headers were emitted for the preflight. In production, Cors.handle() correctly reads cgi.http_origin and cgi.request_method from the engine scope and emits the headers. This path is unit-tested in CorsSpec.cfc.
If a future refactor shifts the origin/method lookup order in Cors.handle(), the unit test would catch it while the integration test would silently remain green. The tests are correct for what they assert; the gap is that there is no end-to-end assertion of emitted headers. This is pre-existing test-environment friction with read-only engine CGI scopes, not a new gap introduced here.
Docs
Both v4-0-0/digging-deeper/cors.mdx and v4-0-1-snapshot/digging-deeper/cors.mdx received the <Aside type="caution"> note explaining that the preflight short-circuit applies only to globally-registered Cors middleware. The CHANGELOG entry under [Unreleased] > Fixed is detailed and accurate.
Commits
All bot-authored commits conform to commitlint.config.js: fix(middleware): and docs(middleware): types, subjects under 100 chars, DCO Signed-off-by: trailers present. The merge commits and chore(web): refresh visual baseline(s) commits are develop-sync noise and are not reviewed.
Design note (not a blocker)
$hasPreflightCapableMiddleware() uses IsInstanceOf(local.mw, "wheels.middleware.Cors"), which means a custom CORS middleware that implements MiddlewareInterface but does not extend wheels.middleware.Cors will not trigger the dispatch short-circuit. The <Aside> in the guides addresses the global-vs-scoped limitation but not this subclassing constraint. Worth a follow-up doc note, not a blocker for this PR.
Wheels Bot -- Reviewer B (round 1)A's review is substantively accurate. All three factual claims I checked against the diff hold up, the COMMENTED verdict is proportionate to the acknowledged gaps, and no cross-engine or security issues were glossed over. I am aligned and calling converged-changes. SycophancyNone detected. A chose COMMENTED rather than APPROVED despite praising the implementation as "surgically correct". That framing is earned: the False positivesNone. I verified each factual claim A made against the actual source:
Missed issuesOne framing gap A did not state with full precision:
Verdict alignmentAligned. COMMENTED is the correct verdict: the implementation is correct, the limitations are documented, and the test coverage split is acceptable given test-environment constraints. ConvergenceA's review is technically sound, the verdict is proportionate, and no false positives or meaningful missed issues require further iteration. Marking convergence on converged-changes -- COMMENTED maps to request-changes in the workflow sense, reflecting the acknowledged follow-up items (subclassing doc note, potential future end-to-end header assertion) that are deferred but not blocking. |
Address Reviewer A's design note (echoed in Reviewer B's converged-changes verdict) about the dispatch preflight short-circuit's subclassing constraint. `Dispatch.$hasPreflightCapableMiddleware()` detects preflight-capable middleware with `IsInstanceOf(mw, "wheels.middleware.Cors")`, so the short-circuit fires for the canonical class and any subclass that extends it -- but a custom CORS middleware that implements `MiddlewareInterface` directly without extending `wheels.middleware.Cors` will not trigger the short-circuit. Existing `<Aside>` only covered the global-vs-scoped limitation; this commit extends it with the subclassing constraint and the recommended workaround (extend `wheels.middleware.Cors`). Docs updated in both v4-0-0 and v4-0-1-snapshot guides; no code change because the behaviour itself is correct and `IsInstanceOf` is the right detection primitive for a stdlib-recognised CORS contract. 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 of the convergence loop on
Intentionally not addressed (not in the consensus to act on):
Test runner: no specs are affected by this docs-only change to 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 round 11. The only new code since round 10 is a single documentation-only commit (c8e84b7aa) that directly addresses the design note raised in rounds 9 and 10 about the IsInstanceOf subclassing constraint. The addition is factually accurate and both doc versions are updated consistently. No new correctness, cross-engine, or security issues. Verdict: --comment.
What changed since round 10
One commit:
c8e84b7aa—docs(middleware): address Reviewer A/B consensus findings (round 2): extends the existing<Aside type="caution">in bothv4-0-0/digging-deeper/cors.mdxandv4-0-1-snapshot/digging-deeper/cors.mdxwith a paragraph explaining that$hasPreflightCapableMiddleware()usesIsInstanceOf(mw, "wheels.middleware.Cors"), meaning subclasses benefit from the dispatch short-circuit but independentMiddlewareInterfaceimplementations do not.
Correctness
The new paragraph's claims are accurate against the code:
vendor/wheels/Dispatch.cfc:373—IsInstanceOf(local.mw, "wheels.middleware.Cors")is exactly the detection primitive described.vendor/wheels/middleware/Cors.cfc:108andvendor/wheels/middleware/MiddlewareInterface.cfc:17—handle()is the correct method name on both the concrete class and the interface; the recommended workaround (extendwheels.middleware.Cors, overridehandle()) is valid.
No code was modified; the IsInstanceOf behaviour itself is correct and unchanged.
Docs
Both v4-0-0 and v4-0-1-snapshot docs are updated consistently. Adding the paragraph to the released v4-0-0 guide is appropriate — the preflight short-circuit fix ships with this PR, so the subclassing constraint is a current v4.0.0 limitation for any user who writes custom CORS middleware.
Commits
docs(middleware): address Reviewer A/B consensus findings (round 2) — valid type (docs is correct for a comment/doc-only change), valid unrestricted scope (middleware), subject within 100 chars, not ALL-CAPS, DCO-signed (Signed-off-by: claude[bot]).
All findings across rounds 1–10 remain resolved. Ready to merge once a human maintainer approves — the bot cannot self-approve.
Wheels Bot — Reviewer B (round 1)A's review of the round-11 delta is accurate and terse. No sycophancy, no false positives, no missed issues. Converging on approve. SycophancyNone detected. A chose COMMENTED rather than APPROVED (bot self-approval restriction applies). The positive framing — "factually accurate", "both doc versions are updated consistently" — is grounded in line-level verification against the source, not a blanket LGTM. False positivesNone. I verified each of A's three code references:
The recommended workaround in the new Aside paragraph (extend A's DCO assessment is also correct: Missed issuesNone. The commit touches only the two Verdict alignmentCOMMENTED (not APPROVED) is consistent with a documentation-only delta and the bot's self-approval restriction. No changes are requested. Appropriate. ConvergenceAligned on approve. A and I agree: the docs addition is factually accurate, both guide versions are updated consistently, and no correctness, cross-engine, or security issues remain open. The PR is review-clean for this SHA. |
…s-middleware-cors-cannot-short-circuit-option Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot - Reviewer A
TL;DR: This is round 12. All prior CORS dispatch-fix findings (rounds 1-11) are fully resolved and the authored CORS code in Dispatch.cfc, Cors.cfc, both test specs, and both MDX docs is byte-for-byte identical to the round-11 review. The only new code arrived via a single develop merge commit (c6b6787) carrying PR #2733 (fix(view): paginationNav anchor modes default to window-aware auto-suppress). That change is functionally correct, cross-engine clean, and well-tested. Three minor observations below; none warrant requesting changes. Verdict: --comment.
What changed since round 11
One commit arrived via the develop merge:
707f60778-- fix(view): paginationNav anchor modes default to window-aware auto-suppress (#2733)
Changes touch:
vendor/wheels/view/pagination.cfc-- newviewStylearg onpageNumberLinks()andpaginationNav();showFirst/showLast/showPrevious/showNextwidened frombooleantoanyaccepting"auto"/"always"/"never";windowSizepromoted to a first-classpaginationNav()arg (removed fromallowedSubArgs, now passed explicitly topageNumberLinks()); new internal helpers$paginationAnchorMode(),$paginationShouldShowAnchor(),$renderPaginationPage(),$renderPaginationNav(),$renderPaginationNavLink().vendor/wheels/events/init/functions.cfm--showFirst/showLast/showPrevious/showNextdefaults changed fromtrueto"auto";windowSize = 2andviewStyle = "plain"added to bothpaginationNavandpageNumberLinksdefaults.vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc-- 18 new viewStyle specs, 18 new anchor-mode specs, two existing tests updated to account for the new"auto"defaults.CLAUDE.md,CHANGELOG.md,v4-0-1-snapshot/upgrading/3x-to-4x.mdx-- documentation updated.
Correctness
Core fix is sound. The root problem addressed by PR #2733 -- windowSize used by the auto-hide boundary predicates was silently different from the windowSize passed to pageNumberLinks() when a caller omitted the argument -- is fixed cleanly. windowSize is now a declared formal parameter on paginationNav() with a default of 2 (matching pageNumberLinks()'s own default), and is explicitly threaded to both pageNumberLinks() and $paginationShouldShowAnchor() so both always agree on the window size.
$paginationAnchorMode() IsBoolean() ordering is correct. IsBoolean("never") returns false in CFML, so the IsBoolean branch does not intercept the string tokens. The ListFindNoCase branch follows and correctly identifies "auto"/"always"/"never". IsBoolean("1") returns true (CFML treats numeric 1 as boolean), so showFirst=1 silently coerces to "always" -- consistent with CFML broad boolean semantics and the documented backwards-compatibility coercion.
$paginationShouldShowAnchor() auto-path for previous/next is intentional. The switch statement matches "first" and "last" with explicit boundary predicates and falls out to return true for "previous" and "next". Under "auto", prev/next always delegate to their sub-helper, which renders a disabled <span class="disabled"> at the boundary -- handled inside previousPageLink()/nextPageLink() on the plain path and inside $renderPaginationNavLink() via isDisabled = local.firstDisabled on the viewStyle path.
Conventions
Nit 1 -- $paginationShouldShowAnchor() switch has no default case.
vendor/wheels/view/pagination.cfc (switch around line 618):
switch (arguments.side) {
case "first":
return (arguments.pg.currentPage - arguments.windowSize) > 1;
case "last":
return arguments.pg.totalPages > (arguments.pg.currentPage + arguments.windowSize);
}
return true;An unrecognised side value silently returns true, rendering the anchor unconditionally. Since side is always a constant literal at every internal call site, this will never fire in practice. A default: return true; case (or a brief comment noting that "previous" and "next" intentionally fall through) would make the intent explicit rather than appearing accidental. Feel free to address in a follow-up.
Nit 2 -- behavioral default change not surfaced in a standalone v4.0.x to v4.0.1 upgrade note.
The showFirst/showLast defaults switching from true to "auto" is a silent behavioral change for existing v4.0.x apps calling paginationNav() with no explicit showFirst/showLast args -- First/Last links that previously always appeared may now be suppressed when the page-number window reaches the boundary. CHANGELOG [Unreleased] > Fixed and CLAUDE.md both document the new semantics, and 3x-to-4x.mdx covers the migration. There is no dedicated v4.0.0 to v4.0.1 upgrade entry pointing existing v4 users at showFirst="always" as the workaround. Not blocking -- CHANGELOG is the authoritative record for within-v4 changes -- but worth a follow-up doc issue if a v4 minor-version migration guide is planned.
Cross-engine
All new helpers use public access -- required for $integrateComponents() to pull them into the view mixin scope on Lucee/Adobe per CLAUDE.md's "private mixin functions" gotcha. No closures, no client scope, no bracket-notation function calls inside closures, no struct.map() member-function collision. switch in CFScript does not fall through without explicit break/return on Lucee, Adobe, or BoxLang. StructCopy(arguments.subArgs) in $renderPaginationNav() is a shallow copy -- safe because subArgs holds only scalar values. No Left(str, 0) or other Lucee 7 edge cases introduced.
Tests
The 36 new specs (18 viewStyle + 18 anchor-mode) cover Bootstrap 5/4/Tailwind markup contracts, plain style unchanged, disabled states at boundary pages, "always"/"never"/boolean coercion, windowSize coherence between predicates and rendered links, and invalid viewStyle strings throwing Wheels.InvalidViewStyle. The two modified existing tests correctly account for the "auto" defaults.
One minor gap: no spec exercises viewStyle="tailwind" disabled-state rendering for next/last anchors on the last page -- only first/previous disabled-on-first-page is tested for Tailwind. Symmetric coverage would close the matrix but is not blocking.
Commits
707f60778 fix(view): paginationNav anchor modes default to window-aware auto-suppress (#2733) -- valid type (fix is correct: the root issue was the auto-suppress predicates using a different windowSize than the rendered window), valid unrestricted scope (view), subject within 100 chars, not ALL-CAPS. Merge commit (c6b678795) carries no commitlint obligation.
Security
No user input reaches SQL strings in the new pagination code. EncodeForHTML() is applied to disabled-span text in $renderPaginationNavLink() when encode=true. Enabled-path text is passed through linkTo() which handles encoding internally. No XSS, CSRF, or secret-leakage concerns.
All findings across rounds 1-11 remain resolved. The PR #2733 pagination changes are clean and correct. Ready to merge once a human maintainer approves -- the bot cannot self-approve.
Wheels Bot — Reviewer B (round 1)A's review is accurate on the CORS code that actually appears in the PR diff. The bulk of the analysis covers pagination changes (PR #2733) that arrived via the develop merge commit and are already in SycophancyNone detected. A chose COMMENTED over APPROVED (self-approval restriction), grounded every positive statement in per-finding verification, and raised three nits rather than issuing a blanket LGTM. False positivesNone detected. The one factual claim about the CORS diff — that Missed issuesNone. The actual PR diff (seven CORS files) has been reviewed across eleven prior rounds. Round 11 issued Verdict alignmentCOMMENTED is consistent with a situation where the authored code is unchanged, the bot cannot self-approve, and the three nits A raises are on develop-side pagination code outside the PR diff. Appropriate. ConvergenceAligned on approve. The CORS dispatch fix is correct and unchanged since round 4. All genuine findings across rounds 1-11 are resolved. The develop merge introduces no new issues to the seven files this PR actually changes. No changes are required before merge — a human maintainer approval is all that remains. |
1 participant
Summary
wheels.middleware.Corscould not short-circuitOPTIONSpreflight requests because the middleware pipeline ran after$findMatchingRoute(). A preflight against a path that only declaredPOST/PUT/PATCH/DELETEreturned404 Wheels.RouteNotFoundbefore the CORS middleware's preflight branch could fire — leaving the middleware strictly less capable than the legacy 3.xset(allowCorsRequests=true)global, and breaking every cross-origin POST/PUT/PATCH/DELETE from configured browsers.Dispatch.$request()now checks the request verb up front. When the verb isOPTIONSand the global pipeline contains awheels.middleware.Corsinstance (detected viaIsInstanceOf), it runs the pipeline against a no-op core handler before route matching, so the CORS middleware can set headers and short-circuit without hitting the route table. Dispatch behavior forOPTIONSwithout CORS middleware (still 404s) and for non-OPTIONSverbs (still routed normally) is unchanged.This matches the triage comment's surgical option 3 — gated on an actual
wheels.middleware.Corsinstance in the pipeline so we don't silently swallowOPTIONSfor apps that aren't using CORS middleware.Related Issue
Fixes #2703
Type of Change
Feature Completeness Checklist
Signed-off-by:(git commit -s)CorsPreflightDispatchSpec.cfcundervendor/wheels/tests/specs/middleware/covers the failing-then-passing scenario (OPTIONS + CORS no longer 404s) plus two regression guards (OPTIONS without CORS still 404s; non-OPTIONS verbs still route normally)bot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-up[Unreleased] > Fixedcurl http://localhost:60007/wheels/core/tests?db=sqlite&format=jsonreports 3556 pass / 0 fail / 0 error (full core suite, Lucee 7 + SQLite) after the fix; middleware bundleCorsPreflightDispatchSpecis 3/3Test Plan
vendor/wheels/tests/specs/dispatchsuite (96 specs) andvendor/wheels/tests/specs/middlewaresuite (147 specs incl. new ones) green/wheels/core/tests?db=sqlitesuite (3556 specs) green