Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 14 additions & 48 deletions .github/workflows/pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
7 changes: 6 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ named `changelog.d/<name>.<type>.md`, where `<type>` 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
Expand Down Expand Up @@ -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

Expand Down
24 changes: 23 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions changelog.d/4202.added.md
Original file line number Diff line number Diff line change
@@ -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.
54 changes: 48 additions & 6 deletions docs/engineering/skills/github-prs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <changed Python files>
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/<base branch>`: 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.
4 changes: 2 additions & 2 deletions docs/engineering/skills/rules-registry.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<default-branch> # everything CI checks
python -m scripts.rules guard --base-ref "$(git merge-base HEAD origin/<default-branch>)" # everything CI checks
python -m unittest discover -s scripts/rules/tests -t . # the tool's own tests
```

Expand Down Expand Up @@ -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 <target>`, 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)). |
Expand Down
10 changes: 3 additions & 7 deletions docs/engineering/skills/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <changed Python files>
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,
Expand Down
Loading
Loading