Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions cmd/config/volumes/cmd.go
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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 {
Expand All @@ -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
}
163 changes: 163 additions & 0 deletions cmd/config/volumes/remove.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
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 <server-name> -- --<volume-name> [--<volume-name>...]",
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 <server-name> -- --<volume-name>",
)
}
// -- 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{}
}
Comment on lines +108 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add a fallback to server.Volumes when RawVolumes is nil.

At Line 108, removal starts from server.RawVolumes only. If RawVolumes is unset but Volumes is populated, the command behaves as if no volumes exist.

Suggested change
-	working := maps.Clone(server.RawVolumes)
-	if working == nil {
-		working = context.VolumeExecutionContext{}
-	}
+	working := maps.Clone(server.RawVolumes)
+	if working == nil {
+		working = maps.Clone(server.Volumes)
+	}
+	if working == nil {
+		working = context.VolumeExecutionContext{}
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
working := maps.Clone(server.RawVolumes)
if working == nil {
working = context.VolumeExecutionContext{}
}
working := maps.Clone(server.RawVolumes)
if working == nil {
working = maps.Clone(server.Volumes)
}
if working == nil {
working = context.VolumeExecutionContext{}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/config/volumes/remove.go` around lines 108 - 111, Currently the code
clones server.RawVolumes into working and falls back to an empty
context.VolumeExecutionContext{}, which ignores server.Volumes when RawVolumes
is nil; change the initialization so you first attempt
maps.Clone(server.RawVolumes), if that result is nil then attempt
maps.Clone(server.Volumes), and only if that is still nil set working =
context.VolumeExecutionContext{}; update the code around the working variable
assignment (the clone of server.RawVolumes) so it checks server.Volumes as the
secondary source.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the output strings could be magic-ified (so tests can reuse them and they can be easily changed later on).

}

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)
}
if strings.Contains(name, "=") {
return nil, fmt.Errorf("invalid volume name '%s': expected --<volume-name>", arg)
}

names = append(names, name)
}

return names, nil
Comment on lines +142 to +162
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider extracting shared prefix-stripping logic.

Both parseRemoveArgs and parseVolumeArgs (in set.go) share similar -- prefix validation and stripping logic. While the formats differ (--name vs --name=path), a small helper for prefix validation could reduce duplication.

This is a minor observation; the current implementation is clear and correct.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/config/volumes/remove.go` around lines 142 - 159, parseRemoveArgs
duplicates the same "--" prefix validation/stripping logic used by
parseVolumeArgs; extract a small helper like ensurePrefixed(arg string, prefix
string) (or stripLeadingDashes(arg string)) that verifies strings.HasPrefix(arg,
"--"), returns the remainder (trimmed) or an error, and use it from both
parseRemoveArgs and parseVolumeArgs to centralize the validation and trimming
logic; keep the existing empty-name checks (e.g., name == "") in each caller
as-needed.

}
Loading