A reviewer who withdraws approval in a comment leaves the PR reporting strict-merge-ready

Context

Captured from a live incident on PR #17606 rather than predicted. Friction → gold, unprompted.

@neo-gpt-emmy approved that PR. I then self-raised that it violated a rule in the file it edits; she issued an owner-side hold stating *"My prior approval is therefore not a current merge authorization. Do not merge at 0163f092bc."*

GitHub still reports mergeStateStatus: CLEAN, reviewDecision: APPROVED, all checks green. I had already broadcast that PR as merge-ready. Had @tobiu acted on that broadcast plus a green gh pr checks, the hold would have changed nothing — it lives in prose that no readiness surface reads.

Live latest-open sweep 2026-08-23T06:21Z, plus state:all searches for merge hold invisible reviewDecision comment and merge readiness projection hold state machine-visible: zero hits. Nothing owns this.

The Problem

ai/scripts/lifecycle/validateMergeReady.mjs is well-built and already solves the adjacent case. Its own docblock:

reviewDecision=APPROVED + green checks are necessary but NOT sufficient: GitHub flattens reviewDecision to APPROVED as soon as it has enough formal review state, even while an explicitly-requested reviewer is still outstanding (reviewRequests non-empty).

So it already knows that APPROVED overstates the contract, and it already blocks on one instance of that (rule 5, the reviewer-contract gate). What it does not model is the reverse-in-time case:

• Rule 2 is reviewDecision === 'APPROVED' — a point-in-time flag with no notion of an approval being superseded.
• A reviewer can approve at T0 and withdraw at T1 via a comment. reviewDecision still reads APPROVED at T1.
• Nothing in the fetched surface distinguishes "approved and still standing" from "approved, then explicitly held".

The convention already exists and is already being used — @neo-gpt-emmy's hold is headed ## `[MERGE_HOLD]`, and the earlier one ## `[RE_REVIEW_HOLD]`. The tokens are in the corpus. Nothing reads them.

The Architectural Reality

This is not a missing guard; it is two existing mechanisms that never meet. A structured hold token is emitted by reviewers, and a fail-closed readiness validator runs on the other side of the same PR, and neither knows about the other.

The failure is silent in the worst direction. Every other gap in this validator fails closed — an unfetched field blocks readiness, UNKNOWN mergeability blocks readiness, an outstanding reviewer blocks readiness. This one fails open: the PR reports green while an owner has explicitly said stop.

It is also the mirror of the case the validator was built for. There, GitHub was too permissive because a pending obligation was invisible. Here it is too permissive because a retracted approval is invisible. Same root: reviewDecision is a flattened snapshot, not a contract state.

Title prefixes and body sections (what I did on #17606) are mitigations for humans, not for the readiness check — and a human who trusts the automated surface is exactly the reader they miss.

The Fix

Extend the existing validator by one rule rather than build machinery. The shape that fits its fail-closed philosophy:

• fetch the PR's review + comment timeline;
• a hold is a comment from a reviewer bearing a recognised token ([MERGE_HOLD], [RE_REVIEW_HOLD]) newer than that reviewer's latest submitted review;
• an unresolved hold blocks strictMergeReady, and the returned reason names the holder and the comment, so the caller can print a cause rather than a bare false;
• a hold is cleared by a newer submitted review from the same reviewer — not by a later comment, and never by another peer.

Unfetched timeline data must fail closed like every other field.

Contract Ledger

Amended 2026-08-24 at review. @neo-gpt-emmy ran the implementation at exact head and found the ledger under-specified in three places, each of which the code had then faithfully implemented. Row 1 said "newer than that reviewer's latest review" and never required an approval — so a commenter with no review at all satisfied it, and any drive-by comment could block any PR. Row 3 claimed an exclusive clearing path a mutable source cannot enforce. Neither the truncation state nor the issuance/demonstration boundary was named at all. The rows below are the corrected authority; the loose original is what shipped, which is the argument for correcting the ledger rather than only the code.

Acceptance Criteria

• A PR whose owner posted a recognised hold token after their approving review does not report strictMergeReady, and the reason names the holding reviewer.

• The hold clears only on a newer submitted review from that same reviewer — proven by an arm where a later comment fails to clear it, and one where another peer's review fails to clear it.

• Unfetched timeline data fails closed, consistent with state / checksGreen / mergeStateStatus / reviewRequests. A fetched-but-truncated window is a distinct third state: held: null, blocking with its own reason rather than claiming a hold nobody saw.

• An unrecognised token is not treated as a hold — no prose scanning; a negative arm proves a comment merely containing the word "hold" does not block. A token inside a fenced or indented code block is a demonstration, not an issuance, and does not block.

• The recognised token set is documented where a reviewer issuing a hold will actually look.

• Only a prior approver may hold. A commenter with no approving review is not a holder, proven by arms for no-review-at-all and reviewed-but-never-approved; an approval submitted after the token means the approval spoke last.

• A control proves the rule can fire, pinned to the observed timeline rather than a hypothetical one. PR #17606 ran the full sequence live:

  t	event	reviewDecision	strictMergeReady today	should be
  T0	@neo-gpt-emmy submits APPROVED	APPROVED	true	true
  T1	she posts [MERGE_HOLD]: "my prior approval is not a current merge authorization"	still APPROVED	true ← the defect	false
  T2	she dismisses the approval manually	(empty)	false	false

  The fixture must block at T1. T2 is the correct manual action and is what happened here — which is exactly why the rule is needed rather than why it is not: remembering to dismiss is currently load-bearing, and nothing prompts it. In this incident the author (me) had already broadcast merge-ready during the T1 window.

Out of Scope

• Human merge authority. Unchanged and human-only; this validates the claim of readiness, exactly as the module already scopes itself.
• Auto-resolving holds, or any workflow that closes a hold on the holder's behalf.
• Changing GitHub review semantics — we cannot make a comment flip reviewDecision; this reads the timeline instead.
• The #17606 hold itself, which is a live governance question with its own gates.

Avoided Traps

• Scanning prose for "hold". Reviewers write that word constantly, including inside sentences declining to hold. Only an exact recognised token counts, and a negative arm proves it.
• Letting any later activity clear a hold. A hold cleared by a comment — or by a different peer — would be worse than no hold, because it would read as deliberately dispositioned.
• Treating this as a #17606 problem. The PR is the specimen; the gap is general, and every hold issued this way has been equally invisible.
• Reaching for a title prefix as the fix. That is what I did as a stopgap, and it addresses the human reader while leaving the automated surface — the one a merge-readiness claim is built on — still wrong.

Related

PR #17606 (the live specimen) · #17605 · ai/scripts/lifecycle/validateMergeReady.mjs · #15592

Retrieval Hint: merge hold invisible reviewDecision APPROVED comment withdrawn approval validateMergeReady strictMergeReady hold token timeline fail closed

Origin Session ID: 3764a1fc-e835-4923-8c65-c092d3d90069