changes from review

Author: taskbot

cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_types.go

@@ -53,7 +53,6 @@ type VirtualMCPCompositeToolDefinitionStatus struct {
 	// ValidationStatus indicates the validation state of the workflow
 	// - Valid: Workflow structure is valid
 	// - Invalid: Workflow has validation errors
-	// +kubebuilder:validation:Enum=Valid;Invalid;Unknown
 	// +optional
 	ValidationStatus ValidationStatus `json:"validationStatus,omitempty"`
 

cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook.go

@@ -2,10 +2,13 @@ package v1alpha1
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"regexp"
+	"strings"
 	"text/template"
 
+	"github.com/santhosh-tekuri/jsonschema/v6"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -100,22 +103,29 @@ func (r *VirtualMCPCompositeToolDefinition) Validate() error {
 
 // validateParameters validates the parameter schema
 func (r *VirtualMCPCompositeToolDefinition) validateParameters() error {
+	// Valid JSON Schema primitive types
+	// Reference: https://json-schema.org/understanding-json-schema/reference/type.html
+	validTypes := map[string]bool{
+		"string":  true,
+		"integer": true,
+		"number":  true,
+		"boolean": true,
+		"array":   true,
+		"object":  true,
+		"null":    true, // null is a valid JSON Schema type
+	}
+
 	for paramName, param := range r.Spec.Parameters {
 		if param.Type == "" {
 			return fmt.Errorf("spec.parameters[%s].type is required", paramName)
 		}
 
-		// Validate parameter type
-		validTypes := map[string]bool{
-			"string":  true,
-			"integer": true,
-			"number":  true,
-			"boolean": true,
-			"array":   true,
-			"object":  true,
-		}
+		// Validate that the type is a valid JSON Schema type
 		if !validTypes[param.Type] {
-			return fmt.Errorf("spec.parameters[%s].type must be one of: string, integer, number, boolean, array, object", paramName)
+			return fmt.Errorf(
+				"spec.parameters[%s].type must be a valid JSON Schema type (string, integer, number, boolean, array, object, null), got: %s",
+				paramName, param.Type,
+			)
 		}
 	}
 
@@ -232,6 +242,13 @@ func (*VirtualMCPCompositeToolDefinition) validateStepTemplates(index int, step
 		}
 	}
 
+	// Validate JSON Schema for elicitation steps
+	if step.Schema != nil {
+		if err := validateJSONSchema(step.Schema.Raw); err != nil {
+			return fmt.Errorf("spec.steps[%d].schema: invalid JSON Schema: %v", index, err)
+		}
+	}
+
 	return nil
 }
 
@@ -321,6 +338,69 @@ func validateTemplate(tmpl string) error {
 	return nil
 }
 
+// validateJSONSchema validates that the provided bytes contain a valid JSON Schema
+func validateJSONSchema(schemaBytes []byte) error {
+	if len(schemaBytes) == 0 {
+		return nil // Empty schema is allowed
+	}
+
+	// Parse the schema JSON
+	var schemaDoc interface{}
+	if err := json.Unmarshal(schemaBytes, &schemaDoc); err != nil {
+		return fmt.Errorf("failed to parse JSON: %v", err)
+	}
+
+	// Compile the schema to validate it's a valid JSON Schema
+	compiler := jsonschema.NewCompiler()
+	schemaID := "schema://validation"
+	if err := compiler.AddResource(schemaID, schemaDoc); err != nil {
+		return formatJSONSchemaError(err)
+	}
+
+	if _, err := compiler.Compile(schemaID); err != nil {
+		return formatJSONSchemaError(err)
+	}
+
+	return nil
+}
+
+// formatJSONSchemaError formats JSON Schema validation errors for better readability
+func formatJSONSchemaError(err error) error {
+	if validationErr, ok := err.(*jsonschema.ValidationError); ok {
+		var errorMessages []string
+		collectJSONSchemaErrors(validationErr, &errorMessages)
+		if len(errorMessages) > 0 {
+			return fmt.Errorf("%s", strings.Join(errorMessages, "; "))
+		}
+	}
+	return err
+}
+
+// collectJSONSchemaErrors recursively collects all validation error messages
+func collectJSONSchemaErrors(err *jsonschema.ValidationError, messages *[]string) {
+	if err == nil {
+		return
+	}
+
+	// If this error has causes, recurse into them
+	if len(err.Causes) > 0 {
+		for _, cause := range err.Causes {
+			collectJSONSchemaErrors(cause, messages)
+		}
+		return
+	}
+
+	// This is a leaf error - format it
+	output := err.BasicOutput()
+	if output != nil && output.Error != nil {
+		errorMsg := output.Error.String()
+		if output.InstanceLocation != "" {
+			errorMsg = fmt.Sprintf("%s at '%s'", errorMsg, output.InstanceLocation)
+		}
+		*messages = append(*messages, errorMsg)
+	}
+}
+
 // validateDuration validates duration format (e.g., "30s", "5m", "1h")
 func validateDuration(duration string) error {
 	// Pattern: one or more segments of number + unit (ms, s, m, h)

cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_webhook_test.go

@@ -151,7 +151,7 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) {
 				},
 			},
 			wantErr: true,
