test: guard input shapes by class, and fix --delivery-group the guard found - #455
Merged
Merged
Conversation
… 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XtSQ2kfpcRXXqweskgXkH
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XtSQ2kfpcRXXqweskgXkH
This was referenced Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today's input-shape fixes (#440, #411, #442, #447) each came with a regression test for the case that was found. None stopped the same mistake in new code. These do — and building one of them found the #411 bug again.
Bug found:
--delivery-group a,breturns nothinggateway event list --delivery-group a,bandgateway request events --delivery-group a,bsent"a,b"as one value, which the API matches against nothing: zero rows, exit 0. The API takesdelivery_groupexactly as it takesid— a single value or an array, no comma splitting — so this is the #411 bug on another parameter.gateway_events_readover MCP had the same bug; its description tells agents to "comma-separate several".Fix:
delivery_groupis list-valued inlistQuery, andGetRequestEvents— which built its own query, and so was missed by #411 — now goes throughlistQuery.Not verified live: delivery groups are feature-flagged and no project I can read has events carrying one (#397 notes there is no acceptance coverage either). The evidence is that the API accepts
delivery_groupidentically toid, whose behaviour was verified live in #444 (0 rows → 2).Guard 1 — list arguments to MCP tools
Input.StringSlicereturnednilfor anything but a JSON array, so a bare or comma-separated string was dropped (gateway_metrics_read still rejects a string "measures"; the v3.0.0 fix was reverted by a merge #440, then reverted by a merge unnoticed). It is now unexported, so tool code that reaches for it does not compile.StringList, which accepts all three forms, is the reader..([]interface{})on a tool argument directly. One existing use is allow-listed with its reason (rulesis an array of objects, and a non-array errors rather than being dropped). The allow-list fails when an entry goes stale.Guard 2 — comma-separated flags
A flag whose help says "comma-separated" promises a list reaches the API. All 62 are now declared with how the 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. Stale entries fail too.The audit that built the table:
--id×2,--delivery-group×2--idfixed in #411;--delivery-groupfixed here--connection-ids--measures/--dimensions, every Outpost filter--destination-oauth2-scopesProven against the failure each guard exists for
delivery_groupfix fails the flag guard on both commands..([]interface{})in a tool file fails the array guard.Not covered
Guard 1 ensures every read that treats an MCP argument as a list goes through
StringList, so a bare or comma-separated string is never dropped. It does not cover the reverse: a tool that declares an argument as an array but reads it within.String(), which would drop a genuine array. Nothing in this PR guards that. A schema-driven test that sends a real array to every list-typed argument would; it needs a valid call per tool, so it is left as a follow-up rather than folded in here.Intended for v3.0.3.
🤖 Generated with Claude Code
https://claude.ai/code/session_012XtSQ2kfpcRXXqweskgXkH