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") }, ) }