diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 000000000..60e98ef7f --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,7 @@ +# Rule machinery: changes need review from a core developer other than the +# author. Deliberately narrow: pyproject.toml files and workflow files are not +# owned, so dependency bots and everyday CI edits are not blocked. +/rules/ @PolicyEngine/core-developers +/scripts/rules/ @PolicyEngine/core-developers +/scripts/run_quality_guards.py @PolicyEngine/core-developers +/.github/CODEOWNERS @PolicyEngine/core-developers diff --git a/.github/scripts/check_ci_complete.sh b/.github/scripts/check_ci_complete.sh new file mode 100755 index 000000000..bcfb70a92 --- /dev/null +++ b/.github/scripts/check_ci_complete.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash + +# Gate behind the "CI complete" check in pr.yml. +# +# In a stack of pull requests, only the lowest unmerged one runs the full +# workflow; the jobs of the others are skipped to save runners. A skipped job +# counts as passing for a required check, so this gate fails on those pull +# requests instead. On every other pull request it passes only when every job +# it needs passed. +# +# Environment: +# JOB_RESULTS toJSON(needs) for the gate job +# PULL_REQUEST_BASE_REF github.event.pull_request.base.ref +# STACK_BASE_REF github.event.pull_request.stack.base.ref, empty when +# the pull request isn't in a stack + +set -euo pipefail + +job_results="${JOB_RESULTS:?JOB_RESULTS is required}" +pull_request_base_ref="${PULL_REQUEST_BASE_REF:?PULL_REQUEST_BASE_REF is required}" +stack_base_ref="${STACK_BASE_REF:-}" + +if [[ -n "${stack_base_ref}" && "${stack_base_ref}" != "${pull_request_base_ref}" ]]; then + echo "::error::Full CI runs only on the lowest unmerged pull request in a stack. This one targets ${pull_request_base_ref}; it runs once the pull request below it merges and this one is rebased onto ${stack_base_ref}." + exit 1 +fi + +unsuccessful_jobs="$( + jq -r 'to_entries[] | select(.value.result != "success") | "\(.key): \(.value.result)"' <<<"${job_results}" +)" + +if [[ -n "${unsuccessful_jobs}" ]]; then + while IFS= read -r unsuccessful_job; do + echo "::error::Job did not pass: ${unsuccessful_job}" + done <<<"${unsuccessful_jobs}" + exit 1 +fi + +echo "Every job passed." diff --git a/.github/workflows/pr-image-smoke.yml b/.github/workflows/pr-image-smoke.yml index 23d3a875f..d1f91e667 100644 --- a/.github/workflows/pr-image-smoke.yml +++ b/.github/workflows/pr-image-smoke.yml @@ -18,6 +18,7 @@ on: - 'libs/policyengine-simulation-contract/**' - 'libs/policyengine-simulation-observability/**' - 'libs/policyengine-fastapi/**' + - 'libs/policyengine-stage12-persistence/**' - '.github/workflows/pr-image-smoke.yml' - '.github/scripts/modal-image-smoke.sh' @@ -29,7 +30,9 @@ jobs: image-smoke: name: Image smoke (staging) runs-on: ubuntu-latest - if: github.event.pull_request.head.repo.full_name == github.repository + # Same-repository pull requests only, and in a stack only the lowest + # unmerged one (see docs/engineering/skills/github-prs.md). + if: ${{ github.event.pull_request.head.repo.full_name == github.repository && (!github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref) }} # Warm cache: ~2 min. After a relock the executor smoke pays the # certified dataset download layer (~15-20 min). timeout-minutes: 45 diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index f4b4916f1..c71f1c4a2 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -9,8 +9,36 @@ concurrency: cancel-in-progress: true jobs: + ci-complete: + # The required check. Needs every other job: on a stacked pull request + # above the lowest unmerged one it fails, because those jobs were skipped + # and a skipped job would count as passing; otherwise it passes only when + # every job passed. See docs/engineering/skills/github-prs.md. + name: CI complete + if: ${{ always() }} + needs: [test, lint, docker-build, test-integration] + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - name: Checkout the gate script + uses: actions/checkout@v6 + with: + sparse-checkout: .github/scripts/check_ci_complete.sh + sparse-checkout-cone-mode: false + + - name: Require full CI, with every job passing + env: + JOB_RESULTS: ${{ toJSON(needs) }} + PULL_REQUEST_BASE_REF: ${{ github.event.pull_request.base.ref }} + STACK_BASE_REF: ${{ github.event.pull_request.stack.base.ref }} + run: bash .github/scripts/check_ci_complete.sh + test: name: Test + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest strategy: matrix: @@ -24,6 +52,7 @@ jobs: - libs/policyengine-simulation-contract - libs/policyengine-simulation-observability - libs/policyengine-stage12-persistence + - libs/policyengine-fastapi steps: - uses: actions/checkout@v6 @@ -50,6 +79,8 @@ jobs: lint: name: Lint + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest steps: @@ -64,7 +95,8 @@ jobs: uses: astral-sh/setup-uv@v8.1.0 - name: Install ruff - run: uv tool install ruff + # Pinned so a ruff release cannot change CI results; see ruff.toml. + run: uv tool install ruff==0.15.22 - name: Run ruff format check run: | diff --git a/AGENTS.md b/AGENTS.md index 6efa4b64c..0f75d5d42 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,8 @@ names, logging, or observability failure handling, read ## GitHub PRs Never open `policyengine-sim-api` PRs from forks. CI and deployment checks are -designed for same-repository branches. +designed for same-repository branches. Agents must never bypass branch rules +(for example with `gh pr merge --admin`), even with admin credentials. Before creating or sharing any PR, all developers and agents must: diff --git a/Makefile b/Makefile index d4e751181..dd81d034b 100644 --- a/Makefile +++ b/Makefile @@ -83,7 +83,8 @@ test: projects/policyengine-simulation-gateway \ libs/policyengine-simulation-contract \ libs/policyengine-simulation-observability \ - libs/policyengine-stage12-persistence; do \ + libs/policyengine-stage12-persistence \ + libs/policyengine-fastapi; do \ echo "Testing $$proj..."; \ (cd "$$proj" && uv sync --extra test && uv run pytest tests/ -v) || exit 1; \ done diff --git a/changelog_entry.yaml b/changelog_entry.yaml index 066e85eb1..ff1c790ca 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -1,5 +1,9 @@ - bump: patch changes: + changed: + - Added CODEOWNERS for the rule machinery, pinned CI's ruff to 0.15.22 with an explicit rule selection, and documented that agents never bypass branch rules + - Ran the Modal image smoke for Stage 12 persistence changes and added policyengine-fastapi to the PR test matrix + - Ran the full PR workflow and the Modal image smoke only on the lowest unmerged pull request in a stack, behind a new CI complete gate job that fails on the others fixed: - Preserved bounded reform stop dates in UK and US simulations while retaining current-law baseline artifact reuse - Resolved PolicyEngine-managed regional datasets using bundled data versions instead of release-manifest commit hashes when loading from GCS diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index a7b2e71ff..2dce64f4c 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -33,6 +33,36 @@ repository, stop and ask for access. Do not create a fork PR as a fallback. If you accidentally create one, close it immediately and replace it with a same-repository draft PR. +## Branch Rules + +Agents must never bypass branch rules, even when their credentials have admin +rights: do not merge with `gh pr merge --admin`, and do not use GitHub's option +to bypass rules. If a required check or review blocks a merge, report it to a +human instead. + +## Stacked PRs and CI + +In a stack of pull requests, only the lowest unmerged one runs the full PR +workflow (`.github/workflows/pr.yml`) and the Modal image smoke. The ones above +it run only the `CI complete` job, which fails and says so. A stack therefore +uses one set of runners instead of one per layer. + +- **`CI complete` is the required check.** It needs every other job in + `pr.yml`. On a pull request that isn't stacked, or is the lowest unmerged + one, it passes only when every job passed. Skipped jobs would otherwise + count as passing, so a stack can only merge bottom-up, and nothing merges + untested. +- **When the pull request below merges,** GitHub rebases the next one onto + the trunk, and that push starts its full run. If no run starts, push again + (`gh stack rebase`, then `gh stack push`), or close and reopen the pull + request. Re-running the old workflow doesn't help: it reuses the old event, + which still describes the pull request as higher in the stack. +- **When you add a job to `pr.yml`,** add it to `CI complete`'s `needs`. If + the job has no `needs` of its own, also give it the same stack condition as + the other jobs. + `projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py` + checks both. + ## PR Title Do not add `[codex]`, `[claude]`, `[copilot]`, or other agent labels to PR diff --git a/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py b/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py new file mode 100644 index 000000000..b1f45dfd2 --- /dev/null +++ b/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py @@ -0,0 +1,167 @@ +"""Tests for the stacked-PR CI gate: pr.yml's shape and check_ci_complete.sh. + +Only the lowest unmerged pull request in a stack runs the full PR workflow. +The "CI complete" job is the required check: it needs every other job, fails on +the pull requests above the bottom of a stack, and otherwise passes only when +every job passed. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path + +import yaml + +REPO = Path(__file__).resolve().parents[3] +PR_WORKFLOW = REPO / ".github" / "workflows" / "pr.yml" +GATE_SCRIPT = ".github/scripts/check_ci_complete.sh" +GATE_JOB = "ci-complete" +GATE_CONDITION = "${{ always() }}" +FULL_CI_CONDITION = ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }}" +) + + +def gate_violations(workflow: dict) -> list[str]: + """Return every way a job in ``workflow`` could escape the CI-complete gate.""" + jobs = workflow.get("jobs") or {} + gate = jobs.get(GATE_JOB) + if gate is None: + return [f"the workflow has no {GATE_JOB} job"] + + violations = [] + if gate.get("if") != GATE_CONDITION: + violations.append(f"{GATE_JOB} must have if: {GATE_CONDITION}") + + gated_jobs = gate.get("needs") or [] + if isinstance(gated_jobs, str): + gated_jobs = [gated_jobs] + for job_id in sorted(set(jobs) - {GATE_JOB} - set(gated_jobs)): + violations.append(f"{GATE_JOB} does not need {job_id}") + + for job_id, job in jobs.items(): + if job_id == GATE_JOB: + continue + condition = job.get("if") + if job.get("needs"): + # A job that needs a skipped job is skipped too, unless its own + # condition overrides that. + if condition is not None and condition != FULL_CI_CONDITION: + violations.append(f"{job_id} has its own condition: {condition}") + elif condition != FULL_CI_CONDITION: + violations.append(f"{job_id} must have if: {FULL_CI_CONDITION}") + return violations + + +def test_pr_workflow_is_gated() -> None: + workflow = yaml.safe_load(PR_WORKFLOW.read_text()) + + assert gate_violations(workflow) == [] + + +def test_gate_check_rejects_an_ungated_job() -> None: + workflow = { + "jobs": { + GATE_JOB: {"if": GATE_CONDITION, "needs": ["lint"]}, + "lint": {"if": FULL_CI_CONDITION}, + "test": {}, + } + } + + assert gate_violations(workflow) == [ + f"{GATE_JOB} does not need test", + f"test must have if: {FULL_CI_CONDITION}", + ] + + +def _run_gate( + job_results: dict[str, str], + *, + pull_request_base_ref: str = "main", + stack_base_ref: str = "", +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["bash", GATE_SCRIPT], + cwd=REPO, + env={ + "HOME": os.environ.get("HOME", ""), + "PATH": os.environ["PATH"], + "JOB_RESULTS": json.dumps( + { + job_id: {"result": result, "outputs": {}} + for job_id, result in job_results.items() + } + ), + "PULL_REQUEST_BASE_REF": pull_request_base_ref, + "STACK_BASE_REF": stack_base_ref, + }, + text=True, + capture_output=True, + check=False, + ) + + +def test_gate_passes_an_unstacked_pull_request_when_every_job_passed() -> None: + result = _run_gate({"lint": "success", "test": "success"}) + + assert result.returncode == 0, result.stdout + assert result.stdout == "Every job passed.\n" + + +def test_gate_passes_the_lowest_unmerged_pull_request_in_a_stack() -> None: + result = _run_gate( + {"lint": "success", "test": "success"}, + pull_request_base_ref="main", + stack_base_ref="main", + ) + + assert result.returncode == 0, result.stdout + + +def test_gate_fails_a_pull_request_higher_in_a_stack() -> None: + result = _run_gate( + {"lint": "skipped", "test": "skipped"}, + pull_request_base_ref="ruff-annotations", + stack_base_ref="main", + ) + + assert result.returncode == 1 + assert "lowest unmerged pull request in a stack" in result.stdout + assert "targets ruff-annotations" in result.stdout + + +def test_gate_fails_and_names_every_job_that_did_not_pass() -> None: + result = _run_gate( + {"lint": "success", "test": "failure", "docker-build": "cancelled"}, + ) + + assert result.returncode == 1 + assert result.stdout.splitlines() == [ + "::error::Job did not pass: test: failure", + "::error::Job did not pass: docker-build: cancelled", + ] + + +def test_gate_fails_when_a_job_was_skipped_on_full_ci() -> None: + result = _run_gate({"lint": "success", "test": "skipped"}, stack_base_ref="main") + + assert result.returncode == 1 + assert "::error::Job did not pass: test: skipped" in result.stdout + + +def test_gate_requires_the_job_results() -> None: + result = subprocess.run( + ["bash", GATE_SCRIPT], + cwd=REPO, + env={"PATH": os.environ["PATH"], "PULL_REQUEST_BASE_REF": "main"}, + text=True, + capture_output=True, + check=False, + ) + + assert result.returncode != 0 + assert "JOB_RESULTS is required" in result.stderr diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 000000000..21a4f7848 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,4 @@ +# Ruff's defaults as of 0.15, made explicit so a ruff upgrade cannot change +# which rules apply. Widen this deliberately, one rule family per PR. +[lint] +select = ["E4", "E7", "E9", "F"]