Skip to content

fix: preserve 5 silently-dropped Responses API fields - #4850

Open
Shaik-Sirajuddin wants to merge 5 commits into
maximhq:devfrom
Shaik-Sirajuddin:schema-compatibility/image-generation-result-nullable-v2
Open

fix: preserve 5 silently-dropped Responses API fields#4850
Shaik-Sirajuddin wants to merge 5 commits into
maximhq:devfrom
Shaik-Sirajuddin:schema-compatibility/image-generation-result-nullable-v2

Conversation

@Shaik-Sirajuddin

@Shaik-Sirajuddin Shaik-Sirajuddin commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What

Closes #5167

A systematic audit of core/schemas/responses.go against OpenAI's OpenAPI spec (all 16 tool/item types checked, each verified with a live decode/re-encode trace) found 5 fields silently dropped, one of them via a hard decode error:

  1. ImageGenerationCall.result — non-pointer string coerced a nullable null (image still generating) into "".
  2. MCPTool.allowed_tools — no custom Marshal/Unmarshal existed. The common array-of-tool-names form threw a hard decode error; the filter-object form silently decoded to {}, dropping a security-relevant tool-scoping restriction.
  3. MCPToolCall.approval_request_id — missing entirely, breaking approval-gated MCP replay.
  4. FileSearchTool.ranking_options.hybrid_search — missing entirely.
  5. WebSearchPreviewTool.search_content_types — missing on the web_search_preview variant (already present on plain web_search).

Cross-checked against litellm's own Responses API typing where a typed model exists (approval_request_id, allowed_tools both corroborated); the other two lack litellm signal but are confirmed directly against the OpenAI spec.

How

Added each missing field/struct, matching its real nullability. approval_request_id needed manual shadowing in ResponsesMessage's UnmarshalJSON/MarshalJSON, since it sits two levels behind anonymous pointer embedding that Go's decoder won't auto-promote through — same root cause as server_label in #4844. allowed_tools got a dedicated Marshal/Unmarshal pair, mirroring the existing pattern used for ResponsesToolMCPAllowedToolsApprovalSetting.

Verified against Anthropic, the only other provider constructing these structs directly (via Go literals rather than JSON decode): no regressions, and the allowed_tools fix incidentally also fixed a second latent bug where Anthropic's empty "deny all" allowlist was marshaling to {} instead of [].

Testing

  • 6 new regression tests in core/schemas/responses_test.go, each covering decode + re-encode.
  • go build ./... and go test ./core/schemas/... pass, aside from one pre-existing unrelated failure (TestResponsesMessageToolCallArguments/real_tool_search_call_frames_from_openai), confirmed to fail identically on a clean dev checkout.

OpenAI's image_generation_call.result is nullable while status is
in_progress/generating. Bifrost modeled it as a non-pointer string,
so a null result silently became "" on decode/re-encode.
Found via a systematic audit of core/schemas/responses.go against
OpenAI's OpenAPI spec (all 16 tool/item types checked), each confirmed
by a live decode/re-encode trace:

- MCPTool.allowed_tools: no custom Marshal/Unmarshal existed at all.
  The array form of allowed_tools threw a hard decode error; the
  filter-object form silently decoded to an empty object, dropping
  the tool-scoping restriction entirely.
- MCPToolCall.approval_request_id: missing entirely, breaking
  approval-gated MCP replay (the model has no way to correlate its
  earlier call with a later approval response).
- ResponsesResponseError: missing type/param, so the specific failing
  request field (e.g. "input[1].server_label") was stripped from any
  response-level error before reaching a client.
- FileSearchTool.ranking_options.hybrid_search: missing entirely.
- WebSearchPreviewTool.search_content_types: missing on the
  web_search_preview variant specifically (already present on the
  plain web_search variant).
…viders

Type had no omitempty, so Replicate/Gemini responses (which construct
this struct with only Code/Message set) gained a spurious "type":""
that never existed on their original error.
Scoping this out of the PR to avoid any risk to other providers'
error handling, even though the omitempty fix verified safe for the
current Replicate/Gemini construction sites.
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR updates JSON handling for several OpenAI Responses API schema types in core/schemas/responses.go, including mcp_call approval IDs, nullable image generation results, file search hybrid search options, MCP allowed tools wire formats, and web search preview fields. Matching regression tests were added.

Changes

Responses schema JSON handling

Layer / File(s) Summary
MCP call approval_request_id round-trip
core/schemas/responses.go, core/schemas/responses_test.go
Adds ApprovalRequestID to ResponsesMCPToolCall, routes it through ResponsesMessage unmarshal/marshal for mcp_call items, and adds round-trip coverage.
Nullable image generation result
core/schemas/responses.go, core/schemas/responses_test.go
Changes ResponsesImageGenerationCall.Result to *string so null remains null through decode and encode.
File search hybrid search options
core/schemas/responses.go, core/schemas/responses_test.go
Adds ResponsesToolFileSearchHybridSearch and a HybridSearch field on ResponsesToolFileSearchRankingOptions, with round-trip coverage for the weights.
MCP allowed_tools oneOf marshalling
core/schemas/responses.go, core/schemas/responses_test.go
Adds custom JSON marshaling and unmarshaling for ResponsesToolMCPAllowedTools to support array and filter-object forms.
Web search preview formatting and test
core/schemas/responses.go, core/schemas/responses_test.go
Reformats ResponsesToolWebSearchPreview field layout and adds a search_content_types round-trip test.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • maximhq/bifrost#4121: Both PRs modify core/schemas/responses.go's ResponsesMessage custom JSON handling for type-specific ResponsesMessage items.
  • maximhq/bifrost#4644: Both PRs extend ResponsesMessage.UnmarshalJSON in core/schemas/responses.go to preserve additional mcp_call-related fields.
  • maximhq/bifrost#4766: Both PRs change ResponsesMessage JSON marshal/unmarshal behavior in core/schemas/responses.go for Responses API union items.

