Model's Debug renders its HashMaps in key order, so the effect fingerprints cannot flake (improvise-pc3b)

context/code/formula-and-io.md

@@ -495,10 +495,22 @@ compiler named every other consumer. Two remain, and both are the wire:
 untouched.
 
 One test consumer had to move: the effect-classification tests fingerprinted the
-model and the views through `serde_json`. They now use `Debug`. That is no weaker:
+model and the views through `serde_json`. They now use `Debug`. `format_md` was
-it renders every field, including the derived `formula_cache` the JSON skipped.
+rejected as the replacement, because it skips the virtual views and sorts the
-`format_md` was rejected as the replacement, because it skips the virtual views
+data, so it would have been a weaker fingerprint.
-and sorts the data, so it would have been a weaker fingerprint.
+
+The first version of this paragraph said `Debug` was "no weaker" because it
+renders every field, including the derived `formula_cache` the JSON skipped.
+**That was wrong (improvise-pc3b).** A derived `Debug` prints a `HashMap` in hash
+order, and `recompute_formulas` clears and refills the cache. So a correct
+`ViewOnly` effect such as `RebuildLayout` changed the fingerprint in 90 of 200
+runs. The suite passed only because the fixture had no formulas, so the cache
+was empty. `Model` now has a hand-written `Debug` that renders `formula_cache`
+and `measure_agg` in key order. The fingerprint keeps every field, including the
+derived one, and a refill in a different hash order is no longer read as a
+change. `recomputing_formulas_leaves_the_model_fingerprint_alone` pins this.
+`view_only_effects_leave_shared_state_alone` now runs over a formula-bearing
+fixture, for the same reason.
 
 `parse_number_cell` returns `CellValue::Error` naming the offending text rather
 than a fabricated `0.0`. No token the current grammar admits actually fails

context/test-evidence/command.md

@@ -5599,3 +5599,29 @@ that is deliberate. Its helpers seed a client *after* mutating the host (mode,
 buffers, cursor), so a `Snapshot` would drop exactly the state those tests
 stage. This test is what makes the copy faithful at startup.
 
+
+### ui::effect::tests::recomputing_formulas_leaves_the_model_fingerprint_alone
+
+improvise-pc3b, found by the review of PR #224 at `e62a289`. That PR moved the
+effect-classification fingerprints from `serde_json` to `Debug`. The derived
+`Debug` for `Model` prints `formula_cache`, a `HashMap` that `recompute_formulas`
+clears and refills, so it prints in hash order. The reviewer measured a correct
+`ViewOnly` effect (`RebuildLayout`) changing the fingerprint in 90 of 200 runs
+once formulas were present. `test_app` has none, so the suite was green over a
+latent flake.
+
+The test builds `formula_app` (two measures across a 2×2 grid, plus
+`Profit = Revenue - Cost` and `Margin = Profit / Revenue`) 32 times. Each build
+is a fresh `App`, so each gets fresh hash states. Each time, it asserts that
+`RebuildLayout` leaves `model_fingerprint` unchanged. One flip in about two makes
+32 independent tries fail near-certainly.
+
+**Red before the fix**: the two renderings differed only in `formula_cache`'s
+entry order. The fix is a hand-written `Debug` for `Model` that renders
+`formula_cache` and `measure_agg` in key order.
+
+**Rival, run:** keep the hand-written `Debug` but print `&self.formula_cache`
+unsorted. This test fails. `view_only_effects_leave_shared_state_alone` now runs
+over `formula_app` too, so every `ViewOnly` case is checked against a populated
+cache. The two earlier injections (a `SetStatus` that writes the model, and one
+that writes a view's `number_format`) still fail it.

crates/improvise-command/src/ui/effect.rs

@@ -1408,6 +1408,57 @@ mod tests {
         format!("{:?}", app.model_state.workbook.views)
     }
 
+    /// `test_app` plus measures, data and two chained formulas, so the derived
+    /// formula cache holds several entries for a recompute to refill.
+    fn formula_app() -> App {
+        let mut app = test_app();
+        let wb = &mut app.model_state.workbook;
+        for m in ["Revenue", "Cost"] {
+            wb.add_item("_Measure", m).unwrap();
+        }
+        let mut n = 1.0;
+        for t in ["Food", "Clothing"] {
+            for mo in ["Jan", "Feb"] {
+                for (m, scale) in [("Revenue", 10.0), ("Cost", 3.0)] {
+                    let key = CellKey::new(vec![
+                        ("Type".into(), t.into()),
+                        ("Month".into(), mo.into()),
+                        ("_Measure".into(), m.into()),
+                    ]);
+                    wb.model.set_cell(key, CellValue::Number(n * scale));
+                }
+                n += 1.0;
+            }
+        }
+        for raw in ["Profit = Revenue - Cost", "Margin = Profit / Revenue"] {
+            AddFormula {
+                raw: raw.into(),
+                target_category: "_Measure".into(),
+            }
+            .apply(&mut app);
+        }
+        app.rebuild_layout();
+        app
+    }
+
+    /// improvise-pc3b: the fingerprint must not read a derived cache's hash
+    /// order. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn recomputing_formulas_leaves_the_model_fingerprint_alone() {
+        // Each App gets fresh hash states; one run observed ~45% flips, so 32
+        // independent tries make a hash-ordered fingerprint fail near-certainly.
+        for _ in 0..32 {
+            let mut app = formula_app();
+            let before = model_fingerprint(&app);
+            RebuildLayout.apply(&mut app);
+            assert_eq!(
+                model_fingerprint(&app),
+                before,
+                "a ViewOnly recompute changed the model fingerprint"
+            );
+        }
+    }
+
     /// improvise-mae: rather than restate each declaration, apply the effect and
     /// check the shared state it claims not to touch is genuinely unchanged.
     #[test]
@@ -1445,7 +1496,8 @@ mod tests {
                 EffectKind::ViewOnly,
                 "{effect:?} is in the ViewOnly case list"
             );
-            let mut app = test_app();
+            // With formulas, so `RebuildLayout` refills a non-empty cache (pc3b).
+            let mut app = formula_app();
             let model_before = model_fingerprint(&app);
             let views_before = view_config_fingerprint(&app);
             effect.apply(&mut app);

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

@@ -237,7 +237,7 @@ impl Aggregation {
 
 /// Pure-data document model: categories, cells, formulas. It knows nothing
 /// about views — see [`crate::workbook::Workbook`] and core.md §10.
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 pub struct Model {
     pub name: String,
     pub categories: IndexMap<String, Category>,
@@ -251,6 +251,26 @@ pub struct Model {
     formula_cache: HashMap<CellKey, CellValue>,
 }
 
+/// Every field, with the two `HashMap`s in key order: a recompute refills the
+/// cache in hash order, which is not a change (improvise-pc3b, gotcha 18).
+impl std::fmt::Debug for Model {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let mut aggs: Vec<_> = self.measure_agg.iter().collect();
+        aggs.sort_by(|a, b| a.0.cmp(b.0));
+        let mut cache: Vec<_> = self.formula_cache.iter().collect();
+        cache.sort_by(|a, b| a.0.0.cmp(&b.0.0));
+        f.debug_struct("Model")
+            .field("name", &self.name)
+            .field("categories", &self.categories)
+            .field("data", &self.data)
+            .field("formulas", &self.formulas)
+            .field("next_category_id", &self.next_category_id)
+            .field("measure_agg", &aggs)
+            .field("formula_cache", &cache)
+            .finish()
+    }
+}
+
 impl Model {
     pub fn new(name: impl Into<String>) -> Self {
         let name = name.into();