From be881f8fca50ab848c3f3118b129d6b2ae178ceb Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 13 Sep 2025 19:19:40 -0700 Subject: [PATCH 01/11] Add instruction generation for enabled toolsets and corresponding tests --- internal/ghmcp/server.go | 12 ++- pkg/github/instructions.go | 88 ++++++++++++++++ pkg/github/instructions_test.go | 181 ++++++++++++++++++++++++++++++++ 3 files changed, 279 insertions(+), 2 deletions(-) create mode 100644 pkg/github/instructions.go create mode 100644 pkg/github/instructions_test.go diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 7ad71532f..e130064eb 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -105,8 +105,6 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { }, } - ghServer := github.NewServer(cfg.Version, server.WithHooks(hooks)) - enabledToolsets := cfg.EnabledToolsets if cfg.DynamicToolsets { // filter "all" from the enabled toolsets @@ -118,6 +116,16 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { } } + // Generate instructions based on enabled toolsets + instructions := github.GenerateInstructions(enabledToolsets) + var serverOpts []server.ServerOption + if instructions != "" { + serverOpts = append(serverOpts, server.WithInstructions(instructions)) + } + serverOpts = append(serverOpts, server.WithHooks(hooks)) + + ghServer := github.NewServer(cfg.Version, serverOpts...) + getClient := func(_ context.Context) (*gogithub.Client, error) { return restClient, nil // closing over client } diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go new file mode 100644 index 000000000..853a0d579 --- /dev/null +++ b/pkg/github/instructions.go @@ -0,0 +1,88 @@ +package github + +import "strings" + +// GenerateInstructions creates server instructions based on enabled toolsets +func GenerateInstructions(enabledToolsets []string) string { + var instructions []string + + // Core instruction - always included if context toolset enabled + if 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); inst != "" { + instructions = append(instructions, inst) + } + } + + // Cross-toolset conditional instructions + if contains(enabledToolsets, "pull_requests") && contains(enabledToolsets, "context") { + instructions = append(instructions, "For team workflows: Use 'get_teams' and 'get_team_members' before assigning PR reviewers.") + } + + if hasAnySecurityToolset(enabledToolsets) { + instructions = append(instructions, "Security alert priority: secret_scanning → dependabot → code_scanning alerts.") + } + + if contains(enabledToolsets, "issues") && contains(enabledToolsets, "pull_requests") { + instructions = append(instructions, "Link issues to PRs using 'closes #123' or 'fixes #123' in PR descriptions.") + } + + if contains(enabledToolsets, "repos") && contains(enabledToolsets, "actions") { + instructions = append(instructions, "For repository operations: Check workflow status before major changes using 'list_workflow_runs'.") + } + + // Only add base instruction if we have specific instructions + if len(instructions) > 0 { + baseInstruction := "GitHub MCP Server provides GitHub API tools. Common parameters: 'owner' (repo owner), 'repo' (repo name), 'page'/'perPage' (pagination)." + instructions = append([]string{baseInstruction}, instructions...) + } + + return strings.Join(instructions, " ") +} + +// getToolsetInstructions returns specific instructions for individual toolsets +func getToolsetInstructions(toolset string) string { + switch toolset { + case "pull_requests": + return "PR review workflow: Use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." + case "actions": + return "CI/CD debugging: Use 'get_job_logs' with 'failed_only=true' and 'return_content=true' for immediate log analysis. Use 'rerun_failed_jobs' instead of 'rerun_workflow_run' to save resources." + case "issues": + return "Issue workflow: 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 "repos": + return "File operations: Use 'get_file_contents' to check if file exists before 'create_or_update_file'. Always specify 'sha' parameter when updating existing files. Use 'push_files' for multiple file operations in single commit." + case "notifications": + return "Notifications: Filter by 'participating' for issues/PRs you're involved in. Use 'mark_all_notifications_read' with repository filters to avoid marking unrelated notifications." + case "gists": + return "Gists: Use 'list_gists' with 'since' parameter to find recent gists. Specify 'public=false' for private gists in 'create_gist'." + case "discussions": + return "Discussions: Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization." + default: + return "" + } +} + +// hasAnySecurityToolset checks if any security-related toolsets are enabled +func hasAnySecurityToolset(toolsets []string) bool { + securityToolsets := []string{"code_security", "secret_protection", "dependabot"} + for _, security := range securityToolsets { + if contains(toolsets, security) { + return true + } + } + return false +} + +// contains checks if a slice contains a specific string +func contains(slice []string, item string) bool { + for _, s := range slice { + if s == item { + return true + } + } + return false +} \ No newline at end of file diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go new file mode 100644 index 000000000..f9d17e662 --- /dev/null +++ b/pkg/github/instructions_test.go @@ -0,0 +1,181 @@ +package github + +import ( + "strings" + "testing" +) + +func TestGenerateInstructions(t *testing.T) { + tests := []struct { + name string + enabledToolsets []string + expectedContains []string + expectedEmpty bool + }{ + { + name: "empty toolsets", + enabledToolsets: []string{}, + expectedEmpty: true, + }, + { + name: "only context toolset", + enabledToolsets: []string{"context"}, + expectedContains: []string{ + "GitHub MCP Server provides GitHub API tools", + "Always call 'get_me' first", + }, + }, + { + name: "pull requests toolset", + enabledToolsets: []string{"pull_requests"}, + expectedContains: []string{ + "create_pending_pull_request_review", + "add_comment_to_pending_review", + "submit_pending_pull_request_review", + }, + }, + { + name: "actions toolset", + enabledToolsets: []string{"actions"}, + expectedContains: []string{ + "get_job_logs", + "failed_only=true", + "rerun_failed_jobs", + }, + }, + { + name: "security toolsets", + enabledToolsets: []string{"code_security", "secret_protection", "dependabot"}, + expectedContains: []string{ + "Security alert priority", + "secret_scanning", + "dependabot", + "code_scanning", + }, + }, + { + name: "cross-toolset instructions", + enabledToolsets: []string{"context", "pull_requests"}, + expectedContains: []string{ + "get_me", + "get_teams", + "get_team_members", + }, + }, + { + name: "issues and pull_requests combination", + enabledToolsets: []string{"issues", "pull_requests"}, + expectedContains: []string{ + "closes #123", + "fixes #123", + "search_issues", + "list_issue_types", + }, + }, + } + + 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) + } + return + } + + for _, expectedContent := range tt.expectedContains { + if !strings.Contains(result, expectedContent) { + t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result) + } + } + }) + } +} + +func TestGetToolsetInstructions(t *testing.T) { + tests := []struct { + toolset string + expected string + }{ + { + toolset: "pull_requests", + expected: "create_pending_pull_request_review", + }, + { + toolset: "actions", + expected: "get_job_logs", + }, + { + toolset: "issues", + expected: "list_issue_types", + }, + { + toolset: "repos", + expected: "get_file_contents", + }, + { + toolset: "nonexistent", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.toolset, func(t *testing.T) { + result := getToolsetInstructions(tt.toolset) + if tt.expected == "" { + if result != "" { + t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result) + } + } else { + if !strings.Contains(result, tt.expected) { + t.Errorf("Expected instructions for '%s' to contain '%s', but got: %s", tt.toolset, tt.expected, result) + } + } + }) + } +} + +func TestHasAnySecurityToolset(t *testing.T) { + tests := []struct { + name string + toolsets []string + expected bool + }{ + { + name: "no security toolsets", + toolsets: []string{"repos", "issues"}, + expected: false, + }, + { + name: "has code_security", + toolsets: []string{"repos", "code_security"}, + expected: true, + }, + { + name: "has secret_protection", + toolsets: []string{"secret_protection", "issues"}, + expected: true, + }, + { + name: "has dependabot", + toolsets: []string{"dependabot"}, + expected: true, + }, + { + name: "has all security toolsets", + toolsets: []string{"code_security", "secret_protection", "dependabot"}, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := hasAnySecurityToolset(tt.toolsets) + if result != tt.expected { + t.Errorf("Expected %v for toolsets %v, but got %v", tt.expected, tt.toolsets, result) + } + }) + } +} \ No newline at end of file From b78cb8930f7fd1b4b58ce24d7e1beed822ec5309 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 13 Sep 2025 19:33:03 -0700 Subject: [PATCH 02/11] Refactor instruction generation to always include base and context management instructions --- internal/ghmcp/server.go | 12 +++++------- pkg/github/instructions.go | 15 +++++++++------ pkg/github/instructions_test.go | 6 +++++- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index e130064eb..9de0682f3 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -118,13 +118,11 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { // Generate instructions based on enabled toolsets instructions := github.GenerateInstructions(enabledToolsets) - var serverOpts []server.ServerOption - if instructions != "" { - serverOpts = append(serverOpts, server.WithInstructions(instructions)) - } - serverOpts = append(serverOpts, server.WithHooks(hooks)) - - ghServer := github.NewServer(cfg.Version, serverOpts...) + + ghServer := github.NewServer(cfg.Version, + server.WithInstructions(instructions), + server.WithHooks(hooks), + ) getClient := func(_ context.Context) (*gogithub.Client, error) { return restClient, nil // closing over client diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 853a0d579..f550a5cae 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -35,13 +35,16 @@ func GenerateInstructions(enabledToolsets []string) string { instructions = append(instructions, "For repository operations: Check workflow status before major changes using 'list_workflow_runs'.") } - // Only add base instruction if we have specific instructions - if len(instructions) > 0 { - baseInstruction := "GitHub MCP Server provides GitHub API tools. Common parameters: 'owner' (repo owner), 'repo' (repo name), 'page'/'perPage' (pagination)." - instructions = append([]string{baseInstruction}, instructions...) - } + // Always add base instructions - they provide universal value + baseInstruction := "GitHub MCP Server provides GitHub API tools. Common parameters: 'owner' (repo owner), 'repo' (repo name), 'page'/'perPage' (pagination)." + + // Add context management instruction for all configurations + contextInstruction := "GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." + + allInstructions := []string{baseInstruction, contextInstruction} + allInstructions = append(allInstructions, instructions...) - return strings.Join(instructions, " ") + return strings.Join(allInstructions, " ") } // getToolsetInstructions returns specific instructions for individual toolsets diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index f9d17e662..684b1e4f1 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -15,7 +15,11 @@ func TestGenerateInstructions(t *testing.T) { { name: "empty toolsets", enabledToolsets: []string{}, - expectedEmpty: true, + expectedContains: []string{ + "GitHub MCP Server provides GitHub API tools", + "prefer 'search_*' tools over 'list_*' tools", + "context windows", + }, }, { name: "only context toolset", From bf3e5252ab13dcb04cd2c3f91634d7bdfcd26a01 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sat, 13 Sep 2025 20:03:48 -0700 Subject: [PATCH 03/11] Refactor base instruction for clarity and adjust context management instruction formatting --- pkg/github/instructions.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index f550a5cae..75376e7b3 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -35,11 +35,10 @@ func GenerateInstructions(enabledToolsets []string) string { instructions = append(instructions, "For repository operations: Check workflow status before major changes using 'list_workflow_runs'.") } - // Always add base instructions - they provide universal value - baseInstruction := "GitHub MCP Server provides GitHub API tools. Common parameters: 'owner' (repo owner), 'repo' (repo name), 'page'/'perPage' (pagination)." + baseInstruction := "The GitHub MCP Server provides GitHub API tools." // Add context management instruction for all configurations - contextInstruction := "GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." + contextInstruction := " GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." allInstructions := []string{baseInstruction, contextInstruction} allInstructions = append(allInstructions, instructions...) From 0cd74fcf92ae3ec3255dc5e581c75d48822b0888 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 07:35:01 -0700 Subject: [PATCH 04/11] Simplify changes for now --- pkg/github/instructions.go | 30 ++---------------- pkg/github/instructions_test.go | 55 +-------------------------------- 2 files changed, 4 insertions(+), 81 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 75376e7b3..d849b6089 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -19,28 +19,14 @@ func GenerateInstructions(enabledToolsets []string) string { } // Cross-toolset conditional instructions - if contains(enabledToolsets, "pull_requests") && contains(enabledToolsets, "context") { - instructions = append(instructions, "For team workflows: Use 'get_teams' and 'get_team_members' before assigning PR reviewers.") - } - - if hasAnySecurityToolset(enabledToolsets) { - instructions = append(instructions, "Security alert priority: secret_scanning → dependabot → code_scanning alerts.") - } - if contains(enabledToolsets, "issues") && contains(enabledToolsets, "pull_requests") { instructions = append(instructions, "Link issues to PRs using 'closes #123' or 'fixes #123' in PR descriptions.") } - if contains(enabledToolsets, "repos") && contains(enabledToolsets, "actions") { - instructions = append(instructions, "For repository operations: Check workflow status before major changes using 'list_workflow_runs'.") - } - - baseInstruction := "The GitHub MCP Server provides GitHub API tools." + // Base instruction with context management + baseInstruction := "The GitHub MCP Server provides GitHub API tools. GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." - // Add context management instruction for all configurations - contextInstruction := " GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." - - allInstructions := []string{baseInstruction, contextInstruction} + allInstructions := []string{baseInstruction} allInstructions = append(allInstructions, instructions...) return strings.Join(allInstructions, " ") @@ -68,16 +54,6 @@ func getToolsetInstructions(toolset string) string { } } -// hasAnySecurityToolset checks if any security-related toolsets are enabled -func hasAnySecurityToolset(toolsets []string) bool { - securityToolsets := []string{"code_security", "secret_protection", "dependabot"} - for _, security := range securityToolsets { - if contains(toolsets, security) { - return true - } - } - return false -} // contains checks if a slice contains a specific string func contains(slice []string, item string) bool { diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index 684b1e4f1..4f4713237 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -47,23 +47,12 @@ func TestGenerateInstructions(t *testing.T) { "rerun_failed_jobs", }, }, - { - name: "security toolsets", - enabledToolsets: []string{"code_security", "secret_protection", "dependabot"}, - expectedContains: []string{ - "Security alert priority", - "secret_scanning", - "dependabot", - "code_scanning", - }, - }, { name: "cross-toolset instructions", enabledToolsets: []string{"context", "pull_requests"}, expectedContains: []string{ "get_me", - "get_teams", - "get_team_members", + "create_pending_pull_request_review", }, }, { @@ -141,45 +130,3 @@ func TestGetToolsetInstructions(t *testing.T) { } } -func TestHasAnySecurityToolset(t *testing.T) { - tests := []struct { - name string - toolsets []string - expected bool - }{ - { - name: "no security toolsets", - toolsets: []string{"repos", "issues"}, - expected: false, - }, - { - name: "has code_security", - toolsets: []string{"repos", "code_security"}, - expected: true, - }, - { - name: "has secret_protection", - toolsets: []string{"secret_protection", "issues"}, - expected: true, - }, - { - name: "has dependabot", - toolsets: []string{"dependabot"}, - expected: true, - }, - { - name: "has all security toolsets", - toolsets: []string{"code_security", "secret_protection", "dependabot"}, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := hasAnySecurityToolset(tt.toolsets) - if result != tt.expected { - t.Errorf("Expected %v for toolsets %v, but got %v", tt.expected, tt.toolsets, result) - } - }) - } -} \ No newline at end of file From 1690c70fa6c2d435072c1bd3f9d5fb3dfea51f07 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 07:41:11 -0700 Subject: [PATCH 05/11] Remove unused toolset instructions and simplify test cases for clarity --- pkg/github/instructions.go | 14 +------------- pkg/github/instructions_test.go | 21 +-------------------- 2 files changed, 2 insertions(+), 33 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index d849b6089..f662dedbb 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -18,11 +18,6 @@ func GenerateInstructions(enabledToolsets []string) string { } } - // Cross-toolset conditional instructions - if contains(enabledToolsets, "issues") && contains(enabledToolsets, "pull_requests") { - instructions = append(instructions, "Link issues to PRs using 'closes #123' or 'fixes #123' in PR descriptions.") - } - // Base instruction with context management baseInstruction := "The GitHub MCP Server provides GitHub API tools. GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." @@ -37,16 +32,10 @@ func getToolsetInstructions(toolset string) string { switch toolset { case "pull_requests": return "PR review workflow: Use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." - case "actions": - return "CI/CD debugging: Use 'get_job_logs' with 'failed_only=true' and 'return_content=true' for immediate log analysis. Use 'rerun_failed_jobs' instead of 'rerun_workflow_run' to save resources." case "issues": return "Issue workflow: 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 "repos": - return "File operations: Use 'get_file_contents' to check if file exists before 'create_or_update_file'. Always specify 'sha' parameter when updating existing files. Use 'push_files' for multiple file operations in single commit." case "notifications": return "Notifications: Filter by 'participating' for issues/PRs you're involved in. Use 'mark_all_notifications_read' with repository filters to avoid marking unrelated notifications." - case "gists": - return "Gists: Use 'list_gists' with 'since' parameter to find recent gists. Specify 'public=false' for private gists in 'create_gist'." case "discussions": return "Discussions: Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization." default: @@ -54,7 +43,6 @@ func getToolsetInstructions(toolset string) string { } } - // contains checks if a slice contains a specific string func contains(slice []string, item string) bool { for _, s := range slice { @@ -63,4 +51,4 @@ func contains(slice []string, item string) bool { } } return false -} \ No newline at end of file +} diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index 4f4713237..98e7389f6 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -38,15 +38,6 @@ func TestGenerateInstructions(t *testing.T) { "submit_pending_pull_request_review", }, }, - { - name: "actions toolset", - enabledToolsets: []string{"actions"}, - expectedContains: []string{ - "get_job_logs", - "failed_only=true", - "rerun_failed_jobs", - }, - }, { name: "cross-toolset instructions", enabledToolsets: []string{"context", "pull_requests"}, @@ -59,10 +50,9 @@ func TestGenerateInstructions(t *testing.T) { name: "issues and pull_requests combination", enabledToolsets: []string{"issues", "pull_requests"}, expectedContains: []string{ - "closes #123", - "fixes #123", "search_issues", "list_issue_types", + "create_pending_pull_request_review", }, }, } @@ -96,18 +86,10 @@ func TestGetToolsetInstructions(t *testing.T) { toolset: "pull_requests", expected: "create_pending_pull_request_review", }, - { - toolset: "actions", - expected: "get_job_logs", - }, { toolset: "issues", expected: "list_issue_types", }, - { - toolset: "repos", - expected: "get_file_contents", - }, { toolset: "nonexistent", expected: "", @@ -129,4 +111,3 @@ func TestGetToolsetInstructions(t *testing.T) { }) } } - From 1f500abbf8429b3a5742a4bff8d4dcd0e0343020 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 07:44:29 -0700 Subject: [PATCH 06/11] Add test cases for issues, notifications, and discussions toolsets in instruction generation --- pkg/github/instructions_test.go | 38 +++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index 98e7389f6..a718198de 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -39,7 +39,33 @@ func TestGenerateInstructions(t *testing.T) { }, }, { - name: "cross-toolset instructions", + name: "issues toolset", + enabledToolsets: []string{"issues"}, + expectedContains: []string{ + "search_issues", + "list_issue_types", + "state_reason", + }, + }, + { + name: "notifications toolset", + enabledToolsets: []string{"notifications"}, + expectedContains: []string{ + "participating", + "mark_all_notifications_read", + "repository filters", + }, + }, + { + name: "discussions toolset", + enabledToolsets: []string{"discussions"}, + expectedContains: []string{ + "list_discussion_categories", + "Filter by category", + }, + }, + { + name: "multiple toolsets (context + pull_requests)", enabledToolsets: []string{"context", "pull_requests"}, expectedContains: []string{ "get_me", @@ -47,7 +73,7 @@ func TestGenerateInstructions(t *testing.T) { }, }, { - name: "issues and pull_requests combination", + name: "multiple toolsets (issues + pull_requests)", enabledToolsets: []string{"issues", "pull_requests"}, expectedContains: []string{ "search_issues", @@ -90,6 +116,14 @@ func TestGetToolsetInstructions(t *testing.T) { toolset: "issues", expected: "list_issue_types", }, + { + toolset: "notifications", + expected: "participating", + }, + { + toolset: "discussions", + expected: "list_discussion_categories", + }, { toolset: "nonexistent", expected: "", From fc3f378650c94229dce78df1b2457bdb73890a47 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 08:01:05 -0700 Subject: [PATCH 07/11] Update base instruction and test expectations for clarity on tool selection and context management --- pkg/github/instructions.go | 2 +- pkg/github/instructions_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index f662dedbb..158342c81 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -19,7 +19,7 @@ func GenerateInstructions(enabledToolsets []string) string { } // Base instruction with context management - baseInstruction := "The GitHub MCP Server provides GitHub API tools. GitHub API responses can overflow context windows. Strategy: 1) Always prefer 'search_*' tools over 'list_*' tools when possible - search tools return filtered results, 2) Process large datasets in batches rather than all at once, 3) For summarization tasks, fetch minimal data first, then drill down into specifics, 4) When analyzing multiple items (issues, PRs, etc), process in groups of 5-10 to manage context." + baseInstruction := "The GitHub MCP Server provides GitHub API tools. Tool selection guidance: 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. 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) GitHub API responses can overflow context windows, 2) Process large datasets in batches of 5-10 items, 3) For summarization tasks, fetch minimal data first, then drill down into specifics." allInstructions := []string{baseInstruction} allInstructions = append(allInstructions, instructions...) diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index a718198de..e4da6a419 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -17,7 +17,8 @@ func TestGenerateInstructions(t *testing.T) { enabledToolsets: []string{}, expectedContains: []string{ "GitHub MCP Server provides GitHub API tools", - "prefer 'search_*' tools over 'list_*' tools", + "Use 'list_*' tools for broad, simple retrieval", + "Use 'search_*' tools for targeted queries", "context windows", }, }, From 26951b5eb99938f3e095d299e4b70ed8097c449a Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 09:41:42 -0700 Subject: [PATCH 08/11] Add support for disabling instructions via environment variable --- pkg/github/instructions.go | 10 ++++- pkg/github/instructions_test.go | 74 +++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 158342c81..4f557dab1 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -1,9 +1,17 @@ package github -import "strings" +import ( + "os" + "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 // Core instruction - always included if context toolset enabled diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index e4da6a419..035179857 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -1,6 +1,7 @@ package github import ( + "os" "strings" "testing" ) @@ -104,6 +105,79 @@ func TestGenerateInstructions(t *testing.T) { } } +func TestGenerateInstructionsWithDisableFlag(t *testing.T) { + tests := []struct { + name string + disableEnvValue string + enabledToolsets []string + expectedEmpty bool + expectedContains []string + }{ + { + 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, + expectedContains: []string{ + "GitHub MCP Server provides GitHub API tools", + "Always call 'get_me' first", + }, + }, + { + name: "DISABLE_INSTRUCTIONS unset returns normal instructions", + disableEnvValue: "", + enabledToolsets: []string{"issues"}, + expectedEmpty: false, + expectedContains: []string{ + "GitHub MCP Server provides GitHub API tools", + "search_issues", + }, + }, + } + + 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) + } + return + } + + for _, expectedContent := range tt.expectedContains { + if !strings.Contains(result, expectedContent) { + t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result) + } + } + }) + } +} + func TestGetToolsetInstructions(t *testing.T) { tests := []struct { toolset string From e5645b9da934ce5afd8b2ea9139eeef6758a752c Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 14 Sep 2025 14:54:02 -0700 Subject: [PATCH 09/11] Clarify PR review workflow instruction for consistency --- pkg/github/instructions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 4f557dab1..6265c8245 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -39,7 +39,7 @@ func GenerateInstructions(enabledToolsets []string) string { func getToolsetInstructions(toolset string) string { switch toolset { case "pull_requests": - return "PR review workflow: Use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." + return "PR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." case "issues": return "Issue workflow: 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 "notifications": From c1386ffd7b2d21ff199c9c0bf8a8c2a327f51e24 Mon Sep 17 00:00:00 2001 From: Ola Hungerford Date: Mon, 22 Sep 2025 19:23:07 -0700 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Ksenia Bobrova --- pkg/github/instructions.go | 25 +++++++++++++++++++++---- pkg/github/instructions_test.go | 33 ++++++--------------------------- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 6265c8245..a33f8c0d4 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -27,7 +27,15 @@ func GenerateInstructions(enabledToolsets []string) string { } // Base instruction with context management - baseInstruction := "The GitHub MCP Server provides GitHub API tools. Tool selection guidance: 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. 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) GitHub API responses can overflow context windows, 2) Process large datasets in batches of 5-10 items, 3) For summarization tasks, fetch minimal data first, then drill down into specifics." + 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. allInstructions := []string{baseInstruction} allInstructions = append(allInstructions, instructions...) @@ -39,13 +47,22 @@ func GenerateInstructions(enabledToolsets []string) string { func getToolsetInstructions(toolset string) string { switch toolset { case "pull_requests": - return "PR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." + return `## Pull Requests + +PR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments. +` case "issues": - return "Issue workflow: 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." + return `## Issues + +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 "notifications": return "Notifications: Filter by 'participating' for issues/PRs you're involved in. Use 'mark_all_notifications_read' with repository filters to avoid marking unrelated notifications." case "discussions": - return "Discussions: Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization." + return `## Discussions + + Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization. +` default: return "" } diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index 035179857..de5088f47 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -34,53 +34,32 @@ func TestGenerateInstructions(t *testing.T) { { name: "pull requests toolset", enabledToolsets: []string{"pull_requests"}, - expectedContains: []string{ - "create_pending_pull_request_review", - "add_comment_to_pending_review", - "submit_pending_pull_request_review", - }, + expectedContains: []string{"## Pull Requests"}, }, { name: "issues toolset", enabledToolsets: []string{"issues"}, - expectedContains: []string{ - "search_issues", - "list_issue_types", - "state_reason", - }, - }, - { - name: "notifications toolset", - enabledToolsets: []string{"notifications"}, - expectedContains: []string{ - "participating", - "mark_all_notifications_read", - "repository filters", - }, + expectedContains: []string{"## Issues"}, }, { name: "discussions toolset", enabledToolsets: []string{"discussions"}, - expectedContains: []string{ - "list_discussion_categories", - "Filter by category", - }, + expectedContains: []string{"## Discussions"}, }, { name: "multiple toolsets (context + pull_requests)", enabledToolsets: []string{"context", "pull_requests"}, expectedContains: []string{ "get_me", - "create_pending_pull_request_review", + "## Pull Requests", }, }, { name: "multiple toolsets (issues + pull_requests)", enabledToolsets: []string{"issues", "pull_requests"}, expectedContains: []string{ - "search_issues", - "list_issue_types", - "create_pending_pull_request_review", + "## Issues", + "## Pull Requests", }, }, } From 914d48761b0beb917b5bffb1fb63105dc7db9b68 Mon Sep 17 00:00:00 2001 From: olaservo Date: Mon, 22 Sep 2025 20:32:32 -0700 Subject: [PATCH 11/11] Refactor instruction generation and testing for clarity and consistency --- pkg/github/instructions.go | 49 +++++---------- pkg/github/instructions_test.go | 103 +++++++++++--------------------- 2 files changed, 49 insertions(+), 103 deletions(-) diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index a33f8c0d4..3f72e707b 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -2,6 +2,7 @@ package github import ( "os" + "slices" "strings" ) @@ -15,7 +16,7 @@ func GenerateInstructions(enabledToolsets []string) string { var instructions []string // Core instruction - always included if context toolset enabled - if contains(enabledToolsets, "context") { + if slices.Contains(enabledToolsets, "context") { instructions = append(instructions, "Always call 'get_me' first to understand current user permissions and context.") } @@ -27,16 +28,16 @@ func GenerateInstructions(enabledToolsets []string) 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. - + 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.` + allInstructions := []string{baseInstruction} allInstructions = append(allInstructions, instructions...) @@ -47,33 +48,13 @@ Context management: func getToolsetInstructions(toolset string) string { switch toolset { case "pull_requests": - return `## Pull Requests - -PR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments. -` + return "## Pull Requests\n\nPR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." case "issues": - return `## Issues - -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 "notifications": - return "Notifications: Filter by 'participating' for issues/PRs you're involved in. Use 'mark_all_notifications_read' with repository filters to avoid marking unrelated notifications." + return "## Issues\n\nCheck '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. -` + return "## Discussions\n\nUse 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization." default: return "" } } -// contains checks if a slice contains a specific string -func contains(slice []string, item string) bool { - for _, s := range slice { - if s == item { - return true - } - } - return false -} diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index de5088f47..8450dc1a1 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -2,65 +2,49 @@ package github import ( "os" - "strings" "testing" ) func TestGenerateInstructions(t *testing.T) { tests := []struct { - name string - enabledToolsets []string - expectedContains []string - expectedEmpty bool + name string + enabledToolsets []string + expectedEmpty bool }{ { name: "empty toolsets", enabledToolsets: []string{}, - expectedContains: []string{ - "GitHub MCP Server provides GitHub API tools", - "Use 'list_*' tools for broad, simple retrieval", - "Use 'search_*' tools for targeted queries", - "context windows", - }, + expectedEmpty: false, }, { name: "only context toolset", enabledToolsets: []string{"context"}, - expectedContains: []string{ - "GitHub MCP Server provides GitHub API tools", - "Always call 'get_me' first", - }, + expectedEmpty: false, }, { name: "pull requests toolset", enabledToolsets: []string{"pull_requests"}, - expectedContains: []string{"## Pull Requests"}, + expectedEmpty: false, }, { name: "issues toolset", enabledToolsets: []string{"issues"}, - expectedContains: []string{"## Issues"}, + expectedEmpty: false, }, { name: "discussions toolset", enabledToolsets: []string{"discussions"}, - expectedContains: []string{"## Discussions"}, + expectedEmpty: false, }, { name: "multiple toolsets (context + pull_requests)", enabledToolsets: []string{"context", "pull_requests"}, - expectedContains: []string{ - "get_me", - "## Pull Requests", - }, + expectedEmpty: false, }, { name: "multiple toolsets (issues + pull_requests)", enabledToolsets: []string{"issues", "pull_requests"}, - expectedContains: []string{ - "## Issues", - "## Pull Requests", - }, + expectedEmpty: false, }, } @@ -72,12 +56,9 @@ func TestGenerateInstructions(t *testing.T) { if result != "" { t.Errorf("Expected empty instructions but got: %s", result) } - return - } - - for _, expectedContent := range tt.expectedContains { - if !strings.Contains(result, expectedContent) { - t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result) + } else { + if result == "" { + t.Errorf("Expected non-empty instructions but got empty result") } } }) @@ -86,11 +67,10 @@ func TestGenerateInstructions(t *testing.T) { func TestGenerateInstructionsWithDisableFlag(t *testing.T) { tests := []struct { - name string - disableEnvValue string - enabledToolsets []string - expectedEmpty bool - expectedContains []string + name string + disableEnvValue string + enabledToolsets []string + expectedEmpty bool }{ { name: "DISABLE_INSTRUCTIONS=true returns empty", @@ -103,20 +83,12 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) { disableEnvValue: "false", enabledToolsets: []string{"context"}, expectedEmpty: false, - expectedContains: []string{ - "GitHub MCP Server provides GitHub API tools", - "Always call 'get_me' first", - }, }, { name: "DISABLE_INSTRUCTIONS unset returns normal instructions", disableEnvValue: "", enabledToolsets: []string{"issues"}, expectedEmpty: false, - expectedContains: []string{ - "GitHub MCP Server provides GitHub API tools", - "search_issues", - }, }, } @@ -145,12 +117,9 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) { if result != "" { t.Errorf("Expected empty instructions but got: %s", result) } - return - } - - for _, expectedContent := range tt.expectedContains { - if !strings.Contains(result, expectedContent) { - t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result) + } else { + if result == "" { + t.Errorf("Expected non-empty instructions but got empty result") } } }) @@ -159,43 +128,39 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) { func TestGetToolsetInstructions(t *testing.T) { tests := []struct { - toolset string - expected string + toolset string + expectedEmpty bool }{ { - toolset: "pull_requests", - expected: "create_pending_pull_request_review", + toolset: "pull_requests", + expectedEmpty: false, }, { - toolset: "issues", - expected: "list_issue_types", + toolset: "issues", + expectedEmpty: false, }, { - toolset: "notifications", - expected: "participating", + toolset: "discussions", + expectedEmpty: false, }, { - toolset: "discussions", - expected: "list_discussion_categories", - }, - { - toolset: "nonexistent", - expected: "", + toolset: "nonexistent", + expectedEmpty: true, }, } for _, tt := range tests { t.Run(tt.toolset, func(t *testing.T) { result := getToolsetInstructions(tt.toolset) - if tt.expected == "" { + if tt.expectedEmpty { if result != "" { t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result) } } else { - if !strings.Contains(result, tt.expected) { - t.Errorf("Expected instructions for '%s' to contain '%s', but got: %s", tt.toolset, tt.expected, result) + if result == "" { + t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset) } } }) } -} +} \ No newline at end of file