From 271b269038be9f401da67345204f30e14cea7845 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 06:57:20 +0000 Subject: [PATCH] fix(ci): give the opencode-review required check a workflow_run second chance opencode-review.yml's opencode-review-target job (the "opencode-review" required check) evaluates synchronously inside its own pull_request_target run, seconds after push, by querying whether an opencode-agent review already exists for the current head. The real review is posted much later by the separate opencode-review-dispatch.yml repository_dispatch path, and nothing re-ran the already-failed check once it landed -- documented and left unfixed in #1485. Direction chosen: workflow_run re-entry (candidate 1 from #1485), but NOT a literal copy of noema-review.yml's own workflow_run source list. Tracing the actual execution model first: - opencode-review.yml is distributed by the org's required-workflow ruleset into every target repository (it runs "as" contextual-orchestrator, etc., not only as .github), so workflow_run re-entry has to work per repository. - opencode-review-dispatch.yml (the workflow that actually posts the review) is deliberately default-branch-only and always runs inside ContextualWisdomLab/.github -- it is NOT part of the required-workflow ruleset's distributed set. workflow_run cannot cross repositories, so a sibling repository's copy of opencode-review.yml could never observe its completion. Listening to "OpenCode Review Dispatch" (a literal mirror of noema's source list swapped in) would silently do nothing for every sibling-repo PR -- the majority of the affected PRs. - "Required PR Review Merge Scheduler" (pr-review-merge-scheduler.yml) IS ruleset-distributed into every target repository, and already reacts to pull_request_review: [submitted, dismissed] -- the exact event GitHub fires the instant the real opencode-agent review posts -- plus its own periodic sweep. Listening to its completion gives this required check a same-repository second chance without needing pull_request_review as a direct trigger (not supported by the required-workflow ruleset's trigger set) and without any cross-repository credential. Change, kept as close to the existing pull_request_target shape as possible: - on.workflow_run listens for "Required PR Review Merge Scheduler" completions (excluding cancelled runs, matching noema-review.yml's own exclusion). - required-workflow-bootstrap gains `if: github.event_name == 'pull_request_target'`; it has no meaningful inputs for a workflow_run event, and coverage-source-tree / coverage-evidence cascade-skip with it (no changes needed there). - opencode-review-target now runs standalone on a workflow_run event (needs.coverage-evidence.result == 'success' is only required on the pull_request_target path, preserving that path's exact prior behavior), resolving PR_NUMBER/HEAD_SHA from github.event.workflow_run.pull_requests[0] (GitHub-computed, not attacker-controlled) when github.event.pull_request is absent, with a new early exit for workflow_run events that carry no associated PR (the periodic-sweep case). - concurrency.group and run-name gain the same PR-number/head-SHA fallback chain for workflow_run events. Security posture, unchanged: no checkout, no secrets, no new permissions. The verdict step still only performs a read via `gh api .../pulls/{PR_NUMBER}/reviews` with the existing read-only github.token, gated by the same opencode-agent/current-head/non-fallback checks as before -- this only changes when that exact check gets a chance to re-run, never what it accepts as passing. New tests in test_opencode_required_verdict_regression.py lock in: the workflow_run trigger source and its #1485 rationale, the bootstrap job's pull_request_target-only gate, the target job's workflow_run re-entry condition, and three executable bash runs of the actual verdict step (no associated PR exits cleanly without calling gh; a workflow_run re-entry with a real review now present passes; the original pull_request_target fail-closed behavior is unchanged). Full suite: 2123 passed, 1 skipped, 21 subtests. Coverage 100% (scripts/ci), interrogate 100%. Refs #1485. Once merged, ContextualWisdomLab/.github#1492 and ContextualWisdomLab/contextual-orchestrator#955/#956 (this session's PRs that hit this exact race) are expected to self-resolve on their next opencode-review re-run. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- .github/workflows/opencode-review.yml | 50 +++- ...st_opencode_required_verdict_regression.py | 218 ++++++++++++++++++ 2 files changed, 263 insertions(+), 5 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index d66979d406..8717166353 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -1,8 +1,10 @@ name: Required OpenCode Review run-name: >- Required OpenCode Review ${{ github.event.pull_request.base.repo.full_name || - github.repository }}#${{ github.event.pull_request.number || 'event' }}@${{ - github.event.pull_request.head.sha || github.sha }} + github.repository }}#${{ github.event.pull_request.number || + github.event.workflow_run.pull_requests[0].number || 'event' }}@${{ + github.event.pull_request.head.sha || + github.event.workflow_run.pull_requests[0].head.sha || github.sha }} on: # This required-workflow entrypoint never checks out or executes pull-request @@ -10,12 +12,29 @@ on: # isolated in opencode-review-dispatch.yml on repository_dispatch only. pull_request_target: types: [opened, synchronize, reopened, ready_for_review, closed] + # opencode-review-dispatch.yml (the workflow that actually posts the real + # opencode-agent review) is default-branch-only and, unlike this file, is + # not distributed by the required-workflow ruleset into sibling repositories + # -- it always runs inside ContextualWisdomLab/.github, so its completion + # can never reach a sibling repository's copy of this file through + # workflow_run (workflow_run cannot cross repositories). "Required PR + # Review Merge Scheduler" IS ruleset-distributed into every target + # repository, already reacts to pull_request_review (submitted/dismissed -- + # the exact event GitHub fires the instant the real review posts) plus its + # own periodic sweep, so listening to its completion gives this required + # check a same-repository second chance once real evidence can exist, + # without checking out or executing any pull-request content. See + # ContextualWisdomLab/.github#1485. + workflow_run: + workflows: ["Required PR Review Merge Scheduler"] + types: [completed] concurrency: 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.event.workflow_run.pull_requests[0].number || + github.run_id }} cancel-in-progress: true permissions: @@ -26,6 +45,12 @@ jobs: required-workflow-bootstrap: name: required-workflow-bootstrap runs-on: ubuntu-latest + # Pingora edge-policy enforcement below only has meaningful inputs (PR + # number, head SHA, event action) on the pull_request_target path; a + # workflow_run second-chance re-entry skips this whole sentinel chain + # (coverage-source-tree and coverage-evidence cascade-skip the same way) + # and opencode-review-target below runs standalone instead. + if: github.event_name == 'pull_request_target' steps: - name: Materialize the required review workflow run: >- @@ -234,6 +259,17 @@ jobs: name: opencode-review needs: [coverage-evidence] runs-on: ubuntu-latest + # pull_request_target still requires the coverage-evidence sentinel chain + # above to have completed successfully, exactly as before. A workflow_run + # second-chance re-entry has no such chain (required-workflow-bootstrap is + # skipped for that event, cascade-skipping coverage-source-tree and + # coverage-evidence too) and evaluates independently instead; a cancelled + # source workflow_run carries no new evidence and is skipped rather than + # re-evaluated. + if: >- + always() && + github.event.workflow_run.conclusion != 'cancelled' && + (github.event_name == 'workflow_run' || needs.coverage-evidence.result == 'success') permissions: contents: read pull-requests: read @@ -242,14 +278,18 @@ jobs: env: GH_TOKEN: ${{ github.token }} TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha || github.event.workflow_run.pull_requests[0].head.sha }} run: | set -euo pipefail if [ "${{ github.event.action }}" = "closed" ]; then echo "PR closed; a current-head OpenCode verdict is not required." exit 0 fi + if [ "${{ github.event_name }}" = "workflow_run" ] && [ -z "${PR_NUMBER:-}" ]; then + echo "No pull request is associated with this workflow_run event; nothing to verify." + 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/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 881e93c2ef..3659462d46 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import os import shutil import subprocess from pathlib import Path @@ -13,6 +14,7 @@ HEAD = "a" * 40 WORKFLOW = Path(".github/workflows/opencode-review.yml") STATUS_HELPER = Path("scripts/ci/opencode_dispatch_status.py") +STEP_NAME = "Fail closed without a current-head OpenCode verdict" def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, object]: @@ -106,3 +108,219 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non "Review approval remains a separate current-head PR review requirement" not in workflow ) + + +def _extract_run_block(workflow_text: str, step_name: str) -> str: + """Return the raw shell body of one named workflow step.""" + lines = workflow_text.splitlines() + step_index = next( + index for index, line in enumerate(lines) if line.strip() == f"- name: {step_name}" + ) + run_index = next( + index + for index in range(step_index + 1, len(lines)) + if lines[index].strip() == "run: |" + ) + run_indent = len(lines[run_index]) - len(lines[run_index].lstrip()) + block_lines = [] + for line in lines[run_index + 1 :]: + if line.strip() and len(line) - len(line.lstrip()) <= run_indent: + break + block_lines.append(line[run_indent + 2 :] if len(line) >= run_indent + 2 else "") + return "\n".join(block_lines) + "\n" + + +def _rendered_verdict_script(*, event_name: str, event_action: str = "") -> str: + """Return the verdict step's shell body with its inline expressions rendered. + + GitHub Actions substitutes ``${{ ... }}`` expressions directly into the + script text before any shell ever runs it, so exercising this step + outside Actions requires performing that same literal substitution -- + ``PR_NUMBER``/``HEAD_SHA``/``TARGET_REPOSITORY`` remain real environment + variables (set by the step's own ``env:`` block, not text substitution) + and are supplied by the caller through the subprocess environment + instead. + """ + script = _extract_run_block(WORKFLOW.read_text(encoding="utf-8"), STEP_NAME) + return script.replace("${{ github.event.action }}", event_action).replace( + "${{ github.event_name }}", event_name + ) + + +def test_workflow_run_trigger_gives_a_same_repository_second_chance() -> None: + """opencode-review.yml re-enters after "Required PR Review Merge Scheduler" completes. + + See ContextualWisdomLab/.github#1485: opencode-review-dispatch.yml (which + posts the real review) always runs inside ContextualWisdomLab/.github and + is not distributed by the required-workflow ruleset into sibling + repositories, so workflow_run -- which cannot cross repositories -- could + never observe its completion from a sibling repository's copy of this + file. "Required PR Review Merge Scheduler" IS ruleset-distributed into + every target repository and reacts to pull_request_review immediately + when the real review posts, so it is the source workflow this file + listens to instead. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + on_block = workflow.split("concurrency:", 1)[0] + assert "workflow_run:" in on_block + assert 'workflows: ["Required PR Review Merge Scheduler"]' in on_block + assert "types: [completed]" in on_block + assert '"OpenCode Review Dispatch"' not in on_block + assert "ContextualWisdomLab/.github#1485" in on_block + assert ( + "github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number" + in workflow + ) + + +def test_required_workflow_bootstrap_is_pull_request_target_only() -> None: + """The Pingora bootstrap chain stays scoped to the pull_request_target path. + + A workflow_run second-chance re-entry has no ``github.event.pull_request`` + context for the Pingora policy inputs, so the bootstrap chain + (required-workflow-bootstrap, coverage-source-tree, coverage-evidence) + must cascade-skip for that event rather than run with wrong inputs. + """ + workflow = WORKFLOW.read_text(encoding="utf-8") + bootstrap_job = workflow.split("required-workflow-bootstrap:\n", 1)[1].split( + "\n coverage-source-tree:", 1 + )[0] + assert "if: github.event_name == 'pull_request_target'" in bootstrap_job + # Regression guard against the redundant ${{ }}-wrapped style this repo + # avoids for job-level `if:` conditions. + assert "if: ${{ github.event_name == 'pull_request_target' }}" not in workflow + + +def test_opencode_review_target_permits_workflow_run_reentry() -> None: + """The required check job runs standalone for a non-cancelled workflow_run event.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + target_job = workflow.split("\n opencode-review-target:\n", 1)[1] + condition = target_job.split("permissions:", 1)[0] + assert "always()" in condition + assert "github.event.workflow_run.conclusion != 'cancelled'" in condition + assert "github.event_name == 'workflow_run'" in condition + assert "needs.coverage-evidence.result == 'success'" in condition + + +def test_verdict_step_skips_cleanly_when_workflow_run_has_no_pull_request( + tmp_path: Path, +) -> None: + """A scheduler completion with no associated PR exits 0 without calling gh. + + This covers the periodic-sweep-triggered "Required PR Review Merge + Scheduler" completions, whose workflow_run event carries no + ``pull_requests`` entry -- there is nothing to verify, and it must not be + treated as the original "missing PR number" fail-closed case. + """ + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is required to execute the production verdict step") + script = _rendered_verdict_script(event_name="workflow_run") + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + fake_gh = fake_bin / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\necho 'gh must not be invoked here' >&2\nexit 1\n", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "GH_TOKEN": "token", + "TARGET_REPOSITORY": "ContextualWisdomLab/contextual-orchestrator", + "PR_NUMBER": "", + "HEAD_SHA": "", + } + result = subprocess.run( + ["bash"], input=script, text=True, capture_output=True, env=env, check=False + ) + assert result.returncode == 0, result.stderr + assert "No pull request is associated with this workflow_run event" in result.stdout + + +def test_verdict_step_workflow_run_reentry_passes_once_the_real_review_landed( + tmp_path: Path, +) -> None: + """A workflow_run re-entry succeeds once the async dispatch posted a real verdict. + + This is the actual race fix: opencode-review-dispatch.yml posts the real + opencode-agent review well after the original pull_request_target run of + this job already failed; this re-entry, triggered once "Required PR + Review Merge Scheduler" reacts to that same review landing + (pull_request_review: submitted), re-evaluates the identical Reviews API + check and now finds 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 verdict step") + script = _rendered_verdict_script(event_name="workflow_run") + reviews_json = json.dumps([review(state="APPROVED")]) + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + fake_gh = fake_bin / "gh" + fake_gh.write_text( + f"""#!/usr/bin/env bash +set -euo pipefail +test "$1" = api +test "$2" = --paginate +test "$3" = "repos/ContextualWisdomLab/contextual-orchestrator/pulls/955/reviews" +printf '%s\\n' '{reviews_json}' +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "GH_TOKEN": "token", + "TARGET_REPOSITORY": "ContextualWisdomLab/contextual-orchestrator", + "PR_NUMBER": "955", + "HEAD_SHA": HEAD, + } + result = subprocess.run( + ["bash"], input=script, text=True, capture_output=True, env=env, check=False + ) + assert result.returncode == 0, result.stderr + assert "Current-head OpenCode verdict: APPROVED." in result.stdout + + +def test_verdict_step_pull_request_target_still_fails_closed_without_a_review( + tmp_path: Path, +) -> None: + """The original synchronous pull_request_target race behavior is unchanged. + + Regression guard: adding the workflow_run re-entry must not weaken the + original required check's fail-closed behavior on its own first, + synchronous evaluation. + """ + 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 verdict step") + script = _rendered_verdict_script(event_name="pull_request_target") + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + fake_gh = fake_bin / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\nset -euo pipefail\nprintf '%s\\n' '[]'\n", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "GH_TOKEN": "token", + "TARGET_REPOSITORY": "ContextualWisdomLab/.github", + "PR_NUMBER": "1492", + "HEAD_SHA": HEAD, + } + result = subprocess.run( + ["bash"], input=script, text=True, capture_output=True, env=env, check=False + ) + assert result.returncode == 1 + assert ( + "No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head" + in result.stdout + )