feat(server): add appstate.py, the router seam (R3) - #833
Conversation
Routes moving out of server.py need `meta_db` and friends, but must not
`import server` -- that goes circular the moment server imports them back.
server.py keeps CONSTRUCTING its singletons and now injects them once via
`appstate.configure(...)`; routers read them back as module attributes at call
time (`import appstate; appstate.meta_db`). The Python analogue of the frontend
refactor's `configureX({...})` seams and of the plugin `setup(app, context)`
contract: dependencies flow one way, server -> routers -> appstate.
Two properties are load-bearing, both pinned by tests/test_appstate.py:
1. `import appstate` constructs nothing and touches no disk. This is why the
~49 test fixtures that `sys.modules.pop("server")` + re-import (to rebuild
meta_db under a patched CONFIG_DIR) keep working UNTOUCHED. A singleton
owned by appstate would survive that pop and go stale -- verified.
2. Reads must be late-bound. `from appstate import meta_db` freezes the binding
and defeats both a later configure() and monkeypatch.setattr -- the same
read-only-binding trap as ES imports.
configure() raises on an unknown slot instead of silently creating a global
nothing reads, and the suite asserts server ACTUALLY calls it. Negative-checked:
dropping the configure() call fails exactly the two wiring tests while the other
five stay green -- those five are the false-green a seam test must not be.
The new suite imports server through an `isolated_server` fixture that patches
CONFIG_DIR to tmp_path and closes both DB connections on teardown. An unguarded
`import server` constructs MetadataDB + AudioEffectsMappingDB under the real
`~/.local/share/feedback` (reproduced: running the file alone created
web_library.db + audio_effects.db there). The full suite now leaves the real
config dir untouched.
Packaging: `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 -- without it the image build fails on the COPY. Verified against the
real docker daemon (build context reaches /app/appstate.py). docker-compose.yml
gains the dev bind-mount; docker-compose.nas.yml runs the baked image, so the
COPY covers it. `routers/` will need the same two entries when it lands.
Verified: pyflakes clean; pytest 2348 passed (2341 + 7 new); eslint 0 errors;
boot smoke serves /api/version, /api/library, /api/audio-effects/mappings, and
all three migrated plugins' src/ graphs, with `appstate.meta_db is server.meta_db`
asserted against the live import.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesRouter seam
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant server.py
participant appstate
participant Router
server.py->>server.py: Construct shared singletons
server.py->>appstate: configure(meta_db, audio_effect_mappings)
Router->>appstate: Read singleton attributes at call time
appstate-->>Router: Return configured instances
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_appstate.py`:
- Around line 117-122: Update
test_reimporting_server_republishes_the_fresh_singletons to change CONFIG_DIR to
a new temporary directory, remove the existing server module from sys.modules,
and import server again before asserting appstate.meta_db points to the newly
created database. Use the re-imported server’s singleton and verify its db_path
reflects the new CONFIG_DIR, ensuring the test exercises appstate
reconfiguration.
- Around line 19-41: Update the isolated_server fixture teardown to reset the
published appstate.meta_db and appstate.audio_effect_mappings references after
closing the server databases. Snapshot their original values before importing or
cleanup and restore them afterward, or explicitly set both slots to None,
ensuring later tests cannot observe closed singleton connections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59762b2f-4e99-436d-a8bb-a6812fde6934
📒 Files selected for processing (8)
.dockerignoreCHANGELOG.mdDockerfileappstate.pydocker-compose.ymldocs/size-exemptions.mdserver.pytests/test_appstate.py
…y re-import Two real findings on #833, both fixed: (1) `isolated_server` closed server's DB connections but left `appstate.meta_db` and `appstate.audio_effect_mappings` published and pointing at the closed handles -- a live-looking, dead singleton for any later test. Teardown now snapshots and restores both slots. (2) `test_reimporting_server_republishes_the_fresh_singletons` never performed a second import: it only re-asserted what `test_server_wires_the_seam` already covers, so it could not detect the very staleness it names. (I introduced that regression while fixing Codex's CONFIG_DIR isolation finding.) It now pops `server`, re-imports under a SECOND CONFIG_DIR, and asserts the seam republishes -- `second_server.meta_db is not first_db` and `appstate.meta_db is second_server.meta_db`. Negative-checked both directions: simulating an appstate-OWNED singleton (configure() only-first-wins) now fails the re-import test, and dropping server's configure() call still fails exactly the two wiring tests. NB CodeRabbit's committable suggestion inserted the snapshot above the fixture docstring, which would have demoted it from __doc__; written by hand instead. pytest 2348 passed; the full suite leaves the real ~/.local/share/feedback untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both CodeRabbit findings were real and are fixed in 1. Stale slots on teardown — accepted. The fixture closed 2. The re-import test didn't re-import — accepted, and the sharper of the two. I introduced that regression myself while fixing Codex's assert second_server.meta_db is not first_db # genuinely rebuilt
assert str(second_config) in second_server.meta_db.db_path
assert appstate.meta_db is second_server.meta_db # ...and re-publishedBoth fixes are negative-checked, since a guard that passes either way guards nothing:
One note on the committable suggestion: it placed the snapshot assignments above the fixture docstring, which would have demoted the docstring to a bare expression statement and dropped
|
…_effects.py (R3) (#834) The first route module through the appstate seam (#833). Picked BY MEASUREMENT, not by the plan's guess: a transitive dep-closure scan over every route group ranked audio-effects at 0 monkeypatch.setattr targets and exactly one exclusive helper. (The same scan disproved the plan's assumption that artists/aliases was free -- api_artist_links reaches _mb_http_get and _enrich_network_enabled, both setattr targets.) Bodies are verbatim. The only edits are mechanical: @app.get(...) -> @router.get(...) audio_effect_mappings.x -> appstate.audio_effect_mappings.x The singleton read must stay a module attribute resolved at call time, so a re-imported server re-publishes a fresh DB into the seam and monkeypatch reaches this module. `routers/` never imports `server`: server -> routers -> appstate. `app.include_router(...)` sits exactly where the routes used to be defined -- FastAPI matches in registration order, so the mount site preserves it. Verified by diffing the FULL route table against origin/main: 143 routes, identical paths, methods AND order. server.py: 9,445 -> 9,386 lines. `fastapi.Query` went dead with the move and was removed (the other four unused imports are pre-existing on main). Packaging: COPY routers/ /app/routers/ plus `!routers/` + `!routers/**` in .dockerignore (that file opens with a blanket `*`). Verified against the real docker daemon: routers/ reaches the build context, __pycache__ does not. Verified: pyflakes clean on routers/; no new undefined name in server.py; pytest 2348 passed (75 in the audio-effects + demo-mode suites); eslint 0 errors; boot smoke drives all five routes end-to-end (create -> read back -> activate -> clear -> delete -> 404 on missing -> 400 on bad body), Query(...) still 422s on a missing required param, and demo mode still 403s all four moved write routes while allowing the read. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… ships them (#836) The packaged desktop app died at startup: File ".../Resources/slopsmith/server.py", line 71, in <module> 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=<bundle>:<bundle>/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) <noreply@anthropic.com>
Third step of R3, after #830 (
MetadataDB) and #831 (AudioEffectsMappingDB). This is the seam the router train needs — landed on its own, per the master plan's "appstate.pygets its own risk-managed PR, never folded into a neutral router split."The problem
Routes moving out of
server.pyneedmeta_dband friends. They must notimport server— the graph goes circular the momentserverimports them back.The shape
server.pykeeps constructing its singletons and now injects them once:and a router reads them back as module attributes, at call time:
Dependencies flow one way:
server → routers → appstate. This is the Python analogue of the frontend refactor's injectedconfigureX({…})seams (stems'configureStreaming, studio'sconfigureAudioGraph, the editor'ssrc/host.js) and of the pluginsetup(app, context)contract in Principle III.Two properties that are load-bearing
1.
import appstateconstructs nothing and touches no disk. This is whyappstatedoes not own the singletons, as originally sketched. 49 test files dosys.modules.pop("server")+ re-import to rebuildmeta_dbunder a patchedCONFIG_DIR. A singleton owned byappstatesurvives that pop and goes stale — reproduced directly:With the injection seam,
serverremains the owner, so all 49 fixtures keep working untouched and the router still sees the fresh DB.2. Reads must be late-bound.
from appstate import meta_dbfreezes the binding, defeating both a laterconfigure()andmonkeypatch.setattr(appstate, "meta_db", fake). Same read-only-binding trap as ESimport. Routers must useappstate.meta_db. Pinned by a test.Guards
configure()raises on an unknown slot rather than silently creating a global nothing reads. A seam whose wiring can no-op undetected is worse than no seam — the frontend refactor lost a regression to exactly that twice, when a scriptedsetHostHooksedit stopped matching its anchor.None: inert but type-honest, so a router running beforeconfigure()fails loudly onNoneTypeinstead of quietly operating on a stand-in.tests/test_appstate.pyassertsserveractually callsconfigure. Negative-checked: dropping the call fails exactly the two wiring tests while the other five stay green — those five are precisely the false-green a seam test must not be.Packaging (verified against the real Docker daemon)
.dockerignoreopens with a blanket*exclusion and re-allows files one by one, soCOPY appstate.py /app/fails the image build without an allowlist entry. Confirmed:Fixed with
!appstate.py, then re-verified that the build context reaches/app/appstate.py.docker-compose.ymlgains the dev bind-mount;docker-compose.nas.ymlruns the baked image so theCOPYcovers it.routers/will need both entries when it lands.Verification
pytest— 2348 passed (2341 + 7 new), 4 skipped.pyflakesclean;npm run lint0 errors./api/version,/api/library,/api/audio-effects/mappingsall answer; all three migrated plugins still serve theirsrc/graphs;appstate.meta_db is server.meta_dbasserted against the live import.serverwithout patchingCONFIG_DIR, so running the file alone createdweb_library.db+audio_effects.dbin the developer's real~/.local/share/feedback. Reproduced, fixed with anisolated_serverfixture (tmp_path + both connections closed on teardown), and the full suite now leaves the real config dir untouched. Re-reviewed: 0 findings.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation