diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index d639c865c..5912934a1 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -20,8 +20,6 @@ jobs: if: ${{ always() }} needs: - ensure-policyengine-bundle-supported-by-simulation-api - - lint - - typecheck-v2 - quality-guards - alembic-v1-check - alembic-v2-check @@ -72,44 +70,11 @@ jobs: - name: Check the PolicyEngine bundle is the simulation API's latest run: bash .github/check-policyengine-bundle-supported.sh - 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: - - name: Checkout repo - uses: actions/checkout@v4 - - name: Setup Python - uses: actions/setup-python@v5 - with: - python-version: "3.12" - - name: Install ruff (version pinned in uv.lock) - run: bash .github/scripts/install_locked_ruff.sh - - name: Format check with ruff - run: ruff format --check . - - name: Lint with ruff - run: ruff check . - typecheck-v2: - name: Type-check API v2 - # 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: - - name: Checkout repo - uses: actions/checkout@v4 - - name: Setup Python - uses: actions/setup-python@v5 - with: - python-version: "3.12" - - name: Setup uv - uses: astral-sh/setup-uv@v6 - - name: Install locked dependencies - run: uv sync --frozen --extra dev - - name: Type-check API v2 - run: uv run --frozen --extra dev mypy quality-guards: - name: Quality guards + # make quality, as an agent runs it locally (api.quality.make-quality): + # ruff's fixes, then uv lock --check, ruff, Import Linter, mypy and the + # quality guards. + name: Quality checks # 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 @@ -132,17 +97,18 @@ jobs: uses: astral-sh/setup-uv@v6 with: version: "0.13.4" - - name: Check uv.lock matches pyproject.toml - run: uv lock --check - - name: Test the rules registry tool - run: uv run --frozen --extra dev python -m unittest discover -s scripts/rules/tests -t . - # The v2 import contracts in [tool.importlinter] in pyproject.toml. - - name: Check v2 import contracts with Import Linter - run: uv run --frozen --extra dev lint-imports --config pyproject.toml - name: Install shellcheck (for the ci-scripts guard) run: pip install "shellcheck-py==0.11.0.1" - - name: Run quality guards - run: python scripts/run_quality_guards.py + - name: Test the rules registry tool + run: uv run --frozen --extra dev python -m unittest discover -s scripts/rules/tests -t . + # uv lock --check, ruff, the v2 import contracts in [tool.importlinter] + # in pyproject.toml, mypy and the quality guards, after ruff's fixes. + - name: Run make quality + run: make quality + # Fails, and shows the change, if ruff's fixes changed a file: the + # pull request must include them. HEAD, so staged changes count too. + - name: Check that ruff's fixes changed no file + run: git diff --exit-code HEAD alembic-v1-check: name: Alembic v1 qualification diff --git a/AGENTS.md b/AGENTS.md index 8bf5b3b92..6ab0e23ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,6 +63,11 @@ named `changelog.d/..md`, where `` is `breaking`, `added`, `changed`, `fixed` or `removed` (see "Changelog fragments" in that skill). Agents must never bypass branch rules (for example with `gh pr merge --admin`). +Before committing, run `make quality`: it applies ruff's fixes, then runs +the checks pr.yml's `quality-guards` job runs, apart from the rules +registry tool's own tests (see "Commit Hygiene" in +`docs/engineering/skills/github-prs.md` for how a local run can differ). + Before creating or sharing a migration PR, run the focused migration guards: ```bash @@ -118,7 +123,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Code quality and types -- Checked: `api.quality.alembic-revisions-unused-imports`, `api.quality.ci-cd-scripts-ruff-exemptions`, `api.quality.command-line-modules-ruff-exemptions`, `api.quality.command-line-scripts-ruff-exemptions`, `api.quality.coverage-measured`, `api.quality.diff-coverage`, `api.quality.environment-variables-load`, `api.quality.file-length`, `api.quality.mypy-v2`, `api.quality.ruff-format`, `api.quality.ruff-lint`, `api.quality.runtime-evaluated-models-resolve`, `api.quality.suppressions-explained`, `api.quality.tests-assert`, `api.quality.tests-pass`, `api.quality.tests-ruff-exemptions`, `api.quality.unittest-ruff-exemptions` +- Checked: `api.quality.alembic-revisions-unused-imports`, `api.quality.ci-cd-scripts-ruff-exemptions`, `api.quality.command-line-modules-ruff-exemptions`, `api.quality.command-line-scripts-ruff-exemptions`, `api.quality.coverage-measured`, `api.quality.diff-coverage`, `api.quality.environment-variables-load`, `api.quality.file-length`, `api.quality.make-quality`, `api.quality.mypy-v2`, `api.quality.ruff-format`, `api.quality.ruff-lint`, `api.quality.runtime-evaluated-models-resolve`, `api.quality.suppressions-explained`, `api.quality.tests-assert`, `api.quality.tests-pass`, `api.quality.tests-ruff-exemptions`, `api.quality.unittest-ruff-exemptions` ### Observability diff --git a/Makefile b/Makefile index d62b61b3b..736532b6d 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: install setup-env debug debug-asgi test-env-vars test quality-guards typecheck-v2 debug-test format changelog +.PHONY: install setup-env debug debug-asgi test-env-vars test quality quality-check quality-guards typecheck-v2 debug-test format changelog # Install and test in the environment uv.lock defines, on the Python version # the Cloud Run image runs (gcp/cloud_run/Dockerfile). .python-version selects @@ -26,6 +26,28 @@ test: MAX_HOUSEHOLDS=1000 uv run --frozen --extra dev --group telemetry-error-issues coverage run -a --branch -m pytest tests/to_refactor tests/unit tests/contract tests/integration/test_budget_window_in_flight_dedupe.py tests/integration/test_installed_us_model_spm.py --disable-pytest-warnings uv run --frozen --extra dev coverage xml -i +# The quality checks, the same locally and in pr.yml's quality-guards job +# (api.quality.make-quality): ruff's fixes first, then every check. +quality: + uv run --frozen --extra dev ruff check --fix . + uv run --frozen --extra dev ruff format . + $(MAKE) quality-check + +# Every check, changing no file. diff-cover isn't here: it needs every test +# shard's coverage report, so pr.yml runs it in a job of its own. The quality +# guards run without site-packages (python -S), so they keep to the standard +# library as push.yml runs them. The rules guard compares baselines and rule +# statuses with RULES_BASE_REF: the commit HEAD shares with the base branch, +# which CI names in GITHUB_BASE_REF, or master. So a branch is compared with +# where it left master, as CI compares its merge with master. +quality-check: + uv lock --check + uv run --frozen --extra dev ruff check . + uv run --frozen --extra dev ruff format --check . + uv run --frozen --extra dev lint-imports --config pyproject.toml + uv run --frozen --extra dev mypy + RULES_BASE_REF=$$(git merge-base HEAD origin/$${GITHUB_BASE_REF:-master}) uv run --frozen --extra dev python -S scripts/run_quality_guards.py + quality-guards: python scripts/run_quality_guards.py diff --git a/README.md b/README.md index d833a4339..8a03ceb52 100644 --- a/README.md +++ b/README.md @@ -210,13 +210,13 @@ We require that you add tests for any new features or bugfixes. Our tests are wr ### Step 2: Formatting -In addition to the tests, we use [Black](https://github.com/psf/black) to lint our codebase, so before opening a pull request, Step 2 is to lint the code by running +In addition to the tests, we check the code with ruff, Import Linter, mypy and the repository's quality guards, so before opening a pull request, Step 2 is to run ``` -make format +make quality ``` -This will automatically format the code for you; no need to do anything else. +This applies ruff's fixes, then runs the checks the pull request's `Quality checks` job runs, apart from the rules registry tool's own tests. Commit the fixes it makes. ### Step 3: Changelogging diff --git a/changelog.d/4202.added.md b/changelog.d/4202.added.md new file mode 100644 index 000000000..7d34bfa1e --- /dev/null +++ b/changelog.d/4202.added.md @@ -0,0 +1 @@ +Added `make quality`, which applies ruff's fixes and then runs every quality check (`uv lock --check`, ruff, Import Linter, mypy and the quality guards); pull request CI now runs the same command in one job and fails if the fixes changed a file. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 4f8c38e54..f1a71b6a0 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -188,14 +188,56 @@ the reason a code change or ruff setting couldn't remove the finding ## Commit Hygiene -AI agents must run formatting and lint checks before committing their own code +AI agents must run the quality checks before committing their own code changes: ```bash -make format -ruff check +make quality ``` -Commit only after formatting succeeds and changed Python files pass lint. If a -broader repo-wide lint command fails on unrelated pre-existing issues, include -that result in the handoff instead of hiding it. +`make quality` applies ruff's fixes (`ruff check --fix`, then `ruff format`), +then runs `make quality-check`: `uv lock --check`, `ruff check`, +`ruff format --check`, Import Linter, mypy and the quality guards +(`scripts/run_quality_guards.py`), without changing files. Commit the fixes +it makes with your change. pr.yml's `quality-guards` job runs the same +command and fails if the fixes changed a file (`api.quality.make-quality`). +Keep the Makefile to rules, recipes and variables. The rules registry +reads it to see which checks the job runs, and gives no credit to a +Makefile that: + +- uses `include`, a conditional, `define`, `$(eval ...)`, a recipe after + `;`, or a computed target or variable name; +- sets something that can hide failures (`.IGNORE`, `.ONESHELL`, + `MAKEFLAGS`, `GNUMAKEFLAGS`, `MAKEFILES`, `SHELL`, `.SHELLFLAGS`); +- or has a `GNUmakefile` or `makefile` beside it. + +`tests/unit/test_make_quality.py` also runs `make quality` with a stand-in +for uv, and fails unless make runs every check and fails whenever one of +them fails. + +A local run can still differ from CI's: + +- The job also runs the rules registry tool's own tests + (`scripts/rules/tests`), which `make quality` leaves out. +- The rules guard compares baselines and rule statuses with + `RULES_BASE_REF`, which `make quality-check` sets to the commit `HEAD` + shares with `origin/`: CI's `GITHUB_BASE_REF`, or `master` + locally. A branch is compared with where it left `master`; CI compares + its merge with `master` with the commit that merge is built on. Without + `origin/master`, or in a shallow clone without the merge base, git + reports an error and the local comparison is skipped, so fetch `origin` + with its history first. +- `uv run --frozen --extra dev` installs what the `dev` extra needs but + removes nothing, so packages `make test` added for other groups stay in + `.venv` and mypy, Import Linter and deptry can see them locally. +- Without `shellcheck` installed, the ci-scripts guard skips that check + locally; CI installs it and fails without it. +- `uv lock --check` runs your uv; CI pins uv 0.13.4. +- `make quality` stops at the first failing check, so fix it and run it + again to see the next one. + +diff-cover isn't part of it: it needs every test shard's coverage report +(see the testing skill). + +Commit only after `make quality` passes. If a check fails on unrelated +pre-existing issues, include that result in the handoff instead of hiding it. diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 302585e85..3923eeab1 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -16,7 +16,7 @@ registry, the checkers and the rendered views disagree. ```bash python -m scripts.rules validate # schema and file checks only python -m scripts.rules render # rewrite rules.md and the AGENTS.md block -python -m scripts.rules guard --base-ref origin/ # everything CI checks +python -m scripts.rules guard --base-ref "$(git merge-base HEAD origin/)" # everything CI checks python -m unittest discover -s scripts/rules/tests -t . # the tool's own tests ``` @@ -57,7 +57,7 @@ gap is visible, rather than leaving it out. | `autofix` | Whether the enforcer can fix violations itself (e.g. ruff format). | | `review_by` | Optional date; the guard fails once it passes. | | `[rule.enforcer]` | What checks the rule; `kind` plus kind-specific fields. | -| `[[rule.ci]]` | `workflow`, `job`, and an `invocation` string that must appear in that job's `run:` text (or its reusable-workflow `uses:`). | +| `[[rule.ci]]` | `workflow`, `job`, and an `invocation` string that must appear in that job's `run:` text (or its reusable-workflow `uses:`), or in a command of a Makefile target the job runs with `make `, following the targets' prerequisites and the `$(MAKE)` calls in their recipes (`scripts/rules/makefile.py`). | | `[rule.proof]` | Optional; see "Proof". | | `[rule.baseline]` | Only for `ratcheted` rules: `file` listing tolerated violations, and their `count`. | | `[[rule.exception]]` | Carve-out: `path`, `reason`, and `expires`; or, for rules whose guard reads them, `match` (one site), `removal`, `issue` and `category` ([dependencies.md](dependencies.md#exceptions); for `api.data.sqlalchemy-and-alembic-only`, `match` is the exact construct and `category` is `driver-access`, `schema-change` or `migration-revision`: [alembic-migrations.md](alembic-migrations.md#declared-exceptions)). | diff --git a/docs/engineering/skills/testing.md b/docs/engineering/skills/testing.md index 931dcf84b..2bb1a9be0 100644 --- a/docs/engineering/skills/testing.md +++ b/docs/engineering/skills/testing.md @@ -444,17 +444,13 @@ avoid depending on specific production data fixtures: API_BASE_URL=https://candidate-url python -m pytest tests/integration/test_cloud_run_candidate.py -v ``` -Before committing AI-authored code changes, run repository formatting and lint: +Before committing AI-authored code changes, run the quality checks, as +"Commit Hygiene" in the GitHub PRs skill describes: ```bash -make format -ruff check +make quality ``` -Commit only after formatting succeeds and changed Python files pass lint. If a -broader repo-wide lint command fails on unrelated pre-existing issues, include -that result in the handoff instead of hiding it. - ## Phase 10 Policy Migration Run the configured static type check for the Phase 10 v2 query, route, diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 3a9edb3b9..8a5de8773 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 69 | 7 | 1 | 8 | 85 | +| mechanical | 70 | 7 | 1 | 8 | 86 | | metric | 2 | 0 | 0 | 0 | 2 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 78 of 88 rules. +**Blocking:** 79 of 89 rules. ## Gaps @@ -368,22 +368,23 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `api.quality.alembic-revisions-unused-imports` | Ruff neither lints nor formats Alembic revision files (extend-exclude in pyproject.toml), because they must stay exactly as Alembic generates them. | mechanical | advisory | no | nothing | — | — | -| `api.quality.ci-cd-scripts-ruff-exemptions` | CI/CD scripts under .github may print their results, run other programs, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S603, S607, S310, N999, TID253); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of N999, S310, S603, S607, T201, TID253 for `.github/**` | `pr.yml:lint` | — | -| `api.quality.command-line-modules-ruff-exemptions` | Application modules that also run as command-line programs (if __name__ == "__main__") may print their output (T201); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of T201 for `policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/privilege_reconciliation,data/v2/user_association_migration_qualification,worker_spm_release}.py` | `pr.yml:lint` | — | -| `api.quality.command-line-scripts-ruff-exemptions` | Command-line scripts under scripts/ may print their output, run other programs, fetch fixed URLs, stamp today's date and pick random samples (T201, S603, S607, S310, DTZ011, S311); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of DTZ011, S310, S311, S603, S607, T201 for `scripts/**` | `pr.yml:lint` | — | +| `api.quality.ci-cd-scripts-ruff-exemptions` | CI/CD scripts under .github may print their results, run other programs, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S603, S607, S310, N999, TID253); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of N999, S310, S603, S607, T201, TID253 for `.github/**` | `pr.yml:quality-guards` | — | +| `api.quality.command-line-modules-ruff-exemptions` | Application modules that also run as command-line programs (if __name__ == "__main__") may print their output (T201); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of T201 for `policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/privilege_reconciliation,data/v2/user_association_migration_qualification,worker_spm_release}.py` | `pr.yml:quality-guards` | — | +| `api.quality.command-line-scripts-ruff-exemptions` | Command-line scripts under scripts/ may print their output, run other programs, fetch fixed URLs, stamp today's date and pick random samples (T201, S603, S607, S310, DTZ011, S311); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of DTZ011, S310, S311, S603, S607, T201 for `scripts/**` | `pr.yml:quality-guards` | — | | `api.quality.coverage-measured` | Codecov measures the same tests on master as on pull requests: every CI job that measures test coverage sends it to Codecov with a flag that codecov.yml carries forward (unit for make test, in pr.yml and in push.yml's release tests; integration for the cross-database integration tests), passing the CODECOV_TOKEN secret, from a job with a timeout of at most 15 minutes; every shard of a report is uploaded, and in a master run an upload fails the run when it fails and no other job waits for it. | mechanical | enforced | yes | pytest `tests/unit/test_codecov_coverage.py` | `pr.yml:test` | yes | | `api.quality.diff-coverage` | The lines a pull request changes in policyengine_api that coverage.py measures (the first line of each statement) are at least 80% covered by the unit and integration tests together: pr.yml's diff-coverage job runs diff-cover --fail-under=80 on every test shard's and the integration tests' coverage reports, against the commit that the merge commit CI checks out is built on (HEAD^1: the base branch, or a lower stacked pull request's merge commit), and CI complete needs it. | metric | enforced | yes | CI job `diff-coverage` | `pr.yml:diff-coverage` | yes | | `api.quality.environment-variables-load` | Required environment variables and credentials load in a deployed-like environment. | mechanical | enforced | yes | CI job `test_env_vars` | `pr.yml:test_env_vars` | — | | `api.quality.file-length` | A tracked Python file has at most 800 physical lines, or at most 1,500 if it is test code (a file under a tests or fixtures directory, or a conftest.py); a longer file is split by responsibility into modules, and keeps its public import path. | metric | enforced | yes | quality guard `file-length` | `pr.yml:quality-guards` | yes | -| `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, the gzip negotiation, the user sign-in and account-deletion libraries, the country list and its startup check pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:typecheck-v2` | — | -| `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:lint` | — | -| `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:lint` | — | +| `api.quality.make-quality` | make quality is the command for the quality checks, locally and in CI: it applies ruff check --fix and ruff format, then runs make quality-check, which runs uv lock --check, ruff check, ruff format --check, Import Linter, mypy and the quality guards without changing files. Run with a stand-in uv, make quality runs each of them and fails whenever one fails, and make -n prints the same commands. pr.yml's quality-guards job runs make quality and then git diff --exit-code HEAD, with neither step skipped, allowed to fail, or run in another shell or directory, no MAKEFLAGS, GNUMAKEFLAGS or MAKEFILES set for them, and no earlier step writing to GITHUB_ENV or GITHUB_PATH; the Makefile uses no syntax the rules registry doesn't follow and nothing that hides a failing command; and no other pr.yml job's commands run ruff, mypy, lint-imports or the quality guards, directly or through a make target. | mechanical | enforced | yes | pytest `tests/unit/test_make_quality.py` | `pr.yml:test` | yes | +| `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, the gzip negotiation, the user sign-in and account-deletion libraries, the country list and its startup check pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:quality-guards` | — | +| `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:quality-guards` | — | +| `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:quality-guards` | — | | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** | | `api.quality.suppressions-explained` | Every noqa, ruff: disable and type: ignore comment names what it suppresses and says why: a noqa comment, line-level or file-level (ruff: noqa, flake8: noqa), lists its ruff codes, a ruff: disable range suppression lists its ruff codes in brackets, a type: ignore comment lists its mypy error codes in brackets, and the comment also gives a reason in another # part, as # noqa: CODE # reason or # type: ignore[code] # reason; apart from the suppressions rules/baselines/suppressions.txt lists. | mechanical | ratcheted | yes | quality guard `suppressions` | `pr.yml:quality-guards`, `push.yml:quality-guards` | yes | | `api.quality.tests-assert` | Every test function pytest or unittest collects checks something: its body contains an assert, a raise AssertionError, pytest.raises, pytest.warns, pytest.deprecated_call or pytest.fail, a call to an assert function or method (unittest's self.assertEqual, a mock's assert_called_once_with) or self.fail, or a call to a test helper that does (a function of the same file, a method of the test's class, or a function imported by name from test code); apart from the tests rules/baselines/tests-without-checks.txt lists. | mechanical | ratcheted | yes | quality guard `asserting-tests` | `pr.yml:quality-guards`, `push.yml:quality-guards` | yes | | `api.quality.tests-pass` | The unit, contract and to-refactor test suites pass, each test in exactly one of the shards they run in. | mechanical | enforced | yes | CI job `test`, `check-test-shards` | `pr.yml:test`, `pr.yml:check-test-shards` | — | -| `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert, need no docstrings, annotations or named constants for magic numbers, and may reach into private members, live outside packages and import inside a test (S101, D1, ANN, PLR2004, SLF001, INP001, PLC0415). Their fakes may name methods after the interface they replace, such as a gRPC stub's Export (N802). They may also use fake credentials, naive datetimes, subprocess calls to repository scripts, SQL built from strings and prints (S105, S106, DTZ001, DTZ011, S603, S607, S608, T201), may import the lazily loaded packages at module level (TID253), and may leave a stand-in's arguments unused (ARG001, ARG002, ARG005). Every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, ARG001, ARG002, ARG005, D1, DTZ001, DTZ011, INP001, N802, PLC0415, PLR2004, S101, S105, S106, S603, S607, S608, SLF001, T201, TID253 for `**/tests/**` | `pr.yml:lint` | — | -| `api.quality.unittest-ruff-exemptions` | The shared rules tool's tests use unittest, because the tool needs only the standard library, so ruff's pytest-style rules (PT) don't apply to them. | mechanical | enforced | yes | ruff per-file ignore of PT for `scripts/rules/tests/**` | `pr.yml:lint` | — | +| `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert, need no docstrings, annotations or named constants for magic numbers, and may reach into private members, live outside packages and import inside a test (S101, D1, ANN, PLR2004, SLF001, INP001, PLC0415). Their fakes may name methods after the interface they replace, such as a gRPC stub's Export (N802). They may also use fake credentials, naive datetimes, subprocess calls to repository scripts, SQL built from strings and prints (S105, S106, DTZ001, DTZ011, S603, S607, S608, T201), may import the lazily loaded packages at module level (TID253), and may leave a stand-in's arguments unused (ARG001, ARG002, ARG005). Every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, ARG001, ARG002, ARG005, D1, DTZ001, DTZ011, INP001, N802, PLC0415, PLR2004, S101, S105, S106, S603, S607, S608, SLF001, T201, TID253 for `**/tests/**` | `pr.yml:quality-guards` | — | +| `api.quality.unittest-ruff-exemptions` | The shared rules tool's tests use unittest, because the tool needs only the standard library, so ruff's pytest-style rules (PT) don't apply to them. | mechanical | enforced | yes | ruff per-file ignore of PT for `scripts/rules/tests/**` | `pr.yml:quality-guards` | — | ```mermaid flowchart LR @@ -391,7 +392,7 @@ flowchart LR gap_unenforced>"not enforced"] r_api_quality_ci_cd_scripts_ruff_exemptions("api.quality.ci-cd-scripts-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___["ruff per-file ignore of N999, S310, S603, S607, T201, TID253 for .github/**"] - j_CI_pr_yml_lint["CI pr.yml:lint"] + j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_quality_command_line_modules_ruff_exemptions("api.quality.command-line-modules-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py["ruff per-file ignore of T201 for policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/privilege_reconciliation,data/v2/user_association_migration_qualification,worker_spm_release}.py"] r_api_quality_command_line_scripts_ruff_exemptions("api.quality.command-line-scripts-ruff-exemptions (enforced)") @@ -407,10 +408,10 @@ flowchart LR j_CI_pr_yml_test_env_vars["CI pr.yml:test_env_vars"] r_api_quality_file_length("api.quality.file-length (enforced)") e_quality_guard_file_length["quality guard file-length"] - j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] + r_api_quality_make_quality("api.quality.make-quality (enforced)") + e_pytest_tests_unit_test_make_quality_py["pytest tests/unit/test_make_quality.py"] r_api_quality_mypy_v2("api.quality.mypy-v2 (enforced)") e_mypy["mypy"] - j_CI_pr_yml_typecheck_v2["CI pr.yml:typecheck-v2"] r_api_quality_ruff_format("api.quality.ruff-format (enforced)") e_ruff_format["ruff format"] r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)") @@ -422,16 +423,13 @@ flowchart LR j_CI_push_yml_quality_guards["CI push.yml:quality-guards"] r_api_quality_tests_assert("api.quality.tests-assert (ratcheted)") e_quality_guard_asserting_tests["quality guard asserting-tests"] - r_api_quality_tests_pass("api.quality.tests-pass (enforced)") - e_CI_job_test__check_test_shards["CI job test, check-test-shards"] - j_CI_pr_yml_check_test_shards["CI pr.yml:check-test-shards"] r_api_quality_alembic_revisions_unused_imports -.-> gap_unenforced r_api_quality_ci_cd_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ - e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ --> j_CI_pr_yml_lint + e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ --> j_CI_pr_yml_quality_guards r_api_quality_command_line_modules_ruff_exemptions --> e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py - e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py --> j_CI_pr_yml_lint + e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py --> j_CI_pr_yml_quality_guards r_api_quality_command_line_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_DTZ011__S310__S311__S603__S607__T201_for_scripts___ - e_ruff_per_file_ignore_of_DTZ011__S310__S311__S603__S607__T201_for_scripts___ --> j_CI_pr_yml_lint + e_ruff_per_file_ignore_of_DTZ011__S310__S311__S603__S607__T201_for_scripts___ --> j_CI_pr_yml_quality_guards r_api_quality_coverage_measured --> e_pytest_tests_unit_test_codecov_coverage_py e_pytest_tests_unit_test_codecov_coverage_py --> j_CI_pr_yml_test r_api_quality_diff_coverage --> e_CI_job_diff_coverage @@ -440,12 +438,14 @@ flowchart LR e_CI_job_test_env_vars --> j_CI_pr_yml_test_env_vars r_api_quality_file_length --> e_quality_guard_file_length e_quality_guard_file_length --> j_CI_pr_yml_quality_guards + r_api_quality_make_quality --> e_pytest_tests_unit_test_make_quality_py + e_pytest_tests_unit_test_make_quality_py --> j_CI_pr_yml_test r_api_quality_mypy_v2 --> e_mypy - e_mypy --> j_CI_pr_yml_typecheck_v2 + e_mypy --> j_CI_pr_yml_quality_guards r_api_quality_ruff_format --> e_ruff_format - e_ruff_format --> j_CI_pr_yml_lint + e_ruff_format --> j_CI_pr_yml_quality_guards r_api_quality_ruff_lint --> e_ruff_check__ALL_ - e_ruff_check__ALL_ --> j_CI_pr_yml_lint + e_ruff_check__ALL_ --> j_CI_pr_yml_quality_guards r_api_quality_runtime_evaluated_models_resolve --> e_pytest_tests_unit_test_runtime_evaluated_annotations_py e_pytest_tests_unit_test_runtime_evaluated_annotations_py --> j_CI_pr_yml_test r_api_quality_suppressions_explained --> e_quality_guard_suppressions @@ -454,22 +454,26 @@ flowchart LR r_api_quality_tests_assert --> e_quality_guard_asserting_tests e_quality_guard_asserting_tests --> j_CI_pr_yml_quality_guards e_quality_guard_asserting_tests --> j_CI_push_yml_quality_guards - r_api_quality_tests_pass --> e_CI_job_test__check_test_shards - e_CI_job_test__check_test_shards --> j_CI_pr_yml_test - e_CI_job_test__check_test_shards --> j_CI_pr_yml_check_test_shards ``` ```mermaid flowchart LR + r_api_quality_tests_pass("api.quality.tests-pass (enforced)") + e_CI_job_test__check_test_shards["CI job test, check-test-shards"] + j_CI_pr_yml_test["CI pr.yml:test"] + j_CI_pr_yml_check_test_shards["CI pr.yml:check-test-shards"] r_api_quality_tests_ruff_exemptions("api.quality.tests-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_ANN__ARG001__ARG002__ARG005__D1__DTZ001__DTZ011__INP001__N802__PLC0415__PLR2004__S101__S105__S106__S603__S607__S608__SLF001__T201__TID253_for____tests___["ruff per-file ignore of ANN, ARG001, ARG002, ARG005, D1, DTZ001, DTZ011, INP001, N802, PLC0415, PLR2004, S101, S105, S106, S603, S607, S608, SLF001, T201, TID253 for **/tests/**"] - j_CI_pr_yml_lint["CI pr.yml:lint"] + j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_quality_unittest_ruff_exemptions("api.quality.unittest-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___["ruff per-file ignore of PT for scripts/rules/tests/**"] + r_api_quality_tests_pass --> e_CI_job_test__check_test_shards + e_CI_job_test__check_test_shards --> j_CI_pr_yml_test + e_CI_job_test__check_test_shards --> j_CI_pr_yml_check_test_shards r_api_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__ARG001__ARG002__ARG005__D1__DTZ001__DTZ011__INP001__N802__PLC0415__PLR2004__S101__S105__S106__S603__S607__S608__SLF001__T201__TID253_for____tests___ - e_ruff_per_file_ignore_of_ANN__ARG001__ARG002__ARG005__D1__DTZ001__DTZ011__INP001__N802__PLC0415__PLR2004__S101__S105__S106__S603__S607__S608__SLF001__T201__TID253_for____tests___ --> j_CI_pr_yml_lint + e_ruff_per_file_ignore_of_ANN__ARG001__ARG002__ARG005__D1__DTZ001__DTZ011__INP001__N802__PLC0415__PLR2004__S101__S105__S106__S603__S607__S608__SLF001__T201__TID253_for____tests___ --> j_CI_pr_yml_quality_guards r_api_quality_unittest_ruff_exemptions --> e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___ - e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___ --> j_CI_pr_yml_lint + e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___ --> j_CI_pr_yml_quality_guards ``` ## Observability diff --git a/rules/fixtures/api.quality.make-quality/accepted/Makefile b/rules/fixtures/api.quality.make-quality/accepted/Makefile new file mode 100644 index 000000000..af66dc905 --- /dev/null +++ b/rules/fixtures/api.quality.make-quality/accepted/Makefile @@ -0,0 +1,16 @@ +# Proof fixture for api.quality.make-quality (accepted), read with +# accepted/pr.yml: make quality applies ruff's fixes, then make quality-check +# runs every check without changing files. + +quality: + uv run --frozen --extra dev ruff check --fix . + uv run --frozen --extra dev ruff format . + $(MAKE) quality-check + +quality-check: + uv lock --check + uv run --frozen --extra dev ruff check . + uv run --frozen --extra dev ruff format --check . + uv run --frozen --extra dev lint-imports --config pyproject.toml + uv run --frozen --extra dev mypy + RULES_BASE_REF=$$(git merge-base HEAD origin/$${GITHUB_BASE_REF:-master}) uv run --frozen --extra dev python -S scripts/run_quality_guards.py diff --git a/rules/fixtures/api.quality.make-quality/accepted/pr.yml b/rules/fixtures/api.quality.make-quality/accepted/pr.yml new file mode 100644 index 000000000..341ef0381 --- /dev/null +++ b/rules/fixtures/api.quality.make-quality/accepted/pr.yml @@ -0,0 +1,15 @@ +# Proof fixture for api.quality.make-quality (accepted), read with +# accepted/Makefile: the quality-guards job runs make quality, then fails if +# ruff's fixes changed a file, and no other job runs a check itself. +jobs: + ci-complete: + needs: + - test + - quality-guards + test: + steps: + - run: make test + quality-guards: + steps: + - run: make quality + - run: git diff --exit-code HEAD diff --git a/rules/fixtures/api.quality.make-quality/rejected/Makefile b/rules/fixtures/api.quality.make-quality/rejected/Makefile new file mode 100644 index 000000000..df410661e --- /dev/null +++ b/rules/fixtures/api.quality.make-quality/rejected/Makefile @@ -0,0 +1,15 @@ +# Proof fixture for api.quality.make-quality (rejected), read with +# rejected/pr.yml: make quality-check leaves out mypy, and pr.yml +# type-checks in a job of its own, so a local run and CI can differ. + +quality: + uv run --frozen --extra dev ruff check --fix . + uv run --frozen --extra dev ruff format . + $(MAKE) quality-check + +quality-check: + uv lock --check + uv run --frozen --extra dev ruff check . + uv run --frozen --extra dev ruff format --check . + uv run --frozen --extra dev lint-imports --config pyproject.toml + RULES_BASE_REF=$$(git merge-base HEAD origin/$${GITHUB_BASE_REF:-master}) uv run --frozen --extra dev python -S scripts/run_quality_guards.py diff --git a/rules/fixtures/api.quality.make-quality/rejected/pr.yml b/rules/fixtures/api.quality.make-quality/rejected/pr.yml new file mode 100644 index 000000000..2b0973d29 --- /dev/null +++ b/rules/fixtures/api.quality.make-quality/rejected/pr.yml @@ -0,0 +1,18 @@ +# Proof fixture for api.quality.make-quality (rejected), read with +# rejected/Makefile: a typecheck job runs mypy itself, beside make quality. +jobs: + ci-complete: + needs: + - test + - typecheck + - quality-guards + test: + steps: + - run: make test + typecheck: + steps: + - run: uv run --frozen --extra dev mypy + quality-guards: + steps: + - run: make quality + - run: git diff --exit-code HEAD diff --git a/rules/registry.toml b/rules/registry.toml index bd7cba33f..6b09b8ecb 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1195,6 +1195,32 @@ test = "tests/unit/services/test_microdata_weight_boundaries.py::test_rule_proof # --- Code quality and types ------------------------------------------------ +[[rule]] +id = "api.quality.make-quality" +category = "quality" +statement = "make quality is the command for the quality checks, locally and in CI: it applies ruff check --fix and ruff format, then runs make quality-check, which runs uv lock --check, ruff check, ruff format --check, Import Linter, mypy and the quality guards without changing files. Run with a stand-in uv, make quality runs each of them and fails whenever one fails, and make -n prints the same commands. pr.yml's quality-guards job runs make quality and then git diff --exit-code HEAD, with neither step skipped, allowed to fail, or run in another shell or directory, no MAKEFLAGS, GNUMAKEFLAGS or MAKEFILES set for them, and no earlier step writing to GITHUB_ENV or GITHUB_PATH; the Makefile uses no syntax the rules registry doesn't follow and nothing that hides a failing command; and no other pr.yml job's commands run ruff, mypy, lint-imports or the quality guards, directly or through a make target." +rationale = "docs/engineering/skills/github-prs.md#commit-hygiene" +scope = ["Makefile", ".github/workflows/pr.yml"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "pytest" +nodes = [ + "tests/unit/test_make_quality.py::test_make_quality_runs_every_check_and_ci_runs_it", + "tests/unit/test_make_quality.py::test_make_quality_fails_whenever_a_check_fails", + "tests/unit/test_make_quality.py::test_make_itself_runs_the_listed_commands", + "tests/unit/test_make_quality.py::test_the_quality_job_can_run_every_check", +] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" +[rule.proof] +bad_fixture = "rules/fixtures/api.quality.make-quality/rejected" +good_fixture = "rules/fixtures/api.quality.make-quality/accepted" +test = "tests/unit/test_make_quality.py::test_rule_proof_make_quality" + [[rule]] id = "api.quality.ruff-format" category = "quality" @@ -1209,7 +1235,7 @@ kind = "ruff" mode = "format" [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff format --check" [[rule]] @@ -1227,7 +1253,7 @@ mode = "lint" codes = ["ALL"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." [[rule]] @@ -1281,7 +1307,7 @@ pattern = "**/tests/**" codes = ["ANN", "ARG001", "ARG002", "ARG005", "D1", "DTZ001", "DTZ011", "INP001", "N802", "PLC0415", "PLR2004", "S101", "S105", "S106", "S603", "S607", "S608", "SLF001", "T201", "TID253"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." [[rule]] @@ -1300,7 +1326,7 @@ pattern = ".github/**" codes = ["N999", "S310", "S603", "S607", "T201", "TID253"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." [[rule]] @@ -1319,7 +1345,7 @@ pattern = "scripts/**" codes = ["DTZ011", "S310", "S311", "S603", "S607", "T201"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." [[rule]] @@ -1345,7 +1371,7 @@ pattern = "policyengine_api/{data/v2/catalog/initialization,data/v2/household_mi codes = ["T201"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." @@ -1365,7 +1391,7 @@ pattern = "scripts/rules/tests/**" codes = ["PT"] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "lint" +job = "quality-guards" invocation = "ruff check ." [[rule]] id = "api.quality.mypy-v2" @@ -1394,7 +1420,7 @@ paths = [ ] [[rule.ci]] workflow = ".github/workflows/pr.yml" -job = "typecheck-v2" +job = "quality-guards" invocation = "mypy" [[rule]] diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index 75feef477..8985fb02f 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -20,7 +20,15 @@ class BaseUnavailableError(Exception): def default_base_ref() -> str | None: - """Return ``origin/`` on a pull-request run, otherwise None.""" + """Return the git revision to compare with, if there is one. + + That is RULES_BASE_REF as given, which make quality-check sets to the + commit the branch shares with its base branch; otherwise + ``origin/`` on a pull-request run; otherwise None. + """ + explicit = os.environ.get("RULES_BASE_REF") + if explicit: + return explicit base = os.environ.get("GITHUB_BASE_REF") return f"origin/{base}" if base else None diff --git a/scripts/rules/cli.py b/scripts/rules/cli.py index 6b370efc7..b441d68a9 100644 --- a/scripts/rules/cli.py +++ b/scripts/rules/cli.py @@ -74,7 +74,10 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("--root", type=Path, default=REPO_ROOT, help="repository root") parser.add_argument( "--base-ref", - help="git ref to compare baselines against (default: origin/$GITHUB_BASE_REF)", + help=( + "git ref to compare baselines against (default: $RULES_BASE_REF, " + "else origin/$GITHUB_BASE_REF)" + ), ) parser.add_argument( "--write-baselines", diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index 52df2b41d..04cf9c2a1 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -105,7 +105,7 @@ def run_guard( return result if ref is None: result.notes.append( - "no base ref (set --base-ref or GITHUB_BASE_REF); " + "no base ref (set --base-ref, RULES_BASE_REF or GITHUB_BASE_REF); " "baseline comparison skipped" ) else: diff --git a/scripts/rules/import_linter.py b/scripts/rules/import_linter.py index 43de89890..43ac321fc 100644 --- a/scripts/rules/import_linter.py +++ b/scripts/rules/import_linter.py @@ -21,6 +21,7 @@ import tomllib from typing import TYPE_CHECKING, Any +from scripts.rules.makefile import job_make_commands from scripts.rules.model import Kind, Rule, Violation from scripts.rules.workflows import parse_jobs @@ -233,7 +234,11 @@ def _ci_command_violations(rule: Rule, root: Path, config_name: str) -> list[Vio """Require each CI job to run every contract, with failures failing the job. The registry's invocation check only looks for a substring, which an - added ``--contract`` or ``|| true`` would still contain. + added ``--contract`` or ``|| true`` would still contain. The commands of + the make targets a job runs count as the job's, and a recipe's ``-`` + prefix, which ignores the command's failure, doesn't match. A Makefile + that can hide a failing command in another way gives its targets no + credit at all (``makefile.makefile_problems``). """ exact = re.compile( rf"^(?:uv run .*\s)?lint-imports --config {re.escape(config_name)}$" @@ -254,7 +259,10 @@ def _ci_command_violations(rule: Rule, root: Path, config_name: str) -> list[Vio f"run exactly 'lint-imports --config {config_name}', optionally " "after 'uv run ...'", ) - for command in (line.strip() for line in job.run_text.splitlines()) + for command in ( + *(line.strip() for line in job.run_text.splitlines()), + *job_make_commands(root, job.run_text), + ) if "lint-imports" in command and not exact.match(command) ) return violations diff --git a/scripts/rules/locked_installs.py b/scripts/rules/locked_installs.py index 9b1e11199..d01fe64be 100644 --- a/scripts/rules/locked_installs.py +++ b/scripts/rules/locked_installs.py @@ -22,6 +22,7 @@ from typing import TYPE_CHECKING from scripts.rules.load import load_registry +from scripts.rules.makefile import make_targets_run, root_makefile_problems from scripts.rules.model import RuleException, Violation from scripts.rules.workflows import parse_jobs @@ -63,7 +64,6 @@ r"|(?:^|\s)(?:tox|nox)(?:\s|$)" ) _SCRIPT_REFERENCE = re.compile(r"\.github/[\w./-]+\.sh") -_MAKE_TARGET_REFERENCE = re.compile(r"\bmake ([\w-]+)") _MAKE_TARGET_HEADER = re.compile(r"^([\w-]+)\s*:(?!=)") _RUN_KEY = re.compile(r"^(?P\s*)(?:-\s+)?run:(?P.*)$") _PULL_REQUEST_TRIGGERS = {"pull_request", "pull_request_target"} @@ -504,7 +504,9 @@ def executed_lines( ) -> list[str]: """Return the command's lines plus those of the scripts and Make targets it runs.""" visited = set() if visited is None else visited - recipes = makefile_recipes(root) + # A Makefile that can hide a failure, or that this reader can't follow, + # gives the targets a command runs no commands of theirs. + recipes = {} if root_makefile_problems(root) else makefile_recipes(root) executed = [] for command_line in command_text.splitlines(): if command_line.lstrip().startswith("#"): @@ -516,7 +518,7 @@ def executed_lines( visited.add(script_path) script_text = (root / script_path).read_text(encoding="utf-8") executed += executed_lines(root, script_text, visited) - for target in _MAKE_TARGET_REFERENCE.findall(command_line): + for target in make_targets_run(command_line): if f"make {target}" in visited or target not in recipes: continue visited.add(f"make {target}") @@ -606,7 +608,8 @@ def lock_violations(root: Path = REPO_ROOT) -> list[Violation]: & _PULL_REQUEST_TRIGGERS for commands in _workflow_job_commands(root, path).values() for command in commands - if _LOCK_CHECK.search(command) + # make ignores the failure of a recipe command with the - prefix. + if _LOCK_CHECK.search(command) and not command.lstrip().startswith("-") } violations += [ Violation( diff --git a/scripts/rules/makefile.py b/scripts/rules/makefile.py new file mode 100644 index 000000000..6e8d7aff8 --- /dev/null +++ b/scripts/rules/makefile.py @@ -0,0 +1,322 @@ +"""What make targets run: their recipes, prerequisites and recursive make calls. + +The rules guard follows a CI job's ``make `` into the Makefile, so a +rule's invocation counts when a target the job runs has it, and the Import +Linter guard checks the commands there too. This reads the subset of make +syntax the repository's Makefile uses: rules with prerequisites, tab-indented +recipes, target-specific variables, backslash continuations and comments. A +Makefile that can hide a failing command (``makefile_problems``) gets no +credit for its targets' commands. Standard library only. +""" + +from __future__ import annotations + +import re +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Iterable + from pathlib import Path + +# `targets: prerequisites`, but not a variable assignment such as `A := b`. +_RULE = re.compile(r"^([^\s:=#][^:=#]*?)\s*:(?![=:])(.*)$") +# A variable assignment, with its optional export or override. +_ASSIGNMENT = re.compile( + r"^\s*(?:(?:export|override|private)\s+)*([^\s:=#?+!]+)\s*(?::{1,3}=|[?+!]?=)" +) +# Variables whose value can stop a failing command from failing make: -i in +# MAKEFLAGS, or a SHELL or .SHELLFLAGS that doesn't run the command. +# GNUMAKEFLAGS works as MAKEFLAGS does, and MAKEFILES reads more Makefiles. +_FAILURE_VARIABLES = frozenset( + {"MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES", "SHELL", ".SHELLFLAGS"} +) +# Special targets that do: .IGNORE ignores errors, and .ONESHELL gives a +# recipe the exit status of its last command only. +_FAILURE_TARGETS = { + ".IGNORE": "makes make ignore failing commands", + ".ONESHELL": ( + "runs each recipe in one shell, whose exit status is its last command's" + ), +} +# Directives that change what make reads or runs in ways this reader doesn't +# follow: other files, conditionals and multi-line variables. +_UNFOLLOWED_DIRECTIVE = re.compile( + r"^\s*(-?include|sinclude|-?load|ifdef|ifndef|ifeq|ifneq|else|endif|define" + r"|endef|undefine)(?=\s|$)" +) +# A special target that hides failures, in a rule of one colon or two. +_FAILURE_TARGET_RULE = re.compile(r"^\s*(\.IGNORE|\.ONESHELL)\s*::?") +# A target or variable whose name make computes, such as $(M)FLAGS. +_COMPUTED_NAME = re.compile(r"^\s*(?:(?:export|override|private)\s+)*[^\s#=:]*\$[({]") +# $(eval ...), which makes make read text as more of the Makefile. +_EVAL = re.compile(r"\$[({]\s*eval\b") +# The Makefile names GNU make reads before Makefile. +_EARLIER_MAKEFILES = ("GNUmakefile", "makefile") +# make, or the $(MAKE) a recipe calls it as, as a word of a shell command. +_MAKE_CALL = re.compile(r"(?:^|(?<=[\s;&|(]))(?:make|\$\(MAKE\)|\$\{MAKE\})(?=\s|$)") +# Where one shell command ends and the next begins. +_COMMAND_END = re.compile(r"[;&|)\n]") +# A shell variable assignment, as a word before a command or after make. +_ASSIGNMENT_WORD = re.compile(r"^([A-Za-z_.][A-Za-z0-9_.]*)=") +# Long options after which make runs no recipe, or ignores its failures, or +# only prints something. GNU make also accepts an unambiguous start of one. +_NO_RUN_OPTIONS = ( + "--dry-run", + "--just-print", + "--recon", + "--question", + "--touch", + "--ignore-errors", + "--version", + "--help", + "--old-file", + "--assume-old", + "--eval", +) +# Short options that do the same: -n, -q, -t, -i, -v and -h; -o, which +# treats a target as up to date; and -E, which reads more of the Makefile. +_NO_RUN_LETTERS = frozenset("nqtivhoE") +# Long options that read another directory's or file's Makefile. +_OTHER_MAKEFILE_OPTIONS = ("--directory", "--file", "--makefile") +# Short options that do the same: -C and -f. +_OTHER_MAKEFILE_LETTERS = frozenset("Cf") +# Short options whose value follows, in the same word or the next one. +_VALUE_LETTERS = frozenset("IjlOW") +# make options whose value is the next word, which isn't a target. +_OPTIONS_WITH_VALUES = frozenset({"-I", "-W"}) +# Recipe prefixes that change nothing about whether a failure fails make: @ +# stops make printing the command, and + runs it under make -n too. The - +# prefix, which ignores the command's failure, is kept. +_HARMLESS_PREFIXES = "@+" + +Recipes = dict[str, tuple[list[str], list[str]]] + + +def _logical_lines(text: str) -> list[str]: + """Join each line that ends in a backslash with the next one.""" + lines: list[str] = [] + pending = "" + for line in text.splitlines(): + if line.endswith("\\"): + pending += line[:-1] + " " + continue + lines.append(pending + line) + pending = "" + if pending: + lines.append(pending) + return lines + + +def make_recipes(text: str) -> Recipes: + """Return each target's prerequisites and recipe commands, in order. + + Shell comments are left out, and the ``@`` and ``+`` prefixes removed. + Special targets such as ``.PHONY`` are left out. As make does, a later + recipe for a target replaces an earlier one, and prerequisites add up. + """ + recipes: Recipes = {} + targets: list[str] = [] + fresh: set[str] = set() + for line in _logical_lines(text): + if line.startswith("\t"): + command = line.strip().lstrip(_HARMLESS_PREFIXES).strip() + if command and not command.startswith("#"): + for target in targets: + if target in fresh: + recipes[target][1].clear() + fresh.discard(target) + recipes[target][1].append(command) + continue + if not line.strip() or line.lstrip().startswith("#"): + continue + match = _RULE.match(line) + if match is None: + targets = [] + continue + names = [name for name in match.group(1).split() if not name.startswith(".")] + rest = match.group(2).split("#", 1)[0] + if _ASSIGNMENT.match(rest): + # A target-specific variable: no prerequisites, and no recipe. + targets = [] + continue + prerequisites = [name for name in rest.split(";", 1)[0].split() if name != "|"] + for target in names: + recipes.setdefault(target, ([], []))[0].extend(prerequisites) + targets = names + fresh = set(names) + return recipes + + +def makefile_problems(text: str) -> list[str]: + """Return what in a Makefile can let a failing command pass, if anything. + + That is a setting that hides failures, or syntax this reader doesn't + follow, which could change what make runs without the reader seeing it. + """ + problems: list[str] = [] + for line in _logical_lines(text): + if _EVAL.search(line): + problems.append("uses $(eval ...), which this reader doesn't follow") + if line.startswith("\t") or line.lstrip().startswith("#"): + continue + if _COMPUTED_NAME.match(line): + problems.append( + "uses a computed target or variable name, which this reader " + "doesn't follow" + ) + continue + special = _FAILURE_TARGET_RULE.match(line) + if special: + name = special.group(1) + problems.append(f"sets {name}, which {_FAILURE_TARGETS[name]}") + directive = _UNFOLLOWED_DIRECTIVE.match(line) + if directive: + problems.append( + f"uses '{directive.group(1)}', which this reader doesn't follow" + ) + continue + match = _RULE.match(line) + rest = match.group(2).split("#", 1)[0] if match else line + assignment = _ASSIGNMENT.match(rest) + if assignment and assignment.group(1) in _FAILURE_VARIABLES: + problems.append( + f"sets {assignment.group(1)}, which can stop a failing command " + "from failing make" + ) + if not match: + continue + problems += [ + f"sets {name}, which {_FAILURE_TARGETS[name]}" + for name in match.group(1).split() + if name in _FAILURE_TARGETS and not special + ] + if not assignment and ";" in rest: + problems.append( + f"gives {match.group(1).strip()} a recipe after ';', which this " + "reader doesn't follow" + ) + return list(dict.fromkeys(problems)) + + +def _credits_no_target(word: str) -> bool: + """Whether a make option means the root Makefile's targets don't count. + + That is an option after which make runs no recipe, ignores its failures, + only prints something, or reads another directory's or file's Makefile. + """ + if word.startswith("--"): + name = word.split("=", 1)[0] + return name != "--" and any( + option.startswith(name) + for option in (*_NO_RUN_OPTIONS, *_OTHER_MAKEFILE_OPTIONS) + ) + if not word.startswith("-"): + return False + for letter in word[1:]: + if letter in _NO_RUN_LETTERS or letter in _OTHER_MAKEFILE_LETTERS: + return True + if letter in _VALUE_LETTERS: + return False + return False + + +def make_targets_run(command: str) -> list[str]: + """Return the targets each ``make`` or ``$(MAKE)`` in a shell command runs. + + A call credits no target when: + + - it runs no recipe, ignores failures or treats a target as up to date + (``-n``, ``-q``, ``-t``, ``-i``, ``-o``, their long forms); + - it only prints something (``-v``, ``-h``); + - it reads another Makefile or more of one (``-C``, ``-f``, ``-E``); + - a variable that can hide failures is set before it or as an argument; + - make isn't the command but an argument (``echo make quality``). + """ + targets = [] + for call in _MAKE_CALL.finditer(command): + before = _COMMAND_END.split(command[: call.start()])[-1].split() + words = _COMMAND_END.split(command[call.end() :], maxsplit=1)[0].split() + settings = [ + match.group(1) + for word in (*before, *words) + if (match := _ASSIGNMENT_WORD.match(word)) + ] + if ( + any(not _ASSIGNMENT_WORD.match(word) for word in before) + or any(name in _FAILURE_VARIABLES for name in settings) + or any(_credits_no_target(word) for word in words) + ): + continue + skip_next = False + for word in words: + if skip_next: + skip_next = False + elif word in _OPTIONS_WITH_VALUES: + skip_next = True + elif not word.startswith("-") and "=" not in word: + targets.append(word) + return targets + + +def make_commands(text: str, targets: Iterable[str]) -> list[str]: + """Return the commands ``make `` runs with Makefile ``text``, in order. + + Each target's prerequisites come first, then its recipe. After a recipe + command that runs make, the commands of the targets it runs follow. Each + target is followed once, so a loop between targets ends. + """ + recipes = make_recipes(text) + commands: list[str] = [] + followed: set[str] = set() + + def follow(target: str) -> None: + if target in followed or target not in recipes: + return + followed.add(target) + prerequisites, recipe = recipes[target] + for prerequisite in prerequisites: + follow(prerequisite) + for command in recipe: + commands.append(command) + for inner in make_targets_run(command): + follow(inner) + + for target in targets: + follow(target) + return commands + + +def job_make_targets(run_text: str) -> list[str]: + """Return the make targets a job's run text runs.""" + return [ + target for line in run_text.splitlines() for target in make_targets_run(line) + ] + + +def root_makefile_problems(root: Path) -> list[str]: + """Return ``makefile_problems`` for ``root``'s Makefile, if it has one.""" + makefile = root / "Makefile" + if not makefile.is_file(): + return [] + # Exact names: a file system that ignores case finds makefile as Makefile. + names = {path.name for path in root.iterdir()} + return [ + f"has {name} beside it, which GNU make reads instead" + for name in _EARLIER_MAKEFILES + if name in names + ] + makefile_problems(makefile.read_text(encoding="utf-8")) + + +def job_make_commands(root: Path, run_text: str) -> list[str]: + """Return the commands the make targets in a job's run text run. + + They come from ``root``'s Makefile. There are none without one, or when + make could run something else or hide a failing command + (``root_makefile_problems``). + """ + makefile = root / "Makefile" + if not makefile.is_file() or root_makefile_problems(root): + return [] + return make_commands( + makefile.read_text(encoding="utf-8"), job_make_targets(run_text) + ) diff --git a/scripts/rules/resolve.py b/scripts/rules/resolve.py index 61392f9dc..5c98b1e74 100644 --- a/scripts/rules/resolve.py +++ b/scripts/rules/resolve.py @@ -20,6 +20,11 @@ out_of_scope_files, ) from scripts.rules.import_linter import resolve_import_linter +from scripts.rules.makefile import ( + job_make_commands, + job_make_targets, + root_makefile_problems, +) from scripts.rules.model import CiRef, Kind, Rule, Violation from scripts.rules.workflows import JobBlock, parse_jobs @@ -412,10 +417,25 @@ def v(message: str, fix: str) -> Violation: v(f"no job '{ref.job}' in {ref.workflow}", f"use one of {sorted(jobs)}") ) return None, violations - if not job.invokes(ref.invocation): + # A make target the job runs counts as the job running its commands, but + # not a command with the - prefix, whose failure make ignores. + if not job.invokes(ref.invocation) and not any( + ref.invocation in command and not command.startswith("-") + for command in job_make_commands(root, job.run_text) + ): + problems = ( + root_makefile_problems(root) if job_make_targets(job.run_text) else [] + ) + reason = ( + f" (the Makefile {'; '.join(problems)}, so the commands of the make " + "targets it runs don't count)" + if problems + else "" + ) violations.append( v( - f"job '{ref.job}' in {ref.workflow} does not run '{ref.invocation}'", + f"job '{ref.job}' in {ref.workflow} does not run " + f"'{ref.invocation}'{reason}", f"correct {field}.invocation, or make the job run it", ) ) diff --git a/scripts/rules/tests/test_baseline_entries.py b/scripts/rules/tests/test_baseline_entries.py index 967b0f289..271d38e4f 100644 --- a/scripts/rules/tests/test_baseline_entries.py +++ b/scripts/rules/tests/test_baseline_entries.py @@ -2,9 +2,12 @@ from __future__ import annotations +import os import subprocess import unittest +from unittest import mock +from scripts.rules.baseline import default_base_ref from scripts.rules.tests.support import Repo from scripts.rules.tests.test_guard import RATCHETED_RULE, with_rule @@ -61,5 +64,24 @@ def test_a_removed_entry_passes(self) -> None: self.assertEqual(repo.messages(base_ref="HEAD"), []) +class TestDefaultBaseRef(unittest.TestCase): + """The ref baselines and rule statuses are compared with, by default.""" + + def test_rules_base_ref_is_used_as_given(self) -> None: + # make quality-check passes the merge base, a commit, this way. + environment = {"RULES_BASE_REF": "4f2960b6", "GITHUB_BASE_REF": "master"} + with mock.patch.dict(os.environ, environment): + self.assertEqual(default_base_ref(), "4f2960b6") + + def test_the_pull_request_s_base_branch_otherwise(self) -> None: + environment = {"RULES_BASE_REF": "", "GITHUB_BASE_REF": "master"} + with mock.patch.dict(os.environ, environment): + self.assertEqual(default_base_ref(), "origin/master") + + def test_none_without_either(self) -> None: + with mock.patch.dict(os.environ, {"RULES_BASE_REF": "", "GITHUB_BASE_REF": ""}): + self.assertIsNone(default_base_ref()) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/rules/tests/test_declared_dependencies.py b/scripts/rules/tests/test_declared_dependencies.py index 2bbb37027..117d5a284 100644 --- a/scripts/rules/tests/test_declared_dependencies.py +++ b/scripts/rules/tests/test_declared_dependencies.py @@ -26,6 +26,7 @@ unparsed_file_violations, unscanned_file_violations, ) +from scripts.rules.makefile import job_make_commands from scripts.rules.model import RuleException from scripts.rules.resolve import workflow_jobs @@ -629,7 +630,13 @@ def test_every_job_running_the_guards_has_uv_on_python_3_12(self) -> None: workflow_text = workflow.read_text() header = workflow_text.split("\njobs:", 1)[0] for job in workflow_jobs(workflow)[0].values(): - if "run_quality_guards.py" not in job.run_text: + # A job may run the guards through a make target, such as + # pr.yml's make quality. + commands = [ + job.run_text, + *job_make_commands(REPO_ROOT, job.run_text), + ] + if not any("run_quality_guards.py" in command for command in commands): continue guard_jobs.append(f"{workflow.name}:{job.id}") with self.subTest(job=f"{workflow.name}:{job.id}"): @@ -643,8 +650,11 @@ def test_every_job_running_the_guards_has_uv_on_python_3_12(self) -> None: 'UV_PYTHON: "3.12"', _job_text(workflow_text, job.id) + header ) self.assertTrue( - "uv sync --frozen --extra dev" in job.run_text - or "uv run --frozen --extra dev" in job.run_text + any( + "uv sync --frozen --extra dev" in command + or "uv run --frozen --extra dev" in command + for command in commands + ) ) self.assertEqual( guard_jobs, ["pr.yml:quality-guards", "push.yml:quality-guards"] diff --git a/scripts/rules/tests/test_import_linter.py b/scripts/rules/tests/test_import_linter.py index 25686554c..d00c26baf 100644 --- a/scripts/rules/tests/test_import_linter.py +++ b/scripts/rules/tests/test_import_linter.py @@ -331,6 +331,39 @@ def test_ci_must_run_every_contract_from_the_named_config(self) -> None: ) self.assertEqual(messages, expected) + def test_a_make_target_the_job_runs_must_run_every_contract_too(self) -> None: + ci = (CiRef(".github/workflows/pr.yml", "quality-guards", "lint-imports"),) + self.repo.write( + ".github/workflows/pr.yml", + "jobs:\n quality-guards:\n steps:\n - run: make quality\n", + ) + for command, accepted in ( + ("uv run --frozen --extra dev lint-imports --config pyproject.toml", True), + ("lint-imports --config pyproject.toml || true", False), + # make ignores the failure of a command with the - prefix. + ("-lint-imports --config pyproject.toml", False), + ): + with self.subTest(command=command): + self.repo.write( + "Makefile", + "quality:\n\t$(MAKE) quality-check\n\n" + f"quality-check:\n\t{command}\n", + ) + rule = dataclasses.replace(import_linter_rule("pkg-layers"), ci=ci) + messages = self.messages(rule) + expected = ( + [] + if accepted + else [ + "[ex.r] enforcer: job 'quality-guards' in " + ".github/workflows/pr.yml runs Import Linter as " + f"'{command}', which may skip contracts or ignore failures. " + "Fix: run exactly 'lint-imports --config pyproject.toml', " + "optionally after 'uv run ...'." + ] + ) + self.assertEqual(messages, expected) + def test_a_config_that_is_not_toml_is_reported(self) -> None: self.repo.write("setup.cfg", "[importlinter]\nroot_package = pkg\n") diff --git a/scripts/rules/tests/test_locked_installs.py b/scripts/rules/tests/test_locked_installs.py index 17788d329..733724cb3 100644 --- a/scripts/rules/tests/test_locked_installs.py +++ b/scripts/rules/tests/test_locked_installs.py @@ -3,6 +3,7 @@ from __future__ import annotations import subprocess +import tempfile import unittest from datetime import date from pathlib import Path @@ -105,6 +106,53 @@ def test_rule_proof_lock_committed(self) -> None: ) self.assertEqual(lock_violations(FIXTURES / "lock_good"), []) + def test_a_lock_check_in_a_recursive_make_target_counts(self) -> None: + # The job runs make quality, which runs make quality-check as a + # recipe calls make: $(MAKE). + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "pyproject.toml").write_text('[project]\nname = "example"\n') + (root / "uv.lock").write_text("version = 1\n") + (root / "Makefile").write_text( + "quality:\n\t$(MAKE) quality-check\n\n" + "quality-check:\n\tuv lock --check\n" + ) + workflows = root / ".github" / "workflows" + workflows.mkdir(parents=True) + (workflows / "pr.yml").write_text( + "on: pull_request\njobs:\n quality:\n steps:\n" + " - run: make quality\n" + ) + + self.assertEqual(lock_violations(root), []) + + def test_a_lock_check_whose_failure_make_ignores_does_not_count(self) -> None: + for makefile in ( + ".IGNORE:\nquality:\n\tuv lock --check\n", + "quality:\n\t-uv lock --check\n", + "quality:\n\t$(MAKE) -n quality-check\n\n" + "quality-check:\n\tuv lock --check\n", + ): + with ( + self.subTest(makefile=makefile), + tempfile.TemporaryDirectory() as directory, + ): + root = Path(directory) + (root / "pyproject.toml").write_text('[project]\nname = "example"\n') + (root / "uv.lock").write_text("version = 1\n") + (root / "Makefile").write_text(makefile) + workflows = root / ".github" / "workflows" + workflows.mkdir(parents=True) + (workflows / "pr.yml").write_text( + "on: pull_request\njobs:\n quality:\n steps:\n" + " - run: make quality\n" + ) + + self.assertEqual( + [violation.message for violation in lock_violations(root)], + ["no pull request job runs uv lock --check for ."], + ) + class TestScheduledRelockProof(unittest.TestCase): """Proof: a scheduled job relocks every uv.lock with upgrades and opens a PR.""" diff --git a/scripts/rules/tests/test_makefile.py b/scripts/rules/tests/test_makefile.py new file mode 100644 index 000000000..49e48a129 --- /dev/null +++ b/scripts/rules/tests/test_makefile.py @@ -0,0 +1,282 @@ +"""Tests for scripts/rules/makefile.py and for CI jobs that run make targets.""" + +from __future__ import annotations + +import tempfile +import unittest +from pathlib import Path + +from scripts.rules.makefile import ( + make_commands, + make_recipes, + make_targets_run, + makefile_problems, + root_makefile_problems, +) +from scripts.rules.tests.support import Repo + +MAKEFILE = ( + ".PHONY: test quality fix quality-check loop\n" + "export UV_PYTHON ?= 3.12\n" + "\n" + "test:\n" + "\tpython -m pytest tests/unit --disable-pytest-warnings\n" + "\n" + "# The checks, after the fixes.\n" + "quality: fix\n" + "\t@echo fixed\n" + "\t$(MAKE) quality-check\n" + "\n" + "fix:\n" + "\truff format .\n" + "\n" + "quality-check:\n" + "\t# mypy runs in another step\n" + "\t-ruff check .\n" + "\tmake loop\n" + "\n" + "loop:\n" + "\t$(MAKE) quality-check\n" +) +# MAKEFILE, with quality-check also type-checking. +MAKEFILE_WITH_MYPY = MAKEFILE.replace("\tmake loop\n", "\tuv run mypy\n\tmake loop\n") + + +class MakeRecipesTest(unittest.TestCase): + def test_reads_each_target_s_recipe_commands(self) -> None: + recipes = make_recipes(MAKEFILE) + + self.assertEqual(recipes["fix"], ([], ["ruff format ."])) + # Shell comments are left out, and the @ prefix is removed. The - + # prefix, which makes make ignore the command's failure, is kept. + self.assertEqual(recipes["quality-check"], ([], ["-ruff check .", "make loop"])) + self.assertEqual( + recipes["quality"], (["fix"], ["echo fixed", "$(MAKE) quality-check"]) + ) + self.assertNotIn(".PHONY", recipes) + self.assertNotIn("export UV_PYTHON ?= 3.12", recipes) + + def test_a_later_recipe_replaces_an_earlier_one(self) -> None: + # make runs only the last recipe of a target defined twice, and adds + # up its prerequisites. + recipes = make_recipes("check: a\n\tmypy\n\ncheck: b\n\truff check .\n") + + self.assertEqual(recipes["check"], (["a", "b"], ["ruff check ."])) + + def test_a_target_specific_variable_is_not_a_prerequisite(self) -> None: + recipes = make_recipes("check: export BASE ?= master\ncheck:\n\tmypy\n") + + self.assertEqual(recipes["check"], ([], ["mypy"])) + + +class MakefileProblemsTest(unittest.TestCase): + def test_reports_what_can_hide_a_failing_command(self) -> None: + for setting, name in ( + (".IGNORE:\n", ".IGNORE"), + (".IGNORE: check\n", ".IGNORE"), + (".ONESHELL:\n", ".ONESHELL"), + ("MAKEFLAGS += -i\n", "MAKEFLAGS"), + ("export SHELL := /usr/bin/true\n", "SHELL"), + ("check: SHELL := /usr/bin/true\n", "SHELL"), + ("override .SHELLFLAGS = -c\n", ".SHELLFLAGS"), + (".IGNORE::\n", ".IGNORE"), + ("GNUMAKEFLAGS += -i\n", "GNUMAKEFLAGS"), + ("MAKEFILES = ci.mk\n", "MAKEFILES"), + ("M = MAKE\n$(M)FLAGS += -i\n", "computed"), + ("I = .IGNORE\n$(I):\n", "computed"), + ): + with self.subTest(setting=setting): + problems = makefile_problems(setting + "check:\n\tmypy\n") + + self.assertTrue(any(name in problem for problem in problems), problems) + + def test_reports_syntax_the_reader_does_not_follow(self) -> None: + # Each can change what make runs without the reader seeing it, so the + # reader rejects it rather than guess. + for setting, name in ( + ("include extra.mk\n", "include"), + ("-include extra.mk\n", "include"), + ("ifdef GITHUB_ACTIONS\ncheck:\n\t@true\nendif\n", "ifdef"), + ("ifeq ($(CI),true)\nendif\n", "ifeq"), + ("define SKIP\n@true\nendef\n", "define"), + ("$(eval MAKEFLAGS += -i)\n", "$(eval"), + ('check: ; @echo "nothing runs"\n', "after ';'"), + ): + with self.subTest(setting=setting): + problems = makefile_problems("check:\n\tmypy\n" + setting) + + self.assertTrue(any(name in problem for problem in problems), problems) + + def test_reports_another_makefile_make_reads_first(self) -> None: + # GNU make reads GNUmakefile, then makefile, before Makefile. + for name in ("GNUmakefile", "makefile"): + with self.subTest(name=name), tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "Makefile").write_text(MAKEFILE) + (root / name).write_text("quality:\n\t@true\n") + listed = {path.name for path in root.iterdir()} + if listed != {"Makefile", name}: + # A file system that ignores case keeps one of the two. + continue + + problems = root_makefile_problems(root) + + self.assertTrue(any(name in problem for problem in problems), problems) + + def test_ordinary_rules_and_variables_are_fine(self) -> None: + self.assertEqual(makefile_problems(MAKEFILE), []) + + +class MakeCommandsTest(unittest.TestCase): + def test_follows_prerequisites_and_the_make_commands_in_recipes(self) -> None: + self.assertEqual( + make_commands(MAKEFILE, ["quality"]), + [ + "ruff format .", + "echo fixed", + "$(MAKE) quality-check", + "-ruff check .", + "make loop", + # loop runs quality-check again, which is already listed. + "$(MAKE) quality-check", + ], + ) + + def test_an_unknown_target_runs_nothing(self) -> None: + self.assertEqual(make_commands(MAKEFILE, ["missing"]), []) + + +class MakeTargetsRunTest(unittest.TestCase): + def test_finds_the_targets_a_command_runs(self) -> None: + self.assertEqual( + make_targets_run("make quality && make -j2 test VERBOSE=1"), + ["quality", "test"], + ) + self.assertEqual(make_targets_run("$(MAKE) quality-check"), ["quality-check"]) + # Another directory's or file's targets aren't the root Makefile's. + for command in ( + "make -C docs html", + "make --directory=docs html", + "make -f other.mk quality", + "make --file=other.mk quality", + ): + with self.subTest(command=command): + self.assertEqual(make_targets_run(command), []) + + def test_a_call_that_runs_no_command_or_ignores_failures_runs_nothing( + self, + ) -> None: + # -n and its long forms only print the commands, -q and -t run none, + # and -i ignores their failures. + for command in ( + "make -n quality", + "make --dry-run quality", + "make --just-print quality", + "make -q quality", + "make -t quality", + "make -i quality", + "make --ignore-errors quality", + "make -ki quality", + # GNU make accepts an unambiguous start of a long option. + "make --dry quality", + "make --just quality", + "make --ign quality", + # These print something and exit. + "make -v quality", + "make --version quality", + "make -h quality", + "make --help quality", + # MAKEFLAGS in the command's environment works as the options do. + "MAKEFLAGS=-i make quality", + "env MAKEFLAGS=i make quality", + "GNUMAKEFLAGS=-i make quality", + "MAKEFILES=ci.mk make quality", + # -o treats a target as up to date; --eval and -E read more of + # the Makefile; settings on the command line override it. + "make -o quality quality", + "make --old-file=quality quality", + "make -oquality quality", + "make --eval=.IGNORE: quality", + "make -E .IGNORE: quality", + "make quality SHELL=/bin/true", + "make quality MAKEFLAGS=-i", + # make here is an argument, not the command. + "echo make quality", + ): + with self.subTest(command=command): + self.assertEqual(make_targets_run(command), []) + # An option's own value isn't read as more options. + self.assertEqual(make_targets_run("make -Otarget -j4 quality"), ["quality"]) + + def test_other_words_that_contain_make_are_not_make(self) -> None: + self.assertEqual(make_targets_run("echo make-believe; cmake build"), []) + + +class MakeTargetInvocationTest(unittest.TestCase): + """A job runs a command if a make target it runs does (rule.ci invocations).""" + + def test_a_job_runs_the_commands_of_the_make_targets_it_runs(self) -> None: + with Repo() as repo: + repo.edit( + ".github/workflows/pr.yml", "- run: uv run mypy", "- run: make quality" + ) + repo.write("Makefile", MAKEFILE_WITH_MYPY) + messages = repo.messages() + + self.assertFalse( + any("does not run 'mypy'" in message for message in messages), + messages, + ) + + def test_no_target_counts_when_the_makefile_can_hide_a_failure(self) -> None: + with Repo() as repo: + repo.edit( + ".github/workflows/pr.yml", "- run: uv run mypy", "- run: make quality" + ) + repo.write("Makefile", ".IGNORE: quality-check\n" + MAKEFILE_WITH_MYPY) + messages = repo.messages() + + self.assertTrue( + any( + "does not run 'mypy'" in message and ".IGNORE" in message + for message in messages + ), + messages, + ) + + def test_a_command_whose_failure_make_ignores_does_not_count(self) -> None: + with Repo() as repo: + repo.edit( + ".github/workflows/pr.yml", "- run: uv run mypy", "- run: make quality" + ) + repo.write( + "Makefile", MAKEFILE.replace("\tmake loop\n", "\t-uv run mypy\n") + ) + messages = repo.messages() + + self.assertTrue( + any("does not run 'mypy'" in message for message in messages), + messages, + ) + + def test_a_make_target_without_the_command_does_not_count(self) -> None: + # MAKEFILE mentions mypy only in a comment. + with Repo() as repo: + repo.edit( + ".github/workflows/pr.yml", "- run: uv run mypy", "- run: make quality" + ) + repo.write("Makefile", MAKEFILE) + messages = repo.messages() + + self.assertTrue( + any( + "job 'typecheck' in .github/workflows/pr.yml does not run 'mypy'" + in message + for message in messages + ), + messages, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_make_quality.py b/tests/unit/test_make_quality.py new file mode 100644 index 000000000..80e13ff6f --- /dev/null +++ b/tests/unit/test_make_quality.py @@ -0,0 +1,484 @@ +"""One command runs the quality checks, locally and in CI (api.quality.make-quality). + +make quality applies ruff's fixes, then runs make quality-check, which runs +every mechanical check without changing files. pr.yml's quality-guards job runs +make quality and fails if the fixes changed a file, so CI gets the result an +agent gets locally. No other pr.yml job runs one of those checks itself, so +the two can't drift apart. diff-cover isn't part of it: it needs every test +shard's coverage report, so pr.yml runs it in a job of its own. +""" + +from __future__ import annotations + +import os +import re +import subprocess +from pathlib import Path + +import pytest +import yaml + +from scripts.rules.makefile import ( + job_make_targets, + make_commands, + make_recipes, + makefile_problems, +) + +REPO = Path(__file__).resolve().parents[2] +FIXTURES = REPO / "rules" / "fixtures" / "api.quality.make-quality" +PR_WORKFLOW = REPO / ".github" / "workflows" / "pr.yml" +LOCKED = "uv run --frozen --extra dev" +FIXES = [f"{LOCKED} ruff check --fix .", f"{LOCKED} ruff format ."] +CHECKS = ( + "uv lock --check", + f"{LOCKED} ruff check .", + f"{LOCKED} ruff format --check .", + f"{LOCKED} lint-imports --config pyproject.toml", + f"{LOCKED} mypy", + # Without site-packages (-S), so the guards keep to the standard library, + # as push.yml runs them; and comparing baselines and rule statuses with + # the commit the branch shares with its base branch (master locally). + "RULES_BASE_REF=$$(git merge-base HEAD origin/$${GITHUB_BASE_REF:-master}) " + f"{LOCKED} python -S scripts/run_quality_guards.py", +) +# The tools that pr.yml may run only through make quality. +CHECK_TOOLS = re.compile(r"\b(ruff|mypy|lint-imports|run_quality_guards\.py)\b") +# The quality job's steps, which may not be skipped or allowed to fail. +REQUIRED_STEPS = ("make quality", "git diff --exit-code HEAD") +JOB = "quality-guards" +# A command in make quality-check that changes files instead of checking them. +CHANGES_FILES = re.compile(r"--fix\b|\b(?:ruff format|uv lock)\b(?!.*--check)") +# Environment variables whose value can make make ignore failing commands. +# GNUMAKEFLAGS works as MAKEFLAGS does, and MAKEFILES reads more Makefiles. +MAKE_FLAG_VARIABLES = ("MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES") +# Step settings that can skip a step, ignore its failure, or run it elsewhere. +STEP_SETTINGS = ("if", "continue-on-error", "shell", "working-directory") +# Variables of make's own that a test's make runs mustn't inherit. +MAKE_ENVIRONMENT = frozenset( + {"MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES", "MAKELEVEL"} +) + + +def quality_problems(makefile: str, workflow: str) -> list[str]: + """Return how a Makefile and pr.yml break api.quality.make-quality.""" + recipes = make_recipes(makefile) + problems = [f"the Makefile {problem}" for problem in makefile_problems(makefile)] + if recipes.get("quality") != ([], [*FIXES, "$(MAKE) quality-check"]): + problems.append( + "make quality must run ruff's two fixes, then $(MAKE) quality-check" + ) + check_commands = recipes.get("quality-check", ([], []))[1] + problems += [ + f"make quality-check doesn't run '{check}'" + for check in CHECKS + if check not in check_commands + ] + problems += [ + f"make quality-check changes files: '{command}'" + for command in check_commands + if CHANGES_FILES.search(command) + ] + settings = yaml.safe_load(workflow) + jobs = settings["jobs"] + job = jobs.get(JOB, {}) + steps = job.get("steps", []) + runs = [step.get("run", "") for step in steps] + first = runs.index(REQUIRED_STEPS[0]) if REQUIRED_STEPS[0] in runs else None + if first is None or runs[first + 1 : first + 2] != [REQUIRED_STEPS[1]]: + problems.append( + f"pr.yml's {JOB} job must run make quality, then " + f"{REQUIRED_STEPS[1]} right after it" + ) + environments = [ + ("pr.yml", settings.get("env") or {}), + (f"pr.yml's {JOB} job", job.get("env") or {}), + *( + (f"pr.yml's '{step['run']}' step", step.get("env") or {}) + for step in steps + if step.get("run") in REQUIRED_STEPS + ), + ] + problems += [ + f"{where} sets {name}, which can make make ignore failing commands" + for where, environment in environments + for name in MAKE_FLAG_VARIABLES + if name in environment + ] + if "continue-on-error" in job: + problems.append(f"pr.yml's {JOB} job ignores its failures (continue-on-error)") + # A default shell runs every step's commands through another program. + problems += [ + f"{where} sets defaults, which can run make quality in another shell" + for where, owner in (("pr.yml", settings), (f"pr.yml's {JOB} job", job)) + if "defaults" in owner + ] + problems += [ + f"pr.yml's {JOB} job may skip or ignore '{step['run']}'" + for step in steps + if step.get("run") in REQUIRED_STEPS + and any(key in step for key in STEP_SETTINGS) + ] + # A step that writes to GITHUB_ENV or GITHUB_PATH sets the environment of + # the steps after it, such as MAKEFLAGS. + problems += [ + f"pr.yml's {JOB} job writes to GITHUB_ENV or GITHUB_PATH before " + f"make quality: '{run}'" + for run in runs[: first if first is not None else len(runs)] + if "GITHUB_ENV" in run or "GITHUB_PATH" in run + ] + for name, other in jobs.items(): + run_text = "\n".join(step.get("run", "") for step in other.get("steps", [])) + # The quality job runs the checks through make quality; any other + # job that runs a make target runs that target's commands itself. + commands = [ + run_text, + *( + [] + if name == JOB + else make_commands(makefile, job_make_targets(run_text)) + ), + ] + tools = sorted( + { + match.group(1) + for command in commands + for match in CHECK_TOOLS.finditer(command) + } + ) + problems += [f"pr.yml's {name} job runs {tool} itself" for tool in tools] + if JOB not in jobs.get("ci-complete", {}).get("needs", []): + problems.append(f"CI complete doesn't need {JOB}") + return problems + + +# A stand-in for uv that records each call and fails the one named in +# UV_STUB_FAIL_ON, so make quality can be run for real without the checks. +STUB_UV = """#!/bin/sh +printf '%s\\n' "$*" >> "$UV_STUB_LOG" +if [ "$*" = "$UV_STUB_FAIL_ON" ]; then + exit 1 +fi +exit 0 +""" + + +def _uv_arguments(command: str) -> str: + """Return what a Makefile command passes to uv: ``run ...`` or ``lock ...``.""" + return command.split("uv ", 1)[1] + + +def make_quality_behavior_problems(makefile: str, directory: Path) -> list[str]: + """Run make quality with a stand-in uv; return how it hides a failing check. + + Whatever the Makefile does, make quality must run the fixes and every check + in order when they pass, and fail when any one of them fails. + """ + (directory / "Makefile").write_text(makefile) + stubs = directory / "stubs" + stubs.mkdir(exist_ok=True) + (stubs / "uv").write_text(STUB_UV) + (stubs / "uv").chmod(0o755) + log = directory / "uv.log" + expected = [_uv_arguments(command) for command in (*FIXES, *CHECKS)] + environment = { + name: value + for name, value in os.environ.items() + if name not in MAKE_ENVIRONMENT + } + environment["PATH"] = f"{stubs}{os.pathsep}{environment.get('PATH', '')}" + environment["UV_STUB_LOG"] = str(log) + + def run(fail_on: str) -> tuple[int, list[str]]: + log.write_text("") + result = subprocess.run( + ["make", "--no-print-directory", "quality"], + cwd=directory, + env={**environment, "UV_STUB_FAIL_ON": fail_on}, + capture_output=True, + text=True, + check=False, + ) + return result.returncode, log.read_text().splitlines() + + problems = [] + returncode, calls = run("") + if returncode != 0 or calls != expected: + problems.append( + f"make quality, with every check passing, exited {returncode} and " + f"ran {calls}, not {expected}" + ) + for arguments in expected: + returncode, _ = run(arguments) + if returncode == 0: + problems.append(f"make quality passes although 'uv {arguments}' fails") + return problems + + +def _fixture_problems(name: str) -> list[str]: + return quality_problems( + (FIXTURES / name / "Makefile").read_text(encoding="utf-8"), + (FIXTURES / name / "pr.yml").read_text(encoding="utf-8"), + ) + + +def test_rule_proof_make_quality(): + assert _fixture_problems("rejected") == [ + f"make quality-check doesn't run '{LOCKED} mypy'", + "pr.yml's typecheck job runs mypy itself", + ] + assert _fixture_problems("accepted") == [] + + +def test_make_quality_runs_every_check_and_ci_runs_it(): + assert ( + quality_problems( + (REPO / "Makefile").read_text(encoding="utf-8"), + PR_WORKFLOW.read_text(encoding="utf-8"), + ) + == [] + ) + + +ACCEPTED_MAKEFILE = (FIXTURES / "accepted" / "Makefile").read_text(encoding="utf-8") +ACCEPTED_WORKFLOW = (FIXTURES / "accepted" / "pr.yml").read_text(encoding="utf-8") +MAKE_QUALITY_STEP = " - run: make quality\n" +DIFF_STEP = " - run: git diff --exit-code HEAD\n" + + +@pytest.mark.parametrize( + ("makefile", "workflow", "problem"), + [ + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, + MAKE_QUALITY_STEP + " continue-on-error: true\n", + ), + "may skip or ignore 'make quality'", + id="step continue-on-error", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, MAKE_QUALITY_STEP + " if: ${{ false }}\n" + ), + "may skip or ignore 'make quality'", + id="step condition", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + " quality-guards:\n", + " quality-guards:\n continue-on-error: true\n", + ), + "ignores its failures", + id="job continue-on-error", + ), + pytest.param( + ".IGNORE: quality-check\n" + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW, + ".IGNORE", + id="Makefile ignores failures", + ), + pytest.param( + ACCEPTED_MAKEFILE + "\ntypecheck:\n\tuv run --frozen --extra dev mypy\n", + ACCEPTED_WORKFLOW + + " typecheck:\n steps:\n - run: make typecheck\n", + "pr.yml's typecheck job runs mypy itself", + id="another job runs a make target with a check", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW + " format:\n steps:\n - run: pipx run ruff\n", + "pr.yml's format job runs ruff itself", + id="ruff at the end of a command", + ), + pytest.param( + ACCEPTED_MAKEFILE, + "env:\n MAKEFLAGS: -i\n" + ACCEPTED_WORKFLOW, + "MAKEFLAGS", + id="MAKEFLAGS for the workflow", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + " quality-guards:\n", + " quality-guards:\n env:\n MAKEFLAGS: -i\n", + ), + "MAKEFLAGS", + id="MAKEFLAGS for the job", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, + MAKE_QUALITY_STEP + " env:\n MAKEFLAGS: -i\n", + ), + "MAKEFLAGS", + id="MAKEFLAGS for the step", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, MAKE_QUALITY_STEP + " shell: bash {0}\n" + ), + "may skip or ignore 'make quality'", + id="step shell", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + DIFF_STEP, " - run: git add -A\n" + DIFF_STEP + ), + "must run make quality, then git diff --exit-code HEAD", + id="a step between make quality and git diff", + ), + pytest.param( + ACCEPTED_MAKEFILE.replace( + "\tuv lock --check\n", "\tuv lock\n\tuv lock --check\n" + ), + ACCEPTED_WORKFLOW, + "changes files: 'uv lock'", + id="quality-check relocks", + ), + pytest.param( + ACCEPTED_MAKEFILE + "\nquality-check: ; @true\n", + ACCEPTED_WORKFLOW, + "after ';'", + id="a recipe after a semicolon replaces the checks", + ), + pytest.param( + ACCEPTED_MAKEFILE, + "defaults:\n run:\n shell: true {0}\n" + ACCEPTED_WORKFLOW, + "defaults", + id="a default shell for the workflow", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + " quality-guards:\n", + " quality-guards:\n" + " defaults:\n run:\n shell: true {0}\n", + ), + "defaults", + id="a default shell for the job", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, + ' - run: echo "MAKEFLAGS=-i" >> "$GITHUB_ENV"\n' + + MAKE_QUALITY_STEP, + ), + "GITHUB_ENV", + id="an earlier step sets the environment", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + " quality-guards:\n", + " quality-guards:\n env:\n GNUMAKEFLAGS: -i\n", + ), + "GNUMAKEFLAGS", + id="GNUMAKEFLAGS for the job", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + " quality-guards:\n", + " quality-guards:\n env:\n MAKEFILES: ci.mk\n", + ), + "MAKEFILES", + id="MAKEFILES for the job", + ), + pytest.param( + ACCEPTED_MAKEFILE, + ACCEPTED_WORKFLOW.replace( + MAKE_QUALITY_STEP, + MAKE_QUALITY_STEP + " working-directory: docs\n", + ), + "may skip or ignore 'make quality'", + id="another working directory", + ), + ], +) +def test_ways_to_skip_or_ignore_a_check_are_rejected(makefile, workflow, problem): + problems = quality_problems(makefile, workflow) + + assert any(problem in reported for reported in problems), problems + + +@pytest.mark.parametrize( + "setting", + [ + pytest.param(".IGNORE::\n", id="double-colon .IGNORE"), + pytest.param("GNUMAKEFLAGS += -i\n", id="GNUMAKEFLAGS"), + pytest.param("M = MAKE\n$(M)FLAGS += -i\n", id="computed MAKEFLAGS"), + pytest.param( + "DRY_RUN := $(findstring n,$(firstword -$(MAKEFLAGS)))\n" + "REAL_RUN_TARGET := $(if $(DRY_RUN),dry-run-only,quality-check)\n" + "$(REAL_RUN_TARGET):\n" + "\t@true\n", + id="a recipe only for real runs", + ), + ], +) +def test_settings_that_hide_a_failing_check_are_rejected(setting, tmp_path): + # Reading the Makefile reports what it recognizes; running make quality + # with a failing check catches the rest, on whichever make honors them + # (GNUMAKEFLAGS needs GNU make 4.0, which CI has). + makefile = ACCEPTED_MAKEFILE + setting + problems = quality_problems( + makefile, ACCEPTED_WORKFLOW + ) + make_quality_behavior_problems(makefile, tmp_path) + + assert problems + + +def test_make_quality_fails_whenever_a_check_fails(tmp_path): + makefile = (REPO / "Makefile").read_text(encoding="utf-8") + + assert make_quality_behavior_problems(makefile, tmp_path) == [] + + +def test_make_itself_runs_the_listed_commands(): + # The Makefile reader follows a subset of make. make -n prints what make + # would run, in CI's environment, so make itself must agree with it. + environment = { + name: value + for name, value in os.environ.items() + if name not in {"MAKEFLAGS", "MFLAGS", "MAKELEVEL"} + } + environment["GITHUB_ACTIONS"] = "true" + printed = subprocess.run( + ["make", "-n", "--no-print-directory", "quality"], + cwd=REPO, + env=environment, + capture_output=True, + text=True, + check=True, + ).stdout.splitlines() + + # make -n also prints the recursive call, as its own path to make. + commands = [line for line in printed if not line.endswith("make quality-check")] + assert commands == [command.replace("$$", "$") for command in (*FIXES, *CHECKS)] + + +def test_the_quality_job_can_run_every_check(): + job = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8"))["jobs"][JOB] + steps = job["steps"] + (checkout,) = [step for step in steps if "actions/checkout" in step.get("uses", "")] + (uv,) = [step for step in steps if "setup-uv" in step.get("uses", "")] + runs = [step.get("run", "") for step in steps] + + # The rules registry guard compares baselines with the base branch. + assert checkout["with"]["fetch-depth"] == 0 + # The uv version the Cloud Run image and the release job use, so + # `uv lock --check` reads uv.lock as they write it. + assert uv["with"]["version"] == "0.13.4" + assert job["env"]["UV_PYTHON"] == "3.12" + # The ci-scripts guard runs shellcheck, and fails in CI without it. + assert any("shellcheck-py" in run for run in runs) + # The registry tool's own tests still run there. + assert f"{LOCKED} python -m unittest discover -s scripts/rules/tests -t ." in runs