Skip to content

fix(mention): skip a third party's content-free approval - #955

Open
tend-agent wants to merge 2 commits into
mainfrom
hourly/review-31605694819
Open

fix(mention): skip a third party's content-free approval#955
tend-agent wants to merge 2 commits into
mainfrom
hourly/review-31605694819

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

Problem

tend-mention starts a full billable session every time someone other than the bot approves a PR with an empty review body and no inline comments. The session reads the PR, finds nothing addressed to it, and exits silently. It can only ever no-op: a bare APPROVED asks for nothing, and the bot is barred from merging, so there is no role left for it to act in.

verify has two terminal-review gates and both key on REVIEW_AUTHOR: the empty-body APPROVED gate (#747) and the synthetic reply-container gate (#849/#866). A third party's approval matches neither, so it falls through to the PR_AUTHOR == bot short-circuit on a bot-authored PR — or to BOT_REVIEWS, which counts the triggering review itself — and should_run=true is unconditional. Structural: there is no decision point, so the same shape recurs every time a human approves one of the bot's PRs without typing anything.

Distinct from the three issues already open or closed on this path. #747 fixed the bot's own empty-body approval; #866 targets the synthetic empty-body COMMENTED container GitHub creates for an inline reply; #915/#916 target the bot's own content-bearing review on someone else's PR. All three are author-keyed by design; this is the residual their author-keying leaves behind, and it was named as such in the evidence log two days before this run.

Evidence

Three occurrences on PRQL/prql, all traced to the review record rather than inferred from the run:

When PR Approver Review Handle job
2026-08-10 #6164 kgutwin APPROVED, empty body, no inline comments 31395281267 — 2 m 10 s, published nothing
2026-08-12 08:18Z #6185 vanillajonathan 4914418538, APPROVED, body length 0, pulls/6185/comments empty 31577748274 — 08:18:45Z→08:20:35Z (110 s), published nothing
2026-08-12 13:27Z #6186 kgutwin 4916991960, APPROVED, body length 0, zero inline comments 31601473217 — 13:27:22Z→13:28:51Z (89 s), published nothing

The third one's session log says why it published nothing, in five tool calls: "The trigger was a plain approval (APPROVED, empty body, no inline comments) from @kgutwin on PR #6186, which is already MERGED. No questions, no requested changes, no unaddressed comments anywhere on the thread. Per the review-response rules, a plain approval gets no reply, so I'm not posting anything." The model reaches the right verdict every time; the cost is that it has to boot to reach it.

Solution

A third clause on the same gate chain, inside the pull_request_review block and after the @-mention scan, so a mention buried in an inline comment still summons the bot:

if [ "$REVIEW_STATE" = "approved" ] \
   && [ -z "$COMMENT_BODY" ] \
   && [ -z "$INLINE" ]; then
  echo "should_run=false" >> "$GITHUB_OUTPUT"
  exit 0
fi

It costs no extra API call — $INLINE is already fetched a few lines above for the mention scan.

Two narrowings, both load-bearing:

  • approved, not any state. A human's reply to one of the bot's review threads also arrives as an empty-body review container, but COMMENTED. Widening the state here would silence the bot on every human reply to its own threads — the same trap test_mention_skips_bot_reply_container pins from the author side, since pull_request_review_comment subscribes to edited only.
  • $INLINE empty, not reply-only. An approval whose nits live in inline comments is a request addressed to the PR's author, and on a bot-authored PR that is a role the bot has to act in. Only a review with nothing in it anywhere is terminal.

The two author-keyed gates stay as they are: the bot's own empty-body APPROVED is skipped before the $INLINE fetch, which keeps that path one API call cheaper.

Gate assessment

  • Evidence level: High — a consistent pattern across three sessions on two PRs and two approvers, spanning two days. High needs 2–3; this is 3.
  • Structural, not stochastic. The gate condition is author-keyed in the workflow YAML, so replaying any of these three scenarios ten times boots the agent ten times. The model's judgement is not involved and is not at fault.
  • Change type: targeted fix — one clause on an existing chain, reusing data already fetched. Normal bar, met by Gate 1.
  • Both gates passed.

Evidence log: https://gist.github.com/192514ea2c36586f9b7f842a482d62ab (review-reviewers evidence: PRQL/prql 2026-08), where this finding was recorded at one occurrence on 2026-08-10 and at two on 2026-08-12 before crossing here.

Testing

test_mention_skips_third_party_bare_approval compares the whole gate condition with whitespace normalized, so an author clause added later — or the $INLINE clause dropped — fails on equality rather than sliding past an in check. It fails on main (ValueError: substring not found). The four mention regtest snapshots are updated. Full generator suite: 420 passed.

@tend-agent tend-agent added the claude-behavior Behavioral issues found by review-reviewers label Aug 12, 2026

@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 reads correctly: $INLINE is populated a few lines above, an empty review-comments array leaves it the empty string, and approved + empty body + zero inline comments is genuinely terminal. Regtest snapshots are all updated and the suite is green locally (420 passed).

One gap in the new test. Its stated job is to pin the load-bearing narrowings, but the property that actually makes [ -z "$INLINE" ] mean "zero inline comments" is positional — the gate has to sit below INLINE=$(gh api ... /reviews/$PAYLOAD_ID/comments ...). The condition-equality assert doesn't encode position, and the existing ordering assert (< BOT_REVIEWS=$() still passes if the gate is hoisted above the fetch. That hoist is a plausible future edit, since the bot-authored approval gate deliberately sits before the fetch and the comment there advertises it as "one API call cheaper" — and it fails silently: an unset $INLINE reads as empty, so every third-party approval carrying inline nits gets skipped, which is exactly the case this gate's comment says must fire.

Comment thread generator/tests/test_generate.py
`[ -z "$INLINE" ]` only means "zero inline comments" below the fetch that populates it. That property is positional, so the condition-equality assert does not cover it, and the existing BOT_REVIEWS ordering assert still passes if the gate is hoisted above the fetch — a plausible edit, since the bot-authored approval gate deliberately sits before it to save an API call. Hoisted, $INLINE is unset, reads as empty, and every third-party approval carrying inline nits is skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-behavior Behavioral issues found by review-reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant