Repository navigation
fix(bedrock): forward stop and max_completion_tokens as sent on Claude InvokeModel - #1344
Conversation
…e InvokeModel Non-streamed chat to Claude on Bedrock goes through InvokeModel, which builds its body with the shared Anthropic request builder and then re-derived `stop_sequences` from `stop` with the Converse helper. That helper drops empty strings and ignores anything that is not a string or an array, and the shaping step then removed `stop` and `max_completion_tokens` unconditionally. So `stop: 5`, `stop: ""`, `stop: ["", "x"]` and a non-integer `max_completion_tokens` were silently dropped or filtered on this route, while the Anthropic route forwards them and lets the upstream reject them. The re-derivation and both removals are gone: the body keeps what the shared builder produced, so InvokeModel now matches the Anthropic route for every input. A native `stop_sequences` still wins over `stop`, a string `stop` is still a one-element array, and `null` or `[]` still sets nothing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: forwarding stop and max_completion_tokens unchanged for Bedrock Claude InvokeModel requests. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| E2e Test Quality Review | |
PASS. The PR adds a real E2E path through the spawned gateway to a captured Bedrock /invoke endpoint. The test covers empty stop values, mixed empty/non-empty arrays, numeric stop values, and nonnum… |
| Security Check | |
No security vulnerability is introduced by the reviewed diff. The change only preserves stop, stop_sequences, and max_completion_tokens in the Bedrock InvokeModel request body. The code does not… |
Full details: E2e Test Quality Review
-
Explanation
PASS. The PR adds a real E2E path through the spawned gateway to a captured Bedrock
/invokeendpoint. The test covers empty stop values, mixed empty/non-empty arrays, numeric stop values, and nonnumericmax_completion_tokens. Rust tests additionally cover null and empty-array omission, nativestop_sequencesprecedence, and combined valid mappings. Assertions are readable, test cases are independent, and the implementation changes only the relevant InvokeModel shaping logic. No blocking error-handling, mocking, scope, or concurrency issue is evident.
Full details: Security Check
-
Explanation
No security vulnerability is introduced by the reviewed diff. The change only preserves
stop,stop_sequences, andmax_completion_tokensin the Bedrock InvokeModel request body. The code does not log or return these values, persist data, change authorization, access ownership, resource deletion, TLS settings, or secret-reference resolution. Category 1: No issues found. Category 2: No issues found. Category 3: No issues found. Category 4: No issues found. Category 5: No issues found. Category 6: No issues found. Category 7: No issues found. The changed request remains sent through the existing SigV4 Bedrock client, and the added tests only capture request bodies in test code.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Non-streamed chat to Claude on Amazon Bedrock now forwards
stopandmax_completion_tokensthe same way the Anthropic route does, instead of silently dropping or filtering values it can't map.The InvokeModel path builds its body with the shared Anthropic request builder, but
shape_invoke_bodythen re-derivedstop_sequencesfromstopusing the Converse helper (which drops empty strings and ignores non-string values) and removedstopandmax_completion_tokensunconditionally. The result was thatstop: 5and a non-integermax_completion_tokensvanished from the request andstop: ""/["", "gamma"]lost their empty entries, so these requests got a 200 that ignored what the caller asked for. On the Anthropic route the same requests reach the upstream as sent and are rejected with a 400.The fix removes that re-derivation and the two removals, so the InvokeModel body carries exactly what the shared builder produced. Unchanged: a native
stop_sequencesstill wins overstop, a stringstopbecomes a one-element array, andnullor[]sets nothing. A side effect is that a provider-key body override ofstop_sequencesis no longer overwritten on this route.Behavior change after upgrading: a non-streamed chat request to a Bedrock Claude model carrying an invalid
stop(a number, an empty string, an array with an empty string) or a non-integermax_completion_tokensnow gets the upstream's 400 (e.g.stop_sequences: each stop sequence must be non-empty,stop: Extra inputs are not permitted) instead of a 200 that ignored the value. Valid values behave as before. Streamed Bedrock chat and non-Claude Bedrock models use Converse, which builds a typed request, and are not changed here.Tests: the InvokeModel mock test
chat_anthropic_maps_max_completion_tokens_and_stopnow pins forwarding of empty stop strings,null/[], an unmappablestopand a non-integermax_completion_tokens, and a new case inbedrock-converse-and-chat-completions-e2echecks the body a non-streamed Claude request puts on/invokefor the same inputs. Both fail against the previousmain.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests