Skip to content

Commit ee8bd2a

Browse files
authored
Prevent servers without tools from being added/running (#173)
1 parent aefb45b commit ee8bd2a

File tree

7 files changed

+243
-0
lines changed

7 files changed

+243
-0
lines changed

cmd/add.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,9 @@ func parseServerEntry(pkg packages.Server, opts serverEntryOptions) (config.Serv
283283
if err != nil {
284284
return config.ServerEntry{}, fmt.Errorf("error matching requested tools: %w", err)
285285
}
286+
if len(requestedTools) == 0 {
287+
return config.ServerEntry{}, fmt.Errorf("tools not available")
288+
}
286289

287290
selectedRuntime, err := selectRuntime(pkg.Installations, opts.Runtime, opts.SupportedRuntimes)
288291
if err != nil {

cmd/add_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,40 @@ func TestParseServerEntry(t *testing.T) {
677677
isErrorExpected: true,
678678
expectedErrorMessage: "installation server name is missing for runtime 'docker'",
679679
},
680+
{
681+
name: "server with no tools available",
682+
installations: map[runtime.Runtime]packages.Installation{
683+
runtime.UVX: {
684+
Package: "mcp-server-no-tools",
685+
Recommended: true,
686+
},
687+
},
688+
supportedRuntimes: []runtime.Runtime{runtime.UVX},
689+
pkgName: "no-tools",
690+
pkgID: "no-tools",
691+
availableTools: []string{}, // No tools available
692+
requestedTools: []string{},
693+
arguments: packages.Arguments{},
694+
isErrorExpected: true,
695+
expectedErrorMessage: "tools not available",
696+
},
697+
{
698+
name: "server with requested tools not available",
699+
installations: map[runtime.Runtime]packages.Installation{
700+
runtime.UVX: {
701+
Package: "mcp-server-limited",
702+
Recommended: true,
703+
},
704+
},
705+
supportedRuntimes: []runtime.Runtime{runtime.UVX},
706+
pkgName: "limited",
707+
pkgID: "limited",
708+
availableTools: []string{"toolA", "toolB"},
709+
requestedTools: []string{"toolC", "toolD"}, // Requesting unavailable tools
710+
arguments: packages.Arguments{},
711+
isErrorExpected: true,
712+
expectedErrorMessage: "error matching requested tools: none of the requested values were found",
713+
},
680714
{
681715
name: "requested tool not found",
682716
installations: map[runtime.Runtime]packages.Installation{

cmd/config/tools/remove.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ func (c *RemoveCmd) run(cmd *cobra.Command, args []string) error {
7676
}
7777
}
7878

79+
// Validate that at least one tool remains.
80+
if len(srv.Tools) == 0 {
81+
return fmt.Errorf("cannot remove all tools from server '%s'\n"+
82+
"To remove the server instead use: mcpd remove %s", serverName, serverName)
83+
}
84+
7985
// Update server in config by removing and re-adding.
8086
err = cfg.RemoveServer(serverName)
8187
if err != nil {

cmd/config/tools/remove_test.go

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
package tools
2+
3+
import (
4+
"bytes"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
"github.com/mozilla-ai/mcpd/v2/internal/cmd"
10+
cmdopts "github.com/mozilla-ai/mcpd/v2/internal/cmd/options"
11+
"github.com/mozilla-ai/mcpd/v2/internal/config"
12+
)
13+
14+
type mockConfigForRemove struct {
15+
servers []config.ServerEntry
16+
addErr error
17+
removeErr error
18+
}
19+
20+
func (m *mockConfigForRemove) AddServer(entry config.ServerEntry) error {
21+
if m.addErr != nil {
22+
return m.addErr
23+
}
24+
m.servers = append(m.servers, entry)
25+
return nil
26+
}
27+
28+
func (m *mockConfigForRemove) RemoveServer(name string) error {
29+
if m.removeErr != nil {
30+
return m.removeErr
31+
}
32+
for i, s := range m.servers {
33+
if s.Name == name {
34+
m.servers = append(m.servers[:i], m.servers[i+1:]...)
35+
return nil
36+
}
37+
}
38+
return nil
39+
}
40+
41+
func (m *mockConfigForRemove) ListServers() []config.ServerEntry {
42+
return m.servers
43+
}
44+
45+
func (m *mockConfigForRemove) SaveConfig() error {
46+
return nil
47+
}
48+
49+
type mockLoaderForRemove struct {
50+
cfg *mockConfigForRemove
51+
err error
52+
}
53+
54+
func (m *mockLoaderForRemove) Load(_ string) (config.Modifier, error) {
55+
return m.cfg, m.err
56+
}
57+
58+
func TestRemoveCmd_RemoveAllTools(t *testing.T) {
59+
t.Parallel()
60+
61+
tests := []struct {
62+
name string
63+
serverName string
64+
initialTools []string
65+
toolsToRemove []string
66+
expectedError string
67+
}{
68+
{
69+
name: "removing all tools should fail",
70+
serverName: "test-server",
71+
initialTools: []string{"tool1", "tool2"},
72+
toolsToRemove: []string{"tool1", "tool2"},
73+
expectedError: "cannot remove all tools from server 'test-server'\nTo remove the server instead use: mcpd remove test-server",
74+
},
75+
{
76+
name: "removing last tool should fail",
77+
serverName: "test-server",
78+
initialTools: []string{"tool1"},
79+
toolsToRemove: []string{"tool1"},
80+
expectedError: "cannot remove all tools from server 'test-server'\nTo remove the server instead use: mcpd remove test-server",
81+
},
82+
{
83+
name: "removing some tools should succeed",
84+
serverName: "test-server",
85+
initialTools: []string{"tool1", "tool2", "tool3"},
86+
toolsToRemove: []string{"tool1", "tool2"},
87+
expectedError: "",
88+
},
89+
}
90+
91+
for _, tc := range tests {
92+
t.Run(tc.name, func(t *testing.T) {
93+
t.Parallel()
94+
95+
cfg := &mockConfigForRemove{
96+
servers: []config.ServerEntry{
97+
{
98+
Name: tc.serverName,
99+
Tools: tc.initialTools,
100+
},
101+
},
102+
}
103+
104+
loader := &mockLoaderForRemove{cfg: cfg}
105+
baseCmd := &cmd.BaseCmd{}
106+
107+
removeCmd, err := NewRemoveCmd(baseCmd, cmdopts.WithConfigLoader(loader))
108+
require.NoError(t, err)
109+
110+
// Prepare arguments: server-name followed by tools to remove.
111+
args := append([]string{tc.serverName}, tc.toolsToRemove...)
112+
113+
var out bytes.Buffer
114+
removeCmd.SetOut(&out)
115+
removeCmd.SetErr(&out)
116+
117+
err = removeCmd.RunE(removeCmd, args)
118+
119+
if tc.expectedError != "" {
120+
require.EqualError(t, err, tc.expectedError)
121+
} else {
122+
require.NoError(t, err)
123+
// Verify some tools remain.
124+
require.Greater(t, len(cfg.servers[0].Tools), 0)
125+
}
126+
})
127+
}
128+
}

docs/configuration.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ The reload process maintains strict consistency - any error causes the daemon to
201201

202202
- **Configuration errors**: Invalid configuration files or loading failures cause the daemon to exit
203203
- **Validation errors**: Invalid server configurations cause the daemon to exit
204+
- **No tools configured**: If a server configuration has no tools (empty tools list or manually removed from config), the daemon will exit with an error
204205
- **Server operation failures**: Any failure to start, stop, or restart a server causes the daemon to exit
205206

206207
This ensures the daemon never runs in an inconsistent or partially-failed state, matching the behavior during initial startup where any server failure prevents the daemon from running.

internal/daemon/daemon.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,17 @@ func (d *Daemon) startMCPServers(ctx context.Context) error {
158158
return nil
159159
}
160160

161+
// startMCPServer starts a single MCP server and registers it with the daemon.
162+
// It validates that the server has tools and a supported runtime before initializing.
161163
func (d *Daemon) startMCPServer(ctx context.Context, server runtime.Server) error {
164+
// Validate that the server has tools configured.
165+
if len(server.Tools) == 0 {
166+
return fmt.Errorf(
167+
"server '%s' has no tools configured - MCP servers require at least one tool to function",
168+
server.Name(),
169+
)
170+
}
171+
162172
runtimeBinary := server.Runtime()
163173
if _, supported := d.supportedRuntimes[runtime.Runtime(runtimeBinary)]; !supported {
164174
return fmt.Errorf(

internal/daemon/daemon_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,67 @@ func TestDaemon_CloseClientWithTimeout_Direct(t *testing.T) {
862862
}
863863
}
864864

865+
// TestDaemon_StartMCPServer_NoTools tests that servers without tools are rejected
866+
func TestDaemon_StartMCPServer_NoTools(t *testing.T) {
867+
t.Parallel()
868+
869+
tests := []struct {
870+
name string
871+
serverName string
872+
tools []string
873+
expectedError string
874+
}{
875+
{
876+
name: "server with no tools should fail",
877+
serverName: "test-no-tools",
878+
tools: []string{},
879+
expectedError: "server 'test-no-tools' has no tools configured - MCP servers require at least one tool to function",
880+
},
881+
{
882+
name: "server with empty tools list should fail",
883+
serverName: "test-empty-tools",
884+
tools: []string{},
885+
expectedError: "server 'test-empty-tools' has no tools configured - MCP servers require at least one tool to function",
886+
},
887+
}
888+
889+
for _, tc := range tests {
890+
t.Run(tc.name, func(t *testing.T) {
891+
t.Parallel()
892+
893+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
894+
t.Cleanup(cancel)
895+
896+
logger := hclog.NewNullLogger()
897+
898+
// Create a daemon with a dummy server to satisfy validation.
899+
dummyServer := runtime.Server{
900+
ServerEntry: config.ServerEntry{
901+
Name: "dummy",
902+
Package: "uvx::dummy",
903+
},
904+
}
905+
deps, err := NewDependencies(logger, ":8085", []runtime.Server{dummyServer})
906+
require.NoError(t, err)
907+
daemon, err := NewDaemon(deps)
908+
require.NoError(t, err)
909+
910+
// Create server with specified tools.
911+
server := runtime.Server{
912+
ServerEntry: config.ServerEntry{
913+
Name: tc.serverName,
914+
Package: "uvx::test-package",
915+
Tools: tc.tools,
916+
},
917+
ServerExecutionContext: configcontext.ServerExecutionContext{},
918+
}
919+
920+
err = daemon.startMCPServer(ctx, server)
921+
require.EqualError(t, err, tc.expectedError)
922+
})
923+
}
924+
}
925+
865926
// TestDaemon_CloseClientWithTimeout_ReturnValue tests the return value behavior of closeClientWithTimeout
866927
func TestDaemon_CloseClientWithTimeout_ReturnValue(t *testing.T) {
867928
t.Parallel()

0 commit comments

Comments
 (0)