Skip to content

fix(update): avoid bogus shallow commit counts in cmd_update - #53498

Closed
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/cmd-update-shallow-count-53479
Closed

izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/cmd-update-shallow-count-53479

Conversation

@izumi0uu

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the hermes update execution path for shallow installer checkouts.

cmd_update() was still fetching without --depth 1 and always running git rev-list HEAD..origin/<branch> --count, even though the update banner and hermes update --check already treat shallow clones as a presence-only check. On a managed shallow checkout that could surface a bogus exact count before the pull.

This patch keeps shallow fetches shallow, compares HEAD and origin/<branch> by SHA in the shallow path, and prints a generic update-available message instead of a bogus exact commit count.

Related Issue

Fixes #53479

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • detect shallow Git checkouts in hermes_cli/main.py before fetching updates
  • preserve the shallow boundary with git fetch --depth 1 origin <branch> in cmd_update()
  • skip rev-list --count in the shallow cmd_update() path and fall back to SHA comparison
  • add shallow-clone regression coverage in tests/hermes_cli/test_update_autostash.py

How to Test

  1. Run ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py tests/hermes_cli/test_update_check.py -q
  2. Run ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py -k shallow_clone -q
  3. In a shallow checkout, run hermes update and confirm it prints a generic update-available message instead of a bogus exact commit count.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • N/A — no documentation changes needed
  • N/A — no cli-config.yaml.example changes needed
  • N/A — no CONTRIBUTING.md or AGENTS.md changes needed
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • N/A — no tool descriptions/schemas changed

Screenshots / Logs

  • Validation run: ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py tests/hermes_cli/test_update_check.py -q (50 passed)
  • Validation run: ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py -k shallow_clone -q (2 passed)
  • Not yet run manually end-to-end inside a real shallow checkout; this PR adds regression coverage for the source path and keeps the shallow behavior aligned with the already-fixed banner / hermes update --check paths.

Match the cmd_update path to the shallow-clone guard already used by the update banner and --check so installer clones stop printing bogus exact counts before pulling.

Constraint: Preserve exact commit counts for full clones while matching the existing shallow-clone behavior in banner and --check.
Rejected: share the desktop update-count helper | the CLI path already has the needed git probes and a local guard keeps the patch narrower.
Confidence: high
Scope-risk: narrow
Directive: Keep cmd_update's shallow fetch and SHA-compare path aligned with hermes update --check when changing update-count behavior again.
Tested: ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py tests/hermes_cli/test_update_check.py -q
Tested: ./.venv/bin/pytest tests/hermes_cli/test_update_autostash.py -k shallow_clone -q
Not-tested: Manual end-to-end hermes update run inside a real shallow checkout
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 27, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #53494 — same CLI fix in hermes_cli/main.py for the shallow rev-list --count bug (#53479). #53494 is the earlier open PR (canonical). Both belong to the merged #50784 family.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment

Addresses the same shallow clone bogus commit count issue as PR #53512 and #53494, with an additional improvement: adds --depth 1 for shallow fetches to prevent unshallowing the repo before deciding whether an exact count is trustworthy.

Note: Competing fix pair with #53512 and #53494. This PR uniquely adds the shallow fetch depth preservation. The maintainer should reconcile which approach to merge.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression coverage. Current main still has the reported apply-path gap: hermes_cli/main.py:9594-9600 performs an ordinary fetch, and hermes_cli/main.py:9683-9691 unconditionally derives an exact count with git rev-list. The existing shallow handling at hermes_cli/main.py:8481-8575 is confined to hermes update --check, matching the scope recorded in #50784 (eecb5b9dd19a).

No blocking problems found in static review. The PR's shallow fetch and presence-only SHA comparison align the apply path with the existing check-path behavior, while preserving exact counts for non-shallow repositories.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/install-update Installer, updater, packaging, wheels, doctor labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Closing — resolved by PR #86318 (merged, d8d7cc0). Your PR was the earliest fix for this exact site (the apply path's unconditional rev-list --count) and its shallow-gate design is what landed: detect shallow state, never trust the raw count. Your branch targeted main.py before the update code moved to update_cmd.py, so the commit no longer applied directly — but you're credited in the merged PR's description as the first submitter. The merged version adds exact-count recovery via the GitHub compare API on top of the presence-only fallback. Thanks!

@teknium1 teknium1 closed this Aug 14, 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-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

CLI updater still trusts rev-list counts for shallow/diverged installs

4 participants