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 <gionnibgud@gmail.com>

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

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