fix(projects): refresh stale automatic GitHub icons during enrichment (#22421)

* fix: repair a bounded stale project-icon case

Based on the report and proposal by @mmarabel.

Co-authored-by: mmarabel <mmarabel@users.noreply.github.com>

* fix(projects): preserve peer-owned remote metadata

* fix(projects): keep enrichment within the owning host

---------

Co-authored-by: mmarabel <mmarabel@users.noreply.github.com>

src/main/repo-git-remote-avatar-refresh.test.ts

@@ -0,0 +1,313 @@
+import { afterEach, expect, it, vi } from 'vitest'
+import { getRepoExecutionHostId, type ExecutionHostId } from '../shared/execution-host'
+import { deriveGitRemoteIdentity } from '../shared/git-remote-identity'
+import { projectHostSetupProjectionFromRepos } from '../shared/project-host-setup-projection'
+import { githubAvatarIcon, type RepoIcon } from '../shared/repo-icon'
+import type { Repo } from '../shared/repo-types'
+import { probeGitRemoteIdentity, type GitRemoteIdentityProbe } from './repo-git-remote-identity'
+import {
+  enrichMissingRepoGitRemoteIdentities,
+  flushRepoGitRemoteIdentityEnrichmentForTests,
+  resetRepoGitRemoteIdentityEnrichmentForTests
+} from './repo-git-remote-identity-enrichment'
+
+vi.mock('./repo-git-remote-identity', () => ({ probeGitRemoteIdentity: vi.fn() }))
+
+function identity(remote = 'https://github.com/org-b/app.git') {
+  const parsed = deriveGitRemoteIdentity(`origin\t${remote} (fetch)`)
+  if (!parsed) {
+    throw new Error('Fixture remote must parse')
+  }
+  return parsed
+}
+
+function repo(overrides: Partial<Repo> = {}): Repo {
+  return {
+    id: 'app',
+    path: '/workspace/app',
+    displayName: 'app',
+    kind: 'git',
+    badgeColor: '',
+    addedAt: 1,
+    upstream: null,
+    gitRemoteIdentity: identity(),
+    repoIcon: githubAvatarIcon({ owner: 'owner-a', repo: 'app' }),
+    ...overrides
+  }
+}
+
+function storeFor(repos: Repo[]) {
+  const getRepo = (id: string, hostId?: ExecutionHostId) =>
+    repos.find((row) => row.id === id && (!hostId || getRepoExecutionHostId(row) === hostId))
+  const updateRepo = vi.fn((id: string, updates: Partial<Repo>, hostId?: ExecutionHostId) => {
+    const current = getRepo(id, hostId)
+    if (!current) {
+      return null
+    }
+    Object.assign(current, updates)
+    return current
+  })
+  return { getRepos: () => repos, getRepo, updateRepo }
+}
+
+async function sweep(store: ReturnType<typeof storeFor>, onChanged = vi.fn()) {
+  enrichMissingRepoGitRemoteIdentities(store, { onChanged })
+  for (let i = 0; i < 8; i++) {
+    await flushRepoGitRemoteIdentityEnrichmentForTests()
+  }
+}
+
+async function refresh(store: ReturnType<typeof storeFor>, onChanged = vi.fn()) {
+  vi.useFakeTimers()
+  vi.setSystemTime(1_000)
+  await sweep(store, onChanged)
+  expect(probeGitRemoteIdentity).not.toHaveBeenCalled()
+  vi.setSystemTime(301_001)
+  await sweep(store, onChanged)
+}
+
+afterEach(() => {
+  resetRepoGitRemoteIdentityEnrichmentForTests()
+  vi.useRealTimers()
+  vi.clearAllMocks()
+})
+
+it.each(['already-current', 'changed'])(
+  'repairs a stale avatar when the canonical key is %s',
+  async (mode) => {
+    const local = repo({
+      gitRemoteIdentity:
+        mode === 'changed' ? identity('https://github.com/owner-a/app.git') : identity()
+    })
+    const peer = repo({
+      id: 'peer',
+      path: '/workspace/app',
+      connectionId: 'build',
+      repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' })
+    })
+    const store = storeFor([local, peer])
+    const onChanged = vi.fn()
+    vi.mocked(probeGitRemoteIdentity).mockResolvedValue({
+      status: 'resolved',
+      identity: identity()
+    })
+
+    expect(projectHostSetupProjectionFromRepos(store.getRepos()).projects).toHaveLength(2)
+    await refresh(store, onChanged)
+
+    expect(local.repoIcon).toEqual(githubAvatarIcon({ owner: 'org-b', repo: 'app' }))
+    expect(store.updateRepo).toHaveBeenCalledTimes(1)
+    expect(onChanged).toHaveBeenCalledTimes(1)
+    const projected = projectHostSetupProjectionFromRepos(store.getRepos())
+    expect(projected.projects.map(({ id }) => id)).toEqual(['github:org-b/app'])
+    expect(projected.setups.map(({ hostId, path }) => ({ hostId, path }))).toEqual([
+      { hostId: 'local', path: '/workspace/app' },
+      { hostId: 'ssh:build', path: '/workspace/app' }
+    ])
+    vi.setSystemTime(301_001 + 6 * 60 * 60 * 1000)
+    await sweep(store, onChanged)
+    expect(store.updateRepo).toHaveBeenCalledTimes(1)
+  }
+)
+
+it.each([
+  'https://github.company.test:8443/org-b/app.git',
+  'ssh://git@ssh.github.com:443/org-b/app.git'
+])('preserves GitHub endpoint identity for %s', async (remote) => {
+  const row = repo({ gitRemoteIdentity: identity(remote) })
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockResolvedValue({
+    status: 'resolved',
+    identity: identity(remote)
+  })
+  await refresh(store)
+  expect(projectHostSetupProjectionFromRepos([row]).projects[0]?.id).toBe(
+    remote.includes('company') ? 'github:github.company.test:8443/org-b/app' : 'github:org-b/app'
+  )
+})
+
+it.each([
+  'git@github-work:org/app.git',
+  'ssh://git@ghe-work/org/app.git',
+  'https://gitlab.com/team/sub/app.git',
+  'https://forgejo.test/team/app.git',
+  'https://gitea.test/team/app.git',
+  'https://code.company.test/team/app.git'
+])('leaves provider metadata intact for unresolved or other-provider remote %s', async (remote) => {
+  const row = repo({ gitRemoteIdentity: identity(remote) })
+  const original = row.repoIcon
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockResolvedValue({
+    status: 'resolved',
+    identity: identity(remote)
+  })
+  await refresh(store)
+  expect(row.repoIcon).toBe(original)
+  expect(store.updateRepo).not.toHaveBeenCalled()
+})
+
+const customIcons: (RepoIcon | null | undefined)[] = [
+  { type: 'emoji', emoji: '🐙' },
+  { type: 'lucide', name: 'Folder' },
+  { type: 'image', source: 'upload', src: 'data:image/png;base64,fixture', label: 'custom' },
+  { type: 'image', source: 'file', src: 'data:image/png;base64,fixture', label: 'custom' },
+  { type: 'image', source: 'favicon', src: 'https://website.test/favicon.png', label: 'custom' },
+  null,
+  undefined
+]
+
+it.each(customIcons)('preserves custom or cleared icon %j', async (repoIcon) => {
+  const row = repo({ repoIcon })
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() })
+  await refresh(store)
+  expect(row.repoIcon).toBe(repoIcon)
+  expect(store.updateRepo).not.toHaveBeenCalled()
+})
+
+it('preserves explicit upstream even when the remote disagrees', async () => {
+  const row = repo({ upstream: { owner: 'parent', repo: 'app', host: 'github.parent.test' } })
+  const original = row.repoIcon
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() })
+  await refresh(store)
+  expect(row.repoIcon).toBe(original)
+  expect(projectHostSetupProjectionFromRepos([row]).projects[0]?.id).toBe(
+    'github:github.parent.test/parent/app'
+  )
+  expect(store.updateRepo).not.toHaveBeenCalled()
+})
+
+it.each(['unavailable', 'no-remote'] as const)(
+  'preserves the last avatar when a refresh is %s',
+  async (status) => {
+    const row = repo()
+    const original = row.repoIcon
+    const store = storeFor([row])
+    vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status })
+    await refresh(store)
+    expect(row.repoIcon).toBe(original)
+    expect(store.updateRepo).not.toHaveBeenCalled()
+  }
+)
+
+it.each(['same', 'different', 'missing'] as const)(
+  'never probes or writes peer-owned metadata when identity is %s',
+  async (kind) => {
+    const row = repo({
+      executionHostId: 'runtime:peer',
+      connectionId: 'nested',
+      gitRemoteIdentity:
+        kind === 'missing'
+          ? undefined
+          : identity(kind === 'different' ? 'https://github.com/peer-only/app.git' : undefined)
+    })
+    const originalIcon = row.repoIcon
+    const originalIdentity = row.gitRemoteIdentity
+    const store = storeFor([row])
+    vi.mocked(probeGitRemoteIdentity).mockResolvedValue({
+      status: 'resolved',
+      identity: identity()
+    })
+    await (kind === 'missing' ? sweep(store) : refresh(store))
+    expect(probeGitRemoteIdentity).not.toHaveBeenCalled()
+    expect(row.repoIcon).toBe(originalIcon)
+    expect(row.gitRemoteIdentity).toBe(originalIdentity)
+    expect(store.updateRepo).not.toHaveBeenCalled()
+  }
+)
+
+it('repairs only the matching owner when repo IDs and paths collide across hosts', async () => {
+  const local = repo({ repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' }) })
+  const ssh = repo({ executionHostId: 'ssh:build' })
+  const store = storeFor([local, ssh])
+  vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'resolved', identity: identity() })
+  await refresh(store)
+  expect(store.updateRepo).toHaveBeenCalledWith(
+    'app',
+    { repoIcon: githubAvatarIcon({ owner: 'org-b', repo: 'app' }) },
+    'ssh:build'
+  )
+  expect(projectHostSetupProjectionFromRepos([local, ssh]).projects).toHaveLength(1)
+})
+
+it.each(['ssh:build', 'runtime:peer'] as const)(
+  'keeps local metadata writes scoped when a same-id %s row comes first',
+  async (hostId) => {
+    const foreignIdentity = identity('https://github.com/foreign/app.git')
+    const foreignIcon = githubAvatarIcon({ owner: 'foreign', repo: 'app' })
+    const foreign = repo({
+      executionHostId: hostId,
+      gitRemoteIdentity: foreignIdentity,
+      repoIcon: foreignIcon
+    })
+    const local = repo({ gitRemoteIdentity: identity('https://github.com/old-local/app.git') })
+    const store = storeFor([foreign, local])
+    vi.mocked(probeGitRemoteIdentity).mockImplementation(async (_path, probeHostId) => ({
+      status: 'resolved',
+      identity: probeHostId === 'local' ? identity() : foreignIdentity
+    }))
+    await refresh(store)
+    expect(foreign.gitRemoteIdentity).toBe(foreignIdentity)
+    expect(foreign.repoIcon).toBe(foreignIcon)
+    expect(local.gitRemoteIdentity).toEqual(identity())
+    expect(local.repoIcon).toEqual(githubAvatarIcon({ owner: 'org-b', repo: 'app' }))
+    expect(store.updateRepo).toHaveBeenCalledExactlyOnceWith(
+      'app',
+      { gitRemoteIdentity: identity(), repoIcon: local.repoIcon },
+      'local'
+    )
+  }
+)
+
+it('does not write after a pending local probe becomes peer-owned', async () => {
+  let answer: ((value: GitRemoteIdentityProbe) => void) | undefined
+  const row = repo({ gitRemoteIdentity: undefined })
+  const originalIcon = row.repoIcon
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockImplementation(
+    () =>
+      new Promise((resolve) => {
+        answer = resolve
+      })
+  )
+  enrichMissingRepoGitRemoteIdentities(store)
+  row.executionHostId = 'runtime:peer'
+  if (!answer) {
+    throw new Error('Expected pending probe')
+  }
+  answer({ status: 'resolved', identity: identity() })
+  await flushRepoGitRemoteIdentityEnrichmentForTests()
+  expect(row.gitRemoteIdentity).toBeUndefined()
+  expect(row.repoIcon).toBe(originalIcon)
+  expect(store.updateRepo).not.toHaveBeenCalled()
+})
+
+it('does not overwrite a custom icon selected while the probe is pending', async () => {
+  let answer: ((value: GitRemoteIdentityProbe) => void) | undefined
+  const row = repo({ gitRemoteIdentity: undefined })
+  const store = storeFor([row])
+  vi.mocked(probeGitRemoteIdentity).mockImplementation(
+    () =>
+      new Promise((resolve) => {
+        answer = resolve
+      })
+  )
+  enrichMissingRepoGitRemoteIdentities(store)
+  const selected: RepoIcon = { type: 'emoji', emoji: '🐙' }
+  row.repoIcon = selected
+  if (!answer) {
+    throw new Error('Expected pending probe')
+  }
+  answer({ status: 'resolved', identity: identity() })
+  await flushRepoGitRemoteIdentityEnrichmentForTests()
+  expect(row.repoIcon).toBe(selected)
+  expect(store.updateRepo).toHaveBeenCalledWith('app', { gitRemoteIdentity: identity() }, 'local')
+})
+
+it('never probes folder workspaces for an avatar repair', async () => {
+  const store = storeFor([repo({ kind: 'folder' })])
+  await refresh(store)
+  expect(probeGitRemoteIdentity).not.toHaveBeenCalled()
+  expect(store.updateRepo).not.toHaveBeenCalled()
+})

