fix(comment-gate): no-finding bot summaries keep approval on hands-off PRs

Merged
DO
1d ago
+904-16

Summary

Stops the maintainer comment gate from dismissing pr-review's approval on dev-lead:hands-off PRs whose only bot comments are no-finding summaries. A Codex or CodeRabbit summary now clears when the bot's own structured state says the review found nothing. SonarCloud's passed notice already cleared. Comments that report real findings still block.

Problem

On a dev-lead:hands-off PR, pr-review approves and then dismisses its own approval about a minute later ("Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)"). This hit #2456 and #2467 on 2026-11-06.

Codex and CodeRabbit each keep one summary comment and edit it on every push. Under #2008 every edit needs a new dev-lead disposition reply. Dev-lead stays off a hands-off PR, so no reply is ever written. Maintainers must not forge the markers. The user approves as the same donpetry-bot account, so the next sweep dismisses their approval too. On #2467 the PR was only unstuck by removing hands-off and running dev-lead-retry.yml by hand three times.

Change

A Codex or CodeRabbit summary no longer needs a disposition when the bot's own structured state says its review found nothing. The match is on that state, never on prose. It holds however often the bot edits the comment. The new no_finding_summary jq predicate is in scripts/lib/maintainer-comment-gate.sh.

BotClears only when
chatgpt-codex-connectorThe body opens with <!-- codex-pull-request-review-summary --> and has exactly one codex-review-state:v1 JSON. In it the code review is completed, has_any_findings and has_unresolved_findings are both false, and commit_sha is 40 hex characters. The security review either never ran (all fields null) or completed the same way.
coderabbitaiThe summary has exactly one final_review_risk_label field reading Merge Risk ⚪ Minimal, exactly one final_review_risk_coverage JSON with kind reviewed, and only passed pre-merge check badges.
sonarqubecloudNo change. The existing #1918 info_status_pattern already clears the passed notice.

Each case also requires all of the following, or the comment still blocks:

  • The author is the GitHub App (author.__typename == "Bot"). A User account, another bot, or a missing type still blocks.
  • No finding-bearing section (reviewer_sources_finding_section_pattern) is present.

Every other state still blocks: failed, in progress, findings present, unparseable, a duplicated state block, or a state quoted further down the body. CodeRabbit 🔵 Low still blocks too. In a sample of the last 40 PRs in five combsmith repos, every Low rating came with a stated follow-up.

The same predicate also filters the gate's sibling lists, so they cannot disagree with the gate:

  • dev-lead's candidate list (maintainer_gate_open_comment_ids)
  • the #2008 re-open list (maintainer_gate_reopen_candidates)
  • the stale-disposition sweep (maintainer_gate_stale_dispositions)
  • the bot-comment retry (bcr_retry_decisions)

Nothing re-opens, chases or retries a summary the gate clears. ADR-0012 gains exemption 6, and docs/pr-review-agent/maintainer-comment-gate.md documents it.

Checked against 1,001 sampled bot comments: 28 Codex and 12 CodeRabbit summaries clear. Every Codex summary with findings or a failed review still blocks, as does every CodeRabbit summary rated Low, Moderate or High or with a failed, warning or inconclusive pre-merge check. The live #2467 and #2456 snapshots (all three bot comments unminimized, as during the deadlock) now return rc=0. Before this change they returned rc=1.

Sweep: one fix-bot-comment per PR per run (confirmed, not changed)

scan_pr_for_undispositioned_bot_comments dispatches only the oldest candidate (first(.[] | select(.decision == "dispatch"))). This is on purpose. Every retry joins the same per-PR concurrency lane, and a second dispatch would cancel the first while it is still pending. That is the loss #2017/#2262 recover from. The fix-bot-comment pass handles a single comment from start to finish: the intent payload, the prompt, the terminal marker and the retry dedup. Handling all comments in one run would need a multi-comment pass, which is a separate change. This PR takes the no-finding summaries out of the retry set entirely, so a hands-off PR no longer needs any retries for them.

Test plan

  • tests/dev-lead/unit/test_maintainer_comment_gate.bats (+24 tests) covers:
    • the full hands-off #2467 snapshot (clears, and has nothing to disposition, re-open or chase);
    • Codex state variants;
    • CodeRabbit risk, coverage and pre-merge variants;
    • Sonar passed and failed notices;
    • author-type checks;
    • finding-bearing sections.
  • tests/dev-lead/unit/test_bot_comment_retry.bats (+2 tests): no-finding summaries are not retry candidates; summaries with findings still dispatch.
  • Live fixtures from #2467 and #2456 are in tests/fixtures/{codex,coderabbit,sonarqubecloud}/. CodeRabbit's scope= link parameter is scrubbed.

Local runs used bash 5.3 (macOS /bin/bash 3.2 cannot parse comment-disposition-verify.sh on main). The gate suite passes 150/150. The sweep(#2262) tests in test_bot_comment_retry.bats also fail locally on unmodified main, so they are not affected by this change; CI is the reference for them.

Risk

Medium. The change is narrow, but it loosens an approval gate. A wrong match would clear a summary that should have blocked. The predicate limits that: Bot author only, structured state only, exact single fields, and the finding-section guard still applies. If Codex or CodeRabbit changes its state format, the predicate stops matching and the gate goes back to blocking (fail closed), never the other way.

Rollback

Revert this PR. The gate goes back to requiring a disposition per edit. No other side effects.

Monitoring

pr-review runs on hands-off PRs should stop logging skip: a PR issue comment lacks a verified disposition and stop posting "Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)" when the only bot comments are no-finding summaries. dev-lead-retry.yml should stop dispatching fix-bot-comment for them.

Interaction contract

  • N/A — this PR does not add or change an agentic role.

Design decision: no head-SHA check on no-finding summaries (maintainer-accepted)

no_finding_summary deliberately does not compare CodeRabbit's coveredCommitId or Codex's commit_sha with headRefOid. A summary that reported no findings for an earlier commit still clears the comment gate when the bot never reviewed the current head (for example, because it was rate-limited). This is intentional:

  • Policy: we don't require every advisory reviewer to take part in every PR. A PR may merge while some bots are rate-limited.
  • Consistent with the advisory gate: check_advisory_reviews (scripts/lib/advisory-review-gate.sh) already removes RATE_LIMITED or UNSUPPORTED bots from the required set (#657). It proceeds on partial evidence after the 20-minute head-age or 10-minute quiescence timeout (#1193), and records it via _record_partial_evidence (#1596). It also counts a bot's latest submission whatever commit it covered. The rate-limited marker only schedules a sweep retry; it is not a hard hold. So no gate requires a bot to review the head, and requiring it here alone would bring back the deadlock this PR fixes, now for any rate-limited PR.
  • Separation of concerns: the comment gate answers "does any bot comment still have an undispositioned finding?", not "did every bot review the head?" A stale summary that reports findings, or a Low/Moderate/High rating, still blocks. Inline findings remain review threads that need resolving.
  • Backstop: pr-review itself always does a full review of the current head before approving.

The qa-lead advisory (stale-head clear) and pr-review finding 1 on 307098a are therefore accepted as designed. The fixture pr2467-summary-minimal-ratelimited.md clearing the gate is the intended behavior.

Release channel

This changes scripts/lib/maintainer-comment-gate.sh, scripts/lib/bot-comment-retry.sh and scripts/dev-lead-fix-reviews.sh. Merging to main alone does not activate it. It rides the Release Manager's next 4-hourly autocut to pr-review/v1-next and dev-lead/v139-next. That is when the deadlock fix takes effect here, because pr-review-trigger.yml pins @pr-review/v1-next. It reaches the consumer repos pinned to pr-review/v1-stable and dev-lead/v139-stable only through gated ring promotion.

Checklist

  • shellcheck passes for the changed shell scripts (maintainer-comment-gate.sh, bot-comment-retry.sh).
  • Docs updated where behavior or standards changed (ADR-0012, docs/pr-review-agent/maintainer-comment-gate.md).

🤖 Generated with Claude Code