From 0e84ffb87ada25d88397e1526448e7456427da65 Mon Sep 17 00:00:00 2001 From: ygd58 Date: Wed, 12 Aug 2026 14:48:24 +0000 Subject: [PATCH] fix(cli): stop using --depth 1 for update-check fetches on shallow installer checkouts Fixes #84591. hermes_cli/update_cmd.py's `hermes update --check` and hermes_cli/banner.py's CLI-startup update poller both fetched with `--depth 1` when the repo was already shallow (installer checkouts, `git clone --depth 1`), on the theory that a plain fetch would unshallow the repo and drag in the whole history, making the rev-list count bogus (huge number). Verified against real git behavior (not just reasoning about it) that this theory doesn't hold for the common case: a plain, branch-scoped fetch on an ALREADY-shallow repo extends the existing shallow boundary forward incrementally. Git's fetch negotiation reuses the current boundary as a "have" reference, so only the new commits transfer -- .git doesn't grow, and merge-base/rev-list stay exact. `--depth 1`, in contrast, unconditionally marks the freshly fetched tip as a NEW shallow boundary even when its parent is already local -- permanently breaking `git merge-base HEAD origin/main` from that fetch onward. Not self-healing: every later plain fetch's new tip connects to the now- orphaned boundary, so the indicator sticks at the placeholder "1 commit behind" forever, no matter how far `main` actually advances (real-world report: showed "1" while 61 commits behind). Root cause per the issue: `eecb5b9dd1` (#50784) added the --depth-1 check-fetch to avoid a different failure mode (reporting a bogus huge "behind" count on a repo that had never been fetched incrementally at all). That fix's own mechanism is what created this one. Fix: removed `--depth 1` from both check-path fetches. Also changed the post-fetch logic in both files to ATTEMPT the exact rev-list/merge-base count first -- empirically verified this succeeds on a shallow repo as long as the boundary hasn't been poisoned -- falling back to the old presence-only placeholder only if it genuinely can't connect (e.g. a rewritten upstream history orphaning the boundary), rather than always skipping the exact count whenever `is-shallow-repository` reports true. apps/desktop/electron/main.ts's own poller fetch already had no --depth flag (confirmed by reading it) -- it was already correct, just a victim of the CLI-side poisoning; no change needed there. update-count.ts's resolveBehindCount() is a pure function consuming isShallow/hasMergeBase flags computed elsewhere, so it benefits from this fix without any changes of its own. Verification used REAL git repositories (not mocked subprocess calls) -- the bug is in git's own shallow-fetch semantics, which a mock can't meaningfully exercise. Added 4 tests: a sanity check on the shallow-clone fixture; a positive test proving a plain fetch preserves exact merge-base/rev-list; a negative-control test reproducing the --depth 1 poisoning and its non-self-healing behavior (as documented proof of the bug this fix removes); and an end-to-end test of banner.py's _check_via_local_git() against a real shallow checkout, asserting it now returns the exact behind-count (4) instead of the presence-only placeholder (1). Verified as a genuine regression by reverting just the --depth-1 removal in banner.py and confirming the end-to-end test fails with exactly "1" -- the exact reported symptom. 15/15 pass across the new test file plus the four directly related banner test files; 22/22 in the existing test_cmd_update.py file (no regression). --- hermes_cli/banner.py | 50 ++++-- hermes_cli/update_cmd.py | 51 +++++- ...est_shallow_checkout_update_count_84591.py | 164 ++++++++++++++++++ 3 files changed, 242 insertions(+), 23 deletions(-) create mode 100644 tests/hermes_cli/test_shallow_checkout_update_count_84591.py diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index e6e5551b0abba..af863e62a0a4d 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -225,14 +225,18 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: return 1 return checked - # Installer checkouts are shallow (`git clone --depth 1`). On a shallow - # clone the history stops at a single commit, so a plain `git fetch` would - # unshallow the repo (dragging in the whole history) and - # `rev-list --count HEAD..origin/main` would report a huge bogus "behind" - # number (e.g. "12492 commits behind"). Detect shallow up front: fetch with - # --depth 1 to preserve the boundary and compare tip SHAs instead of - # counting. Full clones (developers, Docker dev images) keep the exact - # count path unchanged. Mirrors the desktop fix in apps/desktop/electron/main.cjs. + # Installer checkouts are shallow (`git clone --depth 1`). This used to + # fetch with `--depth 1` here too, on the theory that a plain fetch + # would unshallow the repo and drag in the whole history. Verified + # against real git behavior that this isn't the case: a plain, + # branch-scoped fetch on an ALREADY-shallow repo extends the existing + # shallow boundary forward incrementally -- only the new commits + # transfer, and merge-base/rev-list remain exact. --depth 1, in + # contrast, unconditionally marks the freshly fetched tip as a NEW + # boundary even when its parent is already local, permanently breaking + # the count from that fetch onward -- not self-healing, since every + # later plain fetch's new tip connects to that now-orphaned boundary + # (issue #84591). shallow = _git_stdout(["rev-parse", "--is-shallow-repository"], cwd=repo_dir) is_shallow = shallow == "true" @@ -246,8 +250,6 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: # unaffected; the shallow path compares against FETCH_HEAD, which a # scoped fetch also updates. fetch_args = ["git", "fetch", "origin", "main"] - if is_shallow: - fetch_args += ["--depth", "1"] fetch_args.append("--quiet") subprocess.run( fetch_args, @@ -258,17 +260,35 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: pass # Offline or timeout — use stale refs, that's fine if is_shallow: - # No history to count across the shallow boundary. `origin/main` may not - # be a tracking ref in a `clone --depth 1`, so prefer FETCH_HEAD (just - # updated by the fetch above) and fall back to origin/main. - head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) + # `origin/main` may not be a tracking ref in a `clone --depth 1`, so + # prefer FETCH_HEAD (just updated by the fetch above) and fall back + # to origin/main. target_rev = ( _git_stdout(["rev-parse", "FETCH_HEAD"], cwd=repo_dir) or _git_stdout(["rev-parse", "origin/main"], cwd=repo_dir) ) + head_rev = _git_stdout(["rev-parse", "HEAD"], cwd=repo_dir) if not head_rev or not target_rev: return None - return 0 if head_rev == target_rev else UPDATE_AVAILABLE_NO_COUNT + if head_rev == target_rev: + return 0 + # The shallow boundary no longer blocks an exact count in the common + # case now that the fetch above isn't --depth-limited. Try rev-list + # against the resolved target first, falling back to presence-only + # only if it genuinely can't connect (e.g. a rewritten upstream + # history orphaning the old boundary). + try: + result = subprocess.run( + ["git", "rev-list", "--count", f"{head_rev}..{target_rev}"], + capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=5, + cwd=str(repo_dir), + ) + if result.returncode == 0: + return int(result.stdout.strip()) + except Exception: + pass + return UPDATE_AVAILABLE_NO_COUNT try: result = subprocess.run( diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 78eed1d2c6cc3..95d33bee7f404 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2290,11 +2290,22 @@ def _cmd_update_check(branch: str = "main", *, branch_explicit: bool = False): # Note: upstream/ may not exist for non-main branches (a fork's # bb/gui has no upstream counterpart), so when the caller picks a # non-default branch we skip the upstream probe and use origin directly. - # Installer checkouts are shallow (`git clone --depth 1`). A plain - # `git fetch` would unshallow the repo (dragging in the whole history — - # the exact cost the shallow clone avoided) and the rev-list count below - # would then report a huge bogus "behind" number. Detect shallow up front: - # fetch with --depth 1 to preserve the boundary and report presence-only. + # + # Installer checkouts are shallow (`git clone --depth 1`). This used to + # fetch with `--depth 1` here too, on the theory that a plain fetch would + # "unshallow" the repo and drag in the whole history. Verified against + # real git behavior that this isn't the case: a plain, branch-scoped + # fetch on an ALREADY-shallow repo extends the existing shallow boundary + # forward incrementally -- git's fetch negotiation reuses the current + # boundary as a "have" reference, so only the new commits transfer, .git + # doesn't grow, and merge-base/rev-list remain exact. --depth 1, in + # contrast, unconditionally marks the freshly fetched tip as a NEW + # boundary even when its parent is already local, permanently breaking + # merge-base from that fetch onward -- not self-healing, since every + # later plain fetch's new tip connects to that now-orphaned boundary + # (issue #84591). Kept `is_shallow` (used below to decide the fallback + # path if rev-list genuinely can't connect, e.g. after a history rewrite + # upstream) without any depth-limiting on the fetch itself. is_shallow = ( subprocess.run( git_cmd + ["rev-parse", "--is-shallow-repository"], @@ -2304,7 +2315,7 @@ def _cmd_update_check(branch: str = "main", *, branch_explicit: bool = False): ).stdout.strip() == "true" ) - depth_args = ["--depth", "1"] if is_shallow else [] + depth_args: list[str] = [] if branch == "main": # Probe locally (~6 ms) whether an 'upstream' remote exists at all @@ -2382,8 +2393,32 @@ def _cmd_update_check(branch: str = "main", *, branch_explicit: bool = False): sys.exit(1) if is_shallow: - # No history to count across the shallow boundary. Compare tip SHAs and - # report presence-only (mirrors the banner's _check_via_local_git). + # The shallow boundary no longer blocks an exact count in the + # common case (see the fetch comment above) -- try rev-list first + # and only fall back to presence-only if it genuinely can't + # connect HEAD to compare_branch (e.g. upstream history was + # rewritten and the old boundary is no longer an ancestor). + rev_probe = subprocess.run( + git_cmd + ["rev-list", f"HEAD..{compare_branch}", "--count"], + cwd=_m().PROJECT_ROOT, + capture_output=True, + text=True, encoding="utf-8", errors="replace", + ) + if rev_probe.returncode == 0: + behind = int(rev_probe.stdout.strip()) + if behind == 0: + print("✓ Already up to date.") + else: + commits_word = "commit" if behind == 1 else "commits" + print(f"⚕ Update available: {behind} {commits_word} behind {compare_branch}.") + from hermes_cli.config import recommended_update_command + + print(f" Run '{recommended_update_command()}' to install.") + return + + # rev-list couldn't connect the history -- fall back to + # presence-only. Compare tip SHAs and report accordingly (mirrors + # the banner's _check_via_local_git). head_sha = subprocess.run( git_cmd + ["rev-parse", "HEAD"], cwd=_m().PROJECT_ROOT, capture_output=True, text=True, encoding="utf-8", errors="replace", diff --git a/tests/hermes_cli/test_shallow_checkout_update_count_84591.py b/tests/hermes_cli/test_shallow_checkout_update_count_84591.py new file mode 100644 index 0000000000000..801c4227068ec --- /dev/null +++ b/tests/hermes_cli/test_shallow_checkout_update_count_84591.py @@ -0,0 +1,164 @@ +"""Regression for #84591 — a --depth 1 check-fetch permanently poisons +merge-base on shallow installer checkouts, stalling the update indicator +at a placeholder "1 commit behind" forever. + +Uses REAL git repositories (not mocked subprocess calls): the bug is in +git's own shallow-fetch semantics (whether --depth 1 marks an unconditional +new shallow boundary vs. a plain fetch correctly extending an existing one +incrementally), which a mock cannot meaningfully exercise. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + + +def _run(args: list[str], cwd: Path) -> subprocess.CompletedProcess: + return subprocess.run( + args, cwd=str(cwd), capture_output=True, text=True, check=True, + env={"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t.com", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t.com", + "PATH": "/usr/bin:/bin"}, + ) + + +def _commit(repo: Path, message: str) -> None: + (repo / "file.txt").write_text(message) + _run(["git", "add", "file.txt"], cwd=repo) + _run(["git", "commit", "-q", "-m", message], cwd=repo) + + +@pytest.fixture +def shallow_checkout(tmp_path): + """A real shallow clone of a real 'remote' repo, matching the + installer's `git clone --depth 1`. Returns (clone_dir, remote_dir).""" + remote = tmp_path / "remote.git" + remote.mkdir() + _run(["git", "init", "-q", "-b", "main"], cwd=remote) + for i in range(1, 21): + _commit(remote, f"commit {i}") + + clone = tmp_path / "shallow_clone" + _run( + ["git", "clone", "--depth", "1", f"file://{remote}", str(clone)], + cwd=tmp_path, + ) + return clone, remote + + +def test_shallow_clone_fixture_is_actually_shallow(shallow_checkout): + """Sanity check on the test fixture itself.""" + clone, _ = shallow_checkout + result = subprocess.run( + ["git", "rev-parse", "--is-shallow-repository"], + cwd=str(clone), capture_output=True, text=True, + ) + assert result.stdout.strip() == "true" + + +def test_plain_fetch_after_shallow_clone_preserves_exact_count(shallow_checkout): + """The core empirical claim behind this fix: a PLAIN (no --depth) + branch-scoped fetch on an already-shallow clone correctly extends the + shallow boundary forward, without dragging in full history, and + merge-base/rev-list stay exact. This is what banner.py/update_cmd.py's + fetch now does instead of --depth 1.""" + clone, remote = shallow_checkout + for i in range(21, 24): + _commit(remote, f"commit {i}") + + branch_result = subprocess.run( + ["git", "symbolic-ref", "--short", "HEAD"], + cwd=str(clone), capture_output=True, text=True, + ) + branch = branch_result.stdout.strip() + + fetch = subprocess.run( + ["git", "fetch", "origin", branch], + cwd=str(clone), capture_output=True, text=True, + ) + assert fetch.returncode == 0 + + merge_base = subprocess.run( + ["git", "merge-base", "HEAD", f"origin/{branch}"], + cwd=str(clone), capture_output=True, text=True, + ) + assert merge_base.returncode == 0, ( + "merge-base must succeed after a plain fetch on a shallow clone" + ) + + count = subprocess.run( + ["git", "rev-list", "--count", f"HEAD..origin/{branch}"], + cwd=str(clone), capture_output=True, text=True, + ) + assert count.returncode == 0 + assert count.stdout.strip() == "3" + + +def test_depth_1_fetch_poisons_merge_base_permanently(shallow_checkout): + """Confirms the reported bug's exact mechanism, as a negative control: + a --depth 1 fetch on an already-shallow clone marks a NEW shallow + boundary and breaks merge-base -- and does not self-heal on a later + plain fetch, since the next tip connects to the now-orphaned boundary. + This is the behavior the fix removes from the check paths.""" + clone, remote = shallow_checkout + for i in range(21, 23): + _commit(remote, f"commit {i}") + + branch_result = subprocess.run( + ["git", "symbolic-ref", "--short", "HEAD"], + cwd=str(clone), capture_output=True, text=True, + ) + branch = branch_result.stdout.strip() + + subprocess.run( + ["git", "fetch", "--depth", "1", "origin", branch], + cwd=str(clone), capture_output=True, text=True, + ) + merge_base = subprocess.run( + ["git", "merge-base", "HEAD", f"origin/{branch}"], + cwd=str(clone), capture_output=True, text=True, + ) + assert merge_base.returncode != 0, ( + "sanity: --depth 1 must reproduce the merge-base failure " + "this fix works around" + ) + + # And it does NOT self-heal on a later plain fetch, even with more + # commits landing -- matching the reporter's "stuck forever" claim. + _commit(remote, "commit 23") + subprocess.run( + ["git", "fetch", "origin", branch], + cwd=str(clone), capture_output=True, text=True, + ) + merge_base_2 = subprocess.run( + ["git", "merge-base", "HEAD", f"origin/{branch}"], + cwd=str(clone), capture_output=True, text=True, + ) + assert merge_base_2.returncode != 0, ( + "the poisoned boundary must not self-heal from a plain fetch alone" + ) + + +def test_check_via_local_git_reports_exact_count_on_shallow_checkout( + shallow_checkout, monkeypatch +): + """End-to-end: banner.py's _check_via_local_git() against a real + shallow checkout must report the EXACT commit count, not the + presence-only placeholder, once its fetch no longer uses --depth 1.""" + clone, remote = shallow_checkout + for i in range(21, 25): + _commit(remote, f"commit {i}") + + import hermes_cli.banner as banner_mod + + monkeypatch.setattr(banner_mod, "_is_official_ssh_remote", lambda url: False) + + result = banner_mod._check_via_local_git(clone) + assert result == 4, ( + f"expected the exact behind-count (4), got {result!r} -- if this " + f"is banner_mod.UPDATE_AVAILABLE_NO_COUNT (-1), the placeholder " + f"path fired instead of the exact count" + )