From 3369a08f37ee80c48c8fb8c8cfdc2f9d62f697f5 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 4 May 2026 05:08:00 +0000 Subject: [PATCH] fix(updates): use merge-base for compare URL so 'What's new?' link resolves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1579. 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 local HEAD diverges from upstream — unpushed work, dirty stage branches, forks, in-flight rebases, release-time merge commits whose SHA only lives in the maintainer's local history — the compare URL points at a SHA github.com has never seen and returns the standard 404 page. Reporter (@ai-ag2026) observed: https://github.com/nesquena/hermes-webui/compare/c660c7f...86cb22e → 404 because c660c7f was an unpushed local commit. The right base is `git merge-base HEAD ` — the most recent commit local and upstream share. Since `git fetch` succeeded just before, the merge-base is guaranteed to exist on the upstream GitHub repo. Behavior matrix: Pure-behind clone (no local commits): merge-base == HEAD; URL unchanged. Behind + local-only commits (#1579): merge-base != HEAD; URL points at public ancestor instead of local HEAD. merge-base failure (shallow clone): current_sha=None; JS link guard suppresses link rather than emitting a known-broken URL. Also hardens static/ui.js: reset the link's href and display:none on every banner render, so a stale link from a prior render can't survive a re-render where the new payload has current_sha=null. Tests: - test_current_sha_is_merge_base_not_local_HEAD — reporter's scenario - test_current_sha_equals_HEAD_when_no_local_commits — backward compat - test_current_sha_falls_back_to_None_when_merge_base_fails — defensive - test_whats_new_link_resets_display_and_href_on_every_render - test_whats_new_link_suppressed_when_curSha_falsy - test_reporter_url_shape_no_longer_produces_invalid_compare_url 4094 → 4100 passing. 0 regressions. --- CHANGELOG.md | 17 ++ ROADMAP.md | 2 +- TESTING.md | 4 +- api/updates.py | 31 ++- static/ui.js | 25 +- tests/test_issue1579_whats_new_link_404.py | 251 +++++++++++++++++++++ 6 files changed, 317 insertions(+), 13 deletions(-) create mode 100644 tests/test_issue1579_whats_new_link_404.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b000b07a..36121fa5 100644 --- a/CHANGELOG.md +++ b/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 ` 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) diff --git a/ROADMAP.md b/ROADMAP.md index c763f8fa..49db253a 100644 --- a/ROADMAP.md +++ b/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) diff --git a/TESTING.md b/TESTING.md index 19c40109..a1726daf 100644 --- a/TESTING.md +++ b/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: /* diff --git a/api/updates.py b/api/updates.py index 77c1aca3..ecc976fd 100644 --- a/api/updates.py +++ b/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/... and 404'd because + # 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 diff --git a/static/ui.js b/static/ui.js index ade93b0e..1f3f2571 100644 --- a/static/ui.js +++ b/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'; + } } } } diff --git a/tests/test_issue1579_whats_new_link_404.py b/tests/test_issue1579_whats_new_link_404.py new file mode 100644 index 00000000..6f8bb7db --- /dev/null +++ b/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//compare/... returns + GitHub's 404 page because is not a public commit. + +Fix: + Use `git merge-base HEAD ` 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).