From 9fac1322041464a92c960c81fbb3cdcfec09c590 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Mon, 15 Sep 2025 08:52:03 +0100 Subject: [PATCH] Prevent servers without tools from being added/running --- cmd/add.go | 3 + cmd/add_test.go | 34 +++++++++ cmd/config/tools/remove.go | 6 ++ cmd/config/tools/remove_test.go | 128 ++++++++++++++++++++++++++++++++ docs/configuration.md | 1 + internal/daemon/daemon.go | 10 +++ internal/daemon/daemon_test.go | 61 +++++++++++++++ 7 files changed, 243 insertions(+) create mode 100644 cmd/config/tools/remove_test.go diff --git a/cmd/add.go b/cmd/add.go index 4a4f3d3..99c5cb4 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -283,6 +283,9 @@ func parseServerEntry(pkg packages.Server, opts serverEntryOptions) (config.Serv if err != nil { return config.ServerEntry{}, fmt.Errorf("error matching requested tools: %w", err) } + if len(requestedTools) == 0 { + return config.ServerEntry{}, fmt.Errorf("tools not available") + } selectedRuntime, err := selectRuntime(pkg.Installations, opts.Runtime, opts.SupportedRuntimes) if err != nil { diff --git a/cmd/add_test.go b/cmd/add_test.go index 845107e..d68cb24 100644 --- a/cmd/add_test.go +++ b/cmd/add_test.go @@ -677,6 +677,40 @@ func TestParseServerEntry(t *testing.T) { isErrorExpected: true, expectedErrorMessage: "installation server name is missing for runtime 'docker'", }, + { + name: "server with no tools available", + installations: map[runtime.Runtime]packages.Installation{ + runtime.UVX: { + Package: "mcp-server-no-tools", + Recommended: true, + }, + }, + supportedRuntimes: []runtime.Runtime{runtime.UVX}, + pkgName: "no-tools", + pkgID: "no-tools", + availableTools: []string{}, // No tools available + requestedTools: []string{}, + arguments: packages.Arguments{}, + isErrorExpected: true, + expectedErrorMessage: "tools not available", + }, + { + name: "server with requested tools not available", + installations: map[runtime.Runtime]packages.Installation{ + runtime.UVX: { + Package: "mcp-server-limited", + Recommended: true, + }, + }, + supportedRuntimes: []runtime.Runtime{runtime.UVX}, + pkgName: "limited", + pkgID: "limited", + availableTools: []string{"toolA", "toolB"}, + requestedTools: []string{"toolC", "toolD"}, // Requesting unavailable tools + arguments: packages.Arguments{}, + isErrorExpected: true, + expectedErrorMessage: "error matching requested tools: none of the requested values were found", + }, { name: "requested tool not found", installations: map[runtime.Runtime]packages.Installation{ diff --git a/cmd/config/tools/remove.go b/cmd/config/tools/remove.go index 11a01d9..ffb627f 100644 --- a/cmd/config/tools/remove.go +++ b/cmd/config/tools/remove.go @@ -76,6 +76,12 @@ func (c *RemoveCmd) run(cmd *cobra.Command, args []string) error { } } + // Validate that at least one tool remains. + if len(srv.Tools) == 0 { + return fmt.Errorf("cannot remove all tools from server '%s'\n"+ + "To remove the server instead use: mcpd remove %s", serverName, serverName) + } + // Update server in config by removing and re-adding. err = cfg.RemoveServer(serverName) if err != nil { diff --git a/cmd/config/tools/remove_test.go b/cmd/config/tools/remove_test.go new file mode 100644 index 0000000..8a6661b --- /dev/null +++ b/cmd/config/tools/remove_test.go @@ -0,0 +1,128 @@ +package tools + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mozilla-ai/mcpd/v2/internal/cmd" + cmdopts "github.com/mozilla-ai/mcpd/v2/internal/cmd/options" + "github.com/mozilla-ai/mcpd/v2/internal/config" +) + +type mockConfigForRemove struct { + servers []config.ServerEntry + addErr error + removeErr error +} + +func (m *mockConfigForRemove) AddServer(entry config.ServerEntry) error { + if m.addErr != nil { + return m.addErr + } + m.servers = append(m.servers, entry) + return nil +} + +func (m *mockConfigForRemove) RemoveServer(name string) error { + if m.removeErr != nil { + return m.removeErr + } + for i, s := range m.servers { + if s.Name == name { + m.servers = append(m.servers[:i], m.servers[i+1:]...) + return nil + } + } + return nil +} + +func (m *mockConfigForRemove) ListServers() []config.ServerEntry { + return m.servers +} + +func (m *mockConfigForRemove) SaveConfig() error { + return nil +} + +type mockLoaderForRemove struct { + cfg *mockConfigForRemove + err error +} + +func (m *mockLoaderForRemove) Load(_ string) (config.Modifier, error) { + return m.cfg, m.err +} + +func TestRemoveCmd_RemoveAllTools(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + serverName string + initialTools []string + toolsToRemove []string + expectedError string + }{ + { + name: "removing all tools should fail", + serverName: "test-server", + initialTools: []string{"tool1", "tool2"}, + toolsToRemove: []string{"tool1", "tool2"}, + expectedError: "cannot remove all tools from server 'test-server'\nTo remove the server instead use: mcpd remove test-server", + }, + { + name: "removing last tool should fail", + serverName: "test-server", + initialTools: []string{"tool1"}, + toolsToRemove: []string{"tool1"}, + expectedError: "cannot remove all tools from server 'test-server'\nTo remove the server instead use: mcpd remove test-server", + }, + { + name: "removing some tools should succeed", + serverName: "test-server", + initialTools: []string{"tool1", "tool2", "tool3"}, + toolsToRemove: []string{"tool1", "tool2"}, + expectedError: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cfg := &mockConfigForRemove{ + servers: []config.ServerEntry{ + { + Name: tc.serverName, + Tools: tc.initialTools, + }, + }, + } + + loader := &mockLoaderForRemove{cfg: cfg} + baseCmd := &cmd.BaseCmd{} + + removeCmd, err := NewRemoveCmd(baseCmd, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + // Prepare arguments: server-name followed by tools to remove. + args := append([]string{tc.serverName}, tc.toolsToRemove...) + + var out bytes.Buffer + removeCmd.SetOut(&out) + removeCmd.SetErr(&out) + + err = removeCmd.RunE(removeCmd, args) + + if tc.expectedError != "" { + require.EqualError(t, err, tc.expectedError) + } else { + require.NoError(t, err) + // Verify some tools remain. + require.Greater(t, len(cfg.servers[0].Tools), 0) + } + }) + } +} diff --git a/docs/configuration.md b/docs/configuration.md index bd6b073..6de50a2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -201,6 +201,7 @@ The reload process maintains strict consistency - any error causes the daemon to - **Configuration errors**: Invalid configuration files or loading failures cause the daemon to exit - **Validation errors**: Invalid server configurations cause the daemon to exit +- **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 - **Server operation failures**: Any failure to start, stop, or restart a server causes the daemon to exit 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. diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 20d3259..a774407 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -158,7 +158,17 @@ func (d *Daemon) startMCPServers(ctx context.Context) error { return nil } +// startMCPServer starts a single MCP server and registers it with the daemon. +// It validates that the server has tools and a supported runtime before initializing. func (d *Daemon) startMCPServer(ctx context.Context, server runtime.Server) error { + // Validate that the server has tools configured. + if len(server.Tools) == 0 { + return fmt.Errorf( + "server '%s' has no tools configured - MCP servers require at least one tool to function", + server.Name(), + ) + } + runtimeBinary := server.Runtime() if _, supported := d.supportedRuntimes[runtime.Runtime(runtimeBinary)]; !supported { return fmt.Errorf( diff --git a/internal/daemon/daemon_test.go b/internal/daemon/daemon_test.go index 1b35928..9a7546d 100644 --- a/internal/daemon/daemon_test.go +++ b/internal/daemon/daemon_test.go @@ -862,6 +862,67 @@ func TestDaemon_CloseClientWithTimeout_Direct(t *testing.T) { } } +// TestDaemon_StartMCPServer_NoTools tests that servers without tools are rejected +func TestDaemon_StartMCPServer_NoTools(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + serverName string + tools []string + expectedError string + }{ + { + name: "server with no tools should fail", + serverName: "test-no-tools", + tools: []string{}, + expectedError: "server 'test-no-tools' has no tools configured - MCP servers require at least one tool to function", + }, + { + name: "server with empty tools list should fail", + serverName: "test-empty-tools", + tools: []string{}, + expectedError: "server 'test-empty-tools' has no tools configured - MCP servers require at least one tool to function", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + logger := hclog.NewNullLogger() + + // Create a daemon with a dummy server to satisfy validation. + dummyServer := runtime.Server{ + ServerEntry: config.ServerEntry{ + Name: "dummy", + Package: "uvx::dummy", + }, + } + deps, err := NewDependencies(logger, ":8085", []runtime.Server{dummyServer}) + require.NoError(t, err) + daemon, err := NewDaemon(deps) + require.NoError(t, err) + + // Create server with specified tools. + server := runtime.Server{ + ServerEntry: config.ServerEntry{ + Name: tc.serverName, + Package: "uvx::test-package", + Tools: tc.tools, + }, + ServerExecutionContext: configcontext.ServerExecutionContext{}, + } + + err = daemon.startMCPServer(ctx, server) + require.EqualError(t, err, tc.expectedError) + }) + } +} + // TestDaemon_CloseClientWithTimeout_ReturnValue tests the return value behavior of closeClientWithTimeout func TestDaemon_CloseClientWithTimeout_ReturnValue(t *testing.T) { t.Parallel()