diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d4d11c2bce7e..34cd4ffbb121 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -248,8 +248,8 @@ jobs: # ───────────────────────────────────────────────────────────────────── # Gate: runs after everything. ``if: always()`` ensures it reports a - # status even when some deps were skipped. Only actual ``failure`` - # results cause it to fail; ``skipped`` is treated as success. + # status even when some deps were skipped. Only ``success`` and a + # classifier-consistent ``skipped`` result pass the gate. # # Branch protection should require ONLY this check. # @@ -282,29 +282,13 @@ jobs: outputs: needs-json: ${{ steps.evaluate.outputs.needs-json }} steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Evaluate job results id: evaluate env: NEEDS: ${{ toJSON(needs) }} run: | - echo "$NEEDS" | python3 -c " - import json, sys - needs = json.load(sys.stdin) - # Emit compact {job_name: result} for the comment assembler. - compact = {name: info['result'] for name, info in needs.items()} - print(f'needs-json={json.dumps(compact)}') - with open('$GITHUB_OUTPUT', 'a') as f: - f.write(f'needs-json={json.dumps(compact)}\n') - failed = [name for name, info in needs.items() if info['result'] == 'failure'] - for name, info in sorted(needs.items()): - result = info['result'] - icon = '✅' if result in ('success', 'skipped') else '❌' - print(f'{icon} {name}: {result}') - if failed: - print(f'::error::{len(failed)} job(s) failed: {\", \".join(failed)}') - sys.exit(1) - print('All checks passed (or were skipped)') - " + echo "$NEEDS" | python3 scripts/ci/evaluate_needs.py # ───────────────────────────────────────────────────────────────────── # CI timing report: collect per-job/step durations from the GitHub API, diff --git a/scripts/ci/evaluate_needs.py b/scripts/ci/evaluate_needs.py new file mode 100644 index 000000000000..3171e636f340 --- /dev/null +++ b/scripts/ci/evaluate_needs.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python3 +"""Evaluate results consumed by CI's all-checks-pass umbrella job.""" + +from __future__ import annotations + +import json +import os +import sys +from collections.abc import Mapping +from typing import Any + + +_ALLOWED_RESULTS = {"success", "skipped"} + + +def _expected_jobs( + needs: Mapping[str, Mapping[str, Any]], +) -> dict[str, tuple[bool, str]]: + detect = needs.get("detect", {}).get("outputs", {}) + supply_chain = needs.get("supply-chain", {}).get("outputs", {}) + pull_request = detect.get("event_name") == "pull_request" + python = detect.get("python") == "true" + frontend = detect.get("frontend") == "true" + + return { + "tests": (python, "detect.outputs.python == 'true'"), + "lint": (python, "detect.outputs.python == 'true'"), + "js-tests": (frontend, "detect.outputs.frontend == 'true'"), + "e2e-desktop": ( + python or frontend, + "detect.outputs.python == 'true' or detect.outputs.frontend == 'true'", + ), + "docs-site": ( + detect.get("site") == "true", + "detect.outputs.site == 'true'", + ), + "history-check": ( + pull_request, + "detect.outputs.event_name == 'pull_request'", + ), + "contributor-check": (python, "detect.outputs.python == 'true'"), + "lockfile-diff": ( + pull_request and detect.get("npm_lock") == "true", + "pull_request and detect.outputs.npm_lock == 'true'", + ), + "docker-lint": ( + detect.get("docker_meta") == "true", + "detect.outputs.docker_meta == 'true'", + ), + "supply-chain": ( + pull_request + and (detect.get("scan") == "true" or detect.get("deps") == "true"), + "pull_request and (detect.outputs.scan == 'true' or " + "detect.outputs.deps == 'true')", + ), + "review-labels": ( + pull_request + and ( + detect.get("ci_review") == "true" + or detect.get("mcp_catalog") == "true" + or supply_chain.get("critical_findings") == "true" + ), + "pull_request and a review-label trigger is true", + ), + } + + +def evaluate_needs(needs: Mapping[str, Mapping[str, Any]]) -> dict[str, str]: + """Return jobs whose results must fail the umbrella gate.""" + violations = {} + for name, info in needs.items(): + result = info["result"] + if result not in _ALLOWED_RESULTS: + violations[name] = ( + f"{name} concluded {result!r}; expected 'success' or 'skipped'" + ) + + for name, (expected, reason) in _expected_jobs(needs).items(): + if expected and needs.get(name, {}).get("result") == "skipped": + violations[name] = ( + f"classifier inconsistency: {name} was skipped even though {reason}" + ) + return violations + + +def main() -> int: + needs = json.load(sys.stdin) + # Emit compact {job_name: result} for the comment assembler. + compact = {name: info["result"] for name, info in needs.items()} + output = f"needs-json={json.dumps(compact)}" + print(output) + with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as fh: + fh.write(output + "\n") + + failed = evaluate_needs(needs) + for name, info in sorted(needs.items()): + result = info["result"] + icon = "✅" if name not in failed else "❌" + print(f"{icon} {name}: {result}") + if failed: + for message in failed.values(): + print(f"::error::{message}") + print(f"::error::{len(failed)} job(s) failed: {', '.join(failed)}") + return 1 + print("All checks passed (or were skipped)") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/ci/test_evaluate_needs.py b/tests/ci/test_evaluate_needs.py new file mode 100644 index 000000000000..4545262383bd --- /dev/null +++ b/tests/ci/test_evaluate_needs.py @@ -0,0 +1,275 @@ +"""Tests for the CI all-checks-pass evaluator.""" + +from __future__ import annotations + +import importlib.util +import io +import json +import sys +from pathlib import Path + +import pytest + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "evaluate_needs.py" +_spec = importlib.util.spec_from_file_location("evaluate_needs", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load evaluate_needs.py") +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) +evaluate_needs = _mod.evaluate_needs + +_JOBS = ( + "detect", + "tests", + "lint", + "js-tests", + "e2e-desktop", + "docs-site", + "history-check", + "contributor-check", + "uv-lockfile", + "lockfile-diff", + "docker-lint", + "supply-chain", + "review-labels", + "osv-scanner", +) +_FALSE_DETECT_OUTPUTS = { + "python": "false", + "frontend": "false", + "site": "false", + "scan": "false", + "deps": "false", + "npm_lock": "false", + "docker_meta": "false", + "mcp_catalog": "false", + "ci_review": "false", + "event_name": "pull_request", +} + + +def _needs( + *, + results: dict[str, str] | None = None, + detect_outputs: dict[str, str] | None = None, + job_outputs: dict[str, dict[str, str]] | None = None, +) -> dict[str, dict]: + needs = {name: {"result": "success", "outputs": {}} for name in _JOBS} + needs["detect"]["outputs"] = {**_FALSE_DETECT_OUTPUTS, **(detect_outputs or {})} + for name, result in (results or {}).items(): + needs[name]["result"] = result + for name, outputs in (job_outputs or {}).items(): + needs[name]["outputs"] = outputs + return needs + + +def _old_failed_jobs(needs: dict[str, dict]) -> list[str]: + """The pre-fix evaluator: only literal ``failure`` was rejected.""" + return [name for name, info in needs.items() if info["result"] == "failure"] + + +def test_all_success_passes(): + assert evaluate_needs(_needs()) == {} + + +def test_failure_result_fails(): + violations = evaluate_needs(_needs(results={"lint": "failure"})) + + assert set(violations) == {"lint"} + assert "failure" in violations["lint"] + + +def test_cancelled_result_fails_despite_old_logic_accepting_it(): + needs = _needs(results={"tests": "cancelled"}) + + assert _old_failed_jobs(needs) == [] + violations = evaluate_needs(needs) + assert set(violations) == {"tests"} + assert "cancelled" in violations["tests"] + + +def test_unrecognized_result_fails(): + violations = evaluate_needs(_needs(results={"lint": "timed_out"})) + + assert set(violations) == {"lint"} + assert "timed_out" in violations["lint"] + + +def test_tests_skipped_while_python_changed_fails(): + violations = evaluate_needs( + _needs(results={"tests": "skipped"}, detect_outputs={"python": "true"}) + ) + + assert set(violations) == {"tests"} + assert "detect.outputs.python == 'true'" in violations["tests"] + + +def test_tests_skipped_while_python_unchanged_passes(): + assert evaluate_needs(_needs(results={"tests": "skipped"})) == {} + + +@pytest.mark.parametrize( + ("job", "detect_outputs", "job_outputs", "reason"), + [ + pytest.param( + "lint", + {"python": "true"}, + {}, + "detect.outputs.python == 'true'", + id="lint-python", + ), + pytest.param( + "js-tests", + {"frontend": "true"}, + {}, + "detect.outputs.frontend == 'true'", + id="js-tests-frontend", + ), + pytest.param( + "e2e-desktop", + {"python": "true"}, + {}, + "detect.outputs.python == 'true' or detect.outputs.frontend == 'true'", + id="desktop-python", + ), + pytest.param( + "e2e-desktop", + {"frontend": "true"}, + {}, + "detect.outputs.python == 'true' or detect.outputs.frontend == 'true'", + id="desktop-frontend", + ), + pytest.param( + "docs-site", + {"site": "true"}, + {}, + "detect.outputs.site == 'true'", + id="docs-site", + ), + pytest.param( + "history-check", + {}, + {}, + "detect.outputs.event_name == 'pull_request'", + id="history-check-pull-request", + ), + pytest.param( + "contributor-check", + {"python": "true"}, + {}, + "detect.outputs.python == 'true'", + id="contributor-check-python", + ), + pytest.param( + "lockfile-diff", + {"npm_lock": "true"}, + {}, + "pull_request and detect.outputs.npm_lock == 'true'", + id="lockfile-diff", + ), + pytest.param( + "docker-lint", + {"docker_meta": "true"}, + {}, + "detect.outputs.docker_meta == 'true'", + id="docker-lint", + ), + pytest.param( + "supply-chain", + {"scan": "true"}, + {}, + "pull_request and (detect.outputs.scan == 'true' or detect.outputs.deps == 'true')", + id="supply-chain-scan", + ), + pytest.param( + "supply-chain", + {"deps": "true"}, + {}, + "pull_request and (detect.outputs.scan == 'true' or detect.outputs.deps == 'true')", + id="supply-chain-deps", + ), + pytest.param( + "review-labels", + {"ci_review": "true"}, + {}, + "pull_request and a review-label trigger is true", + id="review-labels-ci-review", + ), + pytest.param( + "review-labels", + {"mcp_catalog": "true"}, + {}, + "pull_request and a review-label trigger is true", + id="review-labels-mcp-catalog", + ), + pytest.param( + "review-labels", + {}, + {"supply-chain": {"critical_findings": "true"}}, + "pull_request and a review-label trigger is true", + id="review-labels-supply-chain", + ), + ], +) +def test_classifier_required_job_cannot_be_skipped( + job: str, + detect_outputs: dict[str, str], + job_outputs: dict[str, dict[str, str]], + reason: str, +): + violations = evaluate_needs( + _needs( + results={job: "skipped"}, + detect_outputs=detect_outputs, + job_outputs=job_outputs, + ) + ) + + assert set(violations) == {job} + assert reason in violations[job] + + +def test_js_tests_skipped_while_frontend_unchanged_passes(): + assert evaluate_needs(_needs(results={"js-tests": "skipped"})) == {} + + +def test_classifier_jobs_may_skip_when_no_condition_requires_them(): + classifier_jobs = { + "tests", + "lint", + "js-tests", + "e2e-desktop", + "docs-site", + "history-check", + "contributor-check", + "lockfile-diff", + "docker-lint", + "supply-chain", + "review-labels", + } + + assert ( + evaluate_needs( + _needs( + results={name: "skipped" for name in classifier_jobs}, + detect_outputs={"event_name": "push"}, + ) + ) + == {} + ) + + +def test_main_preserves_compact_needs_json_shape(tmp_path, monkeypatch, capsys): + needs = { + "detect": {"result": "success", "outputs": dict(_FALSE_DETECT_OUTPUTS)}, + "tests": {"result": "skipped", "outputs": {}}, + } + output = tmp_path / "github-output" + monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(needs))) + monkeypatch.setenv("GITHUB_OUTPUT", str(output)) + + assert _mod.main() == 0 + + expected = 'needs-json={"detect": "success", "tests": "skipped"}' + assert capsys.readouterr().out.splitlines()[0] == expected + assert output.read_text(encoding="utf-8") == expected + "\n"