Merge pull request #1596 from nesquena/stage-291

Release v0.50.291 — 'What's new?' link 404 fix (closes #1579)

CHANGELOG.md

@@ -1,5 +1,21 @@
 # Hermes Web UI -- Changelog
 
+## [v0.50.291] — 2026-05-04
+
+### Fixed (1 PR — "What's new?" link 404 — closes #1579)
+
+- **"What's new?" update-banner link no longer 404s when local HEAD diverges from upstream** (closes #1579, reported by @ai-ag2026) — `api/updates.py` was building the GitHub compare URL from local-`HEAD` short SHA: `repoUrl + '/compare/' + curSha + '...' + newSha` where `curSha = git rev-parse --short HEAD`. Whenever the local checkout had commits that weren't in the upstream repo — unpushed work, dirty stage branches, forks, in-flight rebases, release-time merge commits — the compare URL pointed at a SHA that github.com had never seen and returned its standard 404 page. Reporter saw `https://github.com/nesquena/hermes-webui/compare/c660c7f...86cb22e` produce a 404 because `c660c7f` was an unpushed local commit. **Fix:** replace `git rev-parse --short HEAD` with `git merge-base HEAD <compare_ref>` then `git rev-parse --short` on that result. The merge-base is the most recent commit both local and upstream share, and (since `git fetch` succeeded just before) is guaranteed to exist on the upstream GitHub repo. For the common case (pure-behind clone, no local commits) the merge-base equals local HEAD and the URL is unchanged from prior behavior. For the divergent case (the #1579 reporter scenario) the URL points at the public ancestor, which github.com always knows. If `merge-base` itself fails (shallow clone with no shared history), fall back to `current_sha=None` so the existing JS link guard (`if(repoUrl && curSha && newSha)`) suppresses the link entirely rather than emitting a known-broken URL. Also hardens `static/ui.js` to **clear** the link's `href` and `display:none` it on every banner render, so a stale link from a prior render can't survive a re-render where the new payload's `current_sha` is null. 6 regression tests covering merge-base correctness, backward-compat for pure-behind clones, merge-base-failure fallback, JS link reset on every render, JS conditional guard shape, and an end-to-end verification of the reporter's exact scenario.
+
+### Tests
+
+4111 → **4117 passing** (+6 regression tests on `tests/test_issue1579_whats_new_link_404.py`). 0 regressions. Full suite in ~115s.
+
+### Pre-release verification
+
+- Self-built fix (nesquena-hermes) with **independent review APPROVED by nesquena** — full end-to-end behavioral harness using throwaway local+upstream git fixtures verified the reporter's exact scenario produces a 404 pre-fix and resolves post-fix. Cross-tool audit (webui-only, no agent surface). Security audit clean. Race/state analysis: `_check_repo` is single-threaded per request, `_run_git` spawns subprocesses with no shared state. Edge-case trace covered 8 scenarios including pure-behind clone (URL unchanged from pre-fix), 2-unpushed-3-upstream (the reporter's case), pure-ahead, fork checkout, mid-rebase, shallow clone, transient `git merge-base` errors, and stale link from prior render with null current_sha.
+- Bug repro confirmed locally: simulated 2 unpushed commits + 3 upstream commits; `git rev-parse --short HEAD` returns SHA absent from upstream history (verifiable with `git cat-file -e $sha origin/master` failing); `git merge-base HEAD origin/master` returns SHA present in upstream history. Compare URL constructed from merge-base resolves on github.com; URL constructed from local HEAD 404s.
+- All other tests in `test_update_checker.py` (12) and `test_version_badge.py` (21) still pass — no behavioral changes to the diagnostic / version-detection paths.
+
 ## [v0.50.290] — 2026-05-04
 
 ### Fixed + Feature (5-PR batch — login cache + sidebar UX + workspace dropdown polish)
@@ -41,6 +57,7 @@ Two stale source-string assertions were broken by #1591's compact() and messages
 - **Auto-fix on #1464:** ternary inversion + regression test, with `Co-authored-by: Josh Jameson` preserved.
 - **Auto-fix on stage:** widened source-string anchors in two pre-existing brittle tests broken by #1591's structural changes.
 
