fix(compat): restore namespace on function_call items after flattening#5065
Open
tcx4c70 wants to merge 1 commit into
Open
fix(compat): restore namespace on function_call items after flattening#5065tcx4c70 wants to merge 1 commit into
tcx4c70 wants to merge 1 commit into
Conversation
flattenNamespaceTools expands namespace-scoped Responses tools into a flat list for providers that don't support the "namespace" tool type, but it dropped the namespace name. Providers then return function_call items with only a name and no namespace, so namespace-tool clients (e.g. Codex) that match tools by (namespace, name) fail with "unsupported tool <name>". Record a tool-name -> namespace mapping while flattening (stored per-request on BifrostContext), and re-attach the namespace to function_call items in PostLLMHook for both non-streaming (ResponsesResponse.Output) and streaming (ResponsesStreamResponse.Item and Response.Output) responses. A provider-supplied namespace is never overwritten, and OpenAI/Azure-OpenAI paths are unaffected since they skip flattening.
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds namespace-tool flattening with mapping capture in ChangesNamespace flattening and restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PreLLMHook
participant applyParameterConversion
participant flattenNamespaceTools
participant BifrostContext
participant PostLLMHook
participant restoreNamespaceOnResponse
PreLLMHook->>applyParameterConversion: applyParameterConversion(ctx, req)
applyParameterConversion->>flattenNamespaceTools: flattenNamespaceTools(req)
flattenNamespaceTools-->>applyParameterConversion: namespaceByTool map
applyParameterConversion->>BifrostContext: store namespaceToolMap
PostLLMHook->>BifrostContext: read namespaceToolMap
PostLLMHook->>restoreNamespaceOnResponse: restoreNamespaceOnResponse(result, namespaceByTool)
restoreNamespaceOnResponse-->>PostLLMHook: response with restored Namespace
🚥 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 |
Contributor
Confidence Score: 4/5The namespace restore path needs a fix for duplicate inner tool names before merging.
plugins/compat/conversion.go Important Files Changed
Reviews (1): Last reviewed commit: "fix(compat): restore namespace on functi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if namespaceByTool == nil { | ||
| namespaceByTool = make(map[string]string) | ||
| } | ||
| namespaceByTool[*inner.Name] = namespace |
Contributor
There was a problem hiding this comment.
When two namespace tools expose the same inner function name, this map keeps only the last namespace for that name. A non-OpenAI provider then returns function_call.name without the namespace, so restoring by name alone can label ns_a.search as ns_b.search, causing the client to execute the wrong namespaced tool or reject the call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/compat/namespacetools_test.go (1)
74-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the Azure skip path.
flattenNamespaceToolshas a distinct Azure branch (req.Provider == schemas.Azure && !schemas.IsAnthropicModel(req.Model)) that is not exercised. The OpenAI skip is tested, but the Azure condition involves an additionalIsAnthropicModelcheck — both the skip case (Azure + non-Anthropic model) and the proceed case (Azure + Anthropic model) are untested.🧪 Suggested Azure skip test
func TestFlattenNamespaceTools_SkipsOpenAI(t *testing.T) { // ... existing code ... } +func TestFlattenNamespaceTools_SkipsAzureNonAnthropic(t *testing.T) { + req := &schemas.BifrostResponsesRequest{ + Provider: schemas.Azure, + Model: "gpt-5.4", + Params: &schemas.ResponsesParameters{ + Tools: []schemas.ResponsesTool{namespaceTool("mcp__node_repl__", "js")}, + }, + } + + nsMap := flattenNamespaceTools(req) + + if nsMap != nil { + t.Errorf("expected nil map for Azure non-Anthropic, got %v", nsMap) + } + if len(req.Params.Tools) != 1 || req.Params.Tools[0].Type != schemas.ResponsesToolTypeNamespace { + t.Errorf("Azure non-Anthropic namespace tool should be left intact, got %+v", req.Params.Tools) + } +} + +func TestFlattenNamespaceTools_ProceedsForAzureAnthropicModel(t *testing.T) { + req := &schemas.BifrostResponsesRequest{ + Provider: schemas.Azure, + Model: "claude-sonnet-4", + Params: &schemas.ResponsesParameters{ + Tools: []schemas.ResponsesTool{namespaceTool("mcp__node_repl__", "js")}, + }, + } + + nsMap := flattenNamespaceTools(req) + + if nsMap == nil || nsMap["js"] != "mcp__node_repl__" { + t.Fatalf("expected namespace map for Azure Anthropic model, got %v", nsMap) + } + if got := len(req.Params.Tools); got != 1 || req.Params.Tools[0].Type != schemas.ResponsesToolTypeFunction { + t.Fatalf("expected flattened function tool for Azure Anthropic, got %+v", req.Params.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 `@plugins/compat/namespacetools_test.go` around lines 74 - 91, Add coverage for the Azure-specific branch in flattenNamespaceTools. Create tests that exercise the req.Provider == schemas.Azure path with a non-Anthropic model to confirm namespace tools are skipped and left unchanged, and another case with an Anthropic model to confirm the function proceeds as expected. Use flattenNamespaceTools, schemas.Azure, and schemas.IsAnthropicModel in the new test names/fixtures so the Azure behavior is explicitly covered alongside the existing OpenAI test.
🤖 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 `@plugins/compat/namespacetools_test.go`:
- Around line 74-91: Add coverage for the Azure-specific branch in
flattenNamespaceTools. Create tests that exercise the req.Provider ==
schemas.Azure path with a non-Anthropic model to confirm namespace tools are
skipped and left unchanged, and another case with an Anthropic model to confirm
the function proceeds as expected. Use flattenNamespaceTools, schemas.Azure, and
schemas.IsAnthropicModel in the new test names/fixtures so the Azure behavior is
explicitly covered alongside the existing OpenAI test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ef5d1d6-1d33-42b0-ae7f-302186fd098e
📒 Files selected for processing (3)
plugins/compat/conversion.goplugins/compat/main.goplugins/compat/namespacetools_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flattenNamespaceTools expands namespace-scoped Responses tools into a flat list for providers that don't support the "namespace" tool type, but it dropped the namespace name. Providers then return function_call items with only a name and no namespace, so namespace-tool clients (e.g. Codex) that match tools by (namespace, name) fail with "unsupported tool ".
Changes
Record a tool-name -> namespace mapping while flattening (stored per-request on BifrostContext), and re-attach the namespace to function_call items in PostLLMHook for both non-streaming (ResponsesResponse.Output) and streaming (ResponsesStreamResponse.Item and Response.Output) responses. A provider-supplied namespace is never overwritten, and OpenAI/Azure-OpenAI paths are unaffected since they skip flattening.
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines