docs(web/guides): document allowCorsRequests global setting still works in 4.0#2638
Conversation
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This is a pure documentation PR that adds a well-researched "CORS — two paths in 4.0" sub-section to both the v4-0-0 and v4-0-1-snapshot upgrade guides, clarifying that set(allowCorsRequests=true) still works in 4.0, documenting the lifecycle difference between the global setting and wheels.middleware.Cors, and explaining precedence when both are active. The prose accurately reflects the implementation (verified against vendor/wheels/events/EventMethods.cfc, vendor/wheels/events/init/security.cfm, vendor/wheels/Global.cfc, and vendor/wheels/middleware/Cors.cfc). However, the commit header exceeds the 100-character commitlint limit enforced in commitlint.config.js, which is a required gate. One minor doc precision issue is also noted.
Correctness
The implementation claims were verified against the codebase:
- Global setting fires in
onRequestStart, callsaborton OPTIONS (EventMethods.cfclines 239–262) — documented correctly. - Middleware runs during dispatch (
Dispatch.cfclines 314–324, called fromindex.cfmafteronRequestStart) — documented correctly. - Middleware wins on non-OPTIONS requests because
cfheaderinCors.cfc(lines 72–79) overwrites what$setCORSHeadersemitted earlier — documented correctly. Access-Control-Max-Ageabsent from global setting path — confirmed: neither$setCORSHeadersinGlobal.cfcnorsecurity.cfminitializes amaxAgesetting. Table row is accurate.- Available since Wheels 2.x — confirmed by CHANGELOG line 810 (
2.0.0-rc.1entry).
Minor precision issue (web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx, line 78):
All six `accessControlAllow*` settings are initialized in the 4.0 security defaults
security.cfm initializes exactly 5 accessControlAllow*-prefixed settings (accessControlAllowOrigin, accessControlAllowMethods, accessControlAllowMethodsByRoute, accessControlAllowCredentials, accessControlAllowHeaders) plus allowCorsRequests — which does not share the accessControlAllow* prefix. A reader scanning the code will count 5, not 6, entries with that prefix. Suggested fix:
All six CORS settings (`allowCorsRequests` and the five `accessControlAllow*` settings) are initialized in the 4.0 security defaults and read on every request.
The same line appears identically in the v4-0-1-snapshot copy and needs the same correction.
Commits
commitlint violation — header exceeds 100-character limit:
docs(web/guides): document allowCorsRequests global setting still works in 4.0 and clarify precedence vs middleware
This header is 115 characters. commitlint.config.js line 27 sets 'header-max-length': [2, 'always', 100]. This is a hard-error rule (severity 2). Suggested shorter header (97 chars):
docs(web/guides): clarify allowCorsRequests still works in 4.0 and middleware precedence
Tests
No code changes — docs-only PR. No test additions are required or expected.
Security
No security concerns. The PR accurately documents (rather than changes) CORS behavior. The precedence section correctly warns against running both paths simultaneously without intent.
Wheels Bot — Reviewer B (round 1)A's review is substantively accurate. Both findings are real, the code-verification work is solid, and the precision issue in the docs is confirmed against SycophancyNone detected. A's positive framing ("well-researched", "accurately reflects the implementation") is each time backed by a specific file + line citation. That is evidence, not filler. False positives
Otherwise none. A's precision finding about "All six The commitlint violation is also real: the commit header is 115 characters against a Missed issuesNone detected. A covers correctness (verified against four source files), commits, tests, and security. The docs-only scope makes this a complete checklist. The Verdict alignmentA used COMMENTED for a commitlint severity-2 (hard-error) rule violation. REQUEST_CHANGES would better match the finding: a hard-gate CI failure requires a commit rewrite, not just a follow-up note. The finding is accurate; the verdict understates it. ConvergenceAligned on changes needed. Two concrete fixes are required:
Note: fix (2) requires amending the commit and force-pushing the branch; address-review can handle the doc text in (1), but the commit message rewrite may need a separate step or human intervention. |
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Skipped: commitlint header-length violation on commit No test run required — 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
The round-1 address commit (f7f0acaf9) correctly applies the prose precision fix flagged by both Reviewer A and Reviewer B to both the v4-0-0 and v4-0-1-snapshot upgrade guides. That finding is resolved. However, the commitlint violation on the original commit (8999271fa) — a 115-character header against a hard-error 100-character limit — remains unresolved. One of two requested changes is done; one is not. Verdict: changes needed.
Correctness
The fix at line 78 of both guide files is accurate and matches vendor/wheels/events/init/security.cfm lines 17–22:
All six CORS settings (`allowCorsRequests` and the five `accessControlAllow*` settings) are initialized in the 4.0 security defaults and read on every request.That is the correct count: 5 settings share the accessControlAllow* prefix (accessControlAllowOrigin, accessControlAllowMethods, accessControlAllowMethodsByRoute, accessControlAllowCredentials, accessControlAllowHeaders) plus allowCorsRequests (no shared prefix). The wording is now precise in both copies. This finding is closed.
Commits
Outstanding: commitlint violation on the original commit.
Commit 8999271fa still has a 115-character header:
docs(web/guides): document allowCorsRequests global setting still works in 4.0 and clarify precedence vs middleware
commitlint.config.js line 27 enforces 'header-max-length': [2, 'always', 100] — severity 2 is a hard error that fails CI. The round-1 address commit did not amend or squash this. The new commit f7f0acaf9 (docs(web/guides): address Reviewer A/B consensus findings (round 1), 67 chars) is fine on its own.
Required action: squash or rebase-amend 8999271fa to a header within 100 characters. The header proposed by Reviewer B (88 chars) is suitable:
docs(web/guides): clarify allowCorsRequests still works in 4.0 and middleware precedence
Squashing both commits into one with the corrected header would also produce cleaner history for a docs-only change.
Tests
No code changes — docs-only PR. No test additions required or expected.
Security
No concerns. The PR documents existing behavior; no code changes.
Wheels Bot — Reviewer B (round 2)A's round-2 review is accurate. The prose fix is confirmed closed; the commitlint violation on the original commit remains the only outstanding item. The same verdict/state mismatch from round 1 recurs: a hard-error CI failure was submitted as COMMENTED rather than REQUEST_CHANGES. Findings align; convergence is on changes needed. SycophancyNone detected. A's positive closure on the prose finding is accompanied by an explicit inline code block showing the exact corrected text from both guide files, and is cross-referenced to the source at False positivesNone. All of A's claims check out:
Missed issuesNone. This is a docs-only change. No cross-engine concerns, no test gaps, and no security issues. A correctly scoped the review to the one remaining blocking item. One marginal observation A could have noted but is not a blocker: the PR title ( Verdict alignmentFor the second consecutive round A submitted a COMMENTED verdict for a severity-2 (hard-error) commitlint violation. REQUEST_CHANGES is the appropriate verdict when a finding would produce a known CI failure. The finding itself is correct; the state choice understates the urgency. ConvergenceAligned on changes needed. The single remaining action is for a human maintainer to squash or rebase-amend |
Wheels Bot — Address Review (round 2)No commits applied. Reviewer A and Reviewer B converged on a single
Resolution paths for a human maintainer:
Doc content is correct as of |
…ddleware precedence Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
- Clarify CORS settings count in v4-0-0/upgrading/3x-to-4x.mdx line 78 - Mirror same correction in v4-0-1-snapshot/upgrading/3x-to-4x.mdx line 78 Replaces the imprecise "All six `accessControlAllow*` settings" phrasing (which scans as 5 entries with that prefix in security.cfm) with "All six CORS settings (`allowCorsRequests` and the five `accessControlAllow*` settings)" — accurate against vendor/wheels/events/init/security.cfm lines 17–22. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
f7f0aca to
6409c00
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This docs-only PR adds a "CORS — two paths in 4.0" sub-section to both the v4-0-0 and v4-0-1-snapshot upgrade guides, clarifying that the global set(allowCorsRequests=true) path is still honored in 4.0, documenting the lifecycle difference between the two CORS paths, and explaining precedence when both run simultaneously. Both outstanding findings from prior review rounds are now resolved — the diff is clean and ready to merge.
Correctness
Both prior correctness findings are resolved:
-
Precision fix applied (both copies). Line 78 in
v4-0-0/upgrading/3x-to-4x.mdxand its snapshot mirror now reads:All six CORS settings (
allowCorsRequestsand the fiveaccessControlAllow*settings) are initialized in the 4.0 security defaults…This accurately counts the 5
accessControlAllow*-prefixed settings (accessControlAllowOrigin,accessControlAllowMethods,accessControlAllowMethodsByRoute,accessControlAllowCredentials,accessControlAllowHeaders) plusallowCorsRequests, matchingvendor/wheels/events/init/security.cfmlines 17–22. Closed. -
The implementation claims in the comparison table and precedence section (verified against
EventMethods.cfc,security.cfm,Cors.cfcin prior rounds) remain accurate.
Commits
Both commits conform to commitlint.config.js and carry valid DCO sign-offs:
f9ccbc3c0—docs(web/guides): clarify allowCorsRequests still works in 4.0 and middleware precedence— 88 chars,Signed-off-by: claude[bot]present. The prior 115-char violation is resolved.6409c0027—docs(web/guides): address Reviewer A/B consensus findings (round 1)— 67 chars,Signed-off-by: claude[bot]present.
Tests
Docs-only PR — no test additions required or expected.
Security
No concerns. The PR documents existing framework behavior; no code is changed.
Wheels Bot — Reviewer B (round 3)A's round-3 review is accurate and complete. All prior findings are resolved. The branch now contains exactly two commitlint-compliant, DCO-signed commits, CI's "Validate Commit Messages" check passed on the current head, and the docs content is correct. The only observation worth noting is a recurring pattern: A submitted COMMENTED rather than APPROVED for a PR with no outstanding findings, which understates readiness. Converging on approve. SycophancyNone detected. A's positive closure on both prior findings is backed by explicit evidence:
False positivesNone. All of A's claims are accurate:
Missed issuesNone. This is a docs-only PR touching One marginal observation: A did not explicitly confirm that the Verdict alignmentA submitted COMMENTED for a PR with no outstanding findings and all CI green. APPROVED is the appropriate verdict when a review cycle concludes with all issues resolved. This is the inverse of the rounds 1–2 pattern (where COMMENTED understated a blocking issue); here it understates readiness. The finding is not incorrect — it is simply incomplete: a COMMENTED verdict leaves the PR without an approving review, which may block merge depending on branch protection rules. ConvergenceAligned on approve. All prior findings are resolved:
The PR is ready to merge. No further bot action is required. |
Second develop catch-up — five more PRs landed on develop after the first merge (40d70f3): #2637, #2645, #2639, #2638, #2642. Re-resolves the same three documentation conflicts: - CHANGELOG.md: keep develop's #2628 and #2629 Fixed entries; move this PR's #2626 entry to the end of the Fixed list. - v4-0-0/upgrading/3x-to-4x.mdx: keep develop's new #2631 reloadPassword Common-issues bullet; this PR's #2626 static-asset bullet stays at the end. - v4-0-1-snapshot/upgrading/3x-to-4x.mdx: same resolution. cli/lucli/Module.cfc auto-merged again — no manual fixes needed. Signed-off-by: Peter Amiri <peter@alurium.com>
Summary
The 3.x → 4.0 upgrade guide only described the
wheels.middleware.Corspath for CORS configuration, leaving apps that use the globalset(allowCorsRequests=true)setting with no guidance. This PR adds a new sub-section directly under the CORS breaking change (section 1) in both thev4-0-0andv4-0-1-snapshotupgrade guides clarifying:set(allowCorsRequests=true)and the fullaccessControlAllow*settings family are still honored in 4.0 — no migration required.Access-Control-Max-Age).abortinonRequestStartbefore the middleware pipeline runs); the middleware wins all other requests (becausecfheadercalls in the middleware overwrite what the global setting emitted).Fixes #2633
Files changed
web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx— new#### CORS — two paths in 4.0sub-section after the existing breaking change contentweb/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx— same sub-section (snapshot copy)CHANGELOG.md— one-line[Unreleased]documentation entryScreenshots needed
None — this is a prose + table + code-only doc section.