Keep agent-review green after later pushes once a trusted review exists. Fixes #135

.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 @@
         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;

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` |
+| `scripts/pr-agent-review.mjs` | The agent-review check run by `pr_agent_review.yml` |

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-<sha>". 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,49 +269,81 @@ 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.');
   }
   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.