Skip to content

Commit 502298e

Browse files
PLeVasseurPLeVasseur
authored
fix(reviewer-bot): harden stage2 readiness closure gates (#554)
* fix(reviewer-bot): harden stage2 readiness closure gates * test(reviewer-bot): cover tracked workflow-run noop paths
1 parent c158011 commit 502298e

14 files changed

Lines changed: 839 additions & 3 deletions

‎scripts/reviewer_bot_lib/reconcile.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -593,10 +593,10 @@ def _build_result(state_changed: bool, pr_number: int) -> WorkflowRunHandlerResu
593593
pr_number = parsed_payload.pr_number
594594
if pr_number <= 0:
595595
raise RuntimeError("Deferred context is missing a valid PR number")
596-
bot.collect_touched_item(pr_number)
597-
review_data = ensure_review_entry(state, pr_number, create=True)
596+
review_data = ensure_review_entry(state, pr_number)
598597
if review_data is None:
599-
raise RuntimeError(f"No review entry available for PR #{pr_number}")
598+
raise RuntimeError(f"No active review entry available for PR #{pr_number}")
599+
bot.collect_touched_item(pr_number)
600600
try:
601601
handler = _workflow_run_handler_for_payload(parsed_payload)
602602
if handler is None:

‎tests/contract/reviewer_bot/test_stage2_closure_artifacts.py‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
import json
2+
from pathlib import Path
3+
4+
import pytest
5+
6+
pytestmark = pytest.mark.contract
7+
8+
9+
def _load_fixture(name: str) -> dict:
10+
return json.loads(Path("tests/fixtures/workflow_contracts", name).read_text(encoding="utf-8"))
11+
12+
13+
def _transition_notice_gate_ready(payload: dict, expected_ref: str) -> bool:
14+
return (
15+
payload["artifact_id"] == "transition-notice-fallback-closure"
16+
and payload["evaluated_repo"] == "rustfoundation/safety-critical-rust-coding-guidelines"
17+
and payload["evaluated_ref"] == expected_ref
18+
and payload["closure_ready"] is True
19+
and payload["remaining_transition_due_without_notice"] == []
20+
)
21+
22+
23+
def _deferred_payload_gate_ready(payload: dict, expected_ref: str) -> bool:
24+
return (
25+
payload["artifact_id"] == "deferred-payload-legacy-closure"
26+
and payload["evaluated_repo"] == "rustfoundation/safety-critical-rust-coding-guidelines"
27+
and payload["evaluated_ref"] == expected_ref
28+
and payload["closure_ready"] is True
29+
and payload["retained_workflow_inventory_matches"] is True
30+
and payload["blocking_workflows"] == []
31+
and payload["queued_or_in_progress_runs"] == []
32+
and payload["legacy_artifacts_remaining"] == []
33+
)
34+
35+
36+
def _stage2_closure_cluster_gate(
37+
transition_notice_payload: dict,
38+
deferred_payload: dict,
39+
*,
40+
expected_ref: str,
41+
) -> bool:
42+
return _transition_notice_gate_ready(transition_notice_payload, expected_ref) and _deferred_payload_gate_ready(
43+
deferred_payload, expected_ref
44+
)
45+
46+
47+
@pytest.mark.parametrize(
48+
("fixture_name", "expected_ready"),
49+
[
50+
("stage2_transition_notice_fallback_closure_green.json", True),
51+
("stage2_transition_notice_fallback_closure_blocked.json", False),
52+
],
53+
)
54+
def test_transition_notice_closure_fixture_schema_and_gate_rule(fixture_name, expected_ready):
55+
payload = _load_fixture(fixture_name)
56+
57+
assert set(payload) == {
58+
"artifact_id",
59+
"generated_at",
60+
"evaluated_repo",
61+
"evaluated_ref",
62+
"state_issue_number",
63+
"closure_ready",
64+
"active_reviews_scanned",
65+
"resolved_by_marker_backfill",
66+
"resolved_by_legacy_prose_backfill",
67+
"resolved_by_new_marker_notice",
68+
"remaining_transition_due_without_notice",
69+
"commands_run",
70+
}
71+
assert payload["closure_ready"] is expected_ready
72+
assert payload["closure_ready"] == (payload["remaining_transition_due_without_notice"] == [])
73+
74+
75+
@pytest.mark.parametrize(
76+
("fixture_name", "expected_ready"),
77+
[
78+
("stage2_deferred_payload_legacy_closure_green.json", True),
79+
("stage2_deferred_payload_legacy_closure_blocked.json", False),
80+
],
81+
)
82+
def test_deferred_payload_closure_fixture_schema_and_gate_rule(fixture_name, expected_ready):
83+
payload = _load_fixture(fixture_name)
84+
85+
assert set(payload) == {
86+
"artifact_id",
87+
"generated_at",
88+
"evaluated_repo",
89+
"evaluated_ref",
90+
"closure_ready",
91+
"retained_workflow_inventory_matches",
92+
"blocking_workflows",
93+
"queued_or_in_progress_runs",
94+
"legacy_artifacts_remaining",
95+
"control_plane_actions_applied",
96+
"commands_run",
97+
}
98+
assert payload["closure_ready"] is expected_ready
99+
assert payload["closure_ready"] == (
100+
payload["retained_workflow_inventory_matches"]
101+
and payload["blocking_workflows"] == []
102+
and payload["queued_or_in_progress_runs"] == []
103+
and payload["legacy_artifacts_remaining"] == []
104+
)
105+
106+
107+
def test_stage2_closure_artifacts_reject_stale_attempt_refs():
108+
transition_notice_payload = _load_fixture("stage2_transition_notice_fallback_closure_green.json")
109+
deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_green.json")
110+
111+
assert _transition_notice_gate_ready(
112+
transition_notice_payload,
113+
"0000000000000000000000000000000000000000",
114+
) is False
115+
assert _deferred_payload_gate_ready(
116+
deferred_payload,
117+
"0000000000000000000000000000000000000000",
118+
) is False
119+
120+
121+
def test_stage2_closure_cluster_gate_requires_both_current_attempt_green_artifacts():
122+
transition_notice_payload = _load_fixture("stage2_transition_notice_fallback_closure_green.json")
123+
deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_green.json")
124+
blocked_transition_notice_payload = _load_fixture(
125+
"stage2_transition_notice_fallback_closure_blocked.json"
126+
)
127+
blocked_deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_blocked.json")
128+
expected_ref = transition_notice_payload["evaluated_ref"]
129+
130+
assert _stage2_closure_cluster_gate(
131+
transition_notice_payload,
132+
deferred_payload,
133+
expected_ref=expected_ref,
134+
) is True
135+
assert _stage2_closure_cluster_gate(
136+
blocked_transition_notice_payload,
137+
deferred_payload,
138+
expected_ref=expected_ref,
139+
) is False
140+
assert _stage2_closure_cluster_gate(
141+
transition_notice_payload,
142+
blocked_deferred_payload,
143+
expected_ref=expected_ref,
144+
) is False

‎tests/contract/reviewer_bot/test_stage2_deferred_payload_residue.py‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import json
2+
from pathlib import Path
3+
4+
import pytest
5+
6+
pytestmark = pytest.mark.contract
7+
8+
9+
def _load_cases() -> dict:
10+
return json.loads(
11+
Path("tests/fixtures/workflow_contracts/deferred_payload_residue_cases.json").read_text(
12+
encoding="utf-8"
13+
)
14+
)
15+
16+
17+
def _simulate_residue_audit(case: dict) -> dict:
18+
blocking_workflows = []
19+
queued_or_in_progress_runs = []
20+
legacy_artifacts_remaining = []
21+
evaluated_ref = case["evaluated_ref"]
22+
23+
for workflow in case["workflows"]:
24+
if workflow["classification"] == "required_retained" and workflow["state"] != "active":
25+
blocking_workflows.append(
26+
{
27+
"workflow_name": workflow["workflow_name"],
28+
"workflow_path": workflow["workflow_path"],
29+
"reason": "required_retained_workflow_disabled",
30+
}
31+
)
32+
if workflow["classification"] == "removed_legacy" and workflow["state"] == "active":
33+
blocking_workflows.append(
34+
{
35+
"workflow_name": workflow["workflow_name"],
36+
"workflow_path": workflow["workflow_path"],
37+
"reason": "removed_legacy_workflow_still_active",
38+
}
39+
)
40+
41+
for run in case["runs"]:
42+
if run["status"] not in {"queued", "in_progress"}:
43+
continue
44+
if run["classification"] == "removed_legacy":
45+
reason = "removed_legacy_workflow_run"
46+
elif run["head_sha"] != evaluated_ref:
47+
reason = "noncurrent_head_sha"
48+
else:
49+
continue
50+
queued_or_in_progress_runs.append(
51+
{
52+
"workflow_name": run["workflow_name"],
53+
"workflow_path": run["workflow_path"],
54+
"run_id": run["run_id"],
55+
"run_attempt": run["run_attempt"],
56+
"status": run["status"],
57+
"head_sha": run["head_sha"],
58+
"reason": reason,
59+
}
60+
)
61+
62+
for artifact in case["artifacts"]:
63+
if artifact["downloadable"] and artifact["payload_schema_version"] in {1, 2}:
64+
legacy_artifacts_remaining.append(
65+
{
66+
"workflow_name": artifact["workflow_name"],
67+
"workflow_path": artifact["workflow_path"],
68+
"run_id": artifact["run_id"],
69+
"run_attempt": artifact["run_attempt"],
70+
"artifact_id": artifact["artifact_id"],
71+
"payload_schema_version": artifact["payload_schema_version"],
72+
"payload_kind": artifact["payload_kind"],
73+
"artifact_name": artifact["artifact_name"],
74+
"payload_filename": artifact["payload_filename"],
75+
}
76+
)
77+
78+
return {
79+
"retained_workflow_inventory_matches": case["retained_workflow_inventory_matches"],
80+
"blocking_workflows": blocking_workflows,
81+
"queued_or_in_progress_runs": queued_or_in_progress_runs,
82+
"legacy_artifacts_remaining": legacy_artifacts_remaining,
83+
"closure_ready": case["retained_workflow_inventory_matches"]
84+
and blocking_workflows == []
85+
and queued_or_in_progress_runs == []
86+
and legacy_artifacts_remaining == [],
87+
}
88+
89+
90+
def _select_deferred_payload(files: list[str]) -> str | None:
91+
json_files = sorted(path for path in files if path.endswith(".json"))
92+
if len(json_files) > 1:
93+
raise RuntimeError(f"Expected at most one deferred payload, found {len(json_files)}")
94+
if len(json_files) == 1:
95+
return json_files[0]
96+
return None
97+
98+
99+
@pytest.mark.parametrize("case", _load_cases()["residue_cases"], ids=lambda case: case["id"])
100+
def test_b5e_residue_cases_are_simulated_locally(case):
101+
simulated = _simulate_residue_audit(case)
102+
103+
assert simulated == case["expected"]
104+
105+
106+
@pytest.mark.parametrize(
107+
"case",
108+
_load_cases()["artifact_selection_cases"],
109+
ids=lambda case: case["id"],
110+
)
111+
def test_reconcile_artifact_selection_cases_fail_closed(case):
112+
if case["expected_multiple"]:
113+
with pytest.raises(RuntimeError, match="Expected at most one deferred payload"):
114+
_select_deferred_payload(case["files"])
115+
return
116+
117+
assert _select_deferred_payload(case["files"]) == case["expected_selected_path"]
118+
119+
120+
def test_router_zero_artifact_success_is_not_classified_as_residue():
121+
case = next(case for case in _load_cases()["residue_cases"] if case["id"] == "router_zero_artifact_success")
122+
123+
simulated = _simulate_residue_audit(case)
124+
125+
assert simulated["blocking_workflows"] == []
126+
assert simulated["queued_or_in_progress_runs"] == []
127+
assert simulated["legacy_artifacts_remaining"] == []
128+
assert simulated["closure_ready"] is True

‎tests/contract/reviewer_bot/test_workflow_artifact_contracts.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from types import SimpleNamespace
44

55
import pytest
6+
import yaml
67

78
pytestmark = pytest.mark.contract
89

@@ -14,6 +15,12 @@ def _load_fixture_payload(relative_path: str) -> dict:
1415
return data["payload"]
1516

1617

18+
def _load_workflow_job(relative_path: str) -> dict:
19+
workflow = yaml.safe_load(Path(relative_path).read_text(encoding="utf-8"))
20+
job_name = "route-pr-comment" if relative_path.endswith("reviewer-bot-pr-comment-router.yml") else "observer"
21+
return workflow["jobs"][job_name]
22+
23+
1724
@pytest.mark.parametrize(
1825
("workflow_path",),
1926
[
@@ -77,6 +84,49 @@ def test_deferred_comment_payload_parses_without_artifact_name_field():
7784
assert parsed.identity.source_event_name == "issue_comment"
7885

7986

87+
@pytest.mark.parametrize(
88+
("fixture_path", "workflow_path"),
89+
[
90+
(
91+
"tests/fixtures/observer_payloads/workflow_pr_comment_deferred.json",
92+
".github/workflows/reviewer-bot-pr-comment-router.yml",
93+
),
94+
(
95+
"tests/fixtures/observer_payloads/workflow_pr_review_submitted_deferred.json",
96+
".github/workflows/reviewer-bot-pr-review-submitted-observer.yml",
97+
),
98+
(
99+
"tests/fixtures/observer_payloads/workflow_pr_review_dismissed_deferred.json",
100+
".github/workflows/reviewer-bot-pr-review-dismissed-observer.yml",
101+
),
102+
(
103+
"tests/fixtures/observer_payloads/workflow_pr_review_comment_deferred.json",
104+
".github/workflows/reviewer-bot-pr-review-comment-observer.yml",
105+
),
106+
],
107+
)
108+
def test_deferred_payload_fixtures_match_upload_name_and_payload_name_helpers(
109+
fixture_path, workflow_path
110+
):
111+
payload = _load_fixture_payload(fixture_path)
112+
job = _load_workflow_job(workflow_path)
113+
build_step = job["steps"][0]
114+
upload_step = job["steps"][1]
115+
rendered_upload_name = (
116+
upload_step["with"]["name"]
117+
.replace("${{ github.run_id }}", str(payload["source_run_id"]))
118+
.replace("${{ github.run_attempt }}", str(payload["source_run_attempt"]))
119+
)
120+
121+
assert rendered_upload_name == reconcile_payloads.artifact_expected_name(payload)
122+
assert build_step["env"]["PAYLOAD_PATH"].endswith(
123+
reconcile_payloads.artifact_expected_payload_name(payload)
124+
)
125+
assert upload_step["with"]["path"].endswith(
126+
reconcile_payloads.artifact_expected_payload_name(payload)
127+
)
128+
129+
80130
def test_validate_workflow_run_artifact_identity_rejects_run_attempt_mismatch(monkeypatch):
81131
monkeypatch.setenv("WORKFLOW_RUN_TRIGGERING_ID", "1")
82132
monkeypatch.setenv("WORKFLOW_RUN_TRIGGERING_ATTEMPT", "2")

‎tests/contract/reviewer_bot/test_workflow_files.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,16 @@ def test_workflow_summaries_and_runbook_references_exist():
138138
assert "docs/reviewer-bot-review-freshness-operator-runbook.md" in reconcile
139139

140140

141+
def test_reconcile_workflow_selects_at_most_one_recursive_json_payload():
142+
workflow_text = Path(".github/workflows/reviewer-bot-reconcile.yml").read_text(encoding="utf-8")
143+
144+
assert "files = sorted(Path(os.environ['RUNNER_TEMP']).joinpath('observer-artifact').rglob('*.json'))" in workflow_text
145+
assert "if len(files) > 1:" in workflow_text
146+
assert "Expected at most one deferred payload" in workflow_text
147+
assert "if len(files) == 1:" in workflow_text
148+
assert "DEFERRED_CONTEXT_PATH=" in workflow_text
149+
150+
141151
@pytest.mark.parametrize(
142152
("fixture_path", "workflow_file", "payload_kind", "expected_event_name", "expected_event_action"),
143153
[

0 commit comments

Comments
 (0)