# `improvise-formula` and `improvise-io` The arguments that used to live in the comments of these two crates. Written for a reader with the code open; the code says *what*, this says *why*, and why the obvious alternative is wrong. Bug narratives keyed by test name are in `context/test-evidence/formula-and-io.md`. --- ## Pipe quoting has exactly one home `improvise_formula::quote` owns **both halves** of the CL-style `|…|` convention — writing and reading — for both grammars. `formula.pest`'s `bare_ident`/`pipe_quoted` and `improv.pest`'s `bare_name`/`pipe_quoted` mirror each other character for character, and `quote.rs` is the one Rust statement of that rule. It lives in `improvise-formula` because that crate owns the grammar that says what a bare name *is*, and because every emitter needs it: a name that came from a user's data has to be spelled so the parser hands it back unchanged. `improvise-gp` re-exports it, `improvise-io` calls it from `persistence/{format,parse}.rs`, and `parser.rs::unquote_pipe` is a thin shim over `unescape_pipe_body`. Before improvise-i2m5 there were **three** copies and they had drifted, over the keyword rule below. **`improvise-io` no longer has one** — its `persistence/quoting.rs` is deleted. `context/repo-map.md`'s file inventory kept claiming io "still has its own unguarded copy" long after the deletion — it was corrected in wave 1, but any *other* doc or comment still saying so is wrong, and that is the shape this particular fact goes stale in. Three entry points, because the callers do not want the same thing: - `quote_name` — a name as the grammar wants it: bare when `is_bare_name` allows, else pipe-quoted. - `pipe_quote` — unconditional. A `.improv` text cell goes through this, not through `quote_name`, because a bare `42` would read back as a number. - `unescape_pipe_body` — the inverse, over the body between the delimiters (what both grammars' `pipe_inner` matches). The escape table is written once, in the last two. ### The keyword guard is canonical, user-confirmed — do not revert it An item literally named `SUM` is written `|SUM|`. `is_bare_name` is deliberately **stricter than the grammar strictly requires**: bare keywords (`WHERE SUM AVG MIN MAX COUNT IF IS`) *do* parse today, but only because `agg_call` needs a `(` and PEG backtracking then falls through to `ref_expr` — an emergent property of **rule ordering**, not a stated rule. A pipe-quoted name is an identifier in every position, with nothing to fall through, so a name colliding with a keyword is quoted and stops depending on that ordering. `IS` joined the list with `is_expr` (improvise-4zl7), and it is the one keyword whose omission would have been *silently* wrong rather than merely fragile: a category literally named `IS` written bare turns `IS IS East` into a predicate on a category called `East`. Adding a production to `formula.pest` means adding its keyword here — the two are one change. The cost is worth paying because a `## Formulas` line carries formula text **verbatim**: a name `format_md` writes is later read by `parse_formula`. One namespace, one spelling rule. ### The escape table, and the one input outside both grammars `\|` → `|`, `\\` → `\`, `\n` → newline; any other `\X` is preserved verbatim. The trailing arms of `unescape_pipe_body` are unreachable from either grammar (`pipe_inner` is `("\\" ~ ANY | !"|" ~ ANY)*`, so a backslash is always followed by a character). They are written as a **total function** rather than a panic so a caller arriving by some other route gets its bytes back. The two pre-i2m5 unquoting copies differed *only* on a trailing lone backslash, where io returned it and `parser.rs` panicked; the survivor returns it. --- ## `Cat IS Item`: a coordinate predicate in the expression grammar `is_expr = { identifier ~ ^"IS" ~ filter_value }` sits in `primary` **before** `ref_expr`, which would otherwise consume the category name and leave `IS Item` unparseable. A bare name still parses as a reference: `is_expr` needs the keyword, fails without it, and PEG backtracks into `ref_expr`. It reuses `where_clause`'s two operands — an `identifier` for the category and a `filter_value` for the item — and therefore `build_filter`, which already reads exactly that pair. The `^"IS"` literal is a string in the grammar rather than a rule, so it contributes no pair and `into_inner()` yields the two operands and nothing else. One builder, one quoting rule, both halves through `improvise_formula::quote`. The grammar-walking generator needs no new case: it reads `formula.pest` through `pest_meta` at test time, so a production added to `primary` is generated and round-tripped from the day it exists. What the *evaluator* does with it, and why a fitted tree is always wrapped in a `SUM`, is `context/code/core.md` §13. ### `WHERE` carries the same boundary, and the mechanism has to be generatable `where_clause` and `inline_where` are compound-atomic with `sp+` after the keyword, exactly as `is_expr` is (improvise-r08u). Non-atomic, pest auto-skipped `WHITESPACE` after `^"WHERE"`, so `SUM(Rev WHEREAS x = y)` read as a `WHERE` on a category `AS` — a silently wrong filter where it should have been a syntax error, the same class of defect `IS` had. Two things decided the shape, and the second is the one worth carrying. A negative lookahead `!bare_char` with `sp*` would have been *strictly* more permissive — it keeps `WHERE|Region|=East`, which the old rule accepted — but **the grammar-walking generator cannot honour a predicate**: it walks `formula.pest` through `pest_meta` and emits `WHEREa=""`, text the grammar then refuses, so `generated_formula_parses` goes red. A rule whose only guard is a lookahead is a rule the generator can violate. `sp+` is generatable, matches `IS`, and costs exactly one accepted spelling: a category may no longer abut the keyword, so `WHERE |Region|` needs its space. Note compound-atomicity *alone* closes the ticket's own reproducer, because `AS x` cannot be one identifier once implicit whitespace is gone. `sp+` is what closes the residue, `WHEREAS=y`, and the negative test names both. ## A formula target is unquoted, exactly as a reference is `parse_formula` strips the pipes off the target the same way it does off an `Expr::Ref`, because the target **names a `_Measure` item**. A target that kept its delimiters made `Model::add_formula` create an item literally called `|Total Revenue|` — a ghost column the user's real item never matched, and that `Expr::Ref("Total Revenue")` never resolved to. Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through that one call, so both were affected. The writer emits `Formula::raw`, never `target`, so the file text is unaffected either way — **which is exactly why the round-trip tests never saw it**, and why the fix repairs existing files on next load with no migration. --- ## The `.improv` grammar is the single source of truth `persistence/improv.pest` defines the format; `parse.rs` is a tree-walker over pest's parse tree, not a line scanner. Adding a format feature means changing the grammar first. The test generator (`parse.rs::generator`, and its twin in `parser.rs::generator` for `formula.pest`) reads the grammar's AST at test time via `pest_meta` and emits random valid input. **A new production therefore needs generator coverage**, or the proptests quietly stop describing the format: an `Expr` variant the emitter does not handle produces nothing, and `generated_file_parses` then passes over input that never exercised the new rule. `formula.pest`'s generator is explicit about this — lookaheads emit nothing on purpose, and any unused `Expr` variant falls through the `_ => {}` arm; if the grammar starts using one, generated formulas fail to parse and the gap surfaces. Two generator details worth not re-deriving. The `atomic` flag in the formula generator controls whether a `Seq` inserts a space between children, so non-atomic rules produce the whitespace-separated tokens pest's implicit `WHITESPACE` accepts. And `Choice` is 50/50 between *two* branches, so rules 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` on purpose (12 then, 24 since improvise-felo.1), 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 generator coverage this section demands was measured, not assumed.** `kind_line` is an alternative of `category_entry`, which the grammar walk reaches generically, so nothing in the generator needed changing — but that is the kind of claim this section exists to say must be checked. Probed with a throwaway test over 500 files from `generator::improv_file()`: **14 carried a `kind: label` line and all 14 parsed to a `CategoryKind::Label`**, so at the default 256 cases each proptest run exercises the new production about seven times. The probe was removed rather than kept: its rate depends on proptest's internal RNG, so as a permanent assertion it would fail on a dependency bump for a reason unrelated to the format. ### The tree walkers are infallible by construction Every `build_*` in `parser.rs` operates on a `Pair` pest has already validated. "A `formula` has exactly one `target`", "a `comparison` has lhs, op and rhs" — pest guarantees these before the walker sees the pair. So every `.expect(GRAMMAR_INVARIANT)` is a **bug marker**, not a runtime condition: if one fires, the grammar and the walker have diverged. **That reasoning used to be extended to the final `else` arm of each dispatch, and the extension was wrong.** The sentence here said the unlabelled arm was safe because `ref_expr` is the only remaining alternative of `primary`, `COUNT` the only remaining `agg_func` and `>=` the only remaining `cmp_op`. Each clause was true *of the grammar as it stood*, which is exactly the property a grammar edit changes -- so what the arm really encoded was a snapshot, silently re-interpreted as an invariant. "pest validated the pair" says the pair matches **some** alternative; it never said which, and an unlabelled arm reads the answer off a count the next commit is free to change. Two different repairs, and **which one is available is a property of the input**: - **A `&str`, where no compile error exists.** `parse_agg_func` and `parse_cmp_op` take the matched text, because `agg_func` and `cmp_op` are each *one* pest rule over string alternatives and `pest_derive` generates no per-alternative `Rule`. Nothing for a `match` to be exhaustive over, so they spell every alternative and `unreachable!(GRAMMAR_INVARIANT)` on anything else -- loud at the first parse rather than silent forever (improvise-492x). - **A `Rule`, where one does.** The four dispatches on `Pair::as_rule` -- `build_formula`, `build_unary`, `build_primary`, `filter_value_to_string` -- match over an enum, so they name every rule, including a final or-pattern over the ones that cannot appear. Adding **any** rule to `formula.pest` is now a build failure at all four until someone decides what it means. The greppable form: **`== Rule::` appears nowhere in `parser.rs`.** An `if rule == Rule::x` chain cannot be exhaustive, and it is how the snapshot got written down as an invariant in the first place. The cost is four or-patterns of ~28 enum names, which is genuinely noisy, and that is the price of the guarantee -- an invariant made unrepresentable rather than validated, applied to a grammar instead of to a type. The noise is also the tell that a walker has grown a dispatch, which is worth noticing. `build_primary` is where this was demonstrated rather than argued, and **the injection has to be chosen carefully**: a new alternative that is an *atomic* rule is caught by accident, because `first_inner` then finds no child and panics on its own `GRAMMAR_INVARIANT`. Only a **non-atomic** alternative reaches the silent path. With `pct_literal = { "#" ~ identifier }` placed first in `primary`, the pre-fix tree built clean, the whole crate suite passed, and `parse_formula("X = #Revenue")` returned `Ref("Revenue")` -- the `#` discarded and the formula quietly wrong. An injection into an atomic rule would have reported the code safe when it was not. `parse.rs`'s walkers are the opposite and deliberately so: they return `Result` and name the section in the error, because the `.improv` file is hand-editable and a failure there is a user's problem, not a programmer's. `parse_tree` renders pest's error as a located message (line number plus the offending line), because "Parse error: {pest dump}" is unusable to somebody fixing a file by hand. ### Two passes, because sections may appear in any order Pass 1 turns each `## ` section into a plain collector (`PCategory`, `PView`) or a flat list; pass 2 builds the `Workbook`. That split is what makes the format order-independent — a `## View:` may name a category defined further down. `PCategory::items` is a `Vec` in file order because `IndexMap` insertion order is load-bearing (gotcha 18), so pass 2 must replay the list as written. Two `## Category:` sections naming one category are **one** category, and the merge is a concatenation performed in pass 1 (`ParsedFile::push_category`, improvise-349m). Merging in pass 2 — building each section in turn — was not idempotent. A group is created either by the item that names it or by a `>` line, and `build_category` creates its own section's item-groups *before* its own `>` lines; so the first section's `>` groups landed ahead of the second section's item-groups, while `format_md` writes one section with all the items before all the `>` lines. The reparse then built the other order, and the two `format_md` outputs differed. Concatenating first makes the built order the written order by construction. Refusing a duplicate header was the alternative and would have made a legal hand-written file an error for no gain. ### A section header takes its name as the whole line, totally `header_name = { pipe_quoted ~ &NEWLINE | line_name }` (improvise-pczv). The `&NEWLINE` lookahead is load-bearing: without it, `## Category: |A|B` parses as `A` and **silently drops `B`**. `line_name` catches everything else, so a header name can no longer fail to parse. Four rules share it — `model_name`, `initial_view`, `category_section`, `view_section` — which is why it is one rule and not four copies. The writing half is `format.rs::quote_header`, and it is **deliberately not `quote_name`**: that rule is for a name embedded among other tokens, and applying it to a header would pipe-quote every ordinary spaced title the line carries perfectly well, rewriting the header of every existing document for no correctness gain. `quote_header` quotes exactly four things, each one thing the *reader* does: - the empty name, and any name with leading or trailing whitespace — the reader trims, so the category comes back renamed while every data coordinate still names the untrimmed original: silent orphaning; - a name containing `\n` or `\r` — it would start a second line, and `Region\n## Data` a second *section*; - a name already spelled `|…|` — the reader would take the pipes off. So a header is *written* raw where a reference is quoted. The grammar accepts both arms, so that asymmetry is a **writer convention, not a format rule**. --- ## `Storage` is whole-document read/write, and that closed the SQLite question Saving or loading a workbook only ever touches one whole document, so a backend needs no seeking, metadata or partial reads (improvise-6mq). It does need to enumerate and remove documents, because a browser build has no OS file manager behind it — hence `list` and `delete` beside `read` and `write` (improvise-i34). Paths are **opaque document names, not a directory tree**: a backend may treat them as filesystem paths, OPFS keys or map entries. The trait doc says "backends that are not trees treat `dir` as a name prefix" — keep that sentence reachable. It is the sentence that turned out to be *right while the `OpfsFs` impl was wrong*, and the bug was found by reading it. Because the trait is two ops over whole documents, an `sqlite3_vfs` — which needs `xRead(offset,len)`, `xWrite`, `xTruncate`, `xLock` — has nothing to bind to. See gotcha 41 for the measurements that declined improvise-9ix. ### `storage_contract` is a public function, not a test helper It reads like a test helper and it *was* one: `#[cfg(test)]` in `storage.rs`, run against `MemoryFs` and `PhysicalFs` so they could not drift. The one backend that most needed it could never be handed to it — `OpfsFs` is `cfg(target_arch = "wasm32")` and its sync access handles exist in no host harness — so for that backend the contract lived only in a doc comment. Moving it out of `cfg(test)` is what lets a browser run **this** sequence rather than a second transcription of it (improvise-vvl3/xf1). A second copy of a contract is the disease this codebase keeps curing. It returns `Result` rather than asserting because a failure has to travel out of a Web Worker as a string; the host tests `.unwrap()` it and get the same message. **Two bugs fell out of its first actual execution** — see the `dir` parameter and the index rules below. `dir` is a parameter, not a constant, because the three backends must agree at **every** `dir`, not only at the flat root the browser happens to use (improvise-vhcz). A backend whose `list` answered in a different path shape passes at `Path::new("")` — where joining is the identity — and fails at anything else. ### `impl Storage for Rc` lives in `improvise-io` because of the orphan rule `OpfsFs` keeps four genuinely async operations — `open`, `attach`, `refresh`, `commit_deletes` — off the trait, so `Storage` can stay synchronous (gotcha 28). But `App::storage` is a `Box`, and a trait object can name none of the four: **handing the backend to the `App` puts the async seam permanently out of reach**. The host must hold the concrete backend *and* give the `App` a `Storage`, which means sharing one. `impl Storage for Rc` is two foreign types in any other crate, so improvise-9x6 wrote a forwarding newtype in the worker and every future worker would have written it again. Here the trait is local, so the impl is allowed, and it is **blanket** rather than `OpfsFs`-specific because nothing about it is OPFS-shaped: it is the general statement that a shared handle to a backend is usable as that backend (improvise-hmws). `Rc` and not `Arc` because the one caller is a Web Worker — a single thread, and `wasm32-unknown-unknown` has no others. An `Arc` impl would be speculative. --- ## `OpfsFs`: why a browser backend can be synchronous, and what the index holds OPFS has two APIs. The promise-based one (`getFileHandle`, `createWritable`) is async, but `FileSystemFileHandle::createSyncAccessHandle()` yields a handle whose `read`/`write`/`truncate`/`flush`/`close` are ordinary **blocking** calls. It exists so wasm can do blocking file I/O — it is what SQLite's wasm build uses — and is available **only inside a dedicated Web Worker**, which is where improvise-9x6 puts the whole app. So `Storage` never had to become async. *Acquiring* a handle is still async, as are directory listing and removal, so the backend splits in two: - **async, explicit**: `open`, `attach`, `refresh`, `commit_deletes`, called by the worker from its message loop where awaiting is free; - **sync, via the trait**: `read`/`write` use an already-attached handle; `list` answers from an index; `delete` drops the name from that index and queues the removal. So a `delete` is visible to `read` and `list` immediately and lands in OPFS at the end of the batch. That ordering is the price of a synchronous trait and is invisible to a single worker. `write` therefore **un-queues** any pending delete of the same name: otherwise delete-then-write would be listed and readable and then removed by `commit_deletes` at the end of the batch — the one ordering the deferral can get wrong. ### The index holds documents, not entries `Storage::list` promises *document* names, and documents and entries stopped being the same thing the moment `attach` had to create a file to get a handle for a document that does not exist yet (improvise-vvl3). Three writers, and the split between them is the whole rule: - **`attach` indexes nothing.** What it creates is zero bytes. Indexing there made a fresh origin list `demo.improv` before anything had been written, so the seed was skipped as "already there" and `load_from` got an empty buffer. - **`Storage::write` indexes the name** (and un-queues a pending delete). A written document is the first thing there is to list. - **`refresh` rebuilds from OPFS, skipping zero-byte entries** — and counting an entry it cannot *measure* as a document, since the one cause of that is another tab holding the exclusive handle. `holds_bytes` is where the last clause lives, and its two failure modes get **opposite answers**, which is the whole function: - `getFileHandle` fails → the name is not a file. `keys()` yields directories too, and a directory rejects with `TypeMismatchError`; an entry removed between `keys()` and here rejects as well. Neither is a document and neither can ever be attached or read, so listing one is the same picker bug this filter exists to remove (Copilot, PR #24). - `getFile` fails on a real file handle → another context of the same origin holds the exclusive sync access handle (two tabs). An entry somebody else has open is certainly a document, so this **fails open**. Dropping it would hide a real document; failing the whole `refresh` would let a second tab break the first one's picker. Over-reporting costs one row — the direction `Dirty::of` is deliberately sloppy in. Handles are exclusive per file, so `attach` closes any handle it replaces and `detach` releases the lock. ### `list` joins `dir` like the other backends The join is not cosmetic (improvise-vhcz). `MemoryFs` and `PhysicalFs` answer with `dir`-joined paths — paths a caller can hand straight back to `read` — and `OpfsFs` used to answer with bare names. At `Path::new("")` the two shapes coincide, which is the only `dir` the worker ever passes, so the disagreement was invisible: a worker that organised documents under `docs/` would have got a **full picker in every host test and an empty one in a browser**, with nothing saying why. `dir` is decoration rather than a filter, because this backend is flat by construction: `entry_name` reduces every path to its file name, so `docs/a.improv` and `a.improv` are the same OPFS entry and an entry name may not contain a separator at all. That is the trait's own "treat `dir` as a name prefix" — the doc was right and the impl was wrong. --- ## Loading: decompression and decoding are two failures with two answers `load_from` decompresses, *then* decodes UTF-8, with a separate `Context` on each. `read_to_string` would fuse them, and a valid gzip stream holding invalid UTF-8 would then be reported as unreadable gzip — a wrong diagnosis of a document that decompressed perfectly well. Decoding once, afterwards, also leaves **one** answer to "is this UTF-8" rather than one per branch. The `.gz` suffix selects gzip regardless of backend: that is a property of the document name, not of the filesystem (improvise-6mq). **`.improv` text is the only native format (improvise-tt22).** A legacy JSON workbook used to be detected by a leading `{` and loaded read-only through `Workbook`'s serde derives. That path is gone, on the user's direction on PR #210. A document that still starts with `{` is refused with an error that names the format and says it is no longer supported. The test is `a_legacy_json_workbook_is_refused_by_name`. The error is decided rather than defaulted, because otherwise `parse_md` would report a grammar error at line 1, which diagnoses the wrong thing: the file is not malformed `.improv`. The check is on the decompressed text, so `.improv.gz` is covered too. The serde derives that existed only for that path went with it: `Workbook`, `Model`, `View`, `Category`, `Item`, `Group`, `CategoryKind`, `CellKey`, `DataStore`'s manual impls and the formula AST. So did indexmap's `serde` feature in `improvise-core`, `improvise-io` and the root crate, and `improvise-formula`'s serde dependency. Each was removed and the whole workspace rebuilt, so the compiler named every other consumer. Two remained, and both are the wire: `Axis` (whose `rename_all = "lowercase"` is its wire spelling) and `PageAxis`. A third arrived with improvise-km71 by the same rule: `FormulaId`, which crosses the wire in `ModelCommand::ReplaceFormula`, `FormulaSlot` and `FormulaEntry`. So `improvise-formula` depends on serde again, for that one type. No derive went back on the formula AST. A fourth, `CategoryKind`, went back on when `TileEntry` began carrying it (improvise-bqm0; `context/code/command.md`). It returned as a wire type, not as a document one: nothing loads a `Category` through serde. `CellValue` was never a candidate, since it is carried on the wire too. JSON *import* (`StartImportWizard`, `ImportJsonHeadless`) is a separate feature and is untouched. One test consumer had to move: the effect-classification tests fingerprinted the model and the views through `serde_json`. They now use `Debug`. `format_md` was rejected as the replacement, because it skips the virtual views and sorts the data, so it would have been a weaker fingerprint. The first version of this paragraph said `Debug` was "no weaker" because it renders every field, including the derived `formula_cache` the JSON skipped. **That was wrong (improvise-pc3b).** A derived `Debug` prints a `HashMap` in hash order, and `recompute_formulas` clears and refills the cache. So a correct `ViewOnly` effect such as `RebuildLayout` changed the fingerprint in 90 of 200 runs. The suite passed only because the fixture had no formulas, so the cache was empty. `Model` now has a hand-written `Debug` that renders `formula_cache` and `measure_agg` in key order. The fingerprint keeps every field, including the derived one, and a refill in a different hash order is no longer read as a change. `recomputing_formulas_leaves_the_model_fingerprint_alone` pins this. `view_only_effects_leave_shared_state_alone` now runs over a formula-bearing fixture, for the same reason. **That fix was canonical within one instance, not across two (improvise-djcn).** It fixed `Model`'s own maps but not the maps inside its fields. `DataStore` has `index`, `records` and the symbol table, and `View` has `page_selections`, `hidden_items` and `collapsed_groups`. All still printed through a derived `Debug`, in hash order. Each `HashMap` gets its own `RandomState`, so two equal documents built separately printed differently in 32 of 32 runs. - The effect-classification tests compare one instance with itself before and after, so they were unaffected. - `a_model_command_lands_where_its_keypress_lands` compares **two** `App`s. It passed only because its fixture had no cells and no multi-entry view maps. Now: - `DataStore`, `SymbolTable` and `View` have hand-written `Debug` impls. Each sorts wherever its storage is a hash map. - `DataStore` resolves symbols to their strings. A `Symbol`'s number is interning order, not content, so printing it would make two equal stores differ again. - `SymbolTable` prints its interned strings, sorted. - `cells` keeps its `IndexMap` order, because that order is part of the document (gotcha 18). - Every hand-written `Debug`, `Model`'s included, **destructures** `self`. A field added later is then a compile error there, where a `debug_struct` that names fields one by one would skip it silently. The pc3b version of `Model`'s impl had exactly that hole. `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails `f64::from_str` (overflow saturates to ±inf), but if the two ever drift the mismatch must **surface** — never silently store a wrong number. --- ## Persistence write order and the virtual views `format_md` writes Views → Formulas → Categories → Data: views first because they are typically small and orient the reader, data last because it is the largest section. The parser tolerates any order (see the two-pass note above). `_Records` and `_Drill` are scratch views the app rebuilds on demand. They put `_Index`/`_Dim` on axes, and those categories are never written — so persisting the views would produce axis lines naming categories the loader will not find. They are skipped on write, and **dropped on load** if an older file carries them, rather than reinstating a view the app would rebuild anyway; `Initial View` then falls through to the default, since the name it pointed at is gone. `persisted_initial_view` is the writing half: saving from records or drill mode falls back to the first real view rather than naming one the file will not contain (improvise-60z). `_Measure` persists only its non-formula items — formula targets are rebuilt from the `## Formulas` section, so `format_md` collects the targets and excludes them. `Page` and `Filter` both carry the selected item after the axis keyword; the other axes have nothing to select. `Axis::Filter` round-trips even though it occurs only inside the never-written `_Drill` view, because the alternative was silently writing a Filter axis as some other axis (improvise-dz8, gotcha 27). --- ## Import: order is a correctness property, not tidiness `analyzer::analyze_records` emits distinct values in **first-seen order**, never `HashSet` order (improvise-7yx, gotcha 18). `build_model` adds items in exactly that order and `IndexMap` preserves it, so a hash order made the *same CSV* import to a different document on every run — different `.improv` bytes, different row order on screen, and a different detected date format, since `detect_date_format` only inspects the first ten samples and *which* ten it saw varied. The `HashSet` in `analyze_records` is there to deduplicate and **never to iterate**. ### The ambiguous-date heuristic reads American-first, deliberately `01/02/2025` parses under both `%m/%d/%Y` and `%d/%m/%Y` and nothing in the data can tell them apart. `detect_date_format` is first-match over `DATE_FORMATS`, so the American reading wins. That is *a* defensible answer and not obviously the right one: a European CSV imports with days and months swapped, **silently**, and the derived `_Month`/`_Quarter` categories inherit the error. What makes it tolerable is that the wizard shows the detected format and lets the user change it — the same argument two-digit years already rested on. It is pinned by a test so that changing the answer is a decision somebody makes rather than a side effect of reordering the table. **Ruled a known limit** (improvise-l8rh, the user, 2026-09-20). Two things the ruling rests on. Changing the default to day-first only moves which half of the world imports silently wrong; and the honest fix is larger than the heuristic — an ambiguous column should *surface* its ambiguity rather than resolve it, which changes `FieldProposal`'s shape and the wizard step that displays it, the way `Dataset::warnings` returns a decision the user should hear about as data. Worth doing deliberately, not as a patch. What makes the limit reasonable to live with is that it is now *deterministic*: `analyze_records` iterated a `HashSet` until improvise-7yx, so the ten samples `detect_date_format` inspects were a different ten every run and a column whose eleventh value ruled out `%m/%d/%Y` detected a different format run to run. The ten-sample window itself is unchanged and is part of the limit. ### CSV parsing Short rows keep every header column, missing trailing fields filled with `Value::Null`, and each affected column is reported as a warning with a row count. The reader is `.flexible(true)`, so without this a short row parsed cleanly and the trailing columns were simply *absent* from the record map — no error, no warning, and skewed value counts in field-kind analysis (improvise-k8i). A header name becomes a **category name** and survives into the `.improv` file, so anything riding along invisibly in one — a byte-order mark, a stray carriage return — is silent corruption rather than cosmetics. That is why the adversarial inputs are tested with *controls*: a check that cannot see a BOM where there is one proves nothing. `parse_csv_bytes_with_warnings` exists so CSV import can come from any `Storage` backend rather than only a file (improvise-fuj); the path-taking wrappers remain for the native CLI. ### A folder of CSVs is one sparse model, with a view per file (improvise-lpzu) `improvise import ` exists for improvise-1xfb.5.1. code-maat writes one CSV per analysis into a directory, and the schemas differ: coupling has `entity,coupled,degree,average-revs`, age has `entity,age-months`, and abs-churn has `date,added,deleted,commits`. The user ruled (2026-09-25): "Records are already sparse: the model can just leave null/empty values for columns not in a given CSV file." So files with different headers merge into **one** model. It is not refused, and it is not split into one model per schema. **A folder argument means exactly its `*.csv` files.** `expand_import_paths` takes the arguments in order and replaces each folder with its `*.csv` entries. The extension check is case-insensitive, the listing is not recursive, and the order is `Storage::list`'s, which is sorted by contract. Then the existing path runs, with one difference: **a folder always merges**. Two or more files merge through `merge_csvs_from` with a `File` category, and so does a folder holding a single CSV. That gives the one file its `File` item and its per-file view. The single-CSV rule is **the user's ruling (2026-09-25)**, which replaced this PR's first answer, "`import dir/` is exactly `import dir/*.csv`". Their words: "yes, we might want to have some kind of update import eventually (don't plan this yet, but we'd want file identity for this)". A folder names a collection that may grow, so a document built from it should record which file each record came from from the start. **A single CSV named directly, not through a folder, still imports alone, without `File`.** That is deliberate. Naming one file asks for that file, not for a collection, and the shipped recipes (`examples/demo.improv` from `examples/demo.csv`) and every existing single-file script would otherwise gain a `File` category and a second view. File identity for update-import can be asked for by naming a folder. The test is `a_folder_of_one_csv_still_gets_a_file_category_and_its_view_but_a_lone_file_does_not`. Apart from that, a folder still behaves as the glob would, and four cases follow from that rather than being chosen one by one: - **Non-CSV entries are skipped**, not refused, because a glob skips them. This includes the output `.improv`, which a second run finds in the folder. - **A folder with no CSV at all is refused**, naming the folder. A glob that matches nothing is an error too, and importing nothing would write an empty document. - **Files and folders mix freely**, in argument order. - **A file named twice is read twice.** For example, `dir/a.csv dir/` reads `a.csv` twice, and its records collide with themselves. The headless path refuses that like any other collision. Nothing deduplicates, because the shell glob would not either. The listing goes through `Storage::list` so the browser host can reuse it (gotcha 2). `Storage` knows documents, not folders, so **whether an argument is a folder is the caller's question**. The CLI answers it at the edge with `Path::is_dir` and passes an `ImportPath::{File, Folder}`. An OPFS host would answer it from its own UI. **Sparse merge.** A record from a file without some column has no key for it, so `Plan::place` gives it no coordinate there (improvise-8jk6.2 above), and no measure cell. Nothing is fabricated: no `0` and no `""`. The analyzer already classified each column over the values present (improvise-l1f5), so a column only one file has is judged by that file's values alone. The category cap counts the **union** of every file's categories, header-only files included (see the next paragraph). Rows from different files cannot collide, because `File` differs. One caveat on "judged by its values": a JSON `""` is not yet dropped by the analyzer the way `null` is, so a JSON numeric column holding `""` is proposed as a Label (improvise-8jk6.11). CSV is unaffected, because its parser turns a blank field into `null`. **A header-only file is still a file** (the Codex P2 on PR #314). code-maat writes a header and no rows whenever nothing passes its threshold, so `coupling.csv` is often exactly that. The pipeline is analyzed from *records*, and such a file has none, so it had no `File` item, its columns never reached the model, and its view filtered to an item that did not exist. `ImportPipeline::from_merge` therefore **seeds from `Merged::sources`**: - the `File` proposal's items are the sources' items, in file order, so every file has its item whether it has rows or not; - a source column that no record has a value in is proposed as a **Label**. That is the analyzer's own verdict for an all-null column (improvise-l1f5), and a column with no values is exactly that: nothing can be classified, so it takes no pivot slot and does not count toward the category cap until the user reclassifies it in the wizard or with `--category`/`--measure`. This is also the file identity the user wants kept for a future update-import. **A file with no measure column gets a records view** (improvise-8jk6.13). A record with no measure makes no cell, so such a file contributes items and no data. Its view had nothing to page `_Measure` to, and paged onto another file's measure, which the same view hides; the cell there was a `Record`, editable, under this file's `File`. Now `source_view` gives such a file the records layout (`view::records_steps`, the one home of what a records view is) with `File` still on Filter at the file's item. It draws zero rows, because there are no cells. A header-only file whose columns are all Labels is the same case. In records mode the columns are the whole model's categories and measures, not just this file's: records mode has no per-view column filter. With no rows that is cosmetic. **Each file gets a view** (the user, 2026-09-25: "dir import should do this: one view filtered by the file column and projected to the columns in that file"). `merge_csvs_from` returns a `Merged` whose `sources` list each `File` item with the columns its header named. Two files with one stem share an item, so their columns are unioned. The pipeline carries `sources`, and the build adds one view per source after the merged `Default` view, which stays the initial view. Each per-file view: - **Is named after the file's stem.** A name already taken, including the virtual `_Records` and `_Drill`, gets ` 2`, ` 3` and so on. A view named `_Drill` would never be written and would be dropped on load. - **Puts `File` on `Axis::Filter`, fixed at that file's item.** Filter and not Page, because the view *is* that file's projection. Paging to another file would show that file's records through this file's columns and hidden measures, which is wrong on every page but one. Filter constrains without being navigable (gotcha 27). Until now it appeared only in the never-saved `_Drill` view, but the `.improv` grammar has always round-tripped it. `per_file_views_survive_a_round_trip_and_repeat_byte_for_byte` proves that on a saved per-file view, and `a_saved_per_file_view_projects_only_its_file` proves the shared `grid_in` projection, which both the TUI and the browser draw from, carries it as a fixed coordinate with nothing to cycle. - **Seats the file's own categories** (its columns, plus date parts derived from them) through `View::on_category_added`, in model order: the rule a single-file import uses. Every other category goes on `None`. The file's records lack those, so nothing splits. - **Puts `_Measure` on Page**, as `Workbook::new` does, **with other files' measures hidden** and the page selection on the file's first measure. A formula target comes from no file, so it stays visible. A file with no measure gets a records view instead (above). - **Is never named like another view from the same import.** `Default.csv` and `Default 2.csv` become `Default 2` and `Default 2 2`; without the same-batch check both would be named `Default 2` and one view would silently replace the other. A per-file view is made only while `File` is an accepted category. A user who skipped or reclassified it has no filter to build views from. Two things this does not do. Page cycling still steps onto a hidden item and draws an empty grid: `[`/`]` in a per-file view can reach another file's measure (improvise-yd2t; the Default view already behaved this way). And a CSV with its own `File` column has it overwritten by the merge's, which is the existing behaviour of `merge_csvs`. ### The wizard's back path is a history stack `advance` pushes the step it leaves; `previous` pops it. Keeping the path actually taken, rather than re-deriving the reverse of each conditional transition, means back can never disagree with how the user really got here (improvise-907). Preconditions live in `validation_error()` and are checked **on the step where the user can still fix them** — never deferred to `build_model` after the final confirm. Two gates earn that: too many categories (improvise-mzv) and no category at all (improvise-gfq); both are fixable with `c` at `ReviewProposals`, and both used to refuse only at the last keystroke. `MAX_CATEGORIES` here is the model's own constant, which counts regular categories only. Until improvise-felo.1 it was a private copy (core.md §9). `NameModel` is terminal for `advance`: leaving it is a **build**, not a step change. `build()` parks the workbook in `built` on `Done`; a build failure leaves the user on `NameModel` with the error. Going back from `Done` discards `built`, so accepting an import always reflects the latest field decisions. `scalar_name` decides what a JSON scalar contributes as an item name. `Value::Null` — which is also what the CSV parser produces for a blank field — and a blank string are **absences, not names**; stringifying them used to invent an item literally called "null" (improvise-ych). Every other value keeps its printed form. **A record with no value for a category keeps its cell, without that coordinate** (improvise-8jk6.2). Records are sparse: a `CellKey` names only the coordinates it has. So a missing field, a `null` and a blank string all mean "no coordinate here". None of them makes an item: not `"null"`, not `""`, and no synthetic placeholder. The same rule covers labels, which used to become an item named `""`. The user's words, 2026-09-25: "this limit in import should be fixed, it dates to when this wasn't sparse." That limit was improvise-5j9's rule: such a record "has no place in the grid" and was dropped, counted and reported as `Skipped N of M records missing category values`. 5j9's warning made the loss visible, and the loss is now gone, so the warning went with it. The only record still skipped is one that is not a JSON object. Where such a record shows up is decided by the axes, not by the import. The test is `a_record_with_no_coordinate_is_drawn_only_where_that_category_is_off_the_grid`: - **The category on Row or Column:** no position holds the record. Its other coordinates may still draw a row, but the cell under the category's items is empty, and the column total leaves it out. - **The category on Page:** no selection shows it. - **The category on None:** it is aggregated like any other record. - **Records mode:** its row shows a blank cell in that category's column. - **`.improv`:** its Data line omits the category, and it round-trips. - **Drill:** a drill that does not constrain the category reaches it. **No "(none)" position, by the user's decision (2026-09-25).** They were asked whether Row, Column or Page should give such records a place of their own. Their answer: "I think leaving it out for now is fine, maybe a UI element somewhere that shows the number of records underneath the current view?" That indicator is improvise-gtn7.10, and it is not an import concern. ### Colliding records are a question, not a default (improvise-cgqt) Two source records that land on the same coordinate tuple are one cell, so something has to happen to the second one. improvise-2cw9 made that "sum and warn". The user ruled on 2026-09-25, after an Etrade import summed 31 cells and reported it in one truncated line, that the **wizard asks**. It offers three choices: (a) sum them, (b) keep them separate with a generated category, or (c) go back. So `ImportPipeline::on_duplicate` is a `DuplicatePolicy` (`Sum | Separate(name) | Refuse`), and its default is **`Refuse`**. Any caller that never asked therefore gets an error and never a silent sum. `ImportJsonHeadless` builds its pipeline as a struct literal, so the compiler made it choose too. It chose `Refuse` and points at `:import`. **Detection comes before the build and shares the build's placement.** Where a record lands is `Plan::place`: its categories, then its date parts, then its labels, then its measure values. Only a record that is not an object is dropped, since a missing value is no coordinate rather than a reason to drop (improvise-8jk6.2). `collisions()` and `build_model_with_warnings` both call it, so the question the wizard asks and the thing the build does cannot disagree. Neither has a second walk of the record. **A collision is a cell that more than one record writes**, meaning a tuple plus a measure (improvise-9bit). A `Collision` names the tuple and only the records that share one of its cells. A record with no measure makes no cell, so it collides with nothing. The tuple includes label values, as `CellKey` always has. This PR's first version keyed on the tuple alone. It was **superseded in review (improvise-9bit)**, and the argument it gave is withdrawn. Under that rule, two records with **disjoint** measures on one tuple (one has `Revenue`, the other `Cost`) counted as colliding although no cell sums. The wizard then offered "Sum them into one cell" where nothing would sum, and with the `Refuse` default the CLI refused a sparse-measure CSV that had always imported cleanly. The question the user is asked is "these values would be added together", so the rule has to be the one that decides whether anything is added. The data is structured. A `Collision` holds `coords` plus 1-based `records` positions. `WizardCache::collisions` carries it, and `WizardCollision::describe` is the one wording, used by the TUI and by the CLI. A browser wizard must draw the same rows (gotcha 32), not a second sentence. **Where the step sits.** It comes after `ReviewProposals` and after `ConfigureDates` when that step is visited, and before `DefineFormulas`. It appears only when `collisions()` is non-empty, and the list is recomputed each time the step is entered, because it depends on the field choices. The two field steps are exactly the steps that can change it. Date parts are functions of a value that is already a coordinate, so they can never split or merge a collision, but they count toward the category cap, and that is why the question waits until they are known. The history stack (gotcha 22) handles the new paths without change: `ResolveDuplicates → NameDuplicateCategory → DefineFormulas` retraces backwards. The chooser acts **on the cursor**. Enter on this step is `ChooseDuplicate`, never a bare `Advance`: the policy starts at `Refuse`, which `validation_error` blocks, so a bare advance could never leave the step. **Separate is an occurrence index, not a row number.** This is the user's preferred shape. The records in a collision get `1, 2, 3…` in source order, and every other record gets `1`. That includes a record on the same tuple that shares no cell with the others: it lands on occurrence `1` beside the first colliding record, and since the two write different measures, nothing sums. The category holds only as many items as the deepest collision, rather than one item per record, and every existing coordinate keeps its meaning. Items are added `1..=n` in numeric order (gotcha 18). The category goes on **`Axis::None`**. Left to `on_category_added` it would land on `Page`, and the default grid would then show only occurrence `1`, which silently hides the other records. On `None` the grid aggregates across occurrences, so the first screen shows the same totals Sum would. The records stay distinct, so `Avg` and records mode see each one. A policy is **moot when nothing collides**: no category is added. The name is **suggested and editable**: `Occurrence`, or the first `Occurrence_N` that no header (accepted or not) and no category the build will create already uses. `NameDuplicateCategory` refuses an empty name or one that is already a category. `suggested_duplicate_category` lives on the pipeline, so the CLI's `separate` uses the same rule. **At the category cap, Separate is shown and disabled.** The reported import had 12 regular categories, which was `MAX_CATEGORIES` when this was written. Option (b) adds a **regular** category, and the user declined a hidden virtual one. So at the cap the option is drawn "(unavailable)" with its reason and remedy: go back and Skip a column or make it a Measure, or choose Sum. The reason is decided in `ImportWizard::duplicate_options`, so every front end words it the same way. This was the conservative reading, and it left an open question for the user. **The user answered it on 2026-09-25 by raising the cap to 24** (improvise-felo.1), so "keep separate" is available for the reported import now. The option is still disabled for any import that fills the new cap. **Headless paths refuse by default.** `improvise import --no-wizard` has nobody to ask, so `--on-duplicate refuse|sum|separate` defaults to `refuse`. It prints every colliding tuple and the flag, exits non-zero, and writes nothing. Summing by default would repeat exactly the silent outcome the user called critical. Separating by default would reshape a scripted import's document and fail outright at the cap. A refusal is the one default that cannot be wrong without saying so. This changes behaviour: a script that relied on 2cw9's implicit sum now needs `--on-duplicate sum`. `:import-json` has no flag and refuses, naming `:import`. The 2cw9 argument that summing is the right default is **superseded** here, as its own evidence entry anticipated. Its warning survives as the Sum confirmation. --- ## CSV export `render_csv` is separated from writing so the bytes can go to any `Storage` backend, not just a file (improvise-fuj). It goes through the `csv` crate rather than joining with commas, and it honours the view's number format — a value of 1200 under a comma-grouping format renders `1,200`, and a label like `Smith, Jr.` splits into two fields, corrupting every row (improvise-1cz).