From 0a8fbe1c38f0836f6d883dd4f1d8f9b93ce46534 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:04:37 +0400 Subject: [PATCH 01/15] Test the v2-layers guard (failing) scripts/rules/tests/test_v2_layers.py specifies a quality guard over every .py file under services/v2 and fastapi_routes/v2: - each file is assigned a layer by its name or by rules/v2-layer-manifest.toml, and an unassigned file fails; - route modules import no SQLAlchemy, SQLModel, connector, session or v2 database module and call no SQL builder or session method; - services build no SQL, call no data-access session method and import no FastAPI or Starlette; - connectors don't open, commit, roll back or close a session. Calls count only on a receiver named like a session (session.add, not set.add), and SQL builders only when imported from SQLAlchemy or SQLModel. Imports under TYPE_CHECKING count. Proof fixture trees for the four rules include a route that imports a connector and a file with no layer. These fail until scripts/rules/v2_layers.py exists. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 19 + .../good_tree.toml | 16 + .../bad_tree.toml | 18 + .../good_tree.toml | 23 ++ .../bad_tree.toml | 22 ++ .../good_tree.toml | 15 + .../api.v2.services-no-sql/bad_tree.toml | 21 + .../api.v2.services-no-sql/good_tree.toml | 16 + scripts/rules/tests/test_v2_layers.py | 366 ++++++++++++++++++ 9 files changed, 516 insertions(+) create mode 100644 rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml create mode 100644 rules/fixtures/api.v2.connectors-never-commit/good_tree.toml create mode 100644 rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml create mode 100644 rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml create mode 100644 rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml create mode 100644 rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml create mode 100644 rules/fixtures/api.v2.services-no-sql/bad_tree.toml create mode 100644 rules/fixtures/api.v2.services-no-sql/good_tree.toml create mode 100644 scripts/rules/tests/test_v2_layers.py diff --git a/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml new file mode 100644 index 000000000..caed48788 --- /dev/null +++ b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml @@ -0,0 +1,19 @@ +# Proof fixture for rule api.v2.connectors-never-commit. The guard must reject +# it: the connector opens its own session and commits it. + +expected_findings = [ + "policyengine_api/services/v2/policies/database_connectors/creates.py calls get_v2_session_factory", + "policyengine_api/services/v2/policies/database_connectors/creates.py calls session.commit", + "policyengine_api/services/v2/policies/database_connectors/creates.py imports policyengine_api.data.v2.database", +] + +[files] +"policyengine_api/services/v2/policies/database_connectors/creates.py" = """ +from policyengine_api.data.v2.database import get_v2_session_factory + + +def create_policy(row): + session = get_v2_session_factory()() + session.add(row) + session.commit() +""" diff --git a/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml b/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml new file mode 100644 index 000000000..e4d2dfcc2 --- /dev/null +++ b/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml @@ -0,0 +1,16 @@ +# Proof fixture for rule api.v2.connectors-never-commit. The guard must accept +# it: the connector adds the row to the session it receives and flushes it; +# the service's database-session container commits. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/database_connectors/creates.py" = """ +from sqlmodel import Session + + +def create_policy(session: Session, row): + session.add(row) + session.flush() + return row +""" diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml new file mode 100644 index 000000000..872476c4e --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -0,0 +1,18 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject +# it: simulation_client.py matches no layer's file name and no manifest entry +# names it (the case of an open pull request that adds a v2 resource). +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", +] + +[files] +"policyengine_api/services/v2/household_calculations/__init__.py" = "" +"policyengine_api/services/v2/household_calculations/services.py" = """ +from policyengine_api.services.v2.household_calculations.simulation_client import submit +""" +"policyengine_api/services/v2/household_calculations/simulation_client.py" = """ +import httpx +""" diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml new file mode 100644 index 000000000..72f7a07cb --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml @@ -0,0 +1,23 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must accept +# it: every file is named after its layer. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/__init__.py" = "" +"policyengine_api/services/v2/policies/__init__.py" = "" +"policyengine_api/services/v2/policies/services.py" = "" +"policyengine_api/services/v2/policies/validators.py" = "" +"policyengine_api/services/v2/policies/transformations.py" = "" +"policyengine_api/services/v2/policies/types.py" = "" +"policyengine_api/services/v2/policies/database_session.py" = "" +"policyengine_api/services/v2/policies/database_connectors/__init__.py" = "" +"policyengine_api/services/v2/policies/database_connectors/creates.py" = "" +"policyengine_api/services/v2/policies/database_connectors/reads_parameters.py" = "" +"policyengine_api/fastapi_routes/v2/__init__.py" = "" +"policyengine_api/fastapi_routes/v2/routes.py" = "" +"policyengine_api/fastapi_routes/v2/policies/__init__.py" = "" +"policyengine_api/fastapi_routes/v2/policies/routes.py" = "" +"policyengine_api/fastapi_routes/v2/policies/request_models.py" = "" +"policyengine_api/fastapi_routes/v2/policies/response_models.py" = "" +"policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py" = "" diff --git a/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml new file mode 100644 index 000000000..4d6b3fe08 --- /dev/null +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml @@ -0,0 +1,22 @@ +# Proof fixture for rule api.v2.routes-no-sessions-or-sql. The guard must +# reject it: a route module imports a connector and SQLModel, builds a select +# and calls a method on a session. + +expected_findings = [ + "policyengine_api/fastapi_routes/v2/policies/routes.py calls session.add", + "policyengine_api/fastapi_routes/v2/policies/routes.py calls sqlmodel.select", + "policyengine_api/fastapi_routes/v2/policies/routes.py imports policyengine_api.services.v2.policies.database_connectors.reads", + "policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlmodel", +] + +[files] +"policyengine_api/fastapi_routes/v2/policies/routes.py" = """ +from sqlmodel import select + +from policyengine_api.services.v2.policies.database_connectors.reads import read_policy + + +def create_policy(session, row): + session.add(row) + return read_policy(session, select(row)) +""" diff --git a/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml b/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml new file mode 100644 index 000000000..cf65a1149 --- /dev/null +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml @@ -0,0 +1,15 @@ +# Proof fixture for rule api.v2.routes-no-sessions-or-sql. The guard must +# accept it: the route calls one service method. Adding to a set is not a +# session call. + +expected_findings = [] + +[files] +"policyengine_api/fastapi_routes/v2/policies/routes.py" = """ +from policyengine_api.services.v2.policies.services import V2PolicyService + + +def create_policy(service: V2PolicyService, body, seen_ids: set): + seen_ids.add(body.id) + return service.create_policy(body) +""" diff --git a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml new file mode 100644 index 000000000..c2e8f8abe --- /dev/null +++ b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml @@ -0,0 +1,21 @@ +# Proof fixture for rule api.v2.services-no-sql. The guard must reject it: the +# service builds a select, runs it with Session.exec and defines an HTTP +# response with FastAPI. + +expected_findings = [ + "policyengine_api/services/v2/policies/services.py calls _session.exec", + "policyengine_api/services/v2/policies/services.py calls sqlmodel.select", + "policyengine_api/services/v2/policies/services.py imports fastapi.responses", +] + +[files] +"policyengine_api/services/v2/policies/services.py" = """ +from fastapi.responses import JSONResponse +from sqlmodel import select + + +class V2PolicyService: + def get_policy(self, policy_id): + row = self._session.exec(select(policy_id)).one() + return JSONResponse(row) +""" diff --git a/rules/fixtures/api.v2.services-no-sql/good_tree.toml b/rules/fixtures/api.v2.services-no-sql/good_tree.toml new file mode 100644 index 000000000..210358c37 --- /dev/null +++ b/rules/fixtures/api.v2.services-no-sql/good_tree.toml @@ -0,0 +1,16 @@ +# Proof fixture for rule api.v2.services-no-sql. The guard must accept it: the +# service opens a read through its database-session container and passes the +# session to a connector. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/services.py" = """ +from policyengine_api.services.v2.policies.database_connectors.reads import read_policy + + +class V2PolicyService: + def get_policy(self, policy_id): + with self._database_session.read() as session: + return read_policy(session, policy_id) +""" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py new file mode 100644 index 000000000..f4f7fc9dc --- /dev/null +++ b/scripts/rules/tests/test_v2_layers.py @@ -0,0 +1,366 @@ +"""Tests for the v2-layers quality guard (scripts/rules/v2_layers.py).""" + +from __future__ import annotations + +import tempfile +import textwrap +import tomllib +import unittest +from pathlib import Path + +from scripts.rules.v2_layers import ( + CONNECTORS_RULE, + FILES_RULE, + ROUTES_RULE, + SERVICES_RULE, + assign_layer, + run_check, + scan, +) + +REPO_ROOT = Path(__file__).resolve().parents[3] +FIXTURES = REPO_ROOT / "rules" / "fixtures" +ROUTE = "policyengine_api/fastapi_routes/v2/policies/routes.py" +SERVICE = "policyengine_api/services/v2/policies/services.py" +CONNECTOR = "policyengine_api/services/v2/policies/database_connectors/creates.py" +BASELINE_FILE = "rules/baselines/v2-layers-routes.txt" + + +def _write_files(files: dict[str, str], root: Path) -> None: + for relative_path, source in files.items(): + path = root / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(source)) + + +def _findings(files: dict[str, str], rule_id: str) -> set[str]: + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + return { + str(finding) for finding in scan(root, {}) if finding.rule_id == rule_id + } + + +def _assert_proof(test: unittest.TestCase, rule_id: str) -> None: + for fixture in ("bad_tree.toml", "good_tree.toml"): + tree = tomllib.loads((FIXTURES / rule_id / fixture).read_text()) + with test.subTest(fixture=fixture): + test.assertEqual( + _findings(tree["files"], rule_id), set(tree["expected_findings"]) + ) + bad = tomllib.loads((FIXTURES / rule_id / "bad_tree.toml").read_text()) + test.assertTrue(bad["expected_findings"], "the bad fixture must be rejected") + + +class TestV2LayersProof(unittest.TestCase): + """Proofs: a bad tree gives exactly its expected findings, a good tree none.""" + + def test_rule_proof_files_assigned_to_layers(self) -> None: + _assert_proof(self, FILES_RULE) + + def test_rule_proof_routes_no_sessions_or_sql(self) -> None: + _assert_proof(self, ROUTES_RULE) + + def test_rule_proof_services_no_sql(self) -> None: + _assert_proof(self, SERVICES_RULE) + + def test_rule_proof_connectors_never_commit(self) -> None: + _assert_proof(self, CONNECTORS_RULE) + + +class TestLayerAssignment(unittest.TestCase): + def test_files_are_assigned_by_name(self) -> None: + cases = { + "policyengine_api/services/v2/__init__.py": "package", + "policyengine_api/services/v2/policies/services.py": "service", + "policyengine_api/services/v2/policies/validators.py": "validator", + "policyengine_api/services/v2/policies/transformations.py": ( + "transformation" + ), + "policyengine_api/services/v2/policies/types.py": "types", + "policyengine_api/services/v2/policies/database_session.py": "session", + "policyengine_api/services/v2/policies/database_connectors/reads.py": ( + "connector" + ), + "policyengine_api/services/v2/metadata/database_connectors/" + "reads_parameter_tree.py": "connector", + "policyengine_api/fastapi_routes/v2/routes.py": "route", + "policyengine_api/fastapi_routes/v2/policies/routes.py": "route", + "policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py": "route", + "policyengine_api/fastapi_routes/v2/policies/request_models.py": ( + "http-model" + ), + "policyengine_api/fastapi_routes/v2/policies/response_models.py": ( + "http-model" + ), + } + for path, layer in cases.items(): + with self.subTest(path=path): + self.assertEqual(assign_layer(path, {}), layer) + + def test_other_names_have_no_layer_unless_the_manifest_assigns_one(self) -> None: + unnamed = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + misnamed_connector = ( + "policyengine_api/services/v2/policies/database_connectors/helpers.py" + ) + + self.assertIsNone(assign_layer(unnamed, {})) + self.assertIsNone(assign_layer(misnamed_connector, {})) + self.assertEqual( + assign_layer(unnamed, {unnamed: "http-support"}), "http-support" + ) + + def test_every_file_in_the_repository_has_a_layer(self) -> None: + violations = [ + str(violation) + for violation in run_check(REPO_ROOT) + if violation.rule_id == FILES_RULE + ] + + self.assertEqual(violations, []) + + +class TestCallChecks(unittest.TestCase): + def test_only_calls_on_a_session_count_as_session_calls(self) -> None: + source = """ + def handle(session, request_session, names: set, cache: dict): + names.add("a") + cache.get("b") + request_session.exec("c") + session.get("d") + """ + + self.assertEqual( + _findings({ROUTE: source}, ROUTES_RULE), + { + f"{ROUTE} calls request_session.exec", + f"{ROUTE} calls session.get", + }, + ) + + def test_sql_builders_count_only_when_imported_from_sqlalchemy_or_sqlmodel( + self, + ) -> None: + source = """ + import sqlalchemy as sa + from policyengine_api.services.v2.policies.queries import select + + + def handle(): + select("local helper") + return sa.text("raw") + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + {f"{SERVICE} calls sqlalchemy.text"}, + ) + + def test_imports_under_type_checking_count(self) -> None: + source = """ + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from policyengine_api.services.v2.policies.database_session import ( + PolicyDatabaseSession, + ) + """ + + self.assertEqual( + _findings({ROUTE: source}, ROUTES_RULE), + {f"{ROUTE} imports policyengine_api.services.v2.policies.database_session"}, + ) + + def test_package_and_relative_imports_are_resolved(self) -> None: + source = """ + from policyengine_api.data.v2 import database + from ... import database_session + """ + + self.assertEqual( + _findings({CONNECTOR: source}, CONNECTORS_RULE), + { + f"{CONNECTOR} imports policyengine_api.data.v2.database", + f"{CONNECTOR} imports " + "policyengine_api.services.v2.policies.database_session", + }, + ) + + def test_connectors_may_add_and_flush_but_not_end_a_transaction(self) -> None: + source = """ + def write(session, row): + session.add(row) + session.flush() + session.rollback() + session.close() + """ + + self.assertEqual( + _findings({CONNECTOR: source}, CONNECTORS_RULE), + {f"{CONNECTOR} calls session.rollback", f"{CONNECTOR} calls session.close"}, + ) + + +class TestBaselineAndManifest(unittest.TestCase): + """run_check compares findings with the rule's baseline and checks the manifest.""" + + def setUp(self) -> None: + self._directory = tempfile.TemporaryDirectory() + self.addCleanup(self._directory.cleanup) + self.root = Path(self._directory.name) + + def _registry(self, baseline_count: int | None) -> str: + rules = [] + for rule_id in (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE): + ratcheted = rule_id == ROUTES_RULE and baseline_count is not None + baseline = ( + f'[rule.baseline]\nfile = "{BASELINE_FILE}"\ncount = {baseline_count}\n' + if ratcheted + else "" + ) + rules.append( + textwrap.dedent( + f""" + [[rule]] + id = "{rule_id}" + category = "layers" + statement = "A statement." + rationale = "docs/guide.md" + scope = ["policyengine_api/**/*.py"] + tier = "mechanical" + status = "{"ratcheted" if ratcheted else "enforced"}" + autofix = false + [rule.enforcer] + kind = "quality-guard" + name = "v2-layers" + [[rule.ci]] + workflow = ".github/workflows/pr.yml" + job = "quality-guards" + invocation = "run_quality_guards.py" + """ + ) + + baseline + ) + header = ( + 'schema_version = 1\nrepo = "example"\n\n' + '[[category]]\nid = "layers"\ntitle = "Layers"\n' + ) + return header + "".join(rules) + + def _messages(self) -> list[str]: + return [str(violation) for violation in run_check(self.root)] + + def test_a_finding_the_baseline_lists_passes(self) -> None: + _write_files({ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n"}, self.root) + _write_files( + { + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual(self._messages(), []) + + def test_a_finding_the_baseline_does_not_list_fails(self) -> None: + _write_files({ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n"}, self.root) + _write_files({"rules/registry.toml": self._registry(None)}, self.root) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: {ROUTE} imports sqlalchemy.exc. Fix: " + "v2 route modules call one service method and map its typed " + "failures to HTTP; move session and SQL work into the service and " + "its connectors (docs/engineering/skills/v2-code-organization.md)." + ], + ) + + def test_a_baseline_entry_that_no_longer_applies_fails(self) -> None: + _write_files({ROUTE: "def route():\n return 1\n"}, self.root) + _write_files( + { + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: baseline entry no longer applies: " + f"{ROUTE} imports sqlalchemy.exc. Fix: remove it from " + f"{BASELINE_FILE} and lower the rule's baseline count." + ], + ) + + def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: + unassigned = "policyengine_api/services/v2/policies/helpers.py" + _write_files( + {unassigned: "", "rules/registry.toml": self._registry(None)}, self.root + ) + + self.assertEqual( + self._messages(), + [ + f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " + "name it after its layer (services.py, validators.py, " + "transformations.py, types.py, database_session.py, " + "database_connectors/[_name].py, " + "routes.py, _routes.py, request_models.py or " + "response_models.py), or add it to rules/v2-layer-manifest.toml " + "with its layer and a reason." + ], + ) + + def test_manifest_entries_must_name_existing_unmatched_files_and_known_layers( + self, + ) -> None: + cache = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + _write_files( + { + cache: "", + ROUTE: "", + "rules/registry.toml": self._registry(None), + "rules/v2-layer-manifest.toml": textwrap.dedent( + f""" + [files."{cache}"] + layer = "cache" + reason = "Keeps serialized responses." + + [files."{ROUTE}"] + layer = "route" + reason = "Named routes.py already." + + [files."policyengine_api/services/v2/gone.py"] + layer = "service" + reason = "Removed." + """ + ), + }, + self.root, + ) + + self.assertEqual( + sorted(self._messages()), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{cache} the unknown layer 'cache'. Fix: use one of " + "connector, http-model, http-support, package, process-state, " + "route, service, session, transformation, types, validator.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " + f"{ROUTE}, which its name already assigns to a layer. Fix: " + "remove the entry.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " + "policyengine_api/services/v2/gone.py, which does not exist. " + "Fix: remove the entry.", + ] + ), + ) + + +if __name__ == "__main__": + unittest.main() From 0240ecea014883a04105e0a608de52a1cad8e014 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:09:27 +0400 Subject: [PATCH 02/15] Assign every v2 file to a layer and enforce the v2 layer rules Add the v2-layers quality guard (scripts/rules/v2_layers.py, standard library only), run by scripts/run_quality_guards.py in the quality-guards job. - It discovers every .py file under services/v2 and fastapi_routes/v2 and takes its layer from its file name, or from rules/v2-layer-manifest.toml for the 9 files with other names. A file with neither fails: new rule api.v2.files-assigned-to-layers. - api.v2.routes-no-sessions-or-sql is ratcheted. Its baseline lists the 4 route modules that import sqlalchemy.exc to catch SQLAlchemyError. - api.v2.services-no-sql and api.v2.connectors-never-commit are enforced, with no findings today. - Session calls count only on a receiver named like a session, and SQL builders and session factories only when imported from SQLAlchemy, SQLModel or the v2 database module. Imports under TYPE_CHECKING count. Each rule has proof fixture trees, and v2-code-organization.md cites each rule id. The test fix resolves "from .. import database_session" to the resource's own session module, as Python does. Fixes #4190 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 5 +- changelog.d/4190.added.md | 1 + .../skills/v2-code-organization.md | 29 +- docs/generated/rules.md | 33 +- rules/baselines/v2-layers-routes.txt | 8 + rules/registry.toml | 81 ++- rules/v2-layer-manifest.toml | 50 ++ scripts/rules/tests/test_v2_layers.py | 2 +- scripts/rules/v2_layers.py | 526 ++++++++++++++++++ scripts/run_quality_guards.py | 8 +- 10 files changed, 707 insertions(+), 36 deletions(-) create mode 100644 changelog.d/4190.added.md create mode 100644 rules/baselines/v2-layers-routes.txt create mode 100644 rules/v2-layer-manifest.toml create mode 100644 scripts/rules/v2_layers.py diff --git a/AGENTS.md b/AGENTS.md index 1430243e9..2a03fe24c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,11 +87,8 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### API v2 layer boundaries -- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.routes-import-no-database-access`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` +- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. (doc-only, not checked: follow it yourself) -- `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. (doc-only, not checked: follow it yourself) -- `api.v2.routes-no-sessions-or-sql`: v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. (doc-only, not checked: follow it yourself) -- `api.v2.services-no-sql`: v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. (doc-only, not checked: follow it yourself) ### Runtime persistence and route boundaries diff --git a/changelog.d/4190.added.md b/changelog.d/4190.added.md new file mode 100644 index 000000000..b1569ab95 --- /dev/null +++ b/changelog.d/4190.added.md @@ -0,0 +1 @@ +Check that every API v2 route and service file has a layer, and that v2 route modules open no session and build no SQL, services build and run no SQL, and connectors never open, commit, roll back or close a session, in a new v2-layers quality guard. diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 23a0b5f17..185e6bea0 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -1,8 +1,19 @@ # API v2 Code Organization Organize API v2 code first by resource and then by one explicit technical -responsibility. Route modules must not open database sessions or construct SQL. -Service modules must sequence work but must not construct or execute SQL. +responsibility. Route modules must not open database sessions or construct SQL +(rule `api.v2.routes-no-sessions-or-sql`). Service modules must sequence work +but must not construct or execute SQL (rule `api.v2.services-no-sql`). + +Every `.py` file under `policyengine_api/services/v2` and +`policyengine_api/fastapi_routes/v2` has a layer, which the `v2-layers` quality +guard (`scripts/rules/v2_layers.py`) takes from the file's name: `services.py`, +`validators.py`, `transformations.py`, `types.py`, `database_session.py`, +`database_connectors/[_name].py`, `routes.py`, +`_routes.py`, `request_models.py`, `response_models.py` or `__init__.py`. +A file with another name needs an entry, with its layer and a reason, in +`rules/v2-layer-manifest.toml`, or the guard fails (rule +`api.v2.files-assigned-to-layers`). ## HTTP adapters @@ -40,7 +51,12 @@ individual resource paths. Request models describe HTTP bodies. Response models describe public response response structures and OpenAPI output. Route functions handle HTTP-only conditions, -invoke one service method, and convert typed failures to HTTP responses. +invoke one service method, and convert typed failures to HTTP responses. No module +under `fastapi_routes/v2` imports SQLAlchemy, SQLModel, a connector, a +`database_session` module or `policyengine_api.data.v2.database`, or calls a +SQL builder, a session method or a session factory (rule +`api.v2.routes-no-sessions-or-sql`; today's `sqlalchemy.exc` imports are in its +baseline). ## Resource service packages @@ -86,8 +102,9 @@ or more connector functions. A service may expose an entrypoint that accepts an existing session when several resources must change atomically in one caller- owned transaction. -Do not construct SQL expressions, call `Session.exec`, or define HTTP response -models in this module. +Do not construct SQL expressions, call `Session.exec` or another data-access +session method, or define HTTP response models in this module (rule +`api.v2.services-no-sql`, which also forbids importing FastAPI or Starlette). ### `validators.py` @@ -147,7 +164,7 @@ ORM rows belongs in this package. Organize connector files by CRUD operation: - `deletes.py` removes rows. Connector functions receive a session from the service. They do not open, -commit, roll back, or close it. They do not call route functions, perform +commit, roll back, or close it (rule `api.v2.connectors-never-commit`). They do not call route functions, perform request-level or business validation, or convert rows into public response types. Connector modules do not call one another to sequence a workflow; the service makes that ordering explicit. They never import one another (rule diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 46a6496d3..8ed54623f 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 63 | 3 | 1 | 11 | 78 | +| mechanical | 66 | 4 | 1 | 8 | 79 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 67 of 80 rules. +**Blocking:** 71 of 81 rules. ## Gaps @@ -27,9 +27,6 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. -- `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. -- `api.v2.routes-no-sessions-or-sql`: v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. -- `api.v2.services-no-sql`: v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. ### Advisory (checked but not blocking) @@ -66,6 +63,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.v2.connectors-no-cross-calls`: 7 remaining in `rules/baselines/import-linter-api-v2-connectors-independent.txt` - `api.v2.pure-modules-no-database`: 1 remaining in `rules/baselines/import-linter-api-v2-pure-modules-no-database.txt` +- `api.v2.routes-no-sessions-or-sql`: 4 remaining in `rules/baselines/v2-layers-routes.txt` - `api.v2.types-transformations-no-validators`: 4 remaining in `rules/baselines/import-linter-api-v2-types-transformations-no-validators.txt` ### Exceptions and review dates @@ -84,15 +82,16 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | --- | --- | --- | --- | --- | --- | --- | --- | | `api.routes.shared-query-parameters` | Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. | mechanical | doc-only | no | nothing | — | — | | `api.v2.connectors-mutations-no-reads` | v2 mutation connectors (creates, updates, deletes) never issue select or get reads. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it. | mechanical | doc-only | no | nothing | — | — | +| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | +| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | -| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. | mechanical | doc-only | no | nothing | — | — | -| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. | mechanical | doc-only | no | nothing | — | — | +| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions, call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.session-modules-no-queries` | v2 database_session modules own session lifetime only and never build select, insert, update, delete or exec calls. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.types-transformations-no-validators` | v2 types and transformations modules never import a validators module, directly or through other modules; validators import types, not the reverse. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.user-services-no-sql` | v2 user services (identity, accounts, legacy references and user associations) never build or run SQL themselves or import SQLAlchemy; their connectors do. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | yes | @@ -105,17 +104,19 @@ flowchart LR r_api_v2_connectors_mutations_no_reads("api.v2.connectors-mutations-no-reads (enforced)") e_pytest_tests_unit_v2_test_data_crud_boundaries_py["pytest tests/unit/v2/test_data_crud_boundaries.py"] j_CI_pr_yml_test["CI pr.yml:test"] - r_api_v2_connectors_never_commit("api.v2.connectors-never-commit (doc-only)") + r_api_v2_connectors_never_commit("api.v2.connectors-never-commit (enforced)") + e_quality_guard_v2_layers["quality guard v2-layers"] + j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_v2_connectors_no_cross_calls("api.v2.connectors-no-cross-calls (ratcheted)") e_import_linter_pyproject_toml["import-linter pyproject.toml"] - j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_v2_connectors_reads_no_mutations("api.v2.connectors-reads-no-mutations (enforced)") r_api_v2_crud_checks_cover_every_resource("api.v2.crud-checks-cover-every-resource (enforced)") + r_api_v2_files_assigned_to_layers("api.v2.files-assigned-to-layers (enforced)") r_api_v2_import_layers("api.v2.import-layers (enforced)") r_api_v2_pure_modules_no_database("api.v2.pure-modules-no-database (ratcheted)") r_api_v2_routes_import_no_database_access("api.v2.routes-import-no-database-access (enforced)") - r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (doc-only)") - r_api_v2_services_no_sql("api.v2.services-no-sql (doc-only)") + r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (ratcheted)") + r_api_v2_services_no_sql("api.v2.services-no-sql (enforced)") r_api_v2_session_modules_no_queries("api.v2.session-modules-no-queries (enforced)") r_api_v2_types_transformations_no_validators("api.v2.types-transformations-no-validators (ratcheted)") r_api_v2_user_services_no_sql("api.v2.user-services-no-sql (enforced)") @@ -123,16 +124,18 @@ flowchart LR r_api_routes_shared_query_parameters -.-> gap_unenforced r_api_v2_connectors_mutations_no_reads --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py e_pytest_tests_unit_v2_test_data_crud_boundaries_py --> j_CI_pr_yml_test - r_api_v2_connectors_never_commit -.-> gap_unenforced + r_api_v2_connectors_never_commit --> e_quality_guard_v2_layers + e_quality_guard_v2_layers --> j_CI_pr_yml_quality_guards r_api_v2_connectors_no_cross_calls --> e_import_linter_pyproject_toml e_import_linter_pyproject_toml --> j_CI_pr_yml_quality_guards r_api_v2_connectors_reads_no_mutations --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py r_api_v2_crud_checks_cover_every_resource --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py + r_api_v2_files_assigned_to_layers --> e_quality_guard_v2_layers r_api_v2_import_layers --> e_import_linter_pyproject_toml r_api_v2_pure_modules_no_database --> e_import_linter_pyproject_toml r_api_v2_routes_import_no_database_access --> e_import_linter_pyproject_toml - r_api_v2_routes_no_sessions_or_sql -.-> gap_unenforced - r_api_v2_services_no_sql -.-> gap_unenforced + r_api_v2_routes_no_sessions_or_sql --> e_quality_guard_v2_layers + r_api_v2_services_no_sql --> e_quality_guard_v2_layers r_api_v2_session_modules_no_queries --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py r_api_v2_types_transformations_no_validators --> e_import_linter_pyproject_toml r_api_v2_user_services_no_sql --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py diff --git a/rules/baselines/v2-layers-routes.txt b/rules/baselines/v2-layers-routes.txt new file mode 100644 index 000000000..a7684b874 --- /dev/null +++ b/rules/baselines/v2-layers-routes.txt @@ -0,0 +1,8 @@ +# Tolerated existing violations. This list may only shrink. +# Each line is one finding of the v2-layers guard for rule api.v2.routes-no-sessions-or-sql, +# as the guard prints it. These route modules import sqlalchemy.exc to catch SQLAlchemyError; +# the fix is for their services to raise typed failures instead. +policyengine_api/fastapi_routes/v2/countries/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/households/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/user_resources.py imports sqlalchemy.exc diff --git a/rules/registry.toml b/rules/registry.toml index 2002491f8..4461aca5f 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -180,29 +180,80 @@ workflow = ".github/workflows/pr.yml" job = "test" invocation = "make test" +[[rule]] +id = "api.v2.files-assigned-to-layers" +category = "v2-layers" +statement = "Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it." +rationale = "docs/engineering/skills/v2-code-organization.md#resource-service-packages" +scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_files_assigned_to_layers" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + [[rule]] id = "api.v2.routes-no-sessions-or-sql" category = "v2-layers" -statement = "v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP." +statement = "v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP." rationale = "docs/engineering/skills/v2-code-organization.md#http-adapters" scope = ["policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" -status = "doc-only" +status = "ratcheted" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_routes_no_sessions_or_sql" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" +[rule.baseline] +file = "rules/baselines/v2-layers-routes.txt" +count = 4 [[rule]] id = "api.v2.services-no-sql" category = "v2-layers" -statement = "v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models." +statement = "v2 services sequence work but never construct SQL expressions, call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette)." rationale = "docs/engineering/skills/v2-code-organization.md#servicespy" scope = ["policyengine_api/services/v2/*/services.py"] tier = "mechanical" -status = "doc-only" +status = "enforced" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.services-no-sql/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.services-no-sql/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_services_no_sql" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.v2.user-services-no-sql" @@ -228,14 +279,26 @@ test = "tests/unit/v2/test_data_crud_boundaries.py::test_rule_proof_user_service [[rule]] id = "api.v2.connectors-never-commit" category = "v2-layers" -statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it." +statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v2 database or a database_session module." rationale = "docs/engineering/skills/v2-code-organization.md#database_connectors" scope = ["policyengine_api/services/v2/*/database_connectors/*.py"] tier = "mechanical" -status = "doc-only" +status = "enforced" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.connectors-never-commit/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_connectors_never_commit" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.v2.connectors-no-cross-calls" diff --git a/rules/v2-layer-manifest.toml b/rules/v2-layer-manifest.toml new file mode 100644 index 000000000..985ffcdca --- /dev/null +++ b/rules/v2-layer-manifest.toml @@ -0,0 +1,50 @@ +# Layers for the API v2 files whose names don't give one. +# +# The v2-layers quality guard (scripts/rules/v2_layers.py) assigns every .py +# file under policyengine_api/services/v2 and policyengine_api/fastapi_routes/v2 +# a layer by its file name: services.py, validators.py, transformations.py, +# types.py, database_session.py, database_connectors/[_name].py, +# routes.py, _routes.py, request_models.py, response_models.py or +# __init__.py. A file with another name needs an entry here, or the guard fails +# (rule api.v2.files-assigned-to-layers). Every file under fastapi_routes/v2 +# follows rule api.v2.routes-no-sessions-or-sql whatever its layer. +# +# Layers: http-model (request or response schemas), http-support (shared HTTP +# code for route modules) and process-state (data a service keeps for the +# process, docs/engineering/skills/v2-code-organization.md#resource-service-packages). + +[files."policyengine_api/fastapi_routes/v2/errors.py"] +layer = "http-support" +reason = "The error response and serialization every v2 route shares." + +[files."policyengine_api/fastapi_routes/v2/households/document_models.py"] +layer = "http-model" +reason = "Household document schemas shared by the request and response models." + +[files."policyengine_api/fastapi_routes/v2/metadata/common.py"] +layer = "http-support" +reason = "Runs metadata reads for the route modules and maps their failures to HTTP, and the route class that sets caching headers." + +[files."policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py"] +layer = "http-support" +reason = "Typed FastAPI dependencies for the shared metadata query models." + +[files."policyengine_api/fastapi_routes/v2/metadata/response_cache.py"] +layer = "http-support" +reason = "The process-wide store of serialized whole-version responses and their entity tags." + +[files."policyengine_api/fastapi_routes/v2/metadata/search_file_response.py"] +layer = "http-support" +reason = "The HTTP representation of a stored parameter search file, with content encoding." + +[files."policyengine_api/fastapi_routes/v2/user_auth.py"] +layer = "http-support" +reason = "Resolves request credentials to an account principal for user-owned routes." + +[files."policyengine_api/fastapi_routes/v2/user_resources.py"] +layer = "http-support" +reason = "HTTP conventions shared by the user-owned resource routes." + +[files."policyengine_api/services/v2/metadata/selected_catalogs.py"] +layer = "process-state" +reason = "The catalog versions metadata reads have selected, kept for the process." diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index f4f7fc9dc..5ef2cab93 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -175,7 +175,7 @@ def test_imports_under_type_checking_count(self) -> None: def test_package_and_relative_imports_are_resolved(self) -> None: source = """ from policyengine_api.data.v2 import database - from ... import database_session + from .. import database_session """ self.assertEqual( diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py new file mode 100644 index 000000000..9c10a3f58 --- /dev/null +++ b/scripts/rules/v2_layers.py @@ -0,0 +1,526 @@ +"""The v2-layers quality guard: every v2 route and service file has a layer. + +Every ``.py`` file under ``policyengine_api/services/v2`` and +``policyengine_api/fastapi_routes/v2`` gets a layer from its file name (see +``assign_layer``), or from ``rules/v2-layer-manifest.toml`` for the few files +with other names. A file with neither fails (rule +``api.v2.files-assigned-to-layers``), so a new file is never left unchecked. + +Then each file's imports and calls are checked against its layer's rule: + +- every file under ``fastapi_routes/v2`` (``api.v2.routes-no-sessions-or-sql``) + imports no SQLAlchemy, SQLModel, connector, ``database_session`` or v2 + database module, and calls no SQL builder, session method or session + factory; +- ``services.py`` modules (``api.v2.services-no-sql``) call no SQL builder and + no data-access session method, and import no FastAPI or Starlette; +- connector modules (``api.v2.connectors-never-commit``) don't open, commit, + roll back or close a session, or import the v2 database or a + ``database_session`` module. + +A method call counts as a session call only when its receiver's name contains +``session`` (``session.add`` or ``self._session.exec``, not ``set.add``). A SQL +builder or session factory call counts only when the name was imported from +SQLAlchemy, SQLModel or the v2 database module. Imports made only under +``TYPE_CHECKING`` count. + +A ratcheted rule's baseline file lists its tolerated findings, one per line, as +the guard prints them. A finding the baseline doesn't list fails, and so does +a listed finding that no longer occurs, so the baseline only shrinks. + +The rules: docs/engineering/skills/v2-code-organization.md. Standard library +only. +""" + +from __future__ import annotations + +import ast +import tomllib +from dataclasses import dataclass +from pathlib import Path, PurePosixPath +from typing import TYPE_CHECKING + +from scripts.rules.file_length import tracked_python_files +from scripts.rules.load import load_registry +from scripts.rules.model import Violation + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Mapping + +REPO_ROOT = Path(__file__).resolve().parents[2] +CHECK_NAME = "v2-layers" +MANIFEST_PATH = "rules/v2-layer-manifest.toml" +SERVICES_DIRECTORY = "policyengine_api/services/v2" +ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" +ORGANIZATION_DOC = "docs/engineering/skills/v2-code-organization.md" + +FILES_RULE = "api.v2.files-assigned-to-layers" +ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" +SERVICES_RULE = "api.v2.services-no-sql" +CONNECTORS_RULE = "api.v2.connectors-never-commit" +RULES = (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE) + +SERVICE_FILE_LAYERS = { + "services.py": "service", + "validators.py": "validator", + "transformations.py": "transformation", + "types.py": "types", + "database_session.py": "session", +} +CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") +HTTP_MODEL_FILES = frozenset({"request_models.py", "response_models.py"}) +KNOWN_LAYERS = frozenset( + { + *SERVICE_FILE_LAYERS.values(), + "connector", + "http-model", + "http-support", + "package", + "process-state", + "route", + } +) + +SQL_PACKAGES = ("sqlalchemy", "sqlmodel") +DATABASE_MODULE = "policyengine_api.data.v2.database" +SQL_BUILDERS = frozenset( + { + "delete", + "except_", + "exists", + "insert", + "intersect", + "literal_column", + "select", + "text", + "union", + "union_all", + "update", + "values", + } +) +SESSION_FACTORIES = frozenset( + { + "Session", + "create_engine", + "get_v2_read_session_factory", + "get_v2_session_factory", + "sessionmaker", + } +) +TRANSACTION_METHODS = frozenset( + {"begin", "begin_nested", "close", "commit", "rollback"} +) +DATA_ACCESS_METHODS = frozenset( + { + "add", + "add_all", + "delete", + "exec", + "execute", + "flush", + "get", + "merge", + "query", + "refresh", + "scalar", + "scalars", + } +) +HTTP_PACKAGES = ("fastapi", "starlette") + +FIXES = { + FILES_RULE: ( + "name it after its layer (services.py, validators.py, transformations.py, " + "types.py, database_session.py, " + "database_connectors/[_name].py, routes.py, " + "_routes.py, request_models.py or response_models.py), or add it to " + f"{MANIFEST_PATH} with its layer and a reason" + ), + ROUTES_RULE: ( + "v2 route modules call one service method and map its typed failures to " + "HTTP; move session and SQL work into the service and its connectors " + f"({ORGANIZATION_DOC})" + ), + SERVICES_RULE: ( + "services sequence work: build and run SQL in a connector, pass it the " + f"session, and define HTTP models under fastapi_routes/v2 ({ORGANIZATION_DOC})" + ), + CONNECTORS_RULE: ( + "connectors use the session the service passes them; the service's " + "database_session container opens, commits, rolls back and closes it " + f"({ORGANIZATION_DOC})" + ), +} + + +@dataclass(frozen=True) +class Finding: + """One import or call a layer's rule forbids, or a file with no layer.""" + + rule_id: str + path: str + detail: str + + def __str__(self) -> str: + """Format as the baseline lists it: `` ``.""" + return f"{self.path} {self.detail}" + + +def _within(module: str, package: str) -> bool: + return module == package or module.startswith(f"{package}.") + + +def _module_name(path: str) -> tuple[str, bool]: + parts = list(PurePosixPath(path).with_suffix("").parts) + is_package = parts[-1] == "__init__" + if is_package: + parts = parts[:-1] + return ".".join(parts), is_package + + +def assign_layer(path: str, manifest: Mapping[str, str]) -> str | None: + """Return the layer of the file at ``path`` (repository-relative), or None.""" + if path in manifest: + return manifest[path] + return _layer_by_name(path) + + +def _layer_by_name(path: str) -> str | None: + if path.startswith(f"{SERVICES_DIRECTORY}/"): + return _service_layer(PurePosixPath(path).parts) + if path.startswith(f"{ROUTES_DIRECTORY}/"): + return _route_layer(PurePosixPath(path).name) + return None + + +def _service_layer(parts: tuple[str, ...]) -> str | None: + """Assign a file under services/v2 by name, given its path parts.""" + name = parts[-1] + resource_parts = parts[3:-1] + if name == "__init__.py": + return "package" + if "database_connectors" not in resource_parts: + return SERVICE_FILE_LAYERS.get(name) if resource_parts else None + stem = name.removesuffix(".py") + is_operation = any( + stem == operation or stem.startswith(f"{operation}_") + for operation in CONNECTOR_OPERATIONS + ) + in_connector_package = resource_parts[-1] == "database_connectors" + return "connector" if in_connector_package and is_operation else None + + +def _route_layer(name: str) -> str | None: + """Assign a file under fastapi_routes/v2 by its name.""" + if name == "__init__.py": + return "package" + if name == "routes.py" or name.endswith("_routes.py"): + return "route" + return "http-model" if name in HTTP_MODEL_FILES else None + + +def _from_source(node: ast.ImportFrom, path: str) -> str: + """Return the absolute module a ``from ... import`` reads from.""" + if not node.level: + return node.module or "" + module_name, is_package = _module_name(path) + base = module_name.split(".") + if not is_package: + base = base[:-1] + if node.level > 1: + base = base[: -(node.level - 1)] + return ".".join([*base, *([node.module] if node.module else [])]) + + +def _imports(tree: ast.Module, path: str) -> list[list[str]]: + """Return, per imported module, the names that may refer to it. + + ``from package import name`` yields ``package`` and ``package.name``, + because ``name`` may be a submodule. Relative imports are resolved, and + imports under TYPE_CHECKING count like any other. + """ + statements = [] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + statements.extend([alias.name] for alias in node.names) + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + statements.append( + [source, *(f"{source}.{alias.name}" for alias in node.names)] + ) + return statements + + +def _imported_names(tree: ast.Module, path: str) -> dict[str, str]: + """Map each name a module binds by importing to the dotted name it refers to.""" + names = {} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + bound = alias.asname or alias.name.split(".")[0] + names[bound] = alias.name if alias.asname else bound + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + for alias in node.names: + names[alias.asname or alias.name] = f"{source}.{alias.name}" + return names + + +def _dotted_name(node: ast.expr, imported: Mapping[str, str]) -> str | None: + """Resolve a called name such as ``select`` or ``sa.text`` through the imports.""" + attributes = [] + while isinstance(node, ast.Attribute): + attributes.append(node.attr) + node = node.value + if not isinstance(node, ast.Name) or node.id not in imported: + return None + return ".".join([imported[node.id], *reversed(attributes)]) + + +def _receiver_name(node: ast.expr) -> str: + if isinstance(node, ast.Attribute): + return node.attr + if isinstance(node, ast.Name): + return node.id + return "" + + +def _forbidden_import( + candidates: Iterable[str], is_forbidden: Callable[[str], bool] +) -> str | None: + return next( + (candidate for candidate in candidates if is_forbidden(candidate)), None + ) + + +def _is_route_forbidden_import(module: str) -> bool: + return ( + any(_within(module, package) for package in SQL_PACKAGES) + or _within(module, DATABASE_MODULE) + or _is_session_or_connector_module(module) + ) + + +def _is_session_or_connector_module(module: str) -> bool: + parts = module.split(".") + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in parts or "database_session" in parts + ) + + +def _is_connector_forbidden_import(module: str) -> bool: + parts = module.split(".") + return _within(module, DATABASE_MODULE) or ( + _within(module, "policyengine_api.services.v2") and "database_session" in parts + ) + + +def _is_http_import(module: str) -> bool: + return any(_within(module, package) for package in HTTP_PACKAGES) + + +def _is_sql_name(dotted: str) -> bool: + return any(_within(dotted, package) for package in SQL_PACKAGES) + + +@dataclass(frozen=True) +class LayerPolicy: + """What one layer's rule forbids: imports, session methods and call kinds.""" + + rule_id: str + forbidden_import: Callable[[str], bool] + session_methods: frozenset[str] + checks_builders: bool + checks_factories: bool + + +ROUTE_POLICY = LayerPolicy( + ROUTES_RULE, + _is_route_forbidden_import, + DATA_ACCESS_METHODS | TRANSACTION_METHODS, + checks_builders=True, + checks_factories=True, +) +LAYER_POLICIES = { + "service": LayerPolicy( + SERVICES_RULE, + _is_http_import, + DATA_ACCESS_METHODS, + checks_builders=True, + checks_factories=False, + ), + "connector": LayerPolicy( + CONNECTORS_RULE, + _is_connector_forbidden_import, + TRANSACTION_METHODS, + checks_builders=False, + checks_factories=True, + ), +} + + +def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: + """Return the findings for one call to an imported name. + + For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. + """ + last = dotted.rsplit(".", 1)[-1] + details = [] + if policy.checks_builders and _is_sql_name(dotted) and last in SQL_BUILDERS: + details.append(f"calls {dotted}") + from_database_code = _is_sql_name(dotted) or _within( + dotted, "policyengine_api.data" + ) + if policy.checks_factories and last in SESSION_FACTORIES and from_database_code: + details.append(f"calls {last}") + return details + + +def _session_call_detail(call: ast.Call, policy: LayerPolicy) -> str | None: + """Return ``calls .`` for a forbidden method on a session.""" + if not isinstance(call.func, ast.Attribute): + return None + if call.func.attr not in policy.session_methods: + return None + receiver = _receiver_name(call.func.value) + if "session" not in receiver.lower(): + return None + return f"calls {receiver}.{call.func.attr}" + + +def _file_findings(path: str, source: str, layer: str) -> list[tuple[str, str]]: + """Return (rule id, detail) for each forbidden import and call in one file.""" + policy = ( + ROUTE_POLICY + if path.startswith(f"{ROUTES_DIRECTORY}/") + else LAYER_POLICIES.get(layer) + ) + if policy is None: + return [] + tree = ast.parse(source) + details = [ + f"imports {module}" + for candidates in _imports(tree, path) + if (module := _forbidden_import(candidates, policy.forbidden_import)) + ] + imported = _imported_names(tree, path) + for call in (node for node in ast.walk(tree) if isinstance(node, ast.Call)): + dotted = _dotted_name(call.func, imported) + if dotted is not None: + details.extend(_named_call_details(dotted, policy)) + session_call = _session_call_detail(call, policy) + if session_call is not None: + details.append(session_call) + return [(policy.rule_id, detail) for detail in details] + + +def _discovered_files(root: Path) -> list[str]: + return [ + path + for path in tracked_python_files(root) + if path.startswith((f"{SERVICES_DIRECTORY}/", f"{ROUTES_DIRECTORY}/")) + ] + + +def scan( + root: Path, manifest: Mapping[str, str], paths: Iterable[str] | None = None +) -> list[Finding]: + """Return every finding for the v2 files under ``root``, sorted.""" + findings = set() + for path in paths if paths is not None else _discovered_files(root): + layer = assign_layer(path, manifest) + if layer is None: + findings.add(Finding(FILES_RULE, path, "matches no v2 layer")) + continue + source = (root / path).read_text(encoding="utf-8") + findings.update( + Finding(rule_id, path, detail) + for rule_id, detail in _file_findings(path, source, layer) + ) + return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) + + +def load_manifest(root: Path) -> tuple[dict[str, str], list[Violation]]: + """Read the manifest's file layers, and report entries that don't belong.""" + manifest_file = root / MANIFEST_PATH + if not manifest_file.is_file(): + return {}, [] + entries = tomllib.loads(manifest_file.read_text()).get("files", {}) + manifest = {} + violations = [] + for path, entry in sorted(entries.items()): + layer = entry.get("layer") if isinstance(entry, dict) else None + manifest[path] = str(layer) + if not (root / path).is_file(): + violations.append( + Violation( + FILES_RULE, + CHECK_NAME, + f"{MANIFEST_PATH} lists {path}, which does not exist", + "remove the entry", + ) + ) + elif _layer_by_name(path) is not None: + violations.append( + Violation( + FILES_RULE, + CHECK_NAME, + f"{MANIFEST_PATH} lists {path}, which its name already assigns " + "to a layer", + "remove the entry", + ) + ) + if layer not in KNOWN_LAYERS: + violations.append( + Violation( + FILES_RULE, + CHECK_NAME, + f"{MANIFEST_PATH} gives {path} the unknown layer '{layer}'", + f"use one of {', '.join(sorted(KNOWN_LAYERS))}", + ) + ) + return manifest, violations + + +def _baseline_entries(root: Path, baseline_file: str | None) -> set[str]: + if baseline_file is None or not (root / baseline_file).is_file(): + return set() + return { + line.strip() + for line in (root / baseline_file).read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#") + } + + +def run_check(root: Path = REPO_ROOT) -> list[Violation]: + """Scan the v2 files under ``root`` and compare with each rule's baseline.""" + registry, violations = load_registry(root) + if registry is None: + return violations + manifest, violations = load_manifest(root) + findings = scan(root, manifest) + for rule_id in RULES: + rule = registry.rule(rule_id) + baseline_file = rule.baseline.file if rule and rule.baseline else None + tolerated = _baseline_entries(root, baseline_file) + found = {str(finding) for finding in findings if finding.rule_id == rule_id} + violations.extend( + Violation(rule_id, CHECK_NAME, entry, FIXES[rule_id]) + for entry in sorted(found - tolerated) + ) + violations.extend( + Violation( + rule_id, + CHECK_NAME, + f"baseline entry no longer applies: {entry}", + f"remove it from {baseline_file} and lower the rule's baseline count", + ) + for entry in sorted(tolerated - found) + ) + return violations + + +def check() -> list[str]: + """Adapter for scripts/run_quality_guards.py.""" + return [str(violation) for violation in run_check(REPO_ROOT)] diff --git a/scripts/run_quality_guards.py b/scripts/run_quality_guards.py index 9ac66caf5..888152db9 100644 --- a/scripts/run_quality_guards.py +++ b/scripts/run_quality_guards.py @@ -11,7 +11,12 @@ sys.path.insert(0, str(REPO_ROOT)) from scripts.guards import migration_contracts # noqa: E402 -from scripts.rules import ci_scripts, file_length, locked_installs # noqa: E402 +from scripts.rules import ( # noqa: E402 + ci_scripts, + file_length, + locked_installs, + v2_layers, +) from scripts.rules import guard as rules_guard # noqa: E402 Guard = tuple[str, Callable[[], list[str]]] @@ -22,6 +27,7 @@ ("ci-scripts", ci_scripts.check), ("locked-installs", locked_installs.check), ("file-length", file_length.check), + ("v2-layers", v2_layers.check), ) From 0af97a199e428e947fc3f8e174a8bba147aba6a9 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:27:25 +0400 Subject: [PATCH 03/15] Test the review findings on the v2-layers guard (failing) - Validators, transformations, types, process-state modules and package __init__ files touch no session: new rule api.v2.pure-modules-no-session-calls, with proof fixtures. A database_connectors/__init__.py counts as a connector. - The repository's own session factories count: the v1 ones in policyengine_api.data.orm, build_v2_session_factory, the engines, and a database_session container's transaction() in a route. - Services import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc, so and_, or_, func and col count, as does a select() on a model's __table__. - A name that only contains "session" (sessions_router, session_ids, request.session) is not a session. - Each occurrence of a finding needs its own baseline entry, and a duplicated entry is stale. - The manifest gives only the layers of the file's directory, never "package", and every entry needs a reason. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 33 +++ .../good_tree.toml | 19 ++ .../api.v2.services-no-sql/bad_tree.toml | 5 +- scripts/rules/tests/test_v2_layers.py | 215 +++++++++++++++++- 4 files changed, 267 insertions(+), 5 deletions(-) create mode 100644 rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml create mode 100644 rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml new file mode 100644 index 000000000..234c495cc --- /dev/null +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -0,0 +1,33 @@ +# Proof fixture for rule api.v2.pure-modules-no-session-calls. The guard must +# reject it: a validator loads a row through the session it was handed, a +# process-state module runs a query, and a package __init__ commits. +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/policies/__init__.py calls session.commit", + "policyengine_api/services/v2/policies/types.py calls session.exec", + "policyengine_api/services/v2/policies/types.py calls sqlmodel.select", + "policyengine_api/services/v2/policies/validators.py calls session.get", +] + +[files] +"policyengine_api/services/v2/policies/validators.py" = """ +from policyengine_api.data.v2.models import Policy + + +def validate_policy_exists(session, policy_id): + if session.get(Policy, policy_id) is None: + raise LookupError(policy_id) +""" +"policyengine_api/services/v2/policies/types.py" = """ +from sqlmodel import select + + +def latest(session, model): + return session.exec(select(model)).first() +""" +"policyengine_api/services/v2/policies/__init__.py" = """ +def flush_everything(session): + session.commit() +""" diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml new file mode 100644 index 000000000..900fdd36d --- /dev/null +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml @@ -0,0 +1,19 @@ +# Proof fixture for rule api.v2.pure-modules-no-session-calls. The guard must +# accept it: the validator checks a row a connector already loaded, and the +# package __init__ holds only a docstring. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/validators.py" = """ +from policyengine_api.data.v2.models import Policy + + +def validate_policy_exists(policy: Policy | None, policy_id): + if policy is None: + raise LookupError(policy_id) + return policy +""" +"policyengine_api/services/v2/policies/__init__.py" = """ +\"\"\"Immutable v2 policies.\"\"\" +""" diff --git a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml index c2e8f8abe..4bd955ed0 100644 --- a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml +++ b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml @@ -1,11 +1,12 @@ # Proof fixture for rule api.v2.services-no-sql. The guard must reject it: the -# service builds a select, runs it with Session.exec and defines an HTTP -# response with FastAPI. +# service imports and builds a select, runs it with Session.exec and defines +# an HTTP response with FastAPI. expected_findings = [ "policyengine_api/services/v2/policies/services.py calls _session.exec", "policyengine_api/services/v2/policies/services.py calls sqlmodel.select", "policyengine_api/services/v2/policies/services.py imports fastapi.responses", + "policyengine_api/services/v2/policies/services.py imports sqlmodel.select", ] [files] diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 5ef2cab93..d2282d7d6 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -11,6 +11,7 @@ from scripts.rules.v2_layers import ( CONNECTORS_RULE, FILES_RULE, + PURE_RULE, ROUTES_RULE, SERVICES_RULE, assign_layer, @@ -68,6 +69,9 @@ def test_rule_proof_services_no_sql(self) -> None: def test_rule_proof_connectors_never_commit(self) -> None: _assert_proof(self, CONNECTORS_RULE) + def test_rule_proof_pure_modules_no_session_calls(self) -> None: + _assert_proof(self, PURE_RULE) + class TestLayerAssignment(unittest.TestCase): def test_files_are_assigned_by_name(self) -> None: @@ -202,6 +206,126 @@ def write(session, row): ) +class TestEveryLayerIsChecked(unittest.TestCase): + """Only connectors and session modules may touch a session (review findings).""" + + SESSION_WORK = """ + from sqlmodel import select + + + def work(session, row): + session.add(row) + session.commit() + return session.exec(select(row)) + """ + + def test_validators_transformations_types_and_process_state_touch_no_session( + self, + ) -> None: + for path in ( + "policyengine_api/services/v2/policies/validators.py", + "policyengine_api/services/v2/policies/transformations.py", + "policyengine_api/services/v2/policies/types.py", + "policyengine_api/services/v2/policies/__init__.py", + ): + with self.subTest(path=path): + self.assertEqual( + _findings({path: self.SESSION_WORK}, PURE_RULE), + { + f"{path} calls session.add", + f"{path} calls session.commit", + f"{path} calls session.exec", + f"{path} calls sqlmodel.select", + }, + ) + + def test_a_connector_package_init_is_a_connector(self) -> None: + path = "policyengine_api/services/v2/policies/database_connectors/__init__.py" + + self.assertEqual( + _findings({path: self.SESSION_WORK}, CONNECTORS_RULE), + {f"{path} calls session.commit"}, + ) + + def test_the_repository_session_factories_count(self) -> None: + route_source = """ + from policyengine_api.data.orm import get_v1_session_factory + from policyengine_api.data.v2.database import build_v2_session_factory + + + def route(database_session): + with database_session.transaction() as session: + get_v1_session_factory() + build_v2_session_factory() + """ + connector_source = """ + from policyengine_api.data.orm import get_v1_engine + + + def read(): + return get_v1_engine() + """ + + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + { + f"{ROUTE} calls build_v2_session_factory", + f"{ROUTE} calls database_session.transaction", + f"{ROUTE} calls get_v1_session_factory", + f"{ROUTE} imports policyengine_api.data.orm", + f"{ROUTE} imports policyengine_api.data.v2.database", + }, + ) + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls get_v1_engine", + f"{CONNECTOR} imports policyengine_api.data.orm", + }, + ) + + def test_services_import_only_the_session_type_and_errors_from_sql_packages( + self, + ) -> None: + source = """ + from typing import TYPE_CHECKING + + from sqlalchemy import and_, func, or_ + from sqlalchemy.exc import IntegrityError + from sqlmodel import col + + if TYPE_CHECKING: + from sqlmodel import Session + + + def build(Policy, ids): + Policy.__table__.select() + return and_(col(Policy.id).in_(ids), or_(func.lower(Policy.name))) + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + { + f"{SERVICE} calls Policy.__table__.select", + f"{SERVICE} imports sqlalchemy.and_", + f"{SERVICE} imports sqlalchemy.func", + f"{SERVICE} imports sqlalchemy.or_", + f"{SERVICE} imports sqlmodel.col", + }, + ) + + def test_names_that_only_contain_session_are_not_sessions(self) -> None: + source = """ + @sessions_router.get("/sessions") + def route(request, session_ids: set): + session_ids.add(1) + request.session.get("user") + return self._sessions_by_country.get("us") + """ + + self.assertEqual(_findings({ROUTE: source}, ROUTES_RULE), set()) + + class TestBaselineAndManifest(unittest.TestCase): """run_check compares findings with the rule's baseline and checks the manifest.""" @@ -277,6 +401,91 @@ def test_a_finding_the_baseline_does_not_list_fails(self) -> None: ], ) + def test_each_occurrence_needs_its_own_baseline_entry(self) -> None: + _write_files( + { + ROUTE: ( + "from sqlalchemy.exc import SQLAlchemyError\n" + "def later():\n" + " from sqlalchemy.exc import IntegrityError\n" + ), + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: {ROUTE} imports sqlalchemy.exc (2 " + "occurrences, the baseline tolerates 1). Fix: v2 route modules call " + "one service method and map its typed failures to HTTP; move session " + "and SQL work into the service and its connectors " + "(docs/engineering/skills/v2-code-organization.md)." + ], + ) + + def test_a_duplicated_baseline_entry_is_stale(self) -> None: + _write_files( + { + ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n", + "rules/registry.toml": self._registry(2), + BASELINE_FILE: ( + f"{ROUTE} imports sqlalchemy.exc\n{ROUTE} imports sqlalchemy.exc\n" + ), + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: baseline entry no longer applies: " + f"{ROUTE} imports sqlalchemy.exc (1 occurrence, the baseline lists " + f"2). Fix: remove it from {BASELINE_FILE} and lower the rule's " + "baseline count." + ], + ) + + def test_the_manifest_gives_only_layers_of_the_file_s_directory(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + _write_files( + { + client: "", + support: "", + "rules/registry.toml": self._registry(None), + "rules/v2-layer-manifest.toml": textwrap.dedent( + f""" + [files."{client}"] + layer = "package" + reason = "Not a package." + + [files."{support}"] + layer = "http-support" + reason = "" + """ + ), + }, + self.root, + ) + + self.assertEqual( + sorted(self._messages()), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{client} the layer 'package', which a file under " + "policyengine_api/services/v2 can't have from the manifest. Fix: " + "use one of process-state, service, transformation, types, " + "validator.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{support} no reason. Fix: say why the file has that layer.", + ] + ), + ) + def test_a_baseline_entry_that_no_longer_applies_fails(self) -> None: _write_files({ROUTE: "def route():\n return 1\n"}, self.root) _write_files( @@ -348,9 +557,9 @@ def test_manifest_entries_must_name_existing_unmatched_files_and_known_layers( sorted( [ f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " - f"{cache} the unknown layer 'cache'. Fix: use one of " - "connector, http-model, http-support, package, process-state, " - "route, service, session, transformation, types, validator.", + f"{cache} the layer 'cache', which a file under " + "policyengine_api/fastapi_routes/v2 can't have from the " + "manifest. Fix: use one of http-model, http-support, route.", f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " f"{ROUTE}, which its name already assigns to a layer. Fix: " "remove the entry.", From 92c10f85b324b70f93bbf6a688d4b2a4bae5f797 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:31:19 +0400 Subject: [PATCH 04/15] Check every v2 layer's calls and tighten the v2-layers guard - New rule api.v2.pure-modules-no-session-calls: validators, transformations, types, process-state modules and package __init__ files call no session method, SQL builder or session factory. A database_connectors/__init__.py is checked as a connector. - Session factories are any call into policyengine_api.data.orm or policyengine_api.data.v2.database, or SQLAlchemy's Session, sessionmaker and create_engine. Routes and connectors may not import the v1 ORM module, and a route may not call a database_session container's read() or transaction(). - Services import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc, so and_, or_, func and col are caught, and a builder called on a model's __table__ counts. - A session receiver is named session or _session, and request.session and http_session are not sessions, so sessions_router and session_ids are not flagged. - Each occurrence of a finding needs its own baseline line, and a duplicated line is reported as no longer applying. - The manifest may give only the layers of the file's directory, and every entry needs a reason. - v2-code-organization.md has a "File layers" section, which the files rule's rationale now links to. The real code still has exactly the 4 tolerated sqlalchemy.exc imports. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- .../skills/v2-code-organization.md | 18 +- docs/generated/rules.md | 13 +- rules/registry.toml | 32 +- scripts/rules/tests/test_v2_layers.py | 2 +- scripts/rules/v2_layers.py | 471 ++++++++++++------ 6 files changed, 368 insertions(+), 170 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2a03fe24c..10d78c084 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### API v2 layer boundaries -- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` +- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.pure-modules-no-session-calls`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. (doc-only, not checked: follow it yourself) ### Runtime persistence and route boundaries diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 185e6bea0..6694b018f 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -5,6 +5,9 @@ responsibility. Route modules must not open database sessions or construct SQL (rule `api.v2.routes-no-sessions-or-sql`). Service modules must sequence work but must not construct or execute SQL (rule `api.v2.services-no-sql`). + +## File layers + Every `.py` file under `policyengine_api/services/v2` and `policyengine_api/fastapi_routes/v2` has a layer, which the `v2-layers` quality guard (`scripts/rules/v2_layers.py`) takes from the file's name: `services.py`, @@ -13,7 +16,17 @@ guard (`scripts/rules/v2_layers.py`) takes from the file's name: `services.py`, `_routes.py`, `request_models.py`, `response_models.py` or `__init__.py`. A file with another name needs an entry, with its layer and a reason, in `rules/v2-layer-manifest.toml`, or the guard fails (rule -`api.v2.files-assigned-to-layers`). +`api.v2.files-assigned-to-layers`). The manifest can give a file under +`services/v2` the layer `service`, `validator`, `transformation`, `types` or +`process-state`, and a file under `fastapi_routes/v2` the layer `route`, +`http-model` or `http-support`; each layer follows that layer's rules below. + +The guard counts a method call as a session call when its receiver is named +`session` or `_session` (not `request.session` or `http_session`), +a SQL builder when it was imported from SQLAlchemy or SQLModel or is called on +a model's `__table__`, and a session factory when it comes from +`policyengine_api.data.orm` or `policyengine_api.data.v2.database`. A +ratcheted rule's baseline lists one line per tolerated occurrence. ## HTTP adapters @@ -111,6 +124,9 @@ session method, or define HTTP response models in this module (rule Define functions that inspect already-available values and either return a validated value or raise a typed exception. Validators must not load records, open sessions, execute SQL, mutate database rows, or construct HTTP responses. +Validators, transformations, types, process-state modules and package +`__init__` files call no session method, SQL builder or session factory (rule +`api.v2.pure-modules-no-session-calls`). When validation depends on stored state, a database connector loads the required rows and the service passes those rows to a validator. For example, diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 8ed54623f..c590d46fc 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 66 | 4 | 1 | 8 | 79 | +| mechanical | 67 | 4 | 1 | 8 | 80 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 71 of 81 rules. +**Blocking:** 72 of 82 rules. ## Gaps @@ -82,16 +82,17 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | --- | --- | --- | --- | --- | --- | --- | --- | | `api.routes.shared-query-parameters` | Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. | mechanical | doc-only | no | nothing | — | — | | `api.v2.connectors-mutations-no-reads` | v2 mutation connectors (creates, updates, deletes) never issue select or get reads. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | +| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules and package __init__ files call no session method, SQL builder or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | -| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | -| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions, call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.session-modules-no-queries` | v2 database_session modules own session lifetime only and never build select, insert, update, delete or exec calls. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.types-transformations-no-validators` | v2 types and transformations modules never import a validators module, directly or through other modules; validators import types, not the reverse. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.user-services-no-sql` | v2 user services (identity, accounts, legacy references and user associations) never build or run SQL themselves or import SQLAlchemy; their connectors do. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | yes | @@ -114,6 +115,7 @@ flowchart LR r_api_v2_files_assigned_to_layers("api.v2.files-assigned-to-layers (enforced)") r_api_v2_import_layers("api.v2.import-layers (enforced)") r_api_v2_pure_modules_no_database("api.v2.pure-modules-no-database (ratcheted)") + r_api_v2_pure_modules_no_session_calls("api.v2.pure-modules-no-session-calls (enforced)") r_api_v2_routes_import_no_database_access("api.v2.routes-import-no-database-access (enforced)") r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (ratcheted)") r_api_v2_services_no_sql("api.v2.services-no-sql (enforced)") @@ -133,6 +135,7 @@ flowchart LR r_api_v2_files_assigned_to_layers --> e_quality_guard_v2_layers r_api_v2_import_layers --> e_import_linter_pyproject_toml r_api_v2_pure_modules_no_database --> e_import_linter_pyproject_toml + r_api_v2_pure_modules_no_session_calls --> e_quality_guard_v2_layers r_api_v2_routes_import_no_database_access --> e_import_linter_pyproject_toml r_api_v2_routes_no_sessions_or_sql --> e_quality_guard_v2_layers r_api_v2_services_no_sql --> e_quality_guard_v2_layers diff --git a/rules/registry.toml b/rules/registry.toml index 4461aca5f..e3f14fe70 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -184,7 +184,7 @@ invocation = "make test" id = "api.v2.files-assigned-to-layers" category = "v2-layers" statement = "Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it." -rationale = "docs/engineering/skills/v2-code-organization.md#resource-service-packages" +rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" status = "enforced" @@ -207,7 +207,7 @@ invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.v2.routes-no-sessions-or-sql" category = "v2-layers" -statement = "v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP." +statement = "v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP." rationale = "docs/engineering/skills/v2-code-organization.md#http-adapters" scope = ["policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" @@ -234,7 +234,7 @@ count = 4 [[rule]] id = "api.v2.services-no-sql" category = "v2-layers" -statement = "v2 services sequence work but never construct SQL expressions, call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette)." +statement = "v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette)." rationale = "docs/engineering/skills/v2-code-organization.md#servicespy" scope = ["policyengine_api/services/v2/*/services.py"] tier = "mechanical" @@ -276,10 +276,34 @@ bad_fixture = "rules/fixtures/api.v2.user-services-no-sql/bad_services.py" good_fixture = "rules/fixtures/api.v2.user-services-no-sql/good_services.py" test = "tests/unit/v2/test_data_crud_boundaries.py::test_rule_proof_user_services_no_sql" +[[rule]] +id = "api.v2.pure-modules-no-session-calls" +category = "v2-layers" +statement = "v2 validators, transformations, types, process-state modules and package __init__ files call no session method, SQL builder or session factory; they work on values and rows a connector loaded." +rationale = "docs/engineering/skills/v2-code-organization.md#validatorspy" +scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/metadata/selected_catalogs.py", "policyengine_api/services/v2/**/__init__.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_pure_modules_no_session_calls" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + [[rule]] id = "api.v2.connectors-never-commit" category = "v2-layers" -statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v2 database or a database_session module." +statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module." rationale = "docs/engineering/skills/v2-code-organization.md#database_connectors" scope = ["policyengine_api/services/v2/*/database_connectors/*.py"] tier = "mechanical" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index d2282d7d6..6cba50551 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -158,7 +158,7 @@ def handle(): self.assertEqual( _findings({SERVICE: source}, SERVICES_RULE), - {f"{SERVICE} calls sqlalchemy.text"}, + {f"{SERVICE} calls sqlalchemy.text", f"{SERVICE} imports sqlalchemy"}, ) def test_imports_under_type_checking_count(self) -> None: diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 9c10a3f58..e20092170 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -9,24 +9,37 @@ Then each file's imports and calls are checked against its layer's rule: - every file under ``fastapi_routes/v2`` (``api.v2.routes-no-sessions-or-sql``) - imports no SQLAlchemy, SQLModel, connector, ``database_session`` or v2 - database module, and calls no SQL builder, session method or session + imports no SQLAlchemy, SQLModel, connector, ``database_session``, v1 ORM or + v2 database module, and calls no SQL builder, session method or session factory; -- ``services.py`` modules (``api.v2.services-no-sql``) call no SQL builder and - no data-access session method, and import no FastAPI or Starlette; -- connector modules (``api.v2.connectors-never-commit``) don't open, commit, - roll back or close a session, or import the v2 database or a - ``database_session`` module. - -A method call counts as a session call only when its receiver's name contains -``session`` (``session.add`` or ``self._session.exec``, not ``set.add``). A SQL -builder or session factory call counts only when the name was imported from -SQLAlchemy, SQLModel or the v2 database module. Imports made only under +- ``services.py`` modules (``api.v2.services-no-sql``) import nothing from + SQLAlchemy or SQLModel but the ``Session`` type and ``sqlalchemy.exc``, call + no SQL builder or data-access session method, and import no FastAPI or + Starlette; +- connector modules, and a ``database_connectors/__init__.py`` + (``api.v2.connectors-never-commit``), don't open, commit, roll back or close + a session, call no session factory, and import no v1 ORM, v2 database or + ``database_session`` module; +- validators, transformations, types, process-state modules and package + ``__init__`` files (``api.v2.pure-modules-no-session-calls``) call no session + method, SQL builder or session factory. + +``database_session.py`` modules own the session's lifetime; rule +``api.v2.session-modules-no-queries`` checks them. + +A method call counts as a session call when its receiver is named ``session`` +or ``_session`` (``session.add``, ``self._session.exec``), but not +``request.session`` (Starlette's session data), ``http_session`` or names that +only contain the word (``sessions_router``). A SQL builder counts when it was +imported from SQLAlchemy or SQLModel, or is called on a model's ``__table__``; +a session factory, when it was imported from ``policyengine_api.data.orm`` or +``policyengine_api.data.v2.database``, or is ``Session``, ``sessionmaker`` or +``create_engine`` from SQLAlchemy or SQLModel. Imports made only under ``TYPE_CHECKING`` count. -A ratcheted rule's baseline file lists its tolerated findings, one per line, as -the guard prints them. A finding the baseline doesn't list fails, and so does -a listed finding that no longer occurs, so the baseline only shrinks. +A ratcheted rule's baseline file lists its tolerated findings, one line per +occurrence, as the guard prints them. More occurrences than the baseline lists +fail, and so do fewer, so the baseline only shrinks. The rules: docs/engineering/skills/v2-code-organization.md. Standard library only. @@ -36,6 +49,7 @@ import ast import tomllib +from collections import Counter from dataclasses import dataclass from pathlib import Path, PurePosixPath from typing import TYPE_CHECKING @@ -58,7 +72,8 @@ ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" SERVICES_RULE = "api.v2.services-no-sql" CONNECTORS_RULE = "api.v2.connectors-never-commit" -RULES = (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE) +PURE_RULE = "api.v2.pure-modules-no-session-calls" +RULES = (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE, PURE_RULE) SERVICE_FILE_LAYERS = { "services.py": "service", @@ -69,20 +84,23 @@ } CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") HTTP_MODEL_FILES = frozenset({"request_models.py", "response_models.py"}) -KNOWN_LAYERS = frozenset( - { - *SERVICE_FILE_LAYERS.values(), - "connector", - "http-model", - "http-support", - "package", - "process-state", - "route", - } +# Layers the manifest may give, by directory. Connectors, session modules and +# packages get their layer from their names only. +MANIFEST_LAYERS = { + SERVICES_DIRECTORY: frozenset( + {"process-state", "service", "transformation", "types", "validator"} + ), + ROUTES_DIRECTORY: frozenset({"http-model", "http-support", "route"}), +} +PURE_LAYERS = frozenset( + {"package", "process-state", "transformation", "types", "validator"} ) SQL_PACKAGES = ("sqlalchemy", "sqlmodel") -DATABASE_MODULE = "policyengine_api.data.v2.database" +DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") +# What a service may import from SQLAlchemy or SQLModel: the session type, for +# annotations, and the exception classes, to raise typed failures. +SERVICE_SQL_IMPORTS = ("sqlmodel.Session", "sqlalchemy.orm.Session", "sqlalchemy.exc") SQL_BUILDERS = frozenset( { "delete", @@ -99,15 +117,7 @@ "values", } ) -SESSION_FACTORIES = frozenset( - { - "Session", - "create_engine", - "get_v2_read_session_factory", - "get_v2_session_factory", - "sessionmaker", - } -) +SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) TRANSACTION_METHODS = frozenset( {"begin", "begin_nested", "close", "commit", "rollback"} ) @@ -127,6 +137,9 @@ "scalars", } ) +# A database_session container's context managers. +CONTAINER_METHODS = frozenset({"read", "transaction"}) +ALL_SESSION_METHODS = DATA_ACCESS_METHODS | TRANSACTION_METHODS | CONTAINER_METHODS HTTP_PACKAGES = ("fastapi", "starlette") FIXES = { @@ -151,6 +164,11 @@ "database_session container opens, commits, rolls back and closes it " f"({ORGANIZATION_DOC})" ), + PURE_RULE: ( + "validators, transformations and types work on values and loaded rows; " + "load them in a connector and pass them in from the service " + f"({ORGANIZATION_DOC})" + ), } @@ -171,6 +189,10 @@ def _within(module: str, package: str) -> bool: return module == package or module.startswith(f"{package}.") +def _within_any(module: str, packages: Iterable[str]) -> bool: + return any(_within(module, package) for package in packages) + + def _module_name(path: str) -> tuple[str, bool]: parts = list(PurePosixPath(path).with_suffix("").parts) is_package = parts[-1] == "__init__" @@ -179,6 +201,9 @@ def _module_name(path: str) -> tuple[str, bool]: return ".".join(parts), is_package +# --- layers --------------------------------------------------------------- + + def assign_layer(path: str, manifest: Mapping[str, str]) -> str | None: """Return the layer of the file at ``path`` (repository-relative), or None.""" if path in manifest: @@ -198,8 +223,11 @@ def _service_layer(parts: tuple[str, ...]) -> str | None: """Assign a file under services/v2 by name, given its path parts.""" name = parts[-1] resource_parts = parts[3:-1] + in_connector_package = bool(resource_parts) and ( + resource_parts[-1] == "database_connectors" + ) if name == "__init__.py": - return "package" + return "connector" if in_connector_package else "package" if "database_connectors" not in resource_parts: return SERVICE_FILE_LAYERS.get(name) if resource_parts else None stem = name.removesuffix(".py") @@ -207,7 +235,6 @@ def _service_layer(parts: tuple[str, ...]) -> str | None: stem == operation or stem.startswith(f"{operation}_") for operation in CONNECTOR_OPERATIONS ) - in_connector_package = resource_parts[-1] == "database_connectors" return "connector" if in_connector_package and is_operation else None @@ -220,6 +247,9 @@ def _route_layer(name: str) -> str | None: return "http-model" if name in HTTP_MODEL_FILES else None +# --- imports -------------------------------------------------------------- + + def _from_source(node: ast.ImportFrom, path: str) -> str: """Return the absolute module a ``from ... import`` reads from.""" if not node.level: @@ -233,22 +263,27 @@ def _from_source(node: ast.ImportFrom, path: str) -> str: return ".".join([*base, *([node.module] if node.module else [])]) -def _imports(tree: ast.Module, path: str) -> list[list[str]]: - """Return, per imported module, the names that may refer to it. +@dataclass(frozen=True) +class ImportStatement: + """One imported module, or one ``from`` import with the names it binds.""" + + module: str + names: tuple[str, ...] + - ``from package import name`` yields ``package`` and ``package.name``, - because ``name`` may be a submodule. Relative imports are resolved, and - imports under TYPE_CHECKING count like any other. +def _imports(tree: ast.Module, path: str) -> list[ImportStatement]: + """Return every import in a module; relative imports are resolved. + + Imports under TYPE_CHECKING and inside functions count like any other. """ statements = [] for node in ast.walk(tree): if isinstance(node, ast.Import): - statements.extend([alias.name] for alias in node.names) + statements.extend(ImportStatement(alias.name, ()) for alias in node.names) elif isinstance(node, ast.ImportFrom): source = _from_source(node, path) - statements.append( - [source, *(f"{source}.{alias.name}" for alias in node.names)] - ) + names = tuple(f"{source}.{alias.name}" for alias in node.names) + statements.append(ImportStatement(source, names)) return statements @@ -267,6 +302,45 @@ def _imported_names(tree: ast.Module, path: str) -> dict[str, str]: return names +def _is_session_or_connector_module(module: str) -> bool: + parts = module.split(".") + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in parts or "database_session" in parts + ) + + +def _is_route_forbidden_import(module: str) -> bool: + return ( + _within_any(module, SQL_PACKAGES) + or _within_any(module, DATABASE_PACKAGES) + or _is_session_or_connector_module(module) + ) + + +def _is_connector_forbidden_import(module: str) -> bool: + parts = module.split(".") + return _within_any(module, DATABASE_PACKAGES) or ( + _within(module, "policyengine_api.services.v2") and "database_session" in parts + ) + + +def _is_http_import(module: str) -> bool: + return _within_any(module, HTTP_PACKAGES) + + +def _is_service_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any( + name, SERVICE_SQL_IMPORTS + ) + + +def _never(_module: str) -> bool: + return False + + +# --- calls ---------------------------------------------------------------- + + def _dotted_name(node: ast.expr, imported: Mapping[str, str]) -> str | None: """Resolve a called name such as ``select`` or ``sa.text`` through the imports.""" attributes = [] @@ -286,42 +360,30 @@ def _receiver_name(node: ast.expr) -> str: return "" -def _forbidden_import( - candidates: Iterable[str], is_forbidden: Callable[[str], bool] -) -> str | None: - return next( - (candidate for candidate in candidates if is_forbidden(candidate)), None - ) - - -def _is_route_forbidden_import(module: str) -> bool: - return ( - any(_within(module, package) for package in SQL_PACKAGES) - or _within(module, DATABASE_MODULE) - or _is_session_or_connector_module(module) - ) +def _is_session_receiver(node: ast.expr) -> bool: + """Whether a method's receiver is a database session or its container.""" + if ( + isinstance(node, ast.Attribute) + and node.attr == "session" + and isinstance(node.value, ast.Name) + and node.value.id == "request" + ): + return False + name = _receiver_name(node).lower().lstrip("_") + if name.startswith("http"): + return False + return name == "session" or name.endswith("_session") -def _is_session_or_connector_module(module: str) -> bool: - parts = module.split(".") - return _within(module, "policyengine_api.services.v2") and ( - "database_connectors" in parts or "database_session" in parts - ) +def _calls_on_a_table(call: ast.Call) -> bool: + """Whether a call is a SQL builder on a table, as ``Model.__table__.select()``.""" + func = call.func + if not isinstance(func, ast.Attribute) or func.attr not in SQL_BUILDERS: + return False + return "__table__" in ast.unparse(func.value).split(".") -def _is_connector_forbidden_import(module: str) -> bool: - parts = module.split(".") - return _within(module, DATABASE_MODULE) or ( - _within(module, "policyengine_api.services.v2") and "database_session" in parts - ) - - -def _is_http_import(module: str) -> bool: - return any(_within(module, package) for package in HTTP_PACKAGES) - - -def _is_sql_name(dotted: str) -> bool: - return any(_within(dotted, package) for package in SQL_PACKAGES) +# --- policies ------------------------------------------------------------- @dataclass(frozen=True) @@ -329,16 +391,24 @@ class LayerPolicy: """What one layer's rule forbids: imports, session methods and call kinds.""" rule_id: str - forbidden_import: Callable[[str], bool] + forbidden_module: Callable[[str], bool] session_methods: frozenset[str] checks_builders: bool checks_factories: bool + forbidden_name: Callable[[str], bool] = _never ROUTE_POLICY = LayerPolicy( ROUTES_RULE, _is_route_forbidden_import, - DATA_ACCESS_METHODS | TRANSACTION_METHODS, + ALL_SESSION_METHODS, + checks_builders=True, + checks_factories=True, +) +PURE_POLICY = LayerPolicy( + PURE_RULE, + _never, + ALL_SESSION_METHODS, checks_builders=True, checks_factories=True, ) @@ -349,6 +419,7 @@ class LayerPolicy: DATA_ACCESS_METHODS, checks_builders=True, checks_factories=False, + forbidden_name=_is_service_forbidden_sql_name, ), "connector": LayerPolicy( CONNECTORS_RULE, @@ -357,9 +428,33 @@ class LayerPolicy: checks_builders=False, checks_factories=True, ), + **dict.fromkeys(PURE_LAYERS, PURE_POLICY), } +def _policy(path: str, layer: str) -> LayerPolicy | None: + if path.startswith(f"{ROUTES_DIRECTORY}/"): + return ROUTE_POLICY + return LAYER_POLICIES.get(layer) + + +def _import_details(statement: ImportStatement, policy: LayerPolicy) -> list[str]: + """Return ``imports `` for each module or name the policy forbids.""" + if policy.forbidden_module(statement.module): + return [f"imports {statement.module}"] + module_level = next( + (name for name in statement.names if policy.forbidden_module(name)), None + ) + if module_level is not None: + return [f"imports {module_level}"] + if not statement.names: + forbidden = policy.forbidden_name(statement.module) + return [f"imports {statement.module}"] if forbidden else [] + return [ + f"imports {name}" for name in statement.names if policy.forbidden_name(name) + ] + + def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: """Return the findings for one call to an imported name. @@ -367,54 +462,56 @@ def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: """ last = dotted.rsplit(".", 1)[-1] details = [] - if policy.checks_builders and _is_sql_name(dotted) and last in SQL_BUILDERS: + from_sql = _within_any(dotted, SQL_PACKAGES) + if policy.checks_builders and from_sql and last in SQL_BUILDERS: details.append(f"calls {dotted}") - from_database_code = _is_sql_name(dotted) or _within( - dotted, "policyengine_api.data" + opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( + from_sql and last in SQL_SESSION_FACTORIES ) - if policy.checks_factories and last in SESSION_FACTORIES and from_database_code: + if policy.checks_factories and opens_a_session: details.append(f"calls {last}") return details -def _session_call_detail(call: ast.Call, policy: LayerPolicy) -> str | None: - """Return ``calls .`` for a forbidden method on a session.""" - if not isinstance(call.func, ast.Attribute): - return None - if call.func.attr not in policy.session_methods: - return None - receiver = _receiver_name(call.func.value) - if "session" not in receiver.lower(): - return None - return f"calls {receiver}.{call.func.attr}" +def _call_details( + call: ast.Call, imported: Mapping[str, str], policy: LayerPolicy +) -> list[str]: + details = [] + dotted = _dotted_name(call.func, imported) + if dotted is not None: + details.extend(_named_call_details(dotted, policy)) + if policy.checks_builders and _calls_on_a_table(call): + details.append(f"calls {ast.unparse(call.func)}") + func = call.func + if ( + isinstance(func, ast.Attribute) + and func.attr in policy.session_methods + and _is_session_receiver(func.value) + ): + details.append(f"calls {_receiver_name(func.value)}.{func.attr}") + return details def _file_findings(path: str, source: str, layer: str) -> list[tuple[str, str]]: """Return (rule id, detail) for each forbidden import and call in one file.""" - policy = ( - ROUTE_POLICY - if path.startswith(f"{ROUTES_DIRECTORY}/") - else LAYER_POLICIES.get(layer) - ) + policy = _policy(path, layer) if policy is None: return [] tree = ast.parse(source) details = [ - f"imports {module}" - for candidates in _imports(tree, path) - if (module := _forbidden_import(candidates, policy.forbidden_import)) + detail + for statement in _imports(tree, path) + for detail in _import_details(statement, policy) ] imported = _imported_names(tree, path) for call in (node for node in ast.walk(tree) if isinstance(node, ast.Call)): - dotted = _dotted_name(call.func, imported) - if dotted is not None: - details.extend(_named_call_details(dotted, policy)) - session_call = _session_call_detail(call, policy) - if session_call is not None: - details.append(session_call) + details.extend(_call_details(call, imported, policy)) return [(policy.rule_id, detail) for detail in details] +# --- scanning and comparing ----------------------------------------------- + + def _discovered_files(root: Path) -> list[str]: return [ path @@ -426,21 +523,73 @@ def _discovered_files(root: Path) -> list[str]: def scan( root: Path, manifest: Mapping[str, str], paths: Iterable[str] | None = None ) -> list[Finding]: - """Return every finding for the v2 files under ``root``, sorted.""" - findings = set() + """Return every finding for the v2 files under ``root``, one per occurrence.""" + findings = [] for path in paths if paths is not None else _discovered_files(root): layer = assign_layer(path, manifest) if layer is None: - findings.add(Finding(FILES_RULE, path, "matches no v2 layer")) + findings.append(Finding(FILES_RULE, path, "matches no v2 layer")) continue source = (root / path).read_text(encoding="utf-8") - findings.update( + findings.extend( Finding(rule_id, path, detail) for rule_id, detail in _file_findings(path, source, layer) ) return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) +def _manifest_violation(message: str, fix: str) -> Violation: + return Violation(FILES_RULE, CHECK_NAME, message, fix) + + +def _manifest_entry_violations(root: Path, path: str, entry: object) -> list[Violation]: + layer = entry.get("layer") if isinstance(entry, dict) else None + reason = entry.get("reason") if isinstance(entry, dict) else None + directory = next( + (folder for folder in MANIFEST_LAYERS if path.startswith(f"{folder}/")), None + ) + if directory is None: + return [ + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which is not under " + f"{SERVICES_DIRECTORY} or {ROUTES_DIRECTORY}", + "remove the entry", + ) + ] + violations = [] + if not (root / path).is_file(): + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which does not exist", + "remove the entry", + ) + ) + elif _layer_by_name(path) is not None: + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which its name already assigns to " + "a layer", + "remove the entry", + ) + ) + if layer not in MANIFEST_LAYERS[directory]: + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} gives {path} the layer '{layer}', which a file " + f"under {directory} can't have from the manifest", + f"use one of {', '.join(sorted(MANIFEST_LAYERS[directory]))}", + ) + ) + if not isinstance(reason, str) or not reason.strip(): + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} gives {path} no reason", + "say why the file has that layer", + ) + ) + return violations + + def load_manifest(root: Path) -> tuple[dict[str, str], list[Violation]]: """Read the manifest's file layers, and report entries that don't belong.""" manifest_file = root / MANIFEST_PATH @@ -452,45 +601,60 @@ def load_manifest(root: Path) -> tuple[dict[str, str], list[Violation]]: for path, entry in sorted(entries.items()): layer = entry.get("layer") if isinstance(entry, dict) else None manifest[path] = str(layer) - if not (root / path).is_file(): - violations.append( - Violation( - FILES_RULE, - CHECK_NAME, - f"{MANIFEST_PATH} lists {path}, which does not exist", - "remove the entry", - ) - ) - elif _layer_by_name(path) is not None: - violations.append( - Violation( - FILES_RULE, - CHECK_NAME, - f"{MANIFEST_PATH} lists {path}, which its name already assigns " - "to a layer", - "remove the entry", - ) - ) - if layer not in KNOWN_LAYERS: - violations.append( - Violation( - FILES_RULE, - CHECK_NAME, - f"{MANIFEST_PATH} gives {path} the unknown layer '{layer}'", - f"use one of {', '.join(sorted(KNOWN_LAYERS))}", - ) - ) + violations.extend(_manifest_entry_violations(root, path, entry)) return manifest, violations -def _baseline_entries(root: Path, baseline_file: str | None) -> set[str]: +def _baseline_entries(root: Path, baseline_file: str | None) -> Counter[str]: if baseline_file is None or not (root / baseline_file).is_file(): - return set() - return { + return Counter() + return Counter( line.strip() for line in (root / baseline_file).read_text().splitlines() if line.strip() and not line.lstrip().startswith("#") - } + ) + + +def _occurrences(count: int) -> str: + return f"{count} occurrence" if count == 1 else f"{count} occurrences" + + +def _compare( + rule_id: str, + found: Counter[str], + tolerated: Counter[str], + baseline_file: str | None, +) -> list[Violation]: + """Report findings beyond the baseline, and baseline entries beyond the findings.""" + violations = [] + for entry in sorted(found): + if found[entry] <= tolerated[entry]: + continue + message = ( + entry + if not tolerated[entry] + else f"{entry} ({_occurrences(found[entry])}, the baseline tolerates " + f"{tolerated[entry]})" + ) + violations.append(Violation(rule_id, CHECK_NAME, message, FIXES[rule_id])) + for entry in sorted(tolerated): + if tolerated[entry] <= found[entry]: + continue + detail = ( + entry + if not found[entry] + else f"{entry} ({_occurrences(found[entry])}, the baseline lists " + f"{tolerated[entry]})" + ) + violations.append( + Violation( + rule_id, + CHECK_NAME, + f"baseline entry no longer applies: {detail}", + f"remove it from {baseline_file} and lower the rule's baseline count", + ) + ) + return violations def run_check(root: Path = REPO_ROOT) -> list[Violation]: @@ -503,20 +667,11 @@ def run_check(root: Path = REPO_ROOT) -> list[Violation]: for rule_id in RULES: rule = registry.rule(rule_id) baseline_file = rule.baseline.file if rule and rule.baseline else None - tolerated = _baseline_entries(root, baseline_file) - found = {str(finding) for finding in findings if finding.rule_id == rule_id} - violations.extend( - Violation(rule_id, CHECK_NAME, entry, FIXES[rule_id]) - for entry in sorted(found - tolerated) + found = Counter( + str(finding) for finding in findings if finding.rule_id == rule_id ) - violations.extend( - Violation( - rule_id, - CHECK_NAME, - f"baseline entry no longer applies: {entry}", - f"remove it from {baseline_file} and lower the rule's baseline count", - ) - for entry in sorted(tolerated - found) + violations += _compare( + rule_id, found, _baseline_entries(root, baseline_file), baseline_file ) return violations From a21780f9bfd2989416f65b7b5d4e39d82bc3ac28 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:31:59 +0400 Subject: [PATCH 05/15] Describe the new pure-modules rule in the changelog fragment Refs #4190 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4190.added.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/4190.added.md b/changelog.d/4190.added.md index b1569ab95..0cc611cf1 100644 --- a/changelog.d/4190.added.md +++ b/changelog.d/4190.added.md @@ -1 +1 @@ -Check that every API v2 route and service file has a layer, and that v2 route modules open no session and build no SQL, services build and run no SQL, and connectors never open, commit, roll back or close a session, in a new v2-layers quality guard. +Check that every API v2 route and service file has a layer, and that v2 route modules open no session and build no SQL, services construct no SQL, connectors never open, commit, roll back or close a session, and validators, transformations and types make no session calls, in a new v2-layers quality guard. From c1e8e66636866763311fa5466b4d3f8a9a0ab4ad Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:48:04 +0400 Subject: [PATCH 06/15] Test the second review's findings on the v2-layers guard (failing) - Routes and the pure layers may call no session method at all, so get_one, bulk_save_objects, connection and expunge count; services' and connectors' lists gain get_one, the bulk methods, reset and invalidate. - A manifest file in a pure or HTTP layer gets that layer's import and call checks, including session factories. - A database_session.py below a resource package has no layer. - Every call into SQLAlchemy or SQLModel but its exception classes counts, so "import sqlalchemy.exc" doesn't open sqlalchemy.and_. - request.session on any request object, and client or requests sessions, are not database sessions. - Only a builder called on .__table__ counts, once. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 2 +- scripts/rules/tests/test_v2_layers.py | 159 +++++++++++++++++- 2 files changed, 157 insertions(+), 4 deletions(-) diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml index 234c495cc..9048c3e01 100644 --- a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -1,6 +1,6 @@ # Proof fixture for rule api.v2.pure-modules-no-session-calls. The guard must # reject it: a validator loads a row through the session it was handed, a -# process-state module runs a query, and a package __init__ commits. +# types module runs a query, and a package __init__ commits. # scripts/rules/tests/test_v2_layers.py writes each file into a temporary # directory and runs the v2-layers guard's scan on it, with no manifest. diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 6cba50551..e8b9c7dbc 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -128,17 +128,17 @@ def test_every_file_in_the_repository_has_a_layer(self) -> None: class TestCallChecks(unittest.TestCase): def test_only_calls_on_a_session_count_as_session_calls(self) -> None: source = """ - def handle(session, request_session, names: set, cache: dict): + def handle(session, read_session, names: set, cache: dict): names.add("a") cache.get("b") - request_session.exec("c") + read_session.exec("c") session.get("d") """ self.assertEqual( _findings({ROUTE: source}, ROUTES_RULE), { - f"{ROUTE} calls request_session.exec", + f"{ROUTE} calls read_session.exec", f"{ROUTE} calls session.get", }, ) @@ -307,6 +307,10 @@ def build(Policy, ids): _findings({SERVICE: source}, SERVICES_RULE), { f"{SERVICE} calls Policy.__table__.select", + f"{SERVICE} calls sqlalchemy.and_", + f"{SERVICE} calls sqlalchemy.func.lower", + f"{SERVICE} calls sqlalchemy.or_", + f"{SERVICE} calls sqlmodel.col", f"{SERVICE} imports sqlalchemy.and_", f"{SERVICE} imports sqlalchemy.func", f"{SERVICE} imports sqlalchemy.or_", @@ -326,6 +330,155 @@ def route(request, session_ids: set): self.assertEqual(_findings({ROUTE: source}, ROUTES_RULE), set()) +class TestSecondReview(unittest.TestCase): + """Cases from the second review round.""" + + def test_routes_and_pure_layers_may_call_no_session_method_at_all(self) -> None: + source = """ + def handle(session, row): + session.get_one(row, 1) + session.bulk_save_objects([row]) + session.connection() + session.expunge(row) + """ + validator = "policyengine_api/services/v2/policies/validators.py" + + for path, rule_id in ((ROUTE, ROUTES_RULE), (validator, PURE_RULE)): + with self.subTest(path=path): + self.assertEqual( + _findings({path: source}, rule_id), + { + f"{path} calls session.bulk_save_objects", + f"{path} calls session.connection", + f"{path} calls session.expunge", + f"{path} calls session.get_one", + }, + ) + + def test_services_and_connectors_know_every_relevant_session_method(self) -> None: + service_source = """ + class Service: + def run(self, rows): + with self._database_session.transaction(): + self._session.get_one(rows, 1) + self._session.bulk_insert_mappings(rows, []) + """ + connector_source = """ + def write(session): + session.reset() + session.invalidate() + """ + + self.assertEqual( + _findings({SERVICE: service_source}, SERVICES_RULE), + { + f"{SERVICE} calls _session.bulk_insert_mappings", + f"{SERVICE} calls _session.get_one", + }, + ) + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls session.invalidate", + f"{CONNECTOR} calls session.reset", + }, + ) + + def test_manifest_layers_get_their_layer_s_checks(self) -> None: + process_state = "policyengine_api/services/v2/metadata/selected_catalogs.py" + support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + manifest = {process_state: "process-state", support: "http-support"} + files = { + process_state: ( + "from policyengine_api.data.orm import get_v1_session_factory\n" + "from .database_connectors.reads import read_rows\n" + "def load(session):\n" + " get_v1_session_factory()\n" + " return session.exec(read_rows)\n" + ), + support: "def keep(session, row):\n session.add(row)\n", + } + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + found = { + (finding.rule_id, str(finding)) for finding in scan(root, manifest) + } + + self.assertEqual( + found, + { + (PURE_RULE, f"{process_state} calls get_v1_session_factory"), + (PURE_RULE, f"{process_state} calls session.exec"), + (PURE_RULE, f"{process_state} imports policyengine_api.data.orm"), + ( + PURE_RULE, + f"{process_state} imports " + "policyengine_api.services.v2.metadata.database_connectors.reads", + ), + (ROUTES_RULE, f"{support} calls session.add"), + }, + ) + + def test_a_session_module_below_a_resource_package_has_no_layer(self) -> None: + nested = "policyengine_api/services/v2/metadata/catalogs/database_session.py" + + self.assertIsNone(assign_layer(nested, {})) + self.assertEqual( + assign_layer( + "policyengine_api/services/v2/metadata/database_session.py", {} + ), + "session", + ) + + def test_every_call_into_sqlalchemy_counts_except_its_exceptions(self) -> None: + source = """ + import sqlalchemy.exc + + + def build(Policy): + try: + return sqlalchemy.and_(sqlalchemy.func.lower(Policy.name) == "x") + except sqlalchemy.exc.IntegrityError: + raise sqlalchemy.exc.IntegrityError("x", None, None) + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + { + f"{SERVICE} calls sqlalchemy.and_", + f"{SERVICE} calls sqlalchemy.func.lower", + }, + ) + + def test_web_sessions_and_clients_are_not_database_sessions(self) -> None: + source = """ + def handle(self, http_request, requests_session): + self.request.session.get("user") + http_request.session.get("user") + self._client_session.get("https://example.org") + requests_session.get("https://example.org") + """ + + self.assertEqual(_findings({ROUTE: source}, ROUTES_RULE), set()) + + def test_only_a_builder_called_on_a_table_counts(self) -> None: + source = """ + from policyengine_api.data.v2.models import Policy + + + def build(rows): + Policy.__table__.insert().values(name="x") + Policy.__table__.columns.values() + rows[Policy.__table__.name].values() + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + {f"{SERVICE} calls Policy.__table__.insert"}, + ) + + class TestBaselineAndManifest(unittest.TestCase): """run_check compares findings with the rule's baseline and checks the manifest.""" From 5ff9ea66ca3f531c05d50de1d69c4762af5dfdc8 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:50:35 +0400 Subject: [PATCH 07/15] Close the second review's findings on the v2-layers guard - Routes and the pure layers may call no method on a session, so a method missing from a list can't slip through. Services' and connectors' lists gain get_one, the bulk methods, connection, expunge, reset and invalidate. - The pure layers also get import checks (nothing from SQLAlchemy or SQLModel but the Session type; no connector, session, v1 ORM or v2 database module), so a file the manifest assigns to one is checked as fully as a named one. - A database_session.py below a resource package has no layer, so it can't escape the session-module rule. - Every call into SQLAlchemy or SQLModel counts in routes, services and the pure layers, except to its exception classes, so "import sqlalchemy.exc" no longer opens sqlalchemy.and_. - A request's .session and client or requests sessions are not database sessions; only .__table__.() counts as a table builder. - The pure rule's scope leaves out the connector package __init__ files, which are checked as connectors. The docs list what the guard recognises and what it doesn't. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../skills/v2-code-organization.md | 25 ++-- docs/generated/rules.md | 2 +- .../bad_tree.toml | 1 + rules/registry.toml | 4 +- scripts/rules/tests/test_v2_layers.py | 1 + scripts/rules/v2_layers.py | 130 +++++++++++------- 6 files changed, 104 insertions(+), 59 deletions(-) diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 6694b018f..164267671 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -20,13 +20,19 @@ A file with another name needs an entry, with its layer and a reason, in `services/v2` the layer `service`, `validator`, `transformation`, `types` or `process-state`, and a file under `fastapi_routes/v2` the layer `route`, `http-model` or `http-support`; each layer follows that layer's rules below. +A `database_session.py` sits directly in its resource package. The guard counts a method call as a session call when its receiver is named -`session` or `_session` (not `request.session` or `http_session`), -a SQL builder when it was imported from SQLAlchemy or SQLModel or is called on -a model's `__table__`, and a session factory when it comes from -`policyengine_api.data.orm` or `policyengine_api.data.v2.database`. A -ratcheted rule's baseline lists one line per tolerated occurrence. +`session` or `_session`, but not a request's `.session` or an HTTP +or client session (`http_session`, `requests_session`). Route modules and the +pure layers may call no method on a session at all. A call into SQLAlchemy or +SQLModel counts, except to its exception classes, and so does a builder called +on a model's `__table__`. A session factory is any call into +`policyengine_api.data.orm` or `policyengine_api.data.v2.database`, or +SQLAlchemy's `Session`, `sessionmaker` or `create_engine`. The guard doesn't +recognise a session bound to another name, an alias of a model's table, or a +name re-exported through another module. A ratcheted rule's baseline lists one +line per tolerated occurrence. ## HTTP adapters @@ -66,8 +72,9 @@ Request models describe HTTP bodies. Response models describe public response response structures and OpenAPI output. Route functions handle HTTP-only conditions, invoke one service method, and convert typed failures to HTTP responses. No module under `fastapi_routes/v2` imports SQLAlchemy, SQLModel, a connector, a -`database_session` module or `policyengine_api.data.v2.database`, or calls a -SQL builder, a session method or a session factory (rule +`database_session` module, the v1 ORM module `policyengine_api.data.orm` or +`policyengine_api.data.v2.database`, or calls a SQLAlchemy or SQLModel +function, a session method or a session factory (rule `api.v2.routes-no-sessions-or-sql`; today's `sqlalchemy.exc` imports are in its baseline). @@ -125,7 +132,9 @@ Define functions that inspect already-available values and either return a validated value or raise a typed exception. Validators must not load records, open sessions, execute SQL, mutate database rows, or construct HTTP responses. Validators, transformations, types, process-state modules and package -`__init__` files call no session method, SQL builder or session factory (rule +`__init__` files import nothing from SQLAlchemy or SQLModel but the `Session` +type, import no connector or session module, and call no session method, +SQLAlchemy or SQLModel function or session factory (rule `api.v2.pure-modules-no-session-calls`). When validation depends on stored state, a database connector loads the diff --git a/docs/generated/rules.md b/docs/generated/rules.md index c590d46fc..ccb1e2e65 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -89,7 +89,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | -| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules and package __init__ files call no session method, SQL builder or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml index 9048c3e01..f0ebcdcf5 100644 --- a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -8,6 +8,7 @@ expected_findings = [ "policyengine_api/services/v2/policies/__init__.py calls session.commit", "policyengine_api/services/v2/policies/types.py calls session.exec", "policyengine_api/services/v2/policies/types.py calls sqlmodel.select", + "policyengine_api/services/v2/policies/types.py imports sqlmodel.select", "policyengine_api/services/v2/policies/validators.py calls session.get", ] diff --git a/rules/registry.toml b/rules/registry.toml index e3f14fe70..40e39f97e 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -279,9 +279,9 @@ test = "tests/unit/v2/test_data_crud_boundaries.py::test_rule_proof_user_service [[rule]] id = "api.v2.pure-modules-no-session-calls" category = "v2-layers" -statement = "v2 validators, transformations, types, process-state modules and package __init__ files call no session method, SQL builder or session factory; they work on values and rows a connector loaded." +statement = "v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded." rationale = "docs/engineering/skills/v2-code-organization.md#validatorspy" -scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/metadata/selected_catalogs.py", "policyengine_api/services/v2/**/__init__.py"] +scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/metadata/selected_catalogs.py", "policyengine_api/services/v2/__init__.py", "policyengine_api/services/v2/*/__init__.py"] tier = "mechanical" status = "enforced" autofix = false diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index e8b9c7dbc..65d2b66da 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -236,6 +236,7 @@ def test_validators_transformations_types_and_process_state_touch_no_session( f"{path} calls session.commit", f"{path} calls session.exec", f"{path} calls sqlmodel.select", + f"{path} imports sqlmodel.select", }, ) diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index e20092170..5e2cfa8ca 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -21,21 +21,27 @@ a session, call no session factory, and import no v1 ORM, v2 database or ``database_session`` module; - validators, transformations, types, process-state modules and package - ``__init__`` files (``api.v2.pure-modules-no-session-calls``) call no session - method, SQL builder or session factory. + ``__init__`` files (``api.v2.pure-modules-no-session-calls``) import nothing + from SQLAlchemy or SQLModel but the ``Session`` type, import no connector, + ``database_session``, v1 ORM or v2 database module, and call no session + method, SQLAlchemy or SQLModel function or session factory. -``database_session.py`` modules own the session's lifetime; rule -``api.v2.session-modules-no-queries`` checks them. +``database_session.py`` modules, directly in a resource package, own the +session's lifetime; rule ``api.v2.session-modules-no-queries`` checks them. A method call counts as a session call when its receiver is named ``session`` or ``_session`` (``session.add``, ``self._session.exec``), but not -``request.session`` (Starlette's session data), ``http_session`` or names that -only contain the word (``sessions_router``). A SQL builder counts when it was -imported from SQLAlchemy or SQLModel, or is called on a model's ``__table__``; -a session factory, when it was imported from ``policyengine_api.data.orm`` or -``policyengine_api.data.v2.database``, or is ``Session``, ``sessionmaker`` or -``create_engine`` from SQLAlchemy or SQLModel. Imports made only under -``TYPE_CHECKING`` count. +when it is a request's ``.session`` (Starlette's session data) or named like +an HTTP or client session (``http_session``, ``requests_session``), or only +contains the word (``sessions_router``). Routes and the pure layers may call +no method on a session; services and connectors may not call the methods +listed below. A call into SQLAlchemy or SQLModel counts, except to its +exception classes; so does a builder called on a model's ``__table__``. A +session factory is any call into ``policyengine_api.data.orm`` or +``policyengine_api.data.v2.database``, or SQLAlchemy's ``Session``, +``sessionmaker`` or ``create_engine``. Imports made only under +``TYPE_CHECKING`` count. Not recognised: a session bound to another name, an +alias of a model's table, and a name re-exported through another module. A ratcheted rule's baseline file lists its tolerated findings, one line per occurrence, as the guard prints them. More occurrences than the baseline lists @@ -98,9 +104,11 @@ SQL_PACKAGES = ("sqlalchemy", "sqlmodel") DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") +SESSION_TYPES = ("sqlmodel.Session", "sqlalchemy.orm.Session") +SQL_EXCEPTIONS = "sqlalchemy.exc" # What a service may import from SQLAlchemy or SQLModel: the session type, for # annotations, and the exception classes, to raise typed failures. -SERVICE_SQL_IMPORTS = ("sqlmodel.Session", "sqlalchemy.orm.Session", "sqlalchemy.exc") +SERVICE_SQL_IMPORTS = (*SESSION_TYPES, SQL_EXCEPTIONS) SQL_BUILDERS = frozenset( { "delete", @@ -119,17 +127,24 @@ ) SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) TRANSACTION_METHODS = frozenset( - {"begin", "begin_nested", "close", "commit", "rollback"} + {"begin", "begin_nested", "close", "commit", "invalidate", "reset", "rollback"} ) DATA_ACCESS_METHODS = frozenset( { "add", "add_all", + "bulk_insert_mappings", + "bulk_save_objects", + "bulk_update_mappings", + "connection", "delete", "exec", "execute", + "expunge", + "expunge_all", "flush", "get", + "get_one", "merge", "query", "refresh", @@ -137,9 +152,8 @@ "scalars", } ) -# A database_session container's context managers. -CONTAINER_METHODS = frozenset({"read", "transaction"}) -ALL_SESSION_METHODS = DATA_ACCESS_METHODS | TRANSACTION_METHODS | CONTAINER_METHODS +# Receiver name prefixes, before "_session", of web rather than database sessions. +WEB_SESSION_WORDS = ("client", "http", "request") HTTP_PACKAGES = ("fastapi", "starlette") FIXES = { @@ -229,7 +243,10 @@ def _service_layer(parts: tuple[str, ...]) -> str | None: if name == "__init__.py": return "connector" if in_connector_package else "package" if "database_connectors" not in resource_parts: - return SERVICE_FILE_LAYERS.get(name) if resource_parts else None + layer = SERVICE_FILE_LAYERS.get(name) if resource_parts else None + # A session module sits directly in its resource package, where the + # session-module rule looks for it. + return None if layer == "session" and len(resource_parts) > 1 else layer stem = name.removesuffix(".py") is_operation = any( stem == operation or stem.startswith(f"{operation}_") @@ -334,6 +351,16 @@ def _is_service_forbidden_sql_name(name: str) -> bool: ) +def _is_pure_forbidden_import(module: str) -> bool: + return _within_any(module, DATABASE_PACKAGES) or _is_session_or_connector_module( + module + ) + + +def _is_pure_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any(name, SESSION_TYPES) + + def _never(_module: str) -> bool: return False @@ -365,22 +392,25 @@ def _is_session_receiver(node: ast.expr) -> bool: if ( isinstance(node, ast.Attribute) and node.attr == "session" - and isinstance(node.value, ast.Name) - and node.value.id == "request" + and "request" in _receiver_name(node.value).lower() ): return False name = _receiver_name(node).lower().lstrip("_") - if name.startswith("http"): - return False - return name == "session" or name.endswith("_session") + if name == "session": + return True + prefix = name.removesuffix("_session") + return prefix != name and not any(word in prefix for word in WEB_SESSION_WORDS) def _calls_on_a_table(call: ast.Call) -> bool: - """Whether a call is a SQL builder on a table, as ``Model.__table__.select()``.""" + """Whether a call is a SQL builder on a table, as ``Model.__table__.insert()``.""" func = call.func - if not isinstance(func, ast.Attribute) or func.attr not in SQL_BUILDERS: - return False - return "__table__" in ast.unparse(func.value).split(".") + return ( + isinstance(func, ast.Attribute) + and func.attr in SQL_BUILDERS - {"values"} + and isinstance(func.value, ast.Attribute) + and func.value.attr == "__table__" + ) # --- policies ------------------------------------------------------------- @@ -388,12 +418,17 @@ def _calls_on_a_table(call: ast.Call) -> bool: @dataclass(frozen=True) class LayerPolicy: - """What one layer's rule forbids: imports, session methods and call kinds.""" + """What one layer's rule forbids: imports, session methods and call kinds. + + ``session_methods`` of None forbids every method on a session. + ``checks_sql_calls`` forbids every call into SQLAlchemy or SQLModel but + its exception classes, and builders called on a model's ``__table__``. + """ rule_id: str forbidden_module: Callable[[str], bool] - session_methods: frozenset[str] - checks_builders: bool + session_methods: frozenset[str] | None + checks_sql_calls: bool checks_factories: bool forbidden_name: Callable[[str], bool] = _never @@ -401,23 +436,24 @@ class LayerPolicy: ROUTE_POLICY = LayerPolicy( ROUTES_RULE, _is_route_forbidden_import, - ALL_SESSION_METHODS, - checks_builders=True, + None, + checks_sql_calls=True, checks_factories=True, ) PURE_POLICY = LayerPolicy( PURE_RULE, - _never, - ALL_SESSION_METHODS, - checks_builders=True, + _is_pure_forbidden_import, + None, + checks_sql_calls=True, checks_factories=True, + forbidden_name=_is_pure_forbidden_sql_name, ) LAYER_POLICIES = { "service": LayerPolicy( SERVICES_RULE, _is_http_import, DATA_ACCESS_METHODS, - checks_builders=True, + checks_sql_calls=True, checks_factories=False, forbidden_name=_is_service_forbidden_sql_name, ), @@ -425,7 +461,7 @@ class LayerPolicy: CONNECTORS_RULE, _is_connector_forbidden_import, TRANSACTION_METHODS, - checks_builders=False, + checks_sql_calls=False, checks_factories=True, ), **dict.fromkeys(PURE_LAYERS, PURE_POLICY), @@ -461,16 +497,15 @@ def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. """ last = dotted.rsplit(".", 1)[-1] - details = [] from_sql = _within_any(dotted, SQL_PACKAGES) - if policy.checks_builders and from_sql and last in SQL_BUILDERS: - details.append(f"calls {dotted}") opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( from_sql and last in SQL_SESSION_FACTORIES ) - if policy.checks_factories and opens_a_session: - details.append(f"calls {last}") - return details + if opens_a_session: + return [f"calls {last}"] if policy.checks_factories else [] + if policy.checks_sql_calls and from_sql and not _within(dotted, SQL_EXCEPTIONS): + return [f"calls {dotted}"] + return [] def _call_details( @@ -480,14 +515,13 @@ def _call_details( dotted = _dotted_name(call.func, imported) if dotted is not None: details.extend(_named_call_details(dotted, policy)) - if policy.checks_builders and _calls_on_a_table(call): + if policy.checks_sql_calls and _calls_on_a_table(call): details.append(f"calls {ast.unparse(call.func)}") func = call.func - if ( - isinstance(func, ast.Attribute) - and func.attr in policy.session_methods - and _is_session_receiver(func.value) - ): + forbidden_method = isinstance(func, ast.Attribute) and ( + policy.session_methods is None or func.attr in policy.session_methods + ) + if forbidden_method and _is_session_receiver(func.value): details.append(f"calls {_receiver_name(func.value)}.{func.attr}") return details From 51005e07d7ddf65dddf04ab0fe0d1f793aba7125 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 21:25:19 +0400 Subject: [PATCH 08/15] Test the third review's findings on the v2-layers guard (failing) - An injected session factory opens a session: session_factory.begin() and self._session_factory() count in connectors and routes, and so does a database_session container's transaction() in a connector. - New rule api.v2.session-modules-no-data-access: a database_session module opens, commits, rolls back and closes sessions, but adds, gets, merges or executes nothing and builds no SQL. Proof fixtures included. - A request's .session is recognised by its owner (req, websocket, *_request), so request_context.session and request_session are database sessions. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 25 ++++++++ .../good_tree.toml | 28 +++++++++ scripts/rules/tests/test_v2_layers.py | 62 +++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml create mode 100644 rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml new file mode 100644 index 000000000..882e00f99 --- /dev/null +++ b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml @@ -0,0 +1,25 @@ +# Proof fixture for rule api.v2.session-modules-no-data-access. The guard must +# reject it: the database_session module adds a row, loads one with +# Session.get and runs raw SQL, work that belongs in a connector. +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/policies/database_session.py calls session.add", + "policyengine_api/services/v2/policies/database_session.py calls session.execute", + "policyengine_api/services/v2/policies/database_session.py calls session.get", + "policyengine_api/services/v2/policies/database_session.py calls sqlalchemy.text", +] + +[files] +"policyengine_api/services/v2/policies/database_session.py" = """ +from sqlalchemy import text + + +class PolicyDatabaseSession: + def save(self, row, policy_id): + with self._session_factory.begin() as session: + session.add(row) + session.execute(text("DELETE FROM policies")) + return session.get(type(row), policy_id) +""" diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml new file mode 100644 index 000000000..c271d4ee7 --- /dev/null +++ b/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml @@ -0,0 +1,28 @@ +# Proof fixture for rule api.v2.session-modules-no-data-access. The guard must +# accept it: the module opens sessions and transactions, commits and rolls +# back, and leaves reads and writes to the connectors. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/database_session.py" = """ +from contextlib import contextmanager + + +class PolicyDatabaseSession: + @contextmanager + def read(self): + with self._session_factory() as session: + yield session + + @contextmanager + def transaction(self): + with self._session_factory.begin() as session: + savepoint = session.begin_nested() + try: + yield session + savepoint.commit() + except Exception: + savepoint.rollback() + raise +""" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 65d2b66da..4e338dbe6 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -14,6 +14,7 @@ PURE_RULE, ROUTES_RULE, SERVICES_RULE, + SESSION_RULE, assign_layer, run_check, scan, @@ -72,6 +73,9 @@ def test_rule_proof_connectors_never_commit(self) -> None: def test_rule_proof_pure_modules_no_session_calls(self) -> None: _assert_proof(self, PURE_RULE) + def test_rule_proof_session_modules_no_data_access(self) -> None: + _assert_proof(self, SESSION_RULE) + class TestLayerAssignment(unittest.TestCase): def test_files_are_assigned_by_name(self) -> None: @@ -480,6 +484,64 @@ def build(rows): ) +class TestThirdReview(unittest.TestCase): + """Cases from the third review round.""" + + def test_injected_session_factories_and_containers_open_sessions(self) -> None: + connector_source = """ + def create(session_factory, database_session, row): + with session_factory.begin() as session: + session.add(row) + with database_session.transaction() as session: + session.add(row) + + + class Writer: + def write(self, row): + with self._session_factory() as session: + session.add(row) + """ + route_source = """ + def route(session_factory, row): + with session_factory.begin() as db: + return db + """ + + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls _session_factory", + f"{CONNECTOR} calls database_session.transaction", + f"{CONNECTOR} calls session_factory.begin", + }, + ) + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + {f"{ROUTE} calls session_factory.begin"}, + ) + + def test_request_session_data_is_recognised_by_its_owner(self) -> None: + route_source = """ + def route(req, websocket, request_context, row): + req.session.get("user") + websocket.session.get("user") + request_context.session.add(row) + """ + service_source = """ + def run(request_session, statement): + return request_session.exec(statement) + """ + + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + {f"{ROUTE} calls session.add"}, + ) + self.assertEqual( + _findings({SERVICE: service_source}, SERVICES_RULE), + {f"{SERVICE} calls request_session.exec"}, + ) + + class TestBaselineAndManifest(unittest.TestCase): """run_check compares findings with the rule's baseline and checks the manifest.""" From efb919db21cc8ce902e53661637f41e1439cb5e1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 22:30:22 +0400 Subject: [PATCH 09/15] Close the third review's findings on the v2-layers guard - Injected session factories open sessions: session_factory(), self._session_factory() and session_factory.begin() count in routes, connectors and the pure layers, and a connector may not call a database_session container's read() or transaction(). - New rule api.v2.session-modules-no-data-access: database_session modules call no data-access session method or SQL builder and import no connector. Today's modules only open, commit, roll back and close. - A request's .session is recognised by its owner (req, request, websocket, ws, *_request), so request_context.session and request_session are database sessions, and requests_session is not. - The docs say services may open sessions, which their rule doesn't forbid. The real code still has exactly the 4 tolerated sqlalchemy.exc imports. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- .../skills/v2-code-organization.md | 17 ++- docs/generated/rules.md | 9 +- rules/registry.toml | 26 ++++- scripts/rules/v2_layers.py | 104 +++++++++++++++--- 5 files changed, 129 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 10d78c084..68cb7ae1f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### API v2 layer boundaries -- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.pure-modules-no-session-calls`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` +- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.pure-modules-no-session-calls`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-data-access`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. (doc-only, not checked: follow it yourself) ### Runtime persistence and route boundaries diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 164267671..fb3d964e0 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -25,11 +25,14 @@ A `database_session.py` sits directly in its resource package. The guard counts a method call as a session call when its receiver is named `session` or `_session`, but not a request's `.session` or an HTTP or client session (`http_session`, `requests_session`). Route modules and the -pure layers may call no method on a session at all. A call into SQLAlchemy or -SQLModel counts, except to its exception classes, and so does a builder called -on a model's `__table__`. A session factory is any call into -`policyengine_api.data.orm` or `policyengine_api.data.v2.database`, or -SQLAlchemy's `Session`, `sessionmaker` or `create_engine`. The guard doesn't +pure layers may call no method on a session at all. In routes, services and +the pure layers a call into SQLAlchemy or SQLModel counts, except to its +exception classes and, in services, to a session factory; a builder called on +a model's `__table__` counts too. A session factory is any call into +`policyengine_api.data.orm` or `policyengine_api.data.v2.database`, +SQLAlchemy's `Session`, `sessionmaker` or `create_engine`, or an injected +`*session_factory` or its `begin()`; only services and `database_session` +modules may use one. The guard doesn't recognise a session bound to another name, an alias of a model's table, or a name re-exported through another module. A ratcheted rule's baseline lists one line per tolerated occurrence. @@ -171,7 +174,9 @@ baseline). Define the resource's database-session lifetime container. It may open and close sessions, begin transactions, commit, or roll back. It must not construct SQL, choose records, validate business rules, transform records into response -types, or determine multi-step operation order. +types, or determine multi-step operation order. It calls no data-access +session method (`add`, `get`, `merge`, `exec`, `execute` and so on) and +imports no connector (rule `api.v2.session-modules-no-data-access`). Composition code should construct this container and inject it into the service. A request-scoped read service may wrap one already-open session; a diff --git a/docs/generated/rules.md b/docs/generated/rules.md index ccb1e2e65..c425fa6e9 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 67 | 4 | 1 | 8 | 80 | +| mechanical | 68 | 4 | 1 | 8 | 81 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 72 of 82 rules. +**Blocking:** 73 of 83 rules. ## Gaps @@ -82,7 +82,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | --- | --- | --- | --- | --- | --- | --- | --- | | `api.routes.shared-query-parameters` | Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. | mechanical | doc-only | no | nothing | — | — | | `api.v2.connectors-mutations-no-reads` | v2 mutation connectors (creates, updates, deletes) never issue select or get reads. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open (with a session factory or a database_session container), commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | @@ -93,6 +93,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.session-modules-no-data-access` | v2 database_session modules sit directly in their resource package and only open, commit, roll back and close sessions: they call no data-access session method (add, get, merge, exec, execute and so on) or SQL builder, and import no connector. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.session-modules-no-queries` | v2 database_session modules own session lifetime only and never build select, insert, update, delete or exec calls. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.types-transformations-no-validators` | v2 types and transformations modules never import a validators module, directly or through other modules; validators import types, not the reverse. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.user-services-no-sql` | v2 user services (identity, accounts, legacy references and user associations) never build or run SQL themselves or import SQLAlchemy; their connectors do. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | yes | @@ -119,6 +120,7 @@ flowchart LR r_api_v2_routes_import_no_database_access("api.v2.routes-import-no-database-access (enforced)") r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (ratcheted)") r_api_v2_services_no_sql("api.v2.services-no-sql (enforced)") + r_api_v2_session_modules_no_data_access("api.v2.session-modules-no-data-access (enforced)") r_api_v2_session_modules_no_queries("api.v2.session-modules-no-queries (enforced)") r_api_v2_types_transformations_no_validators("api.v2.types-transformations-no-validators (ratcheted)") r_api_v2_user_services_no_sql("api.v2.user-services-no-sql (enforced)") @@ -139,6 +141,7 @@ flowchart LR r_api_v2_routes_import_no_database_access --> e_import_linter_pyproject_toml r_api_v2_routes_no_sessions_or_sql --> e_quality_guard_v2_layers r_api_v2_services_no_sql --> e_quality_guard_v2_layers + r_api_v2_session_modules_no_data_access --> e_quality_guard_v2_layers r_api_v2_session_modules_no_queries --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py r_api_v2_types_transformations_no_validators --> e_import_linter_pyproject_toml r_api_v2_user_services_no_sql --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py diff --git a/rules/registry.toml b/rules/registry.toml index 40e39f97e..3ddccd0c6 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -300,10 +300,34 @@ workflow = ".github/workflows/pr.yml" job = "quality-guards" invocation = "unittest discover -s scripts/rules/tests" +[[rule]] +id = "api.v2.session-modules-no-data-access" +category = "v2-layers" +statement = "v2 database_session modules sit directly in their resource package and only open, commit, roll back and close sessions: they call no data-access session method (add, get, merge, exec, execute and so on) or SQL builder, and import no connector." +rationale = "docs/engineering/skills/v2-code-organization.md#database_sessionpy" +scope = ["policyengine_api/services/v2/*/database_session.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_session_modules_no_data_access" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + [[rule]] id = "api.v2.connectors-never-commit" category = "v2-layers" -statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module." +statement = "v2 connectors receive a session from the service and never open (with a session factory or a database_session container), commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module." rationale = "docs/engineering/skills/v2-code-organization.md#database_connectors" scope = ["policyengine_api/services/v2/*/database_connectors/*.py"] tier = "mechanical" diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 5e2cfa8ca..7593a4817 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -27,21 +27,24 @@ method, SQLAlchemy or SQLModel function or session factory. ``database_session.py`` modules, directly in a resource package, own the -session's lifetime; rule ``api.v2.session-modules-no-queries`` checks them. +session's lifetime (``api.v2.session-modules-no-data-access``): they call no +data-access session method or SQL builder and import no connector. A method call counts as a session call when its receiver is named ``session`` or ``_session`` (``session.add``, ``self._session.exec``), but not -when it is a request's ``.session`` (Starlette's session data) or named like -an HTTP or client session (``http_session``, ``requests_session``), or only -contains the word (``sessions_router``). Routes and the pure layers may call -no method on a session; services and connectors may not call the methods -listed below. A call into SQLAlchemy or SQLModel counts, except to its -exception classes; so does a builder called on a model's ``__table__``. A -session factory is any call into ``policyengine_api.data.orm`` or -``policyengine_api.data.v2.database``, or SQLAlchemy's ``Session``, -``sessionmaker`` or ``create_engine``. Imports made only under -``TYPE_CHECKING`` count. Not recognised: a session bound to another name, an -alias of a model's table, and a name re-exported through another module. +when it is the ``.session`` of a request or websocket (Starlette's session +data), or named like an HTTP or client session (``http_session``, +``requests_session``), or only contains the word (``sessions_router``). +Routes and the pure layers may call no method on a session; services, +connectors and session modules may not call the methods listed below. A call +into SQLAlchemy or SQLModel counts, except to its exception classes; so does a +builder called on a model's ``__table__``. A session factory is any call into +``policyengine_api.data.orm`` or ``policyengine_api.data.v2.database``, +SQLAlchemy's ``Session``, ``sessionmaker`` or ``create_engine``, or an injected +``*session_factory`` (or its ``begin()``); services may open sessions, and +nothing else checked here may. Imports made only under ``TYPE_CHECKING`` +count. Not recognised: a session bound to another name, an alias of a model's +table, and a name re-exported through another module. A ratcheted rule's baseline file lists its tolerated findings, one line per occurrence, as the guard prints them. More occurrences than the baseline lists @@ -79,7 +82,15 @@ SERVICES_RULE = "api.v2.services-no-sql" CONNECTORS_RULE = "api.v2.connectors-never-commit" PURE_RULE = "api.v2.pure-modules-no-session-calls" -RULES = (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE, PURE_RULE) +SESSION_RULE = "api.v2.session-modules-no-data-access" +RULES = ( + FILES_RULE, + ROUTES_RULE, + SERVICES_RULE, + CONNECTORS_RULE, + PURE_RULE, + SESSION_RULE, +) SERVICE_FILE_LAYERS = { "services.py": "service", @@ -152,8 +163,12 @@ "scalars", } ) +# A database_session container's context managers, which open a session. +CONTAINER_METHODS = frozenset({"read", "transaction"}) # Receiver name prefixes, before "_session", of web rather than database sessions. -WEB_SESSION_WORDS = ("client", "http", "request") +WEB_SESSION_WORDS = ("client", "http", "requests") +# Owners whose ``.session`` is Starlette's session data, not a database session. +REQUEST_OWNERS = frozenset({"req", "request", "websocket", "ws"}) HTTP_PACKAGES = ("fastapi", "starlette") FIXES = { @@ -183,6 +198,10 @@ "load them in a connector and pass them in from the service " f"({ORGANIZATION_DOC})" ), + SESSION_RULE: ( + "a database_session module only opens, commits, rolls back and closes " + f"sessions; read and write rows in a connector ({ORGANIZATION_DOC})" + ), } @@ -334,6 +353,12 @@ def _is_route_forbidden_import(module: str) -> bool: ) +def _is_connector_module(module: str) -> bool: + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in module.split(".") + ) + + def _is_connector_forbidden_import(module: str) -> bool: parts = module.split(".") return _within_any(module, DATABASE_PACKAGES) or ( @@ -387,12 +412,17 @@ def _receiver_name(node: ast.expr) -> str: return "" +def _is_request_owner(node: ast.expr) -> bool: + name = _receiver_name(node).lower().lstrip("_") + return name in REQUEST_OWNERS or name.endswith("_request") + + def _is_session_receiver(node: ast.expr) -> bool: """Whether a method's receiver is a database session or its container.""" if ( isinstance(node, ast.Attribute) and node.attr == "session" - and "request" in _receiver_name(node.value).lower() + and _is_request_owner(node.value) ): return False name = _receiver_name(node).lower().lstrip("_") @@ -402,6 +432,28 @@ def _is_session_receiver(node: ast.expr) -> bool: return prefix != name and not any(word in prefix for word in WEB_SESSION_WORDS) +def _is_session_factory_name(name: str) -> bool: + return name.lower().lstrip("_").endswith("session_factory") + + +def _injected_factory_detail(call: ast.Call) -> str | None: + """Return ``calls `` when a call opens a session from an injected factory. + + ``session_factory()``, ``self._session_factory()`` and + ``session_factory.begin()`` count. + """ + func = call.func + if _is_session_factory_name(_receiver_name(func)): + return f"calls {_receiver_name(func)}" + if ( + isinstance(func, ast.Attribute) + and func.attr == "begin" + and _is_session_factory_name(_receiver_name(func.value)) + ): + return f"calls {_receiver_name(func.value)}.begin" + return None + + def _calls_on_a_table(call: ast.Call) -> bool: """Whether a call is a SQL builder on a table, as ``Model.__table__.insert()``.""" func = call.func @@ -422,7 +474,9 @@ class LayerPolicy: ``session_methods`` of None forbids every method on a session. ``checks_sql_calls`` forbids every call into SQLAlchemy or SQLModel but - its exception classes, and builders called on a model's ``__table__``. + its exception classes; ``checks_builders`` only its SQL builders. Either + also forbids builders called on a model's ``__table__``. + ``checks_factories`` forbids opening a session from a session factory. """ rule_id: str @@ -431,6 +485,7 @@ class LayerPolicy: checks_sql_calls: bool checks_factories: bool forbidden_name: Callable[[str], bool] = _never + checks_builders: bool = False ROUTE_POLICY = LayerPolicy( @@ -460,10 +515,18 @@ class LayerPolicy: "connector": LayerPolicy( CONNECTORS_RULE, _is_connector_forbidden_import, - TRANSACTION_METHODS, + TRANSACTION_METHODS | CONTAINER_METHODS, checks_sql_calls=False, checks_factories=True, ), + "session": LayerPolicy( + SESSION_RULE, + _is_connector_module, + DATA_ACCESS_METHODS, + checks_sql_calls=False, + checks_factories=False, + checks_builders=True, + ), **dict.fromkeys(PURE_LAYERS, PURE_POLICY), } @@ -505,6 +568,8 @@ def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: return [f"calls {last}"] if policy.checks_factories else [] if policy.checks_sql_calls and from_sql and not _within(dotted, SQL_EXCEPTIONS): return [f"calls {dotted}"] + if policy.checks_builders and from_sql and last in SQL_BUILDERS: + return [f"calls {dotted}"] return [] @@ -515,8 +580,11 @@ def _call_details( dotted = _dotted_name(call.func, imported) if dotted is not None: details.extend(_named_call_details(dotted, policy)) - if policy.checks_sql_calls and _calls_on_a_table(call): + if (policy.checks_sql_calls or policy.checks_builders) and _calls_on_a_table(call): details.append(f"calls {ast.unparse(call.func)}") + factory = _injected_factory_detail(call) if policy.checks_factories else None + if factory is not None: + details.append(factory) func = call.func forbidden_method = isinstance(func, ast.Attribute) and ( policy.session_methods is None or func.attr in policy.session_methods From a4b2b790bf0884282362b77adffc6514f9782fd5 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:31:59 +0400 Subject: [PATCH 10/15] Test layers taken from file names only, with registry exceptions (failing) - New name rules: process_state.py is process-state, and under fastapi_routes/v2 any _models.py is http-model and any _support.py is http-support. - A file whose name gives no layer can only be allowed by a [[rule.exception]] for api.v2.files-assigned-to-layers. The exception chooses no layer: the file gets its directory's strictest checks, the route checks under fastapi_routes/v2 and the pure-module checks under services/v2. An exception for a missing file, a file whose name already gives a layer, a glob, or a path outside the two directories fails. - The files rule's proof shows both an unassigned name and two excepted files getting the strictest checks. - The tests import the eight renamed modules by their new names: process_state, errors_support, user_auth_support, user_resources_support, route_support, query_support, response_cache_support and search_file_support. These fail until the guard reads exceptions instead of the manifest and the files are renamed. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 25 ++- .../good_tree.toml | 12 +- scripts/rules/tests/test_v2_layers.py | 202 +++++++++--------- tests/fixtures/v2_metadata_service_helpers.py | 2 +- tests/integration/test_v2_metadata_routes.py | 8 +- tests/unit/v2/test_metadata_response_cache.py | 2 +- tests/unit/v2/test_metadata_route_caching.py | 21 +- tests/unit/v2/test_metadata_routes.py | 2 +- tests/unit/v2/test_metadata_service.py | 2 +- .../test_metadata_service_catalog_records.py | 2 +- .../v2/test_parameter_value_listing_route.py | 4 +- tests/unit/v2/test_user_resource_failures.py | 12 +- tests/unit/v2/test_user_route_auth.py | 14 +- tests/unit/v2/test_user_write_limits.py | 2 +- 14 files changed, 172 insertions(+), 138 deletions(-) diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml index 872476c4e..52205b84c 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -1,10 +1,21 @@ # Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject -# it: simulation_client.py matches no layer's file name and no manifest entry -# names it (the case of an open pull request that adds a v2 resource). +# it: +# - simulation_client.py's name gives it no layer, and no [[rule.exception]] +# names it (the case of an open pull request that adds a v2 resource); +# - the two files listed in ``exceptions`` stand for files a +# [[rule.exception]] names. They get no layer, but their directory's +# strictest checks: the pure-module checks under services/v2 and the route +# checks under fastapi_routes/v2. # scripts/rules/tests/test_v2_layers.py writes each file into a temporary -# directory and runs the v2-layers guard's scan on it, with no manifest. +# directory and runs the v2-layers guard's scan on it. +exceptions = [ + "policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py", + "policyengine_api/services/v2/household_calculations/legacy_client.py", +] expected_findings = [ + "policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", + "policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", "policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", ] @@ -16,3 +27,11 @@ from policyengine_api.services.v2.household_calculations.simulation_client impor "policyengine_api/services/v2/household_calculations/simulation_client.py" = """ import httpx """ +"policyengine_api/services/v2/household_calculations/legacy_client.py" = """ +def load(session, household_id): + return session.get(household_id) +""" +"policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py" = """ +def keep(session, row): + session.add(row) +""" diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml index 72f7a07cb..32baf28f5 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml @@ -1,6 +1,8 @@ # Proof fixture for rule api.v2.files-assigned-to-layers. The guard must accept -# it: every file is named after its layer. +# it: every file is named after its layer, and the one excepted file passes +# the strictest checks of its directory. +exceptions = ["policyengine_api/services/v2/household_calculations/legacy_client.py"] expected_findings = [] [files] @@ -10,14 +12,22 @@ expected_findings = [] "policyengine_api/services/v2/policies/validators.py" = "" "policyengine_api/services/v2/policies/transformations.py" = "" "policyengine_api/services/v2/policies/types.py" = "" +"policyengine_api/services/v2/policies/process_state.py" = "" "policyengine_api/services/v2/policies/database_session.py" = "" "policyengine_api/services/v2/policies/database_connectors/__init__.py" = "" "policyengine_api/services/v2/policies/database_connectors/creates.py" = "" "policyengine_api/services/v2/policies/database_connectors/reads_parameters.py" = "" +"policyengine_api/services/v2/household_calculations/legacy_client.py" = """ +def describe(household): + return household.label +""" "policyengine_api/fastapi_routes/v2/__init__.py" = "" "policyengine_api/fastapi_routes/v2/routes.py" = "" +"policyengine_api/fastapi_routes/v2/errors_support.py" = "" "policyengine_api/fastapi_routes/v2/policies/__init__.py" = "" "policyengine_api/fastapi_routes/v2/policies/routes.py" = "" "policyengine_api/fastapi_routes/v2/policies/request_models.py" = "" "policyengine_api/fastapi_routes/v2/policies/response_models.py" = "" +"policyengine_api/fastapi_routes/v2/households/document_models.py" = "" "policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py" = "" +"policyengine_api/fastapi_routes/v2/metadata/route_support.py" = "" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 4e338dbe6..6e1d9b386 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -40,16 +40,30 @@ def _findings(files: dict[str, str], rule_id: str) -> set[str]: root = Path(directory) _write_files(files, root) return { - str(finding) for finding in scan(root, {}) if finding.rule_id == rule_id + str(finding) for finding in scan(root, ()) if finding.rule_id == rule_id } +def _all_findings(files: dict[str, str], exceptions: tuple[str, ...]) -> set[str]: + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + return {str(finding) for finding in scan(root, exceptions)} + + def _assert_proof(test: unittest.TestCase, rule_id: str) -> None: + """Each tree gives exactly its expected findings, of any rule. + + A tree's ``exceptions`` list the files a ``[[rule.exception]]`` for + api.v2.files-assigned-to-layers would name. + """ for fixture in ("bad_tree.toml", "good_tree.toml"): tree = tomllib.loads((FIXTURES / rule_id / fixture).read_text()) + exceptions = tuple(tree.get("exceptions", ())) with test.subTest(fixture=fixture): test.assertEqual( - _findings(tree["files"], rule_id), set(tree["expected_findings"]) + _all_findings(tree["files"], exceptions), + set(tree["expected_findings"]), ) bad = tomllib.loads((FIXTURES / rule_id / "bad_tree.toml").read_text()) test.assertTrue(bad["expected_findings"], "the bad fixture must be rejected") @@ -102,22 +116,32 @@ def test_files_are_assigned_by_name(self) -> None: "policyengine_api/fastapi_routes/v2/policies/response_models.py": ( "http-model" ), + "policyengine_api/services/v2/metadata/process_state.py": "process-state", + "policyengine_api/fastapi_routes/v2/households/document_models.py": ( + "http-model" + ), + "policyengine_api/fastapi_routes/v2/reports/" + "household_response_models.py": "http-model", + "policyengine_api/fastapi_routes/v2/errors_support.py": "http-support", + "policyengine_api/fastapi_routes/v2/metadata/route_support.py": ( + "http-support" + ), } for path, layer in cases.items(): with self.subTest(path=path): - self.assertEqual(assign_layer(path, {}), layer) + self.assertEqual(assign_layer(path, ()), layer) - def test_other_names_have_no_layer_unless_the_manifest_assigns_one(self) -> None: + def test_other_names_have_no_layer_unless_excepted(self) -> None: unnamed = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" misnamed_connector = ( "policyengine_api/services/v2/policies/database_connectors/helpers.py" ) + support_outside_routes = "policyengine_api/services/v2/policies/x_support.py" - self.assertIsNone(assign_layer(unnamed, {})) - self.assertIsNone(assign_layer(misnamed_connector, {})) - self.assertEqual( - assign_layer(unnamed, {unnamed: "http-support"}), "http-support" - ) + self.assertIsNone(assign_layer(unnamed, ())) + self.assertIsNone(assign_layer(misnamed_connector, ())) + self.assertIsNone(assign_layer(support_outside_routes, ())) + self.assertEqual(assign_layer(unnamed, (unnamed,)), "excepted") def test_every_file_in_the_repository_has_a_layer(self) -> None: violations = [ @@ -389,39 +413,36 @@ def write(session): }, ) - def test_manifest_layers_get_their_layer_s_checks(self) -> None: - process_state = "policyengine_api/services/v2/metadata/selected_catalogs.py" - support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" - manifest = {process_state: "process-state", support: "http-support"} + def test_an_excepted_file_gets_its_directory_s_strictest_checks(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + helper = "policyengine_api/fastapi_routes/v2/metadata/legacy_helper.py" + process_state = "policyengine_api/services/v2/metadata/process_state.py" files = { - process_state: ( + client: ( "from policyengine_api.data.orm import get_v1_session_factory\n" - "from .database_connectors.reads import read_rows\n" "def load(session):\n" " get_v1_session_factory()\n" - " return session.exec(read_rows)\n" + " return session.exec(1)\n" ), - support: "def keep(session, row):\n session.add(row)\n", + helper: "def keep(session, row):\n session.add(row)\n", + process_state: "def load(session):\n return session.exec(1)\n", } with tempfile.TemporaryDirectory() as directory: root = Path(directory) _write_files(files, root) found = { - (finding.rule_id, str(finding)) for finding in scan(root, manifest) + (finding.rule_id, str(finding)) + for finding in scan(root, (client, helper)) } self.assertEqual( found, { - (PURE_RULE, f"{process_state} calls get_v1_session_factory"), + (PURE_RULE, f"{client} calls get_v1_session_factory"), + (PURE_RULE, f"{client} calls session.exec"), + (PURE_RULE, f"{client} imports policyengine_api.data.orm"), (PURE_RULE, f"{process_state} calls session.exec"), - (PURE_RULE, f"{process_state} imports policyengine_api.data.orm"), - ( - PURE_RULE, - f"{process_state} imports " - "policyengine_api.services.v2.metadata.database_connectors.reads", - ), - (ROUTES_RULE, f"{support} calls session.add"), + (ROUTES_RULE, f"{helper} calls session.add"), }, ) @@ -542,15 +563,17 @@ def run(request_session, statement): ) -class TestBaselineAndManifest(unittest.TestCase): - """run_check compares findings with the rule's baseline and checks the manifest.""" +class TestBaselineAndExceptions(unittest.TestCase): + """run_check compares findings with the baseline and checks the exceptions.""" def setUp(self) -> None: self._directory = tempfile.TemporaryDirectory() self.addCleanup(self._directory.cleanup) self.root = Path(self._directory.name) - def _registry(self, baseline_count: int | None) -> str: + def _registry( + self, baseline_count: int | None, exceptions: tuple[str, ...] = () + ) -> str: rules = [] for rule_id in (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE): ratcheted = rule_id == ROUTES_RULE and baseline_count is not None @@ -559,6 +582,10 @@ def _registry(self, baseline_count: int | None) -> str: if ratcheted else "" ) + excepted = "".join( + f'[[rule.exception]]\npath = "{path}"\nreason = "Generated."\n' + for path in (exceptions if rule_id == FILES_RULE else ()) + ) rules.append( textwrap.dedent( f""" @@ -581,6 +608,7 @@ def _registry(self, baseline_count: int | None) -> str: """ ) + baseline + + excepted ) header = ( 'schema_version = 1\nrepo = "example"\n\n' @@ -664,44 +692,6 @@ def test_a_duplicated_baseline_entry_is_stale(self) -> None: ], ) - def test_the_manifest_gives_only_layers_of_the_file_s_directory(self) -> None: - client = "policyengine_api/services/v2/household_calculations/client.py" - support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" - _write_files( - { - client: "", - support: "", - "rules/registry.toml": self._registry(None), - "rules/v2-layer-manifest.toml": textwrap.dedent( - f""" - [files."{client}"] - layer = "package" - reason = "Not a package." - - [files."{support}"] - layer = "http-support" - reason = "" - """ - ), - }, - self.root, - ) - - self.assertEqual( - sorted(self._messages()), - sorted( - [ - f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " - f"{client} the layer 'package', which a file under " - "policyengine_api/services/v2 can't have from the manifest. Fix: " - "use one of process-state, service, transformation, types, " - "validator.", - f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " - f"{support} no reason. Fix: say why the file has that layer.", - ] - ), - ) - def test_a_baseline_entry_that_no_longer_applies_fails(self) -> None: _write_files({ROUTE: "def route():\n return 1\n"}, self.root) _write_files( @@ -732,56 +722,68 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: [ f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " "name it after its layer (services.py, validators.py, " - "transformations.py, types.py, database_session.py, " + "transformations.py, types.py, process_state.py, " + "database_session.py, " "database_connectors/[_name].py, " - "routes.py, _routes.py, request_models.py or " - "response_models.py), or add it to rules/v2-layer-manifest.toml " - "with its layer and a reason." + "routes.py, _routes.py, _models.py or " + "_support.py), or, if it can't be renamed, add a " + "[[rule.exception]] for api.v2.files-assigned-to-layers to " + "rules/registry.toml with its path and a reason." ], ) - def test_manifest_entries_must_name_existing_unmatched_files_and_known_layers( - self, - ) -> None: - cache = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" _write_files( { - cache: "", - ROUTE: "", - "rules/registry.toml": self._registry(None), - "rules/v2-layer-manifest.toml": textwrap.dedent( - f""" - [files."{cache}"] - layer = "cache" - reason = "Keeps serialized responses." + client: "def load(session):\n return session.get(1)\n", + "rules/registry.toml": self._registry(None, (client,)), + }, + self.root, + ) - [files."{ROUTE}"] - layer = "route" - reason = "Named routes.py already." + self.assertEqual( + self._messages(), + [ + f"[{PURE_RULE}] v2-layers: {client} calls session.get. Fix: " + "validators, transformations and types work on values and loaded " + "rows; load them in a connector and pass them in from the service " + "(docs/engineering/skills/v2-code-organization.md)." + ], + ) - [files."policyengine_api/services/v2/gone.py"] - layer = "service" - reason = "Removed." - """ + def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: + named = ROUTE + missing = "policyengine_api/services/v2/policies/gone.py" + pattern = "policyengine_api/services/v2/*/client.py" + outside = "policyengine_api/libs/user_auth.py" + _write_files( + { + named: "", + outside: "", + "rules/registry.toml": self._registry( + None, (named, missing, pattern, outside) ), }, self.root, ) + prefix = f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts" + one_file = ( + "which is not one file under policyengine_api/services/v2 or " + "policyengine_api/fastapi_routes/v2. Fix: name one file there, or " + "remove the exception." + ) self.assertEqual( sorted(self._messages()), sorted( [ - f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " - f"{cache} the layer 'cache', which a file under " - "policyengine_api/fastapi_routes/v2 can't have from the " - "manifest. Fix: use one of http-model, http-support, route.", - f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " - f"{ROUTE}, which its name already assigns to a layer. Fix: " - "remove the entry.", - f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " - "policyengine_api/services/v2/gone.py, which does not exist. " - "Fix: remove the entry.", + f"{prefix} {named} from {FILES_RULE}, but its name already " + "gives it a layer. Fix: remove the exception.", + f"{prefix} {missing} from {FILES_RULE}, but it does not " + "exist. Fix: remove the exception.", + f"{prefix} {pattern} from {FILES_RULE}, {one_file}", + f"{prefix} {outside} from {FILES_RULE}, {one_file}", ] ), ) diff --git a/tests/fixtures/v2_metadata_service_helpers.py b/tests/fixtures/v2_metadata_service_helpers.py index fdc6dbd5c..f847d3017 100644 --- a/tests/fixtures/v2_metadata_service_helpers.py +++ b/tests/fixtures/v2_metadata_service_helpers.py @@ -25,7 +25,7 @@ from tests.fixtures.v2_catalog import POLICYENGINE_VERSION, normalized_catalog if TYPE_CHECKING: - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index 82f42645d..ec1af10b2 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -616,10 +616,10 @@ def test_postgres_production_metadata_reader_keeps_catalogs_and_responses( # The production combination: autocommit read sessions, the process's # selected catalogs and its whole-version response cache. from policyengine_api.data.v2.database import build_v2_read_session_factory - from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( + from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) @@ -715,11 +715,11 @@ def test_postgres_current_values_and_their_lifetime_use_one_instant( monkeypatch: pytest.MonkeyPatch, case: tuple[datetime, datetime, list[float], str], ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support read_instant, header_instant, expected_values, expected_cache_control = case instants = iter([read_instant, header_instant]) - monkeypatch.setattr(common, "utc_now", lambda: next(instants)) + monkeypatch.setattr(route_support, "utc_now", lambda: next(instants)) response = _client(published_engine).get( "/v2/parameter-values", diff --git a/tests/unit/v2/test_metadata_response_cache.py b/tests/unit/v2/test_metadata_response_cache.py index a0a788c1c..fafa93b1f 100644 --- a/tests/unit/v2/test_metadata_response_cache.py +++ b/tests/unit/v2/test_metadata_response_cache.py @@ -8,7 +8,7 @@ import pytest from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( BuiltResponse, KeptResponse, MetadataResponseCache, diff --git a/tests/unit/v2/test_metadata_route_caching.py b/tests/unit/v2/test_metadata_route_caching.py index 89719991e..0115d0a49 100644 --- a/tests/unit/v2/test_metadata_route_caching.py +++ b/tests/unit/v2/test_metadata_route_caching.py @@ -10,7 +10,7 @@ MetadataCatalogUnavailableError, ) from policyengine_api.fastapi_routes import dependencies as route_dependencies -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) from policyengine_api.services.v2.metadata.validators import ( @@ -190,9 +190,9 @@ def test_current_parameter_values_are_reused_only_until_the_next_utc_day( query: str, expected_cache_control: str, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support - monkeypatch.setattr(common, "utc_now", lambda: now) + monkeypatch.setattr(route_support, "utc_now", lambda: now) results, _ids = resource_results() response = metadata_route_client(lambda: ResourceReader(results)).get( @@ -206,17 +206,20 @@ def test_current_parameter_values_are_reused_only_until_the_next_utc_day( def test_a_kept_response_is_served_without_hashing_its_body_again( monkeypatch: pytest.MonkeyPatch, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common, response_cache + from policyengine_api.fastapi_routes.v2.metadata import ( + route_support, + response_cache_support, + ) hashed_bodies: list[int] = [] - original_entity_tag = response_cache.body_entity_tag + original_entity_tag = response_cache_support.body_entity_tag def counting_entity_tag(body: bytes) -> str: hashed_bodies.append(len(body)) return original_entity_tag(body) - monkeypatch.setattr(response_cache, "body_entity_tag", counting_entity_tag) - monkeypatch.setattr(common, "body_entity_tag", counting_entity_tag) + monkeypatch.setattr(response_cache_support, "body_entity_tag", counting_entity_tag) + monkeypatch.setattr(route_support, "body_entity_tag", counting_entity_tag) results, _ids = resource_results() client = metadata_route_client( lambda: ResourceReader(results), response_cache=MetadataResponseCache() @@ -259,11 +262,11 @@ def test_current_values_read_just_before_midnight_expire_at_that_midnight( # The read and the response headers must use the same day: values read for # 31 December may not be reused into 1 January, even when the headers are # written after midnight. - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support read_instant = datetime(2026, 12, 31, 23, 59, 59, tzinfo=UTC) instants = iter([read_instant, datetime(2027, 1, 1, 0, 0, 1, tzinfo=UTC)]) - monkeypatch.setattr(common, "utc_now", lambda: next(instants)) + monkeypatch.setattr(route_support, "utc_now", lambda: next(instants)) results, _ids = resource_results() reader = ResourceReader(results) diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 6d92bf348..fbab7a0dc 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -14,7 +14,7 @@ ) from policyengine_api.data.v2.settings import V2ConfigurationError from policyengine_api.fastapi_routes import dependencies as route_dependencies -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) from policyengine_api.services.v2.metadata.types import ( diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index 33e699264..2fa5f0d93 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -32,7 +32,7 @@ from policyengine_api.services.v2.metadata.database_session import ( MetadataDatabaseSession, ) -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) from policyengine_api.services.v2.metadata.services import V2MetadataService diff --git a/tests/unit/v2/test_metadata_service_catalog_records.py b/tests/unit/v2/test_metadata_service_catalog_records.py index da9183e3e..0d7bf614f 100644 --- a/tests/unit/v2/test_metadata_service_catalog_records.py +++ b/tests/unit/v2/test_metadata_service_catalog_records.py @@ -416,7 +416,7 @@ def test_both_variables_routes_serve_unit_and_definition_period( expected_catalog_format: int | None, expected_fields: dict[str, tuple[str | None, str | None]], ) -> None: - from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( + from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) from tests.fixtures.v2_metadata_routes import metadata_route_client diff --git a/tests/unit/v2/test_parameter_value_listing_route.py b/tests/unit/v2/test_parameter_value_listing_route.py index 73b98d472..2e297119d 100644 --- a/tests/unit/v2/test_parameter_value_listing_route.py +++ b/tests/unit/v2/test_parameter_value_listing_route.py @@ -17,10 +17,10 @@ def test_listing_forwards_every_query_parameter( monkeypatch: pytest.MonkeyPatch, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support request_instant = datetime(2026, 6, 1, 12, 0, tzinfo=UTC) - monkeypatch.setattr(common, "utc_now", lambda: request_instant) + monkeypatch.setattr(route_support, "utc_now", lambda: request_instant) results, ids = resource_results() reader = ResourceReader(results) diff --git a/tests/unit/v2/test_user_resource_failures.py b/tests/unit/v2/test_user_resource_failures.py index 420fc26b2..b08df7ea6 100644 --- a/tests/unit/v2/test_user_resource_failures.py +++ b/tests/unit/v2/test_user_resource_failures.py @@ -10,7 +10,7 @@ from sqlalchemy.exc import IntegrityError, OperationalError from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2 import user_resources +from policyengine_api.fastapi_routes.v2 import user_resources_support from policyengine_api.services.v2.associations.validators import ( AssociationConflictError, AssociationLinkNotFoundError, @@ -60,10 +60,10 @@ def failure_records( # Alembic tests run logging.config.fileConfig, which disables loggers that # already exist, so re-enable this one and record on it directly. handler = _RecordingHandler() - monkeypatch.setattr(user_resources.logger, "disabled", False) - user_resources.logger.addHandler(handler) + monkeypatch.setattr(user_resources_support.logger, "disabled", False) + user_resources_support.logger.addHandler(handler) yield handler.records - user_resources.logger.removeHandler(handler) + user_resources_support.logger.removeHandler(handler) def _failing(error: Exception): @@ -183,7 +183,7 @@ class _FailureCase(NamedTuple): def test_each_failure_maps_to_its_typed_response( failure_records: list[logging.LogRecord], case: _FailureCase ) -> None: - response = user_resources.user_resource_operation(_failing(case.error)) + response = user_resources_support.user_resource_operation(_failing(case.error)) assert response.status_code == case.status_code body = json.loads(response.body) @@ -195,4 +195,4 @@ def test_each_failure_maps_to_its_typed_response( def test_a_successful_operation_returns_its_result() -> None: - assert user_resources.user_resource_operation(lambda: "result") == "result" + assert user_resources_support.user_resource_operation(lambda: "result") == "result" diff --git a/tests/unit/v2/test_user_route_auth.py b/tests/unit/v2/test_user_route_auth.py index 37c4c4c35..d6df2dcc2 100644 --- a/tests/unit/v2/test_user_route_auth.py +++ b/tests/unit/v2/test_user_route_auth.py @@ -245,7 +245,7 @@ def test_default_dependencies_publish_the_bearer_security_scheme() -> None: def _recorded_failure_logs(operation) -> tuple[object, list]: import logging - from policyengine_api.fastapi_routes.v2 import user_resources + from policyengine_api.fastapi_routes.v2 import user_resources_support class RecordingHandler(logging.Handler): def __init__(self) -> None: @@ -258,14 +258,14 @@ def emit(self, record: logging.LogRecord) -> None: # Alembic tests run logging.config.fileConfig, which disables loggers that # already exist, so re-enable this one and record on it directly. handler = RecordingHandler() - was_disabled = user_resources.logger.disabled - user_resources.logger.disabled = False - user_resources.logger.addHandler(handler) + was_disabled = user_resources_support.logger.disabled + user_resources_support.logger.disabled = False + user_resources_support.logger.addHandler(handler) try: - response = user_resources.user_resource_operation(operation) + response = user_resources_support.user_resource_operation(operation) finally: - user_resources.logger.removeHandler(handler) - user_resources.logger.disabled = was_disabled + user_resources_support.logger.removeHandler(handler) + user_resources_support.logger.disabled = was_disabled return response, handler.records diff --git a/tests/unit/v2/test_user_write_limits.py b/tests/unit/v2/test_user_write_limits.py index da691a477..85789a9b9 100644 --- a/tests/unit/v2/test_user_write_limits.py +++ b/tests/unit/v2/test_user_write_limits.py @@ -9,7 +9,7 @@ import pytest from pydantic import ValidationError -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( MAXIMUM_USER_WRITE_REQUEST_BYTES, ) from policyengine_api.services.v2.associations.types import ( From 36fda358ea81c4ef3a9585a8003d191074cb93d7 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:35:09 +0400 Subject: [PATCH 11/15] Take v2 layers from file names only, and rename the files that don't fit - The v2-layers guard no longer reads rules/v2-layer-manifest.toml, which is deleted. New name rules: process_state.py under services/v2, and _models.py and _support.py under fastapi_routes/v2. - A file that can't be renamed needs a [[rule.exception]] for api.v2.files-assigned-to-layers in rules/registry.toml. It gets no layer, but its directory's strictest checks; an exception for a missing file, a file whose name gives a layer, a glob or a path outside the two directories fails. None is needed today. - Renamed, with every import updated: - services/v2/metadata/selected_catalogs.py -> process_state.py - fastapi_routes/v2/errors.py -> errors_support.py - fastapi_routes/v2/user_auth.py -> user_auth_support.py - fastapi_routes/v2/user_resources.py -> user_resources_support.py - fastapi_routes/v2/metadata/common.py -> route_support.py - fastapi_routes/v2/metadata/query_dependencies.py -> query_support.py - fastapi_routes/v2/metadata/response_cache.py -> response_cache_support.py - fastapi_routes/v2/metadata/search_file_response.py -> search_file_support.py - v2-code-organization.md, the registry, the routes baseline and the rendered rules.md and AGENTS.md follow. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4190.added.md | 2 +- .../skills/v2-code-organization.md | 67 +++++--- docs/generated/rules.md | 2 +- policyengine_api/asgi_factory.py | 2 +- .../fastapi_routes/dependencies.py | 8 +- .../v2/countries/response_models.py | 2 +- .../fastapi_routes/v2/countries/routes.py | 5 +- .../v2/{errors.py => errors_support.py} | 0 .../v2/households/response_models.py | 2 +- .../fastapi_routes/v2/households/routes.py | 2 +- .../fastapi_routes/v2/me/routes.py | 8 +- .../v2/metadata/catalog_routes.py | 14 +- .../v2/metadata/geography_routes.py | 12 +- .../v2/metadata/model_routes.py | 14 +- .../v2/metadata/parameter_routes.py | 24 +-- ...query_dependencies.py => query_support.py} | 0 ...nse_cache.py => response_cache_support.py} | 0 .../metadata/{common.py => route_support.py} | 4 +- ...ile_response.py => search_file_support.py} | 2 +- .../v2/policies/response_models.py | 2 +- .../fastapi_routes/v2/policies/routes.py | 2 +- policyengine_api/fastapi_routes/v2/routes.py | 2 +- .../v2/{user_auth.py => user_auth_support.py} | 2 +- .../v2/user_households/response_models.py | 2 +- .../v2/user_households/routes.py | 4 +- .../v2/user_policies/response_models.py | 2 +- .../fastapi_routes/v2/user_policies/routes.py | 4 +- .../fastapi_routes/v2/user_reports/routes.py | 4 +- ...resources.py => user_resources_support.py} | 2 +- .../v2/user_simulations/routes.py | 4 +- ...{selected_catalogs.py => process_state.py} | 0 .../services/v2/metadata/services.py | 2 +- rules/baselines/v2-layers-routes.txt | 2 +- rules/registry.toml | 4 +- rules/v2-layer-manifest.toml | 50 ------ scripts/rules/v2_layers.py | 151 ++++++++---------- tests/unit/v2/test_metadata_route_caching.py | 2 +- 37 files changed, 183 insertions(+), 228 deletions(-) rename policyengine_api/fastapi_routes/v2/{errors.py => errors_support.py} (100%) rename policyengine_api/fastapi_routes/v2/metadata/{query_dependencies.py => query_support.py} (100%) rename policyengine_api/fastapi_routes/v2/metadata/{response_cache.py => response_cache_support.py} (100%) rename policyengine_api/fastapi_routes/v2/metadata/{common.py => route_support.py} (99%) rename policyengine_api/fastapi_routes/v2/metadata/{search_file_response.py => search_file_support.py} (97%) rename policyengine_api/fastapi_routes/v2/{user_auth.py => user_auth_support.py} (98%) rename policyengine_api/fastapi_routes/v2/{user_resources.py => user_resources_support.py} (99%) rename policyengine_api/services/v2/metadata/{selected_catalogs.py => process_state.py} (100%) delete mode 100644 rules/v2-layer-manifest.toml diff --git a/changelog.d/4190.added.md b/changelog.d/4190.added.md index 0cc611cf1..f0763f03b 100644 --- a/changelog.d/4190.added.md +++ b/changelog.d/4190.added.md @@ -1 +1 @@ -Check that every API v2 route and service file has a layer, and that v2 route modules open no session and build no SQL, services construct no SQL, connectors never open, commit, roll back or close a session, and validators, transformations and types make no session calls, in a new v2-layers quality guard. +Check that every API v2 route and service file is named after its layer, and that v2 route modules open no session and build no SQL, services construct no SQL, connectors never open, commit, roll back or close a session, session modules do no data access, and validators, transformations and types make no session calls, in a new v2-layers quality guard. Eight v2 modules are renamed to fit (for example `fastapi_routes/v2/errors.py` is now `errors_support.py`). diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index fb3d964e0..d5bbbc190 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -10,17 +10,25 @@ but must not construct or execute SQL (rule `api.v2.services-no-sql`). Every `.py` file under `policyengine_api/services/v2` and `policyengine_api/fastapi_routes/v2` has a layer, which the `v2-layers` quality -guard (`scripts/rules/v2_layers.py`) takes from the file's name: `services.py`, -`validators.py`, `transformations.py`, `types.py`, `database_session.py`, -`database_connectors/[_name].py`, `routes.py`, -`_routes.py`, `request_models.py`, `response_models.py` or `__init__.py`. -A file with another name needs an entry, with its layer and a reason, in -`rules/v2-layer-manifest.toml`, or the guard fails (rule -`api.v2.files-assigned-to-layers`). The manifest can give a file under -`services/v2` the layer `service`, `validator`, `transformation`, `types` or -`process-state`, and a file under `fastapi_routes/v2` the layer `route`, -`http-model` or `http-support`; each layer follows that layer's rules below. -A `database_session.py` sits directly in its resource package. +guard (`scripts/rules/v2_layers.py`) takes from the file's name alone: + +- under `services/v2//`: `services.py` (service), `validators.py`, + `transformations.py`, `types.py`, `process_state.py` (process-wide state), + `database_session.py` (directly in the resource package) and + `database_connectors/[_name].py`; +- under `fastapi_routes/v2/`: `routes.py` and `_routes.py` (routes), + `_models.py` (request, response and document schemas) and + `_support.py` (HTTP code the route modules share); +- `__init__.py` anywhere. + +A file with another name fails the guard (rule +`api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed +needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its +path and a reason. An exception chooses no layer: the file gets its +directory's strictest checks, the route checks under `fastapi_routes/v2` and +the validator, transformation and types checks under `services/v2`. The guard +fails on an exception for a file that no longer exists or whose name already +gives it a layer. The guard counts a method call as a session call when its receiver is named `session` or `_session`, but not a request's `.session` or an HTTP @@ -42,7 +50,9 @@ line per tolerated occurrence. Resource-specific FastAPI code lives under `policyengine_api/fastapi_routes/v2/`: ```text -errors.py +errors_support.py +user_auth_support.py +user_resources_support.py policies/ request_models.py response_models.py @@ -53,21 +63,27 @@ user_policies/ routes.py metadata/ response_models.py - common.py - query_dependencies.py - response_cache.py + route_support.py + query_support.py + response_cache_support.py + search_file_support.py *_routes.py ``` -`common.py` also holds the route class that gives metadata responses their -caching headers. `query_dependencies.py` defines the typed FastAPI +`route_support.py` runs the metadata reads for the route modules, maps their +failures to HTTP, and holds the route class that gives metadata responses +their caching headers. `query_support.py` defines the typed FastAPI dependencies for the shared query models once, for every metadata route -module to import. `response_cache.py` keeps serialized whole-version +module to import. `response_cache_support.py` keeps serialized whole-version responses (bodies and ETags) for the process; it is HTTP-level, so it lives -with the routes. - -`errors.py` defines the strict error response and serialization function shared -by every API v2 resource. Application-wide exception handling distinguishes +with the routes. `search_file_support.py` serves a stored parameter search +file with its content encoding. + +`errors_support.py` defines the strict error response and serialization +function shared by every API v2 resource. `user_auth_support.py` turns +request credentials into an account principal, and +`user_resources_support.py` holds the HTTP conventions the user-owned +resource routes share. Application-wide exception handling distinguishes API v2 from non-v2 requests but must not select error behavior by matching individual resource paths. @@ -100,10 +116,9 @@ services/v2// deletes.py ``` -A resource may also add a module for process-wide state its service reuses -across requests, named for what it keeps, such as the metadata package's -`selected_catalogs.py` (each catalog version a request has selected, which -never changes once published). Such a module holds plain data, constructs no +A resource may also add a `process_state.py` module for process-wide state its +service reuses across requests, such as the metadata package's (each catalog +version a request has selected, which never changes once published). Such a module holds plain data, constructs no SQL and owns no session; the composition code creates one instance per process and injects it into the service. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index c425fa6e9..21cd92418 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -86,7 +86,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it; a file that can't be renamed needs a [[rule.exception]] here, which gives it its directory's strictest checks. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | diff --git a/policyengine_api/asgi_factory.py b/policyengine_api/asgi_factory.py index 415fd09bb..0d6adf0f0 100644 --- a/policyengine_api/asgi_factory.py +++ b/policyengine_api/asgi_factory.py @@ -21,7 +21,7 @@ from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies from policyengine_api.fastapi_routes.health import build_core_health_router from policyengine_api.fastapi_routes.metadata import build_metadata_router -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2AuthenticationRequiredError, V2AuthenticationUnavailableError, V2PermissionDeniedError, diff --git a/policyengine_api/fastapi_routes/dependencies.py b/policyengine_api/fastapi_routes/dependencies.py index 6e2927f83..25f8664cb 100644 --- a/policyengine_api/fastapi_routes/dependencies.py +++ b/policyengine_api/fastapi_routes/dependencies.py @@ -24,13 +24,15 @@ V2UserReportResourceService, V2UserSimulationResourceService, ) -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) -from policyengine_api.fastapi_routes.v2.user_auth import authenticate_account_request +from policyengine_api.fastapi_routes.v2.user_auth_support import ( + authenticate_account_request, +) from policyengine_api.libs.user_auth import UNAUTHENTICATED_USER_HEADER from policyengine_api.readiness import is_ready -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/policyengine_api/fastapi_routes/v2/countries/response_models.py b/policyengine_api/fastapi_routes/v2/countries/response_models.py index 3ea3c7ee8..4d2abe90f 100644 --- a/policyengine_api/fastapi_routes/v2/countries/response_models.py +++ b/policyengine_api/fastapi_routes/v2/countries/response_models.py @@ -6,7 +6,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse if TYPE_CHECKING: from policyengine_api.services.v2.countries.types import CountryRead diff --git a/policyengine_api/fastapi_routes/v2/countries/routes.py b/policyengine_api/fastapi_routes/v2/countries/routes.py index 279aaf54f..1f15c3f70 100644 --- a/policyengine_api/fastapi_routes/v2/countries/routes.py +++ b/policyengine_api/fastapi_routes/v2/countries/routes.py @@ -19,7 +19,10 @@ CountryListResponse, CountryListResult, ) -from policyengine_api.fastapi_routes.v2.errors import V2ErrorMapping, run_v2_operation +from policyengine_api.fastapi_routes.v2.errors_support import ( + V2ErrorMapping, + run_v2_operation, +) from policyengine_api.query_parameters import CountryListQuery if TYPE_CHECKING: diff --git a/policyengine_api/fastapi_routes/v2/errors.py b/policyengine_api/fastapi_routes/v2/errors_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/errors.py rename to policyengine_api/fastapi_routes/v2/errors_support.py diff --git a/policyengine_api/fastapi_routes/v2/households/response_models.py b/policyengine_api/fastapi_routes/v2/households/response_models.py index 35c645c9c..f778c7caa 100644 --- a/policyengine_api/fastapi_routes/v2/households/response_models.py +++ b/policyengine_api/fastapi_routes/v2/households/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.households.document_models import ( HouseholdDocument, ) diff --git a/policyengine_api/fastapi_routes/v2/households/routes.py b/policyengine_api/fastapi_routes/v2/households/routes.py index 54e24dbf5..52b773e95 100644 --- a/policyengine_api/fastapi_routes/v2/households/routes.py +++ b/policyengine_api/fastapi_routes/v2/households/routes.py @@ -14,7 +14,7 @@ _default_v2_household_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorMapping, V2RequestTooLargeError, run_v2_operation, diff --git a/policyengine_api/fastapi_routes/v2/me/routes.py b/policyengine_api/fastapi_routes/v2/me/routes.py index 0919c938c..81c517fc7 100644 --- a/policyengine_api/fastapi_routes/v2/me/routes.py +++ b/policyengine_api/fastapi_routes/v2/me/routes.py @@ -20,7 +20,7 @@ _default_v2_account_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.me.request_models import ( AccountPatchRequest, ) @@ -33,8 +33,10 @@ MeUpgradeResponse, MeUpgradeResult, ) -from policyengine_api.fastapi_routes.v2.user_auth import require_signed_in_principal -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_auth_support import ( + require_signed_in_principal, +) +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py b/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py index 968c802c4..5490ea28a 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py @@ -7,13 +7,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse, Response -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - CacheableMetadataRoute, - read_resource, - read_whole_version_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogParameterQueryDependency, CatalogQueryDependency, ) @@ -23,6 +17,12 @@ MetadataModelCatalogResponse, MetadataResourceSuccessResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + CacheableMetadataRoute, + read_resource, + read_whole_version_resource, +) from policyengine_api.services.v2.metadata.types import ( MetadataCatalogVariablesResult, MetadataModelCatalogResult, diff --git a/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py b/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py index 448826c5d..bffe931cd 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py @@ -8,12 +8,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - CacheableMetadataRoute, - read_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogPageQueryDependency, CatalogQueryDependency, RegionCollectionQueryDependency, @@ -25,6 +20,11 @@ MetadataRegionDetailResponse, MetadataRegionPageResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + CacheableMetadataRoute, + read_resource, +) if TYPE_CHECKING: from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies diff --git a/policyengine_api/fastapi_routes/v2/metadata/model_routes.py b/policyengine_api/fastapi_routes/v2/metadata/model_routes.py index ae3127725..7fbbe530f 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/model_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/model_routes.py @@ -8,13 +8,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - PATH_COUNTRY_ERROR_RESPONSES, - CacheableMetadataRoute, - read_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogPageQueryDependency, CatalogQueryDependency, CatalogVersionQueryDependency, @@ -29,6 +23,12 @@ MetadataVariableDetailResponse, MetadataVariablePageResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + PATH_COUNTRY_ERROR_RESPONSES, + CacheableMetadataRoute, + read_resource, +) if TYPE_CHECKING: from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index 11471f41c..040ff8f3c 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -8,17 +8,7 @@ from fastapi import APIRouter, Request from starlette.responses import JSONResponse, Response -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - PARAMETER_SEARCH_FILE_ERROR_RESPONSES, - PARAMETER_TREE_ERROR_RESPONSES, - CacheableMetadataRoute, - names_entity_tag, - read_resource, - run_metadata_read, - take_utc_instant_for_read, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogQueryDependency, ParameterChildrenQueryDependency, ParameterCollectionQueryDependency, @@ -32,7 +22,17 @@ MetadataParameterValueDetailResponse, MetadataParameterValuePageResponse, ) -from policyengine_api.fastapi_routes.v2.metadata.search_file_response import ( +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + PARAMETER_SEARCH_FILE_ERROR_RESPONSES, + PARAMETER_TREE_ERROR_RESPONSES, + CacheableMetadataRoute, + names_entity_tag, + read_resource, + run_metadata_read, + take_utc_instant_for_read, +) +from policyengine_api.fastapi_routes.v2.metadata.search_file_support import ( SEARCH_FILE_RESPONSE_HEADERS, parameter_search_file_response, search_file_entity_tag, diff --git a/policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py b/policyengine_api/fastapi_routes/v2/metadata/query_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py rename to policyengine_api/fastapi_routes/v2/metadata/query_support.py diff --git a/policyengine_api/fastapi_routes/v2/metadata/response_cache.py b/policyengine_api/fastapi_routes/v2/metadata/response_cache_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/metadata/response_cache.py rename to policyengine_api/fastapi_routes/v2/metadata/response_cache_support.py diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/route_support.py similarity index 99% rename from policyengine_api/fastapi_routes/v2/metadata/common.py rename to policyengine_api/fastapi_routes/v2/metadata/route_support.py index 185fbe4d8..d8e99930f 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/route_support.py @@ -18,11 +18,11 @@ UnsupportedPreviewCountryError, ) from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorResponse, v2_error_response, ) -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( BuiltResponse, KeptResponse, body_entity_tag, diff --git a/policyengine_api/fastapi_routes/v2/metadata/search_file_response.py b/policyengine_api/fastapi_routes/v2/metadata/search_file_support.py similarity index 97% rename from policyengine_api/fastapi_routes/v2/metadata/search_file_response.py rename to policyengine_api/fastapi_routes/v2/metadata/search_file_support.py index 3a2b5b1b0..be224e72d 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/search_file_response.py +++ b/policyengine_api/fastapi_routes/v2/metadata/search_file_support.py @@ -17,7 +17,7 @@ from starlette.responses import Response from policyengine_api.content_encoding import accepts_gzip -from policyengine_api.fastapi_routes.v2.metadata.common import ( +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( CACHE_HEADER_DESCRIPTIONS, ) diff --git a/policyengine_api/fastapi_routes/v2/policies/response_models.py b/policyengine_api/fastapi_routes/v2/policies/response_models.py index 771aaa12d..a73c24ce8 100644 --- a/policyengine_api/fastapi_routes/v2/policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/policies/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict, Field, JsonValue -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse if TYPE_CHECKING: from policyengine_api.services.v2.policies.types import ( diff --git a/policyengine_api/fastapi_routes/v2/policies/routes.py b/policyengine_api/fastapi_routes/v2/policies/routes.py index e65e3e5bc..8b07feeaa 100644 --- a/policyengine_api/fastapi_routes/v2/policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/policies/routes.py @@ -18,7 +18,7 @@ _default_v2_policy_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorMapping, V2RequestTooLargeError, run_v2_operation, diff --git a/policyengine_api/fastapi_routes/v2/routes.py b/policyengine_api/fastapi_routes/v2/routes.py index d3b412252..2309feb46 100644 --- a/policyengine_api/fastapi_routes/v2/routes.py +++ b/policyengine_api/fastapi_routes/v2/routes.py @@ -10,7 +10,7 @@ from policyengine_api.fastapi_routes.v2.countries.routes import ( build_v2_country_router, ) -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.households.routes import ( build_v2_household_router, ) diff --git a/policyengine_api/fastapi_routes/v2/user_auth.py b/policyengine_api/fastapi_routes/v2/user_auth_support.py similarity index 98% rename from policyengine_api/fastapi_routes/v2/user_auth.py rename to policyengine_api/fastapi_routes/v2/user_auth_support.py index aeeb5e46f..617f43703 100644 --- a/policyengine_api/fastapi_routes/v2/user_auth.py +++ b/policyengine_api/fastapi_routes/v2/user_auth_support.py @@ -21,7 +21,7 @@ from __future__ import annotations -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2AuthenticationRequiredError, V2AuthenticationUnavailableError, V2PermissionDeniedError, diff --git a/policyengine_api/fastapi_routes/v2/user_households/response_models.py b/policyengine_api/fastapi_routes/v2/user_households/response_models.py index df4137499..eeb480fe3 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/response_models.py +++ b/policyengine_api/fastapi_routes/v2/user_households/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, ) from policyengine_api.query_parameters import CountryId diff --git a/policyengine_api/fastapi_routes/v2/user_households/routes.py b/policyengine_api/fastapi_routes/v2/user_households/routes.py index 4e86c9b86..43841b9b9 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_households/routes.py @@ -13,7 +13,7 @@ _default_v2_user_household_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response from policyengine_api.fastapi_routes.v2.user_households.request_models import ( UserHouseholdCreateRequest, UserHouseholdPatchRequest, @@ -26,7 +26,7 @@ UserHouseholdPageResponse, UserHouseholdPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( enforce_user_write_request_size, user_resource_operation, ) diff --git a/policyengine_api/fastapi_routes/v2/user_policies/response_models.py b/policyengine_api/fastapi_routes/v2/user_policies/response_models.py index 7886c40c7..08912e1d7 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, ) from policyengine_api.query_parameters import CountryId diff --git a/policyengine_api/fastapi_routes/v2/user_policies/routes.py b/policyengine_api/fastapi_routes/v2/user_policies/routes.py index 6ff192fee..0f55c1fa4 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -13,7 +13,7 @@ _default_v2_user_policy_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response from policyengine_api.fastapi_routes.v2.user_policies.request_models import ( UserPolicyCreateRequest, UserPolicyPatchRequest, @@ -26,7 +26,7 @@ UserPolicyPageResponse, UserPolicyPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( enforce_user_write_request_size, user_resource_operation, ) diff --git a/policyengine_api/fastapi_routes/v2/user_reports/routes.py b/policyengine_api/fastapi_routes/v2/user_reports/routes.py index 241f19882..44b7f607b 100644 --- a/policyengine_api/fastapi_routes/v2/user_reports/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_reports/routes.py @@ -17,7 +17,7 @@ _default_v2_user_report_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorResponse, v2_error_response, ) @@ -35,7 +35,7 @@ UserReportPageResponse, UserReportPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/fastapi_routes/v2/user_resources.py b/policyengine_api/fastapi_routes/v2/user_resources_support.py similarity index 99% rename from policyengine_api/fastapi_routes/v2/user_resources.py rename to policyengine_api/fastapi_routes/v2/user_resources_support.py index 581065a57..b71f76bb2 100644 --- a/policyengine_api/fastapi_routes/v2/user_resources.py +++ b/policyengine_api/fastapi_routes/v2/user_resources_support.py @@ -13,7 +13,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( BEARER_CHALLENGE, V2ErrorResponse, V2RequestTooLargeError, diff --git a/policyengine_api/fastapi_routes/v2/user_simulations/routes.py b/policyengine_api/fastapi_routes/v2/user_simulations/routes.py index 0cac726a4..fbb98c51d 100644 --- a/policyengine_api/fastapi_routes/v2/user_simulations/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_simulations/routes.py @@ -13,8 +13,8 @@ _default_v2_user_simulation_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/services/v2/metadata/selected_catalogs.py b/policyengine_api/services/v2/metadata/process_state.py similarity index 100% rename from policyengine_api/services/v2/metadata/selected_catalogs.py rename to policyengine_api/services/v2/metadata/process_state.py diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 8938f42c6..b8e32a624 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -107,7 +107,7 @@ from policyengine_api.services.v2.metadata.database_session import ( MetadataDatabaseSession, ) - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/rules/baselines/v2-layers-routes.txt b/rules/baselines/v2-layers-routes.txt index a7684b874..43fb5fa5f 100644 --- a/rules/baselines/v2-layers-routes.txt +++ b/rules/baselines/v2-layers-routes.txt @@ -5,4 +5,4 @@ policyengine_api/fastapi_routes/v2/countries/routes.py imports sqlalchemy.exc policyengine_api/fastapi_routes/v2/households/routes.py imports sqlalchemy.exc policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlalchemy.exc -policyengine_api/fastapi_routes/v2/user_resources.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/user_resources_support.py imports sqlalchemy.exc diff --git a/rules/registry.toml b/rules/registry.toml index 3ddccd0c6..ee6ce67fc 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -183,7 +183,7 @@ invocation = "make test" [[rule]] id = "api.v2.files-assigned-to-layers" category = "v2-layers" -statement = "Every .py file under services/v2 and fastapi_routes/v2 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it." +statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it; a file that can't be renamed needs a [[rule.exception]] here, which gives it its directory's strictest checks." rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" @@ -281,7 +281,7 @@ id = "api.v2.pure-modules-no-session-calls" category = "v2-layers" statement = "v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded." rationale = "docs/engineering/skills/v2-code-organization.md#validatorspy" -scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/metadata/selected_catalogs.py", "policyengine_api/services/v2/__init__.py", "policyengine_api/services/v2/*/__init__.py"] +scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/*/process_state.py", "policyengine_api/services/v2/__init__.py", "policyengine_api/services/v2/*/__init__.py"] tier = "mechanical" status = "enforced" autofix = false diff --git a/rules/v2-layer-manifest.toml b/rules/v2-layer-manifest.toml deleted file mode 100644 index 985ffcdca..000000000 --- a/rules/v2-layer-manifest.toml +++ /dev/null @@ -1,50 +0,0 @@ -# Layers for the API v2 files whose names don't give one. -# -# The v2-layers quality guard (scripts/rules/v2_layers.py) assigns every .py -# file under policyengine_api/services/v2 and policyengine_api/fastapi_routes/v2 -# a layer by its file name: services.py, validators.py, transformations.py, -# types.py, database_session.py, database_connectors/[_name].py, -# routes.py, _routes.py, request_models.py, response_models.py or -# __init__.py. A file with another name needs an entry here, or the guard fails -# (rule api.v2.files-assigned-to-layers). Every file under fastapi_routes/v2 -# follows rule api.v2.routes-no-sessions-or-sql whatever its layer. -# -# Layers: http-model (request or response schemas), http-support (shared HTTP -# code for route modules) and process-state (data a service keeps for the -# process, docs/engineering/skills/v2-code-organization.md#resource-service-packages). - -[files."policyengine_api/fastapi_routes/v2/errors.py"] -layer = "http-support" -reason = "The error response and serialization every v2 route shares." - -[files."policyengine_api/fastapi_routes/v2/households/document_models.py"] -layer = "http-model" -reason = "Household document schemas shared by the request and response models." - -[files."policyengine_api/fastapi_routes/v2/metadata/common.py"] -layer = "http-support" -reason = "Runs metadata reads for the route modules and maps their failures to HTTP, and the route class that sets caching headers." - -[files."policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py"] -layer = "http-support" -reason = "Typed FastAPI dependencies for the shared metadata query models." - -[files."policyengine_api/fastapi_routes/v2/metadata/response_cache.py"] -layer = "http-support" -reason = "The process-wide store of serialized whole-version responses and their entity tags." - -[files."policyengine_api/fastapi_routes/v2/metadata/search_file_response.py"] -layer = "http-support" -reason = "The HTTP representation of a stored parameter search file, with content encoding." - -[files."policyengine_api/fastapi_routes/v2/user_auth.py"] -layer = "http-support" -reason = "Resolves request credentials to an account principal for user-owned routes." - -[files."policyengine_api/fastapi_routes/v2/user_resources.py"] -layer = "http-support" -reason = "HTTP conventions shared by the user-owned resource routes." - -[files."policyengine_api/services/v2/metadata/selected_catalogs.py"] -layer = "process-state" -reason = "The catalog versions metadata reads have selected, kept for the process." diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 7593a4817..274c58240 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -1,10 +1,13 @@ """The v2-layers quality guard: every v2 route and service file has a layer. Every ``.py`` file under ``policyengine_api/services/v2`` and -``policyengine_api/fastapi_routes/v2`` gets a layer from its file name (see -``assign_layer``), or from ``rules/v2-layer-manifest.toml`` for the few files -with other names. A file with neither fails (rule +``policyengine_api/fastapi_routes/v2`` gets a layer from its file name alone +(see ``assign_layer``). A file whose name gives none fails (rule ``api.v2.files-assigned-to-layers``), so a new file is never left unchecked. +A file that can't be renamed needs a ``[[rule.exception]]`` for that rule in +rules/registry.toml. The exception gives it no layer: it gets its directory's +strictest checks, the route checks under ``fastapi_routes/v2`` and the +pure-module checks under ``services/v2``. Then each file's imports and calls are checked against its layer's rule: @@ -57,7 +60,6 @@ from __future__ import annotations import ast -import tomllib from collections import Counter from dataclasses import dataclass from pathlib import Path, PurePosixPath @@ -68,11 +70,13 @@ from scripts.rules.model import Violation if TYPE_CHECKING: - from collections.abc import Callable, Iterable, Mapping + from collections.abc import Callable, Collection, Iterable, Mapping + + from scripts.rules.model import Rule REPO_ROOT = Path(__file__).resolve().parents[2] CHECK_NAME = "v2-layers" -MANIFEST_PATH = "rules/v2-layer-manifest.toml" +REGISTRY_FILE = "rules/registry.toml" SERVICES_DIRECTORY = "policyengine_api/services/v2" ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" ORGANIZATION_DOC = "docs/engineering/skills/v2-code-organization.md" @@ -97,21 +101,17 @@ "validators.py": "validator", "transformations.py": "transformation", "types.py": "types", + "process_state.py": "process-state", "database_session.py": "session", } CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") -HTTP_MODEL_FILES = frozenset({"request_models.py", "response_models.py"}) -# Layers the manifest may give, by directory. Connectors, session modules and -# packages get their layer from their names only. -MANIFEST_LAYERS = { - SERVICES_DIRECTORY: frozenset( - {"process-state", "service", "transformation", "types", "validator"} - ), - ROUTES_DIRECTORY: frozenset({"http-model", "http-support", "route"}), -} +# What a [[rule.exception]] for the files rule makes a file: no layer of its +# own, and its directory's strictest checks. +EXCEPTED = "excepted" PURE_LAYERS = frozenset( - {"package", "process-state", "transformation", "types", "validator"} + {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} ) +GLOB_CHARACTERS = frozenset("*?[") SQL_PACKAGES = ("sqlalchemy", "sqlmodel") DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") @@ -174,10 +174,11 @@ FIXES = { FILES_RULE: ( "name it after its layer (services.py, validators.py, transformations.py, " - "types.py, database_session.py, " + "types.py, process_state.py, database_session.py, " "database_connectors/[_name].py, routes.py, " - "_routes.py, request_models.py or response_models.py), or add it to " - f"{MANIFEST_PATH} with its layer and a reason" + "_routes.py, _models.py or _support.py), or, if it " + f"can't be renamed, add a [[rule.exception]] for {FILES_RULE} to " + f"{REGISTRY_FILE} with its path and a reason" ), ROUTES_RULE: ( "v2 route modules call one service method and map its typed failures to " @@ -237,10 +238,14 @@ def _module_name(path: str) -> tuple[str, bool]: # --- layers --------------------------------------------------------------- -def assign_layer(path: str, manifest: Mapping[str, str]) -> str | None: - """Return the layer of the file at ``path`` (repository-relative), or None.""" - if path in manifest: - return manifest[path] +def assign_layer(path: str, exceptions: Collection[str] = ()) -> str | None: + """Return the layer of the file at ``path`` (repository-relative), or None. + + A path in ``exceptions`` is ``excepted``: it gets its directory's + strictest checks. + """ + if path in exceptions: + return EXCEPTED return _layer_by_name(path) @@ -280,7 +285,9 @@ def _route_layer(name: str) -> str | None: return "package" if name == "routes.py" or name.endswith("_routes.py"): return "route" - return "http-model" if name in HTTP_MODEL_FILES else None + if name.endswith("_models.py"): + return "http-model" + return "http-support" if name.endswith("_support.py") else None # --- imports -------------------------------------------------------------- @@ -623,12 +630,12 @@ def _discovered_files(root: Path) -> list[str]: def scan( - root: Path, manifest: Mapping[str, str], paths: Iterable[str] | None = None + root: Path, exceptions: Collection[str] = (), paths: Iterable[str] | None = None ) -> list[Finding]: """Return every finding for the v2 files under ``root``, one per occurrence.""" findings = [] for path in paths if paths is not None else _discovered_files(root): - layer = assign_layer(path, manifest) + layer = assign_layer(path, exceptions) if layer is None: findings.append(Finding(FILES_RULE, path, "matches no v2 layer")) continue @@ -640,71 +647,47 @@ def scan( return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) -def _manifest_violation(message: str, fix: str) -> Violation: - return Violation(FILES_RULE, CHECK_NAME, message, fix) +def _exception_violations(root: Path, path: str) -> list[Violation]: + """Report a files-rule exception that names no unnamed v2 file.""" + def violation(problem: str, fix: str) -> Violation: + return Violation( + FILES_RULE, + CHECK_NAME, + f"{REGISTRY_FILE} excepts {path} from {FILES_RULE}, {problem}", + fix, + ) -def _manifest_entry_violations(root: Path, path: str, entry: object) -> list[Violation]: - layer = entry.get("layer") if isinstance(entry, dict) else None - reason = entry.get("reason") if isinstance(entry, dict) else None - directory = next( - (folder for folder in MANIFEST_LAYERS if path.startswith(f"{folder}/")), None - ) - if directory is None: + in_v2 = path.startswith((f"{SERVICES_DIRECTORY}/", f"{ROUTES_DIRECTORY}/")) + if not in_v2 or GLOB_CHARACTERS & set(path) or not path.endswith(".py"): return [ - _manifest_violation( - f"{MANIFEST_PATH} lists {path}, which is not under " - f"{SERVICES_DIRECTORY} or {ROUTES_DIRECTORY}", - "remove the entry", + violation( + f"which is not one file under {SERVICES_DIRECTORY} or " + f"{ROUTES_DIRECTORY}", + "name one file there, or remove the exception", ) ] - violations = [] if not (root / path).is_file(): - violations.append( - _manifest_violation( - f"{MANIFEST_PATH} lists {path}, which does not exist", - "remove the entry", - ) - ) - elif _layer_by_name(path) is not None: - violations.append( - _manifest_violation( - f"{MANIFEST_PATH} lists {path}, which its name already assigns to " - "a layer", - "remove the entry", - ) - ) - if layer not in MANIFEST_LAYERS[directory]: - violations.append( - _manifest_violation( - f"{MANIFEST_PATH} gives {path} the layer '{layer}', which a file " - f"under {directory} can't have from the manifest", - f"use one of {', '.join(sorted(MANIFEST_LAYERS[directory]))}", - ) - ) - if not isinstance(reason, str) or not reason.strip(): - violations.append( - _manifest_violation( - f"{MANIFEST_PATH} gives {path} no reason", - "say why the file has that layer", - ) - ) - return violations + return [violation("but it does not exist", "remove the exception")] + if _layer_by_name(path) is not None: + return [ + violation("but its name already gives it a layer", "remove the exception") + ] + return [] -def load_manifest(root: Path) -> tuple[dict[str, str], list[Violation]]: - """Read the manifest's file layers, and report entries that don't belong.""" - manifest_file = root / MANIFEST_PATH - if not manifest_file.is_file(): - return {}, [] - entries = tomllib.loads(manifest_file.read_text()).get("files", {}) - manifest = {} +def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Violation]]: + """Return the files the files rule excepts, and its exceptions that don't belong.""" + if rule is None: + return [], [] + excepted = [] violations = [] - for path, entry in sorted(entries.items()): - layer = entry.get("layer") if isinstance(entry, dict) else None - manifest[path] = str(layer) - violations.extend(_manifest_entry_violations(root, path, entry)) - return manifest, violations + for exception in rule.exceptions: + problems = _exception_violations(root, exception.path) + violations += problems + if not problems: + excepted.append(exception.path) + return excepted, violations def _baseline_entries(root: Path, baseline_file: str | None) -> Counter[str]: @@ -764,8 +747,8 @@ def run_check(root: Path = REPO_ROOT) -> list[Violation]: registry, violations = load_registry(root) if registry is None: return violations - manifest, violations = load_manifest(root) - findings = scan(root, manifest) + excepted, violations = file_exceptions(root, registry.rule(FILES_RULE)) + findings = scan(root, excepted) for rule_id in RULES: rule = registry.rule(rule_id) baseline_file = rule.baseline.file if rule and rule.baseline else None diff --git a/tests/unit/v2/test_metadata_route_caching.py b/tests/unit/v2/test_metadata_route_caching.py index 0115d0a49..c4c8e77d0 100644 --- a/tests/unit/v2/test_metadata_route_caching.py +++ b/tests/unit/v2/test_metadata_route_caching.py @@ -207,8 +207,8 @@ def test_a_kept_response_is_served_without_hashing_its_body_again( monkeypatch: pytest.MonkeyPatch, ) -> None: from policyengine_api.fastapi_routes.v2.metadata import ( - route_support, response_cache_support, + route_support, ) hashed_bodies: list[int] = [] From 17807831fe3785b3a4fa07080da5c6ffd72bc88b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:51:41 +0400 Subject: [PATCH 12/15] Test that proofs name each finding's rule and exceptions load cleanly (failing) - The proofs compare ": ", so a finding reported under the wrong rule fails the rule's own proof. - The exception tests require their registry to load without problems; an exception needs an expires date or a removal condition as well as a path and a reason, which the fix message must say. - An unassigned file is told only the names that give a layer in its directory. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_v2_layers.py | 37 +++++++++++++++++++++------ 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 6e1d9b386..da22bc978 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -8,6 +8,7 @@ import unittest from pathlib import Path +from scripts.rules.load import load_registry from scripts.rules.v2_layers import ( CONNECTORS_RULE, FILES_RULE, @@ -45,14 +46,15 @@ def _findings(files: dict[str, str], rule_id: str) -> set[str]: def _all_findings(files: dict[str, str], exceptions: tuple[str, ...]) -> set[str]: + """Return every finding as ``: ``.""" with tempfile.TemporaryDirectory() as directory: root = Path(directory) _write_files(files, root) - return {str(finding) for finding in scan(root, exceptions)} + return {f"{finding.rule_id}: {finding}" for finding in scan(root, exceptions)} def _assert_proof(test: unittest.TestCase, rule_id: str) -> None: - """Each tree gives exactly its expected findings, of any rule. + """Each tree gives exactly its expected findings, each under its rule. A tree's ``exceptions`` list the files a ``[[rule.exception]]`` for api.v2.files-assigned-to-layers would name. @@ -617,6 +619,8 @@ def _registry( return header + "".join(rules) def _messages(self) -> list[str]: + _, problems = load_registry(self.root) + self.assertEqual([str(problem) for problem in problems], []) return [str(violation) for violation in run_check(self.root)] def test_a_finding_the_baseline_lists_passes(self) -> None: @@ -723,12 +727,29 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " "name it after its layer (services.py, validators.py, " "transformations.py, types.py, process_state.py, " - "database_session.py, " - "database_connectors/[_name].py, " - "routes.py, _routes.py, _models.py or " - "_support.py), or, if it can't be renamed, add a " - "[[rule.exception]] for api.v2.files-assigned-to-layers to " - "rules/registry.toml with its path and a reason." + "database_session.py or " + "database_connectors/[_name].py), " + "or, if it can't be renamed, add a [[rule.exception]] for " + "api.v2.files-assigned-to-layers to rules/registry.toml with its " + "path, a reason and an expires date or removal condition." + ], + ) + + def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: + unassigned = "policyengine_api/fastapi_routes/v2/metadata/helpers.py" + _write_files( + {unassigned: "", "rules/registry.toml": self._registry(None)}, self.root + ) + + self.assertEqual( + self._messages(), + [ + f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " + "name it after its layer (routes.py, _routes.py, " + "_models.py or _support.py), or, if it can't be " + "renamed, add a [[rule.exception]] for " + "api.v2.files-assigned-to-layers to rules/registry.toml with its " + "path, a reason and an expires date or removal condition." ], ) From 8b441b8ab8ab74b1465204c0373ff0964c747e5c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:52:21 +0400 Subject: [PATCH 13/15] Name each proof finding's rule and complete the exception guidance - Each bad proof tree lists its findings as ": ", so a finding reported under the wrong rule fails the proof. The files rule's tree shows its excepted files' findings under the route and pure-module rules. - The files rule's fix message names only the layers of the file's directory, and says an exception needs an expires date or removal condition as well as a path and a reason; the test helper's exceptions now carry one and load cleanly. - The registry statements say which checks an excepted file gets, and the pure-module rule's covers excepted files under services/v2. - The Import Linter routes fixture uses route_support.py instead of the old common.py name. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- .../skills/v2-code-organization.md | 7 ++-- docs/generated/rules.md | 4 +-- .../bad_tree.toml | 7 ++-- .../bad_tree.toml | 7 ++-- .../bad_tree.toml | 11 ++++--- .../bad_package.toml | 2 +- .../bad_tree.toml | 9 +++--- .../api.v2.services-no-sql/bad_tree.toml | 9 +++--- .../bad_tree.toml | 9 +++--- rules/registry.toml | 4 +-- scripts/rules/tests/test_v2_layers.py | 1 + scripts/rules/v2_layers.py | 32 +++++++++++++++---- 12 files changed, 64 insertions(+), 38 deletions(-) diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index d5bbbc190..ddf773971 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -24,9 +24,10 @@ guard (`scripts/rules/v2_layers.py`) takes from the file's name alone: A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its -path and a reason. An exception chooses no layer: the file gets its -directory's strictest checks, the route checks under `fastapi_routes/v2` and -the validator, transformation and types checks under `services/v2`. The guard +`path`, a `reason`, and an `expires` date or a `removal` condition. An +exception chooses no layer: the file gets the route checks under +`fastapi_routes/v2`, and the validator, transformation and types checks +(rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already gives it a layer. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 21cd92418..5f2aedd53 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -86,10 +86,10 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it; a file that can't be renamed needs a [[rule.exception]] here, which gives it its directory's strictest checks. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | -| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | diff --git a/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml index caed48788..a85a5145c 100644 --- a/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml +++ b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml @@ -1,10 +1,11 @@ # Proof fixture for rule api.v2.connectors-never-commit. The guard must reject # it: the connector opens its own session and commits it. +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/services/v2/policies/database_connectors/creates.py calls get_v2_session_factory", - "policyengine_api/services/v2/policies/database_connectors/creates.py calls session.commit", - "policyengine_api/services/v2/policies/database_connectors/creates.py imports policyengine_api.data.v2.database", + "api.v2.connectors-never-commit: policyengine_api/services/v2/policies/database_connectors/creates.py calls get_v2_session_factory", + "api.v2.connectors-never-commit: policyengine_api/services/v2/policies/database_connectors/creates.py calls session.commit", + "api.v2.connectors-never-commit: policyengine_api/services/v2/policies/database_connectors/creates.py imports policyengine_api.data.v2.database", ] [files] diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml index 52205b84c..0660d58c4 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -13,10 +13,11 @@ exceptions = [ "policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py", "policyengine_api/services/v2/household_calculations/legacy_client.py", ] +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", - "policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", - "policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", + "api.v2.files-assigned-to-layers: policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", ] [files] diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml index f0ebcdcf5..dd50effcd 100644 --- a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -4,12 +4,13 @@ # scripts/rules/tests/test_v2_layers.py writes each file into a temporary # directory and runs the v2-layers guard's scan on it, with no manifest. +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/services/v2/policies/__init__.py calls session.commit", - "policyengine_api/services/v2/policies/types.py calls session.exec", - "policyengine_api/services/v2/policies/types.py calls sqlmodel.select", - "policyengine_api/services/v2/policies/types.py imports sqlmodel.select", - "policyengine_api/services/v2/policies/validators.py calls session.get", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/__init__.py calls session.commit", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py calls session.exec", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py calls sqlmodel.select", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py imports sqlmodel.select", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/validators.py calls session.get", ] [files] diff --git a/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml b/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml index d77345733..e4d0b82ee 100644 --- a/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml +++ b/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml @@ -16,7 +16,7 @@ from fixture_api.services.v2.policies.database_connectors.reads import read_poli "fixture_api/fastapi_routes/v2/households/routes.py" = """ from fixture_api.services.v2.households.database_session import HouseholdDatabaseSession """ -"fixture_api/fastapi_routes/v2/metadata/common.py" = """ +"fixture_api/fastapi_routes/v2/metadata/route_support.py" = """ from fixture_api.data.v2.database import get_v2_session_factory """ "fixture_api/services/v2/policies/database_connectors/reads.py" = """ diff --git a/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml index 4d6b3fe08..fc78accf3 100644 --- a/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml @@ -2,11 +2,12 @@ # reject it: a route module imports a connector and SQLModel, builds a select # and calls a method on a session. +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/fastapi_routes/v2/policies/routes.py calls session.add", - "policyengine_api/fastapi_routes/v2/policies/routes.py calls sqlmodel.select", - "policyengine_api/fastapi_routes/v2/policies/routes.py imports policyengine_api.services.v2.policies.database_connectors.reads", - "policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlmodel", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py calls session.add", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py calls sqlmodel.select", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py imports policyengine_api.services.v2.policies.database_connectors.reads", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlmodel", ] [files] diff --git a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml index 4bd955ed0..79cc14e4d 100644 --- a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml +++ b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml @@ -2,11 +2,12 @@ # service imports and builds a select, runs it with Session.exec and defines # an HTTP response with FastAPI. +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/services/v2/policies/services.py calls _session.exec", - "policyengine_api/services/v2/policies/services.py calls sqlmodel.select", - "policyengine_api/services/v2/policies/services.py imports fastapi.responses", - "policyengine_api/services/v2/policies/services.py imports sqlmodel.select", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py calls _session.exec", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py calls sqlmodel.select", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py imports fastapi.responses", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py imports sqlmodel.select", ] [files] diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml index 882e00f99..d4cd9b884 100644 --- a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml +++ b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml @@ -4,11 +4,12 @@ # scripts/rules/tests/test_v2_layers.py writes each file into a temporary # directory and runs the v2-layers guard's scan on it, with no manifest. +# Each expected finding is ": ". expected_findings = [ - "policyengine_api/services/v2/policies/database_session.py calls session.add", - "policyengine_api/services/v2/policies/database_session.py calls session.execute", - "policyengine_api/services/v2/policies/database_session.py calls session.get", - "policyengine_api/services/v2/policies/database_session.py calls sqlalchemy.text", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.add", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.execute", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.get", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls sqlalchemy.text", ] [files] diff --git a/rules/registry.toml b/rules/registry.toml index ee6ce67fc..d7c071449 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -183,7 +183,7 @@ invocation = "make test" [[rule]] id = "api.v2.files-assigned-to-layers" category = "v2-layers" -statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it; a file that can't be renamed needs a [[rule.exception]] here, which gives it its directory's strictest checks." +statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" @@ -279,7 +279,7 @@ test = "tests/unit/v2/test_data_crud_boundaries.py::test_rule_proof_user_service [[rule]] id = "api.v2.pure-modules-no-session-calls" category = "v2-layers" -statement = "v2 validators, transformations, types, process-state modules and package __init__ files import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded." +statement = "v2 validators, transformations, types, process-state modules, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded." rationale = "docs/engineering/skills/v2-code-organization.md#validatorspy" scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/*/process_state.py", "policyengine_api/services/v2/__init__.py", "policyengine_api/services/v2/*/__init__.py"] tier = "mechanical" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index da22bc978..db9c34919 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -586,6 +586,7 @@ def _registry( ) excepted = "".join( f'[[rule.exception]]\npath = "{path}"\nreason = "Generated."\n' + "expires = 2099-01-01\n" for path in (exceptions if rule_id == FILES_RULE else ()) ) rules.append( diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 274c58240..df64a7dc3 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -105,6 +105,17 @@ "database_session.py": "session", } CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") +# The names that give a layer, by directory, for the files rule's fix message. +LAYER_NAMES = { + SERVICES_DIRECTORY: ( + "services.py, validators.py, transformations.py, types.py, " + "process_state.py, database_session.py or " + "database_connectors/[_name].py" + ), + ROUTES_DIRECTORY: ( + "routes.py, _routes.py, _models.py or _support.py" + ), +} # What a [[rule.exception]] for the files rule makes a file: no layer of its # own, and its directory's strictest checks. EXCEPTED = "excepted" @@ -173,12 +184,9 @@ FIXES = { FILES_RULE: ( - "name it after its layer (services.py, validators.py, transformations.py, " - "types.py, process_state.py, database_session.py, " - "database_connectors/[_name].py, routes.py, " - "_routes.py, _models.py or _support.py), or, if it " - f"can't be renamed, add a [[rule.exception]] for {FILES_RULE} to " - f"{REGISTRY_FILE} with its path and a reason" + "name it after its layer ({names}), or, if it can't be renamed, add a " + f"[[rule.exception]] for {FILES_RULE} to {REGISTRY_FILE} with its path, " + "a reason and an expires date or removal condition" ), ROUTES_RULE: ( "v2 route modules call one service method and map its typed failures to " @@ -704,6 +712,16 @@ def _occurrences(count: int) -> str: return f"{count} occurrence" if count == 1 else f"{count} occurrences" +def _fix(rule_id: str, entry: str) -> str: + """Return the rule's fix; the files rule's names the layers of its directory.""" + if rule_id != FILES_RULE: + return FIXES[rule_id] + directory = ( + SERVICES_DIRECTORY if entry.startswith(SERVICES_DIRECTORY) else ROUTES_DIRECTORY + ) + return FIXES[FILES_RULE].format(names=LAYER_NAMES[directory]) + + def _compare( rule_id: str, found: Counter[str], @@ -721,7 +739,7 @@ def _compare( else f"{entry} ({_occurrences(found[entry])}, the baseline tolerates " f"{tolerated[entry]})" ) - violations.append(Violation(rule_id, CHECK_NAME, message, FIXES[rule_id])) + violations.append(Violation(rule_id, CHECK_NAME, message, _fix(rule_id, entry))) for entry in sorted(tolerated): if tolerated[entry] <= found[entry]: continue From e8ce3125d653a79d0d8af6621b57c602f36bd9dc Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:11:17 +0400 Subject: [PATCH 14/15] Test that the exception guidance asks for an expires date only (failing) rules-registry.md reserves an exception's removal condition for rules whose guard reads it, which the v2-layers guard doesn't, so a files-rule exception should end on an expires date. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_v2_layers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index db9c34919..ce55711ec 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -732,7 +732,7 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: "database_connectors/[_name].py), " "or, if it can't be renamed, add a [[rule.exception]] for " "api.v2.files-assigned-to-layers to rules/registry.toml with its " - "path, a reason and an expires date or removal condition." + "path, a reason and an expires date." ], ) @@ -750,7 +750,7 @@ def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: "_models.py or _support.py), or, if it can't be " "renamed, add a [[rule.exception]] for " "api.v2.files-assigned-to-layers to rules/registry.toml with its " - "path, a reason and an expires date or removal condition." + "path, a reason and an expires date." ], ) From 9b621b1a678103f1545b04f0519000b937103022 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:11:52 +0400 Subject: [PATCH 15/15] Ask for an expires date on exceptions and say which checks they give - The files rule's fix message and v2-code-organization.md ask for an exception's path, reason and expires date; rules-registry.md reserves removal conditions for rules whose guard reads them. - The guard, its fixtures and the docs say an excepted file gets the pure-module checks under services/v2 and the route checks under fastapi_routes/v2, instead of its directory's "strictest" checks, which overstated the services case. - Two fixture comments no longer mention the deleted manifest. Refs #4190 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/v2-code-organization.md | 4 ++-- .../bad_tree.toml | 5 ++--- .../good_tree.toml | 2 +- .../bad_tree.toml | 2 +- .../bad_tree.toml | 2 +- scripts/rules/v2_layers.py | 16 +++++++++------- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index ddf773971..0e601790e 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -24,8 +24,8 @@ guard (`scripts/rules/v2_layers.py`) takes from the file's name alone: A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its -`path`, a `reason`, and an `expires` date or a `removal` condition. An -exception chooses no layer: the file gets the route checks under +`path`, a `reason` and an `expires` date, by which the file should be renamed. +An exception chooses no layer: the file gets the route checks under `fastapi_routes/v2`, and the validator, transformation and types checks (rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml index 0660d58c4..96af20577 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -3,9 +3,8 @@ # - simulation_client.py's name gives it no layer, and no [[rule.exception]] # names it (the case of an open pull request that adds a v2 resource); # - the two files listed in ``exceptions`` stand for files a -# [[rule.exception]] names. They get no layer, but their directory's -# strictest checks: the pure-module checks under services/v2 and the route -# checks under fastapi_routes/v2. +# [[rule.exception]] names. They get no layer, but the pure-module checks +# under services/v2 and the route checks under fastapi_routes/v2. # scripts/rules/tests/test_v2_layers.py writes each file into a temporary # directory and runs the v2-layers guard's scan on it. diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml index 32baf28f5..bed2c9564 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml @@ -1,6 +1,6 @@ # Proof fixture for rule api.v2.files-assigned-to-layers. The guard must accept # it: every file is named after its layer, and the one excepted file passes -# the strictest checks of its directory. +# the pure-module checks it gets under services/v2. exceptions = ["policyengine_api/services/v2/household_calculations/legacy_client.py"] expected_findings = [] diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml index dd50effcd..f4e4cfc60 100644 --- a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -2,7 +2,7 @@ # reject it: a validator loads a row through the session it was handed, a # types module runs a query, and a package __init__ commits. # scripts/rules/tests/test_v2_layers.py writes each file into a temporary -# directory and runs the v2-layers guard's scan on it, with no manifest. +# directory and runs the v2-layers guard's scan on it. # Each expected finding is ": ". expected_findings = [ diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml index d4cd9b884..55289073f 100644 --- a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml +++ b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml @@ -2,7 +2,7 @@ # reject it: the database_session module adds a row, loads one with # Session.get and runs raw SQL, work that belongs in a connector. # scripts/rules/tests/test_v2_layers.py writes each file into a temporary -# directory and runs the v2-layers guard's scan on it, with no manifest. +# directory and runs the v2-layers guard's scan on it. # Each expected finding is ": ". expected_findings = [ diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index df64a7dc3..279c4a6cf 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -5,9 +5,10 @@ (see ``assign_layer``). A file whose name gives none fails (rule ``api.v2.files-assigned-to-layers``), so a new file is never left unchecked. A file that can't be renamed needs a ``[[rule.exception]]`` for that rule in -rules/registry.toml. The exception gives it no layer: it gets its directory's -strictest checks, the route checks under ``fastapi_routes/v2`` and the -pure-module checks under ``services/v2``. +rules/registry.toml, ending on an ``expires`` date. The exception gives it no +layer: it gets the route checks under ``fastapi_routes/v2``, and the +pure-module (validator, transformation and types) checks under +``services/v2``. Then each file's imports and calls are checked against its layer's rule: @@ -117,7 +118,8 @@ ), } # What a [[rule.exception]] for the files rule makes a file: no layer of its -# own, and its directory's strictest checks. +# own; under services/v2 it gets the pure-module checks (under +# fastapi_routes/v2 every file gets the route checks). EXCEPTED = "excepted" PURE_LAYERS = frozenset( {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} @@ -186,7 +188,7 @@ FILES_RULE: ( "name it after its layer ({names}), or, if it can't be renamed, add a " f"[[rule.exception]] for {FILES_RULE} to {REGISTRY_FILE} with its path, " - "a reason and an expires date or removal condition" + "a reason and an expires date" ), ROUTES_RULE: ( "v2 route modules call one service method and map its typed failures to " @@ -249,8 +251,8 @@ def _module_name(path: str) -> tuple[str, bool]: def assign_layer(path: str, exceptions: Collection[str] = ()) -> str | None: """Return the layer of the file at ``path`` (repository-relative), or None. - A path in ``exceptions`` is ``excepted``: it gets its directory's - strictest checks. + A path in ``exceptions`` is ``excepted``: under services/v2 it gets the + pure-module checks, and under fastapi_routes/v2 the route checks. """ if path in exceptions: return EXCEPTED