Skip to content

Commit 0c16e33

Browse files
tommaso-morotommaso-moro
committed
refactor to use toolset instructions from new toolset metadata
1 parent e743cf1 commit 0c16e33

File tree

2 files changed

+100
-183
lines changed

2 files changed

+100
-183
lines changed

‎pkg/inventory/instructions.go‎

Lines changed: 10 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -2,31 +2,18 @@ package inventory
22

33
import (
44
"os"
5-
"slices"
65
"strings"
76
)
87

98
// generateInstructions creates server instructions based on enabled toolsets
10-
func generateInstructions(enabledToolsets []ToolsetID) string {
9+
func generateInstructions(inv *Inventory) string {
1110
// For testing - add a flag to disable instructions
1211
if os.Getenv("DISABLE_INSTRUCTIONS") == "true" {
1312
return "" // Baseline mode
1413
}
1514

1615
var instructions []string
1716

18-
// Core instruction - always included if context toolset enabled
19-
if slices.Contains(enabledToolsets, ToolsetID("context")) {
20-
instructions = append(instructions, "Always call 'get_me' first to understand current user permissions and context.")
21-
}
22-
23-
// Individual toolset instructions
24-
for _, toolset := range enabledToolsets {
25-
if inst := getToolsetInstructions(toolset, enabledToolsets); inst != "" {
26-
instructions = append(instructions, inst)
27-
}
28-
}
29-
3017
// Base instruction with context management
3118
baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform.
3219
@@ -41,103 +28,16 @@ Context management:
4128
Tool usage guidance:
4229
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.`
4330

44-
allInstructions := []string{baseInstruction}
45-
allInstructions = append(allInstructions, instructions...)
46-
47-
return strings.Join(allInstructions, " ")
48-
}
49-
50-
// getToolsetInstructions returns specific instructions for individual toolsets
51-
func getToolsetInstructions(toolset ToolsetID, enabledToolsets []ToolsetID) string {
52-
switch toolset {
53-
case ToolsetID("pull_requests"):
54-
pullRequestInstructions := `## Pull Requests
55-
56-
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.`
57-
if slices.Contains(enabledToolsets, ToolsetID("repos")) {
58-
pullRequestInstructions += `
31+
instructions = append(instructions, baseInstruction)
5932

60-
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.`
33+
// Collect instructions from each enabled toolset
34+
for _, toolset := range inv.AvailableToolsets() {
35+
if toolset.InstructionsFunc != nil {
36+
if toolsetInstructions := toolset.InstructionsFunc(inv); toolsetInstructions != "" {
37+
instructions = append(instructions, toolsetInstructions)
38+
}
6139
}
62-
return pullRequestInstructions
63-
case ToolsetID("issues"):
64-
return `## Issues
65-
66-
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.`
67-
case ToolsetID("discussions"):
68-
return `## Discussions
69-
70-
Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization.`
71-
case ToolsetID("projects"):
72-
return `## Projects
73-
74-
Workflow: 1) list_project_fields (get field IDs), 2) list_project_items (with pagination), 3) optional updates.
75-
76-
Field usage:
77-
- Call list_project_fields first to understand available fields and get IDs/types before filtering.
78-
- Use EXACT returned field names (case-insensitive match). Don't invent names or IDs.
79-
- Iteration synonyms (sprint/cycle) only if that field exists; map to the actual name (e.g. sprint:@current).
80-
- Only include filters for fields that exist and are relevant.
81-
82-
Pagination (mandatory):
83-
- Loop while pageInfo.hasNextPage=true using after=pageInfo.nextCursor.
84-
- Keep query, fields, per_page IDENTICAL on every page.
85-
- Use before=pageInfo.prevCursor only when explicitly navigating to a previous page.
86-
87-
Counting rules:
88-
- Count items array length after full pagination.
89-
- Never count field objects, content, or nested arrays as separate items.
90-
91-
Summary vs list:
92-
- Summaries ONLY if user uses verbs: analyze | summarize | summary | report | overview | insights.
93-
- Listing verbs (list/show/get/fetch/display/enumerate) → enumerate + total.
94-
95-
Self-check before returning:
96-
- Paginated fully
97-
- Correct IDs used
98-
- Field names valid
99-
- Summary only if requested.
100-
101-
Return COMPLETE data or state what's missing (e.g. pages skipped).
102-
103-
list_project_items query rules:
104-
Query string - For advanced filtering of project items using GitHub's project filtering syntax:
105-
106-
MUST reflect user intent; strongly prefer explicit content type if narrowed:
107-
- "open issues" → state:open is:issue
108-
- "merged PRs" → state:merged is:pr
109-
- "items updated this week" → updated:>@today-7d (omit type only if mixed desired)
110-
- "list all P1 priority items" → priority:p1 (omit state if user wants all, omit type if user specifies "items")
111-
- "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")
112-
- "all open issues I'm working on" → is:issue state:open assignee:@me
113-
114-
Query Construction Heuristics:
115-
a. Extract type nouns: issues → is:issue | PRs, Pulls, or Pull Requests → is:pr | tasks/tickets → is:issue (ask if ambiguity)
116-
b. Map temporal phrases: "this week" → updated:>@today-7d
117-
c. Map negations: "excluding wontfix" → -label:wontfix
118-
d. Map priority adjectives: "high/sev1/p1" → priority:high OR priority:p1 (choose based on field presence)
119-
e. When filtering by label, always use wildcard matching to account for cross-repository differences or emojis: (e.g. "bug 🐛" → label:*bug*)
120-
f. When filtering by milestone, always use wildcard matching to account for cross-repository differences: (e.g. "v1.0" → milestone:*v1.0*)
121-
122-
Syntax Essentials (items):
123-
AND: space-separated. (label:bug priority:high).
124-
OR: comma inside one qualifier (label:bug,critical).
125-
NOT: leading '-' (-label:wontfix).
126-
Hyphenate multi-word field names. (team-name:"Backend Team", story-points:>5).
127-
Quote multi-word values. (status:"In Review" team-name:"Backend Team").
128-
Ranges: points:1..3, updated:<@today-30d.
129-
Wildcards: title:*crash*, label:bug*.
130-
Assigned to User: assignee:@me | assignee:username | no:assignee
131-
132-
Common Qualifier Glossary (items):
133-
is:issue | is:pr | state:open|closed|merged | assignee:@me|username | label:NAME | status:VALUE |
134-
priority:p1|high | sprint-name:@current | team-name:"Backend Team" | parent-issue:"org/repo#123" |
135-
updated:>@today-7d | title:*text* | -label:wontfix | label:bug,critical | no:assignee | has:label
136-
137-
Never:
138-
- Infer field IDs; fetch via list_project_fields.
139-
- Drop 'fields' param on subsequent pages if field values are needed.`
140-
default:
141-
return ""
14240
}
41+
42+
return strings.Join(instructions, " ")
14343
}

‎pkg/inventory/instructions_test.go‎

Lines changed: 90 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -6,52 +6,61 @@ import (
66
"testing"
77
)
88

9+
// createTestInventory creates an inventory with the specified toolsets for testing.
10+
func createTestInventory(toolsets []ToolsetMetadata) *Inventory {
11+
// Create tools for each toolset so they show up in AvailableToolsets()
12+
var tools []ServerTool
13+
for _, ts := range toolsets {
14+
tools = append(tools, ServerTool{
15+
Toolset: ts,
16+
})
17+
}
18+
19+
return NewBuilder().
20+
SetTools(tools).
21+
Build()
22+
}
23+
924
func TestGenerateInstructions(t *testing.T) {
1025
tests := []struct {
11-
name string
12-
enabledToolsets []ToolsetID
13-
expectedEmpty bool
26+
name string
27+
toolsets []ToolsetMetadata
28+
expectedEmpty bool
1429
}{
1530
{
16-
name: "empty toolsets",
17-
enabledToolsets: []ToolsetID{},
18-
expectedEmpty: false,
31+
name: "empty toolsets",
32+
toolsets: []ToolsetMetadata{},
33+
expectedEmpty: false, // base instructions are always included
1934
},
2035
{
21-
name: "only context toolset",
22-
enabledToolsets: []ToolsetID{"context"},
23-
expectedEmpty: false,
24-
},
25-
{
26-
name: "pull requests toolset",
27-
enabledToolsets: []ToolsetID{"pull_requests"},
28-
expectedEmpty: false,
29-
},
30-
{
31-
name: "issues toolset",
32-
enabledToolsets: []ToolsetID{"issues"},
33-
expectedEmpty: false,
34-
},
35-
{
36-
name: "discussions toolset",
37-
enabledToolsets: []ToolsetID{"discussions"},
38-
expectedEmpty: false,
39-
},
40-
{
41-
name: "multiple toolsets (context + pull_requests)",
42-
enabledToolsets: []ToolsetID{"context", "pull_requests"},
43-
expectedEmpty: false,
36+
name: "toolset with instructions",
37+
toolsets: []ToolsetMetadata{
38+
{
39+
ID: "test",
40+
Description: "Test toolset",
41+
InstructionsFunc: func(inv *Inventory) string {
42+
return "Test instructions"
43+
},
44+
},
45+
},
46+
expectedEmpty: false,
4447
},
4548
{
46-
name: "multiple toolsets (issues + pull_requests)",
47-
enabledToolsets: []ToolsetID{"issues", "pull_requests"},
48-
expectedEmpty: false,
49+
name: "toolset without instructions",
50+
toolsets: []ToolsetMetadata{
51+
{
52+
ID: "test",
53+
Description: "Test toolset",
54+
},
55+
},
56+
expectedEmpty: false, // base instructions still included
4957
},
5058
}
5159

5260
for _, tt := range tests {
5361
t.Run(tt.name, func(t *testing.T) {
54-
result := generateInstructions(tt.enabledToolsets)
62+
inv := createTestInventory(tt.toolsets)
63+
result := generateInstructions(inv)
5564

5665
if tt.expectedEmpty {
5766
if result != "" {
@@ -70,25 +79,21 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
7079
tests := []struct {
7180
name string
7281
disableEnvValue string
73-
enabledToolsets []ToolsetID
7482
expectedEmpty bool
7583
}{
7684
{
7785
name: "DISABLE_INSTRUCTIONS=true returns empty",
7886
disableEnvValue: "true",
79-
enabledToolsets: []ToolsetID{"context", "issues", "pull_requests"},
8087
expectedEmpty: true,
8188
},
8289
{
8390
name: "DISABLE_INSTRUCTIONS=false returns normal instructions",
8491
disableEnvValue: "false",
85-
enabledToolsets: []ToolsetID{"context"},
8692
expectedEmpty: false,
8793
},
8894
{
8995
name: "DISABLE_INSTRUCTIONS unset returns normal instructions",
9096
disableEnvValue: "",
91-
enabledToolsets: []ToolsetID{"issues"},
9297
expectedEmpty: false,
9398
},
9499
}
@@ -112,7 +117,10 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
112117
os.Setenv("DISABLE_INSTRUCTIONS", tt.disableEnvValue)
113118
}
114119

115-
result := generateInstructions(tt.enabledToolsets)
120+
inv := createTestInventory([]ToolsetMetadata{
121+
{ID: "test", Description: "Test"},
122+
})
123+
result := generateInstructions(inv)
116124

117125
if tt.expectedEmpty {
118126
if result != "" {
@@ -127,59 +135,68 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
127135
}
128136
}
129137

130-
func TestGetToolsetInstructions(t *testing.T) {
138+
func TestToolsetInstructionsFunc(t *testing.T) {
131139
tests := []struct {
132-
toolset string
133-
expectedEmpty bool
134-
enabledToolsets []ToolsetID
140+
name string
141+
toolsets []ToolsetMetadata
135142
expectedToContain string
136143
notExpectedToContain string
137144
}{
138145
{
139-
toolset: "pull_requests",
140-
expectedEmpty: false,
141-
enabledToolsets: []ToolsetID{"pull_requests", "repos"},
142-
expectedToContain: "pull_request_template.md",
143-
},
144-
{
145-
toolset: "pull_requests",
146-
expectedEmpty: false,
147-
enabledToolsets: []ToolsetID{"pull_requests"},
148-
notExpectedToContain: "pull_request_template.md",
149-
},
150-
{
151-
toolset: "issues",
152-
expectedEmpty: false,
146+
name: "toolset with context-aware instructions includes extra text when dependency present",
147+
toolsets: []ToolsetMetadata{
148+
{ID: "repos", Description: "Repos"},
149+
{
150+
ID: "pull_requests",
151+
Description: "PRs",
152+
InstructionsFunc: func(inv *Inventory) string {
153+
instructions := "PR base instructions"
154+
if inv.HasToolset("repos") {
155+
instructions += " PR template instructions"
156+
}
157+
return instructions
158+
},
159+
},
160+
},
161+
expectedToContain: "PR template instructions",
153162
},
154163
{
155-
toolset: "discussions",
156-
expectedEmpty: false,
164+
name: "toolset with context-aware instructions excludes extra text when dependency missing",
165+
toolsets: []ToolsetMetadata{
166+
{
167+
ID: "pull_requests",
168+
Description: "PRs",
169+
InstructionsFunc: func(inv *Inventory) string {
170+
instructions := "PR base instructions"
171+
if inv.HasToolset("repos") {
172+
instructions += " PR template instructions"
173+
}
174+
return instructions
175+
},
176+
},
177+
},
178+
notExpectedToContain: "PR template instructions",
157179
},
158180
{
159-
toolset: "nonexistent",
160-
expectedEmpty: true,
181+
name: "toolset without InstructionsFunc returns no toolset-specific instructions",
182+
toolsets: []ToolsetMetadata{
183+
{ID: "test", Description: "Test without instructions"},
184+
},
185+
notExpectedToContain: "## Test",
161186
},
162187
}
163188

164189
for _, tt := range tests {
165-
t.Run(tt.toolset, func(t *testing.T) {
166-
result := getToolsetInstructions(ToolsetID(tt.toolset), tt.enabledToolsets)
167-
if tt.expectedEmpty {
168-
if result != "" {
169-
t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result)
170-
}
171-
} else {
172-
if result == "" {
173-
t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset)
174-
}
175-
}
190+
t.Run(tt.name, func(t *testing.T) {
191+
inv := createTestInventory(tt.toolsets)
192+
result := generateInstructions(inv)
176193

177194
if tt.expectedToContain != "" && !strings.Contains(result, tt.expectedToContain) {
178-
t.Errorf("Expected result to contain '%s' for toolset '%s', but it did not. Result: %s", tt.expectedToContain, tt.toolset, result)
195+
t.Errorf("Expected result to contain '%s', but it did not. Result: %s", tt.expectedToContain, result)
179196
}
180197

181198
if tt.notExpectedToContain != "" && strings.Contains(result, tt.notExpectedToContain) {
182-
t.Errorf("Did not expect result to contain '%s' for toolset '%s', but it did. Result: %s", tt.notExpectedToContain, tt.toolset, result)
199+
t.Errorf("Did not expect result to contain '%s', but it did. Result: %s", tt.notExpectedToContain, result)
183200
}
184201
})
185202
}

0 commit comments

Comments
 (0)