From e2219c9d1af0a3b405f2a75e95b9e381468a4274 Mon Sep 17 00:00:00 2001 From: Bryan Frimin Date: Mon, 22 Jun 2026 08:40:10 +0200 Subject: [PATCH] Normalize required fields in agent JSON schemas for OpenAI OpenAI rejects schemas where optional properties are absent from the required array. Promote all properties to required and mark formerly optional ones nullable so the model knows it may pass null. Also upgrade tool error log level from Warn to Error. Signed-off-by: Bryan Frimin --- pkg/agent/run.go | 2 +- pkg/agent/schema.go | 125 +++++++++++++++++++++++++++++++++++++++ pkg/agent/schema_test.go | 33 +++++++++-- pkg/agent/tool_test.go | 12 +++- 4 files changed, 163 insertions(+), 9 deletions(-) diff --git a/pkg/agent/run.go b/pkg/agent/run.go index 0d32d3be1..82706f71a 100644 --- a/pkg/agent/run.go +++ b/pkg/agent/run.go @@ -1279,7 +1279,7 @@ func executeSingleTool( content = content[:200] + "... (truncated)" } - logger.WarnCtx( + logger.ErrorCtx( ctx, "tool returned error", log.String("tool", tool.Name()), diff --git a/pkg/agent/schema.go b/pkg/agent/schema.go index 178f2cfdd..927bf99ed 100644 --- a/pkg/agent/schema.go +++ b/pkg/agent/schema.go @@ -18,6 +18,7 @@ import ( "encoding/json" "fmt" "reflect" + "slices" "github.com/google/jsonschema-go/jsonschema" ) @@ -37,6 +38,14 @@ func jsonSchemaFor[T any]() (json.RawMessage, error) { return nil, fmt.Errorf("cannot marshal schema for %s: %w", t, err) } + // OpenAI rejects schemas where required does not list every key in + // properties. Promote optional properties into required and mark them + // nullable so the model knows it may pass null. + data, err = normalizeRequiredJSON(data) + if err != nil { + return nil, fmt.Errorf("cannot normalize schema for %s: %w", t, err) + } + return json.RawMessage(data), nil } @@ -49,6 +58,122 @@ func mustJSONSchemaFor[T any]() json.RawMessage { return schema } +// normalizeRequiredJSON ensures every property key in an object schema also +// appears in its required array. Properties that were not originally required +// are made nullable (their "type" becomes ["T","null"]) so the LLM knows it +// may pass null for them. The transformation is applied recursively so nested +// object schemas are also normalised. +func normalizeRequiredJSON(data []byte) ([]byte, error) { + var obj map[string]json.RawMessage + if err := json.Unmarshal(data, &obj); err != nil { + return data, nil + } + + propsRaw, hasProps := obj["properties"] + if !hasProps { + if itemsRaw, ok := obj["items"]; ok { + n, err := normalizeRequiredJSON(itemsRaw) + if err != nil { + return nil, err + } + obj["items"] = n + } + if addlRaw, ok := obj["additionalProperties"]; ok { + n, err := normalizeRequiredJSON(addlRaw) + if err != nil { + return nil, err + } + obj["additionalProperties"] = n + } + return json.Marshal(obj) + } + + var props map[string]json.RawMessage + if err := json.Unmarshal(propsRaw, &props); err != nil { + return data, nil + } + + var required []string + if reqRaw, ok := obj["required"]; ok { + _ = json.Unmarshal(reqRaw, &required) + } + + requiredSet := make(map[string]bool, len(required)) + for _, r := range required { + requiredSet[r] = true + } + + for name, propRaw := range props { + n, err := normalizeRequiredJSON(propRaw) + if err != nil { + return nil, err + } + + if !requiredSet[name] { + n, err = makeNullableJSON(n) + if err != nil { + return nil, err + } + required = append(required, name) + requiredSet[name] = true + } + + props[name] = n + } + + propsData, err := json.Marshal(props) + if err != nil { + return nil, err + } + obj["properties"] = propsData + + if len(required) > 0 { + reqData, err := json.Marshal(required) + if err != nil { + return nil, err + } + obj["required"] = reqData + } + + return json.Marshal(obj) +} + +// makeNullableJSON adds "null" to the "type" field of a JSON Schema object so +// that the LLM understands it may pass null for optional properties. +func makeNullableJSON(data []byte) ([]byte, error) { + var obj map[string]json.RawMessage + if err := json.Unmarshal(data, &obj); err != nil { + return data, nil + } + + typeRaw, ok := obj["type"] + if !ok { + return data, nil + } + + var single string + if err := json.Unmarshal(typeRaw, &single); err == nil { + if single != "null" { + arr, _ := json.Marshal([]string{single, "null"}) + obj["type"] = arr + } + return json.Marshal(obj) + } + + var arr []string + if err := json.Unmarshal(typeRaw, &arr); err == nil { + if slices.Contains(arr, "null") { + return data, nil + } + arr = append(arr, "null") + nullable, _ := json.Marshal(arr) + obj["type"] = nullable + return json.Marshal(obj) + } + + return data, nil +} + // stripNullTypes removes "null" from union types produced by pointer fields // (e.g. ["null","string"] becomes "string") and clears integer bounds so that // LLM providers receive a clean schema without Go-specific type constraints. diff --git a/pkg/agent/schema_test.go b/pkg/agent/schema_test.go index a58a7b1a6..9ef0f88bb 100644 --- a/pkg/agent/schema_test.go +++ b/pkg/agent/schema_test.go @@ -25,6 +25,8 @@ import ( func TestGenerateSchema_PointerFieldsStripNull(t *testing.T) { t.Parallel() + // Pointer fields without omitempty are treated as required by the schema + // library. null is stripped and they appear as simple non-nullable types. type Params struct { Name *string `json:"name"` Count *int `json:"count"` @@ -51,7 +53,6 @@ func TestGenerateSchema_PointerFieldsStripNull(t *testing.T) { } { prop := props[field.name].(map[string]any) assert.Equal(t, field.wantType, prop["type"], "field %s", field.name) - assert.Nil(t, prop["types"], "field %s should not have union types", field.name) } } @@ -142,10 +143,12 @@ func TestGenerateSchema_NestedPointerStruct(t *testing.T) { props := schema["properties"].(map[string]any) + // Pointer-to-struct without omitempty is in required with a simple type. innerProp := props["inner"].(map[string]any) assert.Equal(t, "object", innerProp["type"]) assert.Nil(t, innerProp["types"], "pointer to struct should not have union types") + // Nested pointer field without omitempty is also required and non-nullable. innerProps := innerProp["properties"].(map[string]any) valueProp := innerProps["value"].(map[string]any) assert.Equal(t, "string", valueProp["type"]) @@ -239,11 +242,30 @@ func TestGenerateSchema_RequiredVsOptional(t *testing.T) { var schema map[string]any require.NoError(t, json.Unmarshal(raw, &schema)) + // All properties must appear in required (OpenAI requirement). required := schema["required"].([]any) assert.Contains(t, required, "required") assert.Contains(t, required, "also_needed") - assert.NotContains(t, required, "optional") - assert.NotContains(t, required, "omit_empty") + assert.Contains(t, required, "optional") + assert.Contains(t, required, "omit_empty") + + // Optional properties must be nullable so the model can pass null. + props := schema["properties"].(map[string]any) + + optionalType := props["optional"].(map[string]any)["type"].([]any) + assert.Contains(t, optionalType, "string") + assert.Contains(t, optionalType, "null") + + omitEmptyType := props["omit_empty"].(map[string]any)["type"].([]any) + assert.Contains(t, omitEmptyType, "string") + assert.Contains(t, omitEmptyType, "null") + + // Truly required properties must not be nullable. + requiredType := props["required"].(map[string]any)["type"] + assert.Equal(t, "string", requiredType) + + alsoNeededType := props["also_needed"].(map[string]any)["type"] + assert.Equal(t, "integer", alsoNeededType) } func TestGenerateSchema_DeeplyNestedStructure(t *testing.T) { @@ -327,8 +349,9 @@ func TestGenerateSchema_SliceOfStructs(t *testing.T) { assert.Contains(t, itemProps, "count") countProp := itemProps["count"].(map[string]any) - assert.Equal(t, "integer", countProp["type"]) - assert.Nil(t, countProp["types"]) + countType := countProp["type"].([]any) + assert.Contains(t, countType, "integer") + assert.Contains(t, countType, "null") assert.Nil(t, countProp["minimum"]) } diff --git a/pkg/agent/tool_test.go b/pkg/agent/tool_test.go index 5a551ea67..9630c4752 100644 --- a/pkg/agent/tool_test.go +++ b/pkg/agent/tool_test.go @@ -128,7 +128,7 @@ func TestFunctionTool_Definition(t *testing.T) { ) t.Run( - "pointer fields are not nullable in schema", + "omitempty fields are nullable and in required", func(t *testing.T) { t.Parallel() @@ -147,10 +147,16 @@ func TestFunctionTool_Definition(t *testing.T) { var schema map[string]any require.NoError(t, json.Unmarshal(tool.Definition().Parameters, &schema)) + // OpenAI requires all properties to be in required; optional + // fields are represented as nullable. + required := schema["required"].([]any) + assert.Contains(t, required, "title") + props := schema["properties"].(map[string]any) titleProp := props["title"].(map[string]any) - assert.Equal(t, "string", titleProp["type"]) - assert.Nil(t, titleProp["types"]) + titleType := titleProp["type"].([]any) + assert.Contains(t, titleType, "string") + assert.Contains(t, titleType, "null") }, ) }