Skip to content

Commit 982ba0b

Browse files
jithinABclaude
andcommitted
fix(llm/flow/replay): review wf_6783a4b3 — CRITICAL history poisoning + 9 majors
CRITICAL: max_rounds bail in BOTH tool loops (B-G4 run_tool_loop, B-G3 run_flow_loop) left the already-recorded assistant{tool_calls} unpaired — every later request 400s on strict providers, permanently bricking the session (the backstop fired exactly when models loop). Both loops now write synthetic terminal results ("aborted: max rounds") for the final batch before stopping; pinned with a history-pairing audit in the capped-loop test. - ConversationHistory trim is pair-aware: evictions never leave a leading orphan tool result (the same 400-forever shape) — pinned. - Tool-result batches append under ONE history-lock acquisition (add_tool_results_batch): a concurrent turn's user message can no longer interleave inside a batch (persisted pairing corruption). - B-G5 double-commit closed: once the initial inference completes (recorded with its preamble), the streamed accumulator is cleared before the tool loop — a cancel inside the loop no longer commits the preamble twice. - spoke-flag fix: a turn that streamed a round-0 preamble then ran the tool loop now SPEAKS the loop's final answer (was: bot permanently silent after "let me check..."); pinned end-to-end through the orchestrator with a preamble+tool_calls SSE mock. - Flows: stale pending_transition from an aborted turn is discarded at the next turn's start (state machine no longer jumps nodes uncommanded); initialize() routes entry-inference tool calls through the flow loop (lookup-then-greet nodes work; no dangling tool_calls from turn one); flow MAX_ROUNDS bail pairs terminal results (same critical shape); persona is tracked EXPLICITLY (seeded from the client's configured system_prompt, updated by role_message) instead of inferred from history[0] — embedders wiring flows from the standard config keep their system instruction. - D-G1: clear() now keeps the most recent ~250ms onset window (a final never covers audio still in flight — clear-all lost the next utterance's first syllables when the socket died before the next final); clear_all() added for teardown; chaos choreography re-pinned (pre-window audio never replays, onset survivor may, tail byte-identical in order). - Cheap hardening from the unverified tail: orchestrator tool-loop guard requires a NON-EMPTY registry; continue_from_history injects the system prompt on an empty session instead of sending an empty messages array; LiveKit forwarder warn properly rate-limited (1 per 100 sheds) and its doc now matches the newest-drop behavior it implements. Floor: 6094/0 lib (all features) + chaos 6/6 + conversation_loop 10/10 + flow_manager 7/7 + clippy clean. Live: Sarvam tool loop, 3-node intake flow, Deepgram nova-3 STT roundtrip all green post-fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 0ddf8a8 commit 982ba0b

9 files changed

Lines changed: 557 additions & 54 deletions

File tree

gateway/src/core/conversation/mod.rs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,10 +451,23 @@ impl ConversationOrchestrator {
451451
// content lands on the normal speak path below (the streaming pump
452452
// saw no text for a tool-call response, so the !spoke fallback
453453
// speaks it).
454+
let mut ran_tool_loop = false;
454455
let result = match result {
455456
Ok(response)
456-
if !response.tool_calls.is_empty() && self.llm.functions().is_some() =>
457+
if !response.tool_calls.is_empty()
458+
&& self.llm.functions().is_some_and(|r| !r.is_empty()) =>
457459
{
460+
// The initial inference COMPLETED (recorded with its
461+
// preamble): the streamed accumulator is spent — without
462+
// this, a cancel inside the tool loop would commit the
463+
// preamble a SECOND time after the tool results (review
464+
// wf_6783a4b3: duplicate assistant text).
465+
streamed_text.lock().clear();
466+
// The pump spoke (at most) round-0 preamble; the loop's
467+
// FINAL answer must still reach TTS through the fallback
468+
// speak below (review wf_6783a4b3: a true `spoke` here left
469+
// the bot permanently silent after "let me check...").
470+
ran_tool_loop = true;
458471
let registry =
459472
Arc::clone(self.llm.functions().expect("guarded by condition"));
460473
crate::core::llm::run_tool_loop(
@@ -485,7 +498,9 @@ impl ConversationOrchestrator {
485498
// deltas and deliver the answer (or nothing) at the end. If
486499
// the pump spoke nothing, fall back to the final content; if
487500
// that is empty too, say so loudly instead of going silent.
488-
!spoke.load(std::sync::atomic::Ordering::Relaxed)
501+
// A completed TOOL LOOP always speaks its final answer —
502+
// the pump only ever saw the round-0 preamble.
503+
ran_tool_loop || !spoke.load(std::sync::atomic::Ordering::Relaxed)
489504
} else {
490505
true
491506
};

gateway/src/core/flow/mod.rs

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,11 @@ pub struct FlowManager {
147147
/// Available at every node, alongside the node's own functions.
148148
global_functions: Vec<FlowFunctionSchema>,
149149
action_sink: Option<ActionSink>,
150+
/// The session PERSONA — tracked explicitly (review wf_6783a4b3: the
151+
/// "system is history[0]" inference broke when embedders seeded flows
152+
/// from the standard config). Seeded from the LlmClient's configured
153+
/// system prompt; updated by every node `role_message`.
154+
persona: Mutex<Option<String>>,
150155
}
151156

152157
impl std::fmt::Debug for FlowManager {
@@ -165,6 +170,7 @@ impl FlowManager {
165170
session_id: impl Into<String>,
166171
api_key: Option<String>,
167172
) -> Self {
173+
let persona = llm.config().system_prompt.clone();
168174
Self {
169175
llm,
170176
registry,
@@ -174,6 +180,7 @@ impl FlowManager {
174180
pending_transition: Arc::new(Mutex::new(None)),
175181
global_functions: Vec::new(),
176182
action_sink: None,
183+
persona: Mutex::new(persona),
177184
}
178185
}
179186

@@ -200,7 +207,14 @@ impl FlowManager {
200207
first: NodeConfig,
201208
cancel: &CancellationToken,
202209
) -> LlmResult<Option<LlmResponse>> {
203-
self.set_node(first, cancel).await
210+
match self.set_node(first, cancel).await? {
211+
// The entry inference may itself call tools (lookup-then-greet
212+
// nodes are core pipecat-flows usage): run the flow loop, or
213+
// the calls are silently dropped AND the dangling
214+
// assistant{tool_calls} poisons history (review wf_6783a4b3).
215+
Some(response) => self.run_flow_loop(response, cancel).await,
216+
None => Ok(None),
217+
}
204218
}
205219

206220
/// Drive one USER turn through the flow: inference on the current
@@ -214,6 +228,15 @@ impl FlowManager {
214228
transcript: &str,
215229
cancel: &CancellationToken,
216230
) -> LlmResult<Option<LlmResponse>> {
231+
// A transition staged by an aborted/dropped earlier turn must never
232+
// fire on THIS unrelated turn (review wf_6783a4b3: pending leak).
233+
if let Some(stale) = self.pending_transition.lock().take() {
234+
warn!(
235+
session = %self.session_id,
236+
node = ?stale.name,
237+
"discarding stale pending transition from an aborted turn"
238+
);
239+
}
217240
let response = self
218241
.llm
219242
.complete(&self.session_id, transcript, self.api_key.as_deref(), cancel, None)
@@ -235,7 +258,24 @@ impl FlowManager {
235258
return Ok(Some(response));
236259
}
237260
if rounds >= MAX_ROUNDS {
238-
warn!(session = %self.session_id, "flow loop hit max rounds");
261+
// Same CRITICAL shape as B-G4's loop: bailing with the
262+
// assistant{tool_calls} already recorded but unanswered
263+
// bricks the session on strict providers. Pair them with
264+
// terminal results first (review wf_6783a4b3).
265+
warn!(session = %self.session_id, "flow loop hit max rounds; writing terminal results");
266+
let results: Vec<(String, String)> = response
267+
.tool_calls
268+
.iter()
269+
.map(|c| {
270+
(
271+
c.id.clone(),
272+
serde_json::json!({"error": "aborted: flow loop reached max rounds"})
273+
.to_string(),
274+
)
275+
})
276+
.collect();
277+
self.llm.add_tool_results_batch(&self.session_id, results).await;
278+
response.tool_calls.clear();
239279
return Ok(Some(response));
240280
}
241281
rounds += 1;
@@ -246,11 +286,12 @@ impl FlowManager {
246286
// are captured into pending_transition by the wrappers.
247287
let results =
248288
execute_batch(&self.registry, &self.session_id, &batch, cancel, true).await;
249-
for (call, value) in batch.iter().zip(results) {
250-
self.llm
251-
.add_tool_result(&self.session_id, &call.id, &value.to_string())
252-
.await;
253-
}
289+
let rendered: Vec<(String, String)> = batch
290+
.iter()
291+
.zip(results)
292+
.map(|(call, value)| (call.id.clone(), value.to_string()))
293+
.collect();
294+
self.llm.add_tool_results_batch(&self.session_id, rendered).await;
254295

255296
// TWO-PHASE: results are in context; NOW the transition fires.
256297
let next = self.pending_transition.lock().take();
@@ -314,9 +355,14 @@ impl FlowManager {
314355
}
315356
self.registry.swap_tools(items);
316357

317-
// Persona persists until another node changes it.
358+
// Persona persists until another node changes it (tracked
359+
// explicitly — never inferred from history position).
318360
if let Some(role) = &node.role_message {
319-
self.llm.upsert_system(&self.session_id, role).await;
361+
*self.persona.lock() = Some(role.clone());
362+
}
363+
let persona = self.persona.lock().clone();
364+
if let Some(p) = &persona {
365+
self.llm.upsert_system(&self.session_id, p).await;
320366
}
321367

322368
// Context strategy.
@@ -334,20 +380,9 @@ impl FlowManager {
334380
"ResetWithSummary: native summarizer (B-G6) not built; behaving as Reset"
335381
);
336382
}
337-
// Rebuild: persona system message (the node's, else the one
338-
// already in context) + the node's task messages.
339-
let system = match &node.role_message {
340-
Some(r) => Some(r.clone()),
341-
None => self
342-
.llm
343-
.history_snapshot(&self.session_id)
344-
.await
345-
.first()
346-
.filter(|m| m.role == crate::core::llm::MessageRole::System)
347-
.and_then(|m| m.content.clone()),
348-
};
383+
// Rebuild: the tracked persona + the node's task messages.
349384
let mut messages = Vec::new();
350-
if let Some(system) = system {
385+
if let Some(system) = &persona {
351386
messages.push(ChatMessage::system(system));
352387
}
353388
messages.extend(node.task_messages.clone());

gateway/src/core/llm/functions.rs

Lines changed: 102 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -281,10 +281,27 @@ pub async fn run_tool_loop(
281281
let mut rounds = 0usize;
282282
while !response.tool_calls.is_empty() {
283283
if rounds >= opts.max_rounds {
284+
// CRITICAL (review wf_6783a4b3): the assistant{tool_calls}
285+
// message is ALREADY in history — bailing without results leaves
286+
// it unpaired and every later request 400s on strict providers
287+
// (session permanently bricked). Write synthetic terminal
288+
// results so pairing holds, then stop.
284289
warn!(
285290
session = session_id,
286-
rounds, "tool loop hit max_rounds; returning last response"
291+
rounds, "tool loop hit max_rounds; writing terminal results and stopping"
287292
);
293+
let results: Vec<(String, String)> = response
294+
.tool_calls
295+
.iter()
296+
.map(|c| {
297+
(
298+
c.id.clone(),
299+
json!({"error": "aborted: tool loop reached max_rounds"}).to_string(),
300+
)
301+
})
302+
.collect();
303+
llm.add_tool_results_batch(session_id, results).await;
304+
response.tool_calls.clear();
288305
break;
289306
}
290307
rounds += 1;
@@ -299,11 +316,16 @@ pub async fn run_tool_loop(
299316

300317
let results = execute_batch(registry, session_id, &batch, cancel, opts.parallel).await;
301318

302-
// Pairing invariant: exactly one result per tool_call_id, batch order.
303-
for (call, value) in batch.iter().zip(results) {
304-
let rendered = value.to_string();
305-
llm.add_tool_result(session_id, &call.id, &rendered).await;
306-
}
319+
// Pairing invariant: exactly one result per tool_call_id, batch
320+
// order — appended under ONE history-lock acquisition so a
321+
// concurrent turn's user message can never interleave between a
322+
// batch's results (review wf_6783a4b3).
323+
let rendered: Vec<(String, String)> = batch
324+
.iter()
325+
.zip(results)
326+
.map(|(call, value)| (call.id.clone(), value.to_string()))
327+
.collect();
328+
llm.add_tool_results_batch(session_id, rendered).await;
307329

308330
// ONE re-inference for the whole batch (group_id semantics).
309331
response = llm
@@ -838,9 +860,81 @@ mod tests {
838860
let opts = ToolLoopOptions { parallel: true, max_rounds: 3 };
839861
let resp = run_tool_loop(&llm, &reg, "s1", first, None, &token, opts).await.unwrap();
840862
assert!(
841-
!resp.tool_calls.is_empty(),
842-
"loop returns the last tool-call response when capped"
863+
resp.tool_calls.is_empty(),
864+
"capped loop must not hand back live tool calls (they were terminally answered)"
843865
);
844866
assert_eq!(mock.requests.lock().len(), 1 + 3, "initial + max_rounds inferences");
867+
868+
// CRITICAL pairing pin (review wf_6783a4b3): the bail must leave NO
869+
// unpaired assistant{tool_calls} — every call id in history has a
870+
// tool result, so the session is NOT bricked on strict providers.
871+
let history = llm.history_snapshot("s1").await;
872+
let result_ids: std::collections::HashSet<&str> = history
873+
.iter()
874+
.filter(|m| matches!(m.role, MessageRole::Tool))
875+
.filter_map(|m| m.tool_call_id.as_deref())
876+
.collect();
877+
for m in &history {
878+
if let Some(calls) = &m.tool_calls {
879+
for c in calls {
880+
assert!(
881+
result_ids.contains(c.id.as_str()),
882+
"unpaired tool call {} after max_rounds bail: {history:?}",
883+
c.id
884+
);
885+
}
886+
}
887+
}
888+
let last_results: Vec<_> = history
889+
.iter()
890+
.rev()
891+
.take_while(|m| matches!(m.role, MessageRole::Tool))
892+
.collect();
893+
assert_eq!(last_results.len(), 2, "the final batch got terminal results");
894+
assert!(
895+
last_results[0]
896+
.content
897+
.as_deref()
898+
.unwrap()
899+
.contains("max_rounds"),
900+
"terminal results say why"
901+
);
902+
}
903+
904+
#[test]
905+
fn history_trim_never_orphans_tool_messages() {
906+
// review wf_6783a4b3: count-based eviction crossing a tool exchange
907+
// must take the whole exchange, never leaving a leading orphan tool
908+
// result (strict providers 400 on it forever).
909+
use crate::core::llm::{ChatMessage, ConversationHistory, MessageRole};
910+
let mut h = ConversationHistory::new(4);
911+
h.add(ChatMessage::system("p"));
912+
h.add(ChatMessage::user("q1"));
913+
let mut call = ChatMessage::assistant("");
914+
call.tool_calls = Some(vec![crate::core::llm::ToolCall {
915+
id: "c1".into(),
916+
call_type: "function".into(),
917+
function: crate::core::llm::FunctionCall { name: "f".into(), arguments: "{}".into() },
918+
}]);
919+
h.add(call);
920+
h.add(ChatMessage::tool("c1", "result"));
921+
// Over capacity: evictions start. q1 goes, then the assistant{call}
922+
// — its orphaned result MUST go with it.
923+
h.add(ChatMessage::user("q2"));
924+
h.add(ChatMessage::assistant("a2"));
925+
let msgs = h.messages();
926+
assert!(
927+
!msgs
928+
.iter()
929+
.enumerate()
930+
.any(|(i, m)| m.role == MessageRole::Tool
931+
&& !msgs[..i].iter().any(|p| p
932+
.tool_calls
933+
.as_ref()
934+
.is_some_and(|cs| cs.iter().any(|c| Some(c.id.as_str())
935+
== m.tool_call_id.as_deref())))),
936+
"orphan tool message survived the trim: {msgs:?}"
937+
);
938+
assert_eq!(msgs[0].role, MessageRole::System, "system always survives");
845939
}
846940
}

gateway/src/core/llm/mod.rs

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,16 @@ impl ConversationHistory {
524524
.position(|m| m.role != MessageRole::System)
525525
{
526526
self.messages.remove(idx);
527+
// Pair-aware eviction (review wf_6783a4b3): the oldest
528+
// non-system slot must never be left on a tool RESULT whose
529+
// call was just (or previously) evicted — strict providers
530+
// 400 on orphan tool messages. Any tool message now at the
531+
// front is by definition an orphan: drop it with its call.
532+
while idx < self.messages.len()
533+
&& self.messages[idx].role == MessageRole::Tool
534+
{
535+
self.messages.remove(idx);
536+
}
527537
} else {
528538
break;
529539
}
@@ -755,8 +765,16 @@ impl LlmClient {
755765
.or_insert_with(|| ConversationHistory::new(self.config.max_history));
756766

757767
// CONTINUE mode (B-G4 re-inference after tool results): the request
758-
// is the stored history exactly as-is — no new user message.
768+
// is the stored history exactly as-is — no new user message. An
769+
// EMPTY history would render an empty messages array (provider
770+
// 400): inject the configured system prompt so the request is at
771+
// least well-formed (review wf_6783a4b3, unverified #25).
759772
let Some(input) = input else {
773+
if history.messages().is_empty()
774+
&& let Some(system_prompt) = &self.config.system_prompt
775+
{
776+
history.add(ChatMessage::system(system_prompt));
777+
}
760778
return history.messages().to_vec();
761779
};
762780

@@ -1191,6 +1209,22 @@ impl LlmClient {
11911209
}
11921210
}
11931211

1212+
/// Add a BATCH of tool results under one lock acquisition: a concurrent
1213+
/// turn's user message must never interleave between a batch's results
1214+
/// (pairing violation → strict providers 400 forever).
1215+
pub async fn add_tool_results_batch(
1216+
&self,
1217+
session_id: &str,
1218+
results: Vec<(String, String)>,
1219+
) {
1220+
let mut histories = self.histories.write().await;
1221+
if let Some(history) = histories.get_mut(session_id) {
1222+
for (id, result) in results {
1223+
history.add(ChatMessage::tool(id, result));
1224+
}
1225+
}
1226+
}
1227+
11941228
/// Append messages to a session's context (B-G3 flows: APPEND strategy /
11951229
/// task messages). Creates the session entry if missing.
11961230
pub async fn append_context(&self, session_id: &str, messages: Vec<ChatMessage>) {

0 commit comments

Comments
 (0)