feat(cli): non-blocking update check on wheels new

[bpamiri]

Summary

Closes the original ask from the BE-channel thread: when a user runs wheels new myapp, print a small hint at the end if a newer wheels release is available on their channel.

Constraints I made the service enforce:

• Never blocks wheels new — all errors swallow silently. Wrapped in a try/catch belt at the call site.
• 5s hard timeout on the HTTP call.
• 24h per-channel cache at $LUCLI_HOME/.update-check.json. One real network request per channel per day, not per wheels new.
• Channel routing via existing services/ReleaseChannel.cfc:
  • stable → wheels-dev/wheels
  • bleeding-edge → wheels-dev/wheels-snapshots
  • release-candidate / development → skip (RC users opted in explicitly; dev checkouts have no meaningful target)
• Format-bridging comparison: a user on a brew bottle built before the SemVer separator fix has 4.0.0-SNAPSHOT+1656. The latest published is 4.0.0-snapshot.1790. The $snapshotNumber extractor handles both forms and $isNewer correctly flags it as upgrade-worthy.

What lands

File	Purpose
cli/lucli/services/UpdateChecker.cfc	New service. Pure-function comparison + cached HTTP fetch.
cli/lucli/Module.cfc (~24 LOC)	Call site at the end of new(). Try/catch belt.
cli/lucli/tests/specs/services/UpdateCheckerSpec.cfc	Unit tests for comparison + skip paths.

What it looks like at runtime

When there's nothing newer:

Application created!
...
Next steps:
  cd myapp
  wheels start

When there's a newer build:

Application created!
...
Next steps:
  cd myapp
  wheels start

A newer wheels (bleeding-edge) is available: 4.0.0-snapshot.1790 (you have 4.0.0-snapshot.1789)
  Upgrade: brew upgrade wheels-be

Test plan

• Unit tests pass (pure-function paths)
• CI green on this PR
• Manual: wheels new test-app with a stale local install shows the hint
• Manual: wheels new test-app with a fresh install does NOT show the hint
• Manual: wheels new test-app with no network completes silently (no hang, no error)

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR adds a clean, non-blocking update-check hint to wheels new. The design constraints (5s timeout, 24h cache, channel-aware routing, silent failure) are all sound and correctly implemented. The core logic is correct across all code paths I checked. Three minor nits below — none are correctness blockers — so I'm leaving this as a comment rather than requesting changes.

---

Conventions

structKeyExists(variables, "verbose") guard is inconsistent and redundant

cli/lucli/Module.cfc, line 4197:

if (structKeyExists(variables, "verbose")) {
    try { verbose("Update check failed: " & e.message); } catch (any ignore) {}
}

The rest of the file calls verbose() directly without a guard (e.g., lines 551, 859, 1199). Since verbose() is inherited from modules.BaseModule, the check isn't protecting anything the inner try/catch doesn't already cover. The outer condition adds noise and creates an engine-portability question mark (whether inherited methods land in variables scope is engine-dependent). The simpler form already in use elsewhere is:

try { verbose("Update check failed: " & e.message); } catch (any ignore) {}

---

Tests

Missing reason field in struct-shape assertion

cli/lucli/tests/specs/services/UpdateCheckerSpec.cfc, lines 103–112:

it("returns a populated struct even when skipped", () => {
    var r = uc.check(currentVersion="");
    expect(structKeyExists(r, "hasUpdate")).toBeTrue();
    expect(structKeyExists(r, "skipped")).toBeTrue();
    expect(structKeyExists(r, "current")).toBeTrue();
    expect(structKeyExists(r, "latest")).toBeTrue();
    expect(structKeyExists(r, "channel")).toBeTrue();
    expect(structKeyExists(r, "upgradeCommand")).toBeTrue();
});

The component-level docstring documents reason as a required field of the result struct, but the struct-shape test doesn't verify structKeyExists(r, "reason"). Worth adding to lock the contract.

---

Docs

No [Unreleased] entry was added to CHANGELOG.md for this user-visible feature. The wheels new output change (new hint line when a newer version is available) should appear under ### Added in the CLI section, following the project's Keep-a-Changelog convention.

---

Correctness (no issues)

For the record — the following were verified and are correct:

• $isNewer correctly delegates to SemVer.compare for the base version and falls through to $snapshotNumber only when bases tie and channel is bleeding-edge. The stable-channel early return is right.
• $snapshotNumber's character class [.+] correctly matches both . (post-fix format) and + (legacy format) without regex over-matching. The reFindNoCase return shape check (arrayLen(m.match) < 2) correctly handles the no-match case where CFML returns [""].
• $readCache / $writeCache round-trip: CFML struct keys are case-insensitive so the uppercase-key serialization quirk on Adobe CF is a non-issue here.
• dateDiff("s", parseDateTime(entry.checkedAt), now()) for the 24h TTL is correct, and the outer try/catch in $readCache handles a malformed timestamp gracefully.
• The GitHub API endpoint choice (releases/latest for stable skipping pre-releases; releases?per_page=1 for bleeding-edge to include them) is correct.
• The test base class (wheels.wheelstest.system.BaseSpec) matches every other spec in cli/lucli/tests/specs/services/.
• Commit message feat(cli): non-blocking update check on wheels new conforms to commitlint rules.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is clean. No sycophancy, no meaningful false positives. The three nits (redundant verbose guard, missing reason in the struct-shape test, no CHANGELOG entry) are concrete and actionable. One additional dead-code observation below. Aligned on converged-changes.

Sycophancy

None detected. Every approval in the "Correctness" section cites the specific code path A verified. No blanket "looks good" without evidence.

False positives

One partial false positive in finding #1: A says the structKeyExists(variables, "verbose") guard "creates an engine-portability question mark (whether inherited methods land in variables scope is engine-dependent)." This framing is slightly misleading for CLI code — the Wheels CLI runs exclusively on Lucee Express, so engine variance is not a live concern here, and in practice inherited methods do appear in variables scope in all CFML engines. The real argument for removing the guard is the one A also states: it is redundant (the inner try/catch already covers all failure modes) and inconsistent with every other verbose() call-site in the file. A's conclusion (drop the outer if) is correct; the portability framing is noise, not a blocker.

Missed issues

UpdateChecker.cfc ~line 149 — !arrayLen(m.match) is dead code

var m = reFindNoCase("snapshot[.+]([0-9]+)", arguments.version, 1, true);
if (!arrayLen(m.match) || arrayLen(m.match) < 2) return 0;

When reFindNoCase finds no match with returnsubexpressions=true, CFML returns {match: [""], pos: [0]} — an array with one empty element, never a zero-length array. The !arrayLen(m.match) branch can therefore never be true; arrayLen(m.match) < 2 already handles the no-match case on its own. This is not a correctness bug, but it is dead defensive code that creates a false impression the empty-array case is possible. Worth folding into A's other findings.

Verdict alignment

A chose COMMENTED (not request-changes) for three nit-level findings — each is localized to one line/test/file and none affects correctness. That verdict is appropriate and consistent with the findings.

Convergence

Aligned. A has three concrete, addressable findings plus the dead-code nit above. Joint recommendation: apply all four fixes — remove the structKeyExists(variables, "verbose") outer guard; add structKeyExists(r, "reason") to the struct-shape test; add a CHANGELOG entry under ### Added; simplify the $snapshotNumber guard by dropping the dead !arrayLen(m.match) arm. Once those land, the PR is merge-ready.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):

• cli/lucli/Module.cfc ~line 4197 — Dropped the redundant if (structKeyExists(variables, "verbose")) wrapper. The inner try/catch already covers every failure mode and the guard was inconsistent with every other verbose() call-site in the file (lines 551, 859, 1199).
• cli/lucli/services/UpdateChecker.cfc ~line 134 — Simplified the $snapshotNumber guard by removing the dead !arrayLen(m.match) arm. reFindNoCase with returnsubexpressions=true always returns at least {match: [""], pos: [0]} on no match, so the < 2 check alone is sufficient. Added a short comment explaining the invariant.
• cli/lucli/tests/specs/services/UpdateCheckerSpec.cfc ~line 108 — Added expect(structKeyExists(r, "reason")).toBeTrue(); to the struct-shape test so the documented reason field is locked into the contract.
• CHANGELOG.md — [Unreleased] § ### Added / **CLI & LuCLI** — Added a Keep-a-Changelog entry for the user-visible wheels new update-check hint, with channel/cache/timeout details and the PR reference (feat(cli): non-blocking update check on wheels new #2556).

No findings were intentionally skipped — the four items above cover the full union of A's findings and B's missed-issue, and B explicitly aligned on all four in the converged-changes comment.

Tests were not re-run locally for this round (PR branch is peter/update-check-on-wheels-new, not fix/bot-*; changes are mechanical and the affected spec assertion is purely additive). CI will exercise cli/lucli/tests/specs/services/UpdateCheckerSpec.cfc 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 PR adds a non-blocking update-check hint to wheels new. The fixup commit (round 1) correctly addresses every finding raised by the previous review round — the verbose guard is gone, the struct-shape test covers reason, the $snapshotNumber guard is simplified, and the CHANGELOG entry is present. The core logic is correct, cross-engine safe, and the test coverage is honest about what it does and does not exercise. Two minor nits below; neither is a correctness blocker, so this is a comment rather than a request-for-changes.

---

Correctness

Docstring for skipped field is inaccurate (minor)

cli/lucli/services/UpdateChecker.cfc, lines 39-41:

The actual behaviour when the cache is fresh and the cached tag is not newer than the current version is {hasUpdate: false, skipped: false} — skipped stays false. The clause "cache TTL not expired AND no newer version cached" describes a case where the code never sets skipped = true. The three real conditions that produce skipped = true are: (1) channel has no repo (dev/rc), (2) no release tag found on the repo, and (3) any exception in the try/catch. The docstring should reflect this so callers don't write guards against the wrong condition.

Suggested correction:

skipped: boolean,        // true when channel doesn't auto-check (dev/rc),
                          // no release tag found on the repo, or any error
                          // during the check — hasUpdate is always false when skipped

---

Conventions

Fallback cache path is not per-user (minor)

cli/lucli/services/UpdateChecker.cfc, line 157:

return "/tmp/.wheels-update-check.json";

This is only reached when both LUCLI_HOME and user.home are unavailable — an unusual situation. However, /tmp is world-writable, so on a multi-user system two accounts running wheels new concurrently could stomp each other's cache entry. Since this is a last-ditch fallback and all write failures are already swallowed silently, the practical impact is low — but worth noting so the pattern is not copied into something less incidental.

---

Tests

The struct-shape test now verifies all seven documented fields including reason (added in the fixup). The decision to exclude network-path tests is explicitly documented in the spec's opening comment and is the right trade-off given the wrapping try/catch. No gaps.

Cross-engine

No issues found. The cfhttp tag-in-script form, isNull() for Java-null detection, dateTimeFormat, directoryCreate(dir, true), and the reFindNoCase struct shape are all safe across Lucee 5/6/7, Adobe CF 2018-2025, and BoxLang. The [.+] character class in $snapshotNumber is intentional and correct — it is NOT a quantifier; it matches exactly one separator character that is either . or +, covering both the post-fix and legacy snapshot formats.

Commits

Both commits conform to commitlint rules:

• feat(cli): non-blocking update check on wheels new — valid type, valid scope, subject within 100 chars
• fix(cli): address Reviewer A/B consensus findings (round 1) — valid type, valid scope, subject within 100 chars

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review on the updated SHA is clean. Finding #1 (inaccurate skipped docstring) is a genuine and correctly diagnosed inaccuracy with a concrete fix. Finding #2 (/tmp fallback) is an accurate awareness note with no code change required. No sycophancy, no false positives, no missed issues. Aligned on converged-changes to land the docstring correction.

Sycophancy

None detected. A's approvals in the Correctness, Tests, Cross-engine, and Commits sections each cite the specific code path or commit text verified. The "no gaps" summary in Tests is backed by the explicit statement that the decision to skip network-path tests is documented in the spec's opening comment.

False positives

None detected.

Finding #1 — skipped docstring: A claims the clause "cache TTL not expired AND no newer version cached" describes a condition that never sets skipped = true. Verified against the check() function: when the cache is fresh and the fetched tag is not newer, the function returns {hasUpdate: false, skipped: false} — skipped remains at its initialized value of false. The three conditions that actually set skipped = true are (1) empty repo (dev/rc channel), (2) empty latest from the fetch, and (3) catch block. A's diagnosis is accurate and the suggested replacement docstring covers all three correctly.

Finding #2 — /tmp fallback: The $resolveCachePath() catch block does return /tmp/.wheels-update-check.json, which is world-writable on Linux. A's characterization of the practical impact as low is accurate — this path is only reached when both LUCLI_HOME and user.home are unavailable AND an exception is thrown in the Java interop, which is an extraordinary situation. All cache writes are already wrapped in their own try/catch. A correctly notes this as a pattern-awareness nit rather than requesting a code change.

Missed issues

None detected. Spot-checks performed:

• isNull(home) on Adobe CF with Java null: System.getenv() returns Java null when the env var is unset. On Adobe CF, isNull() may not detect Java null, which could cause len(trim(home)) to throw. However, the entire $resolveCachePath() body is wrapped in try { ... } catch (any e) { return "/tmp/..."; }, so any such exception is silently caught and the fallback path is taken. Practically benign, and the CLI runs on Lucee Express anyway, where isNull() correctly detects Java null.
• releases?per_page=1 ordering: GitHub Releases API returns releases in descending created_at order by default, so per_page=1 does return the latest snapshot. Correct.
• $snapshotNumber regex [.+]: A already verified this — it is a character class matching literal . or +, not a quantifier. Correct.
• Closure variable access (uc vs variables.uc): The spec sets variables.uc in beforeAll() and accesses it as bare uc inside arrow-function closures. In CFML, closures capture the enclosing CFC's variables scope, so bare uc resolves to variables.uc. This is not the local scope gotcha called out in CLAUDE.md; no issue.
• Commit messages: Both conform to commitlint — feat(cli) and fix(cli) are valid type+scope combinations, subjects are within 100 chars, neither is ALL-CAPS.

Verdict alignment

A chose COMMENTED rather than REQUEST_CHANGES for two nit-level findings, neither of which affects runtime correctness. That verdict is consistent with the findings.

Convergence

Aligned. Finding #1 has a concrete, targeted fix: replace the inaccurate docstring clause with A's suggested text. Finding #2 requires no code change. Joint recommendation: apply the docstring correction to UpdateChecker.cfc (the skipped field comment, lines 39-41), then the PR is merge-ready.

[wheels-bot]

Wheels Bot — Address Review (round 2)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1 on SHA f7921ebd):

• cli/lucli/services/UpdateChecker.cfc lines 34–37 (component-level docstring) — Corrected the inaccurate skipped field description. The previous text claimed skipped became true "when cache TTL not expired AND no newer version cached", but that code path actually returns {hasUpdate: false, skipped: false} — skipped stays at its initialized value. Replaced with the three real triggers, lifted from Reviewer A’s suggested correction: channel has no repo (dev/rc), no release tag found on the repo, or any error during the check. Also kept A’s "hasUpdate is always false when skipped" invariant in the comment so callers don’t need to read the body to know what to expect.

Skipped (intentionally, not in consensus):

• Reviewer A finding New master #2 (/tmp fallback in $resolveCachePath, line 157) — A characterized this as a pattern-awareness nit with no code change required, and Reviewer B explicitly confirmed "no code change required". Not part of the converged-changes alignment.

Tests were not re-run locally for this round (PR branch is peter/update-check-on-wheels-new, not fix/bot-*; the change is a CFC docstring comment only — no executable code touched). CI will exercise cli/lucli/tests/specs/services/UpdateCheckerSpec.cfc 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: Round 2 is a docstring-only fix. The corrected description of the three skipped = true conditions matches the actual code paths in check() exactly. No new issues. This PR is ready to merge.

---

Correctness

The round 2 change corrects the skipped field comment at cli/lucli/services/UpdateChecker.cfc, lines 34–37. The previous text described a path that does not exist — "cache TTL not expired AND no newer version cached" never produces skipped = true; that code path returns {hasUpdate: false, skipped: false}. The new text correctly identifies the three real triggers:

1. !len(repo) at line 74 — dev/rc channels with no release repo → "channel 'X' does not auto-check"
2. !len(latest) at line 82 — no tag returned from the GitHub API → "no release found on repo"
3. catch (any e) at line 93 — any unhandled exception in the check → "check failed: " & e.message

Verified against the check() implementation at lines 61–98. The docstring is now accurate.

---

Verification of SemVer.compare contract (for the record)

I read cli/lucli/services/SemVer.cfc to confirm the compare() function ignores pre-release labels by design (lines 61–70 — only major/minor/patch are compared, preRelease is stored but never touched in compare). This validates the $isNewer design: SemVer.compare returns 0 for any two versions with the same 4.0.0 base regardless of snapshot suffix, which is exactly why the bleeding-edge branch falls through to $snapshotNumber. The legacy format case (4.0.0-SNAPSHOT+1656 vs 4.0.0-snapshot.1790) also works: parse() splits on - first, so the +1656 lands in preRelease and never reaches the + stripping branch — base parses to {4, 0, 0} for both, and $snapshotNumber then correctly extracts 1656 and 1790 respectively.

---

Prior rounds — all findings resolved

Two prior rounds found and fixed: the superfluous structKeyExists(variables, "verbose") guard, the missing reason field in the struct-shape test, the CHANGELOG entry, and the inaccurate skipped docstring. The world-writable /tmp fallback in $resolveCachePath() was a pattern-awareness note; the commit message explicitly documents the deliberate deferral of that one.

---

Commits

All three commits conform to commitlint rules:

Commit	Type	Scope	Length
feat(cli): non-blocking update check on wheels new	feat	cli	46 chars
fix(cli): address Reviewer A/B consensus findings (round 1)	fix	cli	53 chars
fix(cli): address Reviewer A/B consensus findings (round 2)	fix	cli	53 chars

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review on the current SHA is clean. A docstring-only round with all prior findings already applied; the APPROVED verdict is correct and consistent with the evidence cited.

Sycophancy

None detected. A's approval cites the specific three code paths where skipped = true is set (lines 74, 82, 93 of check()), verified against the implementation. The SemVer.compare verification is backed by explicit line references in SemVer.cfc. No blanket LGTM without evidence.

False positives

None detected. A's claim that the corrected docstring accurately describes the three skipped = true triggers was verified against the diff:

• !len(repo) at the channel-guard block → reason: "channel '...' does not auto-check" ✓
• !len(latest) after the fetch → reason: "no release found on " ✓
• catch (any e) → reason: "check failed: " & e.message ✓

The previous docstring text ("cache TTL not expired AND no newer version cached") was genuinely inaccurate — that code path returns {hasUpdate: false, skipped: false}, not a skip. A's diagnosis is correct.

Missed issues

None detected. Spot-checks performed:

• Prior round 1 fixes confirmed applied: verbose guard removed from Module.cfc outer catch; dead !arrayLen(m.match) arm removed from $snapshotNumber with an explanatory comment; reason field added to the struct-shape test; CHANGELOG entry present under ### Added.
• $snapshotNumber regex [.+]: character class matching literal . or +, not a quantifier — correct for both version formats.
• Commit messages: all three conform to commitlint (feat(cli), fix(cli) x2; subjects within 100 chars, not ALL-CAPS).
• upgradeCommand field: initialized to "" in the result struct and only set when hasUpdate = true. The struct-shape test checks structKeyExists(r, "upgradeCommand") for the skipped path — presence confirmed. The value-when-true path requires network mocking; the spec's opening comment explicitly defers that to manual smoke testing. Acceptable gap, not a finding.

Verdict alignment

A chose APPROVED. Consistent with the findings: one docstring correction with no runtime behavior change, all prior findings addressed, no new issues. Appropriate verdict.

Convergence

Aligned. A's review is accurate and complete for a docstring-only round. Nothing remains to address. Joint recommendation: approve and merge.