mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-10 18:59:56 +00:00
* fix(sloppak): the full mix is a stem — drop the invented `original_audio` 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. * fix(migrate): discover directory-form packs instead of silently skipping 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> * fix(migrate): verify requires an explicit `off` on a retained full mix 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> --------- Signed-off-by: Kris Anderson <topkoa@gmail.com> Co-authored-by: Kris Anderson <topkoa@gmail.com>
278 lines
10 KiB
Python
278 lines
10 KiB
Python
"""The sloppak loader's handling of a pack's complete mixdown (#933).
|
|
|
|
The mixdown is a stem: feedpak spec §5.3 RESERVES the id `full` for it. It is a
|
|
mixdown, not a layer — it already contains every instrument, so a reader that
|
|
sums `stems` must never include it in that sum, and `load_song()` therefore
|
|
lifts it OUT of `LoadedSloppak.stems` and onto `LoadedSloppak.full_mix`.
|
|
|
|
Also covers the DEPRECATED `original_audio:` manifest key — a key this repo
|
|
invented (#583) before the spec reserved `full`, which every pack in the wild
|
|
still carries. We read it as a fallback so those packs keep their full mix; we
|
|
never write it. Those tests are the deprecation contract: they go when the key
|
|
does (#945).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
from pathlib import Path
|
|
|
|
import yaml
|
|
|
|
import sloppak as sloppak_mod
|
|
|
|
|
|
def _write_dir_sloppak(
|
|
root: Path,
|
|
manifest_extras: dict,
|
|
*,
|
|
write_legacy_full_mix: bool = False,
|
|
stems: list[dict] | None = None,
|
|
) -> Path:
|
|
"""Build a minimal directory-form sloppak that load_song will accept.
|
|
|
|
Uses the tmp_path leaf name to make the sloppak filename unique per test,
|
|
avoiding the module-level ``resolve_source_dir`` cache being poisoned by a
|
|
previous test that happened to share the same "song.sloppak" filename.
|
|
"""
|
|
pak = root / f"{root.name}.sloppak"
|
|
pak.mkdir()
|
|
arr_dir = pak / "arrangements"
|
|
arr_dir.mkdir()
|
|
|
|
arr = {
|
|
"name": "Lead",
|
|
"tuning": [0, 0, 0, 0, 0, 0],
|
|
"capo": 0,
|
|
"notes": [],
|
|
"chords": [],
|
|
"anchors": [],
|
|
"handshapes": [],
|
|
"templates": [],
|
|
"beats": [],
|
|
"sections": [],
|
|
}
|
|
(arr_dir / "lead.json").write_text(json.dumps(arr))
|
|
|
|
manifest = {
|
|
"title": "Test",
|
|
"artist": "Tester",
|
|
"album": "",
|
|
"year": 2026,
|
|
"duration": 10.0,
|
|
"arrangements": [{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}],
|
|
"stems": (
|
|
stems
|
|
if stems is not None
|
|
else [{"id": "guitar", "file": "stems/guitar.ogg", "default": True}]
|
|
),
|
|
}
|
|
manifest.update(manifest_extras)
|
|
(pak / "manifest.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False))
|
|
|
|
if write_legacy_full_mix:
|
|
orig_dir = pak / "original"
|
|
orig_dir.mkdir()
|
|
# The loader only checks presence (is_file); contents are irrelevant.
|
|
(orig_dir / "full.ogg").write_bytes(b"OggS-not-real")
|
|
|
|
return pak
|
|
|
|
|
|
def _load(pak_path: Path, tmp_path: Path):
|
|
dlc_root = pak_path.parent
|
|
cache = tmp_path / "cache"
|
|
cache.mkdir()
|
|
return sloppak_mod.load_song(pak_path.name, dlc_root, cache)
|
|
|
|
|
|
def _separated(**extra) -> list[dict]:
|
|
"""A separated pack that RETAINS its mixdown, as spec §5.3 asks writers to."""
|
|
return [
|
|
{"id": "full", "file": "stems/full.ogg", "default": False, **extra},
|
|
{"id": "guitar", "file": "stems/guitar.ogg", "default": True},
|
|
{"id": "drums", "file": "stems/drums.ogg", "default": True},
|
|
]
|
|
|
|
|
|
# ── The `full` stem is the mixdown (spec §5.3) ───────────────────────────────
|
|
|
|
def test_full_stem_is_surfaced_as_the_mixdown(tmp_path: Path):
|
|
pak = _write_dir_sloppak(tmp_path, {}, stems=_separated())
|
|
loaded = _load(pak, tmp_path)
|
|
# Manifest-relative, so the WS builds its URL exactly as it builds a stem's.
|
|
assert loaded.full_mix == "stems/full.ogg"
|
|
|
|
|
|
def test_full_stem_is_removed_from_the_stem_list(tmp_path: Path):
|
|
"""The regression this whole change exists to prevent.
|
|
|
|
Every consumer sums `stems` into one mix and renders one fader per entry. The
|
|
mixdown already contains every instrument, so leaving it in the list doubles
|
|
the entire song — and muting `guitar` would still leave guitar audible inside
|
|
it. That exact trap is why the packer invented `original_audio` rather than
|
|
putting the mixdown where the format says it goes.
|
|
"""
|
|
pak = _write_dir_sloppak(tmp_path, {}, stems=_separated())
|
|
loaded = _load(pak, tmp_path)
|
|
assert [s["id"] for s in loaded.stems] == ["guitar", "drums"]
|
|
|
|
|
|
def test_single_mix_pack_keeps_full_as_its_only_stem(tmp_path: Path):
|
|
"""A pack whose ONLY stem is `full` is a single-mix pack, not a separated one.
|
|
|
|
There are no instruments to be pristine against, so the mixdown stays the sole
|
|
playable stem and nothing is surfaced separately. Anything else would strip the
|
|
stem list of the most common pack shape in the library and leave it silent.
|
|
"""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {}, stems=[{"id": "full", "file": "stems/full.ogg", "default": True}]
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
assert [s["id"] for s in loaded.stems] == ["full"]
|
|
|
|
|
|
def test_every_full_entry_is_removed_not_just_the_first(tmp_path: Path):
|
|
"""A malformed pack listing `full` twice must not leave one behind.
|
|
|
|
Removing the mixdown by object identity would drop only the entry we surface
|
|
and leave its duplicate in the stem list — a whole copy of the song, summed
|
|
with the instruments. That is the exact bug this partition prevents, so a
|
|
duplicate must not smuggle it back in.
|
|
"""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path,
|
|
{},
|
|
stems=[
|
|
{"id": "full", "file": "stems/full.ogg", "default": False},
|
|
{"id": "guitar", "file": "stems/guitar.ogg", "default": True},
|
|
{"id": "full", "file": "original/full.ogg", "default": True},
|
|
],
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix == "stems/full.ogg"
|
|
assert [s["id"] for s in loaded.stems] == ["guitar"]
|
|
|
|
|
|
def test_separated_pack_without_a_full_stem_has_no_mixdown(tmp_path: Path):
|
|
"""Stems only, mixdown discarded — the pre-1.15.0 shape. Nothing to surface."""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path,
|
|
{},
|
|
stems=[
|
|
{"id": "guitar", "file": "stems/guitar.ogg", "default": True},
|
|
{"id": "drums", "file": "stems/drums.ogg", "default": True},
|
|
],
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
assert [s["id"] for s in loaded.stems] == ["guitar", "drums"]
|
|
|
|
|
|
def test_single_mix_pack_ignores_a_lingering_deprecated_key(tmp_path: Path):
|
|
"""`full` is the pack's only stem AND the old key is still there.
|
|
|
|
The stem wins, and it stays the sole playable stem — falling back to the key
|
|
would surface the mixdown twice: once as the stem the player is already
|
|
playing, and once as a "pristine" track for it to cross over to.
|
|
"""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path,
|
|
{"original_audio": "original/full.ogg"},
|
|
stems=[{"id": "full", "file": "stems/full.ogg", "default": True}],
|
|
write_legacy_full_mix=True,
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
assert [s["id"] for s in loaded.stems] == ["full"]
|
|
|
|
|
|
def test_full_stem_wins_over_the_deprecated_key(tmp_path: Path):
|
|
"""A migrated pack that still carries the old key must use the stem."""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path,
|
|
{"original_audio": "original/full.ogg"},
|
|
stems=_separated(),
|
|
write_legacy_full_mix=True,
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix == "stems/full.ogg"
|
|
|
|
|
|
# ── The library index must not advertise the mixdown as an instrument ────────
|
|
|
|
def test_extract_meta_excludes_the_mixdown_from_stem_ids(tmp_path: Path):
|
|
"""The library's stem chips / stem_count come from here, and must agree with
|
|
load_song() — otherwise the filter offers a "full" chip beside guitar+drums
|
|
and counts a third stem that no mixer will ever show."""
|
|
pak = _write_dir_sloppak(tmp_path, {}, stems=_separated())
|
|
meta = sloppak_mod.extract_meta(pak)
|
|
assert meta["stem_ids"] == ["guitar", "drums"]
|
|
assert meta["stem_count"] == 2
|
|
|
|
|
|
def test_extract_meta_keeps_full_for_a_single_mix_pack(tmp_path: Path):
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {}, stems=[{"id": "full", "file": "stems/full.ogg", "default": True}]
|
|
)
|
|
meta = sloppak_mod.extract_meta(pak)
|
|
assert meta["stem_ids"] == ["full"]
|
|
assert meta["stem_count"] == 1
|
|
|
|
|
|
# ── DEPRECATED `original_audio:` fallback — delete with the key (#945) ───────
|
|
|
|
def test_legacy_key_still_provides_the_full_mix(tmp_path: Path):
|
|
"""Every pack written before the spec reserved `full` looks like this. Dropping
|
|
the read would silently take the pristine mix away from all of them."""
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {"original_audio": "original/full.ogg"}, write_legacy_full_mix=True
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix == "original/full.ogg"
|
|
# The legacy mixdown lives OUTSIDE `stems`, so the stem list is untouched.
|
|
assert [s["id"] for s in loaded.stems] == ["guitar"]
|
|
|
|
|
|
def test_legacy_key_absent_means_no_full_mix(tmp_path: Path):
|
|
pak = _write_dir_sloppak(tmp_path, {}, write_legacy_full_mix=True)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
|
|
|
|
def test_legacy_key_none_when_file_missing(tmp_path: Path):
|
|
# Manifest points at a full mix that isn't on disk — disabled silently.
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {"original_audio": "original/full.ogg"}, write_legacy_full_mix=False
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
|
|
|
|
def test_legacy_key_none_when_value_blank(tmp_path: Path):
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {"original_audio": " "}, write_legacy_full_mix=True
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
|
|
|
|
# ── Security / path-traversal branches (legacy key only — a stem `file` is
|
|
# resolved through the same /api/sloppak/.../file/ guard as every other stem)
|
|
|
|
def test_legacy_key_none_when_path_escapes_sloppak(tmp_path: Path):
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {"original_audio": "../outside.ogg"}, write_legacy_full_mix=True
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|
|
|
|
|
|
def test_legacy_key_none_when_path_is_absolute(tmp_path: Path):
|
|
pak = _write_dir_sloppak(
|
|
tmp_path, {"original_audio": "/etc/passwd"}, write_legacy_full_mix=True
|
|
)
|
|
loaded = _load(pak, tmp_path)
|
|
assert loaded.full_mix is None
|