feat(code-review): secondary confirmation of real CI on head commit (part 2 of #2034)

.apache-magpie-overrides/tools/vetted-ops/config.toml

@@ -80,6 +80,9 @@ close_reasons = ["completed", "not planned"]
     "cr-commit-files",
     "cr-path-commits",
     "cr-commit-pulls",
+    # The head commit's check runs, read before an APPROVE to confirm real CI
+    # ran there (code-review disposition's secondary check, #2072).
+    "check-runs",
 ]
 
 # pr-management-triage reads a whole sweep through `vetted-op-read --save`,

plugins/magpie-pr-management/skills/code-review/review-flow.md

@@ -302,7 +302,7 @@ When the result's `auto_merge.eligible` is true — the repository allows auto-m
 
 > *Suggested: `[M]` `APPROVE+AUTO-MERGE` (default) — approve, then enable auto-merge (`<auto_merge.method>`); GitHub merges once the required checks pass. Or `[A]pprove` only, `[C]omment`, `[R]equest changes`, `[E]dit findings first`, `[S]kip-for-now`, `[Q]uit`.*
 
-With a fixup the default is `FIXUP+APPROVE+AUTO-MERGE`. When `ci_pending_held_by_auto_merge` is true, real CI is still running and the plain `[A]pprove` / `[F]ixup` are not offered — without auto-merge they would approve on unfinished CI — so the auto-merge path, `[C]omment` or `[S]kip-for-now` are the choices. When `ci_action` is set, the project's workflows have not run (waiting for approval, or bot checks only), or the installed vetted-ops is too old to show that the rollup belongs to the head commit: offer it first, then re-run `disposition`. [`classifications/disposition-auto-merge.md`](classifications/disposition-auto-merge.md) has the posting order: approve, refresh the liveness read and re-run `guard`, release any workflow runs held at the head (`release-runs` — after the approval and after any fixup push, since a push re-holds them), then enable auto-merge.
+With a fixup the default is `FIXUP+APPROVE+AUTO-MERGE`. When `ci_pending_held_by_auto_merge` is true, real CI is still running and the plain `[A]pprove` / `[F]ixup` are not offered — without auto-merge they would approve on unfinished CI — so the auto-merge path, `[C]omment` or `[S]kip-for-now` are the choices. When `ci_action` is set, the project's workflows have not run (waiting for approval, or bot checks only), the head commit's check runs contain no real CI executions, or the installed vetted-ops is too old to show that the rollup belongs to the head commit: offer it first, then re-run `disposition`. [`classifications/disposition-auto-merge.md`](classifications/disposition-auto-merge.md) has the posting order: approve, refresh the liveness read and re-run `guard`, release any workflow runs held at the head (`release-runs` — after the approval and after any fixup push, since a push re-holds them), then enable auto-merge.
 `[E]dit` lets the maintainer drop or re-classify findings; re-run `disposition` after. A `conflict_note` is always stated in the body, whatever the disposition.
 
 ---

plugins/magpie-security-tools/tools/pr-management/src/pr_management/ci.py

@@ -86,6 +86,23 @@ def real_ci_ran(pr: PR, cfg: Config) -> bool:
     return any(not _is_bot_check(n) for n in names)
 
 
+def check_runs_have_real_ci(document: dict[str, Any] | list[Any], cfg: Config) -> bool:
+    """Verify that the commit's check-runs document contains at least one real CI execution."""
+    pages = document if isinstance(document, list) else [document]
+    names: list[str] = []
+    for page in pages:
+        if isinstance(page, dict):
+            for run in page.get("check_runs") or []:
+                name = run.get("name")
+                if name:
+                    names.append(str(name))
+    if not names:
+        return False
+    if cfg.real_ci_patterns:
+        return any(re.match(p, n, flags=re.IGNORECASE) for p in cfg.real_ci_patterns for n in names)
+    return any(not _is_bot_check(n) for n in names)
+
+
 def collaborator_engaged(pr: PR) -> bool:
     if any(r.association in COLLABORATOR_ASSOCIATIONS and r.author != pr.author for r in pr.reviews):
         return True

plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py

@@ -34,6 +34,7 @@
 from pathlib import Path
 from typing import Any
 
+from .. import ci as shared_ci
 from .. import config, mentions, model
 from ..triage import guards as triage_guards
 from . import body as review_body
