fix: sweep accumulated reviewer nits from the 2026-06 campaign - #2992
Conversation
Behavior fixes: LCase word-form operators in $evaluateLogicalExpression (uppercase EQ threw on Adobe CF); /wheels/cli gate reads the reload password from the form scope only (query-string password satisfied the gate while landing in access logs); dbRollback counts applied migrations by tracked status, not the version<=current heuristic (shared-dev-DB skew); NPE guard for null getErrorStream() in the three CLI HTTP bridge helpers; migrator column cache keys verbatim (no case folding); $getForeignKeys throws on missing adapter instead of emitting unquoted SQL; real \d escapes in $convertToString's dead ISO branch. Plus stale-docblock updates (#2903 references, renderWith/ onlyProvides enforcement notes, debug-panel guide note, .ai finally- loop note, migrator CLAUDE.md cache docs) and spec backfills (waitForText timeout, $get without request.wheels, typed-column outlier defaults, conditional loadRoutesSpec restore, uppercase-EQ conditions, dbDrop/dbRestore stub note). Fixes #2977 Signed-off-by: Peter Amiri <peter@alurium.com>
![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: This is a seven-fix sweep against outstanding reviewer nits from the 2026-06 campaign. Every behavior change is correctly reasoned and the test coverage is solid. I have two minor cosmetic nits but nothing blocking. Verdict: comment.
Correctness
All seven runtime fixes check out:
$evaluateLogicalExpression — LCase for word-form operators (vendor/wheels/model/validations.cfc:969)
The call path for plain numeric conditions like "1 EQ 1" falls through $evaluateConditionString → $evaluateLogicalExpression (not through $splitConditionOnOperator, which already applied LCase). Passing LCase(local.tokens[2]) before hitting the case-sensitive switch in $resolveOperator is the correct fix.
Regex fix in $convertToString ISO branch (vendor/wheels/Global.cfc:2887–2889)
In CFML, \\d in a string literal is two-characters \\d, so the regex engine saw \\d{4} (escaped-backslash + literal d) — the branch was silently dead since it was introduced. Likewise the replacement "\\1-\\2-\\3" produced literal text \1-\2-\3 instead of back-references. The single-backslash forms "\d" and "\1" are the correct CFML runtime values. Fix is accurate.
CLI gate reads form scope only (vendor/wheels/public/views/cli.cfm:23)
request.wheels.params merges URL + form, so ?password=... on a GET was satisfying the mutation gate and landing in access logs. makeBridgePost already sends the password form-encoded (Content-Type: application/x-www-form-urlencoded), so form.password is the right read site.
dbRollback status-based counting (vendor/wheels/public/views/cli.cfm:332)
Switching from version <= data.currentVersion to status == "migrated" correctly handles peer-applied orphan versions on shared dev DBs; matches the equivalent fix already in dbStatus.
getErrorStream() null guard in Module.cfc (cli/lucli/Module.cfc:6346, 6385, 6421)
HttpURLConnection.getErrorStream() per Javadoc returns null on a bodiless error response. The guard is correct. Minor nit below.
Column cache key — verbatim table name (vendor/wheels/migrator/Base.cfc:218)
Removing LCase() from the key prevents Authors and authors sharing a cache slot on case-sensitive engines. Correct.
$getForeignKeys throws MissingAdapter (vendor/wheels/migrator/Base.cfc:115–120)
Fail-loud is the right call; a missing adapter is a broken instantiation, not a recoverable edge case. Better than silently interpolating an unquoted table name into SQL.
Conventions
##2977 in a // comment (vendor/wheels/tests/specs/model/validationsSpec.cfc:180)
// used to hit the case-sensitive switch in $resolveOperator on Adobe CF and throw (##2977).// comments are not CFML-parsed, so the # doesn't need escaping — the source text reads as ##2977 rather than #2977. This has zero runtime impact and won't crash the suite, but any reader following the link will see a double-hash in their source. All other #2977 references in the same PR use a single # correctly.
Suggested: throw (#2977).
Cross-engine
No new cross-engine issues introduced. The regex fix, the LCase fix, and the null guard all improve compatibility with Adobe CF. The MissingAdapter throw uses bare Throw() (no cfthrow tag), which is fine across all supported engines.
Tests
Coverage is good:
validationsSpec.cfc— uppercaseEQfor bothconditionandunlesspaths.getSettingRequestScopeSpec.cfc— correctly saves and restoresrequest.wheelsinfinally; thehad = falsepath (no priorrequest.wheels) doesn't re-add the key. Clean isolation.loadRoutesSpec.cfc— the_had*flags correctly avoid leaving spurious empty keys inapplication.wheelswhen the spec ran before those caches were populated.typedColumnDefaultsSpec.cfc—createTable()returns aTableDefinitionbuilder;t.create()is never called, so no DDL is executed. Pure in-memory assertion ont.columns.BrowserIntegrationSpec.cfc— pins the third$waitOptionscall site alongside the two already covered.CliEndpointHardeningSpec.cfc— the stub comment explaining whydbDrop/dbRestoreare in the read-only list is a welcome guard against a future "promote to mutating but forget the gate check" mistake.
Docs
.ai/wheels/cross-engine-compatibility.md— bothforloop forms documented.vendor/wheels/migrator/CLAUDE.md— both request-scoped caches documented with their key semantics.vendor/wheels/controller/provides.cfcandrendering.cfc—onlyProvides()enforcement note is accurate.vendor/wheels/Public.cfc— "outside development" correction to the$blockInProductiondocblock is accurate per the #2903 change.web/sites/guides/src/content/docs/v4-0-0/digging-deeper/debug-panel.mdx— the:::noteaboutenablePublicComponent=trueoutside development returning 404s is correct and saves future confused reports.CHANGELOG.md— entry present under[Unreleased].
Minor nit — conn.disconnect() on early return (pre-existing pattern)
cli/lucli/Module.cfc:6347, 6386, 6422
if (isNull(inputStream)) {
return "";
}On the early-return path the HttpURLConnection is not disconnected; the JVM cleans it up on GC. The non-null path also omits an explicit disconnect(), so this is a pre-existing pattern being extended consistently, not a regression. Worth a follow-up issue if connection pressure becomes observable, but not a blocker here.
Commits
Single commit fix: sweep accumulated reviewer nits from the 2026-06 campaign (60-char header). Valid fix type, no scope, subject is sentence-case and non-empty. Conforms to commitlint.config.js.
Wheels Bot — Reviewer B (round 1)A's review is clean. All seven behavior-fix claims checked out against the diff, no false positives, no sycophancy, no missed issues of substance. The two nits A raised are both accurate and both genuinely trivial. Converging as approve. SycophancyNone detected. A gave a "comment" (not "approve") verdict backed by line-level reasoning for each fix. False positivesNone detected. Verified each claim against the diff:
Missed issuesNone detected.
Verdict alignmentA's "comment" verdict is consistent with the findings — two cosmetic nits that are non-blocking, no runtime or safety concerns. ConvergenceAligned with A. The |
Eliminates the [Unreleased]-anchor merge conflicts across campaign PRs; fragments are assembled into CHANGELOG.md at release promotion. Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR is a well-executed seven-fix sweep of outstanding reviewer nits, now with a follow-on `chore(docs)` commit that migrates the changelog entry to a `changelog.d/` fragment instead of a direct `[Unreleased]` edit. All runtime fixes are correct and test coverage is solid. The only actionable finding is that the `changelog.d/` pattern introduced by the second commit has no assembly tooling yet, so the fragment risks being silently orphaned when 4.0.4 is cut. Verdict: comment.
Correctness
All seven behavioral fixes hold up:
`$evaluateLogicalExpression` LCase (`vendor/wheels/model/validations.cfc:969`)
Word-form operators arrive unprocessed from the tokenizer; symbolic operators go through `$normalizeConditionOperators` first. Applying `LCase()` before the `switch` in `$resolveOperator` is the correct minimal fix.
Regex escapes in `$convertToString` ISO branch (`vendor/wheels/Global.cfc:2884–2888`)
`"\\d"` in a CFML string literal is two characters — escaped backslash + `d` — so the regex engine was matching a literal backslash, never a digit. The branch was silently dead since introduction. Single-backslash `"\d"` and `"\1"` are the correct CFML runtime forms and match the already-repaired slash-format branch below.
CLI gate reads `form` scope only (`vendor/wheels/public/views/cli.cfm:20`)
`request.wheels.params` merges URL + form; a `?password=...` query string satisfied the gate while writing the credential to access logs. `makeBridgePost` already sends it form-encoded, so `form.password` is correct.
`dbRollback` counts by `status == "migrated"` (`vendor/wheels/public/views/cli.cfm:332`)
The prior `version <= currentVersion` heuristic miscounted on shared dev databases where peer-applied orphan versions sit above the local file set. Status-based counting is the same P3 fix `dbStatus` received in #2947.
`getErrorStream()` null guard (`cli/lucli/Module.cfc:6346, 6385, 6421`)
`HttpURLConnection.getErrorStream()` returns Java `null` on a bodiless 4xx/5xx per Javadoc. Passing `null` to `Scanner.init()` NPEs; the guard returns an empty string consistently across all three HTTP bridge helpers.
Column cache key uses verbatim table name (`vendor/wheels/migrator/Base.cfc:218`)
Removing `LCase()` prevents `Authors` and `authors` sharing a cache slot on case-sensitive databases. Correct.
`$getForeignKeys` throws `Wheels.Migrator.MissingAdapter` (`vendor/wheels/migrator/Base.cfc:115–120`)
A missing adapter is a broken instantiation, not a recoverable edge case. The unquoted-table-name interpolation it replaces was a latent SQL injection vector on that code path.
Conventions
`##2977` in a `//` comment (`vendor/wheels/tests/specs/model/validationsSpec.cfc:183`)
// used to hit the case-sensitive switch in $resolveOperator on Adobe CF and throw (##2977).`//` comments are not CFML-parsed so this has zero runtime impact. However, the issue reference reads as `##2977` in source rather than `#2977`. This was flagged in the prior review at commit `0bf1d3ea6` and is still present at this head. Suggested: `throw (#2977)`.
Docs
`changelog.d/reviewer-nit-sweep.fixed.md` — no assembly tooling (`changelog.d/reviewer-nit-sweep.fixed.md`)
The second commit (`18aa5eefd`) moves the `[Unreleased]` CHANGELOG entry to a new `changelog.d/` fragment directory. The commit message asserts "fragments are assembled into CHANGELOG.md at release promotion," but there is currently no tooling to perform that assembly: no Makefile target, no GitHub Actions step, no towncrier config, and `changelog.d/` has no pre-existing precedent files in this repo (`ls changelog.d/` yields only this fragment).
If a 4.0.4 release is cut before an assembly step is added, this fragment will be silently orphaned and the user-facing changelog will miss all seven behavior fixes.
Suggested paths: (a) add a minimal `scripts/assemble-changelog.sh` (or Makefile target) that collects `changelog.d/*.md` into the `[Unreleased]` section and document it in `CONTRIBUTING.md` before other PRs adopt the pattern; or (b) revert to the direct `[Unreleased]` edit that was present in commit `0bf1d3ea6` until the fragment tooling is ready. The merge-conflict-avoidance motivation is valid — it just needs the assembly step to land before the first release that uses it.
Tests
Coverage is appropriate:
- `validationsSpec.cfc` — uppercase-EQ for both `condition` and `unless` paths
- `getSettingRequestScopeSpec.cfc` — absent-`request.wheels` path; `finally` block uses `if`, not a `for` loop, so the Lucee 7 finally-loop caveat (CLAUDE.md invariant #12) does not apply
- `loadRoutesSpec.cfc` — `_had*` flags prevent spurious empty keys on restore when caches were not populated before the spec ran
- `typedColumnDefaultsSpec.cfc` — `t.create()` is never called so no DDL executes; pure builder-state assertions on `t.columns`
- `BrowserIntegrationSpec.cfc` — third `$waitOptions` call site pinned alongside the two already covered
- `CliEndpointHardeningSpec.cfc` — stub comment guards against a future "promote to mutating without updating the gate check" mistake
Commits
`fix: sweep accumulated reviewer nits from the 2026-06 campaign` — valid `fix` type, 56-char header, sentence-case, DCO signed. ✓
`chore(docs): move changelog entry to changelog.d fragment` — valid `chore` type, 57-char header, sentence-case, DCO signed. ✓
Wheels Bot — Reviewer B (round 1)A's correctness analysis on all seven behavioral fixes is accurate and well-supported against the diff. One missed issue and one verdict disagreement prevent convergence. SycophancyNone detected. A provided evidence for each claim and chose False positivesNone detected. Spot-checked the three most technical claims:
Missed issuesNo regression test for the form-scope security fix ( The change from Verdict alignmentA's findings support ConvergenceNot aligned. A needs to address two things in its next response: (1) whether the missing form-scope gate regression test is an acceptable gap or should be flagged as a |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique is well-founded on both counts. I concede on the verdict and on the missing form-scope regression test. The one point I'm holding is the cosmetic `##2977` nit, which B neither challenged nor endorsed. Updated position: request-changes with two required items.
Conceded points
-
Verdict upgrade (comment → request-changes). B is correct that my own characterization — "the fragment risks being silently orphaned when 4.0.4 is cut" — describes a concrete process failure, not a theoretical one. Choosing `comment` is inconsistent with that language: `comment` doesn't trigger `bot-address-review.yml`, so the changelog orphan concern would stay open with no resolution pressure. The fix path I outlined in the initial review is clear and actionable — either add minimal assembly tooling for `changelog.d/` or revert `changelog.d/reviewer-nit-sweep.fixed.md` to the direct `[Unreleased]` edit that was present at `0bf1d3ea6`. That choice should be made before merge.
-
Missing form-scope gate regression test. B is correct. I reviewed `CliEndpointHardeningSpec.cfc` and described the stub-comment addition as a useful guard, but I missed that there is no test verifying the URL-param bypass is actually closed. The only change to `CliEndpointHardeningSpec.cfc` in this PR is a `// dbDrop and dbRestore are currently STUBS...` comment at line 82; no new test asserts that a request with a query-string password and no form body is now rejected. For a fix explicitly described as closing a SEC-4 credential leak, a regression test is the minimum bar. `cli.cfm` is a view file, so the test would need to sit at the HTTP/browser layer — for example, a `BrowserTest` spec or HTTP-level CFC that sends the password as a query param only and asserts a non-200 response. Until that test exists, a refactor reverting to `request.wheels.params.password` would pass CI silently.
Defended points
- `##2977` cosmetic nit (`vendor/wheels/tests/specs/model/validationsSpec.cfc:183`). B did not challenge this finding. The `//` line reads `(##2977)` in source; `//` comments are not CFML-parsed so there is no runtime impact, but any reader following the reference from their editor sees a double-hash rather than the real issue number. One-character fix; still accurate.
Updated verdict
Request-changes — two items required: (1) resolve the `changelog.d/` pattern (add assembly tooling or revert to the direct `[Unreleased]` edit); (2) add an HTTP/browser-layer regression test for the URL-param password bypass. The `##2977` nit is non-blocking but worth fixing in the same pass.
1 participant
Summary
One sweep PR for the reviewer nits tracked in #2977, driven by a full re-read of the review threads of #2922 / #2933 / #2934 / #2937 / #2947 with every item re-verified against develop. Typed
fix(notchore) because several items change runtime behavior.Behavior fixes
$evaluateLogicalExpressionlowercases word-form operators —condition="1 EQ 0"threw on Adobe CF's case-sensitive switch (symbolic operators were already pre-lowercased; word-form ones arrived raw).request.wheels.paramsmerges URL + form, so?password=...satisfied the gate while logging the password in access logs, contradicting the SEC-4 design. (makeBridgePostalready sends it form-encoded, verified.)status == "migrated"instead ofversion <= currentVersion— on shared dev DBs, peer-applied versions madesteps=Nroll back fewer real migrations. Same P3 fix dbStatus got in fix(dispatch): require POST+password for /wheels/cli destructive ops, contain dbDump #2947 itself.getErrorStream()returns Java null on bodiless 4xx/5xx;Scanner.init(null)NPE'd with a useless "null" message. Guarded uniformly inmakeBridgePost/makeHttpRequest/makeHttpPost.Authors/authorson case-sensitive DBs);$getForeignKeys()throwsWheels.Migrator.MissingAdapterinstead of silently interpolating an unquoted table name.$convertToString's catch-fallback used\\d(literal backslash + d — dead regex); now matches the already-fixed slash branch.Docs / comments
#2903's two remaining stale denylist references (
$loadRegistryPackagesdocblock,InvokeMethodSpeccomment); debug-panel.mdx note thatenablePublicComponent=trueoutside development still 404s every Tools link;renderWith()/onlyProvides()docblocks document the #2901 enforcement + html fallback;.aifinally-loop section notes bothforforms are affected; migrator CLAUDE.md documents the two #2937 caches;CliEndpointHardeningSpecexplains why dbDrop/dbRestore are classified read-only (stubs).Spec backfills
Uppercase-EQ conditions (2),
waitForText()timeout surface,$get()no-throw withoutrequest.wheels(DC16 gap), typed-column outlier defaults (float), conditionalloadRoutesSpecafterAll restore (staticRoutes + namedRoutePositions).Deliberately dropped
public/Application.cfccomment condensation — the reviewer's own verdict was the longer form is justified.Fixes #2977
Test Plan
🤖 Generated with Claude Code