Skip to content

Commit 4ff8c21

Browse files
Fix generateInstructions to use only enabled toolsets
Previously, generateInstructions() iterated over AvailableToolsets() which returns all toolsets that have tools defined, rather than only the enabled toolsets based on WithToolsets() configuration. This caused instructions for all toolsets to be included regardless of which toolsets were actually enabled, leading to bloated instructions (e.g., 5886 chars vs 1226 chars when only 'repos' toolset is enabled). Changes: - Add EnabledToolsets() method to return only enabled toolset metadata - Update generateInstructions() to use EnabledToolsets() - Add regression test for the fix Fixes #1897
1 parent a4670b0 commit 4ff8c21

File tree

3 files changed

+83
-1
lines changed

3 files changed

+83
-1
lines changed

pkg/inventory/instructions.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Tool usage guidance:
3131
instructions = append(instructions, baseInstruction)
3232

3333
// Collect instructions from each enabled toolset
34-
for _, toolset := range inv.AvailableToolsets() {
34+
for _, toolset := range inv.EnabledToolsets() {
3535
if toolset.InstructionsFunc != nil {
3636
if toolsetInstructions := toolset.InstructionsFunc(inv); toolsetInstructions != "" {
3737
instructions = append(instructions, toolsetInstructions)

pkg/inventory/instructions_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
)
88

99
// createTestInventory creates an inventory with the specified toolsets for testing.
10+
// All toolsets are enabled by default using WithToolsets([]string{"all"}).
1011
func createTestInventory(toolsets []ToolsetMetadata) *Inventory {
1112
// Create tools for each toolset so they show up in AvailableToolsets()
1213
var tools []ServerTool
@@ -18,6 +19,7 @@ func createTestInventory(toolsets []ToolsetMetadata) *Inventory {
1819

1920
inv, _ := NewBuilder().
2021
SetTools(tools).
22+
WithToolsets([]string{"all"}).
2123
Build()
2224

2325
return inv
@@ -203,3 +205,61 @@ func TestToolsetInstructionsFunc(t *testing.T) {
203205
})
204206
}
205207
}
208+
209+
// TestGenerateInstructionsOnlyEnabledToolsets verifies that generateInstructions
210+
// only includes instructions from enabled toolsets, not all available toolsets.
211+
// This is a regression test for https://github.com/github/github-mcp-server/issues/1897
212+
func TestGenerateInstructionsOnlyEnabledToolsets(t *testing.T) {
213+
// Create tools for multiple toolsets
214+
reposToolset := ToolsetMetadata{
215+
ID: "repos",
216+
Description: "Repository tools",
217+
InstructionsFunc: func(_ *Inventory) string {
218+
return "REPOS_INSTRUCTIONS"
219+
},
220+
}
221+
issuesToolset := ToolsetMetadata{
222+
ID: "issues",
223+
Description: "Issue tools",
224+
InstructionsFunc: func(_ *Inventory) string {
225+
return "ISSUES_INSTRUCTIONS"
226+
},
227+
}
228+
prsToolset := ToolsetMetadata{
229+
ID: "pull_requests",
230+
Description: "PR tools",
231+
InstructionsFunc: func(_ *Inventory) string {
232+
return "PRS_INSTRUCTIONS"
233+
},
234+
}
235+
236+
tools := []ServerTool{
237+
{Toolset: reposToolset},
238+
{Toolset: issuesToolset},
239+
{Toolset: prsToolset},
240+
}
241+
242+
// Build inventory with only "repos" toolset enabled
243+
inv, err := NewBuilder().
244+
SetTools(tools).
245+
WithToolsets([]string{"repos"}).
246+
Build()
247+
if err != nil {
248+
t.Fatalf("Failed to build inventory: %v", err)
249+
}
250+
251+
result := generateInstructions(inv)
252+
253+
// Should contain instructions from enabled toolset
254+
if !strings.Contains(result, "REPOS_INSTRUCTIONS") {
255+
t.Errorf("Expected instructions to contain 'REPOS_INSTRUCTIONS' for enabled toolset, but it did not. Result: %s", result)
256+
}
257+
258+
// Should NOT contain instructions from non-enabled toolsets
259+
if strings.Contains(result, "ISSUES_INSTRUCTIONS") {
260+
t.Errorf("Did not expect instructions to contain 'ISSUES_INSTRUCTIONS' for disabled toolset, but it did. Result: %s", result)
261+
}
262+
if strings.Contains(result, "PRS_INSTRUCTIONS") {
263+
t.Errorf("Did not expect instructions to contain 'PRS_INSTRUCTIONS' for disabled toolset, but it did. Result: %s", result)
264+
}
265+
}

pkg/inventory/registry.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,28 @@ func (r *Inventory) AvailableToolsets(exclude ...ToolsetID) []ToolsetMetadata {
295295
return result
296296
}
297297

298+
// EnabledToolsets returns the unique toolsets that are enabled based on current filters.
299+
// This is similar to AvailableToolsets but respects the enabledToolsets filter.
300+
// Returns toolsets in sorted order by toolset ID.
301+
func (r *Inventory) EnabledToolsets() []ToolsetMetadata {
302+
// Get all available toolsets first (already sorted by ID)
303+
allToolsets := r.AvailableToolsets()
304+
305+
// If no filter is set, all toolsets are enabled
306+
if r.enabledToolsets == nil {
307+
return allToolsets
308+
}
309+
310+
// Filter to only enabled toolsets
311+
var result []ToolsetMetadata
312+
for _, ts := range allToolsets {
313+
if r.enabledToolsets[ts.ID] {
314+
result = append(result, ts)
315+
}
316+
}
317+
return result
318+
}
319+
298320
func (r *Inventory) Instructions() string {
299321
return r.instructions
300322
}

0 commit comments

Comments
 (0)