mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 00:02:29 +00:00
fix(github): align PR source and review head origin (#10677)
* fix(github): align PR source and review head origin * 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> * test(github): enforce origin preference in review head origin resolution The explicit origin preference must short-circuit before any identity probe, so no remote queries should occur. Add validation to reject unexpected remotes and tighten the test assertion to verify no remote get-url calls happen at all. * fix(github): enforce origin preference in issue open-by-number lookup listWorkItems and getWorkItem must share preference so origin/upstream toggles cannot disagree. Explicit origin preference now fail-closes when origin identity is unresolved (no bare-lookup fallback), matching the PR candidate resolution rule. --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -0,0 +1,181 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type * as GithubApiRepositoryModule from './github-api-repository'
|
||||
import type * as GhUtils from './gh-utils'
|
||||
|
||||
const {
|
||||
ghExecFileAsyncMock,
|
||||
getOwnerRepoMock,
|
||||
getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemoteMock,
|
||||
resolvePRRepositoryCandidatesMock,
|
||||
resolveIssueSourceMock,
|
||||
rateLimitGuardMock,
|
||||
noteRateLimitSpendMock,
|
||||
acquireMock,
|
||||
releaseMock
|
||||
} = vi.hoisted(() => ({
|
||||
ghExecFileAsyncMock: vi.fn(),
|
||||
getOwnerRepoMock: vi.fn(),
|
||||
getIssueOwnerRepoMock: vi.fn(),
|
||||
getOwnerRepoForRemoteMock: vi.fn(),
|
||||
resolvePRRepositoryCandidatesMock: vi.fn(),
|
||||
resolveIssueSourceMock: vi.fn(),
|
||||
rateLimitGuardMock: vi.fn(() => ({ blocked: false })),
|
||||
noteRateLimitSpendMock: vi.fn(),
|
||||
acquireMock: vi.fn(),
|
||||
releaseMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('./gh-utils', async () => {
|
||||
const actual = await vi.importActual<typeof GhUtils>('./gh-utils')
|
||||
return {
|
||||
...actual,
|
||||
execFileAsync: vi.fn(),
|
||||
ghExecFileAsync: ghExecFileAsyncMock,
|
||||
getOwnerRepo: getOwnerRepoMock,
|
||||
getIssueOwnerRepo: getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
|
||||
resolveIssueSource: resolveIssueSourceMock,
|
||||
acquire: acquireMock,
|
||||
release: releaseMock,
|
||||
_resetOwnerRepoCache: vi.fn()
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock('./rate-limit', () => ({
|
||||
rateLimitGuard: rateLimitGuardMock,
|
||||
noteRateLimitSpend: noteRateLimitSpendMock,
|
||||
getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })),
|
||||
repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })),
|
||||
noteRepositoryRateLimitSpend: vi.fn(),
|
||||
spendsSharedGitHubComQuota: () => true
|
||||
}))
|
||||
|
||||
vi.mock('./github-api-repository', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof GithubApiRepositoryModule>()
|
||||
return {
|
||||
...actual,
|
||||
resolveIssueGitHubApiRepositorySource: (
|
||||
repoPath: string,
|
||||
preference: unknown,
|
||||
connectionId?: string | null,
|
||||
localGitOptions?: unknown
|
||||
) => resolveIssueSourceMock(repoPath, preference, connectionId, localGitOptions),
|
||||
getIssueGitHubApiRepository: (repoPath: string, connectionId?: string | null) =>
|
||||
getIssueOwnerRepoMock(repoPath, connectionId),
|
||||
getOriginGitHubApiRepository: (
|
||||
repoPath: string,
|
||||
connectionId?: string | null,
|
||||
localGitOptions?: unknown
|
||||
) => getOwnerRepoMock(repoPath, connectionId, localGitOptions),
|
||||
getGitHubApiRepositoryForRemote: (
|
||||
repoPath: string,
|
||||
remoteName: string,
|
||||
connectionId?: string | null,
|
||||
localGitOptions?: unknown
|
||||
) =>
|
||||
remoteName === 'origin'
|
||||
? getOwnerRepoMock(repoPath, connectionId, localGitOptions)
|
||||
: getOwnerRepoForRemoteMock(repoPath, remoteName, connectionId, localGitOptions),
|
||||
resolveGitHubApiRepositoryCandidates: (
|
||||
repoPath: string,
|
||||
connectionId?: string | null,
|
||||
localGitOptions?: unknown
|
||||
) => resolvePRRepositoryCandidatesMock(repoPath, connectionId, localGitOptions)
|
||||
}
|
||||
})
|
||||
|
||||
import { getWorkItem, _resetOwnerRepoCache } from './client'
|
||||
|
||||
describe('GitHub issue open-by-number origin preference', () => {
|
||||
beforeEach(() => {
|
||||
ghExecFileAsyncMock.mockReset()
|
||||
getOwnerRepoMock.mockReset()
|
||||
getIssueOwnerRepoMock.mockReset()
|
||||
getOwnerRepoForRemoteMock.mockReset()
|
||||
resolvePRRepositoryCandidatesMock.mockReset()
|
||||
resolveIssueSourceMock.mockReset()
|
||||
rateLimitGuardMock.mockReset()
|
||||
rateLimitGuardMock.mockReturnValue({ blocked: false })
|
||||
noteRateLimitSpendMock.mockReset()
|
||||
acquireMock.mockReset()
|
||||
releaseMock.mockReset()
|
||||
acquireMock.mockResolvedValue(undefined)
|
||||
getOwnerRepoForRemoteMock.mockImplementation(
|
||||
async (repoPath: string, remoteName: string, connectionId?: string | null, opts = {}) =>
|
||||
remoteName === 'origin' ? getOwnerRepoMock(repoPath, connectionId, opts) : null
|
||||
)
|
||||
resolvePRRepositoryCandidatesMock.mockImplementation(async (repoPath, connectionId) => {
|
||||
const origin = await getOwnerRepoMock(repoPath, connectionId)
|
||||
const repository = origin ? { host: 'github.com', ...origin } : null
|
||||
return { candidates: repository ? [repository] : [], headRepo: repository }
|
||||
})
|
||||
_resetOwnerRepoCache()
|
||||
})
|
||||
|
||||
it('pins typed issue metadata to explicit origin preference', async () => {
|
||||
const source = { owner: 'fork', repo: 'orca', host: 'github.com' }
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({ source, fellBack: false })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: JSON.stringify({
|
||||
number: 7,
|
||||
title: 'Origin issue',
|
||||
state: 'open',
|
||||
labels: [],
|
||||
url: 'https://github.com/fork/orca/issues/7',
|
||||
updatedAt: '2026-04-02T00:00:00Z',
|
||||
author: { login: 'octocat' }
|
||||
})
|
||||
})
|
||||
|
||||
const item = await getWorkItem('/repo-root', 7, 'issue', null, {}, 'origin')
|
||||
|
||||
expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'origin', null, {})
|
||||
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
|
||||
['api', 'repos/fork/orca/issues/7'],
|
||||
expect.objectContaining({ cwd: '/repo-root', host: 'github.com' })
|
||||
)
|
||||
expect(item).toMatchObject({ number: 7, title: 'Origin issue', type: 'issue' })
|
||||
})
|
||||
|
||||
it('does not run a bare issue lookup when explicit origin identity is unresolved', async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({ source: null, fellBack: false })
|
||||
|
||||
await expect(getWorkItem('/repo-root', 7, 'issue', null, {}, 'origin')).resolves.toBeNull()
|
||||
|
||||
expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'origin', null, {})
|
||||
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('skips the issue probe on untyped open when origin identity is unresolved', async () => {
|
||||
const origin = { owner: 'fork', repo: 'orca', host: 'github.com' }
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({ source: null, fellBack: false })
|
||||
getOwnerRepoMock.mockResolvedValue(origin)
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValue({ candidates: [origin], headRepo: origin })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: JSON.stringify({
|
||||
number: 7,
|
||||
title: 'Origin PR',
|
||||
state: 'open',
|
||||
labels: [],
|
||||
isDraft: false,
|
||||
url: 'https://github.com/fork/orca/pull/7',
|
||||
baseRefName: 'main',
|
||||
headRefName: 'origin/fix',
|
||||
updatedAt: '2026-04-02T00:00:00Z',
|
||||
author: { login: 'octocat' }
|
||||
})
|
||||
})
|
||||
|
||||
const item = await getWorkItem('/repo-root', 7, undefined, null, {}, 'origin')
|
||||
|
||||
expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'origin', null, {})
|
||||
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock.mock.calls[0]?.[0]).toEqual(
|
||||
expect.arrayContaining(['pr', 'view', '--repo', 'fork/orca'])
|
||||
)
|
||||
expect(item).toMatchObject({ number: 7, type: 'pr' })
|
||||
})
|
||||
})
|
||||
@@ -513,6 +513,54 @@ describe('GitHub issue source split', () => {
|
||||
expect(item?.prRepo).toEqual(upstream)
|
||||
})
|
||||
|
||||
it('pins typed PR metadata to explicit origin when upstream has the same number', async () => {
|
||||
const upstream = { owner: 'stablyai', repo: 'orca', host: 'github.com' }
|
||||
const origin = { owner: 'fork', repo: 'orca', host: 'github.com' }
|
||||
getOwnerRepoMock.mockResolvedValue(origin)
|
||||
mockUpstreamCandidate(upstream)
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValue({
|
||||
candidates: [upstream, origin],
|
||||
headRepo: origin
|
||||
})
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: JSON.stringify({
|
||||
number: 42,
|
||||
title: 'Origin PR',
|
||||
state: 'open',
|
||||
url: 'https://github.com/fork/orca/pull/42',
|
||||
labels: [],
|
||||
updatedAt: '2026-04-02T00:00:00Z',
|
||||
author: { login: 'octocat' },
|
||||
isDraft: false,
|
||||
headRefName: 'origin/fix',
|
||||
baseRefName: 'main'
|
||||
})
|
||||
})
|
||||
|
||||
const item = await getWorkItem('/repo-root', 42, 'pr', null, {}, 'origin')
|
||||
|
||||
expect(resolvePRRepositoryCandidatesMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock.mock.calls[0]?.[0]).toEqual(
|
||||
expect.arrayContaining(['pr', 'view', '--repo', 'fork/orca'])
|
||||
)
|
||||
expect(
|
||||
ghExecFileAsyncMock.mock.calls.some((call) =>
|
||||
(call[0] as string[]).some((arg) => arg.includes('upstream/orca'))
|
||||
)
|
||||
).toBe(false)
|
||||
expect(item?.prRepo).toEqual(origin)
|
||||
})
|
||||
|
||||
it('does not run a bare PR lookup when explicit origin identity is unresolved', async () => {
|
||||
getOwnerRepoMock.mockResolvedValue(null)
|
||||
mockUpstreamCandidate({ owner: 'stablyai', repo: 'orca' })
|
||||
|
||||
await expect(getWorkItem('/repo-root', 42, 'pr', null, {}, 'origin')).resolves.toBeNull()
|
||||
|
||||
expect(resolvePRRepositoryCandidatesMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not run a bare gh lookup for an SSH repo without candidates', async () => {
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ candidates: [], headRepo: null })
|
||||
|
||||
|
||||
@@ -3530,6 +3530,44 @@ describe('getPRForBranch', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('pins explicit origin push-target lookup when upstream has the same PR number', async () => {
|
||||
getOwnerRepoMock.mockResolvedValue({ owner: 'fork', repo: 'orca' })
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValue({
|
||||
candidates: [
|
||||
{ owner: 'upstream', repo: 'orca' },
|
||||
{ owner: 'fork', repo: 'orca' }
|
||||
],
|
||||
headRepo: { owner: 'fork', repo: 'orca' }
|
||||
})
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: JSON.stringify({
|
||||
head: {
|
||||
ref: 'contributor/fix',
|
||||
repo: {
|
||||
full_name: 'contributor/orca',
|
||||
name: 'orca',
|
||||
clone_url: 'https://github.com/contributor/orca.git',
|
||||
ssh_url: 'git@github.com:contributor/orca.git',
|
||||
owner: { login: 'contributor' }
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
getRemoteUrlForRepoMock.mockResolvedValueOnce('git@github.com:fork/orca.git')
|
||||
|
||||
await getPullRequestPushTarget('/repo-root', 1738, null, {}, 'origin')
|
||||
|
||||
expect(resolvePRRepositoryCandidatesMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(['api', 'repos/fork/orca/pulls/1738'], {
|
||||
cwd: '/repo-root',
|
||||
host: 'github.com'
|
||||
})
|
||||
expect(ghExecFileAsyncMock).not.toHaveBeenCalledWith(
|
||||
['api', 'repos/upstream/orca/pulls/1738'],
|
||||
expect.anything()
|
||||
)
|
||||
})
|
||||
|
||||
it('surfaces maintainer_can_modify=false alongside a fork PR push target', async () => {
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
|
||||
+57
-16
@@ -76,7 +76,6 @@ import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-d
|
||||
import { readLocalGitConfigSignature } from './local-git-config-signature'
|
||||
import {
|
||||
getGitHubApiRepositoryForRemote,
|
||||
getIssueGitHubApiRepository,
|
||||
getOriginGitHubApiRepository,
|
||||
githubHostExecOptions,
|
||||
githubRepositorySlugArg,
|
||||
@@ -283,16 +282,35 @@ 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,
|
||||
connectionId?: string | null,
|
||||
localGitOptions: LocalGitExecOptions = {}
|
||||
): Promise<GitHubApiRepository[]> {
|
||||
if (preference === 'origin') {
|
||||
const origin = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions)
|
||||
return origin ? [origin] : []
|
||||
}
|
||||
return (await resolveGitHubApiRepositoryCandidates(repoPath, connectionId, localGitOptions))
|
||||
.candidates
|
||||
}
|
||||
|
||||
export async function getPullRequestPushTarget(
|
||||
repoPath: string,
|
||||
prNumber: number,
|
||||
connectionId?: string | null,
|
||||
localGitOptions: LocalGitExecOptions = {}
|
||||
localGitOptions: LocalGitExecOptions = {},
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<PullRequestPushTarget | null> {
|
||||
const context = githubRepoContext(repoPath, connectionId, localGitOptions)
|
||||
const ghOptions = ghRepoExecOptions(context)
|
||||
const { candidates } = await resolveGitHubApiRepositoryCandidates(
|
||||
const candidates = await resolvePullRequestLookupCandidates(
|
||||
repoPath,
|
||||
preference,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
)
|
||||
@@ -954,14 +972,19 @@ async function fetchPullRequestWorkItemFromCandidates(
|
||||
repoPath: string,
|
||||
number: number,
|
||||
connectionId?: string | null,
|
||||
localGitOptions: LocalGitExecOptions = {}
|
||||
localGitOptions: LocalGitExecOptions = {},
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<MainWorkItem | null> {
|
||||
const { candidates } = await resolveGitHubApiRepositoryCandidates(
|
||||
const candidates = await resolvePullRequestLookupCandidates(
|
||||
repoPath,
|
||||
preference,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
)
|
||||
if (candidates.length === 0) {
|
||||
if (preference === 'origin') {
|
||||
return null
|
||||
}
|
||||
return fetchPullRequestWorkItem(repoPath, null, number, connectionId, localGitOptions)
|
||||
}
|
||||
for (const candidate of candidates) {
|
||||
@@ -1953,38 +1976,55 @@ export async function getWorkItem(
|
||||
number: number,
|
||||
type?: 'issue' | 'pr',
|
||||
connectionId?: string | null,
|
||||
localGitOptions: LocalGitExecOptions = {}
|
||||
localGitOptions: LocalGitExecOptions = {},
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<MainWorkItem | null> {
|
||||
await acquire()
|
||||
try {
|
||||
// Why: listWorkItems uses resolveIssueGitHubApiRepositorySource; open-by-number
|
||||
// must share that preference so origin/upstream toggles cannot disagree.
|
||||
if (type === 'issue') {
|
||||
return await fetchIssueWorkItem(
|
||||
const { source } = await resolveIssueGitHubApiRepositorySource(
|
||||
repoPath,
|
||||
await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions),
|
||||
number,
|
||||
preference,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
)
|
||||
// Why: explicit origin with no origin identity must not bare-lookup ambient gh
|
||||
// (same fail-closed rule as origin-pinned PR candidate resolution).
|
||||
if (!source && preference === 'origin') {
|
||||
return null
|
||||
}
|
||||
return await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions)
|
||||
}
|
||||
if (type === 'pr') {
|
||||
return await fetchPullRequestWorkItemFromCandidates(
|
||||
repoPath,
|
||||
number,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
localGitOptions,
|
||||
preference
|
||||
)
|
||||
}
|
||||
|
||||
try {
|
||||
const issue = await fetchIssueWorkItem(
|
||||
const { source } = await resolveIssueGitHubApiRepositorySource(
|
||||
repoPath,
|
||||
await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions),
|
||||
number,
|
||||
preference,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
)
|
||||
if (issue) {
|
||||
return issue
|
||||
if (source || preference !== 'origin') {
|
||||
const issue = await fetchIssueWorkItem(
|
||||
repoPath,
|
||||
source,
|
||||
number,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
)
|
||||
if (issue) {
|
||||
return issue
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
// Why: only fall through to PR #N on a genuine 404; re-throw transient errors so a flake can't surface an unrelated PR.
|
||||
@@ -1997,7 +2037,8 @@ export async function getWorkItem(
|
||||
repoPath,
|
||||
number,
|
||||
connectionId,
|
||||
localGitOptions
|
||||
localGitOptions,
|
||||
preference
|
||||
)
|
||||
} catch {
|
||||
return null
|
||||
|
||||
@@ -111,7 +111,13 @@ describe('resolveGitHubPrStartPoint', () => {
|
||||
resolveRemote: async () => 'origin'
|
||||
})
|
||||
|
||||
expect(getPullRequestPushTargetMock).toHaveBeenCalledWith('/repo-root', 1849, null)
|
||||
expect(getPullRequestPushTargetMock).toHaveBeenCalledWith(
|
||||
'/repo-root',
|
||||
1849,
|
||||
null,
|
||||
{},
|
||||
undefined
|
||||
)
|
||||
expect(result).toEqual({
|
||||
baseBranch: 'def456',
|
||||
headSha: 'def456',
|
||||
@@ -144,7 +150,13 @@ describe('resolveGitHubPrStartPoint', () => {
|
||||
resolveRemote: async () => 'origin'
|
||||
})
|
||||
|
||||
expect(getPullRequestPushTargetMock).toHaveBeenCalledWith('/repo-root', 1849, null)
|
||||
expect(getPullRequestPushTargetMock).toHaveBeenCalledWith(
|
||||
'/repo-root',
|
||||
1849,
|
||||
null,
|
||||
{},
|
||||
undefined
|
||||
)
|
||||
expect(fetchPullRequestHeadRefMock).toHaveBeenCalledWith('origin', 1849)
|
||||
expect(result).toEqual({
|
||||
baseBranch: 'abc123',
|
||||
@@ -383,13 +395,21 @@ describe('resolveGitHubPrStartPoint', () => {
|
||||
const result = await resolveGitHubPrStartPoint({
|
||||
repoPath: '/repo-root',
|
||||
prNumber: 1738,
|
||||
issueSourcePreference: 'origin',
|
||||
gitExec,
|
||||
fetchRemoteTrackingRef,
|
||||
fetchPullRequestHeadRef: fetchPullRequestHeadRefMock,
|
||||
resolveRemote: async () => 'origin'
|
||||
})
|
||||
|
||||
expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 1738, 'pr', null)
|
||||
expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 1738, 'pr', null, {}, 'origin')
|
||||
expect(getPullRequestPushTargetMock).toHaveBeenCalledWith(
|
||||
'/repo-root',
|
||||
1738,
|
||||
null,
|
||||
{},
|
||||
'origin'
|
||||
)
|
||||
expect(result).toEqual({
|
||||
baseBranch: 'abc123',
|
||||
compareBaseRef: 'refs/remotes/origin/main',
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import type { GitHubPrStartPoint, GitPushTarget } from '../../shared/types'
|
||||
import type { GitHubPrStartPoint, GitPushTarget, IssueSourcePreference } from '../../shared/types'
|
||||
import { fetchCompareBaseRefWithLocalFallback } from '../git/compare-base-ref-fetch'
|
||||
import {
|
||||
isMissingRemoteRefGitError,
|
||||
@@ -18,6 +18,7 @@ type ResolveGitHubPrStartPointArgs = {
|
||||
headRefName?: string
|
||||
baseRefName?: string
|
||||
isCrossRepository?: boolean
|
||||
issueSourcePreference?: IssueSourcePreference
|
||||
connectionId?: string | null
|
||||
localGitOptions?: { wslDistro?: string }
|
||||
gitExec: GitExec
|
||||
@@ -30,12 +31,6 @@ type ResolveGitHubPrStartPointArgs = {
|
||||
|
||||
type ResolveGitHubPrStartPointResult = GitHubPrStartPoint | { error: string }
|
||||
|
||||
function localGitOptionArgs(
|
||||
options: { wslDistro?: string } | undefined
|
||||
): [] | [{ wslDistro?: string }] {
|
||||
return options && Object.keys(options).length > 0 ? [options] : []
|
||||
}
|
||||
|
||||
export async function resolveGitHubPrStartPoint(
|
||||
args: ResolveGitHubPrStartPointArgs
|
||||
): Promise<ResolveGitHubPrStartPointResult> {
|
||||
@@ -54,7 +49,8 @@ export async function resolveGitHubPrStartPoint(
|
||||
args.repoPath,
|
||||
args.prNumber,
|
||||
args.connectionId ?? null,
|
||||
...localGitOptionArgs(args.localGitOptions)
|
||||
args.localGitOptions ?? {},
|
||||
args.issueSourcePreference
|
||||
)
|
||||
pushTarget = resolved?.pushTarget
|
||||
maintainerCanModify = resolved?.maintainerCanModify
|
||||
@@ -71,7 +67,8 @@ export async function resolveGitHubPrStartPoint(
|
||||
args.prNumber,
|
||||
'pr',
|
||||
args.connectionId ?? null,
|
||||
...localGitOptionArgs(args.localGitOptions)
|
||||
args.localGitOptions ?? {},
|
||||
args.issueSourcePreference
|
||||
)
|
||||
if (!item || item.type !== 'pr') {
|
||||
return { error: `PR #${args.prNumber} not found.` }
|
||||
|
||||
@@ -38,6 +38,7 @@ describe('resolveGitHubReviewHeadRemote', () => {
|
||||
|
||||
const remote = await resolveGitHubReviewHeadRemote({
|
||||
repoPath: '/repo',
|
||||
issueSourcePreference: 'auto',
|
||||
gitExec: gitExecWithRemotes(['origin', 'upstream'])
|
||||
})
|
||||
|
||||
@@ -52,12 +53,40 @@ describe('resolveGitHubReviewHeadRemote', () => {
|
||||
|
||||
const remote = await resolveGitHubReviewHeadRemote({
|
||||
repoPath: '/repo',
|
||||
issueSourcePreference: 'upstream',
|
||||
gitExec: gitExecWithRemotes(['origin', 'upstream'])
|
||||
})
|
||||
|
||||
expect(remote).toBe('origin')
|
||||
})
|
||||
|
||||
it('uses explicit origin without probing hosting identity on a dual-remote clone', async () => {
|
||||
getGitHubApiRepositoryForRemoteMock.mockResolvedValue({ owner: 'org', repo: 'project' })
|
||||
|
||||
const remote = await resolveGitHubReviewHeadRemote({
|
||||
repoPath: '/repo',
|
||||
issueSourcePreference: 'origin',
|
||||
gitExec: gitExecWithRemotes(['origin', 'upstream'])
|
||||
})
|
||||
|
||||
expect(remote).toBe('origin')
|
||||
expect(getGitHubApiRepositoryForRemoteMock).not.toHaveBeenCalled()
|
||||
expect(getDefaultRemoteMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rejects explicit origin when that remote is not configured', async () => {
|
||||
await expect(
|
||||
resolveGitHubReviewHeadRemote({
|
||||
repoPath: '/repo',
|
||||
issueSourcePreference: 'origin',
|
||||
gitExec: gitExecWithRemotes(['upstream'])
|
||||
})
|
||||
).rejects.toThrow('Repo has no configured origin remote.')
|
||||
|
||||
expect(getGitHubApiRepositoryForRemoteMock).not.toHaveBeenCalled()
|
||||
expect(getDefaultRemoteMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('skips identity probes for a single-remote clone and uses the local default', async () => {
|
||||
getDefaultRemoteMock.mockResolvedValue('origin')
|
||||
|
||||
|
||||
@@ -1,16 +1,15 @@
|
||||
import type { IssueSourcePreference } from '../../shared/types'
|
||||
import { pickPreferredGitRemote } from '../../shared/preferred-git-remote'
|
||||
import { getDefaultRemote } from '../git/repo'
|
||||
import { getGitHubApiRepositoryForRemote } from './github-api-repository'
|
||||
|
||||
type GitExec = (args: string[]) => Promise<{ stdout: string; stderr: string }>
|
||||
|
||||
// Why: PR work-item/API resolution probes upstream before origin
|
||||
// (resolveGitHubApiRepositoryCandidates), so review-head fetches must target
|
||||
// the same hosting project — a contributor clone's fork `origin` has no
|
||||
// refs/pull/<N>/head for an upstream PR. Local and SSH share this resolver so
|
||||
// the two surfaces cannot pick different remotes.
|
||||
// Why: explicit origin must match issue listing; otherwise hosting identity
|
||||
// keeps contributor clones on the upstream project's PR namespace.
|
||||
export async function resolveGitHubReviewHeadRemote(args: {
|
||||
repoPath: string
|
||||
issueSourcePreference?: IssueSourcePreference
|
||||
connectionId?: string | null
|
||||
localGitOptions?: { wslDistro?: string }
|
||||
gitExec: GitExec
|
||||
@@ -20,6 +19,12 @@ export async function resolveGitHubReviewHeadRemote(args: {
|
||||
.split(/\r?\n/)
|
||||
.map((line) => line.trim())
|
||||
.filter(Boolean)
|
||||
if (args.issueSourcePreference === 'origin') {
|
||||
if (remotes.includes('origin')) {
|
||||
return 'origin'
|
||||
}
|
||||
throw new Error('Repo has no configured origin remote.')
|
||||
}
|
||||
// Why: identity probes cost a `remote get-url` (plus a possible gh auth
|
||||
// lookup) each; only multi-remote clones are ambiguous enough to need them.
|
||||
if (remotes.length > 1) {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user