From 97c1691694b2c2965e2299b4e2c5e9d57890c7ae Mon Sep 17 00:00:00 2001 From: tend-agent <270458913+tend-agent@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:08:13 +0000 Subject: [PATCH 1/2] fix(running-in-ci): dismiss a standing approval invalidated by another PR merging Closes #1138 --- generator/tests/test_bot_review_state.py | 20 +++++++++++++++++++ .../skills/running-in-ci/SKILL.md | 18 +++++++++++++++++ 2 files changed, 38 insertions(+) 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..f665ac05 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. + +```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. Dismiss when merging the PR as it stands would be the wrong outcome. + ## CI Monitoring After pushing, what to do depends on whether a red result creates a follow-up. From ad7de8446e0273906f97dbf3ac8110ce5244c5c1 Mon Sep 17 00:00:00 2001 From: tend-agent <270458913+tend-agent@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:16:38 +0000 Subject: [PATCH 2/2] Keep an approval a conflicting branch still earns, and dismiss after the review post MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies both review suggestions: the merge test now says a branch that merely can't merge yet is not grounds for dismissal (resolve-conflicts only ever sees CONFLICTING PRs, so the unqualified wording read as already-satisfied), and the ordering rule from review step 6 — dismiss after the review POST lands — is stated here rather than left to the specific skill. --- plugins/tend-ci-runner/skills/running-in-ci/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f665ac05..ccbc9de1 100644 --- a/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md +++ b/plugins/tend-ci-runner/skills/running-in-ci/SKILL.md @@ -264,7 +264,7 @@ When merging the default branch into a PR branch, **never use `--allow-unrelated 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. +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') @@ -276,7 +276,7 @@ STANDING=$(${CLAUDE_PLUGIN_ROOT}/scripts/bot-review-state.sh | jq -r '. -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. Dismiss when merging the PR as it stands would be the wrong outcome. +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