From 7be19d6d9c1ac1be89be90d39a2fcf3991d1ce6e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:40:31 +0000 Subject: [PATCH 1/9] fix: dispatch OpenCode review for a draft PR on explicit mention An explicit @opencode-agent mention on a draft PR resolved and forwarded correctly through agent-mention-router.py and agent-mention-opencode-dispatch.yml (which already hardcodes enable_auto_merge=false, update_branches=false, merge_mode=disabled, trigger_reviews=true -- structurally review-only), but pr_review_merge_scheduler.py's inspect_pr() unconditionally returned "skip: draft PR" before reaching any dispatch logic, so the request was silently discarded and no review ever posted. Add an opt-in --allow-draft-review-dispatch CLI flag (rejected unless --pr-number is also set) and a matching inspect_pr() parameter, gated in the workflow by a new ALLOW_DRAFT_REVIEW_DISPATCH env var derived from client_payload.agent_invocation_key -- a field only the mention dispatch workflow ever sets, so the ordinary multi-PR queue sweep (schedule/push/pull_request_target/pull_request_review/workflow_run) keeps skipping drafts exactly as before. A draft PR reaching the new path goes through dispatch_draft_review_only(), which runs the same Strix-then-OpenCode dispatch gate the ready-PR pipeline uses, then returns immediately -- before any of inspect_pr's unresolved-thread, changes-requested, branch-update, or auto-merge logic, so a draft still cannot be merged, auto-merged, or have its branch updated through this path. --- .../workflows/pr-review-merge-scheduler.yml | 9 + CHANGELOG.md | 21 ++ scripts/ci/pr_review_merge_scheduler.py | 122 ++++++++++++ tests/test_pr_review_merge_scheduler.py | 187 ++++++++++++++++++ 4 files changed, 339 insertions(+) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 456d47db4b..5737411359 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -157,6 +157,12 @@ jobs: MAX_PRS: ${{ github.event.client_payload.max_prs || inputs.max_prs || '100' }} PROJECT_FLOW_INPUT: ${{ github.event.client_payload.project_flow || inputs.project_flow || vars.PROJECT_FLOW || '' }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || github.event.client_payload.pr_number || inputs.pr_number || '' }} + # Only an explicit mention-triggered review-only invocation sets + # agent_invocation_key (see agent-mention-opencode-dispatch.yml); no + # ordinary schedule/push/pull_request_target/pull_request_review/ + # workflow_run trigger does, so a draft PR keeps being skipped by every + # other event path -- this exception is scoped to that one caller. + ALLOW_DRAFT_REVIEW_DISPATCH: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.agent_invocation_key != '' }} TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_run' || github.event_name == 'push' || github.event_name == 'pull_request_target' || (github.event_name == 'repository_dispatch' && github.event.client_payload.trigger_reviews != false) || inputs.trigger_reviews == true }} REVIEW_DISPATCH_LIMIT_INPUT: ${{ github.event.client_payload.review_dispatch_limit || inputs.review_dispatch_limit || vars.REVIEW_DISPATCH_LIMIT || '1' }} BRANCH_UPDATE_LIMIT_INPUT: ${{ github.event.client_payload.branch_update_limit || inputs.branch_update_limit || vars.BRANCH_UPDATE_LIMIT || '1' }} @@ -550,6 +556,9 @@ jobs: if [ -n "$PULL_REQUEST_NUMBER" ]; then args+=(--pr-number "$PULL_REQUEST_NUMBER") fi + if [ "$ALLOW_DRAFT_REVIEW_DISPATCH" = "true" ]; then + args+=(--allow-draft-review-dispatch) + fi if [ "$DRY_RUN" = "true" ]; then args+=(--dry-run) fi diff --git a/CHANGELOG.md b/CHANGELOG.md index fc84661ed6..e7233a8122 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,27 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Let an explicit mention-triggered review request (`@opencode-agent review`) + actually dispatch a current-head OpenCode review for a **draft** PR. + `pr_review_merge_scheduler.py`'s `inspect_pr()` unconditionally returned + `skip: draft PR` before reaching any review-dispatch logic, so + `agent-mention-opencode-dispatch.yml`'s already-structurally-review-only + forward to the scheduler (`trigger_reviews=true`, `enable_auto_merge=false`, + `update_branches=false`, `merge_mode=disabled`) was silently discarded for + drafts: the mention router resolved and forwarded the request correctly, + but the scheduler never posted a review. New opt-in `--allow-draft-review-dispatch` + CLI flag (requires `--pr-number`; rejected otherwise) and `inspect_pr()` + parameter route a draft PR through a new `dispatch_draft_review_only()` + helper that runs the same Strix-then-OpenCode dispatch gate the ready-PR + pipeline uses, then returns immediately — before any of `inspect_pr`'s + unresolved-thread, changes-requested, branch-update, or auto-merge logic, + so a draft still cannot be merged, auto-merged, or have its branch updated + through this path. `pr-review-merge-scheduler.yml`'s `scan-pr-queue` job + sets the new `ALLOW_DRAFT_REVIEW_DISPATCH` flag from + `github.event.client_payload.agent_invocation_key` — a field only the + mention-dispatch workflow ever sets — so the ordinary multi-PR queue sweep + (schedule/push/pull_request_target/pull_request_review/workflow_run) keeps + skipping drafts exactly as before. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index c9804b492e..ead85f5496 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2377,6 +2377,100 @@ def current_head_can_attempt_merge(pr: dict[str, Any], merge_state: str) -> bool return False +def dispatch_draft_review_only( + repo: str, + pr: dict[str, Any], + *, + dry_run: bool, + review_dispatch_allowed: bool, + workflow: str, + security_workflow: str, + stale_opencode_minutes: int, +) -> Decision: + """Dispatch review evidence for one draft PR, never touching merge/branch state. + + An explicit review-only request (a mention invocation, never the ordinary + queue sweep) may reach this for a draft PR. It runs exactly the same + Strix-then-OpenCode dispatch gate the ready-PR pipeline uses below, so a + draft gets the same evidence chain -- but it returns before any of + ``inspect_pr``'s unresolved-thread, changes-requested, branch-update, or + auto-merge logic, so a draft can never be merged, auto-merged, or have its + branch updated by reaching this function. + """ + number = pr["number"] + opencode_state = opencode_progress_state(pr, stale_after_minutes=stale_opencode_minutes) + if opencode_state == "running": + return Decision(number, "wait", "draft PR review-only dispatch; OpenCode review already running") + if opencode_state == "complete": + return Decision( + number, + "skip", + "draft PR review-only dispatch; current-head OpenCode verdict already exists", + ) + strix_state = strix_evidence_state(pr) + if strix_state == "missing": + if not review_dispatch_allowed: + return Decision( + number, + "wait", + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "review dispatch limit reached", + ) + wait_reason = repository_dispatch_wait_reason(repo, security_workflow) + if wait_reason: + return Decision( + number, + "wait", + f"draft PR review-only dispatch; current head has no completed Strix evidence; {wait_reason}", + ) + dispatch_result = dispatch_strix_evidence(repo, security_workflow, pr, dry_run=dry_run) + if dispatch_result == "already_running": + return Decision( + number, "wait", "draft PR review-only dispatch; same-head Strix evidence is still running" + ) + if dispatch_result == "repository_busy": + return Decision( + number, + "wait", + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "target repository already has active Strix evidence", + ) + return Decision( + number, + "security_dispatch", + "draft PR review-only dispatch; current head has no completed Strix evidence; same-head Strix dispatched", + ) + if strix_state == "running": + return Decision(number, "wait", "draft PR review-only dispatch; same-head Strix evidence is still running") + if not review_dispatch_allowed: + return Decision( + number, + "wait", + "draft PR review-only dispatch; current head has completed Strix evidence; " + "review dispatch limit reached", + ) + wait_reason = repository_dispatch_wait_reason(repo, workflow) + if wait_reason: + return Decision( + number, + "wait", + f"draft PR review-only dispatch; current head has completed Strix evidence; {wait_reason}", + ) + dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "already_running": + return Decision( + number, + "wait", + "draft PR review-only dispatch; current head has completed Strix evidence; " + "same-head OpenCode workflow run is already active", + ) + return Decision( + number, + "review_dispatch", + "draft PR review-only dispatch; current head has completed Strix evidence; same-head OpenCode dispatched", + ) + + def inspect_pr( repo: str, pr: dict[str, Any], @@ -2393,12 +2487,23 @@ def inspect_pr( base_branch: str, merge_mode: str = "direct_or_auto", stale_opencode_minutes: int = DEFAULT_STALE_OPENCODE_MINUTES, + allow_draft_review_dispatch: bool = False, ) -> Decision: """Decide and optionally act on one pull request's merge-readiness state.""" number = pr["number"] base_ref = pr.get("baseRefName") if pr.get("isDraft"): + if allow_draft_review_dispatch and trigger_reviews: + return dispatch_draft_review_only( + repo, + pr, + dry_run=dry_run, + review_dispatch_allowed=review_dispatch_allowed, + workflow=workflow, + security_workflow=security_workflow, + stale_opencode_minutes=stale_opencode_minutes, + ) return Decision(number, "skip", "draft PR") cancel_stale_pr_runs(repo, pr, dry_run=dry_run) if base_ref != base_branch: @@ -3936,6 +4041,17 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser.add_argument("--project-flow", default=os.environ.get("PROJECT_FLOW", "")) parser.add_argument("--max-prs", type=int, default=100) parser.add_argument("--pr-number", type=int, default=0) + parser.add_argument( + "--allow-draft-review-dispatch", + action="store_true", + help=( + "Allow a --pr-number draft PR to receive Strix/OpenCode review " + "dispatch. Structurally review-only: never merges, enables " + "auto-merge, or updates the branch. Set only for an explicit " + "single-PR review request (a mention invocation); never for the " + "ordinary multi-PR queue sweep, which must keep skipping drafts." + ), + ) parser.add_argument("--dry-run", action="store_true") parser.add_argument("--trigger-reviews", action=argparse.BooleanOptionalAction, default=True) parser.add_argument( @@ -3994,6 +4110,11 @@ def main(argv: list[str]) -> int: raise SystemExit("--stacked-review-dispatch-limit must be -1 or greater") if args.branch_update_limit < -1: raise SystemExit("--branch-update-limit must be -1 or greater") + if args.allow_draft_review_dispatch and not args.pr_number: + raise SystemExit( + "--allow-draft-review-dispatch requires --pr-number; it is a single-PR " + "review-only exception, never a default for the multi-PR queue sweep" + ) prs = fetch_pr(args.repo, args.pr_number) if args.pr_number else fetch_open_prs(args.repo, args.max_prs) if not args.pr_number: # Stacked PRs have no injected required workflow and depend exclusively @@ -4031,6 +4152,7 @@ def main(argv: list[str]) -> int: security_workflow=args.security_workflow, base_branch=args.base_branch, stale_opencode_minutes=args.stale_opencode_minutes, + allow_draft_review_dispatch=args.allow_draft_review_dispatch, ) except RuntimeError as exc: decision = Decision( diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 6a874ea0be..15dce57bf6 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -3732,6 +3732,178 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): assert called == [("owner/repo", 1, True)] +def test_draft_pr_still_skipped_by_default_and_without_trigger_reviews(): + """The ordinary multi-PR queue sweep never sets allow_draft_review_dispatch, + so a draft PR keeps being skipped exactly as before this feature existed.""" + assert inspect(make_pr(isDraft=True)).action == "skip" + assert inspect(make_pr(isDraft=True)).reason == "draft PR" + allowed_without_trigger = inspect( + make_pr(isDraft=True), allow_draft_review_dispatch=True, trigger_reviews=False + ) + assert allowed_without_trigger.action == "skip" + assert allowed_without_trigger.reason == "draft PR" + + +def test_draft_pr_review_only_dispatch_never_reaches_merge_or_branch_logic(monkeypatch): + """An explicit review-only draft request must never merge, auto-merge, or + update the branch, no matter how merge-ready-looking the fixture is.""" + mutating_calls = [] + for name in ("update_branch", "enable_auto_merge", "merge_pr", "disable_auto_merge_decision"): + if hasattr(sched, name): + monkeypatch.setattr( + sched, name, lambda *args, _name=name, **kwargs: mutating_calls.append(_name) + ) + + draft_pr = make_pr( + isDraft=True, + mergeStateStatus="CLEAN", + restMergeableState="CLEAN", + reviewDecision="APPROVED", + autoMergeRequest={"enabledAt": "now"}, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + ) + decision = inspect(draft_pr, allow_draft_review_dispatch=True) + assert decision.action == "security_dispatch" + assert mutating_calls == [] + + +def test_draft_pr_review_only_dispatch_strix_missing_then_opencode_chain(): + fresh_draft = make_pr(isDraft=True) + security_dispatch = inspect(fresh_draft, allow_draft_review_dispatch=True) + assert security_dispatch.action == "security_dispatch" + assert security_dispatch.reason == ( + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "same-head Strix dispatched" + ) + + strix_complete_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}} + ) + review_dispatch = inspect(strix_complete_draft, allow_draft_review_dispatch=True) + assert review_dispatch.action == "review_dispatch" + assert review_dispatch.reason == ( + "draft PR review-only dispatch; current head has completed Strix evidence; same-head OpenCode dispatched" + ) + + +def test_draft_pr_review_only_dispatch_strix_running_waits(): + running_strix_draft = make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [strix_check(status="IN_PROGRESS")]}}, + ) + decision = inspect(running_strix_draft, allow_draft_review_dispatch=True) + assert decision.action == "wait" + assert decision.reason == "draft PR review-only dispatch; same-head Strix evidence is still running" + + +def test_draft_pr_review_only_dispatch_strix_missing_dispatch_budget_exhausted(): + decision = inspect( + make_pr(isDraft=True), allow_draft_review_dispatch=True, review_dispatch_allowed=False + ) + assert decision.action == "wait" + assert decision.reason == ( + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "review dispatch limit reached" + ) + + +def test_draft_pr_review_only_dispatch_opencode_dispatch_budget_exhausted(): + strix_complete_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}} + ) + decision = inspect( + strix_complete_draft, allow_draft_review_dispatch=True, review_dispatch_allowed=False + ) + assert decision.action == "wait" + assert decision.reason == ( + "draft PR review-only dispatch; current head has completed Strix evidence; " + "review dispatch limit reached" + ) + + +def test_draft_pr_review_only_dispatch_waits_for_central_required_workflow(monkeypatch): + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REF", "main") + monkeypatch.delenv("SCHEDULER_ALLOW_CROSS_REPO_REPOSITORY_DISPATCH", raising=False) + + missing_strix = inspect(make_pr(isDraft=True), allow_draft_review_dispatch=True) + assert missing_strix.action == "wait" + assert "current head has no completed Strix evidence" in missing_strix.reason + assert "no cross-repository repository-dispatch credential" in missing_strix.reason + + strix_complete_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}} + ) + strix_complete = inspect(strix_complete_draft, allow_draft_review_dispatch=True) + assert strix_complete.action == "wait" + assert "current head has completed Strix evidence" in strix_complete.reason + assert "OpenCode Review dispatch waits" in strix_complete.reason + + +def test_draft_pr_review_only_dispatch_waits_when_strix_already_running(monkeypatch): + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda repo, workflow, pr, dry_run: "already_running", + ) + decision = inspect(make_pr(isDraft=True), allow_draft_review_dispatch=True) + assert decision.action == "wait" + assert decision.reason == "draft PR review-only dispatch; same-head Strix evidence is still running" + + +def test_draft_pr_review_only_dispatch_waits_when_repository_is_busy(monkeypatch): + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda repo, workflow, pr, dry_run: "repository_busy", + ) + decision = inspect(make_pr(isDraft=True), allow_draft_review_dispatch=True) + assert decision.action == "wait" + assert decision.reason == ( + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "target repository already has active Strix evidence" + ) + + +def test_draft_pr_review_only_dispatch_waits_when_opencode_already_running(monkeypatch): + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: "already_running", + ) + strix_complete_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}} + ) + decision = inspect(strix_complete_draft, allow_draft_review_dispatch=True) + assert decision.action == "wait" + assert decision.reason == ( + "draft PR review-only dispatch; current head has completed Strix evidence; " + "same-head OpenCode workflow run is already active" + ) + + +def test_draft_pr_review_only_dispatch_opencode_already_running_skips(): + running_opencode_draft = make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [opencode_check(status="IN_PROGRESS")]}}, + ) + decision = inspect(running_opencode_draft, allow_draft_review_dispatch=True) + assert decision.action == "wait" + assert decision.reason == "draft PR review-only dispatch; OpenCode review already running" + + +def test_draft_pr_review_only_dispatch_skips_when_verdict_already_exists(): + complete_opencode_draft = make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [opencode_check(status="COMPLETED")]}}, + ) + decision = inspect(complete_opencode_draft, allow_draft_review_dispatch=True) + assert decision.action == "skip" + assert decision.reason == ( + "draft PR review-only dispatch; current-head OpenCode verdict already exists" + ) + + def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch): runs = [ {"name": "Other", "id": 10, "head_sha": "old", "pull_requests": [{"number": 1}]}, @@ -4872,6 +5044,21 @@ def test_main_rejects_invalid_branch_update_limit(): ) +def test_main_rejects_allow_draft_review_dispatch_without_pr_number(): + with pytest.raises(SystemExit, match="--allow-draft-review-dispatch requires --pr-number"): + sched.main( + [ + "--repo", + "owner/repo", + "--base-branch", + "main", + "--project-flow", + "github-flow", + "--allow-draft-review-dispatch", + ] + ) + + def test_print_summary_self_test_parse_args_and_main(monkeypatch, capsys): sched.print_summary( [sched.Decision(1, "wait", "ready"), sched.Decision(2, "wait", "queued")], From d6bed097c42ea84c3a4118027687b67529c93089 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 13:56:37 +0000 Subject: [PATCH 2/9] fix: two review-follow-up defects in the draft review-only dispatch Devin's review of the initial draft review-only dispatch found two real bugs: 1. dispatch_draft_review_only() treated opencode_progress_state(pr) == "complete" as proof a current-head verdict exists. That state only means a matching check/status reached a terminal state -- the required-workflow gate itself is a terminal, non-running check when it fails closed because no verdict was ever dispatched, so a failed dispatch attempt would permanently block every later explicit retry. Gate the skip on an actual current-head formal review instead (has_current_head_approval/has_current_head_changes_requested), matching the non-draft path's own review-state checks. 2. When Strix evidence is missing, the initial mention dispatches Strix and ends that scheduler run. The Strix-completion workflow_run that follows carries no repository_dispatch client_payload of its own, so ALLOW_DRAFT_REVIEW_DISPATCH is unset on that later pass and the draft PR falls back to being skipped before ever reaching OpenCode -- the chain silently stops after Strix. Add a durable, exact-head-named marker: agent-mention-opencode-dispatch.yml claims a short-lived Actions artifact (retention-days: 1) alongside its existing invocation ledger, and inspect_pr()'s draft branch checks for it (active_draft_review_request()) whenever the CLI flag isn't set, so a later pass over the same exact head -- single-PR or the bulk sweep -- still recognizes and continues the same explicit request through to OpenCode dispatch. --- .../agent-mention-opencode-dispatch.yml | 31 ++++ CHANGELOG.md | 22 +++ scripts/ci/pr_review_merge_scheduler.py | 79 ++++++++- tests/test_pr_review_merge_scheduler.py | 166 +++++++++++++++++- 4 files changed, 289 insertions(+), 9 deletions(-) diff --git a/.github/workflows/agent-mention-opencode-dispatch.yml b/.github/workflows/agent-mention-opencode-dispatch.yml index 6f648fbacb..5f6514221c 100644 --- a/.github/workflows/agent-mention-opencode-dispatch.yml +++ b/.github/workflows/agent-mention-opencode-dispatch.yml @@ -185,6 +185,37 @@ jobs: overwrite: false include-hidden-files: false + - name: Prepare durable draft review-only request marker + if: steps.ledger.outputs.claim == 'true' + id: draft_marker + run: | + set -euo pipefail + # Names this exactly like scripts/ci/pr_review_merge_scheduler.py's + # draft_review_request_artifact_name(repo, pr_number, head_sha) so a + # later scheduler pass with no repository_dispatch client_payload of + # its own (the Strix-completion workflow_run that follows an initial + # security_dispatch) can still recognize this exact explicit request + # is in flight and continue it -- see active_draft_review_request(). + marker_dir="${RUNNER_TEMP}/cwl-draft-review-request" + mkdir -p "$marker_dir" + marker_name="cwl-draft-review-request-${TARGET_REPOSITORY//\//-}-${PR_NUMBER}-${PR_HEAD_SHA}" + printf '{"target_repository":"%s","pr_number":%s,"pr_head_sha":"%s","requested_by":"%s"}\n' \ + "$TARGET_REPOSITORY" "$PR_NUMBER" "$PR_HEAD_SHA" "$REQUESTED_BY" \ + >"$marker_dir/marker.json" + printf 'marker_name=%s\n' "$marker_name" >>"$GITHUB_OUTPUT" + + - name: Claim durable draft review-only request marker + if: steps.ledger.outputs.claim == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: ${{ steps.draft_marker.outputs.marker_name }} + path: ${{ runner.temp }}/cwl-draft-review-request/marker.json + if-no-files-found: error + retention-days: 1 + compression-level: 0 + overwrite: true + include-hidden-files: false + - name: Forward once to the authoritative review-only scheduler if: steps.ledger.outputs.claim == 'true' run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index e7233a8122..00fa4c61e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,28 @@ Semantic Versioning where the repository publishes a release. mention-dispatch workflow ever sets — so the ordinary multi-PR queue sweep (schedule/push/pull_request_target/pull_request_review/workflow_run) keeps skipping drafts exactly as before. + Two follow-up fixes from adversarial review before this shipped: + - `dispatch_draft_review_only()` treated `opencode_progress_state(pr) == "complete"` + (a matching check/status reached a terminal state) as proof a verdict + exists. That state does not distinguish a posted review from the + required-workflow gate's own terminal failure when no verdict was ever + dispatched, so a failed dispatch attempt would permanently block every + later explicit retry. Now gated on an actual current-head formal review + (`has_current_head_approval`/`has_current_head_changes_requested`), + matching the non-draft path's own review-state checks. + - When Strix evidence is missing, the initial mention dispatches Strix and + ends that scheduler run; the Strix-completion `workflow_run` that follows + carries no `repository_dispatch` `client_payload` of its own, so + `ALLOW_DRAFT_REVIEW_DISPATCH` would be unset on that later pass and the + draft would fall back to being skipped before ever reaching OpenCode. + `agent-mention-opencode-dispatch.yml` now also claims a short-lived + (`retention-days: 1`), exact-head-named Actions artifact + (`cwl-draft-review-request---`) alongside its existing + invocation ledger; `inspect_pr()`'s draft branch checks for this durable + marker (`active_draft_review_request()`) whenever the CLI flag isn't set, + so a later pass over the same exact head — reached via the ordinary + `workflow_run` trigger, single-PR or the bulk sweep — still recognizes + and continues the same explicit request through to OpenCode dispatch. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index ead85f5496..1f1c7bfaac 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2377,6 +2377,72 @@ def current_head_can_attempt_merge(pr: dict[str, Any], merge_state: str) -> bool return False +def draft_review_request_artifact_name(repo: str, pr_number: int, head_sha: str) -> str: + """Return one draft review-only request marker's exact artifact name.""" + return f"cwl-draft-review-request-{repo.replace('/', '-')}-{pr_number}-{head_sha}" + + +def _draft_review_request_records(value: Any, *, expected_name: str) -> tuple[dict[str, Any], ...]: + """Validate one exact-name repository artifact response and return live records. + + The server-side ``name`` filter makes this response directly addressable by + PR and exact head. Any malformed, mismatched, truncated, or ambiguous + response fails closed rather than being interpreted as an active request. + """ + if not isinstance(value, dict): + raise ValueError("artifact response must be an object") + total_count = value.get("total_count") + artifacts = value.get("artifacts") + if type(total_count) is not int or total_count < 0: + raise ValueError("artifact response has an invalid total_count") + if not isinstance(artifacts, list): + raise ValueError("artifact response has an invalid artifacts collection") + if total_count != len(artifacts): + raise ValueError("artifact response is truncated or internally inconsistent") + live: list[dict[str, Any]] = [] + for artifact in artifacts: + if not isinstance(artifact, dict): + raise ValueError("artifact response contains a non-object record") + artifact_id = artifact.get("id") + name = artifact.get("name") + expired = artifact.get("expired") + if type(artifact_id) is not int or artifact_id < 1: + raise ValueError("artifact response contains an invalid artifact id") + if not isinstance(name, str) or name != expected_name: + raise ValueError("artifact response contains a mismatched artifact name") + if type(expired) is not bool: + raise ValueError("artifact response contains an invalid expired flag") + if not expired: + live.append(artifact) + return tuple(live) + + +def active_draft_review_request(repo: str, pr: dict[str, Any]) -> bool: + """Return whether an explicit draft review-only request is active for this head. + + ``agent-mention-opencode-dispatch.yml`` uploads one short-lived artifact per + mention invocation in the central automation repository (the same + repository ``repository_dispatch`` review dispatch always targets, per + :func:`repository_dispatch_target`). The initial mention's own scheduler + pass reaches :func:`dispatch_draft_review_only` through the CLI's + ``--allow-draft-review-dispatch`` flag; a later pass over the same draft + PR -- most commonly the Strix-completion ``workflow_run`` that follows an + initial ``security_dispatch``, which carries no ``repository_dispatch`` + ``client_payload`` of its own -- has no such flag, so it checks here + instead to recognize the same explicit request is still in flight for + this exact head. + """ + head_sha = pr.get("headRefOid") + if not isinstance(head_sha, str) or not head_sha: + return False + dispatch_repo = repository_dispatch_target(validate_github_repository(repo)) + artifact_name = draft_review_request_artifact_name(repo, pr["number"], head_sha) + response = gh_api_json( + f"repos/{dispatch_repo}/actions/artifacts?name={artifact_name}&per_page=100" + ) + return bool(_draft_review_request_records(response, expected_name=artifact_name)) + + def dispatch_draft_review_only( repo: str, pr: dict[str, Any], @@ -2401,7 +2467,14 @@ def dispatch_draft_review_only( opencode_state = opencode_progress_state(pr, stale_after_minutes=stale_opencode_minutes) if opencode_state == "running": return Decision(number, "wait", "draft PR review-only dispatch; OpenCode review already running") - if opencode_state == "complete": + # opencode_state == "complete" means a matching check/status reached a + # terminal state -- it does not mean opencode-agent posted a review. The + # required-workflow gate itself fails closed (a terminal, non-running + # check) whenever no verdict was ever dispatched, so treating "complete" + # alone as a verdict would make a failed dispatch attempt permanently + # block every later explicit retry. Only an actual current-head formal + # review is a verdict. + if has_current_head_approval(pr) or has_current_head_changes_requested(pr): return Decision( number, "skip", @@ -2494,7 +2567,9 @@ def inspect_pr( base_ref = pr.get("baseRefName") if pr.get("isDraft"): - if allow_draft_review_dispatch and trigger_reviews: + if trigger_reviews and ( + allow_draft_review_dispatch or active_draft_review_request(repo, pr) + ): return dispatch_draft_review_only( repo, pr, diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 15dce57bf6..80f3343b27 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -3218,6 +3218,7 @@ def test_summary_section_helpers_handle_empty_and_action_error_cases(): def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): + monkeypatch.setattr(sched, "active_draft_review_request", lambda repo, pr: False) assert inspect(make_pr(isDraft=True)).action == "skip" stacked = inspect(make_pr(baseRefName="develop")) assert stacked.action == "review_dispatch" @@ -3732,9 +3733,11 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): assert called == [("owner/repo", 1, True)] -def test_draft_pr_still_skipped_by_default_and_without_trigger_reviews(): - """The ordinary multi-PR queue sweep never sets allow_draft_review_dispatch, - so a draft PR keeps being skipped exactly as before this feature existed.""" +def test_draft_pr_still_skipped_by_default_and_without_trigger_reviews(monkeypatch): + """The ordinary multi-PR queue sweep never sets allow_draft_review_dispatch + and has no active request marker, so a draft PR keeps being skipped + exactly as before this feature existed.""" + monkeypatch.setattr(sched, "active_draft_review_request", lambda repo, pr: False) assert inspect(make_pr(isDraft=True)).action == "skip" assert inspect(make_pr(isDraft=True)).reason == "draft PR" allowed_without_trigger = inspect( @@ -3744,6 +3747,120 @@ def test_draft_pr_still_skipped_by_default_and_without_trigger_reviews(): assert allowed_without_trigger.reason == "draft PR" +def test_draft_pr_review_request_marker_continues_dispatch_without_the_cli_flag(monkeypatch): + """A later scheduler pass with no repository_dispatch client_payload of its + own (the Strix-completion workflow_run that follows an initial + security_dispatch) still continues the same explicit request when a live + marker exists for this exact head, without --allow-draft-review-dispatch.""" + seen = [] + monkeypatch.setattr( + sched, + "active_draft_review_request", + lambda repo, pr: seen.append((repo, pr["number"])) or True, + ) + strix_complete_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}} + ) + decision = inspect(strix_complete_draft) + assert decision.action == "review_dispatch" + assert seen == [("owner/repo", 1)] + + +def test_draft_pr_review_request_marker_not_checked_when_flag_already_allows(monkeypatch): + """The CLI flag short-circuits the marker lookup entirely -- no live call + is needed when the caller already explicitly allowed draft dispatch.""" + monkeypatch.setattr( + sched, + "active_draft_review_request", + lambda repo, pr: (_ for _ in ()).throw(AssertionError("must not be called")), + ) + decision = inspect(make_pr(isDraft=True), allow_draft_review_dispatch=True) + assert decision.action == "security_dispatch" + + +def test_draft_review_request_artifact_name_is_exact_and_stable(): + assert sched.draft_review_request_artifact_name("owner/repo", 42, "a" * 40) == ( + f"cwl-draft-review-request-owner-repo-42-{'a' * 40}" + ) + + +def test_active_draft_review_request_queries_the_central_dispatch_repository(monkeypatch): + calls = [] + + def fake_gh_api_json(path): + calls.append(path) + return {"total_count": 0, "artifacts": []} + + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") + monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is False + assert len(calls) == 1 + assert calls[0].startswith("repos/ContextualWisdomLab/.github/actions/artifacts?name=") + expected_name = sched.draft_review_request_artifact_name("owner/repo", 1, "b" * 40) + assert expected_name in calls[0] + + +def test_active_draft_review_request_true_when_a_live_artifact_matches(monkeypatch): + def fake_gh_api_json(path): + expected_name = sched.draft_review_request_artifact_name("owner/repo", 1, "b" * 40) + return { + "total_count": 1, + "artifacts": [{"id": 7, "name": expected_name, "expired": False}], + } + + monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is True + + +def test_active_draft_review_request_false_when_the_artifact_expired(monkeypatch): + def fake_gh_api_json(path): + expected_name = sched.draft_review_request_artifact_name("owner/repo", 1, "b" * 40) + return { + "total_count": 1, + "artifacts": [{"id": 7, "name": expected_name, "expired": True}], + } + + monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is False + + +def test_active_draft_review_request_false_without_a_head_sha(): + assert sched.active_draft_review_request("owner/repo", make_pr(headRefOid=None)) is False + + +def test_draft_review_request_records_fail_closed_on_malformed_responses(): + name = "cwl-draft-review-request-owner-repo-1-" + "a" * 40 + with pytest.raises(ValueError, match="must be an object"): + sched._draft_review_request_records([], expected_name=name) + with pytest.raises(ValueError, match="invalid total_count"): + sched._draft_review_request_records({"total_count": -1, "artifacts": []}, expected_name=name) + with pytest.raises(ValueError, match="invalid artifacts collection"): + sched._draft_review_request_records({"total_count": 0, "artifacts": None}, expected_name=name) + with pytest.raises(ValueError, match="truncated or internally inconsistent"): + sched._draft_review_request_records({"total_count": 1, "artifacts": []}, expected_name=name) + with pytest.raises(ValueError, match="non-object record"): + sched._draft_review_request_records({"total_count": 1, "artifacts": [None]}, expected_name=name) + with pytest.raises(ValueError, match="invalid artifact id"): + sched._draft_review_request_records( + {"total_count": 1, "artifacts": [{"id": 0, "name": name, "expired": False}]}, + expected_name=name, + ) + with pytest.raises(ValueError, match="mismatched artifact name"): + sched._draft_review_request_records( + {"total_count": 1, "artifacts": [{"id": 1, "name": "other", "expired": False}]}, + expected_name=name, + ) + with pytest.raises(ValueError, match="invalid expired flag"): + sched._draft_review_request_records( + {"total_count": 1, "artifacts": [{"id": 1, "name": name, "expired": "no"}]}, + expected_name=name, + ) + + def test_draft_pr_review_only_dispatch_never_reaches_merge_or_branch_logic(monkeypatch): """An explicit review-only draft request must never merge, auto-merge, or update the branch, no matter how merge-ready-looking the fixture is.""" @@ -3763,7 +3880,18 @@ def test_draft_pr_review_only_dispatch_never_reaches_merge_or_branch_logic(monke reviews={"nodes": [opencode_review("APPROVED", "head")]}, ) decision = inspect(draft_pr, allow_draft_review_dispatch=True) - assert decision.action == "security_dispatch" + assert decision.action == "skip" + assert mutating_calls == [] + + unreviewed_draft_pr = make_pr( + isDraft=True, + mergeStateStatus="CLEAN", + restMergeableState="CLEAN", + reviewDecision="APPROVED", + autoMergeRequest={"enabledAt": "now"}, + ) + unreviewed_decision = inspect(unreviewed_draft_pr, allow_draft_review_dispatch=True) + assert unreviewed_decision.action == "security_dispatch" assert mutating_calls == [] @@ -3892,17 +4020,41 @@ def test_draft_pr_review_only_dispatch_opencode_already_running_skips(): assert decision.reason == "draft PR review-only dispatch; OpenCode review already running" -def test_draft_pr_review_only_dispatch_skips_when_verdict_already_exists(): - complete_opencode_draft = make_pr( +def test_draft_pr_review_only_dispatch_skips_when_a_current_head_verdict_exists(): + approved_draft = make_pr( isDraft=True, statusCheckRollup={"contexts": {"nodes": [opencode_check(status="COMPLETED")]}}, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, ) - decision = inspect(complete_opencode_draft, allow_draft_review_dispatch=True) + decision = inspect(approved_draft, allow_draft_review_dispatch=True) assert decision.action == "skip" assert decision.reason == ( "draft PR review-only dispatch; current-head OpenCode verdict already exists" ) + changes_requested_draft = make_pr( + isDraft=True, + reviews={"nodes": [opencode_review("CHANGES_REQUESTED", "head")]}, + ) + changes_requested_decision = inspect(changes_requested_draft, allow_draft_review_dispatch=True) + assert changes_requested_decision.action == "skip" + assert changes_requested_decision.reason == ( + "draft PR review-only dispatch; current-head OpenCode verdict already exists" + ) + + +def test_draft_pr_review_only_dispatch_retries_a_failed_required_check_with_no_verdict(): + """A completed-but-failed required-workflow check is not a posted review: + the explicit request must still be able to redispatch (the exact defect + this feature exists to fix -- a required-check-only failure must never + look like a satisfied verdict).""" + failed_gate_draft = make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [opencode_check(status="COMPLETED")]}}, + ) + decision = inspect(failed_gate_draft, allow_draft_review_dispatch=True) + assert decision.action == "security_dispatch" + def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch): runs = [ From ce6b7f8c088e9c5649e882b21564c593a00f2963 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:06:39 +0000 Subject: [PATCH 3/9] security: stop trusting bare agent_invocation_key presence for drafts Devin flagged a real gap in the same commit that added the marker-based continuation: pr-review-merge-scheduler.yml's ALLOW_DRAFT_REVIEW_DISPATCH env var was derived purely from client_payload.agent_invocation_key being nonempty on a merge-scheduler repository_dispatch event. The scheduler never verified that key, so: 1. (security) any dispatch-capable caller could fire a merge-scheduler repository_dispatch with an arbitrary nonempty agent_invocation_key for an arbitrary target repository/PR and get an unrequested draft review dispatched -- the flag proved nothing about the request's legitimacy. 2. (bug) the flag also wasn't bound to a specific head, so a draft that gained new commits after being mentioned would have the newer, unrequested commit reviewed instead of being rejected. Remove ALLOW_DRAFT_REVIEW_DISPATCH and its CLI pass-through entirely. active_draft_review_request()'s artifact marker (added alongside this env var) already provides everything the env var only pretended to: it's created by agent-mention-opencode-dispatch.yml only after that workflow's own HMAC-style canonical-payload check has validated the invocation, and its name is bound to the exact PR and head SHA -- so a forged dispatch has no matching artifact, and a stale mention's marker simply doesn't match a since-changed head. It is now the sole automatic gate for draft review dispatch; --allow-draft-review-dispatch remains only as a manual, direct-CLI operator override. --- .../workflows/pr-review-merge-scheduler.yml | 9 ---- CHANGELOG.md | 33 ++++++++++----- scripts/ci/pr_review_merge_scheduler.py | 42 ++++++++++++------- 3 files changed, 51 insertions(+), 33 deletions(-) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 5737411359..456d47db4b 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -157,12 +157,6 @@ jobs: MAX_PRS: ${{ github.event.client_payload.max_prs || inputs.max_prs || '100' }} PROJECT_FLOW_INPUT: ${{ github.event.client_payload.project_flow || inputs.project_flow || vars.PROJECT_FLOW || '' }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || github.event.client_payload.pr_number || inputs.pr_number || '' }} - # Only an explicit mention-triggered review-only invocation sets - # agent_invocation_key (see agent-mention-opencode-dispatch.yml); no - # ordinary schedule/push/pull_request_target/pull_request_review/ - # workflow_run trigger does, so a draft PR keeps being skipped by every - # other event path -- this exception is scoped to that one caller. - ALLOW_DRAFT_REVIEW_DISPATCH: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.agent_invocation_key != '' }} TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_run' || github.event_name == 'push' || github.event_name == 'pull_request_target' || (github.event_name == 'repository_dispatch' && github.event.client_payload.trigger_reviews != false) || inputs.trigger_reviews == true }} REVIEW_DISPATCH_LIMIT_INPUT: ${{ github.event.client_payload.review_dispatch_limit || inputs.review_dispatch_limit || vars.REVIEW_DISPATCH_LIMIT || '1' }} BRANCH_UPDATE_LIMIT_INPUT: ${{ github.event.client_payload.branch_update_limit || inputs.branch_update_limit || vars.BRANCH_UPDATE_LIMIT || '1' }} @@ -556,9 +550,6 @@ jobs: if [ -n "$PULL_REQUEST_NUMBER" ]; then args+=(--pr-number "$PULL_REQUEST_NUMBER") fi - if [ "$ALLOW_DRAFT_REVIEW_DISPATCH" = "true" ]; then - args+=(--allow-draft-review-dispatch) - fi if [ "$DRY_RUN" = "true" ]; then args+=(--dry-run) fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 00fa4c61e9..8b41c631f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ Semantic Versioning where the repository publishes a release. mention-dispatch workflow ever sets — so the ordinary multi-PR queue sweep (schedule/push/pull_request_target/pull_request_review/workflow_run) keeps skipping drafts exactly as before. - Two follow-up fixes from adversarial review before this shipped: + Three follow-up fixes from adversarial review before this shipped: - `dispatch_draft_review_only()` treated `opencode_progress_state(pr) == "complete"` (a matching check/status reached a terminal state) as proof a verdict exists. That state does not distinguish a posted review from the @@ -37,17 +37,30 @@ Semantic Versioning where the repository publishes a release. matching the non-draft path's own review-state checks. - When Strix evidence is missing, the initial mention dispatches Strix and ends that scheduler run; the Strix-completion `workflow_run` that follows - carries no `repository_dispatch` `client_payload` of its own, so - `ALLOW_DRAFT_REVIEW_DISPATCH` would be unset on that later pass and the - draft would fall back to being skipped before ever reaching OpenCode. - `agent-mention-opencode-dispatch.yml` now also claims a short-lived + carries no `repository_dispatch` `client_payload` of its own, so the + first design's env-var-driven flag would be unset on that later pass and + the draft would fall back to being skipped before ever reaching OpenCode. + `agent-mention-opencode-dispatch.yml` now claims a short-lived (`retention-days: 1`), exact-head-named Actions artifact (`cwl-draft-review-request---`) alongside its existing - invocation ledger; `inspect_pr()`'s draft branch checks for this durable - marker (`active_draft_review_request()`) whenever the CLI flag isn't set, - so a later pass over the same exact head — reached via the ordinary - `workflow_run` trigger, single-PR or the bulk sweep — still recognizes - and continues the same explicit request through to OpenCode dispatch. + invocation ledger, only after its own HMAC-style canonical-payload check + has already validated the invocation; `inspect_pr()`'s draft branch + checks for this durable marker (`active_draft_review_request()`), so a + later pass over the same exact head — the ordinary `workflow_run` + trigger, single-PR or the bulk sweep — still recognizes and continues + the same explicit request through to OpenCode dispatch. + - The first design's `ALLOW_DRAFT_REVIEW_DISPATCH` env var trusted the mere + *presence* of `client_payload.agent_invocation_key` on a `merge-scheduler` + `repository_dispatch` event as proof of a legitimate mention, without + verifying the key or binding it to a specific head. Any dispatch-capable + caller could supply an arbitrary nonempty string for an arbitrary target + repository/PR to get an unrequested draft review dispatched, and a + genuinely stale mention (new commits landed after the request) would + review a commit nobody asked about. Removed that env var and its CLI + pass-through entirely — `active_draft_review_request()`'s cryptographically + gated, exact-head-named artifact marker (above) is now the sole automatic + gate; `--allow-draft-review-dispatch` remains only as a manual, + direct-CLI operator override. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 1f1c7bfaac..301c6a69ba 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2420,17 +2420,26 @@ def _draft_review_request_records(value: Any, *, expected_name: str) -> tuple[di def active_draft_review_request(repo: str, pr: dict[str, Any]) -> bool: """Return whether an explicit draft review-only request is active for this head. - ``agent-mention-opencode-dispatch.yml`` uploads one short-lived artifact per - mention invocation in the central automation repository (the same - repository ``repository_dispatch`` review dispatch always targets, per - :func:`repository_dispatch_target`). The initial mention's own scheduler - pass reaches :func:`dispatch_draft_review_only` through the CLI's - ``--allow-draft-review-dispatch`` flag; a later pass over the same draft - PR -- most commonly the Strix-completion ``workflow_run`` that follows an - initial ``security_dispatch``, which carries no ``repository_dispatch`` - ``client_payload`` of its own -- has no such flag, so it checks here - instead to recognize the same explicit request is still in flight for - this exact head. + This is the sole automatic gate for draft review dispatch. A bare + ``repository_dispatch`` ``client_payload`` field (an invocation key, a PR + number) is never trusted on its own: any dispatch-capable caller could + supply one for an arbitrary target, and a genuinely stale mention (the + draft gained a new commit after being requested) must not review a + commit nobody asked about. ``agent-mention-opencode-dispatch.yml`` + instead uploads one short-lived Actions artifact per mention invocation, + named with the exact PR and head SHA + (:func:`draft_review_request_artifact_name`), only after that workflow's + own HMAC-style canonical-payload check has already validated the + invocation -- so a live artifact is itself the validated proof, bound to + one exact head, that this specific mention was genuine. The artifact + lives in the central automation repository (the same repository + ``repository_dispatch`` review dispatch always targets, per + :func:`repository_dispatch_target`), so every scheduler pass over this + draft PR -- the initial mention-triggered run and any later pass with no + ``repository_dispatch`` ``client_payload`` of its own, most commonly the + Strix-completion ``workflow_run`` that follows an initial + ``security_dispatch`` -- checks the same durable signal here rather than + trusting anything the triggering event itself claims. """ head_sha = pr.get("headRefOid") if not isinstance(head_sha, str) or not head_sha: @@ -4122,9 +4131,14 @@ def parse_args(argv: list[str]) -> argparse.Namespace: help=( "Allow a --pr-number draft PR to receive Strix/OpenCode review " "dispatch. Structurally review-only: never merges, enables " - "auto-merge, or updates the branch. Set only for an explicit " - "single-PR review request (a mention invocation); never for the " - "ordinary multi-PR queue sweep, which must keep skipping drafts." + "auto-merge, or updates the branch. A manual operator override " + "for direct CLI use only -- no caller-supplied signal reaching " + "this script (repository_dispatch client_payload included) is " + "trusted to set this automatically, because it cannot be bound " + "to a specific validated request. The production automatic path " + "is inspect_pr()'s own active_draft_review_request() marker " + "check, gated on a cryptographically validated, exact-head-named " + "artifact that only a legitimate mention invocation can create." ), ) parser.add_argument("--dry-run", action="store_true") From 48ebd77815d672d80c78b6f624a12b12ee455658 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:18:51 +0000 Subject: [PATCH 4/9] Fix strix_evidence_state() to fail closed on non-passing terminal evidence strix_evidence_state() only checked status/state for running-vs-terminal and never inspected the CheckRun conclusion or classic commit-status terminal value, so any terminal outcome (success, failure, error, cancelled, timed_out, skipped, neutral, action_required, stale, startup_failure) satisfied the same "complete" gate. That let non-passing Strix evidence unlock OpenCode dispatch on both the draft review-only path and the ordinary scheduler path. Add a new "failed" state distinguishing authoritative success from any other terminal outcome, and update the three affected call sites (post_update_branch_followup, dispatch_draft_review_only, and the main non-draft inspect_pr Strix-then-OpenCode chain) to treat "failed" exactly like "missing": dispatch a fresh Strix attempt, never fall through to OpenCode. Add exhaustive regression fixtures for every non-passing terminal conclusion/state plus authoritative success, across both CheckRun and classic commit-status shapes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- CHANGELOG.md | 20 ++++++ scripts/ci/pr_review_merge_scheduler.py | 33 +++++++-- tests/test_pr_review_merge_scheduler.py | 92 +++++++++++++++++++++++-- 3 files changed, 134 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b41c631f8..5ce3cff5c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,26 @@ Semantic Versioning where the repository publishes a release. gated, exact-head-named artifact marker (above) is now the sole automatic gate; `--allow-draft-review-dispatch` remains only as a manual, direct-CLI operator override. + - `strix_evidence_state()` classified *any* terminal Strix check-run or + commit-status as `"complete"` because it only ever inspected `status` + (CheckRun) / whether a value was present (classic status) to tell + running from terminal, never the actual `conclusion` (CheckRun) or + terminal `state` value (classic status). A terminal `FAILURE`, `ERROR`, + `CANCELLED`, `TIMED_OUT`, `SKIPPED`, `NEUTRAL`, `ACTION_REQUIRED`, + `STALE`, or `STARTUP_FAILURE` outcome therefore satisfied the same gate + as an authoritative `SUCCESS`, letting non-passing Strix evidence unlock + OpenCode dispatch on both the draft review-only path and the ordinary + scheduler path. The function now returns a new `"failed"` state whenever + Strix evidence is terminal but not an authoritative success, and every + call site (`post_update_branch_followup`, `dispatch_draft_review_only`, + and the main non-draft `inspect_pr` Strix-then-OpenCode chain) treats + `"failed"` exactly like `"missing"`: it dispatches a fresh Strix attempt + and never falls through to OpenCode on that non-authoritative evidence. + Fails closed by design: any single non-success terminal context marks + the whole gate `"failed"` even alongside a successful one. Added + exhaustive regression fixtures for every non-passing terminal + conclusion/state plus authoritative success, for both CheckRun and + classic commit-status shapes. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 301c6a69ba..923161e65f 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1148,9 +1148,20 @@ def opencode_in_progress(pr: dict[str, Any], *, stale_after_minutes: int | None return opencode_progress_state(pr, stale_after_minutes=stale_after) == "running" +_STRIX_SUCCESS_CONCLUSIONS = {"SUCCESS"} + + def strix_evidence_state(pr: dict[str, Any]) -> str: - """Return missing, running, or complete for current-head Strix evidence.""" + """Return missing, running, failed, or complete for current-head Strix evidence. + + "complete" requires authoritative success (CheckRun conclusion or classic + commit-status state of SUCCESS). Any other terminal outcome -- failure, + error, cancelled, timed out, skipped, neutral, action_required, stale, + startup_failure -- is reported as "failed" rather than "complete" so + callers fail closed instead of unlocking on non-passing evidence. + """ found = False + saw_failure = False for node in context_nodes(pr): if not is_strix_context(node): continue @@ -1158,9 +1169,17 @@ def strix_evidence_state(pr: dict[str, Any]) -> str: status = (node.get("status") or node.get("state") or "").upper() if status in RUNNING_CHECK_STATES: return "running" - if node.get("__typename") == "CheckRun" and status != "COMPLETED": - return "running" - return "complete" if found else "missing" + if node.get("__typename") == "CheckRun": + if status != "COMPLETED": + return "running" + conclusion = (node.get("conclusion") or "").upper() + if conclusion not in _STRIX_SUCCESS_CONCLUSIONS: + saw_failure = True + elif status not in _STRIX_SUCCESS_CONCLUSIONS: + saw_failure = True + if not found: + return "missing" + return "failed" if saw_failure else "complete" def unresolved_thread_count(pr: dict[str, Any]) -> int: @@ -1842,7 +1861,7 @@ def post_update_branch_followup( return f"{head_note}; review dispatch limit reached, so no same-head evidence workflow was dispatched" strix_state = strix_evidence_state(updated_pr) - if strix_state == "missing": + if strix_state in {"missing", "failed"}: wait_reason = repository_dispatch_wait_reason(repo, security_workflow) if wait_reason: return f"{head_note}; {wait_reason}" @@ -2490,7 +2509,7 @@ def dispatch_draft_review_only( "draft PR review-only dispatch; current-head OpenCode verdict already exists", ) strix_state = strix_evidence_state(pr) - if strix_state == "missing": + if strix_state in {"missing", "failed"}: if not review_dispatch_allowed: return Decision( number, @@ -3051,7 +3070,7 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio if trigger_reviews: strix_state = strix_evidence_state(pr) - if strix_state == "missing": + if strix_state in {"missing", "failed"}: if not review_dispatch_allowed: return decide( "wait", diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 80f3343b27..88ac1e6a6a 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1036,10 +1036,27 @@ def test_context_review_and_check_helpers(monkeypatch): ) assert sched.strix_evidence_state(unknown_running) == "running" assert sched.strix_evidence_state(make_pr(statusCheckRollup={"contexts": {"nodes": [strix_check()]}})) == "complete" - assert ( - sched.strix_evidence_state(make_pr(statusCheckRollup={"contexts": {"nodes": [strix_check(conclusion="FAILURE")]}})) - == "complete" - ) + for terminal_conclusion in ( + "FAILURE", + "ERROR", + "CANCELLED", + "TIMED_OUT", + "SKIPPED", + "NEUTRAL", + "ACTION_REQUIRED", + "STALE", + "STARTUP_FAILURE", + ): + non_passing = make_pr( + statusCheckRollup={"contexts": {"nodes": [strix_check(conclusion=terminal_conclusion)]}} + ) + assert sched.strix_evidence_state(non_passing) == "failed", terminal_conclusion + classic_failure = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "FAILURE"}]}}) + assert sched.strix_evidence_state(classic_failure) == "failed" + classic_error = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "ERROR"}]}}) + assert sched.strix_evidence_state(classic_error) == "failed" + classic_success = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "SUCCESS"}]}}) + assert sched.strix_evidence_state(classic_success) == "complete" threaded = make_pr( reviewThreads={ @@ -3001,6 +3018,18 @@ def test_inspect_pr_reports_stale_approval_cleanup_in_final_decision(): ) +def test_inspect_pr_treats_failed_strix_like_missing_and_never_dispatches_opencode(): + """A terminal but non-passing Strix conclusion must fail closed: a fresh + Strix attempt is dispatched, exactly as for missing evidence, and + OpenCode is never reached on that non-authoritative evidence.""" + failed_strix = make_pr( + statusCheckRollup={"contexts": {"nodes": [strix_check(conclusion="FAILURE")]}} + ) + decision = inspect(failed_strix) + assert decision.action == "security_dispatch" + assert decision.reason == "current head has no completed Strix evidence; same-head Strix dispatched" + + def test_dismiss_pull_request_review_logs_mutation_failures(monkeypatch, capsys): def fail(_args, stdin=None): raise RuntimeError("Resource not accessible by integration") @@ -3914,6 +3943,21 @@ def test_draft_pr_review_only_dispatch_strix_missing_then_opencode_chain(): ) +def test_draft_pr_review_only_dispatch_treats_failed_strix_like_missing(): + """A terminal but non-passing Strix conclusion on a draft review-only + request must fail closed the same as missing evidence: a fresh Strix + attempt is dispatched and OpenCode is never reached.""" + failed_strix_draft = make_pr( + isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check(conclusion="FAILURE")]}} + ) + decision = inspect(failed_strix_draft, allow_draft_review_dispatch=True) + assert decision.action == "security_dispatch" + assert decision.reason == ( + "draft PR review-only dispatch; current head has no completed Strix evidence; " + "same-head Strix dispatched" + ) + + def test_draft_pr_review_only_dispatch_strix_running_waits(): running_strix_draft = make_pr( isDraft=True, @@ -4423,6 +4467,46 @@ def followup(updated_pr, **overrides): ) +def test_post_update_branch_followup_treats_failed_strix_like_missing(monkeypatch): + """A terminal but non-passing Strix conclusion after a branch update must + fail closed the same as missing evidence: a fresh Strix attempt is + dispatched, and OpenCode is never reached on that non-authoritative + evidence.""" + original = make_pr(headRefOid="old-head") + updated = make_pr( + headRefOid="new-head", + statusCheckRollup={"contexts": {"nodes": [strix_check(conclusion="FAILURE")]}}, + ) + monkeypatch.setattr(sched, "wait_for_updated_branch_head", lambda repo, pr: updated) + strix_dispatched = [] + opencode_dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda repo, workflow, pr, dry_run: strix_dispatched.append(pr["headRefOid"]), + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: opencode_dispatched.append(pr["headRefOid"]), + ) + + note = sched.post_update_branch_followup( + "owner/repo", + original, + dry_run=False, + trigger_reviews=True, + review_dispatch_allowed=True, + workflow="OpenCode Review", + security_workflow="Strix Security Scan", + stale_opencode_minutes=45, + ) + + assert "same-head Strix evidence dispatched" in note + assert strix_dispatched == ["new-head"] + assert opencode_dispatched == [] + + def test_post_update_branch_followup_dismisses_stale_approval_before_dispatch(monkeypatch): original = make_pr(headRefOid="old-head") updated = make_pr( From 347e8dd61e68f82c307367f119d76cd09e4dbf31 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:31:38 +0000 Subject: [PATCH 5/9] Fix Strix retry evidence staleness and draft-artifact-read permission Two adversarial-review findings against the strix_evidence_state() fix: - strix_evidence_state() walked every Strix context node directly, so a rerun's stale failed CheckRun attempt (GitHub keeps every prior attempt's CheckRun node in the rollup alongside the latest one) could permanently keep the gate "failed" even after a later retry succeeded. Extract the CheckRun-identity dedup failed_status_checks() already used into a shared latest_check_run_attempts() helper and evaluate only the latest attempt per Strix CheckRun identity; failed_status_checks() now calls the same helper instead of duplicating the dedup logic. - active_draft_review_request()'s Actions-artifact read used the generic target-repository read credential, but the artifact always lives in the central .github repository regardless of which repository the PR belongs to, and the OpenCode app installation has no Actions permission. For a cross-repository dispatch with only the OpenCode app credential configured, the read would fail, so the initial mention-triggered request for a draft PR outside .github could never get past its own authorization check. New gh_api_json_via_dispatch_token() reads through the same central-repository dispatch credential already used to create the repository dispatch there, which the workflow always sets to the runner's own github.token. Added regression tests for both: a stale failed attempt vs. a later success (and the reverse), a running retry after a failure, and the dispatch-token vs. read-token credential selection. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- CHANGELOG.md | 33 +++++++ scripts/ci/pr_review_merge_scheduler.py | 113 ++++++++++++++++-------- tests/test_pr_review_merge_scheduler.py | 64 +++++++++++++- 3 files changed, 171 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ce3cff5c1..481b70287e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,39 @@ Semantic Versioning where the repository publishes a release. exhaustive regression fixtures for every non-passing terminal conclusion/state plus authoritative success, for both CheckRun and classic commit-status shapes. + - Two more adversarial-review findings against that same fix, both fixed: + - `strix_evidence_state()` walked every Strix context node in the + rollup directly, so a rerun's stale failed CheckRun attempt (GitHub + keeps every prior attempt's CheckRun node alongside the latest one) + could permanently keep the gate `"failed"` even after a later retry + succeeded. Extracted the CheckRun-identity dedup `failed_status_checks()` + already used (latest attempt per `(workflow, name)`, by `startedAt` + then rollup order) into a shared `latest_check_run_attempts()` helper + and evaluate only the latest attempt per Strix CheckRun identity. + `failed_status_checks()` itself now calls the same helper instead of + duplicating the dedup logic, with no behavior change. Added + regression tests for an older failed attempt followed by a newer + success, the reverse ordering, and a running retry after a failure. + - `active_draft_review_request()`'s Actions-artifact read used the + generic target-repository read credential + (`gh_api_json`/`SCHEDULER_READ_TOKEN`), but the artifact always lives + in the central `.github` repository regardless of which repository + the PR belongs to, and — per `scheduler_dispatch_env()`'s own + pre-existing documented fact — "the OpenCode app installation has no + Actions permission." For a cross-repository dispatch with only the + OpenCode app credential configured (no `PR_REVIEW_MERGE_TOKEN`/ + `OPENCODE_APPROVE_TOKEN` secret), the read credential resolved to + that same Actions-permission-less app token, so the artifact read + would fail and the initial mention-triggered request for a draft PR + outside `.github` could never get past its own authorization check. + New `gh_api_json_via_dispatch_token()` reads through + `run_github_dispatch()`/`SCHEDULER_DISPATCH_TOKEN` instead — the same + central-repository dispatch credential already used to create the + `repository_dispatch` there — which the workflow always sets to the + runner's own `github.token`, valid for `.github`'s own Actions + artifacts regardless of the PR's actual repository. Added a + regression test proving the read uses the dispatch token, not + whatever generic `GH_TOKEN` the OpenCode app credential resolves to. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 923161e65f..ea0e6151cc 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -765,6 +765,23 @@ def gh_api_json(path: str) -> Any: return json.loads(run_github_read(["gh", "api", path])) +def gh_api_json_via_dispatch_token(path: str) -> Any: + """Run a GitHub REST API GET via the central-repository dispatch credential. + + The OpenCode app installation has no Actions permission (see + :func:`scheduler_dispatch_env`), and the target-repository read + credential (:func:`gh_api_json`) is not guaranteed to have it either for + a cross-repository dispatch. A read against ``.github``'s own Actions + artifacts -- which always host the central draft-review-request marker + regardless of which repository the PR belongs to -- must use the same + central-repository dispatch credential already used for creating a + ``repository_dispatch`` there, not the target-repository read + credential. + """ + + return json.loads(run_github_dispatch(["gh", "api", path])) + + def rest_review_node(review: dict[str, Any]) -> dict[str, Any]: """Convert a REST review payload into the GraphQL shape used by the scheduler.""" @@ -1151,6 +1168,50 @@ def opencode_in_progress(pr: dict[str, Any], *, stale_after_minutes: int | None _STRIX_SUCCESS_CONCLUSIONS = {"SUCCESS"} +def latest_check_run_attempts(nodes: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Return each CheckRun's most recent attempt per (workflow, name) identity. + + A rerun leaves every earlier attempt's CheckRun node in the rollup + alongside the latest one, so callers that walk ``context_nodes`` directly + can see a stale failed attempt outlive a later successful retry. This + resolves each CheckRun identity to only its most recently started + attempt (falling back to rollup order when ``startedAt`` is missing), + while passing every non-CheckRun (classic commit-status) node through + unchanged. The result preserves the original relative ordering. + """ + latest: dict[tuple[str, str], tuple[datetime | None, int, dict[str, Any]]] = {} + ordered: list[tuple[int, dict[str, Any]]] = [] + for index, node in enumerate(nodes): + if node.get("__typename") != "CheckRun": + ordered.append((index, node)) + continue + workflow = ( + (((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name") + or "" + ) + key = (workflow, node.get("name") or "check-run") + started_at = parse_github_datetime(node.get("startedAt")) + previous = latest.get(key) + if previous is None: + latest[key] = (started_at, index, node) + continue + previous_started_at, previous_index, _ = previous + if started_at is None and previous_started_at is not None: + continue + if previous_started_at is None and started_at is not None: + latest[key] = (started_at, index, node) + continue + if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= ( + previous_started_at or datetime.min.replace(tzinfo=timezone.utc), + previous_index, + ): + latest[key] = (started_at, index, node) + for started_at, index, node in latest.values(): + ordered.append((index, node)) + ordered.sort(key=lambda item: item[0]) + return [node for _, node in ordered] + + def strix_evidence_state(pr: dict[str, Any]) -> str: """Return missing, running, failed, or complete for current-head Strix evidence. @@ -1158,11 +1219,13 @@ def strix_evidence_state(pr: dict[str, Any]) -> str: commit-status state of SUCCESS). Any other terminal outcome -- failure, error, cancelled, timed out, skipped, neutral, action_required, stale, startup_failure -- is reported as "failed" rather than "complete" so - callers fail closed instead of unlocking on non-passing evidence. + callers fail closed instead of unlocking on non-passing evidence. Only + the latest attempt per Strix CheckRun identity is evaluated, so a stale + failed attempt cannot outlive a later successful retry. """ found = False saw_failure = False - for node in context_nodes(pr): + for node in latest_check_run_attempts(context_nodes(pr)): if not is_strix_context(node): continue found = True @@ -1499,43 +1562,16 @@ def dismiss_stale_opencode_change_requests(repo: str, pr: dict[str, Any], *, dry def failed_status_checks(pr: dict[str, Any]) -> list[str]: """Return failing check or status context names from the PR rollup.""" failed: list[str] = [] - latest_check_runs: dict[ - tuple[str, str], - tuple[datetime | None, int, dict[str, Any]], - ] = {} - status_contexts: list[dict[str, Any]] = [] - for index, node in enumerate(context_nodes(pr)): - if node.get("__typename") != "CheckRun": - status_contexts.append(node) - continue - workflow = ( - (((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name") - or "" - ) - key = (workflow, node.get("name") or "check-run") - started_at = parse_github_datetime(node.get("startedAt")) - previous = latest_check_runs.get(key) - if previous is None: - latest_check_runs[key] = (started_at, index, node) - continue - previous_started_at, previous_index, _ = previous - if started_at is None and previous_started_at is not None: - continue - if previous_started_at is None and started_at is not None: - latest_check_runs[key] = (started_at, index, node) - continue - if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= ( - previous_started_at or datetime.min.replace(tzinfo=timezone.utc), - previous_index, - ): - latest_check_runs[key] = (started_at, index, node) - + nodes = latest_check_run_attempts(context_nodes(pr)) + status_contexts = [node for node in nodes if node.get("__typename") != "CheckRun"] successful_status_contexts = { node.get("context") for node in status_contexts if (node.get("state") or "").upper() == "SUCCESS" } - for _, _, node in sorted(latest_check_runs.values(), key=lambda item: item[1]): + for node in nodes: + if node.get("__typename") != "CheckRun": + continue conclusion = (node.get("conclusion") or "").upper() if conclusion in FAILED_CHECK_CONCLUSIONS: if is_strix_context(node) and "strix" in successful_status_contexts: @@ -2458,14 +2494,19 @@ def active_draft_review_request(repo: str, pr: dict[str, Any]) -> bool: ``repository_dispatch`` ``client_payload`` of its own, most commonly the Strix-completion ``workflow_run`` that follows an initial ``security_dispatch`` -- checks the same durable signal here rather than - trusting anything the triggering event itself claims. + trusting anything the triggering event itself claims. The read always + uses the central-repository dispatch credential + (:func:`gh_api_json_via_dispatch_token`), because the artifact always + lives in that central repository regardless of which repository ``repo`` + names, and the target-repository read credential is not guaranteed to + have Actions permission there for a cross-repository dispatch. """ head_sha = pr.get("headRefOid") if not isinstance(head_sha, str) or not head_sha: return False dispatch_repo = repository_dispatch_target(validate_github_repository(repo)) artifact_name = draft_review_request_artifact_name(repo, pr["number"], head_sha) - response = gh_api_json( + response = gh_api_json_via_dispatch_token( f"repos/{dispatch_repo}/actions/artifacts?name={artifact_name}&per_page=100" ) return bool(_draft_review_request_records(response, expected_name=artifact_name)) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 88ac1e6a6a..346252fd2f 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1058,6 +1058,31 @@ def test_context_review_and_check_helpers(monkeypatch): classic_success = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "SUCCESS"}]}}) assert sched.strix_evidence_state(classic_success) == "complete" + # A stale failed attempt must not outlive a later successful retry, and a + # later failed attempt must override an earlier success -- only the + # latest attempt per Strix CheckRun identity counts (regression for a + # rerun leaving every earlier attempt's CheckRun node in the rollup). + older_failed_then_newer_success = make_pr( + statusCheckRollup={ + "contexts": {"nodes": [strix_check(conclusion="FAILURE"), strix_check()]} + } + ) + assert sched.strix_evidence_state(older_failed_then_newer_success) == "complete" + newer_failed_after_older_success = make_pr( + statusCheckRollup={ + "contexts": {"nodes": [strix_check(), strix_check(conclusion="FAILURE")]} + } + ) + assert sched.strix_evidence_state(newer_failed_after_older_success) == "failed" + running_retry_after_failure = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [strix_check(conclusion="FAILURE"), strix_check(status="IN_PROGRESS", conclusion="")] + } + } + ) + assert sched.strix_evidence_state(running_retry_after_failure) == "running" + threaded = make_pr( reviewThreads={ "nodes": [ @@ -3821,7 +3846,7 @@ def fake_gh_api_json(path): return {"total_count": 0, "artifacts": []} monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") - monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json) pr = make_pr(headRefOid="b" * 40) assert sched.active_draft_review_request("owner/repo", pr) is False @@ -3839,7 +3864,7 @@ def fake_gh_api_json(path): "artifacts": [{"id": 7, "name": expected_name, "expired": False}], } - monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json) pr = make_pr(headRefOid="b" * 40) assert sched.active_draft_review_request("owner/repo", pr) is True @@ -3852,7 +3877,7 @@ def fake_gh_api_json(path): "artifacts": [{"id": 7, "name": expected_name, "expired": True}], } - monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json) + monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json) pr = make_pr(headRefOid="b" * 40) assert sched.active_draft_review_request("owner/repo", pr) is False @@ -3861,6 +3886,39 @@ def test_active_draft_review_request_false_without_a_head_sha(): assert sched.active_draft_review_request("owner/repo", make_pr(headRefOid=None)) is False +def test_active_draft_review_request_uses_dispatch_token_not_opencode_app_token(monkeypatch): + """Regression: the OpenCode app installation has no Actions permission, so + reading the draft-review-request artifact must use the same + central-repository dispatch credential as creating a repository + dispatch there, never the target-repository read credential -- which, + for a cross-repository dispatch with only the OpenCode app credential + configured, resolves to a token with no Actions permission.""" + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") + monkeypatch.setenv("GH_TOKEN", "opencode-app-token") + monkeypatch.setenv("SCHEDULER_READ_TOKEN", "opencode-app-token") + monkeypatch.setenv("SCHEDULER_DISPATCH_TOKEN", "runner-token") + + read_calls = [] + dispatch_calls = [] + monkeypatch.setattr( + sched, + "run_github_read", + lambda args, stdin=None: read_calls.append(args) or "{}", + ) + monkeypatch.setattr( + sched, + "run_with_env", + lambda args, stdin=None, env=None: dispatch_calls.append((args, env["GH_TOKEN"])) + or '{"total_count": 0, "artifacts": []}', + ) + + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is False + assert read_calls == [] + assert len(dispatch_calls) == 1 + assert dispatch_calls[0][1] == "runner-token" + + def test_draft_review_request_records_fail_closed_on_malformed_responses(): name = "cwl-draft-review-request-owner-repo-1-" + "a" * 40 with pytest.raises(ValueError, match="must be an object"): From 1038590f26ea543c9a7a9d8f7c916ec6e8c44698 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:43:52 +0000 Subject: [PATCH 6/9] Fail closed when the draft-review-request artifact read cannot succeed active_draft_review_request()'s central-repository dispatch credential (the previous fix) is itself only valid when the scheduler executes inside .github. scan-pr-queue has no such guard: the organization's required-workflow ruleset runs it directly in each sibling repository's own context for that repository's ordinary PR events, where github.token is scoped only to that sibling repository and cannot read .github's artifacts either. The resulting gh failure -- or a malformed/tampered artifact-list response -- previously propagated as an unhandled exception, replacing the intended "skip: draft PR" outcome with an error that would abort the whole multi-PR scan over one draft PR. Any such failure now resolves to False (no confirmed active request), the same safe outcome as a completed check that finds nothing. Added regression tests for both the credential failure and a malformed response. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- CHANGELOG.md | 15 +++++++++++++ scripts/ci/pr_review_merge_scheduler.py | 22 ++++++++++++++----- tests/test_pr_review_merge_scheduler.py | 29 +++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 481b70287e..bb38ac7ff8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,21 @@ Semantic Versioning where the repository publishes a release. artifacts regardless of the PR's actual repository. Added a regression test proving the read uses the dispatch token, not whatever generic `GH_TOKEN` the OpenCode app credential resolves to. + - One more adversarial-review finding against that same dispatch-token + fix: the central-repository dispatch credential is itself only valid + when this scheduler executes inside `.github`. `scan-pr-queue` has no + such guard — the organization's required-workflow ruleset runs it + directly in each sibling repository's own context for that repository's + ordinary (non-mention) PR events, where `github.token` is scoped only + to that sibling repository and cannot read `.github`'s artifacts + either. `active_draft_review_request()` previously let that `gh` + failure -- or a malformed/tampered artifact-list response -- propagate + as an unhandled exception, replacing the intended `skip: draft PR` + outcome with an error that would abort the whole multi-PR scan over one + draft PR. It now resolves any such failure to `False` (no confirmed + active request) instead, the same safe outcome as a completed check + that finds nothing. Added regression tests for both the credential + failure and a malformed response. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index ea0e6151cc..08eb5403e7 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2499,17 +2499,29 @@ def active_draft_review_request(repo: str, pr: dict[str, Any]) -> bool: (:func:`gh_api_json_via_dispatch_token`), because the artifact always lives in that central repository regardless of which repository ``repo`` names, and the target-repository read credential is not guaranteed to - have Actions permission there for a cross-repository dispatch. + have Actions permission there for a cross-repository dispatch. That + dispatch credential is itself only valid when this scheduler executes + inside the central repository; an ordinary required-workflow scan + executing directly in a sibling repository has no credential able to + read the central repository's artifacts at all. Rather than let that + ``gh`` failure -- or a malformed/tampered artifact-list response -- + propagate and abort the whole multi-PR scan over one draft PR, any + failure to positively confirm a live artifact resolves to ``False``: + the same safe "no explicit request" outcome as a live check that + actually completes and finds nothing. """ head_sha = pr.get("headRefOid") if not isinstance(head_sha, str) or not head_sha: return False dispatch_repo = repository_dispatch_target(validate_github_repository(repo)) artifact_name = draft_review_request_artifact_name(repo, pr["number"], head_sha) - response = gh_api_json_via_dispatch_token( - f"repos/{dispatch_repo}/actions/artifacts?name={artifact_name}&per_page=100" - ) - return bool(_draft_review_request_records(response, expected_name=artifact_name)) + try: + response = gh_api_json_via_dispatch_token( + f"repos/{dispatch_repo}/actions/artifacts?name={artifact_name}&per_page=100" + ) + return bool(_draft_review_request_records(response, expected_name=artifact_name)) + except (RuntimeError, ValueError): + return False def dispatch_draft_review_only( diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 346252fd2f..f289abaaf8 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -3886,6 +3886,35 @@ def test_active_draft_review_request_false_without_a_head_sha(): assert sched.active_draft_review_request("owner/repo", make_pr(headRefOid=None)) is False +def test_active_draft_review_request_fails_closed_when_the_read_cannot_be_performed(monkeypatch): + """Regression: an ordinary required-workflow scan executing directly in a + sibling repository has no credential able to read the central .github + repository's Actions artifacts at all -- neither the target-repository + read credential nor the central dispatch credential is valid there. The + resulting gh failure must resolve to False (no confirmed active + request, same as a completed check that finds nothing), never propagate + and abort the whole multi-PR scan over one draft PR.""" + + def raise_runtime_error(path): + raise RuntimeError("Command failed (1): gh api ...\nHTTP 403: Resource not accessible") + + monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", raise_runtime_error) + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is False + + +def test_active_draft_review_request_fails_closed_on_a_malformed_artifact_response(monkeypatch): + """A malformed or internally inconsistent artifact-list response must + never be treated as a confirmed live request; it resolves to False + exactly like any other inability to positively confirm one.""" + + monkeypatch.setattr( + sched, "gh_api_json_via_dispatch_token", lambda path: {"total_count": 1, "artifacts": []} + ) + pr = make_pr(headRefOid="b" * 40) + assert sched.active_draft_review_request("owner/repo", pr) is False + + def test_active_draft_review_request_uses_dispatch_token_not_opencode_app_token(monkeypatch): """Regression: the OpenCode app installation has no Actions permission, so reading the draft-review-request artifact must use the same From 3a91288b03667da7b5a22354f0f224d33e28e17a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 00:25:36 +0000 Subject: [PATCH 7/9] fix(scheduler): a required-workflow Strix CheckRun is now the sole authority Devin Review flagged that strix_evidence_state() treated a classic commit-status Strix context as equally authoritative to a required-workflow CheckRun. A stale classic-status failure (e.g. left over from a same-head manual workflow_dispatch run) kept the gate "failed" forever even after the real CheckRun evidence succeeded, since dispatch_strix_evidence() can only rerun a CheckRun's Actions job and has no way to clear a classic status -- producing an endless, pointless rerun loop that permanently blocked OpenCode dispatch. A CheckRun, when present, is now the sole authority; a classic status is only consulted when no CheckRun exists at all, matching this repo's documented policy that a manual dispatch "may supply review evidence but does not replace required PR checks." --- CHANGELOG.md | 16 +++++++++ scripts/ci/pr_review_merge_scheduler.py | 25 +++++++++---- tests/test_pr_review_merge_scheduler.py | 47 +++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fa0a62308..d4fb14712f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix a Devin Review finding on PR #1456: `strix_evidence_state()` treated a + classic commit-status Strix context (e.g. a same-head manual + `workflow_dispatch` run) as equally authoritative to a required-workflow + Strix CheckRun, so a stale classic-status failure left the gate "failed" + forever even after the real CheckRun evidence succeeded -- + `dispatch_strix_evidence()` can only rerun a CheckRun's Actions job, never + a classic status, so this produced an endless, pointless rerun loop that + permanently blocked OpenCode dispatch. A required-workflow CheckRun is now + the sole authority whenever one is present; a classic status is evaluated + only when no CheckRun exists at all, matching this repo's documented + policy that a manual run "may supply review evidence but does not replace + required PR checks." Added regression tests for a stale classic failure + beside a successful CheckRun (now "complete"), a genuinely failing + CheckRun beside an unrelated classic success (still correctly "failed"), + and a still-running CheckRun beside a stale classic failure (still + "running", not prematurely "failed"). - Let an explicit mention-triggered review request (`@opencode-agent review`) actually dispatch a current-head OpenCode review for a **draft** PR. `pr_review_merge_scheduler.py`'s `inspect_pr()` unconditionally returned diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 08eb5403e7..cfd2a02502 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1222,17 +1222,30 @@ def strix_evidence_state(pr: dict[str, Any]) -> str: callers fail closed instead of unlocking on non-passing evidence. Only the latest attempt per Strix CheckRun identity is evaluated, so a stale failed attempt cannot outlive a later successful retry. + + A required-workflow Strix CheckRun, when present, is the sole authority: + a classic commit-status context (e.g. a same-head manual + `workflow_dispatch` Strix run) is evaluated only when no CheckRun exists + at all. This matches this repo's documented policy that a manual run + "may supply review evidence but does not replace required PR checks" -- + without it, a stale manual-status failure that `dispatch_strix_evidence` + has no way to clear (it can only rerun a CheckRun's Actions job) would + keep this gate "failed" forever even after the real, retryable CheckRun + evidence succeeds, forcing an endless, pointless rerun loop. """ - found = False + strix_nodes = [node for node in latest_check_run_attempts(context_nodes(pr)) if is_strix_context(node)] + if not strix_nodes: + return "missing" + check_run_present = any(node.get("__typename") == "CheckRun" for node in strix_nodes) saw_failure = False - for node in latest_check_run_attempts(context_nodes(pr)): - if not is_strix_context(node): + for node in strix_nodes: + is_check_run = node.get("__typename") == "CheckRun" + if check_run_present and not is_check_run: continue - found = True status = (node.get("status") or node.get("state") or "").upper() if status in RUNNING_CHECK_STATES: return "running" - if node.get("__typename") == "CheckRun": + if is_check_run: if status != "COMPLETED": return "running" conclusion = (node.get("conclusion") or "").upper() @@ -1240,8 +1253,6 @@ def strix_evidence_state(pr: dict[str, Any]) -> str: saw_failure = True elif status not in _STRIX_SUCCESS_CONCLUSIONS: saw_failure = True - if not found: - return "missing" return "failed" if saw_failure else "complete" diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index f289abaaf8..f09229c179 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1058,6 +1058,53 @@ def test_context_review_and_check_helpers(monkeypatch): classic_success = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "SUCCESS"}]}}) assert sched.strix_evidence_state(classic_success) == "complete" + # A required-workflow CheckRun, when present, is the sole authority: a + # stale classic-status failure left over from an unrelated same-head + # manual `workflow_dispatch` Strix run must not keep the gate "failed" + # forever once the real CheckRun evidence succeeds -- `dispatch_strix_evidence` + # can only rerun a CheckRun's Actions job, so a classic status it cannot + # touch must never be what perpetually blocks this gate (regression for + # the endless-rerun loop this would otherwise cause). + checkrun_success_classic_failure = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + {"context": "strix", "state": "FAILURE"}, + strix_check(), + ] + } + } + ) + assert sched.strix_evidence_state(checkrun_success_classic_failure) == "complete" + # The reverse direction stays correctly gated: a genuinely failing + # CheckRun is not excused by an unrelated classic-status success, since + # the CheckRun is the retryable evidence this gate exists to protect. + checkrun_failure_classic_success = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + {"context": "strix", "state": "SUCCESS"}, + strix_check(conclusion="FAILURE"), + ] + } + } + ) + assert sched.strix_evidence_state(checkrun_failure_classic_success) == "failed" + # A CheckRun still in progress is not undermined by a stale classic + # failure either -- the classic status is ignored entirely once any + # CheckRun is present, including while that CheckRun is still running. + checkrun_running_classic_failure = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + {"context": "strix", "state": "FAILURE"}, + strix_check(status="IN_PROGRESS", conclusion=""), + ] + } + } + ) + assert sched.strix_evidence_state(checkrun_running_classic_failure) == "running" + # A stale failed attempt must not outlive a later successful retry, and a # later failed attempt must override an earlier success -- only the # latest attempt per Strix CheckRun identity counts (regression for a From 51e1185a091ea5191b7486a310fe63d23a2a2bfa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 00:33:40 +0000 Subject: [PATCH 8/9] fix(scheduler): either Strix identity's success unlocks review dispatch A second Devin Review finding directly refined the previous commit's fix: making a required-workflow CheckRun the sole authority also meant a genuinely failing CheckRun could never be excused by a same-head manual workflow_dispatch Strix run's classic-status success -- but this repo documents that as intended, precisely for a self-modifying .github PR whose pull_request_target CheckRun runs the base branch's trusted scripts and can legitimately fail against a PR editing those very scripts. strix_evidence_state() now treats either identity's authoritative success as sufficient for "complete" (never substituting for GitHub's own independently enforced required CheckRun at actual merge time); only when no identity ever succeeds does it report "failed". This still resolves the original endless-rerun-loop defect while also letting a genuine same-head manual success unblock review when the CheckRun itself is the one that's wrong. --- CHANGELOG.md | 22 +++++++++ scripts/ci/pr_review_merge_scheduler.py | 64 ++++++++++++++----------- tests/test_pr_review_merge_scheduler.py | 46 ++++++++++++------ 3 files changed, 91 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4fb14712f..0c3b9f87f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,28 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix a second, immediately-following Devin Review finding on PR #1456 + (`strix_evidence_state()`), which directly refined the previous entry's + fix: making a required-workflow CheckRun the sole authority whenever + present also meant a genuinely failing CheckRun could never be excused by + a same-head manual `workflow_dispatch` Strix run's classic-status + success -- but this repo documents exactly that as intended: a manual run + "may supply review evidence but does not replace required PR checks", + precisely for a self-modifying `.github` PR whose `pull_request_target` + CheckRun runs the *base* branch's trusted scripts and can legitimately + fail against a PR editing those very scripts, while a trusted same-head + manual dispatch correctly evaluates the new code. `strix_evidence_state()` + now treats either Strix identity's authoritative success as sufficient + for "complete" (never substituting for GitHub's own independently + enforced required CheckRun at actual merge time, which this function does + not touch); only when *no* identity ever succeeds does it report "failed". + This still resolves the original endless-rerun-loop defect (a stale + classic failure can no longer block a since-succeeded CheckRun) while + also letting a genuine same-head manual success unblock review when the + CheckRun itself is the one that's wrong. Updated the previous round's + regression test asserting the reverse case as "failed" to the corrected + "complete", and added a fourth case (both identities failing, still + correctly "failed") to keep every combination covered. - Fix a Devin Review finding on PR #1456: `strix_evidence_state()` treated a classic commit-status Strix context (e.g. a same-head manual `workflow_dispatch` run) as equally authoritative to a required-workflow diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index cfd2a02502..db10cebf3d 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1216,44 +1216,54 @@ def strix_evidence_state(pr: dict[str, Any]) -> str: """Return missing, running, failed, or complete for current-head Strix evidence. "complete" requires authoritative success (CheckRun conclusion or classic - commit-status state of SUCCESS). Any other terminal outcome -- failure, - error, cancelled, timed out, skipped, neutral, action_required, stale, - startup_failure -- is reported as "failed" rather than "complete" so - callers fail closed instead of unlocking on non-passing evidence. Only - the latest attempt per Strix CheckRun identity is evaluated, so a stale - failed attempt cannot outlive a later successful retry. - - A required-workflow Strix CheckRun, when present, is the sole authority: - a classic commit-status context (e.g. a same-head manual - `workflow_dispatch` Strix run) is evaluated only when no CheckRun exists - at all. This matches this repo's documented policy that a manual run - "may supply review evidence but does not replace required PR checks" -- - without it, a stale manual-status failure that `dispatch_strix_evidence` - has no way to clear (it can only rerun a CheckRun's Actions job) would - keep this gate "failed" forever even after the real, retryable CheckRun - evidence succeeds, forcing an endless, pointless rerun loop. + commit-status state of SUCCESS) from *any* Strix identity present -- a + CheckRun and a classic commit-status context are both accepted, and + either one succeeding is sufficient. This repo documents that a same-head + manual `workflow_dispatch` Strix run, which posts a classic commit + status, "may supply review evidence but does not replace required PR + checks": it can unlock this internal review-dispatch gate even when the + `pull_request_target` CheckRun failed or cannot correctly evaluate a + self-modifying `.github` PR (that CheckRun runs the *base* branch's + trusted scripts, which a PR editing those very scripts can legitimately + fail against) -- but it never substitutes for GitHub's own independently + enforced required CheckRun at actual merge time, which this function + does not touch. Symmetrically, a stale classic-status failure left over + from an unrelated manual run must never keep this gate "failed" forever + once the real, retryable CheckRun evidence succeeds -- `dispatch_strix_evidence` + has no way to clear a classic status, only to rerun a CheckRun's Actions + job, so treating a lingering classic failure as still blocking once a + CheckRun has already succeeded would force an endless, pointless rerun + loop. + + Only when *no* identity reports success is this "failed" (every present + terminal outcome -- failure, error, cancelled, timed out, skipped, + neutral, action_required, stale, startup_failure -- counts as + non-passing) or "running" (something is still in flight and nothing has + succeeded yet), so callers fail closed instead of unlocking on evidence + that never actually passed anywhere. Only the latest attempt per Strix + CheckRun identity is evaluated, so a stale failed attempt cannot outlive + a later successful retry. """ strix_nodes = [node for node in latest_check_run_attempts(context_nodes(pr)) if is_strix_context(node)] if not strix_nodes: return "missing" - check_run_present = any(node.get("__typename") == "CheckRun" for node in strix_nodes) - saw_failure = False + saw_running = False for node in strix_nodes: is_check_run = node.get("__typename") == "CheckRun" - if check_run_present and not is_check_run: - continue status = (node.get("status") or node.get("state") or "").upper() if status in RUNNING_CHECK_STATES: - return "running" + saw_running = True + continue if is_check_run: if status != "COMPLETED": - return "running" + saw_running = True + continue conclusion = (node.get("conclusion") or "").upper() - if conclusion not in _STRIX_SUCCESS_CONCLUSIONS: - saw_failure = True - elif status not in _STRIX_SUCCESS_CONCLUSIONS: - saw_failure = True - return "failed" if saw_failure else "complete" + if conclusion in _STRIX_SUCCESS_CONCLUSIONS: + return "complete" + elif status in _STRIX_SUCCESS_CONCLUSIONS: + return "complete" + return "running" if saw_running else "failed" def unresolved_thread_count(pr: dict[str, Any]) -> int: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index f09229c179..e81bcf3b61 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1058,13 +1058,13 @@ def test_context_review_and_check_helpers(monkeypatch): classic_success = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "SUCCESS"}]}}) assert sched.strix_evidence_state(classic_success) == "complete" - # A required-workflow CheckRun, when present, is the sole authority: a - # stale classic-status failure left over from an unrelated same-head - # manual `workflow_dispatch` Strix run must not keep the gate "failed" - # forever once the real CheckRun evidence succeeds -- `dispatch_strix_evidence` - # can only rerun a CheckRun's Actions job, so a classic status it cannot - # touch must never be what perpetually blocks this gate (regression for - # the endless-rerun loop this would otherwise cause). + # Either Strix identity succeeding is sufficient: a stale classic-status + # failure left over from an unrelated same-head manual `workflow_dispatch` + # Strix run must not keep the gate "failed" forever once the real + # CheckRun evidence succeeds -- `dispatch_strix_evidence` can only rerun + # a CheckRun's Actions job, so a classic status it cannot touch must + # never be what perpetually blocks this gate (regression for the + # endless-rerun loop this would otherwise cause). checkrun_success_classic_failure = make_pr( statusCheckRollup={ "contexts": { @@ -1076,9 +1076,15 @@ def test_context_review_and_check_helpers(monkeypatch): } ) assert sched.strix_evidence_state(checkrun_success_classic_failure) == "complete" - # The reverse direction stays correctly gated: a genuinely failing - # CheckRun is not excused by an unrelated classic-status success, since - # the CheckRun is the retryable evidence this gate exists to protect. + # The reverse direction is also "complete": a same-head manual + # `workflow_dispatch` Strix run's classic-status success may supply + # review evidence even when the `pull_request_target` CheckRun failed -- + # e.g. a self-modifying `.github` PR whose CheckRun runs the *base* + # branch's trusted scripts and can legitimately fail against a PR + # editing those very scripts, while a trusted same-head manual dispatch + # correctly evaluates the new code. This never substitutes for GitHub's + # own independently enforced required CheckRun at actual merge time, + # which this function does not touch. checkrun_failure_classic_success = make_pr( statusCheckRollup={ "contexts": { @@ -1089,10 +1095,10 @@ def test_context_review_and_check_helpers(monkeypatch): } } ) - assert sched.strix_evidence_state(checkrun_failure_classic_success) == "failed" - # A CheckRun still in progress is not undermined by a stale classic - # failure either -- the classic status is ignored entirely once any - # CheckRun is present, including while that CheckRun is still running. + assert sched.strix_evidence_state(checkrun_failure_classic_success) == "complete" + # A CheckRun still in progress and no success anywhere yet correctly + # stays "running" rather than prematurely "failed", even beside a stale + # classic failure. checkrun_running_classic_failure = make_pr( statusCheckRollup={ "contexts": { @@ -1104,6 +1110,18 @@ def test_context_review_and_check_helpers(monkeypatch): } ) assert sched.strix_evidence_state(checkrun_running_classic_failure) == "running" + # Only when *no* identity ever succeeds is the gate genuinely "failed". + checkrun_failure_classic_failure = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + {"context": "strix", "state": "FAILURE"}, + strix_check(conclusion="FAILURE"), + ] + } + } + ) + assert sched.strix_evidence_state(checkrun_failure_classic_failure) == "failed" # A stale failed attempt must not outlive a later successful retry, and a # later failed attempt must override an earlier success -- only the From 92a546bba5e35a78ab1bd8346d9178dfce13f128 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 00:40:43 +0000 Subject: [PATCH 9/9] fix(scheduler): REST fallback now includes classic commit statuses Devin Review found that rest_pr_node (used when GraphQL is unavailable) only ever fetched a head commit's CheckRuns, never its classic commit statuses, so a same-head manual workflow_dispatch Strix run's classic-status evidence silently disappeared under REST fallback -- strix_evidence_state() would see no Strix evidence at all through that identity, exactly the loss of manual evidence the two preceding fixes on this branch were built to preserve. rest_pr_node now also fetches commits/{sha}/statuses and folds them into the same statusCheckRollup.contexts.nodes list via a new rest_status_node shape converter. --- CHANGELOG.md | 14 ++++++++++++++ scripts/ci/pr_review_merge_scheduler.py | 15 +++++++++++++++ tests/test_pr_review_merge_scheduler.py | 22 ++++++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c3b9f87f5..44145d4ec4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix a Devin Review finding on PR #1456: the REST fallback path + (`rest_pr_node`, used when GraphQL is unavailable) only ever fetched a + head commit's CheckRuns (`commits/{sha}/check-runs`), never its classic + commit statuses (`commits/{sha}/statuses`), so a same-head manual + `workflow_dispatch` Strix run's classic-status evidence silently + disappeared under REST fallback -- `strix_evidence_state()` would see no + Strix evidence at all and could never reach `"complete"` through that + identity, exactly the loss of manual evidence the two preceding fixes on + this PR were built to preserve. `rest_pr_node` now also fetches classic + statuses and folds them into the same `statusCheckRollup.contexts.nodes` + list via a new `rest_status_node` shape converter, alongside the existing + CheckRun conversion. Added a regression assertion that a classic status + survives the REST fallback and that `strix_evidence_state()` sees it as + `"complete"` end-to-end. - Fix a second, immediately-following Devin Review finding on PR #1456 (`strix_evidence_state()`), which directly refined the previous entry's fix: making a required-workflow CheckRun the sole authority whenever diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index db10cebf3d..4644d3e78b 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -810,6 +810,16 @@ def rest_check_node(check: dict[str, Any]) -> dict[str, Any]: } +def rest_status_node(status: dict[str, Any]) -> dict[str, Any]: + """Convert a REST classic commit-status payload into the GraphQL status rollup shape.""" + + return { + "context": status.get("context"), + "state": (status.get("state") or "").upper(), + "targetUrl": status.get("target_url"), + } + + def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: """Convert a REST pull request payload into the GraphQL shape used by the scheduler.""" @@ -819,6 +829,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: head_repo = head.get("repo") or {} reviews = gh_api_json(f"repos/{repo}/pulls/{number}/reviews?per_page=100") checks = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-runs?per_page=100") + statuses = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/statuses?per_page=100") files = gh_api_json(f"repos/{repo}/pulls/{number}/files?per_page=20") rest_merge_state = REST_MERGEABLE_STATE_MAP.get( str(pr.get("mergeable_state") or "").lower(), @@ -848,6 +859,10 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: rest_check_node(check) for check in (checks.get("check_runs") or []) ] + + [ + rest_status_node(status) + for status in (statuses or []) + ] } }, "restMergeableState": rest_merge_state, diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index e81bcf3b61..5727863f8d 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -561,6 +561,13 @@ def test_rest_pr_fallback_shapes_reviews_and_checks(monkeypatch): } ] }, + "repos/owner/repo/commits/abc123/statuses?per_page=100": [ + { + "context": "strix", + "state": "success", + "target_url": "https://github.com/owner/repo/actions/runs/3", + } + ], "repos/owner/repo/pulls/42/files?per_page=20": [ {"filename": "scripts/ci/pr_review_merge_scheduler.py"}, ], @@ -593,6 +600,7 @@ def fake_api(path): assert calls == [ "repos/owner/repo/pulls/42/reviews?per_page=100", "repos/owner/repo/commits/abc123/check-runs?per_page=100", + "repos/owner/repo/commits/abc123/statuses?per_page=100", "repos/owner/repo/pulls/42/files?per_page=20", ] assert node["number"] == 42 @@ -605,6 +613,20 @@ def fake_api(path): assert node["reviews"]["nodes"][0]["commit"]["oid"] == "abc123" assert node["statusCheckRollup"]["contexts"]["nodes"][0]["status"] == "COMPLETED" assert node["statusCheckRollup"]["contexts"]["nodes"][0]["conclusion"] == "SUCCESS" + # A classic commit status (e.g. a same-head manual `workflow_dispatch` + # Strix run's evidence) must survive the REST fallback too -- omitting + # it here would silently erase that evidence for every caller, since + # `check-runs` alone never includes classic statuses (regression for a + # Devin Review finding: "REST fallback loses manual evidence"). + classic_nodes = [n for n in node["statusCheckRollup"]["contexts"]["nodes"] if "context" in n] + assert classic_nodes == [ + { + "context": "strix", + "state": "SUCCESS", + "targetUrl": "https://github.com/owner/repo/actions/runs/3", + } + ] + assert sched.strix_evidence_state(node) == "complete" def test_fetch_pr_falls_back_to_rest_when_graphql_denied(monkeypatch):