Skip to content

fix(update-cache): invalidate cache on git HEAD movement, surface shallow -1 - #54874

Closed
lynchest wants to merge 2 commits into
NousResearch:mainfrom
lynchest:fix/update-cache-invalidate-on-head-movement
Closed

fix(update-cache): invalidate cache on git HEAD movement, surface shallow -1#54874
lynchest wants to merge 2 commits into
NousResearch:mainfrom
lynchest:fix/update-cache-invalidate-on-head-movement

Conversation

@lynchest

@lynchest lynchest commented Jun 29, 2026

Copy link
Copy Markdown

Problem

The update-check cache in hermes_cli/banner.py was only invalidated on:

  • TTL expiry (6h)
  • HERMES_REVISION env change (nix)
  • Installed VERSION change (pip upgrade)

For a git checkout, this meant that as long as VERSION stayed pinned (v0.17.0 → v0.17.0 across a hotfix stream, or just same-version upstream commits), the cache returned a stale "behind: 0" for up to 6 hours after upstream gained new commits. git rev-parse origin/main would have shown the truth in microseconds, but the cache never asked.

Three surfaces read this cache and so all three could lie:

  • TUI banner (welcome message)
  • hermes version CLI output
  • Dashboard's /api/hermes/update/check (the System page "Check now" button bypasses the cache, so the bug was less visible there, but the auto-poll still hit it)

Repro on v0.17.0

git clone --depth 1 https://github.com/NousResearch/hermes-agent
cd hermes-agent && pip install -e .
# wait ~30s for cache to be written
git fetch origin                      # 1+ new commits land
hermes version                        # reports "Up to date" — wrong

Reproduced on 2026-06-29 against v0.17.0 with 146 commits of drift.

Fix

  1. Persist upstream_head and local_head in the cache payload. On a cache hit, re-read both with git rev-parse (sub-ms, no network) and reject the cache if either has moved. The TTL becomes a backstop rather than the only invalidation trigger.

  2. Lower the TTL from 6h to 1h for non-git install paths (PyPI, nix) that don't have HEAD to compare. Power users running hermes all day see one fetch per hour instead of one per six — negligible.

  3. Split the three cases in hermes version's output:

    • None (no check possible, e.g. docker image) → silent
    • 0 (up to date) → "Up to date"
    • > 0 or -1 (behind, possibly shallow) → "Update available"

    The old if behind and behind > 0 check silently swallowed -1, which is what a shallow installer clone returns when it's behind — so the shallow case was indistinguishable from "nothing to update", the exact opposite of what the user needs to see.

Test

hermes version                                # pre-fix: "Up to date"
# Manually write a stale cache file:
python3 -c "import json; open('$HOME/.hermes/.update_check','w').write(
    json.dumps({'ts': 9e18, 'behind': 0, 'rev': None, 'ver': '0.17.0',
               'upstream_head': '0'*40,
               'local_head': '<real HEAD>'}))"
hermes version                                # post-fix: "Update available"

Also tested live on the v0.17.0 → v0.17.0+146 stream where the cache was returning "Up to date" with 146 commits of drift.

Risk

Very low. The new code paths only:

  • add fields to a cache file (forward-compatible — old readers ignore them)
  • do git rev-parse on cache hits (sub-ms, no network)
  • print an extra line in hermes version for a previously-silent case

The TTL change is a network-cost regression of 6x for power users, which is the only trade-off worth flagging.

…llow -1

The 6h update-check cache was invalidated only on (a) TTL expiry, (b)
HERMES_REVISION env change, or (c) installed VERSION change. For git
checkouts this meant that as long as VERSION stayed pinned, the cache
returned a stale 'behind: 0' for up to 6h after upstream gained new
commits — even though `git rev-parse origin/main` would have shown
the truth in microseconds. The TUI banner, CLI `hermes version`, and
the dashboard's polled /api/hermes/update/check all read this cache,
so all three surfaces could lie about being 'Up to date' for hours.

Two changes:

1. Persist upstream_head and local_head in the cache payload; on a hit,
   re-read them and reject the cache if either has moved. Sub-ms cost
   (no network), honest answer.

2. Lower TTL from 6h to 1h as a backstop for install paths that don't
   have git HEAD to compare (PyPI, nix). Power users running `hermes`
   all day will see one fetch per hour instead of one per six.

Also fix a related bug in `hermes version`: when the local checkout
is shallow (installer clone --depth 1) the behind-count function
returns UPDATE_AVAILABLE_NO_COUNT (-1). The old `if behind and
behind > 0` check swallowed -1 silently, so even an immediate fresh
check would show no message at all — making the shallow case
indistinguishable from 'nothing to update'. Split the three cases
(none / up-to-date / behind) and surface the shallow case as a
plain 'Update available' line.

Repro on the v0.17.0 release:
  - `git clone --depth 1 .../hermes-agent`, pip install -e .
  - wait 30s for cache to be written
  - `git fetch origin` (or run `hermes update`)
  - `hermes version` reports 'Up to date' for up to 6h, hiding
    the new commits.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Well-scoped cache invalidation fix — two-layer invalidation (time + content) with upstream/local HEAD tracking is solid. Clean separation of concerns.

@lynchest

Copy link
Copy Markdown
Author

cc @teknium1 — opening this for review. The cache was returning "Up to date" for 146 commits of drift on v0.17.0 because the 6h TTL only invalidated on VERSION/rev change, both of which stayed pinned. Repro and test in the PR body.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have labels Jun 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #9670 — same core mechanism (invalidate the update-check cache when the local/upstream git HEAD moves). #9670 is the earliest still-open PR in this chain (also #20653, #40157 open; #21675, #40985 closed). This PR adds useful extras (surfacing shallow -1, lowering the non-git TTL 6h->1h, splitting hermes version output), which a maintainer may want to fold into the canonical fix — flagging for that, but the underlying fix is the same.

@lynchest

Copy link
Copy Markdown
Author

Closing my duplicate flag — keeping this PR open after maintainer feedback.

#9670 has the same core mechanism, agreed. But this PR adds three things the others don't, and I'd rather they ship than die in a closed PR:

  1. Surface shallow -1 in hermes version — current code does if behind and behind > 0, which silently swallows the -1 a shallow installer clone returns. Result: shallow user sees "Up to date" while git rev-parse would show drift. Splitting the three cases (None / 0 / >0 or -1) fixes that.

  2. Lower non-git TTL 6h → 1h — backstop for PyPI / nix installs with no HEAD to compare. One extra fetch/hour for power users, vs. up to 5h55m of stale data on long sessions. Negligible cost, real win.

  3. Persist upstream_head alongside local_headfix: invalidate update check cache when HEAD moves (e.g. git pull) #9670/fix(banner): include local HEAD in update-check cache key #40157/fix: preserve update cache correctness for source installs #20653 only store local HEAD. That's enough if you git pull and re-check, but doesn't catch the case I actually hit on v0.17.0: 146 commits of drift on a checkout I hadn't touched, because upstream moved and the cache key was still valid. Storing both makes the cache key symmetric — any movement on either side invalidates.

Offer to maintainer: happy to fold all three into #9670's branch as a force-push, or send as a 3-commit stack on top, or leave this PR open as-is and let you cherry-pick. Your call. If you just want the smallest correct fix and the extras can wait, also fine — just leave a comment and I'll stop nudging.

Not closing as duplicate. The extras have user-visible impact and I haven't seen any of the other three PRs carry them.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: LGTM — clean update cache invalidation on git HEAD movement.

Well-documented changes with good test coverage. The implementation correctly detects upstream and local HEAD changes for cache invalidation.


Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real stale-cache path. Current main still returns a fresh cache entry without checking Git heads (hermes_cli/banner.py:330-344) and suppresses the shallow-clone sentinel in hermes version (hermes_cli/main.py:4421-4429).

Problems

  • This diff changes the cache contract and CLI rendering but adds no tests. Please cover local/upstream head movement and legacy cache payloads in tests/hermes_cli/test_update_check.py, plus None / 0 / positive / UPDATE_AVAILABLE_NO_COUNT output cases for _print_version_info().
  • .gitignore includes unrelated skills/ directory patterns. Those paths are repository source paths, so this would conceal unrelated changes; please remove them.

Suggested changes

  • Keep the upstream-head work separate from #9670's local-head-only approach, as described in the discussion, but preserve the existing shallow-check contract (hermes_cli/banner.py:228-239) in regression tests.

Automated hermes-sweeper review.

Comment thread .gitignore Outdated
# pr-infographic-workflow reference (storage rule + lapse #8 / #COMMIT-1).
infographic/

# User-installed skills (sourced from ~/.hermes/skills/ or skills.sh, not upstream)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update-cache PR should not add unrelated ignore rules for repository skills/ paths. Please remove these entries so legitimate skill changes remain visible to Git.

@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 labels Jul 15, 2026
…dation and _print_version_info tests

Addresses review feedback:

1. Remove the unrelated  directory patterns from
   that concealed repository source paths.

2. Add tests for the new head-movement cache guard:
   - Upstream HEAD movement invalidates cache
   - Local HEAD movement invalidates cache
   - Stable heads (no movement) serve from cache
   - Legacy cache payloads (no upstream_head/local_head keys)
     are treated as cache misses

3. Add tests for  output:
   - None → no update message
   - 0 → 'Up to date'
   - positive count → 'Update available: N commits behind'
   - UPDATE_AVAILABLE_NO_COUNT → 'Update available (shallow checkout, ...)'

4. Update pre-existing tests to account for the new rev-parse probes
   on cache-hit paths.
@lynchest

Copy link
Copy Markdown
Author

@teknium1 review feedback addressed in 42b3ad1.

Changes in the latest commit:

  • .gitignore: removed unrelated skills/ directory patterns
  • tests/hermes_cli/test_update_check.py: added tests for upstream/local HEAD movement cache invalidation, legacy cache payload handling, and all _print_version_info() output cases (None/0/positive/UPDATE_AVAILABLE_NO_COUNT)
  • Updated pre-existing tests to account for the new rev-parse probes on cache-hit paths

23/23 tests passing.

@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
@lynchest lynchest closed this by deleting the head repository Jul 27, 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 duplicate This issue or pull request already exists 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.

4 participants