feat(career): extract the whole setlist before the gig starts (no more waiting between songs)#971
Conversation
A feedpak is a zip, and the first play of one pays for its extraction into
sloppak_cache. Inside a set that cost landed BETWEEN songs: the player finished
a number and then sat there waiting for the next one to unpack, mid-gig.
A setlist is a known list up front, so unpack it all while the poster is still on
screen. New POST /gigs/prepare walks the set through resolve_source_dir; the
poster's Play button shows "Preparing set…" while it runs.
Best-effort by design, at every level:
- a corrupt pak in the set does not sink the prepare (it is reported in
`failed`; the play itself surfaces the error exactly as it does outside a
gig — slow beats blocked)
- a host without the library resolvers degrades to a no-op rather than 500
- a failed request just falls through to the old lazy extraction
Ordering matters and is pinned: the set is unpacked BEFORE the stage is borrowed
(venue/viz overwritten) and before the queue starts, so a proposal cancelled
while unpacking leaves nothing half-applied to unwind.
Tests unpack REAL zips rather than mocking the extractor: every song of the set
lands on disk before the first note, a re-prepare does not duplicate the unpack,
one bad pak still leaves the good one prepared, and no-library / empty-setlist
degrade cleanly. 18/18.
NB the other half of the gig report — the per-song results popup interrupting
the set (and worse, claimAutoExit'ing so the queue would not advance until it was
dismissed) — is fixed in the note_detect plugin repo, which is not part of this
checkout.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe career plugin prepares gig songs through a bounded backend endpoint before playback, handles per-song extraction failures and unavailable library resolvers, and preserves fallback startup behavior. Tests cover extraction, caching, validation, degradation, timeout handling, and startup ordering. ChangesGig preparation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant CareerScreen
participant CareerAPI
participant SloppakCache
Player->>CareerScreen: Click Play the gig
CareerScreen->>CareerAPI: POST song filenames for preparation
CareerAPI->>SloppakCache: Resolve and extract feedpak assets
SloppakCache-->>CareerAPI: Per-song extraction results
CareerAPI-->>CareerScreen: Prepared and failed songs
CareerScreen->>CareerScreen: Start playback queue
🚥 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}
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 `@plugins/career/routes.py`:
- Around line 738-777: Tighten input validation in prepare_gig before processing
songs: require songs to be a list, reject lists exceeding the allowed size,
reject non-empty string entries, and reject filenames containing path traversal
such as .. components. Return a failed/error response without calling
sloppak.resolve_source_dir for invalid input, while preserving the existing
per-song best-effort behavior for valid entries.
In `@plugins/career/screen.js`:
- Around line 1136-1147: Update prepareGigSongs so the fetch request to
/gigs/prepare is bounded by an abort timeout, ensuring a stalled server response
rejects and the function continues through its existing fallback path. Keep the
existing button restoration behavior and start-anyway semantics intact.
🪄 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: 9abc2438-dda7-415f-8844-c13128f854ce
📒 Files selected for processing (4)
plugins/career/routes.pyplugins/career/screen.jstests/js/gig_song_end.test.jstests/plugins/career/test_routes.py
| @app.post(f"/api/plugins/{PLUGIN_ID}/gigs/prepare") | ||
| def prepare_gig(body: dict = Body(...)): | ||
| """Unpack every song of the set BEFORE the gig starts. | ||
|
|
||
| A feedpak is a zip: the first play of one pays for its extraction into | ||
| sloppak_cache. Inside a set that cost landed BETWEEN songs — the player | ||
| finished a number and then sat waiting for the next one to unpack, mid- | ||
| gig. A set is a known list up front, so extract it all while the player | ||
| is still looking at the poster. | ||
|
|
||
| Idempotent and cheap on a warm cache: resolve_source_dir() returns the | ||
| already-unpacked dir without rewriting it. Best-effort per song — one | ||
| bad feedpak must not block the set from starting (the play itself will | ||
| surface the error, exactly as it does outside a gig). | ||
| """ | ||
| files = [str(f) for f in ((body or {}).get("songs") or []) if f] | ||
| if not files: | ||
| return {"ok": True, "prepared": 0, "failed": []} | ||
|
|
||
| # .get, not []: a host that doesn't hand us the resolvers (or has no | ||
| # library configured) must degrade to "extract lazily, as before" — this | ||
| # is an optimisation, and it is never allowed to be the thing that stops | ||
| # a gig from starting. | ||
| get_dlc = context.get("get_dlc_dir") | ||
| get_cache = context.get("get_sloppak_cache_dir") | ||
| dlc_root = get_dlc() if callable(get_dlc) else None | ||
| cache_root = get_cache() if callable(get_cache) else None | ||
| if dlc_root is None or cache_root is None: | ||
| return {"ok": False, "prepared": 0, "failed": files, "error": "no library"} | ||
|
|
||
| prepared, failed = 0, [] | ||
| for fn in files: | ||
| try: | ||
| sloppak.resolve_source_dir(fn, Path(dlc_root), Path(cache_root)) | ||
| prepared += 1 | ||
| except Exception as exc: # noqa: BLE001 — one bad pak can't sink the set | ||
| _state["log"].warning("career: gig pre-extract failed for %s: %s", fn, exc) | ||
| failed.append(fn) | ||
| return {"ok": True, "prepared": prepared, "failed": failed} | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- plugins/career/routes.py outline ---'
ast-grep outline plugins/career/routes.py --view expanded || true
echo '--- relevant line ranges ---'
sed -n '650,830p' plugins/career/routes.py | cat -n
echo '--- search resolve_source_dir ---'
rg -n "def resolve_source_dir|resolve_source_dir\(" -S .Repository: got-feedBack/feedBack
Length of output: 14344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map sibling endpoints and the cited function.
rg -n "def log_gig|songs|get_pack_file|resolve_source_dir|prepare_gig" plugins/career/routes.py -n -S
# Show only the nearby slices around log_gig and get_pack_file.
sed -n '1,260p' plugins/career/routes.py | cat -n
sed -n '260,520p' plugins/career/routes.py | cat -n
sed -n '520,830p' plugins/career/routes.py | cat -nRepository: got-feedBack/feedBack
Length of output: 45961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow inspection of the targeted area and any resolver definition.
FILE=$(git ls-files 'plugins/career/routes.py' | head -n 1)
echo "FILE=$FILE"
sed -n '700,790p' "$FILE" | cat -n
echo '--- sibling validation around log_gig ---'
rg -n "def log_gig|isinstance\(songs, list\)|len\(songs\) > 8|isinstance\(f, str\)|f.strip\(\)" "$FILE" -n -S
echo '--- resolve_source_dir definition candidates ---'
fd -a 'sloppak*.py|*.py' . | head -n 50
rg -n "def resolve_source_dir|class .*sloppak|resolve_source_dir\(" -S .Repository: got-feedBack/feedBack
Length of output: 6485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lib/sloppak.py resolve_source_dir ---'
sed -n '330,430p' lib/sloppak.py | cat -n
echo '--- lib/sloppak.py nearby helper(s) ---'
sed -n '430,560p' lib/sloppak.py | cat -n
echo '--- plugins/career/routes.py get_pack_file ---'
sed -n '930,1010p' plugins/career/routes.py | cat -nRepository: got-feedBack/feedBack
Length of output: 13727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect log_gig for comparison with prepare_gig.
sed -n '834,910p' plugins/career/routes.py | cat -n
# Inspect any filename canonicalization helpers used elsewhere in career routes.
rg -n "strip\(\)|relative_to\(|resolve\(\)|safe_join|filename" plugins/career/routes.py lib/sloppak.py -SRepository: got-feedBack/feedBack
Length of output: 13980
Tighten prepare_gig input validation plugins/career/routes.py:739-777 — reject non-list/oversized songs values and non-empty strings before the loop; sloppak.resolve_source_dir() trusts the filename, so a crafted .. path can escape the DLC root.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 739-739: Do not perform function call Body in 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 `@plugins/career/routes.py` around lines 738 - 777, Tighten input validation in
prepare_gig before processing songs: require songs to be a list, reject lists
exceeding the allowed size, reject non-empty string entries, and reject
filenames containing path traversal such as .. components. Return a failed/error
response without calling sloppak.resolve_source_dir for invalid input, while
preserving the existing per-song best-effort behavior for valid entries.
…review) Both CodeRabbit findings were right. 1. A HUNG PREPARE COULD BLOCK THE GIG FOREVER. `await fetch(...)` only rejects on a network ERROR. A server that accepts the connection and then never answers hangs indefinitely — and the gig would never start. That makes this optimisation the exact thing the PR promises it can never be: the reason you cannot play. The request is now bounded by an AbortController (PREPARE_TIMEOUT_MS, generous because unpacking a setlist is real work — but a CEILING, not a wait). Past it we start the gig and let the first play extract lazily, as it always did. The Play button is restored in a `finally`, so a timeout cannot strand the poster on "Preparing set…" with Play disabled — which would have been the same bug wearing a different hat. 2. THE `songs` BODY WAS UNVALIDATED. A str is iterable: "abc" would have prepared three one-character "songs". And the endpoint unpacks zips, so an arbitrary caller could ask for unbounded work. Now list-only, string entries, blanks dropped, capped at MAX_GIG_SONGS. Tests: the fetch is abortable and the button is re-enabled on EVERY path including the abort; non-list bodies, non-string/blank entries, and an oversized setlist. 50 career tests, JS 5/5, eslint clean.
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/plugins/career/test_routes.py`:
- Around line 284-285: Split the semicolon-separated directory initialization
statements in the test setup into separate statements for dlc and cache,
resolving the Ruff E702 violations without changing their behavior.
- Around line 294-304: Update test_prepare_caps_the_setlist to verify the
request is truncated to MAX_GIG_SONGS rather than relying on prepared == 0;
assert the response’s failed list contains only the capped number of entries,
while preserving the existing cap bound and endpoint request.
🪄 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: aeb44cc2-b958-46a3-86b4-d02b3c3d2602
📒 Files selected for processing (4)
plugins/career/routes.pyplugins/career/screen.jstests/js/gig_song_end.test.jstests/plugins/career/test_routes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/js/gig_song_end.test.js
- plugins/career/routes.py
…y tests the cap CodeRabbit again, and the first one is a real hole I put there. 1. PATH TRAVERSAL. sloppak.resolve_source_dir() does a bare `dlc_root / filename` with NO containment guard — so `../../x` walks straight out of the library, and my new endpoint handed it attacker-supplied filenames. Every filename now goes through _resolve_dlc_path first, the same check every other filename-bound handler applies. Pinned: `..`, backslash traversal, an absolute POSIX path and a Windows drive path are all refused, and nothing outside the library is unpacked. 2. THE CAP TEST WAS VACUOUS. It asserted `prepared == 0` against a fixture with no library — where the endpoint exits before extraction — so it passed whether or not MAX_GIG_SONGS existed. It now runs against a real library and asserts the endpoint CONSIDERED at most MAX_GIG_SONGS of the 82 it was handed. Verified to fail when the cap is removed. Same class of mistake as the notedetect gigBlock: a test that passes for the wrong reason. Worth saying out loud since it is twice in one day. 3. E702 — semicolon-joined statements in the new tests, split. 51 career tests; full suite green.
|
@coderabbitai the input-validation finding is addressed in The path traversal was real and mine. Also in that commit: And your other two: the E702 semicolons are split, and the cap test was vacuous — it asserted |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show 49da302 --stat
git show 49da302 -- plugins/career/routes.pyLength of output: 5469
The revised cap test (real library, asserting the endpoint only considers up to Nice work tightening this up in one pass. ✏️ Learnings added
|
1 participant
A feedpak is a zip, and the first play of one pays for its extraction. Inside a set that cost landed between songs: you finished a number and then sat waiting for the next one to unpack, mid-gig.
A setlist is a known list up front, so unpack it all while the poster is still on screen. New
POST /gigs/prepare; the poster's Play button shows "Preparing set…" while it runs.Best-effort at every level — this is an optimisation and can never be the reason a gig will not start:
failed; the play surfaces the error as it does outside a gig — slow beats blocked)Ordering is pinned: the set is unpacked before the stage is borrowed (venue/viz overwritten) and before the queue starts, so a proposal cancelled while unpacking leaves nothing half-applied.
Tests unpack real zips rather than mocking the extractor: every song lands on disk before the first note, a re-prepare does not duplicate the unpack, one bad pak still leaves the good one prepared, and no-library / empty-setlist degrade cleanly. 18/18.
The other half of that report — the per-song results popup interrupting the set, and
claimAutoExitstalling the queue until it was dismissed — is in feedBack-plugin-notedetect.Summary by CodeRabbit
New Features
POST /api/plugins/career/gigs/prepareto prepare requested songs in advance.Bug Fixes
Tests