fix(settings): enforce the language-tag and step constraints the manifest declares

A sweep of all 43 manifest definitions against the running server (160 checks:
declared default, both boundaries, and deliberate violations for each remote
key) found two places where the live registry accepts what the contract
forbids. Both are fixed by calling the contract's own validators rather than
adding a second implementation.

playback.audio_language was checked as "32 characters or fewer", so the server
stored "!!!" for a field the manifest declares as language_tag — a value track
matching would then silently never match. It now requires a well-formed tag via
settingscontract.NormalizeLanguageTag. The empty string is still accepted: the
string-only endpoint has no way to send null, and both Android and web send ""
to clear the choice, so rejecting it would break clearing the preference.

player.playback_speed declared step 0.05 and nothing enforced it, so 0.26 was
stored — a value no client's stepper can represent and that every client would
silently snap on the next write. settingscontract.StepAligned is now exported
and used by both the contract validator and the registry, so there is one
definition of "on step" rather than two that can drift.

This gives the contract its first production consumer beyond the startup load,
which is the direction Phase 2 continues in.

Also fixes a genuinely flaky test that the new CI gate would have hit
intermittently: TestRemoveJellyfinCompatWebDisablesWebSetting used t.TempDir as
the install root, but the endpoint returns 202 and its goroutine keeps writing
there after the test body returns, so cleanup tripped "directory not empty"
roughly one run in four. Confirmed pre-existing and unrelated to settings; the
suite now passes six consecutive full-package runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

contracts/settings/v1/manifest.json

@@ -20,7 +20,7 @@
       "label": "Preferred audio language",
       "description": "Choose which spoken language Silo should prefer first.",
       "recommended_control": "select",
