fix(desktop): restore backend lifecycle and update build contracts

apps/desktop/electron/backend-connection-state.test.ts

@@ -1,7 +1,10 @@
 import assert from 'node:assert/strict'
+import { type ChildProcess, spawn } from 'node:child_process'
+import { once } from 'node:events'
 
 import { test } from 'vitest'
 
+import { waitForBackendExit } from './backend-child'
 import { createBackendConnectionState } from './backend-connection-state'
 
 type FakeProcess = { id: string }
@@ -106,6 +109,116 @@ test('an invalidated attempt cannot attach a late-spawned process', () => {
   assert.equal(state.getProcess(), null)
 })
 
+test('a failed primary stop retains its child and blocks a replacement until retry exits', async () => {
+  const state = createBackendConnectionState<ChildProcess, string>()
+
+  const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], {
+    stdio: ['ignore', 'pipe', 'ignore'],
+    windowsHide: true
+  })
+
+  try {
+    await once(child.stdout!, 'data')
+    const attempt = state.startAttempt()
+    state.setPromise(attempt, Promise.resolve('ready'))
+    const owner = state.attachProcess(attempt, child)
+    assert.ok(owner)
+    const failure = new Error('primary is still running')
+    const calls: ChildProcess[] = []
+
+    const fail = async (current: ChildProcess): Promise<void> => {
+      calls.push(current)
+      throw failure
+    }
+
+    const stopping = state.stopProcess(fail)
+    assert.equal(state.getProcess(), null)
+    assert.equal(state.getPromise(), null)
+    assert.throws(() => state.startAttempt(), /has not stopped/)
+    assert.equal(state.stopProcess(fail), stopping)
+    await assert.rejects(stopping, error => error === failure)
+    state.invalidate()
+    assert.throws(() => state.startAttempt(), /has not stopped/)
+    assert.equal(child.exitCode, null)
+    assert.equal(child.signalCode, null)
+
+    await state.stopProcess(async current => {
+      calls.push(current)
+      current.kill()
+      await waitForBackendExit(current, {
+        forceKillProcessTree: (): void => {
+          current.kill('SIGKILL')
+        },
+        killGroup: (): void => {
+          current.kill('SIGKILL')
+        }
+      })
+    })
+    assert.deepEqual(calls, [child, child])
+    assert.ok(child.exitCode !== null || child.signalCode !== null)
+    const replacement = state.startAttempt()
+    const connection = Promise.resolve('new')
+    assert.equal(state.setPromise(replacement, connection), true)
+    assert.equal(state.clearForCurrentProcess(owner), false)
+    assert.equal(state.getPromise(), connection)
+  } finally {
+    if (child.exitCode === null && child.signalCode === null) {
+      const closed = once(child, 'close')
+      child.kill()
+      await closed
+    }
+  }
+}, 15_000)
+
+test('shutdown sees a spawned child while its persistent claim is still pending', async () => {
+  const state = createBackendConnectionState<ChildProcess, string>()
+
+  const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], {
+    stdio: ['ignore', 'pipe', 'ignore'],
+    windowsHide: true
+  })
+
+  const claim = deferred<void>()
+
+  try {
+    await once(child.stdout!, 'data')
+    const attempt = state.startAttempt()
+
+    const claiming = state.claimProcess(attempt, child, async current => {
+      assert.equal(current, child)
+      assert.equal(state.getProcess(), child)
+      await claim.promise
+    })
+
+    assert.equal(state.getProcess(), child)
+    await state.stopProcess(async current => {
+      assert.equal(current, child)
+      current.kill()
+      await waitForBackendExit(current, {
+        forceKillProcessTree: (): void => {
+          current.kill('SIGKILL')
+        },
+        killGroup: (): void => {
+          current.kill('SIGKILL')
+        }
+      })
+    })
+    assert.ok(child.exitCode !== null || child.signalCode !== null)
+    claim.resolve()
+    assert.equal(await claiming, null, 'a completed claim cannot revive the stopped generation')
+    assert.equal(state.getProcess(), null)
+    assert.doesNotThrow(() => state.startAttempt())
+  } finally {
+    claim.resolve()
+
+    if (child.exitCode === null && child.signalCode === null) {
+      const closed = once(child, 'close')
+      child.kill()
+      await closed
+    }
+  }
+}, 15_000)
+
 test('distinguishes a pending connection attempt from a cached settled descriptor', async () => {
   const state = createBackendConnectionState<FakeProcess, string>()
   const connection = deferred<string>()

apps/desktop/electron/connection-apply.test.ts

@@ -4,6 +4,7 @@ import {
   applyConnectionChange,
   commitConnectionFailure,
   resolveTerminalConnection,
+  sshQuitShouldBlock,
   teardownSshState
 } from './connection-apply'
 
@@ -91,6 +92,48 @@ describe('resolveTerminalConnection', () => {
   })
 })
 
+describe('sshQuitShouldBlock', () => {
+  it('waits when connections exist and teardown has not finished', () => {
+    expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 1, bootstrapPending: 0, inFlight: null })).toBe(
+      true
+    )
+  })
+
+  it('waits when bootstrap is still running', () => {
+    expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 0, bootstrapPending: 1, inFlight: null })).toBe(
+      true
+    )
+  })
+
+  it('waits when the map is empty but a remote kill is already in flight', () => {
+    expect(
+      sshQuitShouldBlock({
+        teardownDone: false,
+        connectionCount: 0,
+        bootstrapPending: 0,
+        inFlight: Promise.resolve()
+      })
+    ).toBe(true)
+  })
+
+  it('does not block a second quit after teardown finished', () => {
+    expect(
+      sshQuitShouldBlock({
+        teardownDone: true,
+        connectionCount: 1,
+        bootstrapPending: 1,
+        inFlight: Promise.resolve()
+      })
+    ).toBe(false)
+  })
+
+  it('does not block quit when there is nothing to tear down', () => {
+    expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 0, bootstrapPending: 0, inFlight: null })).toBe(
+      false
+    )
+  })
+})
+
 describe('teardownSshState', () => {
   it('terminates the owned remote backend before closing its tunnel and SSH transport', async () => {
     const events: string[] = []

apps/desktop/electron/connection-apply.ts

@@ -61,6 +61,31 @@ async function resolveTerminalConnectionForSender(webContentsId, getTarget, ensu
   )
 }
 
+/** A second before-quit must still wait for an in-flight remote kill.
+ *
+ *  teardownSshConnection deletes the sshConnections entry first, then
+ *  SSH-execs kill. backendShutdown's finally() calls app.quit() and
+ *  re-enters before-quit with an empty map. Without `inFlight`, Electron
+ *  exits while disconnect is running and the detached serve --isolated
+ *  stays at pid 1 (post-#95085 leftover on #91668: window X on Windows). */
+function sshQuitShouldBlock({
+  teardownDone,
+  connectionCount,
+  bootstrapPending,
+  inFlight
+}: {
+  teardownDone: boolean
+  connectionCount: number
+  bootstrapPending: number
+  inFlight: Promise<unknown> | null
+}): boolean {
+  if (teardownDone) {
+    return false
+  }
+
+  return connectionCount > 0 || bootstrapPending > 0 || Boolean(inFlight)
+}
+
 async function teardownSshState(state, { cleanupRemote }) {
   // Remote process first, while the SSH channel can still exec kill.
   // Then drop the local forward and close the transport. Each step is
@@ -91,5 +116,6 @@ export {
   commitConnectionFailure,
   resolveTerminalConnection,
   resolveTerminalConnectionForSender,
+  sshQuitShouldBlock,
   teardownSshState
 }

apps/desktop/electron/update-count.test.ts

@@ -0,0 +1,302 @@
+import assert from 'node:assert/strict'
+import { execFileSync } from 'node:child_process'
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+
+import { test } from 'vitest'
+
+import {
+  compareApiUrl,
+  parseCompareBehindCount,
+  resolveBehindCount,
+  resolveCommitLogSelection,
+  shouldCountCommits
+} from './update-count'
+
+function createTempGitRepo() {
+  const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-update-count-'))
+  const git = (...args: string[]) => execFileSync('git', args, { cwd, encoding: 'utf8', timeout: 10_000 }).trim()
+
+  try {
+    git('init', '--quiet')
+    git('config', 'commit.gpgSign', 'false')
+    git('config', 'core.hooksPath', '.git/no-hooks')
+    git('config', 'user.name', 'Hermes Test')
+    git('config', 'user.email', 'hermes@example.invalid')
+
+    return { cwd, git }
+  } catch (error) {
+    fs.rmSync(cwd, { recursive: true, force: true })
+    throw error
+  }
+}
+
+// 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) — #51922. Later the branch returned the sentinel
+// `1`, which the UI rendered as a literal "1 change included" even when the
+// true count was far higher (e.g. 90, or the real-world 61 in #84591). An
+// update IS available here, but its exact size is unknown — the only honest
+// value is `null`.
+test('shallow checkout with no merge-base reports null (unknown count), not a fake 1', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '12104',
+      currentSha: 'aaa',
+      targetSha: 'bbb',
+      isShallow: true
+    }),
+    null
+  )
+})
+
+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
+    }),
+    0
+  )
+})
+
+test('shallow local-ahead checkout reports up-to-date when origin is a known ancestor', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '',
+      currentSha: 'local-child',
+      targetSha: 'origin-parent',
+      isShallow: true,
+      targetIsAncestorOfHead: true
+    }),
+    0
+  )
+})
+
+test('shallow Git graph proves the remote tip is an ancestor of a local commit', () => {
+  const { cwd, git } = createTempGitRepo()
+
+  try {
+    git('commit', '--allow-empty', '-m', 'origin tip')
+
+    const targetSha = git('rev-parse', 'HEAD')
+
+    git('update-ref', 'refs/remotes/origin/main', targetSha)
+    fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${targetSha}\n`)
+    git('commit', '--allow-empty', '-m', 'local child')
+
+    const currentSha = git('rev-parse', 'HEAD')
+
+    git('merge-base', '--is-ancestor', 'origin/main', 'HEAD')
+    assert.notEqual(currentSha, targetSha)
+    assert.equal(
+      resolveBehindCount({
+        countStr: '',
+        currentSha,
+        targetSha,
+        isShallow: true,
+        targetIsAncestorOfHead: true
+      }),
+      0
+    )
+  } finally {
+    fs.rmSync(cwd, { recursive: true, force: true })
+  }
+}, 30_000)
+
+test('shallow checkout with a merge-base does not trust an inflated rev-list count', () => {
+  const { cwd, git } = createTempGitRepo()
+
+  try {
+    git('commit', '--allow-empty', '-m', 'root')
+    git('commit', '--allow-empty', '-m', 'ancestor')
+
+    const redundantParent = git('rev-parse', 'HEAD')
+
+    git('commit', '--allow-empty', '-m', 'installed head')
+
+    const currentSha = git('rev-parse', 'HEAD')
+    const tree = git('rev-parse', 'HEAD^{tree}')
+
+    const targetSha = execFileSync('git', ['commit-tree', tree, '-p', currentSha, '-p', redundantParent], {
+      cwd,
+      encoding: 'utf8',
+      input: 'remote merge\n',
+      timeout: 10_000
+    }).trim()
+
+    git('update-ref', 'refs/remotes/origin/main', targetSha)
+
+    const completeCount = git('rev-list', 'HEAD..origin/main', '--count')
+
+    assert.equal(completeCount, '1')
+
+    fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${currentSha}\n`)
+
+    assert.equal(git('rev-parse', '--is-shallow-repository'), 'true')
+    assert.equal(git('merge-base', 'HEAD', 'origin/main'), currentSha)
+
+    const shallowCount = git('rev-list', 'HEAD..origin/main', '--count')
+
+    assert.ok(Number.parseInt(shallowCount, 10) > Number.parseInt(completeCount, 10))
+    assert.equal(
+      resolveBehindCount({
+        countStr: shallowCount,
+        currentSha,
+        targetSha,
+        isShallow: true
+      }),
+      null
+    )
+  } finally {
+    fs.rmSync(cwd, { recursive: true, force: true })
+  }
+}, 30_000)
+
+test('shallow checkout with a merge-base still uses presence-only status', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '3',
+      currentSha: 'aaa',
+      targetSha: 'bbb',
+      isShallow: true
+    }),
+    null
+  )
+})
+
+test('full (non-shallow) clone keeps the exact count path unchanged', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '7',
+      currentSha: 'aaa',
+      targetSha: 'bbb',
+      isShallow: false
+    }),
+    7
+  )
+})
+
+test('up-to-date full clone reports 0', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '0',
+      currentSha: 'x',
+      targetSha: 'x',
+      isShallow: false
+    }),
+    0
+  )
+})
+
+test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => {
+  assert.equal(
+    resolveBehindCount({
+      countStr: '',
+      currentSha: 'aaa',
+      targetSha: 'bbb',
+      isShallow: false
+    }),
+    0
+  )
+})
+
+// shouldCountCommits gates the expensive `rev-list --count` in checkUpdates().
+// Every shallow graph is incomplete, so a visible merge-base is not enough to
+// prove that the count is exact.
+test('shallow checkouts skip the rev-list count', () => {
+  assert.equal(shouldCountCommits({ isShallow: true }), false)
+})
+
+test('full (non-shallow) clones run the rev-list count', () => {
+  assert.equal(shouldCountCommits({ isShallow: false }), true)
+})
+
+test('shallow commit logs select only the fetched remote tip', () => {
+  assert.deepEqual(resolveCommitLogSelection({ branch: 'main', isShallow: true }), {
+    limit: 1,
+    revision: 'origin/main'
+  })
+})
+
+test('full-clone commit logs keep the complete behind range', () => {
+  assert.deepEqual(resolveCommitLogSelection({ branch: 'release', isShallow: false }), {
+    limit: 40,
+    revision: 'HEAD..origin/release'
+  })
+})
+
+// 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
+    }),
+    null
+  )
+  assert.equal(
+    resolveBehindCount({
+      countStr: '',
+      currentSha: 'same',
+      targetSha: 'same',
+      isShallow: true
+    }),
+    0
+  )
+})
+
+// --- compare-API recovery: the accuracy half of the class fix (#84591) ---
+
+const SHA_A = 'a'.repeat(40)
+const SHA_B = 'b'.repeat(40)
+
+test('compareApiUrl builds the GitHub compare URL for HTTPS origins', () => {
+  assert.equal(
+    compareApiUrl({
+      currentSha: SHA_A,
+      originUrl: 'https://github.com/NousResearch/hermes-agent.git',
+      targetSha: SHA_B
+    }),
+    `https://api.github.com/repos/NousResearch/hermes-agent/compare/${SHA_A}...${SHA_B}`
+  )
+})
+
+test('compareApiUrl handles SSH origin forms', () => {
+  for (const originUrl of [
+    'git@github.com:NousResearch/hermes-agent.git',
+    'ssh://git@github.com/NousResearch/hermes-agent.git',
+    'git@github.com:NousResearch/hermes-agent'
+  ]) {
+    assert.equal(
+      compareApiUrl({ currentSha: SHA_A, originUrl, targetSha: SHA_B }),
+      `https://api.github.com/repos/NousResearch/hermes-agent/compare/${SHA_A}...${SHA_B}`
+    )
+  }
+})
+
+test('compareApiUrl refuses non-GitHub remotes and partial SHAs', () => {
+  assert.equal(compareApiUrl({ currentSha: SHA_A, originUrl: 'https://gitlab.com/x/y.git', targetSha: SHA_B }), null)
+  assert.equal(compareApiUrl({ currentSha: 'abc123', originUrl: 'https://github.com/x/y.git', targetSha: SHA_B }), null)
+  assert.equal(compareApiUrl({ currentSha: SHA_A, originUrl: '', targetSha: SHA_B }), null)
+})
+
+test('parseCompareBehindCount returns ahead_by (the behind count)', () => {
+  assert.equal(parseCompareBehindCount({ ahead_by: 61, status: 'ahead' }), 61)
+  assert.equal(parseCompareBehindCount({ ahead_by: 0, status: 'behind' }), 0)
+})
+
+test('parseCompareBehindCount rejects malformed payloads', () => {
+  assert.equal(parseCompareBehindCount(null), null)
+  assert.equal(parseCompareBehindCount({}), null)
+  assert.equal(parseCompareBehindCount({ ahead_by: -2 }), null)
+  assert.equal(parseCompareBehindCount({ ahead_by: '61' }), null)
+  assert.equal(parseCompareBehindCount({ ahead_by: 1.5 }), null)
+  assert.equal(parseCompareBehindCount([]), null)
+})

