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