Skip to content

fix(mention): skip the bot's own review on someone else's PR - #916

Open
tend-agent wants to merge 2 commits into
mainfrom
fix/issue-915
Open

fix(mention): skip the bot's own review on someone else's PR#916
tend-agent wants to merge 2 commits into
mainfrom
fix/issue-915

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

Problem

tend-mention's verify gate starts a full billable session for every non-empty-body review the bot leaves on a same-repo PR someone else authored. The session reads the PR, recognises the trigger as its own review, and exits silently — it can only ever no-op: the bot is the reviewer, the findings are addressed to the PR's author, and pushing unbidden to another author's branch is barred by running-in-ci's conduct rules, so there is no author role left to act in.

For a pull_request_review event the gate falls through to the participation heuristic, which counts the triggering review itselfBOT_REVIEWS is never empty, so should_run=true is unconditional. #747's gate is the same shape narrowed to one leg (approved and empty body), which excludes every review in #915's evidence: five occurrences in one 24h window on PRQL/prql, APPROVED and COMMENTED alike, bodies 455–1920 chars, $1.79 booked plus one run that hit the account's session limit before it could bill.

Solution

One author-keyed gate between the PR_AUTHOR resolution and the BOT_REVIEWS heuristic. Placement is the whole design, in three directions:

Keyed on author alone: reusing #747's REVIEW_STATE / empty-body clauses here would let every bodied COMMENTED review straight through.

Fork PRs are unaffected either way — the relay job's head.repo.full_name == github.repository condition already means a bot review on a fork PR never dispatches.

Testing

test_mention_skips_bot_review_on_another_authors_pr pins the gate and all three orderings, and asserts the state/body clauses are absent from it. It fails on the pre-change template at the first assertion. test_mention_self_comment_skip_spares_review_submissions had its APPROVED-gate slice narrowed to end at the inline-comment fetch, so it keeps discriminating between the two gates rather than passing on either. Full generator suite: 380 passed. pre-commit clean on the changed files.

Relationship to #849

Complementary, not overlapping. #849 (for #866) drops the synthetic zero-body COMMENTED container GitHub wraps around an inline reply, on any PR including the bot's own — where this gate cannot reach, because PR_AUTHOR == bot exits first. This one drops real bodied reviews on other authors' PRs, which #849's -z "$COMMENT_BODY" clause excludes. Both touch mention.yaml.j2; whichever lands second will need a one-line rebase on the header comment listing the self-review skips.


Closes #915 — automated triage

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gate itself is right, and its placement holds up under trace: REVIEW_AUTHOR is only ever set on the pull_request_review arm of the dispatch case, so the second clause can't read a stale value from an issue_comment; the earlier APPROVED + empty-body gate stays load-bearing for the bot's own PRs rather than becoming dead; and all four mention regtests that carry the verify block are updated (test_extras_apply_path_regtest.out renders tend-review, so it's correctly untouched). Suite passes at 380 locally.

Three things on the prose and the test, none blocking.

The header comment now says the two skips are keyed "on the review being terminal for the bot rather than on authorship", while the new gate's own comment eleven lines later says "Keyed on author alone". Both are describing the same gate in opposite terms — "authorship" means self-authorship of the review in the first and review-author as the key in the second. That block's whole job is to steer a future editor away from a blanket self-review skip, so a framing that reads as "these aren't author-keyed" is the wrong signal to leave there.

The rationale in the gate comment — pushing unbidden to another author's branch is barred by conduct rules — is narrower than the set of PRs the gate covers. On a Dependabot/renovate PR the review skill's step 8 explicitly directs pushing the fix, so "no author role left to act in" isn't why mention is skippable there; it's skippable because tend-review pushes that fix in its own session and never needed a mention run. The behaviour is right either way, but as written the comment will read as an oversight to whoever next debugs a bot-authored PR that stopped waking mention.

The last assertion in the new test doesn't discriminate. run.index("reviews/$PAYLOAD_ID/comments") < run.index("BOT_REVIEWS=$(") holds on the pre-change template too (the inline fetch has always preceded the heuristic), so the third ordering the docstring claims to pin isn't pinned by anything except the gate slice bounds. Anchoring the right-hand side to the gate's own index makes it fail pre-change, like the other two.

The #849 overlap is called correctly — that one is only reachable on bot-authored PRs once this gate lands, since everything it targets on someone else's PR is now caught here first. Both edit this same header comment, so whichever is second wants the rebase you flagged.

Comment thread generator/src/tend/templates/mention.yaml.j2 Outdated
Comment thread generator/src/tend/templates/mention.yaml.j2 Outdated
Comment thread generator/tests/test_generate.py Outdated
@tend-agent

Copy link
Copy Markdown
Collaborator Author

All three addressed in 5a3e441.

The header comment no longer denies the gates are author-keyed — it says they are, and that each is additionally narrowed to a case that leaves the run nothing to do, which is the steer against a blanket self-review skip stated without the contradiction.

On the rationale: confirmed against the review skill, which directs pushing the fix on Dependabot/renovate PRs rather than stopping at analysis. So the conduct-rules clause was the wrong load-bearing reason. Reframed to what actually covers both cases — whatever the review warranted, the tend-review session that submitted it has already done: left the findings for a human author (where pushing to their branch unbidden is barred), or pushed the fix itself where no author will act.

The third assertion now anchors its right-hand side to the gate's own index rather than to BOT_REVIEWS=$(. Verified it discriminates: with main's template restored under the new test file, the test fails; with the gate, it passes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tend-mention: bot's own non-empty-body review on someone else's PR starts a guaranteed no-op session (#747's sibling leg)

1 participant