fix(running-in-ci): dismiss a standing approval invalidated by another PR merging - #1139
fix(running-in-ci): dismiss a standing approval invalidated by another PR merging#1139tend-agent wants to merge 2 commits into
Conversation
…r PR merging Closes #1138
tend-agent
left a comment
There was a problem hiding this comment.
Self-review — the gap is real and the placement is right: nightly (Step 0) and resolve-conflicts (line 10) both load running-in-ci, so one section reaches every session that can reach the conclusion, and standing_approval_id reads "" after a dismissal (the jq keeps only APPROVED/CHANGES_REQUESTED, and a dismissal rewrites the state to DISMISSED), so a re-run is a genuine no-op. The test's four assertions are all absent from the base file, so it fails on main as claimed.
Two things in the prose, both inline.
One observation that isn't for this PR: this is the third copy of the same standing_approval_id → dismissals recipe (review step 6, review step 7, now here), and review loads running-in-ci, so those two could become pointers to this section plus their own trigger conditions. Worth folding into whichever PR next touches review, rather than growing this one.
…the review post 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.
Every dismissal site tend has is keyed on an event on the approved PR — a review round (
reviewstep 6), a rewrite (weeklystep 2), a red check (reviewstep 7) — so an approval invalidated from outside the PR, most often by a different PR merging and superseding it, reaches none of them and stands until a human clears it. This adds one generic rule inrunning-in-ci, the skill every session that can reach that conclusion already loads, keyed on the conclusion rather than on posting.The reported case (#1138) is a dependency PR that a later merge turned into a downgrade: two
tend-nightlysessions stated the supersession in their own summaries and left the botAPPROVEDstanding, correctly declining to post a second deferral comment — which is exactly why a rule keyed on a posting site never fired. Merging was blocked there only because the branch happened to be conflicting; a supersession that doesn't touch the same manifest lines leaves the PR mergeable under a stale bot approval.Details
Placement. One site rather than a copy in
nightlyandresolve-conflicts: both loadrunning-in-ci(nightly Step 0, resolve-conflicts line 10), as doweekly,review,triageand the mention paths, so the rule is in context wherever the conclusion is reachable. It also keepsbot-review-state.shas the single mechanism — the recipe reads the existingstanding_approval_idfield from #1136, which reads""once cleared, so a re-run dismisses nothing.Scope guard. The rule states the test as the merge, not tidiness, so it can't be read as "dismiss whenever you have a finding" — that would withdraw verdicts the code still earns, which is the failure #1136's own scoping sentence guards against.
Test.
test_a_dismissal_path_exists_for_an_invalidation_that_is_not_an_eventingenerator/tests/test_bot_review_state.py, alongside the existing dismissal contract tests. It fails onmain(the skill contains nostanding_approval_idreference) and passes with the change. Fulluv run pytest(916 passed) andpre-commit run --all-filesare green.Closes #1138 — automated triage