fix(mcp): tolerate weak-model batch_design field dialects

Map Figma/flex auto-layout field names (layoutMode/direction/itemSpacing/
strokeWeight and nested {type,gap,padding} / {Horizontal:{…}} layout objects)
onto the flat schema, recover image src from url/source aliases, normalize
textGrowth/sizing keywords, and quote bareword DSL values — so a single
unfamiliar spelling no longer drops the whole node or op.
This commit is contained in:
Fini 2026-07-02 21:21:42 +08:00
parent 9e9cdec7cf
commit 6eb3527ebb
3 changed files with 398 additions and 2 deletions

View file

@ -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<String, serde_json::Value>) {
// 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<String, serde_json::Value>) {
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<String, serde_json::Value>, 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<String, serde_json::Value>) {
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<String, serde_json::Value>) {
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<String, serde_json::Value>, 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();

View file

@ -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());

View file

@ -546,6 +546,17 @@ fn parse_json_arg(raw: &str) -> Result<Value, String> {
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();