Wire up dead MCP config schema validation

pkg/parser/schema_test.go

@@ -136,7 +136,133 @@ timeout_minu tes: 10
 	}
 }
 
-// TestGetSafeOutputTypeKeys tests extracting safe output type keys from the embedded schema
+// TestValidateMCPConfigWithSchema tests the ValidateMCPConfigWithSchema function
+// which validates a single MCP server configuration against the MCP config JSON schema.
+func TestValidateMCPConfigWithSchema(t *testing.T) {
+	tests := []struct {
+		name        string
+		mcpConfig   map[string]any
+		wantErr     bool
+		errContains string
+	}{
+		{
+			name: "valid stdio config with command",
+			mcpConfig: map[string]any{
+				"type":    "stdio",
+				"command": "npx",
+				"args":    []any{"-y", "@modelcontextprotocol/server-filesystem"},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid stdio config with container",
+			mcpConfig: map[string]any{
+				"type":      "stdio",
+				"container": "docker.io/mcp/brave-search",
+				"env": map[string]any{
+					"BRAVE_API_KEY": "secret",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid http config",
+			mcpConfig: map[string]any{
+				"type": "http",
+				"url":  "https://api.example.com/mcp",
+				"headers": map[string]any{
+					"Authorization": "Bearer token",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid config inferred from url requires explicit type in schema",
+			mcpConfig: map[string]any{
+				"type": "http",
+				"url":  "http://localhost:8765",
+			},
+			wantErr: false,
+		},
+		{
+			name:        "empty config fails anyOf - missing type, url, command, and container",
+			mcpConfig:   map[string]any{},
+			wantErr:     true,
+			errContains: "missing property",
+		},
+		{
+			name: "invalid container pattern rejected by schema",
+			mcpConfig: map[string]any{
+				"container": "INVALID CONTAINER NAME WITH SPACES",
+			},
+			wantErr:     true,
+			errContains: "jsonschema validation failed",
+		},
+		{
+			name: "invalid env key pattern rejected by schema",
+			mcpConfig: map[string]any{
+				"type":    "stdio",
+				"command": "node",
+				"env": map[string]any{
+					"lowercase-key": "value",
+				},
+			},
+			wantErr:     true,
+			errContains: "jsonschema validation failed",
+		},
+		{
+			name: "invalid mounts item pattern rejected by schema",
+			mcpConfig: map[string]any{
+				"type":      "stdio",
+				"container": "mcp/server",
+				"mounts":    []any{"invalid-mount-format"},
+			},
+			wantErr:     true,
+			errContains: "jsonschema validation failed",
+		},
+		{
+			name: "additional property rejected by schema",
+			mcpConfig: map[string]any{
+				"type":          "stdio",
+				"command":       "node",
+				"unknown-field": "value",
+			},
+			wantErr:     true,
+			errContains: "jsonschema validation failed",
+		},
+		{
+			name: "valid local type (alias for stdio)",
+			mcpConfig: map[string]any{
+				"type":    "local",
+				"command": "node",
+			},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateMCPConfigWithSchema(tt.mcpConfig)
+
+			if tt.wantErr && err == nil {
+				t.Errorf("ValidateMCPConfigWithSchema() expected error, got nil")
+				return
+			}
+
+			if !tt.wantErr && err != nil {
+				t.Errorf("ValidateMCPConfigWithSchema() unexpected error = %v", err)
+				return
+			}
+
+			if tt.wantErr && err != nil && tt.errContains != "" {
+				if !strings.Contains(err.Error(), tt.errContains) {
+					t.Errorf("ValidateMCPConfigWithSchema() error = %v, expected to contain %v", err, tt.errContains)
+				}
+			}
+		})
+	}
+}
+
 func TestGetSafeOutputTypeKeys(t *testing.T) {
 	keys, err := GetSafeOutputTypeKeys()
 	if err != nil {

pkg/parser/schema_validation.go

@@ -99,4 +99,12 @@ func ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatter map[string
 	return validateEngineSpecificRules(filtered)
 }
 
-// ValidateMCPConfigWithSchema validates MCP configuration using JSON schema
+// ValidateMCPConfigWithSchema validates MCP configuration using JSON schema.
+// The caller is responsible for passing only the fields defined in the MCP
+// config schema; additional tool-specific fields (e.g. auth, proxy-args)
+// must be stripped before calling this function because the schema uses
+// additionalProperties: false.
+func ValidateMCPConfigWithSchema(mcpConfig map[string]any) error {
+	schemaValidationLog.Printf("Validating MCP configuration against JSON schema: %d fields", len(mcpConfig))
+	return validateWithSchema(mcpConfig, mcpConfigSchema, "MCP configuration")
+}

pkg/workflow/mcp_config_validation.go

@@ -127,10 +127,21 @@ func ValidateMCPConfigs(tools map[string]any) error {
 
 		mcpValidationLog.Printf("Validating MCP requirements for tool: %s", toolName)
 
-		// Validate MCP configuration requirements (before transformation)
+		// Validate MCP configuration requirements first (before transformation).
+		// Custom validation runs before schema validation to provide better error messages
+		// for the most common mistakes (matching the pattern in ValidateMainWorkflowFrontmatterWithSchemaAndLocation).
 		if err := validateMCPRequirements(toolName, mcpConfig, config); err != nil {
 			return err
 		}
+
+		// Run JSON schema validation as a catch-all after custom validation. Build a
+		// schema-compatible view of the config by extracting only the properties defined
+		// in mcp_config_schema.json. Tool-specific fields (e.g. auth, proxy-args) are
+		// excluded because the schema uses additionalProperties: false.
+		if err := parser.ValidateMCPConfigWithSchema(buildSchemaMCPConfig(config)); err != nil {
+			mcpValidationLog.Printf("JSON schema validation failed for tool %s: %v", toolName, err)
+			return fmt.Errorf("tool '%s' has invalid MCP configuration: %w", toolName, err)
+		}
 	}
 
 	mcpValidationLog.Print("MCP configuration validation completed successfully")
@@ -224,6 +235,23 @@ func getRawMCPConfig(toolConfig map[string]any) (map[string]any, error) {
 	return result, nil
 }
 
+// inferMCPType infers the MCP connection type from the fields present in a config map.
+// Returns "http" when a url field is present, "stdio" when command or container is present,
+// and an empty string when the type cannot be determined. It does not validate the explicit
+// 'type' field — that is done by the caller.
+func inferMCPType(config map[string]any) string {
+	if _, hasURL := config["url"]; hasURL {
+		return "http"
+	}
+	if _, hasCommand := config["command"]; hasCommand {
+		return "stdio"
+	}
+	if _, hasContainer := config["container"]; hasContainer {
+		return "stdio"
+	}
+	return ""
+}
+
 // validateStringProperty validates that a property is a string and returns appropriate error message
 func validateStringProperty(toolName, propertyName string, value any, exists bool) error {
 	if !exists {
@@ -249,13 +277,8 @@ func validateMCPRequirements(toolName string, mcpConfig map[string]any, toolConf
 		typeStr = mcpType.(string)
 	} else {
 		// Infer type from presence of fields
-		if _, hasURL := mcpConfig["url"]; hasURL {
-			typeStr = "http"
-		} else if _, hasCommand := mcpConfig["command"]; hasCommand {
-			typeStr = "stdio"
-		} else if _, hasContainer := mcpConfig["container"]; hasContainer {
-			typeStr = "stdio"
-		} else {
+		typeStr = inferMCPType(mcpConfig)
+		if typeStr == "" {
 			return fmt.Errorf("tool '%s' unable to determine MCP type: missing type, url, command, or container.\n\nExample:\ntools:\n  %s:\n    command: \"node server.js\"\n    args: [\"--port\", \"3000\"]\n\nSee: %s", toolName, toolName, constants.DocsToolsURL)
 		}
 	}
@@ -344,6 +367,68 @@ func validateMCPRequirements(toolName string, mcpConfig map[string]any, toolConf
 	return nil
 }
 
+// mcpSchemaTopLevelFields is the set of properties defined at the top level of
+// mcp_config_schema.json. Only these fields should be passed to
+// parser.ValidateMCPConfigWithSchema; the schema uses additionalProperties: false
+// so any extra field would cause a spurious validation failure.
+//
+// WARNING: This map must be kept in sync with the properties defined in
+// pkg/parser/schemas/mcp_config_schema.json. If you add or remove a property
+// from that schema, update this map accordingly.
+var mcpSchemaTopLevelFields = map[string]bool{
+	"type":           true,
+	"registry":       true,
+	"url":            true,
+	"command":        true,
+	"container":      true,
+	"args":           true,
+	"entrypoint":     true,
+	"entrypointArgs": true,
+	"mounts":         true,
+	"env":            true,
+	"headers":        true,
+	"network":        true,
+	"allowed":        true,
+	"version":        true,
+}
+
+// buildSchemaMCPConfig extracts only the fields defined in mcp_config_schema.json
+// from a full tool config map. Tool-specific fields that are not part of the MCP
+// schema (e.g. auth, proxy-args, mode, github-token) are excluded so that schema
+// validation does not fail on fields unknown to the schema.
+//
+// If the 'type' field is absent but can be inferred from other fields (url → http,
+// command/container → stdio), the inferred type is injected. This is necessary because
+// the schema's if/then conditions use properties-based matching which is vacuously true
+// when 'type' is absent, causing contradictory constraints to fire for valid configs
+// that rely on type inference.
+func buildSchemaMCPConfig(toolConfig map[string]any) map[string]any {
+	result := make(map[string]any, len(mcpSchemaTopLevelFields))
+	for field := range mcpSchemaTopLevelFields {
+		if value, exists := toolConfig[field]; exists {
+			result[field] = value
+		}
+	}
+	// If 'type' is not present, infer it from other fields so the schema's
+	// if/then conditions do not fire vacuously and reject valid inferred-type configs.
+	//
+	// Why this is necessary: the JSON Schema draft-07 `properties` keyword is
+	// vacuously satisfied when the checked property is absent. This means the
+	// `if {"properties": {"type": {"enum": ["stdio"]}}}` condition evaluates to
+	// true even when 'type' is not in the config, causing the stdio `then` clause
+	// (requiring command/container) to apply unexpectedly for HTTP-only configs.
+	// Injecting the inferred type before schema validation ensures the correct
+	// if/then branch fires. When inference is not possible (empty string returned),
+	// the map is left without a 'type'; the schema's anyOf constraint will then
+	// report a clear "missing required property" error on its own.
+	if _, hasType := result["type"]; !hasType {
+		if inferred := inferMCPType(result); inferred != "" {
+			result["type"] = inferred
+		}
+	}
+	return result
+}
+
 // validateMCPMountsSyntax validates that mount strings in a custom MCP server config
 // follow the correct syntax required by MCP Gateway v0.1.5+.
 // Expected format: "source:destination:mode" where mode is either "ro" or "rw".