diff --git a/.github/actions/noema-review/two_phase.py b/.github/actions/noema-review/two_phase.py index 2815d7a050..fb13a96268 100755 --- a/.github/actions/noema-review/two_phase.py +++ b/.github/actions/noema-review/two_phase.py @@ -145,23 +145,81 @@ def _read_envelope(path: Path) -> dict[str, Any]: return payload -def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> int: - """Run model review and seal its verdict without publishing GitHub evidence.""" +def _model_work_eligibility( + repo: str, + number: int, + expected_head: str, + *, + skip_closed_or_stale: bool, + phase: str, +) -> tuple[str, dict[str, Any], str, str] | None: + """Return the validated review identity when this head still needs model work.""" expected = _canonical_head(expected_head) pull_request = gate.fetch_pr(repo, number) try: gate.require_expected_head(pull_request, expected) except RuntimeError: - print("Pull request is closed or stale; Noema verdict preparation skipped.") - return 0 + if not skip_closed_or_stale: + raise + print(f"Pull request is closed or stale; Noema {phase} skipped.") + return None expected_base = _canonical_base(pull_request) actor = _reviewer_actor() if pull_request.get("isDraft"): - print("PR is draft; Noema verdict preparation skipped.") - return 0 + print(f"PR is draft; Noema {phase} skipped.") + return None if gate.existing_noema_review(pull_request, actor): - print("Current head already has a Noema review; verdict preparation skipped.") + print(f"Current head already has a Noema review; Noema {phase} skipped.") + return None + return expected, pull_request, expected_base, actor + + +def admit_model_work(repo: str, number: int, expected_head: str, path: Path) -> int: + """Record whether the current review needs the expensive model sidecar.""" + eligibility = _model_work_eligibility( + repo, + number, + expected_head, + skip_closed_or_stale=False, + phase="model admission", + ) + if eligibility is None: + return 0 + expected, pull_request, expected_base, _actor = eligibility + repository = pull_request.get("repository") + if ( + not isinstance(repository, dict) + or not isinstance(repository.get("nameWithOwner"), str) + or repository["nameWithOwner"].casefold() != repo.casefold() + or repository.get("visibility") not in ("PUBLIC", "PRIVATE", "INTERNAL") + ): + raise RuntimeError("Noema repository visibility could not be verified") + _write_envelope( + path, + { + "schema_version": ENVELOPE_SCHEMA_VERSION, + "repository": repo, + "pull_request_number": number, + "expected_head": expected, + "expected_base": expected_base, + "repository_visibility": repository["visibility"].lower(), + }, + ) + return 0 + + +def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> int: + """Run model review and seal its verdict without publishing GitHub evidence.""" + eligibility = _model_work_eligibility( + repo, + number, + expected_head, + skip_closed_or_stale=True, + phase="verdict preparation", + ) + if eligibility is None: return 0 + expected, pull_request, expected_base, _actor = eligibility diff, truncated = gate.fetch_diff(repo, number) changed_files = gate.fetch_changed_files(repo, number) @@ -253,6 +311,7 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser.add_argument("--pr-number", required=True, type=int) parser.add_argument("--expected-head", required=True) modes = parser.add_mutually_exclusive_group(required=True) + modes.add_argument("--admit-model-file", type=Path) modes.add_argument("--prepare-verdict-file", type=Path) modes.add_argument("--publish-verdict-file", type=Path) return parser.parse_args(argv) @@ -263,6 +322,8 @@ def main(argv: list[str]) -> int: args = parse_args(argv) if args.pr_number <= 0: raise SystemExit("--pr-number must be positive") + if args.admit_model_file is not None: + return admit_model_work(args.repo, args.pr_number, args.expected_head, args.admit_model_file) if args.prepare_verdict_file is not None: return prepare_verdict(args.repo, args.pr_number, args.expected_head, args.prepare_verdict_file) return publish_verdict(args.repo, args.pr_number, args.expected_head, args.publish_verdict_file) diff --git a/.github/workflows/agent-review-runtime-quality-ci.yml b/.github/workflows/agent-review-runtime-quality-ci.yml index 3680da8778..6c5168a318 100644 --- a/.github/workflows/agent-review-runtime-quality-ci.yml +++ b/.github/workflows/agent-review-runtime-quality-ci.yml @@ -7,6 +7,11 @@ on: - ".github/workflows/agent-review-runtime-quality-ci.yml" - ".github/workflows/noema-review.yml" - ".github/actions/noema-review/two_phase.py" + - "scripts/ci/noema_review_gate.py" + - "tests/test_noema_review_gate.py" + - "tests/test_noema_orchestrator_workflow_contract.py" + - "tests/test_required_workflow_queue_contract.py" + - "tests/test_current_head_coalescer_self_cancellation.py" - "tests/test_noema_reviewer_token_lifetime.py" - "tests/test_noema_two_phase_handoff.py" - "tests/test_noema_refreshed_app_identity.py" @@ -154,6 +159,7 @@ jobs: commercial_readiness_suite=false exact_artifact_suite=false + changed_paths="$(git diff --name-only "$BASE_SHA...$HEAD_SHA")" while IFS= read -r changed_path; do case "$changed_path" in .github/workflows/agent-review-runtime-quality-ci.yml) @@ -175,6 +181,9 @@ jobs: ;; .github/workflows/noema-review.yml|\ .github/actions/noema-review/two_phase.py|\ + scripts/ci/noema_review_gate.py|\ + tests/test_noema_review_gate.py|\ + tests/test_noema_orchestrator_workflow_contract.py|\ tests/test_noema_reviewer_token_lifetime.py|\ tests/test_noema_two_phase_handoff.py|\ tests/test_noema_refreshed_app_identity.py|\ @@ -210,7 +219,9 @@ jobs: queue_suite=true review_repair_suite=true ;; - scripts/ci/current_head_run_coalescer.py) + scripts/ci/current_head_run_coalescer.py|\ + tests/test_current_head_coalescer_self_cancellation.py|\ + tests/test_required_workflow_queue_contract.py) queue_suite=true ;; .github/workflows/pr-review-fix-scheduler.yml|\ @@ -286,7 +297,7 @@ jobs: exact_artifact_suite=true ;; esac - done < <(git diff --name-only "$BASE_SHA...$HEAD_SHA") + done <<<"$changed_paths" { echo "noema=$noema_suite" @@ -328,6 +339,8 @@ jobs: run: | set -euo pipefail PYTHONPATH=. python -m pytest -q \ + tests/test_noema_review_gate.py \ + tests/test_noema_orchestrator_workflow_contract.py \ tests/test_noema_reviewer_token_lifetime.py \ tests/test_noema_two_phase_handoff.py \ tests/test_noema_refreshed_app_identity.py \ @@ -374,7 +387,7 @@ jobs: if: steps.affected_suites.outputs.queue == 'true' run: | set -euo pipefail - python -m pytest -q tests/test_current_head_coalescer_self_cancellation.py + python -m pytest -q tests/test_current_head_coalescer_self_cancellation.py tests/test_required_workflow_queue_contract.py python -m compileall -q tests/test_current_head_coalescer_self_cancellation.py - name: Verify scheduler and contextual-orchestrator review-repair contracts diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index f8ab55c896..de420357da 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -580,49 +580,41 @@ jobs: echo "::add-mask::$app_token" echo "token=$app_token" >>"$GITHUB_OUTPUT" - - name: Validate current pull request head + - name: Admit Noema model work if: env.PR_NUMBER != '' + id: noema_model_admission env: GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} + NOEMA_REVIEW_TOKEN_SOURCE: ${{ steps.noema_credential.outputs.source == 'pat' && 'noema-review-pat' || steps.noema_credential.outputs.source == 'github-app' && 'noema-review-github-app' || 'noema-review-app-oidc' }} + NOEMA_REVIEW_ACTOR: ${{ steps.noema_github_app_token.outputs['app-slug'] && format('{0}[bot]', steps.noema_github_app_token.outputs['app-slug']) || '' }} + NOEMA_REVIEW_INSTALLATION_ID: ${{ steps.noema_github_app_token.outputs['installation-id'] }} run: | set -euo pipefail - if ! [[ "$EXPECTED_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then - echo "::error::Noema expected head must be a full commit SHA." - exit 1 - fi - pull_request_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" - live_state="$(jq -r '.state // empty' <<<"$pull_request_json")" - live_head_sha="$(jq -r '.head.sha // empty' <<<"$pull_request_json")" - if [ "$live_state" != "open" ] || [ "${live_head_sha,,}" != "${EXPECTED_HEAD_SHA,,}" ]; then - printf '::error::Noema review target is closed or stale. expected head=%s; live state=%s head=%s.\n' \ - "$EXPECTED_HEAD_SHA" "${live_state:-missing}" "${live_head_sha:-missing}" - exit 1 + admission_file="${RUNNER_TEMP}/noema-model-admission.json" + rm -f "$admission_file" + python3 "$GITHUB_WORKSPACE/.github/actions/noema-review/two_phase.py" \ + --repo "$TARGET_REPOSITORY" \ + --pr-number "$PR_NUMBER" \ + --expected-head "$EXPECTED_HEAD_SHA" \ + --admit-model-file "$admission_file" + if [ -f "$admission_file" ]; then + visibility="$(jq -er '.repository_visibility | select(. == "public" or . == "private" or . == "internal")' "$admission_file")" + echo "repository_visibility=$visibility" >>"$GITHUB_OUTPUT" + rm -f "$admission_file" + echo "admitted=true" >>"$GITHUB_OUTPUT" + else + echo "admitted=false" >>"$GITHUB_OUTPUT" + echo "::notice::Noema model work is unnecessary for the current pull request state." fi - name: Resolve Noema target repository visibility - if: env.PR_NUMBER != '' + if: env.PR_NUMBER != '' && steps.noema_model_admission.outputs.admitted == 'true' id: target_visibility env: - GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} + REPOSITORY_VISIBILITY: ${{ steps.noema_model_admission.outputs.repository_visibility }} run: | set -euo pipefail - if [ -z "${GH_TOKEN:-}" ]; then - echo "::error::Noema target visibility cannot be resolved without the selected repository-scoped reviewer token." - exit 1 - fi - visibility="" - for target_visibility_attempt in 1 2 3 4 5 6; do - if visibility="$( - gh api "/repos/${TARGET_REPOSITORY}" --jq '.visibility // (if .private then "private" else "public" end)' - )"; then - break - fi - visibility="" - if [ "$target_visibility_attempt" -lt 6 ]; then - echo "Repository visibility lookup failed (attempt ${target_visibility_attempt}/6), possibly a transient GitHub API rate limit; retrying after backoff." >&2 - sleep "$(( target_visibility_attempt * 5 ))" - fi - done + visibility="${REPOSITORY_VISIBILITY:-}" case "$visibility" in private|internal) echo "require_zdr=true" >>"$GITHUB_OUTPUT" @@ -638,7 +630,7 @@ jobs: esac - name: Provision contextual-orchestrator review sidecar - if: env.PR_NUMBER != '' + if: env.PR_NUMBER != '' && steps.noema_model_admission.outputs.admitted == 'true' env: BYTEZ_API_KEY: ${{ secrets.BYTEZ_API_KEY }} NVIDIA_NIM_API_KEY: ${{ secrets.NVIDIA_NIM_API_KEY }} @@ -651,7 +643,7 @@ jobs: bash "$GITHUB_WORKSPACE/scripts/ci/contextual_orchestrator_review_sidecar.sh" - name: Prepare Noema model verdict - if: env.PR_NUMBER != '' + if: env.PR_NUMBER != '' && steps.noema_model_admission.outputs.admitted == 'true' id: noema_prepare env: GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} diff --git a/AGENTS.md b/AGENTS.md index e955f8b36a..92f41f7579 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -212,3 +212,12 @@ them alone proves succession. variable in CI, so a failure class exists that cannot reproduce locally. Before calling a scheduler change clean, run the affected tests both ways, including `GITHUB_ACTIONS=true python3 -m pytest `. +- For consolidated CI, verify all three links: changed-path trigger, actual shell suite + selection, and the selected pytest command. A green job does not cover a changed + contract merely because its filename appears elsewhere in the workflow. Reuse the + existing single-runner job and execute its selector in regression tests; record the + hosted test count separately from a local full-suite result. +- Capture changed-file discovery before iterating it. Bash process substitution + can hide a failed `git diff` despite `set -e`; do not publish all-false suite + outputs when the base cannot be read. Execute the real selector with an invalid + base in its regression test and require failure before any selection output. diff --git a/CHANGELOG.md b/CHANGELOG.md index bf192f6a9e..be36826cf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,9 @@ - Raised `hourly-review-repair.yml`'s discovery ceiling from 50 to 200 while rotating deterministic 50-PR deep-inspection windows by hourly run number. The scheduler hydrates only the selected window and stops immediately after its single dispatch, preserving access to newer PRs without quadrupling expensive review/check/comment work. See `docs/doctoring/hourly-review-repair-single-file-consolidation.md`'s 2026-09-03 follow-up. ## [Unreleased] +- Reuse Noema's exact-head, base, reviewer-identity, Draft, and existing-review + eligibility checks before provisioning its model sidecar, while retaining + the same checks immediately before model work. Refs #1992. - Include merge-scheduler entrypoint, core, and regression-test changes in the existing runtime-quality workflow's trigger and suite selector. Scheduler workflow edits retain queue checks and also select the full review-repair diff --git a/docs/doctoring/noema-orchestrator-free-zdr.md b/docs/doctoring/noema-orchestrator-free-zdr.md index ec1add661a..05999ac27f 100644 --- a/docs/doctoring/noema-orchestrator-free-zdr.md +++ b/docs/doctoring/noema-orchestrator-free-zdr.md @@ -82,3 +82,27 @@ is not Noema review evidence. For GitHub App credentials, reviewer identity is bound to the pinned token mint action's app slug and numeric installation ID. PAT and OIDC credentials continue to resolve their actor through GitHub's authenticated API. + +## Draft 조기 판정 + +`.github` run `34045630637`의 Noema job은 sidecar 준비를 시작한 뒤 +706.63초가 지나서야 Draft 상태를 확인하고 모델 작업을 건너뛰었다. 이 +실행에서 불필요한 sidecar 시작은 1회였다. Issue #1992는 이 값을 관측 +baseline으로 추적한다. + +모델 작업 admission은 reviewer credential을 만든 뒤, repository visibility +조회와 sidecar 준비보다 먼저 실행한다. 판정은 `two_phase.py`의 기존 순서인 +exact head, base, 독립 reviewer actor, Draft, 현재 head의 기존 Noema review를 +그대로 공유한다. 실제 verdict 준비도 같은 판정을 다시 실행하므로 admission +뒤 상태 변경을 신뢰하지 않는다. Noema는 계속 OpenCode 승인과 독립적으로 +실행된다. + +Admission은 closed/stale target을 실패 처리하고 실제 verdict 준비는 기존처럼 +성공적으로 건너뛴다. 공유 판정의 명시적 keyword-only flag가 두 의미를 +분리한다. 이 GraphQL 판정이 같은 reviewer token으로 head/state까지 확인하므로 +바로 앞의 중복 REST 검증 step은 제거했다. 실행마다 REST 호출 1회가 줄지만, +eligible 경로의 추가 GraphQL 조회가 REST와 비용이 같다고 간주하지 않는다. + +로컬 회귀는 Draft와 기존 review에서 sidecar admission marker가 생기지 않는 +것을 확인한다. 새 exact-head hosted 실행에서 불필요한 sidecar 시작이 0회인지 +확인하기 전에는 runtime 개선이 완료됐다고 보지 않는다. Refs #1992. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5ab7e830f3..2c2c0a7056 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -299,6 +299,7 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { + repository { nameWithOwner visibility } number title body diff --git a/tests/test_agent_review_runtime_quality_consolidation.py b/tests/test_agent_review_runtime_quality_consolidation.py index 4592cfd166..11b60fea44 100644 --- a/tests/test_agent_review_runtime_quality_consolidation.py +++ b/tests/test_agent_review_runtime_quality_consolidation.py @@ -6,8 +6,10 @@ workflow_level_cancels_in_progress, ) +import os import re import subprocess +import textwrap from pathlib import Path import pytest @@ -139,6 +141,26 @@ def test_exact_head_is_verified_before_selected_suites_run() -> None: assert 'git diff --name-only "$BASE_SHA...$HEAD_SHA"' in selector +def test_unreadable_base_fails_before_publishing_suite_selection(tmp_path: Path) -> None: + """A failed git diff must not turn every affected suite into a clean skip.""" + step = _workflow_text().split("- name: Select affected contract suites", 1)[1].split( + " - name: Install exact hash-verified base dependencies", 1 + )[0] + script = textwrap.dedent(step.split(" run: |\n", 1)[1]) + head = subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=REPOSITORY_ROOT, text=True + ).strip() + output = tmp_path / "suite_outputs" + result = subprocess.run( + ["bash", "-euo", "pipefail", "-c", script], cwd=REPOSITORY_ROOT, + env={**os.environ, "BASE_SHA": "0" * 40, "HEAD_SHA": head, + "GITHUB_OUTPUT": str(output)}, + text=True, capture_output=True, check=False, + ) + assert result.returncode != 0 + assert not output.exists() + + def test_review_repair_suite_is_selected_and_conditionally_executed() -> None: """Run review-repair contracts only when their owned paths change.""" @@ -207,6 +229,44 @@ def test_commercial_readiness_suite_is_selected_and_conditionally_executed() -> assert "--fail-under=100" in workflow +@pytest.mark.parametrize( + ("changed_path", "suite", "test_path"), + ( + ("scripts/ci/noema_review_gate.py", "noema", "tests/test_noema_review_gate.py"), + ("tests/test_noema_review_gate.py", "noema", "tests/test_noema_review_gate.py"), + ("tests/test_noema_orchestrator_workflow_contract.py", "noema", "tests/test_noema_orchestrator_workflow_contract.py"), + ("tests/test_required_workflow_queue_contract.py", "queue", "tests/test_required_workflow_queue_contract.py"), + ("tests/test_current_head_coalescer_self_cancellation.py", "queue", "tests/test_current_head_coalescer_self_cancellation.py"), + ), +) +def test_admission_changes_select_and_execute_owned_contracts( + changed_path: str, suite: str, test_path: str +) -> None: + """A passing selected job must actually execute the changed gate contracts.""" + workflow = _workflow_text() + trigger = workflow.split("on:\n", 1)[1].split("\nconcurrency:\n", 1)[0] + assert f' - "{changed_path}"' in trigger + selector = workflow.split(' case "$changed_path" in\n', 1)[1].split( + " esac", 1 + )[0] + result = subprocess.run( + ["bash", "-euo", "pipefail", "-c", + 'read -r changed_path\nnoema_suite=false\nqueue_suite=false\n' + 'case "$changed_path" in\n' + selector + + 'esac\nprintf "%s,%s" "$noema_suite" "$queue_suite"'], + input=changed_path + "\n", text=True, capture_output=True, check=True, + ) + assert result.stdout == ("true,false" if suite == "noema" else "false,true") + assert result.stderr == "" + selected_step = workflow.split( + f"if: steps.affected_suites.outputs.{suite} == 'true'\n", 1 + )[1].split("\n - name:", 1)[0] + pytest_command = selected_step.split("python -m pytest -q", 1)[1].split( + "python -m compileall", 1 + )[0] + assert test_path in pytest_command + + def test_exact_artifact_suite_preserves_version_and_quality_contracts() -> None: """Compile on Python 3.10 before running full Python 3.14 evidence.""" diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index 628fa3cbc1..1c3279931e 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -13,6 +13,14 @@ from tests.test_required_workflow_queue_contract import workflow_step, workflow_text +def workflow_step_condition(workflow: str, name: str) -> str: + """Return the executable one-line condition from one named workflow step.""" + step = workflow_step(workflow, name) + match = re.search(r"(?m)^ if:\s*(.+?)\s*$", step) + assert match is not None, f"workflow step {name!r} has no one-line condition" + return match.group(1) + + def test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles( tmp_path: Path, ) -> None: @@ -196,7 +204,27 @@ def test_noema_review_credentials_and_llm_use_orchestrator_free() -> None: assert "OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}" in workflow assert 'export NOEMA_LLM_MODEL="orchestrator/free"' in workflow prepare = workflow_step(workflow, "Prepare Noema model verdict") + admission = workflow_step(workflow, "Admit Noema model work") publish = workflow_step(workflow, "Publish prepared Noema verdict on the exact live head") + assert '--admit-model-file "$admission_file"' in admission + assert "id: noema_model_admission" in admission + assert " - name: Validate current pull request head\n" not in workflow + assert ( + "GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }}" + in admission + ) + admission_condition = ( + "env.PR_NUMBER != '' && steps.noema_model_admission.outputs.admitted == 'true'" + ) + assert workflow_step_condition( + workflow, "Resolve Noema target repository visibility" + ) == admission_condition + assert workflow_step_condition( + workflow, "Provision contextual-orchestrator review sidecar" + ) == admission_condition + assert workflow_step_condition( + workflow, "Prepare Noema model verdict" + ) == admission_condition assert '.github/actions/noema-review/two_phase.py' in prepare assert '--prepare-verdict-file "$verdict_file"' in prepare assert '.github/actions/noema-review/two_phase.py' in publish @@ -211,6 +239,14 @@ def test_noema_review_credentials_and_llm_use_orchestrator_free() -> None: assert "Noema app token is unavailable; review skipped." not in workflow assert "COPILOT_GITHUB_TOKEN" not in workflow assert "secrets: inherit" not in workflow + credential_index = workflow.index(" - name: Select fail-closed Noema reviewer credential\n") + app_token_index = workflow.index(" - name: Mint repository-scoped Noema GitHub App token\n") + oidc_token_index = workflow.index(" - name: Exchange Noema app token through OIDC\n") + admission_index = workflow.index(" - name: Admit Noema model work\n") + visibility_index = workflow.index(" - name: Resolve Noema target repository visibility\n") + sidecar_index = workflow.index(" - name: Provision contextual-orchestrator review sidecar\n") + assert credential_index < app_token_index < oidc_token_index < admission_index + assert admission_index < visibility_index < sidecar_index def _expected_head_from_workflow_run_event(event: dict) -> str: @@ -347,18 +383,34 @@ def test_stale_trigger_step_still_rejects_a_genuinely_different_head( assert "Noema trigger is stale" in result.stdout -def test_noema_visibility_lookup_retries_transient_api_failures() -> None: - """Bound transient GitHub API failures without weakening visibility validation.""" +def test_noema_visibility_reuses_admission_without_lookup(tmp_path: Path) -> None: + """Live admission controls ZDR without duplicate API calls or retry sleeps.""" workflow = workflow_text("noema-review.yml") start = workflow.index(" - name: Resolve Noema target repository visibility") end = workflow.index(" - name: Provision contextual-orchestrator review sidecar", start) visibility_step = workflow[start:end] - assert "for target_visibility_attempt in 1 2 3 4 5 6; do" in visibility_step - assert 'if visibility="$(' in visibility_step - assert 'sleep "$(( target_visibility_attempt * 5 ))"' in visibility_step - assert "possibly a transient GitHub API rate limit; retrying after backoff." in visibility_step - assert "case \"$visibility\" in" in visibility_step + assert "steps.noema_model_admission.outputs.repository_visibility" in visibility_step + assert "gh api" not in visibility_step + assert "sleep " not in visibility_step + admission = workflow_step(workflow, "Admit Noema model work") + assert ".repository_visibility | select(" in admission + assert 'echo "repository_visibility=$visibility"' in admission + script = textwrap.dedent(visibility_step.split(" run: |\n", 1)[1]) + for index, visibility in enumerate(("public", "private", "internal", "", "unknown", "PUBLIC")): + output = tmp_path / f"output-{index}" + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", script], + capture_output=True, text=True, check=False, + env={**os.environ, "REPOSITORY_VISIBILITY": visibility, + "EVENT_REPOSITORY_VISIBILITY": "public", "GITHUB_OUTPUT": str(output)}, + ) + if visibility in ("public", "private", "internal"): + assert result.returncode == 0, result.stderr + assert output.read_text() == f"require_zdr={'false' if visibility == 'public' else 'true'}\n" + else: + assert result.returncode != 0 + assert not output.exists() def test_strix_gateway_default_and_noema_sidecar_fail_closed(tmp_path: Path) -> None: diff --git a/tests/test_noema_two_phase_handoff.py b/tests/test_noema_two_phase_handoff.py index 992522be7b..4ea995c641 100644 --- a/tests/test_noema_two_phase_handoff.py +++ b/tests/test_noema_two_phase_handoff.py @@ -32,6 +32,7 @@ def _patch_live_gate(monkeypatch: pytest.MonkeyPatch, module: ModuleType) -> Non "isDraft": False, "headRefOid": HEAD, "baseRefOid": BASE, + "repository": {"nameWithOwner": "ContextualWisdomLab/example", "visibility": "PUBLIC"}, }, ) monkeypatch.setattr(module.gate, "require_expected_head", lambda _pr, _head: None) @@ -151,6 +152,7 @@ def test_prepare_skip_creates_no_publishable_envelope(tmp_path: Path, monkeypatc "fetch_pr", lambda _repo, _number: { "isDraft": True, + "state": "OPEN", "headRefOid": HEAD, "baseRefOid": BASE, }, @@ -166,6 +168,230 @@ def test_prepare_skip_creates_no_publishable_envelope(tmp_path: Path, monkeypatc assert not envelope.exists() +@pytest.mark.parametrize("skip_kind", ["draft", "existing_review"]) +@pytest.mark.parametrize( + ("operation", "phase"), + [("admit_model_work", "model admission"), ("prepare_verdict", "verdict preparation")], +) +def test_model_admission_skips_ineligible_review_before_sidecar( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + skip_kind: str, + operation: str, + phase: str, + capsys: pytest.CaptureFixture[str], +) -> None: + """The shared prepare predicate must decline model work without fabricating admission.""" + module = _load_module() + _patch_live_gate(monkeypatch, module) + if skip_kind == "draft": + monkeypatch.setattr( + module.gate, + "fetch_pr", + lambda _repo, _number: { + "isDraft": True, + "headRefOid": HEAD, + "baseRefOid": BASE, + }, + ) + else: + monkeypatch.setattr(module.gate, "existing_noema_review", lambda _pr, _actor: True) + marker = tmp_path / "model-admission.json" + + assert getattr(module, operation)("ContextualWisdomLab/example", 7, HEAD, marker) == 0 + assert not marker.exists() + assert f"{phase} skipped." in capsys.readouterr().out + + +@pytest.mark.parametrize( + "pull_request", + [ + {"isDraft": False, "state": "CLOSED", "headRefOid": HEAD, "baseRefOid": BASE}, + {"isDraft": False, "state": "OPEN", "headRefOid": "c" * 40, "baseRefOid": BASE}, + ], +) +def test_model_admission_fails_closed_for_inactive_target( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + pull_request: dict[str, object], +) -> None: + """Admission uses the real exact-head validator and rejects closed or stale work.""" + module = _load_module() + monkeypatch.setattr(module.gate, "fetch_pr", lambda _repo, _number: pull_request) + marker = tmp_path / "model-admission.json" + + with pytest.raises(RuntimeError, match="closed or its head changed"): + module.admit_model_work("ContextualWisdomLab/example", 7, HEAD, marker) + assert not marker.exists() + + +@pytest.mark.parametrize( + "pull_request", + [ + {"isDraft": False, "state": "CLOSED", "headRefOid": HEAD, "baseRefOid": BASE}, + {"isDraft": False, "state": "OPEN", "headRefOid": "c" * 40, "baseRefOid": BASE}, + ], +) +def test_prepare_keeps_closed_or_stale_as_a_successful_skip( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + pull_request: dict[str, object], +) -> None: + """The model phase retains its established successful stale-target retirement.""" + module = _load_module() + monkeypatch.setattr(module.gate, "fetch_pr", lambda _repo, _number: pull_request) + envelope = tmp_path / "verdict.json" + + assert module.prepare_verdict("ContextualWisdomLab/example", 7, HEAD, envelope) == 0 + assert not envelope.exists() + + +def test_model_admission_propagates_pull_request_api_failure( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """GitHub lookup failures cannot become successful admission skips.""" + module = _load_module() + monkeypatch.setattr( + module.gate, + "fetch_pr", + lambda _repo, _number: (_ for _ in ()).throw(RuntimeError("GitHub unavailable")), + ) + + with pytest.raises(RuntimeError, match="GitHub unavailable"): + module.admit_model_work( + "ContextualWisdomLab/example", + 7, + HEAD, + tmp_path / "model-admission.json", + ) + + +def test_model_admission_reuses_prepare_identity_checks( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Eligible model work is admitted only after the current prepare identity checks.""" + module = _load_module() + _patch_live_gate(monkeypatch, module) + marker = tmp_path / "model-admission.json" + + assert module.admit_model_work("ContextualWisdomLab/example", 7, HEAD, marker) == 0 + assert module._read_envelope(marker) == { + "expected_base": BASE, + "expected_head": HEAD, + "pull_request_number": 7, + "repository": "ContextualWisdomLab/example", + "schema_version": module.ENVELOPE_SCHEMA_VERSION, + "repository_visibility": "public", + } + + +@pytest.mark.parametrize("visibility", ["PUBLIC", "PRIVATE", "INTERNAL"]) +def test_admission_reuses_live_repository_visibility(tmp_path, monkeypatch, visibility): + """One existing PR query binds privacy; queued event metadata is not authority.""" + module = _load_module() + monkeypatch.setattr(module.gate, "current_actor", lambda: "cwl-noema-review[bot]") + monkeypatch.setattr(module.gate, "existing_noema_review", lambda *_: False) + monkeypatch.setenv("EVENT_REPOSITORY_VISIBILITY", "PUBLIC") + calls = [] + + def graphql(query, **kwargs): + calls.append(kwargs) + assert "repository { nameWithOwner visibility }" in query + return {"data": {"repository": {"pullRequest": { + "state": "OPEN", "isDraft": False, "headRefOid": HEAD, "baseRefOid": BASE, + "repository": {"nameWithOwner": "contextualwisdomlab/EXAMPLE", "visibility": visibility}, + }}}} + + monkeypatch.setattr(module.gate, "graphql", graphql) + marker = tmp_path / "admission.json" + module.admit_model_work("ContextualWisdomLab/example", 7, HEAD, marker) + assert len(calls) == 1 + assert module._read_envelope(marker)["repository_visibility"] == visibility.lower() + + +@pytest.mark.parametrize("repository", [None, {}, "public", + {"nameWithOwner": "ContextualWisdomLab/other", "visibility": "PUBLIC"}, + {"nameWithOwner": "ContextualWisdomLab/example", "visibility": "UNKNOWN"}, + {"nameWithOwner": "ContextualWisdomLab/example", "visibility": None}, +]) +def test_admission_rejects_unverified_repository_visibility(tmp_path, monkeypatch, repository): + """Unknown privacy or a different repository cannot admit a model request.""" + module = _load_module() + _patch_live_gate(monkeypatch, module) + monkeypatch.setattr(module.gate, "fetch_pr", lambda *_: { + "isDraft": False, "headRefOid": HEAD, "baseRefOid": BASE, "repository": repository, + }) + marker = tmp_path / "admission.json" + with pytest.raises(RuntimeError, match="repository visibility"): + module.admit_model_work("ContextualWisdomLab/example", 7, HEAD, marker) + assert not marker.exists() + + +@pytest.mark.parametrize("invalid_identity", ["head", "base", "actor"]) +def test_model_admission_fails_closed_before_skipping_draft( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + invalid_identity: str, +) -> None: + """Draft status cannot bypass the prepare path's head, base, or actor checks.""" + module = _load_module() + monkeypatch.setattr( + module.gate, + "fetch_pr", + lambda _repo, _number: { + "isDraft": True, + "state": "OPEN", + "headRefOid": HEAD, + "baseRefOid": "short" if invalid_identity == "base" else BASE, + }, + ) + monkeypatch.setattr(module.gate, "PRIMARY_REVIEW_AUTHORS", frozenset({"seonghobae"})) + monkeypatch.setattr( + module.gate, + "current_actor", + lambda: "" if invalid_identity == "actor" else "cwl-noema-review[bot]", + ) + marker = tmp_path / "model-admission.json" + + expected_head = "short" if invalid_identity == "head" else HEAD + with pytest.raises(RuntimeError): + module.admit_model_work("ContextualWisdomLab/example", 7, expected_head, marker) + assert not marker.exists() + + +def test_prepare_rechecks_eligibility_after_admission( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A PR becoming Draft after admission still blocks the model call.""" + module = _load_module() + states = iter((False, True)) + monkeypatch.setattr( + module.gate, + "fetch_pr", + lambda _repo, _number: { + "isDraft": next(states), + "headRefOid": HEAD, + "baseRefOid": BASE, + "repository": {"nameWithOwner": "ContextualWisdomLab/example", "visibility": "PUBLIC"}, + }, + ) + monkeypatch.setattr(module.gate, "require_expected_head", lambda _pr, _head: None) + monkeypatch.setattr(module.gate, "current_actor", lambda: "cwl-noema-review[bot]") + monkeypatch.setattr(module.gate, "PRIMARY_REVIEW_AUTHORS", frozenset({"seonghobae"})) + monkeypatch.setattr(module.gate, "existing_noema_review", lambda _pr, _actor: False) + monkeypatch.setattr(module.gate, "call_llm", lambda *_args: pytest.fail("Draft must not call the model")) + marker = tmp_path / "model-admission.json" + envelope = tmp_path / "verdict.json" + + assert module.admit_model_work("ContextualWisdomLab/example", 7, HEAD, marker) == 0 + assert marker.exists() + assert module.prepare_verdict("ContextualWisdomLab/example", 7, HEAD, envelope) == 0 + assert not envelope.exists() + + def test_publish_cleans_untrusted_envelope_even_when_read_validation_fails(tmp_path: Path) -> None: """Malformed handoff state cannot linger after a failed publication attempt.""" module = _load_module() diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 4ab09b1d0c..84afa190fa 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1078,7 +1078,9 @@ def test_noema_triggers_preserve_standalone_pull_request_review() -> None: assert re.search(r"(?m)^concurrency:", workflow) assert not re.search(r"(?m)^ concurrency:", workflow) assert "needs.admit-current-head.outputs.admitted == 'true'" in noema_job - assert '[ "${live_head_sha,,}" != "${EXPECTED_HEAD_SHA,,}" ]' in workflow + admission = workflow_step(workflow, "Admit Noema model work") + assert '--expected-head "$EXPECTED_HEAD_SHA"' in admission + assert '--admit-model-file "$admission_file"' in admission def test_noema_review_credentials_and_orchestrator_configuration_fail_closed() -> None: