test: guard input shapes by class, and fix --delivery-group the guard found

pkg/cmd/comma_separated_flags_test.go

@@ -0,0 +1,162 @@
+package cmd
+
+import (
+	"regexp"
+	"testing"
+
+	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+)
+
+// A flag whose help says "comma-separated" is a promise that a list reaches the
+// API. Twice that promise was broken the same way: the value went out as one
+// query parameter, which the API matched against nothing, so the command
+// returned zero rows with exit 0. --id did it (#411), and --delivery-group did
+// it again after that fix because nothing tied the promise to the encoding.
+//
+// So every such flag must appear below, with how its value is actually sent,
+// and a query filter must be one the client expands into a list. A new
+// comma-separated flag fails here until someone checks it and says how.
+
+type commaFlagHandling int
+
+const (
+	// A filter on a list endpoint, sent through hookdeck.listQuery. The param
+	// must be list-valued there, or a comma-joined value goes out as one.
+	sentAsQueryList commaFlagHandling = iota
+	// Split into an array in the JSON request body before sending.
+	splitIntoBodyArray
+	// Split into a slice the client sends as a list (metrics measures and
+	// dimensions; every Outpost filter, via splitCommaList).
+	splitIntoList
+	// Sent as one string on purpose: the API turns the commas into spaces
+	// (OAuth2 scopes, which the token endpoint expects space-separated).
+	sentAsStringAPIConvertsCommas
+)
+
+type commaFlag struct {
+	kind  commaFlagHandling
+	param string // for sentAsQueryList: the query parameter the flag sets
+}
+
+var commaSeparatedFlags = map[string]commaFlag{
+	"hookdeck gateway event list --delivery-group":                          {kind: sentAsQueryList, param: "delivery_group"},
+	"hookdeck gateway event list --id":                                      {kind: sentAsQueryList, param: "id"},
+	"hookdeck gateway request events --delivery-group":                      {kind: sentAsQueryList, param: "delivery_group"},
+	"hookdeck gateway request list --id":                                    {kind: sentAsQueryList, param: "id"},
+	"hookdeck connection create --rule-deduplicate-exclude-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection create --rule-deduplicate-include-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection create --rule-retry-response-status-codes":         {kind: splitIntoBodyArray},
+	"hookdeck connection create --source-allowed-http-methods":              {kind: splitIntoBodyArray},
+	"hookdeck connection update --rule-deduplicate-exclude-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection update --rule-deduplicate-include-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection update --rule-retry-response-status-codes":         {kind: splitIntoBodyArray},
+	"hookdeck connection upsert --rule-deduplicate-exclude-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection upsert --rule-deduplicate-include-fields":          {kind: splitIntoBodyArray},
+	"hookdeck connection upsert --rule-retry-response-status-codes":         {kind: splitIntoBodyArray},
+	"hookdeck connection upsert --source-allowed-http-methods":              {kind: splitIntoBodyArray},
+	"hookdeck gateway connection create --rule-deduplicate-exclude-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection create --rule-deduplicate-include-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection create --rule-retry-response-status-codes": {kind: splitIntoBodyArray},
+	"hookdeck gateway connection create --source-allowed-http-methods":      {kind: splitIntoBodyArray},
+	"hookdeck gateway connection update --rule-deduplicate-exclude-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection update --rule-deduplicate-include-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection update --rule-retry-response-status-codes": {kind: splitIntoBodyArray},
+	"hookdeck gateway connection upsert --rule-deduplicate-exclude-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection upsert --rule-deduplicate-include-fields":  {kind: splitIntoBodyArray},
+	"hookdeck gateway connection upsert --rule-retry-response-status-codes": {kind: splitIntoBodyArray},
+	"hookdeck gateway connection upsert --source-allowed-http-methods":      {kind: splitIntoBodyArray},
+	"hookdeck gateway request retry --connection-ids":                       {kind: splitIntoBodyArray},
+	"hookdeck gateway source create --allowed-http-methods":                 {kind: splitIntoBodyArray},
+	"hookdeck gateway source update --allowed-http-methods":                 {kind: splitIntoBodyArray},
+	"hookdeck gateway source upsert --allowed-http-methods":                 {kind: splitIntoBodyArray},
+	"hookdeck connection create --destination-oauth2-scopes":                {kind: sentAsStringAPIConvertsCommas},
+	"hookdeck connection upsert --destination-oauth2-scopes":                {kind: sentAsStringAPIConvertsCommas},
+	"hookdeck gateway connection create --destination-oauth2-scopes":        {kind: sentAsStringAPIConvertsCommas},
+	"hookdeck gateway connection upsert --destination-oauth2-scopes":        {kind: sentAsStringAPIConvertsCommas},
+	"hookdeck gateway metrics attempts --dimensions":                        {kind: splitIntoList},
+	"hookdeck gateway metrics attempts --measures":                          {kind: splitIntoList},
+	"hookdeck gateway metrics events --dimensions":                          {kind: splitIntoList},
+	"hookdeck gateway metrics events --measures":                            {kind: splitIntoList},
+	"hookdeck gateway metrics requests --dimensions":                        {kind: splitIntoList},
+	"hookdeck gateway metrics requests --measures":                          {kind: splitIntoList},
+	"hookdeck gateway metrics transformations --dimensions":                 {kind: splitIntoList},
+	"hookdeck gateway metrics transformations --measures":                   {kind: splitIntoList},
+	"hookdeck outpost attempt get --include":                                {kind: splitIntoList},
+	"hookdeck outpost attempt list --destination-id":                        {kind: splitIntoList},
+	"hookdeck outpost attempt list --destination-type":                      {kind: splitIntoList},
+	"hookdeck outpost attempt list --event-id":                              {kind: splitIntoList},
+	"hookdeck outpost attempt list --include":                               {kind: splitIntoList},
+	"hookdeck outpost attempt list --tenant-id":                             {kind: splitIntoList},
+	"hookdeck outpost attempt list --topic":                                 {kind: splitIntoList},
+	"hookdeck outpost destination create --topics":                          {kind: splitIntoList},
+	"hookdeck outpost destination list --topics":                            {kind: splitIntoList},
+	"hookdeck outpost destination list --type":                              {kind: splitIntoList},
+	"hookdeck outpost destination update --topics":                          {kind: splitIntoList},
+	"hookdeck outpost event list --destination-id":                          {kind: splitIntoList},
+	"hookdeck outpost event list --id":                                      {kind: splitIntoList},
+	"hookdeck outpost event list --tenant-id":                               {kind: splitIntoList},
+	"hookdeck outpost event list --topic":                                   {kind: splitIntoList},
+	"hookdeck outpost metrics attempts --dimensions":                        {kind: splitIntoList},
+	"hookdeck outpost metrics attempts --measures":                          {kind: splitIntoList},
+	"hookdeck outpost metrics events --dimensions":                          {kind: splitIntoList},
+	"hookdeck outpost metrics events --measures":                            {kind: splitIntoList},
+	"hookdeck outpost tenant list --id":                                     {kind: splitIntoList},
+}
+
+var commaSeparated = regexp.MustCompile(`(?i)comma[- ]separated`)
+
+func discoverCommaSeparatedFlags() map[string]bool {
+	found := map[string]bool{}
+	var walk func(c *cobra.Command)
+	walk = func(c *cobra.Command) {
+		// Flags() and PersistentFlags(), not LocalFlags(): LocalFlags merges
+		// every parent's persistent flags into the command as a side effect,
+		// which mutates the shared rootCmd for every later test in the package.
+		// It put root's hidden --api-key onto outpost mcp and failed
+		// TestOutpostMCPCommandIsRegistered.
+		visit := func(f *pflag.Flag) {
+			if commaSeparated.MatchString(f.Usage) {
+				found[c.CommandPath()+" --"+f.Name] = true
+			}
+		}
+		c.Flags().VisitAll(visit)
+		c.PersistentFlags().VisitAll(visit)
+		for _, sub := range c.Commands() {
+			walk(sub)
+		}
+	}
+	walk(rootCmd)
+	return found
+}
+
+func TestEveryCommaSeparatedFlagSendsAList(t *testing.T) {
+	found := discoverCommaSeparatedFlags()
+	if len(found) < 40 {
+		t.Fatalf("found only %d comma-separated flags; the walk is probably broken and the guard would pass vacuously", len(found))
+	}
+
+	for flag := range found {
+		handling, ok := commaSeparatedFlags[flag]
+		if !assert.True(t, ok,
+			"%s says comma-separated but is not in commaSeparatedFlags. Check how its value "+
+				"reaches the API -- a query filter sent as one value returns nothing (#411) -- "+
+				"and add it with its handling.", flag) {
+			continue
+		}
+		if handling.kind == sentAsQueryList {
+			assert.True(t, hookdeck.IsListValuedParam(handling.param),
+				"%s is a comma-separated query filter, but %q is not list-valued in hookdeck.listQuery, "+
+					"so \"a,b\" is sent as one value and matches nothing", flag, handling.param)
+		}
+	}
+
+	// A stale entry would quietly cover a different flag added under the same
+	// name later, so every entry must still exist.
+	for flag := range commaSeparatedFlags {
+		assert.True(t, found[flag], "%s is in commaSeparatedFlags but no longer exists or no longer says comma-separated", flag)
+	}
+}

