Files
orca/src/main/github/stacked-pr-creation.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

298 lines
8.9 KiB
TypeScript

import { beforeEach, describe, expect, it, vi } from 'vitest'
const { ghExecFileAsyncMock, repositoryMock } = vi.hoisted(() => ({
ghExecFileAsyncMock: vi.fn(),
repositoryMock: vi.fn()
}))
vi.mock('./gh-utils', () => ({
acquire: vi.fn(),
release: vi.fn(),
ghExecFileAsync: ghExecFileAsyncMock,
ghRepoExecOptions: (context: { repoPath: string; connectionId?: string | null }) =>
context.connectionId ? {} : { cwd: context.repoPath },
githubRepoContext: (
repoPath: string,
connectionId?: string | null,
localGitOptions?: Record<string, unknown>
) => ({ repoPath, connectionId, localGitOptions })
}))
vi.mock('./github-api-repository', () => ({
getOriginGitHubApiRepository: repositoryMock,
githubHostExecOptions: (repository: { host?: string }) => ({ host: repository.host })
}))
import {
prepareGitHubStackedPullRequest,
registerGitHubStackedPullRequest
} from './stacked-pr-creation'
const repository = { owner: 'acme', repo: 'orca', host: 'github.com' }
const parentReview = { number: 41, url: 'https://github.com/acme/orca/pull/41' }
const currentReview = { number: 42, url: 'https://github.com/acme/orca/pull/42' }
function pullRequest(number: number, head: string, base: string) {
return {
number,
html_url: `https://github.com/acme/orca/pull/${number}`,
head: { ref: head },
base: { ref: base }
}
}
function stack(number: number, pullRequests: number[]) {
return {
number,
open: true,
pull_requests: pullRequests.map((pullRequestNumber) => ({ number: pullRequestNumber }))
}
}
beforeEach(() => {
ghExecFileAsyncMock.mockReset()
repositoryMock.mockReset()
repositoryMock.mockResolvedValue(repository)
})
describe('prepareGitHubStackedPullRequest', () => {
it('resolves an open parent PR and an existing current PR', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) })
.mockResolvedValueOnce({
stdout: JSON.stringify([pullRequest(42, 'stack/child', 'stack/parent')])
})
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [40, 41])]) })
.mockResolvedValueOnce({ stdout: '[]' })
const result = await prepareGitHubStackedPullRequest(
'/repo',
{
provider: 'github',
base: 'origin/stack/parent',
head: 'refs/heads/stack/child',
title: 'Child'
},
'local'
)
expect(result).toMatchObject({
ok: true,
parentReview: { number: 41 },
currentReview: { number: 42 }
})
expect(ghExecFileAsyncMock.mock.calls[0][0]).toEqual([
'api',
'repos/acme/orca/pulls?head=acme%3Astack%2Fparent&state=open&per_page=2'
])
expect(ghExecFileAsyncMock.mock.calls[1][0]).toEqual([
'api',
'repos/acme/orca/pulls?head=acme%3Astack%2Fchild&base=stack%2Fparent&state=open&per_page=2'
])
})
it('allows an idempotent retry after the child was already registered', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) })
.mockResolvedValueOnce({
stdout: JSON.stringify([pullRequest(42, 'stack/child', 'stack/parent')])
})
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) })
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) })
const result = await prepareGitHubStackedPullRequest(
'/repo',
{
provider: 'github',
base: 'stack/parent',
head: 'stack/child',
title: 'Child'
},
'local'
)
expect(result).toMatchObject({ ok: true, currentReview: { number: 42 } })
})
it('requires an open PR for the selected parent branch', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: '[]' })
const result = await prepareGitHubStackedPullRequest(
'/repo',
{
provider: 'github',
base: 'feature/parent',
head: 'feature/child',
title: 'Child'
},
'local'
)
expect(result).toMatchObject({ ok: false, code: 'validation' })
if (!result.ok) {
expect(result.error).toContain('does not have an open pull request')
}
})
it('rejects a parent that is not the top of its stack', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) })
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 45])]) })
const result = await prepareGitHubStackedPullRequest(
'/repo',
{
provider: 'github',
base: 'stack/parent',
head: 'stack/child',
title: 'Child'
},
'local'
)
expect(result).toMatchObject({ ok: false, code: 'validation' })
if (!result.ok) {
expect(result.error).toContain('top pull request')
}
})
it('does not offer stacks on GitHub Enterprise Server', async () => {
repositoryMock.mockResolvedValue({
owner: 'acme',
repo: 'orca',
host: 'github.acme.test'
})
const result = await prepareGitHubStackedPullRequest(
'/repo',
{
provider: 'github',
base: 'stack/parent',
head: 'stack/child',
title: 'Child'
},
'local'
)
expect(result).toMatchObject({ ok: false, code: 'validation' })
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
})
})
describe('registerGitHubStackedPullRequest', () => {
it('creates a new stack with the parent and current PR', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: JSON.stringify({ number: 50 }) })
const result = await registerGitHubStackedPullRequest({
executionHostId: 'local',
repoPath: '/repo',
repository,
parentReview,
currentReview
})
expect(result).toMatchObject({ ok: true, number: 42, stackNumber: 50 })
expect(ghExecFileAsyncMock.mock.calls[2][0]).toEqual([
'api',
'-X',
'POST',
'repos/acme/orca/stacks',
'-F',
'pull_requests[]=41',
'-F',
'pull_requests[]=42'
])
})
it('appends the current PR when the parent is the existing top', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [40, 41])]) })
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: JSON.stringify({ number: 50 }) })
const result = await registerGitHubStackedPullRequest({
repoPath: '/repo',
repository,
parentReview,
currentReview,
executionHostId: 'ssh:ssh-1'
})
expect(result).toMatchObject({ ok: true, stackNumber: 50 })
expect(ghExecFileAsyncMock.mock.calls[2][0]).toEqual([
'api',
'-X',
'POST',
'repos/acme/orca/stacks/50/add',
'-F',
'pull_requests[]=42'
])
expect(ghExecFileAsyncMock.mock.calls[2][1]).not.toHaveProperty('cwd')
})
it('treats an already registered parent-child pair as success', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) })
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) })
const result = await registerGitHubStackedPullRequest({
executionHostId: 'local',
repoPath: '/repo',
repository,
parentReview,
currentReview
})
expect(result).toMatchObject({ ok: true, stackNumber: 50 })
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2)
})
it('does not claim registration when the stack no longer holds the parent', async () => {
// A concurrent stack edit can drop the parent while the child sits at index 0.
// Reading index 0 off a findIndex miss would report that pair as registered.
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [42])]) })
.mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [42])]) })
const result = await registerGitHubStackedPullRequest({
executionHostId: 'local',
repoPath: '/repo',
repository,
parentReview,
currentReview
})
expect(result).toMatchObject({
ok: false,
error: 'The pull request already belongs to a different GitHub stack.',
createdReview: currentReview
})
})
it('preserves the created PR when registration fails', async () => {
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: '[]' })
.mockResolvedValueOnce({ stdout: '[]' })
.mockRejectedValueOnce(new Error('HTTP 422'))
const result = await registerGitHubStackedPullRequest({
executionHostId: 'local',
repoPath: '/repo',
repository,
parentReview,
currentReview
})
expect(result).toMatchObject({
ok: false,
createdReview: currentReview
})
})
})