diff --git a/src/main/fork-hosted-review-sitter/provider-github-read.test.ts b/src/main/fork-hosted-review-sitter/provider-github-read.test.ts new file mode 100644 index 000000000000..5fe91985668a --- /dev/null +++ b/src/main/fork-hosted-review-sitter/provider-github-read.test.ts @@ -0,0 +1,124 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { HostedReviewSitterDefinition } from '../../shared/fork-hosted-review-sitter/types' +import type { HostedReviewSitterGitExecution } from './provider-git' +import { readGitHubSitterSnapshot } from './provider-github-read' + +const mocks = vi.hoisted(() => ({ + ghExecFileAsync: vi.fn(), + simulateConflicts: vi.fn() +})) + +vi.mock('../github/gh-utils', () => ({ + acquire: vi.fn(async () => {}), + release: vi.fn(), + extractExecError: () => ({ stderr: '' }), + ghExecFileAsync: mocks.ghExecFileAsync +})) +vi.mock('../github/client/lookup/pr-lookup-rate-limit', () => ({ + assertRateLimitBudget: vi.fn(async () => {}) +})) +vi.mock('../github/rate-limit', () => ({ noteRepositoryRateLimitSpend: vi.fn() })) +vi.mock('../github/github-api-repository', () => ({ + githubHostExecOptions: () => ({}), + resolveGitHubRepoExecution: vi.fn(async () => ({ + ownerRepo: { owner: 'acme', repo: 'widgets' }, + ghOptions: {} + })) +})) +vi.mock('../source-control/hosted-review', () => ({ getHostedReviewForBranch: vi.fn() })) +vi.mock('../github/client', () => ({ getPRCheckDetails: vi.fn() })) + +const HEAD = 'a'.repeat(40) +const RECORDED_BASE = 'b'.repeat(40) +const LIVE_BASE = 'c'.repeat(40) + +const definition: HostedReviewSitterDefinition = { + repoId: 'repo-1', + worktreeId: 'worktree-1', + repoPath: '/repo', + branch: 'feature', + provider: 'github', + reviewNumber: 52, + reviewUrl: 'https://github.com/acme/widgets/pull/52', + capabilities: { updateBranch: 'on', resolveConflicts: 'on', fixChecks: 'on', merge: 'on' }, + branchUpdateMode: 'merge-base-update', + mergeMethod: null, + mergeCheckScope: 'all' +} + +// The reader touches only the push target and conflict simulation; every other Git method stays unused. +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: readGitHubSitterSnapshot calls only reviewPushTarget and simulateConflicts. +const git = { + reviewPushTarget: async () => ({ branchName: 'feature' }), + simulateConflicts: mocks.simulateConflicts +} as unknown as HostedReviewSitterGitExecution + +function pullRequestPayload(pullRequest: Record): string { + return JSON.stringify({ + data: { + repository: { + viewerDefaultMergeMethod: 'SQUASH', + squashMergeAllowed: true, + pullRequest: { + id: 'PR_1', + url: definition.reviewUrl, + state: 'OPEN', + merged: false, + isDraft: false, + headRefOid: HEAD, + headRefName: 'feature', + headRepository: { nameWithOwner: 'acme/widgets' }, + baseRefName: 'main', + // github refreshes baseRefOid only on a push to the PR, so it lags the base branch + baseRefOid: RECORDED_BASE, + mergeStateStatus: 'DIRTY', + isInMergeQueue: false, + isMergeQueueEnabled: false, + mergeQueueEntry: null, + commits: { nodes: [{ commit: { oid: HEAD, statusCheckRollup: null } }] }, + ...pullRequest + } + } + } + }) +} + +function respondWith(pullRequest: Record): void { + mocks.ghExecFileAsync.mockImplementation(async (args: string[]) => ({ + stdout: args.includes('graphql') ? pullRequestPayload(pullRequest) : '[]', + stderr: '' + })) +} + +beforeEach(() => { + vi.clearAllMocks() + mocks.simulateConflicts.mockResolvedValue('present') +}) + +describe('GitHub sitter snapshot base SHA', () => { + it('reports the base branch tip rather than the PR-recorded base commit', async () => { + respondWith({ mergeable: 'CONFLICTING', baseRef: { target: { oid: LIVE_BASE } } }) + + const snapshot = await readGitHubSitterSnapshot(definition, git, { fresh: true }) + + expect(snapshot.baseSha).toBe(LIVE_BASE) + expect(snapshot.conflicts).toBe('present') + }) + + it('simulates unknown mergeability against the base branch tip', async () => { + respondWith({ mergeable: 'UNKNOWN', baseRef: { target: { oid: LIVE_BASE } } }) + + await readGitHubSitterSnapshot(definition, git, { fresh: true }) + + expect(mocks.simulateConflicts).toHaveBeenCalledWith(HEAD, LIVE_BASE, undefined) + }) + + it('leaves the base unknown instead of falling back to the stale recorded base', async () => { + respondWith({ mergeable: 'CONFLICTING', baseRef: null }) + + const snapshot = await readGitHubSitterSnapshot(definition, git, { fresh: true }) + + expect(snapshot.baseSha).toBe('') + expect(snapshot.providerReadiness.verdict).not.toBe('ready') + }) +}) diff --git a/src/main/fork-hosted-review-sitter/provider-github-read.ts b/src/main/fork-hosted-review-sitter/provider-github-read.ts index 76b74df8c0e0..268a5977e7e8 100644 --- a/src/main/fork-hosted-review-sitter/provider-github-read.ts +++ b/src/main/fork-hosted-review-sitter/provider-github-read.ts @@ -36,8 +36,8 @@ query($owner: String!, $repo: String!, $pr: Int!, $after: String) { headRefOid headRefName headRepository { nameWithOwner } - baseRefOid baseRefName + baseRef { target { oid } } mergeable mergeStateStatus reviewDecision @@ -105,8 +105,8 @@ export type GitHubPullRequestState = { headRefOid?: unknown headRefName?: unknown headRepository?: { nameWithOwner?: unknown } | null - baseRefOid?: unknown baseRefName?: unknown + baseRef?: { target?: { oid?: unknown } | null } | null mergeable?: unknown mergeStateStatus?: unknown reviewDecision?: unknown @@ -397,7 +397,8 @@ export async function loadGitHubState( const pages = await loadGitHubPages(ownerRepo, ghOptions, definition.reviewNumber, fresh, signal) const pr = pages.repository.pullRequest! const headSha = stringValue(pr.headRefOid) - const baseSha = stringValue(pr.baseRefOid) + // baseRefOid lags the base branch until the PR is pushed, but mergeability tracks the live tip + const baseSha = stringValue(pr.baseRef?.target?.oid) const baseRefName = stringValue(pr.baseRefName) const reviewPushTarget = fresh ? await git.reviewPushTarget(signal) : null const sourceIdentityComplete = diff --git a/src/shared/fork-hosted-review-sitter/decision.test.ts b/src/shared/fork-hosted-review-sitter/decision.test.ts index 769a0f385d02..ac3fbb8d90c6 100644 --- a/src/shared/fork-hosted-review-sitter/decision.test.ts +++ b/src/shared/fork-hosted-review-sitter/decision.test.ts @@ -9,6 +9,7 @@ import { hostedReviewAttemptFingerprint, hostedReviewContentIdentity } from './a import { deriveHostedReviewSitterDiscrepancies } from './reconciliation' import { paceHostedReview } from './kind-knowledge' import { HOSTED_REVIEW_STOP_PREDICATES } from './stop-policy' +import { buildPrepareConflictAction } from './decision-action-builders' import type { HostedReviewCheckSnapshot, HostedReviewPreparedCommit, @@ -478,6 +479,30 @@ describe('PR sitter desired-action safety policy', () => { }) }) + it('retries conflict preparation only once the base moves past an unverifiable attempt', () => { + // a worker that merged an already-contained base reports failure with no commit; rerunning the + // same (head, base) merge is deterministic, so only a new base tip earns another dispatch + const conflicted = review({ + conflicts: 'present', + providerReadiness: { verdict: 'blocked', blockers: ['conflicts'] } + }) + const history = ledger([ + { + kind: 'action', + eventId: 'prepare-attempt', + actionId: 'prepare-stale-base', + atMs: 1_000, + action: buildPrepareConflictAction(conflicted), + state: 'failed' + } + ]) + + expect(computeDesiredAction(conflicted, sitter(), history)).toBeNull() + expect( + computeDesiredAction({ ...conflicted, baseSha: 'base-2' }, sitter(), history) + ).toMatchObject({ kind: 'prepare-conflict-resolution', headSha: HEAD, baseSha: 'base-2' }) + }) + it('still holds conflict resolution on a draft review', () => { const conflicted = review({ draft: true,