From 61448d6161b41d81e95d11bc6db440b5aa1ed013 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:12:05 +0000 Subject: [PATCH] fix(settings): keep widened numeric bounds resolvable at older revisions A bound was one scalar plus the revision that introduced it, which discards the value it replaced. Widening a maximum from 240 to 480 at revision 3 left a revision-3 client with no correct answer against a revision-1 server: honoring 480 offers values that server rejects, and filtering the tagged bound out leaves the setting unbounded. Since clients are specified to filter their pinned contract against the server's advertised revision, the bound has to carry what it used to be. Bounds now hold their full history, oldest first, and AtRevision hands back the limit a given peer actually enforces. A bound nobody has widened still serializes as a bare number, so the manifest reads the same and untouched entries do not churn the ETag. Validation gains the rules the representation makes checkable: a maximum may only grow and a minimum may only shrink, history is strictly ordered, later entries must say when they arrived, and the first entry cannot predate the definition. That last rule is the lower bound allowed_scopes already enforced; the same gap is closed for enum members, which could previously claim to predate the definition containing them. Reported by Codex review on #479. --- contracts/settings/v1/manifest.json | 2 +- contracts/settings/v1/manifest.schema.json | 54 +++++-- .../api/handlers/settings_contract_test.go | 5 +- internal/settingscontract/contract.go | 84 +++++++++- internal/settingscontract/contract_test.go | 146 +++++++++++++++++- internal/settingscontract/validate.go | 139 +++++++++++++---- 6 files changed, 380 insertions(+), 50 deletions(-) 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",