Skip to content

Commit e5d69a5

Browse files
olaservoalmaleksia
olaservo
authored and
almaleksia
committed
Refactor instruction generation and testing for clarity and consistency
1 parent 64304bc commit e5d69a5

File tree

2 files changed

+49
-103
lines changed

2 files changed

+49
-103
lines changed

‎pkg/github/instructions.go‎

Lines changed: 15 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package github
22

33
import (
44
"os"
5+
"slices"
56
"strings"
67
)
78

@@ -15,7 +16,7 @@ func GenerateInstructions(enabledToolsets []string) string {
1516
var instructions []string
1617

1718
// Core instruction - always included if context toolset enabled
18-
if contains(enabledToolsets, "context") {
19+
if slices.Contains(enabledToolsets, "context") {
1920
instructions = append(instructions, "Always call 'get_me' first to understand current user permissions and context.")
2021
}
2122

@@ -27,16 +28,16 @@ func GenerateInstructions(enabledToolsets []string) string {
2728
}
2829

2930
// Base instruction with context management
30-
baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform.
31-
32-
Tool selection guidance:
33-
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.
34-
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).
35-
36-
Context management:
37-
1. Use pagination whenever possible with batches of 5-10 items.
38-
2. Use minimal_output parameter set to true if the full information is not needed to accomplish a task.
39-
31+
baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform.
32+
33+
Tool selection guidance:
34+
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.
35+
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).
36+
37+
Context management:
38+
1. Use pagination whenever possible with batches of 5-10 items.
39+
2. Use minimal_output parameter set to true if the full information is not needed to accomplish a task.`
40+
4041
allInstructions := []string{baseInstruction}
4142
allInstructions = append(allInstructions, instructions...)
4243

@@ -47,33 +48,13 @@ Context management:
4748
func getToolsetInstructions(toolset string) string {
4849
switch toolset {
4950
case "pull_requests":
50-
return `## Pull Requests
51-
52-
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.
53-
`
51+
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."
5452
case "issues":
55-
return `## Issues
56-
57-
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.
58-
`
59-
case "notifications":
60-
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."
53+
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."
6154
case "discussions":
62-
return `## Discussions
63-
64-
Use 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization.
65-
`
55+
return "## Discussions\n\nUse 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization."
6656
default:
6757
return ""
6858
}
6959
}
7060

71-
// contains checks if a slice contains a specific string
72-
func contains(slice []string, item string) bool {
73-
for _, s := range slice {
74-
if s == item {
75-
return true
76-
}
77-
}
78-
return false
79-
}

‎pkg/github/instructions_test.go‎

Lines changed: 34 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -2,65 +2,49 @@ package github
22

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

98
func TestGenerateInstructions(t *testing.T) {
109
tests := []struct {
11-
name string
12-
enabledToolsets []string
13-
expectedContains []string
14-
expectedEmpty bool
10+
name string
11+
enabledToolsets []string
12+
expectedEmpty bool
1513
}{
1614
{
1715
name: "empty toolsets",
1816
enabledToolsets: []string{},
19-
expectedContains: []string{
20-
"GitHub MCP Server provides GitHub API tools",
21-
"Use 'list_*' tools for broad, simple retrieval",
22-
"Use 'search_*' tools for targeted queries",
23-
"context windows",
24-
},
17+
expectedEmpty: false,
2518
},
2619
{
2720
name: "only context toolset",
2821
enabledToolsets: []string{"context"},
29-
expectedContains: []string{
30-
"GitHub MCP Server provides GitHub API tools",
31-
"Always call 'get_me' first",
32-
},
22+
expectedEmpty: false,
3323
},
3424
{
3525
name: "pull requests toolset",
3626
enabledToolsets: []string{"pull_requests"},
37-
expectedContains: []string{"## Pull Requests"},
27+
expectedEmpty: false,
3828
},
3929
{
4030
name: "issues toolset",
4131
enabledToolsets: []string{"issues"},
42-
expectedContains: []string{"## Issues"},
32+
expectedEmpty: false,
4333
},
4434
{
4535
name: "discussions toolset",
4636
enabledToolsets: []string{"discussions"},
47-
expectedContains: []string{"## Discussions"},
37+
expectedEmpty: false,
4838
},
4939
{
5040
name: "multiple toolsets (context + pull_requests)",
5141
enabledToolsets: []string{"context", "pull_requests"},
52-
expectedContains: []string{
53-
"get_me",
54-
"## Pull Requests",
55-
},
42+
expectedEmpty: false,
5643
},
5744
{
5845
name: "multiple toolsets (issues + pull_requests)",
5946
enabledToolsets: []string{"issues", "pull_requests"},
60-
expectedContains: []string{
61-
"## Issues",
62-
"## Pull Requests",
63-
},
47+
expectedEmpty: false,
6448
},
6549
}
6650

@@ -72,12 +56,9 @@ func TestGenerateInstructions(t *testing.T) {
7256
if result != "" {
7357
t.Errorf("Expected empty instructions but got: %s", result)
7458
}
75-
return
76-
}
77-
78-
for _, expectedContent := range tt.expectedContains {
79-
if !strings.Contains(result, expectedContent) {
80-
t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result)
59+
} else {
60+
if result == "" {
61+
t.Errorf("Expected non-empty instructions but got empty result")
8162
}
8263
}
8364
})
@@ -86,11 +67,10 @@ func TestGenerateInstructions(t *testing.T) {
8667

8768
func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
8869
tests := []struct {
89-
name string
90-
disableEnvValue string
91-
enabledToolsets []string
92-
expectedEmpty bool
93-
expectedContains []string
70+
name string
71+
disableEnvValue string
72+
enabledToolsets []string
73+
expectedEmpty bool
9474
}{
9575
{
9676
name: "DISABLE_INSTRUCTIONS=true returns empty",
@@ -103,20 +83,12 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
10383
disableEnvValue: "false",
10484
enabledToolsets: []string{"context"},
10585
expectedEmpty: false,
106-
expectedContains: []string{
107-
"GitHub MCP Server provides GitHub API tools",
108-
"Always call 'get_me' first",
109-
},
11086
},
11187
{
11288
name: "DISABLE_INSTRUCTIONS unset returns normal instructions",
11389
disableEnvValue: "",
11490
enabledToolsets: []string{"issues"},
11591
expectedEmpty: false,
116-
expectedContains: []string{
117-
"GitHub MCP Server provides GitHub API tools",
118-
"search_issues",
119-
},
12092
},
12193
}
12294

@@ -145,12 +117,9 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
145117
if result != "" {
146118
t.Errorf("Expected empty instructions but got: %s", result)
147119
}
148-
return
149-
}
150-
151-
for _, expectedContent := range tt.expectedContains {
152-
if !strings.Contains(result, expectedContent) {
153-
t.Errorf("Expected instructions to contain '%s', but got: %s", expectedContent, result)
120+
} else {
121+
if result == "" {
122+
t.Errorf("Expected non-empty instructions but got empty result")
154123
}
155124
}
156125
})
@@ -159,43 +128,39 @@ func TestGenerateInstructionsWithDisableFlag(t *testing.T) {
159128

160129
func TestGetToolsetInstructions(t *testing.T) {
161130
tests := []struct {
162-
toolset string
163-
expected string
131+
toolset string
132+
expectedEmpty bool
164133
}{
165134
{
166-
toolset: "pull_requests",
167-
expected: "create_pending_pull_request_review",
135+
toolset: "pull_requests",
136+
expectedEmpty: false,
168137
},
169138
{
170-
toolset: "issues",
171-
expected: "list_issue_types",
139+
toolset: "issues",
140+
expectedEmpty: false,
172141
},
173142
{
174-
toolset: "notifications",
175-
expected: "participating",
143+
toolset: "discussions",
144+
expectedEmpty: false,
176145
},
177146
{
178-
toolset: "discussions",
179-
expected: "list_discussion_categories",
180-
},
181-
{
182-
toolset: "nonexistent",
183-
expected: "",
147+
toolset: "nonexistent",
148+
expectedEmpty: true,
184149
},
185150
}
186151

187152
for _, tt := range tests {
188153
t.Run(tt.toolset, func(t *testing.T) {
189154
result := getToolsetInstructions(tt.toolset)
190-
if tt.expected == "" {
155+
if tt.expectedEmpty {
191156
if result != "" {
192157
t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result)
193158
}
194159
} else {
195-
if !strings.Contains(result, tt.expected) {
196-
t.Errorf("Expected instructions for '%s' to contain '%s', but got: %s", tt.toolset, tt.expected, result)
160+
if result == "" {
161+
t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset)
197162
}
198163
}
199164
})
200165
}
201-
}
166+
}

0 commit comments

Comments
 (0)