From e951857f107a59cfab5c2a06564205b4c07812a3 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Tue, 1 Sep 2026 06:02:49 +0000 Subject: [PATCH 1/7] fix: route every repeatable string flag through one registrar Repeatable string flags were hand-registered as pflag StringSlice in nine files. StringSlice CSV-splits each occurrence, so an empty one is destroyed during parsing: `--user-id "" --user-id REAL` reaches the command as ["REAL"]. On `apps set-owners`, which replaces the full owner list, an unset shell variable silently set one owner and exited 0; the per-value check in that command could only ever catch a lone empty. Add addRepeatableStringFlag (always StringArray) and repeatableStringFlag (rejects an empty or whitespace-only occurrence, including the lone-empty case that reads back as a zero-length slice with Changed set) to cmd/flags.go, and convert all thirteen flags across ten commands to them. The bespoke checks in apps_set_owners.go and tasks_reassign.go are deleted, leaving one implementation and one wording of the rule. cmd/repeatable_flags_test.go mirrors the pagination guards: an AST guard so no file outside flags.go may register such a flag, a tree guard so no live flag is a stringSlice however it was wired up, and a pinned list so a command cannot quietly drop one. BREAKING: `--flag a,b` is now one value, not two. No repeatable flag ever documented comma-splitting; the API now rejects the joined value by name rather than the CLI silently splitting it. Documented in CHANGELOG. --- CHANGELOG.md | 13 ++ cmd/api.go | 14 +- cmd/apps_set_owners.go | 15 +- cmd/flags.go | 51 +++++ cmd/mcp_bindings_by_tools.go | 7 +- cmd/mcp_bindings_create.go | 7 +- cmd/mcp_bindings_delete.go | 7 +- cmd/mcp_servers_config.go | 7 +- cmd/mcp_servers_register.go | 10 +- cmd/mcp_servers_test.go | 16 +- cmd/mcp_servers_update_credentials.go | 2 +- cmd/mcp_tools_search.go | 14 +- cmd/policies_search.go | 14 +- cmd/repeatable_flags_test.go | 310 ++++++++++++++++++++++++++ cmd/tasks_reassign.go | 17 +- cmd/usage_exit_codes_test.go | 33 ++- 16 files changed, 483 insertions(+), 54 deletions(-) create mode 100644 cmd/repeatable_flags_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1df028d..af0dca3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,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 diff --git a/cmd/api.go b/cmd/api.go index 7621e68..c183b73 100644 --- a/cmd/api.go +++ b/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") diff --git a/cmd/apps_set_owners.go b/cmd/apps_set_owners.go index 234083c..01ab867 100644 --- a/cmd/apps_set_owners.go +++ b/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) diff --git a/cmd/flags.go b/cmd/flags.go index 2e79bf6..cd610b5 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -244,3 +244,54 @@ 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 the eight commands using it 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, _ := cmd.Flags().GetStringArray(name) + 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 +} diff --git a/cmd/mcp_bindings_by_tools.go b/cmd/mcp_bindings_by_tools.go index 7bd9288..5530ea1 100644 --- a/cmd/mcp_bindings_by_tools.go +++ b/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) } diff --git a/cmd/mcp_bindings_create.go b/cmd/mcp_bindings_create.go index da20ec6..7357f1b 100644 --- a/cmd/mcp_bindings_create.go +++ b/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) } diff --git a/cmd/mcp_bindings_delete.go b/cmd/mcp_bindings_delete.go index 038b9d3..8fabc4c 100644 --- a/cmd/mcp_bindings_delete.go +++ b/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) } diff --git a/cmd/mcp_servers_config.go b/cmd/mcp_servers_config.go index c1c4517..903f370 100644 --- a/cmd/mcp_servers_config.go +++ b/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 } diff --git a/cmd/mcp_servers_register.go b/cmd/mcp_servers_register.go index 929c922..9327aea 100644 --- a/cmd/mcp_servers_register.go +++ b/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)") diff --git a/cmd/mcp_servers_test.go b/cmd/mcp_servers_test.go index b2d9ef2..2cd417e 100644 --- a/cmd/mcp_servers_test.go +++ b/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", "", "") diff --git a/cmd/mcp_servers_update_credentials.go b/cmd/mcp_servers_update_credentials.go index 865b34d..dc68e0f 100644 --- a/cmd/mcp_servers_update_credentials.go +++ b/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)") diff --git a/cmd/mcp_tools_search.go b/cmd/mcp_tools_search.go index 9a9cfe8..f57eeec 100644 --- a/cmd/mcp_tools_search.go +++ b/cmd/mcp_tools_search.go @@ -29,8 +29,14 @@ var mcpToolsSearchCmd = &cobra.Command{ appID, _ := cmd.Flags().GetString("app-id") connectorID, _ := cmd.Flags().GetString("connector-id") query, _ := cmd.Flags().GetString("query") - states, _ := cmd.Flags().GetStringSlice("state") - classes, _ := cmd.Flags().GetStringSlice("classification") + states, err := repeatableStringFlag(cmd, "state") + if err != nil { + return err + } + classes, err := repeatableStringFlag(cmd, "classification") + if err != nil { + return err + } requestedPageSize := pageSizeFlag(cmd) pageToken, _ := cmd.Flags().GetString("page-token") manualPaging := cmd.Flags().Changed("page-token") @@ -111,8 +117,8 @@ func init() { mcpToolsSearchCmd.Flags().String("app-id", "", "Application ID") mcpToolsSearchCmd.Flags().String("connector-id", "", "Connector ID") mcpToolsSearchCmd.Flags().String("query", "", "Fuzzy search on tool_name or display_name") - mcpToolsSearchCmd.Flags().StringSlice("state", nil, "Filter by state (repeatable): pending, approved, disabled, removed") - mcpToolsSearchCmd.Flags().StringSlice("classification", nil, "Filter by classification (repeatable): read, write, destructive, sensitive, dangerous") + addRepeatableStringFlag(mcpToolsSearchCmd, "state", "Filter by state (repeatable): pending, approved, disabled, removed") + addRepeatableStringFlag(mcpToolsSearchCmd, "classification", "Filter by classification (repeatable): read, write, destructive, sensitive, dangerous") addPaginationFlags(mcpToolsSearchCmd) markRequired(mcpToolsSearchCmd, "app-id", "connector-id") mcpToolsCmd.AddCommand(mcpToolsSearchCmd) diff --git a/cmd/policies_search.go b/cmd/policies_search.go index 423185e..bceee92 100644 --- a/cmd/policies_search.go +++ b/cmd/policies_search.go @@ -43,8 +43,14 @@ it only ignores case, and the names differ by more than that between tenants query, _ := cmd.Flags().GetString("query") displayName, _ := cmd.Flags().GetString("display-name") includeDeleted, _ := cmd.Flags().GetBool("include-deleted") - policyTypes, _ := cmd.Flags().GetStringSlice("policy-type") - excludeIDs, _ := cmd.Flags().GetStringSlice("exclude-policy-id") + policyTypes, err := repeatableStringFlag(cmd, "policy-type") + if err != nil { + return err + } + excludeIDs, err := repeatableStringFlag(cmd, "exclude-policy-id") + if err != nil { + return err + } requestedPageSize := pageSizeFlag(cmd) pageToken, _ := cmd.Flags().GetString("page-token") manualPaging := cmd.Flags().Changed("page-token") @@ -115,9 +121,9 @@ it only ignores case, and the names differ by more than that between tenants func init() { policiesSearchCmd.Flags().String("query", "", "Fuzzy search on display name and description") policiesSearchCmd.Flags().String("display-name", "", "Exact-ish (case-insensitive) display name match") - policiesSearchCmd.Flags().StringSlice("policy-type", nil, "Filter by policy type: grant, revoke, certify, ... (repeatable)") + addRepeatableStringFlag(policiesSearchCmd, "policy-type", "Filter by policy type: grant, revoke, certify, ... (repeatable)") policiesSearchCmd.Flags().Bool("include-deleted", false, "Include soft-deleted policies") - policiesSearchCmd.Flags().StringSlice("exclude-policy-id", nil, "Policy ID to exclude from results (repeatable)") + addRepeatableStringFlag(policiesSearchCmd, "exclude-policy-id", "Policy ID to exclude from results (repeatable)") // The lower default is this endpoint's own; the rest is context the shared // flag wording can't carry without drifting. The floor here is 5, not the // 10 the policy proto's comment claims (9 passes through unclamped) -- and diff --git a/cmd/repeatable_flags_test.go b/cmd/repeatable_flags_test.go new file mode 100644 index 0000000..70229a2 --- /dev/null +++ b/cmd/repeatable_flags_test.go @@ -0,0 +1,310 @@ +package cmd + +import ( + "errors" + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// Repeatable string flags (--user-id, --tool-id, --config-field, …) were +// hand-registered as pflag StringSlice in nine files. StringSlice CSV-splits +// every occurrence, which DESTROYS an empty one during parsing: +// `--user-id "" --user-id REAL` arrives as ["REAL"]. `apps set-owners` +// replaces the full owner list, so an unset shell variable silently dropped an +// intended owner and exited 0; a per-value check in the command could never +// see it. See addRepeatableStringFlag. +// +// The guards below mirror the pagination ones (pagination_flags_test.go): +// +// Guard 1 (source) no file outside flags.go may register a repeatable +// string flag itself — it must call the registrar. +// Guard 2 (real tree) no flag in the live command tree is a stringSlice, +// however it was wired up. +// Guard 3 (real tree) the flags known to be repeatable are still registered +// and still stringArray. +// Guard 4 (registrar) addRepeatableStringFlag itself registers stringArray. +// Guard 5 (behavior) the accessor rejects every empty-occurrence shape +// with exit 2, and preserves a comma verbatim. + +// repeatableFlagMethod matches pflag's repeatable-string registration methods. +// StringArray is the required one; StringSlice is listed so the guard reports +// it rather than ignoring it. +var repeatableFlagMethod = map[string]bool{ + "StringSlice": true, "StringSliceP": true, + "StringSliceVar": true, "StringSliceVarP": true, + "StringArray": true, "StringArrayP": true, + "StringArrayVar": true, "StringArrayVarP": true, +} + +// findRepeatableFlagRegistrations parses every non-test .go file in the cmd +// package and returns each `.Flags().StringSlice|StringArray…(…)` call. +// Parsing the AST rather than grepping means a reformatted or line-wrapped +// registration is still caught. +func findRepeatableFlagRegistrations(t *testing.T) []flagsCallSite { + t.Helper() + + paths, err := filepath.Glob("*.go") + if err != nil { + t.Fatalf("globbing cmd/*.go: %v", err) + } + if len(paths) == 0 { + t.Fatalf("found no .go files in the cmd package directory — has the test's working directory changed?") + } + + var sites []flagsCallSite + for _, path := range paths { + if strings.HasSuffix(path, "_test.go") { + continue + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + t.Fatalf("parsing %s: %v", path, err) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !isFlagsCall(sel.X) || !repeatableFlagMethod[sel.Sel.Name] { + return true + } + name := "?" + if lit, ok := call.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { + if unquoted, err := strconv.Unquote(lit.Value); err == nil { + name = unquoted + } + } + sites = append(sites, flagsCallSite{file: path, line: fset.Position(call.Pos()).Line, flag: sel.Sel.Name + " " + name}) + return true + }) + } + return sites +} + +// TestRepeatableStringFlagsGoThroughSharedRegistrar is Guard 1: the registrar +// is the only place a repeatable string flag may be created, so no command can +// reintroduce StringSlice or skip the empty-value check. +func TestRepeatableStringFlagsGoThroughSharedRegistrar(t *testing.T) { + sites := findRepeatableFlagRegistrations(t) + if len(sites) == 0 { + t.Fatal("found no repeatable string flag registrations at all — this guard is not looking at what it thinks it is") + } + var inRegistrar int + for _, s := range sites { + if s.file == registrarFile { + inRegistrar++ + continue + } + t.Errorf("%s:%d registers %s directly; call addRepeatableStringFlag instead — a hand-registered StringSlice comma-splits, which silently discarded an empty --user-id and set the wrong owner list", s.file, s.line, s.flag) + } + if inRegistrar == 0 { + t.Errorf("no repeatable string flag is registered in %s; the shared registrar has gone missing", registrarFile) + } +} + +// TestNoCommandUsesStringSlice is Guard 2. It inspects the REAL command tree, +// so a flag wired up some way the source guard doesn't recognize (a helper, a +// FlagSet copied from another command) is still caught. +func TestNoCommandUsesStringSlice(t *testing.T) { + var arrays int + walkCommandTree(func(c *cobra.Command) { + c.Flags().VisitAll(func(f *pflag.Flag) { + switch f.Value.Type() { + case "stringSlice": + t.Errorf("%s: --%s is a stringSlice; it comma-splits each occurrence and destroys an empty one before the command can see it — register it with addRepeatableStringFlag", c.CommandPath(), f.Name) + case "stringArray": + arrays++ + } + }) + }) + if arrays == 0 { + t.Fatal("walked the command tree and found no stringArray flag at all — this guard is not looking at what it thinks it is") + } +} + +// repeatableFlagsByCommand pins every repeatable string flag in the tree by +// command path. Guard 2 only proves nothing is a stringSlice, which a command +// that DROPPED its repeatable flag would also satisfy; this notices that. +var repeatableFlagsByCommand = map[string][]string{ + "c1i api": {"query", "header"}, + "c1i apps set-owners": {"user-id"}, + "c1i tasks reassign": {"to-user-id"}, + "c1i mcp bindings create": {"tool-id"}, + "c1i mcp bindings delete": {"tool-id"}, + "c1i mcp bindings by-tools": {"tool-id"}, + "c1i mcp servers register": {"user-id", "config-field"}, + "c1i mcp servers update-credentials": {"config-field"}, + "c1i mcp tools search": {"state", "classification"}, + "c1i policies search": {"policy-type", "exclude-policy-id"}, +} + +func TestPinnedRepeatableFlagsAreStringArrays(t *testing.T) { + found := map[string]bool{} + var checked int + walkCommandTree(func(c *cobra.Command) { + names, ok := repeatableFlagsByCommand[c.CommandPath()] + if !ok { + return + } + found[c.CommandPath()] = true + for _, name := range names { + f := c.Flags().Lookup(name) + if f == nil { + t.Errorf("%s has no --%s flag", c.CommandPath(), name) + continue + } + checked++ + if got := f.Value.Type(); got != "stringArray" { + t.Errorf("%s: --%s is a %s, want stringArray", c.CommandPath(), name, got) + } + } + }) + for path := range repeatableFlagsByCommand { + if !found[path] { + t.Errorf("command %q was not found in the tree; this guard silently covered nothing for it — was it renamed?", path) + } + } + if checked == 0 { + t.Fatal("checked no pinned repeatable flag — this guard is not looking at what it thinks it is") + } +} + +// TestAddRepeatableStringFlagRegistersAStringArray is Guard 4: it pins the +// registrar on a throwaway command, so a regression in it is reported here +// rather than as a confusing failure in every tree-walking guard at once. +func TestAddRepeatableStringFlagRegistersAStringArray(t *testing.T) { + c := &cobra.Command{Use: "throwaway"} + addRepeatableStringFlag(c, "thing-id", "Thing ID (repeatable)") + + f := c.Flags().Lookup("thing-id") + if f == nil { + t.Fatal("addRepeatableStringFlag did not register the flag") + } + if got := f.Value.Type(); got != "stringArray" { + t.Errorf("--thing-id is a %s, want stringArray", got) + } + if f.Usage != "Thing ID (repeatable)" { + t.Errorf("--thing-id usage = %q, want the usage passed in", f.Usage) + } + if f.Changed { + t.Error("--thing-id reports Changed before anything set it") + } +} + +// TestRepeatableStringFlagRejectsEmptyOccurrences is Guard 5, the behavioral +// core. Every "want error" row below was accepted before this change: the CSV +// split either erased the empty occurrence outright or left a blank the +// command shipped to the API. +func TestRepeatableStringFlagRejectsEmptyOccurrences(t *testing.T) { + for _, tc := range []struct { + name string + set []string // values passed as separate occurrences; nil = flag never set + want []string + wantErr bool + }{ + {name: "never set", set: nil, want: nil}, + {name: "one real value", set: []string{"REALID"}, want: []string{"REALID"}}, + {name: "two real values", set: []string{"ID-A", "ID-B"}, want: []string{"ID-A", "ID-B"}}, + // The defect: under StringSlice this arrived as ["REALID"] and the + // command acted on one id while the caller had named two. + {name: "empty before a real value", set: []string{"", "REALID"}, wantErr: true}, + {name: "empty after a real value", set: []string{"REALID", ""}, wantErr: true}, + {name: "empty between real values", set: []string{"ID-A", "", "ID-B"}, wantErr: true}, + // Reads back as a zero-length slice with Changed set, so length alone + // cannot tell it from "never set". + {name: "lone empty", set: []string{""}, wantErr: true}, + {name: "whitespace only", set: []string{" "}, wantErr: true}, + {name: "tab only alongside a real value", set: []string{"\t", "REALID"}, wantErr: true}, + // The documented break: a comma is now part of the value, not a + // separator. One occurrence in, one value out. + {name: "comma is not a separator", set: []string{"a,b"}, want: []string{"a,b"}}, + } { + t.Run(tc.name, func(t *testing.T) { + c := &cobra.Command{Use: "probe"} + addRepeatableStringFlag(c, "thing-id", "") + for _, v := range tc.set { + if err := c.Flags().Set("thing-id", v); err != nil { + t.Fatalf("setting --thing-id %q: %v", v, err) + } + } + + got, err := repeatableStringFlag(c, "thing-id") + if tc.wantErr { + if err == nil { + t.Fatalf("values %q were accepted as %q; an empty occurrence must be a usage error", tc.set, got) + } + var ue *usageError + if !errors.As(err, &ue) { + t.Errorf("returned %T, want *usageError so it exits 2", err) + } + if code := exitCode(err); code != exitUsage { + t.Errorf("exits %d, want %d (exitUsage)", code, exitUsage) + } + if !strings.Contains(err.Error(), "--thing-id") { + t.Errorf("error %q does not name the flag", err.Error()) + } + return + } + if err != nil { + t.Fatalf("values %q rejected: %v", tc.set, err) + } + if len(got) != len(tc.want) { + t.Fatalf("got %q (%d values), want %q (%d)", got, len(got), tc.want, len(tc.want)) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("value %d = %q, want %q", i, got[i], tc.want[i]) + } + } + }) + } +} + +// TestRepeatableStringFlagErrorHasOneWording pins that both rejected shapes +// produce the SAME message. The point of the shared accessor is that the rule +// has exactly one implementation and one wording across all nine commands. +func TestRepeatableStringFlagErrorHasOneWording(t *testing.T) { + msg := func(values ...string) string { + c := &cobra.Command{Use: "probe"} + addRepeatableStringFlag(c, "thing-id", "") + for _, v := range values { + if err := c.Flags().Set("thing-id", v); err != nil { + t.Fatalf("setting --thing-id: %v", err) + } + } + _, err := repeatableStringFlag(c, "thing-id") + if err == nil { + t.Fatalf("values %q were accepted", values) + } + return err.Error() + } + lone, mixed := msg(""), msg("", "REALID") + if lone != mixed { + t.Errorf("the two empty shapes report different messages:\n lone : %q\n mixed: %q", lone, mixed) + } + if want := repeatableStringFlagError("thing-id").Error(); lone != want { + t.Errorf("message = %q, want the shared wording %q", lone, want) + } +} + +// TestRepeatableStringFlagOnAnUnregisteredFlag pins the missing-flag path: +// the accessor must not panic on a name no command registered. +func TestRepeatableStringFlagOnAnUnregisteredFlag(t *testing.T) { + got, err := repeatableStringFlag(&cobra.Command{Use: "bare"}, "nope") + if err != nil { + t.Errorf("unregistered flag returned an error: %v", err) + } + if len(got) != 0 { + t.Errorf("unregistered flag returned %q, want no values", got) + } +} diff --git a/cmd/tasks_reassign.go b/cmd/tasks_reassign.go index 883313a..51ce5b9 100644 --- a/cmd/tasks_reassign.go +++ b/cmd/tasks_reassign.go @@ -23,18 +23,15 @@ The confirmation reports the task id and the policy step acted on, never a state: the action endpoints echo the task's state from before the action.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - toUserIDs, _ := cmd.Flags().GetStringSlice("to-user-id") - // Cobra's required check only proves the flag was set. `--to-user-id ""` - // parses to an empty slice; `a,,b` yields a blank element. Either would - // otherwise post an empty approver id. + // Cobra's required check only proves the flag was set; the accessor is + // what rejects an empty occurrence that would post a blank approver id. + toUserIDs, err := repeatableStringFlag(cmd, "to-user-id") + if err != nil { + return err + } if len(toUserIDs) == 0 { return &usageError{fmt.Errorf("flag --to-user-id requires at least one value")} } - for _, id := range toUserIDs { - if id == "" { - return &usageError{fmt.Errorf("flag --to-user-id requires a non-empty value")} - } - } baseURL, err := GetBaseURL() if err != nil { @@ -86,7 +83,7 @@ state: the action endpoints echo the task's state from before the action.`, } func init() { - tasksReassignCmd.Flags().StringSlice("to-user-id", nil, "User ID to reassign the step to (repeatable)") + addRepeatableStringFlag(tasksReassignCmd, "to-user-id", "User ID to reassign the step to (repeatable)") tasksReassignCmd.Flags().String("policy-step-id", "", "Policy step to reassign (defaults to the task's current step)") tasksReassignCmd.Flags().String("comment", "", "Optional comment") markRequired(tasksReassignCmd, "to-user-id") diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index 79f7613..54d6539 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -142,19 +142,40 @@ func TestValidationGuardsExitUsage(t *testing.T) { }, { // --to-user-id is cobra-required, so omitting it is cobra's job. - // An explicit "" satisfies "required" but comma-splits to an - // empty slice, reaching the len==0 guard. + // A lone "" satisfies "required" but pflag collapses it to an + // empty slice, so only the Changed check sees it. name: "tasks reassign: --to-user-id empty", args: []string{"tasks", "reassign", "task-1", "--to-user-id", ""}, cmds: []*cobra.Command{tasksReassignCmd}, }, { - // A blank element inside a comma-separated value would post an - // empty approver id. - name: "tasks reassign: --to-user-id with a blank element", - args: []string{"tasks", "reassign", "task-1", "--to-user-id", "user-a,,user-b"}, + // The shape that shipped broken: under StringSlice the empty + // occurrence was discarded during parsing and the command posted + // the surviving id as if that were what was asked for. + name: "tasks reassign: --to-user-id empty alongside a real one", + args: []string{"tasks", "reassign", "task-1", "--to-user-id", "", "--to-user-id", "user-b"}, cmds: []*cobra.Command{tasksReassignCmd}, }, + { + name: "apps set-owners: --user-id empty alongside a real one", + args: []string{"apps", "set-owners", "app-1", "--user-id", "", "--user-id", "user-b"}, + cmds: []*cobra.Command{appsSetOwnersCmd}, + }, + { + name: "mcp bindings create: --tool-id empty alongside a real one", + args: []string{"mcp", "bindings", "create", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", "", "--tool-id", "tool-b"}, + cmds: []*cobra.Command{mcpBindingsCreateCmd}, + }, + { + name: "mcp bindings delete: --tool-id empty alongside a real one", + args: []string{"mcp", "bindings", "delete", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", "", "--tool-id", "tool-b"}, + cmds: []*cobra.Command{mcpBindingsDeleteCmd}, + }, + { + name: "mcp bindings by-tools: --tool-id whitespace alongside a real one", + args: []string{"mcp", "bindings", "by-tools", "--app-id", "a", "--connector-id", "c", "--tool-id", " ", "--tool-id", "tool-b"}, + cmds: []*cobra.Command{mcpBindingsByToolsCmd}, + }, { name: "auth login: --client-id without --client-secret", args: []string{"auth", "login", "--client-id", "foo"}, From 01fb3e30f27f3542292a2e134817a3356ee023ba Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Tue, 1 Sep 2026 06:06:05 +0000 Subject: [PATCH 2/7] Drop call-site counts from the repeatable-flag comments Both were already stale (ten commands, fourteen flags), and a count in a comment is the drift this change exists to stop. Co-Authored-By: Claude Opus 5 --- cmd/flags.go | 2 +- cmd/repeatable_flags_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/flags.go b/cmd/flags.go index cd610b5..1505fc9 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -262,7 +262,7 @@ func addRepeatableStringFlag(cmd *cobra.Command, name, usage string) { } // repeatableStringFlagError is the one wording for a repeatable flag given an -// empty value, defined once so the eight commands using it cannot drift apart. +// 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)} } diff --git a/cmd/repeatable_flags_test.go b/cmd/repeatable_flags_test.go index 70229a2..334d766 100644 --- a/cmd/repeatable_flags_test.go +++ b/cmd/repeatable_flags_test.go @@ -272,7 +272,7 @@ func TestRepeatableStringFlagRejectsEmptyOccurrences(t *testing.T) { // TestRepeatableStringFlagErrorHasOneWording pins that both rejected shapes // produce the SAME message. The point of the shared accessor is that the rule -// has exactly one implementation and one wording across all nine commands. +// has exactly one implementation and one wording across every caller. func TestRepeatableStringFlagErrorHasOneWording(t *testing.T) { msg := func(values ...string) string { c := &cobra.Command{Use: "probe"} From 8ff8e1dba5d8ab327f3be068a63450485a63bb34 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:51:53 +0000 Subject: [PATCH 3/7] Validate before building a client, and guard the read as well as the flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two searches read their repeatable flags after constructing the client, so an empty value exited 3 on a missing credential instead of 2 with the reason. Both reads are pure and now happen first, matching every other converted command. Measured with no credentials: exit 2, naming the flag. The registrar guard pinned how these flags are registered but nothing pinned how they are read, so reading one directly reverted the fix silently for six of ten commands. They now have rows in the exit-code test. A wrong flag type no longer reports as an empty value, which sent the reader to fix their command line instead of the code; an unregistered name is an error rather than a silent empty, which is the shape this all exists to stop. The one-value-per-occurrence rule is in the README and the agent doc, not only the changelog — a comma in --config-field is the edge the server may accept. Co-Authored-By: Claude Opus 5 --- README.md | 19 ++++++++++++++++++ cmd/agents.md | 5 +++++ cmd/flags.go | 7 ++++++- cmd/mcp_tools_search.go | 16 +++++++-------- cmd/policies_search.go | 16 +++++++-------- cmd/repeatable_flags_test.go | 15 +++++++++----- cmd/usage_exit_codes_test.go | 38 ++++++++++++++++++++++++++++++++++++ 7 files changed, 94 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index c1ff483..c44a01d 100644 --- a/README.md +++ b/README.md @@ -445,6 +445,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 diff --git a/cmd/agents.md b/cmd/agents.md index 39a2fa3..cd63741 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -1,3 +1,8 @@ +- 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. --- name: c1i description: CLI for the C1 (formerly ConductorOne) identity security platform — manage users, apps, entitlements, tasks, access reviews, and more. diff --git a/cmd/flags.go b/cmd/flags.go index 1505fc9..2c8ee7a 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -280,7 +280,12 @@ func repeatableStringFlagError(name string) error { // 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, _ := cmd.Flags().GetStringArray(name) + 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 diff --git a/cmd/mcp_tools_search.go b/cmd/mcp_tools_search.go index f57eeec..53436a1 100644 --- a/cmd/mcp_tools_search.go +++ b/cmd/mcp_tools_search.go @@ -16,6 +16,14 @@ var mcpToolsSearchCmd = &cobra.Command{ return err } + states, err := repeatableStringFlag(cmd, "state") + if err != nil { + return err + } + classes, err := repeatableStringFlag(cmd, "classification") + if err != nil { + return err + } baseURL, err := GetBaseURL() if err != nil { return err @@ -29,14 +37,6 @@ var mcpToolsSearchCmd = &cobra.Command{ appID, _ := cmd.Flags().GetString("app-id") connectorID, _ := cmd.Flags().GetString("connector-id") query, _ := cmd.Flags().GetString("query") - states, err := repeatableStringFlag(cmd, "state") - if err != nil { - return err - } - classes, err := repeatableStringFlag(cmd, "classification") - if err != nil { - return err - } requestedPageSize := pageSizeFlag(cmd) pageToken, _ := cmd.Flags().GetString("page-token") manualPaging := cmd.Flags().Changed("page-token") diff --git a/cmd/policies_search.go b/cmd/policies_search.go index bceee92..d04771a 100644 --- a/cmd/policies_search.go +++ b/cmd/policies_search.go @@ -30,6 +30,14 @@ tenant's auto-approval policy portably. --display-name is no substitute: it only ignores case, and the names differ by more than that between tenants ("Auto-approval" in one, "Auto approval" in the next).`, RunE: func(cmd *cobra.Command, args []string) error { + policyTypes, err := repeatableStringFlag(cmd, "policy-type") + if err != nil { + return err + } + excludeIDs, err := repeatableStringFlag(cmd, "exclude-policy-id") + if err != nil { + return err + } baseURL, err := GetBaseURL() if err != nil { return err @@ -43,14 +51,6 @@ it only ignores case, and the names differ by more than that between tenants query, _ := cmd.Flags().GetString("query") displayName, _ := cmd.Flags().GetString("display-name") includeDeleted, _ := cmd.Flags().GetBool("include-deleted") - policyTypes, err := repeatableStringFlag(cmd, "policy-type") - if err != nil { - return err - } - excludeIDs, err := repeatableStringFlag(cmd, "exclude-policy-id") - if err != nil { - return err - } requestedPageSize := pageSizeFlag(cmd) pageToken, _ := cmd.Flags().GetString("page-token") manualPaging := cmd.Flags().Changed("page-token") diff --git a/cmd/repeatable_flags_test.go b/cmd/repeatable_flags_test.go index 334d766..2b8a4c5 100644 --- a/cmd/repeatable_flags_test.go +++ b/cmd/repeatable_flags_test.go @@ -15,7 +15,7 @@ import ( ) // Repeatable string flags (--user-id, --tool-id, --config-field, …) were -// hand-registered as pflag StringSlice in nine files. StringSlice CSV-splits +// hand-registered as pflag StringSlice. StringSlice CSV-splits // every occurrence, which DESTROYS an empty one during parsing: // `--user-id "" --user-id REAL` arrives as ["REAL"]. `apps set-owners` // replaces the full owner list, so an unset shell variable silently dropped an @@ -297,14 +297,19 @@ func TestRepeatableStringFlagErrorHasOneWording(t *testing.T) { } } -// TestRepeatableStringFlagOnAnUnregisteredFlag pins the missing-flag path: -// the accessor must not panic on a name no command registered. +// TestRepeatableStringFlagOnAnUnregisteredFlag pins the missing-flag path. A +// name no command registered is a wiring bug, and it used to return no values +// and no error — the silent-empty shape this file exists to eliminate. It must +// surface, and must not be mistaken for the user passing an empty value. func TestRepeatableStringFlagOnAnUnregisteredFlag(t *testing.T) { got, err := repeatableStringFlag(&cobra.Command{Use: "bare"}, "nope") - if err != nil { - t.Errorf("unregistered flag returned an error: %v", err) + if err == nil { + t.Fatal("unregistered flag returned no error; a wiring bug reads as success") } if len(got) != 0 { t.Errorf("unregistered flag returned %q, want no values", got) } + if err.Error() == repeatableStringFlagError("nope").Error() { + t.Error("a wiring bug reports the user's empty-value message, sending the reader to fix their command line") + } } diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index 54d6539..d30616d 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -46,6 +46,44 @@ func TestValidationGuardsExitUsage(t *testing.T) { args []string cmds []*cobra.Command // commands whose flags need resetting between cases }{ + // The registrar guard pins how these flags are REGISTERED; nothing pins + // how they are READ. Reading one with GetStringArray directly reverts the + // fix for that flag silently, and only a row here notices. + { + name: "api: --query empty", + args: []string{"api", "--path", "/x", "--query", ""}, + cmds: []*cobra.Command{apiCmd}, + }, + { + name: "api: --header empty", + args: []string{"api", "--path", "/x", "--header", ""}, + cmds: []*cobra.Command{apiCmd}, + }, + { + name: "policies search: --policy-type empty", + args: []string{"policies", "search", "--policy-type", ""}, + cmds: []*cobra.Command{policiesSearchCmd}, + }, + { + name: "policies search: --exclude-policy-id empty", + args: []string{"policies", "search", "--exclude-policy-id", ""}, + cmds: []*cobra.Command{policiesSearchCmd}, + }, + { + name: "mcp tools search: --state empty", + args: []string{"mcp", "tools", "search", "--state", ""}, + cmds: []*cobra.Command{mcpToolsSearchCmd}, + }, + { + name: "mcp tools search: --classification empty", + args: []string{"mcp", "tools", "search", "--classification", ""}, + cmds: []*cobra.Command{mcpToolsSearchCmd}, + }, + { + name: "mcp servers register: --user-id empty", + args: []string{"mcp", "servers", "register", "--app-id", "a", "--type", "hosted", "--display-name", "d", "--user-id", ""}, + cmds: []*cobra.Command{mcpServersRegisterCmd}, + }, { // --tool-id is a cobra-required flag; omitting it entirely is // intercepted by cobra itself (already exitUsage via From 60b7fe4be41576fc7f64def98e80a537ccacbabb Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:02:26 +0000 Subject: [PATCH 4/7] Restore the agent doc's front matter, and stop rows passing for the wrong reason The repeatable-flags bullet landed above agents.md's YAML block, so the shipped output of docs agents opened with a list item and name/description/ version stopped being front matter at all. A test now pins that the doc opens with the block and carries the four keys the help text promises harnesses parse; the whole suite was green while it was broken. Two of the seven new exit-code rows never reached the accessor: mcp tools search marks --app-id and --connector-id required, and cobra's own error is exit 2 as well, so they passed while proving nothing. The rows now supply those flags, and every repeatable row pins the accessor's own wording, so a row that exits 2 by another path fails. mcp servers register gains --catalog-id for the same reason: it was passing on ordering luck. Co-Authored-By: Claude Opus 5 --- cmd/agents.md | 11 +++---- cmd/docs_agents_test.go | 23 ++++++++++++++ cmd/usage_exit_codes_test.go | 58 +++++++++++++++++++++++------------- 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/cmd/agents.md b/cmd/agents.md index cd63741..824bc8c 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -1,8 +1,3 @@ -- 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. --- name: c1i description: CLI for the C1 (formerly ConductorOne) identity security platform — manage users, apps, entitlements, tasks, access reviews, and more. @@ -248,6 +243,12 @@ 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 diff --git a/cmd/docs_agents_test.go b/cmd/docs_agents_test.go index edcab53..953b40c 100644 --- a/cmd/docs_agents_test.go +++ b/cmd/docs_agents_test.go @@ -175,3 +175,26 @@ 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) { + if !strings.HasPrefix(agentsTemplate, "---\n") { + first, _, _ := strings.Cut(agentsTemplate, "\n") + t.Fatalf("agents.md must open with the YAML front-matter delimiter; it starts with %q", first) + } + rest := strings.TrimPrefix(agentsTemplate, "---\n") + end := strings.Index(rest, "\n---\n") + if end < 0 { + t.Fatal("agents.md opens a front-matter block that is never closed") + } + for _, key := range []string{"name:", "description:", "version:", "required_bins:"} { + if !strings.Contains(rest[:end], key) { + t.Errorf("front matter is missing %q, which docs agents' help says harnesses parse", key) + } + } +} diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index d30616d..d2669ec 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/ConductorOne/c1i/internal/client" @@ -45,44 +46,55 @@ func TestValidationGuardsExitUsage(t *testing.T) { name string args []string cmds []*cobra.Command // commands whose flags need resetting between cases + // wantMsg pins WHICH usage error fired. Cobra's own required-flag error + // is also exit 2, so a row missing a required flag passes while never + // reaching the guard it was written for. + wantMsg string }{ // The registrar guard pins how these flags are REGISTERED; nothing pins // how they are READ. Reading one with GetStringArray directly reverts the // fix for that flag silently, and only a row here notices. { - name: "api: --query empty", - args: []string{"api", "--path", "/x", "--query", ""}, - cmds: []*cobra.Command{apiCmd}, + name: "api: --query empty", + args: []string{"api", "--path", "/x", "--query", ""}, + wantMsg: "--query requires a non-empty value", + cmds: []*cobra.Command{apiCmd}, }, { - name: "api: --header empty", - args: []string{"api", "--path", "/x", "--header", ""}, - cmds: []*cobra.Command{apiCmd}, + name: "api: --header empty", + args: []string{"api", "--path", "/x", "--header", ""}, + wantMsg: "--header requires a non-empty value", + cmds: []*cobra.Command{apiCmd}, }, { - name: "policies search: --policy-type empty", - args: []string{"policies", "search", "--policy-type", ""}, - cmds: []*cobra.Command{policiesSearchCmd}, + name: "policies search: --policy-type empty", + args: []string{"policies", "search", "--policy-type", ""}, + wantMsg: "--policy-type requires a non-empty value", + cmds: []*cobra.Command{policiesSearchCmd}, }, { - name: "policies search: --exclude-policy-id empty", - args: []string{"policies", "search", "--exclude-policy-id", ""}, - cmds: []*cobra.Command{policiesSearchCmd}, + name: "policies search: --exclude-policy-id empty", + args: []string{"policies", "search", "--exclude-policy-id", ""}, + wantMsg: "--exclude-policy-id requires a non-empty value", + cmds: []*cobra.Command{policiesSearchCmd}, }, { - name: "mcp tools search: --state empty", - args: []string{"mcp", "tools", "search", "--state", ""}, - cmds: []*cobra.Command{mcpToolsSearchCmd}, + name: "mcp tools search: --state empty", + args: []string{"mcp", "tools", "search", "--app-id", "a", "--connector-id", "c", "--state", ""}, + wantMsg: "--state requires a non-empty value", + cmds: []*cobra.Command{mcpToolsSearchCmd}, }, { - name: "mcp tools search: --classification empty", - args: []string{"mcp", "tools", "search", "--classification", ""}, - cmds: []*cobra.Command{mcpToolsSearchCmd}, + name: "mcp tools search: --classification empty", + args: []string{"mcp", "tools", "search", "--app-id", "a", "--connector-id", "c", "--classification", ""}, + wantMsg: "--classification requires a non-empty value", + cmds: []*cobra.Command{mcpToolsSearchCmd}, }, { - name: "mcp servers register: --user-id empty", - args: []string{"mcp", "servers", "register", "--app-id", "a", "--type", "hosted", "--display-name", "d", "--user-id", ""}, - cmds: []*cobra.Command{mcpServersRegisterCmd}, + name: "mcp servers register: --user-id empty", + args: []string{"mcp", "servers", "register", "--app-id", "a", "--type", "hosted", "--display-name", "d", "--catalog-id", "cat1", "--user-id", ""}, + wantMsg: "--user-id requires a non-empty value", + cmds: []*cobra.Command{mcpServersRegisterCmd}, }, { // --tool-id is a cobra-required flag; omitting it entirely is @@ -234,6 +246,10 @@ func TestValidationGuardsExitUsage(t *testing.T) { if got, want := exitCode(err), exitUsage; got != want { t.Errorf("exitCode(%v) = %d, want %d (exitUsage); err type %T", err, got, want, err) } + if tc.wantMsg != "" && !strings.Contains(err.Error(), tc.wantMsg) { + t.Errorf("error was %q, want it to contain %q — this row is exiting 2 "+ + "for a different reason than the guard it targets", err, tc.wantMsg) + } }) } } From eade6c613caeaf274dd074bc61f55d73ec825740 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:16:30 +0000 Subject: [PATCH 5/7] Pin the remaining repeatable rows, and close the front-matter scan Correcting the previous commit message: it said every repeatable row pins the accessor's wording. Seven did; nine did not. The four lone-empty rows survive deleting the registrar's Changed branch because each command keeps its own length fallback, so they passed while proving nothing about the shared accessor. They are pinned now, as is --config-field, whose read was the one entry in the registrar map with no end-to-end coverage. The front-matter check searched for the next --- anywhere, so a dropped delimiter would scan into the body and report a body-sized block as valid. It now requires the block to close before the first blank line. Also drops a blank line that made the whole gotchas list loose in CommonMark. Co-Authored-By: Claude Opus 5 --- cmd/agents.md | 1 - cmd/docs_agents_test.go | 6 ++++++ cmd/usage_exit_codes_test.go | 37 ++++++++++++++++++++++++------------ 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/cmd/agents.md b/cmd/agents.md index 824bc8c..f65e887 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -248,7 +248,6 @@ Two things are irreversible in ways their `--help` doesn't make obvious: "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 diff --git a/cmd/docs_agents_test.go b/cmd/docs_agents_test.go index 953b40c..a8da1ae 100644 --- a/cmd/docs_agents_test.go +++ b/cmd/docs_agents_test.go @@ -187,11 +187,17 @@ func TestAgentsDocOpensWithFrontMatter(t *testing.T) { first, _, _ := strings.Cut(agentsTemplate, "\n") t.Fatalf("agents.md must open with the YAML front-matter delimiter; it starts with %q", first) } + // The block must close before the body starts. Scanning for the next "---" + // anywhere would latch onto a thematic break if the real delimiter were + // dropped, and report a body-sized block as valid front matter. rest := strings.TrimPrefix(agentsTemplate, "---\n") end := strings.Index(rest, "\n---\n") if end < 0 { t.Fatal("agents.md opens a front-matter block that is never closed") } + if strings.Contains(rest[:end], "\n\n") { + t.Fatal("agents.md's front-matter block is not closed before the body begins") + } for _, key := range []string{"name:", "description:", "version:", "required_bins:"} { if !strings.Contains(rest[:end], key) { t.Errorf("front matter is missing %q, which docs agents' help says harnesses parse", key) diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index d2669ec..dd92c7d 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -90,6 +90,15 @@ func TestValidationGuardsExitUsage(t *testing.T) { wantMsg: "--classification requires a non-empty value", cmds: []*cobra.Command{mcpToolsSearchCmd}, }, + { + // The only repeatable flag whose READ was unpinned end to end: the + // existing --config-field row passes a non-empty bad pair, which + // parseKeyValues rejects on its own. + name: "mcp servers register: --config-field empty", + args: []string{"mcp", "servers", "register", "--app-id", "a", "--type", "hosted", "--display-name", "d", "--catalog-id", "cat1", "--config-field", ""}, + wantMsg: "--config-field requires a non-empty value", + cmds: []*cobra.Command{mcpServersRegisterCmd}, + }, { name: "mcp servers register: --user-id empty", args: []string{"mcp", "servers", "register", "--app-id", "a", "--type", "hosted", "--display-name", "d", "--catalog-id", "cat1", "--user-id", ""}, @@ -102,23 +111,26 @@ func TestValidationGuardsExitUsage(t *testing.T) { // isCobraUsageError) before RunE ever runs. Passing it as an // explicit empty string satisfies "required" (Changed=true) and // actually reaches the len(toolIDs)==0 guard this test targets. - name: "mcp bindings create: --tool-id empty", - args: []string{"mcp", "bindings", "create", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", ""}, - cmds: []*cobra.Command{mcpBindingsCreateCmd}, + name: "mcp bindings create: --tool-id empty", + args: []string{"mcp", "bindings", "create", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", ""}, + wantMsg: "--tool-id requires a non-empty value", + cmds: []*cobra.Command{mcpBindingsCreateCmd}, }, { - name: "mcp bindings delete: --tool-id empty", - args: []string{"mcp", "bindings", "delete", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", ""}, - cmds: []*cobra.Command{mcpBindingsDeleteCmd}, + name: "mcp bindings delete: --tool-id empty", + args: []string{"mcp", "bindings", "delete", "--app-id", "a", "--connector-id", "c", "--toolset-id", "t", "--tool-id", ""}, + wantMsg: "--tool-id requires a non-empty value", + cmds: []*cobra.Command{mcpBindingsDeleteCmd}, }, { // Also pins the ordering fix: mcp_bindings_by_tools.go used to // construct its client before this check, unlike create/delete // above, so this case would previously have needed real // credentials to reach the guard at all. - name: "mcp bindings by-tools: --tool-id empty", - args: []string{"mcp", "bindings", "by-tools", "--app-id", "a", "--connector-id", "c", "--tool-id", ""}, - cmds: []*cobra.Command{mcpBindingsByToolsCmd}, + name: "mcp bindings by-tools: --tool-id empty", + args: []string{"mcp", "bindings", "by-tools", "--app-id", "a", "--connector-id", "c", "--tool-id", ""}, + wantMsg: "--tool-id requires a non-empty value", + cmds: []*cobra.Command{mcpBindingsByToolsCmd}, }, { name: "mcp bindings history: neither --toolset-id nor --tool-id", @@ -194,9 +206,10 @@ func TestValidationGuardsExitUsage(t *testing.T) { // --to-user-id is cobra-required, so omitting it is cobra's job. // A lone "" satisfies "required" but pflag collapses it to an // empty slice, so only the Changed check sees it. - name: "tasks reassign: --to-user-id empty", - args: []string{"tasks", "reassign", "task-1", "--to-user-id", ""}, - cmds: []*cobra.Command{tasksReassignCmd}, + name: "tasks reassign: --to-user-id empty", + args: []string{"tasks", "reassign", "task-1", "--to-user-id", ""}, + wantMsg: "--to-user-id requires a non-empty value", + cmds: []*cobra.Command{tasksReassignCmd}, }, { // The shape that shipped broken: under StringSlice the empty From 2a7011719700a3a1a026e6588340bec2ce0b5705 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:31:24 +0000 Subject: [PATCH 6/7] Revert a front-matter check that rejected valid YAML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correcting the previous commit message: it justified the check by saying a dropped delimiter would scan into the body and accept a body-sized block. That needs a second "---" line in the body, and this file has none — the pre-existing "never closed" check already catches the dropped delimiter. Meanwhile the new check failed legitimate front matter: a blank line between keys, or a block scalar containing one, both reported "not closed before the body begins". A guard that rejects correct input gets deleted, so it is gone. Also pins the invalid --type row, which passed with its guard deleted: the value falls through the switch and trips a later check naming a flag the caller never passed. Co-Authored-By: Claude Opus 5 --- cmd/docs_agents_test.go | 6 ------ cmd/usage_exit_codes_test.go | 10 +++++++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cmd/docs_agents_test.go b/cmd/docs_agents_test.go index a8da1ae..953b40c 100644 --- a/cmd/docs_agents_test.go +++ b/cmd/docs_agents_test.go @@ -187,17 +187,11 @@ func TestAgentsDocOpensWithFrontMatter(t *testing.T) { first, _, _ := strings.Cut(agentsTemplate, "\n") t.Fatalf("agents.md must open with the YAML front-matter delimiter; it starts with %q", first) } - // The block must close before the body starts. Scanning for the next "---" - // anywhere would latch onto a thematic break if the real delimiter were - // dropped, and report a body-sized block as valid front matter. rest := strings.TrimPrefix(agentsTemplate, "---\n") end := strings.Index(rest, "\n---\n") if end < 0 { t.Fatal("agents.md opens a front-matter block that is never closed") } - if strings.Contains(rest[:end], "\n\n") { - t.Fatal("agents.md's front-matter block is not closed before the body begins") - } for _, key := range []string{"name:", "description:", "version:", "required_bins:"} { if !strings.Contains(rest[:end], key) { t.Errorf("front matter is missing %q, which docs agents' help says harnesses parse", key) diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index dd92c7d..4ab92b4 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -183,9 +183,13 @@ func TestValidationGuardsExitUsage(t *testing.T) { cmds: []*cobra.Command{mcpServersTestConnectionCmd}, }, { - name: "mcp servers update-credentials: invalid --type", - args: []string{"mcp", "servers", "update-credentials", "conn-1", "--app-id", "a", "--type", "bogus"}, - cmds: []*cobra.Command{mcpServersUpdateCredentialsCmd}, + // Without wantMsg this passed even with the invalid-type guard + // deleted: --type bogus falls through the switch and trips a later + // check that names a flag the user never passed. + name: "mcp servers update-credentials: invalid --type", + args: []string{"mcp", "servers", "update-credentials", "conn-1", "--app-id", "a", "--type", "bogus"}, + wantMsg: "invalid --type", + cmds: []*cobra.Command{mcpServersUpdateCredentialsCmd}, }, { name: "mcp servers update-credentials: nothing to update", From 9af87c5485150a371eb234eb6f3478998b24e591 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:54:32 +0000 Subject: [PATCH 7/7] Parse the front matter, and pin three rows that were decorative Reverting the blank-line check removed a false positive and left a false negative: drop the closing --- and the delimiter search runs on to any later --- in the body, reporting twenty lines of prose as valid front matter with the whole suite green. The block is parsed instead, using the yaml package this repo already depends on, against the rendered doc a harness consumes. A blank line between keys now passes, which is what the old check got wrong. Three more rows exited 2 by another path when their own guard was removed: bindings history fell through to the empty-path-segment error, invalid --auth to a different row's guard, and a malformed --config-field pair to "nothing to update". Each pins its own message now, proven by deleting the guard it targets. Co-Authored-By: Claude Opus 5 --- cmd/docs_agents_test.go | 20 +++++++++++++++----- cmd/usage_exit_codes_test.go | 21 ++++++++++++--------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cmd/docs_agents_test.go b/cmd/docs_agents_test.go index 953b40c..f06c823 100644 --- a/cmd/docs_agents_test.go +++ b/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) @@ -183,17 +185,25 @@ func runThroughRoot(t *testing.T, args ...string) string { // Nothing else in the tree checks this, and the whole suite stayed green when // it happened. func TestAgentsDocOpensWithFrontMatter(t *testing.T) { - if !strings.HasPrefix(agentsTemplate, "---\n") { - first, _, _ := strings.Cut(agentsTemplate, "\n") + 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(agentsTemplate, "---\n") + 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") } - for _, key := range []string{"name:", "description:", "version:", "required_bins:"} { - if !strings.Contains(rest[:end], key) { + // 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) } } diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index 4ab92b4..f852b0b 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -133,9 +133,10 @@ func TestValidationGuardsExitUsage(t *testing.T) { cmds: []*cobra.Command{mcpBindingsByToolsCmd}, }, { - name: "mcp bindings history: neither --toolset-id nor --tool-id", - args: []string{"mcp", "bindings", "history", "--app-id", "a", "--connector-id", "c"}, - cmds: []*cobra.Command{mcpBindingsHistoryCmd}, + name: "mcp bindings history: neither --toolset-id nor --tool-id", + args: []string{"mcp", "bindings", "history", "--app-id", "a", "--connector-id", "c"}, + wantMsg: "exactly one of --toolset-id or --tool-id is required", + cmds: []*cobra.Command{mcpBindingsHistoryCmd}, }, { name: "mcp bindings history: --toolset-id and --tool-id both set", @@ -173,9 +174,10 @@ func TestValidationGuardsExitUsage(t *testing.T) { cmds: []*cobra.Command{mcpServersTestConnectionCmd}, }, { - name: "mcp servers test-connection: invalid --auth", - args: []string{"mcp", "servers", "test-connection", "--auth", "bogus"}, - cmds: []*cobra.Command{mcpServersTestConnectionCmd}, + name: "mcp servers test-connection: invalid --auth", + args: []string{"mcp", "servers", "test-connection", "--auth", "bogus"}, + wantMsg: "unsupported --auth", + cmds: []*cobra.Command{mcpServersTestConnectionCmd}, }, { name: "mcp servers test-connection: --server-url and --external-config-file mutually exclusive", @@ -197,9 +199,10 @@ func TestValidationGuardsExitUsage(t *testing.T) { cmds: []*cobra.Command{mcpServersUpdateCredentialsCmd}, }, { - name: "mcp servers update-credentials: invalid --config-field pair", - args: []string{"mcp", "servers", "update-credentials", "conn-1", "--app-id", "a", "--type", "hosted", "--config-field", "badpair"}, - cmds: []*cobra.Command{mcpServersUpdateCredentialsCmd}, + name: "mcp servers update-credentials: invalid --config-field pair", + args: []string{"mcp", "servers", "update-credentials", "conn-1", "--app-id", "a", "--type", "hosted", "--config-field", "badpair"}, + wantMsg: "expected key=value", + cmds: []*cobra.Command{mcpServersUpdateCredentialsCmd}, }, { name: "mcp servers update-credentials: --hosted-config-file mutually exclusive with --catalog-id",