From 282c72d61f150c923b139454fa9b2cb71a3e0eac Mon Sep 17 00:00:00 2001 From: "Sahil (AI)" <266772320+sahilm-ai@users.noreply.github.com> Date: Mon, 15 Jun 2026 16:30:55 +0530 Subject: [PATCH] fix(banner): count "commits behind" against the fork (myfork/main), not upstream, on fork-tracking checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On an inverted-topology checkout (origin = NousResearch upstream, myfork = the operator fork the machine deploys from), the startup banner counted HEAD..origin/main and perpetually nagged "N commits behind" even on a freshly-synced tree. The remedy hint was also wrong: the upstream CLI auto-pull path resets to upstream and discards fork-only commits. banner.py predated the fork-tracking work (PR #47) and never consumed hermes_cli.fork_tracking. This wires _check_via_local_git to it: - Resolve fork-tracking config first (read-only consumer of detect_fork_tracking). When it applies, count HEAD..{cfg.fork_ref} (myfork/main), fetching the fork remote — never origin. The banner then reads "N behind your fork" semantics: 0 on a synced checkout, non-zero only on genuine local drift behind the fork. - Else (canonical upstream-tracking install) keep today's exact behavior: count behind origin/main, suggest the upstream CLI auto-pull command. That path stays byte-identical for PyPI/container/official-remote installs. - Fork-tracking case gets fork-appropriate remedy copy instead of the upstream update hint. - _check_via_local_git now returns (behind, baseline); check_for_updates threads the baseline into the 6h .update_check cache (new "baseline" key) and a module-level getter so a fork cache is never read as an upstream cache or vice-versa, and the render picks the correct remedy. Verified on the live inverted checkout: HEAD..origin/main=5 (old, wrong), HEAD..myfork/main=0 (new, correct) -> banner shows 0 behind / no nag. Adds regression tests for the fork baseline (counts against myfork, fetches the fork, reports drift), the unchanged non-fork path, and baseline cache isolation. Updates the one expired-cache test to assert the upstream-path git calls are present rather than an exact subprocess call count (the fork-detection probe adds one leading call). --- hermes_cli/banner.py | 126 ++++++++++++++++--- tests/hermes_cli/test_update_check.py | 173 +++++++++++++++++++++++++- 2 files changed, 281 insertions(+), 18 deletions(-) diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index 952a09ef99fd7..6ba79da141d56 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -192,12 +192,69 @@ def _check_via_rev(local_rev: str) -> Optional[int]: return 0 if upstream_rev == local_rev else UPDATE_AVAILABLE_NO_COUNT -def _check_via_local_git(repo_dir: Path) -> Optional[int]: - """Count commits behind origin/main in a local checkout.""" +# Baselines the behind-count can be measured against. "upstream" is the +# canonical case (count behind origin/main, remedy = `hermes update`). +# "fork" is the inverted-topology operator case (count behind myfork/main, +# remedy = the fork-sync workflow, NOT `hermes update`). +BASELINE_UPSTREAM = "upstream" +BASELINE_FORK = "fork" + + +def _resolve_fork_tracking(repo_dir: Path): + """Return a ForkTrackingConfig when *repo_dir* is a fork-tracking checkout. + + Read-only consumer of ``hermes_cli.fork_tracking`` — the resolver returns + a config only when ``origin`` is the official upstream AND a separate + non-official fork remote (``myfork`` by convention) is present, else None. + Any import/detection failure degrades to None (canonical behavior). + """ + try: + from hermes_cli.fork_tracking import detect_fork_tracking + return detect_fork_tracking(["git"], repo_dir) + except Exception: + return None + + +def _check_via_local_git(repo_dir: Path) -> tuple[Optional[int], str]: + """Count commits the local checkout is behind its tracking baseline. + + Returns ``(behind, baseline)`` where *baseline* is ``BASELINE_FORK`` for an + inverted-topology fork-tracking checkout (count behind ``myfork/main``) or + ``BASELINE_UPSTREAM`` for the canonical case (count behind ``origin/main``). + *behind* is ``None`` when the check could not be computed. + """ + # Fork-tracking checkout (origin = upstream, myfork = the deployed fork): + # measure drift against the fork we actually track, fetching the fork + # remote — never origin/main, which would perpetually nag the operator. + cfg = _resolve_fork_tracking(repo_dir) + if cfg is not None: + try: + subprocess.run( + ["git", "fetch", cfg.fork_remote, "--quiet"], + capture_output=True, timeout=10, + cwd=str(repo_dir), + ) + except Exception: + pass # Offline or timeout — use stale refs, that's fine + + try: + result = subprocess.run( + ["git", "rev-list", "--count", f"HEAD..{cfg.fork_ref}"], + capture_output=True, text=True, timeout=5, + cwd=str(repo_dir), + ) + if result.returncode == 0: + return int(result.stdout.strip()), BASELINE_FORK + except Exception: + pass + return None, BASELINE_FORK + + # Canonical upstream-tracking install — behavior below stays byte-identical. origin_url = _git_stdout(["remote", "get-url", "origin"], cwd=repo_dir) if _is_official_ssh_remote(origin_url): head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) - return _check_via_rev(head_rev) if head_rev else None + behind = _check_via_rev(head_rev) if head_rev else None + return behind, BASELINE_UPSTREAM try: subprocess.run( @@ -215,10 +272,10 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: cwd=str(repo_dir), ) if result.returncode == 0: - return int(result.stdout.strip()) + return int(result.stdout.strip()), BASELINE_UPSTREAM except Exception: pass - return None + return None, BASELINE_UPSTREAM def _version_tuple(v: str) -> tuple[int, ...]: @@ -297,11 +354,14 @@ 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 if the embedded rev OR installed version OR the + # measured baseline 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. The baseline guard prevents a + # fork-tracking checkout from reading an upstream-baseline cache (or vice + # versa) — the two count against different refs and must not cross-pollute. now = time.time() try: if cache_file.exists(): @@ -311,10 +371,12 @@ def check_for_updates() -> Optional[int]: and cached.get("rev") == embedded_rev and cached.get("ver") == VERSION ): + _set_update_baseline(cached.get("baseline", BASELINE_UPSTREAM)) return cached.get("behind") except Exception: pass + baseline = BASELINE_UPSTREAM if embedded_rev: behind = _check_via_rev(embedded_rev) else: @@ -327,11 +389,16 @@ def check_for_updates() -> Optional[int]: if not (repo_dir / ".git").exists(): behind = check_via_pypi() else: - behind = _check_via_local_git(repo_dir) + behind, baseline = _check_via_local_git(repo_dir) + + _set_update_baseline(baseline) try: 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, "baseline": baseline, + }) ) except Exception: pass @@ -498,6 +565,22 @@ def format_banner_version_label() -> str: _update_result: Optional[int] = None _update_check_done = threading.Event() +# Tracks which baseline the most recent behind-count was measured against +# (BASELINE_UPSTREAM or BASELINE_FORK). The banner reads it to choose the +# correct remedy copy: `hermes update` for upstream installs, the fork-sync +# workflow for fork-tracking checkouts. +_update_baseline: str = BASELINE_UPSTREAM + + +def _set_update_baseline(baseline: str) -> None: + global _update_baseline + _update_baseline = baseline or BASELINE_UPSTREAM + + +def get_update_baseline() -> str: + """Return the baseline the last behind-count was measured against.""" + return _update_baseline + def prefetch_update_check(): """Kick off update check in a background daemon thread.""" @@ -774,12 +857,23 @@ def build_welcome_banner(console: "Console", model: str, cwd: str, behind = get_update_result(timeout=0.5) if behind is not None and behind != 0: from hermes_cli.config import get_managed_update_command, recommended_update_command + fork_tracking = get_update_baseline() == BASELINE_FORK if behind > 0: commits_word = "commit" if behind == 1 else "commits" - right_lines.append( - f"[bold yellow]⚠ {behind} {commits_word} behind[/]" - f"[dim yellow] — run [bold]{recommended_update_command()}[/bold] to update[/]" - ) + if fork_tracking: + # Fork-tracking checkout: `hermes update` is the wrong + # remedy (it resets to upstream). The fork-sync workflow + # integrates upstream; the operator usually just needs to + # let the live checkout catch up to its own fork. + right_lines.append( + f"[bold yellow]⚠ {behind} {commits_word} behind your fork[/]" + f"[dim yellow] — sync the live checkout to myfork/main[/]" + ) + else: + right_lines.append( + f"[bold yellow]⚠ {behind} {commits_word} behind[/]" + f"[dim yellow] — run [bold]{recommended_update_command()}[/bold] to update[/]" + ) else: # UPDATE_AVAILABLE_NO_COUNT: nix-built hermes; we know an update # exists but not by how much, and we don't know how the user diff --git a/tests/hermes_cli/test_update_check.py b/tests/hermes_cli/test_update_check.py index 5c590bff15cfd..d255a727c7bc4 100644 --- a/tests/hermes_cli/test_update_check.py +++ b/tests/hermes_cli/test_update_check.py @@ -93,7 +93,13 @@ def test_check_for_updates_expired_cache(tmp_path, monkeypatch): result = check_for_updates() assert result == 5 - assert mock_run.call_count == 3 # origin probe + git fetch + git rev-list + # Expired cache forces a recheck: the upstream path fetches origin and + # counts HEAD..origin/main. (A leading fork-tracking remote probe also + # runs; assert the upstream-path calls are present rather than an exact + # count so the test survives the fork-detection probe.) + issued = [c.args[0] for c in mock_run.call_args_list if c.args] + assert ["git", "fetch", "origin", "--quiet"] in issued + assert ["git", "rev-list", "--count", "HEAD..origin/main"] in issued def test_check_for_updates_official_ssh_origin_uses_https_probe(tmp_path): @@ -124,7 +130,9 @@ def fake_run(cmd, **kwargs): with patch("hermes_cli.banner.subprocess.run", side_effect=fake_run): result = banner._check_via_local_git(repo_dir) - assert result == banner.UPDATE_AVAILABLE_NO_COUNT + behind, baseline = result + assert behind == banner.UPDATE_AVAILABLE_NO_COUNT + assert baseline == banner.BASELINE_UPSTREAM assert ["git", "fetch", "origin", "--quiet"] not in calls @@ -278,3 +286,164 @@ def test_invalidate_update_cache_no_profiles_dir(tmp_path): _invalidate_update_cache() assert not (default_home / ".update_check").exists() + + +# --------------------------------------------------------------------------- +# Fork-tracking baseline (inverted topology: origin = upstream, myfork = fork) +# --------------------------------------------------------------------------- + + +def _fork_cfg(): + from hermes_cli.fork_tracking import ForkTrackingConfig + return ForkTrackingConfig(upstream_remote="origin", fork_remote="myfork") + + +def test_check_via_local_git_fork_tracking_counts_against_fork(tmp_path): + """On a fork-tracking checkout the count is HEAD..myfork/main, fetching myfork. + + Regression: the banner perpetually nagged on a synced fork checkout because + it measured behind-ness against origin/main (NousResearch upstream) instead + of the fork the machine actually tracks and deploys from. + """ + import hermes_cli.banner as banner + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd) + if cmd == ["git", "fetch", "myfork", "--quiet"]: + return MagicMock(returncode=0, stdout="") + if cmd == ["git", "rev-list", "--count", "HEAD..myfork/main"]: + return MagicMock(returncode=0, stdout="0\n") + raise AssertionError(f"unexpected git command: {cmd!r}") + + with patch("hermes_cli.fork_tracking.detect_fork_tracking", return_value=_fork_cfg()), \ + patch("hermes_cli.banner.subprocess.run", side_effect=fake_run): + behind, baseline = banner._check_via_local_git(repo_dir) + + assert behind == 0 + assert baseline == banner.BASELINE_FORK + # Fetched the fork, NOT origin, and counted against the fork ref. + assert ["git", "fetch", "myfork", "--quiet"] in calls + assert ["git", "fetch", "origin", "--quiet"] not in calls + assert ["git", "rev-list", "--count", "HEAD..origin/main"] not in calls + + +def test_check_via_local_git_fork_tracking_reports_drift(tmp_path): + """When the local tree drifts behind myfork/main, report the fork-relative count.""" + import hermes_cli.banner as banner + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + + def fake_run(cmd, **kwargs): + if cmd == ["git", "fetch", "myfork", "--quiet"]: + return MagicMock(returncode=0, stdout="") + if cmd == ["git", "rev-list", "--count", "HEAD..myfork/main"]: + return MagicMock(returncode=0, stdout="4\n") + raise AssertionError(f"unexpected git command: {cmd!r}") + + with patch("hermes_cli.fork_tracking.detect_fork_tracking", return_value=_fork_cfg()), \ + patch("hermes_cli.banner.subprocess.run", side_effect=fake_run): + behind, baseline = banner._check_via_local_git(repo_dir) + + assert behind == 4 + assert baseline == banner.BASELINE_FORK + + +def test_check_via_local_git_non_fork_unchanged(tmp_path): + """Non-fork install (no myfork remote) keeps the exact upstream behavior. + + Invariant guarding the common case: when fork-tracking does not apply the + count is still HEAD..origin/main, fetching origin, baseline == upstream. + """ + import hermes_cli.banner as banner + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd) + if cmd == ["git", "remote", "get-url", "origin"]: + # Canonical convention: origin is the user's own (non-official) fork. + return MagicMock(returncode=0, stdout="https://github.com/me/hermes-agent.git\n") + if cmd == ["git", "fetch", "origin", "--quiet"]: + return MagicMock(returncode=0, stdout="") + if cmd == ["git", "rev-list", "--count", "HEAD..origin/main"]: + return MagicMock(returncode=0, stdout="7\n") + raise AssertionError(f"unexpected git command: {cmd!r}") + + with patch("hermes_cli.fork_tracking.detect_fork_tracking", return_value=None), \ + patch("hermes_cli.banner.subprocess.run", side_effect=fake_run): + behind, baseline = banner._check_via_local_git(repo_dir) + + assert behind == 7 + assert baseline == banner.BASELINE_UPSTREAM + assert ["git", "fetch", "origin", "--quiet"] in calls + assert not any(c[:2] == ["git", "fetch"] and "myfork" in c for c in calls) + + +def test_fork_cache_carries_baseline_back(tmp_path, monkeypatch): + """A fork-baseline cache surfaces its behind count AND sets the fork baseline. + + The cache stores the baseline alongside behind; reading it back must set the + module baseline so the render picks the fork-appropriate remedy copy. + """ + import hermes_cli.banner as banner + + 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": 9, "rev": None, + "ver": banner.VERSION, "baseline": banner.BASELINE_FORK, + })) + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + + with patch("hermes_cli.config.detect_install_method", return_value="source"), \ + patch("hermes_cli.banner.subprocess.run") as mock_run: + result = banner.check_for_updates() + + assert result == 9 + assert banner.get_update_baseline() == banner.BASELINE_FORK + # Fresh cache hit -> no git probes. + mock_run.assert_not_called() + + +def test_check_for_updates_persists_fork_baseline(tmp_path, monkeypatch): + """check_for_updates writes the measured baseline into the cache.""" + import hermes_cli.banner as banner + + repo_dir = tmp_path / "hermes-agent" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + + cache_file = tmp_path / ".update_check" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.delenv("HERMES_REVISION", raising=False) + # Force the repo_dir resolution to land on HERMES_HOME/hermes-agent by + # pointing banner.__file__ at a location with no .git of its own. + fake_banner = tmp_path / "elsewhere" / "hermes_cli" / "banner.py" + fake_banner.parent.mkdir(parents=True, exist_ok=True) + fake_banner.touch() + monkeypatch.setattr(banner, "__file__", str(fake_banner)) + + with patch("hermes_cli.config.detect_install_method", return_value="source"), \ + patch("hermes_cli.banner._check_via_local_git", + return_value=(0, banner.BASELINE_FORK)) as mock_check: + result = banner.check_for_updates() + + assert result == 0 + mock_check.assert_called_once() + written = json.loads(cache_file.read_text()) + assert written["baseline"] == banner.BASELINE_FORK + assert banner.get_update_baseline() == banner.BASELINE_FORK