From 3c35bc9f7fff5225e5c2fd51ddf7a8e84b8dfb01 Mon Sep 17 00:00:00 2001 From: Patrick Lamber Date: Wed, 30 Sep 2026 12:27:20 +0200 Subject: [PATCH 1/2] Keep agent-review green after later pushes once a trusted review exists. Fixes #135 The check accepted only a marker for the current head commit, so every push after a review turned it red and forced another review to clear the status. One review per pull request is enough: any marker from an OWNER, MEMBER or COLLABORATOR review now counts, and the status names the reviewed commit and how many commits came after it when that is not the head. The marker for the head commit still wins when there is one. Without a trusted marker the check is red as before. Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/pr_agent_review.yml | 14 +++-- readme.md | 13 +++-- scripts/pr-agent-review.mjs | 83 ++++++++++++++++----------- 3 files changed, 65 insertions(+), 45 deletions(-) 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..d080fc4 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: diff --git a/scripts/pr-agent-review.mjs b/scripts/pr-agent-review.mjs index 74b21b0..dc1063e 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,9 +53,9 @@ // 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 +// Once resolved, the review lookup itself is unchanged: it reads the markers on the PR's own +// reviews and compares them with the PR's real head commit (pull.head.sha, fetched by number) +// only to describe how far it 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 @@ -275,28 +281,47 @@ for (const review of reviews) { MARKER.lastIndex = 0; } -const matched = []; -const stale = []; - +// Every trusted marker counts, whatever commit it names. The one that matches 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. +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 { + const comparison = await api(`/repos/${owner}/${repo}/compare/${best.sha}...${headSha}`); + if (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 ${best.sha.slice(0, 7)}, commits since unknown` + : ` at ${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 +329,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 +340,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 +365,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. From 0410b76e27c27336450dc4a0d4a7569c83ceb9b9 Mon Sep 17 00:00:00 2001 From: Patrick Lamber Date: Wed, 30 Sep 2026 12:34:14 +0200 Subject: [PATCH 2/2] Use the review's own commit id and ignore dismissed reviews in agent-review. Associates #135 Review findings on the first commit of this pull request: - The commit a review covers now comes from the review's server-set commit_id, not from marker text, so free-form text never reaches the compare API path and the "commits since" note cannot be steered by the reviewer. - A reviewed commit that is no longer an ancestor of the head (force-push, rebase) is reported as "commits since unknown" instead of a misleading count. - Dismissed reviews do not count; a later push no longer retires a review, so dismissal is the way to take one back. The readme says how a caller makes dismissal re-evaluate at once. - Comments that still described the head binding are reworded; the readme file table no longer calls the check "head-commit". Co-Authored-By: Claude Sonnet 5.5 --- readme.md | 4 +++- scripts/pr-agent-review.mjs | 43 +++++++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/readme.md b/readme.md index d080fc4..60bf1a4 100644 --- a/readme.md +++ b/readme.md @@ -203,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 @@ -237,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 dc1063e..95ec1f7 100644 --- a/scripts/pr-agent-review.mjs +++ b/scripts/pr-agent-review.mjs @@ -54,9 +54,9 @@ // "refs/heads/gh-readonly-queue/main/pr-123-". PR_NUMBER_IN_REF below extracts it. // // Once resolved, the review lookup itself is unchanged: it reads the markers on the PR's own -// reviews and compares them with the PR's real head commit (pull.head.sha, fetched by number) -// only to describe how far it 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 -- +// 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 @@ -120,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 = []; @@ -214,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; @@ -270,20 +269,32 @@ 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; } -// Every trusted marker counts, whatever commit it names. The one that matches 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. +// 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)) { @@ -308,15 +319,19 @@ if (markers.length > 0) { 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 (Number.isInteger(comparison.ahead_by)) count = comparison.ahead_by; + 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 ${best.sha.slice(0, 7)}, commits since unknown` - : ` at ${best.sha.slice(0, 7)}, ${count} ${noun} since`; + ? ` 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}`;