diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 83f6830d5..59b84b3e5 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -179,13 +179,13 @@ jobs: if [ "$live_state" != "open" ] || [ "$live_base_repository" != "$TARGET_REPOSITORY" ] || - [ "$live_head_repository" != "$TARGET_REPOSITORY" ] || + ! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || ! [[ "$live_base_sha" =~ ^[0-9a-fA-F]{40}$ ]] || ! [[ "$live_head_sha" =~ ^[0-9a-fA-F]{40}$ ]] || ! [[ "$live_is_private" =~ ^(true|false)$ ]] || [ -z "$live_base_ref" ] || [ -z "$live_head_ref" ]; then - printf '::error::PR metadata validation rejected closed, missing, cross-repository, or malformed live metadata. target=%s#%s state=%s base_repo=%s head_repo=%s base=%s head=%s\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_sha:-}" "${live_head_sha:-}" + printf '::error::PR metadata validation rejected closed, missing, malformed, or target-base-mismatched live metadata. target=%s#%s state=%s base_repo=%s head_repo=%s base=%s head=%s\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_sha:-}" "${live_head_sha:-}" exit 1 fi @@ -333,8 +333,44 @@ jobs: git -C "$fetch_dir" remote add origin "${GITHUB_SERVER_URL}/${TARGET_REPOSITORY}.git" if ! git -C "$fetch_dir" \ -c http.extraheader="AUTHORIZATION: basic ${auth_header}" \ - fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA" "$PR_HEAD_SHA"; then - echo "::error::Coverage fetch could not authenticate to ${TARGET_REPOSITORY} or read base/head SHAs ${PR_BASE_SHA}/${PR_HEAD_SHA}; check token permissions, target repository access, and SHA visibility." + fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA"; then + echo "::error::Coverage fetch could not authenticate to ${TARGET_REPOSITORY} or read base SHA ${PR_BASE_SHA}; check token permissions, target repository access, and SHA visibility." + exit 1 + fi + head_fetch_succeeded=0 + if git -C "$fetch_dir" \ + -c http.extraheader="AUTHORIZATION: basic ${auth_header}" \ + fetch --no-tags --prune --no-recurse-submodules origin "$PR_HEAD_SHA"; then + head_fetch_succeeded=1 + else + echo "Direct head-SHA coverage fetch was unavailable; resolving the exact head through the target pull request ref." + fi + if [ "$head_fetch_succeeded" -ne 1 ]; then + for pr_head_fetch_attempt in 1 2 3 4 5 6; do + if git -C "$fetch_dir" \ + -c http.extraheader="AUTHORIZATION: basic ${auth_header}" \ + fetch --no-tags --prune --no-recurse-submodules origin \ + "+refs/pull/${PR_NUMBER}/head:refs/remotes/origin/pr-${PR_NUMBER}-head"; then + fetched_head_sha="$(git -C "$fetch_dir" rev-parse "refs/remotes/origin/pr-${PR_NUMBER}-head")" + if [ "$fetched_head_sha" = "$PR_HEAD_SHA" ]; then + head_fetch_succeeded=1 + break + fi + if [ "$pr_head_fetch_attempt" -lt 6 ]; then + echo "Fetched PR head $fetched_head_sha, expected $PR_HEAD_SHA; retrying after propagation delay." + sleep 10 + else + echo "Fetched PR head $fetched_head_sha, expected $PR_HEAD_SHA; no retries remain." + fi + elif [ "$pr_head_fetch_attempt" -lt 6 ]; then + echo "PR head ref fetch failed on attempt $pr_head_fetch_attempt; retrying after propagation delay." + sleep 10 + fi + done + fi + if [ "$head_fetch_succeeded" -ne 1 ] || + ! git -C "$fetch_dir" cat-file -e "${PR_HEAD_SHA}^{commit}"; then + echo "::error::Coverage fetch could not resolve exact head SHA ${PR_HEAD_SHA} from ${TARGET_REPOSITORY} or refs/pull/${PR_NUMBER}/head; check token permissions, target repository access, PR state, and SHA propagation." exit 1 fi git -C "$fetch_dir" checkout --detach "$PR_BASE_SHA" @@ -2146,7 +2182,7 @@ jobs: live_is_private="$(jq -r '.base.repo.private | tostring' <<<"$pull_request_json")" if [ "$live_state" != "open" ] || [ "$base_repository" != "$GH_REPOSITORY" ] || - [ "$head_repository" != "$GH_REPOSITORY" ] || + ! [[ "$head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || [ "$live_base_ref" != "$EXPECTED_BASE_REF" ] || [ "$live_base_sha" != "$EXPECTED_BASE_SHA" ] || [ "$live_head_ref" != "$EXPECTED_HEAD_REF" ] || @@ -2158,7 +2194,7 @@ jobs: "$GH_REPOSITORY" "$PR_NUMBER" "${live_state:-}" "${base_repository:-}" "${live_base_ref:-}" "${live_base_sha:-}" "$EXPECTED_BASE_REF" "$EXPECTED_BASE_SHA" "${head_repository:-}" "${live_head_ref:-}" "${live_head_sha:-}" "$EXPECTED_HEAD_REF" "$EXPECTED_HEAD_SHA" "${live_is_private:-}" "${EXPECTED_IS_PRIVATE:-}" exit 1 fi - printf 'Validated same-repository OpenCode review source for %s#%s (%s).\n' \ + printf 'Validated current-head OpenCode review source for %s#%s (head=%s; this path is review-only for external heads).\n' \ "$GH_REPOSITORY" "$PR_NUMBER" "$head_repository" - name: Exchange OpenCode app token for target repository review reads diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 7f1ad6d00..bd074f7ae 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -20,6 +20,7 @@ concurrency: permissions: contents: read + pull-requests: read jobs: required-workflow-bootstrap: @@ -53,7 +54,56 @@ jobs: name: opencode-review needs: [coverage-evidence] runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read steps: - - run: >- - echo "Review approval remains a separate current-head PR review - requirement produced by the authenticated dispatch workflow." + - name: Fail closed without a current-head OpenCode verdict + 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 }} + 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 [ -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 + reviews="$(gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")" + verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$HEAD_SHA" ' + (add // []) + | [ + .[] + | select( + (.user.login // "" | ascii_downcase) as $user + | $user == "opencode-agent" or $user == "opencode-agent[bot]" + ) + | select((.commit_id // "" | ascii_downcase) == ($sha | ascii_downcase)) + ] + | (last // {}) as $review + | ($review.body // "" | ascii_downcase) as $body + | if $review.state == "CHANGES_REQUESTED" then + "CHANGES_REQUESTED" + elif $review.state == "APPROVED" + and ($body | contains("deterministic current-head evidence") | not) + and ($body | contains("deterministic fallback approval") | not) + and ($body | contains("model-unavailable evidence fallback") | not) + and ($body | contains("did not emit a usable current-head control block") | not) + and ($body | contains("scope: `unsupported`") | not) + and ($body | contains("model-pool outcome: `unknown`") | not) + then + "APPROVED" + else + empty + end + ')" + if [ -z "$verdict" ]; then + echo "::error::No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head. This required check is not a review and must not succeed until the authenticated dispatch posts a current-head verdict." + exit 1 + fi + echo "Current-head OpenCode verdict: ${verdict}." diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 583d7af16..cff63a658 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -246,6 +246,7 @@ jobs: { printf 'repository=%s\n' "$GITHUB_REPOSITORY" printf 'base_branch=%s\n' "$DEFAULT_BRANCH" + printf 'default_branch=%s\n' "$DEFAULT_BRANCH" } >>"$GITHUB_OUTPUT" exit 0 fi @@ -277,21 +278,26 @@ jobs: fi pull_json="$(gh api "repos/${TARGET_REPOSITORY_INPUT}/pulls/${TARGET_PR_NUMBER}")" + repository_json="$(gh api "repos/${TARGET_REPOSITORY_INPUT}")" live_number="$(jq -r '.number // 0' <<<"$pull_json")" live_state="$(jq -r '.state // empty' <<<"$pull_json")" live_base_repository="$(jq -r '.base.repo.full_name // empty' <<<"$pull_json")" live_head_repository="$(jq -r '.head.repo.full_name // empty' <<<"$pull_json")" live_base_branch="$(jq -r '.base.ref // empty' <<<"$pull_json")" + live_default_branch="$(jq -r '.default_branch // empty' <<<"$repository_json")" live_head_sha="$(jq -r '.head.sha // empty' <<<"$pull_json")" if [ "$live_number" != "$TARGET_PR_NUMBER" ] || [ "$live_state" != "open" ] || [ "$live_base_repository" != "$TARGET_REPOSITORY_INPUT" ] || - [ "$live_head_repository" != "$TARGET_REPOSITORY_INPUT" ] || + ! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || [ -z "$live_base_branch" ] || + [ -z "$live_default_branch" ] || ! [[ "$live_head_sha" =~ ^[0-9a-fA-F]{40}$ ]]; then - printf '::error::Targeted scheduler dispatch rejected closed, cross-repository, or malformed live PR metadata. target=%s pr=%s state=%s base_repository=%s head_repository=%s base_branch=%s head_sha=%s\n' "$TARGET_REPOSITORY_INPUT" "$TARGET_PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_branch:-}" "${live_head_sha:-}" + printf '::error::Targeted scheduler dispatch rejected closed, malformed, or target-base-mismatched live metadata. target=%s pr=%s state=%s base_repository=%s head_repository=%s base_branch=%s default_branch=%s head_sha=%s\n' "$TARGET_REPOSITORY_INPUT" "$TARGET_PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_branch:-}" "${live_default_branch:-}" "${live_head_sha:-}" exit 1 fi + printf 'Validated targeted scheduler review dispatch for %s#%s (head=%s; this dispatch is review-only for external heads).\n' \ + "$TARGET_REPOSITORY_INPUT" "$TARGET_PR_NUMBER" "$live_head_repository" if [ -n "$TARGET_BASE_BRANCH_INPUT" ] && [ "$TARGET_BASE_BRANCH_INPUT" != "$live_base_branch" ]; then printf '::error::Targeted scheduler dispatch base branch does not match the live PR. supplied=%s live=%s\n' "$TARGET_BASE_BRANCH_INPUT" "$live_base_branch" @@ -301,9 +307,10 @@ jobs: { printf 'repository=%s\n' "$TARGET_REPOSITORY_INPUT" printf 'base_branch=%s\n' "$live_base_branch" + printf 'default_branch=%s\n' "$live_default_branch" printf 'head_sha=%s\n' "$live_head_sha" } >>"$GITHUB_OUTPUT" - printf 'Validated exact targeted scheduler dispatch for %s#%s at %s on base %s.\n' "$TARGET_REPOSITORY_INPUT" "$TARGET_PR_NUMBER" "$live_head_sha" "$live_base_branch" + printf 'Validated exact targeted scheduler dispatch for %s#%s at %s on base %s; repository default is %s.\n' "$TARGET_REPOSITORY_INPUT" "$TARGET_PR_NUMBER" "$live_head_sha" "$live_base_branch" "$live_default_branch" - name: Resolve trusted scheduler source ref id: trusted_source @@ -484,7 +491,7 @@ jobs: env: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.scheduler_app_token.outputs.token || github.token }} TARGET_REPOSITORY: ${{ steps.targeted_dispatch.outputs.repository }} - TARGET_DEFAULT_BRANCH: ${{ steps.targeted_dispatch.outputs.base_branch }} + TARGET_DEFAULT_BRANCH: ${{ steps.targeted_dispatch.outputs.default_branch }} SCHEDULER_ACTIONS_TOKEN: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.target_repository != '' && (secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.scheduler_app_token.outputs.token) || github.token }} # Same-repository dispatch credential: when this scheduler runs inside # ContextualWisdomLab/.github (the repository the required workflows are diff --git a/CHANGELOG.md b/CHANGELOG.md index 870b34488..5f186b232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,13 @@ Semantic Versioning where the repository publishes a release. ### Changed +- Noema now validates the current-head primary OpenCode approval before accepting an existing Noema verdict, preventing a secondary review from making the required gate look successful on its own. +- Draft pull requests now receive same-head Strix and OpenCode review dispatches while remaining excluded from branch updates, auto-merge changes, direct merge, and review-state cleanup. +- The required `opencode-review` check now fails closed unless `opencode-agent` already posted `APPROVED` or `CHANGES_REQUESTED` on the current head, so a stub success can no longer look like a review (ContextualWisdomLab/contextual-orchestrator#176). +- The merge scheduler now spends its review-dispatch budget on pull requests with no OpenCode verdict on any commit before leftover increments that already have a previous-head APPROVED or CHANGES_REQUESTED, so one-dispatch-per-run no longer starves an empty Reviews tab. +- The scheduler treats GitHub's full `run-name` (`OpenCode Review Dispatch owner/repo#N@sha`) as an in-progress same-head dispatch, so a later sweep cannot `cancel-in-progress` a review that already passed coverage. +- Noema no longer exits 0 when the current head has no primary OpenCode approval, including on draft pull requests; that skip was the green `noema-review` check with an empty Reviews tab. +- `load_codegraph_context` now confines `NOEMA_CODEGRAPH_CONTEXT_PATH` to `GITHUB_WORKSPACE` (or cwd) with `..` rejection and realpath checks, so a Strix path-traversal report on that helper cannot read files outside the review workspace. - Deduplicate central `workflow_run` scheduler scans: metadata-free workflow-run events now cancel an older run in a workflow-run-specific branch fallback, and the organization queue sweep retains only the newest metadata-free scheduler scan for the current default-branch HEAD while preserving push, PR-associated, and unrelated workflow runs. - Require the hourly repair worker to establish an exact-head root cause, enumerate the smallest remediation candidates, and prove writer authority, sealed-path scope, credentials, dependency order, verifiability, and causal effect before editing; infeasible or external blockers leave the tree unchanged while the broader loop continues with another eligible PR or buyer-visible product gap. - Run the bounded Quarantine Sandbox Runtime heartbeat at minute 14 without granting the caller model secrets, repository mutation permissions, approval, merge, release, artifact-execution, or final security-verdict authority. diff --git a/docs/doctoring/required-review-check-is-not-a-verdict.md b/docs/doctoring/required-review-check-is-not-a-verdict.md new file mode 100644 index 000000000..624b300ef --- /dev/null +++ b/docs/doctoring/required-review-check-is-not-a-verdict.md @@ -0,0 +1,93 @@ +# Required OpenCode/Noema checks are not reviews + +검토 기준일: **2026-08-14** + +## Incident + +On ContextualWisdomLab/contextual-orchestrator#176 the required +`opencode-review` and `noema-review` checks were green, but the Reviews +API had no APPROVE or REQUEST_CHANGES. Authors treated the check name as +a review verdict (GitHub, n.d.-a). That is weaker than the modern-review +expectation that a review is an explicit, current-head judgment +(Bacchelli & Bird, 2013). + +## Decision + +The required `opencode-review` job on +`.github/workflows/opencode-review.yml` never runs the model. Privileged +review stays in `opencode-review-dispatch.yml`. The required job now +reads current-head reviews with `pull-requests: read` and **fails +closed** unless `opencode-agent` / `opencode-agent[bot]` already posted +`APPROVED` or `CHANGES_REQUESTED` on that SHA. A COMMENTED review, a +review on an old SHA, or no review at all cannot make the check green. + +`scripts/ci/noema_review_gate.py` no longer returns 0 when the current +head has no primary OpenCode approval. That skip was exit 0, so the +required `noema-review` check looked like a successful review. Draft +status is checked only after that primary-approval gate, so a draft +without an OpenCode verdict cannot turn `noema-review` green. The gate +also validates the primary approval before accepting an existing Noema +review; a secondary verdict cannot independently turn the required gate +green. + +Human `repository_dispatch` as `seonghobae` remains rejected; only +`github-actions[bot]` may start the privileged dispatch. After a real +verdict is posted, re-run the required `opencode-review` job so the +fail-closed check can observe it. + +The one-dispatch-per-run budget used to walk pull requests in created-at +order, so leftover increments that already had a previous-head verdict +consumed the slot while a later PR with an empty Reviews tab waited. The +scheduler now stable-sorts that budget: no OpenCode APPROVED or +CHANGES_REQUESTED on any commit first, then previous-head re-reviews, +then current-head verdicts. COMMENTED-only evidence is not a verdict and +keeps the empty-Reviews priority. + +A second same-head `repository_dispatch` used to cancel the first through +workflow `cancel-in-progress` because `active_review_run_refs` compared +the GitHub `name` field to the short alias `OpenCode Review Dispatch`. +Live runs set `name` to the interpolated run-name. The matcher now +accepts that prefix so a queued or in-progress same-head review is +`already_running`. + +## Draft pull-request review contract + +Draft status is a merge-readiness signal, not a request to suppress early +feedback. The central scheduler therefore dispatches same-head Strix first +and then authenticated OpenCode review for draft pull requests. The draft +path is deliberately review-only: it cannot update the head branch, enable +or disable auto-merge, merge, dismiss reviews, or resolve review threads. +Marking a pull request ready remains the explicit boundary for merge +automation. + +## Verification contract + +- `tests/test_opencode_required_verdict_gate.py` pins + `current_head_opencode_verdict` and `decide_required_verdict_check`. +- `tests/test_noema_review_gate.py` requires exit 1 when there is no + primary OpenCode approval, even when the current head already has a Noema + review, while retaining the idempotent success path after a valid primary + approval exists. +- `tests/test_opencode_agent_contract.py` pins the required workflow + fail-closed error string. +- `tests/test_pr_review_merge_scheduler.py` proves that a draft pull request + receives same-head Strix and OpenCode dispatch while branch updates, + auto-merge mutation, direct merge, review dismissal, and thread cleanup + remain unreachable, and that a never-reviewed pull request consumes the + one-dispatch budget before a leftover increment that already has a + previous-head OpenCode verdict. +- Both repairs were exercised test-first: the draft-dispatch contract failed + against the old unconditional skip, and the Noema ordering contract failed + against the old secondary-review-first branch. The exact repaired source + then passed 988 tests, 7,056 production statements, 2,834 production + branches, and the public-docstring gate at 100%. + +## References (APA 7th) + +Bacchelli, A., & Bird, C. (2013). Expectations, outcomes, and challenges of +modern code review. In *Proceedings of the 35th International Conference on +Software Engineering* (pp. 712–721). IEEE. +https://doi.org/10.1109/ICSE.2013.6606617 + +GitHub. (n.d.-a). *About status checks*. GitHub Docs. Retrieved +August 14, 2026, from https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks \ No newline at end of file diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 9317860e4..01ab88268 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -16,6 +16,7 @@ import urllib.parse import urllib.request from collections.abc import Sequence +from pathlib import Path from typing import Any @@ -380,13 +381,38 @@ def review_thread_context(pr: dict[str, Any]) -> str: return "\n".join(lines) +def codegraph_context_root() -> Path: + """Return the workspace root that may contain CodeGraph context files.""" + raw = os.environ.get("GITHUB_WORKSPACE", "").strip() or os.getcwd() + return Path(raw).resolve() + + +def confined_codegraph_context_path(path: str, root: Path) -> Path | None: + """Return the resolved path when it cannot escape the workspace root.""" + candidate = Path(path) + if ".." in candidate.parts: + return None + if not candidate.is_absolute(): + candidate = Path(root / candidate) + try: + resolved = candidate.resolve() + except OSError: + return None + if not resolved.is_relative_to(root): + return None + return resolved + + def load_codegraph_context() -> str: """Load optional precomputed CodeGraph context for structural review evidence.""" path = os.environ.get("NOEMA_CODEGRAPH_CONTEXT_PATH", "").strip() if not path: return "" + confined = confined_codegraph_context_path(path, codegraph_context_root()) + if confined is None: + return "CodeGraph context unavailable: path escapes the workspace." try: - with open(path, encoding="utf-8") as handle: + with confined.open(encoding="utf-8") as handle: return truncate_text(handle.read(), MAX_REVIEW_CONTEXT_CHARS) except OSError as exc: return f"CodeGraph context unavailable: {exc}" @@ -582,7 +608,12 @@ def submit_review(repo: str, number: int, pr: dict[str, Any], actor: str, verdic def inspect_and_review(repo: str, number: int) -> int: - """Inspect PR state and submit Noema's LLM review when gates are clean.""" + """Inspect PR state and submit Noema's LLM review when gates are clean. + + Missing current-head primary OpenCode approval fails closed, including + on draft pull requests, so the required check cannot look reviewed + without a Reviews-tab verdict. + """ pr = fetch_pr(repo, number) actor = current_actor() if actor in PRIMARY_REVIEW_AUTHORS: @@ -591,15 +622,19 @@ def inspect_and_review(repo: str, number: int) -> int: "Noema review skipped so GitHub receives an independent reviewer." ) return 0 + if not current_primary_approval(pr): + print( + "Current head does not have a primary OpenCode approval; " + "Noema cannot skip as success because that made the required " + "check look like a review." + ) + return 1 if pr.get("isDraft"): - print("PR is draft; Noema review skipped.") + print("PR is draft; Noema review skipped after primary OpenCode approval.") return 0 if existing_noema_review(pr, actor): print("Current head already has a Noema review; nothing to do.") return 0 - if not current_primary_approval(pr): - print("Current head does not have a primary OpenCode approval; Noema review skipped.") - return 0 if has_current_changes_requested(pr): print("Current head has requested changes; Noema review skipped.") return 0 diff --git a/scripts/ci/opencode_dispatch_status.py b/scripts/ci/opencode_dispatch_status.py index 9109a0248..9224361ff 100644 --- a/scripts/ci/opencode_dispatch_status.py +++ b/scripts/ci/opencode_dispatch_status.py @@ -10,16 +10,74 @@ try: from opencode_existing_approval_gate import ( + FALLBACK_MARKERS, OPENCODE_APP_APPROVAL_AUTHORS, review_rejection_reason, ) except ModuleNotFoundError: # pragma: no cover - package import path from scripts.ci.opencode_existing_approval_gate import ( + FALLBACK_MARKERS, OPENCODE_APP_APPROVAL_AUTHORS, review_rejection_reason, ) +OPENCODE_VERDICT_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED"}) +MISSING_VERDICT_MESSAGE = ( + "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." +) + + +def current_head_opencode_verdict( + reviews: Sequence[dict[str, Any]], head_sha: str +) -> str | None: + """Return the latest substantive current-head OpenCode verdict, if any.""" + expected = (head_sha or "").lower() + if not expected: + return None + for review in reversed(reviews): + author = str((review.get("user") or {}).get("login") or "").casefold() + if author not in OPENCODE_APP_APPROVAL_AUTHORS: + continue + if str(review.get("commit_id") or "").lower() != expected: + continue + state = str(review.get("state") or "").upper() + if state not in OPENCODE_VERDICT_STATES: + return None + body = str(review.get("body") or "").casefold() + if state == "APPROVED" and any(marker in body for marker in FALLBACK_MARKERS): + return None + return state + return None + + +def decide_required_verdict_check( + *, + expected_head: str, + pull_request: dict[str, Any], + reviews: Sequence[dict[str, Any]], +) -> dict[str, str]: + """Fail closed unless OpenCode already published a current-head verdict.""" + live_head = str((pull_request.get("head") or {}).get("sha") or "") + if not expected_head or live_head.lower() != expected_head.lower(): + return { + "state": "failure", + "description": ( + "OpenCode required-check target is stale or the live PR head " + "is unavailable." + ), + } + verdict = current_head_opencode_verdict(reviews, expected_head) + if verdict is None: + return {"state": "failure", "description": MISSING_VERDICT_MESSAGE} + return { + "state": "success", + "description": f"Current-head OpenCode verdict: {verdict}.", + } + + def _has_current_approval(reviews: Sequence[dict[str, Any]], head_sha: str) -> bool: """Return whether the latest OpenCode decision is a verified approval.""" for review in reversed(reviews): @@ -67,10 +125,15 @@ def decide_status( def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: - """Parse commit-status evidence inputs.""" + """Parse commit-status or required-verdict evidence inputs.""" parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--model-outcome", required=True) - parser.add_argument("--coverage-result", required=True) + parser.add_argument( + "--mode", + choices=("dispatch-status", "required-verdict"), + default="dispatch-status", + ) + parser.add_argument("--model-outcome") + parser.add_argument("--coverage-result") parser.add_argument("--expected-head", required=True) parser.add_argument("--pull-request-file", required=True, type=Path) parser.add_argument("--reviews-file", required=True, type=Path) @@ -78,12 +141,22 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: def main(argv: Sequence[str] | None = None) -> int: - """Print one JSON commit-status decision.""" + """Print one JSON decision and exit 1 when the required verdict is missing.""" args = parse_args(argv) pull_request = json.loads(args.pull_request_file.read_text(encoding="utf-8")) reviews = json.loads(args.reviews_file.read_text(encoding="utf-8")) if not isinstance(pull_request, dict) or not isinstance(reviews, list): raise SystemExit("pull request evidence must be an object and reviews evidence an array") + if args.mode == "required-verdict": + decision = decide_required_verdict_check( + expected_head=args.expected_head, + pull_request=pull_request, + reviews=reviews, + ) + print(json.dumps(decision, separators=(",", ":"))) + return 0 if decision["state"] == "success" else 1 + if not args.model_outcome or not args.coverage_result: + raise SystemExit("--model-outcome and --coverage-result are required") print( json.dumps( decide_status( @@ -99,5 +172,5 @@ def main(argv: Sequence[str] | None = None) -> int: return 0 -if __name__ == "__main__": +if __name__ == "__main__": # pragma: no cover - exercised through main() raise SystemExit(main()) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 44620fcab..f03d60015 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1277,6 +1277,38 @@ def has_current_head_changes_requested(pr: dict[str, Any]) -> bool: return current_head_review_state(pr, "CHANGES_REQUESTED") +def has_any_opencode_verdict(pr: dict[str, Any]) -> bool: + """Return whether OpenCode ever posted APPROVED or CHANGES_REQUESTED on this PR.""" + for review in (pr.get("reviews") or {}).get("nodes") or []: + if not is_opencode_review(review): + continue + if is_deterministic_fallback_approval(review): + continue + if (review.get("state") or "").upper() in {"APPROVED", "CHANGES_REQUESTED"}: + return True + return False + + +def review_dispatch_priority(pr: dict[str, Any]) -> int: + """Return a lower rank for PRs that should consume the dispatch budget first. + + Rank 0 has no OpenCode APPROVED or CHANGES_REQUESTED on any commit — the + empty Reviews tab from ContextualWisdomLab/contextual-orchestrator#176. + Rank 1 already received a verdict on a previous head and can wait for a + leftover re-review. Rank 2 already has a current-head verdict. + """ + if has_current_head_approval(pr) or has_current_head_changes_requested(pr): + return 2 + if has_any_opencode_verdict(pr): + return 1 + return 0 + + +def prioritize_review_dispatch_queue(prs: Sequence[dict[str, Any]]) -> list[dict[str, Any]]: + """Stable-sort so never-reviewed PRs take the dispatch slot before leftover increments.""" + return sorted(prs, key=review_dispatch_priority) + + def stale_opencode_change_request_ids(pr: dict[str, Any]) -> list[int]: """Return dismissible automated change requests tied to previous heads.""" review_ids: list[int] = [] @@ -1979,6 +2011,22 @@ def stale_opencode_run_ids(repo: str, workflow: str, pr: dict[str, Any]) -> list return stale +def workflow_run_name_matches( + run_name: str, workflow: str, workflow_aliases: frozenset[str] +) -> bool: + """Return whether a GitHub run name is the workflow or a run-name prefix of it. + + Workflow runs use ``run-name:`` as ``name``. OpenCode Review Dispatch therefore + appears as ``OpenCode Review Dispatch owner/repo#1@sha``, which is not equal to + the short alias. Exact-only matching misses the live run, a second dispatch is + posted, and ``cancel-in-progress`` kills the review that was about to finish. + """ + names = {workflow, *workflow_aliases} + if run_name in names: + return True + return any(run_name.startswith(f"{candidate} ") for candidate in names) + + def active_review_run_refs( repo: str, workflow: str, @@ -2010,13 +2058,27 @@ def active_review_run_refs( for run_repo in (dispatch_repo,): for run_data in active_workflow_runs(run_repo, statuses): run_name = str(run_data.get("name") or "") - if run_name != workflow and run_name not in workflow_aliases: + display_title = str(run_data.get("display_title") or "") + # Required-workflow pull_request_target runs materialize the protected + # check only; they never execute the authenticated reviewer. Ignore + # that placeholder even in the legacy same-repository mode so it + # cannot suppress the real repository_dispatch review. + if run_data.get("event") == "pull_request_target" and ( + run_name == run_title + or display_title == run_title + or display_title.startswith(f"{run_title} ") + ): + continue + if not workflow_run_name_matches( + run_name, workflow, workflow_aliases + ) and not workflow_run_name_matches( + display_title, workflow, workflow_aliases + ): continue run_id = run_data.get("id") if not run_id: continue run_ref = (run_repo, str(run_id)) - display_title = str(run_data.get("display_title") or "") dispatch_title_prefix = next( ( prefix @@ -2333,6 +2395,119 @@ def current_head_can_attempt_merge(pr: dict[str, Any], merge_state: str) -> bool return False + +def inspect_draft_pr_for_review( + repo: str, + pr: dict[str, Any], + *, + dry_run: bool, + trigger_reviews: bool, + review_dispatch_allowed: bool, + workflow: str, + security_workflow: str, + stale_opencode_minutes: int, +) -> Decision: + """Dispatch current-head review evidence without mutating a draft PR branch. + + Draft pull requests remain excluded from branch updates, auto-merge, + direct merge, stale-review dismissal, and review-thread mutation. They + still need early Strix and OpenCode feedback so authors can finish the + implementation before marking the pull request ready for review. + """ + number = pr["number"] + if has_current_head_changes_requested(pr): + return Decision( + number, + "skip", + "draft PR; current-head OpenCode review requested changes", + ) + if has_current_head_approval(pr): + return Decision( + number, + "skip", + "draft PR; current-head OpenCode approval recorded", + ) + if not trigger_reviews: + return Decision(number, "skip", "draft PR; review dispatch disabled") + + opencode_state = opencode_progress_state( + pr, + stale_after_minutes=stale_opencode_minutes, + ) + if opencode_state == "running": + return Decision( + number, + "wait", + "draft PR; OpenCode review is already in progress", + ) + if not review_dispatch_allowed: + return Decision(number, "wait", "draft PR; review dispatch limit reached") + + strix_state = strix_evidence_state(pr) + if strix_state == "missing": + wait_reason = repository_dispatch_wait_reason(repo, security_workflow) + if wait_reason: + return Decision( + number, + "wait", + "draft PR; current head has no completed Strix evidence; " + f"{wait_reason}", + ) + strix_dispatch_result = dispatch_strix_evidence( + repo, + security_workflow, + pr, + dry_run=dry_run, + ) + if strix_dispatch_result == "already_running": + return Decision( + number, + "wait", + "draft PR; current head has no completed Strix evidence; " + "same-head Strix workflow run is already active", + ) + return Decision( + number, + "security_dispatch", + "draft PR; current head has no completed Strix evidence; " + "same-head Strix dispatched", + ) + if strix_state == "running": + return Decision( + number, + "wait", + "draft PR; same-head Strix evidence is still running", + ) + + wait_reason = repository_dispatch_wait_reason(repo, workflow) + if wait_reason: + return Decision( + number, + "wait", + "draft PR; current head has completed Strix evidence; " + f"{wait_reason}", + ) + dispatch_result = dispatch_opencode_review( + repo, + workflow, + pr, + dry_run=dry_run, + ) + if dispatch_result == "already_running": + return Decision( + number, + "wait", + "draft PR; current head has completed Strix evidence; " + "same-head OpenCode workflow run is already active", + ) + return Decision( + number, + "review_dispatch", + "draft PR; current head has completed Strix evidence; " + "same-head OpenCode dispatched", + ) + + def inspect_pr( repo: str, pr: dict[str, Any], @@ -2355,7 +2530,16 @@ def inspect_pr( base_ref = pr.get("baseRefName") if pr.get("isDraft"): - return Decision(number, "skip", "draft PR") + return inspect_draft_pr_for_review( + repo, + pr, + dry_run=dry_run, + trigger_reviews=trigger_reviews, + review_dispatch_allowed=review_dispatch_allowed, + workflow=workflow, + security_workflow=security_workflow, + stale_opencode_minutes=stale_opencode_minutes, + ) cancel_stale_pr_runs(repo, pr, dry_run=dry_run) if base_ref != base_branch: # Stacked/cascade PR (base is another feature branch). Org required @@ -3918,6 +4102,8 @@ def main(argv: list[str]) -> int: if args.branch_update_limit < -1: raise SystemExit("--branch-update-limit must be -1 or greater") prs = fetch_pr(args.repo, args.pr_number) if args.pr_number else fetch_open_prs(args.repo, args.max_prs) + if not args.pr_number: + prs = prioritize_review_dispatch_queue(prs) decisions = [] review_dispatches_used = 0 branch_updates_used = 0 diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index e2d318287..a69c149db 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -497,6 +497,7 @@ assert_opencode_review_uses_codegraph_and_gpt5_fallback() { assert_file_contains "$bootstrap_file" "coverage-evidence:" "opencode required workflow preserves the stable coverage-evidence branch-protection context" assert_file_contains "$bootstrap_file" "name: opencode-review" "opencode required workflow preserves the stable opencode-review branch-protection context" assert_file_contains "$bootstrap_file" "authenticated default-branch OpenCode review dispatch" "opencode required workflow delegates real review execution to the protected dispatch path" + assert_file_contains "$bootstrap_file" "This required check is not a review" "opencode required workflow fails closed without a current-head OpenCode verdict" assert_file_not_contains "$bootstrap_file" "repository_dispatch:" "opencode required workflow does not mix privileged dispatch execution with pull_request_target" assert_file_not_contains "$bootstrap_file" "actions/checkout" "opencode required workflow never checks out pull-request content" assert_file_not_contains "$bootstrap_file" '${{ secrets.' "opencode required workflow never binds repository secrets" @@ -924,7 +925,10 @@ assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" ' if [[ "$coverage_merge_tree_step" != *'GH_TOKEN: ${{ steps.coverage_read_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}'* ]]; then record_failure "opencode coverage merge-tree fetch must use the coverage App token and central fallback credentials before github.token for target repository reads" fi - assert_file_contains "$workflow_file" 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA" "$PR_HEAD_SHA"' "coverage evidence fetches exact base and head commits as data" + assert_file_contains "$workflow_file" 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA"' "coverage evidence fetches the exact base commit as data" + assert_file_contains "$workflow_file" 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_HEAD_SHA"' "coverage evidence first attempts the exact head commit as data" + assert_file_contains "$workflow_file" 'refs/pull/${PR_NUMBER}/head:refs/remotes/origin/pr-${PR_NUMBER}-head' "coverage evidence can resolve an external exact head through the target PR ref" + assert_file_contains "$workflow_file" 'fetched_head_sha="$(git -C "$fetch_dir" rev-parse "refs/remotes/origin/pr-${PR_NUMBER}-head")"' "coverage evidence binds the fetched PR ref back to the expected exact head" assert_file_contains "$workflow_file" 'merge --no-ff --no-edit "$PR_HEAD_SHA"' "coverage evidence materializes the current pull request merge tree without action checkout" assert_file_contains "$workflow_file" "Coverage merge tree could not be materialized" "coverage evidence logs an actionable merge-tree failure reason" assert_file_contains "$workflow_file" "--require-hashes" "coverage tooling installs from a hash-pinned lock" @@ -1506,6 +1510,8 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" "github.event_name == 'pull_request_target' && format('pr-{0}', github.event.pull_request.number)" "scheduler scopes pull_request_target concurrency to the active PR" assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' && github.event.workflow_run.pull_requests[0].number && format('pr-{0}', github.event.workflow_run.pull_requests[0].number)" "scheduler scopes workflow_run concurrency to the completed review PR" assert_file_contains "$workflow_file" "github.event_name == 'schedule' && format('schedule-{0}', github.event.schedule)" "scheduler isolates the 15-minute organization sweep from the separate 30-minute scheduled scan" + assert_file_contains "$workflow_file" "github.event_name == 'repository_dispatch' && github.event.client_payload.target_repository != '' && github.event.client_payload.pr_number != ''" "scheduler scopes targeted manual queue scans to the requested PR" + assert_file_contains "$workflow_file" "cancel-in-progress: \${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' || github.event_name == 'repository_dispatch' || (github.event_name == 'workflow_run' && !github.event.workflow_run.pull_requests[0].number) }}" "scheduler cancels stale PR/review/manual queue scans instead of accumulating merge/update attempts" assert_file_contains "$workflow_file" "github.event_name == 'repository_dispatch' && format('repo-dispatch-{0}', github.repository)" "scheduler keeps manual queue scans isolated per repository" assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' && !github.event.workflow_run.pull_requests[0].number" "scheduler cancels only metadata-free workflow-run scans in their isolated fallback group" assert_file_contains "$workflow_file" "timeout-minutes: 60" "organization sweep has enough headroom to finish the complete repository walk" diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 408bb95b9..be96a377f 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -258,6 +258,7 @@ def fake_run(args, stdin=None): monkeypatch.setattr(noema, "run", fake_run) codegraph_path = tmp_path / "codegraph.md" codegraph_path.write_text("call graph: src/a.py -> tests", encoding="utf-8") + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(codegraph_path)) pr = make_pr( headRefOid="head sha", @@ -294,6 +295,7 @@ def fake_run(args, stdin=None): def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, tmp_path): + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) monkeypatch.delenv("NOEMA_CODEGRAPH_CONTEXT_PATH", raising=False) assert noema.load_codegraph_context() == "" @@ -309,6 +311,64 @@ def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, assert "1 changed files omitted from context budget" in context +def test_load_codegraph_context_rejects_workspace_escape(monkeypatch, tmp_path): + """Traversal, absolute, and symlink paths outside the workspace are rejected.""" + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + outside = tmp_path.parent / "passwd-shape" + outside.write_text("root:x:0:0:root:/root:/bin/sh\n", encoding="utf-8") + monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(outside)) + assert noema.load_codegraph_context() == ( + "CodeGraph context unavailable: path escapes the workspace." + ) + + monkeypatch.setenv( + "NOEMA_CODEGRAPH_CONTEXT_PATH", + str(tmp_path / "nested" / ".." / ".." / outside.name), + ) + assert "path escapes the workspace" in noema.load_codegraph_context() + + link = tmp_path / "escape.md" + link.symlink_to(outside) + monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(link)) + assert "path escapes the workspace" in noema.load_codegraph_context() + + +def test_codegraph_context_root_and_resolve_failure(monkeypatch, tmp_path): + """Workspace root prefers GITHUB_WORKSPACE and resolve errors stay closed.""" + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + assert noema.codegraph_context_root() == tmp_path.resolve() + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.chdir(tmp_path) + assert noema.codegraph_context_root() == tmp_path.resolve() + + class FailingPath(type(tmp_path)): + """Path stand-in whose resolve always fails.""" + + def resolve(self, *args, **kwargs): + """Raise OSError to cover the confinement resolve failure.""" + raise OSError("resolve failed") + + monkeypatch.setattr(noema, "Path", FailingPath) + assert noema.confined_codegraph_context_path("graph.md", tmp_path.resolve()) is None + + +def test_relative_codegraph_context_resolves_from_workspace(monkeypatch, tmp_path): + """Relative context paths are workspace-relative even when cwd differs.""" + workspace = tmp_path / "workspace" + context_dir = workspace / "context" + context_dir.mkdir(parents=True) + graph = context_dir / "graph.md" + graph.write_text("trusted workspace graph", encoding="utf-8") + cwd = tmp_path / "cwd" + cwd.mkdir() + + monkeypatch.chdir(cwd) + monkeypatch.setenv("GITHUB_WORKSPACE", str(workspace)) + monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", "context/graph.md") + + assert noema.load_codegraph_context() == "trusted workspace graph" + + class FakeResponse: """Small context-manager response for urllib monkeypatches.""" @@ -526,10 +586,17 @@ def test_inspect_and_review_skip_paths(monkeypatch): assert noema.inspect_and_review("owner/repo", 7) == 0 assert calls + calls.clear() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + assert noema.inspect_and_review("owner/repo", 7) == 1 + assert calls == [] + + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(isDraft=True)) + assert noema.inspect_and_review("owner/repo", 7) == 1 + assert calls == [] + cases = [ - (make_pr(), "noema"), - (make_pr(isDraft=True), "noema"), - (make_pr(reviews={"nodes": [review(login="noema", body="")]}), "noema"), + (make_pr(isDraft=True, reviews={"nodes": [review(body=marker_body)]}), "noema"), (make_pr(reviews={"nodes": [review("CHANGES_REQUESTED"), review(body=marker_body)]}), "noema"), (make_pr(reviews={"nodes": [review(body=marker_body)]}, reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}), "noema"), (make_pr(reviews={"nodes": [review(body=marker_body)]}, statusCheckRollup={"contexts": {"nodes": [{"__typename": "StatusContext", "context": "ci", "state": "FAILURE"}]}}), "noema"), @@ -543,6 +610,37 @@ def test_inspect_and_review_skip_paths(monkeypatch): assert calls == [] + +def test_existing_noema_review_cannot_bypass_primary_approval(monkeypatch): + """A Noema verdict is never sufficient without current-head OpenCode approval.""" + noema_review = review( + login="noema", + body="", + ) + pr = make_pr(reviews={"nodes": [noema_review]}) + submitted = [] + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr( + noema, + "submit_review", + lambda *args, **kwargs: submitted.append(args), + ) + + assert noema.inspect_and_review("owner/repo", 7) == 1 + assert submitted == [] + + primary_review = review( + body=( + "OpenCode reviewed the current-head bounded evidence and found " + "no blocking issues." + ) + ) + pr = make_pr(reviews={"nodes": [primary_review, noema_review]}) + assert noema.inspect_and_review("owner/repo", 7) == 0 + assert submitted == [] + + def test_parse_args_and_main(monkeypatch): parsed = noema.parse_args(["--repo", "owner/repo", "--pr-number", "9"]) assert parsed.repo == "owner/repo" diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 379dded14..87a95e475 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -523,10 +523,13 @@ def test_opencode_target_coverage_materializes_only_after_authorized_dispatch(): assert 'http."${GITHUB_SERVER_URL}/".extraheader' not in step assert "AUTHORIZATION: bearer ${GH_TOKEN}" not in step assert "AUTHORIZATION: bearer" not in step + assert 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA"' in step + assert 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_HEAD_SHA"' in step + assert 'refs/pull/${PR_NUMBER}/head:refs/remotes/origin/pr-${PR_NUMBER}-head' in step assert ( - 'fetch --no-tags --prune --no-recurse-submodules origin "$PR_BASE_SHA" "$PR_HEAD_SHA"' - in step - ) + 'fetched_head_sha="$(git -C "$fetch_dir" rev-parse ' + '"refs/remotes/origin/pr-${PR_NUMBER}-head")"' + ) in step assert "Coverage fetch could not authenticate" in step assert 'merge --no-ff --no-edit "$PR_HEAD_SHA"' in step assert "Coverage merge tree could not be materialized" in step @@ -2184,12 +2187,16 @@ def test_opencode_privileged_review_security_boundaries_are_fail_closed(): assert "repository_dispatch:" not in bootstrap.split("permissions:", 1)[0] assert "actions/checkout" not in bootstrap assert "${{ secrets." not in bootstrap + assert 'jq -r -s --arg sha "$HEAD_SHA"' in bootstrap assert "required-workflow-bootstrap:" in bootstrap assert " coverage-source-tree:\n" in bootstrap assert " coverage-evidence:\n" in bootstrap assert " opencode-review-target:\n" in bootstrap assert " name: opencode-review\n" in bootstrap assert "authenticated default-branch OpenCode review dispatch" in bootstrap + assert "This required check is not a review" in bootstrap + assert "No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head" in bootstrap + assert "pull-requests: read" in bootstrap assert workflow.count("ref: ${{ steps.trusted_source.outputs.ref }}") == 1 assert "TRUSTED_SOURCE_REF: ${{ steps.trusted_source.outputs.ref }}" in workflow assert "ref: ${{ github.workflow_sha }}" not in workflow diff --git a/tests/test_opencode_required_verdict_gate.py b/tests/test_opencode_required_verdict_gate.py new file mode 100644 index 000000000..06431accc --- /dev/null +++ b/tests/test_opencode_required_verdict_gate.py @@ -0,0 +1,246 @@ +"""Fail-closed required OpenCode check when no current-head verdict exists.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from scripts.ci import opencode_dispatch_status as dispatch_status + + +def _review( + *, login: str, state: str, commit_id: str, body: str = "" +) -> dict[str, object]: + """Return one GitHub Reviews API object.""" + return { + "user": {"login": login}, + "state": state, + "commit_id": commit_id, + "body": body, + } + + +def test_current_head_opencode_verdict_reads_latest_matching_state() -> None: + head = "a" * 40 + assert ( + dispatch_status.current_head_opencode_verdict( + [ + _review(login="opencode-agent[bot]", state="APPROVED", commit_id=head), + _review( + login="opencode-agent[bot]", + state="CHANGES_REQUESTED", + commit_id=head, + ), + ], + head, + ) + == "CHANGES_REQUESTED" + ) + assert ( + dispatch_status.current_head_opencode_verdict( + [_review(login="opencode-agent", state="APPROVED", commit_id=head)], + head, + ) + == "APPROVED" + ) + + +def test_current_head_opencode_verdict_ignores_other_actors_and_heads() -> None: + head = "a" * 40 + assert ( + dispatch_status.current_head_opencode_verdict( + [_review(login="coderabbitai[bot]", state="APPROVED", commit_id=head)], + head, + ) + is None + ) + assert ( + dispatch_status.current_head_opencode_verdict( + [ + _review( + login="opencode-agent[bot]", + state="APPROVED", + commit_id="b" * 40, + ) + ], + head, + ) + is None + ) + assert ( + dispatch_status.current_head_opencode_verdict( + [ + _review(login="coderabbitai[bot]", state="APPROVED", commit_id=head), + _review(login="opencode-agent[bot]", state="APPROVED", commit_id="b" * 40), + _review(login="opencode-agent[bot]", state="COMMENTED", commit_id=head), + ], + head, + ) + is None + ) + assert dispatch_status.current_head_opencode_verdict([], "") is None + + +@pytest.mark.parametrize( + "marker", + ( + "deterministic current-head evidence", + "deterministic fallback approval", + "model-unavailable evidence fallback", + "did not emit a usable current-head control block", + "scope: `unsupported`", + "model-pool outcome: `unknown`", + ), +) +def test_current_head_opencode_verdict_rejects_fallback_approval(marker: str) -> None: + """Model-unavailable or deterministic approvals are not formal evidence.""" + head = "a" * 40 + reviews = [ + _review(login="opencode-agent[bot]", state="APPROVED", commit_id=head), + _review( + login="opencode-agent[bot]", + state="APPROVED", + commit_id=head, + body=f"OpenCode {marker}", + ), + ] + + assert dispatch_status.current_head_opencode_verdict(reviews, head) is None + + reviews[-1]["state"] = "CHANGES_REQUESTED" + assert ( + dispatch_status.current_head_opencode_verdict(reviews, head) + == "CHANGES_REQUESTED" + ) + + +def test_current_head_opencode_verdict_uses_latest_current_head_review() -> None: + """A later non-verdict cannot expose an older decision as the latest one.""" + head = "a" * 40 + assert ( + dispatch_status.current_head_opencode_verdict( + [ + _review( + login="opencode-agent[bot]", + state="APPROVED", + commit_id=head, + ), + _review( + login="opencode-agent[bot]", + state="COMMENTED", + commit_id=head, + ), + ], + head, + ) + is None + ) + + +def test_required_workflow_rejects_fallback_approvals() -> None: + """The checkout-free jq twin enforces the same fallback boundary.""" + workflow = Path(".github/workflows/opencode-review.yml").read_text( + encoding="utf-8" + ) + + assert "| (last // {}) as $review" in workflow + for marker in ( + "deterministic current-head evidence", + "deterministic fallback approval", + "model-unavailable evidence fallback", + "did not emit a usable current-head control block", + "scope: `unsupported`", + "model-pool outcome: `unknown`", + ): + assert marker in workflow + + +def test_decide_required_verdict_check_fails_closed_without_verdict() -> None: + head = "a" * 40 + decision = dispatch_status.decide_required_verdict_check( + expected_head=head, + pull_request={"head": {"sha": head}}, + reviews=[], + ) + assert decision["state"] == "failure" + assert "This required check is not a review" in decision["description"] + stale = dispatch_status.decide_required_verdict_check( + expected_head=head, + pull_request={"head": {"sha": "c" * 40}}, + reviews=[_review(login="opencode-agent[bot]", state="APPROVED", commit_id=head)], + ) + assert stale["state"] == "failure" + approved = dispatch_status.decide_required_verdict_check( + expected_head=head, + pull_request={"head": {"sha": head}}, + reviews=[_review(login="opencode-agent[bot]", state="APPROVED", commit_id=head)], + ) + assert approved == { + "state": "success", + "description": "Current-head OpenCode verdict: APPROVED.", + } + + +def test_required_verdict_cli_exits_one_without_verdict(tmp_path: Path) -> None: + head = "a" * 40 + pr_file = tmp_path / "pr.json" + reviews_file = tmp_path / "reviews.json" + pr_file.write_text(json.dumps({"head": {"sha": head}}), encoding="utf-8") + reviews_file.write_text("[]", encoding="utf-8") + assert ( + dispatch_status.main( + [ + "--mode", + "required-verdict", + "--expected-head", + head, + "--pull-request-file", + str(pr_file), + "--reviews-file", + str(reviews_file), + ] + ) + == 1 + ) + reviews_file.write_text( + json.dumps( + [_review(login="opencode-agent[bot]", state="CHANGES_REQUESTED", commit_id=head)] + ), + encoding="utf-8", + ) + assert ( + dispatch_status.main( + [ + "--mode", + "required-verdict", + "--expected-head", + head, + "--pull-request-file", + str(pr_file), + "--reviews-file", + str(reviews_file), + ] + ) + == 0 + ) + + +def test_dispatch_status_cli_still_requires_model_and_coverage(tmp_path: Path) -> None: + head = "a" * 40 + pr_file = tmp_path / "pr.json" + reviews_file = tmp_path / "reviews.json" + pr_file.write_text(json.dumps({"head": {"sha": head}}), encoding="utf-8") + reviews_file.write_text("[]", encoding="utf-8") + with pytest.raises(SystemExit, match="--model-outcome"): + dispatch_status.main( + [ + "--expected-head", + head, + "--pull-request-file", + str(pr_file), + "--reviews-file", + str(reviews_file), + ] + ) diff --git a/tests/test_opencode_workflow_shell_syntax.py b/tests/test_opencode_workflow_shell_syntax.py index ec6edca40..8aa29ff28 100644 --- a/tests/test_opencode_workflow_shell_syntax.py +++ b/tests/test_opencode_workflow_shell_syntax.py @@ -34,6 +34,11 @@ def test_opencode_review_run_blocks_are_valid_bash(): ) assert 'gsub("`"; "'")' in workflow_text assert 'gsub("`"; "\'")' not in workflow_text + assert ( + ' elif [ "$pr_head_fetch_attempt" -lt 6 ]; then\n' + ' echo "PR head ref fetch failed on attempt $pr_head_fetch_attempt; retrying after propagation delay."\n' + ' sleep 10' + ) in workflow_text if sys.platform == "win32": return @@ -148,7 +153,7 @@ def test_merge_scheduler_targeted_dispatch_run_block_is_valid_bash(): def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): - """Only an allowlisted same-repository open PR reaches scheduler outputs.""" + """Only an allowlisted open PR with a target base reaches scheduler outputs.""" if sys.platform == "win32": return bash = shutil.which("bash") @@ -170,8 +175,11 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): """#!/usr/bin/env bash set -euo pipefail test "$1" = api -test "$2" = repos/ContextualWisdomLab/naruon/pulls/1179 -printf '%s\\n' "$FAKE_PULL_JSON" +case "$2" in + repos/ContextualWisdomLab/naruon/pulls/1179) printf '%s\\n' "$FAKE_PULL_JSON" ;; + repos/ContextualWisdomLab/naruon) printf '%s\\n' "$FAKE_REPOSITORY_JSON" ;; + *) exit 1 ;; +esac """, encoding="utf-8", ) @@ -193,6 +201,7 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): **os.environ, "PATH": f"{fake_bin}:{os.environ['PATH']}", "FAKE_PULL_JSON": json.dumps(pull), + "FAKE_REPOSITORY_JSON": json.dumps({"default_branch": "main"}), "GITHUB_EVENT_NAME": "repository_dispatch", "GITHUB_REPOSITORY": "ContextualWisdomLab/.github", "GITHUB_OUTPUT": str(output), @@ -218,6 +227,7 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): assert output.read_text(encoding="utf-8").splitlines() == [ "repository=ContextualWisdomLab/naruon", "base_branch=develop", + "default_branch=main", "head_sha=4afd4af7ad343660356791873d940aa2846f40c2", ] @@ -260,6 +270,92 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): env=cross_repo_env, ) - assert cross_repo.returncode == 1 - assert "cross-repository" in cross_repo.stdout - assert not output.exists() + assert cross_repo.returncode == 0, cross_repo.stderr + assert output.read_text(encoding="utf-8").splitlines() == [ + "repository=ContextualWisdomLab/naruon", + "base_branch=develop", + "default_branch=main", + "head_sha=4afd4af7ad343660356791873d940aa2846f40c2", + ] + + +def test_opencode_dispatch_validation_accepts_external_head_as_review_data(tmp_path): + """The central metadata gate accepts a fork while preserving exact identity.""" + if sys.platform == "win32": + return + bash = shutil.which("bash") + if bash is None: + return + + workflow_text = ( + REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml" + ).read_text(encoding="utf-8") + script = _extract_run_block( + workflow_text, + "Bind workflow inputs to live organization pull request metadata", + ) + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + fake_gh = fake_bin / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +test "$1" = api +test "$2" = repos/ContextualWisdomLab/naruon/pulls/1179 +printf '%s\\n' "$FAKE_PULL_JSON" +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + pull = { + "number": 1179, + "state": "open", + "base": { + "ref": "develop", + "sha": "1" * 40, + "repo": {"full_name": "ContextualWisdomLab/naruon", "private": False}, + }, + "head": { + "ref": "feature/fork-review", + "sha": "2" * 40, + "repo": {"full_name": "outside/fork"}, + }, + } + output = tmp_path / "github-output" + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "FAKE_PULL_JSON": json.dumps(pull), + "EVENT_NAME": "repository_dispatch", + "DISPATCH_ACTOR": "scheduler", + "DISPATCH_SENDER": "scheduler", + "ALLOWED_DISPATCH_ACTOR": "scheduler", + "ALLOWED_DISPATCH_TARGETS": "ContextualWisdomLab/naruon", + "TARGET_REPOSITORY": "ContextualWisdomLab/naruon", + "PR_NUMBER": "1179", + "SUPPLIED_BASE_REF": "develop", + "SUPPLIED_BASE_SHA": "1" * 40, + "SUPPLIED_HEAD_REF": "feature/fork-review", + "SUPPLIED_HEAD_SHA": "2" * 40, + "GITHUB_OUTPUT": str(output), + } + + result = subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env=env, + ) + + assert result.returncode == 0, result.stderr + assert output.read_text(encoding="utf-8").splitlines() == [ + "target_repository=ContextualWisdomLab/naruon", + "pr_number=1179", + "base_ref=develop", + f"base_sha={'1' * 40}", + "head_ref=feature/fork-review", + f"head_sha={'2' * 40}", + "is_private=false", + ] diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 1bbd98750..12cd10d1e 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -3,7 +3,6 @@ import hashlib from pathlib import Path import re -import subprocess import pytest @@ -20,7 +19,6 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "83f6830d5c21a324b4dbcd4e5c21a07968994b81" def _workflow_text(path: Path) -> str: @@ -157,15 +155,33 @@ def test_missing_nvidia_nim_secret_fails_closed_before_model_execution() -> None def test_independent_review_agent_key_system_is_unchanged() -> None: - """Pin the existing read-only reviewer workflow byte-for-byte.""" - result = subprocess.run( - ["git", "hash-object", str(REVIEW_DISPATCH_WORKFLOW)], - check=True, - capture_output=True, - text=True, + """Pin reviewer write credentials without freezing unrelated workflow bytes.""" + workflow = _workflow_text(REVIEW_DISPATCH_WORKFLOW) + for expression in ( + "GH_TOKEN: $" + + "{{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "GH_TOKEN: $" + "{{ secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + "GH_TOKEN: $" + + "{{ steps.opencode_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }}", + ): + assert expression in workflow + assert "pr-review-autofix" not in workflow + assert "COPILOT_GITHUB_TOKEN" not in workflow + + model_step_start = workflow.index(" - name: Run OpenCode PR Review model pool") + model_step_end = workflow.index( + " - name: Publish OpenCode review outcome", model_step_start ) - assert result.stdout.strip() == REVIEW_DISPATCH_BLOB_SHA - assert "pr-review-autofix" not in _workflow_text(REVIEW_DISPATCH_WORKFLOW) + model_step = workflow[model_step_start:model_step_end] + for provider_key in ( + "STRIX_GITHUB_MODELS_TOKEN", + "NVIDIA_API_KEY", + "OPENROUTER_API_KEY", + "OPENAI_API_KEY", + ): + assert provider_key in model_step + assert "PR_REVIEW_MERGE_TOKEN" not in model_step + assert "OPENCODE_APPROVE_TOKEN" not in model_step def test_ordinary_autofix_uses_the_same_exact_write_scope_as_conflict_repair() -> None: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 0e71bdbe2..e45b241d8 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2294,6 +2294,77 @@ def fake_run(args, stdin=None): assert not any(call[:3] == ["gh", "workflow", "run"] for call in calls) +def test_workflow_run_name_matches_accepts_github_run_name_prefix(): + aliases = frozenset({"OpenCode Review Dispatch", "Required OpenCode Review"}) + assert sched.workflow_run_name_matches( + "OpenCode Review Dispatch", "OpenCode Review", aliases + ) + assert sched.workflow_run_name_matches( + "OpenCode Review Dispatch ContextualWisdomLab/.github#1002@" + ("a" * 40), + "OpenCode Review", + aliases, + ) + assert not sched.workflow_run_name_matches( + "Strix Security Scan ContextualWisdomLab/.github#1002@" + ("a" * 40), + "OpenCode Review", + aliases, + ) + + +def test_dispatch_opencode_review_deduplicates_github_run_name_as_display_title( + monkeypatch, capsys +): + calls = [] + head_sha = "a" * 40 + live_name = f"OpenCode Review Dispatch owner/repo#1@{head_sha}" + current_dispatch = { + "id": 9101, + "name": live_name, + "event": "repository_dispatch", + "head_sha": "default-branch-sha", + "display_title": live_name, + "pull_requests": [], + } + + def fake_run(args, stdin=None): + calls.append(args) + if args[:5] == [ + "gh", + "api", + "--method", + "GET", + "repos/ContextualWisdomLab/.github/actions/runs", + ]: + if "status=queued" in args: + return json.dumps({"workflow_runs": [current_dispatch]}) + return json.dumps({"workflow_runs": []}) + if "/actions/runs" in " ".join(args): + return json.dumps({"workflow_runs": []}) + return "" + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "workflow-token") + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + + result = sched.dispatch_opencode_review( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid=head_sha), + dry_run=False, + ) + + assert result == "already_running" + assert ( + "active same-head workflow run(s) ContextualWisdomLab/.github@9101" + in capsys.readouterr().out + ) + assert not any(call[-2:] == ["--input", "-"] for call in calls) + + def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, capsys): calls = [] head_sha = "a" * 40 @@ -2382,6 +2453,13 @@ def test_central_run_filter_ignores_malformed_and_non_dispatch_titles(monkeypatc "head_sha": head_sha, "pull_requests": [{"number": 1}], }, + { + "id": 9405, + "name": "Required OpenCode Review", + "event": "workflow_run", + "head_sha": head_sha, + "pull_requests": [], + }, ] def fake_active_runs(repo, statuses=("queued", "in_progress")): @@ -2461,6 +2539,34 @@ def test_central_run_filter_ignores_same_repository_required_workflow_placeholde ) == ([], []) +def test_legacy_same_repository_filter_ignores_required_workflow_placeholder( + monkeypatch, +): + """A required-workflow placeholder must not suppress the real dispatch.""" + head_sha = "a" * 40 + placeholder = { + "id": 9404, + "name": "Required OpenCode Review", + "event": "pull_request_target", + "head_sha": head_sha, + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "pull_requests": [{"number": 1}], + } + + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda repo, statuses=("queued", "in_progress"): [placeholder], + ) + monkeypatch.delenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", raising=False) + + assert sched.active_opencode_run_refs( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid=head_sha), + ) == ([], []) + + def test_active_run_filters_and_stale_opencode_dry_run(monkeypatch): runs = [ { @@ -2993,8 +3099,201 @@ def test_summary_section_helpers_handle_empty_and_action_error_cases(): assert "- PR #5: `fork/repo` is external" in "\n".join(external_merge_lines) +def test_draft_pr_review_path_never_mutates_branch_or_merge_state(monkeypatch): + mutations = [] + dispatches = [] + for name in ("update_branch", "enable_auto_merge", "merge_pr", "disable_auto_merge"): + monkeypatch.setattr( + sched, + name, + lambda *args, _name=name, **kwargs: mutations.append(_name), + ) + monkeypatch.setattr( + sched, + "repository_dispatch_wait_reason", + lambda *args, **kwargs: "", + ) + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda repo, workflow, pr, dry_run: dispatches.append( + ("strix", pr["number"], dry_run) + ) + or "dispatched", + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: dispatches.append( + ("opencode", pr["number"], dry_run) + ) + or "dispatched", + ) + + missing_strix = inspect(make_pr(isDraft=True), dry_run=False) + assert missing_strix.action == "security_dispatch" + assert missing_strix.reason == ( + "draft PR; current head has no completed Strix evidence; same-head Strix dispatched" + ) + + completed_strix = inspect( + make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ), + dry_run=False, + ) + assert completed_strix.action == "review_dispatch" + assert completed_strix.reason == ( + "draft PR; current head has completed Strix evidence; same-head OpenCode dispatched" + ) + + approved = inspect( + make_pr( + isDraft=True, + autoMergeRequest={"enabledAt": "now"}, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ), + dry_run=False, + ) + assert approved.action == "skip" + assert approved.reason == "draft PR; current-head OpenCode approval recorded" + + changes_requested = inspect( + make_pr( + isDraft=True, + autoMergeRequest={"enabledAt": "now"}, + reviews={"nodes": [opencode_review("CHANGES_REQUESTED", "head")]}, + ), + dry_run=False, + ) + assert changes_requested.action == "skip" + assert changes_requested.reason == ( + "draft PR; current-head OpenCode review requested changes" + ) + + assert dispatches == [("strix", 1, False), ("opencode", 1, False)] + assert mutations == [] + + +def test_draft_pr_review_wait_states_are_read_only(monkeypatch): + dispatches = [] + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda *args, **kwargs: dispatches.append("strix") or "dispatched", + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatches.append("opencode") or "dispatched", + ) + + disabled = inspect(make_pr(isDraft=True), trigger_reviews=False) + assert disabled.action == "skip" + assert disabled.reason == "draft PR; review dispatch disabled" + + running = inspect( + make_pr( + isDraft=True, + statusCheckRollup={ + "contexts": {"nodes": [opencode_check(status="IN_PROGRESS")]} + }, + ) + ) + assert running.action == "wait" + assert running.reason == "draft PR; OpenCode review is already in progress" + + limited = inspect(make_pr(isDraft=True), review_dispatch_allowed=False) + assert limited.action == "wait" + assert limited.reason == "draft PR; review dispatch limit reached" + + strix_running = inspect( + make_pr( + isDraft=True, + statusCheckRollup={ + "contexts": { + "nodes": [strix_check(status="IN_PROGRESS", conclusion=None)] + } + }, + ) + ) + assert strix_running.action == "wait" + assert strix_running.reason == "draft PR; same-head Strix evidence is still running" + assert dispatches == [] + + +def test_draft_pr_review_dispatch_failures_are_wait_states(monkeypatch): + missing_reason = "central Strix dispatch workflow unavailable" + monkeypatch.setattr( + sched, + "repository_dispatch_wait_reason", + lambda repo, workflow: missing_reason + if workflow == "Strix Security Scan" + else "", + ) + missing = inspect(make_pr(isDraft=True)) + assert missing.action == "wait" + assert missing.reason == ( + "draft PR; current head has no completed Strix evidence; " + missing_reason + ) + + opencode_reason = "central OpenCode dispatch workflow unavailable" + monkeypatch.setattr( + sched, + "repository_dispatch_wait_reason", + lambda repo, workflow: opencode_reason + if workflow == "OpenCode Review" + else "", + ) + completed = make_pr( + isDraft=True, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ) + unavailable = inspect(completed) + assert unavailable.action == "wait" + assert unavailable.reason == ( + "draft PR; current head has completed Strix evidence; " + opencode_reason + ) + + monkeypatch.setattr( + sched, + "repository_dispatch_wait_reason", + lambda *args, **kwargs: "", + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: "already_running", + ) + already_running = inspect(completed) + assert already_running.action == "wait" + assert already_running.reason == ( + "draft PR; current head has completed Strix evidence; " + "same-head OpenCode workflow run is already active" + ) + + monkeypatch.setattr( + sched, + "dispatch_strix_evidence", + lambda *args, **kwargs: "already_running", + ) + strix_already_running = inspect(make_pr(isDraft=True)) + assert strix_already_running.action == "wait" + assert strix_already_running.reason == ( + "draft PR; current head has no completed Strix evidence; " + "same-head Strix workflow run is already active" + ) + + def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): - assert inspect(make_pr(isDraft=True)).action == "skip" + monkeypatch.delenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", raising=False) + draft = inspect(make_pr(isDraft=True)) + assert draft.action == "security_dispatch" + assert draft.reason == ( + "draft PR; current head has no completed Strix evidence; same-head Strix dispatched" + ) stacked = inspect(make_pr(baseRefName="develop")) assert stacked.action == "review_dispatch" assert stacked.reason == "stacked PR onto develop; OpenCode review dispatched" @@ -4483,6 +4782,114 @@ def test_main_limits_review_dispatches_and_branch_updates(monkeypatch, capsys): ) +def test_review_dispatch_priority_ranks_empty_reviews_before_leftover_rereview(): + never_reviewed = make_pr(number=176) + leftover_rereview = make_pr( + number=998, + reviews={ + "nodes": [ + opencode_review("COMMENTED", "old-head", login="seonghobae"), + opencode_review("CHANGES_REQUESTED", "old-head"), + ] + }, + ) + already_verdicted = make_pr( + number=1002, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + ) + commented_only = make_pr( + number=42, + reviews={"nodes": [opencode_review("COMMENTED", "head")]}, + ) + + human_only = make_pr( + number=7, + reviews={"nodes": [opencode_review("APPROVED", "head", login="seonghobae")]}, + ) + fallback_only = make_pr( + number=8, + reviews={ + "nodes": [ + { + **opencode_review("APPROVED", "old-head"), + "body": "Deterministic fallback approval: providers unavailable.", + } + ] + }, + ) + assert sched.has_any_opencode_verdict(never_reviewed) is False + assert sched.has_any_opencode_verdict(commented_only) is False + assert sched.has_any_opencode_verdict(human_only) is False + assert sched.has_any_opencode_verdict(fallback_only) is False + assert sched.has_any_opencode_verdict(leftover_rereview) is True + assert sched.review_dispatch_priority(never_reviewed) == 0 + assert sched.review_dispatch_priority(commented_only) == 0 + assert sched.review_dispatch_priority(fallback_only) == 0 + assert sched.review_dispatch_priority(leftover_rereview) == 1 + assert sched.review_dispatch_priority(already_verdicted) == 2 + assert [ + pr["number"] + for pr in sched.prioritize_review_dispatch_queue( + [ + leftover_rereview, + already_verdicted, + never_reviewed, + commented_only, + fallback_only, + ] + ) + ] == [176, 42, 8, 998, 1002] + + +def test_main_prefers_never_reviewed_pr_when_dispatch_budget_is_one(monkeypatch, capsys): + prs = [ + make_pr( + number=998, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + reviews={"nodes": [opencode_review("CHANGES_REQUESTED", "old-head")]}, + ), + make_pr( + number=176, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ), + ] + dispatched = [] + + monkeypatch.setattr(sched, "fetch_open_prs", lambda repo, max_prs: prs) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: dispatched.append(pr["number"]), + ) + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + + assert ( + sched.main( + [ + "--repo", + "owner/repo", + "--base-branch", + "main", + "--project-flow", + "github-flow", + "--review-dispatch-limit", + "1", + ] + ) + == 0 + ) + + output = capsys.readouterr().out + payload = json.loads(output.strip().splitlines()[-1]) + assert dispatched == [176] + assert payload["decisions"][0]["pr"] == 176 + assert payload["decisions"][0]["action"] == "review_dispatch" + assert payload["decisions"][1]["pr"] == 998 + assert payload["decisions"][1]["reason"] == ( + "current head has completed Strix evidence; review dispatch limit reached" + ) + + def test_main_rejects_invalid_review_dispatch_limit(): with pytest.raises(SystemExit, match="--review-dispatch-limit must be -1 or greater"): sched.main( diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index be13a522f..689c77e7c 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -134,11 +134,15 @@ def test_targeted_scheduler_dispatch_is_allowlisted_and_exact_pr_scoped() -> Non assert '"repos/${TARGET_REPOSITORY_INPUT}/pulls/${TARGET_PR_NUMBER}"' in validation assert '[ "$live_state" != "open" ]' in validation assert '[ "$live_base_repository" != "$TARGET_REPOSITORY_INPUT" ]' in validation - assert '[ "$live_head_repository" != "$TARGET_REPOSITORY_INPUT" ]' in validation + assert '! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' in validation + assert "this dispatch is review-only for external heads" in validation assert "Targeted scheduler dispatch base branch does not match the live PR" in validation + assert 'gh api "repos/${TARGET_REPOSITORY_INPUT}"' in validation + assert "live_default_branch=" in validation + assert "printf 'default_branch=%s\\n' \"$live_default_branch\"" in validation assert "TARGET_REPOSITORY: ${{ steps.targeted_dispatch.outputs.repository }}" in inspect assert ( - "TARGET_DEFAULT_BRANCH: ${{ steps.targeted_dispatch.outputs.base_branch }}" + "TARGET_DEFAULT_BRANCH: ${{ steps.targeted_dispatch.outputs.default_branch }}" in inspect ) assert '--repo "$TARGET_REPOSITORY"' in inspect @@ -157,6 +161,23 @@ def test_targeted_scheduler_dispatch_is_allowlisted_and_exact_pr_scoped() -> Non ) in workflow +def test_fork_heads_reach_read_only_opencode_review_dispatch() -> None: + """External heads are reviewable but never become trusted merge inputs.""" + workflow = workflow_text("opencode-review-dispatch.yml") + metadata = workflow_step( + workflow, "Bind workflow inputs to live organization pull request metadata" + ) + privileged = workflow_step(workflow, "Validate pull request head repository trust") + scheduler = (REPO_ROOT / "scripts/ci/pr_review_merge_scheduler.py").read_text( + encoding="utf-8" + ) + + assert '! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' in metadata + assert '! [[ "$head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' in privileged + assert "this path is review-only for external heads" in privileged + assert "fork or external PR heads are excluded from scheduler direct merge and auto-merge" in scheduler + + def test_privileged_review_retries_use_default_branch_repository_dispatch() -> None: """Privileged retries must never load workflow code from a selected ref.""" expected_types = {