fix: preserve tool_search Responses API wire shape

[devonpmack]

What this fixes

When a client enables the tool_search tool (Codex does this for deferred tool discovery), Bifrost mangles three parts of the Responses API wire shape as it re-serializes streamed items. Each one independently breaks the round-trip, and the visible result is that the model's turn just ends: no tool call, no output, a clean end_turn. From the outside it looks like the agent "randomly stopped."

It only happens through Bifrost — talking to OpenAI directly works — because the corruption is introduced in Bifrost's serialization layer.

Root cause

There are three separate issues, all in how core/schemas/responses.go decodes and re-encodes Responses items:

1. tool_search_call.arguments gets stringified.
   OpenAI sends these arguments as a JSON object ({} while in progress, {"query":"..."} when done), but Bifrost re-emitted them as a JSON string. Codex sees the wrong type and aborts the turn (see tool_search aborts with 'unsupported payload' when using non-native API endpoints (Ollama/Bifrost) openai/codex#20574).

2. execution: "client" is dropped from tool_search_call.
   This flag tells the client the call is its to dispatch. ResponsesToolMessage had no field for it, so it was silently discarded and Codex never dispatched the search.

3. type is dropped from the tools inside tool_search_output.
   The discovered tools are ResponsesTool-shaped (a namespace wrapping function entries, discriminated by type). They were being decoded through ResponsesMCPListTools, which doesn't model type or the nesting, so the follow-up request to OpenAI failed with Missing required parameter: 'input[].tools[].type'.

The fix

• Custom MarshalJSON keeps tool_search_call.arguments as a JSON object while leaving function_call.arguments as a string.
• Added an Execution field to ResponsesToolMessage so execution survives the round-trip.
• Recognize tool_search_output as its own item type and pass its tools array through verbatim as raw JSON. The mcp_list_tools path is untouched.

How to reproduce / verify

The schema tests cover each issue directly:

cd core
go test ./schemas/ -run TestResponsesMessage -v

• TestResponsesMessageMarshalsToolSearchArgumentsAsObject — arguments stay an object (issue 1)
• TestResponsesMessagePreservesToolSearchExecution — execution survives (issue 2)
• TestResponsesMessageRoundTripsToolSearchOutputTools — tool type survives (issue 3)

End-to-end, I ran the real Codex ACP client against two local Bifrost builds (before/after this PR), pointed at a stub MCP server with enough tools to trigger tool_search:

	before	after
requests reaching the model	1	3 (full loop)
tool_search dispatched	no	yes
tool actually called	—	yes
turn outcome	silent end_turn, empty output	completes, result returned

Before: the model emits the tool_search step, gets the broken shape back, and ends the turn with nothing. After: it discovers the tool, calls it, gets the result, and answers.

Question for maintainers

All three bugs come from the same root: Bifrost fully decodes provider responses into its typed schema and then re-marshals them, so any field the schema doesn't model is dropped. These three are the ones I hit, but the pattern means there are likely others lurking for any newer/less-common Responses item.

Is there appetite for a less invasive approach on the response path — stream the provider bytes through to the client untouched, and parse a copy on the side for logging/governance/cost rather than reconstructing the payload from a lossy typed model? That would make Bifrost behave like a true proxy and eliminate this whole class of wire-shape bugs, instead of us patching each dropped field as we find it. Happy to take a stab if you think it's worth it, or if there's a reason the round-trip re-marshal is required I'd like to understand it.

Type of change

• Bug fix

Affected areas

• Core (Go)

Breaking changes

• No

Related issues

openai/codex#20574

Checklist

• Tests added for each fix

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 68f95528-9d9b-4cd2-acf8-6a9b5fda5474

📥 Commits

Reviewing files that changed from the base of the PR and between 918f053 and 6c58c43.

📒 Files selected for processing (3)

• core/schemas/responses.go
• core/schemas/responses_test.go
• core/schemas/utils.go

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Improved end-to-end handling of tool-search responses, preserving nested tool data and restoring correct JSON formatting for tool-call arguments.
  • Added support for carrying optional tool execution metadata through marshaling and cloning.

• Bug Fixes

  • Fixed JSON marshal/unmarshal to prevent lossy transformations of tool-search fields, including proper handling of tool output payloads.

• Tests

  • Expanded test coverage for tool-search round-tripping, argument shape (object vs stringified JSON), and deep-copy preservation.

Walkthrough

Adds tool_search_call and tool_search_output response handling, preserves raw tool payloads and execution metadata, and updates marshal/unmarshal and deep-copy behavior to keep tool-search data round-trippable.

Changes

Tool search response round-trip

Layer / File(s)	Summary
Message types and fields core/schemas/responses.go	Adds tool search message types and fields for raw tool payloads and execution metadata.
Normalization and re-emission core/schemas/responses.go	Captures raw tool_search_output tools during unmarshalling and restores tool-search argument shapes during marshaling.
Deep-copy propagation core/schemas/utils.go, framework/streaming/responses.go	Copies raw tool-search payloads and optional tool-message metadata through schema and streaming deep-copy paths.
Round-trip tests core/schemas/responses_test.go	Tests argument encoding, execution preservation, raw tool payload round-tripping, and deep-copy behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: tejasghatte, danpiths

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title concisely summarizes the main change: preserving tool_search Responses API wire shape.
Description check	✅ Passed	The PR description covers the purpose, root cause, fix, verification, change type, area, issue, and checklist, with only a few template sections omitted.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/schemas/responses.go`:
- Around line 939-942: Add ToolSearchOutputTools to both ResponsesMessage
deep-copy paths: update deepCopyResponsesMessage in
framework/streaming/responses.go and DeepCopyResponsesMessage in
core/schemas/utils.go so the new json.RawMessage field is copied alongside
Author and Recipient. Use the same nil-check and raw-byte append pattern already
used for other RawMessage fields, ensuring the ToolSearchOutputTools payload
survives streaming accumulation and plugin processing without being dropped.
- Line 1222: The ResponsesToolMessage deep-copy helper is dropping the Namespace
and Execution fields, so update the field-by-field copy logic in the
ResponsesToolMessage clone routine in core/schemas/utils.go to include both
*string values. Mirror the existing deref-and-copy handling used for CallID,
Name, Arguments, and Error, and make sure the new branches copy Namespace and
Execution alongside the other optional string fields so tool execution metadata
is preserved.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3427bfb6-d2d8-475d-810c-baeb32f8fb3a

📥 Commits

Reviewing files that changed from the base of the PR and between 5e42852 and 1bb7b10.

📒 Files selected for processing (2)

• core/schemas/responses.go
• core/schemas/responses_test.go

[greptile-apps]

Confidence Score: 2/5

Not safe to merge — the package will not compile as written.

The PR adds a second value-receiver MarshalJSON on ResponsesMessage (line 1191) without removing the one already present at line 1169 from the base dev branch. Go rejects duplicate method declarations. Additionally, the ToolSearchOutputTools capture block in UnmarshalJSON can never execute because the isToolSearchItem guard returns early for every tool_search_* item, making the new typed fields permanently nil via unmarshal and causing all four newly added tests to fail.

core/schemas/responses.go requires the conflict between the rawToolSearch pass-through (line 1169) and the new typed MarshalJSON (line 1191) to be resolved before anything else can be validated.

Important Files Changed

Filename	Overview
core/schemas/responses.go	Adds two constants, ToolSearchOutputTools field, Execution field, and a new MarshalJSON — but the new MarshalJSON is a duplicate of the existing rawToolSearch-based one already in the base branch, causing a compile error; the ToolSearchOutputTools capture in UnmarshalJSON is also unreachable dead code.
core/schemas/responses_test.go	Good test coverage for the three described wire-shape bugs (arguments object shape, execution round-trip, tools type discriminator) plus a deep-copy test; however, all four new tests will fail in the current HEAD because the typed fields they exercise (ToolSearchOutputTools, Execution) are never populated via UnmarshalJSON due to the rawToolSearch early-return.
core/schemas/utils.go	Adds ToolSearchOutputTools, Namespace, and Execution to DeepCopyResponsesMessage; changes are straightforward and correct, though ToolSearchOutputTools will always be nil in practice with the current UnmarshalJSON early-return.
framework/streaming/responses.go	Adds reflection-based helpers (copyRawMessageFieldByName, copyOptionalStringFieldByName) for cross-module version compat, and adds Namespace/Execution copying to the streaming deep-copy; reflect logic is safe (IsValid/IsNil guards prevent panics) and the cross-version compatibility rationale is sound.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming Responses API item] --> B{isToolSearchItem?}
    B -- yes --> C[Store rawToolSearch verbatim\nset Type only\nreturn nil]
    B -- no --> D[Alias decode with Arguments shadow]
    D --> E{Arguments present?}
    E -- yes --> F[responsesToolArgumentsToString\nstore in ResponsesToolMessage.Arguments]
    E -- no --> G[skip]
    F --> H{Type == tool_search_output?}
    H -- yes --> I[DEAD CODE: ToolSearchOutputTools capture\nnever reached due to early return above]
    H -- no --> J[return nil]
    G --> J
    C --> K[MarshalJSON dispatch]
    J --> K
    K --> L{rawToolSearch != nil?}
    L -- yes --> M[Re-emit verbatim bytes at line 1169]
    L -- no --> N[COMPILE ERROR: Second MarshalJSON defined at line 1191]

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[Incoming Responses API item] --> B{isToolSearchItem?}
    B -- yes --> C[Store rawToolSearch verbatim\nset Type only\nreturn nil]
    B -- no --> D[Alias decode with Arguments shadow]
    D --> E{Arguments present?}
    E -- yes --> F[responsesToolArgumentsToString\nstore in ResponsesToolMessage.Arguments]
    E -- no --> G[skip]
    F --> H{Type == tool_search_output?}
    H -- yes --> I[DEAD CODE: ToolSearchOutputTools capture\nnever reached due to early return above]
    H -- no --> J[return nil]
    G --> J
    C --> K[MarshalJSON dispatch]
    J --> K
    K --> L{rawToolSearch != nil?}
    L -- yes --> M[Re-emit verbatim bytes at line 1169]
    L -- no --> N[COMPILE ERROR: Second MarshalJSON defined at line 1191]

Loading

_{Reviews (4): Last reviewed commit: "Merge branch 'dev' into fix/tool-search-..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

framework/streaming/responses.go (1)

238-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace reflection-based field copies with direct assignment.

copyRawMessageFieldByName / copyOptionalStringFieldByName use reflect + string field names to do what is plain field copying. The companion implementation in core/schemas/utils.go (DeepCopyResponsesMessage) copies the very same ToolSearchOutputTools, Namespace, and Execution fields with direct assignment — so we now have two divergent approaches for identical logic. Concerns with the reflection variant here:

• Silent failure on rename: both helpers return quietly when FieldByName is invalid. If ToolSearchOutputTools or Execution is ever renamed, the copy is silently dropped and the tool-search round-trip bug this PR fixes regresses with no compile error.
• Hot path cost: this runs per ResponsesMessage during stream accumulation; reflect.ValueOf/FieldByName/Set add avoidable allocations and lookups versus a direct copy.
• Local inconsistency: Namespace is copied inline (Lines 281–284) while Execution goes through reflection (Line 286).

Direct assignment matches the rest of this function and core/schemas/utils.go, and is compile-time safe.

♻️ Proposed direct-copy refactor

-	copyRawMessageFieldByName(&copy, original, "ToolSearchOutputTools")
+	if original.ToolSearchOutputTools != nil {
+		copy.ToolSearchOutputTools = append(json.RawMessage(nil), original.ToolSearchOutputTools...)
+	}

-		copyOptionalStringFieldByName(copy.ResponsesToolMessage, original.ResponsesToolMessage, "Execution")
+		if original.ResponsesToolMessage.Execution != nil {
+			copyExecution := *original.ResponsesToolMessage.Execution
+			copy.ResponsesToolMessage.Execution = &copyExecution
+		}

Then drop the now-unused copyRawMessageFieldByName / copyOptionalStringFieldByName helpers (Lines 460–485) and the reflect import.

Also applies to: 460-485

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@framework/streaming/responses.go` at line 238, Replace the reflection-based
field copying in ResponsesMessage aggregation with direct assignment: in the
copy logic around ToolSearchOutputTools, Execution, and Namespace, assign the
fields directly instead of calling
copyRawMessageFieldByName/copyOptionalStringFieldByName. Then remove the
now-unused helper functions from responses.go and drop the reflect import,
keeping the implementation aligned with DeepCopyResponsesMessage in
core/schemas/utils.go and compile-time safe.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@framework/streaming/responses.go`:
- Line 238: Replace the reflection-based field copying in ResponsesMessage
aggregation with direct assignment: in the copy logic around
ToolSearchOutputTools, Execution, and Namespace, assign the fields directly
instead of calling copyRawMessageFieldByName/copyOptionalStringFieldByName. Then
remove the now-unused helper functions from responses.go and drop the reflect
import, keeping the implementation aligned with DeepCopyResponsesMessage in
core/schemas/utils.go and compile-time safe.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3660f047-eb55-43b0-8d83-e8e95946d046

📥 Commits

Reviewing files that changed from the base of the PR and between 1bb7b10 and e5af582.

📒 Files selected for processing (4)

• core/schemas/responses.go
• core/schemas/responses_test.go
• core/schemas/utils.go
• framework/streaming/responses.go

🚧 Files skipped from review as they are similar to previous changes (2)

• core/schemas/responses_test.go
• core/schemas/responses.go

The merge-base changed after approval.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ devonpmack
❌ akshaydeo
_{You have signed the CLA already but the status is still pending? Let us recheck it.}