fix: persist a category's kind, so a label no longer unloads a document

improvise-dy3j (P1). CategoryKind::Label had no spelling in `.improv` at
all: format_md wrote every category as a bare `## Category: Name` and
build_category read every section back through add_category. The kind was
dropped on the way out AND on the way in, so a round-trip equality test
could not see it.

A label is outside MAX_CATEGORIES = 12 by design and the import wizard
scores it as zero, so twelve categories plus a label is legally
constructible. On load every label came back Regular, the thirteenth was
refused, and parse_md failed: any such document was permanently
unloadable. Below the limit the loss was quieter — a label silently
became a pivot category.

Grammar first. The ticket proposed `## Category: Note [label]`, and
improvise-pczv forbids it: a section header takes its name as the WHOLE
line, so the suffix is indistinguishable from a category named
`Note [label]`. The kind is therefore an entry inside the section,
`kind: label`, spelled with a real token rather than a lookahead — the
grammar-walking generator skips predicates. Absence means `regular`, so
every existing file parses unchanged and a pivot category's section is
byte-identical to before.

Tests assert the parsed Category::kind, never the emitted line. Both
rival faults were injected against the fixed tree and both fail two
tests: a writer that omits the line, and a reader that discards the kind
it parsed (whose file text is correct, so any text-comparing test passes
it). Reasoning in context/code/formula-and-io.md; evidence in
context/test-evidence/formula-and-io.md.

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-22 00:57:43 -07:00
parent d46ec59984
commit d9f732c529
6 changed files with 235 additions and 4 deletions
+53
View File
@@ -154,6 +154,59 @@ non-atomic rules produce the whitespace-separated tokens pest's implicit
with three or more alternatives (nested `Choice`s) pick deeper branches less
often — good enough for random generation, not a uniform distribution.
### The category kind is a line, not a header suffix (improvise-dy3j)
`CategoryKind::Label` had no spelling in the format at all. `format_md` wrote
every category as a bare `## Category: Name` and `build_category` read every
section back through `add_category`, so the kind was dropped on the way out
**and** on the way in. A round-trip equality test cannot see a loss both sides
share, which is why the existing suite was silent — the same shape as the
`Formula::raw` traps above, and the reason the new tests assert the *parsed*
`Category::kind` rather than the emitted text.
The consequence was a lost document, not a lost flag. `Label` sits outside
`MAX_CATEGORIES = 12` on purpose, and the import wizard's own precondition
scores a label as zero, so it will happily build twelve categories plus a
label. On load every label came back `Regular`, the thirteenth was refused, and
`parse_md` failed: **any document where regular + label exceeded twelve was
permanently unloadable.** Below the limit the failure was quieter — a label
silently became a pivot category, entering `regular_category_names()` pickers
and gaining axis auto-assignment.
**The ticket proposed `## Category: Note [label]`, and the grammar forbids
it.** A section header takes its name as the **whole line**, totally
(improvise-pczv) — `header_name` is `pipe_quoted ~ &NEWLINE | line_name` — so a
suffix is not a suffix, it is part of a category literally named `Note
[label]`. The two readings are indistinguishable, and adopting the suffix would
have made a legal name unspellable while quietly re-opening the ambiguity
`header_name` exists to close.
So the kind is an **entry inside the section**, beside the item lines:
```
## Category: Note
kind: label
- see the record view
```
Three properties earned it. It uses a **real token** (`"kind: "`), not a
lookahead, because the grammar-walking generator reads the grammar through
`pest_meta` and *skips predicates* — a rule guarded only by a lookahead is a
rule the generator can violate, which is why `WHERE` carries `sp+`. It cannot
collide with `item_list`, `grouped_item` or `group_hierarchy`, each of which
needs a `- ` or `> ` lead. And **absence is the compatible reading**: `kind_kw`
spells only `label`, the writer emits the line only for a label, so every file
written before this commit parses unchanged and every pivot category's section
is byte-identical to what it was. That is also why `parse_kind_kw` is an
exhaustive match returning `Result` rather than a `bool` read off the rule's
mere presence — a second keyword must fail loudly at the walker, not be read as
`label` because it arrived on a `kind_line`.
One case the format already allowed and this had to answer: two
`## Category:` sections naming one category are one category (improvise-349m),
merged in pass 1. Absence is not a claim, so the merge keeps a stated kind
rather than letting a later kindless section erase it.
### The tree walkers are infallible by construction
Every `build_*` in `parser.rs` operates on a `Pair` pest has already
+66
View File
@@ -1288,3 +1288,69 @@ the parse would fail the test for the wrong reason.
The absent case, so the field above is something the file carried rather than a
constant the walker invents. Without it, a walker hard-coding `Some(".4")`
would pass the test above.
### a_label_category_survives_the_round_trip_as_a_label
`CategoryKind::Label` had no spelling in `.improv` at all: `format_md` wrote
every category as a bare `## Category: Name` and `build_category` read every
section back through `Workbook::add_category`. The kind was dropped on the way
out *and* on the way in, so the loss was symmetric and no round-trip equality
test could see it — the same shape the `Formula::format` entry above records.
Reproduced at `d46ec59`: a workbook with `Region` (regular) and `Note` (label)
came back with `Note.kind == Regular`.
The test asserts the **parsed** `Category::kind`, never the emitted line, for
that reason. Its second assertion — that `Region` still reads back `Regular` —
is the control: a reader that made every category a `Label` satisfies the first
assertion and fails this one.
Faults injected, both against the fixed tree:
- **Writer drops the line** (`if false` around `w!(out, "kind: label")`). Fails
`left: Regular right: Label`, and takes
`a_document_at_the_category_limit_plus_a_label_reloads` with it.
- **Reader ignores the kind it parsed** (`build_category` calls `add_category`
unconditionally, discarding `pc.kind`). Same two failures. This is the
discriminating one: the file text is *correct* in this variant, so a test
comparing emitted output passes.
### a_document_at_the_category_limit_plus_a_label_reloads
The P1 half, and the reason improvise-dy3j was not merely a lost flag. `Label`
is outside `MAX_CATEGORIES = 12` by design — `Model::add_label_category` has no
limit check, and the import wizard's `proposed_category_count` scores a label
as zero — so a document with twelve categories and one label is legally
constructible and the wizard will build one from any CSV carrying a free-text
column. Every label came back `Regular` on load, the thirteenth category was
refused, and `parse_md` returned
`Err("Maximum of 12 categories reached")`: **the document was permanently
unloadable.** Reproduced at `d46ec59` with exactly that error.
The second assertion (`regular_category_names().len() == 12`) is what keeps the
first honest. A "fix" that simply raised or removed the limit would let the
document load while putting a high-cardinality label into the pivot count, and
this assertion fails on it.
### a_category_section_with_no_kind_line_loads_as_regular
The absent case: every `.improv` file written before this commit carries no
`kind:` line, and must still load. Without it, a walker that defaulted
`PCategory::kind` to `Label` would pass both tests above.
The input is hand-written text rather than a round trip, deliberately — a round
trip cannot exhibit an absent line, because the writer under test is the thing
that would have written it.
### a_loaded_label_category_is_not_on_a_pivot_axis
The end-to-end check that the two halves of improvise-dy3j/cj4u agree: a label
is off every pivot axis in every view after a save and a load, including a view
created after the label.
Worth stating what it does and does not witness. It is **not** a second witness
for the kind: `format_md` writes each view's axes explicitly, so `Note: none`
carries the seating across the file whatever the kind does. What it witnesses
is the *seating* — with the `improvise-cj4u` fix reverted, `create_view`
pivots `Note` onto Column in the source workbook, the file faithfully records
`Note: column`, and this test fails. It was written while cj4u was still open
and failed then, which is how that was established rather than assumed.
@@ -181,6 +181,11 @@ pub fn format_md(workbook: &Workbook) -> String {
continue;
}
w!(out, "\n## Category: {}", quote_header(&cat.name));
// The kind is written only where it is not the default, so a pivot
// category's section is byte-identical to before (improvise-dy3j).
if cat.kind == CategoryKind::Label {
w!(out, "kind: label");
}
// Items are written in insertion order, because that is the order the
// grid's axes consume (gotcha 18). A run of bare items shares a line.
let mut bare: Vec<String> = Vec::new();
@@ -36,7 +36,12 @@ category_section = {
category_entry*
}
category_entry = _{ group_hierarchy | grouped_item | item_list }
category_entry = _{ kind_line | group_hierarchy | grouped_item | item_list }
// The category's kind. Absent means `regular` — the compatible reading of
// every file written before this line existed. context/code/formula-and-io.md.
kind_line = { "kind: " ~ kind_kw ~ NEWLINE ~ blank_lines }
kind_kw = @{ "label" }
// Comma-separated bare items (no group): `- Food, Gas, Total`
item_list = {
+28 -2
View File
@@ -7,7 +7,7 @@ use pest::iterators::{Pair, Pairs};
use pest_derive::Parser;
use crate::formula::{AggFunc, parse_formula};
use crate::model::category::Group;
use crate::model::category::{CategoryKind, Group};
use crate::model::cell::{CellKey, CellValue};
use crate::view::Axis;
use crate::workbook::Workbook;
@@ -27,6 +27,9 @@ struct PCategory {
/// is load-bearing, so pass 2 must replay this list as written.
items: Vec<(String, Option<String>)>,
group_parents: Vec<(String, String)>,
/// What a `kind:` line said, `Regular` where none did. Absence is not a
/// claim, so merging two sections keeps the stated one — context/code/formula-and-io.md.
kind: CategoryKind,
}
/// One `## View:` section as pass 1 read it.
@@ -60,16 +63,21 @@ impl ParsedFile {
name,
items,
group_parents,
kind,
} = pc;
match self.categories.iter_mut().find(|e| e.name == name) {
Some(existing) => {
existing.items.extend(items);
existing.group_parents.extend(group_parents);
if kind != CategoryKind::Regular {
existing.kind = kind;
}
}
None => self.categories.push(PCategory {
name,
items,
group_parents,
kind,
}),
}
}
@@ -164,9 +172,14 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
name: section_title(&mut inner, "category_section")?,
items: Vec::new(),
group_parents: Vec::new(),
kind: CategoryKind::Regular,
};
for entry in inner {
match entry.as_rule() {
Rule::kind_line => {
let kw = next(&mut entry.into_inner(), "kind_line")?;
pc.kind = parse_kind_kw(kw.as_str())?;
}
Rule::item_list => {
for name_pair in entry.into_inner() {
pc.items.push((extract_name(name_pair)?, None));
@@ -185,6 +198,15 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result<PCategory> {
Ok(pc)
}
/// The on-disk keyword for a category kind, exhaustively — the reader's half
/// of the `kind: label` line `format.rs` writes.
fn parse_kind_kw(kind: &str) -> Result<CategoryKind> {
match kind {
"label" => Ok(CategoryKind::Label),
_ => bail!("Unknown category kind: {kind}"),
}
}
/// The on-disk keyword for an aggregation, exhaustively — the reader's half of
/// `format.rs::agg_keyword`.
fn parse_agg_kind(kind: &str) -> Result<AggFunc> {
@@ -361,7 +383,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result<ParsedFile> {
/// Register one category's items and groups, in file order.
fn build_category(wb: &mut Workbook, pc: &PCategory) -> Result<()> {
wb.add_category(&pc.name)?;
match pc.kind {
CategoryKind::Label => wb.add_label_category(&pc.name)?,
_ => wb.add_category(&pc.name)?,
};
let cat = wb
.model
.category_mut(&pc.name)
@@ -490,6 +515,7 @@ mod grammar_coverage {
Rule::section => "silent: hoisted into file",
Rule::category_section => "walk_category_section",
Rule::item_list | Rule::grouped_item | Rule::group_hierarchy => "walk_category_section",
Rule::kind_line | Rule::kind_kw => "walk_category_section",
Rule::category_entry => "silent: hoisted into category_section",
Rule::formulas_section | Rule::formula_line | Rule::formula_raw => {
"walk_formulas_section"
@@ -4,7 +4,7 @@
use super::test_helpers::{coord, two_cat_model};
use super::{format_md, parse_md};
use crate::formula::parse_formula;
use crate::model::category::Group;
use crate::model::category::{CategoryKind, Group};
use crate::model::cell::CellValue;
use crate::view::Axis;
use crate::workbook::Workbook;
@@ -1158,3 +1158,79 @@ fn parse_md_reads_a_formula_with_no_format_as_none() {
let loaded = parse_md(&format_md(&m)).unwrap();
assert_eq!(loaded.model.formulas()[0].format, None);
}
// ── Category kinds (improvise-dy3j) ──────────────────────────────────────
/// The kind is a parsed field, not a spelling the file happens to carry.
/// Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_label_category_survives_the_round_trip_as_a_label() {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_label_category("Note").unwrap();
let loaded = parse_md(&format_md(&wb)).unwrap();
assert_eq!(
loaded.model.category("Note").unwrap().kind,
CategoryKind::Label
);
// The control that keeps the assertion above from passing vacuously: a
// reader making every category a Label would clear it and fail here.
assert_eq!(
loaded.model.category("Region").unwrap().kind,
CategoryKind::Regular
);
}
/// A label sits outside `MAX_CATEGORIES`, so a document at the limit plus one
/// label must reload. Evidence: `context/test-evidence/formula-and-io.md`.
#[test]
fn a_document_at_the_category_limit_plus_a_label_reloads() {
let mut wb = Workbook::new("T");
for i in 0..12 {
wb.add_category(format!("C{i}")).unwrap();
}
wb.add_label_category("Note").unwrap();
let loaded = parse_md(&format_md(&wb)).expect("a label must not count against the limit");
assert_eq!(
loaded.model.category("Note").unwrap().kind,
CategoryKind::Label
);
assert_eq!(loaded.model.regular_category_names().len(), 12);
}
/// A file written before the `kind:` line existed reads its categories as
/// `Regular` — the compatible reading, and what the writer still omits.
#[test]
fn a_category_section_with_no_kind_line_loads_as_regular() {
let text = "v2025-04-09\n\
# Budget\n\
\n\
## Category: Region\n\
- North, South\n";
let loaded = parse_md(text).unwrap();
assert_eq!(
loaded.model.category("Region").unwrap().kind,
CategoryKind::Regular
);
}
/// The loader seats a label off every pivot axis, which is the view half of
/// the same fact (improvise-cj4u).
#[test]
fn a_loaded_label_category_is_not_on_a_pivot_axis() {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_label_category("Note").unwrap();
wb.create_view("Other");
let loaded = parse_md(&format_md(&wb)).unwrap();
for (name, view) in &loaded.views {
assert_eq!(
view.axis_of("Note"),
Axis::None,
"view {name} pivoted a label category"
);
}
}