fix(cli): preserve stale positive behind-count on fetch failure (#92578)
huklaa's review: a failed fetch makes origin/main stale, so a stale ref cannot prove *currentness* (rev-list 0 is inconclusive), but a stale positive count is still sound evidence an update exists. On fetch failure, compute the stale behind-count and return it when > 0; otherwise return None (inconclusive) and still skip the cache write. Regression tests: - fetch failure + stale rev-list 0 -> None (not 'up to date') - fetch failure + stale rev-list 5 -> 5 (update evidence preserved) - fetch failure + rev-list error -> None
This commit is contained in:
committed by
Teknium
parent
55d50d5c92
commit
98e87ac886
@@ -349,12 +349,27 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]:
|
||||
except Exception:
|
||||
fetch_ok = False # Offline or timeout — don't use stale refs
|
||||
|
||||
# When the fetch fails, the local origin/main tracking ref is stale.
|
||||
# Comparing HEAD against it can report "0 behind" (up to date) even when
|
||||
# upstream has moved forward — the exact stale-cache symptom in #82166.
|
||||
# Return None so the caller knows the check was inconclusive and doesn't
|
||||
# cache a false "up to date".
|
||||
# When the fetch fails, the local origin/main tracking ref is stale. It
|
||||
# cannot prove *currentness* (a 0 behind-count may just mean the stale ref
|
||||
# hasn't caught up), but if it already shows HEAD behind, that is sound
|
||||
# evidence an update exists — the ref was good at some point in the past.
|
||||
# Return the positive stale count; return None (inconclusive) otherwise so
|
||||
# the caller doesn't cache a false "up to date". (#82166, review #92578)
|
||||
if not fetch_ok:
|
||||
if not is_shallow:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["git", "rev-list", "--count", "HEAD..origin/main"],
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace",
|
||||
timeout=5,
|
||||
cwd=str(repo_dir),
|
||||
)
|
||||
if result.returncode == 0:
|
||||
behind = int(result.stdout.strip())
|
||||
if behind > 0:
|
||||
return behind
|
||||
except Exception:
|
||||
pass
|
||||
return None
|
||||
|
||||
if is_shallow:
|
||||
|
||||
@@ -59,12 +59,12 @@ def test_prefetch_non_blocking():
|
||||
|
||||
|
||||
def test_check_via_local_git_fetch_failure_returns_none(tmp_path, monkeypatch):
|
||||
"""When git fetch fails, _check_via_local_git must return None instead of
|
||||
comparing against stale origin/main refs (#82166).
|
||||
"""When git fetch fails and the stale origin/main ref is not ahead,
|
||||
_check_via_local_git must return None (#82166).
|
||||
|
||||
Without this fix, a fetch failure silently falls through to
|
||||
``git rev-list --count HEAD..origin/main`` using the stale tracking ref,
|
||||
which can report 0 (up to date) even when upstream has moved forward.
|
||||
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
|
||||
|
||||
@@ -80,17 +80,109 @@ def test_check_via_local_git_fetch_failure_returns_none(tmp_path, monkeypatch):
|
||||
return "false"
|
||||
return None
|
||||
|
||||
# Simulate fetch failure (returncode != 0)
|
||||
# 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"
|
||||
|
||||
def mock_run(args, **kwargs):
|
||||
if args[:2] == ["git", "fetch"]:
|
||||
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", MagicMock(return_value=failed_proc))
|
||||
monkeypatch.setattr(banner.subprocess, "run", mock_run)
|
||||
|
||||
result = banner._check_via_local_git(repo_dir)
|
||||
assert result is None, "Fetch failure must return None, not a stale behind-count"
|
||||
assert result is None, (
|
||||
"Fetch failure with stale 0-behind must return None, not 'up to date'"
|
||||
)
|
||||
|
||||
|
||||
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):
|
||||
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):
|
||||
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):
|
||||
|
||||
Reference in New Issue
Block a user