Fix query parameter serialization to respect OpenAPI explode setting#3595
Fix query parameter serialization to respect OpenAPI explode setting#3595
Conversation
jlowin
added
the
bug
marvin-context-protocol
Bot
added
the
openapi
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 947a13e862
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if param_info is not None and isinstance(value, list): | ||
| # OpenAPI default for form style: explode=true | ||
| explode = param_info.explode if param_info.explode is not None else True | ||
| if not explode: | ||
| serialized[key] = ",".join(str(v) for v in value) |
There was a problem hiding this comment.
The new _serialize_query_params() logic only looks at explode: it always comma-joins lists when explode is False, and it defaults omitted explode to True. That is only correct for style=form. Query parameters declared as style=pipeDelimited or style=spaceDelimited now get the wrong wire format (ids=1,2,3 or repeated keys instead of ids=1|2|3 / ids=1%202%203), even though ParameterInfo.style is already parsed and this change is specifically meant to respect style/explode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45579a8a38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not explode: | ||
| style = param_info.style or "form" | ||
| delimiter = self._STYLE_DELIMITERS.get(style, ",") | ||
| serialized[key] = delimiter.join(str(v) for v in value) |
There was a problem hiding this comment.
When an explode: false query array contains non-string scalars, delimiter.join(str(v) for v in value) bypasses httpx’s normal parameter coercion and changes the wire format. For example, {'flags': [True, False]} now becomes ?flags=True,False here, whereas httpx encodes boolean query values as lowercase true/false. That breaks OpenAPI query arrays such as type: array with items.type: boolean against servers that only accept the canonical boolean literals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe8f24b68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serialized[key] = delimiter.join( | ||
| _query_scalar_to_str(v) for v in value |
There was a problem hiding this comment.
When value is an empty list, delimiter.join(...) returns '', and httpx serializes that as ?ids= instead of omitting the parameter entirely. That changes the meaning of valid calls like an optional filter ids=[]: many APIs treat a blank value as invalid or as a single empty element, so the request starts behaving differently solely because the parameter is declared with explode: false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3516476a37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serialized: dict[str, Any] = {} | ||
| for key, value in query_params.items(): | ||
| param_info = param_lookup.get(key) | ||
| if param_info is not None and isinstance(value, list): |
There was a problem hiding this comment.
Any query parameter whose schema is an object and sets explode: false still bypasses the new serializer, because _serialize_query_params() only special-cases list values here. In practice that means OpenAPI-defined forms like style=form (?color=R,100,G,200,B,150) or style=pipeDelimited now still fall through to httpx and get sent as a Python dict repr (?color=%7B'R'%3A+100...%7D), so these endpoints continue to receive invalid query strings even after this explode/style fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63123aab92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serialized[key] = delimiter.join( | ||
| _query_scalar_to_str(v) for v in value |
There was a problem hiding this comment.
When an explode=false query collection contains the style delimiter inside one of its own members, joining the raw strings here and then handing the whole result to httpx makes separators and data indistinguishable. For example, {"values": ["a,b", "c"]} now becomes values=a%2Cb%2Cc; after normal query decoding the server only sees a,b,c, so it cannot reconstruct the original two elements. The same ambiguity affects the object branch below for keys/values containing commas, pipes, or spaces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is inherent to the OpenAPI spec — form + explode=false is defined as delimiter-separated with no escaping mechanism. If a value contains the delimiter character, the format is lossy by design. Not actionable.
| # Delimiter per OpenAPI style when explode=false | ||
| _STYLE_DELIMITERS: ClassVar[dict[str, str]] = { | ||
| "form": ",", | ||
| "spaceDelimited": " ", |
There was a problem hiding this comment.
spaceDelimited separators as %20
Using a literal space for spaceDelimited parameters relies on httpx to emit + in the query string. Servers that parse the URI query per RFC 3986 rather than application/x-www-form-urlencoded rules do not translate + back to a space, so ids=1+2+3 is read as one token instead of a space-delimited list. Any OpenAPI endpoint that actually expects the documented spaceDelimited wire format will break here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
httpx encodes spaces as + via quote_plus, which is standard for query parameters (application/x-www-form-urlencoded). We cannot control this through the params dict API without manually constructing raw query strings — a significant refactor for a marginal edge case. Any server implementing spaceDelimited will accept + in query strings.
Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Jeremiah Lowin <[email protected]> Co-authored-by: Marvin Context Protocol <41898282+Marvin Context [email protected]> Co-authored-by: voidborne-d <[email protected]> Co-authored-by: marvin-context-protocol[bot] <225465937+marvin-context-protocol[bot]@users.noreply.github.com> Co-authored-by: Claude <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: d 🔹 <[email protected]> Co-authored-by: Jeremiah Lowin <[email protected]> Co-authored-by: nightcityblade <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: Bill Easton <[email protected]> Co-authored-by: Sumanshu Nankana <[email protected]> Co-authored-by: Eric Robinson <[email protected]> Co-authored-by: Martim Santos <[email protected]> Co-authored-by: d 🔹 <[email protected]> Co-authored-by: Matthieu B <[email protected]> Co-authored-by: Sascha Buehrle <[email protected]> Co-authored-by: Hakancan <[email protected]> Co-authored-by: nightcityblade <[email protected]> Co-authored-by: Matt Hallowell <[email protected]> Co-authored-by: nate nowack <[email protected]> Co-authored-by: Bill Easton <[email protected]> Co-authored-by: Marcus Shu <[email protected]> Co-authored-by: Rushabh Doshi <[email protected]> Co-authored-by: AIKAWA Shigechika <[email protected]> Co-authored-by: Jeremy Simon <[email protected]> Co-authored-by: Miguel Miranda Dias <[email protected]> Co-authored-by: Anthony James Padavano <[email protected]> Co-authored-by: Mostafa Kamal <[email protected]> Fix auto-close MRE script posting comment without closing (#3386) Fix WorkOS token scope verification bypass 🤖 Generated with Codex (#3407) Fix initialize McpError fallthrough 🤖 Generated with Codex (#3413) Fix transform arg collisions with passthrough params (#3431) Fix get_* returning None when latest version is disabled (#3439) Fix get_* returning None when latest version is disabled (#3421) Fix server lifespan overlap teardown (#3415) Fix $ref output schema object detection regression (#3420) resolved annotations (#3429) Fix async partial callables rejected by iscoroutinefunction (#3438) Fix async partial callables rejected by iscoroutinefunction (#3423) fix: add version to components (#3458) fix: use intent-based flag for OIDC scope patch in load_access_token (#3465) Fixes #3461 fix: normalize Google scope shorthands and surface valid_scopes (#3477) fix: resolve ty 0.0.23 type-checking errors and bump pin (#3481) fix: shield lifespan teardown from cancellation (#3480) fix: forward custom_route endpoints from mounted servers (#3462) fix updates _get_additional_http_routes() to traverse providers, Fixes #3457 fix: remove hardcoded version from CLI help text (#3456) fix: monty 0.0.8 compatibility, drop external_functions from constructor (#3468) fix: task test teardown hanging 5s per test (#3499) Closes #3498 fix: validate workspace path is a directory before cursor install (#3440) Fixes #3426 fix: handle re.error from malformed URI templates in build_regex (#3501) fix: reject empty/OIDC-only required_scopes in AzureProvider (#3503) fix: restrict $ref resolution to local refs only (SSRF/LFI) (#3502) fix warnings and timeouts (#3504) close upgrade check issue when build passes (#3505) Closes #3484 fix: URL-encode path params to prevent SSRF/path traversal (GHSA-vv7q-7jx5-f767) (#3507) fix: prevent path traversal in skill download (#3493) fix: prefer IdP-granted scopes over client-requested scopes in OAuthProxy (#3492) fix: remove unrelated transform and http.py changes from PR scope fix: remove forced follow_redirects from httpx_client_factory calls (#3496) fix: stop passing follow_redirects to httpx_client_factory fix: restore follow_redirects=True for custom httpx client factories Closes #3509 fix: CSRF double-submit cookie check in consent flow (#3519) fix: validate server names in install commands (#3522) fix: use raw strings for regex in pytest.raises match (#3523) fix: reject refresh tokens used as Bearer access tokens (#3524) fix: route ResourcesAsTools/PromptsAsTools through server middleware (#3495) fix: resolve Pyright "Module is not callable" on @tool, @resource, @prompt decorators (#3540) fix: filter warnings by message in KEY_PREFIX test (#3549) fix: suppress output schema for ToolResult subclass annotations (#3548) fix: increase sleep duration in proxy cache tests (#3567) fix: store absolute token expiry to prevent stale expires_in on reload (#3572) fix: preserve tool properties named 'title' during schema compression (#3582) Fix loopback redirect URI port matching per RFC 8252 §7.3 (#3589) Fix app tool routing: visibility check and middleware propagation (#3591) Fix query parameter serialization to respect OpenAPI explode/style settings (#3595) Fix dev apps form: union types, textarea support, JSON parsing (#3597) fix(google): replace deprecated /oauth2/v1/tokeninfo with /oauth2/v3/userinfo (#3603) fix: resolve EntraOBOToken dependency injection through MultiAuth (#3609) fix(docs): correct misleading stateless_http header (#3622) fix: filesystem provider import machinery (#3626) Closes #3625 (issues 2, 3, 6) fix: recover StdioTransport after subprocess exits (#3630) fix(server): preserve mounted tool task metadata (#3632) fix: scope deprecation warning filter to FastMCPDeprecationWarning (#3649) fix imports, add PrefabAppConfig (#3650) fix: resolve CurrentFastMCP/ctx.fastmcp to child server in mounted background tasks (#3651) Fix blocking docs issues: chart imports, Select API, Rx consistency (#3652) closed by default (#3657) Fix prompt caching middleware missing wrap/unwrap round-trip (#3666) fix: serialize object query params per OpenAPI style/explode rules (#3662) Fixes #2857 fix: HTTP request headers not accessible in background task workers (#3631) fix: restore HTTP headers in worker execution path for background tasks (#3681) fix: strip discriminator after dereferencing schemas (#3682) fix: remove stale ty:ignore directives for ty 0.0.26 (#3684) Fix docs gaps in app provider pages (#3690) fix: dev apps log panel UX improvements (#3698) fix dev server empty string args (#3700)
1 participant
RequestDirector.buildwas ignoring theexplodeandstylefields on query parameters, so array values always serialized in exploded form (values=a&values=b) regardless of what the OpenAPI spec declared. Whenexplode=false, the correct serialization depends on the parameter'sstyle:form(default): comma-delimited →ids=1,2,3pipeDelimited: pipe-delimited →ids=1|2|3spaceDelimited: space-delimited →ids=1%202%203The model and parser already extracted and stored these fields — the director just never consulted them. This adds a
_serialize_query_paramsstep between unflattening and URL construction that checks each query parameter'sParameterInfo.explodeandstylesettings and joins list values with the appropriate delimiter.Closes #3594, closes #1543