fix(reviewer): require the full configured check set present + green before merge (guard D) - #282
Merged
Merged
Conversation
…before merge (guard D) #272 and Guard C close "merge on red CI", "merge while checks run", and "merge on a head with no checks". But a hole remained: a required check that lives in a SEPARATE GitHub Actions workflow registers slightly later than the main CI workflow. In that window it is invisible to both get_failed_checks (no conclusion yet — actually no check-run at all) and get_incomplete_checks (not a check-run yet), so the gate sees the main-workflow checks completed+green and merges before the late check ever runs. That is exactly how #277/#278 reached main with a red `audit` job — the very check that surfaces the new OC12/OC13 divergence guards. Without this, a future PR that trips those guards could merge anyway. Fix: - New per-repo `required_checks` config (RepoConfig). A check name satisfies an entry if it contains the entry (case-insensitive), matching ci_ignored_checks. - The primary self-review gate and the WO-3 no-progress merge path now require every configured required check to be PRESENT and passing on the current head before treating CI as green. `failed` is already empty at the gate, so a required check is satisfied iff it appears in the completed set; a missing one defers via the existing ci_wait_cycles bound (escalates ci_never_settled if it never shows). Activate per repo by setting required_checks (e.g. ["audit"]) in the local config; default is empty (no behaviour change for unconfigured repos). +2 gate tests (required check absent → defer; present+green → proceeds to merge). Mock repo_cfg factories default required_checks=[]. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The hole #272 + Guard C left open
Those close "merge on red CI", "merge while checks run", and "merge on a head with no checks at all". But a required check in a separate workflow registers slightly after the main
CIworkflow. In that window it's invisible to bothget_failed_checks(no check-run yet) andget_incomplete_checks(not a run yet), so the gate sees the main-workflow checks completed+green and merges before the late check ever runs.That is exactly how #277/#278 reached main with a red
auditjob — the very check that surfaces the new OC12/OC13 divergence guards. So without this, a future PR that trips those guards could merge anyway. This is the keystone that makes Guards A/B actually block rather than just report.Fix
required_checksconfig (RepoConfig). A check name satisfies an entry if it contains the entry (case-insensitive), matchingci_ignored_checkssemantics.failedis already empty at the gate, so a required check is satisfied iff it appears in the completed set; a missing one defers via the existingci_wait_cyclesbound (escalatingci_never_settledif it never shows).Activation is per-repo via
required_checks(e.g.["audit"]) in the local config; default empty → no behaviour change for unconfigured repos. (I'll setrequired_checks: [audit]for OperationsCenter in the live local config once this merges.)Tests
+2: required check absent → defer (no self-review, no merge, wait counter advanced); required check present+green → proceeds through self-review LGTM to merge. Mockrepo_cfgfactories defaultrequired_checks=[]. Full reviewer suite passes (175);ruffclean; audit clean.🤖 Generated with Claude Code