fix(updates): apply path must follow check-side fall-through past the latest tag
Fixes #2846. After PR #2758 (the #2653 fix) the update check correctly falls through to the branch comparison when HEAD has moved past the latest `v*` tag — so the banner reports the real commit count against `origin/<branch>`. But `_select_apply_compare_ref` was never updated to mirror that decision: as long as any `v*` tag exists, it returns `tags[0]`, even when HEAD is far past it. Result for everyone running hermes-agent past `v2026.5.16` (i.e. anyone on agent master between tagged releases): 1. Banner: `Agent (origin/main): 254 updates available` ← correct 2. User clicks Update Now 3. `_select_apply_compare_ref` picks `v2026.5.16` because tags exist 4. `git pull --ff-only origin v2026.5.16` — no-op (HEAD is already past it) 5. `_schedule_restart()` fires anyway, server bounces 6. Next check still reports 254 behind — banner reappears unchanged `apply_force_update` had the same bug, except worse: `git reset --hard v2026.5.16` would have actively rewound the user's checkout 254 commits. The root cause is the same bug class as #2653 — two parallel paths (`_check_repo_release` and `_select_apply_compare_ref`) that should make the same decision but didn't. Pre-fix, the "is HEAD past the latest tag?" predicate lived inline inside `_check_repo_release` only. Fix --- Extract `_head_is_past_latest_tag(path, current_tag)` and have both paths consult it. When HEAD is past the latest tag: - check path: release check returns None → branch check runs (#2653, unchanged behaviour, just refactored) - apply path: falls through to upstream / `origin/<branch>`, never the stale tag (#2846, new behaviour) Tests ----- - `test_select_apply_compare_ref_uses_tag_when_head_is_on_tag` — unchanged behaviour pinned: HEAD exactly on tag → advance to tag. - `test_select_apply_compare_ref_falls_through_when_head_is_past_tag` — the #2846 repro: HEAD = v2026.5.16 + 608 commits → advance to `origin/main`, not the tag. - `test_select_apply_compare_ref_no_tags_uses_upstream` — unchanged. - `test_select_apply_compare_ref_no_tags_no_upstream_uses_default_branch` — unchanged. - `test_check_and_apply_paths_agree_when_head_is_past_tag` — symmetry test, ensures the two paths can't drift apart again. All 21 tests in `tests/test_updates.py` pass locally (16 existing + 5 new). Refs #2846, #2653.
This commit is contained in:
nesquena-hermes
hermes-agent
parent
4d8a80b54f
commit
5d0d2bd0bf
2 changed files with 159 additions and 9 deletions
|
|
@ -351,6 +351,21 @@ def _release_gap(tags, current, latest):
|
|||
return 1
|
||||
|
||||
|
||||
def _head_is_past_latest_tag(path, current_tag):
|
||||
"""Return True when HEAD has moved past the latest reachable release tag.
|
||||
|
||||
`git describe --tags --always` returns the bare tag name (e.g. ``v2026.5.16``)
|
||||
when HEAD is exactly on the tag, and a ``v2026.5.16-608-g1d22b9c2`` suffix
|
||||
when HEAD has moved 608 commits past it. Used by both the update check and
|
||||
the update apply path so they agree on which ref to advance to — see #2653
|
||||
(check side) and #2846 (apply side).
|
||||
"""
|
||||
if not current_tag:
|
||||
return False
|
||||
full_desc, ok = _run_git(['describe', '--tags', '--always'], path)
|
||||
return bool(ok and full_desc and full_desc != current_tag)
|
||||
|
||||
|
||||
def _select_apply_compare_ref(path):
|
||||
"""Return the same remote ref family that the update check reports.
|
||||
|
||||
|
|
@ -358,10 +373,20 @@ def _select_apply_compare_ref(path):
|
|||
an update must therefore advance to the latest release tag too; otherwise a
|
||||
checkout on a local/fork tracking branch can report release updates, pull a
|
||||
different branch that is already current, restart, and still remain behind.
|
||||
|
||||
When HEAD is past the latest tag (the agent repo's day-to-day state between
|
||||
tagged releases), the check side falls through to the branch comparison via
|
||||
`_check_repo_release` returning None. The apply side must mirror that
|
||||
decision — otherwise we run `git pull --ff-only <latest-tag>` against a
|
||||
checkout that's already past the tag, no-op, restart, and the banner
|
||||
re-appears with the same N commits available. See #2846.
|
||||
"""
|
||||
tags = _release_tags(path)
|
||||
if tags:
|
||||
return tags[0]
|
||||
latest_tag = tags[0]
|
||||
# Mirror the predicate _check_repo_release uses to fall through.
|
||||
if not _head_is_past_latest_tag(path, latest_tag):
|
||||
return latest_tag
|
||||
|
||||
upstream, ok = _run_git(['rev-parse', '--abbrev-ref', '@{upstream}'], path)
|
||||
if ok and upstream:
|
||||
|
|
@ -384,14 +409,11 @@ def _check_repo_release(path, name):
|
|||
# If behind == 0 but HEAD has moved past the tag (e.g. the agent repo
|
||||
# keeps committing to master between tagged releases), the release check
|
||||
# would report "Up to date" even though hundreds of commits are missing.
|
||||
# Detect this by comparing the short describe output (which includes the
|
||||
# -N-gSHA suffix when HEAD is past a tag) against the bare tag name.
|
||||
# When HEAD is ahead of the latest tag, fall through to _check_repo_branch
|
||||
# so the real commit count is reported instead. See #2653.
|
||||
if behind == 0:
|
||||
full_desc, ok = _run_git(['describe', '--tags', '--always'], path)
|
||||
if ok and full_desc and full_desc != current_tag:
|
||||
return None
|
||||
# Fall through to _check_repo_branch so the real commit count is reported
|
||||
# instead. The same predicate is used by _select_apply_compare_ref so the
|
||||
# check and apply sides cannot drift again. See #2653 (check), #2846 (apply).
|
||||
if behind == 0 and _head_is_past_latest_tag(path, current_tag):
|
||||
return None
|
||||
|
||||
remote_url, _ = _run_git(['remote', 'get-url', 'origin'], path)
|
||||
remote_url = _normalize_remote_url(remote_url)
|
||||
|
|
|
|||
|
|
@ -419,3 +419,131 @@ def test_check_repo_branch_check_runs_for_post_tag_commits(tmp_path):
|
|||
assert info.get('release_based') is not True, (
|
||||
'post-tag HEAD should use branch check, not release-based check'
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression tests for #2846: _select_apply_compare_ref must mirror the
|
||||
# check-side decision about whether to advance to the latest tag or to the
|
||||
# upstream branch. Pre-fix, the check correctly fell through to the branch
|
||||
# count when HEAD was past the latest tag, but apply still aimed at the tag —
|
||||
# so clicking "Update Now" no-op'd, restarted the server, and the banner
|
||||
# re-appeared with the same N commits.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_select_apply_compare_ref_uses_tag_when_head_is_on_tag(tmp_path):
|
||||
"""HEAD == latest tag → apply path advances to the tag (unchanged)."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
|
||||
def fake_git(args, cwd, timeout=10):
|
||||
if args == ['tag', '--list', 'v*', '--sort=-v:refname']:
|
||||
return 'v2026.5.16\nv2026.5.10', True
|
||||
if args == ['describe', '--tags', '--always']:
|
||||
return 'v2026.5.16', True
|
||||
raise AssertionError(f'unexpected git args: {args!r}')
|
||||
|
||||
with patch.object(updates, '_run_git', side_effect=fake_git):
|
||||
ref = updates._select_apply_compare_ref(tmp_path)
|
||||
|
||||
assert ref == 'v2026.5.16'
|
||||
|
||||
|
||||
def test_select_apply_compare_ref_falls_through_when_head_is_past_tag(tmp_path):
|
||||
"""HEAD past latest tag → apply path advances to origin/<branch>, not the tag.
|
||||
|
||||
Mirrors the issue #2846 repro: hermes-agent has tag v2026.5.16, master is
|
||||
608 commits ahead, the banner correctly reports 608 commits available
|
||||
(post-#2758), but pre-fix apply ran `git pull --ff-only v2026.5.16` — a
|
||||
no-op — and the banner reappeared after restart.
|
||||
"""
|
||||
(tmp_path / '.git').mkdir()
|
||||
|
||||
def fake_git(args, cwd, timeout=10):
|
||||
if args == ['tag', '--list', 'v*', '--sort=-v:refname']:
|
||||
return 'v2026.5.16', True
|
||||
if args == ['describe', '--tags', '--always']:
|
||||
return 'v2026.5.16-608-g1d22b9c2d', True
|
||||
if args == ['rev-parse', '--abbrev-ref', '@{upstream}']:
|
||||
return 'origin/main', True
|
||||
raise AssertionError(f'unexpected git args: {args!r}')
|
||||
|
||||
with patch.object(updates, '_run_git', side_effect=fake_git):
|
||||
ref = updates._select_apply_compare_ref(tmp_path)
|
||||
|
||||
assert ref == 'origin/main', (
|
||||
'apply path must advance to the upstream branch when HEAD is past the '
|
||||
'latest tag, otherwise Update Now no-ops and the banner loops (#2846)'
|
||||
)
|
||||
|
||||
|
||||
def test_select_apply_compare_ref_no_tags_uses_upstream(tmp_path):
|
||||
"""No `v*` tags → apply path uses the configured upstream (unchanged)."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
|
||||
def fake_git(args, cwd, timeout=10):
|
||||
if args == ['tag', '--list', 'v*', '--sort=-v:refname']:
|
||||
return '', True
|
||||
if args == ['rev-parse', '--abbrev-ref', '@{upstream}']:
|
||||
return 'origin/feat/foo', True
|
||||
raise AssertionError(f'unexpected git args: {args!r}')
|
||||
|
||||
with patch.object(updates, '_run_git', side_effect=fake_git):
|
||||
ref = updates._select_apply_compare_ref(tmp_path)
|
||||
|
||||
assert ref == 'origin/feat/foo'
|
||||
|
||||
|
||||
def test_select_apply_compare_ref_no_tags_no_upstream_uses_default_branch(tmp_path):
|
||||
"""No tags and no upstream → fall back to origin/<default-branch>."""
|
||||
(tmp_path / '.git').mkdir()
|
||||
|
||||
def fake_git(args, cwd, timeout=10):
|
||||
if args == ['tag', '--list', 'v*', '--sort=-v:refname']:
|
||||
return '', True
|
||||
if args == ['rev-parse', '--abbrev-ref', '@{upstream}']:
|
||||
return '', False
|
||||
if args == ['symbolic-ref', 'refs/remotes/origin/HEAD']:
|
||||
return 'refs/remotes/origin/main', True
|
||||
raise AssertionError(f'unexpected git args: {args!r}')
|
||||
|
||||
with patch.object(updates, '_run_git', side_effect=fake_git):
|
||||
ref = updates._select_apply_compare_ref(tmp_path)
|
||||
|
||||
assert ref == 'origin/main'
|
||||
|
||||
|
||||
def test_check_and_apply_paths_agree_when_head_is_past_tag(tmp_path):
|
||||
"""Check and apply paths must agree: both fall through to origin/<branch>.
|
||||
|
||||
The bug class in #2846 (and #2653 before it) was the two paths drifting
|
||||
apart — check said "you're 608 behind origin/main", apply said "advance
|
||||
to v2026.5.16". This test pins the symmetry so they can't drift again.
|
||||
"""
|
||||
(tmp_path / '.git').mkdir()
|
||||
|
||||
def fake_git(args, cwd, timeout=10):
|
||||
if args == ['tag', '--list', 'v*', '--sort=-v:refname']:
|
||||
return 'v2026.5.16', True
|
||||
if args == ['describe', '--tags', '--abbrev=0']:
|
||||
return 'v2026.5.16', True
|
||||
if args == ['describe', '--tags', '--always']:
|
||||
return 'v2026.5.16-608-g1d22b9c2d', True
|
||||
if args == ['rev-parse', '--abbrev-ref', '@{upstream}']:
|
||||
return 'origin/main', True
|
||||
return '', True
|
||||
|
||||
with patch.object(updates, '_run_git', side_effect=fake_git):
|
||||
check_result = updates._check_repo_release(tmp_path, 'agent')
|
||||
apply_ref = updates._select_apply_compare_ref(tmp_path)
|
||||
|
||||
# Check side falls through (release check returns None → branch check runs)
|
||||
assert check_result is None, (
|
||||
'_check_repo_release should fall through when HEAD is past the latest '
|
||||
'tag (#2653)'
|
||||
)
|
||||
# Apply side picks the same branch the check would have reported against
|
||||
assert apply_ref == 'origin/main', (
|
||||
'_select_apply_compare_ref must mirror the check-side fall-through '
|
||||
'when HEAD is past the latest tag (#2846)'
|
||||
)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue