fix(editor): stop dropping manifest type/centOffset on save + infer-once type

CHANGELOG.md

@@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+- **Saving no longer strips `type` / `centOffset` / unknown keys from manifest
+  arrangement entries.** The full-snapshot save path rebuilt every manifest
+  arrangement entry from scratch (`{id, name, file, tuning, capo}`), silently
+  dropping spec fields the editor doesn't author — `type` (§5.2), `centOffset`,
+  and any future additive key — on every save, violating the format's
+  unknown-key preservation rule (§1.2). Rebuilt entries now merge onto the
+  existing entry for the same id, with editor-owned keys taking the fresh
+  values. Tests: `tests/test_manifest_type_preserve.py`.
+
+### Added
+- **Infer-once arrangement `type` stamping.** On save, an arrangement entry with
+  no `type` gets one inferred from its display name (keys-family → `piano`, bass
+  names → `bass`, classic guitar roles → `guitar`) — conservative on purpose:
+  vocals/drums/ambiguous names stay untyped, and an authored `type` is never
+  clobbered. `type` is the queryable instrument facet the spec defines; display
+  names stay free labels — groundwork for safe track renaming in the Parts view.
+
 ### Changed
 - **Canvas repaints are coalesced to one per animation frame.** `draw()` was
   called imperatively from ~150 sites and each call repainted the whole

routes.py

@@ -175,6 +175,58 @@ def _load_arrangement_json(path) -> dict:
     return json.loads(raw)
 
 
+# Keys-family arrangement names (piano/keyboard/synth). The SAME matcher — a
+# word-boundary search over the SAME keyword set — decides both the manifest
+# ``type`` facet (below) and the keys notation sidecar (see the sidecar detector
+# further down, which reuses this regex). Keeping them identical guarantees a
+# name that earns a keys sidecar also infers a keys/piano type, so the two never
+# disagree (e.g. "Electric Piano", "Grand Piano", "Lead Synth").
+_KEYS_NAME_RE = re.compile(r"\b(keys|piano|keyboard|synth)\b", re.IGNORECASE)
+_TYPE_BASS_RE = re.compile(r"bass", re.I)
+_TYPE_SKIP_RE = re.compile(r"vocal|voice|sing|drum", re.I)
+_TYPE_GUITAR_RE = re.compile(r"guitar|lead|rhythm|combo|acoustic|electric", re.I)
+
+
+def _infer_arrangement_type(name) -> str:
+    """Manifest ``type`` (feedpak-spec §5.2) inferred from a display name.
+
+    The infer-once migration: callers write this only when an entry carries
+    no ``type`` yet, never clobbering an authored value. Conservative on
+    purpose — a name that doesn't clearly identify an instrument returns ""
+    and the entry stays untyped, because writing a WRONG type is worse than
+    writing none. Vocals/drums names also return "": per the spec those are
+    side-file mechanisms, not arrangement types.
+    """
+    n = (name or "").strip()
+    if not n:
+        return ""
+    # keys checked before bass so "Synth Bass" resolves to piano (intentional:
+    # a keys arrangement that happens to sit in the bass register still wants
+    # keys notation). Word-boundary search shared with the sidecar detector.
+    if _KEYS_NAME_RE.search(n):
+        return "piano"
+    if _TYPE_BASS_RE.search(n):
+        return "bass"
+    if _TYPE_SKIP_RE.search(n):
+        return ""
+    if _TYPE_GUITAR_RE.search(n):
+        return "guitar"
+    return ""
+
+
+def _merge_manifest_entry(old_entry, rebuilt: dict) -> dict:
+    """Merge a rebuilt manifest arrangement entry ONTO the existing entry
+    with the same id, so spec fields the editor doesn't author (``type``,
+    ``centOffset``, future additive keys) survive a full-snapshot save —
+    the format's unknown-key preservation rule (feedpak-spec §1.2). The
+    editor-owned keys (id/name/file/tuning/capo) always take the rebuilt
+    value. Previously the full-snapshot save path rebuilt every entry from
+    scratch and silently dropped everything else."""
+    out = dict(old_entry) if isinstance(old_entry, dict) else {}
+    out.update(rebuilt)
+    return out
+
+
 def _timeline_round_time(value) -> float:
     try:
         return round(float(value), 3)
@@ -1724,7 +1776,9 @@ def _note_in_chord(n):
 # shared core in slopsmith `lib/notation_lift.py` (factored out of the one-time
 # scripts/lift_keys_notation.py lifter), reached here the same way the rest of
 # this module reaches core helpers — `lib/` is on the host app's import path.
-_KEYS_NAME_RE = re.compile(r"\b(keys|piano|keyboard|synth)\b", re.IGNORECASE)
+# The keys-family name matcher (``_KEYS_NAME_RE``) is defined up by the manifest
+# type-inference helpers and shared here, so the sidecar and the ``type`` facet
+# always agree on what counts as a keys arrangement.
 _NOTATION_SAFE_ID_RE = re.compile(r"[A-Za-z0-9_-]+")
 
 
@@ -3471,6 +3525,13 @@ def _build_wire(arr_dict, is_first):
                 # removed or for safety on every save.
                 used_ids: set = set()
                 merged_arrangements = []
+                # Existing manifest entries by id: rebuilt entries merge onto
+                # these (see _merge_manifest_entry) so `type`/`centOffset`/
+                # unknown additive keys survive the full-snapshot save.
+                _old_by_id = {
+                    e.get("id"): e for e in old_entries
+                    if isinstance(e, dict) and e.get("id")
+                }
                 for i, ad in enumerate(all_arrangements):
                     raw_id = ad.get("id") or ""
                     if raw_id and raw_id not in used_ids:
@@ -3479,13 +3540,13 @@ def _build_wire(arr_dict, is_first):
                         aid = _arrangement_id(ad.get("name", "arr"), used_ids)
                     used_ids.add(aid)
                     wire = _build_wire(ad, i == 0)
-                    _entry = {
+                    _entry = _merge_manifest_entry(_old_by_id.get(aid), {
                         "id": aid,
                         "name": ad.get("name", "arr"),
                         "file": f"arrangements/{aid}.json",
                         "tuning": list(ad.get("tuning", [0]*6)),
                         "capo": int(ad.get("capo", 0)),
-                    }
+                    })
                     # Carry any GP-import notation alongside the entry (NOT on
                     # it — keeping it off the manifest entry means it can never
                     # leak into manifest.yaml); the sidecar writer consumes it.
@@ -3502,6 +3563,13 @@ def _build_wire(arr_dict, is_first):
             for item in merged_arrangements:
                 entry = item["entry"]
                 wire = item["wire"]
+                # Infer-once `type` stamping (both save paths flow through
+                # here): only when the entry has no type yet — an authored
+                # value is never clobbered, and ambiguous names stay untyped.
+                if isinstance(entry, dict) and not entry.get("type"):
+                    _t = _infer_arrangement_type(entry.get("name", ""))
+                    if _t:
+                        entry["type"] = _t
                 if wire is not None:
                     rel = entry.get("file") or f"arrangements/{entry.get('id', 'arr')}.json"
                     arr_path = (source_dir / rel).resolve()

tests/test_manifest_type_preserve.py

