From 19d82245c544f718a92c8b57c02572190f67e0cf Mon Sep 17 00:00:00 2001 From: ProtocolWarden Date: Sat, 13 Jun 2026 18:03:16 -0400 Subject: [PATCH] fix(reviewer): require the full configured check set present + green before merge (guard D) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- .console/log.md | 12 +++ src/operations_center/config/settings.py | 7 ++ .../entrypoints/pr_review_watcher/main.py | 35 ++++++-- .../reviewer/test_ci_green_gate.py | 89 +++++++++++++++++++ tests/test_pr_review_watcher.py | 1 + tests/verdicts/conftest.py | 1 + 6 files changed, 138 insertions(+), 7 deletions(-) diff --git a/.console/log.md b/.console/log.md index 230d6ac8e..2f9f02561 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,15 @@ +## 2026-06-13 — fix(reviewer): gate merge on the full required-check set (guard D) + +#272 + Guard C close "merge on red/incomplete/no-checks CI", but a hole remained: a required check +in a SEPARATE workflow that registers later than the main CI workflow is invisible to both the +failed and incomplete lists, so the gate sees the main-workflow checks green and merges before the +late check (e.g. the `audit` job) ever runs. This is how #277/#278 reached main with red audit (the +very job that surfaces the OC12/OC13 divergence guards). Fix: new per-repo required_checks config; +the self-review gate and the no-progress merge path now require every required check PRESENT and +passing on the current head before green (a required check missing from the completed set defers via +the existing ci_wait_cycles machinery). +2 gate tests; mock repo_cfg defaults required_checks=[]. +Activate per-repo by setting required_checks (e.g. [audit]) in the local config. + ## 2026-06-13 — Stage 9: Commit and push to existing branch (✅ COMPLETE) ### Objective diff --git a/src/operations_center/config/settings.py b/src/operations_center/config/settings.py index 68b87eac8..2df529bb4 100644 --- a/src/operations_center/config/settings.py +++ b/src/operations_center/config/settings.py @@ -345,6 +345,13 @@ class RepoSettings(BaseModel): # (e.g. a file-tag linter that was broken before the PR landed). Checks # whose names contain any of these strings are excluded from the failed list. ci_ignored_checks: list[str] = Field(default_factory=list) + # CI check names that MUST be present, completed, and passing before the + # reviewer treats CI as green. A check name "satisfies" an entry if it + # contains the entry (case-insensitive). This closes the late-registering + # check hole: a required check that lives in a separate workflow and has not + # registered yet would otherwise be invisible to the failed/incomplete lists, + # letting a PR merge before that check runs (e.g. the `audit` job). + required_checks: list[str] = Field(default_factory=list) # Executor selection hint for this repo. Valid values: ``"team_executor"``, # ``"dag_executor"``, ``"critique_executor"``. # Routing decisions are made by SwitchBoard; this is an operator preference hint only. diff --git a/src/operations_center/entrypoints/pr_review_watcher/main.py b/src/operations_center/entrypoints/pr_review_watcher/main.py index eacd3b2a3..6b036b301 100644 --- a/src/operations_center/entrypoints/pr_review_watcher/main.py +++ b/src/operations_center/entrypoints/pr_review_watcher/main.py @@ -1667,13 +1667,27 @@ def _phase1( pr_data=pr_data, ignored_checks=ignored, ) - if pending or not completed: + # Guard D: every configured required check must be PRESENT and passing + # on the current head. `failed` is already empty here (the failed-checks + # branch above returned), so a required check is satisfied iff it appears + # in `completed`. This closes the late-registering-check hole: a required + # check living in a separate workflow that has not registered yet is + # invisible to both failed and pending, so without this a PR could merge + # before that check (e.g. the `audit` job) ever runs. + required = list(getattr(repo_cfg, "required_checks", []) or []) + missing_required = [ + rc + for rc in required + if not any(rc.lower() in name.lower() for name in completed) + ] + if pending or not completed or missing_required: state["ci_wait_cycles"] = state.get("ci_wait_cycles", 0) + 1 - _why = ( - f"{len(pending)} still running: {', '.join(pending[:5])}" - if pending - else "no checks have reported on the current head yet" - ) + if pending: + _why = f"{len(pending)} still running: {', '.join(pending[:5])}" + elif not completed: + _why = "no checks have reported on the current head yet" + else: + _why = f"required checks not yet reported: {', '.join(missing_required)}" if state["ci_wait_cycles"] >= _MAX_CI_WAIT_CYCLES: detail = ( f"CI has not settled-green on the current head after " @@ -1996,7 +2010,14 @@ def _phase1( _completed_np = gh_client.get_completed_checks( owner, repo, pr_number, pr_data=pr_data, ignored_checks=_ign_np ) - if not _failed_np and not _pending_np and _completed_np: + # Guard D: every required check must be present and passing too. + _required_np = list(getattr(_rcfg_np, "required_checks", []) or []) + _missing_np = [ + rc + for rc in _required_np + if not any(rc.lower() in name.lower() for name in _completed_np) + ] + if not _failed_np and not _pending_np and _completed_np and not _missing_np: logger.info( "pr_review_watcher: PR #%d repeated no-progress after " "CI-green retraction budget exhausted; CI still green — " diff --git a/tests/integration/reviewer/test_ci_green_gate.py b/tests/integration/reviewer/test_ci_green_gate.py index 50f5fcd66..9b39d4fbd 100644 --- a/tests/integration/reviewer/test_ci_green_gate.py +++ b/tests/integration/reviewer/test_ci_green_gate.py @@ -177,6 +177,95 @@ def test_ci_with_no_checks_on_head_defers( gh.merge_pr.assert_not_called() assert load_pr_state(state_path)["ci_wait_cycles"] == 1 + def test_required_check_not_yet_reported_defers( + self, + tmp_path: Path, + audit_verdict_builder: AuditVerdictBuilder, + ): + """Guard D: a configured required check that hasn't registered on the head + (e.g. a separate-workflow `audit` job that starts later) → defer. + + Without this, the gate would see the main-workflow checks completed+green + and the audit check simply absent (not failed, not pending) and merge before + audit ever runs — which is how red-audit PRs reached main. + """ + settings = mock_settings() + settings.repos["TestRepo"].required_checks = ["audit"] + gh = mock_github_client() + + state = create_pr_state( + repo_key="TestRepo", + pr_number=42, + phase="self_review", + self_review_loops=0, + ) + state_path = save_pr_state(tmp_path, state) + + gh.get_failed_checks.return_value = [] + gh.get_incomplete_checks.return_value = [] + # main-workflow checks done & green, but `audit` has not registered yet. + gh.get_completed_checks.return_value = ["Test (pytest)", "Lint (ruff)"] + + with patch.object(watcher, "_run_pipeline") as mock_pipeline: + watcher._phase1( + state, + state_path, + {"number": 42, "title": "Test PR", "draft": False, "head": {"ref": "goal/42"}}, + gh, + "owner", + "TestRepo", + tmp_path, + tmp_path / "cfg.yaml", + settings, + ) + + mock_pipeline.assert_not_called() + gh.merge_pr.assert_not_called() + assert load_pr_state(state_path)["ci_wait_cycles"] == 1 + + def test_required_check_present_and_green_proceeds( + self, + tmp_path: Path, + audit_verdict_builder: AuditVerdictBuilder, + ): + """Guard D: once the required check has reported (and passed), proceed.""" + settings = mock_settings() + settings.repos["TestRepo"].required_checks = ["audit"] + gh = mock_github_client() + + state = create_pr_state( + repo_key="TestRepo", + pr_number=42, + phase="self_review", + self_review_loops=0, + ) + state_path = save_pr_state(tmp_path, state) + + gh.get_failed_checks.return_value = [] + gh.get_incomplete_checks.return_value = [] + gh.get_completed_checks.return_value = ["Test (pytest)", "audit"] + gh.get_mergeable.return_value = True + + with patch.object( + watcher, + "_run_direct_review", + return_value={"result": "LGTM", "summary": "All checks passed"}, + ): + watcher._phase1( + state, + state_path, + {"number": 42, "title": "Test PR", "draft": False, "head": {"ref": "goal/42"}}, + gh, + "owner", + "TestRepo", + tmp_path, + tmp_path / "cfg.yaml", + settings, + ) + + # required check satisfied → CI green → self-review LGTM → merge + gh.merge_pr.assert_called_once_with("owner", "TestRepo", 42, merge_method="squash") + def test_ci_red_then_green_allows_merge_after_fix( self, tmp_path: Path, diff --git a/tests/test_pr_review_watcher.py b/tests/test_pr_review_watcher.py index e7efaaea2..016faef9a 100644 --- a/tests/test_pr_review_watcher.py +++ b/tests/test_pr_review_watcher.py @@ -587,6 +587,7 @@ def _settings_with_ci_green_repo() -> MagicMock: repo_cfg = MagicMock( auto_merge_on_ci_green=True, ci_ignored_checks=[], + required_checks=[], clone_url=f"git@github.com:owner/{REPO_KEY}.git", default_branch="main", await_review=True, diff --git a/tests/verdicts/conftest.py b/tests/verdicts/conftest.py index b979c3b56..8d535669d 100644 --- a/tests/verdicts/conftest.py +++ b/tests/verdicts/conftest.py @@ -300,6 +300,7 @@ def mock_settings( repo_cfg = MagicMock( auto_merge_on_ci_green=True, ci_ignored_checks=[], + required_checks=[], clone_url=f"git@github.com:owner/{repo_key}.git", default_branch="main", await_review=True,