diff --git a/internal/providers/schema_cleaner.go b/internal/providers/schema_cleaner.go index 4b118cdc..7075fde2 100644 --- a/internal/providers/schema_cleaner.go +++ b/internal/providers/schema_cleaner.go @@ -7,19 +7,28 @@ func CleanToolSchemas(providerName string, tools []ToolDefinition) []ToolDefinit 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 { + // Exempt multi-action tools from strict mode — their many optional + // params become required under strict, forcing models to send empty values + // for every call (wasting ~200-300 output tokens per tool call). + useStrict := profile.StrictToolMode && !IsMultiActionSchema(t.Function.Parameters) + + var strictPtr *bool + if useStrict { + tr := true + strictPtr = &tr + } + + toolProfile := profile + toolProfile.StrictToolMode = useStrict + cleaned[i] = ToolDefinition{ Type: t.Type, Function: ToolFunctionSchema{ Name: t.Function.Name, Description: t.Function.Description, - Parameters: NormalizeSchema(providerName, t.Function.Parameters), + Parameters: normalizeWithProfile(toolProfile, t.Function.Parameters), Strict: strictPtr, }, } diff --git a/internal/providers/schema_cleaner_test.go b/internal/providers/schema_cleaner_test.go index ebc7000f..2c88fd5d 100644 --- a/internal/providers/schema_cleaner_test.go +++ b/internal/providers/schema_cleaner_test.go @@ -162,6 +162,196 @@ func TestCleanSchema_NestedArray(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Multi-action tool strict mode exemption +// --------------------------------------------------------------------------- + +func TestIsMultiActionSchema(t *testing.T) { + tests := []struct { + name string + schema map[string]any + want bool + }{ + { + name: "team_tasks-like with []string enum", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "action": map[string]any{ + "type": "string", + "enum": []string{"list", "create", "search"}, + }, + "query": map[string]any{"type": "string"}, + }, + "required": []string{"action"}, + }, + want: true, + }, + { + name: "cron-like with []any enum", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "action": map[string]any{ + "type": "string", + "enum": []any{"status", "add", "remove"}, + }, + }, + }, + want: true, + }, + { + name: "single action — not exempt", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "action": map[string]any{ + "type": "string", + "enum": []string{"send"}, + }, + }, + }, + want: false, + }, + { + name: "no action property", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "query": map[string]any{"type": "string"}, + }, + }, + want: false, + }, + { + name: "action without enum", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "action": map[string]any{"type": "string"}, + }, + }, + want: false, + }, + { + name: "enum on non-action field", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "mode": map[string]any{ + "type": "string", + "enum": []string{"markdown", "text"}, + }, + }, + }, + want: false, + }, + { + name: "nil schema", + schema: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsMultiActionSchema(tt.schema) + if got != tt.want { + t.Errorf("IsMultiActionSchema() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestCleanToolSchemas_OpenAI_MultiActionExempt(t *testing.T) { + multiAction := ToolDefinition{ + Type: "function", + Function: ToolFunctionSchema{ + Name: "team_tasks", + Description: "multi-action tool", + Parameters: map[string]any{ + "type": "object", + "properties": map[string]any{ + "action": map[string]any{ + "type": "string", + "enum": []string{"list", "create", "search"}, + }, + "query": map[string]any{ + "type": "string", + "description": "search query", + }, + "subject": map[string]any{ + "type": "string", + "description": "task subject", + }, + }, + "required": []string{"action"}, + }, + }, + } + simple := ToolDefinition{ + Type: "function", + Function: ToolFunctionSchema{ + Name: "read_file", + Description: "simple tool", + Parameters: map[string]any{ + "type": "object", + "properties": map[string]any{ + "path": map[string]any{ + "type": "string", + "description": "file path", + }, + }, + "required": []string{"path"}, + }, + }, + } + + cleaned := CleanToolSchemas("openai", []ToolDefinition{multiAction, simple}) + + // Multi-action tool: strict should be nil (exempt). + if cleaned[0].Function.Strict != nil { + t.Errorf("multi-action tool: expected Strict=nil, got %v", *cleaned[0].Function.Strict) + } + + // Multi-action tool: required should stay ["action"] only. + params0 := cleaned[0].Function.Parameters + reqArr, ok := params0["required"].([]string) + if !ok { + // May be []any from original schema copy. + if reqAny, ok2 := params0["required"].([]any); ok2 { + if len(reqAny) != 1 { + t.Errorf("multi-action: expected 1 required, got %d", len(reqAny)) + } + } + } else if len(reqArr) != 1 { + t.Errorf("multi-action: expected 1 required, got %d: %v", len(reqArr), reqArr) + } + + // Multi-action tool: should NOT have additionalProperties:false. + if _, hasAP := params0["additionalProperties"]; hasAP { + t.Error("multi-action: expected no additionalProperties") + } + + // Simple tool: strict should be true. + if cleaned[1].Function.Strict == nil || !*cleaned[1].Function.Strict { + t.Error("simple tool: expected Strict=true") + } + + // Simple tool: all props required (strict mode applied). + params1 := cleaned[1].Function.Parameters + if reqAny, ok := params1["required"].([]any); ok { + if len(reqAny) != 1 { + t.Errorf("simple tool: expected 1 required (path), got %d", len(reqAny)) + } + } + + // Simple tool: additionalProperties:false should be set. + if ap, ok := params1["additionalProperties"].(bool); !ok || ap { + t.Error("simple tool: expected additionalProperties=false") + } +} + func TestCleanSchema_DeepNesting(t *testing.T) { params := map[string]any{ "type": "object", diff --git a/internal/providers/schema_normalize.go b/internal/providers/schema_normalize.go index 63cc4d62..4b8f6a8e 100644 --- a/internal/providers/schema_normalize.go +++ b/internal/providers/schema_normalize.go @@ -11,10 +11,15 @@ const maxSchemaDepth = 64 // This is the single entry point — all providers should call this (directly or // via CleanToolSchemas / CleanSchemaForProvider wrappers). func NormalizeSchema(providerName string, schema map[string]any) map[string]any { + return normalizeWithProfile(profileForProvider(providerName), schema) +} + +// normalizeWithProfile applies normalization using a pre-resolved profile. +// Used by CleanToolSchemas to pass per-tool profile overrides. +func normalizeWithProfile(profile SchemaProfile, schema map[string]any) map[string]any { if schema == nil { return nil } - profile := profileForProvider(providerName) result := copySchema(schema) if profile.ResolveRefs { diff --git a/internal/providers/schema_strict.go b/internal/providers/schema_strict.go index 9e6de02a..9525c8d0 100644 --- a/internal/providers/schema_strict.go +++ b/internal/providers/schema_strict.go @@ -72,6 +72,33 @@ func applyStrictMode(schema map[string]any, depth int) map[string]any { return schema } +// IsMultiActionSchema detects tool schemas using the multi-action pattern: +// a top-level "action" property with an "enum" list of 2+ values. +// These tools have many optional params shared across actions; +// strict mode forces models to send all params for every call. +func IsMultiActionSchema(schema map[string]any) bool { + props, ok := schema["properties"].(map[string]any) + if !ok { + return false + } + actionProp, ok := props["action"].(map[string]any) + if !ok { + return false + } + enumVal, ok := actionProp["enum"] + if !ok { + return false + } + // Only exempt if 2+ actions — single-action tools benefit from strict. + switch e := enumVal.(type) { + case []string: + return len(e) >= 2 + case []any: + return len(e) >= 2 + } + return false +} + // makeNullable converts a property schema to accept null values. // - Simple type: "string" → ["string", "null"] // - Type array: ["string", "integer"] → ["string", "integer", "null"] diff --git a/internal/tools/team_tasks_tool.go b/internal/tools/team_tasks_tool.go index 0f205890..4ddd4203 100644 --- a/internal/tools/team_tasks_tool.go +++ b/internal/tools/team_tasks_tool.go @@ -81,9 +81,13 @@ func (t *TeamTasksTool) Parameters() map[string]any { "type": "integer", "description": "Progress percentage 0-100 (for progress action)", }, - "file_id": map[string]any{ + "path": map[string]any{ "type": "string", - "description": "Workspace file ID (for attach)", + "description": "Workspace file path (for attach)", + }, + "task_type": map[string]any{ + "type": "string", + "description": "Task type for create: 'general' (default), 'request', or 'note'", }, "assignee": map[string]any{ "type": "string", @@ -99,13 +103,40 @@ func (t *TeamTasksTool) Parameters() map[string]any { } // buildActionDescription returns the action parameter description based on policy. +// Includes per-action param guide so models know which params to send. func (t *TeamTasksTool) buildActionDescription() string { base := "Available actions: " + strings.Join(t.policy.AllowedActions(), ", ") + "." if t.policy.IsAllowed("ask_user") { - base += " ask_user: set a periodic reminder sent to the USER when you need input. clear_ask_user: cancel a previously set reminder." + base += " ask_user: set a periodic reminder. clear_ask_user: cancel reminder." } if t.policy.IsAllowed("retry") { - base += " retry: re-dispatch a stale or failed task." + base += " retry: re-dispatch a stale/failed task." + } + // Per-action param guide — only list actions allowed by policy. + base += "\n\nParams per action (only send listed params):\n" + guide := map[string]string{ + "list": "- list: status?, page?\n", + "get": "- get: task_id\n", + "create": "- create: subject, description, assignee, priority?, blocked_by?, require_approval?, task_type?\n", + "claim": "- claim: task_id\n", + "complete": "- complete: task_id?, result\n", + "cancel": "- cancel: task_id, text\n", + "search": "- search: query, page?\n", + "review": "- review: task_id\n", + "comment": "- comment: task_id?, text, type?\n", + "progress": "- progress: task_id?, percent, text?\n", + "attach": "- attach: task_id, path\n", + "update": "- update: task_id, subject?, description?, priority?, blocked_by?\n", + "approve": "- approve: task_id\n", + "reject": "- reject: task_id, text\n", + "ask_user": "- ask_user: task_id, text\n", + "clear_ask_user": "- clear_ask_user: task_id\n", + "retry": "- retry: task_id\n", + } + for _, action := range t.policy.AllowedActions() { + if line, ok := guide[action]; ok { + base += line + } } return base }