diff --git a/README.md b/README.md index c1495036..39e149ee 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,10 @@ This gateway is used with [GitHub Agentic Workflows](https://github.com/githubne - **Spec-Compliant Validation**: Fail-fast validation with detailed error messages - **Variable Expansion**: Environment variable substitution with `${VAR_NAME}` syntax - **Type Normalization**: Automatic conversion of legacy `"local"` type to `"stdio"` +- **Schema Normalization**: Automatic fixing of malformed JSON schemas from backend MCP servers + - Adds missing `properties` field to object schemas + - Prevents downstream validation errors + - Transparent to both backends and clients - **Routing Modes**: - **Routed**: Each backend server accessible at `/mcp/{serverID}` - **Unified**: Single endpoint `/mcp` that routes to configured servers diff --git a/internal/mcp/schema_normalizer.go b/internal/mcp/schema_normalizer.go new file mode 100644 index 00000000..be72440a --- /dev/null +++ b/internal/mcp/schema_normalizer.go @@ -0,0 +1,50 @@ +package mcp + +import ( + "github.com/githubnext/gh-aw-mcpg/internal/logger" +) + +// NormalizeInputSchema fixes common schema validation issues in tool definitions +// that can cause downstream validation errors. +// +// Known issues fixed: +// 1. Object schemas without properties: When a schema declares "type": "object" +// but is missing the required "properties" field, we add an empty properties +// object to make it valid per JSON Schema standards. +func NormalizeInputSchema(schema map[string]interface{}, toolName string) map[string]interface{} { + if schema == nil { + return schema + } + + // Check if this is an object type schema + typeVal, hasType := schema["type"] + if !hasType { + return schema + } + + typeStr, isString := typeVal.(string) + if !isString || typeStr != "object" { + return schema + } + + // Check if properties field exists + _, hasProperties := schema["properties"] + _, hasAdditionalProperties := schema["additionalProperties"] + + // If it's an object type but missing both properties and additionalProperties, + // add an empty properties object to make it valid + if !hasProperties && !hasAdditionalProperties { + logger.LogWarn("backend", "Tool schema normalized: %s - added empty properties to object type schema", toolName) + + // Create a copy of the schema to avoid modifying the original + normalized := make(map[string]interface{}) + for k, v := range schema { + normalized[k] = v + } + normalized["properties"] = map[string]interface{}{} + + return normalized + } + + return schema +} diff --git a/internal/mcp/schema_normalizer_test.go b/internal/mcp/schema_normalizer_test.go new file mode 100644 index 00000000..434aee09 --- /dev/null +++ b/internal/mcp/schema_normalizer_test.go @@ -0,0 +1,195 @@ +package mcp + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNormalizeInputSchema_NilSchema(t *testing.T) { + result := NormalizeInputSchema(nil, "test-tool") + assert.Nil(t, result, "Nil schema should return nil") +} + +func TestNormalizeInputSchema_EmptySchema(t *testing.T) { + schema := map[string]interface{}{} + result := NormalizeInputSchema(schema, "test-tool") + assert.Equal(t, schema, result, "Empty schema should be unchanged") +} + +func TestNormalizeInputSchema_NonObjectType(t *testing.T) { + testCases := []struct { + name string + schema map[string]interface{} + }{ + { + name: "string type", + schema: map[string]interface{}{ + "type": "string", + }, + }, + { + name: "number type", + schema: map[string]interface{}{ + "type": "number", + }, + }, + { + name: "array type", + schema: map[string]interface{}{ + "type": "array", + "items": map[string]interface{}{ + "type": "string", + }, + }, + }, + { + name: "boolean type", + schema: map[string]interface{}{ + "type": "boolean", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := NormalizeInputSchema(tc.schema, "test-tool") + assert.Equal(t, tc.schema, result, "Non-object schema should be unchanged") + }) + } +} + +func TestNormalizeInputSchema_ObjectWithProperties(t *testing.T) { + schema := map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "name": map[string]interface{}{ + "type": "string", + }, + "age": map[string]interface{}{ + "type": "number", + }, + }, + "required": []interface{}{"name"}, + } + + result := NormalizeInputSchema(schema, "test-tool") + assert.Equal(t, schema, result, "Object schema with properties should be unchanged") +} + +func TestNormalizeInputSchema_ObjectWithAdditionalProperties(t *testing.T) { + schema := map[string]interface{}{ + "type": "object", + "additionalProperties": true, + } + + result := NormalizeInputSchema(schema, "test-tool") + assert.Equal(t, schema, result, "Object schema with additionalProperties should be unchanged") +} + +func TestNormalizeInputSchema_ObjectWithBothPropertiesAndAdditionalProperties(t *testing.T) { + schema := map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "name": map[string]interface{}{ + "type": "string", + }, + }, + "additionalProperties": false, + } + + result := NormalizeInputSchema(schema, "test-tool") + assert.Equal(t, schema, result, "Object schema with both properties and additionalProperties should be unchanged") +} + +func TestNormalizeInputSchema_ObjectWithoutPropertiesBroken(t *testing.T) { + // This is the broken schema case from GitHub MCP Server + schema := map[string]interface{}{ + "type": "object", + } + + result := NormalizeInputSchema(schema, "github-get_commit") + + require.NotNil(t, result, "Result should not be nil") + assert.Equal(t, "object", result["type"], "Type should remain object") + + // Check that properties was added + properties, hasProperties := result["properties"] + require.True(t, hasProperties, "Properties field should be added") + + // Check that properties is an empty map + propertiesMap, isMap := properties.(map[string]interface{}) + require.True(t, isMap, "Properties should be a map") + assert.Empty(t, propertiesMap, "Properties should be an empty map") +} + +func TestNormalizeInputSchema_ObjectWithoutPropertiesDoesNotModifyOriginal(t *testing.T) { + // Verify that the original schema is not modified + schema := map[string]interface{}{ + "type": "object", + } + + result := NormalizeInputSchema(schema, "test-tool") + + // Original should not have properties + _, originalHasProperties := schema["properties"] + assert.False(t, originalHasProperties, "Original schema should not be modified") + + // Result should have properties + _, resultHasProperties := result["properties"] + assert.True(t, resultHasProperties, "Result should have properties field") +} + +func TestNormalizeInputSchema_ComplexObjectWithoutProperties(t *testing.T) { + // Object schema with other fields but missing properties + schema := map[string]interface{}{ + "type": "object", + "title": "Complex Schema", + "description": "A complex schema without properties", + "examples": []interface{}{ + map[string]interface{}{ + "foo": "bar", + }, + }, + } + + result := NormalizeInputSchema(schema, "complex-tool") + + require.NotNil(t, result, "Result should not be nil") + + // Check all original fields are preserved + assert.Equal(t, "object", result["type"]) + assert.Equal(t, "Complex Schema", result["title"]) + assert.Equal(t, "A complex schema without properties", result["description"]) + assert.NotNil(t, result["examples"]) + + // Check that properties was added + properties, hasProperties := result["properties"] + require.True(t, hasProperties, "Properties field should be added") + assert.Empty(t, properties.(map[string]interface{}), "Properties should be empty") +} + +func TestNormalizeInputSchema_NonStringType(t *testing.T) { + // Edge case: type field is not a string + schema := map[string]interface{}{ + "type": 123, // invalid, but should not crash + } + + result := NormalizeInputSchema(schema, "test-tool") + assert.Equal(t, schema, result, "Schema with non-string type should be unchanged") +} + +func TestNormalizeInputSchema_MultipleToolNames(t *testing.T) { + // Verify that different tool names work correctly + schema := map[string]interface{}{ + "type": "object", + } + + tools := []string{"tool-1", "tool-2", "github-get_commit", "issue_read"} + for _, toolName := range tools { + result := NormalizeInputSchema(schema, toolName) + _, hasProperties := result["properties"] + assert.True(t, hasProperties, "Properties should be added for tool: "+toolName) + } +} diff --git a/internal/server/schema_normalization_test.go b/internal/server/schema_normalization_test.go new file mode 100644 index 00000000..78216457 --- /dev/null +++ b/internal/server/schema_normalization_test.go @@ -0,0 +1,153 @@ +package server + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/githubnext/gh-aw-mcpg/internal/config" + "github.com/githubnext/gh-aw-mcpg/internal/mcp" +) + +// TestSchemaNormalization_Integration tests that broken schemas from backends +// are automatically normalized when tools are registered. +func TestSchemaNormalization_Integration(t *testing.T) { + ctx := context.Background() + + // Create a minimal config + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "test": { + Command: "echo", // Dummy command + Args: []string{}, + }, + }, + } + + // Create unified server + us, err := NewUnified(ctx, cfg) + require.NoError(t, err, "Failed to create unified server") + defer us.Close() + + testCases := []struct { + name string + toolName string + inputSchema map[string]interface{} + expectedSchema map[string]interface{} + }{ + { + name: "broken object schema without properties", + toolName: "get_commit", + inputSchema: map[string]interface{}{ + "type": "object", + }, + expectedSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + }, + }, + { + name: "valid object schema with properties", + toolName: "issue_read", + inputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "owner": map[string]interface{}{ + "type": "string", + }, + "repo": map[string]interface{}{ + "type": "string", + }, + }, + "required": []interface{}{"owner", "repo"}, + }, + expectedSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "owner": map[string]interface{}{ + "type": "string", + }, + "repo": map[string]interface{}{ + "type": "string", + }, + }, + "required": []interface{}{"owner", "repo"}, + }, + }, + { + name: "object schema with additionalProperties", + toolName: "list_items", + inputSchema: map[string]interface{}{ + "type": "object", + "additionalProperties": true, + }, + expectedSchema: map[string]interface{}{ + "type": "object", + "additionalProperties": true, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Simulate registering a tool with the given schema + prefixedName := "test___" + tc.toolName + + // Use the NormalizeInputSchema function directly + normalized := mcp.NormalizeInputSchema(tc.inputSchema, prefixedName) + + // Store the normalized tool + us.toolsMu.Lock() + us.tools[prefixedName] = &ToolInfo{ + Name: prefixedName, + Description: "Test tool", + BackendID: "test", + InputSchema: normalized, + } + us.toolsMu.Unlock() + + // Retrieve the tool and verify the schema + us.toolsMu.RLock() + tool, exists := us.tools[prefixedName] + us.toolsMu.RUnlock() + + require.True(t, exists, "Tool should exist") + assert.Equal(t, tc.expectedSchema, tool.InputSchema, "Schema should match expected normalized version") + + // Clean up + us.toolsMu.Lock() + delete(us.tools, prefixedName) + us.toolsMu.Unlock() + }) + } +} + +// TestSchemaNormalization_PreservesOriginal verifies that the normalization +// doesn't modify the original schema object +func TestSchemaNormalization_PreservesOriginal(t *testing.T) { + original := map[string]interface{}{ + "type": "object", + } + + // Make a copy to compare later + originalCopy := make(map[string]interface{}) + for k, v := range original { + originalCopy[k] = v + } + + // Normalize the schema + normalized := mcp.NormalizeInputSchema(original, "test-tool") + + // Verify original is unchanged + assert.Equal(t, originalCopy, original, "Original schema should not be modified") + + // Verify normalized has properties + _, hasProperties := normalized["properties"] + assert.True(t, hasProperties, "Normalized schema should have properties") + + // Verify original still doesn't have properties + _, originalHasProperties := original["properties"] + assert.False(t, originalHasProperties, "Original schema should not have properties") +} diff --git a/internal/server/unified.go b/internal/server/unified.go index 1faa9a01..a018a8f0 100644 --- a/internal/server/unified.go +++ b/internal/server/unified.go @@ -209,12 +209,15 @@ func (us *UnifiedServer) registerToolsFromBackend(serverID string) error { toolDesc := fmt.Sprintf("[%s] %s", serverID, tool.Description) toolNames = append(toolNames, prefixedName) + // Normalize the input schema to fix common validation issues + normalizedSchema := mcp.NormalizeInputSchema(tool.InputSchema, prefixedName) + // Store tool info for routed mode us.toolsMu.Lock() us.tools[prefixedName] = &ToolInfo{ Name: prefixedName, Description: toolDesc, - InputSchema: tool.InputSchema, + InputSchema: normalizedSchema, BackendID: serverID, } us.toolsMu.Unlock()