diff --git a/pkg/vmcp/config/validator.go b/pkg/vmcp/config/validator.go index 242d03814..3c8750bc6 100644 --- a/pkg/vmcp/config/validator.go +++ b/pkg/vmcp/config/validator.go @@ -396,8 +396,9 @@ func (v *DefaultValidator) validateCompositeTools(tools []*CompositeToolConfig) return fmt.Errorf("composite_tools[%d].description is required", i) } - if tool.Timeout <= 0 { - return fmt.Errorf("composite_tools[%d].timeout must be positive", i) + // Timeout can be 0 (uses default) or positive (explicit timeout) + if tool.Timeout < 0 { + return fmt.Errorf("composite_tools[%d].timeout cannot be negative", i) } // Validate steps @@ -451,8 +452,20 @@ func (*DefaultValidator) validateStepBasics(step *WorkflowStepConfig, index int, return nil } -// validateStepType validates step type and type-specific requirements +// validateStepType validates step type and type-specific requirements. +// The type should have been inferred during loading if the 'tool' field is present. +// Elicitation steps must always specify type explicitly for clarity. func (*DefaultValidator) validateStepType(step *WorkflowStepConfig, index int) error { + // Check for ambiguous configuration: both tool and message fields present + if step.Tool != "" && step.Message != "" { + return fmt.Errorf("step[%d] cannot have both tool and message fields - use explicit type to clarify intent", index) + } + + // Type is required at this point (should have been inferred during loading) + if step.Type == "" { + return fmt.Errorf("step[%d].type is required (or omit for tool steps with 'tool' field present)", index) + } + validTypes := []string{"tool", "elicitation"} if !contains(validTypes, step.Type) { return fmt.Errorf("step[%d].type must be one of: %s", index, strings.Join(validTypes, ", ")) diff --git a/pkg/vmcp/config/validator_test.go b/pkg/vmcp/config/validator_test.go index 789b730f7..898e73ed8 100644 --- a/pkg/vmcp/config/validator_test.go +++ b/pkg/vmcp/config/validator_test.go @@ -553,6 +553,104 @@ func TestValidator_ValidateCompositeTools(t *testing.T) { wantErr: true, errMsg: "duplicate composite tool name", }, + { + name: "type inferred from tool field", + tools: []*CompositeToolConfig{ + { + Name: "fetch_data", + Description: "Fetch data workflow", + Timeout: Duration(5 * time.Minute), + Steps: []*WorkflowStepConfig{ + { + ID: "fetch", + Type: "tool", // Type would be inferred by loader from tool field + Tool: "fetch_fetch", + Arguments: map[string]any{ + "url": "https://example.com", + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "timeout omitted uses default", + tools: []*CompositeToolConfig{ + { + Name: "no_timeout", + Description: "Workflow without explicit timeout", + Timeout: 0, // Omitted - should use default (30 minutes) + Steps: []*WorkflowStepConfig{ + { + ID: "step1", + Type: "tool", // Type would be inferred by loader from tool field + Tool: "some_tool", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "elicitation requires explicit type", + tools: []*CompositeToolConfig{ + { + Name: "confirm_action", + Description: "Confirmation workflow", + Timeout: Duration(5 * time.Minute), + Steps: []*WorkflowStepConfig{ + { + ID: "confirm", + Message: "Proceed?", // Elicitation field present + Schema: map[string]any{"type": "object"}, + // Type is missing - should fail + }, + }, + }, + }, + wantErr: true, + errMsg: "type is required", + }, + { + name: "missing both type and identifying fields", + tools: []*CompositeToolConfig{ + { + Name: "invalid_step", + Description: "Invalid step workflow", + Timeout: Duration(5 * time.Minute), + Steps: []*WorkflowStepConfig{ + { + ID: "step1", + // No type, no tool, no message - cannot infer + }, + }, + }, + }, + wantErr: true, + errMsg: "type is required", + }, + { + name: "both tool and message fields present", + tools: []*CompositeToolConfig{ + { + Name: "ambiguous_step", + Description: "Step with both tool and message", + Timeout: Duration(5 * time.Minute), + Steps: []*WorkflowStepConfig{ + { + ID: "step1", + Tool: "some_tool", // Tool field present + Message: "Some message", // Message field also present + // Type will be inferred as "tool" during loading + // This should fail validation due to ambiguity + }, + }, + }, + }, + wantErr: true, + errMsg: "cannot have both tool and message fields", + }, } for _, tt := range tests { diff --git a/pkg/vmcp/config/yaml_loader.go b/pkg/vmcp/config/yaml_loader.go index 2cc2dff45..7aef22069 100644 --- a/pkg/vmcp/config/yaml_loader.go +++ b/pkg/vmcp/config/yaml_loader.go @@ -495,9 +495,14 @@ func (l *YAMLLoader) transformCompositeTools(raw []*rawCompositeTool) ([]*Compos var tools []*CompositeToolConfig for _, rawTool := range raw { - timeout, err := time.ParseDuration(rawTool.Timeout) - if err != nil { - return nil, fmt.Errorf("tool %s: invalid timeout: %w", rawTool.Name, err) + // Parse timeout - empty string means use default (0 duration) + var timeout time.Duration + if rawTool.Timeout != "" { + var err error + timeout, err = time.ParseDuration(rawTool.Timeout) + if err != nil { + return nil, fmt.Errorf("tool %s: invalid timeout: %w", rawTool.Name, err) + } } tool := &CompositeToolConfig{ @@ -553,6 +558,11 @@ func (*YAMLLoader) transformWorkflowStep(raw *rawWorkflowStep) (*WorkflowStepCon Schema: raw.Schema, } + // Apply type inference: if type is empty and tool field is present, infer as "tool" + if step.Type == "" && step.Tool != "" { + step.Type = "tool" + } + if raw.Timeout != "" { timeout, err := time.ParseDuration(raw.Timeout) if err != nil {