Suggested reviewers: akshaydeo, danpiths

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preserving five Responses API fields during round-trip JSON handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description clearly explains the problem, solution, and validation steps, and includes issue linkage and testing details.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 2, 2026
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge — all five field fixes are correct, well-isolated to schemas, and covered by new round-trip tests.

The shadow-and-promote pattern for approval_request_id is mechanically sound (depth-1 field in aux correctly suppresses the depth-4 embedded field during JSON encoding), the custom Marshal/Unmarshal for ResponsesToolMCPAllowedTools handles both oneOf forms correctly, and the Result *string change has no callers that construct the struct via Go literals. The Anthropic converter gap (Filter form silently dropped) is a pre-existing limitation newly made reachable but not a regression introduced by this PR.

No files require special attention; the one observation about core/providers/anthropic/responses.go is a pre-existing converter limitation, not a regression.

Important Files Changed

Filename Overview
core/schemas/responses.go Five missing fields added with correct nullability; custom Marshal/Unmarshal for ResponsesToolMCPAllowedTools handles both oneOf forms; shadow-and-promote pattern for approval_request_id follows the same approach used for server_label in #4844 and correctly suppresses the deeply embedded field during serialization.
core/schemas/responses_test.go Six new regression tests cover all five field fixes plus the Anthropic empty-slice marshaling case; each test exercises decode + re-encode round-trip, and the empty-array/deny-all test explicitly validates the MarshalJSON output is [] and not {} or null.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["OpenAI JSON Input\nmcp_call / mcp tool"] --> B{Field type}

    B -->|"result: null\n(image_generation_call)"| C["ResponsesImageGenerationCall\nResult *string = nil ✓\n(was coerced to '')"]
    B -->|"allowed_tools: [array]"| D["UnmarshalJSON tries []string\n→ ToolNames set ✓\n(was hard error)"]
    B -->|"allowed_tools: {filter}"| E["UnmarshalJSON tries Filter\n→ Filter set ✓\n(was silent empty decode)"]
    B -->|"approval_request_id"| F["Shadowed in aux struct\n→ manually routed to\nResponsesMCPToolCall ✓\n(was missing)"]
    B -->|"hybrid_search"| G["ResponsesToolFileSearchHybridSearch\nnew struct ✓\n(was missing)"]
    B -->|"search_content_types\n(web_search_preview)"| H["ResponsesToolWebSearchPreview\n.SearchContentTypes []string ✓\n(was missing)"]

    D --> I["MarshalJSON: ToolNames != nil\n→ emit as JSON array ✓\nEmpty slice → [] not {}"]
    E --> J["MarshalJSON: Filter != nil\n→ emit as filter object ✓"]
    F --> K["MarshalJSON: isMCPCall path\nApprovalRequestID at depth-1\nshadows embedded depth-4 field\nno double-encoding ✓"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["OpenAI JSON Input\nmcp_call / mcp tool"] --> B{Field type}

    B -->|"result: null\n(image_generation_call)"| C["ResponsesImageGenerationCall\nResult *string = nil ✓\n(was coerced to '')"]
    B -->|"allowed_tools: [array]"| D["UnmarshalJSON tries []string\n→ ToolNames set ✓\n(was hard error)"]
    B -->|"allowed_tools: {filter}"| E["UnmarshalJSON tries Filter\n→ Filter set ✓\n(was silent empty decode)"]
    B -->|"approval_request_id"| F["Shadowed in aux struct\n→ manually routed to\nResponsesMCPToolCall ✓\n(was missing)"]
    B -->|"hybrid_search"| G["ResponsesToolFileSearchHybridSearch\nnew struct ✓\n(was missing)"]
    B -->|"search_content_types\n(web_search_preview)"| H["ResponsesToolWebSearchPreview\n.SearchContentTypes []string ✓\n(was missing)"]

    D --> I["MarshalJSON: ToolNames != nil\n→ emit as JSON array ✓\nEmpty slice → [] not {}"]
    E --> J["MarshalJSON: Filter != nil\n→ emit as filter object ✓"]
    F --> K["MarshalJSON: isMCPCall path\nApprovalRequestID at depth-1\nshadows embedded depth-4 field\nno double-encoding ✓"]
Loading

Reviews (2): Last reviewed commit: "test: add coverage requested by greptile..." | Re-trigger Greptile

Comment thread core/schemas/responses_test.go
Comment thread core/schemas/responses.go
- ResponsesToolMCPAllowedTools with a non-nil empty ToolNames slice
  (Anthropic's deny-all case) now has a direct assertion that it
  marshals as "[]", not "{}" or "null".
- mcp_call with both server_label and approval_request_id present
  together (the common production shape) now has a combined test,
  confirming approval_request_id isn't clobbered regardless of
  whether ResponsesMCPToolCall was already allocated by another
  decode path. server_label itself is not asserted to round-trip
  here — that's tracked separately in maximhq#4844.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: /v1/responses MCP allowed_tools array form errors; approval_request_id and other fields dropped

2 participants