fix: address 3 Anthropic-specific schema-compatibility bugs - #4908
fix: address 3 Anthropic-specific schema-compatibility bugs#4908Shaik-Sirajuddin wants to merge 12 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-call Anthropic web/tool search streaming state, tool_search namespace bridging, OpenAI-native replay conversion, preserved ChangesTool search schemas and bridge
Anthropic streaming and replay
OpenAI forwarding
Estimated code review effort: 5 (Critical) | ~120 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)
⚔️ Resolve merge conflicts
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 |
c2c6aba to
4829f68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/providers/anthropic/websearch_outputitems_test.go (1)
33-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated event-processing loop across all three tests.
Each test duplicates the same "unmarshal →
ToBifrostResponsesStream→ error-check → append" loop (Lines 40-53, 137-150, 243-256). Consider extracting a small helper (e.g.,runAnthropicStreamEvents(t, ctx, state, events []string) []*schemas.BifrostResponsesStreamResponse) shared across this file andtoolsearch_test.goto reduce duplication.♻️ Example helper extraction
func runAnthropicStreamEvents(t *testing.T, ctx *schemas.BifrostContext, state *AnthropicResponsesStreamState, events []string) []*schemas.BifrostResponsesStreamResponse { t.Helper() var emitted []*schemas.BifrostResponsesStreamResponse seq := 0 for _, raw := range events { var chunk AnthropicStreamEvent if err := sonic.Unmarshal([]byte(raw), &chunk); err != nil { t.Fatalf("unmarshal event: %v", err) } responses, bErr, _ := chunk.ToBifrostResponsesStream(ctx, seq, state) if bErr != nil { t.Fatalf("ToBifrostResponsesStream error: %v", bErr) } for _, r := range responses { seq++ emitted = append(emitted, r) } } return emitted }Also applies to: 130-211, 236-274
🤖 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/providers/anthropic/websearch_outputitems_test.go` around lines 33 - 103, The test file repeats the same event-processing loop in multiple tests, making the suite harder to maintain. Extract the shared “unmarshal → ToBifrostResponsesStream → error-check → append” flow into a small helper such as runAnthropicStreamEvents that takes t, ctx, state, and the event list, then reuse it in TestWebSearch_PersistedInOutputItems and the other Anthropic stream tests in this file and toolsearch_test.go.core/providers/anthropic/toolsearch_roundtrip_test.go (1)
218-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse stdlib
slices.Containsinstead of a hand-rolled helper.Go 1.26 (per
go.work) hasslices.Containsin the standard library; this local helper is redundant.♻️ Suggested simplification
-func containsStr(list []string, want string) bool { - for _, v := range list { - if v == want { - return true - } - } - return false -} +// use slices.Contains(names, "get_weather") directly at call sites🤖 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/providers/anthropic/toolsearch_roundtrip_test.go` around lines 218 - 225, Replace the local containsStr helper in toolsearch_roundtrip_test.go with the standard library slices.Contains helper. Update the test code that calls containsStr to use slices.Contains directly, and remove the redundant helper function entirely while keeping the existing test behavior unchanged.
🤖 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 `@core/providers/anthropic/toolsearch_roundtrip_test.go`:
- Around line 218-225: Replace the local containsStr helper in
toolsearch_roundtrip_test.go with the standard library slices.Contains helper.
Update the test code that calls containsStr to use slices.Contains directly, and
remove the redundant helper function entirely while keeping the existing test
behavior unchanged.
In `@core/providers/anthropic/websearch_outputitems_test.go`:
- Around line 33-103: The test file repeats the same event-processing loop in
multiple tests, making the suite harder to maintain. Extract the shared
“unmarshal → ToBifrostResponsesStream → error-check → append” flow into a small
helper such as runAnthropicStreamEvents that takes t, ctx, state, and the event
list, then reuse it in TestWebSearch_PersistedInOutputItems and the other
Anthropic stream tests in this file and toolsearch_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b44e40d-2431-4e59-8909-de8fcfc3b015
📒 Files selected for processing (7)
core/providers/anthropic/reasoningtoolcall_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/toolsearch_roundtrip_test.gocore/providers/anthropic/toolsearch_test.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/websearch_outputitems_test.gocore/schemas/responses.go
Confidence Score: 4/5Safe to merge with awareness of one pre-existing streaming bridge asymmetry not addressed in this PR. The three targeted bug fixes are correct and well-tested. The concurrent accumulation fix (single-slot → per-ID maps) is the most impactful and is implemented correctly with full pool reset coverage. The strict/defer_loading fixes are minimal and accurate. The namespace bridge and cross-provider replay machinery are new but covered by integration tests. One point deducted: the streaming message_stop handler builds response.completed from OutputItems without applying CollapseToolSearchItemToNamespacePair, so streaming callers using the bridge namespace receive raw tool_search_tool_call items rather than the expected function_call/function_call_output pair — the non-streaming path handles this correctly, creating a behavioral asymmetry flagged in a prior review thread. core/providers/anthropic/responses.go — specifically the AnthropicStreamEventTypeMessageStop handler's response.completed construction path when BifrostContextKeyToolSearchBridgeActive is set. Important Files Changed
Reviews (8): Last reviewed commit: "fix: rename rawToolSearch to rawPreserve..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func containsStr(list []string, want string) bool { | ||
| for _, v := range list { | ||
| if v == want { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
containsStr helper is defined only in this file. Both toolsearch_test.go and websearch_outputitems_test.go in the same package already inline similar slice-contains checks, and future test files will either duplicate or import from here. Consider moving it to an existing shared test-helper file (e.g. testutil_test.go or helpers_test.go) so all test files in the package can reference it without redefinition.
| func containsStr(list []string, want string) bool { | |
| for _, v := range list { | |
| if v == want { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // containsStr is a test helper; consider moving to a shared testutil_test.go | |
| // if other files in the package need it to avoid redefinition. | |
| func containsStr(list []string, want string) bool { | |
| for _, v := range list { | |
| if v == want { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Performed a round trip validation againt anthropic platform api @TejasGhatte Could you take a look when you get a chance |
4829f68 to
cdb0cc6
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Hey @Shaik-Sirajuddin is this compatible with both openai and anthropic tool search tool? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/schemas/tool_search_openai_native.go (2)
30-51: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating
argumentsJSONis a JSON object before wrapping asjson.RawMessage.The doc comment states
argumentsJSON must be a JSON object, but nothing enforces this — malformed or non-object input (e.g. a bare string/array) will be silently embedded as-is, producing wire JSON that only fails when the actual OpenAI-compatible backend rejects it. Since this is called with data forwarded from Anthropic's tool-use JSON (seetool_search_anthropic_bridge.go), a cheap sanity check here would surface bad data earlier with a clear error instead of degrading to raw pass-through corruption.🛡️ Proposed validation
func NewOpenAIToolSearchCallItem(callID string, argumentsJSON string) (ResponsesMessage, error) { if argumentsJSON == "" { argumentsJSON = "{}" } + if !json.Valid([]byte(argumentsJSON)) || len(argumentsJSON) == 0 || argumentsJSON[0] != '{' { + return ResponsesMessage{}, fmt.Errorf("tool_search_call arguments must be a JSON object, got: %s", argumentsJSON) + } wire := openAIToolSearchCallWire{🤖 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/tool_search_openai_native.go` around lines 30 - 51, NewOpenAIToolSearchCallItem currently trusts argumentsJSON even though it must be a JSON object. Add a lightweight validation step before building openAIToolSearchCallWire to ensure the input parses as a JSON object (and reject arrays, strings, or malformed JSON) with a clear error. Keep the fix localized in NewOpenAIToolSearchCallItem so callers like the Anthropic bridge get an early, descriptive failure instead of passing invalid raw JSON through.
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: hardcoded
"function"literal instead of a shared constant.
openAIToolSearchFunctionDefWire.Typeand its construction use the raw string"function"rather thanschemas.ResponsesToolTypeFunction(cast to string). Low risk today, but ties this wire encoding to a literal that could drift from the canonical constant.Also applies to: 91-98
🤖 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/tool_search_openai_native.go` around lines 68 - 74, The tool search wire type is using a hardcoded "function" string instead of the shared canonical constant, which can drift from the schema definition. Update openAIToolSearchFunctionDefWire and its construction logic to use schemas.ResponsesToolTypeFunction (cast to string where needed) rather than the raw literal. Apply the same change in the related encoding path noted in the diff so both the struct value and the wire payload stay aligned with the shared constant.
🤖 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/tool_search_namespace_bridge.go`:
- Around line 312-357: CollapseToolSearchItemToNamespacePair is setting the
function_call status to completed even when the tool_search item has no output
yet. Update the call construction in CollapseToolSearchItemToNamespacePair so it
matches the doc comment and the forward path in mergeToolSearchBridgeCall: use
in_progress for calls without Output, and only mark completed when Output is
present (with the paired output message).
---
Nitpick comments:
In `@core/schemas/tool_search_openai_native.go`:
- Around line 30-51: NewOpenAIToolSearchCallItem currently trusts argumentsJSON
even though it must be a JSON object. Add a lightweight validation step before
building openAIToolSearchCallWire to ensure the input parses as a JSON object
(and reject arrays, strings, or malformed JSON) with a clear error. Keep the fix
localized in NewOpenAIToolSearchCallItem so callers like the Anthropic bridge
get an early, descriptive failure instead of passing invalid raw JSON through.
- Around line 68-74: The tool search wire type is using a hardcoded "function"
string instead of the shared canonical constant, which can drift from the schema
definition. Update openAIToolSearchFunctionDefWire and its construction logic to
use schemas.ResponsesToolTypeFunction (cast to string where needed) rather than
the raw literal. Apply the same change in the related encoding path noted in the
diff so both the struct value and the wire payload stay aligned with the shared
constant.
🪄 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: a21903ad-cd13-4765-a4cf-fef5f5f631d8
📒 Files selected for processing (16)
core/providers/anthropic/responses.gocore/providers/anthropic/tool_search_namespace_bridge_integration_test.gocore/providers/anthropic/toolsearch_roundtrip_test.gocore/providers/anthropic/toolsearch_test.gocore/providers/anthropic/types.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/tool_search_anthropic_bridge.gocore/providers/openai/tool_search_anthropic_bridge_test.gocore/providers/openai/tool_search_namespace_bridge_provider_switch_test.gocore/providers/openai/tool_search_native_declaration_test.gocore/providers/openai/types.gocore/schemas/responses.gocore/schemas/tool_search_namespace_bridge.gocore/schemas/tool_search_namespace_bridge_test.gocore/schemas/tool_search_openai_native.go
✅ Files skipped from review due to trivial changes (1)
- core/providers/anthropic/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/responses.go
- core/providers/anthropic/toolsearch_roundtrip_test.go
- core/providers/anthropic/toolsearch_test.go
- core/providers/anthropic/responses.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/anthropic/tool_search_namespace_bridge_integration_test.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename violates the no-underscore convention for Go test files.
tool_search_namespace_bridge_integration_test.gohas underscores in the base name before_test.go. As per coding guidelines,Go filenames use no underscores except for _test.go suffix; use concatenated lowercase words (e.g., pluginpipeline.go, not plugin_pipeline.go). Note this is already inconsistent within the same cohort (toolsearch_test.govs.tool_search_namespace_bridge_integration_test.go).🤖 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/providers/anthropic/tool_search_namespace_bridge_integration_test.go` at line 1, The Go test filename uses underscores in the base name, which violates the no-underscore naming convention. Rename tool_search_namespace_bridge_integration_test.go to a concatenated lowercase name while keeping the _test.go suffix, and update any references or package-level file grouping expectations around the anthropic test files so it matches the naming used by other tests like toolsearch_test.go.Source: Coding guidelines
🤖 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 `@core/providers/anthropic/tool_search_namespace_bridge_integration_test.go`:
- Line 1: The Go test filename uses underscores in the base name, which violates
the no-underscore naming convention. Rename
tool_search_namespace_bridge_integration_test.go to a concatenated lowercase
name while keeping the _test.go suffix, and update any references or
package-level file grouping expectations around the anthropic test files so it
matches the naming used by other tests like toolsearch_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 347cc200-5f45-4610-86c4-0cbc6fea63b1
📒 Files selected for processing (4)
core/providers/anthropic/responses.gocore/providers/anthropic/tool_search_namespace_bridge_integration_test.gocore/providers/openai/responses.gocore/schemas/bifrost.go
✅ Files skipped from review due to trivial changes (1)
- core/schemas/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/providers/openai/responses.go
- core/providers/anthropic/responses.go
|
Addressed issues that was missing handling for OpenAi v1/responses handling with anthropic as provider Anthropic publishes two types of tool_search_tool_regex_* , tool_search_tool_bm25_* , Open AI has one default tool_search_tool , support in its semantics of v1/respones So Drop In Replacement works for both v1/respones with anthropic or openai as backends Additionally , users might want to leverage anthropics both tools , This internally handles using both tools when provider supports ( anthropic in this case ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/providers/openai/toolsearchanthropicbridge.go`:
- Around line 57-60: Handle the error returned by sonic.Unmarshal in the
ResponsesToolCallOutputStr parsing block before constructing the completed
tool-search output. If unmarshalling fails, return the original message through
the same fallback path used by NewOpenAIToolSearchCallItem and
NewOpenAIToolSearchOutputItem failures; only build the completed output when
discoveredNames is parsed successfully.
🪄 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: 93140bae-df26-42c4-8eee-ad1a25534222
📒 Files selected for processing (19)
core/providers/anthropic/responses.gocore/providers/anthropic/toolsearchnamespacebridgeintegration_test.gocore/providers/anthropic/toolsearchroundtrip_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/websearchoutputitems_test.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/toolsearchanthropicbridge.gocore/providers/openai/toolsearchanthropicbridge_test.gocore/providers/openai/toolsearchnamespacebridgeproviderswitch_test.gocore/providers/openai/toolsearchnativedeclaration_test.gocore/providers/openai/types.gocore/schemas/bifrost.gocore/schemas/responses.gocore/schemas/toolsearchnamespacebridge.gocore/schemas/toolsearchnamespacebridge_test.gocore/schemas/toolsearchopenainative.gocore/schemas/toolsearchopenainative_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/bifrost.go
- core/providers/anthropic/utils_test.go
- core/providers/openai/responses.go
- core/providers/anthropic/types.go
- core/providers/openai/responses_marshal_test.go
- core/schemas/responses.go
- core/providers/anthropic/responses.go
…failure CodeRabbit (PR maximhq#4908): sonic.Unmarshal's error was discarded when parsing discovered tool names, so a malformed payload silently produced a "completed, zero results" tool_search_output instead of falling back to the original item like the function's other error paths.
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
- maximhq#4780: tool_search_tool_result was silently dropped on /v1/responses (streaming, non-streaming ingest, and Anthropic egress/replay). Calls are keyed by tool_use ID rather than a single slot, since Claude can emit multiple tool_search calls before any of their results arrive; caller provenance (code-execution-spawned searches) round-trips correctly. - maximhq#3233: tools[].strict is now dropped only for Anthropic-family providers that don't support it (Vertex), and kept for native Anthropic. - maximhq#3802: added a regression test confirming reasoning_content survives assistant tool-call turns with extended thinking (already fixed by maximhq#3584). - web_search_call: same bug class as maximhq#4780 — missing from response.completed and vulnerable to the same multi-call concurrency bug; fixed identically.
… sessions Adds a namespace-based bridge so a caller declaring tool_search via the OpenAI Responses API surface can reach Anthropic's native tool_search_tool_bm25/ _regex sub-tools, and converts completed Anthropic-origin tool_search results into OpenAI's native tool_search_call/tool_search_output shape when a conversation's backend switches mid-session. Also fixes two schema compatibility bugs found while proving this live: tool_search_tool_result's nested content.tool_references shape was modeled flat and silently dropped every discovered tool, and defer_loading was incorrectly stripped as Anthropic-only when it's also required by OpenAI's own tool_search feature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ExpandToolSearchBridgeDeclaration was unconditionally injecting both bm25 and regex Anthropic declarations regardless of which sub-tools were present under the bridge namespace, silently widening a caller's request scope (e.g. a client replaying a previously-collapsed bm25-only namespace would unexpectedly gain regex access). Now only expands the sub-tools actually declared, defaulting to bm25 if none are recognized. Found by an automated codex review pass on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…vider replay Completes the tool_search bridge's response side: a completed tool_search_tool_bm25/_regex call is now collapsed back into the caller-facing namespace-disguised function_call/function_call_output pair (tagged with the reserved bifrost_tool_search_bridge namespace) instead of leaking the internal neutral tool_search_tool_call hub type onto the wire, gated by a new context flag set on ingest when the caller actually declared the bridge namespace. Also normalizes namespace-tagged bridge pairs back to the neutral hub item in the OpenAI request builder, so a bridge pair produced by this new egress collapse can still be correctly converted to OpenAI's native tool_search shape on a subsequent backend switch -- without this, the pair's Anthropic-native call ID reaches a real OpenAI-compatible backend unconverted and gets rejected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three related state-corruption bugs found by an automated codex review pass: - ExpandToolSearchBridgeItems silently dropped a function_call_output with no matching call in the same message slice (e.g. trimmed/paginated history), losing the discovered-tool result. - CollapseToolSearchItemToNamespacePair hardcoded the collapsed function_call's status to "completed" even when the source item was still in_progress, hiding a still-running search from the caller. - convertAnthropicToolSearchCallToOpenAINative always fabricated a completed, empty-result tool_search_output when replaying an Anthropic tool_search call to OpenAI, even if the source call had no Output yet -- corrupting conversation state on a mid-search backend switch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Validate tool_search_call arguments are a JSON object before wrapping as raw bytes, instead of silently passing malformed input through to the wire (coderabbitai). - Use schemas.ResponsesToolTypeFunction instead of a hardcoded "function" string literal in the tool_search_output wire encoding (coderabbitai). - Replace a hand-rolled containsStr test helper with the stdlib slices.Contains, removing the duplication risk instead of just relocating it (greptile). The other two flagged issues (CollapseToolSearchItemToNamespacePair's status handling, and an ID/CallID pointer-sharing concern in responses.go) were already resolved by prior commits on this branch; verified against current code before skipping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Akshay Deo <akshay@akshaydeo.com>
…failure CodeRabbit (PR maximhq#4908): sonic.Unmarshal's error was discarded when parsing discovered tool names, so a malformed payload silently produced a "completed, zero results" tool_search_output instead of falling back to the original item like the function's other error paths.
…, gofmt cleanup Upstream (maximhq#5103, adds addition_tools support) renamed ResponsesMessage's private rawToolSearch field to rawPreserved when generalizing raw-byte preservation beyond tool_search. tool_search_openai_native.go (this branch) predates that rename and still referenced the old name post-rebase.
6cccbc6 to
5672dff
Compare
|
|
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/providers/anthropic/utils_test.go`:
- Around line 3656-3695: Update the test’s terminal-chunk flow around
ToBifrostResponsesStream to invoke HandleAnthropicResponsesStream, or a shared
finalization helper used by that production handler, instead of manually
accumulating usage and reapplying served modifiers. Preserve assertions for the
completed response while ensuring the test exercises the real production wiring.
In `@core/providers/openai/tool_search_namespace_bridge_provider_switch_test.go`:
- Around line 34-46: Replace the hand-built namespace entry in the test’s Tools
declaration with the canonical result from
schemas.BuildToolSearchBridgeNamespaceDeclaration(), preserving the separate
get_weather tool. Remove the manually constructed bridge namespace and grouped
functions so required descriptions are included in the serialized OpenAI shape.
In `@core/schemas/tool_search_namespace_bridge.go`:
- Around line 169-213: Update CollapseToolSearchDeclarationsToBridgeNamespace so
the synthesized bridge namespace is inserted at the position of the first
removed tool_search declaration rather than appended after all remaining tools.
Track that insertion index while iterating, preserve the relative order of
non-tool_search entries, and retain the existing unchanged return when no
tool_search entries are present.
🪄 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: 591a1f02-d1ce-4616-9f23-97c03d0f9bab
📒 Files selected for processing (23)
core/providers/anthropic/passthrough_usage_test.gocore/providers/anthropic/reasoningtoolcall_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/tool_search_namespace_bridge_integration_test.gocore/providers/anthropic/toolsearch_roundtrip_test.gocore/providers/anthropic/toolsearch_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/validatechattools_test.gocore/providers/anthropic/websearch_outputitems_test.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/tool_search_anthropic_bridge.gocore/providers/openai/tool_search_anthropic_bridge_test.gocore/providers/openai/tool_search_namespace_bridge_provider_switch_test.gocore/providers/openai/tool_search_native_declaration_test.gocore/providers/openai/types.gocore/schemas/bifrost.gocore/schemas/responses.gocore/schemas/tool_search_namespace_bridge.gocore/schemas/tool_search_namespace_bridge_test.gocore/schemas/tool_search_openai_native.gocore/schemas/tool_search_openai_native_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/bifrost.go
- core/providers/anthropic/reasoningtoolcall_test.go
- core/providers/anthropic/types.go
- core/providers/openai/types.go
- core/providers/openai/responses_marshal_test.go
- core/providers/openai/responses.go
- core/providers/anthropic/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
🤖 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/providers/anthropic/utils_test.go`:
- Around line 3656-3695: Update the test’s terminal-chunk flow around
ToBifrostResponsesStream to invoke HandleAnthropicResponsesStream, or a shared
finalization helper used by that production handler, instead of manually
accumulating usage and reapplying served modifiers. Preserve assertions for the
completed response while ensuring the test exercises the real production wiring.
In `@core/providers/openai/tool_search_namespace_bridge_provider_switch_test.go`:
- Around line 34-46: Replace the hand-built namespace entry in the test’s Tools
declaration with the canonical result from
schemas.BuildToolSearchBridgeNamespaceDeclaration(), preserving the separate
get_weather tool. Remove the manually constructed bridge namespace and grouped
functions so required descriptions are included in the serialized OpenAI shape.
In `@core/schemas/tool_search_namespace_bridge.go`:
- Around line 169-213: Update CollapseToolSearchDeclarationsToBridgeNamespace so
the synthesized bridge namespace is inserted at the position of the first
removed tool_search declaration rather than appended after all remaining tools.
Track that insertion index while iterating, preserve the relative order of
non-tool_search entries, and retain the existing unchanged return when no
tool_search entries are present.
🪄 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: 591a1f02-d1ce-4616-9f23-97c03d0f9bab
📒 Files selected for processing (23)
core/providers/anthropic/passthrough_usage_test.gocore/providers/anthropic/reasoningtoolcall_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/tool_search_namespace_bridge_integration_test.gocore/providers/anthropic/toolsearch_roundtrip_test.gocore/providers/anthropic/toolsearch_test.gocore/providers/anthropic/types.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/validatechattools_test.gocore/providers/anthropic/websearch_outputitems_test.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/tool_search_anthropic_bridge.gocore/providers/openai/tool_search_anthropic_bridge_test.gocore/providers/openai/tool_search_namespace_bridge_provider_switch_test.gocore/providers/openai/tool_search_native_declaration_test.gocore/providers/openai/types.gocore/schemas/bifrost.gocore/schemas/responses.gocore/schemas/tool_search_namespace_bridge.gocore/schemas/tool_search_namespace_bridge_test.gocore/schemas/tool_search_openai_native.gocore/schemas/tool_search_openai_native_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/bifrost.go
- core/providers/anthropic/reasoningtoolcall_test.go
- core/providers/anthropic/types.go
- core/providers/openai/types.go
- core/providers/openai/responses_marshal_test.go
- core/providers/openai/responses.go
- core/providers/anthropic/responses.go
🛑 Comments failed to post (3)
core/providers/anthropic/utils_test.go (1)
3656-3695: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise the real terminal-chunk wiring instead of reproducing it.
The test manually implements accumulation and reapplication, so a regression in
HandleAnthropicResponsesStreamwould still pass. Invoke the handler or extract its finalization into a shared helper used by production and this test.🤖 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/providers/anthropic/utils_test.go` around lines 3656 - 3695, Update the test’s terminal-chunk flow around ToBifrostResponsesStream to invoke HandleAnthropicResponsesStream, or a shared finalization helper used by that production handler, instead of manually accumulating usage and reapplying served modifiers. Preserve assertions for the completed response while ensuring the test exercises the real production wiring.core/providers/openai/tool_search_namespace_bridge_provider_switch_test.go (1)
34-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C4 \ 'BuildToolSearchBridgeNamespaceDeclaration|ToolSearchBridgeNamespaceDescription|ToolSearchBridgeFuncBM25Description' \ core/schemas core/providers/openaiRepository: maximhq/bifrost
Length of output: 5983
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' core/providers/openai/tool_search_namespace_bridge_provider_switch_test.go printf '\n---\n' sed -n '1,260p' core/schemas/tool_search_namespace_bridge.goRepository: maximhq/bifrost
Length of output: 14788
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "func ToOpenAIResponsesRequest|filterUnsupportedTools|BuildToolSearchBridgeNamespaceDeclaration|ExpandToolSearchBridgeDeclaration|CollapseToolSearchDeclarationsToBridgeNamespace" core/providers/openai core/schemasRepository: maximhq/bifrost
Length of output: 4951
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,140p' core/providers/openai/responses.go printf '\n---\n' sed -n '340,430p' core/providers/openai/responses.go printf '\n---\n' sed -n '1,240p' core/providers/openai/tool_search_namespace_bridge_provider_switch_test.goRepository: maximhq/bifrost
Length of output: 13198
Use the canonical bridge declaration here.
This hand-built namespace skips the required description fields on the namespace and grouped functions, so the test can pass while still serializing an invalid OpenAI wire shape. Useschemas.BuildToolSearchBridgeNamespaceDeclaration()instead.🤖 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/providers/openai/tool_search_namespace_bridge_provider_switch_test.go` around lines 34 - 46, Replace the hand-built namespace entry in the test’s Tools declaration with the canonical result from schemas.BuildToolSearchBridgeNamespaceDeclaration(), preserving the separate get_weather tool. Remove the manually constructed bridge namespace and grouped functions so required descriptions are included in the serialized OpenAI shape.core/schemas/tool_search_namespace_bridge.go (1)
169-213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the original tool-search declaration position.
All
tool_searchentries are removed and the collapsed namespace is appended at the end. Thus[tool_search, function]becomes[function, namespace], breaking round-trip fidelity and changing the serialized tool order. Insert the namespace where the first removed declaration appeared.Proposed fix
func CollapseToolSearchDeclarationsToBridgeNamespace(tools []ResponsesTool) []ResponsesTool { var sawBM25, sawRegex bool out := make([]ResponsesTool, 0, len(tools)) + bridgeIndex := -1 for _, t := range tools { if t.Type == ResponsesToolTypeToolSearch { + if bridgeIndex == -1 { + bridgeIndex = len(out) + } if t.Name != nil && bridgeFuncIsRegex(*t.Name) { sawRegex = true } else { sawBM25 = true @@ } canonical.ResponsesToolNamespace = &ResponsesToolNamespace{Tools: grouped} - out = append(out, canonical) + out = append(out, ResponsesTool{}) + copy(out[bridgeIndex+1:], out[bridgeIndex:]) + out[bridgeIndex] = canonical return out }📝 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.// CollapseToolSearchDeclarationsToBridgeNamespace is the reverse of // ExpandToolSearchBridgeDeclaration: given a tools[] list containing the // neutral tool_search declaration(s), render them back into the single // caller-facing namespace declaration. Used when Bifrost serializes a // tools[] list back out to an OpenAI-Responses-shaped caller whose backend is // not itself OpenAI (e.g. Anthropic). Tools not of type tool_search pass // through unchanged; if no tool_search entries are present, returns the input // unchanged. func CollapseToolSearchDeclarationsToBridgeNamespace(tools []ResponsesTool) []ResponsesTool { var sawBM25, sawRegex bool out := make([]ResponsesTool, 0, len(tools)) bridgeIndex := -1 for _, t := range tools { if t.Type == ResponsesToolTypeToolSearch { if bridgeIndex == -1 { bridgeIndex = len(out) } if t.Name != nil && bridgeFuncIsRegex(*t.Name) { sawRegex = true } else { sawBM25 = true } continue } out = append(out, t) } if !sawBM25 && !sawRegex { return tools } // Build off the canonical, spec-complete declaration (both // description fields and each sub-tool's "type":"function" present — // see BuildToolSearchBridgeNamespaceDeclaration's doc comment for the // two live 400s a hand-rolled, incomplete shape triggers), then drop // whichever sub-tool wasn't actually seen. canonical := BuildToolSearchBridgeNamespaceDeclaration() grouped := make([]ResponsesTool, 0, 2) for _, sub := range canonical.ResponsesToolNamespace.Tools { if sub.Name == nil { continue } if (*sub.Name == ToolSearchBridgeFuncBM25 && sawBM25) || (*sub.Name == ToolSearchBridgeFuncRegex && sawRegex) { grouped = append(grouped, sub) } } canonical.ResponsesToolNamespace = &ResponsesToolNamespace{Tools: grouped} out = append(out, ResponsesTool{}) copy(out[bridgeIndex+1:], out[bridgeIndex:]) out[bridgeIndex] = canonical return out }🤖 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/tool_search_namespace_bridge.go` around lines 169 - 213, Update CollapseToolSearchDeclarationsToBridgeNamespace so the synthesized bridge namespace is inserted at the position of the first removed tool_search declaration rather than appended after all remaining tools. Track that insertion index while iterating, preserve the relative order of non-tool_search entries, and retain the existing unchanged return when no tool_search entries are present.
|
Thanks for the fix! We opened #4780 and the initial fix #4786, and it's great to see the response-side handling carried further here. 🙏 While verifying #4908 against the OpenAI /v1/responses path (the native /anthropic/v1/messages already works), we found a complementary request-side gap: Filed as #5279, with a small standalone fix in #5280 (one case in Happy to land it separately or fold it into #4908 — whatever's easiest for you. Thanks again for the work here! |
44564de to
493bff0
Compare
244a01d to
ce1b2a6
Compare
6 participants
Closes #4780
Closes #3233
Closes #3802
Summary
tool_search_tool_resultwas dropped on/v1/responses(streaming, non-streaming, and Anthropic egress/replay).tools[].strictnow dropped only where Anthropic-family providers don't support it (Vertex), kept for native Anthropic.reasoning_contentsurvives tool-call turns with extended thinking (already fixed by fix: fixes forwarding of reasoning content while conversion of Responses to Chat #3584).web_search_call— same class as [Bug]: Anthropic server-side tool_search results are dropped on /v1/responses (advertised as supported, but not wired up) #4780: missing fromresponse.completed, plus a concurrency bug dropping all but the last call in a multi-search turn.Test plan
core/providers/anthropic, full suite passing (one pre-existing unrelated failure confirmed on baseline:TestResponsesMessageToolCallArguments/real_tool_search_call_frames_from_openai)tool_search, concurrentweb_searchcalls,strictforwarding