Both findings from ChatGPT Codex's review of PR #125. Both are real, and both
are the same mistake in two places: answering "whose format is this" from
something other than the rule that answers "whose value is this".
**1. `FormatPlan` indexed formats in a `HashMap` keyed by target.** A target
category need not be `_Measure` — `[CustomCat]` is in the format's own
documentation — so a key can carry coordinates two formulas claim, and
iterating the map answered from whichever category hashed first: the displayed
format was nondeterministic per process while the value was not. That is
gotcha 18's shape. Worse, only *formatted* formulas were indexed, so a later
formula's `FORMAT` reached a cell whose value an earlier unformatted formula
produces.
`FormatPlan` now holds every formula's `(target_category, target,
Option<format>)` in the model's own order and takes the first coordinate match
— `Model::formula_for`'s rule and order exactly. A match naming no format falls
through to the view; it is a match, not a miss. The list is dropped entirely
when no formula names a format, so the common document still pays one
`is_empty` per cell.
**2. A column total took its format from row 0.** `context/code/core.md` argued
that was safe because "where `_Measure` is on the row axis a column is not a
measure at all". Wrong: a formula's target category is any category, so a
formula can own a row-axis item — the total then inherited a row-local override
for a row it had summed away, and re-ordering the rows changed its spelling.
New `GridLayout::column_key(col)` gives the coordinates that survive the row
aggregation (page/filter constraints plus the column's tuple), and `col_formats`
is resolved from that, once per column, outside the row loop.
Three regression tests, each with the rejected version injected as the fault:
- `the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none` puts
the same two formulas in both orders, so preferring the formatted one fails
one half and ignoring formats fails the other.
- `the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does` rebuilds
the plan five times, since each `HashMap` gets its own `RandomState`.
- `a_column_total_does_not_inherit_a_row_local_format` builds one grid with the
owned row first and one with it last and requires the strips to agree. Every
raw cell holds the same value per measure, so the totals are numerically
identical and only the format can differ.
`context/code/core.md`'s wrong paragraph is replaced rather than amended, and
says what it got wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded
`,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause
is the format, not the arithmetic; exact decimal math would not change the
rendering by one character, so nothing here touches the evaluator and gotcha 8
is unchanged.
Two halves, per the user's ruling:
* `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The
noise on whole numbers is the cheaper error — a currency column with two
redundant zeroes is legible, a ratio with none is wrong — and a format cannot
be guessed from a value.
* A formula may carry its own format, which wins for its target; a raw measure
keeps the view's. `formula.pest` gains
`format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s
reason (a predicate is invisible to the grammar-walking generator) and
`FORMAT` added to `quote::KEYWORDS`.
`improv.pest` is deliberately unchanged: `formula_raw` already carries the
`## Formulas` line verbatim, so the format rides in the formula text with
nothing to add — and the generator, which reads `improv.pest`, sees no new
spelling to violate.
Resolution has one home. `FormatPlan::of(model, view)` replaces the
`(comma, decimals)` pair every render path threaded — `GridLayout::display_text`,
`viewport::grid_body`/`compute_col_widths`, `project::grid_in` and
`persistence/export.rs::render_csv` — rather than being added beside it, which
would be two answers to "what format is this cell". It is asked of the cell's
`CellKey`, so a cell aggregating over `_Measure` keeps the view's format by
construction. `GridBody::col_formats` carries the answer to the totals strip,
which is formatted on its own path.
Persistence is untouched except for metadata: `format_f64` is display,
`persistence/format.rs::format_number` writes round-trip precision, and the two
have no call between them. Newly created views write `format: ,.2`; existing
documents are unaffected, because `format_md` has always written the line
explicitly. `web/demo.improv` moves to `,.2` on both views.
The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a
`FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip
test therefore asserts the parsed field — injecting a walker that drops the
clause fails that one test and nothing else in `improvise-io`, 242 tests
including the whole characterization suite and the grammar properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
The code-review skill run against this branch, and the findings are about my
own tests rather than the code.
1. The formula sweep's predicate is (target_category, target), and nothing
discriminated the first half: a sweep on `target` alone passed every test
I had written. Two categories may legally hold a target of the same name,
so removing Metric/Profit must not take _Measure/Profit. Proved by
injecting `retain(|f| f.target != item_name)` and watching it fail.
2. Both `cycling_an_axis_...` tests read the status off `last()`, which a
resolver emitting the status and NOT the mutation also satisfies. Both now
assert the mutation is first.
3. Two things the fix leaves that nothing guards, both filed and both written
down rather than left in a PR body. improvise-nwgk:
doctrine(a_status_line_composed_beside_a_mutation_lies_when_the_mutation_no_ops)
asks a fallible mutation to RETURN whether it acted, and remove_item still
returns unit -- the reachable lie is gone, a wire RemoveItem naming nothing
is not. improvise-en4s: Dirty::of(CellData) reporting formulas: true was
slack over-reporting and is now load-bearing, because a delete can take a
formula out of the document; narrowing that arm would strand every client's
formula panel forever.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
context/code/core.md gets the two model rules (why removing a name removes
the formulas targeting it, and why a required virtual category refuses) and
the axis rule (why leaving the constraining axes forgets the page selection,
and why the predicate is filters_data rather than 'is it Page').
context/code/clients.md gets improvise-koh6's decision in full -- including
why the state-dependence rule is about Upstream and not about outcomes --
plus the cursor rule and the shared axis sentence.
Evidence entries are module-qualified where they must be: three leaf names
are answered by a test in both resolve.rs and panel.rs, and one in both
model/types.rs and workbook.rs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-s08t: eval_agg narrowed its partial key only where
find_item_category resolved, so SUM(Typo) left the key unconstrained and
aggregated every numeric measure matching the context. An unknown name now
errors ref:{name} before the raw fast path.
improvise-s2w3: the same fast path assumed a bare Ref names a STORED cell. A
formula target has no raw cell behind it (improvise-nrs), so SUM(Double)
summed nothing and answered 0 where SUM(Double + 0) folded and answered 20.
core.md §13's defence of the specialisation holds — what failed is its
premise — so eval_agg asks formula_for, the one home of "is this cell
derived", and a derived ref falls into the fold.
improvise-7iww: recompute_formulas stopped at MAX_EVAL_DEPTH and left the last
iteration in formula_cache, so a circular formula read Error("circular")
directly and Number(26) through the cached path — which is the DISPLAYED path,
since default views put the virtual categories on Axis::None. Cells still
moving when the bound runs out are now marked circular, which is the same
bound evaluate_depth already enforces.
improvise-vyrx: eval_bool_cached wildcarded BinOp where eval_bool lists its
arms. Verified by adding a BinOp::Mod: four sites now fail the build where
three did before.
improvise-pikk: round_half_away scaled by 10^decimals before truncating, so
format_f64(1e307, false, 2) rendered "inf" for a finite number.
improvise-f86q: add_category checked MAX_CATEGORIES before contains_key, so
re-adding an existing name at the limit was refused with a false message — and
per gotcha 21 aborted the rest of the batch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
An adversarial review of this branch found that the prose was worse than the
diff — including, twice, in the exact genre the branch was congratulating itself
for catching. Recorded rather than quietly rewritten, because that is the shape.
Must-fix, all verified before correcting:
- `fit.rs`'s comment on the exact check still read "this is the check that
enforces the bound in both directions of a drift" — verbatim the sentence
gp.md §14 now spends 25 lines refuting. Gotcha 48's third clause exactly: a
fact stated twice is changed by grep, not by edit. I refuted the copy in the
prose and left the copy in the code.
- "false for eight months" was fabricated. `encoded_width` landed 2026-09-19,
the sentence 2026-09-20; the repository itself is six months old. A paragraph
whose point is that unchecked cost claims are checkable closed on one.
- "NumericCategory was the one artefact family with no coverage" is false: it
was one of three, and `ValueOfAMeasure` and `AbsentCoordinate` still have
none. Corrected in both files that said it, and the remaining gap is named.
The decline's stated reason was wrong, which matters most because a decline is
the artefact nothing checks. It claimed one-hot is the only encoding of a
category whose splits are spellable; `is_expr` is a `primary`, so an ordinal
split IS spellable as a weighted indicator sum. Verified by running
`parse_formula`, not by re-reading the grammar, and now pinned by
`an_ordinal_split_on_a_category_is_spellable_as_an_indicator_sum` so the
paragraph cannot go quietly wrong again. e8no stays declined on grounds that
survive: a category's items carry insertion order rather than magnitude, and the
emitted formula spells one IS per item either way, so the width is not saved.
Also weakened two overclaims — "marking it a Label yields the same fit" (false:
the feature count moves the RNG draw sequence, gotcha 37b) and "asserted rather
than argued" (the dichotomy is argued from spell()'s exhaustive match; the test
asserts one instance) — narrowed "the only coordinate predicate is is_expr" to
what is true of it, corrected an uncheckable test count, named which two of the
three consumers of parse_number_format are actually guarded, and moved an
evidence block that had been inserted under the wrong module heading.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`parse_number_format` read the precision as `fmt[i+1..].parse::<u8>()` over
everything after the last `.`, so `,.2f` — the printf spelling the repo-map's
own `.improv` example carries — failed to parse and fell through to zero
decimals, silently, in the TUI, the browser projection and CSV export.
The precision is now the leading digit run after the last `.`, with any
conversion letter ignored. The spellings that are legal are stated once, in
`context/code/core.md`, along with why the trailing `f` is accepted rather than
refused (the grammar takes the format line as an opaque `rest_of_line`, so
refusing it rejects documents this repo already writes) and why the reading is
printf's rather than Excel's (`.00` is the spelling the two disagree on).
`export_csv_quotes_commas_per_rfc4180` asserted the value `starts_with("1,200")`
against a `,.2f` fixture, so it could not tell two decimals from none. It is now
an exact equality, and reverting the fix fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
The parameter was none_cats: &[String] and the body consulted it exactly once,
for emptiness -- a boolean wearing a list's type, since the aggregation runs
over key.0 through matching_values. git log -S back to the commit that
introduced Axis::None shows it never examined an element, so the name promised a
filter the function has never performed.
Aggregation::{Exact, OverHidden} admits only the two states the body
distinguishes, and Aggregation::of reduces the list once at the boundary that
holds it. The regression guard is the signature: two lists with the same
emptiness and different contents is no longer a constructible call.
recompute_formulas and aggregate_raw keep &[String] -- they walk the elements.
improvise-5jm5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
The review re-ran fold_bench across two shapes at the SAME 18,050 cells: the
old code cost 51 ms at 25x361 and 244 ms at 95x95, and after the fix the two
are indistinguishable. A 4.8x spread from group count alone is what a
per-member scan predicts and a constant factor does not, so the spread
vanishing is better evidence for the diagnosis than the absolute number.
Also records the depth asymmetry between the two Agg arms, which bounds a
chain of folded formulas and must not be tidied into one rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third-round review finding, and I verified it against the code before accepting
it. Commit 7d25762's message declined that test on the grounds that a formula
targeting a regular category should project Derived anyway, so the injected
behaviour "is not clearly a fault". That is a different key entirely.
RecordCol::Category(name) carries a CATEGORY name, and RecordRow::cell_key(arg)
puts its argument under _Measure (layout.rs:68-70). So answering Some there
yields `coords + _Measure=<category name>`, and formula_for matches it whenever
a _Measure item happens to share a name with a regular category — projecting
Derived for a COORDINATE column, which is never a formula target and which the
pivot path does not have at all. The two front ends would disagree, which is the
§5 defect this ticket exists to close. RecordCol's own doc names the collision:
the enum resolves a column from its index "so a category and a measure sharing a
name stay distinguishable".
The decision not to write the test is unchanged, and the reviewer's
recommendation too. What changes is the argument: the REACHABLE version of that
fault is a new RecordCol variant answered carelessly, and the exhaustive match
makes that unrepresentable rather than unlikely. A test would pin what the type
system already pins.
Recorded in context/code/core.md because a wrong argument in context/ outlives
the code it describes, and that one would have told a future reader a coordinate
column projecting Derived is defensible. The commit message cannot be corrected
without rewriting history while a review is in flight, so the durable record
says so explicitly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of #54 found the fold reintroducing exactly the divergence the ruling
exists to remove, and the same line turned out to be the 244 ms stall. One
defect, two symptoms.
`eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at
`context.with(cat, name)`. Inside the fold `context` is a MEMBER-RECORD key
carrying the none_cat coordinates, while `formula_cache` is keyed by stems with
none_cats STRIPPED — so the lookup could never hit, fell through to
`aggregate_raw`, found no raw cell behind a formula target, and answered
Err("ref:…"). A routing formula over a derived measure gave Number(11.0)
uncached and Error("ref:Margin") cached, and the cached path is the one a pivot
grid runs whenever any category sits on Axis::None.
`eval_expr`/`eval_bool` are hoisted to module level and BOTH evaluators now hand
the fold the same record-level `eval_expr`. The two agree by construction rather
than by a test. Note the repair not taken: teaching the fold to hit the cache by
stripping none_cats off the member key returns the GROUP's value to every
member — aggregate-then-route through a cache lookup, and silent.
That same per-member `aggregate_raw` was one poorly-narrowed scan per member, an
O(n^2) shape. Measured with the new `examples/fold_bench.rs`, five runs a point,
at 18,050 cells: 51.1-53.3 ms before, 18.1-19.2 ms after, against 7.1-9.0 ms for
a linear formula. Scaling went from superlinear (45x cells, 85x time) to linear
(45x cells, 45x time). Debug 2.75 s -> 132.9-135.1 ms. The fixed point runs
exactly 2 passes, measured by counting, so hoisting the member set has a 2x
ceiling and is declined on improvise-9iyv — a memo keyed by less than the whole
`partial` is a second answer to what a cell depends on.
`Expr::Is` has an arm in each evaluator and the cached one was untested: the
earlier fault was injected into the shared `eval_is`, which both arms call.
Replacing the cached arm alone left the suite green. The first replacement test
ALSO stayed green, which is the interesting part — once the fold routes through
`eval_expr`, an `Is` inside a SUM never reaches the cached arm, so that arm is
reachable only by a bare `Is` outside an aggregate. That is what the new test
uses, and injecting into that arm alone now fails it.
`IS` gained a word boundary: `Region ISLAND` parsed as Is(Region, "LAND"),
silently wrong where it was a parse error. `is_expr` is compound-atomic with
`sp+` written out, because a negative lookahead in an ordinary rule tests the
character AFTER pest's auto-inserted whitespace and fails backwards. The
lookahead was then redundant and removed — deleting it left the suite green,
while relaxing `sp+` to `sp*` fails the test, so `sp+` is the mechanism. `WHERE`
has the same hole, is far less exposed, and is filed as improvise-r08u.
Also: `ambiguous.sort_unstable()` before `dedup`, which only removes consecutive
duplicates, so the refusal could name a column twice.
Every fault re-injected after the refactor: no-fold 10 failures, shared eval_is
5, record-level Is arm 4, cached Is arm 1, feature_roles order 2, context-vs-
partial 1, the new cached-fold divergence 1, `sp+` relaxed 1. Baseline 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-4zl7. A tree is fitted on individual records; an improvise formula is
evaluated at a CellKey, and an aggregating cell used to aggregate each Expr::Ref
first and evaluate the IF once on the totals. That is the same answer for a
linear formula and a different one for anything that routes, so every fitted
tree meant one thing on a record row and something else one row up. The user
ruled: aggregate the rule over the members.
The rule needed no new aggregate vocabulary. SUM(e) already means "fold e over
the members" and simply did not: eval_agg specialised Expr::Ref and, for every
other inner expression, summed the raw matching cells and ignored the expression
entirely. It now folds — one entry per member record, the expression evaluated
at that record's own coordinates. eval_agg is shared by eval_expr and
eval_expr_cached, so the fold has exactly one home; each evaluator passes its
own recursion in.
Expr::Is(Filter) is the new arm in both evaluators: `Cat IS Item` is 1.0 when
the cell's own Cat coordinate is Item, keeping the evaluator f64-only and
reusing Formula::filter's own context.get/unwrap_or(false). A grammar production
in primary, before ref_expr, with both operands through
improvise_formula::quote's one home; IS joins the keyword guard.
The emitter reads feature index -> role off preprocess's plan rather than
reconstructing it: gp_tree::data::feature_roles is the one statement of the
expansion order and encode_row now drives off it too. improvise_gp::Features
carries one Spelling per feature — Measure, Coordinate or Artefact — decided by
role and column, never by the name's shape. A one-hot over a real category is
emitted as `Region IS North` and accepted; col.missing and the date parts stay
Artefact and stay refused (improvise-wd6n).
fit-measure on a document a category explains now lands instead of refusing, and
the refusal no longer depends on which features the search happened to pick.
Evidence: eight hand-computed tests in improvise-core over a four-record fixture
whose Cost straddles the threshold inside each region, so route-then-aggregate
and aggregate-then-route differ at every level. Two faults injected and watched
to fail — reverting the fold fails 7 of 8, and a constant-false eval_is fails
exactly the 3 coordinate tests. Reordering feature_roles fails both it and
encode_row_replays_the_training_expansion.
The fold is O(members) per evaluation per fixed-point pass and unbenchmarked:
improvise-9iyv.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-p1ym. GridCache::page_coords carried (category, selected item), so
PageNext/PagePrev resolved to a command name the browser held but could not
compose an argument for. They reached Host::handle, which answers an empty vec,
so a smoke test found them inert on the live page.
The item list is not a second field beside the selection: page_coords becomes
Vec<PageAxis>, and `items` is Some exactly when the axis is Axis::Page. That
puts gotcha 27's split in the type — a Filter axis constrains without being
navigable, so it carries no list and no caller can compose a cycle for one.
GridLayout::page_coords stays filtering_cats(), which is right for the bar,
and navigation.rs::page_cat_data stops spelling the Page-vs-Filter rule a
second time: it reads the layout's list.
The browser's page_cycle mirrors PageNext/PagePrev line for line, odometer
carry included, and `[`/`]` leave the still-needs-the-host list.
Two decisions stated in context/code/core.md. The list is
effective_item_names — the same call that already chose the default selection,
so selection and list come from one place; page_cat_data used
ordered_item_names before, which for a _Measure page axis let the default
selection be a formula target `[`/`]` could not reach. And the list is NOT
filtered to visible items: the TUI cycles hidden ones, and making the browser
differ would be the divergence rather than the fix.
The lockstep test's first version was vacuous — it rebuilt the client's cache
from the host and compared it against that same host. It now drives two
independent documents and compares View::page_selection; the injection that
reverses the odometer's significance now fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-core's comments are the oldest in the repository and carry the most
expensive artefact in it. This lands the destination before the cap, so the
next commits are a move and not a deletion.
context/code/core.md carries the chains that cannot survive two lines: the
ordering-is-correctness argument for DataStore (improvise-shyt/2yc2), the
unanimity rule and its branch order (improvise-l4qx), the surrogate width and
byte-format argument (ADR 002), the wide-records column vocabulary, the
virtual-on-an-axis rule (improvise-2zq), and the two-evaluator hazard behind
improvise-0bf.
context/test-evidence/core.md carries the bug narratives, keyed by exact test
name, per design-principles §7. 52 entries, all verified against
`--check-evidence`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Edward Langley