Skip to content

Commit 69b1a3a

Browse files
tonytrgtonytrg
committed
cleaning code
1 parent 8d2995d commit 69b1a3a

File tree

2 files changed

+124
-67
lines changed

2 files changed

+124
-67
lines changed

‎internal/ghmcp/server.go‎

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"net/url"
1111
"os"
1212
"os/signal"
13-
"slices"
1413
"strings"
1514
"syscall"
1615
"time"
@@ -107,18 +106,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
107106
},
108107
}
109108

110-
enabledToolsets := cfg.EnabledToolsets
111-
if cfg.DynamicToolsets {
112-
// filter "all" from the enabled toolsets
113-
enabledToolsets = make([]string, 0, len(cfg.EnabledToolsets))
114-
for _, toolset := range cfg.EnabledToolsets {
115-
if toolset != github.ToolsetMetadataAll.ID {
116-
enabledToolsets = append(enabledToolsets, toolset)
117-
}
118-
}
119-
}
120-
121-
enabledToolsets = transformSpecialToolsets(enabledToolsets)
109+
enabledToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets)
122110

123111
// Generate instructions based on enabled toolsets
124112
instructions := github.GenerateInstructions(enabledToolsets)
@@ -474,30 +462,34 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro
474462
return t.transport.RoundTrip(req)
475463
}
476464

477-
// transformSpecialToolsets handles special toolset keywords in the enabled toolsets list:
478-
// - "all": Returns ["all"] immediately, ignoring all other toolsets
465+
// cleanToolsets handles special toolset keywords in the enabled toolsets list:
466+
// Duplicates are removed from the result.
467+
// - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true)
479468
// - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs()
480-
// Duplicates are removed from the final result.
481-
func transformSpecialToolsets(enabledToolsets []string) []string {
482-
// Check if "all" is present - if so, return immediately
483-
if slices.Contains(enabledToolsets, github.ToolsetMetadataAll.ID) {
484-
return []string{github.ToolsetMetadataAll.ID}
485-
}
486-
487-
hasDefault := slices.Contains(enabledToolsets, github.ToolsetMetadataDefault.ID)
488-
469+
// When dynamicToolsets is true, filters out "all" from the enabled toolsets.
470+
func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string {
489471
seen := make(map[string]bool)
490472
result := make([]string, 0, len(enabledToolsets))
491473

492474
// Add non-default toolsets, removing duplicates
493475
for _, toolset := range enabledToolsets {
494-
if toolset != github.ToolsetMetadataDefault.ID && !seen[toolset] {
495-
result = append(result, toolset)
476+
if !seen[toolset] {
496477
seen[toolset] = true
478+
if toolset != github.ToolsetMetadataDefault.ID && toolset != github.ToolsetMetadataAll.ID {
479+
result = append(result, toolset)
480+
}
497481
}
498482
}
499483

500-
// If "default" was found, add the default toolset IDs
484+
hasDefault := seen[github.ToolsetMetadataDefault.ID]
485+
hasAll := seen[github.ToolsetMetadataAll.ID]
486+
487+
// Handle "all" keyword - return early if not in dynamic mode
488+
if hasAll && !dynamicToolsets {
489+
return []string{github.ToolsetMetadataAll.ID}
490+
}
491+
492+
// Expand "default" keyword to actual default toolsets
501493
if hasDefault {
502494
for _, defaultToolset := range github.GetDefaultToolsetIDs() {
503495
if !seen[defaultToolset] {

‎internal/ghmcp/server_test.go‎

Lines changed: 105 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -10,38 +10,53 @@ import (
1010

1111
func TestTransformSpecialToolsets(t *testing.T) {
1212
tests := []struct {
13-
name string
14-
input []string
15-
expected []string
13+
name string
14+
input []string
15+
dynamicToolsets bool
16+
expected []string
1617
}{
1718
{
18-
name: "empty slice",
19-
input: []string{},
20-
expected: []string{},
19+
name: "empty slice",
20+
input: []string{},
21+
dynamicToolsets: false,
22+
expected: []string{},
2123
},
2224
{
23-
name: "all only",
24-
input: []string{"all"},
25-
expected: []string{"all"},
25+
name: "nil input slice",
26+
input: nil,
27+
dynamicToolsets: false,
28+
expected: []string{},
2629
},
30+
// all test cases
2731
{
28-
name: "all with other toolsets",
29-
input: []string{"all", "actions", "gists"},
30-
expected: []string{"all"},
32+
name: "all only",
33+
input: []string{"all"},
34+
dynamicToolsets: false,
35+
expected: []string{"all"},
3136
},
3237
{
33-
name: "all at the end",
34-
input: []string{"actions", "gists", "all"},
35-
expected: []string{"all"},
38+
name: "all appears multiple times",
39+
input: []string{"all", "actions", "all"},
40+
dynamicToolsets: false,
41+
expected: []string{"all"},
3642
},
3743
{
38-
name: "all with default",
39-
input: []string{"default", "all", "actions"},
40-
expected: []string{"all"},
44+
name: "all with other toolsets",
45+
input: []string{"all", "actions", "gists"},
46+
dynamicToolsets: false,
47+
expected: []string{"all"},
4148
},
4249
{
43-
name: "default only",
44-
input: []string{"default"},
50+
name: "all with default",
51+
input: []string{"default", "all", "actions"},
52+
dynamicToolsets: false,
53+
expected: []string{"all"},
54+
},
55+
// default test cases
56+
{
57+
name: "default only",
58+
input: []string{"default"},
59+
dynamicToolsets: false,
4560
expected: []string{
4661
"context",
4762
"repos",
@@ -51,8 +66,9 @@ func TestTransformSpecialToolsets(t *testing.T) {
5166
},
5267
},
5368
{
54-
name: "default with additional toolsets",
55-
input: []string{"default", "actions", "gists"},
69+
name: "default with additional toolsets",
70+
input: []string{"default", "actions", "gists"},
71+
dynamicToolsets: false,
5672
expected: []string{
5773
"actions",
5874
"gists",
@@ -64,44 +80,80 @@ func TestTransformSpecialToolsets(t *testing.T) {
6480
},
6581
},
6682
{
67-
name: "default with overlapping toolsets",
68-
input: []string{"default", "issues", "actions"},
83+
name: "no default present",
84+
input: []string{"actions", "gists", "notifications"},
85+
dynamicToolsets: false,
86+
expected: []string{"actions", "gists", "notifications"},
87+
},
88+
{
89+
name: "duplicate toolsets without default",
90+
input: []string{"actions", "gists", "actions"},
91+
dynamicToolsets: false,
92+
expected: []string{"actions", "gists"},
93+
},
94+
{
95+
name: "duplicate toolsets with default",
96+
input: []string{"context", "repos", "issues", "pull_requests", "users", "default"},
97+
dynamicToolsets: false,
6998
expected: []string{
99+
"context",
100+
"repos",
70101
"issues",
102+
"pull_requests",
103+
"users",
104+
},
105+
},
106+
{
107+
name: "default appears multiple times with different toolsets in between",
108+
input: []string{"default", "actions", "default", "gists", "default"},
109+
dynamicToolsets: false,
110+
expected: []string{
71111
"actions",
112+
"gists",
72113
"context",
73114
"repos",
115+
"issues",
74116
"pull_requests",
75117
"users",
76118
},
77119
},
120+
// Dynamic toolsets test cases
78121
{
79-
name: "no default present",
80-
input: []string{"actions", "gists", "notifications"},
81-
expected: []string{"actions", "gists", "notifications"},
122+
name: "dynamic toolsets - all only should be filtered",
123+
input: []string{"all"},
124+
dynamicToolsets: true,
125+
expected: []string{},
82126
},
83127
{
84-
name: "duplicate toolsets without default",
85-
input: []string{"actions", "gists", "actions"},
86-
expected: []string{"actions", "gists"},
128+
name: "dynamic toolsets - all with other toolsets",
129+
input: []string{"all", "actions", "gists"},
130+
dynamicToolsets: true,
131+
expected: []string{"actions", "gists"},
87132
},
88133
{
89-
name: "duplicate toolsets with default",
90-
input: []string{"actions", "default", "actions", "issues"},
134+
name: "dynamic toolsets - all with default",
135+
input: []string{"all", "default", "actions"},
136+
dynamicToolsets: true,
91137
expected: []string{
92138
"actions",
93-
"issues",
94139
"context",
95140
"repos",
141+
"issues",
96142
"pull_requests",
97143
"users",
98144
},
99145
},
100146
{
101-
name: "multiple defaults (edge case)",
102-
input: []string{"default", "actions", "default"},
147+
name: "dynamic toolsets - no all present",
148+
input: []string{"actions", "gists"},
149+
dynamicToolsets: true,
150+
expected: []string{"actions", "gists"},
151+
},
152+
{
153+
name: "dynamic toolsets - default only",
154+
input: []string{"default"},
155+
dynamicToolsets: true,
103156
expected: []string{
104-
"actions",
105157
"context",
106158
"repos",
107159
"issues",
@@ -110,8 +162,9 @@ func TestTransformSpecialToolsets(t *testing.T) {
110162
},
111163
},
112164
{
113-
name: "all default toolsets already present with default",
114-
input: []string{"context", "repos", "issues", "pull_requests", "users", "default"},
165+
name: "only special keywords with dynamic mode",
166+
input: []string{"all", "default"},
167+
dynamicToolsets: true,
115168
expected: []string{
116169
"context",
117170
"repos",
@@ -120,11 +173,23 @@ func TestTransformSpecialToolsets(t *testing.T) {
120173
"users",
121174
},
122175
},
176+
{
177+
name: "all with default and overlapping default toolsets in dynamic mode",
178+
input: []string{"all", "default", "issues", "repos"},
179+
dynamicToolsets: true,
180+
expected: []string{
181+
"issues",
182+
"repos",
183+
"context",
184+
"pull_requests",
185+
"users",
186+
},
187+
},
123188
}
124189

125190
for _, tt := range tests {
126191
t.Run(tt.name, func(t *testing.T) {
127-
result := transformSpecialToolsets(tt.input)
192+
result := cleanToolsets(tt.input, tt.dynamicToolsets)
128193

129194
// Check that the result has the correct length
130195
require.Len(t, result, len(tt.expected), "result length should match expected length")
@@ -155,7 +220,7 @@ func TestTransformSpecialToolsets(t *testing.T) {
155220
func TestTransformSpecialToolsetsWithActualDefaults(t *testing.T) {
156221
// This test verifies that the function uses the actual default toolsets from GetDefaultToolsetIDs()
157222
input := []string{"default"}
158-
result := transformSpecialToolsets(input)
223+
result := cleanToolsets(input, false)
159224

160225
defaultToolsets := github.GetDefaultToolsetIDs()
161226

0 commit comments

Comments
 (0)