diff --git a/crates/op-mcp/src/batch_design.rs b/crates/op-mcp/src/batch_design.rs index 272e40cd7..0cd04f3dd 100644 --- a/crates/op-mcp/src/batch_design.rs +++ b/crates/op-mcp/src/batch_design.rs @@ -522,6 +522,20 @@ pub(crate) fn normalize_node_shape(value: &mut serde_json::Value) { let serde_json::Value::Object(obj) = value else { return; }; + // Map Figma/Pencil auto-layout field names onto our schema FIRST — MiniMax-M3 + // (trained on Pencil's schema) emits `layoutMode`/`itemSpacing`/`strokeWeight`/ + // `primaryAxisAlignItems`/`counterAxisAlignItems`, which serde would silently + // drop as unknown keys, leaving the frame with no layout → it renders as an + // unstyled horizontal strip. Rename before anything else reads them. + normalize_pencil_autolayout_dialect(obj); + // Flatten a STRUCTURED `layout` object (`{type,gap,padding}` or the + // externally-tagged `{Horizontal:{…}}`) down to our flat `layout` string + + // hoisted gap/padding. glm-5.2 in the loop emits this Figma/flex shape; serde + // rejects it against the string-typed `layout` field, so the WHOLE update + // fails and the root never gets its layout (measured: glm built n1/n2/n3 + // correctly, then every `U(n1,{layout:{type:horizontal…}})` was rejected and + // the tree thrashed to empty). + normalize_layout_object(obj); if let Some(fill) = obj.get_mut("fill") { normalize_fill(fill); } @@ -545,6 +559,10 @@ pub(crate) fn normalize_node_shape(value: &mut serde_json::Value) { super::node_shape_defaults::normalize_text_default_bounds(obj); normalize_layout_keyword(obj, "justifyContent"); normalize_layout_keyword(obj, "alignItems"); + normalize_image_src(obj); + normalize_text_growth(obj); + normalize_sizing_keyword(obj, "width"); + normalize_sizing_keyword(obj, "height"); if let Some(serde_json::Value::Array(children)) = obj.get_mut("children") { for child in children { normalize_node_shape(child); @@ -552,15 +570,243 @@ pub(crate) fn normalize_node_shape(value: &mut serde_json::Value) { } } +/// Rename Figma / Pencil auto-layout keys onto OpenPencil's schema so a model +/// trained on the Pencil dialect (MiniMax-M3) keeps its layout. Each rename is +/// applied ONLY when the canonical key is absent, so a node that already uses +/// our names is untouched. Axis-align values are lifted from Figma's +/// `MIN/MAX/CENTER/SPACE_BETWEEN` enum; unknown spellings pass through for the +/// downstream `normalize_layout_keyword` pass to handle. +fn normalize_pencil_autolayout_dialect(obj: &mut serde_json::Map) { + // layoutMode / direction → layout. `direction` is the flex/CSS alias glm-5.2 + // reaches for (measured: `{…,"direction":"horizontal",…}`), `layoutMode` the + // Figma one M3 uses. First alias present wins. + if !obj.contains_key("layout") { + for alias in ["layoutMode", "direction"] { + let Some(s) = obj + .remove(alias) + .and_then(|v| v.as_str().map(str::to_string)) + else { + continue; + }; + let mapped = match s.trim().to_ascii_lowercase().as_str() { + "horizontal" | "row" => Some("horizontal"), + "vertical" | "column" => Some("vertical"), + "none" | "" => Some("none"), + _ => None, + }; + if let Some(m) = mapped { + obj.insert("layout".into(), serde_json::Value::String(m.into())); + } + break; + } + } + // itemSpacing → gap + if !obj.contains_key("gap") { + if let Some(v) = obj + .remove("itemSpacing") + .filter(serde_json::Value::is_number) + { + obj.insert("gap".into(), v); + } + } + // primaryAxisAlignItems → justifyContent ; counterAxisAlignItems → alignItems + for (from, to) in [ + ("primaryAxisAlignItems", "justifyContent"), + ("counterAxisAlignItems", "alignItems"), + ] { + if obj.contains_key(to) { + continue; + } + let Some(s) = obj + .remove(from) + .and_then(|v| v.as_str().map(str::to_string)) + else { + continue; + }; + let mapped = match s.trim().to_ascii_uppercase().as_str() { + "MIN" => "start", + "MAX" => "end", + "CENTER" => "center", + "SPACE_BETWEEN" => "space_between", + _ => s.as_str(), // leave for normalize_layout_keyword + }; + obj.insert(to.into(), serde_json::Value::String(mapped.to_string())); + } + // strokeWeight → stroke thickness (Figma names the width apart from the color) + if let Some(weight) = obj + .remove("strokeWeight") + .filter(serde_json::Value::is_number) + { + match obj.get_mut("stroke") { + Some(serde_json::Value::Object(s)) => { + s.entry("thickness").or_insert(weight); + } + Some(serde_json::Value::String(color)) => { + let color = color.clone(); + obj.insert( + "stroke".into(), + serde_json::json!({ "thickness": weight, "fill": color }), + ); + } + _ => {} + } + } +} + +/// Flatten a STRUCTURED `layout` object onto our flat schema. glm-5.2 in the +/// agentic loop writes `layout` as a Figma/flex object — +/// `{"type":"horizontal","gap":0,"padding":[…]}` or the externally-tagged +/// `{"Horizontal":{"gap":0,…}}` — but our `layout` field is a plain string +/// (`"horizontal"`), with `gap`/`padding`/`justifyContent`/`alignItems` as +/// sibling keys. serde rejects the object, failing the whole insert/update, so +/// the node's layout never lands (measured: glm built its tree with correct ids, +/// then every `U(n1,{layout:{type:horizontal…}})` was rejected). Lift the +/// direction to the `layout` string and hoist the object's spacing keys to the +/// node (only where the node doesn't already set them). Unknown directions are +/// left untouched rather than guessed. +fn normalize_layout_object(obj: &mut serde_json::Map) { + let layout = match obj.get("layout") { + Some(serde_json::Value::Object(m)) => m.clone(), + _ => return, + }; + // `{type:"horizontal",…}` (type-keyed) OR `{Horizontal:{…}}` (variant-keyed). + let (raw_dir, inner) = if let Some(t) = layout.get("type").and_then(|v| v.as_str()) { + (t.to_string(), None) + } else if let Some((k, v)) = layout.iter().next() { + (k.clone(), v.as_object().cloned()) + } else { + return; + }; + let dir = match raw_dir.trim().to_ascii_lowercase().as_str() { + "horizontal" | "row" => "horizontal", + "vertical" | "column" => "vertical", + "none" => "none", + _ => return, + }; + let source = inner.as_ref().unwrap_or(&layout); + let gap = source.get("gap").cloned(); + let padding = source.get("padding").cloned(); + let justify = source.get("justifyContent").cloned(); + let align = source.get("alignItems").cloned(); + obj.insert("layout".into(), serde_json::Value::String(dir.into())); + if let Some(g) = gap { + obj.entry("gap").or_insert(g); + } + if let Some(p) = padding { + obj.entry("padding").or_insert(p); + } + if let Some(j) = justify { + obj.entry("justifyContent").or_insert(j); + } + if let Some(a) = align { + obj.entry("alignItems").or_insert(a); + } +} + +/// `width` / `height` accept `fill_container` / `fit_content` / a number. A weak +/// model sometimes appends a type-hint suffix (`fill_container_str` — a leaked +/// variable name) or uses a CSS-ish spelling (`fill-container` / `hug` / `auto`), +/// which fails the `Sizing` enum and drops the whole node. Map the known +/// content-hug / fill spellings back to the canonical keyword; leave numbers, +/// numeric strings, and already-valid / unrecognised values untouched. +fn normalize_sizing_keyword(obj: &mut serde_json::Map, key: &str) { + let Some(serde_json::Value::String(raw)) = obj.get(key) else { + return; + }; + let mut canon = raw.trim().to_ascii_lowercase().replace([' ', '-'], "_"); + for suffix in ["_str", "_string", "_val", "_value"] { + if let Some(stripped) = canon.strip_suffix(suffix) { + canon = stripped.to_string(); + break; + } + } + let normalized = match canon.as_str() { + "fill_container" | "fillcontainer" | "fill" | "container" | "full" | "fill_width" + | "fill_parent" => Some("fill_container"), + "fit_content" | "fitcontent" | "fit" | "hug" | "hug_content" | "auto" | "content" => { + Some("fit_content") + } + _ => None, + }; + if let Some(valid) = normalized { + obj.insert(key.into(), serde_json::Value::String(valid.to_string())); + } +} + +/// A weak model sometimes emits an `image` node with NO `src` (or puts the URL +/// under an alias like `url`/`source`). `ImageNode.src` is REQUIRED, so the whole +/// node fails to deserialize and is dropped — the avatar/logo vanishes AND the +/// column it anchored collapses. Recover the src from a common alias, else inject +/// an empty placeholder (renders as a grey box) so the node — and the layout it +/// holds open — survives. (Measured: glm avatar images → 7× `missing field src`.) +fn normalize_image_src(obj: &mut serde_json::Map) { + if obj.get("type").and_then(serde_json::Value::as_str) != Some("image") { + return; + } + let has_src = obj + .get("src") + .and_then(serde_json::Value::as_str) + .is_some_and(|t| !t.trim().is_empty()); + if has_src { + return; + } + for alias in ["url", "source", "imageUrl", "image_url", "uri", "href"] { + if let Some(v) = obj.get(alias).cloned() { + if v.as_str().is_some_and(|t| !t.trim().is_empty()) { + obj.insert("src".into(), v); + return; + } + } + } + obj.insert("src".into(), serde_json::Value::String(String::new())); +} + +/// `textGrowth` only accepts `auto` / `fixed-width` / `fixed-width-height`. A +/// weak model borrows a SIZING keyword (`fit_content` / `fill_container`) for it, +/// and the invalid variant drops the whole text node. Map the content-hugging +/// forms to `auto` (same intent); drop anything else so the node still lands with +/// the default growth. (Measured: glm text nodes → 3× `unknown variant fit_content`.) +fn normalize_text_growth(obj: &mut serde_json::Map) { + let Some(raw) = obj.get("textGrowth").and_then(serde_json::Value::as_str) else { + return; + }; + let canon = raw.trim().to_ascii_lowercase().replace([' ', '_'], "-"); + let normalized = match canon.as_str() { + "auto" | "fit-content" | "hug" | "fill-container" | "fill" | "fit" => Some("auto"), + "fixed-width" => Some("fixed-width"), + "fixed-width-height" | "fixed-width-and-height" | "fixed" => Some("fixed-width-height"), + _ => None, + }; + match normalized { + Some(valid) => { + obj.insert( + "textGrowth".into(), + serde_json::Value::String(valid.to_string()), + ); + } + None => { + obj.remove("textGrowth"); + } + } +} + fn normalize_layout_keyword(obj: &mut serde_json::Map, key: &str) { let Some(serde_json::Value::String(value)) = obj.get_mut(key) else { return; }; let normalized = match (key, value.as_str()) { - ("justifyContent" | "alignItems", "flex-start") => "start", - ("justifyContent" | "alignItems", "flex-end") => "end", + // CSS flexbox value names. A model fluent in CSS (glm-5.2 etc.) writes + // `flex-start`/`flex-end` AND — by analogy to our snake_case `space_between` + // — the underscore form `flex_start`/`flex_end`. The schema only accepts + // `start`/`end`, so without this the WHOLE node fails to deserialize and is + // silently dropped (a 5-column table loses every cell whose alignment is a + // flex_* name — measured: glm dropped the right-aligned amount column + the + // left-aligned header labels, keeping only the `center` ones). + ("justifyContent" | "alignItems", "flex-start" | "flex_start" | "flexstart") => "start", + ("justifyContent" | "alignItems", "flex-end" | "flex_end" | "flexend") => "end", ("justifyContent", "space-between") => "space_between", ("justifyContent", "space-around") => "space_around", + ("justifyContent", "space-evenly" | "space_evenly") => "space_between", _ => return, }; *value = normalized.to_string(); diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index 6afc53c40..ad42ee8b5 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/crates/op-mcp/src/batch_design_tests.rs @@ -302,6 +302,145 @@ fn batch_design_normalizes_ts_layout_keywords() { assert_eq!(value["children"][0]["justifyContent"], "end"); } +#[test] +fn batch_design_normalizes_underscore_flex_keywords() { + // A CSS-fluent model writes the snake_case `flex_start`/`flex_end` (by + // analogy to our `space_between`). The schema only has `start`/`end`, so an + // un-normalized flex_* fails the WHOLE node's deserialize and it is silently + // dropped — measured: glm's right-aligned amount cells + left-aligned header + // labels vanished, leaving only the `center` ones. Both children below MUST + // survive with normalized alignment. + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r##"root=I(null, {"type":"frame","name":"Row","layout":"horizontal","children":[{"type":"frame","name":"Left","justifyContent":"flex_start"},{"type":"frame","name":"Amount","justifyContent":"flex_end","alignItems":"flex_start"}]})"## + .into(), + ); + + let ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { nodes, .. }) = + tool.call(&args) + else { + panic!("expected InsertSubtree command"); + }; + let value = serde_json::to_value(&nodes[0]).expect("node json"); + let children = value["children"].as_array().expect("children survive"); + assert_eq!(children.len(), 2, "BOTH flex_* children must survive"); + assert_eq!(children[0]["justifyContent"], "start"); + assert_eq!(children[1]["justifyContent"], "end"); + assert_eq!(children[1]["alignItems"], "start"); +} + +#[test] +fn batch_design_maps_pencil_autolayout_dialect() { + // MiniMax-M3 is trained on Pencil's schema: it emits `layoutMode` / + // `itemSpacing` / `strokeWeight` / `primaryAxisAlignItems` / + // `counterAxisAlignItems`. serde drops those unknown keys, so the frame + // loses its layout entirely and 229 nodes render as one horizontal strip + // (measured on the barbershop loop run). The dialect map MUST rename them + // onto our schema so the model's layout survives. + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r##"root=I(null, {"type":"frame","name":"Card","layoutMode":"VERTICAL","itemSpacing":16,"strokeWeight":2,"stroke":"#E7E5E4","primaryAxisAlignItems":"SPACE_BETWEEN","counterAxisAlignItems":"CENTER"})"## + .into(), + ); + + let ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { nodes, .. }) = + tool.call(&args) + else { + panic!("expected InsertSubtree command"); + }; + let value = serde_json::to_value(&nodes[0]).expect("node json"); + assert_eq!(value["layout"], "vertical", "layoutMode → layout"); + assert_eq!(value["gap"].as_f64(), Some(16.0), "itemSpacing → gap"); + assert_eq!( + value["justifyContent"], "space_between", + "primaryAxisAlignItems → justifyContent" + ); + assert_eq!( + value["alignItems"], "center", + "counterAxisAlignItems → alignItems" + ); + // strokeWeight folds into the stroke as its thickness (schema uses per-side + // thickness; a scalar lands on `.thickness`). + assert!( + value["stroke"].is_object() || value["stroke"].is_array(), + "stroke survives as a structured value, got {:?}", + value["stroke"] + ); +} + +#[test] +fn batch_design_flattens_structured_layout_object() { + // glm-5.2 in the agentic loop writes `layout` as a Figma/flex OBJECT — + // `{"type":"horizontal","gap":0,"padding":[…]}` and the externally-tagged + // `{"Vertical":{"gap":12}}`. serde rejects both against our string-typed + // `layout` field, so every `U(n1,{layout:{…}})` failed and glm's (correctly + // id-tracked!) tree never got its layout. Both shapes MUST flatten. + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r##"root=I(null, {"type":"frame","name":"Shell","layout":{"type":"horizontal","gap":24,"padding":[8,8,8,8]},"children":[{"type":"frame","name":"Col","layout":{"Vertical":{"gap":12}}}]})"## + .into(), + ); + let ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { nodes, .. }) = + tool.call(&args) + else { + panic!("expected InsertSubtree command"); + }; + let value = serde_json::to_value(&nodes[0]).expect("node json"); + assert_eq!( + value["layout"], "horizontal", + "type-keyed object → layout string" + ); + assert_eq!(value["gap"].as_f64(), Some(24.0), "hoisted gap"); + assert_eq!( + value["padding"] + .as_array() + .and_then(|a| a.first()) + .and_then(|v| v.as_f64()), + Some(8.0), + "hoisted per-side padding" + ); + assert_eq!( + value["children"][0]["layout"], "vertical", + "externally-tagged {{Vertical:{{…}}}} → layout string" + ); + assert_eq!( + value["children"][0]["gap"].as_f64(), + Some(12.0), + "variant-keyed inner gap hoisted" + ); +} + +#[test] +fn batch_design_maps_direction_alias_to_layout() { + // glm-5.2 in the loop reaches for the flex/CSS `direction` alias instead of + // our `layout` (measured: `{…,"direction":"horizontal",…}` on every frame), + // so serde drops it and 0/62 frames got a layout. Map it. + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + r##"root=I(null, {"type":"frame","name":"Row","direction":"horizontal","children":[{"type":"frame","name":"Col","direction":"column"}]})"## + .into(), + ); + let ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { nodes, .. }) = + tool.call(&args) + else { + panic!("expected InsertSubtree command"); + }; + let value = serde_json::to_value(&nodes[0]).expect("node json"); + assert_eq!(value["layout"], "horizontal", "direction → layout"); + assert_eq!( + value["children"][0]["layout"], "vertical", + "direction:column → vertical" + ); +} + #[test] fn batch_design_insert_operations_accept_outer_page_id() { let tool = batch_design_snapshot(&sample()); diff --git a/crates/op-mcp/src/batch_program.rs b/crates/op-mcp/src/batch_program.rs index 8e1f7ddbf..3af54eeca 100644 --- a/crates/op-mcp/src/batch_program.rs +++ b/crates/op-mcp/src/batch_program.rs @@ -546,6 +546,17 @@ fn parse_json_arg(raw: &str) -> Result { normalized = regex(r#":(\s*)([A-Za-z][\w-]*)""#) .replace_all(&normalized, r#":${1}"${2}""#) .into_owned(); + // Repair a FULLY-unquoted string value — `"width":fill_container_str,` meant + // `"width":"fill_container_str"` (a weak model emitted a bare identifier, e.g. + // a leaked JS variable name). A letter-led bareword between a colon and a + // `,`/`}`/`]`. Numbers are digit-led (never match); a real quoted value + // starts with `"` (never matches); `true`/`false`/`null` get re-unquoted next. + normalized = regex(r#":(\s*)([A-Za-z][\w-]*)(\s*[,}\]])"#) + .replace_all(&normalized, r#":${1}"${2}"${3}"#) + .into_owned(); + normalized = regex(r#":(\s*)"(true|false|null)"(\s*[,}\]])"#) + .replace_all(&normalized, r#":${1}${2}${3}"#) + .into_owned(); normalized = regex(r#",\s*""\s*:\s*[^,}\]]+"#) .replace_all(&normalized, "") .into_owned();