From 0845befb130aae0ac248a7f1f07be8ced9415429 Mon Sep 17 00:00:00 2001 From: Yuanhao Li Date: Tue, 25 Aug 2026 00:01:26 +0200 Subject: [PATCH] test: MockProvider rejects transcripts a real provider would MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It discarded StreamConfig entirely, so 600+ tests drove the agent loop without one of them noticing when it built a message sequence every provider rejects. That is not a coverage gap more tests would close — it is structural, and it is exactly how loop detection reached a release branch injecting a steering message between an assistant's tool_use blocks and their tool_results. A history that shape poisons the agent: it is kept, so every later prompt fails too. The mock now checks the one invariant every provider enforces — a tool_use is answered by its tool_results before anything else intervenes — and panics with the reason a provider would give. Panics rather than returns Err on purpose: a malformed transcript is a defect in the code under test, and returning a provider error would be swallowed by the very retry path that should never see it. Opt out with MockProvider::without_transcript_validation() for tests that build a malformed history deliberately. Nothing needed it. Effect, measured by reverting the 0.18 steer fix: 5 tests now fail with the precise diagnostic, where the whole suite previously stayed green. 600+ existing tests became transcript guards without writing one. One real fixture bug found: test_continue_from_tool_result resumed from [user, tool_result] — a tool result answering nothing, which no provider accepts and which therefore cannot arise in production, so the test proved nothing about the path it named. It now includes the assistant turn that made the call. 629 passed, clippy clean under -Dwarnings. --- src/provider/mock.rs | 99 +++++++++++++++++++++++++++++++++++++++- tests/agent_loop_test.rs | 16 +++++++ 2 files changed, 114 insertions(+), 1 deletion(-) 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(),