From eea8901042f8d5bdcee29c78c25b64ac0c55940d Mon Sep 17 00:00:00 2001 From: Fini Date: Tue, 14 Jul 2026 00:27:55 +0800 Subject: [PATCH] fix(mcp): harden batch design and image semantics --- .../src/image_search_session.rs | 727 +++++++++++++----- .../src/image_search_session_tests.rs | 567 +++++++++++++- .../op-host-services/src/mcp_serve/schemas.rs | 2 +- crates/op-mcp/src/batch_design.rs | 144 ++-- crates/op-mcp/src/batch_design_result.rs | 29 +- crates/op-mcp/src/batch_design_tests.rs | 121 ++- crates/op-mcp/src/batch_direct_ops.rs | 50 +- crates/op-mcp/src/batch_layered_tests.rs | 37 + crates/op-mcp/src/batch_program.rs | 290 ++++++- .../op-mcp/src/batch_program_image_tests.rs | 379 +++++++++ crates/op-mcp/src/batch_program_tests.rs | 145 +++- crates/op-mcp/src/design_prompt.rs | 2 +- crates/op-mcp/src/design_prompt_tests.rs | 6 +- crates/op-mcp/src/read_tools.rs | 100 ++- crates/op-mcp/src/read_tools_extra.rs | 76 +- crates/op-mcp/src/script_runner.rs | 20 +- crates/op-mcp/src/script_runner_tests.rs | 44 +- 17 files changed, 2283 insertions(+), 456 deletions(-) create mode 100644 crates/op-mcp/src/batch_program_image_tests.rs diff --git a/crates/op-host-desktop/src/image_search_session.rs b/crates/op-host-desktop/src/image_search_session.rs index 57c2d6419..43e5d6fc2 100644 --- a/crates/op-host-desktop/src/image_search_session.rs +++ b/crates/op-host-desktop/src/image_search_session.rs @@ -1,6 +1,7 @@ //! Background image-search enrichment for generated image nodes. use std::collections::{HashMap, HashSet}; +use std::hash::{Hash, Hasher}; use std::sync::mpsc::{self, Receiver, TryRecvError}; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -145,12 +146,21 @@ pub(crate) struct ImageSearchTarget { /// AI-generation prompt bound to the node (`image_prompt`), if any. Used when /// an image-gen model is configured; falls back to `query`. pub prompt: Option, + /// Explicit `G()` acquisition mode, or Auto for legacy/heuristic slots. + pub mode: ImageRequestMode, /// Resolved numeric dimensions (for the gen provider's aspect mapping). pub width: Option, pub height: Option, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ImageRequestMode { + Auto, + Search, + Generate, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub(crate) enum ImageAspectRatio { Wide, Tall, @@ -191,9 +201,26 @@ impl OpenverseCredentials { struct ImageSearchJob { node_id: NodeId, + /// Exact node intent at enqueue time. Production jobs always set this; + /// hand-built unit jobs may omit it when testing unrelated bookkeeping. + intent: Option, rx: Receiver>, } +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +struct SearchIntentKey { + query: String, + aspect_ratio: Option, +} + +enum SearchMemoEntry { + Pending { + request_id: u64, + waiters: Vec>>, + }, + Ready(String), +} + #[derive(Default)] pub(crate) struct ImageSearchSession { in_flight: HashSet, @@ -224,26 +251,64 @@ pub(crate) struct ImageSearchSession { /// target is already known via `in_flight`/`completed`). #[cfg(test)] scan_count: u32, - /// Result URLs already used this session — similar queries ("playlist - /// cover daily mix" / "... chill vibes" / "... discover weekly") share - /// their Openverse top hit, which filled three different cards with the - /// SAME photo (measured: test0711-22). Selection skips these best-effort. + /// Canonical provider identities plus compact content digests already used + /// this session. Similar queries otherwise share their Openverse top hit + /// and fill several cards with the same artwork (measured: test0711-22). + /// Never stores full embedded data URLs. used_urls: Arc>>, - /// Query → the photo that query already resolved to, this session. + /// Full stock-search intent → pending waiters or the resolved photo. /// /// The dedup above must NOT fire when the SAME subject comes back: the /// model rebuilds a section mid-run (fresh node ids), the same query /// ("Bali Indonesia") searches again, and its own good photo is now in - /// `used_urls` — so the second search skipped it and took a junk result + /// the used-image set — so the second search skipped it and took a junk result /// instead (measured 2026-07-12: a real Bali temple photo turned into a /// plain blue sky halfway through a run). One subject, one photo: a repeat /// query resolves from this memo with no network call at all. - resolved: Arc>>, + search_memo: Arc>>, + /// Monotonic identity for a memo fetch. It is deliberately not reset: + /// an old network thread may finish after `reset()` and after the new + /// document has enqueued the same intent. Matching the request id avoids + /// that old completion consuming the new waiters (the classic ABA race). + next_search_request_id: u64, } -/// Memo key — the subject, not its spelling. -fn query_key(query: &str) -> String { - query.trim().to_ascii_lowercase() +/// Memo key for the authored stock-search intent. +/// +/// This deliberately does NOT use `simplify_search_query`: that function is a +/// lossy provider adapter (it drops words such as `album` / `cover` and caps +/// the request at four keywords). Those transformations are useful for a +/// photo corpus, but they must not make two distinct authored subjects share a +/// cached image or make the stale-result guard treat a changed intent as the +/// same intent. Case, punctuation, and repeated whitespace are canonicalized; +/// every authored word remains part of identity. Aspect remains part of intent +/// so a square cover never reuses a wide hero. +fn search_intent_key(query: &str, aspect_ratio: Option) -> SearchIntentKey { + SearchIntentKey { + query: canonical_search_intent_query(query), + aspect_ratio, + } +} + +fn canonical_search_intent_query(query: &str) -> String { + let mut canonical = String::with_capacity(query.len()); + let mut pending_separator = false; + for character in query.trim().to_lowercase().chars() { + if character.is_alphanumeric() { + if pending_separator && !canonical.is_empty() { + canonical.push(' '); + } + canonical.push(character); + pending_separator = false; + } else { + pending_separator = true; + } + } + if canonical.is_empty() { + query.trim().to_lowercase() + } else { + canonical + } } impl ImageSearchSession { @@ -268,8 +333,11 @@ impl ImageSearchSession { self.completed.clear(); self.jobs.clear(); self.invalidate_scan_gate(); - self.used_urls.lock().unwrap().clear(); - self.resolved.lock().unwrap().clear(); + // Replace the generations, do not merely clear them. Detached network + // threads still own the old Arcs and may finish after reset; writing to + // those abandoned maps must not contaminate the replacement document. + self.used_urls = Arc::new(Mutex::new(HashSet::new())); + self.search_memo = Arc::new(Mutex::new(HashMap::new())); } pub(crate) fn is_pending(&self) -> bool { @@ -317,23 +385,30 @@ impl ImageSearchSession { if targets.is_empty() { return false; } - // Strategy: a configured image-GEN model wins (generate from the bound - // `image_prompt`); otherwise fall back to stock SEARCH (Openverse). The - // prompt/query stay on the node either way, so the UI can re-gen/re-search - // and a later config change re-resolves on the next enqueue. + // An explicit G(...,"search"|"generate",...) mode wins. Legacy and + // heuristic slots remain Auto: configured generation first, otherwise + // stock search. A generate request without a configured provider fails + // visibly; it is never silently changed into a stock-photo request. let gen_profile = crate::image_panel_host::active_image_gen_profile(state).cloned(); let credentials = OpenverseCredentials::from_state(state); for target in targets { let id = target.node_id.as_str().to_string(); self.in_flight.insert(id); - let job = match &gen_profile { - Some(profile) => spawn_gen_job(target, profile.clone()), - None => spawn_job( - target, - credentials.clone(), - Arc::clone(&self.used_urls), - Arc::clone(&self.resolved), - ), + let job = match (target.mode, &gen_profile) { + (ImageRequestMode::Generate, Some(profile)) + | (ImageRequestMode::Auto, Some(profile)) => spawn_gen_job(target, profile.clone()), + (ImageRequestMode::Generate, None) => spawn_unavailable_gen_job(target), + (ImageRequestMode::Search, _) | (ImageRequestMode::Auto, None) => { + let request_id = self.next_search_request_id; + self.next_search_request_id = self.next_search_request_id.wrapping_add(1); + spawn_job( + target, + credentials.clone(), + Arc::clone(&self.used_urls), + Arc::clone(&self.search_memo), + request_id, + ) + } }; self.jobs.push(job); } @@ -357,6 +432,15 @@ impl ImageSearchSession { // revision, but the gate invalidation still covers the // failure path.) self.last_scanned = None; + if job.intent.as_ref().is_some_and(|expected| { + current_intent_fingerprint(state, &job.node_id).as_ref() != Some(expected) + }) { + tracing::info!( + node_id = %job.node_id, + "discarding stale image result because the node's image intent changed" + ); + continue; + } // Ours: a failed search lands the theme-adaptive dashed // placeholder (sentinel src) instead of a bare grey box. let url = url.unwrap_or_else(|| SEARCH_FAILED_PLACEHOLDER_SRC.to_string()); @@ -391,18 +475,46 @@ fn spawn_job( target: ImageSearchTarget, credentials: Option, used_urls: Arc>>, - resolved: Arc>>, + search_memo: Arc>>, + request_id: u64, ) -> ImageSearchJob { let (tx, rx) = mpsc::channel(); let node_id = target.node_id.clone(); let aspect_ratio = target.aspect_ratio; - let key = query_key(&target.query); - // One subject, one photo: a query this session already answered resolves - // from the memo — no network, and (crucially) no dedup-forced downgrade - // when a rebuilt section asks for the same picture again. - if let Some(url) = resolved.lock().unwrap().get(&key).cloned() { - let _ = tx.send(Some(url)); - return ImageSearchJob { node_id, rx }; + let key = search_intent_key(&target.query, aspect_ratio); + let intent = Some(intent_fingerprint(&target, None)); + // One full search intent, one in-flight request and one session result. + // Rebuilt nodes subscribe to the same pending request instead of racing + // duplicate searches; completed intents return from the memo. + { + let mut memo = search_memo.lock().unwrap(); + match memo.get_mut(&key) { + Some(SearchMemoEntry::Ready(url)) => { + let _ = tx.send(Some(url.clone())); + return ImageSearchJob { + node_id, + intent, + rx, + }; + } + Some(SearchMemoEntry::Pending { waiters, .. }) => { + waiters.push(tx); + return ImageSearchJob { + node_id, + intent, + rx, + }; + } + None => { + memo.insert( + key.clone(), + SearchMemoEntry::Pending { + request_id, + waiters: vec![tx], + }, + ); + } + } } std::thread::spawn(move || { let url = fetch_first_image_url_blocking( @@ -411,12 +523,48 @@ fn spawn_job( credentials.as_ref(), &used_urls, ); - if let Some(found) = url.as_ref() { - resolved.lock().unwrap().insert(key, found.clone()); - } - let _ = tx.send(url); + publish_search_result(&search_memo, key, request_id, url); }); - ImageSearchJob { node_id, rx } + ImageSearchJob { + node_id, + intent, + rx, + } +} + +/// Publish only into the exact Pending entry that launched this request. A +/// reset may remove it and a new document may insert the same key before the +/// old thread returns; the request id keeps that old result isolated. +fn publish_search_result( + search_memo: &Arc>>, + key: SearchIntentKey, + request_id: u64, + url: Option, +) -> bool { + let waiters = { + let mut memo = search_memo.lock().unwrap(); + let matches_request = matches!( + memo.get(&key), + Some(SearchMemoEntry::Pending { + request_id: pending_id, + .. + }) if *pending_id == request_id + ); + if !matches_request { + return false; + } + let Some(SearchMemoEntry::Pending { waiters, .. }) = memo.remove(&key) else { + unreachable!("request identity was checked under the same lock") + }; + if let Some(found) = url.as_ref() { + memo.insert(key, SearchMemoEntry::Ready(found.clone())); + } + waiters + }; + for waiter in waiters { + let _ = waiter.send(url.clone()); + } + true } /// Enrich via the configured image-GEN model instead of stock search. Prefers the @@ -426,6 +574,7 @@ fn spawn_job( fn spawn_gen_job(target: ImageSearchTarget, profile: ImageGenProfile) -> ImageSearchJob { let (tx, rx) = mpsc::channel(); let node_id = target.node_id.clone(); + let intent = Some(intent_fingerprint(&target, Some(&profile))); std::thread::spawn(move || { let prompt = target .prompt @@ -441,44 +590,93 @@ fn spawn_gen_job(target: ImageSearchTarget, profile: ImageGenProfile) -> ImageSe .ok(); let _ = tx.send(url); }); - ImageSearchJob { node_id, rx } + ImageSearchJob { + node_id, + intent, + rx, + } +} + +fn spawn_unavailable_gen_job(target: ImageSearchTarget) -> ImageSearchJob { + let (tx, rx) = mpsc::channel(); + let node_id = target.node_id.clone(); + let intent = Some(intent_fingerprint(&target, None)); + let _ = tx.send(None); + ImageSearchJob { + node_id, + intent, + rx, + } +} + +fn intent_fingerprint(target: &ImageSearchTarget, profile: Option<&ImageGenProfile>) -> String { + let generate = target.mode == ImageRequestMode::Generate + || (target.mode == ImageRequestMode::Auto && profile.is_some()); + if generate { + let (profile_id, model) = profile + .map(|profile| (profile.id.as_str(), profile.model.as_str())) + .unwrap_or(("unconfigured", "unconfigured")); + format!( + "generate|{profile_id}|{model}|{}|{:?}|{:?}", + target + .prompt + .as_deref() + .filter(|prompt| !prompt.trim().is_empty()) + .unwrap_or(target.query.as_str()) + .trim(), + target.width.map(f64::to_bits), + target.height.map(f64::to_bits) + ) + } else { + let key = search_intent_key(&target.query, target.aspect_ratio); + format!("search|{}|{:?}", key.query, key.aspect_ratio) + } +} + +fn current_intent_fingerprint(state: &EditorState, node_id: &NodeId) -> Option { + let target = collect_targets(state, &HashSet::new()) + .into_iter() + .find(|target| &target.node_id == node_id)?; + let profile = crate::image_panel_host::active_image_gen_profile(state); + Some(intent_fingerprint(&target, profile)) } pub(crate) fn collect_targets( state: &EditorState, known_node_ids: &HashSet, ) -> Vec { - let rects = resolved_sizes(state); + let resolved_sizes = resolved_node_sizes(state); let mut targets = Vec::new(); collect_from_children( state.active_children(), known_node_ids, - &rects, + &resolved_sizes, &mut targets, &[], - &[], ); targets } -/// Every node's RESOLVED size, from the real layout pass. -/// -/// Name-and-authored-size heuristics are blind to the shape weak models -/// actually ship: DeepSeek built every card's photo area as an UNNAMED -/// rectangle sized `fill_container` x `fill_container`, so it carried neither -/// a keyword nor a number and the whole page came out as grey boxes (measured -/// test0711-1-ds, 2026-07-12). What a slot IS is a question about geometry — -/// so ask the layout. -fn resolved_sizes(state: &EditorState) -> HashMap { +/// Resolved node dimensions from the real layout pass. This map is used only +/// after a node has independently qualified as media; geometry never turns an +/// anonymous surface into an image target. It lets a G()-created fill/fill +/// child inherit the actual slot aspect for search, generation, and stale-job +/// detection instead of losing that intent as `None`. +fn resolved_node_sizes(state: &EditorState) -> HashMap { let scene = op_pen_loader::editor_state_to_layout_scene(state); let mut out = HashMap::new(); fn walk( nodes: &[op_editor_ui::layout_scene::SceneNode], - out: &mut HashMap, + out: &mut HashMap, ) { for node in nodes { let bounds = node.aggregate_bounds(); - out.insert(node.id.clone(), (bounds.size.x, bounds.size.y)); + if bounds.size.x > 0.0 && bounds.size.y > 0.0 { + out.insert( + node.id.clone(), + (f64::from(bounds.size.x), f64::from(bounds.size.y)), + ); + } walk(&node.children, out); } } @@ -488,78 +686,37 @@ fn resolved_sizes(state: &EditorState) -> HashMap { out } -/// The smallest box that reads as a picture rather than a swatch or a rule. -const RESOLVED_SLOT_MIN_W: f32 = 80.0; -const RESOLVED_SLOT_MIN_H: f32 = 60.0; - -/// An EMPTY, painted box that resolved to picture size, carries no name of its -/// own (or only a generic one), and sits in a card that has words in it — the -/// card's words say what the picture is. Geometry decides; the name is only -/// allowed to VETO (a box called "Divider" is not a photo). -fn is_resolved_media_slot( - node: &PenNode, - rects: &HashMap, - sibling_text: &[String], -) -> bool { - let (base, container) = match node { - PenNode::Frame(f) => (&f.base, &f.container), - PenNode::Rectangle(r) => (&r.base, &r.container), - _ => return false, - }; - if sibling_text.is_empty() { - return false; - } - if node.children().is_some_and(|kids| !kids.is_empty()) { - return false; - } - if !matches!(container.fill.as_deref(), Some([PenFill::Solid(_)])) { - return false; - } - if let Some(name) = base.name.as_deref().map(str::trim) { - if !name.is_empty() && !is_generic_placeholder_name(name) && !has_image_area_keyword(name) { - return false; - } - } - rects - .get(base.id.as_str()) - .is_some_and(|(w, h)| *w >= RESOLVED_SLOT_MIN_W && *h >= RESOLVED_SLOT_MIN_H) -} - fn collect_from_children( children: &[PenNode], known_node_ids: &HashSet, - rects: &HashMap, + resolved_sizes: &HashMap, targets: &mut Vec, parent_names: &[String], - inherited_text: &[String], ) { - // Sibling text of a bare anonymous slot IS its subject ("Blinding - // Lights" next to a nameless 120px square = that track's cover). + // Direct sibling text of a bare anonymous slot may name its subject + // ("Blinding Lights" next to a nameless 120px square = that track's + // cover). Do not search sibling container subtrees or inherit text across + // levels: at a rail/list boundary those words belong to cousin cards, not + // to this slot. for (index, node) in children.iter().enumerate() { - // The words that name a picture are rarely the slot's literal siblings: - // a card is [photo band, info frame] and the title lives INSIDE the - // info frame (measured test0711-1-ds — every photo area came out - // contextless and the page shipped as grey boxes). Take the words from - // the OTHER siblings' subtrees; a slot alone in its band inherits its - // card's words. Cousin cards are never consulted — that would name the - // Bali card's photo "Santorini". - let mut context: Vec = Vec::new(); - for (other_index, other) in children.iter().enumerate() { - if other_index == index { - continue; - } - collect_text_from_subtree(other, 3, &mut context); - if context.len() >= 2 { - break; - } - } - context.truncate(2); - if context.is_empty() { - context = inherited_text.to_vec(); - } + let context: Vec = children + .iter() + .enumerate() + .filter(|(other_index, _)| *other_index != index) + .filter_map(|(_, other)| match other { + PenNode::Text(text) => match &text.content { + TextContent::Plain(value) if !value.trim().is_empty() => { + Some(value.trim().to_string()) + } + _ => None, + }, + _ => None, + }) + .take(2) + .collect(); if let Some(target) = - image_search_target_for(node, known_node_ids, rects, parent_names, &context) + image_search_target_for(node, known_node_ids, resolved_sizes, parent_names, &context) { targets.push(target); } @@ -577,40 +734,18 @@ fn collect_from_children( collect_from_children( grand, known_node_ids, - rects, + resolved_sizes, targets, &child_parent_names, - &context, ); } } } -fn collect_text_from_subtree(node: &PenNode, depth: usize, out: &mut Vec) { - if out.len() >= 2 { - return; - } - if let PenNode::Text(t) = node { - if let TextContent::Plain(text) = &t.content { - let trimmed = text.trim(); - if !trimmed.is_empty() { - out.push(trimmed.to_string()); - } - } - return; - } - if depth == 0 { - return; - } - for child in node.children().into_iter().flatten() { - collect_text_from_subtree(child, depth - 1, out); - } -} - fn image_search_target_for( node: &PenNode, known_node_ids: &HashSet, - rects: &HashMap, + resolved_sizes: &HashMap, parent_names: &[String], sibling_text: &[String], ) -> Option { @@ -624,7 +759,8 @@ fn image_search_target_for( // way with no names and no G() bindings (measured test0711-2-ds); the // sibling text is the only, and a good, subject source. let bare_slot_with_context = !sibling_text.is_empty() - && (is_bare_anonymous_slot(node) || is_resolved_media_slot(node, rects, sibling_text)); + && !has_non_media_context(parent_names) + && is_bare_anonymous_slot(node); let needs_image = match node { PenNode::Image(image) => is_placeholder_src(&image.src), PenNode::Frame(_) => is_frame_placeholder_still_unfilled(node) || bare_slot_with_context, @@ -677,13 +813,62 @@ fn image_search_target_for( PenNode::Image(image) => image.image_prompt.clone(), _ => None, }; + let mode = match node { + // Legacy / script-generated Image nodes intentionally carry both + // fields: generation uses the richer prompt when a profile exists, + // otherwise stock search falls back to the query. `G("search")` and + // `G("generate")` remain unambiguous because they emit only one field. + PenNode::Image(image) + if image + .image_prompt + .as_deref() + .is_some_and(|prompt| !prompt.trim().is_empty()) + && image + .image_search_query + .as_deref() + .is_some_and(|query| !query.trim().is_empty()) => + { + ImageRequestMode::Auto + } + PenNode::Image(image) + if image + .image_prompt + .as_deref() + .is_some_and(|prompt| !prompt.trim().is_empty()) => + { + ImageRequestMode::Generate + } + PenNode::Image(image) + if image + .image_search_query + .as_deref() + .is_some_and(|query| !query.trim().is_empty()) => + { + ImageRequestMode::Search + } + PenNode::Frame(frame) + if frame + .image_search_query + .as_deref() + .is_some_and(|query| !query.trim().is_empty()) => + { + ImageRequestMode::Search + } + _ => ImageRequestMode::Auto, + }; + let (width, height) = resolved_sizes + .get(id) + .copied() + .map(|(width, height)| (Some(width), Some(height))) + .unwrap_or_else(|| (node.width_px(), node.height_px())); Some(ImageSearchTarget { node_id: NodeId::new(id), query, - aspect_ratio: infer_aspect_ratio(node), + aspect_ratio: infer_aspect_ratio(width, height), prompt, - width: node.width_px(), - height: node.height_px(), + mode, + width, + height, }) } @@ -794,6 +979,9 @@ fn is_unnamed_media_slot_in_context(node: &PenNode, parent_names: &[String]) -> let PenNode::Rectangle(rect) = node else { return false; }; + if has_non_media_context(parent_names) { + return false; + } if rect .base .name @@ -826,6 +1014,40 @@ fn is_unnamed_media_slot_in_context(node: &PenNode, parent_names: &[String]) -> }) } +/// Explicit structural/control vocabulary vetoes the anonymous-slot fallback. +/// These surfaces often have the same small rounded solid geometry as cover +/// art, but filling a KPI, swatch, badge, or button with a photo is always a +/// worse failure than leaving an ambiguous box untouched. Explicit image +/// nodes, placeholder roles, and media-named slots do not depend on this +/// fallback and remain eligible. +fn has_non_media_context(parent_names: &[String]) -> bool { + const NON_MEDIA_WORDS: [&str; 18] = [ + "kpi", + "metric", + "stat", + "stats", + "analytics", + "chart", + "graph", + "swatch", + "palette", + "button", + "control", + "badge", + "indicator", + "progress", + "separator", + "divider", + "toggle", + "status", + ]; + parent_names.iter().any(|name| { + name.to_ascii_lowercase() + .split(|character: char| !character.is_ascii_alphanumeric()) + .any(|token| NON_MEDIA_WORDS.contains(&token)) + }) +} + fn is_image_area_rectangle_by_heuristic(node: &PenNode) -> bool { let PenNode::Rectangle(rect) = node else { return false; @@ -872,14 +1094,8 @@ fn image_area_dimension_ok(size: &Option, min_px: f64) -> (bool, } } -fn infer_aspect_ratio(node: &PenNode) -> Option { - let (width, height) = match node { - PenNode::Image(image) => (&image.width, &image.height), - PenNode::Frame(frame) => (&frame.container.width, &frame.container.height), - PenNode::Rectangle(rect) => (&rect.container.width, &rect.container.height), - _ => return None, - }; - let (Some(width), Some(height)) = (dimension_number(width), dimension_number(height)) else { +fn infer_aspect_ratio(width: Option, height: Option) -> Option { + let (Some(width), Some(height)) = (width, height) else { return None; }; if width <= 0.0 || height <= 0.0 { @@ -903,6 +1119,21 @@ fn dimension_number(size: &Option) -> Option { } fn has_image_area_keyword(name: &str) -> bool { + let compact: String = name + .chars() + .filter(|character| character.is_ascii_alphanumeric()) + .flat_map(char::to_lowercase) + .collect(); + if compact.ends_with("img") + || compact.ends_with("image") + || compact.ends_with("photo") + || compact.ends_with("cover") + || compact.ends_with("thumbnail") + || compact.ends_with("artwork") + || compact.ends_with("media") + { + return true; + } name.split(|c: char| !c.is_ascii_alphanumeric()) .map(str::to_ascii_lowercase) .any(|word| { @@ -1162,16 +1393,12 @@ fn fetch_first_image_url_blocking( .enable_all() .build() .ok()?; - let picked = runtime.block_on(fetch_first_image_url( + runtime.block_on(fetch_first_image_url( query, aspect_ratio, credentials, used_urls, - )); - if let Some(url) = picked.as_ref() { - used_urls.lock().unwrap().insert(url.clone()); - } - picked + )) } async fn fetch_first_image_url( @@ -1198,11 +1425,11 @@ async fn fetch_first_image_url( { return Some(url); } - if let Some(url) = fetch_wikimedia(&client, &truncated).await { + if let Some(url) = fetch_wikimedia(&client, &truncated, used_urls).await { return Some(url); } } - fetch_wikimedia(&client, &query).await + fetch_wikimedia(&client, &query, used_urls).await } pub(crate) fn simplify_search_query(prompt: &str) -> String { @@ -1261,8 +1488,7 @@ async fn fetch_openverse( } let json: serde_json::Value = resp.json().await.ok()?; let results = json.get("results")?.as_array()?; - let used = used_urls.lock().unwrap().clone(); - let result = select_openverse_result(results, query, &used)?; + let (result, identity) = claim_openverse_result(results, query, used_urls)?; let mut candidates = Vec::new(); push_candidate_url( &mut candidates, @@ -1272,7 +1498,8 @@ async fn fetch_openverse( &mut candidates, result.get("url").and_then(serde_json::Value::as_str), ); - first_renderable_image_src(client, candidates).await + let outcome = first_unused_renderable_image_src(client, candidates, used_urls).await; + settle_provider_identity(used_urls, &identity, outcome) } /// Titles that mark a result as noise no matter how well it ranks — the @@ -1309,12 +1536,15 @@ pub(crate) fn select_openverse_result<'results>( used_urls: &HashSet, ) -> Option<&'results serde_json::Value> { let is_used = |result: &serde_json::Value| { - ["url", "thumbnail"].iter().any(|key| { - result - .get(*key) - .and_then(serde_json::Value::as_str) - .is_some_and(|url| used_urls.contains(url)) - }) + openverse_result_identity(result).is_some_and(|identity| used_urls.contains(&identity)) + // Backward-compatible URL keys keep the pure selector useful to + // callers/tests, but production claims canonical result identity. + || ["url", "thumbnail"].iter().any(|key| { + result + .get(*key) + .and_then(serde_json::Value::as_str) + .is_some_and(|url| used_urls.contains(url)) + }) }; let non_junk: Vec<&serde_json::Value> = results .iter() @@ -1342,6 +1572,36 @@ pub(crate) fn select_openverse_result<'results>( .or_else(|| non_junk.first().copied()) } +fn openverse_result_identity(result: &serde_json::Value) -> Option { + if let Some(id) = result.get("id") { + if let Some(id) = id.as_str().filter(|id| !id.trim().is_empty()) { + return Some(format!("openverse:{id}")); + } + if id.is_number() { + return Some(format!("openverse:{id}")); + } + } + ["url", "thumbnail"].iter().find_map(|field| { + result + .get(*field) + .and_then(serde_json::Value::as_str) + .map(str::trim) + .filter(|url| !url.is_empty()) + .map(|url| format!("openverse-url:{url}")) + }) +} + +fn claim_openverse_result<'results>( + results: &'results [serde_json::Value], + query: &str, + used_images: &Mutex>, +) -> Option<(&'results serde_json::Value, String)> { + let mut used = used_images.lock().unwrap(); + let result = select_openverse_result(results, query, &used)?; + let identity = openverse_result_identity(result)?; + used.insert(identity.clone()).then_some((result, identity)) +} + fn openverse_search_url( query: &str, aspect_ratio: Option, @@ -1384,7 +1644,11 @@ pub(crate) async fn fetch_openverse_token( .map(str::to_string) } -async fn fetch_wikimedia(client: &reqwest::Client, query: &str) -> Option { +async fn fetch_wikimedia( + client: &reqwest::Client, + query: &str, + used_urls: &Mutex>, +) -> Option { let url = reqwest::Url::parse_with_params( "https://commons.wikimedia.org/w/api.php", &[ @@ -1408,28 +1672,55 @@ async fn fetch_wikimedia(client: &reqwest::Client, query: &str) -> Option Option { + if let Some(page_id) = page.get("pageid") { + if page_id.is_number() || page_id.is_string() { + return Some(format!("wikimedia:{page_id}")); + } + } + page.get("title") + .and_then(serde_json::Value::as_str) + .map(str::trim) + .filter(|title| !title.is_empty()) + .map(|title| format!("wikimedia-title:{title}")) +} + +fn wikimedia_image_candidates(page: &serde_json::Value) -> Vec { + let mut candidates = Vec::new(); + if let Some(info) = page + .get("imageinfo") + .and_then(serde_json::Value::as_array) + .and_then(|items| items.first()) + { + push_candidate_url( + &mut candidates, + info.get("thumburl").and_then(serde_json::Value::as_str), + ); + push_candidate_url( + &mut candidates, + info.get("url").and_then(serde_json::Value::as_str), + ); + } + candidates +} + fn push_candidate_url(candidates: &mut Vec, url: Option<&str>) { let Some(url) = url.map(str::trim).filter(|url| !url.is_empty()) else { return; @@ -1439,16 +1730,66 @@ fn push_candidate_url(candidates: &mut Vec, url: Option<&str>) { } } -async fn first_renderable_image_src( - client: &reqwest::Client, - candidates: Vec, +#[derive(Debug, PartialEq, Eq)] +enum ImageCandidateClaim { + /// A renderable image won the session-wide content claim. + Claimed(String), + /// A renderable download matched content another provider result already + /// owns. The provider identity stays used because its artwork is known to + /// be a duplicate even though this request cannot return it. + Duplicate, + /// No candidate URL produced a renderable image. The provider reservation + /// must be released so a later request can retry a transient failure. + Unavailable, +} + +fn settle_provider_identity( + used_urls: &Mutex>, + identity: &str, + outcome: ImageCandidateClaim, ) -> Option { - for candidate in candidates { - if let Some(src) = fetch_image_data_url(client, &candidate).await { - return Some(src); + match outcome { + ImageCandidateClaim::Claimed(src) => Some(src), + ImageCandidateClaim::Duplicate => None, + ImageCandidateClaim::Unavailable => { + used_urls.lock().unwrap().remove(identity); + None } } - None +} + +async fn first_unused_renderable_image_src( + client: &reqwest::Client, + candidates: Vec, + used_urls: &Mutex>, +) -> ImageCandidateClaim { + let mut found_duplicate = false; + for candidate in candidates { + if let Some(src) = fetch_image_data_url(client, &candidate).await { + // Claim the embedded result under one lock. Different queries run + // concurrently and may resolve to the same underlying image; a + // snapshot-then-insert check lets both win. The atomic claim lets + // the loser continue to another candidate/fallback instead. + if claim_unused_image_src(used_urls, &src) { + return ImageCandidateClaim::Claimed(src); + } + found_duplicate = true; + } + } + if found_duplicate { + ImageCandidateClaim::Duplicate + } else { + ImageCandidateClaim::Unavailable + } +} + +fn claim_unused_image_src(used_urls: &Mutex>, src: &str) -> bool { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + src.hash(&mut hasher); + used_urls + .lock() + .unwrap() + .insert(format!("content:{:016x}", hasher.finish())) } pub(crate) async fn fetch_image_data_url(client: &reqwest::Client, url: &str) -> Option { diff --git a/crates/op-host-desktop/src/image_search_session_tests.rs b/crates/op-host-desktop/src/image_search_session_tests.rs index bf04dbfd7..79a0d4538 100644 --- a/crates/op-host-desktop/src/image_search_session_tests.rs +++ b/crates/op-host-desktop/src/image_search_session_tests.rs @@ -434,6 +434,7 @@ fn poll_into_applies_finished_job_to_placeholder_frame() { completed: HashSet::new(), jobs: vec![ImageSearchJob { node_id: NodeId::new("photo"), + intent: None, rx, }], ..Default::default() @@ -472,6 +473,7 @@ fn successful_apply_does_not_suppress_later_unfilled_retry() { completed: HashSet::new(), jobs: vec![ImageSearchJob { node_id: NodeId::new("photo"), + intent: None, rx, }], ..Default::default() @@ -529,6 +531,7 @@ fn poll_into_completion_invalidates_scan_gate() { completed: HashSet::new(), jobs: vec![ImageSearchJob { node_id: NodeId::new("img1"), + intent: None, rx, }], ..Default::default() @@ -553,6 +556,96 @@ fn poll_into_completion_invalidates_scan_gate() { ); } +#[test] +fn poll_discards_a_result_when_the_nodes_image_intent_changed() { + let mut state = EditorState::default(); + state.active_children_mut().clear(); + state + .active_children_mut() + .push(image_node("img1", "", Some("burger fries"))); + let original = collect_targets(&state, &HashSet::new()) + .into_iter() + .next() + .expect("original target"); + let expected = intent_fingerprint(&original, None); + + let PenNode::Image(image) = &mut state.active_children_mut()[0] else { + panic!("image") + }; + image.image_search_query = Some("latte cup".into()); + state.mark_document_changed(); + + let (tx, rx) = std::sync::mpsc::channel(); + tx.send(Some("https://stale.example.com/burger.jpg".to_string())) + .unwrap(); + let mut session = ImageSearchSession { + in_flight: HashSet::from(["img1".to_string()]), + jobs: vec![ImageSearchJob { + node_id: NodeId::new("img1"), + intent: Some(expected), + rx, + }], + ..Default::default() + }; + + assert!(!session.poll_into(&mut state)); + let PenNode::Image(image) = &state.active_children()[0] else { + panic!("image") + }; + assert!(image.src.is_empty(), "stale URL must not land"); + assert!(!session.completed.contains("img1")); +} + +#[test] +fn poll_discards_a_result_when_only_provider_truncated_words_changed() { + let before = "santorini greece white buildings blue dome"; + let after = "santorini greece white buildings sunset beach"; + assert_eq!( + simplify_search_query(before), + simplify_search_query(after), + "the provider intentionally sees the same four-keyword request" + ); + + let mut state = EditorState::default(); + state.active_children_mut().clear(); + state + .active_children_mut() + .push(image_node("img1", "", Some(before))); + let original = collect_targets(&state, &HashSet::new()) + .into_iter() + .next() + .expect("original target"); + let expected = intent_fingerprint(&original, None); + + let PenNode::Image(image) = &mut state.active_children_mut()[0] else { + panic!("image") + }; + image.image_search_query = Some(after.into()); + state.mark_document_changed(); + + let (tx, rx) = std::sync::mpsc::channel(); + tx.send(Some("https://stale.example.com/blue-dome.jpg".to_string())) + .unwrap(); + let mut session = ImageSearchSession { + in_flight: HashSet::from(["img1".to_string()]), + jobs: vec![ImageSearchJob { + node_id: NodeId::new("img1"), + intent: Some(expected), + rx, + }], + ..Default::default() + }; + + assert!(!session.poll_into(&mut state)); + let PenNode::Image(image) = &state.active_children()[0] else { + panic!("image") + }; + assert!( + image.src.is_empty(), + "the provider-level collision must not weaken authored intent identity" + ); +} + #[test] fn reset_invalidates_scan_gate() { let mut state = EditorState::default(); @@ -737,6 +830,7 @@ fn document_replacement_reset_drops_stale_pending_job_so_it_cannot_apply_to_new_ completed: HashSet::new(), jobs: vec![ImageSearchJob { node_id: NodeId::new("photo"), + intent: None, rx, }], ..Default::default() @@ -903,6 +997,7 @@ fn failed_search_writes_the_adaptive_placeholder_sentinel() { in_flight: HashSet::from(["img1".to_string()]), jobs: vec![ImageSearchJob { node_id: NodeId::new("img1"), + intent: None, rx, }], ..Default::default() @@ -1016,6 +1111,81 @@ fn anonymous_cover_slot_uses_sibling_text_as_query() { ); } +#[test] +fn anonymous_slot_does_not_borrow_text_from_a_cousin_card() { + let mut slot = frame_node("slot", "", None, Some(vec![solid_fill()]), vec![]); + if let PenNode::Frame(frame) = &mut slot { + frame.base.name = None; + frame.container.width = Some(SizingBehavior::Number(120.0)); + frame.container.height = Some(SizingBehavior::Number(120.0)); + frame.container.clip_content = Some(true); + } + let mut empty_card = frame_node("empty-card", "", None, None, vec![slot]); + if let PenNode::Frame(frame) = &mut empty_card { + frame.base.name = None; + } + let other_card = frame_node( + "other-card", + "Santorini Card", + None, + None, + vec![frame_node( + "other-info", + "Info", + None, + None, + vec![text_label("other-title", None, "Santorini, Greece")], + )], + ); + let rail = frame_node( + "rail", + "Destination Rail", + None, + None, + vec![empty_card, other_card], + ); + let mut state = EditorState::default(); + state.active_children_mut().clear(); + state.active_children_mut().push(rail); + + let targets = collect_targets(&state, &HashSet::new()); + assert!( + targets + .iter() + .all(|target| target.node_id.as_str() != "slot"), + "an unlabelled card must stay empty, not borrow its cousin's title: {targets:?}" + ); +} + +#[test] +fn rounded_kpi_tile_is_not_an_anonymous_image_slot() { + let mut tile = frame_node("tile", "", None, Some(vec![solid_fill()]), vec![]); + if let PenNode::Frame(frame) = &mut tile { + frame.base.name = None; + frame.container.width = Some(SizingBehavior::Number(64.0)); + frame.container.height = Some(SizingBehavior::Number(64.0)); + frame.container.corner_radius = Some(jian_ops_schema::node::CornerRadius::Uniform(16.0)); + } + let card = frame_node( + "kpi", + "Revenue KPI Card", + None, + None, + vec![tile, text_label("label", None, "Monthly revenue")], + ); + let mut state = EditorState::default(); + state.active_children_mut().clear(); + state.active_children_mut().push(card); + + let targets = collect_targets(&state, &HashSet::new()); + assert!( + targets + .iter() + .all(|target| target.node_id.as_str() != "tile"), + "rounded KPI geometry plus a label is not media intent: {targets:?}" + ); +} + /// The measured churn: the model rebuilds a section mid-run, the same subject /// searches again, and the session-wide dedup skips the very photo it picked /// the first time — so a real Bali temple photo became a plain blue sky. One @@ -1023,30 +1193,40 @@ fn anonymous_cover_slot_uses_sibling_text_as_query() { /// only ever guards DIFFERENT subjects from sharing a picture. #[test] fn a_repeat_query_gets_the_same_photo_back_not_a_dedup_downgrade() { - use super::{query_key, spawn_job, ImageSearchTarget}; + use super::{ + search_intent_key, spawn_job, ImageSearchTarget, SearchIntentKey, SearchMemoEntry, + }; use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; let used_urls: Arc>> = Arc::new(Mutex::new(HashSet::new())); - let resolved: Arc>> = Arc::new(Mutex::new(HashMap::new())); + let resolved: Arc>> = + Arc::new(Mutex::new(HashMap::new())); // The first search already answered "Bali Indonesia" and marked its photo used. let good = "https://example.org/bali-temple.jpg".to_string(); - resolved - .lock() - .unwrap() - .insert(query_key("Bali Indonesia"), good.clone()); + resolved.lock().unwrap().insert( + search_intent_key("Bali, Indonesia", None), + SearchMemoEntry::Ready(good.clone()), + ); used_urls.lock().unwrap().insert(good.clone()); // The rebuilt card asks again — differently spelled, same subject. let target = ImageSearchTarget { node_id: op_editor_core::NodeId::new("n99".to_string()), - query: " bali indonesia ".to_string(), + query: " bali indonesia ".to_string(), prompt: None, + mode: ImageRequestMode::Search, aspect_ratio: None, width: None, height: None, }; - let job = spawn_job(target, None, Arc::clone(&used_urls), Arc::clone(&resolved)); + let job = spawn_job( + target, + None, + Arc::clone(&used_urls), + Arc::clone(&resolved), + 1, + ); let answer = job .rx .recv_timeout(std::time::Duration::from_secs(1)) @@ -1058,6 +1238,203 @@ fn a_repeat_query_gets_the_same_photo_back_not_a_dedup_downgrade() { ); } +#[test] +fn a_pending_search_intent_is_singleflight() { + use super::{ + search_intent_key, spawn_job, ImageSearchTarget, SearchIntentKey, SearchMemoEntry, + }; + use std::collections::{HashMap, HashSet}; + use std::sync::{Arc, Mutex}; + + let key = search_intent_key("Bali Indonesia", None); + let memo: Arc>> = + Arc::new(Mutex::new(HashMap::from([( + key.clone(), + SearchMemoEntry::Pending { + request_id: 7, + waiters: Vec::new(), + }, + )]))); + let target = ImageSearchTarget { + node_id: NodeId::new("n100"), + query: "Bali, Indonesia".into(), + prompt: None, + mode: ImageRequestMode::Search, + aspect_ratio: None, + width: None, + height: None, + }; + let job = spawn_job( + target, + None, + Arc::new(Mutex::new(HashSet::new())), + Arc::clone(&memo), + 8, + ); + + let waiters = match memo.lock().unwrap().remove(&key) { + Some(SearchMemoEntry::Pending { waiters, .. }) => waiters, + _ => panic!("the second caller joins the pending intent"), + }; + assert_eq!(waiters.len(), 1, "no second fetch thread was created"); + waiters[0] + .send(Some("https://example.org/bali.jpg".into())) + .unwrap(); + assert_eq!( + job.rx.recv_timeout(Duration::from_secs(1)).unwrap(), + Some("https://example.org/bali.jpg".into()) + ); +} + +#[test] +fn stale_pre_reset_request_cannot_publish_into_same_key_in_new_session() { + use super::{publish_search_result, search_intent_key, SearchIntentKey, SearchMemoEntry}; + use std::collections::HashMap; + use std::sync::{mpsc, Arc, Mutex}; + + let key = search_intent_key("Bali Indonesia", None); + let memo: Arc>> = + Arc::new(Mutex::new(HashMap::new())); + let (old_tx, _old_rx) = mpsc::channel(); + memo.lock().unwrap().insert( + key.clone(), + SearchMemoEntry::Pending { + request_id: 10, + waiters: vec![old_tx], + }, + ); + + // reset(), followed by a new document asking for the same intent. + memo.lock().unwrap().clear(); + let (new_tx, new_rx) = mpsc::channel(); + memo.lock().unwrap().insert( + key.clone(), + SearchMemoEntry::Pending { + request_id: 11, + waiters: vec![new_tx], + }, + ); + + assert!(!publish_search_result( + &memo, + key.clone(), + 10, + Some("https://old.example/bali.jpg".into()) + )); + assert!(matches!(new_rx.try_recv(), Err(mpsc::TryRecvError::Empty))); + assert!(publish_search_result( + &memo, + key, + 11, + Some("https://new.example/bali.jpg".into()) + )); + assert_eq!( + new_rx.recv_timeout(Duration::from_secs(1)).unwrap(), + Some("https://new.example/bali.jpg".into()) + ); +} + +#[test] +fn reset_detaches_dedup_and_memo_generations_from_old_threads() { + let mut session = ImageSearchSession::default(); + let old_used = std::sync::Arc::clone(&session.used_urls); + let old_memo = std::sync::Arc::clone(&session.search_memo); + old_used.lock().unwrap().insert("openverse:old".into()); + + session.reset(); + + assert!(!std::sync::Arc::ptr_eq(&old_used, &session.used_urls)); + assert!(!std::sync::Arc::ptr_eq(&old_memo, &session.search_memo)); + old_used + .lock() + .unwrap() + .insert("openverse:late-old-thread".into()); + assert!( + session.used_urls.lock().unwrap().is_empty(), + "late old-document claims stay in the detached generation" + ); +} + +#[test] +fn search_memo_separates_aspect_ratio_intents() { + use super::{search_intent_key, ImageAspectRatio}; + + assert_eq!( + search_intent_key("Bali, Indonesia", Some(ImageAspectRatio::Square)), + search_intent_key("bali indonesia", Some(ImageAspectRatio::Square)), + "fetch-equivalent spelling shares one intent" + ); + assert_ne!( + search_intent_key("Bali Indonesia", Some(ImageAspectRatio::Square)), + search_intent_key("Bali Indonesia", Some(ImageAspectRatio::Wide)), + "a cover and hero must not share a cached crop" + ); +} + +#[test] +fn memo_identity_keeps_lossy_provider_query_collisions_separate() { + let album = "album cover neon lights night"; + let playlist = "playlist artwork neon lights night"; + assert_eq!( + simplify_search_query(album), + simplify_search_query(playlist), + "both authored intents intentionally adapt to the same photo-corpus query" + ); + assert_ne!( + search_intent_key(album, Some(ImageAspectRatio::Square)), + search_intent_key(playlist, Some(ImageAspectRatio::Square)), + "provider adaptation must not merge authored image identity" + ); + + let dome = "santorini greece white buildings blue dome"; + let beach = "santorini greece white buildings sunset beach"; + assert_eq!(simplify_search_query(dome), simplify_search_query(beach)); + assert_ne!( + search_intent_key(dome, Some(ImageAspectRatio::Wide)), + search_intent_key(beach, Some(ImageAspectRatio::Wide)), + "words beyond the provider's four-keyword cap remain part of identity" + ); +} + +#[test] +fn g_style_fill_image_uses_resolved_parent_slot_aspect() { + let state_for_slot = |width: f64, height: f64| { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(serde_json::json!({ + "version":"1.0", "children":[{ + "type":"frame", "id":"root", "width":600, "height":400, "layout":"vertical", + "children":[{ + "type":"frame", "id":"slot", "name":"Bali Hero", "width":width, + "height":height, "layout":"vertical", "clipContent":true, "children":[{ + "type":"image", "id":"photo", "name":"Bali Indonesia", "src":"", + "imagePrompt":"Bali Indonesia", "width":"fill_container", + "height":"fill_container", "objectFit":"crop" + }] + }] + }] + })) + .expect("G-shaped document"); + EditorState::from_document(doc) + }; + + let wide = collect_targets(&state_for_slot(320.0, 180.0), &HashSet::new()) + .into_iter() + .find(|target| target.node_id.as_str() == "photo") + .expect("wide image target"); + let square = collect_targets(&state_for_slot(180.0, 180.0), &HashSet::new()) + .into_iter() + .find(|target| target.node_id.as_str() == "photo") + .expect("square image target"); + + assert_eq!(wide.aspect_ratio, Some(ImageAspectRatio::Wide)); + assert_eq!(square.aspect_ratio, Some(ImageAspectRatio::Square)); + assert_eq!((wide.width, wide.height), (Some(320.0), Some(180.0))); + assert_ne!( + intent_fingerprint(&wide, None), + intent_fingerprint(&square, None), + "a parent-slot aspect change invalidates the in-flight search" + ); +} + /// MiniMax-M3 builds every card around a RECTANGLE named "img" (or a "ph" /// rectangle inside a frame named "img") — neither word was in the keyword /// table, so a whole page of destination cards shipped as grey boxes with no @@ -1113,13 +1490,11 @@ fn m3_style_img_and_ph_rectangles_are_image_slots() { assert_eq!(by_id.get("img"), Some(&"Bali, Indonesia")); } -/// DeepSeek builds a card's photo area as an UNNAMED rectangle sized -/// `fill_container` x `fill_container` — no keyword, no number — so the -/// name-and-authored-size heuristics saw nothing and the page shipped as grey -/// boxes (measured test0711-1-ds, 2026-07-12). What a slot IS is a question -/// about geometry: the resolved layout answers it. +/// Geometry alone cannot distinguish a photo slot from a chart, swatch, or +/// decorative surface. The in-loop diagnostic can ask the model about it, but +/// background enrichment requires explicit media semantics. #[test] -fn an_unnamed_fill_container_rectangle_in_a_card_is_an_image_slot() { +fn an_unnamed_fill_container_rectangle_is_not_auto_filled_from_geometry() { let doc: jian_ops_schema::PenDocument = serde_json::from_str( r##"{ "version": "1.0", "children": [{ "type": "frame", "id": "root", "width": 390, "height": 844, "layout": "vertical", @@ -1142,14 +1517,11 @@ fn an_unnamed_fill_container_rectangle_in_a_card_is_an_image_slot() { .expect("parse"); let state = op_editor_core::EditorState::from_document(doc); let targets = super::collect_targets(&state, &std::collections::HashSet::new()); - let slot = targets - .iter() - .find(|t| t.node_id.as_str() == "slot") - .expect("the photo area is a slot: {targets:?}"); assert!( - slot.query.contains("Santorini") || slot.query.contains("Card"), - "the card's own words say what the picture is: {}", - slot.query + targets + .iter() + .all(|target| target.node_id.as_str() != "slot"), + "an unnamed solid box needs an explicit role/name/query: {targets:?}" ); } @@ -1178,6 +1550,159 @@ fn a_thin_divider_rectangle_is_not_an_image_slot() { ); } +#[test] +fn image_fields_preserve_search_generate_and_legacy_auto_modes() { + let doc: jian_ops_schema::PenDocument = serde_json::from_str( + r##"{ "version":"1.0", "children":[{ + "type":"frame", "id":"root", "width":390, "height":844, "layout":"vertical", + "children":[ + {"type":"image", "id":"search", "name":"Search Photo", "src":"", + "imageSearchQuery":"Kyoto temple", "width":160, "height":90}, + {"type":"image", "id":"generate", "name":"Generated Art", "src":"", + "imagePrompt":"surreal Kyoto at dusk", "width":160, "height":90}, + {"type":"image", "id":"legacy-auto", "name":"Compatible Art", "src":"", + "imageSearchQuery":"Kyoto dusk", "imagePrompt":"surreal Kyoto at dusk", + "width":160, "height":90} + ] + }] }"##, + ) + .expect("parse"); + let state = op_editor_core::EditorState::from_document(doc); + let targets = collect_targets(&state, &std::collections::HashSet::new()); + + assert_eq!( + targets + .iter() + .find(|target| target.node_id.as_str() == "search") + .expect("search target") + .mode, + ImageRequestMode::Search + ); + assert_eq!( + targets + .iter() + .find(|target| target.node_id.as_str() == "generate") + .expect("generate target") + .mode, + ImageRequestMode::Generate + ); + assert_eq!( + targets + .iter() + .find(|target| target.node_id.as_str() == "legacy-auto") + .expect("legacy auto target") + .mode, + ImageRequestMode::Auto + ); +} + +#[test] +fn image_result_claim_is_atomic_across_queries() { + let used = std::sync::Arc::new(std::sync::Mutex::new(std::collections::HashSet::new())); + let winners = (0..8) + .map(|_| { + let used = std::sync::Arc::clone(&used); + std::thread::spawn(move || claim_unused_image_src(&used, "data:image/png;base64,SAME")) + }) + .map(|thread| thread.join().expect("claim thread")) + .filter(|claimed| *claimed) + .count(); + + assert_eq!(winners, 1, "only one concurrent query may claim an image"); + assert!( + used.lock() + .unwrap() + .iter() + .all(|key| key.starts_with("content:") && !key.contains("base64")), + "dedup stores compact digests, not multi-megabyte data URIs" + ); +} + +#[test] +fn provider_identity_reservation_releases_only_unavailable_downloads() { + let used = std::sync::Mutex::new(std::collections::HashSet::from([ + "openverse:unavailable".to_string(), + "openverse:claimed".to_string(), + "openverse:duplicate".to_string(), + ])); + + assert_eq!( + settle_provider_identity( + &used, + "openverse:unavailable", + ImageCandidateClaim::Unavailable, + ), + None + ); + assert_eq!( + settle_provider_identity( + &used, + "openverse:claimed", + ImageCandidateClaim::Claimed("data:image/png;base64,OK".into()), + ), + Some("data:image/png;base64,OK".into()) + ); + assert_eq!( + settle_provider_identity(&used, "openverse:duplicate", ImageCandidateClaim::Duplicate,), + None + ); + + let used = used.lock().unwrap(); + assert!(!used.contains("openverse:unavailable")); + assert!(used.contains("openverse:claimed")); + assert!( + used.contains("openverse:duplicate"), + "a successfully downloaded duplicate remains excluded" + ); +} + +#[test] +fn wikimedia_missing_or_empty_imageinfo_releases_the_page_reservation() { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + let client = reqwest::Client::new(); + for page in [ + serde_json::json!({"pageid": 41, "title": "Missing imageinfo"}), + serde_json::json!({"pageid": 42, "title": "Empty imageinfo", "imageinfo": []}), + ] { + let identity = wikimedia_page_identity(&page).expect("page identity"); + let used = std::sync::Mutex::new(std::collections::HashSet::from([identity.clone()])); + let candidates = wikimedia_image_candidates(&page); + assert!(candidates.is_empty()); + + let outcome = runtime.block_on(first_unused_renderable_image_src( + &client, candidates, &used, + )); + assert_eq!(outcome, ImageCandidateClaim::Unavailable); + assert_eq!(settle_provider_identity(&used, &identity, outcome), None); + assert!( + !used.lock().unwrap().contains(&identity), + "an unusable page must remain retryable" + ); + } +} + +#[test] +fn openverse_claim_uses_artwork_identity_not_thumbnail_variant() { + let used = std::sync::Mutex::new(std::collections::HashSet::new()); + let first = vec![serde_json::json!({ + "id":"art-42", "title":"Kyoto temple", "thumbnail":"https://x/thumb-400.jpg", + "url":"https://x/full.jpg" + })]; + let resized = vec![serde_json::json!({ + "id":"art-42", "title":"Kyoto temple", "thumbnail":"https://x/thumb-800.jpg", + "url":"https://x/full.jpg" + })]; + + assert!(claim_openverse_result(&first, "Kyoto temple", &used).is_some()); + assert!( + claim_openverse_result(&resized, "Kyoto temple", &used).is_none(), + "one artwork id owns all thumbnail/full-size URL variants" + ); +} + /// Forensic: `OP_SLOT_PROBE=` prints every slot the enrichment pass /// would enqueue for a saved document, with the query it would search. #[test] diff --git a/crates/op-host-services/src/mcp_serve/schemas.rs b/crates/op-host-services/src/mcp_serve/schemas.rs index ed19854a3..9224e95f5 100644 --- a/crates/op-host-services/src/mcp_serve/schemas.rs +++ b/crates/op-host-services/src/mcp_serve/schemas.rs @@ -91,7 +91,7 @@ pub const TOOL_SCHEMAS: &[&str] = &[ r#"{"name":"paste_clipboard","description":"Cmd+V parity. Pastes the document clipboard as top-level siblings on the active page, offset by offset_px doc-px (defaults to 10). Mints fresh ids past max_node_id(). Replaces selection with the new ids. Apply-time false when the clipboard is empty or id-space is exhausted.","inputSchema":{"type":"object","properties":{"offset_px":{"type":"string","description":"i32 doc-px offset; defaults to 10 when omitted"}},"required":[]}}"#, // --- write tools --- r##"{"name":"set_variable_color","description":"Set a Color-kind variable's value.","inputSchema":{"type":"object","properties":{"name":{"type":"string"},"hex":{"type":"string","description":"#rgb / #rrggbb / #rrggbbaa"}},"required":["name","hex"]}}"##, - r#"{"name":"batch_design","description":"Insert or refine design content. Accepts nodes_json, I(parent,nodeJson) insert operations, multi-line I/U/C/R/G/M/D programs, or a sandboxed JavaScript program via script. Transactional: if any operation fails, NONE of the batch is applied (the document is unchanged) and errors[] lists every failing line — fix them and resend the whole batch. Keep each call to <=25 operations; split large screens into logical section batches.","inputSchema":{"type":"object","properties":{"filePath":{"type":"string","description":"Optional target .op file path; omit to use the server document"},"nodes_json":{"type":"string","description":"JSON array of simple leaf descriptors"},"operations":{"type":"string","description":"TS batch_design DSL, e.g. root=I(null,{...}), U(\"n1\",{\"x\":10}), D(\"n1\"), M(\"n1\",null), copy=C(root,null,{...}), img=G(\"root\",\"search\",\"prompt\")"},"script":{"type":"string","description":"JavaScript program run in a sandboxed QuickJS: build nodes by calling I(parent, obj) (loops and data arrays allowed; C/U/D/M/R/G and console are no-op stubs). Expands to the operations DSL. Mutually exclusive with nodes_json/operations. Limits: 256 KiB source, 4096 inserts, 2s, 64 MiB, 8 MiB recorded output."},"postProcess":{"type":"boolean"},"canvasWidth":{"type":"number"},"pageId":{"type":"string"}}}}"#, + r#"{"name":"batch_design","description":"Insert or refine design content. Accepts nodes_json, I(parent,nodeJson) insert operations, multi-line I/U/C/R/G/M/D programs, or a sandboxed JavaScript program via script. Transactional: if any operation fails, NONE of the batch is applied (the document is unchanged) and errors[] lists every failing line — fix them and resend the whole batch. Keep each call to <=25 operations; split large screens into logical section batches.","inputSchema":{"type":"object","properties":{"filePath":{"type":"string","description":"Optional target .op file path; omit to use the server document"},"nodes_json":{"type":"string","description":"JSON array of simple leaf descriptors"},"operations":{"type":"string","description":"TS batch_design DSL, e.g. root=I(null,{...}), U(\"n1\",{\"x\":10}), D(\"n1\"), M(\"n1\",null), copy=C(root,null,{...}), img=G(\"image-slot-id\",\"search\",\"prompt\") requires an existing EMPTY target, while img=G(\"rail-id\",\"search\",\"prompt\",\"append\") is accepted only when the parent declares horizontal/vertical layout and should be followed by U(img,{\"width\":120,\"height\":90}); null/populated slot targets and layout-none/omitted append targets are rejected"},"script":{"type":"string","description":"JavaScript program run in a sandboxed QuickJS: build nodes by calling I(parent, obj) or K(kitId,parent,overrides), with loops and data arrays allowed. C/U/D/M/R/G are rejected here with an instruction to use operations; console is a no-op. Expands to the operations DSL. Mutually exclusive with nodes_json/operations. Limits: 256 KiB source, 4096 inserts, 2s, 64 MiB, 8 MiB recorded output."},"postProcess":{"type":"boolean"},"canvasWidth":{"type":"number"},"pageId":{"type":"string"}}}}"#, r#"{"name":"get_design_prompt","description":"Get OpenPencil design-generation prompt knowledge. Pass section for a focused subset; omit it for all sections. style and design-md are derived from the live document's design.md when present.","inputSchema":{"type":"object","properties":{"section":{"type":"string","description":"Prompt section name, e.g. all, layout, style, design-md, elements, codegen-react"},"filePath":{"type":"string","description":"Optional target .op file path; omit to use the server document"}},"required":[]}}"#, r#"{"name":"design_skeleton","description":"Layered design workflow phase 1: create a root frame plus section frames. Accepts TS-style rootFrame/sections plus optional canvasWidth/pageId; legacy nodes_json/operations payloads remain accepted for compatibility.","inputSchema":{"type":"object","properties":{"filePath":{"type":"string","description":"Optional target .op file path; omit to use the server document"},"rootFrame":{"type":"object","description":"Root frame definition: name, width, height, layout, gap, fill, padding"},"sections":{"type":"array","description":"Section frame definitions; each item needs name and may include height, layout, padding, gap, fill, role, justifyContent, alignItems"},"styleGuide":{"type":"object","description":"Optional style guide metadata"},"canvasWidth":{"type":"number","description":"Canvas width for section contentWidth estimates"},"pageId":{"type":"string","description":"Target page ID or index"},"nodes_json":{"type":"string","description":"Legacy JSON array of simple leaf descriptors"},"operations":{"type":"string","description":"Legacy TS batch_design DSL"}},"required":["rootFrame","sections"]}}"#, r#"{"name":"design_content","description":"Layered design workflow phase 2: fill child nodes into a section frame created by design_skeleton. Accepts TS-style sectionId/children plus optional postProcess/canvasWidth/pageId; legacy nodes_json/operations payloads remain accepted for compatibility.","inputSchema":{"type":"object","properties":{"filePath":{"type":"string","description":"Optional target .op file path; omit to use the server document"},"sectionId":{"type":"string","description":"ID of the section frame from design_skeleton"},"children":{"type":"array","description":"Child node definitions to insert under the section"},"postProcess":{"type":"boolean","description":"Apply post-processing after insertion; default true in the TS MCP"},"canvasWidth":{"type":"number","description":"Canvas width for post-processing; default 1200"},"pageId":{"type":"string","description":"Target page ID or index"},"nodes_json":{"type":"string","description":"Legacy JSON array of simple leaf descriptors"},"operations":{"type":"string","description":"Legacy TS batch_design DSL with I(parent,nodeJson) inserts or single U/D/M refine op"}},"required":["sectionId","children"]}}"#, diff --git a/crates/op-mcp/src/batch_design.rs b/crates/op-mcp/src/batch_design.rs index d78479e12..f385834a2 100644 --- a/crates/op-mcp/src/batch_design.rs +++ b/crates/op-mcp/src/batch_design.rs @@ -7,7 +7,7 @@ use jian_ops_schema::node::PenNode; use jian_ops_schema::promote::{promote_frame, widget_kind_for, PromoteNote}; use op_editor_core::{NodeId, PenNodeExt}; -use super::batch_direct_ops::parse_single_direct_operation; +use super::batch_direct_ops::{is_direct_image_operation, parse_single_direct_operation}; use super::batch_layered::{dispatch_design_content, dispatch_design_skeleton}; use super::batch_page::{command_with_outer_page_id, optional_page_id}; use super::write_tools::{validate_hex, ALLOWED_KINDS}; @@ -61,9 +61,61 @@ pub(crate) fn carries_input(args: &BTreeMap, key: &str) -> bool }) } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum BatchInputKind { + Script, + Operations, + NodesJson, +} + +pub(crate) type BatchInputError = (ToolErrorCode, String); + +/// Select the one non-empty write payload. Empty placeholders do not compete, +/// but two real payloads are always an error regardless of argument order. +pub(crate) fn select_batch_input( + args: &BTreeMap, +) -> Result { + let active: Vec = [ + ("script", BatchInputKind::Script), + ("operations", BatchInputKind::Operations), + ("nodes_json", BatchInputKind::NodesJson), + ] + .into_iter() + .filter_map(|(key, kind)| carries_input(args, key).then_some(kind)) + .collect(); + match active.as_slice() { + [kind] => Ok(*kind), + [] => { + // Preserve a lone slot's own parser error (`nodes_json:{}` is + // malformed, `script:""` is empty) while treating placeholders + // as absent whenever another slot carries the real input. + let present: Vec = [ + ("script", BatchInputKind::Script), + ("operations", BatchInputKind::Operations), + ("nodes_json", BatchInputKind::NodesJson), + ] + .into_iter() + .filter_map(|(key, kind)| args.contains_key(key).then_some(kind)) + .collect(); + match present.as_slice() { + [kind] => Ok(*kind), + _ => Err(( + ToolErrorCode::MissingArgument, + "one non-empty input is required: script, operations, or nodes_json".into(), + )), + } + } + _ => Err(( + ToolErrorCode::InvalidArgument, + "provide only one of script, operations, or nodes_json".into(), + )), + } +} + /// Expand a `script` arg into the `operations` DSL program the rest of /// `batch_design` already understands. Returns: -/// - `None` — `args` carries no `script` key; caller proceeds unchanged. +/// - `None` — the one active input is `operations` or `nodes_json`; an empty +/// `script` placeholder does not steal the route. /// - `Some(Ok(rewritten))` — `script` removed, `operations` set to the /// program the sandboxed runner recorded. Caller re-dispatches with the /// rewritten args so BOTH the flat `dispatch_batch_design` path (used by @@ -72,22 +124,17 @@ pub(crate) fn carries_input(args: &BTreeMap, key: &str) -> bool /// (`BatchDesign::call`, which intercepts `operations` before ever /// calling `dispatch_batch_design` — see `batch_design_result.rs`) see /// the exact same expansion and report through their own native shape. -/// - `Some(Err(outcome))` — `script` combined with `operations`/ -/// `nodes_json`, or (feature off) `script` used at all. +/// - `Some(Err(outcome))` — zero/multiple real inputs, or (feature off) a real +/// `script` input. pub(crate) fn expand_script_arg( args: &BTreeMap, ) -> Option, ToolOutcome>> { - let script = args.get("script")?; - // Only a COMPETING input is an error. Models routinely send the unused - // slots along as empty placeholders (`"operations": []`, `"nodes_json": - // ""`, a literal `null`) — rejecting those killed a whole batch over an - // empty field the caller never meant to fill (measured 2026-07-12). - if carries_input(args, "operations") || carries_input(args, "nodes_json") { - return Some(Err(ToolOutcome::Err( - ToolErrorCode::InvalidArgument, - "provide only one of script, operations, or nodes_json".into(), - ))); + match select_batch_input(args) { + Ok(BatchInputKind::Script) => {} + Ok(BatchInputKind::Operations | BatchInputKind::NodesJson) => return None, + Err((code, message)) => return Some(Err(ToolOutcome::Err(code, message))), } + let script = args.get("script").expect("selected script exists"); #[cfg(feature = "script")] { let program = match crate::script_runner::run_script_to_program(script) { @@ -119,16 +166,21 @@ pub(crate) fn dispatch_batch_design( Err(outcome) => outcome, }; } + let input = match select_batch_input(args) { + Ok(input) => input, + Err((code, message)) => return ToolOutcome::Err(code, message), + }; let page_id = optional_page_id(args); - // An empty placeholder (`[]`, `""`, `null`) is not a choice of input: when - // the OTHER slot carries the real program, fall through to it. A lone empty - // slot still reports its own error (an empty descriptor list is a mistake, - // not a missing argument). - let nodes_json_is_real = carries_input(args, "nodes_json"); - if let Some(operations) = args - .get("operations") - .filter(|_| carries_input(args, "operations") || !nodes_json_is_real) - { + if input == BatchInputKind::Operations { + let operations = args.get("operations").expect("selected operations exists"); + if let Some(phase) = phase.filter(|_| is_direct_image_operation(operations)) { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!( + "design_{phase} legacy operations cannot execute G() safely: this compatibility path has no document snapshot, so it cannot enforce G() placement or target geometry. Use batch_design with the same operations payload." + ), + ); + } return match parse_operations(operations) { Ok(ParsedOperations::Insert { parent_id, @@ -171,12 +223,9 @@ pub(crate) fn dispatch_batch_design( Err(e) => ToolOutcome::Err(ToolErrorCode::InvalidArgument, e), }; } - let Some(raw) = args.get("nodes_json") else { - return ToolOutcome::Err( - ToolErrorCode::MissingArgument, - "nodes_json or operations is required".into(), - ); - }; + let raw = args + .get("nodes_json") + .expect("selected nodes_json exists after script expansion"); match parse_batch_items(raw) { Ok(items) if items.is_empty() => ToolOutcome::Err( ToolErrorCode::InvalidArgument, @@ -645,7 +694,6 @@ pub(crate) fn normalize_node_shape(value: &mut serde_json::Value) { // correctly, then every `U(n1,{layout:{type:horizontal…}})` was rejected and // the tree thrashed to empty). normalize_layout_object(obj); - default_container_layout(obj); if let Some(fill) = obj.get_mut("fill") { normalize_fill(fill); } @@ -922,42 +970,6 @@ fn normalize_text_growth(obj: &mut serde_json::Map) { } } -/// A container with flow children and NO `layout` renders as a ROW — the -/// engine's flex default. That is never what an omission means: a section -/// whose children are [title row, card rail] wants them STACKED, and models -/// that want a row always say so explicitly (measured test0711-1-ds: every -/// section frame omitted `layout`, so each title landed to the LEFT of its -/// rail and the cards ran off the screen). An omitted layout stacks. -/// -/// Absolutely-positioned children are out of flow, so a frame whose children -/// all carry `x`/`y` is left alone — its direction is irrelevant, and forcing -/// one could contradict a `layout: none` the caller means to set later. -fn default_container_layout(obj: &mut serde_json::Map) { - const CONTAINERS: [&str; 3] = ["frame", "group", "rectangle"]; - let is_container = obj - .get("type") - .and_then(serde_json::Value::as_str) - .is_some_and(|t| CONTAINERS.contains(&t)); - if !is_container || obj.contains_key("layout") { - return; - } - let flow_children = obj - .get("children") - .and_then(serde_json::Value::as_array) - .map(|kids| { - kids.iter() - .filter(|c| c.get("x").is_none_or(serde_json::Value::is_null)) - .count() - }) - .unwrap_or(0); - if flow_children >= 2 { - obj.insert( - "layout".to_string(), - serde_json::Value::String("vertical".to_string()), - ); - } -} - fn normalize_layout_keyword(obj: &mut serde_json::Map, key: &str) { let Some(serde_json::Value::String(value)) = obj.get_mut(key) else { return; diff --git a/crates/op-mcp/src/batch_design_result.rs b/crates/op-mcp/src/batch_design_result.rs index 382441c45..fe90db768 100644 --- a/crates/op-mcp/src/batch_design_result.rs +++ b/crates/op-mcp/src/batch_design_result.rs @@ -20,8 +20,10 @@ use op_editor_core::{EditorState, NodeId, PenNodeExt}; use serde_json::{json, Value}; use super::batch_design::{ - carries_input, dispatch_batch_design, expand_script_arg, parse_operations, ParsedOperations, + dispatch_batch_design, expand_script_arg, parse_operations, select_batch_input, BatchInputKind, + ParsedOperations, }; +use super::batch_direct_ops::is_direct_image_operation; use super::batch_page::optional_page_id; use super::read_nodes::{page_nodes_snapshots, PageNodes}; use super::{EditorCommand, McpTool, ToolOutcome}; @@ -59,16 +61,14 @@ impl McpTool for BatchDesign { Err(outcome) => outcome, }; } - // An EMPTY `operations` placeholder beside a real `nodes_json` is not a - // program — fall through to the flat dispatch, which picks the slot the - // caller actually filled (a lone empty program still errors there). - let Some(operations) = args - .get("operations") - .filter(|_| carries_input(args, "operations") || !carries_input(args, "nodes_json")) - else { - // nodes_json (a Rust convenience) / missing → flat dispatch. - return dispatch_batch_design(args, None); + let input = match select_batch_input(args) { + Ok(input) => input, + Err((code, message)) => return ToolOutcome::Err(code, message), }; + if input == BatchInputKind::NodesJson { + return dispatch_batch_design(args, None); + } + let operations = args.get("operations").expect("selected operations exists"); match parse_operations(operations) { Ok(ParsedOperations::Insert { parent_id, @@ -77,8 +77,13 @@ impl McpTool for BatchDesign { bindings, promoted, }) => self.insert_with_result(args, parent_id, nodes, bindings, promoted), - // A single direct op keeps the flat dispatch (which re-parses - // and returns the existing command-per-op shape). + // G() needs the document snapshot to size the generated image to + // its target slot. The flat direct parser has no parent geometry, + // so route even a single G through the program simulator. + Ok(ParsedOperations::Direct(_)) if is_direct_image_operation(operations) => { + super::batch_program::run_batch_design_program(&self.state, operations, args) + } + // Other single direct ops keep the flat command-per-op shape. Ok(ParsedOperations::Direct(_)) => dispatch_batch_design(args, None), // Everything else — multi-line MIXED programs, per-line parse // failures — runs the DSL program executor. Transactional by diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index a66073b19..a5300708f 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/crates/op-mcp/src/batch_design_tests.rs @@ -5,7 +5,7 @@ //! `EditorState::apply` checks; the apply-path correctness is covered //! by `op-editor-core`'s `command_tests.rs`. -use super::test_fixtures::sample; +use super::test_fixtures::{frame, sample, state_with}; use super::{BatchInsertItem, EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; use crate::batch_design_snapshot; use op_editor_core::PenNodeExt; @@ -880,62 +880,98 @@ fn batch_design_accepts_bound_single_replace_operation() { #[test] fn batch_design_accepts_single_image_operation_without_fetcher() { - let tool = batch_design_snapshot(&sample()); + let state = state_with(vec![frame("slot", "Slot", 0.0, 0.0, 160.0, 90.0, vec![])]); + let tool = batch_design_snapshot(&state); let mut args = BTreeMap::new(); args.insert( "operations".into(), - r##"G("n10", "search", "hero product photo")"##.into(), + r##"G("slot", "search", "hero product photo")"##.into(), ); match tool.call(&args) { - ToolOutcome::OkWithCommand( + ToolOutcome::OkJsonWithCommand( result, - EditorCommand::InsertSubtree { + EditorCommand::InsertAuthoredSubtree { nodes, parent_id, page_id, }, ) => { - assert_eq!(result.get("count"), Some(&"1".to_string())); - assert_eq!(parent_id.as_str(), "n10"); + let result: serde_json::Value = serde_json::from_str(&result).unwrap(); + assert_eq!(result["results"].as_array().map(Vec::len), Some(1)); + assert_eq!(parent_id.as_str(), "slot"); assert!(page_id.is_none()); assert_eq!(nodes.len(), 1); - assert!(matches!(nodes[0], jian_ops_schema::node::PenNode::Image(_))); + let jian_ops_schema::node::PenNode::Image(image) = &nodes[0] else { + panic!("expected image") + }; + assert!(matches!( + image.width, + Some(jian_ops_schema::sizing::SizingBehavior::Keyword( + jian_ops_schema::sizing::SizingKeyword::FillContainer + )) + )); + assert!(matches!( + image.height, + Some(jian_ops_schema::sizing::SizingBehavior::Keyword( + jian_ops_schema::sizing::SizingKeyword::FillContainer + )) + )); + assert!(matches!( + image.object_fit, + Some(jian_ops_schema::node::ImageFitMode::Crop) + )); assert_eq!(nodes[0].base().name.as_deref(), Some("hero product photo")); } - other => panic!("expected image InsertSubtree command, got {other:?}"), + other => panic!("expected image InsertAuthoredSubtree command, got {other:?}"), } } #[test] fn batch_design_accepts_bound_single_image_operation_without_fetcher() { - let tool = batch_design_snapshot(&sample()); + let state = state_with(vec![frame("slot", "Slot", 0.0, 0.0, 160.0, 90.0, vec![])]); + let tool = batch_design_snapshot(&state); let mut args = BTreeMap::new(); args.insert( "operations".into(), - r##"hero=G(null, "generate", "dashboard background")"##.into(), + r##"hero=G("slot", "generate", "dashboard background")"##.into(), ); match tool.call(&args) { - ToolOutcome::OkWithCommand( + ToolOutcome::OkJsonWithCommand( result, - EditorCommand::InsertSubtree { + EditorCommand::InsertAuthoredSubtree { nodes, parent_id, page_id, }, ) => { - assert_eq!(result.get("count"), Some(&"1".to_string())); - assert!(!parent_id.is_real()); + let result: serde_json::Value = serde_json::from_str(&result).unwrap(); + assert_eq!(result["results"].as_array().map(Vec::len), Some(1)); + assert_eq!(parent_id.as_str(), "slot"); assert!(page_id.is_none()); assert_eq!(nodes.len(), 1); - assert!(matches!(nodes[0], jian_ops_schema::node::PenNode::Image(_))); + let jian_ops_schema::node::PenNode::Image(image) = &nodes[0] else { + panic!("expected image") + }; + assert!(matches!( + image.width, + Some(jian_ops_schema::sizing::SizingBehavior::Keyword( + jian_ops_schema::sizing::SizingKeyword::FillContainer + )) + )); + assert!(matches!( + image.height, + Some(jian_ops_schema::sizing::SizingBehavior::Keyword( + jian_ops_schema::sizing::SizingKeyword::FillContainer + )) + )); assert_eq!( nodes[0].base().name.as_deref(), Some("dashboard background") ); } - other => panic!("expected bound image InsertSubtree command, got {other:?}"), + other => panic!("expected bound image InsertAuthoredSubtree command, got {other:?}"), } } @@ -1466,11 +1502,47 @@ fn a_lone_empty_operations_list_still_reports_its_own_error() { } } -/// Measured test0711-1-ds: every section frame omitted `layout`, so the engine -/// laid [title, card rail] out as a ROW — each section title sat to the LEFT of -/// its cards and the rail ran off the screen. An omitted layout stacks. #[test] -fn a_container_without_a_layout_stacks_its_children() { +fn two_real_batch_inputs_are_rejected_symmetrically() { + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r#"root=I(null,{"type":"frame","width":100,"height":100})"#.into(), + ); + args.insert( + "nodes_json".into(), + r#"[{"kind":"rect","name":"A","x":0,"y":0,"width":10,"height":10}]"#.into(), + ); + match tool.call(&args) { + ToolOutcome::Err(ToolErrorCode::InvalidArgument, message) => { + assert!(message.contains("only one of")); + } + other => panic!("two real inputs must not silently prefer one: {other:?}"), + } +} + +#[test] +fn an_empty_script_does_not_block_real_operations() { + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert("script".into(), String::new()); + args.insert( + "operations".into(), + r#"root=I(null,{"type":"frame","width":100,"height":100})"#.into(), + ); + assert!(matches!( + tool.call(&args), + ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { .. }) + )); +} + +/// Layout omission is ambiguous: a title+rail section probably stacks, while +/// a toolbar or comparison group probably forms a row. Shape normalization +/// must preserve the omission; the agent loop reports it and asks the model to +/// choose explicitly instead of silently inventing design intent. +#[test] +fn a_container_without_a_layout_remains_unspecified() { let mut section = serde_json::json!({ "type": "frame", "id": "n6", "name": "Popular Destinations", "width": "fill_container", "height": 240, @@ -1480,10 +1552,9 @@ fn a_container_without_a_layout_stacks_its_children() { ] }); crate::batch_design::normalize_node_shape(&mut section); - assert_eq!( - section.get("layout").and_then(|v| v.as_str()), - Some("vertical"), - "the section stacks its title above its rail" + assert!( + section.get("layout").is_none(), + "normalization must not guess whether the children form a row or column" ); } diff --git a/crates/op-mcp/src/batch_direct_ops.rs b/crates/op-mcp/src/batch_direct_ops.rs index cd2b08b38..9dc0459a3 100644 --- a/crates/op-mcp/src/batch_direct_ops.rs +++ b/crates/op-mcp/src/batch_direct_ops.rs @@ -33,6 +33,18 @@ pub(crate) fn parse_single_direct_operation(line: &str) -> Result bool { + let line = line.trim().trim_end_matches(';').trim(); + let operation = find_top_level_char(line, '=') + .map(|eq| line[eq + 1..].trim()) + .unwrap_or(line); + operation.starts_with("G(") +} + fn strip_bound_direct_operation(line: &str) -> &str { let Some(eq) = find_top_level_char(line, '=') else { return line; @@ -200,8 +212,8 @@ fn parse_replace_operation(body: &str) -> Result { fn parse_image_operation(body: &str) -> Result { let parts = split_top_level_args(body); - if parts.len() != 3 { - return Err("G() requires parent id, mode, and prompt".into()); + if !matches!(parts.len(), 3 | 4) { + return Err("G() requires parent id, mode, prompt, and optional placement".into()); } let parent_id = parse_parent_node_id(parts[0])?; let mode = parse_ref_token(parts[1])?; @@ -211,17 +223,39 @@ fn parse_image_operation(body: &str) -> Result { )); } let prompt = parse_ref_token(parts[2])?; + if let Some(raw) = parts.get(3) { + let placement = parse_ref_token(raw)?; + if !matches!(placement.as_str(), "slot" | "append") { + return Err(format!( + "G() placement must be \"slot\" or \"append\", got {placement:?}" + )); + } + } let name = prompt.chars().take(40).collect::(); - let node: PenNode = serde_json::from_value(serde_json::json!({ + let (width, height) = if parent_id.is_real() { + ( + serde_json::json!("fill_container"), + serde_json::json!("fill_container"), + ) + } else { + (serde_json::json!(400), serde_json::json!(300)) + }; + let mut value = serde_json::json!({ "type": "image", "id": "__op_tmp_image_1", "name": name, - "imagePrompt": prompt, "src": "", - "width": 400, - "height": 300 - })) - .map_err(|e| format!("invalid G() image node: {e}"))?; + "objectFit": "crop", + "width": width, + "height": height + }); + if mode == "generate" { + value["imagePrompt"] = serde_json::json!(prompt); + } else { + value["imageSearchQuery"] = serde_json::json!(prompt); + } + let node: PenNode = + serde_json::from_value(value).map_err(|e| format!("invalid G() image node: {e}"))?; Ok(EditorCommand::InsertSubtree { nodes: vec![node], parent_id, diff --git a/crates/op-mcp/src/batch_layered_tests.rs b/crates/op-mcp/src/batch_layered_tests.rs index 284c14a23..d8c943cc6 100644 --- a/crates/op-mcp/src/batch_layered_tests.rs +++ b/crates/op-mcp/src/batch_layered_tests.rs @@ -353,6 +353,43 @@ fn design_refine_rejects_script_combined_with_structured_payload() { } } +#[test] +fn legacy_phase_tools_reject_direct_images_with_batch_design_guidance() { + fn assert_rejected(outcome: ToolOutcome, phase_tool: &str) { + match outcome { + ToolOutcome::Err(code, message) => { + assert_eq!(code, ToolErrorCode::InvalidArgument); + assert!(message.contains(phase_tool), "{message}"); + assert!(message.contains("G()"), "{message}"); + assert!(message.contains("placement"), "{message}"); + assert!(message.contains("target geometry"), "{message}"); + assert!(message.contains("batch_design"), "{message}"); + } + other => panic!("expected legacy phase G() rejection, got {other:?}"), + } + } + + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r#"G("slot-1", "search", "Prague coffee")"#.into(), + ); + assert_rejected(design_skeleton_snapshot().call(&args), "design_skeleton"); + + args.insert( + "operations".into(), + r#"photo=G("row-1", "generate", "Coffee shop", "append")"#.into(), + ); + assert_rejected(design_content_snapshot().call(&args), "design_content"); + + args.insert( + "operations".into(), + r#"G("slot-2", "search", "Coffee cup", "slot")"#.into(), + ); + let state = EditorState::new(); + assert_rejected(design_refine_snapshot(&state).call(&args), "design_refine"); +} + #[test] fn design_refine_missing_root_errors_like_ts() { // TS design-refine throws "Root node not found: " when the root is diff --git a/crates/op-mcp/src/batch_program.rs b/crates/op-mcp/src/batch_program.rs index 965f47ea6..92633b864 100644 --- a/crates/op-mcp/src/batch_program.rs +++ b/crates/op-mcp/src/batch_program.rs @@ -53,9 +53,10 @@ //! first, so override keys (source ids) never match — a no-op there, //! an explicit skip here. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; -use jian_ops_schema::node::PenNode; +use jian_ops_schema::node::{ContainerProps, LayoutMode, PenNode}; +use jian_scene::layout_scene::SceneNode; use op_editor_core::command_node::remap_subtree_ids_mapping; use op_editor_core::{EditorState, NodeId, PenNodeExt}; use regex::Regex; @@ -84,6 +85,7 @@ pub(crate) fn run_batch_design_program( .or_else(|| args.get("post_process")) .map(|raw| matches!(raw.trim(), "true" | "1")) .unwrap_or(false); + let lines = split_operations(operations); let mut ctx = ProgramCtx { sim: snapshot.clone(), page_id: page_id.clone(), @@ -93,6 +95,8 @@ pub(crate) fn run_batch_design_program( commands: Vec::new(), post_process, auto_seq: 0, + current_line: 0, + explicitly_sized_append_lines: explicitly_sized_append_lines(&lines), }; // Pin the sim's active page to the requested page so sim READS // (path lookups, node counts) see the same children every emitted @@ -113,7 +117,8 @@ pub(crate) fn run_batch_design_program( let transactional = args.get("_line_policy").map(String::as_str) != Some("best_effort"); let mut errors: Vec = Vec::new(); - for line in split_operations(operations) { + for (line_index, line) in lines.into_iter().enumerate() { + ctx.current_line = line_index; if let Err(error) = execute_line(&line, &mut ctx) { errors.push(json!({ "line": line_preview(&line), "error": error })); } @@ -174,6 +179,11 @@ struct ProgramCtx { post_process: bool, /// Monotonic counter for `_auto_*` bindless-line bindings. auto_seq: usize, + /// Index of the operation currently being executed. + current_line: usize, + /// Append G() lines whose result binding receives explicit positive + /// numeric width and height later in this same program. + explicitly_sized_append_lines: BTreeSet, } impl ProgramCtx { @@ -265,6 +275,99 @@ fn execute_line(line: &str, ctx: &mut ProgramCtx) -> Result<(), String> { Err(format!("Cannot parse operation: {line}")) } +/// Return append-G line indexes that are robustly sized by later U() calls in +/// the same program. Parsing uses the DSL's top-level delimiter rules, so an +/// '=' inside a quoted image prompt is never mistaken for a result binding. +fn explicitly_sized_append_lines(lines: &[String]) -> BTreeSet { + let mut sized = BTreeSet::new(); + for (index, line) in lines.iter().enumerate() { + let Some((Some(binding), 'G', args)) = parsed_operation(line) else { + continue; + }; + let parts = split_top_level_args(args); + let is_append = parts.len() == 4 + && matches!( + serde_json::from_str::(parts[3].trim()), + Ok(placement) if placement == "append" + ); + if !is_append { + continue; + } + + let mut width_is_positive_number = false; + let mut height_is_positive_number = false; + for later in &lines[index + 1..] { + let Some((later_binding, op, later_args)) = parsed_operation(later) else { + continue; + }; + // Rebinding closes this append's sizing window. A U() beyond it + // would target the newer node, not this image. + if later_binding == Some(binding) { + break; + } + if op != 'U' { + continue; + } + let Some(comma) = find_top_level_char(later_args, ',') else { + continue; + }; + let target = strip_outer_quotes(later_args[..comma].trim()); + if target != binding { + continue; + } + let Ok(value) = parse_json_arg(&later_args[comma + 1..]) else { + continue; + }; + let Some(patch) = value.as_object() else { + continue; + }; + if let Some(width) = patch.get("width") { + width_is_positive_number = positive_json_number(width); + } + if let Some(height) = patch.get("height") { + height_is_positive_number = positive_json_number(height); + } + } + if width_is_positive_number && height_is_positive_number { + sized.insert(index); + } + } + sized +} + +/// Parse one complete DSL operation without splitting on delimiters nested in +/// calls or quoted strings. Returns `(binding, opcode, argument body)`. +fn parsed_operation(line: &str) -> Option<(Option<&str>, char, &str)> { + let line = line.trim().trim_end_matches(';').trim(); + let (binding, call) = match find_top_level_char(line, '=') { + Some(eq) => { + let binding = line[..eq].trim(); + if binding.is_empty() + || !binding + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '_') + { + return None; + } + (Some(binding), line[eq + 1..].trim()) + } + None => (None, line), + }; + let mut chars = call.chars(); + let op = chars.next()?; + let rest = chars.as_str(); + if !rest.starts_with('(') || !call.ends_with(')') { + return None; + } + Some((binding, op, &rest[1..rest.len() - 1])) +} + +fn positive_json_number(value: &Value) -> bool { + value + .as_f64() + .is_some_and(|number| number.is_finite() && number > 0.0) +} + fn execute_assign(op: &str, binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { match op { "I" => execute_insert(binding, args, ctx), @@ -537,28 +640,138 @@ fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<() Ok(()) } -/// `binding=G(parent, mode, prompt)` — emit an image node. TS requires -/// every argument quoted; `mode` must be `search` or `generate`. No -/// fetcher at this layer — `src` stays empty (browser-caller parity); +/// `binding=G(parent, mode, prompt[, placement])` — emit an image node. The +/// parent accepts a binding or quoted existing id (`null` is rejected because +/// both placements require a concrete target); `mode` must be `search` or +/// `generate`. Placement defaults to `slot`; the explicit `append` escape +/// hatch allows a new sibling only under a horizontal/vertical flow parent. +/// No fetcher at this layer — `src` stays empty (browser-caller parity); /// the host's own image pipeline enriches later. fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { - let g = regex(r#"^"([^"]+)"\s*,\s*"(search|generate)"\s*,\s*"([^"]+)"$"#); - let Some(c) = g.captures(args.trim()) else { + let parts = split_top_level_args(args); + if !matches!(parts.len(), 3 | 4) { return Err(format!("Invalid G() syntax: {args}")); + } + let parent_raw = parts[0].trim(); + let parent = if matches!(parent_raw, "null" | "undefined" | "0" | "\"\"" | "\"0\"") { + String::new() + } else { + resolve_path_expr(parent_raw, &ctx.bindings) }; - let parent = resolve_ref(c.get(1).map_or("", |m| m.as_str()), &ctx.bindings); - let prompt = c.get(3).map_or("", |m| m.as_str()); + let mode = serde_json::from_str::(parts[1].trim()) + .map_err(|_| format!("Invalid G() syntax: {args}"))?; + if !matches!(mode.as_str(), "search" | "generate") { + return Err(format!("G() mode must be search or generate: {mode}")); + } + let prompt = serde_json::from_str::(parts[2].trim()) + .map_err(|_| format!("Invalid G() syntax: {args}"))?; + let placement = match parts.get(3) { + None => "slot".to_string(), + Some(raw) => serde_json::from_str::(raw.trim()) + .map_err(|_| format!("Invalid G() syntax: {args}"))?, + }; + if !matches!(placement.as_str(), "slot" | "append") { + return Err(format!( + "G() placement must be \"slot\" or \"append\", got {placement:?}" + )); + } let name: String = prompt.chars().take(40).collect(); - let node: PenNode = serde_json::from_value(json!({ + let mut value = json!({ "type": "image", "id": "__op_tmp_image_1", "name": name, - "imagePrompt": prompt, "src": "", + "objectFit": "crop", "width": 400, "height": 300 - })) - .map_err(|e| format!("invalid G() image node: {e}"))?; + }); + if mode == "generate" { + value["imagePrompt"] = json!(prompt); + } else { + value["imageSearchQuery"] = json!(prompt); + } + if parent.trim().is_empty() || parent.trim() == "0" { + return Err(format!( + "G() placement {placement:?} requires an explicit frame/rectangle target id; create the target first instead of using null" + )); + } + let target = find_node_by_path(ctx.sim.active_children(), &parent, &ctx.alias) + .ok_or_else(|| format!("G() parent not found or not a container: {parent}"))?; + // `parent` may be a slash path or an authored id that `find_node_by_path` + // translated through `ctx.alias`. The emitted insert must target the live + // resolved node id, never the caller's path/alias spelling. + let target_id = target.id_str().to_string(); + let container = node_container(target) + .ok_or_else(|| format!("G() parent not found or not a container: {parent}"))?; + // Placement is an explicit structural contract. Slot-fill accepts only an + // EMPTY target; append accepts only an explicitly-authored flow parent. + // Never recover intent from names, dimensions, child kinds, or position. + match placement.as_str() { + "slot" => { + let child_ids = target + .children() + .into_iter() + .flatten() + .map(PenNode::id_str) + .collect::>(); + if !child_ids.is_empty() { + return Err(format!( + "G() slot target {} must be empty, but it has children [{}]. Pass the exact empty frame/rectangle slot id; use \"append\" only for an intentional child of an explicit horizontal/vertical flow parent", + target.id_str(), + child_ids.join(", ") + )); + } + } + "append" => { + if explicit_flow_layout(container).is_none() { + return Err(format!( + "G() append target {} must declare layout \"horizontal\" or \"vertical\"; got {}. Append means a new flow sibling and is never an absolute overlay", + target.id_str(), + layout_label(container) + )); + } + if !ctx + .explicitly_sized_append_lines + .contains(&ctx.current_line) + { + return Err( + "G() append requires a result binding followed in the same batch by U(binding, {\"width\": , \"height\": }); refusing an unsized flow child with default fill_container width and height" + .into(), + ); + } + } + _ => unreachable!("placement validated above"), + } + if matches!(container.layout, Some(LayoutMode::None)) { + let has_declared_size = container.width.is_some() && container.height.is_some(); + let resolved = has_declared_size + .then(|| resolved_node_size(&ctx.sim, &target_id)) + .flatten(); + let width = target.width_px().or_else(|| resolved.map(|size| size.0)); + let height = target.height_px().or_else(|| resolved.map(|size| size.1)); + let (Some(width), Some(height)) = (width, height) else { + return Err(format!( + "G() target {target_id} uses layout none, so it needs declared width and height that resolve above zero before an image can fill it" + )); + }; + if width <= 0.0 || height <= 0.0 { + return Err(format!( + "G() target {target_id} uses layout none, so it needs declared width and height that resolve above zero before an image can fill it" + )); + } + value["x"] = json!(0); + value["y"] = json!(0); + value["width"] = json!(width); + value["height"] = json!(height); + } else { + // A G() image serves its target slot; its intrinsic/search size is + // not evidence for card geometry. Flex sizing keeps the image + // inside the slot and lets crop/object-fit do the visual work. + value["width"] = json!("fill_container"); + value["height"] = json!("fill_container"); + } + let node: PenNode = + serde_json::from_value(value).map_err(|e| format!("invalid G() image node: {e}"))?; let mut nodes = vec![node]; let map = ctx.remap(&mut nodes)?; let image_id = map @@ -568,7 +781,7 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), ctx.emit( EditorCommand::InsertAuthoredSubtree { nodes, - parent_id: parent_node_id(Some(&parent)), + parent_id: NodeId::new(&target_id), page_id: ctx.page_id.clone(), }, &format!("G() parent not found or not a container: {parent}"), @@ -577,6 +790,53 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), Ok(()) } +fn explicit_flow_layout(container: &ContainerProps) -> Option<&'static str> { + match container.layout { + Some(LayoutMode::Horizontal) => Some("horizontal"), + Some(LayoutMode::Vertical) => Some("vertical"), + _ => None, + } +} + +fn layout_label(container: &ContainerProps) -> &'static str { + match container.layout { + Some(LayoutMode::Horizontal) => "horizontal", + Some(LayoutMode::Vertical) => "vertical", + Some(LayoutMode::None) => "none", + None => "omitted", + } +} + +fn node_container(node: &PenNode) -> Option<&ContainerProps> { + match node { + PenNode::Frame(node) => Some(&node.container), + PenNode::Group(node) => Some(&node.container), + PenNode::Rectangle(node) => Some(&node.container), + _ => None, + } +} + +fn resolved_node_size(state: &EditorState, node_id: &str) -> Option<(f64, f64)> { + fn find(nodes: &[SceneNode], node_id: &str) -> Option<(f64, f64)> { + for node in nodes { + if node.id == node_id { + let bounds = node.aggregate_bounds(); + return Some((f64::from(bounds.size.x), f64::from(bounds.size.y))); + } + if let Some(size) = find(&node.children, node_id) { + return Some(size); + } + } + None + } + + let scene = op_pen_loader::editor_state_to_layout_scene(state); + scene + .pages + .iter() + .find_map(|page| find(&page.children, node_id)) +} + /// `U(path, data)` — shallow-patch the node at `path`. No result entry /// (TS call-form ops don't push results). fn execute_update(args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { diff --git a/crates/op-mcp/src/batch_program_image_tests.rs b/crates/op-mcp/src/batch_program_image_tests.rs new file mode 100644 index 000000000..acc128e91 --- /dev/null +++ b/crates/op-mcp/src/batch_program_image_tests.rs @@ -0,0 +1,379 @@ +//! Image/G() placement tests for the multi-op batch-design executor. + +use super::*; + +#[test] +fn image_op_accepts_binding_or_quoted_parent_syntax() { + // Best-effort policy: the deliberately-bad third line must be + // dropped (not roll back the batch) so the G() parse + binding + // resolution of the good lines stays observable. + let mut state = sample(); + let program = r##"wrap=I(null, {"type":"frame","name":"Wrap","x":500,"y":0,"width":400,"height":300}) +img=G(wrap, "search", "sunset photo") +G(wrap, "search")"##; + let (envelope, cmd) = call_operations_best_effort(&state, program); + let errors = envelope["errors"].as_array().expect("errors"); + assert_eq!(errors.len(), 1, "{envelope}"); + assert!( + errors[0]["error"] + .as_str() + .unwrap() + .starts_with("Invalid G() syntax:"), + "{envelope}" + ); + let wrap_id = binding_id(&envelope, "wrap"); + let img_id = binding_id(&envelope, "img"); + + assert!(state.apply(cmd.expect("command"))); + let wrap = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(&wrap_id)) + .expect("wrap"); + let img = wrap + .children() + .expect("children") + .iter() + .find(|c| c.id_str() == img_id) + .expect("image under wrap"); + let jian_ops_schema::node::PenNode::Image(image) = img else { + panic!("expected image") + }; + assert_eq!(img.base().name.as_deref(), Some("sunset photo")); + assert_eq!(image.image_search_query.as_deref(), Some("sunset photo")); + assert_eq!(image.image_prompt, None); +} + +#[test] +fn image_op_preserves_generate_mode_on_the_node() { + let mut state = sample(); + let program = r##"slot=I(null, {"type":"frame","name":"Hero","x":500,"y":0,"width":160,"height":90,"layout":"none"}) +img=G(slot, "generate", "cinematic sunset coast")"##; + let (envelope, cmd) = call_operations(&state, program); + let image_id = binding_id(&envelope, "img"); + + assert!(state.apply(cmd.expect("command"))); + let image = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(image_id)) + .expect("image"); + let jian_ops_schema::node::PenNode::Image(image) = image else { + panic!("expected image") + }; + assert_eq!( + image.image_prompt.as_deref(), + Some("cinematic sunset coast") + ); + assert_eq!(image.image_search_query, None); +} + +#[test] +fn image_op_rejects_a_populated_flow_row_and_accepts_its_explicit_slot() { + // Exact live-QA failure shape: the row already owns a cover slot, text + // column, and play control. G(row, ...) used to append the image as a + // fourth sibling, leaving the cover slot empty. The contract gate uses + // only explicit layout/parent structure; it does not infer intent from + // these fixture names or their 56px dimensions. + let row: jian_ops_schema::node::PenNode = serde_json::from_value(serde_json::json!({ + "type": "frame", "id": "n42", "name": "Daily Mix 1", + "layout": "horizontal", "width": "fill_container", "height": "fit_content", + "children": [ + {"type": "frame", "id": "n43", "name": "Daily Mix 1 Cover", "layout": "none", "width": 56, "height": 56}, + {"type": "frame", "id": "n44", "name": "Daily Mix 1 Text", "layout": "vertical", "width": "fill_container", "height": "fit_content"}, + {"type": "icon_font", "id": "n47", "iconFontFamily": "lucide", "iconFontName": "play", "width": 20, "height": 20} + ] + })) + .unwrap(); + let mut state = state_with(vec![row]); + + let (rejected, command) = call_operations( + &state, + r##"img=G("n42", "search", "green forest ambient")"##, + ); + assert!( + command.is_none(), + "the bad parent must not mutate: {rejected}" + ); + assert_eq!(rejected["applied"], Value::Bool(false)); + let message = rejected["errors"][0]["error"] + .as_str() + .expect("G target error"); + assert!( + message.contains("slot target n42 must be empty"), + "{message}" + ); + assert!(message.contains("[n43, n44, n47]"), "{message}"); + assert!(message.contains("exact empty"), "{message}"); + + let (accepted, command) = call_operations( + &state, + r##"img=G("n43", "search", "green forest ambient")"##, + ); + let image_id = binding_id(&accepted, "img"); + assert!(state.apply(command.expect("slot-targeted G command"))); + let slot = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n43")) + .expect("explicit slot"); + assert!( + slot.children() + .is_some_and(|children| children.iter().any(|child| child.id_str() == image_id)), + "the accepted image must land inside the explicit slot" + ); + let row = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n42")) + .expect("row"); + assert_eq!( + row.children().map(Vec::len), + Some(3), + "no fourth row sibling" + ); + + let (duplicate, command) = + call_operations(&state, r##"duplicate=G("n43", "search", "second cover")"##); + assert!(command.is_none(), "populated slot must reject: {duplicate}"); + let message = duplicate["errors"][0]["error"] + .as_str() + .expect("populated slot error"); + assert!( + message.contains("slot target n43 must be empty"), + "{message}" + ); + assert!(message.contains(&image_id), "{message}"); + + let (overlay, command) = call_operations( + &state, + r##"overlay=G("n43", "search", "overlay", "append")"##, + ); + assert!( + command.is_none(), + "layout-none append must reject: {overlay}" + ); + let message = overlay["errors"][0]["error"] + .as_str() + .expect("append layout error"); + assert!(message.contains("must declare layout"), "{message}"); + assert!(message.contains("got none"), "{message}"); + assert!(message.contains("never an absolute overlay"), "{message}"); + + let (unsized_result, command) = call_operations( + &state, + r##"img=G("n42", "search", "unsized gallery artwork", "append")"##, + ); + assert!( + command.is_none(), + "unsized append must reject: {unsized_result}" + ); + assert_eq!( + unsized_result["applied"], + Value::Bool(false), + "{unsized_result}" + ); + let message = unsized_result["errors"][0]["error"] + .as_str() + .expect("append sizing error"); + assert!( + message.contains("followed in the same batch by U"), + "{message}" + ); + assert!(message.contains("positive number"), "{message}"); + assert!(message.contains("fill_container"), "{message}"); + + let (appended, command) = call_operations( + &state, + r##"img=G("n42", "search", "new gallery artwork", "append") +U(img, {"width":72,"height":72})"##, + ); + let appended_id = binding_id(&appended, "img"); + assert!(state.apply(command.expect("explicit append command"))); + let row = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n42")) + .expect("row"); + let appended = row + .children() + .expect("row children") + .iter() + .find(|child| child.id_str() == appended_id) + .expect("explicit image sibling"); + assert_eq!(appended.width_px(), Some(72.0)); + assert_eq!(appended.height_px(), Some(72.0)); + assert_eq!(row.children().map(Vec::len), Some(4)); +} + +#[test] +fn bindless_image_with_equals_in_prompt_cannot_bypass_populated_flow_slot_gate() { + let row: jian_ops_schema::node::PenNode = serde_json::from_value(serde_json::json!({ + "type": "frame", "id": "n42", "name": "Event Rail", + "layout": "horizontal", "width": "fill_container", "height": "fit_content", + "children": [ + {"type": "frame", "id": "n43", "name": "Cover Slot", "layout": "none", "width": 168, "height": 112}, + {"type": "frame", "id": "n44", "name": "Event Details", "layout": "vertical", "width": "fill_container", "height": "fit_content"} + ] + })) + .unwrap(); + let state = state_with(vec![row]); + + // The '=' is data inside the quoted prompt, not a result-binding + // delimiter. This bindless call must still route through the snapshot + // program validator instead of the geometry-blind direct parser. + let (rejected, command) = call_operations( + &state, + r##"G("n42", "search", "festival crowd ratio=16:9")"##, + ); + + assert!(command.is_none(), "populated row must reject: {rejected}"); + assert_eq!(rejected["applied"], Value::Bool(false), "{rejected}"); + let message = rejected["errors"][0]["error"] + .as_str() + .expect("strict G slot error"); + assert!( + message.contains("slot target n42 must be empty"), + "{message}" + ); + assert!(message.contains("[n43, n44]"), "{message}"); +} + +#[test] +fn image_op_inserts_under_live_target_resolved_from_slash_path_and_authored_alias() { + let mut state = op_editor_core::EditorState::new(); + let program = r##"root=I(null, {"type":"frame","id":"auth-root","name":"Root","width":390,"height":844,"layout":"vertical","children":[{"type":"frame","id":"auth-slot","name":"Hero Slot","width":160,"height":90,"layout":"none"}]}) +img=G(root+"/auth-slot", "search", "sunset coast ratio=16:9")"##; + + let (envelope, command) = call_operations(&state, program); + assert!(envelope.get("errors").is_none(), "{envelope}"); + let root_id = binding_id(&envelope, "root"); + let image_id = binding_id(&envelope, "img"); + assert!(state.apply(command.expect("slash-path G command"))); + + let root = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(&root_id)) + .expect("root"); + let slot = root + .children() + .expect("root children") + .iter() + .find(|child| child.base().name.as_deref() == Some("Hero Slot")) + .expect("authored slot remapped to a live child"); + assert_ne!(slot.id_str(), "auth-slot", "authored id must be remapped"); + assert!( + slot.children() + .is_some_and(|children| children.iter().any(|child| child.id_str() == image_id)), + "image must be inserted under the resolved live slot id" + ); +} + +#[test] +fn image_op_rejects_populated_layout_omitted_slot_and_omitted_append_parent() { + let populated = sample(); + let (slot_result, command) = + call_operations(&populated, r##"img=G("n10", "search", "product photo")"##); + assert!( + command.is_none(), + "populated slot must reject: {slot_result}" + ); + let message = slot_result["errors"][0]["error"] + .as_str() + .expect("slot error"); + assert!( + message.contains("slot target n10 must be empty"), + "{message}" + ); + assert!(message.contains("[n11, n12]"), "{message}"); + + let empty_omitted = state_with(vec![frame("slot", "Slot", 0.0, 0.0, 100.0, 80.0, vec![])]); + let (append_result, command) = call_operations( + &empty_omitted, + r##"img=G("slot", "search", "product photo", "append")"##, + ); + assert!( + command.is_none(), + "omitted-layout append must reject: {append_result}" + ); + let message = append_result["errors"][0]["error"] + .as_str() + .expect("append error"); + assert!(message.contains("must declare layout"), "{message}"); + assert!(message.contains("got omitted"), "{message}"); +} + +#[test] +fn image_op_rejects_a_missing_target_for_slot_and_append() { + let state = sample(); + for operation in [ + r##"img=G(null, "search", "photo")"##, + r##"img=G(null, "search", "photo", "append")"##, + ] { + let (envelope, command) = call_operations(&state, operation); + assert!(command.is_none(), "null target must reject: {envelope}"); + assert!(envelope["errors"][0]["error"] + .as_str() + .is_some_and(|message| message.contains("explicit frame/rectangle target id"))); + } +} + +#[test] +fn image_op_rejects_an_unknown_placement() { + let state = sample(); + let (envelope, command) = call_operations( + &state, + r##"img=G("n10", "search", "product photo", "guess")"##, + ); + assert!(command.is_none(), "transaction rolls back: {envelope}"); + assert!(envelope["errors"][0]["error"] + .as_str() + .is_some_and(|message| message.contains("placement must be"))); +} + +#[test] +fn image_op_fills_an_absolute_slot_without_importing_400x300_geometry() { + let mut state = sample(); + let program = r##"slot=I(null, {"type":"frame","name":"Hero Image","x":500,"y":0,"width":160,"height":90,"layout":"none","clipContent":true}) +img=G("slot", "search", "sunset coast")"##; + let (envelope, cmd) = call_operations(&state, program); + let slot_id = binding_id(&envelope, "slot"); + let img_id = binding_id(&envelope, "img"); + + assert!(state.apply(cmd.expect("command"))); + let slot = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(slot_id)) + .expect("slot"); + let image = slot + .children() + .expect("children") + .iter() + .find(|node| node.id_str() == img_id) + .expect("image"); + assert_eq!(image.width_px(), Some(160.0)); + assert_eq!(image.height_px(), Some(90.0)); + assert_eq!(image.base().x, Some(0.0)); + assert_eq!(image.base().y, Some(0.0)); +} + +#[test] +fn image_op_uses_resolved_width_for_fill_sized_absolute_slot() { + let mut state = sample(); + let program = r##"root=I(null, {"type":"frame","name":"Travel","x":500,"y":0,"width":390,"height":844,"layout":"vertical"}) +slot=I(root, {"type":"frame","name":"Hero Image","width":"fill_container","height":140,"layout":"none","clipContent":true}) +img=G(slot, "search", "sunset coast")"##; + let (envelope, cmd) = call_operations(&state, program); + let slot_id = binding_id(&envelope, "slot"); + let img_id = binding_id(&envelope, "img"); + + assert!(state.apply(cmd.expect("command"))); + let slot = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(slot_id)) + .expect("slot"); + let image = slot + .children() + .expect("children") + .iter() + .find(|node| node.id_str() == img_id) + .expect("image"); + assert_eq!(image.width_px(), Some(390.0)); + assert_eq!(image.height_px(), Some(140.0)); + assert_eq!(image.base().x, Some(0.0)); + assert_eq!(image.base().y, Some(0.0)); +} + +#[test] +fn image_op_rejects_an_unsized_absolute_slot() { + let state = sample(); + let program = r##"slot=I(null, {"type":"frame","name":"Hero Image","layout":"none"}) +img=G("slot", "search", "sunset coast")"##; + let (envelope, cmd) = call_operations(&state, program); + + assert!(cmd.is_none(), "transaction rolls back: {envelope}"); + assert!(envelope["errors"].as_array().is_some_and(|errors| errors + .iter() + .any(|error| error["error"] + .as_str() + .is_some_and(|message| message.contains("declared width and height"))))); +} diff --git a/crates/op-mcp/src/batch_program_tests.rs b/crates/op-mcp/src/batch_program_tests.rs index fd4a509c5..06675245c 100644 --- a/crates/op-mcp/src/batch_program_tests.rs +++ b/crates/op-mcp/src/batch_program_tests.rs @@ -223,6 +223,115 @@ b=I("n10", {"type":"rectangle","name":"B","width":10,"height":10})"##; ); } +#[test] +fn failed_rail_reconstruction_keeps_all_four_populated_cards_byte_identical() { + let cards: Vec = (1..=4) + .map(|index| { + serde_json::from_value(serde_json::json!({ + "type": "frame", + "id": format!("card-{index}"), + "name": format!("Event Card {index}"), + "layout": "vertical", + "width": 168, + "height": 220, + "children": [ + { + "type": "image", + "id": format!("card-{index}-image"), + "name": format!("Event {index} Photo"), + "src": format!("https://example.invalid/event-{index}.jpg"), + "objectFit": "crop", + "width": "fill_container", + "height": 112 + }, + { + "type": "frame", + "id": format!("card-{index}-details"), + "name": format!("Event {index} Details"), + "layout": "vertical", + "width": "fill_container", + "height": "fit_content", + "children": [ + { + "type": "text", + "id": format!("card-{index}-title"), + "name": "Title", + "content": format!("Popular Event {index}"), + "width": "fill_container", + "height": 24 + }, + { + "type": "text", + "id": format!("card-{index}-venue"), + "name": "Venue", + "content": format!("Venue {index}"), + "width": "fill_container", + "height": 18 + } + ] + } + ] + })) + .expect("populated event card") + }) + .collect(); + let rail: jian_ops_schema::node::PenNode = serde_json::from_value(serde_json::json!({ + "type": "frame", + "id": "event-rail", + "name": "Popular Near You", + "layout": "horizontal", + "width": 390, + "height": 220, + "children": cards + })) + .expect("populated rail"); + let state = state_with(vec![rail]); + let before = serde_json::to_vec(state.active_children()).expect("pre-transaction bytes"); + + // This mirrors the destructive-redraft failure mode: four valid cards are + // deleted in the simulated batch, then reconstruction targets a bad id. + // Transactional execution must ship no partial delete commands. + let program = r##"D("card-1") +D("card-2") +D("card-3") +D("card-4") +replacement=I("missing-rail-parent", {"type":"frame","name":"Rebuilt Card","width":168,"height":220,"children":[{"type":"text","content":"replacement","width":120,"height":24}]})"##; + let (envelope, command) = call_operations(&state, program); + + assert!( + command.is_none(), + "failed reconstruction must not emit deletes" + ); + assert_eq!(envelope["applied"], Value::Bool(false), "{envelope}"); + assert_eq!(envelope["results"], serde_json::json!([]), "{envelope}"); + assert!(envelope["errors"][0]["error"] + .as_str() + .is_some_and(|message| message.contains("missing-rail-parent"))); + assert_eq!( + serde_json::to_vec(state.active_children()).expect("post-transaction bytes"), + before, + "the original populated rail must remain byte-identical" + ); + + let rail = + op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("event-rail")) + .expect("original rail"); + let cards = rail.children().expect("original cards"); + assert_eq!(cards.len(), 4, "all four cards must remain"); + for (index, card) in cards.iter().enumerate() { + let children = card.children().expect("populated card subtree"); + assert_eq!(children.len(), 2, "card {} lost content", index + 1); + assert!(matches!( + children.first(), + Some(jian_ops_schema::node::PenNode::Image(image)) if !image.src.as_str().is_empty() + )); + assert!(children + .get(1) + .and_then(jian_ops_schema::node::PenNode::children) + .is_some_and(|details| details.len() == 2)); + } +} + #[test] fn best_effort_policy_keeps_ts_survivor_semantics_for_internal_callers() { // The orchestrator's script-gen path (`program_gen.rs`) opts back @@ -479,40 +588,8 @@ D("ghost")"## ); } -#[test] -fn image_op_requires_ts_quoted_syntax_and_resolves_binding_parents() { - // Best-effort policy: the deliberately-bad third line must be - // dropped (not roll back the batch) so the G() parse + binding - // resolution of the good lines stays observable. - let mut state = sample(); - let program = r##"wrap=I(null, {"type":"frame","name":"Wrap","x":500,"y":0,"width":400,"height":300,"children":[{"type":"text","name":"caption","content":"x","width":10,"height":10}]}) -img=G("wrap", "search", "sunset photo") -G(wrap, "search", "bad syntax")"##; - let (envelope, cmd) = call_operations_best_effort(&state, program); - let errors = envelope["errors"].as_array().expect("errors"); - assert_eq!(errors.len(), 1, "{envelope}"); - assert!( - errors[0]["error"] - .as_str() - .unwrap() - .starts_with("Invalid G() syntax:"), - "{envelope}" - ); - let wrap_id = binding_id(&envelope, "wrap"); - let img_id = binding_id(&envelope, "img"); - - assert!(state.apply(cmd.expect("command"))); - let wrap = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(&wrap_id)) - .expect("wrap"); - let img = wrap - .children() - .expect("children") - .iter() - .find(|c| c.id_str() == img_id) - .expect("image under wrap"); - assert!(matches!(img, jian_ops_schema::node::PenNode::Image(_))); - assert_eq!(img.base().name.as_deref(), Some("sunset photo")); -} +#[path = "batch_program_image_tests.rs"] +mod image_tests; #[test] fn post_process_flag_marks_the_envelope() { diff --git a/crates/op-mcp/src/design_prompt.rs b/crates/op-mcp/src/design_prompt.rs index 349b716c8..4bb6c02c0 100644 --- a/crates/op-mcp/src/design_prompt.rs +++ b/crates/op-mcp/src/design_prompt.rs @@ -72,7 +72,7 @@ const AESTHETIC_QUALITY_GUIDE: &str = r#"AESTHETIC QUALITY BAR: const RUST_ELEMENT_TOOL_GUIDE: &str = r##"RUST MCP ELEMENT TOOL COMPATIBILITY: - This Rust MCP server does not expose the TS `add_*_v1` element-tool family unless those exact tools appear in tools/list. Do not call `add_*` tools just because older prompt text or examples mention them. -- For custom UI trees, use `batch_design` with the TS operations DSL in the `operations` argument. Supported writes: `binding=I(parent, nodeJson)` for inserts, plus one-at-a-time `U(nodeId, patchJson)`, `D(nodeId)`, `M(nodeId, parent, index?)`, `binding=C(sourceId, parent, overrides?)`, `binding=R(nodeId, nodeJson)`, and `binding=G(parent, "search"|"generate", prompt)` refine operations. +- For custom UI trees, use `batch_design` with the TS operations DSL in the `operations` argument. Supported writes: `binding=I(parent, nodeJson)` for inserts, plus one-at-a-time `U(nodeId, patchJson)`, `D(nodeId)`, `M(nodeId, parent, index?)`, `binding=C(sourceId, parent, overrides?)`, `binding=R(nodeId, nodeJson)`, and `binding=G(slotIdOrBinding, "search"|"generate", prompt[, "append"])` refine operations. The default 3-argument G is strict slot-fill: its target must exist and have zero children. The explicit fourth argument `"append"` is accepted only on a parent that declares layout `"horizontal"` or `"vertical"`; it never overlays a layout-none/omitted parent. Size the appended binding with `U()` in the same batch. - Parent can be `null` for the active page root, a previous binding name, or one real existing parent id. The node JSON is canonical PenNode JSON; omit `id` if you do not care, because Rust remaps inserted ids. - `U` currently patches geometry/name/fill fields; use dedicated `set_node_*` tools for text, rotation, stroke, font, effects, and other specialized fields. diff --git a/crates/op-mcp/src/design_prompt_tests.rs b/crates/op-mcp/src/design_prompt_tests.rs index bbccc08a2..4638d3202 100644 --- a/crates/op-mcp/src/design_prompt_tests.rs +++ b/crates/op-mcp/src/design_prompt_tests.rs @@ -72,7 +72,11 @@ fn get_design_prompt_elements_section_points_to_batch_design_operations() { assert!(prompt.contains("U(nodeId, patchJson)")); assert!(prompt.contains("C(sourceId, parent")); assert!(prompt.contains("R(nodeId, nodeJson)")); - assert!(prompt.contains("G(parent")); + assert!(prompt.contains("G(slotIdOrBinding")); + assert!(prompt.contains("target must exist and have zero children")); + assert!(prompt.contains("accepted only on a parent that declares layout")); + assert!(prompt.contains("never overlays")); + assert!(prompt.contains("Size the appended binding")); assert!(!prompt.contains("add_card_row_v1")); } other => panic!("expected prompt ok, got {other:?}"), diff --git a/crates/op-mcp/src/read_tools.rs b/crates/op-mcp/src/read_tools.rs index 36a16d335..1824665ef 100644 --- a/crates/op-mcp/src/read_tools.rs +++ b/crates/op-mcp/src/read_tools.rs @@ -8,7 +8,6 @@ use std::collections::BTreeMap; use jian_ops_schema::node::PenNode; use jian_scene::layout_scene::{NodeKind, SceneNode}; -use op_editor_core::geometry::aggregate_bounds; use op_editor_core::pen_node_ext::PenNodeExt; use op_editor_core::walkers::find_node; use op_editor_core::{EditorState, NodeId}; @@ -335,16 +334,32 @@ fn page_layout_snapshots(state: &EditorState) -> (Vec, String) { } fn page_space_snapshots(state: &EditorState) -> (Vec, String) { + let scene = op_pen_loader::editor_state_to_layout_scene(state); match state.doc.pages.as_ref() { Some(pages) if !pages.is_empty() => { let active = state.ui.active_page_index.min(pages.len() - 1); let out = pages .iter() - .map(|page| page_space(&page.id, &page.children)) + .enumerate() + .map(|(idx, page)| { + let roots = scene + .pages + .get(idx) + .map(|scene_page| scene_page.children.as_slice()) + .unwrap_or_default(); + page_space(&page.id, roots) + }) .collect(); (out, pages[active].id.clone()) } - _ => (vec![page_space("0", &state.doc.children)], "0".into()), + _ => { + let roots = scene + .pages + .first() + .map(|page| page.children.as_slice()) + .unwrap_or_default(); + (vec![page_space("0", roots)], "0".into()) + } } } @@ -506,13 +521,11 @@ fn layout_records_in(roots: &[PenNode], all_nodes: &[PenNode]) -> Vec PageSpace { - fn walk(nodes: &[PenNode], out: &mut Vec) { +fn page_space(id: &str, roots: &[SceneNode]) -> PageSpace { + fn walk(nodes: &[SceneNode], out: &mut Vec) { for n in nodes { out.push(bounds_record(n)); - if let Some(children) = n.children() { - walk(children, out); - } + walk(&n.children, out); } } let root_bounds = roots.iter().map(bounds_record).collect(); @@ -525,14 +538,14 @@ fn page_space(id: &str, roots: &[PenNode]) -> PageSpace { } } -fn bounds_record(node: &PenNode) -> BoundsRecord { - let b = aggregate_bounds(node); +fn bounds_record(node: &SceneNode) -> BoundsRecord { + let b = node.aggregate_bounds(); BoundsRecord { - id: node.id_str().to_string(), - x: b.x as i32, - y: b.y as i32, - w: b.w as i32, - h: b.h as i32, + id: node.id.clone(), + x: b.origin.x as i32, + y: b.origin.y as i32, + w: b.size.x as i32, + h: b.size.y as i32, } } @@ -600,3 +613,60 @@ fn optional_i32_arg( None => Ok(default), } } + +#[cfg(test)] +mod tests { + use super::*; + + fn bottom_args(page_id: Option<&str>) -> BTreeMap { + let mut args = BTreeMap::from([ + ("width".into(), "50".into()), + ("height".into(), "50".into()), + ("padding".into(), "10".into()), + ("direction".into(), "bottom".into()), + ]); + if let Some(id) = page_id { + args.insert("pageId".into(), id.into()); + } + args + } + + #[test] + fn empty_space_uses_resolved_omitted_height_and_requested_page() { + let src = r#"{ + "version":"1.0.0","pages":[ + {"id":"hug-page","name":"Hug","children":[ + {"type":"frame","id":"hug","x":10,"y":20,"width":390, + "layout":"vertical","gap":5,"padding":[10,20],"children":[ + {"type":"rectangle","id":"a","width":"fill_container","height":40}, + {"type":"rectangle","id":"b","width":"fill_container","height":50} + ]}, + {"type":"frame","id":"fixed","x":500,"y":30,"width":200,"height":20} + ]}, + {"id":"other-page","name":"Other","children":[ + {"type":"frame","id":"other","x":900,"y":100,"width":20,"height":30} + ]} + ],"children":[] + }"#; + let parsed = jian_ops_schema::load_str(src).expect("parse fixture"); + let mut state = EditorState::from_document(parsed.value); + state.ui.active_page_index = 1; + let tool = find_empty_space_snapshot(&state); + + let ToolOutcome::Ok(active) = tool.call(&bottom_args(None)) else { + panic!("active-page lookup should succeed"); + }; + assert_eq!(active.get("x").map(String::as_str), Some("900")); + assert_eq!(active.get("y").map(String::as_str), Some("140")); + + let ToolOutcome::Ok(hug) = tool.call(&bottom_args(Some("hug-page"))) else { + panic!("requested-page lookup should succeed"); + }; + assert_eq!(hug.get("x").map(String::as_str), Some("10")); + assert_eq!( + hug.get("y").map(String::as_str), + Some("145"), + "bottom placement must include the resolved 115px Hug root" + ); + } +} diff --git a/crates/op-mcp/src/read_tools_extra.rs b/crates/op-mcp/src/read_tools_extra.rs index 4dc1f275b..b51846c3c 100644 --- a/crates/op-mcp/src/read_tools_extra.rs +++ b/crates/op-mcp/src/read_tools_extra.rs @@ -8,7 +8,6 @@ use std::collections::BTreeMap; use jian_ops_schema::node::PenNode; -use op_editor_core::geometry::aggregate_bounds; use op_editor_core::pen_node_ext::PenNodeExt; use op_editor_core::EditorState; @@ -48,34 +47,16 @@ impl McpTool for GetCanvasBounds { } pub fn get_canvas_bounds_snapshot(state: &EditorState) -> GetCanvasBounds { - let children = active_children(state); - if children.is_empty() { - return GetCanvasBounds { bounds: None }; - } - let mut min_x = f64::INFINITY; - let mut min_y = f64::INFINITY; - let mut max_x = f64::NEG_INFINITY; - let mut max_y = f64::NEG_INFINITY; - for n in children { - let b = aggregate_bounds(n); - if b.is_empty() { - continue; - } - min_x = min_x.min(b.x); - min_y = min_y.min(b.y); - max_x = max_x.max(b.x + b.w); - max_y = max_y.max(b.y + b.h); - } - if !min_x.is_finite() { - return GetCanvasBounds { bounds: None }; - } + let scene = op_pen_loader::editor_state_to_layout_scene(state); GetCanvasBounds { - bounds: Some(( - min_x as i32, - min_y as i32, - (max_x - min_x) as i32, - (max_y - min_y) as i32, - )), + bounds: scene.content_bounds().map(|bounds| { + ( + bounds.origin.x as i32, + bounds.origin.y as i32, + bounds.size.x as i32, + bounds.size.y as i32, + ) + }), } } @@ -346,3 +327,42 @@ pub fn get_selection_set_snapshot(state: &EditorState) -> GetSelectionSet { .collect(), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn canvas_bounds_use_resolved_hug_root_and_active_page() { + let src = r#"{ + "version":"1.0.0","pages":[ + {"id":"hug-page","name":"Hug","children":[ + {"type":"frame","id":"hug","x":10,"y":20,"width":390, + "height":"fit_content","layout":"vertical","gap":5,"padding":[10,20], + "children":[ + {"type":"rectangle","id":"a","width":"fill_container","height":40}, + {"type":"rectangle","id":"b","width":"fill_container","height":50} + ]}, + {"type":"frame","id":"fixed","x":500,"y":30,"width":200,"height":20} + ]}, + {"id":"other-page","name":"Other","children":[ + {"type":"frame","id":"other","x":900,"y":100,"width":20,"height":30} + ]} + ],"children":[] + }"#; + let parsed = jian_ops_schema::load_str(src).expect("parse fixture"); + let mut state = EditorState::from_document(parsed.value); + + assert_eq!( + get_canvas_bounds_snapshot(&state).bounds, + Some((10, 20, 690, 115)), + "multi-root union must include the layout-resolved Hug height" + ); + state.ui.active_page_index = 1; + assert_eq!( + get_canvas_bounds_snapshot(&state).bounds, + Some((900, 100, 20, 30)), + "fixed-size bounds and active-page selection stay unchanged" + ); + } +} diff --git a/crates/op-mcp/src/script_runner.rs b/crates/op-mcp/src/script_runner.rs index 0c6f87514..de65e4284 100644 --- a/crates/op-mcp/src/script_runner.rs +++ b/crates/op-mcp/src/script_runner.rs @@ -32,14 +32,17 @@ globalThis.I = function (parent, obj) { globalThis.K = function (kitComponentId, parent, overrides) { return __recordK(JSON.stringify(String(kitComponentId)), parent == null ? "null" : String(parent), JSON.stringify(overrides == null ? {} : overrides)); }; -var __stubSeq = 0; -function __opStub() { __stubSeq += 1; return "stub" + __stubSeq; } -globalThis.C = __opStub; -globalThis.U = __opStub; -globalThis.D = __opStub; -globalThis.M = __opStub; -globalThis.R = __opStub; -globalThis.G = __opStub; +function __unsupported(op) { + return function () { + throw new Error("OP_SCRIPT_MODE_UNSUPPORTED: " + op + "() has no effect in script mode; use batch_design operations mode"); + }; +} +globalThis.C = __unsupported("C"); +globalThis.U = __unsupported("U"); +globalThis.D = __unsupported("D"); +globalThis.M = __unsupported("M"); +globalThis.R = __unsupported("R"); +globalThis.G = __unsupported("G"); var __noop = function () {}; globalThis.console = { log: __noop, warn: __noop, error: __noop, info: __noop, debug: __noop }; "#; @@ -191,6 +194,7 @@ fn eval_to_program(script: &str) -> Result { } match outcome { Ok(()) => Ok(program), + Err(e) if e.contains("OP_SCRIPT_MODE_UNSUPPORTED") => Err(e), Err(e) if program.trim().is_empty() => Err(e), Err(e) => { tracing::warn!( diff --git a/crates/op-mcp/src/script_runner_tests.rs b/crates/op-mcp/src/script_runner_tests.rs index ce52318d1..56d2ed5df 100644 --- a/crates/op-mcp/src/script_runner_tests.rs +++ b/crates/op-mcp/src/script_runner_tests.rs @@ -221,38 +221,26 @@ fn unrepairable_garbage_still_errors() { } #[test] -fn pencil_ops_and_console_are_noop_stubs() { - // The PRELUDE advertises C/U/D/M/R/G plus console.* as no-op stubs so a - // script generated against the batch_design DSL vocabulary (and any - // stray console logging) never aborts the sandbox — only I(...) may - // cause an effect. Calling every stub plus a couple of real inserts must - // still return Ok with exactly the I() lines recorded. +fn unsupported_mutations_are_rejected_instead_of_silently_dropped() { + let error = run_script_to_program( + r#"const root = I(null, {type: "frame", name: "Root"}); +U(root, {x: 10});"#, + ) + .expect_err("U() must not look successful while doing nothing"); + assert!(error.contains("OP_SCRIPT_MODE_UNSUPPORTED"), "{error}"); + assert!(error.contains("operations mode"), "{error}"); +} + +#[test] +fn console_remains_a_noop_while_insert_records() { let program = run_script_to_program( r#"console.log("building card"); console.warn("heads up"); -console.error("nope"); -console.info("fyi"); -console.debug("trace"); -G("root", "search", "prompt"); -C(null, {type: "frame", name: "Copy"}); -U("n1", {x: 10}); -D("n2"); -M("n3", null); -R("n4", "n5"); -I(null, {type: "frame", name: "Root"}); -I(null, {type: "text", content: "Hi"});"#, +I(null, {type: "frame", name: "Root"});"#, ) - .expect("pencil-op + console stubs must not abort the script"); - let lines: Vec<&str> = program.lines().collect(); - assert_eq!( - lines.len(), - 2, - "only the two I() calls record a line; stub calls are no-ops" - ); - assert!(lines[0].starts_with("b0=I(null, ")); - assert!(lines[0].contains(r#""name":"Root""#)); - assert!(lines[1].starts_with("b1=I(null, ")); - assert!(lines[1].contains(r#""content":"Hi""#)); + .expect("console does not affect the design program"); + assert_eq!(program.lines().count(), 1); + assert!(program.contains(r#""name":"Root""#)); } #[test]