diff --git a/.github/scripts/build-simulation-api-client.sh b/.github/scripts/build-simulation-api-client.sh new file mode 100755 index 000000000..e382d8c2e --- /dev/null +++ b/.github/scripts/build-simulation-api-client.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash +# Version and build the generated simulation API Python client. +# Usage: build-simulation-api-client.sh +# Required env vars: RUN_NUMBER (the workflow run number). +# The client is versioned 0.. and built into +# /dist/. + +set -euo pipefail + +CLIENT_DIR="${1:?Client directory required}" +: "${RUN_NUMBER:?RUN_NUMBER is required}" + +cd "$CLIENT_DIR" +# Update version +DATE=$(date +%Y%m%d) +NEW_VERSION="0.${DATE}.${RUN_NUMBER}" +sed -i.bak "s/^version = .*/version = \"${NEW_VERSION}\"/" pyproject.toml +rm pyproject.toml.bak +uv build diff --git a/.github/scripts/modal-precompute.sh b/.github/scripts/modal-precompute.sh index eb7b884ba..9d8dd4e79 100755 --- a/.github/scripts/modal-precompute.sh +++ b/.github/scripts/modal-precompute.sh @@ -22,7 +22,7 @@ fi # waste the whole run. if [ -z "${GITHUB_OUTPUT:-}" ]; then echo "GITHUB_OUTPUT is required (the manifest digest is exported there)." >&2 - echo 'For local runs: export GITHUB_OUTPUT="$(mktemp)".' >&2 + echo "For local runs: export GITHUB_OUTPUT=\"\$(mktemp)\"." >&2 exit 1 fi diff --git a/.github/scripts/start-local-services.sh b/.github/scripts/start-local-services.sh new file mode 100755 index 000000000..5a712cc41 --- /dev/null +++ b/.github/scripts/start-local-services.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# Start the local Docker Compose services and wait for them to answer. +# Usage: start-local-services.sh +# +# Waits 10 seconds, then polls the local gateway's liveness endpoint up to 30 +# times, 2 seconds apart. It does not fail when the services never answer: the +# integration tests that follow report that. + +set -euo pipefail + +COMPOSE_FILE="${1:?Docker Compose file required}" + +docker compose -f "$COMPOSE_FILE" up -d +# Wait for services to be ready +sleep 10 +# Check services are responding +for i in {1..30}; do + if curl -f http://localhost:8082/ping/alive; then + echo "Services are ready!" + break + fi + echo "Waiting for services... (attempt $i/30)" + sleep 2 +done diff --git a/.github/workflows/pr-image-smoke.yml b/.github/workflows/pr-image-smoke.yml index d1f91e667..b68114579 100644 --- a/.github/workflows/pr-image-smoke.yml +++ b/.github/workflows/pr-image-smoke.yml @@ -65,6 +65,4 @@ jobs: MODAL_TOKEN_ID: ${{ secrets.MODAL_TOKEN_ID }} MODAL_TOKEN_SECRET: ${{ secrets.MODAL_TOKEN_SECRET }} PE_UK_PRIVATE_HF_READ_TOKEN: ${{ secrets.PE_UK_PRIVATE_HF_READ_TOKEN }} - run: | - chmod +x .github/scripts/modal-image-smoke.sh - .github/scripts/modal-image-smoke.sh staging + run: .github/scripts/modal-image-smoke.sh staging diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 96cc7e737..d0d75e5e2 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -68,14 +68,12 @@ jobs: enable-cache: true - name: Install dependencies - run: | - cd ${{ matrix.project }} - uv sync --extra test + working-directory: ${{ matrix.project }} + run: uv sync --extra test - name: Run tests - run: | - cd ${{ matrix.project }} - uv run pytest tests/ -v + working-directory: ${{ matrix.project }} + run: uv run pytest tests/ -v lint: name: Lint @@ -99,13 +97,7 @@ jobs: run: uv tool install ruff==0.15.22 - name: Run ruff format check - run: | - for dir in projects/*/src libs/*/src; do - if [ -d "$dir" ]; then - echo "Checking format in $dir..." - uv run ruff format --check $dir - fi - done + run: uv run ruff format --check projects/*/src libs/*/src quality-guards: name: Quality guards @@ -184,26 +176,16 @@ jobs: ./scripts/generate-clients.sh - name: Start services - run: | - docker compose -f deployment/docker-compose.yml up -d - # Wait for services to be ready - sleep 10 - # Check services are responding - for i in {1..30}; do - if curl -f http://localhost:8082/ping/alive; then - echo "Services are ready!" - break - fi - echo "Waiting for services... (attempt $i/30)" - sleep 2 - done + run: .github/scripts/start-local-services.sh deployment/docker-compose.yml + + - name: Install integration test dependencies + working-directory: projects/policyengine-apis-integ + run: uv sync --extra test - name: Run integration tests (local services only) - run: | - cd projects/policyengine-apis-integ - uv sync --extra test - # Run tests against local Docker services only (not Modal staging) - uv run pytest tests/ -v -m "not requires_gcp and not beta_only" + working-directory: projects/policyengine-apis-integ + # Run tests against local Docker services only (not Modal staging) + run: uv run pytest tests/ -v -m "not requires_gcp and not beta_only" - name: Show service logs on failure if: failure() diff --git a/.github/workflows/publish-clients.yml b/.github/workflows/publish-clients.yml index e77fc7d3c..d2565c573 100644 --- a/.github/workflows/publish-clients.yml +++ b/.github/workflows/publish-clients.yml @@ -38,14 +38,9 @@ jobs: ./scripts/generate-clients.sh - name: Build simulation API client from the entrypoint contract - run: | - cd projects/policyengine-simulation-entry/artifacts/clients/python - # Update version - DATE=$(date +%Y%m%d) - RUN_NUMBER="${{ github.run_number }}" - NEW_VERSION="0.${DATE}.${RUN_NUMBER}" - sed -i "s/^version = .*/version = \"${NEW_VERSION}\"/" pyproject.toml - uv build + env: + RUN_NUMBER: ${{ github.run_number }} + run: .github/scripts/build-simulation-api-client.sh projects/policyengine-simulation-entry/artifacts/clients/python # PyPI trusted publishing: uploads accepted only from this workflow in the # pypi environment (limited to main). No stored token. diff --git a/changelog_entry.yaml b/changelog_entry.yaml index 979e3506e..c3fa6bd88 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -4,6 +4,7 @@ - Made CLAUDE.md and the Copilot instructions thin pointers to AGENTS.md, required workflow logic longer than one line to live in tested and shellchecked scripts, and registered the tracking-issue rule - Added a rules registry (rules/registry.toml) with a standard-library tool that validates it, checks every rule is enforced where it claims, and renders docs/generated/rules.md and the Rules block in AGENTS.md changed: + - Moved multi-line `run` blocks in the PR, image smoke and client publishing workflows into single-line steps and tested scripts, tested the dependency-update scripts, fixed the shellcheck finding in modal-precompute.sh, and enforced the CI-script testing and shellcheck rules with empty baselines - 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 diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 94ce0fd09..d6913f2df 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -1,6 +1,6 @@ # Rules -Generated by `python -m scripts.rules render` (scripts/rules v0.1.0, tool sha256 `afc55ba219f1`) from `rules/registry.toml`. Do not edit by hand. +Generated by `python -m scripts.rules render` (scripts/rules v0.1.0, tool sha256 `86b191f2ebb3`) from `rules/registry.toml`. Do not edit by hand. Every rule that governs `policyengine-sim-api` is listed here with how it is enforced. Blocking rules fail CI. The Gaps section lists what is written down but not yet enforced or proven. To add or change a rule, see `docs/engineering/skills/rules-registry.md`. @@ -8,7 +8,7 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 26 | 3 | 1 | 14 | 44 | +| mechanical | 28 | 1 | 1 | 14 | 44 | | metric | 0 | 0 | 0 | 0 | 0 | | judgment | 0 | 0 | 0 | 0 | 0 | @@ -60,9 +60,7 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf ### Ratcheted (existing violations still tolerated) -- `sim.ci.multiline-run-is-script`: 20 remaining in `rules/baselines/ci-multiline-run.txt` -- `sim.ci.shell-scripts-pass-shellcheck`: 1 remaining in `rules/baselines/ci-shellcheck.txt` -- `sim.ci.workflow-scripts-tested`: 4 remaining in `rules/baselines/ci-untested-scripts.txt` +- `sim.ci.multiline-run-is-script`: 13 remaining in `rules/baselines/ci-multiline-run.txt` ### Exceptions and review dates @@ -267,16 +265,16 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `sim.ci.multiline-run-is-script` | A GitHub Actions run: block holds one line of code and no chained commands (&&, \|\|, ;); anything more lives in a tested script under .github/scripts/, called from a single-line step. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | -| `sim.ci.shell-scripts-pass-shellcheck` | Every shell script under .github/scripts/ passes shellcheck. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | -| `sim.ci.workflow-scripts-tested` | Every shell script a workflow calls is run by at least one test. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | +| `sim.ci.shell-scripts-pass-shellcheck` | Every shell script under .github/scripts/ passes shellcheck. | mechanical | enforced | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | +| `sim.ci.workflow-scripts-tested` | Every shell script a workflow calls is run by at least one test. | mechanical | enforced | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | ```mermaid flowchart LR r_sim_ci_multiline_run_is_script("sim.ci.multiline-run-is-script (ratcheted)") e_quality_guard_ci_scripts["quality guard ci-scripts"] j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] - r_sim_ci_shell_scripts_pass_shellcheck("sim.ci.shell-scripts-pass-shellcheck (ratcheted)") - r_sim_ci_workflow_scripts_tested("sim.ci.workflow-scripts-tested (ratcheted)") + r_sim_ci_shell_scripts_pass_shellcheck("sim.ci.shell-scripts-pass-shellcheck (enforced)") + r_sim_ci_workflow_scripts_tested("sim.ci.workflow-scripts-tested (enforced)") r_sim_ci_multiline_run_is_script --> e_quality_guard_ci_scripts e_quality_guard_ci_scripts --> j_CI_pr_yml_quality_guards r_sim_ci_shell_scripts_pass_shellcheck --> e_quality_guard_ci_scripts diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py new file mode 100644 index 000000000..4a1544da6 --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -0,0 +1,459 @@ +"""Tests for the workflow scripts under .github/scripts/ that no deployment +test covers: each runs the script with fake commands first on PATH.""" + +from __future__ import annotations + +import os +import re +from pathlib import Path + +import pytest + +from workflow_script_support import ( + REPOSITORY_ROOT, + WORKFLOWS_DIR, + FakeCommands, + github_output, + run_script, +) + + +@pytest.fixture +def fakes(tmp_path: Path) -> FakeCommands: + return FakeCommands.create(tmp_path) + + +def test_scripts_that_workflows_run_directly_are_executable(): + """A step that runs a script without ``bash`` needs its executable bit.""" + direct = re.compile(r"^\s*run: (\.github/scripts/[\w.-]+\.sh)\b", re.MULTILINE) + scripts = { + script + for workflow in WORKFLOWS_DIR.glob("*.yml") + for script in direct.findall(workflow.read_text(encoding="utf-8")) + } + + assert ".github/scripts/modal-image-smoke.sh" in scripts + for script in sorted(scripts): + assert os.access(REPOSITORY_ROOT / script, os.X_OK), script + + +# --- update-dependencies.yml ---------------------------------------------- + +PREPARE_BRANCH = "dependency-update-prepare-branch.sh" +COMMIT_AND_PUSH = "dependency-update-commit-and-push.sh" +CREATE_PR = "dependency-update-create-pr.sh" + +FAKE_GH_PR_LIST = """\ +if [[ "$1 $2" == "pr list" ]]; then + printf '%s' "${FAKE_OPEN_PR:-}" + exit "${FAKE_GH_EXIT:-0}" +fi +exit "${FAKE_GH_EXIT:-0}" +""" + +FAKE_GIT = """\ +case "$1" in + status) printf '%s' "${FAKE_GIT_STATUS:-}" ;; + rev-list) printf '%s\\n' "${FAKE_COMMITS_TO_PUSH:-0}" ;; + push) exit "${FAKE_GIT_PUSH_EXIT:-0}" ;; +esac +""" + + +def test_update_dependencies_workflow_runs_the_tested_scripts(): + workflow = (WORKFLOWS_DIR / "update-dependencies.yml").read_text(encoding="utf-8") + + for script in (PREPARE_BRANCH, COMMIT_AND_PUSH, CREATE_PR): + assert f"run: .github/scripts/{script}" in workflow + + +def _prepare_branch(fakes: FakeCommands, tmp_path: Path, **env: str): + output = tmp_path / "github-output" + fakes.add("gh", FAKE_GH_PR_LIST) + fakes.add("git") + result = run_script( + PREPARE_BRANCH, + env=fakes.env( + GITHUB_REPOSITORY="PolicyEngine/policyengine-sim-api", + GITHUB_OUTPUT=str(output), + **env, + ), + ) + return result, output + + +def test_prepare_branch_reuses_the_branch_of_an_open_update_pr(fakes, tmp_path): + result, output = _prepare_branch(fakes, tmp_path, FAKE_OPEN_PR="42") + + assert result.returncode == 0, result.stderr + assert github_output(output) == {"has_open_pr": "true"} + assert fakes.calls("gh") == [ + [ + "gh", + "pr", + "list", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--base", + "main", + "--head", + "update-dependencies", + "--state", + "open", + "--json", + "number", + "--jq", + ".[0].number // empty", + ] + ] + assert fakes.calls("git") == [ + ["git", "config", "user.name", "policyengine-auto"], + [ + "git", + "config", + "user.email", + "policyengine-auto@users.noreply.github.com", + ], + ["git", "fetch", "origin", "update-dependencies"], + [ + "git", + "checkout", + "-B", + "update-dependencies", + "origin/update-dependencies", + ], + ["git", "merge", "--no-edit", "-X", "theirs", "origin/main"], + ] + + +def test_prepare_branch_starts_from_main_without_an_open_update_pr(fakes, tmp_path): + result, output = _prepare_branch(fakes, tmp_path) + + assert result.returncode == 0, result.stderr + assert github_output(output) == {"has_open_pr": "false"} + assert fakes.calls("git")[2:] == [ + ["git", "checkout", "-B", "update-dependencies", "origin/main"] + ] + + +def test_prepare_branch_fails_when_the_pr_lookup_fails(fakes, tmp_path): + result, output = _prepare_branch(fakes, tmp_path, FAKE_GH_EXIT="1") + + assert result.returncode != 0 + assert github_output(output) == {} + assert not any(call[1] == "checkout" for call in fakes.calls("git")) + + +def test_prepare_branch_requires_the_repository(fakes, tmp_path): + fakes.add("gh", FAKE_GH_PR_LIST) + fakes.add("git") + env = fakes.env(GITHUB_OUTPUT=str(tmp_path / "github-output")) + env.pop("GITHUB_REPOSITORY", None) + + result = run_script(PREPARE_BRANCH, env=env) + + assert result.returncode != 0 + assert "GITHUB_REPOSITORY is required" in result.stderr + assert fakes.calls() == [] + + +def _commit_and_push(fakes: FakeCommands, tmp_path: Path, has_open_pr: str, **env): + output = tmp_path / "github-output" + fakes.add("git", FAKE_GIT) + fakes.add("date", "echo 2026-10-13\n") + result = run_script( + COMMIT_AND_PUSH, + env=fakes.env(GITHUB_OUTPUT=str(output), HAS_OPEN_PR=has_open_pr, **env), + ) + return result, output + + +def test_commit_and_push_replaces_the_branch_when_no_pr_owns_it(fakes, tmp_path): + result, output = _commit_and_push( + fakes, tmp_path, "false", FAKE_GIT_STATUS=" M uv.lock\n" + ) + + assert result.returncode == 0, result.stderr + assert "Changes detected" in result.stdout + assert github_output(output) == {"branch_pushed": "true"} + assert fakes.calls("git") == [ + ["git", "status", "--porcelain"], + ["git", "add", "--all"], + ["git", "commit", "-m", "Update dependencies (2026-10-13)"], + [ + "git", + "push", + "--force-with-lease", + "origin", + "HEAD:update-dependencies", + ], + ] + assert fakes.calls("date") == [["date", "-u", "+%Y-%m-%d"]] + + +def test_commit_and_push_appends_to_the_branch_of_an_open_pr(fakes, tmp_path): + result, output = _commit_and_push( + fakes, + tmp_path, + "true", + FAKE_GIT_STATUS=" M uv.lock\n", + FAKE_COMMITS_TO_PUSH="1", + ) + + assert result.returncode == 0, result.stderr + assert github_output(output) == {"branch_pushed": "true"} + assert fakes.calls("git")[-2:] == [ + ["git", "rev-list", "--count", "origin/update-dependencies..HEAD"], + ["git", "push", "origin", "HEAD:update-dependencies"], + ] + + +def test_commit_and_push_pushes_earlier_commits_to_an_open_pr(fakes, tmp_path): + """A merge from main can leave commits to push even without new changes.""" + result, output = _commit_and_push(fakes, tmp_path, "true", FAKE_COMMITS_TO_PUSH="2") + + assert result.returncode == 0, result.stderr + assert "No changes detected" in result.stdout + assert github_output(output) == {"branch_pushed": "true"} + assert ["git", "push", "origin", "HEAD:update-dependencies"] in fakes.calls("git") + assert not any(call[1] == "commit" for call in fakes.calls("git")) + + +@pytest.mark.parametrize("has_open_pr", ["true", "false"]) +def test_commit_and_push_skips_the_push_when_nothing_is_new( + fakes, tmp_path, has_open_pr +): + result, output = _commit_and_push(fakes, tmp_path, has_open_pr) + + assert result.returncode == 0, result.stderr + assert github_output(output) == {"branch_pushed": "false"} + assert not any(call[1] in ("commit", "push") for call in fakes.calls("git")) + + +def test_commit_and_push_rejects_an_invalid_open_pr_flag(fakes, tmp_path): + result, output = _commit_and_push(fakes, tmp_path, "yes") + + assert result.returncode == 1 + assert "HAS_OPEN_PR must be either true or false" in result.stderr + assert github_output(output) == {} + assert fakes.calls() == [] + + +def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): + result, output = _commit_and_push( + fakes, + tmp_path, + "false", + FAKE_GIT_STATUS=" M uv.lock\n", + FAKE_GIT_PUSH_EXIT="1", + ) + + assert result.returncode != 0 + assert github_output(output) == {} + + +def test_create_pr_opens_the_update_pr(fakes, tmp_path): + fakes.add("gh") + + result = run_script( + CREATE_PR, env=fakes.env(GITHUB_REPOSITORY="PolicyEngine/policyengine-sim-api") + ) + + assert result.returncode == 0, result.stderr + assert fakes.calls() == [ + [ + "gh", + "pr", + "create", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--title", + "Update dependencies", + "--body", + "Automated daily dependency updates. Subsequent runs append commits " + "to this branch while the pull request remains open.", + "--base", + "main", + "--head", + "update-dependencies", + ] + ] + + +def test_create_pr_fails_when_gh_fails(fakes, tmp_path): + fakes.add("gh", "exit 1\n") + + result = run_script( + CREATE_PR, env=fakes.env(GITHUB_REPOSITORY="PolicyEngine/policyengine-sim-api") + ) + + assert result.returncode == 1 + + +def test_create_pr_requires_the_repository(fakes, tmp_path): + fakes.add("gh") + env = fakes.env() + env.pop("GITHUB_REPOSITORY", None) + + result = run_script(CREATE_PR, env=env) + + assert result.returncode != 0 + assert "GITHUB_REPOSITORY is required" in result.stderr + assert fakes.calls() == [] + + +# --- pr.yml --------------------------------------------------------------- + +START_LOCAL_SERVICES = "start-local-services.sh" + +FAKE_CURL_READY_AFTER = """\ +count=$(($(cat "$FAKE_CURL_COUNT" 2>/dev/null || echo 0) + 1)) +echo "$count" > "$FAKE_CURL_COUNT" +[[ "$count" -ge "${FAKE_CURL_READY_AT:-999}" ]] +""" + + +def test_pr_workflow_starts_local_services_with_the_tested_script(): + workflow = (WORKFLOWS_DIR / "pr.yml").read_text(encoding="utf-8") + + assert ( + f"run: .github/scripts/{START_LOCAL_SERVICES} deployment/docker-compose.yml" + in workflow + ) + + +def _start_local_services(fakes: FakeCommands, tmp_path: Path, *args: str, **env): + fakes.add("docker", 'exit "${FAKE_DOCKER_EXIT:-0}"\n') + fakes.add("curl", FAKE_CURL_READY_AFTER) + fakes.add("sleep") + return run_script( + START_LOCAL_SERVICES, + *args, + env=fakes.env(FAKE_CURL_COUNT=str(tmp_path / "curl-count"), **env), + ) + + +def test_start_local_services_waits_until_the_gateway_answers(fakes, tmp_path): + result = _start_local_services( + fakes, tmp_path, "deployment/docker-compose.yml", FAKE_CURL_READY_AT="3" + ) + + assert result.returncode == 0, result.stderr + assert "Waiting for services... (attempt 2/30)" in result.stdout + assert "Services are ready!" in result.stdout + alive = ["curl", "-f", "http://localhost:8082/ping/alive"] + assert fakes.calls() == [ + ["docker", "compose", "-f", "deployment/docker-compose.yml", "up", "-d"], + ["sleep", "10"], + alive, + ["sleep", "2"], + alive, + ["sleep", "2"], + alive, + ] + + +def test_start_local_services_gives_up_quietly_after_thirty_attempts(fakes, tmp_path): + """Unchanged from the inline step: the integration tests report the outage.""" + result = _start_local_services(fakes, tmp_path, "deployment/docker-compose.yml") + + assert result.returncode == 0, result.stderr + assert "Waiting for services... (attempt 30/30)" in result.stdout + assert "Services are ready!" not in result.stdout + assert len(fakes.calls("curl")) == 30 + assert len(fakes.calls("sleep")) == 31 + + +def test_start_local_services_fails_when_compose_fails(fakes, tmp_path): + result = _start_local_services( + fakes, tmp_path, "deployment/docker-compose.yml", FAKE_DOCKER_EXIT="1" + ) + + assert result.returncode == 1 + assert fakes.calls("curl") == [] + + +def test_start_local_services_requires_a_compose_file(fakes, tmp_path): + result = _start_local_services(fakes, tmp_path) + + assert result.returncode != 0 + assert "Docker Compose file required" in result.stderr + assert fakes.calls() == [] + + +# --- publish-clients.yml -------------------------------------------------- + +BUILD_CLIENT = "build-simulation-api-client.sh" +CLIENT_DIR = "projects/policyengine-simulation-entry/artifacts/clients/python" +CLIENT_PYPROJECT = """\ +[project] +name = "policyengine_api_simulation_client" +version = "0.1.0" +description = "A client library for accessing PolicyEngine simulation API" +""" + + +def test_publish_workflow_builds_the_client_with_the_tested_script(): + workflow = (WORKFLOWS_DIR / "publish-clients.yml").read_text(encoding="utf-8") + + assert f"run: .github/scripts/{BUILD_CLIENT} {CLIENT_DIR}" in workflow + assert "RUN_NUMBER: ${{ github.run_number }}" in workflow + assert f"packages-dir: {CLIENT_DIR}/dist/" in workflow + + +def _build_client(fakes: FakeCommands, tmp_path: Path, *args: str, **env): + client = tmp_path / "client" + client.mkdir() + (client / "pyproject.toml").write_text(CLIENT_PYPROJECT, encoding="utf-8") + fakes.add("date", "echo 20261013\n") + fakes.add("uv", 'pwd > "$FAKE_UV_CWD"\nexit "${FAKE_UV_EXIT:-0}"\n') + result = run_script( + BUILD_CLIENT, + *(args or (str(client),)), + env=fakes.env(FAKE_UV_CWD=str(tmp_path / "uv-cwd"), **env), + ) + return result, client + + +def test_build_client_versions_the_client_by_date_and_run_number(fakes, tmp_path): + result, client = _build_client(fakes, tmp_path, RUN_NUMBER="57") + + assert result.returncode == 0, result.stderr + assert (client / "pyproject.toml").read_text( + encoding="utf-8" + ) == CLIENT_PYPROJECT.replace('version = "0.1.0"', 'version = "0.20261013.57"') + assert sorted(path.name for path in client.iterdir()) == ["pyproject.toml"] + assert fakes.calls() == [["date", "+%Y%m%d"], ["uv", "build"]] + uv_cwd = (tmp_path / "uv-cwd").read_text(encoding="utf-8").strip() + assert Path(uv_cwd).resolve() == client.resolve() + + +def test_build_client_fails_when_the_build_fails(fakes, tmp_path): + result, _ = _build_client(fakes, tmp_path, RUN_NUMBER="57", FAKE_UV_EXIT="1") + + assert result.returncode == 1 + + +def test_build_client_requires_the_run_number(fakes, tmp_path): + env = fakes.env() + env.pop("RUN_NUMBER", None) + client = tmp_path / "client" + client.mkdir() + (client / "pyproject.toml").write_text(CLIENT_PYPROJECT, encoding="utf-8") + fakes.add("uv") + + result = run_script(BUILD_CLIENT, str(client), env=env) + + assert result.returncode != 0 + assert "RUN_NUMBER is required" in result.stderr + assert (client / "pyproject.toml").read_text(encoding="utf-8") == CLIENT_PYPROJECT + assert fakes.calls() == [] + + +def test_build_client_fails_without_the_generated_client(fakes, tmp_path): + result, _ = _build_client( + fakes, tmp_path, str(tmp_path / "missing"), RUN_NUMBER="57" + ) + + assert result.returncode != 0 + assert fakes.calls("uv") == [] diff --git a/projects/policyengine-simulation-entry/tests/workflow_script_support.py b/projects/policyengine-simulation-entry/tests/workflow_script_support.py new file mode 100644 index 000000000..8873cfe2a --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/workflow_script_support.py @@ -0,0 +1,94 @@ +"""Run a .github/scripts/ shell script with fake commands first on PATH. + +Each fake command records its arguments, then runs an optional bash body, so a +test can control what the command prints and returns and assert on every call +the script made, in order. +""" + +from __future__ import annotations + +import os +import subprocess +import textwrap +from dataclasses import dataclass +from pathlib import Path + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +SCRIPTS_DIR = REPOSITORY_ROOT / ".github" / "scripts" +WORKFLOWS_DIR = REPOSITORY_ROOT / ".github" / "workflows" + +# Separates the recorded arguments of one call; arguments may contain spaces. +_FIELD = "\x1f" + + +@dataclass +class FakeCommands: + """A directory of fake commands that record their calls to one log.""" + + directory: Path + log: Path + + @classmethod + def create(cls, tmp_path: Path) -> FakeCommands: + directory = tmp_path / "fake-bin" + directory.mkdir() + return cls(directory=directory, log=tmp_path / "fake-calls.log") + + def add(self, name: str, body: str = "") -> None: + """Install ``name``: record ``name`` and its arguments, then run ``body``.""" + path = self.directory / name + path.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + f'printf \'%s{_FIELD}\' {name} "$@" >> "$FAKE_CALLS_LOG"\n' + "printf '\\n' >> \"$FAKE_CALLS_LOG\"\n" + textwrap.dedent(body), + encoding="utf-8", + ) + path.chmod(0o755) + + def calls(self, name: str | None = None) -> list[list[str]]: + """Every recorded call (``[command, *arguments]``), optionally of one command.""" + if not self.log.exists(): + return [] + recorded = [ + line.split(_FIELD)[:-1] + for line in self.log.read_text(encoding="utf-8").splitlines() + ] + return [call for call in recorded if name is None or call[0] == name] + + def env(self, **overrides: str) -> dict[str, str]: + """The environment to run a script in, with the fakes first on PATH.""" + return { + **os.environ, + "PATH": f"{self.directory}{os.pathsep}{os.environ['PATH']}", + "FAKE_CALLS_LOG": str(self.log), + **overrides, + } + + +def run_script( + name: str, + *args: str, + env: dict[str, str], + cwd: Path | None = None, +) -> subprocess.CompletedProcess[str]: + """Run ``.github/scripts/`` directly, as a workflow step does.""" + return subprocess.run( + [str(SCRIPTS_DIR / name), *args], + capture_output=True, + text=True, + check=False, + env=env, + cwd=cwd, + ) + + +def github_output(path: Path) -> dict[str, str]: + """The ``name=value`` pairs a script appended to ``$GITHUB_OUTPUT``.""" + if not path.exists(): + return {} + pairs = {} + for line in path.read_text(encoding="utf-8").splitlines(): + key, _, value = line.partition("=") + pairs[key] = value + return pairs diff --git a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py index 7a2dbffca..d00fbb7cc 100644 --- a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py @@ -610,6 +610,8 @@ def test_requires_github_output_before_running(self, tmp_path): assert result.returncode != 0 assert "GITHUB_OUTPUT is required" in result.stderr + # The local-run hint is printed literally, not expanded. + assert 'export GITHUB_OUTPUT="$(mktemp)".' in result.stderr assert calls == [], "Should not invoke modal without GITHUB_OUTPUT" def test_runs_precompute_app_and_exports_digest(self, tmp_path): diff --git a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py index 449d91545..011834260 100644 --- a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py @@ -46,6 +46,15 @@ def test_update_policyengine_package_uses_a_separate_python_helper() -> None: assert result.returncode == 0, result.stderr +def test_update_policyengine_workflow_runs_the_tested_script() -> None: + workflow = ( + SCRIPT.parent.parent / "workflows" / "check-policyengine-updates.yml" + ).read_text(encoding="utf-8") + + assert SCRIPT.name == "update-policyengine-package.sh" + assert "run: bash .github/scripts/update-policyengine-package.sh" in workflow + + def test_update_policyengine_workflow_requests_issue_and_pr_permissions() -> None: workflow = ( SCRIPT.parent.parent / "workflows" / "check-policyengine-updates.yml" diff --git a/rules/baselines/ci-multiline-run.txt b/rules/baselines/ci-multiline-run.txt index bb27165c7..0b690cf88 100644 --- a/rules/baselines/ci-multiline-run.txt +++ b/rules/baselines/ci-multiline-run.txt @@ -1,11 +1,4 @@ # Tolerated existing violations. This list may only shrink. -.github/workflows/pr-image-smoke.yml :: image-smoke :: Run image smokes -.github/workflows/pr.yml :: lint :: Run ruff format check -.github/workflows/pr.yml :: test :: Install dependencies -.github/workflows/pr.yml :: test :: Run tests -.github/workflows/pr.yml :: test-integration :: Run integration tests (local services only) -.github/workflows/pr.yml :: test-integration :: Start services -.github/workflows/publish-clients.yml :: publish :: Build simulation API client from the entrypoint contract .github/workflows/simulation-deploy.reusable.yml :: deploy_entrypoint :: Build and push entrypoint image .github/workflows/simulation-deploy.reusable.yml :: deploy_entrypoint :: Capture stable service state before deployment .github/workflows/simulation-deploy.reusable.yml :: deploy_entrypoint :: Resolve exact candidate metadata diff --git a/rules/baselines/ci-shellcheck.txt b/rules/baselines/ci-shellcheck.txt index 9bc2c636a..924c0983a 100644 --- a/rules/baselines/ci-shellcheck.txt +++ b/rules/baselines/ci-shellcheck.txt @@ -1,2 +1 @@ # Tolerated existing violations. This list may only shrink. -.github/scripts/modal-precompute.sh diff --git a/rules/baselines/ci-untested-scripts.txt b/rules/baselines/ci-untested-scripts.txt index bcc106dcb..924c0983a 100644 --- a/rules/baselines/ci-untested-scripts.txt +++ b/rules/baselines/ci-untested-scripts.txt @@ -1,5 +1 @@ # Tolerated existing violations. This list may only shrink. -.github/scripts/dependency-update-commit-and-push.sh -.github/scripts/dependency-update-create-pr.sh -.github/scripts/dependency-update-prepare-branch.sh -.github/scripts/update-policyengine-package.sh diff --git a/rules/registry.toml b/rules/registry.toml index 603b88d34..733f164ef 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -670,7 +670,7 @@ job = "quality-guards" invocation = "run_quality_guards.py" [rule.baseline] file = "rules/baselines/ci-multiline-run.txt" -count = 20 +count = 13 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/multiline_bad" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/multiline_good" @@ -686,7 +686,7 @@ statement = "Every shell script a workflow calls is run by at least one test." rationale = "docs/engineering/skills/ci-workflows.md#testing-a-workflow-script" scope = [".github/scripts/*.sh"] tier = "mechanical" -status = "ratcheted" +status = "enforced" autofix = false [rule.enforcer] kind = "quality-guard" @@ -695,9 +695,6 @@ name = "ci-scripts" workflow = ".github/workflows/pr.yml" job = "quality-guards" invocation = "run_quality_guards.py" -[rule.baseline] -file = "rules/baselines/ci-untested-scripts.txt" -count = 4 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/untested_bad" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/untested_good" @@ -713,7 +710,7 @@ statement = "Every shell script under .github/scripts/ passes shellcheck." rationale = "docs/engineering/skills/ci-workflows.md#writing-a-workflow-script" scope = [".github/scripts/*.sh"] tier = "mechanical" -status = "ratcheted" +status = "enforced" autofix = false [rule.enforcer] kind = "quality-guard" @@ -722,9 +719,6 @@ name = "ci-scripts" workflow = ".github/workflows/pr.yml" job = "quality-guards" invocation = "run_quality_guards.py" -[rule.baseline] -file = "rules/baselines/ci-shellcheck.txt" -count = 1 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/shellcheck/bad_script.sh" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/shellcheck/good_script.sh" diff --git a/scripts/rules/ci_scripts.py b/scripts/rules/ci_scripts.py index 02b5f3f47..5a0a6a066 100644 --- a/scripts/rules/ci_scripts.py +++ b/scripts/rules/ci_scripts.py @@ -25,6 +25,13 @@ REPO_ROOT = Path(__file__).resolve().parents[2] SEPARATOR = " :: " +# Rule ids end with these names (``.ci.``). Used to attribute a +# finding to its rule once the rule is enforced and has no baseline left. +RULE_NAMES = { + "multiline": "multiline-run-is-script", + "untested": "workflow-scripts-tested", + "shellcheck": "shell-scripts-pass-shellcheck", +} @dataclass @@ -118,10 +125,15 @@ def _baseline(root: Path, path: str) -> set[str]: } -def _rule_for(registry: Registry, baseline: str) -> str | None: +def _rule_for(registry: Registry, baseline: str, check: str) -> str | None: for rule in registry.rules: if rule.baseline is not None and rule.baseline.file == baseline: return rule.id + for rule in registry.rules: + if rule.enforcer.get("name") == "ci-scripts" and rule.id.endswith( + f".ci.{RULE_NAMES[check]}" + ): + return rule.id return None @@ -130,6 +142,7 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio assert config is not None checks = ( ( + "multiline", "multi-line run: block", findings.multiline, config.multiline_baseline, @@ -137,12 +150,14 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio f"{config.scripts_dir} and call it from a single-line run:", ), ( + "untested", "untested workflow script", findings.untested, config.untested_baseline, "add a test that runs the script (see docs/engineering/skills/ci-workflows.md)", ), ( + "shellcheck", "shellcheck finding in", findings.shellcheck, config.shellcheck_baseline, @@ -150,10 +165,10 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio ), ) violations = [] - for label, found, baseline_path, fix in checks: + for check, label, found, baseline_path, fix in checks: if found is None: continue - rule_id = _rule_for(registry, baseline_path) + rule_id = _rule_for(registry, baseline_path, check) baseline = _baseline(root, baseline_path) for entry in sorted(found - baseline): violations.append(Violation(rule_id, "ci-scripts", f"{label} {entry}", fix)) @@ -199,7 +214,9 @@ def run_check(root: Path = REPO_ROOT) -> tuple[list[Violation], list[str]]: if findings.shellcheck is None and os.environ.get("GITHUB_ACTIONS"): violations.append( Violation( - _rule_for(registry, registry.ci_scripts.shellcheck_baseline), + _rule_for( + registry, registry.ci_scripts.shellcheck_baseline, "shellcheck" + ), "ci-scripts", "shellcheck is not installed in this CI job", "install it in the job (pip install shellcheck-py==)", diff --git a/scripts/rules/tests/test_ci_scripts.py b/scripts/rules/tests/test_ci_scripts.py index a4ec38b7c..6b76f396c 100644 --- a/scripts/rules/tests/test_ci_scripts.py +++ b/scripts/rules/tests/test_ci_scripts.py @@ -189,6 +189,22 @@ def test_new_finding_fails_with_its_rule_id(self) -> None: messages[0], ) + def test_enforced_rule_without_baseline_keeps_its_id(self) -> None: + findings = Findings(multiline=set(), untested={"x.sh"}, shellcheck=set()) + registry, _ = load_text( + REGISTRY.replace("ex.ci.multiline", "ex.ci.workflow-scripts-tested") + .replace('status = "ratcheted"', 'status = "enforced"') + .replace( + '[rule.baseline]\nfile = "rules/baselines/ci-multiline-run.txt"\ncount = 1\n', + "", + ) + ) + assert registry is not None + messages = [str(v) for v in compare(registry, findings, self.root)] + self.assertTrue( + messages[-1].startswith("[ex.ci.workflow-scripts-tested]"), messages + ) + def test_fixed_finding_must_leave_the_baseline(self) -> None: messages = self.messages(set()) self.assertIn(