Skip to content

fix(cli): avoid bogus shallow update counts - #53494

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

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

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • avoid exact rev-list counts when a shallow checkout has no merge-base with the remote branch
  • reuse the same guarded count helper in both hermes update and hermes update --check
  • add CLI tests that assert shallow/no-merge-base repos skip rev-list --count and show a generic update message

Testing

  • uv run python --version
  • git diff --check
  • uv run --extra dev python -m pytest tests/hermes_cli/test_cmd_update.py -k "shallow_history or check_reports_generic_update_when_shallow_history_has_no_merge_base"
  • uv run --extra dev python -m pytest tests/hermes_cli/test_cmd_update.py
  • uv run --extra dev ruff check hermes_cli/main.py tests/hermes_cli/test_cmd_update.py

@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 labels Jun 27, 2026

@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 #53512 and #53498. Extracts a shared _get_remote_update_commit_count() helper that handles both shallow and non-shallow repos, and applies it to both cmd_update and cmd_update --check.

Note: Competing fix pair with #53512 and #53498. This is the most comprehensive of the three — it also fixes the --check path and extracts a reusable helper. The maintainer should reconcile which approach to merge.


Reviewed by Hermes Agent

@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 covering both updater entry points and adding regression intent. The apply-path issue is still present on current main, but this patch needs a re-scope before it can fix it safely.

Problems

  • hermes_cli/main.py:9595-9600 on current main fetches before the PR's proposed count helper would run. Merged #50784 (eecb5b9dd19a4234ebf64c45e5440d85c60a6696) establishes that shallow detection must occur before fetch and that shallow fetches need --depth 1; otherwise the checkout can be unshallowed before the guard evaluates it.
  • The --check portion is already implemented on main: hermes_cli/main.py:8486-8590 detects shallow state pre-fetch, preserves depth, compares tip SHAs, and skips rev-list.

Suggested changes

  • Salvage the apply-path fix by moving the shallow probe ahead of hermes_cli/main.py:9595 and using a depth-preserving fetch plus SHA presence check there.
  • Drop the duplicate --check changes and rebase the tests on the current updater layout.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py
"""
shallow_result = subprocess.run(
git_cmd + ["rev-parse", "--is-shallow-repository"],
cwd=cwd,

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 shallow probe is reached only after _cmd_update_impl has fetched from origin. The existing merged fix #50784 requires probing before fetch and using --depth 1; otherwise a plain fetch can unshallow the checkout before this guard runs, so the exact-count path remains reachable for the reported failure.

@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 15, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Closing — resolved by PR #86257 + #86318 (merged). Your _get_remote_update_commit_count design (shallow + no merge-base ⇒ generic update message, exact count otherwise) matches what landed for the --check path; the merged version extends it with exact-count recovery via the GitHub compare API so shallow installs usually still get a real number. 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 #86318's description alongside #53498. 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 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.

4 participants