From 435e27392778f1b5e3eab5b6192c1d3acb49a101 Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 23 Jul 2026 00:33:50 +0200 Subject: [PATCH 1/4] Carry rig bindings through the sloppak tone payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sloppak_tone_changes` emitted `{t, name}` only, so a chart's declared sound never reached the client: `base_rig` was never read and each change's `rig` was dropped at the wire boundary. Both survive load intact (`Arrangement.tones` is an opaque passthrough) — the strip happened here, at the last step before send. That left the rig model (feedpak-spec 1.18.0 §6.9/§7.9) unreachable from core: a pack could declare which rig voices a part, and nothing downstream could ever see it. First step of the core reader for source rigs; the rig library itself and the manifest precedence cascade follow. Return `(base, base_rig, changes)` and keep `rig` on each change. Both ids are validated as non-blank strings and stripped — anything else is dropped rather than forwarded, so presence of the key means the change binds a rig. Resolution against `rigs.json` deliberately does NOT happen here: this builder preserves the declared binding, while realization selection and the `intent.gm` fallback belong to whatever voices the part. On the wire `base_rig` is omitted entirely when empty, so packs that bind no rig produce the byte-identical `tone_changes` message they always did. Signed-off-by: gionnibgud --- lib/routers/ws_highway.py | 21 +++++++++---- lib/tones.py | 34 ++++++++++++++------ tests/test_tones.py | 65 ++++++++++++++++++++++++++++++++++----- 3 files changed, 97 insertions(+), 23 deletions(-) diff --git a/lib/routers/ws_highway.py b/lib/routers/ws_highway.py index 61cb65b6..02b27568 100644 --- a/lib/routers/ws_highway.py +++ b/lib/routers/ws_highway.py @@ -774,20 +774,29 @@ def _evict_audio_cache(): # (Arrangement.tones, populated by the converter), so read it straight # off `arr` rather than walking for XML that doesn't exist. if is_slop: - # `sloppak_tone_changes` builds the (base, sorted changes) pair - # from `Arrangement.tones`, skipping non-string names and - # non-finite/non-numeric times — unit-tested in test_tones.py. + # `sloppak_tone_changes` builds the (base, base_rig, sorted + # changes) triple from `Arrangement.tones`, skipping non-string + # names, non-finite/non-numeric times, and unusable rig ids — + # unit-tested in test_tones.py. from tones import sloppak_tone_changes - base_name, tone_changes = sloppak_tone_changes(getattr(arr, "tones", None)) + base_name, base_rig, tone_changes = sloppak_tone_changes( + getattr(arr, "tones", None) + ) # Send when there's a base tone OR timed changes — a single-tone # arrangement has a base but no switches, and the highway should # still be able to show the initial tone. if tone_changes or base_name: - await websocket.send_json({ + payload = { "type": "tone_changes", "base": base_name, "data": tone_changes, - }) + } + # `base_rig` is additive (feedpak-spec §6.9) — omitted entirely + # when the chart binds no rig, so consumers that predate the rig + # model see the exact payload they always did. + if base_rig: + payload["base_rig"] = base_rig + await websocket.send_json(payload) else: xml_paths = sorted(_xml_walk("*.xml")) diff --git a/lib/tones.py b/lib/tones.py index 3d56134f..91371631 100644 --- a/lib/tones.py +++ b/lib/tones.py @@ -32,20 +32,29 @@ def tokens(s: str) -> set[str]: return {t for t in re.split(r"[^a-z0-9]+", (s or "").lower()) if t} -def sloppak_tone_changes(arr_tones) -> tuple[str, list[dict]]: +def sloppak_tone_changes(arr_tones) -> tuple[str, str, list[dict]]: """Build the highway tone-change payload from an arrangement's tone block. Given ``Arrangement.tones`` (the dict embedded in the sloppak, or ``None``), - returns ``(base, changes)`` where ``base`` is the initial tone name and - ``changes`` is a time-sorted ``[{"t", "name"}]`` list. Non-string names, - non-dict entries, and non-numeric / non-finite times are skipped — a - hand-edited or third-party sloppak must not crash the highway WebSocket - or emit NaN/inf (which the client's ``JSON.parse`` rejects). + returns ``(base, base_rig, changes)`` where ``base`` is the initial tone + name, ``base_rig`` is the ``rigs.json`` rig id bound to it (feedpak-spec + §6.9; ``""`` when absent), and ``changes`` is a time-sorted + ``[{"t", "name", "rig"?}]`` list. Non-string names, non-dict entries, and + non-numeric / non-finite times are skipped — a hand-edited or third-party + sloppak must not crash the highway WebSocket or emit NaN/inf (which the + client's ``JSON.parse`` rejects). + + ``rig`` / ``base_rig`` are carried through but NOT resolved against + ``rigs.json`` here: this builder only preserves the binding the chart + declared. Realization selection and the ``intent.gm`` fallback (§7.9) belong + to the consumer that actually voices the part. """ if not isinstance(arr_tones, dict): - return "", [] + return "", "", [] base_val = arr_tones.get("base", "") base = base_val.strip() if isinstance(base_val, str) else "" + base_rig_val = arr_tones.get("base_rig", "") + base_rig = base_rig_val.strip() if isinstance(base_rig_val, str) else "" changes: list[dict] = [] raw_changes = arr_tones.get("changes") @@ -65,6 +74,13 @@ def sloppak_tone_changes(arr_tones) -> tuple[str, list[dict]]: continue if not math.isfinite(t): continue - changes.append({"t": round(t, 3), "name": name}) + change = {"t": round(t, 3), "name": name} + # ponytail: `rig` only when it's a usable id — a non-string or blank + # value is dropped rather than forwarded, so a consumer can treat + # presence of the key as "this change binds a rig". + rig = c.get("rig") + if isinstance(rig, str) and rig.strip(): + change["rig"] = rig.strip() + changes.append(change) changes.sort(key=lambda x: x["t"]) - return base, changes + return base, base_rig, changes diff --git a/tests/test_tones.py b/tests/test_tones.py index a5b83a8a..3eb8db55 100644 --- a/tests/test_tones.py +++ b/tests/test_tones.py @@ -6,16 +6,17 @@ # ── sloppak_tone_changes (highway payload builder) ─────────────────────────── def test_sloppak_tone_changes_sorts_and_returns_base(): - base, changes = sloppak_tone_changes({ + base, base_rig, changes = sloppak_tone_changes({ "base": "Clean", "changes": [{"t": 12.5, "name": "Drive"}, {"t": 3.0, "name": "Clean"}], }) assert base == "Clean" + assert base_rig == "" assert changes == [{"t": 3.0, "name": "Clean"}, {"t": 12.5, "name": "Drive"}] def test_sloppak_tone_changes_skips_malformed_markers(): - _, changes = sloppak_tone_changes({ + _, _, changes = sloppak_tone_changes({ "changes": [ {"t": "nan", "name": "BadStr"}, {"t": float("inf"), "name": "Inf"}, @@ -29,18 +30,66 @@ def test_sloppak_tone_changes_skips_malformed_markers(): def test_sloppak_tone_changes_handles_none_and_bad_base(): - assert sloppak_tone_changes(None) == ("", []) - base, changes = sloppak_tone_changes({"base": 123, "changes": []}) - assert base == "" and changes == [] + assert sloppak_tone_changes(None) == ("", "", []) + base, base_rig, changes = sloppak_tone_changes({"base": 123, "changes": []}) + assert base == "" and base_rig == "" and changes == [] def test_sloppak_tone_changes_non_dict_input(): """A truthy non-dict payload must not crash.""" - assert sloppak_tone_changes(["not", "a", "dict"]) == ("", []) - assert sloppak_tone_changes("nope") == ("", []) + assert sloppak_tone_changes(["not", "a", "dict"]) == ("", "", []) + assert sloppak_tone_changes("nope") == ("", "", []) def test_sloppak_tone_changes_non_list_changes(): """A truthy non-list `changes` value must not raise on iteration.""" - base, changes = sloppak_tone_changes({"base": "Clean", "changes": 1}) + base, _, changes = sloppak_tone_changes({"base": "Clean", "changes": 1}) assert base == "Clean" and changes == [] + + +# ── rig bindings (feedpak-spec 1.18.0 §6.9) ────────────────────────────────── + +def test_sloppak_tone_changes_carries_rig_bindings(): + """`base_rig` and per-change `rig` reach the wire — the binding a chart + declares is what core must hand the consumer that voices the part.""" + base, base_rig, changes = sloppak_tone_changes({ + "base": "Clean Rhythm", + "base_rig": "clean-rhythm", + "changes": [ + {"t": 12.5, "name": "Lead Drive", "rig": "lead-drive"}, + {"t": 48.0, "name": "Clean Rhythm", "rig": "clean-rhythm"}, + ], + }) + assert base == "Clean Rhythm" + assert base_rig == "clean-rhythm" + assert changes == [ + {"t": 12.5, "name": "Lead Drive", "rig": "lead-drive"}, + {"t": 48.0, "name": "Clean Rhythm", "rig": "clean-rhythm"}, + ] + + +def test_sloppak_tone_changes_omits_unusable_rig_ids(): + """A non-string or blank `rig` is dropped rather than forwarded, so a + consumer can treat presence of the key as "this change binds a rig".""" + _, base_rig, changes = sloppak_tone_changes({ + "base_rig": " ", + "changes": [ + {"t": 1.0, "name": "A", "rig": 7}, + {"t": 2.0, "name": "B", "rig": ""}, + {"t": 3.0, "name": "C", "rig": None}, + {"t": 4.0, "name": "D", "rig": " padded-id "}, + ], + }) + assert base_rig == "" + assert changes == [ + {"t": 1.0, "name": "A"}, + {"t": 2.0, "name": "B"}, + {"t": 3.0, "name": "C"}, + {"t": 4.0, "name": "D", "rig": "padded-id"}, + ] + + +def test_sloppak_tone_changes_non_string_base_rig(): + """A non-string `base_rig` must not crash or leak a non-id onto the wire.""" + _, base_rig, _ = sloppak_tone_changes({"base": "Clean", "base_rig": 42}) + assert base_rig == "" From 9075939668c1ef20e30e3b5ff03fafdb17b0339c Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 23 Jul 2026 08:45:40 +0200 Subject: [PATCH 2/4] Load the pack's rig library from the manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit feedpak 1.18.0 lets a chart declare what a MIDI part should sound like by binding a rig id, but core had nothing to bind to: `rigs`, `base_rig` and `drum_tones` appeared nowhere in lib/, server.py or static/. The preceding commit carries the reference onto the wire; this adds the library it references. Read the manifest `rigs:` key into a new `LoadedSloppak.rigs`, alongside the other side-files rather than on Song — every side-file (drum_tab, song_timeline, keys, notation) hangs off the load result, and rigs is pack-level, not per-arrangement. Same permissive posture as its neighbours: missing, unreadable, malformed or traversing disables rigs with a warning and never fails the pack, which §7.9 requires outright. Rig objects pass through VERBATIM. §7.9 obliges a Reader to preserve unknown role/engine/kind values and `ext` namespaces, so validating block structure here would be wrong as well as premature — realization selection and the `intent.gm` floor belong to whatever voices the part. The only entries dropped are ones unreachable by construction: a rig is addressable solely by `id`, so a non-dict entry or one without a usable string id can never be referenced. Ids are stripped to match the reference side, and a duplicate id resolves first-wins with a warning, since ambiguity there would surface as the wrong sound rather than an error. Signed-off-by: gionnibgud --- lib/sloppak.py | 88 ++++++++++++++ tests/test_sloppak_rigs_load.py | 207 ++++++++++++++++++++++++++++++++ 2 files changed, 295 insertions(+) create mode 100644 tests/test_sloppak_rigs_load.py diff --git a/lib/sloppak.py b/lib/sloppak.py index 1081faac..cacd8648 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -698,6 +698,14 @@ class LoadedSloppak: # absent / unreadable / malformed. Streamed over the highway WS as a # `keys` message; consumers (renderers, plugins) read it from there. keys: dict | None = None + # Parsed `rigs.json` payload (manifest `rigs:` key, spec §7.9) — the pack's + # library of engine-agnostic signal chains: effect chains and, since + # feedpak 1.18.0, MIDI-voiced sound sources. Arrangements bind rigs to time + # by referencing a rig `id` from `tones.base_rig` / `tones.changes[].rig` + # (§6.9), which `lib/tones.py` carries onto the wire. None when absent / + # unreadable / malformed. Rig objects are kept verbatim — this loader does + # not select realizations or apply the `intent.gm` floor. + rigs: dict | None = None # Sanitized song-level tempo + time-signature maps from `song_timeline.json` # (feedpak 1.2.0). `tempos`: [{time, bpm}]; `time_signatures`: [{time, ts}]. # None when absent/empty. Streamed over the highway WS (`tempos` / @@ -776,6 +784,77 @@ def _load_drum_tab_file(source_dir: Path, rel: str, label: str) -> dict | None: return raw +def _load_rigs_file(source_dir: Path, rel: str) -> dict | None: + """Load the pack's rig library (manifest `rigs:` key, spec §7.9). + + Returns `{"version": int, "rigs": [...]}` or None. Same permissive posture + as every other side-file: missing / unreadable / malformed -> None, never + fatal — spec §7.9 is explicit that a rig library a Reader can't use MUST NOT + fail the pack. + + Rig objects are kept **verbatim**. Only entries that could never be + addressed are dropped — a rig is reachable solely by `id` (from + `tones.base_rig` / `changes[].rig`), so a non-dict entry or one without a + usable string id is unreferenceable by construction. Everything else, + including unknown `role` / `engine` / `kind` values and `ext` namespaces, + passes through untouched, because this loader does not interpret rigs: + realization selection and the `intent.gm` fallback belong to whatever + voices the part. + """ + try: + r_path = (source_dir / rel).resolve() + r_path.relative_to(source_dir.resolve()) + except ValueError: + log.warning("sloppak: rigs path %r escapes source_dir — skipped", rel) + return None + except OSError as e: + log.warning("sloppak: rigs path resolution failed (%s) — skipped", e) + return None + if not r_path.exists(): + return None + try: + raw = load_json(r_path) + except Exception as e: + log.warning("sloppak: failed to parse rigs %r: %s", rel, e) + return None + if not isinstance(raw, dict): + log.warning("sloppak: rigs %r ignored — expected dict, got %s", + rel, type(raw).__name__) + return None + if not isinstance(raw.get("rigs"), list): + log.warning("sloppak: rigs %r ignored — 'rigs' must be a list", rel) + return None + + clean_rigs: list[dict] = [] + seen: set[str] = set() + for rig in raw["rigs"]: + if not isinstance(rig, dict): + continue + rid = rig.get("id") + if not isinstance(rid, str) or not rid.strip(): + continue + # Normalize the library side of the lookup the same way the reference + # side is normalized in lib/tones.py — otherwise a pack with padded ids + # fails to resolve against a stripped `base_rig` / `rig`. + rid = rid.strip() + # A duplicate id makes `tones.base_rig` ambiguous, which would surface + # as the wrong sound rather than an error. First wins, loudly. + if rid in seen: + log.warning("sloppak: rigs %r has duplicate rig id %r — later one ignored", + rel, rid) + continue + seen.add(rid) + clean_rigs.append({**rig, "id": rid}) + + # int only — a float version (incl. NaN/Inf, which json.loads accepts) + # would raise on int(); default rather than abort an optional side-file. + _ver = raw.get("version") + return { + "version": _ver if isinstance(_ver, int) and not isinstance(_ver, bool) else 1, + "rigs": clean_rigs, + } + + def _resolve_drum_parts( source_dir: Path, drum_tab_rel: object, @@ -1325,6 +1404,14 @@ def load_song( "events": clean_events, } + # Optional rigs.json — the pack's rig library (manifest `rigs:` key, + # spec §7.9). Loaded here so the highway WS can hand it to whatever voices + # the part; the bindings that reference it ride the arrangement's `tones`. + rigs_data: dict | None = None + rigs_rel = manifest.get("rigs") + if isinstance(rigs_rel, str) and rigs_rel: + rigs_data = _load_rigs_file(source_dir, rigs_rel) + _fpv = manifest.get("feedpak_version") # The pack's full mix. Normally the RESERVED `full` stem partitioned out # above (spec §5.3) — no path work needed, it was validated with the other @@ -1355,6 +1442,7 @@ def load_song( tempos=tempos_data, time_signatures=time_sigs_data, keys=keys_data, + rigs=rigs_data, notation_by_id=notation_by_id_data, arrangement_ids=arrangement_ids_acc, full_mix=full_mix_data, diff --git a/tests/test_sloppak_rigs_load.py b/tests/test_sloppak_rigs_load.py new file mode 100644 index 00000000..4ce5779e --- /dev/null +++ b/tests/test_sloppak_rigs_load.py @@ -0,0 +1,207 @@ +"""End-to-end test for the sloppak loader recognising a `rigs:` manifest key +(rigs.json — the pack-level library of engine-agnostic rigs, spec §7.9) and +surfacing the payload on the LoadedSloppak. + +The governing posture: rig objects pass through VERBATIM. This loader does not +select realizations or apply the `intent.gm` floor — it only makes the library +addressable by `id`, which is what `tones.base_rig` / `tones.changes[].rig` +reference.""" + +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, rigs_payload) -> Path: + """Minimal directory-form sloppak; writes rigs.json when a payload is given. + + Unique filename per test (tmp_path leaf) so the module-level + resolve_source_dir cache isn't poisoned across tests.""" + 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": [{"id": "full", "file": "stems/full.ogg", "default": True}], + } + manifest.update(manifest_extras) + (pak / "manifest.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False)) + + if rigs_payload is not None: + (pak / "rigs.json").write_text(json.dumps(rigs_payload)) + 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) + + +# ── Happy path ─────────────────────────────────────────────────────────────── + +def test_load_song_attaches_rigs_when_manifest_opts_in(tmp_path: Path): + """A source rig (spec §7.9 1.18.0) survives the load intact — including the + `soundfont` realization and the `intent.gm` floor a consumer needs to voice + the part.""" + payload = { + "version": 1, + "rigs": [ + { + "id": "grand-piano", + "name": "Grand Piano", + "instrument": "keys", + "blocks": [ + { + "role": "source", + "name": "Concert Grand", + "intent": {"kind": "instrument", "gm": {"program": 0}}, + "realizations": [ + {"engine": "soundfont", "format": "sf2", + "ref": "sounds/grand.sf2", "bank": 0, "program": 0}, + ], + }, + ], + }, + ], + } + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, payload) + loaded = _load(pak, tmp_path) + assert loaded.rigs is not None + assert loaded.rigs["version"] == 1 + assert loaded.rigs["rigs"] == payload["rigs"] + + +def test_load_song_rigs_absent_without_manifest_key(tmp_path: Path): + """The file alone must not opt a pack in — the manifest is the opt-in + (spec §9.1, "manifest opt-in, file off to the side").""" + pak = _write_dir_sloppak(tmp_path, {}, {"version": 1, "rigs": []}) + assert _load(pak, tmp_path).rigs is None + + +# ── Verbatim passthrough ───────────────────────────────────────────────────── + +def test_load_song_preserves_unknown_rig_content(tmp_path: Path): + """Unknown `role` / `engine` / `kind` values and `ext` namespaces MUST + survive (spec §7.9) — core does not interpret rigs, so it must not prune + what a newer writer or a plugin put there.""" + payload = { + "version": 2, + "rigs": [ + { + "id": "future-rig", + "blocks": [ + {"role": "quantum-flux", "intent": {"kind": "not-yet-invented"}, + "realizations": [{"engine": "some-future-engine", "ref": "x.bin"}], + "ext": {"vendor.custom": {"anything": [1, 2, 3]}}}, + ], + "graph": {"nodes": ["input", "output"], "edges": [["input", "output"]]}, + "ext": {"vendor.rig": "kept"}, + }, + ], + } + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, payload) + loaded = _load(pak, tmp_path) + assert loaded.rigs["version"] == 2 + assert loaded.rigs["rigs"] == payload["rigs"] + + +# ── Addressability ─────────────────────────────────────────────────────────── + +def test_load_song_drops_unaddressable_rigs_and_normalizes_ids(tmp_path: Path): + """A rig is reachable only by `id`, so entries without a usable one are + unreferenceable by construction. Ids are stripped to match the reference + side, which lib/tones.py strips before it reaches the wire.""" + payload = { + "rigs": [ + "not-a-dict", + {"name": "no id at all"}, + {"id": "", "name": "blank id"}, + {"id": " ", "name": "whitespace id"}, + {"id": 7, "name": "non-string id"}, + {"id": " padded-rig ", "name": "Padded"}, + {"id": "plain-rig", "name": "Plain"}, + ], + } + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, payload) + loaded = _load(pak, tmp_path) + assert [r["id"] for r in loaded.rigs["rigs"]] == ["padded-rig", "plain-rig"] + # Everything except the normalized id is untouched. + assert loaded.rigs["rigs"][0]["name"] == "Padded" + # `version` defaults when the file omits it. + assert loaded.rigs["version"] == 1 + + +def test_load_song_first_rig_wins_on_duplicate_id(tmp_path: Path): + """A duplicate id makes `tones.base_rig` ambiguous, which would surface as + the wrong sound rather than an error.""" + payload = { + "rigs": [ + {"id": "dupe", "name": "First"}, + {"id": "dupe", "name": "Second"}, + {"id": " dupe ", "name": "Third, padded into a collision"}, + ], + } + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, payload) + loaded = _load(pak, tmp_path) + assert len(loaded.rigs["rigs"]) == 1 + assert loaded.rigs["rigs"][0]["name"] == "First" + + +# ── Permissive posture (spec §7.9: never fail the pack) ────────────────────── + +def test_load_song_survives_malformed_rigs(tmp_path: Path): + """Malformed / missing / traversing rig libraries disable rigs, never the + pack — the song itself must still load.""" + cases = [ + {"version": 1, "rigs": "not-a-list"}, # wrong `rigs` type + ["top-level-not-a-dict"], # wrong document type + {"version": 1}, # no `rigs` key at all + ] + for i, payload in enumerate(cases): + sub = tmp_path / f"case{i}" + sub.mkdir() + pak = _write_dir_sloppak(sub, {"rigs": "rigs.json"}, payload) + loaded = _load(pak, sub) + assert loaded.rigs is None, f"case {i} should disable rigs" + assert loaded.song is not None, f"case {i} must not fail the pack" + + +def test_load_song_survives_unparseable_rigs(tmp_path: Path): + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, None) + (pak / "rigs.json").write_text("{ not json at all ") + loaded = _load(pak, tmp_path) + assert loaded.rigs is None + assert loaded.song is not None + + +def test_load_song_survives_missing_rigs_file(tmp_path: Path): + pak = _write_dir_sloppak(tmp_path, {"rigs": "rigs.json"}, None) + loaded = _load(pak, tmp_path) + assert loaded.rigs is None + assert loaded.song is not None + + +def test_load_song_rejects_traversing_rigs_path(tmp_path: Path): + """A crafted manifest must not read outside the pack.""" + (tmp_path / "outside.json").write_text(json.dumps({"rigs": [{"id": "leaked"}]})) + pak = _write_dir_sloppak(tmp_path, {"rigs": "../outside.json"}, None) + loaded = _load(pak, tmp_path) + assert loaded.rigs is None + assert loaded.song is not None From e57ad16caac1a70b1b8f72d60c0c5efa38fcd2b2 Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 23 Jul 2026 09:24:48 +0200 Subject: [PATCH 3/4] Resolve which tones block binds a part feedpak 1.18.0 lets a sound binding arrive from three places, and core honoured none of them: the manifest arrangement entry, the arrangement JSON, and the top-level drum_tones. Reading them needs a precedence rule, because two of the three can be present at once. Arrangement entries: the entry's `tones` replaces the arrangement JSON's WHOLESALE (spec 5.2), unlike name/tuning/capo/centOffset beside it, which override field by field. A merge would produce a sound nobody authored -- one source's base under the other's changes -- which is worse than either block alone. This is also what makes a notation-only keys entry bindable at all, since it has no arrangement JSON to carry tones in the first place. Drums: the top-level drum_tones binds the song-level primary part, and a `type: drums` entry's own tones takes precedence, with a Reader forbidden from applying both to the same part (5.1). That is the same shape as the drum_tab alias rule, so it lives inside _resolve_drum_parts next to it rather than beside it -- one precedence resolver, not two that drift. drum_tones is the PRIMARY's fallback only: a second drummer with no binding gets None, never the primary's kit. An empty `tones: {}` reads as absent rather than as an override to silence, matching how arrangement_from_wire already normalizes the in-JSON empty dict, so a stray empty object cannot quietly unbind a part. Spec-conformance gate passes with drum_tones added to the keys core reads (22 of the spec's 32, all declared). Signed-off-by: gionnibgud --- lib/sloppak.py | 62 ++++++- tests/test_sloppak_tones_precedence.py | 230 +++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 2 deletions(-) create mode 100644 tests/test_sloppak_tones_precedence.py diff --git a/lib/sloppak.py b/lib/sloppak.py index cacd8648..7c4028a5 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -855,18 +855,46 @@ def _load_rigs_file(source_dir: Path, rel: str) -> dict | None: } +def _entry_tones(entry: dict) -> dict | None: + """A manifest entry's `tones` binding, or None when it doesn't carry one. + + Spec §5.2: a manifest arrangement entry's `tones` overrides the arrangement + JSON's `tones` **wholesale** — no field-level merge. This normalizes the + "does it carry one" test for both the arrangement path and the drum path. + + An empty dict reads as *absent*, not as "override to silence": it is what a + Writer emits by accident, `arrangement_from_wire` already normalizes the + in-JSON `{}` to None the same way, and treating it as an override would let + a stray empty object silently unbind a part's sound. + """ + tones = entry.get("tones") + return tones if isinstance(tones, dict) and tones else None + + def _resolve_drum_parts( source_dir: Path, drum_tab_rel: object, drum_tab_data: dict | None, drum_pointer_entries: list[dict], + drum_tones: dict | None = None, ) -> tuple[dict | None, list[dict] | None]: - """Resolve drum pointers into a primary-first list with unique ids.""" + """Resolve drum pointers into a primary-first list with unique ids. + + Also binds each part's sound (feedpak 1.18.0). The precedence mirrors the + `drum_tab` alias rule this function already implements: a `type: drums` + entry's own `tones` wins for that part, and the song-level `drum_tones` is + the fallback for the **primary** part only. A Reader MUST NOT apply both to + the same part (spec §5.1/§5.2), which is why the primary picks one or the + other here rather than merging them. + """ if drum_tab_data is None and not drum_pointer_entries: return drum_tab_data, None primary_id = "drums" primary_name = None + # The primary's own binding, lifted from its alias pointer entry when it has + # one. Stays None if no entry claims the primary — `drum_tones` fills in. + primary_tones = None extra_parts: list[dict] = [] seen_rels: set[str] = set() # Use the same canonical, traversal-safe identity as zip member lookup so @@ -890,6 +918,11 @@ def _resolve_drum_parts( primary_id = entry_id if entry_name: primary_name = entry_name + # This entry IS the primary (an alias pointer at the same file), so + # its binding is the primary's — and it outranks `drum_tones`. + _alias_tones = _entry_tones(entry) + if _alias_tones is not None: + primary_tones = _alias_tones continue tab = _load_drum_tab_file(source_dir, rel, f"drum part {entry_id or rel}") if tab is None: @@ -900,6 +933,9 @@ def _resolve_drum_parts( "name": entry_name or (tab_name if isinstance(tab_name, str) and tab_name else "Drums"), "drum_tab": tab, + # Non-primary parts bind through their own entry only; `drum_tones` + # is explicitly the primary's fallback, never theirs. + "tones": _entry_tones(entry), }) parts: list[dict] = [] @@ -908,7 +944,14 @@ def _resolve_drum_parts( if primary_name is None: tab_name = drum_tab_data.get("name") primary_name = tab_name if isinstance(tab_name, str) and tab_name else "Drums" - parts.append({"id": primary_id, "name": primary_name, "drum_tab": drum_tab_data}) + parts.append({ + "id": primary_id, + "name": primary_name, + "drum_tab": drum_tab_data, + # Entry `tones` takes precedence; `drum_tones` is the fallback. One + # or the other, never both on the same part (spec §5.1). + "tones": primary_tones if primary_tones is not None else drum_tones, + }) used_ids.add(primary_id) next_generated_id = 2 @@ -1027,6 +1070,14 @@ def load_song( # _finite_float keeps a malformed manifest NaN/Infinity from # poisoning the song_info JSON (same guard as the wire path). arr.cent_offset = _finite_float(entry["centOffset"]) + # `tones` overrides WHOLESALE, unlike the field-level overrides above: + # the entry's object replaces the arrangement JSON's entirely, with no + # per-field merge (spec §5.2). A Writer SHOULD NOT emit both, but when + # one does, a half-merged sound — this pack's base with that pack's + # changes — would be worse than either source alone. + _entry_tone_block = _entry_tones(entry) + if _entry_tone_block is not None: + arr.tones = _entry_tone_block # Beats/sections can live on the arrangement itself in the wire format. # If the manifest-level arrangement JSON carries them, pull them onto @@ -1099,8 +1150,15 @@ def load_song( # Keep the dense compatibility logic independently testable and guarantee # ids are unique before the highway exposes them as selectors. + # Top-level `drum_tones` (spec §5.1) binds the song-level drum part — the + # fallback for packs without `type: drums` arrangements. Same shape as an + # arrangement entry's `tones`; `_resolve_drum_parts` owns the precedence. + _raw_drum_tones = manifest.get("drum_tones") + drum_tones_data = _raw_drum_tones if isinstance(_raw_drum_tones, dict) and _raw_drum_tones else None + drum_tab_data, drum_parts = _resolve_drum_parts( source_dir, drum_tab_rel, drum_tab_data, drum_pointer_entries, + drum_tones_data, ) # Drum-only sloppak: every GP track was percussion, so it ships a diff --git a/tests/test_sloppak_tones_precedence.py b/tests/test_sloppak_tones_precedence.py new file mode 100644 index 00000000..490b5ef3 --- /dev/null +++ b/tests/test_sloppak_tones_precedence.py @@ -0,0 +1,230 @@ +"""Loader coverage for the manifest-vs-in-JSON `tones` precedence cascade +(feedpak 1.18.0, spec §5.1 / §5.2). + +Two rules, both about *which* sound binding wins, neither about interpreting it: + + - A manifest arrangement entry's `tones` replaces the arrangement JSON's + `tones` **WHOLESALE** — no field-level merge. A half-merged block (this + source's `base` with that source's `changes`) would be a sound nobody + authored, so the two never blend. + - Top-level `drum_tones` binds the song-level (primary) drum part and is the + fallback; a `type: drums` entry's own `tones` takes precedence, and a + Reader MUST NOT apply both to the same part. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import yaml + +import sloppak as sloppak_mod + + +IN_JSON_TONES = { + "base": "In-JSON Clean", + "base_rig": "injson-clean", + "changes": [{"t": 5.0, "name": "In-JSON Lead", "rig": "injson-lead"}], +} +ENTRY_TONES = { + "base": "Entry Grand", + "base_rig": "entry-grand", + "changes": [{"t": 9.0, "name": "Entry Rhodes", "rig": "entry-rhodes"}], +} + + +def _tab(name: str) -> dict: + return { + "version": 1, + "name": name, + "kit": [{"id": "kick", "name": "Kick"}], + "hits": [{"t": 1.0, "p": "kick", "v": 100}], + } + + +def _write_pak(root: Path, manifest_extras: dict, arr_tones: dict | None = None, + files: dict[str, dict] | None = None) -> Path: + """Directory-form sloppak with one Lead arrangement, optionally carrying an + in-JSON `tones` block, plus any extra files.""" + 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": [], + } + if arr_tones is not None: + arr["tones"] = arr_tones + (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": [{"id": "full", "file": "stems/full.ogg", "default": True}], + } + manifest.update(manifest_extras) + (pak / "manifest.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False)) + for rel, payload in (files or {}).items(): + (pak / rel).write_text(json.dumps(payload)) + return pak + + +def _load(pak_path: Path, tmp_path: Path): + cache = tmp_path / "cache" + cache.mkdir() + return sloppak_mod.load_song(pak_path.name, pak_path.parent, cache) + + +# ── Arrangement entry vs in-JSON (§5.2) ────────────────────────────────────── + +def test_entry_tones_replaces_in_json_wholesale(tmp_path: Path): + """The entry object replaces the in-JSON one entirely — no key survives + from the loser, not even ones the winner doesn't define.""" + entry_tones = {"base": "Entry Only"} # no base_rig, no changes + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", + "tones": entry_tones}]}, + arr_tones=IN_JSON_TONES, + ) + arr = _load(pak, tmp_path).song.arrangements[0] + assert arr.tones == entry_tones + # The in-JSON `base_rig` and `changes` must NOT have been merged in. + assert "base_rig" not in arr.tones + assert "changes" not in arr.tones + + +def test_in_json_tones_survive_when_entry_has_none(tmp_path: Path): + pak = _write_pak(tmp_path, {}, arr_tones=IN_JSON_TONES) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_empty_entry_tones_is_absent_not_an_override(tmp_path: Path): + """`{}` reads as "didn't specify", not "override to silence" — otherwise a + stray empty object silently unbinds the part's sound.""" + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", "tones": {}}]}, + arr_tones=IN_JSON_TONES, + ) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_malformed_entry_tones_is_ignored(tmp_path: Path): + """A non-dict `tones` must not override, and must not crash the load.""" + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", + "tones": ["not", "a", "dict"]}]}, + arr_tones=IN_JSON_TONES, + ) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_entry_tones_binds_a_notation_only_arrangement(tmp_path: Path): + """§5.2: entry `tones` is available whether or not the arrangement has a + `file` — a keys part is a notation-only entry, and binding its sound is the + whole point of the 1.18.0 work.""" + notation = {"version": 1, "measures": []} + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "keys", "name": "Keys", + "notation": "notation_keys.json", + "tones": ENTRY_TONES}]}, + files={"notation_keys.json": notation}, + ) + arr = _load(pak, tmp_path).song.arrangements[0] + assert arr.name == "Keys" + assert arr.tones == ENTRY_TONES + + +# ── drum_tones vs entry tones (§5.1) ───────────────────────────────────────── + +def test_drum_tones_binds_the_primary_part(tmp_path: Path): + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json", "drum_tones": ENTRY_TONES}, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert len(parts) == 1 + assert parts[0]["tones"] == ENTRY_TONES + + +def test_entry_tones_outrank_drum_tones_on_the_primary(tmp_path: Path): + """An alias pointer entry naming the same file IS the primary, so its own + binding wins — and `drum_tones` must not also be applied.""" + alias_tones = {"base": "Alias Kit", "base_rig": "alias-kit"} + pak = _write_pak( + tmp_path, + { + "drum_tab": "drum_tab.json", + "drum_tones": ENTRY_TONES, + "arrangements": [ + {"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}, + {"id": "drums", "name": "Drums", "type": "drums", + "drum_tab": "drum_tab.json", "tones": alias_tones}, + ], + }, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert len(parts) == 1 + assert parts[0]["tones"] == alias_tones + + +def test_drum_tones_does_not_leak_to_secondary_parts(tmp_path: Path): + """`drum_tones` is the PRIMARY's fallback only. A second drummer with no + binding of its own gets None — not the primary's kit.""" + live_tones = {"base": "Live Kit", "base_rig": "live-kit"} + pak = _write_pak( + tmp_path, + { + "drum_tab": "drum_tab.json", + "drum_tones": ENTRY_TONES, + "arrangements": [ + {"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}, + {"id": "drums-live", "name": "Drums (Live)", "type": "drums", + "drum_tab": "drum_tab_live.json", "tones": live_tones}, + {"id": "drums-prog", "name": "Drums (Prog)", "type": "drums", + "drum_tab": "drum_tab_prog.json"}, + ], + }, + files={ + "drum_tab.json": _tab("Drums"), + "drum_tab_live.json": _tab("Drums Live"), + "drum_tab_prog.json": _tab("Drums Prog"), + }, + ) + parts = {p["id"]: p for p in _load(pak, tmp_path).drum_parts} + assert parts["drums"]["tones"] == ENTRY_TONES # primary, from drum_tones + assert parts["drums-live"]["tones"] == live_tones # own entry + assert parts["drums-prog"]["tones"] is None # no binding, no leak + + +def test_drum_parts_carry_none_when_pack_binds_nothing(tmp_path: Path): + """A pack with drums and no sound binding at all still loads, with the key + present and None — consumers can read `part["tones"]` unconditionally.""" + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json"}, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert parts[0]["tones"] is None + + +def test_malformed_drum_tones_is_ignored(tmp_path: Path): + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json", "drum_tones": "not-a-dict"}, + files={"drum_tab.json": _tab("Drums")}, + ) + assert _load(pak, tmp_path).drum_parts[0]["tones"] is None From 61d81220527d19dba51c35eb954eef18d389eac8 Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 23 Jul 2026 10:31:52 +0200 Subject: [PATCH 4/4] Document the rig bindings on the tone_changes wire message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG entry for the core rig reader, plus the WS protocol table in CLAUDE.md, which described `tone_changes` as carrying only base + name. While in that row: its time key was documented as `time`, but every producer emits `t` — both the sloppak builder and the legacy XML path. The 3D highway already carries a comment warning readers about exactly this discrepancy. Corrected here rather than left sitting next to the newly-added keys, where a reader would reasonably assume both were equally reliable. Signed-off-by: gionnibgud --- CHANGELOG.md | 14 ++++++++++++++ CLAUDE.md | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6294fd6c..a67ef5b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **Core reader for source rigs (feedpak 1.18.0).** A pack can declare what a + MIDI part should sound like by binding a rig; core now reads that binding and + hands it to the client instead of dropping it. Three parts: the + `tone_changes` WS message carries the pack's rig bindings (`base_rig`, and + `rig` per change) alongside the tone names it already sent; the manifest + `rigs:` key loads the pack's rig library (`rigs.json`, spec §7.9) verbatim; + and the binding precedence is resolved per spec §5.1/§5.2 — a manifest + arrangement entry's `tones` replaces the arrangement JSON's **wholesale** + (no field-level merge), while top-level `drum_tones` binds the primary drum + part as the fallback a `type: drums` entry's own `tones` outranks. Core + deliberately stops there: it does not select a realization or apply the + `intent.gm` floor, which belong to whatever actually voices the part. Packs + that bind no rig produce a byte-identical `tone_changes` payload, so existing + consumers are unaffected. - **Opt-in career venue packs (#122)** — higher-tier venue crowd media (`club`, `arena`) is no longer bundled; the app downloads each pack on demand from its release when you reach the venue (sha256-verified), keeping the diff --git a/CLAUDE.md b/CLAUDE.md index 1084f4ea..43186353 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -690,7 +690,7 @@ The highway WebSocket at `/ws/highway/{filename}?arrangement={index}` streams th | `anchors` | `{ type, data: [{ time, fret, width }] }` | Fret zoom anchors | | `chord_templates` | `{ type, data: [{ name, frets: [6] }] }` | Named chord shapes | | `lyrics` | `{ type, data: [{ w, t, d }], source }` | Syllables: `w`=word, `t`=time, `d`=duration. `-` joins to previous, `+` = line break. `source` is one of `"xml"`, `"whisperx"`, `"user"` — UI can use it to render an "auto-transcribed" badge for `whisperx`. Sloppaks always include `source` (legacy sloppaks without a `lyrics_source` manifest key default to `"xml"` at load time). Loose folders set it based on which extractor matched. Absent only when no lyrics fired the message at all | -| `tone_changes` | `{ type: 'tone_changes', base, data: [{ time, name }] }` | Optional — tone change events relative to the arrangement base tone; only sent if tones were found | +| `tone_changes` | `{ type: 'tone_changes', base, base_rig?, data: [{ t, name, rig? }] }` | Optional — tone change events relative to the arrangement base tone; only sent if tones were found. Note the time key is **`t`**, not `time` (both the sloppak path and the legacy XML path emit `t`). `base_rig` and each entry's `rig` are the pack's **rig bindings** — ids into [`rigs.json`](https://github.com/got-feedback/feedpak-spec/blob/main/spec/feedpak-v1.md#79-rigsjson) (feedpak §6.9/§7.9), carried through verbatim and **not** resolved by core: selecting a realization and applying the `intent.gm` floor belong to whatever voices the part. Both are **omitted entirely** when the chart binds no rig, so consumers predating the rig model see the payload they always did. | | `notes` | `{ type, data: [{ t, s, f, sus, ho, po, sl, bn, ... }] }` | Single notes | | `chords` | `{ type, data: [{ t, notes: [{ s, f, sus, ... }] }] }` | Chord events | | `phrases` | `{ type, data: [{ start_time, end_time, max_difficulty, levels: [{ difficulty, notes, chords, anchors, handshapes }] }], total }` | Optional — per-phrase difficulty ladder for master-difficulty slider (feedBack#48). Only sent when the source chart carries multi-level phrase data (phrase-aware sloppak). Sent in chunks (`data` is a batch, `total` is the full count across messages) to avoid multi-MB single frames. Absent for GP imports and legacy sloppak; consumers must treat missing message as "single fixed difficulty — slider disabled". |