Skip to content

Commit 3b9f6a9

Browse files
fix(inventory): detach numeric schema bounds and dedupe default traversal
Two Copilot findings on 431b7a2: - CloneSchemas only deep-copies subschema nodes, so pointer-valued numeric validation keywords (Minimum, Maximum, MinLength, MinItems, etc.) remained aliased with the caller-owned schema after CloneSchema/CachedSchema. A caller mutating one of those pointers after caching could race with concurrent cached-schema serialization or silently change the cached validation bounds. Clone every *int/*float64 keyword explicitly alongside the existing collection/metadata cloning. - CloneSchemaWithoutDefaults's removeSchemaDefaults visited every map/pointer child twice: once via two literal child-collection loops, and again via schemaChildren, which already covers the same fields. This doubled traversal at every nested level, making default removal exponential for deeply nested object/array schemas. Traverse only the single comprehensive schemaChildren list. Add regression coverage across CloneSchema, CachedSchema, schema inference option cloning, and CloneSchemaWithoutDefaults: every *int/*float64 keyword is isolated from caller mutation (including under concurrent marshaling), and a 48-level deep chain is used for every schema-child field kind to confirm linear, non-exponential default removal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 431b7a2 commit 3b9f6a9

3 files changed

Lines changed: 161 additions & 30 deletions

File tree

‎docs/typed-tool-schemas.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ The SDK applies defaults from the runtime input schema before decoding. If an
3838
omitted field must remain omitted, build the runtime schema with
3939
`inventory.CloneSchemaWithoutDefaults` before applying validation-only
4040
changes. Use `inventory.CloneSchema` when deriving other runtime-only schema
41-
variants so nested metadata is detached as well as subschemas. The advertised
41+
variants so numeric bound pointers and nested metadata are detached as well as
42+
subschemas. Default removal visits each child once per parent. The advertised
4243
schema can retain its defaults; the constructor caches the runtime schema
4344
without mutating either caller-owned schema.
4445

‎pkg/inventory/typed_output_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,143 @@ func TestCachedSchemaDeepCopiesMutableMetadata(t *testing.T) {
241241
workers.Wait()
242242
}
243243

244+
func TestSchemaClonesDetachEveryNumericKeyword(t *testing.T) {
245+
for name, clone := range map[string]func(*jsonschema.Schema) *jsonschema.Schema{
246+
"direct": CloneSchema,
247+
"cache": func(schema *jsonschema.Schema) *jsonschema.Schema {
248+
cached, err := CachedSchema(schema)
249+
require.NoError(t, err)
250+
return cached
251+
},
252+
"inference options": func(schema *jsonschema.Schema) *jsonschema.Schema {
253+
options := cloneJSONSchemaForOptions(&jsonschema.ForOptions{
254+
TypeSchemas: map[reflect.Type]*jsonschema.Schema{reflect.TypeFor[int](): schema},
255+
})
256+
return options.TypeSchemas[reflect.TypeFor[int]()]
257+
},
258+
"without defaults": CloneSchemaWithoutDefaults,
259+
} {
260+
t.Run(name, func(t *testing.T) {
261+
bounds := &jsonschema.Schema{}
262+
fields := reflect.ValueOf(bounds).Elem()
263+
var numericFields []int
264+
for i := range fields.NumField() {
265+
field := fields.Field(i)
266+
if field.Type() != reflect.TypeFor[*int]() && field.Type() != reflect.TypeFor[*float64]() {
267+
continue
268+
}
269+
value := reflect.New(field.Type().Elem())
270+
if value.Elem().Kind() == reflect.Int {
271+
value.Elem().SetInt(11)
272+
} else {
273+
value.Elem().SetFloat(11)
274+
}
275+
field.Set(value)
276+
numericFields = append(numericFields, i)
277+
}
278+
require.NotEmpty(t, numericFields)
279+
original := &jsonschema.Schema{
280+
Type: "object",
281+
Comment: "numeric keyword isolation " + name,
282+
Properties: map[string]*jsonschema.Schema{"bounds": bounds},
283+
}
284+
cloned := clone(original)
285+
clonedFields := reflect.ValueOf(cloned.Properties["bounds"]).Elem()
286+
for _, i := range numericFields {
287+
keyword := fields.Type().Field(i).Name
288+
assert.NotEqual(t, fields.Field(i).Pointer(), clonedFields.Field(i).Pointer(), keyword)
289+
assert.Equal(t, fields.Field(i).Elem().Interface(), clonedFields.Field(i).Elem().Interface(), keyword)
290+
}
291+
292+
// Mutating caller-owned bounds must not race with cached-schema reads.
293+
var workers sync.WaitGroup
294+
workers.Add(2)
295+
go func() {
296+
defer workers.Done()
297+
for range 100 {
298+
for _, i := range numericFields {
299+
value := fields.Field(i).Elem()
300+
if value.Kind() == reflect.Int {
301+
value.SetInt(22)
302+
} else {
303+
value.SetFloat(22)
304+
}
305+
}
306+
}
307+
}()
308+
go func() {
309+
defer workers.Done()
310+
for range 100 {
311+
if _, err := json.Marshal(cloned); err != nil {
312+
t.Errorf("marshal detached schema: %v", err)
313+
return
314+
}
315+
}
316+
}()
317+
workers.Wait()
318+
for _, i := range numericFields {
319+
value := clonedFields.Field(i).Elem()
320+
if value.Kind() == reflect.Int {
321+
assert.Equal(t, int64(11), value.Int())
322+
} else {
323+
assert.Equal(t, float64(11), value.Float())
324+
}
325+
}
326+
assert.Nil(t, cloned.Minimum, "absent bounds must remain absent")
327+
})
328+
}
329+
}
330+
331+
func TestCloneSchemaWithoutDefaultsTraversesAllChildKinds(t *testing.T) {
332+
schemaType := reflect.TypeFor[jsonschema.Schema]()
333+
for i := range schemaType.NumField() {
334+
field := schemaType.Field(i)
335+
if field.Type != reflect.TypeFor[*jsonschema.Schema]() &&
336+
field.Type != reflect.TypeFor[[]*jsonschema.Schema]() &&
337+
field.Type != reflect.TypeFor[map[string]*jsonschema.Schema]() {
338+
continue
339+
}
340+
t.Run(field.Name, func(t *testing.T) {
341+
original := &jsonschema.Schema{Default: json.RawMessage(`1`)}
342+
// A single deep chain makes repeated per-level traversal exponential.
343+
const depth = 48
344+
for range depth {
345+
parent := &jsonschema.Schema{Default: json.RawMessage(`1`)}
346+
target := reflect.ValueOf(parent).Elem().Field(i)
347+
switch field.Type {
348+
case reflect.TypeFor[*jsonschema.Schema]():
349+
target.Set(reflect.ValueOf(original))
350+
case reflect.TypeFor[[]*jsonschema.Schema]():
351+
target.Set(reflect.ValueOf([]*jsonschema.Schema{original}))
352+
case reflect.TypeFor[map[string]*jsonschema.Schema]():
353+
target.Set(reflect.ValueOf(map[string]*jsonschema.Schema{"child": original}))
354+
}
355+
original = parent
356+
}
357+
cloned := CloneSchemaWithoutDefaults(original)
358+
for level := 0; level <= depth; level++ {
359+
require.NotSame(t, original, cloned)
360+
assert.Equal(t, json.RawMessage(`1`), original.Default)
361+
assert.Nil(t, cloned.Default)
362+
var originalChild, clonedChild *jsonschema.Schema
363+
for _, child := range schemaChildren(original) {
364+
if child != nil {
365+
originalChild = child
366+
}
367+
}
368+
for _, child := range schemaChildren(cloned) {
369+
if child != nil {
370+
clonedChild = child
371+
}
372+
}
373+
original, cloned = originalChild, clonedChild
374+
}
375+
assert.Nil(t, original)
376+
assert.Nil(t, cloned)
377+
})
378+
}
379+
}
380+
244381
func TestSchemaInferenceOptionsDeepCopyOverrides(t *testing.T) {
245382
type input struct {
246383
State string `json:"state"`

‎pkg/inventory/typed_schema.go‎

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ func CloneSchemaWithoutDefaults(schema *jsonschema.Schema) *jsonschema.Schema {
122122
return clonedSchema
123123
}
124124

125-
// CloneSchema deep-copies schema nodes and mutable metadata collections.
125+
// CloneSchema deep-copies schema nodes, numeric bounds, and mutable metadata.
126126
func CloneSchema(schema *jsonschema.Schema) *jsonschema.Schema {
127127
clonedSchema := schema.CloneSchemas()
128128
cloneSchemaMetadata(clonedSchema)
@@ -138,6 +138,19 @@ func cloneSchemaMetadata(schema *jsonschema.Schema) {
138138
if schema == nil {
139139
return
140140
}
141+
schema.MultipleOf = cloneSchemaPointer(schema.MultipleOf)
142+
schema.Minimum = cloneSchemaPointer(schema.Minimum)
143+
schema.Maximum = cloneSchemaPointer(schema.Maximum)
144+
schema.ExclusiveMinimum = cloneSchemaPointer(schema.ExclusiveMinimum)
145+
schema.ExclusiveMaximum = cloneSchemaPointer(schema.ExclusiveMaximum)
146+
schema.MinLength = cloneSchemaPointer(schema.MinLength)
147+
schema.MaxLength = cloneSchemaPointer(schema.MaxLength)
148+
schema.MinItems = cloneSchemaPointer(schema.MinItems)
149+
schema.MaxItems = cloneSchemaPointer(schema.MaxItems)
150+
schema.MinContains = cloneSchemaPointer(schema.MinContains)
151+
schema.MaxContains = cloneSchemaPointer(schema.MaxContains)
152+
schema.MinProperties = cloneSchemaPointer(schema.MinProperties)
153+
schema.MaxProperties = cloneSchemaPointer(schema.MaxProperties)
141154
schema.Types = slices.Clone(schema.Types)
142155
schema.Enum = cloneSchemaValue(reflect.ValueOf(schema.Enum), make(map[schemaValueVisit]reflect.Value)).Interface().([]any)
143156
schema.Default = slices.Clone(schema.Default)
@@ -176,6 +189,14 @@ func cloneSchemaMetadata(schema *jsonschema.Schema) {
176189
}
177190
}
178191

192+
func cloneSchemaPointer[T int | float64](value *T) *T {
193+
if value == nil {
194+
return nil
195+
}
196+
cloned := *value
197+
return &cloned
198+
}
199+
179200
// cloneSchemaValue preserves the concrete metadata types while detaching
180201
// nested mutable collections from schemas retained by callers.
181202
func cloneSchemaValue(value reflect.Value, visited map[schemaValueVisit]reflect.Value) reflect.Value {
@@ -284,34 +305,6 @@ func removeSchemaDefaults(schema *jsonschema.Schema) {
284305
return
285306
}
286307
schema.Default = nil
287-
for _, children := range []map[string]*jsonschema.Schema{
288-
schema.Defs,
289-
schema.Definitions,
290-
schema.Properties,
291-
schema.PatternProperties,
292-
schema.DependentSchemas,
293-
schema.DependencySchemas,
294-
} {
295-
for _, child := range children {
296-
removeSchemaDefaults(child)
297-
}
298-
}
299-
for _, child := range []*jsonschema.Schema{
300-
schema.Items,
301-
schema.AdditionalItems,
302-
schema.Contains,
303-
schema.UnevaluatedItems,
304-
schema.AdditionalProperties,
305-
schema.PropertyNames,
306-
schema.UnevaluatedProperties,
307-
schema.Not,
308-
schema.If,
309-
schema.Then,
310-
schema.Else,
311-
schema.ContentSchema,
312-
} {
313-
removeSchemaDefaults(child)
314-
}
315308
for _, child := range schemaChildren(schema) {
316309
removeSchemaDefaults(child)
317310
}

0 commit comments

Comments
 (0)