From 6754901d1c84e807ca6e53d7493df7df07a47c8a Mon Sep 17 00:00:00 2001 From: zpyoung Date: Mon, 19 Oct 2026 11:49:39 -0500 Subject: [PATCH] fix(hosted-review-sitter): wait out a running GitHub workflow before rerunning it GitHub refuses rerun-failed-jobs with "This workflow is already running (HTTP 403)" until every job in the run finishes. The sitter fired the rerun as soon as one job failed, the refusal settled as indeterminate, and the decision then emitted a landing-failed deviation every tick for that head. - Decline with awaiting-workflow-run while any other check in the same workflow run is pending. - Classify the "already running" 403 as not-landed (reason workflow-run-in-progress), unless an earlier workflow in the same action already reran. - Fold the run's other check observations into the rerun evidence key, so a not-landed rerun gets a fresh fingerprint once the run moves and the gate lets it retry. check-runs/{id}/rerequest has no run-level lock, so it keeps its current handling. GitLab job retries are per job and are unaffected. --- .../provider-github-actions.ts | 45 +++-- .../provider-github-rerun.test.ts | 147 +++++++++++++++++ .../decision-action-builders.ts | 13 +- .../decision-check-groups.ts | 28 ++++ .../decision-workflow-run.test.ts | 154 ++++++++++++++++++ .../fork-hosted-review-sitter/decision.ts | 15 +- 6 files changed, 385 insertions(+), 17 deletions(-) create mode 100644 src/main/fork-hosted-review-sitter/provider-github-rerun.test.ts create mode 100644 src/shared/fork-hosted-review-sitter/decision-workflow-run.test.ts 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 (