mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 08:03:13 +00:00
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 <help@stably.ai>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<GitHubWorkItemDetails | null> {
|
||||
const item: Omit<GitHubWorkItem, 'repoId'> | null = await getWorkItem(
|
||||
repoPath,
|
||||
number,
|
||||
type,
|
||||
connectionId,
|
||||
...localGitOptionArgs(localGitOptions)
|
||||
localGitOptions,
|
||||
preference
|
||||
)
|
||||
if (!item) {
|
||||
return null
|
||||
|
||||
@@ -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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<T>(
|
||||
n: number,
|
||||
t?: 'issue' | 'pr',
|
||||
connectionId?: string | null,
|
||||
localGitOptions?: LocalGitExecOptions
|
||||
localGitOptions?: LocalGitExecOptions,
|
||||
preference?: IssueSourcePreference
|
||||
) => Promise<T | null>,
|
||||
localGitOptions?: LocalGitExecOptions
|
||||
): Promise<T | null> | null {
|
||||
@@ -38,5 +41,14 @@ export function dispatchWorkItem<T>(
|
||||
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
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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 })
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -16139,12 +16139,15 @@ export class OrcaRuntimeService {
|
||||
type?: 'issue' | 'pr'
|
||||
): Promise<Awaited<ReturnType<typeof getWorkItem>>> {
|
||||
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
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user