From 3313a6739cf65ba963aef0983ee281a06c5b4090 Mon Sep 17 00:00:00 2001 From: zpyoung Date: Mon, 19 Oct 2026 11:43:42 -0500 Subject: [PATCH] fix(hosted-review-sitter): key GitHub conflict work on the live base tip GitHub refreshes a pull request's baseRefOid only when the PR is pushed, but computes mergeability against the base branch's current tip. The sitter read baseRefOid, so after the base advanced it dispatched conflict preparation against an old base commit the head already contained. The worker's merge was a no-op, the attempt settled unverifiable, and because (head, base) never changed the sitter raised the same deviation every tick. Read baseRef.target.oid in the same GraphQL query instead, matching the GitLab reader, which already resolves the target branch tip. An unreadable base ref now leaves baseSha empty (evidence incomplete) rather than falling back to the stale commit. A no-op conflict preparation stays non-retryable for the same (head, base): rerunning that merge is deterministic, and a moved base already produces a fresh attempt fingerprint. --- .../provider-github-read.test.ts | 124 ++++++++++++++++++ .../provider-github-read.ts | 7 +- .../decision.test.ts | 25 ++++ 3 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 src/main/fork-hosted-review-sitter/provider-github-read.test.ts 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,