From daa5bda73399a56f3524ebe4a49f26c095b2b679 Mon Sep 17 00:00:00 2001 From: banana Date: Fri, 7 Aug 2026 13:19:56 +0800 Subject: [PATCH 1/3] fix(agent): stabilize design retries and execution --- crates/op-editor-host-core/src/design.rs | 18 +- crates/op-editor-host-core/tests/design.rs | 2 + .../src/chat_session_launch.rs | 31 ++- crates/op-host-desktop/src/design_session.rs | 54 +++++- .../src/design_session_tests.rs | 75 ++++++++ .../op-host-services/src/chat_http_server.rs | 182 ++++++++++++------ .../src/chat_http_server_tests.rs | 70 +++++++ crates/op-host-services/src/chat_intent.rs | 102 +++++++++- .../op-host-services/src/chat_intent_tests.rs | 130 ++++++++++++- .../op-host-services/src/chat_provider_llm.rs | 43 ++++- crates/op-orchestrator/src/subagent.rs | 26 ++- 11 files changed, 643 insertions(+), 90 deletions(-) diff --git a/crates/op-editor-host-core/src/design.rs b/crates/op-editor-host-core/src/design.rs index 2307f37a9..564fb7436 100644 --- a/crates/op-editor-host-core/src/design.rs +++ b/crates/op-editor-host-core/src/design.rs @@ -44,6 +44,9 @@ pub enum DesignDelta { /// Request from worker to UI to apply one editor mutation or batch boundary. pub struct DesignCmdReq { pub op: DesignCmdOp, + /// Page the design turn started on. The desktop applies every command + /// against this page even if the user switches pages while the LLM runs. + pub target_page_id: Option, pub ack: SyncSender, } @@ -151,19 +154,32 @@ impl DesignSession { pub struct RemoteDocSink { cmd_tx: Sender, mirror: EditorState, + target_page_id: Option, } impl RemoteDocSink { pub fn new(cmd_tx: Sender, initial_state: EditorState) -> Self { + let target_page_id = initial_state + .doc + .pages + .as_ref() + .and_then(|pages| pages.get(initial_state.ui.active_page_index)) + .map(|page| page.id.clone()) + .or_else(|| Some("0".into())); Self { cmd_tx, mirror: initial_state, + target_page_id, } } fn send_and_wait(&mut self, op: DesignCmdOp) -> bool { let (ack_tx, ack_rx) = mpsc::sync_channel::(1); - let req = DesignCmdReq { op, ack: ack_tx }; + let req = DesignCmdReq { + op, + target_page_id: self.target_page_id.clone(), + ack: ack_tx, + }; if self.cmd_tx.send(req).is_err() { return false; } diff --git a/crates/op-editor-host-core/tests/design.rs b/crates/op-editor-host-core/tests/design.rs index 36c65501e..86cb94728 100644 --- a/crates/op-editor-host-core/tests/design.rs +++ b/crates/op-editor-host-core/tests/design.rs @@ -24,6 +24,7 @@ fn remote_doc_sink_updates_mirror_on_ack() { let ui_thread = thread::spawn(move || { let req = rx.recv().expect("request"); + assert_eq!(req.target_page_id.as_deref(), Some("0")); let mut new_state = initial.clone(); new_state.viewport.zoom = 2.0; req.ack @@ -96,6 +97,7 @@ fn design_session_drains_progress_and_command_requests() { cmd_tx .send(DesignCmdReq { op: DesignCmdOp::Apply(EditorCommand::ClearSelection), + target_page_id: None, ack: ack_tx, }) .expect("cmd"); diff --git a/crates/op-host-desktop/src/chat_session_launch.rs b/crates/op-host-desktop/src/chat_session_launch.rs index 0be7ca262..e47b5c5ea 100644 --- a/crates/op-host-desktop/src/chat_session_launch.rs +++ b/crates/op-host-desktop/src/chat_session_launch.rs @@ -78,22 +78,30 @@ pub fn launch_if_pending( .selected_model_entry() .map(|entry| entry.builtin_provider_id.is_some() || entry.acp_agent_id().is_some()) .unwrap_or(false); + let restart_available = op_host_services::chat_intent::is_restart_command(&user_text) + && op_host_services::chat_intent::latest_design_request_for_restart(host.editor_state()) + .is_some(); if !is_builtin_or_acp { if launch_cli_standard_turn(host, &user_text, current_chat, current_design) { return true; } // CLI transport construction failed — fall through to the // honest-error path below. - } else if should_launch_direct_modify(host.editor_state(), &user_text) { + } else if !restart_available && should_launch_direct_modify(host.editor_state(), &user_text) { if launch_direct_modify_turn(host, &user_text, current_chat, current_design) { return true; } - } else if matches!(classify_intent(&user_text), Intent::Design) { + } else if restart_available + || (!op_host_services::chat_intent::is_non_request_text(&user_text) + && matches!(classify_intent(&user_text), Intent::Design)) + { // Phase 2.3: When the design-agent-loop flag is ON and a built-in // provider is configured, run the agentic tool-loop with the 14-tool // design toolset instead of the orchestrator pipeline. Flag OFF falls // through to the orchestrator path below — byte-for-byte unchanged. - if launch_design_loop_turn(host, user_text.clone(), current_chat, current_design) { + if !restart_available + && launch_design_loop_turn(host, user_text.clone(), current_chat, current_design) + { return true; } // Orchestrator path — unchanged when flag is OFF or no built-in @@ -116,7 +124,14 @@ pub fn launch_if_pending( &user_text, ); let initial_state = host.editor_state().clone(); - let request = build_design_request(user_text, &initial_state, append_context); + let current_request = + build_design_request(user_text.clone(), &initial_state, append_context); + let request = op_host_services::chat_intent::restore_design_request_for_restart( + &initial_state, + &user_text, + ¤t_request, + ) + .unwrap_or(current_request); // Persist the request onto the turn's assistant bubble (already // pushed by `begin_send`) BEFORE it moves into the worker — the // manual per-subtask "Retry" button needs it to re-run a failed @@ -399,8 +414,14 @@ fn launch_cli_standard_turn( let modify_plan = op_host_services::chat_intent::build_modify_plan(state, user_text); let append_context = op_host_services::chat_intent::detect_append_intent(state, user_text); let initial_state = state.clone(); - let design_request = + let current_request = build_design_request(user_text.to_string(), &initial_state, append_context); + let design_request = op_host_services::chat_intent::restore_design_request_for_restart( + &initial_state, + user_text, + ¤t_request, + ) + .unwrap_or(current_request); // Same stash as the builtin/design-intent path above — this turn may or // may not actually classify as `DesignIntent::New` on the worker (the // classifier runs async), but setting it unconditionally is harmless: diff --git a/crates/op-host-desktop/src/design_session.rs b/crates/op-host-desktop/src/design_session.rs index dd686c145..3fe5133b0 100644 --- a/crates/op-host-desktop/src/design_session.rs +++ b/crates/op-host-desktop/src/design_session.rs @@ -46,10 +46,37 @@ pub fn pump_commands( let state = host.editor_state_mut(); let mut any_applied = false; for req in reqs { + let target_page_index = + req.target_page_id + .as_deref() + .and_then(|page_id| match state.doc.pages.as_ref() { + Some(pages) if !pages.is_empty() => pages + .iter() + .position(|page| page.id == page_id) + .or_else(|| { + page_id + .parse::() + .ok() + .filter(|idx| *idx < pages.len()) + }), + _ if page_id == "0" => Some(0), + _ => None, + }); + let original_page_index = state.ui.active_page_index; + let original_selection = state.selection.clone(); + if let Some(target) = target_page_index { + state.ui.active_page_index = target; + if target != original_page_index { + state.clear_selection(); + } + } + let target_available = req.target_page_id.is_none() || target_page_index.is_some(); + let target_is_visible = + req.target_page_id.is_none() || target_page_index == Some(original_page_index); let applied = match req.op { DesignCmdOp::Apply(cmd) => { - let applied = state.apply(cmd); - if applied { + let applied = target_available && state.apply(cmd); + if applied && target_is_visible { fit_design_viewport_to_content(state, viewport_width, viewport_height); } applied @@ -60,7 +87,13 @@ pub fn pump_commands( // functionally correct, just finer-grained than ideal. DesignCmdOp::BeginUndoBatch | DesignCmdOp::EndUndoBatch => true, }; + // The worker mirror stays on the design target, while the visible + // editor returns to the page and selection the user is viewing. let snapshot = state.clone(); + if target_page_index.is_some_and(|target| target != original_page_index) { + state.ui.active_page_index = original_page_index; + state.selection = original_selection; + } let ack = DesignCmdAck { applied, new_state: snapshot, @@ -293,11 +326,11 @@ fn apply_progress(msg: &mut ChatMessage, progress: &[Progress], locale: Locale) ChatActivityStatus::Done, Some(element_count(locale, *node_count)), ), - Progress::SubtaskFailed { id, .. } => update_activity( + Progress::SubtaskFailed { id, error } => update_activity( msg, id, ChatActivityStatus::Error, - Some(op_i18n::translate(locale, "ai.designProgress.detail.needsAttention").into()), + Some(subtask_failure_detail(locale, error)), ), Progress::SubtaskRetry { id, attempt, .. } => update_activity( msg, @@ -500,6 +533,19 @@ fn element_count(locale: Locale, count: usize) -> String { op_i18n::translate(locale, key).replace("{{count}}", &count.to_string()) } +fn subtask_failure_detail(locale: Locale, error: &str) -> String { + let label = op_i18n::translate(locale, "ai.designProgress.detail.needsAttention"); + let compact = error.split_whitespace().collect::>().join(" "); + if compact.is_empty() { + return label.into(); + } + let mut visible: String = compact.chars().take(220).collect(); + if compact.chars().count() > 220 { + visible.push('…'); + } + format!("{label}: {visible}") +} + fn planned_narration(locale: Locale, count: usize) -> String { let key = if count == 1 { "ai.designProgress.narration.plannedOne" diff --git a/crates/op-host-desktop/src/design_session_tests.rs b/crates/op-host-desktop/src/design_session_tests.rs index 9bc4c09dc..fa6b7eb6f 100644 --- a/crates/op-host-desktop/src/design_session_tests.rs +++ b/crates/op-host-desktop/src/design_session_tests.rs @@ -429,6 +429,7 @@ fn pump_commands_refits_viewport_after_design_insert() { parent_id: op_editor_core::NodeId::NONE, page_id: None, }), + target_page_id: None, ack: ack_tx, }) .expect("request should queue"); @@ -457,6 +458,58 @@ fn pump_commands_refits_viewport_after_design_insert() { ); } +#[test] +fn pump_commands_keeps_a_design_turn_bound_to_its_start_page() { + use jian_ops_schema::page::PenPage; + + let (_delta_tx, delta_rx) = mpsc::channel::(); + let (cmd_tx, cmd_rx) = mpsc::channel::(); + let mut current = Some(DesignSession::from_channels(delta_rx, cmd_rx)); + let mut host = WidgetHostNative::new(); + host.editor_state_mut().doc.children.clear(); + host.editor_state_mut().doc.pages = Some(vec![ + PenPage { + id: "page-design".into(), + name: "Design".into(), + children: Vec::new(), + background_color: None, + state: None, + lifecycle: None, + }, + PenPage { + id: "page-user".into(), + name: "User".into(), + children: Vec::new(), + background_color: None, + state: None, + lifecycle: None, + }, + ]); + host.editor_state_mut().ui.active_page_index = 1; + + let (ack_tx, ack_rx) = mpsc::sync_channel::(1); + cmd_tx + .send(DesignCmdReq { + op: DesignCmdOp::Apply(EditorCommand::InsertSubtree { + nodes: vec![mobile_root()], + parent_id: op_editor_core::NodeId::NONE, + page_id: None, + }), + target_page_id: Some("page-design".into()), + ack: ack_tx, + }) + .unwrap(); + + assert!(pump_commands(&mut host, &mut current, 1440.0, 900.0)); + let ack = ack_rx.recv_timeout(Duration::from_secs(1)).unwrap(); + assert!(ack.applied); + assert_eq!(ack.new_state.ui.active_page_index, 0); + let pages = host.editor_state().doc.pages.as_ref().unwrap(); + assert_eq!(pages[0].children.len(), 1); + assert!(pages[1].children.is_empty()); + assert_eq!(host.editor_state().ui.active_page_index, 1); +} + #[test] fn fit_design_viewport_uses_resolved_layout_for_fit_content_root() { let mut state = EditorState::new(); @@ -614,6 +667,28 @@ fn typed_progress_updates_retry_without_duplicating_the_activity() { assert!(!format!("{:?}", message.activities).contains("zero nodes")); } +#[test] +fn failed_subtask_keeps_the_actionable_error_in_the_activity() { + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + assert!(super::apply_progress( + &mut message, + &[ + Progress::SubtaskStarted { + id: "hero".into(), + label: "Hero".into(), + }, + Progress::SubtaskFailed { + id: "hero".into(), + error: "InsertSubtree rejected: parent_id=root status=missing".into(), + }, + ], + Locale::EnUs, + )); + let detail = message.activities[0].detail.as_deref().unwrap(); + assert!(detail.contains("Needs attention"), "{detail}"); + assert!(detail.contains("parent_id=root status=missing"), "{detail}"); +} + #[test] fn cli_progress_uses_the_editor_locale_for_visible_process_and_summary() { let mut message = op_editor_core::ChatMessage::assistant_streaming(); diff --git a/crates/op-host-services/src/chat_http_server.rs b/crates/op-host-services/src/chat_http_server.rs index 884a9cbd0..88d4b2eb2 100644 --- a/crates/op-host-services/src/chat_http_server.rs +++ b/crates/op-host-services/src/chat_http_server.rs @@ -315,7 +315,11 @@ async fn run_opencode_turn( tokio::time::sleep(SSE_SETTLE).await; // 6. Send the prompt (TS chat.ts:659-664, 710-716). - let mut prompt_payload = serde_json::json!({ "parts": parts }); + // This is a text-completion bridge; tools would strand the response. + let mut prompt_payload = serde_json::json!({ + "parts": parts, + "tools": { "*": false }, + }); if let Some((provider_id, model_id)) = &parsed_model { prompt_payload["model"] = serde_json::json!({ "providerID": provider_id, @@ -337,15 +341,21 @@ async fn run_opencode_turn( // 7. Consume events until idle / error / timeout // (TS chat.ts:718-776). let deadline = tokio::time::Instant::now() + STREAM_TIMEOUT; - let mut emitted_text = false; + let mut streamed_text = String::new(); let mut canceled = false; + let mut timed_out = false; + let mut terminal_error = None; + let mut tool_escape = None; loop { tokio::select! { biased; _ = tx.closed() => { canceled = true; break } // TS streamWithTimeout: the 180s budget ends the stream; // the fallback + empty checks still run after it. - _ = tokio::time::sleep_until(deadline) => break, + _ = tokio::time::sleep_until(deadline) => { + timed_out = true; + break; + }, ev = events_rx.recv() => { let Some(val) = ev else { break }; // SSE stream ended let Some(ty) = val.get("type").and_then(|v| v.as_str()) else { continue }; @@ -365,7 +375,7 @@ async fn run_opencode_turn( canceled = true; break; } - emitted_text = true; + streamed_text.push_str(delta); } // Forward reasoning deltas as thinking chunks. if prop_session == Some(session_id.as_str()) @@ -376,6 +386,23 @@ async fn run_opencode_turn( break; } } + "message.part.updated" => { + let part = props.and_then(|p| p.get("part")); + let part_session = part + .and_then(|p| p.get("sessionID")) + .and_then(|v| v.as_str()); + if part_session == Some(session_id.as_str()) + && part.and_then(|p| p.get("type")).and_then(|v| v.as_str()) + == Some("tool") + { + let name = part + .and_then(|p| p.get("tool")) + .and_then(|v| v.as_str()) + .unwrap_or("unknown"); + tool_escape = Some(name.to_string()); + break; + } + } // Session went idle — response complete. "session.idle" => { if prop_session == Some(session_id.as_str()) { @@ -383,16 +410,14 @@ async fn run_opencode_turn( } } // Session error: ours, or one with no session id. - "session.error" => { - if prop_session == Some(session_id.as_str()) || prop_session.is_none() { - let err = props.and_then(|p| p.get("error")); - let msg = format_opencode_error(err); - eprintln!("[AI] OpenCode session error: {msg}"); - if tx.send(ChatDelta::Error(msg)).await.is_err() { - canceled = true; - } - break; - } + "session.error" + if prop_session == Some(session_id.as_str()) || prop_session.is_none() => + { + let err = props.and_then(|p| p.get("error")); + let msg = format_opencode_error(err); + eprintln!("[AI] OpenCode session error: {msg}"); + terminal_error = Some(msg); + break; } _ => {} } @@ -404,54 +429,73 @@ async fn run_opencode_turn( return; } - // 8. Fallback: no streamed text → read the session messages - // directly (TS chat.ts:778-802; failures fall through to the - // empty-response error). - if !emitted_text { - if let Ok(messages) = get_json(&ops, &format!("{base}/session/{session_id}/message")).await - { - if let Some(items) = messages.as_array() { - let assistant = items.iter().rev().find(|m| { - m.get("info") - .and_then(|i| i.get("role")) - .and_then(|v| v.as_str()) - == Some("assistant") - }); - if let Some(parts) = assistant - .and_then(|m| m.get("parts")) - .and_then(|p| p.as_array()) + if timed_out { + abort_session(&ops, &base, &session_id).await; + return fail( + tx, + format!( + "OpenCode timed out after {} seconds before the session completed.", + STREAM_TIMEOUT.as_secs() + ), + ) + .await; + } + if let Some(name) = tool_escape { + abort_session(&ops, &base, &session_id).await; + return fail( + tx, + format!( + "OpenCode attempted the forbidden `{name}` tool during a text-only completion." + ), + ) + .await; + } + if let Some(error) = terminal_error { + return fail(tx, error).await; + } + + // 8. Reconcile against the persisted assistant message even when SSE + // produced text. OpenCode can reach idle after dropping a final delta; + // emitting only the missing suffix keeps the completion exact. + if let Ok(messages) = get_json(&ops, &format!("{base}/session/{session_id}/message")).await { + if let Some(final_text) = latest_assistant_text(&messages) { + if streamed_text.is_empty() { + if tx + .send(ChatDelta::TextDelta(final_text.clone())) + .await + .is_err() { - for part in parts { - if part.get("type").and_then(|v| v.as_str()) == Some("text") { - if let Some(text) = part - .get("text") - .and_then(|v| v.as_str()) - .filter(|t| !t.is_empty()) - { - if tx - .send(ChatDelta::TextDelta(text.to_string())) - .await - .is_err() - { - return; - } - emitted_text = true; - } - } - } + return; } + streamed_text = final_text; + } else if let Some(suffix) = final_text.strip_prefix(&streamed_text) { + if !suffix.is_empty() + && tx + .send(ChatDelta::TextDelta(suffix.to_string())) + .await + .is_err() + { + return; + } + streamed_text = final_text; + } else if final_text != streamed_text { + return fail( + tx, + "OpenCode stream did not match its persisted final response.".into(), + ) + .await; } } } - // 9. Still nothing → the TS empty-response error (chat.ts:804-811). - if !emitted_text { - let _ = tx - .send(ChatDelta::Error( - "OpenCode returned an empty response. The model may not have generated any output." - .into(), - )) - .await; + // 9. Still nothing → terminal empty-response failure. + if streamed_text.is_empty() { + return fail( + tx, + "OpenCode returned an empty response. The model may not have generated any output." + .into(), + ) + .await; } let _ = tx @@ -461,6 +505,32 @@ async fn run_opencode_turn( .await; } +fn latest_assistant_text(messages: &serde_json::Value) -> Option { + let assistant = messages.as_array()?.iter().rev().find(|message| { + message + .get("info") + .and_then(|info| info.get("role")) + .and_then(|role| role.as_str()) + == Some("assistant") + })?; + let text = assistant + .get("parts")? + .as_array()? + .iter() + .filter(|part| part.get("type").and_then(|value| value.as_str()) == Some("text")) + .filter_map(|part| part.get("text").and_then(|value| value.as_str())) + .collect::(); + (!text.is_empty()).then_some(text) +} + +async fn abort_session(client: &reqwest::Client, base: &str, session_id: &str) { + let _ = client + .post(format!("{base}/session/{session_id}/abort")) + .json(&serde_json::json!({})) + .send() + .await; +} + /// Probe an already-running server (TS `client.config.providers()`). async fn probe_server(client: &reqwest::Client, base: &str) -> bool { match tokio::time::timeout( diff --git a/crates/op-host-services/src/chat_http_server_tests.rs b/crates/op-host-services/src/chat_http_server_tests.rs index e8b9ffce8..c6f8f2bae 100644 --- a/crates/op-host-services/src/chat_http_server_tests.rs +++ b/crates/op-host-services/src/chat_http_server_tests.rs @@ -96,6 +96,7 @@ fn handle_connection(mut stream: TcpStream, scenario: Scenario, log: RequestLog) ("POST", "/session") => write_json(&mut stream, 200, r#"{"id":"ses_mock"}"#), ("POST", "/session/ses_mock/message") => write_json(&mut stream, 200, r#"{"info":{}}"#), ("POST", "/session/ses_mock/prompt_async") => write_json(&mut stream, 200, "{}"), + ("POST", "/session/ses_mock/abort") => write_json(&mut stream, 200, "true"), ("GET", "/session/ses_mock/message") => { write_json(&mut stream, 200, &scenario.messages_fallback) } @@ -197,6 +198,8 @@ fn opencode_turn_streams_text_thinking_and_done() { assert!(prompt.2.contains(r#""providerID":"anthropic""#)); assert!(prompt.2.contains(r#""modelID":"claude-test""#)); assert!(prompt.2.contains(r#""text":"hi""#)); + let prompt_json: serde_json::Value = serde_json::from_str(&prompt.2).unwrap(); + assert_eq!(prompt_json["tools"]["*"], false); } #[test] @@ -259,6 +262,73 @@ fn opencode_empty_stream_falls_back_to_session_messages() { ); } +#[test] +fn opencode_reconciles_a_missing_final_sse_suffix() { + let scenario = Scenario { + sse_events: vec![ + r#"{"type":"message.part.delta","properties":{"sessionID":"ses_mock","field":"text","delta":"partial"}}"#.into(), + r#"{"type":"session.idle","properties":{"sessionID":"ses_mock"}}"#.into(), + ], + messages_fallback: r#"[ + {"info":{"role":"assistant"},"parts":[{"type":"text","text":"partial-final"}]} + ]"# + .into(), + }; + let server = start_mock(scenario); + let deltas = collect_deltas( + &server, + ChatRequest { + user_message: "hi".into(), + ..Default::default() + }, + ); + let text: String = deltas + .iter() + .filter_map(|delta| match delta { + ChatDelta::TextDelta(text) => Some(text.as_str()), + _ => None, + }) + .collect(); + assert_eq!(text, "partial-final", "{deltas:?}"); +} + +#[test] +fn opencode_aborts_if_a_tool_escapes_the_text_only_contract() { + let scenario = Scenario { + sse_events: vec![ + r#"{"type":"message.part.updated","properties":{"part":{"id":"prt_1","sessionID":"ses_mock","messageID":"msg_1","type":"tool","callID":"call_1","tool":"write","state":{"status":"running","input":{},"time":{"start":1}}}}}"#.into(), + ], + messages_fallback: "[]".into(), + }; + let server = start_mock(scenario); + let deltas = collect_deltas( + &server, + ChatRequest { + user_message: "hi".into(), + ..Default::default() + }, + ); + assert!( + deltas.iter().any(|delta| matches!( + delta, + ChatDelta::Error(message) if message.contains("forbidden `write` tool") + )), + "{deltas:?}" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::Aborted + }) + )); + assert!(server + .requests + .lock() + .unwrap() + .iter() + .any(|(method, path, _)| method == "POST" && path == "/session/ses_mock/abort")); +} + #[test] fn opencode_idle_with_no_output_emits_empty_response_error() { let scenario = Scenario { diff --git a/crates/op-host-services/src/chat_intent.rs b/crates/op-host-services/src/chat_intent.rs index fd6aab485..60ad7c659 100644 --- a/crates/op-host-services/src/chat_intent.rs +++ b/crates/op-host-services/src/chat_intent.rs @@ -5,7 +5,7 @@ //! providers (builtin / ACP turns return early in both stacks): //! //! - `apps/web/src/components/panels/ai-chat-intent-classifier.ts` -//! — `classifyIntent` (LLM call, 8s abort, fallback `new`) and +//! — `classifyIntent` (LLM call, 8s abort) and //! `classifyByKeywords`, both verbatim. //! - `apps/web/src/components/panels/ai-chat-handlers.ts:693-776` //! — modify-vs-new degrade rules, `generateDesignModification` @@ -142,6 +142,84 @@ fn matches_any_word_phrase(text_lower: &str, phrases: &[&str]) -> bool { phrases.iter().any(|p| matches_word_phrase(text_lower, p)) } +/// True when the message has no word/number content to route. Punctuation-only +/// reactions must stay in chat instead of opening an unrelated design turn. +pub fn is_non_request_text(text: &str) -> bool { + !text.chars().any(char::is_alphanumeric) +} + +/// Exact, standalone rerun commands. Keeping this exact avoids treating +/// requests such as "restart the server" as design-session control. +pub fn is_restart_command(text: &str) -> bool { + let normalized = text + .trim() + .trim_matches(|c: char| { + c.is_whitespace() + || matches!( + c, + '.' | ',' | '!' | '?' | ';' | ':' | '。' | '，' | '！' | '？' | '；' | '：' + ) + }) + .split_whitespace() + .collect::>() + .join(" ") + .to_lowercase(); + matches!( + normalized.as_str(), + "restart" + | "start over" + | "try again" + | "retry" + | "重新开始" + | "重新生成" + | "再试一次" + | "重来" + | "重试" + ) +} + +/// Recover the latest persisted design request from this chat tab. +pub fn latest_design_request_for_restart(state: &EditorState) -> Option { + state + .chat + .messages + .iter() + .rev() + // CLI-standard stages a request before asynchronous classification, + // even for turns that later route to plain chat. Require evidence that + // the design worker actually owned this message. + .filter(|message| { + message.completion.is_some() + || !message.activities.is_empty() + || !message.failed_subtasks.is_empty() + }) + .find_map(|message| { + message + .design_request_json_for_retry + .as_deref() + .and_then(|json| serde_json::from_str(json).ok()) + }) +} + +/// Reuse the previous design content while honoring the controls selected for +/// the new run (model/provider/concurrency/validation). +pub fn restore_design_request_for_restart( + state: &EditorState, + text: &str, + current: &DesignRequest, +) -> Option { + if !is_restart_command(text) { + return None; + } + let mut previous = latest_design_request_for_restart(state)?; + previous.model = current.model.clone(); + previous.provider = current.provider.clone(); + previous.concurrency = current.concurrency; + previous.validation_enabled = current.validation_enabled; + previous.visual_ref_enabled = current.visual_ref_enabled; + Some(previous) +} + /// TS `classifyByKeywords` — verbatim rule order. pub fn classify_by_keywords(text: &str) -> DesignIntent { let lower = text.to_lowercase(); @@ -172,6 +250,16 @@ pub fn classify_intent_for_standard_route( text: &str, model: Option, ) -> DesignIntent { + if is_restart_command(text) { + return if latest_design_request_for_restart(state).is_some() { + DesignIntent::New + } else { + DesignIntent::Chat + }; + } + if is_non_request_text(text) { + return DesignIntent::Chat; + } // A whole-screen *draw* (creation verb + page noun, e.g. "重新画一个 // search 页面") is unambiguously a new screen — it must win over the // modify classifier so it routes to the new-frame path, not edit-in-place. @@ -212,12 +300,12 @@ pub fn parse_classified(text: &str) -> DesignIntent { if upper.contains("CHAT") { return DesignIntent::Chat; } - DesignIntent::New + DesignIntent::Chat } /// TS `classifyIntent` — one lightweight LLM call through the (chat- /// session-untracked) provider, with the TS 8s abort and the TS -/// fallback to `new` on any failure / timeout. +/// conservative fallback to chat on any failure / timeout. pub fn classify_intent_llm( provider: &dyn ChatProvider, text: &str, @@ -261,17 +349,15 @@ fn classify_intent_llm_with_timeout( loop { let now = Instant::now(); if now >= deadline { - // TS: AbortController fires → catch → { intent: 'new' }. - return DesignIntent::New; + return parse_classified(&out); } match rx.recv_timeout(deadline - now) { Ok(ChatDelta::TextDelta(s)) => out.push_str(&s), // TS consumeSSEAsText only accumulates text chunks. Ok(ChatDelta::Thinking(_)) | Ok(ChatDelta::ToolUse { .. }) => {} - // TS: `if (!response.ok) throw` → catch → 'new'. - Ok(ChatDelta::Error(_)) => return DesignIntent::New, + Ok(ChatDelta::Error(_)) => return parse_classified(&out), Ok(ChatDelta::Done { .. }) => break, - Err(mpsc::RecvTimeoutError::Timeout) => return DesignIntent::New, + Err(mpsc::RecvTimeoutError::Timeout) => return parse_classified(&out), Err(mpsc::RecvTimeoutError::Disconnected) => break, } } diff --git a/crates/op-host-services/src/chat_intent_tests.rs b/crates/op-host-services/src/chat_intent_tests.rs index 0ff09cc48..0a57575c5 100644 --- a/crates/op-host-services/src/chat_intent_tests.rs +++ b/crates/op-host-services/src/chat_intent_tests.rs @@ -83,9 +83,9 @@ fn classification_tag_parsing_matches_ts() { // Bare DESIGN counts as new (TS `upper.includes('DESIGN')`). assert_eq!(parse_classified("This is a DESIGN task"), DesignIntent::New); assert_eq!(parse_classified("CHAT"), DesignIntent::Chat); - // Unknown / empty → new. - assert_eq!(parse_classified("gibberish"), DesignIntent::New); - assert_eq!(parse_classified(""), DesignIntent::New); + // Unknown / empty → chat: a classifier failure must not mutate the canvas. + assert_eq!(parse_classified("gibberish"), DesignIntent::Chat); + assert_eq!(parse_classified(""), DesignIntent::Chat); } // --------------------------------------------------------------------------- @@ -206,21 +206,135 @@ fn llm_classifier_parses_provider_reply() { } #[test] -fn llm_classifier_falls_back_to_new_on_error() { - // TS: classify failure → { intent: 'new' }. +fn llm_classifier_falls_back_to_chat_on_error() { let provider = Scripted::error("boom"); assert_eq!( classify_intent_llm(&provider, "anything", None), - DesignIntent::New + DesignIntent::Chat ); } #[test] -fn llm_classifier_falls_back_to_new_on_timeout() { +fn llm_classifier_falls_back_to_chat_on_timeout() { let provider = Scripted::slow("CHAT", Duration::from_millis(300)); let got = classify_intent_llm_with_timeout(&provider, "anything", None, Duration::from_millis(30)); - assert_eq!(got, DesignIntent::New, "timeout mirrors the TS abort → new"); + assert_eq!(got, DesignIntent::Chat); +} + +#[test] +fn llm_classifier_keeps_a_complete_tag_received_before_timeout() { + struct PartialThenSlow; + impl ChatProvider for PartialThenSlow { + fn provider_label(&self) -> &str { + "partial-then-slow" + } + + fn send(&self, _request: ChatRequest) -> Box + Send> { + let mut first = true; + Box::new( + vec![ + ChatDelta::TextDelta("CHAT".into()), + ChatDelta::Done { + stop_reason: StopReason::EndTurn, + }, + ] + .into_iter() + .inspect(move |_| { + if first { + first = false; + } else { + std::thread::sleep(Duration::from_millis(100)); + } + }), + ) + } + } + + assert_eq!( + classify_intent_llm_with_timeout( + &PartialThenSlow, + "anything", + None, + Duration::from_millis(30), + ), + DesignIntent::Chat + ); +} + +fn design_request(prompt: &str, model: Option<&str>, concurrency: u32) -> DesignRequest { + DesignRequest { + prompt: prompt.into(), + model: model.map(str::to_string), + provider: None, + design_md: None, + concurrency, + append_context: None, + validation_enabled: true, + visual_ref_enabled: false, + } +} + +#[test] +fn restart_command_recovers_previous_prompt_with_current_run_controls() { + let mut state = EditorState::new(); + let previous = design_request("design a travel app", Some("old/model"), 1); + let mut message = op_editor_core::ChatMessage::assistant("done"); + message.design_request_json_for_retry = Some(serde_json::to_string(&previous).unwrap()); + message.completion = Some(op_editor_core::ChatCompletion { + succeeded: 1, + failed: 0, + nodes: 10, + }); + state.chat.messages.push(message); + let current = design_request("重新开始", Some("new/model"), 4); + + let restored = restore_design_request_for_restart(&state, "重新开始？", ¤t).unwrap(); + assert_eq!(restored.prompt, "design a travel app"); + assert_eq!(restored.model.as_deref(), Some("new/model")); + assert_eq!(restored.concurrency, 4); + + let provider = Scripted::text("CHAT"); + assert_eq!( + classify_intent_for_standard_route(&provider, &state, "重新开始", None), + DesignIntent::New + ); +} + +#[test] +fn restart_skips_requests_staged_on_plain_chat_turns() { + let mut state = EditorState::new(); + let design = design_request("design a travel app", None, 1); + let mut design_message = op_editor_core::ChatMessage::assistant("done"); + design_message.design_request_json_for_retry = Some(serde_json::to_string(&design).unwrap()); + design_message.completion = Some(op_editor_core::ChatCompletion { + succeeded: 1, + failed: 0, + nodes: 10, + }); + state.chat.messages.push(design_message); + + let staged_chat = design_request("explain auto layout", None, 1); + let mut chat_message = op_editor_core::ChatMessage::assistant("Auto layout is..."); + chat_message.design_request_json_for_retry = Some(serde_json::to_string(&staged_chat).unwrap()); + state.chat.messages.push(chat_message); + + let restored = latest_design_request_for_restart(&state).unwrap(); + assert_eq!(restored.prompt, "design a travel app"); +} + +#[test] +fn restart_without_history_and_punctuation_only_stay_in_chat() { + let provider = Scripted::text("DESIGN_NEW"); + let state = EditorState::new(); + assert_eq!( + classify_intent_for_standard_route(&provider, &state, "重新开始", None), + DesignIntent::Chat + ); + assert_eq!( + classify_intent_for_standard_route(&provider, &state, "？？？？", None), + DesignIntent::Chat + ); } // --------------------------------------------------------------------------- diff --git a/crates/op-host-services/src/chat_provider_llm.rs b/crates/op-host-services/src/chat_provider_llm.rs index f78b83d73..dfd94c611 100644 --- a/crates/op-host-services/src/chat_provider_llm.rs +++ b/crates/op-host-services/src/chat_provider_llm.rs @@ -145,12 +145,15 @@ impl LlmClient for ChatProviderLlmClient { // orchestrator parses the accumulated text and // decides what to do. ChatDelta::Done { .. } => break, - // Tool calls aren't routed through the orchestrator - // — it expects a single text completion per call. - // If a CLI agent decides to invoke an MCP tool - // mid-turn the result text follows in subsequent - // `TextDelta`s anyway. - ChatDelta::ToolUse { .. } => None, + // The orchestrator expects exactly one text completion; + // silently ignoring a tool call can leave it parsing a + // truncated pre-tool response as a design script. + ChatDelta::ToolUse { name, .. } => Some(Err(LlmError { + message: format!( + "provider attempted unsupported tool `{name}` during design generation" + ), + aborted: false, + })), }; if let Some(c) = chunk { if tx.unbounded_send(c).is_err() { @@ -286,6 +289,22 @@ mod tests { } } + struct ToolEscapingProvider; + impl ChatProvider for ToolEscapingProvider { + fn provider_label(&self) -> &str { + "tool-escaping" + } + fn send(&self, _request: ChatRequest) -> Box + Send> { + Box::new( + vec![ChatDelta::ToolUse { + name: "write".into(), + args: "{}".into(), + }] + .into_iter(), + ) + } + } + fn collect_chunks(client: ChatProviderLlmClient) -> Vec> { let req = CallRequest { system_prompt: "sys".into(), @@ -344,4 +363,16 @@ mod tests { assert_eq!(text, "I(null, {\"type\":\"frame\"});"); assert_eq!(thinking, "considering the layout..."); } + + #[test] + fn tool_use_is_a_design_generation_error() { + let chunks = collect_chunks(ChatProviderLlmClient::new(Arc::new(ToolEscapingProvider))); + assert!( + chunks.iter().any(|chunk| matches!( + chunk, + Err(error) if error.message.contains("unsupported tool `write`") + )), + "{chunks:?}" + ); + } } diff --git a/crates/op-orchestrator/src/subagent.rs b/crates/op-orchestrator/src/subagent.rs index 45fa3ce83..8bf210a9d 100644 --- a/crates/op-orchestrator/src/subagent.rs +++ b/crates/op-orchestrator/src/subagent.rs @@ -312,11 +312,33 @@ pub(crate) async fn run_subtask_with_reveal_at( let Some(inserted_root_ids) = apply_insert_subtree_with_reveal( sink, nodes, - parent_id, + parent_id.clone(), indicator_epoch, reveal_started_ms, ) else { - return fail("InsertSubtree rejected by document".into()); + let state = sink.state(); + let parent_status = if !parent_id.is_real() { + "page-root" + } else { + match op_editor_core::walkers::find_node(state.active_children(), &parent_id) { + None => "missing", + Some(node) if node.is_container() => "container", + Some(_) => "non-container", + } + }; + let active_page = state + .doc + .pages + .as_ref() + .and_then(|pages| pages.get(state.ui.active_page_index)) + .map(|page| format!("{} ({})", page.name, page.id)) + .unwrap_or_else(|| "legacy page 0".into()); + let error = format!( + "InsertSubtree rejected: parent_id={} status={parent_status} active_page={active_page}", + parent_id.as_str() + ); + tracing::warn!(subtask = %subtask.id, error = %error, "subagent insert rejected"); + return fail(error); }; SubtaskOutcome { From 31d121165cfdafa0c36d01bc09db71da61852230 Mon Sep 17 00:00:00 2001 From: banana Date: Fri, 7 Aug 2026 16:18:01 +0800 Subject: [PATCH 2/3] chore(agent): drop overlapping retry fix --- .../src/chat_session_launch.rs | 31 ++------ crates/op-host-services/src/chat_intent.rs | 79 ------------------- .../op-host-services/src/chat_intent_tests.rs | 67 +--------------- 3 files changed, 7 insertions(+), 170 deletions(-) diff --git a/crates/op-host-desktop/src/chat_session_launch.rs b/crates/op-host-desktop/src/chat_session_launch.rs index e47b5c5ea..674edd14f 100644 --- a/crates/op-host-desktop/src/chat_session_launch.rs +++ b/crates/op-host-desktop/src/chat_session_launch.rs @@ -78,30 +78,24 @@ pub fn launch_if_pending( .selected_model_entry() .map(|entry| entry.builtin_provider_id.is_some() || entry.acp_agent_id().is_some()) .unwrap_or(false); - let restart_available = op_host_services::chat_intent::is_restart_command(&user_text) - && op_host_services::chat_intent::latest_design_request_for_restart(host.editor_state()) - .is_some(); if !is_builtin_or_acp { if launch_cli_standard_turn(host, &user_text, current_chat, current_design) { return true; } // CLI transport construction failed — fall through to the // honest-error path below. - } else if !restart_available && should_launch_direct_modify(host.editor_state(), &user_text) { + } else if should_launch_direct_modify(host.editor_state(), &user_text) { if launch_direct_modify_turn(host, &user_text, current_chat, current_design) { return true; } - } else if restart_available - || (!op_host_services::chat_intent::is_non_request_text(&user_text) - && matches!(classify_intent(&user_text), Intent::Design)) + } else if !op_host_services::chat_intent::is_non_request_text(&user_text) + && matches!(classify_intent(&user_text), Intent::Design) { // Phase 2.3: When the design-agent-loop flag is ON and a built-in // provider is configured, run the agentic tool-loop with the 14-tool // design toolset instead of the orchestrator pipeline. Flag OFF falls // through to the orchestrator path below — byte-for-byte unchanged. - if !restart_available - && launch_design_loop_turn(host, user_text.clone(), current_chat, current_design) - { + if launch_design_loop_turn(host, user_text.clone(), current_chat, current_design) { return true; } // Orchestrator path — unchanged when flag is OFF or no built-in @@ -124,14 +118,7 @@ pub fn launch_if_pending( &user_text, ); let initial_state = host.editor_state().clone(); - let current_request = - build_design_request(user_text.clone(), &initial_state, append_context); - let request = op_host_services::chat_intent::restore_design_request_for_restart( - &initial_state, - &user_text, - ¤t_request, - ) - .unwrap_or(current_request); + let request = build_design_request(user_text, &initial_state, append_context); // Persist the request onto the turn's assistant bubble (already // pushed by `begin_send`) BEFORE it moves into the worker — the // manual per-subtask "Retry" button needs it to re-run a failed @@ -414,14 +401,8 @@ fn launch_cli_standard_turn( let modify_plan = op_host_services::chat_intent::build_modify_plan(state, user_text); let append_context = op_host_services::chat_intent::detect_append_intent(state, user_text); let initial_state = state.clone(); - let current_request = + let design_request = build_design_request(user_text.to_string(), &initial_state, append_context); - let design_request = op_host_services::chat_intent::restore_design_request_for_restart( - &initial_state, - user_text, - ¤t_request, - ) - .unwrap_or(current_request); // Same stash as the builtin/design-intent path above — this turn may or // may not actually classify as `DesignIntent::New` on the worker (the // classifier runs async), but setting it unconditionally is harmless: diff --git a/crates/op-host-services/src/chat_intent.rs b/crates/op-host-services/src/chat_intent.rs index 60ad7c659..71073daf9 100644 --- a/crates/op-host-services/src/chat_intent.rs +++ b/crates/op-host-services/src/chat_intent.rs @@ -148,78 +148,6 @@ pub fn is_non_request_text(text: &str) -> bool { !text.chars().any(char::is_alphanumeric) } -/// Exact, standalone rerun commands. Keeping this exact avoids treating -/// requests such as "restart the server" as design-session control. -pub fn is_restart_command(text: &str) -> bool { - let normalized = text - .trim() - .trim_matches(|c: char| { - c.is_whitespace() - || matches!( - c, - '.' | ',' | '!' | '?' | ';' | ':' | '。' | '，' | '！' | '？' | '；' | '：' - ) - }) - .split_whitespace() - .collect::>() - .join(" ") - .to_lowercase(); - matches!( - normalized.as_str(), - "restart" - | "start over" - | "try again" - | "retry" - | "重新开始" - | "重新生成" - | "再试一次" - | "重来" - | "重试" - ) -} - -/// Recover the latest persisted design request from this chat tab. -pub fn latest_design_request_for_restart(state: &EditorState) -> Option { - state - .chat - .messages - .iter() - .rev() - // CLI-standard stages a request before asynchronous classification, - // even for turns that later route to plain chat. Require evidence that - // the design worker actually owned this message. - .filter(|message| { - message.completion.is_some() - || !message.activities.is_empty() - || !message.failed_subtasks.is_empty() - }) - .find_map(|message| { - message - .design_request_json_for_retry - .as_deref() - .and_then(|json| serde_json::from_str(json).ok()) - }) -} - -/// Reuse the previous design content while honoring the controls selected for -/// the new run (model/provider/concurrency/validation). -pub fn restore_design_request_for_restart( - state: &EditorState, - text: &str, - current: &DesignRequest, -) -> Option { - if !is_restart_command(text) { - return None; - } - let mut previous = latest_design_request_for_restart(state)?; - previous.model = current.model.clone(); - previous.provider = current.provider.clone(); - previous.concurrency = current.concurrency; - previous.validation_enabled = current.validation_enabled; - previous.visual_ref_enabled = current.visual_ref_enabled; - Some(previous) -} - /// TS `classifyByKeywords` — verbatim rule order. pub fn classify_by_keywords(text: &str) -> DesignIntent { let lower = text.to_lowercase(); @@ -250,13 +178,6 @@ pub fn classify_intent_for_standard_route( text: &str, model: Option, ) -> DesignIntent { - if is_restart_command(text) { - return if latest_design_request_for_restart(state).is_some() { - DesignIntent::New - } else { - DesignIntent::Chat - }; - } if is_non_request_text(text) { return DesignIntent::Chat; } diff --git a/crates/op-host-services/src/chat_intent_tests.rs b/crates/op-host-services/src/chat_intent_tests.rs index 0a57575c5..491b9bd70 100644 --- a/crates/op-host-services/src/chat_intent_tests.rs +++ b/crates/op-host-services/src/chat_intent_tests.rs @@ -262,75 +262,10 @@ fn llm_classifier_keeps_a_complete_tag_received_before_timeout() { ); } -fn design_request(prompt: &str, model: Option<&str>, concurrency: u32) -> DesignRequest { - DesignRequest { - prompt: prompt.into(), - model: model.map(str::to_string), - provider: None, - design_md: None, - concurrency, - append_context: None, - validation_enabled: true, - visual_ref_enabled: false, - } -} - #[test] -fn restart_command_recovers_previous_prompt_with_current_run_controls() { - let mut state = EditorState::new(); - let previous = design_request("design a travel app", Some("old/model"), 1); - let mut message = op_editor_core::ChatMessage::assistant("done"); - message.design_request_json_for_retry = Some(serde_json::to_string(&previous).unwrap()); - message.completion = Some(op_editor_core::ChatCompletion { - succeeded: 1, - failed: 0, - nodes: 10, - }); - state.chat.messages.push(message); - let current = design_request("重新开始", Some("new/model"), 4); - - let restored = restore_design_request_for_restart(&state, "重新开始？", ¤t).unwrap(); - assert_eq!(restored.prompt, "design a travel app"); - assert_eq!(restored.model.as_deref(), Some("new/model")); - assert_eq!(restored.concurrency, 4); - - let provider = Scripted::text("CHAT"); - assert_eq!( - classify_intent_for_standard_route(&provider, &state, "重新开始", None), - DesignIntent::New - ); -} - -#[test] -fn restart_skips_requests_staged_on_plain_chat_turns() { - let mut state = EditorState::new(); - let design = design_request("design a travel app", None, 1); - let mut design_message = op_editor_core::ChatMessage::assistant("done"); - design_message.design_request_json_for_retry = Some(serde_json::to_string(&design).unwrap()); - design_message.completion = Some(op_editor_core::ChatCompletion { - succeeded: 1, - failed: 0, - nodes: 10, - }); - state.chat.messages.push(design_message); - - let staged_chat = design_request("explain auto layout", None, 1); - let mut chat_message = op_editor_core::ChatMessage::assistant("Auto layout is..."); - chat_message.design_request_json_for_retry = Some(serde_json::to_string(&staged_chat).unwrap()); - state.chat.messages.push(chat_message); - - let restored = latest_design_request_for_restart(&state).unwrap(); - assert_eq!(restored.prompt, "design a travel app"); -} - -#[test] -fn restart_without_history_and_punctuation_only_stay_in_chat() { +fn punctuation_only_stays_in_chat() { let provider = Scripted::text("DESIGN_NEW"); let state = EditorState::new(); - assert_eq!( - classify_intent_for_standard_route(&provider, &state, "重新开始", None), - DesignIntent::Chat - ); assert_eq!( classify_intent_for_standard_route(&provider, &state, "？？？？", None), DesignIntent::Chat From 17b446a2c6a0bc2a4bd6427ee4e2106bc1e87eaf Mon Sep 17 00:00:00 2001 From: banana Date: Fri, 7 Aug 2026 18:19:42 +0800 Subject: [PATCH 3/3] fix(agent): surface actionable failure diagnostics --- crates/op-editor-core/src/chat.rs | 10 +-- .../src/widgets/ai_chat_transcript.rs | 7 ++- .../widgets/ai_chat_transcript_cache_tests.rs | 2 +- .../src/widgets/ai_chat_transcript_flow.rs | 2 +- .../src/widgets/ai_chat_transcript_tests.rs | 35 +++++++++++ crates/op-host-desktop/src/design_session.rs | 6 +- .../src/design_session_terminal_tests.rs | 11 ++++ .../src/design_session_tests.rs | 36 ++++++++++- .../src/design_session_worker_tests.rs | 29 ++++++++- .../src/design_session_workers.rs | 63 +++++++++++++------ crates/op-i18n/src/i18n/en.rs | 16 ++++- crates/op-i18n/src/i18n/zh_cn.rs | 10 ++- 12 files changed, 189 insertions(+), 38 deletions(-) diff --git a/crates/op-editor-core/src/chat.rs b/crates/op-editor-core/src/chat.rs index 99c46265a..4c6732ec4 100644 --- a/crates/op-editor-core/src/chat.rs +++ b/crates/op-editor-core/src/chat.rs @@ -230,7 +230,7 @@ pub struct ChatMessage { pub design_block_expanded_overrides: Vec>, /// Per-action-step (subtask card) expanded-state overrides. Missing /// / `None` entries fall back to the transcript default (expanded - /// only while the step is the active/streaming one). + /// while the step is active or failed, so diagnostics stay visible). pub action_step_expanded_overrides: Vec>, /// True while this (assistant) message's turn streams in. pub streaming: bool, @@ -869,8 +869,8 @@ impl ChatState { /// Begin a manual retry for the failed subtask row at /// `activities[source_index]` in message `msg_idx` — the click handler /// for the progress panel's per-row "Retry" button. Flips that - /// activity's status back to `Running` and clears its stale "Needs - /// attention" detail so the row shows a spinner immediately, then + /// activity's status back to `Running` and clears its stale failure + /// diagnostic so the row shows a spinner immediately, then /// raises `pending_subtask_retry` for the desktop host to drain. /// /// No-ops (leaves everything untouched) when the message/activity index @@ -1355,7 +1355,7 @@ mod tests { msg.activities.push(ChatActivity { id: "hero".into(), title: "Hero".into(), - detail: Some("Needs attention".into()), + detail: Some("Reason: provider returned no nodes".into()), status: ChatActivityStatus::Error, content_offset: Some(0), }); @@ -1391,7 +1391,7 @@ mod tests { msg.activities.push(ChatActivity { id: "hero".into(), title: "Hero".into(), - detail: Some("Needs attention".into()), + detail: Some("Reason: provider returned no nodes".into()), status: ChatActivityStatus::Error, content_offset: Some(0), }); diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript.rs index aa7e28a5e..7197c3681 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript.rs @@ -317,14 +317,15 @@ pub(crate) fn build_item( .iter() .flat_map(|line| wrap_units(line, budget.saturating_sub(4))) .collect(); - // Default: expanded only while this step is the active/streaming - // one. A user click records a per-step override (collapse/expand). + // Active work and failures default open: a terminal error must expose + // its concrete diagnostic without making the user discover a hidden + // accordion. A user click still records an explicit override. let expanded = msg .action_step_expanded_overrides .get(i) .copied() .flatten() - .unwrap_or(active); + .unwrap_or(active || failed); let step_h = action_step_height(expanded, details.len()); // `i` only aligns with `msg.activities`' own index when the // structured (non-interleaved) path built `progress_steps` directly diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript_cache_tests.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript_cache_tests.rs index 76150da0d..5ed950e3b 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript_cache_tests.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript_cache_tests.rs @@ -116,7 +116,7 @@ fn failed_subtask_message(retryable: bool) -> ChatMessage { m.activities.push(op_editor_core::ChatActivity { id: "hero".into(), title: "Hero".into(), - detail: Some("Needs attention".into()), + detail: Some("Reason: provider returned no nodes".into()), status: op_editor_core::ChatActivityStatus::Error, content_offset: Some(0), }); diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript_flow.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript_flow.rs index 12ca94769..820f83b8e 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript_flow.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript_flow.rs @@ -98,7 +98,7 @@ pub(crate) fn build_activity_flow( .get(source_index) .copied() .flatten() - .unwrap_or(active); + .unwrap_or(active || failed); let height = action_step_height(expanded, details.len()); // `index` is a direct `msg.activities` index here (unlike the // legacy thinking-text-derived steps in `build_item`), so the diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests.rs index bd808abf3..0d386552d 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests.rs @@ -628,6 +628,41 @@ fn structured_activities_render_as_compact_rows_without_thinking_text() { assert!(items[0].bubble.is_none()); } +#[test] +fn failed_structured_activity_defaults_open_to_show_its_diagnostic() { + let mut message = ChatMessage::assistant("Finished with issues"); + message.activities.push(op_editor_core::ChatActivity { + id: "customer-table".into(), + title: "Customer Table".into(), + detail: Some("Reason: parent_id=dashboard was not found".into()), + status: op_editor_core::ChatActivityStatus::Error, + content_offset: Some(0), + }); + + let items = build_transcript( + std::slice::from_ref(&message), + body(), + op_editor_core::Locale::EnUs, + ); + assert!(items[0].steps[0].failed); + assert!(items[0].steps[0].expanded); + assert_eq!( + items[0].steps[0].details, + vec!["Reason: parent_id=dashboard was not found"] + ); + + message.action_step_expanded_overrides = vec![Some(false)]; + let collapsed = build_transcript( + std::slice::from_ref(&message), + body(), + op_editor_core::Locale::EnUs, + ); + assert!( + !collapsed[0].steps[0].expanded, + "an explicit user collapse must still win over the failure default" + ); +} + #[test] fn structured_activities_interleave_with_cli_narration_by_offset() { let first = "I mapped the screen."; diff --git a/crates/op-host-desktop/src/design_session.rs b/crates/op-host-desktop/src/design_session.rs index 3fe5133b0..01067322f 100644 --- a/crates/op-host-desktop/src/design_session.rs +++ b/crates/op-host-desktop/src/design_session.rs @@ -534,16 +534,16 @@ fn element_count(locale: Locale, count: usize) -> String { } fn subtask_failure_detail(locale: Locale, error: &str) -> String { - let label = op_i18n::translate(locale, "ai.designProgress.detail.needsAttention"); let compact = error.split_whitespace().collect::>().join(" "); if compact.is_empty() { - return label.into(); + return op_i18n::translate(locale, "ai.designProgress.detail.noDiagnostic").into(); } let mut visible: String = compact.chars().take(220).collect(); if compact.chars().count() > 220 { visible.push('…'); } - format!("{label}: {visible}") + op_i18n::translate(locale, "ai.designProgress.detail.failureReason") + .replace("{{reason}}", &visible) } fn planned_narration(locale: Locale, count: usize) -> String { diff --git a/crates/op-host-desktop/src/design_session_terminal_tests.rs b/crates/op-host-desktop/src/design_session_terminal_tests.rs index b1a1a2b26..3de8f8b32 100644 --- a/crates/op-host-desktop/src/design_session_terminal_tests.rs +++ b/crates/op-host-desktop/src/design_session_terminal_tests.rs @@ -151,6 +151,17 @@ fn companion_chat_disconnect_cannot_drop_validation_completion_or_retry_payload( .status, ChatActivityStatus::Error ); + assert_eq!( + message + .activities + .iter() + .find(|activity| activity.id == "sun_arc") + .unwrap() + .detail + .as_deref(), + Some("Reason: self-check failed"), + "the terminal summary must preserve the concrete subtask failure" + ); assert_eq!(message.failed_subtasks.len(), 1); assert_eq!(message.failed_subtasks[0].subtask_id, "sun_arc"); assert_eq!( diff --git a/crates/op-host-desktop/src/design_session_tests.rs b/crates/op-host-desktop/src/design_session_tests.rs index fa6b7eb6f..a0e625995 100644 --- a/crates/op-host-desktop/src/design_session_tests.rs +++ b/crates/op-host-desktop/src/design_session_tests.rs @@ -257,6 +257,13 @@ fn pump_progress_captures_failed_subtask_specs_for_manual_retry() { .messages .last() .expect("seeded bubble survives"); + assert_eq!(msg.activities.len(), 1); + assert_eq!(msg.activities[0].title, "Hero"); + assert_eq!( + msg.activities[0].detail.as_deref(), + Some("Reason: empty content from provider"), + "a summary-only failure must still identify its section and exact cause" + ); assert_eq!(msg.failed_subtasks.len(), 1, "{:?}", msg.failed_subtasks); assert_eq!(msg.failed_subtasks[0].subtask_id, "hero"); let restored: op_orchestrator::plan::Subtask = @@ -685,10 +692,37 @@ fn failed_subtask_keeps_the_actionable_error_in_the_activity() { Locale::EnUs, )); let detail = message.activities[0].detail.as_deref().unwrap(); - assert!(detail.contains("Needs attention"), "{detail}"); + assert!(detail.starts_with("Reason:"), "{detail}"); + assert!(!detail.contains("Needs attention"), "{detail}"); assert!(detail.contains("parent_id=root status=missing"), "{detail}"); } +#[test] +fn failed_subtask_uses_localized_reason_label() { + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + assert!(super::apply_progress( + &mut message, + &[Progress::SubtaskFailed { + id: "customer-table".into(), + error: "parent_id=dashboard was not found".into(), + }], + Locale::ZhCn, + )); + + assert_eq!( + message.activities[0].detail.as_deref(), + Some("失败原因：parent_id=dashboard was not found") + ); +} + +#[test] +fn failed_subtask_without_provider_detail_reports_the_missing_diagnostic() { + assert_eq!( + super::subtask_failure_detail(Locale::ZhCn, " \n\t"), + "Agent 执行失败，但没有返回错误说明。" + ); +} + #[test] fn cli_progress_uses_the_editor_locale_for_visible_process_and_summary() { let mut message = op_editor_core::ChatMessage::assistant_streaming(); diff --git a/crates/op-host-desktop/src/design_session_worker_tests.rs b/crates/op-host-desktop/src/design_session_worker_tests.rs index 0fe35d518..fe744a5ef 100644 --- a/crates/op-host-desktop/src/design_session_worker_tests.rs +++ b/crates/op-host-desktop/src/design_session_worker_tests.rs @@ -248,7 +248,9 @@ fn worker_summary_finishes_all_messages_and_keeps_retry_on_owning_worker() { assert_eq!(worker.failed_subtasks.len(), 1); assert_eq!(worker.failed_subtasks[0].subtask_id, "hero"); assert_eq!(worker.activities[0].status, ChatActivityStatus::Error); - assert!(worker.content.contains("need attention")); + assert!(worker + .content + .contains("failed sections are expanded with their reasons")); } #[test] @@ -290,12 +292,20 @@ fn terminal_design_error_stops_primary_and_every_worker_message() { assert!(messages.iter().all(|message| !message.streaming)); assert!(messages[0].content.contains("error:")); assert_eq!(messages[0].activities[0].status, ChatActivityStatus::Error); + assert!(messages[0].activities[0] + .detail + .as_deref() + .is_some_and(|detail| detail.starts_with("Reason:") && detail.contains("boom"))); let worker = messages .iter() .find(|message| message.design_worker_group == Some(1)) .unwrap(); assert!(worker.content.contains("Stopped designing")); assert_eq!(worker.activities[0].status, ChatActivityStatus::Error); + assert!(worker.activities[0] + .detail + .as_deref() + .is_some_and(|detail| detail.starts_with("Reason:") && detail.contains("boom"))); } #[test] @@ -304,6 +314,7 @@ fn disconnected_session_marks_active_worker_rows_error_before_stopping() { let (_cmd_tx, cmd_rx) = mpsc::channel::(); let mut current = Some(DesignSession::from_channels(delta_rx, cmd_rx)); let mut host = WidgetHostNative::new(); + host.editor_state_mut().editor_ui.locale = Locale::EnUs; host.editor_state_mut() .chat .messages @@ -332,6 +343,10 @@ fn disconnected_session_marks_active_worker_rows_error_before_stopping() { .unwrap(); assert!(!worker.streaming); assert_eq!(worker.activities[0].status, ChatActivityStatus::Error); + assert_eq!( + worker.activities[0].detail.as_deref(), + Some("The agent connection closed before this section returned a result.") + ); } #[test] @@ -566,6 +581,13 @@ fn partial_summary_marks_omitted_active_rows_error() { assert_eq!(row("trips").status, ChatActivityStatus::Done); assert_eq!(row("profile").status, ChatActivityStatus::Error); assert_eq!(row("saved").status, ChatActivityStatus::Error); + for id in ["profile", "saved"] { + assert_eq!( + row(id).detail.as_deref(), + Some("The agent stopped before returning a result for this section."), + "omitted summary row {id} needs a concrete terminal reason" + ); + } assert!(messages .iter() .filter(|message| message.role == ChatRole::Assistant) @@ -580,7 +602,10 @@ fn partial_summary_marks_omitted_active_rows_error() { let primary = &messages[1]; assert_eq!(primary.completion.unwrap().succeeded, 1); assert_eq!(primary.completion.unwrap().failed, 2); - assert!(primary.content.contains("2 need attention")); + assert!(primary.content.contains("2 failed")); + assert!(primary + .content + .contains("failed sections below show the exact reasons")); } #[test] diff --git a/crates/op-host-desktop/src/design_session_workers.rs b/crates/op-host-desktop/src/design_session_workers.rs index 96e9f7df8..7fb48ae82 100644 --- a/crates/op-host-desktop/src/design_session_workers.rs +++ b/crates/op-host-desktop/src/design_session_workers.rs @@ -5,7 +5,7 @@ use op_orchestrator::{Progress, RunSummary, WorkerEvent}; use super::{ append_completion_narration, append_narration, apply_progress, count_u32, friendly_quota_error, - update_activity, + subtask_failure_detail, update_activity, upsert_activity, }; /// Route global progress to the primary message and worker-scoped progress to @@ -336,8 +336,7 @@ pub(super) fn finish_design_success( None => { activity.status = ChatActivityStatus::Error; activity.detail = Some( - op_i18n::translate(locale, "ai.designProgress.detail.needsAttention") - .into(), + op_i18n::translate(locale, "ai.designProgress.detail.noResult").into(), ); unreported_active += 1; if is_worker && !stopped_workers.contains(&index) { @@ -359,13 +358,33 @@ pub(super) fn finish_design_success( .any(|activity| activity.id == outcome.id) }) .unwrap_or(primary); - if outcome.error.is_some() { - update_activity( - &mut messages[target], - &outcome.id, - ChatActivityStatus::Error, - Some(op_i18n::translate(locale, "ai.designProgress.detail.needsAttention").into()), - ); + if let Some(error) = outcome.error.as_deref() { + let detail = Some(subtask_failure_detail(locale, error)); + if messages[target] + .activities + .iter() + .any(|activity| activity.id == outcome.id) + { + update_activity( + &mut messages[target], + &outcome.id, + ChatActivityStatus::Error, + detail, + ); + } else { + let title = outcome + .subtask + .as_ref() + .map(|subtask| subtask.label.as_str()) + .unwrap_or(outcome.id.as_str()); + upsert_activity( + &mut messages[target], + &outcome.id, + title, + ChatActivityStatus::Error, + detail, + ); + } } if let Some(subtask) = &outcome.subtask { if let Ok(subtask_json) = serde_json::to_string(subtask) { @@ -433,8 +452,9 @@ pub(super) fn finish_design_error(messages: &mut [ChatMessage], raw: &str, local else { return false; }; + let detail = subtask_failure_detail(locale, raw); for &index in &indices { - mark_active_activities_error(&mut messages[index]); + mark_active_activities_error(&mut messages[index], &detail); if messages[index].design_worker_group.is_some() { let terminal = worker_stopped_narration(locale, messages[index].design_worker_screen.as_deref()); @@ -463,6 +483,7 @@ pub(super) fn finish_disconnected_design_messages( // durable design ownership here so that channel cannot terminate the real // plain-chat bubble. let indices = current_owned_design_message_indices(messages); + let detail = op_i18n::translate(locale, "ai.designProgress.detail.connectionClosed"); for &index in &indices { let had_active_activity = messages[index].activities.iter().any(|activity| { matches!( @@ -470,7 +491,7 @@ pub(super) fn finish_disconnected_design_messages( ChatActivityStatus::Pending | ChatActivityStatus::Running ) }); - mark_active_activities_error(&mut messages[index]); + mark_active_activities_error(&mut messages[index], detail); // A manual retry closes its channel after sending SubtaskDone/Failed // instead of a whole-turn summary. In that normal path the row is // already terminal, so merely stop streaming; only an abrupt @@ -527,6 +548,7 @@ pub(super) fn stop_design_messages(messages: &mut [ChatMessage], locale: Locale) } let mut changed = false; + let detail = op_i18n::translate(locale, "ai.designProgress.detail.stoppedByUser"); for index in indices { let had_active = messages[index].activities.iter().any(|activity| { matches!( @@ -534,7 +556,7 @@ pub(super) fn stop_design_messages(messages: &mut [ChatMessage], locale: Locale) ChatActivityStatus::Pending | ChatActivityStatus::Running ) }); - changed |= mark_active_activities_error(&mut messages[index]); + changed |= mark_active_activities_error(&mut messages[index], detail); if messages[index].design_worker_group.is_some() && had_active { let terminal = worker_stopped_narration(locale, messages[index].design_worker_screen.as_deref()); @@ -548,15 +570,18 @@ pub(super) fn stop_design_messages(messages: &mut [ChatMessage], locale: Locale) changed } -fn mark_active_activities_error(message: &mut ChatMessage) -> bool { +fn mark_active_activities_error(message: &mut ChatMessage, detail: &str) -> bool { let mut changed = false; for activity in &mut message.activities { if matches!( activity.status, ChatActivityStatus::Pending | ChatActivityStatus::Running ) { + let next_detail = Some(detail.to_owned()); + changed |= + activity.status != ChatActivityStatus::Error || activity.detail != next_detail; activity.status = ChatActivityStatus::Error; - changed = true; + activity.detail = next_detail; } } changed @@ -581,11 +606,13 @@ fn worker_finished_narration(locale: Locale, screen: Option<&str>, has_error: bo .unwrap_or("screen"); match (locale, has_error) { (Locale::ZhCn, false) => format!("**{screen}** 已完成。"), - (Locale::ZhCn, true) => format!("**{screen}** 已完成，但有项目需要处理。"), + (Locale::ZhCn, true) => format!("**{screen}** 已结束；失败区块已展开并标明具体原因。"), (Locale::ZhTw, false) => format!("**{screen}** 已完成。"), - (Locale::ZhTw, true) => format!("**{screen}** 已完成，但有項目需要處理。"), + (Locale::ZhTw, true) => format!("**{screen}** 已結束；失敗區塊已展開並標明具體原因。"), (_, false) => format!("Finished **{screen}**."), - (_, true) => format!("Finished **{screen}** with items that need attention."), + (_, true) => { + format!("Finished **{screen}**; failed sections are expanded with their reasons.") + } } } diff --git a/crates/op-i18n/src/i18n/en.rs b/crates/op-i18n/src/i18n/en.rs index 68d59f941..a46ddbef8 100644 --- a/crates/op-i18n/src/i18n/en.rs +++ b/crates/op-i18n/src/i18n/en.rs @@ -681,7 +681,19 @@ pub fn lookup(key: &str) -> Option<&'static str> { "ai.designProgress.activity.polishing" => "Polishing the layout", "ai.designProgress.activity.checking" => "Checking the design", "ai.designProgress.activity.visualReference" => "Preparing the visual reference", - "ai.designProgress.detail.needsAttention" => "Needs attention", + "ai.designProgress.detail.failureReason" => "Reason: {{reason}}", + "ai.designProgress.detail.noDiagnostic" => { + "The agent failed without returning an error description." + } + "ai.designProgress.detail.noResult" => { + "The agent stopped before returning a result for this section." + } + "ai.designProgress.detail.connectionClosed" => { + "The agent connection closed before this section returned a result." + } + "ai.designProgress.detail.stoppedByUser" => { + "Stopped by the user before this section completed." + } "ai.designProgress.detail.retrying" => "Retrying · attempt {{attempt}}", "ai.designProgress.detail.refining" => "Refining details", "ai.designProgress.detail.standardPath" => "Using the standard design path", @@ -697,7 +709,7 @@ pub fn lookup(key: &str) -> Option<&'static str> { "Done — all {{count}} planned sections are in place and the final layout has been checked." } "ai.designProgress.completion.issues" => { - "Finished with issues — {{completed}} completed and {{failed}} need attention." + "Finished with issues — {{completed}} completed and {{failed}} failed. The failed sections below show the exact reasons." } "figma.importNotWired" => ".fig file import not yet wired", "dialog.loadErrorInvalidUtf8" => "The file is not valid UTF-8 text: {{detail}}", diff --git a/crates/op-i18n/src/i18n/zh_cn.rs b/crates/op-i18n/src/i18n/zh_cn.rs index bf5b0ff94..7982d2841 100644 --- a/crates/op-i18n/src/i18n/zh_cn.rs +++ b/crates/op-i18n/src/i18n/zh_cn.rs @@ -689,7 +689,13 @@ pub fn lookup(key: &str) -> Option<&'static str> { "ai.designProgress.activity.polishing" => "润色布局", "ai.designProgress.activity.checking" => "检查设计", "ai.designProgress.activity.visualReference" => "准备视觉参考", - "ai.designProgress.detail.needsAttention" => "需要处理", + "ai.designProgress.detail.failureReason" => "失败原因：{{reason}}", + "ai.designProgress.detail.noDiagnostic" => "Agent 执行失败，但没有返回错误说明。", + "ai.designProgress.detail.noResult" => "Agent 在该区块返回结果前已停止。", + "ai.designProgress.detail.connectionClosed" => { + "Agent 连接在该区块返回结果前已断开。" + } + "ai.designProgress.detail.stoppedByUser" => "用户已停止，该区块尚未完成。", "ai.designProgress.detail.retrying" => "正在重试 · 第 {{attempt}} 次", "ai.designProgress.detail.refining" => "正在润色细节", "ai.designProgress.detail.standardPath" => "改用标准设计流程", @@ -703,7 +709,7 @@ pub fn lookup(key: &str) -> Option<&'static str> { "已完成——计划中的 {{count}} 个区块已全部就位，最终布局也已检查。" } "ai.designProgress.completion.issues" => { - "本次已结束，但仍有问题——已完成 {{completed}} 项，{{failed}} 项需要处理。" + "本次已结束——已完成 {{completed}} 项，失败 {{failed}} 项。下方失败区块已标明具体原因。" } "figma.importNotWired" => ".fig 文件导入尚未接入", "dialog.loadErrorInvalidUtf8" => "文件不是有效的 UTF-8 文本:{{detail}}",