fix(desktop): align design session identity and lifecycle
This commit is contained in:
parent
6e2bbd379a
commit
a02b124aae
|
|
@ -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::<serde_json::Value>(&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::<serde_json::Value>(&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<usize> {
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
52
crates/op-host-desktop/src/chat_session_transcript_tests.rs
Normal file
52
crates/op-host-desktop/src/chat_session_transcript_tests.rs
Normal file
|
|
@ -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);
|
||||
}
|
||||
|
|
@ -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<DesignLoopIndicator> = 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();
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<SubAgentSession>, 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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in a new issue