fix(build): move appstate.py + routers/ under lib/ so the packaged desktop app ships them - #836
Merged
Merged
Conversation
… ships them
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>
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThe PR adds audio-effects mapping routes under ChangesAudio-effects routing and packaging
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant audio_effects_router
participant appstate_audio_effect_mappings
Client->>audio_effects_router: Send audio-effects mapping request
audio_effects_router->>appstate_audio_effect_mappings: Execute mapping operation
appstate_audio_effect_mappings-->>audio_effects_router: Return result or ValueError
audio_effects_router-->>Client: Return JSON response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 29-30: Update the changelog heading to use `lib/routers/` instead
of `routers/`, keeping it consistent with the documented implementation path.
In `@lib/routers/audio_effects.py`:
- Line 43: Update upsert_audio_effect_mapping to declare its request body
parameter using Annotated rather than Body(...) as a default value, avoiding
Ruff B008; preserve default_factory=dict on activate_audio_effect_mapping.
In `@tests/test_packaging.py`:
- Around line 33-35: Remove "plugins" from BUNDLED_ROOTS and represent only
plugins/__init__.py in the explicit bundled-file expectations, updating the
packaging guard and related documentation/tests to enforce the desktop bundler’s
exact plugin inclusion rather than allowing the entire directory.
🪄 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: 1543a541-5136-4e6f-90f0-e7cdb663679a
📒 Files selected for processing (9)
.dockerignoreCHANGELOG.mdDockerfiledocker-compose.ymllib/appstate.pylib/routers/__init__.pylib/routers/audio_effects.pyserver.pytests/test_packaging.py
💤 Files with no reviewable changes (3)
- .dockerignore
- docker-compose.yml
- Dockerfile
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 29-30: Update the changelog heading to use `lib/routers/` instead
of `routers/`, keeping it consistent with the documented implementation path.
In `@lib/routers/audio_effects.py`:
- Line 43: Update upsert_audio_effect_mapping to declare its request body
parameter using Annotated rather than Body(...) as a default value, avoiding
Ruff B008; preserve default_factory=dict on activate_audio_effect_mapping.
In `@tests/test_packaging.py`:
- Around line 33-35: Remove "plugins" from BUNDLED_ROOTS and represent only
plugins/__init__.py in the explicit bundled-file expectations, updating the
packaging guard and related documentation/tests to enforce the desktop bundler’s
exact plugin inclusion rather than allowing the entire directory.
🪄 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: 1543a541-5136-4e6f-90f0-e7cdb663679a
📒 Files selected for processing (9)
.dockerignoreCHANGELOG.mdDockerfiledocker-compose.ymllib/appstate.pylib/routers/__init__.pylib/routers/audio_effects.pyserver.pytests/test_packaging.py
💤 Files with no reviewable changes (3)
- .dockerignore
- docker-compose.yml
- Dockerfile
🛑 Comments failed to post (3)
CHANGELOG.md (1)
29-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual
lib/routerspath in the heading.The heading still says
routers/, contradicting the implementation path documented on Line 30. Rename it tolib/routers/for consistency.🤖 Prompt for 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. In `@CHANGELOG.md` around lines 29 - 30, Update the changelog heading to use `lib/routers/` instead of `routers/`, keeping it consistent with the documented implementation path.lib/routers/audio_effects.py (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash fd -HI '^(pyproject\.toml|setup\.cfg|tox\.ini)$' -x \ rg -n 'fastapi|ruff|B008' {}Repository: got-feedBack/feedBack
Length of output: 159
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '\n== file list ==\n' git ls-files | rg '^(lib/routers/audio_effects\.py|pyproject\.toml|setup\.cfg|tox\.ini|ruff\.toml|requirements.*\.txt|Pipfile|poetry\.lock|uv\.lock)$' printf '\n== audio_effects outline ==\n' ast-grep outline lib/routers/audio_effects.py --view expanded || true printf '\n== relevant lines ==\n' cat -n lib/routers/audio_effects.py | sed -n '1,220p' printf '\n== lint config / fastapi pins ==\n' for f in pyproject.toml setup.cfg tox.ini ruff.toml requirements.txt requirements-dev.txt Pipfile poetry.lock uv.lock; do if [ -f "$f" ]; then echo "--- $f ---" rg -n '(^\s*(fastapi|ruff)\b|B008|select\s*=|ignore\s*=|per-file-ignores|extend-ignore|line-length|version\s*=|==)' "$f" || true fi doneRepository: got-feedBack/feedBack
Length of output: 4412
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail echo '== lib/routers/audio_effects.py ==' cat -n lib/routers/audio_effects.py | sed -n '1,220p' echo echo '== lint / dependency config ==' for f in pyproject.toml setup.cfg tox.ini ruff.toml requirements.txt requirements-dev.txt Pipfile poetry.lock uv.lock; do if [ -f "$f" ]; then echo "--- $f ---" rg -n 'fastapi|ruff|B008|select\s*=|ignore\s*=|extend-ignore|per-file-ignores|version\s*=|==|dependencies' "$f" || true fi doneRepository: got-feedBack/feedBack
Length of output: 3893
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail echo '== lib/routers/audio_effects.py ==' cat -n lib/routers/audio_effects.py | sed -n '1,220p'Repository: got-feedBack/feedBack
Length of output: 3681
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail echo '== fastapi / ruff references ==' rg -n --hidden --glob '!**/.git/**' -S 'fastapi|B008|Body\(' . echo echo '== audio_effects file ==' cat -n lib/routers/audio_effects.py | sed -n '1,180p'Repository: got-feedBack/feedBack
Length of output: 12008
Move these request-body params to
Annotated.Body(...)in the default position triggers Ruff B008; keepdefault_factory=dictonactivate_audio_effect_mapping.🧰 Tools
🪛 Ruff (0.15.20)
[warning] 43-43: Do not perform function call
Bodyin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🤖 Prompt for 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. In `@lib/routers/audio_effects.py` at line 43, Update upsert_audio_effect_mapping to declare its request body parameter using Annotated rather than Body(...) as a default value, avoiding Ruff B008; preserve default_factory=dict on activate_audio_effect_mapping.Source: Linters/SAST tools
tests/test_packaging.py (1)
33-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not model
plugins/as a wholesale bundled root.The module documentation says the desktop bundler copies only
plugins/__init__.py, butBUNDLED_ROOTSallows every module underplugins/. A futureserver.pyimport ofplugins.some_modulewould therefore pass this guard while still being omitted from the packaged app.Proposed fix
-BUNDLED_ROOTS = ("lib", "plugins", "data", "static") -BUNDLED_FILES = ("server.py", "main.py") +BUNDLED_ROOTS = ("lib", "data", "static") +BUNDLED_FILES = ("server.py", "main.py", "plugins/__init__.py")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Directories every packaging path copies wholesale, plus the files copied by name. BUNDLED_ROOTS = ("lib", "data", "static") BUNDLED_FILES = ("server.py", "main.py", "plugins/__init__.py")🤖 Prompt for 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. In `@tests/test_packaging.py` around lines 33 - 35, Remove "plugins" from BUNDLED_ROOTS and represent only plugins/__init__.py in the explicit bundled-file expectations, updating the packaging guard and related documentation/tests to enforce the desktop bundler’s exact plugin inclusion rather than allowing the entire directory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The break
The packaged desktop app dies at startup on current
main:feedback-desktop'sscripts/bundle-slopsmith.sh:51-53copies a hardcoded list of core files into the bundle:The root-level
appstate.py(#833) androuters/(#834) ship correctly in Docker, pass the whole suite, and are then silently dropped from the packaged app. I verified the Dockerfile anddocker-compose.ymlwhen I added them; I never checked the desktop bundler. My miss.The fix
Both move under
lib/— the one core directory every packaging path already copies wholesale, and that all three put onsys.path:lib/sys.pathCOPY lib/ /app/lib/ENV PYTHONPATH=/app/lib:/app- ./lib:/app/libPYTHONPATH=/app/lib:/appcp -r "$SLOPSMITH_DIR/lib"python.ts:520PYTHONPATH; on Windowsbuild-windows.shwrites../slopsmith+../slopsmith/libinto the embeddable._pth, since PYTHONPATH is ignored in isolated modeNo
feedback-desktopchange and no new release are required for this to take effect. TheDockerfile/.dockerignore/docker-compose.ymlentries I added for the root layout are reverted.git mv, so--followsurvives. Nothing else in core changes.lib/is also the correct home under Principle V, and always was once the design settled: with the injection seam,appstate.pyconstructs nothing and does no import-time IO, and a route module only builds anAPIRouter. The premise that forced root placement — "appstate opens sqlite at import" — stopped being true the momentconfigure()replaced ownership. I didn't revisit the placement decision after the design changed.The guard
tests/test_packaging.pywalksserver.py's module-level imports, keeps the ones that resolve inside this repo, and fails if any lives outside a directory the packagers copy:Negative-checked: restoring
appstate.pyto the root fails it. It also has to skiporigin in {"built-in", "frozen"}— on Python 3.14 the frozen stdlib reportsorigin="frozen", andPath("frozen").resolve()lands inside the repo, which flaggedosandstatas first-party.Verification
bundle-slopsmith.sh's copy exactly into a temp dir and booted it withPYTHONPATH=<bundle>:<bundle>/lib:import server,import appstate,import routers.audio_effectsall resolve,appstate.meta_db is server.meta_db, 143 routes. The same simulation againstorigin/mainreproduces the productionModuleNotFoundError. That's the check that was missing.pytest— 2398 passed (2348 + 50 new), 4 skipped.origin/main— 143 routes, same paths, methods, registration order.lib/appstate.pyandlib/routers/, with no__pycache__(verified against the real daemon)./api/version,/api/library, the moved audio-effects router, and all three migrated plugins'src/module graphs.pyflakesclean;npm run lint0 errors; Codex preflight 0 findings.One small follow-up for feedback-desktop (not blocking)
bundle-slopsmith.sh:54doesrm -rf "$BUNDLE_DIR/lib/__pycache__"— top level only.lib/routers/__pycache__now exists in a dev checkout and would ride along in a local build (CI clones fresh, so CI builds are unaffected). Harmless — CPython validates.pycagainst source mtime — butfind "$BUNDLE_DIR/lib" -name __pycache__ -prune -exec rm -rf {} +would be tidier.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation