[bpamiri]

Replaces #2806 (auto-closed when its base branch `claude/upgrade-advisory-tier-pr3` was deleted on PR3 merge).

Summary

Adds two advisory entries to the `wheels upgrade check` scanner, populating the "Recommended Improvements" section added in #2805 with the first concrete opt-in convention from the #2781 work.

Advisories added

1. `t.references()` opt-in suggestion

Fires when any migration file under `app/migrator/migrations/` uses `t.references(`. Suggests opting into `_id` naming via `set(useUnderscoreReferenceColumns=true)` to match Wheels model `belongsTo` defaults. Explicitly notes that already-applied migrations are unaffected — only new migrations adopt the new suffix.

2. Mixed-convention warning

Fires when `config/` contains `set(useUnderscoreReferenceColumns=true)`. Reminds the user that if this flag was flipped after some migrations were already applied, the database still has `id` columns alongside any new `_id` columns. Informational — read once, ignore if not applicable.

Both use `severity: "advisory"` — surface in the cyan "Recommended Improvements" section, run regardless of major-version bump, never gate CI exit code.

Tests

`cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc` — 5 source-level specs verifying both advisory entries are present and reference the correct patterns, paths, and fix-message phrasing.

CLI suite: 591 pass / 5 fail (same 5 pre-existing failures unrelated to upgrade).

