feat: add mcpd config volumes remove command#250
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove.go`:
- Around line 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.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cmd/config/volumes/cmd.gocmd/config/volumes/remove.gocmd/config/volumes/remove_test.gocmd/config/volumes/set.gocmd/config/volumes/set_test.go
Implement `mcpd config volumes remove <server-name> -- --<volume-name>` 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
peteski22
force-pushed
the
feature/config-volumes-remove
branch
from
b0e37d9 to
70da343
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove.go`:
- Around line 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.
- Around line 146-153: The arg-parsing loop in remove.go currently accepts flags
like "--workspace=/tmp" as a literal volume name; update the loop that iterates
over args (the for _, arg := range args block) to reject any arg that contains
an '=' after the "--" prefix (e.g., detect strings.Contains(arg[len("--"):],
"=") or check for '=' in the derived name variable) and return an error such as
"invalid volume name '%s': flag assignments are not allowed for remove" instead
of treating them as names; keep the existing empty-name check on name
(strings.TrimSpace(strings.TrimPrefix(arg, "--"))), but add the '=' check before
trimming/using name so remove will not accept "--name=path".
In `@cmd/config/volumes/set.go`:
- Around line 113-116: The current logic initializes working from
server.RawVolumes and falls back to an empty context if nil, which can drop
existing mappings in server.Volumes; change the initialization so working is
maps.Clone(server.RawVolumes) if non-nil, otherwise maps.Clone(server.Volumes)
(and if both are nil use an empty context like
context.VolumeExecutionContext{}), updating the working variable creation in
set.go to prefer server.Volumes as the fallback before applying updates.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cmd/config/volumes/cmd.gocmd/config/volumes/remove.gocmd/config/volumes/remove_test.gocmd/config/volumes/set.gocmd/config/volumes/set_test.go
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/config/volumes/remove.go (1)
108-111:⚠️ Potential issue | 🟠 MajorUse
server.Volumesas fallback before mutating volume state.At Line 108,
workingis cloned fromserver.RawVolumesonly. IfRawVolumesis nil butVolumesis populated, this path can apply removals against an empty map and then persist that empty state viawithVolumes, causing unintended volume loss.#!/bin/bash # Verify whether RawVolumes is always initialised during load, and where it is assigned. # Expected result: # - If loader guarantees RawVolumes <- Volumes (or raw source), current code may be safe. # - If not, remove.go should fallback to server.Volumes before empty-map initialisation. rg -n --type=go -C4 '\bRawVolumes\b' rg -n --type=go -C6 'func .*Load\(' internal/context rg -n --type=go -C4 '\bwithVolumes\s*\(' cmd/config/volumesSuggested patch
- 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, The code clones only server.RawVolumes into working which can be nil and cause removals to be applied to an empty map; change the logic in remove.go to choose a source map by preferring server.RawVolumes but falling back to server.Volumes before calling maps.Clone (then if both nil, initialize working = context.VolumeExecutionContext{}), and continue to pass working into withVolumes so you don't persist an unintended empty map; update references around maps.Clone(server.RawVolumes), the working variable, and the call to withVolumes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove_test.go`:
- Around line 41-118: Add a new test case in cmd/config/volumes/remove_test.go
within the existing tests slice that covers the regression where RawVolumes is
nil but Volumes is populated: create a case (e.g., name "rawVolumes nil but
volumes present") with serverName "filesystem", volumeArgs like
[]string{"--workspace"}, existingServers containing
context.ServerExecutionContext where Volumes has "workspace": "/Users/foo/repos"
and RawVolumes is nil (omit or set to nil), then assert expectedOutput and
expectedVolumes match the removal behavior (operation updated/deleted as
appropriate) to validate remove logic handles persisted runtime shapes;
reference the test harness that reads existingServers and uses
context.ServerExecutionContext, Volumes and RawVolumes fields to locate the
change.
---
Duplicate comments:
In `@cmd/config/volumes/remove.go`:
- Around line 108-111: The code clones only server.RawVolumes into working which
can be nil and cause removals to be applied to an empty map; change the logic in
remove.go to choose a source map by preferring server.RawVolumes but falling
back to server.Volumes before calling maps.Clone (then if both nil, initialize
working = context.VolumeExecutionContext{}), and continue to pass working into
withVolumes so you don't persist an unintended empty map; update references
around maps.Clone(server.RawVolumes), the working variable, and the call to
withVolumes accordingly.
Extract shared "--" prefix validation and stripping into stripDashPrefix helper, used by both parseRemoveArgs and parseVolumeArgs. Consolidates the flag-format constraint in one place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcpd config volumes remove <server-name> -- --<volume-name> [--<volume-name>...]command to remove volume mappings from MCP server runtime configurationssetcommand: noop-aware messaging, consistent validation naming (validateSetArgsCore), clearerwithVolumesdoc commentwithVolumeshelper shared by both commandsTest plan
go test ./cmd/config/volumes/... -count=1passesgo test ./... -count=1passes (full suite)deletedresult)--separator, invalid volume format, empty volume nameCloses #200
Summary by CodeRabbit
New Features
Improvements
Tests