diff --git a/src/provider/mock.rs b/src/provider/mock.rs index 26af56d..bf945cd 100644 --- a/src/provider/mock.rs +++ b/src/provider/mock.rs @@ -32,15 +32,95 @@ pub struct MockToolCall { /// Mock LLM provider for tests. Supply a sequence of responses. pub struct MockProvider { responses: std::sync::Mutex>, + /// Whether to reject a request whose transcript a real provider would. + /// + /// On by default, and that default is the point. This type used to discard + /// `StreamConfig` entirely, so 600+ tests drove the agent loop without one + /// of them noticing when it built a message sequence every provider + /// rejects. Loop detection shipped exactly that bug to a release branch: + /// a steering message injected between an assistant's `tool_use` blocks and + /// their `tool_result`s, which poisons the agent for every later prompt. + /// + /// Turning this off is legitimate for a test that deliberately constructs a + /// malformed history — say the compaction orphan-handling paths — but it + /// should be deliberate and commented. + validate_transcript: bool, +} + +/// Reject a message sequence a real provider would reject. +/// +/// The invariant every provider enforces: an assistant's `tool_use` blocks must +/// be answered by their `tool_result`s before anything else intervenes. +/// Anthropic returns "tool_use ids were found without tool_result blocks +/// immediately after"; OpenAI returns the equivalent about tool messages. The +/// crate treats this as sacred elsewhere — `context.rs` calls an unanswered +/// call "an orphan every provider rejects", and `llm_compaction.rs` has a +/// dedicated test for it — but nothing enforced it on the loop's own output. +/// +/// Returns the reason a provider would give, or `None` if the transcript is +/// well-formed. +fn transcript_violation(messages: &[Message]) -> Option { + let mut pending: Vec = Vec::new(); + for (i, msg) in messages.iter().enumerate() { + match msg { + Message::Assistant { content, .. } => { + if !pending.is_empty() { + return Some(format!( + "assistant message at [{i}] while tool calls {pending:?} are still \ + unanswered" + )); + } + pending = content + .iter() + .filter_map(|c| match c { + Content::ToolCall { id, .. } => Some(id.clone()), + _ => None, + }) + .collect(); + } + Message::ToolResult { tool_call_id, .. } => { + match pending.iter().position(|p| p == tool_call_id) { + Some(at) => { + pending.remove(at); + } + None => { + return Some(format!( + "tool_result at [{i}] for {tool_call_id:?} answers no open tool call" + )) + } + } + } + Message::User { .. } => { + if !pending.is_empty() { + return Some(format!( + "a user message at [{i}] lands between an assistant's tool_use and its \ + tool_result — {pending:?} still unanswered" + )); + } + } + } + } + None } impl MockProvider { pub fn new(responses: Vec) -> Self { Self { responses: std::sync::Mutex::new(responses), + validate_transcript: true, } } + /// Accept message sequences a real provider would reject. + /// + /// For tests that construct a malformed history *on purpose* — orphan + /// handling, compaction edge cases, recovery paths. Say why at the call + /// site; the default exists because the alternative let a real bug through. + pub fn without_transcript_validation(mut self) -> Self { + self.validate_transcript = false; + self + } + /// Convenience: provider that always returns the same text pub fn text(text: impl Into) -> Self { Self::new(vec![MockResponse::Text(text.into())]) @@ -61,10 +141,27 @@ impl MockProvider { impl StreamProvider for MockProvider { async fn stream( &self, - _config: StreamConfig, + config: StreamConfig, tx: mpsc::UnboundedSender, cancel: tokio_util::sync::CancellationToken, ) -> Result { + // A mock that accepts anything tests the loop against a provider that + // does not exist. Panic rather than return an error: a malformed + // transcript is a defect in the code under test, and the loop is + // designed to survive provider *errors*, so returning one would be + // swallowed by the very retry path that should never see this. + if self.validate_transcript { + if let Some(why) = transcript_violation(&config.messages) { + panic!( + "MockProvider received a transcript a real provider would reject: {why}.\n\n\ + This is the shape that poisons an agent — the history is kept, so every \ + later prompt fails too. If the malformed sequence is deliberate, use \ + `MockProvider::without_transcript_validation()` and say why.\n\n\ + Messages: {:#?}", + config.messages + ); + } + } let response = { let mut responses = self.responses.lock().unwrap(); if responses.is_empty() { diff --git a/tests/agent_loop_test.rs b/tests/agent_loop_test.rs index b678b52..b816241 100644 --- a/tests/agent_loop_test.rs +++ b/tests/agent_loop_test.rs @@ -230,6 +230,22 @@ async fn test_continue_from_tool_result() { system_prompt: "test".into(), messages: vec![ AgentMessage::Llm(Message::user("do something")), + // The assistant turn that *made* the call. Without it the + // tool_result answers nothing, which is a history no provider + // accepts — and which therefore cannot arise in production, so + // asserting resume behaviour on it proved nothing about the real + // path. MockProvider now rejects it. + AgentMessage::Llm(Message::assistant( + vec![Content::tool_call( + "tc-1", + "test_tool", + serde_json::json!({}), + )], + StopReason::ToolUse, + "mock", + "mock", + Usage::default(), + )), AgentMessage::Llm(Message::ToolResult { tool_call_id: "tc-1".into(), tool_name: "test_tool".into(),