refactor to use toolset instructions from new toolset metadata

tommaso-moro · Jan 22, 2026 · Jan 22, 2026 · Jan 22, 2026 · Jan 22, 2026 · Jan 22, 2026
commit 0c16e33286621e26b5f316211408d39e2ab6de40
diff --git a/pkg/inventory/instructions.go b/pkg/inventory/instructions.go
@@ -2,31 +2,18 @@ package inventory
 
 import (
 	"os"
-	"slices"
 	"strings"
 )
 
 // generateInstructions creates server instructions based on enabled toolsets
-func generateInstructions(enabledToolsets []ToolsetID) string {
+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
 
-	// Core instruction - always included if context toolset enabled
-	if slices.Contains(enabledToolsets, ToolsetID("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)
-		}
-	}
-
 	// Base instruction with context management
 	baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform.
 
@@ -41,103 +28,16 @@ Context management:
 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.`
 
-	allInstructions := []string{baseInstruction}
-	allInstructions = append(allInstructions, instructions...)
-
-	return strings.Join(allInstructions, " ")
-}
-
-// getToolsetInstructions returns specific instructions for individual toolsets
-func getToolsetInstructions(toolset ToolsetID, enabledToolsets []ToolsetID) string {
-	switch toolset {
-	case ToolsetID("pull_requests"):
-		pullRequestInstructions := `## 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, ToolsetID("repos")) {
-			pullRequestInstructions += `
+	instructions = append(instructions, baseInstruction)
 
-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.`
+	// 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 pullRequestInstructions
-	case ToolsetID("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 ToolsetID("discussions"):
-		return `## Discussions
-
-Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization.`
-	case ToolsetID("projects"):
-		return `## Projects
-
-Workflow: 1) list_project_fields (get field IDs), 2) list_project_items (with pagination), 3) optional updates.
-
-Field usage:
-	- Call list_project_fields first to understand available fields and get IDs/types before filtering.
-	- Use EXACT returned field names (case-insensitive match). Don't invent names or IDs.
-	- Iteration synonyms (sprint/cycle) only if that field exists; map to the actual name (e.g. sprint:@current).
-	- Only include filters for fields that exist and are relevant.
-
-Pagination (mandatory):
-	- Loop while pageInfo.hasNextPage=true using after=pageInfo.nextCursor.
-	- Keep query, fields, per_page IDENTICAL on every page.
-	- Use before=pageInfo.prevCursor only when explicitly navigating to a previous page.
-
-Counting rules:
-	- Count items array length after full pagination.
-	- Never count field objects, content, or nested arrays as separate items.
-
-Summary vs list:
-	- Summaries ONLY if user uses verbs: analyze | summarize | summary | report | overview | insights.
-	- Listing verbs (list/show/get/fetch/display/enumerate) → enumerate + total.
-
-Self-check before returning:
-	- Paginated fully
-	- Correct IDs used
-	- Field names valid
-	- Summary only if requested.
-
-Return COMPLETE data or state what's missing (e.g. pages skipped).
-
-list_project_items query rules:
-Query string - For advanced filtering of project items using GitHub's project filtering syntax:
-
-MUST reflect user intent; strongly prefer explicit content type if narrowed:
-	- "open issues" → state:open is:issue
-	- "merged PRs" → state:merged is:pr
-	- "items updated this week" → updated:>@today-7d (omit type only if mixed desired)
-	- "list all P1 priority items" → priority:p1 (omit state if user wants all, omit type if user specifies "items")
-	- "list all open P2 issues" → is:issue state:open priority:p2 (include state if user wants open or closed, include type if user specifies "issues" or "PRs")
-	- "all open issues I'm working on" → is:issue state:open assignee:@me
-
-Query Construction Heuristics:
-	a. Extract type nouns: issues → is:issue | PRs, Pulls, or Pull Requests → is:pr | tasks/tickets → is:issue (ask if ambiguity)
-	b. Map temporal phrases: "this week" → updated:>@today-7d
-	c. Map negations: "excluding wontfix" → -label:wontfix
-	d. Map priority adjectives: "high/sev1/p1" → priority:high OR priority:p1 (choose based on field presence)
-	e. When filtering by label, always use wildcard matching to account for cross-repository differences or emojis: (e.g. "bug 🐛" → label:*bug*)
-	f. When filtering by milestone, always use wildcard matching to account for cross-repository differences: (e.g. "v1.0" → milestone:*v1.0*)
-
-Syntax Essentials (items):
-   AND: space-separated. (label:bug priority:high).
-   OR: comma inside one qualifier (label:bug,critical).
-   NOT: leading '-' (-label:wontfix).
-   Hyphenate multi-word field names. (team-name:"Backend Team", story-points:>5).
-   Quote multi-word values. (status:"In Review" team-name:"Backend Team").
-   Ranges: points:1..3, updated:<@today-30d.
-   Wildcards: title:*crash*, label:bug*.
-   Assigned to User: assignee:@me | assignee:username | no:assignee
-
-Common Qualifier Glossary (items):
-   is:issue | is:pr | state:open|closed|merged | assignee:@me|username | label:NAME | status:VALUE |
-   priority:p1|high | sprint-name:@current | team-name:"Backend Team" | parent-issue:"org/repo#123" |
-   updated:>@today-7d | title:*text* | -label:wontfix | label:bug,critical | no:assignee | has:label
-
-Never:
-   - Infer field IDs; fetch via list_project_fields.
-   - Drop 'fields' param on subsequent pages if field values are needed.`
-	default:
-		return ""
 	}
+
+	return strings.Join(instructions, " ")
 }
