diff --git a/scripts/reviewer_bot_lib/reconcile.py b/scripts/reviewer_bot_lib/reconcile.py index 0dec535ee..80eb79aa3 100644 --- a/scripts/reviewer_bot_lib/reconcile.py +++ b/scripts/reviewer_bot_lib/reconcile.py @@ -593,10 +593,10 @@ def _build_result(state_changed: bool, pr_number: int) -> WorkflowRunHandlerResu pr_number = parsed_payload.pr_number if pr_number <= 0: raise RuntimeError("Deferred context is missing a valid PR number") - bot.collect_touched_item(pr_number) - review_data = ensure_review_entry(state, pr_number, create=True) + review_data = ensure_review_entry(state, pr_number) if review_data is None: - raise RuntimeError(f"No review entry available for PR #{pr_number}") + raise RuntimeError(f"No active review entry available for PR #{pr_number}") + bot.collect_touched_item(pr_number) try: handler = _workflow_run_handler_for_payload(parsed_payload) if handler is None: diff --git a/tests/contract/reviewer_bot/test_stage2_closure_artifacts.py b/tests/contract/reviewer_bot/test_stage2_closure_artifacts.py new file mode 100644 index 000000000..f7268ba6c --- /dev/null +++ b/tests/contract/reviewer_bot/test_stage2_closure_artifacts.py @@ -0,0 +1,144 @@ +import json +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.contract + + +def _load_fixture(name: str) -> dict: + return json.loads(Path("tests/fixtures/workflow_contracts", name).read_text(encoding="utf-8")) + + +def _transition_notice_gate_ready(payload: dict, expected_ref: str) -> bool: + return ( + payload["artifact_id"] == "transition-notice-fallback-closure" + and payload["evaluated_repo"] == "rustfoundation/safety-critical-rust-coding-guidelines" + and payload["evaluated_ref"] == expected_ref + and payload["closure_ready"] is True + and payload["remaining_transition_due_without_notice"] == [] + ) + + +def _deferred_payload_gate_ready(payload: dict, expected_ref: str) -> bool: + return ( + payload["artifact_id"] == "deferred-payload-legacy-closure" + and payload["evaluated_repo"] == "rustfoundation/safety-critical-rust-coding-guidelines" + and payload["evaluated_ref"] == expected_ref + and payload["closure_ready"] is True + and payload["retained_workflow_inventory_matches"] is True + and payload["blocking_workflows"] == [] + and payload["queued_or_in_progress_runs"] == [] + and payload["legacy_artifacts_remaining"] == [] + ) + + +def _stage2_closure_cluster_gate( + transition_notice_payload: dict, + deferred_payload: dict, + *, + expected_ref: str, +) -> bool: + return _transition_notice_gate_ready(transition_notice_payload, expected_ref) and _deferred_payload_gate_ready( + deferred_payload, expected_ref + ) + + +@pytest.mark.parametrize( + ("fixture_name", "expected_ready"), + [ + ("stage2_transition_notice_fallback_closure_green.json", True), + ("stage2_transition_notice_fallback_closure_blocked.json", False), + ], +) +def test_transition_notice_closure_fixture_schema_and_gate_rule(fixture_name, expected_ready): + payload = _load_fixture(fixture_name) + + assert set(payload) == { + "artifact_id", + "generated_at", + "evaluated_repo", + "evaluated_ref", + "state_issue_number", + "closure_ready", + "active_reviews_scanned", + "resolved_by_marker_backfill", + "resolved_by_legacy_prose_backfill", + "resolved_by_new_marker_notice", + "remaining_transition_due_without_notice", + "commands_run", + } + assert payload["closure_ready"] is expected_ready + assert payload["closure_ready"] == (payload["remaining_transition_due_without_notice"] == []) + + +@pytest.mark.parametrize( + ("fixture_name", "expected_ready"), + [ + ("stage2_deferred_payload_legacy_closure_green.json", True), + ("stage2_deferred_payload_legacy_closure_blocked.json", False), + ], +) +def test_deferred_payload_closure_fixture_schema_and_gate_rule(fixture_name, expected_ready): + payload = _load_fixture(fixture_name) + + assert set(payload) == { + "artifact_id", + "generated_at", + "evaluated_repo", + "evaluated_ref", + "closure_ready", + "retained_workflow_inventory_matches", + "blocking_workflows", + "queued_or_in_progress_runs", + "legacy_artifacts_remaining", + "control_plane_actions_applied", + "commands_run", + } + assert payload["closure_ready"] is expected_ready + assert payload["closure_ready"] == ( + payload["retained_workflow_inventory_matches"] + and payload["blocking_workflows"] == [] + and payload["queued_or_in_progress_runs"] == [] + and payload["legacy_artifacts_remaining"] == [] + ) + + +def test_stage2_closure_artifacts_reject_stale_attempt_refs(): + transition_notice_payload = _load_fixture("stage2_transition_notice_fallback_closure_green.json") + deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_green.json") + + assert _transition_notice_gate_ready( + transition_notice_payload, + "0000000000000000000000000000000000000000", + ) is False + assert _deferred_payload_gate_ready( + deferred_payload, + "0000000000000000000000000000000000000000", + ) is False + + +def test_stage2_closure_cluster_gate_requires_both_current_attempt_green_artifacts(): + transition_notice_payload = _load_fixture("stage2_transition_notice_fallback_closure_green.json") + deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_green.json") + blocked_transition_notice_payload = _load_fixture( + "stage2_transition_notice_fallback_closure_blocked.json" + ) + blocked_deferred_payload = _load_fixture("stage2_deferred_payload_legacy_closure_blocked.json") + expected_ref = transition_notice_payload["evaluated_ref"] + + assert _stage2_closure_cluster_gate( + transition_notice_payload, + deferred_payload, + expected_ref=expected_ref, + ) is True + assert _stage2_closure_cluster_gate( + blocked_transition_notice_payload, + deferred_payload, + expected_ref=expected_ref, + ) is False + assert _stage2_closure_cluster_gate( + transition_notice_payload, + blocked_deferred_payload, + expected_ref=expected_ref, + ) is False diff --git a/tests/contract/reviewer_bot/test_stage2_deferred_payload_residue.py b/tests/contract/reviewer_bot/test_stage2_deferred_payload_residue.py new file mode 100644 index 000000000..831fc59b2 --- /dev/null +++ b/tests/contract/reviewer_bot/test_stage2_deferred_payload_residue.py @@ -0,0 +1,128 @@ +import json +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.contract + + +def _load_cases() -> dict: + return json.loads( + Path("tests/fixtures/workflow_contracts/deferred_payload_residue_cases.json").read_text( + encoding="utf-8" + ) + ) + + +def _simulate_residue_audit(case: dict) -> dict: + blocking_workflows = [] + queued_or_in_progress_runs = [] + legacy_artifacts_remaining = [] + evaluated_ref = case["evaluated_ref"] + + for workflow in case["workflows"]: + if workflow["classification"] == "required_retained" and workflow["state"] != "active": + blocking_workflows.append( + { + "workflow_name": workflow["workflow_name"], + "workflow_path": workflow["workflow_path"], + "reason": "required_retained_workflow_disabled", + } + ) + if workflow["classification"] == "removed_legacy" and workflow["state"] == "active": + blocking_workflows.append( + { + "workflow_name": workflow["workflow_name"], + "workflow_path": workflow["workflow_path"], + "reason": "removed_legacy_workflow_still_active", + } + ) + + for run in case["runs"]: + if run["status"] not in {"queued", "in_progress"}: + continue + if run["classification"] == "removed_legacy": + reason = "removed_legacy_workflow_run" + elif run["head_sha"] != evaluated_ref: + reason = "noncurrent_head_sha" + else: + continue + queued_or_in_progress_runs.append( + { + "workflow_name": run["workflow_name"], + "workflow_path": run["workflow_path"], + "run_id": run["run_id"], + "run_attempt": run["run_attempt"], + "status": run["status"], + "head_sha": run["head_sha"], + "reason": reason, + } + ) + + for artifact in case["artifacts"]: + if artifact["downloadable"] and artifact["payload_schema_version"] in {1, 2}: + legacy_artifacts_remaining.append( + { + "workflow_name": artifact["workflow_name"], + "workflow_path": artifact["workflow_path"], + "run_id": artifact["run_id"], + "run_attempt": artifact["run_attempt"], + "artifact_id": artifact["artifact_id"], + "payload_schema_version": artifact["payload_schema_version"], + "payload_kind": artifact["payload_kind"], + "artifact_name": artifact["artifact_name"], + "payload_filename": artifact["payload_filename"], + } + ) + + return { + "retained_workflow_inventory_matches": case["retained_workflow_inventory_matches"], + "blocking_workflows": blocking_workflows, + "queued_or_in_progress_runs": queued_or_in_progress_runs, + "legacy_artifacts_remaining": legacy_artifacts_remaining, + "closure_ready": case["retained_workflow_inventory_matches"] + and blocking_workflows == [] + and queued_or_in_progress_runs == [] + and legacy_artifacts_remaining == [], + } + + +def _select_deferred_payload(files: list[str]) -> str | None: + json_files = sorted(path for path in files if path.endswith(".json")) + if len(json_files) > 1: + raise RuntimeError(f"Expected at most one deferred payload, found {len(json_files)}") + if len(json_files) == 1: + return json_files[0] + return None + + +@pytest.mark.parametrize("case", _load_cases()["residue_cases"], ids=lambda case: case["id"]) +def test_b5e_residue_cases_are_simulated_locally(case): + simulated = _simulate_residue_audit(case) + + assert simulated == case["expected"] + + +@pytest.mark.parametrize( + "case", + _load_cases()["artifact_selection_cases"], + ids=lambda case: case["id"], +) +def test_reconcile_artifact_selection_cases_fail_closed(case): + if case["expected_multiple"]: + with pytest.raises(RuntimeError, match="Expected at most one deferred payload"): + _select_deferred_payload(case["files"]) + return + + assert _select_deferred_payload(case["files"]) == case["expected_selected_path"] + + +def test_router_zero_artifact_success_is_not_classified_as_residue(): + case = next(case for case in _load_cases()["residue_cases"] if case["id"] == "router_zero_artifact_success") + + simulated = _simulate_residue_audit(case) + + assert simulated["blocking_workflows"] == [] + assert simulated["queued_or_in_progress_runs"] == [] + assert simulated["legacy_artifacts_remaining"] == [] + assert simulated["closure_ready"] is True diff --git a/tests/contract/reviewer_bot/test_workflow_artifact_contracts.py b/tests/contract/reviewer_bot/test_workflow_artifact_contracts.py index bf244cac3..6363b7d49 100644 --- a/tests/contract/reviewer_bot/test_workflow_artifact_contracts.py +++ b/tests/contract/reviewer_bot/test_workflow_artifact_contracts.py @@ -3,6 +3,7 @@ from types import SimpleNamespace import pytest +import yaml pytestmark = pytest.mark.contract @@ -14,6 +15,12 @@ def _load_fixture_payload(relative_path: str) -> dict: return data["payload"] +def _load_workflow_job(relative_path: str) -> dict: + workflow = yaml.safe_load(Path(relative_path).read_text(encoding="utf-8")) + job_name = "route-pr-comment" if relative_path.endswith("reviewer-bot-pr-comment-router.yml") else "observer" + return workflow["jobs"][job_name] + + @pytest.mark.parametrize( ("workflow_path",), [ @@ -77,6 +84,49 @@ def test_deferred_comment_payload_parses_without_artifact_name_field(): assert parsed.identity.source_event_name == "issue_comment" +@pytest.mark.parametrize( + ("fixture_path", "workflow_path"), + [ + ( + "tests/fixtures/observer_payloads/workflow_pr_comment_deferred.json", + ".github/workflows/reviewer-bot-pr-comment-router.yml", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_submitted_deferred.json", + ".github/workflows/reviewer-bot-pr-review-submitted-observer.yml", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_dismissed_deferred.json", + ".github/workflows/reviewer-bot-pr-review-dismissed-observer.yml", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_comment_deferred.json", + ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + ), + ], +) +def test_deferred_payload_fixtures_match_upload_name_and_payload_name_helpers( + fixture_path, workflow_path +): + payload = _load_fixture_payload(fixture_path) + job = _load_workflow_job(workflow_path) + build_step = job["steps"][0] + upload_step = job["steps"][1] + rendered_upload_name = ( + upload_step["with"]["name"] + .replace("${{ github.run_id }}", str(payload["source_run_id"])) + .replace("${{ github.run_attempt }}", str(payload["source_run_attempt"])) + ) + + assert rendered_upload_name == reconcile_payloads.artifact_expected_name(payload) + assert build_step["env"]["PAYLOAD_PATH"].endswith( + reconcile_payloads.artifact_expected_payload_name(payload) + ) + assert upload_step["with"]["path"].endswith( + reconcile_payloads.artifact_expected_payload_name(payload) + ) + + def test_validate_workflow_run_artifact_identity_rejects_run_attempt_mismatch(monkeypatch): monkeypatch.setenv("WORKFLOW_RUN_TRIGGERING_ID", "1") monkeypatch.setenv("WORKFLOW_RUN_TRIGGERING_ATTEMPT", "2") diff --git a/tests/contract/reviewer_bot/test_workflow_files.py b/tests/contract/reviewer_bot/test_workflow_files.py index c96be44b3..5826f62fc 100644 --- a/tests/contract/reviewer_bot/test_workflow_files.py +++ b/tests/contract/reviewer_bot/test_workflow_files.py @@ -138,6 +138,16 @@ def test_workflow_summaries_and_runbook_references_exist(): assert "docs/reviewer-bot-review-freshness-operator-runbook.md" in reconcile +def test_reconcile_workflow_selects_at_most_one_recursive_json_payload(): + workflow_text = Path(".github/workflows/reviewer-bot-reconcile.yml").read_text(encoding="utf-8") + + assert "files = sorted(Path(os.environ['RUNNER_TEMP']).joinpath('observer-artifact').rglob('*.json'))" in workflow_text + assert "if len(files) > 1:" in workflow_text + assert "Expected at most one deferred payload" in workflow_text + assert "if len(files) == 1:" in workflow_text + assert "DEFERRED_CONTEXT_PATH=" in workflow_text + + @pytest.mark.parametrize( ("fixture_path", "workflow_file", "payload_kind", "expected_event_name", "expected_event_action"), [ diff --git a/tests/fixtures/workflow_contracts/deferred_payload_residue_cases.json b/tests/fixtures/workflow_contracts/deferred_payload_residue_cases.json new file mode 100644 index 000000000..39a14c9a2 --- /dev/null +++ b/tests/fixtures/workflow_contracts/deferred_payload_residue_cases.json @@ -0,0 +1,259 @@ +{ + "residue_cases": [ + { + "id": "disabled_retained_workflow", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": true, + "workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Router", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-router.yml", + "classification": "required_retained", + "state": "disabled_manually" + } + ], + "runs": [], + "artifacts": [], + "expected": { + "retained_workflow_inventory_matches": true, + "blocking_workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Router", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-router.yml", + "reason": "required_retained_workflow_disabled" + } + ], + "queued_or_in_progress_runs": [], + "legacy_artifacts_remaining": [], + "closure_ready": false + } + }, + { + "id": "removed_legacy_workflow_still_active", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": false, + "workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "classification": "removed_legacy", + "state": "active" + } + ], + "runs": [], + "artifacts": [], + "expected": { + "retained_workflow_inventory_matches": false, + "blocking_workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "reason": "removed_legacy_workflow_still_active" + } + ], + "queued_or_in_progress_runs": [], + "legacy_artifacts_remaining": [], + "closure_ready": false + } + }, + { + "id": "queued_removed_legacy_run", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": true, + "workflows": [], + "runs": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "classification": "removed_legacy", + "run_id": 9001, + "run_attempt": 1, + "status": "queued", + "head_sha": "74c8f6d25636c1772befbb3cc97e0e7598103047" + } + ], + "artifacts": [], + "expected": { + "retained_workflow_inventory_matches": true, + "blocking_workflows": [], + "queued_or_in_progress_runs": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "run_id": 9001, + "run_attempt": 1, + "status": "queued", + "head_sha": "74c8f6d25636c1772befbb3cc97e0e7598103047", + "reason": "removed_legacy_workflow_run" + } + ], + "legacy_artifacts_remaining": [], + "closure_ready": false + } + }, + { + "id": "noncurrent_retained_emitter_run", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": true, + "workflows": [], + "runs": [ + { + "workflow_name": "Reviewer Bot PR Review Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + "classification": "retained_emitter", + "run_id": 9002, + "run_attempt": 3, + "status": "in_progress", + "head_sha": "74c8f6d25636c1772befbb3cc97e0e7598103047" + } + ], + "artifacts": [], + "expected": { + "retained_workflow_inventory_matches": true, + "blocking_workflows": [], + "queued_or_in_progress_runs": [ + { + "workflow_name": "Reviewer Bot PR Review Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + "run_id": 9002, + "run_attempt": 3, + "status": "in_progress", + "head_sha": "74c8f6d25636c1772befbb3cc97e0e7598103047", + "reason": "noncurrent_head_sha" + } + ], + "legacy_artifacts_remaining": [], + "closure_ready": false + } + }, + { + "id": "downloadable_legacy_schema_artifacts", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": true, + "workflows": [], + "runs": [], + "artifacts": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "run_id": 9003, + "run_attempt": 1, + "artifact_id": 7001, + "payload_schema_version": 1, + "payload_kind": "observer_noop", + "artifact_name": "reviewer-bot-comment-context-9003-attempt-1", + "payload_filename": "deferred-comment.json", + "downloadable": true + }, + { + "workflow_name": "Reviewer Bot PR Review Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + "run_id": 9004, + "run_attempt": 2, + "artifact_id": 7002, + "payload_schema_version": 2, + "payload_kind": "deferred_review_comment", + "artifact_name": "reviewer-bot-review-comment-context-9004-attempt-2", + "payload_filename": "deferred-review-comment.json", + "downloadable": true + } + ], + "expected": { + "retained_workflow_inventory_matches": true, + "blocking_workflows": [], + "queued_or_in_progress_runs": [], + "legacy_artifacts_remaining": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "run_id": 9003, + "run_attempt": 1, + "artifact_id": 7001, + "payload_schema_version": 1, + "payload_kind": "observer_noop", + "artifact_name": "reviewer-bot-comment-context-9003-attempt-1", + "payload_filename": "deferred-comment.json" + }, + { + "workflow_name": "Reviewer Bot PR Review Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + "run_id": 9004, + "run_attempt": 2, + "artifact_id": 7002, + "payload_schema_version": 2, + "payload_kind": "deferred_review_comment", + "artifact_name": "reviewer-bot-review-comment-context-9004-attempt-2", + "payload_filename": "deferred-review-comment.json" + } + ], + "closure_ready": false + } + }, + { + "id": "router_zero_artifact_success", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "retained_workflow_inventory_matches": true, + "workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Router", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-router.yml", + "classification": "required_retained", + "state": "active" + } + ], + "runs": [ + { + "workflow_name": "Reviewer Bot PR Comment Router", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-router.yml", + "classification": "retained_emitter", + "run_id": 9005, + "run_attempt": 4, + "status": "completed", + "head_sha": "c158011fd015a27ad5d0cac06675fb2d01276362" + } + ], + "artifacts": [], + "expected": { + "retained_workflow_inventory_matches": true, + "blocking_workflows": [], + "queued_or_in_progress_runs": [], + "legacy_artifacts_remaining": [], + "closure_ready": true + } + } + ], + "artifact_selection_cases": [ + { + "id": "zero_json_files", + "files": [], + "expected_selected_path": null, + "expected_multiple": false + }, + { + "id": "one_json_file", + "files": [ + "observer-artifact/deferred-comment.json" + ], + "expected_selected_path": "observer-artifact/deferred-comment.json", + "expected_multiple": false + }, + { + "id": "multiple_json_files", + "files": [ + "observer-artifact/deferred-comment.json", + "observer-artifact/deferred-review-comment.json" + ], + "expected_selected_path": null, + "expected_multiple": true + }, + { + "id": "stray_nested_json_residue", + "files": [ + "observer-artifact/deferred-comment.json", + "observer-artifact/nested/extra.json" + ], + "expected_selected_path": null, + "expected_multiple": true + } + ] +} diff --git a/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_blocked.json b/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_blocked.json new file mode 100644 index 000000000..a1f362928 --- /dev/null +++ b/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_blocked.json @@ -0,0 +1,43 @@ +{ + "artifact_id": "deferred-payload-legacy-closure", + "generated_at": "2026-04-13T18:00:00Z", + "evaluated_repo": "rustfoundation/safety-critical-rust-coding-guidelines", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "closure_ready": false, + "retained_workflow_inventory_matches": false, + "blocking_workflows": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "reason": "removed_legacy_workflow_still_active" + } + ], + "queued_or_in_progress_runs": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "run_id": 9001, + "run_attempt": 1, + "status": "queued", + "head_sha": "74c8f6d25636c1772befbb3cc97e0e7598103047", + "reason": "removed_legacy_workflow_run" + } + ], + "legacy_artifacts_remaining": [ + { + "workflow_name": "Reviewer Bot PR Comment Observer", + "workflow_path": ".github/workflows/reviewer-bot-pr-comment-observer.yml", + "run_id": 9002, + "run_attempt": 1, + "artifact_id": 7001, + "payload_schema_version": 1, + "payload_kind": "observer_noop", + "artifact_name": "reviewer-bot-comment-context-9002-attempt-1", + "payload_filename": "deferred-comment.json" + } + ], + "control_plane_actions_applied": [], + "commands_run": [ + "gh workflow list --repo rustfoundation/safety-critical-rust-coding-guidelines" + ] +} diff --git a/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_green.json b/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_green.json new file mode 100644 index 000000000..68f7961c1 --- /dev/null +++ b/tests/fixtures/workflow_contracts/stage2_deferred_payload_legacy_closure_green.json @@ -0,0 +1,19 @@ +{ + "artifact_id": "deferred-payload-legacy-closure", + "generated_at": "2026-04-13T18:00:00Z", + "evaluated_repo": "rustfoundation/safety-critical-rust-coding-guidelines", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "closure_ready": true, + "retained_workflow_inventory_matches": true, + "blocking_workflows": [], + "queued_or_in_progress_runs": [], + "legacy_artifacts_remaining": [], + "control_plane_actions_applied": [ + "disable removed legacy workflow", + "delete legacy deferred artifact" + ], + "commands_run": [ + "gh workflow list --repo rustfoundation/safety-critical-rust-coding-guidelines", + "gh run list --repo rustfoundation/safety-critical-rust-coding-guidelines --workflow \"Reviewer Bot PR Comment Router\"" + ] +} diff --git a/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_blocked.json b/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_blocked.json new file mode 100644 index 000000000..c7e501009 --- /dev/null +++ b/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_blocked.json @@ -0,0 +1,22 @@ +{ + "artifact_id": "transition-notice-fallback-closure", + "generated_at": "2026-04-13T18:00:00Z", + "evaluated_repo": "rustfoundation/safety-critical-rust-coding-guidelines", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "state_issue_number": 314, + "closure_ready": false, + "active_reviews_scanned": 2, + "resolved_by_marker_backfill": [], + "resolved_by_legacy_prose_backfill": [], + "resolved_by_new_marker_notice": [], + "remaining_transition_due_without_notice": [ + { + "issue_number": 43, + "reviewer": "carol", + "reason": "transition eligible review still lacks transition_notice_sent_at" + } + ], + "commands_run": [ + "gh workflow run reviewer-bot-sweeper-repair.yml --repo rustfoundation/safety-critical-rust-coding-guidelines --ref main -f action=check-overdue" + ] +} diff --git a/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_green.json b/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_green.json new file mode 100644 index 000000000..2ddcfc95c --- /dev/null +++ b/tests/fixtures/workflow_contracts/stage2_transition_notice_fallback_closure_green.json @@ -0,0 +1,29 @@ +{ + "artifact_id": "transition-notice-fallback-closure", + "generated_at": "2026-04-13T18:00:00Z", + "evaluated_repo": "rustfoundation/safety-critical-rust-coding-guidelines", + "evaluated_ref": "c158011fd015a27ad5d0cac06675fb2d01276362", + "state_issue_number": 314, + "closure_ready": true, + "active_reviews_scanned": 3, + "resolved_by_marker_backfill": [ + { + "issue_number": 41, + "reviewer": "alice", + "notice_timestamp": "2026-04-13T17:55:00Z" + } + ], + "resolved_by_legacy_prose_backfill": [], + "resolved_by_new_marker_notice": [ + { + "issue_number": 42, + "reviewer": "bob", + "notice_timestamp": "2026-04-13T17:58:00Z" + } + ], + "remaining_transition_due_without_notice": [], + "commands_run": [ + "gh workflow run reviewer-bot-sweeper-repair.yml --repo rustfoundation/safety-critical-rust-coding-guidelines --ref main -f action=check-overdue", + "gh workflow run reviewer-bot-sweeper-repair.yml --repo rustfoundation/safety-critical-rust-coding-guidelines --ref main -f action=repair-review-status-labels" + ] +} diff --git a/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py b/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py index 7f01413cd..fda1b85fc 100644 --- a/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py +++ b/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py @@ -2,6 +2,7 @@ from scripts.reviewer_bot_lib import review_state from tests.fixtures.app_harness import AppHarness +from tests.fixtures.reconcile_harness import issue_comment_payload from tests.fixtures.reviewer_bot import make_state pytestmark = pytest.mark.integration @@ -91,3 +92,50 @@ def test_execute_run_closed_issue_comment_without_entry_skips_save(monkeypatch): assert result.exit_code == 0 assert save_called["value"] is False assert sync_calls == [[42]] + + +def test_execute_run_late_workflow_run_reconcile_does_not_recreate_removed_review_entry(monkeypatch): + harness = AppHarness(monkeypatch) + harness.set_workflow_run_name("Reviewer Bot PR Comment Router") + harness.set_event( + EVENT_NAME="workflow_run", + EVENT_ACTION="completed", + REVIEWER_BOT_WORKFLOW_KIND="reconcile", + WORKFLOW_RUN_TRIGGERING_CONCLUSION="success", + WORKFLOW_RUN_TRIGGERING_ID="610", + WORKFLOW_RUN_TRIGGERING_ATTEMPT="1", + ) + harness.runtime.stub_deferred_payload( + issue_comment_payload( + pr_number=42, + comment_id=210, + source_event_key="issue_comment:210", + body="@guidelines-bot /queue", + comment_class="command_only", + has_non_command_text=False, + source_created_at="2026-03-17T10:00:00Z", + actor_login="bob", + source_run_id=610, + source_run_attempt=1, + ) + ) + + state = make_state() + save_called = {"value": False} + sync_calls = [] + + harness.stub_lock(acquire=lambda: None, release=lambda: True) + harness.stub_load_state(lambda *, fail_on_unavailable=False: state) + harness.stub_pass_until(lambda current: (current, [])) + harness.stub_sync_members(lambda current: (current, [])) + harness.stub_save_state(lambda current: save_called.__setitem__("value", True) or True) + harness.stub_sync_status_labels( + lambda current, issue_numbers: sync_calls.append(list(issue_numbers)) or False + ) + + result = harness.run_execute() + + assert result.exit_code == 1 + assert state["active_reviews"] == {} + assert save_called["value"] is False + assert sync_calls == [] diff --git a/tests/integration/reviewer_bot/test_reconcile_workflow_run.py b/tests/integration/reviewer_bot/test_reconcile_workflow_run.py index 527ad58c5..dcdb84015 100644 --- a/tests/integration/reviewer_bot/test_reconcile_workflow_run.py +++ b/tests/integration/reviewer_bot/test_reconcile_workflow_run.py @@ -69,6 +69,31 @@ def test_handle_workflow_run_event_returns_true_for_submitted_review_bookkeeping assert "pull_request_review:11" not in _deferred_gaps(review) +def test_late_workflow_run_reconcile_does_not_recreate_removed_review_entry(monkeypatch): + state = make_state() + harness = ReconcileHarness( + monkeypatch, + issue_comment_payload( + pr_number=42, + comment_id=210, + source_event_key="issue_comment:210", + body="@guidelines-bot /queue", + comment_class="command_only", + has_non_command_text=False, + source_created_at="2026-03-17T10:00:00Z", + actor_login="bob", + source_run_id=610, + source_run_attempt=1, + ), + ) + + with pytest.raises(RuntimeError, match="No active review entry available for PR #42"): + reconcile.handle_workflow_run_event_result(harness.runtime, state) + + assert state["active_reviews"] == {} + assert harness.runtime.drain_touched_items() == [] + + def test_handle_workflow_run_event_persists_fail_closed_diagnostic_without_raising(monkeypatch): state = make_state() review = make_tracked_review_state(state, 42, reviewer="alice") @@ -102,6 +127,7 @@ def test_handle_workflow_run_event_persists_fail_closed_diagnostic_without_raisi def test_handle_workflow_run_event_treats_observer_noop_payload_as_no_mutation(monkeypatch): state = make_state(epoch="freshness_v15") + make_tracked_review_state(state, 42, reviewer="alice") harness = ReconcileHarness( monkeypatch, { diff --git a/tests/unit/reviewer_bot/test_reconcile_artifacts.py b/tests/unit/reviewer_bot/test_reconcile_artifacts.py index 8a8c58283..8b8a06914 100644 --- a/tests/unit/reviewer_bot/test_reconcile_artifacts.py +++ b/tests/unit/reviewer_bot/test_reconcile_artifacts.py @@ -1,3 +1,5 @@ +import json +from pathlib import Path from types import SimpleNamespace import pytest @@ -15,6 +17,10 @@ from tests.fixtures.reviewer_bot_fakes import RouteGitHubApi, github_result +def _load_fixture_payload(relative_path: str) -> dict: + return json.loads(Path(relative_path).read_text(encoding="utf-8"))["payload"] + + def test_review_comment_artifact_identity_validation(monkeypatch): state = make_state() review = review_state.ensure_review_entry(state, 42, create=True) @@ -87,6 +93,56 @@ def test_validate_workflow_run_artifact_identity_accepts_matching_contract(): reconcile_payloads.validate_workflow_run_artifact_identity(bot, payload) +@pytest.mark.parametrize( + ("fixture_path", "expected_workflow_name", "expected_workflow_file", "expected_artifact_name", "expected_payload_name"), + [ + ( + "tests/fixtures/observer_payloads/workflow_pr_comment_deferred.json", + "Reviewer Bot PR Comment Router", + ".github/workflows/reviewer-bot-pr-comment-router.yml", + "reviewer-bot-comment-context-401-attempt-3", + "deferred-comment.json", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_comment_deferred.json", + "Reviewer Bot PR Review Comment Observer", + ".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + "reviewer-bot-review-comment-context-404-attempt-6", + "deferred-review-comment.json", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_submitted_deferred.json", + "Reviewer Bot PR Review Submitted Observer", + ".github/workflows/reviewer-bot-pr-review-submitted-observer.yml", + "reviewer-bot-review-submitted-context-402-attempt-4", + "deferred-review-submitted.json", + ), + ( + "tests/fixtures/observer_payloads/workflow_pr_review_dismissed_deferred.json", + "Reviewer Bot PR Review Dismissed Observer", + ".github/workflows/reviewer-bot-pr-review-dismissed-observer.yml", + "reviewer-bot-review-dismissed-context-403-attempt-5", + "deferred-review-dismissed.json", + ), + ], +) +def test_deferred_artifact_expected_helpers_match_v3_payload_contract( + fixture_path, + expected_workflow_name, + expected_workflow_file, + expected_artifact_name, + expected_payload_name, +): + payload = _load_fixture_payload(fixture_path) + + assert reconcile_payloads.expected_observer_identity(payload) == ( + expected_workflow_name, + expected_workflow_file, + ) + assert reconcile_payloads.artifact_expected_name(payload) == expected_artifact_name + assert reconcile_payloads.artifact_expected_payload_name(payload) == expected_payload_name + + def test_read_reconcile_object_fails_closed_for_unavailable(monkeypatch): runtime = FakeReviewerBotRuntime(monkeypatch) routes = RouteGitHubApi().add_request( diff --git a/tests/unit/reviewer_bot/test_reconcile_unit.py b/tests/unit/reviewer_bot/test_reconcile_unit.py index 7b99e9dcc..164505e24 100644 --- a/tests/unit/reviewer_bot/test_reconcile_unit.py +++ b/tests/unit/reviewer_bot/test_reconcile_unit.py @@ -252,6 +252,8 @@ def test_handle_workflow_run_event_result_collects_touched_item(monkeypatch): } ) state = make_state(epoch="freshness_v15") + review = review_state.ensure_review_entry(state, 42, create=True) + assert review is not None result = reconcile.handle_workflow_run_event_result(runtime, state)