pkg/hookdeck/list_query.go

@@ -11,10 +11,21 @@ import (
 // The comma-joined string used to be sent as one scalar value, which the API
 // matched against nothing: `--id evt_A,evt_B` returned zero rows with exit 0,
 // while each id on its own returned its row. See #411.
+//
+// delivery_group is declared the same way as id in the API -- a single value or
+// an array, with no comma splitting -- and was sent as one value in the same
+// way, so --delivery-group a,b returned nothing too.
 var listValuedParams = map[string]bool{
-	"id": true,
+	"id":             true,
+	"delivery_group": true,
 }
 
+// IsListValuedParam reports whether a list-endpoint filter is sent as a list
+// when given a comma-separated value. The CLI's guard over flags documented as
+// comma-separated uses it, so a flag cannot promise a list the query does not
+// send.
+func IsListValuedParam(name string) bool { return listValuedParams[name] }
+
 // listParamKey is how a repeated value is spelled on the wire.
 var listParamKey = func(key string) string { return key + "[]" }
 

pkg/hookdeck/list_query_test.go

@@ -68,3 +68,38 @@ func TestListEventsSendsIDsAsAList(t *testing.T) {
 	assert.Equal(t, []string{"evt_A", "evt_B"}, got["id[]"])
 	assert.Empty(t, got["id"])
 }
+
+// delivery_group is declared exactly like id in the API -- a single value or an
+// array, no comma splitting -- so "a,b" as one value matched nothing and
+// --delivery-group a,b returned zero rows with exit 0, the #411 shape. Found by
+// the guard over flags documented as comma-separated.
+func TestListQueryExpandsCommaSeparatedDeliveryGroups(t *testing.T) {
+	q := listQuery(map[string]string{"delivery_group": "cus_1,cus_2"})
+	assert.Equal(t, []string{"cus_1", "cus_2"}, q["delivery_group[]"])
+	assert.Empty(t, q["delivery_group"])
+}
+
+// GetRequestEvents built its own query and so was missed by the #411 fix. This
+// covers it using the shared builder, for both list-valued filters.
+func TestGetRequestEventsSendsListValuedFiltersAsLists(t *testing.T) {
+	var got url.Values
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		got = r.URL.Query()
+		_ = json.NewEncoder(w).Encode(map[string]any{"models": []any{}, "pagination": map[string]any{}})
+	}))
+	defer srv.Close()
+
+	base, err := url.Parse(srv.URL)
+	require.NoError(t, err)
+	c := &Client{BaseURL: base}
+
+	_, err = c.GetRequestEvents(context.Background(), "req_1", map[string]string{
+		"id":             "evt_A,evt_B",
+		"delivery_group": "cus_1,cus_2",
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, []string{"evt_A", "evt_B"}, got["id[]"])
+	assert.Equal(t, []string{"cus_1", "cus_2"}, got["delivery_group[]"])
+	assert.Empty(t, got["delivery_group"])
+}

pkg/hookdeck/requests.go

@@ -133,13 +133,12 @@ func (c *Client) GetRequestEvents(ctx context.Context, requestID string, params
 	if err != nil {
 		return nil, err
 	}
+	// Through listQuery like the other event lists, so a comma-separated
+	// list-valued filter is sent as a list. This built its own query and was
+	// missed by #411, so --delivery-group a,b still went out as one value.
 	queryStr := ""
 	if len(params) > 0 {
-		q := url.Values{}
-		for k, v := range params {
-			q.Add(k, v)
-		}
-		queryStr = q.Encode()
+		queryStr = listQuery(params).Encode()
 	}
 	resp, err := c.Get(ctx, path, queryStr, nil)
 	if err != nil {

pkg/mcpcore/input.go

@@ -163,8 +163,17 @@ func (in Input) BoolOrStringE(key string) (*bool, error) {
 	return nil, fmt.Errorf("%s must be true or false, got %v", key, v)
 }
 
-// StringSlice returns the string slice for a key, or nil if missing.
-func (in Input) StringSlice(key string) []string {
+// stringSlice reads a value only when it is a JSON array, and returns nil for
+// anything else -- including the bare or comma-separated string a model sends
+// where an array is declared.
+//
+// It is unexported on purpose. As a public method, tool handlers read
+// caller-supplied lists with it, and a string was dropped without an error:
+// gateway_metrics_read told callers "measures is required" for a measures they
+// had just passed (#440), and a merge later reverted the fix unnoticed. Tool
+// code must use StringList, which accepts both forms; with this unexported, a
+// handler that reaches for the array-only read no longer compiles.
+func (in Input) stringSlice(key string) []string {
 	v, ok := in[key]
 	if !ok {
 		return nil
@@ -240,7 +249,7 @@ func SetPayloadSearchFilters(params map[string]string, in Input) error {
 // StringList reads a value that may be given either as an array of strings or,
 // mirroring the CLI's comma-separated flags, as a single string.
 func StringList(in Input, key string) []string {
-	if values := in.StringSlice(key); len(values) > 0 {
+	if values := in.stringSlice(key); len(values) > 0 {
 		return values
 	}
 	raw := in.String(key)

pkg/mcpcore/input_shape_guard_test.go

@@ -0,0 +1,73 @@
+package mcpcore
+
+import (
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+// Models routinely send a bare or comma-separated string where a tool declares
+// an array. mcpcore.StringList accepts both; reading a caller-supplied list any
+// other way drops the string without an error. That is how gateway_metrics_read
+// came to tell callers "measures is required" for a measures they had passed
+// (#440) -- and how a merge later put the bug back unnoticed.
+//
+// stringSlice is unexported, so tool code cannot call the array-only reader.
+// This covers the remaining way round it: asserting .([]interface{}) directly
+// on a tool argument. Each existing use is listed with the reason it is right;
+// a new one fails here until someone decides it belongs on that list.
+var arrayAssertionAllowed = map[string]string{
+	// rules is an array of rule OBJECTS, not strings, so StringList does not
+	// apply -- and ruleList errors on a non-array rather than dropping it.
+	"gateway/mcp/tool_connections.go": "rules: array of objects, and a non-array is an error",
+}
+
+var arrayAssertion = regexp.MustCompile(`\.\(\s*\[\](interface\{\}|any)\s*\)`)
+
+func TestToolArgumentsAreNotReadAsArraysDirectly(t *testing.T) {
+	for _, dir := range []string{"../gateway/mcp", "../outpost/mcp"} {
+		files, err := filepath.Glob(filepath.Join(dir, "*.go"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(files) == 0 {
+			t.Fatalf("no Go files found in %s; the guard would pass vacuously", dir)
+		}
+		for _, f := range files {
+			if strings.HasSuffix(f, "_test.go") {
+				continue
+			}
+			src, err := os.ReadFile(f)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !arrayAssertion.Match(src) {
+				continue
+			}
+			key := strings.TrimPrefix(filepath.ToSlash(f), "../")
+			if _, ok := arrayAssertionAllowed[key]; ok {
+				continue
+			}
+			t.Errorf("%s asserts .([]interface{}) on tool input. Read a list argument with "+
+				"mcpcore.StringList, which also accepts the string forms models send; "+
+				"if this really is not a list of strings, add the file to arrayAssertionAllowed with the reason.", key)
+		}
+	}
+}
+
+// An allow-list entry for a file that no longer has the assertion is stale, and
+// a stale entry would silently cover the next use added to that file.
+func TestArrayAssertionAllowListIsCurrent(t *testing.T) {
+	for key := range arrayAssertionAllowed {
+		src, err := os.ReadFile(filepath.Join("..", key))
+		if err != nil {
+			t.Errorf("allow-listed file %s: %v", key, err)
+			continue
+		}
+		if !arrayAssertion.Match(src) {
+			t.Errorf("%s is allow-listed but no longer asserts .([]interface{}); remove the entry", key)
+		}
+	}
+}

pkg/mcpcore/input_test.go

@@ -69,8 +69,8 @@ func TestInput_Accessors(t *testing.T) {
 	assert.Equal(t, 99, in.Int("nonexistent", 99))
 	assert.Equal(t, true, in.Bool("active"))
 	assert.Equal(t, false, in.Bool("nonexistent"))
-	assert.Equal(t, []string{"a", "b"}, in.StringSlice("tags"))
-	assert.Nil(t, in.StringSlice("nonexistent"))
+	assert.Equal(t, []string{"a", "b"}, in.stringSlice("tags"))
+	assert.Nil(t, in.stringSlice("nonexistent"))
 
 	bp := in.BoolOrString("active")
 	require.NotNil(t, bp)