From f6e8702ca7a5c3c60e293aa6bd638145bb9d7bad Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Wed, 29 Oct 2025 15:23:22 +0000 Subject: [PATCH 1/3] Add plugins 'add' command Implement 'mcpd config plugins add' command to add new plugins to a category. --- cmd/config/plugins/add.go | 159 +++++++++++- cmd/config/plugins/add_test.go | 360 ++++++++++++++++++++++++++ cmd/config/plugins/cmd_test.go | 7 +- internal/config/plugin_config.go | 39 ++- internal/config/plugin_config_test.go | 148 +++++++++++ 5 files changed, 700 insertions(+), 13 deletions(-) create mode 100644 cmd/config/plugins/add_test.go diff --git a/cmd/config/plugins/add.go b/cmd/config/plugins/add.go index f9c7b6a..525da86 100644 --- a/cmd/config/plugins/add.go +++ b/cmd/config/plugins/add.go @@ -2,24 +2,163 @@ package plugins import ( "fmt" + "maps" + "slices" + "strings" "github.com/spf13/cobra" - "github.com/mozilla-ai/mcpd/v2/internal/cmd" - "github.com/mozilla-ai/mcpd/v2/internal/cmd/options" + internalcmd "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" ) -// NewAddCmd creates the add command for plugins. -// TODO: Implement in a future PR. -func NewAddCmd(baseCmd *cmd.BaseCmd, opt ...options.CmdOption) (*cobra.Command, error) { +const ( + flagFlow = "flow" + flagRequired = "required" + flagCommitHash = "commit-hash" +) + +// AddCmd represents the command for adding a new plugin entry. +// NOTE: Use NewAddCmd to create instances of AddCmd. +type AddCmd struct { + *internalcmd.BaseCmd + + // cfgLoader is used to load the configuration. + cfgLoader config.Loader + + // category is the category to add the plugin to. + category config.Category + + // flows is the list of flows. + flows []string + + // required indicates if the plugin is required. + required bool + + // commitHash is the optional commit hash for version validation. + commitHash string +} + +// NewAddCmd creates a new add command for plugin entries. +func NewAddCmd(baseCmd *internalcmd.BaseCmd, opt ...cmdopts.CmdOption) (*cobra.Command, error) { + opts, err := cmdopts.NewOptions(opt...) + if err != nil { + return nil, err + } + + c := &AddCmd{ + BaseCmd: baseCmd, + cfgLoader: opts.ConfigLoader, + } + cobraCmd := &cobra.Command{ - Use: "add", + Use: "add ", Short: "Add a new plugin entry to a category", - Long: "Add a new plugin entry to a category. The configuration is saved automatically.", - RunE: func(cmd *cobra.Command, args []string) error { - return fmt.Errorf("not yet implemented") - }, + Long: `Add a new plugin entry to a category. The configuration is saved automatically. + +The plugin name must exactly match the name of the plugin binary file. + +This command creates new plugin entries only. If a plugin with the same name already exists +in the category, the command fails with an error. To update an existing plugin, use the 'set' command.`, + Example: ` # Add new plugin with all fields + mcpd config plugins add jwt-auth --category=authentication --flow=request --required + + # Add plugin with multiple flows + mcpd config plugins add metrics --category=observability --flow=request --flow=response --commit-hash=abc123 + + # Add without required flag (defaults to false) + mcpd config plugins add rbac --category=authorization --flow=response`, + RunE: c.run, + Args: cobra.ExactArgs(1), // plugin-name } + allowedCategories := config.OrderedCategories() + cobraCmd.Flags().Var( + &c.category, + flagCategory, + fmt.Sprintf("Specify the category (one of: %s)", allowedCategories.String()), + ) + _ = cobraCmd.MarkFlagRequired(flagCategory) + + cobraCmd.Flags().StringArrayVar( + &c.flows, + flagFlow, + nil, + "Flow to execute plugin in: request, response (can be repeated)", + ) + _ = cobraCmd.MarkFlagRequired(flagFlow) + + cobraCmd.Flags().BoolVar( + &c.required, + flagRequired, + false, + "Optional, mark plugin as required", + ) + + cobraCmd.Flags().StringVar( + &c.commitHash, + flagCommitHash, + "", + "Optional, commit hash for runtime version validation", + ) + return cobraCmd, nil } + +func (c *AddCmd) run(cmd *cobra.Command, args []string) error { + pluginName := strings.TrimSpace(args[0]) + if pluginName == "" { + return fmt.Errorf("plugin name cannot be empty") + } + + cfg, err := c.LoadConfig(c.cfgLoader) + if err != nil { + return err + } + + if _, exists := cfg.Plugin(c.category, pluginName); exists { + return fmt.Errorf( + "plugin '%s' already exists in category '%s'\n\n"+ + "To update an existing plugin, use: mcpd config plugins set %s --category=%s [flags]", + pluginName, + c.category, + pluginName, + c.category, + ) + } + + flowsMap := config.ParseFlows(c.flows) + if len(flowsMap) == 0 { + return fmt.Errorf("at least one valid flow is required (must be 'request' or 'response')") + } + + parsedFlows := slices.Sorted(maps.Keys(flowsMap)) + + entry := config.PluginEntry{ + Name: pluginName, + Flows: parsedFlows, + } + + // Set optional fields only if they were provided. + if cmd.Flags().Changed(flagRequired) { + entry.Required = &c.required + } + + if c.commitHash != "" { + entry.CommitHash = &c.commitHash + } + + if _, err := cfg.UpsertPlugin(c.category, entry); err != nil { + return err + } + + _, _ = fmt.Fprintf( + cmd.OutOrStdout(), + "✓ Plugin '%s' added to category '%s'\n", + pluginName, + c.category, + ) + + return nil +} diff --git a/cmd/config/plugins/add_test.go b/cmd/config/plugins/add_test.go new file mode 100644 index 0000000..fa1e543 --- /dev/null +++ b/cmd/config/plugins/add_test.go @@ -0,0 +1,360 @@ +package plugins + +import ( + "bytes" + "os" + "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" +) + +// createTempConfigFile creates a temporary config file for testing. +func createTempConfigFile(t *testing.T) string { + t.Helper() + + tempFile, err := os.CreateTemp(t.TempDir(), ".mcpd.toml") + require.NoError(t, err) + + // Write minimal valid config. + content := "servers = []\n" + require.NoError(t, os.WriteFile(tempFile.Name(), []byte(content), 0o644)) + + return tempFile.Name() +} + +// mockLoaderFromFile creates a mock loader that loads from a real temp file. +// This allows the config to be saved properly during tests. +type mockLoaderFromFile struct { + filePath string + loader *config.DefaultLoader +} + +func newMockLoaderFromFile(t *testing.T) *mockLoaderFromFile { + t.Helper() + + return &mockLoaderFromFile{ + filePath: createTempConfigFile(t), + loader: &config.DefaultLoader{}, + } +} + +func (m *mockLoaderFromFile) Load(_ string) (config.Modifier, error) { + return m.loader.Load(m.filePath) +} + +func TestNewAddCmd(t *testing.T) { + t.Parallel() + + base := &cmd.BaseCmd{} + c, err := NewAddCmd(base) + require.NoError(t, err) + require.NotNil(t, c) + + require.Equal(t, "add ", c.Use) + require.Contains(t, c.Short, "Add a new plugin entry") + require.NotNil(t, c.RunE) +} + +func TestAddCmd_Success(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt-auth"}) + require.NoError(t, err) + require.Empty(t, stderr.String()) + require.Contains(t, stdout.String(), "✓ Plugin 'jwt-auth' added to category 'authentication'") + + // Verify plugin was added by reloading the config. + cfg, err := loader.Load("") + require.NoError(t, err) + + authPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryAuthentication) + require.Len(t, authPlugins, 1) + require.Equal(t, "jwt-auth", authPlugins[0].Name) + require.Equal(t, []config.Flow{config.FlowRequest}, authPlugins[0].Flows) +} + +func TestAddCmd_WithAllFields(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "observability") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "response") + require.NoError(t, err) + err = addCmd.Flags().Set(flagRequired, "true") + require.NoError(t, err) + err = addCmd.Flags().Set(flagCommitHash, "abc123") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"metrics"}) + require.NoError(t, err) + require.Empty(t, stderr.String()) + require.Contains(t, stdout.String(), "✓ Plugin 'metrics' added to category 'observability'") + + // Verify plugin was added with all fields by reloading the config. + cfg, err := loader.Load("") + require.NoError(t, err) + + obsPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryObservability) + require.Len(t, obsPlugins, 1) + require.Equal(t, "metrics", obsPlugins[0].Name) + require.Equal(t, []config.Flow{config.FlowRequest, config.FlowResponse}, obsPlugins[0].Flows) + require.NotNil(t, obsPlugins[0].Required) + require.True(t, *obsPlugins[0].Required) + require.NotNil(t, obsPlugins[0].CommitHash) + require.Equal(t, "abc123", *obsPlugins[0].CommitHash) +} + +func TestAddCmd_PluginAlreadyExists(t *testing.T) { + t.Parallel() + + cfg := &config.Config{ + Plugins: &config.PluginConfig{ + Dir: "/path/to/plugins", + Authentication: []config.PluginEntry{{Name: "jwt-auth", Flows: []config.Flow{config.FlowRequest}}}, + }, + } + + mockLoader := &mockLoader{cfg: cfg} + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(mockLoader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt-auth"}) + require.Error(t, err) + require.Contains(t, err.Error(), "plugin 'jwt-auth' already exists in category 'authentication'") + require.Contains(t, err.Error(), "To update an existing plugin, use: mcpd config plugins set") +} + +func TestAddCmd_InvalidFlows(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "invalid") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt"}) + require.Error(t, err) + require.Contains(t, err.Error(), "at least one valid flow is required") +} + +func TestAddCmd_DuplicateFlows(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt"}) + require.NoError(t, err) + require.Empty(t, stderr.String()) + + // Verify duplicates were deduplicated - should only have one flow. + cfg, err := loader.Load("") + require.NoError(t, err) + + authPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryAuthentication) + require.Len(t, authPlugins, 1) + require.Equal(t, "jwt", authPlugins[0].Name) + require.Equal(t, []config.Flow{config.FlowRequest}, authPlugins[0].Flows) +} + +func TestAddCmd_EmptyName(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{" "}) + require.Error(t, err) + require.Contains(t, err.Error(), "plugin name cannot be empty") +} + +func TestAddCmd_InvalidCategory(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "invalid-category") + require.Error(t, err) + require.ErrorContains(t, err, "invalid category 'invalid-category'") +} + +func TestAddCmd_CaseInsensitiveFlows(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "rate_limiting") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "REQUEST") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "RESPONSE") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"rate-limiter"}) + require.NoError(t, err) + require.Empty(t, stderr.String()) + + // Verify plugin was added by reloading the config. + cfg, err := loader.Load("") + require.NoError(t, err) + + rateLimitPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryRateLimiting) + require.Len(t, rateLimitPlugins, 1) + require.Equal(t, "rate-limiter", rateLimitPlugins[0].Name) + require.Equal(t, []config.Flow{config.FlowRequest, config.FlowResponse}, rateLimitPlugins[0].Flows) +} + +func TestAddCmd_RequiredFalseNotSet(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt-auth"}) + require.NoError(t, err) + + // Verify Required field is nil (not set) by reloading the config. + cfg, err := loader.Load("") + require.NoError(t, err) + + authPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryAuthentication) + require.Len(t, authPlugins, 1) + require.Nil(t, authPlugins[0].Required) +} + +func TestAddCmd_RequiredFalseExplicitlySet(t *testing.T) { + t.Parallel() + + loader := newMockLoaderFromFile(t) + base := &cmd.BaseCmd{} + addCmd, err := NewAddCmd(base, cmdopts.WithConfigLoader(loader)) + require.NoError(t, err) + + err = addCmd.Flags().Set(flagCategory, "authentication") + require.NoError(t, err) + err = addCmd.Flags().Set(flagFlow, "request") + require.NoError(t, err) + err = addCmd.Flags().Set(flagRequired, "false") + require.NoError(t, err) + + var stdout bytes.Buffer + var stderr bytes.Buffer + addCmd.SetOut(&stdout) + addCmd.SetErr(&stderr) + + err = addCmd.RunE(addCmd, []string{"jwt-auth"}) + require.NoError(t, err) + + // Verify Required field is set to false by reloading the config. + cfg, err := loader.Load("") + require.NoError(t, err) + + authPlugins := cfg.(*config.Config).Plugins.ListPlugins(config.CategoryAuthentication) + require.Len(t, authPlugins, 1) + require.NotNil(t, authPlugins[0].Required) + require.False(t, *authPlugins[0].Required) +} diff --git a/cmd/config/plugins/cmd_test.go b/cmd/config/plugins/cmd_test.go index 727f298..ee7e031 100644 --- a/cmd/config/plugins/cmd_test.go +++ b/cmd/config/plugins/cmd_test.go @@ -1,6 +1,7 @@ package plugins_test import ( + "strings" "testing" "github.com/stretchr/testify/require" @@ -35,8 +36,10 @@ func TestPlugins_NewCmd_Success(t *testing.T) { } for _, command := range commands { - require.True(t, expectedCmds[command.Use], "unexpected command: %s", command.Use) - delete(expectedCmds, command.Use) + // Extract command name (first word) from Use field to handle cases like "add ". + cmdName := strings.Fields(command.Use)[0] + require.True(t, expectedCmds[cmdName], "unexpected command: %s", command.Use) + delete(expectedCmds, cmdName) } require.Empty(t, expectedCmds, "missing expected commands") diff --git a/internal/config/plugin_config.go b/internal/config/plugin_config.go index a3ce304..c64fb22 100644 --- a/internal/config/plugin_config.go +++ b/internal/config/plugin_config.go @@ -3,11 +3,13 @@ package config import ( "errors" "fmt" + "maps" "slices" "strings" "github.com/mozilla-ai/mcpd/v2/internal/context" "github.com/mozilla-ai/mcpd/v2/internal/files" + "github.com/mozilla-ai/mcpd/v2/internal/filter" ) const ( @@ -54,6 +56,13 @@ var orderedCategories = Categories{ CategoryAudit, // Last. } +// flows defines the set of valid flow types. +// NOTE: This variable should not be modified in other parts of the codebase. +var flows = map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, +} + // PluginModifier defines operations for managing plugin configuration. type PluginModifier interface { // Plugin retrieves a plugin by category and name. @@ -78,6 +87,34 @@ type Category string // Flow represents the execution phase for a plugin. type Flow string +// Flows returns the canonical set of allowed flows. +// Returns a clone to prevent modification of the internal map. +func Flows() map[Flow]struct{} { + return maps.Clone(flows) +} + +// IsValid returns true if the Flow is a recognized value. +func (f Flow) IsValid() bool { + _, ok := flows[f] + return ok +} + +// ParseFlows validates and reduces flow strings to a distinct set. +// Flow strings are normalized before validation. +// Invalid flows are silently ignored. Returns an empty map if no valid flows are found. +func ParseFlows(flags []string) map[Flow]struct{} { + valid := make(map[Flow]struct{}, len(flows)) + + for _, s := range flags { + f := Flow(filter.NormalizeString(s)) + if _, ok := flows[f]; ok { + valid[f] = struct{}{} + } + } + + return valid +} + // PluginConfig represents the top-level plugin configuration. // // NOTE: if you add/remove fields you must review the associated validation implementation. @@ -208,7 +245,7 @@ func (e *PluginEntry) Validate() error { seen := make(map[Flow]struct{}) for _, flow := range e.Flows { // Check for valid flow values. - if flow != FlowRequest && flow != FlowResponse { + if !flow.IsValid() { validationErrors = append( validationErrors, fmt.Errorf("invalid flow '%s', must be '%s' or '%s'", flow, FlowRequest, FlowResponse), diff --git a/internal/config/plugin_config_test.go b/internal/config/plugin_config_test.go index ed1a856..4990d87 100644 --- a/internal/config/plugin_config_test.go +++ b/internal/config/plugin_config_test.go @@ -724,6 +724,154 @@ func TestPluginConfig_Validate_errorMessages(t *testing.T) { }) } +func TestFlows(t *testing.T) { + t.Parallel() + + flows := Flows() + + // Should contain exactly request and response. + require.Len(t, flows, 2) + require.Contains(t, flows, FlowRequest) + require.Contains(t, flows, FlowResponse) + + // Verify that modifications don't affect subsequent calls (clone behavior). + delete(flows, FlowRequest) + require.Len(t, flows, 1) + + // Get a fresh copy - should still have both flows. + freshFlows := Flows() + require.Len(t, freshFlows, 2) + require.Contains(t, freshFlows, FlowRequest) + require.Contains(t, freshFlows, FlowResponse) +} + +func TestFlow_IsValid(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + flow Flow + valid bool + }{ + { + name: "valid request flow", + flow: FlowRequest, + valid: true, + }, + { + name: "valid response flow", + flow: FlowResponse, + valid: true, + }, + { + name: "invalid empty flow", + flow: Flow(""), + valid: false, + }, + { + name: "invalid unknown flow", + flow: Flow("unknown"), + valid: false, + }, + { + name: "invalid uppercase", + flow: Flow("REQUEST"), + valid: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + result := tc.flow.IsValid() + require.Equal(t, tc.valid, result) + }) + } +} + +func TestParseFlows(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input []string + expected map[Flow]struct{} + }{ + { + name: "single valid flow", + input: []string{"request"}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + }, + }, + { + name: "two valid flows", + input: []string{"request", "response"}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, + }, + }, + { + name: "duplicates are deduplicated", + input: []string{"request", "request", "response", "response"}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, + }, + }, + { + name: "invalid flows are ignored", + input: []string{"invalid", "foo", "bar"}, + expected: map[Flow]struct{}{}, + }, + { + name: "mixed valid and invalid", + input: []string{"request", "invalid", "response", "foo"}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, + }, + }, + { + name: "empty input", + input: []string{}, + expected: map[Flow]struct{}{}, + }, + { + name: "nil input", + input: nil, + expected: map[Flow]struct{}{}, + }, + { + name: "case insensitive", + input: []string{"REQUEST", "Response", "REQUEST"}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, + }, + }, + { + name: "with whitespace", + input: []string{" request ", " response "}, + expected: map[Flow]struct{}{ + FlowRequest: {}, + FlowResponse: {}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + result := ParseFlows(tc.input) + require.Equal(t, tc.expected, result) + }) + } +} + func testPluginStringPtr(t *testing.T, s string) *string { t.Helper() return &s From 554773b4fa8f881e8e9caf9591a9bfb2229bfd77 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Thu, 30 Oct 2025 09:15:42 +0000 Subject: [PATCH 2/3] PR feedback --- cmd/config/plugins/add.go | 18 ++++++---- internal/config/plugin_config.go | 34 ++++++++++++------- internal/config/plugin_config_test.go | 47 +++++++++++++++++++++------ 3 files changed, 70 insertions(+), 29 deletions(-) diff --git a/cmd/config/plugins/add.go b/cmd/config/plugins/add.go index 525da86..7947bc5 100644 --- a/cmd/config/plugins/add.go +++ b/cmd/config/plugins/add.go @@ -85,7 +85,10 @@ in the category, the command fails with an error. To update an existing plugin, &c.flows, flagFlow, nil, - "Flow to execute plugin in: request, response (can be repeated)", + fmt.Sprintf( + "Flow during which, the plugin should execute (%s) (can be repeated)", + strings.Join(config.OrderedFlowNames(), ", "), + ), ) _ = cobraCmd.MarkFlagRequired(flagFlow) @@ -128,16 +131,17 @@ func (c *AddCmd) run(cmd *cobra.Command, args []string) error { ) } - flowsMap := config.ParseFlows(c.flows) - if len(flowsMap) == 0 { - return fmt.Errorf("at least one valid flow is required (must be 'request' or 'response')") + flows := config.ParseFlowsDistinct(c.flows) + if len(flows) == 0 { + return fmt.Errorf( + "at least one valid flow is required (%s)", + strings.Join(config.OrderedFlowNames(), ", "), + ) } - parsedFlows := slices.Sorted(maps.Keys(flowsMap)) - entry := config.PluginEntry{ Name: pluginName, - Flows: parsedFlows, + Flows: slices.Sorted(maps.Keys(flows)), } // Set optional fields only if they were provided. diff --git a/internal/config/plugin_config.go b/internal/config/plugin_config.go index c64fb22..280232c 100644 --- a/internal/config/plugin_config.go +++ b/internal/config/plugin_config.go @@ -99,10 +99,10 @@ func (f Flow) IsValid() bool { return ok } -// ParseFlows validates and reduces flow strings to a distinct set. +// ParseFlowsDistinct validates and reduces flow strings to a distinct set. // Flow strings are normalized before validation. // Invalid flows are silently ignored. Returns an empty map if no valid flows are found. -func ParseFlows(flags []string) map[Flow]struct{} { +func ParseFlowsDistinct(flags []string) map[Flow]struct{} { valid := make(map[Flow]struct{}, len(flows)) for _, s := range flags { @@ -244,12 +244,10 @@ func (e *PluginEntry) Validate() error { } else { seen := make(map[Flow]struct{}) for _, flow := range e.Flows { - // Check for valid flow values. if !flow.IsValid() { - validationErrors = append( - validationErrors, - fmt.Errorf("invalid flow '%s', must be '%s' or '%s'", flow, FlowRequest, FlowResponse), - ) + allowedFlows := strings.Join(OrderedFlowNames(), ", ") + err := fmt.Errorf("invalid flow '%s' (allowed: %s)", flow, allowedFlows) + validationErrors = append(validationErrors, err) } // Check for duplicates. @@ -386,7 +384,9 @@ func (p *PluginConfig) plugin(category Category, name string) (PluginEntry, bool // upsertPlugin creates or updates a plugin entry. func (p *PluginConfig) upsertPlugin(category Category, entry PluginEntry) (context.UpsertResult, error) { - if strings.TrimSpace(entry.Name) == "" { + // Handle sanitizing the plugin name. + entry.Name = strings.TrimSpace(entry.Name) + if entry.Name == "" { return context.Noop, fmt.Errorf("plugin name cannot be empty") } @@ -399,11 +399,9 @@ func (p *PluginConfig) upsertPlugin(category Category, entry PluginEntry) (conte return context.Noop, err } - name := strings.TrimSpace(entry.Name) - // Check if plugin already exists. for i, existing := range *slice { - if existing.Name != name { + if existing.Name != entry.Name { continue } @@ -526,10 +524,22 @@ func OrderedCategories() Categories { return slices.Clone(orderedCategories) } +// OrderedFlowNames returns the names of allowed flows in order. +func OrderedFlowNames() []string { + sortedFlows := slices.Sorted(maps.Keys(flows)) + + flowNames := make([]string, len(sortedFlows)) + for i, f := range sortedFlows { + flowNames[i] = string(f) + } + + return flowNames +} + // Set is used by Cobra to set the category value from a string. // NOTE: This is also required by Cobra as part of implementing flag.Value. func (c *Category) Set(v string) error { - v = strings.ToLower(strings.TrimSpace(v)) + v = filter.NormalizeString(v) allowed := OrderedCategories() for _, a := range allowed { diff --git a/internal/config/plugin_config_test.go b/internal/config/plugin_config_test.go index 4990d87..8e50382 100644 --- a/internal/config/plugin_config_test.go +++ b/internal/config/plugin_config_test.go @@ -8,6 +8,16 @@ import ( "github.com/mozilla-ai/mcpd/v2/internal/context" ) +func testPluginStringPtr(t *testing.T, s string) *string { + t.Helper() + return &s +} + +func testPluginBoolPtr(t *testing.T, b bool) *bool { + t.Helper() + return &b +} + func TestPluginEntry_Validate(t *testing.T) { t.Parallel() @@ -395,6 +405,7 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { entry PluginEntry wantResult context.UpsertResult wantErr bool + wantName string }{ { name: "create new plugin", @@ -404,6 +415,7 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { Name: "jwt-auth", Flows: []Flow{FlowRequest}, }, + wantName: "jwt-auth", wantResult: context.Created, wantErr: false, }, @@ -419,6 +431,7 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { Name: "jwt-auth", Flows: []Flow{FlowRequest, FlowResponse}, }, + wantName: "jwt-auth", wantResult: context.Updated, wantErr: false, }, @@ -434,6 +447,7 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { Name: "jwt-auth", Flows: []Flow{FlowRequest}, }, + wantName: "jwt-auth", wantResult: context.Noop, wantErr: false, }, @@ -459,6 +473,18 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { wantResult: context.Noop, wantErr: true, }, + { + name: "trim whitespace", + initial: &PluginConfig{}, + category: CategoryAuthentication, + entry: PluginEntry{ + Name: " jwt-auth ", + Flows: []Flow{FlowRequest}, + }, + wantName: "jwt-auth", + wantResult: context.Created, + wantErr: false, + }, } for _, tc := range tests { @@ -471,6 +497,10 @@ func TestPluginConfig_upsertPlugin(t *testing.T) { require.Error(t, err) } else { require.NoError(t, err) + + updated, found := tc.initial.plugin(tc.category, tc.wantName) + require.True(t, found) + require.Equal(t, tc.wantName, updated.Name) } require.Equal(t, tc.wantResult, result) @@ -790,7 +820,7 @@ func TestFlow_IsValid(t *testing.T) { } } -func TestParseFlows(t *testing.T) { +func TestParseFlowsDistinct(t *testing.T) { t.Parallel() tests := []struct { @@ -866,18 +896,15 @@ func TestParseFlows(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - result := ParseFlows(tc.input) + result := ParseFlowsDistinct(tc.input) require.Equal(t, tc.expected, result) }) } } -func testPluginStringPtr(t *testing.T, s string) *string { - t.Helper() - return &s -} - -func testPluginBoolPtr(t *testing.T, b bool) *bool { - t.Helper() - return &b +func TestAddCmd_OrderedFlowNames(t *testing.T) { + flows := OrderedFlowNames() + require.Len(t, flows, 2) + require.Equal(t, "request", flows[0]) + require.Equal(t, "response", flows[1]) } From 685ce68749bcc651dcc475c53b5d9f4010774d5e Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Thu, 30 Oct 2025 16:34:22 +0000 Subject: [PATCH 3/3] Bump mcp-go to v0.42.0 (#220) --- NOTICE | 4 ++-- go.mod | 2 +- go.sum | 4 ++-- internal/api/mcp.go | 27 ++++++++------------------- internal/api/resources.go | 20 ++------------------ 5 files changed, 15 insertions(+), 42 deletions(-) diff --git a/NOTICE b/NOTICE index 276eba7..002d10e 100644 --- a/NOTICE +++ b/NOTICE @@ -289,9 +289,9 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI -## github.com/mark3labs/mcp-go (v0.41.1) +## github.com/mark3labs/mcp-go (v0.42.0) -License: [MIT](https://github.com/mark3labs/mcp-go/blob/v0.41.1/LICENSE) +License: [MIT](https://github.com/mark3labs/mcp-go/blob/v0.42.0/LICENSE) MIT License diff --git a/go.mod b/go.mod index 810d2ef..ac9acdd 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/go-chi/chi/v5 v5.2.3 github.com/go-chi/cors v1.2.2 github.com/hashicorp/go-hclog v1.6.3 - github.com/mark3labs/mcp-go v0.41.1 + github.com/mark3labs/mcp-go v0.42.0 github.com/mozilla-ai/mcpd-plugins-sdk-go v0.0.2 github.com/spf13/cobra v1.10.1 github.com/spf13/pflag v1.0.10 diff --git a/go.sum b/go.sum index 2eeb1b9..8205ac3 100644 --- a/go.sum +++ b/go.sum @@ -46,8 +46,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= github.com/mailru/easyjson v0.9.0/go.mod h1:1+xMtQp2MRNVL/V1bOzuP3aP8VNwRW55fQUto+XFtTU= -github.com/mark3labs/mcp-go v0.41.1 h1:w78eWfiQam2i8ICL7AL0WFiq7KHNJQ6UB53ZVtH4KGA= -github.com/mark3labs/mcp-go v0.41.1/go.mod h1:T7tUa2jO6MavG+3P25Oy/jR7iCeJPHImCZHRymCn39g= +github.com/mark3labs/mcp-go v0.42.0 h1:gk/8nYJh8t3yroCAOBhNbYsM9TCKvkM13I5t5Hfu6Ls= +github.com/mark3labs/mcp-go v0.42.0/go.mod h1:YnJfOL382MIWDx1kMY+2zsRHU/q78dBg9aFb8W6Thdw= github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= github.com/mattn/go-colorable v0.1.12/go.mod h1:u5H1YNBxpqRaxsYJYSkiCWKzEfiAb1Gb520KVy5xxl4= github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= diff --git a/internal/api/mcp.go b/internal/api/mcp.go index a22ebdb..077a2f6 100644 --- a/internal/api/mcp.go +++ b/internal/api/mcp.go @@ -1,6 +1,10 @@ package api -import "github.com/mark3labs/mcp-go/mcp" +import ( + "maps" + + "github.com/mark3labs/mcp-go/mcp" +) // DomainMeta wraps mcp.Meta for API conversion. type DomainMeta mcp.Meta @@ -13,25 +17,10 @@ type Meta map[string]any // Returns empty Meta{} if domain type is nil. // See: https://modelcontextprotocol.io/specification/2025-06-18/basic/index#meta func (d DomainMeta) ToAPIType() (Meta, error) { - if (*mcp.Meta)(&d) == nil { + m := (*mcp.Meta)(&d) + if m == nil || m.AdditionalFields == nil { return Meta{}, nil } - // The _meta field is MCP's reserved extensibility mechanism that allows both: - // - progressToken: for out-of-band progress notifications (defined by spec) - // - Additional fields: custom metadata from servers/clients (extensible) - // Both types of fields are merged at the same level in the resulting map. - result := make(Meta) - - // Add progressToken if present (using MCP spec-defined field name). - if d.ProgressToken != nil { - result["progressToken"] = d.ProgressToken - } - - // Merge additional fields at the same level. - for k, v := range d.AdditionalFields { - result[k] = v - } - - return result, nil + return maps.Clone(m.AdditionalFields), nil } diff --git a/internal/api/resources.go b/internal/api/resources.go index 411dfbc..42a1c82 100644 --- a/internal/api/resources.go +++ b/internal/api/resources.go @@ -296,34 +296,18 @@ func handleServerResourceContent( for _, content := range result.Contents { switch c := content.(type) { case mcp.TextResourceContents: - var meta Meta - if c.Meta != nil { - var err error - meta, err = DomainMeta(*c.Meta).ToAPIType() - if err != nil { - return nil, err - } - } contents = append(contents, ResourceContent{ URI: c.URI, MIMEType: c.MIMEType, Text: c.Text, - Meta: meta, + Meta: c.Meta, }) case mcp.BlobResourceContents: - var meta Meta - if c.Meta != nil { - var err error - meta, err = DomainMeta(*c.Meta).ToAPIType() - if err != nil { - return nil, err - } - } contents = append(contents, ResourceContent{ URI: c.URI, MIMEType: c.MIMEType, Blob: c.Blob, - Meta: meta, + Meta: c.Meta, }) } }