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 7e43e6aa6a..44145d4ec4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,182 @@ 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 + 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 + 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 + `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. + 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 + 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 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, 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. + - `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. + - 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. + - 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. - Fix one more Devin Review finding on PR #1452, a genuine gap in the round-4 malformed-gateway-reply fix (`scripts/ci/contextual_orchestrator_review_sidecar.sh`, `tests/test_contextual_orchestrator_review_runtime_preflight.py`): diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index c9804b492e..4644d3e78b 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.""" @@ -793,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.""" @@ -802,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(), @@ -831,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, @@ -1148,19 +1180,105 @@ def opencode_in_progress(pr: dict[str, Any], *, stale_after_minutes: int | None return opencode_progress_state(pr, stale_after_minutes=stale_after) == "running" -def strix_evidence_state(pr: dict[str, Any]) -> str: - """Return missing, running, or complete for current-head Strix evidence.""" - found = False - for node in context_nodes(pr): - if not is_strix_context(node): +_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 - found = True + 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. + + "complete" requires authoritative success (CheckRun conclusion or classic + 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" + saw_running = False + for node in strix_nodes: + is_check_run = node.get("__typename") == "CheckRun" 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" + saw_running = True + continue + if is_check_run: + if status != "COMPLETED": + saw_running = True + continue + conclusion = (node.get("conclusion") or "").upper() + 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: @@ -1480,43 +1598,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: @@ -1842,7 +1933,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}" @@ -2377,6 +2468,199 @@ 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. + + 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. 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. 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) + 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( + 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") + # 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", + "draft PR review-only dispatch; current-head OpenCode verdict already exists", + ) + strix_state = strix_evidence_state(pr) + if strix_state in {"missing", "failed"}: + 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 +2677,25 @@ 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 trigger_reviews and ( + allow_draft_review_dispatch or active_draft_review_request(repo, pr) + ): + 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: @@ -2862,7 +3159,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", @@ -3936,6 +4233,22 @@ 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. 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") parser.add_argument("--trigger-reviews", action=argparse.BooleanOptionalAction, default=True) parser.add_argument( @@ -3994,6 +4307,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 +4349,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..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): @@ -1036,10 +1058,117 @@ 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" + + # 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": { + "nodes": [ + {"context": "strix", "state": "FAILURE"}, + strix_check(), + ] + } + } ) + assert sched.strix_evidence_state(checkrun_success_classic_failure) == "complete" + # 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": { + "nodes": [ + {"context": "strix", "state": "SUCCESS"}, + strix_check(conclusion="FAILURE"), + ] + } + } + ) + 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": { + "nodes": [ + {"context": "strix", "state": "FAILURE"}, + strix_check(status="IN_PROGRESS", conclusion=""), + ] + } + } + ) + 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 + # 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={ @@ -3001,6 +3130,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") @@ -3218,6 +3359,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,6 +3874,406 @@ 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(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( + 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_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_via_dispatch_token", 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_via_dispatch_token", 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_via_dispatch_token", 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_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 + 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"): + 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.""" + 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 == "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 == [] + + +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_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, + 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_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(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 = [ {"name": "Other", "id": 10, "head_sha": "old", "pull_requests": [{"number": 1}]}, @@ -4099,6 +4641,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( @@ -4872,6 +5454,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")],