-			errMsg:  "spec.parameters[environment].type must be one of: string, integer, number, boolean, array, object",
+			errMsg:  "spec.parameters[environment].type must be a valid JSON Schema type",
 		},
 		{
 			name: "missing step ID",
@@ -242,6 +242,44 @@ func TestVirtualMCPCompositeToolDefinitionValidate(t *testing.T) {
 			wantErr: true,
 			errMsg:  "spec.steps[0].message is required when type is elicitation",
 		},
+		{
+			name: "invalid JSON Schema in elicitation step",
+			ctd: &VirtualMCPCompositeToolDefinition{
+				Spec: VirtualMCPCompositeToolDefinitionSpec{
+					Name:        "interactive_deploy",
+					Description: "Deploy with user confirmation",
+					Steps: []WorkflowStep{
+						{
+							ID:      "confirm",
+							Type:    WorkflowStepTypeElicitation,
+							Message: "Confirm deployment?",
+							Schema:  &runtime.RawExtension{Raw: []byte(`{"type": "invalid_json_schema_type"}`)},
+						},
+					},
+				},
+			},
+			wantErr: true,
+			errMsg:  "spec.steps[0].schema: invalid JSON Schema",
+		},
+		{
+			name: "malformed JSON in elicitation step schema",
+			ctd: &VirtualMCPCompositeToolDefinition{
+				Spec: VirtualMCPCompositeToolDefinitionSpec{
+					Name:        "interactive_deploy",
+					Description: "Deploy with user confirmation",
+					Steps: []WorkflowStep{
+						{
+							ID:      "confirm",
+							Type:    WorkflowStepTypeElicitation,
+							Message: "Confirm deployment?",
+							Schema:  &runtime.RawExtension{Raw: []byte(`{invalid json}`)},
+						},
+					},
+				},
+			},
+			wantErr: true,
+			errMsg:  "spec.steps[0].schema: invalid JSON Schema",
+		},
 		{
 			name: "valid workflow with dependencies",
 			ctd: &VirtualMCPCompositeToolDefinition{

deploy/charts/operator-crds/crds/toolhive.stacklok.dev_virtualmcpcompositetooldefinitions.yaml

@@ -288,19 +288,14 @@ spec:
                   type: string
                 type: array
               validationStatus:
-                allOf:
-                - enum:
-                  - Valid
-                  - Invalid
-                  - Unknown
-                - enum:
-                  - Valid
-                  - Invalid
-                  - Unknown
                 description: |-
                   ValidationStatus indicates the validation state of the workflow
                   - Valid: Workflow structure is valid
                   - Invalid: Workflow has validation errors
+                enum:
+                - Valid
+                - Invalid
+                - Unknown
                 type: string
             type: object
         type: object

docs/operator/virtualmcpcompositetooldefinition-guide.md

@@ -150,14 +150,17 @@ Define execution order using `dependsOn`:
 spec:
   steps:
     - id: step1
+      type: tool_call
       tool: workload.tool_a
 
     - id: step2
+      type: tool_call
       tool: workload.tool_b
       dependsOn:
         - step1
 
     - id: step3
+      type: tool_call
       tool: workload.tool_c
       dependsOn:
         - step1