From 13eeca6ff8ef57c9b1226815e53e11885cf36292 Mon Sep 17 00:00:00 2001 From: Kris Anderson Date: Mon, 13 Jul 2026 12:14:08 -0400 Subject: [PATCH] fix(migrate): verify requires an explicit `off` on a retained full mix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/test_migrate_full_mix_stem.py | 53 +++++++++++++++++++++++++++++ tools/migrate_full_mix_stem.py | 20 +++++++++-- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/tests/test_migrate_full_mix_stem.py b/tests/test_migrate_full_mix_stem.py index 7e43b66..8865995 100644 --- a/tests/test_migrate_full_mix_stem.py +++ b/tests/test_migrate_full_mix_stem.py @@ -264,6 +264,59 @@ def test_verify_rejects_a_retained_mixdown_that_plays_on_open(tmp_path: Path): assert mig.verify_zip(pak) == "full-stem-default-on" +@pytest.mark.parametrize( + "default, expected", + [ + ({"default": "off"}, "ok"), # the one safe, canonical shape + ({"default": "OFF"}, "ok"), # case-insensitive + ({"default": " off "}, "ok"), # surrounding whitespace tolerated + ({}, "full-stem-default-not-off"), # MISSING — core defaults to True (ON) + ({"default": ""}, "full-stem-default-not-off"), # empty → ON in core + ({"default": False}, "full-stem-default-not-off"), # boolean, not the string + ({"default": True}, "full-stem-default-on"), # boolean truthy → plays + ({"default": "false"}, "full-stem-default-not-off"), # off-ish but non-canonical + ({"default": "0"}, "full-stem-default-not-off"), + ({"default": "no"}, "full-stem-default-not-off"), + ({"default": "maybe"}, "full-stem-default-not-off"), # malformed + ({"default": "on"}, "full-stem-default-on"), + ({"default": "yes"}, "full-stem-default-on"), + ({"default": "1"}, "full-stem-default-on"), + ], +) +def test_verify_requires_an_explicit_off_on_a_retained_mixdown(tmp_path, default, expected): + """Beside instrument stems, `full` is safe only with an explicit normalized + `off`. Core defaults an ABSENT `default` to ON and treats empty/unknown as + ON, so a missing or blank default is the double-audio hazard itself, not a + lesser one — `verify` must not certify it.""" + m = _manifest( + stems=[ + {"id": "full", "file": "stems/full.ogg", **default}, + {"id": "guitar", "file": "stems/guitar.ogg", "default": "on"}, + ] + ) + del m["original_audio"] + pak = _write_pack( + tmp_path / f"{tmp_path.name}.feedpak", + m, + files={"stems/full.ogg": b"M", "stems/guitar.ogg": b"g"}, + ) + assert mig.verify_zip(pak) == expected + + +def test_verify_ignores_default_on_a_sole_full_stem(tmp_path: Path): + """A single `full` stem IS the audio — the len>1 gate means its default is + not policed, so an on/absent default is fine (off would mute the pack).""" + for default in ({"default": "on"}, {}, {"default": ""}): + m = _manifest(stems=[{"id": "full", "file": "stems/full.ogg", **default}]) + del m["original_audio"] + pak = _write_pack( + tmp_path / f"{tmp_path.name}-{len(default)}.feedpak", + m, + files={"stems/full.ogg": b"M"}, + ) + assert mig.verify_zip(pak) == "ok" + + def test_verify_rejects_an_unmigrated_pack(tmp_path: Path): pak = _write_pack(tmp_path / "song.feedpak", _manifest()) assert mig.verify_zip(pak) == "still-has-key" diff --git a/tools/migrate_full_mix_stem.py b/tools/migrate_full_mix_stem.py index 0599403..894e132 100644 --- a/tools/migrate_full_mix_stem.py +++ b/tools/migrate_full_mix_stem.py @@ -256,8 +256,24 @@ def verify_zip(path: Path) -> str: return "full-stem-missing-file" # A retained mixdown that plays on open would double the mix in any reader # that sums the stem list — the whole hazard this migration must not create. - if len(stems) > 1 and str(full.get("default", "")).lower() in ("true", "on", "yes", "1"): - return "full-stem-default-on" + # Beside instrument stems, `full` MUST carry an explicit, normalized "off": + # - core (lib/sloppak.py) defaults an ABSENT `default` to True (ON) and + # treats an empty / unrecognized string as ON, so a missing or blank + # default is not merely non-canonical — core would play the mixdown on + # open, doubling the song. It is the exact hazard, not a lesser one. + # - the migrator always writes the literal "off", so requiring it also + # certifies the pack is in the shape this tool produces — the most + # portable spelling, understood even by a reader that only knows + # "on"/"off" and would choke on a boolean or `false`/`0`/`no`. + # So: `on`-ish values are reported as actively-playing; everything that is + # not a normalized "off" (missing, empty, boolean, `false`/`no`/`0`, + # malformed) is reported as an unsafe/non-canonical default. + if len(stems) > 1: + default = str(full.get("default", "")).strip().lower() + if default in ("true", "on", "yes", "1"): + return "full-stem-default-on" + if default != "off": + return "full-stem-default-not-off" return "ok"