diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 9f5bca5b12..c4fb1f0424 100644 --- a/src/openhuman/flows/ops.rs +++ b/src/openhuman/flows/ops.rs @@ -3711,26 +3711,25 @@ pub async fn flows_build( } }; - // Emit the terminal chat event so a client viewing the copilot thread stops - // "processing" and finalizes the assistant bubble (the bridge streams only - // intermediate deltas). Success delivers `chat_done`; a run error delivers - // `chat_error`. The blocking return below is unchanged. - if let Some(target) = &stream { - let terminal: Result = match &run_error { - None => Ok(assistant_text.clone()), - Some(err) => Err(err.clone()), - }; - finalize_flow_stream(target, &terminal, &prompt).await; - } - // Capture the proposal from the run's tool history (propose/revise/save all // emit the same self-describing `{ type: "workflow_proposal", … }` payload). + // Extracted BEFORE the stream is finalized below (issue: builder + // convergence): the trail-off backstop needs `proposal`/`capped` to decide + // whether to override `assistant_text`, and the streamed copilot-pane chat + // bubble must render the SAME (possibly-overridden) text as the RPC + // response — the frontend renders from the stream, not the return value, + // so patching only the latter would still leave an interactive user + // staring at the original silent/status-only text. let proposal = extract_workflow_proposal(agent.history()); // A run that both errored AND produced no proposal is a hard failure; a run // that proposed before erroring still returns the proposal for review. if proposal.is_none() { if let Some(err) = &run_error { + if let Some(target) = &stream { + let terminal: Result = Err(err.clone()); + finalize_flow_stream(target, &terminal, &prompt).await; + } return Err(format!("workflow_builder produced no proposal: {err}")); } } @@ -3748,11 +3747,51 @@ pub async fn flows_build( let hit_cap = agent.last_turn_hit_cap(); let capped = hit_cap && proposal.is_none(); + // Terminal-state guarantee (builder convergence fix): a turn can end + // "naturally" (no more tool calls, not capped, no run error) yet still + // produce neither a proposal nor a real question — the model ran out of + // steam mid-build and left a status dump ("Done so far: checked + // connections…") as its final reply. `prompt.md` tells the model to + // always end a building turn in a proposal or a question, but a prompt + // rule can be silently ignored; this is the fail-closed backend backstop + // that makes it a hard invariant regardless of model behavior — the user + // is NEVER left with silence or an unanswerable status note. + let trail_off = !capped && proposal.is_none() && run_error.is_none(); + let assistant_text = if trail_off && !text_looks_like_question(&assistant_text) { + let fallback = build_trail_off_fallback(agent.history()); + tracing::warn!( + target: "flows", + flow_id = req.flow_id.as_deref().unwrap_or(""), + original_len = assistant_text.len(), + fallback_len = fallback.len(), + "[flows] flows_build: trail-off detected (no proposal, no cap, no question) — \ + guaranteeing a fallback question instead of silence" + ); + fallback + } else { + assistant_text + }; + + // Emit the terminal chat event so a client viewing the copilot thread stops + // "processing" and finalizes the assistant bubble (the bridge streams only + // intermediate deltas). Success delivers `chat_done`; a run error delivers + // `chat_error`. The blocking return below is unchanged. Uses the + // (possibly trail-off-overridden) `assistant_text` above. + if let Some(target) = &stream { + let terminal: Result = match &run_error { + None => Ok(assistant_text.clone()), + Some(err) => Err(err.clone()), + }; + finalize_flow_stream(target, &terminal, &prompt).await; + } + tracing::info!( target: "flows", + flow_id = req.flow_id.as_deref().unwrap_or(""), has_proposal = proposal.is_some(), hit_cap, capped, + trail_off, "[flows] flows_build: workflow_builder turn complete" ); Ok(RpcOutcome::single_log( @@ -3761,11 +3800,155 @@ pub async fn flows_build( "assistant_text": assistant_text, "error": run_error, "capped": capped, + "trail_off": trail_off, }), "workflow builder turn complete", )) } +/// Heuristic: does `text` already end with a clear, answerable question? +/// Conservative by design (issue: builder convergence) — a false negative (an +/// actual question this misses) just wraps it in the trail-off fallback, +/// which still includes the blocker context, so the safe failure mode is +/// "over-wrap", never "under-detect and stay silent". +fn text_looks_like_question(text: &str) -> bool { + let trimmed = text + .trim() + .trim_end_matches(|c: char| matches!(c, '"' | '\'' | ')' | ']' | '*' | '_' | '`' | '.')) + .trim_end(); + if trimmed.is_empty() { + return false; + } + if trimmed.ends_with('?') { + return true; + } + // The question may not be the literal last character (trailing markdown + // like a closing code fence or list marker on its own line) — fall back + // to the last non-blank line. This does NOT catch a question followed by + // a further trailing sentence ("...channel?\n\nLet me know!") — that's + // an accepted false negative: the turn still ends in a real (if + // over-eagerly replaced) question, never in silence, which is the + // invariant this function exists to protect. + trimmed + .lines() + .filter(|line| !line.trim().is_empty()) + .next_back() + .is_some_and(|last_line| last_line.trim_end().ends_with('?')) +} + +/// Builder-authoring tools whose result body can explain a trail-off — the +/// authoring belt `dry_run_workflow`/`validate_workflow`/`propose_workflow`/ +/// `revise_workflow`/`edit_workflow`/`save_workflow` all report either a hard +/// gate rejection (`ToolResult::error`) or a self-reported broken-graph +/// result (`"ok": false` in a successful body), so a plain-text read-only +/// tool's output is never misattributed as the blocker. +const TRAIL_OFF_BLOCKER_TOOLS: &[&str] = &[ + "dry_run_workflow", + "validate_workflow", + "propose_workflow", + "revise_workflow", + "edit_workflow", + "save_workflow", +]; + +/// Synthesizes a guaranteed, user-facing fallback for a trail-off turn (no +/// proposal, not capped, no run error, and the model's own text isn't a +/// question). Scans the run's tool history for the last builder-tool result +/// that looks like a blocker (a hard-gate rejection, or a `dry_run_workflow`/ +/// `validate_workflow` report with `"ok": false`) and asks the user about it; +/// falls back to a generic "what should I focus on" question when no such +/// blocker is found (the model may have simply stopped with nothing to point +/// to). +fn build_trail_off_fallback( + history: &[crate::openhuman::inference::provider::ConversationMessage], +) -> String { + match last_builder_tool_blocker(history) { + Some(blocker) => format!( + "I wasn't able to finish building this workflow. Here's where I got stuck:\n\n{blocker}\n\n\ + Could you tell me how you'd like me to resolve that, or share more detail about what's needed here?" + ), + None => "I wasn't able to finish building this workflow in this turn. Could you describe \ + what you'd like in more detail, or tell me which part to focus on?" + .to_string(), + } +} + +/// Scans `history` in reverse for the last result from a +/// [`TRAIL_OFF_BLOCKER_TOOLS`] call that reads as a failure — a plain-text +/// error message (gate rejection), or a JSON body with `"ok": false` — and +/// returns a truncated, human-readable description of it. Tool names are +/// resolved by correlating each `ToolResults` entry's `tool_call_id` back to +/// the `AssistantToolCalls` message that issued it, so this never +/// misattributes an unrelated read-only tool's plain-text output as a +/// blocker. +fn last_builder_tool_blocker( + history: &[crate::openhuman::inference::provider::ConversationMessage], +) -> Option { + use crate::openhuman::inference::provider::ConversationMessage; + + let mut call_names: std::collections::HashMap = + std::collections::HashMap::new(); + for message in history { + if let ConversationMessage::AssistantToolCalls { tool_calls, .. } = message { + for call in tool_calls { + call_names.insert(call.id.clone(), call.name.clone()); + } + } + } + + for message in history.iter().rev() { + let ConversationMessage::ToolResults(results) = message else { + continue; + }; + for result in results.iter().rev() { + let Some(name) = call_names.get(&result.tool_call_id) else { + continue; + }; + if !TRAIL_OFF_BLOCKER_TOOLS.contains(&name.as_str()) { + continue; + } + // This is the MOST RECENT authoring-belt tool result in the + // turn (results are scanned newest-first). Whatever it reads as + // is authoritative: a success/progress result here means any + // earlier failure from the same tool was already resolved + // within this turn, so we must stop at this result rather than + // keep walking backward and surfacing a stale, already-fixed + // blocker (see review discussion on this PR). + return describe_tool_result_blocker(&result.content) + .map(|desc| crate::openhuman::util::truncate_with_ellipsis(&desc, 500)); + } + } + None +} + +/// Reads one builder tool result's content as a failure description, or +/// `None` when it reads as success/progress (a `workflow_proposal` payload, +/// or an `"ok": true` report). The whole body is the description, never one +/// hardcoded field, so this stays correct regardless of which fields a given +/// tool uses to explain its failure. +fn describe_tool_result_blocker(content: &str) -> Option { + let trimmed = content.trim(); + if trimmed.is_empty() { + return None; + } + if let Ok(value) = serde_json::from_str::(trimmed) { + if value.get("type").and_then(Value::as_str) == Some("workflow_proposal") { + return None; // Success: a proposal was emitted. + } + if let Some(ok) = value.get("ok").and_then(Value::as_bool) { + return if ok { None } else { Some(value.to_string()) }; + } + // Some other structured payload with no `ok`/`type` marker this + // function recognises — not confidently a blocker, skip it. + return None; + } + // Non-JSON content: a hard-gate rejection (`ToolResult::error`) puts the + // plain error message straight into the content — since every builder + // tool's SUCCESS shape is JSON (a proposal or a `{ ok, ... }` report), a + // bare string here is, by elimination, an error message. + Some(trimmed.to_string()) +} + /// Scans an agent run's conversation history for the workflow proposal a builder /// tool emitted. `propose_workflow` / `revise_workflow` / `save_workflow` all /// return a self-describing `{ "type": "workflow_proposal", … }` JSON string as diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 4349f0523f..26433e2ff2 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/src/openhuman/flows/ops_tests.rs @@ -3995,3 +3995,189 @@ async fn compute_required_connections_skips_native_and_http_nodes() { "native oh: and http_request need no connection: {required:?}" ); } + +// ───────────────────────────────────────────────────────────────────────────── +// Builder convergence fix — trail-off backstop (`flows_build`'s terminal-state +// guarantee: every turn ends in a proposal or a real question, never silence). +// ───────────────────────────────────────────────────────────────────────────── + +fn builder_tool_call( + id: &str, + name: &str, +) -> crate::openhuman::inference::provider::ConversationMessage { + use crate::openhuman::inference::provider::{ConversationMessage, ToolCall}; + ConversationMessage::AssistantToolCalls { + text: None, + tool_calls: vec![ToolCall { + id: id.to_string(), + name: name.to_string(), + arguments: "{}".to_string(), + extra_content: None, + }], + reasoning_content: None, + extra_metadata: None, + } +} + +fn builder_tool_result( + call_id: &str, + content: &str, +) -> crate::openhuman::inference::provider::ConversationMessage { + use crate::openhuman::inference::provider::{ConversationMessage, ToolResultMessage}; + ConversationMessage::ToolResults(vec![ToolResultMessage { + tool_call_id: call_id.to_string(), + content: content.to_string(), + }]) +} + +#[test] +fn text_looks_like_question_detects_trailing_question_mark() { + assert!(text_looks_like_question( + "Which Slack channel should I post to?" + )); + assert!(text_looks_like_question("Which channel?\n")); + // Trailing markdown/punctuation noise after the '?' shouldn't defeat it. + assert!(text_looks_like_question("Which channel should I use?\"")); + // A trailing blank line after the question is still detected (the last + // NON-BLANK line is what's checked). + assert!(text_looks_like_question( + "Which channel should I post to?\n\n" + )); +} + +/// A question followed by a further trailing sentence on its own line +/// ("...channel?\n\nLet me know!") is an accepted false negative — the +/// heuristic is deliberately conservative (see the function doc). Pin that +/// this case is NOT detected so a future "improvement" doesn't silently +/// change the accepted trade-off without a matching design review. +#[test] +fn text_looks_like_question_accepts_false_negative_on_trailing_pleasantry() { + assert!(!text_looks_like_question( + "Which channel should I post to?\n\nLet me know!" + )); +} + +#[test] +fn text_looks_like_question_rejects_status_dumps_and_silence() { + assert!(!text_looks_like_question( + "## Done so far\n- Checked connections\n- Verified contracts" + )); + assert!(!text_looks_like_question("")); + assert!(!text_looks_like_question(" ")); + assert!(!text_looks_like_question("I'll continue working on this.")); +} + +/// The terminal-state guarantee's core invariant: whatever `build_trail_off_fallback` +/// returns, it must ALWAYS read as a question — the user is never left with +/// silence, regardless of what (if anything) the tool history contains. +#[test] +fn build_trail_off_fallback_always_yields_a_question() { + let fallback = build_trail_off_fallback(&[]); + assert!( + text_looks_like_question(&fallback), + "fallback with no tool history must still be a question: {fallback}" + ); + assert!(!fallback.trim().is_empty()); +} + +#[test] +fn build_trail_off_fallback_surfaces_last_dry_run_blocker() { + let history = vec![ + builder_tool_call("call_1", "dry_run_workflow"), + builder_tool_result( + "call_1", + r#"{"ok": false, "null_resolutions": [{"node_id": "send", "path": "args.channel"}]}"#, + ), + ]; + let fallback = build_trail_off_fallback(&history); + assert!( + text_looks_like_question(&fallback), + "blocker fallback must still end in a question: {fallback}" + ); + assert!( + fallback.contains("null_resolutions"), + "fallback should surface the actual dry-run blocker, got: {fallback}" + ); +} + +#[test] +fn build_trail_off_fallback_surfaces_gate_rejection_error_text() { + let history = vec![ + builder_tool_call("call_1", "propose_workflow"), + builder_tool_result( + "call_1", + "propose_workflow rejected: tool slug 'slack:not_a_real_action' does not exist", + ), + ]; + let fallback = build_trail_off_fallback(&history); + assert!(text_looks_like_question(&fallback)); + assert!(fallback.contains("does not exist")); +} + +#[test] +fn build_trail_off_fallback_ignores_unrelated_read_tool_output() { + // A plain-text result from a tool OUTSIDE the builder authoring belt (e.g. + // a read-only history lookup) must never be misattributed as the blocker + // — this stays tool-agnostic within the authoring belt, not "any tool". + let history = vec![ + builder_tool_call("call_1", "get_flow_history"), + builder_tool_result("call_1", "no prior revisions found"), + ]; + let fallback = build_trail_off_fallback(&history); + assert!(text_looks_like_question(&fallback)); + assert!( + !fallback.contains("no prior revisions found"), + "must not surface an unrelated read-tool's output as the blocker: {fallback}" + ); +} + +#[test] +fn build_trail_off_fallback_ignores_a_successful_proposal_payload() { + let history = vec![ + builder_tool_call("call_1", "propose_workflow"), + builder_tool_result( + "call_1", + r#"{"type": "workflow_proposal", "name": "demo", "graph": {}}"#, + ), + ]; + let fallback = build_trail_off_fallback(&history); + assert!(text_looks_like_question(&fallback)); + assert!(!fallback.contains("workflow_proposal")); +} + +#[test] +fn build_trail_off_fallback_picks_the_most_recent_blocker() { + // Two dry-run failures in the history: the fallback should describe the + // LAST one (the one the agent was still stuck on), not the first. + let history = vec![ + builder_tool_call("call_1", "dry_run_workflow"), + builder_tool_result("call_1", r#"{"ok": false, "errors": ["first issue"]}"#), + builder_tool_call("call_2", "dry_run_workflow"), + builder_tool_result("call_2", r#"{"ok": false, "errors": ["second issue"]}"#), + ]; + let fallback = build_trail_off_fallback(&history); + assert!(fallback.contains("second issue")); + assert!(!fallback.contains("first issue")); +} + +/// Regression for review feedback (chatgpt-codex-connector, PR #4887): a +/// dry-run failure that the agent goes on to FIX later in the same turn +/// (a later `{"ok": true}` from the same authoring belt) must not be +/// resurfaced as "here's where I got stuck" — that failure is already +/// resolved. The scan must stop at the most recent authoring-belt result, +/// not keep walking backward past a success to an older, stale blocker. +#[test] +fn build_trail_off_fallback_does_not_resurface_a_resolved_blocker() { + let history = vec![ + builder_tool_call("call_1", "dry_run_workflow"), + builder_tool_result("call_1", r#"{"ok": false, "errors": ["first issue"]}"#), + builder_tool_call("call_2", "dry_run_workflow"), + builder_tool_result("call_2", r#"{"ok": true, "warnings": []}"#), + ]; + let fallback = build_trail_off_fallback(&history); + assert!( + !fallback.contains("first issue"), + "must not surface an already-resolved blocker: {fallback}" + ); + assert!(text_looks_like_question(&fallback)); +}