fix: one registrar for every repeatable flag; reject an empty occurrence

CHANGELOG.md

@@ -57,6 +57,19 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Changed
 
+- **BREAKING — repeatable flags no longer split on commas, and an empty
+  occurrence is a usage error (exit 2).** Every repeatable string flag
+  (`--user-id`, `--to-user-id`, `--tool-id`, `--config-field`, `--state`,
+  `--classification`, `--policy-type`, `--exclude-policy-id`, `--query`,
+  `--header`) was a pflag `StringSlice`, which CSV-splits each occurrence and
+  so destroys an empty one during parsing: `--user-id "" --user-id REAL`
+  arrived as `["REAL"]`, too late for any command-level check to see. On
+  `apps set-owners`, which replaces the full owner list, an unset shell
+  variable silently set one owner and exited 0. They are now `StringArray`,
+  registered and read through one shared pair of helpers that reject an empty
+  or whitespace-only occurrence before anything is sent. The break: `--flag
+  a,b` is now one value, not two — repeat the flag instead (`--flag a --flag
+  b`), the form every help string and documented example already used.
 - **BREAKING — a negative `--limit` or `--page-size` is now a usage error
   (exit 2)** instead of being accepted or sent. `--limit` never reaches the
   API, so `--limit -1` had silently behaved exactly like the documented

README.md

@@ -499,6 +499,25 @@ both print identical output.
 - List commands auto-paginate by default. Pass `--page-token` to fetch a single page manually.
 - `--page-size` **requests** a per-call batch size (max 100; `mcp tools history` and `mcp bindings history` allow 200). It is not a guarantee: a page can contain more rows than you asked for, by an amount that varies per endpoint and per size — `apps list --page-size 10` returned 23 rows, `policies list` 12, `users list` exactly 10. A positive value below 5 usually returns 5, though `policies list` floors at 6 and `mcp servers catalog list` has no floor. `--page-size 0` means the server's default of 25, not "none". A value over the max is clamped by c1i rather than rejected. A negative `--page-size` or `--limit` is a usage error (exit 2), rejected before any request. Use `--limit N` for an exact total: it is enforced client-side, so it holds even when a page overshoots, and it stops auto-pagination once reached.
 
+### Repeatable flags
+
+A flag documented as **repeatable** takes one value per occurrence, and a comma
+is a literal character rather than a separator:
+
+```sh
+c1i mcp bindings create --app-id A --connector-id C --toolset-id T \
+  --tool-id tool-a --tool-id tool-b     # two tools
+```
+
+`--tool-id tool-a,tool-b` is one id containing a comma, not two ids. This is
+easy to miss on `--config-field`, where `--config-field "region=us1,env=prod"`
+sets `region` to `us1,env=prod` and the server may accept it.
+
+An empty occurrence is a usage error (exit 2), rejected before any request, so
+an unset shell variable cannot silently shorten the list — which for a
+list-replacing flag like `apps set-owners --user-id` would drop an owner.
+Contrast `--fields`, which *is* comma-separated.
+
 ### Field selection
 
 `--fields` trims every emitted JSON object to just the keys you name — a big

cmd/agents.md

@@ -247,6 +247,11 @@ Two things are irreversible in ways their `--help` doesn't make obvious:
 
 ## Things that will surprise you
 
