# `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 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. The same reasoning makes
the final `else` arm of each dispatch unlabelled-but-safe (`ref_expr` is the
only remaining alternative of `primary`, `COUNT` the only remaining `agg_func`,
`>=` the only remaining `cmp_op`).

`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<T>` 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<dyn Storage>`, 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<OpfsFs>` 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). Legacy JSON is detected by
a leading `{` and is never written.

`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. Filed as **improvise-l8rh**.

### 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.

### 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 mirrors
the model's private limit and counts regular categories only.

`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 missing a value for an accepted Category field has no place in the grid
and is dropped; `build_model_with_warnings` counts it against the *first*
category it is missing and reports the total, because without a warning that
loss is invisible in a thousand-row import (improvise-5j9).

---

## 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).