diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index c97d5dd15c..19d12da311 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -126,7 +126,12 @@ def get_available_skills() -> Dict[str, List[str]]: # === Update check === -_UPDATE_CHECK_CACHE_SECONDS = 6 * 3600 # avoid repeated git fetches +# Passive checks hit GitHub at most once a day per install; a failed check retries after an hour +# so a flaky line can't turn every startup into a request (nor stay wrong for a day). +_UPDATE_CHECK_CACHE_SECONDS = 24 * 3600 +_UPDATE_CHECK_FAILURE_CACHE_SECONDS = 3600 +# Upstream tip seen by the most recent check; recorded in the cache file for the changelog. +_last_target_rev: Optional[str] = None # Returned when an update is known to exist but commits can't be counted (e.g. nix builds). UPDATE_AVAILABLE_NO_COUNT = -1 @@ -197,10 +202,7 @@ def _git_ok(args: list[str], **kw) -> bool: def _git_count(args: list[str], *, cwd: Path) -> Optional[int]: - """``int`` of a successful ``git rev-list --count``-style command, else None. - - Deliberately bypasses ``_git_stdout`` so tests can stub the two layers independently. - """ + """``int`` of a successful ``git rev-list --count``-style command, else None.""" result = _git_run(args, cwd=cwd) if result is not None and result.returncode == 0: return _quiet(lambda: int(result.stdout.strip())) @@ -211,14 +213,21 @@ def _is_full_sha(value: Optional[str]) -> bool: return isinstance(value, str) and len(value) == 40 and all(c in "0123456789abcdefABCDEF" for c in value) -def _github_compare_behind(current_rev: str, target_rev: str) -> Optional[int]: - """Exact behind-count via the GitHub compare API for uncountable graphs. +_compare_payload_cache: Dict[tuple, dict] = {} - Shallow installer clones and ls-remote-only probes know the two tip SHAs but have no local - history to run ``rev-list --count`` across. + +def _github_compare(current_rev: str, target_rev: str) -> Optional[dict]: + """Compare payload for ``current...target`` from the GitHub API; memoized per process. + + Shallow installer clones and API-only probes know the two tip SHAs but have no local history + to run ``rev-list --count`` or ``git log`` across; the payload carries both the count + (``ahead_by``) and the commit list the dashboard/desktop render as "what's changed". """ if not (_is_full_sha(current_rev) and _is_full_sha(target_rev)): return None + key = (current_rev, target_rev) + if key in _compare_payload_cache: + return _compare_payload_cache[key] url = f"https://api.github.com/repos/nousresearch/hermes-agent/compare/{current_rev}...{target_rev}" def _fetch(): @@ -229,10 +238,49 @@ def _github_compare_behind(current_rev: str, target_rev: str) -> Optional[int]: with urllib.request.urlopen(req, timeout=10) as resp: return json.loads(resp.read().decode("utf-8")) payload = _quiet(_fetch) - ahead = payload.get("ahead_by") if isinstance(payload, dict) else None + if not isinstance(payload, dict): + return None + _compare_payload_cache[key] = payload + return payload + + +def _github_compare_behind(current_rev: str, target_rev: str) -> Optional[int]: + """Exact behind-count via the GitHub compare API for uncountable graphs.""" + payload = _github_compare(current_rev, target_rev) + ahead = payload.get("ahead_by") if payload else None return ahead if isinstance(ahead, int) and not isinstance(ahead, bool) and ahead >= 0 else None +def upstream_commits_behind(n: int = 20) -> List[Dict[str, Any]]: + """Commits between the last checked HEAD and upstream tip, newest first; [] when unknown. + + Reads the tips recorded by ``check_for_updates`` so it costs no extra request when the + compare payload is already memoized for this process. + """ + cached = _read_json(get_hermes_home() / ".update_check") or {} + head_rev, target_rev = cached.get("head"), cached.get("target") + if not head_rev or not target_rev or head_rev == target_rev: + return [] + payload = _github_compare(head_rev, target_rev) + rows: List[Dict[str, Any]] = [] + for entry in (payload or {}).get("commits", []) if isinstance(payload, dict) else []: + commit = entry.get("commit") or {} + when = ((commit.get("committer") or {}).get("date") or "") + try: + from datetime import datetime + at = int(datetime.fromisoformat(when.replace("Z", "+00:00")).timestamp()) if when else 0 + except ValueError: + at = 0 + rows.append({ + "sha": str(entry.get("sha", ""))[:7], + "summary": str(commit.get("message", "")).split("\n", 1)[0], + "author": str((commit.get("author") or {}).get("name", "")), + "at": at, + }) + rows.reverse() # compare returns oldest first + return rows[:n] + + def _tips_behind(head_rev: Optional[str], target_rev: Optional[str], repo_dir: Optional[Path] = None) -> Optional[int]: """Behind-count from two tip SHAs: None if either is unknown, 0 when equal, else count/sentinel. @@ -250,8 +298,27 @@ def _tips_behind(head_rev: Optional[str], target_rev: Optional[str], repo_dir: O return counted if counted is not None else UPDATE_AVAILABLE_NO_COUNT +def _github_branch_tip(repo_slug: str, branch: str) -> Optional[str]: + """Tip SHA of ``branch`` on GitHub via the REST API (40-byte body, no git, no auth).""" + from urllib.parse import quote + + url = f"https://api.github.com/repos/{repo_slug}/commits/{quote(branch, safe='')}" + + def _fetch(): + import urllib.request + req = urllib.request.Request( + url, headers={"Accept": "application/vnd.github.sha", "User-Agent": "hermes-cli-update-check"}) + with urllib.request.urlopen(req, timeout=10) as resp: + return resp.read().decode("utf-8").strip() + sha = _quiet(_fetch) + return sha if _is_full_sha(sha) else None + + def _upstream_main_sha() -> Optional[str]: - """Tip SHA of upstream main via HTTPS ls-remote (no auth, no prompts).""" + """Tip SHA of upstream main; API first, HTTPS ``ls-remote`` (no auth, no prompts) as fallback.""" + sha = _github_branch_tip(_OFFICIAL_REPO_CANONICAL.removeprefix("github.com/"), "main") + if sha: + return sha result = _git_run(["ls-remote", _UPSTREAM_REPO_URL, "refs/heads/main"], timeout=10, network=True) if result is None or result.returncode != 0 or not result.stdout: return None @@ -259,68 +326,40 @@ def _upstream_main_sha() -> Optional[str]: def _check_via_rev(local_rev: str) -> Optional[int]: - """Compare an embedded git revision to upstream main via ls-remote (see ``_tips_behind``).""" - return _tips_behind(local_rev, _upstream_main_sha()) + """Compare an embedded git revision to upstream main via the API (see ``_tips_behind``).""" + global _last_target_rev + _last_target_rev = _upstream_main_sha() + return _tips_behind(local_rev, _last_target_rev) def _check_via_local_git(repo_dir: Path) -> Optional[int]: - """Count commits behind origin/main in a local checkout.""" - # Probe the origin URL under the same config-isolated env as the fetch below. A plain - # get-url applies a global url..insteadOf rewrite, so an SSH origin masquerades as - # HTTPS, the SSH-avoiding fast path is skipped — and the fetch, whose env drops global - # config (GIT_CONFIG_GLOBAL=/dev/null), dials the raw SSH origin; its host-key prompt opens - # /dev/tty directly and steals the CLI's keystrokes (#104591). + """Count commits behind origin/main in a local checkout. + + Passive checks never run ``git fetch``: every CLI/TUI/gateway start used to negotiate a pack + with GitHub, and across the install base that was tens of millions of fetch requests a day + (GitHub asked us to poll the API instead). Two tip SHAs are enough — the remote one from the + API, the local one from ``rev-parse`` — and ``_tips_behind`` recovers the exact count through + the compare API when they differ. ``git fetch`` happens only inside ``hermes update``. + """ + # Probe the origin URL under the config-isolated env: a global url..insteadOf rewrite + # otherwise makes an SSH origin masquerade as HTTPS (#104591). origin_url = _git_stdout(["remote", "get-url", "origin"], cwd=repo_dir, network=True) - if _is_official_ssh_remote(origin_url): - head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) - if not head_rev: - return None - # Passive probe via HTTPS ls-remote (never SSH — no hardware-key prompts). Tip SHAs alone - # can't distinguish "behind" from a local commit AHEAD of origin/main, and misreporting an - # ahead checkout nudges the user into `hermes update`, which can wipe carried work — hence - # the ancestor check, against the FRESH upstream SHA (a stale tracking ref can't fake an - # up-to-date report). - return _tips_behind(head_rev, _upstream_main_sha(), repo_dir) - - # Installer checkouts are shallow (`git clone --depth 1`): a plain `git fetch` would unshallow - # the repo and `rev-list --count HEAD..origin/main` would report a bogus "12492 commits - # behind". Fetch with --depth 1 to preserve the boundary and compare tip SHAs instead. Full - # clones keep the exact count path. Mirrors apps/desktop/electron/main.cjs. - is_shallow = _git_stdout(["rev-parse", "--is-shallow-repository"], cwd=repo_dir) == "true" - - def _fetch() -> bool: - # Self-heal abandoned git lock files first. A stale .git/shallow.lock from a crashed fetch - # makes every fetch fail silently and stale refs get compared against HEAD until a human - # removes the lock. This passive check is also the main tmp_pack GENERATOR on flaky lines, - # so it must be the janitor too (#93732). - from hermes_cli.gitlock import clear_stale_git_locks, clear_stale_tmp_packs - clear_stale_git_locks(repo_dir) - clear_stale_tmp_packs(repo_dir) - - # Scope the fetch to the one branch compared against: an unscoped ``git fetch origin`` - # transfers ~1,400 remote heads (3.0 s vs 0.55 s measured) and can burn the full timeout. - # A scoped fetch still updates ``origin/main`` and FETCH_HEAD; ``--depth 1`` preserves - # the shallow boundary. - fetch_args = ["fetch", "origin", "main", *(["--depth", "1"] if is_shallow else []), "--quiet"] - return _git_ok(fetch_args, cwd=repo_dir, timeout=10, network=True) - - fetch_ok = _quiet(_fetch, False) # Offline or timeout — don't use stale refs - # When the fetch fails the local origin/main ref is stale: it cannot prove *currentness*, but - # if it already shows HEAD behind, that is sound evidence an update exists. Return the positive - # stale count; None (inconclusive) otherwise so the caller doesn't cache a false "up to date". - if is_shallow: - # (#82166, review #92578) - if not fetch_ok: - return None - # No history across the shallow boundary. `origin/main` may not be a tracking ref in a - # `clone --depth 1`, so prefer FETCH_HEAD (just updated) and fall back to origin/main. - head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) - target_rev = ( - _git_stdout(["rev-parse", "FETCH_HEAD"], cwd=repo_dir) - or _git_stdout(["rev-parse", "origin/main"], cwd=repo_dir)) - return _tips_behind(head_rev, target_rev) - behind = _git_count(["rev-list", "--count", "HEAD..origin/main"], cwd=repo_dir) - return behind if fetch_ok or (behind is not None and behind > 0) else None + head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) + if not head_rev: + return None + canonical = _canonical_github_remote(origin_url) + if canonical.startswith("github.com/"): + target_rev = _github_branch_tip(canonical.removeprefix("github.com/"), "main") + else: + # Non-GitHub origin: one ls-remote for the tip (ref advertisement only, no pack transfer). + result = _git_run(["ls-remote", "origin", "refs/heads/main"], cwd=repo_dir, timeout=10, network=True) + target_rev = result.stdout.split()[0] if result is not None and result.returncode == 0 and result.stdout else None + global _last_target_rev + _last_target_rev = target_rev + # Tip SHAs alone can't distinguish "behind" from a local commit AHEAD of origin/main, and + # misreporting an ahead checkout nudges the user into `hermes update`, which can wipe carried + # work — hence the ancestor check inside _tips_behind, against the FRESH upstream SHA. + return _tips_behind(head_rev, target_rev, repo_dir) def _read_json(path: Path) -> Optional[dict]: @@ -332,8 +371,8 @@ def _read_json(path: Path) -> Optional[dict]: def check_for_updates(*, passive: bool = False) -> Optional[int]: """Check whether a Hermes update is available. - If ``HERMES_REVISION`` is set (nix builds embed it), compare it to upstream main via - ``git ls-remote``; otherwise count commits behind ``origin/main`` in the local checkout. + If ``HERMES_REVISION`` is set (nix builds embed it), compare it to upstream main; otherwise + compare the local checkout's HEAD. Both go through the GitHub API, never ``git fetch``. """ def _read_config_opt_out(): from hermes_cli.config import load_config @@ -354,22 +393,26 @@ def check_for_updates(*, passive: bool = False) -> Optional[int]: if _quiet(_install_method) in {"docker", "apt"}: return None # Cache is invalidated when the embedded rev OR installed version changed since the last check. + # For a git checkout the local HEAD is part of the key too: `hermes update` moves HEAD, and a + # stale "3 behind" must not survive the update it just prompted. now = time.time() + repo_dir = None if embedded_rev else _resolve_repo_dir() + head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) if repo_dir is not None else None cached = _read_json(cache_file) - if (cached is not None and now - cached.get("ts", 0) < _UPDATE_CHECK_CACHE_SECONDS - and cached.get("rev") == embedded_rev and cached.get("ver") == VERSION): - return cached.get("behind") + if cached is not None and cached.get("rev") == embedded_rev and cached.get("ver") == VERSION \ + and cached.get("head") == head_rev: + ttl = _UPDATE_CHECK_CACHE_SECONDS if cached.get("behind") is not None else _UPDATE_CHECK_FAILURE_CACHE_SECONDS + if now - cached.get("ts", 0) < ttl: + return cached.get("behind") if embedded_rev: behind = _check_via_rev(embedded_rev) else: # No checkout and no embedded revision — status can't be determined. - repo_dir = _resolve_repo_dir() behind = _check_via_local_git(repo_dir) if repo_dir is not None else None - # Don't cache inconclusive results: None means the check could not run (typically a failed - # fetch), and caching it would suppress retries for the full 6-hour window (#82166). - if behind is not None: - _quiet(lambda: cache_file.write_text( - json.dumps({"ts": now, "behind": behind, "rev": embedded_rev, "ver": VERSION}), encoding="utf-8")) + _quiet(lambda: cache_file.write_text( + json.dumps({"ts": now, "behind": behind, "rev": embedded_rev, "ver": VERSION, + "head": head_rev or embedded_rev, "target": _last_target_rev}), + encoding="utf-8")) return behind diff --git a/hermes_cli/web_routers/actions.py b/hermes_cli/web_routers/actions.py index e5026f61d2..c135e7219f 100644 --- a/hermes_cli/web_routers/actions.py +++ b/hermes_cli/web_routers/actions.py @@ -230,36 +230,6 @@ async def update_hermes(): return {"ok": True, "pid": proc.pid, "name": "hermes-update", "action_id": action_id} -def _recent_upstream_commits(n: int = 20) -> List[Dict[str, Any]]: - """Commits the local checkout is behind ``origin/main`` by, newest first; [] on any failure. - - Logs the SAME range the behind-count uses (``HEAD..origin/main``, see - ``banner._check_via_local_git``), NOT ``@{upstream}``: on a feature branch that is - the branch's own tip (zero commits), leaving the changelog empty while the count is non-zero. - """ - try: - # git log emits UTF-8 (emoji/CJK subjects). On Windows text=True defaults to - # the ANSI code page; an undefined cp1252 byte crashed the stdlib - # _readerthread and killed the desktop backend — hence encoding="utf-8". - out = subprocess.run( - [ - "git", "-C", str(_server_path("PROJECT_ROOT")), "log", "--format=%H%x1f%s%x1f%an%x1f%ct", - "HEAD..origin/main", f"-n{int(n)}", - ], - capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5, - ) - if out.returncode != 0: - return [] - rows: List[Dict[str, Any]] = [] - for line in out.stdout.splitlines(): - if line.strip(): - sha, summary, author, at = (line.split("\x1f") + ["", "", "", "0"])[:4] - rows.append({"sha": sha[:7], "summary": summary, "author": author, "at": int(at or 0)}) - return rows - except Exception: - return [] - - _NON_APPLYABLE_MESSAGES = { "docker": format_docker_update_message, "apt": lambda: "Hermes is managed by Termux APT; run `pkg upgrade hermes-agent`.", @@ -295,10 +265,10 @@ async def check_hermes_update(force: bool = False): payload["message"] = non_applyable() return payload - # banner.check_for_updates() handles git / nix-revision paths and caches - # the result for 6h. ``force`` busts the cache so "Check now" reflects reality. + # banner.check_for_updates() handles git / nix-revision paths through the GitHub API and + # caches the result for 24h. ``force`` busts the cache so "Check now" reflects reality. try: - from hermes_cli.banner import check_for_updates + from hermes_cli.banner import check_for_updates, upstream_commits_behind if force: with contextlib.suppress(OSError): @@ -315,10 +285,9 @@ async def check_hermes_update(force: bool = False): payload["message"] = "You're on the latest version." else: payload["update_available"] = True - # "What's changed" for the desktop's remote update overlay; git only, - # best-effort (empty list on any failure). - if install_method == "git": - payload["commits"] = await asyncio.to_thread(_recent_upstream_commits) + # "What's changed" for the desktop's remote update overlay; best-effort + # (empty list on any failure). + payload["commits"] = await asyncio.to_thread(upstream_commits_behind) return payload diff --git a/tests/hermes_cli/test_banner_git_state.py b/tests/hermes_cli/test_banner_git_state.py index 23cdf4c234..e5bd64e5d0 100644 --- a/tests/hermes_cli/test_banner_git_state.py +++ b/tests/hermes_cli/test_banner_git_state.py @@ -177,13 +177,12 @@ def test_check_via_local_git_insteadof_rewrite_routes_to_ssh_fastpath(tmp_path, def spy_run(args, **kwargs): calls.append((list(args), kwargs)) - if args[1] == "ls-remote": - return MagicMock(returncode=0, stdout=f"{head_sha}\trefs/heads/main\n") - if args[1] == "fetch": - return MagicMock(returncode=1, stdout="") + if args[1] in {"ls-remote", "fetch"}: + raise AssertionError(f"a GitHub origin must be probed via the API, not git {args[1]}") return real_run(args, **kwargs) monkeypatch.setattr(banner.subprocess, "run", spy_run) + monkeypatch.setattr(banner, "_github_branch_tip", lambda slug, branch: head_sha) behind = banner._check_via_local_git(repo_dir) diff --git a/tests/hermes_cli/test_passive_update_opt_out.py b/tests/hermes_cli/test_passive_update_opt_out.py index 0d387820ae..053bb1f4df 100644 --- a/tests/hermes_cli/test_passive_update_opt_out.py +++ b/tests/hermes_cli/test_passive_update_opt_out.py @@ -10,8 +10,11 @@ def test_passive_check_obeys_config_before_using_cached_notice(monkeypatch): from hermes_cli import banner home = get_hermes_home() + # The cache is keyed on the checkout's HEAD (an update moving HEAD invalidates it). + repo_dir = banner._resolve_repo_dir() + head = banner._git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) if repo_dir else None (home / ".update_check").write_text(json.dumps({ - "ts": time.time(), "behind": 17, "rev": None, "ver": banner.VERSION, + "ts": time.time(), "behind": 17, "rev": None, "ver": banner.VERSION, "head": head, }), encoding="utf-8") monkeypatch.delenv("HERMES_REVISION", raising=False) config = home / "config.yaml" diff --git a/tests/hermes_cli/test_update_behind_count_recovery.py b/tests/hermes_cli/test_update_behind_count_recovery.py index a57ec786c6..930dd9ef3e 100644 --- a/tests/hermes_cli/test_update_behind_count_recovery.py +++ b/tests/hermes_cli/test_update_behind_count_recovery.py @@ -44,12 +44,20 @@ class _FakeResponse: def _patch_urlopen(payload): + banner._compare_payload_cache.clear() return patch( "urllib.request.urlopen", return_value=_FakeResponse(json.dumps(payload).encode()), ) +@pytest.fixture(autouse=True) +def _fresh_compare_cache(): + banner._compare_payload_cache.clear() + yield + banner._compare_payload_cache.clear() + + # --------------------------------------------------------------------------- # _github_compare_behind # --------------------------------------------------------------------------- @@ -98,69 +106,54 @@ def test_compare_behind_rejects_malformed_payloads(payload): # --------------------------------------------------------------------------- -def _ls_remote_result(sha): - return MagicMock(returncode=0, stdout=f"{sha}\trefs/heads/main\n") +def _upstream_tip(sha): + return patch.object(banner, "_github_branch_tip", return_value=sha) def test_check_via_rev_recovers_exact_count(): - with patch( - "hermes_cli.banner.subprocess.run", return_value=_ls_remote_result(SHA_B) - ), patch.object(banner, "_github_compare_behind", return_value=61) as compare: + with _upstream_tip(SHA_B), patch.object(banner, "_github_compare_behind", return_value=61) as compare: assert banner._check_via_rev(SHA_A) == 61 compare.assert_called_once_with(SHA_A, SHA_B) def test_check_via_rev_falls_back_to_sentinel_offline(): """FAIL-BEFORE (class): this path returned a fabricated 1 via callers.""" - with patch( - "hermes_cli.banner.subprocess.run", return_value=_ls_remote_result(SHA_B) - ), patch.object(banner, "_github_compare_behind", return_value=None): + with _upstream_tip(SHA_B), patch.object(banner, "_github_compare_behind", return_value=None): assert banner._check_via_rev(SHA_A) == banner.UPDATE_AVAILABLE_NO_COUNT def test_check_via_rev_up_to_date_short_circuits_compare(): - with patch( - "hermes_cli.banner.subprocess.run", return_value=_ls_remote_result(SHA_A) - ), patch.object(banner, "_github_compare_behind") as compare: + with _upstream_tip(SHA_A), patch.object(banner, "_github_compare_behind") as compare: assert banner._check_via_rev(SHA_A) == 0 compare.assert_not_called() def test_check_via_rev_local_ahead_reports_up_to_date(): """ahead_by == 0 with differing tips = local commits on top, not behind.""" - with patch( - "hermes_cli.banner.subprocess.run", return_value=_ls_remote_result(SHA_B) - ), patch.object(banner, "_github_compare_behind", return_value=0): + with _upstream_tip(SHA_B), patch.object(banner, "_github_compare_behind", return_value=0): assert banner._check_via_rev(SHA_A) == 0 # --------------------------------------------------------------------------- -# _check_via_local_git: shallow path recovers the exact count +# _check_via_local_git: tips from the API, exact count via compare, no fetch # --------------------------------------------------------------------------- -def _shallow_git(head_sha, fetch_head_sha): +def _local_git(head_sha): def fake_run(cmd, **kwargs): if cmd[:4] == ["git", "remote", "get-url", "origin"]: - return MagicMock( - returncode=0, - stdout="https://github.com/NousResearch/hermes-agent.git\n", - ) - if cmd[:3] == ["git", "rev-parse", "--is-shallow-repository"]: - return MagicMock(returncode=0, stdout="true\n") - if cmd[:2] == ["git", "fetch"]: - return MagicMock(returncode=0, stdout="") + return MagicMock(returncode=0, stdout="https://github.com/NousResearch/hermes-agent.git\n") if cmd[:3] == ["git", "rev-parse", "HEAD"]: return MagicMock(returncode=0, stdout=f"{head_sha}\n") - if cmd[:3] == ["git", "rev-parse", "FETCH_HEAD"]: - return MagicMock(returncode=0, stdout=f"{fetch_head_sha}\n") + if cmd[:3] == ["git", "merge-base", "--is-ancestor"]: + return MagicMock(returncode=1, stdout="") raise AssertionError(f"unexpected git command: {cmd!r}") return fake_run -def test_shallow_checkout_recovers_exact_count(tmp_path): - """The #84591 shape: shallow boundary kills merge-base, tips differ. +def test_local_checkout_recovers_exact_count(tmp_path): + """The #84591 shape: no local history across the tips (shallow clone), tips differ. FAIL-BEFORE (class): reported UPDATE_AVAILABLE_NO_COUNT (or, further back, a fabricated 1) even though the compare API could count exactly. @@ -168,31 +161,28 @@ def test_shallow_checkout_recovers_exact_count(tmp_path): repo_dir = tmp_path / "hermes-agent" repo_dir.mkdir() - with patch( - "hermes_cli.banner.subprocess.run", side_effect=_shallow_git(SHA_A, SHA_B) - ), patch.object(banner, "_github_compare_behind", return_value=61): + with patch("hermes_cli.banner.subprocess.run", side_effect=_local_git(SHA_A)), \ + patch.object(banner, "_github_branch_tip", return_value=SHA_B), \ + patch.object(banner, "_github_compare_behind", return_value=61): assert banner._check_via_local_git(repo_dir) == 61 -def test_shallow_checkout_offline_keeps_honest_sentinel(tmp_path): +def test_local_checkout_offline_compare_keeps_honest_sentinel(tmp_path): repo_dir = tmp_path / "hermes-agent" repo_dir.mkdir() - with patch( - "hermes_cli.banner.subprocess.run", side_effect=_shallow_git(SHA_A, SHA_B) - ), patch.object(banner, "_github_compare_behind", return_value=None): - assert ( - banner._check_via_local_git(repo_dir) - == banner.UPDATE_AVAILABLE_NO_COUNT - ) + with patch("hermes_cli.banner.subprocess.run", side_effect=_local_git(SHA_A)), \ + patch.object(banner, "_github_branch_tip", return_value=SHA_B), \ + patch.object(banner, "_github_compare_behind", return_value=None): + assert banner._check_via_local_git(repo_dir) == banner.UPDATE_AVAILABLE_NO_COUNT -def test_shallow_checkout_equal_tips_up_to_date_without_compare(tmp_path): +def test_local_checkout_equal_tips_up_to_date_without_compare(tmp_path): repo_dir = tmp_path / "hermes-agent" repo_dir.mkdir() - with patch( - "hermes_cli.banner.subprocess.run", side_effect=_shallow_git(SHA_A, SHA_A) - ), patch.object(banner, "_github_compare_behind") as compare: + with patch("hermes_cli.banner.subprocess.run", side_effect=_local_git(SHA_A)), \ + patch.object(banner, "_github_branch_tip", return_value=SHA_A), \ + patch.object(banner, "_github_compare_behind") as compare: assert banner._check_via_local_git(repo_dir) == 0 compare.assert_not_called() diff --git a/tests/hermes_cli/test_update_check.py b/tests/hermes_cli/test_update_check.py index 1669cf82ba..e872988dd7 100644 --- a/tests/hermes_cli/test_update_check.py +++ b/tests/hermes_cli/test_update_check.py @@ -1,70 +1,119 @@ -"""Tests for the update check mechanism in hermes_cli.banner.""" +"""Tests for the update check mechanism in hermes_cli.banner. + +Passive checks go through the GitHub REST API — never ``git fetch``. Every CLI, TUI and desktop +start used to fetch; across the install base that was tens of millions of fetch requests a day +and GitHub asked us to poll the API instead. These tests pin that contract plus the cache +policy that keeps the API traffic to one request a day per install. +""" import json -import os import threading import time -from pathlib import Path from unittest.mock import MagicMock, patch import pytest +import hermes_cli.banner as banner + +SHA_A = "a" * 40 +SHA_B = "b" * 40 - -def test_check_for_updates_uses_cache(tmp_path, monkeypatch): - """When cache is fresh, check_for_updates should return cached value without calling git.""" - from hermes_cli.banner import check_for_updates - from hermes_cli import __version__ - - # Create a fake git repo and fresh cache +@pytest.fixture +def git_repo(tmp_path, monkeypatch): + """A fake checkout the update check resolves to, with git calls stubbed out.""" repo_dir = tmp_path / "hermes-agent" repo_dir.mkdir() (repo_dir / ".git").mkdir() - - cache_file = tmp_path / ".update_check" - cache_file.write_text( - json.dumps({"ts": time.time(), "behind": 3, "ver": __version__}), - encoding="utf-8", - ) - monkeypatch.setenv("HERMES_HOME", str(tmp_path)) - with patch("hermes_cli.banner.subprocess.run") as mock_run: - result = check_for_updates() - - assert result == 3 - mock_run.assert_not_called() + monkeypatch.delenv("HERMES_REVISION", raising=False) + monkeypatch.setattr(banner, "_resolve_repo_dir", lambda: repo_dir) + monkeypatch.setattr("hermes_cli.config.detect_install_method", lambda root: "git") + monkeypatch.setattr("hermes_cli.config.get_project_root", lambda: repo_dir) + return repo_dir +def _stub_git(monkeypatch, *, head=SHA_A, origin="https://github.com/NousResearch/hermes-agent.git"): + calls = [] + + def fake_run(args, **kwargs): + calls.append(list(args)) + sub = args[1] + if sub == "rev-parse": + return MagicMock(returncode=0, stdout=f"{head}\n") + if sub == "remote": + return MagicMock(returncode=0, stdout=f"{origin}\n") + if sub == "merge-base": + return MagicMock(returncode=1, stdout="") + raise AssertionError(f"passive check must not run git {sub}: {args}") + + monkeypatch.setattr(banner.subprocess, "run", fake_run) + return calls +def test_passive_check_uses_the_api_and_never_fetches(git_repo, monkeypatch): + """The whole point: no ``git fetch`` / ``ls-remote`` for a GitHub origin, exact count via compare.""" + calls = _stub_git(monkeypatch, head=SHA_A) + tip = MagicMock(return_value=SHA_B) + monkeypatch.setattr(banner, "_github_branch_tip", tip) + monkeypatch.setattr(banner, "_github_compare_behind", lambda cur, tgt: 61) + + assert banner.check_for_updates() == 61 + tip.assert_called_once_with("nousresearch/hermes-agent", "main") + assert not any(c[1] in {"fetch", "ls-remote"} for c in calls) + + cached = json.loads((git_repo.parent / ".update_check").read_text()) + assert (cached["head"], cached["target"], cached["behind"]) == (SHA_A, SHA_B, 61) + + +def test_cache_is_daily_but_invalidated_when_head_moves(git_repo, monkeypatch): + """A fresh cache answers without any network; ``hermes update`` moving HEAD busts it at once; + an inconclusive (None) result is retried after the shorter failure window, not never.""" + from hermes_cli import __version__ + + cache_file = git_repo.parent / ".update_check" + _stub_git(monkeypatch, head=SHA_A) + tip = MagicMock(return_value=None) + monkeypatch.setattr(banner, "_github_branch_tip", tip) + + def write_cache(*, ts, head, behind): + cache_file.write_text(json.dumps( + {"ts": ts, "behind": behind, "rev": None, "ver": __version__, "head": head})) + + write_cache(ts=time.time() - banner._UPDATE_CHECK_CACHE_SECONDS + 60, head=SHA_A, behind=3) + assert banner.check_for_updates() == 3 + tip.assert_not_called() + + write_cache(ts=time.time(), head=SHA_B, behind=3) # cached for a different HEAD + assert banner.check_for_updates() is None # API unreachable → inconclusive, re-asked + tip.assert_called_once() + + tip.reset_mock() + write_cache(ts=time.time() - banner._UPDATE_CHECK_FAILURE_CACHE_SECONDS + 60, head=SHA_A, behind=None) + assert banner.check_for_updates() is None + tip.assert_not_called() + + write_cache(ts=time.time() - banner._UPDATE_CHECK_FAILURE_CACHE_SECONDS - 1, head=SHA_A, behind=None) + banner.check_for_updates() + tip.assert_called_once() def test_prefetch_non_blocking(): """prefetch_update_check() should return immediately without blocking.""" - import hermes_cli.banner as banner - - # Reset module state banner._update_result = None banner._update_check_done = threading.Event() with patch.object(banner, "check_for_updates", return_value=5): start = time.monotonic() banner.prefetch_update_check() - elapsed = time.monotonic() - start - - # Should return almost immediately (well under 1 second) - assert elapsed < 1.0 - - # Wait for the background thread to finish + assert time.monotonic() - start < 1.0 banner._update_check_done.wait(timeout=5) assert banner._update_result == 5 -def test_upstream_main_sha_disables_git_prompts(monkeypatch): - """The passive HTTPS probe must never inherit the interactive terminal.""" - from hermes_cli import banner - +def test_upstream_main_sha_ls_remote_fallback_disables_git_prompts(monkeypatch): + """When the API is unreachable the HTTPS ls-remote fallback must never inherit the terminal.""" + monkeypatch.setattr(banner, "_github_branch_tip", lambda slug, branch: None) completed = MagicMock(returncode=1, stdout="", stderr="auth required") run = MagicMock(return_value=completed) monkeypatch.setattr(banner.subprocess, "run", run) @@ -74,202 +123,3 @@ def test_upstream_main_sha_disables_git_prompts(monkeypatch): assert kwargs["stdin"] is banner.subprocess.DEVNULL assert kwargs["env"]["GIT_TERMINAL_PROMPT"] == "0" assert kwargs["env"]["GCM_INTERACTIVE"] == "Never" - - -def test_check_via_local_git_fetch_failure_returns_none(tmp_path, monkeypatch): - """When git fetch fails and the stale origin/main ref is not ahead, - _check_via_local_git must return None (#82166). - - A stale tracking ref cannot prove *currentness* (rev-list 0 just means - the ref hasn't caught up), so returning None is the honest inconclusive - result — and the caller must not cache it as "up to date". - """ - from hermes_cli import banner - - repo_dir = tmp_path / "hermes-agent" - repo_dir.mkdir() - (repo_dir / ".git").mkdir() - - # Simulate a non-shallow, non-SSH-remote checkout - def mock_git_stdout(args, *, cwd, timeout=5, network=False): - if args[:2] == ["remote", "get-url"]: - return "https://github.com/NousResearch/hermes-agent.git" - if args[:2] == ["rev-parse", "--is-shallow-repository"]: - return "false" - return None - - # Fetch fails (returncode != 0); stale rev-list reports 0 behind - failed_proc = MagicMock() - failed_proc.returncode = 1 - failed_proc.stdout = "" - failed_proc.stderr = "fatal: could not reach remote" - - stale_zero_proc = MagicMock() - stale_zero_proc.returncode = 0 - stale_zero_proc.stdout = "0" - - fetch_kwargs = None - - def mock_run(args, **kwargs): - nonlocal fetch_kwargs - if args[:2] == ["git", "fetch"]: - fetch_kwargs = kwargs - return failed_proc - if args[:2] == ["git", "rev-list"]: - return stale_zero_proc - raise AssertionError(f"unexpected subprocess.run: {args}") - - monkeypatch.setattr(banner, "_git_stdout", mock_git_stdout) - monkeypatch.setattr(banner.subprocess, "run", mock_run) - - result = banner._check_via_local_git(repo_dir) - assert result is None, ( - "Fetch failure with stale 0-behind must return None, not 'up to date'" - ) - assert fetch_kwargs is not None - assert fetch_kwargs["stdin"] is banner.subprocess.DEVNULL - assert fetch_kwargs["env"]["GIT_TERMINAL_PROMPT"] == "0" - assert fetch_kwargs["env"]["GCM_INTERACTIVE"] == "Never" - - -def test_check_via_local_git_fetch_failure_keeps_positive_stale_count(tmp_path, monkeypatch): - """A failed fetch must preserve sound evidence: if the stale origin/main - ref already shows HEAD behind, that positive count is still an update - signal and must be returned (review #92578).""" - from hermes_cli import banner - - repo_dir = tmp_path / "hermes-agent" - repo_dir.mkdir() - (repo_dir / ".git").mkdir() - - def mock_git_stdout(args, *, cwd, timeout=5, network=False): - if args[:2] == ["remote", "get-url"]: - return "https://github.com/NousResearch/hermes-agent.git" - if args[:2] == ["rev-parse", "--is-shallow-repository"]: - return "false" - return None - - failed_proc = MagicMock() - failed_proc.returncode = 1 - failed_proc.stdout = "" - failed_proc.stderr = "fatal: could not reach remote" - - stale_behind_proc = MagicMock() - stale_behind_proc.returncode = 0 - stale_behind_proc.stdout = "5" - - def mock_run(args, **kwargs): - if args[:2] == ["git", "fetch"]: - return failed_proc - if args[:2] == ["git", "rev-list"]: - return stale_behind_proc - raise AssertionError(f"unexpected subprocess.run: {args}") - - monkeypatch.setattr(banner, "_git_stdout", mock_git_stdout) - monkeypatch.setattr(banner.subprocess, "run", mock_run) - - result = banner._check_via_local_git(repo_dir) - assert result == 5, "Stale positive behind-count must be preserved on fetch failure" - - -def test_check_via_local_git_fetch_failure_rev_list_error_returns_none(tmp_path, monkeypatch): - """If the stale rev-list itself fails, the check stays inconclusive (None).""" - from hermes_cli import banner - - repo_dir = tmp_path / "hermes-agent" - repo_dir.mkdir() - (repo_dir / ".git").mkdir() - - def mock_git_stdout(args, *, cwd, timeout=5, network=False): - if args[:2] == ["remote", "get-url"]: - return "https://github.com/NousResearch/hermes-agent.git" - if args[:2] == ["rev-parse", "--is-shallow-repository"]: - return "false" - return None - - failed_proc = MagicMock() - failed_proc.returncode = 1 - failed_proc.stdout = "" - failed_proc.stderr = "fatal: could not reach remote" - - bad_rev_list = MagicMock() - bad_rev_list.returncode = 128 - bad_rev_list.stdout = "" - bad_rev_list.stderr = "fatal: ambiguous argument 'HEAD..origin/main'" - - def mock_run(args, **kwargs): - if args[:2] == ["git", "fetch"]: - return failed_proc - if args[:2] == ["git", "rev-list"]: - return bad_rev_list - raise AssertionError(f"unexpected subprocess.run: {args}") - - monkeypatch.setattr(banner, "_git_stdout", mock_git_stdout) - monkeypatch.setattr(banner.subprocess, "run", mock_run) - - result = banner._check_via_local_git(repo_dir) - assert result is None - - -def test_check_for_updates_does_not_cache_none(tmp_path, monkeypatch): - """check_for_updates must not cache None results so a transient fetch - failure doesn't suppress retries for the full 6-hour cache window (#82166). - - Instead of mocking the full Path resolution chain, we verify the cache-write - guard directly: call check_for_updates with a mocked _check_via_local_git - that returns None, and confirm no cache file is created. - """ - import hermes_cli.banner as banner - - cache_file = tmp_path / ".update_check" - monkeypatch.setenv("HERMES_HOME", str(tmp_path)) - monkeypatch.delenv("HERMES_REVISION", raising=False) - - # Create a fake repo dir so the .git check passes - repo_dir = tmp_path / "hermes-agent" - repo_dir.mkdir() - (repo_dir / ".git").mkdir() - - # Mock the internal functions to force the local-git path returning None - monkeypatch.setattr(banner, "_check_via_local_git", lambda rd: None) - monkeypatch.setattr( - "hermes_cli.config.detect_install_method", lambda root: "git" - ) - monkeypatch.setattr( - "hermes_cli.config.get_project_root", lambda: repo_dir - ) - - # Patch __file__ resolution by monkeypatching the module's Path calls. - # check_for_updates does: Path(__file__).parent.parent.resolve() - # We intercept by making the resolve() return our fake repo_dir. - original_init = Path.__init__ - - def patched_path_init(self, *args, **kwargs): - original_init(self, *args, **kwargs) - - # Simpler: just patch the get_hermes_home and the repo_dir resolution - # by making check_for_updates find our fake repo via hermes_home fallback. - # The code checks Path(__file__).parent.parent/.git first, then falls - # back to hermes_home / "hermes-agent". We ensure the fallback hits. - # To do this, we make Path(__file__).parent.parent.resolve() return - # a path without .git, so it falls through to hermes_home / "hermes-agent". - real_resolve = Path.resolve - - def fake_resolve(self, *args, **kwargs): - s = str(self) - if "banner.py" in s or s.endswith("hermes_cli"): - # Return a path that has no .git, forcing the fallback - return tmp_path / "no-git-here" - return real_resolve(self, *args, **kwargs) - - monkeypatch.setattr(Path, "resolve", fake_resolve) - - result = banner.check_for_updates() - assert result is None - - # The cache file must NOT have been written with a None result - assert not cache_file.exists(), "None result must not be cached" - - - - diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 49ee09fa17..7ca818974f 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -152,11 +152,22 @@ Leaving these unset keeps the legacy defaults (`HERMES_API_TIMEOUT=1800`s, `HERM ## Update Behavior -### Background checks and SSH authentication +### Background checks + +Passive update checks (CLI banner, TUI badge, dashboard, desktop app) ask the +GitHub REST API for the tip of `main` and, when it differs from your checkout, +the compare endpoint for the exact count and changelog. They never run +`git fetch`, and every install asks at most **once per 24 hours** (a failed check +retries after an hour). Applying an update (`hermes update`, or the desktop's +Update button) always fetches fresh and invalidates the cached answer. Explicit +checks — `hermes update --check`, the desktop's "Check for Updates…" menu item, +Settings → About → "Check now" — bypass the cache. + +### SSH authentication The startup update check reads the origin URL with the same isolated Git configuration used for its network calls. Global `url.*.insteadOf` rewrites -therefore cannot hide an official SSH remote from the public HTTPS check. +therefore cannot hide an official SSH remote from the public HTTPS path. Hermes's isolated internal Git commands default to `ssh -o BatchMode=yes`: unknown host keys, passwords, and encrypted keys needing a passphrase fail diff --git a/website/docs/user-guide/features/web-dashboard.md b/website/docs/user-guide/features/web-dashboard.md index e3a8bbff75..8a38239e44 100644 --- a/website/docs/user-guide/features/web-dashboard.md +++ b/website/docs/user-guide/features/web-dashboard.md @@ -586,7 +586,7 @@ same auth gate as the rest of `/api/`. | `GET /api/ops/checkpoints` · `POST .../prune` | Inspect / prune the `/rollback` store | | `POST /api/ops/hooks` · `DELETE /api/ops/hooks` | Create / remove a shell hook (consent-gated) | | `GET /api/system/stats` | Host stats — OS, CPU, memory, disk, uptime | -| `GET /api/hermes/update/check` | Report update availability (commits behind, install method) without applying. For git installs that are behind, also returns a `commits` list (`sha`, `summary`, `author`, `at`) of what's changed. `?force=1` busts the 6h cache | +| `GET /api/hermes/update/check` | Report update availability (commits behind, install method) without applying. For git installs that are behind, also returns a `commits` list (`sha`, `summary`, `author`, `at`) of what's changed. `?force=1` busts the 24h cache (the check goes through the GitHub API, never `git fetch`) | | `GET /api/curator` · `PUT .../paused` · `POST .../run` | Skill-curator status + pause/resume + run | | `GET /api/portal` | Nous Portal auth + Tool Gateway routing (read-only) | | `POST /api/ops/prompt-size` · `/dump` · `/config-migrate` | Diagnostics (backgrounded) |