mirror of
https://github.com/storytold/gridcraft.git
synced 2026-10-24 02:36:48 -06:00
run --script: a line's unknown key stops the run (#2256)
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.
This commit is contained in:
@@ -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<Option<(String, Value)>, 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]
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user