From cb01ffa8028b1dd304f7099b0e66d04a44b1199f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:32:59 +0400 Subject: [PATCH 1/9] Test that file exceptions expire and cover whole files (failing) - A files-rule exception that sets category, removal or match, or has no expires date, is reported: the guard reads none of them, so such an exception could last forever or claim to cover one site. - A database_connectors module can't be excepted, because rule api.v2.crud-checks-cover-every-resource also checks connector names, and an unassigned one is told to take its operation's name. - The services fix message says where each name must sit. - The files rule's proof also has an unassigned file under fastapi_routes/v2. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- .../bad_tree.toml | 10 +- scripts/rules/tests/test_v2_layers.py | 99 +++++++++++++++++-- 2 files changed, 101 insertions(+), 8 deletions(-) diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml index 96af20577..d3e9c58da 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -1,7 +1,8 @@ # Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject # it: -# - simulation_client.py's name gives it no layer, and no [[rule.exception]] -# names it (the case of an open pull request that adds a v2 resource); +# - simulation_client.py and reports/helpers.py have names that give no layer, +# and no [[rule.exception]] names them (simulation_client.py is the case of +# an open pull request that adds a v2 resource); # - the two files listed in ``exceptions`` stand for files a # [[rule.exception]] names. They get no layer, but the pure-module checks # under services/v2 and the route checks under fastapi_routes/v2. @@ -17,6 +18,7 @@ expected_findings = [ "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", "api.v2.files-assigned-to-layers: policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", + "api.v2.files-assigned-to-layers: policyengine_api/fastapi_routes/v2/reports/helpers.py matches no v2 layer", ] [files] @@ -35,3 +37,7 @@ def load(session, household_id): def keep(session, row): session.add(row) """ +"policyengine_api/fastapi_routes/v2/reports/helpers.py" = """ +def format_title(report): + return report.title.strip() +""" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index ce55711ec..9dcfa7125 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -574,7 +574,10 @@ def setUp(self) -> None: self.root = Path(self._directory.name) def _registry( - self, baseline_count: int | None, exceptions: tuple[str, ...] = () + self, + baseline_count: int | None, + exceptions: tuple[str, ...] = (), + exception_fields: str = "expires = 2099-01-01\n", ) -> str: rules = [] for rule_id in (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE): @@ -586,7 +589,7 @@ def _registry( ) excepted = "".join( f'[[rule.exception]]\npath = "{path}"\nreason = "Generated."\n' - "expires = 2099-01-01\n" + + exception_fields for path in (exceptions if rule_id == FILES_RULE else ()) ) rules.append( @@ -727,15 +730,35 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: [ f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " "name it after its layer (services.py, validators.py, " - "transformations.py, types.py, process_state.py, " - "database_session.py or " - "database_connectors/[_name].py), " - "or, if it can't be renamed, add a [[rule.exception]] for " + "transformations.py, types.py or process_state.py in a resource " + "package under services/v2, database_session.py directly in " + "one, or database_connectors/" + "[_name].py directly in its database_connectors package), or, " + "if it can't be renamed, add a [[rule.exception]] for " "api.v2.files-assigned-to-layers to rules/registry.toml with its " "path, a reason and an expires date." ], ) + def test_an_unassigned_connector_file_must_be_renamed(self) -> None: + unassigned = ( + "policyengine_api/services/v2/policies/database_connectors/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 operation, " + "[_name].py; an exception can't " + "stand in, because rule api.v2.crud-checks-cover-every-resource " + "also checks connector names." + ], + ) + def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: unassigned = "policyengine_api/fastapi_routes/v2/metadata/helpers.py" _write_files( @@ -774,6 +797,70 @@ def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None: ], ) + def test_an_exception_ends_on_its_expires_date_and_covers_the_whole_file( + self, + ) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + prefix = f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client}" + no_date = ( + f"{prefix} from {FILES_RULE}, but the exception has no expires date. " + "Fix: give it an expires date by which the file is renamed." + ) + unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." + for fields, field_name, ends in ( + ('category = "forever"\n', "category", False), + ('removal = "never"\n', "removal", False), + ('match = "session.get"\nexpires = 2099-01-01\n', "match", True), + ): + with self.subTest(field=field_name): + _write_files( + { + client: "", + "rules/registry.toml": self._registry( + None, (client,), exception_fields=fields + ), + }, + self.root, + ) + expected = [ + f"{prefix} from {FILES_RULE}, but the exception sets " + f"{field_name}, which this rule doesn't read. Fix: remove it; " + "a files-rule exception covers the whole file and ends on " + "its expires date." + ] + if not ends: + expected.append(no_date) + + messages = self._messages() + + self.assertEqual( + sorted( + message + for message in messages + if not message.startswith(unassigned) + ), + sorted(expected), + ) + self.assertTrue( + any(message.startswith(unassigned) for message in messages) + ) + + def test_a_connector_module_cannot_be_excepted(self) -> None: + helper = "policyengine_api/services/v2/policies/database_connectors/helpers.py" + _write_files( + {helper: "", "rules/registry.toml": self._registry(None, (helper,))}, + self.root, + ) + + self.assertIn( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {helper} from " + f"{FILES_RULE}, but a database_connectors module must be named after " + "its operation (rule api.v2.crud-checks-cover-every-resource). Fix: " + "rename it to [_name].py and remove " + "the exception.", + self._messages(), + ) + def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: named = ROUTE missing = "policyengine_api/services/v2/policies/gone.py" From f651b4a2a602b9b97accd605454c95faaac1b3db Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:34:07 +0400 Subject: [PATCH 2/9] Require file exceptions to expire and cover whole files; split the guard - A files-rule exception that sets category, removal or match, or has no expires date, is reported and not applied; one for a module in a database_connectors package is rejected, because api.v2.crud-checks-cover-every-resource needs its operation name. - The services fix message says where each name must sit, and a misnamed connector module is told to take its operation's name. - The guard reached 834 lines, over the 800-line limit, so the per-file import and call checks move to scripts/rules/v2_layer_checks.py. scripts/rules/v2_layers.py keeps the layer assignment, exceptions, scanning and baselines, and the names it exported. - v2-code-organization.md describes the exception constraints. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- .../skills/v2-code-organization.md | 5 +- scripts/rules/v2_layer_checks.py | 438 ++++++++++++++++ scripts/rules/v2_layers.py | 496 +++--------------- 3 files changed, 511 insertions(+), 428 deletions(-) create mode 100644 scripts/rules/v2_layer_checks.py diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 0e601790e..4e8373d3e 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -25,7 +25,10 @@ A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its `path`, a `reason` and an `expires` date, by which the file should be renamed. -An exception chooses no layer: the file gets the route checks under +It covers the whole file, so it sets no `match`, `category` or `removal`, and a +module in a `database_connectors` package can't be excepted: rule +`api.v2.crud-checks-cover-every-resource` needs its operation name. An +exception chooses no layer: the file gets the route checks under `fastapi_routes/v2`, and the validator, transformation and types checks (rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already diff --git a/scripts/rules/v2_layer_checks.py b/scripts/rules/v2_layer_checks.py new file mode 100644 index 000000000..5791eceb1 --- /dev/null +++ b/scripts/rules/v2_layer_checks.py @@ -0,0 +1,438 @@ +"""Import and call checks for the v2-layers quality guard, one layer at a time. + +``file_findings`` parses one v2 file and returns what its layer's rule +forbids in it. scripts/rules/v2_layers.py assigns the layers, runs these +checks over every v2 file and compares the findings with the baselines; its +docstring describes what each layer may import and call. + +Standard library only. +""" + +from __future__ import annotations + +import ast +from dataclasses import dataclass +from pathlib import PurePosixPath +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Mapping + +SERVICES_DIRECTORY = "policyengine_api/services/v2" +ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" + +ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" +SERVICES_RULE = "api.v2.services-no-sql" +CONNECTORS_RULE = "api.v2.connectors-never-commit" +PURE_RULE = "api.v2.pure-modules-no-session-calls" +SESSION_RULE = "api.v2.session-modules-no-data-access" + +# What a [[rule.exception]] for the files rule makes a file: no layer of its +# own; under services/v2 it gets the pure-module checks (under +# fastapi_routes/v2 every file gets the route checks). +EXCEPTED = "excepted" +PURE_LAYERS = frozenset( + {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} +) + +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 = (*SESSION_TYPES, SQL_EXCEPTIONS) +SQL_BUILDERS = frozenset( + { + "delete", + "except_", + "exists", + "insert", + "intersect", + "literal_column", + "select", + "text", + "union", + "union_all", + "update", + "values", + } +) +SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) +TRANSACTION_METHODS = frozenset( + {"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", + "scalar", + "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", "requests") +# Owners whose ``.session`` is Starlette's session data, not a database session. +REQUEST_OWNERS = frozenset({"req", "request", "websocket", "ws"}) +HTTP_PACKAGES = ("fastapi", "starlette") + + +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__" + if is_package: + parts = parts[:-1] + return ".".join(parts), is_package + + +# --- imports -------------------------------------------------------------- + + +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 [])]) + + +@dataclass(frozen=True) +class ImportStatement: + """One imported module, or one ``from`` import with the names it binds.""" + + module: str + names: tuple[str, ...] + + +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(ImportStatement(alias.name, ()) for alias in node.names) + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + names = tuple(f"{source}.{alias.name}" for alias in node.names) + statements.append(ImportStatement(source, 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 _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_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 ( + _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 _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 + + +# --- 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 = [] + 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 _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 _is_request_owner(node.value) + ): + return False + name = _receiver_name(node).lower().lstrip("_") + 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 _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 + return ( + isinstance(func, ast.Attribute) + and func.attr in SQL_BUILDERS - {"values"} + and isinstance(func.value, ast.Attribute) + and func.value.attr == "__table__" + ) + + +# --- policies ------------------------------------------------------------- + + +@dataclass(frozen=True) +class LayerPolicy: + """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; ``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 + forbidden_module: Callable[[str], bool] + session_methods: frozenset[str] | None + checks_sql_calls: bool + checks_factories: bool + forbidden_name: Callable[[str], bool] = _never + checks_builders: bool = False + + +ROUTE_POLICY = LayerPolicy( + ROUTES_RULE, + _is_route_forbidden_import, + None, + checks_sql_calls=True, + checks_factories=True, +) +PURE_POLICY = LayerPolicy( + PURE_RULE, + _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_sql_calls=True, + checks_factories=False, + forbidden_name=_is_service_forbidden_sql_name, + ), + "connector": LayerPolicy( + CONNECTORS_RULE, + _is_connector_forbidden_import, + 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), +} + + +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. + + For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. + """ + last = dotted.rsplit(".", 1)[-1] + from_sql = _within_any(dotted, SQL_PACKAGES) + opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( + from_sql and last in SQL_SESSION_FACTORIES + ) + 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}"] + if policy.checks_builders and from_sql and last in SQL_BUILDERS: + return [f"calls {dotted}"] + return [] + + +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_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 + ) + if forbidden_method 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 = _policy(path, layer) + if policy is None: + return [] + tree = ast.parse(source) + details = [ + 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)): + details.extend(_call_details(call, imported, policy)) + return [(policy.rule_id, detail) for detail in details] diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 279c4a6cf..983e1f749 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -60,34 +60,38 @@ from __future__ import annotations -import ast from collections import Counter from dataclasses import dataclass +from datetime import date 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 +from scripts.rules.v2_layer_checks import ( + CONNECTORS_RULE, + EXCEPTED, + PURE_RULE, + ROUTES_DIRECTORY, + ROUTES_RULE, + SERVICES_DIRECTORY, + SERVICES_RULE, + SESSION_RULE, + file_findings, +) if TYPE_CHECKING: - from collections.abc import Callable, Collection, Iterable, Mapping + from collections.abc import Collection, Iterable - from scripts.rules.model import Rule + from scripts.rules.model import Rule, RuleException REPO_ROOT = Path(__file__).resolve().parents[2] CHECK_NAME = "v2-layers" REGISTRY_FILE = "rules/registry.toml" -SERVICES_DIRECTORY = "policyengine_api/services/v2" -ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" ORGANIZATION_DOC = "docs/engineering/skills/v2-code-organization.md" 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" -PURE_RULE = "api.v2.pure-modules-no-session-calls" -SESSION_RULE = "api.v2.session-modules-no-data-access" RULES = ( FILES_RULE, ROUTES_RULE, @@ -109,81 +113,18 @@ # The names that give a layer, by directory, for the files rule's fix message. LAYER_NAMES = { SERVICES_DIRECTORY: ( - "services.py, validators.py, transformations.py, types.py, " - "process_state.py, database_session.py or " - "database_connectors/[_name].py" + "services.py, validators.py, transformations.py, types.py or " + "process_state.py in a resource package under services/v2, " + "database_session.py directly in one, or " + "database_connectors/[_name].py directly " + "in its database_connectors package" ), ROUTES_DIRECTORY: ( "routes.py, _routes.py, _models.py or _support.py" ), } -# What a [[rule.exception]] for the files rule makes a file: no layer of its -# own; under services/v2 it gets the pure-module checks (under -# fastapi_routes/v2 every file gets the route checks). -EXCEPTED = "excepted" -PURE_LAYERS = frozenset( - {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} -) GLOB_CHARACTERS = frozenset("*?[") -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 = (*SESSION_TYPES, SQL_EXCEPTIONS) -SQL_BUILDERS = frozenset( - { - "delete", - "except_", - "exists", - "insert", - "intersect", - "literal_column", - "select", - "text", - "union", - "union_all", - "update", - "values", - } -) -SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) -TRANSACTION_METHODS = frozenset( - {"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", - "scalar", - "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", "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 = { FILES_RULE: ( "name it after its layer ({names}), or, if it can't be renamed, add a " @@ -229,22 +170,6 @@ def __str__(self) -> str: return f"{self.path} {self.detail}" -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__" - if is_package: - parts = parts[:-1] - return ".".join(parts), is_package - - # --- layers --------------------------------------------------------------- @@ -300,334 +225,6 @@ def _route_layer(name: str) -> str | None: return "http-support" if name.endswith("_support.py") 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: - 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 [])]) - - -@dataclass(frozen=True) -class ImportStatement: - """One imported module, or one ``from`` import with the names it binds.""" - - module: str - names: tuple[str, ...] - - -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(ImportStatement(alias.name, ()) for alias in node.names) - elif isinstance(node, ast.ImportFrom): - source = _from_source(node, path) - names = tuple(f"{source}.{alias.name}" for alias in node.names) - statements.append(ImportStatement(source, 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 _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_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 ( - _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 _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 - - -# --- 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 = [] - 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 _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 _is_request_owner(node.value) - ): - return False - name = _receiver_name(node).lower().lstrip("_") - 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 _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 - return ( - isinstance(func, ast.Attribute) - and func.attr in SQL_BUILDERS - {"values"} - and isinstance(func.value, ast.Attribute) - and func.value.attr == "__table__" - ) - - -# --- policies ------------------------------------------------------------- - - -@dataclass(frozen=True) -class LayerPolicy: - """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; ``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 - forbidden_module: Callable[[str], bool] - session_methods: frozenset[str] | None - checks_sql_calls: bool - checks_factories: bool - forbidden_name: Callable[[str], bool] = _never - checks_builders: bool = False - - -ROUTE_POLICY = LayerPolicy( - ROUTES_RULE, - _is_route_forbidden_import, - None, - checks_sql_calls=True, - checks_factories=True, -) -PURE_POLICY = LayerPolicy( - PURE_RULE, - _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_sql_calls=True, - checks_factories=False, - forbidden_name=_is_service_forbidden_sql_name, - ), - "connector": LayerPolicy( - CONNECTORS_RULE, - _is_connector_forbidden_import, - 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), -} - - -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. - - For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. - """ - last = dotted.rsplit(".", 1)[-1] - from_sql = _within_any(dotted, SQL_PACKAGES) - opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( - from_sql and last in SQL_SESSION_FACTORIES - ) - 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}"] - if policy.checks_builders and from_sql and last in SQL_BUILDERS: - return [f"calls {dotted}"] - return [] - - -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_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 - ) - if forbidden_method 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 = _policy(path, layer) - if policy is None: - return [] - tree = ast.parse(source) - details = [ - 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)): - details.extend(_call_details(call, imported, policy)) - return [(policy.rule_id, detail) for detail in details] - - # --- scanning and comparing ----------------------------------------------- @@ -652,13 +249,18 @@ def scan( source = (root / path).read_text(encoding="utf-8") findings.extend( Finding(rule_id, path, detail) - for rule_id, detail in _file_findings(path, source, layer) + for rule_id, detail in file_findings(path, source, layer) ) return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) -def _exception_violations(root: Path, path: str) -> list[Violation]: - """Report a files-rule exception that names no unnamed v2 file.""" +def _exception_violations(root: Path, exception: RuleException) -> list[Violation]: + """Report a files-rule exception that doesn't name one unnamed v2 file. + + It must also end on an ``expires`` date and cover the whole file: the + guard reads no ``category``, ``removal`` or ``match``. + """ + path = exception.path def violation(problem: str, fix: str) -> Violation: return Violation( @@ -683,7 +285,32 @@ def violation(problem: str, fix: str) -> Violation: return [ violation("but its name already gives it a layer", "remove the exception") ] - return [] + if _in_connector_package(path): + return [ + violation( + "but a database_connectors module must be named after its " + "operation (rule api.v2.crud-checks-cover-every-resource)", + "rename it to [_name].py and remove " + "the exception", + ) + ] + violations = [ + violation( + f"but the exception sets {field}, which this rule doesn't read", + "remove it; a files-rule exception covers the whole file and ends on " + "its expires date", + ) + for field in ("category", "removal", "match") + if getattr(exception, field) is not None + ] + if exception.expires is None or exception.expires == date.max: + violations.append( + violation( + "but the exception has no expires date", + "give it an expires date by which the file is renamed", + ) + ) + return violations def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Violation]]: @@ -693,7 +320,7 @@ def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Viol excepted = [] violations = [] for exception in rule.exceptions: - problems = _exception_violations(root, exception.path) + problems = _exception_violations(root, exception) violations += problems if not problems: excepted.append(exception.path) @@ -714,10 +341,25 @@ def _occurrences(count: int) -> str: return f"{count} occurrence" if count == 1 else f"{count} occurrences" +CONNECTOR_NAME_FIX = ( + "name it after its operation, [_name].py; an " + "exception can't stand in, because rule " + "api.v2.crud-checks-cover-every-resource also checks connector names" +) + + +def _in_connector_package(path: str) -> bool: + return path.startswith(f"{SERVICES_DIRECTORY}/") and ( + "database_connectors" in PurePosixPath(path).parts[:-1] + ) + + def _fix(rule_id: str, entry: str) -> str: """Return the rule's fix; the files rule's names the layers of its directory.""" if rule_id != FILES_RULE: return FIXES[rule_id] + if _in_connector_package(entry.split(" ", 1)[0]): + return CONNECTOR_NAME_FIX directory = ( SERVICES_DIRECTORY if entry.startswith(SERVICES_DIRECTORY) else ROUTES_DIRECTORY ) From b6e956008965dc266becea715c3631013d7a52f8 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:34:59 +0400 Subject: [PATCH 3/9] Add changelog fragment for #4204 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4204.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4204.fixed.md diff --git a/changelog.d/4204.fixed.md b/changelog.d/4204.fixed.md new file mode 100644 index 000000000..db5b5360e --- /dev/null +++ b/changelog.d/4204.fixed.md @@ -0,0 +1 @@ +Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, or names a `database_connectors` module, and say in the fix message where each layer name must sit. From c991814cac0b1a01bc989dea73c8e0d58ee77b56 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:49:50 +0400 Subject: [PATCH 4/9] Test connector placement, exception path spelling and missing dates (failing) - A file in a database_connectors package that already has an operation's name but sits one level too deep is told to move directly into the package, not to take a name it already has. The exception refusal no longer says the CRUD rule checks such a file, which it doesn't. - An exception path with a '.', '..' or empty segment is reported: the guard compares paths as written, so it would match no scanned file. - An exception with no expires, category or removal, or an unreadable date, is reported. This test already passes; it covers a branch no test reached. - The match subtest's flag is named has_expires_date. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_v2_layers.py | 136 ++++++++++++++++++++------ 1 file changed, 105 insertions(+), 31 deletions(-) diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 9dcfa7125..414b067a3 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -740,24 +740,34 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: ], ) - def test_an_unassigned_connector_file_must_be_renamed(self) -> None: - unassigned = ( - "policyengine_api/services/v2/policies/database_connectors/helpers.py" - ) - _write_files( - {unassigned: "", "rules/registry.toml": self._registry(None)}, self.root - ) + def test_an_unassigned_connector_file_is_told_its_name_and_place(self) -> None: + connectors = "policyengine_api/services/v2/policies/database_connectors" + # The second file already has an operation's name, but sits one level + # too deep, so the message must say where it goes too. + for unassigned in ( + f"{connectors}/helpers.py", + f"{connectors}/legacy/creates.py", + ): + with self.subTest(path=unassigned): + _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 operation, " - "[_name].py; an exception can't " - "stand in, because rule api.v2.crud-checks-cover-every-resource " - "also checks connector names." - ], - ) + messages = self._messages() + (self.root / unassigned).unlink() + + self.assertEqual( + messages, + [ + f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 " + "layer. Fix: name it after its operation, " + "[_name].py, and put it " + "directly in its database_connectors package; an " + "exception can't stand in, because a database_connectors " + "package holds only connector modules." + ], + ) def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: unassigned = "policyengine_api/fastapi_routes/v2/metadata/helpers.py" @@ -807,7 +817,7 @@ def test_an_exception_ends_on_its_expires_date_and_covers_the_whole_file( "Fix: give it an expires date by which the file is renamed." ) unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." - for fields, field_name, ends in ( + for fields, field_name, has_expires_date in ( ('category = "forever"\n', "category", False), ('removal = "never"\n', "removal", False), ('match = "session.get"\nexpires = 2099-01-01\n', "match", True), @@ -828,7 +838,7 @@ def test_an_exception_ends_on_its_expires_date_and_covers_the_whole_file( "a files-rule exception covers the whole file and ends on " "its expires date." ] - if not ends: + if not has_expires_date: expected.append(no_date) messages = self._messages() @@ -846,20 +856,84 @@ def test_an_exception_ends_on_its_expires_date_and_covers_the_whole_file( ) def test_a_connector_module_cannot_be_excepted(self) -> None: - helper = "policyengine_api/services/v2/policies/database_connectors/helpers.py" - _write_files( - {helper: "", "rules/registry.toml": self._registry(None, (helper,))}, - self.root, - ) + connectors = "policyengine_api/services/v2/policies/database_connectors" + for helper in (f"{connectors}/helpers.py", f"{connectors}/legacy/creates.py"): + with self.subTest(path=helper): + _write_files( + { + helper: "", + "rules/registry.toml": self._registry(None, (helper,)), + }, + self.root, + ) - self.assertIn( - f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {helper} from " - f"{FILES_RULE}, but a database_connectors module must be named after " - "its operation (rule api.v2.crud-checks-cover-every-resource). Fix: " - "rename it to [_name].py and remove " - "the exception.", - self._messages(), + self.assertIn( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " + f"{helper} from {FILES_RULE}, but a database_connectors " + "package holds only connector modules (rule " + "api.v2.crud-checks-cover-every-resource also fails any other " + "module directly in services/v2//database_connectors)" + ". Fix: name it after its operation, " + "[_name].py, put it directly " + "in its database_connectors package, and remove the " + "exception.", + self._messages(), + ) + + def test_an_exception_with_no_readable_expires_date_is_reported(self) -> None: + # The registry loader also reports these, so this calls the guard + # directly rather than through _messages, which requires a clean load. + client = "policyengine_api/services/v2/household_calculations/client.py" + no_date = ( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client} from " + f"{FILES_RULE}, but the exception has no expires date. Fix: give it " + "an expires date by which the file is renamed." ) + for fields, case in (("", "no expires"), ('expires = "soon"\n', "bad date")): + with self.subTest(case=case): + _write_files( + { + client: "", + "rules/registry.toml": self._registry( + None, (client,), exception_fields=fields + ), + }, + self.root, + ) + + self.assertIn( + no_date, [str(violation) for violation in run_check(self.root)] + ) + + def test_an_exception_path_must_be_written_as_git_lists_it(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." + for spelling in ( + "policyengine_api/services/v2/household_calculations/./client.py", + "policyengine_api/services/v2/household_calculations//client.py", + "policyengine_api/services/v2/policies/../household_calculations/client.py", + ): + with self.subTest(path=spelling): + _write_files( + { + client: "", + "rules/registry.toml": self._registry(None, (spelling,)), + }, + self.root, + ) + + messages = self._messages() + + self.assertIn( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " + f"{spelling} from {FILES_RULE}, but its path has a '.', '..' " + "or empty segment, so it matches no file the guard scans. " + "Fix: write the path as git ls-files lists it.", + messages, + ) + self.assertTrue( + any(message.startswith(unassigned) for message in messages) + ) def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: named = ROUTE From 14b1d9cc25b2bd51fbf3efd5172343270f5f5f3b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:50:54 +0400 Subject: [PATCH 5/9] Name connector placement in fix messages and reject unscanned exception paths - The fix for an unassigned file in a database_connectors package, and the refusal of an exception for one, say to put it directly in the package as well as to name it after its operation. The refusal's reason is that the package holds only connector modules; the CRUD rule is cited only for the files it checks. - An exception whose path has a '.', '..' or empty segment is reported and not applied, because assign_layer compares paths as git ls-files writes them. - v2-code-organization.md and the changelog fragment say the same. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4204.fixed.md | 2 +- .../skills/v2-code-organization.md | 11 +++++--- scripts/rules/v2_layers.py | 28 ++++++++++++++----- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/changelog.d/4204.fixed.md b/changelog.d/4204.fixed.md index db5b5360e..bc71028fb 100644 --- a/changelog.d/4204.fixed.md +++ b/changelog.d/4204.fixed.md @@ -1 +1 @@ -Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, or names a `database_connectors` module, and say in the fix message where each layer name must sit. +Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, names a module in a `database_connectors` package, or writes its path with a `.`, `..` or empty segment, and say in the fix message where each layer name must sit. diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 4e8373d3e..65c676ea4 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -25,10 +25,13 @@ A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its `path`, a `reason` and an `expires` date, by which the file should be renamed. -It covers the whole file, so it sets no `match`, `category` or `removal`, and a -module in a `database_connectors` package can't be excepted: rule -`api.v2.crud-checks-cover-every-resource` needs its operation name. An -exception chooses no layer: the file gets the route checks under +Write the path as `git ls-files` lists it. An exception covers the whole file, +so it sets no `match`, `category` or `removal`. A module in a +`database_connectors` package can't be excepted: that package holds only +connector modules, each named after its operation and placed directly in it +(rule `api.v2.crud-checks-cover-every-resource` also fails any other module +directly in `services/v2//database_connectors`). An exception +chooses no layer: the file gets the route checks under `fastapi_routes/v2`, and the validator, transformation and types checks (rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 983e1f749..de236cef8 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -279,6 +279,15 @@ def violation(problem: str, fix: str) -> Violation: "name one file there, or remove the exception", ) ] + if any(segment in ("", ".", "..") for segment in path.split("/")): + # assign_layer compares paths as git ls-files writes them. + return [ + violation( + "but its path has a '.', '..' or empty segment, so it matches no " + "file the guard scans", + "write the path as git ls-files lists it", + ) + ] if not (root / path).is_file(): return [violation("but it does not exist", "remove the exception")] if _layer_by_name(path) is not None: @@ -288,10 +297,10 @@ def violation(problem: str, fix: str) -> Violation: if _in_connector_package(path): return [ violation( - "but a database_connectors module must be named after its " - "operation (rule api.v2.crud-checks-cover-every-resource)", - "rename it to [_name].py and remove " - "the exception", + "but a database_connectors package holds only connector modules " + "(rule api.v2.crud-checks-cover-every-resource also fails any " + "other module directly in services/v2//database_connectors)", + f"{CONNECTOR_NAME}, {CONNECTOR_PLACE}, and remove the exception", ) ] violations = [ @@ -341,10 +350,15 @@ def _occurrences(count: int) -> str: return f"{count} occurrence" if count == 1 else f"{count} occurrences" +# A connector gets its layer only with an operation's name, directly in a +# database_connectors package, so the fix names both. +CONNECTOR_NAME = ( + "name it after its operation, [_name].py" +) +CONNECTOR_PLACE = "put it directly in its database_connectors package" CONNECTOR_NAME_FIX = ( - "name it after its operation, [_name].py; an " - "exception can't stand in, because rule " - "api.v2.crud-checks-cover-every-resource also checks connector names" + f"{CONNECTOR_NAME}, and {CONNECTOR_PLACE}; an exception can't stand in, " + "because a database_connectors package holds only connector modules" ) From c91586109b399eabb4445cd3ffa59b9e26543883 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:01:01 +0400 Subject: [PATCH 6/9] Test exception paths against the scanned files and the connector location (failing) - An exception path that names a real file under another spelling is reported with one message, whatever the spelling. The '..' case now has its middle directory, so it reaches the real file as the bug needs. - Connector fix messages name services/v2//database_connectors, the location the CRUD checks cover, rather than any database_connectors package. - The connector-exception test removes each subtest's file and checks the refused module is still reported as having no layer. - The expiry test is named for what the guard checks: that an expires date is required. The registry's expiry check ends the exception. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_v2_layers.py | 54 ++++++++++++++++++--------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 414b067a3..47f0a2e56 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -763,9 +763,9 @@ def test_an_unassigned_connector_file_is_told_its_name_and_place(self) -> None: f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 " "layer. Fix: name it after its operation, " "[_name].py, and put it " - "directly in its database_connectors package; an " - "exception can't stand in, because a database_connectors " - "package holds only connector modules." + "directly in services/v2//database_connectors; " + "an exception can't stand in, because a " + "database_connectors package holds only connector modules." ], ) @@ -807,7 +807,7 @@ def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None: ], ) - def test_an_exception_ends_on_its_expires_date_and_covers_the_whole_file( + def test_an_exception_needs_an_expires_date_and_covers_the_whole_file( self, ) -> None: client = "policyengine_api/services/v2/household_calculations/client.py" @@ -867,17 +867,32 @@ def test_a_connector_module_cannot_be_excepted(self) -> None: self.root, ) - self.assertIn( - f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " - f"{helper} from {FILES_RULE}, but a database_connectors " - "package holds only connector modules (rule " - "api.v2.crud-checks-cover-every-resource also fails any other " - "module directly in services/v2//database_connectors)" - ". Fix: name it after its operation, " - "[_name].py, put it directly " - "in its database_connectors package, and remove the " - "exception.", - self._messages(), + messages = self._messages() + (self.root / helper).unlink() + + self.assertEqual( + sorted(messages), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " + f"{helper} from {FILES_RULE}, but a database_connectors " + "package holds only connector modules (rule " + "api.v2.crud-checks-cover-every-resource also fails " + "any other module directly in " + "services/v2//database_connectors). Fix: " + "name it after its operation, " + "[_name].py, put it " + "directly in services/v2//database_connectors, " + "and remove the exception.", + f"[{FILES_RULE}] v2-layers: {helper} matches no v2 " + "layer. Fix: name it after its operation, " + "[_name].py, and put it " + "directly in services/v2//database_connectors; " + "an exception can't stand in, because a " + "database_connectors package holds only connector " + "modules.", + ] + ), ) def test_an_exception_with_no_readable_expires_date_is_reported(self) -> None: @@ -906,7 +921,12 @@ def test_an_exception_with_no_readable_expires_date_is_reported(self) -> None: ) def test_an_exception_path_must_be_written_as_git_lists_it(self) -> None: + # Each spelling names the real file, so is_file() accepts it, but the + # guard compares exceptions with the paths it scans. client = "policyengine_api/services/v2/household_calculations/client.py" + _write_files( + {"policyengine_api/services/v2/policies/services.py": ""}, self.root + ) unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." for spelling in ( "policyengine_api/services/v2/household_calculations/./client.py", @@ -926,8 +946,8 @@ def test_an_exception_path_must_be_written_as_git_lists_it(self) -> None: self.assertIn( f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " - f"{spelling} from {FILES_RULE}, but its path has a '.', '..' " - "or empty segment, so it matches no file the guard scans. " + f"{spelling} from {FILES_RULE}, but no file the guard scans " + "has that exact path, so the exception would match nothing. " "Fix: write the path as git ls-files lists it.", messages, ) From bcc4756ed7c60d5927f5e32a8ebfbac640762252 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:01:48 +0400 Subject: [PATCH 7/9] Accept only scanned exception paths and name the CRUD-checked connector location - A files-rule exception must name a path the guard scans, as git ls-files writes it. Any other spelling of a real file (a '.', '..' or empty segment, a symlinked directory, or different letter case on macOS) is reported instead of being accepted and never applied. run_check lists the files once and passes them to both steps. - The connector fix messages say to put the module directly in services/v2//database_connectors, where the CRUD checks find it, rather than in any database_connectors package. - The changelog fragment says the same. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4204.fixed.md | 2 +- scripts/rules/v2_layers.py | 45 +++++++++++++++++++++++++------------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/changelog.d/4204.fixed.md b/changelog.d/4204.fixed.md index bc71028fb..0b2b600f6 100644 --- a/changelog.d/4204.fixed.md +++ b/changelog.d/4204.fixed.md @@ -1 +1 @@ -Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, names a module in a `database_connectors` package, or writes its path with a `.`, `..` or empty segment, and say in the fix message where each layer name must sit. +Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, names a module in a `database_connectors` package, or gives a path other than the one `git ls-files` lists, and say in the fix message where each layer name must sit. diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index de236cef8..88eaeb1cd 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -254,11 +254,15 @@ def scan( return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) -def _exception_violations(root: Path, exception: RuleException) -> list[Violation]: +def _exception_violations( + root: Path, exception: RuleException, scanned: Collection[str] +) -> list[Violation]: """Report a files-rule exception that doesn't name one unnamed v2 file. - It must also end on an ``expires`` date and cover the whole file: the - guard reads no ``category``, ``removal`` or ``match``. + The path must be one of ``scanned``, the paths as git ls-files writes + them, because assign_layer compares exceptions with those. The exception + must also end on an ``expires`` date and cover the whole file: the guard + reads no ``category``, ``removal`` or ``match``. """ path = exception.path @@ -279,17 +283,18 @@ def violation(problem: str, fix: str) -> Violation: "name one file there, or remove the exception", ) ] - if any(segment in ("", ".", "..") for segment in path.split("/")): - # assign_layer compares paths as git ls-files writes them. + if path not in scanned: + if not (root / path).is_file(): + return [violation("but it does not exist", "remove the exception")] + # Another spelling of a real file: a '.', '..' or empty segment, a + # symlinked directory, or different letter case on macOS. return [ violation( - "but its path has a '.', '..' or empty segment, so it matches no " - "file the guard scans", + "but no file the guard scans has that exact path, so the " + "exception would match nothing", "write the path as git ls-files lists it", ) ] - if not (root / path).is_file(): - return [violation("but it does not exist", "remove the exception")] if _layer_by_name(path) is not None: return [ violation("but its name already gives it a layer", "remove the exception") @@ -322,14 +327,19 @@ def violation(problem: str, fix: str) -> Violation: return violations -def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Violation]]: - """Return the files the files rule excepts, and its exceptions that don't belong.""" +def file_exceptions( + root: Path, rule: Rule | None, scanned: Collection[str] +) -> tuple[list[str], list[Violation]]: + """Return the files the files rule excepts, and its exceptions that don't belong. + + ``scanned`` holds the paths the guard scans. + """ if rule is None: return [], [] excepted = [] violations = [] for exception in rule.exceptions: - problems = _exception_violations(root, exception) + problems = _exception_violations(root, exception, scanned) violations += problems if not problems: excepted.append(exception.path) @@ -355,7 +365,9 @@ def _occurrences(count: int) -> str: CONNECTOR_NAME = ( "name it after its operation, [_name].py" ) -CONNECTOR_PLACE = "put it directly in its database_connectors package" +# The location the CRUD checks cover; deeper connector packages get a layer +# from this guard but aren't checked by them. +CONNECTOR_PLACE = "put it directly in services/v2//database_connectors" CONNECTOR_NAME_FIX = ( f"{CONNECTOR_NAME}, and {CONNECTOR_PLACE}; an exception can't stand in, " "because a database_connectors package holds only connector modules" @@ -423,8 +435,11 @@ def run_check(root: Path = REPO_ROOT) -> list[Violation]: registry, violations = load_registry(root) if registry is None: return violations - excepted, violations = file_exceptions(root, registry.rule(FILES_RULE)) - findings = scan(root, excepted) + scanned = _discovered_files(root) + excepted, violations = file_exceptions( + root, registry.rule(FILES_RULE), frozenset(scanned) + ) + findings = scan(root, excepted, scanned) for rule_id in RULES: rule = registry.rule(rule_id) baseline_file = rule.baseline.file if rule and rule.baseline else None From 9f973eb4eaaf120d2ee53a6681716dfe2f4f9afb Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:11:49 +0400 Subject: [PATCH 8/9] Test untracked exception files and the connector-package remedies (failing) - An exception that spells an untracked file's path exactly, in a git checkout, is told to git add the file as well as to check the spelling. This test runs the guard against git ls-files, which the other exception tests, in plain directories, don't. - A module in a database_connectors package is told to move out of the package if it isn't a connector, as well as how to name and place a connector. - The refusal of a connector-package exception says the CRUD rule fails a module there whose name doesn't start with an operation; its globs pass names such as createsfoo.py. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_v2_layers.py | 61 ++++++++++++++++++--------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index 47f0a2e56..5299d11f4 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -2,6 +2,7 @@ from __future__ import annotations +import subprocess import tempfile import textwrap import tomllib @@ -27,6 +28,17 @@ 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" +# What the files rule tells a module in a database_connectors package to do. +CONNECTOR_REMEDY_TEXT = ( + "if it reads or writes rows, name it after its operation, " + "[_name].py, and put it directly in " + "services/v2//database_connectors; otherwise move it out of the " + "database_connectors package and name it after its layer" +) +CONNECTOR_FIX_TEXT = ( + f"{CONNECTOR_REMEDY_TEXT}; an exception can't stand in, because a " + "database_connectors package holds only connector modules" +) def _write_files(files: dict[str, str], root: Path) -> None: @@ -761,11 +773,7 @@ def test_an_unassigned_connector_file_is_told_its_name_and_place(self) -> None: messages, [ f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 " - "layer. Fix: name it after its operation, " - "[_name].py, and put it " - "directly in services/v2//database_connectors; " - "an exception can't stand in, because a " - "database_connectors package holds only connector modules." + f"layer. Fix: {CONNECTOR_FIX_TEXT}." ], ) @@ -877,20 +885,13 @@ def test_a_connector_module_cannot_be_excepted(self) -> None: f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " f"{helper} from {FILES_RULE}, but a database_connectors " "package holds only connector modules (rule " - "api.v2.crud-checks-cover-every-resource also fails " - "any other module directly in " - "services/v2//database_connectors). Fix: " - "name it after its operation, " - "[_name].py, put it " - "directly in services/v2//database_connectors, " - "and remove the exception.", + "api.v2.crud-checks-cover-every-resource also fails a " + "module directly in " + "services/v2//database_connectors whose name " + "doesn't start with an operation). Fix: remove the " + f"exception; {CONNECTOR_REMEDY_TEXT}.", f"[{FILES_RULE}] v2-layers: {helper} matches no v2 " - "layer. Fix: name it after its operation, " - "[_name].py, and put it " - "directly in services/v2//database_connectors; " - "an exception can't stand in, because a " - "database_connectors package holds only connector " - "modules.", + f"layer. Fix: {CONNECTOR_FIX_TEXT}.", ] ), ) @@ -948,13 +949,35 @@ def test_an_exception_path_must_be_written_as_git_lists_it(self) -> None: f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " f"{spelling} from {FILES_RULE}, but no file the guard scans " "has that exact path, so the exception would match nothing. " - "Fix: write the path as git ls-files lists it.", + "Fix: write the path as git ls-files lists it, and git add the " + "file if git doesn't track it.", messages, ) self.assertTrue( any(message.startswith(unassigned) for message in messages) ) + def test_an_exception_for_a_file_git_does_not_track_is_reported(self) -> None: + # In a git checkout the guard scans only the files git tracks, so an + # exactly spelled exception for an untracked file would match nothing. + client = "policyengine_api/services/v2/household_calculations/client.py" + _write_files( + {client: "", "rules/registry.toml": self._registry(None, (client,))}, + self.root, + ) + subprocess.run(["git", "init", "-q"], cwd=self.root, check=True) + + self.assertEqual( + self._messages(), + [ + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client} " + f"from {FILES_RULE}, but no file the guard scans has that exact " + "path, so the exception would match nothing. Fix: write the path " + "as git ls-files lists it, and git add the file if git doesn't " + "track it." + ], + ) + def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: named = ROUTE missing = "policyengine_api/services/v2/policies/gone.py" From ed5fcca3a8a06ead497c2da5da87964e1105295a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:12:35 +0400 Subject: [PATCH 9/9] Tell untracked and non-connector files the right fix; correct the CRUD claim - The fix for an exception path the guard doesn't scan also says to git add the file if git doesn't track it. - A module in a database_connectors package is told to move out of the package if it isn't a connector, or else to take an operation's name directly in services/v2//database_connectors. - The refusal of a connector-package exception, and v2-code-organization.md, say the CRUD rule fails a module there whose name doesn't start with an operation, which is what its globs check. - The files rule's statement says the exception is for a file outside a database_connectors package; docs/generated/rules.md is regenerated. Refs #4204 Co-Authored-By: Claude Opus 5.5 --- .../skills/v2-code-organization.md | 13 +++---- docs/generated/rules.md | 2 +- rules/registry.toml | 2 +- scripts/rules/v2_layers.py | 34 +++++++++++-------- 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 65c676ea4..edd12d18e 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -25,13 +25,14 @@ A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its `path`, a `reason` and an `expires` date, by which the file should be renamed. -Write the path as `git ls-files` lists it. An exception covers the whole file, -so it sets no `match`, `category` or `removal`. A module in a -`database_connectors` package can't be excepted: that package holds only +Write the path as `git ls-files` lists it, for a file git tracks. An exception +covers the whole file, so it sets no `match`, `category` or `removal`. A module +in a `database_connectors` package can't be excepted: that package holds only connector modules, each named after its operation and placed directly in it -(rule `api.v2.crud-checks-cover-every-resource` also fails any other module -directly in `services/v2//database_connectors`). An exception -chooses no layer: the file gets the route checks under +(rule `api.v2.crud-checks-cover-every-resource` also fails a module directly in +`services/v2//database_connectors` whose name doesn't start with an +operation). Move a module that isn't a connector out of the package. An +exception chooses no layer: the file gets the route checks under `fastapi_routes/v2`, and the validator, transformation and types checks (rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 6b5450373..0438fae16 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -97,7 +97,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed, outside a database_connectors package, needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | diff --git a/rules/registry.toml b/rules/registry.toml index 3af49483b..2785a1a68 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -183,7 +183,7 @@ invocation = "make test" [[rule]] id = "api.v2.files-assigned-to-layers" category = "v2-layers" -statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." +statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed, outside a database_connectors package, needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 88eaeb1cd..8d27f205d 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -286,13 +286,15 @@ def violation(problem: str, fix: str) -> Violation: if path not in scanned: if not (root / path).is_file(): return [violation("but it does not exist", "remove the exception")] - # Another spelling of a real file: a '.', '..' or empty segment, a - # symlinked directory, or different letter case on macOS. + # Another spelling of a real file (a '.', '..' or empty segment, a + # symlinked directory, or different letter case on macOS), or a file + # git doesn't track. return [ violation( "but no file the guard scans has that exact path, so the " "exception would match nothing", - "write the path as git ls-files lists it", + "write the path as git ls-files lists it, and git add the file if " + "git doesn't track it", ) ] if _layer_by_name(path) is not None: @@ -303,9 +305,10 @@ def violation(problem: str, fix: str) -> Violation: return [ violation( "but a database_connectors package holds only connector modules " - "(rule api.v2.crud-checks-cover-every-resource also fails any " - "other module directly in services/v2//database_connectors)", - f"{CONNECTOR_NAME}, {CONNECTOR_PLACE}, and remove the exception", + "(rule api.v2.crud-checks-cover-every-resource also fails a module " + "directly in services/v2//database_connectors whose name " + "doesn't start with an operation)", + f"remove the exception; {CONNECTOR_REMEDY}", ) ] violations = [ @@ -361,16 +364,19 @@ def _occurrences(count: int) -> str: # A connector gets its layer only with an operation's name, directly in a -# database_connectors package, so the fix names both. -CONNECTOR_NAME = ( - "name it after its operation, [_name].py" +# database_connectors package, so the fix names both. It points to the +# location the CRUD checks cover: deeper connector packages get a layer from +# this guard but aren't checked by them. A module that isn't a connector +# belongs outside the package. +CONNECTOR_REMEDY = ( + "if it reads or writes rows, name it after its operation, " + "[_name].py, and put it directly in " + "services/v2//database_connectors; otherwise move it out of the " + "database_connectors package and name it after its layer" ) -# The location the CRUD checks cover; deeper connector packages get a layer -# from this guard but aren't checked by them. -CONNECTOR_PLACE = "put it directly in services/v2//database_connectors" CONNECTOR_NAME_FIX = ( - f"{CONNECTOR_NAME}, and {CONNECTOR_PLACE}; an exception can't stand in, " - "because a database_connectors package holds only connector modules" + f"{CONNECTOR_REMEDY}; an exception can't stand in, because a " + "database_connectors package holds only connector modules" )