fix(bedrock): developer-role, empty-content, and streaming terminal events#3188
fix(bedrock): developer-role, empty-content, and streaming terminal events#3188amarcin wants to merge 3 commits into
Conversation
OpenAI's Chat Completions and Responses APIs permit role="developer" as a system-message variant (introduced for GPT-5 / reasoning models). Bedrock Converse rejects anything other than user/assistant in the messages array, so passing "developer" through verbatim produces: Value 'developer' at 'messages.N.member.role' failed to satisfy constraint: Member must satisfy enum value set: [user, assistant] Fold "developer" into the system-message branch in both code paths so the message text lands in Converse's system field instead. This matches what normalizeDeveloperRoleForChatFallback already does for the responses-to-chat fallback path and what the Gemini provider does in core/providers/gemini/responses.go:2962. Fixes maximhq#2492
|
|
📝 WalkthroughWalkthroughStreaming response conversion now buffers assistant text per output index and emits finalized text only at stream finalization; message conversion treats role "developer" as "system" and drops converted Bedrock messages with empty content blocks to avoid sending blank turns. ChangesStreaming response buffering & finalization
Role folding and empty-content guard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Confidence Score: 5/5Safe to merge; the core developer-role fix is correct and the additional streaming improvements are well-guarded. Only P2 findings present. The developer-role mapping is straightforward and consistent with the Gemini provider and mux fallback. Nil returns from convertBifrostMessageToBedrockMessage are properly guarded at the call site. The incomplete tool-call population in response.completed is a pre-existing limitation explicitly acknowledged in comments, not a regression. core/providers/bedrock/responses.go — the response.completed Output loop is text-only; tool-call items remain absent from that event. Important Files Changed
Reviews (3): Last reviewed commit: "fix(bedrock): accumulate streamed text f..." | Re-trigger Greptile |
…nses Codex CLI emits placeholder assistant messages with content=[] between tool-call turns, and historic transcripts may contain user/assistant messages whose content consisted entirely of reasoning/metadata blocks that this converter does not map to Bedrock. In either case, the current code produces a BedrockMessage with empty/null Content, which Bedrock Converse rejects: Value null at 'messages.N.member.content' failed to satisfy constraint: Member must not be null Skip such messages in both the chat completions and responses translators so upstream callers receive a coherent message sequence. Part of maximhq#2492
|
Added a second commit for a related defect surfaced by Codex CLI & Desktop 0.128: Codex emits placeholder assistant messages with The new commit skips user/assistant turns whose converted content is empty, in both Verified live on a Bifrost v1.4.24 container patched with both commits. |
Streaming responses emitted via Bedrock were sending correct per-token response.output_text.delta events but dropping the accumulated text on the terminal events: - response.output_text.done → text="" - response.content_part.done → part.text="" - response.output_item.done → item.content=[] - response.completed → response.output=null Clients that accumulate deltas themselves (OpenAI SDK, Codex CLI) were unaffected. Clients that rehydrate from terminal events (Codex Desktop) showed empty responses because the authoritative post-stream state said the message had no content. Track accumulated text in a new state.TextBuffers map (keyed by output_index), populated from both text-delta emitters. The done/ completed emitters now retrieve and emit that text instead of a placeholder empty string. Reproduced live on Codex Desktop 0.128.0-alpha.1 → Bifrost → Bedrock (Opus 4.7): empty bubbles before, full text after. Part of maximhq#2492
|
Added a third commit for a related streaming bug surfaced while verifying Codex Desktop 0.128: The Bedrock responses-stream translator emits per-token
Clients that build the message by accumulating deltas (OpenAI SDK, Codex CLI) were unaffected. Clients that rehydrate from the terminal events (Codex Desktop) rendered empty bubbles because the authoritative post-stream state said the message had no content. The new commit adds a |
Summary
Three related defects in the Bedrock Converse translator that together blocked OpenAI Codex (CLI 0.128.0 and Desktop 0.128.0-alpha.1) from completing a single turn against Bedrock via Bifrost. All three live in
core/providers/bedrock/{responses.go,utils.go}, all three were discovered in one integration pass, and all three are verified together onv1.4.24deployed live.Keeping them bundled because (a) they're the same class — assumptions about OpenAI's wire format that don't hold for every client, (b) scope is identical (two files, both Bedrock-only), (c) they compound: fixing any one in isolation doesn't unblock Codex.
Changes
1.
developerrole not mapped tosystem(fixes #2492)OpenAI's Chat Completions and Responses APIs accept
role: "developer"as a system-message variant (introduced with GPT-5 / reasoning models). Bedrock Converse rejects anything other thanuser/assistantin itsmessagesarray.The responses-to-chat fallback in
core/schemas/mux.goalready handles this vianormalizeDeveloperRoleForChatFallback, and the Gemini provider handles it atcore/providers/gemini/responses.go:2962. The native Bedrock translators just missed the case.core/providers/bedrock/responses.go: foldResponsesInputMessageRoleDeveloperinto the system-message branchcore/providers/bedrock/utils.go: same forChatMessageRoleDeveloper2. Empty-content messages forwarded to Bedrock
Codex emits placeholder assistant messages with
content: []between tool-call turns. The translator forwarded these as Bedrock messages with empty Content slices, which Bedrock rejects:convertBifrostMessageToBedrockMessagereturnsnilwhen converted content is empty (caller already handles nil by skipping)convertMessagesskips user/assistant messages whose converted content is empty3. Streaming terminal events dropped accumulated text
The Bedrock responses-stream translator emits per-token
response.output_text.deltaevents correctly, but hardcodedemptyText := ""on every terminal event:response.output_text.done—text=""response.content_part.done—part.text=""response.output_item.done—item.content=[]response.completed—response.output=nullClients that accumulate deltas themselves (OpenAI SDK, Codex CLI) were unaffected. Clients that rehydrate from terminal events (Codex Desktop) rendered empty bubbles because the authoritative post-stream state said the message had no content.
TextBuffers map[int]stringtoBedrockResponsesStreamState, populated from both text-delta emit sitesresponse.completedreconstructs plain-text items inresponse.Outputfrom the buffer (tool-call items are materialised separately during the stream and are a pre-existing out-of-scope limitation)Diff size
core/providers/bedrock/responses.go: +85 / -7core/providers/bedrock/utils.go: +10 / -2How to test
End-to-end: Codex Desktop 0.128.0-alpha.1 → Bifrost → Bedrock Opus 4.7 (us-east-1) rendered empty bubbles before; full multi-turn with tool use after.
Type of change
Affected areas
Breaking changes
Each fix strictly expands the set of inputs the translator accepts or enriches the set of outputs it emits. No existing working call pattern changes shape.
Related issues
Fixes #2492. (#2528 was an earlier attempt at #1 that closed without merging — this PR takes the simpler, single-line approach the Gemini provider and chat fallback already use.)