From ed1f5a23ee7ff3f4b0a62459c2c551d348b9f0d6 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:15:56 +0400 Subject: [PATCH 01/10] Describe tables from PostgreSQL's catalog and list how two descriptions differ The persistence tests gain schema_comparison, the comparison the next commit runs between sim's table models and the tables the Public API's migration chain creates. describe_tables reads, for each table of the public schema, its columns (type, with an enum named as "enum "; whether they allow null; their default), its constraints (primary key, unique, foreign key, check and exclusion) by name with PostgreSQL's own definition, and its other indexes by name with their definition, and the values of each enum its columns use. schema_differences lists every way the models' tables differ from the real ones, in both directions; tables the models leave out are not compared. Both sides are read through the same catalog queries, so a type, default or definition is compared the way PostgreSQL itself writes it. Co-Authored-By: Claude Opus 5.5 --- .../tests/schema_comparison.py | 251 ++++++++++++++ .../tests/test_schema_comparison.py | 309 ++++++++++++++++++ 2 files changed, 560 insertions(+) create mode 100644 libs/policyengine-simulation-persistence/tests/schema_comparison.py create mode 100644 libs/policyengine-simulation-persistence/tests/test_schema_comparison.py diff --git a/libs/policyengine-simulation-persistence/tests/schema_comparison.py b/libs/policyengine-simulation-persistence/tests/schema_comparison.py new file mode 100644 index 000000000..3c559f905 --- /dev/null +++ b/libs/policyengine-simulation-persistence/tests/schema_comparison.py @@ -0,0 +1,251 @@ +"""Describe tables from PostgreSQL's catalog, and list how two descriptions differ. + +Sim's table models describe tables the Public API's migration chain creates. +The comparison builds the models' tables in one database and reads both that +database and one the chain built through the same catalog queries, so every +type, default and definition is written the way PostgreSQL itself writes it. + +``describe_tables`` reads, for each table: its columns (type, with an enum +named as ``enum ``; whether they allow null; their default), its +constraints (primary key, unique, foreign key, check and exclusion) by name +with PostgreSQL's definition, and its other indexes by name with their +definition; and the values of each enum its columns use. +``schema_differences`` lists every way the models' tables differ from the +real ones. Tables the models leave out are not compared. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from sqlalchemy import text + +if TYPE_CHECKING: + from collections.abc import Collection + + from sqlalchemy import Connection + +ENUM_PREFIX = "enum " + +_COLUMNS = text( + "SELECT c.relname AS table_name, a.attname AS column_name, " + "CASE WHEN t.typtype = 'e' THEN 'enum ' || t.typname " + "ELSE format_type(a.atttypid, a.atttypmod) END AS column_type, " + "NOT a.attnotnull AS nullable, " + "pg_get_expr(d.adbin, d.adrelid) AS column_default " + "FROM pg_attribute a " + "JOIN pg_class c ON c.oid = a.attrelid " + "JOIN pg_namespace n ON n.oid = c.relnamespace " + "JOIN pg_type t ON t.oid = a.atttypid " + "LEFT JOIN pg_attrdef d ON d.adrelid = a.attrelid AND d.adnum = a.attnum " + "WHERE n.nspname = 'public' AND c.relkind IN ('r', 'p') " + "AND a.attnum > 0 AND NOT a.attisdropped " + "AND c.relname = ANY(:table_names) " + "ORDER BY c.relname, a.attnum" +) +_CONSTRAINTS = text( + "SELECT c.relname AS table_name, con.conname AS constraint_name, " + "pg_get_constraintdef(con.oid) AS definition " + "FROM pg_constraint con " + "JOIN pg_class c ON c.oid = con.conrelid " + "JOIN pg_namespace n ON n.oid = c.relnamespace " + "WHERE n.nspname = 'public' AND con.contype IN ('p', 'u', 'f', 'c', 'x') " + "AND c.relname = ANY(:table_names) " + "ORDER BY c.relname, con.conname" +) +# Indexes that back a primary key, unique or exclusion constraint are compared +# as that constraint. +_INDEXES = text( + "SELECT t.relname AS table_name, i.relname AS index_name, " + "pg_get_indexdef(i.oid) AS definition " + "FROM pg_index x " + "JOIN pg_class i ON i.oid = x.indexrelid " + "JOIN pg_class t ON t.oid = x.indrelid " + "JOIN pg_namespace n ON n.oid = t.relnamespace " + "WHERE n.nspname = 'public' AND t.relname = ANY(:table_names) " + "AND NOT EXISTS (SELECT 1 FROM pg_constraint con " + "WHERE con.conindid = x.indexrelid AND con.contype IN ('p', 'u', 'x')) " + "ORDER BY t.relname, i.relname" +) +_ENUM_VALUES = text( + "SELECT t.typname AS enum_name, " + "array_agg(e.enumlabel::text ORDER BY e.enumsortorder) AS enum_values " + "FROM pg_enum e JOIN pg_type t ON t.oid = e.enumtypid " + "JOIN pg_namespace n ON n.oid = t.typnamespace " + "WHERE n.nspname = 'public' GROUP BY t.typname" +) + + +@dataclass(frozen=True) +class ColumnShape: + """A column's type, nullability and default, as PostgreSQL writes them.""" + + column_type: str + nullable: bool + default: str | None + + +@dataclass(frozen=True) +class TableShape: + """A table's columns, its constraints and its other indexes, by name.""" + + columns: dict[str, ColumnShape] + constraints: dict[str, str] + indexes: dict[str, str] + + +@dataclass(frozen=True) +class SchemaShape: + """Tables by name, and the values of each enum their columns use.""" + + tables: dict[str, TableShape] + enums: dict[str, tuple[str, ...]] + + +def describe_tables( + connection: Connection, table_names: Collection[str] +) -> SchemaShape: + """Describe these tables of the ``public`` schema from the database's catalog.""" + names = sorted(table_names) + parameters = {"table_names": names} + columns: dict[str, dict[str, ColumnShape]] = {} + for row in connection.execute(_COLUMNS, parameters).mappings(): + columns.setdefault(row["table_name"], {})[row["column_name"]] = ColumnShape( + column_type=row["column_type"], + nullable=row["nullable"], + default=row["column_default"], + ) + constraints: dict[str, dict[str, str]] = {} + for row in connection.execute(_CONSTRAINTS, parameters).mappings(): + constraints.setdefault(row["table_name"], {})[row["constraint_name"]] = row[ + "definition" + ] + indexes: dict[str, dict[str, str]] = {} + for row in connection.execute(_INDEXES, parameters).mappings(): + indexes.setdefault(row["table_name"], {})[row["index_name"]] = row["definition"] + used_enums = { + column.column_type.removeprefix(ENUM_PREFIX) + for table_columns in columns.values() + for column in table_columns.values() + if column.column_type.startswith(ENUM_PREFIX) + } + enums = { + row["enum_name"]: tuple(row["enum_values"]) + for row in connection.execute(_ENUM_VALUES).mappings() + if row["enum_name"] in used_enums + } + return SchemaShape( + tables={ + name: TableShape( + columns=columns[name], + constraints=constraints.get(name, {}), + indexes=indexes.get(name, {}), + ) + for name in names + if name in columns + }, + enums=enums, + ) + + +def _default_difference(model: str | None, real: str | None) -> str: + model_default = "has no default" if model is None else f"defaults to {model}" + real_default = "has no default" if real is None else f"defaults to {real}" + return f"the model {model_default}, the Public API's {real_default}" + + +def _column_differences( + table_name: str, models: TableShape, real: TableShape +) -> list[str]: + found = [] + for column_name, modelled in models.columns.items(): + actual = real.columns.get(column_name) + where = f"{table_name}.{column_name}" + if actual is None: + found.append(f"{where}: the Public API's table has no such column") + continue + if modelled.column_type != actual.column_type: + found.append( + f"{where}: the model is {modelled.column_type}, the Public API's " + f"is {actual.column_type}" + ) + if modelled.nullable != actual.nullable: + found.append( + f"{where}: the model " + f"{'allows' if modelled.nullable else 'refuses'} null, the Public " + f"API's {'allows' if actual.nullable else 'refuses'} it" + ) + if modelled.default != actual.default: + found.append( + f"{where}: {_default_difference(modelled.default, actual.default)}" + ) + found.extend( + f"{table_name}.{column_name}: the model has no such column" + for column_name in real.columns + if column_name not in models.columns + ) + return found + + +def _definition_differences( + table_name: str, + kind: str, + models: dict[str, str], + real: dict[str, str], +) -> list[str]: + found = [] + for name, modelled in models.items(): + actual = real.get(name) + if actual is None: + found.append( + f"{table_name}: the Public API's table has no {kind} {name} " + f"({modelled})" + ) + elif actual != modelled: + found.append( + f"{table_name}: {name} is {modelled} in the model, {actual} in " + "the Public API" + ) + found.extend( + f"{table_name}: the model has no {kind} {name} ({actual})" + for name, actual in real.items() + if name not in models + ) + return found + + +def schema_differences(models: SchemaShape, real: SchemaShape) -> list[str]: + """Return every way the models' tables differ from the real ones, or nothing.""" + found = [] + for table_name, modelled in models.tables.items(): + actual = real.tables.get(table_name) + if actual is None: + found.append(f"{table_name}: the Public API has no such table") + continue + found.extend(_column_differences(table_name, modelled, actual)) + found.extend( + _definition_differences( + table_name, "constraint", modelled.constraints, actual.constraints + ) + ) + found.extend( + _definition_differences( + table_name, "index", modelled.indexes, actual.indexes + ) + ) + for enum_name, modelled_values in models.enums.items(): + actual_values = real.enums.get(enum_name) + if actual_values is None: + # Enum columns are named without their schema, so an enum moved out + # of the public schema would otherwise pass unchecked. + found.append( + f"enum {enum_name}: the Public API has no such enum in its public " + "schema" + ) + elif actual_values != modelled_values: + found.append( + f"enum {enum_name}: the model has ({', '.join(modelled_values)}), " + f"the Public API has ({', '.join(actual_values)})" + ) + return found diff --git a/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py new file mode 100644 index 000000000..b5d27b8cb --- /dev/null +++ b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py @@ -0,0 +1,309 @@ +"""How sim's table models are compared with the Public API's tables. + +``schema_differences`` lists every way the models' tables differ from the +tables the Public API's migration chain builds. These tests need no database: +they compare descriptions built here, except the one marked ``postgres``, +which describes a table it creates. +""" + +from __future__ import annotations + +import pytest +from schema_comparison import ( + ColumnShape, + SchemaShape, + TableShape, + describe_tables, + schema_differences, +) +from sqlalchemy import create_engine, text +from sqlalchemy.engine import make_url + +STATUS_ENUM = "v2_simulation_status" + + +def _simulation_runs(**changes) -> SchemaShape: + """Return a simulation_runs table and its status enum, changed as given.""" + columns = { + "id": ColumnShape("uuid", nullable=False, default=None), + "simulation_id": ColumnShape("uuid", nullable=False, default=None), + "status": ColumnShape( + f"enum {STATUS_ENUM}", + nullable=False, + default=f"'pending'::{STATUS_ENUM}", + ), + "created_at": ColumnShape( + "timestamp with time zone", nullable=False, default="now()" + ), + "modal_call_id": ColumnShape( + "character varying(255)", nullable=True, default=None + ), + } + columns.update(changes.pop("columns", {})) + for removed in changes.pop("without_columns", ()): + del columns[removed] + constraints = { + "pk_simulation_runs": "PRIMARY KEY (id)", + "fk_simulation_runs_simulation_id_simulations": ( + "FOREIGN KEY (simulation_id) REFERENCES simulations(id) ON DELETE CASCADE" + ), + } + constraints.update(changes.pop("constraints", {})) + for removed in changes.pop("without_constraints", ()): + del constraints[removed] + indexes = { + "ix_simulation_runs_status_started_at": ( + "CREATE INDEX ix_simulation_runs_status_started_at ON " + "public.simulation_runs USING btree (status, created_at)" + ), + } + indexes.update(changes.pop("indexes", {})) + for removed in changes.pop("without_indexes", ()): + del indexes[removed] + enums = changes.pop( + "enums", {STATUS_ENUM: ("pending", "running", "succeeded", "failed")} + ) + assert not changes, changes + return SchemaShape( + tables={ + "simulation_runs": TableShape( + columns=columns, constraints=constraints, indexes=indexes + ) + }, + enums=enums, + ) + + +def test_identical_tables_have_no_differences() -> None: + assert schema_differences(_simulation_runs(), _simulation_runs()) == [] + + +@pytest.mark.parametrize( + ("models", "real", "difference"), + [ + ( + _simulation_runs( + columns={ + "status": ColumnShape( + "character varying(32)", + nullable=False, + default=f"'pending'::{STATUS_ENUM}", + ) + } + ), + _simulation_runs(), + "simulation_runs.status: the model is character varying(32), the " + "Public API's is enum v2_simulation_status", + ), + ( + _simulation_runs( + columns={ + "modal_call_id": ColumnShape( + "character varying(255)", nullable=False, default=None + ) + } + ), + _simulation_runs(), + "simulation_runs.modal_call_id: the model refuses null, the Public " + "API's allows it", + ), + ( + _simulation_runs( + columns={ + "created_at": ColumnShape( + "timestamp with time zone", nullable=False, default=None + ) + } + ), + _simulation_runs(), + "simulation_runs.created_at: the model has no default, the Public " + "API's defaults to now()", + ), + ( + _simulation_runs(without_columns=["modal_call_id"]), + _simulation_runs(), + "simulation_runs.modal_call_id: the model has no such column", + ), + ( + _simulation_runs(), + _simulation_runs(without_columns=["modal_call_id"]), + "simulation_runs.modal_call_id: the Public API's table has no such column", + ), + ( + _simulation_runs( + constraints={ + "fk_simulation_runs_simulation_id_simulations": ( + "FOREIGN KEY (simulation_id) REFERENCES simulations(id)" + ) + } + ), + _simulation_runs(), + "simulation_runs: fk_simulation_runs_simulation_id_simulations is " + "FOREIGN KEY (simulation_id) REFERENCES simulations(id) in the " + "model, FOREIGN KEY (simulation_id) REFERENCES simulations(id) ON " + "DELETE CASCADE in the Public API", + ), + ( + _simulation_runs( + without_constraints=["fk_simulation_runs_simulation_id_simulations"] + ), + _simulation_runs(), + "simulation_runs: the model has no constraint " + "fk_simulation_runs_simulation_id_simulations (FOREIGN KEY " + "(simulation_id) REFERENCES simulations(id) ON DELETE CASCADE)", + ), + ( + _simulation_runs(), + _simulation_runs( + without_constraints=["fk_simulation_runs_simulation_id_simulations"] + ), + "simulation_runs: the Public API's table has no constraint " + "fk_simulation_runs_simulation_id_simulations (FOREIGN KEY " + "(simulation_id) REFERENCES simulations(id) ON DELETE CASCADE)", + ), + ( + _simulation_runs(without_indexes=["ix_simulation_runs_status_started_at"]), + _simulation_runs(), + "simulation_runs: the model has no index " + "ix_simulation_runs_status_started_at (CREATE INDEX " + "ix_simulation_runs_status_started_at ON public.simulation_runs USING " + "btree (status, created_at))", + ), + ( + _simulation_runs(), + _simulation_runs(enums={STATUS_ENUM: ("pending", "running", "failed")}), + "enum v2_simulation_status: the model has (pending, running, " + "succeeded, failed), the Public API has (pending, running, failed)", + ), + ( + _simulation_runs(), + _simulation_runs(enums={}), + "enum v2_simulation_status: the Public API has no such enum in its " + "public schema", + ), + ( + _simulation_runs(), + SchemaShape(tables={}, enums=_simulation_runs().enums), + "simulation_runs: the Public API has no such table", + ), + ], + ids=[ + "type", + "nullability", + "default", + "column-only-in-the-public-api", + "column-only-in-the-model", + "constraint-definition", + "constraint-only-in-the-public-api", + "constraint-only-in-the-model", + "index-only-in-the-public-api", + "enum-values", + "missing-enum", + "missing-table", + ], +) +def test_each_difference_is_reported(models, real, difference) -> None: + assert schema_differences(models, real) == [difference] + + +def test_an_index_with_another_definition_is_reported() -> None: + reordered = ( + "CREATE INDEX ix_simulation_runs_status_started_at ON " + "public.simulation_runs USING btree (created_at, status)" + ) + models = _simulation_runs( + indexes={"ix_simulation_runs_status_started_at": reordered} + ) + + assert schema_differences(models, _simulation_runs()) == [ + f"simulation_runs: ix_simulation_runs_status_started_at is {reordered} in " + "the model, CREATE INDEX ix_simulation_runs_status_started_at ON " + "public.simulation_runs USING btree (status, created_at) in the Public API", + ] + + +def test_tables_the_models_leave_out_are_not_compared() -> None: + real = _simulation_runs() + real.tables["users"] = TableShape( + columns={"id": ColumnShape("uuid", nullable=False, default=None)}, + constraints={}, + indexes={}, + ) + + assert schema_differences(_simulation_runs(), real) == [] + + +@pytest.mark.postgres +def test_the_catalog_describes_each_part_of_a_table(disposable_database) -> None: + engine = create_engine( + make_url(disposable_database.superuser_url).set( + drivername="postgresql+psycopg" + ), + isolation_level="AUTOCOMMIT", + ) + statements = [ + "CREATE TYPE v2_simulation_status AS ENUM ('pending', 'running')", + "CREATE TABLE simulations (id uuid CONSTRAINT pk_simulations PRIMARY KEY)", + "CREATE TABLE simulation_runs (" + "id uuid CONSTRAINT pk_simulation_runs PRIMARY KEY, " + "simulation_id uuid NOT NULL CONSTRAINT " + "fk_simulation_runs_simulation_id_simulations REFERENCES simulations (id) " + "ON DELETE CASCADE, " + "status v2_simulation_status NOT NULL DEFAULT 'pending', " + "attempt integer NOT NULL CONSTRAINT ck_simulation_runs_attempt " + "CHECK (attempt > 0), " + "created_at timestamp with time zone NOT NULL DEFAULT now(), " + "modal_call_id varchar(255), " + "CONSTRAINT uq_simulation_runs_simulation_attempt " + "UNIQUE (simulation_id, attempt))", + "CREATE INDEX ix_simulation_runs_status_created_at " + "ON simulation_runs (status, created_at)", + ] + try: + with engine.connect() as connection: + for statement in statements: + connection.execute(text(statement)) + described = describe_tables(connection, ["simulation_runs", "absent"]) + finally: + engine.dispose() + + assert described == SchemaShape( + tables={ + "simulation_runs": TableShape( + columns={ + "id": ColumnShape("uuid", nullable=False, default=None), + "simulation_id": ColumnShape("uuid", nullable=False, default=None), + "status": ColumnShape( + "enum v2_simulation_status", + nullable=False, + default="'pending'::v2_simulation_status", + ), + "attempt": ColumnShape("integer", nullable=False, default=None), + "created_at": ColumnShape( + "timestamp with time zone", nullable=False, default="now()" + ), + "modal_call_id": ColumnShape( + "character varying(255)", nullable=True, default=None + ), + }, + constraints={ + "ck_simulation_runs_attempt": "CHECK ((attempt > 0))", + "fk_simulation_runs_simulation_id_simulations": ( + "FOREIGN KEY (simulation_id) REFERENCES simulations(id) " + "ON DELETE CASCADE" + ), + "pk_simulation_runs": "PRIMARY KEY (id)", + "uq_simulation_runs_simulation_attempt": ( + "UNIQUE (simulation_id, attempt)" + ), + }, + indexes={ + "ix_simulation_runs_status_created_at": ( + "CREATE INDEX ix_simulation_runs_status_created_at ON " + "public.simulation_runs USING btree (status, created_at)" + ), + }, + ) + }, + enums={"v2_simulation_status": ("pending", "running")}, + ) From b5fc3d46a1a0595b9da267fc59660bd185ac4b72 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:31:25 +0400 Subject: [PATCH 02/10] Compare sim's table models with the Public API's tables at a pinned commit in CI A new PR job, Public API schema, checks out PolicyEngine/policyengine-api (public, so without a secret or persisted credentials) at the commit pinned in libs/policyengine-simulation-persistence/public-api-commit, builds its tables with its own v2 migration chain into the job's PostgreSQL and grants the privileges its manifest declares (public-api-schema-reference.sh), and runs the persistence tests marked public_api_schema, which fail rather than skip there. test_public_api_schema builds the models' tables from service_metadata in a disposable database and compares both through the catalog: every column's type, nullability and default, every constraint and index by name and definition, and every enum's values. The role-write tests run in the same job. Last, the job requires the pinned commit to be on the Public API's master branch, so the models only describe a merged chain. CI complete needs the job. A sim pull request that changes the models moves the pin with them. The new Public API schema drift workflow runs the same comparison every Monday and on demand against the head of the Public API's master, so a merged change there to a modelled table shows up without waiting for a sim change. The new rule sim.persistence.models-match-public-api records this, with a proof whose bad models copy simulation_runs.status as a varchar and leave out modal_call_id. Every script the jobs call is tested with fake commands. Co-Authored-By: Claude Opus 5.5 --- .../public-api-commit-on-default-branch.sh | 18 ++ .github/scripts/public-api-schema-commit.sh | 21 ++ .../scripts/public-api-schema-reference.sh | 33 +++ .github/workflows/pr.yml | 84 ++++++- .github/workflows/public-api-schema-drift.yml | 81 ++++++ AGENTS.md | 2 +- .../b2-public-api-schema-check.added.md | 1 + docs/generated/rules.md | 12 +- .../README.md | 38 +++ .../public-api-commit | 1 + .../tests/schema_comparison.py | 28 ++- .../tests/test_public_api_schema.py | 80 ++++++ .../tests/test_public_api_schema_job.py | 134 ++++++++++ .../tests/test_public_api_schema_scripts.py | 238 ++++++++++++++++++ .../tests/test_schema_comparison.py | 10 + .../bad_models.json | 100 ++++++++ .../good_models.json | 112 +++++++++ .../public_api_schema.json | 112 +++++++++ rules/registry.toml | 37 +++ 19 files changed, 1137 insertions(+), 5 deletions(-) create mode 100755 .github/scripts/public-api-commit-on-default-branch.sh create mode 100755 .github/scripts/public-api-schema-commit.sh create mode 100755 .github/scripts/public-api-schema-reference.sh create mode 100644 .github/workflows/public-api-schema-drift.yml create mode 100644 changelog.d/b2-public-api-schema-check.added.md create mode 100644 libs/policyengine-simulation-persistence/public-api-commit create mode 100644 libs/policyengine-simulation-persistence/tests/test_public_api_schema.py create mode 100644 libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py create mode 100644 libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py create mode 100644 rules/fixtures/sim.persistence.models-match-public-api/bad_models.json create mode 100644 rules/fixtures/sim.persistence.models-match-public-api/good_models.json create mode 100644 rules/fixtures/sim.persistence.models-match-public-api/public_api_schema.json diff --git a/.github/scripts/public-api-commit-on-default-branch.sh b/.github/scripts/public-api-commit-on-default-branch.sh new file mode 100755 index 000000000..6b13911e4 --- /dev/null +++ b/.github/scripts/public-api-commit-on-default-branch.sh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +# Require the checked-out Public API commit to be on the Public API's default +# branch, so sim's table models only ever describe a migration chain the +# Public API has merged (rule sim.persistence.models-match-public-api). +# +# Usage: public-api-commit-on-default-branch.sh +set -euo pipefail + +checkout="${1:?usage: public-api-commit-on-default-branch.sh }" +branch="${2:?usage: public-api-commit-on-default-branch.sh }" +commit="$(git -C "${checkout}" rev-parse HEAD)" +git -C "${checkout}" fetch --quiet origin "${branch}" +if ! git -C "${checkout}" merge-base --is-ancestor "${commit}" FETCH_HEAD; then + printf '%s is not on the Public API'"'"'s %s branch yet; pin a merged commit\n' \ + "${commit}" "${branch}" >&2 + exit 1 +fi +printf '%s is on the Public API'"'"'s %s branch\n' "${commit}" "${branch}" diff --git a/.github/scripts/public-api-schema-commit.sh b/.github/scripts/public-api-schema-commit.sh new file mode 100755 index 000000000..02a592bc3 --- /dev/null +++ b/.github/scripts/public-api-schema-commit.sh @@ -0,0 +1,21 @@ +#!/usr/bin/env bash +# Read the pinned Public API commit whose v2 migration chain sim's table +# models describe (rule sim.persistence.models-match-public-api). +# +# Usage: public-api-schema-commit.sh +# +# The pin file holds one full commit hash. The script prints it and, on +# GitHub Actions, passes it to the later steps as the step output `commit`. +set -euo pipefail + +pin_file="${1:?usage: public-api-schema-commit.sh }" +commit="$(tr -d '[:space:]' < "${pin_file}")" +if ! [[ "${commit}" =~ ^[0-9a-f]{40}$ ]]; then + printf '%s must hold one Public API commit as 40 lowercase hexadecimal characters\n' \ + "${pin_file}" >&2 + exit 1 +fi +printf 'Pinned Public API commit: %s\n' "${commit}" +if [ -n "${GITHUB_OUTPUT:-}" ]; then + printf 'commit=%s\n' "${commit}" >> "${GITHUB_OUTPUT}" +fi diff --git a/.github/scripts/public-api-schema-reference.sh b/.github/scripts/public-api-schema-reference.sh new file mode 100755 index 000000000..dd8366bf4 --- /dev/null +++ b/.github/scripts/public-api-schema-reference.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +# Build the Public API's tables with its own v2 migration chain, and grant the +# table privileges its manifest declares, for the comparison with sim's table +# models (rule sim.persistence.models-match-public-api). +# +# Usage: public-api-schema-reference.sh +# +# PUBLIC_API_SCHEMA_DATABASE_URL names the empty database to build, as +# postgresql://user:password@host:port/database. The Public API's disposable +# migration mode accepts only the local policyengine_v2_alembic_test database, +# with explicit credentials, and creates the roles its manifest manages +# without a login. The script prints the Public API commit it built and, on +# GitHub Actions, passes it to the later steps as PUBLIC_API_SCHEMA_COMMIT, so +# a failed comparison names it. +set -euo pipefail + +checkout="${1:?usage: public-api-schema-reference.sh }" +database_url="${PUBLIC_API_SCHEMA_DATABASE_URL:?PUBLIC_API_SCHEMA_DATABASE_URL must name the database to build}" + +cd "${checkout}" +commit="$(git rev-parse HEAD)" +printf 'Public API commit: %s\n' "${commit}" +if [ -n "${GITHUB_ENV:-}" ]; then + printf 'PUBLIC_API_SCHEMA_COMMIT=%s\n' "${commit}" >> "${GITHUB_ENV}" +fi +# The Public API installs its lock on Python 3.12, as its own CI does. +export UV_PYTHON=3.12 +export V2_MIGRATION_DATABASE_URL="postgresql+psycopg://${database_url#*://}" +export V2_ALEMBIC_DISPOSABLE_TEST=1 +export DEPLOYMENT_ENVIRONMENT=ci +uv sync --frozen --extra dev +uv run --frozen --extra dev alembic -c alembic-v2.ini upgrade head +uv run --frozen --extra dev python scripts/reconcile_v2_privileges.py apply --create-missing-roles diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index f828253a2..a8c132e6e 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -16,7 +16,7 @@ jobs: # every job passed. See docs/engineering/skills/github-prs.md. name: CI complete if: ${{ always() }} - needs: [test, simulation-database, lint, pyright, quality-guards, check-changelog, docker-build, test-integration] + needs: [test, simulation-database, public-api-schema, lint, pyright, quality-guards, check-changelog, docker-build, test-integration] runs-on: ubuntu-latest permissions: contents: read @@ -127,6 +127,88 @@ jobs: working-directory: libs/policyengine-simulation-persistence run: uv run --frozen pytest tests/ -v -m postgres + public-api-schema: + name: Public API schema + # Builds the Public API's tables with its own v2 migration chain, at the + # commit pinned in libs/policyengine-simulation-persistence/public-api-commit, + # grants the privileges its manifest declares, and runs the persistence + # tests marked public_api_schema against them: sim's table models must + # match those tables, and the simulation service's role must write them + # (rule sim.persistence.models-match-public-api). The pinned commit must + # also be on the Public API's default branch. The Public API's repository + # is public, so the job reads no secret. Full CI runs only on the lowest + # unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} + runs-on: ubuntu-latest + permissions: + contents: read + services: + postgres: + image: postgres:17 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: simulation_ci + # The Public API's disposable migration mode accepts only this + # database; the server's postgres database holds the models' tables. + POSTGRES_DB: policyengine_v2_alembic_test + ports: + - 5432:5432 + options: >- + --health-cmd="pg_isready -U postgres" + --health-interval=5s + --health-timeout=5s + --health-retries=20 + env: + SIMULATION_TEST_DATABASE_URL: postgresql://postgres:simulation_ci@127.0.0.1:5432/postgres + PUBLIC_API_SCHEMA_DATABASE_URL: postgresql://postgres:simulation_ci@127.0.0.1:5432/policyengine_v2_alembic_test + # A missing server or Public API database fails the tests instead of + # skipping them. + SIMULATION_REQUIRE_DATABASE_TESTS: "1" + SIMULATION_REQUIRE_PUBLIC_API_SCHEMA: "1" + + steps: + - uses: actions/checkout@v6 + + - name: Read the pinned Public API commit + id: public-api + run: .github/scripts/public-api-schema-commit.sh libs/policyengine-simulation-persistence/public-api-commit + + - name: Check out the pinned Public API commit + uses: actions/checkout@v6 + with: + repository: PolicyEngine/policyengine-api + ref: ${{ steps.public-api.outputs.commit }} + path: public-api + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@v6 + with: + # 3.12 for the Public API's lock, as its own CI uses; 3.13 for sim's. + python-version: | + 3.12 + 3.13 + + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + with: + version: "0.13.6" + enable-cache: true + + - name: Build the Public API's tables at the pinned commit + run: .github/scripts/public-api-schema-reference.sh public-api + + - name: Install dependencies + working-directory: libs/policyengine-simulation-persistence + run: uv sync --extra test --frozen + + - name: Compare sim's table models with the Public API's tables + working-directory: libs/policyengine-simulation-persistence + run: uv run --frozen pytest tests/ -v -m public_api_schema + + - name: Require the pinned commit on the Public API's default branch + run: .github/scripts/public-api-commit-on-default-branch.sh public-api master + lint: name: Lint # Full CI runs only on the lowest unmerged pull request in a stack. diff --git a/.github/workflows/public-api-schema-drift.yml b/.github/workflows/public-api-schema-drift.yml new file mode 100644 index 000000000..9da571d81 --- /dev/null +++ b/.github/workflows/public-api-schema-drift.yml @@ -0,0 +1,81 @@ +name: Public API schema drift + +# Compares sim's table models with the tables the Public API's v2 migration +# chain builds at the head of its default branch, every Monday and on demand. +# Pull requests compare them at the pinned commit instead (the PR workflow's +# public-api-schema job); a failure here means the Public API has merged a +# change to a table the simulation service writes, so the models and the pin +# need to follow it (rule sim.persistence.models-match-public-api). The +# Public API's repository is public, so the workflow reads no secret. + +on: + schedule: + - cron: "17 6 * * 1" + workflow_dispatch: + +permissions: + contents: read + +jobs: + public-api-schema-drift: + name: Compare the models with the Public API's default branch + runs-on: ubuntu-latest + services: + postgres: + image: postgres:17 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: simulation_ci + # The Public API's disposable migration mode accepts only this + # database; the server's postgres database holds the models' tables. + POSTGRES_DB: policyengine_v2_alembic_test + ports: + - 5432:5432 + options: >- + --health-cmd="pg_isready -U postgres" + --health-interval=5s + --health-timeout=5s + --health-retries=20 + env: + SIMULATION_TEST_DATABASE_URL: postgresql://postgres:simulation_ci@127.0.0.1:5432/postgres + PUBLIC_API_SCHEMA_DATABASE_URL: postgresql://postgres:simulation_ci@127.0.0.1:5432/policyengine_v2_alembic_test + # A missing server or Public API database fails the tests instead of + # skipping them. + SIMULATION_REQUIRE_DATABASE_TESTS: "1" + SIMULATION_REQUIRE_PUBLIC_API_SCHEMA: "1" + + steps: + - uses: actions/checkout@v6 + + - name: Check out the Public API's default branch + uses: actions/checkout@v6 + with: + repository: PolicyEngine/policyengine-api + ref: master + path: public-api + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@v6 + with: + # 3.12 for the Public API's lock, as its own CI uses; 3.13 for sim's. + python-version: | + 3.12 + 3.13 + + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + with: + version: "0.13.6" + enable-cache: true + + - name: Build the Public API's tables at its default branch + run: .github/scripts/public-api-schema-reference.sh public-api + + - name: Install dependencies + working-directory: libs/policyengine-simulation-persistence + run: uv sync --extra test --frozen + + - name: Compare sim's table models with the Public API's tables + working-directory: libs/policyengine-simulation-persistence + run: uv run --frozen pytest tests/ -v -m public_api_schema diff --git a/AGENTS.md b/AGENTS.md index cc4a77446..74b9f523d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -133,7 +133,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Simulation service database -- Checked: `sim.persistence.missing-privileges-reported` +- Checked: `sim.persistence.missing-privileges-reported`, `sim.persistence.models-match-public-api` ### Deployment pipeline diff --git a/changelog.d/b2-public-api-schema-check.added.md b/changelog.d/b2-public-api-schema-check.added.md new file mode 100644 index 000000000..c8015f316 --- /dev/null +++ b/changelog.d/b2-public-api-schema-check.added.md @@ -0,0 +1 @@ +CI checks sim's models of the tables the simulation service writes against the tables the Public API's migration chain builds at a pinned Public API commit, and that the service's role can write them; a weekly run compares them with the Public API's default branch. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index fbc62d4ed..6364ed150 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 58 | 2 | 0 | 14 | 74 | +| mechanical | 59 | 2 | 0 | 14 | 75 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 0 | 0 | -**Blocking:** 61 of 75 rules. +**Blocking:** 62 of 76 rules. ## Gaps @@ -141,14 +141,22 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `sim.persistence.missing-privileges-reported` | The simulation service's privilege check, which its readiness check reports, names every table privilege its role needs on the Public API's tables and lacks, as PostgreSQL reports it, counts a table missing from the public schema as lacking all of them, and names a missing USAGE privilege on that schema. | mechanical | enforced | yes | pytest `libs/policyengine-simulation-persistence/tests/test_access.py` | `pr.yml:simulation-database` | yes | +| `sim.persistence.models-match-public-api` | Sim's models of the Public API's tables match the tables the Public API's v2 migration chain builds at the pinned Public API commit, which is on the Public API's default branch: every column's type, nullability and default, every constraint and index by name and definition, and every enum's values; and the simulation service's role, with the privileges the Public API's manifest declares, writes every modelled table. | mechanical | enforced | yes | pytest `libs/policyengine-simulation-persistence/tests/test_public_api_schema.py`, `libs/policyengine-simulation-persistence/tests/test_service_writes.py` | `pr.yml:public-api-schema`, `public-api-schema-drift.yml:public-api-schema-drift` | yes | ```mermaid flowchart LR r_sim_persistence_missing_privileges_reported("sim.persistence.missing-privileges-reported (enforced)") e_pytest_libs_policyengine_simulation_persistence_tests_test_access_py["pytest libs/policyengine-simulation-persistence/tests/test_access.py"] j_CI_pr_yml_simulation_database["CI pr.yml:simulation-database"] + r_sim_persistence_models_match_public_api("sim.persistence.models-match-public-api (enforced)") + e_pytest_libs_policyengine_simulation_persistence_tests_test_public_api_schema_py__libs_policyengine_simulation_persistence_tests_test_service_writes_py["pytest libs/policyengine-simulation-persistence/tests/test_public_api_schema.py, libs/policyengine-simulation-persistence/tests/test_service_writes.py"] + j_CI_pr_yml_public_api_schema["CI pr.yml:public-api-schema"] + j_CI_public_api_schema_drift_yml_public_api_schema_drift["CI public-api-schema-drift.yml:public-api-schema-drift"] r_sim_persistence_missing_privileges_reported --> e_pytest_libs_policyengine_simulation_persistence_tests_test_access_py e_pytest_libs_policyengine_simulation_persistence_tests_test_access_py --> j_CI_pr_yml_simulation_database + r_sim_persistence_models_match_public_api --> e_pytest_libs_policyengine_simulation_persistence_tests_test_public_api_schema_py__libs_policyengine_simulation_persistence_tests_test_service_writes_py + e_pytest_libs_policyengine_simulation_persistence_tests_test_public_api_schema_py__libs_policyengine_simulation_persistence_tests_test_service_writes_py --> j_CI_pr_yml_public_api_schema + e_pytest_libs_policyengine_simulation_persistence_tests_test_public_api_schema_py__libs_policyengine_simulation_persistence_tests_test_service_writes_py --> j_CI_public_api_schema_drift_yml_public_api_schema_drift ``` ## Deployment pipeline diff --git a/libs/policyengine-simulation-persistence/README.md b/libs/policyengine-simulation-persistence/README.md index 934246c69..f53e96c20 100644 --- a/libs/policyengine-simulation-persistence/README.md +++ b/libs/policyengine-simulation-persistence/README.md @@ -32,6 +32,44 @@ between these tables, because another service writes the policy, household and catalog rows they name. Status and other enum columns use the `StrEnum` classes beside them and store each member's value. +## Checking the models against the Public API + +Rule `sim.persistence.models-match-public-api`. The PR workflow's +`Public API schema` job checks out the Public API at the commit pinned in +`public-api-commit`, builds its tables with its own v2 migration chain and +grants the privileges its manifest declares +(`.github/scripts/public-api-schema-reference.sh`), then runs the tests +marked `public_api_schema`. `test_public_api_schema` builds the models' tables +from `service_metadata` in a disposable database and compares both through +PostgreSQL's catalog (`tests/schema_comparison.py`): every column's type, +nullability and default, every constraint and index by name and definition, +and every enum's values must match. `test_service_writes` creates a row in +every modelled table as the service's role. Last, the job requires the +pinned commit to be on the Public API's `master` branch, so the models only +ever describe a chain the Public API has merged. + +**Choosing the pin.** Pin the Public API commit whose migration chain the +models describe: the merge commit of the Public API change that last +altered a modelled table, or, while that change is still a pull request, its +head commit. Until that pull request merges, the job's last step fails and +says so. + +**Keeping it current.** A sim pull request that changes the models moves the +pin in the same change. The `Public API schema drift` workflow runs the same +comparison every Monday, and on demand, against the head of the Public +API's `master`; it fails when the Public API has merged a change to a +modelled table that the models and the pin haven't followed yet. + +To run the comparison locally, build the reference with the script (it +needs an empty `policyengine_v2_alembic_test` database and a checkout of the +Public API at the pinned commit), then: + +```bash +SIMULATION_TEST_DATABASE_URL=postgresql://postgres:password@127.0.0.1:5432/postgres \ +PUBLIC_API_SCHEMA_DATABASE_URL=postgresql://postgres:password@127.0.0.1:5432/policyengine_v2_alembic_test \ +uv run pytest tests/ -m public_api_schema +``` + ## The role's privileges `SERVICE_TABLE_PRIVILEGES` lists the table privileges the service's role diff --git a/libs/policyengine-simulation-persistence/public-api-commit b/libs/policyengine-simulation-persistence/public-api-commit new file mode 100644 index 000000000..0832b932c --- /dev/null +++ b/libs/policyengine-simulation-persistence/public-api-commit @@ -0,0 +1 @@ +dc617db3e961bfff3ff43e889687666245227cea diff --git a/libs/policyengine-simulation-persistence/tests/schema_comparison.py b/libs/policyengine-simulation-persistence/tests/schema_comparison.py index 3c559f905..081dabb9d 100644 --- a/libs/policyengine-simulation-persistence/tests/schema_comparison.py +++ b/libs/policyengine-simulation-persistence/tests/schema_comparison.py @@ -16,13 +16,15 @@ from __future__ import annotations -from dataclasses import dataclass +import json +from dataclasses import asdict, dataclass from typing import TYPE_CHECKING from sqlalchemy import text if TYPE_CHECKING: from collections.abc import Collection + from pathlib import Path from sqlalchemy import Connection @@ -249,3 +251,27 @@ def schema_differences(models: SchemaShape, real: SchemaShape) -> list[str]: f"the Public API has ({', '.join(actual_values)})" ) return found + + +def schema_to_json(schema: SchemaShape) -> str: + """Write a description as JSON, as the rule's proof fixtures store it.""" + return json.dumps(asdict(schema), indent=2, sort_keys=True) + "\n" + + +def schema_from_json(path: Path) -> SchemaShape: + """Read a description that ``schema_to_json`` wrote.""" + payload = json.loads(path.read_text(encoding="utf-8")) + return SchemaShape( + tables={ + name: TableShape( + columns={ + column_name: ColumnShape(**column) + for column_name, column in table["columns"].items() + }, + constraints=dict(table["constraints"]), + indexes=dict(table["indexes"]), + ) + for name, table in payload["tables"].items() + }, + enums={name: tuple(values) for name, values in payload["enums"].items()}, + ) diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py new file mode 100644 index 000000000..b4ffc721c --- /dev/null +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py @@ -0,0 +1,80 @@ +"""Sim's table models match the tables the Public API's migration chain builds. + +Rule sim.persistence.models-match-public-api. ``PUBLIC_API_SCHEMA_DATABASE_URL`` +names a database the Public API's v2 migration chain built at the pinned +commit (the ``public-api-schema`` job builds it). The models' own tables are +built from ``service_metadata`` in a disposable database, and both are read +through the same catalog queries. Without the Public API's database the +comparison skips, unless ``SIMULATION_REQUIRE_PUBLIC_API_SCHEMA=1`` (set in +that job), where it fails instead. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest +from policyengine_simulation_persistence import service_metadata +from schema_comparison import describe_tables, schema_differences, schema_from_json +from sqlalchemy import create_engine +from sqlalchemy.engine import make_url + +# The reference script sets it on GitHub Actions: the Public API commit built. +COMMIT_VARIABLE = "PUBLIC_API_SCHEMA_COMMIT" +PROOF_FIXTURES = ( + Path(__file__).resolve().parents[3] + / "rules" + / "fixtures" + / "sim.persistence.models-match-public-api" +) + + +def _engine(database_url: str): + return create_engine(make_url(database_url).set(drivername="postgresql+psycopg")) + + +@pytest.mark.postgres +@pytest.mark.public_api_schema +def test_the_models_match_the_public_api_tables( + disposable_database, public_api_database_url +) -> None: + models_engine = _engine(disposable_database.superuser_url) + real_engine = _engine(public_api_database_url) + try: + # Tests may build the models' tables; runtime code never does. + service_metadata.create_all(models_engine) + with models_engine.connect() as connection: + models = describe_tables(connection, service_metadata.tables) + with real_engine.connect() as connection: + real = describe_tables(connection, service_metadata.tables) + finally: + models_engine.dispose() + real_engine.dispose() + + assert set(models.tables) == set(service_metadata.tables) + compared_with = os.environ.get(COMMIT_VARIABLE) or "an unrecorded commit" + assert schema_differences(models, real) == [], ( + f"compared with the Public API at {compared_with}" + ) + + +def test_rule_proof_models_match_public_api() -> None: + """Proof for rule sim.persistence.models-match-public-api. + + ``public_api_schema.json`` describes ``simulation_runs`` as the Public + API's chain builds it. The bad models copy ``status`` as + ``character varying(32)``, where the Public API's column is the enum + ``v2_simulation_status``, and leave out ``modal_call_id``, which a later + revision added. + """ + real = schema_from_json(PROOF_FIXTURES / "public_api_schema.json") + bad = schema_from_json(PROOF_FIXTURES / "bad_models.json") + good = schema_from_json(PROOF_FIXTURES / "good_models.json") + + assert schema_differences(bad, real) == [ + "simulation_runs.status: the model is character varying(32), the Public " + "API's is enum v2_simulation_status", + "simulation_runs.modal_call_id: the model has no such column", + ] + assert schema_differences(good, real) == [] diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py new file mode 100644 index 000000000..3dd3ebe37 --- /dev/null +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -0,0 +1,134 @@ +"""The PR job that compares sim's table models with the Public API's tables. + +Rule sim.persistence.models-match-public-api. The job builds the Public +API's tables with its own v2 migration chain at the pinned commit, runs the +tests marked ``public_api_schema`` against them, and requires the pinned +commit to be on the Public API's default branch. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +PR_WORKFLOW = REPOSITORY_ROOT / ".github" / "workflows" / "pr.yml" +JOB_ID = "public-api-schema" +PIN_FILE = "libs/policyengine-simulation-persistence/public-api-commit" +DRIFT_WORKFLOW = ( + REPOSITORY_ROOT / ".github" / "workflows" / "public-api-schema-drift.yml" +) +DRIFT_JOB_ID = "public-api-schema-drift" +STACK_CONDITION = ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }}" +) + + +def _jobs() -> dict: + return yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8"))["jobs"] + + +def _step(job: dict, name: str) -> dict: + return next(step for step in job["steps"] if step.get("name") == name) + + +def test_the_job_checks_out_the_pinned_public_api_commit() -> None: + job = _jobs()[JOB_ID] + pin = _step(job, "Read the pinned Public API commit") + checkout = _step(job, "Check out the pinned Public API commit") + + assert pin["run"] == f".github/scripts/public-api-schema-commit.sh {PIN_FILE}" + assert checkout["uses"] == "actions/checkout@v6" + assert checkout["with"] == { + "repository": "PolicyEngine/policyengine-api", + "ref": f"${{{{ steps.{pin['id']}.outputs.commit }}}}", + "path": "public-api", + "persist-credentials": False, + } + + +def test_the_job_builds_the_reference_and_compares_without_skipping() -> None: + job = _jobs()[JOB_ID] + server = job["services"]["postgres"] + password = server["env"]["POSTGRES_PASSWORD"] + + assert server["image"] == "postgres:17" + # The Public API's disposable migration mode accepts only this database. + assert server["env"]["POSTGRES_DB"] == "policyengine_v2_alembic_test" + assert job["env"] == { + "SIMULATION_TEST_DATABASE_URL": ( + f"postgresql://postgres:{password}@127.0.0.1:5432/postgres" + ), + "PUBLIC_API_SCHEMA_DATABASE_URL": ( + f"postgresql://postgres:{password}@127.0.0.1:5432/" + "policyengine_v2_alembic_test" + ), + "SIMULATION_REQUIRE_DATABASE_TESTS": "1", + "SIMULATION_REQUIRE_PUBLIC_API_SCHEMA": "1", + } + names = [step.get("name") for step in job["steps"]] + build = _step(job, "Build the Public API's tables at the pinned commit") + compare = _step(job, "Compare sim's table models with the Public API's tables") + on_default_branch = _step( + job, "Require the pinned commit on the Public API's default branch" + ) + + assert build["run"] == ".github/scripts/public-api-schema-reference.sh public-api" + assert compare["working-directory"] == "libs/policyengine-simulation-persistence" + assert compare["run"] == "uv run --frozen pytest tests/ -v -m public_api_schema" + assert on_default_branch["run"] == ( + ".github/scripts/public-api-commit-on-default-branch.sh public-api master" + ) + assert ( + names.index(build["name"]) + < names.index(compare["name"]) + < names.index(on_default_branch["name"]) + ) + + +def test_the_job_reads_no_secret() -> None: + job_text = yaml.safe_dump(_jobs()[JOB_ID]) + + assert "secrets." not in job_text + assert _jobs()[JOB_ID]["permissions"] == {"contents": "read"} + + +def test_ci_complete_needs_the_job_at_the_bottom_of_a_stack() -> None: + jobs = _jobs() + + assert JOB_ID in jobs["ci-complete"]["needs"] + assert jobs[JOB_ID]["if"] == STACK_CONDITION + + +def _drift_workflow() -> dict: + return yaml.safe_load(DRIFT_WORKFLOW.read_text(encoding="utf-8")) + + +def test_a_weekly_run_compares_the_models_with_the_default_branch() -> None: + workflow = _drift_workflow() + # PyYAML reads the bare key `on` as True. + triggers = workflow[True] + job = workflow["jobs"][DRIFT_JOB_ID] + checkout = _step(job, "Check out the Public API's default branch") + compare = _step(job, "Compare sim's table models with the Public API's tables") + + assert [entry["cron"] for entry in triggers["schedule"]] == ["17 6 * * 1"] + assert "workflow_dispatch" in triggers + assert checkout["with"] == { + "repository": "PolicyEngine/policyengine-api", + "ref": "master", + "path": "public-api", + "persist-credentials": False, + } + assert compare["run"] == "uv run --frozen pytest tests/ -v -m public_api_schema" + assert job["env"] == _jobs()[JOB_ID]["env"] + assert job["services"] == _jobs()[JOB_ID]["services"] + + +def test_the_weekly_run_reads_no_secret() -> None: + workflow = _drift_workflow() + + assert "secrets." not in DRIFT_WORKFLOW.read_text(encoding="utf-8") + assert workflow["permissions"] == {"contents": "read"} diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py new file mode 100644 index 000000000..8a1b4d35a --- /dev/null +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py @@ -0,0 +1,238 @@ +"""The scripts the Public API schema job runs, with fake commands on PATH. + +``public-api-schema-commit.sh`` reads the pinned Public API commit, +``public-api-commit-on-default-branch.sh`` requires it to be on the Public +API's default branch, and ``public-api-schema-reference.sh`` builds the +Public API's tables at that commit and grants its declared privileges. +""" + +from __future__ import annotations + +import os +import subprocess +import textwrap +from pathlib import Path + +import pytest + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +SCRIPTS = REPOSITORY_ROOT / ".github" / "scripts" +COMMIT_SCRIPT = SCRIPTS / "public-api-schema-commit.sh" +DEFAULT_BRANCH_SCRIPT = SCRIPTS / "public-api-commit-on-default-branch.sh" +REFERENCE_SCRIPT = SCRIPTS / "public-api-schema-reference.sh" +PINNED_COMMIT = "0123456789abcdef0123456789abcdef01234567" +DATABASE_URL = ( + "postgresql://postgres:simulation_ci@127.0.0.1:5432/policyengine_v2_alembic_test" +) + + +def _fake_command(directory: Path, name: str, body: str) -> None: + """Write an executable that logs its arguments and runs ``body``.""" + command = directory / name + command.write_text( + textwrap.dedent( + f"""\ + #!/usr/bin/env bash + set -euo pipefail + printf '%s|%s\\n' "{name}" "$*" >> "${{FAKE_COMMAND_LOG}}" + {body} + """ + ), + encoding="utf-8", + ) + command.chmod(0o755) + + +def _run(script: Path, arguments: list[str], tmp_path: Path, **environment): + fake_bin = tmp_path / "bin" + fake_bin.mkdir(exist_ok=True) + log = tmp_path / "commands.log" + log.touch() + command_environment = { + "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "FAKE_COMMAND_LOG": str(log), + "HOME": str(tmp_path), + } | environment + result = subprocess.run( + ["bash", str(script), *arguments], + check=False, + capture_output=True, + text=True, + env=command_environment, + ) + return result, log.read_text(encoding="utf-8").splitlines() + + +# --- The pinned commit ---------------------------------------------------- + + +def test_the_pinned_commit_is_read_and_passed_to_later_steps(tmp_path): + pin = tmp_path / "public-api-commit" + pin.write_text(f"{PINNED_COMMIT}\n", encoding="utf-8") + github_output = tmp_path / "github-output" + + result, _ = _run( + COMMIT_SCRIPT, [str(pin)], tmp_path, GITHUB_OUTPUT=str(github_output) + ) + + assert result.returncode == 0, result.stderr + assert f"Pinned Public API commit: {PINNED_COMMIT}" in result.stdout + assert github_output.read_text(encoding="utf-8") == f"commit={PINNED_COMMIT}\n" + + +@pytest.mark.parametrize( + "content", + ["", "master\n", f"{PINNED_COMMIT[:12]}\n", f"{PINNED_COMMIT.upper()}\n"], + ids=["empty", "branch", "short", "upper-case"], +) +def test_a_pin_that_is_not_a_full_commit_fails(tmp_path, content): + pin = tmp_path / "public-api-commit" + pin.write_text(content, encoding="utf-8") + + result, _ = _run(COMMIT_SCRIPT, [str(pin)], tmp_path) + + assert result.returncode != 0 + assert "40 lowercase hexadecimal characters" in result.stderr + + +def test_the_repository_pins_a_full_commit(tmp_path): + result, _ = _run( + COMMIT_SCRIPT, + [ + str( + REPOSITORY_ROOT + / "libs" + / "policyengine-simulation-persistence" + / "public-api-commit" + ) + ], + tmp_path, + ) + + assert result.returncode == 0, result.stderr + + +# --- The default branch --------------------------------------------------- + + +def _fake_git(tmp_path, *, ancestor: bool) -> None: + fake_bin = tmp_path / "bin" + fake_bin.mkdir(exist_ok=True) + _fake_command( + fake_bin, + "git", + textwrap.dedent( + f"""\ + case "$*" in + *"rev-parse HEAD"*) printf '%s\\n' "{PINNED_COMMIT}" ;; + *"merge-base --is-ancestor"*) exit {0 if ancestor else 1} ;; + esac + """ + ), + ) + + +def test_a_commit_on_the_default_branch_passes(tmp_path): + _fake_git(tmp_path, ancestor=True) + + result, commands = _run(DEFAULT_BRANCH_SCRIPT, ["public-api", "master"], tmp_path) + + assert result.returncode == 0, result.stderr + assert "git|-C public-api fetch --quiet origin master" in commands + assert ( + f"git|-C public-api merge-base --is-ancestor {PINNED_COMMIT} FETCH_HEAD" + in commands + ) + + +def test_a_commit_not_yet_on_the_default_branch_fails_and_says_so(tmp_path): + _fake_git(tmp_path, ancestor=False) + + result, _ = _run(DEFAULT_BRANCH_SCRIPT, ["public-api", "master"], tmp_path) + + assert result.returncode == 1 + assert f"{PINNED_COMMIT} is not on the Public API's master branch" in ( + result.stderr + ) + + +# --- The reference database ----------------------------------------------- + + +def _fake_reference_commands(tmp_path, *, failing_step: str = "") -> None: + fake_bin = tmp_path / "bin" + fake_bin.mkdir(exist_ok=True) + _fake_command(fake_bin, "git", f"printf '%s\\n' '{PINNED_COMMIT}'") + _fake_command( + fake_bin, + "uv", + textwrap.dedent( + f"""\ + printf 'uv-env|%s|%s|%s\\n' "${{V2_MIGRATION_DATABASE_URL:-}}" \\ + "${{V2_ALEMBIC_DISPOSABLE_TEST:-}}" "${{DEPLOYMENT_ENVIRONMENT:-}}" \\ + >> "${{FAKE_COMMAND_LOG}}" + case "$*" in + *"{failing_step}"*) [ -n "{failing_step}" ] && exit 3 ;; + esac + exit 0 + """ + ), + ) + + +def test_the_reference_migrates_the_public_api_and_grants_its_privileges(tmp_path): + checkout = tmp_path / "public-api" + checkout.mkdir() + github_env = tmp_path / "github-env" + _fake_reference_commands(tmp_path) + + result, commands = _run( + REFERENCE_SCRIPT, + [str(checkout)], + tmp_path, + PUBLIC_API_SCHEMA_DATABASE_URL=DATABASE_URL, + GITHUB_ENV=str(github_env), + ) + + assert result.returncode == 0, result.stderr + uv_calls = [line for line in commands if line.startswith("uv|")] + assert uv_calls == [ + "uv|sync --frozen --extra dev", + "uv|run --frozen --extra dev alembic -c alembic-v2.ini upgrade head", + "uv|run --frozen --extra dev python scripts/reconcile_v2_privileges.py " + "apply --create-missing-roles", + ] + psycopg_url = "postgresql+psycopg://" + DATABASE_URL.split("://", 1)[1] + assert f"uv-env|{psycopg_url}|1|ci" in commands + assert f"Public API commit: {PINNED_COMMIT}" in result.stdout + assert github_env.read_text(encoding="utf-8") == ( + f"PUBLIC_API_SCHEMA_COMMIT={PINNED_COMMIT}\n" + ) + + +@pytest.mark.parametrize("failing_step", ["upgrade head", "reconcile_v2_privileges"]) +def test_a_failed_reference_step_fails_the_job(tmp_path, failing_step): + checkout = tmp_path / "public-api" + checkout.mkdir() + _fake_reference_commands(tmp_path, failing_step=failing_step) + + result, _ = _run( + REFERENCE_SCRIPT, + [str(checkout)], + tmp_path, + PUBLIC_API_SCHEMA_DATABASE_URL=DATABASE_URL, + ) + + assert result.returncode == 3 + + +def test_the_reference_needs_a_database_to_migrate(tmp_path): + checkout = tmp_path / "public-api" + checkout.mkdir() + _fake_reference_commands(tmp_path) + + result, commands = _run(REFERENCE_SCRIPT, [str(checkout)], tmp_path) + + assert result.returncode != 0 + assert "PUBLIC_API_SCHEMA_DATABASE_URL" in result.stderr + assert not [line for line in commands if line.startswith("uv|")] diff --git a/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py index b5d27b8cb..2ead9dbd7 100644 --- a/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py +++ b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py @@ -15,6 +15,8 @@ TableShape, describe_tables, schema_differences, + schema_from_json, + schema_to_json, ) from sqlalchemy import create_engine, text from sqlalchemy.engine import make_url @@ -307,3 +309,11 @@ def test_the_catalog_describes_each_part_of_a_table(disposable_database) -> None }, enums={"v2_simulation_status": ("pending", "running")}, ) + + +def test_a_description_round_trips_through_json(tmp_path) -> None: + described = _simulation_runs() + path = tmp_path / "schema.json" + path.write_text(schema_to_json(described), encoding="utf-8") + + assert schema_from_json(path) == described diff --git a/rules/fixtures/sim.persistence.models-match-public-api/bad_models.json b/rules/fixtures/sim.persistence.models-match-public-api/bad_models.json new file mode 100644 index 000000000..eafd6da7b --- /dev/null +++ b/rules/fixtures/sim.persistence.models-match-public-api/bad_models.json @@ -0,0 +1,100 @@ +{ + "enums": {}, + "tables": { + "simulation_runs": { + "columns": { + "completed_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "country_package_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "created_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + }, + "dataset_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "error_message": { + "column_type": "character varying", + "default": null, + "nullable": true + }, + "household_result": { + "column_type": "json", + "default": null, + "nullable": true + }, + "id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "output_sha256": { + "column_type": "character varying(64)", + "default": null, + "nullable": true + }, + "output_uri": { + "column_type": "character varying(2048)", + "default": null, + "nullable": true + }, + "policyengine_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "region_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "simulation_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "started_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "status": { + "column_type": "character varying(32)", + "default": null, + "nullable": false + }, + "tax_benefit_model_version_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "updated_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + } + }, + "constraints": { + "ck_simulation_runs_output_pair": "CHECK (((output_uri IS NULL) = (output_sha256 IS NULL)))", + "ck_simulation_runs_output_sha256_length": "CHECK (((output_sha256 IS NULL) OR (length((output_sha256)::text) = 64)))", + "ck_simulation_runs_terminal_completion": "CHECK (((status <> ALL (ARRAY['succeeded'::v2_simulation_status, 'failed'::v2_simulation_status])) OR (completed_at IS NOT NULL)))", + "fk_simulation_runs_simulation_id_simulations": "FOREIGN KEY (simulation_id) REFERENCES simulations(id) ON DELETE CASCADE", + "pk_simulation_runs": "PRIMARY KEY (id)" + }, + "indexes": { + "ix_simulation_runs_current_output": "CREATE INDEX ix_simulation_runs_current_output ON public.simulation_runs USING btree (simulation_id, status, country_package_version, policyengine_version, completed_at, id)", + "ix_simulation_runs_status_started_at": "CREATE INDEX ix_simulation_runs_status_started_at ON public.simulation_runs USING btree (status, started_at)" + } + } + } +} diff --git a/rules/fixtures/sim.persistence.models-match-public-api/good_models.json b/rules/fixtures/sim.persistence.models-match-public-api/good_models.json new file mode 100644 index 000000000..4cfc0fd9e --- /dev/null +++ b/rules/fixtures/sim.persistence.models-match-public-api/good_models.json @@ -0,0 +1,112 @@ +{ + "enums": { + "v2_simulation_status": [ + "pending", + "running", + "succeeded", + "failed" + ] + }, + "tables": { + "simulation_runs": { + "columns": { + "completed_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "country_package_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "created_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + }, + "dataset_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "error_message": { + "column_type": "character varying", + "default": null, + "nullable": true + }, + "household_result": { + "column_type": "json", + "default": null, + "nullable": true + }, + "id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "modal_call_id": { + "column_type": "character varying(255)", + "default": null, + "nullable": true + }, + "output_sha256": { + "column_type": "character varying(64)", + "default": null, + "nullable": true + }, + "output_uri": { + "column_type": "character varying(2048)", + "default": null, + "nullable": true + }, + "policyengine_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "region_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "simulation_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "started_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "status": { + "column_type": "enum v2_simulation_status", + "default": null, + "nullable": false + }, + "tax_benefit_model_version_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "updated_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + } + }, + "constraints": { + "ck_simulation_runs_output_pair": "CHECK (((output_uri IS NULL) = (output_sha256 IS NULL)))", + "ck_simulation_runs_output_sha256_length": "CHECK (((output_sha256 IS NULL) OR (length((output_sha256)::text) = 64)))", + "ck_simulation_runs_terminal_completion": "CHECK (((status <> ALL (ARRAY['succeeded'::v2_simulation_status, 'failed'::v2_simulation_status])) OR (completed_at IS NOT NULL)))", + "fk_simulation_runs_simulation_id_simulations": "FOREIGN KEY (simulation_id) REFERENCES simulations(id) ON DELETE CASCADE", + "pk_simulation_runs": "PRIMARY KEY (id)" + }, + "indexes": { + "ix_simulation_runs_current_output": "CREATE INDEX ix_simulation_runs_current_output ON public.simulation_runs USING btree (simulation_id, status, country_package_version, policyengine_version, completed_at, id)", + "ix_simulation_runs_status_started_at": "CREATE INDEX ix_simulation_runs_status_started_at ON public.simulation_runs USING btree (status, started_at)" + } + } + } +} diff --git a/rules/fixtures/sim.persistence.models-match-public-api/public_api_schema.json b/rules/fixtures/sim.persistence.models-match-public-api/public_api_schema.json new file mode 100644 index 000000000..4cfc0fd9e --- /dev/null +++ b/rules/fixtures/sim.persistence.models-match-public-api/public_api_schema.json @@ -0,0 +1,112 @@ +{ + "enums": { + "v2_simulation_status": [ + "pending", + "running", + "succeeded", + "failed" + ] + }, + "tables": { + "simulation_runs": { + "columns": { + "completed_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "country_package_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "created_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + }, + "dataset_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "error_message": { + "column_type": "character varying", + "default": null, + "nullable": true + }, + "household_result": { + "column_type": "json", + "default": null, + "nullable": true + }, + "id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "modal_call_id": { + "column_type": "character varying(255)", + "default": null, + "nullable": true + }, + "output_sha256": { + "column_type": "character varying(64)", + "default": null, + "nullable": true + }, + "output_uri": { + "column_type": "character varying(2048)", + "default": null, + "nullable": true + }, + "policyengine_version": { + "column_type": "character varying(128)", + "default": null, + "nullable": false + }, + "region_id": { + "column_type": "uuid", + "default": null, + "nullable": true + }, + "simulation_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "started_at": { + "column_type": "timestamp with time zone", + "default": null, + "nullable": true + }, + "status": { + "column_type": "enum v2_simulation_status", + "default": null, + "nullable": false + }, + "tax_benefit_model_version_id": { + "column_type": "uuid", + "default": null, + "nullable": false + }, + "updated_at": { + "column_type": "timestamp with time zone", + "default": "now()", + "nullable": false + } + }, + "constraints": { + "ck_simulation_runs_output_pair": "CHECK (((output_uri IS NULL) = (output_sha256 IS NULL)))", + "ck_simulation_runs_output_sha256_length": "CHECK (((output_sha256 IS NULL) OR (length((output_sha256)::text) = 64)))", + "ck_simulation_runs_terminal_completion": "CHECK (((status <> ALL (ARRAY['succeeded'::v2_simulation_status, 'failed'::v2_simulation_status])) OR (completed_at IS NOT NULL)))", + "fk_simulation_runs_simulation_id_simulations": "FOREIGN KEY (simulation_id) REFERENCES simulations(id) ON DELETE CASCADE", + "pk_simulation_runs": "PRIMARY KEY (id)" + }, + "indexes": { + "ix_simulation_runs_current_output": "CREATE INDEX ix_simulation_runs_current_output ON public.simulation_runs USING btree (simulation_id, status, country_package_version, policyengine_version, completed_at, id)", + "ix_simulation_runs_status_started_at": "CREATE INDEX ix_simulation_runs_status_started_at ON public.simulation_runs USING btree (status, started_at)" + } + } + } +} diff --git a/rules/registry.toml b/rules/registry.toml index 66cbc9ddf..e435a0a4d 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -168,6 +168,43 @@ good_fixture = "rules/fixtures/sim.persistence.missing-privileges-reported/good_ project = "libs/policyengine-simulation-persistence" test = "tests/test_access.py::test_rule_proof_missing_privileges_reported" +[[rule]] +id = "sim.persistence.models-match-public-api" +category = "persistence" +statement = "Sim's models of the Public API's tables match the tables the Public API's v2 migration chain builds at the pinned Public API commit, which is on the Public API's default branch: every column's type, nullability and default, every constraint and index by name and definition, and every enum's values; and the simulation service's role, with the privileges the Public API's manifest declares, writes every modelled table." +rationale = "libs/policyengine-simulation-persistence/README.md#checking-the-models-against-the-public-api" +scope = [ + "libs/policyengine-simulation-persistence/src/policyengine_simulation_persistence/tables.py", + "libs/policyengine-simulation-persistence/public-api-commit", + ".github/scripts/public-api-schema-commit.sh", + ".github/scripts/public-api-schema-reference.sh", + ".github/scripts/public-api-commit-on-default-branch.sh", +] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "pytest" +project = "libs/policyengine-simulation-persistence" +nodes = [ + "tests/test_public_api_schema.py::test_the_models_match_the_public_api_tables", + "tests/test_service_writes.py::test_the_service_role_holds_every_privilege_the_service_needs", + "tests/test_service_writes.py::test_the_service_role_creates_and_reads_a_row_in_every_table_it_writes", +] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "public-api-schema" +invocation = "uv run --frozen pytest tests/ -v -m public_api_schema" +[[rule.ci]] +workflow = ".github/workflows/public-api-schema-drift.yml" +job = "public-api-schema-drift" +invocation = "uv run --frozen pytest tests/ -v -m public_api_schema" +[rule.proof] +bad_fixture = "rules/fixtures/sim.persistence.models-match-public-api/bad_models.json" +good_fixture = "rules/fixtures/sim.persistence.models-match-public-api/good_models.json" +project = "libs/policyengine-simulation-persistence" +test = "tests/test_public_api_schema.py::test_rule_proof_models_match_public_api" + # --- Stage 12 persistence -------------------------------------------------- [[rule]] From ccacc35ed912f819a3ea99d0bc87c5c00e2a7b80 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:10:55 +0400 Subject: [PATCH 03/10] Compare the models' tables by name now that the models name their schema service_metadata keys its tables by schema and name since the models name the public schema, while the catalog descriptions are keyed by table name alone, so the comparison described no table of the models. It now passes the tables' names. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_public_api_schema.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py index b4ffc721c..93ef0fa48 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py @@ -39,20 +39,21 @@ def _engine(database_url: str): def test_the_models_match_the_public_api_tables( disposable_database, public_api_database_url ) -> None: + table_names = [table.name for table in service_metadata.tables.values()] models_engine = _engine(disposable_database.superuser_url) real_engine = _engine(public_api_database_url) try: # Tests may build the models' tables; runtime code never does. service_metadata.create_all(models_engine) with models_engine.connect() as connection: - models = describe_tables(connection, service_metadata.tables) + models = describe_tables(connection, table_names) with real_engine.connect() as connection: - real = describe_tables(connection, service_metadata.tables) + real = describe_tables(connection, table_names) finally: models_engine.dispose() real_engine.dispose() - assert set(models.tables) == set(service_metadata.tables) + assert set(models.tables) == set(table_names) compared_with = os.environ.get(COMMIT_VARIABLE) or "an unrecorded commit" assert schema_differences(models, real) == [], ( f"compared with the Public API at {compared_with}" From b64b13641c581eb47d7bbd4846745c64dc86a387 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:11:15 +0400 Subject: [PATCH 04/10] Mark the schema rule's proof so the rule's own CI jobs run it The rules registry ties sim.persistence.models-match-public-api's proof to the jobs in its [[rule.ci]] entries, the Public API schema job and the drift run, but both select -m public_api_schema and the proof had no marker, so neither collected it. It is marked now, and a test keeps it so. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_public_api_schema.py | 2 ++ .../tests/test_public_api_schema_job.py | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py index 93ef0fa48..a2380d2d0 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py @@ -60,6 +60,8 @@ def test_the_models_match_the_public_api_tables( ) +# Marked so the rule's own CI jobs, which select public_api_schema, collect it. +@pytest.mark.public_api_schema def test_rule_proof_models_match_public_api() -> None: """Proof for rule sim.persistence.models-match-public-api. diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py index 3dd3ebe37..e55537508 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -132,3 +132,14 @@ def test_the_weekly_run_reads_no_secret() -> None: assert "secrets." not in DRIFT_WORKFLOW.read_text(encoding="utf-8") assert workflow["permissions"] == {"contents": "read"} + + +def test_the_rule_proof_runs_in_the_jobs_that_enforce_the_rule() -> None: + """The jobs select ``-m public_api_schema``; an unmarked proof is skipped.""" + from test_public_api_schema import test_rule_proof_models_match_public_api + + marks = { + mark.name for mark in test_rule_proof_models_match_public_api.pytestmark + } + + assert "public_api_schema" in marks From 42db4d2140fc62c4a0706dbb9681b361fe10e0e1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:12:22 +0400 Subject: [PATCH 05/10] Keep no token in the schema jobs, and close the review's coverage and documentation gaps Both jobs run the Public API's code, from a pinned and perhaps unmerged commit, after checking out sim, whose checkout kept the job's token; now neither checkout keeps it. The tests now also check that the reference script installs the Public API's lock on Python 3.12, that the weekly run builds and installs as the PR job does, and that an index only the models have is reported. The persistence readme says what each marked test needs and what to pin after a squash or rebase merge, the weekly run's comment and readme say what else can fail it, the database-access skill names the one shell script that reaches a database, and AGENTS.md lists the comparison among PR CI's checks. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yml | 4 +++ .github/workflows/public-api-schema-drift.yml | 14 +++++--- AGENTS.md | 4 ++- docs/engineering/skills/database-access.md | 8 +++-- .../README.md | 24 ++++++++------ .../tests/test_public_api_schema_job.py | 32 +++++++++++++++++-- .../tests/test_public_api_schema_scripts.py | 7 ++-- .../tests/test_schema_comparison.py | 11 +++++++ 8 files changed, 81 insertions(+), 23 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index a8c132e6e..80b39dd23 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -167,7 +167,11 @@ jobs: SIMULATION_REQUIRE_PUBLIC_API_SCHEMA: "1" steps: + # Neither checkout keeps the job's token, since the job runs the Public + # API's code from a pinned commit. - uses: actions/checkout@v6 + with: + persist-credentials: false - name: Read the pinned Public API commit id: public-api diff --git a/.github/workflows/public-api-schema-drift.yml b/.github/workflows/public-api-schema-drift.yml index 9da571d81..e8f97b62e 100644 --- a/.github/workflows/public-api-schema-drift.yml +++ b/.github/workflows/public-api-schema-drift.yml @@ -3,10 +3,12 @@ name: Public API schema drift # Compares sim's table models with the tables the Public API's v2 migration # chain builds at the head of its default branch, every Monday and on demand. # Pull requests compare them at the pinned commit instead (the PR workflow's -# public-api-schema job); a failure here means the Public API has merged a -# change to a table the simulation service writes, so the models and the pin -# need to follow it (rule sim.persistence.models-match-public-api). The -# Public API's repository is public, so the workflow reads no secret. +# public-api-schema job). A failure here means the Public API has merged a +# change to a table the simulation service writes, or to its role's grants, +# that the models and the pin need to follow, or that its tables can't be +# built at that head; the log says which (rule +# sim.persistence.models-match-public-api). The Public API's repository is +# public, so the workflow reads no secret. on: schedule: @@ -45,7 +47,11 @@ jobs: SIMULATION_REQUIRE_PUBLIC_API_SCHEMA: "1" steps: + # Neither checkout keeps the job's token, since the job runs the Public + # API's code. - uses: actions/checkout@v6 + with: + persist-credentials: false - name: Check out the Public API's default branch uses: actions/checkout@v6 diff --git a/AGENTS.md b/AGENTS.md index 74b9f523d..421595ad5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,7 +92,9 @@ same-repository draft PR. - The simulation service lives in `projects/policyengine-simulation-executor`. - API integration tests live in `projects/policyengine-apis-integ`. - PR CI runs simulation unit tests, the persistence tests that need - PostgreSQL, Ruff format and lint checks, Pyright for every package, quality + PostgreSQL, the comparison of the persistence models with the Public API's + tables at a pinned commit, Ruff format and lint checks, Pyright for every + package, quality guards (including the rules registry), the changelog fragment check, Docker build, and local integration tests. A separate `PR description` job checks that every open PR's description starts with diff --git a/docs/engineering/skills/database-access.md b/docs/engineering/skills/database-access.md index 89fc3e13f..73609c9ab 100644 --- a/docs/engineering/skills/database-access.md +++ b/docs/engineering/skills/database-access.md @@ -73,9 +73,11 @@ but still connect through SQLAlchemy. The guard reads source, not types. It misses imports through `importlib` or `__import__`, a table reached through some other local variable, and SQL text -assembled from separate literals. It doesn't read shell scripts or SQL files; -today no shell script calls a database client, and the repository has no SQL -files. +assembled from separate literals. It doesn't read shell scripts or SQL files. +The one shell script that reaches a database, +`.github/scripts/public-api-schema-reference.sh`, runs the Public API's own +migration and privilege commands against a disposable CI database for +`sim.persistence.models-match-public-api`; the repository has no SQL files. ## Out of scope diff --git a/libs/policyengine-simulation-persistence/README.md b/libs/policyengine-simulation-persistence/README.md index f53e96c20..dbf6d56c4 100644 --- a/libs/policyengine-simulation-persistence/README.md +++ b/libs/policyengine-simulation-persistence/README.md @@ -52,13 +52,17 @@ ever describe a chain the Public API has merged. models describe: the merge commit of the Public API change that last altered a modelled table, or, while that change is still a pull request, its head commit. Until that pull request merges, the job's last step fails and -says so. +says so. A merge commit keeps the pull request's head on `master`; if the +Public API squashes or rebases the pull request instead, its head never +reaches `master`, so move the pin to the commit that landed. **Keeping it current.** A sim pull request that changes the models moves the pin in the same change. The `Public API schema drift` workflow runs the same -comparison every Monday, and on demand, against the head of the Public -API's `master`; it fails when the Public API has merged a change to a -modelled table that the models and the pin haven't followed yet. +tests every Monday, and on demand, against the head of the Public API's +`master`. It fails when the Public API has merged a change to a modelled +table, or to the service role's grants, that the models and the pin haven't +followed yet, and also when the Public API's tables can't be built at that +head; its log says which. To run the comparison locally, build the reference with the script (it needs an empty `policyengine_v2_alembic_test` database and a checkout of the @@ -96,11 +100,13 @@ uv sync --extra test uv run pytest tests/ ``` -Tests marked `public_api_schema` run as the service's role in a database -the Public API's own v2 migration chain built, with the privileges its -manifest declares granted: set `PUBLIC_API_SCHEMA_DATABASE_URL` to that -database's superuser URL. They create a row in every modelled table and roll -it back. Without the variable they skip. +Tests marked `public_api_schema` need a database the Public API's own v2 +migration chain built, with the privileges its manifest declares granted: +set `PUBLIC_API_SCHEMA_DATABASE_URL` to that database's superuser URL. +`test_service_writes` creates a row in every modelled table as the service's +role and rolls it back; the comparison in `test_public_api_schema` also needs +`SIMULATION_TEST_DATABASE_URL`, to build the models' own tables. Without the +variables they skip. Tests marked `postgres` need a PostgreSQL server: set `SIMULATION_TEST_DATABASE_URL` to its superuser URL, such as diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py index e55537508..63b25001c 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -138,8 +138,34 @@ def test_the_rule_proof_runs_in_the_jobs_that_enforce_the_rule() -> None: """The jobs select ``-m public_api_schema``; an unmarked proof is skipped.""" from test_public_api_schema import test_rule_proof_models_match_public_api - marks = { - mark.name for mark in test_rule_proof_models_match_public_api.pytestmark - } + marks = {mark.name for mark in test_rule_proof_models_match_public_api.pytestmark} assert "public_api_schema" in marks + + +def test_no_checkout_leaves_a_token_while_the_public_api_code_runs() -> None: + """The jobs run the Public API's code from a pinned, perhaps unmerged commit.""" + jobs = [_jobs()[JOB_ID], _drift_workflow()["jobs"][DRIFT_JOB_ID]] + + for job in jobs: + checkouts = [ + step + for step in job["steps"] + if step.get("uses", "").startswith("actions/checkout@") + ] + assert len(checkouts) == 2 + for checkout in checkouts: + assert checkout["with"]["persist-credentials"] is False, checkout + + +def test_the_weekly_run_builds_the_reference_and_installs_like_the_pr_job() -> None: + pr_job = _jobs()[JOB_ID] + drift_job = _drift_workflow()["jobs"][DRIFT_JOB_ID] + build = _step(drift_job, "Build the Public API's tables at its default branch") + + assert build["run"] == ".github/scripts/public-api-schema-reference.sh public-api" + assert _step(drift_job, "Install dependencies") == _step( + pr_job, "Install dependencies" + ) + assert _step(drift_job, "Install uv") == _step(pr_job, "Install uv") + assert _step(drift_job, "Set up Python") == _step(pr_job, "Set up Python") diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py index 8a1b4d35a..d973f4464 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_scripts.py @@ -168,9 +168,9 @@ def _fake_reference_commands(tmp_path, *, failing_step: str = "") -> None: "uv", textwrap.dedent( f"""\ - printf 'uv-env|%s|%s|%s\\n' "${{V2_MIGRATION_DATABASE_URL:-}}" \\ + printf 'uv-env|%s|%s|%s|%s\\n' "${{V2_MIGRATION_DATABASE_URL:-}}" \\ "${{V2_ALEMBIC_DISPOSABLE_TEST:-}}" "${{DEPLOYMENT_ENVIRONMENT:-}}" \\ - >> "${{FAKE_COMMAND_LOG}}" + "${{UV_PYTHON:-}}" >> "${{FAKE_COMMAND_LOG}}" case "$*" in *"{failing_step}"*) [ -n "{failing_step}" ] && exit 3 ;; esac @@ -203,7 +203,8 @@ def test_the_reference_migrates_the_public_api_and_grants_its_privileges(tmp_pat "apply --create-missing-roles", ] psycopg_url = "postgresql+psycopg://" + DATABASE_URL.split("://", 1)[1] - assert f"uv-env|{psycopg_url}|1|ci" in commands + # The Public API's lock installs on Python 3.12, as its own CI does. + assert f"uv-env|{psycopg_url}|1|ci|3.12" in commands assert f"Public API commit: {PINNED_COMMIT}" in result.stdout assert github_env.read_text(encoding="utf-8") == ( f"PUBLIC_API_SCHEMA_COMMIT={PINNED_COMMIT}\n" diff --git a/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py index 2ead9dbd7..0b4698580 100644 --- a/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py +++ b/libs/policyengine-simulation-persistence/tests/test_schema_comparison.py @@ -224,6 +224,17 @@ def test_an_index_with_another_definition_is_reported() -> None: ] +def test_an_index_only_the_model_has_is_reported() -> None: + real = _simulation_runs(without_indexes=["ix_simulation_runs_status_started_at"]) + + assert schema_differences(_simulation_runs(), real) == [ + "simulation_runs: the Public API's table has no index " + "ix_simulation_runs_status_started_at (CREATE INDEX " + "ix_simulation_runs_status_started_at ON public.simulation_runs USING " + "btree (status, created_at))", + ] + + def test_tables_the_models_leave_out_are_not_compared() -> None: real = _simulation_runs() real.tables["users"] = TableShape( From 97cadb8d7925df13e5bd66e310fba67276205cf3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:29:30 +0400 Subject: [PATCH 06/10] Keep every test the schema rule names in the schema jobs, and move the pin to the Public API's latest head Both schema jobs select tests by the public_api_schema marker, and the rules guard trusts their pytest command without reading markers, so a test that lost the marker would leave the jobs while every job stayed green. A test now checks that each test the rule names, its proof included, carries the marker and not postgres. The comparison test is no longer marked postgres: the Simulation database job selects that marker but has no Public API database, so the test could only skip there. The marker's descriptions say what it now means. The pin moves to the Public API branch's new head, e7c362b16, whose two newer revisions change result-table keys and grant SELECT on dynamics; the models and the service role's required privileges still match it. Co-Authored-By: Claude Opus 5.5 --- .../README.md | 6 ++-- .../public-api-commit | 2 +- .../pyproject.toml | 2 +- .../tests/conftest.py | 3 +- .../tests/test_public_api_schema.py | 3 +- .../tests/test_public_api_schema_job.py | 33 +++++++++++++++---- 6 files changed, 37 insertions(+), 12 deletions(-) diff --git a/libs/policyengine-simulation-persistence/README.md b/libs/policyengine-simulation-persistence/README.md index dbf6d56c4..ee26afaa7 100644 --- a/libs/policyengine-simulation-persistence/README.md +++ b/libs/policyengine-simulation-persistence/README.md @@ -100,8 +100,10 @@ uv sync --extra test uv run pytest tests/ ``` -Tests marked `public_api_schema` need a database the Public API's own v2 -migration chain built, with the privileges its manifest declares granted: +Tests marked `public_api_schema` run in the `Public API schema` jobs; the +rule's proof among them needs no database, and the others need one the +Public API's own v2 migration chain built, with the privileges its manifest +declares granted: set `PUBLIC_API_SCHEMA_DATABASE_URL` to that database's superuser URL. `test_service_writes` creates a row in every modelled table as the service's role and rolls it back; the comparison in `test_public_api_schema` also needs diff --git a/libs/policyengine-simulation-persistence/public-api-commit b/libs/policyengine-simulation-persistence/public-api-commit index 0832b932c..c7b6770c0 100644 --- a/libs/policyengine-simulation-persistence/public-api-commit +++ b/libs/policyengine-simulation-persistence/public-api-commit @@ -1 +1 @@ -dc617db3e961bfff3ff43e889687666245227cea +e7c362b16fd40b64135e747c5947b9007d0b68df diff --git a/libs/policyengine-simulation-persistence/pyproject.toml b/libs/policyengine-simulation-persistence/pyproject.toml index ae0ebed92..62aca85ea 100644 --- a/libs/policyengine-simulation-persistence/pyproject.toml +++ b/libs/policyengine-simulation-persistence/pyproject.toml @@ -36,7 +36,7 @@ pythonpath = ["src"] testpaths = ["tests"] markers = [ "postgres: needs a disposable PostgreSQL server named by SIMULATION_TEST_DATABASE_URL", - "public_api_schema: needs a database the Public API's migration chain built, named by PUBLIC_API_SCHEMA_DATABASE_URL", + "public_api_schema: run by the Public API schema jobs; those that use a database need one the Public API's migration chain built, named by PUBLIC_API_SCHEMA_DATABASE_URL", ] [tool.pyright] diff --git a/libs/policyengine-simulation-persistence/tests/conftest.py b/libs/policyengine-simulation-persistence/tests/conftest.py index b1aa8306e..6ca1e8b3e 100644 --- a/libs/policyengine-simulation-persistence/tests/conftest.py +++ b/libs/policyengine-simulation-persistence/tests/conftest.py @@ -6,7 +6,8 @@ the variable those tests skip, unless ``SIMULATION_REQUIRE_DATABASE_TESTS=1`` (set in CI), where they fail instead. -Tests marked ``public_api_schema`` need a database the Public API's own v2 +Tests marked ``public_api_schema`` run in the Public API schema jobs; those +that use ``public_api_database_url`` need a database the Public API's own v2 migration chain built, with the privileges its manifest declares granted (run its ``alembic -c alembic-v2.ini upgrade head`` and ``scripts/reconcile_v2_privileges.py apply --create-missing-roles`` against diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py index a2380d2d0..6700cc7b0 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema.py @@ -34,7 +34,8 @@ def _engine(database_url: str): return create_engine(make_url(database_url).set(drivername="postgresql+psycopg")) -@pytest.mark.postgres +# Not marked postgres: the Simulation database job, which selects that +# marker, has no Public API database, so the test could only skip there. @pytest.mark.public_api_schema def test_the_models_match_the_public_api_tables( disposable_database, public_api_database_url diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py index 63b25001c..fa68fcfc8 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -8,12 +8,15 @@ from __future__ import annotations +import importlib from pathlib import Path +import tomllib import yaml REPOSITORY_ROOT = Path(__file__).resolve().parents[3] PR_WORKFLOW = REPOSITORY_ROOT / ".github" / "workflows" / "pr.yml" +REGISTRY = REPOSITORY_ROOT / "rules" / "registry.toml" JOB_ID = "public-api-schema" PIN_FILE = "libs/policyengine-simulation-persistence/public-api-commit" DRIFT_WORKFLOW = ( @@ -134,13 +137,31 @@ def test_the_weekly_run_reads_no_secret() -> None: assert workflow["permissions"] == {"contents": "read"} -def test_the_rule_proof_runs_in_the_jobs_that_enforce_the_rule() -> None: - """The jobs select ``-m public_api_schema``; an unmarked proof is skipped.""" - from test_public_api_schema import test_rule_proof_models_match_public_api +def test_every_test_the_rule_names_runs_in_its_jobs_alone() -> None: + """The jobs select ``-m public_api_schema``; an unmarked test leaves them. - marks = {mark.name for mark in test_rule_proof_models_match_public_api.pytestmark} - - assert "public_api_schema" in marks + The rules guard trusts the jobs' pytest command without reading markers, + so this checks each test the rule names, its proof included, and that + none is also marked ``postgres``, which the Simulation database job + selects and where it could only skip. + """ + rule = next( + entry + for entry in tomllib.loads(REGISTRY.read_text(encoding="utf-8"))["rule"] + if entry["id"] == "sim.persistence.models-match-public-api" + ) + node_ids = [*rule["enforcer"]["nodes"], rule["proof"]["test"]] + + for node_id in node_ids: + module_path, test_name = node_id.split("::") + module = importlib.import_module(Path(module_path).stem) + module_marks = getattr(module, "pytestmark", []) + if not isinstance(module_marks, list): + module_marks = [module_marks] + test_marks = getattr(getattr(module, test_name), "pytestmark", []) + marks = {mark.name for mark in [*module_marks, *test_marks]} + assert "public_api_schema" in marks, node_id + assert "postgres" not in marks, node_id def test_no_checkout_leaves_a_token_while_the_public_api_code_runs() -> None: From bfa290814de1bed35252fbeb35b8c2ac9ba13f6f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:46:49 +0400 Subject: [PATCH 07/10] Check every persistence rule's tests against the markers its jobs select The marker test covered one rule and one marker pair. It now reads, for every pytest rule of the persistence library, the markers its CI jobs select, and requires each test the rule names, its proof included, to carry them, to carry no marker that would send a schema test to the Simulation database job, and to carry no skip, skipif or xfail mark, which would let it pass its job unrun. The marker descriptions, the Simulation database job's comment and its test's docstring say which job runs which tests. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yml | 5 +- .../pyproject.toml | 4 +- .../tests/test_database_job.py | 4 +- .../tests/test_public_api_schema_job.py | 67 +++++++++++++------ 4 files changed, 53 insertions(+), 27 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 80b39dd23..9282f9e24 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -79,8 +79,9 @@ jobs: simulation-database: name: Simulation database - # Runs the persistence tests that need PostgreSQL, which the Test job - # skips, on a fresh server. Full CI runs only on the lowest unmerged pull + # Runs the persistence tests marked postgres, which the Test job skips, + # on a fresh server; the Public API schema job runs those that also need + # the Public API's tables. Full CI runs only on the lowest unmerged pull # request in a stack. if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest diff --git a/libs/policyengine-simulation-persistence/pyproject.toml b/libs/policyengine-simulation-persistence/pyproject.toml index 62aca85ea..779a46096 100644 --- a/libs/policyengine-simulation-persistence/pyproject.toml +++ b/libs/policyengine-simulation-persistence/pyproject.toml @@ -35,8 +35,8 @@ packages = ["src/policyengine_simulation_persistence"] pythonpath = ["src"] testpaths = ["tests"] markers = [ - "postgres: needs a disposable PostgreSQL server named by SIMULATION_TEST_DATABASE_URL", - "public_api_schema: run by the Public API schema jobs; those that use a database need one the Public API's migration chain built, named by PUBLIC_API_SCHEMA_DATABASE_URL", + "postgres: run by the Simulation database job; needs a disposable PostgreSQL server named by SIMULATION_TEST_DATABASE_URL", + "public_api_schema: run by the Public API schema jobs; most need a database the Public API's migration chain built (PUBLIC_API_SCHEMA_DATABASE_URL), and the comparison also a disposable PostgreSQL server (SIMULATION_TEST_DATABASE_URL)", ] [tool.pyright] diff --git a/libs/policyengine-simulation-persistence/tests/test_database_job.py b/libs/policyengine-simulation-persistence/tests/test_database_job.py index e1436f30f..ab7ab03d8 100644 --- a/libs/policyengine-simulation-persistence/tests/test_database_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_database_job.py @@ -1,7 +1,9 @@ -"""The PR job that runs the persistence tests needing PostgreSQL. +"""The PR job that runs the persistence tests marked ``postgres``. Those tests skip without a server, so the ``Test`` job never runs them; the ``Simulation database`` job gives them one and makes a missing server fail. +The tests that also need the Public API's tables are marked +``public_api_schema`` instead, and run in the Public API schema jobs. """ from __future__ import annotations diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py index fa68fcfc8..122060e5b 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -137,31 +137,54 @@ def test_the_weekly_run_reads_no_secret() -> None: assert workflow["permissions"] == {"contents": "read"} -def test_every_test_the_rule_names_runs_in_its_jobs_alone() -> None: - """The jobs select ``-m public_api_schema``; an unmarked test leaves them. +SKIPPING_MARKS = frozenset({"skip", "skipif", "xfail"}) +# The Simulation database job selects postgres but has no Public API +# database, so a schema test marked postgres could only skip there. +EXCLUDED_MARKERS = {"sim.persistence.models-match-public-api": {"postgres"}} - The rules guard trusts the jobs' pytest command without reading markers, - so this checks each test the rule names, its proof included, and that - none is also marked ``postgres``, which the Simulation database job - selects and where it could only skip. - """ - rule = next( + +def _persistence_rules() -> list[dict]: + return [ entry for entry in tomllib.loads(REGISTRY.read_text(encoding="utf-8"))["rule"] - if entry["id"] == "sim.persistence.models-match-public-api" - ) - node_ids = [*rule["enforcer"]["nodes"], rule["proof"]["test"]] - - for node_id in node_ids: - module_path, test_name = node_id.split("::") - module = importlib.import_module(Path(module_path).stem) - module_marks = getattr(module, "pytestmark", []) - if not isinstance(module_marks, list): - module_marks = [module_marks] - test_marks = getattr(getattr(module, test_name), "pytestmark", []) - marks = {mark.name for mark in [*module_marks, *test_marks]} - assert "public_api_schema" in marks, node_id - assert "postgres" not in marks, node_id + if entry["enforcer"].get("project") + == "libs/policyengine-simulation-persistence" + and entry["enforcer"]["kind"] == "pytest" + ] + + +def _marks(node_id: str) -> set[str]: + """Return the names of a test's marks, its module's included.""" + module_path, test_name = node_id.split("::") + module = importlib.import_module(Path(module_path).stem) + module_marks = getattr(module, "pytestmark", []) + if not isinstance(module_marks, list): + module_marks = [module_marks] + test_marks = getattr(getattr(module, test_name), "pytestmark", []) + return {mark.name for mark in [*module_marks, *test_marks]} + + +def test_every_persistence_rule_test_runs_in_the_jobs_that_enforce_it() -> None: + """Each rule's jobs select tests by marker, which the rules guard can't read. + + So each test a persistence rule names, its proof included, must carry + every marker its jobs select, none that would send it to a job without + its database, and no mark that would let it skip or fail unnoticed. + """ + rules = _persistence_rules() + assert {rule["id"] for rule in rules} >= set(EXCLUDED_MARKERS) + for rule in rules: + selected = { + ci["invocation"].split(" -m ", 1)[1].split()[0] + for ci in rule["ci"] + if " -m " in ci["invocation"] + } + assert selected, rule["id"] + for node_id in [*rule["enforcer"]["nodes"], rule["proof"]["test"]]: + marks = _marks(node_id) + assert selected <= marks, (rule["id"], node_id) + assert not marks & EXCLUDED_MARKERS.get(rule["id"], set()), node_id + assert not marks & SKIPPING_MARKS, node_id def test_no_checkout_leaves_a_token_while_the_public_api_code_runs() -> None: From 6de90875c7e57deec36cf2d9edf58dcba3522a73 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:50:45 +0400 Subject: [PATCH 08/10] Pin the Public API commit that moves report runs' untabled results to their own table The Public API's pull request now ends at c9c6eb6d3, whose revision dce0caa329cc moves four report_runs columns to report_run_comparisons; the models below follow it, and the comparison passes there. Co-Authored-By: Claude Opus 5.5 --- libs/policyengine-simulation-persistence/public-api-commit | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/policyengine-simulation-persistence/public-api-commit b/libs/policyengine-simulation-persistence/public-api-commit index c7b6770c0..49c1d60d5 100644 --- a/libs/policyengine-simulation-persistence/public-api-commit +++ b/libs/policyengine-simulation-persistence/public-api-commit @@ -1 +1 @@ -e7c362b16fd40b64135e747c5947b9007d0b68df +c9c6eb6d3fe8f79661a5d884496f22d89e8c1abe From 771bf661f6c6ec949fdf6fd6437bb835effca2e7 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 02:00:26 +0400 Subject: [PATCH 09/10] Require every persistence rule's job to select its tests by one marker The marker check skipped a rule's CI entry whose invocation had no -m, so a second job running the rule's tests unfiltered, where they skip without a database, went unchecked while the test passed. Each entry must now select exactly one marker, read with shell quoting, and an invocation without one, or with a marker expression, fails the test. The module docstring and the Simulation database job's comment say which job runs which tests. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yml | 6 +-- .../tests/test_public_api_schema_job.py | 51 ++++++++++++++----- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 9282f9e24..4d8d3ad7e 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -80,9 +80,9 @@ jobs: simulation-database: name: Simulation database # Runs the persistence tests marked postgres, which the Test job skips, - # on a fresh server; the Public API schema job runs those that also need - # the Public API's tables. Full CI runs only on the lowest unmerged pull - # request in a stack. + # on a fresh server. Tests that need the Public API's tables are marked + # public_api_schema instead and run in the Public API schema job. Full CI + # runs only on the lowest unmerged pull request in a stack. if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest permissions: diff --git a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py index 122060e5b..17153f30b 100644 --- a/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py +++ b/libs/policyengine-simulation-persistence/tests/test_public_api_schema_job.py @@ -1,16 +1,20 @@ -"""The PR job that compares sim's table models with the Public API's tables. - -Rule sim.persistence.models-match-public-api. The job builds the Public -API's tables with its own v2 migration chain at the pinned commit, runs the -tests marked ``public_api_schema`` against them, and requires the pinned -commit to be on the Public API's default branch. +"""The CI jobs that run the persistence library's database rules. + +Rule sim.persistence.models-match-public-api: the PR job and the weekly run +build the Public API's tables with its own v2 migration chain (at the pinned +commit, or at the head of its default branch), run the tests marked +``public_api_schema`` against them, and the PR job requires the pinned +commit to be on the Public API's default branch. A last test checks, for +every persistence rule, that its jobs run each test it names. """ from __future__ import annotations import importlib +import shlex from pathlib import Path +import pytest import tomllib import yaml @@ -153,6 +157,19 @@ def _persistence_rules() -> list[dict]: ] +def _selected_marker(invocation: str) -> str: + """Return the one marker a pytest invocation selects with ``-m``. + + Fail for an invocation that selects none, or an expression rather than + one marker: this test couldn't tell which tests such a job runs. + """ + arguments = shlex.split(invocation) + assert "-m" in arguments, f"{invocation!r} selects no marker" + marker = arguments[arguments.index("-m") + 1] + assert marker.isidentifier(), f"{invocation!r} selects {marker!r}, not one marker" + return marker + + def _marks(node_id: str) -> set[str]: """Return the names of a test's marks, its module's included.""" module_path, test_name = node_id.split("::") @@ -174,12 +191,9 @@ def test_every_persistence_rule_test_runs_in_the_jobs_that_enforce_it() -> None: rules = _persistence_rules() assert {rule["id"] for rule in rules} >= set(EXCLUDED_MARKERS) for rule in rules: - selected = { - ci["invocation"].split(" -m ", 1)[1].split()[0] - for ci in rule["ci"] - if " -m " in ci["invocation"] - } - assert selected, rule["id"] + # Every job of the rule selects its tests by one marker, so each + # job's tests are known. + selected = {_selected_marker(ci["invocation"]) for ci in rule["ci"]} for node_id in [*rule["enforcer"]["nodes"], rule["proof"]["test"]]: marks = _marks(node_id) assert selected <= marks, (rule["id"], node_id) @@ -213,3 +227,16 @@ def test_the_weekly_run_builds_the_reference_and_installs_like_the_pr_job() -> N ) assert _step(drift_job, "Install uv") == _step(pr_job, "Install uv") assert _step(drift_job, "Set up Python") == _step(pr_job, "Set up Python") + + +def test_a_rule_job_must_select_its_tests_by_one_marker() -> None: + """An invocation without a marker can't be checked, so it is refused.""" + assert _selected_marker("uv run --frozen pytest tests/ -v -m postgres") == ( + "postgres" + ) + for invocation in ( + "uv run --frozen pytest tests/", + 'uv run --frozen pytest tests/ -m "postgres and not slow"', + ): + with pytest.raises(AssertionError): + _selected_marker(invocation) From bd0bd238ce01924294b02dcbcfacf9a73f688de8 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 02:41:39 +0400 Subject: [PATCH 10/10] Pin the Public API pull request's head after its move onto master The Public API's pull request for these tables was moved onto its master (528cef1f4), so its old head, c9c6eb6d3, can never reach master and the job's last step would never pass. The pin moves to the new head, d674032a2, where the tables the simulation service writes, their keys and its role's grants are as before: the comparison and the role-write tests pass against a database built there. Co-Authored-By: Claude Opus 5.5 --- libs/policyengine-simulation-persistence/public-api-commit | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/policyengine-simulation-persistence/public-api-commit b/libs/policyengine-simulation-persistence/public-api-commit index 49c1d60d5..b693efb80 100644 --- a/libs/policyengine-simulation-persistence/public-api-commit +++ b/libs/policyengine-simulation-persistence/public-api-commit @@ -1 +1 @@ -c9c6eb6d3fe8f79661a5d884496f22d89e8c1abe +d674032a25b4f764ce8bd73095de39813562591f