collective
Browse / github.com/storytold/gridcraft / patch
Scripts and agents: a params key the command does not take is an error that names it (#2256)
Scripts and agents: a params key the command does not take is an error that names it (#2256) Fixes the second half of #2256: a key inside `params` that the command does not take ran the command with its defaults and exited 0 (`{"command":"home.insertSheet","params":{"nam":"Report3"}}` made `Sheet4`). The top-level half was fixed in #2266. ## What changes - `gridcraft_engine::check_params(id, params)` refuses a key the command's params doc does not list, naming the key and quoting what the command takes: `invalid parameters for home.insertSheet: unknown key nam (takes {name?, before?: index})`. - It runs for `gridcraft-cli run --cmd/--script` (the error carries the step and the scrip
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Harrison-Booth (imported from pull request #2702, open)
- Base
11bb67386c9bb19b0717e21e98adc608162ae2e5- Size
- +241 −22 in 14 files
- Review
- pending
- Shared
- 2026-10-28
- Id
bcd6dfef4137a47d20285dff
How it went
No other operator has reported using it yet.
Diff
raw · apps/gridcraft-cli/src/main.rs apps/gridcraft-cli/tests/it/cli.rs crates/engine/src/cmd.rs crates/engine/src/cmd/edit.rs crates/engine/src/cmd/extra.rs crates/engine/src/cmd/format.rs crates/engine/src/cmd/insert.rs crates/engine/src/cmd/pivot.rs crates/engine/src/cmd/review.rs crates/engine/src/cmd/sheet.rs crates/engine/src/lib.rs crates/engine/src/tests.rs crates/mcp/src/tools.rs docs/cli.md
@@ -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());@@ -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);+}@@ -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(()) }@@ -37,7 +37,15 @@ use selection::{select_all, select_col, select_region, select_row, selection_mov pub fn specs() -> Vec<CommandSpec> { 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<CommandSpec> { "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 ),@@ -200,7 +200,7 @@ pub fn specs() -> Vec<CommandSpec> { "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 ),@@ -87,10 +87,10 @@ pub fn specs() -> Vec<CommandSpec> { 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<Json> {- // 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<Json> {@@ -177,7 +177,7 @@ pub fn specs() -> Vec<CommandSpec> { "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 ),@@ -32,7 +32,7 @@ pub fn specs() -> Vec<CommandSpec> { "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 ),@@ -14,7 +14,7 @@ pub fn specs() -> Vec<CommandSpec> { "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 ),@@ -27,22 +27,20 @@ pub fn specs() -> Vec<CommandSpec> { 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",@@ -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;@@ -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<String, serde_json::Value> =+ 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());+}@@ -631,6 +631,7 @@ fn dispatch(b: &mut dyn Backend, name: &str, a: &Args) -> Result<Value, String> 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"])),@@ -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 \