From 5d0d2bd0bfd6271a86be2a59b7ae0c5ea742411a Mon Sep 17 00:00:00 2001 From: nesquena-hermes <[email protected]> Date: Sun, 24 May 2026 16:54:49 +0000 Subject: [PATCH] fix(updates): apply path must follow check-side fall-through past the latest tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/`. 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/`, 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. --- api/updates.py | 40 ++++++++++--- tests/test_updates.py | 128 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 9 deletions(-) diff --git a/api/updates.py b/api/updates.py index 2df4baf0..cc1fb53c 100644 --- a/api/updates.py +++ b/api/updates.py @@ -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 ` 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) diff --git a/tests/test_updates.py b/tests/test_updates.py index 94c29e34..76da7593 100644 --- a/tests/test_updates.py +++ b/tests/test_updates.py @@ -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/, 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/.""" + (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/. + + 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)' + ) +