From c0e23e885b78303e874e85ce3d2c8be697ac86a8 Mon Sep 17 00:00:00 2001 From: "Claude Opus 4.8 (1M context)" Date: Sat, 4 Jul 2026 22:29:52 +0200 Subject: [PATCH] fix(settings): profile-aware saves/resets/switch, provider tunings, bass-5 Five regressions from the instrument-profiles rework: 1. save_settings canonicalized profiles on EVERY save -> empty/unrelated POST froze default profiles into config.json (broke test_empty_post_preserves_all_existing_keys). Gate on the save touching instrument settings; GET already virtualizes profiles. 2. pathway is profile-mirrored, so the Gameplay reset (flat-key delete) was a no-op. reset_settings now resets pathway inside the persisted profiles too. 3. Per-profile tuning validation rejected provider/custom tunings (tuner plugin, /api/tunings). _valid_tuning_for_key now accepts a name unknown to every built-in table while still rejecting a built-in misapplied to the wrong key. 4. First-migration overwrote an explicit active_instrument_profile with the legacy-inferred one, so a fresh-config switch to 'bass' was lost. Use setdefault so an explicit request wins. 5. Pre-existing test_instrument_fields_persist used bass-5 + 'Drop D' (a 4-string tuning). Updated to the valid 'Drop A'. Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/tunings.py | 21 +++++++++++++++++++-- server.py | 29 ++++++++++++++++++++++++----- tests/test_settings_api.py | 29 +++++++++++++++++++++++++++++ tests/test_settings_instrument.py | 6 ++++-- tests/test_tunings.py | 14 ++++++++++++++ 5 files changed, 90 insertions(+), 9 deletions(-) diff --git a/lib/tunings.py b/lib/tunings.py index 9de3ab8..2b9bede 100644 --- a/lib/tunings.py +++ b/lib/tunings.py @@ -207,7 +207,19 @@ def _valid_reference_pitch(value) -> float | None: def _valid_tuning_for_key(key: str, tuning): if isinstance(tuning, str): - return tuning if tuning in TUNING_PRESET_MIDIS.get(key, {}) and len(tuning) <= 64 else None + if len(tuning) > 64: + return None + if tuning in TUNING_PRESET_MIDIS.get(key, {}): + return tuning + # A name that IS a built-in preset for a different key is a misapplied + # built-in (e.g. "Drop D" on a 5-string bass, whose low string is B) — + # reject it. A name unknown to every built-in table is a provider/custom + # tuning (the tuner plugin's, exposed via /api/tunings) that this pure + # layer can't resolve — accept it so settings round-trip; the provider + # owns its validity. + if any(tuning in names for names in TUNING_PRESET_MIDIS.values()): + return None + return tuning if isinstance(tuning, list): expected = len(STANDARD_OPEN_MIDIS.get(key, [])) if len(tuning) != expected: @@ -327,7 +339,12 @@ def settings_with_instrument_profiles(cfg: dict) -> dict: if "instrument_profiles" not in out: legacy = profile_from_legacy_settings(out) profiles[legacy["id"]] = legacy - out["active_instrument_profile"] = legacy["id"] + # Default the active profile to the one migrated from the legacy flat + # fields, but DON'T clobber an explicit request — a fresh-config + # `POST {"active_instrument_profile": "bass"}` must switch, not be + # overwritten by the guitar-lead inferred from defaults. active_profile_id + # below normalizes an invalid value. + out.setdefault("active_instrument_profile", legacy["id"]) active = active_profile_id(out.get("active_instrument_profile")) selected = profiles[active] out["instrument_profiles"] = profiles diff --git a/server.py b/server.py index 1321840..7994650 100644 --- a/server.py +++ b/server.py @@ -9360,11 +9360,20 @@ def save_settings(data: dict): if cfg is None: cfg = _default_settings() cfg.update(updates) - try: - cfg = apply_flat_instrument_patch_to_profiles(cfg, updates) - except ValueError as exc: - return {"error": str(exc)} - cfg = settings_with_instrument_profiles(cfg) + # Only canonicalize/persist the instrument profiles when this save + # actually touches them (or the config already carries them). GET always + # virtualizes profiles via settings_with_instrument_profiles, so a save + # that doesn't touch instrument settings must stay a plain partial merge + # — otherwise an empty (or unrelated) POST would freeze the default + # profiles into the on-disk config. + _profile_keys = ("instrument", "string_count", "tuning", "reference_pitch", + "pathway", "instrument_profiles", "active_instrument_profile") + if "instrument_profiles" in cfg or any(k in updates for k in _profile_keys): + try: + cfg = apply_flat_instrument_patch_to_profiles(cfg, updates) + except ValueError as exc: + return {"error": str(exc)} + cfg = settings_with_instrument_profiles(cfg) _atomic_write_file(config_file, json.dumps(cfg, indent=2).encode("utf-8")) return {"message": ". ".join(messages) if messages else "Settings saved"} @@ -9402,6 +9411,16 @@ def reset_settings(data: dict): removed = [k for k in keys if k in cfg] for k in removed: del cfg[k] + # `pathway` is mirrored into every instrument profile, so deleting the + # flat key alone doesn't reset it — GET re-derives the value from the + # active profile. Reset it inside the persisted profiles too (back to the + # "songs" default), without disturbing the rest of the instrument config. + if "pathway" in keys and isinstance(cfg.get("instrument_profiles"), dict): + for prof in cfg["instrument_profiles"].values(): + if isinstance(prof, dict): + prof["pathway"] = "songs" + if "pathway" not in removed: + removed.append("pathway") _atomic_write_file(config_file, json.dumps(cfg, indent=2).encode("utf-8")) return {"message": "Settings reset", "reset": removed} diff --git a/tests/test_settings_api.py b/tests/test_settings_api.py index 487466a..2428929 100644 --- a/tests/test_settings_api.py +++ b/tests/test_settings_api.py @@ -834,6 +834,35 @@ def test_reset_clears_requested_keys(client, tmp_path): assert cfg["demucs_server_url"] == "http://demucs.example:9000" +def test_active_profile_switch_on_fresh_config(client, tmp_path): + # A fresh config has no instrument_profiles; an explicit active-profile + # switch must be honored, not overwritten by the profile inferred from the + # legacy flat defaults (guitar-lead). + r = client.post("/api/settings", json={"active_instrument_profile": "bass"}) + assert r.status_code == 200 and "error" not in r.json() + got = client.get("/api/settings").json() + assert got["active_instrument_profile"] == "bass" + assert got["instrument"] == "bass" + + +def test_reset_pathway_reaches_into_instrument_profiles(client, tmp_path): + # pathway is mirrored into every instrument profile, so a Gameplay reset + # that only deleted the flat key would leave GET re-deriving the old value + # from the profile. The reset must reach into the persisted profiles too. + client.post("/api/settings", json={"pathway": "studio"}) + assert client.get("/api/settings").json()["pathway"] == "studio" + profiles = _read_cfg(tmp_path)["instrument_profiles"] + assert any(p["pathway"] == "studio" for p in profiles.values()) + + r = client.post("/api/settings/reset", json={"keys": ["pathway"]}) + assert r.status_code == 200 + assert "pathway" in r.json()["reset"] + # GET re-derives from the profile — which must now be back to the default. + assert client.get("/api/settings").json()["pathway"] == "songs" + for prof in _read_cfg(tmp_path)["instrument_profiles"].values(): + assert prof["pathway"] == "songs" + + def test_reset_ignores_unknown_keys(client, tmp_path): (tmp_path / "config.json").write_text(json.dumps({"master_difficulty": 40})) # Unknown / non-resettable keys are silently ignored, not an error, and diff --git a/tests/test_settings_instrument.py b/tests/test_settings_instrument.py index 9abbf5c..0a967a6 100644 --- a/tests/test_settings_instrument.py +++ b/tests/test_settings_instrument.py @@ -30,12 +30,14 @@ def _cfg(tmp_path): def test_instrument_fields_persist(env): srv, tmp = env c = TestClient(srv.app) + # "Drop A" is the 5-string bass drop tuning (its low string is B, not E, so + # "Drop D" is a 4-string tuning — now correctly rejected per-profile). r = c.post("/api/settings", json={"instrument": "bass", "string_count": 5, - "tuning": "Drop D", "reference_pitch": 442}) + "tuning": "Drop A", "reference_pitch": 442}) assert r.status_code == 200 cfg = _cfg(tmp) assert cfg["instrument"] == "bass" and cfg["string_count"] == 5 - assert cfg["tuning"] == "Drop D" and cfg["reference_pitch"] == 442.0 + assert cfg["tuning"] == "Drop A" and cfg["reference_pitch"] == 442.0 # Reflected back through GET. got = c.get("/api/settings").json() assert got["instrument"] == "bass" and got["reference_pitch"] == 442.0 diff --git a/tests/test_tunings.py b/tests/test_tunings.py index d788ea2..b2f67e2 100644 --- a/tests/test_tunings.py +++ b/tests/test_tunings.py @@ -5,6 +5,7 @@ import pytest from tunings import ( DEFAULT_TUNINGS, TUNING_PRESET_MIDIS, + _valid_tuning_for_key, apply_flat_instrument_patch_to_profiles, open_midis_to_freqs, settings_with_instrument_profiles, @@ -15,6 +16,19 @@ from tunings import ( ) +def test_valid_tuning_for_key_builtin_and_provider_names(): + # A built-in valid for the key is accepted; a built-in valid only for a + # DIFFERENT key (misapplied, e.g. "Drop D" on a 5-string bass) is rejected. + assert _valid_tuning_for_key("bass-5", "Drop A") == "Drop A" + assert _valid_tuning_for_key("bass-5", "Drop D") is None + assert _valid_tuning_for_key("guitar-6", "Standard") == "Standard" + # A name unknown to every built-in table is a provider/custom tuning (tuner + # plugin, /api/tunings) the pure layer can't resolve — accept it so settings + # round-trip rather than normalizing it away to Standard. + assert _valid_tuning_for_key("bass-5", "My Custom DADGAD") == "My Custom DADGAD" + assert _valid_tuning_for_key("guitar-6", "x" * 65) is None # length cap kept + + # ── Standard tunings (all six strings share the same offset) ───────────────── STANDARD_CASES = [