From ef6c5cdf26c3044af650aa0abb6886ceac6f1530 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 14 Nov 2025 08:33:28 -0600 Subject: [PATCH 1/2] Improve composite tool configuration ergonomics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/vmcp/config/validator.go | 19 ++++++-- pkg/vmcp/config/validator_test.go | 75 +++++++++++++++++++++++++++++++ pkg/vmcp/config/yaml_loader.go | 11 +++-- 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/pkg/vmcp/config/validator.go b/pkg/vmcp/config/validator.go index 242d03814..54e6339ae 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. +// Type inference: If type is not specified and 'tool' field is present, infers type as "tool". +// Elicitation steps must always specify type explicitly for clarity. func (*DefaultValidator) validateStepType(step *WorkflowStepConfig, index int) error { + // Infer type as "tool" if tool field is present and type is unspecified + if step.Type == "" && step.Tool != "" { + step.Type = "tool" + } + + // Type is required if it cannot be inferred + 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..931c474ad 100644 --- a/pkg/vmcp/config/validator_test.go +++ b/pkg/vmcp/config/validator_test.go @@ -553,6 +553,81 @@ 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", + Tool: "fetch_fetch", // Type omitted - should be inferred as "tool" + 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", + 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", + }, } for _, tt := range tests { diff --git a/pkg/vmcp/config/yaml_loader.go b/pkg/vmcp/config/yaml_loader.go index 2cc2dff45..1d8501e8c 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{ From e05c7417d9cf3822e1d425660ddf08be29e8913e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Fri, 14 Nov 2025 16:30:39 +0000 Subject: [PATCH 2/2] 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 --- pkg/vmcp/config/validator.go | 10 +++++----- pkg/vmcp/config/validator_test.go | 25 ++++++++++++++++++++++++- pkg/vmcp/config/yaml_loader.go | 5 +++++ 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/pkg/vmcp/config/validator.go b/pkg/vmcp/config/validator.go index 54e6339ae..3c8750bc6 100644 --- a/pkg/vmcp/config/validator.go +++ b/pkg/vmcp/config/validator.go @@ -453,15 +453,15 @@ func (*DefaultValidator) validateStepBasics(step *WorkflowStepConfig, index int, } // validateStepType validates step type and type-specific requirements. -// Type inference: If type is not specified and 'tool' field is present, infers type as "tool". +// 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 { - // Infer type as "tool" if tool field is present and type is unspecified - if step.Type == "" && step.Tool != "" { - step.Type = "tool" + // 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 if it cannot be inferred + // 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) } diff --git a/pkg/vmcp/config/validator_test.go b/pkg/vmcp/config/validator_test.go index 931c474ad..898e73ed8 100644 --- a/pkg/vmcp/config/validator_test.go +++ b/pkg/vmcp/config/validator_test.go @@ -563,7 +563,8 @@ func TestValidator_ValidateCompositeTools(t *testing.T) { Steps: []*WorkflowStepConfig{ { ID: "fetch", - Tool: "fetch_fetch", // Type omitted - should be inferred as "tool" + Type: "tool", // Type would be inferred by loader from tool field + Tool: "fetch_fetch", Arguments: map[string]any{ "url": "https://example.com", }, @@ -583,6 +584,7 @@ func TestValidator_ValidateCompositeTools(t *testing.T) { Steps: []*WorkflowStepConfig{ { ID: "step1", + Type: "tool", // Type would be inferred by loader from tool field Tool: "some_tool", }, }, @@ -628,6 +630,27 @@ func TestValidator_ValidateCompositeTools(t *testing.T) { 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 1d8501e8c..7aef22069 100644 --- a/pkg/vmcp/config/yaml_loader.go +++ b/pkg/vmcp/config/yaml_loader.go @@ -558,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 {