Scope (4 of 4 PRs from the #2781 audit)

• ✅ #2802 — t.references() at TableDefinition (closes Migrator: t.references() API quirks — 'referenceNames' (not 'columnNames'), and produces '<x>id' without underscore #2781)
• ✅ #2804 — Migration.cfc command consistency
• ✅ #2805 — wheels upgrade scanner advisory tier
• 🟢 This PR — specific advisory entries (closes the loop)

Test plan

• `bash tools/test-cli-local.sh` — 5 new advisory specs pass; pre-existing failures unchanged
• Cross-engine matrix (CI)

[wheels-bot]

Wheels Bot — Reviewer A\n\nTL;DR: This PR adds two advisory entries to the wheels upgrade check scanner -- one prompting users to opt into <name>_id column naming, and one reminding users who have already opted in to audit legacy migrations. The intent is sound, but two correctness issues need fixing before merge: advisory #1 can fire a false positive on apps that have already opted in, and neither advisory strips CFML comments before grepping, violating Anti-Pattern #14. Requesting changes.\n\n---\n\n### Correctness\n\nAdvisory #1 fires even when the flag is already set\n\ncli/lucli/Module.cfc lines 3907-3915\n\nThe inline comment at line 3903 says "when the underscore flag is not yet set", but the check struct has no such guard -- it fires whenever t.references( appears in any migration file, regardless of whether useUnderscoreReferenceColumns=true is already present in config/.\n\nAn app that has already opted in will see both advisories simultaneously: advisory #1 says "opt into useUnderscoreReferenceColumns=true" (contradicting reality) while advisory #2 says "you have opted in -- check legacy migrations" (correct). The fix message in advisory #1 is actively misleading for this user.\n\nThe simplest fix: move advisory #1's arrayAppend inside an explicit guard that pre-scans config/settings.cfm for the flag before appending. The check struct format does not support multi-condition AND logic natively, but a local pre-check variable is straightforward:\n\ncfml\nvar flagAlreadySet = false;\nvar settingsFile = variables.projectRoot & "/config/settings.cfm";\nif (fileExists(settingsFile)) {\n flagAlreadySet = len(reFindNoCase(\n "useUnderscoreReferenceColumns\s*=\s*true",\n stripCfmlComments(fileRead(settingsFile))\n ));\n}\nif (!flagAlreadySet) {\n arrayAppend(checks, { description: "...", severity: "advisory", ... });\n}\n\n\n---\n\nNo CFML comment stripping before the grep scan (Anti-Pattern #14)\n\ncli/lucli/Module.cfc lines 3994-3998\n\ncfml\nvar content = fileRead(filePath);\nvar lines = listToArray(content, chr(10), true);\nfor (var lineNum = 1; lineNum <= arrayLen(lines); lineNum++) {\n if (reFindNoCase(check.pattern, lines[lineNum])) {\n\n\nRaw file content is scanned without stripping comments first. A migration containing // t.references(columnNames="user"); will trigger advisory #1, and a config/settings.cfm containing // set(useUnderscoreReferenceColumns=true); triggers advisory #2.\n\nCLAUDE.md Anti-Pattern #14: "Any validator, analyzer, scanner, or upgrade-check that does substring-matching over CFML source must strip line comments, block comments, AND tag comments first." The helper is already on the same CFC at line 6174 -- private string function stripCfmlComments(required string source). The fix is one line at line 3995:\n\ncfml\nvar content = stripCfmlComments(fileRead(filePath));\n\n\nMulti-line block comments collapse on strip and may shift subsequent line numbers in match output -- the same tradeoff the existing callers at lines 970 and 5532 already accept.\n\n---\n\n### Tests\n\ncli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc\n\nThe source-level approach is consistent with UpgradeCommandSpec and the TestBox limitation is documented. However, the false-positive scenario is not covered at any level -- there is no spec confirming advisory #1 is suppressed when the flag is already set. Once the correctness fix lands, a source-level assertion on the guard variable name would confirm the fix survives future edits.\n\n---\n\n### Docs\n\nNo CHANGELOG.md [Unreleased] entry for this PR. The advisory-tier scaffolding (#2805) has an entry under Changed, but the two new user-visible advisory checks warrant an Added bullet -- e.g., "Two advisory checks in wheels upgrade check: t.references() opt-in suggestion and mixed-convention migration warning (closes #2781)".\n\n---\n\n### Commits\n\nfeat(cli): underscore-references opt-in advisories in wheels upgrade check -- type feat, scope cli, subject conforms to commitlint.config.js. No issues.\n\n

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

Reviewer A's review is a bot workflow artifact — the body is the literal string "PLACEHOLDER_TO_REPLACE" with no line comments. The CHANGES_REQUESTED verdict has zero evidentiary basis. Because there is no actual review content to critique, I ran an independent scan of the diff to determine the correct verdict.

Sycophancy

None detected. (No content to be sycophantic with.)

False positives

Not applicable — A made no claims.

Missed issues

Bot workflow bug (blocking convergence): Review ID 4351713509 was submitted with state: CHANGES_REQUESTED but body: "PLACEHOLDER_TO_REPLACE" and no line comments. This is a defect in the review-a workflow. The verdict is unactionable.

Advisory noise (minor, non-blocking): The second advisory (useUnderscoreReferenceColumns=true grep on config/) fires for every app generated by wheels new — they all ship with that flag set — even when there are no legacy migrations to worry about. The fix message is written conditionally ("If migrations were applied before this flag was set...") so it reads correctly, and severity is advisory which never gates CI. Acceptable noise for a first iteration, but worth noting for a future tightening pass.

No other issues found. Diff is two struct-literal entries appended to an existing checks array, plus a new source-level spec file that follows the established UpgradeCommandSpec.cfc pattern. Commit message and test structure are clean.

Verdict alignment

A's CHANGES_REQUESTED is inconsistent with the diff, which shows no blocking issues. The verdict is unsupported by any finding.

Convergence

I am aligning on approve. A's CHANGES_REQUESTED is a broken artifact with no content; continuing the loop would not produce a meaningful response from A. My independent scan finds no issues warranting changes. The advisory-noise note above is informational and does not block merge.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: Round-2 review of the fix commit (d52f894). Both correctness issues from round-1 were addressed in principle, but the flag-guard fix introduced a new correctness bug: len(reFindNoCase(...)) always evaluates truthy in CFML, so advisory #1 is effectively dead code for every app whose config/settings.cfm exists. One-line fix required.

---

Correctness

len(reFindNoCase(...)) is always truthy — advisory #1 is never appended

cli/lucli/Module.cfc lines 3918–3921 (head d52f894):

underscoreFlagAlreadySet = len(reFindNoCase(
    "useUnderscoreReferenceColumns\s*=\s*true",
    stripCfmlComments(fileRead(settingsFile))
));

reFindNoCase(pattern, string) — the two-argument form — returns an integer: 0 if not found, or the 1-based start position if found. In CFML, len() coerces its argument to a string before measuring:

reFindNoCase result	len() call	return value	boolean
0 (not found)	len("0")	1	truthy
25 (found)	len("25")	2	truthy

Because underscoreFlagAlreadySet is always >= 1 when config/settings.cfm exists, if (!underscoreFlagAlreadySet) is always false, and advisory #1 is never appended to the checks array. Every Wheels app has a config/settings.cfm, so this suppresses the opt-in suggestion for the exact audience it is meant to reach.

Note: round-1 review suggested len(reFindNoCase(...)) in its example snippet — the author implemented it faithfully, but the suggestion itself was incorrect. Apologies for the noise.

Fix — drop the len() wrapper and use a direct comparison:

underscoreFlagAlreadySet = reFindNoCase(
    "useUnderscoreReferenceColumns\s*=\s*true",
    stripCfmlComments(fileRead(settingsFile))
) > 0;

reFindNoCase(...) > 0 is false when not found (0 > 0) and true when found (pos > 0). No coercion hazard.

---

Tests

The source-level assertions in UpgradeAdvisorySpec.cfc (the new "false-positive guards" describe block) verify that the guard variable name and the if (!underscoreFlagAlreadySet) string exist in the source. They pass even with the len() wrapper because they only check string presence, not runtime behavior. After the one-line fix above, they continue to pass and remain useful as structural guardrails — no new test file needed.

---

Everything else looks good

• Comment-stripping in the grep loop (stripCfmlComments(fileRead(filePath))): correctly applied.
• Pre-check guard also strips comments on the settings-file read: correctly applied.
• Advisory #2 (mixed-convention warning): unconditional append is correct — it fires when the flag IS set.
• CHANGELOG [Unreleased] / Added entry: present and accurate.
• Commit messages: both conform to commitlint.config.js.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's round-2 review is accurate. The len(reFindNoCase(...)) bug is real, the fix is correct, and A's self-correction (taking responsibility for the round-1 suggestion that introduced it) is appropriate. I align on CHANGES_REQUESTED.

Sycophancy

None detected. A's "everything else looks good" is backed by a specific enumeration of items checked (comment-stripping, advisory 2, CHANGELOG, commit messages) — it is not a vacuous LGTM.

False positives

None. I verified the cited lines directly (Module.cfc:3918-3921 in the diff). The analysis is correct:

• reFindNoCase(pattern, string) (two-argument form) returns 0 (not found) or a 1-based integer position (found).
• len(0) in CFML coerces to len("0") = 1 (truthy); len(25) = len("25") = 2 (truthy).
• Therefore underscoreFlagAlreadySet is always >= 1 when config/settings.cfm exists, and if (!underscoreFlagAlreadySet) is always false.

Every other reFindNoCase call in the file uses it directly in an if condition (where 0 is falsy), confirming this len() wrapper is an anomaly, not an established pattern. A's fix (reFindNoCase(...) > 0) is correct.

Missed issues

None. The one-liner fix resolves the only runtime bug. A's characterization of the test suite as source-level-only (structural guardrails, not runtime coverage) is accurate and does not warrant new tests — the existing specs remain valid structural guardrails after the fix.

Verdict alignment

CHANGES_REQUESTED is consistent with A's findings. There is a concrete runtime bug where advisory 1 is unconditionally suppressed, which warrants blocking the merge.

Convergence

Aligned. A found a real bug; the fix is specific and unambiguous. No disputes on any finding. Joint recommendation: apply the one-line fix (reFindNoCase(...) > 0 replacing len(reFindNoCase(...))) and re-merge.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: Round-3 (head a116586). The two correctness issues flagged in rounds 1 and 2 are both resolved: the false-positive guard now uses reFindNoCase(...) > 0 (not the always-truthy len() wrapper), and the grep loop strips CFML comments before pattern matching. One minor asymmetry remains worth noting, but it is an edge case that does not warrant blocking merge. Approving.

---

Correctness

All prior issues resolved

cli/lucli/Module.cfc lines 3915–3937:

var underscoreFlagAlreadySet = false;
var settingsFile = variables.projectRoot & "/config/settings.cfm";
if (fileExists(settingsFile)) {
    underscoreFlagAlreadySet = reFindNoCase(
        "useUnderscoreReferenceColumns\s*=\s*true",
        stripCfmlComments(fileRead(settingsFile))
    ) > 0;
}
if (!underscoreFlagAlreadySet) {
    arrayAppend(checks, { ... advisory #1 ... });
}

• > 0 comparison is correct: reFindNoCase returns 0 on no-match and a 1-based position on match. Round-2's len() coercion hazard is gone.
• Comment-stripping on the settings-file pre-check: correct — a commented-out // set(useUnderscoreReferenceColumns=true); no longer satisfies the guard.
• Comment-stripping in the shared grep loop (line 4023): correct — applies to every check struct, not just these two advisories.

Minor nit — pre-check scope is narrower than advisory #2's scan scope

The guard reads only config/settings.cfm, while advisory #2 uses scanDir: "config" with extensions: "cfm,cfc" — scanning every config file recursively. An app that sets the flag in config/environment.cfm (unusual but valid) would have underscoreFlagAlreadySet = false, causing advisory #1 to fire alongside advisory #2 — the exact contradiction the guard is meant to prevent.

Not a blocking issue because config/settings.cfm is the canonical location per framework convention and the wheels new template. Worth a follow-up issue or a later scan-all-of-config extension if this surfaces as user noise.

---

Tests

cli/lucli/tests/specs/commands/UpgradeAdvisorySpec.cfc — 5 source-level specs, consistent with the established UpgradeCommandSpec pattern (Module.cfc cannot be instantiated under TestBox). The notToInclude("len(reFindNoCase") assertion at line 83 specifically guards the round-2 regression — a useful structural guardrail that would catch an accidental re-introduction. ##14 in the CFScript string literal at line 61 is the correct escape for a literal # inside double-quoted string context.

---

Commits

• feat(cli): underscore-references opt-in advisories in wheels upgrade check — conforms.
• fix(cli): address Reviewer A round-1 — advisory false-positive + comment-stripping — conforms.
• fix(cli): drop dead len() wrapper from underscore-flag guard — conforms. Clear "why" in the subject.

---

Docs

CHANGELOG [Unreleased] / Added entry is present and accurate. No .ai/wheels/ doc update needed for a pure CLI scanner addition.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's round-3 review is accurate and well-evidenced. All prior correctness issues are confirmed resolved; the minor scope-asymmetry nit is correctly classified as non-blocking. Aligning on approve.

Sycophancy

None detected. A's approval is backed by specific code references and a point-by-point accounting of the prior findings.

False positives

None detected. I verified all cited lines against the diff:

• reFindNoCase(...) > 0 guard: correct. The diff shows ) > 0; at the end of the pre-check block.
• stripCfmlComments on the settings pre-check: confirmed — stripCfmlComments(fileRead(settingsFile)).
• stripCfmlComments in the shared grep loop: confirmed — var content = stripCfmlComments(fileRead(filePath));.
• ##14 CFML escape in spec line 61: correct — CFML double-quoted strings require ## for a literal #.

Missed issues

None. A correctly identifies the one remaining edge case (scope asymmetry: guard reads only config/settings.cfm; advisory #2 scans all of config/) and correctly marks it non-blocking given framework convention. The advisory-noise issue (advisory #2 firing for every wheels new-generated app) was previously acknowledged in round-1 B and is consistent with severity: advisory semantics. No cross-engine issues apply to pure CLI-side file-scanning code.

Verdict alignment

APPROVED is consistent with A's findings. No blocking issues remain; the scope-asymmetry nit is accurately characterized as a follow-up candidate, not a merge gate.

Convergence

Aligned on approve. All prior blocking issues are resolved; A's single remaining nit is non-blocking and correctly scoped. Joint recommendation: merge.

[wheels-bot]

Wheels Bot — Address Review (round 1)

The consensus finding from Reviewer B's converged-changes marker at d52f89418 — replace the always-truthy len(reFindNoCase(...)) wrapper with reFindNoCase(...) > 0 — is already resolved at the current head SHA (a116586f4).

• cli/lucli/Module.cfc:3922-3925 uses reFindNoCase(...) > 0 (no len() wrapper), with an inline comment documenting the coercion hazard.
• Commit a116586f4 fix(cli): drop dead len() wrapper from underscore-flag guard lands the exact one-line fix the reviewers aligned on.
• Reviewer A's round-3 review at a116586f4 is APPROVED — both round-1 and round-2 correctness issues are resolved.

No new commits required. Skipped: A's round-3 minor nit (pre-check scans only config/settings.cfm while advisory #2 scans config/ recursively) — A flagged it explicitly as non-blocking follow-up, not part of the convergence-changes consensus.