diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index 7b85a146e..10873660d 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -673,44 +673,68 @@ pub fn finalize_design(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ids pub fn run_cleanup_passes(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ids: &[&str]) { for root_id in root_ids { - // FIRST: turn a flat-vertical desktop dashboard with a full-width - // sidebar into a horizontal [sidebar | content] app-shell, so the - // remaining cleanup passes (esp. `adjust_root_height_to_content`) see - // the corrected shape. This is the shared whole-doc finalize point for - // BOTH the orchestrator (per-subtask role passes already ran) and the - // agentic loop (whole-doc role passes ran in `apply_loop_finalize`), so - // the moved sections keep their resolved roles. - restructure_app_shell_root(sink, root_id); - remove_duplicate_status_bars(sink, root_id); - repair_light_mobile_nav_surfaces(sink, root_id); - repair_mobile_content_sections(sink, root_id); - cleanup_mobile_chrome::repair_mobile_structural_chrome(sink, root_id); - cleanup_mobile_dense::repair_dense_mobile_rows(sink, root_id); - cleanup_desktop_dashboard::repair_sparse_desktop_dashboard_rows(sink, plan, root_id); - repair_overbold_text_hierarchy(sink, root_id); - strip_decorative_filled_strokes(sink, root_id); - adjust_root_height_to_content(sink, root_id); + // FIRST: whole-root structural restructures, shared by BOTH the + // orchestrator (per-subtask role passes already ran) and the agentic + // loop (whole-doc role passes ran in `apply_loop_finalize`), so the + // moved sections keep their resolved roles. These swap the root via + // `ReplaceSubtree`, which allocates a FRESH root id — `apply_root_transform` + // returns the new id so the per-root cleanup passes below don't look up a + // stale id and silently no-op. + let mut rid = root_id.to_string(); + // Flat-vertical / crammed-horizontal sidebar dashboard → [sidebar | content]. + rid = apply_root_transform(sink, &rid, crate::app_shell::reshape_sidebar_to_app_shell); + // Flat table cells → Table → Row → Cell. + rid = apply_root_transform(sink, &rid, crate::table_repair::regroup_flat_table_rows); + let rid = rid.as_str(); + + remove_duplicate_status_bars(sink, rid); + repair_light_mobile_nav_surfaces(sink, rid); + repair_mobile_content_sections(sink, rid); + cleanup_mobile_chrome::repair_mobile_structural_chrome(sink, rid); + cleanup_mobile_dense::repair_dense_mobile_rows(sink, rid); + cleanup_desktop_dashboard::repair_sparse_desktop_dashboard_rows(sink, plan, rid); + repair_overbold_text_hierarchy(sink, rid); + strip_decorative_filled_strokes(sink, rid); + adjust_root_height_to_content(sink, rid); } } -/// Restructure the page-root in place when it is a flat-vertical desktop -/// dashboard whose first child is a full-width sidebar (see -/// [`crate::app_shell::reshape_sidebar_to_app_shell`] for the strict gate). The -/// whole subtree is swapped via `ReplaceSubtree` (the new root carries the -/// reorganized children, so `drop_children: true` loses nothing). -fn restructure_app_shell_root(sink: &mut dyn DocSink, root_id: &str) { - let Some(root) = find_root(sink.state(), root_id) else { - return; +/// Apply a whole-root transform (the serialize → mutate → deserialize round-trip +/// the structural passes use) to the page-root and commit it via `ReplaceSubtree`. +/// +/// `ReplaceSubtree` allocates a FRESH id for the replaced node (see +/// `command_replace_tests`), so the root's id changes on every successful +/// transform. This returns the root's CURRENT id (re-resolved by its unchanged +/// position) so the caller threads it into the next pass — otherwise every +/// subsequent per-root cleanup pass would look up the stale id and no-op. +fn apply_root_transform( + sink: &mut dyn DocSink, + root_id: &str, + transform: fn(&mut PenNode) -> bool, +) -> String { + let Some(idx) = sink + .state() + .active_children() + .iter() + .position(|n| n.id_str() == root_id) + else { + return root_id.to_string(); }; - let mut new_root = root.clone(); - if crate::app_shell::reshape_sidebar_to_app_shell(&mut new_root) { - sink.apply(EditorCommand::ReplaceSubtree { - node_id: NodeId::new(root_id.to_string()), - node: Box::new(new_root), - drop_children: true, - page_id: None, - }); + let mut new_root = sink.state().active_children()[idx].clone(); + if !transform(&mut new_root) { + return root_id.to_string(); } + sink.apply(EditorCommand::ReplaceSubtree { + node_id: NodeId::new(root_id.to_string()), + node: Box::new(new_root), + drop_children: true, + page_id: None, + }); + sink.state() + .active_children() + .get(idx) + .map(|n| n.id_str().to_string()) + .unwrap_or_else(|| root_id.to_string()) } /// Strip the REDUNDANT border off a filled, shadowed container. When a diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index f374f01c5..5a898ba82 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -57,6 +57,7 @@ pub mod run; pub mod scaffold; pub mod spawn_concurrent; pub mod subagent; +pub mod table_repair; pub mod tree_heuristics; #[cfg(test)] diff --git a/crates/op-orchestrator/src/table_repair.rs b/crates/op-orchestrator/src/table_repair.rs new file mode 100644 index 000000000..9d976d047 --- /dev/null +++ b/crates/op-orchestrator/src/table_repair.rs @@ -0,0 +1,326 @@ +//! Table repair — regroup flat table cells into a Table→Row→Cell hierarchy. +//! +//! Weak models (glm-5.2 "Client Roster") emit a table as a header row followed +//! by FLAT sibling cells stacked vertically: `Table Header`, `R1 Client Cell`, +//! `R1 Visit`, `R1 Barber`, `R1 Spend`, `R1 Status Badge`, `R2 Client Cell`, … +//! Each client's cells render stacked (left-aligned, not under their columns) +//! and the full-width cells become full-width bars. This pass groups those flat +//! cells back into `Table(vertical) → Row(horizontal) → Cell` whose body cells +//! share the header's per-column widths. +//! +//! Run alongside `app_shell` in `cleanup::run_cleanup_passes`. Detection is +//! deliberately NARROW and never guesses (the adversarial design review showed +//! a heuristic header + chunk-of-N grouping over-fires on toolbars / tab bars / +//! text feeds and mis-segments when the column count drifts): it requires BOTH +//! an explicit header (role/name) AND every trailing cell to carry an explicit +//! `R{n}` / `row N` row-index name, and aborts on any ragged / conflicting / +//! ambiguous run. It is a safe backstop for the exact glm shape, not a general +//! table inferencer. + +use jian_ops_schema::node::PenNode; +use serde_json::{json, Value}; + +/// Regroup a dashboard's flat table cells into Table→Row→Cell. Mutates the +/// page-root in place via the serialize → mutate `Value` → deserialize +/// round-trip the section passes use. Returns `true` iff it restructured at +/// least one table; no-op + `false` otherwise (the node is never dropped). +pub(crate) fn regroup_flat_table_rows(root: &mut PenNode) -> bool { + let Ok(mut v) = serde_json::to_value(&*root) else { + return false; + }; + if !regroup_in_value(&mut v) { + return false; + } + match serde_json::from_value::(v) { + Ok(new_node) => { + *root = new_node; + true + } + Err(_) => false, + } +} + +/// Post-order: recurse into children first, then try to regroup THIS node's +/// children (so a freshly-built Table subtree isn't re-descended in one pass). +fn regroup_in_value(v: &mut Value) -> bool { + let mut changed = false; + if let Some(kids) = v.get_mut("children").and_then(Value::as_array_mut) { + for c in kids.iter_mut() { + changed |= regroup_in_value(c); + } + } + changed | try_regroup_children(v) +} + +// ── tolerant Value readers (mirror the app_shell idiom) ── + +fn num(v: &Value, key: &str) -> Option { + let f = v.get(key)?; + f.as_f64() + .or_else(|| f.as_str().and_then(|s| s.parse::().ok())) +} + +fn layout_str(v: &Value) -> Option<&str> { + v.get("layout").and_then(Value::as_str) +} + +fn role_str(v: &Value) -> Option<&str> { + v.get("role").and_then(Value::as_str) +} + +fn ident_text(v: &Value) -> String { + let name = v.get("name").and_then(Value::as_str).unwrap_or(""); + let role = v.get("role").and_then(Value::as_str).unwrap_or(""); + format!("{name} {role}").to_lowercase() +} + +fn is_column_layout(v: &Value) -> bool { + !matches!(layout_str(v), Some("horizontal")) +} + +/// An explicit table-header row: a horizontal frame of ≥2 column labels, tagged +/// by role or name. The heuristic "any horizontal frame of short texts" header +/// is intentionally NOT accepted — it false-positives on toolbars / tab bars. +fn is_table_header(v: &Value) -> Option { + let t = ident_text(v); + let tagged = role_str(v) == Some("table-header") + || t.contains("table header") + || t.contains("header row") + || t.contains("column header") + || t.contains("thead"); + if !tagged { + return None; + } + if layout_str(v) != Some("horizontal") { + return None; + } + let n = v + .get("children") + .and_then(Value::as_array) + .map_or(0, Vec::len); + (2..=12).contains(&n).then_some(n) +} + +/// A cell's explicit row index from an `R{n}` / `row N` / `row-N` name. Returns +/// `None` when the name carries no row index — which aborts the whole regroup +/// (we never guess boundaries from position alone). +fn row_index(v: &Value) -> Option { + let name = v.get("name").and_then(Value::as_str)?.trim().to_lowercase(); + let rest = name + .strip_prefix('r') + .or_else(|| name.strip_prefix("row-")) + .or_else(|| name.strip_prefix("row ")) + .or_else(|| name.strip_prefix("row"))?; + let digits: String = rest.chars().take_while(char::is_ascii_digit).collect(); + digits.parse::().ok() +} + +/// A node that can be a table cell: a text, or a frame/group whose role/name +/// reads as a cell-ish element. Crucially every candidate must ALSO carry a row +/// index (checked by the caller) — this predicate only bounds the run. +fn is_cell_like(v: &Value) -> bool { + matches!( + v.get("type").and_then(Value::as_str), + Some("text" | "frame" | "group") + ) +} + +/// A structural region that terminates the flat-cell run (and is preserved). +fn is_run_terminator(v: &Value) -> bool { + let t = ident_text(v); + role_str(v) == Some("section") + || t.contains("section") + || t.contains("pagination") + || t.contains("footer") + || t.contains("navbar") + || t.contains("table-row") +} + +// ── detection + grouping ── + +fn try_regroup_children(parent: &mut Value) -> bool { + if !is_column_layout(parent) { + return false; + } + let Some(kids) = parent.get("children").and_then(Value::as_array) else { + return false; + }; + if kids.len() < 3 { + return false; + } + // Locate the header + its column count. + let Some((h_idx, n_cols)) = kids + .iter() + .enumerate() + .find_map(|(i, c)| is_table_header(c).map(|n| (i, n))) + else { + return false; + }; + // Already-grouped? A table-row sibling means this ran already. + if kids[h_idx + 1..] + .iter() + .any(|c| role_str(c) == Some("table-row")) + { + return false; + } + // Collect the contiguous flat-cell run after the header, requiring EVERY + // cell to carry an explicit row index (abort on the first that doesn't). + let header = &kids[h_idx]; + let col_widths = header_column_widths(header, n_cols); + let (header_pad, header_gap) = header_metrics(header); + + let mut run: Vec<(u32, &Value)> = Vec::new(); + for c in &kids[h_idx + 1..] { + if is_run_terminator(c) { + break; + } + if !is_cell_like(c) { + break; + } + let Some(idx) = row_index(c) else { + // A cell with no row index → ambiguous → do not guess; abort. + return false; + }; + run.push((idx, c)); + } + let group = match group_rows(&run, n_cols) { + Some(g) => g, + None => return false, + }; + let k = group.len(); + let run_len = run.len(); + + // Build the Table subtree: [header, row_0, … row_{k-1}]. + let parent_id = parent.get("id").and_then(Value::as_str).unwrap_or("table"); + let mut table_children: Vec = Vec::with_capacity(k + 1); + table_children.push(header.clone()); + for (ri, row_cells) in group.into_iter().enumerate() { + table_children.push(build_row( + parent_id, + ri, + row_cells, + &col_widths, + &header_pad, + header_gap, + )); + } + let table = json!({ + "type": "frame", + "id": format!("{parent_id}-table"), + "name": "Table", + "role": "table", + "width": "fill_container", + "layout": "vertical", + "children": table_children, + }); + + // Splice: keep children[..h_idx], drop header+run, insert the Table frame, + // keep the rest (the terminator and anything after it). + let Some(arr) = parent.get_mut("children").and_then(Value::as_array_mut) else { + return false; + }; + let tail_start = h_idx + 1 + run_len; + let tail: Vec = arr.split_off(tail_start); + arr.truncate(h_idx); // drop the header + the flat cells + arr.push(table); + arr.extend(tail); + true +} + +/// Group the row-indexed run into `k` rows of exactly `n_cols` each. Returns +/// `None` (abort) on any irregularity: <2 rows, a row whose size ≠ n_cols, or a +/// non-contiguous index sequence. Order is preserved as encountered. +fn group_rows<'a>(run: &[(u32, &'a Value)], n_cols: usize) -> Option>> { + if run.is_empty() { + return None; + } + let mut rows: Vec<(u32, Vec<&'a Value>)> = Vec::new(); + for (idx, cell) in run { + match rows.last_mut() { + Some((cur, cells)) if cur == idx => cells.push(cell), + _ => rows.push((*idx, vec![*cell])), + } + } + if rows.len() < 2 { + return None; + } + // Every row must be exactly the header width, and the indices must be a + // strictly increasing contiguous sequence (1,2,3,… or 0,1,2,…). + let first = rows[0].0; + for (offset, (idx, cells)) in rows.iter().enumerate() { + if cells.len() != n_cols { + return None; + } + if *idx != first + offset as u32 { + return None; + } + } + Some(rows.into_iter().map(|(_, cells)| cells).collect()) +} + +/// Per-column width from the header children: numeric width, else the x-gap to +/// the next column, else `fill_container`. +fn header_column_widths(header: &Value, n_cols: usize) -> Vec { + let kids = header.get("children").and_then(Value::as_array); + let mut out = Vec::with_capacity(n_cols); + for j in 0..n_cols { + let w = kids.and_then(|k| k.get(j)).and_then(|c| num(c, "width")); + out.push(match w { + Some(v) => json!(v), + None => json!("fill_container"), + }); + } + out +} + +fn header_metrics(header: &Value) -> (Value, f64) { + let pad = header + .get("padding") + .cloned() + .unwrap_or_else(|| json!([12, 16])); + let gap = num(header, "gap").unwrap_or(16.0); + (pad, gap) +} + +/// Build one Row frame (role table-row → horizontal/fill/center defaults), +/// mapping each cell positionally to its column width. +fn build_row( + parent_id: &str, + ri: usize, + cells: Vec<&Value>, + col_widths: &[Value], + pad: &Value, + gap: f64, +) -> Value { + let row_cells: Vec = cells + .into_iter() + .enumerate() + .map(|(j, c)| { + let mut cell = c.clone(); + if let Some(obj) = cell.as_object_mut() { + let w = col_widths + .get(j) + .cloned() + .unwrap_or(json!("fill_container")); + obj.insert("width".into(), w); + } + cell + }) + .collect(); + json!({ + "type": "frame", + "id": format!("{parent_id}-row-{ri}"), + "name": format!("Row {}", ri + 1), + "role": "table-row", + "layout": "horizontal", + "width": "fill_container", + "alignItems": "center", + "padding": pad, + "gap": gap, + "children": row_cells, + }) +} + +#[cfg(test)] +#[path = "table_repair_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/table_repair_tests.rs b/crates/op-orchestrator/src/table_repair_tests.rs new file mode 100644 index 000000000..6af078b35 --- /dev/null +++ b/crates/op-orchestrator/src/table_repair_tests.rs @@ -0,0 +1,184 @@ +//! Tests for the flat-table regroup pass. Positive = the glm "Client Roster" +//! flat-cell shape; negatives are the false-positives the adversarial review +//! flagged (toolbar/tab-bar header, no row index, ragged run, already grouped, +//! plain text feed). + +use super::*; + +fn node(v: Value) -> PenNode { + serde_json::from_value::(v).expect("valid PenNode fixture") +} +fn val(n: &PenNode) -> Value { + serde_json::to_value(n).expect("serialize PenNode") +} +fn txt(id: &str, name: &str, w: i64) -> Value { + json!({"type":"text","id":id,"name":name,"width":w,"content":name}) +} + +fn header_5col() -> Value { + json!({"type":"frame","id":"hd","name":"Table Header","layout":"horizontal", + "width":"fill_container","padding":[12,16],"gap":16,"children":[ + txt("th1","TH Client",452), txt("th2","TH Last Visit",120), + txt("th3","TH Barber",140), txt("th4","TH Spend",100), txt("th5","TH Status",80)]}) +} + +/// `Main Content` (vertical) holding a prefix section, the header, then 2×5 flat +/// row-indexed cells — the reported shape. +fn flat_table_root() -> PenNode { + node(json!({ + "type":"frame","id":"main","name":"Main Content","width":940,"layout":"vertical", + "children":[ + {"type":"frame","id":"kpi","name":"Key Metrics","layout":"horizontal","width":"fill_container","children":[]}, + header_5col(), + {"type":"frame","id":"r1c","name":"R1 Client Cell","width":940,"layout":"horizontal","children":[]}, + txt("r1v","R1 Visit",120), txt("r1b","R1 Barber",140), txt("r1s","R1 Spend",100), + {"type":"frame","id":"r1st","name":"R1 Status Badge","width":940,"layout":"horizontal","children":[]}, + {"type":"frame","id":"r2c","name":"R2 Client Cell","width":940,"layout":"horizontal","children":[]}, + txt("r2v","R2 Visit",120), txt("r2b","R2 Barber",140), txt("r2s","R2 Spend",100), + {"type":"frame","id":"r2st","name":"R2 Status Badge","width":940,"layout":"horizontal","children":[]} + ] + })) +} + +#[test] +fn positive_flat_cells_grouped_into_table_rows() { + let mut root = flat_table_root(); + assert!( + regroup_flat_table_rows(&mut root), + "flat table must regroup" + ); + let v = val(&root); + let kids = v["children"].as_array().unwrap(); + // Prefix section preserved; header+10 cells collapsed into one Table frame. + assert_eq!(kids.len(), 2, "[Key Metrics, Table]"); + assert_eq!(kids[0]["name"], json!("Key Metrics")); + let table = &kids[1]; + assert_eq!(table["role"], json!("table")); + assert_eq!(layout_str(table), Some("vertical")); + let tkids = table["children"].as_array().unwrap(); + assert_eq!(tkids.len(), 3, "[header, Row 1, Row 2]"); + assert_eq!(tkids[0]["name"], json!("Table Header")); + + let row1 = &tkids[1]; + assert_eq!(row1["role"], json!("table-row")); + assert_eq!(layout_str(row1), Some("horizontal")); + let r1: Vec<&str> = row1["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["name"].as_str().unwrap()) + .collect(); + assert_eq!( + r1, + [ + "R1 Client Cell", + "R1 Visit", + "R1 Barber", + "R1 Spend", + "R1 Status Badge" + ] + ); + // Body cells take the header column widths (fixes the full-width status bar). + let w: Vec = row1["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["width"].as_f64().unwrap()) + .collect(); + assert_eq!(w, [452.0, 120.0, 140.0, 100.0, 80.0]); + + let row2: Vec<&str> = tkids[2]["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["name"].as_str().unwrap()) + .collect(); + assert_eq!( + row2, + [ + "R2 Client Cell", + "R2 Visit", + "R2 Barber", + "R2 Spend", + "R2 Status Badge" + ] + ); +} + +fn assert_untouched(mut root: PenNode, why: &str) { + let before = val(&root); + assert!( + !regroup_flat_table_rows(&mut root), + "must NOT regroup: {why}" + ); + assert_eq!(val(&root), before, "unchanged: {why}"); +} + +#[test] +fn negative_toolbar_not_a_header() { + // A horizontal toolbar of short labels is NOT tagged table-header → ignored. + assert_untouched( + node( + json!({"type":"frame","id":"m","name":"Main","width":940,"layout":"vertical","children":[ + {"type":"frame","id":"tb","name":"Toolbar","layout":"horizontal","children":[ + txt("a","Filter",60), txt("b","Sort",50), txt("c","Export",60)]}, + txt("x1","R1 Item",100), txt("x2","R2 Item",100), txt("x3","R3 Item",100)]}), + ), + "toolbar is not a tagged table header", + ); +} + +#[test] +fn negative_cells_without_row_index() { + // Header present, but the cells carry no R{n} index → never guess → abort. + assert_untouched( + node( + json!({"type":"frame","id":"m","name":"Main","width":940,"layout":"vertical","children":[ + header_5col(), + txt("a","Client A",452), txt("b","Oct 12",120), txt("c","Marcus",140), + txt("d","$1,240",100), {"type":"frame","id":"e","name":"Status","width":80,"children":[]}]}), + ), + "cells lack explicit row index", + ); +} + +#[test] +fn negative_ragged_run() { + // R1 has 5 cells, R2 has only 3 → group size != header columns → abort. + assert_untouched( + node( + json!({"type":"frame","id":"m","name":"Main","width":940,"layout":"vertical","children":[ + header_5col(), + txt("a","R1 Client",452), txt("b","R1 Visit",120), txt("c","R1 Barber",140), + txt("d","R1 Spend",100), txt("e","R1 Status",80), + txt("f","R2 Client",452), txt("g","R2 Visit",120), txt("h","R2 Barber",140)]}), + ), + "row 2 is ragged (3 != 5 columns)", + ); +} + +#[test] +fn negative_already_grouped() { + assert_untouched( + node( + json!({"type":"frame","id":"m","name":"Main","width":940,"layout":"vertical","children":[ + header_5col(), + {"type":"frame","id":"row1","name":"Row 1","role":"table-row","layout":"horizontal","children":[]}, + {"type":"frame","id":"row2","name":"Row 2","role":"table-row","layout":"horizontal","children":[]}]}), + ), + "rows already grouped (table-row role present)", + ); +} + +#[test] +fn negative_plain_text_feed() { + // A vertical list of texts with no table header → nothing to regroup. + assert_untouched( + node( + json!({"type":"frame","id":"m","name":"Activity Feed","width":600,"layout":"vertical","children":[ + txt("a","Alice commented",400), txt("b","Bob shared a file",400), + txt("c","Carol joined",400), txt("d","Dave updated status",400)]}), + ), + "plain text feed, no header", + ); +}