diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 81faf57757..4dcaff221a 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: >- @@ -264,22 +264,59 @@ 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 }} 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')" + 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 + # 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' "$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" 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 + 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 ! 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 // []) | [ .[] @@ -313,16 +350,28 @@ jobs: fi - name: Request current-head OpenCode review execution - if: github.event.action != 'closed' && 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 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 @@ -361,6 +410,9 @@ jobs: if [ "$VERDICT" = "CLOSED" ]; then exit 0 fi + if [ "$VERDICT" = "DRAFT" ]; then + exit 0 + fi if [ -z "$VERDICT" ]; then echo "::error::No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head. This required check is not a review and must not succeed until the authenticated dispatch posts a current-head verdict." exit 1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 43020db98e..07b9cec22b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,81 @@ 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 + 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. The + `Resolve current-head formal OpenCode verdict` step now exits early with + `verdict=DRAFT` for a draft PR (mirroring its existing `closed` early + exit), the `Request current-head OpenCode review execution` dispatch + step's own `if:` also skips drafts (so a transient OIDC/dispatch failure + can't turn a draft PR's required check red before the exemption even + runs), and the trivial `Fail closed without a current-head OpenCode + verdict` step treats `VERDICT=DRAFT` the same as `VERDICT=CLOSED`. Also + adds `converted_to_draft` to the workflow's `pull_request_target` trigger + types, so a ready PR converted back to draft with no new commit still + gets a fresh required-workflow run that can apply the exemption (without + it, its previously failed check would show failure indefinitely). This + was re-derived from scratch against the event-driven + `opencode-review-target` design `#1507`/`#1532` landed on `main` (the + 325-minute synchronous poll loop this fix originally targeted no longer + exists); see `tests/test_opencode_required_verdict_regression.py` for + 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. +- 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." +- 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. +- 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. @@ -671,7 +746,8 @@ Semantic Versioning where the repository publishes a release. Informational, no change: the gap-baseline's repeated review-round narrative is this repo's own documented, intentional convention (ADR-0002: the baseline is "an operational snapshot," not a duplicate of - the ADR's design record), not accidental redundancy.- Raise `contextual_orchestrator_review_sidecar.sh`'s + the ADR's design record), not accidental redundancy. +- 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 blocking `noema-review`/`opencode-review`/`strix` org-wide to diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..c5ea975da1 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,57 @@ 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 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 + 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. +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. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index d08c2cdd9e..8c53799bad 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -512,7 +512,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 7fb4456f56..eb33dcf41d 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -5,6 +5,7 @@ import json import os import shutil +import stat import subprocess import textwrap from pathlib import Path @@ -16,6 +17,7 @@ WORKFLOW = Path(".github/workflows/opencode-review.yml") DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.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]: @@ -34,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( @@ -127,6 +129,546 @@ 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 test_request_review_execution_step_is_also_gated_on_draft() -> None: + """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() + 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: steps.verdict.outputs.verdict == ''" + + +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 _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]], + *, + 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. + + 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. + 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, + "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( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + 'test "$1" = api\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, + "base": {"ref": "main", "sha": "b" * 40}, + "head": {"ref": "feature", "sha": HEAD}, + } + ), + 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) + return fake_gh + + +def _run_verdict_step( + tmp_path: Path, + *, + 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, + 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. + + 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") + 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() + 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 [], + 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") + 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, + "GITHUB_OUTPUT": str(output_file), + } + return subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env=env, + ) + + +def _run_fail_closed_step(verdict: str) -> subprocess.CompletedProcess[str]: + """Execute the trivial "Fail closed without a current-head OpenCode + verdict" step body given a resolved ``$VERDICT``. + + Unlike the old design, this step no longer calls ``gh`` or loops at all + -- the Reviews API call moved entirely into the verdict-resolution step + above, so this one only ever inspects the ``VERDICT`` string it is + handed. + """ + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is required to execute the production step body") + workflow = WORKFLOW.read_text(encoding="utf-8") + script = _extract_run_block(workflow, STEP_NAME) + return subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env={**os.environ, "VERDICT": verdict}, + ) + + +def test_draft_pr_verdict_step_short_circuits_before_the_reviews_api_call( + tmp_path: Path, +) -> None: + """A draft PR's verdict step 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. 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, + 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") + + +def test_draft_pr_verdict_step_ignores_the_stale_triggering_event_action( + tmp_path: Path, +) -> None: + """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, + 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 + + +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_verdict_step( + tmp_path, + 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 + assert "Current-head OpenCode verdict: APPROVED." in result.stdout + assert "verdict=APPROVED" in (tmp_path / "github_output").read_text(encoding="utf-8") + + +def test_non_draft_pr_without_a_verdict_leaves_the_gate_empty(tmp_path: Path) -> None: + """A non-draft PR with no matching review resolves an empty verdict.""" + result = _run_verdict_step( + tmp_path, + pr_number="1437", + head_sha=HEAD, + gh_fixture="reviews", + pr_state="open", + pr_draft=False, + reviews=[], + ) + assert result.returncode == 0, result.stderr + assert "verdict=" in (tmp_path / "github_output").read_text(encoding="utf-8") + assert "verdict=APPROVED" not in (tmp_path / "github_output").read_text(encoding="utf-8") + assert "verdict=CHANGES_REQUESTED" not in (tmp_path / "github_output").read_text( + encoding="utf-8" + ) + 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_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. + + ``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( + tmp_path, + 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 + assert "PR is a draft" not in result.stdout + 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") + assert result.returncode == 0, result.stderr + + +def test_fail_closed_step_passes_on_closed_verdict() -> None: + """The trivial fail-closed step still passes VERDICT=CLOSED unchanged.""" + result = _run_fail_closed_step("CLOSED") + assert result.returncode == 0, result.stderr + + +def test_fail_closed_step_fails_without_a_verdict() -> None: + """The trivial fail-closed step still fails closed on an empty verdict.""" + result = _run_fail_closed_step("") + assert result.returncode == 1 + assert "No APPROVED or CHANGES_REQUESTED from opencode-agent" in result.stdout + + def test_formal_receipt_reruns_failed_required_job_without_runner_polling() -> None: """A formal receipt wakes the failed required run instead of polling for hours.""" required = WORKFLOW.read_text(encoding="utf-8") diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 5a295da25f..f740ea7eff 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -459,8 +459,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