diff --git a/scripts/ci/pr_review_autofix_context.py b/scripts/ci/pr_review_autofix_context.py index f3f652b8d4..cc7b6fb003 100755 --- a/scripts/ci/pr_review_autofix_context.py +++ b/scripts/ci/pr_review_autofix_context.py @@ -13,6 +13,11 @@ from pathlib import Path from typing import Any +try: + from pr_review_fix_scheduler import current_head_failed_checks +except ModuleNotFoundError: + from scripts.ci.pr_review_fix_scheduler import current_head_failed_checks + REPO_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") @@ -327,6 +332,7 @@ def write_context( reviews = current_reviews(repo, number, head_sha) threads = review_threads(repo, number) detected_rca_mode = review_requires_rca(reviews) + direct_rca_mode = bool(current_head_failed_checks(pr)) if repair_mode is None: rca_mode = detected_rca_mode elif repair_mode == "conflict": @@ -334,12 +340,14 @@ def write_context( # Failed-check reviews may coexist on the same head, but they must not # widen this approved conflict-only invocation to every changed path. rca_mode = False - elif (repair_mode == "rca") != detected_rca_mode: + elif repair_mode == "rca" and not (detected_rca_mode or direct_rca_mode): raise RuntimeError( - "requested repair mode does not match exact-head review evidence" + "requested RCA mode lacks exact-head review or failed-check evidence" ) + elif repair_mode != "rca" and detected_rca_mode: + raise RuntimeError("requested repair mode does not match exact-head review evidence") else: - rca_mode = detected_rca_mode + rca_mode = repair_mode == "rca" if failed_check_evidence_path is not None and not rca_mode: raise RuntimeError( "failed-check evidence is accepted only for exact-head RCA repair" diff --git a/scripts/ci/pr_review_fix_scheduler.py b/scripts/ci/pr_review_fix_scheduler.py index 33bd7ca1bb..5d13f68108 100755 --- a/scripts/ci/pr_review_fix_scheduler.py +++ b/scripts/ci/pr_review_fix_scheduler.py @@ -14,22 +14,30 @@ try: from pr_review_merge_scheduler import ( + complete_paginated_pr_contexts, fetch_open_prs, fetch_pr, + context_nodes, has_current_head_approval, has_current_head_changes_requested, is_opencode_review, + latest_check_run_attempts, + REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW, review_matches_current_head, run, unresolved_thread_count, ) except ModuleNotFoundError: from scripts.ci.pr_review_merge_scheduler import ( + complete_paginated_pr_contexts, fetch_open_prs, fetch_pr, + context_nodes, has_current_head_approval, has_current_head_changes_requested, is_opencode_review, + latest_check_run_attempts, + REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW, review_matches_current_head, run, unresolved_thread_count, @@ -69,6 +77,26 @@ "sast semgrep failed", "codeql failed", ) +RCA_IGNORED_CHECK_NAMES = frozenset( + { + "metadata-only gate evaluation", + "opencode-review", + "PR governance metadata controller", + "scan-pr-queue", + } +) +RCA_IGNORED_WORKFLOW_NAMES = frozenset( + { + "OpenCode Review", + "Required OpenCode Review", + "OpenCode PR Review", + REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW, + } +) +FAILED_CHECK_CONCLUSIONS = frozenset( + {"FAILURE", "STARTUP_FAILURE", "TIMED_OUT"} +) +FAILED_STATUS_STATES = frozenset({"ERROR", "FAILURE"}) def run_json(args: list[str]) -> Any: @@ -203,14 +231,52 @@ def needs_autofix(pr: dict[str, Any]) -> tuple[bool, tuple[str, ...]]: def needs_rca_repair(pr: dict[str, Any]) -> tuple[bool, tuple[str, ...]]: """Return whether exact-head failed-check evidence warrants RCA and repair.""" - if not ( + review_requires_rca = ( has_current_head_changes_requested(pr) and change_request_requires_rca(pr) - ): + ) + failed_checks = current_head_failed_checks(pr) + if not review_requires_rca and not failed_checks: return False, () + if failed_checks: + return True, ( + "current-head failed check(s) require RCA: " + ", ".join(failed_checks), + ) return True, ("current-head failed-check blocker requires RCA",) +def current_head_failed_checks(pr: dict[str, Any]) -> tuple[str, ...]: + """Return terminal failed checks that can carry source-backed RCA evidence.""" + failed: list[str] = [] + rollup = pr.get("statusCheckRollup") or {} + nodes = rollup if isinstance(rollup, list) else context_nodes(pr) + for node in latest_check_run_attempts(nodes): + if node.get("__typename") == "CheckRun": + name = str(node.get("name") or "").strip() + workflow_name = str( + ( + ((node.get("checkSuite") or {}).get("workflowRun") or {}).get( + "workflow" + ) + or {} + ).get("name") + or "" + ).strip() + conclusion = str(node.get("conclusion") or "").upper() + if ( + name not in RCA_IGNORED_CHECK_NAMES + and workflow_name not in RCA_IGNORED_WORKFLOW_NAMES + and conclusion in FAILED_CHECK_CONCLUSIONS + ): + failed.append(name or "unnamed check") + else: + name = str(node.get("context") or "").strip() + state = str(node.get("state") or "").upper() + if name not in RCA_IGNORED_CHECK_NAMES and state in FAILED_STATUS_STATES: + failed.append(name or "unnamed status") + return tuple(dict.fromkeys(failed)) + + CONFLICT_MERGE_STATES = frozenset({"DIRTY", "CONFLICTING"}) @@ -336,8 +402,6 @@ def inspect_pr( ) -> tuple[str, tuple[str, ...]]: """Inspect one PR and optionally dispatch a bounded repair.""" number = int(pr["number"]) - if pr.get("isDraft"): - return "skip", ("draft PR",) if not _base_branch_matches(pr, args.base_branch): return "skip", ( f"base branch is {pr.get('baseRefName')}; expected {args.base_branch}", @@ -347,28 +411,39 @@ def inspect_pr( "external PR head is not writable by repository workflow credentials", ) - needs_fix, reasons = needs_autofix(pr) - repair_mode = "review" - resolve_conflict = False - if not needs_fix: - needs_rca, rca_reasons = needs_rca_repair(pr) - if needs_rca: - repair_mode = "rca" - reasons = rca_reasons - else: - needs_resolve, resolve_reasons = needs_conflict_resolution( - pr, - allow_unreviewed=bool( - getattr(args, "resolve_unreviewed_conflicts", False) - ), - ) - if not needs_resolve: - return "skip", ( - "no current-head autofixable review, failed-check RCA, or approved merge conflict", - ) - resolve_conflict = True - repair_mode = "conflict" - reasons = resolve_reasons + conflicted = str(pr.get("mergeStateStatus") or "").upper() in CONFLICT_MERGE_STATES + if conflicted: + if pr.get("isDraft"): + return "skip", ("draft PR",) + needs_resolve, resolve_reasons = needs_conflict_resolution( + pr, + allow_unreviewed=bool( + getattr(args, "resolve_unreviewed_conflicts", False) + ), + ) + if not needs_resolve: + return "skip", ("merge conflict is not authorized for repair",) + needs_fix = True + reasons = resolve_reasons + repair_mode = "conflict" + resolve_conflict = True + else: + needs_fix, reasons = needs_autofix(pr) + repair_mode = "review" + resolve_conflict = False + + needs_rca, rca_reasons = needs_rca_repair(pr) + if pr.get("isDraft") and not needs_rca: + return "skip", ("draft PR",) + + if not conflicted and needs_rca: + needs_fix = True + repair_mode = "rca" + reasons = rca_reasons + elif not needs_fix and not conflicted: + return "skip", ( + "no current-head autofixable review, failed-check RCA, or approved merge conflict", + ) if comments is None: comments = issue_comments(repo, number) @@ -400,13 +475,24 @@ def process_queue(args: argparse.Namespace) -> int: if args.pr_number else fetch_open_prs(args.repo, args.max_prs) ) + pagination_errors: set[int] = set() + for pr in prs: + if not _base_branch_matches(pr, args.base_branch): + continue + if not same_repository_head(args.repo, pr): + continue + try: + complete_paginated_pr_contexts(args.repo, pr) + except RuntimeError: + pagination_errors.add(int(pr["number"])) + dispatched = 0 inspected = 0 decisions: list[dict[str, Any]] = [] prs_needing_comments = [] for pr in prs: - if pr.get("isDraft"): + if int(pr["number"]) in pagination_errors: continue if not _base_branch_matches(pr, args.base_branch): continue @@ -420,7 +506,9 @@ def process_queue(args: argparse.Namespace) -> int: getattr(args, "resolve_unreviewed_conflicts", False) ), ) - if needs_fix or needs_rca or needs_resolve: + if (needs_fix and not pr.get("isDraft")) or needs_rca or ( + needs_resolve and not pr.get("isDraft") + ): prs_needing_comments.append(pr) comments_by_pr: dict[int, list[dict[str, Any]]] = {} @@ -462,6 +550,17 @@ def fetch_comments( for pr in prs: inspected += 1 + pr_number = int(pr["number"]) + if pr_number in pagination_errors: + reasons = ( + "status-context pagination failed; deferring this PR without " + "evaluating partial check evidence", + ) + decisions.append( + {"pr": pr["number"], "action": "wait", "reasons": list(reasons)} + ) + print(f"PR #{pr['number']}: wait: {reasons[0]}") + continue if dispatched >= args.max_dispatches: decisions.append( { @@ -471,7 +570,6 @@ def fetch_comments( } ) continue - pr_number = int(pr["number"]) if pr_number in comment_fetch_errors: decisions.append( { diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 6d77596c7c..00d71905d9 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -182,6 +182,7 @@ "OpenCode Review Dispatch", } OPENCODE_REVIEW_WORKFLOW_PATH = ".github/workflows/opencode-review.yml" +REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW = "__unknown_github_actions_workflow__" RUNNING_CHECK_STATES = {"PENDING", "EXPECTED", "QUEUED", "IN_PROGRESS", "WAITING", "REQUESTED"} FAILED_CHECK_CONCLUSIONS = {"FAILURE", "ERROR", "CANCELLED", "TIMED_OUT", "STARTUP_FAILURE"} ACTION_REQUIRED_CONCLUSIONS = {"ACTION_REQUIRED"} @@ -977,20 +978,59 @@ def fetch_all_pr_reviews_rest(repo: str, number: int) -> list[dict[str, Any]]: return reviews +def fetch_workflow_names_by_check_suite_rest( + repo: str, head_sha: str +) -> dict[int, str]: + """Return exact-head GitHub Actions workflow names keyed by check-suite ID. + + REST check-run payloads omit workflow identity. The Actions run list + preserves the shared check-suite ID, allowing the REST fallback to + retain the same workflow-level policy boundary as the GraphQL path. + When the integration cannot read Actions, callers receive an empty + map and GitHub Actions checks are marked with a fail-closed sentinel. + """ + workflow_names: dict[int, str] = {} + page = 1 + while True: + try: + payload = gh_api_json( + f"repos/{repo}/actions/runs?head_sha={quote(head_sha, safe='')}" + f"&per_page=100&page={page}" + ) + except RuntimeError as exc: + if github_resource_inaccessible(exc): + return {} + raise + workflow_runs = payload.get("workflow_runs") or [] + for workflow_run in workflow_runs: + suite_id = workflow_run.get("check_suite_id") + workflow_name = str(workflow_run.get("name") or "").strip() + if suite_id is not None and workflow_name: + workflow_names[int(suite_id)] = workflow_name + if len(workflow_runs) < 100: + break + page += 1 + return workflow_names + + def rest_check_node( - check: dict[str, Any], suite_created_at_by_id: dict[int, str] | None = None + check: dict[str, Any], + suite_created_at_by_id: dict[int, str] | None = None, + workflow_name_by_suite_id: dict[int, str] | None = None, ) -> dict[str, Any]: """Convert a REST check-run payload into the GraphQL status rollup shape. - ``suite_created_at_by_id`` maps each check suite's REST ``id`` to its - ``created_at`` timestamp -- the REST check-run payload itself only - carries the check suite's bare ``id`` (see ``rest_pr_node``), so that - lookup is how this function attaches the same ``checkSuite.createdAt`` - signal the GraphQL fragment fetches directly, keeping - ``check_run_recency_key`` behaviorally consistent across both paths. + ``suite_created_at_by_id`` and ``workflow_name_by_suite_id`` attach + the check-suite recency and workflow identity that GraphQL exposes + directly. Unknown GitHub Actions workflow identity is represented by + a fail-closed sentinel so it cannot be mistaken for a source failure. """ suite_id = (check.get("check_suite") or {}).get("id") suite_created_at = (suite_created_at_by_id or {}).get(suite_id) + workflow_name = (workflow_name_by_suite_id or {}).get(suite_id) + if not workflow_name and (check.get("app") or {}).get("slug") == "github-actions": + workflow_name = REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW + workflow = {"name": workflow_name} if workflow_name else {} return { "__typename": "CheckRun", "name": check.get("name"), @@ -998,7 +1038,10 @@ def rest_check_node( "conclusion": (check.get("conclusion") or "").upper() if check.get("conclusion") else None, "startedAt": check.get("started_at"), "detailsUrl": check.get("details_url"), - "checkSuite": {"createdAt": suite_created_at, "workflowRun": {"workflow": {}}}, + "checkSuite": { + "createdAt": suite_created_at, + "workflowRun": {"workflow": workflow}, + }, } @@ -1032,12 +1075,21 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: head_repo = head.get("repo") or {} reviews = fetch_all_pr_reviews_rest(repo, number) checks = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-runs?per_page=100") + check_runs = checks.get("check_runs") or [] check_suites = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-suites?per_page=100") suite_created_at_by_id = { suite["id"]: suite.get("created_at") for suite in (check_suites.get("check_suites") or []) if suite.get("id") is not None } + workflow_name_by_suite_id = ( + fetch_workflow_names_by_check_suite_rest(repo, str(head.get("sha") or "")) + if any( + (check.get("app") or {}).get("slug") == "github-actions" + for check in check_runs + ) + else {} + ) combined_status = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/status") files = gh_api_json(f"repos/{repo}/pulls/{number}/files?per_page=20") rest_merge_state = REST_MERGEABLE_STATE_MAP.get( @@ -1066,8 +1118,12 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: "statusCheckRollup": { "contexts": { "nodes": [ - rest_check_node(check, suite_created_at_by_id) - for check in (checks.get("check_runs") or []) + rest_check_node( + check, + suite_created_at_by_id, + workflow_name_by_suite_id, + ) + for check in check_runs ] + [ rest_status_node(status) @@ -1307,7 +1363,8 @@ def is_strix_context(node: dict[str, Any]) -> bool: ) workflow_name = workflow.get("name") return workflow_name in {"Strix Security Scan", "Strix"} or ( - node.get("name") == "strix" and workflow_name is None + node.get("name") == "strix" + and workflow_name in {None, REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW} ) return (node.get("context") or "") in {"strix", "Strix Security Scan"} diff --git a/tests/test_pr_review_autofix_context_import_fallback.py b/tests/test_pr_review_autofix_context_import_fallback.py new file mode 100644 index 0000000000..4a0ccf8116 --- /dev/null +++ b/tests/test_pr_review_autofix_context_import_fallback.py @@ -0,0 +1,39 @@ +"""Regression coverage for the autofix context package-import fallback.""" + +from __future__ import annotations + +import builtins +from pathlib import Path +from typing import Any + +from scripts.ci import pr_review_autofix_context as context +from scripts.ci import pr_review_fix_scheduler as scheduler + + +def test_package_import_fallback_uses_package_scheduler(monkeypatch: Any) -> None: + """A package import still resolves the exact failed-check helper.""" + real_import = builtins.__import__ + + def import_with_missing_top_level( + name: str, + globals_: dict[str, Any] | None = None, + locals_: dict[str, Any] | None = None, + fromlist: tuple[str, ...] = (), + level: int = 0, + ) -> Any: + if name == "pr_review_fix_scheduler" and level == 0: + raise ModuleNotFoundError("forced top-level import miss") + return real_import(name, globals_, locals_, fromlist, level) + + monkeypatch.setattr(builtins, "__import__", import_with_missing_top_level) + module_path = Path(context.__file__) + namespace: dict[str, Any] = { + "__builtins__": builtins, + "__file__": str(module_path), + "__name__": "scripts.ci.pr_review_autofix_context_fallback_probe", + "__package__": "scripts.ci", + } + + exec(compile(module_path.read_text(encoding="utf-8"), str(module_path), "exec"), namespace) + + assert namespace["current_head_failed_checks"] is scheduler.current_head_failed_checks diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index bd836379dc..3b4416bdc3 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -37,6 +37,183 @@ def test_recent_fix_marker_is_head_scoped(): assert not fix.recent_fix_marker_exists([{"body": f"{fix.FIX_MARKER} head_sha={head} epoch=oops -->"}], head, 24 * 3600) +def test_terminal_failed_check_triggers_rca_without_prior_opencode_review(): + """Exact-head check evidence can start RCA without a circular review prerequisite.""" + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "Application CI", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ] + } + } + ) + + assert fix.current_head_failed_checks(pr) == ("Application CI",) + assert fix.needs_rca_repair(pr) == ( + True, + ("current-head failed check(s) require RCA: Application CI",), + ) + + +def test_control_plane_failure_and_pending_checks_do_not_trigger_rca(): + """The metadata gate and nonterminal checks cannot recursively dispatch repair.""" + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "metadata-only gate evaluation", + "status": "COMPLETED", + "conclusion": "FAILURE", + }, + { + "__typename": "CheckRun", + "name": "Application CI", + "status": "IN_PROGRESS", + "conclusion": None, + }, + { + "__typename": "CheckRun", + "name": "Superseded run", + "status": "COMPLETED", + "conclusion": "CANCELLED", + }, + { + "__typename": "CheckRun", + "name": "Approval gate", + "status": "COMPLETED", + "conclusion": "ACTION_REQUIRED", + }, + ] + } + } + ) + + assert fix.current_head_failed_checks(pr) == () + assert fix.needs_rca_repair(pr) == (False, ()) + + +@pytest.mark.parametrize( + ("older_conclusion", "newer_status", "newer_conclusion", "expected"), + [ + ("FAILURE", "COMPLETED", "SUCCESS", ()), + ("SUCCESS", "COMPLETED", "FAILURE", ("Application CI",)), + ("FAILURE", "IN_PROGRESS", None, ()), + ], +) +def test_failed_checks_use_only_latest_check_run_attempt( + older_conclusion, newer_status, newer_conclusion, expected +): + """Successful or pending reruns supersede stale failures by suite creation time.""" + def attempt(created_at, status, conclusion): + return { + "__typename": "CheckRun", + "name": "Application CI", + "status": status, + "conclusion": conclusion, + "checkSuite": { + "createdAt": created_at, + "workflowRun": {"workflow": {"name": "Application CI"}}, + }, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + attempt("2026-09-01T00:00:00Z", "COMPLETED", older_conclusion), + attempt("2026-09-01T01:00:00Z", newer_status, newer_conclusion), + ] + } + } + ) + + assert fix.current_head_failed_checks(pr) == expected + + +def test_draft_with_failed_check_dispatches_rca(monkeypatch): + """A draft stays unmergeable while its real source failure reaches bounded RCA.""" + captured = {} + pr = make_pr( + isDraft=True, + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "StatusContext", + "context": "Application CI", + "state": "FAILURE", + } + ] + } + }, + ) + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda repo, pr, **kwargs: captured.update(kwargs), + ) + monkeypatch.setattr(fix, "create_fix_marker", lambda repo, pr, dry_run: None) + args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main", "--dry-run"]) + + action, reasons = fix.inspect_pr("owner/repo", pr, args) + + assert action == "dispatch" + assert reasons == ("current-head failed check(s) require RCA: Application CI",) + assert captured["repair_mode"] == "rca" + + +def test_conflict_repair_precedes_failed_check_rca(monkeypatch): + """A conflicted tree must be repaired before check failures can be diagnosed.""" + captured = {} + pr = make_pr( + mergeStateStatus="DIRTY", + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "StatusContext", + "context": "Application CI", + "state": "FAILURE", + } + ] + } + }, + ) + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda repo, pr, **kwargs: captured.update(kwargs), + ) + monkeypatch.setattr(fix, "create_fix_marker", lambda repo, pr, dry_run: None) + args = fix.parse_args( + [ + "--repo", + "owner/repo", + "--base-branch", + "main", + "--resolve-unreviewed-conflicts", + "--dry-run", + ] + ) + + action, reasons = fix.inspect_pr("owner/repo", pr, args) + + assert action == "dispatch" + assert "auto-resolving" in reasons[0] + assert captured["resolve_conflict"] is True + assert "repair_mode" not in captured + + def test_needs_autofix_uses_current_head_evidence(): """Autofix starts from current-head OpenCode change requests.""" head = "a" * 40 @@ -368,6 +545,50 @@ def test_context_explicit_rca_uses_precollected_evidence(monkeypatch, tmp_path): assert "redacted exact-head failure" in body +def test_context_direct_rca_uses_live_failed_check_without_review(monkeypatch, tmp_path): + """Trusted context authorizes direct RCA from live exact-head failure evidence.""" + head = "a" * 40 + pr = { + "number": 7, + "title": "Repair failed checks", + "url": "https://example.test/pr/7", + "headRefName": "feature", + "baseRefName": "main", + "headRefOid": head, + "baseRefOid": "b" * 40, + "mergeStateStatus": "CLEAN", + "statusCheckRollup": [ + { + "__typename": "CheckRun", + "name": "Application CI", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ], + } + monkeypatch.setattr(context, "pr_view", lambda repo, number: pr) + monkeypatch.setattr(context, "current_reviews", lambda repo, number, head_sha: []) + monkeypatch.setattr(context, "review_threads", lambda repo, number: []) + monkeypatch.setattr(context, "pr_changed_paths", lambda repo, number: ["src/app.py"]) + evidence = tmp_path / "failed-checks.md" + evidence.write_text("exact-head Application CI failure", encoding="utf-8") + output = tmp_path / "context.md" + + context.write_context( + "owner/repo", + 7, + head, + output, + repair_mode="rca", + failed_check_evidence_path=evidence, + ) + + body = output.read_text(encoding="utf-8") + assert "Repair mode: failed-check-rca" in body + assert "exact-head Application CI failure" in body + assert "- `src/app.py`" in body + + def test_context_inferred_rca_collects_evidence(monkeypatch, tmp_path): """Legacy callers still infer RCA and invoke the trusted collector once.""" head = "a" * 40 @@ -436,7 +657,7 @@ def test_context_explicit_mode_and_evidence_fail_closed(monkeypatch, tmp_path): output = tmp_path / "context.md" monkeypatch.setattr(context, "current_reviews", lambda repo, number, head_sha: []) - with pytest.raises(RuntimeError, match="does not match"): + with pytest.raises(RuntimeError, match="lacks exact-head"): context.write_context("owner/repo", 7, head, output, repair_mode="rca") evidence = tmp_path / "review-only.md" diff --git a/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py b/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py new file mode 100644 index 0000000000..c5a0c965b5 --- /dev/null +++ b/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py @@ -0,0 +1,262 @@ +"""Regression tests for direct failed-check RCA arbitration.""" + +from __future__ import annotations + +import json +from typing import Any + +import pytest + +from scripts.ci import pr_review_fix_scheduler as fix + + +def make_pr(*, is_draft: bool = False) -> dict[str, Any]: + """Return a clean same-repository PR with review and failed-check evidence.""" + head = "a" * 40 + return { + "number": 7, + "isDraft": is_draft, + "baseRefName": "main", + "baseRefOid": "b" * 40, + "headRefName": "feature", + "headRefOid": head, + "headRepository": {"nameWithOwner": "owner/repo"}, + "mergeStateStatus": "CLEAN", + "reviews": { + "nodes": [ + { + "state": "CHANGES_REQUESTED", + "author": {"login": "opencode-agent"}, + "commit": {"oid": head}, + "body": "Actionable source-backed finding with a suggested diff.", + } + ] + }, + "reviewThreads": {"nodes": []}, + "statusCheckRollup": { + "contexts": { + "pageInfo": {"hasNextPage": False, "endCursor": None}, + "nodes": [ + { + "__typename": "CheckRun", + "name": "Application CI", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ], + } + }, + } + + +@pytest.mark.parametrize("is_draft", [False, True]) +def test_failed_check_rca_precedes_ordinary_review(monkeypatch: Any, is_draft: bool) -> None: + """A terminal source failure wins over ordinary review feedback, including drafts.""" + captured: dict[str, Any] = {} + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda repo, pr, **kwargs: captured.update(kwargs), + ) + monkeypatch.setattr(fix, "create_fix_marker", lambda repo, pr, dry_run: None) + args = fix.parse_args( + ["--repo", "owner/repo", "--base-branch", "main", "--dry-run"] + ) + + action, reasons = fix.inspect_pr("owner/repo", make_pr(is_draft=is_draft), args) + + assert action == "dispatch" + assert reasons == ("current-head failed check(s) require RCA: Application CI",) + assert captured["repair_mode"] == "rca" + assert captured["resolve_conflict"] is False + + +def test_scan_queue_control_plane_failure_does_not_trigger_rca() -> None: + """A failed queue scanner cannot consume a source-repair retry by itself.""" + pr = make_pr() + pr["reviews"] = {"nodes": []} + pr["statusCheckRollup"]["contexts"]["nodes"] = [ + { + "__typename": "CheckRun", + "name": "scan-pr-queue", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ] + + assert fix.current_head_failed_checks(pr) == () + assert fix.needs_rca_repair(pr) == (False, ()) + + +@pytest.mark.parametrize( + "workflow_name", + ["OpenCode Review", "Required OpenCode Review", "OpenCode PR Review"], +) +def test_opencode_control_plane_workflow_failure_does_not_trigger_rca( + workflow_name: str, +) -> None: + """Renamed jobs in categorically excluded review workflows stay excluded.""" + pr = make_pr() + pr["reviews"] = {"nodes": []} + pr["statusCheckRollup"]["contexts"]["nodes"] = [ + { + "__typename": "CheckRun", + "name": "renamed review job", + "status": "COMPLETED", + "conclusion": "FAILURE", + "checkSuite": {"workflowRun": {"workflow": {"name": workflow_name}}}, + } + ] + + assert fix.current_head_failed_checks(pr) == () + assert fix.needs_rca_repair(pr) == (False, ()) + + +@pytest.mark.parametrize("single_pr", [False, True]) +def test_process_queue_completes_check_pages_before_rca_decision( + monkeypatch: Any, capsys: Any, single_pr: bool +) -> None: + """Queue and single fetches load later failures before choosing repair mode.""" + pr = make_pr() + pr["statusCheckRollup"]["contexts"] = { + "pageInfo": {"hasNextPage": True, "endCursor": "page_1"}, + "nodes": [ + { + "__typename": "CheckRun", + "name": "Application CI", + "status": "COMPLETED", + "conclusion": "SUCCESS", + } + ], + } + order: list[str] = [] + captured: dict[str, Any] = {} + + def complete_pages(repo: str, candidate: dict[str, Any]) -> None: + order.append("paginate") + contexts = candidate["statusCheckRollup"]["contexts"] + contexts["nodes"].append( + { + "__typename": "CheckRun", + "name": "Security Scan", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ) + contexts["pageInfo"] = {"hasNextPage": False, "endCursor": None} + + def dispatch(repo: str, candidate: dict[str, Any], **kwargs: Any) -> None: + order.append("dispatch") + captured.update(kwargs) + + monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) + monkeypatch.setattr(fix, "fetch_pr", lambda repo, number: [pr]) + monkeypatch.setattr(fix, "complete_paginated_pr_contexts", complete_pages) + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "dispatch_autofix", dispatch) + monkeypatch.setattr(fix, "create_fix_marker", lambda repo, candidate, dry_run: None) + argv = ["--repo", "owner/repo", "--base-branch", "main", "--dry-run"] + if single_pr: + argv.extend(["--pr-number", "7"]) + args = fix.parse_args(argv) + + assert fix.process_queue(args) == 0 + + assert order == ["paginate", "dispatch"] + assert captured["repair_mode"] == "rca" + payload = json.loads(capsys.readouterr().out.strip().splitlines()[-1]) + assert payload["autofix_dispatches"] == 1 + assert payload["decisions"][0]["reasons"] == [ + "current-head failed check(s) require RCA: Security Scan" + ] + + +def test_process_queue_isolates_one_pagination_failure( + monkeypatch: Any, capsys: Any +) -> None: + """One incomplete rollup waits while another PR still dispatches repair.""" + blocked = make_pr() + blocked["number"] = 1 + repairable = make_pr() + repairable["number"] = 2 + paginated: list[int] = [] + dispatched: list[int] = [] + + def complete_pages(repo: str, candidate: dict[str, Any]) -> None: + paginated.append(int(candidate["number"])) + if candidate["number"] == 1: + raise RuntimeError("status rollup pagination unavailable") + + monkeypatch.setattr( + fix, + "fetch_open_prs", + lambda repo, max_prs: [blocked, repairable], + ) + monkeypatch.setattr(fix, "complete_paginated_pr_contexts", complete_pages) + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda repo, candidate, **kwargs: dispatched.append(int(candidate["number"])), + ) + monkeypatch.setattr(fix, "create_fix_marker", lambda repo, candidate, dry_run: None) + args = fix.parse_args( + [ + "--repo", + "owner/repo", + "--base-branch", + "main", + "--max-dispatches", + "2", + "--dry-run", + ] + ) + + assert fix.process_queue(args) == 0 + + assert paginated == [1, 2] + assert dispatched == [2] + payload = json.loads(capsys.readouterr().out.strip().splitlines()[-1]) + decisions = {entry["pr"]: entry for entry in payload["decisions"]} + assert decisions[1]["action"] == "wait" + assert "status-context pagination failed" in decisions[1]["reasons"][0] + assert decisions[2]["action"] == "dispatch" + + +def test_process_queue_does_not_paginate_out_of_scope_pr( + monkeypatch: Any, capsys: Any +) -> None: + """Base-filtered PRs do not spend status-rollup pagination requests.""" + out_of_scope = make_pr() + out_of_scope["number"] = 1 + out_of_scope["baseRefName"] = "develop" + in_scope = make_pr() + in_scope["number"] = 2 + paginated: list[int] = [] + + def complete_pages(repo: str, candidate: dict[str, Any]) -> None: + paginated.append(int(candidate["number"])) + if candidate["number"] == 1: + raise AssertionError("out-of-scope PR must not be paginated") + + monkeypatch.setattr( + fix, + "fetch_open_prs", + lambda repo, max_prs: [out_of_scope, in_scope], + ) + monkeypatch.setattr(fix, "complete_paginated_pr_contexts", complete_pages) + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "dispatch_autofix", lambda *args, **kwargs: None) + monkeypatch.setattr(fix, "create_fix_marker", lambda *args, **kwargs: None) + args = fix.parse_args( + ["--repo", "owner/repo", "--base-branch", "main", "--dry-run"] + ) + + assert fix.process_queue(args) == 0 + + assert paginated == [2] + payload = json.loads(capsys.readouterr().out.strip().splitlines()[-1]) + decisions = {entry["pr"]: entry for entry in payload["decisions"]} + assert decisions[1]["action"] == "skip" + assert decisions[2]["action"] == "dispatch" diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py new file mode 100644 index 0000000000..c24cfb05f9 --- /dev/null +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -0,0 +1,156 @@ +"""Regression coverage for REST-fallback workflow identity.""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from scripts.ci import pr_review_fix_scheduler as fix +from scripts.ci import pr_review_merge_scheduler as merge + + +def test_rest_fallback_preserves_renamed_opencode_workflow_identity( + monkeypatch: Any, +) -> None: + """A renamed OpenCode job remains a control-plane check after REST conversion.""" + head_sha = "a" * 40 + calls: list[str] = [] + payloads: dict[str, Any] = { + "repos/owner/repo/pulls/42/reviews?per_page=100&page=1": [], + f"repos/owner/repo/commits/{head_sha}/check-runs?per_page=100": { + "check_runs": [ + { + "name": "renamed review policy gate", + "status": "completed", + "conclusion": "failure", + "started_at": "2026-09-01T03:00:00Z", + "details_url": ( + "https://github.com/owner/repo/actions/runs/123/job/456" + ), + "check_suite": {"id": 777}, + "app": {"slug": "github-actions"}, + } + ] + }, + f"repos/owner/repo/commits/{head_sha}/check-suites?per_page=100": { + "check_suites": [ + {"id": 777, "created_at": "2026-09-01T03:00:00Z"} + ] + }, + f"repos/owner/repo/commits/{head_sha}/status": {"statuses": []}, + "repos/owner/repo/pulls/42/files?per_page=20": [], + } + + def fake_api(path: str) -> Any: + calls.append(path) + if path.startswith("repos/owner/repo/actions/runs?"): + return { + "workflow_runs": [ + { + "check_suite_id": 777, + "name": "Required OpenCode Review", + } + ] + } + return payloads[path] + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + pr = merge.rest_pr_node( + "owner/repo", + { + "number": 42, + "title": "REST fallback", + "draft": False, + "mergeable": True, + "mergeable_state": "clean", + "maintainer_can_modify": True, + "auto_merge": None, + "user": {"login": "author"}, + "head": { + "ref": "feature", + "sha": head_sha, + "repo": {"full_name": "owner/repo"}, + }, + "base": {"ref": "main", "sha": "b" * 40}, + }, + ) + + contexts = merge.context_nodes(pr) + workflow = contexts[0]["checkSuite"]["workflowRun"]["workflow"] + assert workflow["name"] == "Required OpenCode Review" + assert fix.current_head_failed_checks(pr) == () + assert any("/actions/runs?" in path for path in calls) + + +@pytest.mark.parametrize( + ("conclusion", "expected_state"), + [("success", "complete"), ("failure", "failed")], +) +def test_rest_fallback_keeps_name_only_strix_when_actions_runs_are_inaccessible( + monkeypatch: Any, + conclusion: str, + expected_state: str, +) -> None: + """Unknown workflow identity does not erase authoritative name-only Strix evidence.""" + head_sha = "c" * 40 + payloads: dict[str, Any] = { + "repos/owner/repo/pulls/43/reviews?per_page=100&page=1": [], + f"repos/owner/repo/commits/{head_sha}/check-runs?per_page=100": { + "check_runs": [ + { + "name": "strix", + "status": "completed", + "conclusion": conclusion, + "started_at": "2026-09-01T04:00:00Z", + "details_url": ( + "https://github.com/owner/repo/actions/runs/321/job/654" + ), + "check_suite": {"id": 778}, + "app": {"slug": "github-actions"}, + } + ] + }, + f"repos/owner/repo/commits/{head_sha}/check-suites?per_page=100": { + "check_suites": [ + {"id": 778, "created_at": "2026-09-01T04:00:00Z"} + ] + }, + f"repos/owner/repo/commits/{head_sha}/status": {"statuses": []}, + "repos/owner/repo/pulls/43/files?per_page=20": [], + } + + def fake_api(path: str) -> Any: + if path.startswith("repos/owner/repo/actions/runs?"): + raise RuntimeError("Resource not accessible by integration") + return payloads[path] + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + pr = merge.rest_pr_node( + "owner/repo", + { + "number": 43, + "title": "REST fallback Strix", + "draft": False, + "mergeable": True, + "mergeable_state": "clean", + "maintainer_can_modify": True, + "auto_merge": None, + "user": {"login": "author"}, + "head": { + "ref": "feature", + "sha": head_sha, + "repo": {"full_name": "owner/repo"}, + }, + "base": {"ref": "main", "sha": "d" * 40}, + }, + ) + + context = merge.context_nodes(pr)[0] + workflow = context["checkSuite"]["workflowRun"]["workflow"] + assert workflow["name"] == merge.REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW + assert merge.is_strix_context(context) + assert merge.strix_evidence_state(pr) == expected_state + assert fix.current_head_failed_checks(pr) == ()