src/main/repo-git-remote-identity-enrichment.test.ts

@@ -129,23 +129,16 @@ describe('enrichMissingRepoGitRemoteIdentities', () => {
     await flushRepoGitRemoteIdentityEnrichmentForTests()
   })
 
-  it('never hands a runtime row nested SSH target to this client dispatch table', async () => {
-    // Why: `connectionId` on a `runtime:` row names a target inside that server's namespace, so
-    // dialing it here reaches a same-named box of ours. The row keeps the probe this process has
-    // always run for it; what it must never do is dial our same-named target.
+  it('never probes a runtime row through this client dispatch table', async () => {
     vi.mocked(probeGitRemoteIdentity).mockResolvedValue({ status: 'unavailable' })
     const store = makeStore(
       makeRepo({ connectionId: 'nested-1', executionHostId: 'runtime:env-a' })
     )
 
-    enrichMissingRepoGitRemoteIdentities(store)
+    await sweep(store)
 
-    expect(probeGitRemoteIdentity).toHaveBeenCalledWith(
-      '/workspace/sample-app',
-      'local',
-      expect.objectContaining({ signal: expect.any(AbortSignal) })
-    )
-    await flushRepoGitRemoteIdentityEnrichmentForTests()
+    expect(probeGitRemoteIdentity).not.toHaveBeenCalled()
+    expect(store.updateRepo).not.toHaveBeenCalled()
   })
 
   it('keeps same-path rows on two different SSH hosts from sharing one backoff', async () => {
@@ -208,7 +201,7 @@ describe('enrichMissingRepoGitRemoteIdentities', () => {
     enrichMissingRepoGitRemoteIdentities(store)
     await flushRepoGitRemoteIdentityEnrichmentForTests()
 
-    expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: null })
+    expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: null }, 'local')
     expect(repo.gitRemoteIdentity).toBeNull()
   })
 
