diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 6297ee5ce2..023defa856 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -910,6 +910,48 @@ jobs: --private-target "$PRIVATE_TARGET" echo 'selected=true' >>"$GITHUB_OUTPUT" + - name: Initialize current Noema sidecar evidence + id: noema_evidence + if: env.PR_NUMBER != '' && steps.live_draft.outputs.live_draft != 'true' && steps.personal_route.outputs.selected != 'true' + run: | + set -euo pipefail + # Separate setup from the producer: unsafe paths fail before its outcome + # can admit an upload. Clear only these two files, before early startup. + python3 -I <<'PY' + import os + import stat + from pathlib import Path + + workspace_text = os.environ["GITHUB_WORKSPACE"] + workspace = Path(workspace_text) + if ( + not workspace.is_absolute() + or ".." in workspace.parts + or workspace.resolve(strict=True) != workspace + or not workspace.is_dir() + ): + raise SystemExit("Noema evidence workspace must be an unaliased directory") + evidence = workspace / "strix_runs" + if evidence.is_symlink() or (evidence.exists() and not evidence.is_dir()): + raise SystemExit("Noema evidence parent must be a non-symlink directory") + owned = ( + evidence / "contextual-orchestrator-preflight.json", + evidence / "contextual-orchestrator-sidecar.stderr.log", + ) + # Validate both before deleting either; reject dangling links, special + # files and hardlink aliases as well as nonregular evidence paths. + for path in owned: + try: + metadata = path.lstat() + except FileNotFoundError: + continue + if not stat.S_ISREG(metadata.st_mode) or metadata.st_nlink != 1: + raise SystemExit("Noema evidence must be regular, single-link files") + evidence.mkdir(mode=0o700, exist_ok=True) + for path in owned: + path.unlink(missing_ok=True) + PY + - name: Provision contextual-orchestrator review sidecar id: noema_sidecar if: env.PR_NUMBER != '' && steps.live_draft.outputs.live_draft != 'true' && steps.personal_route.outputs.selected != 'true' @@ -1016,7 +1058,10 @@ jobs: fi - name: Upload contextual-orchestrator sidecar evidence - if: always() && env.PR_NUMBER != '' + # A live Draft never provisions the sidecar. Retain only current producer + # terminal diagnostics; absent/skipped outcomes cannot publish stale files. + # Non-Draft upload failures still fail the required review job. + if: always() && env.PR_NUMBER != '' && steps.live_draft.outputs.live_draft != 'true' && (steps.noema_sidecar.outcome == 'success' || steps.noema_sidecar.outcome == 'failure' || steps.noema_sidecar.outcome == 'cancelled') uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: noema-sidecar-evidence diff --git a/tests/test_noema_draft_admission_before_sidecar.py b/tests/test_noema_draft_admission_before_sidecar.py index 72ecfaf077..5a7e72866e 100644 --- a/tests/test_noema_draft_admission_before_sidecar.py +++ b/tests/test_noema_draft_admission_before_sidecar.py @@ -19,10 +19,19 @@ import textwrap from pathlib import Path +import pytest +import yaml + from tests.test_required_workflow_queue_contract import workflow_step, workflow_text DRAFT_STEP = "Check live pull request draft state before sidecar provisioning" DRAFT_GATE = "steps.live_draft.outputs.live_draft != 'true'" +EXPECTED_UPLOAD_CONDITION = ( + "always() && env.PR_NUMBER != '' && " + DRAFT_GATE + + " && (steps.noema_sidecar.outcome == 'success'" + + " || steps.noema_sidecar.outcome == 'failure'" + + " || steps.noema_sidecar.outcome == 'cancelled')" +) REVIEWER_TOKEN = ( "GH_TOKEN: ${{ env.TARGET_REPOSITORY == 'ContextualWisdomLab/k-csap-skills' && " "steps.noema_github_app_token.outputs.token || (env.TARGET_REPOSITORY != " @@ -49,6 +58,114 @@ def _step_index(job: str, name: str) -> int: return job.index(f" - name: {name}\n") +def _parsed_noema_job(workflow: str) -> dict: + """Read the actual YAML job, retaining GitHub's literal ``on`` key.""" + return yaml.load(workflow, Loader=yaml.BaseLoader)["jobs"]["noema-review"] + + +def _sidecar_upload_is_fail_closed(workflow: str) -> bool: + """Bind the exact gate to one case-normalized action, never a display-name decoy.""" + uploads = [ + step for step in _parsed_noema_job(workflow)["steps"] + if str(step.get("uses", "")).casefold().startswith("actions/upload-artifact@") + ] + if len(uploads) != 1: + return False + upload = uploads[0] + return ( + upload.get("name") == "Upload contextual-orchestrator sidecar evidence" + and upload.get("if") == EXPECTED_UPLOAD_CONDITION + and upload.get("with", {}).get("name") == "noema-sidecar-evidence" + and "continue-on-error" not in upload + ) + + +def _noema_review_job_is_fail_closed(workflow: str) -> bool: + """The review job itself must propagate upload and producer failures.""" + return "continue-on-error" not in _parsed_noema_job(workflow) + + +def _upload_admitted(workflow: str, pr: str, draft: str, outcome: str) -> bool: + """Evaluate the actual condition's bounded grammar; unknown expressions fail.""" + uploads = [ + step for step in _parsed_noema_job(workflow)["steps"] + if str(step.get("uses", "")).casefold().startswith("actions/upload-artifact@") + ] + assert len(uploads) == 1 + terms = { + "always()": True, + "env.PR_NUMBER != ''": bool(pr), + DRAFT_GATE: draft != "true", + "steps.noema_sidecar.outcome == 'success'": outcome == "success", + "steps.noema_sidecar.outcome == 'failure'": outcome == "failure", + "steps.noema_sidecar.outcome == 'cancelled'": outcome == "cancelled", + } + values = [] + for part in uploads[0]["if"].split(" && "): + if part.startswith("(") and part.endswith(")"): + alternatives = part[1:-1].split(" || ") + assert all(term in terms for term in alternatives), part + values.append(any(terms[term] for term in alternatives)) + else: + assert part in terms, part + values.append(terms[part]) + return all(values) + + +@pytest.mark.parametrize("pr", ["", "2565"]) +@pytest.mark.parametrize("draft", ["true", "false", ""]) +@pytest.mark.parametrize("outcome", ["", "skipped", "unknown", "success", "failure", "cancelled"]) +def test_upload_admission_requires_current_terminal_producer(pr: str, draft: str, outcome: str) -> None: + """Absent/skipped producers cannot publish stale files; terminal diagnostics survive.""" + admitted = _upload_admitted(workflow_text("noema-review.yml"), pr, draft, outcome) + assert admitted == ( + bool(pr) and draft != "true" and outcome in {"success", "failure", "cancelled"} + ), (pr, draft, outcome) + + +def test_upload_action_identity_oracle_rejects_decoys_and_failure_bypasses() -> None: + """Keep the owner's unique casefold oracle and quoted/spaced failure controls.""" + workflow = workflow_text("noema-review.yml") + assert _sidecar_upload_is_fail_closed(workflow) + assert _noema_review_job_is_fail_closed(workflow) + assert not _sidecar_upload_is_fail_closed( + workflow.replace(EXPECTED_UPLOAD_CONDITION, EXPECTED_UPLOAD_CONDITION + " || true", 1) + ) + marker = " - name: Upload contextual-orchestrator sidecar evidence\n" + for bypass in ("continue-on-error: true", '"continue-on-error": true', "continue-on-error : true"): + assert not _sidecar_upload_is_fail_closed( + workflow.replace(marker, marker + " " + bypass + "\n", 1) + ), bypass + assert not _noema_review_job_is_fail_closed( + workflow.replace(" noema-review:\n", " noema-review:\n " + bypass + "\n", 1) + ), bypass + run_decoy = marker + f" if: {EXPECTED_UPLOAD_CONDITION}\n run: 'true'\n\n" + assert _sidecar_upload_is_fail_closed(workflow.replace(marker, run_decoy + marker, 1)) + assert not _sidecar_upload_is_fail_closed( + workflow.replace(marker, run_decoy + marker + " continue-on-error: true\n", 1) + ) + action = next( + step["uses"] for step in _parsed_noema_job(workflow)["steps"] + if str(step.get("uses", "")).casefold().startswith("actions/upload-artifact@") + ) + mixed_case = workflow.replace( + f"uses: {action}", f"uses: Actions/Upload-Artifact@{action.split('@', 1)[1]}", 1 + ) + assert mixed_case != workflow + assert _sidecar_upload_is_fail_closed(mixed_case) + action_decoy = ( + marker + f" if: {EXPECTED_UPLOAD_CONDITION}\n uses: {action}\n" + + " with:\n name: noema-sidecar-evidence\n path: CHANGELOG.md\n\n" + ) + assert not _sidecar_upload_is_fail_closed(mixed_case.replace(marker, action_decoy + marker, 1)) + assert not _sidecar_upload_is_fail_closed(mixed_case.replace( + marker, action_decoy + marker + " continue-on-error: true\n", 1 + )) + assert _noema_review_job_is_fail_closed(workflow.replace( + " continue-noema-transport:\n", " continue-noema-transport:\n continue-on-error: true\n", 1 + )) + + def test_live_draft_check_runs_after_head_validation_and_before_sidecar() -> None: """The draft decision sits between live-head validation and model provisioning.""" job = _noema_job() @@ -121,9 +238,8 @@ def test_downstream_publication_treats_unset_prepare_outputs_as_skipped() -> Non assert "needs.noema-review.result == 'failure'" in continuation assert "needs.noema-review.outputs.transport_capacity_unavailable == 'true'" in continuation assert "needs.noema-review.outputs.transport_retry_eligible == 'true'" in continuation - assert "if: always() && env.PR_NUMBER != ''" in workflow_step( - workflow, "Upload contextual-orchestrator sidecar evidence" - ) + assert _noema_review_job_is_fail_closed(workflow) + assert _sidecar_upload_is_fail_closed(workflow) def test_trigger_types_are_unchanged_by_the_runtime_draft_check() -> None: @@ -141,6 +257,7 @@ def _run_draft_step(tmp_path: Path, gh_body: str, gh_token: str = "synthetic-tok script = textwrap.dedent( workflow_step(workflow_text("noema-review.yml"), DRAFT_STEP).split(" run: |\n", 1)[1] ) + script = script.replace("/tmp/noema-live-draft-error", str(tmp_path / "live-draft-error")) fake_gh = tmp_path / "gh" fake_gh.write_text(f"#!/usr/bin/env bash\n{gh_body}\n", encoding="utf-8") fake_gh.chmod(0o755) diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index 327856ea7c..0f4ad7988a 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -10,6 +10,13 @@ import textwrap from pathlib import Path +import pytest + +from tests.test_noema_draft_admission_before_sidecar import ( + EXPECTED_UPLOAD_CONDITION, + _parsed_noema_job, + _sidecar_upload_is_fail_closed, +) from tests.test_required_workflow_queue_contract import workflow_step, workflow_text @@ -704,28 +711,166 @@ def test_noema_review_job_has_no_job_level_timeout() -> None: def test_noema_review_retains_sanitized_sidecar_evidence_after_any_outcome() -> None: - """Retain existing sanitized evidence on success, failure and cancellation. + """Only current non-Draft terminal producers retain the two sanitized paths. - A forced runner shutdown can still prevent upload; this contract only - removes the failure-only gate without adding raw logs or new files. + Forced runner shutdown may still prevent upload. This local contract does + not prove hosted retention, inference, or a qualifying App-owned approval. """ workflow = workflow_text("noema-review.yml") name = "Upload contextual-orchestrator sidecar evidence" - step = workflow_step(workflow, name) - assert "if: always() && env.PR_NUMBER != ''" in step + uploads = [ + step for step in _parsed_noema_job(workflow)["steps"] + if str(step.get("uses", "")).casefold().startswith("actions/upload-artifact@") + ] + assert len(uploads) == 1 + step = uploads[0] + assert _sidecar_upload_is_fail_closed(workflow) + assert step["if"] == EXPECTED_UPLOAD_CONDITION strix_pin = re.search( r"actions/upload-artifact@([0-9a-f]{40})", workflow_text("strix.yml") ).group(1) - assert f"actions/upload-artifact@{strix_pin}" in step - assert "name: noema-sidecar-evidence" in step - assert "strix_runs/contextual-orchestrator-sidecar.stderr.log" in step - assert "strix_runs/contextual-orchestrator-preflight.json" in step - assert "if-no-files-found: ignore" in step - assert "retention-days: 5" in step + assert step["uses"] == f"actions/upload-artifact@{strix_pin}" + assert step["with"] == { + "name": "noema-sidecar-evidence", + "path": "strix_runs/contextual-orchestrator-sidecar.stderr.log\n" + "strix_runs/contextual-orchestrator-preflight.json\n", + "if-no-files-found": "ignore", + "retention-days": "5", + } prepare = workflow.index(" - name: Prepare Noema model verdict\n") upload = workflow.index(f" - name: {name}\n") refresh = workflow.index( " - name: Refresh repository-scoped Noema GitHub App token for publication\n" ) assert prepare < upload < refresh - assert workflow.count("actions/upload-artifact@") == 1 + + +OWNED_EVIDENCE = ( + "contextual-orchestrator-preflight.json", + "contextual-orchestrator-sidecar.stderr.log", +) + + +def _evidence_lifecycle_steps() -> tuple[dict, dict]: + """Read initialization and the real producer by parsed identities, in order.""" + steps = _parsed_noema_job(workflow_text("noema-review.yml"))["steps"] + producer = next(step for step in steps if step.get("id") == "noema_sidecar") + # Until initialization exists, execute the defective producer prefix for RED. + initializer = next((step for step in steps if step.get("id") == "noema_evidence"), producer) + return initializer, producer + + +def _run_evidence_lifecycle(workspace: Path, phase: str, marker: Path) -> subprocess.CompletedProcess[str]: + """Execute actual setup/run bodies with a harmless early-failure sidecar.""" + initializer, producer = _evidence_lifecycle_steps() + sidecar = workspace / "scripts/ci/contextual_orchestrator_review_sidecar.sh" + sidecar.parent.mkdir(parents=True, exist_ok=True) + sidecar.write_text( + '#!/bin/bash\nset -euo pipefail\nprintf reached >"$PHASE_MARKER"\n' + 'if [ "$PHASE" = early ]; then exit 23; fi\n' + 'printf "current sanitized diagnostic\\n" >"$GITHUB_WORKSPACE/strix_runs/contextual-orchestrator-sidecar.stderr.log"\n' + 'printf \'{"contract":"strix-plain-chat-preflight-v2","ready_count":0,"probed_count":1,\n' + '"routes":[{"status":"rejected","http_status":429}]}\\n\' ' + '>"$GITHUB_WORKSPACE/strix_runs/contextual-orchestrator-preflight.json"\n' + 'if [ "$PHASE" = failure ]; then exit 23; fi\n', + encoding="utf-8", + ) + bodies = [initializer["run"]] if initializer is producer else [initializer["run"], producer["run"]] + env = {**os.environ, "GITHUB_WORKSPACE": str(workspace), "PHASE": phase, "PHASE_MARKER": str(marker)} + result = None + for body in bodies: + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", body], + env=env, capture_output=True, text=True, check=False, timeout=5, + ) + if result.returncode: + break # Actions default success() skips the producer after setup failure. + assert result is not None + return result + + +def test_evidence_initialization_failure_skips_actual_producer() -> None: + """An unsafe setup must not become a terminal producer eligible for upload.""" + initializer, producer = _evidence_lifecycle_steps() + steps = _parsed_noema_job(workflow_text("noema-review.yml"))["steps"] + assert initializer.get("id") == "noema_evidence" + assert steps.index(initializer) < steps.index(producer) + assert initializer["if"] == producer["if"] + assert "always()" not in producer["if"] + assert "continue-on-error" not in initializer + assert "continue-on-error" not in producer + + +@pytest.mark.parametrize("phase", ["early", "failure", "success"]) +def test_evidence_lifecycle_clears_stale_files_before_sidecar(tmp_path: Path, phase: str) -> None: + """Repeat actual initialization; preserve sentinels and genuine current diagnostics.""" + workspace = tmp_path / "workspace" + evidence = workspace / "strix_runs" + evidence.mkdir(parents=True) + sentinel = evidence / "unrelated-sentinel" + sentinel.write_text("keep unrelated\n") + marker = tmp_path / "producer-reached" + for _ in range(2): + marker.unlink(missing_ok=True) + for name in OWNED_EVIDENCE: + (evidence / name).write_text("old evidence\n") + result = _run_evidence_lifecycle(workspace, phase, marker) + assert marker.read_text() == "reached" + assert result.returncode == (0 if phase == "success" else 23), result.stderr + assert sentinel.read_text() == "keep unrelated\n" + if phase == "early": + assert all(not (evidence / name).exists() for name in OWNED_EVIDENCE) + else: + assert (evidence / OWNED_EVIDENCE[1]).read_text() == "current sanitized diagnostic\n" + report = json.loads((evidence / OWNED_EVIDENCE[0]).read_text()) + assert report["contract"] == "strix-plain-chat-preflight-v2" + assert report["routes"][0]["http_status"] == 429 + assert sorted(p.name for p in evidence.iterdir()) == ( + ["unrelated-sentinel"] if phase == "early" else sorted((*OWNED_EVIDENCE, "unrelated-sentinel")) + ) + + +@pytest.mark.parametrize("alias", ["linked-parent", "linked-workspace", "linked-evidence", "dotdot", + "symlink-preflight", "symlink-stderr", "hardlink-preflight", "hardlink-stderr", + "directory-preflight", "directory-stderr"]) +def test_evidence_initialization_rejects_path_aliases_without_deletion(tmp_path: Path, alias: str) -> None: + """Reject aliases before deleting either owned file or reaching the sidecar.""" + actual = tmp_path / "real/workspace" + evidence = actual / "strix_runs" + evidence.mkdir(parents=True) + sentinel = tmp_path / "unrelated-sentinel" + sentinel.write_text("keep outside\n") + for name in OWNED_EVIDENCE: + (evidence / name).write_text("old evidence\n") + workspace = actual + if alias == "linked-parent": + (tmp_path / "alias").symlink_to(actual.parent, target_is_directory=True) + workspace = tmp_path / "alias/workspace" + elif alias == "linked-workspace": + workspace = tmp_path / "alias" + workspace.symlink_to(actual, target_is_directory=True) + elif alias == "linked-evidence": + evidence.rename(actual / "outside") + evidence.symlink_to(actual / "outside", target_is_directory=True) + elif alias == "dotdot": + (actual / "child").mkdir() + workspace = actual / "child/.." + else: + index = 0 if alias.endswith("preflight") else 1 + target = evidence / OWNED_EVIDENCE[index] + target.unlink() + if alias.startswith("symlink"): + target.symlink_to(sentinel) + elif alias.startswith("hardlink"): + target.hardlink_to(sentinel) + else: + target.mkdir() + marker = tmp_path / "producer-reached" + result = _run_evidence_lifecycle(workspace, "early", marker) + assert result.returncode != 0 + assert not marker.exists(), result.stderr + assert sentinel.read_text() == "keep outside\n" + assert all((evidence / name).exists() for name in OWNED_EVIDENCE) + for path in evidence.iterdir(): + if path.is_file() and not path.is_symlink() and path.stat().st_nlink == 1: + assert path.read_text() == "old evidence\n"