[DO NOT MERGE] fix(openai): preserve responses tool fields (#3063)#3127
[DO NOT MERGE] fix(openai): preserve responses tool fields (#3063)#3127roroghost17 wants to merge 3 commits intov1.4.xfrom
Conversation
Co-authored-by: Prince Pal <princepal7021@gmail.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds support for a new Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Confidence Score: 1/5Not safe to merge — both new UnmarshalJSON methods will cause a stack overflow at runtime. Two P0 infinite-recursion bugs affect core deserialization paths (ResponsesMessage and BifrostResponsesStreamResponse). Any code that deserializes these types will crash immediately with a stack overflow. core/schemas/responses.go — both UnmarshalJSON implementations must be fixed with a type alias before merging. Important Files Changed
Reviews (3): Last reviewed commit: "add custom unmarshaller" | 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: 1
🧹 Nitpick comments (1)
core/providers/openai/responses_test.go (1)
1608-1678: Add a directnamespacemarshal/unmarshal round-trip test.This covers provider conversion well, but it still doesn't exercise the new
ResponsesTool.MarshalJSON/UnmarshalJSONnamespace branches incore/schemas/responses.go. A schema-level round-trip test, parallel totool_search, would pin the union wiring independently ofToOpenAIResponsesRequest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/responses_test.go` around lines 1608 - 1678, Add a new unit test that performs a direct marshal/unmarshal round-trip for the namespace branch of the ResponsesTool union to exercise ResponsesTool.MarshalJSON and ResponsesTool.UnmarshalJSON in core/schemas/responses.go; create a ResponsesTool value with Type=ResponsesToolTypeNamespace and a populated ResponsesToolNamespace.Tools slice (including a child function tool with Parameters), marshal it to JSON, unmarshal back to a ResponsesTool, and assert the resulting struct has the same Type, non-nil ResponsesToolNamespace, and preserved child tool fields (name, type, parameters); name the test something like TestResponsesTool_MarshalUnmarshal_Namespace so it parallels the existing tool_search round-trip test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/responses.go`:
- Line 321: The ResponsesToolNamespace type is being allowed through without
recursing into namespace.Tools, so child tools (e.g., nested function schemas)
skip the top-level normalization/filtering path; update the Responses handling
to iterate over ResponsesToolNamespace.Tools and run the same normalize/filter
helper used by ToOpenAIResponsesRequest (ensuring Parameters.Normalized() is
applied) on each child tool so unsupported fields/types are stripped and nested
function parameters are normalized before being added to the final map.
---
Nitpick comments:
In `@core/providers/openai/responses_test.go`:
- Around line 1608-1678: Add a new unit test that performs a direct
marshal/unmarshal round-trip for the namespace branch of the ResponsesTool union
to exercise ResponsesTool.MarshalJSON and ResponsesTool.UnmarshalJSON in
core/schemas/responses.go; create a ResponsesTool value with
Type=ResponsesToolTypeNamespace and a populated ResponsesToolNamespace.Tools
slice (including a child function tool with Parameters), marshal it to JSON,
unmarshal back to a ResponsesTool, and assert the resulting struct has the same
Type, non-nil ResponsesToolNamespace, and preserved child tool fields (name,
type, parameters); name the test something like
TestResponsesTool_MarshalUnmarshal_Namespace so it parallels the existing
tool_search round-trip test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997caa4a-ed9c-48c9-85d0-672f992ec47f
📒 Files selected for processing (3)
core/providers/openai/responses.gocore/providers/openai/responses_test.gocore/schemas/responses.go
| schemas.ResponsesToolTypeWebSearchPreview: true, | ||
| schemas.ResponsesToolTypeMemory: true, | ||
| schemas.ResponsesToolTypeToolSearch: true, | ||
| schemas.ResponsesToolTypeNamespace: true, |
There was a problem hiding this comment.
Recurse into namespace.Tools before allowing this type through.
Now that namespace survives filtering, its child tools bypass the top-level normalization/sanitization path. Nested function schemas never go through Parameters.Normalized(), and unsupported child tool fields/types can still leak inside ResponsesToolNamespace.Tools, which can change request-prefix serialization and still trigger OpenAI validation errors.
Suggested direction
+func normalizeAndFilterTools(tools []schemas.ResponsesTool) []schemas.ResponsesTool {
+ filtered := make([]schemas.ResponsesTool, 0, len(tools))
+ for _, tool := range tools {
+ switch tool.Type {
+ case schemas.ResponsesToolTypeFunction:
+ if tool.ResponsesToolFunction != nil && tool.ResponsesToolFunction.Parameters != nil {
+ funcCopy := *tool.ResponsesToolFunction
+ funcCopy.Parameters = tool.ResponsesToolFunction.Parameters.Normalized()
+ tool.ResponsesToolFunction = &funcCopy
+ }
+ filtered = append(filtered, tool)
+ case schemas.ResponsesToolTypeNamespace:
+ if tool.ResponsesToolNamespace != nil {
+ nsCopy := *tool.ResponsesToolNamespace
+ nsCopy.Tools = normalizeAndFilterTools(nsCopy.Tools)
+ tool.ResponsesToolNamespace = &nsCopy
+ }
+ filtered = append(filtered, tool)
+ // ...existing supported-tool filtering/copy logic...
+ }
+ }
+ return filtered
+}Use the same helper from ToOpenAIResponsesRequest instead of only normalizing/filtering the top-level slice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/openai/responses.go` at line 321, The ResponsesToolNamespace
type is being allowed through without recursing into namespace.Tools, so child
tools (e.g., nested function schemas) skip the top-level normalization/filtering
path; update the Responses handling to iterate over ResponsesToolNamespace.Tools
and run the same normalize/filter helper used by ToOpenAIResponsesRequest
(ensuring Parameters.Normalized() is applied) on each child tool so unsupported
fields/types are stripped and nested function parameters are normalized before
being added to the final map.
|
|
roroghost17
changed the title
| func (m *ResponsesMessage) UnmarshalJSON(data []byte) error { | ||
| if argVal := gjson.GetBytes(data, "arguments"); argVal.Exists() && argVal.Type == gjson.JSON { | ||
| normalized, err := Marshal(argVal.Raw) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data, err = sjson.SetRawBytes(data, "arguments", normalized) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if err := Unmarshal(data, m); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Unmarshal(data, m) where m is *ResponsesMessage will call m.UnmarshalJSON(data) again (both sonic.Unmarshal and json.Unmarshal honour the json.Unmarshaler interface). The call is unconditional, so every unmarshal of a ResponsesMessage will immediately stack-overflow regardless of whether the arguments normalisation branch is taken.
Fix: use a local type alias to break the recursion, as is standard in Go:
func (m *ResponsesMessage) UnmarshalJSON(data []byte) error {
if argVal := gjson.GetBytes(data, "arguments"); argVal.Exists() && argVal.Type == gjson.JSON {
normalized, err := Marshal(argVal.Raw)
if err != nil {
return err
}
data, err = sjson.SetRawBytes(data, "arguments", normalized)
if err != nil {
return err
}
}
type alias ResponsesMessage
return Unmarshal(data, (*alias)(m))
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/responses.go`:
- Around line 575-594: The UnmarshalJSON methods are calling Unmarshal(data,
m/resp) on the same concrete type which triggers recursion; fix by defining a
local alias type (e.g. type responsesMessageAlias ResponsesMessage), unmarshal
into a variable of that alias (var alias responsesMessageAlias; if err :=
Unmarshal(data, &alias); err != nil { return err }), then assign back ( *m =
ResponsesMessage(alias) ); apply the same alias-based pattern to the other
UnmarshalJSON implementation that currently calls Unmarshal(data, resp).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a77300e4-5125-4bc7-a97d-c754dfaae6be
📒 Files selected for processing (1)
core/schemas/responses.go
| // UnmarshalJSON normalises the "arguments" field which OpenAI returns as a plain | ||
| // JSON string for most tool types but as a raw JSON object for some (e.g. | ||
| // tool_search). When an object is encountered it is compact-encoded as a JSON | ||
| // string so all downstream consumers continue to see a *string value. | ||
| func (m *ResponsesMessage) UnmarshalJSON(data []byte) error { | ||
| if argVal := gjson.GetBytes(data, "arguments"); argVal.Exists() && argVal.Type == gjson.JSON { | ||
| normalized, err := Marshal(argVal.Raw) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data, err = sjson.SetRawBytes(data, "arguments", normalized) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if err := Unmarshal(data, m); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Avoid self-recursive UnmarshalJSON calls (stack overflow risk).
Line 590 and Line 2320 call Unmarshal(data, m/resp) on the same type that defines UnmarshalJSON, which can recurse indefinitely. Unmarshal into an alias type, then assign back.
✅ Proposed fix
func (m *ResponsesMessage) UnmarshalJSON(data []byte) error {
if argVal := gjson.GetBytes(data, "arguments"); argVal.Exists() && argVal.Type == gjson.JSON {
normalized, err := Marshal(argVal.Raw)
if err != nil {
return err
}
data, err = sjson.SetRawBytes(data, "arguments", normalized)
if err != nil {
return err
}
}
- if err := Unmarshal(data, m); err != nil {
+ type alias ResponsesMessage
+ var decoded alias
+ if err := Unmarshal(data, &decoded); err != nil {
return err
}
+ *m = ResponsesMessage(decoded)
return nil
} func (resp *BifrostResponsesStreamResponse) UnmarshalJSON(data []byte) error {
if argVal := gjson.GetBytes(data, "arguments"); argVal.Exists() && argVal.Type == gjson.JSON {
normalized, err := Marshal(argVal.Raw)
if err != nil {
return err
}
data, err = sjson.SetRawBytes(data, "arguments", normalized)
if err != nil {
return err
}
}
- if err := Unmarshal(data, resp); err != nil {
+ type alias BifrostResponsesStreamResponse
+ var decoded alias
+ if err := Unmarshal(data, &decoded); err != nil {
return err
}
+ *resp = BifrostResponsesStreamResponse(decoded)
return nil
}Also applies to: 2307-2324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/responses.go` around lines 575 - 594, The UnmarshalJSON methods
are calling Unmarshal(data, m/resp) on the same concrete type which triggers
recursion; fix by defining a local alias type (e.g. type responsesMessageAlias
ResponsesMessage), unmarshal into a variable of that alias (var alias
responsesMessageAlias; if err := Unmarshal(data, &alias); err != nil { return
err }), then assign back ( *m = ResponsesMessage(alias) ); apply the same
alias-based pattern to the other UnmarshalJSON implementation that currently
calls Unmarshal(data, resp).
3 participants
Summary
Adds support for two new Responses API tool types —
tool_searchandnamespace— and extends theweb_searchtool schema withexternal_web_accessandsearch_content_typesfields that were previously dropped during request conversion.Changes
ResponsesToolTypeNamespaceto the allowlist infilterUnsupportedToolsso namespace tools are no longer stripped before being sent to OpenAI.ExternalWebAccess(*bool) andSearchContentTypes([]string) fields toResponsesToolWebSearchand ensured they are copied throughfilterUnsupportedToolsrather than silently discarded.ResponsesToolToolSearchstruct withExecutionandParametersfields, andResponsesToolNamespace(referenced but defined elsewhere) to theResponsesToolunion type.tool_searchandnamespacecases into bothMarshalJSONandUnmarshalJSONonResponsesToolso these tool types round-trip correctly.TestResponsesTool_MarshalUnmarshal_ToolSearchToolto verify marshal/unmarshal correctness fortool_search.TestToOpenAIResponsesRequest_PreservesNamespaceAndWebSearchFieldsto verify thatnamespacetools and the newweb_searchfields survive the full conversion pipeline.Type of change
Affected areas
How to test
go test ./core/providers/openai/... ./core/schemas/...Expected: all tests pass, including the two new test cases covering
tool_searchmarshal/unmarshal and preservation ofnamespacetools and extendedweb_searchfields throughToOpenAIResponsesRequest.Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No new auth, secrets, or PII handling introduced. The
namespacetool type may encapsulate child tools (e.g., MCP-backed functions); no additional sandboxing is required beyond what the upstream API enforces.Checklist
docs/contributing/README.mdand followed the guidelines