@@ -243,7 +236,11 @@ describe('enrichMissingRepoGitRemoteIdentities', () => {
     enrichMissingRepoGitRemoteIdentities(store)
     await flushRepoGitRemoteIdentityEnrichmentForTests()
 
-    expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: remoteIdentity })
+    expect(store.updateRepo).toHaveBeenCalledWith(
+      'repo-1',
+      { gitRemoteIdentity: remoteIdentity },
+      'local'
+    )
   })
 
   it('does not re-probe a resolved identity before the refresh window elapses', async () => {
@@ -296,7 +293,11 @@ describe('enrichMissingRepoGitRemoteIdentities', () => {
     enrichMissingRepoGitRemoteIdentities(store, { onChanged })
     await drainEnrichmentSweep()
 
-    expect(store.updateRepo).toHaveBeenCalledWith('repo-1', { gitRemoteIdentity: movedIdentity })
+    expect(store.updateRepo).toHaveBeenCalledWith(
+      'repo-1',
+      { gitRemoteIdentity: movedIdentity },
+      'local'
+    )
     expect(repo.gitRemoteIdentity).toEqual(movedIdentity)
     expect(onChanged).toHaveBeenCalledTimes(1)
   })

src/main/repo-git-remote-identity-enrichment.ts

@@ -5,6 +5,9 @@ import {
   type ExecutionHostId
 } from '../shared/execution-host'
 import type { Repo } from '../shared/repo-types'
+import { githubAvatarIcon, type RepoIcon } from '../shared/repo-icon'
+import { isUnresolvedSshHostAlias } from '../shared/git-remote-host-alias'
+import { getProjectProviderIdentity } from '../shared/project-host-setup-projection'
 import { probeGitRemoteIdentity } from './repo-git-remote-identity'
 
 const NO_IDENTITY_RETRY_TTL_MS = 5 * 60 * 1000
@@ -21,8 +24,12 @@ const MAX_IDENTITY_REFRESHES_PER_SWEEP = 4
 
 type RepoIdentityStore = {
   getRepos(): Repo[]
-  getRepo?(id: string): Repo | undefined
-  updateRepo(id: string, updates: Pick<Partial<Repo>, 'gitRemoteIdentity'>): Repo | null
+  getRepo?(id: string, hostId?: ExecutionHostId): Repo | undefined
+  updateRepo(
+    id: string,
+    updates: Pick<Partial<Repo>, 'gitRemoteIdentity' | 'repoIcon'>,
+    hostId?: ExecutionHostId
+  ): Repo | null
 }
 
 type EnrichmentOptions = {
@@ -51,18 +58,22 @@ function getRepoLocationKey(repo: Pick<Repo, 'path' | 'connectionId' | 'executio
   return `${getRepoExecutionHostId(repo)}\0${repo.path}`
 }
 
-/**
- * The host *this* process may run the probe on. `getSshTargetIdForExecutionHost`, not the row's
- * file-holding target: a `runtime:` row's nested SSH target lives in that server's namespace, so
- * dialing it here would reach a same-named box of ours — a wrong-host answer, not a local one.
- */
-function getRepoProbeHostId(repo: Repo): ExecutionHostId {
+// A peer's nested SSH targets belong to its dispatch table, never this client's.
+function getRepoProbeHostId(repo: Repo): ExecutionHostId | null {
   const hostId = getRepoExecutionHostId(repo)
-  return getSshTargetIdForExecutionHost(hostId) ? hostId : LOCAL_EXECUTION_HOST_ID
+  return hostId === LOCAL_EXECUTION_HOST_ID || getSshTargetIdForExecutionHost(hostId)
+    ? hostId
+    : null
 }
 
-function getCurrentRepo(store: RepoIdentityStore, id: string): Repo | undefined {
-  return store.getRepo?.(id) ?? store.getRepos().find((repo) => repo.id === id)
+function getCurrentRepo(store: RepoIdentityStore, snapshot: Repo): Repo | undefined {
+  const hostId = getRepoExecutionHostId(snapshot)
+  const found = store.getRepo?.(snapshot.id, hostId)
+  return found && getRepoExecutionHostId(found) === hostId
+    ? found
+    : store
+        .getRepos()
+        .find((repo) => repo.id === snapshot.id && getRepoExecutionHostId(repo) === hostId)
 }
 
 function isSameProbedRepo(snapshot: Repo, current: Repo | undefined): current is Repo {
@@ -87,22 +98,65 @@ function shouldWriteProbedIdentity(current: Repo, probed: Repo['gitRemoteIdentit
   return !!probed && probed.canonicalKey !== existing.canonicalKey
 }
 
+function getAutomaticGitHubIconRefresh(
+  current: Repo,
+  probed: NonNullable<Repo['gitRemoteIdentity']>
+): RepoIcon | undefined {
+  if (
+    (current.upstream?.owner && current.upstream.repo) ||
+    current.repoIcon?.type !== 'image' ||
+    current.repoIcon.source !== 'github'
+  ) {
+    return undefined
+  }
+  const identity = getProjectProviderIdentity({
+    upstream: null,
+    repoIcon: undefined,
+    gitRemoteIdentity: probed
+  })
+  if (!identity || (identity.host && isUnresolvedSshHostAlias(identity.host))) {
+    return undefined
+  }
+  const icon = githubAvatarIcon(identity)
+  return icon.type === 'image' &&
+    current.repoIcon.src === icon.src &&
+    current.repoIcon.label === icon.label
+    ? undefined
+    : icon
+}
+
 function writeIdentity(
   store: RepoIdentityStore,
   snapshot: Repo,
   gitRemoteIdentity: Repo['gitRemoteIdentity']
 ): boolean {
-  const current = getCurrentRepo(store, snapshot.id)
-  if (
-    !isSameProbedRepo(snapshot, current) ||
-    !shouldWriteProbedIdentity(current, gitRemoteIdentity)
-  ) {
+  // A peer's repo metadata must never be repaired from a client-local probe.
+  const hostId = getRepoProbeHostId(snapshot)
+  if (!hostId) {
     return false
   }
-  return !!store.updateRepo(snapshot.id, { gitRemoteIdentity })
+  const current = getCurrentRepo(store, snapshot)
+  if (!isSameProbedRepo(snapshot, current)) {
+    return false
+  }
+  const writeRemote = shouldWriteProbedIdentity(current, gitRemoteIdentity)
+  const icon = gitRemoteIdentity
+    ? getAutomaticGitHubIconRefresh(current, gitRemoteIdentity)
+    : undefined
+  const update = (updates: Pick<Partial<Repo>, 'gitRemoteIdentity' | 'repoIcon'>): Repo | null => {
+    return store.updateRepo(snapshot.id, updates, hostId)
+  }
+  if (icon) {
+    return !!update({ ...(writeRemote ? { gitRemoteIdentity } : {}), repoIcon: icon })
+  }
+  return writeRemote && !!update({ gitRemoteIdentity })
 }
 
 async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo): Promise<boolean> {
+  const hostId = getRepoProbeHostId(repo)
+  if (!hostId) {
+    return false
+  }
   const locationKey = getRepoLocationKey(repo)
   const retryAfter = probeRetryAfterByLocation.get(locationKey) ?? 0
   if (retryAfter > Date.now()) {
@@ -119,7 +173,7 @@ async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo)
     : NO_IDENTITY_RETRY_TTL_MS
   const controller = new AbortController()
   const promise = (async () => {
-    const result = await probeGitRemoteIdentity(repo.path, getRepoProbeHostId(repo), {
+    const result = await probeGitRemoteIdentity(repo.path, hostId, {
       signal: controller.signal
     })
     // Why the signal and not a catch: probeGitRemoteIdentity swallows the AbortError and RESOLVES
@@ -187,7 +241,9 @@ function retireRemovedLocations(allRepos: Repo[]): void {
 
 function selectEnrichmentCandidates(store: RepoIdentityStore): Repo[] {
   const now = Date.now()
-  const repos = store.getRepos().filter((repo) => repo.kind !== 'folder')
+  const repos = store
+    .getRepos()
+    .filter((repo) => repo.kind !== 'folder' && getRepoProbeHostId(repo) !== null)
   // Why: the settled `null` marker stays a candidate on purpose — a repo that
   // gains a remote later must still resolve. Do not tighten this to
   // `=== undefined`; the retry TTL already bounds the cost and `writeIdentity`