fix(editor): treat a no-op MergeAppState as success

MergeAppState is additive by design — doc-owned keys win and lower
plan_idx wins, so "nothing to add" is the designed outcome, not a
failure. Returning false made Batch[merge, insert] reject the entire
generated insert whenever the target document already carried every
declared key (regenerating a section over an opened file), and made
batch_program report a line failed after its insert had already
landed. The return now signals "command processed"; regression tests
cover the batch-survival and pre-seeded-doc cases.

crates/op-editor-core/src/command_app_state_tests.rs

@@ -1,7 +1,9 @@
 #![cfg(test)]
 
 use crate::command::EditorCommand;
+use crate::node_id::NodeId;
 use crate::state::EditorState;
+use crate::test_support::frame;
 use jian_ops_schema::state::{PrimitiveType, StateEntry, StateType};
 use std::collections::BTreeMap;
 
@@ -32,6 +34,15 @@ fn merge_app_state_adds_new_keys() {
 }
 
 // (b) Pre-existing doc-root key is NEVER overwritten (old .op file compat).
+//
+// Contract change (deliberate): every incoming key being skipped is a
+// legitimate additive no-op, not a failure — `apply` now reports `true`
+// ("command processed") even though the doc-owned value didn't change.
+// The old `!s.apply(...)` assertion encoded the wrong contract: `Batch`
+// rolls back on the first `false` sub-command, so a `false` here used
+// to reject an otherwise-valid insert/replace riding in the same batch
+// whenever a regenerated section declared a key the document already
+// owned (see `merge_app_state`'s doc comment in `command_apply.rs`).
 #[test]
 fn merge_app_state_does_not_overwrite_existing_key() {
     let mut s = EditorState::new();
@@ -41,8 +52,9 @@ fn merge_app_state_does_not_overwrite_existing_key() {
 
     let mut m = BTreeMap::new();
     m.insert("owned".into(), entry(0));
-    // Apply returns false — no change was made.
-    assert!(!s.apply(EditorCommand::MergeAppState {
+    // Apply returns true — the merge was processed (nothing to add is
+    // the designed outcome), but the doc-owned value must be untouched.
+    assert!(s.apply(EditorCommand::MergeAppState {
         plan_idx: 0,
         state: m,
     }));
@@ -69,8 +81,9 @@ fn merge_app_state_is_additive_lower_plan_idx_wins_order_independent() {
             let mut m = BTreeMap::new();
             m.insert("owned".into(), entry(def)); // collides w/ pre-existing
             m.insert("count".into(), entry(def)); // generation-added, conflicts across subtasks
-                                                  // Return value is "did anything change?" — may be false when a
-                                                  // higher plan_idx loses to a previously registered owner.
+                                                  // Return value means "processed", not "changed" — it stays
+                                                  // true even when a higher plan_idx loses to a previously
+                                                  // registered owner (a legitimate no-op, not a failure).
             let _ = st.apply(EditorCommand::MergeAppState { plan_idx, state: m });
         }
         let s = st.doc.state.clone().unwrap();
@@ -95,17 +108,68 @@ fn merge_app_state_is_additive_lower_plan_idx_wins_order_independent() {
     assert_eq!(in_order, reversed, "merge must be order-independent");
 }
 
-// Empty incoming state is a no-op (returns false, doc unchanged).
+// Empty incoming state is a no-op — processed successfully (returns
+// true), doc left untouched. Same contract-change rationale as
+// `merge_app_state_does_not_overwrite_existing_key` above: an empty
+// merge is not an invalid command, so it must not be able to sink a
+// `Batch` it happens to ride in.
 #[test]
 fn merge_app_state_empty_incoming_is_noop() {
     let mut s = EditorState::new();
-    assert!(!s.apply(EditorCommand::MergeAppState {
+    assert!(s.apply(EditorCommand::MergeAppState {
         plan_idx: 0,
         state: BTreeMap::new(),
     }));
     assert!(s.doc.state.is_none());
 }
 
+// Regression for the codex BLOCKER: a no-op merge (every incoming key
+// already doc-owned) must report `true` so a `Batch` carrying it
+// alongside a real insert does NOT roll the whole batch back. Before
+// the fix, `merge_app_state` returned `false` here, `cmd_batch` saw
+// that as sub-command #1 failing, and rolled back — discarding the
+// insert too, even though the insert itself was perfectly valid. This
+// is exactly the shape `op-mcp`'s `with_hoisted_state` builds on every
+// generation path (`insert_node` / `replace_node` / `design_content` /
+// `design_skeleton` / `batch_design` operations-with-state).
+#[test]
+fn noop_merge_is_success_and_batch_survives() {
+    let mut s = EditorState::new();
+    let mut existing: BTreeMap<String, StateEntry> = BTreeMap::new();
+    existing.insert("owned".into(), entry(99));
+    s.doc.state = Some(existing);
+
+    let mut m = BTreeMap::new();
+    m.insert("owned".into(), entry(0)); // same key the doc already owns — pure no-op
+    let before = s.active_children().len();
+    assert!(
+        s.apply(EditorCommand::Batch {
+            commands: vec![
+                EditorCommand::MergeAppState {
+                    plan_idx: 0,
+                    state: m,
+                },
+                EditorCommand::InsertSubtree {
+                    nodes: vec![frame("f1", "Card", 0.0, 0.0, 100.0, 100.0, vec![])],
+                    parent_id: NodeId::NONE,
+                    page_id: None,
+                },
+            ],
+        }),
+        "a no-op merge must not sink the batch"
+    );
+    assert_eq!(
+        s.active_children().len(),
+        before + 1,
+        "the insert must have landed"
+    );
+    assert_eq!(
+        s.doc.state.as_ref().unwrap().get("owned").unwrap().default,
+        Some(serde_json::json!(99)),
+        "doc-owned value must be unchanged by the no-op merge"
+    );
+}
+
 // (e) A rolled-back batch must not leave stale ownership: the failed
 // batch's MergeAppState never landed in doc.state, so a later merge of
 // the same key (any plan_idx) must land instead of being skipped

crates/op-editor-core/src/command_apply.rs

@@ -16,7 +16,10 @@
 //! The result type is `bool` — identical to shell-core's
 //! `apply_mcp_command`: `true` when the command changed something (so
 //! a host can decide whether to push undo / persist), `false` on an
-//! apply-time validation failure.
+//! apply-time validation failure. **Exception:** [`EditorState::
+//! merge_app_state`] (`MergeAppState`) reports "processed", not
+//! "changed" — see its doc comment for why a no-op merge must still
+//! return `true`.
 //!
 use crate::align::AlignAction;
 use crate::command::{EditorCommand, VariableScalarPayload};
@@ -154,9 +157,12 @@ fn apply_import_svg_on_active_page(
 }
 
 impl EditorState {
-    /// Apply one [`EditorCommand`]. Returns `true` when the command
-    /// actually changed the document / editor state, `false` on an
-    /// apply-time validation failure.
+    /// Apply one [`EditorCommand`]. Returns `true` when the command was
+    /// processed (which for most commands means it changed the document
+    /// / editor state), `false` on an apply-time validation failure.
+    /// Exception: an additive [`EditorCommand::MergeAppState`] whose
+    /// every key defers to an existing owner is a designed no-op and
+    /// still returns `true` — see [`Self::merge_app_state`].
     pub fn apply(&mut self, cmd: EditorCommand) -> bool {
         match cmd {
             // --- Raw node CRUD -------------------------------------
@@ -821,32 +827,62 @@ impl EditorState {
     /// inserted. On a conflicting key the incoming `plan_idx` is compared
     /// to the registered owner; if it is strictly lower it replaces both
     /// the owner record and the document value.
+    ///
+    /// ## Return contract
+    ///
+    /// The return value signals **"command processed"**, not **"keys
+    /// landed"**. `MergeAppState` is additive by design: doc-owned keys
+    /// always win, and among generation-added keys the lower `plan_idx`
+    /// wins. A run where every incoming key was skipped (already
+    /// doc-owned, or lost the `plan_idx` ownership race) is the designed
+    /// steady-state outcome, not a failure — it MUST return `true`.
+    ///
+    /// This matters beyond the local call site: `MergeAppState` rides
+    /// inside `EditorCommand::Batch` alongside a node insert/replace on
+    /// every generation path (`hoist_generation_state` +
+    /// `with_hoisted_state` in `op-mcp`), and `Batch`'s apply loop
+    /// (`command_batch.rs::cmd_batch`) treats the first sub-command that
+    /// returns `false` as a hard failure and rolls the ENTIRE batch back.
+    /// Returning `false` for a legitimate no-op merge would silently
+    /// reject an otherwise-valid insert/replace every time a regenerated
+    /// section declares a state key the document root already carries —
+    /// a completely normal flow, not a collision. There is currently no
+    /// invalid-command shape for `MergeAppState` (any `plan_idx` /
+    /// `StateEntry` payload is well-formed), so every path below returns
+    /// `true`.
     fn merge_app_state(
         &mut self,
         plan_idx: usize,
         incoming: std::collections::BTreeMap<String, jian_ops_schema::state::StateEntry>,
     ) -> bool {
         if incoming.is_empty() {
-            return false;
+            // Nothing to merge is a no-op, not a failure — see the
+            // return-contract note above. Kept as an early return
+            // (rather than falling into the loop) purely to skip the
+            // `get_or_insert_with` allocation on doc.state when there is
+            // nothing to write into it.
+            return true;
         }
         let root = self
             .doc
             .state
             .get_or_insert_with(std::collections::BTreeMap::new);
-        let mut changed = false;
         for (key, entry) in incoming {
             match self.app_state_owner.entry(key.clone()) {
                 std::collections::btree_map::Entry::Vacant(slot) => {
                     // Pre-existing doc-root key: owned by the file, skip.
+                    // Not a failure — the file's value is authoritative
+                    // and is left untouched.
                     if root.contains_key(&key) {
                         continue;
                     }
                     root.insert(key, entry);
                     slot.insert(plan_idx);
-                    changed = true;
                 }
                 std::collections::btree_map::Entry::Occupied(mut slot) => {
-                    // Generation-added key: lower plan_idx wins.
+                    // Generation-added key: lower plan_idx wins. Losing
+                    // the race is not a failure — the earlier subtask's
+                    // value already won and stays in place.
                     if plan_idx < *slot.get() {
                         tracing::warn!(
                             target: "op.skills",
@@ -857,11 +893,10 @@ impl EditorState {
                         );
                         root.insert(key, entry);
                         slot.insert(plan_idx);
-                        changed = true;
                     }
                 }
             }
         }
-        changed
+        true
     }
 }

crates/op-mcp/src/batch_design_tests.rs

@@ -1195,6 +1195,76 @@ fn batch_design_without_node_state_keeps_plain_command() {
     }
 }
 
+#[test]
+fn batch_design_noop_state_merge_still_lands_the_insert() {
+    // Regression for the codex BLOCKER: regenerating a section into a
+    // document whose root state ALREADY carries the declared key is a
+    // completely normal flow (the merge is a legitimate additive
+    // no-op), not a failure. Before the fix, `merge_app_state` returned
+    // `false` for the fully-skipped-keys case, so the sim-validated
+    // `ctx.emit` in `batch_program.rs` treated the merge as a failed
+    // line — misreporting an `errors[]` entry for a line whose insert
+    // had already landed — and the SAME `merge_app_state` bug would
+    // sink the whole `Batch` at HOST apply time on the five other
+    // `with_hoisted_state` producers (insert_node / replace_node /
+    // design_content / design_skeleton / batch_design), since none of
+    // them sim-validate before batching.
+    use jian_ops_schema::state::{PrimitiveType, StateEntry, StateType};
+    let mut state = sample();
+    let mut existing: BTreeMap<String, StateEntry> = BTreeMap::new();
+    existing.insert(
+        "count".into(),
+        StateEntry {
+            kind: StateType::Primitive(PrimitiveType::Int),
+            default: Some(serde_json::json!(1)),
+            description: None,
+            persist: None,
+        },
+    );
+    state.doc.state = Some(existing);
+
+    let tool = batch_design_snapshot(&state);
+    let mut args = BTreeMap::new();
+    args.insert(
+        "operations".into(),
+        r##"root=I(null, {"type":"frame","name":"Card","width":320,"height":240,"state":{"count":{"type":"int","default":1}}})"##
+            .into(),
+    );
+    let (json, cmd) = match tool.call(&args) {
+        ToolOutcome::OkJsonWithCommand(json, cmd) => (json, cmd),
+        other => panic!("expected OkJsonWithCommand, got {other:?}"),
+    };
+    // The line must not be misreported as errored — the merge is a
+    // designed no-op, not a failure.
+    assert!(
+        !json.contains("\"errors\""),
+        "a no-op state merge must not surface as a line error: {json}"
+    );
+
+    let before = state.active_children().len();
+    assert!(
+        state.apply(cmd),
+        "the outcome command must apply cleanly despite the pre-existing state key"
+    );
+    assert_eq!(
+        state.active_children().len(),
+        before + 1,
+        "the insert must land even though its declared state key was a no-op"
+    );
+    assert_eq!(
+        state
+            .doc
+            .state
+            .as_ref()
+            .unwrap()
+            .get("count")
+            .unwrap()
+            .default,
+        Some(serde_json::json!(1)),
+        "the doc-owned state entry is untouched"
+    );
+}
+
 #[test]
 fn batch_design_promotes_radio_group_role() {
     // Task D2: jian's promote table grew a `radio-group` role (D1) — a