diff --git a/crates/op-mcp/src/batch_design_normalize.rs b/crates/op-mcp/src/batch_design_normalize.rs index 18719088f..eed3d10ea 100644 --- a/crates/op-mcp/src/batch_design_normalize.rs +++ b/crates/op-mcp/src/batch_design_normalize.rs @@ -47,6 +47,10 @@ pub(crate) fn normalize_node_shape(value: &mut serde_json::Value) { if let Some(padding) = obj.get_mut("padding") { normalize_padding(padding); } + // `effects` bodies carry REQUIRED fields (`spread` and friends). A model + // that omits one fails the whole node, and in the program DSL that + // cascades through every descendant line — see `effect_normalize`. + crate::effect_normalize::normalize_node_effects(obj); super::node_shape_defaults::normalize_text_default_bounds(obj); normalize_layout_keyword(obj, "justifyContent"); normalize_layout_keyword(obj, "alignItems"); diff --git a/crates/op-mcp/src/batch_program_repair_tests.rs b/crates/op-mcp/src/batch_program_repair_tests.rs index c27980391..9292b3cd3 100644 --- a/crates/op-mcp/src/batch_program_repair_tests.rs +++ b/crates/op-mcp/src/batch_program_repair_tests.rs @@ -238,3 +238,40 @@ fn children_after_a_redraft_attach_to_the_new_draft() { "utility must nest under the final draft" ); } + +#[test] +fn shadow_missing_spread_does_not_cascade_into_the_whole_card() { + // 2026-07-28 production log (desktop built-in agent, program-gen): a + // "Challenge Card" frame carried a shadow written without `spread`, serde + // rejected the node payload, `b9` never got a binding, and all 19 + // following `I(b9, …)` lines died with "Insert parent not found" — the + // entire card vanished from the design without a visible error. + let mut state = sample(); + let program = concat!( + "b9=I(\"n10\", {\"type\":\"frame\",\"name\":\"Challenge Card\",\"width\":320,\"height\":200,\"layout\":\"vertical\",", + "\"effects\":[{\"type\":\"shadow\",\"offsetX\":0,\"offsetY\":4,\"blur\":12,\"color\":\"#00000014\"}]})\n", + "b10=I(b9, {\"type\":\"text\",\"content\":\"Daily Challenge\",\"fontSize\":18})\n", + "b11=I(b9, {\"type\":\"text\",\"content\":\"3 of 5 complete\",\"fontSize\":14})" + ); + let (envelope, cmd) = call_operations(&state, program); + assert!(envelope.get("errors").is_none(), "{envelope}"); + let card_id = binding_id(&envelope, "b9"); + binding_id(&envelope, "b10"); + binding_id(&envelope, "b11"); + assert!(state.apply(cmd.expect("command"))); + let card = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(&card_id)) + .expect("the card itself must land"); + assert_eq!( + card.children().map(|c| c.len()).unwrap_or(0), + 2, + "both descendant lines must attach to the recovered card" + ); + // The authored shadow keeps its full semantics — the repair fills the + // missing field, it does not drop the effect. + let shadow = &serde_json::to_value(card).expect("card json")["effects"][0]; + assert_eq!(shadow["type"], "shadow"); + assert_eq!(shadow["offsetY"], 4.0); + assert_eq!(shadow["blur"], 12.0); + assert_eq!(shadow["spread"], 0.0); + assert_eq!(shadow["color"], "#00000014"); +} diff --git a/crates/op-mcp/src/effect_normalize.rs b/crates/op-mcp/src/effect_normalize.rs new file mode 100644 index 000000000..841fec84d --- /dev/null +++ b/crates/op-mcp/src/effect_normalize.rs @@ -0,0 +1,263 @@ +//! Lenient normalization for a node's `effects` array. +//! +//! `PenEffect` is an internally-tagged enum whose bodies carry REQUIRED +//! fields — `ShadowBody` needs `offsetX` / `offsetY` / `blur` / `spread` / +//! `color`, `BlurBody` needs `radius` — so a model that writes a shadow and +//! forgets one of them fails the WHOLE node, not just its decoration. In the +//! `I(parent, node)` program DSL that failure CASCADES: the rejected node +//! never gets a binding, so every subsequent line targeting it as a parent +//! dies with "Insert parent not found", and an entire card silently +//! disappears from the design. (Measured 2026-07-28: one missing `spread` on +//! a "Challenge Card" frame took the card plus 19 descendant lines with it.) +//! +//! Every default injected here is the identity value for the property — the +//! same value CSS uses when the author omits it, so filling it in cannot +//! change how a well-formed effect renders. `color` is the one property with +//! no identity, and its default is the neutral 25%-black every skill example +//! already uses. + +use serde_json::{Map, Value}; + +/// Neutral shadow tint used when a model omits `color`. Matches the value the +/// skill corpus teaches, so a recovered shadow looks like an authored one. +const DEFAULT_SHADOW_COLOR: &str = "#00000040"; + +/// Which `PenEffect` variant an authored effect object means. +#[derive(Clone, Copy)] +enum EffectKind { + /// `inner` carries the inner-shadow / inner-glow spelling forward. + Shadow { + inner: bool, + }, + Blur, + BackgroundBlur, +} + +/// Normalize the `effects` field of a node object in place. +/// +/// Also accepts the singular `effect` spelling: the schema field is `effects`, +/// and serde drops the singular key without a word, so a model writing +/// `effect` loses its shadow silently rather than loudly. +pub fn normalize_node_effects(obj: &mut Map) { + if !obj.contains_key("effects") { + if let Some(value) = obj.remove("effect") { + obj.insert("effects".into(), value); + } + } + if let Some(effects) = obj.get_mut("effects") { + normalize_effects(effects); + } +} + +/// Normalize an `effects` value: accept a single effect object (or a bare +/// kind name) where the schema wants an array, then repair each entry. +pub fn normalize_effects(value: &mut Value) { + match value { + Value::Object(_) | Value::String(_) => { + let single = std::mem::take(value); + *value = Value::Array(vec![single]); + } + Value::Array(_) => {} + _ => return, + } + let Value::Array(items) = value else { + return; + }; + for item in items { + normalize_effect(item); + } +} + +/// Repair one effect entry: canonicalize its `type`, then fill the required +/// body fields its variant needs. +fn normalize_effect(value: &mut Value) { + // A bare kind name (`"effects": ["shadow"]`) becomes an empty object of + // that kind; the body pass below fills every required field. + if let Value::String(name) = value { + let name = name.clone(); + *value = serde_json::json!({ "type": name }); + } + let Value::Object(obj) = value else { + return; + }; + // An unrecognized kind is left exactly as authored — guessing a variant + // for it would invent a look the model never asked for, and the node + // still fails loudly rather than rendering something wrong. + let Some(kind) = canonical_kind(obj) else { + return; + }; + let tag = match kind { + EffectKind::Shadow { .. } => "shadow", + EffectKind::Blur => "blur", + EffectKind::BackgroundBlur => "background_blur", + }; + obj.insert("type".into(), Value::String(tag.into())); + match kind { + EffectKind::Shadow { inner } => { + if inner { + obj.insert("inner".into(), Value::Bool(true)); + } + normalize_shadow_body(obj); + } + EffectKind::Blur | EffectKind::BackgroundBlur => normalize_blur_body(obj), + } +} + +/// Resolve the effect's variant from its `type`, inferring one from the body +/// shape when `type` is missing entirely. +fn canonical_kind(obj: &Map) -> Option { + let Some(raw) = obj.get("type").and_then(Value::as_str) else { + return infer_kind(obj); + }; + let canon: String = raw + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .map(|c| c.to_ascii_lowercase()) + .collect(); + // Background/backdrop blur must be tested before the plain `blur` + // substring, which it also contains. + if canon.contains("background") || canon.contains("backdrop") { + return Some(EffectKind::BackgroundBlur); + } + // A glow IS a shadow — zero offset, wide blur, tinted colour — and the + // dark style guides describe their accents in exactly those words, so + // models reach for the name. Routing it to `shadow` keeps the intent. + if canon.contains("shadow") || canon.contains("glow") { + return Some(EffectKind::Shadow { + inner: canon.contains("inner"), + }); + } + if canon.contains("blur") { + return Some(EffectKind::Blur); + } + None +} + +/// Infer the variant from which fields the object carries. Only fires when +/// `type` is absent — a present-but-unknown `type` is never overridden. +fn infer_kind(obj: &Map) -> Option { + const SHADOW_ONLY: [&str; 9] = [ + "offsetX", + "offsetY", + "offset_x", + "offset_y", + "spread", + "spreadRadius", + "dx", + "dy", + "color", + ]; + if SHADOW_ONLY.iter().any(|key| obj.contains_key(*key)) { + return Some(EffectKind::Shadow { inner: false }); + } + if obj.contains_key("radius") || obj.contains_key("blur") { + return Some(EffectKind::Blur); + } + None +} + +/// Fill `ShadowBody`'s five required fields, accepting the aliases models +/// borrow from CSS and Figma. +fn normalize_shadow_body(obj: &mut Map) { + ensure_number(obj, "offsetX", &["offset_x", "offsetx", "x", "dx"], 0.0); + ensure_number(obj, "offsetY", &["offset_y", "offsety", "y", "dy"], 0.0); + ensure_number(obj, "blur", &["blurRadius", "blur_radius", "radius"], 0.0); + ensure_number(obj, "spread", &["spreadRadius", "spread_radius"], 0.0); + let mut color = None; + for alias in ["color", "colour", "shadowColor", "shadow_color", "fill"] { + let candidate = obj.remove(alias); + if color.is_none() { + color = candidate.as_ref().and_then(extract_color); + } + } + obj.insert( + "color".into(), + Value::String(color.unwrap_or_else(|| DEFAULT_SHADOW_COLOR.to_string())), + ); + normalize_flag(obj, "inner"); + normalize_flag(obj, "visible"); +} + +/// Fill `BlurBody`'s required `radius`. +fn normalize_blur_body(obj: &mut Map) { + ensure_number( + obj, + "radius", + &["blur", "blurRadius", "amount", "size"], + 0.0, + ); + normalize_flag(obj, "visible"); +} + +/// Write `key` as a JSON number, taking the first alias present and coercing +/// a numeric string (`"8"` / `"8px"`) on the way. Every alias is consumed so +/// none survives as a stray key. +fn ensure_number(obj: &mut Map, key: &str, aliases: &[&str], default: f64) { + let mut found = obj.remove(key).as_ref().and_then(as_number); + for alias in aliases { + let candidate = obj.remove(*alias); + if found.is_none() { + found = candidate.as_ref().and_then(as_number); + } + } + let value = found.unwrap_or(default); + let number = if value.fract() == 0.0 && value.is_finite() { + serde_json::json!(value as i64) + } else { + serde_json::json!(value) + }; + obj.insert(key.into(), number); +} + +/// Coerce an optional boolean field. A string `"true"` or a 0/1 number fails +/// `Option` as hard as a missing required field does; an +/// uninterpretable value is dropped so the schema default applies. +fn normalize_flag(obj: &mut Map, key: &str) { + let Some(value) = obj.get(key) else { + return; + }; + let coerced = match value { + Value::Bool(_) => return, + Value::String(text) => match text.trim().to_ascii_lowercase().as_str() { + "true" | "yes" | "1" => Some(true), + "false" | "no" | "0" => Some(false), + _ => None, + }, + Value::Number(number) => number.as_f64().map(|n| n != 0.0), + _ => None, + }; + match coerced { + Some(flag) => { + obj.insert(key.into(), Value::Bool(flag)); + } + None => { + obj.remove(key); + } + } +} + +fn as_number(value: &Value) -> Option { + match value { + Value::Number(number) => number.as_f64(), + Value::String(text) => { + let trimmed = text.trim().trim_end_matches("px").trim(); + trimmed.parse::().ok() + } + _ => None, + } +} + +/// Pull a colour string out of the shapes a model writes it in: a bare +/// string, a fill array, or a `{type,color}` fill object. +fn extract_color(value: &Value) -> Option { + match value { + Value::String(text) if !text.trim().is_empty() => Some(text.trim().to_string()), + Value::Array(items) => items.iter().find_map(extract_color), + Value::Object(map) => map.get("color").and_then(extract_color), + _ => None, + } +} + +#[cfg(test)] +#[path = "effect_normalize_tests.rs"] +mod tests; diff --git a/crates/op-mcp/src/effect_normalize_tests.rs b/crates/op-mcp/src/effect_normalize_tests.rs new file mode 100644 index 000000000..a79a2e52b --- /dev/null +++ b/crates/op-mcp/src/effect_normalize_tests.rs @@ -0,0 +1,214 @@ +//! Unit tests for the `effects` normalizer. The end-to-end cascade +//! regression (a shadow missing `spread` taking its whole subtree down) lives +//! in `batch_program_repair_tests.rs`, where the program executor is driven. + +use super::*; + +fn normalized(node: serde_json::Value) -> serde_json::Value { + let mut value = node; + let obj = value.as_object_mut().expect("object"); + normalize_node_effects(obj); + value +} + +fn effect(node: &serde_json::Value) -> &serde_json::Value { + &node["effects"][0] +} + +/// Deserialize the way the real parse path does — ids are stamped first, so +/// these assertions test the effect repair and not the id plumbing. +fn parse_node( + node: serde_json::Value, +) -> Result { + let mut value = node; + let mut next = 1usize; + crate::batch_design::ensure_node_ids(&mut value, &mut next); + serde_json::from_value(value) +} + +#[test] +fn shadow_missing_spread_gains_the_css_identity_default() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ "type": "shadow", "offsetX": 0, "offsetY": 4, "blur": 12, "color": "#00000014" }], + })); + let shadow = effect(&out); + assert_eq!(shadow["spread"], serde_json::json!(0)); + // The authored values survive untouched — the repair only fills the gap. + assert_eq!(shadow["offsetY"], serde_json::json!(4)); + assert_eq!(shadow["blur"], serde_json::json!(12)); + assert_eq!(shadow["color"], serde_json::json!("#00000014")); + let parsed = parse_node(out).expect("deserializes"); + assert!(matches!(parsed, jian_ops_schema::node::PenNode::Frame(_))); +} + +#[test] +fn shadow_missing_every_required_field_still_deserializes() { + let out = normalized(serde_json::json!({ + "type": "rectangle", + "width": 10, + "height": 10, + "effects": [{ "type": "shadow" }], + })); + let shadow = effect(&out); + for key in ["offsetX", "offsetY", "blur", "spread"] { + assert_eq!( + shadow[key], + serde_json::json!(0), + "{key} defaults to identity" + ); + } + assert_eq!(shadow["color"], serde_json::json!(DEFAULT_SHADOW_COLOR)); + parse_node(out).expect("deserializes"); +} + +#[test] +fn shadow_aliases_and_numeric_strings_are_coerced() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ + "type": "drop-shadow", + "x": 0, "y": "8px", "blurRadius": "24", "spreadRadius": -2, + "colour": "#00000033", + }], + })); + let shadow = effect(&out); + assert_eq!(shadow["offsetX"], serde_json::json!(0)); + assert_eq!(shadow["offsetY"], serde_json::json!(8)); + assert_eq!(shadow["blur"], serde_json::json!(24)); + assert_eq!(shadow["spread"], serde_json::json!(-2)); + assert_eq!(shadow["color"], serde_json::json!("#00000033")); + // Every alias is consumed, so no stray key is left behind. + for alias in ["x", "y", "blurRadius", "spreadRadius", "colour"] { + assert!(shadow.get(alias).is_none(), "{alias} must be consumed"); + } + parse_node(out).expect("deserializes"); +} + +#[test] +fn shadow_color_is_read_out_of_a_fill_array() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ "type": "shadow", "offsetY": 2, "blur": 4, "fill": [{ "type": "solid", "color": "#112233" }] }], + })); + assert_eq!(effect(&out)["color"], serde_json::json!("#112233")); +} + +#[test] +fn a_single_effect_object_is_wrapped_into_the_schema_array() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": { "type": "shadow", "offsetY": 4, "blur": 8 }, + })); + assert!(out["effects"].is_array()); + assert_eq!(effect(&out)["spread"], serde_json::json!(0)); + parse_node(out).expect("deserializes"); +} + +#[test] +fn singular_effect_key_is_renamed_to_the_schema_field() { + // `effect` is an unknown key: serde drops it without a word, so the + // shadow disappears silently instead of failing loudly. + let out = normalized(serde_json::json!({ + "type": "frame", + "effect": [{ "type": "shadow", "offsetY": 4, "blur": 8 }], + })); + assert!(out.get("effect").is_none()); + assert_eq!(effect(&out)["type"], serde_json::json!("shadow")); +} + +#[test] +fn glow_and_inner_shadow_spellings_route_to_the_shadow_variant() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [ + { "type": "glow", "blur": 20, "color": "#A855F766" }, + { "type": "inner-shadow", "offsetY": 2, "blur": 4, "color": "#00000022" }, + ], + })); + assert_eq!(out["effects"][0]["type"], serde_json::json!("shadow")); + assert_eq!(out["effects"][1]["type"], serde_json::json!("shadow")); + assert_eq!(out["effects"][1]["inner"], serde_json::json!(true)); + parse_node(out).expect("deserializes"); +} + +#[test] +fn blur_variants_gain_their_required_radius() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [ + { "type": "blur", "amount": 10 }, + { "type": "backdrop-blur" }, + ], + })); + assert_eq!(out["effects"][0]["radius"], serde_json::json!(10)); + assert_eq!( + out["effects"][1]["type"], + serde_json::json!("background_blur") + ); + assert_eq!(out["effects"][1]["radius"], serde_json::json!(0)); + parse_node(out).expect("deserializes"); +} + +#[test] +fn a_typeless_effect_body_is_classified_by_its_fields() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ "offsetY": 4, "blur": 8, "color": "#00000020" }], + })); + assert_eq!(effect(&out)["type"], serde_json::json!("shadow")); + parse_node(out).expect("deserializes"); +} + +#[test] +fn string_flags_are_coerced_and_junk_flags_are_dropped() { + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ "type": "shadow", "offsetY": 4, "blur": 8, "visible": "true", "inner": "maybe" }], + })); + assert_eq!(effect(&out)["visible"], serde_json::json!(true)); + assert!(effect(&out).get("inner").is_none()); + parse_node(out).expect("deserializes"); +} + +#[test] +fn an_unknown_effect_kind_is_left_exactly_as_authored() { + // The fallback is deliberate: inventing a variant for a name we don't + // recognise would paint something the model never asked for. The node + // still fails loudly, which is the documented tail behaviour. + let out = normalized(serde_json::json!({ + "type": "frame", + "effects": [{ "type": "noise", "intensity": 3 }], + })); + assert_eq!(effect(&out)["type"], serde_json::json!("noise")); + assert_eq!(effect(&out)["intensity"], serde_json::json!(3)); + parse_node(out).expect_err("a genuinely unknown effect still fails"); +} + +#[test] +fn a_well_formed_effect_is_byte_for_byte_unchanged() { + let authored = serde_json::json!({ + "type": "frame", + "effects": [{ "type": "shadow", "offsetX": 0, "offsetY": 4, "blur": 12, "spread": 0, "color": "#00000014" }], + }); + assert_eq!(normalized(authored.clone()), authored); +} + +#[test] +fn nested_children_effects_are_normalized_too() { + // The node-shape pass recurses into `children`; this pins that the + // effects repair rides along with it. + let mut value = serde_json::json!({ + "type": "frame", + "children": [{ + "type": "rectangle", "width": 4, "height": 4, + "effects": [{ "type": "shadow", "offsetY": 4, "blur": 8 }], + }], + }); + crate::batch_design::normalize_node_shape(&mut value); + assert_eq!( + value["children"][0]["effects"][0]["spread"], + serde_json::json!(0) + ); + parse_node(value).expect("deserializes"); +} diff --git a/crates/op-mcp/src/lib.rs b/crates/op-mcp/src/lib.rs index 3db75a8a7..ef39a085b 100644 --- a/crates/op-mcp/src/lib.rs +++ b/crates/op-mcp/src/lib.rs @@ -92,6 +92,7 @@ pub mod document_save; #[cfg(test)] mod document_save_tests; pub mod editor_state_tool; +pub mod effect_normalize; pub mod element_tools; pub mod extra_read_tools; #[cfg(test)] diff --git a/crates/op-orchestrator/src/parse.rs b/crates/op-orchestrator/src/parse.rs index e2c801ad3..0f9d59649 100644 --- a/crates/op-orchestrator/src/parse.rs +++ b/crates/op-orchestrator/src/parse.rs @@ -328,6 +328,11 @@ pub fn normalize_generated_node_json(value: &mut serde_json::Value) { } normalize_stroke_json(object); normalize_layout_enum_json(object); + // Shared with the program-DSL parse path: an `effects` body is + // missing-required-field fragile (`spread` above all), and one + // rejected node drops a whole card. Single-sourced in op-mcp so + // the two parse paths can't drift. + op_mcp::effect_normalize::normalize_node_effects(object); for (key, child) in object.iter_mut() { // 数值型设计 token(`$type-*-size` 等)只在**数值字段**上就地解析 diff --git a/crates/op-orchestrator/src/parse_tests.rs b/crates/op-orchestrator/src/parse_tests.rs index 6ec989e53..6d6b3409f 100644 --- a/crates/op-orchestrator/src/parse_tests.rs +++ b/crates/op-orchestrator/src/parse_tests.rs @@ -420,3 +420,28 @@ fn parse_nodes_ignores_draft_in_truncated_second_think() { // 只有 a;draft-x(在被截断的 think#2 里)不计入。 assert_eq!(crate::cleanup::count_descendants(&nodes[0]), 1); } + +#[test] +fn parse_nodes_tolerates_a_shadow_missing_spread() { + // The modify/chat flat-JSONL path shares the program-DSL病灶: `ShadowBody` + // 的 `spread` 是必填,模型漏写就整个节点报废。这里连子节点一起验证 —— + // 老逻辑下带阴影的卡片会被整张丢掉,只剩它旁边的节点。 + let text = r##"[ + {"type":"frame","id":"card","name":"Challenge Card","x":0,"y":0,"width":320,"height":200, + "effects":[{"type":"shadow","offsetX":0,"offsetY":4,"blur":12,"color":"#00000014"}]}, + {"type":"text","id":"title","content":"Daily Challenge","fontSize":18,"_parent":"card"} +]"##; + + let nodes = parse_nodes(text).expect("a shadow missing spread must not drop the card"); + assert_eq!(nodes.len(), 1, "the card is the only root"); + assert_eq!(nodes[0].id_str(), "card"); + assert_eq!( + crate::cleanup::count_descendants(&nodes[0]), + 1, + "the child must still attach to the recovered card" + ); + let effects = &serde_json::to_value(&nodes[0]).expect("card json")["effects"][0]; + assert_eq!(effects["type"], "shadow"); + assert_eq!(effects["spread"], 0.0); + assert_eq!(effects["blur"], 12.0); +}