diff --git a/contracts/settings/v1/manifest.json b/contracts/settings/v1/manifest.json index 0d30a519..4eb26d56 100644 --- a/contracts/settings/v1/manifest.json +++ b/contracts/settings/v1/manifest.json @@ -400,7 +400,7 @@ "label": "Default sleep timer", "description": "Duration the sleep timer starts on when you turn it on. 0 leaves it off.", "recommended_control": "stepper", - "notes": "Android keeps this device-local today and clamps to 0..240; it was never written to the server rather than written and rejected. The maximum matches that clamp rather than exceeding it, and the default matches Android's shipped 30, because a manifest that disagrees with the only client implementing a setting is the drift this contract exists to remove — and a default of 0 would silently turn the preset off for everyone at cutover. Raising the maximum later is additive under the widening rule: bump it and tag it with maximum_introduced_in. This is the duration the timer starts on, not whether one is running: the design classes a running sleep timer as private local, so only the persisted default is registered." + "notes": "Android keeps this device-local today and clamps to 0..240; it was never written to the server rather than written and rejected. The maximum matches that clamp rather than exceeding it, and the default matches Android's shipped 30, because a manifest that disagrees with the only client implementing a setting is the drift this contract exists to remove — and a default of 0 would silently turn the preset off for everyone at cutover. Raising the maximum later is additive under the widening rule: replace the bare maximum with its history so a client can still see the 240 an older server enforces. This is the duration the timer starts on, not whether one is running: the design classes a running sleep timer as private local, so only the persisted default is registered." }, { "key": "ui.theme", diff --git a/contracts/settings/v1/manifest.schema.json b/contracts/settings/v1/manifest.schema.json index 76ea93d5..e49ec010 100644 --- a/contracts/settings/v1/manifest.schema.json +++ b/contracts/settings/v1/manifest.schema.json @@ -61,6 +61,44 @@ } ] }, + "integerBound": { + "description": "A numeric bound. Write a bare number for a bound that has never been widened. Widening replaces it with the full history, oldest first, so a client can recover the bound an older server still enforces; the bare form alone would discard it.", + "oneOf": [ + { "type": "integer" }, + { + "type": "array", + "minItems": 2, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["value"], + "properties": { + "value": { "type": "integer" }, + "introduced_in": { "$ref": "#/$defs/revisionRef" } + } + } + } + ] + }, + "numberBound": { + "description": "A numeric bound. Write a bare number for a bound that has never been widened. Widening replaces it with the full history, oldest first, so a client can recover the bound an older server still enforces; the bare form alone would discard it.", + "oneOf": [ + { "type": "number" }, + { + "type": "array", + "minItems": 2, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["value"], + "properties": { + "value": { "type": "number" }, + "introduced_in": { "$ref": "#/$defs/revisionRef" } + } + } + } + ] + }, "enumMember": { "description": "Enum members are objects so members added later can carry their own revision.", "type": "object", @@ -90,12 +128,10 @@ "required": ["type", "minimum", "maximum"], "properties": { "type": { "const": "integer" }, - "minimum": { "type": "integer" }, - "maximum": { "type": "integer" }, + "minimum": { "$ref": "#/$defs/integerBound" }, + "maximum": { "$ref": "#/$defs/integerBound" }, "step": { "type": "integer", "exclusiveMinimum": 0 }, - "nullable": { "type": "boolean", "default": false }, - "minimum_introduced_in": { "$ref": "#/$defs/revisionRef" }, - "maximum_introduced_in": { "$ref": "#/$defs/revisionRef" } + "nullable": { "type": "boolean", "default": false } } }, { @@ -104,12 +140,10 @@ "required": ["type", "minimum", "maximum"], "properties": { "type": { "const": "number" }, - "minimum": { "type": "number" }, - "maximum": { "type": "number" }, + "minimum": { "$ref": "#/$defs/numberBound" }, + "maximum": { "$ref": "#/$defs/numberBound" }, "step": { "type": "number", "exclusiveMinimum": 0 }, - "nullable": { "type": "boolean", "default": false }, - "minimum_introduced_in": { "$ref": "#/$defs/revisionRef" }, - "maximum_introduced_in": { "$ref": "#/$defs/revisionRef" } + "nullable": { "type": "boolean", "default": false } } }, { diff --git a/internal/api/handlers/settings_contract_test.go b/internal/api/handlers/settings_contract_test.go index e03bf049..341e08be 100644 --- a/internal/api/handlers/settings_contract_test.go +++ b/internal/api/handlers/settings_contract_test.go @@ -203,7 +203,10 @@ func TestRegistryStepMatchesTheManifest(t *testing.T) { t.Fatal("the manifest no longer declares a step; drop the registry check too") } step := *def.ValueSchema.Step - min := *def.ValueSchema.Minimum + min, ok := def.ValueSchema.Minimum.Current() + if !ok { + t.Fatal("the manifest no longer declares a minimum for player.playback_speed") + } // A value one half-step above the minimum must be rejected by the live // validator for whatever step the manifest currently declares. diff --git a/internal/settingscontract/contract.go b/internal/settingscontract/contract.go index 96d670b9..f53aa498 100644 --- a/internal/settingscontract/contract.go +++ b/internal/settingscontract/contract.go @@ -169,12 +169,9 @@ type ValueSchema struct { Nullable bool `json:"nullable,omitempty"` // Numeric (integer, number). - Minimum *float64 `json:"minimum,omitempty"` - Maximum *float64 `json:"maximum,omitempty"` + Minimum *Bound `json:"minimum,omitempty"` + Maximum *Bound `json:"maximum,omitempty"` Step *float64 `json:"step,omitempty"` - // Revision tags for additively widened bounds. - MinimumIntroducedIn int `json:"minimum_introduced_in,omitempty"` - MaximumIntroducedIn int `json:"maximum_introduced_in,omitempty"` // String. MinLength *int `json:"min_length,omitempty"` @@ -193,6 +190,83 @@ type ValueSchema struct { compiledPattern *regexp.Regexp } +// Bound is a numeric limit together with every earlier value it has had. +// +// A widened bound cannot be represented as one scalar plus the revision that +// introduced it, because that discards the value it replaced. A client pinned +// to the newer revision talking to a server on the older one would then have no +// correct answer: honoring the new bound offers values the server rejects, and +// filtering the tagged bound out leaves the setting with no bound at all. +// Keeping the history lets AtRevision hand back the limit the peer actually +// enforces. +type Bound struct { + // History is ordered oldest first. The last entry is the bound in force on + // the manifest that declares it. + History []BoundEntry +} + +// BoundEntry is one value of a bound and the revision that introduced it. A +// zero IntroducedIn means the bound has held since its definition appeared. +type BoundEntry struct { + Value float64 `json:"value"` + IntroducedIn int `json:"introduced_in,omitempty"` +} + +// UnmarshalJSON accepts 240 or +// [{"value":240},{"value":480,"introduced_in":3}]. The bare form is the common +// case — most bounds are never widened — and keeping it readable is worth the +// dual shape, the same trade ScopeEntry makes. +func (b *Bound) UnmarshalJSON(data []byte) error { + var bare float64 + if err := json.Unmarshal(data, &bare); err == nil { + b.History = []BoundEntry{{Value: bare}} + return nil + } + var history []BoundEntry + if err := json.Unmarshal(data, &history); err != nil { + return fmt.Errorf("bound must be a number or an array of {value, introduced_in}: %w", err) + } + b.History = history + return nil +} + +// MarshalJSON emits the bare number when a bound has never been widened, so +// round-tripping the manifest does not rewrite untouched entries. +func (b Bound) MarshalJSON() ([]byte, error) { + if len(b.History) == 1 && b.History[0].IntroducedIn == 0 { + return json.Marshal(b.History[0].Value) + } + return json.Marshal(b.History) +} + +// Current returns the bound in force on this manifest. +func (b *Bound) Current() (float64, bool) { + if b == nil || len(b.History) == 0 { + return 0, false + } + return b.History[len(b.History)-1].Value, true +} + +// AtRevision returns the bound a peer at revision enforces: the newest entry +// introduced no later than that revision. Clients call this so they never offer +// a value the connected server will refuse. +func (b *Bound) AtRevision(revision int) (float64, bool) { + if b == nil { + return 0, false + } + var ( + value float64 + found bool + ) + for _, entry := range b.History { + if entry.IntroducedIn > revision { + continue + } + value, found = entry.Value, true + } + return value, found +} + // EnumMember is one allowed enum value. Members are objects rather than bare // strings so a member added after the definition can carry its own revision. type EnumMember struct { diff --git a/internal/settingscontract/contract_test.go b/internal/settingscontract/contract_test.go index 9647ecd0..dd23737c 100644 --- a/internal/settingscontract/contract_test.go +++ b/internal/settingscontract/contract_test.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "sort" "strings" @@ -604,6 +605,139 @@ func TestRevisionAwareFilteringHidesNewerElements(t *testing.T) { } } +// TestWidenedBoundsStayResolvableAtOlderRevisions is the case a bare scalar +// plus a revision tag could not express. player.sleep_timer_default_minutes +// ships with a 240 maximum; widening it to 480 later must not erase the 240, +// because a client pinned to the newer revision still has to talk to servers +// that enforce the older one. +func TestWidenedBoundsStayResolvableAtOlderRevisions(t *testing.T) { + widened := &Bound{History: []BoundEntry{ + {Value: 240}, + {Value: 480, IntroducedIn: 3}, + }} + + for _, tc := range []struct { + revision int + want float64 + }{ + {1, 240}, // the revision that introduced the definition + {2, 240}, // still before the widening + {3, 480}, // the widening itself + {9, 480}, // and everything after it + } { + got, ok := widened.AtRevision(tc.revision) + if !ok { + t.Errorf("AtRevision(%d) found no bound", tc.revision) + continue + } + if got != tc.want { + t.Errorf("AtRevision(%d) = %g, want %g", tc.revision, got, tc.want) + } + } + + if got, _ := widened.Current(); got != 480 { + t.Errorf("Current() = %g, want the newest bound 480", got) + } + + // The server validates against its own newest bound regardless of any + // peer's revision. + schema := &ValueSchema{Type: TypeInteger, Minimum: fixedBound(0), Maximum: widened} + if err := schema.ValidateValue(json.RawMessage(`480`), nil); err != nil { + t.Errorf("ValidateValue(480) = %v, want nil", err) + } + if err := schema.ValidateValue(json.RawMessage(`481`), nil); err == nil { + t.Error("481 was accepted above the widened maximum") + } +} + +// TestBoundsRoundTripInTheirAuthoredShape keeps the manifest diff honest: a +// bound nobody has widened must not sprout a history array when the contract is +// re-serialized, or the ETag changes for a file nobody edited. +func TestBoundsRoundTripInTheirAuthoredShape(t *testing.T) { + for _, tc := range []struct { + name string + raw string + }{ + {"bare", `240`}, + {"history", `[{"value":240},{"value":480,"introduced_in":3}]`}, + } { + t.Run(tc.name, func(t *testing.T) { + var bound Bound + if err := json.Unmarshal([]byte(tc.raw), &bound); err != nil { + t.Fatalf("unmarshalling %s: %v", tc.raw, err) + } + encoded, err := json.Marshal(bound) + if err != nil { + t.Fatalf("marshalling: %v", err) + } + if string(encoded) != tc.raw { + t.Errorf("round trip = %s, want %s", encoded, tc.raw) + } + }) + } +} + +func TestBoundHistoriesAreValidated(t *testing.T) { + for _, tc := range []struct { + name string + maximum *Bound + want string + }{ + { + name: "narrowing is not a widening", + maximum: &Bound{History: []BoundEntry{{Value: 480}, {Value: 240, IntroducedIn: 6}}}, + want: "narrows", + }, + { + name: "history must move forward", + maximum: &Bound{History: []BoundEntry{{Value: 240}, {Value: 480, IntroducedIn: 5}}}, + want: "not ordered", + }, + { + name: "later entries must say when they arrived", + maximum: &Bound{History: []BoundEntry{{Value: 240}, {Value: 480}}}, + want: "must declare introduced_in", + }, + { + name: "a bound cannot predate its definition", + maximum: &Bound{History: []BoundEntry{{Value: 240, IntroducedIn: 2}}}, + want: "the definition was introduced in 5", + }, + { + name: "a bound cannot postdate the manifest", + maximum: &Bound{History: []BoundEntry{{Value: 240}, {Value: 480, IntroducedIn: 99}}}, + want: "after the manifest revision", + }, + } { + t.Run(tc.name, func(t *testing.T) { + schema := &ValueSchema{Type: TypeInteger, Minimum: fixedBound(0), Maximum: tc.maximum} + errs := schema.validate(5, 10, nil) + if len(errs) == 0 { + t.Fatalf("validate accepted %s", tc.name) + } + if joined := errors.Join(errs...).Error(); !strings.Contains(joined, tc.want) { + t.Errorf("error %q does not mention %q", joined, tc.want) + } + }) + } +} + +// TestEnumMembersCannotPredateTheirDefinition is the enum half of the same +// lower-bound rule allowed_scopes has always enforced. +func TestEnumMembersCannotPredateTheirDefinition(t *testing.T) { + schema := &ValueSchema{ + Type: TypeEnum, + Values: []EnumMember{{Value: "old"}, {Value: "new", IntroducedIn: 2}}, + } + errs := schema.validate(5, 10, nil) + if len(errs) == 0 { + t.Fatal("an enum member claiming to predate its definition was accepted") + } + if joined := errors.Join(errs...).Error(); !strings.Contains(joined, "before the definition's own 5") { + t.Errorf("error %q does not name the definition revision", joined) + } +} + func TestValidateValueRejectsOutOfContractValues(t *testing.T) { manifest, err := Load() if err != nil { @@ -850,19 +984,25 @@ func TestEnumMatchingIsTypeSafe(t *testing.T) { Type: TypeEnum, Values: []EnumMember{{Value: "3"}, {Value: float64(3)}}, } - if errs := mixed.validate(1, nil); len(errs) != 0 { + if errs := mixed.validate(1, 1, nil); len(errs) != 0 { t.Errorf(`members "3" and 3 reported as duplicates: %v`, errs) } } +// fixedBound is a bound that has never been widened, which is every bound built +// by hand in a test. +func fixedBound(value float64) *Bound { + return &Bound{History: []BoundEntry{{Value: value}}} +} + // TestStepIsEnforced closes the gap between a declared constraint and the // single validation path. player.playback_speed advertises a 0.05 step, so a // server that stores 1.4372 hands every client a value its stepper cannot // represent. func TestStepIsEnforced(t *testing.T) { - min, max, step := 0.25, 3.0, 0.05 + step := 0.05 schema := &ValueSchema{ - Type: TypeNumber, Minimum: &min, Maximum: &max, Step: &step, + Type: TypeNumber, Minimum: fixedBound(0.25), Maximum: fixedBound(3.0), Step: &step, } for _, raw := range []string{`0.25`, `0.75`, `1`, `1.25`, `1.4`, `2.5`, `3`} { diff --git a/internal/settingscontract/validate.go b/internal/settingscontract/validate.go index 0e2992dd..4a28e939 100644 --- a/internal/settingscontract/validate.go +++ b/internal/settingscontract/validate.go @@ -54,7 +54,7 @@ func (d *Definition) validate(manifestRevision int, objectSchemas map[string]*js errs = append(errs, d.validateScopes(manifestRevision)...) errs = append(errs, d.validateResolutionOrder()...) - errs = append(errs, d.ValueSchema.validate(manifestRevision, objectSchemas)...) + errs = append(errs, d.ValueSchema.validate(d.IntroducedIn, manifestRevision, objectSchemas)...) errs = append(errs, d.validateDefault(objectSchemas)...) errs = append(errs, d.validateConstraint()...) @@ -156,7 +156,10 @@ func (d *Definition) validateResolutionOrder() []error { return errs } -func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*jsonschema.Schema) []error { +func (v *ValueSchema) validate( + definitionRevision, manifestRevision int, + objectSchemas map[string]*jsonschema.Schema, +) []error { var errs []error switch v.Type { @@ -168,29 +171,28 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j errs = append(errs, fmt.Errorf("%s requires minimum and maximum", v.Type)) break } - if *v.Minimum > *v.Maximum { + // A slice, not a map: ranging a map would order these errors randomly, + // so the same broken manifest would report differently from run to run. + for _, bound := range []struct { + label string + bound *Bound + widensUp bool + }{ + {"minimum", v.Minimum, false}, + {"maximum", v.Maximum, true}, + } { + errs = append(errs, bound.bound.validate( + bound.label, bound.widensUp, definitionRevision, manifestRevision)...) + } + minimum, hasMinimum := v.Minimum.Current() + maximum, hasMaximum := v.Maximum.Current() + if hasMinimum && hasMaximum && minimum > maximum { errs = append(errs, fmt.Errorf( - "minimum %g exceeds maximum %g", *v.Minimum, *v.Maximum)) + "minimum %g exceeds maximum %g", minimum, maximum)) } if v.Step != nil && *v.Step <= 0 { errs = append(errs, fmt.Errorf("step must be positive, got %g", *v.Step)) } - // A slice, not a map: ranging a map would order these two errors - // randomly, so the same broken manifest would report differently from - // run to run. - for _, tagged := range []struct { - label string - rev int - }{ - {"minimum_introduced_in", v.MinimumIntroducedIn}, - {"maximum_introduced_in", v.MaximumIntroducedIn}, - } { - if tagged.rev != 0 && tagged.rev > manifestRevision { - errs = append(errs, fmt.Errorf( - "%s is %d, after the manifest revision %d", - tagged.label, tagged.rev, manifestRevision)) - } - } case TypeString: if v.MaxLength == nil { @@ -228,10 +230,21 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j errs = append(errs, fmt.Errorf("enum repeats value %s", displayEnumValue(member.Value))) } seen[token] = struct{}{} - if member.IntroducedIn != 0 && member.IntroducedIn > manifestRevision { - errs = append(errs, fmt.Errorf( - "enum member %s claims introduced_in %d, after the manifest revision %d", - displayEnumValue(member.Value), member.IntroducedIn, manifestRevision)) + if member.IntroducedIn != 0 { + // Same lower bound validateScopes enforces: a member cannot + // claim to predate the definition that contains it, or a client + // filtering by revision would offer it to a server too old to + // have the setting at all. + if member.IntroducedIn < definitionRevision { + errs = append(errs, fmt.Errorf( + "enum member %s claims introduced_in %d, before the definition's own %d", + displayEnumValue(member.Value), member.IntroducedIn, definitionRevision)) + } + if member.IntroducedIn > manifestRevision { + errs = append(errs, fmt.Errorf( + "enum member %s claims introduced_in %d, after the manifest revision %d", + displayEnumValue(member.Value), member.IntroducedIn, manifestRevision)) + } } } @@ -252,6 +265,67 @@ func (v *ValueSchema) validate(manifestRevision int, objectSchemas map[string]*j return errs } +// validate checks a bound's history. widensUp says which direction is a +// widening for this bound: a maximum may only grow and a minimum may only +// shrink, because the manifest's widening rule says a later revision must +// accept every value an earlier one did. A bound that moved the other way is a +// narrowing, which needs a new key rather than a revision tag. +func (b *Bound) validate(label string, widensUp bool, definitionRevision, manifestRevision int) []error { + if b == nil || len(b.History) == 0 { + return []error{fmt.Errorf("%s has no value", label)} + } + + var errs []error + previousRevision := 0 + for i, entry := range b.History { + switch { + case i == 0: + // The original bound may be written bare, which reads as "has held + // since the definition appeared". + if entry.IntroducedIn != 0 && entry.IntroducedIn != definitionRevision { + errs = append(errs, fmt.Errorf( + "%s history starts at revision %d, but the definition was introduced in %d", + label, entry.IntroducedIn, definitionRevision)) + } + previousRevision = definitionRevision + default: + if entry.IntroducedIn == 0 { + errs = append(errs, fmt.Errorf( + "%s history entry %d must declare introduced_in", label, i)) + continue + } + if entry.IntroducedIn <= previousRevision { + errs = append(errs, fmt.Errorf( + "%s history is not ordered: entry %d claims introduced_in %d, at or before %d", + label, i, entry.IntroducedIn, previousRevision)) + } + previousRevision = entry.IntroducedIn + } + + if entry.IntroducedIn > manifestRevision { + errs = append(errs, fmt.Errorf( + "%s history entry %d claims introduced_in %d, after the manifest revision %d", + label, i, entry.IntroducedIn, manifestRevision)) + } + + if i > 0 { + previous := b.History[i-1].Value + if widensUp && entry.Value < previous { + errs = append(errs, fmt.Errorf( + "%s narrows from %g to %g at revision %d; a narrowing needs a new key", + label, previous, entry.Value, entry.IntroducedIn)) + } + if !widensUp && entry.Value > previous { + errs = append(errs, fmt.Errorf( + "%s narrows from %g to %g at revision %d; a narrowing needs a new key", + label, previous, entry.Value, entry.IntroducedIn)) + } + } + } + + return errs +} + func (d *Definition) validateDefault(objectSchemas map[string]*jsonschema.Schema) []error { raw := bytes.TrimSpace(d.DefaultValue) if len(raw) == 0 { @@ -447,19 +521,24 @@ func StepAligned(value, base, step float64) bool { return math.Abs(steps-math.Round(steps))*step <= stepTolerance } +// checkRange validates against the bounds this server enforces, which are +// always the newest in the history. Revision filtering is a client-side concern +// — the server accepts everything its own manifest allows. 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) + minimum, hasMinimum := v.Minimum.Current() + if hasMinimum && value < minimum { + return fmt.Errorf("%g is below the minimum %g", value, minimum) } - if v.Maximum != nil && value > *v.Maximum { - return fmt.Errorf("%g is above the maximum %g", value, *v.Maximum) + maximum, hasMaximum := v.Maximum.Current() + if hasMaximum && value > maximum { + return fmt.Errorf("%g is above the maximum %g", value, maximum) } 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 + if hasMinimum { + base = minimum } if !StepAligned(value, base, *v.Step) { return fmt.Errorf("%g is not a multiple of the step %g from %g",