diff --git a/docs/installation-guides/install-antigravity.md b/docs/installation-guides/install-antigravity.md index c24d8e01d..89e20f4ba 100644 --- a/docs/installation-guides/install-antigravity.md +++ b/docs/installation-guides/install-antigravity.md @@ -102,6 +102,10 @@ If you prefer running the server locally with Docker: ## Troubleshooting +### File contents not displaying properly + +If you experience issues with the `get_file_contents` tool not displaying file contents correctly, this may be due to limited embedded resource support in some versions of Antigravity. This is a known limitation that affects how file contents are displayed. + ### "Error: serverUrl or command must be specified" Make sure you're using `serverUrl` (not `url`) for the remote server configuration. Antigravity requires `serverUrl` for HTTP-based MCP servers. diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 46ec3bc64..91e7e98dc 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -345,6 +345,22 @@ See [Scope Filtering](./scope-filtering.md) for details on how filtering works w --- +## Feature Flags + +The GitHub MCP Server supports runtime feature flags that can modify tool behavior for improved compatibility with different MCP clients. + +### MCP_DISABLE_EMBEDDED_RESOURCES + +When enabled, the `get_file_contents` tool returns file content as standard MCP content types instead of embedded resources: +- **Text files**: Returned as `TextContent` with MIME type in metadata +- **Binary files**: Returned as `ImageContent` with raw binary data + +**Configuration:** + +Feature flags are checked at runtime via the feature flag checker passed to `BaseDeps`. You can configure this through environment variables or command-line arguments depending on your deployment setup. + +--- + ## Troubleshooting | Problem | Cause | Solution | diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index fd06a659b..bbae6c028 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -5,3 +5,9 @@ type FeatureFlags struct { LockdownMode bool InsidersMode bool } + +// FeatureFlagDisableEmbeddedResources is a feature flag that when enabled, +// returns file contents as regular MCP content (TextContent/ImageContent) +// instead of EmbeddedResource. This provides better compatibility with +// clients that don't support embedded resources. +const FeatureFlagDisableEmbeddedResources = "MCP_DISABLE_EMBEDDED_RESOURCES" diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index f6203f39f..93424a2ec 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -768,6 +768,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool strings.HasSuffix(contentType, "+json") || strings.HasSuffix(contentType, "+xml") + // Check if embedded resources should be disabled + disableEmbedded := deps.IsFeatureEnabled(ctx, FeatureFlagDisableEmbeddedResources) + if isTextContent { result := &mcp.ResourceContents{ URI: resourceURI, @@ -776,9 +779,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } // Include SHA in the result metadata if fileSHA != "" { - return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)", fileSHA)+successNote, result), nil, nil + return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)", fileSHA)+successNote, result, disableEmbedded), nil, nil } - return utils.NewToolResultResource("successfully downloaded text file"+successNote, result), nil, nil + return utils.NewToolResultResource("successfully downloaded text file"+successNote, result, disableEmbedded), nil, nil } result := &mcp.ResourceContents{ @@ -788,9 +791,9 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } // Include SHA in the result metadata if fileSHA != "" { - return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)", fileSHA)+successNote, result), nil, nil + return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)", fileSHA)+successNote, result, disableEmbedded), nil, nil } - return utils.NewToolResultResource("successfully downloaded binary file"+successNote, result), nil, nil + return utils.NewToolResultResource("successfully downloaded binary file"+successNote, result, disableEmbedded), nil, nil } // Raw API call failed diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index d91af8851..1b3067503 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -403,6 +403,183 @@ func Test_GetFileContents(t *testing.T) { } } +func Test_GetFileContents_WithDisabledEmbeddedResourcesFlag(t *testing.T) { + // Verify that when MCP_DISABLE_EMBEDDED_RESOURCES flag is enabled, + // file contents are returned as TextContent/ImageContent instead of EmbeddedResource + serverTool := GetFileContents(translations.NullTranslationHelper) + + mockRawContent := []byte("# Test Content\n\nSample text.") + mockBinaryContent := []byte{0x89, 0x50, 0x4E, 0x47} // PNG header + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + flagEnabled bool + expectTextContent bool // true for TextContent, false for ImageContent + expectedText string + expectedMimeType string + }{ + { + name: "text file with flag enabled", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("test.md"), + Path: github.Ptr("test.md"), + SHA: github.Ptr("abc123"), + Type: github.Ptr("file"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }, + GetRawReposContentsByOwnerByRepoByBranchByPath: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/markdown") + _, _ = w.Write(mockRawContent) + }, + }), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "path": "test.md", + "ref": "refs/heads/main", + }, + flagEnabled: true, + expectTextContent: true, + expectedText: string(mockRawContent), + expectedMimeType: "text/markdown", + }, + { + name: "binary file with flag enabled", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("image.png"), + Path: github.Ptr("image.png"), + SHA: github.Ptr("def456"), + Type: github.Ptr("file"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }, + GetRawReposContentsByOwnerByRepoByBranchByPath: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write(mockBinaryContent) + }, + }), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "path": "image.png", + "ref": "refs/heads/main", + }, + flagEnabled: true, + expectTextContent: false, + expectedMimeType: "image/png", + }, + { + name: "text file with flag disabled (default behavior)", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("test.md"), + Path: github.Ptr("test.md"), + SHA: github.Ptr("abc123"), + Type: github.Ptr("file"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }, + GetRawReposContentsByOwnerByRepoByBranchByPath: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/markdown") + _, _ = w.Write(mockRawContent) + }, + }), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "path": "test.md", + "ref": "refs/heads/main", + }, + flagEnabled: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + mockRawClient := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"}) + + // Create feature flag checker + featureChecker := func(_ context.Context, flagName string) (bool, error) { + if flagName == FeatureFlagDisableEmbeddedResources { + return tc.flagEnabled, nil + } + return false, nil + } + + deps := BaseDeps{ + Client: client, + RawClient: mockRawClient, + featureChecker: featureChecker, + } + handler := serverTool.Handler(deps) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + // Verify results + require.Len(t, result.Content, 2, "Expected 2 content items") + assert.False(t, result.IsError) + + // First item should always be the message text + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "First content should be TextContent") + assert.Contains(t, textContent.Text, "successfully downloaded") + + if tc.flagEnabled { + // When flag is enabled, second item should be TextContent or ImageContent + if tc.expectTextContent { + // Expecting TextContent for text files + content, ok := result.Content[1].(*mcp.TextContent) + require.True(t, ok, "Expected TextContent for text file with flag enabled, got %T", result.Content[1]) + assert.Equal(t, tc.expectedText, content.Text) + assert.NotNil(t, content.Meta) + assert.Equal(t, tc.expectedMimeType, content.Meta["mimeType"]) + assert.NotNil(t, content.Annotations) + } else { + // Expecting ImageContent for binary files + content, ok := result.Content[1].(*mcp.ImageContent) + require.True(t, ok, "Expected ImageContent for binary file with flag enabled, got %T", result.Content[1]) + assert.Equal(t, tc.expectedMimeType, content.MIMEType) + assert.NotNil(t, content.Meta) + assert.NotNil(t, content.Annotations) + // Verify data contains raw binary (SDK handles base64 encoding during JSON marshaling) + assert.Equal(t, mockBinaryContent, content.Data) + } + } else { + // When flag is disabled, should use EmbeddedResource (default) + _, ok := result.Content[1].(*mcp.EmbeddedResource) + assert.True(t, ok, "Expected EmbeddedResource when flag is disabled, got %T", result.Content[1]) + } + }) + } +} + func Test_ForkRepository(t *testing.T) { // Verify tool definition once serverTool := ForkRepository(translations.NullTranslationHelper) diff --git a/pkg/utils/result.go b/pkg/utils/result.go index 533fe0573..c2292ec95 100644 --- a/pkg/utils/result.go +++ b/pkg/utils/result.go @@ -1,6 +1,8 @@ package utils //nolint:revive //TODO: figure out a better name for this package -import "github.com/modelcontextprotocol/go-sdk/mcp" +import ( + "github.com/modelcontextprotocol/go-sdk/mcp" +) func NewToolResultText(message string) *mcp.CallToolResult { return &mcp.CallToolResult{ @@ -34,15 +36,75 @@ func NewToolResultErrorFromErr(message string, err error) *mcp.CallToolResult { } } -func NewToolResultResource(message string, contents *mcp.ResourceContents) *mcp.CallToolResult { +// NewToolResultResource returns a CallToolResult with either an embedded resource +// or regular content based on the disableEmbeddedResources flag. +// When disableEmbeddedResources is true, text content is returned as TextContent and +// binary content is returned as ImageContent, providing better client compatibility. +func NewToolResultResource(message string, contents *mcp.ResourceContents, disableEmbeddedResources bool) *mcp.CallToolResult { + if !disableEmbeddedResources { + // Default behavior - return as embedded resource + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{ + Text: message, + }, + &mcp.EmbeddedResource{ + Resource: contents, + }, + }, + IsError: false, + } + } + + // When flag is enabled, return as regular content + var content mcp.Content + switch { + case contents.Text != "": + // Text content - use TextContent with MIME type in metadata + content = &mcp.TextContent{ + Text: contents.Text, + Annotations: &mcp.Annotations{ + Audience: []mcp.Role{"user"}, + }, + Meta: mcp.Meta{ + "mimeType": contents.MIMEType, + "uri": contents.URI, + }, + } + case len(contents.Blob) > 0: + // Binary content - use ImageContent with raw binary data + // Note: MCP SDK will handle base64 encoding during JSON marshaling + content = &mcp.ImageContent{ + Data: contents.Blob, + MIMEType: contents.MIMEType, + Meta: mcp.Meta{ + "uri": contents.URI, + }, + Annotations: &mcp.Annotations{ + Audience: []mcp.Role{"user"}, + }, + } + default: + // Fallback to embedded resource if neither text nor blob + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{ + Text: message, + }, + &mcp.EmbeddedResource{ + Resource: contents, + }, + }, + IsError: false, + } + } + return &mcp.CallToolResult{ Content: []mcp.Content{ &mcp.TextContent{ Text: message, }, - &mcp.EmbeddedResource{ - Resource: contents, - }, + content, }, IsError: false, } diff --git a/pkg/utils/result_test.go b/pkg/utils/result_test.go new file mode 100644 index 000000000..0f270da15 --- /dev/null +++ b/pkg/utils/result_test.go @@ -0,0 +1,116 @@ +package utils //nolint:revive //TODO: figure out a better name for this package + +import ( + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewToolResultResource_DisabledFlag(t *testing.T) { + // When flag is disabled, should return embedded resource (default behavior) + contents := &mcp.ResourceContents{ + URI: "test://file.txt", + Text: "Hello, World!", + MIMEType: "text/plain", + } + + result := NewToolResultResource("Test message", contents, false) + + require.NotNil(t, result) + require.Len(t, result.Content, 2) + assert.False(t, result.IsError) + + // First content should be text message + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Equal(t, "Test message", textContent.Text) + + // Second content should be embedded resource + embeddedResource, ok := result.Content[1].(*mcp.EmbeddedResource) + require.True(t, ok) + assert.Equal(t, contents.URI, embeddedResource.Resource.URI) + assert.Equal(t, contents.Text, embeddedResource.Resource.Text) + assert.Equal(t, contents.MIMEType, embeddedResource.Resource.MIMEType) +} + +func TestNewToolResultResource_EnabledFlag_TextContent(t *testing.T) { + // When flag is enabled with text content, should return TextContent + contents := &mcp.ResourceContents{ + URI: "test://file.txt", + Text: "Hello, World!", + MIMEType: "text/plain", + } + + result := NewToolResultResource("Test message", contents, true) + + require.NotNil(t, result) + require.Len(t, result.Content, 2) + assert.False(t, result.IsError) + + // First content should be text message + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Equal(t, "Test message", textContent.Text) + + // Second content should be TextContent (not embedded resource) + textContent, ok = result.Content[1].(*mcp.TextContent) + require.True(t, ok, "Expected TextContent but got %T", result.Content[1]) + assert.Equal(t, "Hello, World!", textContent.Text) + assert.NotNil(t, textContent.Meta) + assert.Equal(t, "text/plain", textContent.Meta["mimeType"]) + assert.Equal(t, "test://file.txt", textContent.Meta["uri"]) + assert.NotNil(t, textContent.Annotations) + assert.Contains(t, textContent.Annotations.Audience, mcp.Role("user")) +} + +func TestNewToolResultResource_EnabledFlag_BinaryContent(t *testing.T) { + // When flag is enabled with binary content, should return ImageContent + binaryData := []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A} // PNG header + contents := &mcp.ResourceContents{ + URI: "test://image.png", + Blob: binaryData, + MIMEType: "image/png", + } + + result := NewToolResultResource("Binary message", contents, true) + + require.NotNil(t, result) + require.Len(t, result.Content, 2) + assert.False(t, result.IsError) + + // First content should be text message + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Equal(t, "Binary message", textContent.Text) + + // Second content should be ImageContent (not embedded resource) + imageContent, ok := result.Content[1].(*mcp.ImageContent) + require.True(t, ok, "Expected ImageContent but got %T", result.Content[1]) + + // Data should be raw binary (SDK handles base64 encoding during JSON marshaling) + assert.Equal(t, binaryData, imageContent.Data) + assert.Equal(t, "image/png", imageContent.MIMEType) + assert.NotNil(t, imageContent.Meta) + assert.Equal(t, "test://image.png", imageContent.Meta["uri"]) + assert.NotNil(t, imageContent.Annotations) + assert.Contains(t, imageContent.Annotations.Audience, mcp.Role("user")) +} + +func TestNewToolResultResource_EnabledFlag_EmptyContent(t *testing.T) { + // When flag is enabled but neither text nor blob exists, should fallback to embedded resource + contents := &mcp.ResourceContents{ + URI: "test://empty", + MIMEType: "application/octet-stream", + } + + result := NewToolResultResource("Empty message", contents, true) + + require.NotNil(t, result) + require.Len(t, result.Content, 2) + + // Should fallback to embedded resource + _, ok := result.Content[1].(*mcp.EmbeddedResource) + assert.True(t, ok, "Expected fallback to EmbeddedResource for empty content") +}