From 8901f7209fbb4289a513bf0871c32e3e189775b2 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Fri, 3 Jul 2026 23:14:00 +0800 Subject: [PATCH] fix(editor): treat a no-op MergeAppState as success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/command_app_state_tests.rs | 76 +++++++++++++++++-- crates/op-editor-core/src/command_apply.rs | 55 +++++++++++--- crates/op-mcp/src/batch_design_tests.rs | 70 +++++++++++++++++ 3 files changed, 185 insertions(+), 16 deletions(-) diff --git a/crates/op-editor-core/src/command_app_state_tests.rs b/crates/op-editor-core/src/command_app_state_tests.rs index 00fa5c392..1fe895933 100644 --- a/crates/op-editor-core/src/command_app_state_tests.rs +++ b/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 = 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 diff --git a/crates/op-editor-core/src/command_apply.rs b/crates/op-editor-core/src/command_apply.rs index e7bdc2910..983550db8 100644 --- a/crates/op-editor-core/src/command_apply.rs +++ b/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, ) -> 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 } } diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index 7d5fcc9e0..6abd2bdbb 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/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 = 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