From d70cd2bc8b005021b0784036f2dc71d9fdf77db5 Mon Sep 17 00:00:00 2001 From: Fini Date: Fri, 24 Jul 2026 21:12:05 +0800 Subject: [PATCH] feat(agent): detect cross-subtree text collisions in geometry diagnostics --- crates/op-orchestrator/src/geometry_echo.rs | 2 + .../src/geometry_validation.rs | 7 +- crates/op-orchestrator/src/text_collision.rs | 187 ++++++++++++++ .../src/text_collision_tests.rs | 240 ++++++++++++++++++ 4 files changed, 435 insertions(+), 1 deletion(-) create mode 100644 crates/op-orchestrator/src/text_collision.rs create mode 100644 crates/op-orchestrator/src/text_collision_tests.rs diff --git a/crates/op-orchestrator/src/geometry_echo.rs b/crates/op-orchestrator/src/geometry_echo.rs index a57f6db75..22230f58b 100644 --- a/crates/op-orchestrator/src/geometry_echo.rs +++ b/crates/op-orchestrator/src/geometry_echo.rs @@ -13,6 +13,7 @@ use op_editor_core::{EditorState, NodeId}; +use super::text_collision::push_text_collision_diagnostics; use super::{collect_diagnostics, resolved_rects, MAX_DIAGNOSTICS}; /// Geometry diagnostics for exactly the subtrees rooted at `root_ids` — @@ -42,6 +43,7 @@ pub(crate) fn geometry_diagnostics_for_roots( let Ok(v) = serde_json::to_value(root) else { continue; }; + push_text_collision_diagnostics(&v, &rects, &mut out); collect_diagnostics(&v, &rects, &mut out); } out.truncate(MAX_DIAGNOSTICS); diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index 6cca2b1d5..12ead954b 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -34,7 +34,11 @@ use geometry_compact_status::{ is_compact_status_badge_structure, stack_compact_status_header, }; -#[derive(Clone, Copy)] +#[path = "text_collision.rs"] +mod text_collision; +use text_collision::push_text_collision_diagnostics; + +#[derive(Clone, Copy, Debug)] struct Rect { x: f64, /// Not read yet — vertical stacking-overlap detection will need it; kept @@ -1045,6 +1049,7 @@ pub fn geometry_diagnostics(state: &EditorState) -> Vec { if let Ok(v) = serde_json::to_value(root) { bottom_nav_root_containment_diagnostic(&v, &rects, &mut out); bottom_nav_order_diagnostic(&v, &mut out); + push_text_collision_diagnostics(&v, &rects, &mut out); collect_diagnostics(&v, &rects, &mut out); } } diff --git a/crates/op-orchestrator/src/text_collision.rs b/crates/op-orchestrator/src/text_collision.rs new file mode 100644 index 000000000..d47f30f9e --- /dev/null +++ b/crates/op-orchestrator/src/text_collision.rs @@ -0,0 +1,187 @@ +//! Cross-subtree TEXT-vs-TEXT collision detector. +//! +//! `collect_sibling_jam_diagnostics` (the existing overlap/jam detector) +//! only compares DIRECT siblings inside one flex row/column, and it +//! deliberately steps over `layout:none` parents — a `layout:none` card +//! stack is usually an intentional deck where the CARDS overlap on +//! purpose (front card hides the back one). That gate is correct for +//! boxes, but it also makes the sibling-jam detector structurally blind +//! to the failure this module catches: a weak model authoring a +//! `layout:none` deck routinely lands a LEAF text node from one card +//! directly on top of a leaf text node from another card. The two text +//! nodes are cousins, not siblings, and neither one's ancestor chain sets +//! `layout:none` at the SAME level the jam detector inspects, so the jam +//! pass never sees the pair at all. +//! +//! The contract here is deliberately flatter than the box-overlap one: +//! unlike containers (where overlap is often the deliberate point — deck +//! stacking, ring badges, notification dots), two rendered TEXT blocks +//! landing on the same pixels is never design intent. So this detector +//! applies a single fact-level rule with no name/role exceptions: real +//! resolved overlap between two non-empty visible text leaves is always a +//! defect. Detect-only — which text should move is an intent judgment for +//! the model in the loop, not a fixer here. +//! +//! Measured root cause (0724-1-gm-2.op, "今日词卡" stacked word-card +//! preview): the front card's Chinese meaning resolves to y 708–728 and +//! the back card's example sentence resolves to y 705–722 (68% of the +//! smaller block's area) because both cards anchor their text near the +//! bottom of a shared `layout:none` container without either one +//! accounting for the other card's text block. + +use std::collections::HashMap; + +use serde_json::Value; + +use super::{children, Rect}; + +/// Both axes must show more than this many px of overlap before a pair +/// counts as colliding — guards against sub-pixel rounding touches +/// between adjacent (non-overlapping) flow text. +const MIN_AXIS_OVERLAP_PX: f64 = 4.0; + +/// The overlapping area must cover more than this fraction of the +/// SMALLER text block's own area — guards against a long line merely +/// clipping the corner of a short label it doesn't meaningfully collide +/// with. +const MIN_OVERLAP_AREA_RATIO: f64 = 0.25; + +/// One detected TEXT-vs-TEXT collision. Deliberately a structured payload +/// (id + name + resolved rect for each side, plus the measured overlap) +/// rather than a pre-formatted string — this is `geometry_validation`'s +/// first detector with a real category/shape, laying the ground for +/// `loop_blocker_ledger` to eventually classify it directly instead of +/// pattern-matching message text. +#[derive(Debug)] +pub(crate) struct TextCollision { + pub(crate) a_id: String, + pub(crate) a_name: String, + pub(crate) a_rect: Rect, + pub(crate) b_id: String, + pub(crate) b_name: String, + pub(crate) b_rect: Rect, + pub(crate) overlap_x: f64, + pub(crate) overlap_y: f64, + /// Overlap area as a fraction of the SMALLER of the two text blocks' + /// own areas (0.0–1.0+; values are already known > MIN_OVERLAP_AREA_RATIO). + pub(crate) overlap_area_ratio: f64, +} + +fn is_visible_nonempty_text(v: &Value) -> bool { + v.get("type").and_then(Value::as_str) == Some("text") + && v.get("visible").and_then(Value::as_bool) != Some(false) + && v.get("content") + .and_then(Value::as_str) + .is_some_and(|c| !c.trim().is_empty()) +} + +/// Depth-first collection of every visible, non-empty text LEAF under `v` +/// that has a positive-area resolved rect. Document order, so pairwise +/// comparison downstream is deterministic. +fn collect_text_leaves<'a>( + v: &'a Value, + rects: &HashMap, + out: &mut Vec<(&'a str, &'a str, Rect)>, +) { + if is_visible_nonempty_text(v) { + if let Some(id) = v.get("id").and_then(Value::as_str) { + if let Some(rect) = rects.get(id) { + if rect.w > 0.0 && rect.h > 0.0 { + let name = v.get("name").and_then(Value::as_str).unwrap_or("text"); + out.push((id, name, *rect)); + } + } + } + } + // Text is a leaf in this schema; recursing defensively costs nothing + // and matches the `bears_text` convention elsewhere in this module. + for c in children(v) { + collect_text_leaves(c, rects, out); + } +} + +/// All TEXT-vs-TEXT collisions inside `root`'s subtree (root included), +/// scoped to that one root the same way the caller scopes every other +/// per-root geometry diagnostic. +pub(crate) fn collect_text_collisions( + root: &Value, + rects: &HashMap, +) -> Vec { + let mut leaves = Vec::new(); + collect_text_leaves(root, rects, &mut leaves); + let mut out = Vec::new(); + for i in 0..leaves.len() { + let (a_id, a_name, a) = leaves[i]; + for &(b_id, b_name, b) in &leaves[i + 1..] { + let overlap_x = (a.x + a.w).min(b.x + b.w) - a.x.max(b.x); + let overlap_y = (a.y + a.h).min(b.y + b.h) - a.y.max(b.y); + if overlap_x <= MIN_AXIS_OVERLAP_PX || overlap_y <= MIN_AXIS_OVERLAP_PX { + continue; + } + let smaller_area = (a.w * a.h).min(b.w * b.h); + if smaller_area <= 0.0 { + continue; + } + let overlap_area_ratio = (overlap_x * overlap_y) / smaller_area; + if overlap_area_ratio <= MIN_OVERLAP_AREA_RATIO { + continue; + } + out.push(TextCollision { + a_id: a_id.to_string(), + a_name: a_name.to_string(), + a_rect: a, + b_id: b_id.to_string(), + b_name: b_name.to_string(), + b_rect: b, + overlap_x, + overlap_y, + overlap_area_ratio, + }); + } + } + out +} + +fn format_collision(c: &TextCollision) -> String { + format!( + "{} ({}, resolved at {}x{} @{},{}) and {} ({}, resolved at {}x{} @{},{}): TEXT leaves \ + OVERLAP by {}x{}px (~{}% of the smaller block's area) — two separate text blocks are \ + rendering on the same pixels; give each its own non-overlapping position (or \ + hide/remove one) so neither is unreadable", + c.a_name, + c.a_id, + c.a_rect.w.round(), + c.a_rect.h.round(), + c.a_rect.x.round(), + c.a_rect.y.round(), + c.b_name, + c.b_id, + c.b_rect.w.round(), + c.b_rect.h.round(), + c.b_rect.x.round(), + c.b_rect.y.round(), + c.overlap_x.round(), + c.overlap_y.round(), + (c.overlap_area_ratio * 100.0).round() + ) +} + +/// Detect + format text collisions under `root`, appending to `out` and +/// respecting the shared `MAX_DIAGNOSTICS` budget the same way every +/// other collector in this module does. +pub(crate) fn push_text_collision_diagnostics( + root: &Value, + rects: &HashMap, + out: &mut Vec, +) { + for collision in collect_text_collisions(root, rects) { + if out.len() >= super::MAX_DIAGNOSTICS { + return; + } + out.push(format_collision(&collision)); + } +} + +#[cfg(test)] +#[path = "text_collision_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/text_collision_tests.rs b/crates/op-orchestrator/src/text_collision_tests.rs new file mode 100644 index 000000000..ecc34906c --- /dev/null +++ b/crates/op-orchestrator/src/text_collision_tests.rs @@ -0,0 +1,240 @@ +use super::*; +use serde_json::json; +use std::collections::HashMap; + +fn text_node(id: &str, name: &str, content: &str) -> serde_json::Value { + json!({ "type": "text", "id": id, "name": name, "content": content }) +} + +fn rect(x: f64, y: f64, w: f64, h: f64) -> Rect { + Rect { x, y, w, h } +} + +// ── positive: reproduces the measured 0724-1-gm-2.op "今日词卡" shape ── +// +// A `layout:none` deck stacks a front card over a back card. Both anchor +// their own text near the bottom of their own card, and because the two +// cards are offset by only 12px (front on top, back peeking out below — +// the deliberate "stacked deck" look), the front card's meaning line and +// the back card's example line land on the same pixels. The two text +// nodes are cousins (different parents, both `layout:none` descendants), +// so `collect_sibling_jam_diagnostics` never sees this pair at all. + +#[test] +fn stacked_deck_cousin_text_collision_is_detected() { + let deck = json!({ + "type": "frame", "id": "deck", "name": "Stacked Card Container", "layout": "none", + "children": [ + { "type": "frame", "id": "front", "name": "Front Vocabulary Card", "children": [ + text_node("front-meaning", "Chinese Meaning", "有适应力的；能迅速恢复的") + ] }, + { "type": "frame", "id": "back", "name": "Back Example Card", "children": [ + text_node("back-example", "Example English", "She is resilient in facing hard times") + ] } + ] + }); + let mut rects = HashMap::new(); + // Mirrors the measured resolved geometry: both text blocks land in the + // same 300-ish-wide column, ~14-20px tall, offset by only a few px. + rects.insert("front-meaning".to_string(), rect(118.0, 708.0, 244.0, 20.0)); + rects.insert("back-example".to_string(), rect(126.0, 705.0, 283.0, 17.0)); + + let collisions = collect_text_collisions(&deck, &rects); + assert_eq!( + collisions.len(), + 1, + "exactly one colliding pair: {collisions:?}" + ); + let c = &collisions[0]; + assert_eq!(c.a_id, "front-meaning"); + assert_eq!(c.b_id, "back-example"); + assert!(c.overlap_x > MIN_AXIS_OVERLAP_PX && c.overlap_y > MIN_AXIS_OVERLAP_PX); + assert!(c.overlap_area_ratio > MIN_OVERLAP_AREA_RATIO); + + let mut out = Vec::new(); + push_text_collision_diagnostics(&deck, &rects, &mut out); + assert!( + out.iter().any(|line| line.contains("Chinese Meaning") + && line.contains("Example English") + && line.contains("OVERLAP")), + "diagnostic line must name both text nodes: {out:?}" + ); +} + +/// Real jian layout end-to-end (not a manually-set rects map): a +/// `layout:none` deck of two cards, each bottom-anchoring its own text via +/// `justifyContent: end`, with the cards offset by only 4px — the +/// canonical "stacked deck" authoring shape. Proves the detector is +/// actually wired into `geometry_diagnostics`, not just unit-testable in +/// isolation. +#[test] +fn wired_into_geometry_diagnostics_under_real_layout() { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(json!({ + "version": "1.0", + "children": [{ + "type": "frame", "id": "root", "name": "Screen", "width": 375, "height": 200, + "children": [{ + "type": "frame", "id": "deck", "name": "Stacked Card Container", "layout": "none", + "width": "fill_container", "height": "fill_container", + "children": [ + { "type": "frame", "id": "front", "name": "Front Card", + "x": 0, "y": 0, "width": 300, "height": 130, "layout": "vertical", + "justifyContent": "end", + "children": [ text_node("front-meaning", "Chinese Meaning", "有适应力的；能迅速恢复的") ] }, + { "type": "frame", "id": "back", "name": "Back Card", + "x": 8, "y": 4, "width": 284, "height": 130, "layout": "vertical", + "justifyContent": "end", + "children": [ text_node("back-example", "Example English", "She is resilient in facing hard times") ] } + ] + }] + }] + })) + .expect("doc"); + let state = op_editor_core::EditorState::from_document(doc); + let issues = crate::geometry_validation::geometry_diagnostics(&state); + assert!( + issues.iter().any(|i| i.contains("Chinese Meaning") + && i.contains("Example English") + && i.contains("OVERLAP")), + "real-layout deck must surface the text collision: {issues:?}" + ); +} + +// ── false-positive defenses ── + +/// Normal vertical stack (title above subtitle) that merely touches by a +/// rounding-scale ~1px gap must not be reported — no real overlap. +#[test] +fn adjacent_flow_text_with_a_hairline_gap_is_not_reported() { + let col = json!({ + "type": "frame", "id": "col", "name": "Text Column", "layout": "vertical", + "children": [ + text_node("title", "Task Title", "听力训练"), + text_node("subtitle", "Task Subtitle", "听短文回答 5 个问题 · +20 XP") + ] + }); + let mut rects = HashMap::new(); + rects.insert("title".to_string(), rect(0.0, 0.0, 120.0, 20.0)); + rects.insert("subtitle".to_string(), rect(0.0, 21.0, 200.0, 16.0)); + let collisions = collect_text_collisions(&col, &rects); + assert!( + collisions.is_empty(), + "no overlap, must not report: {collisions:?}" + ); +} + +/// A ring/badge number centered on a donut (ellipse) is not a text-vs-text +/// pair at all — the ellipse never enters the text-leaf list, so there is +/// nothing to compare it against. +#[test] +fn ring_center_text_over_a_non_text_ring_is_not_reported() { + let stack = json!({ + "type": "frame", "id": "ring-stack", "name": "Progress Ring Stack", "layout": "none", + "children": [ + { "type": "frame", "id": "ring-center", "name": "Ring Center", "children": [ + text_node("percent", "Percent Text", "2/3") + ] }, + { "type": "ellipse", "id": "arc", "name": "Progress Arc" }, + { "type": "ellipse", "id": "track", "name": "Track Ring" } + ] + }); + let mut rects = HashMap::new(); + rects.insert("percent".to_string(), rect(20.0, 20.0, 20.0, 14.0)); + rects.insert("arc".to_string(), rect(0.0, 0.0, 60.0, 60.0)); + rects.insert("track".to_string(), rect(0.0, 0.0, 60.0, 60.0)); + let collisions = collect_text_collisions(&stack, &rects); + assert!( + collisions.is_empty(), + "ellipse is never a text leaf: {collisions:?}" + ); +} + +/// A correctly-authored opaque deck where only the FRONT card carries text +/// (the back card is pure chrome, no text child) — nothing to collide with. +#[test] +fn opaque_deck_with_text_only_on_the_front_card_is_not_reported() { + let deck = json!({ + "type": "frame", "id": "deck", "name": "Stacked Card Container", "layout": "none", + "children": [ + { "type": "frame", "id": "front", "name": "Front Card", "children": [ + text_node("only-text", "Word", "resilient") + ] }, + { "type": "frame", "id": "back", "name": "Back Card", "children": [] } + ] + }); + let mut rects = HashMap::new(); + rects.insert("only-text".to_string(), rect(0.0, 0.0, 80.0, 20.0)); + let collisions = collect_text_collisions(&deck, &rects); + assert!( + collisions.is_empty(), + "no second text leaf to collide with: {collisions:?}" + ); +} + +/// Ordinary horizontal flex row of short text/value pairs sitting flush +/// side-by-side (not overlapping) must not be reported — that shape is +/// `collect_sibling_jam_diagnostics`'s territory (and even there, flush +/// but non-overlapping is a JAM warning, not an OVERLAP). +#[test] +fn normal_flex_row_text_siblings_are_not_reported() { + let row = json!({ + "type": "frame", "id": "row", "name": "Price Row", "layout": "horizontal", + "children": [ + text_node("price", "Price", "$29"), + text_node("unit", "Unit", "/mo") + ] + }); + let mut rects = HashMap::new(); + rects.insert("price".to_string(), rect(0.0, 0.0, 30.0, 20.0)); + rects.insert("unit".to_string(), rect(30.0, 4.0, 20.0, 14.0)); + let collisions = collect_text_collisions(&row, &rects); + assert!( + collisions.is_empty(), + "flush but non-overlapping: {collisions:?}" + ); +} + +/// Empty / whitespace-only text and explicitly hidden text never enter +/// the leaf list, regardless of their resolved rect. +#[test] +fn empty_and_hidden_text_are_excluded() { + let group = json!({ + "type": "frame", "id": "group", "name": "Group", "layout": "none", + "children": [ + text_node("empty", "Empty", " "), + { "type": "text", "id": "hidden", "name": "Hidden", "content": "resilient", "visible": false }, + text_node("real", "Real", "resilient") + ] + }); + let mut rects = HashMap::new(); + // Deliberately overlapping rects — if either exclusion were missing + // this would false-positive against "real". + for id in ["empty", "hidden", "real"] { + rects.insert(id.to_string(), rect(0.0, 0.0, 80.0, 20.0)); + } + let collisions = collect_text_collisions(&group, &rects); + assert!( + collisions.is_empty(), + "empty/hidden text must never participate: {collisions:?}" + ); +} + +// ── real-sample harness (manual, not part of the default suite) ── +// +// `OP_TEXT_COLLISION_SAMPLE=/path/to/0724-1-gm-2.op cargo test -p +// op-orchestrator text_collision -- --ignored --nocapture` +#[test] +#[ignore] +fn detects_the_measured_stacked_word_card_sample() { + let path = std::env::var("OP_TEXT_COLLISION_SAMPLE").expect("OP_TEXT_COLLISION_SAMPLE"); + let text = std::fs::read_to_string(&path).expect("read sample .op"); + let doc: jian_ops_schema::PenDocument = serde_json::from_str(&text).expect("parse .op"); + let state = op_editor_core::EditorState::from_document(doc); + let issues = crate::geometry_validation::geometry_diagnostics(&state); + assert!( + issues.iter().any(|i| i.contains("Chinese Meaning") + && i.contains("Example English") + && i.contains("OVERLAP")), + "front card meaning vs back card example must be reported: {issues:?}" + ); +}