From 56207f2d3cc55f5f0574415998ebcbaf61c90216 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 22 Oct 2026 17:50:09 +0500 Subject: [PATCH] run --script: a line's unknown key stops the run (#2256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A script line whose parameters sat under a key the runner does not read ran the command with no parameters at all, and the run still exited 0: {"command":"home.insertSheet","args":{"name":"Report"}} {"command":"home.insertSheet","name":"Report2"} made Sheet2 and Sheet3 instead of Report and Report2, and said nothing. A typo in a script produced a valid, wrong workbook. A line is read for `command` (or `id`) and `params`; any other key is a usage error now, naming the keys and the line, and the run stops before the workbook is written: gridcraft-cli: s.jsonl:2: unknown key `args`: a line is {"command": "id", "params": {…}} The usage text says what a line looks like, which is the other half of the report — the format was not written down anywhere, so the reporter found it by trying `args`, a flat object and `id` in turn. Tested at both levels: `parse_command` for the keys, their sorted list when there are several, and the forms that are read (`id`, `params`, `id={json}`, comments); and a run of a four-line script that stops at line 2 without writing the file, then the same script corrected, whose sheets come out named. Without the check the mistaken line parses again. An unknown key *inside* `params` is still ignored — the rest of #2256 — and needs the command's parameter list, which the engine does not expose yet. Noted on the issue. --- apps/gridcraft-cli/src/main.rs | 33 ++++++++++++++++++++++++++++ apps/gridcraft-cli/tests/cli.rs | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/apps/gridcraft-cli/src/main.rs b/apps/gridcraft-cli/src/main.rs index 0b763e378..972c023fa 100644 --- a/apps/gridcraft-cli/src/main.rs +++ b/apps/gridcraft-cli/src/main.rs @@ -43,6 +43,8 @@ const USAGE: &str = "usage: cached-value calc oracle + XLSX round-trip fidelity (exit 1 on any difference) gridcraft-cli run [--in FILE | --sample NAME] [--cmd 'id={json}']... [--script FILE.jsonl] [--out FILE] [--print RANGE] [--quiet] + a script line is id={json} or {\"command\": \"id\", \"params\": {…}}; + blank lines and lines starting with # or // are skipped gridcraft-cli commands [--json] [--search X] gridcraft-cli functions [--json] [--search X] [--category C] gridcraft-cli mcp [--connect PORT] [--in FILE | --sample NAME] @@ -375,6 +377,17 @@ fn parse_command(text: &str) -> Result, String> { } if t.starts_with('{') { let v: Value = serde_json::from_str(t).map_err(|e| format!("bad JSON: {e}"))?; + // A key this does not read is a mistake, not a comment: `{"command": …, "args": {…}}` and + // `{"command": …, "name": "Report"}` ran the command with no parameters at all and the + // script still exited 0, so a typo made a valid, wrong workbook (#2256). + if let Some(o) = v.as_object() { + let mut unknown: Vec<&str> = o.keys().map(String::as_str).filter(|k| !matches!(*k, "command" | "id" | "params")).collect(); + if !unknown.is_empty() { + unknown.sort_unstable(); + let (plural, list) = (if unknown.len() == 1 { "key" } else { "keys" }, unknown.join("`, `")); + return Err(format!("unknown {plural} `{list}`: a line is {{\"command\": \"id\", \"params\": {{…}}}}")); + } + } let id = v.get("command").or_else(|| v.get("id")).and_then(Value::as_str).ok_or("JSON command needs a `command` field")?; return Ok(Some((id.to_string(), v.get("params").cloned().unwrap_or_else(|| json!({}))))); } @@ -567,6 +580,26 @@ mod tests { assert_eq!(parse_command("{\"command\":\"edit.redo\"}").unwrap(), Some(("edit.redo".into(), json!({})))); assert_eq!(parse_command(" # comment").unwrap(), None); assert!(parse_command("x={bad").is_err()); + // A key this does not read is a mistake: the command used to run with no parameters and + // the script still exited 0, so a typo made a valid, wrong workbook (#2256). + for (line, named) in [ + (r#"{"command":"home.insertSheet","args":{"name":"R"}}"#, "`args`"), + (r#"{"command":"home.insertSheet","name":"R"}"#, "`name`"), + (r#"{"command":"home.insertSheet","params":{},"note":"why"}"#, "`note`"), + ] { + let e = parse_command(line).expect_err("refused"); + assert!(e.contains("unknown key") && e.contains(named), "{line}: {e}"); + assert!(e.contains(r#"{"command": "id", "params": {…}}"#), "{line}: {e}"); + } + // Several at once are listed in one message, sorted. + let e = parse_command(r#"{"command":"x","name":"R","args":{}}"#).expect_err("refused"); + assert!(e.contains("unknown keys `args`, `name`"), "{e}"); + // `id` is still the accepted alternative to `command`, and `params` still carries them. + assert_eq!(parse_command(r#"{"id":"edit.undo"}"#).unwrap(), Some(("edit.undo".into(), json!({})))); + assert_eq!( + parse_command(r#"{"command":"home.insertSheet","params":{"name":"R"}}"#).unwrap(), + Some(("home.insertSheet".into(), json!({"name": "R"}))) + ); } #[test] diff --git a/apps/gridcraft-cli/tests/cli.rs b/apps/gridcraft-cli/tests/cli.rs index 3105819ec..5295368e3 100644 --- a/apps/gridcraft-cli/tests/cli.rs +++ b/apps/gridcraft-cli/tests/cli.rs @@ -206,3 +206,41 @@ fn check_reports_a_depth_limited_scan_while_checking_readable_files() { assert!(report["files"].as_array().unwrap().iter().any(|r| r["error"].as_str().is_some_and(|e| e.contains("depth limit")))); let _ = std::fs::remove_dir_all(dir); } + +/// A script line whose parameters are under a key `run` does not read used to run the command +/// with no parameters at all, and the run still exited 0: three mistaken lines made Sheet2, +/// Sheet3 and Sheet4 instead of the named sheets, and nothing said so (#2256). +#[test] +fn a_script_line_with_an_unknown_key_stops_the_run() { + let dir = tmpdir("script-keys"); + let script = dir.join("s.jsonl"); + let out = dir.join("t.xlsx"); + let write = |lines: &[&str]| std::fs::write(&script, lines.join("\n")).unwrap(); + + write(&[r#"{"command":"file.new"}"#, r#"{"command":"home.insertSheet","args":{"name":"Report"}}"#]); + let o = run(&["run", "--script", script.to_str().unwrap(), "--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("s.jsonl:2"), "it names the line: {err}"); + assert!(err.contains("unknown key `args`"), "and the key: {err}"); + assert!(!out.exists(), "and writes no workbook: {err}"); + + // The forms that are read still work, in one script: `params`, `id=json`, and a comment. + write(&[ + r#"{"command":"file.new"}"#, + r#"{"command":"home.insertSheet","params":{"name":"Report"}}"#, + "# a comment", + r#"home.insertSheet={"name":"Report2"}"#, + ]); + let o = run(&["run", "--script", script.to_str().unwrap(), "--out", out.to_str().unwrap()]); + assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr)); + let info = ok(&["info", out.to_str().unwrap()]); + assert!(info.contains("Report:") && info.contains("Report2:"), "the sheets are named: {info}"); + assert!(!info.contains("Sheet2"), "and no sheet was added with a default name: {info}"); + + // The usage text says what a line looks like, which is how this was found. + let usage = String::from_utf8_lossy(&run(&[]).stderr).into_owned() + &String::from_utf8_lossy(&run(&["--help"]).stdout); + assert!(usage.contains(r#"{"command": "id", "params": {…}}"#), "{usage}"); + + let _ = std::fs::remove_dir_all(&dir); +}