From 70da3431fe26a39758de085a74e09f0abda44c3a Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Mon, 2 Mar 2026 19:56:07 +0000 Subject: [PATCH 1/2] feat: add mcpd config volumes remove command Implement `mcpd config volumes remove -- --` to remove volume mappings from MCP server runtime configurations. Also improves the existing `set` command: noop-aware messaging, consistent validation naming, and clearer withVolumes doc comment. Closes #200 --- cmd/config/volumes/cmd.go | 21 +- cmd/config/volumes/remove.go | 160 +++++++++++++ cmd/config/volumes/remove_test.go | 383 ++++++++++++++++++++++++++++++ cmd/config/volumes/set.go | 69 +++--- cmd/config/volumes/set_test.go | 66 +++-- 5 files changed, 638 insertions(+), 61 deletions(-) create mode 100644 cmd/config/volumes/remove.go create mode 100644 cmd/config/volumes/remove_test.go diff --git a/cmd/config/volumes/cmd.go b/cmd/config/volumes/cmd.go index cfaf967..1419693 100644 --- a/cmd/config/volumes/cmd.go +++ b/cmd/config/volumes/cmd.go @@ -1,10 +1,13 @@ package volumes import ( + "maps" + "github.com/spf13/cobra" "github.com/mozilla-ai/mcpd/internal/cmd" cmdopts "github.com/mozilla-ai/mcpd/internal/cmd/options" + "github.com/mozilla-ai/mcpd/internal/context" ) // NewCmd creates a new volumes command with its sub-commands. @@ -18,8 +21,9 @@ func NewCmd(baseCmd *cmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, err // Sub-commands for: mcpd config volumes fns := []func(baseCmd *cmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, error){ - NewListCmd, // list - NewSetCmd, // set + NewListCmd, // list + NewRemoveCmd, // remove + NewSetCmd, // set } for _, fn := range fns { @@ -32,3 +36,16 @@ func NewCmd(baseCmd *cmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, err return cobraCmd, nil } + +// withVolumes returns a new ServerExecutionContext with both volume fields +// set to unexpanded values. Volumes (the TOML-serialized field) preserves +// env var references on disk. RawVolumes is kept in sync for Equals/IsEmpty +// comparisons during Upsert. +func withVolumes( + server context.ServerExecutionContext, + working context.VolumeExecutionContext, +) context.ServerExecutionContext { + server.RawVolumes = maps.Clone(working) + server.Volumes = maps.Clone(working) + return server +} diff --git a/cmd/config/volumes/remove.go b/cmd/config/volumes/remove.go new file mode 100644 index 0000000..e9d3cb1 --- /dev/null +++ b/cmd/config/volumes/remove.go @@ -0,0 +1,160 @@ +package volumes + +import ( + "fmt" + "maps" + "slices" + "strings" + + "github.com/spf13/cobra" + + "github.com/mozilla-ai/mcpd/internal/cmd" + cmdopts "github.com/mozilla-ai/mcpd/internal/cmd/options" + "github.com/mozilla-ai/mcpd/internal/context" + "github.com/mozilla-ai/mcpd/internal/flags" +) + +// removeCmd handles removing volume mappings from MCP servers. +type removeCmd struct { + *cmd.BaseCmd + ctxLoader context.Loader +} + +// NewRemoveCmd creates a new remove command for volume configuration. +func NewRemoveCmd(baseCmd *cmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, error) { + opts, err := cmdopts.NewOptions(opt...) + if err != nil { + return nil, err + } + + c := &removeCmd{ + BaseCmd: baseCmd, + ctxLoader: opts.ContextLoader, + } + + cobraCmd := &cobra.Command{ + Use: "remove -- -- [--...]", + Short: "Remove volume mappings from an MCP server", + Long: "Remove volume mappings from an MCP server in the runtime context " + + "configuration file (e.g. " + flags.RuntimeFile + ").\n\n" + + "Use -- to separate the server name from the volume names to remove.\n\n" + + "Examples:\n" + + " # Remove a single volume mapping\n" + + " mcpd config volumes remove filesystem -- --workspace\n\n" + + " # Remove multiple volume mappings\n" + + " mcpd config volumes remove filesystem -- --workspace --gdrive", + RunE: c.run, + Args: validateRemoveArgs, + } + + return cobraCmd, nil +} + +// validateRemoveArgs validates the arguments for the remove command. +// It wraps validateRemoveArgsCore to extract the dash position from the cobra command. +func validateRemoveArgs(cmd *cobra.Command, args []string) error { + return validateRemoveArgsCore(cmd.ArgsLenAtDash(), args) +} + +// validateRemoveArgsCore validates the remove command arguments given the dash position and args slice. +func validateRemoveArgsCore(dashPos int, args []string) error { + if len(args) == 0 { + return fmt.Errorf("server-name is required") + } + if dashPos == -1 { + return fmt.Errorf( + "missing '--' separator: usage: mcpd config volumes remove -- --", + ) + } + // -- at position 0 means no server name before it. + if dashPos < 1 { + return fmt.Errorf("server-name is required") + } + if strings.TrimSpace(args[0]) == "" { + return fmt.Errorf("server-name is required") + } + if dashPos > 1 { + return fmt.Errorf("too many arguments before --") + } + if len(args) < 2 { + return fmt.Errorf("volume name(s) required after --") + } + return nil +} + +// run executes the remove command, deleting volume mappings from the server config. +func (c *removeCmd) run(cobraCmd *cobra.Command, args []string) error { + serverName := strings.TrimSpace(args[0]) + + // volumeArgs contains everything after the -- separator. + // validateRemoveArgs guarantees len(args) >= 2. + volumeArgs := args[1:] + volumeNames, err := parseRemoveArgs(volumeArgs) + if err != nil { + return err + } + + cfg, err := c.ctxLoader.Load(flags.RuntimeFile) + if err != nil { + return fmt.Errorf("failed to load execution context config: %w", err) + } + + // Get returns a value copy; safe to modify. + server, ok := cfg.Get(serverName) + if !ok { + return fmt.Errorf("server '%s' not found in configuration", serverName) + } + + working := maps.Clone(server.RawVolumes) + if working == nil { + working = context.VolumeExecutionContext{} + } + for _, name := range volumeNames { + delete(working, name) + } + server = withVolumes(server, working) + + res, err := cfg.Upsert(server) + if err != nil { + return fmt.Errorf("error removing volumes for server '%s': %w", serverName, err) + } + + sorted := slices.Clone(volumeNames) + slices.Sort(sorted) + + out := cobraCmd.OutOrStdout() + + var msg string + switch res { + case context.Noop: + msg = fmt.Sprintf("No changes — specified volumes not present on server '%s': %v", serverName, sorted) + default: + msg = fmt.Sprintf("✓ Volumes removed for server '%s' (operation: %s): %v", serverName, string(res), sorted) + } + + if _, err := fmt.Fprintln(out, msg); err != nil { + return fmt.Errorf("failed to write output: %w", err) + } + + return nil +} + +// parseRemoveArgs parses volume name arguments in the format --name. +func parseRemoveArgs(args []string) ([]string, error) { + names := make([]string, 0, len(args)) + + for _, arg := range args { + if !strings.HasPrefix(arg, "--") { + return nil, fmt.Errorf("invalid volume name '%s': must start with --", arg) + } + + name := strings.TrimSpace(strings.TrimPrefix(arg, "--")) + if name == "" { + return nil, fmt.Errorf("volume name cannot be empty in '%s'", arg) + } + + names = append(names, name) + } + + return names, nil +} diff --git a/cmd/config/volumes/remove_test.go b/cmd/config/volumes/remove_test.go new file mode 100644 index 0000000..79e2c89 --- /dev/null +++ b/cmd/config/volumes/remove_test.go @@ -0,0 +1,383 @@ +package volumes + +import ( + "bytes" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mozilla-ai/mcpd/internal/cmd" + cmdopts "github.com/mozilla-ai/mcpd/internal/cmd/options" + "github.com/mozilla-ai/mcpd/internal/context" +) + +func TestNewRemoveCmd(t *testing.T) { + t.Parallel() + + base := &cmd.BaseCmd{} + c, err := NewRemoveCmd(base) + require.NoError(t, err) + require.NotNil(t, c) + + require.True(t, strings.HasPrefix(c.Use, "remove ")) + require.Contains(t, c.Short, "Remove volume mappings") + require.NotNil(t, c.RunE) +} + +func TestRemoveCmd_run(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + serverName string + volumeArgs []string + existingServers map[string]context.ServerExecutionContext + expectedOutput string + expectedError string + expectedVolumes context.VolumeExecutionContext + }{ + { + name: "remove single volume", + serverName: "filesystem", + volumeArgs: []string{"--workspace"}, + existingServers: map[string]context.ServerExecutionContext{ + "filesystem": { + Name: "filesystem", + Volumes: context.VolumeExecutionContext{ + "workspace": "/Users/foo/repos", + "gdrive": "/mcp/gdrive", + }, + RawVolumes: context.VolumeExecutionContext{ + "workspace": "/Users/foo/repos", + "gdrive": "/mcp/gdrive", + }, + }, + }, + expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: updated): [workspace]", + expectedVolumes: context.VolumeExecutionContext{"gdrive": "/mcp/gdrive"}, + }, + { + name: "remove multiple volumes", + serverName: "filesystem", + volumeArgs: []string{"--workspace", "--gdrive"}, + existingServers: map[string]context.ServerExecutionContext{ + "filesystem": { + Name: "filesystem", + Volumes: context.VolumeExecutionContext{ + "workspace": "/Users/foo/repos", + "gdrive": "/mcp/gdrive", + "data": "vol", + }, + RawVolumes: context.VolumeExecutionContext{ + "workspace": "/Users/foo/repos", + "gdrive": "/mcp/gdrive", + "data": "vol", + }, + }, + }, + expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: updated): [gdrive workspace]", + expectedVolumes: context.VolumeExecutionContext{"data": "vol"}, + }, + { + name: "remove all volumes", + serverName: "filesystem", + volumeArgs: []string{"--workspace"}, + existingServers: map[string]context.ServerExecutionContext{ + "filesystem": { + Name: "filesystem", + Volumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, + RawVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, + }, + }, + expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: deleted): [workspace]", + expectedVolumes: context.VolumeExecutionContext{}, + }, + { + name: "remove nonexistent volume is a noop", + serverName: "filesystem", + volumeArgs: []string{"--nonexistent"}, + existingServers: map[string]context.ServerExecutionContext{ + "filesystem": { + Name: "filesystem", + Volumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, + RawVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, + }, + }, + expectedOutput: "No changes — specified volumes not present on server 'filesystem': [nonexistent]", + expectedVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, + }, + { + name: "server not found", + serverName: "nonexistent", + volumeArgs: []string{"--workspace"}, + existingServers: map[string]context.ServerExecutionContext{}, + expectedError: "server 'nonexistent' not found in configuration", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + modifier := &mockModifier{ + servers: tc.existingServers, + } + loader := &mockLoader{modifier: modifier} + + base := &cmd.BaseCmd{} + removeCmd, err := NewRemoveCmd(base, cmdopts.WithContextLoader(loader)) + require.NoError(t, err) + + // Capture output. + var output bytes.Buffer + removeCmd.SetOut(&output) + removeCmd.SetErr(&output) + + // Build args with -- separator. + allArgs := append([]string{tc.serverName, "--"}, tc.volumeArgs...) + removeCmd.SetArgs(allArgs) + + err = removeCmd.Execute() + + if tc.expectedError != "" { + require.EqualError(t, err, tc.expectedError) + return + } + + require.NoError(t, err) + + actualOutput := strings.TrimSpace(output.String()) + require.Equal(t, tc.expectedOutput, actualOutput) + + // Verify the volumes were updated correctly. + if tc.expectedVolumes != nil { + require.Equal(t, tc.expectedVolumes, modifier.lastUpsert.Volumes) + require.Equal(t, tc.expectedVolumes, modifier.lastUpsert.RawVolumes) + } + }) + } +} + +func TestRemoveCmd_Validation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + expectedError string + }{ + { + name: "missing server name", + args: []string{"--", "--workspace"}, + expectedError: "server-name is required", + }, + { + name: "empty server name", + args: []string{"", "--", "--workspace"}, + expectedError: "server-name is required", + }, + { + name: "no volume names after separator", + args: []string{"server", "--"}, + expectedError: "volume name(s) required after --", + }, + { + name: "missing -- separator", + args: []string{"server"}, + expectedError: "missing '--' separator: usage: mcpd config volumes remove -- --", + }, + { + name: "invalid volume name - no prefix", + args: []string{"server", "--", "workspace"}, + expectedError: "invalid volume name 'workspace': must start with --", + }, + { + name: "empty volume name", + args: []string{"server", "--", "--"}, + expectedError: "volume name cannot be empty in '--'", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + modifier := &mockModifier{ + servers: map[string]context.ServerExecutionContext{}, + } + loader := &mockLoader{modifier: modifier} + + base := &cmd.BaseCmd{} + removeCmd, err := NewRemoveCmd(base, cmdopts.WithContextLoader(loader)) + require.NoError(t, err) + + removeCmd.SetArgs(tc.args) + err = removeCmd.Execute() + require.EqualError(t, err, tc.expectedError) + }) + } +} + +func TestRemoveCmd_LoaderError(t *testing.T) { + t.Parallel() + + loader := &mockLoader{ + loadError: fmt.Errorf("failed to load"), + } + + base := &cmd.BaseCmd{} + removeCmd, err := NewRemoveCmd(base, cmdopts.WithContextLoader(loader)) + require.NoError(t, err) + + removeCmd.SetArgs([]string{"server", "--", "--workspace"}) + err = removeCmd.Execute() + require.EqualError(t, err, "failed to load execution context config: failed to load") +} + +func TestRemoveCmd_UpsertError(t *testing.T) { + t.Parallel() + + modifier := &mockModifier{ + servers: map[string]context.ServerExecutionContext{ + "server": { + Name: "server", + Volumes: context.VolumeExecutionContext{"workspace": "/path"}, + RawVolumes: context.VolumeExecutionContext{"workspace": "/path"}, + }, + }, + upsertError: fmt.Errorf("upsert failed"), + } + loader := &mockLoader{modifier: modifier} + + base := &cmd.BaseCmd{} + removeCmd, err := NewRemoveCmd(base, cmdopts.WithContextLoader(loader)) + require.NoError(t, err) + + removeCmd.SetArgs([]string{"server", "--", "--workspace"}) + err = removeCmd.Execute() + require.EqualError(t, err, "error removing volumes for server 'server': upsert failed") +} + +func TestValidateRemoveArgsCore(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + dashPos int + expectedError string + }{ + { + name: "valid args", + args: []string{"server", "--workspace"}, + dashPos: 1, + }, + { + name: "no args at all", + args: []string{}, + dashPos: -1, + expectedError: "server-name is required", + }, + { + name: "missing dash separator", + args: []string{"server"}, + dashPos: -1, + expectedError: "missing '--' separator: usage: mcpd config volumes remove -- --", + }, + { + name: "empty server name", + args: []string{"", "--workspace"}, + dashPos: 1, + expectedError: "server-name is required", + }, + { + name: "whitespace only server name", + args: []string{" ", "--workspace"}, + dashPos: 1, + expectedError: "server-name is required", + }, + { + name: "dash at position 0", + args: []string{"--workspace"}, + dashPos: 0, + expectedError: "server-name is required", + }, + { + name: "too many args before dash", + args: []string{"server", "extra", "--workspace"}, + dashPos: 2, + expectedError: "too many arguments before --", + }, + { + name: "no volume names after dash", + args: []string{"server"}, + dashPos: 1, + expectedError: "volume name(s) required after --", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := validateRemoveArgsCore(tc.dashPos, tc.args) + + if tc.expectedError != "" { + require.EqualError(t, err, tc.expectedError) + return + } + + require.NoError(t, err) + }) + } +} + +func TestParseRemoveArgs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + expected []string + expectedError string + }{ + { + name: "single volume name", + args: []string{"--workspace"}, + expected: []string{"workspace"}, + }, + { + name: "multiple volume names", + args: []string{"--workspace", "--gdrive"}, + expected: []string{"workspace", "gdrive"}, + }, + { + name: "missing -- prefix", + args: []string{"workspace"}, + expectedError: "invalid volume name 'workspace': must start with --", + }, + { + name: "empty name after prefix", + args: []string{"--"}, + expectedError: "volume name cannot be empty in '--'", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + result, err := parseRemoveArgs(tc.args) + + if tc.expectedError != "" { + require.EqualError(t, err, tc.expectedError) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expected, result) + }) + } +} diff --git a/cmd/config/volumes/set.go b/cmd/config/volumes/set.go index c2884ea..c8f9808 100644 --- a/cmd/config/volumes/set.go +++ b/cmd/config/volumes/set.go @@ -54,13 +54,13 @@ func NewSetCmd(baseCmd *cmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, } // validateSetArgs validates the arguments for the set command. -// It wraps validateArgs to extract the dash position from the cobra command. +// It wraps validateSetArgsCore to extract the dash position from the cobra command. func validateSetArgs(cmd *cobra.Command, args []string) error { - return validateArgs(cmd.ArgsLenAtDash(), args) + return validateSetArgsCore(cmd.ArgsLenAtDash(), args) } -// validateArgs validates the set command arguments given the dash position and args slice. -func validateArgs(dashPos int, args []string) error { +// validateSetArgsCore validates the set command arguments given the dash position and args slice. +func validateSetArgsCore(dashPos int, args []string) error { // No args at all. if len(args) == 0 { return fmt.Errorf("server-name is required") @@ -71,8 +71,11 @@ func validateArgs(dashPos int, args []string) error { "missing '--' separator: usage: mcpd config volumes set -- --=", ) } - // -- at position 0 (no server name before it) or server name is empty. - if dashPos < 1 || strings.TrimSpace(args[0]) == "" { + // -- at position 0 means no server name before it. + if dashPos < 1 { + return fmt.Errorf("server-name is required") + } + if strings.TrimSpace(args[0]) == "" { return fmt.Errorf("server-name is required") } if dashPos > 1 { @@ -85,7 +88,7 @@ func validateArgs(dashPos int, args []string) error { } // run executes the set command, parsing volume mappings and updating the server config. -func (c *setCmd) run(cmd *cobra.Command, args []string) error { +func (c *setCmd) run(cobraCmd *cobra.Command, args []string) error { serverName := strings.TrimSpace(args[0]) // volumeArgs contains everything after the -- separator. @@ -101,29 +104,19 @@ func (c *setCmd) run(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to load execution context config: %w", err) } + // Get returns a value copy; safe to modify. server, exists := cfg.Get(serverName) if !exists { server.Name = serverName } - // Clone into a working map to avoid mutating the config's internal state. - // Use RawVolumes as the source of truth to avoid persisting expanded - // environment variables (e.g. ${MCPD__...} placeholders) back to disk. - // Fall back to Volumes if RawVolumes is nil to preserve existing mappings. working := maps.Clone(server.RawVolumes) - if working == nil { - working = maps.Clone(server.Volumes) - } if working == nil { working = context.VolumeExecutionContext{} } - + // Overwrite existing mappings for provided volume names. maps.Copy(working, volumeMap) - - // Assign the working map back so Upsert persists unexpanded values. - // RawVolumes is the single source of truth; Volumes is derived. - server.RawVolumes = working - server.Volumes = maps.Clone(working) + server = withVolumes(server, working) res, err := cfg.Upsert(server) if err != nil { @@ -132,10 +125,18 @@ func (c *setCmd) run(cmd *cobra.Command, args []string) error { volumeNames := slices.Collect(maps.Keys(volumeMap)) slices.Sort(volumeNames) - if _, err := fmt.Fprintf( - cmd.OutOrStdout(), - "✓ Volumes set for server '%s' (operation: %s): %v\n", serverName, string(res), volumeNames, - ); err != nil { + + out := cobraCmd.OutOrStdout() + + var msg string + switch res { + case context.Noop: + msg = fmt.Sprintf("No changes — volumes already match for server '%s': %v", serverName, volumeNames) + default: + msg = fmt.Sprintf("✓ Volumes set for server '%s' (operation: %s): %v", serverName, string(res), volumeNames) + } + + if _, err := fmt.Fprintln(out, msg); err != nil { return fmt.Errorf("failed to write output: %w", err) } @@ -143,30 +144,28 @@ func (c *setCmd) run(cmd *cobra.Command, args []string) error { } // parseVolumeArgs parses volume arguments in the format --name=path or --name="path". -func parseVolumeArgs(args []string) (map[string]string, error) { - volumes := make(map[string]string, len(args)) +func parseVolumeArgs(args []string) (context.VolumeExecutionContext, error) { + volumes := make(context.VolumeExecutionContext, len(args)) for _, arg := range args { - originalArg := arg - - // Expect format: --name=path + // Expect format: --name=path. if !strings.HasPrefix(arg, "--") { return nil, fmt.Errorf("invalid volume format '%s': must start with --", arg) } // Remove the -- prefix for parsing. - arg = strings.TrimPrefix(arg, "--") + trimmed := strings.TrimPrefix(arg, "--") - parts := strings.SplitN(arg, "=", 2) + parts := strings.SplitN(trimmed, "=", 2) if len(parts) != 2 { - return nil, fmt.Errorf("invalid volume format '%s': expected --=", originalArg) + return nil, fmt.Errorf("invalid volume format '%s': expected --=", arg) } name := strings.TrimSpace(parts[0]) path := strings.TrimSpace(parts[1]) if name == "" { - return nil, fmt.Errorf("volume name cannot be empty in '%s'", originalArg) + return nil, fmt.Errorf("volume name cannot be empty in '%s'", arg) } if path == "" { @@ -185,7 +184,9 @@ func parseVolumeArgs(args []string) (map[string]string, error) { return volumes, nil } -// trimQuotes removes surrounding single or double quotes from a string. +// trimQuotes removes a single pair of matching outer quotes from a string. +// This handles cases where quotes survive shell processing (e.g., programmatic +// invocations or certain Windows shells). func trimQuotes(s string) string { if len(s) >= 2 { if (s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'') { diff --git a/cmd/config/volumes/set_test.go b/cmd/config/volumes/set_test.go index 9802844..ee09dac 100644 --- a/cmd/config/volumes/set_test.go +++ b/cmd/config/volumes/set_test.go @@ -38,17 +38,32 @@ func (m *mockModifier) List() []context.ServerExecutionContext { return servers } +// Upsert mirrors the real ExecutionContextConfig.Upsert semantics: +// Noop if empty and not existing, Noop if unchanged, Deleted if emptied, +// Updated if existing, Created if new. func (m *mockModifier) Upsert(ec context.ServerExecutionContext) (context.UpsertResult, error) { m.lastUpsert = ec if m.upsertError != nil { return context.Noop, m.upsertError } - if _, exists := m.servers[ec.Name]; exists { + + current, exists := m.servers[ec.Name] + + switch { + case !exists && ec.IsEmpty(): + return context.Noop, nil + case exists && current.Equals(ec): + return context.Noop, nil + case ec.IsEmpty(): + delete(m.servers, ec.Name) + return context.Deleted, nil + case exists: m.servers[ec.Name] = ec return context.Updated, nil + default: + m.servers[ec.Name] = ec + return context.Created, nil } - m.servers[ec.Name] = ec - return context.Created, nil } // mockLoader implements context.Loader for testing. @@ -72,7 +87,7 @@ func TestNewSetCmd(t *testing.T) { require.NoError(t, err) require.NotNil(t, c) - require.Equal(t, "set", c.Use[:3]) + require.True(t, strings.HasPrefix(c.Use, "set ")) require.Contains(t, c.Short, "Set or update volume mappings") require.NotNil(t, c.RunE) } @@ -111,19 +126,6 @@ func TestSetCmd_run(t *testing.T) { expectedOutput: "✓ Volumes set for server 'test-server' (operation: updated): [gdrive]", expectedVolumes: context.VolumeExecutionContext{"workspace": "/existing/path", "gdrive": "/mcp/gdrive"}, }, - { - name: "add volume to existing server with nil RawVolumes", - serverName: "test-server", - volumeArgs: []string{"--gdrive=/mcp/gdrive"}, - existingServers: map[string]context.ServerExecutionContext{ - "test-server": { - Name: "test-server", - Volumes: context.VolumeExecutionContext{"workspace": "/existing/path"}, - }, - }, - expectedOutput: "✓ Volumes set for server 'test-server' (operation: updated): [gdrive]", - expectedVolumes: context.VolumeExecutionContext{"workspace": "/existing/path", "gdrive": "/mcp/gdrive"}, - }, { name: "update existing volume", serverName: "test-server", @@ -162,6 +164,20 @@ func TestSetCmd_run(t *testing.T) { expectedOutput: "✓ Volumes set for server 'test-server' (operation: created): [data]", expectedVolumes: context.VolumeExecutionContext{"data": "my-named-volume"}, }, + { + name: "set same value is noop", + serverName: "test-server", + volumeArgs: []string{"--workspace=/existing/path"}, + existingServers: map[string]context.ServerExecutionContext{ + "test-server": { + Name: "test-server", + Volumes: context.VolumeExecutionContext{"workspace": "/existing/path"}, + RawVolumes: context.VolumeExecutionContext{"workspace": "/existing/path"}, + }, + }, + expectedOutput: "No changes — volumes already match for server 'test-server': [workspace]", + expectedVolumes: context.VolumeExecutionContext{"workspace": "/existing/path"}, + }, } for _, tc := range tests { @@ -196,7 +212,7 @@ func TestSetCmd_run(t *testing.T) { require.NoError(t, err) actualOutput := strings.TrimSpace(output.String()) - require.Contains(t, actualOutput, tc.expectedOutput) + require.Equal(t, tc.expectedOutput, actualOutput) // Verify the volumes were set correctly. if tc.expectedVolumes != nil { @@ -311,7 +327,7 @@ func TestSetCmd_UpsertError(t *testing.T) { require.EqualError(t, err, "error setting volumes for server 'server': upsert failed") } -func TestValidateArgs(t *testing.T) { +func TestValidateSetArgsCore(t *testing.T) { t.Parallel() tests := []struct { @@ -374,7 +390,7 @@ func TestValidateArgs(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - err := validateArgs(tc.dashPos, tc.args) + err := validateSetArgsCore(tc.dashPos, tc.args) if tc.expectedError != "" { require.EqualError(t, err, tc.expectedError) @@ -392,28 +408,28 @@ func TestParseVolumeArgs(t *testing.T) { tests := []struct { name string args []string - expected map[string]string + expected context.VolumeExecutionContext expectedError string }{ { name: "single volume", args: []string{"--workspace=/path/to/workspace"}, - expected: map[string]string{"workspace": "/path/to/workspace"}, + expected: context.VolumeExecutionContext{"workspace": "/path/to/workspace"}, }, { name: "multiple volumes", args: []string{"--workspace=/path1", "--gdrive=/path2"}, - expected: map[string]string{"workspace": "/path1", "gdrive": "/path2"}, + expected: context.VolumeExecutionContext{"workspace": "/path1", "gdrive": "/path2"}, }, { name: "volume with double quotes", args: []string{`--workspace="/path with spaces"`}, - expected: map[string]string{"workspace": "/path with spaces"}, + expected: context.VolumeExecutionContext{"workspace": "/path with spaces"}, }, { name: "volume with single quotes", args: []string{`--workspace='/path with spaces'`}, - expected: map[string]string{"workspace": "/path with spaces"}, + expected: context.VolumeExecutionContext{"workspace": "/path with spaces"}, }, { name: "missing -- prefix", From 40fd4309008a96dba970a5b96b94d07946ad602d Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Tue, 3 Mar 2026 16:52:22 +0000 Subject: [PATCH 2/2] fix: reject key=value format in volumes remove args The remove command only accepts volume names (--workspace), not key=value pairs (--workspace=/tmp). Without this check, a mistyped --name=path arg would be parsed as a literal volume name that never matches, resulting in a silent noop. --- cmd/config/volumes/remove.go | 3 +++ cmd/config/volumes/remove_test.go | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/cmd/config/volumes/remove.go b/cmd/config/volumes/remove.go index e9d3cb1..9649014 100644 --- a/cmd/config/volumes/remove.go +++ b/cmd/config/volumes/remove.go @@ -152,6 +152,9 @@ func parseRemoveArgs(args []string) ([]string, error) { if name == "" { return nil, fmt.Errorf("volume name cannot be empty in '%s'", arg) } + if strings.Contains(name, "=") { + return nil, fmt.Errorf("invalid volume name '%s': expected --", arg) + } names = append(names, name) } diff --git a/cmd/config/volumes/remove_test.go b/cmd/config/volumes/remove_test.go index 79e2c89..9a6ac60 100644 --- a/cmd/config/volumes/remove_test.go +++ b/cmd/config/volumes/remove_test.go @@ -363,6 +363,11 @@ func TestParseRemoveArgs(t *testing.T) { args: []string{"--"}, expectedError: "volume name cannot be empty in '--'", }, + { + name: "rejects key=value format", + args: []string{"--workspace=/tmp"}, + expectedError: "invalid volume name '--workspace=/tmp': expected --", + }, } for _, tc := range tests {