-      "notes": "Migrates user_profiles.language as the roaming fallback. Existing user_device_settings values become real overrides, and the per-series value comes from AudioPreference.audio_language. AudioPreference.audio_track_index and track_signature stay specialized: they identify a concrete track, not a default."
+      "notes": "Migrates user_profiles.language as the roaming fallback. Existing user_device_settings values become real overrides, and the per-series value comes from AudioPreference.audio_language. AudioPreference.audio_track_index and track_signature stay specialized: they identify a concrete track, not a default. The legacy string-only endpoint has no way to send null, so it spells \"no preference\" as the empty string and both Android and web send that to clear the choice; its validator accepts \"\" and otherwise requires a well-formed tag via settingscontract.NormalizeLanguageTag. Migration maps \"\" to no stored row, the same way playback.subtitle_mode handles it."
     },
     {
       "key": "playback.subtitle_language",

internal/api/handlers/jellyfin_compat_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"strings"
 	"testing"
 
@@ -234,7 +235,7 @@ func TestRemoveJellyfinCompatWebDisablesWebSetting(t *testing.T) {
 	settings := &fakeServerSettingsStore{values: map[string]string{
 		"jellyfin_compat.enabled":         "true",
 		"jellyfin_compat.web_enabled":     "true",
-		"jellyfin_compat.web_install_dir": t.TempDir(),
+		"jellyfin_compat.web_install_dir": asyncWebInstallRoot(t),
 	}}
 	published := map[string]string{}
 	handler := &AdminHandler{
@@ -337,3 +338,21 @@ func TestPersistJellyfinCompatWebInstallSettingsEnablesWebUI(t *testing.T) {
 		t.Fatalf("jellyfin_compat.web_source_url = %q", got)
 	}
 }
+
+// asyncWebInstallRoot returns a temp dir for a handler that removes or installs
+// Jellyfin Web assets in a background goroutine.
+//
+// t.TempDir is wrong here: the endpoint returns 202 and its goroutine keeps
+// writing into the root after the test body returns, so t.TempDir's cleanup
+// intermittently trips "directory not empty" and fails an otherwise passing
+// test. os.RemoveAll recurses, so a late write is harmless, and cleanup errors
+// are ignored rather than failing the test.
+func asyncWebInstallRoot(t *testing.T) string {
+	t.Helper()
+	dir, err := os.MkdirTemp("", "jellyfin-web-root-*")
+	if err != nil {
+		t.Fatalf("MkdirTemp: %v", err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(dir) })
+	return dir
+}

internal/api/handlers/settings.go

@@ -15,6 +15,7 @@ import (
 
 	apimw "github.com/Silo-Server/silo-server/internal/api/middleware"
 	"github.com/Silo-Server/silo-server/internal/cache"
+	"github.com/Silo-Server/silo-server/internal/settingscontract"
 	"github.com/Silo-Server/silo-server/internal/userstore"
 )
 
@@ -157,12 +158,7 @@ var settingsRegistry = map[string]settingSpec{
 	"playback.audio_language": {
 		Scope:        scopeDevice,
 		DefaultValue: "",
-		Validate: func(value string) error {
-			if len(strings.TrimSpace(value)) > 32 {
-				return fmt.Errorf("playback.audio_language must be 32 characters or fewer")
-			}
-			return nil
-		},
+		Validate:     validateLanguageTagSetting("playback.audio_language"),
 	},
 	"playback.auto_skip_intro": {
 		Scope:        scopeDevice,
@@ -253,7 +249,7 @@ var settingsRegistry = map[string]settingSpec{
 	"player.playback_speed": {
 		Scope:        scopeDevice,
 		DefaultValue: "1",
-		Validate:     validateFloatRange("player.playback_speed", 0.25, 3.0),
+		Validate:     validateFloatRangeStep("player.playback_speed", 0.25, 3.0, 0.05),
 	},
 	"player.audio_sync_ms": {
 		Scope:        scopeDevice,
@@ -862,6 +858,18 @@ func validateIntRange(key string, min, max int) func(string) error {
 }
 
 func validateFloatRange(key string, min, max float64) func(string) error {
+	return validateFloatRangeStep(key, min, max, 0)
+}
+
+// validateFloatRangeStep enforces the range and, when step is positive, that
+// the value sits on the step grid anchored at min.
+//
+// The step check delegates to settingscontract.StepAligned so this endpoint
+// enforces exactly what contracts/settings/v1/manifest.json declares. Before
+// this, player.playback_speed advertised a 0.05 step that nothing enforced, so
+// the server happily stored 0.26 — a value no client's stepper can represent
+// and that every client would silently snap on the next write.
+func validateFloatRangeStep(key string, min, max, step float64) func(string) error {
 	return func(value string) error {
 		parsed, err := strconv.ParseFloat(value, 64)
 		if err != nil {
@@ -870,6 +878,33 @@ func validateFloatRange(key string, min, max float64) func(string) error {
 		if math.IsNaN(parsed) || parsed < min || parsed > max {
 			return fmt.Errorf("%s must be between %g and %g", key, min, max)
 		}
+		if !settingscontract.StepAligned(parsed, min, step) {
+			return fmt.Errorf("%s must be a multiple of %g starting from %g", key, step, min)
+		}
+		return nil
+	}
+}
+
+// validateLanguageTagSetting accepts a BCP 47 language tag, or the empty string.
+//
+// The empty string is the legacy wire form for "no preference": the string-only
+// settings API has no way to send null, and both the Android and web clients
+// send "" to clear the choice. The contract expresses the same state as null,
+// which is why every language definition there is nullable.
+//
+// Anything else must be a well-formed tag. The previous check was "32
+// characters or fewer", so the server accepted "!!!" for a field the manifest
+// declares as language_tag — and stored it where track matching would silently
+// never match.
+func validateLanguageTagSetting(key string) func(string) error {
+	return func(value string) error {
+		trimmed := strings.TrimSpace(value)
+		if trimmed == "" {
+			return nil
+		}
+		if _, ok := settingscontract.NormalizeLanguageTag(trimmed); !ok {
+			return fmt.Errorf("%s must be a BCP 47 language tag such as en or en-US", key)
+		}
 		return nil
 	}
 }

internal/api/handlers/settings_contract_test.go

@@ -139,3 +139,76 @@ func TestContractLoadsUnderTheServerBuild(t *testing.T) {
 		}
 	}
 }
+
+// TestAudioLanguageRejectsMalformedTags closes a drift measured against the
+// live server: the manifest declares playback.audio_language as language_tag,
+// but the registry check was "32 characters or fewer", so "!!!" was stored for
+// a field track matching would then silently never match.
+func TestAudioLanguageRejectsMalformedTags(t *testing.T) {
+	const key = "playback.audio_language"
+
+	// The empty string is how the string-only API says "no preference", and
+	// both Android and web send it to clear the choice. It must keep working.
+	accepted := []string{"", "  ", "en", "EN", "en-US", "en_US", "pt-BR", "zh-Hant-TW", "es-419"}
+	for _, v := range accepted {
+		if err := validateRegisteredSetting(key, v, scopeDevice); err != nil {
+			t.Errorf("value %q was rejected: %v", v, err)
+		}
+	}
+
+	rejected := []string{"!!!", "english please", "e", "en-", "-US", "en--US", "123", "<script>"}
+	for _, v := range rejected {
+		if err := validateRegisteredSetting(key, v, scopeDevice); err == nil {
+			t.Errorf("value %q was accepted; the manifest declares this key as language_tag", v)
+		}
+	}
+}
+
+// TestPlaybackSpeedEnforcesDeclaredStep closes the other measured drift: the
+// manifest declares step 0.05 over 0.25..3.0 and nothing enforced it, so the
+// server stored values no client's stepper can represent.
+func TestPlaybackSpeedEnforcesDeclaredStep(t *testing.T) {
+	const key = "player.playback_speed"
+
+	for _, v := range []string{"0.25", "0.75", "1", "1.0", "1.25", "1.4", "2.5", "3", "3.0"} {
+		if err := validateRegisteredSetting(key, v, scopeDevice); err != nil {
+			t.Errorf("on-step value %q was rejected: %v", v, err)
+		}
+	}
+	for _, v := range []string{"0.26", "1.4372", "1.01", "2.99"} {
+		if err := validateRegisteredSetting(key, v, scopeDevice); err == nil {
+			t.Errorf("off-step value %q was accepted despite the declared 0.05 step", v)
+		}
+	}
+	// Range still wins where both apply.
+	for _, v := range []string{"0.2", "3.05", "abc"} {
+		if err := validateRegisteredSetting(key, v, scopeDevice); err == nil {
+			t.Errorf("out-of-range value %q was accepted", v)
+		}
+	}
+}
+
+// TestRegistryStepMatchesTheManifest keeps the two in lockstep: if the manifest
+// widens or narrows the step, this fails until the registry follows.
+func TestRegistryStepMatchesTheManifest(t *testing.T) {
+	manifest, err := settingscontract.Load()
+	if err != nil {
+		t.Fatalf("loading contract: %v", err)
+	}
+	def, ok := manifest.Lookup("player.playback_speed")
+	if !ok {
+		t.Fatal("player.playback_speed is not in the manifest")
+	}
+	if def.ValueSchema.Step == nil {
+		t.Fatal("the manifest no longer declares a step; drop the registry check too")
+	}
+	step := *def.ValueSchema.Step
+	min := *def.ValueSchema.Minimum
+
+	// A value one half-step above the minimum must be rejected by the live
+	// validator for whatever step the manifest currently declares.
+	offStep := strconv.FormatFloat(min+step/2, 'f', -1, 64)
+	if err := validateRegisteredSetting("player.playback_speed", offStep, scopeDevice); err == nil {
+		t.Errorf("%s is off the manifest's declared %g step but was accepted", offStep, step)
+	}
+}

internal/settingscontract/validate.go

@@ -432,6 +432,21 @@ func (v *ValueSchema) NormalizeValue(
 // exact multiple would reject values every client can legitimately produce.
 const stepTolerance = 1e-9
 
+// StepAligned reports whether value sits on the grid of `step` anchored at
+// `base`. A non-positive step imposes no constraint.
+//
+// Exported so the legacy settings registry in internal/api/handlers enforces
+// exactly what the manifest declares instead of carrying a second,
+// nearly-identical implementation — the duplication this contract exists to
+// remove.
+func StepAligned(value, base, step float64) bool {
+	if step <= 0 {
+		return true
+	}
+	steps := (value - base) / step
+	return math.Abs(steps-math.Round(steps))*step <= stepTolerance
+}
+
 func (v *ValueSchema) checkRange(value float64) error {
 	if v.Minimum != nil && value < *v.Minimum {
 		return fmt.Errorf("%g is below the minimum %g", value, *v.Minimum)
@@ -439,15 +454,14 @@ func (v *ValueSchema) checkRange(value float64) error {
 	if v.Maximum != nil && value > *v.Maximum {
 		return fmt.Errorf("%g is above the maximum %g", value, *v.Maximum)
 	}
-	if v.Step != nil && *v.Step > 0 {
+	if v.Step != nil {
 		// Steps are counted from the minimum, which is the only origin every
 		// client's stepper agrees on.
 		base := 0.0
 		if v.Minimum != nil {
 			base = *v.Minimum
 		}
-		steps := (value - base) / *v.Step
-		if math.Abs(steps-math.Round(steps))**v.Step > stepTolerance {
+		if !StepAligned(value, base, *v.Step) {
 			return fmt.Errorf("%g is not a multiple of the step %g from %g",
 				value, *v.Step, base)
 		}