fix(review): dismiss a standing bot approval when the review withholds the verdict

[tend-agent]

Reported in #1135, with evidence from two adopter PRs that merged reading APPROVED over blocking bot findings. The review skill stated the "dismiss a stale approval" rule only in the force-push branch; the ordinary-push branch beside it had no equivalent, and GitHub never lets a later COMMENTED supersede an APPROVED, so the outward failure is identical. This moves the rule to the posting site in step 6, where it covers both push shapes, and gives bot-review-state.sh a field that names the approval currently deciding the PR.

The reported fix keyed detection on .last_substantive — that catches only the first findings review after an approval. In the reported case three findings reviews followed, and from the second onward last_substantive is the previous COMMENTED, not the approval. The new standing_approval_id is the newest bot APPROVED that still sets the review decision, so it keeps firing until the approval is actually gone.

Details

Skill — skills/review/SKILL.md: step 6 gains "A findings review never supersedes a standing approval — dismiss it", with the recipe. The dismissal runs after the review POST lands, so a failed post can't leave the PR carrying neither a verdict nor findings. Step 1's force-push paragraph keeps the fact specific to it (the approval is re-anchored onto the rewritten head) and defers the mechanics to step 6 rather than restating them, so the net skill text is slightly shorter.

Script — scripts/bot-review-state.sh: new standing_approval_id. It is the last bot review among APPROVED/CHANGES_REQUESTED, emitted only when that one is APPROVED — so a dismissal (which rewrites the record's state to DISMISSED) or a later CHANGES_REQUESTED clears it, and a re-run over the same PR dismisses nothing. Distinct from stale_approval_id, which stays keyed on a rewrite for weekly's rebase paths.

Tests — five new cases in generator/tests/test_bot_review_state.py, written failing first. The central one replays the reported shape: APPROVED at an older commit, ordinary pushes, two findings reviews, no force push — standing_approval_id is the approval, stale_approval_id is "". Others cover the dismissed, superseded-by-a-later-approval, and CHANGES_REQUESTED cases, plus a contract test pinning that the skill reads the field and that the force-push branch no longer carries its own copy of the rule.

Verified: uv run pytest (912 passed) and pre-commit over the changed files.

---

Closes #1135 — automated triage

Review round (c2aa48d). Two things the step 1 rewrite exposed. Step 7's CI-failure dismissal still read $REVIEW_ID, and the sentence this PR removed was the only site naming it — followed literally the path collapsed to reviews//dismissals and the approval stayed standing over a red check, the same outward shape this PR closes. That recipe now reads standing_approval_id, so both dismissal sites use one field and one mechanism. Separately the step 6 rule fired only on findings; the withheld-merge-readiness COMMENT beside it is reachable with an approval already standing and leaves the same wrong signal, so the trigger became any COMMENT rather than an approval. A new contract test pins that no $REVIEW_ID remains and that both dismissal recipes read the same field.

Review round (8c14dea). "Any COMMENT" widened past the cases it was meant to cover. Step 1's unanswered-question exception posts a COMMENT at a head the approval already covers, so the rule dismissed the approval of unchanged code — and nothing restores it: last_substantive then anchors that head, the next run hits the already-reviewed shortcut, and the PR sits with no verdict because the bot answered a question. The trigger is now a COMMENT that withholds the verdict, which keeps findings and the withheld-merge-readiness blocker and drops that path; the carve-out is stated in the paragraph rather than left to inference. Pinned by test_review_skill_spares_the_approval_when_the_comment_withholds_nothing.

[tend-agent]

One thing the step 1 rewrite takes with it.

$REVIEW_ID in step 7's dismissal recipe now has no definition anywhere in the skill. The sentence this PR removes was its only naming site — it ended "jq -r '.last_substantive.state, .last_substantive.id' <<<"$STATE" gives the state and the $REVIEW_ID for step 7's reviews/$REVIEW_ID/dismissals call" — and the recipe it named is untouched, under ### 7. Monitor CI, in the "A check failed and it's related to the PR" bullet:

gh api "repos/$REPO/pulls/<number>/reviews/$REVIEW_ID/dismissals" \
  -X PUT -f message="CI failed — <reason>"

Followed literally the path collapses to reviews//dismissals and the dismissal fails, leaving the approval this session posted standing over a red check — the same outward shape the PR closes. A session might recover by reading the id off its own POST response, but nothing in the skill says to.

standing_approval_id is the value that recipe wants: after step 6 approved, the approval this session posted is the one setting the review decision, so step 7 can read the same field the new step 6 block does rather than a variable nobody sets. That also leaves one dismissal mechanism in the skill instead of two.

Separately, the new rule fires "whenever this round posts findings rather than an approval". The withheld-merge-readiness path just above it posts a COMMENT that names a blocker rather than findings, and it is reachable with an approval already standing (bot approves, author then says hold off, next push draws the blocker COMMENT). Widening the trigger to any COMMENT would cover it — the bolded lead can stay as-is, so the contract test's assertion still holds.

Everything else reads right to me: the standing_approval_id filter over APPROVED/CHANGES_REQUESTED gives "" for the empty, dismissed, and superseded cases (last yields null, select drops it, // supplies the default), stale_approval_id stays keyed on a rewrite for weekly's paths, and uv run pytest generator/tests/test_bot_review_state.py is 29 passed here.

[tend-agent]

Both taken, in c2aa48d.

$REVIEW_ID — step 7's dismissal now reads standing_approval_id off the same script call step 6 uses. After step 6 approved, that approval is the one setting the review decision, so it is exactly the id the recipe wanted, and it reads "" once dismissed, which subsumes the "skip if already dismissed" sentence. Both dismissal sites now use one field and one mechanism; no $REVIEW_ID remains in the skill.

Trigger width — the rule now fires whenever the round posts a COMMENT rather than an approval, naming the withheld-merge-readiness path as the second case. The bolded lead is unchanged, so the existing contract assertion still holds.

Added test_review_skill_defines_every_id_its_dismissal_recipes_use, which pins both: no $REVIEW_ID in the skill, and exactly two reviews/$STANDING/dismissals recipes.

uv run pytest is 913 passed; pre-commit clean over the two changed files.

[tend-agent]

The $REVIEW_ID fix reads right — one field, one mechanism, and the contract test pins both sites. One thing the widening takes with it.

Making the trigger "any COMMENT" rather than "findings" catches a COMMENT that withholds nothing. Step 1 carves out exactly one: "An unanswered conversation question directed at the bot (check below) is the exception: proceed so the review can answer it", with step 6's dedup paragraph routing the answer into the review body rather than gh pr comment. So on a head the bot already approved — no push since — a run that takes that exception posts a COMMENT whose only content is the answer, and step 6 now dismisses the approval of unchanged code. The message it posts is wrong there too: nothing was "superseded by the review on a later commit", it is the same commit.

Nothing restores it. After that COMMENT, last_substantive anchors the current head, so the next run hits the already-reviewed shortcut and posts nothing until a new push — the PR sits with no verdict because the bot answered a question.

The paragraph's own second half already names the right predicate ("the review names a blocker"), so scoping the rule to a COMMENT that withholds the verdict keeps both cases you want and drops this one. Suggestion inline.