Skip to content

fix(banner): include local HEAD in update-check cache key - #40157

Closed
Kyzcreig wants to merge 1 commit into
NousResearch:mainfrom
ANG-Ventures:pr/update-cache-head-key
Closed

fix(banner): include local HEAD in update-check cache key#40157
Kyzcreig wants to merge 1 commit into
NousResearch:mainfrom
ANG-Ventures:pr/update-cache-head-key

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

The update-check cache key was {ts, rev, ver} only. For source installs, an in-place git pull/rebase moves HEAD without changing VERSION or HERMES_REVISION (both None outside nix builds), so a stale "commits behind" count survives the full 6h TTL after a manual update.

Symptom

After a successful manual git pull/rebase to an up-to-date checkout, hermes --version keeps printing e.g. Update available: 182 commits behind even though git rev-list --count HEAD..origin/main is 0 and hermes update --check reports up to date. The stale value is read straight from ~/.hermes/.update_check.

Cause

check_for_updates() only invalidates the cache when rev (HERMES_REVISION, nix-only) or ver (VERSION) changes. A manual git update changes neither, so the cached behind is reused until the 6h TTL expires. hermes update calls _invalidate_update_cache(), but a manual git pull does not.

Fix

Add the active checkout’s HEAD SHA to the cache key. Computed after the docker short-circuit so container images (no .git) never shell out to git. An in-place update now self-invalidates the cached count. hermes update’s explicit invalidation still works; this just additionally covers manual git updates.

Tests

  • test_check_for_updates_uses_cache — cache hit now requires matching head; verifies only the cheap git rev-parse HEAD runs (no fetch/rev-list).
  • test_check_for_updates_expired_cache — expects 3 git calls (rev-parse HEAD + fetch + rev-list).
  • Full tests/hermes_cli/test_update_check.py passes (11).

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 5, 2026
@Kyzcreig
Kyzcreig force-pushed the pr/update-cache-head-key branch from bd54a39 to 058936b Compare July 12, 2026 09:28
An in-place `git pull`/rebase moves HEAD without changing VERSION or
HERMES_REVISION (both None for source installs tracking a fork). The
update-check cache key was {rev, ver} only, so a stale 'commits behind'
count survived the full 6h TTL after a manual update — e.g. `hermes
--version` kept printing '182 commits behind' immediately after a
successful rebase to up-to-date.

Add the active checkout's HEAD SHA to the cache key (computed after the
docker short-circuit so containers never shell out to git) so an
in-place update self-invalidates the cached count. `hermes update`'s
explicit _invalidate_update_cache() still works; this just covers manual
git updates too.

Tests updated for the extra cheap `git rev-parse HEAD` on the cache-hit
and expired-cache paths.
@Kyzcreig
Kyzcreig force-pushed the pr/update-cache-head-key branch from 058936b to 0a526ac Compare July 12, 2026 09:51
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a remaining source-install cache-staleness path. Current main still returns a fresh cache using only ts, rev, and ver at hermes_cli/banner.py:339-344, before it resolves and checks the local repository at hermes_cli/banner.py:348-360.

Problems

  • The updated tests do not directly exercise the reported HEAD-mismatch transition. tests/hermes_cli/test_update_check.py:19-97 covers matching-head cache reuse and expiry, but not a fresh cached old HEAD followed by a new live HEAD.

Suggested changes

  • Add a regression test that makes git rev-parse HEAD differ from the cache, then verifies the cache is bypassed and rewritten with the new SHA.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026
@Kyzcreig

Kyzcreig commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #72013, which carries this fix/feature forward (fresh port with maintainer engagement). Consolidating our open PRs after an audit — the newer PR is the one to review. Thanks!

@Kyzcreig Kyzcreig closed this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants