From 1ef0551bc138724d736583eac788f4183bef07e2 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:10:50 -0700 Subject: [PATCH] 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 * 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 --- src/cli/handlers/core.test.ts | 4 +- .../windows-firewall-remote-scope.test.ts | 10 +- ...hosted-review-creation-eligibility.test.ts | 529 ++++++++++++++++++ .../hosted-review-creation.test.ts | 282 ++-------- .../source-control/hosted-review-creation.ts | 108 +++- ...urce-control-create-pr-intent-flow.test.ts | 23 +- .../source-control-create-pr-intent-flow.ts | 11 +- ...ce-control-create-review-blocked-action.ts | 3 + .../source-control-dropdown-items.ts | 2 + ...control-primary-create-pr-intent-action.ts | 9 +- .../useCreatePullRequestDialogFields.test.ts | 28 +- .../useCreatePullRequestDialogFields.ts | 8 +- .../agent-completion-coordinator.test.ts | 21 +- ...gent-hook-completion-notifications.test.ts | 5 +- src/shared/hosted-review.ts | 4 + 15 files changed, 767 insertions(+), 280 deletions(-) create mode 100644 src/main/source-control/hosted-review-creation-eligibility.test.ts diff --git a/src/cli/handlers/core.test.ts b/src/cli/handlers/core.test.ts index 69f27dc9bd6..ac5444566d9 100644 --- a/src/cli/handlers/core.test.ts +++ b/src/cli/handlers/core.test.ts @@ -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 diff --git a/src/main/runtime/windows-firewall-remote-scope.test.ts b/src/main/runtime/windows-firewall-remote-scope.test.ts index 2c9a013d964..6e2462c0334 100644 --- a/src/main/runtime/windows-firewall-remote-scope.test.ts +++ b/src/main/runtime/windows-firewall-remote-scope.test.ts @@ -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 ) diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts new file mode 100644 index 00000000000..010d950d50f --- /dev/null +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -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[0]> = {} + ): Parameters[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() + }) +}) diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index 3ae82fd4010..04c751d7c3b 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -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() - }) -}) diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 01caac804d8..1e19f1d4fa8 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -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 { + 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, - 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, 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> = 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 } } diff --git a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts index 74853dd4147..516c0711369 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts @@ -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({ diff --git a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts index 7e183122ec5..566ee54e1d7 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts @@ -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() || '' ) } diff --git a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts index e59354da5c8..2475a7d293e 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts @@ -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 } diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts index 58bbfe9454e..62801dcfe31 100644 --- a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts @@ -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' diff --git a/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts b/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts index ce4849a89f9..6ee7bce037f 100644 --- a/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts +++ b/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts @@ -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) } diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts index e23db872e1b..d09fc41cc32 100644 --- a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts @@ -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') diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts index a480689334e..08a3b311de6 100644 --- a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts @@ -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( diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts index 6a4c8b197cf..957e0a9bb1e 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts @@ -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) diff --git a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts index 1801c78bace..228af0058eb 100644 --- a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts +++ b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts @@ -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 { - const { observeAgentHookCompletionForNotification } = await import( - './agent-hook-completion-notifications' - ) + const { observeAgentHookCompletionForNotification } = + await import('./agent-hook-completion-notifications') observeAgentHookCompletionForNotification({ paneKey, worktreeId: 'wt-1', diff --git a/src/shared/hosted-review.ts b/src/shared/hosted-review.ts index 864baceba87..04094cf0774 100644 --- a/src/shared/hosted-review.ts +++ b/src/shared/hosted-review.ts @@ -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 =