apps/desktop/electron/update-count.ts

@@ -0,0 +1,115 @@
+// Whether `git rev-list HEAD..origin/<branch> --count` produces a meaningful
+// number worth computing. Installer checkouts are shallow (`--depth 1`), so
+// their visible graph is incomplete even when `merge-base` happens to find a
+// common commit. A merge can expose ancestry that the local shallow boundary
+// hides from HEAD, inflating the count with old commits. Exact counts are only
+// trustworthy in full clones; shallow checkouts use presence-only status plus
+// any positively proven local-ahead ancestry.
+function shouldCountCommits({ isShallow }: { isShallow: boolean }): boolean {
+  return !isShallow
+}
+
+// Resolve how many commits the local checkout is behind origin for the desktop
+// update indicator. Shallow checkouts use SHA equality plus any positively
+// proven local-ahead ancestry; exact counts remain exclusive to full clones.
+function resolveBehindCount({
+  countStr,
+  currentSha,
+  targetSha,
+  isShallow,
+  targetIsAncestorOfHead = false
+}: {
+  countStr: string
+  currentSha: string
+  targetSha: string
+  isShallow: boolean
+  targetIsAncestorOfHead?: boolean
+}): number | null {
+  if (!shouldCountCommits({ isShallow })) {
+    if (currentSha && targetSha && (currentSha === targetSha || targetIsAncestorOfHead)) {
+      return 0
+    }
+
+    // An update IS available, but its size is unknowable without a merge-base.
+    // Return null — never a numeric sentinel: the UI used to render the old
+    // `1` as a literal "1 change included" even when the true distance was
+    // far larger. null lets every surface say "update available" honestly.
+    return null
+  }
+
+  return Number.parseInt(countStr, 10) || 0
+}
+
+// Shallow history can also contaminate the changelog range. Trust the fetched
+// remote tip itself, but do not walk its ancestry. Full clones retain the
+// detailed range used by the existing update overlay.
+function resolveCommitLogSelection({ branch, isShallow }: { branch: string; isShallow: boolean }): {
+  limit: number
+  revision: string
+} {
+  const remote = `origin/${branch}`
+
+  return isShallow ? { limit: 1, revision: remote } : { limit: 40, revision: `HEAD..${remote}` }
+}
+
+// When the local graph can't count (behind === null), the GitHub compare API
+// still can: `GET /repos/<owner>/<repo>/compare/<current>...<target>` returns
+// `ahead_by` — how many commits the remote tip is ahead of the local HEAD,
+// i.e. exactly the behind count the shallow clone lost. Unauthenticated, no
+// clone depth required. Pure URL builder + response parser here; the network
+// call lives with the caller.
+function compareApiUrl({
+  currentSha,
+  originUrl,
+  targetSha
+}: {
+  currentSha: string
+  originUrl: string
+  targetSha: string
+}): string | null {
+  const sha = /^[0-9a-f]{40}$/i
+
+  if (!sha.test(currentSha || '') || !sha.test(targetSha || '')) {
+    return null
+  }
+
+  // Only GitHub remotes have a compare API. Reuse the canonical form the
+  // official-remote check produces: `github.com/<owner>/<repo>`.
+  const canonical = canonicalRemoteForCompare(originUrl)
+
+  if (!canonical) {
+    return null
+  }
+
+  return `https://api.github.com/repos/${canonical}/compare/${currentSha}...${targetSha}`
+}
+
+function canonicalRemoteForCompare(originUrl: string): string | null {
+  const value = String(originUrl || '').trim()
+
+  const match =
+    /^git@github\.com:([^/]+\/[^/]+?)(?:\.git)?\/?$/i.exec(value) ||
+    /^(?:ssh:\/\/git@|https:\/\/|http:\/\/)github\.com\/([^/]+\/[^/]+?)(?:\.git)?\/?$/i.exec(value)
+
+  return match ? match[1] : null
+}
+
+// `ahead_by` counts target commits not reachable from current — the behind
+// count. `status` is "ahead" / "behind" / "diverged" / "identical" relative to
+// current...target; any shape surprise returns null so the caller keeps the
+// honest "update available" fallback instead of trusting a partial answer.
+function parseCompareBehindCount(payload: unknown): number | null {
+  if (!payload || typeof payload !== 'object') {
+    return null
+  }
+
+  const ahead = (payload as { ahead_by?: unknown }).ahead_by
+
+  if (typeof ahead !== 'number' || !Number.isInteger(ahead) || ahead < 0) {
+    return null
+  }
+
+  return ahead
+}
+
+export { compareApiUrl, parseCompareBehindCount, resolveBehindCount, resolveCommitLogSelection, shouldCountCommits }