From 6b3ef27d4d5e8b9a581c90cd7c2c40dd1c5b5bfb Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 25 Jul 2026 22:16:41 -0700 Subject: [PATCH] fix(github): pin number-based work item open to the repo source preference Open-by-number and details still ran the upstream-first multi-candidate PR probe, so a fork and its upstream sharing a PR number opened different PRs than the list and start-point paths did once #10677 pinned those to origin. Thread repo.issueSourcePreference through dispatchWorkItem, getWorkItemDetails, getRepoWorkItem, and getRepoWorkItemDetails. getWorkItemByOwnerRepo is left alone: explicit owner/repo already pins identity. auto/upstream/undefined keep the multi-candidate probe. Co-authored-by: Orca --- src/main/github/client.ts | 3 ++ .../work-item-details-enterprise-host.test.ts | 8 +-- src/main/github/work-item-details.test.ts | 37 +++++++++++-- src/main/github/work-item-details.ts | 7 ++- src/main/ipc/github-work-item-args.test.ts | 35 ++++++++++-- src/main/ipc/github-work-item-args.ts | 16 +++++- src/main/ipc/github.test.ts | 54 ++++++++++++++++++- src/main/runtime/orca-runtime.test.ts | 45 +++++++++++++++- src/main/runtime/orca-runtime.ts | 8 ++- 9 files changed, 192 insertions(+), 21 deletions(-) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index db9e2c051b5..bda85399cbd 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -283,6 +283,9 @@ export type PullRequestPushTarget = { maintainerCanModify?: boolean } +// Why: only an explicit `origin` preference is origin-only; `upstream`/`auto`/ +// undefined keep the multi-candidate probe ordered upstream-first, matching +// resolvePrWorkItemSource list semantics. async function resolvePullRequestLookupCandidates( repoPath: string, preference: IssueSourcePreference | undefined, diff --git a/src/main/github/work-item-details-enterprise-host.test.ts b/src/main/github/work-item-details-enterprise-host.test.ts index aca6e4a7c1c..be2576051b5 100644 --- a/src/main/github/work-item-details-enterprise-host.test.ts +++ b/src/main/github/work-item-details-enterprise-host.test.ts @@ -137,7 +137,7 @@ describe('getWorkItemDetails Enterprise host routing', () => { const details = await getWorkItemDetails('/remote/repo', 7, 'issue', 'ssh-1') expect(details?.body).toBe('Enterprise issue body') - expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'issue', 'ssh-1') + expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'issue', 'ssh-1', {}, undefined) expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith(enterpriseRepository, 'graphql', { @@ -156,7 +156,7 @@ describe('getWorkItemDetails Enterprise host routing', () => { await expect(getWorkItemDetails('/remote/repo', 7, 'issue', 'ssh-1')).resolves.toBeNull() - expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'issue', 'ssh-1') + expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'issue', 'ssh-1', {}, undefined) expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) @@ -247,7 +247,7 @@ describe('getWorkItemDetails Enterprise host routing', () => { viewerViewedState: 'VIEWED' } ]) - expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'pr', 'ssh-1') + expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'pr', 'ssh-1', {}, undefined) expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() expect(getPRCommentsMock).toHaveBeenCalledWith( '/remote/repo', @@ -280,7 +280,7 @@ describe('getWorkItemDetails Enterprise host routing', () => { await expect(getWorkItemDetails('/remote/repo', 7, 'pr', 'ssh-1')).resolves.toBeNull() - expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'pr', 'ssh-1') + expect(getWorkItemMock).toHaveBeenCalledWith('/remote/repo', 7, 'pr', 'ssh-1', {}, undefined) expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index bc2bef2a722..11a063b51ba 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -206,7 +206,14 @@ describe('getWorkItemDetails', () => { const details = await getWorkItemDetails('/repo-root', 923, 'issue') - expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 923, 'issue', undefined) + expect(getWorkItemMock).toHaveBeenCalledWith( + '/repo-root', + 923, + 'issue', + undefined, + {}, + undefined + ) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(ghExecFileAsyncMock.mock.calls[0][0][0]).toBe('api') expect(ghExecFileAsyncMock.mock.calls[0][0][1]).toBe('graphql') @@ -579,7 +586,14 @@ describe('getWorkItemDetails', () => { const details = await getWorkItemDetails('/home/tester/widgets', 923, 'issue', 'ssh-test-1') - expect(getWorkItemMock).toHaveBeenCalledWith('/home/tester/widgets', 923, 'issue', 'ssh-test-1') + expect(getWorkItemMock).toHaveBeenCalledWith( + '/home/tester/widgets', + 923, + 'issue', + 'ssh-test-1', + {}, + undefined + ) expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith( '/home/tester/widgets', 'upstream', @@ -649,7 +663,14 @@ describe('getWorkItemDetails', () => { const details = await getWorkItemDetails('/repo-root', 42, 'pr', null, localGitOptions) expect(details?.body).toBe('PR body') - expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 42, 'pr', null, localGitOptions) + expect(getWorkItemMock).toHaveBeenCalledWith( + '/repo-root', + 42, + 'pr', + null, + localGitOptions, + undefined + ) expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith( '/repo-root', 'origin', @@ -677,6 +698,16 @@ describe('getWorkItemDetails', () => { ) }) + // Why: details open by number, so it must pin the same source as the list; + // otherwise a fork and its upstream sharing PR #42 render different PRs. + it('forwards the explicit origin source preference to the work item lookup', async () => { + getWorkItemMock.mockResolvedValueOnce(null) + + await expect(getWorkItemDetails('/repo-root', 42, 'pr', null, {}, 'origin')).resolves.toBeNull() + + expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 42, 'pr', null, {}, 'origin') + }) + // Why: a rate-limited/auth-failed file fetch must not render as an empty PR; // the Files tab keys its retry state off details.filesUnavailable. it('flags filesUnavailable when the PR file fetch fails but leaves the PR empty otherwise intact', async () => { diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 5e389be8581..2df6d9200e9 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -8,6 +8,7 @@ import type { GitHubIssueTimelineTarget, GitHubWorkItem, GitHubWorkItemDetails, + IssueSourcePreference, PRCheckDetail, PRComment } from '../../shared/types' @@ -1047,14 +1048,16 @@ export async function getWorkItemDetails( number: number, type?: 'issue' | 'pr', connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + preference?: IssueSourcePreference ): Promise { const item: Omit | null = await getWorkItem( repoPath, number, type, connectionId, - ...localGitOptionArgs(localGitOptions) + localGitOptions, + preference ) if (!item) { return null diff --git a/src/main/ipc/github-work-item-args.test.ts b/src/main/ipc/github-work-item-args.test.ts index 234906a376e..8e253bc95cc 100644 --- a/src/main/ipc/github-work-item-args.test.ts +++ b/src/main/ipc/github-work-item-args.test.ts @@ -33,19 +33,19 @@ describe('dispatchWorkItem', () => { type: 'bogus' as unknown as 'issue' | 'pr' } await dispatchWorkItem(bogus, repo, fn) - expect(fn).toHaveBeenCalledWith('/r', 42, undefined, null, undefined) + expect(fn).toHaveBeenCalledWith('/r', 42, undefined, null, undefined, undefined) }) it('passes valid issue type through', async () => { const fn = vi.fn().mockResolvedValue(null) await dispatchWorkItem({ repoPath: '/r', number: 42, type: 'issue' }, repo, fn) - expect(fn).toHaveBeenCalledWith('/r', 42, 'issue', null, undefined) + expect(fn).toHaveBeenCalledWith('/r', 42, 'issue', null, undefined, undefined) }) it('passes valid pr type through', async () => { const fn = vi.fn().mockResolvedValue(null) await dispatchWorkItem({ repoPath: '/r', number: 42, type: 'pr' }, repo, fn) - expect(fn).toHaveBeenCalledWith('/r', 42, 'pr', null, undefined) + expect(fn).toHaveBeenCalledWith('/r', 42, 'pr', null, undefined, undefined) }) it('passes SSH connection context through', async () => { @@ -55,6 +55,33 @@ describe('dispatchWorkItem', () => { { path: '/remote/repo', connectionId: 'ssh-1' }, fn ) - expect(fn).toHaveBeenCalledWith('/remote/repo', 42, 'issue', 'ssh-1', undefined) + expect(fn).toHaveBeenCalledWith('/remote/repo', 42, 'issue', 'ssh-1', undefined, undefined) + }) + + it('pins the repo issue source preference for open-by-number', async () => { + const fn = vi.fn().mockResolvedValue(null) + await dispatchWorkItem( + { repoPath: '/r', number: 42, type: 'pr' }, + { path: '/r', connectionId: null, issueSourcePreference: 'origin' }, + fn, + { wslDistro: 'Ubuntu' } + ) + expect(fn).toHaveBeenCalledWith('/r', 42, 'pr', null, { wslDistro: 'Ubuntu' }, 'origin') + }) + + it('leaves upstream and auto preferences on the multi-candidate probe', async () => { + const fn = vi.fn().mockResolvedValue(null) + await dispatchWorkItem( + { repoPath: '/r', number: 7, type: 'pr' }, + { path: '/r', connectionId: null, issueSourcePreference: 'upstream' }, + fn + ) + await dispatchWorkItem( + { repoPath: '/r', number: 7, type: 'pr' }, + { path: '/r', connectionId: null, issueSourcePreference: 'auto' }, + fn + ) + expect(fn).toHaveBeenNthCalledWith(1, '/r', 7, 'pr', null, undefined, 'upstream') + expect(fn).toHaveBeenNthCalledWith(2, '/r', 7, 'pr', null, undefined, 'auto') }) }) diff --git a/src/main/ipc/github-work-item-args.ts b/src/main/ipc/github-work-item-args.ts index e3dc63b2e9e..0aff921ee24 100644 --- a/src/main/ipc/github-work-item-args.ts +++ b/src/main/ipc/github-work-item-args.ts @@ -1,4 +1,5 @@ import type { TaskSourceContext } from '../../shared/task-source-context' +import type { IssueSourcePreference } from '../../shared/types' export type WorkItemArgs = { repoPath: string @@ -11,6 +12,7 @@ export type WorkItemArgs = { type RegisteredRepoContext = { path: string connectionId?: string | null + issueSourcePreference?: IssueSourcePreference } type LocalGitExecOptions = { @@ -29,7 +31,8 @@ export function dispatchWorkItem( n: number, t?: 'issue' | 'pr', connectionId?: string | null, - localGitOptions?: LocalGitExecOptions + localGitOptions?: LocalGitExecOptions, + preference?: IssueSourcePreference ) => Promise, localGitOptions?: LocalGitExecOptions ): Promise | null { @@ -38,5 +41,14 @@ export function dispatchWorkItem( return null } const safeType = type === 'issue' || type === 'pr' ? type : undefined - return fn(repo.path, number, safeType, repo.connectionId ?? null, localGitOptions) + // Why: open-by-number must pin the same source the list and start-point use, + // else a fork and its upstream sharing a PR number resolve to different PRs. + return fn( + repo.path, + number, + safeType, + repo.connectionId ?? null, + localGitOptions, + repo.issueSourcePreference + ) } diff --git a/src/main/ipc/github.test.ts b/src/main/ipc/github.test.ts index bb1dd0cc1f6..bbd26f84d6a 100644 --- a/src/main/ipc/github.test.ts +++ b/src/main/ipc/github.test.ts @@ -843,6 +843,48 @@ describe('registerGitHubHandlers', () => { ) }) + // Why: open-by-number must pin the same source the list uses, else a fork and + // its upstream sharing PR #42 open different PRs from the same click. + it('pins the repo origin source preference on work item and details IPC', async () => { + repos = [ + { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + issueSourcePreference: 'origin' + } + ] + getWorkItemMock.mockResolvedValue(null) + getWorkItemDetailsMock.mockResolvedValue(null) + registerGitHubHandlers(store as never, stats as never) + + await handlers['gh:workItem'](null, { repoPath: '/workspace/repo', number: 42, type: 'pr' }) + await handlers['gh:workItemDetails'](null, { + repoPath: '/workspace/repo', + number: 42, + type: 'pr' + }) + + expect(getWorkItemMock).toHaveBeenCalledWith( + '/workspace/repo', + 42, + 'pr', + null, + undefined, + 'origin' + ) + expect(getWorkItemDetailsMock).toHaveBeenCalledWith( + '/workspace/repo', + 42, + 'pr', + null, + undefined, + 'origin' + ) + }) + it('routes local WSL project GitHub PR detail and action IPC through project git options', async () => { setPlatform('win32') projects = [ @@ -1027,7 +1069,14 @@ describe('registerGitHubHandlers', () => { } ) - expect(getWorkItemMock).toHaveBeenCalledWith('/workspace/repo', 42, 'pr', null, localGitOptions) + expect(getWorkItemMock).toHaveBeenCalledWith( + '/workspace/repo', + 42, + 'pr', + null, + localGitOptions, + undefined + ) expect(getWorkItemByOwnerRepoMock).toHaveBeenCalledWith( '/workspace/repo', prRepo, @@ -1041,7 +1090,8 @@ describe('registerGitHubHandlers', () => { 42, 'pr', null, - localGitOptions + localGitOptions, + undefined ) expect(getPRFileContentsMock).toHaveBeenCalledWith( expect.objectContaining({ repoPath: '/workspace/repo', localGitOptions, prRepo }) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index fe0ddbc6fdb..52717e22489 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -6123,6 +6123,45 @@ describe('OrcaRuntimeService', () => { ) }) + it('pins explicit origin preference on runtime open-by-number work item lookups', async () => { + const originRepo = { + id: TEST_REPO_ID, + path: TEST_REPO_PATH, + displayName: 'repo', + badgeColor: 'blue', + addedAt: 1, + issueSourcePreference: 'origin' as const + } + const runtime = new OrcaRuntimeService({ + ...store, + getRepos: () => [originRepo], + getRepo: (id: string) => (id === originRepo.id ? originRepo : undefined) + } as never) + const prRepo = { owner: 'acme', repo: 'orca' } + + await runtime.getRepoWorkItem('id:repo-1', 42, 'pr') + await runtime.getRepoWorkItemDetails('id:repo-1', 42, 'pr') + await runtime.getRepoWorkItemByOwnerRepo('id:repo-1', prRepo, 42, 'pr') + + expect(getGitHubWorkItemMock).toHaveBeenCalledWith(TEST_REPO_PATH, 42, 'pr', null, {}, 'origin') + expect(getGitHubWorkItemDetailsMock).toHaveBeenCalledWith( + TEST_REPO_PATH, + 42, + 'pr', + null, + {}, + 'origin' + ) + // Why: explicit owner/repo already pins identity, so it stays preference-free. + expect(getGitHubWorkItemByOwnerRepoMock).toHaveBeenCalledWith( + TEST_REPO_PATH, + prRepo, + 42, + 'pr', + null + ) + }) + it('routes runtime GitHub PR details and actions through the selected WSL project runtime', async () => { setPlatform('win32') const runtimeStore = { @@ -6220,7 +6259,8 @@ describe('OrcaRuntimeService', () => { 42, 'pr', null, - localGitOptions + localGitOptions, + undefined ) expect(getGitHubWorkItemByOwnerRepoMock).toHaveBeenCalledWith( TEST_REPO_PATH, @@ -6235,7 +6275,8 @@ describe('OrcaRuntimeService', () => { 42, 'pr', null, - localGitOptions + localGitOptions, + undefined ) expect(getGitHubPRChecksMock).toHaveBeenCalledWith( TEST_REPO_PATH, diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 95c6f873f0f..7e7afd44893 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -16139,12 +16139,15 @@ export class OrcaRuntimeService { type?: 'issue' | 'pr' ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) + // Why: open-by-number must pin the same source the list and start-point use, + // else a fork and its upstream sharing a PR number resolve to different PRs. return getWorkItem( repo.path, number, type, repo.connectionId ?? null, - ...this.getLocalGitExecutionOptionArgs(repo) + this.getLocalGitExecutionOptionArgs(repo)[0] ?? {}, + repo.issueSourcePreference ) } @@ -16176,7 +16179,8 @@ export class OrcaRuntimeService { number, type, repo.connectionId ?? null, - ...this.getLocalGitExecutionOptionArgs(repo) + this.getLocalGitExecutionOptionArgs(repo)[0] ?? {}, + repo.issueSourcePreference ) }