From a02b124aaef4a06f8240421a6b6e4a5b814d89b4 Mon Sep 17 00:00:00 2001 From: Fini Date: Tue, 14 Jul 2026 00:29:08 +0800 Subject: [PATCH] fix(desktop): align design session identity and lifecycle --- crates/op-host-desktop/src/chat_session.rs | 68 +++++++++++++- .../src/chat_session_launch_design.rs | 1 + .../src/chat_session_transcript_tests.rs | 52 +++++++++++ .../src/design_loop_indicator.rs | 53 ++++++++++- crates/op-host-desktop/src/keyboard_input.rs | 14 +++ .../src/keyboard_input_tests.rs | 90 +++++++++++++++++++ crates/op-host-desktop/src/persistence.rs | 49 ++++++++++ .../op-host-desktop/src/sub_agent_session.rs | 7 ++ .../src/sub_agent_session_tests.rs | 19 ++++ 9 files changed, 349 insertions(+), 4 deletions(-) create mode 100644 crates/op-host-desktop/src/chat_session_transcript_tests.rs diff --git a/crates/op-host-desktop/src/chat_session.rs b/crates/op-host-desktop/src/chat_session.rs index 474948d0a..48fbbb732 100644 --- a/crates/op-host-desktop/src/chat_session.rs +++ b/crates/op-host-desktop/src/chat_session.rs @@ -322,8 +322,7 @@ fn attach_tool_result_to_transcript_with( if obj.get("status").and_then(serde_json::Value::as_str) != Some("running") { continue; } - let result_value = serde_json::from_str::(&result.content) - .unwrap_or_else(|_| serde_json::Value::String(result.content.clone())); + let result_value = transcript_result_value(name, result); obj.insert("result".into(), result_value); let status = if result.is_error { "error" } else { "done" }; obj.insert( @@ -336,10 +335,75 @@ fn attach_tool_result_to_transcript_with( false } +/// Keep screenshot bytes on the live executor acknowledgement, but never copy +/// them into the long-lived UI transcript. The agent loop still receives the +/// original [`ChatToolResult`]; only the tool-card display value is summarized. +fn transcript_result_value(name: &str, result: &ChatToolResult) -> serde_json::Value { + let mut value = serde_json::from_str::(&result.content) + .unwrap_or_else(|_| serde_json::Value::String(result.content.clone())); + if name != "get_screenshot" || result.is_error { + return value; + } + if value.get("success").and_then(serde_json::Value::as_bool) == Some(false) { + return value; + } + + let payload = if value.get("data").is_some_and(serde_json::Value::is_object) { + value + .get_mut("data") + .and_then(serde_json::Value::as_object_mut) + } else { + value.as_object_mut() + }; + let Some(payload) = payload else { + return value; + }; + let Some(serde_json::Value::String(encoded)) = payload.remove("image_base64") else { + return value; + }; + + let encoded_chars = encoded.len(); + payload.insert( + "image_summary".into(), + serde_json::Value::String( + "Screenshot rendered; binary payload omitted from the UI transcript.".into(), + ), + ); + payload.insert("image_base64_chars".into(), encoded_chars.into()); + if let Some(decoded_bytes) = standard_base64_decoded_len(&encoded) { + payload.insert("image_bytes".into(), decoded_bytes.into()); + } + value +} + +/// Exact decoded length for canonical padded base64 without allocating a +/// second screenshot-sized buffer. Production `get_screenshot` uses this form. +fn standard_base64_decoded_len(encoded: &str) -> Option { + if encoded.is_empty() || !encoded.len().is_multiple_of(4) { + return None; + } + let padding = encoded + .as_bytes() + .iter() + .rev() + .take_while(|byte| **byte == b'=') + .count() + .min(2); + encoded + .len() + .checked_div(4)? + .checked_mul(3)? + .checked_sub(padding) +} + #[cfg(test)] #[path = "chat_session_tests.rs"] mod tests; +#[cfg(test)] +#[path = "chat_session_transcript_tests.rs"] +mod transcript_tests; + #[cfg(test)] #[path = "chat_session_identity_tests.rs"] mod identity_tests; diff --git a/crates/op-host-desktop/src/chat_session_launch_design.rs b/crates/op-host-desktop/src/chat_session_launch_design.rs index ea4cfe7ac..d1ba507c9 100644 --- a/crates/op-host-desktop/src/chat_session_launch_design.rs +++ b/crates/op-host-desktop/src/chat_session_launch_design.rs @@ -213,6 +213,7 @@ pub(super) fn launch_design_loop_turn( { system_prompt.push_str("\n\n---\n\n"); system_prompt.push_str(&brief); + op_ai_skills::append_image_self_check_scope(&mut system_prompt); } let req = ChatRequest { system_prompt, diff --git a/crates/op-host-desktop/src/chat_session_transcript_tests.rs b/crates/op-host-desktop/src/chat_session_transcript_tests.rs new file mode 100644 index 000000000..69a713d6f --- /dev/null +++ b/crates/op-host-desktop/src/chat_session_transcript_tests.rs @@ -0,0 +1,52 @@ +//! Transcript-only result compaction tests. + +use super::*; +use op_editor_core::ChatToolCall; + +#[test] +fn production_screenshot_envelope_keeps_metadata_but_drops_base64_from_transcript() { + let mut chat = ChatState::default(); + let mut message = ChatMessage::assistant_streaming(); + message.tool_calls.push(ChatToolCall { + name: "get_screenshot".into(), + args: r#"{"level":"read","args":{"nodeId":"root"},"status":"running"}"#.into(), + content_offset: None, + }); + chat.messages.push(message); + + let original = serde_json::json!({ + "success": true, + "data": { + "image_base64": "QUJDRA==", + "format": "png" + } + }) + .to_string(); + let result = ChatToolResult { + content: original.clone(), + is_error: false, + }; + + assert!(attach_tool_result_to_transcript( + &mut chat, + "get_screenshot", + &result + )); + + let card: serde_json::Value = + serde_json::from_str(&chat.messages[0].tool_calls[0].args).unwrap(); + assert_eq!(card["status"], "done"); + assert_eq!(card["result"]["success"], true); + assert_eq!(card["result"]["data"]["format"], "png"); + assert_eq!(card["result"]["data"]["image_base64_chars"], 8); + assert_eq!(card["result"]["data"]["image_bytes"], 4); + assert!(card["result"]["data"]["image_summary"] + .as_str() + .is_some_and(|summary| summary.contains("omitted from the UI transcript"))); + assert!(card["result"]["data"].get("image_base64").is_none()); + assert!(!chat.messages[0].tool_calls[0].args.contains("QUJDRA==")); + + // Transcript compaction is display-only: the executor acknowledgement + // consumed by the in-flight agent loop remains byte-for-byte unchanged. + assert_eq!(result.content, original); +} diff --git a/crates/op-host-desktop/src/design_loop_indicator.rs b/crates/op-host-desktop/src/design_loop_indicator.rs index 83062f900..bc9ef8d87 100644 --- a/crates/op-host-desktop/src/design_loop_indicator.rs +++ b/crates/op-host-desktop/src/design_loop_indicator.rs @@ -204,11 +204,40 @@ pub(super) fn pump_design_session_indicator( .into_iter() .next() .expect("assign_agent_identities(1) always yields one"); + // The transcript is the source of truth for a single-agent + // design turn. CLI turns are stamped by `ChatState::begin_send` + // (for example "Claude Code"), while builtin turns use their + // provider display name. Reusing that label keeps the canvas + // cursor and chat bubble from presenting two different agents. + // A persona remains the fallback for harnesses that do not seed + // a streaming assistant message; its colour also supplies the + // canvas accent when the transcript has no orchestrated colour. + let (name, color) = state + .chat + .messages + .iter() + .rev() + .find(|message| { + message.role == op_editor_core::ChatRole::Assistant && message.streaming + }) + .map(|message| { + ( + message + .agent_name + .clone() + .unwrap_or_else(|| id.name.clone()), + message + .agent_color + .clone() + .unwrap_or_else(|| id.color.clone()), + ) + }) + .unwrap_or((id.name, id.color)); let initial = collect_top_level_frame_ids(state); *indicator = Some(DesignLoopIndicator { epoch, - color: id.color, - name: id.name, + color, + name, initial_frame_ids: initial, }); } @@ -408,6 +437,26 @@ mod tests { ); } + #[test] + fn design_session_indicator_reuses_streaming_transcript_identity() { + let _guard = lock_agent_indicators(); + agent_indicators::clear(); + let mut state = make_state(); + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + message.agent_name = Some("Claude Code".into()); + state.chat.messages.push(message); + let (session, _epoch) = design_session_with_epoch(); + let current = Some(session); + let mut indicator: Option = None; + + pump_design_session_indicator(&mut indicator, ¤t, &state); + + assert_eq!( + indicator.as_ref().map(|value| value.name.as_str()), + Some("Claude Code") + ); + } + #[test] fn design_session_indicator_teardown_clears_local_handle_only() { let _guard = lock_agent_indicators(); diff --git a/crates/op-host-desktop/src/keyboard_input.rs b/crates/op-host-desktop/src/keyboard_input.rs index 809035bf2..e9f78fe14 100644 --- a/crates/op-host-desktop/src/keyboard_input.rs +++ b/crates/op-host-desktop/src/keyboard_input.rs @@ -549,12 +549,24 @@ impl DesktopApp { /// Drain a New Chat request (the widget handler already opened the fresh /// tab). Aborts any in-flight worker and clears the now-stale tab binding. pub(crate) fn drain_new_chat(&mut self) -> bool { + let running_tab = self.chat_running_tab; let drained = chat_session::drain_new_chat_request( &mut self.host, &mut self.current_chat, &mut self.current_design, ); if drained { + crate::sub_agent_session::abort_all(&mut self.sub_agents, &mut self.active_sub_agent); + if let Some(chat) = + running_tab.and_then(|idx| self.host.editor_state_mut().chat.tab_mut(idx)) + { + chat.agents_running = (0, 0); + chat.pending_send = None; + chat.pending_stop_chat = false; + for message in &mut chat.messages { + message.streaming = false; + } + } self.chat_running_tab = None; } drained @@ -569,6 +581,8 @@ impl DesktopApp { &mut self.current_design, ); if drained { + crate::sub_agent_session::abort_all(&mut self.sub_agents, &mut self.active_sub_agent); + self.host.editor_state_mut().chat.agents_running = (0, 0); self.chat_running_tab = None; } drained diff --git a/crates/op-host-desktop/src/keyboard_input_tests.rs b/crates/op-host-desktop/src/keyboard_input_tests.rs index f62bda00e..1a6b2f264 100644 --- a/crates/op-host-desktop/src/keyboard_input_tests.rs +++ b/crates/op-host-desktop/src/keyboard_input_tests.rs @@ -231,3 +231,93 @@ fn internal_node_clipboard_remains_the_canvas_fallback() { Some(PenNode::Image(_)) )); } + +#[test] +fn stop_chat_aborts_sub_agents_and_clears_the_running_counter() { + let _guard = crate::agent_indicator_test_lock::LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + op_editor_core::agent_indicators::clear(); + + let mut app = DesktopApp::new(None); + let (_hold_tx, rx) = std::sync::mpsc::channel(); + app.sub_agents + .push(crate::sub_agent_session::SubAgentSession { + session: Some(crate::chat_session::ChatSession::from_channels(rx, None)), + identity: op_orchestrator::agent_identity::AgentIdentity { + color: "#5B8DEF".into(), + name: "Fern".into(), + }, + indicator: None, + root_seed_mobile: true, + }); + app.active_sub_agent = 0; + app.host.editor_state_mut().chat.agents_running = (1, 1); + app.host + .editor_state_mut() + .chat + .messages + .push(op_editor_core::ChatMessage::assistant_streaming()); + + assert!(app.host.editor_state_mut().chat.stop_streaming()); + assert!(app.drain_stop_chat()); + + assert!(app.sub_agents.is_empty()); + assert_eq!(app.active_sub_agent, 0); + assert_eq!(app.host.editor_state().chat.agents_running, (0, 0)); + assert!( + app.host + .editor_state() + .chat + .messages + .iter() + .all(|message| !message.streaming), + "Stop must leave no hidden streaming bubble behind" + ); + op_editor_core::agent_indicators::clear(); +} + +#[test] +fn new_chat_aborts_the_old_running_tab_without_dirtying_the_fresh_tab() { + let _guard = crate::agent_indicator_test_lock::LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + op_editor_core::agent_indicators::clear(); + + let mut app = DesktopApp::new(None); + let (_hold_tx, rx) = std::sync::mpsc::channel(); + app.sub_agents + .push(crate::sub_agent_session::SubAgentSession { + session: Some(crate::chat_session::ChatSession::from_channels(rx, None)), + identity: op_orchestrator::agent_identity::AgentIdentity { + color: "#5B8DEF".into(), + name: "Fern".into(), + }, + indicator: None, + root_seed_mobile: true, + }); + app.active_sub_agent = 0; + app.chat_running_tab = Some(0); + { + let old_tab = app.host.editor_state_mut().chat.tab_mut(0).unwrap(); + old_tab.agents_running = (1, 1); + old_tab + .messages + .push(op_editor_core::ChatMessage::assistant_streaming()); + } + + let fresh = app.host.editor_state_mut().chat.new_tab(); + app.host.editor_state_mut().chat.pending_new_chat = true; + assert_eq!(fresh, 1); + assert!(app.drain_new_chat()); + + assert!(app.sub_agents.is_empty()); + assert_eq!(app.active_sub_agent, 0); + assert_eq!(app.chat_running_tab, None); + let tabs = app.host.editor_state().chat.tabs(); + assert_eq!(tabs[0].agents_running, (0, 0)); + assert!(tabs[0].messages.iter().all(|message| !message.streaming)); + assert_eq!(tabs[1].agents_running, (0, 0)); + assert!(tabs[1].messages.is_empty()); + op_editor_core::agent_indicators::clear(); +} diff --git a/crates/op-host-desktop/src/persistence.rs b/crates/op-host-desktop/src/persistence.rs index b2a72828c..56be65d67 100644 --- a/crates/op-host-desktop/src/persistence.rs +++ b/crates/op-host-desktop/src/persistence.rs @@ -689,4 +689,53 @@ mod tests { let _ = std::fs::remove_file(&path); let _ = std::fs::remove_file(sidecar_path(&path)); } + + #[test] + fn opening_document_fits_and_centers_fit_content_root() { + let mut host = WidgetHostNative::new(); + let doc = jian_ops_schema::load_str( + r#"{ + "version":"0.8.0", + "children":[{ + "type":"frame","id":"root","name":"Explore", + "x":12,"y":24,"width":390,"height":"fit_content", + "layout":"vertical", + "children":[ + {"type":"frame","id":"header","width":"fill_container","height":62}, + {"type":"frame","id":"content","width":"fill_container","height":616}, + {"type":"frame","id":"tabs","width":"fill_container","height":72} + ] + }] + }"#, + ) + .expect("fixture JSON parses") + .value; + let state_to_open = EditorState::from_document(doc); + let path = temp_op_path("open-centers-fit-content-root"); + save_to_path(&state_to_open, &path).expect("save succeeds"); + let mut current_path = None; + + assert!(open_path(&mut host, path.clone(), &mut current_path, None)); + + let (min_x, min_y, max_x, max_y) = + active_page_bbox(host.editor_state()).expect("resolved content has bounds"); + assert!((max_y - min_y - 750.0).abs() < 0.01); + let content_center_x = ((min_x + max_x) / 2.0) as f32; + let content_center_y = ((min_y + max_y) / 2.0) as f32; + let (canvas_w, canvas_h) = op_host_services::design_session::design_canvas_size( + host.editor_state(), + super::super::INITIAL_VIEWPORT_W, + super::super::INITIAL_VIEWPORT_H, + ); + let screen_center_x = host.editor_state().viewport.pan_x + + content_center_x * host.editor_state().viewport.zoom; + let screen_center_y = host.editor_state().viewport.pan_y + + content_center_y * host.editor_state().viewport.zoom; + + assert!((screen_center_x - canvas_w / 2.0).abs() < 0.5); + assert!((screen_center_y - canvas_h / 2.0).abs() < 0.5); + + let _ = std::fs::remove_file(&path); + let _ = std::fs::remove_file(sidecar_path(&path)); + } } diff --git a/crates/op-host-desktop/src/sub_agent_session.rs b/crates/op-host-desktop/src/sub_agent_session.rs index 6c69c5ecf..ffad8ac19 100644 --- a/crates/op-host-desktop/src/sub_agent_session.rs +++ b/crates/op-host-desktop/src/sub_agent_session.rs @@ -178,6 +178,8 @@ pub(crate) fn build_sub_agent_prompt(spec: &SpawnSpec, context_brief: Option<&st } } + op_ai_skills::append_image_self_check_scope(&mut prompt); + prompt } @@ -261,6 +263,11 @@ pub(crate) fn abort_all(subs: &mut Vec, active: &mut usize) { agent_indicators::end_if_epoch(ind.epoch); } } + // A spawn request can be stashed by the parent one frame before the host + // launches it. Stop/New Chat must cancel that queued batch too, otherwise + // it starts after the user has already ended the turn. + PENDING_SPAWN.lock().unwrap().take(); + SUB_AGENT_ACTIVE.store(false, Ordering::SeqCst); subs.clear(); *active = 0; } diff --git a/crates/op-host-desktop/src/sub_agent_session_tests.rs b/crates/op-host-desktop/src/sub_agent_session_tests.rs index 6e1a43c76..7d761787b 100644 --- a/crates/op-host-desktop/src/sub_agent_session_tests.rs +++ b/crates/op-host-desktop/src/sub_agent_session_tests.rs @@ -117,6 +117,25 @@ fn build_prompt_tolerates_unknown_styleguide_and_guideline() { assert!(prompt.contains("batch_design")); } +#[test] +fn build_prompt_keeps_landing_image_self_check_presentation_only() { + let s = spec( + "Design the landing-page hero", + &["n1"], + REAL_STYLEGUIDE, + &["landing-page"], + ); + let prompt = build_sub_agent_prompt(&s, None); + assert!(prompt.contains("initial selection heuristic before inserting the image")); + assert!(prompt.contains("self-check is presentation-only")); + assert!(prompt.contains("automatic screenshot-driven self-check")); + assert!(prompt.contains("unless the user explicitly requests an image edit")); + assert!(!prompt.contains("If not, change it")); + assert!(prompt + .trim_end() + .ends_with(op_ai_skills::IMAGE_SELF_CHECK_SCOPE.trim_end())); +} + // --------------------------------------------------------------------------- // parse_spawn_args // ---------------------------------------------------------------------------