Skip to content

fix(update): key update-check cache by current git head - #22118

Open
sunwz1115 wants to merge 1 commit into
NousResearch:mainfrom
sunwz1115:fix/update-check-cache-by-head-pr-forkbase
Open

sunwz1115 wants to merge 1 commit into
NousResearch:mainfrom
sunwz1115:fix/update-check-cache-by-head-pr-forkbase

Conversation

@sunwz1115

@sunwz1115 sunwz1115 commented May 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Invalidate the update-check cache when the local git HEAD changes.
  • Prevents stale behind counts after a local update/rebase in editable git checkouts.

Test Plan

  • tests/hermes_cli/test_update_check.py

Uploaded from LangLang production local patch after rebasing on the latest origin/main.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists labels May 8, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

Duplicate of #9670 / #20792. Multiple open PRs already address update-check cache invalidation when local HEAD moves (also #18236, #21675). This is the same fix approach — keying cache by git HEAD.

@sunwz1115
sunwz1115 force-pushed the fix/update-check-cache-by-head-pr-forkbase branch from bb91b24 to b0ccb85 Compare May 24, 2026 04:10

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused cache-invalidation fix. The on-disk stale-cache premise remains valid on current main: hermes_cli/banner.py:339-344 returns a fresh cache before local HEAD is examined.

Problems

  • The PR's cache_rev is not None condition (hermes_cli/banner.py:274 on the PR head) disables cache hits for non-git/PyPI installs. Current main retains those hits and keys them by VERSION (hermes_cli/banner.py:330-364, commit 689ef5e233980f5d5a32080e959f44c8991dd03a).
  • The PR predates that VERSION-keyed contract and removes ver; salvage should preserve it while adding the git-HEAD discriminator.
  • tests/hermes_cli/test_update_check.py:89 covers legacy rev: null, but not a fresh git:oldhead entry whose current HEAD becomes newhead.
  • Current _check_via_local_git() now probes remote get-url origin and shallow status (hermes_cli/banner.py:196,212), so the PR's narrower subprocess mocks need updating.

Suggested changes

  • Retain ver == VERSION for all installs and require matching HEAD only for local git checkouts.
  • Add the fresh old-HEAD/new-HEAD regression case and current git preflight mock responses.

Automated hermes-sweeper review.

Comment thread hermes_cli/banner.py
if (
now - cached.get("ts", 0) < _UPDATE_CHECK_CACHE_SECONDS
and cached.get("rev") == embedded_rev
cache_rev is not None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When neither HERMES_REVISION nor a local .git checkout exists, cache_rev stays None, so this rejects every otherwise-valid PyPI cache entry. Preserve current main's ver == VERSION cache path (689ef5e) and apply HEAD matching only to git installs.

fake_banner.parent.mkdir(parents=True, exist_ok=True)
fake_banner.touch()
cache_file = tmp_path / ".update_check"
cache_file.write_text(json.dumps({"ts": time.time(), "behind": 83, "rev": None}))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This verifies migration from the legacy rev: null schema, not the steady-state bug. Add a fresh rev: "git:oldhead" case with git rev-parse HEAD returning newhead, and assert it rechecks and rewrites the cache.

cache_file.write_text(json.dumps({"ts": 0, "behind": 1, "rev": "git:old"}))

mock_result = MagicMock(returncode=0, stdout="5\n")
def fake_run(cmd, **kwargs):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When salvaging onto current main, this mock must also handle git remote get-url origin and git rev-parse --is-shallow-repository; current _check_via_local_git() runs both before fetch/rev-list.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants