mirror of
https://github.com/nesquena/hermes-webui.git
synced 2026-05-25 03:00:23 +00:00
fix(updates): use merge-base for compare URL so 'What's new?' link resolves
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 <compare_ref>` — 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.
This commit is contained in:
@@ -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)
|
||||
|
||||
+1
-1
@@ -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)
|
||||
|
||||
|
||||
+2
-2
@@ -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>/*
|
||||
|
||||
+29
-2
@@ -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
|
||||
|
||||
+17
-8
@@ -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';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
Reference in New Issue
Block a user