[wheels-bot]

Summary

The advisory-1 suppression guard in cli/lucli/Module.cfc::runUpgradeCheck() previously read only config/settings.cfm, while advisory #2 below it scans all of config/ recursively. If a user set useUnderscoreReferenceColumns=true in an environment override file (e.g. config/production/settings.cfm), the guard missed it, advisory #1 fired ("opt into the flag!"), AND advisory #2 also fired ("the flag is set!") — the contradiction the round-1 fix was meant to prevent. The pre-check now walks config/ recursively (matching advisory #2's scanDir: "config" / extensions: "cfm,cfc" scope), stripping CFML comments per file so commented-out flag lines cannot satisfy the guard (Anti-Pattern #14).

Related Issue

Fixes #2808.

Type of Change

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

Feature Completeness Checklist

• DCO sign-off -- commit carries Signed-off-by: trailer
• Tests -- cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc updated (source-level): replaced the "uses stripCfmlComments on the settings-file pre-check" assertion with two new assertions covering recursive config/ scanning + per-file comment-stripping. Failing-before / passing-after verified against Module.cfc via grep (the spec is mechanical text matching against the module source).
• Framework Docs -- handled by bot-update-docs.yml
• AI Reference Docs -- handled by bot-update-docs.yml
• CLAUDE.md -- handled by bot-update-docs.yml
• CHANGELOG.md -- entry added under [Unreleased] → Fixed
• Test runner passes -- spec assertions verified deterministically against post-fix source (tools/test-local.sh and tools/test-cli-local.sh cannot be executed in this environment: no wheels binary on PATH; CI will run them)

Test Plan

• cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc source-level assertions:
  • expect(moduleSource).toInclude("directoryExists(configDir)") — confirms the guard walks the directory
  • expect(moduleSource).toInclude("directoryList(configDir, true") — confirms recursive scan
  • expect(moduleSource).toInclude("stripCfmlComments(fileRead(configFile))") — confirms per-file comment-stripping
  • existing notToInclude("len(reFindNoCase") still satisfied (no len() anti-pattern reintroduced)
• Behavior matrix the fix covers:
  • config/settings.cfm has set(useUnderscoreReferenceColumns=true); → guard fires → advisory New master #1 suppressed (unchanged from before)
  • config/production/settings.cfm (or any other override path) has the flag → guard now fires → advisory New master #1 suppressed (new behavior, was broken)
  • config/settings.cfm has // set(useUnderscoreReferenceColumns=true); (commented out) → comment-stripped → guard does NOT fire → advisory New master #1 still emitted (unchanged)
  • No config/ directory present → guard short-circuits, advisory New master #1 emits as before

[wheels-bot]

Wheels Bot — No doc updates

Reviewed this PR's diff and found no docs that need updating (purely internal bug fix — the suppression guard's scan scope was corrected but no user-facing check description, output text, or public API changed; the advisory itself was introduced by #2807 and any docs gap for it belongs to that PR's documentation debt).

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This is a clean, focused bug fix that broadens the underscore-flag suppression guard from config/settings.cfm alone to all of config/ recursively, matching the scope of advisory #2. The logic is correct, the CHANGELOG entry is accurate, and the commit conforms to the project conventions. One stale test description is the only finding — --comment verdict.

Tests

cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc, line 52 — stale it() description

The PR correctly replaces the old single-file assertion with two new ones, but the existing test above them was not updated:

