From b3215694e70c119ec6b0d1306e80ceeccee083a1 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Fri, 10 Jul 2026 17:16:34 +0200 Subject: [PATCH] fix(build): move appstate.py + routers/ under lib/ so the desktop app ships them (#836) The packaged desktop app died at startup: File ".../Resources/slopsmith/server.py", line 71, in import appstate ModuleNotFoundError: No module named 'appstate' feedback-desktop's scripts/bundle-slopsmith.sh copies a HARDCODED list of core files into the bundle -- server.py, VERSION, lib/, data/, static/, plugins/__init__.py. The root-level appstate.py (#833) and routers/ (#834) shipped fine in Docker, passed every test, and were silently dropped from the packaged app. Both now live under lib/, the one core directory every packaging path already copies wholesale -- Dockerfile `COPY lib/`, docker-compose.yml, and the desktop bundler's `cp -r lib` -- and that all three put on sys.path (on Windows via the embeddable-Python ._pth, where PYTHONPATH is ignored: build-windows.sh writes `../slopsmith` and `../slopsmith/lib`). No feedback-desktop change and no new release are needed for this to take effect. lib/ is also the CORRECT home under Principle V, and always was once the design settled: with the injection seam, appstate.py constructs nothing and does no import-time IO, and a route module only builds an APIRouter. The premise that forced root placement -- "appstate opens sqlite at import" -- stopped being true when configure() replaced ownership. The Dockerfile / .dockerignore / docker-compose.yml entries added for the root layout are reverted; nothing else in core changes (git mv, so --follow survives). tests/test_packaging.py is the guard: it walks server.py's module-level imports, keeps the ones resolving inside this repo, and fails if any lives outside a directory the packagers copy -- with the ModuleNotFoundError spelled out. So the next root-level core module can't ship broken. Negative-checked: restoring appstate.py to the root fails it; the message names the file and the four packaging files a root module would have to teach. (It also has to skip `origin in {"built-in","frozen"}` -- on 3.14 the frozen stdlib reports origin="frozen", and Path("frozen").resolve() lands inside the repo, which flagged `os` and `stat` as first-party.) Verified: the bundler's copy replicated exactly into a temp dir and booted with PYTHONPATH=:/lib -- `import server`, `import appstate`, `import routers.audio_effects` all resolve, appstate.meta_db is server.meta_db, 143 routes. The same simulation against origin/main reproduces the production ModuleNotFoundError. pytest 2398 passed (2348 + 50 new); route table still identical to origin/main (paths, methods, order); docker build context reaches lib/appstate.py and lib/routers/ with no __pycache__; native uvicorn boot smoke serves /api/version, /api/library, the moved audio-effects router, and all three migrated plugins' src/ graphs; eslint 0 errors. Co-authored-by: Claude Opus 4.8 (1M context) --- .dockerignore | 7 -- CHANGELOG.md | 28 ++++-- Dockerfile | 4 - docker-compose.yml | 2 - appstate.py => lib/appstate.py | 9 ++ {routers => lib/routers}/__init__.py | 9 ++ {routers => lib/routers}/audio_effects.py | 0 server.py | 1 + tests/test_packaging.py | 112 ++++++++++++++++++++++ 9 files changed, 152 insertions(+), 20 deletions(-) rename appstate.py => lib/appstate.py (83%) rename {routers => lib/routers}/__init__.py (60%) rename {routers => lib/routers}/audio_effects.py (100%) create mode 100644 tests/test_packaging.py diff --git a/.dockerignore b/.dockerignore index a9c08a2..9ee9d00 100644 --- a/.dockerignore +++ b/.dockerignore @@ -5,13 +5,6 @@ !dockerfile !requirements.txt !server.py -# The router seam server.py injects its singletons into (R3). Root-level Python -# that ships in the image must be re-allowed explicitly — this file starts with -# a blanket `*` exclusion. -!appstate.py -# Route modules extracted from server.py (R3). -!routers/ -!routers/** !main.py !VERSION !tailwind.config.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a033c07..37ff723 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **The packaged desktop app could not start (`ModuleNotFoundError: No module named + 'appstate'`).** feedback-desktop's `scripts/bundle-slopsmith.sh` copies a *hardcoded + list* of core files into the app bundle — `server.py`, `VERSION`, `lib/`, `data/`, + `static/`, `plugins/__init__.py`. The root-level `appstate.py` and `routers/` added in + R3 shipped correctly in Docker and passed every test, and were then silently dropped + from the packaged app, which died at startup. Both now live under **`lib/`** — the one + core directory the Dockerfile (`COPY lib/`), `docker-compose.yml`, and the desktop + bundler (`cp -r lib`) all copy wholesale, and that all three put on `sys.path` (on + Windows via the embeddable-Python `._pth`, where `PYTHONPATH` is ignored). This needs no + change in feedback-desktop and no new release to take effect. Placing them there is also + correct under Principle V: with the injection seam, `appstate.py` constructs nothing and + does no import-time IO, and a route module only builds an `APIRouter`. The + `Dockerfile` / `.dockerignore` / `docker-compose.yml` entries added for the root layout + are reverted. New `tests/test_packaging.py` walks `server.py`'s module-level imports and + fails if any first-party module resolves outside a directory the packagers copy, so the + next root-level module can't ship broken. + ### Added - **`routers/` — the first extracted route module (R3).** The five audio-effects mapping - endpoints move out of `server.py` into `routers/audio_effects.py` as a + endpoints move out of `server.py` into `lib/routers/audio_effects.py` as a `fastapi.APIRouter`, mounted with `app.include_router(...)` **at the point in the file where they used to be defined** — FastAPI matches routes in registration order, so the mount site preserves it. Verified: the full 143-route table (paths, methods, *and* @@ -19,8 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 resolved at call time). This proves the seam from #833 under a real consumer, including the second slot. The `_demo_mode_guard` middleware still blocks all four moved write routes with 403, and `Query(...)` validation still 422s — both checked against a running - server. Ships via `COPY routers/ /app/routers/` plus `!routers/` + `!routers/**` in - `.dockerignore`. `server.py`: **9,445 → 9,386 lines**. + server. `server.py`: **9,445 → 9,386 lines**. - **`appstate.py` — the router seam (R3).** Route modules moving out of `server.py` need `meta_db` and friends but must not `import server`, or the import graph goes circular the moment `server` imports them back. So `server.py` keeps *constructing* @@ -38,10 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 both a later `configure()` and `monkeypatch.setattr` — the same read-only-binding trap as ES `import`. `configure()` rejects an unknown slot rather than silently creating a global nothing reads, and the suite asserts `server` actually calls it (a seam whose - wiring can no-op undetected is worse than no seam). Ships in the image via - `Dockerfile` `COPY appstate.py /app/` plus a `.dockerignore` allowlist entry — that - file opens with a blanket `*` exclusion, so root-level Python must be re-allowed - explicitly or the build fails. + wiring can no-op undetected is worse than no seam). Lives at `lib/appstate.py`. ### Changed - **`AudioEffectsMappingDB` moved out of `server.py` into `lib/audio_effects_db.py` diff --git a/Dockerfile b/Dockerfile index ddc39cb..ebbd38f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -206,10 +206,6 @@ COPY --from=tailwind-builder /build/static/tailwind.min.css /app/static/tailwind # when a plugin is installed at runtime (see update_manager on-install hook). COPY tailwind.config.js /app/tailwind.config.js COPY server.py /app/ -# The router seam server.py injects its singletons into (R3). Root-level, like -# server.py, so `import appstate` resolves off PYTHONPATH=/app. -COPY appstate.py /app/ -COPY routers/ /app/routers/ COPY main.py /app/ COPY VERSION /app/ # Built-in diagnostic sloppaks seeded into DLC_DIR/diagnostics-builtin/ at scan diff --git a/docker-compose.yml b/docker-compose.yml index acdbe95..19d931e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -11,8 +11,6 @@ services: # Mount source for live reload during development - ./static:/app/static - ./server.py:/app/server.py - - ./appstate.py:/app/appstate.py - - ./routers:/app/routers - ./VERSION:/app/VERSION - ./ug_browser.py:/app/ug_browser.py - ./lib:/app/lib diff --git a/appstate.py b/lib/appstate.py similarity index 83% rename from appstate.py rename to lib/appstate.py index 6c6c69e..d515673 100644 --- a/appstate.py +++ b/lib/appstate.py @@ -47,6 +47,15 @@ quietly operating on a stand-in. Slots are added here only when a router actually needs one — this is a seam, not a grab-bag for everything in ``server.py``. + +**Why this lives in ``lib/`` and not the repo root.** Because it constructs +nothing and does no import-time IO, it satisfies Principle V's rule for ``lib/`` +modules — and ``lib/`` is the only core directory every packaging path already +copies: the Dockerfile (``COPY lib/``), ``docker-compose.yml``, and +feedback-desktop's ``bundle-slopsmith.sh`` (``cp -r lib``). All three also put +both the bundle root and ``lib/`` on ``sys.path``. A root-level module ships in +Docker but is silently dropped from the packaged desktop app, whose bundler +copies a hardcoded file list — that regression is what moved this file here. """ # The singletons routers may read. Every name here must also be a `_SLOTS` key. diff --git a/routers/__init__.py b/lib/routers/__init__.py similarity index 60% rename from routers/__init__.py rename to lib/routers/__init__.py index ede4d39..c81595c 100644 --- a/routers/__init__.py +++ b/lib/routers/__init__.py @@ -19,4 +19,13 @@ and always as a **module attribute, at call time** — never later ``appstate.configure()`` and ``monkeypatch.setattr``. See ``appstate.py``. Dependencies flow one way: ``server -> routers -> appstate``. + +**Why this lives under ``lib/``.** ``lib/`` is the only core directory every +packaging path already copies wholesale — the Dockerfile (``COPY lib/``), +``docker-compose.yml``, and feedback-desktop's ``bundle-slopsmith.sh`` +(``cp -r lib``) — and all three put it on ``sys.path``. A root-level package +ships in Docker but is silently dropped from the packaged desktop app, whose +bundler copies a hardcoded file list. Route modules import nothing at module +scope beyond FastAPI and ``appstate``, so they do no import-time IO and satisfy +Principle V's rule for ``lib/``. """ diff --git a/routers/audio_effects.py b/lib/routers/audio_effects.py similarity index 100% rename from routers/audio_effects.py rename to lib/routers/audio_effects.py diff --git a/server.py b/server.py index 10afa53..4e92b9e 100644 --- a/server.py +++ b/server.py @@ -68,6 +68,7 @@ from metadata_db import ( from audio_effects_db import AudioEffectsMappingDB # The router seam. Imported as a module (never `from appstate import ...`) so # `appstate.configure(...)` below publishes into the same namespace routers read. +# Lives in lib/ because that is the one core dir every packaging path copies. import appstate # Extracted route modules. They import `appstate`, never `server` — one-way graph. from routers import audio_effects diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..fe39830 --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,112 @@ +"""Guard: every first-party module `server.py` imports must be one the packagers copy. + +feedback-desktop's `scripts/bundle-slopsmith.sh` copies a **hardcoded list** from +core into the app bundle — `server.py`, `VERSION`, `lib/`, `data/`, `static/`, +`plugins/__init__.py`. A new root-level module (say `appstate.py`) ships fine in +Docker, imports fine under pytest, and is then *silently dropped* from the +packaged desktop app, which dies at startup with: + + File ".../Resources/slopsmith/server.py", line 71, in + import appstate + ModuleNotFoundError: No module named 'appstate' + +That shipped once. This test is why it can't ship twice: it walks `server.py`'s +module-level imports, keeps the ones that resolve inside this repo, and asserts +each lives under a directory every packaging path already copies wholesale. + +If you add a first-party module for `server.py`, put it in `lib/` — the one core +directory the Dockerfile (`COPY lib/`), `docker-compose.yml`, and the desktop +bundler (`cp -r lib`) all copy, and that all three put on `sys.path`. If you +genuinely need it at the repo root, you must also teach `bundle-slopsmith.sh`, +the `Dockerfile`, `.dockerignore`, and `docker-compose.yml` about it — and then +update `BUNDLED_ROOTS` below. +""" + +import ast +import importlib.util +import pathlib + +import pytest + +REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent + +# Directories every packaging path copies wholesale, plus the files copied by name. +BUNDLED_ROOTS = ("lib", "plugins", "data", "static") +BUNDLED_FILES = ("server.py", "main.py") + + +def _server_toplevel_imports(): + """Module names imported at `server.py`'s top level (not inside a function).""" + tree = ast.parse((REPO_ROOT / "server.py").read_text()) + names = set() + for node in tree.body: # top level only — lazy imports are fine + if isinstance(node, ast.Import): + names.update(a.name.split(".")[0] for a in node.names) + elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: + names.add(node.module.split(".")[0]) + return sorted(names) + + +# `spec.origin` is not always a path: built-in and frozen stdlib modules use +# these sentinels. `Path("frozen").resolve()` would land inside the repo and +# report `os` as first-party — so filter them before touching the filesystem. +_NON_PATH_ORIGINS = {"built-in", "frozen", "namespace"} + + +def _first_party_origin(name): + """Path of `name` if it resolves inside this repo, else None (stdlib/site-package).""" + try: + spec = importlib.util.find_spec(name) + except (ImportError, ValueError): + return None + if spec is None: + return None + + if spec.origin and spec.origin not in _NON_PATH_ORIGINS: + origin = pathlib.Path(spec.origin) + else: + # Namespace/frozen: fall back to the first search location, if any. + locations = list(getattr(spec, "submodule_search_locations", None) or []) + if not locations: + return None + origin = pathlib.Path(locations[0]) + + if not origin.is_absolute(): + return None # a sentinel, not a real path + origin = origin.resolve() + try: + origin.relative_to(REPO_ROOT) + except ValueError: + return None # outside the repo → a dependency + return origin + + +@pytest.mark.parametrize("name", _server_toplevel_imports()) +def test_server_import_is_bundled(name): + origin = _first_party_origin(name) + if origin is None: + return # stdlib or an installed dependency + + rel = origin.relative_to(REPO_ROOT) + if rel.as_posix() in BUNDLED_FILES or rel.parts[0] in BUNDLED_ROOTS: + return + + raise AssertionError( + f"server.py imports `{name}` from {rel}, which no packager copies.\n" + f"The desktop bundler (scripts/bundle-slopsmith.sh) copies only " + f"{BUNDLED_FILES} and {BUNDLED_ROOTS}/, so the packaged app would die " + f"at startup with ModuleNotFoundError: No module named '{name}'.\n" + f"Move it under lib/, or teach bundle-slopsmith.sh + Dockerfile + " + f".dockerignore + docker-compose.yml about it and update BUNDLED_ROOTS." + ) + + +def test_the_seam_and_routers_live_under_lib(): + """Pin the two that already caused a shipped break.""" + for name in ("appstate", "routers"): + origin = _first_party_origin(name) + assert origin is not None, f"{name} does not resolve inside the repo" + assert origin.relative_to(REPO_ROOT).parts[0] == "lib", ( + f"{name} resolved to {origin.relative_to(REPO_ROOT)}; it must live " + f"under lib/ or the packaged desktop app will not ship it" + )