diff --git a/pkg/inventory/instructions_test.go b/pkg/inventory/instructions_test.go
@@ -6,52 +6,61 @@ import (
 	"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
-		enabledToolsets []ToolsetID
-		expectedEmpty   bool
+		name          string
+		toolsets      []ToolsetMetadata
+		expectedEmpty bool
 	}{
 		{
-			name:            "empty toolsets",
-			enabledToolsets: []ToolsetID{},
-			expectedEmpty:   false,
+			name:          "empty toolsets",
+			toolsets:      []ToolsetMetadata{},
+			expectedEmpty: false, // base instructions are always included
 		},
 		{
-			name:            "only context toolset",
-			enabledToolsets: []ToolsetID{"context"},
-			expectedEmpty:   false,
-		},
-		{
-			name:            "pull requests toolset",
-			enabledToolsets: []ToolsetID{"pull_requests"},
-			expectedEmpty:   false,
-		},
-		{
-			name:            "issues toolset",
-			enabledToolsets: []ToolsetID{"issues"},
-			expectedEmpty:   false,
-		},
-		{
-			name:            "discussions toolset",
-			enabledToolsets: []ToolsetID{"discussions"},
-			expectedEmpty:   false,
-		},
-		{
-			name:            "multiple toolsets (context + pull_requests)",
-			enabledToolsets: []ToolsetID{"context", "pull_requests"},
-			expectedEmpty:   false,
+			name: "toolset with instructions",
+			toolsets: []ToolsetMetadata{
+				{
+					ID:          "test",
+					Description: "Test toolset",
+					InstructionsFunc: func(inv *Inventory) string {
+						return "Test instructions"
+					},
+				},
+			},
+			expectedEmpty: false,
 		},
 		{
-			name:            "multiple toolsets (issues + pull_requests)",
-			enabledToolsets: []ToolsetID{"issues", "pull_requests"},
-			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) {
-			result := generateInstructions(tt.enabledToolsets)
+			inv := createTestInventory(tt.toolsets)
+			result := generateInstructions(inv)
 
 			if tt.expectedEmpty {
 				if result != "" {
@@ -70,25 +79,21 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
 	tests := []struct {
 		name            string
 		disableEnvValue string
-		enabledToolsets []ToolsetID
 		expectedEmpty   bool
 	}{
 		{
 			name:            "DISABLE_INSTRUCTIONS=true returns empty",
 			disableEnvValue: "true",
-			enabledToolsets: []ToolsetID{"context", "issues", "pull_requests"},
 			expectedEmpty:   true,
 		},
 		{
 			name:            "DISABLE_INSTRUCTIONS=false returns normal instructions",
 			disableEnvValue: "false",
-			enabledToolsets: []ToolsetID{"context"},
 			expectedEmpty:   false,
 		},
 		{
 			name:            "DISABLE_INSTRUCTIONS unset returns normal instructions",
 			disableEnvValue: "",
-			enabledToolsets: []ToolsetID{"issues"},
 			expectedEmpty:   false,
 		},
 	}
@@ -112,7 +117,10 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
 				os.Setenv("DISABLE_INSTRUCTIONS", tt.disableEnvValue)
 			}
 
-			result := generateInstructions(tt.enabledToolsets)
+			inv := createTestInventory([]ToolsetMetadata{
+				{ID: "test", Description: "Test"},
+			})
+			result := generateInstructions(inv)
 
 			if tt.expectedEmpty {
 				if result != "" {
@@ -127,59 +135,68 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
 	}
 }
 
-func TestGetToolsetInstructions(t *testing.T) {
+func TestToolsetInstructionsFunc(t *testing.T) {
 	tests := []struct {
-		toolset              string
-		expectedEmpty        bool
-		enabledToolsets      []ToolsetID
+		name                 string
+		toolsets             []ToolsetMetadata
 		expectedToContain    string
 		notExpectedToContain string
 	}{
 		{
-			toolset:           "pull_requests",
-			expectedEmpty:     false,
-			enabledToolsets:   []ToolsetID{"pull_requests", "repos"},
-			expectedToContain: "pull_request_template.md",
-		},
-		{
-			toolset:              "pull_requests",
-			expectedEmpty:        false,
-			enabledToolsets:      []ToolsetID{"pull_requests"},
-			notExpectedToContain: "pull_request_template.md",
-		},
-		{
-			toolset:       "issues",
-			expectedEmpty: false,
+			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",
 		},
 		{
-			toolset:       "discussions",
-			expectedEmpty: false,
+			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",
 		},
 		{
-			toolset:       "nonexistent",
-			expectedEmpty: true,
+			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.toolset, func(t *testing.T) {
-			result := getToolsetInstructions(ToolsetID(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)
-				}
-			}
+		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' for toolset '%s', but it did not. Result: %s", tt.expectedToContain, tt.toolset, result)
+				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' for toolset '%s', but it did. Result: %s", tt.notExpectedToContain, tt.toolset, result)
+				t.Errorf("Did not expect result to contain '%s', but it did. Result: %s", tt.notExpectedToContain, result)
 			}
 		})
 	}