diff --git a/src/main/fork-hosted-review-sitter/provider-github-actions.ts b/src/main/fork-hosted-review-sitter/provider-github-actions.ts index 1005eb783c26..3e1fa131aed3 100644 --- a/src/main/fork-hosted-review-sitter/provider-github-actions.ts +++ b/src/main/fork-hosted-review-sitter/provider-github-actions.ts @@ -5,6 +5,7 @@ import type { HostedReviewSitterDefinition } from '../../shared/fork-hosted-review-sitter/types' import { areCurrentHeadChecksGreen } from '../../shared/fork-hosted-review-sitter/decision-check-groups' +import { extractExecError } from '../git/exec-error' import { throwIfAborted, type HostedReviewSitterGitExecution } from './provider-git' import { expectedStateMismatch, @@ -114,6 +115,12 @@ function parseRerunTargets(checkIds: readonly string[]): { return { workflowIds: [...workflowIds], checkIds: [...directCheckIds] } } +/** GitHub's refusal to rerun a run that has not finished; it guarantees no job was requeued. */ +function isWorkflowRunInProgressRefusal(error: unknown): boolean { + const { stderr } = extractExecError(error) + return /already running[\s\S]*HTTP 403/i.test(stderr) +} + async function rerunChecks( state: GitHubState, action: Extract, @@ -136,22 +143,34 @@ async function rerunChecks( if (targets.workflowIds.length === 0 && targets.checkIds.length === 0) { throw new Error('GitHub returned no rerunnable failed checks.') } + let rerunAccepted = false for (const workflowId of targets.workflowIds) { throwIfAborted(signal) await assertLeaseHeld() - await runGitHubApi( - state.ownerRepo, - state.ghOptions, - 'core', - [ - 'api', - '-X', - 'POST', - `repos/${state.ownerRepo.owner}/${state.ownerRepo.repo}/actions/runs/${workflowId}/rerun-failed-jobs` - ], - signal, - () => tracker.markDispatched() - ) + try { + await runGitHubApi( + state.ownerRepo, + state.ghOptions, + 'core', + [ + 'api', + '-X', + 'POST', + `repos/${state.ownerRepo.owner}/${state.ownerRepo.repo}/actions/runs/${workflowId}/rerun-failed-jobs` + ], + signal, + () => tracker.markDispatched() + ) + } catch (error) { + if (!rerunAccepted && isWorkflowRunInProgressRefusal(error)) { + throw Object.assign(error instanceof Error ? error : new Error(String(error)), { + effect: 'not-landed' as const, + reason: 'workflow-run-in-progress' + }) + } + throw error + } + rerunAccepted = true } for (const checkId of targets.checkIds) { throwIfAborted(signal) diff --git a/src/main/fork-hosted-review-sitter/provider-github-rerun.test.ts b/src/main/fork-hosted-review-sitter/provider-github-rerun.test.ts new file mode 100644 index 000000000000..0f49eed62694 --- /dev/null +++ b/src/main/fork-hosted-review-sitter/provider-github-rerun.test.ts @@ -0,0 +1,147 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { buildRerunAction } from '../../shared/fork-hosted-review-sitter/decision-action-builders' +import type { + HostedReviewCheckSnapshot, + HostedReviewSitterDefinition, + HostedReviewSnapshot +} from '../../shared/fork-hosted-review-sitter/types' +import { createHostedReviewSitterMutationTracker } from './provider-action-effect' +import { executeGitHubSitterAction } from './provider-github-actions' +import type { GitHubState } from './provider-github-read' +import type { HostedReviewSitterGitExecution } from './provider-git' + +const mocks = vi.hoisted(() => ({ + loadGitHubState: vi.fn(), + runGitHubApi: vi.fn() +})) + +vi.mock('./provider-github-read', () => ({ + ENQUEUE_PULL_REQUEST_MUTATION: 'mutation EnqueuePullRequest', + loadGitHubState: mocks.loadGitHubState, + runGitHubApi: mocks.runGitHubApi, + stringValue: (value: unknown) => (typeof value === 'string' ? value : '') +})) + +vi.mock('./provider-git', () => ({ throwIfAborted: vi.fn() })) + +const HEAD = 'head-1' + +function failedCheck(checkId: string, checkKey: string): HostedReviewCheckSnapshot { + return { + checkKey, + checkId, + name: checkKey, + required: true, + headSha: HEAD, + state: 'failed', + observationId: `${checkId}:1`, + failureSignature: null + } +} + +function snapshot(checks: HostedReviewCheckSnapshot[]): HostedReviewSnapshot { + return { + provider: 'github', + reviewNumber: 42, + url: 'https://github.com/acme/repo/pull/42', + lifecycle: 'open', + headSha: HEAD, + baseSha: 'base-1', + draft: false, + checks, + checksComplete: true, + providerReadiness: { verdict: 'blocked', blockers: ['checks'] }, + behindBase: false, + conflicts: 'none', + queue: { required: false, membership: 'not-enqueued' }, + defaultMergeMethod: 'squash' + } +} + +const definition: HostedReviewSitterDefinition = { + repoId: 'repo-1', + worktreeId: 'worktree-1', + repoPath: '/repo', + branch: 'feature', + provider: 'github', + reviewNumber: 42, + reviewUrl: 'https://github.com/acme/repo/pull/42', + capabilities: { updateBranch: 'on', resolveConflicts: 'on', fixChecks: 'on', merge: 'on' }, + branchUpdateMode: 'merge-base-update', + mergeMethod: null, + mergeCheckScope: 'required' +} + +function githubState(review: HostedReviewSnapshot): GitHubState { + return { + snapshot: review, + ownerRepo: { owner: 'acme', repo: 'repo' }, + ghOptions: {}, + pullRequestId: 'pr-42', + baseRefName: 'main', + sourceIdentityComplete: true + } +} + +function ghFailure(stderr: string): Error { + return Object.assign(new Error('Command failed: gh api'), { stderr }) +} + +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: a rerun never touches a Git method. +const git = {} as unknown as HostedReviewSitterGitExecution + +async function rerun(checks: HostedReviewCheckSnapshot[]) { + const review = snapshot(checks) + mocks.loadGitHubState.mockResolvedValue(githubState(review)) + const action = buildRerunAction(review, { checkKey: checks[0]!.checkKey, checks }) + return executeGitHubSitterAction( + definition, + action, + git, + undefined, + createHostedReviewSitterMutationTracker(), + async () => {} + ) +} + +beforeEach(() => { + vi.clearAllMocks() +}) + +describe('GitHub sitter check reruns', () => { + it('reports a rerun refused because the workflow run is still going as landing nothing', async () => { + mocks.runGitHubApi.mockRejectedValueOnce( + ghFailure('gh: This workflow is already running (HTTP 403)\n') + ) + + await expect(rerun([failedCheck('workflow:900:check:1', 'test')])).rejects.toMatchObject({ + effect: 'not-landed', + reason: 'workflow-run-in-progress' + }) + }) + + it('keeps the refusal indeterminate once an earlier workflow in the same action reran', async () => { + mocks.runGitHubApi + .mockResolvedValueOnce('') + .mockRejectedValueOnce(ghFailure('gh: This workflow is already running (HTTP 403)\n')) + + const error: unknown = await rerun([ + failedCheck('workflow:900:check:1', 'test'), + failedCheck('workflow:901:check:2', 'test') + ]).catch((caught: unknown) => caught) + expect(error).toBeInstanceOf(Error) + expect(error).not.toHaveProperty('effect') + }) + + it('leaves other forbidden responses indeterminate', async () => { + mocks.runGitHubApi.mockRejectedValueOnce( + ghFailure('gh: Resource not accessible by integration (HTTP 403)\n') + ) + + const error: unknown = await rerun([failedCheck('workflow:900:check:1', 'test')]).catch( + (caught: unknown) => caught + ) + expect(error).toBeInstanceOf(Error) + expect(error).not.toHaveProperty('effect') + }) +}) diff --git a/src/shared/fork-hosted-review-sitter/decision-action-builders.ts b/src/shared/fork-hosted-review-sitter/decision-action-builders.ts index bb786ad2e1a8..6786237fc15a 100644 --- a/src/shared/fork-hosted-review-sitter/decision-action-builders.ts +++ b/src/shared/fork-hosted-review-sitter/decision-action-builders.ts @@ -3,7 +3,11 @@ import { hostedReviewContentIdentity, makeHostedReviewEvidenceKey } from './action-identity' -import { currentHeadChecks, type FailedCheckGroup } from './decision-check-groups' +import { + currentHeadChecks, + workflowRunSiblingChecks, + type FailedCheckGroup +} from './decision-check-groups' import type { HostedReviewAttemptEntry, HostedReviewCheckSnapshot, @@ -29,12 +33,17 @@ export function buildRerunAction( .filter((signature): signature is string => signature !== null) .sort() const failureSignature = signatures[0] ?? null + // the gate retries a not-landed rerun only on new evidence; its workflow run moving is that evidence + const siblingObservationIds = workflowRunSiblingChecks(review, group) + .map((check) => check.observationId) + .sort() const evidenceKey = makeHostedReviewEvidenceKey([ 'rerun-check', review.headSha, group.checkKey, failureSignature, - ...observationIds + ...observationIds, + ...(siblingObservationIds.length > 0 ? ['workflow-run', ...siblingObservationIds] : []) ]) return { kind: 'rerun-check', diff --git a/src/shared/fork-hosted-review-sitter/decision-check-groups.ts b/src/shared/fork-hosted-review-sitter/decision-check-groups.ts index 349f9e817cb3..9d5597f1056f 100644 --- a/src/shared/fork-hosted-review-sitter/decision-check-groups.ts +++ b/src/shared/fork-hosted-review-sitter/decision-check-groups.ts @@ -90,6 +90,34 @@ export function areCurrentHeadChecksGreen( return true } +function githubWorkflowRunId(check: HostedReviewCheckSnapshot): string | null { + return check.checkId.match(/^workflow:(\d+):check:/)?.[1] ?? null +} + +/** + * Current-head checks outside the group that share a GitHub workflow run with it, in any scope. + * GitHub refuses to rerun a run's failed jobs until every job in that run has finished. + */ +export function workflowRunSiblingChecks( + review: HostedReviewSnapshot, + group: FailedCheckGroup +): readonly HostedReviewCheckSnapshot[] { + if (review.provider !== 'github') { + return [] + } + const runIds = new Set(group.checks.map(githubWorkflowRunId).filter((id) => id !== null)) + const own = new Set(group.checks.map((check) => check.checkId)) + return review.checks.filter((check) => { + const runId = githubWorkflowRunId(check) + return ( + check.headSha === review.headSha && + runId !== null && + runIds.has(runId) && + !own.has(check.checkId) + ) + }) +} + /** Failed current-head checks in scope, grouped by check key in a stable order. */ export function failedCheckGroups( review: HostedReviewSnapshot, diff --git a/src/shared/fork-hosted-review-sitter/decision-workflow-run.test.ts b/src/shared/fork-hosted-review-sitter/decision-workflow-run.test.ts new file mode 100644 index 000000000000..94ea2cf7dd26 --- /dev/null +++ b/src/shared/fork-hosted-review-sitter/decision-workflow-run.test.ts @@ -0,0 +1,154 @@ +import { describe, expect, it } from 'vitest' +import type { WatcherLedger } from '../fork-heimdall/ledger-types' +import { hostedReviewAttemptFingerprint } from './action-identity' +import { explainDesiredAction } from './decision' +import type { + HostedReviewAttemptEntry, + HostedReviewCheckSnapshot, + HostedReviewSitterAction, + HostedReviewSitterDefinition, + HostedReviewSnapshot +} from './types' + +const HEAD = 'head-1' + +function check(overrides: Partial = {}): HostedReviewCheckSnapshot { + return { + checkKey: 'test', + checkId: 'workflow:900:check:1', + name: 'test', + required: true, + headSha: HEAD, + state: 'failed', + observationId: 'test:1', + failureSignature: 'failure:test', + ...overrides + } +} + +function sibling(overrides: Partial = {}): HostedReviewCheckSnapshot { + return check({ + checkKey: 'lint', + checkId: 'workflow:900:check:2', + name: 'lint', + required: false, + state: 'passed', + observationId: 'lint:1', + failureSignature: null, + ...overrides + }) +} + +function review( + checks: HostedReviewCheckSnapshot[], + provider: HostedReviewSnapshot['provider'] = 'github' +): HostedReviewSnapshot { + return { + provider, + reviewNumber: 42, + url: 'https://github.com/acme/repo/pull/42', + lifecycle: 'open', + headSha: HEAD, + baseSha: 'base-1', + draft: false, + checks, + checksComplete: true, + providerReadiness: { verdict: 'blocked', blockers: ['checks'] }, + behindBase: false, + conflicts: 'none', + queue: { required: false, membership: 'not-enqueued' }, + defaultMergeMethod: 'squash' + } +} + +const sitter: HostedReviewSitterDefinition = { + repoId: 'repo-1', + worktreeId: 'worktree-1', + repoPath: '/repo', + branch: 'feature', + provider: 'github', + reviewNumber: 42, + reviewUrl: 'https://github.com/acme/repo/pull/42', + capabilities: { updateBranch: 'on', resolveConflicts: 'on', fixChecks: 'on', merge: 'on' }, + branchUpdateMode: 'merge-base-update', + mergeMethod: null, + mergeCheckScope: 'required' +} + +function notLandedAttempt(action: HostedReviewSitterAction): HostedReviewAttemptEntry { + return { + kind: 'attempt', + class: 'fact', + origin: 'owner', + watcherId: 'sitter-1', + eventId: `event:${action.evidenceKey}`, + attemptId: `attempt:${action.evidenceKey}`, + atMs: 1_000, + action, + fingerprint: hostedReviewAttemptFingerprint(action), + state: 'settled', + effect: 'not-landed', + reason: 'workflow-run-in-progress' + } +} + +function ledger(entries: readonly HostedReviewAttemptEntry[] = []): WatcherLedger { + return { watcherId: 'sitter-1', entries } +} + +function decide(input: HostedReviewSnapshot, history: WatcherLedger = ledger()) { + return explainDesiredAction(input, sitter, history, { freshness: 'live', preparedCommit: null }) +} + +describe('PR sitter reruns against a GitHub workflow run', () => { + it('waits for the rest of the workflow run before rerunning its failed job', () => { + const otherRun = sibling({ + checkKey: 'docs', + checkId: 'workflow:901:check:3', + state: 'pending', + observationId: 'docs:1' + }) + + const outcome = decide(review([check(), sibling({ state: 'pending' }), otherRun])) + expect(outcome.action).toBeNull() + if (outcome.action === null && 'considered' in outcome) { + expect(outcome.considered).toContainEqual( + expect.objectContaining({ phase: 'fix-checks', reason: 'awaiting-workflow-run' }) + ) + } + + expect(decide(review([check(), sibling(), otherRun])).action).toMatchObject({ + kind: 'rerun-check', + checkIds: ['workflow:900:check:1'] + }) + }) + + it('does not wait on a GitLab pipeline, whose jobs retry individually', () => { + const job = check({ checkId: 'job:acme%2Frepo:1' }) + const running = sibling({ checkId: 'job:acme%2Frepo:2', state: 'pending' }) + + expect(decide(review([job, running], 'gitlab')).action).toMatchObject({ + kind: 'rerun-check' + }) + }) + + it('retries a rerun that landed nothing once its workflow run moves on', () => { + const refused = decide(review([check(), sibling()])).action + if (refused?.kind !== 'rerun-check') { + throw new Error('expected rerun') + } + const history = ledger([notLandedAttempt(refused)]) + + // the gate holds an identical fingerprint, so a retry needs evidence the run moved + const unchanged = decide(review([check(), sibling()]), history).action + expect(unchanged && hostedReviewAttemptFingerprint(unchanged)).toBe( + hostedReviewAttemptFingerprint(refused) + ) + + const retry = decide(review([check(), sibling({ observationId: 'lint:2' })]), history).action + expect(retry).toMatchObject({ kind: 'rerun-check', checkIds: ['workflow:900:check:1'] }) + expect(retry && hostedReviewAttemptFingerprint(retry)).not.toBe( + hostedReviewAttemptFingerprint(refused) + ) + }) +}) diff --git a/src/shared/fork-hosted-review-sitter/decision.ts b/src/shared/fork-hosted-review-sitter/decision.ts index b572773c07f7..0e09ae71fdb3 100644 --- a/src/shared/fork-hosted-review-sitter/decision.ts +++ b/src/shared/fork-hosted-review-sitter/decision.ts @@ -24,6 +24,7 @@ import { freshFailedChecksAfterRerun, groupRequiresOwnerForRecovery, primaryFailedCheckGroup, + workflowRunSiblingChecks, type FailedCheckGroup } from './decision-check-groups' import type { @@ -69,6 +70,7 @@ export type HostedReviewSitterNoActionReason = | 'rerun-in-flight' | 'rerun-unresolved' | 'awaiting-rerun-result' + | 'awaiting-workflow-run' | 'failure-signature-unavailable' | 'check-rerun-unavailable' | 'fix-already-attempted' @@ -146,6 +148,15 @@ function latestRerunForCheck( return latest } +function rerunWhenWorkflowRunSettled( + review: HostedReviewSnapshot, + group: FailedCheckGroup +): PhaseOutcome { + return workflowRunSiblingChecks(review, group).some((check) => check.state === 'pending') + ? declined('awaiting-workflow-run', group.checkKey) + : { action: buildRerunAction(review, group) } +} + function desiredFixAction( review: HostedReviewSnapshot, ledger: WatcherLedger, @@ -169,14 +180,14 @@ function desiredFixAction( } else { const rerunEntry = latestRerunForCheck(ledger, review.headSha, group.checkKey) if (!rerunEntry) { - return { action: buildRerunAction(review, group) } + return rerunWhenWorkflowRunSettled(review, group) } const rerunDisposition = getHostedReviewAttemptDisposition(ledger, rerunEntry.action) if (rerunDisposition === 'in-flight') { return declined('rerun-in-flight') } if (rerunDisposition === 'retryable-failure') { - return { action: buildRerunAction(review, group) } + return rerunWhenWorkflowRunSettled(review, group) } if (rerunDisposition === 'unresolved') { if (