From 1191a1df52fc6e0003b41f347521cdb35add46c4 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 5 Nov 2026 05:35:51 -0800 Subject: [PATCH 1/3] Reject ambiguous table targets --- crates/engine/src/cmd/table/registry.rs | 2 +- crates/engine/src/cmd/table/target.rs | 25 ++++++++-- crates/engine/src/tests/table.rs | 63 +++++++++++++++++++++++++ crates/ui-egui/src/panels/table.rs | 11 +++-- docs/mcp.md | 6 +++ 5 files changed, 96 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/table/registry.rs b/crates/engine/src/cmd/table/registry.rs index 0a8166f89..aca8b2cd4 100644 --- a/crates/engine/src/cmd/table/registry.rs +++ b/crates/engine/src/cmd/table/registry.rs @@ -407,6 +407,6 @@ fn table_specs() -> Vec { in_cell, |s, _| toggle_selection(s) ), - cmd!(query "table.get", "Get Table", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} → rows, columns, cells (text, spans, fill), options", in_table_or_ids, get), + cmd!(query "table.get", "Get Table", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} → rows, columns, cells (text, spans, fill), options. Supplied `story` and `table` values must be nonnegative integer IDs. When a table ID occurs in multiple stories, supply `story` with `table`.", in_table_or_ids, get), ] } diff --git a/crates/engine/src/cmd/table/target.rs b/crates/engine/src/cmd/table/target.rs index 2e1784560..9f3d08bfc 100644 --- a/crates/engine/src/cmd/table/target.rs +++ b/crates/engine/src/cmd/table/target.rs @@ -41,8 +41,20 @@ pub(super) struct Tgt { pub(super) range: CellRange, } -fn find_table(doc: &Document, id: u64) -> Option { - doc.stories.iter().find(|(_, s)| s.tables.contains_key(&id)).map(|(k, _)| *k) +fn find_table(doc: &Document, id: u64, cmd: &str) -> Result { + let mut stories = doc.stories.iter().filter(|(_, story)| story.tables.contains_key(&id)).map(|(sid, _)| *sid); + let first = stories.next().ok_or_else(|| bad(cmd, format!("no table {id}")))?; + if stories.next().is_some() { + return Err(bad(cmd, format!("table {id} exists in multiple stories; specify `story`"))); + } + Ok(first) +} + +fn id_param(p: &Value, key: &str, cmd: &str) -> Result> { + match p.get(key) { + None => Ok(None), + Some(value) => value.as_u64().map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be a nonnegative integer"))), + } } fn pair(p: &Value, many: &str, one: &str) -> Option<(usize, usize)> { @@ -57,10 +69,13 @@ fn pair(p: &Value, many: &str, one: &str) -> Option<(usize, usize)> { pub(super) fn target(s: &Session, p: &Value, cmd: &str) -> Result { let st = s.doc()?; let doc = &st.doc; - let explicit_table = p.get("table").and_then(Value::as_u64); - let explicit_story = p.get("story").and_then(Value::as_u64).map(StoryId); + let explicit_table = id_param(p, "table", cmd)?; + let explicit_story = id_param(p, "story", cmd)?.map(StoryId); let (story, table, range) = if let Some(tid) = explicit_table { - let sid = explicit_story.or_else(|| find_table(doc, tid)).ok_or_else(|| bad(cmd, format!("no table {tid}")))?; + let sid = match explicit_story { + Some(sid) => sid, + None => find_table(doc, tid, cmd)?, + }; (sid, tid, None) } else if let Some(c) = st.selection.cells { (c.story, c.table, Some(c.range)) diff --git a/crates/engine/src/tests/table.rs b/crates/engine/src/tests/table.rs index 77bcb9bfd..3ccc66789 100644 --- a/crates/engine/src/tests/table.rs +++ b/crates/engine/src/tests/table.rs @@ -17,6 +17,69 @@ fn check(s: &Session) { s.doc().unwrap().doc.check().unwrap(); } +#[test] +fn table_id_without_story_rejects_multiple_matching_stories() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let first = s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "text"})).unwrap(); + let first_story = first["story"].as_u64().unwrap(); + let shared_id = s.execute("table.insert", &json!({"rows": 1, "cols": 1})).unwrap()["table"].as_u64().unwrap(); + let second = s.execute("frame.create", &json!({"rect": [320, 36, 580, 200], "content": "text"})).unwrap(); + let second_story = second["story"].as_u64().unwrap(); + let second_id = s.execute("table.insert", &json!({"rows": 2, "cols": 1})).unwrap()["table"].as_u64().unwrap(); + s.edit(|d, _| { + let story = d.story_mut(doc::StoryId(second_story)).unwrap(); + let mut table = (*story.tables.remove(&second_id).unwrap()).clone(); + table.id = shared_id; + story.tables.insert(shared_id, std::sync::Arc::new(table)); + for para in &mut story.paras { + if para.table == Some(second_id) { + para.table = Some(shared_id); + } + } + Ok(()) + }) + .unwrap(); + check(&s); + + let error = s.execute("table.get", &json!({"table": shared_id})).unwrap_err(); + assert!(error.to_string().contains("multiple stories"), "{error}"); + assert!(error.to_string().contains("story"), "{error}"); + assert_eq!(s.execute("table.get", &json!({"story": first_story, "table": shared_id})).unwrap()["rows"].as_array().unwrap().len(), 1); + assert_eq!(s.execute("table.get", &json!({"story": second_story, "table": shared_id})).unwrap()["rows"].as_array().unwrap().len(), 2); +} + +#[test] +fn table_target_rejects_wrong_typed_story_and_table_ids() { + let (mut s, story, _) = session_with_frame(); + let table = s.execute("table.insert", &json!({"rows": 1, "cols": 1})).unwrap()["table"].as_u64().unwrap(); + for (key, value) in [ + ("story", json!("other")), + ("story", json!("")), + ("story", json!(-1)), + ("story", json!(1.5)), + ("story", Value::Null), + ("table", json!("other")), + ("table", json!("")), + ("table", json!(-1)), + ("table", json!(1.5)), + ("table", Value::Null), + ] { + let mut params = json!({"story": story, "table": table}); + params[key] = value; + let error = s.execute("table.get", ¶ms).unwrap_err(); + assert!(error.to_string().contains(&format!("`{key}`")), "{key}: {error}"); + } + let undo = s.doc().unwrap().history.undo.len(); + let error = s.execute("table.setCell", &json!({"story": null, "table": table, "insets": 7})).unwrap_err(); + assert!(error.to_string().contains("`story`"), "{error}"); + assert_eq!(s.doc().unwrap().history.undo.len(), undo); + for params in [json!({"story": story, "table": u64::MAX}), json!({"story": u64::MAX, "table": table})] { + let error = s.execute("table.get", ¶ms).unwrap_err(); + assert!(error.to_string().contains("no table"), "{error}"); + } +} + #[test] fn table_text_replacement_floors_utf8_selection_and_allows_quote_typing() { for (original, (a, b), replacement, (after_a, after_b), quoted) in [ diff --git a/crates/ui-egui/src/panels/table.rs b/crates/ui-egui/src/panels/table.rs index 90cf274ea..ef99bcce3 100644 --- a/crates/ui-egui/src/panels/table.rs +++ b/crates/ui-egui/src/panels/table.rs @@ -26,6 +26,7 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { let cols = info["columns"].as_array().cloned().unwrap_or_default(); let range = info["range"].clone(); let (r0, c0) = (range["r0"].as_u64().unwrap_or(0) as usize, range["c0"].as_u64().unwrap_or(0) as usize); + let sid = info["story"].clone(); let tid = info["table"].clone(); let cell = info["cells"].as_array().and_then(|cs| cs.iter().find(|c| c["row"] == r0 && c["col"] == c0)).cloned().unwrap_or(Value::Null); let run = |app: &mut DesignApp, id: &str, p: Value| { @@ -72,7 +73,7 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { let combo = craft_ui::ComboBox::from_id_salt(("tp_style", kind)).selected_text(&cur).width(130.0).show_ui(ui, |ui| { for n in &names { if ui.selectable_label(*n == cur, n).clicked() { - run(app, &format!("style.{kind}.apply"), json!({"name": n, "table": tid})); + run(app, &format!("style.{kind}.apply"), json!({"name": n, "story": sid, "table": tid})); } } }); @@ -103,10 +104,10 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { run( app, "table.insertRow", - json!({"table": tid, "rows": [rows.len() - 1, rows.len() - 1], "where": "below", "count": n - rows.len()}), + json!({"story": sid, "table": tid, "rows": [rows.len() - 1, rows.len() - 1], "where": "below", "count": n - rows.len()}), ); } else if n < rows.len() { - run(app, "table.deleteRow", json!({"table": tid, "rows": [n, rows.len() - 1]})); + run(app, "table.deleteRow", json!({"story": sid, "table": tid, "rows": [n, rows.len() - 1]})); } } caption(ui, crate::i18n::tr_context(&app.ui.language, "Columns", "table")); @@ -116,10 +117,10 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { run( app, "table.insertColumn", - json!({"table": tid, "cols": [cols.len() - 1, cols.len() - 1], "where": "right", "count": n - cols.len()}), + json!({"story": sid, "table": tid, "cols": [cols.len() - 1, cols.len() - 1], "where": "right", "count": n - cols.len()}), ); } else if n < cols.len() { - run(app, "table.deleteColumn", json!({"table": tid, "cols": [n, cols.len() - 1]})); + run(app, "table.deleteColumn", json!({"story": sid, "table": tid, "cols": [n, cols.len() - 1]})); } } ui.end_row(); diff --git a/docs/mcp.md b/docs/mcp.md index 61e59df4d..365d2bd9f 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -185,6 +185,12 @@ In connected mode every tool maps to a control-channel method (`engine.execute`, what the agent does shows up live in the window. A failed request is not resent: the operation may have completed, so inspect the document before retrying. The next request re-establishes the connection. +## Table targets + +Table IDs can repeat across stories. For `execute` commands that target an existing table, supply `story` with `table` +when an ID repeats. An ambiguous table-only request returns an error. Supplied `story` and `table` values must be +nonnegative integers; `null` and values of other types return an error. + ## Coordinates Points (1/72 in), y down, in **spread** space: on a single-page spread the page's top-left is (0, 0); on a facing From 00d7f0e1910cfd7b8c331576e68a40b9b95b4a89 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:03:44 +0000 Subject: [PATCH 2/3] Apply review fixes - crates/ui-egui/src/panels/table.rs --- crates/ui-egui/src/panels/table.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ui-egui/src/panels/table.rs b/crates/ui-egui/src/panels/table.rs index ef99bcce3..6d2213ba4 100644 --- a/crates/ui-egui/src/panels/table.rs +++ b/crates/ui-egui/src/panels/table.rs @@ -73,7 +73,7 @@ pub fn show(app: &mut DesignApp, ui: &mut egui::Ui) { let combo = craft_ui::ComboBox::from_id_salt(("tp_style", kind)).selected_text(&cur).width(130.0).show_ui(ui, |ui| { for n in &names { if ui.selectable_label(*n == cur, n).clicked() { - run(app, &format!("style.{kind}.apply"), json!({"name": n, "story": sid, "table": tid})); + run(app, &format!("style.{kind}.apply"), json!({"name": n, "story": sid, "table": tid, "rows": [range["r0"], range["r1"]], "cols": [range["c0"], range["c1"]]})); } } }); From 9f4bc29c3e9e654f67463bc63b6e61e40f4f9c53 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 13:31:04 -0800 Subject: [PATCH 3/3] Tables: a malformed table id is refused with nothing in a table selected With nothing in a table selected, a table command given a `table` that is not a nonnegative integer (`null`, a string, a negative number) was refused as disabled, since the enablement check read it as no table. It is now refused with "`table` must be a nonnegative integer", as it is with a table selected. --- crates/engine/src/cmd.rs | 12 ++++++++---- crates/engine/src/lib.rs | 4 ++-- crates/engine/src/tests/table.rs | 9 +++++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index dc1acaf15..b9bc2b9c2 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -332,12 +332,16 @@ pub(crate) fn named_targets(spec: &CommandSpec, p: &Value) -> Option } /// The table a call names with `table`, for a command whose params document an optional `table`. -/// `None` when the call names no table. [`table_story`] reads the `story` that goes with it. -pub(crate) fn named_table(spec: &CommandSpec, p: &Value) -> Option { +/// `None` when the call names no table. A `table` that is present but not a non-negative integer, +/// `null` included, is an error. [`table_story`] reads the `story` that goes with it. +pub(crate) fn named_table(spec: &CommandSpec, p: &Value, cmd: &str) -> Result> { if !spec.params.split(|c: char| !c.is_ascii_alphanumeric() && c != '?').any(|w| w == "table?") { - return None; + return Ok(None); + } + match p.get("table") { + None => Ok(None), + Some(v) => v.as_u64().map(Some).ok_or_else(|| bad(cmd, "`table` must be a nonnegative integer")), } - p.get("table").and_then(Value::as_u64) } /// The `story` a table command's call names alongside `table`, or `None` when the call has no diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 3d43a80e0..716068c89 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -686,8 +686,8 @@ impl Session { // A table command that takes `story` and `table` acts on the table the call names, so // the selection doesn't matter: with nothing selected, the caret outside a table, or // selected rows the command can't act on, `{"table": 3}` still runs. - if let Some(table) = cmd::named_table(spec, params) - && cmd::has_doc(self).is_ok() + if cmd::has_doc(self).is_ok() + && let Some(table) = cmd::named_table(spec, params, id)? { let story = cmd::table_story(params, id)?; let st = self.doc()?; diff --git a/crates/engine/src/tests/table.rs b/crates/engine/src/tests/table.rs index 2546fa667..7626005f4 100644 --- a/crates/engine/src/tests/table.rs +++ b/crates/engine/src/tests/table.rs @@ -78,6 +78,15 @@ fn table_target_rejects_wrong_typed_story_and_table_ids() { let error = s.execute("table.get", ¶ms).unwrap_err(); assert!(error.to_string().contains("no table"), "{error}"); } + // With nothing in a table selected, a malformed `table` is refused as such, not as disabled. + s.execute("edit.deselectAll", &json!({})).unwrap(); + for id in ["table.selectTable", "table.insertRow"] { + for value in [json!("other"), json!(-1), Value::Null] { + let error = s.execute(id, &json!({ "table": value })).unwrap_err(); + assert!(!matches!(error, crate::EngineError::Disabled(..)) && error.to_string().contains("`table`"), "{id} {value}: {error}"); + } + } + assert_eq!(s.doc().unwrap().history.undo.len(), undo); } #[test]