Merge pull request #3913 from PolicyEngine/ruff-exceptions

Enable ruff exception rules (TRY, EM, BLE)

Author: anth-volk

.github/find-api-model-versions.py

@@ -30,15 +30,15 @@
 def _manifest_versions(bundle: dict) -> dict[str, str]:
     packages = bundle.get("packages")
     if not isinstance(packages, dict):
-        raise RuntimeError("PolicyEngine bundle manifest has no package mapping.")
+        msg = "PolicyEngine bundle manifest has no package mapping."
+        raise TypeError(msg)
 
     versions = {}
     for package_name in REQUIRED_PACKAGES:
         package = packages.get(package_name)
         if not isinstance(package, dict) or not package.get("version"):
-            raise RuntimeError(
-                f"PolicyEngine bundle manifest has no version for {package_name}."
-            )
+            msg = f"PolicyEngine bundle manifest has no version for {package_name}."
+            raise RuntimeError(msg)
         versions[package_name] = str(package["version"])
     return versions
 
@@ -47,26 +47,27 @@ def _data_release_version(bundle: dict, country_id: str) -> str:
     data_releases = bundle.get("data_releases")
     release = data_releases.get(country_id) if isinstance(data_releases, dict) else None
     if not isinstance(release, dict) or not release.get("version"):
-        raise RuntimeError(
-            f"PolicyEngine bundle manifest has no {country_id.upper()} data release."
-        )
+        msg = f"PolicyEngine bundle manifest has no {country_id.upper()} data release."
+        raise RuntimeError(msg)
     return str(release["version"])
 
 
 def find_api_model_versions() -> dict[str, str]:
     """Find and validate the package and data versions in the installed bundle."""
     bundle = get_current_bundle()
     if not isinstance(bundle, dict):
-        raise RuntimeError("Installed PolicyEngine bundle manifest is not an object.")
+        msg = "Installed PolicyEngine bundle manifest is not an object."
+        raise TypeError(msg)
 
     manifest_versions = _manifest_versions(bundle)
     for package_name, manifest_version in manifest_versions.items():
         installed_version = distribution_version(package_name)
         if installed_version != manifest_version:
-            raise RuntimeError(
+            msg = (
                 f"Installed {package_name} version {installed_version} does not match "
                 f"bundle manifest version {manifest_version}."
             )
+            raise RuntimeError(msg)
 
     expected_constants = {
         "policyengine": POLICYENGINE_VERSION,
@@ -76,10 +77,11 @@ def find_api_model_versions() -> dict[str, str]:
     }
     for package_name, constant_version in expected_constants.items():
         if constant_version != manifest_versions[package_name]:
-            raise RuntimeError(
+            msg = (
                 f"API version for {package_name} is {constant_version}, but the bundle "
                 f"manifest specifies {manifest_versions[package_name]}."
             )
+            raise RuntimeError(msg)
 
     return {
         "POLICYENGINE_VERSION": manifest_versions["policyengine"],

.github/scripts/verify_observability_collector.py

@@ -74,7 +74,8 @@ class VerificationProbe(NamedTuple):
 def _authority(endpoint: str) -> str:
     parsed = urlparse(endpoint)
     if parsed.scheme != "https" or not parsed.hostname:
-        raise ValueError("collector endpoint must be an HTTPS URL")
+        msg = "collector endpoint must be an HTTPS URL"
+        raise ValueError(msg)
     return f"{parsed.hostname}:{parsed.port or 443}"
 
 
@@ -193,7 +194,8 @@ def _raise_for_partial_rejection(
     rejected = getattr(partial, rejected_field, 0)
     if rejected:
         message = getattr(partial, "error_message", "")
-        raise RuntimeError(f"collector rejected {rejected} {signal}: {message}")
+        msg = f"collector rejected {rejected} {signal}: {message}"
+        raise RuntimeError(msg)
 
 
 def _get_json(url: str, access_token: str) -> dict[str, object] | None:
@@ -208,9 +210,8 @@ def _get_json(url: str, access_token: str) -> dict[str, object] | None:
         if error.code == 404:
             return None
         detail = error.read().decode("utf-8", errors="replace")
-        raise RuntimeError(
-            f"Google Cloud verification request failed ({error.code}): {detail}"
-        ) from error
+        msg = f"Google Cloud verification request failed ({error.code}): {detail}"
+        raise RuntimeError(msg) from error
 
 
 def _trace_available(
@@ -302,7 +303,8 @@ def _wait_for_delivery(
                 )
                 if not found
             )
-            raise RuntimeError(f"collector verification data was not stored: {missing}")
+            msg = f"collector verification data was not stored: {missing}"
+            raise RuntimeError(msg)
         time.sleep(min(poll_seconds, remaining))
 
 
@@ -357,7 +359,8 @@ def verify(
             if error.code() is not grpc.StatusCode.UNIMPLEMENTED:
                 raise
         else:
-            raise RuntimeError("collector unexpectedly accepted application logs")
+            msg = "collector unexpectedly accepted application logs"
+            raise RuntimeError(msg)
     _wait_for_delivery(
         project_id=project_id,
         trace_id=trace_id,

AGENTS.md

@@ -123,7 +123,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`.
 - `api.quality.environment-variables-load`: Required environment variables and credentials load in a deployed-like environment. (blocking: CI job `test_env_vars`)
 - `api.quality.mypy-v2`: Shared query models, FastAPI dependencies, v2 routes and services, and the user sign-in and account-deletion libraries pass mypy with untyped definitions disallowed. (blocking: mypy)
 - `api.quality.ruff-format`: All Python files are formatted with ruff format. (blocking: ruff format)
-- `api.quality.ruff-lint`: All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. (blocking: ruff check (ANN, D, E4, E7, E9, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, UP, W))
+- `api.quality.ruff-lint`: All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. (blocking: ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W))
 - `api.quality.runtime-evaluated-models-resolve`: Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. (blocking: pytest `tests/unit/test_runtime_evaluated_annotations.py`)
 - `api.quality.tests-pass`: The unit, contract and to-refactor test suites pass. (blocking: CI job `test`)
 - `api.quality.tests-ruff-exemptions`: Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**`)

changelog.d/ruff-exceptions.changed.md

@@ -0,0 +1 @@
+Enabled ruff's exception rules (TRY, EM, BLE): exception messages are built before the raise, raises inside try blocks moved into helpers, the v1 services raise typed exceptions, best-effort telemetry and logging calls use a commented contextlib.suppress block, and every other deliberate catch-all carries a reasoned suppression. Period parsing and the simulation health probe now catch only the errors they expect, and a non-object .py bundle manifest raises TypeError. Error responses and message text are unchanged, except that a non-HTTP failure in the simulation health probe now gets the route's existing 503 for a client failure.

docs/generated/rules.md

@@ -272,7 +272,7 @@ flowchart LR
 | `api.quality.environment-variables-load` | Required environment variables and credentials load in a deployed-like environment. | mechanical | enforced | yes | CI job `test_env_vars` | `pr.yml:test_env_vars` | — |
 | `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, and the user sign-in and account-deletion libraries pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:typecheck-v2` | — |
 | `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:lint` | — |
-| `api.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, D, E4, E7, E9, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, UP, W) | `pr.yml:lint` | — |
+| `api.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W) | `pr.yml:lint` | — |
 | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** |
 | `api.quality.tests-pass` | The unit, contract and to-refactor test suites pass. | mechanical | enforced | yes | CI job `test` | `pr.yml:test` | — |
 | `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**` | `pr.yml:lint` | — |
@@ -292,7 +292,7 @@ flowchart LR
   e_ruff_format["ruff format"]
   j_CI_pr_yml_lint["CI pr.yml:lint"]
   r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)")
-  e_ruff_check__ANN__D__E4__E7__E9__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__UP__W_["ruff check (ANN, D, E4, E7, E9, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, UP, W)"]
+  e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_["ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W)"]
   r_api_quality_runtime_evaluated_models_resolve("api.quality.runtime-evaluated-models-resolve (enforced)")
   e_pytest_tests_unit_test_runtime_evaluated_annotations_py["pytest tests/unit/test_runtime_evaluated_annotations.py"]
   j_CI_pr_yml_test["CI pr.yml:test"]
@@ -308,8 +308,8 @@ flowchart LR
   e_mypy --> j_CI_pr_yml_typecheck_v2
   r_api_quality_ruff_format --> e_ruff_format
   e_ruff_format --> j_CI_pr_yml_lint
-  r_api_quality_ruff_lint --> e_ruff_check__ANN__D__E4__E7__E9__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__UP__W_
-  e_ruff_check__ANN__D__E4__E7__E9__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__UP__W_ --> j_CI_pr_yml_lint
+  r_api_quality_ruff_lint --> e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_
+  e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_ --> j_CI_pr_yml_lint
   r_api_quality_runtime_evaluated_models_resolve --> e_pytest_tests_unit_test_runtime_evaluated_annotations_py
   e_pytest_tests_unit_test_runtime_evaluated_annotations_py --> j_CI_pr_yml_test
   r_api_quality_tests_pass --> e_CI_job_test

migrations/v1/env.py

@@ -22,18 +22,21 @@
 provided_connection = config.attributes.get("connection")
 if provided_connection is not None:
     if provided_connection.dialect.name != "mysql":
-        raise RuntimeError("Alembic migrations must target a MySQL database")
+        msg = "Alembic migrations must target a MySQL database"
+        raise RuntimeError(msg)
 else:
     database_url = os.environ.get("ALEMBIC_DATABASE_URL") or config.get_main_option(
         "sqlalchemy.url"
     )
     if not database_url:
-        raise RuntimeError(
+        msg = (
             "ALEMBIC_DATABASE_URL is required; the local SQLite cache is not an "
             "Alembic migration target"
         )
+        raise RuntimeError(msg)
     if make_url(database_url).get_backend_name() != "mysql":
-        raise RuntimeError("Alembic migrations must target a MySQL database")
+        msg = "Alembic migrations must target a MySQL database"
+        raise RuntimeError(msg)
     config.set_main_option("sqlalchemy.url", database_url.replace("%", "%%"))
 
 target_metadata = V1Base.metadata

migrations/v2/env.py

@@ -65,10 +65,11 @@ def _configure(connection: Connection) -> None:
 
 def run_migrations_offline() -> None:
     """Raise, because v2 migrations need a live connection to qualify the target."""
-    raise RuntimeError(
+    msg = (
         "v2 migrations require an online connection so target identity and "
         "generated application-data before/after states can be verified"
     )
+    raise RuntimeError(msg)
 
 
 def run_migrations_online() -> None:

policyengine_api/asgi_factory.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import contextlib
 import time
 from contextlib import asynccontextmanager
 from typing import TYPE_CHECKING, Any
@@ -236,19 +237,19 @@ async def add_request_context_and_migration_logging(
                     request.state.policyengine_request_id = request_id
                     _asgi_request_id.reset(context_token)
                     context_token = _asgi_request_id.set(request_id)
-            except Exception:
+            except Exception:  # noqa: BLE001  # telemetry must never fail the request
                 pass
-            try:
+            # Telemetry must never fail the request.
+            with contextlib.suppress(Exception):
                 request_runtime.set_context(
                     request_id=request_id,
                 )
-            except Exception:
-                pass
 
         def log_native_route(status_code: int) -> None:
             if not native_request:
                 return
-            try:
+            # Migration logging must never fail the request.
+            with contextlib.suppress(Exception):
                 log_migration_request(
                     request_id=request_id,
                     method=request.method,
@@ -261,8 +262,6 @@ def log_native_route(status_code: int) -> None:
                     ),
                     route_impl=RouteImplementation.FASTAPI_NATIVE,
                 )
-            except Exception:
-                pass
 
         def finish_native_route(
             status_code: int,
@@ -271,21 +270,18 @@ def finish_native_route(
             if not native_request:
                 return
             resolved_route = getattr(request.scope.get("route"), "path", initial_route)
-            try:
+            # Telemetry must never fail the request.
+            with contextlib.suppress(Exception):
                 request_runtime.update_request_route(resolved_route)
-            except Exception:
-                pass
-            try:
+            # Telemetry must never fail the request.
+            with contextlib.suppress(Exception):
                 request_runtime.update_request_status(status_code)
-            except Exception:
-                pass
-            try:
+            # Telemetry must never fail the request.
+            with contextlib.suppress(Exception):
                 request_runtime.end_request(
                     status_code=status_code,
                     error=error,
                 )
-            except Exception:
-                pass
 
         try:
             try:
@@ -301,20 +297,19 @@ def finish_native_route(
                 )
             )
             if native_request and response_observability_id is not None:
-                try:
+                # Telemetry must never fail the request.
+                with contextlib.suppress(Exception):
                     # Route-level spans have completed at this point, so this
                     # call applies the selected calculation identifier to the
                     # still-current server request span.
                     request_runtime.set_context(
                         observability_id=response_observability_id
                     )
-                except Exception:
-                    pass
             if native_request:
                 try:
                     for name, value in request_runtime.response_headers().items():
                         response.headers[name] = value
-                except Exception:
+                except Exception:  # noqa: BLE001  # telemetry must never fail the request
                     pass
             _apply_request_id_header(response, request_id)
             if response_observability_id is not None:

policyengine_api/constants.py

@@ -62,12 +62,14 @@ def _load_policyengine_bundle() -> dict | None:
         with open(get_py_manifest()) as manifest_file:
             manifest = json.load(manifest_file)
     except Exception as exc:
-        raise RuntimeError(
+        msg = (
             "Could not read PolicyEngine .py bundle manifest from the installed "
             "policyengine wheel."
-        ) from exc
+        )
+        raise RuntimeError(msg) from exc
     if not isinstance(manifest, dict):
-        raise RuntimeError("PolicyEngine .py bundle manifest must be a JSON object.")
+        msg = "PolicyEngine .py bundle manifest must be a JSON object."
+        raise TypeError(msg)
     return manifest
 
 
@@ -162,7 +164,7 @@ def get_bundle_default_dataset_option(country_id: str) -> dict:
         country: _resolve_distribution_version(_dist_versions, package_name)
         for country, package_name in zip(COUNTRIES, COUNTRY_PACKAGE_NAMES)
     }
-except Exception:
+except Exception:  # noqa: BLE001  # unreadable package metadata falls back to 0.0.0; import must not fail
     _dist_versions = {}
     COUNTRY_PACKAGE_VERSIONS = {country: "0.0.0" for country in COUNTRIES}
 
@@ -204,9 +206,8 @@ def _build_runtime_cache_version(
     prefix = "e" if scope == "economy_impact" else "r"
     digest_length = 10 - len(prefix) - len(schema_version)
     if digest_length < 4:
-        raise ValueError(
-            f"Runtime cache version for {scope} does not fit in VARCHAR(10)"
-        )
+        msg = f"Runtime cache version for {scope} does not fit in VARCHAR(10)"
+        raise ValueError(msg)
 
     raw = "|".join(
         (

policyengine_api/country.py

@@ -635,7 +635,7 @@ def _normalize_reform_values(
                     node_type = float
                 try:
                     value = float(value)
-                except Exception:
+                except (TypeError, ValueError, OverflowError):
                     pass
                 normalized_reform[parameter_name][time_period] = node_type(value)
         return normalized_reform