Files
orca/src/main/github/client-create-pr.test.ts
T
Neil d05dd8ef50 fix(source-control): route hosted reviews by resolved execution host (#18382)
`ForgeProvider.createReview(repoPath, input, connectionId, options)` and the
`connectionId` on `ForgeProviderRepositoryContext` carried the same collapse the
five prior migrations closed: `string | null` spells "genuinely local", "runtime
host" and "could not resolve" with one value. Because it was decided two layers
up -- `repo.connectionId ?? null` at the `hostedReview:*` IPC handlers and in
`RuntimeHostedReviewCommands` -- a row naming its owner only as
`executionHostId: ssh:<target>` ran the whole review path against this machine's
copy of a remote path (#11163): `git rev-parse`, `git status`, the base-on-remote
ref probe, the upstream divergence read, and `gh`/`glab` with no host flags.

Replace it with a required `ExecutionHostId` threaded from the decision point
through the contract, routed by #18296's `resolveGitRouteForHost`. The parameter
is removed rather than added beside, so all five implementations -- GitLab,
GitHub, Bitbucket, Azure DevOps, Gitea -- and every caller became a compile
error. None of these families carries `@ts-nocheck`, so unlike #18325 that
guarantee is real here; `orca-runtime-file-commands.ts` does, but it only
constructs `RuntimeHostedReviewCommands` with unchanged deps.

Also fixed at the sites:

- The branch cache scoped entries on `connectionId ?? ''`, so two rows at one
  path on different hosts shared one cached review, one backoff deadline and one
  invalidation. Keyed on the resolved host now, as #18377 did for its probe key.
- `hostedReview:create` resolved shared symlink paths and normalized worktree
  paths off the raw field, so an `executionHostId`-only SSH row read `orca.yaml`
  and `resolve()`d a remote POSIX path on the client. Those ask the file-holder
  question -- `getRepoSshConnectionId` -- not the dialable one.
- An SSH host with no provider now refuses inside the git-state layer instead of
  reaching the local branch, keeping "remote and unreachable" distinct from
  "local" (docs/reference/ssh-execution-boundary.md).

`runtime:` is a routing mistake inside `hostedReviewSshConnectionId` -- that
environment's server runs its own git, and the SSH target on its repo row is
nested in that server's namespace, so dialing it here reaches a same-named box of
ours. But store-backed callers ask `getRepoHostedReviewExecutionHostId` first,
which is "what may this client dial" and answers `local` for a `runtime:` row.
That is deliberate and matches #18377: the runtime registration controller only
adopts a `runtime:` stamp onto a row with no `connectionId`
(`runtimeRepoMatchesExecutionHost` refuses to match an SSH row), so the checkout
really is in this process and refusing would regress a runtime server creating
reviews for its own rows.

No wire change. `connectionId` on `CreateHostedReviewArgs`,
`CreateStackedHostedReviewArgs` and `HostedReviewCreationEligibilityArgs` in
src/shared/hosted-review.ts is untouched -- every host already ignores it in
favor of the repo row, and removing it from the request types would only churn
the schema older clients still populate. The main-side eligibility input `Omit`s
it so nothing on this side can read the ambiguous field again.
2026-09-03 01:32:46 -07:00

532 lines
16 KiB
TypeScript

import { beforeEach, describe, expect, it, vi } from 'vitest'
import { readFile } from 'node:fs/promises'
const {
ghExecFileAsyncMock,
getOwnerRepoMock,
getOwnerRepoForRemoteMock,
getEnterpriseGitHubRepoSlugMock,
extractExecErrorMock,
acquireMock,
releaseMock,
getSshFilesystemProviderMock
} = vi.hoisted(() => ({
ghExecFileAsyncMock: vi.fn(),
getOwnerRepoMock: vi.fn(),
getOwnerRepoForRemoteMock: vi.fn(),
getEnterpriseGitHubRepoSlugMock: vi.fn(),
extractExecErrorMock: vi.fn((error: unknown) => {
const value = error as { stderr?: string; stdout?: string; message?: string }
return {
stderr: value?.stderr ?? value?.message ?? '',
stdout: value?.stdout ?? ''
}
}),
acquireMock: vi.fn(),
releaseMock: vi.fn(),
getSshFilesystemProviderMock: vi.fn()
}))
vi.mock('../providers/ssh-filesystem-dispatch', () => ({
getSshFilesystemProvider: getSshFilesystemProviderMock
}))
vi.mock('./gh-utils', () => ({
execFileAsync: vi.fn(),
ghExecFileAsync: ghExecFileAsyncMock,
getOwnerRepo: getOwnerRepoMock,
getIssueOwnerRepo: vi.fn(),
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
githubRepoContext: vi.fn((repoPath: string, connectionId?: string | null) => ({
repoPath,
connectionId: connectionId ?? null
})),
ghRepoExecOptions: vi.fn((context: { repoPath: string; connectionId?: string | null }) =>
context.connectionId ? {} : { cwd: context.repoPath }
),
gitExecFileAsync: vi.fn(),
extractExecError: extractExecErrorMock,
parseGitHubOwnerRepo: vi.fn(),
acquire: acquireMock,
release: releaseMock,
_resetOwnerRepoCache: vi.fn()
}))
vi.mock('../git/runner', () => ({
gitExecFileAsync: vi.fn()
}))
vi.mock('./github-enterprise-repository', () => ({
getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock,
isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true)
}))
import { createGitHubPullRequest } from './client'
import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository'
// The origin-repository cache is module-level state; reset it so slugs
// resolved by one test cannot leak into the next.
beforeEach(() => {
_resetOriginGitHubApiRepositoryCache()
})
describe('createGitHubPullRequest', () => {
beforeEach(() => {
ghExecFileAsyncMock.mockReset()
getOwnerRepoMock.mockReset()
getOwnerRepoForRemoteMock.mockReset()
// Why: createGitHubPullRequest resolves its target via the explicit origin
// remote (getOwnerRepo became upstream-first in #7331). Delegate the origin
// probe to getOwnerRepoMock so existing tests keep defining it there.
getOwnerRepoForRemoteMock.mockImplementation(
async (repoPath: string, remoteName: string, connectionId?: string | null, opts = {}) =>
remoteName === 'origin' ? getOwnerRepoMock(repoPath, connectionId, opts) : null
)
getEnterpriseGitHubRepoSlugMock.mockReset()
getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null)
extractExecErrorMock.mockClear()
acquireMock.mockReset()
releaseMock.mockReset()
getSshFilesystemProviderMock.mockReset()
acquireMock.mockResolvedValue(undefined)
})
it('creates a GitHub pull request with normalized refs and a body file', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 42,
url: 'https://github.com/acme/widgets/pull/42'
})
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'origin/main',
head: 'refs/heads/feature/create-pr',
title: ' Create PR UI ',
body: 'Body text',
draft: true
},
'local'
)
).resolves.toEqual({
ok: true,
number: 42,
url: 'https://github.com/acme/widgets/pull/42'
})
const [args, options] = ghExecFileAsyncMock.mock.calls[0]
expect(args).toEqual(
expect.arrayContaining([
'pr',
'create',
'--repo',
'acme/widgets',
'--base',
'main',
'--head',
'feature/create-pr',
'--title',
'Create PR UI',
'--draft'
])
)
expect(args[args.indexOf('--body-file') + 1]).toMatch(/body\.md$/)
expect(options).toMatchObject({
cwd: '/repo-root',
timeout: 60_000,
idempotent: false
})
expect(acquireMock).toHaveBeenCalledOnce()
expect(releaseMock).toHaveBeenCalledOnce()
})
it('targets the origin fork (not the upstream parent) on a fork checkout (#7331)', async () => {
// Fork checkout: origin is the personal fork, upstream is the parent. The
// head branch is unqualified and lives on the fork, so `gh pr create` must
// run with --repo <fork> even though PR reads prefer upstream since #7331.
getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) =>
remoteName === 'origin'
? { owner: 'fsdwen', repo: 'orca' }
: { owner: 'stablyai', repo: 'orca' }
)
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 5,
url: 'https://github.com/fsdwen/orca/pull/5'
})
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'my-branch',
title: 'Fork PR'
},
'local'
)
).resolves.toEqual({
ok: true,
number: 5,
url: 'https://github.com/fsdwen/orca/pull/5'
})
const [args] = ghExecFileAsyncMock.mock.calls[0]
expect(args[args.indexOf('--repo') + 1]).toBe('fsdwen/orca')
expect(args[args.indexOf('--head') + 1]).toBe('my-branch')
})
it('routes --repo to the Enterprise server via options.host for a GHES remote (#8312)', async () => {
// github.com-only slug parsing misses GHES, so creation comes from the
// enterprise resolver, which carries the host.
getOwnerRepoMock.mockResolvedValueOnce(null)
getEnterpriseGitHubRepoSlugMock.mockResolvedValueOnce({
owner: 'team',
repo: 'orca',
host: 'github.acme-corp.com'
})
// gh prints the PR URL (not JSON); the GHES host must still parse directly.
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: 'https://github.acme-corp.com/team/orca/pull/7\n'
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/create-pr',
title: 'GHES PR'
},
'local'
)
).resolves.toEqual({
ok: true,
number: 7,
url: 'https://github.acme-corp.com/team/orca/pull/7'
})
const [args, options] = ghExecFileAsyncMock.mock.calls[0]
// The runner host-qualifies argv at spawn time from options.host, so the
// mocked call sees a bare owner/repo plus the host in exec options.
expect(args[args.indexOf('--repo') + 1]).toBe('team/orca')
expect(options).toMatchObject({ host: 'github.acme-corp.com' })
})
it('routes the GHES existing-PR fallback lookup through options.host (#8312)', async () => {
getOwnerRepoMock.mockResolvedValue(null)
getEnterpriseGitHubRepoSlugMock.mockResolvedValue({
owner: 'team',
repo: 'orca',
host: 'github.acme-corp.com'
})
// Create reports "already exists", forcing the pr-list fallback.
ghExecFileAsyncMock
.mockRejectedValueOnce(
Object.assign(new Error('exists'), {
stderr: 'a pull request for branch "feature/create-pr" already exists',
stdout: ''
})
)
.mockResolvedValueOnce({
stdout: JSON.stringify([
{ number: 9, url: 'https://github.acme-corp.com/team/orca/pull/9' }
])
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/create-pr',
title: 'GHES PR'
},
'local'
)
).resolves.toMatchObject({
ok: false,
code: 'already_exists',
existingReview: { number: 9, url: 'https://github.acme-corp.com/team/orca/pull/9' }
})
const [listArgs, listOptions] = ghExecFileAsyncMock.mock.calls[1]
expect(listArgs).toEqual(expect.arrayContaining(['pr', 'list']))
expect(listArgs[listArgs.indexOf('--repo') + 1]).toBe('team/orca')
expect(listOptions).toMatchObject({ host: 'github.acme-corp.com' })
})
it('runs local WSL project pull request creation through the selected distro', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 43,
url: 'https://github.com/acme/widgets/pull/43'
})
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/wsl-create-pr',
title: 'WSL Create PR'
},
'local',
{ localGitExecOptions: { wslDistro: 'Ubuntu' } }
)
).resolves.toEqual({
ok: true,
number: 43,
url: 'https://github.com/acme/widgets/pull/43'
})
const [, options] = ghExecFileAsyncMock.mock.calls[0]
expect(options).toMatchObject({
cwd: '/repo-root',
wslDistro: 'Ubuntu',
timeout: 60_000,
idempotent: false
})
})
it('creates SSH-backed pull requests without using the remote path as a local cwd', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 45,
url: 'https://github.com/acme/widgets/pull/45'
})
})
await expect(
createGitHubPullRequest(
'/remote/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/ssh-create-pr',
title: 'SSH Create PR'
},
'ssh:ssh-1'
)
).resolves.toEqual({
ok: true,
number: 45,
url: 'https://github.com/acme/widgets/pull/45'
})
expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith(
'/remote/repo-root',
'origin',
'ssh-1',
{}
)
const [args, options] = ghExecFileAsyncMock.mock.calls[0]
expect(args).toEqual(
expect.arrayContaining([
'pr',
'create',
'--repo',
'acme/widgets',
'--base',
'main',
'--head',
'feature/ssh-create-pr'
])
)
expect(options).toMatchObject({
timeout: 60_000,
idempotent: false
})
expect(options).not.toHaveProperty('cwd')
})
it.each([
['.github/pull_request_template.md', ['.github/pull_request_template.md']],
[
'.github/PULL_REQUEST_TEMPLATE.md',
['.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md']
],
[
'docs/PULL_REQUEST_TEMPLATE.md',
[
'.github/pull_request_template.md',
'.github/PULL_REQUEST_TEMPLATE.md',
'pull_request_template.md',
'PULL_REQUEST_TEMPLATE.md',
'docs/pull_request_template.md',
'docs/PULL_REQUEST_TEMPLATE.md'
]
]
] as [string, string[]][])(
'reads PR templates from the SSH filesystem provider at %s',
async (relativeTemplatePath, expectedRelativeLookups) => {
const templateBody = `Remote template body from ${relativeTemplatePath}`
const readRemoteFile = vi.fn(async (path: string) => {
if (path === `/remote/repo-root/${relativeTemplatePath}`) {
return {
content: templateBody,
isBinary: false
}
}
throw new Error('missing template')
})
getSshFilesystemProviderMock.mockReturnValue({ readFile: readRemoteFile })
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
const writtenBodies: string[] = []
ghExecFileAsyncMock.mockImplementationOnce(async (args: string[]) => {
const bodyPath = args[args.indexOf('--body-file') + 1]
writtenBodies.push(await readFile(bodyPath, 'utf8'))
return {
stdout: JSON.stringify({
number: 46,
url: 'https://github.com/acme/widgets/pull/46'
})
}
})
await expect(
createGitHubPullRequest(
'/remote/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/ssh-template',
title: 'SSH Template PR',
body: '',
useTemplate: true
},
'ssh:ssh-1'
)
).resolves.toEqual({
ok: true,
number: 46,
url: 'https://github.com/acme/widgets/pull/46'
})
expect(readRemoteFile.mock.calls.map(([path]) => path)).toEqual(
expectedRelativeLookups.map((relativeLookup) => `/remote/repo-root/${relativeLookup}`)
)
expect(writtenBodies).toEqual([templateBody])
}
)
it('falls back to parsing the PR URL for older gh output', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: 'https://github.com/acme/widgets/pull/43\n'
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'feature/url-output',
title: 'URL output'
},
'local'
)
).resolves.toEqual({
ok: true,
number: 43,
url: 'https://github.com/acme/widgets/pull/43'
})
})
it('returns the existing PR when gh reports an already-open pull request', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock
.mockRejectedValueOnce({ stderr: 'a pull request already exists for feature/existing' })
.mockResolvedValueOnce({
stdout: JSON.stringify([
{
number: 44,
url: 'https://github.com/acme/widgets/pull/44'
}
])
})
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'main',
head: 'refs/remotes/origin/feature/existing',
title: 'Existing'
},
'local'
)
).resolves.toEqual({
ok: false,
code: 'already_exists',
error: 'A pull request already exists for this branch.',
existingReview: {
number: 44,
url: 'https://github.com/acme/widgets/pull/44'
}
})
expect(ghExecFileAsyncMock.mock.calls[1]).toEqual([
[
'pr',
'list',
'--repo',
'acme/widgets',
'--head',
'feature/existing',
'--base',
'main',
'--state',
'open',
'--limit',
'2',
'--json',
'number,url'
],
// Why: dotcom slugs resolve with host:'github.com' so creation stays
// pinned against a process-level GH_HOST.
{ cwd: '/repo-root', host: 'github.com' }
])
})
it('validates base, head, and title before invoking gh', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
await expect(
createGitHubPullRequest(
'/repo-root',
{
provider: 'github',
base: 'refs/heads/feature',
head: 'feature',
title: 'Feature'
},
'local'
)
).resolves.toEqual({
ok: false,
code: 'validation',
error: 'Create PR failed: choose a different base branch before creating a pull request.'
})
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(acquireMock).not.toHaveBeenCalled()
})
})