fix(mcp): default omitted effect fields instead of dropping the whole node

ShadowBody deserializes with five required fields, so a model omitting
just spread lost the entire node — and every child line that named it as
a parent cascaded into 'Insert parent not found' (a measured 19-line,
whole-card silent loss). Effects are now normalized before the strict
parse: identity defaults for offsets/blur/spread, alias folding for the
common synonyms, a lone effect object wrapped into the array, numeric
strings coerced, and glow/drop-shadow spellings canonicalized. Truly
unknown effect kinds still fail the node — inventing an effect the model
did not ask for would be worse than dropping one. The modify path's
parser reuses the same normalizer so the two lanes cannot drift.

Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x
This commit is contained in:
Fini 2026-07-28 22:12:36 +08:00
parent 3571d70a53
commit 2ca873346c
7 changed files with 549 additions and 0 deletions

View file

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

View file

@ -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");
}

View file

@ -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<String, Value>) {
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<String, Value>) -> Option<EffectKind> {
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<String, Value>) -> Option<EffectKind> {
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<String, Value>) {
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<String, Value>) {
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<String, Value>, 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<bool>` as hard as a missing required field does; an
/// uninterpretable value is dropped so the schema default applies.
fn normalize_flag(obj: &mut Map<String, Value>, 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<f64> {
match value {
Value::Number(number) => number.as_f64(),
Value::String(text) => {
let trimmed = text.trim().trim_end_matches("px").trim();
trimmed.parse::<f64>().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<String> {
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;

View file

@ -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<jian_ops_schema::node::PenNode, serde_json::Error> {
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");
}

View file

@ -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)]

View file

@ -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` 等)只在**数值字段**上就地解析

View file

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