Skip to content

fix(desktop): make shallow update status presence-only - #64469

Closed
metamindedu wants to merge 1 commit into
NousResearch:mainfrom
metamindedu:fix/desktop-shallow-update-count
Closed

fix(desktop): make shallow update status presence-only#64469
metamindedu wants to merge 1 commit into
NousResearch:mainfrom
metamindedu:fix/desktop-shallow-update-count

Conversation

@metamindedu

@metamindedu metamindedu commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This is a focused follow-up to #51922 / #52201. The existing Desktop guard treats a shallow checkout as unsafe only when merge-base is absent, and explicitly assumes that a shallow checkout with a visible merge-base has a reliable rev-list count.

That assumption does not always hold. A clean Windows installer checkout briefly reported 6,588 commits behind while the complete object graph between the same tips contained 49 commits. In a shallow graph, HEAD can hide ancestry that a remote merge path exposes, so merge-base can still succeed while git rev-list HEAD..origin/main --count includes old commits.

This change treats exact counts as trustworthy only in full clones. Shallow installs report 0 when the tips are equal or when Git positively proves that the remote tip is an ancestor of local HEAD; every other SHA mismatch remains the presence-only 1 sentinel. The changelog shows only the fetched remote tip instead of walking the contaminated ancestry range.

Related Issue

Follow-up to #51922 and the merged fix #52201.

Related but intentionally non-overlapping: #53479 / #53494 cover the CLI updater path; this PR only fixes the Desktop checkUpdates() path.

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

  • apps/desktop/electron/update-count.ts
    • trust exact commit counts only for full clones;
    • preserve shallow local-ahead checkouts by accepting only a positive remote-tip-is-ancestor result, while treating other SHA mismatches as presence-only updates;
    • select only origin/<branch> tip metadata for shallow changelogs while preserving the existing 40-commit range for full clones.
  • apps/desktop/electron/main.ts
    • replace the count-oriented merge-base assumption with a directional merge-base --is-ancestor proof for shallow local-ahead checkouts;
    • apply the shallow-safe count and changelog selection.
  • apps/desktop/electron/update-count.test.ts
    • add a real temporary-Git regression where a merge-base exists but shallow rev-list overcounts;
    • add a real shallow local-ahead graph so a local commit on top of origin/main remains up to date;
    • isolate Git signing/hooks, bound subprocess time, and cover shallow tip-only changelogs plus unchanged full-clone behavior.

How to Test

  1. cd apps/desktop && npm run test:desktop:platforms -- electron/update-count.test.ts electron/update-remote.test.ts
    • 20 passed (14 update-count + 6 update-remote).
    • The regression creates a complete graph where the real behind count is 1, marks the installed HEAD shallow, confirms merge-base still succeeds, and proves the visible shallow count is inflated before asserting the presence-only result.
    • A second real Git graph creates a local child on top of the shallow remote tip and verifies the directional ancestry check keeps behind at 0.
  2. cd apps/desktop && npm run typecheck
    • passed.
  3. cd apps/desktop && npx eslint electron/update-count.ts electron/update-count.test.ts
    • passed.
  4. cd apps/desktop && npx prettier --check electron/update-count.ts electron/update-count.test.ts
    • passed.
  5. cd apps/desktop && npm run build
    • passed, including Electron main/preload bundles and assert-dist-built.
  6. npm run test:desktop:platforms was also exercised on Windows. The changed update-count.test.ts passed under the full parallel suite (including the Git regression); the run retained unrelated existing Windows/POSIX baseline failures in windows-hermes-path.test.ts, update-relaunch.test.ts, git-worktree-ops.test.ts, and two .mjs test-loading suites.

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)
  • N/A — no Python files changed; targeted Desktop tests, typecheck, lint, formatting, and build were run instead
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 10

Documentation & Housekeeping

  • N/A — no user-facing documentation changes needed
  • N/A — no config keys changed
  • N/A — no architecture or contributor workflow changes
  • I've considered cross-platform impact; the regression uses Git commands already required by Desktop update checks and cleans its temporary repository in finally
  • N/A — no tool descriptions or schemas changed

Screenshots / Logs

Observed real installer state before the fix:

is_shallow=true
merge-base present
Desktop behind count=6588
complete object-graph behind count=49

After the shallow boundary was refreshed, the same checkout switched to the existing presence-only 1 display without moving HEAD. The regression test captures the underlying graph condition directly so the fix does not depend on a screenshot or a particular upstream commit count.

@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 Jul 14, 2026
@metamindedu
metamindedu force-pushed the fix/desktop-shallow-update-count branch from f373481 to bbe14a4 Compare July 14, 2026 15:39
@metamindedu
metamindedu force-pushed the fix/desktop-shallow-update-count branch from bbe14a4 to 686f306 Compare July 14, 2026 16:18

@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: Approved

Fix for desktop shallow update status to be presence-only. Well-scoped (119 additions, 65 deletions). No security concerns.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused follow-up. The premise is verified against current main: apps/desktop/electron/update-count.ts:9 still permits exact counts for shallow repositories whenever hasMergeBase is true, and apps/desktop/electron/main.ts:2117-2145 consequently runs and trusts rev-list --count in that state. The proposed change centralizes the safer shallow policy, preserves the full-clone count path, and adds both resolver and real-Git graph coverage.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #86257 — your commit was cherry-picked onto current main with your authorship preserved in git log (bf10349). Thanks! Your design carried three key pieces of the class fix: shallow checkouts always skip rev-list --count (merge-base or not), the merge-base --is-ancestor proof that keeps local-ahead checkouts reading "up to date", and the shallow changelog restriction to the fetched tip (including the real-git-fixture tests). Combined with #83381's null sentinel and topped with GitHub compare-API exact-count recovery.

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/desktop Electron desktop app (apps/desktop/*) 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 sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants