From a6485bddb855536e60baa7074573a820d3d1ee76 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 24 Jun 2026 08:32:08 -0700 Subject: [PATCH 1/2] fix(desktop): don't report a bogus update count for a shallow checkout 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/ --count` enumerates the entire remote ancestry and returns a meaningless huge number, surfacing as e.g. "v0.17.0 (+12104)" in the update indicator (#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. --- apps/desktop/electron/main.cjs | 17 +++++-- apps/desktop/electron/update-count.cjs | 20 +++++++++ apps/desktop/electron/update-count.test.cjs | 49 +++++++++++++++++++++ apps/desktop/package.json | 2 +- 4 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 apps/desktop/electron/update-count.cjs create mode 100644 apps/desktop/electron/update-count.test.cjs diff --git a/apps/desktop/electron/main.cjs b/apps/desktop/electron/main.cjs index 4ecefa4604b..7633d71b812 100644 --- a/apps/desktop/electron/main.cjs +++ b/apps/desktop/electron/main.cjs @@ -57,6 +57,7 @@ const { const { gitRootForIpc } = require('./git-root.cjs') const { worktreesForIpc } = require('./git-worktrees.cjs') const { OFFICIAL_REPO_HTTPS_URL, isOfficialSshRemote } = require('./update-remote.cjs') +const { resolveBehindCount } = require('./update-count.cjs') const { runRebuildWithRetry } = require('./update-rebuild.cjs') const { buildPosixCleanupScript, @@ -1706,15 +1707,25 @@ async function checkUpdates() { } const git = args => runGit(args, { cwd: updateRoot }).then(r => r.stdout.trim()) - const [currentSha, targetSha, countStr, dirtyStr, currentBranch] = await Promise.all([ + 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']), - git(['rev-parse', '--abbrev-ref', 'HEAD']) + git(['rev-parse', '--abbrev-ref', 'HEAD']), + git(['rev-parse', '--is-shallow-repository']), + // merge-base exits non-zero with empty stdout when HEAD shares no common + // ancestor with the freshly fetched tip — exactly the shallow-clone case. + git(['merge-base', 'HEAD', `origin/${branch}`]) ]) - const behind = Number.parseInt(countStr, 10) || 0 + const behind = resolveBehindCount({ + countStr, + currentSha, + targetSha, + isShallow: shallowStr === 'true', + hasMergeBase: Boolean(mergeBaseStr) + }) const commits = behind > 0 ? await readCommitLog(updateRoot, branch) : [] return { diff --git a/apps/desktop/electron/update-count.cjs b/apps/desktop/electron/update-count.cjs new file mode 100644 index 00000000000..c39deecc3d0 --- /dev/null +++ b/apps/desktop/electron/update-count.cjs @@ -0,0 +1,20 @@ +'use strict' + +// Resolve how many commits the local checkout is behind origin for the desktop +// update indicator. On a SHALLOW checkout (installer clones with --depth 1) the +// local history often shares no merge-base with the freshly fetched origin tip, +// so `git rev-list HEAD..origin/ --count` enumerates the entire remote +// ancestry and returns a bogus huge number (e.g. 12104) — see #51922. In that +// case the count is meaningless; fall back to a binary up-to-date check by SHA, +// exactly like the official-SSH path in checkUpdates() and the CLI guard in +// hermes_cli/banner.py. Full clones (developers / Docker dev images) keep the +// exact count path unchanged. +function resolveBehindCount({ countStr, currentSha, targetSha, isShallow, hasMergeBase }) { + if (isShallow && !hasMergeBase) { + if (currentSha && targetSha && currentSha === targetSha) return 0 + return 1 // behind by an unknown amount — show a generic "update available" + } + return Number.parseInt(countStr, 10) || 0 +} + +module.exports = { resolveBehindCount } diff --git a/apps/desktop/electron/update-count.test.cjs b/apps/desktop/electron/update-count.test.cjs new file mode 100644 index 00000000000..08c38059f7f --- /dev/null +++ b/apps/desktop/electron/update-count.test.cjs @@ -0,0 +1,49 @@ +'use strict' +const test = require('node:test') +const assert = require('node:assert/strict') +const { resolveBehindCount } = require('./update-count.cjs') + +// FAIL-BEFORE: pre-fix the function did `Number.parseInt(countStr) || 0` +// unconditionally, so a shallow checkout with no merge-base surfaced the bogus +// rev-list count (e.g. 12104). This asserts the new shallow/no-merge-base branch. +test('shallow checkout with no merge-base does NOT trust the bogus rev-list count', () => { + assert.equal(resolveBehindCount({ + countStr: '12104', currentSha: 'aaa', targetSha: 'bbb', + isShallow: true, hasMergeBase: false, + }), 1) +}) + +test('shallow checkout with no merge-base but identical SHA reports up-to-date', () => { + assert.equal(resolveBehindCount({ + countStr: '12104', currentSha: 'abc', targetSha: 'abc', + isShallow: true, hasMergeBase: false, + }), 0) +}) + +test('shallow checkout WITH a merge-base keeps the exact count (reliable)', () => { + assert.equal(resolveBehindCount({ + countStr: '3', currentSha: 'aaa', targetSha: 'bbb', + isShallow: true, hasMergeBase: true, + }), 3) +}) + +test('full (non-shallow) clone keeps the exact count path unchanged', () => { + assert.equal(resolveBehindCount({ + countStr: '7', currentSha: 'aaa', targetSha: 'bbb', + isShallow: false, hasMergeBase: true, + }), 7) +}) + +test('up-to-date full clone reports 0', () => { + assert.equal(resolveBehindCount({ + countStr: '0', currentSha: 'x', targetSha: 'x', + isShallow: false, hasMergeBase: true, + }), 0) +}) + +test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => { + assert.equal(resolveBehindCount({ + countStr: '', currentSha: 'aaa', targetSha: 'bbb', + isShallow: false, hasMergeBase: true, + }), 0) +}) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 2661f8af18b..5ab50597139 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -37,7 +37,7 @@ "test:desktop:nsis": "node scripts/test-desktop.mjs nsis", "test:desktop:existing": "node scripts/test-desktop.mjs existing", "test:desktop:fresh": "node scripts/test-desktop.mjs fresh", - "test:desktop:platforms": "node --test electron/bootstrap-platform.test.cjs electron/hardening.test.cjs electron/backend-env.test.cjs electron/backend-probes.test.cjs electron/backend-ready.test.cjs electron/bootstrap-runner.test.cjs electron/connection-config.test.cjs electron/dashboard-token.test.cjs electron/gateway-ws-probe.test.cjs electron/oauth-net-request.test.cjs electron/desktop-uninstall.test.cjs electron/session-windows.test.cjs electron/link-title-window.test.cjs electron/workspace-cwd.test.cjs electron/fs-read-dir.test.cjs electron/git-root.test.cjs electron/windows-child-process.test.cjs electron/update-remote.test.cjs electron/update-rebuild.test.cjs electron/update-marker.test.cjs electron/update-relaunch.test.cjs electron/windows-user-env.test.cjs electron/window-state.test.cjs", + "test:desktop:platforms": "node --test electron/bootstrap-platform.test.cjs electron/hardening.test.cjs electron/backend-env.test.cjs electron/backend-probes.test.cjs electron/backend-ready.test.cjs electron/bootstrap-runner.test.cjs electron/connection-config.test.cjs electron/dashboard-token.test.cjs electron/gateway-ws-probe.test.cjs electron/oauth-net-request.test.cjs electron/desktop-uninstall.test.cjs electron/session-windows.test.cjs electron/link-title-window.test.cjs electron/workspace-cwd.test.cjs electron/fs-read-dir.test.cjs electron/git-root.test.cjs electron/windows-child-process.test.cjs electron/update-remote.test.cjs electron/update-count.test.cjs electron/update-rebuild.test.cjs electron/update-marker.test.cjs electron/update-relaunch.test.cjs electron/windows-user-env.test.cjs electron/window-state.test.cjs", "typecheck": "tsc -p . --noEmit", "lint": "eslint src/ electron/", "lint:fix": "eslint src/ electron/ --fix", From cb6edbf448e7878fd25bf6db20df4c18ed8755a7 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:27:54 -0700 Subject: [PATCH 2/2] fix(desktop): skip the rev-list count when it is discarded anyway checkUpdates() ran `git rev-list HEAD..origin/ --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 #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. --- apps/desktop/electron/main.cjs | 19 ++++++++---- apps/desktop/electron/update-count.cjs | 28 +++++++++++------- apps/desktop/electron/update-count.test.cjs | 32 ++++++++++++++++++++- 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/apps/desktop/electron/main.cjs b/apps/desktop/electron/main.cjs index 7633d71b812..ce42e3474dc 100644 --- a/apps/desktop/electron/main.cjs +++ b/apps/desktop/electron/main.cjs @@ -57,7 +57,7 @@ const { const { gitRootForIpc } = require('./git-root.cjs') const { worktreesForIpc } = require('./git-worktrees.cjs') const { OFFICIAL_REPO_HTTPS_URL, isOfficialSshRemote } = require('./update-remote.cjs') -const { resolveBehindCount } = require('./update-count.cjs') +const { resolveBehindCount, shouldCountCommits } = require('./update-count.cjs') const { runRebuildWithRetry } = require('./update-rebuild.cjs') const { buildPosixCleanupScript, @@ -1707,10 +1707,9 @@ async function checkUpdates() { } const git = args => runGit(args, { cwd: updateRoot }).then(r => r.stdout.trim()) - const [currentSha, targetSha, countStr, dirtyStr, currentBranch, shallowStr, mergeBaseStr] = await Promise.all([ + const [currentSha, targetSha, 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']), git(['rev-parse', '--abbrev-ref', 'HEAD']), git(['rev-parse', '--is-shallow-repository']), @@ -1719,12 +1718,22 @@ async function checkUpdates() { git(['merge-base', 'HEAD', `origin/${branch}`]) ]) + const isShallow = shallowStr === 'true' + const hasMergeBase = Boolean(mergeBaseStr) + // Only enumerate the commit count when it is meaningful. On a shallow checkout + // with no merge-base, `rev-list --count` walks the entire remote ancestry + // (thousands of commits, see #51922) and resolveBehindCount discards the + // result anyway in favour of a SHA compare — so skip the expensive query. + const countStr = shouldCountCommits({ isShallow, hasMergeBase }) + ? await git(['rev-list', `HEAD..origin/${branch}`, '--count']) + : '' + const behind = resolveBehindCount({ countStr, currentSha, targetSha, - isShallow: shallowStr === 'true', - hasMergeBase: Boolean(mergeBaseStr) + isShallow, + hasMergeBase }) const commits = behind > 0 ? await readCommitLog(updateRoot, branch) : [] diff --git a/apps/desktop/electron/update-count.cjs b/apps/desktop/electron/update-count.cjs index c39deecc3d0..de8d57c4ee6 100644 --- a/apps/desktop/electron/update-count.cjs +++ b/apps/desktop/electron/update-count.cjs @@ -1,20 +1,28 @@ 'use strict' +// Whether `git rev-list HEAD..origin/ --count` produces a meaningful +// number worth computing. On a SHALLOW checkout (installer clones with +// --depth 1) the local history often shares no merge-base with the freshly +// fetched origin tip, so the count enumerates the entire remote ancestry and +// returns a bogus huge number (e.g. 12104) — see #51922. resolveBehindCount +// discards that bogus count in favour of a SHA compare, so the caller should +// SKIP the expensive rev-list entirely in that case rather than run it and +// throw the result away. +function shouldCountCommits({ isShallow, hasMergeBase }) { + return !(isShallow && !hasMergeBase) +} + // Resolve how many commits the local checkout is behind origin for the desktop -// update indicator. On a SHALLOW checkout (installer clones with --depth 1) the -// local history often shares no merge-base with the freshly fetched origin tip, -// so `git rev-list HEAD..origin/ --count` enumerates the entire remote -// ancestry and returns a bogus huge number (e.g. 12104) — see #51922. In that -// case the count is meaningless; fall back to a binary up-to-date check by SHA, -// exactly like the official-SSH path in checkUpdates() and the CLI guard in -// hermes_cli/banner.py. Full clones (developers / Docker dev images) keep the -// exact count path unchanged. +// update indicator. When the count isn't meaningful (shallow + no merge-base) +// fall back to a binary up-to-date check by SHA, exactly like the official-SSH +// path in checkUpdates() and the CLI guard in hermes_cli/banner.py. Full clones +// (developers / Docker dev images) keep the exact count path unchanged. function resolveBehindCount({ countStr, currentSha, targetSha, isShallow, hasMergeBase }) { - if (isShallow && !hasMergeBase) { + if (!shouldCountCommits({ isShallow, hasMergeBase })) { if (currentSha && targetSha && currentSha === targetSha) return 0 return 1 // behind by an unknown amount — show a generic "update available" } return Number.parseInt(countStr, 10) || 0 } -module.exports = { resolveBehindCount } +module.exports = { resolveBehindCount, shouldCountCommits } diff --git a/apps/desktop/electron/update-count.test.cjs b/apps/desktop/electron/update-count.test.cjs index 08c38059f7f..69ee99aa616 100644 --- a/apps/desktop/electron/update-count.test.cjs +++ b/apps/desktop/electron/update-count.test.cjs @@ -1,7 +1,7 @@ 'use strict' const test = require('node:test') const assert = require('node:assert/strict') -const { resolveBehindCount } = require('./update-count.cjs') +const { resolveBehindCount, shouldCountCommits } = require('./update-count.cjs') // FAIL-BEFORE: pre-fix the function did `Number.parseInt(countStr) || 0` // unconditionally, so a shallow checkout with no merge-base surfaced the bogus @@ -47,3 +47,33 @@ test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => isShallow: false, hasMergeBase: true, }), 0) }) + +// shouldCountCommits gates the expensive `rev-list --count` in checkUpdates(). +// FAIL-BEFORE: in the shallow + no-merge-base case the caller ran rev-list +// unconditionally and discarded the bogus result; this predicate lets the +// caller SKIP the whole-ancestry enumeration in exactly that case (#51922). +test('shallow checkout with no merge-base SKIPS the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: true, hasMergeBase: false }), false) +}) + +test('shallow checkout WITH a merge-base still runs the count', () => { + assert.equal(shouldCountCommits({ isShallow: true, hasMergeBase: true }), true) +}) + +test('full (non-shallow) clone always runs the count', () => { + assert.equal(shouldCountCommits({ isShallow: false, hasMergeBase: true }), true) + assert.equal(shouldCountCommits({ isShallow: false, hasMergeBase: false }), true) +}) + +// The skip path produces an empty countStr; resolveBehindCount must NOT trust +// it and must fall through to the SHA compare (mirrors the live call site). +test('skipped-count path resolves via SHA compare, never via empty countStr', () => { + assert.equal(resolveBehindCount({ + countStr: '', currentSha: 'aaa', targetSha: 'bbb', + isShallow: true, hasMergeBase: false, + }), 1) + assert.equal(resolveBehindCount({ + countStr: '', currentSha: 'same', targetSha: 'same', + isShallow: true, hasMergeBase: false, + }), 0) +})