diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index 217eb2bb9656..5b1b2004aaea 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -114,8 +114,17 @@ def get_available_skills() -> Dict[str, List[str]]: # Update check # ========================================================================= -# Cache update check results for 6 hours to avoid repeated git fetches -_UPDATE_CHECK_CACHE_SECONDS = 6 * 3600 +# Cache update check results. Two layers of invalidation: +# 1. Time-bounded: this TTL bounds the worst-case staleness for cases +# that don't go through the git head-hash check below (PyPI/nix). +# 2. Content-bounded: for git checkouts, a successful cache hit also +# requires local HEAD and origin/main to match the values we cached +# last time. That probe catches real upstream updates within seconds +# of `git fetch` landing new commits, regardless of this TTL. +# Keep this short so a user who lets their machine sleep for a day and +# comes back gets a real answer on the first run after wake, not a +# 6-hour-stale one. Network cost is one `git fetch` per hour per user. +_UPDATE_CHECK_CACHE_SECONDS = 60 * 60 # Sentinel returned when we know an update exists but can't count commits # (e.g. nix-built hermes — no local git history to count against). @@ -328,12 +337,19 @@ def check_for_updates() -> Optional[int]: except Exception: pass - # Read cache — invalidate if the embedded rev OR installed version has - # changed since the last check. The version guard matters for pip installs: - # `check_via_pypi()` compares against VERSION, so a `pip install --upgrade` - # changes VERSION but leaves rev unchanged (both None), and without this - # the stale "behind" count would survive the upgrade for up to 6h. See #34491. + # Read cache — invalidate when ANY of these change: + # - embedded rev (nix) + # - installed VERSION (pip upgrades) + # - upstream HEAD hash at the active git checkout (git installs) + # - local HEAD hash (a fresh `git pull` or branch switch) + # The upstream/local-head guards are the load-bearing ones for git + # checkouts: without them the cache returns a stale "behind: 0" for up + # to 6h after upstream gains new commits, while VERSION stays pinned. + # We pay one extra `git rev-parse` per cached hit (sub-millisecond) to + # keep the "Up to date" line honest. now = time.time() + cached_upstream_head = None + cached_local_head = None try: if cache_file.exists(): cached = json.loads(cache_file.read_text()) @@ -342,7 +358,28 @@ def check_for_updates() -> Optional[int]: and cached.get("rev") == embedded_rev and cached.get("ver") == VERSION ): - return cached.get("behind") + # For git checkouts, refuse to trust the cache if either + # side of the comparison moved. Cheap probe — skips network. + if not embedded_rev: + repo_dir = _resolve_repo_dir() + live_local = ( + _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) + if repo_dir else None + ) or None + live_upstream = ( + _git_stdout(["rev-parse", "origin/main"], cwd=repo_dir) + if repo_dir else None + ) or None + cached_upstream_head = cached.get("upstream_head") + cached_local_head = cached.get("local_head") + if ( + cached_upstream_head == live_upstream + and cached_local_head == live_local + ): + return cached.get("behind") + # Mismatch — fall through and re-check upstream. + else: + return cached.get("behind") except Exception: pass @@ -360,9 +397,44 @@ def check_for_updates() -> Optional[int]: else: behind = _check_via_local_git(repo_dir) + # Persist the upstream/local heads we just observed. Three cases: + # 1. Cache miss (no cache file, or rev/ver mismatch): probe never ran, + # cached_*_head is None → do a fresh `git rev-parse`. + # 2. Cache hit with head mismatch: probe DID read live values, but + # cached_*_head still holds the OLD cached values (line 373). + # We need to re-read live values, not persist the stale ones. + # 3. Cache hit with no head movement: probe's live values match cache, + # we can re-persist the (now-verified) live values to keep the + # cache file's mtime fresh without re-running git. + # The fix unifies 1+2 by always using live values when the probe ran + # (i.e. on a cache hit of any kind), and only doing a fresh read on a + # true cache miss. try: + if embedded_rev: + persisted_upstream = None + persisted_local = None + else: + repo_dir = _resolve_repo_dir() + if repo_dir is None: + persisted_upstream = persisted_local = None + else: + # Live values are what we want to persist. cached_*_head + # is only valid on a true cache miss (None). On a hit of + # either kind the probe populated live_local/live_upstream + # already — re-read to be safe (sub-ms, no network). + persisted_local = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) or None + persisted_upstream = _git_stdout(["rev-parse", "origin/main"], cwd=repo_dir) or None cache_file.write_text( - json.dumps({"ts": now, "behind": behind, "rev": embedded_rev, "ver": VERSION}) + json.dumps( + { + "ts": now, + "behind": behind, + "rev": embedded_rev, + "ver": VERSION, + "upstream_head": persisted_upstream, + "local_head": persisted_local, + } + ) ) except Exception: pass diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 5f76c1fc8d4f..eee417e08675 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -4360,18 +4360,32 @@ def _print_version_info(*, check_updates: bool = True) -> None: # Show update status (synchronous — acceptable since user asked for version info) try: - from hermes_cli.banner import check_for_updates + from hermes_cli.banner import check_for_updates, UPDATE_AVAILABLE_NO_COUNT from hermes_cli.config import recommended_update_command behind = check_for_updates() - if behind and behind > 0: - commits_word = "commit" if behind == 1 else "commits" - print( - f"Update available: {behind} {commits_word} behind — " - f"run '{recommended_update_command()}'" - ) + if behind is None: + # Check didn't run (offline, no remote, docker image, etc.) + pass elif behind == 0: print("Up to date") + elif behind == UPDATE_AVAILABLE_NO_COUNT or behind > 0: + # -1 = "behind but shallow clone can't count exact number" — still + # an update, just no precise figure. Surface it as "an update is + # available" rather than silently swallowing the case (the prior + # `if behind and behind > 0` check dropped -1, hiding the very + # signal the user is asking for). + if behind > 0: + commits_word = "commit" if behind == 1 else "commits" + print( + f"Update available: {behind} {commits_word} behind — " + f"run '{recommended_update_command()}'" + ) + else: + print( + f"Update available — run '{recommended_update_command()}' " + f"(shallow checkout, exact count unavailable)" + ) except Exception: pass diff --git a/tests/hermes_cli/test_update_check.py b/tests/hermes_cli/test_update_check.py index 84b9e3a6c991..8c23442bcdb1 100644 --- a/tests/hermes_cli/test_update_check.py +++ b/tests/hermes_cli/test_update_check.py @@ -17,24 +17,60 @@ def test_version_string_no_v_prefix(): def test_check_for_updates_uses_cache(tmp_path, monkeypatch): - """When cache is fresh, check_for_updates should return cached value without calling git.""" + """When cache is fresh and heads match, check_for_updates returns cached value. + + The cache-hit path now probes local HEAD and origin/main to detect stale + cache entries (cheap rev-parse, no network). Those two calls are expected. + No fetch, ls-remote, rev-list, or pypi should happen. + """ from hermes_cli.banner import check_for_updates from hermes_cli import __version__ - # Create a fake git repo and fresh cache repo_dir = tmp_path / "hermes-agent" repo_dir.mkdir() (repo_dir / ".git").mkdir() + fake_banner = repo_dir / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + + import hermes_cli.banner as banner + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + # Cache with matching heads — should be accepted cache_file = tmp_path / ".update_check" - cache_file.write_text(json.dumps({"ts": time.time(), "behind": 3, "ver": __version__})) + cache_file.write_text(json.dumps({ + "ts": time.time(), + "behind": 3, + "rev": None, + "ver": __version__, + "upstream_head": "live-upstream", + "local_head": "live-local", + })) monkeypatch.setenv("HERMES_HOME", str(tmp_path)) - with patch("hermes_cli.banner.subprocess.run") as mock_run: + + rev_parse_calls = 0 + + def fake_run(cmd, **kwargs): + nonlocal rev_parse_calls + if cmd == ["git", "rev-parse", "HEAD"]: + rev_parse_calls += 1 + return MagicMock(returncode=0, stdout="live-local\n") + if cmd == ["git", "rev-parse", "origin/main"]: + rev_parse_calls += 1 + return MagicMock(returncode=0, stdout="live-upstream\n") + raise AssertionError(f"unexpected network command: {cmd}") + + with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.banner.check_via_pypi") as mock_pypi: result = check_for_updates() assert result == 3 - mock_run.assert_not_called() + # Two rev-parse probes are fine; no network operations + assert rev_parse_calls == 2 + mock_pypi.assert_not_called() def test_check_for_updates_invalidates_on_version_change(tmp_path, monkeypatch): @@ -93,8 +129,8 @@ def test_check_for_updates_expired_cache(tmp_path, monkeypatch): result = check_for_updates() assert result == 5 - # origin probe + is-shallow probe + git fetch + git rev-list - assert mock_run.call_count == 4 + # remote get-url + is-shallow + fetch + rev-list + 2× rev-parse (cache write) + assert mock_run.call_count == 6 def test_check_for_updates_official_ssh_origin_uses_https_probe(tmp_path): @@ -372,3 +408,334 @@ def test_invalidate_update_cache_no_profiles_dir(tmp_path): _invalidate_update_cache() assert not (default_home / ".update_check").exists() + + +# ========================================================================= +# Head-movement cache invalidation tests +# ========================================================================= + + +def test_check_for_updates_cache_invalidates_on_upstream_head_movement(tmp_path, monkeypatch): + """Cache hit with stale upstream_head must be rejected and re-checked. + + When upstream has advanced (``git fetch`` landed new commits) but + VERSION and rev are unchanged, the old cache payload's ``upstream_head`` + no longer matches the live ``origin/main`` SHA. The probe must detect + this, reject the cache, and run a fresh check. Without this guard the + banner and ``hermes version`` would report "Up to date" for up to the + TTL window despite real upstream movement. + """ + import hermes_cli.banner as banner + from hermes_cli import __version__ + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + + # Set __file__ to point inside repo_dir so _resolve_repo_dir finds it + fake_banner = repo_dir / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + + # Populate a cache with OLD upstream_head and local_head + cache_file = tmp_path / ".update_check" + cache_file.write_text( + json.dumps({ + "ts": time.time(), + "behind": 0, + "rev": None, + "ver": __version__, + "upstream_head": "old-upstream-sha", + "local_head": "local-sha", + }) + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + call_log = [] + + def fake_run(cmd, **kwargs): + call_log.append(cmd) + if cmd[:2] == ["git", "rev-parse"] and "--is-shallow-repository" in cmd: + return MagicMock(returncode=0, stdout="false\n") + if cmd == ["git", "remote", "get-url", "origin"]: + return MagicMock(returncode=0, stdout="https://github.com/NousResearch/hermes-agent.git\n") + if cmd[:2] == ["git", "fetch"]: + return MagicMock(returncode=0, stdout="") + if cmd[:3] == ["git", "rev-list", "--count"]: + return MagicMock(returncode=0, stdout="3\n") + if cmd == ["git", "rev-parse", "HEAD"]: + return MagicMock(returncode=0, stdout="local-sha\n") + if cmd == ["git", "rev-parse", "origin/main"]: + return MagicMock(returncode=0, stdout="new-upstream-sha\n") + raise AssertionError(f"unexpected command: {cmd}") + + with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.config.detect_install_method", return_value="git"): + result = banner.check_for_updates() + + # Upstream moved → cache rejected → full check ran → 3 behind + assert result == 3 + # Fresh check proceeded: remote get-url, is-shallow, fetch, rev-list + assert any(c[:3] == ["git", "rev-list", "--count"] for c in call_log), ( + "should have run rev-list --count on a cache miss" + ) + + # Cache was rewritten with the new upstream_head + written = json.loads(cache_file.read_text()) + assert written["upstream_head"] == "new-upstream-sha" + assert written["local_head"] == "local-sha" + + +def test_check_for_updates_cache_invalidates_on_local_head_movement(tmp_path, monkeypatch): + """Cache hit with stale local_head must be rejected and re-checked. + + A ``git pull`` or ``git checkout`` can change local HEAD while + VERSION stays pinned. Without the local_head guard, the cache + would return a stale "behind" count until the TTL expired. + """ + import hermes_cli.banner as banner + from hermes_cli import __version__ + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + + fake_banner = repo_dir / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + + # Cache has OLD local_head but current upstream_head + cache_file = tmp_path / ".update_check" + cache_file.write_text( + json.dumps({ + "ts": time.time(), + "behind": 0, + "rev": None, + "ver": __version__, + "upstream_head": "upstream-sha", + "local_head": "old-local-sha", + }) + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + def fake_run(cmd, **kwargs): + if cmd[:2] == ["git", "rev-parse"] and "--is-shallow-repository" in cmd: + return MagicMock(returncode=0, stdout="false\n") + if cmd == ["git", "remote", "get-url", "origin"]: + return MagicMock(returncode=0, stdout="https://github.com/NousResearch/hermes-agent.git\n") + if cmd[:2] == ["git", "fetch"]: + return MagicMock(returncode=0, stdout="") + if cmd[:3] == ["git", "rev-list", "--count"]: + return MagicMock(returncode=0, stdout="0\n") + if cmd == ["git", "rev-parse", "HEAD"]: + return MagicMock(returncode=0, stdout="new-local-sha\n") + if cmd == ["git", "rev-parse", "origin/main"]: + return MagicMock(returncode=0, stdout="upstream-sha\n") + raise AssertionError(f"unexpected command: {cmd}") + + with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.config.detect_install_method", return_value="git"): + result = banner.check_for_updates() + + # Local HEAD moved → cache rejected → fresh check → up to date (0) + assert result == 0 + + written = json.loads(cache_file.read_text()) + assert written["local_head"] == "new-local-sha" + + +def test_check_for_updates_cache_honours_stable_heads(tmp_path, monkeypatch): + """When both local and upstream heads match the cache, the cached value is returned. + + On a stable checkout (no ``git pull``, no upstream movement) the + git rev-parse probes should match the cached values and the behind + count should be served from cache without any network operation. + """ + import hermes_cli.banner as banner + from hermes_cli import __version__ + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + + fake_banner = repo_dir / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + + cache_file = tmp_path / ".update_check" + cache_file.write_text( + json.dumps({ + "ts": time.time(), + "behind": 0, + "rev": None, + "ver": __version__, + "upstream_head": "stable-upstream", + "local_head": "stable-local", + }) + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + # Only rev-parse calls should happen — no fetch, no ls-remote, no pypi + rev_parse_count = 0 + + def fake_run(cmd, **kwargs): + nonlocal rev_parse_count + if cmd == ["git", "rev-parse", "HEAD"]: + rev_parse_count += 1 + return MagicMock(returncode=0, stdout="stable-local\n") + if cmd == ["git", "rev-parse", "origin/main"]: + rev_parse_count += 1 + return MagicMock(returncode=0, stdout="stable-upstream\n") + raise AssertionError(f"unexpected network command: {cmd}") + + with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.config.detect_install_method", return_value="git"), \ + patch("hermes_cli.banner.check_via_pypi") as mock_pypi: + result = banner.check_for_updates() + + assert result == 0 + # Exactly two rev-parse calls (local + upstream), no network + assert rev_parse_count == 2 + mock_pypi.assert_not_called() + + +def test_check_for_updates_cache_accepts_legacy_payload(tmp_path, monkeypatch): + """A legacy cache payload (without upstream_head/local_head keys) is + treated as a cache miss — the mismatch between cached None and live + SHA forces a fresh check. + + Before the head-guard change (PR #9670) the cache was + ``{ts, behind, rev, ver}`` with no head fields. An upgraded client + must not accept a stale "behind" from a pre-head-guard cache, because + that cached value may have been written hours ago and upstream may + have moved since. The probe reads ``cached.get("upstream_head")`` + which returns None for legacy payloads, while live values are real + SHAs — the mismatch causes a fresh check. + """ + import hermes_cli.banner as banner + from hermes_cli import __version__ + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + + fake_banner = repo_dir / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + + # Legacy payload — no upstream_head or local_head keys + cache_file = tmp_path / ".update_check" + cache_file.write_text( + json.dumps({ + "ts": time.time(), + "behind": 0, + "rev": None, + "ver": __version__, + }) + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + def fake_run(cmd, **kwargs): + if cmd[:2] == ["git", "rev-parse"] and "--is-shallow-repository" in cmd: + return MagicMock(returncode=0, stdout="false\n") + if cmd == ["git", "remote", "get-url", "origin"]: + return MagicMock(returncode=0, stdout="https://github.com/NousResearch/hermes-agent.git\n") + if cmd[:2] == ["git", "fetch"]: + return MagicMock(returncode=0, stdout="") + if cmd[:3] == ["git", "rev-list", "--count"]: + return MagicMock(returncode=0, stdout="1\n") + if cmd == ["git", "rev-parse", "HEAD"]: + return MagicMock(returncode=0, stdout="some-local-sha\n") + if cmd == ["git", "rev-parse", "origin/main"]: + return MagicMock(returncode=0, stdout="some-upstream-sha\n") + raise AssertionError(f"unexpected command: {cmd}") + + with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.config.detect_install_method", return_value="git"): + result = banner.check_for_updates() + + # Legacy cache rejected → fresh check ran + assert result == 1 + + # Cache rewritten with head fields + written = json.loads(cache_file.read_text()) + assert written["upstream_head"] == "some-upstream-sha" + assert written["local_head"] == "some-local-sha" + + +# ========================================================================= +# _print_version_info output tests +# ========================================================================= + + +def test_print_version_info_behind_none(tmp_path, monkeypatch, capsys): + """When check_for_updates() returns None, _print_version_info() prints + no update message (the check was inconclusive). + """ + from hermes_cli.main import _print_version_info + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + with patch("hermes_cli.banner.check_for_updates", return_value=None): + _print_version_info(check_updates=True) + + captured = capsys.readouterr() + assert "Up to date" not in captured.out + assert "Update available" not in captured.out + + +def test_print_version_info_behind_zero(tmp_path, monkeypatch, capsys): + """When check_for_updates() returns 0, print 'Up to date'.""" + from hermes_cli.main import _print_version_info + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + with patch("hermes_cli.banner.check_for_updates", return_value=0): + _print_version_info(check_updates=True) + + captured = capsys.readouterr() + assert "Up to date" in captured.out + assert "Update available" not in captured.out + + +def test_print_version_info_behind_positive(tmp_path, monkeypatch, capsys): + """When check_for_updates() returns a positive count, print the exact + number and the update command. + """ + from hermes_cli.main import _print_version_info + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + with patch("hermes_cli.banner.check_for_updates", return_value=5), \ + patch("hermes_cli.config.recommended_update_command", return_value="hermes update"): + _print_version_info(check_updates=True) + + captured = capsys.readouterr() + assert "Update available: 5 commits behind" in captured.out + assert "hermes update" in captured.out + + +def test_print_version_info_behind_update_available_no_count(tmp_path, monkeypatch, capsys): + """When check_for_updates() returns UPDATE_AVAILABLE_NO_COUNT (-1), + print a plain 'Update available' message noting the shallow checkout. + """ + from hermes_cli.main import _print_version_info + from hermes_cli.banner import UPDATE_AVAILABLE_NO_COUNT + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + with patch("hermes_cli.banner.check_for_updates", return_value=UPDATE_AVAILABLE_NO_COUNT), \ + patch("hermes_cli.config.recommended_update_command", return_value="hermes update"): + _print_version_info(check_updates=True) + + captured = capsys.readouterr() + assert "Update available" in captured.out + assert "shallow checkout" in captured.out + assert "hermes update" in captured.out + # No bogus commit count in the shallow message + assert "0 commits" not in captured.out + assert "-1" not in captured.out