@@ -0,0 +1,121 @@
+"""Tests for manifest `type` preservation + infer-once stamping.
+
+The full-snapshot save path rebuilt every manifest arrangement entry from
+scratch (`{id, name, file, tuning, capo}`), silently dropping spec fields the
+editor doesn't author — `type` (§5.2), `centOffset`, and any future additive
+key — on EVERY save, violating the format's unknown-key preservation rule
+(feedpak-spec §1.2). Rebuilt entries now merge onto the existing entry for
+the same id (`_merge_manifest_entry`), and entries with no `type` get an
+inferred one from the display name exactly once (`_infer_arrangement_type`)
+— conservative: ambiguous / vocals / drums names stay untyped, because a
+wrong type is worse than none.
+
+Run: python -m pytest tests/test_manifest_type_preserve.py -q
+"""
+
+from routes import _infer_arrangement_type, _merge_manifest_entry  # noqa: E402
+
+
+# ── inference ────────────────────────────────────────────────────────────────
+
+def test_infer_keys_family_maps_to_piano():
+    # Spec §5.2 spells the keyboard type `piano` (`keys` is a read alias).
+    for name in ("Keys", "Piano", "keyboard", "Synth Lead", "PIANO RH"):
+        assert _infer_arrangement_type(name) == "piano", name
+
+
+def test_infer_keys_family_matches_mid_name_like_the_sidecar_detector():
+    # Regression: type inference must use the SAME word-boundary matcher as the
+    # keys notation-sidecar detector (`_KEYS_NAME_RE`), not a prefix-anchored
+    # one. Names where the keys keyword isn't the first word still earn a keys
+    # sidecar, so they must infer a keys/piano `type` too — otherwise the
+    # manifest facet and the notation renderer disagree for the same entry.
+    for name in ("Electric Piano", "Grand Piano", "Lead Synth", "Rhodes Keys"):
+        assert _infer_arrangement_type(name) == "piano", name
+
+
+def test_infer_bass_anywhere_in_the_name():
+    assert _infer_arrangement_type("Bass") == "bass"
+    assert _infer_arrangement_type("5-string Bass") == "bass"
+    assert _infer_arrangement_type("bass (DI)") == "bass"
+    # Keys is checked before bass, so synth-family names take the piano-roll
+    # pathway even when they also read as bass (intentional, unchanged by the
+    # prefix→word-boundary switch: "synth" still matches at a word boundary).
+    assert _infer_arrangement_type("Synth Bass") == "piano"
+
+
+def test_infer_guitar_roles():
+    for name in ("Lead", "Rhythm 2", "Combo", "Guitar 3", "Acoustic", "Electric"):
+        assert _infer_arrangement_type(name) == "guitar", name
+
+
+def test_infer_stays_silent_on_ambiguous_vocals_and_drums_names():
+    # Vocals/drums are side-file mechanisms per the spec, never arrangement
+    # types; unknown names stay untyped rather than guessing wrong.
+    for name in ("Vocals", "Voice", "Singing", "Drums", "Solo Thing", "Track 7", "", None):
+        assert _infer_arrangement_type(name) == "", repr(name)
+
+
+# ── merge ────────────────────────────────────────────────────────────────────
+
+def test_merge_preserves_spec_and_unknown_keys():
+    old = {
+        "id": "lead",
+        "name": "Old Name",
+        "file": "arrangements/lead.json",
+        "tuning": [0, 0, 0, 0, 0, 0],
+        "capo": 0,
+        "type": "guitar",
+        "centOffset": -6,
+        "notation": "notation_lead.json",
+        "x_future_key": {"nested": True},
+    }
+    rebuilt = {
+        "id": "lead",
+        "name": "Lead (renamed)",
+        "file": "arrangements/lead.json",
+        "tuning": [-2, -2, -2, -2, -2, -2],
+        "capo": 2,
+    }
+    out = _merge_manifest_entry(old, rebuilt)
+    # Editor-owned keys take the rebuilt values…
+    assert out["name"] == "Lead (renamed)"
+    assert out["tuning"] == [-2, -2, -2, -2, -2, -2]
+    assert out["capo"] == 2
+    # …and everything the editor doesn't author survives.
+    assert out["type"] == "guitar"
+    assert out["centOffset"] == -6
+    assert out["notation"] == "notation_lead.json"
+    assert out["x_future_key"] == {"nested": True}
+
+
+def test_merge_preserves_zero_cent_offset():
+    # Guard against a truthiness regression: a deliberate `centOffset: 0`
+    # (retuned back to concert pitch) must survive the merge exactly like a
+    # non-zero offset. The merge keeps it because the rebuilt entry never
+    # carries centOffset, so `out.update(rebuilt)` can't clobber it — but a
+    # future "only copy truthy fields" shortcut would silently drop the 0.
+    old = {"id": "lead", "name": "Old", "centOffset": 0, "type": "guitar"}
+    rebuilt = {"id": "lead", "name": "Lead", "file": "arrangements/lead.json",
+               "tuning": [0] * 6, "capo": 0}
+    out = _merge_manifest_entry(old, rebuilt)
+    assert "centOffset" in out
+    assert out["centOffset"] == 0
+    assert out["type"] == "guitar"
+
+
+def test_merge_with_no_old_entry_is_just_the_rebuilt_entry():
+    rebuilt = {"id": "new", "name": "New", "file": "arrangements/new.json",
+               "tuning": [0] * 6, "capo": 0}
+    assert _merge_manifest_entry(None, rebuilt) == rebuilt
+    assert _merge_manifest_entry("garbage", rebuilt) == rebuilt
+
+
+def test_merge_does_not_mutate_inputs():
+    old = {"id": "a", "type": "bass"}
+    rebuilt = {"id": "a", "name": "A"}
+    out = _merge_manifest_entry(old, rebuilt)
+    out["type"] = "changed"
+    out["name"] = "changed"
+    assert old["type"] == "bass"
+    assert rebuilt["name"] == "A"