diff --git a/.github/workflows/pr_agent_review.yml b/.github/workflows/pr_agent_review.yml index f2b8cef..f163167 100644 --- a/.github/workflows/pr_agent_review.yml +++ b/.github/workflows/pr_agent_review.yml @@ -1,14 +1,16 @@ -# Verifies a Claude review exists for the pull request's CURRENT head commit. +# Verifies a Claude review exists for the pull request. # # The approval rule is "one human approval plus a passing agent review on every pull request". # The human half is enforced by the branch ruleset. This is the other half. # # It does not read the review or judge what it found -- the judgement is in the review itself, # posted by the /el-review skill in the approver's own session. This answers only "did one -# happen, for this commit". +# happen on this pull request". # -# Binding it to the head commit is the whole point. A review of an earlier commit must not -# satisfy the check for code pushed afterwards, or you review once, push anything, and merge. +# One review per pull request is enough. The marker names the commit that was reviewed; the check +# does not require that to be the head, so a push after the review does not turn it red. The +# status names the reviewed commit and how many commits came after it instead, so an approver can +# see how far the code has moved past the review and re-run /el-review if that matters to them. # # --------------------------------------------------------------------------- # CALLER REQUIREMENTS -- this needs more than the other reusable workflows @@ -84,7 +86,7 @@ # THE JOB'S COLOUR IS NOT THE CHECK'S ANSWER # # This job goes green whenever it managed to post a status, whatever that status says. The -# finding is in the status: red when no review exists for the head commit, green when one does. +# finding is in the status: red when the pull request has no trusted review, green when it has one. # # That split matters because a check run is never retracted. When the review lands, the # pull_request_review run adds a SECOND check run of the same name -- so if the first run had @@ -166,7 +168,7 @@ jobs: with: node-version: ${{ inputs.node-version }} - - name: Check for an agent review on the head commit + - name: Check for an agent review on the pull request env: GITHUB_TOKEN: ${{ github.token }} # pull_request and pull_request_review carry the pull request under different keys; diff --git a/readme.md b/readme.md index bc2f5be..60bf1a4 100644 --- a/readme.md +++ b/readme.md @@ -23,7 +23,7 @@ Every repository calls these rather than copying the steps, so a fix lands once. | `azure-storage-sync.yml` | Sync blob storage between rings | | **`pr_compliance.yml`** | **Deterministic pull request checks — the facts** | | **`pr_code_review.yml`** | **The EasyLife 365 review standard, run on the pull request — the judgement** | -| **`pr_agent_review.yml`** | **Verifies a Claude review exists for the pull request's current head commit — the gate** | +| **`pr_agent_review.yml`** | **Verifies a Claude review exists for the pull request — the gate** | ### Versioning @@ -81,11 +81,14 @@ approval, and must not be a required check — an LLM finding is an opinion wort flaky merge gate teaches people to ignore the checks that are not flaky. **`pr_agent_review.yml`** is the third layer and behaves unlike the other two. It does not read -the diff and forms no opinion. It answers one question: *did a Claude review happen for this -exact head commit?* The approver of record runs `/el-review` in their own session; that skill posts +the diff and forms no opinion. It answers one question: *did a Claude review happen on +this pull request?* The approver of record runs `/el-review` in their own session; that skill posts the findings and records a marker naming the commit it reviewed, and this check looks for the -marker. Binding it to the head commit is the whole point — a review of an earlier commit must -not satisfy the check for code pushed afterwards, or you review once, push anything, and merge. +marker. One review per pull request is enough: a push after the review does not turn the check red. +When the reviewed commit is not the head, the status says so (`Reviewed by @user at fd0066f, 2 +commits since`), so an approver can see how far the code has moved past the review and re-run +`/el-review` if that matters to them. A small first commit reviewed and a large change pushed +afterwards stays green; that trade-off is accepted (see `.github`#135). Because it is a fact rather than a judgement, it is safe to require. It enforces the agent half of the approval rule: @@ -200,6 +203,8 @@ commit status. than `!= 'pull_request_review'`, so a trigger added later has to opt in deliberately instead of silently inheriting a payload `pr-compliance.mjs` cannot parse. +**Dismissing a review takes it back, but only at the next run.** `agent-review` ignores dismissed reviews. Because a later push no longer retires a review, a caller that wants a dismissal to turn the status red straight away adds `dismissed` to its `pull_request_review` types; without it the status is recomputed on the next push or review. + **The concurrency group is keyed by event.** The two event types do not run the same job set — a review run skips compliance — so sharing a group with `cancel-in-progress` lets a review submission cancel an in-flight push run's compliance job and never replace it, leaving the @@ -234,4 +239,4 @@ Prerequisites, the rules the review applies, and the rollout order are documente |---|---| | `scripts/check-wcag.mjs` | Accessibility rules for the WCAG check | | `scripts/pr-compliance.mjs` | The deterministic pull request checks run by `pr_compliance.yml` | -| `scripts/pr-agent-review.mjs` | The head-commit agent-review check run by `pr_agent_review.yml` | \ No newline at end of file +| `scripts/pr-agent-review.mjs` | The agent-review check run by `pr_agent_review.yml` | \ No newline at end of file diff --git a/scripts/pr-agent-review.mjs b/scripts/pr-agent-review.mjs index 74b21b0..95ec1f7 100644 --- a/scripts/pr-agent-review.mjs +++ b/scripts/pr-agent-review.mjs @@ -1,7 +1,7 @@ #!/usr/bin/env node // EasyLife 365 agent-review check. // -// Verifies that a Claude review exists FOR THE CURRENT HEAD COMMIT, posted as a REVIEW by +// Verifies that a Claude review exists FOR THIS PULL REQUEST, posted as a REVIEW by // someone with write standing. It does not read the // review, judge it, or care what it found -- the judgement lives in the review itself, posted // by the /el-review skill in the approver's own session. This only answers "did one happen". @@ -10,17 +10,23 @@ // // One human approval plus a passing agent review on every pull request. // -// Why the head commit matters: a review of an earlier commit must not satisfy the check for -// code pushed afterwards. Without that binding you can review once, push anything, and merge -// -- which is a formality wearing a gate's clothes. +// One review per pull request is enough. The marker names the commit that was reviewed, but the +// check does not require it to be the head: a push after the review does not turn the status red +// and does not force another review. What it does instead is say so -- the status names the +// reviewed commit and how many commits came after it, so an approver can see how far the code has +// moved past the review and re-run /el-review if that matters to them. +// +// The weak spot is accepted on purpose (EasyLife365/.github#135): review a small first commit, push +// a large change, and the status stays green. The transparency note makes that visible; nothing +// blocks it. // // WHAT THIS SCRIPT'S EXIT CODE MEANS // // It reports whether the CHECK RAN, not what the check FOUND. Those are different questions and // conflating them is a bug this script used to have. // -// The finding lives in the commit status: red when no review exists for the head commit, green -// when one does. The status is the required context, so the gate is unaffected by the exit code. +// The finding lives in the commit status: red when the pull request has no trusted review marker, +// green when it has one. The status is the required context, so the gate is unaffected by the exit code. // // Exiting non-zero on "no review yet" made the job itself red -- and a check run is never // retracted, so the later review-triggered run added a SECOND check run of the same name while @@ -47,10 +53,10 @@ // instead: github.event.merge_group.head_ref looks like // "refs/heads/gh-readonly-queue/main/pr-123-". PR_NUMBER_IN_REF below extracts it. // -// Once resolved, the review lookup itself is unchanged: it still checks for a marker against -// the PR's real head commit (pull.head.sha, fetched by number), which is what /el-review -// actually reviewed. What changes is where the ANSWER gets posted: a merge queue evaluates -// required checks against the merge group's own temporary commit, not the PR's head commit -- +// Once resolved, the review lookup itself is unchanged: it reads the markers on the PR's own +// reviews and compares the reviewed commit with the PR's real head commit (pull.head.sha, fetched +// by number) only to describe how far the head has moved. What changes is where the ANSWER gets +// posted: a merge queue evaluates required checks against the merge group's own temporary commit, not the PR's head commit -- // GitHub's own docs describe GITHUB_SHA as "SHA of the merge group" for this event -- so // postStatus targets GITHUB_SHA instead of the PR's head commit specifically when this run was // triggered by merge_group. For every other trigger this is a no-op: GITHUB_EVENT_NAME is not @@ -114,9 +120,8 @@ async function api(path) { /** * Follows pagination. Both endpoints return oldest first, so on a pull request with more than - * one page the newest review -- the one most likely to match the head commit -- is on the LAST - * page. Reading only the first would report "no review" on exactly the busy pull requests - * where one is most likely to exist. + * one page the newest review is on the LAST page. Reading only the first would report "no review" + * on exactly the busy pull requests where one is most likely to exist. */ async function apiAll(path) { const items = []; @@ -208,8 +213,8 @@ const pull = await api(`/repos/${owner}/${repo}/pulls/${cfg.prNumber}`); const headSha = pull.head.sha; const author = (pull.user?.login || '').toLowerCase(); -// Where the status is POSTED, as distinct from headSha (what was actually reviewed, used for -// every marker-matching decision below). Identical to headSha outside a merge queue -- see the +// Where the status is POSTED, as distinct from headSha (the pull request's real head, which the +// reviewed commit is compared with below). Identical to headSha outside a merge queue -- see the // merge-queue comment near the top of this file. const statusSha = mergeGroupSha || headSha; @@ -264,39 +269,74 @@ const untrusted = []; const all = []; for (const review of reviews) { + // A dismissed review is GitHub's way of saying "this no longer counts". The reviews list still + // returns it, and now that a push no longer retires a review, dismissing it is the one way left + // to take it back. + if (review.state === 'DISMISSED') continue; const entry = { body: review.body, at: review.submitted_at, by: review.user?.login, association: review.author_association, + // The commit GitHub recorded the review against. It is set by the server when the review is + // submitted, so it is used instead of the marker's own `sha` for everything below: the + // marker text is free-form, this is not, and it can never name a commit pushed after the + // review. + sha: review.commit_id, }; if (TRUSTED_ASSOCIATIONS.has(review.author_association)) all.push(entry); else if (MARKER.test(review.body || '')) untrusted.push(entry); MARKER.lastIndex = 0; } -const matched = []; -const stale = []; - +// Every trusted marker counts, whatever commit its review was recorded against. The one for the +// head commit wins when there is one, so a re-review of the head reports its own model and finding +// count; otherwise the most recent marker is the one described. +// +// A marker still has to carry a `sha` field to be well formed, but the value is not used: the +// review's own commit_id replaces it (the review's fields come last in the spread). +const markers = []; for (const item of all) { for (const marker of markersIn(item.body)) { if (!marker.sha) continue; - (marker.sha === headSha ? matched : stale).push({ ...marker, ...item }); + markers.push({ ...marker, ...item }); } } -if (matched.length > 0) { - // Most recent wins. A re-review of the same commit should report its own model and finding - // count, not the first attempt's. - matched.sort((a, b) => String(b.at || '').localeCompare(String(a.at || ''))); - const best = matched[0]; +if (markers.length > 0) { + markers.sort((a, b) => String(b.at || '').localeCompare(String(a.at || ''))); + const best = markers.find((marker) => marker.sha === headSha) || markers[0]; const detail = [best.model && `model ${best.model}`, best.effort && `effort ${best.effort}`, best.findings !== undefined && `${best.findings} finding(s)`].filter(Boolean).join(', '); // Naming the reviewer is the point of allowing self-review: an approver can see who ran it. const who = best.by ? ` by @${best.by}` : ''; - await postStatus(statusSha, 'success', detail ? `Reviewed${who} (${detail})` : `Reviewed${who}`); - console.log(`Agent review found for ${headSha}${who}${detail ? ` -- ${detail}` : ''}.`); + + // How far the head has moved past the reviewed commit. Never a reason to fail: a comparison + // that cannot be made (the commit is gone after a force-push, or the API is unavailable) is + // reported as unknown rather than as a failure. + let since = ''; + if (best.sha !== headSha) { + let count = null; + try { + // Only a linear history gives a meaningful count. A reviewed commit that is no longer an + // ancestor of the head (a force-push or rebase) compares as diverged or behind, and its + // ahead_by would understate the change, so that case is reported as unknown. + if (!/^[0-9a-f]{40}$/i.test(String(best.sha))) throw new Error('the review has no usable commit id'); + const comparison = await api(`/repos/${owner}/${repo}/compare/${best.sha}...${headSha}`); + if (comparison.status === 'ahead' && Number.isInteger(comparison.ahead_by)) count = comparison.ahead_by; + } catch (error) { + console.warn(`::warning::Could not compare ${best.sha} with ${headSha}: ${error.message}`); + } + const noun = count === 1 ? 'commit' : 'commits'; + since = count === null + ? ` at ${String(best.sha).slice(0, 7)}, commits since unknown` + : ` at ${String(best.sha).slice(0, 7)}, ${count} ${noun} since`; + } + + const summary = `Reviewed${who}${since}`; + await postStatus(statusSha, 'success', detail ? `${summary} (${detail})` : summary); + console.log(`Agent review found${who} for ${best.sha}${since ? `; head is ${headSha}${since}` : ''}${detail ? ` -- ${detail}` : ''}.`); if (best.by && best.by.toLowerCase() === author) { console.log('Note: the marker was posted by the pull request author. That is allowed, and the'); console.log('status says so, so an approver can weigh it or re-run /el-review themselves.'); @@ -304,9 +344,6 @@ if (matched.length > 0) { process.exit(0); } -// A review of an older commit is the interesting failure, and it gets its own message: the -// author did the right thing and then pushed, which reads very differently from never having -// reviewed at all. // A reviewer requested with NO review behind it at all is the ordering mistake the pull request // rules exist to prevent, and it reads very differently from simply not having got to it yet. // @@ -318,23 +355,15 @@ const reviewersRequested = (pull.requested_reviewers || []).length > 0 || (pull.requested_teams || []).length > 0; const nobodyHasReviewed = reviews.length === 0; -const description = stale.length > 0 - ? `Review is for an older commit - re-run /el-review on ${headSha.slice(0, 7)}` - : reviewersRequested && nobodyHasReviewed - ? `Reviewer requested before any review - run /el-review on ${headSha.slice(0, 7)}` - : `No agent review for ${headSha.slice(0, 7)} - run /el-review`; +const description = reviewersRequested && nobodyHasReviewed + ? 'Reviewer requested before any review - run /el-review' + : 'No agent review on this pull request - run /el-review'; await postStatus(statusSha, 'failure', description); // A warning, not an error: the job did what it was asked to do. The red lives in the commit // status, where it belongs and where the ruleset reads it. console.warn(`::warning::${description}`); -if (stale.length > 0) { - console.warn(`::warning::Found ${stale.length} review marker(s), none matching the head commit ${headSha}.`); - for (const item of stale) { - console.warn(` - ${item.sha} by ${item.by || 'unknown'} at ${item.at || 'unknown time'}`); - } -} if (untrusted.length > 0) { console.warn(`::warning::Ignored ${untrusted.length} marker(s) from an author without write access.`); for (const item of untrusted) { @@ -351,6 +380,7 @@ console.warn('writes the marker this check reads -- so it leaves this red howeve console.warn('findings were.'); console.warn(''); console.warn('/el-review posts the findings and records a marker naming the commit it reviewed.'); +console.warn('One review per pull request is enough; later pushes do not turn this red again.'); // Exit 0: the status was posted and it is accurate. The pull request is gated by that status, // not by this job's colour -- see the note at the top of this file.