From 850fa167b384e8f6aaca4371a3a22c663798fbc2 Mon Sep 17 00:00:00 2001 From: Vardhman Gupta Date: Sat, 14 Nov 2026 09:21:11 +0530 Subject: [PATCH 1/2] feat(code-review): secondary confirmation of real CI on head commit (part 2 of #2034) --- .../skills/code-review/review-flow.md | 2 +- .../pr-management/src/pr_management/ci.py | 17 ++++++ .../src/pr_management/code_review/cli.py | 54 ++++++++++++++++++- .../pr_management/code_review/disposition.py | 8 +++ tools/pr-management/src/pr_management/ci.py | 17 ++++++ .../src/pr_management/code_review/cli.py | 54 ++++++++++++++++++- .../pr_management/code_review/disposition.py | 8 +++ .../tests/code_review/test_cli.py | 23 ++++++++ .../tests/code_review/test_decisions.py | 14 +++++ tools/spec-loop/specs/pr-management-family.md | 4 +- 10 files changed, 197 insertions(+), 4 deletions(-) diff --git a/plugins/magpie-pr-management/skills/code-review/review-flow.md b/plugins/magpie-pr-management/skills/code-review/review-flow.md index 8d4f64423..dade762f6 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/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 (``); 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. --- diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/ci.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/ci.py index ae0786e9e..f48b7a5a3 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/ci.py +++ b/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 diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py index f486509af..8f8fe13f0 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py +++ b/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,7 +499,8 @@ 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( + # Check if disposition without secondary CI would be eligible for approval + tentative = disposition.pick( findings, ci_state=pr.rollup_state, real_ci=selectors.real_ci(pr, cfg), @@ -521,6 +523,56 @@ def _decide( ci_outdated_query=pr.ci_outdated_query, ) + 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", + ) + ) + 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, + 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]: saved: Path = args.saved_dir diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/disposition.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/disposition.py index ad152bc0a..11b87e283 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/disposition.py +++ b/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: diff --git a/tools/pr-management/src/pr_management/ci.py b/tools/pr-management/src/pr_management/ci.py index ae0786e9e..f48b7a5a3 100644 --- a/tools/pr-management/src/pr_management/ci.py +++ b/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 diff --git a/tools/pr-management/src/pr_management/code_review/cli.py b/tools/pr-management/src/pr_management/code_review/cli.py index f486509af..8f8fe13f0 100644 --- a/tools/pr-management/src/pr_management/code_review/cli.py +++ b/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,7 +499,8 @@ 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( + # Check if disposition without secondary CI would be eligible for approval + tentative = disposition.pick( findings, ci_state=pr.rollup_state, real_ci=selectors.real_ci(pr, cfg), @@ -521,6 +523,56 @@ def _decide( ci_outdated_query=pr.ci_outdated_query, ) + 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", + ) + ) + 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, + 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]: saved: Path = args.saved_dir diff --git a/tools/pr-management/src/pr_management/code_review/disposition.py b/tools/pr-management/src/pr_management/code_review/disposition.py index ad152bc0a..11b87e283 100644 --- a/tools/pr-management/src/pr_management/code_review/disposition.py +++ b/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: diff --git a/tools/pr-management/tests/code_review/test_cli.py b/tools/pr-management/tests/code_review/test_cli.py index e0452dde4..3c187341c 100644 --- a/tools/pr-management/tests/code_review/test_cli.py +++ b/tools/pr-management/tests/code_review/test_cli.py @@ -1042,3 +1042,26 @@ 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_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 + dis = _disposition(capsys, project, saved) + assert any(n["op"] == "check-runs" and n["params"] == [HEAD] for n in dis["needs"]) + # 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"] diff --git a/tools/pr-management/tests/code_review/test_decisions.py b/tools/pr-management/tests/code_review/test_decisions.py index ed6a648d7..c019d9737 100644 --- a/tools/pr-management/tests/code_review/test_decisions.py +++ b/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"] diff --git a/tools/spec-loop/specs/pr-management-family.md b/tools/spec-loop/specs/pr-management-family.md index 86cf5ac07..4bb145325 100644 --- a/tools/spec-loop/specs/pr-management-family.md +++ b/tools/spec-loop/specs/pr-management-family.md @@ -372,7 +372,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 From 171b49294f197897f348c68c6aea74610deed38e Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 14 Nov 2026 13:03:21 +0900 Subject: [PATCH 2/2] fix(code-review): fail closed until the head check runs are read, and grant the read - code_review/cli.py (+ vendored copy): with no check-runs read saved for the head, the decision asks for it and gates APPROVE meanwhile; render --auto-merge checks the decision, not needs, so a missing read could approve and auto-merge unchecked. The two pick() calls share one kwargs dict. - vetted-ops policy: grant check-runs to pr-management-code-review. - vetted-command-surface.md: record the grant. - Tests: the fixture saves real-CI check runs for its heads; render --auto-merge refuses without the read; the needs test asserts the gate. Generated-by: Claude Opus 5 --- .../tools/vetted-ops/config.toml | 3 + .../src/pr_management/code_review/cli.py | 73 ++++++++----------- .../src/pr_management/code_review/cli.py | 73 ++++++++----------- .../tests/code_review/test_cli.py | 19 ++++- .../spec-loop/specs/vetted-command-surface.md | 3 + 5 files changed, 86 insertions(+), 85 deletions(-) diff --git a/.apache-magpie-overrides/tools/vetted-ops/config.toml b/.apache-magpie-overrides/tools/vetted-ops/config.toml index cf95b0ed8..c76891163 100644 --- a/.apache-magpie-overrides/tools/vetted-ops/config.toml +++ b/.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`, diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py index 8f8fe13f0..c55260dc1 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py @@ -499,29 +499,30 @@ def _decide( {k: v for k, v in outdated[i].items() if not k.endswith("_untrusted")} for i in dict.fromkeys(args.addressed_thread) ] - # Check if disposition without secondary CI would be eligible for approval - tentative = 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, - ) + # 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 @@ -537,6 +538,12 @@ def _decide( "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")) @@ -548,25 +555,7 @@ def _decide( 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, ) diff --git a/tools/pr-management/src/pr_management/code_review/cli.py b/tools/pr-management/src/pr_management/code_review/cli.py index 8f8fe13f0..c55260dc1 100644 --- a/tools/pr-management/src/pr_management/code_review/cli.py +++ b/tools/pr-management/src/pr_management/code_review/cli.py @@ -499,29 +499,30 @@ def _decide( {k: v for k, v in outdated[i].items() if not k.endswith("_untrusted")} for i in dict.fromkeys(args.addressed_thread) ] - # Check if disposition without secondary CI would be eligible for approval - tentative = 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, - ) + # 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 @@ -537,6 +538,12 @@ def _decide( "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")) @@ -548,25 +555,7 @@ def _decide( 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, ) diff --git a/tools/pr-management/tests/code_review/test_cli.py b/tools/pr-management/tests/code_review/test_cli.py index 3c187341c..898dc0d19 100644 --- a/tools/pr-management/tests/code_review/test_cli.py +++ b/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( @@ -1044,13 +1048,26 @@ def test_render_refuses_auto_merge_when_the_read_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 + # 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"}]}) diff --git a/tools/spec-loop/specs/vetted-command-surface.md b/tools/spec-loop/specs/vetted-command-surface.md index 246de36c7..0e5a155c7 100644 --- a/tools/spec-loop/specs/vetted-command-surface.md +++ b/tools/spec-loop/specs/vetted-command-surface.md @@ -282,6 +282,9 @@ per [`docs/adapters/registry.md`](../../../docs/adapters/registry.md). `pr-management-code-review`'s auto-merge path added `run-rerun`, for a run held in `action_required` that `workflow-approve` answers is not waiting for approval (#1951). + It also reads `check-runs` for the head commit before proposing an + approval, the secondary confirmation that real CI ran there (#2072); the + read is the triage one, granted to `pr-management-code-review` as well. `pr-management-triage`'s recovery journal added two read-only operations, `pr-triage-recovery-state` (the PR fields a triage write changes, read before each write) and `pr-triage-comment-receipt` (one issue comment by