fix(github): fail closed when the SSH git provider is gone

getCurrentHeadOid and probeTrackedUpstreamBranches only routed through the SSH provider when one was registered. With connectionId set but the provider unregistered (dropped connection, not yet reattached) they fell through to client-side git with cwd pointing at the remote repoPath — on a machine with a same-named local path that silently answers for the wrong repository. getCurrentHeadOid feeds shouldHideMergedImplicitPR, so a wrong OID changes which PR the UI attributes to a worktree. Both now take their existing unknown path (null / probeFailed) instead, matching repo-default-branch.ts. Local and WSL routing is unchanged.
2026-10-22 08:03:37 +00:00 · 2026-08-16 18:50:40 -07:00
parent 886dec1d2a
commit dfe64856f6
2 changed files with 227 additions and 2 deletions
@@ -0,0 +1,217 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import type * as GithubApiRepositoryModule from './github-api-repository'
+import type * as GitHubEnterpriseRepositoryModule from './github-enterprise-repository'
+
+const { clientMocks, moduleMocks } = await vi.hoisted(async () => {
+  const moduleMocks = await import('./client-test-mocks')
+  return { clientMocks: moduleMocks.createGitHubClientMocks(), moduleMocks }
+})
+
+vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks))
+vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks))
+vi.mock('../providers/ssh-git-dispatch', () => moduleMocks.sshGitDispatchModuleMock(clientMocks))
+vi.mock('./local-git-config-signature', () =>
+  moduleMocks.localGitConfigSignatureModuleMock(clientMocks)
+)
+vi.mock('./github-enterprise-repository', async (importOriginal) =>
+  moduleMocks.githubEnterpriseRepositoryModuleMock(
+    await importOriginal<typeof GitHubEnterpriseRepositoryModule>()
+  )
+)
+vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(clientMocks))
+vi.mock('./github-api-repository', async (importOriginal) =>
+  moduleMocks.githubApiRepositoryModuleMock(
+    clientMocks,
+    await importOriginal<typeof GithubApiRepositoryModule>()
+  )
+)
+
+import { getPRForBranch } from './client'
+import { resetPRForBranchMocks } from './client-test-harness'
+
+const { ghExecFileAsyncMock, getOwnerRepoMock, gitExecFileAsyncMock, getSshGitProviderMock } =
+  clientMocks
+
+const MERGED_BRANCH = 'fix-tab-strip-layout-test'
+const MERGED_HEAD_OID = 'current-head-oid'
+
+function mockMergedBranchPRLookup(): void {
+  ghExecFileAsyncMock
+    .mockResolvedValueOnce({
+      stdout: JSON.stringify([
+        {
+          number: 5875,
+          title: 'Merged current branch PR',
+          state: 'closed',
+          merged_at: '2026-06-20T04:53:05Z',
+          html_url: 'https://github.com/acme/widgets/pull/5875',
+          updated_at: '2026-06-20T04:53:05Z',
+          draft: false,
+          mergeable_state: 'clean',
+          head: { ref: MERGED_BRANCH, sha: MERGED_HEAD_OID },
+          base: { ref: 'main', sha: 'base-oid' }
+        }
+      ])
+    })
+    .mockResolvedValueOnce({
+      stdout: JSON.stringify({
+        number: 5875,
+        title: 'Merged current branch PR',
+        state: 'MERGED',
+        url: 'https://github.com/acme/widgets/pull/5875',
+        statusCheckRollup: [],
+        updatedAt: '2026-06-20T04:53:05Z',
+        isDraft: false,
+        mergeable: 'MERGEABLE',
+        baseRefName: 'main',
+        headRefName: MERGED_BRANCH,
+        baseRefOid: 'base-oid',
+        headRefOid: MERGED_HEAD_OID
+      })
+    })
+}
+
+function mockUpstreamOnlyPRLookup(): void {
+  ghExecFileAsyncMock
+    .mockResolvedValueOnce({ stdout: JSON.stringify([]) })
+    .mockResolvedValueOnce({
+      stdout: JSON.stringify([
+        {
+          number: 78,
+          title: 'Upstream branch PR',
+          state: 'open',
+          html_url: 'https://github.com/acme/widgets/pull/78',
+          updated_at: '2026-03-28T00:00:00Z',
+          draft: false,
+          mergeable: true,
+          base: { ref: 'main', sha: 'base-oid' },
+          head: { ref: 'contributor/original', sha: 'upstream-head-oid' }
+        }
+      ])
+    })
+    .mockResolvedValueOnce({
+      stdout: JSON.stringify({
+        number: 78,
+        title: 'Hydrated upstream branch PR',
+        state: 'OPEN',
+        url: 'https://github.com/acme/widgets/pull/78',
+        statusCheckRollup: [],
+        updatedAt: '2026-03-28T00:00:00Z',
+        isDraft: false,
+        mergeable: 'MERGEABLE',
+        baseRefName: 'main',
+        headRefName: 'contributor/original',
+        baseRefOid: 'base-oid',
+        headRefOid: 'upstream-head-oid'
+      })
+    })
+}
+
+// The strict execution boundary: for an SSH-hosted repoPath, an unregistered
+// provider means "we could not ask" — it must never degrade into a client-side
+// git run, which on a same-named local path answers for the wrong repository.
+describe('getPRForBranch SSH execution boundary', () => {
+  beforeEach(() => {
+    resetPRForBranchMocks(clientMocks)
+    getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' })
+  })
+
+  it('does not rev-parse HEAD locally when the SSH provider is unregistered', async () => {
+    getSshGitProviderMock.mockReturnValue(undefined)
+    mockMergedBranchPRLookup()
+    gitExecFileAsyncMock.mockResolvedValue({ stdout: `${MERGED_HEAD_OID}\n`, stderr: '' })
+
+    const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1')
+
+    expect(gitExecFileAsyncMock).not.toHaveBeenCalled()
+    // Unknown HEAD keeps the merged implicit match hidden rather than attributing it from a local answer.
+    expect(pr).toBeNull()
+  })
+
+  it('rev-parses HEAD through the SSH provider when it is registered', async () => {
+    const sshGitProvider = {
+      exec: vi.fn(async (args: string[]) =>
+        args[0] === 'rev-parse' && args[1] === 'HEAD'
+          ? { stdout: `${MERGED_HEAD_OID}\n`, stderr: '' }
+          : { stdout: '', stderr: '' }
+      )
+    }
+    getSshGitProviderMock.mockReturnValue(sshGitProvider)
+    mockMergedBranchPRLookup()
+
+    const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1')
+
+    expect(sshGitProvider.exec).toHaveBeenCalledWith(['rev-parse', 'HEAD'], '/remote/repo')
+    expect(gitExecFileAsyncMock).not.toHaveBeenCalled()
+    expect(pr).toMatchObject({ number: 5875, state: 'merged', headSha: MERGED_HEAD_OID })
+  })
+
+  it('rev-parses HEAD through the local runtime for a WSL repository', async () => {
+    mockMergedBranchPRLookup()
+    gitExecFileAsyncMock.mockResolvedValue({ stdout: `${MERGED_HEAD_OID}\n`, stderr: '' })
+
+    const pr = await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, {
+      localGitExecOptions: { wslDistro: 'Ubuntu' }
+    })
+
+    expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['rev-parse', 'HEAD'], {
+      cwd: '/repo-root',
+      wslDistro: 'Ubuntu'
+    })
+    expect(pr).toMatchObject({ number: 5875, state: 'merged', headSha: MERGED_HEAD_OID })
+  })
+
+  it('does not read tracked upstreams locally when the SSH provider is unregistered', async () => {
+    getSshGitProviderMock.mockReturnValue(undefined)
+    mockUpstreamOnlyPRLookup()
+    gitExecFileAsyncMock.mockResolvedValue({
+      stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`,
+      stderr: ''
+    })
+
+    const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1')
+
+    expect(gitExecFileAsyncMock).not.toHaveBeenCalled()
+    // The failed probe yields no upstream, so the upstream-named PR is never claimed for this branch.
+    expect(pr).toBeNull()
+    expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
+  })
+
+  it('reads tracked upstreams through the SSH provider when it is registered', async () => {
+    const forEachRefArgs = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads']
+    const sshGitProvider = {
+      exec: vi.fn(async (args: string[]) =>
+        args[0] === 'for-each-ref'
+          ? { stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, stderr: '' }
+          : { stdout: '', stderr: '' }
+      )
+    }
+    getSshGitProviderMock.mockReturnValue(sshGitProvider)
+    mockUpstreamOnlyPRLookup()
+
+    const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1')
+
+    expect(sshGitProvider.exec).toHaveBeenCalledWith(forEachRefArgs, '/remote/repo')
+    expect(gitExecFileAsyncMock).not.toHaveBeenCalled()
+    expect(pr).toMatchObject({ number: 78, title: 'Hydrated upstream branch PR' })
+  })
+
+  it('reads tracked upstreams through the local runtime for a WSL repository', async () => {
+    const forEachRefArgs = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads']
+    mockUpstreamOnlyPRLookup()
+    gitExecFileAsyncMock.mockResolvedValue({
+      stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`,
+      stderr: ''
+    })
+
+    const pr = await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, {
+      localGitExecOptions: { wslDistro: 'Ubuntu' }
+    })
+
+    expect(gitExecFileAsyncMock).toHaveBeenCalledWith(forEachRefArgs, {
+      cwd: '/repo-root',
+      wslDistro: 'Ubuntu'
+    })
+    expect(pr).toMatchObject({ number: 78, title: 'Hydrated upstream branch PR' })
+  })
+})
@@ -2315,8 +2315,12 @@ async function getCurrentHeadOid(
  connectionId?: string | null,
  localGitOptions: { wslDistro?: string } = {}
 ): Promise<string | null> {
+  const provider = connectionId ? getSshGitProvider(connectionId) : null
+  if (connectionId && !provider) {
+    // Why: repoPath is remote — a dropped SSH provider must answer "unknown", never run client git against a same-named local path.
+    return null
+  }
  try {
-    const provider = connectionId ? getSshGitProvider(connectionId) : null
    const result = provider
      ? await provider.exec(['rev-parse', 'HEAD'], repoPath)
      : await gitExecFileAsync(['rev-parse', 'HEAD'], {
@@ -2814,8 +2818,12 @@ async function probeTrackedUpstreamBranches(
  upstreamsByBranchName: Map<string, TrackedUpstreamBranch | null>
 }> {
  const args = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads']
+  const provider = connectionId ? getSshGitProvider(connectionId) : null
+  if (connectionId && !provider) {
+    // Why: repoPath is remote — a dropped SSH provider must fail the probe, never enumerate a same-named local repo's refs.
+    return { probeFailed: true, upstreamsByBranchName: new Map() }
+  }
  try {
-    const provider = connectionId ? getSshGitProvider(connectionId) : null
    const result = provider
      ? await provider.exec(args, repoPath)
      : await gitExecFileAsync(args, {