diff --git a/crates/op-editor-core/src/hoist_app_state.rs b/crates/op-editor-core/src/hoist_app_state.rs index f2dbc1227..ff6102fd7 100644 --- a/crates/op-editor-core/src/hoist_app_state.rs +++ b/crates/op-editor-core/src/hoist_app_state.rs @@ -7,9 +7,9 @@ //! (`op-editor-core`) then resolves cross-subtask conflicts //! deterministically regardless of concurrent replay order. +use crate::EditorCommand; use jian_ops_schema::node::PenNode; use jian_ops_schema::state::StateSchema; -use crate::EditorCommand; /// `plan_idx` for generation paths that have no orchestrator plan /// (agentic-loop finalize, MCP inserts). `usize::MAX` is the weakest diff --git a/crates/op-orchestrator/src/loop_finalize.rs b/crates/op-orchestrator/src/loop_finalize.rs index c62f2dbc1..34595db13 100644 --- a/crates/op-orchestrator/src/loop_finalize.rs +++ b/crates/op-orchestrator/src/loop_finalize.rs @@ -479,6 +479,25 @@ pub fn apply_loop_finalize(state: &mut EditorState) { crate::role_post_pass::enforce_surface_color_discipline(forest); } + // Hoist node-level `state` into the document root, mirroring the + // orchestrator's per-subtask `hoist_app_state` — without this the + // agentic-loop path leaves `$app.*` unseeded, so generated bindings + // and events reference keys that never reach `doc.state`. Runs over + // the REAL top-level nodes (not the unwrapped section forest) so a + // page wrapper's own `state` is hoisted too. Unplanned priority: + // any planned subtask default wins a conflict; doc-owned keys + // always win regardless. + let hoist_cmd = op_editor_core::hoist_app_state( + state.active_children_mut(), + op_editor_core::UNPLANNED_APP_STATE_IDX, + ); + if matches!( + &hoist_cmd, + EditorCommand::MergeAppState { state: hoisted, .. } if !hoisted.is_empty() + ) { + state.apply(hoist_cmd); + } + // The app-shell restructure (flat-vertical sidebar dashboard → horizontal // [sidebar | content]) runs inside `cleanup::finalize_design` below, the // whole-doc finalize point SHARED with the orchestrator path — so both the diff --git a/crates/op-orchestrator/src/loop_finalize_tests.rs b/crates/op-orchestrator/src/loop_finalize_tests.rs index be56f0d5f..9ecd68fa4 100644 --- a/crates/op-orchestrator/src/loop_finalize_tests.rs +++ b/crates/op-orchestrator/src/loop_finalize_tests.rs @@ -285,6 +285,30 @@ fn loop_finalize_gives_fill_less_text_a_visible_fill_on_dark_surface() { ); } +#[test] +fn loop_finalize_hoists_node_state_to_doc_root() { + let mut state = state_with_forest(json!([ + {"type": "frame", "id": "sec", "name": "Hero", "width": 1200, "height": 400, + "state": {"count": {"type": "int", "default": 3}}, + "children": [ + {"type": "text", "id": "t", "name": "HeroLabel", "content": "hi"} + ]} + ])); + apply_loop_finalize(&mut state); + + // Node-level `state` must be stripped from the tree… + let hero = find_by_name(state.active_children(), "Hero").expect("hero survives finalize"); + let v = serde_json::to_value(hero).expect("serialize hero"); + assert!(v.get("state").is_none(), "node state must be stripped"); + + // …and merged into the document root (the `$app` store). + let root_state = state.doc.state.as_ref().expect("doc-root state seeded"); + assert_eq!( + root_state.get("count").and_then(|e| e.default.clone()), + Some(json!(3)) + ); +} + #[test] fn loop_finalize_leaves_text_on_gradient_surface_alone() { // A gradient (or image) fill cannot be reduced to a solid hex — guessing a diff --git a/crates/op-orchestrator/src/program_gen.rs b/crates/op-orchestrator/src/program_gen.rs index ac6bd748d..bd584af15 100644 --- a/crates/op-orchestrator/src/program_gen.rs +++ b/crates/op-orchestrator/src/program_gen.rs @@ -16,18 +16,35 @@ //! JavaScript program (`op_mcp::script_runner`), and the recorded //! `batch_design` program it produces is handed to //! [`run_program_to_forest`] here to build the section forest. - +//! +//! ## Why this returns a `StateSchema` alongside the forest +//! +//! `op-mcp`'s generation-insert path (`batch_design.rs::hoist_generation_state`) +//! hoists any node-level `state` block into a doc-root `MergeAppState` command +//! BEFORE the insert command, tagged with the "unplanned" priority — it has no +//! orchestrator plan index to stamp. That `MergeAppState` lands on whatever +//! document it is applied to. Here that document is a SCRATCH `EditorState::new()` +//! that exists only to run the program and get a forest back — its `doc.state` +//! is discarded once this function returns the forest alone. The orchestrator +//! (`subagent.rs`) is the one caller that actually knows the subtask's real +//! `plan_idx`, so it needs the hoisted state handed back separately in order to +//! re-tag it with that index (rather than the unplanned one baked in here) before +//! merging it into the live document. Returning `(forest, program_state)` lets the +//! caller do exactly that instead of silently losing every `$app.*` key a +//! script-gen'd subtask declared. use std::collections::BTreeMap; use jian_ops_schema::node::PenNode; +use jian_ops_schema::state::StateSchema; use op_editor_core::EditorState; /// Run a `batch_design` DSL PROGRAM string against a FRESH empty document and -/// return the produced section forest. The caller (`script_gen`) hands in the -/// DSL a JS engine emitted by calling the bound `I`/`C`/… functions. The -/// executor collects per-line errors and applies the surviving lines -/// (best-effort); a program that builds nothing is an error. -pub fn run_program_to_forest(program: &str) -> Result, String> { +/// return the produced section forest plus any doc-root `state` the program's +/// nodes hoisted (see the module doc for why the latter matters). The caller +/// (`script_gen`) hands in the DSL a JS engine emitted by calling the bound +/// `I`/`C`/… functions. The executor collects per-line errors and applies the +/// surviving lines (best-effort); a program that builds nothing is an error. +pub fn run_program_to_forest(program: &str) -> Result<(Vec, StateSchema), String> { let mut state = EditorState::new(); let mut args: BTreeMap = BTreeMap::new(); args.insert("operations".to_string(), program.to_string()); @@ -52,7 +69,11 @@ pub fn run_program_to_forest(program: &str) -> Result, String> { if nodes.is_empty() { return Err("program produced no nodes".into()); } - Ok(nodes) + // The scratch document starts empty, so anything sitting in `doc.state` + // now came from the program's own hoisted `MergeAppState` — drain it so + // the caller can re-tag it with the subtask's real plan_idx. + let program_state = state.doc.state.take().unwrap_or_default(); + Ok((nodes, program_state)) } /// Echo the executor's per-line `errors[]` (if any) to stderr — visibility diff --git a/crates/op-orchestrator/src/program_gen_tests.rs b/crates/op-orchestrator/src/program_gen_tests.rs index cb88aab61..001789196 100644 --- a/crates/op-orchestrator/src/program_gen_tests.rs +++ b/crates/op-orchestrator/src/program_gen_tests.rs @@ -21,8 +21,9 @@ fn run_program_to_forest_nests_cells_under_rows_via_bindings() { "c1=I(r1, {\"type\":\"frame\",\"name\":\"Cell\"})\n", "I(c1, {\"type\":\"text\",\"content\":\"Alice\"})" ); - let nodes = run_program_to_forest(program).expect("program builds a forest"); + let (nodes, state) = run_program_to_forest(program).expect("program builds a forest"); assert_eq!(nodes.len(), 1, "exactly one section root"); + assert!(state.is_empty(), "program declared no state"); let sec = &nodes[0]; let tbl = &sec.children().expect("sec children")[0]; let row = &tbl.children().expect("table children")[0]; @@ -51,7 +52,7 @@ fn image_without_src_and_bad_textgrowth_survive() { "I(sec, {\"type\":\"image\",\"name\":\"Avatar\",\"width\":40,\"height\":40})\n", "I(sec, {\"type\":\"text\",\"content\":\"Hi\",\"textGrowth\":\"fit_content\"})" ); - let nodes = run_program_to_forest(program).expect("program builds a forest"); + let (nodes, _state) = run_program_to_forest(program).expect("program builds a forest"); let sec = &nodes[0]; let kids = sec.children().expect("sec children"); assert_eq!( @@ -71,8 +72,36 @@ fn bare_identifier_sizing_value_survives() { "sec=I(null, {\"type\":\"frame\",\"name\":\"Sec\",\"layout\":\"vertical\",\"width\":\"fill_container\"})\n", "I(sec, {\"type\":\"text\",\"name\":\"Col Service\",\"content\":\"SERVICE\",\"width\":fill_container_str})" ); - let nodes = run_program_to_forest(program).expect("program builds a forest"); + let (nodes, _state) = run_program_to_forest(program).expect("program builds a forest"); let sec = &nodes[0]; let kids = sec.children().expect("sec children"); assert_eq!(kids.len(), 1, "the bare-identifier-sized text survived"); } + +#[test] +fn run_program_to_forest_drains_hoisted_state_off_the_scratch_document() { + // A program whose `I()` node carries a `state` block must come back with + // that state DRAINED from the returned schema — and the returned nodes + // must have their own `state` field stripped (op-mcp's generation-hoist + // already stripped it before this executor ever saw the forest; this is + // a regression guard against that hoist landing on the scratch doc and + // getting silently discarded instead of returned to the caller). + let program = concat!( + "I(null, {\"type\":\"frame\",\"name\":\"Sec\",\"width\":\"fill_container\",", + "\"state\":{\"n\":{\"type\":\"int\",\"default\":0}},", + "\"children\":[{\"type\":\"text\",\"content\":\"Hi\"}]})" + ); + let (nodes, state) = run_program_to_forest(program).expect("program builds a forest"); + assert!( + state.contains_key("n"), + "hoisted state key must be returned to the caller, got {state:?}" + ); + assert_eq!(nodes.len(), 1); + let jian_ops_schema::node::PenNode::Frame(frame) = &nodes[0] else { + panic!("expected a frame root"); + }; + assert!( + frame.state.is_none(), + "returned node must have its state drained, not just the scratch schema" + ); +} diff --git a/crates/op-orchestrator/src/script_gen.rs b/crates/op-orchestrator/src/script_gen.rs index ad230d82c..a5aa80bd6 100644 --- a/crates/op-orchestrator/src/script_gen.rs +++ b/crates/op-orchestrator/src/script_gen.rs @@ -7,10 +7,13 @@ //! program into the section-forest executor. use jian_ops_schema::node::PenNode; +use jian_ops_schema::state::StateSchema; /// Expand the emitted JS into a `batch_design` program and run it to a -/// section forest via the shared executor. -pub fn parse_script(text: &str) -> Result, String> { +/// section forest via the shared executor. Returns the forest plus any +/// doc-root `state` the program's nodes hoisted (see `program_gen`'s module +/// doc for why the caller needs this separately from the forest). +pub fn parse_script(text: &str) -> Result<(Vec, StateSchema), String> { let program = op_mcp::script_runner::run_script_to_program(text)?; crate::program_gen::run_program_to_forest(&program) } diff --git a/crates/op-orchestrator/src/script_gen_tests.rs b/crates/op-orchestrator/src/script_gen_tests.rs index a7c2f9e9e..ddbb7a37e 100644 --- a/crates/op-orchestrator/src/script_gen_tests.rs +++ b/crates/op-orchestrator/src/script_gen_tests.rs @@ -26,7 +26,7 @@ fn js_loop_builds_repeated_rows_nested_under_the_table() { I(cell, {type:"text", content:r.name}); } "#; - let nodes = parse_script(script).expect("script builds a forest"); + let (nodes, _state) = parse_script(script).expect("script builds a forest"); assert_eq!(nodes.len(), 1, "one section root"); let sec = &nodes[0]; let tbl = &sec.children().expect("sec children")[0]; @@ -65,7 +65,7 @@ fn flex_aligned_cells_survive_the_forest() { I(amtCell, {type:"text", content:d.amount}); } "#; - let nodes = parse_script(script).expect("script builds a forest"); + let (nodes, _state) = parse_script(script).expect("script builds a forest"); let json = serde_json::to_string(&nodes).unwrap(); // The flex_end amount cells must NOT be dropped — both amounts present. assert!( @@ -104,7 +104,8 @@ fn truncated_script_repair_still_builds_a_forest_via_parse_script() { const b = I(sec, {type:"text", content:"kept-2"}); I(sec, {type:"text", content:"dangl "#; - let nodes = parse_script(cut).expect("truncation repair must salvage a runnable forest"); + let (nodes, _state) = + parse_script(cut).expect("truncation repair must salvage a runnable forest"); let sec = &nodes[0]; assert_eq!( sec.children().map(|c| c.len()).unwrap_or(0), diff --git a/crates/op-orchestrator/src/subagent.rs b/crates/op-orchestrator/src/subagent.rs index c78535aab..9e0929b81 100644 --- a/crates/op-orchestrator/src/subagent.rs +++ b/crates/op-orchestrator/src/subagent.rs @@ -167,8 +167,12 @@ pub(crate) async fn run_subtask_with_reveal_at( // Script-gen is THE protocol on the full first attempt; the reduced / // minimal retry rungs teach raw JSONL, so parsing falls back to // `parse_nodes` there (matching the prompt in build_subagent_prompt). + // `program_state` carries any doc-root `state` script-gen's underlying + // `run_program_to_forest` hoisted on the SCRATCH document it builds the + // forest against (see `program_gen`'s module doc) — the flat-JSONL rung + // has no such hoist, so it stays an empty schema there. let script_on = !reduced_complexity && !minimal_skills; - let mut nodes = if script_on { + let (mut nodes, program_state) = if script_on { match crate::script_gen::parse_script(&text) { Ok(n) => n, Err(e) => { @@ -184,7 +188,7 @@ pub(crate) async fn run_subtask_with_reveal_at( } } else { match parse_nodes(&text) { - Ok(n) => n, + Ok(n) => (n, jian_ops_schema::state::StateSchema::new()), Err(e) => { tracing::warn!( subtask = %subtask.id, @@ -286,7 +290,18 @@ pub(crate) async fn run_subtask_with_reveal_at( .iter() .position(|s| s.id == subtask.id) .unwrap_or(0); - let merge_state = op_editor_core::hoist_app_state(&mut nodes, plan_idx); + let mut merge_state = op_editor_core::hoist_app_state(&mut nodes, plan_idx); + // Union in whatever state script-gen's scratch-document run already + // hoisted (`program_state`) — it was tagged "unplanned" there since + // `program_gen`/`op-mcp` don't know this subtask's real plan_idx. Node- + // drained state (above) takes priority on key collisions via `or_insert`; + // in practice there's no real overlap since exactly one protocol runs per + // attempt, but `or_insert` keeps the merge deterministic regardless. + if let EditorCommand::MergeAppState { state, .. } = &mut merge_state { + for (k, v) in program_state { + state.entry(k).or_insert(v); + } + } let has_state = matches!(&merge_state, EditorCommand::MergeAppState { state, .. } if !state.is_empty()); if has_state {