fix: dispatch OpenCode review for a draft PR on explicit mention

.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: |

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-<repo>-<pr>-<head-sha>`) 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`):