A drill commit naming a removed category is refused and writes nothing (improvise-qz9p)

While a drill was open, another client's RemoveCategory could remove a
category the drill's rows name. On commit, AddItem refused, but the rewrite's
clears and writes still ran, leaving a cell keyed under a category the model no
longer has. A measure edit alone on such a row was worse: no AddItem, so no
refusal, and the orphan was written silently.

RecordRewrite::of now asks whether every category a row names, its
coordinates and its renames, still exists. If one is gone, the row is
unwritable: it refuses and carries no writes. merged drops every write when any
row is unwritable, because a drill commit is atomic (design-principles §4). The
effect refuses rather than stops (gotcha 21): ViewBack after it assumes nothing
about the writes, and the category cannot come back through the drill, so
stopping would trap the user.

Reasoning: context/code/command.md, RecordRewrite.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

context/code/command.md

@@ -986,6 +986,39 @@ single edit the output is the effects the plain path used to emit, reordered fro
 One behaviour moved on the drill side: it used to discard `add_item`'s error, and
 now goes through `effect::AddItem`, which refuses when the category has gone.
 
+**A row naming a category the model no longer has is unwritable, and one
+unwritable row makes the whole drill commit write nothing** (improvise-qz9p).
+Another client's `RemoveCategory` can land while a drill is open. That removes the
+category and every cell under it, and the drill's staged edits then address
+coordinates nothing owns. The fix has two parts.
+
+- **The check is structural, not a reaction to `AddItem` refusing.** The ticket
+  framed it as "should a failed `AddItem` drop the row's writes", but that misses
+  the plainer case. A **measure edit alone** on such a row stages no `AddItem`, so
+  nothing refuses, and it still writes an orphan cell keyed under the removed
+  category. `RecordRewrite::of` therefore asks the model whether every category
+  the row names still exists, both its coordinates and its renames. If any is
+  gone, the row is `unwritable`: it refuses with `Unknown category '…'` and
+  carries no writes. This is ADR 001's "force never creates structure", applied
+  inside the drill.
+- **Why the whole commit and not just that row.** The drill's contract is atomic
+  (`design-principles.md` §4: "`ApplyAndClearDrill` writes them all atomically …
+  No partial writes reach the model"). A per-row skip would commit the other
+  rows' edits and drop this one's, a partial write of exactly the kind that
+  contract rules out. `merged` therefore keeps only the refusals when any row is
+  unwritable.
+- **`refuse`, not `refuse_and_stop`.** This follows #181's rule (gotcha 21):
+  abort only where a later effect in the batch assumes the failed one happened.
+  The effect after `ApplyAndClearDrill` in `view-back`'s batch is `ViewBack`,
+  which assumes nothing about the writes. Stopping would also keep the user in a
+  drill they cannot fix, because the category cannot come back through it, so
+  every exit would fail again. The staged edits are discarded, as on cancel, and
+  the refusal says why.
+
+A blank coordinate edit is a different kind of refusal. It is refused **alone**,
+and the row's other edits still land, as before: that edit is invalid input, not
+a missing structure.
+
 ### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae)
 
 A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains

context/test-evidence/command.md

@@ -5599,3 +5599,33 @@ 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.rs` — a drill commit into a category removed mid-drill (improvise-qz9p)
+
+### a_drill_rename_into_a_removed_category_is_refused_and_writes_nothing
+
+improvise-qz9p, the ticket's own probe. `Type` is removed while the drill is open,
+and row 1 stages `Type → Drink` and `Cost = 999`. Before the fix the commit left
+`{Drink/Jan/Cost = 999}`, a cell keyed under a category the model no longer has,
+which no grid shows. `AddItem`'s refusal was visible, but the clears and writes
+ran anyway. Red before the fix on the store assertion. It asserts both the
+refusal (`last_batch_refused`) and the whole store, so a fix that only refuses
+and a fix that only drops writes both fail it. Rival B (refuse, but still
+write) fails here.
+
+### a_drill_measure_edit_on_a_record_whose_category_was_removed_is_refused
+
+improvise-qz9p, the case the ticket did not predict. A **measure edit alone**
+stages no `AddItem`, so nothing refused at all, and the commit silently wrote
+`Clothing/Jan/Cost = 999` under the removed `Type`. Red before the fix on the
+refusal assertion. It is the test that separates the structural check from the
+ticket's "drop the writes when `AddItem` refuses": rival C, which checks only
+renamed categories, fails here and on the atomicity test.
+
+### a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows
+
+improvise-qz9p. The drill is atomic (`design-principles.md` §4), so one
+unwritable row stops every row. Row 0 names `Type`, and row 1 is a record with
+no `Type` coordinate at all, so it stays writable after `Type` is removed. Both
+rows stage a `Cost`. The expected store is row 1's untouched `-/Jan/Cost = 17`.
+Rival A skips only the unwritable row, and it fails this test and nothing else
+in the workspace, writing row 1's `999`. Rival B fails here too.

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

@@ -690,7 +690,9 @@ impl Effect for ApplyAndClearDrill {
 /// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`.
 #[derive(Debug, Default, PartialEq)]
 pub(crate) struct RecordRewrite {
-    refused: bool,
+    refusals: Vec<String>,
+    /// A category the row names is gone: nothing of this rewrite may be written.
+    unwritable: bool,
     items: Vec<(String, String)>,
     clears: Vec<CellKey>,
     sets: Vec<(CellKey, CellValue)>,
@@ -704,12 +706,14 @@ impl RecordRewrite {
         row: &RecordRow,
         edits: impl IntoIterator<Item = (&'a RecordCol, &'a str)>,
     ) -> Self {
-        let mut refused = false;
+        let mut refusals = Vec::new();
         let mut renames: BTreeMap<&str, &str> = BTreeMap::new();
         let mut measures: BTreeMap<&str, Option<CellValue>> = BTreeMap::new();
         for (col, value) in edits {
             match col {
-                RecordCol::Category(_) if value.is_empty() => refused = true,
+                RecordCol::Category(_) if value.is_empty() => {
+                    refusals.push(RECORD_COORDS_CANNOT_BE_EMPTY.to_string())
+                }
                 RecordCol::Category(category) => {
                     renames.insert(category.as_str(), value);
                 }
@@ -721,6 +725,40 @@ impl RecordRewrite {
                 RecordCol::NewMeasure | RecordCol::NewCategory => {}
             }
         }
+        let gone: std::collections::BTreeSet<&str> = row
+            .coords
+            .0
+            .iter()
+            .map(|(category, _)| category.as_str())
+            .chain(renames.keys().copied())
+            .filter(|category| model.category(category).is_none())
+            .collect();
+        match gone.is_empty() {
+            true => Self::compose(model, row, refusals, renames, measures),
+            false => Self::unwritable(gone),
+        }
+    }
+
+    /// A row naming a category the model no longer has: refused, and nothing
+    /// written, because a write would create a cell no category owns (improvise-qz9p).
+    fn unwritable(gone: std::collections::BTreeSet<&str>) -> Self {
+        Self {
+            refusals: gone
+                .into_iter()
+                .map(|category| format!("Unknown category '{category}'"))
+                .collect(),
+            unwritable: true,
+            ..Self::default()
+        }
+    }
+
+    fn compose(
+        model: &crate::model::Model,
+        row: &RecordRow,
+        refusals: Vec<String>,
+        renames: BTreeMap<&str, &str>,
+        mut measures: BTreeMap<&str, Option<CellValue>>,
+    ) -> Self {
         let renamed = |key: &CellKey| {
             renames.iter().fold(key.clone(), |key, (category, item)| {
                 key.with(*category, *item)
@@ -737,7 +775,7 @@ impl RecordRewrite {
                 .collect(),
         };
         let mut rewrite = RecordRewrite {
-            refused,
+            refusals,
             items: renames
                 .iter()
                 .map(|(category, item)| (category.to_string(), item.to_string()))
@@ -762,20 +800,29 @@ impl RecordRewrite {
         rewrite
     }
 
-    /// Several rows' rewrites as one: every clear lands before any write, so a
-    /// swap is a swap, and where two rows land on one cell the later row wins.
+    /// Several rows' rewrites as one: every clear before any write, the later row winning
+    /// a shared cell, nothing written if any row is unwritable. `context/code/command.md`.
     pub(crate) fn merged(rewrites: impl IntoIterator<Item = Self>) -> Self {
-        rewrites.into_iter().fold(Self::default(), |mut all, one| {
-            all.refused |= one.refused;
+        let all = rewrites.into_iter().fold(Self::default(), |mut all, one| {
+            all.refusals.extend(one.refusals);
+            all.unwritable |= one.unwritable;
             all.items.extend(one.items);
             all.clears.extend(one.clears);
             all.sets.extend(one.sets);
             all
-        })
+        });
+        match all.unwritable {
+            true => Self {
+                refusals: all.refusals,
+                unwritable: true,
+                ..Self::default()
+            },
+            false => all,
+        }
     }
 
     pub(crate) fn effects(self) -> Vec<Box<dyn Effect>> {
-        let refusal = self.refused.then(|| refuse(RECORD_COORDS_CANNOT_BE_EMPTY));
+        let refusal = self.refusals.into_iter().map(refuse);
         let items = self
             .items
             .into_iter()
@@ -3433,6 +3480,72 @@ mod tests {
         );
     }
 
+    // ── A category removed while the drill is open (improvise-qz9p) ─────
+
+    /// Leave the drill after staging `edits` into a drill whose `Type` was removed
+    /// underneath it; the store left behind, and whether the commit was refused.
+    fn commit_after_removing_type(
+        mut app: App,
+        edits: &[(usize, RecordCol, &str)],
+    ) -> (std::collections::BTreeSet<String>, bool) {
+        app.model_state.workbook.remove_category("Type");
+        for (row, col, value) in edits {
+            stage(&mut app, *row, col.clone(), value);
+        }
+        app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
+        (drill_store(&app), app.last_batch_refused())
+    }
+
+    #[test]
+    fn a_drill_rename_into_a_removed_category_is_refused_and_writes_nothing() {
+        let (store, refused) = commit_after_removing_type(
+            two_record_drill(),
+            &[(1, category("Type"), "Drink"), (1, measure("Cost"), "999")],
+        );
+        assert!(refused, "the commit must say it could not be applied");
+        assert_eq!(store, std::collections::BTreeSet::new(), "no orphan cell");
+    }
+
+    #[test]
+    fn a_drill_measure_edit_on_a_record_whose_category_was_removed_is_refused() {
+        let (store, refused) =
+            commit_after_removing_type(two_record_drill(), &[(1, measure("Cost"), "999")]);
+        assert!(refused, "the commit must say it could not be applied");
+        assert_eq!(store, std::collections::BTreeSet::new(), "no orphan cell");
+    }
+
+    #[test]
+    fn a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows() {
+        let mut app = test_app();
+        let typed = drill_cell("Food", "Jan", "Cost");
+        let untyped = CellKey::new(vec![
+            ("Month".into(), "Jan".into()),
+            ("_Measure".into(), "Cost".into()),
+        ]);
+        let cells = [
+            (typed, CellValue::Number(2.0)),
+            (untyped, CellValue::Number(17.0)),
+        ];
+        for (key, value) in cells.clone() {
+            app.model_state.workbook.model.set_cell(key, value);
+        }
+        let records = RecordRow::group(cells);
+        assert_eq!(records.len(), 2);
+        assert_eq!(records[1].coords.get("Type"), None, "row 1 names no Type");
+        StartDrill(records).apply(&mut app);
+
+        let (store, refused) = commit_after_removing_type(
+            app,
+            &[(0, measure("Cost"), "5"), (1, measure("Cost"), "999")],
+        );
+        assert!(refused, "the commit must say it could not be applied");
+        assert_eq!(
+            store,
+            ["-/Jan/Cost = 17".to_string()].into_iter().collect(),
+            "row 1 is writable, but a drill commit is atomic"
+        );
+    }
+
     // ── Toggle effects ──────────────────────────────────────────────────
 
     #[test]