From 4ba6e7131d35a6152f88f78358fab2f5d68feec7 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 1/9] 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 6ae4136c31ded4b999607b6f76da117ca6234305 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 2/9] 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 532879272340ea6f6f7bb26130c7f1e6e1572f18 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 3/9] 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 834b127cf24e2e0cac2b727aa0ce7727c6595b38 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 4/9] 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 ef3738407415ff3e120c11a3c63c1527f3688d12 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 5/9] 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 d0a49ed3962d58da524d189240137eeee756c7d2 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 6/9] 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 d18175c20c88e947ae8387f3107f3fd3865fb75b 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 7/9] 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 f7a58d2c6066cbdf991bba862451e4c4866f6b00 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 8/9] 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 5af955d75e7dd0aaeb18da595bd2d5b90214c78e 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 9/9] 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