Skip to content

fix(desktop): don't report a bogus update count for a shallow checkout - #51979

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/desktop-shallow-update-count-51922
Closed

fix(desktop): don't report a bogus update count for a shallow checkout#51979
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/desktop-shallow-update-count-51922

Conversation

@briandevans

@briandevans briandevans commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

A public desktop install is cloned with --depth 1, so its local history often shares no merge-base with the freshly fetched origin tip. In that state git rev-list HEAD..origin/<branch> --count enumerates the entire remote ancestry and returns a meaningless huge number, which the update indicator renders as e.g. v0.17.0 (+12104) (#51922).

This mirrors the shallow-guard already present elsewhere in the codebase and brings the passive desktop update-check count path to parity:

  • the official-SSH branch of checkUpdates() already avoids the count entirely with a binary SHA check (behind: currentSha === targetSha ? 0 : 1), and
  • hermes_cli/banner.py (count_commits_behind) already detects shallow checkouts up front to avoid the same bogus number for the CLI banner.

The non-SSH desktop count path was the one place the guard was missing. It now detects shallow + no-merge-base up front and falls back to the same SHA-based binary check; full clones (developers / Docker dev images) keep the exact count path unchanged.

Audited siblings: the SSH-remote branch of checkUpdates and hermes_cli/banner.py already guard shallow/no-merge-base; this PR brings the non-SSH desktop count path to parity. No further widening needed.

Related Issue

Fixes #51922

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/electron/update-count.cjs (new): pure resolveBehindCount() helper — returns the exact rev-list count for full clones, but for a shallow checkout with no merge-base falls back to a binary 0/1 by comparing HEAD vs target SHA. Extracted so the logic is unit-testable without booting Electron (matches the existing update-remote.cjs / update-marker.cjs convention).
  • apps/desktop/electron/main.cjs: in the non-SSH branch of checkUpdates(), also fetch git rev-parse --is-shallow-repository and git merge-base HEAD origin/<branch>, and compute behind via the new helper.
  • apps/desktop/electron/update-count.test.cjs (new): node:test coverage for the shallow/full × merge-base matrix.
  • apps/desktop/package.json: register the new test in test:desktop:platforms.

How to Test

  1. cd apps/desktop && node --test electron/update-count.test.cjs — 6 tests pass.
  2. Regression proof: reverting the helper to the old unconditional Number.parseInt(countStr) || 0 makes the two shallow-checkout tests fail (the bogus 12104 count is surfaced instead of 1/0); restoring the guard makes all 6 pass.
  3. npm run test:desktop:platforms — the full platforms suite still passes with the new entry (one unrelated windows-child-process failure is a pre-existing baseline on clean main, untouched by this PR).

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 the desktop test suite (node --test over electron/*.test.cjs) and tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — the fix is git-behavior based and platform-independent
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@briandevans
fix(desktop): don't report a bogus update count for a shallow checkout
a1d0bdd 
The desktop installer clones with `--depth 1`, so a public install's local
history often shares no merge-base with the freshly fetched origin tip. In
that state `git rev-list HEAD..origin/<branch> --count` enumerates the
entire remote ancestry and returns a meaningless huge number, surfacing as
e.g. "v0.17.0 (+12104)" in the update indicator (NousResearch#51922).

The official-SSH branch of checkUpdates() already sidesteps this by reporting
a binary up-to-date check (`behind: currentSha === targetSha ? 0 : 1`), and
hermes_cli/banner.py guards the identical class for the CLI banner. The
passive desktop count path was the one place the shallow guard was missing.

Detect shallow + no-merge-base up front and fall back to the same SHA-based
binary check; full clones (developers / Docker dev images) keep the exact
count path unchanged. The resolution logic lives in a pure update-count.cjs
helper so it is unit-testable without booting Electron.
@briandevans
briandevans requested review from a team and Copilot June 24, 2026 15:32
Copilot AI reviewed Jun 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes the desktop client update indicator showing a bogus “(+N)” behind count when running from a shallow git checkout that has no merge-base with the fetched origin tip (e.g. installer --depth 1 clones).

Changes:

  • Adds a small helper (resolveBehindCount) that falls back to a SHA-based binary behind check when the repo is shallow and merge-base is unavailable.
  • Wires the helper into the non-SSH checkUpdates() path by also probing --is-shallow-repository and git merge-base.
  • Adds node:test coverage for shallow/full and merge-base/no-merge-base cases, and registers the new test in the desktop platforms test script.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
apps/desktop/package.json Adds the new update-count unit test to the desktop platforms test suite.
apps/desktop/electron/update-count.cjs Introduces resolveBehindCount() to avoid trusting rev-list --count in shallow/no-merge-base checkouts.
apps/desktop/electron/update-count.test.cjs Adds unit tests covering the behind-count resolution matrix.
apps/desktop/electron/main.cjs Uses shallow + merge-base probes and the helper to compute behind safely in the non-SSH update-check path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/desktop/electron/main.cjs Outdated
Comment on lines 1671 to 1675
const [currentSha, targetSha, countStr, dirtyStr, currentBranch, shallowStr, mergeBaseStr] = await Promise.all([
git(['rev-parse', 'HEAD']),
git(['rev-parse', `origin/${branch}`]),
git(['rev-list', `HEAD..origin/${branch}`, '--count']),
git(['status', '--porcelain']),
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Jun 24, 2026
@briandevans
fix(desktop): skip the rev-list count when it is discarded anyway
e25d97e 
checkUpdates() ran `git rev-list HEAD..origin/<branch> --count`
unconditionally in the parallel probe batch, even on the shallow +
no-merge-base path where resolveBehindCount() ignores the result and
falls back to a SHA compare. In the NousResearch#51922 failure mode that count walks
the entire remote ancestry (thousands of commits), so the work was pure
latency on every update check for the exact case the fix targets.

Split the probes into two phases: resolve --is-shallow-repository and
merge-base first, then run rev-list --count only when shouldCountCommits
says the number is meaningful (full clone, or shallow-with-merge-base).
The shallow/no-merge-base SHA fallback is preserved unchanged.
@briandevans

briandevans commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Good catch — addressed in e25d97e.

checkUpdates() now splits the git probes into two phases: --is-shallow-repository + merge-base (plus the cheap rev-parse/status queries) run first, and rev-list HEAD..origin/<branch> --count runs only when the count is meaningful, gated by a new shouldCountCommits({ isShallow, hasMergeBase }) helper in update-count.cjs. On the shallow + no-merge-base path (#51922) the whole-ancestry enumeration is now skipped entirely instead of being computed and discarded, so the latency you flagged is gone. The SHA-compare fallback in resolveBehindCount is unchanged, and the helper is unit-tested (fail-before/pass-after) in update-count.test.cjs.

@OutThisLife OutThisLife mentioned this pull request Jun 25, 2026
Merged
@briandevans

briandevans commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Closing to keep the queue lean — this branch has drifted into a merge conflict with main, never drew a review, and only corrects a cosmetic shallow-checkout update count. Happy to reopen if the underlying fix is still wanted.

@briandevans briandevans closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Desktop shows bogus client update count in dirty shallow checkout

3 participants

@briandevans @alt-glitch