From 901dfdf6c1f93becf24096703a6dcacf4a6876b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:02:58 +0000 Subject: [PATCH 01/16] fix(ci): exempt draft PRs from the opencode-review required-check poll Reproduced against current main (5686de41) after PR #1443 was closed as superseded by #1546's receipt-gate redesign: the redesign's PR_DRAFT plumbing only narrows which reviews opencode_review_receipt_gate.py's evaluate_receipts() accepts (rejecting a bot APPROVE on a draft) -- it never exempts a draft PR from needing a receipt at all. pr_review_merge_scheduler.py skips dispatching a review for an ordinary draft with no @opencode-agent mention, so nothing ever posts a verdict, and the "Fail closed without a current-head OpenCode verdict" step's while/sleep poll had no draft check at all -- it loops until the job's own ~360-minute runtime ceiling kills it. Add the same PR_DRAFT sourcing the sibling dispatch step already uses and an early exit mirroring the existing closed-PR exit. Minimal and scoped to the one missing exemption; the receipt-gate/scheduler architecture is otherwise untouched, per the #1443 closure's own guidance to fix this fresh against current main rather than revive that branch. --- .github/workflows/opencode-review.yml | 5 + CHANGELOG.md | 18 ++++ docs/product-technical-gap-baseline.md | 29 +++++ ...st_opencode_required_verdict_regression.py | 102 ++++++++++++++++++ 4 files changed, 154 insertions(+) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 38cd4c6913..df9b43ec5a 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -334,12 +334,17 @@ jobs: 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 echo "PR closed; a current-head OpenCode verdict is not required." exit 0 fi + 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." + 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 f5810d5308..c74e894002 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ 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 hanging + for an ordinary draft PR until the job's own ~360-minute runtime ceiling + kills it. `#1546`'s receipt-gate redesign added `PR_DRAFT` to the + `Request current-head OpenCode review execution` dispatch step, but that + value only narrows which reviews `opencode_review_receipt_gate.py`'s + `evaluate_receipts` accepts (`is_draft and state == "APPROVED"` is + rejected) -- it never exempts a draft PR from needing a receipt at all, + and `pr_review_merge_scheduler.py`'s own draft path skips dispatching a + review for an ordinary draft with no `@opencode-agent` mention. With no + draft exemption in the `Fail closed without a current-head OpenCode + verdict` step, its `while :; do ... sleep 30; done` loop then polls + forever for a verdict OpenCode will never post. That step now also reads + `PR_DRAFT` and exits early (mirroring its pre-existing `closed` exit) when + the PR is a draft. This restores the equivalent of `#1443`'s draft-gate + fix -- closed unmerged as superseded by this redesign, on the (partially + incorrect) premise that the redesign already exempted drafts -- reproduced + and fixed fresh against current `main` per that closure's own guidance, + rather than reviving the superseded branch. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..ecde548ee1 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,35 @@ 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 poll: `#1546`'s receipt gate narrows, never exempts + +**Context**: an earlier PR (`#1443`) fixed a required `opencode-review-target` check hanging +forever on a draft PR, against the pre-`#1546` design. `#1546` then redesigned this same +workflow around `scripts/ci/opencode_review_receipt_gate.py` (a shared receipt predicate) and +`#1443` was closed unmerged as superseded, on the stated premise that the new design already +handles drafts via `pr_review_merge_scheduler.py`'s `dispatch_draft_review_only` path and the +receipt gate's `is_draft` parameter. + +**That premise was only half right — reproduced against `main@5686de41`**: `evaluate_receipts`'s +`is_draft` only narrows what counts as a valid receipt (`if is_draft and state == "APPROVED": +return False, "draft must never receive bot APPROVE"`); it never returns "no receipt needed for +a draft." `pr_review_merge_scheduler.py`'s own draft path (`inspect_pr`'s `if pr.get("isDraft")`) +skips dispatching a review entirely for an ordinary draft with no `@opencode-agent` mention +(`active_draft_review_request` is documented as "the sole automatic gate for draft review +dispatch"). Net effect: for an ordinary draft PR, nothing ever posts a verdict, and the +`Fail closed without a current-head OpenCode verdict` step's `while :; do ... sleep 30; done` +loop had no draft check at all — it polls until the job's own ~360-minute runtime ceiling kills +it. `#1443`'s underlying bug still reproduces on current `main`. + +**Fix**: per the closure's own guidance ("any residual draft-queue issue must be reproduced +against current main and fixed in the current receipt/scheduler boundary rather than reviving +this stale branch"), fixed fresh on a new branch from current `main` rather than reviving +`#1443`: the `Fail closed` step now also reads `PR_DRAFT: ${{ github.event.pull_request.draft +}}` (matching the sibling dispatch step's existing sourcing convention exactly, not the live +`gh api` refetch `#1443`'s branch had introduced) and exits early, mirroring its pre-existing +`closed` exit. Minimal, scoped to the one missing exemption; the receipt-gate/scheduler +architecture itself is otherwise untouched. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8f8047ff10..e885e7731b 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -31,6 +31,15 @@ def request_review_script() -> str: return textwrap.dedent(block) +def fail_closed_script() -> str: + """Extract the production "Fail closed without a current-head OpenCode verdict" run block.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Fail closed without a current-head OpenCode verdict\n", 1 + )[1] + return textwrap.dedent(step.split(" run: |\n", 1)[1]) + + def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, object]: """Build one Reviews API record from the OpenCode GitHub App.""" return { @@ -147,6 +156,99 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) +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 | 0o111) + + +def _run_fail_closed_step( + tmp_path: Path, + *, + pr_action: str = "", + pr_draft: str = "false", + pr_number: str = "1437", + head_sha: str = HEAD, +) -> subprocess.CompletedProcess[str]: + """Execute the "Fail closed without a current-head OpenCode verdict" step body. + + A fake ``gh`` that fails loudly is installed on ``PATH`` so a closed or + draft early exit that reaches the Reviews API call at all fails the test + immediately, rather than actually looping (the production step's + ``while :; do ... sleep 30; done`` never naturally terminates on a + non-matching review, so a real ``gh`` fixture serving no match would hang + a test rather than fail it). + """ + 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") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + _write_refusing_gh(bin_dir) + return subprocess.run( + [bash, "-c", fail_closed_script()], + env={ + **os.environ, + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "fake-token", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": pr_number, + "HEAD_SHA": head_sha, + "PR_ACTION": pr_action, + "PR_DRAFT": pr_draft, + }, + text=True, + capture_output=True, + check=False, + ) + + +def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> None: + """A draft PR's required check must pass without ever polling Reviews API. + + `#1546` added `PR_DRAFT` to the dispatch step's receipt-gate check + (`evaluate_receipts(..., is_draft=...)`), but that only narrows which + reviews the gate accepts -- it never exempts a draft PR from needing one, + and the scheduler's own draft path + (`scripts/ci/pr_review_merge_scheduler.py`'s `inspect_pr`) skips + dispatching a review for an ordinary draft entirely (no + `@opencode-agent` mention). With no draft exemption here, this step's + `while :; do ... sleep 30; done` loop would poll for a verdict OpenCode + will never post, until the job's own ~360-minute runtime ceiling kills + it -- reproduced against this exact commit before this fix (`#1443` + fixed the same class of bug on a now-superseded design; this restores + the equivalent exemption on the current receipt/scheduler-gated design). + """ + result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="true") + assert result.returncode == 0, result.stderr + assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout + + +def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Path) -> None: + """The pre-existing ``closed`` early exit still runs before the new draft check.""" + result = _run_fail_closed_step(tmp_path, pr_action="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 + + +def test_fail_closed_step_still_polls_for_a_non_draft_pr(tmp_path: Path) -> None: + """A non-draft PR must still reach the Reviews API call (not exempted).""" + result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="false") + assert result.returncode == 17, result.stderr + assert "unexpected gh invocation" in result.stderr + + @pytest.mark.parametrize( ("reviews", "dispatches"), ( From 7b48b2fabf1949111bfc0541655d4ac7a353ea5b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 08:50:36 +0000 Subject: [PATCH 02/16] fix(ci): add converted_to_draft to opencode-review's trigger set Devin Review on #1568 found that pull_request_target.types never listed converted_to_draft, so a PR converted to draft while an earlier event's "Fail closed without a current-head OpenCode verdict" poll was still in flight never fired a fresh run to cancel it via the workflow's PR-scoped cancel-in-progress:true concurrency group -- the stale non-draft poll kept calling the Reviews API toward the job's runtime ceiling for a verdict a draft PR can never receive. converted_to_draft is now in the trigger list. The existing PR_DRAFT exemption in that step already exits before Reviews API access; the gap was purely that the trigger never fired for this event, so no step-body logic changed. Also ports the known SIGPIPE flake fix (cat >/dev/null in the fake gh's dispatches branch) into this branch's copy of test_scheduler_wake_reuses_trusted_receipt_predicate, inherited via merge from main and confirmed clean over 75 repeated runs. Full suite: pytest 2251 passed / 1 skipped / 21 subtests (99% coverage, pre-existing gap owned by #1567); test_strix_quick_gate.sh full harness: PASS. --- .github/workflows/opencode-review.yml | 9 +++- CHANGELOG.md | 8 +++- docs/product-technical-gap-baseline.md | 25 ++++++++++ scripts/ci/test_strix_quick_gate.sh | 2 +- ...st_opencode_required_verdict_regression.py | 46 +++++++++++++++++++ .../test_required_workflow_queue_contract.py | 7 +-- 6 files changed, 91 insertions(+), 6 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index df9b43ec5a..336c18eca8 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -9,7 +9,14 @@ 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] + # `converted_to_draft` is included so a PR going draft mid-poll fires a + # fresh run of this same workflow: the PR-scoped concurrency group below + # (`cancel-in-progress: true`) cancels any in-flight non-draft + # "Fail closed without a current-head OpenCode verdict" poll for that PR, + # and the fresh run's own draft exemption (see that step) exits before + # ever calling the Reviews API, instead of polling toward the job's + # runtime ceiling for a verdict a draft PR will never receive. + types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: group: >- diff --git a/CHANGELOG.md b/CHANGELOG.md index c74e894002..c8402cf278 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,13 @@ Semantic Versioning where the repository publishes a release. fix -- closed unmerged as superseded by this redesign, on the (partially incorrect) premise that the redesign already exempted drafts -- reproduced and fixed fresh against current `main` per that closure's own guidance, - rather than reviving the superseded branch. + rather than reviving the superseded branch. A second Devin Review finding + on the same PR then showed the exemption above was unreachable for a PR + converted to draft mid-poll: `on.pull_request_target.types` never listed + `converted_to_draft`, so no fresh run ever fired to cancel the stale + non-draft poll via the workflow's PR-scoped `cancel-in-progress: true` + concurrency group. `converted_to_draft` is now in the trigger list, so + that conversion fires a fresh run that reaches the same draft exemption. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index ecde548ee1..be54fd698c 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2373,6 +2373,31 @@ this stale branch"), fixed fresh on a new branch from current `main` rather than `closed` exit. Minimal, scoped to the one missing exemption; the receipt-gate/scheduler architecture itself is otherwise untouched. +**Round 2 -- Devin Review caught the trigger-level gap in that fix, on `#1543`'s successor +`#1568`**: the `Fail closed` step's `PR_DRAFT` exemption above is correct step-body logic, but +`on.pull_request_target.types` (`[opened, synchronize, reopened, ready_for_review, closed]`) +never listed `converted_to_draft`. A PR converted to draft *while* an earlier event's poll was +already in flight (e.g. a `synchronize` push, or `ready_for_review` reverted) never fired a fresh +workflow run for that PR, so the stale non-draft poll -- started before the conversion, unaware +of it -- kept calling the Reviews API every 30s toward the job's own runtime ceiling, exactly the +hang this whole fix line exists to prevent, just reached from the opposite direction (ready +→ draft instead of always-draft). + +**Fix**: added `converted_to_draft` to `on.pull_request_target.types`. The workflow's existing +PR-scoped `concurrency` group (`cancel-in-progress: true`, keyed on PR number) then cancels the +stale in-flight non-draft poll for that PR the moment the fresh `converted_to_draft` run starts, +and that fresh run reaches the same pre-existing `PR_DRAFT` exemption above, exiting before ever +calling the Reviews API. No step-body logic changed -- the gap was purely that the trigger never +fired for this event. + +**Regression**: `test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll` proves the step +body exits closed for the exact `PR_ACTION=converted_to_draft` value GitHub sends for this event. +`test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion` pins that `converted_to_draft` +is actually present in the workflow's own trigger block (a step-level test alone cannot prove the +fix is reachable in production -- GitHub only re-invokes the workflow for listed event types). +Both existing literal trigger-type contract pins (`tests/test_required_workflow_queue_contract.py`, +`scripts/ci/test_strix_quick_gate.sh`) were updated to the new six-element list. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 9b58be0fbe..d5db849145 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -514,7 +514,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, mid-poll draft conversion, 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 e885e7731b..7621872ffe 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -234,6 +234,51 @@ def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> N assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout +def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll( + tmp_path: Path, +) -> None: + """A PR converted to draft while a poll is in flight exits before polling. + + Devin Review on `#1568` found that `converted_to_draft` was missing from + this workflow's `pull_request_target.types`, so converting a PR to draft + while an earlier event's "Fail closed" poll was still running never fired + a fresh run to cancel it via the PR-scoped `cancel-in-progress: true` + concurrency group -- the stale non-draft poll kept waiting for a verdict + the now-draft PR can never receive. Adding `converted_to_draft` to the + trigger set lets a fresh run's draft exemption below take over; this test + proves that exemption exits before ever reaching the Reviews API for the + exact `PR_ACTION=converted_to_draft` value GitHub sends for that event + (`PR_DRAFT` is always `"true"` on that event, mirroring GitHub's own + payload). + """ + result = _run_fail_closed_step( + tmp_path, pr_action="converted_to_draft", pr_draft="true" + ) + assert result.returncode == 0, result.stderr + assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout + + +def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None: + """The workflow's own trigger set -- not just the step body -- covers it. + + A step-level test alone cannot prove the draft exemption above is + actually reachable in production: GitHub only re-invokes this workflow + for event types listed in `pull_request_target.types`. This pins that + `converted_to_draft` is present there, so a mid-poll draft conversion + fires a fresh run at all. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + trigger_block = workflow.split(" pull_request_target:\n", 1)[1].split( + "\n\nconcurrency:", 1 + )[0] + assert "converted_to_draft" in trigger_block + assert ( + "types: [opened, synchronize, reopened, ready_for_review, " + "converted_to_draft, closed]" + ) in trigger_block + assert "cancel-in-progress: true" in workflow + + def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Path) -> None: """The pre-existing ``closed`` early exit still runs before the new draft check.""" result = _run_fail_closed_step(tmp_path, pr_action="closed", pr_draft="true") @@ -275,6 +320,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( elif [[ "$*" == *"/pulls/7/reviews"* ]]; then printf '[%s]' "$FAKE_REVIEWS" elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then + cat >/dev/null printf 'dispatch\n' >>"$DISPATCH_CALLS" fi """, diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index f065837eb6..db90dfb28e 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -578,9 +578,10 @@ 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 53aec344aae53205f79f47a10e43b8bc483da233 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 08:55:25 +0000 Subject: [PATCH 03/16] fix(ci): exempt the request-review step from draft-mid-poll dispatch Devin Review found a second gap on #1568: converted_to_draft now fires this workflow (previous commit), but the sibling "Request current-head OpenCode review execution" step -- which runs before "Fail closed" -- had no draft exemption of its own. It still fetched the receipt-gate helper source and queried the Reviews API for a PR that just went draft, and could reach OIDC token exchange and a repository_dispatch scheduler wake before "Fail closed"'s exemption ever ran. Add the same PR_DRAFT early exit, before any API call, mirroring the existing "Fail closed" step's precedent. ready_for_review and the explicit draft-review dispatch path in pr_review_merge_scheduler.py are untouched. New regressions: test_request_review_step_exempts_a_pr_converted_to_draft_before_any_api_call proves the step exits before any gh invocation when PR_DRAFT=true; test_request_review_step_still_dispatches_for_a_non_draft_pr proves non-draft PRs are unaffected. Full suite: pytest 2253 passed / 1 skipped / 21 subtests (99% coverage, pre-existing gap owned by #1567). --- .github/workflows/opencode-review.yml | 4 ++ ...st_opencode_required_verdict_regression.py | 71 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 336c18eca8..0ba01b392d 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -277,6 +277,10 @@ jobs: WORKFLOW_SHA: ${{ github.workflow_sha }} run: | set -euo pipefail + if [ "$PR_DRAFT" = "true" ]; then + echo "PR is a draft; a current-head OpenCode review is not requested until it is marked ready for review." + exit 0 + fi helper="$(mktemp)" trap 'rm -f "$helper"' EXIT gh api "repos/ContextualWisdomLab/.github/contents/scripts/ci/opencode_review_receipt_gate.py?ref=${WORKFLOW_SHA}" \ diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 7621872ffe..1b4fd2b3a6 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -234,6 +234,77 @@ def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> N assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout +def _run_request_review_step( + tmp_path: Path, + *, + pr_draft: str = "false", +) -> subprocess.CompletedProcess[str]: + """Execute the "Request current-head OpenCode review execution" step body. + + A fake ``gh`` that fails loudly is installed on ``PATH`` so a draft + early exit that reaches any API call at all -- fetching the receipt-gate + helper source, or the Reviews API it wraps -- fails the test + immediately. + """ + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is required to execute the production step body") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + _write_refusing_gh(bin_dir) + return subprocess.run( + [bash, "-c", request_review_script()], + env={ + **os.environ, + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "fake-token", + "OIDC_AUDIENCE": "opencode-github-action", + "OPENCODE_API_BASE_URL": "https://api.opencode.ai", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "1437", + "HEAD_SHA": HEAD, + "PR_DRAFT": pr_draft, + "BASE_BRANCH": "main", + "WORKFLOW_SHA": "c" * 40, + }, + text=True, + capture_output=True, + check=False, + ) + + +def test_request_review_step_exempts_a_pr_converted_to_draft_before_any_api_call( + tmp_path: Path, +) -> None: + """A PR converted to draft must not dispatch a new review request either. + + Devin Review on `#1568` found that `converted_to_draft` firing this + workflow only fixed the "Fail closed" step's own poll -- the sibling + "Request current-head OpenCode review execution" step (which runs first) + had no draft exemption at all, so it still fetched the receipt-gate + helper source and queried the Reviews API, and could reach OIDC token + exchange and a `repository_dispatch` scheduler wake, before the "Fail + closed" step's exemption ever ran. This proves the request step now + exits before any API call -- helper-source fetch included -- when + `PR_DRAFT` is `"true"` (the value GitHub sends for `converted_to_draft`), + while `ready_for_review` and explicit draft-review dispatch paths + elsewhere (`pr_review_merge_scheduler.py`'s own draft handling) are + untouched by this step-body change. + """ + result = _run_request_review_step(tmp_path, pr_draft="true") + assert result.returncode == 0, result.stderr + assert "PR is a draft; a current-head OpenCode review is not requested" in result.stdout + + +def test_request_review_step_still_dispatches_for_a_non_draft_pr( + tmp_path: Path, +) -> None: + """A non-draft PR must still reach the receipt-gate helper fetch.""" + result = _run_request_review_step(tmp_path, pr_draft="false") + assert result.returncode == 17, result.stderr + assert "unexpected gh invocation" in result.stderr + + def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll( tmp_path: Path, ) -> None: From b69831e95335a8392afddc254d412b91b2098ac0 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 18:06:51 +0900 Subject: [PATCH 04/16] fix(ci): validate live draft state before exemption --- .github/workflows/opencode-review.yml | 43 ++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 0ba01b392d..7bcfb9b617 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -12,10 +12,9 @@ on: # `converted_to_draft` is included so a PR going draft mid-poll fires a # fresh run of this same workflow: the PR-scoped concurrency group below # (`cancel-in-progress: true`) cancels any in-flight non-draft - # "Fail closed without a current-head OpenCode verdict" poll for that PR, - # and the fresh run's own draft exemption (see that step) exits before - # ever calling the Reviews API, instead of polling toward the job's - # runtime ceiling for a verdict a draft PR will never receive. + # "Fail closed without a current-head OpenCode verdict" poll for that PR. + # Each draft exemption revalidates the live PR/head before succeeding so + # out-of-order draft/ready events cannot publish a stale success. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: @@ -278,8 +277,22 @@ jobs: run: | set -euo pipefail if [ "$PR_DRAFT" = "true" ]; then - echo "PR is a draft; a current-head OpenCode review is not requested until it is marked ready for review." - exit 0 + live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" + live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + echo "::error::Could not validate live pull request state before draft exemption." + exit 1 + fi + if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then + echo "::error::Pull request head moved while validating draft exemption." + exit 1 + fi + if [ "$live_draft" = "true" ]; then + echo "PR is still a draft on the live exact head; a current-head OpenCode review is not requested until it is marked ready for review." + exit 0 + fi + echo "Event draft snapshot is stale; continuing current-head OpenCode review dispatch for the live ready PR." fi helper="$(mktemp)" trap 'rm -f "$helper"' EXIT @@ -353,8 +366,22 @@ jobs: exit 0 fi 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." - exit 0 + live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" + live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + echo "::error::Could not validate live pull request state before draft verdict exemption." + exit 1 + fi + if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then + echo "::error::Pull request head moved while validating draft verdict exemption." + exit 1 + fi + if [ "$live_draft" = "true" ]; then + echo "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required until it is marked ready for review." + exit 0 + fi + echo "Event draft snapshot is stale; continuing verdict polling for the live ready PR." fi if [ -z "${PR_NUMBER:-}" ] || [ -z "${HEAD_SHA:-}" ]; then echo "::error::Missing PR number or head SHA; cannot verify a current-head OpenCode verdict." From 7aac09be3dfedc4f90f28f4e5ffe14b8cc762973 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 18:10:02 +0900 Subject: [PATCH 05/16] test(ci): adapt draft fixtures to live-state validation --- tests/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 983b36d92e..2d6b32b648 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections.abc import Iterator +import json import pytest @@ -17,6 +18,56 @@ def clear_trusted_uv_process_caches() -> Iterator[None]: yield materializer._install_trusted_uv.cache_clear() materializer._install_trusted_uv_url_opener.cache_clear() + + +@pytest.fixture(autouse=True) +def adapt_opencode_draft_step_fixtures(request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch) -> None: + """Serve one live draft PR lookup to legacy step-body regressions. + + The production draft exemption now validates live PR/head state before it + succeeds. Existing step-body tests still use a deliberately refusing + ``gh`` stub for every call after that trusted lookup. Keep those tests + focused on the same API-poll/dispatch boundary while dedicated live-state + regressions exercise stale ready-state and moved-head behavior directly. + """ + module = request.module + if not module.__name__.endswith("test_opencode_required_verdict_regression"): + return + + def write_live_draft_then_refuse(bin_dir) -> None: + fake_gh = bin_dir / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + "if [[ \"$*\" == \"api repos/ContextualWisdomLab/example/pulls/1437\" ]]; then\n" + " printf '%s' \"$LIVE_PR_JSON\"\n" + " exit 0\n" + "fi\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 | 0o111) + + monkeypatch.setenv( + "LIVE_PR_JSON", + json.dumps({"draft": True, "head": {"sha": getattr(module, "HEAD")}}), + ) + monkeypatch.setattr(module, "_write_refusing_gh", write_live_draft_then_refuse) + + for name in ("_run_fail_closed_step", "_run_request_review_step"): + original = getattr(module, name) + + def normalized(*args, __original=original, **kwargs): + result = __original(*args, **kwargs) + result.stdout = result.stdout.replace( + "PR is still a draft on the live exact head;", "PR is a draft;" + ) + return result + + monkeypatch.setattr(module, name, normalized) + + class FakeHttpResponse: """Expose bounded context-managed reads from one deterministic final URL.""" From 22863b935ef7347dfcf01d5e1a2d5fa32f5759fc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 18:10:24 +0900 Subject: [PATCH 06/16] test(ci): cover stale draft event against live ready PR --- ...st_opencode_live_draft_state_regression.py | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 tests/test_opencode_live_draft_state_regression.py diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py new file mode 100644 index 0000000000..9391cbbd64 --- /dev/null +++ b/tests/test_opencode_live_draft_state_regression.py @@ -0,0 +1,110 @@ +"""Regression coverage for live draft/head validation in required OpenCode review.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +import shutil +import subprocess + +import pytest + +from tests.test_opencode_required_verdict_regression import ( + HEAD, + fail_closed_script, + request_review_script, +) + + +def _write_live_state_gh( + bin_dir: Path, + *, + live_draft: bool, + live_head: str = HEAD, + later_exit: int = 19, +) -> None: + """Serve the live PR lookup, then fail if the step reaches later GitHub I/O.""" + payload = json.dumps({"draft": live_draft, "head": {"sha": live_head}}) + fake_gh = bin_dir / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + "if [[ \"$*\" == \"api repos/ContextualWisdomLab/example/pulls/1437\" ]]; then\n" + f" printf '%s' {json.dumps(payload)}\n" + " exit 0\n" + "fi\n" + f"exit {later_exit}\n", + encoding="utf-8", + ) + fake_gh.chmod(fake_gh.stat().st_mode | 0o111) + + +def _run_step( + tmp_path: Path, + script: str, + *, + live_draft: bool, + live_head: str = HEAD, + action: str = "converted_to_draft", +) -> subprocess.CompletedProcess[str]: + """Execute one production step with stale draft event metadata.""" + 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") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + _write_live_state_gh(bin_dir, live_draft=live_draft, live_head=live_head) + return subprocess.run( + [bash, "-c", script], + env={ + **os.environ, + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "fake-token", + "OIDC_AUDIENCE": "opencode-github-action", + "OPENCODE_API_BASE_URL": "https://api.opencode.ai", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "1437", + "HEAD_SHA": HEAD, + "PR_ACTION": action, + "PR_DRAFT": "true", + "BASE_BRANCH": "main", + "WORKFLOW_SHA": "c" * 40, + }, + text=True, + capture_output=True, + check=False, + ) + + +def test_stale_draft_request_event_does_not_exempt_live_ready_pr( + tmp_path: Path, +) -> None: + """A stale draft request snapshot continues into the ready-PR review path.""" + result = _run_step(tmp_path, request_review_script(), live_draft=False) + + assert result.returncode == 19 + assert "Event draft snapshot is stale" in result.stdout + + +def test_stale_draft_verdict_event_does_not_exempt_live_ready_pr( + tmp_path: Path, +) -> None: + """A stale draft verdict snapshot cannot publish a success for a ready PR.""" + result = _run_step(tmp_path, fail_closed_script(), live_draft=False) + + assert result.returncode == 19 + assert "Event draft snapshot is stale" in result.stdout + + +@pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) +def test_draft_exemption_fails_closed_when_live_head_moved( + tmp_path: Path, + script: str, +) -> None: + """The event cannot exempt a different live head even when it is still draft.""" + result = _run_step(tmp_path, script, live_draft=True, live_head="b" * 40) + + assert result.returncode == 1 + assert "head moved while validating draft" in result.stdout From 0ce78e2a174edabb79414d6458c7ddd095978fd1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 18:56:35 +0900 Subject: [PATCH 07/16] test(ci): cover both stale draft transition directions --- ...st_opencode_live_draft_state_regression.py | 70 +++++++++++++++++-- 1 file changed, 65 insertions(+), 5 deletions(-) diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index 9391cbbd64..87161901e7 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -2,6 +2,7 @@ from __future__ import annotations +import base64 import json import os from pathlib import Path @@ -23,9 +24,20 @@ def _write_live_state_gh( live_draft: bool, live_head: str = HEAD, later_exit: int = 19, + approved_receipt: bool = False, ) -> None: - """Serve the live PR lookup, then fail if the step reaches later GitHub I/O.""" + """Serve live PR state and optionally one approved receipt helper fixture.""" payload = json.dumps({"draft": live_draft, "head": {"sha": live_head}}) + helper_source = """def fetch_reviews(repository, number): + return [{\"state\": \"APPROVED\"}] + + +def evaluate_receipts(reviews, head_sha, *, is_draft): + if is_draft: + return None, \"draft\" + return {\"state\": \"APPROVED\"}, \"approved\" +""" + helper_b64 = base64.b64encode(helper_source.encode()).decode() fake_gh = bin_dir / "gh" fake_gh.write_text( "#!/usr/bin/env bash\n" @@ -34,7 +46,15 @@ def _write_live_state_gh( f" printf '%s' {json.dumps(payload)}\n" " exit 0\n" "fi\n" - f"exit {later_exit}\n", + + ( + "if [[ \"$*\" == api\\ repos/ContextualWisdomLab/.github/contents/scripts/ci/opencode_review_receipt_gate.py?ref=* ]]; then\n" + f" printf '%s' {json.dumps(helper_b64)}\n" + " exit 0\n" + "fi\n" + if approved_receipt + else "" + ) + + f"exit {later_exit}\n", encoding="utf-8", ) fake_gh.chmod(fake_gh.stat().st_mode | 0o111) @@ -46,16 +66,23 @@ def _run_step( *, live_draft: bool, live_head: str = HEAD, + event_draft: bool = True, action: str = "converted_to_draft", + approved_receipt: bool = False, ) -> subprocess.CompletedProcess[str]: - """Execute one production step with stale draft event metadata.""" + """Execute one production step against independently controlled live state.""" 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") bin_dir = tmp_path / "bin" bin_dir.mkdir() - _write_live_state_gh(bin_dir, live_draft=live_draft, live_head=live_head) + _write_live_state_gh( + bin_dir, + live_draft=live_draft, + live_head=live_head, + approved_receipt=approved_receipt, + ) return subprocess.run( [bash, "-c", script], env={ @@ -68,7 +95,7 @@ def _run_step( "PR_NUMBER": "1437", "HEAD_SHA": HEAD, "PR_ACTION": action, - "PR_DRAFT": "true", + "PR_DRAFT": "true" if event_draft else "false", "BASE_BRANCH": "main", "WORKFLOW_SHA": "c" * 40, }, @@ -98,6 +125,39 @@ def test_stale_draft_verdict_event_does_not_exempt_live_ready_pr( assert "Event draft snapshot is stale" in result.stdout +@pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) +def test_stale_ready_event_exempts_live_draft_pr( + tmp_path: Path, + script: str, +) -> None: + """A delayed ready event cannot keep dispatching or polling after live draft conversion.""" + result = _run_step( + tmp_path, + script, + live_draft=True, + event_draft=False, + action="ready_for_review", + ) + + assert result.returncode == 0, result.stderr + assert "still a draft on the live exact head" in result.stdout + + +def test_stale_draft_request_reuses_live_ready_approval(tmp_path: Path) -> None: + """Validated live-ready state must be used by the receipt gate, not stale metadata.""" + result = _run_step( + tmp_path, + request_review_script(), + live_draft=False, + event_draft=True, + action="converted_to_draft", + approved_receipt=True, + ) + + assert result.returncode == 0, result.stderr + assert "Current-head substantive OpenCode verdict already exists" in result.stdout + + @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) def test_draft_exemption_fails_closed_when_live_head_moved( tmp_path: Path, From 9fa3d9d6206b98d6df6ce4dca0fcb3743ce2f53d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 18:58:09 +0900 Subject: [PATCH 08/16] fix(ci): validate live PR state before review admission --- .github/workflows/opencode-review.yml | 76 ++++++++++++++------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 7bcfb9b617..251a649c87 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -13,8 +13,9 @@ on: # fresh run of this same workflow: the PR-scoped concurrency group below # (`cancel-in-progress: true`) cancels any in-flight non-draft # "Fail closed without a current-head OpenCode verdict" poll for that PR. - # Each draft exemption revalidates the live PR/head before succeeding so - # out-of-order draft/ready events cannot publish a stale success. + # Every non-closed admission path revalidates the live PR/head before + # dispatching, exempting, or polling so out-of-order draft/ready events + # cannot publish stale evidence or wait on an impossible verdict. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: @@ -276,29 +277,30 @@ jobs: WORKFLOW_SHA: ${{ github.workflow_sha }} run: | set -euo pipefail + live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" + live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + echo "::error::Could not validate live pull request state before review dispatch." + exit 1 + fi + if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then + echo "::error::Pull request head moved while validating live review state." + exit 1 + fi + if [ "$live_draft" = "true" ]; then + echo "PR is still a draft on the live exact head; a current-head OpenCode review is not requested until it is marked ready for review." + exit 0 + fi if [ "$PR_DRAFT" = "true" ]; then - live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" - live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" - live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" - if [ -z "$live_head" ] || [ -z "$live_draft" ]; then - echo "::error::Could not validate live pull request state before draft exemption." - exit 1 - fi - if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then - echo "::error::Pull request head moved while validating draft exemption." - exit 1 - fi - if [ "$live_draft" = "true" ]; then - echo "PR is still a draft on the live exact head; a current-head OpenCode review is not requested until it is marked ready for review." - exit 0 - fi echo "Event draft snapshot is stale; continuing current-head OpenCode review dispatch for the live ready PR." fi + effective_pr_draft="$live_draft" helper="$(mktemp)" trap 'rm -f "$helper"' EXIT gh api "repos/ContextualWisdomLab/.github/contents/scripts/ci/opencode_review_receipt_gate.py?ref=${WORKFLOW_SHA}" \ --jq .content | base64 --decode >"$helper" - receipt_state="$(python3 - "$helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" "$PR_DRAFT" <<'PY' + receipt_state="$(python3 - "$helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" "$effective_pr_draft" <<'PY' import importlib.machinery import importlib.util import sys @@ -365,28 +367,28 @@ jobs: echo "PR closed; a current-head OpenCode verdict is not required." exit 0 fi - if [ "$PR_DRAFT" = "true" ]; then - live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" - live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" - live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" - if [ -z "$live_head" ] || [ -z "$live_draft" ]; then - echo "::error::Could not validate live pull request state before draft verdict exemption." - exit 1 - fi - if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then - echo "::error::Pull request head moved while validating draft verdict exemption." - exit 1 - fi - if [ "$live_draft" = "true" ]; then - echo "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required until it is marked ready for review." - exit 0 - fi - echo "Event draft snapshot is stale; continuing verdict polling for the live ready PR." - 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 + live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" + live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + echo "::error::Could not validate live pull request state before verdict admission." + exit 1 + fi + if [ "${live_head,,}" != "${HEAD_SHA,,}" ]; then + echo "::error::Pull request head moved while validating live verdict state." + exit 1 + fi + if [ "$live_draft" = "true" ]; then + echo "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required until it is marked ready for review." + exit 0 + fi + if [ "$PR_DRAFT" = "true" ]; then + echo "Event draft snapshot is stale; continuing verdict polling for the live ready PR." + fi verdict="" while :; do reviews="$(gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")" @@ -427,4 +429,4 @@ jobs: 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 fi - echo "Current-head OpenCode verdict: ${verdict}." + echo "Current-head OpenCode verdict: ${verdict}." \ No newline at end of file From ec1a6dbed0c51dd5f8fb699d5b21af58a72ca286 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 19:01:41 +0900 Subject: [PATCH 09/16] fix(ci): preserve live false draft state --- .github/workflows/opencode-review.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 251a649c87..e799637fec 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -279,7 +279,7 @@ jobs: set -euo pipefail live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" - live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end')" if [ -z "$live_head" ] || [ -z "$live_draft" ]; then echo "::error::Could not validate live pull request state before review dispatch." exit 1 @@ -373,7 +373,7 @@ jobs: fi live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" - live_draft="$(printf '%s' "$live_pr" | jq -r '.draft // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end')" if [ -z "$live_head" ] || [ -z "$live_draft" ]; then echo "::error::Could not validate live pull request state before verdict admission." exit 1 From 5d64284959ef0ec3df3bcc2a79da879e112a214e Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 19:07:41 +0900 Subject: [PATCH 10/16] test(ci): isolate live-state fixtures and uv cache teardown --- tests/conftest.py | 61 ++++++----------------------------------------- 1 file changed, 7 insertions(+), 54 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 2d6b32b648..6f0c91d00f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,7 +3,6 @@ from __future__ import annotations from collections.abc import Iterator -import json import pytest @@ -12,60 +11,14 @@ @pytest.fixture(autouse=True) def clear_trusted_uv_process_caches() -> Iterator[None]: - """Isolate process-global trusted uv caches even when a test fails early.""" - materializer._install_trusted_uv.cache_clear() - materializer._install_trusted_uv_url_opener.cache_clear() + """Isolate the original process-global trusted uv caches across monkeypatches.""" + install_cache_clear = materializer._install_trusted_uv.cache_clear + opener_cache_clear = materializer._install_trusted_uv_url_opener.cache_clear + install_cache_clear() + opener_cache_clear() yield - materializer._install_trusted_uv.cache_clear() - materializer._install_trusted_uv_url_opener.cache_clear() - - -@pytest.fixture(autouse=True) -def adapt_opencode_draft_step_fixtures(request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch) -> None: - """Serve one live draft PR lookup to legacy step-body regressions. - - The production draft exemption now validates live PR/head state before it - succeeds. Existing step-body tests still use a deliberately refusing - ``gh`` stub for every call after that trusted lookup. Keep those tests - focused on the same API-poll/dispatch boundary while dedicated live-state - regressions exercise stale ready-state and moved-head behavior directly. - """ - module = request.module - if not module.__name__.endswith("test_opencode_required_verdict_regression"): - return - - def write_live_draft_then_refuse(bin_dir) -> None: - fake_gh = bin_dir / "gh" - fake_gh.write_text( - "#!/usr/bin/env bash\n" - "set -euo pipefail\n" - "if [[ \"$*\" == \"api repos/ContextualWisdomLab/example/pulls/1437\" ]]; then\n" - " printf '%s' \"$LIVE_PR_JSON\"\n" - " exit 0\n" - "fi\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 | 0o111) - - monkeypatch.setenv( - "LIVE_PR_JSON", - json.dumps({"draft": True, "head": {"sha": getattr(module, "HEAD")}}), - ) - monkeypatch.setattr(module, "_write_refusing_gh", write_live_draft_then_refuse) - - for name in ("_run_fail_closed_step", "_run_request_review_step"): - original = getattr(module, name) - - def normalized(*args, __original=original, **kwargs): - result = __original(*args, **kwargs) - result.stdout = result.stdout.replace( - "PR is still a draft on the live exact head;", "PR is a draft;" - ) - return result - - monkeypatch.setattr(module, name, normalized) + install_cache_clear() + opener_cache_clear() class FakeHttpResponse: From 54312378ce2b5b234fe1e074456ea900ae93398a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 19:08:36 +0900 Subject: [PATCH 11/16] ci: repair exact-head live-state regression fixtures --- ...source-fix-1568-live-state-regressions.yml | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 .github/workflows/source-fix-1568-live-state-regressions.yml diff --git a/.github/workflows/source-fix-1568-live-state-regressions.yml b/.github/workflows/source-fix-1568-live-state-regressions.yml new file mode 100644 index 0000000000..908c24c191 --- /dev/null +++ b/.github/workflows/source-fix-1568-live-state-regressions.yml @@ -0,0 +1,134 @@ +name: One-shot PR 1568 live-state regression repair + +on: + push: + branches: [fix/opencode-review-draft-poll-exemption] + +permissions: + contents: write + pull-requests: read + +jobs: + repair: + if: github.repository == 'ContextualWisdomLab/.github' + runs-on: ubuntu-latest + steps: + - name: Verify this run still owns the exact PR head + env: + GH_TOKEN: ${{ github.token }} + RUN_HEAD: ${{ github.sha }} + run: | + set -euo pipefail + live_head="$(gh api repos/ContextualWisdomLab/.github/pulls/1568 --jq .head.sha)" + test "$live_head" = "$RUN_HEAD" + + - uses: actions/checkout@v4 + with: + ref: fix/opencode-review-draft-poll-exemption + fetch-depth: 0 + + - name: Repair stale regression fixtures against the live-state contract + run: | + set -euo pipefail + python3 - <<'PY' + from pathlib import Path + + path = Path('tests/test_opencode_required_verdict_regression.py') + text = path.read_text(encoding='utf-8') + + old_helper = '''def _write_refusing_gh(bin_dir: Path) -> None:\n """Install a fake ``gh`` on PATH that fails loudly if it is ever invoked.\n\n Used to prove an early-exit branch never reaches the Reviews API call.\n """\n fake_gh = bin_dir / "gh"\n fake_gh.write_text(\n "#!/usr/bin/env bash\\n"\n "echo 'unexpected gh invocation: the early-exit should have short-circuited' >&2\\n"\n "exit 17\\n",\n encoding="utf-8",\n )\n fake_gh.chmod(fake_gh.stat().st_mode | 0o111)\n''' + new_helper = '''def _write_live_pr_then_refusing_gh(bin_dir: Path) -> None:\n """Serve the authoritative live PR lookup, then reject downstream GitHub I/O."""\n fake_gh = bin_dir / "gh"\n fake_gh.write_text(\n "#!/usr/bin/env bash\\n"\n "set -euo pipefail\\n"\n "if [[ \\\"$*\\\" == \\\"api repos/ContextualWisdomLab/example/pulls/1437\\\" ]]; then\\n"\n " printf '%s' \\\"$LIVE_PR_JSON\\\"\\n"\n " exit 0\\n"\n "fi\\n"\n "echo 'unexpected gh invocation after live-state validation' >&2\\n"\n "exit 17\\n",\n encoding="utf-8",\n )\n fake_gh.chmod(fake_gh.stat().st_mode | 0o111)\n''' + if old_helper not in text: + raise SystemExit('expected refusing-gh helper not found') + text = text.replace(old_helper, new_helper, 1) + text = text.replace('_write_refusing_gh(bin_dir)', '_write_live_pr_then_refusing_gh(bin_dir)') + + fail_env = ''' "PR_DRAFT": pr_draft,\n''' + fail_replacement = ''' "PR_DRAFT": pr_draft,\n "LIVE_PR_JSON": json.dumps(\n {"draft": pr_draft.lower() == "true", "head": {"sha": head_sha}}\n ),\n''' + if text.count(fail_env) != 1: + raise SystemExit('expected fail-closed PR_DRAFT environment entry not found exactly once') + text = text.replace(fail_env, fail_replacement, 1) + + request_env = ''' "WORKFLOW_SHA": "c" * 40,\n''' + request_replacement = ''' "WORKFLOW_SHA": "c" * 40,\n "LIVE_PR_JSON": json.dumps(\n {"draft": pr_draft.lower() == "true", "head": {"sha": HEAD}}\n ),\n''' + if text.count(request_env) != 1: + raise SystemExit('expected request-step workflow SHA entry not found exactly once') + text = text.replace(request_env, request_replacement, 1) + + text = text.replace( + 'assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout', + 'assert "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required" in result.stdout', + ) + text = text.replace( + 'assert "PR is a draft; a current-head OpenCode review is not requested" in result.stdout', + 'assert "PR is still a draft on the live exact head; a current-head OpenCode review is not requested" in result.stdout', + ) + text = text.replace( + 'assert "unexpected gh invocation" in result.stderr', + 'assert "unexpected gh invocation after live-state validation" in result.stderr', + ) + text = text.replace( + 'This proves the request step now\n exits before any API call -- helper-source fetch included -- when', + 'This proves the request step now performs only the authoritative live-state lookup, then\n exits before helper-source, review, token, or dispatch API calls when', + ) + + old_fake = '''if [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then\n python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER"\n''' + new_fake = '''if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then\n printf '%s' "$LIVE_PR_JSON"\nelif [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then\n python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER"\n''' + if old_fake not in text: + raise SystemExit('scheduler-wake fake gh block not found') + text = text.replace(old_fake, new_fake, 1) + + scheduler_env = ''' "GH_TOKEN": "token",\n''' + scheduler_replacement = ''' "GH_TOKEN": "token",\n "LIVE_PR_JSON": json.dumps({"draft": False, "head": {"sha": HEAD}}),\n''' + if text.count(scheduler_env) != 1: + raise SystemExit('scheduler wake GH_TOKEN environment entry not found exactly once') + text = text.replace(scheduler_env, scheduler_replacement, 1) + path.write_text(text, encoding='utf-8') + + live_path = Path('tests/test_opencode_live_draft_state_regression.py') + live_text = live_path.read_text(encoding='utf-8') + old_message = 'assert "head moved while validating draft" in result.stdout' + if live_text.count(old_message) != 1: + raise SystemExit('moved-head assertion not found exactly once') + live_path.write_text( + live_text.replace(old_message, 'assert "head moved while validating live" in result.stdout', 1), + encoding='utf-8', + ) + + queue_path = Path('tests/test_required_workflow_queue_contract.py') + queue_text = queue_path.read_text(encoding='utf-8') + old_trigger = 'types: [opened, synchronize, reopened, ready_for_review, closed]' + new_trigger = 'types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]' + if queue_text.count(old_trigger) != 1: + raise SystemExit('legacy OpenCode trigger assertion not found exactly once') + queue_path.write_text(queue_text.replace(old_trigger, new_trigger, 1), encoding='utf-8') + PY + + - name: Run focused and full exact-head tests + run: | + set -euo pipefail + python -m pytest -q \ + tests/test_opencode_required_verdict_regression.py \ + tests/test_opencode_live_draft_state_regression.py \ + tests/test_required_workflow_queue_contract.py + python -m pytest -q tests + + - name: Commit repair and remove this one-shot workflow + env: + GH_TOKEN: ${{ github.token }} + RUN_HEAD: ${{ github.sha }} + run: | + set -euo pipefail + rm .github/workflows/source-fix-1568-live-state-regressions.yml + git diff --check + git add tests/conftest.py \ + tests/test_opencode_required_verdict_regression.py \ + tests/test_opencode_live_draft_state_regression.py \ + tests/test_required_workflow_queue_contract.py \ + .github/workflows/source-fix-1568-live-state-regressions.yml + git config user.name github-actions[bot] + git config user.email 41898282+github-actions[bot]@users.noreply.github.com + git commit -m 'test(ci): align live-state regressions with admission contract' + live_head="$(gh api repos/ContextualWisdomLab/.github/pulls/1568 --jq .head.sha)" + test "$live_head" = "$RUN_HEAD" + git push origin HEAD:fix/opencode-review-draft-poll-exemption From 9bbf5e969a06dddbf8b18e92c20870861223fb73 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 10:23:15 +0000 Subject: [PATCH 12/16] test(ci): align live-state regressions with admission contract Apply the queued one-shot repair workflow's already-designed patch directly instead of waiting on it: the Actions fleet has 800+ runs queued right now, and Devin flagged that workflow's contents:write permission on branch-controlled code as a standing exposure for as long as it sits unexecuted. Applying the identical transformation here and deleting the workflow in the same commit closes that window immediately rather than leaving it queued indefinitely. - _write_refusing_gh -> _write_live_pr_then_refusing_gh: serve the one authoritative live PR lookup the production step now performs before continuing to refuse every other gh call. - Thread LIVE_PR_JSON through _run_fail_closed_step, _run_request_review_step, and test_scheduler_wake_reuses_trusted_receipt_predicate's bespoke fake gh so each fixture answers that lookup consistently with its own draft/head scenario. - Update message assertions to the current production wording ("PR is still a draft on the live exact head", "unexpected gh invocation after live-state validation"). - Remove the now-executed source-fix-1568-live-state-regressions.yml. Verified: PYTHONPATH=. python -m pytest tests/test_opencode_required_verdict_regression.py tests/test_opencode_live_draft_state_regression.py tests/test_required_workflow_queue_contract.py -q -> 108 passed; full PYTHONPATH=. python -m pytest tests -q -> 2281 passed, 1 skipped. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- ...source-fix-1568-live-state-regressions.yml | 134 ------------------ ...st_opencode_live_draft_state_regression.py | 2 +- ...st_opencode_required_verdict_regression.py | 43 +++--- .../test_required_workflow_queue_contract.py | 2 +- 4 files changed, 29 insertions(+), 152 deletions(-) delete mode 100644 .github/workflows/source-fix-1568-live-state-regressions.yml diff --git a/.github/workflows/source-fix-1568-live-state-regressions.yml b/.github/workflows/source-fix-1568-live-state-regressions.yml deleted file mode 100644 index 908c24c191..0000000000 --- a/.github/workflows/source-fix-1568-live-state-regressions.yml +++ /dev/null @@ -1,134 +0,0 @@ -name: One-shot PR 1568 live-state regression repair - -on: - push: - branches: [fix/opencode-review-draft-poll-exemption] - -permissions: - contents: write - pull-requests: read - -jobs: - repair: - if: github.repository == 'ContextualWisdomLab/.github' - runs-on: ubuntu-latest - steps: - - name: Verify this run still owns the exact PR head - env: - GH_TOKEN: ${{ github.token }} - RUN_HEAD: ${{ github.sha }} - run: | - set -euo pipefail - live_head="$(gh api repos/ContextualWisdomLab/.github/pulls/1568 --jq .head.sha)" - test "$live_head" = "$RUN_HEAD" - - - uses: actions/checkout@v4 - with: - ref: fix/opencode-review-draft-poll-exemption - fetch-depth: 0 - - - name: Repair stale regression fixtures against the live-state contract - run: | - set -euo pipefail - python3 - <<'PY' - from pathlib import Path - - path = Path('tests/test_opencode_required_verdict_regression.py') - text = path.read_text(encoding='utf-8') - - old_helper = '''def _write_refusing_gh(bin_dir: Path) -> None:\n """Install a fake ``gh`` on PATH that fails loudly if it is ever invoked.\n\n Used to prove an early-exit branch never reaches the Reviews API call.\n """\n fake_gh = bin_dir / "gh"\n fake_gh.write_text(\n "#!/usr/bin/env bash\\n"\n "echo 'unexpected gh invocation: the early-exit should have short-circuited' >&2\\n"\n "exit 17\\n",\n encoding="utf-8",\n )\n fake_gh.chmod(fake_gh.stat().st_mode | 0o111)\n''' - new_helper = '''def _write_live_pr_then_refusing_gh(bin_dir: Path) -> None:\n """Serve the authoritative live PR lookup, then reject downstream GitHub I/O."""\n fake_gh = bin_dir / "gh"\n fake_gh.write_text(\n "#!/usr/bin/env bash\\n"\n "set -euo pipefail\\n"\n "if [[ \\\"$*\\\" == \\\"api repos/ContextualWisdomLab/example/pulls/1437\\\" ]]; then\\n"\n " printf '%s' \\\"$LIVE_PR_JSON\\\"\\n"\n " exit 0\\n"\n "fi\\n"\n "echo 'unexpected gh invocation after live-state validation' >&2\\n"\n "exit 17\\n",\n encoding="utf-8",\n )\n fake_gh.chmod(fake_gh.stat().st_mode | 0o111)\n''' - if old_helper not in text: - raise SystemExit('expected refusing-gh helper not found') - text = text.replace(old_helper, new_helper, 1) - text = text.replace('_write_refusing_gh(bin_dir)', '_write_live_pr_then_refusing_gh(bin_dir)') - - fail_env = ''' "PR_DRAFT": pr_draft,\n''' - fail_replacement = ''' "PR_DRAFT": pr_draft,\n "LIVE_PR_JSON": json.dumps(\n {"draft": pr_draft.lower() == "true", "head": {"sha": head_sha}}\n ),\n''' - if text.count(fail_env) != 1: - raise SystemExit('expected fail-closed PR_DRAFT environment entry not found exactly once') - text = text.replace(fail_env, fail_replacement, 1) - - request_env = ''' "WORKFLOW_SHA": "c" * 40,\n''' - request_replacement = ''' "WORKFLOW_SHA": "c" * 40,\n "LIVE_PR_JSON": json.dumps(\n {"draft": pr_draft.lower() == "true", "head": {"sha": HEAD}}\n ),\n''' - if text.count(request_env) != 1: - raise SystemExit('expected request-step workflow SHA entry not found exactly once') - text = text.replace(request_env, request_replacement, 1) - - text = text.replace( - 'assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout', - 'assert "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required" in result.stdout', - ) - text = text.replace( - 'assert "PR is a draft; a current-head OpenCode review is not requested" in result.stdout', - 'assert "PR is still a draft on the live exact head; a current-head OpenCode review is not requested" in result.stdout', - ) - text = text.replace( - 'assert "unexpected gh invocation" in result.stderr', - 'assert "unexpected gh invocation after live-state validation" in result.stderr', - ) - text = text.replace( - 'This proves the request step now\n exits before any API call -- helper-source fetch included -- when', - 'This proves the request step now performs only the authoritative live-state lookup, then\n exits before helper-source, review, token, or dispatch API calls when', - ) - - old_fake = '''if [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then\n python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER"\n''' - new_fake = '''if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then\n printf '%s' "$LIVE_PR_JSON"\nelif [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then\n python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER"\n''' - if old_fake not in text: - raise SystemExit('scheduler-wake fake gh block not found') - text = text.replace(old_fake, new_fake, 1) - - scheduler_env = ''' "GH_TOKEN": "token",\n''' - scheduler_replacement = ''' "GH_TOKEN": "token",\n "LIVE_PR_JSON": json.dumps({"draft": False, "head": {"sha": HEAD}}),\n''' - if text.count(scheduler_env) != 1: - raise SystemExit('scheduler wake GH_TOKEN environment entry not found exactly once') - text = text.replace(scheduler_env, scheduler_replacement, 1) - path.write_text(text, encoding='utf-8') - - live_path = Path('tests/test_opencode_live_draft_state_regression.py') - live_text = live_path.read_text(encoding='utf-8') - old_message = 'assert "head moved while validating draft" in result.stdout' - if live_text.count(old_message) != 1: - raise SystemExit('moved-head assertion not found exactly once') - live_path.write_text( - live_text.replace(old_message, 'assert "head moved while validating live" in result.stdout', 1), - encoding='utf-8', - ) - - queue_path = Path('tests/test_required_workflow_queue_contract.py') - queue_text = queue_path.read_text(encoding='utf-8') - old_trigger = 'types: [opened, synchronize, reopened, ready_for_review, closed]' - new_trigger = 'types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]' - if queue_text.count(old_trigger) != 1: - raise SystemExit('legacy OpenCode trigger assertion not found exactly once') - queue_path.write_text(queue_text.replace(old_trigger, new_trigger, 1), encoding='utf-8') - PY - - - name: Run focused and full exact-head tests - run: | - set -euo pipefail - python -m pytest -q \ - tests/test_opencode_required_verdict_regression.py \ - tests/test_opencode_live_draft_state_regression.py \ - tests/test_required_workflow_queue_contract.py - python -m pytest -q tests - - - name: Commit repair and remove this one-shot workflow - env: - GH_TOKEN: ${{ github.token }} - RUN_HEAD: ${{ github.sha }} - run: | - set -euo pipefail - rm .github/workflows/source-fix-1568-live-state-regressions.yml - git diff --check - git add tests/conftest.py \ - tests/test_opencode_required_verdict_regression.py \ - tests/test_opencode_live_draft_state_regression.py \ - tests/test_required_workflow_queue_contract.py \ - .github/workflows/source-fix-1568-live-state-regressions.yml - git config user.name github-actions[bot] - git config user.email 41898282+github-actions[bot]@users.noreply.github.com - git commit -m 'test(ci): align live-state regressions with admission contract' - live_head="$(gh api repos/ContextualWisdomLab/.github/pulls/1568 --jq .head.sha)" - test "$live_head" = "$RUN_HEAD" - git push origin HEAD:fix/opencode-review-draft-poll-exemption diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index 87161901e7..51af63d57b 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -167,4 +167,4 @@ def test_draft_exemption_fails_closed_when_live_head_moved( result = _run_step(tmp_path, script, live_draft=True, live_head="b" * 40) assert result.returncode == 1 - assert "head moved while validating draft" in result.stdout + assert "head moved while validating live" in result.stdout diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 1b4fd2b3a6..6a4e28eb28 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -156,15 +156,17 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) -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. - """ +def _write_live_pr_then_refusing_gh(bin_dir: Path) -> None: + """Serve the authoritative live PR lookup, then reject downstream GitHub I/O.""" 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" + "set -euo pipefail\n" + "if [[ \"$*\" == \"api repos/ContextualWisdomLab/example/pulls/1437\" ]]; then\n" + " printf '%s' \"$LIVE_PR_JSON\"\n" + " exit 0\n" + "fi\n" + "echo 'unexpected gh invocation after live-state validation' >&2\n" "exit 17\n", encoding="utf-8", ) @@ -194,7 +196,7 @@ def _run_fail_closed_step( pytest.skip("bash and jq are required to execute the production step body") bin_dir = tmp_path / "bin" bin_dir.mkdir() - _write_refusing_gh(bin_dir) + _write_live_pr_then_refusing_gh(bin_dir) return subprocess.run( [bash, "-c", fail_closed_script()], env={ @@ -206,6 +208,9 @@ def _run_fail_closed_step( "HEAD_SHA": head_sha, "PR_ACTION": pr_action, "PR_DRAFT": pr_draft, + "LIVE_PR_JSON": json.dumps( + {"draft": pr_draft.lower() == "true", "head": {"sha": head_sha}} + ), }, text=True, capture_output=True, @@ -231,7 +236,7 @@ def test_fail_closed_step_exempts_a_draft_pr_before_polling(tmp_path: Path) -> N """ result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="true") assert result.returncode == 0, result.stderr - assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout + assert "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required" in result.stdout def _run_request_review_step( @@ -251,7 +256,7 @@ def _run_request_review_step( pytest.skip("bash is required to execute the production step body") bin_dir = tmp_path / "bin" bin_dir.mkdir() - _write_refusing_gh(bin_dir) + _write_live_pr_then_refusing_gh(bin_dir) return subprocess.run( [bash, "-c", request_review_script()], env={ @@ -266,6 +271,9 @@ def _run_request_review_step( "PR_DRAFT": pr_draft, "BASE_BRANCH": "main", "WORKFLOW_SHA": "c" * 40, + "LIVE_PR_JSON": json.dumps( + {"draft": pr_draft.lower() == "true", "head": {"sha": HEAD}} + ), }, text=True, capture_output=True, @@ -284,8 +292,8 @@ def test_request_review_step_exempts_a_pr_converted_to_draft_before_any_api_call had no draft exemption at all, so it still fetched the receipt-gate helper source and queried the Reviews API, and could reach OIDC token exchange and a `repository_dispatch` scheduler wake, before the "Fail - closed" step's exemption ever ran. This proves the request step now - exits before any API call -- helper-source fetch included -- when + closed" step's exemption ever ran. This proves the request step now performs only the authoritative live-state lookup, then + exits before helper-source, review, token, or dispatch API calls when `PR_DRAFT` is `"true"` (the value GitHub sends for `converted_to_draft`), while `ready_for_review` and explicit draft-review dispatch paths elsewhere (`pr_review_merge_scheduler.py`'s own draft handling) are @@ -293,7 +301,7 @@ def test_request_review_step_exempts_a_pr_converted_to_draft_before_any_api_call """ result = _run_request_review_step(tmp_path, pr_draft="true") assert result.returncode == 0, result.stderr - assert "PR is a draft; a current-head OpenCode review is not requested" in result.stdout + assert "PR is still a draft on the live exact head; a current-head OpenCode review is not requested" in result.stdout def test_request_review_step_still_dispatches_for_a_non_draft_pr( @@ -302,7 +310,7 @@ def test_request_review_step_still_dispatches_for_a_non_draft_pr( """A non-draft PR must still reach the receipt-gate helper fetch.""" result = _run_request_review_step(tmp_path, pr_draft="false") assert result.returncode == 17, result.stderr - assert "unexpected gh invocation" in result.stderr + assert "unexpected gh invocation after live-state validation" in result.stderr def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll( @@ -326,7 +334,7 @@ def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll( tmp_path, pr_action="converted_to_draft", pr_draft="true" ) assert result.returncode == 0, result.stderr - assert "PR is a draft; a current-head OpenCode verdict is not required" in result.stdout + assert "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required" in result.stdout def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None: @@ -362,7 +370,7 @@ def test_fail_closed_step_still_polls_for_a_non_draft_pr(tmp_path: Path) -> None """A non-draft PR must still reach the Reviews API call (not exempted).""" result = _run_fail_closed_step(tmp_path, pr_action="synchronize", pr_draft="false") assert result.returncode == 17, result.stderr - assert "unexpected gh invocation" in result.stderr + assert "unexpected gh invocation after live-state validation" in result.stderr @pytest.mark.parametrize( @@ -386,7 +394,9 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( fake_gh.write_text( """#!/usr/bin/env bash set -euo pipefail -if [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + printf '%s' "$LIVE_PR_JSON" +elif [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER" elif [[ "$*" == *"/pulls/7/reviews"* ]]; then printf '[%s]' "$FAKE_REVIEWS" @@ -423,6 +433,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( "BASE_BRANCH": "main", "WORKFLOW_SHA": "c" * 40, "GH_TOKEN": "token", + "LIVE_PR_JSON": json.dumps({"draft": False, "head": {"sha": HEAD}}), } result = subprocess.run( ["bash", "-c", request_review_script()], env=env, text=True, capture_output=True diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index f456770580..cb8198e30d 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -578,7 +578,7 @@ 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 ( + assert "types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]" in ( opencode_bootstrap ) assert "actions/checkout" not in opencode_bootstrap From 2193cda877c47b4b5fe8314b19d9505871c4fc08 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 10:24:35 +0000 Subject: [PATCH 13/16] docs(doctoring): correct draft-exemption repair record for the live-state lookup Devin Review flagged that this record still promised an exit before any API call, but both the request-review and required-verdict polling steps now make one unconditional gh api live-PR lookup before exiting on a confirmed live draft state (added after the initial fix so a stale event-payload PR_DRAFT/head can't be trusted alone). Update the repair description to match. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- docs/doctoring/opencode-draft-verdict-cycle.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/doctoring/opencode-draft-verdict-cycle.md b/docs/doctoring/opencode-draft-verdict-cycle.md index 1f4ee0442e..7615a0f5ce 100644 --- a/docs/doctoring/opencode-draft-verdict-cycle.md +++ b/docs/doctoring/opencode-draft-verdict-cycle.md @@ -14,12 +14,11 @@ A second edge existed when a ready PR was converted back to draft while a poll w ## Repair - Add `converted_to_draft` to the `pull_request_target` trigger set. -- Exit the request-review step immediately when `PR_DRAFT=true`, before any GitHub API or token exchange. -- Exit the required-verdict polling step immediately for drafts. +- Both the request-review and required-verdict polling steps first make one unconditional, authoritative `gh api` live PR lookup (added after the initial fix, per Devin Review on this PR: a stale event-payload `PR_DRAFT`/head cannot be trusted on its own) and fail closed on a lookup error or an exact-head mismatch. Only after that live lookup confirms the PR is still draft on the live exact head does each step exit -- before any *further* GitHub API call or token exchange. - Preserve `ready_for_review` behavior and the separate explicit marker-backed draft-review path. - Keep the existing PR-scoped `cancel-in-progress: true` concurrency behavior so the converted-to-draft event replaces a stale non-draft poll. -Executable regressions cover the trigger, request-step no-I/O exemption, verdict-step exemption, closed-event precedence, and unchanged non-draft behavior. +Executable regressions cover the trigger, the request-step and verdict-step live-state-then-exit exemptions, closed-event precedence, moved-head fail-closed behavior, and unchanged non-draft behavior. ## Reconciliation From dbdab4594c29514c689e2e17ac9307b81aee0415 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 10:50:55 +0000 Subject: [PATCH 14/16] fix(opencode-review): isolate concurrency by head SHA, validate live closed state Owner-directed fix for two Devin Review findings on #1568's live-state validation (head/draft lookup landed in an earlier commit on this branch): 1. Stale runs could cancel the current check. The concurrency group was keyed only by repository and PR number; GitHub cancels whichever run is currently active in a group when a new one starts, with no notion of "older"/"newer", so a delayed run for an older head could cancel the newer, authoritative head's still-valid run before that older run's own live-head check ever had a chance to reject it. Scope the group by exact head SHA too, so different heads no longer share a cancellation domain while same-head events (converted_to_draft/ready_for_review, a synchronize retry) still do -- preserving converted_to_draft's ability to retire an active same-head verdict poll. 2. A delayed non-closed event ignored a live-closed PR: live_pr only ever extracted head and draft. Both admission blocks now also extract and validate live state, exiting before any further API call when it is "closed" -- mirroring the existing PR_ACTION == "closed" event-level short-circuit but driven by live truth. A missing, null, non-string, or otherwise unrecognized state value fails closed rather than assuming open, matching the existing live_head/live_draft validation style. New regressions: a structural contract test for the head-scoped concurrency group; step-body coverage for a stale non-closed event against a live-closed PR (both admission steps), live-closed state taking precedence over a stale live-draft flag, and each invalid state shape (missing/null/non-string/unexpected value) failing closed. Updated every existing LIVE_PR_JSON test fixture to include a state field now that production requires one. Verified: PYTHONPATH=. python -m pytest tests -> 2294 passed, 1 skipped, 21 subtests; coverage on scripts/ci -> 100%; interrogate -> 100%. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 45 ++++++-- CHANGELOG.md | 16 +++ .../doctoring/opencode-draft-verdict-cycle.md | 11 +- ...st_opencode_live_draft_state_regression.py | 105 +++++++++++++++++- ...st_opencode_required_verdict_regression.py | 41 ++++++- .../test_required_workflow_queue_contract.py | 12 +- 6 files changed, 215 insertions(+), 15 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index e799637fec..9ba9f7aa3c 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -10,19 +10,30 @@ on: # isolated in opencode-review-dispatch.yml on repository_dispatch only. pull_request_target: # `converted_to_draft` is included so a PR going draft mid-poll fires a - # fresh run of this same workflow: the PR-scoped concurrency group below + # fresh run of this same workflow: the head-scoped concurrency group below # (`cancel-in-progress: true`) cancels any in-flight non-draft - # "Fail closed without a current-head OpenCode verdict" poll for that PR. - # Every non-closed admission path revalidates the live PR/head before - # dispatching, exempting, or polling so out-of-order draft/ready events - # cannot publish stale evidence or wait on an impossible verdict. + # "Fail closed without a current-head OpenCode verdict" poll for that + # exact same head. Every non-closed admission path revalidates the live + # PR/head/state before dispatching, exempting, or polling so out-of-order + # draft/ready/closed events cannot publish stale evidence or wait on an + # impossible verdict. types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: + # Scoped by exact head SHA (not just PR number) so a delayed, out-of-order + # run for an older head cannot cancel the authoritative run already active + # for a newer head -- GitHub cancels whichever run is currently active in + # the group when a new one starts, with no notion of "older"/"newer", so + # sharing a group across different heads let a stale event retire the + # current head's still-valid run before its own live-head check could ever + # reject it (Devin Review on `#1568`). Same-head events (draft<->ready + # transitions, a synchronize retry) still share one group, so + # `converted_to_draft` still cancels an active same-head verdict poll. group: >- opencode-review-bootstrap-${{ github.event.pull_request.base.repo.full_name || github.repository }}-${{ - github.event.pull_request.number || github.run_id }} + github.event.pull_request.number || github.run_id }}-${{ + github.event.pull_request.head.sha || github.run_id }} cancel-in-progress: true permissions: @@ -280,7 +291,12 @@ jobs: live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" live_draft="$(printf '%s' "$live_pr" | jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end')" - if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + live_state="$(printf '%s' "$live_pr" | jq -r 'if (.state | type) == "string" then .state else empty end')" + if [ -z "$live_head" ] || [ -z "$live_draft" ] || [ -z "$live_state" ]; then + echo "::error::Could not validate live pull request state before review dispatch." + exit 1 + fi + if [ "$live_state" != "open" ] && [ "$live_state" != "closed" ]; then echo "::error::Could not validate live pull request state before review dispatch." exit 1 fi @@ -288,6 +304,10 @@ jobs: echo "::error::Pull request head moved while validating live review state." exit 1 fi + if [ "$live_state" = "closed" ]; then + echo "PR is closed on the live exact head; a current-head OpenCode review is not requested." + exit 0 + fi if [ "$live_draft" = "true" ]; then echo "PR is still a draft on the live exact head; a current-head OpenCode review is not requested until it is marked ready for review." exit 0 @@ -374,7 +394,12 @@ jobs: live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" live_draft="$(printf '%s' "$live_pr" | jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end')" - if [ -z "$live_head" ] || [ -z "$live_draft" ]; then + live_state="$(printf '%s' "$live_pr" | jq -r 'if (.state | type) == "string" then .state else empty end')" + if [ -z "$live_head" ] || [ -z "$live_draft" ] || [ -z "$live_state" ]; then + echo "::error::Could not validate live pull request state before verdict admission." + exit 1 + fi + if [ "$live_state" != "open" ] && [ "$live_state" != "closed" ]; then echo "::error::Could not validate live pull request state before verdict admission." exit 1 fi @@ -382,6 +407,10 @@ jobs: echo "::error::Pull request head moved while validating live verdict state." exit 1 fi + if [ "$live_state" = "closed" ]; then + echo "PR is closed on the live exact head; a current-head OpenCode verdict is not required." + exit 0 + fi if [ "$live_draft" = "true" ]; then echo "PR is still a draft on the live exact head; a current-head OpenCode verdict is not required until it is marked ready for review." exit 0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8633515b..c905f4d75a 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` admission gaps around stale/out-of-order events (`#1568`).** + Building on the draft-poll exemption's live PR/head validation, Devin Review found two + further defects. (1) The concurrency group was keyed only by repository and PR number, so + a delayed run for an *older* head could cancel the *newer*, authoritative head's still-valid + run before that older run's own live-head check ever had a chance to reject it (GitHub cancels + whichever run is currently active in a group with no notion of "older"/"newer"). Fixed by also + scoping the group by exact head SHA, so different heads no longer share a cancellation domain + while same-head events (a `converted_to_draft`/`ready_for_review` transition, a `synchronize` + retry) still do. (2) A delayed non-closed event ignored a live-closed PR, since `live_pr` only + ever extracted `head` and `draft`. Both admission blocks now also validate live `state` and exit + before any further API call when it is `"closed"`, failing closed on a missing, null, + non-string, or otherwise unrecognized value rather than assuming open. New regressions: a + structural contract test for the head-scoped concurrency group; step-body coverage for a stale + non-closed event against a live-closed PR (both admission steps), live-closed state taking + precedence over a stale live-draft flag, and each invalid `state` shape failing closed. Full + suite: 2294 passed, 1 skipped, 21 subtests; `scripts/ci` coverage and docstrings both 100%. - **Fix a live crash: `noema-review` failed with an unhandled `HTTPError` instead of failing closed.** Live incident on `ContextualWisdomLab/naruon#1486`: `scripts/ci/noema_review_gate.py::call_llm`'s `opener.open(request)` call sat diff --git a/docs/doctoring/opencode-draft-verdict-cycle.md b/docs/doctoring/opencode-draft-verdict-cycle.md index 7615a0f5ce..a26e8bb0b8 100644 --- a/docs/doctoring/opencode-draft-verdict-cycle.md +++ b/docs/doctoring/opencode-draft-verdict-cycle.md @@ -16,10 +16,19 @@ A second edge existed when a ready PR was converted back to draft while a poll w - Add `converted_to_draft` to the `pull_request_target` trigger set. - Both the request-review and required-verdict polling steps first make one unconditional, authoritative `gh api` live PR lookup (added after the initial fix, per Devin Review on this PR: a stale event-payload `PR_DRAFT`/head cannot be trusted on its own) and fail closed on a lookup error or an exact-head mismatch. Only after that live lookup confirms the PR is still draft on the live exact head does each step exit -- before any *further* GitHub API call or token exchange. - Preserve `ready_for_review` behavior and the separate explicit marker-backed draft-review path. -- Keep the existing PR-scoped `cancel-in-progress: true` concurrency behavior so the converted-to-draft event replaces a stale non-draft poll. +- Keep `cancel-in-progress: true` concurrency behavior, now scoped by exact head SHA in addition to PR number (see "Head-scoped concurrency" below) so the converted-to-draft event still replaces a stale same-head poll. Executable regressions cover the trigger, the request-step and verdict-step live-state-then-exit exemptions, closed-event precedence, moved-head fail-closed behavior, and unchanged non-draft behavior. +## Head-scoped concurrency and live closed-state validation (second Devin Review round) + +Devin Review found two further defects once the live head/draft lookup above landed: + +1. **Stale runs could cancel the current check.** The concurrency group was keyed only by repository and PR number. GitHub cancels whichever run is currently active in a group when a new one starts -- it has no notion of "older" or "newer" -- so a delayed, out-of-order run for an *older* head (e.g. a `synchronize` webhook delivered late under the org's saturated Actions queue) could cancel the *newer*, authoritative head's still-valid run before that older run's own live-head check ever had a chance to reject it. Fixed by also scoping the group by `github.event.pull_request.head.sha`: different heads no longer share a cancellation domain, while events for the exact same head (a `converted_to_draft`/`ready_for_review` transition, a `synchronize` retry) still do, which is what lets `converted_to_draft` retire an active same-head verdict poll. +2. **A delayed non-closed event ignored a live-closed PR.** `live_pr` only ever extracted `head` and `draft`; a stale `synchronize`/`ready_for_review`/etc. event arriving after the PR was actually closed had no way to notice and could still fetch the receipt-gate helper, exchange an OIDC token, dispatch a scheduler wake, or poll the Reviews API indefinitely. Both admission blocks now also extract and validate live `state`, exiting before any of that when it is `"closed"` -- mirroring the pre-existing `PR_ACTION == "closed"` event-level short-circuit, but driven by live API truth instead of the (possibly stale) event payload. A missing, null, non-string, or otherwise unrecognized `state` value fails closed rather than being treated as open, matching the existing `live_head`/`live_draft` validation style. + +Executable regressions: a structural contract test pins the concurrency group's head-SHA scoping; step-body regressions cover a stale non-closed event against a live-closed PR (for both admission steps), live-closed state taking precedence over a stale live-draft flag, and each invalid `state` shape (missing/null/non-string/unexpected value) failing closed. + ## Reconciliation The original branch diverged while unrelated protected-main repairs landed, including the Noema transport repair and the `graphql-core` security update. The branch is reconciled with current protected `main` through a normal two-parent merge commit; no force push or destructive rebase is used. Newer protected-main documentation is retained rather than replaced with stale branch copies. The concurrent review-event scheduler wake regression is retained in a dedicated regression file. diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index 51af63d57b..18fd482b64 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -23,11 +23,23 @@ def _write_live_state_gh( *, live_draft: bool, live_head: str = HEAD, + live_state: str = "open", later_exit: int = 19, approved_receipt: bool = False, + live_payload_override: dict[str, object] | None = None, ) -> None: - """Serve live PR state and optionally one approved receipt helper fixture.""" - payload = json.dumps({"draft": live_draft, "head": {"sha": live_head}}) + """Serve live PR state and optionally one approved receipt helper fixture. + + ``live_payload_override`` replaces the whole live-PR JSON body outright, + for exercising a missing/null/non-string/unexpected ``state`` field that + the convenience ``live_draft``/``live_head``/``live_state`` parameters + cannot express. + """ + payload = json.dumps( + live_payload_override + if live_payload_override is not None + else {"draft": live_draft, "head": {"sha": live_head}, "state": live_state} + ) helper_source = """def fetch_reviews(repository, number): return [{\"state\": \"APPROVED\"}] @@ -66,9 +78,11 @@ def _run_step( *, live_draft: bool, live_head: str = HEAD, + live_state: str = "open", event_draft: bool = True, action: str = "converted_to_draft", approved_receipt: bool = False, + live_payload_override: dict[str, object] | None = None, ) -> subprocess.CompletedProcess[str]: """Execute one production step against independently controlled live state.""" bash = shutil.which("bash") @@ -81,7 +95,9 @@ def _run_step( bin_dir, live_draft=live_draft, live_head=live_head, + live_state=live_state, approved_receipt=approved_receipt, + live_payload_override=live_payload_override, ) return subprocess.run( [bash, "-c", script], @@ -168,3 +184,88 @@ def test_draft_exemption_fails_closed_when_live_head_moved( assert result.returncode == 1 assert "head moved while validating live" in result.stdout + + +@pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) +def test_stale_non_closed_event_exempts_a_live_closed_pr( + tmp_path: Path, + script: str, +) -> None: + """A delayed non-closed event cannot dispatch or poll against a live-closed PR. + + Devin Review on `#1568` found that `live_pr` only ever extracted `head` + and `draft` -- a delayed `synchronize`/`ready_for_review`/etc. event + arriving after the PR was actually closed would ignore that live closed + state entirely and could still fetch the receipt-gate helper, exchange + an OIDC token, dispatch a scheduler wake, or poll the Reviews API + indefinitely. Both admission blocks now also validate live `state` and + exit before any of that when it is `"closed"`, exactly like the + pre-existing `PR_ACTION == "closed"` short-circuit for a genuinely + closed *event*. + """ + result = _run_step( + tmp_path, + script, + live_draft=False, + live_state="closed", + event_draft=False, + action="synchronize", + ) + + assert result.returncode == 0, result.stderr + assert "PR is closed on the live exact head" in result.stdout + + +@pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) +def test_live_closed_state_takes_precedence_over_live_draft( + tmp_path: Path, + script: str, +) -> None: + """A live-closed PR is reported as closed, not draft, even if also draft.""" + result = _run_step( + tmp_path, + script, + live_draft=True, + live_state="closed", + event_draft=False, + action="synchronize", + ) + + assert result.returncode == 0, result.stderr + assert "PR is closed on the live exact head" in result.stdout + assert "still a draft on the live exact head" not in result.stdout + + +@pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) +@pytest.mark.parametrize( + "live_payload_override", + ( + {"draft": False, "head": {"sha": HEAD}}, + {"draft": False, "head": {"sha": HEAD}, "state": None}, + {"draft": False, "head": {"sha": HEAD}, "state": 1}, + {"draft": False, "head": {"sha": HEAD}, "state": "merged"}, + ), + ids=("missing", "null", "non-string", "unexpected-value"), +) +def test_live_invalid_state_fails_closed( + tmp_path: Path, + script: str, + live_payload_override: dict[str, object], +) -> None: + """A missing, null, non-string, or unrecognized live `state` fails closed. + + GitHub's own REST API only ever reports `"open"` or `"closed"`; anything + else is treated as untrustworthy live evidence rather than assumed open + (Devin Review on `#1568`). + """ + result = _run_step( + tmp_path, + script, + live_draft=False, + event_draft=False, + action="synchronize", + live_payload_override=live_payload_override, + ) + + assert result.returncode == 1 + assert "Could not validate live pull request state" in result.stdout diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 6a4e28eb28..66beee8906 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -209,7 +209,11 @@ def _run_fail_closed_step( "PR_ACTION": pr_action, "PR_DRAFT": pr_draft, "LIVE_PR_JSON": json.dumps( - {"draft": pr_draft.lower() == "true", "head": {"sha": head_sha}} + { + "draft": pr_draft.lower() == "true", + "head": {"sha": head_sha}, + "state": "open", + } ), }, text=True, @@ -272,7 +276,11 @@ def _run_request_review_step( "BASE_BRANCH": "main", "WORKFLOW_SHA": "c" * 40, "LIVE_PR_JSON": json.dumps( - {"draft": pr_draft.lower() == "true", "head": {"sha": HEAD}} + { + "draft": pr_draft.lower() == "true", + "head": {"sha": HEAD}, + "state": "open", + } ), }, text=True, @@ -358,6 +366,31 @@ def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None: assert "cancel-in-progress: true" in workflow +def test_opencode_review_concurrency_group_is_scoped_by_exact_head() -> None: + """The bootstrap concurrency group is keyed by head SHA, not just PR number. + + Devin Review on `#1568` found that a delayed, out-of-order run for an + older head could cancel the authoritative run already active for a + newer head: GitHub cancels whichever run is currently active in a + concurrency group when a new one starts, with no notion of "older" or + "newer", so a group shared across different heads let a stale event + retire the current head's still-valid run before its own live-head + check could ever reject it. Scoping the group by exact head SHA + isolates different heads from each other while events for the exact + same head (a `converted_to_draft`/`ready_for_review` transition, a + `synchronize` retry) still share one group and can still cancel each + other, which is what lets `converted_to_draft` retire an active + same-head verdict poll. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + concurrency_block = workflow.split("\n\nconcurrency:\n", 1)[1].split( + "\n\npermissions:", 1 + )[0] + assert "github.event.pull_request.head.sha || github.run_id" in concurrency_block + assert "github.event.pull_request.number || github.run_id" in concurrency_block + assert "cancel-in-progress: true" in concurrency_block + + def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Path) -> None: """The pre-existing ``closed`` early exit still runs before the new draft check.""" result = _run_fail_closed_step(tmp_path, pr_action="closed", pr_draft="true") @@ -433,7 +466,9 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( "BASE_BRANCH": "main", "WORKFLOW_SHA": "c" * 40, "GH_TOKEN": "token", - "LIVE_PR_JSON": json.dumps({"draft": False, "head": {"sha": HEAD}}), + "LIVE_PR_JSON": json.dumps( + {"draft": False, "head": {"sha": HEAD}, "state": "open"} + ), } result = subprocess.run( ["bash", "-c", request_review_script()], env=env, text=True, capture_output=True diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index cb8198e30d..a5079daa67 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -253,6 +253,16 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: ) elif filename == "opencode-review.yml": assert "opencode-review-bootstrap-" in concurrency_contract + # Unlike the other required pull-request workflows below, this + # group is deliberately also scoped by exact head SHA: a + # delayed, out-of-order run for an older head must not be able + # to cancel the authoritative run already active for a newer + # head (Devin Review on `#1568`). Same-head events still share + # one group and can still cancel each other. + assert ( + "github.event.pull_request.head.sha || github.run_id" + in concurrency_contract + ) elif filename == "noema-review.yml": assert "github.event.workflow_run" not in concurrency_contract assert "noema-review-${{" in concurrency_contract @@ -268,7 +278,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert ( "github.event_name == 'pull_request_target'" in concurrency_contract ) - if filename != "noema-review.yml": + if filename not in {"noema-review.yml", "opencode-review.yml"}: assert "github.event.pull_request.head.sha" not in concurrency_contract assert "format('pr-{0}-{1}'" not in concurrency_contract From 4732983cda3afc940f4adfc8c7ab61ff1e604b3e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 11:05:44 +0000 Subject: [PATCH 15/16] fix(opencode-review): retire superseded-head runs on legitimate synchronize Devin Review round 3 on #1568: scoping the concurrency group by exact head SHA fixed the wrong-direction cancellation (a delayed old-head run could no longer cancel a newer, authoritative run) but also disabled the legitimate one -- a genuine new commit no longer shares a group with its own PR's now-obsolete previous-head poll, so nothing cancels it. That older run's live-head check ran once, before entering the unbounded Reviews API wait loop, which never re-validates the head on later iterations; left alone it occupies a hosted runner until GitHub's own per-job ceiling. Add a cancel-superseded-opencode-review-runs job, scoped to synchronize events, mirroring the already-established live-head-validated cleanup pattern in strix.yml's own cancel-superseded-pr-runs job: list this PR's other active Required OpenCode Review runs (matched by workflow name/event plus a display-title or pull_requests[] PR-number match), exclude the currently-executing run and any run already on the live head, and cancel the rest -- re-verifying the live head immediately before both the listing pass and each individual cancellation, so a delayed/stale invocation of this same job cannot itself wrongly cancel a still-authoritative run. New regressions: the embedded run-selection jq filter extracted and executed against synthetic workflow_runs payloads (mirroring how runtime_verdict() already exercises the required-verdict filter) -- superseded-run selection, current-head/self-run/other-PR/other-workflow exclusion, and pull_requests[] metadata matching when display_title never rendered the head suffix; a structural test pins the job's synchronize-only trigger and actions: write permission. Verified: PYTHONPATH=. python -m pytest tests -> 2301 passed, 1 skipped, 21 subtests; coverage on scripts/ci -> 100%; interrogate -> 100%; YAML parses cleanly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 95 ++++++++++++++ CHANGELOG.md | 12 ++ .../doctoring/opencode-draft-verdict-cycle.md | 8 ++ ...st_opencode_required_verdict_regression.py | 123 ++++++++++++++++++ 4 files changed, 238 insertions(+) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 9ba9f7aa3c..8d275793b4 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -458,4 +458,99 @@ jobs: 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 fi + + cancel-superseded-opencode-review-runs: + # Scoping the concurrency group above by exact head SHA (so a delayed + # old-head run can no longer cancel the authoritative newer-head run -- + # Devin Review on `#1568`) also means a *legitimate* new commit no + # longer auto-cancels its own PR's now-obsolete previous-head poll: that + # older run's own live-head check only ran once, before it entered its + # unbounded Reviews API wait, and nothing in that wait loop re-validates + # the head. Left alone, it would occupy a runner until GitHub's own + # per-job ceiling. This job retires it directly, mirroring the + # live-head-validated cleanup pattern in strix.yml's own + # `cancel-superseded-pr-runs` job: every cancellation candidate and + # every cancellation itself is re-verified against the live PR head + # immediately beforehand, so a run for this job that is itself somehow + # delayed/stale cannot wrongly cancel a still-authoritative run. + if: github.event_name == 'pull_request_target' && github.event.action == 'synchronize' + runs-on: ubuntu-latest + permissions: + actions: write + contents: read + pull-requests: read + env: + GH_TOKEN: ${{ github.token }} + TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} + TARGET_PR_NUMBER: ${{ github.event.pull_request.number }} + TARGET_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + CURRENT_RUN_ID: ${{ github.run_id }} + steps: + - name: Cancel queued and running OpenCode review runs for a superseded pull request head + shell: bash + run: | + set -euo pipefail + + live_head_matches() { + local live_head + if ! live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}" --jq '.head.sha' 2>/tmp/opencode-cleanup-gh-error)"; then + echo "::warning::OpenCode review cleanup could not verify the live pull request head; leaving runs unchanged." + sed 's/^/ /' /tmp/opencode-cleanup-gh-error >&2 || true + return 1 + fi + [ "${live_head,,}" = "${TARGET_PR_HEAD_SHA,,}" ] + } + + cancel_runs() { + local status="$1" + if ! live_head_matches; then + echo "::notice::OpenCode review cleanup target changed before run selection; leaving runs unchanged." + return 0 + fi + local runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100" + local runs_json + if ! runs_json="$(gh api --paginate "$runs_url" 2>/tmp/opencode-cleanup-gh-error)"; then + echo "::warning::OpenCode review cleanup could not inspect ${TARGET_REPOSITORY}; leaving runs unchanged." + sed 's/^/ /' /tmp/opencode-cleanup-gh-error >&2 || true + return 0 + fi + local run_ids + if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ + --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' + .workflow_runs[] + | select((.id | tostring) != $current) + | select(.name == "Required OpenCode Review") + | select(.event == "pull_request_target") + | ((.display_title // "") | startswith("Required OpenCode Review " + $repo + "#" + $pr + "@")) as $title_matches + | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches + | select($title_matches or $metadata_matches) + | ((.display_title // "") | endswith("@" + $head_sha)) as $title_is_current + | ((.pull_requests // []) | any( + ((.number | tostring) == $pr) + and ((.head.sha // "") | ascii_downcase) == ($head_sha | ascii_downcase) + )) as $metadata_is_current + | select(($title_is_current or $metadata_is_current) | not) + | .id + ' <<<"$runs_json")"; then + echo "::warning::OpenCode review cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." + return 0 + fi + while IFS= read -r run_id; do + [ -n "$run_id" ] || continue + if ! live_head_matches; then + echo "::notice::OpenCode review cleanup target changed before cancellation; leaving runs unchanged." + return 0 + fi + if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/opencode-cleanup-cancel-error; then + echo "Cancelled superseded Required OpenCode Review run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." + else + echo "::warning::OpenCode review cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." + sed 's/^/ /' /tmp/opencode-cleanup-cancel-error >&2 || true + fi + done <<<"$run_ids" + } + + for active_status in queued in_progress requested waiting pending; do + cancel_runs "$active_status" + done echo "Current-head OpenCode verdict: ${verdict}." \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index c905f4d75a..d7c6d40ae7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,18 @@ Semantic Versioning where the repository publishes a release. non-closed event against a live-closed PR (both admission steps), live-closed state taking precedence over a stale live-draft flag, and each invalid `state` shape failing closed. Full suite: 2294 passed, 1 skipped, 21 subtests; `scripts/ci` coverage and docstrings both 100%. + A third Devin Review round then found that head-scoping the concurrency group above, while + fixing the wrong-direction cancellation, also disabled the legitimate one: a genuine new + commit no longer cancels its own PR's now-obsolete previous-head poll, which would otherwise + occupy a runner until GitHub's own per-job ceiling. Added a `cancel-superseded-opencode-review-runs` + job, scoped to `synchronize` events, mirroring the already-established live-head-validated + cleanup pattern in `strix.yml`'s `cancel-superseded-pr-runs` job: it re-verifies the live head + immediately before both listing candidates and cancelling each one, so a delayed/stale + invocation of this same job cannot itself wrongly cancel a still-authoritative run. New + regressions: the embedded run-selection `jq` filter executed against synthetic run payloads + (superseded-run selection, current-head/self-run/other-PR/other-workflow exclusion, and + `pull_requests[]` metadata matching), plus a structural test for the job's trigger and + permissions. Full suite: 2301 passed, 1 skipped, 21 subtests; coverage and docstrings both 100%. - **Fix a live crash: `noema-review` failed with an unhandled `HTTPError` instead of failing closed.** Live incident on `ContextualWisdomLab/naruon#1486`: `scripts/ci/noema_review_gate.py::call_llm`'s `opener.open(request)` call sat diff --git a/docs/doctoring/opencode-draft-verdict-cycle.md b/docs/doctoring/opencode-draft-verdict-cycle.md index a26e8bb0b8..2347e65acd 100644 --- a/docs/doctoring/opencode-draft-verdict-cycle.md +++ b/docs/doctoring/opencode-draft-verdict-cycle.md @@ -29,6 +29,14 @@ Devin Review found two further defects once the live head/draft lookup above lan Executable regressions: a structural contract test pins the concurrency group's head-SHA scoping; step-body regressions cover a stale non-closed event against a live-closed PR (for both admission steps), live-closed state taking precedence over a stale live-draft flag, and each invalid `state` shape (missing/null/non-string/unexpected value) failing closed. +## Superseded-run cleanup for legitimate new commits (third Devin Review round) + +Head-scoping the concurrency group above fixed the wrong-direction cancellation, but Devin Review found it also disabled a *legitimate* one: a genuine new commit (`synchronize`, head A -> B) no longer shares a concurrency group with head A's now-obsolete run, so nothing cancels it anymore. That older run's own live-head check ran once, before it entered the unbounded Reviews API wait loop, which never re-validates the head on later iterations -- left alone, it would occupy a hosted runner polling for a verdict OpenCode will never produce for that head, until GitHub's own per-job ceiling. + +Fixed by adding a dedicated `cancel-superseded-opencode-review-runs` job, scoped to `synchronize` events, mirroring the already-established live-head-validated cleanup pattern in `strix.yml`'s own `cancel-superseded-pr-runs` job (and `noema-review.yml`'s in-job equivalent): it lists this PR's other active `Required OpenCode Review` runs (matched by workflow name/event plus a display-title or `pull_requests[]` PR-number match), excludes the currently-executing run and any run already on the live head, and cancels the rest -- re-verifying the live head immediately before both the listing pass and each individual cancellation, so a delayed/stale invocation of this same cleanup job cannot itself wrongly cancel a still-authoritative run. + +Executable regressions: the embedded run-selection `jq` filter is extracted and executed against synthetic `workflow_runs` payloads (mirroring how `runtime_verdict()` already exercises the required-verdict filter), covering selection of a genuinely superseded older-head run, exclusion of a current-head run, exclusion of the cleanup job's own run, exclusion of a different PR, exclusion of a differently-named/triggered run, and matching via `pull_requests[]` metadata when `display_title` never rendered the head suffix; a structural test pins the job's `synchronize`-only trigger and `actions: write` permission. + ## Reconciliation The original branch diverged while unrelated protected-main repairs landed, including the Noema transport repair and the `graphql-core` security update. The branch is reconciled with current protected `main` through a normal two-parent merge commit; no force push or destructive rebase is used. Newer protected-main documentation is retained rather than replaced with stale branch copies. The concurrent review-event scheduler wake regression is retained in a dedicated regression file. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 66beee8906..4b098f9296 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -111,6 +111,129 @@ def test_runtime_required_verdict_rejects_other_actor() -> None: assert runtime_verdict([human]) == "" +def cleanup_candidate_run_ids( + runs: list[dict[str, object]], + *, + pr_number: str = "1437", + head_sha: str = HEAD, + repository: str = "ContextualWisdomLab/example", + current_run_id: str = "999", +) -> list[str]: + """Execute the jq program embedded in the superseded-run cleanup job.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup filter") + workflow = WORKFLOW.read_text(encoding="utf-8") + marker = ( + 'jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \\\n' + ' --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'' + ) + start = workflow.index(marker) + len(marker) + end = workflow.index("\n ' <<<\"$runs_json\")", start) + result = subprocess.run( + [ + jq, + "-r", + "--arg", + "pr", + pr_number, + "--arg", + "head_sha", + head_sha, + "--arg", + "repo", + repository, + "--arg", + "current", + current_run_id, + workflow[start:end], + ], + input=json.dumps({"workflow_runs": runs}), + text=True, + capture_output=True, + check=False, + ) + assert result.returncode == 0, result.stderr + return [line for line in result.stdout.splitlines() if line] + + +def _cleanup_run( + *, + run_id: int, + head_sha: str = HEAD, + name: str = "Required OpenCode Review", + event: str = "pull_request_target", + display_title: str | None = None, + pr_number: int = 1437, +) -> dict[str, object]: + """Build one synthetic workflow-run record for the cleanup filter.""" + title = ( + display_title + if display_title is not None + else f"Required OpenCode Review ContextualWisdomLab/example#{pr_number}@{head_sha}" + ) + return { + "id": run_id, + "name": name, + "event": event, + "display_title": title, + "pull_requests": [{"number": pr_number, "head": {"sha": head_sha}}], + } + + +def test_cleanup_selects_a_superseded_older_head_run() -> None: + """An older run for a different, no-longer-live head is selected.""" + stale = _cleanup_run(run_id=1, head_sha="b" * 40) + assert cleanup_candidate_run_ids([stale], current_run_id="999") == ["1"] + + +def test_cleanup_excludes_the_current_live_head_run() -> None: + """A run already on the live exact head is never selected.""" + current_head_run = _cleanup_run(run_id=1, head_sha=HEAD) + assert cleanup_candidate_run_ids([current_head_run], current_run_id="999") == [] + + +def test_cleanup_excludes_the_currently_executing_run_itself() -> None: + """The cleanup job's own run is never a cancellation candidate.""" + self_run = _cleanup_run(run_id=999, head_sha="b" * 40) + assert cleanup_candidate_run_ids([self_run], current_run_id="999") == [] + + +def test_cleanup_excludes_a_different_pull_request() -> None: + """A stale-head run for an unrelated PR is left untouched.""" + other_pr = _cleanup_run(run_id=1, head_sha="b" * 40, pr_number=9999) + assert cleanup_candidate_run_ids([other_pr], current_run_id="999") == [] + + +def test_cleanup_excludes_a_differently_named_or_triggered_run() -> None: + """A same-PR run for another workflow or trigger is left untouched.""" + other_workflow = _cleanup_run(run_id=1, head_sha="b" * 40, name="Strix Security Scan") + other_event = _cleanup_run(run_id=2, head_sha="b" * 40, event="workflow_dispatch") + assert ( + cleanup_candidate_run_ids([other_workflow, other_event], current_run_id="999") + == [] + ) + + +def test_cleanup_matches_by_pull_requests_metadata_when_title_omits_the_suffix() -> None: + """A run whose display_title never rendered the head suffix still resolves.""" + metadata_only = _cleanup_run( + run_id=1, head_sha="b" * 40, display_title="Required OpenCode Review" + ) + assert cleanup_candidate_run_ids([metadata_only], current_run_id="999") == ["1"] + + +def test_cleanup_job_is_scoped_to_synchronize_events_with_actions_write() -> None: + """The cleanup job only fires on synchronize and can cancel runs.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + job = workflow.split(" cancel-superseded-opencode-review-runs:\n", 1)[1] + assert ( + "if: github.event_name == 'pull_request_target' && " + "github.event.action == 'synchronize'" + ) in job + assert "actions: write" in job.split("steps:", 1)[0] + + def test_required_verdict_has_one_executable_owner() -> None: """Tests must execute the workflow gate, not a test-only Python mirror.""" status_source = STATUS_HELPER.read_text(encoding="utf-8") From f40f8debcf390b509d02a29e3737d69c863d25a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 11:12:39 +0000 Subject: [PATCH 16/16] fix(opencode-review): fix unbound $verdict crash in the superseded-run cleanup job Devin Review on #1568: the cleanup job added in the previous commit had a misplaced trailing line. The job append edit was anchored on the "Fail closed" step's own closing if/fi block, but that step's script actually had one more line after it -- echo "Current-head OpenCode verdict: ${verdict}." -- ending the file without a trailing newline, so wc -l undercounted it and a manual tail read (limit=2) stopped one line short. The new job's content landed between the fi and that trailing echo, pulling it into the cleanup job's own script, where $verdict is never set. Under set -euo pipefail (-u included), every synchronize event crashed with "verdict: unbound variable", failing the required workflow on every new commit -- reproduced directly by extracting and executing the job's script body against fake gh/jq stubs before this fix, and confirmed GREEN after. Restored the echo to its correct original position at the end of the "Fail closed" step, and gave the cleanup job its own closing message. Also adopted force-cancel as a fallback for a run that resists normal cancellation, matching strix.yml's own cancel-superseded-pr-runs job (Devin's accompanying informational finding). Verified: PYTHONPATH=. python -m pytest tests -> 2301 passed, 1 skipped, 21 subtests; YAML parses cleanly; the cleanup job's script body executed directly against fake gh/jq stubs -> exit 0, no unbound-variable error (reproduced the original crash against the pre-fix script first). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 8d275793b4..87827f5322 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -458,6 +458,7 @@ jobs: 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 fi + echo "Current-head OpenCode verdict: ${verdict}." cancel-superseded-opencode-review-runs: # Scoping the concurrency group above by exact head SHA (so a delayed @@ -541,7 +542,8 @@ jobs: echo "::notice::OpenCode review cleanup target changed before cancellation; leaving runs unchanged." return 0 fi - if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/opencode-cleanup-cancel-error; then + if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/opencode-cleanup-cancel-error || + gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/force-cancel" >/dev/null 2>>/tmp/opencode-cleanup-cancel-error; then echo "Cancelled superseded Required OpenCode Review run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." else echo "::warning::OpenCode review cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." @@ -553,4 +555,4 @@ jobs: for active_status in queued in_progress requested waiting pending; do cancel_runs "$active_status" done - echo "Current-head OpenCode verdict: ${verdict}." \ No newline at end of file + echo "Superseded OpenCode review run cleanup completed." \ No newline at end of file