fix: minor claude code fixes - #4932
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughAdds an explicit passthrough mode flag to the Anthropic reverse-stream conversion state, gating content-block index allocation for web_search/web_fetch completions so indices stay aligned with raw upstream frames during passthrough and remain contiguous otherwise. Refactors tool-type matching to prefix-based recognition for versioned server tools, wires the flag from the HTTP transport, and adds regression tests. ChangesAnthropic passthrough index alignment and tool version recognition
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/anthropic/websearch_test.go (1)
296-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
Name: "web_fetch"reused for web_search test cases.Every case constructs the tool with
Name: "web_fetch", even theweb_search_*rows. Doesn't affect correctness sinceconvertAnthropicToolToBifrostdispatches onTypefirst, but it's confusing when reading/debugging a failing web_search case.🧹 Suggested tweak
- in := &AnthropicTool{Type: schemas.Ptr(AnthropicToolType(c.toolType)), Name: "web_fetch"} + in := &AnthropicTool{Type: schemas.Ptr(AnthropicToolType(c.toolType)), Name: c.toolType}🤖 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_test.go` around lines 296 - 298, The test setup in convertAnthropicToolToBifrost uses the same tool name for both web_fetch and web_search cases, which makes the web_search scenarios harder to read and debug. Update the AnthropicTool construction in websearch_test.go so the name matches the case being exercised, especially in the web_search rows, while keeping the existing Type-based dispatch unchanged. Use the convertAnthropicToolToBifrost test table and the t.Run(c.toolType, ...) setup to locate the shared test initializer.
🤖 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/websearch_test.go`:
- Around line 296-298: The test setup in convertAnthropicToolToBifrost uses the
same tool name for both web_fetch and web_search cases, which makes the
web_search scenarios harder to read and debug. Update the AnthropicTool
construction in websearch_test.go so the name matches the case being exercised,
especially in the web_search rows, while keeping the existing Type-based
dispatch unchanged. Use the convertAnthropicToolToBifrost test table and the
t.Run(c.toolType, ...) setup to locate the shared test initializer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b144d7e1-381e-49cd-8ba8-17e221da85f0
📒 Files selected for processing (4)
core/providers/anthropic/passthroughstream_test.gocore/providers/anthropic/responses.gocore/providers/anthropic/websearch_test.gotransports/bifrost-http/integrations/anthropic.go
Merge activity
|
## Summary Adds end-to-end test coverage for a set of provider egress streaming and truncation correctness bugs. The new tests assert that Bedrock `converse-stream` properly closes content blocks before terminating, that Anthropic normalized and Claude Code passthrough streams emit contiguous `content_block_start` indices starting from 0, and that Bedrock Responses API truncated responses correctly signal `status=incomplete` with `reason=max_output_tokens` in both streaming and non-streaming modes. ## Changes - Added a `contentBlockStop`-before-`messageStop` assertion to the existing Bedrock `converse-stream` basic test to catch #4923. - Added a `content_block_start` index contiguity check to the existing Anthropic normalized streaming test to catch gaps introduced by server-tool rewrites (#4890 / #4932). - Added a new **section 17 – Provider Egress Streaming/Truncation Guards** with four requests: - Bedrock forced-tool `converse-stream` verifies `toolUse`, `contentBlockStop`, and `messageStop` ordering (#4923). - Anthropic normalized `web_fetch` streaming verifies contiguous `content_block_start` indices (#4932). - Bedrock Responses non-streaming truncation verifies `status=incomplete` and `incomplete_details.reason=max_output_tokens` (#4680). - Bedrock Responses streaming truncation verifies `response.incomplete` is emitted and `response.completed` is absent (#4680). - Added a new **section 18 – Claude Code Passthrough server-tool streaming index contiguity** with three requests covering `web_search` (normal results), `web_search` (zero results), and `web_fetch` via the `claude-cli` User-Agent passthrough path (#4890). ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the Postman/Newman collection against a live Bifrost instance: ```sh newman run tests/e2e/api/collections/provider-harness.json \ --env-var baseUrl=<BIFROST_URL> \ --env-var bedrockModel=<BEDROCK_MODEL_ID> \ --env-var anthropicKey=<ANTHROPIC_API_KEY> ``` All tests in sections 17 and 18 should pass. Specifically: - Bedrock `converse-stream` responses must contain `contentBlockStop` before `messageStop`. - All Anthropic streaming responses must have `content_block_start` indices `[0, 1, 2, …]` with no gaps. - Bedrock Responses truncated (non-streaming) must return `status=incomplete` with `incomplete_details.reason=max_output_tokens`. - Bedrock Responses truncated (streaming) must emit `response.incomplete` and must **not** emit `response.completed`. ## Breaking changes - [x] No ## Related issues Closes #4923, #4932, #4890, #4680 ## Security considerations None. These are read-only test assertions against existing API endpoints; no new credentials or secrets are introduced beyond those already required by the collection. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Fixes two bugs in the Anthropic reverse-stream converter that caused incorrect
content_blockindex numbering depending on the request path:Passthrough path (Claude Code): The converter was unconditionally consuming (and discarding) a content-block index for dropped server-tool result blocks (
web_fetch, zero-resultweb_search). This is correct only on the passthrough path, where verbatim raw upstream frames are interleaved and indices must stay in lockstep. On the all-normalized path, this caused a gap in indices (e.g.[0, 1, 3]instead of[0, 1, 2]), which strict Anthropic SDK clients reject as a missing block.Tool version recognition:
convertAnthropicToolToBifrostmatchedweb_searchandweb_fetchtool types by exact version string. Any future-dated version (e.g.web_fetch_20260318) silently fell through to the client-function default instead of being recognized as a server tool.Changes
passthrough boolfield onanthropicToResponsesStreamStateand aSetResponsesStreamPassthrough(ctx)function. The transport calls this whenshouldUsePassthroughis true; the converter reads it to decide whether to consume the discarded result-block index.allocBlockIndex("")calls forweb_fetchand resultlessweb_searchare now gated onstate.passthrough, keeping indices contiguous on the all-normalized path.convertAnthropicToolToBifrostnow matchesweb_search_*andweb_fetch_*tool types by prefix (viastrings.HasPrefix) in a separateswitch typeStr := string(*tool.Type); { case ... }block before the exact-match switch, so any current or future dated version is correctly recognized.runAnthropicPassthrough) now callsSetResponsesStreamPassthroughto mirror what the transport does, keeping existing passthrough tests accurate.TestAnthropicConverterOnly_IndicesContiguousto assert that the all-normalized path emits contiguouscontent_block_startindices0, 1, 2, …acrossweb_fetch, zero-resultweb_search, and combinations thereof.TestServerSearchTools_VersionRecognitionto assert that known and future-datedweb_search_*/web_fetch_*tool types all map to the correct neutral server-tool type and never fall through toResponsesToolTypeFunction.Type of change
Affected areas
How to test
TestAnthropicConverterOnly_IndicesContiguouswill fail before the fix with output like:TestServerSearchTools_VersionRecognitionwill fail before the fix forweb_search_20260318andweb_fetch_20260318with:Breaking changes
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines