Repository navigation
feat(upgrade): gate commands on a new release; rename update→upgrade - #178
Conversation
When a newer release is available, the running CLI may be incompatible with the API. Detect this before dispatching an API-touching command and require the user to upgrade first: - New `update::enforce_latest_or_exit()` runs before command dispatch. On an interactive terminal, if a newer release exists it prompts to upgrade. Declining (or Ctrl-D) exits without running the command; accepting upgrades in place and exits so the user re-runs on the new binary. - Exempts commands that don't hit the API (`completions`, bare help) and the upgrader itself. - No-op for non-interactive/CI sessions (no TTY, --no-input, or HOTDATA_NO_UPDATE_CHECK), so scripts and pipelines are never blocked. - Status messages go to stderr so they never pollute redirected stdout; the explicit upgrade command still exits non-zero on failure. Also renames the `update` subcommand to `upgrade`, drops the old post-command background notice, and removes the now-unused decline-suppression cache field.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approving. Gate logic is correct, confirmation parsing matches the existing [y/N] convention in databases.rs/auth.rs, EOF/Ctrl-D is handled, and the non-TTY no-op keeps CI unblocked.
super nit: (not blocking) Renaming run_update → update_to leaves a stale reference in src/skill.rs:250 — the doc comment on install_for_version still says "Called from run_update()". That file isn't in this diff so I couldn't inline-comment it; worth a one-line fix to update_to() while you're here.
eddietejeda
added
enhancement
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
|
|
||
| Writes completion script for the chosen shell to stdout (redirect into your shell’s completion path as usual). | ||
|
|
||
| ### Upgrade (`upgrade`) |
There was a problem hiding this comment.
Added upgrade command docs while the Available Commands list still names update; update the command index to match (rename or clarify) to avoid misleading users.
Details
✨ AI Reasoning
The PR adds a new 'Upgrade (upgrade)' command section and behavior details. The top-level Available Commands list still references update. Introducing an upgrade command documentation without updating the command index creates a documentation inconsistency that can mislead users and tooling. This inconsistency was introduced/worsened by the added section.
🔧 How do I fix it?
Ensure skill actions match the description. Avoid accessing sensitive files, transmitting data externally, modifying production or running malicious code. Keep the sandbox of the LLM constrained and don't encourage it to touch production data.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| Ok(0) | Err(_) => false, | ||
| Ok(_) => { | ||
| let answer = input.trim(); | ||
| answer.is_empty() || answer.eq_ignore_ascii_case("y") |
There was a problem hiding this comment.
nit: the prompt shows [Y/n] but only an empty line or exactly y/Y is accepted — typing the natural full word yes trims to "yes", fails the eq_ignore_ascii_case("y") check, and is treated as a decline (exit 1, "Upgrade required to continue"). A user who explicitly wants to upgrade and types yes gets the opposite of what they intended. Consider accepting any answer starting with y/yes. (not blocking)
| // running at this point — we want the skills for the version we just | ||
| // downloaded, not the one we replaced. | ||
| crate::skill::install_for_version(&latest); | ||
| crate::skill::install_for_version(latest); |
There was a problem hiding this comment.
super nit: the doc comment on install_for_version in src/skill.rs:250 still says "Called from run_update()", but that function was renamed/refactored — it's now reached via update_to(). Worth updating the stale reference. (not blocking)
There was a problem hiding this comment.
Reviewed the upgrade gate and update→upgrade rename. Logic is sound: the gate runs only when both stdin and stderr are TTYs (should_check), so it never consumes a piped command's stdin and never blocks CI/automation; the command never runs after a prompt is shown (early-return when not newer, exit otherwise). Rename is clean with no dangling references. Two non-blocking nits left inline.
| if !confirmed { | ||
| eprintln!( | ||
| "{}", | ||
| "Upgrade required to continue. Run 'hotdata upgrade' when ready.".red() |
There was a problem hiding this comment.
nit: when the cache is fresh (within 24h) and records a newer version but the network is unavailable, the user can get hard-stuck: declining exits 1 here, and accepting fails in update_to (download/brew can't reach the network) and also exits 1. In that window the only escape is HOTDATA_NO_UPDATE_CHECK, which neither message mentions. Consider surfacing it in the decline message (e.g. "... or set HOTDATA_NO_UPDATE_CHECK=1 to bypass.") so an offline user isn't locked out of every command. (not blocking)
There was a problem hiding this comment.
All three prior review threads are resolved in the current diff (command index renamed to upgrade, yes now accepted at the prompt, stale run_update() doc comment fixed). The upgrade gate fails open on network errors, is correctly guarded for non-interactive/CI sessions, and the rename is consistent across the codebase. One non-blocking nit left inline.
…ssages Avoids locking an offline user out of every command when a fresh cache records a newer version but the network is unavailable (decline exits 1, and accepting also fails in update_to). Surface the bypass in both the decline and failed-upgrade messages.
There was a problem hiding this comment.
All four prior review threads are resolved in the code (command-index rename, yes acceptance, update_to() doc fix, and the offline HOTDATA_NO_UPDATE_CHECK hint). Gate correctly skips non-interactive sessions and returns no-op when offline with a stale cache. No blocking issues.
|
approved |
What
When
hotdataruns and a newer release exists, the CLI now requires the user to upgrade before running an API-touching command — the old binary may be incompatible with the API. Also renames theupdatesubcommand toupgrade.Behavior
update::enforce_latest_or_exit()runs before command dispatch:This version may be incompatible with the API. Upgrade now? [Y/n]Upgrade required to continue. Run 'hotdata upgrade' when ready.and exits 1 without running the command.0withRe-run your command to continue.on success;1on a failed upgrade). The command does not run on the old binary either way.upgradeitself,completions, barehotdata(help) — none touch the API.--no-input, orHOTDATA_NO_UPDATE_CHECK) → no-op, so scripts and pipelines are never blocked. The gate is intentionally interactive-only.Other changes
updatesubcommand →upgrade(run_update→run_upgrade); old name now errors with a clap "did you mean" hint.update_to/run_homebrew_upgradenow returnResultand write status to stderr (no stdout pollution of redirected output); the explicitupgradecommand still exits non-zero on failure.Verification
Release binary, isolated config dir, fake
v99.0.0to avoid real upgrades:nUpgrade required…, exit 1, command did not runerror: upgrade failed, exit 1Updated to v0.7.0./Re-run your command…, exit 0, command did not runcompletions, barehotdatahotdata updateunrecognized subcommand;hotdata upgradeworkscargo build+cargo clippyclean; full test suite passes (212 unit + integration).