+
 ## [v0.50.289] — 2026-05-03
 
 ### Fixed (1 PR — TCP keepalive on accepted connections — closes #1580)

ROADMAP.md

@@ -2,7 +2,7 @@
 
 > Web companion to the Hermes Agent CLI. Same workflows, browser-native.
 >
-> Last updated: v0.50.290 (May 04, 2026) — 4111 tests collected
+> Last updated: v0.50.291 (May 04, 2026) — 4117 tests collected
 > Test source: `pytest tests/ --collect-only -q`
 > Per-version detail: see [CHANGELOG.md](./CHANGELOG.md)
 

TESTING.md

@@ -1835,8 +1835,8 @@ Bridged CLI sessions:
 
 ---
 
-*Last updated: v0.50.290, May 04, 2026*
-*Total automated tests collected: 4111*
+*Last updated: v0.50.291, May 04, 2026*
+*Total automated tests collected: 4117*
 *Regression gate: tests/test_regressions.py*
 *Run: pytest tests/ -v --timeout=60*
 *Source: <repo>/*

api/updates.py

@@ -172,8 +172,35 @@ def _check_repo(path, name):
     out, ok = _run_git(['rev-list', '--count', f'HEAD..{compare_ref}'], path)
     behind = int(out) if ok and out.isdigit() else 0
 
-    # Get short SHAs for display
-    current, _ = _run_git(['rev-parse', '--short', 'HEAD'], path)
+    # Get short SHAs for display.
+    #
+    # latest_sha = upstream tip (compare_ref). Always exists on github.com
+    # because it is literally the commit `git fetch` just pulled.
+    #
+    # current_sha is trickier. The intuitive choice — local HEAD — breaks
+    # the "What's new?" compare URL whenever HEAD is not a public commit:
+    # unpushed work, dirty stage branches, forks, in-flight rebases, or
+    # release-time merge commits whose SHA only lives in the maintainer's
+    # checkout. We saw exactly this in #1579: a banner reporting "17 updates"
+    # linked to /compare/<localHEAD>...<upstream> and 404'd because <localHEAD>
+    # was never pushed to the canonical repo.
+    #
+    # The right base is the merge-base between HEAD and the upstream ref —
+    # that's the most recent commit both sides agree on, and (because
+    # `git fetch` succeeded above) it is guaranteed to be present upstream.
+    # If a user is 17 commits behind with no local-only commits, merge-base
+    # equals local HEAD and the URL is identical to what we shipped before;
+    # if they ARE ahead with local-only commits, the URL still resolves to
+    # the public history they share with upstream. If merge-base fails for
+    # any reason (e.g. shallow clone where the bases diverge before the
+    # cutoff), fall back to None so the JS link guard suppresses the link
+    # rather than emitting a known-broken URL.
+    mb_full, mb_ok = _run_git(['merge-base', 'HEAD', compare_ref], path)
+    if mb_ok and mb_full:
+        short, ok = _run_git(['rev-parse', '--short', mb_full], path)
+        current = short if (ok and short) else None
+    else:
+        current = None
     latest, _ = _run_git(['rev-parse', '--short', compare_ref], path)
 
     # Get repo URL for "What's new?" link

static/ui.js

@@ -2854,15 +2854,24 @@ function _showUpdateBanner(data){
   const banner=$('updateBanner');
   if(banner) banner.classList.add('visible');
   window._updateData=data;
-  // Wire up "What's new?" link
+  // Wire up "What's new?" link.
+  //
+  // Reset display:none + clear the href on every render — otherwise a stale
+  // link from a prior update banner can stay visible after we've moved past
+  // a state where the new payload no longer carries usable SHAs (#1579 case
+  // when the local HEAD diverges from upstream and the compare URL would 404).
   const link=$('updateWhatsNew');
-  if(link && data.webui){
-    const repoUrl=data.webui.repo_url;
-    const curSha=data.webui.current_sha;
-    const newSha=data.webui.latest_sha;
-    if(repoUrl && curSha && newSha){
-      link.href=repoUrl+'/compare/'+curSha+'...'+newSha;
-      link.style.display='inline';
+  if(link){
+    link.style.display='none';
+    link.removeAttribute('href');
+    if(data.webui){
+      const repoUrl=data.webui.repo_url;
+      const curSha=data.webui.current_sha;
+      const newSha=data.webui.latest_sha;
+      if(repoUrl && curSha && newSha){
+        link.href=repoUrl+'/compare/'+curSha+'...'+newSha;
+        link.style.display='inline';
+      }
     }
   }
 }

tests/test_issue1579_whats_new_link_404.py

@@ -0,0 +1,251 @@
+"""Tests for issue #1579: What's new link can open a 404 GitHub compare page.
+
+Bug shape:
+  api/updates.py shipped current_sha=local-HEAD-short. When the local HEAD
+  is not present upstream (unpushed work, dirty stage, fork, in-flight
+  rebase, release-time merge commit), the resulting compare URL
+  https://github.com/<repo>/compare/<localHEAD>...<upstream> returns
+  GitHub's 404 page because <localHEAD> is not a public commit.
+
+Fix:
+  Use `git merge-base HEAD <compare_ref>` instead of `git rev-parse HEAD`.
+  merge-base is the most recent commit both local and upstream agree on,
+  and (since `git fetch` succeeded just before) it is guaranteed to exist
+  in the upstream GitHub repo. If merge-base fails (shallow clone with
+  divergent histories), fall back to current_sha=None — the JS link guard
+  suppresses the link rather than emitting a known-broken URL.
+"""
+
+import os
+import re
+import subprocess
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+REPO_ROOT = Path(__file__).resolve().parent.parent
+sys.path.insert(0, str(REPO_ROOT))
+
+
+# ── 1. Server-side: api.updates._check_repo uses merge-base, not HEAD ──
+
+def _make_throwaway_repo(tmp_path, *, local_only_commits=0, upstream_advanced=0):
+    """Create a tiny git repo with a fake 'origin' remote.
+
+    Returns the local clone path. Set local_only_commits>0 to put commits
+    on local HEAD that don't exist on origin (the #1579 trigger). Set
+    upstream_advanced>0 to make the remote ahead.
+    """
+    upstream = tmp_path / 'upstream.git'
+    subprocess.run(['git', 'init', '--quiet', '--bare', str(upstream)], check=True)
+
+    seed = tmp_path / 'seed'
+    subprocess.run(['git', 'init', '--quiet', '--initial-branch=master', str(seed)], check=True)
+    for cmd in [
+        ['git', '-C', str(seed), 'config', 'user.email', 'test@test.test'],
+        ['git', '-C', str(seed), 'config', 'user.name', 'test'],
+        ['git', '-C', str(seed), 'commit', '--allow-empty', '-m', 'initial', '--quiet'],
+        ['git', '-C', str(seed), 'remote', 'add', 'origin', str(upstream)],
+        ['git', '-C', str(seed), 'push', '--quiet', '-u', 'origin', 'master'],
+    ]:
+        subprocess.run(cmd, check=True)
+
+    # Clone FIRST — local and upstream share the initial commit only.
+    local = tmp_path / 'local'
+    subprocess.run(['git', 'clone', '--quiet', str(upstream), str(local)], check=True)
+    subprocess.run(['git', '-C', str(local), 'config', 'user.email', 'test@test.test'], check=True)
+    subprocess.run(['git', '-C', str(local), 'config', 'user.name', 'test'], check=True)
+
+    # Add local-only commits to the local clone (the #1579 trigger). These never
+    # get pushed — they exist only on the local clone's master branch.
+    for i in range(local_only_commits):
+        subprocess.run(['git', '-C', str(local), 'commit', '--allow-empty',
+                        '-m', f'local-only commit {i}', '--quiet'], check=True)
+
+    # Advance upstream by committing on the seed and pushing — so local clone
+    # is now `upstream_advanced` commits behind on the remote-tracking branch.
+    for i in range(upstream_advanced):
+        subprocess.run(['git', '-C', str(seed), 'commit', '--allow-empty',
+                        '-m', f'upstream commit {i}', '--quiet'], check=True)
+    if upstream_advanced:
+        subprocess.run(['git', '-C', str(seed), 'push', '--quiet'], check=True)
+
+    return local
+
+
+def _short_sha(repo, ref):
+    out = subprocess.run(['git', '-C', str(repo), 'rev-parse', '--short', ref],
+                         capture_output=True, text=True, check=True)
+    return out.stdout.strip()
+
+
+def test_current_sha_is_merge_base_not_local_HEAD(tmp_path, monkeypatch):
+    """Reporter's exact scenario: local has unpushed commits, upstream advanced.
+
+    Before #1579 fix: current_sha = local HEAD = unpublished SHA → URL 404s.
+    After fix: current_sha = merge-base = the public ancestor commit → URL resolves.
+    """
+    # Clear cached config (api.updates may import HERMES_HOME at import time)
+    repo = _make_throwaway_repo(
+        tmp_path, local_only_commits=2, upstream_advanced=3,
+    )
+
+    head_sha = _short_sha(repo, 'HEAD')
+    expected_base = _short_sha(repo, 'HEAD~2')  # merge-base in this scenario
+
+    # Import updates with a stable CWD
+    if 'api.updates' in sys.modules:
+        del sys.modules['api.updates']
+    from api import updates as upd
+
+    result = upd._check_repo(repo, 'webui')
+
+    assert result is not None, "non-bare repo with origin should return a result"
+    assert result['behind'] == 3, f"expected 3 commits behind, got {result['behind']}"
+
+    # The core fix: current_sha must be the merge-base, not local HEAD.
+    # merge-base = HEAD~2 in this scenario (local has 2 unpushed commits,
+    # so the most recent shared point with upstream is 2 commits before HEAD).
+    assert result['current_sha'] == expected_base, (
+        f"current_sha should be merge-base ({expected_base}), got {result['current_sha']} "
+        f"(local HEAD is {head_sha}). Old #1579 bug regressed."
+    )
+    assert result['current_sha'] != head_sha, (
+        f"current_sha must NOT be local HEAD ({head_sha}) — that's the #1579 bug."
+    )
+    # latest_sha is what _check_repo's own fetch+rev-parse returns
+    assert result['latest_sha'], "latest_sha must be populated"
+    # Critical compare-URL property: current_sha and latest_sha both correspond
+    # to commits the upstream knows about (one by being upstream tip, the other
+    # by being a shared ancestor). The merge-base is verifiable via the local
+    # clone's remote-tracking branch:
+    upstream_history = subprocess.run(
+        ['git', '-C', str(repo), 'log', '--format=%h', 'origin/master'],
+        capture_output=True, text=True, check=True,
+    ).stdout.split()
+    assert result['current_sha'] in upstream_history or any(
+        h.startswith(result['current_sha']) for h in upstream_history
+    ), (
+        f"current_sha ({result['current_sha']}) must be present in upstream history "
+        f"— that's what guarantees the GitHub /compare/ URL won't 404."
+    )
+
+
+def test_current_sha_equals_HEAD_when_no_local_commits(tmp_path):
+    """Backward-compat: pure-behind clone (no local-only commits) is unchanged.
+
+    merge-base equals HEAD in this case — so the URL is identical to what
+    we shipped before #1579.
+    """
+    repo = _make_throwaway_repo(tmp_path, local_only_commits=0, upstream_advanced=4)
+    if 'api.updates' in sys.modules:
+        del sys.modules['api.updates']
+    from api import updates as upd
+    result = upd._check_repo(repo, 'webui')
+
+    head_sha = _short_sha(repo, 'HEAD')
+    assert result['current_sha'] == head_sha, (
+        "Pure-behind clone: merge-base equals HEAD; URL should be unchanged "
+        "from pre-#1579 behavior."
+    )
+    assert result['behind'] == 4
+
+
+def test_current_sha_falls_back_to_None_when_merge_base_fails(tmp_path):
+    """Defensive: if merge-base errors (shallow clone, no shared history),
+    return current_sha=None so the JS link guard suppresses the bad link
+    rather than emitting one that 404s.
+    """
+    repo = _make_throwaway_repo(tmp_path, local_only_commits=0, upstream_advanced=1)
+    if 'api.updates' in sys.modules:
+        del sys.modules['api.updates']
+    from api import updates as upd
+
+    # Patch _run_git so any 'merge-base' call returns failure
+    real_run = upd._run_git
+
+    def fake_run(args, *a, **kw):
+        if args and args[0] == 'merge-base':
+            return ('', False)
+        return real_run(args, *a, **kw)
+
+    with patch.object(upd, '_run_git', side_effect=fake_run):
+        result = upd._check_repo(repo, 'webui')
+
+    assert result is not None
+    assert result['current_sha'] is None, (
+        "merge-base failure must fall back to None so JS suppresses the link "
+        "(emitting a known-broken URL is worse than no link)."
+    )
+    # latest_sha should still be populated — that path doesn't depend on merge-base
+    assert result['latest_sha']
+
+
+# ── 2. Client-side: ui.js link guard suppresses URL on null current_sha ──
+
+def _read_ui_js():
+    return (REPO_ROOT / 'static' / 'ui.js').read_text(encoding='utf-8')
+
+
+def test_whats_new_link_resets_display_and_href_on_every_render():
+    """Without reset, a stale link from a prior banner can stay visible after
+    a re-render where the new payload has current_sha=None.
+    """
+    src = _read_ui_js()
+    # Find the "What's new" wiring block (~50-line window)
+    idx = src.find("Wire up \"What's new?\" link")
+    assert idx != -1, "What's-new link wiring block not found"
+    block = src[idx:idx + 800]
+
+    # Reset must happen BEFORE the conditional href set
+    reset_idx = block.find("style.display='none'")
+    set_idx = block.find("style.display='inline'")
+    href_clear_idx = block.find("removeAttribute('href')")
+    href_set_idx = block.find("link.href=repoUrl")
+
+    assert reset_idx != -1, "Missing display='none' reset on every render"
+    assert href_clear_idx != -1, "Missing removeAttribute('href') reset on every render"
+    assert reset_idx < set_idx, "display reset must precede inline set"
+    assert href_clear_idx < href_set_idx, "href clear must precede href assignment"
+
+
+def test_whats_new_link_suppressed_when_curSha_falsy():
+    """The conditional must guard on all three of repoUrl/curSha/newSha."""
+    src = _read_ui_js()
+    idx = src.find("Wire up \"What's new?\" link")
+    block = src[idx:idx + 800]
+    # Match "if(repoUrl && curSha && newSha)" with arbitrary whitespace
+    pattern = re.compile(r'if\s*\(\s*repoUrl\s*&&\s*curSha\s*&&\s*newSha\s*\)')
+    assert pattern.search(block), (
+        "Link must require all three of repoUrl, curSha, newSha to be truthy. "
+        "If any is null/empty, link stays display:none."
+    )
+
+
+# ── 3. End-to-end: simulate the exact reporter URL shape ──
+
+def test_reporter_url_shape_no_longer_produces_invalid_compare_url(tmp_path):
+    """Reporter saw https://github.com/.../compare/c660c7f...86cb22e where
+    c660c7f was an unpublished local SHA. After fix, the URL should use
+    a SHA that exists upstream.
+    """
+    repo = _make_throwaway_repo(tmp_path, local_only_commits=2, upstream_advanced=5)
+    if 'api.updates' in sys.modules:
+        del sys.modules['api.updates']
+    from api import updates as upd
+    result = upd._check_repo(repo, 'webui')
+
+    head_sha = _short_sha(repo, 'HEAD')
+    base_sha = _short_sha(repo, 'HEAD~2')  # the merge-base
+
+    # The compare URL the JS would build
+    cur, latest = result['current_sha'], result['latest_sha']
+    # In a real run repo_url is converted from origin's URL; in this test the
+    # value will be a file:// path, but that's fine — what we care about is
+    # the cur and latest shas.
+    assert cur == base_sha
+    assert cur != head_sha, "Must not use local HEAD (the #1579 reporter URL bug)"
+
+    # The "merge-base...upstream-tip" URL is by construction valid because
+    # both endpoints exist on the upstream (one by being the upstream tip,
+    # the other by being a shared ancestor of upstream and local).