fix(core): summaries are tested for exact merges, impossible parts are refused, and the bypass list is complete (improvise-qddo)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

context/code/source.md

@@ -60,8 +60,11 @@ either returns the numbers (and the adapter sums them) or accepts a last-bit dif
 
 `Model::source()` returns the `&dyn CellSource` that the evaluator, the stems of `recompute_formulas`
 and the records of `GridLayout` read through. Today it is always the owned `DataStore`; the point of
-routing the reads is that PR 3 (improvise-rnak) changes only this accessor. Reads that bypass it
+routing the reads is that PR 3 (improvise-rnak) changes only this accessor. It is not the only path: `Model::data` is `pub`, and `Model::get_cell` reads the owned
-(`model.data.*` in other crates, `Eager::occupancy`, `Shapes::of`) are the ones later steps move.
+store directly. Both are the facade of a document that owns its cells; their callers are persistence,
+the import wizard and `improvise-command` (`ui/effect.rs`, the `old` value of a rename at line 890, is
+the one production read of `get_cell` outside those), and a model over a source has no cells there.
+`Eager::occupancy` and `Shapes::of` were the other readers; later steps moved them.
 
 ## `TableSource`
 
@@ -73,6 +76,8 @@ lists, and two), at both widths and for all five functions: over 5,000 triples.
 
 ## Building a `Summary` from an engine's answer
 
-`Summary::from_parts(Numbers::from_parts(count, exact_sum, least, greatest), Agreement)`. `Agreement`
+`Summary::from_parts(Numbers::from_parts(count, exact_sum, least, greatest)?, Agreement)?`. `Agreement`
-is what the non-number values hold: `None`, `One(v)` or `Mixed`. `from_parts` drops extremes that
+is what the non-number values hold: `None`, `One(v)` or `Mixed`. Both constructors return a `Result`
-contradict the count or are NaN, so a source cannot hand the evaluator a state `add` could not reach.
+and **refuse** a state no sequence of values could produce (extremes with no numbers, a NaN extreme,
+least above greatest, one extreme without the other, a number among the "others"), because repairing
+it would hide an engine's bug.

context/test-evidence/core.md

@@ -2314,6 +2314,11 @@ lost). The first two also fail `aggregating_differing_text_values_is_an_explicit
 
 ### model::source::tests::a_merge_of_summaries_is_the_summary_of_the_union
 
+Review of #356 (Opus): the pool gained `1e16`, `1.0`, `-1e16`, `0.1` and `0.2`. A merge rounding each part's
+sum before pooling, keeping the zero sign, changed 514 production answers while the workspace passed;
+that rival now fails this test, `numbers_pool_what_the_formula_aggregate_pools` (a NaN-free split now,
+compared as bits), `summaries_built_from_parts_merge_as_the_union_does` and the fixture test below, by name.
+
 A read of several parts is a merge. The test merges every ordered pair of sequences of up to two
 values and compares with the reference of the concatenation. Rivals: the least not merged, the
 greatest merged with the other side's least, the zeros unordered. Each fails it.
@@ -2335,7 +2340,7 @@ span, and the store ignores `exact` for a spanned one. The first also fails
 ### model::source::tests::the_store_and_a_scanned_table_list_the_same_cells
 
 `keys` and `cells` under every base of the fixture, and `value_at` for every row and an absent key.
-Rival: `DataStore::keys` ignores its partial. It fails here and in five evaluator tests that read
+Rival: `DataStore::keys` ignores its partial. It fails here and in seven evaluator tests that read
 the member records of an aggregate over an expression
 (`route_then_aggregate_tests::a_routing_formula_at_a_record_is_the_rule_itself` among them).
 
@@ -2348,13 +2353,28 @@ this is the contract a second source implements. Rival: `exact` ignored when the
 ### a_summary_built_from_an_engines_parts_reduces_as_one_built_from_the_values
 
 Review of #356 (Codex): an engine outside core could not build a `Summary` without replaying rows. The
-parts (count, exact sum, extremes, agreement of the non-numbers) are computed in the test without
+parts are computed in the test without `Summary::of`, for every sequence of up to three values of the
-`Summary::of`, for the 585 sequences of the reference test. Rivals: the greatest taken from the least;
+pool. No rival of the first version failed this test alone (its filter was untested); see below.
-a NaN allowed as an extreme. Each fails it.
 
-### parts_that_contradict_themselves_are_made_consistent
+### summaries_built_from_parts_merge_as_the_union_does
 
-Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from_parts`.
+A summary built from parts, merged with one built from values and with one built from parts, equals the
+reference of the union, for every pair of sequences of up to two values. This is where a bad extreme
+would matter, since merge reads the extremes.
+
+### parts_no_sequence_of_values_could_produce_are_refused
+
+Review of #356 (Opus, Codex): `from_parts` repaired some impossible states and accepted others
+(`One(Number)`, least above greatest). It refuses all of them with a `BadParts` naming the reason.
+The earlier entry's claim that a NaN-extreme rival failed `parts_that_contradict_themselves_...` was
+false: that test passed with the filter removed. Rivals now, each failing this test alone: NaN extreme
+allowed, least above greatest allowed, extremes with no numbers allowed, a number accepted among the
+others.
+
+### the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts
+
+An exactness test needs inputs whose float sum is inexact and no NaN. `1e16 + 1.0 - 1e16` rounded part
+by part is `0.0`; pooled exactly it is `1.0`. This pins the fixture rather than the code.
 
 ### keys_of_a_store_are_its_cells_keys_without_the_values
 

crates/improvise-core/src/model/source.rs

@@ -52,20 +52,28 @@ impl Numbers {
     }
 
     /// Numbers from what an engine computed: how many, their exact sum and the least and
-    /// greatest that are not NaN. Extremes that contradict the count or are NaN are dropped.
+    /// greatest that are not NaN. A state `add` cannot reach is refused, not repaired.
     pub fn from_parts(
         count: u64,
         sum: ExactSum,
         least: Option<f64>,
         greatest: Option<f64>,
-    ) -> Self {
+    ) -> Result<Self, BadParts> {
-        let held = |x: Option<f64>| x.filter(|x| count > 0 && !x.is_nan());
+        let bad = |why| Err(BadParts(why));
-        Numbers {
+        match (least, greatest) {
+            (None, None) => {}
+            (Some(_), Some(_)) if count == 0 => return bad("extremes with no numbers"),
+            (Some(l), Some(g)) if l.is_nan() || g.is_nan() => return bad("a NaN extreme"),
+            (Some(l), Some(g)) if l.total_cmp(&g).is_gt() => return bad("least above greatest"),
+            (Some(_), Some(_)) => {}
+            _ => return bad("one extreme without the other"),
+        }
+        Ok(Numbers {
             count,
             sum,
-            least: held(least),
+            least,
-            greatest: held(greatest),
+            greatest,
-        }
+        })
     }
 
     pub fn add(&mut self, x: f64) {
@@ -121,6 +129,10 @@ fn merged(a: Option<f64>, b: Option<f64>, pick: fn(f64, f64) -> f64) -> Option<f
     }
 }
 
