Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .console/log.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
7 changes: 7 additions & 0 deletions src/operations_center/config/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
35 changes: 28 additions & 7 deletions src/operations_center/entrypoints/pr_review_watcher/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down Expand Up @@ -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 — "
Expand Down
89 changes: 89 additions & 0 deletions tests/integration/reviewer/test_ci_green_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions tests/test_pr_review_watcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions tests/verdicts/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading