fix: gate merge-ready on the real merge predicate, not the watcher exit (BRO-1489)#4
Conversation
…it (BRO-1489)
Rule-of-three this session on bstack PR #78: `gh pr checks --watch` returned
exit 0 three times while the PR was NOT safe to merge (UNSTABLE / pending
required checks / mid-flight bot review). The watcher exit code is
necessary-not-sufficient, but `cmd_merge_ready` trusted GREEN (= watcher exit 0)
and promoted straight to MERGE_READY → auto-merge could fire on a not-actually-
mergeable PR. Caught each time only by agent reasoning, not the mechanism.
Fix (scripts/p9.py):
- merge_ready_verdict(pr, repo): query `gh pr view --json mergeable,
mergeStateStatus,reviewDecision` + best-effort `gh api graphql` unresolved-
review-thread count. Ready iff mergeStateStatus ∈ {CLEAN, UNSTABLE}, not
CONFLICTING, reviewDecision != CHANGES_REQUESTED, 0 unresolved threads.
BLOCKED/DIRTY/BEHIND/DRAFT/UNKNOWN or ANY gh error → not ready (fail-safe).
Trusts GitHub's server-computed required-vs-non-required (mergeStateStatus)
instead of fragile isRequired parsing.
- _unresolved_review_threads(): graphql count; -1 if undeterminable (does not
block — mergeStateStatus governs; the thread check is a stricter reflex-18 layer).
- cmd_merge_ready VERIFIES the verdict before GREEN→MERGE_READY (--no-verify escape).
- New `p9 merge-status <pr> [--json]` subcommand — query the predicate directly,
exit 0 iff merge-ready.
Validation: 22 new tests (predicate states + fail-safes + the gate) + full suite
130/130 green. Dogfooded live: `p9 merge-status 78` on the merged bstack PR →
NOT-READY; graphql thread count returned 0 against real gh. Dogfooding ALSO
caught the original `reviewThreads`-is-not-a-pr-view-field bug → fixed (moved to
graphql). P20: independent review PASS 9/10, no blockers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 36 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
BRO-1489 — the watcher exit code is necessary-not-sufficient
Rule-of-three this session on bstack PR #78:
gh pr checks --watchreturned exit 0 three times while the PR was NOT safe to merge — UNSTABLE / pending required checks / mid-flight CodeRabbit review.cmd_merge_readytrustedGREEN(= watcher exit 0) and promoted straight toMERGE_READY, soauto-mergecould fire on a not-actually-mergeable PR. Each time it was caught only by agent reasoning, never by the mechanism.Fix
merge_ready_verdict(pr, repo)— the real predicate:gh pr view --json mergeable,mergeStateStatus,reviewDecision+ a best-effortgh api graphqlunresolved-review-thread count. Ready iffmergeStateStatus ∈ {CLEAN, UNSTABLE}, notCONFLICTING,reviewDecision != CHANGES_REQUESTED, 0 unresolved threads.BLOCKED/DIRTY/BEHIND/DRAFT/UNKNOWNor any gh error → not ready (fail-safe). Trusts GitHub's server-computed required-vs-non-required (mergeStateStatus) over fragileisRequiredparsing.cmd_merge_readyverifies the verdict beforeGREEN→MERGE_READY(--no-verifyescape for test/offline). Closes the loop: refuse → staysGREEN→auto-mergeblocks.p9 merge-status <pr> [--json]— query the predicate directly; exit 0 iff merge-ready.Test plan / validation
tests/test_p9_merge_verdict.py): every predicate state, all fail-safe error paths, the gate refusing on not-ready. Full suite 130/130 green.p9 merge-status 78on the merged bstack PR → NOT-READY; the graphql thread count returned 0 against real gh. Dogfooding also caught the originalreviewThreads-is-not-a-gh pr view-field bug → fixed (moved to graphql).P20 cross-review
Independent reviewer: PASS 9/10, no blockers. Verified no false-positive READY (hard-blocking states checked first;
REVIEW_REQUIRED→BLOCKEDgap closed via mergeStateStatus), every error path fails safe, the gate closes the loop, tests non-tautological. Informational: graphqlfirst:100thread cap only relaxes the stricter layer.Provenance: bstack PR #78 (BRO-1482) this session.
🤖 Generated with Claude Code