diff --git a/crates/openhuman-core/src/agent/harness/session/turn/core.rs b/crates/openhuman-core/src/agent/harness/session/turn/core.rs index 0fb99a5674..b1e3119cd9 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn/core.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn/core.rs @@ -15,6 +15,35 @@ use crate::memory::MemoryCategory; use anyhow::Result; use std::hash::{Hash, Hasher}; +/// The outcome captured for one assistant tool call, matched by call id, tool +/// name **and occurrence**. +/// +/// Prompt-guided (XML / P-Format) calls carry no provider id, so every model +/// response synthesizes `call_0`, `call_1`, … afresh (`tinyagents::model`), and a +/// turn with several tool rounds repeats the same ids. A first-match lookup gave +/// every later round the first round's result, so a turn's records could hide its +/// latest failure (Codex review on #6289). The n-th call under an `(id, name)` +/// takes the n-th outcome recorded under it: rounds run in order and ids are +/// unique within a round, so parallel execution inside a round cannot reorder +/// them. Keying on the name as well stops an unknown-tool call, which never +/// reaches the capture sink, from consuming a later round's real outcome. +fn nth_call_outcome<'a>( + tool_outcomes: &'a [crate::agent::tinyagents::ToolCallOutcome], + seen: &mut std::collections::HashMap<(String, String), usize>, + call_id: &str, + name: &str, +) -> Option<&'a crate::agent::tinyagents::ToolCallOutcome> { + let occurrence = seen + .entry((call_id.to_string(), name.to_string())) + .or_insert(0); + let outcome = tool_outcomes + .iter() + .filter(|o| o.call_id == call_id && o.name == name) + .nth(*occurrence); + *occurrence += 1; + outcome +} + /// Flatten the assistant tool calls a turn produced into [`ToolCallRecord`]s for /// post-turn hooks + the deterministic cap checkpoint. Per-call success + /// sanitized output summary are recovered from the turn's captured @@ -26,10 +55,11 @@ fn tool_records_from_conversation( tool_outcomes: &[crate::agent::tinyagents::ToolCallOutcome], ) -> Vec { let mut records = Vec::new(); + let mut seen = std::collections::HashMap::new(); for msg in conversation { if let ConversationMessage::AssistantToolCalls { tool_calls, .. } = msg { for call in tool_calls { - let outcome = tool_outcomes.iter().find(|o| o.call_id == call.id); + let outcome = nth_call_outcome(tool_outcomes, &mut seen, &call.id, &call.name); // Default a MISSING outcome to `false` (#4467, item 7): a call // with no captured outcome is a hallucinated/unknown tool the // crate recovered via `ReturnToolError` without running @@ -69,10 +99,11 @@ fn checkpoint_results_from_conversation( tool_outcomes: &[crate::agent::tinyagents::ToolCallOutcome], ) -> Vec { let mut results = Vec::new(); + let mut seen = std::collections::HashMap::new(); for msg in conversation { if let ConversationMessage::AssistantToolCalls { tool_calls, .. } = msg { for call in tool_calls { - let outcome = tool_outcomes.iter().find(|o| o.call_id == call.id); + let outcome = nth_call_outcome(tool_outcomes, &mut seen, &call.id, &call.name); // Same missing-outcome rule as `tool_records_from_conversation`: // a call the crate recovered without running `after_tool` never // reached the capture sink, so it is reported as failed rather @@ -214,5 +245,6 @@ include!("core_turn.rs"); // calls, split by responsibility into real submodules. Their `impl Agent` // blocks complement `turn()` above. mod experience_context; +mod grounded_close; mod harness_turn; mod resumed_prefix; diff --git a/crates/openhuman-core/src/agent/harness/session/turn/core/grounded_close.rs b/crates/openhuman-core/src/agent/harness/session/turn/core/grounded_close.rs new file mode 100644 index 0000000000..e83496f0fd --- /dev/null +++ b/crates/openhuman-core/src/agent/harness/session/turn/core/grounded_close.rs @@ -0,0 +1,123 @@ +//! Close a turn that has no usable reply of its own from its tool records: a +//! tool turn that ended without final text (#4093, #6278) or a run the +//! no-progress breaker halted (#6279). + +use crate::agent::harness::session::turn_checkpoint::{self, CloseVerdict}; +use crate::agent::harness::session::types::Agent; +use crate::agent::messages::{ChatMessage, ConversationMessage}; +use crate::agent::tinyagents::TinyagentsTurnOutcome; +use crate::inference::provider::UsageInfo; + +impl Agent { + /// Write the closing message for a turn whose run produced no usable reply: + /// the loop ended on empty text after tool work (#4093), or the breaker + /// halted it (`outcome.breaker_halt`, #6279). + /// + /// Three steps, each closing a way the old path shipped a non-answer: + /// + /// 1. **Grounded wrap-up.** One tools-disabled call whose instruction + /// restates this turn's tool records, failures and their messages + /// included, plus the breaker's stop note as input. Context middleware + /// may have cleared or summarised those bodies earlier in the turn, and a + /// model that cannot see a success will say it never happened (#6278). + /// 2. **Check.** A separate call that sees only the request, the records and + /// the candidate rejects a reply that narrates intent, contradicts a + /// record, or drops the failure that explains an unfinished request. + /// 3. **Fallback.** An empty, tool-calling, rejected or unverified close + /// (the check failed or gave no verdict) is replaced by + /// [`turn_checkpoint::build_deterministic_final_summary`], which quotes + /// each result and the stop note. + /// + /// Accepted model text is streamed only after the check, so a rejected + /// reply never renders and the streamed text matches the persisted one. The + /// reply is pushed onto `history`, and the usage of every extra call is + /// returned for the caller's turn accounting. + pub(super) async fn close_turn_from_records( + &mut self, + outcome: &TinyagentsTurnOutcome, + user_message: &str, + effective_model: &str, + ) -> (String, Vec) { + let stop_reason = outcome.breaker_halt.as_deref(); + let mut usage = Vec::new(); + + // The run folded its blank terminal assistant response into history (an + // empty `Chat(assistant(""))`). Drop it before the wrap-up request and + // before the reply is appended: strict providers reject empty content, + // and the transcript must not carry a dangling blank turn. + if self + .history + .last() + .is_some_and(super::is_empty_assistant_chat) + { + self.history.pop(); + } + + let results = super::checkpoint_results_from_conversation( + &outcome.conversation, + &outcome.tool_outcomes, + ); + let records = + turn_checkpoint::render_tool_results(&results, turn_checkpoint::GROUNDING_TOTAL_CHARS); + let iteration = outcome.model_calls as u32 + 1; + + let base = self.tool_dispatcher.to_provider_messages(&self.history); + let (candidate, candidate_usage) = self + .summarize_turn_wrapup( + &base, + effective_model, + iteration, + &turn_checkpoint::final_answer_instruction(stop_reason, &records), + false, + ) + .await; + usage.extend(candidate_usage); + + let verdict = if candidate.trim().is_empty() { + None + } else { + let prompt = + turn_checkpoint::close_verification_prompt(user_message, &records, &candidate); + let (verdict_text, verdict_usage) = self + .silent_completion( + &[ChatMessage::user(prompt)], + effective_model, + "closing-message check", + ) + .await; + usage.extend(verdict_usage); + Some(turn_checkpoint::parse_close_verdict(&verdict_text)) + }; + if verdict == Some(CloseVerdict::Unclear) { + log::warn!( + "[agent_loop] closing-message check returned no ACCEPT/REJECT verdict; using the deterministic fallback" + ); + } + + // Only an explicit ACCEPT ships the model's text. A failed or malformed + // check leaves the reply unverified, and shipping unverified closing text + // is the defect this path exists to stop (CodeRabbit on #6289). + let accepted = verdict == Some(CloseVerdict::Accept); + let reply = if accepted { + self.stream_text_continuation(&candidate, iteration).await; + candidate + } else { + turn_checkpoint::build_deterministic_final_summary(&results, stop_reason) + }; + + log::info!( + "[agent_loop] closed turn from tool records after {} tool call(s): halted={} verdict={:?} fallback={} ({} chars) — #4093 #6278 #6279", + outcome.tool_calls, + stop_reason.is_some(), + verdict, + !accepted, + reply.chars().count() + ); + + self.history + .push(ConversationMessage::Chat(ChatMessage::assistant( + reply.clone(), + ))); + (reply, usage) + } +} diff --git a/crates/openhuman-core/src/agent/harness/session/turn/core/harness_turn.rs b/crates/openhuman-core/src/agent/harness/session/turn/core/harness_turn.rs index 4921821c3b..efe4a56610 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn/core/harness_turn.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn/core/harness_turn.rs @@ -273,6 +273,7 @@ impl Agent { effective_model, outcome.model_calls as u32 + 1, turn_checkpoint::MAX_ITER_CHECKPOINT_INSTRUCTION, + true, ) .await; if let Some(u) = summary_usage { @@ -328,61 +329,31 @@ impl Agent { iteration: outcome.model_calls, }, )); - } else if outcome.text.trim().is_empty() { - // #4093: the loop ran tool calls (tool_calls > 0, so the branch - // above did not fire) and then yielded a terminating response with - // no final text — the turn did work but would otherwise end - // silently, leaving the user with nothing. Enforce the - // "must produce a final response" terminal step: re-prompt the - // model (tools disabled) for a closing summary of what it did, - // falling back to a deterministic summary of the tool calls so the - // synthesized message is never itself empty. Fold the extra call's - // usage into the turn accounting, exactly like the cap path above. - let base = self.tool_dispatcher.to_provider_messages(&self.history); - let (summary, summary_usage) = self - .summarize_turn_wrapup( - &base, - effective_model, - outcome.model_calls as u32 + 1, - turn_checkpoint::FINAL_ANSWER_INSTRUCTION, - ) + } else if outcome.breaker_halt.is_some() || outcome.text.trim().is_empty() { + // Two ways a turn reaches here without a reply it can ship: + // + // * #4093: the loop ran tool calls (tool_calls > 0, so the branch + // above did not fire) and then ended with no final text — the + // turn did work but would otherwise end silently. + // * #6279: the no-progress breaker halted the run. `outcome.text` + // then holds the breaker's stop note, which is worded for a model + // ("Report this back instead of retrying") and must not be the + // user's reply. + // + // Both close from the turn's tool records: a grounded wrap-up, a + // check that rejects intent narration or claims the records + // contradict (#6278), and a deterministic fallback that quotes the + // failures. Fold the extra calls' usage into the turn accounting, + // exactly like the cap path above. + let (final_answer, close_usage) = self + .close_turn_from_records(&outcome, user_message, effective_model) .await; - if let Some(u) = summary_usage { + for u in close_usage { input_tokens += u.input_tokens; output_tokens += u.output_tokens; cached_input_tokens += u.cached_input_tokens; charged_amount_usd += u.charged_amount_usd; } - let final_answer = if summary.trim().is_empty() { - turn_checkpoint::build_deterministic_final_summary(&tool_records_from_conversation( - &outcome.conversation, - &outcome.tool_outcomes, - )) - } else { - summary - }; - log::info!( - "[agent_loop] turn produced no final text after {} tool call(s); synthesized a closing summary ({} chars) — #4093", - outcome.tool_calls, - final_answer.chars().count() - ); - // The empty terminal assistant response was already folded into - // `self.history` via `outcome.conversation` above (an empty - // `Chat(assistant(""))` — see `messages_to_conversation`). Drop that - // blank turn before appending the synthesized answer so the - // transcript and the next prompt don't carry a dangling empty - // assistant message immediately before the real reply (Codex review). - if matches!( - self.history.last(), - Some(ConversationMessage::Chat(msg)) - if msg.role == "assistant" && msg.content.trim().is_empty() - ) { - self.history.pop(); - } - self.history - .push(ConversationMessage::Chat(ChatMessage::assistant( - final_answer.clone(), - ))); final_answer } else if outcome.early_exit_tool.is_some() { // Paused on `ask_user_clarification`. The run stopped right after the diff --git a/crates/openhuman-core/src/agent/harness/session/turn/session_io/wrapup.rs b/crates/openhuman-core/src/agent/harness/session/turn/session_io/wrapup.rs index fc17fb4eff..babae2fe54 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn/session_io/wrapup.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn/session_io/wrapup.rs @@ -19,6 +19,10 @@ impl Agent { /// tool-call-cap checkpoint (`MAX_ITER_CHECKPOINT_INSTRUCTION`) or the /// no-final-answer close (`FINAL_ANSWER_INSTRUCTION`, issue #4093). /// + /// `stream_text` forwards the validated text to the progress sink. The + /// no-final-answer close passes `false` and streams only after its own + /// check accepts the text (issue #6278), so a rejected close never renders. + /// /// Returns the summary text (empty when the provider call fails or /// yields nothing — the caller then falls back to a deterministic builder /// so the turn is never left without a well-formed assistant message, @@ -31,6 +35,7 @@ impl Agent { effective_model: &str, iteration_for_stream: u32, instruction: &str, + stream_text: bool, ) -> (String, Option) { let mut messages = base_messages.to_vec(); messages.push(ChatMessage::user(instruction)); @@ -148,7 +153,7 @@ impl Agent { // Hold wrap-up deltas until protocol validation completes. Otherwise a // rejected XML/P-Format tool call briefly renders in chat even though // the caller subsequently replaces it with a deterministic fallback. - if !checkpoint.is_empty() { + if stream_text && !checkpoint.is_empty() { if let Some(sink) = &self.on_progress { if let Err(error) = sink .send(AgentProgress::TextDelta { @@ -242,7 +247,7 @@ impl Agent { let mut base = self.tool_dispatcher.to_provider_messages(&self.history); base.push(ChatMessage::user(ro::repair_instruction(contract))); let (repair_text, usage) = self - .reprompt_for_required_block(&base, effective_model) + .silent_completion(&base, effective_model, "required-output re-prompt") .await; let repair_text = repair_text.trim().to_string(); @@ -309,9 +314,10 @@ impl Agent { Some((repaired, usage)) } - /// Ask the provider once for a reply that includes the required - /// structured-output block, with native tools **disabled** and **without** - /// forwarding any delta to the progress sink. Returns the parsed prose paired + /// Ask the provider once, with native tools **disabled** and **without** + /// forwarding any delta to the progress sink: the required-output repair + /// (issue #4117) and the closing-message check (issue #6278). `purpose` + /// labels the logs. Returns the parsed prose paired /// with the call's usage (empty text + `None` usage when the call fails or /// yields only tool-call markup). /// @@ -319,10 +325,11 @@ impl Agent { /// deliberately silent: `enforce_required_output` validates the result before /// deciding what (if anything) to stream, so a malformed repair attempt is /// never shown to the client. - async fn reprompt_for_required_block( + pub(in crate::agent::harness::session::turn) async fn silent_completion( &self, base_messages: &[ChatMessage], effective_model: &str, + purpose: &str, ) -> (String, Option) { let chat_model = match self .turn_model_source @@ -333,7 +340,8 @@ impl Agent { tracing::error!( error = %error, model = effective_model, - "[agent::session] failed to build required-output re-prompt model" + purpose, + "[agent::session] failed to build silent-completion model" ); return (String::new(), None); } @@ -353,7 +361,8 @@ impl Agent { tracing::warn!( error = %error, model = effective_model, - "[agent::session] required-output re-prompt stream failed to start" + purpose, + "[agent::session] silent-completion stream failed to start" ); return (String::new(), None); } @@ -369,18 +378,21 @@ impl Agent { } ModelStreamItem::Completed(response) => completed = Some(response), ModelStreamItem::Failed(error) => { - tracing::warn!(%error, "[agent::session] required-output re-prompt stream failed"); + tracing::warn!(%error, purpose, "[agent::session] silent-completion stream failed"); return (String::new(), None); } ModelStreamItem::ProviderFailed(error) => { - tracing::warn!(error = %error.message, "[agent::session] required-output re-prompt provider failed"); + tracing::warn!(error = %error.message, purpose, "[agent::session] silent-completion provider failed"); return (String::new(), None); } _ => {} } } let Some(response) = completed else { - tracing::warn!("[agent::session] required-output re-prompt ended without completion"); + tracing::warn!( + purpose, + "[agent::session] silent-completion ended without completion" + ); return (String::new(), None); }; let usage = crate::agent::tinyagents::model::usage_info_from_response(&response); @@ -399,7 +411,11 @@ impl Agent { /// Emit `text` to the progress sink as a `TextDelta` continuation so a /// repaired required-output block appears in the UI appended after the /// already-streamed reply (issue #4117). No-op when no sink is attached. - async fn stream_text_continuation(&self, text: &str, iteration: u32) { + pub(in crate::agent::harness::session::turn) async fn stream_text_continuation( + &self, + text: &str, + iteration: u32, + ) { if text.is_empty() { return; } diff --git a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs index 83323348ad..c844f69fe1 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs @@ -1,4 +1,3 @@ -use crate::agent::hooks::ToolCallRecord; use crate::agent::messages::ChatMessage; pub(crate) fn assistant_message_has_tool_calls(msg: &ChatMessage) -> bool { @@ -100,7 +99,7 @@ conclusion yet, say that plainly and name what is missing."; /// One completed tool call, carrying enough of its **actual output** to stand /// in for an answer (issue #6014). /// -/// Deliberately not [`ToolCallRecord`], and the difference is a contract rather +/// Deliberately not [`ToolCallRecord`](crate::agent::hooks::ToolCallRecord), and the difference is a contract rather /// than a convenience: that type's `output_summary` is produced by /// [`sanitize_tool_output`](crate::agent::hooks::sanitize_tool_output), /// which by design "never contains raw tool output or PII" and renders a @@ -183,48 +182,7 @@ pub(super) fn build_deterministic_checkpoint( if results.is_empty() { out.push_str("\n- (no tools completed yet)\n"); } else { - // Render each block before choosing, so the budget is charged what - // the checkpoint will actually contain (CodeRabbit on #6068). Walking - // `r.content` alone undercounts by the per-result header and the `" > "` - // on every line — a newline-heavy result costs well over its body, and - // several of them overran the limit while the walk believed it had room. - let blocks: Vec = results - .iter() - .map(|r| { - let status = if r.success { "ok" } else { "failed" }; - let mut block = format!("\n- `{}` — {}\n", r.name, status); - for line in r.content.lines() { - block.push_str(" > "); - block.push_str(line); - block.push('\n'); - } - block - }) - .collect(); - // Choose how far back the budget reaches by walking from the newest - // result, then render in the original order — a checkpoint read - // backwards is harder to follow than one that simply starts later. - let mut budget = CHECKPOINT_TOTAL_CHARS; - let mut first_shown = results.len(); - for (idx, block) in blocks.iter().enumerate().rev() { - let cost = block.chars().count(); - if cost > budget && idx + 1 < results.len() { - // The newest result is always shown, however long, so a single - // oversized payload cannot empty the checkpoint entirely; every - // earlier one has to fit what is left. - break; - } - budget = budget.saturating_sub(cost); - first_shown = idx; - } - if first_shown > 0 { - out.push_str(&format!( - "\n_({first_shown} earlier tool result(s) omitted for length — the most recent are shown.)_\n" - )); - } - for block in &blocks[first_shown..] { - out.push_str(block); - } + out.push_str(&render_tool_results(results, CHECKPOINT_TOTAL_CHARS)); } out.push_str( "\n**Next steps:** I'll continue from here — just reply (e.g. \"continue\") and I'll pick up where I left off.", @@ -232,31 +190,218 @@ pub(super) fn build_deterministic_checkpoint( out } +/// Render tool results as `` - `name` — ok|failed `` blocks quoting each +/// result's own output, spending `total_budget` characters from the **newest** +/// result backwards (see [`CHECKPOINT_TOTAL_CHARS`] for why newest first) and +/// disclosing how many earlier results were left out. +pub(super) fn render_tool_results(results: &[CheckpointToolResult], total_budget: usize) -> String { + // Render each block before choosing, so the budget is charged what + // the checkpoint will actually contain (CodeRabbit on #6068). Walking + // `r.content` alone undercounts by the per-result header and the `" > "` + // on every line — a newline-heavy result costs well over its body, and + // several of them overran the limit while the walk believed it had room. + let blocks: Vec = results + .iter() + .map(|r| { + let status = if r.success { "ok" } else { "failed" }; + let mut block = format!("\n- `{}` — {}\n", r.name, status); + for line in r.content.lines() { + block.push_str(" > "); + block.push_str(line); + block.push('\n'); + } + block + }) + .collect(); + // Choose how far back the budget reaches by walking from the newest + // result, then render in the original order — a checkpoint read + // backwards is harder to follow than one that simply starts later. + let mut budget = total_budget; + let mut first_shown = results.len(); + for (idx, block) in blocks.iter().enumerate().rev() { + let cost = block.chars().count(); + if cost > budget && idx + 1 < results.len() { + // The newest result is always shown, however long, so a single + // oversized payload cannot empty the checkpoint entirely; every + // earlier one has to fit what is left. + break; + } + budget = budget.saturating_sub(cost); + first_shown = idx; + } + let mut out = String::new(); + if first_shown > 0 { + out.push_str(&format!( + "\n_({first_shown} earlier tool result(s) omitted for length — the most recent are shown.)_\n" + )); + } + for block in &blocks[first_shown..] { + out.push_str(block); + } + out +} + +/// Budget for the tool records a closing message is grounded in and checked +/// against (issues #6278, #6279). +/// +/// Larger than [`CHECKPOINT_TOTAL_CHARS`] because these records go to a model +/// call, not a chat bubble. A reply is only as checkable as the records the +/// check can see: a success from ten calls back is exactly what a false "that +/// does not exist" contradicts. Still bounded, so a turn with a very long tool +/// history cannot push the wrap-up past a small context window. Past the bound +/// the oldest results drop first, disclosed as omitted. +pub(super) const GROUNDING_TOTAL_CHARS: usize = 16_000; + /// Instruction appended (as a synthetic user turn) when a turn finished its /// tool work but the model produced **no final answer** — it yielded a -/// terminating response with empty text after running tools (issue #4093). -/// Native tools are disabled for this call so the model wraps up in prose -/// instead of requesting more tools. +/// terminating response with empty text after running tools (issue #4093) — +/// or when the no-progress breaker halted the run (issue #6279). Native tools +/// are disabled for this call so the model wraps up in prose instead of +/// requesting more tools. Used through [`final_answer_instruction`], which +/// appends the turn's tool records. +/// +/// Issue #6278: "summarise what you did" drew replies that narrated intent +/// ("I'll search the registry") after the work was over, and replies that +/// contradicted results the context middleware had already cleared from view. +/// So this names both failure shapes, and the records are restated below it. pub(super) const FINAL_ANSWER_INSTRUCTION: &str = "\ You have finished using tools for this turn but have not yet written a reply to the user. \ -Do not call any more tools. Write a short, self-contained final message that summarises what you did and \ -what you found or accomplished, grounded in the tool results above. If nothing conclusive resulted, say so plainly."; +Tools are no longer available and nothing more will run this turn, so do not call any tools and do not \ +describe steps you are about to take. Write a self-contained final message that reports what actually happened: \ +what you found, changed or established, grounded in the tool results above and the tool records below. \ +If the request was not completed, say so and give the reason from the failing tool's own error message, \ +keeping any link it includes. Do not state anything the tool records contradict. \ +If nothing conclusive resulted, say so plainly."; + +/// The full closing-message instruction: [`FINAL_ANSWER_INSTRUCTION`], the +/// breaker's stop note when the run was halted (issue #6279), and this turn's +/// rendered tool records. +/// +/// The stop note is passed as input, not as text to repeat. The breaker words it +/// for a model ("Report this back instead of retrying"), which is right for a +/// sub-agent's parent and wrong on a user's screen. +pub(super) fn final_answer_instruction(stop_reason: Option<&str>, records: &str) -> String { + let mut out = String::new(); + if let Some(reason) = stop_reason { + out.push_str( + "The harness stopped this turn early because its tool calls stopped making progress: they \ + kept failing, or kept repeating the same step. Its stop note is written for you, not for \ + the user, so explain it in your own words rather than repeating it, and describe each \ + call as the tool records show it:\n\ + \n", + ); + out.push_str(reason.trim()); + out.push_str("\n\n\n"); + } + out.push_str(FINAL_ANSWER_INSTRUCTION); + out.push_str("\n\n\n"); + out.push_str(if records.trim().is_empty() { + "(no tool calls completed)" + } else { + records.trim() + }); + out.push_str("\n"); + out +} + +/// Prompt for the separate call that checks a closing message before it is +/// shown (issue #6278). +/// +/// The check sees only the request, the records and the candidate. It does not +/// see the conversation, so it cannot copy the pattern of the turn's own tool-call +/// preambles, which is what wrote the intent-only reply in the first place. The +/// three rules are shapes of reply, not particular tools or tasks. +pub(super) fn close_verification_prompt(user_request: &str, records: &str, reply: &str) -> String { + format!( + "You are checking a reply before it is shown to a user. Below are the user's request, the \ + records of the tool calls made while handling it, and the reply.\n\n\ + Answer REJECT if any of these is true:\n\ + 1. The reply only says what the assistant will do or is about to do, instead of reporting \ + what happened.\n\ + 2. The reply states something the tool records contradict, for example that something does \ + not exist or did not work when a record shows it succeeded, or that something succeeded \ + when its record shows it failed.\n\ + 3. The request was not completed, a failed record gives the reason, and the reply does not \ + pass that reason on.\n\n\ + Otherwise answer ACCEPT. Reply with the single word ACCEPT or REJECT.\n\n\ + \n{}\n\n\n\n{}\n\n\n\n{}\n", + truncate_chars(user_request, CHECKPOINT_TOTAL_CHARS), + if records.trim().is_empty() { + "(no tool calls completed)" + } else { + records.trim() + }, + reply.trim(), + ) +} + +/// The check call's verdict on a closing message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum CloseVerdict { + Accept, + Reject, + /// Neither word found (or the call failed). The reply is unverified, so the + /// caller does not ship it and uses the deterministic fallback instead. + Unclear, +} + +/// Read the verdict from the check call's text: the **last** standalone +/// `ACCEPT`/`REJECT` token wins, so a model that reasons aloud before its +/// answer is read by its conclusion, and `UNACCEPTABLE` is not an `ACCEPT`. +pub(super) fn parse_close_verdict(text: &str) -> CloseVerdict { + text.split(|c: char| !c.is_ascii_alphabetic()) + .filter_map(|token| match token.to_ascii_uppercase().as_str() { + "ACCEPT" => Some(CloseVerdict::Accept), + "REJECT" => Some(CloseVerdict::Reject), + _ => None, + }) + .next_back() + .unwrap_or(CloseVerdict::Unclear) +} -/// Build a deterministic final answer from this turn's tool-call records. +/// Build a deterministic final answer from this turn's tool results. /// Used as the guaranteed non-empty fallback when a turn ran tools but the -/// model produced no closing message and the re-prompt for one also came -/// back empty — so a turn that did work can never end silently (issue #4093). +/// model produced no usable closing message (empty, a tool call, or rejected +/// by the check), so a turn that did work can never end silently (issue #4093). /// Distinct from [`build_deterministic_checkpoint`]: the turn did NOT hit the /// iteration cap, so this reads as a completed summary, not a paused one. -pub(super) fn build_deterministic_final_summary(records: &[ToolCallRecord]) -> String { - if records.is_empty() { +/// +/// Quotes each result's own output (issue #6278): a failure's message is +/// usually the only explanation of why the request was not done, and it used to +/// be reduced to the word "failed". When the breaker halted the run (issue +/// #6279) its stop note is quoted too, because it names the rung that tripped +/// and, for a missing connection or exhausted credits, what the user must do. +/// The lead does not say the calls failed: `RepeatProgressMiddleware` halts +/// through the same slot when identical calls keep *succeeding*, and the +/// records below carry each call's real status. +pub(super) fn build_deterministic_final_summary( + results: &[CheckpointToolResult], + stop_reason: Option<&str>, +) -> String { + if results.is_empty() && stop_reason.is_none() { return "I finished this turn but produced no result to report.".to_string(); } - let mut out = String::from("Here's a summary of what I did this turn:\n\n"); - for r in records { - let status = if r.success { "ok" } else { "failed" }; - out.push_str(&format!("- `{}` — {}\n", r.name, status)); + let mut out = match stop_reason { + Some(reason) => { + let mut lead = String::from( + "I stopped this turn early because my tool calls were not making progress, so I \ + could not finish the request.\n\n**Why I stopped**\n", + ); + for line in reason.trim().lines() { + lead.push_str("> "); + lead.push_str(line); + lead.push('\n'); + } + lead.push_str("\n**What each tool call returned**\n"); + lead + } + None => String::from("I finished this turn without writing up a result. Here is what each tool call returned:\n"), + }; + if results.is_empty() { + out.push_str("\n- (no tool calls completed)\n"); + } else { + out.push_str(&render_tool_results(results, CHECKPOINT_TOTAL_CHARS)); } - out.push_str("\nLet me know if you'd like me to go further."); + out.push_str("\nTell me how you'd like to proceed."); out } diff --git a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_and_wrapup_tests.rs b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_and_wrapup_tests.rs index 002da6c6d0..3eab29c104 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_and_wrapup_tests.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_and_wrapup_tests.rs @@ -285,6 +285,11 @@ async fn turn_synthesizes_final_answer_when_tool_turn_yields_no_text() { usage: None, reasoning_content: None, }), + // The check on that closing message accepts it (#6278). + Ok(ChatResponse { + text: Some("ACCEPT".into()), + ..ChatResponse::default() + }), ]), requests: AsyncMutex::new(Vec::new()), tool_counts: AsyncMutex::new(Vec::new()), @@ -436,7 +441,7 @@ async fn summarize_turn_wrapup_rejects_prompt_tool_call_and_preserves_usage() { ); let (summary, usage) = agent - .summarize_turn_wrapup(&[], "test-model", 1, "write a wrap-up") + .summarize_turn_wrapup(&[], "test-model", 1, "write a wrap-up", true) .await; assert!( diff --git a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_tests.rs b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_tests.rs index 0683447387..9d846a6de6 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_tests.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint_tests.rs @@ -121,3 +121,127 @@ fn newline_heavy_results_are_charged_what_they_render() { assert!(out.contains("`list_issues_19` — ok")); assert!(out.contains(" > y\n"), "the newest body must be rendered"); } + +const STOP_NOTE: &str = "Stopping: the `install_item` call was retried 3 times with identical \ + arguments and kept failing. Report this back instead of retrying."; + +/// Issue #6278: the fallback used to reduce a failure to the word "failed", and +/// the failure's own message is usually the only explanation of why the +/// request was not done. +#[test] +fn the_final_summary_quotes_each_failure_message() { + let out = build_deterministic_final_summary( + &[result( + "install_item", + false, + "no direct download. View it at https://example.test/demo", + )], + None, + ); + assert!( + out.contains("`install_item` — failed"), + "status missing: {out}" + ); + assert!( + out.contains(" > no direct download. View it at https://example.test/demo"), + "the failure message must be quoted: {out}" + ); + assert!( + !out.contains("Why I stopped"), + "no halt, no stop section: {out}" + ); + assert!(!out.contains("tool-call limit"), "not a capped turn: {out}"); +} + +/// Issue #6279: when the breaker halted the run, the fallback says the turn +/// stopped early and keeps the stop note as a quoted reason, beside the +/// records, instead of standing in for the whole reply. +#[test] +fn the_final_summary_of_a_halted_turn_keeps_the_stop_note_and_the_records() { + let out = build_deterministic_final_summary( + &[result("install_item", false, "no direct download")], + Some(STOP_NOTE), + ); + assert!( + out.starts_with("I stopped this turn early"), + "lead missing: {out}" + ); + assert!( + out.contains("**Why I stopped**\n> Stopping:"), + "stop note must be quoted: {out}" + ); + assert!( + out.contains(" > no direct download"), + "records must follow: {out}" + ); +} + +/// The breaker also halts a run whose identical calls keep succeeding +/// (`RepeatProgressMiddleware`). The fallback must not call those calls failed +/// when its own records show them `ok` (Codex review on #6289). +#[test] +fn the_final_summary_of_a_successful_repeat_halt_does_not_claim_failure() { + let out = build_deterministic_final_summary( + &[result("list_items", true, "3 items")], + Some( + "Stopping: the same successful tool-call batch was issued 3 times in a row with \ + identical arguments and no new information.", + ), + ); + assert!( + !out.to_lowercase().contains("fail"), + "a halt over successful calls must not be described as failing: {out}" + ); + assert!( + out.contains("`list_items` — ok"), + "records keep their status: {out}" + ); +} + +/// The wrap-up is grounded in the records it is handed, and only a halted run +/// passes a stop note. +#[test] +fn the_final_answer_instruction_carries_the_records_and_the_stop_note() { + let records = render_tool_results( + &[result("install_item", false, "no direct download")], + 1_000, + ); + + let plain = final_answer_instruction(None, &records); + assert!(plain.contains("") && plain.contains(" > no direct download")); + assert!(!plain.contains("")); + assert!(plain.contains("do not describe steps you are about to take")); + + let halted = final_answer_instruction(Some(STOP_NOTE), &records); + assert!(halted.contains(&format!("\n{STOP_NOTE}\n"))); + assert!(halted.contains(" > no direct download")); +} + +/// The check is read by its conclusion: the last standalone verdict token wins, +/// a word that merely contains one is not a verdict, and no verdict is unclear. +#[test] +fn the_close_verdict_is_the_last_standalone_verdict_token() { + assert_eq!(parse_close_verdict("ACCEPT"), CloseVerdict::Accept); + assert_eq!(parse_close_verdict("reject."), CloseVerdict::Reject); + assert_eq!( + parse_close_verdict("It could ACCEPT, but rule 1 applies.\nREJECT"), + CloseVerdict::Reject + ); + assert_eq!(parse_close_verdict("UNACCEPTABLE"), CloseVerdict::Unclear); + assert_eq!(parse_close_verdict(""), CloseVerdict::Unclear); +} + +/// The check prompt holds the request, the records and the candidate, so the +/// checker can judge the reply against what actually ran. +#[test] +fn the_close_verification_prompt_holds_request_records_and_reply() { + let prompt = close_verification_prompt( + "install the demo item", + "\n- `install_item` — failed\n > no direct download\n", + "I'll search the registry.", + ); + assert!(prompt.contains("\ninstall the demo item\n")); + assert!(prompt.contains(" > no direct download")); + assert!(prompt.contains("\nI'll search the registry.\n")); + assert!(prompt.contains("ACCEPT or REJECT")); +} diff --git a/crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs b/crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs new file mode 100644 index 0000000000..10f5ca5527 --- /dev/null +++ b/crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs @@ -0,0 +1,300 @@ +//! Issues #6278 and #6279: a turn with no usable reply of its own is closed from +//! its tool records, and the closing message is checked before it is shown. + +use super::*; +use crate::agent::progress::AgentProgress; + +/// What the failing tool returns. It carries a link on purpose: these defects +/// dropped exactly this kind of user-actionable detail. +const INSTALL_FAILURE: &str = "item 'demo' has no direct download, so it can't be installed \ + automatically. View it at https://example.test/demo"; + +const INSTALL_CALL: &str = "{\"name\":\"install_item\",\"arguments\":{}}"; + +struct FailingInstallTool; + +#[async_trait] +impl Tool for FailingInstallTool { + fn name(&self) -> &str { + "install_item" + } + + fn description(&self) -> &str { + "install an item" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({"type":"object"}) + } + + async fn execute(&self, _args: serde_json::Value) -> Result { + Ok(ToolResult::error(INSTALL_FAILURE)) + } +} + +/// Succeeds on its first call and fails on every later one, so each tool round's +/// record is distinguishable from the others. +struct TwoRoundInstallTool { + calls: AtomicUsize, +} + +#[async_trait] +impl Tool for TwoRoundInstallTool { + fn name(&self) -> &str { + "install_item" + } + + fn description(&self) -> &str { + "install an item" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({"type":"object"}) + } + + async fn execute(&self, _args: serde_json::Value) -> Result { + if self.calls.fetch_add(1, Ordering::SeqCst) == 0 { + Ok(ToolResult::success( + "round one: found the demo item in the catalog", + )) + } else { + Ok(ToolResult::error(INSTALL_FAILURE)) + } + } +} + +fn respond(text: &str) -> anyhow::Result { + Ok(ChatResponse { + text: Some(text.into()), + ..ChatResponse::default() + }) +} + +fn scripted(responses: Vec>) -> Arc { + Arc::new(SequenceProvider { + responses: AsyncMutex::new(responses), + requests: AsyncMutex::new(Vec::new()), + tool_counts: AsyncMutex::new(Vec::new()), + }) +} + +/// Run one turn against `provider`, returning the reply and everything streamed. +async fn run_turn( + provider: Arc, + tools: Vec>, +) -> (Agent, String, String) { + let mut agent = make_agent_with_builder( + provider, + tools, + vec![], + crate::config::AgentConfig { + max_tool_iterations: 8, + ..crate::config::AgentConfig::default() + }, + crate::config::ContextConfig::default(), + ); + let (progress_tx, mut progress_rx) = tokio::sync::mpsc::channel(256); + agent.set_on_progress(Some(progress_tx)); + let reply = agent + .turn("install the demo item") + .await + .expect("the turn should close with a reply, not error"); + agent.set_on_progress(None); + let mut streamed = String::new(); + while let Ok(progress) = progress_rx.try_recv() { + if let AgentProgress::TextDelta { delta, .. } = progress { + streamed.push_str(&delta); + } + } + (agent, reply, streamed) +} + +fn history_ends_on(agent: &Agent, reply: &str) -> bool { + matches!( + agent.history.last(), + Some(ConversationMessage::Chat(msg)) if msg.role == "assistant" && msg.content == reply + ) +} + +#[tokio::test] +async fn an_intent_only_close_is_rejected_and_replaced_by_the_tool_records() { + const NARRATION: &str = "I'll search the registry for the demo item."; + let recorded = scripted(vec![ + respond(INSTALL_CALL), + // The silent end: tool work done, no final text (#4093). + respond(""), + // The wrap-up narrates intent instead of reporting the outcome. + respond(NARRATION), + // The check rejects it. + respond("REJECT"), + ]); + + let (agent, reply, streamed) = + run_turn(recorded.clone(), vec![Box::new(FailingInstallTool)]).await; + + assert!( + !reply.contains(NARRATION), + "a close the check rejected must not be the reply, got: {reply}" + ); + assert!( + reply.contains("View it at https://example.test/demo"), + "the fallback must carry the failing tool's own message, got: {reply}" + ); + assert!( + !streamed.contains(NARRATION), + "a rejected close must never be streamed, got: {streamed}" + ); + assert!( + history_ends_on(&agent, &reply), + "history must end on the reply the user saw, got: {:?}", + agent.history.last() + ); + + let requests = recorded.requests.lock().await; + assert_eq!(requests.len(), 4, "tool round, silent end, wrap-up, check"); + let wrap_up = &requests[2].last().expect("wrap-up instruction").content; + assert!( + wrap_up.contains("") && wrap_up.contains(INSTALL_FAILURE), + "the wrap-up must be grounded in this turn's tool records, got: {wrap_up}" + ); + assert_eq!( + requests[3].len(), + 1, + "the check must see only its own prompt, not the conversation" + ); + assert!( + requests[3][0].content.contains(NARRATION) + && requests[3][0].content.contains(INSTALL_FAILURE), + "the check must be given the candidate and the records, got: {}", + requests[3][0].content + ); +} + +/// A check that fails or answers without a verdict leaves the close unverified, +/// and unverified closing text must not ship (CodeRabbit on #6289). +#[tokio::test] +async fn an_unverified_close_is_replaced_by_the_tool_records() { + const CANDIDATE: &str = "The demo item is installed."; + let recorded = scripted(vec![ + respond(INSTALL_CALL), + respond(""), + respond(CANDIDATE), + // No ACCEPT/REJECT verdict. + respond("I am not sure."), + ]); + + let (agent, reply, streamed) = run_turn(recorded, vec![Box::new(FailingInstallTool)]).await; + + assert!( + !reply.contains(CANDIDATE), + "a close the check could not verify must not be the reply, got: {reply}" + ); + assert!( + reply.contains("View it at https://example.test/demo"), + "the fallback must carry the failing tool's own message, got: {reply}" + ); + assert!( + !streamed.contains(CANDIDATE), + "an unverified close must never be streamed, got: {streamed}" + ); + assert!(history_ends_on(&agent, &reply)); +} + +#[tokio::test] +async fn an_accepted_close_is_streamed_and_kept() { + const CLOSE: &str = "I could not install the demo item: it has no direct download. \ + You can view it at https://example.test/demo."; + let recorded = scripted(vec![ + respond(INSTALL_CALL), + respond(""), + respond(CLOSE), + respond("ACCEPT"), + ]); + + let (agent, reply, streamed) = run_turn(recorded, vec![Box::new(FailingInstallTool)]).await; + + assert_eq!(reply, CLOSE, "an accepted close is the reply"); + assert!( + streamed.contains(CLOSE), + "an accepted close must be streamed once the check passes, got: {streamed}" + ); + assert!(history_ends_on(&agent, &reply)); +} + +#[tokio::test] +async fn a_breaker_halt_is_closed_for_the_user_instead_of_showing_the_stop_note() { + const CLOSE: &str = "I could not install the demo item: it has no direct download. \ + You can view it at https://example.test/demo."; + let recorded = scripted(vec![ + // The same failing call three times trips the identical-retry breaker. + respond(INSTALL_CALL), + respond(INSTALL_CALL), + respond(INSTALL_CALL), + // Wrap-up, then its check. + respond(CLOSE), + respond("ACCEPT"), + ]); + + let (agent, reply, _) = run_turn(recorded.clone(), vec![Box::new(FailingInstallTool)]).await; + + assert!( + !reply.contains("instead of retrying") && !reply.starts_with("Stopping:"), + "the breaker's model-directed stop note must not be the user's reply, got: {reply}" + ); + assert_eq!(reply, CLOSE, "the checked close is the reply"); + assert!( + history_ends_on(&agent, &reply), + "a halted turn's reply must be recorded in history, got: {:?}", + agent.history.last() + ); + + let requests = recorded.requests.lock().await; + assert_eq!( + requests.len(), + 5, + "three tool rounds, the wrap-up and the check; the halted loop makes no further call" + ); + let wrap_up = &requests[3].last().expect("wrap-up instruction").content; + assert!( + wrap_up.contains("") && wrap_up.contains(INSTALL_FAILURE), + "the wrap-up must receive the stop note and the records as input, got: {wrap_up}" + ); +} + +/// XML-dialect calls carry no provider id, so both rounds' calls are `call_0`. +/// Each round must still be recorded with its own result: the second round's +/// failure is why the request was not done, and a first-match lookup replaced it +/// with the first round's success (Codex review on #6289). +#[tokio::test] +async fn each_tool_round_is_recorded_with_its_own_result() { + let recorded = scripted(vec![ + respond(INSTALL_CALL), + respond(INSTALL_CALL), + respond(""), + respond("I'll look into the demo item."), + respond("REJECT"), + ]); + + let (_, reply, _) = run_turn( + recorded.clone(), + vec![Box::new(TwoRoundInstallTool { + calls: AtomicUsize::new(0), + })], + ) + .await; + + assert!( + reply.contains("round one: found the demo item in the catalog"), + "the first round keeps its own result, got: {reply}" + ); + assert!( + reply.contains("View it at https://example.test/demo"), + "the second round must be recorded with its own failure, not the first round's result, got: {reply}" + ); + let requests = recorded.requests.lock().await; + let wrap_up = &requests[3].last().expect("wrap-up instruction").content; + assert!( + wrap_up.contains(INSTALL_FAILURE), + "the wrap-up records must carry the second round's failure, got: {wrap_up}" + ); +} diff --git a/crates/openhuman-core/src/agent/harness/session/turn_tests.rs b/crates/openhuman-core/src/agent/harness/session/turn_tests.rs index c18f06b3a8..6045c92dba 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_tests.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_tests.rs @@ -538,6 +538,8 @@ fn tool_calls_envelope(id: &str) -> String { mod turn_auto_recall_tests; #[path = "turn_checkpoint_and_wrapup_tests.rs"] mod turn_checkpoint_and_wrapup_tests; +#[path = "turn_final_reply_grounding_tests.rs"] +mod turn_final_reply_grounding_tests; #[path = "turn_history_and_context_tests.rs"] mod turn_history_and_context_tests; #[path = "turn_learned_context_and_announcements_tests.rs"] diff --git a/crates/openhuman-core/src/agent/tinyagents/payload_summarizer.rs b/crates/openhuman-core/src/agent/tinyagents/payload_summarizer.rs index 08b398af76..69dd4a6258 100644 --- a/crates/openhuman-core/src/agent/tinyagents/payload_summarizer.rs +++ b/crates/openhuman-core/src/agent/tinyagents/payload_summarizer.rs @@ -411,7 +411,7 @@ impl SubagentPayloadSummarizer { // (`run_child(.., streaming = false)`), which per its own contract // "leav[es] the parent's event stream unchanged", while still sharing // the sink so the sub-agent lifecycle events (started/completed) keep - // reaching observers. Mirrors `reprompt_for_required_block`, which is + // reaching observers. Mirrors `silent_completion`, which is // likewise deliberately silent about an internal repair call. // // Two bits of config that `invoke_in_parent` threaded are dropped by diff --git a/docs/TEST-COVERAGE-MATRIX.md b/docs/TEST-COVERAGE-MATRIX.md index 2de51d76de..12b4c837df 100644 --- a/docs/TEST-COVERAGE-MATRIX.md +++ b/docs/TEST-COVERAGE-MATRIX.md @@ -223,6 +223,7 @@ End-to-end coverage of the agent harness via the web-chat RPC surface against an | 4.4.10 | Provider Error Retry | RI | `tests/agent_harness_e2e.rs` | ✅ | First upstream 500 retried by ReliableProvider; second succeeds | | 4.4.11 | Inference Phase Transitions | WD | `app/test/e2e/specs/agent-harness-behaviors.spec.ts` | ✅ | Redux `inferenceStatusByThread` observes `subagent` phase then clears to idle | | 4.4.12 | Tool Timeline Completeness | WD | `app/test/e2e/specs/agent-harness-behaviors.spec.ts` | ✅ | Timeline entries carry id/name/status/round; subagent row reaches `success`; rounds non-decreasing | +| 4.4.13 | Grounded Close (no final text / breaker halt) | RU | `crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs`, `crates/openhuman-core/src/agent/harness/session/turn_checkpoint_tests.rs` | ✅ | Tool-records wrap-up, check rejects intent narration / contradicted claims, fallback quotes failure messages; breaker stop note never shown verbatim (#6278, #6279) | --- diff --git a/gitbooks/developing/architecture/agent-harness.md b/gitbooks/developing/architecture/agent-harness.md index ed3ca3b837..314fc2fd7e 100644 --- a/gitbooks/developing/architecture/agent-harness.md +++ b/gitbooks/developing/architecture/agent-harness.md @@ -582,7 +582,13 @@ Three cooperating mechanisms keep runs from wandering or dying silently: - `AwaitingUser { question, options }`: the child called `ask_user_clarification`; a full checkpoint (history, question, options, overrides) is written to `{workspace}/.openhuman/subagent_checkpoints/{task_id}.json`, and the run resumes from it when the user answers. - `Incomplete { reason }`: the child was halted by the breaker or hit its model-call cap. The delegating parent **relays the blocker** instead of treating a halted child as a finished answer or re-spinning the identical delegation. -A breaker halt at the top level is likewise never a silent finish: the turn's final text is overridden with the breaker's root-cause summary, and `hit_cap` / `breaker_halt` are surfaced on the turn result. +A breaker halt at the top level is likewise never a silent finish, and the breaker's root-cause summary is not shown to the user as is either: it is worded for a model ("Report this back instead of retrying"). `hit_cap` / `breaker_halt` are surfaced on the turn result, and the chat turn closes the halted run the same way it closes a tool turn that ended without final text (`turn/core/grounded_close.rs`, #4093 / #6278 / #6279): + +1. A tools-disabled wrap-up call whose instruction restates the turn's tool records, each failure's own message included, with the breaker summary passed as a stop note to explain rather than repeat. +2. A separate check call that sees only the request, the records and the candidate reply. It rejects a reply that only narrates intent, contradicts a record, or leaves out the failure that explains an unfinished request. +3. A deterministic fallback for an empty, tool-calling, rejected or unverified reply (a check that failed or gave no verdict). It quotes each tool result and the stop note. + +Accepted text is streamed only after the check, so a rejected reply never renders. **Classified tool failures** (`crates/openhuman-core/src/tools/status/`): every failed tool call is classified into a transport-agnostic `ClassifiedFailure { class, category, cause_plain, next_action, recoverable }`. Classes cover `MissingPermission`, `MissingApp`, `ServiceUnavailable`, `BadCredentials`, `BlockedByPolicy`, `ModelConnection`, `Timeout`, `Denied`, `ApprovalExpired`; categories map 1:1 to UI states: _recoverable_ (safe auto-retry), _blocked by policy_ (change settings), _needs user confirmation_ (sign in / install / grant), _user declined_ (never auto-retried). The classification rides `AgentProgress::ToolCallCompleted.failure` (including for sub-agent calls) into the chat timeline.