diff --git a/.github/bump_version.py b/.github/bump_version.py index e29c6bf8c..dee431c03 100644 --- a/.github/bump_version.py +++ b/.github/bump_version.py @@ -3,6 +3,12 @@ import re import sys from pathlib import Path +from typing import TextIO + + +def _report(message: str, *, file: TextIO | None = None) -> None: + """Print one line of this command's output, to stdout unless ``file`` is given.""" + print(message, file=file) def get_current_version(pyproject_path: Path) -> str: @@ -10,7 +16,7 @@ def get_current_version(pyproject_path: Path) -> str: text = pyproject_path.read_text() match = re.search(r'^version\s*=\s*"(\d+\.\d+\.\d+)"', text, re.MULTILINE) if not match: - print( + _report( "Could not find version in pyproject.toml", file=sys.stderr, ) @@ -28,7 +34,7 @@ def infer_bump(changelog_dir: Path) -> str: f for f in changelog_dir.iterdir() if f.is_file() and f.name != ".gitkeep" ] if not fragments: - print("No changelog fragments found", file=sys.stderr) + _report("No changelog fragments found", file=sys.stderr) sys.exit(1) categories = {f.suffix.lstrip(".") for f in fragments} @@ -63,7 +69,7 @@ def update_file(path: Path, old_version: str, new_version: str) -> None: ) if updated != text: path.write_text(updated) - print(f" Updated {path}") + _report(f" Updated {path}") def main() -> None: @@ -76,7 +82,7 @@ def main() -> None: bump = infer_bump(changelog_dir) new = bump_version(current, bump) - print(f"Version: {current} -> {new} ({bump})") + _report(f"Version: {current} -> {new} ({bump})") update_file(pyproject, current, new) diff --git a/.github/scripts/check_updates.py b/.github/scripts/check_updates.py old mode 100644 new mode 100755 diff --git a/AGENTS.md b/AGENTS.md index 891a47e70..f68e940c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -70,63 +70,33 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### API v2 layer boundaries +- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.session-modules-no-queries`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. (doc-only, not checked: follow it yourself) -- `api.v2.connectors-mutations-no-reads`: v2 mutation connectors (creates, updates, deletes) never issue select or get reads. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) - `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. (doc-only, not checked: follow it yourself) - `api.v2.connectors-no-cross-calls`: v2 connector modules never call one another to sequence a workflow; the service does the sequencing. (doc-only, not checked: follow it yourself) -- `api.v2.connectors-reads-no-mutations`: v2 read connectors never insert, add or delete rows. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) -- `api.v2.crud-checks-cover-every-resource`: The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) - `api.v2.routes-no-sessions-or-sql`: v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. (doc-only, not checked: follow it yourself) - `api.v2.services-no-sql`: v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. (doc-only, not checked: follow it yourself) -- `api.v2.session-modules-no-queries`: v2 database_session modules own session lifetime only and never build select, insert, update, delete or exec calls. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) -- `api.v2.user-services-no-sql`: v2 user services (identity, accounts, legacy references and user associations) never build or run SQL themselves or import SQLAlchemy; their connectors do. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) -- `api.v2.validators-transformations-no-db`: v2 validators and transformations never import SQLAlchemy or SQLModel query or session APIs. (blocking: pytest `tests/unit/v2/test_data_crud_boundaries.py`) ### Runtime persistence and route boundaries -- `api.data.runtime-sql-confined`: Raw SQL is confined to the data-access layer; runtime modules and report orchestration use sessions and mapped models. (blocking: pytest `tests/unit/data/test_runtime_sql_boundaries.py`) -- `api.legacy.removed-layers-stay-removed`: Removed legacy persistence layers, the temporary DAO module and the legacy endpoints package stay removed. (blocking: pytest `tests/unit/data/test_runtime_sql_boundaries.py`, `tests/unit/routes/test_flask_route_boundaries.py`, `tests/unit/services/test_orm_service_boundaries.py`) -- `api.routes.flask-blueprints-own-urls`: Legacy Flask URLs are owned by blueprints, and the Flask app assembles blueprints without legacy endpoint wiring. (blocking: pytest `tests/unit/routes/test_flask_route_boundaries.py`) -- `api.routes.no-domain-logic-in-http`: Flask routes delegate domain processing (economy comparison, calculation) to services instead of doing it in the HTTP layer. (blocking: pytest `tests/unit/routes/test_flask_route_boundaries.py`) -- `api.routes.no-inline-json-errors`: Flask routes never build JSON error responses inline; they use the shared error handling. (blocking: pytest `tests/unit/routes/test_flask_route_boundaries.py`) -- `api.services.no-direct-queries`: Services never issue database queries or open database connections directly; they delegate persistence to the data-access layer. (blocking: pytest `tests/unit/services/test_local_data_service_boundaries.py`, `tests/unit/services/test_orchestration_service_database_boundaries.py`, `tests/unit/services/test_orm_service_boundaries.py`, `tests/unit/services/test_run_and_spec_service_database_boundaries.py`) -- `api.services.session-ownership`: Migrated services use sessions and mapped models; their public methods never accept persistence objects, and routes and the economy service never manage sessions. (blocking: pytest `tests/unit/services/test_local_data_service_boundaries.py`, `tests/unit/services/test_orm_service_boundaries.py`) +- Checked: `api.data.runtime-sql-confined`, `api.legacy.removed-layers-stay-removed`, `api.routes.flask-blueprints-own-urls`, `api.routes.no-domain-logic-in-http`, `api.routes.no-inline-json-errors`, `api.services.no-direct-queries`, `api.services.session-ownership` ### Import and startup side effects -- `api.catalog.initializer-not-at-startup`: Application startup modules never import the metadata catalog initializer. (blocking: pytest `tests/unit/v2/test_catalog_initialization.py`) -- `api.startup.fail-closed-configuration`: Default Cloud SQL startup needs no Supabase configuration, and a selected v2 runtime without its database URL fails closed. (blocking: pytest `tests/unit/v2/test_import_side_effects.py`) -- `api.startup.no-import-side-effects`: Importing v2 modules, starting up and serving a request open no network connections and create no files or runtime SQLite databases. (blocking: pytest `tests/unit/v2/test_import_side_effects.py`) -- `api.startup.no-runtime-sqlite-modules`: Runtime SQLite modules are absent from the production package. (blocking: pytest `tests/unit/v2/test_import_side_effects.py`) +- Checked: `api.catalog.initializer-not-at-startup`, `api.startup.fail-closed-configuration`, `api.startup.no-import-side-effects`, `api.startup.no-runtime-sqlite-modules` ### Data source isolation -- `api.catalog.extractor-import-isolation`: The catalog extractor never imports PolicyEngine core, country packages or v1 metadata directly. (blocking: pytest `tests/unit/v2/test_catalog_extraction.py`) -- `api.metadata.read-modules-isolated`: v2 metadata read modules never import PolicyEngine packages or v1 metadata sources. (blocking: pytest `tests/unit/v2/test_metadata_service.py`) -- `api.reports.v1-persistence-only`: The existing report route and service import only v1 persistence. (blocking: pytest `tests/unit/v2/test_report_v1_runtime_selection.py`) -- `api.stage12.comparison-tables-isolated`: Serving routes and services never read the Stage 12 comparison tables, and production tables never depend on them. (blocking: pytest `tests/unit/v2/test_stage12_comparison_isolation.py`) -- `api.stage12.retirement-owned`: Retiring the Stage 12 comparison schema stays owned by a named follow-up change in the Stage 12 worker contract. (blocking: pytest `tests/unit/v2/test_stage12_comparison_isolation.py`) -- `api.v2.single-orm-layer`: v2 models never create a parallel SQLAlchemy ORM layer alongside SQLModel. (blocking: pytest `tests/unit/v2/test_model_persistence.py`) +- Checked: `api.catalog.extractor-import-isolation`, `api.metadata.read-modules-isolated`, `api.reports.v1-persistence-only`, `api.stage12.comparison-tables-isolated`, `api.stage12.retirement-owned`, `api.v2.single-orm-layer` ### Migration and schema +- Checked: `api.migration.alembic-v1-qualified`, `api.migration.alembic-v2-qualified`, `api.migration.bundle-supported-by-simulation-api`, `api.migration.contracts-consistent`, `api.migration.cross-database-integration` - `api.migration.alembic-autogenerated-only`: Alembic revisions are autogenerated against an explicitly selected v1 or v2 configuration; never hand-written DDL or data operations. (doc-only, not checked: follow it yourself) -- `api.migration.alembic-v1-qualified`: v1 Alembic revisions qualify against a disposable MySQL database. (blocking: CI job `alembic-v1-check`) -- `api.migration.alembic-v2-qualified`: v2 Alembic revisions qualify against a disposable Postgres database, and alembic check finds no schema drift. (blocking: CI job `alembic-v2-check`) -- `api.migration.bundle-supported-by-simulation-api`: A PolicyEngine bundle version change is supported by the simulation API before it merges. (blocking: CI job `ensure-policyengine-bundle-supported-by-simulation-api`) -- `api.migration.contracts-consistent`: Migration route groups and workflow contracts are internally consistent, and their generated docs are current. (blocking: quality guard `migration-contracts`) -- `api.migration.cross-database-integration`: Cross-database integration tests pass against real MySQL and Postgres. (blocking: CI job `v2-integration-check`) ### Code quality and types -- `api.quality.alembic-revisions-unused-imports`: Alembic revision files are exempt from ruff lint rules, because they must stay exactly as Alembic generates them. (advisory: ruff per-file ignore of ALL for `migrations/*/versions/*.py`, not blocking) -- `api.quality.environment-variables-load`: Required environment variables and credentials load in a deployed-like environment. (blocking: CI job `test_env_vars`) -- `api.quality.mypy-v2`: Shared query models, FastAPI dependencies, v2 routes and services, and the user sign-in and account-deletion libraries pass mypy with untyped definitions disallowed. (blocking: mypy) -- `api.quality.ruff-format`: All Python files are formatted with ruff format. (blocking: ruff format) -- `api.quality.ruff-lint`: All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. (blocking: ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W)) -- `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. (blocking: pytest `tests/unit/test_runtime_evaluated_annotations.py`) -- `api.quality.tests-pass`: The unit, contract and to-refactor test suites pass. (blocking: CI job `test`) -- `api.quality.tests-ruff-exemptions`: Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**`) +- 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.environment-variables-load`, `api.quality.mypy-v2`, `api.quality.ruff-format`, `api.quality.ruff-lint`, `api.quality.runtime-evaluated-models-resolve`, `api.quality.tests-pass`, `api.quality.tests-ruff-exemptions` ### Observability @@ -135,21 +105,15 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### CI workflows and scripts -- `api.ci.multiline-run-is-script`: A GitHub Actions run: block holds one line of code and no chained commands (&&, ||, ;); anything more lives in a tested script under .github/scripts/, called from a single-line step. (blocking: quality guard `ci-scripts`) -- `api.ci.shell-scripts-pass-shellcheck`: Every shell script under .github/scripts/ passes shellcheck. (blocking: quality guard `ci-scripts`) -- `api.ci.workflow-scripts-tested`: Every shell script a workflow calls is run by at least one test. (blocking: quality guard `ci-scripts`) +- Checked: `api.ci.multiline-run-is-script`, `api.ci.shell-scripts-pass-shellcheck`, `api.ci.workflow-scripts-tested` ### Process and repository hygiene -- `api.agents.adapters-point-to-agents-md`: CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. (blocking: quality guard `rules-registry`) +- Checked: `api.agents.adapters-point-to-agents-md`, `api.process.changelog-fragment`, `api.process.cloud-run-image-builds`, `api.process.docker-image-builds`, `api.process.rules-registry`, `api.process.stacked-prs-full-ci-at-bottom` - `api.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. (doc-only, not checked: follow it yourself) -- `api.process.changelog-fragment`: Every PR adds a changelog.d fragment. (blocking: CI job `check-changelog`) -- `api.process.cloud-run-image-builds`: The Cloud Run container image builds. (blocking: CI job `test_cloud_run_container_builds`) -- `api.process.docker-image-builds`: The API Docker image builds. (blocking: CI job `test_container_builds`) +- `api.process.agents-suppress-as-last-resort`: Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. (doc-only, not checked: follow it yourself) - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. (doc-only, not checked: follow it yourself) - `api.process.pr-tracking-issue`: Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. (doc-only, not checked: follow it yourself) - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. (doc-only, not checked: follow it yourself) -- `api.process.rules-registry`: Every rule is registered in rules/registry.toml, is enforced where it claims to be, and its rendered views are current. (blocking: quality guard `rules-registry`) -- `api.process.stacked-prs-full-ci-at-bottom`: In a stack of pull requests, only the lowest unmerged one runs the full PR workflow. Every pr.yml job is needed by the CI complete gate, which fails on the pull requests above it and otherwise passes only when every job passed, so a stack merges bottom-up and nothing merges untested. (blocking: pytest `tests/unit/test_ci_complete_gate.py`) diff --git a/changelog.d/ruff-security-logging.changed.md b/changelog.d/ruff-security-logging.changed.md new file mode 100644 index 000000000..f89ee335d --- /dev/null +++ b/changelog.d/ruff-security-logging.changed.md @@ -0,0 +1 @@ +Enabled ruff's security, logging and debugging rules (S, T20, LOG, G, DTZ, ERA, TD, FIX, EXE): type-narrowing asserts became explicit checks, command-line scripts keep their printed output with reasons, commented-out code and TODO markers are gone, the remaining best-effort telemetry calls use a commented contextlib.suppress block, and every other deliberate exception carries a reasoned suppression; error responses and published schemas are unchanged. The unused GPT-4 dashboard experiment is removed, and a doc-only rule (api.process.agents-suppress-as-last-resort) has agents suppress a ruff finding only when no code change or ruff setting removes it, and list each new suppression in the PR description. diff --git a/changelog.d/ruff-security-logging.fixed.md b/changelog.d/ruff-security-logging.fixed.md new file mode 100644 index 000000000..a1c1c0ea9 --- /dev/null +++ b/changelog.d/ruff-security-logging.fixed.md @@ -0,0 +1 @@ +Removed the debug prints from the country validator and the report output and simulation routes, so requests no longer write context-free lines to stdout. diff --git a/dashboard/experiments/gpt4_api_user.py b/dashboard/experiments/gpt4_api_user.py deleted file mode 100644 index 1fd37047b..000000000 --- a/dashboard/experiments/gpt4_api_user.py +++ /dev/null @@ -1,162 +0,0 @@ -"""Streamlit experiment that answers a UK policy question with GPT-4 and the API. - -It finds related parameters by embedding similarity, asks GPT-4 to write the -reform as JSON, runs it on the public PolicyEngine API, and asks GPT-4 to answer -from the resulting economic impact. -""" - -import json -import os -import time - -import openai -import pandas as pd -import requests -import streamlit as st -from openai.embeddings_utils import cosine_similarity, get_embedding - -# Point OpenAI to the API key -openai.api_key = os.environ["OPENAI_API_KEY"] - - -@st.cache_data -def get_embeddings_df() -> pd.DataFrame: - """Load parameter_embeddings.csv.gz, evaluating each embedding string to a list.""" - embeddings_df = pd.read_csv("parameter_embeddings.csv.gz", compression="gzip") - embeddings_df.parameter_embedding = embeddings_df.parameter_embedding.apply( - lambda x: eval(x) - ) - return embeddings_df - - -@st.cache_data -def embed(prompt: str, engine: str = "text-embedding-ada-002") -> list[float]: - """Return the OpenAI embedding of ``prompt``, cached across Streamlit reruns.""" - return get_embedding(prompt, engine=engine) - - -embeddings_df = get_embeddings_df().copy() - -st.title("GPT-4 PolicyEngine analysis") - -# Text box for people to write "change the basic rate of income tax to 20%", single line - -question = st.text_input( - "Enter a policy question", - "Revenue impact of changing the basic rate of income tax to 25%", -) - -# First task: convert to: -# { 'hmrc.income_tax.rates.uk[0].rate': { '2023-01-01.2024-01-01': 0.25 } } - -# We're going to do this with GPT4- we'll pass in the question, the related parameter metadata, and the task description. - -# First, get related parameters via a search through GPT-embedded parameters. - - -# columns are name, json, parameter_embedding - -# Get the top 5 most similar parameters - -embedding = embed(question) -embeddings_df["similarities"] = embeddings_df.parameter_embedding.apply( - lambda x: cosine_similarity(x, embedding) -) - -top5 = embeddings_df.sort_values("similarities", ascending=False).head(5)["json"].values -# display in streamlit - - -def write_prompt(question: str, relevant_parameters: object) -> str: - """Build the GPT-4 prompt that rewrites ``question`` as a reform JSON object.""" - return f"""Question: {question} - Here's some metadata about the parameters that are relevant to your question: - Relevant parameters: {relevant_parameters} - - Task: Convert the question into a reform of the syntax below. If not specified, assume the reform starts on 2023-01-01 and ends on 2024-01-01. - - - Reform syntax you should write in: {{ "parameter_name": {{ "start_date.end_date": value }} }} - e.g. "basic rate at 25%" -> {{ "hmrc.income_tax.rates.uk[0].rate": {{ "2023-01-01.2024-01-01": 0.25 }} }} - Reply only with valid JSON. - """ - - -reform_generation_prompt = write_prompt(question, top5) - -# Now, pass the prompt to GPT-4 - - -@st.cache_data -def get_gpt4_response(prompt: str) -> str: - """Return GPT-4's reply to ``prompt`` as one user message, cached across reruns.""" - return openai.ChatCompletion.create( - model="gpt-4", - messages=[{"role": "user", "content": prompt}], - )["choices"][0]["message"]["content"] - - -response = get_gpt4_response(reform_generation_prompt) - -reform = json.loads(response) - -# Next step: register the reform on the PolicyEngine API - - -@st.cache_data -def get_policyengine_impact(reform: object) -> object: - """Create ``reform`` on the public UK API and return its 2023 UK economic impact. - - Poll every five seconds, with no time limit, while the impact is computing. - """ - payload = {"data": reform} - url = "https://api.policyengine.org/uk/policy" - - response = requests.post(url, json=payload) - - # json.result.policy_id - - policy_id = response.json()["result"]["policy_id"] - - # Next step: run the reform on the PolicyEngine API - - url = f"https://api.policyengine.org/uk/economy/{policy_id}/over/1?time_period=2023®ion=uk" - - response = requests.get(url).json() - - # if 'status' = 'computing', wait 5 seconds and try again until 'status' = 'ok' - - while response["status"] == "computing": - time.sleep(5) - response = requests.get(url).json() - - return response["result"] - - -# Now, get GPT4 to answer the question with the impact JSON. - -impact = get_policyengine_impact(reform) - -question_answer_prompt = f"""Relevant economic impact simulation data: {impact} - - Question: {question} - - Task: Write a concise single-sentence response to the question, using only the impact data. If you can't from the data, say it. - - Give financial amounts in short-form (e.g. £1bn, 250m, etc.). -""" - -answer = get_gpt4_response(question_answer_prompt) - -st.write(answer) - -st.write("## How I worked it out") - -with st.expander("My prompt for understanding your reform"): - st.write(reform_generation_prompt) - -with st.expander("What I thought your reform was"): - st.write(reform) - -with st.expander("My prompt for answering your question"): - st.write(question_answer_prompt) diff --git a/dashboard/experiments/requirements.txt b/dashboard/experiments/requirements.txt deleted file mode 100644 index 10de23426..000000000 --- a/dashboard/experiments/requirements.txt +++ /dev/null @@ -1,5 +0,0 @@ -pip>=23 -streamlit -openai -pandas -requests diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index dcb2ab7d8..2973ae04d 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -61,6 +61,10 @@ For migration work, identify: or Modal migration; - which user-visible API contract changes are intentionally introduced. +For any PR that adds inline ruff suppressions (`# noqa`), list each one with +the reason a code change or ruff setting couldn't remove the finding +(`api.process.agents-suppress-as-last-resort`). + ## Commit Hygiene AI agents must run formatting and lint checks before committing their own code diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index d78d7824b..e86901988 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -69,7 +69,7 @@ gap is visible, rather than leaving it out. | --- | --- | --- | | `pytest` | `nodes` (`path::test` or `path::Class::test`), optional `project` (sub-project directory the paths are relative to) | Each test exists, and the CI job's command collects it | | `quality-guard` | `name`, optional `runner` | The name is in `GUARDS` in `scripts/run_quality_guards.py` | -| `ruff` | `mode` = `format`, `lint` (with `codes`) or `per-file-ignore` (with `pattern`, `codes`) | Codes are selected / the ignore entry exists | +| `ruff` | `mode` = `format`, `lint` (with `codes`) or `per-file-ignore` (with `pattern`, `codes`) | Codes are selected / the ignore entry exists and covers only [files allowed file-level exceptions](#file-level-exceptions) | | `mypy` | `paths` | Each path is in `[tool.mypy].files` | | `workflow-job` | optional `step` | The CI job exists (the job itself is the check) | | `pyright`, `import-linter` | `config` (+ `contract` for Import Linter) | The config file exists | @@ -117,13 +117,64 @@ fail the guard yet. - **Ruff is never ratcheted.** Adopting a ruff rule family is one standalone PR that changes the select list and fixes every resulting violation. Don't use - `--add-noqa` or blanket per-file ignores. + `--add-noqa`. A finding that is kept gets an inline `# noqa: CODE # reason`, + except in the files below. - Other enforcers (mypy, Pyright, Import Linter, custom guards) may start `ratcheted` with a baseline file of tolerated violations. Baselines may only shrink: the guard compares counts against the base branch. - Moving a rule from enforced/ratcheted to advisory/doc-only needs an explicit `[rule.downgrade] reason`. +## File-level exceptions + +A ruff `per-file-ignores` entry switches rules off for whole files. It is +allowed only where that's the norm rather than an exception to it: + +- **tests**: files under a `tests` or `authenticated_tests` directory; +- **CI/CD scripts**: files under `.github`; +- **command-line scripts**: files under a `scripts` directory, or modules that + run as a program through `if __name__ == "__main__":`. + +Everywhere else, fix the finding. An inline suppression is the last resort +(see "Inline suppressions"). + +Each entry needs a `per-file-ignore` rule (see below). The guard expands the +entry's pattern like ruff does and fails when the pattern: + +- covers any other file; +- is negated; +- matches nothing. + +`scripts/rules/file_exceptions.py` holds these definitions. + +## Inline suppressions + +An inline suppression (`# noqa: # reason`) is the last resort for a +ruff finding, not the usual way to clear one +(`api.process.agents-suppress-as-last-resort`). Before adding one, try: + +1. **A code change** that removes the finding: narrow an `except` to the + errors the code expects, pass the missing argument, or route repeated code + through one helper so a boundary is handled once instead of at every call + site. +2. **A ruff setting**, when several findings share a cause: for example, + list a lazily loaded package in `banned-module-level-imports` instead of + suppressing each deferred import of it. +3. **A file-level exception**, only where the section above allows one. + +Keep a suppression only when each of these would change behaviour that +callers rely on, or when the rule doesn't fit the code. Give the reason after +the code, as every suppression does. + +For a deliberate best-effort call whose failure must be ignored, use +`with contextlib.suppress(Exception):` with the reason in a comment above it, +the form ruff's `SIM105` recommends, instead of `try`/`except Exception: pass` +with suppressions. It still ignores every exception, so it needs the same +justification as a suppression. + +A pull request that adds inline suppressions lists each one in its +description, with the reason a code change or setting wasn't used. + ## Hidden rules The guard fails when something enforces a rule that the registry doesn't diff --git a/docs/generated/rules.md b/docs/generated/rules.md index fe7b0b4eb..60db81a67 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 | 44 | 0 | 1 | 12 | 57 | +| mechanical | 47 | 0 | 1 | 12 | 60 | | metric | 0 | 0 | 0 | 0 | 0 | -| judgment | 0 | 0 | 0 | 0 | 0 | +| judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 44 of 57 rules. +**Blocking:** 47 of 61 rules. ## Gaps @@ -22,6 +22,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.observability.id-is-diagnostic-only`: observability_id is used only to query diagnostic records, never for idempotency, cache ownership, authorization, database identity, routing or calculation behavior. - `api.observability.registered-stages-only`: Span names come from the stage plans in policyengine_api.observability.stages; route and service code never adds span-name string literals. - `api.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. +- `api.process.agents-suppress-as-last-resort`: Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. - `api.process.pr-tracking-issue`: Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. @@ -33,7 +34,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce ### Advisory (checked but not blocking) -- `api.quality.alembic-revisions-unused-imports`: Alembic revision files are exempt from ruff lint rules, because they must stay exactly as Alembic generates them. +- `api.quality.alembic-revisions-unused-imports`: Ruff doesn't lint Alembic revision files (lint.exclude in pyproject.toml), because they must stay exactly as Alembic generates them. ### Unproven (no proof that the checker rejects a bad example) @@ -268,20 +269,29 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | -| `api.quality.alembic-revisions-unused-imports` | Alembic revision files are exempt from ruff lint rules, because they must stay exactly as Alembic generates them. | mechanical | advisory | no | ruff per-file ignore of ALL for `migrations/*/versions/*.py` | — | — | +| `api.quality.alembic-revisions-unused-imports` | Ruff doesn't lint Alembic revision files (lint.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 and fetch fixed URLs (T201, S310); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of S310, T201 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/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.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.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, and the user sign-in and account-deletion libraries 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 rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W) | `pr.yml:lint` | — | +| `api.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W) | `pr.yml:lint` | — | | `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.tests-pass` | The unit, contract and to-refactor test suites pass. | mechanical | enforced | yes | CI job `test` | `pr.yml:test` | — | -| `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**` | `pr.yml:lint` | — | +| `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004). 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). Every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, DTZ001, DTZ011, PLR2004, S101, S105, S106, S603, S607, S608, T201 for `**/tests/**` | `pr.yml:lint` | — | ```mermaid flowchart LR r_api_quality_alembic_revisions_unused_imports("api.quality.alembic-revisions-unused-imports (advisory)") - e_ruff_per_file_ignore_of_ALL_for_migrations___versions___py["ruff per-file ignore of ALL for migrations/*/versions/*.py"] - local_only>"not run in CI"] + 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_S310__T201_for__github___["ruff per-file ignore of S310, T201 for .github/**"] + j_CI_pr_yml_lint["CI pr.yml:lint"] + 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_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/user_association_migration_qualification,worker_spm_release}.py"] + r_api_quality_command_line_scripts_ruff_exemptions("api.quality.command-line-scripts-ruff-exemptions (enforced)") + e_ruff_per_file_ignore_of_DTZ011__S310__S311__S603__S607__T201_for_scripts___["ruff per-file ignore of DTZ011, S310, S311, S603, S607, T201 for scripts/**"] r_api_quality_environment_variables_load("api.quality.environment-variables-load (enforced)") e_CI_job_test_env_vars["CI job test_env_vars"] j_CI_pr_yml_test_env_vars["CI pr.yml:test_env_vars"] @@ -290,32 +300,36 @@ flowchart LR 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"] - j_CI_pr_yml_lint["CI pr.yml:lint"] r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)") - e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_["ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W)"] + e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_["ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)"] r_api_quality_runtime_evaluated_models_resolve("api.quality.runtime-evaluated-models-resolve (enforced)") e_pytest_tests_unit_test_runtime_evaluated_annotations_py["pytest tests/unit/test_runtime_evaluated_annotations.py"] j_CI_pr_yml_test["CI pr.yml:test"] r_api_quality_tests_pass("api.quality.tests-pass (enforced)") e_CI_job_test["CI job test"] r_api_quality_tests_ruff_exemptions("api.quality.tests-ruff-exemptions (enforced)") - e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___["ruff per-file ignore of ANN, D1, PLR2004, S101 for **/tests/**"] - r_api_quality_alembic_revisions_unused_imports --> e_ruff_per_file_ignore_of_ALL_for_migrations___versions___py - e_ruff_per_file_ignore_of_ALL_for_migrations___versions___py -.-> local_only + e_ruff_per_file_ignore_of_ANN__D1__DTZ001__DTZ011__PLR2004__S101__S105__S106__S603__S607__S608__T201_for____tests___["ruff per-file ignore of ANN, D1, DTZ001, DTZ011, PLR2004, S101, S105, S106, S603, S607, S608, T201 for **/tests/**"] + r_api_quality_alembic_revisions_unused_imports -.-> gap_unenforced + r_api_quality_ci_cd_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_S310__T201_for__github___ + e_ruff_per_file_ignore_of_S310__T201_for__github___ --> j_CI_pr_yml_lint + 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_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_user_association_migration_qualification_worker_spm_release__py --> j_CI_pr_yml_lint + 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 r_api_quality_environment_variables_load --> e_CI_job_test_env_vars e_CI_job_test_env_vars --> j_CI_pr_yml_test_env_vars r_api_quality_mypy_v2 --> e_mypy e_mypy --> j_CI_pr_yml_typecheck_v2 r_api_quality_ruff_format --> e_ruff_format e_ruff_format --> j_CI_pr_yml_lint - r_api_quality_ruff_lint --> e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_ - e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_ --> j_CI_pr_yml_lint + r_api_quality_ruff_lint --> e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ + e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ --> j_CI_pr_yml_lint 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_tests_pass --> e_CI_job_test e_CI_job_test --> j_CI_pr_yml_test - r_api_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___ - e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___ --> j_CI_pr_yml_lint + r_api_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__DTZ001__DTZ011__PLR2004__S101__S105__S106__S603__S607__S608__T201_for____tests___ + e_ruff_per_file_ignore_of_ANN__D1__DTZ001__DTZ011__PLR2004__S101__S105__S106__S603__S607__S608__T201_for____tests___ --> j_CI_pr_yml_lint ``` ## Observability @@ -361,6 +375,7 @@ flowchart LR | --- | --- | --- | --- | --- | --- | --- | --- | | `api.agents.adapters-point-to-agents-md` | CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | yes | | `api.process.agents-never-bypass` | Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. | mechanical | doc-only | no | nothing | — | — | +| `api.process.agents-suppress-as-last-resort` | Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. | judgment | doc-only | no | nothing | — | — | | `api.process.changelog-fragment` | Every PR adds a changelog.d fragment. | mechanical | enforced | yes | CI job `check-changelog` | `pr.yml:check-changelog` | — | | `api.process.cloud-run-image-builds` | The Cloud Run container image builds. | mechanical | enforced | yes | CI job `test_cloud_run_container_builds` | `pr.yml:test_cloud_run_container_builds` | — | | `api.process.docker-image-builds` | The API Docker image builds. | mechanical | enforced | yes | CI job `test_container_builds` | `pr.yml:test_container_builds` | — | @@ -377,6 +392,7 @@ flowchart LR j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_process_agents_never_bypass("api.process.agents-never-bypass (doc-only)") gap_unenforced>"not enforced"] + r_api_process_agents_suppress_as_last_resort("api.process.agents-suppress-as-last-resort (doc-only)") r_api_process_changelog_fragment("api.process.changelog-fragment (enforced)") e_CI_job_check_changelog["CI job check-changelog"] j_CI_pr_yml_check_changelog["CI pr.yml:check-changelog"] @@ -397,6 +413,7 @@ flowchart LR r_api_agents_adapters_point_to_agents_md --> e_quality_guard_rules_registry e_quality_guard_rules_registry --> j_CI_pr_yml_quality_guards r_api_process_agents_never_bypass -.-> gap_unenforced + r_api_process_agents_suppress_as_last_resort -.-> gap_unenforced r_api_process_changelog_fragment --> e_CI_job_check_changelog e_CI_job_check_changelog --> j_CI_pr_yml_check_changelog r_api_process_cloud_run_image_builds --> e_CI_job_test_cloud_run_container_builds diff --git a/policyengine_api/api.py b/policyengine_api/api.py index df4fea71c..f2beba2fb 100644 --- a/policyengine_api/api.py +++ b/policyengine_api/api.py @@ -10,7 +10,7 @@ def log_timing(message: str) -> None: """Print a startup milestone to stderr with the seconds since import began.""" elapsed = time.time() - start_time - print(f"[{elapsed:.2f}s] {message}", file=sys.stderr, flush=True) + print(f"[{elapsed:.2f}s] {message}", file=sys.stderr, flush=True) # noqa: T201 # startup timings go straight to stderr while the app imports log_timing("Basic imports completed") @@ -31,8 +31,6 @@ def log_timing(message: str) -> None: log_timing("Caching utilities import completed") -# from werkzeug.middleware.profiler import ProfilerMiddleware - # Endpoints from policyengine_api.routes.error_routes import error_bp diff --git a/policyengine_api/asgi_factory.py b/policyengine_api/asgi_factory.py index d8f3ef1b6..a4f30e938 100644 --- a/policyengine_api/asgi_factory.py +++ b/policyengine_api/asgi_factory.py @@ -222,7 +222,8 @@ async def add_request_context_and_migration_logging( initial_route = native_route_template or request.url.path if native_request: - try: + # Telemetry must never fail the request. + with contextlib.suppress(Exception): runtime_request_id = request_runtime.begin_request( headers={ **dict(request.headers), @@ -237,8 +238,6 @@ async def add_request_context_and_migration_logging( request.state.policyengine_request_id = request_id _asgi_request_id.reset(context_token) context_token = _asgi_request_id.set(request_id) - except Exception: # noqa: BLE001 # telemetry must never fail the request - pass # Telemetry must never fail the request. with contextlib.suppress(Exception): request_runtime.set_context( @@ -306,11 +305,10 @@ def finish_native_route( observability_id=response_observability_id ) if native_request: - try: + # Telemetry must never fail the request. + with contextlib.suppress(Exception): for name, value in request_runtime.response_headers().items(): response.headers[name] = value - except Exception: # noqa: BLE001 # telemetry must never fail the request - pass _apply_request_id_header(response, request_id) if response_observability_id is not None: _apply_observability_id_header(response, response_observability_id) diff --git a/policyengine_api/constants.py b/policyengine_api/constants.py index d5249a237..a51b45627 100644 --- a/policyengine_api/constants.py +++ b/policyengine_api/constants.py @@ -18,7 +18,7 @@ UPDATE = "UPDATE" LIST = "LIST" VERSION = "3.38.1" -CURRENT_YEAR = str(datetime.now().year) +CURRENT_YEAR = str(datetime.now().year) # noqa: DTZ005 # the default year is the host's local calendar year COUNTRIES = ("uk", "us", "ca", "ng", "il") COUNTRY_PACKAGE_NAMES = ( "policyengine_uk", @@ -220,8 +220,8 @@ def _build_runtime_cache_version( schema_version, ) ) - digest = hashlib.sha1(raw.encode("utf-8")).hexdigest()[:digest_length] - return f"{prefix}{schema_version}{digest}" + digest = hashlib.sha1(raw.encode("utf-8"), usedforsecurity=False).hexdigest() + return f"{prefix}{schema_version}{digest[:digest_length]}" def get_economy_impact_cache_version( diff --git a/policyengine_api/data/v2/catalog/extraction.py b/policyengine_api/data/v2/catalog/extraction.py index 7a760a385..4111c966a 100644 --- a/policyengine_api/data/v2/catalog/extraction.py +++ b/policyengine_api/data/v2/catalog/extraction.py @@ -179,7 +179,7 @@ def normalize_json_value(value: object) -> JSONValue: def _aware_datetime(value: object, *, field_name: str) -> datetime: if isinstance(value, date) and not isinstance(value, datetime): - value = datetime(value.year, value.month, value.day) + value = datetime(value.year, value.month, value.day, tzinfo=UTC) if not isinstance(value, datetime): msg = f"{field_name} must be a date or datetime" raise CatalogExtractionError(msg) diff --git a/policyengine_api/data/v2/settings.py b/policyengine_api/data/v2/settings.py index 2b951e28f..33d8fdfea 100644 --- a/policyengine_api/data/v2/settings.py +++ b/policyengine_api/data/v2/settings.py @@ -20,7 +20,7 @@ from collections.abc import Callable, Mapping V2_RUNTIME_DATABASE_URL = "V2_RUNTIME_DATABASE_URL" -V2_RUNTIME_DATABASE_URL_SECRET_RESOURCE = "V2_RUNTIME_DATABASE_URL_SECRET_RESOURCE" +V2_RUNTIME_DATABASE_URL_SECRET_RESOURCE = "V2_RUNTIME_DATABASE_URL_SECRET_RESOURCE" # noqa: S105 # an environment variable name, not a secret V2_MIGRATION_DATABASE_URL = "V2_MIGRATION_DATABASE_URL" V2_DATA_WRITE_DATABASE_URL = "V2_DATA_WRITE_DATABASE_URL" V2_SUPABASE_PROJECT_REF = "V2_SUPABASE_PROJECT_REF" diff --git a/policyengine_api/fastapi_routes/v2/errors.py b/policyengine_api/fastapi_routes/v2/errors.py index c5ba09cfd..e99ace645 100644 --- a/policyengine_api/fastapi_routes/v2/errors.py +++ b/policyengine_api/fastapi_routes/v2/errors.py @@ -7,7 +7,7 @@ from starlette.responses import JSONResponse BEARER_CHALLENGE = "Bearer" -INVALID_TOKEN_BEARER_CHALLENGE = 'Bearer error="invalid_token"' +INVALID_TOKEN_BEARER_CHALLENGE = 'Bearer error="invalid_token"' # noqa: S105 # a WWW-Authenticate challenge, not a secret class V2RequestTooLargeError(ValueError): diff --git a/policyengine_api/fastapi_routes/v2/user_resources.py b/policyengine_api/fastapi_routes/v2/user_resources.py index 2496e9585..b1513b5bf 100644 --- a/policyengine_api/fastapi_routes/v2/user_resources.py +++ b/policyengine_api/fastapi_routes/v2/user_resources.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import logging import traceback from typing import TYPE_CHECKING, Any, TypeVar @@ -148,7 +149,8 @@ def _log_failure(event: str, error: Exception) -> None: the type and the innermost frame locations are logged. Logging never changes the response. """ - try: + # Observability must not alter the response. + with contextlib.suppress(Exception): frames = traceback.extract_tb(error.__traceback__)[-8:] logger.error( event, @@ -163,8 +165,6 @@ def _log_failure(event: str, error: Exception) -> None: } }, ) - except Exception: # noqa: BLE001 # observability must not alter the response - pass def user_resource_operation( diff --git a/policyengine_api/gcp_logging.py b/policyengine_api/gcp_logging.py index f03cdfb27..b24115e39 100644 --- a/policyengine_api/gcp_logging.py +++ b/policyengine_api/gcp_logging.py @@ -8,6 +8,7 @@ from __future__ import annotations +import contextlib from collections.abc import Mapping from typing import Any @@ -25,7 +26,8 @@ def log_struct( labels: Mapping[str, Any] | None = None, ) -> None: """Record an allowlisted structured message without affecting callers.""" - try: + # Logging must never affect the caller. + with contextlib.suppress(Exception): message = str(info.get("message") or "API event") attributes = { key: value @@ -42,8 +44,6 @@ def log_struct( severity=severity, attributes=attributes, ) - except Exception: # noqa: BLE001 # logging must never affect the caller - pass logger = _RuntimeLogger() diff --git a/policyengine_api/libs/auth0_management.py b/policyengine_api/libs/auth0_management.py index 6254b11ce..1db5aea88 100644 --- a/policyengine_api/libs/auth0_management.py +++ b/policyengine_api/libs/auth0_management.py @@ -30,8 +30,8 @@ AUTH0_MANAGEMENT_DOMAIN_ENV = "AUTH0_MANAGEMENT_DOMAIN" AUTH0_MANAGEMENT_CLIENT_ID_ENV = "AUTH0_MANAGEMENT_CLIENT_ID" -AUTH0_MANAGEMENT_CLIENT_SECRET_ENV = "AUTH0_MANAGEMENT_CLIENT_SECRET" -AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE_ENV = "AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE" +AUTH0_MANAGEMENT_CLIENT_SECRET_ENV = "AUTH0_MANAGEMENT_CLIENT_SECRET" # noqa: S105 # an environment variable name, not a secret +AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE_ENV = "AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE" # noqa: S105 # an environment variable name, not a secret AUTH0_MANAGEMENT_ENV_VARS = ( AUTH0_MANAGEMENT_DOMAIN_ENV, AUTH0_MANAGEMENT_CLIENT_ID_ENV, diff --git a/policyengine_api/libs/gateway_auth.py b/policyengine_api/libs/gateway_auth.py index 1388b91e7..c940f61db 100644 --- a/policyengine_api/libs/gateway_auth.py +++ b/policyengine_api/libs/gateway_auth.py @@ -23,8 +23,8 @@ GATEWAY_AUTH_ISSUER_ENV = "GATEWAY_AUTH_ISSUER" GATEWAY_AUTH_AUDIENCE_ENV = "GATEWAY_AUTH_AUDIENCE" GATEWAY_AUTH_CLIENT_ID_ENV = "GATEWAY_AUTH_CLIENT_ID" -GATEWAY_AUTH_CLIENT_SECRET_ENV = "GATEWAY_AUTH_CLIENT_SECRET" -GATEWAY_AUTH_CLIENT_SECRET_RESOURCE_ENV = "GATEWAY_AUTH_CLIENT_SECRET_RESOURCE" +GATEWAY_AUTH_CLIENT_SECRET_ENV = "GATEWAY_AUTH_CLIENT_SECRET" # noqa: S105 # an environment variable name, not a secret +GATEWAY_AUTH_CLIENT_SECRET_RESOURCE_ENV = "GATEWAY_AUTH_CLIENT_SECRET_RESOURCE" # noqa: S105 # an environment variable name, not a secret GATEWAY_AUTH_REQUIRED_ENV = "GATEWAY_AUTH_REQUIRED" GATEWAY_AUTH_CORE_ENV_VARS = ( diff --git a/policyengine_api/libs/simulation_entrypoint.py b/policyengine_api/libs/simulation_entrypoint.py index 47c4ae8a5..7d738db32 100644 --- a/policyengine_api/libs/simulation_entrypoint.py +++ b/policyengine_api/libs/simulation_entrypoint.py @@ -237,7 +237,7 @@ def __init__(self, entrypoint: str | None = None) -> None: "GATEWAY_AUTH_CLIENT_SECRET." ) raise GatewayAuthError(msg) - print( + print( # noqa: T201 # the unauthenticated-client warning goes to stderr whatever the logging setup "SimulationEntrypointClient initialized without gateway auth; " "all GATEWAY_AUTH_* env vars are unset and " "GATEWAY_AUTH_REQUIRED is not enabled.", diff --git a/policyengine_api/request_context.py b/policyengine_api/request_context.py index 832a75f05..d8cf806ae 100644 --- a/policyengine_api/request_context.py +++ b/policyengine_api/request_context.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import uuid from contextvars import ContextVar @@ -59,12 +60,11 @@ def _bind_observability_id(observability_id: str) -> str: flask.g.observability_id = observability_id else: _asgi_observability_id.set(observability_id) - try: + # Telemetry must never fail the request. + with contextlib.suppress(Exception): from policyengine_api.observability import get_runtime get_runtime().set_context(observability_id=observability_id) - except Exception: # noqa: BLE001 # telemetry must never fail the request - pass return observability_id diff --git a/policyengine_api/routes/household_routes.py b/policyengine_api/routes/household_routes.py index 246e01765..72f08c6f0 100644 --- a/policyengine_api/routes/household_routes.py +++ b/policyengine_api/routes/household_routes.py @@ -55,6 +55,10 @@ _ViewResult = TypeVar("_ViewResult") +# Standard-library logger for tracebacks; ``logger`` is the structured +# log_struct facade. +module_logger = logging.getLogger(__name__) + household_bp = Blueprint("household", __name__) household_service = HouseholdService() household_calculation_service = HouseholdCalculationService() @@ -412,7 +416,7 @@ def get_household_under_policy( response = _spm_error_response(error) if response is not None: return response - logging.exception( + module_logger.exception( "Error calculating household #%s under policy #%s", household_id, policy_id, @@ -446,7 +450,7 @@ def _calculate() -> dict | Response: if response is not None: return response start_observability_id() - logging.exception("Error calculating household under policy") + module_logger.exception("Error calculating household under policy") return _make_error_response( f"Error calculating household under policy: {error}", 500, @@ -476,7 +480,7 @@ def _calculate() -> dict | Response: response = _spm_error_response(error) if response is not None: return response - logging.exception("Error calculating household under policy") + module_logger.exception("Error calculating household under policy") return _make_error_response( f"Error calculating household under policy: {error}", 500, diff --git a/policyengine_api/routes/report_output_routes.py b/policyengine_api/routes/report_output_routes.py index 3048358e3..0975a853f 100644 --- a/policyengine_api/routes/report_output_routes.py +++ b/policyengine_api/routes/report_output_routes.py @@ -1,6 +1,7 @@ """Flask routes for creating, reading and updating v1 report outputs.""" import json +import logging import jsonschema import pydantic @@ -15,6 +16,8 @@ ) from policyengine_api.utils.payload_validators import validate_country +logger = logging.getLogger(__name__) + report_output_bp = Blueprint("report_output", __name__) report_output_service = ReportOutputService() @@ -62,7 +65,6 @@ def create_report_output(country_id: str) -> Response: - year (str | None): The year for the report (optional) """ - print(f"Creating report output for country {country_id}") payload = request.json if payload is None: msg = "Payload missing from request" @@ -146,8 +148,6 @@ def get_report_output(country_id: str, report_id: int) -> Response: report_id (int): The report output ID. """ - print(f"Getting report output {report_id} for country {country_id}") - view = report_output_service.resolve_report_output(country_id, report_id) if view is None: msg = f"Report #{report_id} not found." @@ -193,7 +193,7 @@ def update_report_output(country_id: str) -> Response: report_id = payload.get("id") output = payload.get("output") error_message = payload.get("error_message") - print(f"Updating report #{report_id} for country {country_id}") + logger.debug("Updating report #%s for country %s", report_id, country_id) # Validate status if provided if status is not None and status not in [ diff --git a/policyengine_api/routes/simulation_routes.py b/policyengine_api/routes/simulation_routes.py index bffb45680..d3d127d9e 100644 --- a/policyengine_api/routes/simulation_routes.py +++ b/policyengine_api/routes/simulation_routes.py @@ -1,6 +1,7 @@ """Flask routes for creating, reading and updating v1 simulation records.""" import json +import logging import jsonschema import pydantic @@ -13,6 +14,8 @@ from policyengine_api.spm import SPMValidationError from policyengine_api.utils.payload_validators import validate_country +logger = logging.getLogger(__name__) + simulation_bp = Blueprint("simulation", __name__) simulation_service = SimulationService() @@ -39,9 +42,10 @@ def _serialize_v1_simulation(simulation: Simulation) -> dict: if value is not None and not isinstance(value, str): response[field] = json.dumps(value) - # TODO: Remove this compatibility projection when the v1 contract is - # retired. New v2 response models should expose these fields as native JSON - # objects instead of carrying the historical JSON-in-a-string shape forward. + # This JSON-in-a-string projection exists only for the v1 contract and can + # go when that contract is retired. v2 response models should expose these + # fields as native JSON objects rather than carry the historical shape + # forward. return response @@ -67,8 +71,6 @@ def create_simulation(country_id: str) -> Response: - policy_id (int): The policy ID """ - print(f"Creating simulation for country {country_id}") - payload = request.json if payload is None: msg = "Payload missing from request" @@ -152,8 +154,6 @@ def get_simulation(country_id: str, simulation_id: int) -> Response: simulation_id (int): The simulation ID. """ - print(f"Getting simulation {simulation_id} for country {country_id}") - if not isinstance(simulation_id, int): msg = "simulation_id must be an integer" raise BadRequest(msg) @@ -209,7 +209,7 @@ def update_simulation(country_id: str) -> Response: simulation_id = payload.get("id") output = payload.get("output") error_message = payload.get("error_message") - print(f"Updating simulation #{simulation_id} for country {country_id}") + logger.debug("Updating simulation #%s for country %s", simulation_id, country_id) # Validate status if provided if status is not None and status not in ["pending", "complete", "error"]: diff --git a/policyengine_api/routes/user_profile_routes.py b/policyengine_api/routes/user_profile_routes.py index 1ab09dc6d..3cc175b5b 100644 --- a/policyengine_api/routes/user_profile_routes.py +++ b/policyengine_api/routes/user_profile_routes.py @@ -126,8 +126,9 @@ def update_user_profile(country_id: str) -> Response: msg = "No user data provided in request" raise BadRequest(msg) - # TODO: we should validate the payload - # to ensure type safety https://github.com/PolicyEngine/policyengine-api/issues/2054 + # The payload's field types are not validated here. Request validation was + # proposed in https://github.com/PolicyEngine/policyengine-api/issues/2054, + # which was closed as not planned. user_id = payload.pop("user_id") username = payload.pop("username", None) primary_country = payload.pop("primary_country", None) diff --git a/policyengine_api/runtime_cache/reform_impacts.py b/policyengine_api/runtime_cache/reform_impacts.py index 4d204fe66..decb7ba25 100644 --- a/policyengine_api/runtime_cache/reform_impacts.py +++ b/policyengine_api/runtime_cache/reform_impacts.py @@ -537,7 +537,7 @@ def matching( selected, key=lambda impact: ( impact.options_hash == options_hash, - impact.start_time or datetime.min, + impact.start_time or datetime.min, # noqa: DTZ901 # start_time is a naive column, so the fallback must be naive to compare with it impact.reform_impact_id, ), reverse=True, diff --git a/policyengine_api/services/__init__.py b/policyengine_api/services/__init__.py index 5bd08f8f8..0f71c72b0 100644 --- a/policyengine_api/services/__init__.py +++ b/policyengine_api/services/__init__.py @@ -1,3 +1 @@ """Business logic for the v1 API resources; ``v2`` holds the v2 resource services.""" - -# from .economy_service import EconomyService, JobService, PolicyService diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 41a1dae4d..9535be956 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -1795,7 +1795,7 @@ def _set_reform_impact_computing( status=ImpactStatus.COMPUTING.value, api_version=setup_options.api_version, reform_impact_json={}, - start_time=datetime.datetime.now(), + start_time=datetime.datetime.now(), # noqa: DTZ005 # start_time is a naive column that has always held local time execution_id=execution_id, observability_id=setup_options.observability_id, ) diff --git a/policyengine_api/services/household_calculation_service.py b/policyengine_api/services/household_calculation_service.py index 8ceb14170..3f54eeabc 100644 --- a/policyengine_api/services/household_calculation_service.py +++ b/policyengine_api/services/household_calculation_service.py @@ -110,7 +110,7 @@ def get_household_year(household: dict) -> int | str: This is the period used when filling in missing yearly variables. """ - household_year: int | str = date.today().year + household_year: int | str = date.today().year # noqa: DTZ011 # the default period is the host's local calendar year household_age_list = list( household.get("people", {}).get("you", {}).get("age", {}).keys() ) diff --git a/policyengine_api/services/v2/households/services.py b/policyengine_api/services/v2/households/services.py index f4444ee3b..0ad26681b 100644 --- a/policyengine_api/services/v2/households/services.py +++ b/policyengine_api/services/v2/households/services.py @@ -60,7 +60,9 @@ def normalize_creation_input( ) -> HouseholdCreationInput: """Return the input with its household document validated and normalized.""" document = household_input.household_data - assert isinstance(document, dict) + if not isinstance(document, dict): + msg = "household_data must be an object" + raise TypeError(msg) return HouseholdCreationInput( country_id=household_input.country_id, default_year=household_input.default_year, diff --git a/policyengine_api/services/v2/households/transformations.py b/policyengine_api/services/v2/households/transformations.py index 020705e11..a0ebe170d 100644 --- a/policyengine_api/services/v2/households/transformations.py +++ b/policyengine_api/services/v2/households/transformations.py @@ -84,7 +84,9 @@ def canonical_household_document(household_input: HouseholdCreationInput) -> byt "people", *SUPPORTED_ENTITY_COLLECTIONS[household_input.country_id], }: - assert isinstance(records, list) + if not isinstance(records, list): + msg = f"household {collection} must be a list" + raise TypeError(msg) typed_records = cast("list[dict[str, Any]]", records) content[collection] = sorted( typed_records, @@ -198,7 +200,9 @@ def translate_legacy_household( translated exactly. """ source = snapshot.household_json - assert isinstance(source, dict) + if not isinstance(source, dict): + msg = "legacy household_json must be an object" + raise TypeError(msg) collections = SUPPORTED_ENTITY_COLLECTIONS[snapshot.country_id] allowed_source = { "people", @@ -244,7 +248,9 @@ def translate_legacy_household( msg = f"legacy {source_name} members must reference named people" raise LegacyHouseholdTranslationError(msg) for raw_person_name in value: - assert isinstance(raw_person_name, str) + if not isinstance(raw_person_name, str): + msg = f"legacy {source_name} member names must be strings" + raise TypeError(msg) person_name = raw_person_name memberships = person_memberships[person_name] previous = memberships.get(collection) diff --git a/policyengine_api/services/v2/households/validators.py b/policyengine_api/services/v2/households/validators.py index 83298c844..e46520cd6 100644 --- a/policyengine_api/services/v2/households/validators.py +++ b/policyengine_api/services/v2/households/validators.py @@ -190,7 +190,9 @@ def normalize_household_document( required_memberships = set(collections) for person in normalized["people"]: memberships = person["memberships"] - assert isinstance(memberships, dict) + if not isinstance(memberships, dict): + msg = "people record memberships must be an object" + raise TypeError(msg) if set(memberships) != required_memberships: msg = ( "people record memberships must identify every country-supported " diff --git a/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index cb4c53a3a..5c21c124d 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -215,7 +215,9 @@ def mirror_legacy_policy_in_session( running_policyengine_version=running_policyengine_version, ) policy_json = snapshot.policy_json - assert isinstance(policy_json, dict) + if not isinstance(policy_json, dict): + msg = "legacy policy_json must be an object" + raise TypeError(msg) parameters = read_parameters_by_name( session, model_version_id=selected.model_version.id, diff --git a/policyengine_api/services/v2/policies/transformations.py b/policyengine_api/services/v2/policies/transformations.py index 8bb05633f..e20790ee1 100644 --- a/policyengine_api/services/v2/policies/transformations.py +++ b/policyengine_api/services/v2/policies/transformations.py @@ -270,7 +270,9 @@ def translate_legacy_policy( msg = "legacy policy api_version does not match the running country package" raise LegacyPolicyTranslationError(msg) policy_json = snapshot.policy_json - assert isinstance(policy_json, dict) + if not isinstance(policy_json, dict): + msg = "legacy policy_json must be an object" + raise TypeError(msg) parameter_names = set(policy_json) if set(parameters) != parameter_names: msg = "every legacy parameter path must exist in the running catalog" diff --git a/policyengine_api/services/v2/user_households/services.py b/policyengine_api/services/v2/user_households/services.py index 65e09c0d0..efe9c874a 100644 --- a/policyengine_api/services/v2/user_households/services.py +++ b/policyengine_api/services/v2/user_households/services.py @@ -265,7 +265,9 @@ def patch_user_household( """ repoint = "legacy_household_id" in association_input.model_fields_set if repoint: - assert association_input.legacy_household_id is not None + if association_input.legacy_household_id is None: + msg = "legacy_household_id must not be null" + raise TypeError(msg) self._require_legacy_household( country_id=country_id, legacy_household_id=association_input.legacy_household_id, diff --git a/policyengine_api/services/v2/user_policies/services.py b/policyengine_api/services/v2/user_policies/services.py index 6a65b90bf..508819a83 100644 --- a/policyengine_api/services/v2/user_policies/services.py +++ b/policyengine_api/services/v2/user_policies/services.py @@ -539,7 +539,9 @@ def patch_user_policy( """ repoint = "legacy_policy_id" in association_input.model_fields_set if repoint: - assert association_input.legacy_policy_id is not None + if association_input.legacy_policy_id is None: + msg = "legacy_policy_id must not be null" + raise TypeError(msg) self._require_legacy_policy( country_id=country_id, legacy_policy_id=association_input.legacy_policy_id, diff --git a/policyengine_api/utils/payload_validators/validate_country.py b/policyengine_api/utils/payload_validators/validate_country.py index 33f2e7a15..d8e6a06c3 100644 --- a/policyengine_api/utils/payload_validators/validate_country.py +++ b/policyengine_api/utils/payload_validators/validate_country.py @@ -41,7 +41,6 @@ def validate_country( def validate_country_wrapper( country_id: str, *args: object, **kwargs: object ) -> _ViewResult | Response: - print("Validating country") try: ensure_supported_country(country_id) except InvalidCountryError as error: diff --git a/pyproject.toml b/pyproject.toml index fd6003993..7b150a0ec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -96,18 +96,39 @@ select = [ "D", # Group 5: exceptions and messages. "BLE", "EM", "TRY", + # Group 6: security, logging and debugging leftovers. + "DTZ", "ERA", "EXE", "FIX", "G", "LOG", "S", "T20", "TD", ] # Mutually exclusive pydocstyle pairs: keep D211 (no blank line before a class # docstring) and D212 (summary on the first line). ignore = ["D203", "D213"] +# Alembic revisions must stay exactly as Alembic generates them +# (api.migration.alembic-autogenerated-only), so ruff does not lint them. This +# is an exclusion, not a file-level exception: per-file-ignores are only for +# tests, CI/CD scripts and command-line scripts. +exclude = ["migrations/*/versions/*.py"] [tool.ruff.lint.per-file-ignores] -# Alembic revisions must stay exactly as Alembic generates them -# (api.migration.alembic-autogenerated-only), so ruff does not lint them. -"migrations/*/versions/*.py" = ["ALL"] +# File-level exceptions are allowed only for tests, CI/CD scripts and +# command-line scripts; the rules guard checks each pattern covers nothing else +# (docs/engineering/skills/rules-registry.md#file-level-exceptions). # Tests may use pytest's bare assert, and need no docstrings, annotations or -# named constants for magic numbers. Every other rule applies to tests. -"**/tests/**" = ["ANN", "D1", "PLR2004", "S101"] +# named constants for magic numbers. They may also use fake credentials, naive +# datetimes, subprocess calls to repository scripts, SQL built from strings and +# prints. Every other rule applies to tests. +"**/tests/**" = [ + "ANN", "D1", "DTZ001", "DTZ011", "PLR2004", "S101", "S105", "S106", "S603", + "S607", "S608", "T201", +] +# CI/CD scripts print their results and fetch fixed URLs. +".github/**" = ["S310", "T201"] +# Command-line scripts print their output, run other programs, fetch fixed +# URLs, stamp today's date and pick random samples. +"scripts/**" = [ + "DTZ011", "S310", "S311", "S603", "S607", "T201", +] +# Modules that also run as command-line programs print their output. +"policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/user_association_migration_qualification,worker_spm_release}.py" = ["T201"] [tool.mypy] python_version = "3.11" diff --git a/rules/registry.toml b/rules/registry.toml index 4c8b8a997..57efe03c3 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -700,7 +700,7 @@ autofix = true [rule.enforcer] kind = "ruff" mode = "lint" -codes = ["ANN", "BLE", "D", "E4", "E7", "E9", "EM", "F", "FLY", "FURB", "I", "PIE", "PYI", "RET", "RSE", "TC", "TRY", "UP", "W"] +codes = ["ANN", "BLE", "D", "DTZ", "E4", "E7", "E9", "EM", "ERA", "EXE", "F", "FIX", "FLY", "FURB", "G", "I", "LOG", "PIE", "PYI", "RET", "RSE", "S", "T20", "TC", "TD", "TRY", "UP", "W"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint" @@ -732,23 +732,20 @@ invocation = "make test" [[rule]] id = "api.quality.alembic-revisions-unused-imports" category = "quality" -statement = "Alembic revision files are exempt from ruff lint rules, because they must stay exactly as Alembic generates them." +statement = "Ruff doesn't lint Alembic revision files (lint.exclude in pyproject.toml), because they must stay exactly as Alembic generates them." rationale = "docs/engineering/skills/alembic-migrations.md#mandatory-generation-rule" scope = ["migrations/*/versions/*.py"] tier = "mechanical" status = "advisory" autofix = false [rule.enforcer] -kind = "ruff" -mode = "per-file-ignore" -pattern = "migrations/*/versions/*.py" -codes = ["ALL"] +kind = "none" [[rule]] id = "api.quality.tests-ruff-exemptions" category = "quality" -statement = "Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests." -rationale = "docs/engineering/skills/rules-registry.md#baselines" +statement = "Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004). 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). Every other ruff rule applies to tests." +rationale = "docs/engineering/skills/rules-registry.md#file-level-exceptions" scope = ["tests/**/*.py", "scripts/rules/tests/**/*.py"] tier = "mechanical" status = "enforced" @@ -757,7 +754,70 @@ autofix = false kind = "ruff" mode = "per-file-ignore" pattern = "**/tests/**" -codes = ["ANN", "D1", "PLR2004", "S101"] +codes = ["ANN", "D1", "DTZ001", "DTZ011", "PLR2004", "S101", "S105", "S106", "S603", "S607", "S608", "T201"] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "lint" +invocation = "ruff check ." + +[[rule]] +id = "api.quality.ci-cd-scripts-ruff-exemptions" +category = "quality" +statement = "CI/CD scripts under .github may print their results and fetch fixed URLs (T201, S310); every other ruff rule applies to them." +rationale = "docs/engineering/skills/rules-registry.md#file-level-exceptions" +scope = [".github/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "ruff" +mode = "per-file-ignore" +pattern = ".github/**" +codes = ["S310", "T201"] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "lint" +invocation = "ruff check ." + +[[rule]] +id = "api.quality.command-line-scripts-ruff-exemptions" +category = "quality" +statement = "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." +rationale = "docs/engineering/skills/rules-registry.md#file-level-exceptions" +scope = ["scripts/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "ruff" +mode = "per-file-ignore" +pattern = "scripts/**" +codes = ["DTZ011", "S310", "S311", "S603", "S607", "T201"] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "lint" +invocation = "ruff check ." + +[[rule]] +id = "api.quality.command-line-modules-ruff-exemptions" +category = "quality" +statement = "Application modules that also run as command-line programs (if __name__ == \"__main__\") may print their output (T201); every other ruff rule applies to them." +rationale = "docs/engineering/skills/rules-registry.md#file-level-exceptions" +scope = [ + "policyengine_api/data/v2/catalog/initialization.py", + "policyengine_api/data/v2/household_migration_qualification.py", + "policyengine_api/data/v2/policy_migration_qualification.py", + "policyengine_api/data/v2/user_association_migration_qualification.py", + "policyengine_api/worker_spm_release.py", +] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "ruff" +mode = "per-file-ignore" +pattern = "policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/user_association_migration_qualification,worker_spm_release}.py" +codes = ["T201"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint" @@ -1021,6 +1081,18 @@ autofix = false [rule.enforcer] kind = "none" +[[rule]] +id = "api.process.agents-suppress-as-last-resort" +category = "process" +statement = "Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason." +rationale = "docs/engineering/skills/rules-registry.md#inline-suppressions" +scope = ["**/*.py"] +tier = "judgment" +status = "doc-only" +autofix = false +[rule.enforcer] +kind = "none" + [[rule]] id = "api.process.stacked-prs-full-ci-at-bottom" category = "process" diff --git a/scripts/capture_migration_baseline.py b/scripts/capture_migration_baseline.py index 674871fcf..8cb02d539 100644 --- a/scripts/capture_migration_baseline.py +++ b/scripts/capture_migration_baseline.py @@ -22,6 +22,11 @@ from collections.abc import Iterable +def _report(message: str) -> None: + """Print one line of the capture's output.""" + print(message) + + @dataclass(frozen=True) class RouteProbe: """One API request to time, and the status codes that count as success.""" @@ -289,7 +294,7 @@ def main(argv: list[str] | None = None) -> int: results: list[ProbeResult] = [] if not args.base_url: - print("API_BASE_URL is not set; skipping API route baseline capture.") + _report("API_BASE_URL is not set; skipping API route baseline capture.") else: results.extend(run_probes(args.base_url, args.repetitions)) @@ -304,16 +309,16 @@ def main(argv: list[str] | None = None) -> int: ) ) elif args.simulation_gateway_url or args.simulation_payload_file: - print( + _report( "SIMULATION_ENTRYPOINT_URL and SIMULATION_PAYLOAD_FILE are both required; " "skipping simulation gateway baseline capture." ) if not results: - print("No baseline probes ran.") + _report("No baseline probes ran.") return 0 - print(json.dumps(summarize(results), indent=2, sort_keys=True)) + _report(json.dumps(summarize(results), indent=2, sort_keys=True)) return 0 diff --git a/scripts/process_v1_household_mirror_event.py b/scripts/process_v1_household_mirror_event.py old mode 100644 new mode 100755 diff --git a/scripts/publish_v2_metadata_catalog.py b/scripts/publish_v2_metadata_catalog.py old mode 100644 new mode 100755 diff --git a/scripts/qualify_v2_household_migration.py b/scripts/qualify_v2_household_migration.py old mode 100644 new mode 100755 diff --git a/scripts/qualify_v2_policy_migration.py b/scripts/qualify_v2_policy_migration.py old mode 100644 new mode 100755 diff --git a/scripts/qualify_v2_user_association_migration.py b/scripts/qualify_v2_user_association_migration.py old mode 100644 new mode 100755 diff --git a/scripts/rules/ci_scripts.py b/scripts/rules/ci_scripts.py index 0a0edecd2..571b63ac2 100644 --- a/scripts/rules/ci_scripts.py +++ b/scripts/rules/ci_scripts.py @@ -149,7 +149,9 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio A check that could not run is skipped rather than reported. """ config = registry.ci_scripts - assert config is not None + if config is None: + msg = "the registry has no [ci_scripts] table" + raise TypeError(msg) checks = ( ( "multiline", @@ -197,7 +199,9 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio def write_baselines(registry: Registry, findings: Findings, root: Path) -> None: """Record today's findings as the baselines (used once, when adopting the rule).""" config = registry.ci_scripts - assert config is not None + if config is None: + msg = "the registry has no [ci_scripts] table" + raise TypeError(msg) for found, path in ( (findings.multiline, config.multiline_baseline), (findings.untested, config.untested_baseline), diff --git a/scripts/rules/cli.py b/scripts/rules/cli.py index a4134cacb..78ebf6149 100644 --- a/scripts/rules/cli.py +++ b/scripts/rules/cli.py @@ -14,12 +14,17 @@ from scripts.rules.validate import validate +def _report(message: str) -> None: + """Print one line of the command's output.""" + print(message) + + def _print(violations: list, notes: list[str] | None = None) -> int: for note in notes or []: - print(f"note: {note}") + _report(f"note: {note}") for violation in violations: - print(f" - {violation}") - print("rules: failed" if violations else "rules: passed") + _report(f" - {violation}") + _report("rules: failed" if violations else "rules: passed") return 1 if violations else 0 @@ -55,7 +60,7 @@ def main(argv: list[str] | None = None) -> int: ci_scripts.write_baselines( registry, ci_scripts.find(registry.ci_scripts, root), root ) - print("wrote ci_scripts baselines; update the rules' baseline counts") + _report("wrote ci_scripts baselines; update the rules' baseline counts") return 0 violations, notes = ci_scripts.run_check(root) return _print(violations, notes) @@ -71,9 +76,9 @@ def main(argv: list[str] | None = None) -> int: violations += validate(registry, root, date.today()) if args.command == "render": if violations: - print("render refused: fix the registry first") + _report("render refused: fix the registry first") return _print(violations) for path in write_outputs(registry, root): - print(f"wrote {path.relative_to(root)}") + _report(f"wrote {path.relative_to(root)}") return 0 return _print(violations) diff --git a/scripts/rules/file_exceptions.py b/scripts/rules/file_exceptions.py new file mode 100644 index 000000000..2bca805b4 --- /dev/null +++ b/scripts/rules/file_exceptions.py @@ -0,0 +1,160 @@ +"""Where file-level lint exceptions are allowed. + +A ruff ``per-file-ignores`` entry switches rules off for whole files, so it is +allowed only for files where that is the norm rather than an exception to it: + +- **tests**: files under a ``tests`` or ``authenticated_tests`` directory; +- **CI/CD scripts**: files under ``.github``; +- **command-line scripts**: files under a ``scripts`` directory, or Python + modules that run as a program through ``if __name__ == "__main__":``. + +Everywhere else a kept finding gets an inline suppression with its reason. See +docs/engineering/skills/rules-registry.md#file-level-exceptions. +""" + +from __future__ import annotations + +import ast +import os +import re +from functools import cache +from pathlib import Path + +TEST_DIRECTORIES = frozenset({"tests", "authenticated_tests"}) +CI_DIRECTORY = ".github" +SCRIPT_DIRECTORY = "scripts" +LINTED_SUFFIXES = (".py", ".pyi") +# Directories that hold no source of this repository. +SKIPPED_DIRECTORIES = frozenset( + { + ".git", + ".venv", + "venv", + "node_modules", + "__pycache__", + ".mypy_cache", + ".ruff_cache", + } +) + + +# One piece of a ruff (globset) pattern: a wildcard, a character class, an +# alternation character, or a run of literal characters. +GLOB_PIECE = re.compile(r"\*\*/|\*\*|\*|\?|\[!?[^\]]*\]|[{},\[]|[^*?\[{},]+") +# As in ruff, "*" and "?" also match "/". +WILDCARDS = {"**/": "(?:.*/)?", "**": ".*", "*": ".*", "?": "."} + + +def _character_class(piece: str) -> str: + if piece == "[": + return re.escape(piece) + body = piece[1:-1].replace("\\", "\\\\") + if body.startswith("!"): + body = "^" + body[1:] + return f"[{body}]" + + +def glob_regex(pattern: str) -> re.Pattern[str]: + """Translate a ruff (globset) file pattern into a regex over relative paths. + + Supports ``**``, ``*``, ``?``, ``[...]`` and ``{a,b}`` alternatives. As in + ruff, ``*`` and ``?`` also match ``/``, so ``pkg/*.py`` covers + ``pkg/sub/module.py`` too. + """ + parts = [] + open_alternations = 0 + for piece in GLOB_PIECE.findall(pattern): + if piece in WILDCARDS: + parts.append(WILDCARDS[piece]) + elif piece.startswith("["): + parts.append(_character_class(piece)) + elif piece == "{": + parts.append("(?:") + open_alternations += 1 + elif piece == "}" and open_alternations: + parts.append(")") + open_alternations -= 1 + elif piece == "," and open_alternations: + parts.append("|") + else: + parts.append(re.escape(piece)) + return re.compile("".join(parts) + "$") + + +@cache +def linted_files(root: Path) -> tuple[str, ...]: + """Return the repository's Python files as sorted root-relative POSIX paths.""" + found = [] + for directory, subdirectories, files in os.walk(root): + subdirectories[:] = [ + name + for name in subdirectories + if name not in SKIPPED_DIRECTORIES + and (not name.startswith(".") or name == CI_DIRECTORY) + ] + found.extend( + Path(directory, name).relative_to(root).as_posix() + for name in files + if name.endswith(LINTED_SUFFIXES) + ) + return tuple(sorted(found)) + + +def matching_files(root: Path, pattern: str) -> list[str]: + """Return the files a ruff ``per-file-ignores`` pattern applies to. + + Like ruff, a pattern matches either a file's path or its name. + """ + regex = glob_regex(pattern) + return [ + path + for path in linted_files(root) + if regex.match(path) or regex.match(path.rsplit("/", 1)[-1]) + ] + + +def runs_as_program(root: Path, path: str) -> bool: + """Return whether a module has a top-level ``if __name__ == "__main__":``.""" + try: + tree = ast.parse((root / path).read_text(encoding="utf-8")) + except (OSError, SyntaxError, UnicodeDecodeError): + return False + for node in tree.body: + if ( + isinstance(node, ast.If) + and isinstance(node.test, ast.Compare) + and isinstance(node.test.left, ast.Name) + and node.test.left.id == "__name__" + and any( + isinstance(comparator, ast.Constant) and comparator.value == "__main__" + for comparator in node.test.comparators + ) + ): + return True + return False + + +def allows_file_level_exceptions(root: Path, path: str) -> bool: + """Return whether a file is a test, a CI/CD script or a command-line script.""" + directories = path.split("/")[:-1] + return ( + any(directory in TEST_DIRECTORIES for directory in directories) + or (bool(directories) and directories[0] == CI_DIRECTORY) + or SCRIPT_DIRECTORY in directories + or runs_as_program(root, path) + ) + + +def out_of_scope_files(root: Path, pattern: str) -> list[str] | None: + """Return the files a pattern covers that may not take file-level exceptions. + + ``None`` means the pattern can't be checked: it is negated, so it covers + every file it doesn't name. + """ + if pattern.startswith("!"): + return None + return [ + path + for path in matching_files(root, pattern) + if not allows_file_level_exceptions(root, path) + ] diff --git a/scripts/rules/resolve.py b/scripts/rules/resolve.py index ed2bba559..d00d15e91 100644 --- a/scripts/rules/resolve.py +++ b/scripts/rules/resolve.py @@ -13,6 +13,11 @@ from functools import cache from typing import TYPE_CHECKING, Any +from scripts.rules.file_exceptions import ( + linted_files, + matching_files, + out_of_scope_files, +) from scripts.rules.model import CiRef, Kind, Rule, Violation from scripts.rules.workflows import JobBlock, parse_jobs @@ -23,6 +28,8 @@ DEFAULT_RUNNER = "scripts/run_quality_guards.py" RUFF_DEFAULT_SELECT = ("E4", "E7", "E9", "F") PROOF_CI_FIELDS = ("workflow", "job", "invocation") +# How many offending files a file-level exception violation names. +SHOWN_OUT_OF_SCOPE_FILES = 3 # --- shared readers ------------------------------------------------------- @@ -38,6 +45,7 @@ def clear_caches() -> None: _read.cache_clear() workflow_jobs.cache_clear() test_names.cache_clear() + linted_files.cache_clear() @cache @@ -264,6 +272,45 @@ def _resolve_ruff(rule: Rule, root: Path) -> list[Violation]: "match enforcer.pattern/codes to the ruff config", ) ] + return _file_level_exception_scope(rule, root, pattern) + return [] + + +def _file_level_exception_scope( + rule: Rule, root: Path, pattern: str +) -> list[Violation]: + """Check a per-file ignore covers only tests, CI/CD and command-line scripts.""" + out_of_scope = out_of_scope_files(root, pattern) + if out_of_scope is None: + return [ + _v( + rule, + f"ruff per-file-ignores pattern {pattern!r} is negated", + "name the files the exception covers", + ) + ] + if not matching_files(root, pattern): + return [ + _v( + rule, + f"ruff per-file-ignores pattern {pattern!r} matches no files", + "fix the pattern, or remove the exception", + ) + ] + if out_of_scope: + shown = ", ".join(out_of_scope[:SHOWN_OUT_OF_SCOPE_FILES]) + hidden = len(out_of_scope) - SHOWN_OUT_OF_SCOPE_FILES + more = f" and {hidden} more" if hidden > 0 else "" + return [ + _v( + rule, + f"ruff per-file-ignores {pattern!r} covers files that may not take " + f"file-level exceptions: {shown}{more}", + "file-level exceptions are only for tests, CI/CD scripts and " + "command-line scripts; narrow the pattern and suppress other " + "findings inline with a reason", + ) + ] return [] diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 400a68b7e..00151ed23 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -38,6 +38,34 @@ def with_rule(extra: str) -> dict[str, str]: return {"rules/registry.toml": REGISTRY + extra} +def per_file_ignore_rule(name: str, pattern: str) -> str: + """Return a registry rule describing a ruff per-file ignore of E501.""" + return f""" +[[rule]] +id = "ex.quality.{name}-ignores" +category = "quality" +statement = "Long lines are allowed here." +rationale = "docs/guide.md" +scope = ["rules/registry.toml"] +tier = "mechanical" +status = "advisory" +autofix = false +[rule.enforcer] +kind = "ruff" +mode = "per-file-ignore" +pattern = "{pattern}" +codes = ["E501"] +""" + + +def per_file_ignore_config(pattern: str) -> str: + """Return a pyproject.toml whose ruff config ignores E501 for ``pattern``.""" + return ( + '[tool.mypy]\nfiles = ["pkg"]\n\n' + f'[tool.ruff.lint.per-file-ignores]\n"{pattern}" = ["E501"]\n' + ) + + class ValidRepositoryTest(unittest.TestCase): def test_valid_repository_passes(self) -> None: with Repo() as repo: @@ -442,6 +470,66 @@ def test_unregistered_ruff_ignore(self) -> None: ) self.assertViolation(repo, "ruff per-file-ignores 'pkg/*.py' has no rule") + def test_file_level_exceptions_only_for_tests_ci_and_scripts(self) -> None: + program = 'def main():\n pass\n\n\nif __name__ == "__main__":\n main()\n' + allowed = { + "tests/**": "tests", + ".github/**": "ci", + "scripts/**": "scripts", + "pkg/cli.py": "cli", + } + for pattern, name in allowed.items(): + with ( + self.subTest(pattern=pattern), + Repo( + { + "pkg/cli.py": program, + ".github/scripts/release.py": "print(1)\n", + **with_rule(per_file_ignore_rule(name, pattern)), + } + ) as repo, + ): + repo.write("pyproject.toml", per_file_ignore_config(pattern)) + self.assertEqual([m for m in repo.messages() if "per-file" in m], []) + + def test_file_level_exception_outside_allowed_files_is_rejected(self) -> None: + with Repo(with_rule(per_file_ignore_rule("routes", "pkg/*.py"))) as repo: + repo.write("pyproject.toml", per_file_ignore_config("pkg/*.py")) + self.assertViolation( + repo, + "covers files that may not take file-level exceptions: pkg/routes.py", + ) + + def test_file_level_exception_needs_a_program_outside_scripts(self) -> None: + # A module with a main() that is never run as a program isn't a + # command-line script. + with Repo( + { + "pkg/cli.py": "def main():\n pass\n", + **with_rule(per_file_ignore_rule("cli", "pkg/cli.py")), + } + ) as repo: + repo.write("pyproject.toml", per_file_ignore_config("pkg/cli.py")) + self.assertViolation(repo, "may not take file-level exceptions: pkg/cli.py") + + def test_file_level_exception_star_crosses_directories(self) -> None: + # Ruff's * also matches "/", so "*/routes.py" reaches pkg/routes.py. + with Repo(with_rule(per_file_ignore_rule("star", "*/routes.py"))) as repo: + repo.write("pyproject.toml", per_file_ignore_config("*/routes.py")) + self.assertViolation( + repo, "may not take file-level exceptions: pkg/routes.py" + ) + + def test_negated_file_level_exception_is_rejected(self) -> None: + with Repo(with_rule(per_file_ignore_rule("negated", "!tests/**"))) as repo: + repo.write("pyproject.toml", per_file_ignore_config("!tests/**")) + self.assertViolation(repo, "pattern '!tests/**' is negated") + + def test_file_level_exception_must_match_a_file(self) -> None: + with Repo(with_rule(per_file_ignore_rule("stale", "nowhere/**"))) as repo: + repo.write("pyproject.toml", per_file_ignore_config("nowhere/**")) + self.assertViolation(repo, "pattern 'nowhere/**' matches no files") + def test_unregistered_ci_job(self) -> None: with Repo() as repo: text = repo.read(".github/workflows/pr.yml") diff --git a/scripts/smoke_test_local_boot.py b/scripts/smoke_test_local_boot.py old mode 100644 new mode 100755 index eca2fd272..8aa3cdde8 --- a/scripts/smoke_test_local_boot.py +++ b/scripts/smoke_test_local_boot.py @@ -25,6 +25,11 @@ HTTP_TIMEOUT_SECONDS = 2 +def _report(message: str) -> None: + """Print one line of the smoke test's output.""" + print(message) + + def find_free_port() -> int: """Return a localhost TCP port that was free when checked (nothing reserves it).""" with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: @@ -49,8 +54,8 @@ def fetch(url: str) -> tuple[int, str]: def print_section(title: str, body: str) -> None: """Print a titled block of captured output, or "(empty)" when there is none.""" - print(f"\n=== {title} ===") - print(body.rstrip() if body else "(empty)") + _report(f"\n=== {title} ===") + _report(body.rstrip() if body else "(empty)") def main() -> int: @@ -87,12 +92,12 @@ def main() -> int: except importlib.metadata.PackageNotFoundError: policyengine_version = "not installed" - print(f"repo: {REPO_ROOT}") - print(f"python: {sys.executable}") - print(f"policyengine: {policyengine_version}") - print(f"port: {port}") - print(f"stdout log: {stdout_path}") - print(f"stderr log: {stderr_path}") + _report(f"repo: {REPO_ROOT}") + _report(f"python: {sys.executable}") + _report(f"policyengine: {policyengine_version}") + _report(f"port: {port}") + _report(f"stdout log: {stdout_path}") + _report(f"stderr log: {stderr_path}") with ( stdout_path.open("w", encoding="utf-8") as stdout_handle, @@ -142,22 +147,22 @@ def main() -> int: ) if process.poll() is not None else process.terminate() if process.poll() is None: process.wait(timeout=5) - print("\nBoot smoke test failed: app did not become ready.") + _report("\nBoot smoke test failed: app did not become ready.") print_section("stdout", read_text(stdout_path)) print_section("stderr", read_text(stderr_path)) return 1 - print("\nBoot smoke test reached a live app. Probing health endpoints:") + _report("\nBoot smoke test reached a live app. Probing health endpoints:") for path in HEALTH_PATHS: status, body = fetch(f"http://127.0.0.1:{port}{path}") - print(f"{path}: {status} {body.strip()}") + _report(f"{path}: {status} {body.strip()}") if status != 200 or body.strip() != "OK": - print("\nHealth endpoint probe failed.") + _report("\nHealth endpoint probe failed.") print_section("stdout", read_text(stdout_path)) print_section("stderr", read_text(stderr_path)) return 1 - print("\nBoot smoke test passed.") + _report("\nBoot smoke test passed.") return 0 finally: if "process" in locals() and process.poll() is None: diff --git a/tests/integration/test_runtime_cache_redis.py b/tests/integration/test_runtime_cache_redis.py index 941b4953d..2b114f3cc 100644 --- a/tests/integration/test_runtime_cache_redis.py +++ b/tests/integration/test_runtime_cache_redis.py @@ -23,6 +23,9 @@ ) RUNTIME_CACHE_TEST_URL = "RUNTIME_CACHE_TEST_URL" +# Claim tokens are opaque ids that tell competing workers apart. +WRITER_CLAIM_TOKEN = "writer" +CONTENDER_CLAIM_TOKEN = "contender" @pytest.fixture @@ -184,30 +187,30 @@ def test_reform_submission_claim_is_shared_across_connections(redis_pair) -> Non assert writer.claim_start( **arguments, - claim_token="writer", + claim_token=WRITER_CLAIM_TOKEN, observability_id=writer_observability_id, ) assert not contender.claim_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) winning_claim = contender.get_start_claim(**arguments) - assert winning_claim.submission_claim_id == "writer" + assert winning_claim.submission_claim_id == WRITER_CLAIM_TOKEN assert winning_claim.observability_id == writer_observability_id assert not contender.release_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) assert writer.release_start( **arguments, - claim_token="writer", + claim_token=WRITER_CLAIM_TOKEN, observability_id=writer_observability_id, ) assert contender.claim_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) diff --git a/tests/to_refactor/python/test_policy.py b/tests/to_refactor/python/test_policy.py index b6adf8397..1857bee9a 100644 --- a/tests/to_refactor/python/test_policy.py +++ b/tests/to_refactor/python/test_policy.py @@ -57,61 +57,3 @@ def test_create_nonunique_policy(self, api_client): def test_create_policy_invalid_country(self, api_client): res = api_client.post("/au/policy", json=self.test_policy) assert res.status_code == 400 - - -# The below test is very prone to race conditions due to emission against live db; -# should be refactored to use mock/test db and reinstated - -# class TestPolicySearch: -# country_id = "us" -# policy_json = {"sample_input": {"2023-01-01.2024-12-31": True}} -# label = "maxwell" -# policy_hash = hash_object(policy_json) -# api_version = COUNTRY_PACKAGE_VERSIONS.get(country_id) -# -# # Pre-seed database with duplicate policies -# for i in range(2): -# database.query( -# f"INSERT INTO policy (country_id, label, policy_json, policy_hash, api_version) VALUES (?, ?, ?, ?, ?)", -# ( -# country_id, -# label, -# json.dumps(policy_json), -# policy_hash, -# api_version, -# ), -# ) -# -# db_output = database.query( -# f"SELECT * FROM policy WHERE label = ?", -# (label,), -# ).fetchall() -# -# def test_search_all_policies(self, api_client): -# res = api_client.get("/us/policies") -# return_object = json.loads(res.text) -# -# filtered_return = list( -# filter(lambda x: x["label"] == self.label, return_object["result"]) -# ) -# -# assert return_object["status"] == "ok" -# assert len(filtered_return) == len(self.db_output) -# -# def test_search_unique_policies(self, api_client): -# res = api_client.get("/us/policies?unique_only=true") -# return_object = json.loads(res.text) -# -# filtered_return = list( -# filter(lambda x: x["label"] == self.label, return_object["result"]) -# ) -# -# assert return_object["status"] == "ok" -# assert len(filtered_return) == 1 -# -# # Clean up duplicate policies created -# database.query( -# f"DELETE FROM policy WHERE policy_hash = ? AND label = ? AND country_id = ?", -# (self.policy_hash, self.label, self.country_id), -# ) -# diff --git a/tests/unit/data/test_v1_database_migration.py b/tests/unit/data/test_v1_database_migration.py index 29d172aa7..2b5d37579 100644 --- a/tests/unit/data/test_v1_database_migration.py +++ b/tests/unit/data/test_v1_database_migration.py @@ -15,11 +15,15 @@ upgrade_database, ) +# Fake credentials; the first holds characters that URLs must escape. +FAKE_PASSWORD_WITH_URL_CHARACTERS = "p@ss:/word" +FAKE_PASSWORD = "p@ssword" + def test_database_url_uses_sqlalchemy_url_without_stringifying_credentials(): url = build_database_url( username="schema reader", - password="p@ss:/word", + password=FAKE_PASSWORD_WITH_URL_CHARACTERS, host="127.0.0.1", port=3307, database="policyengine", @@ -27,7 +31,7 @@ def test_database_url_uses_sqlalchemy_url_without_stringifying_credentials(): assert isinstance(url, URL) assert url.username == "schema reader" - assert url.password == "p@ss:/word" + assert url.password == FAKE_PASSWORD_WITH_URL_CHARACTERS assert url.host == "127.0.0.1" assert url.port == 3307 assert url.database == "policyengine" @@ -57,13 +61,13 @@ def test_database_target_builds_in_memory_url_for_each_database_role( ): monkeypatch.delenv("STAGE7_EXISTING_DATABASE_URL", raising=False) monkeypatch.delenv("ALEMBIC_DATABASE_URL", raising=False) - monkeypatch.setenv(password_name, "p@ssword") + monkeypatch.setenv(password_name, FAKE_PASSWORD) target = migration._database_target(mode) assert isinstance(target, URL) assert target.username == expected_user - assert target.password == "p@ssword" + assert target.password == FAKE_PASSWORD assert target.host == "127.0.0.1" assert target.port == 3307 assert target.database == "policyengine" diff --git a/tests/unit/runtime_cache/test_reform_impacts.py b/tests/unit/runtime_cache/test_reform_impacts.py index 26a02cca7..2f47afc6f 100644 --- a/tests/unit/runtime_cache/test_reform_impacts.py +++ b/tests/unit/runtime_cache/test_reform_impacts.py @@ -18,6 +18,12 @@ reform_impact_id, ) +# Claim tokens are opaque ids that tell competing workers apart. +OWNER_CLAIM_TOKEN = "owner" +CONTENDER_CLAIM_TOKEN = "contender" +CLIFF_OWNER_CLAIM_TOKEN = "cliff-owner" +NEXT_OWNER_CLAIM_TOKEN = "next-owner" + def _namespace() -> CacheNamespace: return CacheNamespace("test", "api") @@ -70,7 +76,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.claim_start( **arguments, - claim_token="owner", + claim_token=OWNER_CLAIM_TOKEN, observability_id=owner_observability_id, ) is True @@ -78,7 +84,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.claim_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) is False @@ -86,7 +92,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.claim_start( **_claim_arguments(target="cliff"), - claim_token="cliff-owner", + claim_token=CLIFF_OWNER_CLAIM_TOKEN, observability_id=owner_observability_id, ) is True @@ -95,12 +101,12 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: claim = cache.get_start_claim(**arguments) assert claim is not None - assert claim.submission_claim_id == "owner" + assert claim.submission_claim_id == OWNER_CLAIM_TOKEN assert claim.observability_id == owner_observability_id assert ( cache.release_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) is False @@ -108,7 +114,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.release_start( **arguments, - claim_token="owner", + claim_token=OWNER_CLAIM_TOKEN, observability_id=contender_observability_id, ) is False @@ -116,7 +122,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.release_start( **arguments, - claim_token="owner", + claim_token=OWNER_CLAIM_TOKEN, observability_id=owner_observability_id, ) is True @@ -124,7 +130,7 @@ def test_reform_impact_start_claim_is_atomic_exact_ttl_and_token_safe() -> None: assert ( cache.claim_start( **arguments, - claim_token="next-owner", + claim_token=NEXT_OWNER_CLAIM_TOKEN, observability_id=contender_observability_id, ) is True diff --git a/tests/unit/services/test_reform_impacts_service.py b/tests/unit/services/test_reform_impacts_service.py index c4371bffa..84acb781b 100644 --- a/tests/unit/services/test_reform_impacts_service.py +++ b/tests/unit/services/test_reform_impacts_service.py @@ -11,6 +11,10 @@ ReformImpactsService, ) +# Claim tokens are opaque ids that tell competing workers apart. +OWNER_CLAIM_TOKEN = "owner" +CONTENDER_CLAIM_TOKEN = "contender" + @pytest.fixture def service(): @@ -100,25 +104,25 @@ def test_reform_impact_start_claim_is_exclusive_and_releasable(service): assert service.claim_reform_impact_start( **arguments, - claim_token="owner", + claim_token=OWNER_CLAIM_TOKEN, observability_id=owner_observability_id, ) assert not service.claim_reform_impact_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) claim = service.get_reform_impact_start_claim(**arguments) - assert claim.submission_claim_id == "owner" + assert claim.submission_claim_id == OWNER_CLAIM_TOKEN assert claim.observability_id == owner_observability_id service.release_reform_impact_start( **arguments, - claim_token="owner", + claim_token=OWNER_CLAIM_TOKEN, observability_id=owner_observability_id, ) assert service.claim_reform_impact_start( **arguments, - claim_token="contender", + claim_token=CONTENDER_CLAIM_TOKEN, observability_id=contender_observability_id, ) diff --git a/tests/unit/test_cloud_run_deploy_scripts.py b/tests/unit/test_cloud_run_deploy_scripts.py index ce44fb366..e5cc55354 100644 --- a/tests/unit/test_cloud_run_deploy_scripts.py +++ b/tests/unit/test_cloud_run_deploy_scripts.py @@ -19,6 +19,7 @@ TEST_V2_RUNTIME_SECRET_RESOURCE = ( "projects/test-project/secrets/v2-runtime-database-url/versions/latest" ) +TEST_AUTH0_SECRET_RESOURCE = "projects/p/secrets/auth0-management/versions/latest" TEST_OTEL_ENDPOINT = "https://collector.example.test" CLOUD_RUN_SERVICE_SCRIPTS = ( "scripts/deploy_cloud_run_candidate.sh", @@ -26,11 +27,12 @@ "scripts/resolve_cloud_run_candidate_state.sh", "scripts/set_cloud_run_revision.sh", ) +PRODUCTION_DB_PASSWORD_SECRET = "policyengine-api-prod-db-password:latest" DEDICATED_CLOUD_RUN_RUNTIME_SERVICE_ACCOUNT = ( "policyengine-api-cr-runtime@policyengine-api.iam.gserviceaccount.com" ) CLOUD_RUN_SECRET_MAPPINGS = { - "POLICYENGINE_DB_PASSWORD": "policyengine-api-prod-db-password:latest", + "POLICYENGINE_DB_PASSWORD": PRODUCTION_DB_PASSWORD_SECRET, "POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN": ( "policyengine-api-prod-github-microdata-token:latest" ), @@ -110,11 +112,9 @@ def _required_runtime_env() -> dict[str, str]: TEST_V2_RUNTIME_SECRET_RESOURCE ), "PRODUCTION_CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET": ( - "policyengine-api-prod-db-password:latest" - ), - "CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET": ( - "policyengine-api-prod-db-password:latest" + PRODUCTION_DB_PASSWORD_SECRET ), + "CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET": PRODUCTION_DB_PASSWORD_SECRET, "POLICYENGINE_DB_PASSWORD": "raw-db-secret-value", "POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN": ("raw-github-secret-value"), "OPENAI_API_KEY": "raw-openai-secret-value", @@ -138,13 +138,12 @@ def _required_runtime_env() -> dict[str, str]: } -def _run_script( - path: str, - env: dict[str, str], - *args: str, +def _run_bash( + *arguments: str, + env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess[str]: return subprocess.run( - ["bash", path, *args], + ["bash", *arguments], cwd=REPO, env=env, text=True, @@ -153,6 +152,14 @@ def _run_script( ) +def _run_script( + path: str, + env: dict[str, str], + *args: str, +) -> subprocess.CompletedProcess[str]: + return _run_bash(path, *args, env=env) + + def _fake_gcloud(tmp_path: Path) -> tuple[Path, Path]: state_path = tmp_path / "cloud-run-state.json" state_path.write_text( @@ -398,17 +405,16 @@ def _run_simulation_version_guard( f"{shlex.quote(versions_json)}" '; }; . .github/request-simulation-model-versions.sh "$@"' ) - return subprocess.run( - ["bash", "-c", command, "request-simulation-model-versions.sh", *args], - cwd=REPO, + return _run_bash( + "-c", + command, + "request-simulation-model-versions.sh", + *args, env=_script_env( SIM_ENTRYPOINT=entrypoint, SIMULATION_ENTRYPOINT_URL="https://simulation.example.test", OLD_SIMULATION_GATEWAY_URL="https://old-gateway.example.test", ), - text=True, - capture_output=True, - check=False, ) @@ -471,13 +477,7 @@ def test_cloud_run_startup_uses_asgi_entrypoint(): def test_cloud_run_startup_script_is_shell_syntax_valid(): - result = subprocess.run( - ["bash", "-n", "gcp/cloud_run/start.sh"], - cwd=REPO, - text=True, - capture_output=True, - check=False, - ) + result = _run_bash("-n", "gcp/cloud_run/start.sh") assert result.returncode == 0, result.stderr @@ -569,13 +569,13 @@ def test_simulation_version_guard_rejects_missing_selected_endpoint( command = ( "curl() { printf '{}'; }; . .github/request-simulation-model-versions.sh \"$@\"" ) - result = subprocess.run( - ["bash", "-c", command, "request-simulation-model-versions.sh", "-py", "1.0"], - cwd=REPO, + result = _run_bash( + "-c", + command, + "request-simulation-model-versions.sh", + "-py", + "1.0", env=_script_env(SIM_ENTRYPOINT=entrypoint), - text=True, - capture_output=True, - check=False, ) assert result.returncode == 1 @@ -592,16 +592,12 @@ def test_policyengine_bundle_support_check_passes_pyproject_pin_to_guard(tmp_pat encoding="utf-8", ) - result = subprocess.run( - ["bash", ".github/check-policyengine-bundle-supported.sh"], - cwd=REPO, + result = _run_bash( + ".github/check-policyengine-bundle-supported.sh", env=_script_env( SIMULATION_VERSION_GUARD_SCRIPT=str(guard_script), CAPTURE_PATH=str(capture_path), ), - text=True, - capture_output=True, - check=False, ) pyproject = (REPO / "pyproject.toml").read_text(encoding="utf-8") @@ -750,11 +746,9 @@ def test_validate_cloud_run_deploy_env_accepts_direct_mode_from_environment(): TEST_V2_RUNTIME_SECRET_RESOURCE ), PRODUCTION_CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=( - "policyengine-api-prod-db-password:latest" - ), - CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=( - "policyengine-api-prod-db-password:latest" + PRODUCTION_DB_PASSWORD_SECRET ), + CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=PRODUCTION_DB_PASSWORD_SECRET, **_v2_target_env(), **_gateway_auth_env(), **_public_api_auth_env(), @@ -908,11 +902,9 @@ def test_validate_cloud_run_deploy_env_requires_only_selected_url( TEST_V2_RUNTIME_SECRET_RESOURCE ), PRODUCTION_CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=( - "policyengine-api-prod-db-password:latest" - ), - CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=( - "policyengine-api-prod-db-password:latest" + PRODUCTION_DB_PASSWORD_SECRET ), + CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=PRODUCTION_DB_PASSWORD_SECRET, **_v2_target_env(), **_gateway_auth_env(), **_public_api_auth_env(), @@ -2326,9 +2318,7 @@ def test_sync_cloud_run_secrets_is_idempotent_and_grants_complete_batch(tmp_path CLOUD_RUN_PROJECT="policyengine-api", CLOUD_RUN_REGION="us-central1", CLOUD_RUN_SERVICE=PRODUCTION_CLOUD_RUN_SERVICE, - CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=( - "policyengine-api-prod-db-password:latest" - ), + CLOUD_RUN_POLICYENGINE_DB_PASSWORD_SECRET=PRODUCTION_DB_PASSWORD_SECRET, **secret_values, ) @@ -2420,25 +2410,13 @@ def test_hugging_face_credential_uses_purpose_specific_managed_names(): def test_sync_cloud_run_secrets_script_is_shell_syntax_valid(): - result = subprocess.run( - ["bash", "-n", ".github/scripts/sync_cloud_run_secrets.sh"], - cwd=REPO, - text=True, - capture_output=True, - check=False, - ) + result = _run_bash("-n", ".github/scripts/sync_cloud_run_secrets.sh") assert result.returncode == 0, result.stderr def test_changelog_fragment_script_is_shell_syntax_valid(): - result = subprocess.run( - ["bash", "-n", ".github/scripts/check_changelog_fragment.sh"], - cwd=REPO, - text=True, - capture_output=True, - check=False, - ) + result = _run_bash("-n", ".github/scripts/check_changelog_fragment.sh") assert result.returncode == 0, result.stderr @@ -2558,16 +2536,14 @@ def test_deploy_cloud_run_candidate_passes_user_auth_settings(): CLOUD_RUN_TAG="stage3-test", AUTH0_MANAGEMENT_DOMAIN="policyengine.us.auth0.com", AUTH0_MANAGEMENT_CLIENT_ID="management-client", - AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE=( - "projects/p/secrets/auth0-management/versions/latest" - ), + AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE=TEST_AUTH0_SECRET_RESOURCE, ), ) assert with_management.returncode == 0, with_management.stderr assert "AUTH0_MANAGEMENT_DOMAIN=policyengine.us.auth0.com" in with_management.stdout assert ( - "AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE=" - "projects/p/secrets/auth0-management/versions/latest" in with_management.stdout + f"AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE={TEST_AUTH0_SECRET_RESOURCE}" + in with_management.stdout ) @@ -2606,9 +2582,7 @@ def test_validate_cloud_run_deploy_env_keeps_auth0_management_out_of_staging(): management = { "AUTH0_MANAGEMENT_DOMAIN": "policyengine.us.auth0.com", "AUTH0_MANAGEMENT_CLIENT_ID": "management-client", - "AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE": ( - "projects/p/secrets/auth0-management/versions/latest" - ), + "AUTH0_MANAGEMENT_CLIENT_SECRET_RESOURCE": TEST_AUTH0_SECRET_RESOURCE, } staging = _script_env(**_staging_runtime_env(), **management) production = _script_env(**_required_runtime_env(), **management) diff --git a/tests/unit/test_observability_collector_verifier.py b/tests/unit/test_observability_collector_verifier.py index fce1d8c49..06d7669d1 100644 --- a/tests/unit/test_observability_collector_verifier.py +++ b/tests/unit/test_observability_collector_verifier.py @@ -11,6 +11,7 @@ ROOT = Path(__file__).parents[2] VERIFIER_PATH = ROOT / ".github" / "scripts" / "verify_observability_collector.py" +ACCESS_TOKEN = "access-token" def _load_verifier(): @@ -80,7 +81,7 @@ def Export(self, request, *, metadata, timeout): "identity-token", project_id="observability-project", metric_location="us-central1", - access_token="access-token", + access_token=ACCESS_TOKEN, timeout_seconds=3.0, delivery_timeout_seconds=9.0, ) @@ -117,7 +118,7 @@ def Export(self, request, *, metadata, timeout): "trace_id": probe.trace_id, "verification_id": probe.verification_id, "metric_location": "us-central1", - "access_token": "access-token", + "access_token": ACCESS_TOKEN, "started_at": probe.started_at, "timeout_seconds": 9.0, } @@ -150,7 +151,7 @@ def Export(self, *_args, **_kwargs): "identity-token", project_id="observability-project", metric_location="us-central1", - access_token="access-token", + access_token=ACCESS_TOKEN, ) @@ -173,7 +174,7 @@ def get_json(url: str, _access_token: str): "observability-project", "collector-verify-123", "us-central1", - "access-token", + ACCESS_TOKEN, 1_700_000_000.0, ) assert len(urls) == 2