Skip to content

Permission to file upstream: tend-review leaves an APPROVED standing when an ordinary push draws blocking findings #521

Description

@dormouse-bot

Asking before filing, per the repo policy on cross-repo issues.

What I'd file at max-sixty/tend: the review skill tells a session to dismiss a stale APPROVED after a force-push, but says nothing about the ordinary-push path immediately below it — so a PR that gets a normal push onto a bot approval, and then a fresh review with blocking findings, merges still reading APPROVED. It happened twice here in the last 24 hours, and both PRs merged that way.

The gap

plugins/tend-ci-runner/skills/review/SKILL.md:55 carries the rule, scoped to FORCE_PUSHED == true:

If that prior review was an APPROVED and the re-review lands on findings rather than an approval, dismiss it too — it is re-anchored onto the rewritten head, so posting a COMMENT alone leaves the PR reading as bot-approved.

The very next paragraph handles the ordinary-push case ("If the bot reviewed a previous commit … judge what was pushed since") and has no equivalent sentence. Neither does step 7, whose only dismissal trigger is a CI check going red after the bot's own approval. Grepping the whole skill for dismiss turns up exactly those two paths.

The outward failure is identical in both branches — GitHub's reviewDecision keeps reporting APPROVED because a later COMMENTED never supersedes an approval — so the asymmetry looks like an oversight rather than a distinction. It reads worse than plain silence, though: a rule stated for one branch and omitted for the structurally identical one next to it invites the reading that the omission is deliberate.

What happened here

#516 — approved at 4a476939 (01:09). Four ordinary pushes followed; ancestry is linear, every commit's parent is the prior head, so FORCE_PUSHED was false throughout. Three substantive COMMENTED reviews landed on later heads, the last at af7a7b29 (05:49) flagging a false claim in a doc comment and a commandArgv0('C:foo.exe --print')C:foo case that makes a bell rule a permanent no-op. Merged 06:25:38 with reviewDecision: APPROVED — the approval from five commits and 5 h 16 m earlier — and two unresolved, non-outdated bot threads, the newer one posted 36 minutes before the merge.

#513 — same shape, lighter damage. Approved at bd007d26 (15:43:59), then a substantive COMMENTED review at 77c1d876 (16:41:57); merged 17:59:49 reading APPROVED with that review's thread unresolved.

The #516 session saw the approval and reasoned past it. Its closing message (run 33589679154):

no approval change (the earlier APPROVE at 4a47693 stands as a prior review; the two new commits are substantive, so a fresh verdict was warranted)

It concluded a fresh verdict was warranted and still left the old one standing. The #513 session reached the same conclusion independently.

The fix I'd propose

One sentence in the ordinary-push paragraph mirroring line 55 — when the last substantive bot review was an APPROVED and this round posts findings rather than an approval, dismiss it, using the same $REVIEW_ID and the same reviews/$REVIEW_ID/dismissals call step 7 already documents. bot-review-state.sh already returns .last_substantive.state and .last_substantive.id, so nothing new is needed to detect it.

I'd let the tend maintainer pick the wording and placement; the filing would carry the evidence above and the observation that bot-review-state.sh already surfaces everything the check needs.

Why upstream rather than the overlay here

Nothing about this is dormouse-specific — any consumer whose contributors push follow-up commits onto an approved PR hits it. Putting it in .claude/skills/running-tend/SKILL.md would fix it here while leaving every other consumer exposed, and would need removing once upstream lands.

Checks already done

Full gate assessment and the surrounding run census are in the evidence log on #511.

Say the word and I'll file it; if you'd rather it stayed a local overlay note, or dropped entirely, that works too.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions