Make v2 file-layer exceptions expire and cover whole files

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`, 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.

docs/engineering/skills/v2-code-organization.md

@@ -25,7 +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.
-An exception chooses no layer: the file gets the route checks under
+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 a module directly in
+`services/v2/<resource>/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

docs/generated/rules.md

@@ -98,7 +98,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 |

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()
+"""

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"

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, "
+    "<creates|reads|updates|deletes>[_name].py, and put it directly in "
+    "services/v2/<resource>/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:
@@ -574,7 +586,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 +601,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 +742,41 @@ 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/<creates|reads|updates|deletes>[_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/<creates|reads|updates|deletes>"
+                "[_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_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,
+                )
+
+                messages = self._messages()
+                (self.root / unassigned).unlink()
+
+                self.assertEqual(
+                    messages,
+                    [
+                        f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 "
+                        f"layer. Fix: {CONNECTOR_FIX_TEXT}."
+                    ],
+                )
+
     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 +815,169 @@ def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None:
             ],
         )
 
+    def test_an_exception_needs_an_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, has_expires_date 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 has_expires_date:
+                    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:
+        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,
+                )
+
+                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 a "
+                            "module directly in "
+                            "services/v2/<resource>/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 "
+                            f"layer. Fix: {CONNECTOR_FIX_TEXT}.",
+                        ]
+                    ),
+                )
+
+    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:
+        # 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",
+            "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 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.",
+                    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"