From e32c7825b0e5e8ff773bb2d6b9c0f043283c75c7 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 10 Oct 2026 21:57:36 +0900 Subject: [PATCH 1/2] fix(ci): isolate review follow-up and self-host coalescing Keep post-publication inspection dry-run and merge-disabled; route trusted central schedule metadata to the admitted MCP pool. Preserve independent merge authorities and repair aging test clock fixtures without changing freshness thresholds. Co-Authored-By: Claude Code --- .../opencode-review-coalesce-tick.yml | 9 ++- .../workflows/opencode-review-dispatch.yml | 7 +- ...tral-review-handoff-and-runner-20261009.md | 79 +++++++++++++++++++ docs/pr-review-and-merge-procedure.md | 9 +++ scripts/ci/pr_review_merge_scheduler.py | 5 +- tests/test_audit_org_codeql_coverage.py | 16 +++- tests/test_opencode_agent_contract.py | 4 +- tests/test_opencode_review_coalesce_tick.py | 19 +++++ ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- ...eduler_opencode_followup_defer_contract.py | 10 ++- ...heduler_rate_limit_fail_fast_entrypoint.py | 27 ++++++- 11 files changed, 172 insertions(+), 15 deletions(-) create mode 100644 docs/doctoring/central-review-handoff-and-runner-20261009.md diff --git a/.github/workflows/opencode-review-coalesce-tick.yml b/.github/workflows/opencode-review-coalesce-tick.yml index 35f1da22c7..6c23834618 100644 --- a/.github/workflows/opencode-review-coalesce-tick.yml +++ b/.github/workflows/opencode-review-coalesce-tick.yml @@ -53,8 +53,12 @@ jobs: # Skip before runner admission when coalescing is off. Do not move this # gate back to step scope: that reintroduces multi-hour queue wait for # an inert echo under the org concurrent-job ceiling. - if: vars.OPENCODE_REVIEW_COALESCE_ENABLED == 'true' - runs-on: ubuntu-24.04 + if: vars.OPENCODE_REVIEW_COALESCE_ENABLED == 'true' && github.repository == 'ContextualWisdomLab/.github' && github.event_name == 'schedule' && github.workflow_ref == 'ContextualWisdomLab/.github/.github/workflows/opencode-review-coalesce-tick.yml@refs/heads/main' + # This trusted metadata-only tick runs exclusively from central main and + # uses the already-admitted organization review automation pool. + runs-on: + group: CWL MCP remediation + labels: [self-hosted, linux, x64] timeout-minutes: 4 permissions: actions: write @@ -128,6 +132,7 @@ jobs: --review-dispatch-limit -1 \ --branch-update-limit 0 \ --no-enable-auto-merge \ + --merge-mode disabled \ --no-update-branches \ --trigger-reviews \ || echo "::warning::Coalesce tick pass failed for ${repo}; continuing with remaining repositories." diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 09bbf8181a..01500ff2d6 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -8022,7 +8022,10 @@ jobs: main|master) project_flow="github-flow" ;; esac + # Publication does not grant merge authority. This follow-up only + # assesses the exact head; separately authorized scheduler runs own mutations. args=( + --dry-run --repo "$GH_REPOSITORY" --base-branch "$base_branch" --max-prs 1 @@ -8031,8 +8034,8 @@ jobs: --security-workflow "Strix Security Scan" --review-dispatch-limit 0 --no-trigger-reviews - --enable-auto-merge - --merge-mode direct_or_auto + --no-enable-auto-merge + --merge-mode disabled --no-update-branches ) if [ -n "${PR_NUMBER:-}" ]; then diff --git a/docs/doctoring/central-review-handoff-and-runner-20261009.md b/docs/doctoring/central-review-handoff-and-runner-20261009.md new file mode 100644 index 0000000000..ece6a91a59 --- /dev/null +++ b/docs/doctoring/central-review-handoff-and-runner-20261009.md @@ -0,0 +1,79 @@ +# Central review handoff and runner repair + +## Scope + +This change repairs existing review control paths. It does not claim that the +organization's hosted workload migration, LiteLLM onboarding, or product +validation is complete. + +- Central Noema handoff/base binding is already owned by + ContextualWisdomLab/.github#2111. A duplicate implementation was removed + from this change; this patch does not claim that repair is deployed. +- OpenCode's immediate post-approval scheduler invocation is dry-run, + merge-disabled, auto-merge-disabled, and branch-update-disabled. A formal + review is evidence, not permission for that job to mutate the reviewed PR. +- The coalescing tick is a trusted central-main schedule with no PR checkout. + It selects the already-admitted `CWL MCP remediation` group with + `self-hosted`, `linux`, and `x64` labels. The existing job-level enable flag + still prevents inert ticks from obtaining a runner. +- The coalescing invocation explicitly selects `--merge-mode disabled` as + well as disabling auto-merge and branch updates. Disabling auto-merge alone + does not disable a direct-merge mode. + +The regular review-event and scheduled merge scheduler paths are unchanged. +This patch does not establish an end-to-end no-merge guarantee for a PR that +those separately authorized paths inspect. + +## Current operational evidence + +GitHub check-run annotations for +`ContextualWisdomLab/psychometrics-commons#165`, head +`bb9028714d996871ed2588ea9a3aa83fd581af84`, reported that the hosted job could +not start because the account was locked due to a billing issue. The jobs +had runner id `0` and no executed steps. They are not executed test failures. + +The product fix was previously verified locally, including branch coverage +1724/1724. That evidence does not replace GitHub required checks, independent +review, or an admitted isolated self-hosted build lane. + +An organization runner-group readback showed `CWL MCP remediation` already +admitted the coalescing workflow at central `refs/heads/main`, with two online +runners. Runtime availability is re-read before dispatch; this observation is +not a permanent capacity claim. + +## Remaining prerequisites + +- Product PR execution must use a verified isolation boundary; do not send + untrusted builds to shared metadata/review runners. +- A Noema approval is not yet accepted by the scheduler's blanket bot + exclusion. A future change must recognize only authenticated authorized + App evidence on the exact head, preserve revocations, and not admit arbitrary + bot approvals. +- Current review configuration remains `orchestrator/free`; it is not a + literal `auto` model setting. An authenticated LiteLLM onboarding and + explicit gateway contract/policy update are required before claiming the + requested `auto` route. +- The public LiteLLM schema at `https://litellm.poinnetworks.net/openapi.json` + declares authentication for `/key/generate`; no unauthenticated key was + requested or issued. A dedicated inference credential must enter the + gateway's approved secret boundary, not source, logs, or review text. +- No hosted security or product-validation workflow was disabled by this + patch. Mandatory unavailable evidence stays non-passing until an equivalent + isolated replacement is verified. + +## Verification + +Regression tests enforce assessment-only post-publication scheduler arguments +and pin the trusted coalescer's runner selection. Workflow shell syntax and +actionlint validate the modified callers. Two pre-existing CodeQL tests now +freeze their default-clock input so September fixtures do not expire under +October's real wall clock; production freshness thresholds are unchanged. + +## References + +- GitHub. (n.d.). *Secure use reference*. + https://docs.github.com/en/actions/reference/security/secure-use +- GitHub. (n.d.). *Events that trigger workflows*. + https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows +- LiteLLM. (n.d.). *Self serve*. + https://docs.litellm.ai/docs/proxy/self_serve diff --git a/docs/pr-review-and-merge-procedure.md b/docs/pr-review-and-merge-procedure.md index e34c3957cb..5a2dc88ad7 100644 --- a/docs/pr-review-and-merge-procedure.md +++ b/docs/pr-review-and-merge-procedure.md @@ -63,6 +63,15 @@ by the OpenCode GitHub App. A GitHub Actions-authored review is not OpenCode approval evidence. The separate scheduler also listens for that App review, waits for the publishing OpenCode check to finish, and then retries direct merge outside the review job when repository auto-merge is unavailable. +The OpenCode job's own post-publication scheduler is assessment-only: +`--dry-run`, `--merge-mode disabled`, `--no-enable-auto-merge`, and +`--no-update-branches`. Review publication never grants mutation authority to +that follow-up. Separately authorized review-event and scheduled scheduler +runs retain their existing policy; these flags do not disable those runs. + +Central Noema handoff and live base binding are tracked separately in +ContextualWisdomLab/.github#2111. This assessment-only follow-up change does not +claim that handoff is deployed. A dispatch is neither review nor approval. Every merge keeps `--match-head-commit`. It prefers squash and retries with a merge commit only when the target repository explicitly reports that squash diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 051a3bbd5b..1cc18dc818 100755 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -139,10 +139,11 @@ def _is_opencode_post_approval_followup( and _argument_value(argument_values, "--max-prs") == "1" and _argument_value(argument_values, "--review-dispatch-limit") == "0" and _argument_value(argument_values, "--merge-mode") - == "direct_or_auto" + == "disabled" + and "--dry-run" in argument_set and "--pr-number" in argument_set and "--no-trigger-reviews" in argument_set - and "--enable-auto-merge" in argument_set + and "--no-enable-auto-merge" in argument_set and "--no-update-branches" in argument_set ) diff --git a/tests/test_audit_org_codeql_coverage.py b/tests/test_audit_org_codeql_coverage.py index 0ffc8fa749..18be9e46aa 100644 --- a/tests/test_audit_org_codeql_coverage.py +++ b/tests/test_audit_org_codeql_coverage.py @@ -262,7 +262,13 @@ def test_null_latest_analysis_is_not_coverage() -> None: ] -def test_audit_codeql_coverage_defaults_now_to_current_time() -> None: +def test_audit_codeql_coverage_defaults_now_to_current_time(monkeypatch) -> None: + class Clock(datetime): + @classmethod + def now(cls, tz=None): + return NOW.astimezone(tz) + + monkeypatch.setattr(audit, "datetime", Clock) repositories = [covered_by_recent_analysis("DefaultNowRepo", days_ago=0)] assert audit.audit_codeql_coverage(repositories) == [] @@ -299,7 +305,13 @@ def test_main_fail_path_reports_gaps_from_stdin(monkeypatch, capsys) -> None: assert "FAIL: 1 repositories have no CodeQL coverage" in captured.err -def test_main_pass_path_reports_from_file_arg(tmp_path, capsys) -> None: +def test_main_pass_path_reports_from_file_arg(tmp_path, capsys, monkeypatch) -> None: + class Clock(datetime): + @classmethod + def now(cls, tz=None): + return NOW.astimezone(tz) + + monkeypatch.setattr(audit, "datetime", Clock) payload_path = tmp_path / "repositories.json" payload_path.write_text( json.dumps([covered_by_default_setup("ELUNVERA"), covered_by_recent_analysis("Orgmetra")]), diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 8a80d65461..668f28fac7 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2531,7 +2531,9 @@ def test_opencode_runs_merge_scheduler_after_review_without_repo_local_dispatch( "'opencode-app' || 'github-token' }}" ) in workflow assert "--no-trigger-reviews" in workflow - assert "--enable-auto-merge" in workflow + assert "--no-enable-auto-merge" in workflow + assert "--merge-mode disabled" in workflow + assert "--enable-auto-merge" not in workflow assert "--no-update-branches" in workflow assert "--require-opencode-app" in workflow assert "approval_attempt in 1 2 3 4 5 6" in workflow diff --git a/tests/test_opencode_review_coalesce_tick.py b/tests/test_opencode_review_coalesce_tick.py index cc44534640..2f37ad9adf 100644 --- a/tests/test_opencode_review_coalesce_tick.py +++ b/tests/test_opencode_review_coalesce_tick.py @@ -56,6 +56,24 @@ def test_tick_does_not_stack(): assert "cancel-in-progress: false" in concurrency_block +def test_tick_uses_the_admitted_central_self_hosted_group(): + """Trusted schedule metadata work must not consume billing-locked hosted jobs.""" + job = _job_block() + assert ( + " runs-on:\n group: CWL MCP remediation\n" + " labels: [self-hosted, linux, x64]" + ) in job + assert "runs-on: ubuntu" not in job + assert "pull_request:" not in _workflow_text() + header = job.split("runs-on:", 1)[0] + assert "github.repository == 'ContextualWisdomLab/.github'" in header + assert "github.event_name == 'schedule'" in header + assert ( + "github.workflow_ref == 'ContextualWisdomLab/.github/.github/workflows/" + "opencode-review-coalesce-tick.yml@refs/heads/main'" + ) in header + + def test_tick_bounds_its_own_wall_clock(): job = _job_block() assert "timeout-minutes: 4" in job @@ -74,6 +92,7 @@ def test_tick_scopes_each_repository_pass_to_review_dispatch_only(): 1, )[1] assert "--no-enable-auto-merge" in dispatch_step + assert "--merge-mode disabled" in dispatch_step assert "--no-update-branches" in dispatch_step assert "--branch-update-limit 0" in dispatch_step assert "--trigger-reviews" in dispatch_step diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 1bfc13292e..921556ee20 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -17,7 +17,7 @@ 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 = "09bbf8181a443f7a5630ec91ca958440c5dcfc68" +REVIEW_DISPATCH_BLOB_SHA = "01500ff2d61340953dbc34fd7d5f700cc44fb574" def _workflow_text(path: Path) -> str: diff --git a/tests/test_scheduler_opencode_followup_defer_contract.py b/tests/test_scheduler_opencode_followup_defer_contract.py index c6564e6d6d..33f04310ba 100644 --- a/tests/test_scheduler_opencode_followup_defer_contract.py +++ b/tests/test_scheduler_opencode_followup_defer_contract.py @@ -35,12 +35,13 @@ def test_facade_signature_matches_the_live_opencode_followup_caller() -> None: assert workflow_source.startswith("name: OpenCode Review Dispatch\n") for required_argument in ( + '--dry-run', '--max-prs 1', '--review-dispatch-limit 0', - '--merge-mode direct_or_auto', + '--merge-mode disabled', '--pr-number "$PR_NUMBER"', '--no-trigger-reviews', - '--enable-auto-merge', + '--no-enable-auto-merge', '--no-update-branches', ): assert required_argument in scheduler_step @@ -51,7 +52,10 @@ def test_facade_signature_matches_the_live_opencode_followup_caller() -> None: '_argument_value(argument_values, "--review-dispatch-limit") == "0"' in facade_source ) - assert '== "direct_or_auto"' in facade_source + assert '== "disabled"' in facade_source + assert '"--dry-run" in argument_set' in facade_source + assert '--enable-auto-merge' not in scheduler_step + assert '--merge-mode direct_or_auto' not in scheduler_step def test_followup_documents_the_authoritative_retry_owner() -> None: diff --git a/tests/test_scheduler_rate_limit_fail_fast_entrypoint.py b/tests/test_scheduler_rate_limit_fail_fast_entrypoint.py index a0995aebaa..f5eb07f886 100644 --- a/tests/test_scheduler_rate_limit_fail_fast_entrypoint.py +++ b/tests/test_scheduler_rate_limit_fail_fast_entrypoint.py @@ -26,6 +26,7 @@ def _post_approval_arguments() -> list[str]: """Return the exact OpenCode post-publication scheduler signature.""" return [ + "--dry-run", "--repo", "ContextualWisdomLab/example-service", "--base-branch", @@ -41,9 +42,9 @@ def _post_approval_arguments() -> list[str]: "--review-dispatch-limit", "0", "--no-trigger-reviews", - "--enable-auto-merge", + "--no-enable-auto-merge", "--merge-mode", - "direct_or_auto", + "disabled", "--no-update-branches", "--pr-number", "42", @@ -353,6 +354,28 @@ def deferred_main(received_arguments: list[str]) -> int: assert scheduler_facade.run_cli(argument_values) == 1 +def test_scheduler_followup_requires_dry_run_for_rate_limit_defer( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Never treat a mutation-capable follow-up as a review-only defer.""" + + argument_values = [ + value for value in _post_approval_arguments() if value != "--dry-run" + ] + calls: list[list[str]] = [] + + def deferred_main(received_arguments: list[str]) -> int: + assert received_arguments == argument_values + raise RuntimeError("API rate limit exceeded for installation") + + monkeypatch.setattr(scheduler_core, "main", deferred_main) + monkeypatch.setattr(scheduler_core, "run", lambda args, **_: calls.append(list(args))) + monkeypatch.setenv("GITHUB_WORKFLOW", "OpenCode Review Dispatch") + + assert scheduler_facade.run_cli(argument_values) == 1 + assert calls == [] + + def test_caller_name_alone_cannot_relabel_org_scan_as_accepted_defer( monkeypatch: pytest.MonkeyPatch, ) -> None: From c3e689fe3d6948dafa0e586786e1108c74c11ec1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 11 Oct 2026 23:37:49 +0900 Subject: [PATCH 2/2] fix(review): recognize authenticated independent Noema App evidence Require canonical non-author App identity, attached exact-head commit and coherent publisher receipt; preserve latest formal revocation and reject arbitrary bots. Keep repository merge/check/security policy separate and unchanged. Focused scheduler tests pass; full suite remains in progress. Co-Authored-By: Claude Code --- ...tral-review-handoff-and-runner-20261009.md | 13 +- docs/org-required-workflow-rollout.md | 10 ++ scripts/ci/pr_review_merge_scheduler_core.py | 60 ++++++- tests/test_noema_scheduler_approval.py | 160 ++++++++++++++++++ 4 files changed, 234 insertions(+), 9 deletions(-) create mode 100644 tests/test_noema_scheduler_approval.py diff --git a/docs/doctoring/central-review-handoff-and-runner-20261009.md b/docs/doctoring/central-review-handoff-and-runner-20261009.md index ece6a91a59..551beb914f 100644 --- a/docs/doctoring/central-review-handoff-and-runner-20261009.md +++ b/docs/doctoring/central-review-handoff-and-runner-20261009.md @@ -45,10 +45,15 @@ not a permanent capacity claim. - Product PR execution must use a verified isolation boundary; do not send untrusted builds to shared metadata/review runners. -- A Noema approval is not yet accepted by the scheduler's blanket bot - exclusion. A future change must recognize only authenticated authorized - App evidence on the exact head, preserve revocations, and not admit arbitrary - bot approvals. +- The scheduler recognizes only the canonical GitHub-authenticated Noema + App as an exception to blanket bot exclusion. It requires the real review + commit to match the exact head, a coherent publisher head/result/actor/App + credential receipt, and non-author identity after REST/GraphQL normalization. + The latest relevant formal review is resolved before receipt validation: + revocations or malformed latest approvals cannot resurrect older approvals. + This establishes independent-review eligibility only; GitHub's aggregate + approval policy, checks, security evidence, threads, and merge freshness + remain separate and unchanged. - Current review configuration remains `orchestrator/free`; it is not a literal `auto` model setting. An authenticated LiteLLM onboarding and explicit gateway contract/policy update are required before claiming the diff --git a/docs/org-required-workflow-rollout.md b/docs/org-required-workflow-rollout.md index 674a5d0b5a..b7804b65bc 100644 --- a/docs/org-required-workflow-rollout.md +++ b/docs/org-required-workflow-rollout.md @@ -223,6 +223,16 @@ mints a short-lived installation token restricted to the target repository; the App has read-only Actions/checks/contents/status/code-scanning/Dependabot access and write access only to pull-request reviews. +The scheduler's independent-review gate recognizes this canonical App only +when GitHub authenticates its bot actor, the attached review commit is the +exact head, and the publisher's head/result/actor/App-source receipt agrees. +The PR author cannot satisfy this condition, including alternate REST and +GraphQL spellings of the same bot. Latest malformed formal approvals or +revocations block older receipts; comments do not replace formal policy state. +Other automation remains excluded. This reviewer eligibility does not replace +GitHub approval policy, required checks, security evidence, resolved threads, +or fresh head/base validation before a head-guarded merge. + The PydanticAI `ReviewAgent` product in `ContextualWisdomLab/noema` (`reviewer/noema_reviewer`, `ContextualWisdomLab/noema#9`) is the target standalone judgement plane, while the central Python gate remains the deployed fail-closed reviewer. The diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 5a86bd24c8..5d1b3eb711 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -1208,7 +1208,10 @@ def rest_review_node(review: dict[str, Any]) -> dict[str, Any]: "state": review.get("state"), "body": review.get("body"), "submittedAt": review.get("submitted_at"), - "author": {"login": ((review.get("user") or {}).get("login"))}, + "author": { + "login": ((review.get("user") or {}).get("login")), + **({"__typename": review["user"]["type"]} if (review.get("user") or {}).get("type") is not None else {}), + }, "commit": {"oid": commit_id} if commit_id else None, } @@ -2183,10 +2186,9 @@ def is_bot_review_author(review: dict[str, Any]) -> bool: ``login`` (e.g. ``dependabot[bot]``), but GitHub's GraphQL API can return the bare account name for that same actor (e.g. ``dependabot``) while exposing ``__typename: "Bot"`` on the ``author`` field instead of - the suffix. Checking both keeps bot exclusion correct regardless of - which API surface -- and which suffix convention -- produced the - review node; ``rest_review_node`` never sets ``__typename``, so REST - reviews continue to rely solely on the login suffix. + the suffix. Checking both preserves the API actor classification across + both surfaces; ``rest_review_node`` carries REST ``user.type`` into + ``__typename`` when present, retaining suffix compatibility when absent. """ if review_author_login(review).endswith("[bot]"): return True @@ -2249,6 +2251,39 @@ def has_current_head_approval(pr: dict[str, Any]) -> bool: return current_head_review_state(pr, "APPROVED") +def is_noema_app_review(review: dict[str, Any]) -> bool: + """Accept only the canonical Noema App identity authenticated by GitHub.""" + actor = review.get("author") or {} + login = review_author_login(review) + return ( + login in {"cwl-noema-review", "cwl-noema-review[bot]"} + and actor.get("__typename") in {None, "Bot"} + and (actor.get("__typename") == "Bot" or login.endswith("[bot]")) + ) + + +def has_noema_app_approval_receipt(review: dict[str, Any], head: str) -> bool: + """Validate the publisher's App/head receipt, not broader merge authority.""" + if not re.fullmatch(r"[0-9a-f]{40}", head) or (review.get("commit") or {}).get("oid") != head: + return False + body = str(review.get("body") or "") + footer_marker = "" + if body.count(footer_marker) != 1: + return False + _, _, footer = body.partition(footer_marker) + receipt = ( + r"\n- Result: APPROVE\n- Head SHA: `" + re.escape(head) + + r"`\n- Reviewer credential: `(?:noema-review-github-app(?:-refresh)?|noema-review-app-oidc)`" + + r"\n- Actor: `cwl-noema-review\[bot\]`\n\n" + + r"" + ) + if re.fullmatch(receipt, footer) is None: + return False + # The authenticated publisher validates model substance before posting; + # human-facing headings are not an additional authorization contract. + return True + + def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: """Return whether an eligible reviewer's latest exact-head policy state approves.""" author = ((pr.get("author") or {}).get("login") or "").lower() @@ -2258,6 +2293,21 @@ def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: for review in reversed((pr.get("reviews") or {}).get("nodes") or []): reviewer = review_author_login(review) state = (review.get("state") or "").upper() + if reviewer.removesuffix("[bot]") == "cwl-noema-review": + if state not in {"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}: + continue + identity = "cwl-noema-review" + if identity in seen_reviewers: + continue + seen_reviewers.add(identity) + if ( + author.removesuffix("[bot]") != identity + and is_noema_app_review(review) + and state == "APPROVED" + and has_noema_app_approval_receipt(review, str(pr.get("headRefOid") or "")) + ): + return True + continue if ( not reviewer or reviewer == author diff --git a/tests/test_noema_scheduler_approval.py b/tests/test_noema_scheduler_approval.py new file mode 100644 index 0000000000..6dceec3836 --- /dev/null +++ b/tests/test_noema_scheduler_approval.py @@ -0,0 +1,160 @@ +"""Recognize only authenticated exact-head Noema App review receipts.""" + +from __future__ import annotations + +import json +from copy import deepcopy + +import pytest + +from scripts.ci import noema_review_gate as publisher +from scripts.ci import pr_review_merge_scheduler_core as scheduler + +HEAD = "a" * 40 +BASE = "b" * 40 + + +@pytest.fixture +def published_review(monkeypatch: pytest.MonkeyPatch) -> dict: + """Capture the real publisher without a network write or model call.""" + payloads = [] + + def capture(_args, *, stdin=None): + payloads.append(json.loads(stdin)) + return "" + + monkeypatch.setattr(publisher, "run", capture) + monkeypatch.setenv("NOEMA_REVIEW_TOKEN_SOURCE", "noema-review-github-app-refresh") + publisher.submit_review( + "ContextualWisdomLab/example", 7, + {"headRefOid": HEAD, "baseRefOid": BASE}, "cwl-noema-review[bot]", + { + "decision": "approve", "summary": "Changed-line review found no blocking regression.", + "findings": [], + "reviewed_lines": [{"path": "src/runtime.py", "line": 12, "side": "RIGHT", "analysis": "Invalid references are rejected before lookup."}], + "adversarial_validation": { + "status": "passed", "residual_risk": "Deployment evidence remains separate.", + "probes": [{"path": "src/runtime.py", "line": 12, "side": "RIGHT", "outcome": "falsified", "hypothesis": "Invalid references reach lookup.", "evidence": "Fixture test at src/runtime.py:12 returned rejected input."}], + }, + }, + ) + payload = payloads[0] + assert payload["commit_id"] == HEAD and payload["event"] == "APPROVE" + return scheduler.rest_review_node({"id": 17, "state": "APPROVED", "commit_id": HEAD, "user": {"login": "cwl-noema-review[bot]"}, "body": payload["body"]}) + + +def pull_request(reviews: list[dict], *, author="participant") -> dict: + """Build an authenticated API snapshot; independent gates retain their authority.""" + return {"author": {"login": author}, "headRefOid": HEAD, "baseRefOid": BASE, "reviewDecision": "APPROVED", "reviews": {"nodes": reviews}} + + +@pytest.mark.parametrize("api_type,eligible", [("Bot", True), ("User", False)]) +def test_rest_conversion_preserves_authenticated_actor_type(published_review, api_type, eligible): + """Conversion must not turn an explicit REST User into an App by suffix.""" + converted = scheduler.rest_review_node({ + "id": 18, + "state": "APPROVED", + "commit_id": HEAD, + "user": {"login": "cwl-noema-review[bot]", "type": api_type}, + "body": published_review["body"], + }) + assert converted["author"]["__typename"] == api_type + assert scheduler.has_independent_current_head_approval(pull_request([converted])) is eligible + + +@pytest.mark.parametrize("graphql_actor", [False, True]) +def test_actual_publisher_receipt_counts_as_independent_review(published_review, graphql_actor): + if graphql_actor: + published_review["author"] = {"login": "cwl-noema-review", "__typename": "Bot"} + pr = pull_request([published_review]) + assert scheduler.has_independent_current_head_approval(pr) + assert scheduler.merge_approval_block_reason(pr) is None + + +@pytest.mark.parametrize("source", ["noema-review-github-app", "noema-review-app-oidc"]) +def test_authenticated_app_credential_sources(published_review, source): + published_review["body"] = published_review["body"].replace("noema-review-github-app-refresh", source) + assert scheduler.has_independent_current_head_approval(pull_request([published_review])) + + +@pytest.mark.parametrize("author", ["cwl-noema-review", "cwl-noema-review[bot]", "CWL-NOEMA-REVIEW"]) +def test_app_never_approves_itself(published_review, author): + assert not scheduler.has_independent_current_head_approval(pull_request([published_review], author=author)) + + +@pytest.mark.parametrize("actor", [ + {"login": "cwl-noema-review", "__typename": "User"}, + {"login": "cwl-noema-review"}, + {"login": "cwl-noema-review[bot]", "__typename": "User"}, + {"login": "another-bot[bot]", "__typename": "Bot"}, + {"login": "opencode-agent[bot]", "__typename": "Bot"}, +]) +def test_only_canonical_authenticated_bot_qualifies(published_review, actor): + published_review["author"] = actor + assert not scheduler.has_independent_current_head_approval(pull_request([published_review])) + + +@pytest.mark.parametrize("commit", [None, {"oid": "b" * 40}, {"oid": "A" * 40}, {"oid": "short"}]) +def test_body_head_never_replaces_actual_exact_commit(published_review, commit): + published_review["commit"] = commit + assert not scheduler.has_independent_current_head_approval(pull_request([published_review])) + + +@pytest.mark.parametrize("old,new", [ + (f"- Head SHA: `{HEAD}`", f"- Head SHA: `{'b' * 40}`"), + ("decision=approve", "decision=comment"), + ("- Result: APPROVE", "- Result: COMMENT"), + ("- Actor: `cwl-noema-review[bot]`", "- Actor: `opencode-agent[bot]`"), + ("noema-review-github-app-refresh", "noema-review-pat"), + (publisher.NOEMA_REVIEW_FOOTER_MARKER, ""), +]) +def test_incoherent_or_incomplete_receipt_fails_closed(published_review, old, new): + published_review["body"] = published_review["body"].replace(old, new) + assert not scheduler.has_independent_current_head_approval(pull_request([published_review])) + + +@pytest.mark.parametrize("state", ["CHANGES_REQUESTED", "DISMISSED", "APPROVED"]) +def test_latest_formal_policy_state_blocks_old_approval(published_review, state): + latest = deepcopy(published_review) + latest.update(state=state, body="Malformed receipt.", author={"login": "cwl-noema-review", "__typename": "Bot"}) + assert not scheduler.has_independent_current_head_approval(pull_request([published_review, latest])) + + +@pytest.mark.parametrize("head", ["", "short", "A" * 40]) +def test_noncanonical_snapshot_head_cannot_authorize_app_review(published_review, head): + pr = pull_request([published_review]) + pr["headRefOid"] = head + assert not scheduler.has_independent_current_head_approval(pr) + + +@pytest.mark.parametrize("mutation", ["missing_commit", "stale_commit", "wrong_api_type", "trailing_text", "duplicate_footer"]) +def test_latest_invalid_formal_receipt_never_falls_back(published_review, mutation): + latest = deepcopy(published_review) + if mutation == "missing_commit": + latest["commit"] = None + elif mutation == "stale_commit": + latest["commit"] = {"oid": "b" * 40} + elif mutation == "wrong_api_type": + latest["author"] = {"login": "cwl-noema-review", "__typename": "User"} + elif mutation == "trailing_text": + latest["body"] += "\nUntrusted continuation." + else: + latest["body"] += publisher.NOEMA_REVIEW_FOOTER_MARKER + assert not scheduler.has_independent_current_head_approval(pull_request([published_review, latest])) + + +def test_comment_does_not_revoke_formal_approval(published_review): + latest = deepcopy(published_review) + latest.update(state="COMMENTED", body="Additional observation.") + assert scheduler.has_independent_current_head_approval(pull_request([published_review, latest])) + + +def test_github_review_decision_remains_required(published_review): + pr = pull_request([published_review]) + pr["reviewDecision"] = "REVIEW_REQUIRED" + assert "repository approval policy is unsatisfied" in scheduler.merge_approval_block_reason(pr) + + +def test_human_approval_behavior_is_unchanged(): + review = {"state": "APPROVED", "author": {"login": "reviewer", "__typename": "User"}, "commit": {"oid": HEAD}} + assert scheduler.has_independent_current_head_approval(pull_request([review]))