From 98ee393aa387a2afd8cd6e7719e3fa3099547115 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 11:13:08 +0000 Subject: [PATCH 01/10] fix(ci): exempt draft PRs from the opencode-review current-head verdict gate opencode-review.yml's required opencode-review-target check ("Fail closed without a current-head OpenCode verdict") unconditionally demanded an APPROVED/CHANGES_REQUESTED review from opencode-agent on the current head for every opened/synchronize/reopened event, with no draft handling. Meanwhile scripts/ci/pr_review_merge_scheduler.py deliberately never dispatches an OpenCode review request for a draft PR: if pr.get("isDraft"): return Decision(number, "skip", "draft PR") Net effect: every draft PR showed this required check as a hard exit-1 failure on every push, forever, until marked ready for review -- a permanent false alarm, not a transient/pending state. Add a github.event.pull_request.draft early-exit mirroring the existing closed early-exit exactly in style and placement (right after it, before the PR_NUMBER/HEAD_SHA check). The job still always runs and always reports a status -- it just reports success instead of a misleading failure for a state where a verdict was never going to be requested. Once the PR is marked ready for review, ready_for_review and subsequent synchronize events carry draft: false, so the real gate applies unchanged. Adds shell-level regression coverage in tests/test_opencode_required_verdict_regression.py that executes the production step body directly (draft short-circuits before any Reviews API call, closed still takes precedence over draft, and non-draft PRs still genuinely require a verdict). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 4 + CHANGELOG.md | 16 ++ ...st_opencode_required_verdict_regression.py | 197 ++++++++++++++++++ 3 files changed, 217 insertions(+) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index d66979d406..6dc2c5bbd8 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -250,6 +250,10 @@ jobs: echo "PR closed; a current-head OpenCode verdict is not required." exit 0 fi + if [ "${{ github.event.pull_request.draft }}" = "true" ]; then + echo "PR is a draft; a current-head OpenCode verdict is not required until it is marked ready for review." + exit 0 + fi if [ -z "${PR_NUMBER:-}" ] || [ -z "${HEAD_SHA:-}" ]; then echo "::error::Missing PR number or head SHA; cannot verify a current-head OpenCode verdict." exit 1 diff --git a/CHANGELOG.md b/CHANGELOG.md index fc84661ed6..208b082b78 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 `opencode-review.yml`'s required `opencode-review-target` check + (`Fail closed without a current-head OpenCode verdict`) reporting a hard + `exit 1` failure on every push to a draft PR, forever, until the PR is + marked ready for review. Root cause: `scripts/ci/pr_review_merge_scheduler.py` + deliberately never dispatches an OpenCode review request for a draft PR + (`if pr.get("isDraft"): return Decision(number, "skip", "draft PR")`), but + the required check had no draft handling and unconditionally demanded a + current-head verdict on every `opened`/`synchronize`/`reopened` event + regardless. Adds a `github.event.pull_request.draft` early-exit mirroring + the existing `closed` early-exit in style and placement — the job still + always runs and reports a status, it just reports success instead of a + false-alarm failure for a state that was never going to get a verdict in + the first place. The gate is unchanged once a PR is marked ready for + review (`ready_for_review` and subsequent `synchronize` events carry + `draft: false`). See `tests/test_opencode_required_verdict_regression.py` + for shell-level regression coverage executing the production step body. - 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/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 881e93c2ef..a0570fa9fe 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -3,7 +3,9 @@ from __future__ import annotations import json +import os import shutil +import stat import subprocess from pathlib import Path @@ -13,6 +15,7 @@ HEAD = "a" * 40 WORKFLOW = Path(".github/workflows/opencode-review.yml") STATUS_HELPER = Path("scripts/ci/opencode_dispatch_status.py") +STEP_NAME = "Fail closed without a current-head OpenCode verdict" def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, object]: @@ -106,3 +109,197 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non "Review approval remains a separate current-head PR review requirement" not in workflow ) + + +def _extract_run_block(workflow_text: str, step_name: str) -> str: + """Return the literal bash text of one workflow step's ``run: |`` block.""" + lines = workflow_text.splitlines() + step_index = next( + index for index, line in enumerate(lines) if line.strip() == f"- name: {step_name}" + ) + run_index = next( + index + for index in range(step_index + 1, len(lines)) + if lines[index].strip() == "run: |" + ) + run_indent = len(lines[run_index]) - len(lines[run_index].lstrip()) + block_lines = [] + for line in lines[run_index + 1 :]: + if line.strip() and len(line) - len(line.lstrip()) <= run_indent: + break + block_lines.append(line[run_indent + 2 :] if len(line) >= run_indent + 2 else "") + return "\n".join(block_lines) + "\n" + + +def _render_step(script: str, *, event_action: str, draft: str) -> str: + """Substitute the two inline ``${{ github.* }}`` expressions GitHub Actions + would resolve before invoking bash, so the raw step body becomes directly + executable outside of Actions.""" + rendered = script.replace("${{ github.event.action }}", event_action) + rendered = rendered.replace("${{ github.event.pull_request.draft }}", draft) + assert "${{" not in rendered, "unresolved GitHub Actions expression remains" + return rendered + + +def _write_refusing_gh(bin_dir: Path) -> None: + """Install a fake ``gh`` on PATH that fails loudly if it is ever invoked. + + Used to prove an early-exit branch never reaches the Reviews API call. + """ + fake_gh = bin_dir / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "echo 'unexpected gh invocation: the early-exit should have short-circuited' >&2\n" + "exit 17\n", + encoding="utf-8", + ) + fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) + + +def _write_reviews_gh(bin_dir: Path, reviews: list[dict[str, object]]) -> Path: + """Install a fake ``gh`` on PATH that serves a fixed Reviews API page.""" + fake_gh = bin_dir / "gh" + fixture = bin_dir / "reviews.json" + fixture.write_text(json.dumps(reviews), encoding="utf-8") + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + 'test "$1" = api\n' + f"cat {fixture}\n", + encoding="utf-8", + ) + fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) + return fake_gh + + +def _run_step( + tmp_path: Path, + *, + event_action: str, + draft: str, + pr_number: str = "", + head_sha: str = "", + gh_fixture: str = "refuse", + reviews: list[dict[str, object]] | None = None, +) -> subprocess.CompletedProcess[str]: + """Execute the production step body with the given event shape. + + ``gh_fixture`` selects a fake ``gh`` on PATH: ``"refuse"`` fails loudly if + invoked (proving an early exit never reaches the Reviews API call), and + ``"reviews"`` serves ``reviews`` back from ``gh api``. + """ + bash = shutil.which("bash") + jq = shutil.which("jq") + if bash is None or jq is None: + pytest.skip("bash and jq are required to execute the production step body") + + workflow = WORKFLOW.read_text(encoding="utf-8") + script = _render_step( + _extract_run_block(workflow, STEP_NAME), + event_action=event_action, + draft=draft, + ) + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + if gh_fixture == "refuse": + _write_refusing_gh(bin_dir) + else: + _write_reviews_gh(bin_dir, reviews or []) + + env = { + **os.environ, + "PATH": f"{bin_dir}:{os.environ['PATH']}", + "GH_TOKEN": "fake-token", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": pr_number, + "HEAD_SHA": head_sha, + } + return subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env=env, + ) + + +def test_draft_pr_short_circuits_before_the_reviews_api_call(tmp_path: Path) -> None: + """A draft PR passes without ever calling the Reviews API. + + The merge scheduler (``scripts/ci/pr_review_merge_scheduler.py``) never + dispatches a review request for a draft PR, so this required check must + not demand a verdict that was never going to be requested. ``PR_NUMBER`` + and ``HEAD_SHA`` are deliberately left unset here to prove the draft + early-exit runs before the "missing PR number or head SHA" fail-closed + check that follows it. + """ + result = _run_step( + tmp_path, + event_action="synchronize", + draft="true", + gh_fixture="refuse", + ) + assert result.returncode == 0, result.stderr + assert "PR is a draft" in result.stdout + + +@pytest.mark.parametrize("event_action", ("opened", "synchronize", "reopened")) +def test_draft_pr_short_circuits_on_every_non_closed_event_type( + tmp_path: Path, event_action: str +) -> None: + """The draft exemption applies uniformly across opened/synchronize/reopened.""" + result = _run_step( + tmp_path, + event_action=event_action, + draft="true", + gh_fixture="refuse", + ) + assert result.returncode == 0, result.stderr + assert "PR is a draft" in result.stdout + + +def test_ready_for_review_pr_still_requires_a_current_head_verdict( + tmp_path: Path, +) -> None: + """Once a PR is not a draft, the real gate still runs unchanged.""" + result = _run_step( + tmp_path, + event_action="ready_for_review", + draft="false", + pr_number="1437", + head_sha=HEAD, + gh_fixture="reviews", + reviews=[review(state="APPROVED")], + ) + assert result.returncode == 0, result.stderr + assert "Current-head OpenCode verdict: APPROVED." in result.stdout + + +def test_non_draft_pr_without_a_verdict_still_fails_closed(tmp_path: Path) -> None: + """A non-draft PR with no matching review still fails closed as before.""" + result = _run_step( + tmp_path, + event_action="synchronize", + draft="false", + pr_number="1437", + head_sha=HEAD, + gh_fixture="reviews", + reviews=[], + ) + assert result.returncode == 1 + assert "No APPROVED or CHANGES_REQUESTED from opencode-agent" in result.stdout + + +def test_closed_pr_short_circuits_before_the_draft_check(tmp_path: Path) -> None: + """The pre-existing ``closed`` early-exit still takes precedence over draft.""" + result = _run_step( + tmp_path, + event_action="closed", + draft="true", + gh_fixture="refuse", + ) + assert result.returncode == 0, result.stderr + assert "PR closed; a current-head OpenCode verdict is not required." in result.stdout + assert "PR is a draft" not in result.stdout From 644255af8376c2645cb85a32adfe08e5db7ab31b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 12:22:25 +0000 Subject: [PATCH 02/10] fix(ci): rerun the opencode-review draft gate on draft reconversion A ready PR converted back to draft with no new commit never fired the required-workflow gate again (converted_to_draft wasn't in its trigger list), so a previously failed opencode-review check stayed failed forever even though the existing draft exemption would have passed it. Add converted_to_draft to opencode-review.yml's pull_request_target types, and update the matching contract/regression tests. Found by Devin's automated review on PR #1443. --- .github/workflows/opencode-review.yml | 2 +- CHANGELOG.md | 6 ++++++ scripts/ci/test_strix_quick_gate.sh | 2 +- ...st_opencode_required_verdict_regression.py | 19 ++++++++++++++++++- .../test_required_workflow_queue_contract.py | 5 +++-- 5 files changed, 29 insertions(+), 5 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 6dc2c5bbd8..45b4f570c0 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -9,7 +9,7 @@ on: # content and never binds repository secrets. Privileged review execution is # isolated in opencode-review-dispatch.yml on repository_dispatch only. pull_request_target: - types: [opened, synchronize, reopened, ready_for_review, closed] + types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: group: >- diff --git a/CHANGELOG.md b/CHANGELOG.md index 208b082b78..5ca78df098 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,12 @@ Semantic Versioning where the repository publishes a release. review (`ready_for_review` and subsequent `synchronize` events carry `draft: false`). See `tests/test_opencode_required_verdict_regression.py` for shell-level regression coverage executing the production step body. + Follow-up: add `converted_to_draft` to the same workflow's + `pull_request_target` trigger types. Without it, a ready PR converted + back to draft with no new commit never gets a fresh required-workflow + run, so its previously failed `opencode-review` check keeps showing + failure indefinitely even though the draft exemption above would have + passed it. - 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/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index b528e8bafc..17a63f111e 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -499,7 +499,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { local opencode_config="$REPO_ROOT/opencode.jsonc" assert_file_contains "$bootstrap_file" "pull_request_target:" "opencode required workflow loads its metadata-only bootstrap from the protected base ref" - assert_file_contains "$bootstrap_file" "types: [opened, synchronize, reopened, ready_for_review, closed]" "opencode required workflow reacts to current PR head changes and closed-PR cleanup" + assert_file_contains "$bootstrap_file" "types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]" "opencode required workflow reacts to current PR head changes, draft reconversion, and closed-PR cleanup" assert_file_contains "$bootstrap_file" "required-workflow-bootstrap:" "opencode required workflow materializes at least one job for pull_request ruleset runs" assert_file_contains "$bootstrap_file" "Required OpenCode workflow materialized without checking out or" "opencode required workflow bootstrap documents its data-only trust boundary" assert_file_contains "$bootstrap_file" "coverage-source-tree:" "opencode required workflow preserves the stable coverage-source-tree branch-protection context" diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index a0570fa9fe..ca676a06a5 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -111,6 +111,21 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) +def test_required_workflow_reruns_on_draft_reconversion() -> None: + """A ready PR converted back to draft must get a fresh required-workflow run. + + Without ``converted_to_draft`` in the trigger list, a PR that goes + ready -> draft with no new commit keeps its previously failed + ``opencode-review`` check forever: no event refires the job that could + apply the draft exemption below. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + assert ( + "types: [opened, synchronize, reopened, ready_for_review, " + "converted_to_draft, closed]" + ) in workflow + + def _extract_run_block(workflow_text: str, step_name: str) -> str: """Return the literal bash text of one workflow step's ``run: |`` block.""" lines = workflow_text.splitlines() @@ -245,7 +260,9 @@ def test_draft_pr_short_circuits_before_the_reviews_api_call(tmp_path: Path) -> assert "PR is a draft" in result.stdout -@pytest.mark.parametrize("event_action", ("opened", "synchronize", "reopened")) +@pytest.mark.parametrize( + "event_action", ("opened", "synchronize", "reopened", "converted_to_draft") +) def test_draft_pr_short_circuits_on_every_non_closed_event_type( tmp_path: Path, event_action: str ) -> None: diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 77594cc1fb..51632ff5d7 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -425,8 +425,9 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - assert "github.event.action != 'closed'" in workflow opencode_bootstrap = workflow_text("opencode-review.yml") - assert "types: [opened, synchronize, reopened, ready_for_review, closed]" in ( - opencode_bootstrap + assert ( + "types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]" + in opencode_bootstrap ) assert "actions/checkout" not in opencode_bootstrap assert "${{ secrets." not in opencode_bootstrap From 0999e2c74ceb24feabfe4fa3370cc9b400b3dab2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 22:28:36 +0000 Subject: [PATCH 03/10] fix(ci): also skip the OpenCode dispatch step for draft PRs opencode-review-target's earlier "Request current-head OpenCode review execution" step still ran unconditionally (only excluding closed events), performing its own OIDC token exchange, app-token exchange, and repository_dispatch call under set -euo pipefail. Any transient failure there could fail the job before the later step's draft exemption ever ran, keeping the required check red on draft PRs during an infrastructure outage unrelated to draft status. Gate this step on !github.event.pull_request.draft as well, and add a regression test asserting the exact if: condition on the raw workflow YAML. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 2 +- CHANGELOG.md | 14 +++++++++ ...st_opencode_required_verdict_regression.py | 31 +++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 6885d0ba71..b540148f25 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -242,7 +242,7 @@ jobs: id-token: write steps: - name: Request current-head OpenCode review execution - if: github.event.action != 'closed' + if: github.event.action != 'closed' && !github.event.pull_request.draft env: OIDC_AUDIENCE: opencode-github-action OPENCODE_API_BASE_URL: https://api.opencode.ai diff --git a/CHANGELOG.md b/CHANGELOG.md index df500b25e8..5d120d9135 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,20 @@ Semantic Versioning where the repository publishes a release. run, so its previously failed `opencode-review` check keeps showing failure indefinitely even though the draft exemption above would have passed it. +- Also gate `opencode-review-target`'s earlier `Request current-head + OpenCode review execution` step on `!github.event.pull_request.draft`, + not only the later `Fail closed without a current-head OpenCode verdict` + step. That earlier step performs its own OIDC token exchange, OpenCode + app-token exchange, and `repository_dispatch` call under + `set -euo pipefail`; any transient failure there (an OIDC hiccup, the + OpenCode token endpoint erroring, a `gh api` dispatch failure) stopped + the job with `exit 1` before the later step's draft exemption ever ran, + so a genuinely infrastructure-caused outage could still turn a draft + PR's required check red even after the fix above. The dispatch step now + short-circuits for drafts exactly like the verdict step does, while + still running for every non-draft, non-`closed` event. See the new + `test_request_review_execution_step_is_also_gated_on_draft` regression + test asserting the exact `if:` condition string on this step. - Harden the review sidecar's per-account catalog cap against silent drift: `contextual_orchestrator_review_launcher.py`'s two `build_zdr_prioritized_catalog` call sites now source their diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index c39c65609c..6524f989ba 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -137,6 +137,37 @@ def test_required_workflow_reruns_on_draft_reconversion() -> None: ) in workflow +def test_request_review_execution_step_is_also_gated_on_draft() -> None: + """The dispatch step must not run for drafts either. + + ``Fail closed without a current-head OpenCode verdict`` exempts drafts, + but the earlier ``Request current-head OpenCode review execution`` step + performs its own OIDC token exchange, OpenCode app-token exchange, and + ``repository_dispatch`` call under ``set -euo pipefail``, any of which + can fail on infrastructure trouble unrelated to draft status. Without a + matching draft guard on this step's own ``if:``, that failure happens + before the later exemption ever gets a chance to run, keeping the + required check red on every draft PR whenever OIDC or the dispatch API + has trouble. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + lines = workflow.splitlines() + step_index = next( + index + for index, line in enumerate(lines) + if line.strip() == "- name: Request current-head OpenCode review execution" + ) + if_line = next( + line.strip() + for line in lines[step_index + 1 :] + if line.strip().startswith("if:") + ) + assert ( + if_line + == "if: github.event.action != 'closed' && !github.event.pull_request.draft" + ) + + def _extract_run_block(workflow_text: str, step_name: str) -> str: """Return the literal bash text of one workflow step's ``run: |`` block.""" lines = workflow_text.splitlines() From 0de29e08b655d79ce4e1bb249b12d280a0716453 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 03:37:19 +0000 Subject: [PATCH 04/10] fix(ci): stop trusting stale event payload for opencode-review draft/closed state Devin review on #1443: a manual re-run of an old workflow run (e.g. a stale converted_to_draft run) replays that event's stored github.event.* fields verbatim, so a since-ready, unreviewed PR at the same head SHA could pass the required opencode-review check on a stale "still draft" reading. The verdict step now decides closed/draft from the pull request's live state via gh api instead of the triggering event's payload, and fails closed if that lookup itself fails. --- .github/workflows/opencode-review.yml | 26 ++- CHANGELOG.md | 12 ++ ...st_opencode_required_verdict_regression.py | 185 ++++++++++++++---- 3 files changed, 174 insertions(+), 49 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 38563208d2..b8e68afff7 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -264,24 +264,34 @@ jobs: TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} - PR_ACTION: ${{ github.event.action }} - PR_DRAFT: ${{ github.event.pull_request.draft }} run: | set -euo pipefail - if [ "$PR_ACTION" = "closed" ]; then + if [ -z "${PR_NUMBER:-}" ] || [ -z "${HEAD_SHA:-}" ]; then + echo "::error::Missing PR number or head SHA; cannot verify a current-head OpenCode verdict." + exit 1 + fi + # Decide closed/draft from the pull request's live state, never from + # the triggering event's own stored payload -- a manual re-run of an + # old workflow run (e.g. an old converted_to_draft event) replays + # that event's github.event.* fields verbatim, which would let a + # since-ready, unreviewed PR at the same head SHA pass this required + # check on a stale "still draft" reading (Devin review on #1443). + if ! pr="$(timeout 25 gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")"; then + echo "::error::Could not fetch the pull request's live state; cannot verify a current-head OpenCode verdict." + exit 1 + fi + pr_state="$(printf '%s' "$pr" | jq -r '.state // ""')" + pr_draft="$(printf '%s' "$pr" | jq -r '.draft // false')" + if [ "$pr_state" = "closed" ]; then echo "PR closed; a current-head OpenCode verdict is not required." echo "verdict=CLOSED" >>"$GITHUB_OUTPUT" exit 0 fi - if [ "$PR_DRAFT" = "true" ]; then + if [ "$pr_draft" = "true" ]; then echo "PR is a draft; a current-head OpenCode verdict is not required until it is marked ready for review." echo "verdict=DRAFT" >>"$GITHUB_OUTPUT" exit 0 fi - if [ -z "${PR_NUMBER:-}" ] || [ -z "${HEAD_SHA:-}" ]; then - echo "::error::Missing PR number or head SHA; cannot verify a current-head OpenCode verdict." - exit 1 - fi if ! reviews="$(timeout 25 gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")"; then reviews="[]" fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 86ab08ffce..fe5c13bdf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,18 @@ Semantic Versioning where the repository publishes a release. shell-level regression coverage executing each step's actual production body, including the new `_run_verdict_step`/`_run_fail_closed_step` helpers matching the three-step split. +- Fix a review-flagged gap in the draft-gate fix above (Devin review on + `#1443`): the `Resolve current-head formal OpenCode verdict` step decided + closed/draft purely from the triggering event's own stored payload + (`github.event.action`/`github.event.pull_request.draft`). A manual + re-run of an old workflow run (e.g. a stale `converted_to_draft` run) + replays that event's payload verbatim, so a since-ready, unreviewed PR at + the same head SHA could pass this required check on a stale "still draft" + reading — a real required-review bypass, not merely a false alarm. The + step now fetches the pull request's live state (`gh api + repos/.../pulls/`) and decides closed/draft from that instead, + failing closed if the lookup itself fails; the two payload-derived `env:` + vars (`PR_ACTION`/`PR_DRAFT`) are removed entirely from this step. - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8353e79619..f53c3b0397 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -211,16 +211,63 @@ def _write_refusing_gh(bin_dir: Path) -> None: fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) -def _write_reviews_gh(bin_dir: Path, reviews: list[dict[str, object]]) -> Path: - """Install a fake ``gh`` on PATH that serves a fixed Reviews API page.""" +def _write_reviews_gh( + bin_dir: Path, + reviews: list[dict[str, object]], + *, + pr_state: str = "open", + pr_draft: bool = False, +) -> Path: + """Install a fake ``gh`` on PATH serving a live PR object, then Reviews API page. + + The production step now fetches the pull request's own live state + (``pulls/``, no ``--paginate``) before the paginated Reviews API + call, so this fixture dispatches on the presence of ``--paginate`` in the + arguments rather than assuming only one ``gh api`` shape is ever called. + """ fake_gh = bin_dir / "gh" - fixture = bin_dir / "reviews.json" - fixture.write_text(json.dumps(reviews), encoding="utf-8") + pr_fixture = bin_dir / "pr.json" + reviews_fixture = bin_dir / "reviews.json" + pr_fixture.write_text( + json.dumps({"state": pr_state, "draft": pr_draft}), encoding="utf-8" + ) + reviews_fixture.write_text(json.dumps(reviews), encoding="utf-8") fake_gh.write_text( "#!/usr/bin/env bash\n" "set -euo pipefail\n" 'test "$1" = api\n' - f"cat {fixture}\n", + "if [[ \" $* \" == *' --paginate '* ]]; then\n" + f" cat {reviews_fixture}\n" + "else\n" + f" cat {pr_fixture}\n" + "fi\n", + encoding="utf-8", + ) + fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) + return fake_gh + + +def _write_closed_or_draft_gh(bin_dir: Path, *, pr_state: str, pr_draft: bool) -> Path: + """Install a fake ``gh`` on PATH that serves only the live PR object. + + Used for closed/draft cases that must short-circuit before any Reviews + API call, mirroring ``_write_refusing_gh``'s "prove the early exit" + intent but for the live-state call that now precedes it. + """ + fake_gh = bin_dir / "gh" + pr_fixture = bin_dir / "pr.json" + pr_fixture.write_text( + json.dumps({"state": pr_state, "draft": pr_draft}), encoding="utf-8" + ) + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + 'test "$1" = api\n' + "if [[ \" $* \" == *' --paginate '* ]]; then\n" + " echo 'unexpected Reviews API call: the early-exit should have short-circuited' >&2\n" + " exit 17\n" + "fi\n" + f"cat {pr_fixture}\n", encoding="utf-8", ) fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) @@ -230,23 +277,27 @@ def _write_reviews_gh(bin_dir: Path, reviews: list[dict[str, object]]) -> Path: def _run_verdict_step( tmp_path: Path, *, - event_action: str, - draft: str, pr_number: str = "", head_sha: str = "", gh_fixture: str = "refuse", + pr_state: str = "open", + pr_draft: bool = False, reviews: list[dict[str, object]] | None = None, ) -> subprocess.CompletedProcess[str]: """Execute the "Resolve current-head formal OpenCode verdict" step body. - Unlike the old poll-based design, ``PR_ACTION``/``PR_DRAFT`` are passed - the same way GitHub Actions passes them in production: as plain ``env:`` - variables, not inline ``${{ }}`` expressions substituted into the script - text. ``gh_fixture`` selects a fake ``gh`` on PATH: ``"refuse"`` fails - loudly if invoked (proving an early exit never reaches the Reviews API - call), and ``"reviews"`` serves ``reviews`` back from ``gh api``. The - step's ``$GITHUB_OUTPUT`` writes are captured in ``tmp_path / - "github_output"`` for the caller to inspect. + The production step decides closed/draft from the pull request's own + live state (a ``gh api repos/.../pulls/`` call), never from the + triggering event's own stored payload -- so ``pr_state``/``pr_draft`` + drive a fake ``gh``'s live-PR-object response, not env vars. ``gh_fixture`` + selects which fake ``gh`` goes on PATH: ``"refuse"`` fails loudly if + invoked at all (proving the missing-PR_NUMBER/HEAD_SHA guard runs before + any API call), ``"closed_or_draft"`` serves only the live PR object and + fails loudly on a Reviews API call (proving that early exit short-circuits + before it), and ``"reviews"`` serves the live PR object and then + ``reviews`` from the paginated Reviews API call. The step's + ``$GITHUB_OUTPUT`` writes are captured in ``tmp_path / "github_output"`` + for the caller to inspect. """ bash = shutil.which("bash") jq = shutil.which("jq") @@ -262,8 +313,10 @@ def _run_verdict_step( bin_dir.mkdir() if gh_fixture == "refuse": _write_refusing_gh(bin_dir) + elif gh_fixture == "closed_or_draft": + _write_closed_or_draft_gh(bin_dir, pr_state=pr_state, pr_draft=pr_draft) else: - _write_reviews_gh(bin_dir, reviews or []) + _write_reviews_gh(bin_dir, reviews or [], pr_state=pr_state, pr_draft=pr_draft) output_file = tmp_path / "github_output" output_file.write_text("", encoding="utf-8") @@ -274,8 +327,6 @@ def _run_verdict_step( "TARGET_REPOSITORY": "ContextualWisdomLab/example", "PR_NUMBER": pr_number, "HEAD_SHA": head_sha, - "PR_ACTION": event_action, - "PR_DRAFT": draft, "GITHUB_OUTPUT": str(output_file), } return subprocess.run( @@ -319,34 +370,44 @@ def test_draft_pr_verdict_step_short_circuits_before_the_reviews_api_call( The merge scheduler (``scripts/ci/pr_review_merge_scheduler.py``) never dispatches a review request for a draft PR, so this required check must - not demand a verdict that was never going to be requested. ``PR_NUMBER`` - and ``HEAD_SHA`` are deliberately left unset here to prove the draft - early-exit runs before the "missing PR number or head SHA" fail-closed - check that follows it. + not demand a verdict that was never going to be requested. The live PR + object (not a stale event field) is the source of the draft state, and + the fake ``gh`` refuses any Reviews API call to prove the early-exit + short-circuits before it. """ result = _run_verdict_step( tmp_path, - event_action="synchronize", - draft="true", - gh_fixture="refuse", + pr_number="1437", + head_sha=HEAD, + gh_fixture="closed_or_draft", + pr_state="open", + pr_draft=True, ) assert result.returncode == 0, result.stderr assert "PR is a draft" in result.stdout assert "verdict=DRAFT" in (tmp_path / "github_output").read_text(encoding="utf-8") -@pytest.mark.parametrize( - "event_action", ("opened", "synchronize", "reopened", "converted_to_draft") -) -def test_draft_pr_verdict_step_short_circuits_on_every_non_closed_event_type( - tmp_path: Path, event_action: str +def test_draft_pr_verdict_step_ignores_the_stale_triggering_event_action( + tmp_path: Path, ) -> None: - """The draft exemption applies uniformly across opened/synchronize/reopened.""" + """A stale re-run of an old event must not resurrect a bypass. + + Regardless of which event originally triggered this run (opened, + synchronize, reopened, converted_to_draft -- even a replayed old run), + the verdict step now asks GitHub for the pull request's live state + instead of trusting that event's own stored payload, so a live draft PR + is exempted the same way no matter which stale action label the + original workflow run happened to carry (Devin review on #1443, on the + inverse of this case: a stale run must not falsely claim draft either). + """ result = _run_verdict_step( tmp_path, - event_action=event_action, - draft="true", - gh_fixture="refuse", + pr_number="1437", + head_sha=HEAD, + gh_fixture="closed_or_draft", + pr_state="open", + pr_draft=True, ) assert result.returncode == 0, result.stderr assert "PR is a draft" in result.stdout @@ -358,11 +419,11 @@ def test_ready_for_review_pr_still_requires_a_current_head_verdict( """Once a PR is not a draft, the real gate still runs unchanged.""" result = _run_verdict_step( tmp_path, - event_action="ready_for_review", - draft="false", pr_number="1437", head_sha=HEAD, gh_fixture="reviews", + pr_state="open", + pr_draft=False, reviews=[review(state="APPROVED")], ) assert result.returncode == 0, result.stderr @@ -374,11 +435,11 @@ def test_non_draft_pr_without_a_verdict_leaves_the_gate_empty(tmp_path: Path) -> """A non-draft PR with no matching review resolves an empty verdict.""" result = _run_verdict_step( tmp_path, - event_action="synchronize", - draft="false", pr_number="1437", head_sha=HEAD, gh_fixture="reviews", + pr_state="open", + pr_draft=False, reviews=[], ) assert result.returncode == 0, result.stderr @@ -394,9 +455,11 @@ def test_closed_pr_short_circuits_before_the_draft_check(tmp_path: Path) -> None """The pre-existing ``closed`` early-exit still takes precedence over draft.""" result = _run_verdict_step( tmp_path, - event_action="closed", - draft="true", - gh_fixture="refuse", + pr_number="1437", + head_sha=HEAD, + gh_fixture="closed_or_draft", + pr_state="closed", + pr_draft=True, ) assert result.returncode == 0, result.stderr assert "PR closed; a current-head OpenCode verdict is not required." in result.stdout @@ -404,6 +467,46 @@ def test_closed_pr_short_circuits_before_the_draft_check(tmp_path: Path) -> None assert "verdict=CLOSED" in (tmp_path / "github_output").read_text(encoding="utf-8") +def test_verdict_step_fails_closed_when_live_pr_state_is_unavailable( + tmp_path: Path, +) -> None: + """A failed live-state lookup fails closed instead of silently proceeding.""" + bash = shutil.which("bash") + jq = shutil.which("jq") + if bash is None or jq is None: + pytest.skip("bash and jq are required to execute the production step body") + workflow = WORKFLOW.read_text(encoding="utf-8") + script = _extract_run_block(workflow, "Resolve current-head formal OpenCode verdict") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + fake_gh = bin_dir / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\nexit 1\n", + encoding="utf-8", + ) + fake_gh.chmod(fake_gh.stat().st_mode | stat.S_IEXEC) + output_file = tmp_path / "github_output" + output_file.write_text("", encoding="utf-8") + result = subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env={ + **os.environ, + "PATH": f"{bin_dir}:{os.environ['PATH']}", + "GH_TOKEN": "fake-token", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "1437", + "HEAD_SHA": HEAD, + "GITHUB_OUTPUT": str(output_file), + }, + ) + assert result.returncode == 1 + assert "Could not fetch the pull request's live state" in result.stdout + + def test_fail_closed_step_passes_on_draft_verdict() -> None: """The trivial fail-closed step treats VERDICT=DRAFT like VERDICT=CLOSED.""" result = _run_fail_closed_step("DRAFT") From 0d897e22fa24a8ae2519e9ed792f89fdd4a895e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 03:45:34 +0000 Subject: [PATCH 05/10] fix(ci): gate opencode-review dispatch on the live verdict alone Devin review on #1443: the "Request current-head OpenCode review execution" step's if: still combined github.event.action/ github.event.pull_request.draft with steps.verdict.outputs.verdict == ''. Once the verdict step resolves closed/draft from live PR state, those stale-payload conjuncts became a liability: a manual re-run of an old closed/draft-era job could still suppress the dispatch for a since- reopened/ready PR at the same head SHA, leaving the required check red with no review ever requested. Gate on the live verdict signal alone. --- .github/workflows/opencode-review.yml | 12 +++++++- CHANGELOG.md | 12 ++++++++ ...st_opencode_required_verdict_regression.py | 29 +++++++++---------- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index b8e68afff7..d4e6209857 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -329,7 +329,17 @@ jobs: fi - name: Request current-head OpenCode review execution - if: github.event.action != 'closed' && !github.event.pull_request.draft && steps.verdict.outputs.verdict == '' + # steps.verdict.outputs.verdict alone is the live, authoritative + # signal now: the verdict step resolves closed/draft from a live + # gh api call, not this event's own stored payload, so it is already + # empty only when a real dispatch is actually needed. The former + # extra github.event.action/github.event.pull_request.draft conjuncts + # were themselves stale-payload reads -- a manual re-run of an old + # closed/draft-era job could still suppress this dispatch on a since- + # reopened/ready PR even after the verdict step's own fix, leaving the + # required check red with no review ever requested (Devin review on + # #1443). + if: steps.verdict.outputs.verdict == '' env: OIDC_AUDIENCE: opencode-github-action OPENCODE_API_BASE_URL: https://api.opencode.ai diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5c13bdf6..9f5f68ed9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,18 @@ Semantic Versioning where the repository publishes a release. repos/.../pulls/`) and decides closed/draft from that instead, failing closed if the lookup itself fails; the two payload-derived `env:` vars (`PR_ACTION`/`PR_DRAFT`) are removed entirely from this step. +- Fix a second review-flagged gap in the same lineage (Devin review on + `#1443`): the `Request current-head OpenCode review execution` dispatch + step's own `if:` still gated on `github.event.action`/ + `github.event.pull_request.draft` alongside `steps.verdict.outputs.verdict + == ''`. Once the verdict step above was fixed to resolve closed/draft from + live state, that made the dispatch step's stale-payload conjuncts a + liability rather than a safety net: a manual re-run of an old closed/draft + job could still suppress the dispatch for a since-reopened/ready PR at the + same head SHA, leaving the required check red with no review ever + requested. The dispatch step now gates purely on + `steps.verdict.outputs.verdict == ''`, the one live-computed signal that + already means "a real dispatch is needed." - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index f53c3b0397..8adceeb3ee 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -145,17 +145,18 @@ def test_required_workflow_reruns_on_draft_reconversion() -> None: def test_request_review_execution_step_is_also_gated_on_draft() -> None: - """The dispatch step must not run for drafts either. - - ``Fail closed without a current-head OpenCode verdict`` exempts drafts, - but the earlier ``Request current-head OpenCode review execution`` step - performs its own OIDC token exchange, OpenCode app-token exchange, and - ``repository_dispatch`` call under ``set -euo pipefail``, any of which - can fail on infrastructure trouble unrelated to draft status. Without a - matching draft guard on this step's own ``if:``, that failure happens - before the later exemption ever gets a chance to run, keeping the - required check red on every draft PR whenever OIDC or the dispatch API - has trouble. + """The dispatch step's own gate must be the live verdict alone. + + ``Fail closed without a current-head OpenCode verdict`` exempts drafts + and closed PRs via ``steps.verdict.outputs.verdict``, which the verdict + step now resolves from the pull request's *live* state. The earlier + ``Request current-head OpenCode review execution`` step must gate on + that same live signal alone -- gating it (also) on + ``github.event.action``/``github.event.pull_request.draft`` would let a + manual re-run of an old closed/draft-era job suppress the dispatch for a + since-reopened/ready PR at the same head SHA using those stale payload + fields, even though the verdict step's own live-state fix means a real + dispatch is exactly what's needed then (Devin review on #1443). """ workflow = WORKFLOW.read_text(encoding="utf-8") lines = workflow.splitlines() @@ -169,11 +170,7 @@ def test_request_review_execution_step_is_also_gated_on_draft() -> None: for line in lines[step_index + 1 :] if line.strip().startswith("if:") ) - assert ( - if_line - == "if: github.event.action != 'closed' && !github.event.pull_request.draft " - "&& steps.verdict.outputs.verdict == ''" - ) + assert if_line == "if: steps.verdict.outputs.verdict == ''" def _extract_run_block(workflow_text: str, step_name: str) -> str: From ba405e8c52b8dfd85c1468fe267130fcd677d3ad Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 03:46:46 +0000 Subject: [PATCH 06/10] fix: remove trailing whitespace in CHANGELOG.md (git diff --check gate) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f5f68ed9f..6aec0dc793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,7 +35,7 @@ Semantic Versioning where the repository publishes a release. `#1443`): the `Resolve current-head formal OpenCode verdict` step decided closed/draft purely from the triggering event's own stored payload (`github.event.action`/`github.event.pull_request.draft`). A manual - re-run of an old workflow run (e.g. a stale `converted_to_draft` run) + re-run of an old workflow run (e.g. a stale `converted_to_draft` run) replays that event's payload verbatim, so a since-ready, unreviewed PR at the same head SHA could pass this required check on a stale "still draft" reading — a real required-review bypass, not merely a false alarm. The From ba267e95bf28ad59d7470e34d330b3f857d44750 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 03:58:55 +0000 Subject: [PATCH 07/10] fix(ci): source opencode-review dispatch payload from live PR state Devin review on #1443: even with the dispatch step correctly enabled from the live verdict, it still built its repository_dispatch payload's pr_base_ref/pr_base_sha/pr_head_ref/pr_head_sha from github.event.pull_request.* -- the same stale-payload source the prior two fixes removed from the pass/fail decision. On a manual re-run of an old job whose base branch has since advanced, opencode-review-dispatch.yml's live validate-pr-metadata check hard-rejects that stale base_sha, so the dispatch would fail even though the verdict step correctly decided one was needed. The verdict step now exposes base_ref/base_sha/head_ref/head_sha as step outputs from the same live gh api response it already uses for closed/draft, and the dispatch step builds its payload from those instead. --- .github/workflows/opencode-review.yml | 22 +++- CHANGELOG.md | 15 +++ ...st_opencode_required_verdict_regression.py | 101 +++++++++++++++++- 3 files changed, 131 insertions(+), 7 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index d4e6209857..56dba81ccf 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -282,6 +282,18 @@ jobs: fi pr_state="$(printf '%s' "$pr" | jq -r '.state // ""')" pr_draft="$(printf '%s' "$pr" | jq -r '.draft // false')" + # Exposed for the dispatch step below so its repository_dispatch + # payload also comes from this same live fetch, not from + # github.event.pull_request.* -- a manual re-run of an old job + # would otherwise still build the dispatch payload from that stale + # event payload, and opencode-review-dispatch.yml's live + # validate-pr-metadata check hard-rejects a stale base_ref/base_sha, + # so the dispatch would fail even once this step correctly decides + # one is needed (Devin review on #1443). + printf 'base_ref=%s\n' "$(printf '%s' "$pr" | jq -r '.base.ref')" >>"$GITHUB_OUTPUT" + printf 'base_sha=%s\n' "$(printf '%s' "$pr" | jq -r '.base.sha')" >>"$GITHUB_OUTPUT" + printf 'head_ref=%s\n' "$(printf '%s' "$pr" | jq -r '.head.ref')" >>"$GITHUB_OUTPUT" + printf 'head_sha=%s\n' "$(printf '%s' "$pr" | jq -r '.head.sha')" >>"$GITHUB_OUTPUT" if [ "$pr_state" = "closed" ]; then echo "PR closed; a current-head OpenCode verdict is not required." echo "verdict=CLOSED" >>"$GITHUB_OUTPUT" @@ -345,10 +357,12 @@ jobs: OPENCODE_API_BASE_URL: https://api.opencode.ai TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} - BASE_BRANCH: ${{ github.event.pull_request.base.ref }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_BRANCH: ${{ github.event.pull_request.head.ref }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # Sourced from the verdict step's own live gh api fetch, not + # github.event.pull_request.* -- see the comment on that step. + BASE_BRANCH: ${{ steps.verdict.outputs.base_ref }} + BASE_SHA: ${{ steps.verdict.outputs.base_sha }} + HEAD_BRANCH: ${{ steps.verdict.outputs.head_ref }} + HEAD_SHA: ${{ steps.verdict.outputs.head_sha }} run: | set -euo pipefail if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then diff --git a/CHANGELOG.md b/CHANGELOG.md index 6aec0dc793..15553b1a2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,21 @@ Semantic Versioning where the repository publishes a release. requested. The dispatch step now gates purely on `steps.verdict.outputs.verdict == ''`, the one live-computed signal that already means "a real dispatch is needed." +- Fix a third review-flagged gap in the same lineage (Devin review on + `#1443`): even with the dispatch step correctly enabled from the live + verdict, it still built its `repository_dispatch` payload's + `pr_base_ref`/`pr_base_sha`/`pr_head_ref`/`pr_head_sha` fields from + `github.event.pull_request.*` — the same stale-payload source the two + fixes above removed from the pass/fail decision. On a manual re-run of an + old job whose base branch has since advanced (head SHA unchanged), + `opencode-review-dispatch.yml`'s live `validate-pr-metadata` check + hard-rejects that stale `base_sha`, so the dispatch would fail even though + the verdict step correctly decided one was needed — leaving the required + check red with no review ever requested. The verdict step now exposes + `base_ref`/`base_sha`/`head_ref`/`head_sha` as step outputs from the same + live `gh api repos/.../pulls/` response it already uses for + closed/draft, and the dispatch step builds its payload from + `steps.verdict.outputs.*` instead of the event payload. - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8adceeb3ee..980b3173cf 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -214,6 +214,10 @@ def _write_reviews_gh( *, pr_state: str = "open", pr_draft: bool = False, + base_ref: str = "main", + base_sha: str = "b" * 40, + head_ref: str = "feature", + head_sha: str = HEAD, ) -> Path: """Install a fake ``gh`` on PATH serving a live PR object, then Reviews API page. @@ -221,12 +225,23 @@ def _write_reviews_gh( (``pulls/``, no ``--paginate``) before the paginated Reviews API call, so this fixture dispatches on the presence of ``--paginate`` in the arguments rather than assuming only one ``gh api`` shape is ever called. + The live PR object also carries ``base``/``head`` refs/shas, mirroring + what the production step now exposes as step outputs for the dispatch + step's payload. """ fake_gh = bin_dir / "gh" pr_fixture = bin_dir / "pr.json" reviews_fixture = bin_dir / "reviews.json" pr_fixture.write_text( - json.dumps({"state": pr_state, "draft": pr_draft}), encoding="utf-8" + json.dumps( + { + "state": pr_state, + "draft": pr_draft, + "base": {"ref": base_ref, "sha": base_sha}, + "head": {"ref": head_ref, "sha": head_sha}, + } + ), + encoding="utf-8", ) reviews_fixture.write_text(json.dumps(reviews), encoding="utf-8") fake_gh.write_text( @@ -254,7 +269,15 @@ def _write_closed_or_draft_gh(bin_dir: Path, *, pr_state: str, pr_draft: bool) - fake_gh = bin_dir / "gh" pr_fixture = bin_dir / "pr.json" pr_fixture.write_text( - json.dumps({"state": pr_state, "draft": pr_draft}), encoding="utf-8" + json.dumps( + { + "state": pr_state, + "draft": pr_draft, + "base": {"ref": "main", "sha": "b" * 40}, + "head": {"ref": "feature", "sha": HEAD}, + } + ), + encoding="utf-8", ) fake_gh.write_text( "#!/usr/bin/env bash\n" @@ -280,6 +303,10 @@ def _run_verdict_step( pr_state: str = "open", pr_draft: bool = False, reviews: list[dict[str, object]] | None = None, + base_ref: str = "main", + base_sha: str = "b" * 40, + head_ref: str = "feature", + live_head_sha: str = HEAD, ) -> subprocess.CompletedProcess[str]: """Execute the "Resolve current-head formal OpenCode verdict" step body. @@ -313,7 +340,16 @@ def _run_verdict_step( elif gh_fixture == "closed_or_draft": _write_closed_or_draft_gh(bin_dir, pr_state=pr_state, pr_draft=pr_draft) else: - _write_reviews_gh(bin_dir, reviews or [], pr_state=pr_state, pr_draft=pr_draft) + _write_reviews_gh( + bin_dir, + reviews or [], + pr_state=pr_state, + pr_draft=pr_draft, + base_ref=base_ref, + base_sha=base_sha, + head_ref=head_ref, + head_sha=live_head_sha, + ) output_file = tmp_path / "github_output" output_file.write_text("", encoding="utf-8") @@ -448,6 +484,65 @@ def test_non_draft_pr_without_a_verdict_leaves_the_gate_empty(tmp_path: Path) -> assert _run_fail_closed_step("").returncode == 1 +def test_verdict_step_exposes_live_base_and_head_for_the_dispatch_payload( + tmp_path: Path, +) -> None: + """The verdict step's live fetch also feeds the dispatch step's payload. + + A manual re-run of an old workflow run replays github.event.pull_request's + stored base/head refs/shas verbatim; if the base branch has since + advanced, opencode-review-dispatch.yml's live validate-pr-metadata check + hard-rejects that stale base_sha, so a dispatch that the verdict step + correctly decided is needed would still fail to actually request a + review (Devin review on #1443). base_ref/base_sha/head_ref/head_sha are + now emitted as step outputs from the same live gh api response the + verdict decision itself uses, deliberately using values distinct from + the event-derived HEAD_SHA env var passed in below to prove they come + from the live fetch, not from the caller's env. + """ + live_head = "c" * 40 + result = _run_verdict_step( + tmp_path, + pr_number="1437", + head_sha=HEAD, + gh_fixture="reviews", + pr_state="open", + pr_draft=False, + reviews=[], + base_ref="release/live", + base_sha="d" * 40, + head_ref="feature/live", + live_head_sha=live_head, + ) + assert result.returncode == 0, result.stderr + output = (tmp_path / "github_output").read_text(encoding="utf-8") + assert "base_ref=release/live" in output + assert f"base_sha={'d' * 40}" in output + assert "head_ref=feature/live" in output + assert f"head_sha={live_head}" in output + + +def test_dispatch_step_payload_sources_base_and_head_from_the_verdict_step() -> None: + """The dispatch step must build its payload from live outputs, not the event. + + ``steps.verdict.outputs.base_ref``/``base_sha``/``head_ref``/``head_sha`` + replace the former ``github.event.pull_request.base.ref``/``base.sha``/ + ``head.ref``/``head.sha`` reads in this step's own ``env:`` block. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + dispatch_step = workflow.split( + "- name: Request current-head OpenCode review execution", 1 + )[1].split("- name: Fail closed", 1)[0] + assert "steps.verdict.outputs.base_ref" in dispatch_step + assert "steps.verdict.outputs.base_sha" in dispatch_step + assert "steps.verdict.outputs.head_ref" in dispatch_step + assert "steps.verdict.outputs.head_sha" in dispatch_step + assert "github.event.pull_request.base.ref" not in dispatch_step + assert "github.event.pull_request.base.sha" not in dispatch_step + assert "github.event.pull_request.head.ref" not in dispatch_step + assert "github.event.pull_request.head.sha" not in dispatch_step + + def test_closed_pr_short_circuits_before_the_draft_check(tmp_path: Path) -> None: """The pre-existing ``closed`` early-exit still takes precedence over draft.""" result = _run_verdict_step( From d3a31ff89c1a050794fc7031ba307dc8a2a7a94f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:12:44 +0000 Subject: [PATCH 08/10] docs(gap-baseline): record opencode-review draft-gate fix traceability Append-only dated entry documenting the three-round live-state hardening on .github#1443 (draft-gate exemption, then two Devin-flagged stale-event- payload gaps in the dispatch if: and dispatch payload), tied to #1531's required-workflow queue-pressure tracking. --- docs/product-technical-gap-baseline.md | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..d33feccf88 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,47 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr "today" reference. Landed in the same PR (`#1463`) as the streaming revert, not split out, since the revert is unsafe without it. +## 2026-09-01 opencode-review.yml draft-gate fix (`.github#1443`): three-round live-state hardening, tied to `#1531` queue pressure + +**Buyer/control-plane effect.** Before this fix, every draft PR org-wide showed the required +`opencode-review` check as a hard, permanent `exit 1` failure: `scripts/ci/pr_review_merge_scheduler.py` +deliberately never dispatches an OpenCode review request for a draft PR +(`if pr.get("isDraft"): return Decision(number, "skip", "draft PR")`), but the required check had no +draft handling and unconditionally demanded a current-head verdict. Net effect: draft PRs allocated +required-workflow OpenCode review dispatch work they could never complete or merge from — the exact +kind of wasted required-workflow queue allocation `#1531` tracks — while presenting a false-alarm +failure with no actionable next step for the PR author. + +**Fix, in three review-driven rounds, all on `.github#1443`:** +1. The `Resolve current-head formal OpenCode verdict` step now exits early (`verdict=DRAFT`) for a + draft PR, mirroring its existing `closed` early exit, and the `Request current-head OpenCode review + execution` dispatch step's own `if:` matches — so a draft PR no longer dispatches OpenCode review + work it can never merge from. `converted_to_draft` was added to the workflow's `pull_request_target` + trigger types so a ready-to-draft conversion with no new commit still gets a fresh required-workflow + run that can apply the exemption. +2. Devin review found the draft/closed decision, and later the dispatch step's own `if:`, were still + reading `github.event.action`/`github.event.pull_request.draft` — the *triggering event's own stored + payload*. A manual re-run of an old workflow run (e.g. a stale `converted_to_draft` run) replays that + payload verbatim, so a since-ready, unreviewed PR at the same head SHA could pass the required check + on a stale "still draft" reading, or a since-ready PR's real dispatch could stay wrongly suppressed. + Both steps now decide from the pull request's *live* state (one `gh api repos/.../pulls/` + fetch per run), failing closed if the lookup itself fails. +3. Devin review (third round) found that even with the dispatch step correctly gated on the live + verdict, it still built its `repository_dispatch` payload's `pr_base_ref`/`pr_base_sha`/ + `pr_head_ref`/`pr_head_sha` from that same stale event payload — so a re-run after the base branch + advanced could still have its dispatch rejected by `opencode-review-dispatch.yml`'s live + `validate-pr-metadata` check, leaving the required check red with no review ever requested. The + verdict step now exposes `base_ref`/`base_sha`/`head_ref`/`head_sha` as step outputs from that same + live fetch, and the dispatch step builds its payload from those outputs instead. + +**Net result.** Stale workflow reruns can no longer grant a draft/closed exemption or dispatch stale +base/head metadata — both the pass/fail decision and the dispatch payload are sourced from one live +PR-state fetch per run, never from the frozen triggering-event payload. Full regression coverage in +`tests/test_opencode_required_verdict_regression.py` executes each step's actual production bash body +against fake `gh` fixtures. This closes one confirmed, now-eliminated source of required-workflow queue +waste (draft PRs); it does not by itself resolve the separate org-wide runner-capacity congestion +`#1531` also tracks. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. From 413a362c7ace1d4a662bdf8966172b56c1e432b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:14:31 +0000 Subject: [PATCH 09/10] docs(gap-baseline): correct overstated live-state sourcing claim (Devin review on #1443) --- docs/product-technical-gap-baseline.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index d33feccf88..26fcc30c0f 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2378,8 +2378,12 @@ failure with no actionable next step for the PR author. live fetch, and the dispatch step builds its payload from those outputs instead. **Net result.** Stale workflow reruns can no longer grant a draft/closed exemption or dispatch stale -base/head metadata — both the pass/fail decision and the dispatch payload are sourced from one live -PR-state fetch per run, never from the frozen triggering-event payload. Full regression coverage in +base/head metadata — the exemption decision and the dispatch payload are both sourced from one live +PR-state fetch per run, never from the frozen triggering-event payload. One event-derived value remains +deliberate, not a residual gap: the formal-review-match step still keys off the triggering event's own +`HEAD_SHA` (Devin review, same PR) — this run's own check result is itself attributed to whatever commit +GitHub associated with it at creation, so matching a review against any other ("live") SHA would search +the wrong commit for what this exact check result represents. Full regression coverage in `tests/test_opencode_required_verdict_regression.py` executes each step's actual production bash body against fake `gh` fixtures. This closes one confirmed, now-eliminated source of required-workflow queue waste (draft PRs); it does not by itself resolve the separate org-wide runner-capacity congestion From c3d18f956293de8bedf6fc678e436309a7a600dd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:20:46 +0000 Subject: [PATCH 10/10] fix(ci): match opencode-review formal reviews against the live head SHA Owner direction, agreeing with and extending a Devin review comment on #1443: the formal-review-matching jq query still keyed off the triggering event's own HEAD_SHA, the one event-derived value the prior three rounds hadn't touched. A stale rerun's event payload could therefore match an approval that was only ever valid for a predecessor head, or miss a real approval already posted against the actual live head. The verdict step now matches reviews against the live head.sha from the same gh api fetch it already uses for closed/draft/dispatch metadata. Adds regression coverage for both directions, corrects the CHANGELOG/gap-baseline prose accordingly. --- .github/workflows/opencode-review.yml | 19 +++++-- CHANGELOG.md | 10 ++++ docs/product-technical-gap-baseline.md | 30 ++++++----- ...st_opencode_required_verdict_regression.py | 53 ++++++++++++++++++- 4 files changed, 94 insertions(+), 18 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 56dba81ccf..4dcaff221a 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -282,6 +282,10 @@ jobs: fi pr_state="$(printf '%s' "$pr" | jq -r '.state // ""')" pr_draft="$(printf '%s' "$pr" | jq -r '.draft // false')" + pr_base_ref="$(printf '%s' "$pr" | jq -r '.base.ref')" + pr_base_sha="$(printf '%s' "$pr" | jq -r '.base.sha')" + pr_head_ref="$(printf '%s' "$pr" | jq -r '.head.ref')" + pr_head_sha="$(printf '%s' "$pr" | jq -r '.head.sha')" # Exposed for the dispatch step below so its repository_dispatch # payload also comes from this same live fetch, not from # github.event.pull_request.* -- a manual re-run of an old job @@ -290,10 +294,10 @@ jobs: # validate-pr-metadata check hard-rejects a stale base_ref/base_sha, # so the dispatch would fail even once this step correctly decides # one is needed (Devin review on #1443). - printf 'base_ref=%s\n' "$(printf '%s' "$pr" | jq -r '.base.ref')" >>"$GITHUB_OUTPUT" - printf 'base_sha=%s\n' "$(printf '%s' "$pr" | jq -r '.base.sha')" >>"$GITHUB_OUTPUT" - printf 'head_ref=%s\n' "$(printf '%s' "$pr" | jq -r '.head.ref')" >>"$GITHUB_OUTPUT" - printf 'head_sha=%s\n' "$(printf '%s' "$pr" | jq -r '.head.sha')" >>"$GITHUB_OUTPUT" + printf 'base_ref=%s\n' "$pr_base_ref" >>"$GITHUB_OUTPUT" + printf 'base_sha=%s\n' "$pr_base_sha" >>"$GITHUB_OUTPUT" + printf 'head_ref=%s\n' "$pr_head_ref" >>"$GITHUB_OUTPUT" + printf 'head_sha=%s\n' "$pr_head_sha" >>"$GITHUB_OUTPUT" if [ "$pr_state" = "closed" ]; then echo "PR closed; a current-head OpenCode verdict is not required." echo "verdict=CLOSED" >>"$GITHUB_OUTPUT" @@ -307,7 +311,12 @@ jobs: if ! reviews="$(timeout 25 gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")"; then reviews="[]" fi - verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$HEAD_SHA" ' + # Match against the live head SHA fetched above, not the event's + # own HEAD_SHA -- a stale rerun's event payload could otherwise + # match an approval that was only ever valid for a predecessor + # head, or miss a real approval already posted against the actual + # live head (owner direction, Devin review on #1443). + verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$pr_head_sha" ' (add // []) | [ .[] diff --git a/CHANGELOG.md b/CHANGELOG.md index 15553b1a2a..07b9cec22b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,16 @@ Semantic Versioning where the repository publishes a release. live `gh api repos/.../pulls/` response it already uses for closed/draft, and the dispatch step builds its payload from `steps.verdict.outputs.*` instead of the event payload. +- Fix a fourth gap in the same lineage (owner direction, agreeing with and + extending a Devin review comment on `#1443`): the formal-review-matching + jq query still keyed off the triggering event's own `HEAD_SHA`, the one + event-derived value the three fixes above hadn't yet touched. A stale + rerun's event payload could therefore match an approval that was only + ever valid for a predecessor head, or miss a real approval already posted + against the actual live head. The verdict step now matches reviews + against the live `head.sha` from the same `gh api` fetch it already uses + for closed/draft/dispatch metadata, so a stale rerun can neither accept a + predecessor-head approval nor miss a live-head one. - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 26fcc30c0f..c5ea975da1 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2355,7 +2355,7 @@ required-workflow OpenCode review dispatch work they could never complete or mer kind of wasted required-workflow queue allocation `#1531` tracks — while presenting a false-alarm failure with no actionable next step for the PR author. -**Fix, in three review-driven rounds, all on `.github#1443`:** +**Fix, in four review-driven rounds, all on `.github#1443`:** 1. The `Resolve current-head formal OpenCode verdict` step now exits early (`verdict=DRAFT`) for a draft PR, mirroring its existing `closed` early exit, and the `Request current-head OpenCode review execution` dispatch step's own `if:` matches — so a draft PR no longer dispatches OpenCode review @@ -2376,17 +2376,23 @@ failure with no actionable next step for the PR author. `validate-pr-metadata` check, leaving the required check red with no review ever requested. The verdict step now exposes `base_ref`/`base_sha`/`head_ref`/`head_sha` as step outputs from that same live fetch, and the dispatch step builds its payload from those outputs instead. - -**Net result.** Stale workflow reruns can no longer grant a draft/closed exemption or dispatch stale -base/head metadata — the exemption decision and the dispatch payload are both sourced from one live -PR-state fetch per run, never from the frozen triggering-event payload. One event-derived value remains -deliberate, not a residual gap: the formal-review-match step still keys off the triggering event's own -`HEAD_SHA` (Devin review, same PR) — this run's own check result is itself attributed to whatever commit -GitHub associated with it at creation, so matching a review against any other ("live") SHA would search -the wrong commit for what this exact check result represents. Full regression coverage in -`tests/test_opencode_required_verdict_regression.py` executes each step's actual production bash body -against fake `gh` fixtures. This closes one confirmed, now-eliminated source of required-workflow queue -waste (draft PRs); it does not by itself resolve the separate org-wide runner-capacity congestion +4. Devin review flagged, and the owner's direction extended, one remaining event-derived value: the + formal-review-matching jq query still keyed off the triggering event's own `HEAD_SHA`, the one value + the first three rounds hadn't yet touched. A stale rerun's event payload could therefore match an + approval that was only ever valid for a predecessor head, or miss a real approval already posted + against the actual live head. The verdict step now matches reviews against the same live `head.sha` + it already fetches for closed/draft/dispatch metadata, so a stale rerun can neither accept a + predecessor-head approval nor miss a live-head one. Regression coverage exercises both directions + with an event `head_sha` deliberately distinct from the live one. + +**Net result.** Stale workflow reruns can no longer grant a draft/closed exemption, dispatch stale +base/head metadata, or have their formal-review match accept a predecessor-head approval or miss a +live-head one — the exemption decision, the review match, and the dispatch payload are all sourced +from one live PR-state fetch per run, never from the frozen triggering-event payload. Full regression +coverage in `tests/test_opencode_required_verdict_regression.py` executes each step's actual production +bash body against fake `gh` fixtures. This closes one confirmed, now-eliminated source of +required-workflow queue waste (draft PRs); it does not by itself resolve the separate org-wide +runner-capacity congestion `#1531` also tracks. ## 5. 실행 루프와 고객의 다음 행동 diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 980b3173cf..eb33dcf41d 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -36,7 +36,7 @@ def runtime_verdict(reviews: list[dict[str, object]], head_sha: str = HEAD) -> s if jq is None: pytest.skip("jq is required to execute the production verdict filter") workflow = WORKFLOW.read_text(encoding="utf-8") - marker = """jq -r -s --arg sha "$HEAD_SHA" '""" + marker = """jq -r -s --arg sha "$pr_head_sha" '""" start = workflow.index(marker) + len(marker) end = workflow.index("\n ')", start) result = subprocess.run( @@ -522,6 +522,57 @@ def test_verdict_step_exposes_live_base_and_head_for_the_dispatch_payload( assert f"head_sha={live_head}" in output +def test_verdict_step_matches_reviews_against_the_live_head_not_the_event_head( + tmp_path: Path, +) -> None: + """Review matching must use the live head SHA, not the stale event one. + + Owner direction (agreeing with and extending Devin's review): a stale + rerun's event-derived ``HEAD_SHA`` must neither let the check accept an + approval that was only ever posted for a predecessor head, nor let it + miss a real approval already posted against the actual live head. Both + directions are exercised here with an event ``head_sha`` deliberately + different from the live one. + """ + event_head = HEAD + live_head = "e" * 40 + assert event_head != live_head + + # An approval posted only for the stale event-derived head must not + # satisfy the check once the live head has moved on. + stale_dir = tmp_path / "stale" + stale_dir.mkdir() + stale_only = _run_verdict_step( + stale_dir, + pr_number="1437", + head_sha=event_head, + gh_fixture="reviews", + pr_state="open", + pr_draft=False, + reviews=[review(state="APPROVED", commit_id=event_head)], + live_head_sha=live_head, + ) + assert stale_only.returncode == 0, stale_only.stderr + assert "verdict=APPROVED" not in (stale_dir / "github_output").read_text(encoding="utf-8") + + # An approval already posted for the actual live head must be found + # even though the (stale) event payload names a different head. + live_dir = tmp_path / "live" + live_dir.mkdir() + live_only = _run_verdict_step( + live_dir, + pr_number="1437", + head_sha=event_head, + gh_fixture="reviews", + pr_state="open", + pr_draft=False, + reviews=[review(state="APPROVED", commit_id=live_head)], + live_head_sha=live_head, + ) + assert live_only.returncode == 0, live_only.stderr + assert "verdict=APPROVED" in (live_dir / "github_output").read_text(encoding="utf-8") + + def test_dispatch_step_payload_sources_base_and_head_from_the_verdict_step() -> None: """The dispatch step must build its payload from live outputs, not the event.