Create pr not working for stacked worktree (#8651)

* Fix stacked-worktree PR creation targeting a local-only parent branch

- Resolve the eligibility default base to a remote-tracking ref instead
  of blindly trusting the submitted parent branch, since a stacked
  worktree's base is often a local-only branch the remote can't resolve
- Add a create-time hard block (base_not_on_remote) so a stale or
  unpushed submitted base fails with actionable copy instead of the
  provider's opaque error
- Update the dialog's default-base resolution and blocked-action/
  dropdown copy to match the new remote-validated default

* Split hosted-review-creation.test.ts to fix max-lines lint error

Moved getHostedReviewCreationEligibility tests to a separate file (hosted-review-creation-eligibility.test.ts) to reduce the original file size from 880 to 579 lines, satisfying the max-lines lint constraint.

Co-authored-by: Orca <help@stably.ai>

* Fix Create PR intent flow to use remote-validated eligibility default fo

Prefer eligibilityDefaultBaseRef over the raw compare base when resolving
the review base for the one-click Create PR intent flow, since eligibility
is recomputed from the same compare base right before creation and already
corrects a local-only stacked parent to the repo default. Falls back to
the compare base only when eligibility supplies no default.

* Simplify base-ref remote existence check into a single for-each-ref call

Combine the wildcard and exact-tracking-ref lookups into one for-each-ref
invocation with multiple patterns instead of two sequential git calls,
removing the redundant rev-parse fallback path.

---------

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinjing
2026-07-13 20:10:50 -07:00
committed by GitHub
co-authored by Orca
parent 8662e5a7ab
commit 1ef0551bc1
15 changed files with 767 additions and 280 deletions
+3 -1
View File
@@ -64,7 +64,9 @@ describe('orca claude-teams CLI handler', () => {
spawnMock.mockImplementation(() => mockClaudeChild())
callMock.mockReset()
callMock.mockResolvedValue({
result: { launch: { env: { CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1', PATH: '/shim:/usr/bin' } } }
result: {
launch: { env: { CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1', PATH: '/shim:/usr/bin' } }
}
})
previousRunAsNode = process.env.ELECTRON_RUN_AS_NODE
previousPaneKey = process.env.ORCA_PANE_KEY
@@ -126,16 +126,18 @@ describe('Windows firewall remote-address scope', () => {
// A /32 subnet is just the desktop itself, so an explicit range cannot prove
// the phone (a different host) is allowed — only the keywords can.
expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], '100.64.1.20', 32)).toBe(true)
expect(hasSufficientWindowsFirewallRemoteScope([rule(['LocalSubnet'])], '100.64.1.20', 32)).toBe(
true
)
expect(
hasSufficientWindowsFirewallRemoteScope([rule(['LocalSubnet'])], '100.64.1.20', 32)
).toBe(true)
expect(
hasSufficientWindowsFirewallRemoteScope([rule(['100.64.0.0/10'])], '100.64.1.20', 32)
).toBe(false)
})
it('fails address-family keywords closed when the interface family is unknown', () => {
expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], undefined, undefined)).toBe(true)
expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], undefined, undefined)).toBe(
true
)
expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any4'])], undefined, undefined)).toBe(
false
)
@@ -0,0 +1,529 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
const {
createGitHubPullRequestMock,
createGitLabMergeRequestMock,
createAzureDevOpsPullRequestMock,
createGiteaPullRequestMock,
isAzureDevOpsReviewCreationAuthenticatedMock,
isGiteaReviewCreationAuthenticatedMock,
getRepoSlugMock,
getProjectSlugMock,
getBitbucketRepoSlugMock,
getAzureDevOpsRepoSlugMock,
getGiteaRepoSlugMock,
getHostedReviewForBranchMock,
ghExecFileAsyncMock,
glabExecFileAsyncMock,
gitExecFileAsyncMock,
getUpstreamStatusMock,
getSshGitProviderMock,
getEnterpriseGitHubRepoSlugMock
} = vi.hoisted(() => ({
createGitHubPullRequestMock: vi.fn(),
createGitLabMergeRequestMock: vi.fn(),
createAzureDevOpsPullRequestMock: vi.fn(),
createGiteaPullRequestMock: vi.fn(),
isAzureDevOpsReviewCreationAuthenticatedMock: vi.fn(),
isGiteaReviewCreationAuthenticatedMock: vi.fn(),
getRepoSlugMock: vi.fn(),
getProjectSlugMock: vi.fn(),
getBitbucketRepoSlugMock: vi.fn(),
getAzureDevOpsRepoSlugMock: vi.fn(),
getGiteaRepoSlugMock: vi.fn(),
getHostedReviewForBranchMock: vi.fn(),
ghExecFileAsyncMock: vi.fn(),
glabExecFileAsyncMock: vi.fn(),
gitExecFileAsyncMock: vi.fn(),
getUpstreamStatusMock: vi.fn(),
getSshGitProviderMock: vi.fn(),
getEnterpriseGitHubRepoSlugMock: vi.fn()
}))
vi.mock('../github/client', () => ({
createGitHubPullRequest: createGitHubPullRequestMock,
getRepoSlug: getRepoSlugMock,
getPRForBranch: vi.fn()
}))
vi.mock('../github/github-enterprise-repository', () => ({
getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock
}))
vi.mock('../gitlab/client', () => ({
getProjectSlug: getProjectSlugMock,
getMergeRequestForBranch: vi.fn(),
getMergeRequest: vi.fn()
}))
vi.mock('../gitlab/merge-request-creation', () => ({
createGitLabMergeRequest: createGitLabMergeRequestMock
}))
vi.mock('../bitbucket/client', () => ({
getBitbucketRepoSlug: getBitbucketRepoSlugMock,
getBitbucketPullRequestForBranch: vi.fn(),
getBitbucketPullRequest: vi.fn()
}))
vi.mock('../azure-devops/client', () => ({
getAzureDevOpsRepoSlug: getAzureDevOpsRepoSlugMock,
getAzureDevOpsPullRequestForBranch: vi.fn(),
getAzureDevOpsPullRequest: vi.fn()
}))
vi.mock('../azure-devops/pull-request-creation', () => ({
createAzureDevOpsPullRequest: createAzureDevOpsPullRequestMock,
isAzureDevOpsReviewCreationAuthenticated: isAzureDevOpsReviewCreationAuthenticatedMock
}))
vi.mock('../gitea/client', () => ({
getGiteaRepoSlug: getGiteaRepoSlugMock,
getGiteaPullRequestForBranch: vi.fn(),
getGiteaPullRequest: vi.fn()
}))
vi.mock('../gitea/pull-request-creation', () => ({
createGiteaPullRequest: createGiteaPullRequestMock,
isGiteaReviewCreationAuthenticated: isGiteaReviewCreationAuthenticatedMock
}))
vi.mock('../github/gh-utils', () => ({
acquire: vi.fn(),
release: vi.fn(),
ghExecFileAsync: ghExecFileAsyncMock,
gitExecFileAsync: gitExecFileAsyncMock
}))
vi.mock('../gitlab/gl-utils', () => ({
acquire: vi.fn(),
release: vi.fn(),
glabExecFileAsync: glabExecFileAsyncMock,
glabRepoExecOptions: (repoPath: string, connectionId?: string | null) =>
connectionId ? {} : { cwd: repoPath }
}))
vi.mock('../git/upstream', () => ({
getUpstreamStatus: getUpstreamStatusMock
}))
vi.mock('../providers/ssh-git-dispatch', () => ({
getSshGitProvider: getSshGitProviderMock
}))
vi.mock('./hosted-review', () => ({
getHostedReviewForBranch: getHostedReviewForBranchMock
}))
import { getHostedReviewCreationEligibility } from './hosted-review-creation'
function resetMocks(): void {
for (const mock of [
createGitHubPullRequestMock,
createGitLabMergeRequestMock,
createAzureDevOpsPullRequestMock,
createGiteaPullRequestMock,
isAzureDevOpsReviewCreationAuthenticatedMock,
isGiteaReviewCreationAuthenticatedMock,
getRepoSlugMock,
getProjectSlugMock,
getBitbucketRepoSlugMock,
getAzureDevOpsRepoSlugMock,
getGiteaRepoSlugMock,
getHostedReviewForBranchMock,
ghExecFileAsyncMock,
glabExecFileAsyncMock,
gitExecFileAsyncMock,
getUpstreamStatusMock,
getSshGitProviderMock,
getEnterpriseGitHubRepoSlugMock
]) {
mock.mockReset()
}
}
function mockGitHubProvider(): void {
getProjectSlugMock.mockResolvedValue(null)
getRepoSlugMock.mockResolvedValue({ owner: 'acme', repo: 'orca' })
getBitbucketRepoSlugMock.mockResolvedValue(null)
getAzureDevOpsRepoSlugMock.mockResolvedValue(null)
getGiteaRepoSlugMock.mockResolvedValue(null)
getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null)
}
// GHES: github.com-only slug parsing misses the custom host, so the enterprise
// resolver claims the repo and reports the host for the gh auth probe (#8312).
function mockGitHubEnterpriseProvider(): void {
getProjectSlugMock.mockResolvedValue(null)
getRepoSlugMock.mockResolvedValue(null)
getBitbucketRepoSlugMock.mockResolvedValue(null)
getAzureDevOpsRepoSlugMock.mockResolvedValue(null)
getGiteaRepoSlugMock.mockResolvedValue(null)
getEnterpriseGitHubRepoSlugMock.mockResolvedValue({
owner: 'acme',
repo: 'orca',
host: 'github.acme-corp.com'
})
}
function mockGitLabProvider(): void {
getProjectSlugMock.mockResolvedValue({ host: 'gitlab.com', path: 'acme/orca' })
getRepoSlugMock.mockResolvedValue(null)
getBitbucketRepoSlugMock.mockResolvedValue(null)
getAzureDevOpsRepoSlugMock.mockResolvedValue(null)
getGiteaRepoSlugMock.mockResolvedValue(null)
}
function mockAzureDevOpsProvider(): void {
getProjectSlugMock.mockResolvedValue(null)
getRepoSlugMock.mockResolvedValue(null)
getBitbucketRepoSlugMock.mockResolvedValue(null)
getAzureDevOpsRepoSlugMock.mockResolvedValue({
host: 'dev.azure.com',
project: 'Project',
repository: 'orca',
apiBaseUrl: 'https://dev.azure.com/acme/Project',
webBaseUrl: 'https://dev.azure.com/acme/Project/_git/orca'
})
getGiteaRepoSlugMock.mockResolvedValue(null)
}
function mockGiteaProvider(): void {
getProjectSlugMock.mockResolvedValue(null)
getRepoSlugMock.mockResolvedValue(null)
getBitbucketRepoSlugMock.mockResolvedValue(null)
getAzureDevOpsRepoSlugMock.mockResolvedValue(null)
getGiteaRepoSlugMock.mockResolvedValue({
host: 'git.example.com',
owner: 'acme',
repo: 'orca',
apiBaseUrl: 'https://git.example.com/api/v1',
webBaseUrl: 'https://git.example.com'
})
}
describe('getHostedReviewCreationEligibility', () => {
beforeEach(() => {
resetMocks()
mockGitHubProvider()
getHostedReviewForBranchMock.mockResolvedValue(null)
ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' })
gitExecFileAsyncMock.mockResolvedValue({ stdout: 'Feature title\n', stderr: '' })
isAzureDevOpsReviewCreationAuthenticatedMock.mockReturnValue(true)
isGiteaReviewCreationAuthenticatedMock.mockReturnValue(true)
})
it('treats short remote base refs as the default branch name', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'main',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
canCreate: false,
blockedReason: 'default_branch',
defaultBaseRef: 'origin/main'
})
})
// Stacked-worktree base resolution (Change 1/2). `stackedArgs` defaults to a
// bare local-only parent; `mockRefs` controls the remote-tracking snapshot.
const stackedArgs = (
overrides: Partial<Parameters<typeof getHostedReviewCreationEligibility>[0]> = {}
): Parameters<typeof getHostedReviewCreationEligibility>[0] => ({
repoPath: '/repo',
branch: 'feature/stacked',
base: 'stacked-parent',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0,
...overrides
})
const mockRefs = (opts: {
symbolicRef?: string
forEachRef?: string
forEachThrows?: boolean
revParseThrows?: boolean
}): void => {
gitExecFileAsyncMock.mockImplementation(async (args: string[]) => {
if (args[0] === 'symbolic-ref') {
return { stdout: opts.symbolicRef ?? '', stderr: '' }
}
if (args[0] === 'for-each-ref') {
if (opts.forEachThrows) {
throw new Error('ssh: connect: connection refused')
}
return { stdout: opts.forEachRef ?? '', stderr: '' }
}
if (args[0] === 'rev-parse' && opts.revParseThrows) {
throw new Error('unknown revision')
}
return { stdout: 'refs/remotes/origin/main\n', stderr: '' }
})
}
it('falls back to the repo default when a stacked parent base is local-only', async () => {
mockRefs({ symbolicRef: 'refs/remotes/origin/main\n' })
await expect(getHostedReviewCreationEligibility(stackedArgs())).resolves.toMatchObject({
canCreate: true,
blockedReason: null,
defaultBaseRef: 'origin/main'
})
})
it('preserves a stacked parent base that exists on the remote', async () => {
mockRefs({ forEachRef: 'refs/remotes/origin/parent-pushed\n' })
await expect(
getHostedReviewCreationEligibility(stackedArgs({ base: 'parent-pushed' }))
).resolves.toMatchObject({
canCreate: true,
blockedReason: null,
defaultBaseRef: 'parent-pushed'
})
})
it('keeps the candidate base when no repo default can be resolved', async () => {
mockRefs({ revParseThrows: true })
await expect(getHostedReviewCreationEligibility(stackedArgs())).resolves.toMatchObject({
canCreate: true,
blockedReason: null,
defaultBaseRef: 'stacked-parent'
})
})
it('preserves the candidate base when the remote probe cannot reach the host', async () => {
// Transport failure must not be read as "absent" — that would demote a
// legitimately-pushed parent to the repo default on a transient SSH blip.
mockRefs({ forEachThrows: true })
await expect(
getHostedReviewCreationEligibility(stackedArgs({ base: 'parent-pushed' }))
).resolves.toMatchObject({ canCreate: true, defaultBaseRef: 'parent-pushed' })
})
it('blocks dirty tracked GitHub branches before PR creation', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: true,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: false,
blockedReason: 'dirty',
nextAction: 'commit',
head: 'feature/create-pr'
})
})
it('keeps dirty feature branches eligible for PR preparation when review lookup fails', async () => {
getHostedReviewForBranchMock.mockRejectedValueOnce(new Error('gh lookup failed'))
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: true,
hasUpstream: false,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: false,
blockedReason: 'dirty',
nextAction: 'commit',
head: 'feature/create-pr'
})
})
it('enables creation for clean, in-sync, authenticated GitHub feature branches', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'refs/heads/feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
blockedReason: null,
nextAction: null,
defaultBaseRef: 'origin/main',
head: 'feature/create-pr'
})
})
it('detects a GitHub Enterprise Server branch as the GitHub provider (#8312)', async () => {
mockGitHubEnterpriseProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
blockedReason: null,
nextAction: null
})
// Enterprise auth was already confirmed during detection; the gate must not
// fire a redundant gh probe.
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
})
it('resolves remote eligibility through SSH repo metadata without generating PR copy', async () => {
const remoteGit = {
exec: vi.fn(async () => ({ stdout: '', stderr: '' }))
}
getSshGitProviderMock.mockReturnValue(remoteGit)
await expect(
getHostedReviewCreationEligibility({
repoPath: '/remote/repo',
connectionId: 'ssh-1',
branch: 'feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
head: 'feature/create-pr'
})
expect(getProjectSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1')
expect(getRepoSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1')
expect(getHostedReviewForBranchMock).toHaveBeenCalledWith(
expect.objectContaining({ repoPath: '/remote/repo', connectionId: 'ssh-1' })
)
// Why: the base-on-remote probe must run on the SSH host that will execute
// the provider create, so it flows through the relay exec, not local git.
expect(remoteGit.exec).toHaveBeenCalledWith(
['for-each-ref', '--count=1', '--format=%(refname)', 'refs/remotes/*/main'],
'/remote/repo'
)
})
it('offers push as the next action for authenticated branches with local-only commits', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 2,
behind: 0
})
).resolves.toMatchObject({
canCreate: false,
blockedReason: 'needs_push',
nextAction: 'push'
})
})
it('enables creation for clean, in-sync, authenticated GitLab feature branches', async () => {
mockGitLabProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/gitlab',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'gitlab',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/gitlab'
})
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).toHaveBeenCalledWith(
['auth', 'status', '--hostname', 'gitlab.com'],
{ cwd: '/repo' }
)
})
it('enables creation for clean, in-sync, token-configured Azure DevOps feature branches', async () => {
mockAzureDevOpsProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/azure',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'azure-devops',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/azure'
})
expect(isAzureDevOpsReviewCreationAuthenticatedMock).toHaveBeenCalledOnce()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).not.toHaveBeenCalled()
})
it('enables creation for clean, in-sync, token-configured Gitea feature branches', async () => {
mockGiteaProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/gitea',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'gitea',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/gitea'
})
expect(isGiteaReviewCreationAuthenticatedMock).toHaveBeenCalledOnce()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).not.toHaveBeenCalled()
})
})
@@ -115,7 +115,7 @@ vi.mock('./hosted-review', () => ({
getHostedReviewForBranch: getHostedReviewForBranchMock
}))
import { createHostedReview, getHostedReviewCreationEligibility } from './hosted-review-creation'
import { createHostedReview } from './hosted-review-creation'
function resetMocks(): void {
for (const mock of [
@@ -223,6 +223,11 @@ describe('createHostedReview', () => {
if (args[0] === 'status') {
return { stdout: '', stderr: '' }
}
// Why: base-on-remote probe (Change 2 enforcement) — the default base
// resolves to a remote-tracking branch so create-time validation passes.
if (args[0] === 'for-each-ref') {
return { stdout: 'refs/remotes/origin/main\n', stderr: '' }
}
if (args[0] === 'log' && args.includes('--pretty=%s')) {
return { stdout: 'Feature title\n', stderr: '' }
}
@@ -301,6 +306,32 @@ describe('createHostedReview', () => {
expect(createGitHubPullRequestMock).not.toHaveBeenCalled()
})
it('blocks creation with actionable copy when the submitted base is local-only', async () => {
// for-each-ref falls through to '' → the submitted stacked parent is not on
// the remote, so create-time enforcement blocks with actionable copy.
gitExecFileAsyncMock.mockImplementation(async (args: string[]) => {
if (args[0] === 'rev-parse') {
return { stdout: 'feature\n', stderr: '' }
}
return { stdout: '', stderr: '' }
})
await expect(
createHostedReview('/repo', {
provider: 'github',
base: 'stacked-parent',
head: 'feature',
title: 'Feature'
})
).resolves.toEqual({
ok: false,
code: 'validation',
error:
'Create PR failed: the base branch "stacked-parent" hasn\'t been pushed to the remote. Choose a pushed base or push it first.'
})
expect(createGitHubPullRequestMock).not.toHaveBeenCalled()
})
it('creates the pull request after fresh main-process validation passes', async () => {
await expect(
createHostedReview('/repo', {
@@ -501,6 +532,10 @@ describe('createHostedReview', () => {
if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref' && args[2] === 'HEAD') {
return { stdout: 'feature\n', stderr: '' }
}
if (args[0] === 'for-each-ref') {
// Base-on-remote probe (Change 2) runs on the SSH host; base is pushed.
return { stdout: 'refs/remotes/origin/main\n', stderr: '' }
}
if (args[0] === 'log' && args.includes('--pretty=%s')) {
return { stdout: 'Feature title\n', stderr: '' }
}
@@ -588,248 +623,3 @@ describe('createHostedReview', () => {
expect(createGitHubPullRequestMock).not.toHaveBeenCalled()
})
})
describe('getHostedReviewCreationEligibility', () => {
beforeEach(() => {
resetMocks()
mockGitHubProvider()
getHostedReviewForBranchMock.mockResolvedValue(null)
ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' })
gitExecFileAsyncMock.mockResolvedValue({ stdout: 'Feature title\n', stderr: '' })
isAzureDevOpsReviewCreationAuthenticatedMock.mockReturnValue(true)
isGiteaReviewCreationAuthenticatedMock.mockReturnValue(true)
})
it('treats short remote base refs as the default branch name', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'main',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
canCreate: false,
blockedReason: 'default_branch',
defaultBaseRef: 'origin/main'
})
})
it('blocks dirty tracked GitHub branches before PR creation', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: true,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: false,
blockedReason: 'dirty',
nextAction: 'commit',
head: 'feature/create-pr'
})
})
it('keeps dirty feature branches eligible for PR preparation when review lookup fails', async () => {
getHostedReviewForBranchMock.mockRejectedValueOnce(new Error('gh lookup failed'))
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: true,
hasUpstream: false,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: false,
blockedReason: 'dirty',
nextAction: 'commit',
head: 'feature/create-pr'
})
})
it('enables creation for clean, in-sync, authenticated GitHub feature branches', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'refs/heads/feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
blockedReason: null,
nextAction: null,
defaultBaseRef: 'origin/main',
head: 'feature/create-pr'
})
})
it('detects a GitHub Enterprise Server branch as the GitHub provider (#8312)', async () => {
mockGitHubEnterpriseProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
blockedReason: null,
nextAction: null
})
// Enterprise auth was already confirmed during detection; the gate must not
// fire a redundant gh probe.
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
})
it('resolves remote eligibility through SSH repo metadata without generating PR copy', async () => {
const remoteGit = {
exec: vi.fn(async () => ({ stdout: '', stderr: '' }))
}
getSshGitProviderMock.mockReturnValue(remoteGit)
await expect(
getHostedReviewCreationEligibility({
repoPath: '/remote/repo',
connectionId: 'ssh-1',
branch: 'feature/create-pr',
base: 'origin/main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'github',
canCreate: true,
head: 'feature/create-pr'
})
expect(getProjectSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1')
expect(getRepoSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1')
expect(getHostedReviewForBranchMock).toHaveBeenCalledWith(
expect.objectContaining({ repoPath: '/remote/repo', connectionId: 'ssh-1' })
)
expect(remoteGit.exec).not.toHaveBeenCalled()
})
it('offers push as the next action for authenticated branches with local-only commits', async () => {
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/create-pr',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 2,
behind: 0
})
).resolves.toMatchObject({
canCreate: false,
blockedReason: 'needs_push',
nextAction: 'push'
})
})
it('enables creation for clean, in-sync, authenticated GitLab feature branches', async () => {
mockGitLabProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/gitlab',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'gitlab',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/gitlab'
})
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).toHaveBeenCalledWith(
['auth', 'status', '--hostname', 'gitlab.com'],
{ cwd: '/repo' }
)
})
it('enables creation for clean, in-sync, token-configured Azure DevOps feature branches', async () => {
mockAzureDevOpsProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/azure',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'azure-devops',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/azure'
})
expect(isAzureDevOpsReviewCreationAuthenticatedMock).toHaveBeenCalledOnce()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).not.toHaveBeenCalled()
})
it('enables creation for clean, in-sync, token-configured Gitea feature branches', async () => {
mockGiteaProvider()
await expect(
getHostedReviewCreationEligibility({
repoPath: '/repo',
branch: 'feature/gitea',
base: 'main',
hasUncommittedChanges: false,
hasUpstream: true,
ahead: 0,
behind: 0
})
).resolves.toMatchObject({
provider: 'gitea',
canCreate: true,
blockedReason: null,
nextAction: null,
head: 'feature/gitea'
})
expect(isGiteaReviewCreationAuthenticatedMock).toHaveBeenCalledOnce()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(glabExecFileAsyncMock).not.toHaveBeenCalled()
})
})
@@ -42,6 +42,10 @@ import {
type HostedReviewCreationEligibilityInput = HostedReviewCreationEligibilityArgs & {
connectionId?: string | null
// Why: only the create-time preflight enforces base-on-remote as a hard block;
// the renderer's eligibility probe passes the base as a candidate and relies on
// Change 1 to correct a local-only parent, so it must never set this.
enforceBaseOnRemote?: boolean
} & HostedReviewExecutionOptions
function stripRefPrefix(ref: string): string {
@@ -133,6 +137,52 @@ async function getDefaultBaseRef(
)
}
/**
* Whether the candidate base resolves to a remote-tracking branch on the
* executing host.
*
* Why: a stacked worktree's `worktree.baseRef` is typically a bare parent
* branch name with no remote qualifier, so the probe must match the branch
* under *any* configured remote rather than assume `origin` — otherwise fork
* workflows (`upstream/main`) would be missed. Runs through
* `runGitForHostedReview` so it evaluates on the same host that will run the
* provider create (native/WSL/SSH/relay). This reads the local remote-tracking
* snapshot, not the live remote; see the design doc's Open Questions for the
* ls-remote/staleness tradeoff left as a follow-up.
*/
async function baseRefExistsOnRemote(
candidate: string,
repoPath: string,
connectionId?: string | null,
options: HostedReviewExecutionOptions = {}
): Promise<boolean> {
const base = normalizeHostedReviewBaseRef(candidate).trim()
if (!base) {
return false
}
const run = (argv: string[]): Promise<{ stdout: string }> =>
runGitForHostedReview(repoPath, argv, connectionId, options)
const patterns = [`refs/remotes/*/${base}`]
// Non-origin remote-qualified candidate (e.g. `fork/main`): the wildcard glob
// above only matches a branch literally named that because `*` does not cross `/`.
// Include the exact tracking ref directly.
if (base.includes('/')) {
patterns.push(`refs/remotes/${base}`)
}
try {
// for-each-ref exits 0 whether or not the pattern matches, so a clean empty
// result is an authoritative "absent" while a thrown error is a transport
// failure. Never conflate the two: on an unreachable host, preserve the
// candidate rather than silently demoting a legitimately-pushed parent base.
const { stdout } = await run(['for-each-ref', '--count=1', '--format=%(refname)', ...patterns])
return stdout.trim().length > 0
} catch {
return true
}
}
async function getCurrentBranch(
repoPath: string,
connectionId?: string | null,
@@ -255,9 +305,11 @@ async function isProviderAuthenticated(
function blockedCreateResultForReason(
reason: NonNullable<HostedReviewCreationBlockedReason>,
provider: HostedReviewProvider
provider: HostedReviewProvider,
submittedBase?: string | null
): CreateHostedReviewResult | null {
const copy = reviewCopy(provider)
const baseLabel = submittedBase?.trim() ? `"${submittedBase.trim()}" ` : ''
const blockedCreateResultByReason = {
auth_required: {
ok: false,
@@ -303,6 +355,11 @@ function blockedCreateResultForReason(
ok: false,
code: 'validation',
error: `Create ${copy.shortLabel} failed: refresh source control status and try again.`
},
base_not_on_remote: {
ok: false,
code: 'validation',
error: `Create ${copy.shortLabel} failed: the base branch ${baseLabel}hasn't been pushed to the remote. Choose a pushed base or push it first.`
}
} satisfies Partial<
Record<NonNullable<HostedReviewCreationBlockedReason>, CreateHostedReviewResult>
@@ -311,7 +368,8 @@ function blockedCreateResultForReason(
}
function blockedEligibilityToCreateResult(
eligibility: HostedReviewCreationEligibility
eligibility: HostedReviewCreationEligibility,
submittedBase?: string | null
): CreateHostedReviewResult | null {
if (eligibility.canCreate) {
return null
@@ -326,7 +384,11 @@ function blockedEligibilityToCreateResult(
}
}
if (eligibility.blockedReason) {
return blockedCreateResultForReason(eligibility.blockedReason, eligibility.provider)
return blockedCreateResultForReason(
eligibility.blockedReason,
eligibility.provider,
submittedBase
)
}
const copy = reviewCopy(eligibility.provider)
return {
@@ -358,20 +420,24 @@ async function validateCurrentBranchCanCreateReview(
hasUncommittedChanges(repoPath, connectionId, options),
getHostedReviewUpstreamStatus(repoPath, connectionId, options)
])
const submittedBase = normalizeHostedReviewBaseRef(input.base)
const eligibility = await getHostedReviewCreationEligibility({
repoPath,
branch: requestedHead || currentBranch,
base: normalizeHostedReviewBaseRef(input.base),
base: submittedBase,
hasUncommittedChanges: dirty,
hasUpstream: upstreamStatus.hasUpstream,
ahead: upstreamStatus.ahead,
behind: upstreamStatus.behind,
connectionId,
// Why: this is the last gate before the provider create, which targets the
// submitted base verbatim — enforce that the base exists on the remote here.
enforceBaseOnRemote: true,
...options
})
// Why: renderer eligibility can be stale by submit time; the main process
// is the last chance to avoid creating a PR from an out-of-date remote head.
return blockedEligibilityToCreateResult(eligibility)
return blockedEligibilityToCreateResult(eligibility, submittedBase)
} catch (error) {
console.warn('Hosted review creation preflight failed:', error)
return {
@@ -391,8 +457,22 @@ export async function getHostedReviewCreationEligibility(
connectionId: args.connectionId,
...hostedReviewExecutionContext(args)
})
const defaultBaseRef =
args.base?.trim() || (await getDefaultBaseRef(args.repoPath, args.connectionId, args))
// Why: an incoming base is only a *candidate* for the default merge target. A
// stacked worktree's parent base resolves on the remote only when it was
// actually pushed; a local-only parent must fall back to the repo default so
// the PR targets a ref the remote can resolve. Never regress to "no base" —
// keep the candidate if the repo default itself is unavailable.
const candidateBase = args.base?.trim() || null
const candidateBaseOnRemote =
candidateBase != null &&
(await baseRefExistsOnRemote(candidateBase, args.repoPath, args.connectionId, args))
let defaultBaseRef: string | null
if (candidateBase && candidateBaseOnRemote) {
defaultBaseRef = candidateBase
} else {
const repoDefaultBaseRef = await getDefaultBaseRef(args.repoPath, args.connectionId, args)
defaultBaseRef = repoDefaultBaseRef ?? candidateBase
}
const baseBranch = defaultBaseRef ? normalizeHostedReviewBaseRef(defaultBaseRef) : null
let review: Awaited<ReturnType<typeof getHostedReviewForBranch>> = null
try {
@@ -481,6 +561,20 @@ export async function getHostedReviewCreationEligibility(
if ((args.ahead ?? 0) > 0) {
return { ...baseResult, canCreate: false, blockedReason: 'needs_push', nextAction: 'push' }
}
// Why: at create-time, `gh pr create` (and the other providers) target the
// submitted base verbatim — Change 1 only corrects the *default*, not a stale
// renderer's submitted value. Block a local-only submitted base here so it
// fails with actionable copy instead of the provider's opaque error. Only the
// create-time preflight enforces this; the renderer's eligibility probe leaves
// enforceBaseOnRemote unset so a local-only parent is silently auto-corrected.
if (args.enforceBaseOnRemote && candidateBase && !candidateBaseOnRemote) {
return {
...baseResult,
canCreate: false,
blockedReason: 'base_not_on_remote',
nextAction: null
}
}
return { ...baseResult, canCreate: Boolean(baseBranch), blockedReason: null, nextAction: null }
}
@@ -143,14 +143,18 @@ describe('source-control Create PR intent flow helpers', () => {
).toEqual(['safe.ts', 'new.ts'])
})
it('prefers the current compare base over stale eligibility defaults', () => {
it('prefers the remote-validated eligibility default so it cannot diverge from the composer', () => {
// Why: the intent flow's eligibility is recomputed from the same compare
// base right before creation, so its default already corrects a local-only
// stacked parent to the repo default. The one-click path must target that
// same base as the composer, not the raw (possibly unpushable) compare base.
expect(
resolveCreatePrIntentReviewBase({
currentBaseRef: 'refs/remotes/origin/release',
currentBaseRef: 'stacked-parent',
eligibilityDefaultBaseRef: 'refs/remotes/origin/main',
composerBaseRef: 'main'
})
).toBe('release')
).toBe('main')
expect(
resolveCreatePrIntentReviewBase({
@@ -161,6 +165,19 @@ describe('source-control Create PR intent flow helpers', () => {
).toBe('develop')
})
it('falls back to the compare base when eligibility supplies no default', () => {
// Why: never blank the base. If the main process could not resolve a default
// (no candidate on remote and repo default unavailable), keep the user's
// compare base rather than dropping to an empty target.
expect(
resolveCreatePrIntentReviewBase({
currentBaseRef: 'refs/remotes/origin/release',
eligibilityDefaultBaseRef: null,
composerBaseRef: 'main'
})
).toBe('release')
})
it('resolves safe remote steps for publish, push, and patch-equivalent force-push', () => {
expect(
resolveCreatePrIntentRemoteStep({
@@ -103,10 +103,15 @@ export function resolveCreatePrIntentReviewBase({
eligibilityDefaultBaseRef?: string | null
composerBaseRef?: string | null
}): string {
// Why: the compare-base picker is the user's latest target; eligibility can
// lag behind while Create PR intent is preparing the branch.
// Why: prefer the remote-validated eligibility default over the raw compare
// base. The intent flow auto-submits, and its eligibility is recomputed from
// this same compare base right before creation — so `eligibilityDefaultBaseRef`
// already keeps a pushed base verbatim and corrects a local-only stacked parent
// to the repo default. Using it keeps the one-click path consistent with the
// composer instead of submitting a base the remote cannot resolve. Fall back to
// the compare base only when eligibility supplied no default.
return normalizeHostedReviewBaseRef(
currentBaseRef?.trim() || eligibilityDefaultBaseRef?.trim() || composerBaseRef?.trim() || ''
eligibilityDefaultBaseRef?.trim() || currentBaseRef?.trim() || composerBaseRef?.trim() || ''
)
}
@@ -66,6 +66,9 @@ export function resolveBlockedCreateReviewNoticeMessage(
case 'existing_review':
case 'fork_head_unsupported':
case 'unsupported_provider':
// Why: base_not_on_remote is a create-time hard failure surfaced as an error
// result, not an inline-actionable eligibility state, so it is non-clickable.
case 'base_not_on_remote':
case null:
return null
}
@@ -513,6 +513,8 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr
return `A ${createReviewCopy.reviewLabel} already exists`
case 'fork_head_unsupported':
return 'Fork head unsupported'
case 'base_not_on_remote':
return 'Base branch is not on the remote'
case null:
case undefined:
return upstreamLoading ? 'Checking branch status…' : 'Branch is not ready'
@@ -180,6 +180,7 @@ export function resolveDisabledCreatePrHeaderAction(
case 'existing_review':
case 'fork_head_unsupported':
case 'unsupported_provider':
case 'base_not_on_remote':
case null:
title = translate(
'auto.components.right.sidebar.source.control.primary.action.f0c6e2a581',
@@ -311,11 +312,7 @@ export function resolveCreatePrHeaderAction(inputs: PrimaryActionInputs): Primar
return createPrIntent
}
// Why: blocked notices are only for states the preparation intent cannot
// safely resolve, such as auth/default-branch/unsafe sync blockers.
if (canClickBlockedCreateReviewReason(inputs.hostedReviewCreation?.blockedReason)) {
return resolveDisabledCreatePrHeaderAction(inputs)
}
// Why: any remaining blocked state (including create-time-only base_not_on_remote)
// falls back to the disabled header action with its explanatory title.
return resolveDisabledCreatePrHeaderAction(inputs)
}
@@ -155,14 +155,36 @@ describe('useCreatePullRequestDialogFields', () => {
}
})
it('prefers the selected current base ref over stale eligibility defaults', async () => {
it('prefers the remote-validated eligibility default over a stacked local-only base', async () => {
// Why: for a stacked worktree the current base is the local-only parent
// branch, which the main process resolves to the repo default. The seeded
// field must follow the remote-validated eligibility default, not the parent.
const harness = renderDialogFields({
eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/main' }),
currentBaseRef: 'refs/remotes/origin/release'
currentBaseRef: 'stacked-parent'
})
try {
await harness.rerender({
eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/main' }),
currentBaseRef: 'stacked-parent'
})
expect(harness.current().base).toBe('main')
} finally {
harness.unmount()
}
})
it('falls back to the current base ref when eligibility supplies no default', async () => {
// Why: when the main process cannot resolve a default (e.g. origin/HEAD
// unset and no probes match), keep the current base rather than blanking it.
const harness = renderDialogFields({
eligibility: createEligibility({ defaultBaseRef: null }),
currentBaseRef: 'refs/remotes/origin/release'
})
try {
await harness.rerender({
eligibility: createEligibility({ defaultBaseRef: null }),
currentBaseRef: 'refs/remotes/origin/release'
})
@@ -212,7 +234,7 @@ describe('useCreatePullRequestDialogFields', () => {
const seedRevisions = { ...harness.current().fieldRevisions }
await harness.rerender({
eligibility: createEligibility(),
eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/release' }),
currentBaseRef: 'refs/remotes/origin/release'
})
expect(harness.current().base).toBe('release')
@@ -91,7 +91,13 @@ function resolveCreateReviewDefaultBaseRef({
currentBaseRef?: string | null
eligibilityDefaultBaseRef?: string | null
}): string {
return stripBaseRef(currentBaseRef?.trim() || eligibilityDefaultBaseRef?.trim() || '')
// Why: prefer the remote-validated main-process default over the worktree's
// local parent base. For a stacked worktree whose parent is local-only,
// `currentBaseRef` is that unpushable parent; the eligibility default has
// already fallen back to a ref the remote can resolve. Fall back to
// `currentBaseRef` only when eligibility supplied no default. Manual
// `setUserBase` still wins via the base-resync suppression.
return stripBaseRef(eligibilityDefaultBaseRef?.trim() || currentBaseRef?.trim() || '')
}
export function normalizeCreateReviewBaseSearchResults(
@@ -1832,14 +1832,29 @@ describe('agent completion coordinator', () => {
const turn = { prompt: 'fix the bug', agentType: 'codex' as const }
coordinator.observeHookStatus({ state: 'working', ...turn })
coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command', toolInput: 'ls' })
coordinator.observeHookStatus({
state: 'waiting',
...turn,
toolName: 'exec_command',
toolInput: 'ls'
})
// First pause auto-resolves before the window elapses.
coordinator.observeHookStatus({ state: 'working', ...turn, toolName: 'exec_command', toolInput: 'ls' })
coordinator.observeHookStatus({
state: 'working',
...turn,
toolName: 'exec_command',
toolInput: 'ls'
})
vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS)
expect(dispatchAttention).not.toHaveBeenCalled()
// A later, genuinely-distinct pause must re-arm the debounce and fire.
coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'apply_patch', toolInput: 'diff' })
coordinator.observeHookStatus({
state: 'waiting',
...turn,
toolName: 'apply_patch',
toolInput: 'diff'
})
expect(dispatchAttention).not.toHaveBeenCalled()
vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS)
expect(dispatchAttention).toHaveBeenCalledTimes(1)
@@ -112,9 +112,8 @@ describe('agent hook completion notifications', () => {
// Why: the Codex permission-pause tests share a working→pause→quiet-window
// sequence; centralizing it keeps the debounce advance (issue #8387) in one spot.
async function observeCodexPermissionPause(state: 'waiting' | 'blocked'): Promise<void> {
const { observeAgentHookCompletionForNotification } = await import(
'./agent-hook-completion-notifications'
)
const { observeAgentHookCompletionForNotification } =
await import('./agent-hook-completion-notifications')
observeAgentHookCompletionForNotification({
paneKey,
worktreeId: 'wt-1',
+4
View File
@@ -104,6 +104,10 @@ export type HostedReviewCreationBlockedReason =
| 'fork_head_unsupported'
| 'unsupported_provider'
| 'existing_review'
// Why: a stacked worktree's local-only parent base is unresolvable on the
// remote; blocked at create-time so the submit fails with actionable copy
// instead of the provider's opaque error.
| 'base_not_on_remote'
| null
export type HostedReviewCreationNextAction =