fix(sloppak): the full mix is a stem — drop the invented original_audio key - #946
Merged
Merged
Conversation
Contributor
|
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 PR standardizes complete mixdowns around the reserved ChangesFull mix migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SloppakPack
participant load_song
participant ws_highway
participant highway_js
SloppakPack->>load_song: provide full stem or legacy manifest key
load_song-->>ws_highway: LoadedSloppak.full_mix and instrument stems
ws_highway-->>highway_js: song_info with full_mix_url and has_full_mix
highway_js-->>highway_js: select full-mix routing
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
byrongamatos
force-pushed
the
fix/933-full-mix-is-a-stem
branch
from
July 13, 2026 13:30
26016b0 to
770eeb1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/routers/ws_highway.py (1)
324-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the current full-only payload consistently. The reserved
fullstem is removed fromLoadedSloppak.stemsand exposed throughfull_mix, so a full-only pack emitshas_full_mix: true,has_stems: false, and no instrument stems.
lib/routers/ws_highway.py#L324-L333: remove the claim thatfull_mix_urlis absent for a reserved full-only pack.lib/routers/ws_highway.py#L373-L391: state that both reserved full-only and legacy stem-less packs reach thefull_mix_urlbranch.static/highway.js#L1911-L1925: describe the actualhas_full_mix/has_stemspayload rather than the old[full]stems representation.🤖 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/ws_highway.py` around lines 324 - 333, The documentation incorrectly excludes reserved full-only packs from full_mix_url handling. In lib/routers/ws_highway.py lines 324-333, remove that exclusion; in lines 373-391, document that reserved full-only and legacy stem-less packs both use the full_mix_url branch; in static/highway.js lines 1911-1925, update the payload documentation to describe has_full_mix: true, has_stems: false, and no instrument stems instead of a [full] stems representation.tests/test_migrate_full_mix_stem.py (1)
249-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every non-
offfull-mix default.Parameterize this test with missing, empty, boolean, and invalid values so the verifier cannot regress to accepting unsafe migrated shapes.
🤖 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_migrate_full_mix_stem.py` around lines 249 - 264, Parameterize test_verify_rejects_a_retained_mixdown_that_plays_on_open across missing, empty, boolean, and invalid full-stem default values, while retaining the existing "on" case. Build each manifest with the parameterized value and assert mig.verify_zip returns "full-stem-default-on" for every non-"off" value, ensuring unsafe migrated shapes are rejected.
🤖 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 `@tools/migrate_full_mix_stem.py`:
- Around line 269-273: Update the ThreadPoolExecutor processing around
pool.map(work, packs) so exceptions from an individual pack are caught at the
task boundary and converted into an error status associated with that pack.
Continue processing all remaining packs, increment counts, append failed packs
to problems, and preserve the final problem summary instead of allowing
pool.map() to abort the batch.
- Around line 231-234: The verification in tools/migrate_full_mix_stem.py at
lines 231-234 must accept a multi-stem full mix only when its normalized default
is exactly "off"; reject missing, empty, boolean, malformed, and "on" values.
Add corresponding coverage in tests/test_migrate_full_mix_stem.py at lines
249-264 for each of those invalid defaults while preserving the valid "off"
case.
- Around line 130-131: Update iter_packs() to discover both suffix-matching
files and directories, including directories passed directly, without
restricting candidates to filenames. In migrate_zip() and the related
verification/dispatch paths around the affected lines, detect directory packs
and route them through directory-aware migration and verification logic while
retaining ZipFile handling for archive files.
---
Nitpick comments:
In `@lib/routers/ws_highway.py`:
- Around line 324-333: The documentation incorrectly excludes reserved full-only
packs from full_mix_url handling. In lib/routers/ws_highway.py lines 324-333,
remove that exclusion; in lines 373-391, document that reserved full-only and
legacy stem-less packs both use the full_mix_url branch; in static/highway.js
lines 1911-1925, update the payload documentation to describe has_full_mix:
true, has_stems: false, and no instrument stems instead of a [full] stems
representation.
In `@tests/test_migrate_full_mix_stem.py`:
- Around line 249-264: Parameterize
test_verify_rejects_a_retained_mixdown_that_plays_on_open across missing, empty,
boolean, and invalid full-stem default values, while retaining the existing "on"
case. Build each manifest with the parameterized value and assert mig.verify_zip
returns "full-stem-default-on" for every non-"off" value, ensuring unsafe
migrated shapes are rejected.
🪄 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: dc91d6eb-596e-4bfb-8f8b-71220c4e94da
📒 Files selected for processing (10)
CHANGELOG.mdfeedpak-spec-exceptions.ymllib/enrichment.pylib/routers/ws_highway.pylib/sloppak.pystatic/highway.jstests/test_migrate_full_mix_stem.pytests/test_sloppak_full_mix_load.pytests/test_sloppak_original_audio_load.pytools/migrate_full_mix_stem.py
💤 Files with no reviewable changes (1)
- tests/test_sloppak_original_audio_load.py
byrongamatos
force-pushed
the
fix/933-full-mix-is-a-stem
branch
from
July 13, 2026 13:48
770eeb1 to
1d988ed
Compare
…dio` key (#933) Core read, served, and depended on `original_audio:` — a top-level manifest key this repo invented in #583 that the feedpak spec never defined. The format already had a home for the pre-separation mixdown: it is a stem. feedpak 1.15.0 (feedpak-spec#53) RESERVES the id `full` for it, so read it from there. The key existed to work around a bug in our own reader. The packer's comment said so plainly: "we must NOT list the full mix as a playable stem — the player sums every entry in `stems` and does not gate playback on `default`, so a listed full mix plays on top of the stems". Faced with a reader that would double the song, the packer put the mixdown outside `stems` and invented a key to point at it. The fix belongs in the reader, and that is what this is. load_song() now partitions the stem list: `full` comes out as LoadedSloppak.full_mix, the instruments stay in .stems. Nothing that sums stems or draws one fader per stem can see the mixdown, so retaining it is safe — which is what lets the packer put it where the format says it goes. - ws_highway: `song_info` gains full_mix_url / has_full_mix. The old original_audio_url / has_original_audio remain as deprecated aliases for one release so an older stems plugin keeps working (#945). - `stems` on the wire, and stem_ids / stem_count in the library index, are now INSTRUMENT stems only — a separated pack that retains its mixdown no longer advertises a bogus "full" chip or an inflated stem count. - enrichment: fingerprint against the mixdown wherever it lives. This widens coverage — _song_audio_file() previously returned None for any pack without the invented key, so fingerprinting silently did nothing for nearly every pack. - sloppak: `original_audio:` is still READ as a deprecated fallback, because every pack in the wild carries it and would otherwise lose its pristine mix. tools/migrate_full_mix_stem.py rewrites those packs into the spec shape (original/full.ogg -> stems/full.ogg, add the `full` stem at default:off, drop the key); the fallback and the aliases die with #945. The spec gate keeps the debt honest: the grandfather entry now tracks #945, and the gate fails if it goes stale. Verified: spec gate OK (4/4, incl. ingesting the spec's new example pack that retains `full`); 2493 python tests, 995 js tests; migrator round-tripped over real packs from the library and the results pass the spec's reference validator.
byrongamatos
force-pushed
the
fix/933-full-mix-is-a-stem
branch
from
July 13, 2026 14:54
1d988ed to
ed82b64
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_migrate_full_mix_stem.py (1)
249-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the remaining
verify_zipstatus codes.
verify_zipreturns six distinct statuses (ok,no-manifest,still-has-key,no-full-stem,full-stem-missing-file,full-stem-default-on), but onlyfull-stem-default-onandstill-has-keyare exercised here. Since this tool re-verifies migrated shape across the whole library, a regression in the untested branches (e.g.no-full-stem,full-stem-missing-file) could silently pass CI.🧪 Example additions
def test_verify_rejects_a_pack_with_no_full_stem(tmp_path: Path): m = _manifest(stems=[{"id": "guitar", "file": "stems/guitar.ogg", "default": "on"}]) del m["original_audio"] pak = _write_pack(tmp_path / "song.feedpak", m, files={"stems/guitar.ogg": b"g"}) assert mig.verify_zip(pak) == "no-full-stem" def test_verify_rejects_a_full_stem_whose_file_is_missing(tmp_path: Path): m = _manifest(stems=[{"id": "full", "file": "stems/full.ogg", "default": "on"}]) del m["original_audio"] pak = _write_pack(tmp_path / "song.feedpak", m, files={"arrangements/lead.json": b"{}"}) assert mig.verify_zip(pak) == "full-stem-missing-file"🤖 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_migrate_full_mix_stem.py` around lines 249 - 270, Add tests in tests covering every remaining verify_zip status: verify valid migrated input returns “ok”, missing manifests return “no-manifest”, packs without a full stem return “no-full-stem”, and full stems whose files are absent return “full-stem-missing-file”. Keep the existing full-stem-default-on and still-has-key coverage unchanged, using the established _manifest and _write_pack helpers.
🤖 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.
Nitpick comments:
In `@tests/test_migrate_full_mix_stem.py`:
- Around line 249-270: Add tests in tests covering every remaining verify_zip
status: verify valid migrated input returns “ok”, missing manifests return
“no-manifest”, packs without a full stem return “no-full-stem”, and full stems
whose files are absent return “full-stem-missing-file”. Keep the existing
full-stem-default-on and still-has-key coverage unchanged, using the established
_manifest and _write_pack helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a5a8ffb-8780-4a7e-9583-df54853e551a
📒 Files selected for processing (10)
CHANGELOG.mdfeedpak-spec-exceptions.ymllib/enrichment.pylib/routers/ws_highway.pylib/sloppak.pystatic/highway.jstests/test_migrate_full_mix_stem.pytests/test_sloppak_full_mix_load.pytests/test_sloppak_original_audio_load.pytools/migrate_full_mix_stem.py
💤 Files with no reviewable changes (1)
- tests/test_sloppak_original_audio_load.py
🚧 Files skipped from review as they are similar to previous changes (6)
- feedpak-spec-exceptions.yml
- lib/enrichment.py
- lib/routers/ws_highway.py
- static/highway.js
- CHANGELOG.md
- lib/sloppak.py
…ing them iter_packs() searched only files, so a directory-form pack (`song.sloppak/`, the authoring shape) was walked INTO and never yielded — silently missed by a run that's meant to be exhaustive. Discover suffix-named directories too (yielded whole, not descended into), and route packs through migrate_pack/verify_pack. Directory packs are REPORTED as `dir-form-unsupported`, not rewritten in place: a single-file pack is replaced atomically (a fully-built temp archive swapped in with one os.replace), but a populated directory can't be swapped that way, so an interrupted in-place rewrite could leave an authoring pack half-migrated. The status is a problem status, so it counts against the run's exit code and shows in the summary — the operator re-packs or migrates it as a `.feedpak` instead of it vanishing from the report. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson <topkoa@gmail.com>
verify_zip accepted any non-truthy `default` on a multi-stem `full` (missing,
empty, boolean, `false`/`no`/`0`, malformed) as "ok". But core defaults an ABSENT
`default` to True — ON (lib/sloppak.py: `s.get("default", True)`) — and treats an
empty/unrecognized string as ON too, so a migrated-shape pack whose `full` stem
has a missing or blank default beside instrument stems would actually play the
mixdown on open and double the song. verify was certifying that as safe.
Require an explicit normalized `off` beside instrument stems: `on`-ish values are
reported `full-stem-default-on` (actively plays), everything that is not a
normalized `off` is reported `full-stem-default-not-off`. The migrator already
writes the literal `off`, so its own output is unaffected; this also certifies
the pack is in the tool's canonical, most-portable shape. The len>1 gate is kept,
so a sole `full` stem (which IS the audio) is not policed.
Adds parametrized coverage for missing / empty / boolean / off-ish / malformed
defaults, and a sole-full-stem case. Addresses a CodeRabbit review finding.
Signed-off-by: Kris Anderson <topkoa@gmail.com>
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
2 participants
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.
Closes #933. Depends on got-feedBack/feedpak-spec#53 (feedpak 1.15.0) — the spec must land first, since this reads a stem id that PR reserves. Pairs with got-feedBack/feedBack-plugin-stems#39.
The bug behind the bug
Core read, served, and depended on
original_audio:— a top-level manifest key this repo invented in #583 that the feedpak spec never defined. The format already had a home for the pre-separation mixdown: it is a stem.But the key was not carelessness. The packer's own comment explains exactly why it existed:
So the chain was: a reader that sums every stem ⇒ the mixdown cannot live in
stems[]⇒ invent a key and a directory to hold it. A reader limitation drove format drift, and the drift then escaped into every pack in the wild. The fix belongs in the reader — and that is what this is.What changed
load_song()now partitions the stem list: the RESERVEDfullstem (spec §5.3) comes out asLoadedSloppak.full_mix; the instruments stay in.stems. Nothing that sums stems or draws one fader per stem can see the mixdown — which is precisely what makes retaining it safe, and lets the packer put it where the format says it goes.song_infogainsfull_mix_url/has_full_mix.original_audio_url/has_original_audiostay as deprecated aliases (same values) for one release, so an older bundled stems plugin keeps working. Removed in Remove the deprecatedoriginal_audio:fallback (and its WS aliases) once packs are migrated #945.stemson the wire, andstem_ids/stem_countin the library index, are now instrument stems only — a separated pack that retains its mixdown no longer advertises a bogus "full" chip or an inflated stem count.lib/enrichment.pyfingerprints against the mixdown wherever it lives. This widens coverage:_song_audio_file()previously returnedNonefor any pack without the invented key, so audio fingerprinting silently did nothing for the overwhelming majority of packs.lib/sloppak.pystill readsoriginal_audio:as a deprecated fallback — every pack produced before the spec caught up carries it and would otherwise silently lose its pristine mix. Never written.tools/migrate_full_mix_stem.py(new) rewrites those packs into the spec shape:original/full.ogg→stems/full.ogg, add{id: full, default: off}, drop the key, stampfeedpak_version: 1.15.0. Atomic per pack, idempotent,--dry-run/--verify, and it refuses rather than guesses when the mixdown is missing or the target path is taken.default: offon the retained mixdown is the safety margin: honouringdefaulthas been normative since feedpak 1.0.0, so even a reader that predates the reserved id will not playfullon open.The gate stays honest
feedpak-spec-exceptions.ymlkeeps its one entry while the deprecated read exists, now tracking #945 instead of this issue. The gate fails if the entry goes stale, so the debt cannot be quietly forgotten — deleting the read is what makes it stale.Verification
full.original/gone, mixdown atstems/full.ogg, bytes preserved, unknown keys (source_tool) preserved verbatim — and the results pass the feedpak reference validator. Core then loads a migrated pack and an un-migrated one to identical WS frames apart from the mixdown's path, so the migration is invisible to clients.fullremoved by identity (a duplicate entry survived into the summed list), the migrator demanding a canonical path from an already-migrated pack, a single-mix pack falling back to the legacy key and surfacing its mixdown twice, and the migrator preserving adefault: onmixdown beside instrument stems.Not done here
The pack builder lives outside this repo; it has been updated to emit the
fullstem and no key. The library migration has not been run — the script is delivered for @byrongamatos to run.Summary by CodeRabbit
fullstem as the dedicated full-mix source, improving full-mix fingerprinting coverage when available.song_infoto exposefull_mix_url/has_full_mix, with deprecatedoriginal_audio_*aliases retained for one release.has_full_mixwhen stems are absent.fullisn’t double-counted for separated packs.