diff --git a/CHANGELOG.md b/CHANGELOG.md index e421f76..c541851 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ adheres to [Semantic Versioning](https://semver.org/). ## Unreleased +- **Bedrock, Gemini and Vertex no longer send an empty message** (#369). An assistant turn that errored, was refused or was cancelled with no content (the loop's error message is one empty text block, which the builders filter) — or whose only content is thinking the provider does not replay (on Gemini and Vertex any thinking; on Bedrock unsigned reasoning to a Claude model, or another API's) — went out as `content: []` / `parts: []`, which both APIs reject (as did a user message with only empty text); the Anthropic provider already skipped such an assistant turn. So the prompt after a failed or cancelled run was rejected too. Empty messages of either role are now dropped before consecutive same-role messages are merged (#368), so the user turns around a dropped one go back as one. - **Bedrock, Gemini and Vertex: a parallel tool call's results go back in one message** (#223). Each tool result was sent as its own user-role message, and a steering message, stop marker or compaction summary after them added another, while Bedrock's Converse requires roles to alternate and Gemini requires a turn's function responses in one content, as many as its function calls. So the request after any turn with two or more tool calls (the default parallel strategy) could be rejected. The three request builders now merge consecutive same-role messages, keeping every block in order: the results in call order, then whatever followed; on Gemini and Vertex a merged turn's function responses come first, then any images and text (images among the responses are a reported cause of Gemini's response-count 400). Pinned by request-body tests; not run against a live endpoint. ## 0.25.12 (2026-10-21) diff --git a/CLAUDE.md b/CLAUDE.md index 4039766..9cf6bc6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,7 +61,7 @@ The Responses and Azure providers share one request builder + read loop (`respon Anthropic SSE (`anthropic.rs`): block `index` → position in `content` via a map, never padding, so a block type it does not surface (server tool blocks, `fallback`, anything new) leaves no placeholder and later blocks keep their deltas; unknown block/delta types `warn_once` per type by name, unparseable start/delta events are logged. `redacted_thinking` (`data` arrives whole at `content_block_start`, no delta type) → `Content::thinking_redacted(ApiProtocol::AnthropicMessages, data)` → replayed as `{"type":"redacted_thinking","data"}` in place (#197). -Bedrock is the one non-SSE provider: ConverseStream answers with binary `application/vnd.amazon.eventstream` frames, decoded by the private `provider/eventstream.rs` (bytes buffered across chunks; prelude + message CRC-32 verified with an in-house table, no dependency; all ten header types; checksum/malformed → `Other`, a body ending mid-frame → `Network`; pinned by aws-sdk-go's published test vectors). `bedrock.rs` dispatches on the `:message-type` / `:event-type` / `:exception-type` headers — payloads are the event structs themselves (no wrapper key), shapes per the AWS Bedrock Runtime API reference. Blocks accumulate per `contentBlockIndex`; a tool call is pushed with the `__partial_json` marker and only `contentBlockStop` finalizes it (`finalize_tool_arguments`), so an unclosed block is never runnable. Exceptions classify via their documented HTTP status (`throttlingException`/`serviceUnavailableException` → `RateLimited`; other 5xx stay `Api`). No `messageStop` (incl. empty body) or a transport error → `Network`, never an empty `Ok` — except once `messageStop` **and** `metadata` arrived: a later drop/truncation keeps the response (no re-billed retry). `messageStop` without `metadata` → zero usage + `warn!(usage_missing = true)` (same zero as other providers). Unsurfaced content (image/citation/toolResult, server_tool_use, unknown union members via `#[serde(flatten)] other`, unknown event types) is dropped with a once-per-block warning; an unknown `contentBlockStart` member creates no block, so the first delta types it. Reasoning replay: signed → `reasoningText`, `redactedContent` kept in `Content::Thinking::redacted` (base64, bytes concatenated across deltas; `redacted_protocol = BedrockConverseStream`) → `redactedContent`, only if it came from Bedrock (another API's redacted data is skipped, logged at debug); unsigned → `reasoningText {text}` with no signature key, except to a model id containing `claude` (signature-verifying), where it is skipped at debug (never `signature: ""`); the Anthropic provider skips Bedrock's redacted blocks. Signature deltas are appended. Tested only with mock frames (`tests/bedrock_stream_test.rs`). Consecutive same-role messages are merged before sending (`provider::merge_same_role(messages, "content")`; Gemini and Vertex do the same with `"parts"`, then `google::function_responses_first` puts each user content's `functionResponse` parts before its images and text), so a parallel tool call's results go back as one user message and roles alternate (#223). Auth (`resolve_auth`, first match wins): an `authorization` key in `ModelConfig.headers` (any case) → sent alone; non-empty `api_key` → `access:secret[:token]` = SigV4 (`ASIA…` without a token refused), no `:` = Bedrock API key (`Bearer`) after `checked_bearer` refuses whitespace/control chars, an `AKIA`/`ASIA` start, non-base64 chars and a bare 40-char secret (unknown prefix — not `bedrock-api-key-`/`ABSK` — only warns once); empty `api_key` → env read per request (type `Env` = `std::env::var`-shaped, faked in tests), `AWS_BEARER_TOKEN_BEDROCK` (same checks) then `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY`(/`AWS_SESSION_TOKEN`); half a pair / NotUnicode / none → `Auth` naming the variable, before sending. `resolve_api_key_or_warn(Option<&ModelConfig>)` (Agent, SubAgentTool, LlmCompaction) keys on the **protocol**: any `BedrockConverseStream` config gets an empty key (never `API_KEY`/`YOAGENT_API_KEY`, whatever the provider string), so the provider reads env itself; `resolve_api_key("bedrock")` (public) prefers `AWS_BEARER_TOKEN_BEDROCK`. `request_headers` builds the whole `HeaderMap`: every value via `header_value` (credential values must be ASCII; failure → `Auth` naming the header, never the value; user non-auth header → `Other`), `authorization`/`x-amz-security-token` `set_sensitive`; on the SigV4 path a user header named like a signed one is refused; a `send()` `is_builder()` error → `Other`, not `Network`. SigV4 is the private `provider/sigv4.rs` (sha2 + hmac, no AWS SDK; pinned to aws-c-auth's `aws-sig-v4-test-suite` vectors incl. the `*-normalized` path ones): signing name `bedrock`, region from the host (`bedrock-runtime[-fips]..`, suffix ∈ `AWS_DNS_SUFFIXES` = partition dnsSuffix/dualStackDnsSuffix from botocore + VPC forms) > `AWS_REGION` > `AWS_DEFAULT_REGION` > `Auth` error; signs `host`, `content-type`, `x-amz-date`, `x-amz-content-sha256`, `x-amz-security-token` over the exact body bytes sent (serialize once — never `.json()`); `amz_date_now` errors on a pre-1970 clock; a 403 "Signature expired"/"not yet current" gets a check-the-clock hint. `base_url` is `trim_end_matches('/')`-ed; `canonical_uri` normalizes (drops empty/`.` segments, resolves `..`, keeps a trailing `/`) and encodes each segment again, so the model id (`:` → `%3A` on the wire, as the SDKs send it) is `%253A` in the canonical URI. `tests/bedrock_auth_test.rs` (env-mutating, serialized by a tokio `Mutex`) recomputes each signature from what wiremock received and uses `expect(0)` for every refusal. Never run against a live endpoint. +Bedrock is the one non-SSE provider: ConverseStream answers with binary `application/vnd.amazon.eventstream` frames, decoded by the private `provider/eventstream.rs` (bytes buffered across chunks; prelude + message CRC-32 verified with an in-house table, no dependency; all ten header types; checksum/malformed → `Other`, a body ending mid-frame → `Network`; pinned by aws-sdk-go's published test vectors). `bedrock.rs` dispatches on the `:message-type` / `:event-type` / `:exception-type` headers — payloads are the event structs themselves (no wrapper key), shapes per the AWS Bedrock Runtime API reference. Blocks accumulate per `contentBlockIndex`; a tool call is pushed with the `__partial_json` marker and only `contentBlockStop` finalizes it (`finalize_tool_arguments`), so an unclosed block is never runnable. Exceptions classify via their documented HTTP status (`throttlingException`/`serviceUnavailableException` → `RateLimited`; other 5xx stay `Api`). No `messageStop` (incl. empty body) or a transport error → `Network`, never an empty `Ok` — except once `messageStop` **and** `metadata` arrived: a later drop/truncation keeps the response (no re-billed retry). `messageStop` without `metadata` → zero usage + `warn!(usage_missing = true)` (same zero as other providers). Unsurfaced content (image/citation/toolResult, server_tool_use, unknown union members via `#[serde(flatten)] other`, unknown event types) is dropped with a once-per-block warning; an unknown `contentBlockStart` member creates no block, so the first delta types it. Reasoning replay: signed → `reasoningText`, `redactedContent` kept in `Content::Thinking::redacted` (base64, bytes concatenated across deltas; `redacted_protocol = BedrockConverseStream`) → `redactedContent`, only if it came from Bedrock (another API's redacted data is skipped, logged at debug); unsigned → `reasoningText {text}` with no signature key, except to a model id containing `claude` (signature-verifying), where it is skipped at debug (never `signature: ""`); the Anthropic provider skips Bedrock's redacted blocks. Signature deltas are appended. Tested only with mock frames (`tests/bedrock_stream_test.rs`). Consecutive same-role messages are merged before sending (`provider::merge_same_role(messages, "content")`; Gemini and Vertex do the same with `"parts"`, then `google::function_responses_first` puts each user content's `functionResponse` parts before its images and text), so a parallel tool call's results go back as one user message and roles alternate (#223); a message whose array is empty (an errored, refused or cancelled turn, or one with only unreplayable thinking; either role) is dropped first and its neighbours merge (#369). Auth (`resolve_auth`, first match wins): an `authorization` key in `ModelConfig.headers` (any case) → sent alone; non-empty `api_key` → `access:secret[:token]` = SigV4 (`ASIA…` without a token refused), no `:` = Bedrock API key (`Bearer`) after `checked_bearer` refuses whitespace/control chars, an `AKIA`/`ASIA` start, non-base64 chars and a bare 40-char secret (unknown prefix — not `bedrock-api-key-`/`ABSK` — only warns once); empty `api_key` → env read per request (type `Env` = `std::env::var`-shaped, faked in tests), `AWS_BEARER_TOKEN_BEDROCK` (same checks) then `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY`(/`AWS_SESSION_TOKEN`); half a pair / NotUnicode / none → `Auth` naming the variable, before sending. `resolve_api_key_or_warn(Option<&ModelConfig>)` (Agent, SubAgentTool, LlmCompaction) keys on the **protocol**: any `BedrockConverseStream` config gets an empty key (never `API_KEY`/`YOAGENT_API_KEY`, whatever the provider string), so the provider reads env itself; `resolve_api_key("bedrock")` (public) prefers `AWS_BEARER_TOKEN_BEDROCK`. `request_headers` builds the whole `HeaderMap`: every value via `header_value` (credential values must be ASCII; failure → `Auth` naming the header, never the value; user non-auth header → `Other`), `authorization`/`x-amz-security-token` `set_sensitive`; on the SigV4 path a user header named like a signed one is refused; a `send()` `is_builder()` error → `Other`, not `Network`. SigV4 is the private `provider/sigv4.rs` (sha2 + hmac, no AWS SDK; pinned to aws-c-auth's `aws-sig-v4-test-suite` vectors incl. the `*-normalized` path ones): signing name `bedrock`, region from the host (`bedrock-runtime[-fips]..`, suffix ∈ `AWS_DNS_SUFFIXES` = partition dnsSuffix/dualStackDnsSuffix from botocore + VPC forms) > `AWS_REGION` > `AWS_DEFAULT_REGION` > `Auth` error; signs `host`, `content-type`, `x-amz-date`, `x-amz-content-sha256`, `x-amz-security-token` over the exact body bytes sent (serialize once — never `.json()`); `amz_date_now` errors on a pre-1970 clock; a 403 "Signature expired"/"not yet current" gets a check-the-clock hint. `base_url` is `trim_end_matches('/')`-ed; `canonical_uri` normalizes (drops empty/`.` segments, resolves `..`, keeps a trailing `/`) and encodes each segment again, so the model id (`:` → `%3A` on the wire, as the SDKs send it) is `%253A` in the canonical URI. `tests/bedrock_auth_test.rs` (env-mutating, serialized by a tokio `Mutex`) recomputes each signature from what wiremock received and uses `expect(0)` for every refusal. Never run against a live endpoint. `ModelConfig` + quirk structs handle per-provider differences: `OpenAiCompat` (reasoning format, max_tokens field name, `max_reasoning_effort` ceiling — the ceiling, and `encrypted_reasoning`, are also read by Responses/Azure), `AnthropicCompat` (adaptive vs budget thinking, bearer auth, `native_structured_output`; `for_claude_id` infers them from an id), `GoogleCompat` (`thinkingLevel` vs `thinkingBudget`). `ThinkingLevel::Off` sends no thinking/effort field on any provider (DeepSeek's explicit `thinking: disabled` aside), so always-thinking models run at their default. diff --git a/docs/providers/bedrock.md b/docs/providers/bedrock.md index 8267555..18121a7 100644 --- a/docs/providers/bedrock.md +++ b/docs/providers/bedrock.md @@ -209,7 +209,7 @@ is kept in `Content::Thinking::redacted` (with `redacted_protocol` set to `BedrockConverseStream`) and sent back as `redactedContent`. Encrypted reasoning from another API is not: AWS does not document `redactedContent` as the same bytes as the Anthropic API's `redacted_thinking` data, so an Anthropic -redacted block is skipped here with a warning (and Bedrock's is skipped by the +redacted block is skipped here, logged at debug (and Bedrock's is skipped by the Anthropic provider). Reasoning without a signature is sent back as `reasoningText {text}` with no signature key (the signature is optional in `ReasoningTextBlock`, and Bedrock's diff --git a/src/provider/bedrock.rs b/src/provider/bedrock.rs index 12e8bec..e69057b 100644 --- a/src/provider/bedrock.rs +++ b/src/provider/bedrock.rs @@ -1611,6 +1611,31 @@ impl TokenUsage { mod tests { use super::*; + /// An errored assistant turn — as the loop builds it, one empty text block, + /// or with no content at all — is not sent as an empty message; the user + /// turns around it go back as one (#369). + #[test] + fn an_empty_assistant_turn_is_not_sent() { + for errored in [ + vec![Content::Text { + text: String::new(), + }], + vec![], + ] { + let mut config = StreamConfig::new("anthropic.claude-sonnet", "a:b"); + config.messages = vec![ + Message::user("first"), + Message::assistant(errored, StopReason::Error, "m", "bedrock", Usage::default()), + Message::user("again"), + ]; + let body = build_bedrock_body(&config); + assert_eq!( + body["messages"], + serde_json::json!([{"role": "user", "content": [{"text": "first"}, {"text": "again"}]}]) + ); + } + } + /// A parallel tool call's results go back as one user message, the /// steering message after them, so Converse's roles alternate (#223). #[test] diff --git a/src/provider/google.rs b/src/provider/google.rs index b2e6f39..f8c9f8b 100644 --- a/src/provider/google.rs +++ b/src/provider/google.rs @@ -833,6 +833,31 @@ mod tests { use super::*; use crate::provider::{GoogleCompat, ModelConfig}; + /// A model turn whose only content is thinking (not replayed), or the + /// loop's error message (one empty text block), is not sent as an empty + /// content (#369). + #[test] + fn an_empty_model_turn_is_not_sent() { + for turn in [ + vec![Content::thinking("thought")], + vec![Content::Text { + text: String::new(), + }], + ] { + let mut config = StreamConfig::new("gemini-2.5-pro", "k"); + config.messages = vec![ + Message::user("first"), + Message::assistant(turn, StopReason::Error, "m", "google", Usage::default()), + Message::user("again"), + ]; + let body = build_request_body(&config); + assert_eq!( + body["contents"], + serde_json::json!([{"role": "user", "parts": [{"text": "first"}, {"text": "again"}]}]) + ); + } + } + /// A parallel tool call's results go back as one user turn, with the /// steering message after them, so roles alternate (#223). #[test] diff --git a/src/provider/google_vertex.rs b/src/provider/google_vertex.rs index 6df8912..6fc45a3 100644 --- a/src/provider/google_vertex.rs +++ b/src/provider/google_vertex.rs @@ -373,6 +373,29 @@ mod tests { ); } + /// A model turn with only thinking (not replayed) is not sent as an empty + /// content; the user turns around it go back as one (#369). + #[test] + fn an_empty_model_turn_is_not_sent() { + let mut c = config(ThinkingLevel::Off); + c.messages = vec![ + Message::user("first"), + Message::assistant( + vec![Content::thinking("thought")], + crate::types::StopReason::Error, + "m", + "vertex", + crate::types::Usage::default(), + ), + Message::user("again"), + ]; + let body = build_vertex_request_body(&c); + assert_eq!( + body["contents"], + serde_json::json!([{"role": "user", "parts": [{"text": "first"}, {"text": "again"}]}]) + ); + } + /// A parallel tool call's results go back as one user turn (#223). #[test] fn parallel_tool_results_go_back_in_one_turn() { diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 515c588..49a26e2 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -71,12 +71,28 @@ pub(crate) fn tool_result_text(content: &[crate::types::Content]) -> String { /// message, a stop marker or a compaction summary right after them), so these /// builders produce one wire message per result. Merging keeps every block in /// order: the results first, in call order, then whatever followed them. +/// +/// A message whose `key` array is empty is dropped first, of either role: +/// both APIs reject an empty message, and a turn that errored, was refused or +/// was cancelled with no content (the loop's error message is one empty text +/// block, which the builders filter), or whose only content is thinking the +/// provider does not replay, would otherwise be one. Its neighbours then +/// merge, so the roles still alternate (#369). A first message dropped this +/// way can leave an assistant turn first, which Bedrock rejects — as it +/// rejected the empty message before. pub(crate) fn merge_same_role( messages: Vec, key: &str, ) -> Vec { let mut merged: Vec = Vec::with_capacity(messages.len()); for message in messages { + if message + .get(key) + .and_then(serde_json::Value::as_array) + .is_some_and(Vec::is_empty) + { + continue; + } if let Some(last) = merged.last_mut() { if last.get("role") == message.get("role") { let from = message.get(key).and_then(serde_json::Value::as_array); @@ -130,6 +146,53 @@ mod merge_same_role_tests { use super::merge_same_role; use serde_json::json; + /// An empty message is dropped, first, in the middle or last, and its + /// neighbours merge: an assistant turn that errored with no content + /// between two user turns leaves one user message (#369). + #[test] + fn empty_messages_are_dropped_and_their_neighbours_merge() { + let merged = merge_same_role( + vec![ + json!({"role": "user", "parts": []}), + json!({"role": "user", "parts": [{"text": "1"}]}), + json!({"role": "model", "parts": []}), + json!({"role": "user", "parts": [{"text": "2"}]}), + json!({"role": "model", "parts": [{"text": "3"}]}), + json!({"role": "model", "parts": []}), + ], + "parts", + ); + assert_eq!( + merged, + vec![ + json!({"role": "user", "parts": [{"text": "1"}, {"text": "2"}]}), + json!({"role": "model", "parts": [{"text": "3"}]}), + ] + ); + } + + /// An empty user message is dropped too, and the model turns around it + /// then merge (#369). + #[test] + fn an_empty_user_message_is_dropped() { + let merged = merge_same_role( + vec![ + json!({"role": "user", "content": [{"text": "a"}]}), + json!({"role": "assistant", "content": [{"text": "x"}]}), + json!({"role": "user", "content": []}), + json!({"role": "assistant", "content": [{"text": "y"}]}), + ], + "content", + ); + assert_eq!( + merged, + vec![ + json!({"role": "user", "content": [{"text": "a"}]}), + json!({"role": "assistant", "content": [{"text": "x"}, {"text": "y"}]}), + ] + ); + } + #[test] fn adjacent_same_role_messages_become_one_in_order() { let merged = merge_same_role( diff --git a/tests/bedrock_stream_test.rs b/tests/bedrock_stream_test.rs index 1e3f8f6..f2d7d41 100644 --- a/tests/bedrock_stream_test.rs +++ b/tests/bedrock_stream_test.rs @@ -634,6 +634,49 @@ async fn reasoning_with_signature_is_accumulated_and_replayed() { ); } +/// A run that failed leaves an errored assistant message (one empty text +/// block) in the history; the next prompt does not send it as an empty +/// message, so the two user turns go back as one (#369). The history keeps it. +#[tokio::test] +async fn an_errored_turn_is_not_resent_as_an_empty_message() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(400).set_body_json(json!({"message": "Bad request"}))) + .up_to_n_times(1) + .with_priority(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_body_raw(text_response("ok"), EVENTSTREAM)) + .with_priority(2) + .mount(&server) + .await; + let mut agent = Agent::from_provider(BedrockProvider, model_config(&server.uri())) + .with_api_key("test-bedrock-api-key"); + for prompt in ["one", "two"] { + let mut rx = agent.prompt(prompt).await; + while rx.recv().await.is_some() {} + agent.finish().await; + } + let errored = agent.messages().iter().any(|m| { + matches!( + m, + AgentMessage::Llm(Message::Assistant { + stop_reason: StopReason::Error, + .. + }) + ) + }); + assert!(errored, "the history keeps the errored turn"); + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2); + let body: Value = serde_json::from_slice(&requests[1].body).unwrap(); + assert_eq!( + body["messages"], + json!([{"role": "user", "content": [{"text": "one"}, {"text": "two"}]}]) + ); +} + /// A parallel tool call through the real loop: both results go back in one /// user message, in call order, so Converse's roles alternate (#223). #[tokio::test]