Admin section: complete the sidebar, kill double-edits, real Doctor

.github/workflows/pr.yml

@@ -158,7 +158,10 @@ jobs:
       SM_USERS_BOOTSTRAP_PASSWORD: admin
       # Exclude Keycloak module — SM020 prevents both users and keycloak
       # from running simultaneously. E2E tests use the users module.
-      SM_MODULES_ENABLED: '["Auth","Users","Dashboard","Permissions","Settings","BackgroundTasks","FileStorage","FeatureFlags","AuditLog"]'
+      # Branding is included so the admin section's "edited on its own page,
+      # never in the generic module editor" rule is actually exercised — the
+      # server-side half of that is a 409 guard, which is worth CI coverage.
+      SM_MODULES_ENABLED: '["Auth","Users","Dashboard","Permissions","Settings","BackgroundTasks","FileStorage","FeatureFlags","AuditLog","Branding"]'
       E2E_BASE_URL: http://localhost:8000
     steps:
       - uses: actions/checkout@v7

framework/cli/tests/test_cli_package_update.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import importlib
 from pathlib import Path
 from urllib.error import HTTPError
 
@@ -156,17 +157,30 @@ def fetcher(url: str) -> dict:
     assert "simple_module_core>=2.0.0rc1" in pyproject.read_text(encoding="utf-8")
 
 
+def _exit_exception_types() -> tuple[type[BaseException], ...]:
+    """Every ``Exit`` class this typer build might raise.
+
+    typer's layout is not fixed by its version number: some 0.27.1 installs
+    vendor click as ``typer._click`` (whose ``Exit`` does not inherit from
+    click's) and ship no ``typer.exceptions``, while others ship
+    ``typer.exceptions`` and no ``typer._click``. Naming either path directly
+    is what made this test pass on one machine and fail on another, so probe
+    both and keep whatever exists.
+    """
+    found: list[type[BaseException]] = [click.exceptions.Exit]
+    for module_path in ("typer._click.exceptions", "typer.exceptions"):
+        try:
+            module = importlib.import_module(module_path)
+        except ImportError:
+            continue
+        exit_cls = getattr(module, "Exit", None)
+        if isinstance(exit_cls, type) and issubclass(exit_cls, BaseException):
+            found.append(exit_cls)
+    return tuple(found)
+
+
 def test_missing_pyproject_exits_nonzero(tmp_path: Path) -> None:
