From 8058734654dc731010edd5ae303d62f8a9a17828 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Thu, 24 Sep 2026 22:13:00 +0100 Subject: [PATCH 1/2] test: guard input shapes by class, and fix --delivery-group the guard found Today's input-shape fixes each came with a regression test for the case that was found. None stopped the same mistake in new code. These do. Array reads of MCP arguments. Input.StringSlice returns nil for anything but a JSON array, so a bare or comma-separated string -- what models send -- was dropped: gateway_metrics_read's "measures is required" (#440), later reverted by a merge unnoticed. It is now unexported (stringSlice), so tool code that reaches for it does not compile; StringList is the reader. A structural test covers the way round that, asserting .([]interface{}) on a tool argument directly, with an allow-list that carries a reason per entry and fails when an entry goes stale. Comma-separated flags. A flag whose help says "comma-separated" promises a list reaches the API. Every one of the 62 is now declared with how its value is sent, and a query filter must be list-valued in listQuery. A new comma-separated flag fails until someone checks it and says how. Writing that table found the #411 bug again. --delivery-group on `gateway event list` and `gateway request events` sent "a,b" as one value, which the API matches against nothing: zero rows, exit 0. The API takes delivery_group exactly as it takes id -- one value or an array, no comma splitting. gateway_events_read over MCP, whose description tells agents to comma-separate, had the same bug. delivery_group is now list-valued, and GetRequestEvents -- which built its own query and so was missed by #411 -- goes through listQuery. Everything else in the audit sends a list: body flags are split into arrays, metrics and Outpost filters are split into slices, and OAuth2 scopes go as one string by design, since the API turns the commas into spaces. Each guard is verified against the failure it exists for: reverting the delivery_group fix fails the flag guard on both commands; a planted .([]interface{}) in a tool file fails the array guard. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_012XtSQ2kfpcRXXqweskgXkH --- pkg/cmd/comma_separated_flags_test.go | 155 ++++++++++++++++++++++++++ pkg/hookdeck/list_query.go | 13 ++- pkg/hookdeck/list_query_test.go | 35 ++++++ pkg/hookdeck/requests.go | 9 +- pkg/mcpcore/input.go | 15 ++- pkg/mcpcore/input_shape_guard_test.go | 73 ++++++++++++ pkg/mcpcore/input_test.go | 4 +- 7 files changed, 293 insertions(+), 11 deletions(-) create mode 100644 pkg/cmd/comma_separated_flags_test.go create mode 100644 pkg/mcpcore/input_shape_guard_test.go diff --git a/pkg/cmd/comma_separated_flags_test.go b/pkg/cmd/comma_separated_flags_test.go new file mode 100644 index 00000000..6bfda7dd --- /dev/null +++ b/pkg/cmd/comma_separated_flags_test.go @@ -0,0 +1,155 @@ +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) { + c.LocalFlags().VisitAll(func(f *pflag.Flag) { + if commaSeparated.MatchString(f.Usage) { + found[c.CommandPath()+" --"+f.Name] = true + } + }) + 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) + } +} diff --git a/pkg/hookdeck/list_query.go b/pkg/hookdeck/list_query.go index b30be2aa..01690f55 100644 --- a/pkg/hookdeck/list_query.go +++ b/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 + "[]" } diff --git a/pkg/hookdeck/list_query_test.go b/pkg/hookdeck/list_query_test.go index 6016f64b..23c79870 100644 --- a/pkg/hookdeck/list_query_test.go +++ b/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"]) +} diff --git a/pkg/hookdeck/requests.go b/pkg/hookdeck/requests.go index 53ec6de2..a91b9343 100644 --- a/pkg/hookdeck/requests.go +++ b/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 { diff --git a/pkg/mcpcore/input.go b/pkg/mcpcore/input.go index e35e7ca4..1f4da200 100644 --- a/pkg/mcpcore/input.go +++ b/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) diff --git a/pkg/mcpcore/input_shape_guard_test.go b/pkg/mcpcore/input_shape_guard_test.go new file mode 100644 index 00000000..5ca4a187 --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/mcpcore/input_test.go b/pkg/mcpcore/input_test.go index e23814d1..d76aa321 100644 --- a/pkg/mcpcore/input_test.go +++ b/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) From 7acd99a50e43a030a680c4ae81450dd45a7e9969 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Thu, 24 Sep 2026 22:27:48 +0100 Subject: [PATCH 2/2] test: walk flags without mutating the shared command tree The comma-separated flag guard walked the tree with cobra's LocalFlags(), which merges every parent's persistent flags into the command as a side effect. That mutated the package-level rootCmd for every later test: root's hidden --api-key appeared on outpost mcp, and TestOutpostMCPCommandIsRegistered failed in CI. It walks Flags() and PersistentFlags() instead, which read without merging. The local run before the push did fail. Its output was piped through head, and log lines from deliberately failing mock requests filled the lines ahead of FAIL, so the failure was cut off rather than seen. Checked this time by exit code, with CI's exact steps -- go test -short ./pkg/... and go vet ./... -- on every open branch. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_012XtSQ2kfpcRXXqweskgXkH --- pkg/cmd/comma_separated_flags_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/comma_separated_flags_test.go b/pkg/cmd/comma_separated_flags_test.go index 6bfda7dd..7b5299bd 100644 --- a/pkg/cmd/comma_separated_flags_test.go +++ b/pkg/cmd/comma_separated_flags_test.go @@ -113,11 +113,18 @@ func discoverCommaSeparatedFlags() map[string]bool { found := map[string]bool{} var walk func(c *cobra.Command) walk = func(c *cobra.Command) { - c.LocalFlags().VisitAll(func(f *pflag.Flag) { + // 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) }