From fbe83fcc57cbcde3b8a9c636cfaf1e2142f57f69 Mon Sep 17 00:00:00 2001 From: "Claude (agent-headless-import)" Date: Thu, 24 Sep 2026 23:27:45 -0700 Subject: [PATCH] feat(import): --strict refuses a skipped record, and the headless suite covers the reviewed gaps (improvise-8jk6.6, improvise-8jk6.17) - 8jk6.6, the user's ruling on PR #321 ("Add a --strict flag"): the default stays exit 0 with the counted warning. With --strict, any skipped record is a refusal that exits 1 and writes nothing. The check runs before the build and reads ImportPipeline::skip_warning, the same function the build's warning now comes from. - 8jk6.17: the headless suite now sees three faults that only io unit tests caught at efaa5050: - naming only the first bad formula (two bad --formula flags); - dropping CSV decimal measures; - import-json reading a CSV through the JSON constructor. - Docs record the user's three answers. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/cli.md | 7 ++- context/code/formula-and-io.md | 24 ++++++-- context/test-evidence/cli.md | 49 ++++++++++++++-- context/test-evidence/formula-and-io.md | 2 +- crates/improvise-io/src/import/wizard.rs | 13 +++-- src/main.rs | 10 ++++ tests/headless_cli.rs | 72 ++++++++++++++++++++++++ 7 files changed, 159 insertions(+), 18 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index c012412e..a38299bb 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -414,9 +414,10 @@ keep". The CLI's contract: - **An import that kept no record is a refusal.** It exits non-zero, writes nothing, and says why: every record was skipped as not a JSON object, or the source had none. -- **An import that skipped *some* records exits 0.** It writes the model and - warns with a count. That is a decision put to the user, not a settled - contract. The alternatives and the argument are in `formula-and-io.md`. +- **An import that skipped *some* records exits 0 by default.** It writes the + model and warns with a count. With `--strict` (the user's ruling on PR #321) + any skipped record is a refusal instead: exit 1, the same count named, and + nothing written. The argument is in `formula-and-io.md`. Each refusal comes from `build_model_with_warnings` as an `Err`, which `main` already turns into exit 1 before `persistence::save`. So "writes diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 321ca347..b8d09940 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -715,8 +715,8 @@ CSVs are **all** header-only has no records, only sources. Built from `from_merge`'s seeded proposals alone, it would save `File` items, empty columns and one empty records view per file. It is refused here, because it kept no record. A header-only file *beside* files with rows is unaffected, -since some record was kept. Whether an all-header-only folder should instead -save its structure is a question for the user on the PR. +since some record was kept. The user confirmed this on PR #321: "Refuse (as +#321 does)". **A partial skip stays a warning.** Some records kept and some skipped still builds, exits 0, and warns with a count: `Skipped 1 of 3 records that were not @@ -726,9 +726,23 @@ trailing `null` or a stray scalar in a hand-edited file. Every record that *is* a record was kept, and the count is on stderr for a script to read. Making it fatal would fail every import that carries one stray element. Making it a distinct non-zero exit that still writes gives a contract where -`import … && next` stops even though the file was written. That is a question -put to the user on the PR that fixed 8jk6.6, and `an_import_that_skips_some_records_warns_with_a_count_and_still_writes` -pins the current answer so that changing it is a decision. +`import … && next` stops even though the file was written. + +The user ruled on PR #321: "Add a --strict flag". The default stays exit 0 +with the counted warning, pinned by +`an_import_that_skips_some_records_warns_with_a_count_and_still_writes`. +`import --no-wizard --strict` turns any skipped record into a refusal: +`--strict: Skipped 1 of 3 records that were not JSON objects`, exit 1. + +**A strict refusal writes nothing.** A refusal that still wrote the file would +be the half-met contract argued against above: `&&` stops while the output +sits there, and a script that checks only the file gets a model that `--strict` +existed to prevent. So the check runs in `run_headless_import` before the +build. It reads `ImportPipeline::skip_warning`, the same function the build's +warning comes from, so the two cannot disagree about what was skipped. +"Skipped" here means a record that is not an object, the only skip left after +8jk6.2. An unparsed date or a short CSV row is a warning about a record that +was kept, and `--strict` does not refuse it. ### A folder of CSVs is one sparse model, with a view per file (improvise-lpzu) diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 8a74cd10..7f9b2233 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -478,7 +478,9 @@ formula exited 0`). The test passes a good formula too (`Z = x`) and asserts it is not named, so a refusal that lists every formula cannot pass. `expected` comes from pest's error, which proves the parse error is carried and not only the formula text. **R4**, a build that drops what does not -parse, fails it. +parse, fails it. Since improvise-8jk6.17 it passes two bad formulas, so +**R5**, naming only the first, fails it here too. At `efaa5050` only the io +unit test saw R5. ### an_import_that_keeps_no_record_refuses_and_says_why @@ -490,9 +492,9 @@ the user at a fix that cannot help. The base failure was on the message. ### an_import_that_skips_some_records_warns_with_a_count_and_still_writes -improvise-8jk6.6, the partial case. It passed on the base: it pins the current -answer, which the PR puts to the user as a question, so that changing it is a -decision and not a drift. It asserts the warning's count (`1 of 3`), exit 0, +improvise-8jk6.6, the partial case. It passed on the base. It pins the +default the user kept on PR #321 ("Add a --strict flag"), so that changing +the default is a decision and not a drift. It asserts the warning's count (`1 of 3`), exit 0, and a Data line from the record *after* the skipped one. That last check shows the skip did not end the import early. **R9**, making any skip fatal, fails it. @@ -507,3 +509,42 @@ other half pins the direction the ruling did *not* change: the same CSV named directly imports without any `File`. The rival that merges every all-CSV import, lone files included, fails that half ("a lone file keeps today's shape"). So the test separates "a folder merges" from "everything merges". + +### a_strict_import_that_skips_a_record_refuses_and_writes_nothing + +improvise-8jk6.6, the user's ruling on PR #321. Written before `--strict` +existed, it failed on clap's unknown-argument error. It asserts the refusal +names the flag and the same count as the default warning, and that the output +path does not exist. + +- **S1**, the flag parsed but ignored, fails it on the exit status. +- **S3**, refusing only after `persistence::save`, fails it on the output + path. That is the "a strict refusal still writes" contract this PR decided + against. + +### a_strict_import_that_skips_nothing_still_writes + +The control for the entry above. Without it, a `--strict` that refuses +**every** import passes that test. **S2** is that rival, and it fails here. +It failed before the flag existed too, but only on clap's error, which is not +the discriminating failure. The discriminating one is S2. + +### a_csv_decimal_measure_imports_as_its_number + +improvise-8jk6.17 (F3), from the adversarial review of #321. The fixture has +decimal CSV measures (`1000.50`, `-2.25`). With `number_in`'s `f64` branch +disabled (integers only), no headless test failed at `efaa5050`, and a decimal +measure was silently dropped. **F3** now fails this test and the next one. +This is coverage, not a bug: it passed when written. + +### import_json_on_a_csv_reads_its_numeric_column_as_a_measure + +improvise-8jk6.17 (F1). `improvise cmd 'import-json f.csv'` reaches +`ImportJsonHeadless`, a third CSV entry point besides `import`. **F1** +restores its CSV branch to the JSON constructor via +`ImportPipeline::new(Value::Array(recs.to_vec()))`. That still compiles, +because `CsvRecords` derefs to a slice. `Revenue` then becomes a category of +text items, and this test fails on its Data lines. At `efaa5050` only +`import_headless_refuses_records_that_collide` failed under F1, and only by +accident. The `## Category: Revenue` assertion names the symptom directly. +This is coverage: it passed when written. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 11b6a9a0..1e45fc70 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1904,4 +1904,4 @@ records. So without `nothing_to_import` the build succeeds and saves `File` items and empty views: the category check alone cannot refuse. The `setup` assertion proves that seeded proposal exists, so the test is not passing only because there was no category. **R7**, the check removed from the build, fails -it. Whether this case should refuse is put to the user on the PR. +it. The user confirmed the refusal on PR #321: "Refuse (as #321 does)". diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index f8bd66a9..8178ac13 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -173,6 +173,12 @@ impl ImportPipeline { self.proposals = analyze_records(&typed); } + /// The warning for the records the build will skip, if it skips any. + pub fn skip_warning(&self) -> Option { + let skipped = self.records.iter().filter(|r| !r.is_object()).count(); + skip_warnings(self.records.len(), skipped) + } + /// Why there is nothing to build, when no record is an object to place. pub fn nothing_to_import(&self) -> Option { let total = self.records.len(); @@ -379,7 +385,6 @@ impl ImportPipeline { wb.add_item("_Measure", &m.field)?; } - let mut skipped_non_objects = 0usize; // Per-extraction tally of values the detected format could not parse. A // derived coordinate that is merely absent is improvise-yss5. let mut unparsed_dates = vec![0usize; date_extractions.len()]; @@ -395,7 +400,7 @@ impl ImportPipeline { for (at, placement) in placements.into_iter().enumerate() { match placement { - Placement::NotObject => skipped_non_objects += 1, + Placement::NotObject => {} Placement::Placed { mut coords, measures, @@ -451,9 +456,7 @@ impl ImportPipeline { wb.views.insert(view.name.clone(), view); } - let mut warnings: Vec = skip_warnings(self.records.len(), skipped_non_objects) - .into_iter() - .collect(); + let mut warnings: Vec = self.skip_warning().into_iter().collect(); warnings.extend(unparsed_date_warnings(date_extractions, &unparsed_dates)); warnings.extend(collision_warning(&summed)); warnings.extend(occurrence.map(|name| separated_note(name, &collisions))); diff --git a/src/main.rs b/src/main.rs index c1d3f5f9..3bb31e30 100644 --- a/src/main.rs +++ b/src/main.rs @@ -97,6 +97,10 @@ struct ImportArgs { #[arg(long, value_enum, default_value_t = OnDuplicate::Refuse)] on_duplicate: OnDuplicate, + /// With --no-wizard, refuse (and write nothing) if any record is skipped + #[arg(long)] + strict: bool, + /// Save to file instead of opening TUI #[arg(short, long)] output: Option, @@ -157,6 +161,7 @@ impl Runnable for ImportArgs { formulas: self.formula, name: self.name, on_duplicate: self.on_duplicate, + strict: self.strict, }; if self.no_wizard { @@ -191,6 +196,7 @@ struct ImportConfig { formulas: Vec, name: Option, on_duplicate: OnDuplicate, + strict: bool, } fn parse_colon_pairs(args: &[String]) -> Vec<(String, String)> { @@ -295,6 +301,10 @@ fn run_headless_import( model_file: Option, ) -> Result<()> { apply_config_to_pipeline(&mut pipeline, config); + // A strict skip refuses before the build, so nothing is written (8jk6.6). + if let (true, Some(skipped)) = (config.strict, pipeline.skip_warning()) { + anyhow::bail!("--strict: {skipped}"); + } if pipeline.on_duplicate == DuplicatePolicy::Refuse { let collisions = pipeline.collisions(); if !collisions.is_empty() { diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index fab470e7..cf6f6771 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -777,11 +777,14 @@ fn an_import_formula_that_does_not_parse_refuses_names_it_and_writes_nothing() { "Y = x +", "--formula", "Z = x", + "--formula", + "W = * 2", ], ); let err = stderr_of(&out); assert!(!out.status.success(), "a dropped formula exited 0: {err}"); assert!(err.contains("'Y = x +'"), "names the formula: {err}"); + assert!(err.contains("'W = * 2'"), "names every bad one: {err}"); assert!(err.contains("expected"), "carries the parse error: {err}"); assert!(!err.contains("'Z = x'"), "names only the bad one: {err}"); assert!(!out_path.exists(), "nothing is written"); @@ -823,6 +826,75 @@ fn an_import_that_skips_some_records_warns_with_a_count_and_still_writes() { assert!(doc.contains("_Measure=x, a=m = 4"), "{doc}"); } +/// A JSON array with one stray scalar among its records. +const ONE_STRAY: &str = r#"[{"a": "k", "x": 1}, 2, {"a": "m", "x": 4}]"#; + +/// improvise-8jk6.6, the user's ruling: `--strict` makes any skip a refusal +/// that writes nothing. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_strict_import_that_skips_a_record_refuses_and_writes_nothing() { + let scratch = Scratch::new("strict_skip"); + let (out, out_path) = import_of(&scratch, "p.json", ONE_STRAY, &["--strict"]); + let err = stderr_of(&out); + assert!(!out.status.success(), "{err}"); + assert!( + err.contains("--strict: Skipped 1 of 3 records that were not JSON objects"), + "{err}" + ); + assert!(!out_path.exists(), "a strict refusal writes nothing"); +} + +/// The control: `--strict` refuses a skip, not every import. +#[test] +fn a_strict_import_that_skips_nothing_still_writes() { + let scratch = Scratch::new("strict_clean"); + let body = r#"[{"a": "k", "x": 1}, {"a": "m", "x": 4}]"#; + let (out, out_path) = import_of(&scratch, "p.json", body, &["--strict"]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + assert!(doc.contains("_Measure=x, a=m = 4"), "{doc}"); +} + +/// improvise-8jk6.17 (F3): a CSV decimal measure is its number, not dropped +/// text. Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_csv_decimal_measure_imports_as_its_number() { + let scratch = Scratch::new("csv_decimal"); + let body = "Region,Revenue\nEast,1000.50\nWest,-2.25\n"; + let (out, out_path) = import_of(&scratch, "f.csv", body, &[]); + assert!(out.status.success(), "{}", stderr_of(&out)); + let doc = std::fs::read_to_string(&out_path).unwrap(); + for line in [ + "Region=East, _Measure=Revenue = 1000.5", + "Region=West, _Measure=Revenue = -2.25", + ] { + assert!(doc.lines().any(|l| l == line), "{line} missing from\n{doc}"); + } +} + +/// improvise-8jk6.17 (F1): `import-json` reads a CSV as CSV, so its numeric +/// column is a measure. Evidence: `context/test-evidence/cli.md`. +#[test] +fn import_json_on_a_csv_reads_its_numeric_column_as_a_measure() { + let scratch = Scratch::new("import_json_csv"); + let csv = scratch.path("f.csv"); + std::fs::write(&csv, "Region,Revenue\nEast,1000.50\nWest,7\n").unwrap(); + let doc = scratch.path("m.improv"); + let out = run_cmd(&[&format!("import-json {}", csv.display())], &doc); + assert!(out.status.success(), "{}", stderr_of(&out)); + let text = std::fs::read_to_string(&doc).unwrap(); + assert!(!text.contains("## Category: Revenue"), "{text}"); + for line in [ + "Region=East, _Measure=Revenue = 1000.5", + "Region=West, _Measure=Revenue = 7", + ] { + assert!( + text.lines().any(|l| l == line), + "{line} missing from\n{text}" + ); + } +} + /// improvise-lpzu, the user's ruling: a folder always merges, so even one CSV /// keeps its file identity. Evidence: `context/test-evidence/cli.md`. #[test]