From ddc0d3fabc2ba7f05f5b99f087fd4821e60caa89 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 15 Sep 2026 23:15:59 +0530 Subject: [PATCH 1/3] fix(harness): close empty and breaker-halted turns from tool records and check the reply A tool turn that ends without final text (#4093) shipped any non-empty wrap-up, including intent narration and claims its own tool results contradicted. A top-level breaker halt replaced the reply with the model-directed stop note and dropped the tool errors. Both now close through Agent::close_turn_from_records: a wrap-up grounded in the turn's tool records (with the stop note as input on a halt), a separate check call that rejects intent-only, contradicted or reason-dropping replies, and a deterministic fallback that quotes each result's output. Accepted text streams only after the check. Closes #6278 Closes #6279 --- .../src/agent/harness/session/turn/core.rs | 1 + .../session/turn/core/grounded_close.rs | 119 ++++++++ .../harness/session/turn/core/harness_turn.rs | 69 ++--- .../harness/session/turn/session_io/wrapup.rs | 40 ++- .../agent/harness/session/turn_checkpoint.rs | 259 ++++++++++++++---- .../turn_checkpoint_and_wrapup_tests.rs | 7 +- .../harness/session/turn_checkpoint_tests.rs | 102 +++++++ .../turn_final_reply_grounding_tests.rs | 197 +++++++++++++ .../src/agent/harness/session/turn_tests.rs | 2 + .../agent/tinyagents/payload_summarizer.rs | 2 +- docs/TEST-COVERAGE-MATRIX.md | 1 + .../developing/architecture/agent-harness.md | 8 +- 12 files changed, 684 insertions(+), 123 deletions(-) create mode 100644 crates/openhuman-core/src/agent/harness/session/turn/core/grounded_close.rs create mode 100644 crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs 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..442aa141a8 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn/core.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn/core.rs @@ -214,5 +214,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..1aecb05a00 --- /dev/null +++ b/crates/openhuman-core/src/agent/harness/session/turn/core/grounded_close.rs @@ -0,0 +1,119 @@ +//! 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 or rejected close 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; keeping the grounded reply" + ); + } + + let accepted = matches!(verdict, Some(CloseVerdict::Accept | CloseVerdict::Unclear)); + 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..08e1374061 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,214 @@ 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 after tool failures. Its stop note is written for \ + you, not for the user, so explain it in your own words rather than repeating 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 caller keeps the reply: + /// the check is a guard added on top of the grounded instruction, and an + /// unavailable checker is no evidence against the reply. + 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, + }) + .last() + .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. +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 kept failing, 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..dca8a4429b 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,105 @@ 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 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..7259743e72 --- /dev/null +++ b/crates/openhuman-core/src/agent/harness/session/turn_final_reply_grounding_tests.rs @@ -0,0 +1,197 @@ +//! 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)) + } +} + +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) -> (Agent, String, String) { + let mut agent = make_agent_with_builder( + provider, + vec![Box::new(FailingInstallTool)], + 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()).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 + ); +} + +#[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).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()).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}" + ); +} 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..c8a6cd2800 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 or rejected reply. 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. From 74ca95cab2b6c4028784c87e564dc76cbb4347aa Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 15 Sep 2026 23:42:17 +0530 Subject: [PATCH 2/3] fix(harness): match tool outcomes by occurrence and word breaker halts neutrally Prompt-guided tool calls reuse call_0, call_1 ids every round, and the record builders took the first outcome with a matching id, so later rounds were recorded with the first round's result. Match by id, tool name and occurrence instead. The breaker also halts runs whose identical calls keep succeeding, so the stop-note preamble and the fallback lead no longer say the calls failed. Also replaces Iterator::last with next_back in parse_close_verdict (clippy double_ended_iterator_last). --- .../src/agent/harness/session/turn/core.rs | 35 +++++++- .../agent/harness/session/turn_checkpoint.rs | 15 ++-- .../harness/session/turn_checkpoint_tests.rs | 22 +++++ .../turn_final_reply_grounding_tests.rs | 83 +++++++++++++++++-- 4 files changed, 143 insertions(+), 12 deletions(-) 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 442aa141a8..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 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 08e1374061..2d3e9d6f1d 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs @@ -284,8 +284,10 @@ pub(super) fn final_answer_instruction(stop_reason: Option<&str>, records: &str) let mut out = String::new(); if let Some(reason) = stop_reason { out.push_str( - "The harness stopped this turn early after tool failures. Its stop note is written for \ - you, not for the user, so explain it in your own words rather than repeating it:\n\ + "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()); @@ -354,7 +356,7 @@ pub(super) fn parse_close_verdict(text: &str) -> CloseVerdict { "REJECT" => Some(CloseVerdict::Reject), _ => None, }) - .last() + .next_back() .unwrap_or(CloseVerdict::Unclear) } @@ -370,6 +372,9 @@ pub(super) fn parse_close_verdict(text: &str) -> CloseVerdict { /// 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>, @@ -380,8 +385,8 @@ pub(super) fn build_deterministic_final_summary( let mut out = match stop_reason { Some(reason) => { let mut lead = String::from( - "I stopped this turn early because my tool calls kept failing, so I could not \ - finish the request.\n\n**Why I stopped**\n", + "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("> "); 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 dca8a4429b..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 @@ -176,6 +176,28 @@ fn the_final_summary_of_a_halted_turn_keeps_the_stop_note_and_the_records() { ); } +/// 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] 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 index 7259743e72..a9f6f3cbd8 100644 --- 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 @@ -32,6 +32,37 @@ impl Tool for FailingInstallTool { } } +/// 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()), @@ -48,10 +79,13 @@ fn scripted(responses: Vec>) -> Arc) -> (Agent, String, String) { +async fn run_turn( + provider: Arc, + tools: Vec>, +) -> (Agent, String, String) { let mut agent = make_agent_with_builder( provider, - vec![Box::new(FailingInstallTool)], + tools, vec![], crate::config::AgentConfig { max_tool_iterations: 8, @@ -95,7 +129,8 @@ async fn an_intent_only_close_is_rejected_and_replaced_by_the_tool_records() { respond("REJECT"), ]); - let (agent, reply, streamed) = run_turn(recorded.clone()).await; + let (agent, reply, streamed) = + run_turn(recorded.clone(), vec![Box::new(FailingInstallTool)]).await; assert!( !reply.contains(NARRATION), @@ -146,7 +181,7 @@ async fn an_accepted_close_is_streamed_and_kept() { respond("ACCEPT"), ]); - let (agent, reply, streamed) = run_turn(recorded).await; + let (agent, reply, streamed) = run_turn(recorded, vec![Box::new(FailingInstallTool)]).await; assert_eq!(reply, CLOSE, "an accepted close is the reply"); assert!( @@ -170,7 +205,7 @@ async fn a_breaker_halt_is_closed_for_the_user_instead_of_showing_the_stop_note( respond("ACCEPT"), ]); - let (agent, reply, _) = run_turn(recorded.clone()).await; + let (agent, reply, _) = run_turn(recorded.clone(), vec![Box::new(FailingInstallTool)]).await; assert!( !reply.contains("instead of retrying") && !reply.starts_with("Stopping:"), @@ -195,3 +230,41 @@ async fn a_breaker_halt_is_closed_for_the_user_instead_of_showing_the_stop_note( "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}" + ); +} From 7d47b9489e76f9218ef2e4dcd9d085798ddd0e49 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 15 Sep 2026 23:48:29 +0530 Subject: [PATCH 3/3] fix(harness): use the fallback when the closing-message check gives no verdict A failed or malformed check left the close unverified, and it was shipped anyway. Only an explicit ACCEPT now ships the model's text; anything else uses the deterministic fallback. Adds a regression test for an unclear verdict. --- .../session/turn/core/grounded_close.rs | 10 +++++-- .../agent/harness/session/turn_checkpoint.rs | 5 ++-- .../turn_final_reply_grounding_tests.rs | 30 +++++++++++++++++++ .../developing/architecture/agent-harness.md | 2 +- 4 files changed, 40 insertions(+), 7 deletions(-) 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 index 1aecb05a00..e83496f0fd 100644 --- 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 @@ -23,7 +23,8 @@ impl Agent { /// 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 or rejected close is replaced by + /// 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. /// @@ -89,11 +90,14 @@ impl Agent { }; if verdict == Some(CloseVerdict::Unclear) { log::warn!( - "[agent_loop] closing-message check returned no ACCEPT/REJECT verdict; keeping the grounded reply" + "[agent_loop] closing-message check returned no ACCEPT/REJECT verdict; using the deterministic fallback" ); } - let accepted = matches!(verdict, Some(CloseVerdict::Accept | CloseVerdict::Unclear)); + // 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 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 2d3e9d6f1d..c844f69fe1 100644 --- a/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs +++ b/crates/openhuman-core/src/agent/harness/session/turn_checkpoint.rs @@ -340,9 +340,8 @@ pub(super) fn close_verification_prompt(user_request: &str, records: &str, reply pub(super) enum CloseVerdict { Accept, Reject, - /// Neither word found (or the call failed). The caller keeps the reply: - /// the check is a guard added on top of the grounded instruction, and an - /// unavailable checker is no evidence against the reply. + /// 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, } 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 index a9f6f3cbd8..10f5ca5527 100644 --- 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 @@ -170,6 +170,36 @@ async fn an_intent_only_close_is_rejected_and_replaced_by_the_tool_records() { ); } +/// 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. \ diff --git a/gitbooks/developing/architecture/agent-harness.md b/gitbooks/developing/architecture/agent-harness.md index c8a6cd2800..314fc2fd7e 100644 --- a/gitbooks/developing/architecture/agent-harness.md +++ b/gitbooks/developing/architecture/agent-harness.md @@ -586,7 +586,7 @@ A breaker halt at the top level is likewise never a silent finish, and the break 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 or rejected reply. It quotes each tool result and the stop note. +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.