Add server instructions based on toolsets (#1091)

* Add instruction generation for enabled toolsets and corresponding tests

* Refactor instruction generation to always include base and context management instructions

* Refactor base instruction for clarity and adjust context management instruction formatting

* Simplify changes for now

* Remove unused toolset instructions and simplify test cases for clarity

* Add test cases for issues, notifications, and discussions toolsets in instruction generation

* Update base instruction and test expectations for clarity on tool selection and context management

* Add support for disabling instructions via environment variable

* Clarify PR review workflow instruction for consistency

* Apply suggestions from code review

Co-authored-by: Ksenia Bobrova <[email protected]>

* Refactor instruction generation and testing for clarity and consistency

---------

Co-authored-by: Ksenia Bobrova <[email protected]>

Author: olaservo

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,14 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
 		}
 	}
 
+	// Generate instructions based on enabled toolsets
+	instructions := github.GenerateInstructions(enabledToolsets)
+	
+	ghServer := github.NewServer(cfg.Version, 
+		server.WithInstructions(instructions),
+		server.WithHooks(hooks),
+	)
+
 	getClient := func(_ context.Context) (*gogithub.Client, error) {
 		return restClient, nil // closing over client
 	}

pkg/github/instructions.go

@@ -0,0 +1,60 @@
+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
+	
+	// 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); inst != "" {
+			instructions = append(instructions, inst)
+		}
+	}
+	
+	// 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.`
+
+	allInstructions := []string{baseInstruction}
+	allInstructions = append(allInstructions, instructions...)
+	
+	return strings.Join(allInstructions, " ")
+}
+
+// getToolsetInstructions returns specific instructions for individual toolsets
+func getToolsetInstructions(toolset string) string {
+	switch toolset {
+	case "pull_requests":
+		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\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\n\nUse 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization."
+	default:
+		return ""
+	}
+}
+

pkg/github/instructions_test.go

@@ -0,0 +1,166 @@
+package github
+
+import (
+	"os"
+	"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
+	}{
+		{
+			toolset:       "pull_requests",
+			expectedEmpty: false,
+		},
+		{
+			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)
+			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)
+				}
+			}
+		})
+	}
+}