diff --git a/internal/mcp/bridge_tool.go b/internal/mcp/bridge_tool.go index 2315b30a..d1bba9d6 100644 --- a/internal/mcp/bridge_tool.go +++ b/internal/mcp/bridge_tool.go @@ -154,9 +154,8 @@ func inputSchemaToMap(schema mcpgo.ToolInputSchema) map[string]any { } // stripEmptyOptionalArgs removes optional args with empty/placeholder values. -// LLMs often send "" or placeholder strings (e.g. "__OMIT__", "null", "none") -// for optional fields instead of omitting them, causing MCP servers to reject -// invalid values (e.g. empty string for UUID fields). +// LLMs often send "", "optional", "null", or null for optional fields instead +// of omitting them, causing MCP servers to reject invalid values. func (t *BridgeTool) stripEmptyOptionalArgs(args map[string]any) map[string]any { if len(args) == 0 { return args @@ -167,35 +166,56 @@ func (t *BridgeTool) stripEmptyOptionalArgs(args map[string]any) map[string]any cleaned[k] = v continue } - // Strip nil for optional fields. + // Strip nil/null for optional fields (also handles strict mode where model sends null). if v == nil { continue } - // Strip empty strings and common LLM placeholder values for optional fields. - if s, ok := v.(string); ok && isPlaceholderValue(s) { - continue + if s, ok := v.(string); ok { + // Strip known placeholder values (e.g. "optional", "null", "http://example.com"). + if isPlaceholderValue(s) { + continue + } + // Type-aware empty string: keep for string-typed params (user may want empty), + // strip for non-string params (empty string is never valid for number/boolean/UUID). + if s == "" && t.propertyType(k) != "string" { + continue + } } cleaned[k] = v } return cleaned } -// isPlaceholderValue returns true for empty or placeholder strings that LLMs -// commonly use when they don't intend to set an optional parameter. +// propertyType returns the JSON Schema "type" for a property, or "" if unknown. +func (t *BridgeTool) propertyType(name string) string { + props, _ := t.inputSchema["properties"].(map[string]any) + if props == nil { + return "" + } + prop, _ := props[name].(map[string]any) + if prop == nil { + return "" + } + typ, _ := prop["type"].(string) + return typ +} + +// isPlaceholderValue returns true for placeholder strings that LLMs commonly +// generate when they don't intend to set an optional parameter. +// Empty string ("") is NOT handled here — see stripEmptyOptionalArgs for type-aware handling. func isPlaceholderValue(s string) bool { - // NOTE: empty string "" is NOT stripped — it may be intentional for text fields. - // Only strip known placeholder keywords and all-caps patterns. if s == "" { return false } - // Normalize for case-insensitive comparison. lower := strings.ToLower(strings.TrimSpace(s)) switch lower { case "null", "none", "nil", "undefined", "n/a", - "__omit__", "__skip__", "__empty__": + "optional", "skip", // LLMs copy these from schema descriptions + "__omit__", "__skip__", "__empty__", + "http://example.com", "https://example.com", // common hallucinated URLs + "http://localhost", "https://localhost": return true } - // Catch all-caps placeholder patterns like "SHOULD_NOT_BE_HERE", "DO_NOT_SEND", "NOT_SET". if isAllCapsPlaceholder(s) { return true } diff --git a/internal/mcp/bridge_tool_test.go b/internal/mcp/bridge_tool_test.go index 783048e9..cd7d751e 100644 --- a/internal/mcp/bridge_tool_test.go +++ b/internal/mcp/bridge_tool_test.go @@ -131,6 +131,110 @@ func TestBridgeToolNaming(t *testing.T) { } } +func TestIsPlaceholderValue(t *testing.T) { + // Should be detected as placeholder. + placeholders := []string{ + "null", "None", "nil", "UNDEFINED", "n/a", + "optional", "Optional", "OPTIONAL", + "skip", "Skip", + "__OMIT__", "__skip__", "__EMPTY__", + "http://example.com", "https://example.com", + "http://localhost", "https://localhost", + "PLACEHOLDER", "NOT_SET", "DO_NOT_SEND", + } + for _, s := range placeholders { + if !isPlaceholderValue(s) { + t.Errorf("expected isPlaceholderValue(%q) = true", s) + } + } + + // Should NOT be detected as placeholder (real values). + realValues := []string{ + "", // empty string handled separately by type-aware check + "sk-abc123", + "my-proxy.example.com", + "https://api.reviewweb.site/v1", + "gpt-4o-mini", + "bullet", + "hello world", + "ab", // too short for all-caps check + } + for _, s := range realValues { + if isPlaceholderValue(s) { + t.Errorf("expected isPlaceholderValue(%q) = false", s) + } + } +} + +func TestStripEmptyOptionalArgs(t *testing.T) { + bt := &BridgeTool{ + requiredSet: map[string]bool{"url": true}, + inputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "url": map[string]any{"type": "string"}, + "api_key": map[string]any{"type": "string"}, + "timeout": map[string]any{"type": "number"}, + "debug": map[string]any{"type": "boolean"}, + "keywords": map[string]any{"type": "string"}, + }, + }, + } + + args := map[string]any{ + "url": "https://example.com", + "api_key": "optional", // placeholder → strip + "timeout": nil, // nil → strip + "debug": true, // real boolean → keep + "keywords": "", // empty string for string-typed → keep + } + + cleaned := bt.stripEmptyOptionalArgs(args) + + if cleaned["url"] != "https://example.com" { + t.Error("required param 'url' should be preserved") + } + if _, ok := cleaned["api_key"]; ok { + t.Error("placeholder 'optional' should be stripped for api_key") + } + if _, ok := cleaned["timeout"]; ok { + t.Error("nil should be stripped for timeout") + } + if cleaned["debug"] != true { + t.Error("real boolean value should be preserved") + } + if v, ok := cleaned["keywords"]; !ok || v != "" { + t.Error("empty string should be kept for string-typed optional param 'keywords'") + } +} + +func TestStripEmptyOptionalArgs_EmptyStringNonString(t *testing.T) { + bt := &BridgeTool{ + requiredSet: map[string]bool{}, + inputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "timeout": map[string]any{"type": "number"}, + "count": map[string]any{"type": "integer"}, + }, + }, + } + + args := map[string]any{ + "timeout": "", + "count": "", + } + + cleaned := bt.stripEmptyOptionalArgs(args) + + if _, ok := cleaned["timeout"]; ok { + t.Error("empty string should be stripped for number-typed param") + } + if _, ok := cleaned["count"]; ok { + t.Error("empty string should be stripped for integer-typed param") + } +} + func TestEnsureMCPPrefix(t *testing.T) { tests := []struct { name string diff --git a/internal/providers/schema_cleaner.go b/internal/providers/schema_cleaner.go index d527865f..4b118cdc 100644 --- a/internal/providers/schema_cleaner.go +++ b/internal/providers/schema_cleaner.go @@ -6,6 +6,12 @@ func CleanToolSchemas(providerName string, tools []ToolDefinition) []ToolDefinit if len(tools) == 0 { return tools } + profile := profileForProvider(providerName) + var strictPtr *bool + if profile.StrictToolMode { + t := true + strictPtr = &t + } cleaned := make([]ToolDefinition, len(tools)) for i, t := range tools { cleaned[i] = ToolDefinition{ @@ -14,6 +20,7 @@ func CleanToolSchemas(providerName string, tools []ToolDefinition) []ToolDefinit Name: t.Function.Name, Description: t.Function.Description, Parameters: NormalizeSchema(providerName, t.Function.Parameters), + Strict: strictPtr, }, } } diff --git a/internal/providers/schema_normalize.go b/internal/providers/schema_normalize.go index ff51bd20..63cc4d62 100644 --- a/internal/providers/schema_normalize.go +++ b/internal/providers/schema_normalize.go @@ -39,6 +39,9 @@ func NormalizeSchema(providerName string, schema map[string]any) map[string]any if len(profile.StripKeys) > 0 { result = stripKeys(result, profile.StripKeys, 0) } + if profile.StrictToolMode { + result = applyStrictMode(result, 0) + } return result } diff --git a/internal/providers/schema_normalize_test.go b/internal/providers/schema_normalize_test.go index 15a853ec..444724e9 100644 --- a/internal/providers/schema_normalize_test.go +++ b/internal/providers/schema_normalize_test.go @@ -52,7 +52,8 @@ func TestResolveRefs_Circular(t *testing.T) { }, }, } - result := NormalizeSchema("openai", schema) + // Use "anthropic" to test ref resolution in isolation (no strict mode transform). + result := NormalizeSchema("anthropic", schema) node := prop(result, "node") if node == nil { t.Fatal("expected node property") @@ -77,7 +78,8 @@ func TestResolveRefs_LegacyDefinitions(t *testing.T) { "Item": map[string]any{"type": "string"}, }, } - result := NormalizeSchema("openai", schema) + // Use "anthropic" to test ref resolution in isolation (no strict mode transform). + result := NormalizeSchema("anthropic", schema) item := prop(result, "item") if item == nil || item["type"] != "string" { t.Error("expected definitions/ ref resolved to string type") @@ -410,6 +412,123 @@ func TestNormalizeSchema_DoesNotMutateOriginal(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Strict tool mode +// --------------------------------------------------------------------------- + +func TestApplyStrictMode_Basic(t *testing.T) { + schema := map[string]any{ + "type": "object", + "properties": map[string]any{ + "url": map[string]any{"type": "string", "description": "The URL"}, + "api_key": map[string]any{"type": "string", "description": "API key"}, + "timeout": map[string]any{"type": "number", "description": "Timeout"}, + }, + "required": []any{"url"}, + } + result := NormalizeSchema("openai", schema) + + // All properties should be required. + reqArr, _ := result["required"].([]any) + reqSet := make(map[string]bool, len(reqArr)) + for _, r := range reqArr { + reqSet[r.(string)] = true + } + for _, name := range []string{"url", "api_key", "timeout"} { + if !reqSet[name] { + t.Errorf("expected %q in required array", name) + } + } + + // additionalProperties should be false. + if result["additionalProperties"] != false { + t.Error("expected additionalProperties:false") + } + + // Required param 'url' should keep original type. + urlProp := prop(result, "url") + if urlProp["type"] != "string" { + t.Errorf("expected url type:string, got %v", urlProp["type"]) + } + + // Optional params should be nullable. + apiKeyProp := prop(result, "api_key") + apiKeyType, ok := apiKeyProp["type"].([]any) + if !ok { + t.Fatalf("expected api_key type to be array, got %T: %v", apiKeyProp["type"], apiKeyProp["type"]) + } + hasNull := false + for _, v := range apiKeyType { + if v == "null" { + hasNull = true + } + } + if !hasNull { + t.Error("expected api_key type to include 'null'") + } + + timeoutProp := prop(result, "timeout") + timeoutType, ok := timeoutProp["type"].([]any) + if !ok { + t.Fatalf("expected timeout type to be array, got %T", timeoutProp["type"]) + } + hasNull = false + for _, v := range timeoutType { + if v == "null" { + hasNull = true + } + } + if !hasNull { + t.Error("expected timeout type to include 'null'") + } +} + +func TestApplyStrictMode_NestedObject(t *testing.T) { + schema := map[string]any{ + "type": "object", + "properties": map[string]any{ + "config": map[string]any{ + "type": "object", + "properties": map[string]any{ + "key": map[string]any{"type": "string"}, + }, + }, + }, + "required": []any{}, + } + result := NormalizeSchema("openai", schema) + + // Nested object should also have additionalProperties:false. + config := prop(result, "config") + if config == nil { + t.Fatal("expected config property") + } + if config["additionalProperties"] != false { + t.Error("expected nested object to have additionalProperties:false") + } +} + +func TestApplyStrictMode_SkipsAnthropic(t *testing.T) { + schema := map[string]any{ + "type": "object", + "properties": map[string]any{ + "url": map[string]any{"type": "string"}, + "debug": map[string]any{"type": "boolean"}, + }, + "required": []any{"url"}, + } + result := NormalizeSchema("anthropic", schema) + + // Anthropic should NOT get strict mode transforms. + if result["additionalProperties"] == false { + t.Error("Anthropic should not have additionalProperties:false") + } + debugProp := prop(result, "debug") + if debugProp["type"] != "boolean" { + t.Error("Anthropic should keep original type (no nullable transform)") + } +} + // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- diff --git a/internal/providers/schema_profile.go b/internal/providers/schema_profile.go index 908e1153..7944638e 100644 --- a/internal/providers/schema_profile.go +++ b/internal/providers/schema_profile.go @@ -13,6 +13,7 @@ type SchemaProfile struct { StripNullType bool // anyOf:[T, null] → T RemoveTypeOnUnion bool // strip "type" when anyOf/oneOf present (Gemini conflict) StripKeys []string // keys to recursively remove + StrictToolMode bool // OpenAI strict mode: optional→nullable, all props required, additionalProperties:false } // Provider-specific strip key lists. @@ -59,7 +60,15 @@ func profileForProvider(name string) SchemaProfile { InjectObjectType: true, StripKeys: xaiStripKeys, } - default: // openai, codex, openrouter, deepseek, groq, dashscope, etc. + case isOpenAIStrict(name): + return SchemaProfile{ + ResolveRefs: true, + FlattenUnions: true, + InjectObjectType: true, + StrictToolMode: true, + StripKeys: refOnlyStripKeys, + } + default: // openrouter, deepseek, groq, dashscope, bailian, minimax, etc. return SchemaProfile{ ResolveRefs: true, FlattenUnions: true, @@ -69,6 +78,14 @@ func profileForProvider(name string) SchemaProfile { } } +// isOpenAIStrict returns true for providers known to support strict tool mode. +// Only first-party OpenAI and Codex are safe — third-party proxies (OpenRouter, +// DeepSeek, Groq, DashScope, etc.) may reject nullable unions or the strict flag. +func isOpenAIStrict(name string) bool { + lower := strings.ToLower(name) + return lower == "openai" || lower == "codex" +} + // isGeminiName matches config names ("gemini", "gemini-flash") and // DB provider types ("gemini_native"). Uses Contains for robustness // with user-defined names (e.g. "my-gemini-proxy"). diff --git a/internal/providers/schema_strict.go b/internal/providers/schema_strict.go new file mode 100644 index 00000000..9e6de02a --- /dev/null +++ b/internal/providers/schema_strict.go @@ -0,0 +1,116 @@ +package providers + +// schema_strict.go — OpenAI strict tool mode transform. +// Converts optional properties to nullable unions, requires all properties, +// and sets additionalProperties:false so constrained decoding can enforce schema compliance. + +// applyStrictMode transforms a tool's JSON Schema for OpenAI strict mode. +// - Optional properties (not in "required") get their type changed to ["", "null"] +// - ALL property names are added to "required" +// - "additionalProperties": false is set on object schemas +// This is applied recursively to nested object schemas. +func applyStrictMode(schema map[string]any, depth int) map[string]any { + if schema == nil || depth > maxSchemaDepth { + return schema + } + + typ, _ := schema["type"].(string) + props, hasProps := schema["properties"].(map[string]any) + + if typ != "object" || !hasProps { + return schema + } + + // Build the set of currently required properties. + reqSet := make(map[string]bool) + if reqArr, ok := schema["required"].([]any); ok { + for _, r := range reqArr { + if s, ok := r.(string); ok { + reqSet[s] = true + } + } + } + if reqArr, ok := schema["required"].([]string); ok { + for _, s := range reqArr { + reqSet[s] = true + } + } + + // Collect all property names for the new required array. + allRequired := make([]any, 0, len(props)) + + for name, prop := range props { + allRequired = append(allRequired, name) + + pm, ok := prop.(map[string]any) + if !ok { + continue + } + + // Recurse into nested objects first. + pm = applyStrictMode(pm, depth+1) + props[name] = pm + + // Recurse into array items. + if items, ok := pm["items"].(map[string]any); ok { + pm["items"] = applyStrictMode(items, depth+1) + } + + // Already required — no need to make nullable. + if reqSet[name] { + continue + } + + // Make optional property nullable: type:"string" → type:["string","null"] + makeNullable(pm) + } + + schema["properties"] = props + schema["required"] = allRequired + schema["additionalProperties"] = false + + return schema +} + +// makeNullable converts a property schema to accept null values. +// - Simple type: "string" → ["string", "null"] +// - Type array: ["string", "integer"] → ["string", "integer", "null"] +// - anyOf/oneOf: appends {"type":"null"} variant +// - No type: adds type:["null"] (fallback) +func makeNullable(schema map[string]any) { + // Already has null variant in anyOf/oneOf — skip. + for _, key := range []string{"anyOf", "oneOf"} { + if variants, ok := schema[key].([]any); ok { + for _, v := range variants { + if m, ok := v.(map[string]any); ok && isNullSchema(m) { + return // already nullable + } + } + // Append null variant. + schema[key] = append(variants, map[string]any{"type": "null"}) + return + } + } + + typ, hasType := schema["type"] + + switch t := typ.(type) { + case string: + if t == "null" { + return // already null + } + schema["type"] = []any{t, "null"} + case []any: + for _, v := range t { + if s, ok := v.(string); ok && s == "null" { + return // already has null + } + } + schema["type"] = append(t, "null") + default: + if !hasType { + // No type field — add nullable object as fallback. + schema["type"] = []any{"object", "null"} + } + } +} diff --git a/internal/providers/types.go b/internal/providers/types.go index 1f740da7..a56c7e7d 100644 --- a/internal/providers/types.go +++ b/internal/providers/types.go @@ -160,6 +160,7 @@ type ToolFunctionSchema struct { Name string `json:"name"` Description string `json:"description"` Parameters map[string]any `json:"parameters"` + Strict *bool `json:"strict,omitempty"` // OpenAI strict mode — constrained decoding } // Usage tracks token consumption.