PR feedback

Author: peteski22

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.

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 {

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])
 }