diff --git a/generator/tests/test_bot_review_state.py b/generator/tests/test_bot_review_state.py index df94dab2..ce3c3b1c 100644 --- a/generator/tests/test_bot_review_state.py +++ b/generator/tests/test_bot_review_state.py @@ -25,6 +25,9 @@ / "bot-review-state.sh" ) REVIEW_SKILL = BOT_REVIEW_STATE.parent.parent / "skills" / "review" / "SKILL.md" +RUNNING_IN_CI_SKILL = ( + BOT_REVIEW_STATE.parent.parent / "skills" / "running-in-ci" / "SKILL.md" +) BOT = "tend-bot" HEAD = "head000" @@ -539,3 +542,20 @@ def test_review_skill_spares_the_approval_when_the_comment_withholds_nothing() - assert "posts a COMMENT that withholds the verdict" in skill assert "A COMMENT that withholds nothing does not qualify" in skill assert "whenever this round posts a COMMENT rather than an approval" not in skill + + +def test_a_dismissal_path_exists_for_an_invalidation_that_is_not_an_event() -> None: + """Every dismissal site in `review` and `weekly` is keyed on something that + happened *on* the approved PR — a review round, a rewrite, a red check. An + approval superseded by a *different* PR merging reaches none of them, so it + stands until a human clears it. The generic rule lives in `running-in-ci`, + which every skill that can reach that conclusion loads, and it fires on the + conclusion rather than on a post — the dedup rules routinely (and rightly) + suppress the comment that would otherwise carry it.""" + skill = RUNNING_IN_CI_SKILL.read_text() + + assert ".standing_approval_id" in skill + assert "reviews/$STANDING/dismissals" in skill + assert "-X PUT" in skill + # Not keyed on this session posting anything. + assert "whether or not this session posts" in skill diff --git a/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md b/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md index 4f5e8524..ccbc9de1 100644 --- a/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md +++ b/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md @@ -260,6 +260,24 @@ If it moved, `git fetch` and read the new commits before verifying: drop whateve When merging the default branch into a PR branch, **never use `--allow-unrelated-histories`**: if `git merge` fails because no merge base exists, the checkout is broken (usually shallow — re-checkout with `fetch-depth: 0`), and forcing the merge creates add/add conflicts in every file. If the merge fails because untracked files would be overwritten, stash them (`git stash --include-untracked`, merge, `git stash pop`) rather than deleting them. +## Dismiss a standing bot approval the moment you conclude the PR shouldn't merge + +A bot `APPROVED` keeps deciding the PR until a dismissal or a `CHANGES_REQUESTED` replaces it. A later COMMENT doesn't, and neither does the event that actually invalidated it: another PR merging and superseding this one, a dependency bump that turns this PR into a downgrade, an approach the thread has since rejected. None of those touch the approved PR, so no review round fires and the approval stands indefinitely — with nothing between it and a merge once the branch stops conflicting. Whichever session reaches the conclusion is the one that has to clear it, whether or not this session posts anything. + +Keying the dismissal to a post is what leaves it standing: **Recheck Before Posting** rightly suppresses a second deferral comment when one already stands, and a dismissal that rides on that comment is suppressed with it. Dismiss on the conclusion, then say so in the summary — where this session does post a review carrying that conclusion, dismiss after the post lands, so a failed post doesn't leave the PR with neither a verdict nor findings. + +```bash +REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner') +# "" once a dismissal or a later CHANGES_REQUESTED has cleared it, so a second +# session over the same PR dismisses nothing. +STANDING=$(${CLAUDE_PLUGIN_ROOT}/scripts/bot-review-state.sh | jq -r '.standing_approval_id') +# PUT, not POST — the dismiss endpoint requires it. +[ -z "$STANDING" ] || gh api "repos/$REPO/pulls//reviews/$STANDING/dismissals" \ + -X PUT -f message="" +``` + +The test is the merge, not tidiness: a finding you'd have left as a review comment is no reason to withdraw a verdict the code still earns, and neither is a branch that merely can't merge yet — a conflicting PR whose code the approval still covers keeps it. Dismiss when merging the PR, once it could merge, would be the wrong outcome. + ## CI Monitoring After pushing, what to do depends on whether a red result creates a follow-up.