+/// Parts of a summary that no sequence of values could have produced.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct BadParts(pub &'static str);
+
 /// What the values that are not numbers agree on: nothing held, one value, or disagreement.
 #[derive(Debug, Clone, Default, PartialEq)]
 pub enum Agreement {
@@ -146,8 +158,11 @@ impl Summary {
     }
 
     /// A summary from what an engine computed: the numbers, and what the rest agree on.
-    pub fn from_parts(numbers: Numbers, others: Agreement) -> Self {
+    pub fn from_parts(numbers: Numbers, others: Agreement) -> Result<Self, BadParts> {
-        Summary { numbers, others }
+        match &others {
+            Agreement::One(v) if v.as_f64().is_some() => Err(BadParts("a number among the others")),
+            _ => Ok(Summary { numbers, others }),
+        }
     }
 
     pub fn add(&mut self, value: &CellValue) {
@@ -324,6 +339,11 @@ mod tests {
             CellValue::Number(0.0),
             CellValue::Number(f64::NAN),
             CellValue::Number(f64::INFINITY),
+            CellValue::Number(1e16),
+            CellValue::Number(1.0),
+            CellValue::Number(-1e16),
+            CellValue::Number(0.1),
+            CellValue::Number(0.2),
             text("a"),
             text("b"),
             CellValue::Error("e".into()),
@@ -416,23 +436,41 @@ mod tests {
 
     #[test]
     fn numbers_pool_what_the_formula_aggregate_pools() {
-        let xs = [1.0, -0.0, f64::NAN, 3.0, 0.0];
+        let xs = [1e16, 1.0, -1e16, 0.1, 0.2, -0.0, 0.0, 3.0];
         for split in 0..=xs.len() {
             let mut pooled = Numbers::of(xs[..split].iter().copied());
             pooled.merge(&Numbers::of(xs[split..].iter().copied()));
             let whole = Numbers::of(xs);
             for func in &FUNCS {
                 let (a, b) = (pooled.reduce(func), whole.reduce(func));
-                let equal = a.map(f64::to_bits) == b.map(f64::to_bits)
+                assert_eq!(
-                    || a.zip(b).is_some_and(|(x, y)| x.is_nan() && y.is_nan());
+                    a.map(f64::to_bits),
-                assert!(equal, "{func:?} split at {split}: {a:?} != {b:?}");
+                    b.map(f64::to_bits),
+                    "{func:?} split at {split}"
+                );
             }
-            assert_eq!(pooled.total().to_bits(), whole.total().to_bits());
+            assert_eq!(
+                pooled.total().to_bits(),
+                whole.total().to_bits(),
+                "split at {split}"
+            );
         }
         assert_eq!(Numbers::default().total().to_bits(), 0.0f64.to_bits());
         assert_eq!(Numbers::default().reduce(&AggFunc::Sum), None);
     }
 
+    #[test]
+    fn the_exact_sum_of_a_pool_of_cancelling_values_is_not_the_sum_of_rounded_parts() {
+        let rounded = [1e16, 1.0].iter().sum::<f64>() + [-1e16].iter().sum::<f64>();
+        let mut pooled = Numbers::of([1e16, 1.0]);
+        pooled.merge(&Numbers::of([-1e16]));
+        assert_eq!(
+            rounded, 0.0,
+            "the fixture is inexact: summing rounded parts loses the 1"
+        );
+        assert_eq!(pooled.total(), 1.0);
+    }
+
     fn pair(c: &str, i: &str) -> (String, String) {
         (c.to_string(), i.to_string())
     }
@@ -597,8 +635,8 @@ mod tests {
             }
             Some(_) => Agreement::Mixed,
         };
-        let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest);
+        let parts = Numbers::from_parts(numbers.len() as u64, sum, least, greatest).unwrap();
-        Summary::from_parts(parts, agreement)
+        Summary::from_parts(parts, agreement).unwrap()
     }
 
     #[test]
@@ -617,12 +655,61 @@ mod tests {
     }
 
     #[test]
-    fn parts_that_contradict_themselves_are_made_consistent() {
+    fn summaries_built_from_parts_merge_as_the_union_does() {
-        let none = Numbers::from_parts(0, ExactSum::new(), Some(1.0), Some(f64::NAN));
+        let all = sequences(2);
-        assert_eq!(none.reduce(&AggFunc::Min), None, "no numbers, no extremes");
+        for (a, b) in all.iter().flat_map(|a| all.iter().map(move |b| (a, b))) {
-        let nan = Numbers::from_parts(2, ExactSum::new(), Some(f64::NAN), None);
+            let mut merged = parts_of(a);
-        let min = nan.reduce(&AggFunc::Min).unwrap();
+            merged.merge(&Summary::of(b));
-        assert!(min.is_nan(), "a NaN is never an extreme");
+            let mut both = parts_of(a);
+            both.merge(&parts_of(b));
+            let union: Vec<&CellValue> = a.iter().chain(b).collect();
+            for func in &FUNCS {
+                let want = reference(&union, func);
+                assert!(
+                    same(&want, &merged.reduce(func)),
+                    "{func:?} of {a:?} + {b:?}"
+                );
+                assert!(
+                    same(&want, &both.reduce(func)),
+                    "{func:?} of {a:?} + {b:?}, both from parts"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn parts_no_sequence_of_values_could_produce_are_refused() {
+        let sum = ExactSum::new;
+        let refused = |r: Result<Numbers, BadParts>| r.expect_err("must be refused").0;
+        assert_eq!(
+            refused(Numbers::from_parts(0, sum(), Some(1.0), Some(1.0))),
+            "extremes with no numbers"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(f64::NAN), Some(1.0))),
+            "a NaN extreme"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(2.0), Some(1.0))),
+            "least above greatest"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(0.0), Some(-0.0))),
+            "least above greatest"
+        );
+        assert_eq!(
+            refused(Numbers::from_parts(2, sum(), Some(1.0), None)),
+            "one extreme without the other"
+        );
+        let ok = Numbers::from_parts(2, sum(), None, None).unwrap();
+        assert!(
+            ok.reduce(&AggFunc::Min).unwrap().is_nan(),
+            "all NaN: no extremes, a count"
+        );
+        let number = Agreement::One(CellValue::Number(1.0));
+        let bad = Summary::from_parts(ok.clone(), number).expect_err("a number is not an other");
+        assert_eq!(bad.0, "a number among the others");
+        assert!(Summary::from_parts(ok, Agreement::One(CellValue::Text("t".into()))).is_ok());
     }
 
     #[test]