Skip to content

Commit 5c75327

Browse files
manusamanusa
authored
test(mcp): refactor tool filtering tests
- Prevent declaring tools that are both read-only and destructive - Remove redundant tests and preserve those behavioral and semantic
1 parent 83c37ce commit 5c75327

File tree

3 files changed

+119
-213
lines changed

3 files changed

+119
-213
lines changed

‎pkg/mcp/mcp.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func (c *Configuration) isToolApplicable(tool server.ServerTool) bool {
2828
if c.StaticConfig.ReadOnly && !ptr.Deref(tool.Tool.Annotations.ReadOnlyHint, false) {
2929
return false
3030
}
31-
if c.StaticConfig.DisableDestructive && !ptr.Deref(tool.Tool.Annotations.ReadOnlyHint, false) && ptr.Deref(tool.Tool.Annotations.DestructiveHint, false) {
31+
if c.StaticConfig.DisableDestructive && ptr.Deref(tool.Tool.Annotations.DestructiveHint, false) {
3232
return false
3333
}
3434
if c.StaticConfig.EnabledTools != nil && !slices.Contains(c.StaticConfig.EnabledTools, tool.Tool.Name) {

‎pkg/mcp/mcp_test.go‎

Lines changed: 0 additions & 212 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,8 @@ import (
99
"testing"
1010
"time"
1111

12-
"k8s.io/utils/ptr"
13-
14-
"github.com/manusa/kubernetes-mcp-server/pkg/config"
1512
"github.com/mark3labs/mcp-go/client"
1613
"github.com/mark3labs/mcp-go/mcp"
17-
"github.com/mark3labs/mcp-go/server"
1814
)
1915

2016
func TestWatchKubeConfig(t *testing.T) {
@@ -55,214 +51,6 @@ func TestWatchKubeConfig(t *testing.T) {
5551
})
5652
}
5753

58-
func TestReadOnly(t *testing.T) {
59-
readOnlyServer := func(c *mcpContext) { c.staticConfig = &config.StaticConfig{ReadOnly: true} }
60-
testCaseWithContext(t, &mcpContext{before: readOnlyServer}, func(c *mcpContext) {
61-
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
62-
t.Run("ListTools returns tools", func(t *testing.T) {
63-
if err != nil {
64-
t.Fatalf("call ListTools failed %v", err)
65-
}
66-
})
67-
t.Run("ListTools returns only read-only tools", func(t *testing.T) {
68-
for _, tool := range tools.Tools {
69-
if tool.Annotations.ReadOnlyHint == nil || !*tool.Annotations.ReadOnlyHint {
70-
t.Errorf("Tool %s is not read-only but should be", tool.Name)
71-
}
72-
if tool.Annotations.DestructiveHint != nil && *tool.Annotations.DestructiveHint {
73-
t.Errorf("Tool %s is destructive but should not be in read-only mode", tool.Name)
74-
}
75-
}
76-
})
77-
})
78-
}
79-
80-
func TestIsToolApplicableReadOnly(t *testing.T) {
81-
tests := []struct {
82-
config Configuration
83-
expected bool
84-
tool server.ServerTool
85-
}{
86-
{
87-
config: Configuration{
88-
StaticConfig: &config.StaticConfig{
89-
ReadOnly: true,
90-
},
91-
},
92-
expected: true,
93-
tool: server.ServerTool{
94-
Tool: mcp.Tool{
95-
Annotations: mcp.ToolAnnotation{
96-
ReadOnlyHint: ptr.To(true),
97-
},
98-
},
99-
},
100-
},
101-
{
102-
config: Configuration{
103-
StaticConfig: &config.StaticConfig{
104-
ReadOnly: true,
105-
},
106-
},
107-
expected: false,
108-
tool: server.ServerTool{
109-
Tool: mcp.Tool{
110-
Annotations: mcp.ToolAnnotation{
111-
ReadOnlyHint: ptr.To(false),
112-
},
113-
},
114-
},
115-
},
116-
{
117-
config: Configuration{
118-
StaticConfig: &config.StaticConfig{
119-
DisableDestructive: true,
120-
},
121-
},
122-
expected: true,
123-
tool: server.ServerTool{
124-
Tool: mcp.Tool{
125-
Annotations: mcp.ToolAnnotation{
126-
DestructiveHint: ptr.To(false),
127-
},
128-
},
129-
},
130-
},
131-
{
132-
config: Configuration{
133-
StaticConfig: &config.StaticConfig{
134-
DisableDestructive: true,
135-
},
136-
},
137-
expected: true,
138-
tool: server.ServerTool{
139-
Tool: mcp.Tool{
140-
Annotations: mcp.ToolAnnotation{
141-
DestructiveHint: ptr.To(true),
142-
ReadOnlyHint: ptr.To(true),
143-
},
144-
},
145-
},
146-
},
147-
{
148-
config: Configuration{
149-
StaticConfig: &config.StaticConfig{
150-
DisableDestructive: true,
151-
},
152-
},
153-
expected: false,
154-
tool: server.ServerTool{
155-
Tool: mcp.Tool{
156-
Annotations: mcp.ToolAnnotation{
157-
DestructiveHint: ptr.To(true),
158-
},
159-
},
160-
},
161-
},
162-
{
163-
config: Configuration{
164-
StaticConfig: &config.StaticConfig{
165-
EnabledTools: []string{"namespaces_list"},
166-
},
167-
},
168-
expected: true,
169-
tool: server.ServerTool{
170-
Tool: mcp.Tool{
171-
Name: "namespaces_list",
172-
},
173-
},
174-
},
175-
{
176-
config: Configuration{
177-
StaticConfig: &config.StaticConfig{
178-
DisabledTools: []string{"namespaces_list"},
179-
},
180-
},
181-
expected: false,
182-
tool: server.ServerTool{
183-
Tool: mcp.Tool{
184-
Name: "namespaces_list",
185-
},
186-
},
187-
},
188-
}
189-
for _, test := range tests {
190-
t.Run("", func(t *testing.T) {
191-
isToolApplicable := test.config.isToolApplicable(test.tool)
192-
if isToolApplicable != test.expected {
193-
t.Errorf("isToolApplicable should return %t, got %t", test.expected, isToolApplicable)
194-
}
195-
})
196-
}
197-
198-
}
199-
200-
func TestIsToolApplicableEnabledTools(t *testing.T) {
201-
testCaseWithContext(t, &mcpContext{
202-
staticConfig: &config.StaticConfig{
203-
EnabledTools: []string{"namespaces_list", "events_list"},
204-
},
205-
}, func(c *mcpContext) {
206-
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
207-
t.Run("ListTools returns tools", func(t *testing.T) {
208-
if err != nil {
209-
t.Fatalf("call ListTools failed %v", err)
210-
}
211-
})
212-
t.Run("ListTools does not only return enabled tools", func(t *testing.T) {
213-
if len(tools.Tools) != 2 {
214-
t.Fatalf("ListTools should return 2 tools, got %d", len(tools.Tools))
215-
}
216-
for _, tool := range tools.Tools {
217-
if tool.Name != "namespaces_list" && tool.Name != "events_list" {
218-
t.Errorf("Tool %s is not enabled but should be", tool.Name)
219-
}
220-
}
221-
})
222-
})
223-
}
224-
225-
func TestIsToolApplicableDisabledTools(t *testing.T) {
226-
testCaseWithContext(t, &mcpContext{
227-
staticConfig: &config.StaticConfig{
228-
DisabledTools: []string{"namespaces_list", "events_list"},
229-
},
230-
}, func(c *mcpContext) {
231-
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
232-
t.Run("ListTools returns tools", func(t *testing.T) {
233-
if err != nil {
234-
t.Fatalf("call ListTools failed %v", err)
235-
}
236-
})
237-
t.Run("ListTools does not only return disabled tools", func(t *testing.T) {
238-
for _, tool := range tools.Tools {
239-
if tool.Name == "namespaces_list" || tool.Name == "events_list" {
240-
t.Errorf("Tool %s is not disabled but should be", tool.Name)
241-
}
242-
}
243-
})
244-
})
245-
}
246-
247-
func TestDisableDestructive(t *testing.T) {
248-
disableDestructiveServer := func(c *mcpContext) { c.staticConfig = &config.StaticConfig{DisableDestructive: true} }
249-
testCaseWithContext(t, &mcpContext{before: disableDestructiveServer}, func(c *mcpContext) {
250-
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
251-
t.Run("ListTools returns tools", func(t *testing.T) {
252-
if err != nil {
253-
t.Fatalf("call ListTools failed %v", err)
254-
}
255-
})
256-
t.Run("ListTools does not return destructive tools", func(t *testing.T) {
257-
for _, tool := range tools.Tools {
258-
if tool.Annotations.DestructiveHint != nil && *tool.Annotations.DestructiveHint {
259-
t.Errorf("Tool %s is destructive but should not be", tool.Name)
260-
}
261-
}
262-
})
263-
})
264-
}
265-
26654
func TestSseHeaders(t *testing.T) {
26755
mockServer := NewMockServer()
26856
defer mockServer.Close()

‎pkg/mcp/mcp_tools_test.go‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
package mcp
2+
3+
import (
4+
"k8s.io/utils/ptr"
5+
"testing"
6+
7+
"github.com/mark3labs/mcp-go/mcp"
8+
9+
"github.com/manusa/kubernetes-mcp-server/pkg/config"
10+
)
11+
12+
func TestUnrestricted(t *testing.T) {
13+
testCase(t, func(c *mcpContext) {
14+
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
15+
t.Run("ListTools returns tools", func(t *testing.T) {
16+
if err != nil {
17+
t.Fatalf("call ListTools failed %v", err)
18+
}
19+
})
20+
t.Run("Destructive tools ARE NOT read only", func(t *testing.T) {
21+
for _, tool := range tools.Tools {
22+
readOnly := ptr.Deref(tool.Annotations.ReadOnlyHint, false)
23+
destructive := ptr.Deref(tool.Annotations.DestructiveHint, false)
24+
if readOnly && destructive {
25+
t.Errorf("Tool %s is read-only and destructive, which is not allowed", tool.Name)
26+
}
27+
}
28+
})
29+
})
30+
}
31+
32+
func TestReadOnly(t *testing.T) {
33+
readOnlyServer := func(c *mcpContext) { c.staticConfig = &config.StaticConfig{ReadOnly: true} }
34+
testCaseWithContext(t, &mcpContext{before: readOnlyServer}, func(c *mcpContext) {
35+
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
36+
t.Run("ListTools returns tools", func(t *testing.T) {
37+
if err != nil {
38+
t.Fatalf("call ListTools failed %v", err)
39+
}
40+
})
41+
t.Run("ListTools returns only read-only tools", func(t *testing.T) {
42+
for _, tool := range tools.Tools {
43+
if tool.Annotations.ReadOnlyHint == nil || !*tool.Annotations.ReadOnlyHint {
44+
t.Errorf("Tool %s is not read-only but should be", tool.Name)
45+
}
46+
if tool.Annotations.DestructiveHint != nil && *tool.Annotations.DestructiveHint {
47+
t.Errorf("Tool %s is destructive but should not be in read-only mode", tool.Name)
48+
}
49+
}
50+
})
51+
})
52+
}
53+
54+
func TestDisableDestructive(t *testing.T) {
55+
disableDestructiveServer := func(c *mcpContext) { c.staticConfig = &config.StaticConfig{DisableDestructive: true} }
56+
testCaseWithContext(t, &mcpContext{before: disableDestructiveServer}, func(c *mcpContext) {
57+
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
58+
t.Run("ListTools returns tools", func(t *testing.T) {
59+
if err != nil {
60+
t.Fatalf("call ListTools failed %v", err)
61+
}
62+
})
63+
t.Run("ListTools does not return destructive tools", func(t *testing.T) {
64+
for _, tool := range tools.Tools {
65+
if tool.Annotations.DestructiveHint != nil && *tool.Annotations.DestructiveHint {
66+
t.Errorf("Tool %s is destructive but should not be", tool.Name)
67+
}
68+
}
69+
})
70+
})
71+
}
72+
73+
func TestEnabledTools(t *testing.T) {
74+
testCaseWithContext(t, &mcpContext{
75+
staticConfig: &config.StaticConfig{
76+
EnabledTools: []string{"namespaces_list", "events_list"},
77+
},
78+
}, func(c *mcpContext) {
79+
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
80+
t.Run("ListTools returns tools", func(t *testing.T) {
81+
if err != nil {
82+
t.Fatalf("call ListTools failed %v", err)
83+
}
84+
})
85+
t.Run("ListTools returns only explicitly enabled tools", func(t *testing.T) {
86+
if len(tools.Tools) != 2 {
87+
t.Fatalf("ListTools should return 2 tools, got %d", len(tools.Tools))
88+
}
89+
for _, tool := range tools.Tools {
90+
if tool.Name != "namespaces_list" && tool.Name != "events_list" {
91+
t.Errorf("Tool %s is not enabled but should be", tool.Name)
92+
}
93+
}
94+
})
95+
})
96+
}
97+
98+
func TestDisabledTools(t *testing.T) {
99+
testCaseWithContext(t, &mcpContext{
100+
staticConfig: &config.StaticConfig{
101+
DisabledTools: []string{"namespaces_list", "events_list"},
102+
},
103+
}, func(c *mcpContext) {
104+
tools, err := c.mcpClient.ListTools(c.ctx, mcp.ListToolsRequest{})
105+
t.Run("ListTools returns tools", func(t *testing.T) {
106+
if err != nil {
107+
t.Fatalf("call ListTools failed %v", err)
108+
}
109+
})
110+
t.Run("ListTools does not return disabled tools", func(t *testing.T) {
111+
for _, tool := range tools.Tools {
112+
if tool.Name == "namespaces_list" || tool.Name == "events_list" {
113+
t.Errorf("Tool %s is not disabled but should be", tool.Name)
114+
}
115+
}
116+
})
117+
})
118+
}

0 commit comments

Comments
 (0)