fix: preserve conversation history and improve worker retrigger reliability

src/agent/channel.rs

@@ -35,6 +35,11 @@ const RETRIGGER_DEBOUNCE_MS: u64 = 500;
 /// infinite retrigger cascades where each retrigger spawns more work.
 const MAX_RETRIGGERS_PER_TURN: usize = 3;
 
+/// Max LLM turns for retrigger relay. Retriggers are simple relay tasks —
+/// the LLM just needs to call the reply tool once. A low cap avoids wasting
+/// tokens on retries when the model struggles with the retrigger format.
+const RETRIGGER_MAX_TURNS: usize = 3;
+
 #[derive(Debug, Clone)]
 enum TemporalTimezone {
     Named { timezone_name: String, timezone: Tz },
@@ -969,25 +974,63 @@ impl Channel {
         self.handle_agent_result(result, &skip_flag, &replied_flag, is_retrigger)
             .await;
 
-        // After a successful retrigger relay, inject a compact record into
-        // history so the conversation has context about what was relayed.
-        // The retrigger turn itself is rolled back by apply_history_after_turn
-        // (PromptCancelled leaves dangling tool calls), so without this the
-        // LLM would have no memory of the background result on subsequent turns.
-        if is_retrigger && replied_flag.load(std::sync::atomic::Ordering::Relaxed) {
+        // After a retrigger turn, replace the synthetic bridge message in history
+        // with a compact summary so the conversation has context about what
+        // happened. The retrigger turn itself is rolled back by
+        // apply_history_after_turn (PromptCancelled leaves dangling tool calls),
+        // so without this the LLM would have no memory of the background result
+        // on subsequent turns.
+        //
+        // This runs on both success and failure paths: if the LLM successfully
+        // relayed (replied_flag=true), it records what was sent. If the LLM
+        // failed (CompletionError, empty response, etc.), it preserves the result
+        // so it's not silently lost — the next real user turn will see it.
+        if is_retrigger {
+            let replied = replied_flag.load(std::sync::atomic::Ordering::Relaxed);
             // Extract the result summaries from the metadata we attached in
             // flush_pending_retrigger, so we record only the substance (not
             // the retrigger instructions/template scaffolding).
             let summary = message
                 .metadata
                 .get("retrigger_result_summary")
                 .and_then(|v| v.as_str())
-                .unwrap_or("[background work completed and result relayed to user]");
+                .unwrap_or("[background work completed]");
+
+            let record = if replied {
+                summary.to_string()
+            } else {
+                tracing::warn!(
+                    channel_id = %self.id,
+                    "retrigger relay failed, preserving result in history for next turn"
+                );
+                format!(
+                    "[background work completed but relay to user failed — include this in your next response]\n{summary}"
+                )
+            };
 
             let mut history = self.state.history.write().await;
+            // Replace the synthetic bridge message (if present) with the summary
+            // to avoid consecutive assistant messages in history.
+            let replaced = if history.last().is_some_and(|m| match m {
+                rig::message::Message::Assistant { content, .. } => content.iter().any(|c| {
+                    matches!(c, rig::message::AssistantContent::Text(t) if t.text.contains("[acknowledged"))
+                }),
+                _ => false,
+            }) {
+                history.pop();
+                true
+            } else {
+                false
+            };
+            tracing::debug!(
+                channel_id = %self.id,
+                replaced_bridge = replaced,
+                replied,
+                "injecting retrigger summary into history"
+            );
             history.push(rig::message::Message::Assistant {
                 id: None,
-                content: OneOrMany::one(rig::message::AssistantContent::text(summary)),
+                content: OneOrMany::one(rig::message::AssistantContent::text(record)),
             });
         }
 
@@ -1195,7 +1238,11 @@ impl Channel {
 
         let rc = &self.deps.runtime_config;
         let routing = rc.routing.load();
-        let max_turns = **rc.max_turns.load();
+        let max_turns = if is_retrigger {
+            RETRIGGER_MAX_TURNS
+        } else {
+            **rc.max_turns.load()
+        };
         let model_name = routing.resolve(ProcessType::Channel, None);
         let model = SpacebotModel::make(&self.deps.llm_manager, model_name)
             .with_context(&*self.deps.agent_id, "channel")
@@ -1222,6 +1269,29 @@ impl Channel {
             drop(history);
         }
 
+        // For retrigger turns, inject a synthetic assistant acknowledgment so the
+        // LLM sees proper user/assistant role alternation. Without this, the API
+        // receives back-to-back user messages (the original user prompt preserved
+        // from the prior turn + the retrigger system message), which causes some
+        // models to return empty responses or get confused about whose turn it is.
+        if is_retrigger {
+            let mut history = self.state.history.write().await;
+            // Only inject if the last message is a user message (avoid double-stacking
+            // if history already ends with an assistant message).
+            let needs_bridge = history
+                .last()
+                .is_some_and(|m| matches!(m, rig::message::Message::User { .. }));
+            if needs_bridge {
+                history.push(rig::message::Message::Assistant {
+                    id: None,
+                    content: OneOrMany::one(rig::message::AssistantContent::text(
+                        "[acknowledged — working on it in background]",
+                    )),
+                });
+            }
+            drop(history);
+        }
+
         // Clone history out so the write lock is released before the agentic loop.
         // The branch tool needs a read lock on history to clone it for the branch,
         // and holding a write lock across the entire agentic loop would deadlock.
@@ -3024,9 +3094,12 @@ fn apply_history_after_turn(
         }
         Err(rig::completion::PromptError::PromptCancelled { .. }) => {
             // Rig appended the user prompt and possibly an assistant tool-call
-            // message to history before cancellation. We keep only the first
-            // user text message (the actual user prompt) and discard everything else
-            // (assistant tool-calls without results, tool-result user messages).
+            // message to history before cancellation. We preserve:
+            // 1. The first user text message (the actual user prompt)
+            // 2. A clean assistant text message extracted from the reply tool call
+            //
+            // We discard: dangling tool calls (without results), tool-result user
+            // messages, and internal correction prompts.
             //
             // Exception: retrigger turns. The "user prompt" Rig pushed is actually
             // the synthetic system retrigger message (internal template scaffolding),
@@ -3042,18 +3115,34 @@ fn apply_history_after_turn(
             }
             let new_messages = &history[history_len_before..];
             let mut preserved = 0usize;
+
+            // Preserve the user text message
             if let Some(message) = new_messages.iter().find(|m| is_user_text_message(m)) {
                 guard.push(message.clone());
-                preserved = 1;
+                preserved += 1;
+            }
+
+            // Extract and preserve the reply content from the assistant's tool call.
+            // The assistant message contains ToolCall(reply, {content: "..."}), but
+            // we can't store the tool call without its result (it would poison future
+            // turns). Instead, extract the content and push a clean text-only message.
+            if let Some(reply_content) = extract_reply_content_from_cancelled_history(new_messages)
+            {
+                guard.push(rig::message::Message::Assistant {
+                    id: None,
+                    content: rig::OneOrMany::one(rig::message::AssistantContent::text(
+                        reply_content,
+                    )),
+                });
+                preserved += 1;
             }
-            // Skip: Assistant messages (contain tool calls without results),
-            // user ToolResult messages, and internal correction prompts.
+
             tracing::debug!(
                 channel_id = %channel_id,
                 total_new = new_messages.len(),
                 preserved,
                 discarded = new_messages.len() - preserved,
-                "selectively preserved first user message after PromptCancelled"
+                "preserved user message and assistant reply after PromptCancelled"
             );
         }
         Err(_) => {
@@ -3068,6 +3157,35 @@ fn apply_history_after_turn(
     }
 }
 
+/// Extract reply content from a cancelled turn's assistant message.
+///
+/// When the reply tool fires, Rig's history contains an Assistant message with
+/// a ToolCall for `reply` with args like `{"content": "Hey there!"}`. This
+/// extracts that content string so we can inject a clean text-only assistant
+/// message into history (the tool call itself can't be preserved since it has
+/// no matching result).
+fn extract_reply_content_from_cancelled_history(
+    new_messages: &[rig::message::Message],
+) -> Option<String> {
+    for message in new_messages {
+        if let rig::message::Message::Assistant { content, .. } = message {
+            for item in content.iter() {
+                if let rig::message::AssistantContent::ToolCall(tool_call) = item {
+                    if tool_call.function.name == "reply" {
+                        // Extract the "content" field from the reply tool args
+                        if let Some(content_value) = tool_call.function.arguments.get("content") {
+                            if let Some(text) = content_value.as_str() {
+                                return Some(text.to_string());
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+    None
+}
+
 /// Returns true if a message is a User message containing only text content
 /// (i.e., an actual user prompt, not a tool result).
 fn is_user_text_message(message: &rig::message::Message) -> bool {
@@ -3149,16 +3267,23 @@ mod tests {
         assert_eq!(guard, history);
     }
 
-    /// PromptCancelled preserves user text messages but discards assistant
-    /// tool-call messages (which have no matching tool results).
+    /// PromptCancelled preserves user text messages and extracts reply content
+    /// from the assistant's tool call, discarding the dangling tool call itself.
     #[test]
-    fn prompt_cancelled_preserves_user_prompt() {
+    fn prompt_cancelled_preserves_user_and_reply() {
         let initial = make_history(&["hello", "thinking..."]);
         let mut guard = initial.clone();
-        // Simulate what Rig does: push user prompt + assistant tool-call
+        // Simulate what Rig does: push user prompt + assistant reply tool-call
         let mut history = initial.clone();
         history.push(user_msg("new user prompt")); // should be preserved
-        history.push(assistant_msg("tool call without result")); // should be discarded
+        history.push(Message::Assistant {
+            id: None,
+            content: rig::OneOrMany::one(rig::message::AssistantContent::tool_call(
+                "call_1",
+                "reply",
+                serde_json::json!({"content": "Hey there!"}),
+            )),
+        });
         let len_before = initial.len();
 
         let err = Err(PromptError::PromptCancelled {
@@ -3168,18 +3293,19 @@ mod tests {
 
         apply_history_after_turn(&err, &mut guard, history, len_before, "test", false);
 
-        // User prompt should be preserved, assistant tool-call discarded
+        // User prompt and reply content should be preserved, tool-call structure discarded
         let mut expected = initial;
         expected.push(user_msg("new user prompt"));
+        expected.push(assistant_msg("Hey there!"));
         assert_eq!(
             guard, expected,
-            "user text messages should be preserved, assistant messages discarded"
+            "user text message and reply content should be preserved"
         );
     }
 
-    /// PromptCancelled discards tool-result User messages (ToolResult content).
+    /// PromptCancelled extracts reply content and discards tool-result User messages.
     #[test]
-    fn prompt_cancelled_discards_tool_results() {
+    fn prompt_cancelled_extracts_reply_discards_tool_results() {
         let initial = make_history(&["hello", "thinking..."]);
         let mut guard = initial.clone();
         let mut history = initial.clone();
@@ -3215,9 +3341,10 @@ mod tests {
 
         let mut expected = initial;
         expected.push(user_msg("new user prompt"));
+        expected.push(assistant_msg("hello"));
         assert_eq!(
             guard, expected,
-            "tool-call and tool-result messages should be discarded"
+            "reply content should be extracted, tool-result messages discarded"
         );
     }
 
@@ -3230,8 +3357,15 @@ mod tests {
         let mut history = initial.clone();
         history.push(user_msg("real user prompt")); // preserved
         history.push(assistant_msg("bad tool syntax"));
-        history.push(user_msg("Please proceed and use the available tools.")); // dropped
-        history.push(assistant_msg("tool call without result"));
+        history.push(user_msg("Please proceed and use the available tools.")); // dropped (correction)
+        history.push(Message::Assistant {
+            id: None,
+            content: rig::OneOrMany::one(rig::message::AssistantContent::tool_call(
+                "call_1",
+                "reply",
+                serde_json::json!({"content": "Got it!"}),
+            )),
+        });
         let len_before = initial.len();
 
         let err = Err(PromptError::PromptCancelled {
@@ -3243,9 +3377,10 @@ mod tests {
 
         let mut expected = initial;
         expected.push(user_msg("real user prompt"));
+        expected.push(assistant_msg("Got it!"));
         assert_eq!(
             guard, expected,
-            "only the first user prompt should be preserved"
+            "only the first user prompt and final reply should be preserved"
         );
     }
 
@@ -3390,15 +3525,19 @@ mod tests {
             false,
         );
 
-        // User prompt preserved, assistant tool-call discarded
+        // User prompt and reply content preserved, tool-call structure discarded
         assert_eq!(
             guard.len(),
-            initial.len() + 1,
-            "user prompt should be preserved"
+            initial.len() + 2,
+            "user prompt and assistant reply should be preserved"
+        );
+        assert!(
+            matches!(&guard[guard.len() - 2], Message::User { .. }),
+            "second-to-last message should be the preserved user prompt"
         );
         assert!(
-            matches!(&guard[guard.len() - 1], Message::User { .. }),
-            "last message should be the preserved user prompt"
+            matches!(&guard[guard.len() - 1], Message::Assistant { .. }),
+            "last message should be the extracted reply content"
         );
 
         // Second turn: new user message appended, successful response