diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 250f6b4cc..4ddc76a87 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -198,6 +198,9 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext) ghServer.AddReceivingMiddleware(addUserAgentsMiddleware(cfg, clients.rest, clients.gqlHTTP)) + // Create feature checker + featureChecker := createFeatureChecker(cfg.EnabledFeatures) + // Create dependencies for tool handlers deps := github.NewBaseDeps( clients.rest, @@ -207,6 +210,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { cfg.Translator, github.FeatureFlags{LockdownMode: cfg.LockdownMode}, cfg.ContentWindowSize, + featureChecker, ) // Inject dependencies into context for all tool handlers @@ -222,7 +226,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { WithReadOnly(cfg.ReadOnly). WithToolsets(enabledToolsets). WithTools(github.CleanTools(cfg.EnabledTools)). - WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)) + WithFeatureChecker(featureChecker) // Apply token scope filtering if scopes are known (for PAT filtering) if cfg.TokenScopes != nil { diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index b41bf0b87..201e434b0 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -3,6 +3,8 @@ package github import ( "context" "errors" + "fmt" + "os" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" @@ -77,6 +79,11 @@ type ToolDependencies interface { // GetContentWindowSize returns the content window size for log truncation GetContentWindowSize() int + + // IsFeatureEnabled checks if a feature flag is enabled. + // Returns false if checker is nil or flag is not enabled. + // This allows tools to conditionally change behavior based on feature flags. + IsFeatureEnabled(ctx context.Context, flagName string) bool } // BaseDeps is the standard implementation of ToolDependencies for the local server. @@ -93,6 +100,9 @@ type BaseDeps struct { T translations.TranslationHelperFunc Flags FeatureFlags ContentWindowSize int + + // Feature flag checker for runtime checks + featureChecker inventory.FeatureFlagChecker } // NewBaseDeps creates a BaseDeps with the provided clients and configuration. @@ -104,6 +114,7 @@ func NewBaseDeps( t translations.TranslationHelperFunc, flags FeatureFlags, contentWindowSize int, + featureChecker inventory.FeatureFlagChecker, ) *BaseDeps { return &BaseDeps{ Client: client, @@ -113,6 +124,7 @@ func NewBaseDeps( T: t, Flags: flags, ContentWindowSize: contentWindowSize, + featureChecker: featureChecker, } } @@ -143,6 +155,24 @@ func (d BaseDeps) GetFlags() FeatureFlags { return d.Flags } // GetContentWindowSize implements ToolDependencies. func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// IsFeatureEnabled checks if a feature flag is enabled. +// Returns false if the feature checker is nil, flag name is empty, or an error occurs. +// This allows tools to conditionally change behavior based on feature flags. +func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { + if d.featureChecker == nil || flagName == "" { + return false + } + + enabled, err := d.featureChecker(ctx, flagName) + if err != nil { + // Log error but don't fail the tool - treat as disabled + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err) + return false + } + + return enabled +} + // NewTool creates a ServerTool that retrieves ToolDependencies from context at call time. // This avoids creating closures at registration time, which is important for performance // in servers that create a new server instance per request (like the remote server). diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go new file mode 100644 index 000000000..8d3d3fa4d --- /dev/null +++ b/pkg/github/dependencies_test.go @@ -0,0 +1,109 @@ +package github_test + +import ( + "context" + "errors" + "testing" + + "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/stretchr/testify/assert" +) + +func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { + t.Parallel() + + // Create a feature checker that returns true for "test_flag" + checker := func(ctx context.Context, flagName string) (bool, error) { + return flagName == "test_flag", nil + } + + // Create deps with the checker using NewBaseDeps + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Test enabled flag + result := deps.IsFeatureEnabled(context.Background(), "test_flag") + assert.True(t, result, "Expected test_flag to be enabled") + + // Test disabled flag + result = deps.IsFeatureEnabled(context.Background(), "other_flag") + assert.False(t, result, "Expected other_flag to be disabled") +} + +func TestIsFeatureEnabled_WithoutChecker(t *testing.T) { + t.Parallel() + + // Create deps without feature checker (nil) + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + nil, // featureChecker (nil) + ) + + // Should return false when checker is nil + result := deps.IsFeatureEnabled(context.Background(), "any_flag") + assert.False(t, result, "Expected false when checker is nil") +} + +func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { + t.Parallel() + + // Create a feature checker + checker := func(ctx context.Context, flagName string) (bool, error) { + return true, nil + } + + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Should return false for empty flag name + result := deps.IsFeatureEnabled(context.Background(), "") + assert.False(t, result, "Expected false for empty flag name") +} + +func TestIsFeatureEnabled_CheckerError(t *testing.T) { + t.Parallel() + + // Create a feature checker that returns an error + checker := func(ctx context.Context, flagName string) (bool, error) { + return false, errors.New("checker error") + } + + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Should return false and log error (not crash) + result := deps.IsFeatureEnabled(context.Background(), "error_flag") + assert.False(t, result, "Expected false when checker returns error") +} + diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go index 8d12b78c2..3fc701eb4 100644 --- a/pkg/github/dynamic_tools_test.go +++ b/pkg/github/dynamic_tools_test.go @@ -133,7 +133,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) { deps := DynamicToolDependencies{ Server: server, Inventory: reg, - ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0), + ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil), T: translations.NullTranslationHelper, } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 047042e44..04ab4e7c2 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -4,3 +4,7 @@ package github type FeatureFlags struct { LockdownMode bool } + +// RemoteMCPExperimental is a long-lived feature flag for experimental remote MCP features. +// This flag enables experimental behaviors in tools that are being tested for remote server deployment. +const RemoteMCPExperimental = "remote_mcp_experimental" diff --git a/pkg/github/hello.go b/pkg/github/hello.go new file mode 100644 index 000000000..ff27ede77 --- /dev/null +++ b/pkg/github/hello.go @@ -0,0 +1,70 @@ +package github + +import ( + "context" + "encoding/json" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// HelloWorld returns a simple greeting tool that demonstrates feature flag conditional behavior. +// This tool is for testing and demonstration purposes only. +func HelloWorld(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataContext, // Use existing "context" toolset + mcp.Tool{ + Name: "hello_world", + Description: t("TOOL_HELLO_WORLD_DESCRIPTION", "A simple greeting tool that demonstrates feature flag conditional behavior"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_HELLO_WORLD_TITLE", "Hello World"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "Name to greet (optional, defaults to 'World')", + }, + }, + }, + }, + []scopes.Scope{}, // No GitHub scopes required - purely demonstrative + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // Extract name parameter (optional) + name := "World" + if nameArg, ok := args["name"].(string); ok && nameArg != "" { + name = nameArg + } + + // Check feature flag to determine greeting style + var greeting string + if deps.IsFeatureEnabled(ctx, RemoteMCPExperimental) { + // Experimental: More enthusiastic greeting + greeting = "🚀 Hello, " + name + "! Welcome to the EXPERIMENTAL future of MCP! 🎉" + } else { + // Default: Simple greeting + greeting = "Hello, " + name + "!" + } + + // Build response + response := map[string]any{ + "greeting": greeting, + "experimental_mode": deps.IsFeatureEnabled(ctx, RemoteMCPExperimental), + "timestamp": "2026-01-12", // Static for demonstration + } + + jsonBytes, err := json.Marshal(response) + if err != nil { + return utils.NewToolResultError("failed to marshal response"), nil, nil + } + + return utils.NewToolResultText(string(jsonBytes)), nil, nil + }, + ) +} diff --git a/pkg/github/hello_test.go b/pkg/github/hello_test.go new file mode 100644 index 000000000..d30c54e8f --- /dev/null +++ b/pkg/github/hello_test.go @@ -0,0 +1,83 @@ +package github_test + +import ( + "context" + "testing" + + "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHelloWorld_ToolDefinition(t *testing.T) { + t.Parallel() + + // Create tool + tool := github.HelloWorld(translations.NullTranslationHelper) + + // Verify tool definition + assert.Equal(t, "hello_world", tool.Tool.Name) + assert.NotEmpty(t, tool.Tool.Description) + assert.True(t, tool.Tool.Annotations.ReadOnlyHint, "hello_world should be read-only") + assert.NotNil(t, tool.Tool.InputSchema) + assert.NotNil(t, tool.HandlerFunc, "Tool must have a handler") + + // Verify it's in the context toolset + assert.Equal(t, "context", string(tool.Toolset.ID)) + + // Verify no scopes required + assert.Empty(t, tool.RequiredScopes) + + // Verify no feature flags set (tool itself isn't gated by flags) + assert.Empty(t, tool.FeatureFlagEnable) + assert.Empty(t, tool.FeatureFlagDisable) +} + +func TestHelloWorld_IsFeatureEnabledIntegration(t *testing.T) { + t.Parallel() + + // This test verifies that the feature flag checking mechanism works + // by testing deps.IsFeatureEnabled directly + + // Test 1: With feature flag disabled + checkerDisabled := func(ctx context.Context, flagName string) (bool, error) { + return false, nil + } + depsDisabled := github.NewBaseDeps( + nil, nil, nil, nil, + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, + checkerDisabled, + ) + + result := depsDisabled.IsFeatureEnabled(context.Background(), github.RemoteMCPExperimental) + assert.False(t, result, "Feature flag should be disabled") + + // Test 2: With feature flag enabled + checkerEnabled := func(ctx context.Context, flagName string) (bool, error) { + return flagName == github.RemoteMCPExperimental, nil + } + depsEnabled := github.NewBaseDeps( + nil, nil, nil, nil, + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, + checkerEnabled, + ) + + result = depsEnabled.IsFeatureEnabled(context.Background(), github.RemoteMCPExperimental) + assert.True(t, result, "Feature flag should be enabled") + + result = depsEnabled.IsFeatureEnabled(context.Background(), "other_flag") + assert.False(t, result, "Other flag should be disabled") +} + +func TestHelloWorld_FeatureFlagConstant(t *testing.T) { + t.Parallel() + + // Verify the constant exists and has the expected value + assert.Equal(t, "remote_mcp_experimental", github.RemoteMCPExperimental) + require.NotEmpty(t, github.RemoteMCPExperimental, "Feature flag constant should not be empty") +} diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index a59cd9a93..503ee39d0 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -55,6 +55,7 @@ func (s stubDeps) GetRepoAccessCache() *lockdown.RepoAccessCache { return s.repo func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t } func (s stubDeps) GetFlags() FeatureFlags { return s.flags } func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } +func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } // Helper functions to create stub client functions for error testing func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*github.Client, error) {