diff --git a/src/main/ipc/github-pr-refresh-handlers.ts b/src/main/ipc/github-pr-refresh-handlers.ts index 71e1d2793a5..5c45c96901a 100644 --- a/src/main/ipc/github-pr-refresh-handlers.ts +++ b/src/main/ipc/github-pr-refresh-handlers.ts @@ -21,6 +21,7 @@ import type { StatsCollector } from '../stats/collector' import { applyRegisteredRepoToPRRefreshCandidate, assertRegisteredGitHubRepo, + assertRegisteredPRRefreshCandidateRepo, getGitHubLocalGitOptionArgs, getGitHubRepoConnectionId, isPRRefreshCandidateRegistered, @@ -115,7 +116,7 @@ export function registerGitHubPRRefreshHandlers(store: Store, stats: StatsCollec _event, args: { candidate: GitHubPRRefreshCandidate; reason?: GitHubPRRefreshReason } ) => { - const repo = assertRegisteredGitHubRepo(args.candidate, store) + const repo = assertRegisteredPRRefreshCandidateRepo(args.candidate, store) const outcome = await refreshPRNow( applyRegisteredRepoToPRRefreshCandidate(store, repo, args.candidate), args.reason diff --git a/src/main/ipc/github-repo-routing-twin-host.test.ts b/src/main/ipc/github-repo-routing-twin-host.test.ts new file mode 100644 index 00000000000..a37e7ef815c --- /dev/null +++ b/src/main/ipc/github-repo-routing-twin-host.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it, vi } from 'vitest' +import type { GitHubPRRefreshCandidate } from '../../shared/github/pull-request-refresh-types' +import type { Repo } from '../../shared/repo-types' +import type { Store } from '../persistence' + +vi.mock('../project-runtime-git-options', () => ({ + getLocalProjectGhExecOptions: () => ({}) +})) + +import { + assertRegisteredGitHubRepo, + assertRegisteredPRRefreshCandidateRepo, + validateAutomaticPRRefreshCandidate +} from './github-repo-routing' + +const localTwin: Repo = { + id: 'twin', + path: '/laptop/twin', + displayName: 'Twin', + badgeColor: '#000', + addedAt: 1 +} +const sshTwin: Repo = { ...localTwin, path: '/remote/twin', connectionId: 'builder' } +const samePathSshTwin: Repo = { ...sshTwin, path: localTwin.path } + +function storeWith(repos: Repo[]): Store { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: these paths read only getRepos. + return { getRepos: () => repos } as unknown as Store +} + +function candidate(overrides: Partial): GitHubPRRefreshCandidate { + return { + cacheKey: 'twin::branch', + repoPath: sshTwin.path, + branch: 'feature', + repoKind: 'git', + repoId: 'twin', + connectionId: 'builder', + ...overrides + } +} + +describe('registered GitHub repo lookup for a repo id on two hosts', () => { + it("accepts the second twin's fetch by its own path", () => { + const store = storeWith([localTwin, sshTwin]) + + expect(assertRegisteredGitHubRepo({ repoPath: '/remote/twin', repoId: 'twin' }, store)).toBe( + sshTwin + ) + expect(assertRegisteredGitHubRepo({ repoPath: '/laptop/twin', repoId: 'twin' }, store)).toBe( + localTwin + ) + }) + + it("queues the SSH twin's PR refresh on the SSH row, even when it is listed second", () => { + const result = validateAutomaticPRRefreshCandidate( + candidate({}), + storeWith([localTwin, sshTwin]) + ) + + expect(result).toMatchObject({ + kind: 'ok', + candidate: { repoPath: '/remote/twin', connectionId: 'builder' } + }) + }) + + it('picks the candidate host when the twins share a path', () => { + const store = storeWith([localTwin, samePathSshTwin]) + + expect( + validateAutomaticPRRefreshCandidate(candidate({ repoPath: localTwin.path }), store) + ).toMatchObject({ kind: 'ok', candidate: { connectionId: 'builder' } }) + expect( + validateAutomaticPRRefreshCandidate( + candidate({ repoPath: localTwin.path, connectionId: null }), + store + ) + ).toMatchObject({ kind: 'ok', candidate: { connectionId: null } }) + }) + + it('resolves a manual PR refresh to the same twin as the automatic queue', () => { + const store = storeWith([localTwin, samePathSshTwin]) + + expect( + assertRegisteredPRRefreshCandidateRepo(candidate({ repoPath: localTwin.path }), store) + ).toBe(samePathSshTwin) + }) + + it("keeps a lone row's own host over a stale candidate host", () => { + const result = validateAutomaticPRRefreshCandidate( + candidate({ repoPath: localTwin.path, connectionId: 'gone' }), + storeWith([localTwin]) + ) + + expect(result).toMatchObject({ kind: 'ok', candidate: { connectionId: null } }) + }) + + it('still refuses a path that no row with that id holds', () => { + expect(() => + assertRegisteredGitHubRepo( + { repoPath: '/elsewhere', repoId: 'twin' }, + storeWith([localTwin, sshTwin]) + ) + ).toThrow('repository path does not match repo id') + }) +}) diff --git a/src/main/ipc/github-repo-routing.ts b/src/main/ipc/github-repo-routing.ts index bc147d41e92..c82949cbe94 100644 --- a/src/main/ipc/github-repo-routing.ts +++ b/src/main/ipc/github-repo-routing.ts @@ -26,54 +26,101 @@ type RegisteredRepoValidationResult = | { kind: 'ok'; repo: Repo } | { kind: 'denied'; reason: PRRefreshValidationDenialReason; message: string } +/** A registered-repo lookup: the caller's path and id, plus the host it believes owns the row. */ +type RegisteredRepoQuery = { + repoPath: string + repoId?: string | null + /** Picks between twins (one id on two hosts); a lone row's own fields still win (they may be stale). */ + hostHint?: Parameters[0] | null + sourceContext?: TaskSourceContext | null +} + function validateRegisteredRepo( - args: string | GitHubRepoScopedArgs, - store: Store, - repos = store.getRepos() + query: RegisteredRepoQuery, + repos: readonly Repo[] ): RegisteredRepoValidationResult { - const repoPath = typeof args === 'string' ? args : args.repoPath - const repoId = typeof args === 'string' ? undefined : args.repoId - const resolvedRepoPath = resolve(repoPath) - const repo = repos.find((candidate) => - repoId ? candidate.id === repoId : resolve(candidate.path) === resolvedRepoPath - ) - if (!repo) { + const { repoId, sourceContext } = query + const resolvedRepoPath = resolve(query.repoPath) + const idRows = repoId ? repos.filter((candidate) => candidate.id === repoId) : [] + if (repoId && idRows.length === 0) { return { kind: 'denied', reason: 'unknown-repo', message: 'Access denied: unknown repository path' } } - if (repoId && resolve(repo.path) !== resolvedRepoPath) { - return { - kind: 'denied', - reason: 'repo-path-mismatch', - message: 'Access denied: repository path does not match repo id' - } + // Why id and path together: a twin's rows share the id, so the first id match can be the other host. + const pathRows = (repoId ? idRows : repos).filter( + (candidate) => resolve(candidate.path) === resolvedRepoPath + ) + if (pathRows.length === 0) { + return repoId + ? { + kind: 'denied', + reason: 'repo-path-mismatch', + message: 'Access denied: repository path does not match repo id' + } + : { + kind: 'denied', + reason: 'unknown-repo', + message: 'Access denied: unknown repository path' + } } - if ( - typeof args !== 'string' && - args.sourceContext?.provider === 'github' && - args.sourceContext.hostId !== getRepoExecutionHostId(repo) - ) { - return { - kind: 'denied', - reason: 'host-mismatch', - message: 'Access denied: GitHub source host does not match repository host' - } + if (sourceContext?.provider === 'github') { + const repo = pathRows.find( + (candidate) => getRepoExecutionHostId(candidate) === sourceContext.hostId + ) + return repo + ? { kind: 'ok', repo } + : { + kind: 'denied', + reason: 'host-mismatch', + message: 'Access denied: GitHub source host does not match repository host' + } } + const hintHostId = query.hostHint ? getRepoExecutionHostId(query.hostHint) : null + const repo = + (hintHostId + ? pathRows.find((candidate) => getRepoExecutionHostId(candidate) === hintHostId) + : undefined) ?? pathRows[0] return { kind: 'ok', repo } } +function registeredRepoQuery(args: string | GitHubRepoScopedArgs): RegisteredRepoQuery { + return typeof args === 'string' + ? { repoPath: args } + : { repoPath: args.repoPath, repoId: args.repoId, sourceContext: args.sourceContext } +} + +function prRefreshCandidateQuery(candidate: GitHubPRRefreshCandidate): RegisteredRepoQuery { + return { + repoPath: candidate.repoPath, + repoId: candidate.repoId, + hostHint: { connectionId: candidate.connectionId, executionHostId: candidate.executionHostId } + } +} + +function assertRegisteredRepo(query: RegisteredRepoQuery, store: Store): Repo { + const result = validateRegisteredRepo(query, store.getRepos()) + if (result.kind === 'denied') { + throw new Error(result.message) + } + return result.repo +} + export function assertRegisteredGitHubRepo( args: string | GitHubRepoScopedArgs, store: Store ): Repo { - const result = validateRegisteredRepo(args, store) - if (result.kind === 'denied') { - throw new Error(result.message) - } - return result.repo + return assertRegisteredRepo(registeredRepoQuery(args), store) +} + +/** A manual PR refresh resolves its twin the way the automatic queue does. */ +export function assertRegisteredPRRefreshCandidateRepo( + candidate: GitHubPRRefreshCandidate, + store: Store +): Repo { + return assertRegisteredRepo(prRefreshCandidateQuery(candidate), store) } /** Pure: a project is still registered at the candidate's path on its execution host (the queue key's identity). */ @@ -129,7 +176,7 @@ export function validateAutomaticPRRefreshCandidate( ): | { kind: 'ok'; candidate: GitHubPRRefreshCandidate } | { kind: 'skipped'; result: Extract } { - const result = validateRegisteredRepo(candidate, store, repos) + const result = validateRegisteredRepo(prRefreshCandidateQuery(candidate), repos) if (result.kind === 'denied') { const skippedReason = notePRRefreshValidationDenial({ repoId: candidate.repoId,