it("suppresses the opt-in advisory when the flag is already set in config/settings.cfm", () => {

After this fix the guard scans all of config/ recursively, so the description "in config/settings.cfm" is now misleading. The assertion body (toInclude("underscoreFlagAlreadySet") and toInclude("if (!underscoreFlagAlreadySet)")) is still valid — nothing will break — but a future reader learning from the spec will get the wrong mental model.

Suggested wording:

it("suppresses the opt-in advisory when the flag is already set anywhere in config/", () => {

Correctness

The new loop is logically sound:

• directoryList(configDir, true, "path", "*." & ext) returns full absolute paths when the third argument is "path" on both Lucee and Adobe CF — consistent with the downstream fileRead(configFile) calls.
• The two-pass approach (.cfm then .cfc) correctly mirrors advisory #2's extensions: "cfm,cfc" scope.
• Early exit via break on the first match is correct — once the flag is found there is no reason to continue.
• var found re-declared inside the for (var ext in ...) loop is function-scoped in CFML CFScript and re-assigns the same local slot each iteration, which is the intended behaviour.

Cross-engine

No concerns here. This code runs inside the wheels CLI binary (LuCLI / Lucee) rather than in the framework proper, so cross-engine incompatibilities that affect Adobe CF or BoxLang do not apply to this path. The directoryList, fileRead, and reFindNoCase BIFs are standard across all CFML engines regardless.

Commits

fix(cli): wheels upgrade underscore-flag pre-check scans all of config/ — correct type, valid scope, 71-char header, body explains the "why" well, Signed-off-by: trailer present. Conforms to commitlint.config.js.

Docs

CHANGELOG [Unreleased] → Fixed entry is present and accurate.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-grounded. The one finding, stale it() description at line 52, is verified against the post-patch file. No sycophancy, no false positives, no missed issues. A and I are aligned: the logic is correct, the only action needed is the description update A suggested.

Sycophancy

None detected. A's clean-fix framing is backed by specific evidence (correctness walk-through, cross-engine reasoning, commit + changelog checks) rather than bare assertion, and A did not approve despite having a finding.

False positives

None detected. I verified each of A's correctness claims against the diff:

• directoryList(configDir, true, "path", "*." & ext) returning full absolute paths on both Lucee and Adobe CF is correct; listInfo = "path" is standard CFML.
• var found re-declared inside the outer for loop is function-scoped in CFML (no block scope); harmless re-assignment each iteration. A's analysis is accurate.
• The break exits the configFiles loop; the two loops are sequential not nested, so the break correctly stops the search on first match.
• CLI runs inside LuCLI (Lucee-based runtime); A's cross-engine exemption is correct for this code path.

Missed issues

None detected. The stale test description A flagged at line 52 is the only loose end. The description says the flag is already set in config/settings.cfm but the guard now scans all of config/ recursively. The assertions inside remain valid and will not fail. A's suggested replacement is accurate: change to "anywhere in config/" instead of "in config/settings.cfm".

No other issues surfaced from a re-scan of the diff.

Verdict alignment

A's COMMENT verdict is consistent with the finding being non-blocking (stale description, not a logic error). Given A and B are now aligned that the description should be updated, the right outcome is converged-changes to apply the fix rather than leaving it as an informal suggestion.

Convergence

Aligned. The code is correct; the only change needed is updating the it() description at cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc line 52 from "in config/settings.cfm" to "anywhere in config/" (A's exact suggestion). Joint recommendation: apply that one-line fix.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied consensus finding from Reviewer A and Reviewer B's convergence (round 1 of the convergence loop):

• cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc:52 — updated the it() description from "…flag is already set in config/settings.cfm" to "…flag is already set anywhere in config/" so the spec label matches the broadened recursive scan scope shipped in c482150. Assertion bodies unchanged.

No findings were skipped — A's review surfaced exactly one finding (the stale description), B confirmed it without identifying any missed issues, and both reviewers converged on this single change.

Test runner note: tools/test-cli-local.sh is unavailable in this prompt's environment (sandbox restriction), but the change is a pure label-string edit — the assertion bodies (toInclude("underscoreFlagAlreadySet") and toInclude("if (!underscoreFlagAlreadySet)")) are untouched, so there is no logic to regress. CI will run the spec on the new SHA.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This is a two-commit stack. The core fix (c482150) broadens the underscore-flag suppression guard from a single-file read to a recursive config/ walk, eliminating the advisory contradiction first reported in #2808. The follow-up commit (8e41bbd) addresses the one finding from the previous round-1 review — the stale it() description in the spec. Both commits are correct, the tests are comprehensive, and the diff is clean. No further findings — would approve if not a self-review.

Correctness

The follow-up commit is a one-line label change that exactly matches what was requested in round 1:

// before
it("suppresses the opt-in advisory when the flag is already set in config/settings.cfm", () => {

// after
it("suppresses the opt-in advisory when the flag is already set anywhere in config/", () => {

The assertion bodies (toInclude("underscoreFlagAlreadySet") and toInclude("if (!underscoreFlagAlreadySet)")) are unchanged and still valid against the implementation.

The core logic in cli/lucli/Module.cfc remains correct as assessed in round 1:

• directoryList(configDir, true, "path", "*." & ext) — recursive, returns absolute paths, consistent with the downstream fileRead(configFile) call.
• Flat configFiles accumulator followed by a single scan loop with break on first match is sound; the break exits only the file-iteration loop, which is the only remaining loop at that point.
• underscoreFlagAlreadySet = false default with the guard wrapped in if (directoryExists(configDir)) correctly handles the no-config/ case.

One minor observation (not blocking): the advisory's fix message at line ~3949 still reads "in config/settings.cfm" while the guard now accepts the flag from any file in config/. Recommending the canonical location to users who need to add the flag is actually reasonable UX, so this is not a bug — but a future pass could soften it to "in config/settings.cfm or any environment override file" to avoid confusion.

Tests

cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc — all four assertions in the "false-positive guards" describe block remain coherent with the implementation:

• toInclude("underscoreFlagAlreadySet") / toInclude("if (!underscoreFlagAlreadySet)") — guard variable present.
• toInclude("directoryExists(configDir)") / toInclude("directoryList(configDir, true") — recursive walk confirmed.
• toInclude("stripCfmlComments(fileRead(configFile))") — per-file comment-stripping confirmed (distinct from the existing stripCfmlComments(fileRead(filePath)) assertion that covers the shared grep loop).
• notToInclude("len(reFindNoCase") — len()-wrapping anti-pattern absent.

Commits

• fix(cli): wheels upgrade underscore-flag pre-check scans all of config/ — 71-char header, Signed-off-by: present, body explains the why. Conforms to commitlint.config.js.
• fix(cli): address Reviewer A/B consensus findings (round 1) — 55-char header, Signed-off-by: present, body itemises the change with file + line. Conforms.

Cross-engine

No concerns. This code runs inside the wheels CLI binary (LuCLI / Lucee only) — Adobe CF and BoxLang compat rules from .ai/wheels/cross-engine-compatibility.md do not apply.

Docs

CHANGELOG [Unreleased] → Fixed entry is accurate and present.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate on every substantive point. One prose imprecision noted below, but it does not affect A's correctness assessment. No sycophancy, no false positives, no missed issues. Aligning on approve.

Sycophancy

None detected. A's "would approve if not a self-review" framing is backed by a concrete correctness walk-through, test-by-test verification, commit format checks, and an explicit cross-engine exemption justification — not bare assertion.

False positives

None detected. One prose inaccuracy worth noting: A describes the follow-up commit (8e41bbd) as "a one-line label change." The diff shows it also replaced the old it("uses stripCfmlComments on the settings-file pre-check...") block — which carried toInclude("stripCfmlComments(fileRead(settingsFile))") — with two new it() blocks using toInclude("stripCfmlComments(fileRead(configFile))"). The old assertion would have failed against the post-fix source (settingsFile variable no longer exists); the update was necessary, not cosmetic. However, A did list all four current assertions explicitly in the Tests section and verified each against the implementation, so the analysis is substantively correct even if the summary sentence was imprecise. This is not a false positive in A's findings.

Missed issues

None detected. Re-scan of the diff confirmed:

• The two-loop accumulator pattern in Module.cfc is sound: configFiles is built first, then scanned with an early break. The break exits only the scan loop; the two loops are sequential, not nested.
• var found re-declared inside the outer for loop is harmless in CFML (function scope, no block scope).
• Empty-configFiles edge case (both directoryList calls return nothing) correctly leaves underscoreFlagAlreadySet = false and emits advisory New master #1.
• All four test assertions map to actual strings in the post-fix source. The stripCfmlComments(fileRead(configFile)) assertion is distinct from the existing stripCfmlComments(fileRead(filePath)) assertion (the shared grep loop), so there is no cross-contamination between the two tests.
• A's observation about the advisory fix message still reading "in config/settings.cfm" is correctly characterized as non-blocking UX: recommending the canonical file to users who need to add the flag is reasonable, and the message is not part of the guard logic.

Verdict alignment

A's COMMENTED verdict with effective "would approve" is consistent with the findings: the one non-blocking observation (advisory fix-message wording) does not warrant blocking changes, and no logic errors were identified.

Convergence

Aligned on approve. Both commits are correct, the tests accurately cover the new behavior, the CHANGELOG entry is accurate, and the commits conform to commitlint.config.js. No changes are needed on this SHA.