Add plugins 'add' command #219
Conversation
Implement 'mcpd config plugins add' command to add new plugins to a category.
WalkthroughImplements a fully functional plugins Add command with flags and run logic, adds canonical flow handling and validation utilities in the config package, and expands tests covering add behaviour, flow normalisation/validation and config persistence. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/config/plugin_config.go (2)
248-253: Avoid hard‑coded allowed flow list in errorBuild the allowed list from the canonical set to prevent drift.
Apply this diff:
- if !flow.IsValid() { - validationErrors = append( - validationErrors, - fmt.Errorf("invalid flow '%s', must be '%s' or '%s'", flow, FlowRequest, FlowResponse), - ) - } + if !flow.IsValid() { + allowed := slices.Sorted(maps.Keys(Flows())) + validationErrors = append( + validationErrors, + fmt.Errorf("invalid flow %q, must be one of %v", flow, allowed), + ) + }
388-396: Normalise and persist trimmed plugin name in upsertCurrently you trim for comparison but not for persistence, which can leave leading/trailing spaces in stored names added by other paths. Trim early and reuse.
Apply this diff:
func (p *PluginConfig) upsertPlugin(category Category, entry PluginEntry) (context.UpsertResult, error) { - if strings.TrimSpace(entry.Name) == "" { + entry.Name = strings.TrimSpace(entry.Name) + if entry.Name == "" { return context.Noop, fmt.Errorf("plugin name cannot be empty") } if err := entry.Validate(); err != nil { return context.Noop, fmt.Errorf("plugin validation failed: %w", err) } @@ - name := strings.TrimSpace(entry.Name) + name := entry.NameAlso applies to: 402-416
♻️ Duplicate comments (1)
cmd/config/plugins/add.go (1)
5-7: Confirm Go toolchain version for maps/slices usageAs noted elsewhere, maps/slices require Go 1.21+. Please verify go.mod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cmd/config/plugins/add.go(1 hunks)cmd/config/plugins/add_test.go(1 hunks)cmd/config/plugins/cmd_test.go(2 hunks)internal/config/plugin_config.go(4 hunks)internal/config/plugin_config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/config/plugin_config.go (1)
internal/filter/match.go (1)
NormalizeString(36-38)
internal/config/plugin_config_test.go (1)
internal/config/plugin_config.go (5)
Flows(92-94)FlowRequest(40-40)FlowResponse(43-43)Flow(88-88)ParseFlows(105-116)
cmd/config/plugins/add_test.go (5)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)cmd/config/plugins/add.go (1)
NewAddCmd(44-107)internal/cmd/options/options.go (1)
WithConfigLoader(43-48)internal/config/types.go (1)
Config(83-88)internal/config/plugin_config.go (9)
CategoryAuthentication(17-17)Flow(88-88)FlowRequest(40-40)Flows(92-94)CategoryObservability(32-32)FlowResponse(43-43)PluginConfig(121-145)PluginEntry(148-161)CategoryRateLimiting(23-23)
cmd/config/plugins/add.go (3)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)internal/config/plugin_config.go (5)
Category(85-85)OrderedCategories(525-527)ParseFlows(105-116)PluginEntry(148-161)Flows(92-94)internal/cmd/options/options.go (1)
CmdOption(10-10)
🔇 Additional comments (27)
internal/config/plugin_config.go (3)
90-95: Flows() clone is correctReturning a clone prevents accidental mutation. LGTM.
96-101: Flow.IsValid() is straightforwardEfficient membership check against canonical set. LGTM.
6-6: Go version requirement confirmedThe go.mod specifies go 1.25.1, which exceeds the required Go 1.21+ for maps.Clone and maps.Keys usage. No action required.
internal/config/plugin_config_test.go (3)
727-746: Flows() clone behaviour verifiedGood assertions on exact members and mutation isolation. LGTM.
748-791: Flow.IsValid() coverage is adequateCovers valid, empty, unknown, and case. LGTM.
793-873: ParseFlows happy/edge paths well coveredDedup, mixed validity, nil/empty, case and whitespace all covered. LGTM.
cmd/config/plugins/add.go (7)
16-20: Flag constants read wellNames are consistent with existing commands. LGTM.
22-41: Command state struct looks fineClear separation of parsed flags and wiring. LGTM.
120-129: Helpful duplicate guidanceClear error with remediation. LGTM.
136-141: Stable flow ordering is goodUsing slices.Sorted(maps.Keys(...)) gives deterministic serialisation. LGTM.
143-151: Optional fields handling is correctOnly set when flag is provided/non-empty. LGTM.
152-164: Upsert + success messageCorrect save and clear user feedback. LGTM.
109-118: No changes required – guard rails are adequateVerification confirms that
BaseCmd.LoadConfigdoes not support nil loaders (line 191 would panic), but this is not a concern. Theconfig.Loaderis always initialised to&config.DefaultLoader{}through the options defaults mechanism (internal/cmd/options/options.go:20-25), and tests explicitly verify this invariant. No constructor changes are needed – the code is safe as written.cmd/config/plugins/add_test.go (12)
15-27: Temp config helper is cleanMinimal valid TOML and safe perms. LGTM.
49-61: NewAddCmd wiring test is adequateVerifies Use/Short/RunE. LGTM.
62-95: Happy path add with persistence verifiedGood stdout/stderr assertions and reload check. LGTM.
96-137: All fields path coveredValidates flow order, Required pointer behaviour, and commitHash. LGTM.
139-168: Duplicate plugin guarded with actionable errorStrong UX. LGTM.
170-191: Invalid flows properly rejectedAsserts helpful error. LGTM.
193-227: Duplicate flows deduplicatedGood verification via reload. LGTM.
229-250: Empty name rejection testMatches run-time guard. LGTM.
252-263: Invalid category surfaced at flag parseAssertions match config.Category.Set behaviour. LGTM.
265-297: Case-insensitive flows verifiedGood coverage. LGTM.
299-327: Required flag unset yields nil pointerCovers intended tri-state. LGTM.
329-360: Required flag explicitly false setPointer set and value false verified. LGTM.
cmd/config/plugins/cmd_test.go (2)
4-4: Import is appropriatestrings is needed for tokenising Use. LGTM.
39-43: Subcommand detection made robustUsing first word avoids brittle matching against arg hints. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
cmd/config/plugins/add.go(1 hunks)cmd/config/plugins/add_test.go(1 hunks)internal/config/plugin_config.go(4 hunks)internal/config/plugin_config_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T09:54:59.315Z
Learnt from: peteski22
PR: mozilla-ai/mcpd#215
File: internal/plugin/types_test.go:83-90
Timestamp: 2025-10-24T09:54:59.315Z
Learning: In internal/plugin/types_test.go, TestOrderedCategories_ReturnsCorrectOrder uses hard-coded assertions to act as a regression check. If the category order changes in the config package, this test will catch it. This is intentional to ensure order stability.
Applied to files:
internal/config/plugin_config_test.gocmd/config/plugins/add.gocmd/config/plugins/add_test.go
🧬 Code graph analysis (4)
internal/config/plugin_config_test.go (1)
internal/config/plugin_config.go (5)
Flows(92-94)FlowRequest(40-40)FlowResponse(43-43)Flow(88-88)ParseFlowsDistinct(105-116)
cmd/config/plugins/add.go (3)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)internal/config/plugin_config.go (5)
Category(85-85)OrderedCategories(525-527)ParseFlowsDistinct(105-116)PluginEntry(148-161)Flows(92-94)internal/cmd/options/options.go (1)
CmdOption(10-10)
internal/config/plugin_config.go (1)
internal/filter/match.go (1)
NormalizeString(36-38)
cmd/config/plugins/add_test.go (5)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)cmd/config/plugins/add.go (2)
NewAddCmd(44-110)OrderedFlowNames(168-177)internal/cmd/options/options.go (1)
WithConfigLoader(43-48)internal/config/types.go (1)
Config(83-88)internal/config/plugin_config.go (9)
CategoryAuthentication(17-17)Flow(88-88)FlowRequest(40-40)Flows(92-94)CategoryObservability(32-32)FlowResponse(43-43)PluginConfig(121-145)PluginEntry(148-161)CategoryRateLimiting(23-23)
🔇 Additional comments (18)
internal/config/plugin_config_test.go (3)
880-883: LGTM!The helper function follows the established pattern and provides a clean way to create bool pointers for tests.
727-746: LGTM!The test properly validates both the returned flow set contents and the clone behaviour, ensuring modifications to the returned map don't affect the internal canonical set.
748-791: LGTM!The test comprehensively validates the IsValid() method across all expected scenarios, including valid flows, invalid flows, and edge cases like empty strings and incorrect casing.
internal/config/plugin_config.go (6)
6-6: LGTM!The new imports are properly used for cloning the flows map and normalising flow strings.
Also applies to: 12-12
59-64: LGTM!The canonical flows map is appropriately unexported and serves as the single source of truth for valid flow values. This design enables the Flows(), IsValid(), and ParseFlowsDistinct() functions to derive their behaviour from this authoritative set.
90-94: LGTM!The function correctly returns a clone of the internal flows map, preventing external code from modifying the canonical set. The documentation clearly explains this behaviour.
96-100: LGTM!The IsValid() method provides a clean API for validating Flow values against the canonical set.
102-116: LGTM!The function correctly normalises input strings, validates them against the canonical flows set, and returns a distinct set. The documentation clearly states that invalid flows are silently ignored, which is appropriate for this use case.
248-248: LGTM!The validation logic now correctly uses the IsValid() method, ensuring consistency with the canonical flows definition.
cmd/config/plugins/add.go (7)
3-14: LGTM!The imports are appropriate for the command implementation, including standard library packages for data manipulation and internal packages for configuration management.
16-20: LGTM!The flag constants provide a single source of truth for flag names, reducing the risk of typos and making refactoring easier.
22-41: LGTM!The AddCmd struct is well-structured with clear field documentation. The embedding of BaseCmd provides access to common functionality like logging.
43-110: LGTM!The command constructor is well-structured with comprehensive documentation and examples. The flag help text correctly derives the allowed flow names from the canonical source using OrderedFlowNames(), addressing previous review feedback.
112-132: LGTM!The validation logic is robust. The check for existing plugins prevents accidental overwrites, and the error message helpfully directs users to the correct command for updates.
134-152: LGTM!The flow parsing uses the canonical ParseFlowsDistinct function and provides a clear error message that derives allowed flow names from the canonical source. The optional fields are correctly set only when provided, ensuring nil values for unspecified options. Sorting the flows ensures consistent output.
167-177: LGTM!The OrderedFlowNames helper correctly derives the ordered flow names from the canonical flows set, ensuring consistency across the codebase and addressing previous review feedback.
cmd/config/plugins/add_test.go (2)
15-47: LGTM!The test helpers are well-designed. Using a real temporary file with the DefaultLoader enables comprehensive testing of the full configuration persistence flow, providing better coverage than purely in-memory mocks.
49-367: LGTM!The test suite provides comprehensive coverage of the Add command functionality, including:
- Successful plugin addition with minimal and complete field sets
- Error handling for duplicates, invalid inputs, and validation failures
- Edge cases such as case-insensitive flow parsing, deduplication, and optional field handling
- Verification of configuration persistence by reloading after operations
The tests are well-structured, use appropriate assertions, and validate both the command output and the resulting configuration state.
peteski22
force-pushed
the
peteski22/cli-plugins-add
branch
from
9961b65 to
80ea3dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cmd/config/plugins/add.go(1 hunks)internal/config/plugin_config.go(5 hunks)internal/config/plugin_config_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T09:54:59.315Z
Learnt from: peteski22
PR: mozilla-ai/mcpd#215
File: internal/plugin/types_test.go:83-90
Timestamp: 2025-10-24T09:54:59.315Z
Learning: In internal/plugin/types_test.go, TestOrderedCategories_ReturnsCorrectOrder uses hard-coded assertions to act as a regression check. If the category order changes in the config package, this test will catch it. This is intentional to ensure order stability.
Applied to files:
cmd/config/plugins/add.gointernal/config/plugin_config_test.go
🧬 Code graph analysis (3)
cmd/config/plugins/add.go (3)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)internal/config/plugin_config.go (6)
Category(85-85)OrderedCategories(523-525)OrderedFlowNames(528-537)ParseFlowsDistinct(105-116)PluginEntry(148-161)Flows(92-94)internal/cmd/options/options.go (1)
CmdOption(10-10)
internal/config/plugin_config.go (1)
internal/filter/match.go (1)
NormalizeString(36-38)
internal/config/plugin_config_test.go (1)
internal/config/plugin_config.go (6)
Flows(92-94)FlowRequest(40-40)FlowResponse(43-43)Flow(88-88)ParseFlowsDistinct(105-116)OrderedFlowNames(528-537)
🔇 Additional comments (13)
internal/config/plugin_config_test.go (5)
11-19: LGTM: Helper functions reduce duplication.The helper functions correctly use
t.Helper()and will reduce duplication throughout the test file.
737-756: LGTM: Clone behaviour correctly verified.The test properly verifies that
Flows()returns a defensive copy by modifying the returned map and checking that subsequent calls return unmodified data.
758-801: LGTM: Comprehensive validation coverage.The test cases thoroughly cover valid and invalid flow values, including edge cases like empty strings and case sensitivity.
803-883: LGTM: Thorough flow parsing coverage.The test comprehensively covers
ParseFlowsDistinctbehaviour including deduplication, normalisation, case handling, and invalid input filtering.
885-890: LGTM: Order regression check.The hardcoded assertions act as a regression check for flow order stability, consistent with the established pattern for category order tests.
Based on learnings
internal/config/plugin_config.go (4)
6-6: LGTM: Necessary imports for new functionality.The new imports support the flow handling APIs (
mapsfor cloning/keys,internal/filterfor normalisation).Also applies to: 12-12
59-64: LGTM: Canonical flow set correctly defined.The unexported
flowsvariable serves as the single source of truth for valid flow types, with clear documentation warning against modification.
90-116: LGTM: Flow handling APIs well designed.The three functions work together effectively:
Flows()provides safe access to canonical flows via defensive copyIsValid()offers efficient validationParseFlowsDistinct()properly normalises, validates, and deduplicates input with clear documented behaviour
247-250: LGTM: Validation uses canonical sources.The validation correctly uses
IsValid()and derives error messages fromOrderedFlowNames(), eliminating hardcoded flow names and ensuring consistency.cmd/config/plugins/add.go (4)
16-20: LGTM: Flag constants follow best practices.Defining flag names as constants improves maintainability and reduces the risk of typos.
22-41: LGTM: Well-documented command structure.The struct is clearly documented with appropriate fields and usage guidance.
43-110: LGTM: Command setup follows best practices.The function properly:
- Handles options with error checking
- Creates detailed help documentation with examples
- Derives allowed categories and flows from canonical sources
- Marks required flags appropriately
- Configures optional flags with sensible defaults
112-168: LGTM: Run logic is robust and user-friendly.The implementation correctly:
- Validates input and provides clear error messages
- Checks for existing plugins and guides users to the appropriate command
- Parses and validates flows using the canonical APIs
- Handles optional fields properly (only sets when provided/changed)
- Sorts flows for deterministic ordering
- Provides helpful success feedback
peteski22
force-pushed
the
peteski22/cli-plugins-add
branch
from
80ea3dc to
49a507e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cmd/config/plugins/add.go(1 hunks)internal/config/plugin_config.go(7 hunks)internal/config/plugin_config_test.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T09:54:59.315Z
Learnt from: peteski22
PR: mozilla-ai/mcpd#215
File: internal/plugin/types_test.go:83-90
Timestamp: 2025-10-24T09:54:59.315Z
Learning: In internal/plugin/types_test.go, TestOrderedCategories_ReturnsCorrectOrder uses hard-coded assertions to act as a regression check. If the category order changes in the config package, this test will catch it. This is intentional to ensure order stability.
Applied to files:
cmd/config/plugins/add.gointernal/config/plugin_config_test.gointernal/config/plugin_config.go
🧬 Code graph analysis (3)
cmd/config/plugins/add.go (3)
internal/cmd/basecmd.go (1)
BaseCmd(45-47)internal/config/plugin_config.go (6)
Category(85-85)OrderedCategories(523-525)OrderedFlowNames(528-537)ParseFlowsDistinct(105-116)PluginEntry(148-161)Flows(92-94)internal/cmd/options/options.go (1)
CmdOption(10-10)
internal/config/plugin_config_test.go (2)
internal/config/plugin_config.go (9)
PluginConfig(121-145)CategoryAuthentication(17-17)PluginEntry(148-161)Flows(92-94)Flow(88-88)FlowRequest(40-40)FlowResponse(43-43)ParseFlowsDistinct(105-116)OrderedFlowNames(528-537)internal/context/loader.go (1)
Created(28-28)
internal/config/plugin_config.go (1)
internal/filter/match.go (1)
NormalizeString(36-38)
Implement
mcpd config plugins addcommand to add new plugins to a category.Summary by CodeRabbit
New Features
Bug Fixes
Tests