diff --git a/.github/workflows/pr-guards.yml b/.github/workflows/pr-guards.yml index 09325fcde..a9ad3f096 100644 --- a/.github/workflows/pr-guards.yml +++ b/.github/workflows/pr-guards.yml @@ -4,7 +4,7 @@ on: pull_request: types: [opened, synchronize, reopened, ready_for_review] pull_request_review: - types: [submitted, dismissed] + types: [submitted, edited, dismissed] pull_request_review_comment: types: [created, edited, deleted] issue_comment: @@ -1170,6 +1170,7 @@ jobs: group: ${{ github.workflow }}-engine-review-pass-${{ github.event.pull_request.number || github.event.issue.number }} cancel-in-progress: false permissions: + actions: write statuses: write pull-requests: read issues: read @@ -1180,6 +1181,7 @@ jobs: - name: Verify engine review pass uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: + retries: 3 script: | const owner = context.repo.owner; const repo = context.repo.repo; @@ -1210,7 +1212,9 @@ jobs: }); pr = prResponse.data; } catch (err) { - failRun(`Could not load pull request #${context.payload.issue.number}: ${err.message}`); + // Without the head SHA there is nowhere to post a status, so an earlier + // success stands. Fail even though this run belongs to main. + core.setFailed(`Could not load pull request #${context.payload.issue.number}: ${err.message}`); return; } } @@ -1249,7 +1253,7 @@ jobs: core.info(`Commit status already posted. Skipping duplicate (${state}: ${description})`); return; } - statusPosted = true; + statusPosted = state !== 'pending'; try { await github.rest.repos.createCommitStatus({ owner, @@ -1263,10 +1267,75 @@ jobs: core.info(`Created commit status '${state}' on ${headSha}: ${description}`); } catch (err) { core.warning(`Failed to create commit status on ${headSha}: ${err.message}`); + // A comment run has no check run on the PR head to carry the verdict, so + // make the lost write visible even though the run belongs to main. + if (context.eventName === 'issue_comment') { + core.setFailed(`Could not post Engine Review Pass on ${headSha}: ${err.message}`); + } + } + } + + // A failed or cancelled run of this job stays on the head SHA and blocks the + // PR even after a later run passes. Rerun the oldest one: it re-reads the + // reviews and, if it passes, reruns the next. One at a time, because a queued + // run is cancelled by the next one in the concurrency group. Best-effort: if + // this fails, the old result stands and the PR stays blocked until a rerun. + async function rerunStaleEvaluation() { + const stale = (conclusion) => ['failure', 'cancelled', 'timed_out'].includes(conclusion); + try { + // A rerun joins this job's concurrency group, where it would replace a + // queued evaluation of a newer head. + const { data: current } = await github.rest.pulls.get({ owner, repo, pull_number: pr.number }); + if (current.head?.sha !== headSha) { + core.info(`Not rerunning stale evaluations: PR head moved from ${headSha}`); + return; + } + + const runs = await github.paginate(github.rest.actions.listWorkflowRunsForRepo, { + owner, + repo, + head_sha: headSha, + per_page: 100, + }); + const candidates = runs + .filter((r) => r.name === context.workflow && r.id !== context.runId && r.status === 'completed' && stale(r.conclusion)) + // Another PR at this SHA may not have passed, and its run shares this status. + .filter((r) => r.pull_requests?.some((p) => p.number === pr.number)) + .sort((a, b) => a.id - b.id); + for (const run of candidates) { + try { + const jobs = await github.paginate(github.rest.actions.listJobsForWorkflowRun, { + owner, + repo, + run_id: run.id, + per_page: 100, + }); + // Runs that are red for another job's sake would rerun forever. + const staleJob = jobs.find((j) => j.name === 'Engine Review Pass' && stale(j.conclusion)); + if (staleJob) { + await github.rest.actions.reRunJobForWorkflowRun({ owner, repo, job_id: staleJob.id }); + core.info(`Reran stale evaluation in run ${run.id}`); + return; + } + } catch (err) { + // An expired run must not stop newer ones from being repaired. + core.warning(`Could not rerun run ${run.id}: ${err.message}`); + } + } + } catch (err) { + core.warning(`Could not rerun stale evaluations on ${headSha}: ${err.message}`); } } try { + // A comment run has no job under the required name on the PR head, so an + // earlier success would outlive a run that dies or cannot post its verdict. + // Replacing it first leaves the PR blocked instead. This runs for every + // comment, because a queued run with a marker can be replaced by one without. + if (context.eventName === 'issue_comment') { + await setCommitStatus('pending', 'Evaluating reviews'); + } + const headRef = pr.head?.ref || ''; const slashIndex = headRef.indexOf('/'); const rawPrefix = slashIndex > 0 ? headRef.slice(0, slashIndex).trim() : ''; @@ -1392,9 +1461,29 @@ jobs: return m.action === 'PASS' && m.engine.toLowerCase() !== authorEngineLower; }); - if (blockedMarkers.length > 0) { - const blockedEngines = blockedMarkers.map((m) => m.engine).join(', '); - const msg = `Blocked by ${blockedEngines}`; + // Native reviews follow GitHub's rule: a member's latest decision stands, and + // a later comment does not clear it. These are read from the raw list because + // eligibleReviews drops reviews with an empty body. + const latestDecisions = new Map(); + const decisions = reviews + .filter((r) => ['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(r.state) && r.user?.login) + .sort((a, b) => new Date(a.submitted_at).getTime() - new Date(b.submitted_at).getTime() || a.id - b.id); + for (const r of decisions) { + latestDecisions.set(r.user.login, r); + } + const changesRequestedBy = Array.from(latestDecisions.values()) + .filter((r) => r.state === 'CHANGES_REQUESTED' && ALLOWED_ASSOCIATIONS.has((r.author_association || '').toUpperCase())) + .map((r) => r.user.login); + + if (blockedMarkers.length > 0 || changesRequestedBy.length > 0) { + const reasons = []; + if (blockedMarkers.length > 0) { + reasons.push(`Blocked by ${blockedMarkers.map((m) => m.engine).join(', ')}`); + } + if (changesRequestedBy.length > 0) { + reasons.push(`Changes requested by ${changesRequestedBy.join(', ')}`); + } + const msg = reasons.join('; '); await setCommitStatus('failure', msg); failRun(`Engine Review Pass blocked: ${msg}`); return; @@ -1411,6 +1500,7 @@ jobs: const successMsg = `Passed by ${passEngines}`; await setCommitStatus('success', successMsg); core.info(`Engine Review Pass succeeded: ${successMsg}`); + await rerunStaleEvaluation(); } catch (err) { core.error(`Unexpected error: ${err.message}`); await setCommitStatus('failure', `Error: ${err.message}`.slice(0, 140));