fix(core,io): three findings from the code-review skill on this branch

Self-review by fault injection, not reading. Three findings, one a real bug
this branch would otherwise have shipped and two vacuous tests of my own.

1. A REAL BUG THIS BRANCH CREATED THE DURABILITY OF. Model::remove_item left
   measure_agg holding an entry for a measure it had just deleted -- exactly
   improvise-qx1z's defect one layer down, in the same branch that fixed the
   View half and did not look at the Model half. Harmless while improvise-ppru
   was unfixed, because nothing persisted the map; persisting it turned a
   stale entry into a permanent one, so the document grows an Aggregation line
   naming a measure it does not have and re-adding that measure brings back an
   aggregation nobody asked for. A fix that introduces a format for state that
   outlives what it names is how a latent bug becomes a permanent one, so the
   sweep ships with the writer.

2. A VACUOUS CONTROL. a_file_naming_no_view_keeps_the_seeded_default PASSED
   under the over-broad "always drop Default" it exists to catch, because
   delete_view refuses the LAST view -- a guard one level away from the
   condition under test. It answered "does delete_view refuse the last view"
   where it meant "does drop_unnamed_default decline to fire". Split in two:
   a discriminating case naming Default AND a sibling, where the wrong
   condition really deletes, and the legacy no-views case kept separate and
   marked as the weaker statement it is.

3. A VACUOUS NEW TEST. a_category_named_prune_is_not_read_as_the_prune_flag
   passed under the rejected presence-style grammar that swallows "prune: row"
   whole, because the fixture used Row and on_category_added RE-DERIVES Row for
   the first regular category on reload -- the axis matched by reinvention, not
   by being read. Now uses Page plus a page selection, neither of which any
   default supplies.

Also verified and unchanged: the generator reaches all three new grammar
branches, including the scalar data line, each proved by a separate injection;
awkward measure names (|A: B|, |sum|, true) round-trip through the Aggregation
section; and improvise-nvq4 still does not reproduce.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-21 15:17:06 -07:00
parent 98be603cb9
commit f26cc2a22a
3 changed files with 126 additions and 9 deletions
+56 -4
View File
@@ -491,12 +491,45 @@ test fails with `["Default", "Plan"]`. The fix is narrow on purpose — it drops
`Default` only when the file named **at least one** view and none of them was
`Default`, so the two cases in the control below are untouched.
#### a_file_that_names_default_itself_keeps_it_alongside_its_siblings
The control the fix must not break, and **the second view is the whole point**.
This entry replaces a vacuous one, found by review injecting
`default_is_a_phantom = true` — the over-broad "always drop Default" — and
watching the control **pass**. It passed because `Workbook::delete_view` refuses
the last view: with `Default` alone in the map the drop was refused by a guard
one level away from the condition under test, so the test answered *does
`delete_view` refuse the last view* where it meant *does `drop_unnamed_default`
decline to fire*. Naming `Default` **and** `Other` gives the map two entries, so
the wrong condition really does delete and the test really can fail. (Two
pre-existing tests, `parse_md_round_trips_multiple_views` and
`parse_md_order_independent_new_view_before_categories`, also catch it — the
repo was covered, but not by the test that claimed to be the control.)
#### a_file_naming_no_view_keeps_the_seeded_default
The control the fix must not break, in both directions: a file with no `## View`
section still opens on the seeded view (the legacy reading), and a file that
names `Default` itself keeps it. Without this, "drop Default" could have been
implemented as "always drop Default" and the test above would not have noticed.
The legacy reading, kept separate and **deliberately weaker**: a file with no
`## View` section at all still opens on the seeded view. It cannot be made
discriminating the way its neighbour was — with no view named there is only ever
one in the map, so `delete_view`'s last-view refusal masks any condition. It is
a statement of behaviour, not evidence of the fix.
#### a_category_named_prune_is_not_read_as_the_prune_flag
`prune: true` and `prune: row` differ only in the value, and `view_entry` tries
`prune_line` **before** `axis_line` — so a category literally named `prune`
round-trips only because `bool_kw` is `"true" | "false"` and fails on an axis
keyword. Safety rests on the disjointness of two keyword sets, which nothing
else checks, which is why the test exists.
**Its first version was vacuous, and the injection is what showed it.** Feeding
the rejected presence-style design — `prune_line = { "prune: " ~ rest_of_line }`,
which swallows `prune: row` whole — left the suite green, because the fixture put
the category on `Row` and `Workbook::add_category` → `View::on_category_added`
**re-derives** `Row` for the first regular category on reload. The axis matched
by reinvention rather than by being read. The fixture now uses `Page` plus a page
selection, neither of which any default supplies, and the same injection fails
it.
### the_aggregation_section_is_written_in_a_stable_order
@@ -508,6 +541,25 @@ then asserts `format(parse(text)) == text`. Fault injected: drop the
`sort_by_key`; the assertion fails on most runs, which is itself the argument
for pinning it rather than trusting a single green run.
### a_removed_measures_aggregation_is_not_written
Found by **review of this branch**, not by a ticket, and it is improvise-qx1z's
own defect one layer down: `Model::remove_item` removed the item and its cells
and left `measure_agg` holding an entry for a measure that no longer exists.
It was harmless while improvise-ppru was unfixed, because nothing persisted the
map — the stale entry evaporated on reload. **Persisting it made it durable**, so
the document grew a `## Aggregation` line naming a measure it does not have, and
re-adding that measure brought back an aggregation nobody asked for. A fix that
introduces a format for state that outlives what it names is how a latent bug
becomes a permanent one; the sweep had to ship with the writer.
The fixture carries **two** measures and removes one, so the assertion separates
"forget this measure" from "clear the map" — the same shape
`on_item_removed_forgets_a_hidden_item` needs for the view half. Fault injected:
delete the `measure_agg.remove` line; the test fails on the written text before
it ever reaches the reader.
### an_interleaved_category_keeps_its_item_order
improvise-3yv3. `format_md` collected every ungrouped item into `bare` and
+5
View File
@@ -332,6 +332,11 @@ impl Model {
if let Some(cat) = self.categories.get_mut(cat_name) {
cat.remove_item(item_name);
}
// The measure's aggregation is part of what the measure IS, so it goes
// with it — improvise-qx1z's asymmetry, on the model side.
if CategoryKind::of_name(cat_name).is_measure() {
self.measure_agg.remove(item_name);
}
let to_remove: Vec<CellKey> = self
.data
.iter_cells()
@@ -964,18 +964,78 @@ fn a_file_naming_its_own_views_gains_no_default() {
assert_eq!(wb.active_view, "Plan");
}
/// The control the fix must not break: a file naming NO view still opens on
/// the seeded one, and a file that names `Default` itself keeps it.
/// The control the fix must not break, and the SECOND view is what makes it
/// discriminating. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_file_that_names_default_itself_keeps_it_alongside_its_siblings() {
let text = "v2025-04-09\n# T\nInitial View: Default\n\n## View: Default\n\n\
## View: Other\n\n## Category: C\n- a\n";
let wb = parse_md(text).unwrap();
assert_eq!(
wb.views.keys().collect::<Vec<_>>(),
vec!["Default", "Other"]
);
assert_eq!(wb.active_view, "Default");
}
/// The legacy reading: a file with no `## View` section at all still opens on
/// the seeded view. Weaker than its neighbour on purpose — see the evidence.
#[test]
fn a_file_naming_no_view_keeps_the_seeded_default() {
let text = "v2025-04-09\n# T\n\n## Category: C\n- a\n";
let wb = parse_md(text).unwrap();
assert_eq!(wb.views.keys().collect::<Vec<_>>(), vec!["Default"]);
}
let named = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n";
/// A category named `prune` writes `prune: row`, which the reader must take as
/// an axis line. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_category_named_prune_is_not_read_as_the_prune_flag() {
let mut wb = Workbook::new("T");
wb.add_category("prune").unwrap();
wb.add_item("prune", "x").unwrap();
wb.active_view_mut().prune_empty = true;
// Page, NOT the Row `on_category_added` would re-derive: a swallowed axis
// line is otherwise invisible, because reloading reinvents the default.
wb.active_view_mut().set_axis("prune", Axis::Page);
wb.active_view_mut().set_page_selection("prune", "x");
let text = format_md(&wb);
let back = parse_md(&text).unwrap_or_else(|e| panic!("unparseable: {e}\n{text}"));
assert!(back.active_view().prune_empty, "the flag was lost:\n{text}");
assert_eq!(
parse_md(named).unwrap().views.keys().collect::<Vec<_>>(),
vec!["Default"]
back.active_view().axis_of("prune"),
Axis::Page,
"the axis line was swallowed by the prune line:\n{text}"
);
assert_eq!(back.active_view().page_selection("prune"), Some("x"));
}
/// improvise-ppru made this durable, so the sweep had to come with it.
/// Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_removed_measures_aggregation_is_not_written() {
let mut wb = Workbook::new("T");
for name in ["Price", "Volume"] {
wb.add_item("_Measure", name).unwrap();
wb.model
.measure_agg
.insert(name.into(), crate::formula::AggFunc::Avg);
}
wb.remove_item("_Measure", "Price");
let text = format_md(&wb);
assert!(
!text.contains("- Price:"),
"Price outlived its item:\n{text}"
);
let back = parse_md(&text).unwrap();
assert!(!back.model.measure_agg.contains_key("Price"));
assert!(
matches!(
back.model.measure_agg.get("Volume"),
Some(crate::formula::AggFunc::Avg)
),
"the sweep must take one measure, not the map: {:?}",
back.model.measure_agg
);
}