Skip to content

fix(pr-management): verify CI status check rollup belongs to PR head (part 1 of #2034) #2061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kaap10 wants to merge 2 commits into
base: main
Choose a base branch
from
+271 −4
Open

fix(pr-management): verify CI status check rollup belongs to PR head (part 1 of #2034) #2061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91c1829
fix(pr-management): verify CI status check rollup belongs to PR head …
Kaap10 Nov 13, 2026
cd9cf50
fix(pr-management): handle query skew on missing OID and clarify test…
Kaap10 Nov 13, 2026
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
1 change: 1 addition & 0 deletions plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)


Expand Down
21 changes: 20 additions & 1 deletion plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"))
Expand Down Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions ...ns/magpie-security-tools/tools/pr-management/src/pr_management/code_review/disposition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-open.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ query($owner: String!, $repo: String!, $endCursor: String) {
latest: commits(last: 1) {
nodes {
commit {
oid
statusCheckRollup {
state
contexts(first: 50) {
Expand Down
1 change: 1 addition & 0 deletions plugins/magpie-vetted-ops/tools/vetted-ops/src/vetted_ops/queries/cr-pr.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ query($owner: String!, $repo: String!, $number: Int!) {
latest: commits(last: 1) {
nodes {
commit {
oid
statusCheckRollup {
state
contexts(first: 100) {
Expand Down
1 change: 1 addition & 0 deletions tools/pr-management/src/pr_management/code_review/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

@shahar1 shahar1 Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainers are immediately informed

A missing OID fails closed to prevent unverified approvals, but surfaces an explicit "vetted-ops is out of date" approval gate and ci_action so maintainers are immediately informed.

ci_outdated_query reaches only disposition (Step 6). Under the same skew, context still prints CI: none reported (_ci_line reads only rollup_state) and returns "ci": {"state": null, "real_ci": false}, and queue ranks every PR as real-CI-never-ran with no announcement. The maintainer reads the wrong headline for the whole review and learns the cause at the last step. Have _ci_line return something like CI: cannot be verified (vetted-ops query out of date) when the flag is set, and carry the flag in the ci object; a queue announcement can follow in part 2.

)


Expand Down
21 changes: 20 additions & 1 deletion tools/pr-management/src/pr_management/code_review/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 = {}
Comment thread
shahar1 marked this conversation as resolved.
Resolved
users, teams = _requests(node)
threads = _nodes(node.get("reviewThreads"))
reviews_raw = _nodes(node.get("reviews")) or _nodes(node.get("latestReviews"))
Expand Down Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions tools/pr-management/src/pr_management/code_review/disposition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"

@shahar1 shahar1 Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec-sync pre-check before pushing a functionality PR

Before pushing a PR that ships or changes a skill, tool, or mode — and before pushing a rebase of one onto a main that has moved — confirm tools/spec-loop/.last-sync is at (or near) the current main tip and that the affected specs reflect what actually shipped.

This adds a CI gate and a third meaning of ci_action, and two texts still describe the old set. tools/spec-loop/specs/pr-management-family.md lists the gates as "Failing CI, a rollup green from bot checks only, and workflows waiting for approval". plugins/magpie-pr-management/skills/code-review/review-flow.md (Step 6) tells the agent "When ci_action is set, the project's workflows have not run (waiting for approval, or bot checks only)", which is now wrong for the upgrade case: the agent would point the maintainer at workflow approval. Add the head-commit rule and the out-of-date-query gate to both.

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:
Expand Down
3 changes: 2 additions & 1 deletion tools/pr-management/tests/code_review/builders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}},
}
}
]
Expand Down
142 changes: 142 additions & 0 deletions tools/pr-management/tests/code_review/test_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
74 changes: 73 additions & 1 deletion tools/pr-management/tests/code_review/test_decisions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Loading
Loading