From 8033161efbad37ca04dad12c63b4aa8534f331d6 Mon Sep 17 00:00:00 2001 From: Brandon Walter <51866976+echoparkbaby@users.noreply.github.com> Date: Sun, 3 May 2026 15:04:38 -0500 Subject: [PATCH] fix: address Codex routes-split follow-up review (1.0.0-39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three low-severity findings from Codex on the 1.0.0-37 split: 1. Trim dead package-level imports in routes/__init__.py — only `poller` was actually used; auth/burnin/mailer/settings_store were the exact shadowing footgun the absolute sub-router imports work around. Reword the comment block to match. 2. Thread `operator` through smart_start + smart_cancel. Previously the JS client sent it but the server ignored it; add audit_events rows (smart_test_start / smart_test_cancel) so the field is actually meaningful. 3. New tests/test_routes_resolution.py — guards two historical regressions: /api/v1/burnin/export.csv must register before /{job_id} (FastAPI int-coerce 422 trap) and the mailer back-compat shim `from app.routes import _fetch_drives_for_template` must keep importing. Plus a sub-router enumeration test that catches missed include_router calls in future splits. --- app/config.py | 2 +- app/routes/__init__.py | 12 ++--- app/routes/drives.py | 23 ++++++++++ tests/test_routes_resolution.py | 79 +++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 tests/test_routes_resolution.py diff --git a/app/config.py b/app/config.py index 4827a50..816a05b 100644 --- a/app/config.py +++ b/app/config.py @@ -83,7 +83,7 @@ class Settings(BaseSettings): ssh_key: str = "" # PEM private key content (paste full key including headers) # Application version — used by the /api/v1/updates/check endpoint - app_version: str = "1.0.0-38" + app_version: str = "1.0.0-39" # ---- Authentication (1.0.0-22) ---- # session_secret: HMAC key for signing session cookies. Empty = generate diff --git a/app/routes/__init__.py b/app/routes/__init__.py index 46338f0..75a639c 100644 --- a/app/routes/__init__.py +++ b/app/routes/__init__.py @@ -9,7 +9,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query, Request from fastapi.responses import HTMLResponse, StreamingResponse from sse_starlette.sse import EventSourceResponse -from app import auth, burnin, mailer, poller, settings_store +from app import poller from app.config import settings from app.database import get_db from app.models import ( @@ -39,10 +39,12 @@ router = APIRouter() # working unchanged. Future slices can extract more — drives, burnin, # settings, history — using the same pattern. # -# Absolute imports (vs `from . import auth`) because the line-12 -# `from app import auth` binds `auth` as an attribute on this package's -# namespace, which would shadow the relative-submodule lookup and yield -# `app.auth` instead of `app.routes.auth`. +# Absolute imports (`import app.routes.X as _Y`) instead of relative +# (`from . import X as _Y`) so we stay safe even if a future top-level +# `from app import X` is reintroduced here — `from app import auth` +# would bind `auth` on the `app.routes` package namespace and shadow +# any relative-submodule lookup. Absolute imports always resolve to +# `app.routes.X` regardless of what's already bound on the package. import app.routes.auth as _auth_routes # noqa: E402 import app.routes.system as _system_routes # noqa: E402 import app.routes.history as _history_routes # noqa: E402 diff --git a/app/routes/drives.py b/app/routes/drives.py index dd951dd..03ef6be 100644 --- a/app/routes/drives.py +++ b/app/routes/drives.py @@ -131,6 +131,7 @@ async def get_drive(drive_id: int, db: aiosqlite.Connection = Depends(get_db)): @router.post("/api/v1/drives/{drive_id}/smart/start") async def smart_start( drive_id: int, + request: Request, body: dict, db: aiosqlite.Connection = Depends(get_db), ): @@ -152,6 +153,7 @@ async def smart_start( raise HTTPException(status_code=404, detail="Drive not found") devname = row[0] + operator = operator_for(request, body.get("operator")) now = datetime.now(timezone.utc).isoformat() ttype_lower = test_type.lower() @@ -173,6 +175,12 @@ async def smart_start( raw_output=excluded.raw_output""", (drive_id, ttype_lower, "running", 0, now, output), ) + await db.execute( + """INSERT INTO audit_events (event_type, drive_id, operator, message) + VALUES (?,?,?,?)""", + ("smart_test_start", drive_id, operator, + f"{test_type} SMART test started on {devname}"), + ) await db.commit() poller._notify_subscribers() return {"devname": devname, "type": test_type, "message": output[:200]} @@ -186,12 +194,20 @@ async def smart_start( tn_job_id = await client.start_smart_test([devname], test_type) except Exception as exc: raise HTTPException(status_code=502, detail=f"TrueNAS error: {exc}") + await db.execute( + """INSERT INTO audit_events (event_type, drive_id, operator, message) + VALUES (?,?,?,?)""", + ("smart_test_start", drive_id, operator, + f"{test_type} SMART test started on {devname}"), + ) + await db.commit() return {"job_id": tn_job_id, "devname": devname, "type": test_type} @router.post("/api/v1/drives/{drive_id}/smart/cancel") async def smart_cancel( drive_id: int, + request: Request, body: dict, db: aiosqlite.Connection = Depends(get_db), ): @@ -205,6 +221,7 @@ async def smart_cancel( if not row: raise HTTPException(status_code=404, detail="Drive not found") devname = row[0] + operator = operator_for(request, body.get("operator")) client = burnin._client if client is None: @@ -248,6 +265,12 @@ async def smart_cancel( "UPDATE smart_tests SET state='aborted', finished_at=? WHERE drive_id=? AND test_type=? AND state='running'", (now, drive_id, test_type), ) + await db.execute( + """INSERT INTO audit_events (event_type, drive_id, operator, message) + VALUES (?,?,?,?)""", + ("smart_test_cancel", drive_id, operator, + f"{test_type.upper()} SMART test cancelled on {devname}"), + ) await db.commit() return {"cancelled": True, "devname": devname, "type": test_type} diff --git a/tests/test_routes_resolution.py b/tests/test_routes_resolution.py new file mode 100644 index 0000000..6f11762 --- /dev/null +++ b/tests/test_routes_resolution.py @@ -0,0 +1,79 @@ +"""Route-resolution invariants for the routes/ package. + +Guards two historical regressions Codex flagged were untested: + +1. /api/v1/burnin/export.csv must resolve to the CSV export, not to + /api/v1/burnin/{job_id} with int("export.csv") → 422. FastAPI's + path matching tries declarations in registration order, so the + literal must be declared before the parameterized route. + +2. app.mailer reaches into app.routes for _fetch_drives_for_template + (back-compat from before the routes/ split). The shim re-export + in app/routes/__init__.py must remain importable. + +Run inside the container image so app deps are present. +""" + +from __future__ import annotations + +import unittest + + +class TestRouteResolution(unittest.TestCase): + + def test_export_csv_declared_before_job_id(self): + """Route order in burnin.py: /export.csv must come before + /{job_id} or FastAPI will int-coerce 'export.csv' and 422. + """ + from app.routes import burnin as burnin_routes + + paths = [r.path for r in burnin_routes.router.routes] + self.assertIn("/api/v1/burnin/export.csv", paths) + self.assertIn("/api/v1/burnin/{job_id}", paths) + self.assertLess( + paths.index("/api/v1/burnin/export.csv"), + paths.index("/api/v1/burnin/{job_id}"), + "/export.csv must be registered before /{job_id} or FastAPI " + "will try to int-coerce 'export.csv' and return 422", + ) + + def test_mailer_backcompat_shim(self): + """app.mailer imports _fetch_drives_for_template from app.routes + (NOT app.routes._drives_helpers) — the shim re-export in + routes/__init__.py keeps that working post-split. + """ + from app.routes import _fetch_drives_for_template + self.assertTrue(callable(_fetch_drives_for_template)) + + def test_all_subrouters_included(self): + """Sanity check: every sub-router in app.routes.* is wired into + the package-level router.include_router calls. If a future split + adds a new file but forgets the include, this catches it. + """ + import importlib + import pkgutil + import app.routes as routes_pkg + + sub_modules = [ + name for _, name, _ in pkgutil.iter_modules(routes_pkg.__path__) + if not name.startswith("_") # skip _helpers, _drives_helpers + ] + + registered_paths = {r.path for r in routes_pkg.router.routes} + for mod_name in sub_modules: + mod = importlib.import_module(f"app.routes.{mod_name}") + sub_router = getattr(mod, "router", None) + self.assertIsNotNone( + sub_router, + f"app.routes.{mod_name} has no `router` attribute", + ) + for r in sub_router.routes: + self.assertIn( + r.path, registered_paths, + f"{mod_name}.router has {r.path} but the package " + "router didn't include it", + ) + + +if __name__ == "__main__": + unittest.main()