@@ -498,28 +499,68 @@ def _decide(
         {k: v for k, v in outdated[i].items() if not k.endswith("_untrusted")}
         for i in dict.fromkeys(args.addressed_thread)
     ]
-    return disposition.pick(
+    # Shared by both picks below, so a new gate input cannot reach only one of them.
+    pick_kwargs: dict[str, Any] = {
+        "ci_state": pr.rollup_state,
+        "real_ci": selectors.real_ci(pr, cfg),
+        "unresolved_threads": pr.unresolved_threads - len(resolve_first),
+        "other_changes_requested": disposition.other_changes_requested(list(pr.reviews), viewer, pr.author),
+        "unanswered_question": args.unanswered_question,
+        "ci_diff_caused": args.ci_diff_caused,
+        "mergeable": pr.mergeable,
+        "merge_state": pr.merge_state,
+        "permission": permission,
+        "head_pushable": args.head_pushable,
+        "resolve_first": resolve_first,
+        "auto_merge_allowed": pr.auto_merge_allowed,
+        "viewer_can_enable_auto_merge": pr.viewer_can_enable_auto_merge,
+        "auto_merge_enabled": pr.auto_merge_enabled,
+        "self_authored": viewer.lower() == pr.author.lower(),
+        "merge_method": pr.default_merge_method,
+        "pending_not_required": pr.pending_not_required,
+        "viewer_known": bool(pr.viewer),
+        "ci_outdated_query": pr.ci_outdated_query,
+    }
+    # Would the disposition approve without the secondary CI check?
+    tentative = disposition.pick(findings, **pick_kwargs)
+
+    needs: list[dict[str, Any]] = []
+    secondary_ci_verified = True
+    secondary_ci_reason = None
+    if tentative["disposition"] == "APPROVE" or tentative.get("fixup", {}).get("eligible"):
+        check_runs_path = _file(args.saved_dir, f"check-runs-{pr.head_sha}.json")
+        if check_runs_path is None:
+            needs.append(
+                _need(
+                    "check-runs",
+                    [pr.head_sha],
+                    f"check-runs-{pr.head_sha}.json",
+                    "secondary confirmation of real CI on head commit",
+                )
+            )
+            # Fail closed until the read is saved: `render --auto-merge` checks the
+            # decision, not `needs`, so a missing read must not leave APPROVE standing.
+            secondary_ci_verified = False
+            secondary_ci_reason = (
+                f"check runs for head commit {pr.head_sha[:8]} are not read yet — save them, then re-run"
+            )
+        else:
+            try:
+                check_runs_data = json.loads(check_runs_path.read_text(encoding="utf-8"))
+            except (json.JSONDecodeError, OSError):
+                check_runs_data = None
+            if check_runs_data is None or not shared_ci.check_runs_have_real_ci(check_runs_data, cfg):
+                secondary_ci_verified = False
+                secondary_ci_reason = f"secondary CI confirmation failed — no real CI check runs recorded for head commit {pr.head_sha[:8]}"
+
+    result = disposition.pick(
         findings,
-        ci_state=pr.rollup_state,
-        real_ci=selectors.real_ci(pr, cfg),
-        unresolved_threads=pr.unresolved_threads - len(resolve_first),
-        other_changes_requested=disposition.other_changes_requested(list(pr.reviews), viewer, pr.author),
-        unanswered_question=args.unanswered_question,
-        ci_diff_caused=args.ci_diff_caused,
-        mergeable=pr.mergeable,
-        merge_state=pr.merge_state,
-        permission=permission,
-        head_pushable=args.head_pushable,
-        resolve_first=resolve_first,
-        auto_merge_allowed=pr.auto_merge_allowed,
-        viewer_can_enable_auto_merge=pr.viewer_can_enable_auto_merge,
-        auto_merge_enabled=pr.auto_merge_enabled,
-        self_authored=viewer.lower() == pr.author.lower(),
-        merge_method=pr.default_merge_method,
-        pending_not_required=pr.pending_not_required,
-        viewer_known=bool(pr.viewer),
-        ci_outdated_query=pr.ci_outdated_query,
+        **pick_kwargs,
+        secondary_ci_verified=secondary_ci_verified,
+        secondary_ci_reason=secondary_ci_reason,
     )
+    result["needs"] = needs
+    return result
 
 
 def _reviewers(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]:

plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/disposition.py

@@ -103,6 +103,8 @@ def pick(
     pending_not_required: tuple[str, ...] | list[str] = (),
     viewer_known: bool = True,
     ci_outdated_query: bool = False,
+    secondary_ci_verified: bool = True,
+    secondary_ci_reason: str | None = None,
 ) -> dict[str, Any]:
     resolve_first = resolve_first or []
     counts = Counter(str(f.get("severity") or "").lower() for f in findings)
@@ -155,6 +157,12 @@ def pick(
         ci_action = (
             "approve or re-run the project's workflows (pr-management-triage), then re-run disposition"
         )
+    elif not secondary_ci_verified:
+        gates.append(
+            secondary_ci_reason or "secondary CI confirmation failed — no real CI check runs on head commit"
+        )
+        if not ci_action:
+            ci_action = "trigger or wait for real CI on head commit, then re-run disposition"
     if unresolved_threads:
         gates.append(f"{unresolved_threads} unresolved review thread(s)")
     if other_changes_requested:

tools/pr-management/src/pr_management/ci.py

@@ -86,6 +86,23 @@ def real_ci_ran(pr: PR, cfg: Config) -> bool:
     return any(not _is_bot_check(n) for n in names)
 
 
+def check_runs_have_real_ci(document: dict[str, Any] | list[Any], cfg: Config) -> bool:
+    """Verify that the commit's check-runs document contains at least one real CI execution."""
+    pages = document if isinstance(document, list) else [document]
+    names: list[str] = []
+    for page in pages:
+        if isinstance(page, dict):
+            for run in page.get("check_runs") or []:
+                name = run.get("name")
+                if name:
+                    names.append(str(name))
+    if not names:
+        return False
+    if cfg.real_ci_patterns:
+        return any(re.match(p, n, flags=re.IGNORECASE) for p in cfg.real_ci_patterns for n in names)
+    return any(not _is_bot_check(n) for n in names)
+
+
 def collaborator_engaged(pr: PR) -> bool:
     if any(r.association in COLLABORATOR_ASSOCIATIONS and r.author != pr.author for r in pr.reviews):
         return True

tools/pr-management/src/pr_management/code_review/cli.py

@@ -34,6 +34,7 @@
 from pathlib import Path
 from typing import Any
 
+from .. import ci as shared_ci
 from .. import config, mentions, model
 from ..triage import guards as triage_guards
 from . import body as review_body
@@ -498,28 +499,68 @@ def _decide(
         {k: v for k, v in outdated[i].items() if not k.endswith("_untrusted")}
         for i in dict.fromkeys(args.addressed_thread)
     ]
-    return disposition.pick(
+    # Shared by both picks below, so a new gate input cannot reach only one of them.
+    pick_kwargs: dict[str, Any] = {
+        "ci_state": pr.rollup_state,
+        "real_ci": selectors.real_ci(pr, cfg),
+        "unresolved_threads": pr.unresolved_threads - len(resolve_first),
+        "other_changes_requested": disposition.other_changes_requested(list(pr.reviews), viewer, pr.author),
+        "unanswered_question": args.unanswered_question,
+        "ci_diff_caused": args.ci_diff_caused,
+        "mergeable": pr.mergeable,
+        "merge_state": pr.merge_state,
+        "permission": permission,
+        "head_pushable": args.head_pushable,
+        "resolve_first": resolve_first,
+        "auto_merge_allowed": pr.auto_merge_allowed,
+        "viewer_can_enable_auto_merge": pr.viewer_can_enable_auto_merge,
+        "auto_merge_enabled": pr.auto_merge_enabled,
+        "self_authored": viewer.lower() == pr.author.lower(),
+        "merge_method": pr.default_merge_method,
+        "pending_not_required": pr.pending_not_required,
+        "viewer_known": bool(pr.viewer),
+        "ci_outdated_query": pr.ci_outdated_query,
+    }
+    # Would the disposition approve without the secondary CI check?
+    tentative = disposition.pick(findings, **pick_kwargs)
+
+    needs: list[dict[str, Any]] = []
+    secondary_ci_verified = True
+    secondary_ci_reason = None
+    if tentative["disposition"] == "APPROVE" or tentative.get("fixup", {}).get("eligible"):
+        check_runs_path = _file(args.saved_dir, f"check-runs-{pr.head_sha}.json")
+        if check_runs_path is None:
+            needs.append(
+                _need(
+                    "check-runs",
+                    [pr.head_sha],
+                    f"check-runs-{pr.head_sha}.json",
+                    "secondary confirmation of real CI on head commit",
+                )
+            )
+            # Fail closed until the read is saved: `render --auto-merge` checks the
+            # decision, not `needs`, so a missing read must not leave APPROVE standing.
+            secondary_ci_verified = False
+            secondary_ci_reason = (
+                f"check runs for head commit {pr.head_sha[:8]} are not read yet — save them, then re-run"
+            )
+        else:
+            try:
+                check_runs_data = json.loads(check_runs_path.read_text(encoding="utf-8"))
+            except (json.JSONDecodeError, OSError):
+                check_runs_data = None
+            if check_runs_data is None or not shared_ci.check_runs_have_real_ci(check_runs_data, cfg):
+                secondary_ci_verified = False
+                secondary_ci_reason = f"secondary CI confirmation failed — no real CI check runs recorded for head commit {pr.head_sha[:8]}"
+
+    result = disposition.pick(
         findings,
-        ci_state=pr.rollup_state,
-        real_ci=selectors.real_ci(pr, cfg),
-        unresolved_threads=pr.unresolved_threads - len(resolve_first),
-        other_changes_requested=disposition.other_changes_requested(list(pr.reviews), viewer, pr.author),
-        unanswered_question=args.unanswered_question,
-        ci_diff_caused=args.ci_diff_caused,
-        mergeable=pr.mergeable,
-        merge_state=pr.merge_state,
-        permission=permission,
-        head_pushable=args.head_pushable,
-        resolve_first=resolve_first,
-        auto_merge_allowed=pr.auto_merge_allowed,
-        viewer_can_enable_auto_merge=pr.viewer_can_enable_auto_merge,
-        auto_merge_enabled=pr.auto_merge_enabled,
-        self_authored=viewer.lower() == pr.author.lower(),
-        merge_method=pr.default_merge_method,
-        pending_not_required=pr.pending_not_required,
-        viewer_known=bool(pr.viewer),
-        ci_outdated_query=pr.ci_outdated_query,
+        **pick_kwargs,
+        secondary_ci_verified=secondary_ci_verified,
+        secondary_ci_reason=secondary_ci_reason,
     )
+    result["needs"] = needs
+    return result
 
 
 def _reviewers(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]:

tools/pr-management/src/pr_management/code_review/disposition.py

@@ -103,6 +103,8 @@ def pick(
     pending_not_required: tuple[str, ...] | list[str] = (),
     viewer_known: bool = True,
     ci_outdated_query: bool = False,
+    secondary_ci_verified: bool = True,
+    secondary_ci_reason: str | None = None,
 ) -> dict[str, Any]:
     resolve_first = resolve_first or []
     counts = Counter(str(f.get("severity") or "").lower() for f in findings)
@@ -155,6 +157,12 @@ def pick(
         ci_action = (
             "approve or re-run the project's workflows (pr-management-triage), then re-run disposition"
         )
+    elif not secondary_ci_verified:
+        gates.append(
+            secondary_ci_reason or "secondary CI confirmation failed — no real CI check runs on head commit"
+        )
+        if not ci_action:
+            ci_action = "trigger or wait for real CI on head commit, then re-run disposition"
     if unresolved_threads:
         gates.append(f"{unresolved_threads} unresolved review thread(s)")
     if other_changes_requested:

tools/pr-management/tests/code_review/test_cli.py

@@ -82,6 +82,10 @@ def saved(tmp_path: Path) -> Path:
     (path / "cr-codeowners-github.json").write_text(json.dumps(owners))
     (path / "team-core.txt").write_text("alice\nbob\n")
     (path / "viewer-commits.json").write_text(json.dumps([[]]))
+    # Real CI on the fixture heads, so APPROVE paths pass the secondary head check.
+    real_runs = json.dumps({"check_runs": [{"name": "Tests", "conclusion": "SUCCESS"}]})
+    for head in ("abc1234def", "0123456789abcdef0123456789abcdef01234567"):
+        (path / f"check-runs-{head}.json").write_text(real_runs)
     full = node(7, requested=["alice"], files=["scheduler/job.py"], reviews=[review("bob", "COMMENTED")])
     (path / "cr-pr-7.json").write_text(
         json.dumps(
@@ -1042,3 +1046,39 @@ def test_render_refuses_auto_merge_when_the_read_has_no_viewer(
     _without_viewer(saved)
     with pytest.raises(SystemExit, match="has no viewer"):
         _render_approve(capsys, project, saved, tmp_path, "--auto-merge", "--viewer", "alice")
+
+
+def test_render_auto_merge_refuses_without_the_head_check_runs(
+    capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path
+) -> None:
+    """`render --auto-merge` re-runs the decision, so a missing head read cannot slip past it."""
+    _with_auto_merge(saved)
+    (saved / f"check-runs-{HEAD}.json").unlink()
+    with pytest.raises(SystemExit, match="auto-merge is not available"):
+        _render_approve(capsys, project, saved, tmp_path, "--auto-merge", "--viewer", "alice")
+
+
+def test_disposition_needs_check_runs_before_approving(
+    capsys: pytest.CaptureFixture[str], project: Path, saved: Path
+) -> None:
+    _with_auto_merge(saved)
+    # Head check-runs is not saved yet: the read is asked for, and APPROVE fails closed meanwhile
+    (saved / f"check-runs-{HEAD}.json").unlink()
+    dis = _disposition(capsys, project, saved)
+    assert any(n["op"] == "check-runs" and n["params"] == [HEAD] for n in dis["needs"])
+    assert dis["disposition"] == "COMMENT"
+    assert any("not read yet" in g for g in dis["approval_gates"])
+    # If check-runs has only bot checks, APPROVE is gated
+    (saved / f"check-runs-{HEAD}.json").write_text(
+        json.dumps({"check_runs": [{"name": "boring-cyborg", "conclusion": "SUCCESS"}]})
+    )
+    dis2 = _disposition(capsys, project, saved)
+    assert dis2["disposition"] == "COMMENT"
+    assert any("secondary CI confirmation failed" in g for g in dis2["approval_gates"])
+    # If check-runs has real CI, APPROVE is unlocked
+    (saved / f"check-runs-{HEAD}.json").write_text(
+        json.dumps({"check_runs": [{"name": "Tests", "conclusion": "SUCCESS"}]})
+    )
+    dis3 = _disposition(capsys, project, saved)
+    assert dis3["disposition"] == "APPROVE"
+    assert not dis3["approval_gates"]

tools/pr-management/tests/code_review/test_decisions.py

@@ -540,3 +540,17 @@ def test_pr_with_outdated_vetted_ops_query_gates_approval_with_upgrade_action()
     assert result["disposition"] == "COMMENT"
     assert any("vetted-ops is out of date" in g for g in result["approval_gates"])
     assert result["ci_action"] == "upgrade vetted-ops to update GraphQL queries, then re-run disposition"
+
+
+def test_secondary_ci_unverified_blocks_approval() -> None:
+    # Direction 2 of #2034: secondary CI check fails or has 0 real CI checks on head commit
+    result = _pick_findings([], secondary_ci_verified=False)
+    assert result["disposition"] == "COMMENT"
+    assert any("secondary CI confirmation failed" in g for g in result["approval_gates"])
+    assert "trigger or wait for real CI on head commit" in (result["ci_action"] or "")
+
+
+def test_secondary_ci_verified_permits_approval() -> None:
+    result = _pick_findings([], secondary_ci_verified=True)
+    assert result["disposition"] == "APPROVE"
+    assert not result["approval_gates"]

tools/spec-loop/specs/pr-management-family.md

@@ -381,7 +381,9 @@ same directory; the behaviour is unchanged.
   A rollup is used only when its commit is the PR's head commit; when the
   installed vetted-ops queries do not return that commit's oid, the rollup is
   dropped, the headline says CI cannot be verified, and `ci_action` names the
-  vetted-ops upgrade.
+  vetted-ops upgrade. Before an approval is emitted, disposition performs secondary
+  confirmation of real CI on the head commit via check-runs, failing closed if no
+  real CI runs are recorded (#2034).
   The maintainer's confirmation of the disposition covers the auto-merge
   write, and the prompt names it.
   After the approval posts, the skill refreshes the liveness read, re-runs