fix: bedrock stop reason - #3506
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughStop-reason handling was unified: Bedrock and Anthropic providers now convert known finish reasons via explicit mapping functions, unmapped reasons pass through unchanged, and tests were added/updated to verify forward and reverse mappings and precedence rules. ChangesStop-Reason Conversion Unification
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers:
🚥 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5Safe to merge — all changes are narrowly scoped stop reason normalization fixes with good test coverage on the main paths. The logic is straightforward and correct: the forward and reverse lookup maps are consistent, the previously missing conversion in ToBifrostResponsesResponse is now present, and the Anthropic path is aligned with every other path in the codebase. The one gap is that the streaming path implicitly picks up two additional mappings (stop_sequence and content_filtered) with no dedicated test, but the behavior is correct and the risk of regression is low. core/providers/bedrock/responses.go — specifically the ToBifrostResponsesStream streaming path, which gained two additional stop reason conversions without test coverage. Important Files Changed
Reviews (2): Last reviewed commit: "fix: bedrock stop reason" | Re-trigger Greptile |
The merge-base changed after approval.
f4bd3f6 to
f231f09
Compare
Merge activity
|
## Summary Stop reason normalization was inconsistent across the Bedrock and Anthropic providers. Unknown or unmappable stop reasons (e.g. `guardrail_intervened`) were silently coerced to `"stop"`, and the Responses API path for Bedrock was not applying any stop reason conversion at all. This PR fixes both issues and adds a proper reverse mapping for converting Bifrost stop reasons back to Bedrock format. ## Changes - `guardrail_intervened` no longer maps to `content_filter` — it passes through as-is since there is no clean semantic equivalent in the Bifrost format. - Unknown stop reasons now pass through unchanged instead of defaulting to `"stop"`. - Added `convertBifrostToBedrockStopReason` and a `bifrostToBedrockStopReason` reverse lookup map to support converting Bifrost stop reasons back to Bedrock format. - `BedrockConverseResponse.ToBifrostResponsesResponse` now applies stop reason conversion via `convertBedrockStopReason`, which was previously missing. - `ToBedrockConverseResponse` now prioritizes `StopReason` over `IncompleteDetails` when deriving the Bedrock stop reason. - `AnthropicMessageResponse.ToBifrostResponsesResponse` now uses `ConvertAnthropicFinishReasonToBifrost` instead of passing the raw Anthropic stop reason string through. - Added `TestBedrockStopReasonMappingResponsesPath` and `TestBifrostToBedrockStopReasonReverseMapping` to cover the Responses API path and reverse mapping behavior. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/bedrock/... ./core/providers/anthropic/... ``` Expected: all existing and new tests pass, including `TestBedrockStopReasonMapping`, `TestBedrockStopReasonMappingResponsesPath`, and `TestBifrostToBedrockStopReasonReverseMapping`. ## Breaking changes - [x] Yes - [ ] No `guardrail_intervened` previously mapped to `content_filter` in Bedrock stop reason normalization. It now passes through as `guardrail_intervened`. Consumers relying on the old mapping will need to handle this value explicitly. ## Related issues ## Security considerations None. ## 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 Stop reason normalization was inconsistent across the Bedrock and Anthropic providers. Unknown or unmappable stop reasons (e.g. `guardrail_intervened`) were silently coerced to `"stop"`, and the Responses API path for Bedrock was not applying any stop reason conversion at all. This PR fixes both issues and adds a proper reverse mapping for converting Bifrost stop reasons back to Bedrock format. ## Changes - `guardrail_intervened` no longer maps to `content_filter` — it passes through as-is since there is no clean semantic equivalent in the Bifrost format. - Unknown stop reasons now pass through unchanged instead of defaulting to `"stop"`. - Added `convertBifrostToBedrockStopReason` and a `bifrostToBedrockStopReason` reverse lookup map to support converting Bifrost stop reasons back to Bedrock format. - `BedrockConverseResponse.ToBifrostResponsesResponse` now applies stop reason conversion via `convertBedrockStopReason`, which was previously missing. - `ToBedrockConverseResponse` now prioritizes `StopReason` over `IncompleteDetails` when deriving the Bedrock stop reason. - `AnthropicMessageResponse.ToBifrostResponsesResponse` now uses `ConvertAnthropicFinishReasonToBifrost` instead of passing the raw Anthropic stop reason string through. - Added `TestBedrockStopReasonMappingResponsesPath` and `TestBifrostToBedrockStopReasonReverseMapping` to cover the Responses API path and reverse mapping behavior. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/bedrock/... ./core/providers/anthropic/... ``` Expected: all existing and new tests pass, including `TestBedrockStopReasonMapping`, `TestBedrockStopReasonMappingResponsesPath`, and `TestBifrostToBedrockStopReasonReverseMapping`. ## Breaking changes - [x] Yes - [ ] No `guardrail_intervened` previously mapped to `content_filter` in Bedrock stop reason normalization. It now passes through as `guardrail_intervened`. Consumers relying on the old mapping will need to handle this value explicitly. ## Related issues ## Security considerations None. ## 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
Stop reason normalization was inconsistent across the Bedrock and Anthropic providers. Unknown or unmappable stop reasons (e.g.
guardrail_intervened) were silently coerced to"stop", and the Responses API path for Bedrock was not applying any stop reason conversion at all. This PR fixes both issues and adds a proper reverse mapping for converting Bifrost stop reasons back to Bedrock format.Changes
guardrail_intervenedno longer maps tocontent_filter— it passes through as-is since there is no clean semantic equivalent in the Bifrost format."stop".convertBifrostToBedrockStopReasonand abifrostToBedrockStopReasonreverse lookup map to support converting Bifrost stop reasons back to Bedrock format.BedrockConverseResponse.ToBifrostResponsesResponsenow applies stop reason conversion viaconvertBedrockStopReason, which was previously missing.ToBedrockConverseResponsenow prioritizesStopReasonoverIncompleteDetailswhen deriving the Bedrock stop reason.AnthropicMessageResponse.ToBifrostResponsesResponsenow usesConvertAnthropicFinishReasonToBifrostinstead of passing the raw Anthropic stop reason string through.TestBedrockStopReasonMappingResponsesPathandTestBifrostToBedrockStopReasonReverseMappingto cover the Responses API path and reverse mapping behavior.Type of change
Affected areas
How to test
go test ./core/providers/bedrock/... ./core/providers/anthropic/...Expected: all existing and new tests pass, including
TestBedrockStopReasonMapping,TestBedrockStopReasonMappingResponsesPath, andTestBifrostToBedrockStopReasonReverseMapping.Breaking changes
guardrail_intervenedpreviously mapped tocontent_filterin Bedrock stop reason normalization. It now passes through asguardrail_intervened. Consumers relying on the old mapping will need to handle this value explicitly.Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines