diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 89b290db2..317936533 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -124,7 +124,8 @@ func NewStdioMCPServer(cfg github.MCPServerConfig) (*mcp.Server, error) { WithDeprecatedAliases(github.DeprecatedToolAliases). WithReadOnly(cfg.ReadOnly). WithToolsets(cfg.EnabledToolsets). - WithTools(github.CleanTools(cfg.EnabledTools)) + WithTools(github.CleanTools(cfg.EnabledTools)). + WithServerInstructions() // WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)) // Apply token scope filtering if scopes are known (for PAT filtering) diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go deleted file mode 100644 index b8ad2ba8c..000000000 --- a/pkg/github/instructions_test.go +++ /dev/null @@ -1,186 +0,0 @@ -package github - -import ( - "os" - "strings" - "testing" -) - -func TestGenerateInstructions(t *testing.T) { - tests := []struct { - name string - enabledToolsets []string - expectedEmpty bool - }{ - { - name: "empty toolsets", - enabledToolsets: []string{}, - expectedEmpty: false, - }, - { - name: "only context toolset", - enabledToolsets: []string{"context"}, - expectedEmpty: false, - }, - { - name: "pull requests toolset", - enabledToolsets: []string{"pull_requests"}, - expectedEmpty: false, - }, - { - name: "issues toolset", - enabledToolsets: []string{"issues"}, - expectedEmpty: false, - }, - { - name: "discussions toolset", - enabledToolsets: []string{"discussions"}, - expectedEmpty: false, - }, - { - name: "multiple toolsets (context + pull_requests)", - enabledToolsets: []string{"context", "pull_requests"}, - expectedEmpty: false, - }, - { - name: "multiple toolsets (issues + pull_requests)", - enabledToolsets: []string{"issues", "pull_requests"}, - expectedEmpty: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := GenerateInstructions(tt.enabledToolsets) - - if tt.expectedEmpty { - if result != "" { - t.Errorf("Expected empty instructions but got: %s", result) - } - } else { - if result == "" { - t.Errorf("Expected non-empty instructions but got empty result") - } - } - }) - } -} - -func TestGenerateInstructionsWithDisableFlag(t *testing.T) { - tests := []struct { - name string - disableEnvValue string - enabledToolsets []string - expectedEmpty bool - }{ - { - name: "DISABLE_INSTRUCTIONS=true returns empty", - disableEnvValue: "true", - enabledToolsets: []string{"context", "issues", "pull_requests"}, - expectedEmpty: true, - }, - { - name: "DISABLE_INSTRUCTIONS=false returns normal instructions", - disableEnvValue: "false", - enabledToolsets: []string{"context"}, - expectedEmpty: false, - }, - { - name: "DISABLE_INSTRUCTIONS unset returns normal instructions", - disableEnvValue: "", - enabledToolsets: []string{"issues"}, - expectedEmpty: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Save original env value - originalValue := os.Getenv("DISABLE_INSTRUCTIONS") - defer func() { - if originalValue == "" { - os.Unsetenv("DISABLE_INSTRUCTIONS") - } else { - os.Setenv("DISABLE_INSTRUCTIONS", originalValue) - } - }() - - // Set test env value - if tt.disableEnvValue == "" { - os.Unsetenv("DISABLE_INSTRUCTIONS") - } else { - os.Setenv("DISABLE_INSTRUCTIONS", tt.disableEnvValue) - } - - result := GenerateInstructions(tt.enabledToolsets) - - if tt.expectedEmpty { - if result != "" { - t.Errorf("Expected empty instructions but got: %s", result) - } - } else { - if result == "" { - t.Errorf("Expected non-empty instructions but got empty result") - } - } - }) - } -} - -func TestGetToolsetInstructions(t *testing.T) { - tests := []struct { - toolset string - expectedEmpty bool - enabledToolsets []string - expectedToContain string - notExpectedToContain string - }{ - { - toolset: "pull_requests", - expectedEmpty: false, - enabledToolsets: []string{"pull_requests", "repos"}, - expectedToContain: "pull_request_template.md", - }, - { - toolset: "pull_requests", - expectedEmpty: false, - enabledToolsets: []string{"pull_requests"}, - notExpectedToContain: "pull_request_template.md", - }, - { - toolset: "issues", - expectedEmpty: false, - }, - { - toolset: "discussions", - expectedEmpty: false, - }, - { - toolset: "nonexistent", - expectedEmpty: true, - }, - } - - for _, tt := range tests { - t.Run(tt.toolset, func(t *testing.T) { - result := getToolsetInstructions(tt.toolset, tt.enabledToolsets) - if tt.expectedEmpty { - if result != "" { - t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result) - } - } else { - if result == "" { - t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset) - } - } - - if tt.expectedToContain != "" && !strings.Contains(result, tt.expectedToContain) { - t.Errorf("Expected result to contain '%s' for toolset '%s', but it did not. Result: %s", tt.expectedToContain, tt.toolset, result) - } - - if tt.notExpectedToContain != "" && strings.Contains(result, tt.notExpectedToContain) { - t.Errorf("Did not expect result to contain '%s' for toolset '%s', but it did. Result: %s", tt.notExpectedToContain, tt.toolset, result) - } - }) - } -} diff --git a/pkg/github/server.go b/pkg/github/server.go index cc8d815ef..047647206 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -71,18 +71,9 @@ type MCPServerConfig struct { type MCPServerOption func(*mcp.ServerOptions) func NewMCPServer(cfg *MCPServerConfig, deps ToolDependencies, inventory *inventory.Inventory) (*mcp.Server, error) { - enabledToolsets := resolveEnabledToolsets(cfg) - // For instruction generation, we need actual toolset names (not nil). - // nil means "use defaults" in inventory, so expand it for instructions. - instructionToolsets := enabledToolsets - if instructionToolsets == nil { - instructionToolsets = GetDefaultToolsetIDs() - } - - // Create the MCP server serverOpts := &mcp.ServerOptions{ - Instructions: GenerateInstructions(instructionToolsets), + Instructions: inventory.Instructions(), Logger: cfg.Logger, CompletionHandler: CompletionsHandler(deps.GetClient), } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index b15c4fc9a..a4db2e0be 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -28,10 +28,11 @@ var ( Icon: "check-circle", } ToolsetMetadataContext = inventory.ToolsetMetadata{ - ID: "context", - Description: "Tools that provide context about the current user and GitHub context you are operating in", - Default: true, - Icon: "person", + ID: "context", + Description: "Tools that provide context about the current user and GitHub context you are operating in", + Default: true, + Icon: "person", + InstructionsFunc: generateContextToolsetInstructions, } ToolsetMetadataRepos = inventory.ToolsetMetadata{ ID: "repos", @@ -45,16 +46,18 @@ var ( Icon: "git-branch", } ToolsetMetadataIssues = inventory.ToolsetMetadata{ - ID: "issues", - Description: "GitHub Issues related tools", - Default: true, - Icon: "issue-opened", + ID: "issues", + Description: "GitHub Issues related tools", + Default: true, + Icon: "issue-opened", + InstructionsFunc: generateIssuesToolsetInstructions, } ToolsetMetadataPullRequests = inventory.ToolsetMetadata{ - ID: "pull_requests", - Description: "GitHub Pull Request related tools", - Default: true, - Icon: "git-pull-request", + ID: "pull_requests", + Description: "GitHub Pull Request related tools", + Default: true, + Icon: "git-pull-request", + InstructionsFunc: generatePullRequestsToolsetInstructions, } ToolsetMetadataUsers = inventory.ToolsetMetadata{ ID: "users", @@ -93,9 +96,10 @@ var ( Icon: "bell", } ToolsetMetadataDiscussions = inventory.ToolsetMetadata{ - ID: "discussions", - Description: "GitHub Discussions related tools", - Icon: "comment-discussion", + ID: "discussions", + Description: "GitHub Discussions related tools", + Icon: "comment-discussion", + InstructionsFunc: generateDiscussionsToolsetInstructions, } ToolsetMetadataGists = inventory.ToolsetMetadata{ ID: "gists", @@ -108,9 +112,10 @@ var ( Icon: "shield", } ToolsetMetadataProjects = inventory.ToolsetMetadata{ - ID: "projects", - Description: "GitHub Projects related tools", - Icon: "project", + ID: "projects", + Description: "GitHub Projects related tools", + Icon: "project", + InstructionsFunc: generateProjectsToolsetInstructions, } ToolsetMetadataStargazers = inventory.ToolsetMetadata{ ID: "stargazers", diff --git a/pkg/github/instructions.go b/pkg/github/toolset_instructions.go similarity index 65% rename from pkg/github/instructions.go rename to pkg/github/toolset_instructions.go index 3a5fb54bb..bf2388a3d 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/toolset_instructions.go @@ -1,75 +1,41 @@ package github -import ( - "os" - "slices" - "strings" -) - -// GenerateInstructions creates server instructions based on enabled toolsets -func GenerateInstructions(enabledToolsets []string) string { - // For testing - add a flag to disable instructions - if os.Getenv("DISABLE_INSTRUCTIONS") == "true" { - return "" // Baseline mode - } - - var instructions []string +import "github.com/github/github-mcp-server/pkg/inventory" - // Core instruction - always included if context toolset enabled - if slices.Contains(enabledToolsets, "context") { - instructions = append(instructions, "Always call 'get_me' first to understand current user permissions and context.") - } - - // Individual toolset instructions - for _, toolset := range enabledToolsets { - if inst := getToolsetInstructions(toolset, enabledToolsets); inst != "" { - instructions = append(instructions, inst) - } - } +// Toolset instruction functions - these generate context-aware instructions for each toolset. +// They are called during inventory build to generate server instructions. - // Base instruction with context management - baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform. - -Tool selection guidance: - 1. Use 'list_*' tools for broad, simple retrieval and pagination of all items of a type (e.g., all issues, all PRs, all branches) with basic filtering. - 2. Use 'search_*' tools for targeted queries with specific criteria, keywords, or complex filters (e.g., issues with certain text, PRs by author, code containing functions). - -Context management: - 1. Use pagination whenever possible with batches of 5-10 items. - 2. Use minimal_output parameter set to true if the full information is not needed to accomplish a task. - -Tool usage guidance: - 1. For 'search_*' tools: Use separate 'sort' and 'order' parameters if available for sorting results - do not include 'sort:' syntax in query strings. Query strings should contain only search criteria (e.g., 'org:google language:python'), not sorting instructions.` +func generateContextToolsetInstructions(_ *inventory.Inventory) string { + return "Always call 'get_me' first to understand current user permissions and context." +} - allInstructions := []string{baseInstruction} - allInstructions = append(allInstructions, instructions...) +func generateIssuesToolsetInstructions(_ *inventory.Inventory) string { + return `## Issues - return strings.Join(allInstructions, " ") +Check 'list_issue_types' first for organizations to use proper issue types. Use 'search_issues' before creating new issues to avoid duplicates. Always set 'state_reason' when closing issues.` } -// getToolsetInstructions returns specific instructions for individual toolsets -func getToolsetInstructions(toolset string, enabledToolsets []string) string { - switch toolset { - case "pull_requests": - pullRequestInstructions := `## Pull Requests +func generatePullRequestsToolsetInstructions(inv *inventory.Inventory) string { + instructions := `## Pull Requests PR review workflow: Always use 'pull_request_review_write' with method 'create' to create a pending review, then 'add_comment_to_pending_review' to add comments, and finally 'pull_request_review_write' with method 'submit_pending' to submit the review for complex reviews with line-specific comments.` - if slices.Contains(enabledToolsets, "repos") { - pullRequestInstructions += ` + + if inv.HasToolset("repos") { + instructions += ` Before creating a pull request, search for pull request templates in the repository. Template files are called pull_request_template.md or they're located in '.github/PULL_REQUEST_TEMPLATE' directory. Use the template content to structure the PR description and then call create_pull_request tool.` - } - return pullRequestInstructions - case "issues": - return `## Issues + } + return instructions +} + +func generateDiscussionsToolsetInstructions(_ *inventory.Inventory) string { + return `## Discussions -Check 'list_issue_types' first for organizations to use proper issue types. Use 'search_issues' before creating new issues to avoid duplicates. Always set 'state_reason' when closing issues.` - case "discussions": - return `## Discussions - Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization.` - case "projects": - return `## Projects +} + +func generateProjectsToolsetInstructions(_ *inventory.Inventory) string { + return `## Projects Workflow: 1) list_project_fields (get field IDs), 2) list_project_items (with pagination), 3) optional updates. @@ -137,7 +103,4 @@ Common Qualifier Glossary (items): Never: - Infer field IDs; fetch via list_project_fields. - Drop 'fields' param on subsequent pages if field values are needed.` - default: - return "" - } } diff --git a/pkg/http/handler.go b/pkg/http/handler.go index f2fcb531f..7181fc8ee 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -125,6 +125,7 @@ func DefaultInventoryFactory(cfg *HTTPServerConfig, t translations.TranslationHe } b = InventoryFiltersForRequestHeaders(r, b) + b = b.WithServerInstructions() return b.Build() } } diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 0400c2a24..ed5e33e42 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -33,12 +33,13 @@ type Builder struct { deprecatedAliases map[string]string // Configuration options (processed at Build time) - readOnly bool - toolsetIDs []string // raw input, processed at Build() - toolsetIDsIsNil bool // tracks if nil was passed (nil = defaults) - additionalTools []string // raw input, processed at Build() - featureChecker FeatureFlagChecker - filters []ToolFilter // filters to apply to all tools + readOnly bool + toolsetIDs []string // raw input, processed at Build() + toolsetIDsIsNil bool // tracks if nil was passed (nil = defaults) + additionalTools []string // raw input, processed at Build() + featureChecker FeatureFlagChecker + filters []ToolFilter // filters to apply to all tools + generateInstructions bool } // NewBuilder creates a new Builder. @@ -83,6 +84,11 @@ func (b *Builder) WithReadOnly(readOnly bool) *Builder { return b } +func (b *Builder) WithServerInstructions() *Builder { + b.generateInstructions = true + return b +} + // WithToolsets specifies which toolsets should be enabled. // Special keywords: // - "all": enables all toolsets @@ -161,6 +167,10 @@ func (b *Builder) Build() *Inventory { } } + if b.generateInstructions { + r.instructions = generateInstructions(r) + } + return r } diff --git a/pkg/inventory/instructions.go b/pkg/inventory/instructions.go new file mode 100644 index 000000000..e4524eb43 --- /dev/null +++ b/pkg/inventory/instructions.go @@ -0,0 +1,43 @@ +package inventory + +import ( + "os" + "strings" +) + +// generateInstructions creates server instructions based on enabled toolsets +func generateInstructions(inv *Inventory) string { + // For testing - add a flag to disable instructions + if os.Getenv("DISABLE_INSTRUCTIONS") == "true" { + return "" // Baseline mode + } + + var instructions []string + + // Base instruction with context management + baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform. + +Tool selection guidance: + 1. Use 'list_*' tools for broad, simple retrieval and pagination of all items of a type (e.g., all issues, all PRs, all branches) with basic filtering. + 2. Use 'search_*' tools for targeted queries with specific criteria, keywords, or complex filters (e.g., issues with certain text, PRs by author, code containing functions). + +Context management: + 1. Use pagination whenever possible with batches of 5-10 items. + 2. Use minimal_output parameter set to true if the full information is not needed to accomplish a task. + +Tool usage guidance: + 1. For 'search_*' tools: Use separate 'sort' and 'order' parameters if available for sorting results - do not include 'sort:' syntax in query strings. Query strings should contain only search criteria (e.g., 'org:google language:python'), not sorting instructions.` + + instructions = append(instructions, baseInstruction) + + // Collect instructions from each enabled toolset + for _, toolset := range inv.AvailableToolsets() { + if toolset.InstructionsFunc != nil { + if toolsetInstructions := toolset.InstructionsFunc(inv); toolsetInstructions != "" { + instructions = append(instructions, toolsetInstructions) + } + } + } + + return strings.Join(instructions, " ") +} diff --git a/pkg/inventory/instructions_test.go b/pkg/inventory/instructions_test.go new file mode 100644 index 000000000..e12840b2a --- /dev/null +++ b/pkg/inventory/instructions_test.go @@ -0,0 +1,203 @@ +package inventory + +import ( + "os" + "strings" + "testing" +) + +// createTestInventory creates an inventory with the specified toolsets for testing. +func createTestInventory(toolsets []ToolsetMetadata) *Inventory { + // Create tools for each toolset so they show up in AvailableToolsets() + var tools []ServerTool + for _, ts := range toolsets { + tools = append(tools, ServerTool{ + Toolset: ts, + }) + } + + return NewBuilder(). + SetTools(tools). + Build() +} + +func TestGenerateInstructions(t *testing.T) { + tests := []struct { + name string + toolsets []ToolsetMetadata + expectedEmpty bool + }{ + { + name: "empty toolsets", + toolsets: []ToolsetMetadata{}, + expectedEmpty: false, // base instructions are always included + }, + { + name: "toolset with instructions", + toolsets: []ToolsetMetadata{ + { + ID: "test", + Description: "Test toolset", + InstructionsFunc: func(inv *Inventory) string { + return "Test instructions" + }, + }, + }, + expectedEmpty: false, + }, + { + name: "toolset without instructions", + toolsets: []ToolsetMetadata{ + { + ID: "test", + Description: "Test toolset", + }, + }, + expectedEmpty: false, // base instructions still included + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inv := createTestInventory(tt.toolsets) + result := generateInstructions(inv) + + if tt.expectedEmpty { + if result != "" { + t.Errorf("Expected empty instructions but got: %s", result) + } + } else { + if result == "" { + t.Errorf("Expected non-empty instructions but got empty result") + } + } + }) + } +} + +func TestGenerateInstructionsWithDisableFlag(t *testing.T) { + tests := []struct { + name string + disableEnvValue string + expectedEmpty bool + }{ + { + name: "DISABLE_INSTRUCTIONS=true returns empty", + disableEnvValue: "true", + expectedEmpty: true, + }, + { + name: "DISABLE_INSTRUCTIONS=false returns normal instructions", + disableEnvValue: "false", + expectedEmpty: false, + }, + { + name: "DISABLE_INSTRUCTIONS unset returns normal instructions", + disableEnvValue: "", + expectedEmpty: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Save original env value + originalValue := os.Getenv("DISABLE_INSTRUCTIONS") + defer func() { + if originalValue == "" { + os.Unsetenv("DISABLE_INSTRUCTIONS") + } else { + os.Setenv("DISABLE_INSTRUCTIONS", originalValue) + } + }() + + // Set test env value + if tt.disableEnvValue == "" { + os.Unsetenv("DISABLE_INSTRUCTIONS") + } else { + os.Setenv("DISABLE_INSTRUCTIONS", tt.disableEnvValue) + } + + inv := createTestInventory([]ToolsetMetadata{ + {ID: "test", Description: "Test"}, + }) + result := generateInstructions(inv) + + if tt.expectedEmpty { + if result != "" { + t.Errorf("Expected empty instructions but got: %s", result) + } + } else { + if result == "" { + t.Errorf("Expected non-empty instructions but got empty result") + } + } + }) + } +} + +func TestToolsetInstructionsFunc(t *testing.T) { + tests := []struct { + name string + toolsets []ToolsetMetadata + expectedToContain string + notExpectedToContain string + }{ + { + name: "toolset with context-aware instructions includes extra text when dependency present", + toolsets: []ToolsetMetadata{ + {ID: "repos", Description: "Repos"}, + { + ID: "pull_requests", + Description: "PRs", + InstructionsFunc: func(inv *Inventory) string { + instructions := "PR base instructions" + if inv.HasToolset("repos") { + instructions += " PR template instructions" + } + return instructions + }, + }, + }, + expectedToContain: "PR template instructions", + }, + { + name: "toolset with context-aware instructions excludes extra text when dependency missing", + toolsets: []ToolsetMetadata{ + { + ID: "pull_requests", + Description: "PRs", + InstructionsFunc: func(inv *Inventory) string { + instructions := "PR base instructions" + if inv.HasToolset("repos") { + instructions += " PR template instructions" + } + return instructions + }, + }, + }, + notExpectedToContain: "PR template instructions", + }, + { + name: "toolset without InstructionsFunc returns no toolset-specific instructions", + toolsets: []ToolsetMetadata{ + {ID: "test", Description: "Test without instructions"}, + }, + notExpectedToContain: "## Test", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inv := createTestInventory(tt.toolsets) + result := generateInstructions(inv) + + if tt.expectedToContain != "" && !strings.Contains(result, tt.expectedToContain) { + t.Errorf("Expected result to contain '%s', but it did not. Result: %s", tt.expectedToContain, result) + } + + if tt.notExpectedToContain != "" && strings.Contains(result, tt.notExpectedToContain) { + t.Errorf("Did not expect result to contain '%s', but it did. Result: %s", tt.notExpectedToContain, result) + } + }) + } +} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 885617b43..e4113b452 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -58,6 +58,8 @@ type Inventory struct { filters []ToolFilter // unrecognizedToolsets holds toolset IDs that were requested but don't match any registered toolsets unrecognizedToolsets []string + // server instructions hold high-level instructions for agents to use the server effectively + instructions string } // UnrecognizedToolsets returns toolset IDs that were passed to WithToolsets but don't @@ -292,3 +294,7 @@ func (r *Inventory) AvailableToolsets(exclude ...ToolsetID) []ToolsetMetadata { } return result } + +func (r *Inventory) Instructions() string { + return r.instructions +} diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 095bedf2b..752a4c2bd 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -31,6 +31,9 @@ type ToolsetMetadata struct { // Use the base name without size suffix, e.g., "repo" not "repo-16". // See https://primer.style/foundations/icons for available icons. Icon string + // InstructionsFunc optionally returns instructions for this toolset. + // It receives the inventory so it can check what other toolsets are enabled. + InstructionsFunc func(inv *Inventory) string } // Icons returns MCP Icon objects for this toolset, or nil if no icon is set.