From 4c8f1585cd2207982ea90e1182c3ef9c60509dc7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 13:40:34 +0100 Subject: [PATCH 01/15] docs: doctor-migration-checks design (#895) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- ...26-11-10-doctor-migration-checks-design.md | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md diff --git a/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md b/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md new file mode 100644 index 00000000..f511b5e0 --- /dev/null +++ b/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md @@ -0,0 +1,104 @@ +# Doctor runs the migration checks SM010 and SM011 (#895) + +## Goal & scope + +`smpy doctor` / `make doctor` never report `SM010` (database behind head) or +`SM011` (a module table no migration creates), although `CLAUDE.md` and +`docs/reference/diagnostic-codes.md` promise both. `run_diagnostics()` runs them +only when it is handed a `migration_state`, and neither caller (doctor in +`simple_module_core/__main__.py`, the dev boot in +`simple_module_hosting/_dev_boot.py`) passes one. A new module's table can go +unmigrated with no warning. + +In scope: doctor reports both codes; the development boot (and so the in-app +Doctor screen) reports `SM011`; tests; docs; CHANGELOG. +Out of scope: changing what a production boot refuses; `alembic check`-style +column drift (CI already runs `alembic check`, #869). + +## Approach + +Chosen: **read the state from the database doctor already points at.** +Doctor reads `alembic_version` and the table list from `SM_DATABASE_URL`, and +the heads from the host's `alembic.ini`. When the database is at every head, +any table a selected module's models declare that the database does not have +is a table no migration creates: `SM011`. Behind head is `SM010`. + +Rejected: +- *Scan the revision files for `op.create_table(...)`* (what the issue + sketched). It misses `rename_table`, batch operations, tables built in + helpers and raw SQL, so it would raise false `SM011`s on this repo's own + history; the database is the ground truth for "what the migrations create". +- *Migrate a throwaway SQLite database inside doctor.* Accurate, but slow on + every run, and `make ci-doctor` already does exactly this before calling + doctor, so the database approach gets the same answer in CI for free. + +## Design + +**`simple_module_db.migration_state`** (new; db owns Alembic, core may not +import hosting): +`async def read_migration_state(database_url, alembic_ini) -> MigrationState | None` +returns `heads`, `current` (plural `get_current_heads()`, like +`migration_status`) and `tables` (`inspect(...).get_table_names()`), or `None` +when there is no `alembic.ini` / no revisions. It never creates a database: a +SQLite file URL whose file does not exist yields an empty state instead of +connecting (connecting would create `app.db`). + +**`MigrationDiagnostics.check_heads(current, heads)`** (core, new beside the +existing `check_revision_mismatch`, which stays for `run_diagnostics` +callers): the same rule the boot uses (`heads - current`): +- at every head → nothing; +- never migrated (no `alembic_version` rows) → `SM010` **WARNING**, "run + `make migrate`" (a production boot sends this database to `/setup` rather + than refusing it, so it is not an error); +- behind on some branch → `SM010` **ERROR** naming the missing heads (a + production boot refuses this). +`check_table_coverage` (existing) gives `SM011`, run only when at every head. + +**Doctor** (`simple_module_core/__main__.py` → a `_migration_doctor.py` helper +like `_data_doctor.py`): skips silently when `simple_module_db` is not +installed; resolves `alembic.ini` as the CLI does (project root `alembic.ini`, +else `host/alembic.ini`); module tables come from `module_all_tables()` over the +selected modules (the set SM026 already uses). A database it cannot reach (a +stopped Postgres) gives one `SM010` WARNING saying the checks were skipped, +not a crash. + +**Dev boot**: the diagnostics runner runs at build time, before the database +is open, so `SM011` is computed in the lifespan instead: in development, once +`migration_status` reports the database at head, the lifespan reads the +table list from the app's own engine, prints any `SM011`, and wraps +`diagnostics_state.runner` so the in-app Doctor screen (which already maps +`SM010`/`SM011` to its migrations check) shows them on every re-run. `SM010` at +boot is already enforced by the lifespan's own refusal. + +## Decisions made + +- Never-migrated is a WARNING, behind is an ERROR (mirrors production boot). +- Unreachable database: WARNING, migration checks skipped; doctor still exits + on the other findings. +- `SM011` only when at every head; behind head the missing table may simply be + in a pending revision. +- Module tables = tables in each selected module's `models` (as SM026). Tables + declared outside `models` are not checked (missed, never falsely reported). +- No new setting, no new code; docs: `docs/reference/diagnostic-codes.md` + (when each fires, that doctor reads `SM_DATABASE_URL`), CHANGELOG Fixed. + +## Testing + +- `check_heads`: at heads, never migrated, behind (multi-head), extra unknown + revision is not "behind". +- `read_migration_state`: a SQLite file stamped at heads (the test plugin's + create-all + stamp), an unstamped one, a missing file is not created. +- Doctor `main()` end to end: a database at heads with one module table + dropped reports `SM011` and exits 0 (warning); a database stamped one + revision behind reports `SM010` and exits 1; a missing DB file warns. +- Dev boot: the lifespan helper adds `SM011` to `diagnostics_state` and + `rerun()` keeps it. +No browser QA (`--skip-browser`): CLI and boot diagnostics only. + +## Risks + +- `make doctor` on a developer checkout whose dev database is behind will now + exit 1. That is the point of SM010, and the message says `make migrate`. +- `make ci-doctor` migrates first, so CI should stay green; if this repo's own + modules have a table no migration creates, doctor will now say so and the + fix is a migration (that would be a real bug the check exists to find). From f7e19b83ac647dba9e5767180a1decda786080e0 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:45:22 +0100 Subject: [PATCH 02/15] docs: doctor-migration-checks plan (#895) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../2026-11-10-doctor-migration-checks.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/superpowers/plans/2026-11-10-doctor-migration-checks.md diff --git a/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md b/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md new file mode 100644 index 00000000..193be092 --- /dev/null +++ b/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md @@ -0,0 +1,46 @@ +# Doctor migration checks (#895) — implementation plan + +Design: `docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md`. + +Tasks run in order; 1 and 2 are independent of each other. + +1. **`MigrationDiagnostics.check_heads` + `module_table_names`** (core) + - Files: `framework/core/simple_module_core/diagnostics/_migration.py`, + `framework/core/tests/test_diagnostics.py`. + - Test first: at heads → `[]`; no current → SM010 WARNING; one of several + heads missing → SM010 ERROR naming it; an extra unknown current revision + with every head present → `[]`. + - Done: targeted tests pass. + +2. **`simple_module_db.migration_state.read_migration_state`** (db) + - Files: `framework/db/simple_module_db/migration_state.py` (new; also + takes over `_database_exists` from `data_compatibility.py`), + `framework/db/tests/test_db_migration_state.py` (new). + - Test first: no ini → `None`; missing SQLite file → empty current and + tables, file not created; unstamped file → empty current, its tables; + stamped file → current revisions. + - Done: targeted tests pass. + +3. **Doctor wiring** (hard: cross-package) + - Files: `framework/core/simple_module_core/_migration_doctor.py` (new), + `framework/core/simple_module_core/__main__.py`, + `framework/core/tests/test_core_migration_doctor.py` (new). + - Test first, through `main()` with a temp project root holding an + `alembic.ini` and two revisions: DB at heads with a module table missing + → SM011, exit 0; DB behind → SM010, exit 1; missing DB → SM010 warning, + exit 0; unreachable DB → one warning. + - Done: targeted tests + existing doctor tests pass. + +4. **Dev boot SM011** + - Files: `framework/hosting/simple_module_hosting/_table_coverage.py` (new), + `framework/hosting/simple_module_hosting/_lifespan.py`, + `framework/hosting/tests/test_hosting_table_coverage.py` (new). + - Test first: with a runner set and a table missing, results gain SM011 and + `rerun()` keeps it; with no runner nothing runs. + - Done: targeted tests pass. + +5. **Docs + CHANGELOG**: `docs/reference/diagnostic-codes.md` (SM010/SM011 + rows, "When diagnostics fire"), `CHANGELOG.md` Fixed. + +6. **Full checks**: `make lint`, `make test-py` (PYTEST_WORKERS=8), `make + ci-doctor`. From 004c813b6c27224cf01c5af42634750eb546b710 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:46:16 +0100 Subject: [PATCH 03/15] feat(doctor-migration-checks): check_heads and module_table_names Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../diagnostics/_migration.py | 58 +++++++++++++++++-- framework/core/tests/test_diagnostics.py | 19 ++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/framework/core/simple_module_core/diagnostics/_migration.py b/framework/core/simple_module_core/diagnostics/_migration.py index 5c135b0d..023e15ec 100644 --- a/framework/core/simple_module_core/diagnostics/_migration.py +++ b/framework/core/simple_module_core/diagnostics/_migration.py @@ -2,12 +2,60 @@ from __future__ import annotations +from collections.abc import Iterable, Set +from typing import TYPE_CHECKING + +from simple_module_core.diagnostics._expression_index import module_all_tables from simple_module_core.diagnostics._types import Diagnostic, DiagnosticLevel +if TYPE_CHECKING: + from simple_module_core.module import ModuleBase + +_CODE_BEHIND = "SM010" +_MODULE_NAME = "migrations" +_SUGGEST_MIGRATE = "Run: make migrate" + + +def module_table_names(modules: Iterable[ModuleBase]) -> set[str]: + """Names of the tables declared in each module's ``models`` (the set SM026 checks).""" + return {table.name for mod in modules for table in module_all_tables(mod)} + class MigrationDiagnostics: """Validates database migration state.""" + def check_heads(self, current: Set[str], heads: Set[str]) -> list[Diagnostic]: + """SM010 for a database missing any head; the boot check's ``heads - current`` rule. + + A never-migrated database is a WARNING: a production boot sends it to + ``/setup`` rather than refusing it. One behind on any branch is an ERROR, + because a production boot refuses it. + """ + missing = set(heads) - set(current) + if not missing: + return [] + if not current: + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code=_CODE_BEHIND, + message="Database has never been migrated (no alembic_version)", + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, + ) + ] + return [ + Diagnostic( + level=DiagnosticLevel.ERROR, + code=_CODE_BEHIND, + message=( + f"Database is behind: missing heads {', '.join(map(repr, sorted(missing)))}" + ), + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, + ) + ] + def check_revision_mismatch( self, current_revision: str | None, @@ -19,10 +67,10 @@ def check_revision_mismatch( return [ Diagnostic( level=DiagnosticLevel.ERROR, - code="SM010", + code=_CODE_BEHIND, message=(f"Database at revision {current_revision!r}, expected {head_revision!r}"), - module_name="migrations", - suggestion="Run: make migrate", + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, ) ] @@ -38,8 +86,8 @@ def check_table_coverage( level=DiagnosticLevel.WARNING, code="SM011", message=f"Table '{table}' declared in models but not found in migration history", - module_name="migrations", - suggestion=f'Run: make migration msg="add {table}"', + module_name=_MODULE_NAME, + suggestion=f'Run: make migration LABEL= msg="add {table}"', ) for table in sorted(missing) ] diff --git a/framework/core/tests/test_diagnostics.py b/framework/core/tests/test_diagnostics.py index 5a329a9d..a6e6f6c1 100644 --- a/framework/core/tests/test_diagnostics.py +++ b/framework/core/tests/test_diagnostics.py @@ -52,6 +52,25 @@ async def test_sm011_no_warning_when_covered(self): ) assert len(results) == 0 + def test_check_heads_at_every_head(self): + assert MigrationDiagnostics().check_heads({"a", "b"}, {"a", "b"}) == [] + + def test_check_heads_never_migrated_warns(self): + (finding,) = MigrationDiagnostics().check_heads(set(), {"a", "b"}) + assert finding.code == "SM010" + assert finding.level == DiagnosticLevel.WARNING + assert "make migrate" in finding.suggestion + + def test_check_heads_behind_names_the_missing_heads(self): + (finding,) = MigrationDiagnostics().check_heads({"a", "old"}, {"a", "b", "c"}) + assert finding.code == "SM010" + assert finding.level == DiagnosticLevel.ERROR + assert "missing heads 'b', 'c'" in finding.message + + def test_check_heads_extra_revision_is_not_behind(self): + # Same rule as the boot check: only heads the database lacks count. + assert MigrationDiagnostics().check_heads({"a", "b", "extra"}, {"a", "b"}) == [] + class TestPrintDiagnostics: async def test_writes_to_stderr(self, capsys): From ce9a0c1937ed62afe20eed53a73156763cd92b65 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:46:23 +0100 Subject: [PATCH 04/15] fix(doctor-migration-checks): keep the scaffold-compatible SM011 suggestion Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/diagnostics/_migration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/simple_module_core/diagnostics/_migration.py b/framework/core/simple_module_core/diagnostics/_migration.py index 023e15ec..2aa50315 100644 --- a/framework/core/simple_module_core/diagnostics/_migration.py +++ b/framework/core/simple_module_core/diagnostics/_migration.py @@ -87,7 +87,7 @@ def check_table_coverage( code="SM011", message=f"Table '{table}' declared in models but not found in migration history", module_name=_MODULE_NAME, - suggestion=f'Run: make migration LABEL= msg="add {table}"', + suggestion=f'Run: make migration msg="add {table}"', ) for table in sorted(missing) ] From d57e1d5f57e7d974a7bec36573d0eca0df1ae831 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:47:16 +0100 Subject: [PATCH 05/15] feat(doctor-migration-checks): read_migration_state in simple_module_db Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../db/simple_module_db/data_compatibility.py | 15 +-- .../db/simple_module_db/migration_state.py | 71 +++++++++++++++ framework/db/tests/test_db_migration_state.py | 91 +++++++++++++++++++ 3 files changed, 164 insertions(+), 13 deletions(-) create mode 100644 framework/db/simple_module_db/migration_state.py create mode 100644 framework/db/tests/test_db_migration_state.py diff --git a/framework/db/simple_module_db/data_compatibility.py b/framework/db/simple_module_db/data_compatibility.py index 170837ac..d5f029f5 100644 --- a/framework/db/simple_module_db/data_compatibility.py +++ b/framework/db/simple_module_db/data_compatibility.py @@ -1,7 +1,6 @@ """Database-backed Doctor diagnostics, separate from core's static checks.""" from collections.abc import Sequence -from pathlib import Path from alembic.runtime.migration import MigrationContext from simple_module_core.data_compatibility import collect_data_compatibility @@ -9,6 +8,7 @@ from simple_module_core.module import ModuleBase from simple_module_db.listeners import register_listeners +from simple_module_db.migration_state import database_exists from simple_module_db.session import init_db DATA_COMPATIBILITY_CODE = "SM033" @@ -19,7 +19,7 @@ async def database_compatibility_diagnostics( ) -> list[Diagnostic]: state = None try: - if not _database_exists(database_url): + if not database_exists(database_url): return [] # Nothing migrated yet; opening it would create an empty SQLite file. # Read-only diagnostics: never switch a legacy SQLite file to WAL. state = init_db(database_url, sqlite_wal=False) @@ -80,14 +80,3 @@ def _problem(level: DiagnosticLevel, message: str) -> Diagnostic: suggestion="Check database connectivity and apply pending host schema migrations, " "then rerun doctor.", ) - - -def _database_exists(database_url: str) -> bool: - """False for a SQLite file that isn't there yet; every other URL is tried.""" - from sqlalchemy.engine import make_url - - url = make_url(database_url) - if not url.get_backend_name().startswith("sqlite"): - return True - database = url.database or "" - return database in ("", ":memory:") or Path(database).exists() diff --git a/framework/db/simple_module_db/migration_state.py b/framework/db/simple_module_db/migration_state.py new file mode 100644 index 00000000..8ce0ad00 --- /dev/null +++ b/framework/db/simple_module_db/migration_state.py @@ -0,0 +1,71 @@ +"""Read a database's Alembic state without a host app (Doctor's SM010/SM011, #895).""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +from alembic.config import Config +from alembic.runtime.migration import MigrationContext +from alembic.script import ScriptDirectory +from alembic.util.exc import CommandError +from sqlalchemy import inspect +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import create_async_engine +from sqlalchemy.pool import NullPool + + +@dataclass(frozen=True, slots=True) +class MigrationState: + """What the migrations define (``heads``) and what the database holds.""" + + heads: frozenset[str] + #: ``alembic_version`` rows; empty for a database that was never migrated. + current: frozenset[str] + tables: frozenset[str] + + +def database_exists(database_url: str) -> bool: + """False for a SQLite file that isn't there yet; every other URL is tried.""" + url = make_url(database_url) + if not url.get_backend_name().startswith("sqlite"): + return True + database = url.database or "" + return database in ("", ":memory:") or Path(database).exists() + + +def _heads(alembic_ini: Path) -> frozenset[str]: + if not alembic_ini.is_file(): + return frozenset() + try: + return frozenset(ScriptDirectory.from_config(Config(str(alembic_ini))).get_heads()) + except (CommandError, FileNotFoundError): + return frozenset() + + +async def read_migration_state(database_url: str, alembic_ini: str | Path) -> MigrationState | None: + """The heads in *alembic_ini* beside the current revisions and tables of the database. + + ``None`` when there is no ``alembic.ini`` or it defines no revisions: there + is nothing to compare against. A SQLite file that does not exist reads as + empty rather than being opened, which would create it. Connection errors + propagate; their text can carry credentials, so callers must not echo it. + """ + heads = _heads(Path(alembic_ini)) + if not heads: + return None + if not database_exists(database_url): + return MigrationState(heads=heads, current=frozenset(), tables=frozenset()) + + def _read(sync_conn) -> tuple[frozenset[str], frozenset[str]]: + # Plural, like the boot check: a branching history keeps one row per branch. + current = MigrationContext.configure(sync_conn).get_current_heads() + return frozenset(current), frozenset(inspect(sync_conn).get_table_names()) + + engine = create_async_engine(database_url, poolclass=NullPool) + try: + async with engine.connect() as conn: + current, tables = await conn.run_sync(_read) + finally: + await engine.dispose() + return MigrationState(heads=heads, current=current, tables=tables) diff --git a/framework/db/tests/test_db_migration_state.py b/framework/db/tests/test_db_migration_state.py new file mode 100644 index 00000000..4dc888c6 --- /dev/null +++ b/framework/db/tests/test_db_migration_state.py @@ -0,0 +1,91 @@ +"""Doctor reads the migration state from the database it points at (#895).""" + +from pathlib import Path + +import sqlalchemy as sa +from simple_module_db.migration_state import read_migration_state + +_REVISION = """revision = "{rev}" +down_revision = {down} +branch_labels = None +depends_on = None + + +def upgrade(): + pass + + +def downgrade(): + pass +""" + + +def write_alembic_project(root: Path, chain: list[str]) -> Path: + """An ``alembic.ini`` over the ``"rev:down_revision"`` entries in *chain*. + + ``down_revision`` is empty for a root, so a test can build several heads + without a real ``env.py``. + """ + versions = root / "migrations" / "versions" + versions.mkdir(parents=True) + for entry in chain: + rev, _, down = entry.partition(":") + (versions / f"{rev}.py").write_text( + _REVISION.format(rev=rev, down=repr(down) if down else "None") + ) + ini = root / "alembic.ini" + ini.write_text("[alembic]\nscript_location = %(here)s/migrations\n") + return ini + + +def _sqlite(path: Path, *statements: str) -> str: + engine = sa.create_engine(f"sqlite:///{path}") + with engine.begin() as conn: + for statement in statements: + conn.execute(sa.text(statement)) + engine.dispose() + return f"sqlite+aiosqlite:///{path}" + + +async def test_db_migration_state_none_without_an_ini(tmp_path): + assert await read_migration_state("sqlite+aiosqlite://", tmp_path / "alembic.ini") is None + + +async def test_db_migration_state_missing_file_is_not_created(tmp_path): + ini = write_alembic_project(tmp_path, ["a:", "b:"]) + path = tmp_path / "app.db" + + state = await read_migration_state(f"sqlite+aiosqlite:///{path}", ini) + + assert state is not None + assert state.heads == {"a", "b"} + assert state.current == set() + assert state.tables == set() + assert not path.exists() + + +async def test_db_migration_state_reads_an_unstamped_database(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + url = _sqlite(tmp_path / "app.db", "CREATE TABLE orders_order (id INTEGER)") + + state = await read_migration_state(url, ini) + + assert state is not None + assert state.current == set() + assert state.tables == {"orders_order"} + + +async def test_db_migration_state_reads_current_heads(tmp_path): + ini = write_alembic_project(tmp_path, ["a:", "a2:a", "b:"]) + url = _sqlite( + tmp_path / "app.db", + "CREATE TABLE alembic_version (version_num VARCHAR(32))", + "INSERT INTO alembic_version VALUES ('a'), ('b')", + ) + + state = await read_migration_state(url, ini) + + assert state is not None + assert state.heads == {"a2", "b"} + assert state.current == {"a", "b"} + assert "alembic_version" in state.tables From ca3c87f3194ed13777ddd7ce19fac6b7ed84701b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:48:21 +0100 Subject: [PATCH 06/15] feat(doctor-migration-checks): doctor reports SM010/SM011 from SM_DATABASE_URL Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/__main__.py | 2 + .../simple_module_core/_migration_doctor.py | 54 ++++++++++ framework/db/tests/_alembic_project.py | 46 +++++++++ .../db/tests/test_db_migration_doctor.py | 99 +++++++++++++++++++ framework/db/tests/test_db_migration_state.py | 49 +-------- 5 files changed, 204 insertions(+), 46 deletions(-) create mode 100644 framework/core/simple_module_core/_migration_doctor.py create mode 100644 framework/db/tests/_alembic_project.py create mode 100644 framework/db/tests/test_db_migration_doctor.py diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 335f31ba..b03494bf 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -25,6 +25,7 @@ from pathlib import Path from simple_module_core._data_doctor import data_compatibility_diagnostics +from simple_module_core._migration_doctor import migration_diagnostics from simple_module_core.diagnostics import ( Diagnostic, DiagnosticLevel, @@ -188,6 +189,7 @@ def main() -> int: database_dialect=_database_dialect(), ) diagnostics.extend(override_findings) + diagnostics.extend(migration_diagnostics(modules, _database_url(), find_env_file().parent)) diagnostics.extend(data_compatibility_diagnostics(modules, _database_url())) # A missing dependency only warns at a dev boot, but production refuses # to start with it (GH #501), so doctor reports it as what it will be. diff --git a/framework/core/simple_module_core/_migration_doctor.py b/framework/core/simple_module_core/_migration_doctor.py new file mode 100644 index 00000000..4c25afe6 --- /dev/null +++ b/framework/core/simple_module_core/_migration_doctor.py @@ -0,0 +1,54 @@ +"""Doctor's migration checks (SM010, SM011) against ``SM_DATABASE_URL`` (#895).""" + +import asyncio +import importlib.util +from collections.abc import Sequence +from pathlib import Path + +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_core.diagnostics._migration import MigrationDiagnostics, module_table_names +from simple_module_core.module import ModuleBase + + +def _alembic_ini(project_root: Path) -> Path | None: + """``alembic.ini`` at the project root, else ``host/alembic.ini``, as the CLI resolves it.""" + for candidate in (project_root / "alembic.ini", project_root / "host" / "alembic.ini"): + if candidate.is_file(): + return candidate + return None + + +def migration_diagnostics( + modules: Sequence[ModuleBase], database_url: str, project_root: Path +) -> list[Diagnostic]: + # Core-only installations have neither Alembic nor a database runtime. + if importlib.util.find_spec("simple_module_db") is None: + return [] + ini = _alembic_ini(project_root) + if ini is None: + return [] + from simple_module_db.migration_state import read_migration_state + + try: + state = asyncio.run(read_migration_state(database_url, ini)) + except Exception: + # Unreachable is a warning, as for SM033: doctor also runs where no + # database is up. The error text can carry credentials, so it is not shown. + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code="SM010", + message="Could not read the migration state from the database; " + "SM010 and SM011 were not checked", + module_name="migrations", + suggestion="Check SM_DATABASE_URL and that the database is running.", + ) + ] + if state is None: + return [] + checks = MigrationDiagnostics() + behind = checks.check_heads(state.current, state.heads) + if behind: + # Behind head, a missing table may simply be in a pending revision. + return behind + return checks.check_table_coverage(module_table_names(modules), set(state.tables)) diff --git a/framework/db/tests/_alembic_project.py b/framework/db/tests/_alembic_project.py new file mode 100644 index 00000000..caae27b3 --- /dev/null +++ b/framework/db/tests/_alembic_project.py @@ -0,0 +1,46 @@ +"""A throwaway Alembic project and SQLite database for the migration-state tests.""" + +from pathlib import Path + +import sqlalchemy as sa + +_REVISION = """revision = "{rev}" +down_revision = {down} +branch_labels = None +depends_on = None + + +def upgrade(): + pass + + +def downgrade(): + pass +""" + + +def write_alembic_project(root: Path, chain: list[str]) -> Path: + """An ``alembic.ini`` over the ``"rev:down_revision"`` entries in *chain*. + + ``down_revision`` is empty for a root, so a test can build several heads + without a real ``env.py``. + """ + versions = root / "migrations" / "versions" + versions.mkdir(parents=True) + for entry in chain: + rev, _, down = entry.partition(":") + (versions / f"{rev}.py").write_text( + _REVISION.format(rev=rev, down=repr(down) if down else "None") + ) + ini = root / "alembic.ini" + ini.write_text("[alembic]\nscript_location = %(here)s/migrations\n") + return ini + + +def sqlite_database(path: Path, *statements: str) -> str: + engine = sa.create_engine(f"sqlite:///{path}") + with engine.begin() as conn: + for statement in statements: + conn.execute(sa.text(statement)) + engine.dispose() + return f"sqlite+aiosqlite:///{path}" diff --git a/framework/db/tests/test_db_migration_doctor.py b/framework/db/tests/test_db_migration_doctor.py new file mode 100644 index 00000000..f593309d --- /dev/null +++ b/framework/db/tests/test_db_migration_doctor.py @@ -0,0 +1,99 @@ +"""``make doctor`` reports SM010 and SM011 from the database it points at (#895).""" + +from __future__ import annotations + +import pytest +from _alembic_project import sqlite_database, write_alembic_project +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_core import __main__ as doctor +from simple_module_core import _migration_doctor + +_STAMP = "CREATE TABLE alembic_version (version_num VARCHAR(32))" + + +class _Module(ModuleBase): + meta = ModuleMeta(name="Orders") + + +@pytest.fixture +def run_doctor(tmp_path, monkeypatch, capsys): + """Run ``main()`` over one module declaring ``orders_order`` and ``orders_line``.""" + monkeypatch.setattr(doctor, "discover_modules", lambda strict: [_Module()]) + monkeypatch.setattr(doctor, "find_env_file", lambda: tmp_path / ".env") + monkeypatch.setattr(doctor, "parse_dotenv", dict) + monkeypatch.setattr(doctor, "run_diagnostics", lambda *a, **kw: []) + monkeypatch.setattr( + _migration_doctor, "module_table_names", lambda modules: {"orders_order", "orders_line"} + ) + + def run(url: str) -> tuple[int, str]: + monkeypatch.setenv("SM_DATABASE_URL", url) + code = doctor.main() + return code, capsys.readouterr().err + + return run + + +def test_db_doctor_reports_a_table_no_migration_creates(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:", "b:"]) + url = sqlite_database( + tmp_path / "app.db", + _STAMP, + "INSERT INTO alembic_version VALUES ('a'), ('b')", + "CREATE TABLE orders_order (id INTEGER)", + ) + + code, err = run_doctor(url) + + assert code == 0 + assert "SM011" in err + assert "orders_line" in err + assert "orders_order" not in err + assert "SM010" not in err + + +def test_db_doctor_fails_on_a_database_behind_head(tmp_path, run_doctor): + # The CLI's fallback location: host/alembic.ini under the project root. + write_alembic_project(tmp_path / "host", ["a:", "a2:a", "b:"]) + url = sqlite_database( + tmp_path / "app.db", _STAMP, "INSERT INTO alembic_version VALUES ('a'), ('b')" + ) + + code, err = run_doctor(url) + + assert code == 1 + assert "SM010" in err + assert "missing heads 'a2'" in err + # Behind head the table may only be in a pending revision. + assert "SM011" not in err + + +def test_db_doctor_warns_on_a_missing_database(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + + code, err = run_doctor(f"sqlite+aiosqlite:///{path}") + + assert code == 0 + assert "SM010" in err + assert "never been migrated" in err + assert not path.exists() + + +def test_db_doctor_warns_on_an_unreadable_database(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:"]) + + # A directory exists but cannot be opened as a SQLite database. + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path}") + + assert code == 0 + assert "SM010" in err + assert "could not read the migration state" in err.lower() + + +def test_db_doctor_skips_without_an_alembic_project(tmp_path, run_doctor): + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path / 'app.db'}") + + assert code == 0 + assert "SM010" not in err + assert "SM011" not in err diff --git a/framework/db/tests/test_db_migration_state.py b/framework/db/tests/test_db_migration_state.py index 4dc888c6..ef452395 100644 --- a/framework/db/tests/test_db_migration_state.py +++ b/framework/db/tests/test_db_migration_state.py @@ -1,51 +1,8 @@ """Doctor reads the migration state from the database it points at (#895).""" -from pathlib import Path - -import sqlalchemy as sa +from _alembic_project import sqlite_database, write_alembic_project from simple_module_db.migration_state import read_migration_state -_REVISION = """revision = "{rev}" -down_revision = {down} -branch_labels = None -depends_on = None - - -def upgrade(): - pass - - -def downgrade(): - pass -""" - - -def write_alembic_project(root: Path, chain: list[str]) -> Path: - """An ``alembic.ini`` over the ``"rev:down_revision"`` entries in *chain*. - - ``down_revision`` is empty for a root, so a test can build several heads - without a real ``env.py``. - """ - versions = root / "migrations" / "versions" - versions.mkdir(parents=True) - for entry in chain: - rev, _, down = entry.partition(":") - (versions / f"{rev}.py").write_text( - _REVISION.format(rev=rev, down=repr(down) if down else "None") - ) - ini = root / "alembic.ini" - ini.write_text("[alembic]\nscript_location = %(here)s/migrations\n") - return ini - - -def _sqlite(path: Path, *statements: str) -> str: - engine = sa.create_engine(f"sqlite:///{path}") - with engine.begin() as conn: - for statement in statements: - conn.execute(sa.text(statement)) - engine.dispose() - return f"sqlite+aiosqlite:///{path}" - async def test_db_migration_state_none_without_an_ini(tmp_path): assert await read_migration_state("sqlite+aiosqlite://", tmp_path / "alembic.ini") is None @@ -66,7 +23,7 @@ async def test_db_migration_state_missing_file_is_not_created(tmp_path): async def test_db_migration_state_reads_an_unstamped_database(tmp_path): ini = write_alembic_project(tmp_path, ["a:"]) - url = _sqlite(tmp_path / "app.db", "CREATE TABLE orders_order (id INTEGER)") + url = sqlite_database(tmp_path / "app.db", "CREATE TABLE orders_order (id INTEGER)") state = await read_migration_state(url, ini) @@ -77,7 +34,7 @@ async def test_db_migration_state_reads_an_unstamped_database(tmp_path): async def test_db_migration_state_reads_current_heads(tmp_path): ini = write_alembic_project(tmp_path, ["a:", "a2:a", "b:"]) - url = _sqlite( + url = sqlite_database( tmp_path / "app.db", "CREATE TABLE alembic_version (version_num VARCHAR(32))", "INSERT INTO alembic_version VALUES ('a'), ('b')", From be2111ca1e26802755ccdbd1fc04ac0f2c142c94 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:49:02 +0100 Subject: [PATCH 07/15] feat(doctor-migration-checks): dev boot reports SM011 and keeps it for rerun Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_hosting/_lifespan.py | 2 + .../simple_module_hosting/_table_coverage.py | 36 +++++++++ .../tests/test_hosting_table_coverage.py | 73 +++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 framework/hosting/simple_module_hosting/_table_coverage.py create mode 100644 framework/hosting/tests/test_hosting_table_coverage.py diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index ef3c0ca5..8bbf4984 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -22,6 +22,7 @@ from simple_module_hosting._deferred_startup import hydrate_settings_from_db from simple_module_hosting._route_guard import guard_included_routers +from simple_module_hosting._table_coverage import add_table_coverage from simple_module_hosting.data_compatibility import check_data_compatibility from simple_module_hosting.migrations import migration_status from simple_module_hosting.setup_gate import STEP_MIGRATIONS @@ -89,6 +90,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: migration = app.state.migration if migration["is_current"] and migration.get("current_revision") is not None: await check_data_compatibility(app, modules) + await add_table_coverage(app, modules) else: app.state.deferred_data_compatibility = list(modules) diff --git a/framework/hosting/simple_module_hosting/_table_coverage.py b/framework/hosting/simple_module_hosting/_table_coverage.py new file mode 100644 index 00000000..05e68e77 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_table_coverage.py @@ -0,0 +1,36 @@ +"""SM011 at a development boot, once the database is open and at head (#895).""" + +from __future__ import annotations + +from collections.abc import Sequence + +from fastapi import FastAPI +from simple_module_core.diagnostics import Diagnostic, print_diagnostics +from simple_module_core.diagnostics._migration import MigrationDiagnostics, module_table_names +from simple_module_core.module import ModuleBase +from sqlalchemy import inspect + + +async def add_table_coverage(app: FastAPI, modules: Sequence[ModuleBase]) -> None: + """Report module tables the migrated database lacks, and keep them for the Doctor screen. + + The boot diagnostics run while the app is built, before the database is + open, so this half runs from the lifespan. Only development keeps a runner; + wrapping it lets the Doctor screen's ``rerun()`` show the findings again. + Call it only for a database at every head: behind head, a missing table may + simply be in a pending revision. + """ + state = getattr(app.state.sm, "diagnostics", None) + runner = state.runner if state is not None else None + if state is None or runner is None: + return + async with app.state.sm.db.engine.connect() as conn: + tables = set(await conn.run_sync(lambda sync: inspect(sync).get_table_names())) + found: list[Diagnostic] = MigrationDiagnostics().check_table_coverage( + module_table_names(modules), tables + ) + if not found: + return + print_diagnostics(found) + state.runner = lambda: [*runner(), *found] + state.results = [*state.results, *found] diff --git a/framework/hosting/tests/test_hosting_table_coverage.py b/framework/hosting/tests/test_hosting_table_coverage.py new file mode 100644 index 00000000..fa770a86 --- /dev/null +++ b/framework/hosting/tests/test_hosting_table_coverage.py @@ -0,0 +1,73 @@ +"""A development boot reports SM011 once the database is open (#895).""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_core.services import DiagnosticsState +from simple_module_hosting import _table_coverage +from simple_module_hosting._table_coverage import add_table_coverage +from sqlalchemy.ext.asyncio import create_async_engine + +_BOOT = Diagnostic( + level=DiagnosticLevel.WARNING, code="SM019", message="boot finding", module_name="Demo" +) + + +@pytest.fixture +async def engine(): + engine = create_async_engine("sqlite+aiosqlite://") + async with engine.begin() as conn: + await conn.execute(sa.text("CREATE TABLE orders_order (id INTEGER)")) + yield engine + await engine.dispose() + + +def _app(engine, state: DiagnosticsState) -> SimpleNamespace: + return SimpleNamespace( + state=SimpleNamespace( + sm=SimpleNamespace(diagnostics=state, db=SimpleNamespace(engine=engine)) + ) + ) + + +@pytest.fixture(autouse=True) +def _module_tables(monkeypatch): + monkeypatch.setattr( + _table_coverage, "module_table_names", lambda modules: {"orders_order", "orders_line"} + ) + + +async def test_hosting_table_coverage_survives_rerun(engine, capsys): + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(engine, state), []) + + assert [d.code for d in state.results] == ["SM019", "SM011"] + assert "orders_line" in state.results[1].message + assert "SM011" in capsys.readouterr().err + assert [d.code for d in state.rerun()] == ["SM019", "SM011"] + + +async def test_hosting_table_coverage_is_quiet_when_covered(engine, monkeypatch): + monkeypatch.setattr(_table_coverage, "module_table_names", lambda modules: {"orders_order"}) + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(engine, state), []) + + assert [d.code for d in state.rerun()] == ["SM019"] + + +async def test_hosting_table_coverage_skips_without_a_runner(engine): + # Outside development no diagnostics run, so the database is not inspected. + state = DiagnosticsState() + + await add_table_coverage(_app(engine, state), []) + + assert state.results == [] + assert state.supported is False From 2df404e91aaa85f283cd72cca68f503cbd41bb82 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:49:36 +0100 Subject: [PATCH 08/15] docs(doctor-migration-checks): SM010/SM011 now run in doctor and dev boot Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 1 + docs/database/migrations.md | 4 ++-- docs/reference/diagnostic-codes.md | 8 ++++---- skills/simple-module-doctor/SKILL.md | 6 +++--- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4c5631d..f1918558 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed +- `make doctor` (and `smpy doctor`) now runs the migration checks it documents. It reads `alembic_version` and the table list from `SM_DATABASE_URL` and the heads from the project's `alembic.ini`: `SM010` is an error when the database lacks any head and a warning when it was never migrated or could not be read, and `SM011` reports a module table that a database at every head does not have. A development boot reports `SM011` too once its database is open, and the Doctor screen keeps it on re-run. Before, neither code was ever reported. **Behaviour change:** `make doctor` now exits 1 against a dev database that is behind head; run `make migrate` (#895). - `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). diff --git a/docs/database/migrations.md b/docs/database/migrations.md index d08a1d65..14000f23 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -213,13 +213,13 @@ Fix locally with `make migrate`. In production, run migrations before rolling ov ## Diagnostic: `SM011` -Fires as a warning when a module's model declares a table that doesn't appear in any Alembic migration. Typical causes: +Fires as a warning when a database at every head lacks a table a module's `models` declare, so no migration creates it. `make doctor` checks the database `SM_DATABASE_URL` points at; a development boot checks its own database once it is open. Typical causes: - You added a model but haven't generated a migration for it yet. - You renamed a table but the old migration still references the old name. - You used `__abstract__ = True` somewhere it shouldn't be. -The dev-mode boot log prints the offending table names. Resolution: run `make migration msg="..."`, review, `make migrate`. +`make doctor` and the dev-mode boot log print the offending table names; while the database is behind head the check is skipped, since the table may be in a pending revision. Resolution: run `make migration msg="..."`, review, `make migrate`. ## Cross-module foreign keys diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index 8d176223..c32e9b22 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -18,8 +18,8 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM007` | INFO | Module overrides zero `register_*` hooks and has no `on_startup`/`on_shutdown`. | Delete the module if it's vestigial; otherwise ignore. | | `SM008` | ERROR | Two modules declare the same `ModuleMeta.name`. Table-prefix collision. | Rename one. Remember: the name is the table-name prefix (all modules share one DB schema) and the Inertia namespace. | | `SM009` | ERROR | A file under `framework/*` imports from any package under `modules/*`, including dynamic imports with literal names or fixed f-string package prefixes. | Invert the dependency: have the module register a callback into the framework (see `principal_serializer` pattern). | -| `SM010` | ERROR | DB `alembic_version` is behind the migration head. | Run `make migrate`. The lifespan migration check raises and aborts boot in every environment before serving. | -| `SM011` | WARNING | A module's model declares a table that doesn't appear in any Alembic migration. | Run `uv run alembic revision --autogenerate -m "..."`, review, `make migrate`. | +| `SM010` | ERROR or WARNING | `make doctor` reads `alembic_version` from `SM_DATABASE_URL` and the heads from the project's `alembic.ini` (else `host/alembic.ini`). ERROR when the database lacks any head; WARNING when it was never migrated (a production boot sends that to `/setup`) or could not be read, in which case SM011 is skipped too. A SQLite file that does not exist yet counts as never migrated and is not created. | Run `make migrate`. The lifespan migration check also refuses a behind-head database at boot in every environment. | +| `SM011` | WARNING | A table in a module's `models` is missing from a database at every head, so no migration creates it. Reported by `make doctor` and, once the database is open, a development boot (also on the Doctor screen). Not checked while the database is behind head. | Generate a revision on the module's branch (`make migration LABEL= msg="..."` in this repo, `make migration msg="..."` in a scaffolded app), review it, `make migrate`. | | `SM012` | WARNING | Module overrides `register_settings` but does not assign to `app.state.`. Dev-only. | Either remove the override or move your state assignment into it. | | `SM013` | WARNING | Supported locale has no corresponding file in some module's `locales/`. | Add `modules///locales/.json`, or drop the locale from `SM_I18N_SUPPORTED_LOCALES`. | | `SM014` | WARNING | Non-default locale is missing keys present in the default (untranslated). | Translate the missing keys. | @@ -50,7 +50,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | Context | What runs | |---|---| -| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. After the routes are wired, SM031 (unguarded `/api` routes) is added. | +| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. After the routes are wired, SM031 (unguarded `/api` routes) is added; once the database is open and at head, SM011. | | App boot in non-development | Strict module discovery (raises on SM001-class failures) + the migration check (SM010) + SM025 (logged as a warning). The page/locale static suite is dev-only. | Sample dev-mode output: @@ -60,7 +60,7 @@ running diagnostics … WARNING SM003 modules/orders/orders/pages/Unused.tsx: no inertia.render("Orders/Unused") call found WARNING SM013 locale "es" missing file for module "orders" (expected modules/orders/orders/locales/es.json) -ERROR SM010 DB revision 3a1b2c3d4e5f is behind migration head 9f8e7d6c5b4a — run `make migrate` +ERROR SM010 Database is behind: missing heads '9f8e7d6c5b4a' — run `make migrate` 1 error, 2 warnings ``` diff --git a/skills/simple-module-doctor/SKILL.md b/skills/simple-module-doctor/SKILL.md index 8a3dfe44..3a9cf6e0 100644 --- a/skills/simple-module-doctor/SKILL.md +++ b/skills/simple-module-doctor/SKILL.md @@ -9,7 +9,7 @@ description: Use when interpreting a simple_module_python diagnostic code (SM001 The same diagnostics also run at host boot, but **only in development** (`SM_ENVIRONMENT == development`) — they print to the boot logs and `raise SystemExit` if there's any ERROR. **In production the diagnostics pass is skipped**; what protects prod is that module *discovery* runs strictly (any bad entry point — broken import, missing `meta`, non-`ModuleBase` class — or a missing `depends_on` target raises and fails the boot). So "errors fail prod boot" is true for discovery-level breakage (surfaced as **SM001**), not for the per-finding checks below, which you'll catch via `make doctor` / dev boot before you ever deploy. -You can also call `run_diagnostics(...)` from `simple_module_core` programmatically (e.g. in a CI job) to surface issues without booting the full app — pass `migration_state=` / `module_tables=` / `migrated_tables=` to additionally get **SM010/SM011** (the migration checks below), and `i18n_supported_locales=` / `i18n_default_locale=` to get the locale checks. +You can also call `run_diagnostics(...)` from `simple_module_core` programmatically (e.g. in a CI job) to surface issues without booting the full app — pass `migration_state=` / `module_tables=` / `migrated_tables=` to additionally get **SM010/SM011** (`make doctor` reads those from the database itself), and `i18n_supported_locales=` / `i18n_default_locale=` to get the locale checks. ## Code reference @@ -21,8 +21,8 @@ You can also call `run_diagnostics(...)` from `simple_module_core` programmatica | **SM007** | INFO | Module overrides no `register_*` hooks at all — likely scaffolded but empty | Either implement at least one hook or delete the module if unused | | **SM008** | ERROR | Two modules declare the same `ModuleMeta.name`, or two names collide once lowercased as table prefixes in the shared schema (one finding per collision, from either the duplicate-name or the schema-conflict check) | Rename one module's `meta.name` | | **SM009** | ERROR | A `framework/*` package directly imports from a plugin module (`modules/*`) | Move the symbol *up* into `simple_module_core` / `simple_module_hosting`, or invert the dependency via the event bus / a registry | -| **SM010** | ERROR | Live DB revision is behind the migration head | Run `make migrate` before booting; in CI, ensure migrations are part of the deploy step. *Note:* only emitted when a caller passes `migration_state=` to `run_diagnostics` — `make doctor` and dev boot do **not** check this. The live boot enforces it separately: `check_migrations()` raises a plain `RuntimeError` (not an SM code) in dev and prod if the DB is behind head | -| **SM011** | WARNING | A module declares a SQLModel table that has no entry in migration history | Run `make migration msg="..."` and apply. *Note:* only emitted when `module_tables=`/`migrated_tables=` are passed to `run_diagnostics`; not surfaced by `make doctor` or boot | +| **SM010** | ERROR / WARNING | `make doctor` compares `alembic_version` in `SM_DATABASE_URL` with the heads in `alembic.ini` (else `host/alembic.ini`): ERROR when any head is missing, WARNING when the DB was never migrated or could not be read | Run `make migrate` before booting; in CI, ensure migrations are part of the deploy step. The live boot enforces it separately: the lifespan raises a plain `RuntimeError` in dev and prod if the DB is behind head (a never-migrated first run goes to `/setup`) | +| **SM011** | WARNING | A table in a module's `models` is missing from a database at every head, so no migration creates it. Reported by `make doctor` and by a development boot once the database is open; skipped while the DB is behind head | Run `make migration msg="..."`, review the revision, apply | | **SM012** | WARNING (dev only) | `register_settings` is overridden but nothing was added to `app.state.` | Either store the module's settings dataclass on `app.state.` or remove the empty override | | **SM013** | WARNING | A locale file declared in `locale_dirs()` is missing for one of the supported locales | Create the file (even if empty) or trim `SM_I18N_SUPPORTED_LOCALES` | | **SM014** | WARNING | A non-default locale is missing keys present in the default locale | Add the missing keys, or accept that a fallback to default applies | From 2ebbbf51f124249c9c4dd55344342271ed617bb4 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:50:43 +0100 Subject: [PATCH 09/15] fix(doctor-migration-checks): SM011 message says what is checked Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/diagnostics/_migration.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/framework/core/simple_module_core/diagnostics/_migration.py b/framework/core/simple_module_core/diagnostics/_migration.py index 2aa50315..d6eec29d 100644 --- a/framework/core/simple_module_core/diagnostics/_migration.py +++ b/framework/core/simple_module_core/diagnostics/_migration.py @@ -79,13 +79,16 @@ def check_table_coverage( module_tables: set[str], migrated_tables: set[str], ) -> list[Diagnostic]: - """SM011: Warning if module tables are missing from migration history.""" + """SM011: Warning for module tables a database at every head does not have.""" missing = module_tables - migrated_tables return [ Diagnostic( level=DiagnosticLevel.WARNING, code="SM011", - message=f"Table '{table}' declared in models but not found in migration history", + message=( + f"Table '{table}' is declared in models but missing from the migrated " + "database, so no migration creates it" + ), module_name=_MODULE_NAME, suggestion=f'Run: make migration msg="add {table}"', ) From 89baa177f68cf5e67c99895b5da579e097302878 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 15:52:25 +0100 Subject: [PATCH 10/15] style(doctor-migration-checks): sort imports Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/tests/test_db_migration_doctor.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/framework/db/tests/test_db_migration_doctor.py b/framework/db/tests/test_db_migration_doctor.py index f593309d..9cc14419 100644 --- a/framework/db/tests/test_db_migration_doctor.py +++ b/framework/db/tests/test_db_migration_doctor.py @@ -4,9 +4,8 @@ import pytest from _alembic_project import sqlite_database, write_alembic_project -from simple_module_core import ModuleBase, ModuleMeta +from simple_module_core import ModuleBase, ModuleMeta, _migration_doctor from simple_module_core import __main__ as doctor -from simple_module_core import _migration_doctor _STAMP = "CREATE TABLE alembic_version (version_num VARCHAR(32))" From 663bcd6c8a541911d45f4522be55589c20a1ac59 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 16:00:33 +0100 Subject: [PATCH 11/15] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/database/migrations.md | 4 ++-- docs/database/per-module-base.md | 2 +- docs/framework-conventions.md | 4 ++-- framework/core/simple_module_core/diagnostics/_migration.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/database/migrations.md b/docs/database/migrations.md index 14000f23..2410b90a 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -206,8 +206,8 @@ context.configure( On startup, the framework compares `alembic_version` to **every** head (`resolve_head_revisions` in `simple_module_hosting.migrations`; the history has several, so it never asks for a single head): -- **Development:** prints a warning if the DB is behind. Result stored on `app.state.migration`. -- **Production:** fails boot with `SM010`. Don't ship a web process that's pointed at an unmigrated DB. +- **Every environment:** a database behind head fails the boot (`Database is N revision(s) behind … Run: make migrate`), except a never-migrated first run, which boots far enough to serve the `/setup` wizard. The result is stored on `app.state.migration`. +- `make doctor` reports the same state as `SM010` before you boot. Don't ship a web process that's pointed at an unmigrated DB. Fix locally with `make migrate`. In production, run migrations before rolling over the web tier. diff --git a/docs/database/per-module-base.md b/docs/database/per-module-base.md index 6fd7c052..b153af8d 100644 --- a/docs/database/per-module-base.md +++ b/docs/database/per-module-base.md @@ -86,4 +86,4 @@ for t in meta.sorted_tables: print(t.name) ``` -This is also what the boot-time `SM011` check uses — it compares this set against the Alembic history to detect tables that exist in code but not in any migration. +This is also what the boot-time `SM011` check uses — it compares this set against the migrated database's tables to detect tables that exist in code but that no migration creates. diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 1a22e989..943b3ef0 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -626,8 +626,8 @@ Dispatch keys on the **exact** event class (`module.qualname`), not its MRO — | SM007 | INFO | Module defines no `register_*` hooks | | SM008 | ERROR | Duplicate module name / schema prefix conflict | | SM009 | ERROR | Framework package directly imports from a plugin module | -| SM010 | ERROR | DB revision behind migration head | -| SM011 | WARNING | Module table not in migration history | +| SM010 | ERROR or WARNING | DB lacks a migration head (WARNING: never migrated) | +| SM011 | WARNING | Module table missing from a database at head | | SM012 | WARNING | `register_settings` overridden but nothing added to `app.state` | | SM013 | WARNING | Locale file missing for a supported locale | | SM014 | WARNING | Non-default locale missing keys present in the default | diff --git a/framework/core/simple_module_core/diagnostics/_migration.py b/framework/core/simple_module_core/diagnostics/_migration.py index d6eec29d..9e470bdf 100644 --- a/framework/core/simple_module_core/diagnostics/_migration.py +++ b/framework/core/simple_module_core/diagnostics/_migration.py @@ -31,7 +31,7 @@ def check_heads(self, current: Set[str], heads: Set[str]) -> list[Diagnostic]: ``/setup`` rather than refusing it. One behind on any branch is an ERROR, because a production boot refuses it. """ - missing = set(heads) - set(current) + missing = heads - current if not missing: return [] if not current: From 6678479164eb9222c775f0be88621e249cdde320 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 16:07:52 +0100 Subject: [PATCH 12/15] fix: address code review findings (round 1, Codex) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 +- framework/core/simple_module_core/__main__.py | 11 +++++++--- framework/core/simple_module_core/dotenv.py | 20 ++++++++++++++++++ .../db/simple_module_db/migration_state.py | 8 +++++-- .../db/tests/test_db_migration_doctor.py | 16 ++++++++++++++ framework/db/tests/test_db_migration_state.py | 15 +++++++++++++ .../_deferred_startup.py | 2 ++ .../bootstrap_settings.py | 21 +------------------ .../tests/test_hosting_table_coverage.py | 20 ++++++++++++++++++ 9 files changed, 89 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1918558..59414d47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed -- `make doctor` (and `smpy doctor`) now runs the migration checks it documents. It reads `alembic_version` and the table list from `SM_DATABASE_URL` and the heads from the project's `alembic.ini`: `SM010` is an error when the database lacks any head and a warning when it was never migrated or could not be read, and `SM011` reports a module table that a database at every head does not have. A development boot reports `SM011` too once its database is open, and the Doctor screen keeps it on re-run. Before, neither code was ever reported. **Behaviour change:** `make doctor` now exits 1 against a dev database that is behind head; run `make migrate` (#895). +- `make doctor` (and `smpy doctor`) now runs the migration checks it documents. It reads `alembic_version` and the table list from `SM_DATABASE_URL` and the heads from the project's `alembic.ini`: `SM010` is an error when the database lacks any head and a warning when it was never migrated or could not be read, and `SM011` reports a module table that a database at every head does not have. A development boot reports `SM011` too once its database is at head (or once the setup wizard has migrated it), and the Doctor screen keeps it on re-run. Doctor now resolves a relative SQLite `SM_DATABASE_URL` against the project root as the app does, so run from `host/` it reads the app's database for `SM033` too. Before, neither code was ever reported. **Behaviour change:** `make doctor` now exits 1 against a dev database that is behind head; run `make migrate` (#895). - `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index b03494bf..94e008b5 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -40,7 +40,7 @@ select_auth_provider, topological_sort, ) -from simple_module_core.dotenv import find_env_file, parse_dotenv +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file, parse_dotenv from simple_module_core.exceptions import InvalidModuleError @@ -78,8 +78,13 @@ def _load_i18n_settings_from_env() -> tuple[list[str] | None, str]: def _database_url() -> str: - """``SM_DATABASE_URL``, defaulting like ``Settings.database_url``.""" - return os.environ.get("SM_DATABASE_URL", "sqlite+aiosqlite:///./app.db") + """``SM_DATABASE_URL``, defaulting and anchored like ``Settings.database_url``. + + A relative SQLite path is resolved against the project root, as the app + resolves it, so doctor run from ``host/`` reads the app's database. + """ + url = os.environ.get("SM_DATABASE_URL", "sqlite+aiosqlite:///./app.db") + return absolutize_sqlite_url(url, anchor=find_env_file().parent) def _database_dialect() -> str: diff --git a/framework/core/simple_module_core/dotenv.py b/framework/core/simple_module_core/dotenv.py index 3f228c1f..591d5aae 100644 --- a/framework/core/simple_module_core/dotenv.py +++ b/framework/core/simple_module_core/dotenv.py @@ -139,3 +139,23 @@ def env_bool(name: str, default: bool = False) -> bool: if raw in BOOL_LITERALS_FALSE: return False return default + + +def absolutize_sqlite_url(url: str, *, anchor: Path) -> str: + """Rewrite a relative sqlite path to an absolute one under ``anchor``. + + ``sqlite+aiosqlite:///./host/app.db`` means "relative to the project + root" by convention, but SQLAlchemy resolves it against the process cwd + — correct in the web process (which chdirs) and silently wrong in every + CLI run from a subdirectory. Absolute paths (``:////...``), ``:memory:``, + SQLite URI-mode paths (``file:...?uri=true``), and non-sqlite URLs pass + through untouched. + """ + if not url.startswith("sqlite"): + return url + scheme, sep, rest = url.partition(":///") + if not sep or not rest or rest.startswith(("/", ":memory:", "file:")): + return url + path_part, query_sep, query = rest.partition("?") + resolved = (anchor / path_part).resolve() + return f"{scheme}:///{resolved}{query_sep}{query}" diff --git a/framework/db/simple_module_db/migration_state.py b/framework/db/simple_module_db/migration_state.py index 8ce0ad00..fa7357be 100644 --- a/framework/db/simple_module_db/migration_state.py +++ b/framework/db/simple_module_db/migration_state.py @@ -26,12 +26,16 @@ class MigrationState: def database_exists(database_url: str) -> bool: - """False for a SQLite file that isn't there yet; every other URL is tried.""" + """False for a SQLite file that isn't there yet; every other URL is tried. + + A URI-mode path (``file:...?uri=true``) is not a plain filesystem path, so + it is tried too rather than reported missing. + """ url = make_url(database_url) if not url.get_backend_name().startswith("sqlite"): return True database = url.database or "" - return database in ("", ":memory:") or Path(database).exists() + return database in ("", ":memory:") or database.startswith("file:") or Path(database).exists() def _heads(alembic_ini: Path) -> frozenset[str]: diff --git a/framework/db/tests/test_db_migration_doctor.py b/framework/db/tests/test_db_migration_doctor.py index 9cc14419..62e28612 100644 --- a/framework/db/tests/test_db_migration_doctor.py +++ b/framework/db/tests/test_db_migration_doctor.py @@ -96,3 +96,19 @@ def test_db_doctor_skips_without_an_alembic_project(tmp_path, run_doctor): assert code == 0 assert "SM010" not in err assert "SM011" not in err + + +def test_db_doctor_anchors_a_relative_sqlite_url_at_the_project_root( + tmp_path, monkeypatch, run_doctor +): + # The app resolves ./app.db against the project root, not the cwd; doctor must too. + write_alembic_project(tmp_path, ["a:"]) + sqlite_database(tmp_path / "app.db", _STAMP, "INSERT INTO alembic_version VALUES ('a')") + (tmp_path / "host").mkdir() + monkeypatch.chdir(tmp_path / "host") + + code, err = run_doctor("sqlite+aiosqlite:///./app.db") + + assert code == 0 + assert "never been migrated" not in err + assert not (tmp_path / "host" / "app.db").exists() diff --git a/framework/db/tests/test_db_migration_state.py b/framework/db/tests/test_db_migration_state.py index ef452395..2549ba45 100644 --- a/framework/db/tests/test_db_migration_state.py +++ b/framework/db/tests/test_db_migration_state.py @@ -46,3 +46,18 @@ async def test_db_migration_state_reads_current_heads(tmp_path): assert state.heads == {"a2", "b"} assert state.current == {"a", "b"} assert "alembic_version" in state.tables + + +async def test_db_migration_state_reads_a_uri_mode_database(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + sqlite_database( + path, + "CREATE TABLE alembic_version (version_num VARCHAR(32))", + "INSERT INTO alembic_version VALUES ('a')", + ) + + state = await read_migration_state(f"sqlite+aiosqlite:///file:{path}?mode=ro&uri=true", ini) + + assert state is not None + assert state.current == {"a"} diff --git a/framework/hosting/simple_module_hosting/_deferred_startup.py b/framework/hosting/simple_module_hosting/_deferred_startup.py index d2b67e2b..55022be3 100644 --- a/framework/hosting/simple_module_hosting/_deferred_startup.py +++ b/framework/hosting/simple_module_hosting/_deferred_startup.py @@ -11,6 +11,7 @@ from simple_module_db.stored_settings import SessionOverrideSource, settings_table_exists from simple_module_hosting._hydrate_step import hydrate_all +from simple_module_hosting._table_coverage import add_table_coverage from simple_module_hosting.data_compatibility import check_data_compatibility logger = logging.getLogger(__name__) @@ -52,6 +53,7 @@ async def run_deferred_startup(app: FastAPI) -> None: pending_checks = getattr(app.state, "deferred_data_compatibility", None) if pending_checks: await check_data_compatibility(app, pending_checks) + await add_table_coverage(app, pending_checks) app.state.deferred_data_compatibility = [] deferred = get_deferred_startup(app) if not deferred: diff --git a/framework/hosting/simple_module_hosting/bootstrap_settings.py b/framework/hosting/simple_module_hosting/bootstrap_settings.py index 8fc68a23..c6237bd3 100644 --- a/framework/hosting/simple_module_hosting/bootstrap_settings.py +++ b/framework/hosting/simple_module_hosting/bootstrap_settings.py @@ -22,32 +22,13 @@ from pydantic import model_validator from pydantic_settings import BaseSettings, SettingsConfigDict +from simple_module_core.dotenv import absolutize_sqlite_url as _absolutize_sqlite_url from simple_module_core.dotenv import find_env_file from simple_module_core.environments import NON_PROD_ENVIRONMENTS PLACEHOLDER_SECRET_KEY = "change-me-in-production" -def _absolutize_sqlite_url(url: str, *, anchor: Path) -> str: - """Rewrite a relative sqlite path to an absolute one under ``anchor``. - - ``sqlite+aiosqlite:///./host/app.db`` means "relative to the project - root" by convention, but SQLAlchemy resolves it against the process cwd - — correct in the web process (which chdirs) and silently wrong in every - CLI run from a subdirectory. Absolute paths (``:////...``), ``:memory:``, - SQLite URI-mode paths (``file:...?uri=true``), and non-sqlite URLs pass - through untouched. - """ - if not url.startswith("sqlite"): - return url - scheme, sep, rest = url.partition(":///") - if not sep or not rest or rest.startswith(("/", ":memory:", "file:")): - return url - path_part, query_sep, query = rest.partition("?") - resolved = (anchor / path_part).resolve() - return f"{scheme}:///{resolved}{query_sep}{query}" - - class BootstrapSettings(BaseSettings): """Pre-DB bootstrap environment knobs.""" diff --git a/framework/hosting/tests/test_hosting_table_coverage.py b/framework/hosting/tests/test_hosting_table_coverage.py index fa770a86..d6baa501 100644 --- a/framework/hosting/tests/test_hosting_table_coverage.py +++ b/framework/hosting/tests/test_hosting_table_coverage.py @@ -71,3 +71,23 @@ async def test_hosting_table_coverage_skips_without_a_runner(engine): assert state.results == [] assert state.supported is False + + +async def test_hosting_table_coverage_runs_after_the_wizard_migrates(engine, monkeypatch): + # A first run defers the check; replaying deferred startup reports it once at head. + from simple_module_hosting import _deferred_startup + + async def no_compat_issues(app, modules): + return None + + monkeypatch.setattr(_deferred_startup, "check_data_compatibility", no_compat_issues) + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + app = _app(engine, state) + app.state.deferred_data_compatibility = ["module"] + app.state.deferred_startup = [] + + await _deferred_startup.run_deferred_startup(app) + + assert [d.code for d in state.rerun()] == ["SM019", "SM011"] + assert app.state.deferred_data_compatibility == [] From 4a150d817f1c4c108e0231424388336a9d6c0b2e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 16:10:36 +0100 Subject: [PATCH 13/15] fix: address code review findings (round 1, pass 2) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/dotenv.py | 3 +++ .../db/simple_module_db/migration_state.py | 10 +++++++--- framework/db/tests/test_db_migration_state.py | 11 +++++++++++ .../simple_module_hosting/_table_coverage.py | 12 ++++++++++-- .../bootstrap_settings.py | 5 ++--- .../hosting/tests/test_sqlite_path_anchor.py | 19 +++++++++---------- 6 files changed, 42 insertions(+), 18 deletions(-) diff --git a/framework/core/simple_module_core/dotenv.py b/framework/core/simple_module_core/dotenv.py index 591d5aae..6da511f1 100644 --- a/framework/core/simple_module_core/dotenv.py +++ b/framework/core/simple_module_core/dotenv.py @@ -1,5 +1,8 @@ """Minimal ``.env`` parser + env-var helpers — dependency-free. +Also holds ``absolutize_sqlite_url``, which anchors a relative SQLite +``SM_DATABASE_URL`` at the project root for both the app and the doctor CLI. + Used in places that can't or shouldn't pull in ``pydantic-settings`` (the diagnostics CLI runs before the host package is imported; the users-module bootstrap runs after settings are constructed and needs to read values that diff --git a/framework/db/simple_module_db/migration_state.py b/framework/db/simple_module_db/migration_state.py index fa7357be..1710522e 100644 --- a/framework/db/simple_module_db/migration_state.py +++ b/framework/db/simple_module_db/migration_state.py @@ -28,14 +28,18 @@ class MigrationState: def database_exists(database_url: str) -> bool: """False for a SQLite file that isn't there yet; every other URL is tried. - A URI-mode path (``file:...?uri=true``) is not a plain filesystem path, so - it is tried too rather than reported missing. + A URI-mode path (``file:...?uri=true``) is checked by the file it names, + since opening a missing one in the default ``rwc`` mode would create it. """ url = make_url(database_url) if not url.get_backend_name().startswith("sqlite"): return True database = url.database or "" - return database in ("", ":memory:") or database.startswith("file:") or Path(database).exists() + if database.startswith("file:"): + if url.query.get("mode") == "memory": + return True + database = database.removeprefix("file:").split("?", 1)[0] + return database in ("", ":memory:") or Path(database).exists() def _heads(alembic_ini: Path) -> frozenset[str]: diff --git a/framework/db/tests/test_db_migration_state.py b/framework/db/tests/test_db_migration_state.py index 2549ba45..c45a7ea0 100644 --- a/framework/db/tests/test_db_migration_state.py +++ b/framework/db/tests/test_db_migration_state.py @@ -61,3 +61,14 @@ async def test_db_migration_state_reads_a_uri_mode_database(tmp_path): assert state is not None assert state.current == {"a"} + + +async def test_db_migration_state_missing_uri_mode_file_is_not_created(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + + state = await read_migration_state(f"sqlite+aiosqlite:///file:{path}?uri=true", ini) + + assert state is not None + assert state.current == set() + assert not path.exists() diff --git a/framework/hosting/simple_module_hosting/_table_coverage.py b/framework/hosting/simple_module_hosting/_table_coverage.py index 05e68e77..de920130 100644 --- a/framework/hosting/simple_module_hosting/_table_coverage.py +++ b/framework/hosting/simple_module_hosting/_table_coverage.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging from collections.abc import Sequence from fastapi import FastAPI @@ -10,6 +11,8 @@ from simple_module_core.module import ModuleBase from sqlalchemy import inspect +logger = logging.getLogger(__name__) + async def add_table_coverage(app: FastAPI, modules: Sequence[ModuleBase]) -> None: """Report module tables the migrated database lacks, and keep them for the Doctor screen. @@ -24,8 +27,13 @@ async def add_table_coverage(app: FastAPI, modules: Sequence[ModuleBase]) -> Non runner = state.runner if state is not None else None if state is None or runner is None: return - async with app.state.sm.db.engine.connect() as conn: - tables = set(await conn.run_sync(lambda sync: inspect(sync).get_table_names())) + try: + async with app.state.sm.db.engine.connect() as conn: + tables = set(await conn.run_sync(lambda sync: inspect(sync).get_table_names())) + except Exception: + # A diagnostics-only read must never fail the boot or the wizard's migrations action. + logger.warning("SM011 table coverage could not be read; skipped", exc_info=True) + return found: list[Diagnostic] = MigrationDiagnostics().check_table_coverage( module_table_names(modules), tables ) diff --git a/framework/hosting/simple_module_hosting/bootstrap_settings.py b/framework/hosting/simple_module_hosting/bootstrap_settings.py index c6237bd3..7b0e3242 100644 --- a/framework/hosting/simple_module_hosting/bootstrap_settings.py +++ b/framework/hosting/simple_module_hosting/bootstrap_settings.py @@ -22,8 +22,7 @@ from pydantic import model_validator from pydantic_settings import BaseSettings, SettingsConfigDict -from simple_module_core.dotenv import absolutize_sqlite_url as _absolutize_sqlite_url -from simple_module_core.dotenv import find_env_file +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file from simple_module_core.environments import NON_PROD_ENVIRONMENTS PLACEHOLDER_SECRET_KEY = "change-me-in-production" @@ -58,7 +57,7 @@ def __init__(self, **values: Any) -> None: anchor = ( Path(env_file).parent if isinstance(env_file, (str, Path)) else find_env_file().parent ) - self.database_url = _absolutize_sqlite_url(self.database_url, anchor=anchor) + self.database_url = absolutize_sqlite_url(self.database_url, anchor=anchor) database_url: str = "sqlite+aiosqlite:///./app.db" diff --git a/framework/hosting/tests/test_sqlite_path_anchor.py b/framework/hosting/tests/test_sqlite_path_anchor.py index 2f763a3c..26625998 100644 --- a/framework/hosting/tests/test_sqlite_path_anchor.py +++ b/framework/hosting/tests/test_sqlite_path_anchor.py @@ -19,42 +19,41 @@ from pathlib import Path import pytest -from simple_module_core.dotenv import find_env_file -from simple_module_hosting.bootstrap_settings import _absolutize_sqlite_url +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file class TestAbsolutize: def test_relative_path_resolves_against_anchor(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///./host/app.db", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///./host/app.db", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'host' / 'app.db'}" def test_bare_relative_path_resolves(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///app.db", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///app.db", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'app.db'}" def test_absolute_path_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite:////var/data/app.db" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_memory_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite:///:memory:" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_uri_mode_untouched(self, tmp_path: Path) -> None: """SQLite URI-mode paths (shared in-memory DBs) are not file paths.""" url = "sqlite+aiosqlite:///file:shared?mode=memory&cache=shared&uri=true" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_bare_scheme_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite://" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_postgres_untouched(self, tmp_path: Path) -> None: url = "postgresql+asyncpg://u:p@localhost/db" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_query_string_survives(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///app.db?mode=ro", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///app.db?mode=ro", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'app.db'}?mode=ro" From aa28d786ec44eb55a38337f6b523a63bf0c29d96 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 16:12:59 +0100 Subject: [PATCH 14/15] fix: address code review findings (round 1, pass 3) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/_migration_doctor.py | 8 +++++--- .../hosting/tests/test_hosting_table_coverage.py | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/framework/core/simple_module_core/_migration_doctor.py b/framework/core/simple_module_core/_migration_doctor.py index 4c25afe6..fe37e16c 100644 --- a/framework/core/simple_module_core/_migration_doctor.py +++ b/framework/core/simple_module_core/_migration_doctor.py @@ -33,15 +33,17 @@ def migration_diagnostics( state = asyncio.run(read_migration_state(database_url, ini)) except Exception: # Unreachable is a warning, as for SM033: doctor also runs where no - # database is up. The error text can carry credentials, so it is not shown. + # database is up. A revision file that fails to import lands here too. + # The error text can carry credentials, so it is not shown. return [ Diagnostic( level=DiagnosticLevel.WARNING, code="SM010", - message="Could not read the migration state from the database; " + message="Could not read the migration state (database or Alembic revisions); " "SM010 and SM011 were not checked", module_name="migrations", - suggestion="Check SM_DATABASE_URL and that the database is running.", + suggestion="Check SM_DATABASE_URL and that the database is running, and that " + "the revisions load (`make migration-history`).", ) ] if state is None: diff --git a/framework/hosting/tests/test_hosting_table_coverage.py b/framework/hosting/tests/test_hosting_table_coverage.py index d6baa501..95913f26 100644 --- a/framework/hosting/tests/test_hosting_table_coverage.py +++ b/framework/hosting/tests/test_hosting_table_coverage.py @@ -63,6 +63,21 @@ async def test_hosting_table_coverage_is_quiet_when_covered(engine, monkeypatch) assert [d.code for d in state.rerun()] == ["SM019"] +async def test_hosting_table_coverage_skips_when_the_database_cannot_be_read(caplog): + # A diagnostics-only read must not fail the boot or the wizard's migrations action. + class _BrokenEngine: + def connect(self): + raise RuntimeError("database is gone") + + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(_BrokenEngine(), state), []) + + assert [d.code for d in state.rerun()] == ["SM019"] + assert "SM011 table coverage could not be read" in caplog.text + + async def test_hosting_table_coverage_skips_without_a_runner(engine): # Outside development no diagnostics run, so the database is not inspected. state = DiagnosticsState() From 6768b14ca6fa9779d38eff089aff15b8db2393bb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 10 Nov 2026 16:17:22 +0100 Subject: [PATCH 15/15] fix: report an alembic.ini doctor cannot load (Codex final review) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/migration_state.py | 15 +++++++-------- framework/db/tests/test_db_migration_doctor.py | 9 +++++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/framework/db/simple_module_db/migration_state.py b/framework/db/simple_module_db/migration_state.py index 1710522e..53f60071 100644 --- a/framework/db/simple_module_db/migration_state.py +++ b/framework/db/simple_module_db/migration_state.py @@ -8,7 +8,6 @@ from alembic.config import Config from alembic.runtime.migration import MigrationContext from alembic.script import ScriptDirectory -from alembic.util.exc import CommandError from sqlalchemy import inspect from sqlalchemy.engine import make_url from sqlalchemy.ext.asyncio import create_async_engine @@ -43,21 +42,21 @@ def database_exists(database_url: str) -> bool: def _heads(alembic_ini: Path) -> frozenset[str]: + # An ini that exists but cannot be loaded (bad script_location, a broken + # revision) raises: the caller reports it rather than skipping silently. if not alembic_ini.is_file(): return frozenset() - try: - return frozenset(ScriptDirectory.from_config(Config(str(alembic_ini))).get_heads()) - except (CommandError, FileNotFoundError): - return frozenset() + return frozenset(ScriptDirectory.from_config(Config(str(alembic_ini))).get_heads()) async def read_migration_state(database_url: str, alembic_ini: str | Path) -> MigrationState | None: """The heads in *alembic_ini* beside the current revisions and tables of the database. ``None`` when there is no ``alembic.ini`` or it defines no revisions: there - is nothing to compare against. A SQLite file that does not exist reads as - empty rather than being opened, which would create it. Connection errors - propagate; their text can carry credentials, so callers must not echo it. + is nothing to compare against. An ini Alembic cannot load raises. A SQLite + file that does not exist reads as empty rather than being opened, which + would create it. Connection errors propagate; their text can carry + credentials, so callers must not echo it. """ heads = _heads(Path(alembic_ini)) if not heads: diff --git a/framework/db/tests/test_db_migration_doctor.py b/framework/db/tests/test_db_migration_doctor.py index 62e28612..14bdefb8 100644 --- a/framework/db/tests/test_db_migration_doctor.py +++ b/framework/db/tests/test_db_migration_doctor.py @@ -112,3 +112,12 @@ def test_db_doctor_anchors_a_relative_sqlite_url_at_the_project_root( assert code == 0 assert "never been migrated" not in err assert not (tmp_path / "host" / "app.db").exists() + + +def test_db_doctor_warns_on_a_broken_alembic_ini(tmp_path, run_doctor): + (tmp_path / "alembic.ini").write_text("[alembic]\nscript_location = %(here)s/missing\n") + + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path / 'app.db'}") + + assert code == 0 + assert "could not read the migration state" in err.lower()