From dfe64856f696474ee4030dbd2fd0dea0703cf9dc Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:34:36 -0700 Subject: [PATCH] fix(github): fail closed when the SSH git provider is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...nt-ssh-provider-execution-boundary.test.ts | 217 ++++++++++++++++++ src/main/github/client.ts | 12 +- 2 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 src/main/github/client-ssh-provider-execution-boundary.test.ts diff --git a/src/main/github/client-ssh-provider-execution-boundary.test.ts b/src/main/github/client-ssh-provider-execution-boundary.test.ts new file mode 100644 index 00000000000..d5e3bf185c1 --- /dev/null +++ b/src/main/github/client-ssh-provider-execution-boundary.test.ts @@ -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() + ) +) +vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(clientMocks)) +vi.mock('./github-api-repository', async (importOriginal) => + moduleMocks.githubApiRepositoryModuleMock( + clientMocks, + await importOriginal() + ) +) + +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' }) + }) +}) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index c3cae80520f..e0f9ef00dde 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -2315,8 +2315,12 @@ async function getCurrentHeadOid( connectionId?: string | null, localGitOptions: { wslDistro?: string } = {} ): Promise { + 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 }> { 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, {