fix: signal Bedrock max_output_tokens truncation on Responses API - #4680
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBedrock Responses API now marks ChangesBedrock truncation signaling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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/bedrock/responses.go`:
- Around line 1513-1524: The streaming path in responses.go is incorrectly
defaulting every non-length stop reason to response.Status="completed", which
causes unmapped Bedrock reasons like content_filter and guardrail_intervened to
diverge from ToBifrostResponsesResponse. Update the terminal handling around the
Responses stream response mapping so only the explicit length truncation case
sets status to "incomplete", and the completed status is assigned only for known
completed cases; leave Status unset for unmapped stop reasons so streaming and
non-streaming behavior stay aligned.
🪄 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: 1b0bf952-5100-4874-875e-af6e0d7f6529
📒 Files selected for processing (3)
core/changelog.mdcore/providers/bedrock/bedrock_test.gocore/providers/bedrock/responses.go
a391acb to
8054779
Compare
| switch *response.StopReason { | ||
| case string(schemas.BifrostFinishReasonLength): | ||
| terminalEventType = schemas.ResponsesStreamResponseTypeIncomplete | ||
| response.Status = schemas.Ptr("incomplete") |
There was a problem hiding this comment.
can we move them to constants for better reusability?
There was a problem hiding this comment.
Done in 8918d56 — moved them into schemas constants (ResponsesResponseStatusCompleted / ResponsesResponseStatusIncomplete and ResponsesResponseIncompleteReasonMaxOutputTokens, plus the sibling status/reason values for reuse) and used them on both the streaming and non-streaming Bedrock Responses paths.
81e9a26 to
23b134c
Compare
The merge-base changed after approval.
|
Hi @akshaydeo it looks like the most recent merges have caused some conflicts. Ok if I rebase? |
When a /v1/responses request to a Bedrock model is truncated by max_output_tokens, the OpenAI-canonical truncation signals were dropped: response.status and response.incomplete_details were left unset, and the terminal streaming event was response.completed. Consumers on the OpenAI Responses-API contract could not detect Bedrock-side truncation. Set Status="incomplete" + IncompleteDetails.Reason="max_output_tokens" on the non-streaming path, and emit response.incomplete on the streaming terminal event, when the mapped stop reason is "length". Unmapped reasons (content_filter, guardrail_intervened, ...) leave Status unset on both paths. Affected packages: - core/providers/bedrock/ - Status/IncompleteDetails + terminal event type - core/schemas/ - Responses status + incomplete-reason constants Closes maximhq#4679
23b134c to
eb48fd6
Compare
…ximhq#4680) When a /v1/responses request to a Bedrock model is truncated by max_output_tokens, the OpenAI-canonical truncation signals were dropped: response.status and response.incomplete_details were left unset, and the terminal streaming event was response.completed. Consumers on the OpenAI Responses-API contract could not detect Bedrock-side truncation. Set Status="incomplete" + IncompleteDetails.Reason="max_output_tokens" on the non-streaming path, and emit response.incomplete on the streaming terminal event, when the mapped stop reason is "length". Unmapped reasons (content_filter, guardrail_intervened, ...) leave Status unset on both paths. Affected packages: - core/providers/bedrock/ - Status/IncompleteDetails + terminal event type - core/schemas/ - Responses status + incomplete-reason constants Closes maximhq#4679
) When a /v1/responses request to a Bedrock model is truncated by max_output_tokens, the OpenAI-canonical truncation signals were dropped: response.status and response.incomplete_details were left unset, and the terminal streaming event was response.completed. Consumers on the OpenAI Responses-API contract could not detect Bedrock-side truncation. Set Status="incomplete" + IncompleteDetails.Reason="max_output_tokens" on the non-streaming path, and emit response.incomplete on the streaming terminal event, when the mapped stop reason is "length". Unmapped reasons (content_filter, guardrail_intervened, ...) leave Status unset on both paths. Affected packages: - core/providers/bedrock/ - Status/IncompleteDetails + terminal event type - core/schemas/ - Responses status + incomplete-reason constants Closes #4679
## 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
When a
/v1/responsesrequest to a Bedrock model is truncated bymax_output_tokens, Bifrost drops the OpenAI-canonical truncation signals on the wire response:response.statusis left unset (OpenAI sets"incomplete")response.incomplete_detailsis left unset (OpenAI sets{"reason": "max_output_tokens"})response.completed(OpenAI emitsresponse.incomplete)Consumers on the OpenAI Responses-API contract therefore cannot detect Bedrock-side truncation. This is especially damaging for streaming tool calls, where the truncated bytes parse as valid-but-incomplete JSON (required fields silently missing or
{}), so a downstream parser cannot fall back on a parse error.Changes
Two narrow edits in
core/providers/bedrock/responses.go:BedrockConverseResponse.ToBifrostResponsesResponse): after the existingbifrostResp.StopReason = &stopReason, setStatus/IncompleteDetailsfrom the mapped stop reason ("length"→incomplete+max_output_tokens; clean reasons →completed). Mirrorscore/schemas/mux.go::responsesStatusFromChatFinishReason.FinalizeBedrockStream): the terminal chunk's hard-codedResponsesStreamResponseTypeCompletedbecomes conditional — when the mapped stop reason is"length", emitResponsesStreamResponseTypeIncompleteand set the same fields on the embeddedResponse.Value flow: Bedrock
stopReason "max_tokens"→ Bifrost finish reason"length"→ OpenAIstatus "incomplete"+incomplete_details.reason "max_output_tokens". No new code paths or API surface;status/incomplete_detailsare existing optional fields.Changelog entry added to
core/changelog.md.Type of change
Affected areas
How to test
Covered by
TestBedrockStopReasonMappingResponsesPath(extended withStatus/IncompleteDetailsper stop reason),TestFinalizeBedrockStream_MaxTokensTruncation, andTestFinalizeBedrockStream_CleanCompletionUnaffected. AWS reportsstopReason: "max_tokens"for the same input (aws bedrock-runtime converse-stream); this brings the Bifrost translation in line with that signal.Breaking changes
statusandincomplete_detailsare existing optional fields. The streaming terminal event type changes fromresponse.completedtoresponse.incompleteonly when Bedrock reportsmax_tokens— which is the bug fix (consumers now see the OpenAI-canonical type).Related issues
Closes #4679
Security considerations
None — no auth, secret, or PII surface changed.
Checklist