diff --git a/CHANGELOG.md b/CHANGELOG.md index c7b0d0cfac..ac1985d86f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- **Fail closed before cancelling stale PR workflow runs.** Validate snapshot `headRefOid` and re-read live PR/run identity immediately before destructive cancellation, including OpenCode/Strix dispatch cleanup, so a missing head or concurrent push cannot cancel the sole current-head evidence or trigger a duplicate review. Also ensures every cancellation path (`cancel_stale_pr_runs`, `cancel_stale_opencode_runs`, `_cancel_revalidated_review_run_refs`) treats a run as cancelled only when `force_cancel_workflow_runs` actually reports success, not merely when live revalidation proved it stale -- superseding PR #1712's simpler `force_cancel_workflow_run_refs` wrapper (removed as dead code; its safety guarantee is preserved inline at every call site by this more thorough revalidate-then-cancel design). - **Cache `active_workflow_runs` for the life of one `pr_review_merge_scheduler.py` invocation.** `inspect_pr()` calls `cancel_stale_pr_runs()` unconditionally for every non-draft PR before any eligibility gate, and several other call sites diff --git a/docs/doctoring/scheduler-stale-headrefoid-cancellation.md b/docs/doctoring/scheduler-stale-headrefoid-cancellation.md new file mode 100644 index 0000000000..8f526516e7 --- /dev/null +++ b/docs/doctoring/scheduler-stale-headrefoid-cancellation.md @@ -0,0 +1,46 @@ +# Scheduler stale-head cancellation: fail closed at the destructive boundary + +## Incident + +On 2026-09-02, `ContextualWisdomLab/naruon#1528` had Strix run `33581213829` +cancelled while head `cf472cf77fb93325858f485a22e967449d7c387a` was still the pull +request's sole current head. The run-local Strix supersession job was skipped; +the shared merge scheduler remained a separate cancellation authority. + +## Root cause + +`stale_pr_run_ids()` and `active_review_run_refs()` converted an unresolved or +malformed `headRefOid` into non-authoritative comparison state. Their downstream +destructive paths trusted an earlier snapshot. A push between classification and +cancellation could therefore make a newly current run appear stale. The direct +OpenCode and Strix dispatch paths also cancelled their classified stale refs +without refreshing run and pull-request identity. + +## Repair contract + +- Snapshot heads pass the canonical 40-hex SHA validator. Missing or malformed + heads preserve all active runs. +- Every direct and central-review cancellation candidate is re-read immediately + before its destructive cancellation call. +- The live pull request must still be open, expose an explicit live draft state, and + expose a valid head SHA. Open drafts remain eligible for stale review-run cleanup + because draft review-only dispatch is supported; merge admission stays independently draft-gated. +- The candidate run must still be queued/in-progress and retain the expected + direct PR association or trusted central dispatch target. +- A candidate that now matches the live head, or whose identity/state cannot be + proven, is preserved and blocks duplicate dispatch rather than being cancelled. +- Genuine older-head runs remain cancellable, including the bounded parallel + multi-candidate path. + +This aligns the Python scheduler with the live-reference race contract already +used by `scripts/ci/revalidate_queue_cancellation.sh`. + +## Verification + +The one-shot publisher first installs isolated regressions and requires each one +to finish as exactly one ordinary pytest failure (`exit=1`, `1 failed`) before +production transformation. Collection/environment failures are not accepted as +RED evidence. Final verification runs the focused scheduler suite, complete +repository suite with 100% statement/branch coverage, 100% `scripts/ci` +docstring coverage, compileall, and diff hygiene. The publisher, workflow, and +all temporary repair artifacts delete themselves from the published successor. diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index a0364dbcb4..d8c4ce9b63 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2779,7 +2779,15 @@ def stale_pr_run_ids( statuses: Sequence[str] = ("queued", "in_progress"), ) -> list[str]: """Return active run ids for older heads of the same pull request.""" - head = str(pr.get("headRefOid") or "").lower() + raw_head = pr.get("headRefOid") + try: + head = validate_git_sha(str(raw_head or "")).lower() + except (TypeError, ValueError) as exc: + print( + f"::warning::stale_pr_run_ids: PR #{pr.get('number')} in {repo} has an " + f"invalid or unresolved headRefOid; preserving active runs ({exc})." + ) + return [] number = int(pr["number"]) stale: list[str] = [] for run_data in active_workflow_runs(repo, statuses): @@ -2816,7 +2824,15 @@ def active_review_run_refs( centralized_dispatch = bool( (os.environ.get("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY") or "").strip() ) - head = str(pr.get("headRefOid") or "").lower() + raw_head = pr.get("headRefOid") + try: + head = validate_git_sha(str(raw_head or "")).lower() + except (TypeError, ValueError) as exc: + print( + f"::warning::active_review_run_refs: PR #{pr.get('number')} in {target_repo} has an " + f"invalid or unresolved headRefOid; preserving review runs ({exc})." + ) + return [], [] number = int(pr["number"]) dispatch_title_prefixes = tuple( f"{title} {target_repo}#{number}@" @@ -3010,44 +3026,146 @@ def cancel_one(run_id: str) -> tuple[str, str | None]: return failures -def force_cancel_workflow_run_refs(run_refs: Sequence[tuple[str, str]]) -> list[tuple[str, str]]: - """Force-cancel repository-qualified runs and return the ones actually cancelled. +def _fresh_open_pr_for_cancellation(repo: str, number: int) -> dict[str, Any]: + """Return fresh open PR authority, including explicitly identified draft state.""" + payload = gh_api_json(f"repos/{repo}/pulls/{number}") + if not isinstance(payload, dict) or str(payload.get("state") or "").lower() != "open": + raise ValueError(f"PR #{number} in {repo} is not a resolvable open pull request") + if payload.get("draft") not in {True, False}: + raise ValueError(f"PR #{number} in {repo} has no authoritative live draft state") + validate_git_sha(str(((payload.get("head") or {}).get("sha")) or "")) + return payload - ``force_cancel_workflow_runs`` reports GitHub's per-run cancellation rejections - as a ``{run_id: failure_reason}`` dict rather than raising. A caller that treats - every requested ref as gone once this returns would misclassify a run GitHub - refused to cancel as cancelled -- letting a duplicate review dispatch alongside - a run that is, in fact, still active. Exclude rejected refs from the result so - every caller can tell the difference. - """ - runs_by_repo: dict[str, list[str]] = {} - for run_repo, run_id in run_refs: - runs_by_repo.setdefault(run_repo, []).append(run_id) - cancelled: list[tuple[str, str]] = [] - for run_repo, run_ids in runs_by_repo.items(): - failures = force_cancel_workflow_runs(run_repo, run_ids) - cancelled.extend((run_repo, run_id) for run_id in run_ids if run_id not in failures) - return cancelled + +def _fresh_active_run_for_cancellation(run_repo: str, run_id: str) -> dict[str, Any]: + """Return fresh active workflow-run evidence immediately before cancellation.""" + payload = gh_api_json(f"repos/{run_repo}/actions/runs/{run_id}") + if not isinstance(payload, dict) or str(payload.get("status") or "").lower() not in { + "queued", + "in_progress", + }: + raise ValueError(f"workflow run {run_repo}#{run_id} is not active") + return payload + + +def _fresh_pr_head_for_cancellation(repo: str, number: int) -> str: + """Return the validated head SHA from fresh ready/open PR authority.""" + payload = _fresh_open_pr_for_cancellation(repo, number) + return validate_git_sha(str(((payload.get("head") or {}).get("sha")) or "")).lower() + + +def _direct_pr_run_still_superseded(repo: str, number: int, run_id: str) -> bool: + """Return whether a direct PR run is still older than the freshly fetched live head.""" + try: + run_data = _fresh_active_run_for_cancellation(repo, run_id) + if run_data.get("event") == "repository_dispatch" or not workflow_run_mentions_pr( + run_data, number + ): + raise ValueError("workflow run no longer has direct pull-request authority") + run_head = validate_git_sha(str(run_data.get("head_sha") or "")).lower() + live_head = _fresh_pr_head_for_cancellation(repo, number) + except (KeyError, RuntimeError, TypeError, ValueError) as exc: + print( + f"::warning::Preserving workflow run {run_id} in {repo}: " + f"live stale-run revalidation failed closed ({exc})." + ) + return False + return run_head != live_head + + +def _review_run_target_head( + run_data: dict[str, Any], repo: str, workflow: str, number: int +) -> str: + """Return a validated target head for one direct or trusted central review run.""" + if run_data.get("event") == "repository_dispatch": + titles = {"Required OpenCode Review", workflow, *OPENCODE_WORKFLOW_NAMES} + display_title = str(run_data.get("display_title") or "") + prefixes = tuple( + f"{title} {repo}#{number}@" for title in sorted(titles, key=len, reverse=True) + ) + prefix = next((candidate for candidate in prefixes if display_title.startswith(candidate)), None) + if prefix is None: + raise ValueError("repository_dispatch run has no trusted target identity") + return validate_git_sha(display_title.removeprefix(prefix)).lower() + if not workflow_run_mentions_pr(run_data, number): + raise ValueError("review run no longer belongs to the target pull request") + return validate_git_sha(str(run_data.get("head_sha") or "")).lower() + + +def _review_run_still_superseded( + repo: str, + workflow: str, + number: int, + run_repo: str, + run_id: str, +) -> bool: + """Return whether one review run remains stale against fresh ready/open PR authority.""" + try: + run_data = _fresh_active_run_for_cancellation(run_repo, run_id) + run_head = _review_run_target_head(run_data, repo, workflow, number) + live_head = _fresh_pr_head_for_cancellation(repo, number) + except (KeyError, RuntimeError, TypeError, ValueError) as exc: + print( + f"::warning::Preserving review run {run_repo}#{run_id}: " + f"live stale-run revalidation failed closed ({exc})." + ) + return False + return run_head != live_head def cancel_stale_pr_runs(repo: str, pr: dict[str, Any], *, dry_run: bool) -> list[str]: - """Force-cancel queued or running workflows for older heads of the same PR.""" + """Force-cancel only direct-run candidates still proven stale at the destructive boundary.""" if dry_run: return [] require_github_actions_control_actor("force-cancel-stale-pr-runs") - run_ids = stale_pr_run_ids(repo, pr) - failures = force_cancel_workflow_runs(repo, run_ids) - return [run_id for run_id in run_ids if run_id not in failures] + number = int(pr["number"]) + candidates = [str(run_id) for run_id in stale_pr_run_ids(repo, pr)] + + def cancel_one(run_id: str) -> str | None: + """Revalidate and cancel one direct workflow-run candidate when still stale.""" + if not _direct_pr_run_still_superseded(repo, number, run_id): + return None + failures = force_cancel_workflow_runs(repo, [run_id]) + if run_id in failures: + return None + return run_id + + if len(candidates) <= 1: + results = [cancel_one(run_id) for run_id in candidates] + else: + max_workers = min(REST_MERGEABLE_STATE_WORKERS, len(candidates)) + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + results = list(executor.map(cancel_one, candidates)) + return [run_id for run_id in results if run_id is not None] def cancel_stale_opencode_runs(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> list[str]: - """Force-cancel older OpenCode runs for the same PR before retrying current head.""" + """Force-cancel only review candidates still proven stale at the destructive boundary.""" if dry_run: return [] require_github_actions_control_actor("force-cancel-stale-opencode-review") + number = int(pr["number"]) _, stale_refs = active_opencode_run_refs(repo, workflow, pr) - cancelled_refs = force_cancel_workflow_run_refs(stale_refs) - return [run_id for _, run_id in cancelled_refs] + + def cancel_one(run_ref: tuple[str, str]) -> str | None: + """Revalidate and cancel one review-run candidate when still stale.""" + run_repo, run_id = run_ref + if not _review_run_still_superseded(repo, workflow, number, run_repo, run_id): + return None + failures = force_cancel_workflow_runs(run_repo, [run_id]) + if run_id in failures: + return None + return run_id + + if len(stale_refs) <= 1: + results = [cancel_one(run_ref) for run_ref in stale_refs] + else: + max_workers = min(REST_MERGEABLE_STATE_WORKERS, len(stale_refs)) + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + results = list(executor.map(cancel_one, stale_refs)) + return [run_id for run_id in results if run_id is not None] + + def discover_opencode_required_run_id(repo: str, head_sha: str) -> int | None: @@ -3100,6 +3218,44 @@ def discover_opencode_required_run_id(repo: str, head_sha: str) -> int | None: return newest_id +def _cancel_revalidated_review_run_refs( + repo: str, + workflow: str, + pr: dict[str, Any], + run_refs: list[tuple[str, str]], +) -> tuple[list[tuple[str, str]], list[tuple[str, str]]]: + """Cancel only review refs still proven stale immediately before each destructive call. + + A failed/malformed live read is preservation authority, not permission to + dispatch a duplicate review. The returned first list therefore contains + every active candidate that could not be proven stale; callers fold those + refs into their current/busy set. Multiple candidates retain the scheduler's + existing bounded executor and deterministic input ordering. + """ + if not run_refs: + return [], [] + number = int(pr["number"]) + + def cancel_one(run_ref: tuple[str, str]) -> tuple[str, tuple[str, str]]: + """Revalidate one candidate and cancel it only while it remains stale.""" + run_repo, run_id = run_ref + if not _review_run_still_superseded(repo, workflow, number, run_repo, run_id): + return "preserved", run_ref + failures = force_cancel_workflow_runs(run_repo, [run_id]) + if run_id in failures: + return "preserved", run_ref + return "cancelled", run_ref + + if len(run_refs) == 1: + outcomes = [cancel_one(run_refs[0])] + else: + max_workers = min(REST_MERGEABLE_STATE_WORKERS, len(run_refs)) + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + outcomes = list(executor.map(cancel_one, run_refs)) + preserved = [run_ref for state, run_ref in outcomes if state == "preserved"] + cancelled = [run_ref for state, run_ref in outcomes if state == "cancelled"] + return preserved, cancelled + def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> str: """Dispatch trusted OpenCode for the PR head, or report an active run. @@ -3112,7 +3268,10 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr if not dry_run: require_github_actions_control_actor("inspect-active-opencode-review") current_run_refs, stale_run_refs = active_opencode_run_refs(repo, workflow, pr) - force_cancel_workflow_run_refs(stale_run_refs) + preserved_run_refs, _cancelled_run_refs = _cancel_revalidated_review_run_refs( + repo, workflow, pr, stale_run_refs + ) + current_run_refs = [*current_run_refs, *preserved_run_refs] if current_run_refs: print( "OpenCode review dispatch skipped: active same-head workflow run(s) " @@ -3189,7 +3348,10 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry run_title="Strix Security Scan", workflow_aliases=frozenset({"Strix Security Scan"}), ) - cancelled_refs = force_cancel_workflow_run_refs(stale_run_refs) + preserved_run_refs, cancelled_refs = _cancel_revalidated_review_run_refs( + repo, workflow, pr, stale_run_refs + ) + current_run_refs = [*current_run_refs, *preserved_run_refs] if current_run_refs: print( "Strix evidence dispatch skipped: active same-head workflow run(s) " diff --git a/tests/test_pr1669_cancel_stale_opencode_runs.py b/tests/test_pr1669_cancel_stale_opencode_runs.py new file mode 100644 index 0000000000..9529b87f43 --- /dev/null +++ b/tests/test_pr1669_cancel_stale_opencode_runs.py @@ -0,0 +1,154 @@ +"""Permanent regression coverage for PR #1669's headRefOid cancellation bug. + +Reproduces the live ``ContextualWisdomLab/naruon#1528`` incident: Strix run +``33581213829`` for head ``cf472cf77fb93325858f485a22e967449d7c387a`` was +force-cancelled while it was the PR's sole, unchanged current head, because +``stale_pr_run_ids()`` and ``active_review_run_refs()`` computed the expected +head as ``str(pr.get("headRefOid") or "").lower()`` -- a missing/falsy +``headRefOid`` silently coerced to ``""``, which never equals a real 40-hex +``head_sha``, so every active run for the PR (including the true current-head +run) was misclassified as stale. See +``docs/doctoring/scheduler-stale-headrefoid-cancellation.md``. +""" + +from scripts.ci import pr_review_merge_scheduler as sched + +NARUON_REPO = "ContextualWisdomLab/naruon" +NARUON_PR_NUMBER = 1528 +NARUON_RUN_ID = 33581213829 +NARUON_HEAD_SHA = "cf472cf77fb93325858f485a22e967449d7c387a" + + +def test_stale_pr_run_ids_preserves_current_head_run_when_head_ref_oid_missing(monkeypatch): + """A missing headRefOid must not classify the live current-head run stale.""" + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda *_args, **_kwargs: [ + { + "id": NARUON_RUN_ID, + "head_sha": NARUON_HEAD_SHA, + "pull_requests": [{"number": NARUON_PR_NUMBER}], + } + ], + ) + + stale = sched.stale_pr_run_ids( + NARUON_REPO, {"number": NARUON_PR_NUMBER, "headRefOid": None} + ) + + assert stale == [] + + +def test_active_review_run_refs_preserves_current_head_run_when_head_ref_oid_missing( + monkeypatch, +): + """A missing headRefOid must not classify the live current-head review run stale.""" + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda *_args, **_kwargs: [ + { + "id": NARUON_RUN_ID, + "event": "pull_request", + "name": "Strix Security Scan", + "head_sha": NARUON_HEAD_SHA, + "pull_requests": [{"number": NARUON_PR_NUMBER}], + } + ], + ) + + current, stale = sched.active_review_run_refs( + NARUON_REPO, + "Strix Security Scan", + {"number": NARUON_PR_NUMBER, "headRefOid": None}, + run_title="Strix Security Scan", + workflow_aliases=frozenset({"Strix Security Scan"}), + ) + + assert current == [] + assert stale == [] + + +def test_cancel_stale_pr_runs_issues_no_cancel_call_when_head_ref_oid_missing(monkeypatch): + """A missing headRefOid must yield no stale candidate before the second, + live-revalidation safety net ever runs -- isolated here (by forcing that + net to say "still superseded") so this test depends only on the + ``stale_pr_run_ids`` guard under test, not on the independent live re-fetch.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda *_args, **_kwargs: [ + { + "id": NARUON_RUN_ID, + "head_sha": NARUON_HEAD_SHA, + "pull_requests": [{"number": NARUON_PR_NUMBER}], + } + ], + ) + monkeypatch.setattr(sched, "_direct_pr_run_still_superseded", lambda *_a, **_k: True) + cancelled = [] + monkeypatch.setattr( + sched, + "force_cancel_workflow_runs", + lambda *args: cancelled.append(args), + ) + + run_ids = sched.cancel_stale_pr_runs( + NARUON_REPO, + {"number": NARUON_PR_NUMBER, "headRefOid": None}, + dry_run=False, + ) + + assert run_ids == [] + assert cancelled == [] + + +def test_cancel_stale_opencode_runs_uses_revalidated_refs(monkeypatch): + """Revalidate every candidate and cancel only refs still proven stale.""" + actor_calls: list[str] = [] + revalidated: list[tuple[str, str, int, str, str]] = [] + cancelled: list[tuple[str, list[str]]] = [] + stale_refs = [("owner/repo", "101"), ("owner/repo", "202")] + + monkeypatch.setattr( + sched, + "require_github_actions_control_actor", + lambda action: actor_calls.append(action), + ) + monkeypatch.setattr( + sched, + "active_opencode_run_refs", + lambda _repo, _workflow, _pr: ([], stale_refs), + ) + + def still_superseded(repo, workflow, number, run_repo, run_id): + revalidated.append((repo, workflow, number, run_repo, run_id)) + return True + + monkeypatch.setattr(sched, "_review_run_still_superseded", still_superseded) + + def cancel(repo, run_ids): + cancelled.append((repo, list(run_ids))) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + + run_ids = sched.cancel_stale_opencode_runs( + "owner/repo", + "OpenCode Review", + {"number": 7, "headRefOid": "a" * 40}, + dry_run=False, + ) + + assert actor_calls == ["force-cancel-stale-opencode-review"] + assert sorted(revalidated) == [ + ("owner/repo", "OpenCode Review", 7, "owner/repo", "101"), + ("owner/repo", "OpenCode Review", 7, "owner/repo", "202"), + ] + assert sorted(cancelled) == [ + ("owner/repo", ["101"]), + ("owner/repo", ["202"]), + ] + assert sorted(run_ids) == ["101", "202"] diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 94da03902a..8b5ddfcbce 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1424,6 +1424,7 @@ def map(self, func, items): def test_cancel_stale_opencode_runs_uses_bounded_executor_for_multiple_runs(monkeypatch): + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) seen_workers = [] class FakeExecutor: @@ -1505,14 +1506,14 @@ def test_cancel_revalidated_review_run_refs_preserves_failed_cancellation(monkey Discovered mid-flight during PR #1669's development (the naruon headRefOid incident fix) and intentionally scoped out of that PR; landing fresh here per - docs/doctoring/scheduler-stale-headrefoid-cancellation.md. That branch's - prototype named this cancellation path ``_cancel_revalidated_review_run_refs``; - current main's actual shared choke point for cancelling a revalidated batch of - stale/superseded review run refs -- used by both ``dispatch_opencode_review`` - and ``dispatch_strix_evidence`` -- is :func:`force_cancel_workflow_run_refs`, - so this test (kept under the established name) targets that real function. + docs/doctoring/scheduler-stale-headrefoid-cancellation.md. The live-revalidating + ``_cancel_revalidated_review_run_refs`` (used by both ``dispatch_opencode_review`` + and ``dispatch_strix_evidence``) must not report a ref as cancelled when the + underlying ``force_cancel_workflow_runs`` call itself was rejected by GitHub, + even though the ref was independently proven still-stale by live revalidation. """ stale_refs = [("owner/repo", "101"), ("owner/repo", "202")] + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) def cancel(_repo, run_ids): run_id = str(run_ids[0]) @@ -1520,8 +1521,11 @@ def cancel(_repo, run_ids): monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) - cancelled = sched.force_cancel_workflow_run_refs(stale_refs) + preserved, cancelled = sched._cancel_revalidated_review_run_refs( + "owner/repo", "OpenCode Review", make_pr(), stale_refs + ) + assert ("owner/repo", "101") in preserved assert ("owner/repo", "101") not in cancelled assert ("owner/repo", "202") in cancelled @@ -1535,6 +1539,7 @@ def test_cancel_stale_opencode_runs_preserves_failed_cancellation(monkeypatch): "active_opencode_run_refs", lambda _repo, _workflow, _pr: ([], stale_refs), ) + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) def cancel(_repo, run_ids): run_id = str(run_ids[0]) @@ -1973,7 +1978,6 @@ def test_dispatch_opencode_review_falls_back_to_bounded_discovery(monkeypatch): monkeypatch.setattr( sched, "active_opencode_run_refs", lambda repo, workflow, pr: ([], []) ) - monkeypatch.setattr(sched, "force_cancel_workflow_run_refs", lambda refs: None) monkeypatch.setattr( sched, "discover_opencode_required_run_id", @@ -4786,6 +4790,7 @@ def fake_run(args, stdin=None): def test_dispatch_opencode_review_force_cancels_same_pr_old_head_runs(monkeypatch): + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) calls = [] head_sha = "a" * 40 base_sha = "b" * 40 @@ -5288,6 +5293,7 @@ def fake_run(args, stdin=None): def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, capsys): + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) calls = [] head_sha = "a" * 40 stale_sha = "c" * 40 @@ -5515,6 +5521,7 @@ def test_active_run_filters_and_stale_opencode_dry_run(monkeypatch): def test_cancel_stale_pr_runs_force_cancels_queued_and_in_progress_old_heads(monkeypatch): + monkeypatch.setattr(sched, "_direct_pr_run_still_superseded", lambda *_args: True) calls = [] head_sha = "a" * 40 stale_same_pr = { @@ -5583,6 +5590,7 @@ def test_cancel_stale_pr_runs_preserves_failed_cancellation(monkeypatch): """Do not report a stale run cancelled when GitHub rejected the API call.""" monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) monkeypatch.setattr(sched, "stale_pr_run_ids", lambda _repo, _pr: ["101", "202"]) + monkeypatch.setattr(sched, "_direct_pr_run_still_superseded", lambda *_args: True) def cancel(_repo, run_ids): run_id = str(run_ids[0]) @@ -7114,6 +7122,7 @@ def test_draft_pr_review_only_dispatch_retries_a_failed_required_check_with_no_v def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch): + monkeypatch.setattr(sched, "validate_git_sha", lambda value: str(value)) runs = [ {"name": "Other", "id": 10, "head_sha": "old", "pull_requests": [{"number": 1}]}, {"name": "OpenCode Review", "id": 11, "head_sha": "head", "pull_requests": [{"number": 1}]}, @@ -7128,6 +7137,7 @@ def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch def test_workflow_run_filters_skip_mismatched_workflow_and_current_head_other_pr(monkeypatch): + monkeypatch.setattr(sched, "validate_git_sha", lambda value: str(value)) runs = [ {"name": "Other", "id": 20, "head_sha": "old", "pull_requests": [{"number": 1}]}, {"name": "OpenCode Review", "id": 21, "head_sha": "head", "pull_requests": [{"number": 2}]}, @@ -9028,3 +9038,512 @@ def test_inspect_pr_dry_run_skips_merge_revalidation_refetch(monkeypatch): assert direct_decision.action == "merge" assert auto_decision.action == "auto_merge" assert fetch_calls == [] + + + +def test_pr1669_malformed_snapshot_head_never_classifies_direct_run_stale(monkeypatch): + """Malformed snapshot head authority cannot classify a valid active run stale.""" + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda *_args, **_kwargs: [ + {"id": 33581213829, "head_sha": "a" * 40, "pull_requests": [{"number": 1528}]} + ], + ) + assert sched.stale_pr_run_ids( + "ContextualWisdomLab/naruon", + make_pr(number=1528, headRefOid="malformed-but-truthy"), + ) == [] + + +def test_pr1669_malformed_snapshot_head_never_classifies_review_run_stale(monkeypatch): + """Malformed snapshot head authority cannot classify central review runs stale.""" + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda *_args, **_kwargs: [ + { + "id": 33581213829, + "event": "pull_request", + "name": "OpenCode Review", + "head_sha": "a" * 40, + "pull_requests": [{"number": 1528}], + } + ], + ) + assert sched.active_review_run_refs( + "ContextualWisdomLab/naruon", + "OpenCode Review", + make_pr(number=1528, headRefOid="malformed-but-truthy"), + run_title="Required OpenCode Review", + workflow_aliases=frozenset(sched.OPENCODE_WORKFLOW_NAMES), + ) == ([], []) + + +def test_pr1669_snapshot_race_preserves_new_current_head(monkeypatch): + """A push after classification cannot make the new current-head run cancellable.""" + old_head, new_head = "a" * 40, "b" * 40 + candidate = { + "id": 77, + "event": "pull_request", + "status": "queued", + "head_sha": new_head, + "pull_requests": [{"number": 7}], + } + monkeypatch.setattr(sched, "stale_pr_run_ids", lambda *_args, **_kwargs: ["77"]) + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + calls = [] + + def fake_api(path): + calls.append(path) + if path.endswith("/actions/runs/77"): + return candidate + return {"state": "open", "draft": False, "head": {"sha": new_head}} + + cancelled = [] + monkeypatch.setattr(sched, "gh_api_json", fake_api) + monkeypatch.setattr( + sched, + "force_cancel_workflow_runs", + lambda *_args: cancelled.append(_args), + ) + assert sched.cancel_stale_pr_runs( + "owner/repo", make_pr(number=7, headRefOid=old_head), dry_run=False + ) == [] + assert cancelled == [] + assert calls[-1] == "repos/owner/repo/pulls/7" + + +@pytest.mark.parametrize( + "live_pr", + [ + None, + {"state": "closed", "draft": False, "head": {"sha": "b" * 40}}, + {"state": "open", "draft": None, "head": {"sha": "b" * 40}}, + {"state": "open", "draft": False, "head": {"sha": "bad"}}, + ], +) +def test_pr1669_fresh_open_pr_fails_closed_without_open_exact_head(monkeypatch, live_pr): + """Only an open PR with explicit draft state and valid SHA grants stale-run cancellation authority.""" + monkeypatch.setattr(sched, "gh_api_json", lambda _path: live_pr) + with pytest.raises(ValueError): + sched._fresh_open_pr_for_cancellation("owner/repo", 7) + + +@pytest.mark.parametrize("payload", [None, {"status": "completed"}]) +def test_pr1669_fresh_active_run_requires_active_mapping(monkeypatch, payload): + """Only a freshly active run mapping can authorize destructive cancellation.""" + monkeypatch.setattr(sched, "gh_api_json", lambda _path: payload) + with pytest.raises(ValueError, match="is not active"): + sched._fresh_active_run_for_cancellation("owner/repo", "94") + + +@pytest.mark.parametrize( + "run", + [ + { + "event": "repository_dispatch", + "status": "queued", + "head_sha": "a" * 40, + "pull_requests": [{"number": 7}], + }, + { + "event": "pull_request", + "status": "queued", + "head_sha": "a" * 40, + "pull_requests": [{"number": 8}], + }, + ], +) +def test_pr1669_direct_revalidation_rejects_changed_run_identity(monkeypatch, run): + """A direct candidate must remain a direct run attached to the target PR.""" + monkeypatch.setattr( + sched, + "gh_api_json", + lambda path: run + if "/actions/runs/" in path + else {"state": "open", "draft": False, "head": {"sha": "b" * 40}}, + ) + assert sched._direct_pr_run_still_superseded("owner/repo", 7, "93") is False + + +def test_pr1669_direct_revalidation_allows_genuine_supersession(monkeypatch): + """A genuinely older direct PR run remains cancellable after fresh reads.""" + monkeypatch.setattr( + sched, + "gh_api_json", + lambda path: { + "event": "pull_request", + "status": "in_progress", + "head_sha": "a" * 40, + "pull_requests": [{"number": 7}], + } + if "/actions/runs/" in path + else {"state": "open", "draft": False, "head": {"sha": "b" * 40}}, + ) + assert sched._direct_pr_run_still_superseded("owner/repo", 7, "98") is True + + +def test_pr1669_review_target_rejects_untrusted_dispatch_title(): + """A central dispatch without exact target identity has no cancellation authority.""" + with pytest.raises(ValueError, match="trusted target identity"): + sched._review_run_target_head( + {"event": "repository_dispatch", "display_title": "unrelated"}, + "owner/repo", + "OpenCode Review", + 7, + ) + + +def test_pr1669_review_target_rejects_changed_direct_pr_association(): + """A direct review run must remain attached to the target pull request.""" + with pytest.raises(ValueError, match="target pull request"): + sched._review_run_target_head( + { + "event": "pull_request", + "head_sha": "a" * 40, + "pull_requests": [{"number": 8}], + }, + "owner/repo", + "OpenCode Review", + 7, + ) + + +def test_pr1669_review_target_accepts_direct_and_trusted_dispatch_identity(): + """Direct and trusted central review identities expose validated target heads.""" + assert sched._review_run_target_head( + { + "event": "pull_request", + "head_sha": "a" * 40, + "pull_requests": [{"number": 7}], + }, + "owner/repo", + "OpenCode Review", + 7, + ) == "a" * 40 + assert sched._review_run_target_head( + { + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#7@{'a' * 40}", + }, + "owner/repo", + "OpenCode Review", + 7, + ) == "a" * 40 + + +def test_pr1669_review_revalidation_handles_stale_and_current_heads(monkeypatch): + """Fresh review authority distinguishes genuine supersession from the current head.""" + run = { + "event": "repository_dispatch", + "status": "in_progress", + "display_title": f"Required OpenCode Review owner/repo#7@{'a' * 40}", + } + live_head = {"value": "b" * 40} + + def fake_api(path): + if "/actions/runs/" in path: + return run + return {"state": "open", "draft": False, "head": {"sha": live_head["value"]}} + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + assert sched._review_run_still_superseded( + "owner/repo", "OpenCode Review", 7, "ContextualWisdomLab/.github", "95" + ) is True + live_head["value"] = "a" * 40 + assert sched._review_run_still_superseded( + "owner/repo", "OpenCode Review", 7, "ContextualWisdomLab/.github", "95" + ) is False + + +def test_pr1669_single_direct_candidate_cancels_only_when_revalidated_stale(monkeypatch): + """The direct single-candidate path preserves current and cancels proven stale runs.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr(sched, "stale_pr_run_ids", lambda *_args, **_kwargs: ["97"]) + stale = {"value": False} + monkeypatch.setattr(sched, "_direct_pr_run_still_superseded", lambda *_args: stale["value"]) + cancelled = [] + + def cancel(repo, run_ids): + cancelled.append((repo, run_ids)) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + pr = make_pr(number=7) + assert sched.cancel_stale_pr_runs("owner/repo", pr, dry_run=False) == [] + stale["value"] = True + assert sched.cancel_stale_pr_runs("owner/repo", pr, dry_run=False) == ["97"] + assert cancelled == [("owner/repo", ["97"])] + + +def test_pr1669_single_review_candidate_cancels_only_when_revalidated_stale(monkeypatch): + """The review single-candidate path preserves current and cancels proven stale runs.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_opencode_run_refs", + lambda *_args, **_kwargs: ([], [("ContextualWisdomLab/.github", "96")]), + ) + stale = {"value": False} + monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: stale["value"]) + cancelled = [] + + def cancel(repo, run_ids): + cancelled.append((repo, run_ids)) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + pr = make_pr(number=7) + assert sched.cancel_stale_opencode_runs( + "owner/repo", "OpenCode Review", pr, dry_run=False + ) == [] + stale["value"] = True + assert sched.cancel_stale_opencode_runs( + "owner/repo", "OpenCode Review", pr, dry_run=False + ) == ["96"] + assert cancelled == [("ContextualWisdomLab/.github", ["96"])] + + + +def test_pr1669_opencode_dispatch_preserves_candidate_that_is_current_after_revalidation(monkeypatch): + """OpenCode dispatch must preserve a candidate that became the live current-head run.""" + pr = make_pr(number=7, headRefOid="b" * 40) + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_opencode_run_refs", + lambda *_args, **_kwargs: ([], [("ContextualWisdomLab/.github", "96")]), + ) + monkeypatch.setattr( + sched, + "_review_run_still_superseded", + lambda *_args: False, + raising=False, + ) + direct_cancellations = [] + batch_cancellations = [] + dispatches = [] + monkeypatch.setattr( + sched, + "force_cancel_workflow_runs", + lambda repo, run_ids: direct_cancellations.append((repo, list(run_ids))), + ) + monkeypatch.setattr( + sched, + "force_cancel_workflow_run_refs", + lambda refs: batch_cancellations.append(list(refs)), + raising=False, + ) + monkeypatch.setattr( + sched, + "validated_pr_dispatch_fields", + lambda _pr: ("main", "c" * 40, "b" * 40), + ) + monkeypatch.setattr(sched, "validate_git_ref", lambda value: value) + monkeypatch.setattr(sched, "repository_dispatch_target", lambda _repo: "ContextualWisdomLab/.github") + monkeypatch.setattr(sched, "complete_paginated_pr_contexts", lambda *_args: []) + monkeypatch.setattr(sched, "matching_actions_run_id", lambda *_args: None) + monkeypatch.setattr(sched, "discover_opencode_required_run_id", lambda *_args: None) + monkeypatch.setattr(sched, "run_github_dispatch", lambda *args, **kwargs: dispatches.append((args, kwargs))) + + assert sched.dispatch_opencode_review("owner/repo", "OpenCode Review", pr, dry_run=False) == "already_running" + assert direct_cancellations == [] + assert batch_cancellations == [] + assert dispatches == [] + + +def test_pr1669_strix_dispatch_preserves_candidate_that_is_current_after_revalidation(monkeypatch): + """Strix dispatch must preserve a candidate that became the live current-head run.""" + pr = make_pr(number=7, headRefOid="b" * 40) + monkeypatch.setattr(sched, "matching_actions_job_id", lambda *_args: None) + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_review_run_refs", + lambda *_args, **_kwargs: ([], [("ContextualWisdomLab/.github", "97")]), + ) + monkeypatch.setattr( + sched, + "_review_run_still_superseded", + lambda *_args: False, + raising=False, + ) + direct_cancellations = [] + batch_cancellations = [] + dispatches = [] + monkeypatch.setattr( + sched, + "force_cancel_workflow_runs", + lambda repo, run_ids: direct_cancellations.append((repo, list(run_ids))), + ) + monkeypatch.setattr( + sched, + "force_cancel_workflow_run_refs", + lambda refs: batch_cancellations.append(list(refs)), + raising=False, + ) + monkeypatch.setattr(sched, "active_workflow_runs", lambda *_args, **_kwargs: []) + monkeypatch.setattr(sched, "repository_dispatch_target", lambda _repo: "ContextualWisdomLab/.github") + monkeypatch.setattr( + sched, + "validated_pr_dispatch_fields", + lambda _pr: ("main", "c" * 40, "b" * 40), + ) + monkeypatch.setattr(sched, "run_github_dispatch", lambda *args, **kwargs: dispatches.append((args, kwargs))) + + assert sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", pr, dry_run=False) == "already_running" + assert direct_cancellations == [] + assert batch_cancellations == [] + assert dispatches == [] + + + +def test_pr1669_direct_revalidation_fails_closed_when_live_authority_is_unreadable(monkeypatch, capsys): + """Direct cancellation must preserve the candidate when fresh authority cannot be read.""" + def fail_api(_path): + raise RuntimeError("simulated live-authority outage") + + monkeypatch.setattr(sched, "gh_api_json", fail_api) + assert sched._direct_pr_run_still_superseded("owner/repo", 7, "94") is False + assert "Preserving workflow run 94 in owner/repo" in capsys.readouterr().out + + +def test_pr1669_review_revalidation_fails_closed_when_live_authority_is_unreadable(monkeypatch, capsys): + """Review cancellation must preserve the candidate when fresh authority cannot be read.""" + def fail_api(_path): + raise RuntimeError("simulated live-authority outage") + + monkeypatch.setattr(sched, "gh_api_json", fail_api) + assert sched._review_run_still_superseded( + "owner/repo", "OpenCode Review", 7, "ContextualWisdomLab/.github", "95" + ) is False + assert "Preserving review run ContextualWisdomLab/.github#95" in capsys.readouterr().out + + +def test_pr1669_revalidated_review_refs_cover_empty_and_parallel_mixed_candidates(monkeypatch): + """The review helper preserves uncertain refs and cancels only concurrently proven stale refs.""" + pr = make_pr(number=7, headRefOid="b" * 40) + assert sched._cancel_revalidated_review_run_refs( + "owner/repo", "OpenCode Review", pr, [] + ) == ([], []) + + stale = {"96": True, "97": False} + monkeypatch.setattr( + sched, + "_review_run_still_superseded", + lambda _repo, _workflow, _number, _run_repo, run_id: stale[run_id], + ) + cancelled = [] + + def cancel(repo, run_ids): + cancelled.append((repo, list(run_ids))) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + preserved, cancelled_refs = sched._cancel_revalidated_review_run_refs( + "owner/repo", + "OpenCode Review", + pr, + [ + ("ContextualWisdomLab/.github", "96"), + ("ContextualWisdomLab/.github", "97"), + ], + ) + assert preserved == [("ContextualWisdomLab/.github", "97")] + assert cancelled_refs == [("ContextualWisdomLab/.github", "96")] + assert cancelled == [("ContextualWisdomLab/.github", ["96"])] + + +def test_pr1669_parallel_direct_candidates_preserve_live_and_cancel_only_stale(monkeypatch): + """Parallel direct-run cleanup must keep a revalidated current-head candidate.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr(sched, "stale_pr_run_ids", lambda *_args, **_kwargs: ["94", "95"]) + monkeypatch.setattr( + sched, + "_direct_pr_run_still_superseded", + lambda _repo, _number, run_id: run_id == "94", + ) + cancelled = [] + + def cancel(repo, run_ids): + cancelled.append((repo, list(run_ids))) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + assert sched.cancel_stale_pr_runs("owner/repo", make_pr(number=7), dry_run=False) == ["94"] + assert cancelled == [("owner/repo", ["94"])] + + +def test_pr1669_parallel_opencode_candidates_preserve_live_and_cancel_only_stale(monkeypatch): + """Parallel OpenCode cleanup must keep a revalidated current-head review candidate.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_opencode_run_refs", + lambda *_args, **_kwargs: ( + [], + [ + ("ContextualWisdomLab/.github", "96"), + ("ContextualWisdomLab/.github", "97"), + ], + ), + ) + monkeypatch.setattr( + sched, + "_review_run_still_superseded", + lambda _repo, _workflow, _number, _run_repo, run_id: run_id == "96", + ) + cancelled = [] + + def cancel(repo, run_ids): + cancelled.append((repo, list(run_ids))) + return {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + assert sched.cancel_stale_opencode_runs( + "owner/repo", "OpenCode Review", make_pr(number=7), dry_run=False + ) == ["96"] + assert cancelled == [("ContextualWisdomLab/.github", ["96"])] + + +def test_pr1669_opencode_open_draft_old_head_remains_cancellable(monkeypatch): + """An old OpenCode run on an open draft must not block current-head review-only dispatch.""" + old_head = "a" * 40 + live_head = "b" * 40 + run = { + "event": "repository_dispatch", + "status": "in_progress", + "display_title": f"Required OpenCode Review owner/repo#7@{old_head}", + } + + def fake_api(path): + if "/actions/runs/" in path: + return run + return {"state": "open", "draft": True, "head": {"sha": live_head}} + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + assert sched._review_run_still_superseded( + "owner/repo", "OpenCode Review", 7, "ContextualWisdomLab/.github", "96" + ) is True + + +def test_pr1669_strix_open_draft_old_head_remains_cancellable(monkeypatch): + """An old Strix run on an open draft must not block current-head review-only dispatch.""" + old_head = "a" * 40 + live_head = "b" * 40 + run = { + "event": "repository_dispatch", + "status": "queued", + "display_title": f"Strix Security Scan owner/repo#7@{old_head}", + } + + def fake_api(path): + if "/actions/runs/" in path: + return run + return {"state": "open", "draft": True, "head": {"sha": live_head}} + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + assert sched._review_run_still_superseded( + "owner/repo", "Strix Security Scan", 7, "ContextualWisdomLab/.github", "97" + ) is True