A failed MCP batch goes back to where it started however long the undo history is: rollback undid as many steps as the undo history had grown, but the history is capped at 200 steps, so once it was full a failed batch reported undone: 0 and left its sketches behind (an empty Sketch1, two sketches named TapHole). New commands edit.checkpoint and edit.restore_checkpoint (forget: true drops one) remember and restore the design, the active sketch and the undo history; batch takes a checkpoint before its first step, restores it on a failure and forgets it on success. Against an app without checkpoints batch falls back to counting undo steps
This commit is contained in:
1 parent
54a86dc6f5
commit
ab23fc9906
6 files changed
+195
-13
No files matched your search
@@ -213,6 +213,35 @@ fn batch_stops_at_first_error_and_rolls_back() {
|
||||
assert_eq!(tool(&mut s, "measure", json!({}))["body_count"], 2);
|
||||
}
|
||||
|
||||
/// Regression: with a full undo history a failed batch reported `undone: 0` and left its
|
||||
/// sketches behind (an empty "Sketch1", two sketches named "TapHole").
|
||||
#[test]
|
||||
fn batch_rolls_back_with_a_full_undo_history() {
|
||||
let mut s = server();
|
||||
let fill: Vec<Value> =
|
||||
(0..230).map(|i| json!({"command": "parameters.add", "params": {"name": format!("p{i}"), "expression": "1 mm"}})).collect();
|
||||
tool(&mut s, "batch", json!({"commands": fill}));
|
||||
let undo = tool(&mut s, "inspect_design", json!({"measure": false}))["undo"].clone();
|
||||
for _ in 0..2 {
|
||||
let r = raw_tool(
|
||||
&mut s,
|
||||
"batch",
|
||||
json!({"commands": [
|
||||
{"command": "sketch.create", "params": {"plane": "XY", "name": "TapHole"}},
|
||||
{"command": "sketch.circle.center", "params": {"center": [0, 0], "radius": 2}},
|
||||
{"command": "sketch.dimension", "params": {"entities": ["nope"], "value": 3}},
|
||||
]}),
|
||||
);
|
||||
let body: Value = serde_json::from_str(r["result"]["content"][0]["text"].as_str().unwrap()).unwrap();
|
||||
assert_eq!(body["failed"]["index"], 2, "{body}");
|
||||
assert_eq!(body["undone"], 2, "{body}");
|
||||
}
|
||||
let d = tool(&mut s, "inspect_design", json!({"measure": false, "sketches": false}));
|
||||
assert_eq!(d["sketches"].as_array().map(Vec::len), Some(0), "{d}");
|
||||
assert!(d["active_sketch"].is_null(), "{d}");
|
||||
assert_eq!(d["undo"], undo);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn helper_sketches_can_be_hidden() {
|
||||
let mut s = server();
|
||||
|
||||
+53
-11
@@ -361,7 +361,56 @@ fn undo_depth(b: &mut dyn Backend) -> Option<u64> {
|
||||
b.call("document.inspect", json!({})).ok().and_then(|d| d["undo"].as_u64())
|
||||
}
|
||||
|
||||
/// `batch`: run steps in order, stop at the first failure and (by default) undo what ran.
|
||||
/// How a failed batch goes back to where it started.
|
||||
enum Undo {
|
||||
/// An engine checkpoint: the design, active sketch and undo history exactly as they were.
|
||||
Checkpoint(u64),
|
||||
/// An app without checkpoints: undo as many steps as the batch added (wrong once the undo
|
||||
/// history is full, as its depth no longer grows).
|
||||
Depth(u64),
|
||||
None,
|
||||
}
|
||||
|
||||
impl Undo {
|
||||
fn begin(b: &mut dyn Backend) -> Undo {
|
||||
match exec(b, "edit.checkpoint", json!({})).ok().and_then(|v| v["checkpoint"].as_u64()) {
|
||||
Some(id) => Undo::Checkpoint(id),
|
||||
None => undo_depth(b).map_or(Undo::None, Undo::Depth),
|
||||
}
|
||||
}
|
||||
|
||||
/// Go back; the number of steps undone.
|
||||
fn rollback(self, b: &mut dyn Backend, steps: usize) -> u64 {
|
||||
match self {
|
||||
Undo::Checkpoint(id) => match exec(b, "edit.restore_checkpoint", json!({"checkpoint": id})) {
|
||||
Ok(_) => steps as u64,
|
||||
Err(_) => 0,
|
||||
},
|
||||
Undo::Depth(before) => {
|
||||
let after = undo_depth(b).unwrap_or(before);
|
||||
let mut undone = 0u64;
|
||||
for _ in before..after {
|
||||
if exec(b, "edit.undo", json!({})).is_err() {
|
||||
break;
|
||||
}
|
||||
undone += 1;
|
||||
}
|
||||
undone
|
||||
}
|
||||
Undo::None => 0,
|
||||
}
|
||||
}
|
||||
|
||||
/// The batch went through: forget the checkpoint.
|
||||
fn finish(self, b: &mut dyn Backend) {
|
||||
if let Undo::Checkpoint(id) = self {
|
||||
let _ = exec(b, "edit.restore_checkpoint", json!({"checkpoint": id, "forget": true}));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// `batch`: run steps in order, stop at the first failure and (by default) go back to the
|
||||
/// design as it was before the batch.
|
||||
fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
|
||||
let Some(steps) = a.get("commands").and_then(Value::as_array) else {
|
||||
return text_result(&json!("`commands` must be an array of {command, params}"), true);
|
||||
@@ -370,7 +419,7 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
|
||||
return text_result(&json!(format!("batch too long ({} steps, at most {MAX_BATCH})", steps.len())), true);
|
||||
}
|
||||
let rollback = a.get("rollback").and_then(Value::as_bool).unwrap_or(true);
|
||||
let before = if rollback { undo_depth(b) } else { None };
|
||||
let start = if rollback { Undo::begin(b) } else { Undo::None };
|
||||
let mut results = Vec::with_capacity(steps.len());
|
||||
for (i, step) in steps.iter().enumerate() {
|
||||
let command = step.get("command").or_else(|| step.get("id")).and_then(Value::as_str);
|
||||
@@ -381,15 +430,7 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
|
||||
match r {
|
||||
Ok(v) => results.push(v),
|
||||
Err(e) => {
|
||||
let mut undone = 0u64;
|
||||
if let (Some(before), Some(after)) = (before, rollback.then(|| undo_depth(b)).flatten()) {
|
||||
for _ in before..after {
|
||||
if exec(b, "edit.undo", json!({})).is_err() {
|
||||
break;
|
||||
}
|
||||
undone += 1;
|
||||
}
|
||||
}
|
||||
let undone = start.rollback(b, i);
|
||||
return text_result(
|
||||
&json!({
|
||||
"ok": false,
|
||||
@@ -404,6 +445,7 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
|
||||
}
|
||||
}
|
||||
}
|
||||
start.finish(b);
|
||||
text_result(&json!({"ok": true, "completed": results.len(), "results": results}), false)
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user