diff --git a/crates/goose-provider-types/src/conversation.rs b/crates/goose-provider-types/src/conversation.rs index 0541cf485..7f8276579 100644 --- a/crates/goose-provider-types/src/conversation.rs +++ b/crates/goose-provider-types/src/conversation.rs @@ -135,6 +135,10 @@ impl Conversation { self.0.pop() } + pub fn remove(&mut self, index: usize) -> Message { + self.0.remove(index) + } + pub fn truncate(&mut self, len: usize) { self.0.truncate(len); } @@ -247,6 +251,7 @@ fn fix_messages(messages: Vec) -> (Vec, Vec) { fix_empty_tool_results, fix_tool_calling, merge_consecutive_messages, + dedupe_signed_thinking, fix_lead_trail, populate_if_empty, ] @@ -502,6 +507,71 @@ pub fn merge_consecutive_messages(messages: Vec) -> (Vec, Vec< (merged_messages, issues) } +/// Signed thinking carries a signature; redacted thinking is always signed. +/// Signed blocks must be replayed exactly; unsigned reasoning summaries need not. +fn is_signed_thinking(content: &MessageContent) -> bool { + match content { + MessageContent::Thinking(t) => !t.signature.is_empty(), + MessageContent::RedactedThinking(_) => true, + _ => false, + } +} + +/// Drops duplicate signed thinking blocks, keeping the first occurrence. Some +/// signed-replay APIs (like Anthropic) reject a request that repeats the same +/// signed block more than once. +/// +/// Duplicates arise two ways, both handled here: +/// - Within one assistant message, when a standalone thinking message is +/// merged with a tool-call message that re-embedded the same thinking. +/// - Across assistant messages, when the agent splits one provider turn into +/// several tool-call messages (interleaved with tool results) that each +/// carry a copy of the turn's signed thinking. +/// +/// The `seen` set spans the whole conversation. A signed block carries a +/// cryptographic signature unique to its generation, so an exact (text + +/// signature) match can only be the same turn's thinking copied onto split +/// messages — never two genuinely distinct thoughts. Unsigned reasoning +/// summaries are left untouched, since providers like Kimi/DeepSeek require +/// them echoed on every tool-call message. +/// +/// This runs before any provider formatter, so it covers every Claude transport +/// (direct Anthropic, Bedrock, Databricks, Vertex) in one place. +fn dedupe_signed_thinking(messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + let mut seen: Vec = Vec::new(); + + let fixed_messages = messages + .into_iter() + .map(|mut message| { + if message.role != Role::Assistant { + return message; + } + + let original_len = message.content.len(); + let mut deduped: Vec = Vec::with_capacity(original_len); + for content in &message.content { + let is_signed = is_signed_thinking(content); + if is_signed && seen.contains(content) { + continue; + } + if is_signed { + seen.push(content.clone()); + } + deduped.push(content.clone()); + } + + if deduped.len() != original_len { + issues.push("Removed duplicate signed thinking block".to_string()); + message.content = deduped; + } + message + }) + .collect(); + + (fixed_messages, issues) +} + fn has_tool_response(message: &Message) -> bool { message .content @@ -1304,6 +1374,237 @@ mod tests { assert!(!fixed_messages[5].metadata.agent_visible); } + #[test] + fn test_dedupes_duplicate_signed_thinking_around_tool_call() { + use crate::conversation::message::MessageContent; + use rmcp::model::Content; + + // Reproduces the Anthropic 400 scenario: a standalone signed thinking + // message immediately followed by an assistant message that repeats the + // same signed thinking plus a tool_use. After merging consecutive + // assistant messages these become two adjacent identical thinking blocks. + let messages = vec![ + Message::user().with_text("Do the thing"), + Message::assistant().with_thinking("Let me think about this", "sig-1"), + Message::assistant() + .with_thinking("Let me think about this", "sig-1") + .with_tool_request( + "tool_1", + Ok(CallToolRequestParams::new("do_thing").with_arguments(object!({"x": 1}))), + ), + Message::user().with_tool_response( + "tool_1", + Ok(rmcp::model::CallToolResult::success(vec![Content::text( + "done", + )])), + ), + Message::user().with_text("Now continue"), + ]; + + let (fixed, issues) = fix_conversation(Conversation::new_unvalidated(messages)); + + assert!( + issues + .iter() + .any(|i| i == "Removed duplicate signed thinking block"), + "expected dedupe issue, got: {:?}", + issues + ); + + let fixed_messages = fixed.messages(); + let assistant = fixed_messages + .iter() + .find(|m| { + m.role == Role::Assistant + && m.content + .iter() + .any(|c| matches!(c, MessageContent::ToolRequest(_))) + }) + .expect("assistant tool-call message should exist"); + + let thinking_count = assistant + .content + .iter() + .filter(|c| { + matches!( + c, + MessageContent::Thinking(_) | MessageContent::RedactedThinking(_) + ) + }) + .count(); + assert_eq!( + thinking_count, 1, + "duplicate signed thinking should be collapsed to one block" + ); + } + + #[test] + fn test_keeps_distinct_signed_thinking_blocks_in_assistant_message() { + use crate::conversation::message::MessageContent; + use rmcp::model::Content; + + // Distinct signed thinking blocks (different signatures) must be preserved. + let messages = vec![ + Message::user().with_text("Do the thing"), + Message::assistant() + .with_thinking("First thought", "sig-A") + .with_thinking("Second thought", "sig-B") + .with_tool_request( + "tool_1", + Ok(CallToolRequestParams::new("do_thing").with_arguments(object!({"x": 1}))), + ), + Message::user().with_tool_response( + "tool_1", + Ok(rmcp::model::CallToolResult::success(vec![Content::text( + "done", + )])), + ), + Message::user().with_text("Now continue"), + ]; + + let (fixed, issues) = fix_conversation(Conversation::new_unvalidated(messages)); + + assert!( + !issues + .iter() + .any(|i| i == "Removed duplicate signed thinking block"), + "should not dedupe distinct thinking blocks, got: {:?}", + issues + ); + + let fixed_messages = fixed.messages(); + let thinking_count = fixed_messages[1] + .content + .iter() + .filter(|c| { + matches!( + c, + MessageContent::Thinking(_) | MessageContent::RedactedThinking(_) + ) + }) + .count(); + assert_eq!(thinking_count, 2, "distinct thinking blocks must be kept"); + } + + #[test] + fn test_keeps_duplicate_unsigned_thinking_blocks() { + use crate::conversation::message::MessageContent; + + // Unsigned thinking (reasoning summaries from non-Anthropic providers) + // can legitimately repeat and must not be dropped, since only signed + // blocks trigger the Anthropic exact-replay 400. + let messages = vec![ + Message::user().with_text("Do the thing"), + Message::assistant() + .with_thinking("same reasoning", "") + .with_thinking("same reasoning", ""), + Message::user().with_text("Now continue"), + ]; + + let (fixed, issues) = fix_conversation(Conversation::new_unvalidated(messages)); + + assert!( + !issues + .iter() + .any(|i| i == "Removed duplicate signed thinking block"), + "unsigned thinking must not be deduped, got: {:?}", + issues + ); + + let fixed_messages = fixed.messages(); + let thinking_count = fixed_messages + .iter() + .flat_map(|m| m.content.iter()) + .filter(|c| matches!(c, MessageContent::Thinking(_))) + .count(); + assert_eq!( + thinking_count, 2, + "duplicate unsigned thinking blocks must be kept" + ); + } + + #[test] + fn test_dedupes_signed_thinking_across_split_tool_messages() { + use crate::conversation::message::MessageContent; + use rmcp::model::Content; + + // The agent splits one provider turn with multiple tool calls into one + // assistant message per call (interleaved with tool results), each + // carrying the same signed thinking. merge_consecutive_messages cannot + // merge them because tool results sit between, so the dedupe must span + // messages and keep the signed block only on the first. + let messages = vec![ + Message::user().with_text("Use both tools"), + Message::assistant() + .with_thinking("multi-tool reasoning", "sig-1") + .with_tool_request( + "call_1", + Ok(CallToolRequestParams::new("tool_a").with_arguments(object!({"p": 1}))), + ), + Message::user().with_tool_response( + "call_1", + Ok(rmcp::model::CallToolResult::success(vec![Content::text( + "ok", + )])), + ), + Message::assistant() + .with_thinking("multi-tool reasoning", "sig-1") + .with_tool_request( + "call_2", + Ok(CallToolRequestParams::new("tool_b").with_arguments(object!({"p": 2}))), + ), + Message::user().with_tool_response( + "call_2", + Ok(rmcp::model::CallToolResult::success(vec![Content::text( + "ok", + )])), + ), + Message::user().with_text("Now continue"), + ]; + + let (fixed, issues) = fix_conversation(Conversation::new_unvalidated(messages)); + + assert!( + issues + .iter() + .any(|i| i == "Removed duplicate signed thinking block"), + "expected cross-message dedupe issue, got: {:?}", + issues + ); + + let fixed_messages = fixed.messages(); + let total_thinking = fixed_messages + .iter() + .flat_map(|m| m.content.iter()) + .filter(|c| { + matches!( + c, + MessageContent::Thinking(_) | MessageContent::RedactedThinking(_) + ) + }) + .count(); + assert_eq!( + total_thinking, 1, + "the repeated signed block must survive only once across the turn" + ); + + let total_tool_requests = fixed_messages + .iter() + .flat_map(|m| m.content.iter()) + .filter(|c| matches!(c, MessageContent::ToolRequest(_))) + .count(); + assert_eq!(total_tool_requests, 2, "both tool calls must be preserved"); + + // The first split message keeps the thinking; the second loses it. + assert!( + fixed_messages[1] + .content + .iter() + .any(|c| matches!(c, MessageContent::Thinking(_))), + "first split message keeps signed thinking" + ); + } + #[test] fn test_push_coalesces_thinking_deltas() { use crate::conversation::message::MessageContent; diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 0f2687706..39a2d888f 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -2291,9 +2291,16 @@ impl Agent { } } - // Preserve thinking/reasoning content from the original response. - // Gemini (and other thinking models) require thinking to be echoed back. - // Kimi/DeepSeek require reasoning_content on assistant tool call messages. + // Thinking/reasoning belongs on the tool-call messages, not also + // as a separate standalone message: Gemini and Kimi/DeepSeek + // require it echoed on each assistant tool-call message, and the + // provider formatters reconstruct per-provider shape from there. + // Storing it both standalone AND on the tool-call message + // duplicates it; once merge_consecutive_messages glues the adjacent + // standalone and tool-call messages together, the duplicate signed + // blocks make Anthropic reject the turn with a 400. So the thinking + // is carried onto the split request messages below and never kept + // as a redundant standalone message. let direct_thinking: Vec = response .content .iter() @@ -2306,36 +2313,25 @@ impl Agent { }) .cloned() .collect(); - if !direct_thinking.is_empty() { - let thinking_msg = Message::new( - response.role.clone(), - response.created, - direct_thinking.clone(), - ) - .with_id(format!("msg_{}", Uuid::new_v4())); - messages_to_add.push(thinking_msg); - } - // When thinking arrived in an earlier stream chunk (stored as a - // thinking-only message) and this chunk has only tool calls, - // reuse that thinking so each split request_msg carries it. + // When thinking arrived in an earlier stream chunk it was stored as + // a standalone thinking-only message; reuse that thinking on the + // tool-call messages and drop the standalone so it isn't duplicated. let response_thinking = if direct_thinking.is_empty() { - messages_to_add - .messages() - .iter() - .rev() - .find(|m| { - m.role == response.role - && !m.content.is_empty() - && m.content.iter().all(|c| { - matches!( - c, - MessageContent::Thinking(_) - | MessageContent::RedactedThinking(_) - ) - }) - }) - .map(|m| m.content.clone()) - .unwrap_or_default() + let prior = messages_to_add.messages().iter().rposition(|m| { + m.role == response.role + && !m.content.is_empty() + && m.content.iter().all(|c| { + matches!( + c, + MessageContent::Thinking(_) + | MessageContent::RedactedThinking(_) + ) + }) + }); + match prior { + Some(idx) => messages_to_add.remove(idx).content, + None => Vec::new(), + } } else { direct_thinking }; diff --git a/crates/goose/tests/agent.rs b/crates/goose/tests/agent.rs index 56305607e..175128640 100644 --- a/crates/goose/tests/agent.rs +++ b/crates/goose/tests/agent.rs @@ -1988,6 +1988,105 @@ mod tests { Ok(()) } + + /// Regression for the Anthropic 400: signed thinking arriving in a + /// separate chunk before the tool calls must be stored once per + /// tool-call message and never as an extra standalone message. When the + /// Anthropic formatter serializes the persisted history, each assistant + /// turn must carry exactly one thinking block — a duplicate signed block + /// is rejected with `thinking blocks ... cannot be modified`. + #[tokio::test] + async fn test_signed_thinking_not_duplicated_for_anthropic() -> Result<()> { + use goose_providers::formats::anthropic::format_messages as anthropic_format; + + let temp_dir = tempfile::tempdir()?; + let session_manager = Arc::new(SessionManager::new(temp_dir.path().to_path_buf())); + let config = AgentConfig::new( + session_manager.clone(), + PermissionManager::instance(), + None, + GooseMode::Auto, + true, + GoosePlatform::GooseCli, + ); + let agent = Agent::with_config(config); + let provider = Arc::new(MultiToolThinkingProvider::new()); + + let session = session_manager + .create_session( + PathBuf::default(), + "anthropic-signed-thinking-test".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + + let session_id = session.id.clone(); + agent + .update_provider(provider, ModelConfig::new("mock-model"), &session_id) + .await?; + + let session_config = SessionConfig { + id: session_id.clone(), + schedule_id: None, + max_turns: Some(2), + retry_config: None, + }; + + let reply_stream = agent + .reply( + Message::user().with_text("Use both tools"), + session_config, + None, + ) + .await?; + tokio::pin!(reply_stream); + while let Some(event) = reply_stream.next().await { + event?; + } + + let reloaded = session_manager.get_session(&session_id, true).await?; + let messages = reloaded + .conversation + .expect("should have conversation") + .messages() + .to_vec(); + + // No standalone thinking-only assistant message should be persisted — + // thinking lives on the tool-call messages. + let standalone_thinking = messages.iter().any(|m| { + m.role == rmcp::model::Role::Assistant + && !m.content.is_empty() + && m.content + .iter() + .all(|c| matches!(c, MessageContent::Thinking(_))) + }); + assert!( + !standalone_thinking, + "thinking must not be persisted as a standalone message: {messages:#?}" + ); + + // Every serialized Anthropic assistant message must contain at most + // one thinking block; a duplicate is what triggers the 400. + let spec = anthropic_format(&messages); + for msg in &spec { + if msg.get("role") == Some(&serde_json::json!("assistant")) { + if let Some(content) = msg.get("content").and_then(|c| c.as_array()) { + let thinking_blocks = content + .iter() + .filter(|c| c.get("type") == Some(&serde_json::json!("thinking"))) + .count(); + assert!( + thinking_blocks <= 1, + "assistant message has {thinking_blocks} thinking blocks, \ + Anthropic rejects duplicates: {msg}" + ); + } + } + } + + Ok(()) + } } #[cfg(test)]