-    # typer >= 0.26 vendors click as ``typer._click``; the raised ``Exit``
-    # no longer inherits from ``click.exceptions.Exit``.  Catch both.
-    _exit_types: tuple[type[BaseException], ...] = (click.exceptions.Exit,)
-    try:
-        from typer._click.exceptions import Exit as _TyExit
-
-        _exit_types = (*_exit_types, _TyExit)
-    except ImportError:
-        pass
-    with pytest.raises(_exit_types) as exc:
+    with pytest.raises(_exit_exception_types()) as exc:
         pu.run_update(
             path=tmp_path,
             dry_run=False,

framework/core/simple_module_core/diagnostics/_pages.py

@@ -12,17 +12,62 @@
     from simple_module_core.module import ModuleBase
 
 
+def _assignment(s: ast.stmt) -> tuple[ast.Name, ast.expr] | None:
+    """Return ``(target, value)`` for a single-target top-level assignment.
+
+    Covers both ``NAME = ...`` and the annotated ``NAME: Final = ...`` form
+    module constants are conventionally written in.
+    """
+    if isinstance(s, ast.Assign) and len(s.targets) == 1 and isinstance(s.targets[0], ast.Name):
+        return s.targets[0], s.value
+    if isinstance(s, ast.AnnAssign) and isinstance(s.target, ast.Name) and s.value is not None:
+        return s.target, s.value
+    return None
+
+
 def _module_level_str_consts(tree: ast.Module) -> dict[str, str]:
     """Return ``{name: literal}`` for top-level ``NAME = "string"`` assignments."""
-    return {
-        s.targets[0].id: s.value.value
-        for s in tree.body
-        if isinstance(s, ast.Assign)
-        and len(s.targets) == 1
-        and isinstance(s.targets[0], ast.Name)
-        and isinstance(s.value, ast.Constant)
-        and isinstance(s.value.value, str)
-    }
+    consts: dict[str, str] = {}
+    for s in tree.body:
+        assignment = _assignment(s)
+        if assignment is None:
+            continue
+        target, value = assignment
+        if isinstance(value, ast.Constant) and isinstance(value.value, str):
+            consts[target.id] = value.value
+    return consts
+
+
+def _resolve_fstring_consts(tree: ast.Module, consts: dict[str, str]) -> dict[str, str]:
+    """Resolve top-level ``NAME = f"{CONST}/lit"`` against already-known consts.
+
+    Only plain interpolations of known string constants count — a conversion,
+    format spec, or unknown name makes the value non-static, so it is skipped
+    rather than guessed at.
+    """
+    resolved: dict[str, str] = {}
+    for s in tree.body:
+        assignment = _assignment(s)
+        if assignment is None or not isinstance(assignment[1], ast.JoinedStr):
+            continue
+        target, value = assignment
+        parts: list[str] = []
+        for piece in value.values:
+            if isinstance(piece, ast.Constant) and isinstance(piece.value, str):
+                parts.append(piece.value)
+            elif (
+                isinstance(piece, ast.FormattedValue)
+                and isinstance(piece.value, ast.Name)
+                and piece.value.id in consts
+                and piece.conversion == -1
+                and piece.format_spec is None
+            ):
+                parts.append(consts[piece.value.id])
+            else:
+                break
+        else:
+            resolved[target.id] = "".join(parts)
+    return resolved
 
 
 def _iter_render_components(tree: ast.Module, consts: dict[str, str]) -> list[str]:
@@ -79,6 +124,19 @@ def find_render_calls(mod: ModuleBase, src_dir: Path) -> set[str]:
     consts: dict[str, str] = {}
     for tree in trees:
         consts.update(_module_level_str_consts(tree))
+    # Then f-string constants built from the plain ones above
+    # (``PAGE = f"{MODULE_NAME}/Browse"`` is the conventional shape). Repeat to
+    # a fixed point so a chain — ``PREFIX = f"{NAME}/sub"`` then
+    # ``PAGE = f"{PREFIX}/Browse"`` — resolves too; one pass would only learn
+    # the first hop and report the page as an SM003 orphan.
+    while True:
+        resolved: dict[str, str] = {}
+        for tree in trees:
+            resolved.update(_resolve_fstring_consts(tree, consts))
+        new = {k: v for k, v in resolved.items() if consts.get(k) != v}
+        if not new:
+            break
+        consts.update(new)
 
     prefix = f"{mod.meta.name}/"
     rendered: set[str] = set()

framework/core/simple_module_core/redirect_safety.py

@@ -53,4 +53,26 @@ def safe_next_or_none(raw: str | None) -> str | None:
     return result or None
 
 
-__all__ = ["DEFAULT_FALLBACK", "SESSION_NEXT_KEY", "safe_next", "safe_next_or_none"]
+def non_empty_redirect(value: str, *, default: str) -> str:
+    """Normalise a configured redirect destination, never returning ``""``.
+
+    Any auth provider can expose a ``login_redirect_url``-style setting, and
+    nothing stops an admin clearing it in the generic module-settings editor.
+    Every consumer treats the value as a destination — Inertia's
+    ``router.visit("")`` silently reloads the current page, and an empty
+    ``Location`` header is a broken redirect — so providers normalise on their
+    settings class, where hydration and ``apply_changes_and_reload`` both run.
+    It lives here rather than in one provider because the providers must not
+    import each other (cross-module coupling), and this is the same concern as
+    the rest of this module.
+    """
+    return value.strip() or default
+
+
+__all__ = [
+    "DEFAULT_FALLBACK",
+    "SESSION_NEXT_KEY",
+    "non_empty_redirect",
+    "safe_next",
+    "safe_next_or_none",
+]

framework/core/tests/test_module_diagnostics.py

@@ -33,42 +33,6 @@ def _mk_module_tree(root: Path, name: str, *, with_pkg_json: bool, with_tsconfig
     return src_dir
 
 
-class TestSm003PageRenderResolution:
-    """SM003 must resolve PAGE_X constants imported from sibling files."""
-
-    def _diags(self, src_dir: Path, mod_name: str):
-        from simple_module_core.diagnostics._pages import check_pages, find_render_calls
-
-        mod = _FakeModule(meta=_FakeMeta(name=mod_name))
-        rendered = find_render_calls(mod, src_dir)  # pyright: ignore[reportArgumentType]
-        return [d for d in check_pages(mod, src_dir, rendered) if d.code == "SM003"]  # pyright: ignore[reportArgumentType]
-
-    async def test_resolves_constant_imported_from_sibling_file(self, tmp_path: Path):
-        src_dir = tmp_path / "feature_flags" / "feature_flags"
-        (src_dir / "pages").mkdir(parents=True)
-        (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}")
-        (src_dir / "constants.py").write_text('PAGE_BROWSE = "FeatureFlags/Browse"\n')
-        endpoints = src_dir / "endpoints"
-        endpoints.mkdir()
-        (endpoints / "views.py").write_text(
-            "from feature_flags.constants import PAGE_BROWSE\n"
-            "async def view(inertia):\n"
-            "    return await inertia.render(PAGE_BROWSE, {})\n"
-        )
-        assert self._diags(src_dir, "FeatureFlags") == []
-
-    async def test_still_flags_truly_orphan_pages(self, tmp_path: Path):
-        src_dir = tmp_path / "m" / "m"
-        (src_dir / "pages").mkdir(parents=True)
-        (src_dir / "pages" / "Ghost.tsx").write_text("export default function G() {}")
-        (src_dir / "endpoints.py").write_text(
-            'async def view(inertia):\n    return await inertia.render("M/Other", {})\n'
-        )
-        results = self._diags(src_dir, "M")
-        assert [r.code for r in results] == ["SM003"]
-        assert "Ghost.tsx" in results[0].message
-
-
 class TestSm017JsWorkspaceFiles:
     async def test_fires_when_both_missing(self, tmp_path: Path):
         src_dir = _mk_module_tree(tmp_path, "orders", with_pkg_json=False, with_tsconfig=False)

framework/core/tests/test_sm003_page_render.py

@@ -0,0 +1,122 @@
+"""SM003 page-render resolution tests, split from test_module_diagnostics.
+
+The resolver reads inertia.render() targets out of module source; these tests
+cover the constant shapes modules actually use (plain, imported, annotated
+f-string) and the dynamic shapes it must refuse to guess at.
+"""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from pathlib import Path
+
+
+@dataclass
+class _FakeMeta:
+    name: str
+
+
+@dataclass
+class _FakeModule:
+    meta: _FakeMeta
+
+
+class TestSm003PageRenderResolution:
+    """SM003 must resolve PAGE_X constants imported from sibling files."""
+
+    def _diags(self, src_dir: Path, mod_name: str):
+        from simple_module_core.diagnostics._pages import check_pages, find_render_calls
+
+        mod = _FakeModule(meta=_FakeMeta(name=mod_name))
+        rendered = find_render_calls(mod, src_dir)  # pyright: ignore[reportArgumentType]
+        return [d for d in check_pages(mod, src_dir, rendered) if d.code == "SM003"]  # pyright: ignore[reportArgumentType]
+
+    async def test_resolves_constant_imported_from_sibling_file(self, tmp_path: Path):
+        src_dir = tmp_path / "feature_flags" / "feature_flags"
+        (src_dir / "pages").mkdir(parents=True)
+        (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}")
+        (src_dir / "constants.py").write_text('PAGE_BROWSE = "FeatureFlags/Browse"\n')
+        endpoints = src_dir / "endpoints"
+        endpoints.mkdir()
+        (endpoints / "views.py").write_text(
+            "from feature_flags.constants import PAGE_BROWSE\n"
+            "async def view(inertia):\n"
+            "    return await inertia.render(PAGE_BROWSE, {})\n"
+        )
+        assert self._diags(src_dir, "FeatureFlags") == []
+
+    async def test_still_flags_truly_orphan_pages(self, tmp_path: Path):
+        src_dir = tmp_path / "m" / "m"
+        (src_dir / "pages").mkdir(parents=True)
+        (src_dir / "pages" / "Ghost.tsx").write_text("export default function G() {}")
+        (src_dir / "endpoints.py").write_text(
+            'async def view(inertia):\n    return await inertia.render("M/Other", {})\n'
+        )
+        results = self._diags(src_dir, "M")
+        assert [r.code for r in results] == ["SM003"]
+        assert "Ghost.tsx" in results[0].message
+
+    async def test_resolves_annotated_fstring_constant(self, tmp_path: Path):
+        """The conventional shape: ``PAGE: Final = f"{MODULE_NAME}/Browse"``.
+
+        Regression test for a false SM003 against audit_log — the resolver
+        skipped both annotated assignments and f-strings, so every module
+        writing its page name this way was flagged as an orphan.
+        """
+        src_dir = tmp_path / "audit_log" / "audit_log"
+        (src_dir / "pages").mkdir(parents=True)
+        (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}")
+        (src_dir / "constants.py").write_text(
+            "from typing import Final\n"
+            'MODULE_NAME: Final = "AuditLog"\n'
+            'PAGE_BROWSE: Final = f"{MODULE_NAME}/Browse"\n'
+        )
+        endpoints = src_dir / "endpoints"
+        endpoints.mkdir()
+        (endpoints / "views.py").write_text(
+            "from audit_log.constants import PAGE_BROWSE\n"
+            "async def view(inertia):\n"
+            "    return await inertia.render(PAGE_BROWSE, {})\n"
+        )
+        assert self._diags(src_dir, "AuditLog") == []
+
+    async def test_resolves_a_chained_fstring_constant(self, tmp_path: Path):
+        """``PREFIX = f"{NAME}/sub"`` then ``PAGE = f"{PREFIX}/Browse"``.
+
+        Resolution runs to a fixed point; a single pass would learn only
+        ``PREFIX`` and wrongly report the page as an orphan.
+        """
+        src_dir = tmp_path / "m" / "m"
+        (src_dir / "pages").mkdir(parents=True)
+        (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}")
+        (src_dir / "constants.py").write_text(
+            "from typing import Final\n"
+            'MODULE_NAME: Final = "M"\n'
+            'SECTION: Final = f"{MODULE_NAME}/admin"\n'
+            'PAGE_BROWSE: Final = f"{SECTION}/Browse"\n'
+        )
+        (src_dir / "views.py").write_text(
+            "from m.constants import PAGE_BROWSE\n"
+            "async def view(inertia):\n"
+            "    return await inertia.render(PAGE_BROWSE, {})\n"
+        )
+        # The rendered component is "M/admin/Browse" while the page file is
+        # pages/Browse.tsx, so this asserts the constant resolved at all —
+        # an unresolved chain reports Browse.tsx as an SM003 orphan.
+        from simple_module_core.diagnostics._pages import find_render_calls
+
+        mod = _FakeModule(meta=_FakeMeta(name="M"))
+        assert "admin/Browse" in find_render_calls(mod, src_dir)  # pyright: ignore[reportArgumentType]
+
+    async def test_fstring_with_unknown_name_stays_flagged(self, tmp_path: Path):
+        """An f-string over a runtime value is not static — don't guess."""
+        src_dir = tmp_path / "m" / "m"
+        (src_dir / "pages").mkdir(parents=True)
+        (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}")
+        (src_dir / "endpoints.py").write_text(
+            'PAGE = f"{dynamic()}/Browse"\n'
+            "async def view(inertia):\n"
+            "    return await inertia.render(PAGE, {})\n"
+        )
+        results = self._diags(src_dir, "M")
+        assert [r.code for r in results] == ["SM003"]

framework/hosting/simple_module_hosting/migrations.py

@@ -7,15 +7,26 @@
 logger = logging.getLogger(__name__)
 
 
+def script_directory(alembic_ini_path: str = "host/alembic.ini"):
+    """Build the ``ScriptDirectory`` for ``alembic_ini_path``.
+
+    Shared by every caller that needs to walk or query alembic revisions
+    (boot-time head check, the in-app Doctor screen) so the config/ini-path
+    construction has exactly one source of truth.
+    """
+    from alembic.config import Config as AlembicConfig
+    from alembic.script import ScriptDirectory
+
+    return ScriptDirectory.from_config(AlembicConfig(alembic_ini_path))
+
+
 def resolve_head_revision(alembic_ini_path: str = "host/alembic.ini") -> str | None:
     """Return the current head revision string, or ``None`` if alembic
     isn't configured at ``alembic_ini_path`` or has no revisions."""
-    from alembic.config import Config as AlembicConfig
-    from alembic.script import ScriptDirectory
     from alembic.util.exc import CommandError
 
     try:
-        return ScriptDirectory.from_config(AlembicConfig(alembic_ini_path)).get_current_head()
+        return script_directory(alembic_ini_path).get_current_head()
     except (CommandError, FileNotFoundError) as exc:
         logger.debug("Alembic not available: %s", exc)
         return None
@@ -26,9 +37,7 @@ async def check_migrations(engine, alembic_ini_path: str = "host/alembic.ini") -
 
     Returns a dict with migration status for storage on app.state.
     """
-    from alembic.config import Config as AlembicConfig
     from alembic.runtime.migration import MigrationContext
-    from alembic.script import ScriptDirectory
 
     _no_migrations = {
         "current_revision": None,
@@ -40,7 +49,7 @@ async def check_migrations(engine, alembic_ini_path: str = "host/alembic.ini") -
     head = resolve_head_revision(alembic_ini_path)
     if head is None:
         return _no_migrations
-    script = ScriptDirectory.from_config(AlembicConfig(alembic_ini_path))
+    script = script_directory(alembic_ini_path)
 
     async with engine.connect() as conn: