fix: preserve tool search responses API round-trip for Codex - #4749
fix: preserve tool search responses API round-trip for Codex#4749Shaik-Sirajuddin wants to merge 14 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds tool-search response types and wire-format handling, including preservation of object arguments, execution, and discovered tools. Core and streaming deep-copy paths now retain nested tool-search fields without aliasing, with round-trip and mutation-isolation tests. ChangesTool-search response handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Confidence Score: 4/5This is close, but this tool-search copy path should be fixed before merging.
core/schemas/responses.go Important Files Changed
Reviews (8): Last reviewed commit: "fix: resolve tools field collision betwe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1240-1245: ResponsesToolMessage currently collides on the json
"tools" field because it embeds ResponsesMCPListTools, so the new top-level
Tools []ResponsesTool will shadow the MCP tools payload during
marshal/unmarshal. Fix this by renaming the field in ResponsesToolMessage or by
adding explicit MarshalJSON/UnmarshalJSON handling so tool_search_output data
and MCP list_tools data both round-trip correctly without clobbering each other.
In `@core/schemas/utils.go`:
- Around line 1328-1332: `copy.ResponsesToolMessage.Tools` in the deep-copy
helper is still performing only a shallow copy because `ResponsesTool` contains
nested pointers/slices that are copied by value. Update the copy logic to
recursively deep-copy each `ResponsesTool` (including nested metadata such as
`Name` and `ResponsesToolNamespace.Tools`) so mutations to the original cannot
leak into the copy, and reuse the same deep-copy helper from
`framework/streaming/responses.go` instead of assigning the struct directly.
In `@framework/streaming/responses.go`:
- Around line 294-298: The current tool copy in ResponsesMessage cloning is only
a shallow struct copy, so nested fields inside schemas.ResponsesTool can still
alias and mutate already published stream items. Update the copy logic in the
ResponsesToolMessage handling to use a recursive deep copy helper such as
deepCopyResponsesTool for each tool instead of assigning the struct by value,
ensuring nested slices/pointers like ResponsesToolNamespace.Tools are fully
cloned and isolated from later mutations.
🪄 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: 5125029c-186d-4488-a10a-2f33832d712a
📒 Files selected for processing (6)
core/schemas/responses.gocore/schemas/responses_test.gocore/schemas/utils.gocore/schemas/utils_test.goframework/streaming/responses.goframework/streaming/responses_test.go
|
multi_agent v1 tools are made available via deferred tool_search tool |
|
|
…_list_tools ResponsesToolMessage.Tools (added for tool_search_output round-tripping) and the embedded ResponsesMCPListTools.Tools (mcp_list_tools) both used the wire key "tools". Struct-tag-based encoding can't disambiguate two same-named fields at different embedding depths — the shallower field silently wins for every message type and the other never survives marshal/unmarshal, which would have broken mcp_list_tools' own tools array. Tags the new field json:"-" and routes "tools" manually in ResponsesMessage's (Un)MarshalJSON based on Type, the same pattern already used for arguments/execution. Found by a CodeRabbit review comment on PR maximhq#4749.
The merge-base changed after approval.
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…ximhq#4719) ## Summary Fixes the layout of the Audit Logs page to correctly fill the viewport and apply the appropriate background and border styles. ## Changes - Replaced `h-[calc(100dvh-1rem)]` with `h-[calc(100vh-16px)]` for consistent viewport height calculation - Swapped `mx-auto flex flex-col p-4` utility classes for `no-border-parent bg-background flex` to align with the layout conventions used elsewhere in the app - Added missing newline at end of file ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test Navigate to the Audit Logs page and verify: - The page fills the full viewport height without overflow or clipping - The background color and border styling match the rest of the workspace layout ```sh cd ui pnpm i || npm i pnpm build || npm run build ``` ## Screenshots/Recordings Add before/after screenshots showing the corrected Audit Logs page layout. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds collapsible tag support to the `TagInput` component and applies it to the keyword lists on the Complexity Router page. When a keyword list exceeds a configurable limit, tags beyond that limit are hidden behind a gradient overlay with a "Show more" toggle, keeping the UI compact while still allowing full access to all tags. ## Changes - Added `collapsedTagLimit` and `expandButtonTestId` props to `TagInput`. When `collapsedTagLimit` is provided, the component renders in a collapsible layout: tags beyond the limit are hidden with a fade gradient, and "Show more" / "Show less" buttons toggle the expanded state. The collapsed state auto-resets when the tag count drops back to or below the limit. - Set `KEYWORD_COLLAPSED_LIMIT = 8` on the Complexity Router page and passed it along with a `expandButtonTestId` to each keyword `TagInput`. - Standardized border radius tokens from `rounded-lg`/`rounded-md`/`rounded-full` to `rounded-sm` across the Complexity Router page for visual consistency. - Reformatted `index.html` inline shell skeleton from a single minified line to readable, indented HTML and CSS. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` 1. Navigate to the Complexity Router page. 2. Add more than 8 keywords to any keyword list. 3. Verify that tags beyond 8 are hidden with a gradient overlay and a "Show more" button appears. 4. Click "Show more" and confirm all tags are visible with a "Show less" button. 5. Click "Show less" and confirm the list collapses again. 6. Remove tags until 8 or fewer remain and confirm the list stays expanded without the toggle controls. ## Screenshots/Recordings Before/after screenshots of the keyword lists with collapse behavior recommended. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
OpenAI sends tool_search_call.arguments as a JSON object on the wire.
The existing UnmarshalJSON (added in the previous commit) normalizes this
into the *string field so internal Bifrost code is unaffected.
This commit adds the symmetric MarshalJSON on ResponsesMessage so that
when Bifrost re-emits a tool_search_call item to the client, arguments
is serialized back to a JSON object rather than a quoted string.
Also adds ResponsesMessageTypeToolSearchCall constant and unit tests
covering:
- completed frame: arguments marshaled as JSON object
- in_progress frame: empty-object arguments marshaled as {}
- full round-trip: object -> unmarshal -> marshal -> object
- regression guard: function_call arguments still marshal as string
…bility ResponsesToolMessage was missing an Execution field, so OpenAI's "execution" value was silently dropped on ingest and could never be re-emitted. Codex (codex-rs) requires the field to be present and non-optional on ToolSearchCall items; without it serde fails with "missing field execution" and the item is dropped, causing search to never execute. Changes: - Add Execution *string to ResponsesToolMessage so the field survives unmarshal from OpenAI's SSE frames - In MarshalJSON's tool_search_call branch, always emit execution via the aux struct (suppress from embedded toolCopy to avoid double-emit): use the stored value if present, default to "client" otherwise - Update existing unmarshal test to assert execution is captured - Add two new marshal sub-tests: default-to-client and preserve-custom - Add execution assertions to existing marshal sub-tests
- tests/e2e/clis/codex_tool_search_e2e_test.go: live e2e for Codex->Bifrost
Responses path; verifies tool search discovers and names the shell tool
- transports/bifrost-http/integrations/openai_tool_search_test.go: unit tests
for normalizeOpenAIResponsesRaw{,Stream}Response ensuring tool_search_call
arguments are re-emitted as a JSON object, not a string-escaped value
Removes duplicate subtests that asserted the same behavior twice (execution defaulting, multi-tool type preservation) and drops the second MCP-tool variant from both deep-copy tests, since DeepCopyResponsesTool copies every ResponsesTool union variant identically via JSON round-trip and testing a second variant added no new coverage.
…_list_tools ResponsesToolMessage.Tools (added for tool_search_output round-tripping) and the embedded ResponsesMCPListTools.Tools (mcp_list_tools) both used the wire key "tools". Struct-tag-based encoding can't disambiguate two same-named fields at different embedding depths — the shallower field silently wins for every message type and the other never survives marshal/unmarshal, which would have broken mcp_list_tools' own tools array. Tags the new field json:"-" and routes "tools" manually in ResponsesMessage's (Un)MarshalJSON based on Type, the same pattern already used for arguments/execution. Found by a CodeRabbit review comment on PR maximhq#4749.
5bfaaf2 to
b47a38c
Compare
| switch { | ||
| case m.Type != nil && *m.Type == ResponsesMessageTypeToolSearchOutput: | ||
| var tools []ResponsesTool | ||
| if err := Unmarshal(aux.Tools, &tools); err != nil { | ||
| return fmt.Errorf("tool_search_output tools: %w", err) | ||
| } | ||
| if m.ResponsesToolMessage == nil { | ||
| m.ResponsesToolMessage = &ResponsesToolMessage{} | ||
| } | ||
| m.ResponsesToolMessage.Tools = tools |
There was a problem hiding this comment.
tool_search_output branch does not run for the real Codex round-trip path because tool_search_output is handled by the raw-preserved fast path earlier in UnmarshalJSON. That path returns after storing the raw bytes and only surfaces arguments, so the new typed Execution and ResponsesToolMessage.Tools fields stay empty. The new deep-copy paths copy those typed fields and do not copy the private raw bytes, so an unmarshal → deep-copy → marshal flow can still produce a tool_search_output without the discovered tools or execution mode. Populate these fields before returning from the raw-preserved path, or keep the raw bytes preserved across the copy while keeping the typed fields in sync.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/schemas/responses_test.go (2)
278-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that typed fields are populated during unmarshal.
Because the test only verifies the marshaled string (which passes due to
rawPreserved), it masks the fact that fields likeExecutionwere not actually populated on the struct during unmarshal.I recommend asserting the parsed fields explicitly to ensure they are correctly hydrated and available for downstream consumers:
💡 Suggested enhancement
if msg.Arguments == nil || *msg.Arguments != `{"query":"observability","limit":10}` { t.Fatalf("expected Arguments surfaced as string, got: %v", msg.Arguments) } + if msg.ResponsesToolMessage == nil || msg.ResponsesToolMessage.Execution == nil || *msg.ResponsesToolMessage.Execution != "client" { + t.Fatalf("expected Execution field to be populated, got: %#v", msg.ResponsesToolMessage) + } encoded, err := MarshalSorted(msg)🤖 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 `@core/schemas/responses_test.go` around lines 278 - 295, Extend the “completed frame round-trips arguments object and execution verbatim” test to assert that unmarshalling populates the typed Execution field with “client” and the parsed arguments value with the expected query and limit, rather than relying only on MarshalSorted’s rawPreserved output. Keep the existing round-trip assertion intact.
538-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the
Toolsarray is populated during unmarshal.This test verifies that
MarshalSortedproduces the correct JSON output, but becausetool_search_outputusesrawPreserved, the marshaling will succeed even if the struct fields were left entirely uninitialized byUnmarshalJSON.To ensure the typed fields are available for deep-copy and downstream introspection, consider adding an assertion before marshaling to confirm that
msg.ResponsesToolMessage.Toolsis populated with the expected tools.🤖 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 `@core/schemas/responses_test.go` around lines 538 - 569, Extend the test loop for ResponsesMessage to assert that Unmarshal populates msg.ResponsesToolMessage.Tools before calling MarshalSorted. Verify the slice contains the expected tool entries from the current test case, ensuring typed fields are initialized rather than relying only on rawPreserved output.
🤖 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 235-238: Update BifrostResponsesResponse.WithDefaults() to copy
Speed, InferenceGeo, Diagnostics, and Container into the normalized response.
Preserve each field’s existing pointer/value semantics so telemetry, billing,
and response metadata are not dropped.
- Around line 1166-1175: Remove the early return in UnmarshalJSON’s
isRawPreservedItem branch after setting Type, tool arguments, and rawPreserved.
Allow execution to continue into the aux unmarshalling and manual routing logic
so raw-preserved tool_search_output values populate fields such as Execution and
ResponsesToolMessage.Tools while retaining the preserved raw bytes and
arguments.
- Around line 1120-1132: Update DeepCopyResponsesMessage in
core/schemas/utils.go to deep-copy the rawPreserved byte slice when copying
ResponsesMessage values. Preserve nil as nil and allocate independent storage
for non-nil payloads so copied tool_search_call, tool_search_output, and
additional_tools messages retain their original bytes during marshaling.
---
Nitpick comments:
In `@core/schemas/responses_test.go`:
- Around line 278-295: Extend the “completed frame round-trips arguments object
and execution verbatim” test to assert that unmarshalling populates the typed
Execution field with “client” and the parsed arguments value with the expected
query and limit, rather than relying only on MarshalSorted’s rawPreserved
output. Keep the existing round-trip assertion intact.
- Around line 538-569: Extend the test loop for ResponsesMessage to assert that
Unmarshal populates msg.ResponsesToolMessage.Tools before calling MarshalSorted.
Verify the slice contains the expected tool entries from the current test case,
ensuring typed fields are initialized rather than relying only on rawPreserved
output.
🪄 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: 4b7a8b2f-2369-4c06-9b53-e2146427529b
📒 Files selected for processing (6)
core/schemas/responses.gocore/schemas/responses_test.gocore/schemas/utils.gocore/schemas/utils_test.goframework/streaming/responses.goframework/streaming/responses_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/utils.go
- framework/streaming/responses_test.go
- core/schemas/utils_test.go
- framework/streaming/responses.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/schemas/responses_test.go (2)
278-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that typed fields are populated during unmarshal.
Because the test only verifies the marshaled string (which passes due to
rawPreserved), it masks the fact that fields likeExecutionwere not actually populated on the struct during unmarshal.I recommend asserting the parsed fields explicitly to ensure they are correctly hydrated and available for downstream consumers:
💡 Suggested enhancement
if msg.Arguments == nil || *msg.Arguments != `{"query":"observability","limit":10}` { t.Fatalf("expected Arguments surfaced as string, got: %v", msg.Arguments) } + if msg.ResponsesToolMessage == nil || msg.ResponsesToolMessage.Execution == nil || *msg.ResponsesToolMessage.Execution != "client" { + t.Fatalf("expected Execution field to be populated, got: %#v", msg.ResponsesToolMessage) + } encoded, err := MarshalSorted(msg)🤖 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 `@core/schemas/responses_test.go` around lines 278 - 295, Extend the “completed frame round-trips arguments object and execution verbatim” test to assert that unmarshalling populates the typed Execution field with “client” and the parsed arguments value with the expected query and limit, rather than relying only on MarshalSorted’s rawPreserved output. Keep the existing round-trip assertion intact.
538-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the
Toolsarray is populated during unmarshal.This test verifies that
MarshalSortedproduces the correct JSON output, but becausetool_search_outputusesrawPreserved, the marshaling will succeed even if the struct fields were left entirely uninitialized byUnmarshalJSON.To ensure the typed fields are available for deep-copy and downstream introspection, consider adding an assertion before marshaling to confirm that
msg.ResponsesToolMessage.Toolsis populated with the expected tools.🤖 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 `@core/schemas/responses_test.go` around lines 538 - 569, Extend the test loop for ResponsesMessage to assert that Unmarshal populates msg.ResponsesToolMessage.Tools before calling MarshalSorted. Verify the slice contains the expected tool entries from the current test case, ensuring typed fields are initialized rather than relying only on rawPreserved output.
🤖 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 235-238: Update BifrostResponsesResponse.WithDefaults() to copy
Speed, InferenceGeo, Diagnostics, and Container into the normalized response.
Preserve each field’s existing pointer/value semantics so telemetry, billing,
and response metadata are not dropped.
- Around line 1166-1175: Remove the early return in UnmarshalJSON’s
isRawPreservedItem branch after setting Type, tool arguments, and rawPreserved.
Allow execution to continue into the aux unmarshalling and manual routing logic
so raw-preserved tool_search_output values populate fields such as Execution and
ResponsesToolMessage.Tools while retaining the preserved raw bytes and
arguments.
- Around line 1120-1132: Update DeepCopyResponsesMessage in
core/schemas/utils.go to deep-copy the rawPreserved byte slice when copying
ResponsesMessage values. Preserve nil as nil and allocate independent storage
for non-nil payloads so copied tool_search_call, tool_search_output, and
additional_tools messages retain their original bytes during marshaling.
---
Nitpick comments:
In `@core/schemas/responses_test.go`:
- Around line 278-295: Extend the “completed frame round-trips arguments object
and execution verbatim” test to assert that unmarshalling populates the typed
Execution field with “client” and the parsed arguments value with the expected
query and limit, rather than relying only on MarshalSorted’s rawPreserved
output. Keep the existing round-trip assertion intact.
- Around line 538-569: Extend the test loop for ResponsesMessage to assert that
Unmarshal populates msg.ResponsesToolMessage.Tools before calling MarshalSorted.
Verify the slice contains the expected tool entries from the current test case,
ensuring typed fields are initialized rather than relying only on rawPreserved
output.
🪄 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: 4b7a8b2f-2369-4c06-9b53-e2146427529b
📒 Files selected for processing (6)
core/schemas/responses.gocore/schemas/responses_test.gocore/schemas/utils.gocore/schemas/utils_test.goframework/streaming/responses.goframework/streaming/responses_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/utils.go
- framework/streaming/responses_test.go
- core/schemas/utils_test.go
- framework/streaming/responses.go
🛑 Comments failed to post (3)
core/schemas/responses.go (3)
235-238: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Ensure new fields are preserved in
WithDefaults().The fields
Speed,InferenceGeo,Diagnostics, andContainerwere added toBifrostResponsesResponse, but they appear to be missing from theWithDefaults()cloning method (lines 313-464).If
WithDefaults()is used to construct normalized responses, these fields will be dropped. This could break downstream telemetry, observability, or billing logic (e.g., Anthropic fast-mode billing which relies onSpeed). Please add them toWithDefaults()to ensure they are safely propagated.🤖 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 `@core/schemas/responses.go` around lines 235 - 238, Update BifrostResponsesResponse.WithDefaults() to copy Speed, InferenceGeo, Diagnostics, and Container into the normalized response. Preserve each field’s existing pointer/value semantics so telemetry, billing, and response metadata are not dropped.
1120-1132: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg 'rawPreserved' core/schemas/utils.goRepository: maximhq/bifrost
Length of output: 153
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== utils.go outline ==" ast-grep outline core/schemas/utils.go --view expanded || true echo echo "== search for DeepCopyResponsesMessage and rawPreserved ==" rg -n 'DeepCopyResponsesMessage|rawPreserved' core/schemas -S echo echo "== locate responses-related deep copy code ==" rg -n 'DeepCopy.*ResponsesMessage|ResponsesMessage' core/schemas/utils.go core/schemas -SRepository: maximhq/bifrost
Length of output: 33660
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1265,1395p' core/schemas/utils.goRepository: maximhq/bifrost
Length of output: 4971
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== rawPreserved in DeepCopyResponsesMessage body ==" awk ' /func DeepCopyResponsesMessage\(original ResponsesMessage\) ResponsesMessage/ {infn=1} infn && /}/ && depth==0 {print NR ":" $0; exit} infn { if ($0 ~ /{/ ) depth++ if ($0 ~ /}/ ) depth-- if ($0 ~ /rawPreserved/) print NR ":" $0 } ' core/schemas/utils.go echo echo "== test covering deep copy behavior ==" sed -n '1,170p' core/schemas/utils_test.goRepository: maximhq/bifrost
Length of output: 5708
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1130,1250p' core/schemas/responses.goRepository: maximhq/bifrost
Length of output: 5108
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1395,1475p' core/schemas/utils.goRepository: maximhq/bifrost
Length of output: 3560
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'DeepCopyResponsesMessage\(' core/schemas -SRepository: maximhq/bifrost
Length of output: 333
Copy
rawPreservedinDeepCopyResponsesMessage
rawPreservedneeds to be deep-copied incore/schemas/utils.go; otherwise copiedtool_search_call/tool_search_output/additional_toolsmessages lose their verbatim payload and re-marshal through the struct path instead of preserving the original bytes.🤖 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 `@core/schemas/responses.go` around lines 1120 - 1132, Update DeepCopyResponsesMessage in core/schemas/utils.go to deep-copy the rawPreserved byte slice when copying ResponsesMessage values. Preserve nil as nil and allocate independent storage for non-nil payloads so copied tool_search_call, tool_search_output, and additional_tools messages retain their original bytes during marshaling.
1166-1175: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix early return preventing parsing of
tool_search_outputtyped fields.The early return at line 1174 prevents the rest of
UnmarshalJSONfrom executing forrawPreserveditems. As a result, the manual routing ofaux.Toolstom.ResponsesToolMessage.Tools(lines 1194-1202) is unreachable dead code, and fields likeExecutionandToolswill remainnilon the parsed struct.Remove the early return so that
auxcan unmarshal the standard fields (likeCallIDandExecution) and the manual routing logic can correctly populateTools. Sinceauxintentionally interceptsargumentsandtoolsintojson.RawMessage, the standard struct unmarshaler won't crash on object-shaped arguments or mismatched tool arrays.🐛 Proposed fix to remove early return
*m = ResponsesMessage{} if t := gjson.GetBytes(data, "type").String(); isRawPreservedItem(t) { - mt := ResponsesMessageType(t) - m.Type = &mt - // Also surface `arguments` (a JSON object for tool_search_call) so downstream - // consumers that read Arguments keep working; MarshalJSON still re-emits the - // preserved bytes verbatim, so this is additive and does not affect round-trip. - m.setToolArguments(json.RawMessage(gjson.GetBytes(data, "arguments").Raw)) m.rawPreserved = append([]byte(nil), data...) - return nil } type Alias ResponsesMessage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if t := gjson.GetBytes(data, "type").String(); isRawPreservedItem(t) { m.rawPreserved = append([]byte(nil), data...) }🤖 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 `@core/schemas/responses.go` around lines 1166 - 1175, Remove the early return in UnmarshalJSON’s isRawPreservedItem branch after setting Type, tool arguments, and rawPreserved. Allow execution to continue into the aux unmarshalling and manual routing logic so raw-preserved tool_search_output values populate fields such as Execution and ResponsesToolMessage.Tools while retaining the preserved raw bytes and arguments.
44564de to
493bff0
Compare
244a01d to
ce1b2a6
Compare
5 participants
Codex's tool-search Responses API calls broke on round-trip because Bifrost re-marshaled
tool_search_call/tool_search_outputfields incorrectly or dropped them in deep-copy paths.Fixes #4713
Details
tool_search_call.argumentsas a JSON object instead of a quoted string.executionfortool_search_call/tool_search_output.tool_search_output.tools[].typeon request round-trip.Checklist