diff --git a/apps/gridcraft-cli/src/main.rs b/apps/gridcraft-cli/src/main.rs --- a/apps/gridcraft-cli/src/main.rs +++ b/apps/gridcraft-cli/src/main.rs @@ -443,6 +443,7 @@ fn run(args: &[String]) -> Result<(), String> { }; for (id, p, origin) in cmds { step += 1; + gridcraft_engine::check_params(&id, &p).map_err(|e| format!("step {step} ({origin}, {id}): {e}"))?; let r = exec(&mut s, &id, p).map_err(|e| format!("step {step} ({origin}, {id}): {e}"))?; if !quiet && !r.is_null() { outln!("{}", serde_json::to_string(&json!({"command": id, "result": r})).unwrap_or_default()); diff --git a/apps/gridcraft-cli/tests/it/cli.rs b/apps/gridcraft-cli/tests/it/cli.rs --- a/apps/gridcraft-cli/tests/it/cli.rs +++ b/apps/gridcraft-cli/tests/it/cli.rs @@ -242,3 +242,21 @@ fn a_script_line_with_an_unknown_key_stops_the_run() { let _ = std::fs::remove_dir_all(&dir); } + +/// A key the command does not take, inside `params`, is refused too: the third line of the report +/// in #2256 (`{"nam": "Report3"}`) made a sheet with a default name and the run exited 0. +#[test] +fn a_params_key_the_command_does_not_take_stops_the_run() { + let dir = tmpdir("params-keys"); + let out = dir.join("t.xlsx"); + let o = run(&["run", "--cmd", r#"home.insertSheet={"nam":"Report3"}"#, "--out", out.to_str().unwrap()]); + let err = String::from_utf8_lossy(&o.stderr); + assert!(!o.status.success(), "the run must stop: {err}"); + assert!(err.contains("--cmd, home.insertSheet") && err.contains("unknown key `nam`"), "it names the command and the key: {err}"); + assert!(!out.exists(), "and writes no workbook: {err}"); + + // The documented key still works, and so does a `range` for a command that lists only some keys. + ok(&["run", "--cmd", r#"home.insertSheet={"name":"Report3"}"#, "--cmd", r#"home.bold={"range":"A1","sheet":0}"#, "--out", out.to_str().unwrap()]); + assert!(ok(&["info", out.to_str().unwrap()]).contains("Report3")); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -89,6 +89,113 @@ pub fn journal_params(params: &Json) -> Json { } } +/// Params that choose where a command acts, and the alias of `mode` that every object command +/// reads. The shared helpers read them for commands whose params doc names only `range` or `mode`, +/// so [`check_params`] always lets them through. +const SHARED_KEYS: &[&str] = &["range", "cell", "sheet", "window", "doc", "anchorMode"]; + +/// Commands [`check_params`] leaves alone: they copy their params into another command or read +/// them in a way a params doc cannot list (found by listing the commands whose code uses `p` +/// other than to read a named key). +const FREE_FORM_PARAMS: &[&str] = &[ + "data.analysisToolPak", + "data.removeDuplicates", + "data.textToColumns", + "data.flashFill", + "edit.paste", + "chart.changeType", + "arrange.group", + "table.removeDuplicates", + "automate.recordActions", + "automate.runScript", + "pageLayout.themeColors", + "pageLayout.themeFonts", + "pageLayout.background", + "home.drawBorder", + "formulas.recentlyUsed", + "formulas.moreFunctions", + "file.save", + "file.saveAs", + "home.doubleUnderline", + "home.formatCells", + "formulas.selectPrecedents", + "formulas.selectDependents", + "home.formatAsTable", + "insert.recommendedCharts", + "insert.textBox", + "table.summarizePivot", + "pivot.grandTotals", + "pivot.subtotals", + "pivot.refresh", + "data.refreshAll", + "pageLayout.pageSetup", + "data.solver", + "data.solverReport", +]; + +/// The names of the params a params doc lists: the words that start a `{…}` group or follow a comma +/// directly inside one (outside quotes, brackets and parentheses), and the alternative names of +/// `input | value`. The words of a value, such as `index` in `before?: index` or `a | b` after a +/// colon, are not names. The doc's response after `→` is not input and is not read. +pub(crate) fn documented_keys(doc: &str) -> Vec<&str> { + let (mut keys, mut depth, mut nest, mut quote, mut start, mut escaped) = (Vec::new(), 0usize, 0usize, None, None, false); + let (mut expect, mut is_key, mut in_value) = (false, false, false); + for (i, c) in doc.char_indices().chain(std::iter::once((doc.len(), ' '))) { + if let Some(from) = start { + if c.is_alphanumeric() || c == '_' { + continue; + } + if is_key { + keys.extend(doc.get(from..i)); + } + start = None; + } + match (quote, c) { + (Some(_), _) if escaped => escaped = false, + (Some(_), '\\') => escaped = true, + (Some(q), c) if c == q => quote = None, + (Some(_), _) => {} + (None, '"' | '`') => quote = Some(c), + (None, '{') => { + depth += 1; + (expect, in_value) = (true, false); + } + (None, '→') => break, + (None, '}') => depth = depth.saturating_sub(1), + (None, '[' | '(') => nest += 1, + (None, ']' | ')') => nest = nest.saturating_sub(1), + (None, ',') if depth > 0 && nest == 0 => (expect, in_value) = (true, false), + (None, ':') if depth > 0 && nest == 0 => in_value = true, + (None, '|') if depth > 0 && nest == 0 && !in_value => expect = true, + (None, c) if depth > 0 && (c.is_alphabetic() || c == '_') => { + (start, is_key, expect) = (Some(i), expect && nest == 0, false); + } + _ => {} + } + } + keys +} + +/// Refuses a key in `params` that the command's params doc does not list, naming it, so a typo +/// (`{"nam": "Report"}`) is an error instead of a command that ran with its defaults (#2256). +/// For callers that take a command and its params from a script, an agent or a person: commands +/// the engine runs for itself are not checked. A command with no params doc to read, or listed in +/// [`FREE_FORM_PARAMS`], accepts anything. +pub fn check_params(id: &str, params: &Json) -> Result<()> { + let (Some(spec), Some(given)) = (find_command(id), params.as_object()) else { return Ok(()) }; + if FREE_FORM_PARAMS.contains(&id) || !spec.params.contains('{') { + return Ok(()); + } + let known = documented_keys(spec.params); + let mut unknown: Vec<&str> = given.keys().map(String::as_str).filter(|k| !SHARED_KEYS.contains(k) && !known.contains(k)).collect(); + if unknown.is_empty() { + return Ok(()); + } + unknown.sort_unstable(); + let (plural, list) = (if unknown.len() == 1 { "key" } else { "keys" }, unknown.join("`, `")); + Err(bad(id, format!("unknown {plural} `{list}` (takes {})", spec.params))) +} + pub fn always(_: &Session) -> std::result::Result<(), String> { Ok(()) } diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -37,7 +37,15 @@ use selection::{select_all, select_col, select_region, select_row, selection_mov pub fn specs() -> Vec { vec![ - cmd!("cell.set", "Enter Cell", [], None, "{cell?: \"B2\", input: \"text, number or =formula\", sheet?, array?: bool}", has_doc, cell_set), + cmd!( + "cell.set", + "Enter Cell", + [], + None, + "{cell?: \"B2\", input | value: \"text, number or =formula\", sheet?, array?: bool}", + has_doc, + cell_set + ), cmd!( "range.setValues", "Set Values", @@ -83,7 +91,7 @@ pub fn specs() -> Vec { "Paste Special…", ["Home", "Clipboard"], Some("Ctrl+Cmd+V"), - "{what: all|formulas|values|formats|comments|validation|allExceptBorders|columnWidths|formulasAndNumberFormats|valuesAndNumberFormats, operation?: none|add|subtract|multiply|divide, skipBlanks?, transpose?, link?}", + "{what: all|formulas|values|formats|comments|validation|allExceptBorders|columnWidths|formulasAndNumberFormats|valuesAndNumberFormats, operation?: none|add|subtract|multiply|divide, skipBlanks?, transpose?, link?, at?: \"C3\"}", has_clipboard, paste_special ), diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -200,7 +200,7 @@ pub fn specs() -> Vec { "Get Data", ["Data", "Get & Transform Data"], None, - "{path | text} (CSV/TSV/JSON array of objects) → new sheet table", + "{path | text, delimiter?, sheet?} (CSV/TSV/JSON array of objects) → new sheet table", has_doc, from_csv ), diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -87,10 +87,10 @@ pub fn specs() -> Vec { format_cells ), // Alignment - cmd!("home.alignLeft", "Align Left", ["Home", "Alignment"], Some("Cmd+L"), "{range?}", has_doc, |s, p| halign(s, p, HAlign::Left)), - cmd!("home.alignCenter", "Center", ["Home", "Alignment"], Some("Cmd+E"), "{range?}", has_doc, |s, p| halign(s, p, HAlign::Center)), - cmd!("home.alignRight", "Align Right", ["Home", "Alignment"], None, "{range?}", has_doc, |s, p| halign(s, p, HAlign::Right)), - cmd!("home.justify", "Justify", ["Home", "Alignment"], None, "{range?}", has_doc, |s, p| halign(s, p, HAlign::Justify)), + cmd!("home.alignLeft", "Align Left", ["Home", "Alignment"], Some("Cmd+L"), "{range?, on?}", has_doc, |s, p| halign(s, p, HAlign::Left)), + cmd!("home.alignCenter", "Center", ["Home", "Alignment"], Some("Cmd+E"), "{range?, on?}", has_doc, |s, p| halign(s, p, HAlign::Center)), + cmd!("home.alignRight", "Align Right", ["Home", "Alignment"], None, "{range?, on?}", has_doc, |s, p| halign(s, p, HAlign::Right)), + cmd!("home.justify", "Justify", ["Home", "Alignment"], None, "{range?, on?}", has_doc, |s, p| halign(s, p, HAlign::Justify)), cmd!("home.alignTop", "Top Align", ["Home", "Alignment"], None, "{range?}", has_doc, |s, p| valign(s, p, VAlign::Top)), cmd!("home.alignMiddle", "Middle Align", ["Home", "Alignment"], None, "{range?}", has_doc, |s, p| valign(s, p, VAlign::Center)), cmd!("home.alignBottom", "Bottom Align", ["Home", "Alignment"], None, "{range?}", has_doc, |s, p| valign(s, p, VAlign::Bottom)), @@ -588,10 +588,19 @@ pub(crate) fn merge_json(mut base: Json, patch: &Json) -> Json { } fn halign(s: &mut Session, p: &Json, a: HAlign) -> Result { - // Clicking the active alignment again returns to General. + // Clicking the active alignment again returns to General; `on: false` does so only while it is the active one. let cur = active_style(s)?.align.h; - let a = if cur == a && p.get("on").is_none() { HAlign::General } else { a }; - apply_style(s, p, |st| st.align.h = a) + let toggled = if cur == a { HAlign::General } else { a }; + match p.get("on").map(Json::as_bool) { + None => apply_style(s, p, |st| st.align.h = toggled), + // Clear only the cells that already have `a`, and only while the active cell has it too. + Some(Some(false)) => apply_style(s, p, |st| { + if cur == a && st.align.h == a { + st.align.h = HAlign::General; + } + }), + Some(_) => apply_style(s, p, |st| st.align.h = a), + } } fn valign(s: &mut Session, p: &Json, a: VAlign) -> Result { diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -177,7 +177,7 @@ pub fn specs() -> Vec { "Open Hyperlink", [], None, - "{url?: \"https://…\" | \"mailto:…\" | \"Sheet2!A1\", cell?} (default: the cell's link); only http, https and mailto are opened, any other scheme is an error; the UI asks before opening", + "{url?: \"https://…\" | \"mailto:…\" | \"Sheet2!A1\", target? (alias of url), cell?} (default: the cell's link); only http, https and mailto are opened, any other scheme is an error; the UI asks before opening", has_doc, open_link ), diff --git a/crates/engine/src/cmd/pivot.rs b/crates/engine/src/cmd/pivot.rs --- a/crates/engine/src/cmd/pivot.rs +++ b/crates/engine/src/cmd/pivot.rs @@ -32,7 +32,7 @@ pub fn specs() -> Vec { "PivotTable", ["Insert", "Tables"], None, - "{source?: \"Sheet1!$A$1:$E$200\" | table name (default: the table or current region of the selection), destination?: \"new\" (default: new sheet before the current one, report at A3) | \"Sheet1!H3\", name?, rows?: [field | {field, group?: years|quarters|months|days}], columns?: [..], values?: [field | {field, func?, name?}], filters?: [field], replace?: bool} → {id, name, sheet, anchor, fields}", + "{source?: \"Sheet1!$A$1:$E$200\" | table name (default: the table or current region of the selection), destination?: \"new\" (default: new sheet before the current one, report at A3) | \"Sheet1!H3\", name?, rows?: [field | {field, group?: years|quarters|months|days}], columns?: [..], values?: [field | {field, func?, name?}], filters?: [field], replace?: bool, location? (alias of destination)} → {id, name, sheet, anchor, fields}", has_doc, insert_pivot ), diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -14,7 +14,7 @@ pub fn specs() -> Vec { "Protect Sheet…", ["Review", "Protect"], None, - "{password?, formatCells?, formatColumns?, formatRows?, insertRows?, insertColumns?, deleteRows?, deleteColumns?, sort?, autofilter?}", + "{password?, formatCells?, formatColumns?, formatRows?, insertRows?, insertColumns?, deleteRows?, deleteColumns?, sort?, autofilter?, selectLockedCells?, selectUnlockedCells?}", has_doc, protect_sheet ), diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -27,22 +27,20 @@ pub fn specs() -> Vec { has_doc, |s, p| insert_lines(s, p, Axis::Rows) ), - cmd!("home.insertColumns", "Insert Sheet Columns", ["Home", "Cells", "Insert"], None, "{cols?: \"B:C\"}", has_doc, |s, p| insert_lines( - s, - p, - Axis::Cols - )), - cmd!("home.deleteRows", "Delete Sheet Rows", ["Home", "Cells", "Delete"], Some("Cmd+-"), "{rows?}", has_doc, |s, p| delete_lines( + cmd!("home.insertColumns", "Insert Sheet Columns", ["Home", "Cells", "Insert"], None, "{cols?: \"B:C\", count?}", has_doc, |s, p| { + insert_lines(s, p, Axis::Cols) + }), + cmd!("home.deleteRows", "Delete Sheet Rows", ["Home", "Cells", "Delete"], Some("Cmd+-"), "{rows?, count?}", has_doc, |s, p| delete_lines( s, p, Axis::Rows )), - cmd!("home.deleteColumns", "Delete Sheet Columns", ["Home", "Cells", "Delete"], None, "{cols?}", has_doc, |s, p| delete_lines( + cmd!("home.deleteColumns", "Delete Sheet Columns", ["Home", "Cells", "Delete"], None, "{cols?, count?}", has_doc, |s, p| delete_lines( s, p, Axis::Cols )), - cmd!("sheet.moveColumns", "Move Columns", [], None, "{cols?: \"B:C\", before: zero-based column index}", has_doc, move_columns), + cmd!("sheet.moveColumns", "Move Columns", [], None, "{cols?: \"B:C\", before: zero-based column index, count?}", has_doc, move_columns), cmd!("home.insertCells", "Insert Cells…", ["Home", "Cells", "Insert"], None, "{range?, shift: right|down|row|column}", has_doc, insert_cells), cmd!( "home.insertCopiedCells", diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -33,7 +33,7 @@ use gridcraft_model::Workbook; use serde::Serialize; use serde_json::Value as Json; -pub use cmd::{CommandInfo, CommandSpec, command_specs, find_command, register_test_command}; +pub use cmd::{CommandInfo, CommandSpec, check_params, command_specs, find_command, register_test_command}; pub use gridcraft_calc as calc; pub use gridcraft_core as core; pub use gridcraft_formula as formula; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -5117,3 +5117,78 @@ fn a_saved_workbook_keeps_its_values_until_an_edit_reaches_them() { assert_eq!(v(&saved, "B1"), Value::Number(6.0)); assert_eq!(v(&saved, "C1"), Value::Number(7.0)); } + +/// A key a command's params doc does not list is refused by name, so a typo is not a command that +/// ran with its defaults (#2256); the keys it lists, and the ones that choose where it acts, pass. +#[test] +fn check_params_names_the_keys_a_command_does_not_take() { + use crate::cmd::check_params; + let err = |id: &str, params: serde_json::Value| check_params(id, ¶ms).unwrap_err().to_string(); + let e = err("home.insertSheet", json!({"nam": "Report3"})); + assert!(e.contains("unknown key `nam`") && e.contains("{name?, before?: index}"), "{e}"); + let e = err("home.insertSheet", json!({"name": "R", "colour": 1, "args": {}})); + assert!(e.contains("unknown keys `args`, `colour`"), "{e}"); + for (id, params) in [ + ("home.insertSheet", json!({"name": "R", "before": 0})), + ("home.insertSheet", json!({})), + ("cell.set", json!({"cell": "A1", "input": "1", "sheet": 0})), + ("home.bold", json!({"range": "A1", "sheet": "Sheet1", "on": true})), + ("home.alignLeft", json!({"range": "A1", "on": true})), + ("home.insertRows", json!({"rows": "2:3", "count": 2})), + ("home.deleteColumns", json!({"cols": "B", "count": 2})), + ("file.open", json!({"base64": "", "name": "a.csv", "csvAsText": true})), + ("home.insertSheet", json!({"window": 1, "doc": 1})), + // Commands with free-form params, ids that are not engine commands and a params value + // that is not an object are left to the command. + ("insert.textBox", json!({"fill": "red"})), + ("ui.dialog", json!({"anything": 1})), + ("home.insertSheet", serde_json::Value::Null), + ] { + check_params(id, ¶ms).unwrap_or_else(|e| panic!("{id} {params}: {e}")); + } +} + +/// The keys are the names a params doc lists inside its braces, not the words of their values. +#[test] +fn documented_keys_reads_the_words_in_braces() { + use crate::cmd::documented_keys; + assert_eq!(documented_keys(r#"{a?: "skip me", b_2?: bool} → {c} then prose"#), ["a", "b_2"]); + // `|` between names gives alternatives; after a colon it separates the words of a value. + assert_eq!(documented_keys("{input | value: \"text\", mode?: a|b, path? | base64?: \"..\"}"), ["input", "value", "mode", "path", "base64"]); + assert_eq!(documented_keys("{name?, before?: index}"), ["name", "before"]); + assert_eq!(documented_keys("{v: [[1, two], [3]], w?}"), ["v", "w"]); + assert_eq!(documented_keys("{} no params"), Vec::<&str>::new()); + assert_eq!(documented_keys("{x?: `not this`}"), ["x"]); + // An escaped quote does not end the quoted text: the keys after it still count. + assert_eq!(documented_keys(r#"{q?: "\"", after?}"#), ["q", "after"]); + for id in ["cell.set", "insert.picture"] { + let key = if id == "cell.set" { "value" } else { "base64" }; + assert!(documented_keys(crate::cmd::find_command(id).map_or("", |c| c.params)).contains(&key), "{id}"); + } + // Every key a command's doc lists passes, whatever the doc's shape. + for spec in crate::cmd::command_specs() { + let params: serde_json::Map = + documented_keys(spec.params).into_iter().map(|k| (k.to_string(), json!(null))).collect(); + crate::cmd::check_params(spec.id, &serde_json::Value::Object(params)).unwrap_or_else(|e| panic!("{}: {e}", spec.id)); + } +} + +#[test] +fn align_on_false_clears_only_the_active_alignment() { + let mut s = Session::new(); + s.new_workbook(); + let h = |s: &mut Session| s.execute("cell.get", json!({"cell": "A1"})).unwrap()["style"]["align"]["h"].clone(); + s.execute("home.alignLeft", json!({"range": "A1", "on": true})).unwrap(); + assert_eq!(h(&mut s), json!("Left")); + s.execute("home.alignRight", json!({"range": "A1", "on": false})).unwrap(); + assert_eq!(h(&mut s), json!("Left")); + s.execute("home.alignLeft", json!({"range": "A1", "on": false})).unwrap(); + assert_eq!(h(&mut s), json!("General")); +} + +#[test] +fn paste_special_and_insert_sheet_reject_a_misspelled_key() { + assert!(crate::cmd::check_params("edit.pasteSpecial", &json!({"waht": "values"})).is_err()); + assert!(crate::cmd::check_params("edit.pasteSpecial", &json!({"what": "values", "at": "C3"})).is_ok()); + assert!(crate::cmd::check_params("home.insertSheet", &json!({"index": 0})).is_err()); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -631,6 +631,7 @@ fn dispatch(b: &mut dyn Backend, name: &str, a: &Args) -> Result Some(v @ Value::Object(_)) => v.clone(), Some(_) => return Err("`params` must be an object".into()), }; + gridcraft_engine::check_params(cmd, &p).map_err(|e| e.to_string())?; exec(b, cmd, p) } "list_commands" => b.call("engine.commands", pick(a, &["search"])), diff --git a/docs/cli.md b/docs/cli.md --- a/docs/cli.md +++ b/docs/cli.md @@ -48,6 +48,8 @@ The first failing step stops the run with a non-zero exit. A `--cmd` is `id`, `id={json}` or `id {json}`. A script has one command per line in the same forms, or as JSON objects `{"command": "cell.set", "params": {...}}`; blank lines and `#` comments are skipped. +A key the line or the command does not take is an error naming it (``unknown key `nam` (takes {name?, before?: index})``), not +a command that ran with its defaults; `gridcraft-cli commands --search X` shows what each command takes. ```sh gridcraft-cli run --in book.xlsx \