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 176a146d1..a8a63b691 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 @@ -443,6 +443,7 @@ def _decide( 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, ) diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/data.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/data.py index c6c2ca765..34e82f591 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/data.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/data.py @@ -110,6 +110,7 @@ class PR: default_merge_method: str | None = None auto_merge_enabled: bool = False pending_not_required: tuple[str, ...] = () + ci_outdated_query: bool = False #: The last two commits with parents, verified signer and linked author logins (fixup checks). head_commits: tuple[dict[str, Any], ...] = () extra: dict[str, Any] = field(default_factory=dict) @@ -205,7 +206,24 @@ def _commit_authors(commit: dict[str, Any]) -> tuple[str, ...]: def from_node(node: dict[str, Any]) -> PR: latest = _nodes(node.get("latest")) - rollup = ((latest[-1].get("commit") if latest else None) or {}).get("statusCheckRollup") or {} + latest_commit = (latest[-1].get("commit") if latest else None) or {} + head_sha = str(node.get("headRefOid") or "").strip().lower() + commit_oid = str(latest_commit.get("oid") or "").strip().lower() + + # Golden rule 8: the CI status check rollup must strictly belong to the PR's head commit. + # If commit OID matches headRefOid, the rollup is accepted. + # If commit OID mismatches headRefOid (e.g. lagging commit edge), fail closed by discarding. + # If statusCheckRollup exists but commit OID is missing, the installed vetted-ops queries are + # out of date (older vetted-ops without `oid` under `latest`). Fail closed to protect the gate, + # but flag `ci_outdated_query` so disposition can give an actionable upgrade instruction. + ci_outdated_query = False + if head_sha and commit_oid and head_sha == commit_oid: + rollup = latest_commit.get("statusCheckRollup") or {} + elif latest_commit.get("statusCheckRollup") and not commit_oid: + rollup = {} + ci_outdated_query = True + else: + rollup = {} users, teams = _requests(node) threads = _nodes(node.get("reviewThreads")) reviews_raw = _nodes(node.get("reviews")) or _nodes(node.get("latestReviews")) @@ -263,6 +281,7 @@ def from_node(node: dict[str, Any]) -> PR: rollup_state=rollup.get("state") if rollup else None, contexts=_contexts(rollup), pending_not_required=_pending_not_required(rollup) if rollup else (), + ci_outdated_query=ci_outdated_query, head_commits=_head_commits(node), node_id=node.get("id"), thread_details=tuple(threads), 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 800924826..ad152bc0a 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 @@ -102,6 +102,7 @@ def pick( merge_method: str | None = None, pending_not_required: tuple[str, ...] | list[str] = (), viewer_known: bool = True, + ci_outdated_query: bool = False, ) -> dict[str, Any]: resolve_first = resolve_first or [] counts = Counter(str(f.get("severity") or "").lower() for f in findings) @@ -142,6 +143,9 @@ def pick( elif ci_state == "EXPECTED": gates.append("CI is waiting for workflow approval — the project's real CI has not run") ci_action = "approve the waiting workflow runs (pr-management-triage), then re-run disposition" + elif ci_outdated_query: + gates.append("CI rollup cannot be verified — vetted-ops is out of date (latest commit lacks OID)") + ci_action = "upgrade vetted-ops to update GraphQL queries, then re-run disposition" elif ci_state != "SUCCESS" and not held_by_auto_merge: gates.append(f"CI is {ci_state or 'not reported'}") if ci_state == "PENDING" and not real_ci: diff --git a/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql b/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql index 35f330103..71fa61e90 100644 --- a/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql +++ b/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql @@ -71,6 +71,7 @@ query($owner: String!, $repo: String!, $endCursor: String) { latest: commits(last: 1) { nodes { commit { + oid statusCheckRollup { state contexts(first: 50) { diff --git a/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql b/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql index 8d2a8eba0..da48007f1 100644 --- a/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql +++ b/plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql @@ -103,6 +103,7 @@ query($owner: String!, $repo: String!, $number: Int!) { latest: commits(last: 1) { nodes { commit { + oid statusCheckRollup { state contexts(first: 100) { 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 176a146d1..a8a63b691 100644 --- a/tools/pr-management/src/pr_management/code_review/cli.py +++ b/tools/pr-management/src/pr_management/code_review/cli.py @@ -443,6 +443,7 @@ def _decide( 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, ) diff --git a/tools/pr-management/src/pr_management/code_review/data.py b/tools/pr-management/src/pr_management/code_review/data.py index c6c2ca765..34e82f591 100644 --- a/tools/pr-management/src/pr_management/code_review/data.py +++ b/tools/pr-management/src/pr_management/code_review/data.py @@ -110,6 +110,7 @@ class PR: default_merge_method: str | None = None auto_merge_enabled: bool = False pending_not_required: tuple[str, ...] = () + ci_outdated_query: bool = False #: The last two commits with parents, verified signer and linked author logins (fixup checks). head_commits: tuple[dict[str, Any], ...] = () extra: dict[str, Any] = field(default_factory=dict) @@ -205,7 +206,24 @@ def _commit_authors(commit: dict[str, Any]) -> tuple[str, ...]: def from_node(node: dict[str, Any]) -> PR: latest = _nodes(node.get("latest")) - rollup = ((latest[-1].get("commit") if latest else None) or {}).get("statusCheckRollup") or {} + latest_commit = (latest[-1].get("commit") if latest else None) or {} + head_sha = str(node.get("headRefOid") or "").strip().lower() + commit_oid = str(latest_commit.get("oid") or "").strip().lower() + + # Golden rule 8: the CI status check rollup must strictly belong to the PR's head commit. + # If commit OID matches headRefOid, the rollup is accepted. + # If commit OID mismatches headRefOid (e.g. lagging commit edge), fail closed by discarding. + # If statusCheckRollup exists but commit OID is missing, the installed vetted-ops queries are + # out of date (older vetted-ops without `oid` under `latest`). Fail closed to protect the gate, + # but flag `ci_outdated_query` so disposition can give an actionable upgrade instruction. + ci_outdated_query = False + if head_sha and commit_oid and head_sha == commit_oid: + rollup = latest_commit.get("statusCheckRollup") or {} + elif latest_commit.get("statusCheckRollup") and not commit_oid: + rollup = {} + ci_outdated_query = True + else: + rollup = {} users, teams = _requests(node) threads = _nodes(node.get("reviewThreads")) reviews_raw = _nodes(node.get("reviews")) or _nodes(node.get("latestReviews")) @@ -263,6 +281,7 @@ def from_node(node: dict[str, Any]) -> PR: rollup_state=rollup.get("state") if rollup else None, contexts=_contexts(rollup), pending_not_required=_pending_not_required(rollup) if rollup else (), + ci_outdated_query=ci_outdated_query, head_commits=_head_commits(node), node_id=node.get("id"), thread_details=tuple(threads), 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 800924826..ad152bc0a 100644 --- a/tools/pr-management/src/pr_management/code_review/disposition.py +++ b/tools/pr-management/src/pr_management/code_review/disposition.py @@ -102,6 +102,7 @@ def pick( merge_method: str | None = None, pending_not_required: tuple[str, ...] | list[str] = (), viewer_known: bool = True, + ci_outdated_query: bool = False, ) -> dict[str, Any]: resolve_first = resolve_first or [] counts = Counter(str(f.get("severity") or "").lower() for f in findings) @@ -142,6 +143,9 @@ def pick( elif ci_state == "EXPECTED": gates.append("CI is waiting for workflow approval — the project's real CI has not run") ci_action = "approve the waiting workflow runs (pr-management-triage), then re-run disposition" + elif ci_outdated_query: + gates.append("CI rollup cannot be verified — vetted-ops is out of date (latest commit lacks OID)") + ci_action = "upgrade vetted-ops to update GraphQL queries, then re-run disposition" elif ci_state != "SUCCESS" and not held_by_auto_merge: gates.append(f"CI is {ci_state or 'not reported'}") if ci_state == "PENDING" and not real_ci: diff --git a/tools/pr-management/tests/code_review/builders.py b/tools/pr-management/tests/code_review/builders.py index e52338d23..ea2b423d1 100644 --- a/tools/pr-management/tests/code_review/builders.py +++ b/tools/pr-management/tests/code_review/builders.py @@ -106,9 +106,10 @@ def node( "nodes": [ { "commit": { + "oid": head, "statusCheckRollup": None if rollup is None - else {"state": rollup, "contexts": {"nodes": checks}} + else {"state": rollup, "contexts": {"nodes": checks}}, } } ] diff --git a/tools/pr-management/tests/code_review/test_data.py b/tools/pr-management/tests/code_review/test_data.py index dffd2d2d1..de014885b 100644 --- a/tools/pr-management/tests/code_review/test_data.py +++ b/tools/pr-management/tests/code_review/test_data.py @@ -41,3 +41,145 @@ def test_the_path_commits_save_name_is_one_vetted_ops_accepts(path: str) -> None def test_long_paths_that_share_a_stem_get_different_names() -> None: assert data.slug(LONG) != data.slug(LONG.replace("state.py", "other.py")) + + +def test_from_node_accepts_rollup_when_commit_oid_matches_head_ref_oid() -> None: + node = { + "number": 1, + "headRefOid": "3ac12395", + "latest": { + "nodes": [ + { + "commit": { + "oid": "3ac12395", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "build", + "conclusion": "SUCCESS", + "status": "COMPLETED", + } + ] + }, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.head_sha == "3ac12395" + assert pr.rollup_state == "SUCCESS" + assert len(pr.contexts) == 1 + assert pr.contexts[0].name == "build" + assert pr.contexts[0].conclusion == "SUCCESS" + + +def test_from_node_discards_rollup_when_commit_oid_mismatches_head_ref_oid() -> None: + # Guards against a lagging commit edge where latest: commits(last: 1) + # has not advanced to headRefOid (Direction 1 of #2034). + node = { + "number": 272, + "headRefOid": "3ac12395", + "latest": { + "nodes": [ + { + "commit": { + "oid": "4eb3567c", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "build", + "conclusion": "SUCCESS", + "status": "COMPLETED", + } + ] + }, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.head_sha == "3ac12395" + assert pr.rollup_state is None + assert pr.contexts == () + assert pr.pending_not_required == () + assert pr.ci_outdated_query is False + + +def test_from_node_discards_rollup_and_flags_outdated_query_when_commit_oid_missing() -> None: + # Simulates query skew: an older vetted-ops installation that did not request `oid` + # under latest: commits(last: 1). Rollup is discarded for safety, but flagged as outdated. + node = { + "number": 1, + "headRefOid": "3ac12395", + "latest": { + "nodes": [ + { + "commit": { + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": {"nodes": []}, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.rollup_state is None + assert pr.contexts == () + assert pr.ci_outdated_query is True + + +def test_from_node_discards_rollup_when_head_ref_oid_missing() -> None: + node = { + "number": 1, + "latest": { + "nodes": [ + { + "commit": { + "oid": "3ac12395", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": {"nodes": []}, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.rollup_state is None + assert pr.contexts == () + + +def test_from_node_accepts_rollup_case_insensitively() -> None: + node = { + "number": 1, + "headRefOid": "3AC12395", + "latest": { + "nodes": [ + { + "commit": { + "oid": " 3ac12395 ", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": {"nodes": []}, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.head_sha == "3AC12395" + assert pr.rollup_state == "SUCCESS" diff --git a/tools/pr-management/tests/code_review/test_decisions.py b/tools/pr-management/tests/code_review/test_decisions.py index d1627729f..ed6a648d7 100644 --- a/tools/pr-management/tests/code_review/test_decisions.py +++ b/tools/pr-management/tests/code_review/test_decisions.py @@ -28,7 +28,7 @@ import pytest -from pr_management.code_review import codeowners, deps, disposition, reviewers +from pr_management.code_review import codeowners, data, deps, disposition, reviewers from .builders import NOW, ago, review from .builders import pr as build_pr @@ -468,3 +468,75 @@ def test_markers_split_environments() -> None: } ) assert result["evidence"] == {"uninstallable_in": ["py39"]} + + +def test_pr_with_lagging_commit_edge_fails_closed_to_comment() -> None: + # Direction 1 of #2034: latest commit node lags behind headRefOid (4eb3567c != 3ac12395). + node = { + "number": 272, + "headRefOid": "3ac12395", + "latest": { + "nodes": [ + { + "commit": { + "oid": "4eb3567c", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "tests", + "conclusion": "SUCCESS", + "status": "COMPLETED", + } + ] + }, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.rollup_state is None + # With clean findings, APPROVE is blocked because CI is unverified (fail-closed Golden rule 8). + result = _pick_findings([], ci_state=pr.rollup_state) + assert result["disposition"] == "COMMENT" + assert "CI is not reported" in result["reason"] + + +def test_pr_with_outdated_vetted_ops_query_gates_approval_with_upgrade_action() -> None: + # Query skew: older vetted-ops without `oid` under `latest: commits(last: 1)`. + node = { + "number": 272, + "headRefOid": "3ac12395", + "latest": { + "nodes": [ + { + "commit": { + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "tests", + "conclusion": "SUCCESS", + "status": "COMPLETED", + } + ] + }, + }, + } + } + ] + }, + } + pr = data.from_node(node) + assert pr.rollup_state is None + assert pr.ci_outdated_query is True + result = _pick_findings([], ci_state=pr.rollup_state, ci_outdated_query=pr.ci_outdated_query) + 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" diff --git a/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql b/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql index 35f330103..71fa61e90 100644 --- a/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql +++ b/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql @@ -71,6 +71,7 @@ query($owner: String!, $repo: String!, $endCursor: String) { latest: commits(last: 1) { nodes { commit { + oid statusCheckRollup { state contexts(first: 50) { diff --git a/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql b/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql index 8d2a8eba0..da48007f1 100644 --- a/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql +++ b/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql @@ -103,6 +103,7 @@ query($owner: String!, $repo: String!, $number: Int!) { latest: commits(last: 1) { nodes { commit { + oid statusCheckRollup { state contexts(first: 100) {