From 97177112a3d987f7b565e0f3598f342a0795be86 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:48:34 -0700 Subject: [PATCH 1/4] perf(io): the grammar generator re-parsed improv.pest per case, not per run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-8shu. improvise-io's lib binary was the run's outlier at 1.42s against a 0.01s median. The ticket's diagnosis was an unbounded grammar walk generating deep documents; measuring the two halves separately falsified it. load_grammar() x1000 was 2.50s and the walk x1000 was 15.7ms — improv_file() called load_grammar() inside its prop_map, so pest_meta re-parsed the grammar once per generated case. The strategy's input space is unchanged. The grammar is read once through a OnceLock, and a guard counts parses rather than calls so it is independent of which test in the binary runs first. Case counts go 500 -> 256, the default design-principles §7 prescribes and never below it. 1.42s -> 0.21s; improvise-io is no longer the run's longest binary. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/formula-and-io.md | 32 ++++++++++++++++ crates/improvise-io/src/persistence/parse.rs | 39 ++++++++++++++++++-- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index cff43e6a..2622f924 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -699,3 +699,35 @@ The one spelling `sp+` costs, stated rather than discovered later: required its space since improvise-4zl7, so this is the two keywords agreeing. The paired positive arm — the same text with the space — is what stops it degrading into "any WHERE with a quoted category fails". + +### the_grammar_is_parsed_once_however_many_files_the_strategy_yields + +improvise-8shu. `improvise-io`'s lib binary was the run's outlier — 1.42s here +against a 0.01s median, measured at 1.40–1.45s, 1.50s and 3.70–5.24s on three +machines. The ticket's diagnosis was that the grammar walk had no size bound, +so a few deeply nested documents dominated. **That was wrong**, and measuring +before fixing is the only reason it did not ship as the fix: instrumenting the +two halves separately gave `load_grammar()` ×1000 = **2.50s** and the walk +itself ×1000 = **15.7ms**. The walk was 0.6% of the cost. `improv_file()` +called `load_grammar()` inside its `prop_map`, so `pest_meta` re-parsed +`improv.pest` once for every generated case — 1000 times across the two +properties, plus shrink attempts. + +The strategy's input space was never the problem and is unchanged: the same +entropy range, the same 0..N repetitions, the same properties over groups, +formulas, views, hidden items and pipe quoting. The grammar is now read once +through a `OnceLock`. + +The guard counts *parses* rather than calls, which is what makes it +order-independent: other tests in the binary reach `grammar()` too, and the +count stays 1 however many of them run first. It drives `improv_file()` itself +through a `TestRunner` rather than re-implementing what the strategy does, so +a future edit that reverts the call site is what it catches. Fault injected: +`Gen::new(grammar(), …)` restored to `let rules = load_grammar();` — the test +fails with `left: 64, right: 1`. + +Case counts went 500 → 256 in the same file, which is the default +`context/design-principles.md` §7 prescribes and never below it; that is worth +a further 0.39s → 0.21s but it is not the fix, and reporting it as one would +be the mistake this entry exists to record. Net: 1.42s → 0.21s, and +`improvise-io` is no longer the run's longest binary. diff --git a/crates/improvise-io/src/persistence/parse.rs b/crates/improvise-io/src/persistence/parse.rs index e35a02a7..c5408227 100644 --- a/crates/improvise-io/src/persistence/parse.rs +++ b/crates/improvise-io/src/persistence/parse.rs @@ -633,7 +633,7 @@ mod parser_prop_tests { } proptest! { - #![proptest_config(ProptestConfig::with_cases(500))] + #![proptest_config(ProptestConfig::with_cases(256))] /// improvise-pczv as a property rather than a list of clauses. /// Evidence: `context/test-evidence/formula-and-io.md`. @@ -1323,9 +1323,23 @@ mod generator { use pest_meta::parser; use proptest::prelude::*; use std::collections::HashMap; + use std::sync::OnceLock; + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// Counts how often the grammar text is actually re-read; the guard below + /// is what keeps `grammar()` from becoming a per-case parse again. + static GRAMMAR_PARSES: AtomicUsize = AtomicUsize::new(0); + + /// The parsed grammar, read once for the process. Evidence: + /// `context/test-evidence/formula-and-io.md`. + fn grammar() -> &'static HashMap { + static RULES: OnceLock> = OnceLock::new(); + RULES.get_or_init(load_grammar) + } /// Parse the grammar file and return rules keyed by name. fn load_grammar() -> HashMap { + GRAMMAR_PARSES.fetch_add(1, Ordering::Relaxed); let grammar = include_str!("improv.pest"); let pairs = parser::parse(parser::Rule::grammar_rules, grammar) .unwrap_or_else(|e| panic!("Bad grammar: {e}")); @@ -1449,11 +1463,28 @@ mod generator { pub fn improv_file() -> impl Strategy { // Use random bytes as entropy for choices in the grammar walk prop::collection::vec(any::(), 64..=256).prop_map(|choices| { - let rules = load_grammar(); - let mut g = Gen::new(&rules, choices); + let mut g = Gen::new(grammar(), choices); g.generate("file") }) } + + /// improvise-8shu. Evidence: `context/test-evidence/formula-and-io.md`. + #[test] + fn the_grammar_is_parsed_once_however_many_files_the_strategy_yields() { + use proptest::strategy::ValueTree; + use proptest::test_runner::TestRunner; + + let mut runner = TestRunner::deterministic(); + let strategy = improv_file(); + for _ in 0..64 { + let _ = strategy.new_tree(&mut runner).unwrap().current(); + } + assert_eq!( + GRAMMAR_PARSES.load(Ordering::Relaxed), + 1, + "improv.pest was re-parsed per generated case" + ); + } } #[cfg(test)] @@ -1464,7 +1495,7 @@ mod grammar_prop_tests { use proptest::prelude::*; proptest! { - #![proptest_config(ProptestConfig::with_cases(500))] + #![proptest_config(ProptestConfig::with_cases(256))] /// parse(generate()) — every generated file parses without error. #[test] From 173d9820a6714d901b4a956c9418a780165f11f4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:51:17 -0700 Subject: [PATCH 2/4] feat(gp): decline the artefact families separately, in their own words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-wd6n. Spelling::Artefact was one variant standing for five distinct families, the last five reached through a `_ =>` wildcard in spell(). The one refusal they fed said the tree "splits on a col.missing flag or a date part", so three of the five were declined in words naming something they are not. Artefact now names which family, spell() is exhaustive (a sixth FeatureRole is a compile error rather than a sixth thing that message misdescribes), and the refusal gives one clause per family. The two families the ticket asks about are declined in writing in context/code/gp.md §11, separately, because each needs different vocabulary: col.missing needs a word for absence that reaches apply_agg and eval_agg, and the date parts need a date type improvise does not have anywhere. Nothing new is emitted, so improvise-4zl7's route-then-aggregate rule is untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/gp.md | 55 ++++++++-- context/test-evidence/gp.md | 30 ++++++ crates/improvise-gp/src/lib.rs | 185 +++++++++++++++++++++++++++++---- 3 files changed, 242 insertions(+), 28 deletions(-) diff --git a/context/code/gp.md b/context/code/gp.md index a3409c94..703779fd 100644 --- a/context/code/gp.md +++ b/context/code/gp.md @@ -289,7 +289,19 @@ never by the shape of the name: |---|---|---| | `Numeric` | a measure | `Expr::Ref(measure)` | | `OneHot { value }` | a category, `value != "(missing)"` | `Cat IS Item` | -| anything else | | `Artefact` | +| `Numeric` | a category | `Artefact(NumericCategory)` | +| `DatePart` | | `Artefact(DatePart)` | +| `Missing` | | `Artefact(MissingFlag)` | +| `OneHot` | a measure | `Artefact(ValueOfAMeasure)` | +| `OneHot { "(missing)" }` | a category | `Artefact(AbsentCoordinate)` | + +The last five arms were **one** `_ => Artefact` until improvise-wd6n, and the +wildcard was not merely untidy. The refusal it fed read "a `col.missing` flag or +a date part is an encoding artefact", so three of the five families were refused +in words naming something they are not — and a future `FeatureRole` variant +would have joined them silently, declined under whichever family the message +happened to mention. The match is now exhaustive, so a sixth role is a compile +error rather than a sixth thing that message misdescribes. The two exclusions are not fussiness. A one-hot over a **measure** column names no coordinate at all — the record's value for that measure is text, and @@ -326,14 +338,41 @@ flags stay covered without a second copy of `preprocess`'s naming rules. `Artefact` reaching it is a precondition failure and panics, the same split `number_text` makes for a non-finite value. -### The two thirds this still does not close +### The two families improvise-wd6n declines, separately -`col.missing` means "this record has no value for measure X" — a different -vocabulary, and one the formula language has no word for at all. The date -sub-features (`when.year`, `when.month`) need date arithmetic improvise does not -have. Both stay `Artefact`, both stay refused, and both are filed as -improvise-wd6n rather than smuggled in — separately from each other, because -they need different vocabulary. +Both stay `Artefact` and both stay refused. The decline is written here rather +than left open, because each needs a **formula-language** change and neither +needs an emitter change — and they are answered separately because the +vocabulary each wants is different. What changed with the decline is that the +refusal now says which, in that family's own words. + +**`col.missing` is declined because the language has no word for absence.** The +flag means "this record has no value for measure X". `Expr::Ref` over an absent +cell is an `Error`, not a zero, so there is no existing spelling to reuse and no +way to phrase the question as an expression. Closing it means deciding what an +absence *is* in a language whose aggregates already treat it as "no rows" — and +that decision reaches `apply_agg`, `eval_agg` and the text-unanimity rule, not +just this emitter. It is not worth that for a split a fit happens to like. + +**The date sub-features are declined because improvise has no dates.** +`when.epoch_day`, `.year`, `.month`, `.day`, `.dow`, and with a time component +`.hour`, `.minute` — `gp_tree::data::build_date` emits them. A `.improv` cell +holding a date is `CellValue::Text`; nothing in `eval_expr` reads one, and there +is no date type, no date literal and no date arithmetic anywhere in the crate. +Closing this is a type addition to the value language, and the sub-features are +the *smallest* part of it: a user who could split on `when.year` would +immediately want to filter and aggregate by it. + +Both are visible to the user only as a refusal naming the feature and the +reason, which is the right interim: a fit that cannot be stored says so, rather +than storing a formula whose every cell is a reference error (improvise-8xhy). +`improvise-4zl7`'s route-then-aggregate rule (`context/code/core.md` §13) is +untouched — nothing new is emitted, so there is nothing new for it to hold over. + +The other three families are declined permanently rather than pending: a one-hot +over a measure and the `(missing)` pseudo-item name no coordinate *by +construction*, and a numeric category column is a name collision the document +itself creates. No vocabulary closes those; they are refusals, not gaps. ## 12. A fit is persisted as its formula, and nothing else (improvise-7na) diff --git a/context/test-evidence/gp.md b/context/test-evidence/gp.md index 74bf7c89..4be5232e 100644 --- a/context/test-evidence/gp.md +++ b/context/test-evidence/gp.md @@ -577,3 +577,33 @@ value is written out in the test name or the assertion. The third assertion is what stops this one being satisfiable by accident: routing once on the total lands on a single leaf, so the test refuses a fixture in which a leaf happens to equal the right answer. + +### a_missing_flag_a_date_part_and_a_measures_value_are_refused_differently + +improvise-wd6n. `Spelling::Artefact` was one variant standing for **five** +distinct families — `Missing`, `DatePart`, a one-hot over a measure column, the +`(missing)` pseudo-item, and a `Numeric` role over a category column — the last +five reached through a `_ =>` wildcard in `spell`. The single refusal they all +fed said the tree "splits on a `col.missing` flag or a date part", so three of +the five were declined in words naming something they are not. A user fitting +over a text-valued measure column was told about missing-value flags. + +The test drives `Features::of` over a real `preprocess` output rather than +fabricating `Spelling`s — which is the point, since what is under test is +`spell`'s classification, and the helper `artefacts()` beside it fabricates +exactly what `spell` decides. That helper now takes the family as an argument +so a fabricated spelling at least names one on purpose; `Features::of` is still +what really decides, and this test is the one that asks it. + +Each family's assertion has two clauses, and the second is what earns it: the +error must contain that family's reason **and not the other two's**. The first +clause alone passes against one message naming every family at once, which is +precisely the state the ticket describes. Fault injected: the five arms of +`Artefact::reason` collapsed to one shared string — the test fails on the first +family, and with the injection removed all three arms pass. + +The fixture reaches three families from one `preprocess` call (a text measure +column, a measure with a hole in it, a column of ISO dates). The remaining two — +`AbsentCoordinate` and `NumericCategory` — are not reachable from it and are not +asserted here; they are refusals rather than pending gaps, argued in +`context/code/gp.md` §11. diff --git a/crates/improvise-gp/src/lib.rs b/crates/improvise-gp/src/lib.rs index 10986aea..9440ce92 100644 --- a/crates/improvise-gp/src/lib.rs +++ b/crates/improvise-gp/src/lib.rs @@ -24,9 +24,54 @@ pub enum Spelling { Measure(String), /// `Cat IS Item` — a coordinate predicate; the document must have `Cat`. Coordinate { category: String, item: String }, - /// An encoding artefact: a `col.missing` flag or a date part, which the - /// formula language cannot spell at all. - Artefact, + /// An encoding artefact the formula language cannot spell at all. The arm + /// names *which* family, because they are declined for different reasons. + Artefact(Artefact), +} + +/// Why a feature is unspellable. Five families, declined separately — each needs +/// different vocabulary to close. `context/code/gp.md` §11 carries the arguments. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Artefact { + /// `col.missing`: "this record has no value for X" (improvise-wd6n). + MissingFlag, + /// `when.year` and its siblings: date arithmetic (improvise-wd6n). + DatePart, + /// A one-hot over a *measure* column — the record's value, not a coordinate. + ValueOfAMeasure, + /// The `(missing)` pseudo-item: the absence of a coordinate, not an item. + AbsentCoordinate, + /// A category column whose items all parse as numbers; a reference to its + /// name would resolve to a `_Measure` item, which is a different column. + NumericCategory, +} + +impl Artefact { + /// The refusal's own clause — read off a status bar, so one line each. + fn reason(self) -> &'static str { + match self { + Self::MissingFlag => { + "says this record has no value for that measure, which the formula language \ + has no word for: a reference to an absent cell is an error, not a zero" + } + Self::DatePart => { + "is a part of a date, and date arithmetic is something improvise does not have \ + anywhere: a cell holding a date is text, and nothing in eval reads one" + } + Self::ValueOfAMeasure => { + "is a value of a measure rather than a coordinate, so `IS` cannot ask about it \ + — the record's value for that column is text" + } + Self::AbsentCoordinate => { + "is the absence of a coordinate rather than an item, which `IS` reads as false \ + rather than as a question" + } + Self::NumericCategory => { + "names a category whose items all parse as numbers, so a reference to it would \ + resolve to a `_Measure` item of the same name — a different column" + } + } + } } /// One fitted feature: what `preprocess` called it, and how improvise says it. @@ -84,7 +129,13 @@ fn spell(role: &FeatureRole, columns: &[ColumnSpec], categories: &[String]) -> S item: value.clone(), } } - _ => Spelling::Artefact, + // No wildcard: a new `FeatureRole` must be classified rather than + // silently declining as whichever family the message happened to name. + FeatureRole::Numeric { .. } => Spelling::Artefact(Artefact::NumericCategory), + FeatureRole::DatePart { .. } => Spelling::Artefact(Artefact::DatePart), + FeatureRole::Missing { .. } => Spelling::Artefact(Artefact::MissingFlag), + FeatureRole::OneHot { .. } if !is_category => Spelling::Artefact(Artefact::ValueOfAMeasure), + FeatureRole::OneHot { .. } => Spelling::Artefact(Artefact::AbsentCoordinate), } } @@ -98,6 +149,31 @@ pub fn formula_text(tree: &Tree, features: &Features, target: &str) -> String { ) } +/// The refused features grouped by family, each family's names in tree order. +/// Grouping is what lets the refusal give one reason per family, not per feature. +fn artefacts_by_family<'a>(used: &[&'a Feature]) -> Vec<(Artefact, Vec<&'a str>)> { + let mut families: Vec<(Artefact, Vec<&str>)> = Vec::new(); + for feature in used { + let Spelling::Artefact(family) = feature.spelling else { + continue; + }; + match families.iter_mut().find(|(f, _)| *f == family) { + Some((_, names)) => names.push(feature.name.as_str()), + None => families.push((family, vec![feature.name.as_str()])), + } + } + families +} + +/// One clause per family: the names it refused, then that family's own reason. +fn artefact_clauses(families: &[(Artefact, Vec<&str>)]) -> String { + families + .iter() + .map(|(family, names)| format!("{names:?} {}", family.reason())) + .collect::>() + .join("; and ") +} + /// [`formula_text`], parsed **and** checked against the document it is to be stored /// in — the form improvise stores. Why, and what is left: `context/code/gp.md` §11. pub fn fitted_formula( @@ -108,18 +184,14 @@ pub fn fitted_formula( model: &Model, ) -> anyhow::Result { let used = used_features(tree, features); - let artefacts: Vec<&str> = used - .iter() - .filter(|f| f.spelling == Spelling::Artefact) - .map(|f| f.name.as_str()) - .collect(); + let artefacts = artefacts_by_family(&used); anyhow::ensure!( artefacts.is_empty(), // One line, because it is read off a status bar: a `rustfmt` pass over a // multi-line literal leaves runs of ten spaces mid-sentence. - "the fitted tree splits on {artefacts:?}, which the formula language cannot spell \ - at all — a `col.missing` flag or a date part is an encoding artefact rather than \ - anything {target:?} could refer to. Fit over measures and categories for now", + "the fitted tree splits on features the formula language cannot spell at all, so \ + nothing {target:?} could refer to: {}. Fit over measures and categories for now", + artefact_clauses(&artefacts), ); let unresolvable: Vec<&str> = used .iter() @@ -147,7 +219,7 @@ fn resolves_in(spelling: &Spelling, model: &Model) -> bool { match spelling { Spelling::Measure(name) => model.find_item_category(name).is_some(), Spelling::Coordinate { category, .. } => model.category(category).is_some(), - Spelling::Artefact => false, + Spelling::Artefact(_) => false, } } @@ -203,7 +275,7 @@ fn spelling_text(spelling: &Spelling) -> String { Spelling::Coordinate { category, item } => { format!("{} IS {}", quote_name(category), quote_name(item)) } - Spelling::Artefact => { + Spelling::Artefact(_) => { panic!("expr_text wants a feature the formula language can spell") } } @@ -272,14 +344,15 @@ mod tests { ) } - /// Features no formula can spell: a `col.missing` flag or a date part. - fn artefacts(names: &[&str]) -> Features { + /// Features no formula can spell. The family is named rather than assumed: + /// these spellings are fabricated, and `Features::of` is what really decides. + fn artefacts(family: Artefact, names: &[&str]) -> Features { Features( names .iter() .map(|n| Feature { name: (*n).to_string(), - spelling: Spelling::Artefact, + spelling: Spelling::Artefact(family), }) .collect(), ) @@ -607,7 +680,7 @@ mod tests { /// `improvise_eval`, which fabricates an item per feature name (improvise-7na). mod evaluability_in_a_real_document { use super::{MEASURE, TARGET, artefacts, coordinates, leaf, measures, split}; - use crate::{fitted_formula, formula_text}; + use crate::{Artefact, fitted_formula, formula_text}; use improvise_core::formula::parse_formula; use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::types::Model; @@ -723,9 +796,14 @@ mod tests { /// enumerates them: the check asks the precondition `Expr::Ref` has. #[test] fn the_missing_flag_and_the_date_sub_features_are_refused_too() { - for name in ["Cost.missing", "when.year", "when.month"] { + for (family, name) in [ + (Artefact::MissingFlag, "Cost.missing"), + (Artefact::DatePart, "when.year"), + (Artefact::DatePart, "when.month"), + ] { let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); - let err = fitted_formula(&tree, &artefacts(&[name]), TARGET, MEASURE, &document()) + let features = artefacts(family, &[name]); + let err = fitted_formula(&tree, &features, TARGET, MEASURE, &document()) .expect_err(name) .to_string(); assert!(err.contains(name), "{err}"); @@ -765,4 +843,71 @@ mod tests { assert!(fitted_formula(&tree, &features, TARGET, MEASURE, &model).is_ok()); } } + /// improvise-wd6n. The artefact families are refused *separately*, because + /// they are declined for different reasons. Evidence: `context/test-evidence/gp.md`. + mod each_artefact_family_is_refused_in_its_own_words { + use super::{MEASURE, TARGET, leaf, split}; + use crate::{Features, fitted_formula}; + use gp_tree::data::{Overrides, preprocess}; + use improvise_core::model::types::Model; + + /// One document category (`Region`), a text-valued measure, a measure with a + /// hole in it, and a date column — one row per artefact family. + fn features() -> Features { + let header: Vec = ["Region", "Grade", "Cost", "When", "Score"] + .iter() + .map(|s| s.to_string()) + .collect(); + let rows: Vec> = [ + ["North", "A", "40", "2024-01-02", "1"], + ["South", "B", "", "2024-06-03", "2"], + ["North", "A", "70", "2024-09-04", "3"], + ["South", "B", "10", "2024-11-05", "4"], + ] + .iter() + .map(|r| r.iter().map(|s| s.to_string()).collect()) + .collect(); + let data = preprocess(&header, &rows, &Overrides::default()); + Features::of(&data, &header[..1]) + } + + fn refusal_for(features: &Features, name: &str) -> String { + let feat = (0..features.len()) + .find(|i| features.get(*i).name == name) + .unwrap_or_else(|| { + panic!( + "preprocess emitted no {name:?}; it emitted {:?}", + (0..features.len()) + .map(|i| features.get(i).name.as_str()) + .collect::>() + ) + }); + let tree = split(feat, 0.5, leaf(1.0), leaf(2.0)); + fitted_formula(&tree, features, TARGET, MEASURE, &Model::new("d")) + .expect_err(name) + .to_string() + } + + /// Each family's own reason, and **none of the others'** — one message + /// naming every family at once would pass the first clause alone. + #[test] + fn a_missing_flag_a_date_part_and_a_measures_value_are_refused_differently() { + let features = features(); + let missing = "has no value for"; + let date = "date arithmetic"; + let text = "a value of a measure"; + + let err = refusal_for(&features, "Cost.missing"); + assert!(err.contains(missing), "{err}"); + assert!(!err.contains(date) && !err.contains(text), "{err}"); + + let err = refusal_for(&features, "When.year"); + assert!(err.contains(date), "{err}"); + assert!(!err.contains(missing) && !err.contains(text), "{err}"); + + let err = refusal_for(&features, "Grade=A"); + assert!(err.contains(text), "{err}"); + assert!(!err.contains(missing) && !err.contains(date), "{err}"); + } + } } From 22bb621dd84c977e334a44ccb804ceb203807b49 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:56:52 -0700 Subject: [PATCH 3/4] test(harness): close the stale-assemble hole, and decide the dropped assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-669b. PR #65 single-homed HANDSHAKE_TIMEOUT_MS in web/shell.js, but the home is single in the TREE: the spec imports web/shell.js and the page loads harness/serve/app/shell.js. createShell now exposes timeoutMs() beside timesArmed(), and outlastTheWatchdog requires the running page's value to equal the one it imported. Injected with the ticket's recipe (8000 -> 100 in source, no rebuild): 5 passed / 1 failed where it was previously 6/6 green. Also in scope, the same file's stale line: HANDSHAKE_TIMEOUT_MS restates nothing and one test outlasts it, not two. improvise-qf57. The formula-row click's `#grid input` toHaveCount(0) stays dropped, and now deliberately. The reviewer's argument was that the exact `#mode` assertion subsumes it; measuring gives a better one. app.js creates a grid in exactly one place, the grid cell's own mousedown, so no panel handler can reach it — and an invented formula-row activation opens the MINIBUFFER, not a grid editor. Injecting ChangeMode(formula_edit()) into click_formula_row: the toHaveCount(0) probe PASSES and the run fails on the #mode assertion below it. Restoring it would add an assertion that cannot fail, which context/test-evidence/clients.md already records as the reason a sibling assert! was removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/clients.md | 16 ++++++++++++++++ context/test-evidence/clients.md | 29 ++++++++++++++++++++++++++--- harness/tests/contract.spec.js | 2 ++ harness/tests/shell.spec.js | 6 ++++++ web/shell.js | 3 +++ 5 files changed, 53 insertions(+), 3 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 24a347d9..6896ca3d 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -810,6 +810,22 @@ the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_p asserts the binding's absence and names `click_formula_row` as what must change if it ever appears. +**The browser assertion that PR #60 dropped stays dropped, and improvise-qf57 +is the decision rather than the oversight.** `#grid input` `toHaveCount(0)` was +argued to be redundant with the `#mode` assertion on the grounds that +`toHaveText("FORMULAS")` is exact and `AppMode::FormulaEdit` labels as +`"FORMULA"` — a coincidence of two strings differing by one character, which +would be a thin reason to leave it out. Measuring it gives a better one: it is +not redundant, it is **unreachable**. `web/app.js` creates a grid `` in +exactly one place, the grid cell's own `mousedown`, gated on `cell.editable`; +no repaint creates one and no panel handler can reach it. And an invented +formula-row activation opens the **minibuffer**, not a grid editor. Injecting +`Command::ChangeMode(AppMode::formula_edit())` into `click_formula_row` and +running the spec: `#grid input` `toHaveCount(0)` **passes**, and the run fails +on the `#mode` assertion below it. Restoring it would add an assertion that +cannot fail — the same defect this file already records one line down, where an +explanatory `assert!` was removed for never being able to be the failing one. + **Rule 2 — a click is all of its intents or none of them** (improvise-hkr8). Each `click_*` returns `Option>` and `click` does the single `unwrap_or_default`. The case that matters is an activation the client cannot diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index cce8fcc0..3c630857 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -537,9 +537,13 @@ file server — **deterministic where a closed port is not**: a refused port's `close` timing is a property of the host's network stack, and on the GitHub runner it did not arrive inside the window. The page cannot tell the two apart and neither can `web/app.js`: in both cases the socket never opened. -`HANDSHAKE_TIMEOUT_MS` restates `web/app.js`'s own constant, because two tests -have to outlast it and a number that silently disagreed with the page would make -them pass for the wrong reason. `logStatus` records **every distinct `#status`**, +`HANDSHAKE_TIMEOUT_MS` is **imported from `web/shell.js`** and restates nothing +(PR #65), and **one** test outlasts it — this sentence said the opposite of both +for a while, which is gotcha 48's third clause: a fact stated twice is changed by +a grep, not an edit. Since improvise-669b the spec also compares the *running +page's* window against the one it imported, because the single home is in the +TREE and the page loads `harness/serve/app/shell.js` — see +`outlastTheWatchdog` below. `logStatus` records **every distinct `#status`**, not just the last one, because several of these tests are about a message being *replaced*, which the final value cannot see. @@ -624,6 +628,25 @@ watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` rea moved. Demonstrated both ways: with the install removed the suite passed 6/6, and with the guard in place the same omission fails by name. +**And it guards its second premise since improvise-669b: that the page runs the +constant the spec imported.** PR #65 single-homed the handshake window in +`web/shell.js`, which closed the *understated spec constant* — but the home is +single in the **tree**, not in what the page runs: the spec imports +`../../web/shell.js` and the page loads `harness/serve/app/shell.js`, the copy +`web/assemble.sh` put there. A stale `serve/` restores the exact vacuity, and +review demonstrated it — shrink the constant in source only, revert both socket +arms, 6/6 green. `createShell` now exposes `timeoutMs()` beside `timesArmed()` +and `outlastTheWatchdog` reads the running page's value and requires it to equal +the import. Injected with the ticket's own recipe (`8000` → `100` in +`web/shell.js`, no rebuild): the run goes to 5 passed / 1 failed, on +"the page's handshake window is not the one this spec imported", where the same +staleness was previously invisible. + +Note what this does and does not reach: it guards the tests that *call the +helper*. `"a peer that accepts and then says nothing trips the watchdog"` also +names `HANDSHAKE_TIMEOUT_MS`, but as a Playwright `timeout:` margin rather than +as a window to outlast — a stale value there costs patience, not evidence. + **The window is single-homed, and that is the second half of the same finding.** `HANDSHAKE_TIMEOUT_MS` lived twice — once in `web/app.js`, once in the spec — with a comment saying a silent disagreement "would pass for the diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 06c87b03..fc4c5f70 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -214,6 +214,8 @@ test.describe("a pointer event reaches the client and its answer is painted", () // A formula row: navigation only, because FormulaPanel binds no Enter and // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. + // `#grid input` toHaveCount(0) belongs here and is deliberately absent: + // it cannot fail, measured by injection (improvise-qf57, clients.md). await page.keyboard.press("Shift+F"); await expect(page.locator("#formulas li").first()).toBeVisible(); await page.keyboard.press("Escape"); diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 0b51272f..435ba6c5 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -41,6 +41,12 @@ async function outlastTheWatchdog(page) { await page.evaluate(() => window.__improviseShell?.timesArmed() ?? 0), "the watchdog was never armed on this path: denying its message proves nothing", ).toBeGreaterThan(0); + // The import is the SOURCE tree's constant; the page runs the ASSEMBLED + // copy. A stale `serve/` makes them differ — improvise-669b, clients.md. + expect( + await page.evaluate(() => window.__improviseShell?.timeoutMs() ?? null), + "the page's handshake window is not the one this spec imported: re-run harness/build.sh", + ).toBe(HANDSHAKE_TIMEOUT_MS); const jump = HANDSHAKE_TIMEOUT_MS + 2_000; const before = await page.evaluate(() => Date.now()); await page.clock.runFor(jump); diff --git a/web/shell.js b/web/shell.js index 7100f234..90861beb 100644 --- a/web/shell.js +++ b/web/shell.js @@ -153,6 +153,9 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT this.setShell(text); }, armed: () => watchdog !== null, + /** The window this shell was built with. Exposed so a spec can compare the + * RUNNING page's value against the one it imported — improvise-669b. */ + timeoutMs: () => timeoutMs, /** How many times the watchdog has been armed. A test denying its message * needs this to be nonzero, or it is denying an unreachable event. */ timesArmed: () => arms, From 630b84066c97514f193d72ba5a75e6169f79c50f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 22:06:34 -0700 Subject: [PATCH 4/4] fix(gp): NumericCategory is a filed gap, not a permanent refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on this branch, all three in the gp commit. The prose was the worst of them: §11 called Artefact::NumericCategory a permanent refusal that "no vocabulary closes". It is not. fit_measure hands preprocess an Overrides::default(), so it never says which header columns are categories — although the next line tells Features::of — and preprocess then infers Numeric -> Date -> Categorical from the values. A `Year` category with items 2024/2025 becomes a numeric column and is refused, with `Year IS 2024` spellable all along. Forced::Categorical is the existing knob. Filed as improvise-zv8t. Prose that calls something permanent is what stops the ticket being filed, which makes this the branch's own defect class one level up. MissingFlag's words said "that measure", but its arm is not conditioned on is_category and dataset.rs writes "" for an absent coordinate, so a .missing flag over a category is reachable — the same "declined in words naming something they are not" the commit was written to fix. Now "that column". ValueOfAMeasure's tail likewise covered only the present case. The test module claimed "each artefact family" while its fixture reaches three of five; named for what it reaches. Re-injected the collapsed-reason fault after the reword: still fails, so the negative clauses still bite. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/gp.md | 30 ++++++++++++++++++++++++++---- context/test-evidence/gp.md | 14 ++++++++++---- crates/improvise-gp/src/lib.rs | 13 +++++++------ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/context/code/gp.md b/context/code/gp.md index 703779fd..39190691 100644 --- a/context/code/gp.md +++ b/context/code/gp.md @@ -369,10 +369,32 @@ than storing a formula whose every cell is a reference error (improvise-8xhy). `improvise-4zl7`'s route-then-aggregate rule (`context/code/core.md` §13) is untouched — nothing new is emitted, so there is nothing new for it to hold over. -The other three families are declined permanently rather than pending: a one-hot -over a measure and the `(missing)` pseudo-item name no coordinate *by -construction*, and a numeric category column is a name collision the document -itself creates. No vocabulary closes those; they are refusals, not gaps. +Two of the other three are declined permanently: a one-hot over a measure and +the `(missing)` pseudo-item name no coordinate *by construction*, so no +vocabulary closes them. They are refusals rather than gaps. + +**`NumericCategory` is not, and this paragraph said it was** — which is the +more useful half of improvise-wd6n, because prose that calls something a +permanent refusal is what stops anyone filing the ticket. `fit_measure` hands +`preprocess` an `Overrides::default()`, so it never says which header columns +are document categories, although the very next line tells `Features::of`. +`preprocess` then infers `Numeric` → `Date` → `Categorical` from the cell +values, so a `Year` category with items `2024`/`2025` becomes a *numeric column* +and a category of ISO dates becomes a *date column* — and both are refused, +`Year IS 2024` being exactly spellable all along. That needs no +formula-language change whatever: `Forced::Categorical` already exists and is +the knob. Filed as **improvise-zv8t**. The same accident is why a `DatePart` +can arrive over a category column, so the date family's decline above is +narrower than it looks: it covers a genuine date *measure*, not a category +misread as one. + +**And the refusal's own words must not name a column kind the arm does not +know.** `FeatureRole::Missing` is *not* conditioned on `is_category` — unlike +its `Numeric` and `OneHot` neighbours — and `dataset.rs` writes `""` for a +record with no coordinate for a category, so a `.missing` flag over a +**category** is reachable. Its clause therefore says "that column" and not +"that measure": naming the wrong kind there would be this section's own charge +— declined in words naming something they are not — surviving one level down. ## 12. A fit is persisted as its formula, and nothing else (improvise-7na) diff --git a/context/test-evidence/gp.md b/context/test-evidence/gp.md index 4be5232e..4dbd184b 100644 --- a/context/test-evidence/gp.md +++ b/context/test-evidence/gp.md @@ -603,7 +603,13 @@ precisely the state the ticket describes. Fault injected: the five arms of family, and with the injection removed all three arms pass. The fixture reaches three families from one `preprocess` call (a text measure -column, a measure with a hole in it, a column of ISO dates). The remaining two — -`AbsentCoordinate` and `NumericCategory` — are not reachable from it and are not -asserted here; they are refusals rather than pending gaps, argued in -`context/code/gp.md` §11. +column, a measure with a hole in it, a column of ISO dates), which is why the +module is named for what it reaches rather than for "each family". The +remaining two are not reachable from it and are not asserted here. +`AbsentCoordinate` is a permanent refusal; `NumericCategory` is **not**, and +the first draft of this entry and of `gp.md` §11 both called it one — review +found that `fit_measure` simply never tells `preprocess` which columns are +categories, so it is a filed gap (improvise-zv8t) rather than a refusal. Worth +recording as the failure mode: the branch that *invented* a classification also +wrote the prose declaring one of its arms unfixable, and nothing but a reader +could have caught that. diff --git a/crates/improvise-gp/src/lib.rs b/crates/improvise-gp/src/lib.rs index 9440ce92..dff2543d 100644 --- a/crates/improvise-gp/src/lib.rs +++ b/crates/improvise-gp/src/lib.rs @@ -33,7 +33,8 @@ pub enum Spelling { /// different vocabulary to close. `context/code/gp.md` §11 carries the arguments. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Artefact { - /// `col.missing`: "this record has no value for X" (improvise-wd6n). + /// `col.missing`: "this record has no value for that column" — of a measure + /// or of a category, so the words must not claim either (improvise-wd6n). MissingFlag, /// `when.year` and its siblings: date arithmetic (improvise-wd6n). DatePart, @@ -51,7 +52,7 @@ impl Artefact { fn reason(self) -> &'static str { match self { Self::MissingFlag => { - "says this record has no value for that measure, which the formula language \ + "says this record has no value for that column, which the formula language \ has no word for: a reference to an absent cell is an error, not a zero" } Self::DatePart => { @@ -60,7 +61,7 @@ impl Artefact { } Self::ValueOfAMeasure => { "is a value of a measure rather than a coordinate, so `IS` cannot ask about it \ - — the record's value for that column is text" + — a measure's values are text, and its absent case is no value at all" } Self::AbsentCoordinate => { "is the absence of a coordinate rather than an item, which `IS` reads as false \ @@ -843,9 +844,9 @@ mod tests { assert!(fitted_formula(&tree, &features, TARGET, MEASURE, &model).is_ok()); } } - /// improvise-wd6n. The artefact families are refused *separately*, because - /// they are declined for different reasons. Evidence: `context/test-evidence/gp.md`. - mod each_artefact_family_is_refused_in_its_own_words { + /// improvise-wd6n. The families a `preprocess` run reaches — three of five — + /// are refused separately. Evidence: `context/test-evidence/gp.md`. + mod the_artefact_families_this_fixture_reaches_are_refused_separately { use super::{MEASURE, TARGET, leaf, split}; use crate::{Features, fitted_formula}; use gp_tree::data::{Overrides, preprocess};