Improve composite tool configuration ergonomics (#2589)

* Improve composite tool configuration ergonomics

Makes composite tool workflow configuration more user-friendly by
reducing verbosity and following "convention over configuration":

1. **Step type inference**: When 'tool' field is present, 'type' is
   automatically inferred as "tool". Elicitation steps still require
   explicit type for clarity.

2. **Optional timeout**: Timeout can be omitted and defaults to 30
   minutes at runtime. Validator now allows 0 (use default) instead
   of requiring explicit positive value.

Before:
```yaml
composite_tools:
  - name: "fetch_data"
    timeout: "30m"  # Required even for default
    steps:
      - id: "fetch"
        type: "tool"  # Redundant when 'tool' field present
        tool: "fetch_fetch"
```

After:
```yaml
composite_tools:
  - name: "fetch_data"
    # timeout omitted - uses 30m default
    steps:
      - id: "fetch"
        tool: "fetch_fetch"  # Type inferred as "tool"
```

Changes:
- Infer type="tool" when tool field is present
- Allow timeout=0 in validator (engine applies default)
- Handle empty timeout string in YAML loader
- Add comprehensive tests for both features

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

* Move type inference and defaults to YAMLLoader

This refactoring addresses the architectural concern that validators should
be read-only and not mutate configuration values. The changes follow the
'convention over configuration' principle while maintaining clean separation
of concerns.

Changes:
- Move step type inference from validator to yaml_loader transformWorkflowStep()
- Update validator to be read-only (no longer mutates step.Type)
- Add validation to prevent ambiguous configurations (both tool and message fields)
- Add test case for the ambiguous scenario
- Update existing tests to reflect loader-based type inference

Benefits:
- Clear separation: Loader applies defaults, Validator validates invariants
- Follows existing patterns (elicitation timeout defaults in loader)
- Platform consistency (CLI and K8s produce same Config before validation)
- Validator becomes pure validation logic (no side effects)

Co-authored-by: Juan Antonio Osorio <[email protected]>

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Juan Antonio Osorio <[email protected]>

Author: 3 people

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

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 {

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 {