+- A **repeatable** flag takes one value per occurrence; a comma is literal, not
+  a separator. `--tool-id a,b` is one id, not two. `--config-field
+  "region=us1,env=prod"` sets `region` to `us1,env=prod`, which the server may
+  accept. An empty occurrence is exit 2 before any request, so an unset shell
+  variable cannot silently shorten a list. `--fields` IS comma-separated.
 - Owner and grant provisioning are asynchronous. A read immediately after a
   write can look like a silent no-op for a couple of minutes (owner writes
   observed at 45-150s across set-owners, add-owner, remove-owner and the

cmd/api.go

@@ -57,8 +57,14 @@ var apiCmd = &cobra.Command{
 		bodyFile, _ := cmd.Flags().GetString("body-file")
 		paginate, _ := cmd.Flags().GetBool("paginate")
 		listKey, _ := cmd.Flags().GetString("list-key")
-		queryPairs, _ := cmd.Flags().GetStringArray("query")
-		headerPairs, _ := cmd.Flags().GetStringArray("header")
+		queryPairs, err := repeatableStringFlag(cmd, "query")
+		if err != nil {
+			return err
+		}
+		headerPairs, err := repeatableStringFlag(cmd, "header")
+		if err != nil {
+			return err
+		}
 		allowDeleteBody, _ := cmd.Flags().GetBool("allow-delete-body")
 		limit := getIntFlag(cmd, "limit")
 
@@ -291,8 +297,8 @@ func init() {
 	apiCmd.Flags().String("body", "", "JSON request body (implies POST)")
 	apiCmd.Flags().String("body-file", "", "Read the JSON request body from a file (\"-\" for stdin); mutually exclusive with --body")
 	apiCmd.Flags().Bool("allow-delete-body", false, "Allow --body/--body-file with --method DELETE (some C1 endpoints, e.g. remove-membership, require a body on DELETE; without this flag such a request is refused)")
-	apiCmd.Flags().StringArray("query", nil, "Query parameter as key=value (repeatable)")
-	apiCmd.Flags().StringArray("header", nil, "Extra request header as key=value (repeatable)")
+	addRepeatableStringFlag(apiCmd, "query", "Query parameter as key=value (repeatable)")
+	addRepeatableStringFlag(apiCmd, "header", "Extra request header as key=value (repeatable)")
 	apiCmd.Flags().Bool("paginate", false, "Automatically follow pagination to fetch all pages")
 	apiCmd.Flags().String("list-key", "", "Force the response field name to drain as the list (default: auto-detect the first array-valued field, e.g. 'list', 'automationExecutions', 'automations')")
 	markRequired(apiCmd, "path")

cmd/apps_set_owners.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/ConductorOne/c1i/internal/client"
@@ -43,17 +42,13 @@ Honors --dry-run (with --wait, dry-run still only previews the PUT; it never
 polls).`,
 	Args: cobra.ExactArgs(1),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		userIDs, _ := cmd.Flags().GetStringSlice("user-id")
+		userIDs, err := repeatableStringFlag(cmd, "user-id")
+		if err != nil {
+			return err
+		}
 		if len(userIDs) == 0 {
 			return &usageError{fmt.Errorf("at least one --user-id is required")}
 		}
-		for _, id := range userIDs {
-			if strings.TrimSpace(id) == "" {
-				// An empty id would send userIds:[""] and earn a confusing 4xx
-				// (the API requires a 27-char user id); reject it up front.
-				return &usageError{fmt.Errorf("--user-id values must be non-empty")}
-			}
-		}
 		wait, waitTimeout, err := waitFlagValues(cmd)
 		if err != nil {
 			return err
@@ -145,7 +140,7 @@ func buildSetOwnersBody(userIDs []string) map[string]any {
 }
 
 func init() {
-	appsSetOwnersCmd.Flags().StringSlice("user-id", nil, "C1 user ID to set as owner (repeatable; replaces the full owner list)")
+	addRepeatableStringFlag(appsSetOwnersCmd, "user-id", "C1 user ID to set as owner (repeatable; replaces the full owner list)")
 	markRequired(appsSetOwnersCmd, "user-id")
 	addWaitFlags(appsSetOwnersCmd, "GET .../ownerids until the requested owners appear", 4*time.Minute)
 	appsCmd.AddCommand(appsSetOwnersCmd)

cmd/docs_agents_test.go

@@ -7,6 +7,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"gopkg.in/yaml.v3"
 )
 
 // runDocsAgents drives docsAgentsCmd.RunE directly (no auth, no network)
@@ -175,3 +177,34 @@ func runThroughRoot(t *testing.T, args ...string) string {
 	}
 	return out.String()
 }
+
+// TestAgentsDocOpensWithFrontMatter pins that agents.md starts with its YAML
+// block. `docs agents`'s own help promises the output opens with front matter
+// that harnesses parse, and front matter is only front matter on line 1 — a
+// bullet prepended above it silently demotes name/description/version to prose.
+// Nothing else in the tree checks this, and the whole suite stayed green when
+// it happened.
+func TestAgentsDocOpensWithFrontMatter(t *testing.T) {
+	rendered := strings.ReplaceAll(agentsTemplate, "{{VERSION}}", Version)
+	if !strings.HasPrefix(rendered, "---\n") {
+		first, _, _ := strings.Cut(rendered, "\n")
+		t.Fatalf("agents.md must open with the YAML front-matter delimiter; it starts with %q", first)
+	}
+	rest := strings.TrimPrefix(rendered, "---\n")
+	end := strings.Index(rest, "\n---\n")
+	if end < 0 {
+		t.Fatal("agents.md opens a front-matter block that is never closed")
+	}
+	// Parse it rather than trusting the delimiter search: if the real closing
+	// --- were dropped, that search would run on to any later --- in the body
+	// and report twenty lines of prose as valid front matter.
+	var front map[string]any
+	if err := yaml.Unmarshal([]byte(rest[:end]), &front); err != nil {
+		t.Fatalf("agents.md's front matter is not valid YAML, so a harness parsing it gets nothing: %v", err)
+	}
+	for _, key := range []string{"name", "description", "version", "required_bins"} {
+		if _, ok := front[key]; !ok {
+			t.Errorf("front matter is missing %q, which docs agents' help says harnesses parse", key)
+		}
+	}
+}

cmd/flags.go

@@ -244,3 +244,59 @@ func requireNonEmpty(cmd *cobra.Command, names ...string) error {
 		return &usageError{fmt.Errorf("flags %s require non-empty values", strings.Join(missing, ", "))}
 	}
 }
+
+// addRepeatableStringFlag registers a repeatable string flag. It always uses
+// StringArray, never StringSlice: StringSlice CSV-splits every occurrence, so
+// `--user-id "" --user-id REAL` reaches the command as ["REAL"] — the empty
+// occurrence is destroyed during parsing, before any command-level check can
+// see it. On `apps set-owners`, which REPLACES the owner list, that set one
+// owner and exited 0 while the caller had asked for two.
+//
+// The deliberate trade: `--flag a,b` is now ONE value, not two. No repeatable
+// flag ever documented comma-splitting; see CHANGELOG.
+//
+// TestRepeatableStringFlagsGoThroughSharedRegistrar keeps this the only place
+// such a flag is created.
+func addRepeatableStringFlag(cmd *cobra.Command, name, usage string) {
+	cmd.Flags().StringArray(name, nil, usage)
+}
+
+// repeatableStringFlagError is the one wording for a repeatable flag given an
+// empty value, defined once so its callers cannot drift apart.
+func repeatableStringFlagError(name string) error {
+	return &usageError{fmt.Errorf("flag --%s requires a non-empty value for every occurrence", name)}
+}
+
+// repeatableStringFlag reads a flag registered by addRepeatableStringFlag and
+// rejects an empty or whitespace-only occurrence with a *usageError (exit 2).
+//
+// The two rejected shapes need separate checks. A blank inside a repetition
+// survives as an element (`--x "" --x REAL` -> ["", "REAL"]), but a lone
+// `--x ""` reads back as an EMPTY slice: GetStringArray round-trips the value
+// through a CSV string, and a single empty element serializes to "" which
+// parses back as no elements at all. Only Changed distinguishes that from
+// "flag never passed". Both shapes measured against pflag v1.0.10.
+//
+// Not passing the flag at all is not an error here: whether the flag is
+// required is the command's business, and several callers treat it as optional.
+func repeatableStringFlag(cmd *cobra.Command, name string) ([]string, error) {
+	values, err := cmd.Flags().GetStringArray(name)
+	if err != nil {
+		// Wrong flag type, not user input: reporting it as an empty value would
+		// send the reader to fix their command line instead of the code.
+		return nil, fmt.Errorf("--%s is not a repeatable string flag: %w", name, err)
+	}
+	f := cmd.Flags().Lookup(name)
+	if f == nil || !f.Changed {
+		return values, nil
+	}
+	if len(values) == 0 {
+		return nil, repeatableStringFlagError(name)
+	}
+	for _, v := range values {
+		if strings.TrimSpace(v) == "" {
+			return nil, repeatableStringFlagError(name)
+		}
+	}
+	return values, nil
+}

cmd/mcp_bindings_by_tools.go

@@ -26,7 +26,10 @@ no bindings are still emitted with an empty toolsets array.`,
 
 		appID, _ := cmd.Flags().GetString("app-id")
 		connectorID, _ := cmd.Flags().GetString("connector-id")
-		toolIDs, _ := cmd.Flags().GetStringSlice("tool-id")
+		toolIDs, err := repeatableStringFlag(cmd, "tool-id")
+		if err != nil {
+			return err
+		}
 		if len(toolIDs) == 0 {
 			return &usageError{fmt.Errorf("flag --tool-id requires at least one value")}
 		}
@@ -78,7 +81,7 @@ no bindings are still emitted with an empty toolsets array.`,
 func init() {
 	mcpBindingsByToolsCmd.Flags().String("app-id", "", "Application ID")
 	mcpBindingsByToolsCmd.Flags().String("connector-id", "", "Connector ID")
-	mcpBindingsByToolsCmd.Flags().StringSlice("tool-id", nil, "MCP tool ID to look up (repeatable; max 32)")
+	addRepeatableStringFlag(mcpBindingsByToolsCmd, "tool-id", "MCP tool ID to look up (repeatable; max 32)")
 	markRequired(mcpBindingsByToolsCmd, "app-id", "connector-id", "tool-id")
 	mcpBindingsCmd.AddCommand(mcpBindingsByToolsCmd)
 }

cmd/mcp_bindings_create.go

@@ -23,7 +23,10 @@ var mcpBindingsCreateCmd = &cobra.Command{
 		appID, _ := cmd.Flags().GetString("app-id")
 		connectorID, _ := cmd.Flags().GetString("connector-id")
 		toolsetID, _ := cmd.Flags().GetString("toolset-id")
-		toolIDs, _ := cmd.Flags().GetStringSlice("tool-id")
+		toolIDs, err := repeatableStringFlag(cmd, "tool-id")
+		if err != nil {
+			return err
+		}
 		if len(toolIDs) == 0 {
 			return &usageError{fmt.Errorf("flag --tool-id requires at least one value")}
 		}
@@ -57,7 +60,7 @@ func init() {
 	mcpBindingsCreateCmd.Flags().String("app-id", "", "Application ID")
 	mcpBindingsCreateCmd.Flags().String("connector-id", "", "Connector ID")
 	mcpBindingsCreateCmd.Flags().String("toolset-id", "", "MCP toolset (access profile) ID")
-	mcpBindingsCreateCmd.Flags().StringSlice("tool-id", nil, "MCP tool ID to bind (repeatable; max 100)")
+	addRepeatableStringFlag(mcpBindingsCreateCmd, "tool-id", "MCP tool ID to bind (repeatable; max 100)")
 	markRequired(mcpBindingsCreateCmd, "app-id", "connector-id", "toolset-id", "tool-id")
 	mcpBindingsCmd.AddCommand(mcpBindingsCreateCmd)
 }

cmd/mcp_bindings_delete.go

@@ -28,7 +28,10 @@ in the request body; HTTP DELETE doesn't reliably support that.`,
 		appID, _ := cmd.Flags().GetString("app-id")
 		connectorID, _ := cmd.Flags().GetString("connector-id")
 		toolsetID, _ := cmd.Flags().GetString("toolset-id")
-		toolIDs, _ := cmd.Flags().GetStringSlice("tool-id")
+		toolIDs, err := repeatableStringFlag(cmd, "tool-id")
+		if err != nil {
+			return err
+		}
 		if len(toolIDs) == 0 {
 			return &usageError{fmt.Errorf("flag --tool-id requires at least one value")}
 		}
@@ -67,7 +70,7 @@ func init() {
 	mcpBindingsDeleteCmd.Flags().String("app-id", "", "Application ID")
 	mcpBindingsDeleteCmd.Flags().String("connector-id", "", "Connector ID")
 	mcpBindingsDeleteCmd.Flags().String("toolset-id", "", "MCP toolset (access profile) ID")
-	mcpBindingsDeleteCmd.Flags().StringSlice("tool-id", nil, "MCP tool ID to unbind (repeatable; max 100)")
+	addRepeatableStringFlag(mcpBindingsDeleteCmd, "tool-id", "MCP tool ID to unbind (repeatable; max 100)")
 	markRequired(mcpBindingsDeleteCmd, "app-id", "connector-id", "toolset-id", "tool-id")
 	mcpBindingsCmd.AddCommand(mcpBindingsDeleteCmd)
 }

cmd/mcp_servers_config.go

@@ -195,9 +195,12 @@ func buildExternalConfig(cmd *cobra.Command) (map[string]any, error) {
 	return cfg, nil
 }
 
-// parseKeyValues reads a repeatable "key=value" string-slice flag into a map.
+// parseKeyValues reads a repeatable "key=value" flag into a map.
 func parseKeyValues(cmd *cobra.Command, name string) (map[string]string, error) {
-	pairs, _ := cmd.Flags().GetStringSlice(name)
+	pairs, err := repeatableStringFlag(cmd, name)
+	if err != nil {
+		return nil, err
+	}
 	if len(pairs) == 0 {
 		return nil, nil
 	}

cmd/mcp_servers_register.go

@@ -119,7 +119,11 @@ func buildRegisterBody(cmd *cobra.Command) (map[string]any, error) {
 	if v, _ := cmd.Flags().GetString("tool-prefix"); v != "" {
 		body["toolPrefix"] = v
 	}
-	if ids, _ := cmd.Flags().GetStringSlice("user-id"); len(ids) > 0 {
+	ids, err := repeatableStringFlag(cmd, "user-id")
+	if err != nil {
+		return nil, err
+	}
+	if len(ids) > 0 {
 		body["userIds"] = ids
 	}
 
@@ -160,11 +164,11 @@ func init() {
 	mcpServersRegisterCmd.Flags().String("description", "", "Description")
 	mcpServersRegisterCmd.Flags().String("data-sensitivity", "", "Data sensitivity: public, internal, confidential, restricted")
 	mcpServersRegisterCmd.Flags().String("tool-prefix", "", "Prefix for exposed tool names")
-	mcpServersRegisterCmd.Flags().StringSlice("user-id", nil, "Integration owner user ID (repeatable)")
+	addRepeatableStringFlag(mcpServersRegisterCmd, "user-id", "Integration owner user ID (repeatable)")
 	// HOSTED config
 	mcpServersRegisterCmd.Flags().String("catalog-id", "", "Catalog entry ID (HOSTED)")
 	mcpServersRegisterCmd.Flags().String("source-app-id", "", "Source app ID for connector-backed HOSTED servers")
-	mcpServersRegisterCmd.Flags().StringSlice("config-field", nil, "Extra config field key=value (HOSTED, repeatable)")
+	addRepeatableStringFlag(mcpServersRegisterCmd, "config-field", "Extra config field key=value (HOSTED, repeatable)")
 	mcpServersRegisterCmd.Flags().String("hosted-config-file", "", "Full hostedConfig JSON (file or \"-\" for stdin)")
 	// EXTERNAL config
 	mcpServersRegisterCmd.Flags().String("server-url", "", "External MCP server URL (EXTERNAL)")

cmd/mcp_servers_test.go

@@ -104,7 +104,12 @@ func TestFlexInt64(t *testing.T) {
 
 func TestParseKeyValues(t *testing.T) {
 	cmd := &cobra.Command{}
-	cmd.Flags().StringSlice("config-field", []string{"region=us1", "env=prod"}, "")
+	addRepeatableStringFlag(cmd, "config-field", "")
+	for _, pair := range []string{"region=us1", "env=prod"} {
+		if err := cmd.Flags().Set("config-field", pair); err != nil {
+			t.Fatalf("setting --config-field: %v", err)
+		}
+	}
 	got, err := parseKeyValues(cmd, "config-field")
 	if err != nil {
 		t.Fatalf("parseKeyValues: %v", err)
@@ -115,7 +120,10 @@ func TestParseKeyValues(t *testing.T) {
 	}
 
 	bad := &cobra.Command{}
-	bad.Flags().StringSlice("config-field", []string{"noequals"}, "")
+	addRepeatableStringFlag(bad, "config-field", "")
+	if err := bad.Flags().Set("config-field", "noequals"); err != nil {
+		t.Fatalf("setting --config-field: %v", err)
+	}
 	if _, err := parseKeyValues(bad, "config-field"); err == nil {
 		t.Error("expected error for missing '='")
 	}
@@ -132,10 +140,10 @@ func newServerFlagCmd() *cobra.Command {
 	f.String("description", "", "")
 	f.String("data-sensitivity", "", "")
 	f.String("tool-prefix", "", "")
-	f.StringSlice("user-id", nil, "")
+	addRepeatableStringFlag(cmd, "user-id", "")
 	f.String("catalog-id", "", "")
 	f.String("source-app-id", "", "")
-	f.StringSlice("config-field", nil, "")
+	addRepeatableStringFlag(cmd, "config-field", "")
 	f.String("hosted-config-file", "", "")
 	f.String("server-url", "", "")
 	f.String("transport", "", "")

cmd/mcp_servers_update_credentials.go

@@ -150,7 +150,7 @@ func init() {
 	// HOSTED config
 	mcpServersUpdateCredentialsCmd.Flags().String("catalog-id", "", "Catalog entry ID (HOSTED)")
 	mcpServersUpdateCredentialsCmd.Flags().String("source-app-id", "", "Source app ID (HOSTED)")
-	mcpServersUpdateCredentialsCmd.Flags().StringSlice("config-field", nil, "Extra config field key=value (HOSTED, repeatable)")
+	addRepeatableStringFlag(mcpServersUpdateCredentialsCmd, "config-field", "Extra config field key=value (HOSTED, repeatable)")
 	mcpServersUpdateCredentialsCmd.Flags().String("hosted-config-file", "", "Full hostedConfig JSON (file or \"-\" for stdin)")
 	// EXTERNAL config
 	mcpServersUpdateCredentialsCmd.Flags().String("server-url", "", "External MCP server URL (EXTERNAL)")