Merge PR #223 (e1453797) into the 2026-09-24 integration wave

context/code/cli.md

@@ -325,3 +325,13 @@ path, and that is how these were found. `q` on a dirty document is deliberately
 report: headless, its text is false — the CLI saves anyway (improvise-ep5o).
 The two rivals udt2 named both stay green: a successful `:w` and a successful
 `fit-measure` report through `set_status` and exit 0.
+
+**`remove-formula` of a formula that is not there now exits 1** (improvise-tej1,
+improvise-9trl). It used to exit 0: the effect was keyed by names and removing
+nothing was a silent no-op. Since tej1, `RemoveFormula` names its formula by
+`FormulaId`, and `remove-formula <category> <target>` resolves the two names
+to an id in `execute`. When nothing matches it `refuse`s with `No formula for
+<target> in <category>`, so a script that removes a formula it never added now
+fails where it used to pass. That is the §10 rule applied: nothing changed, so
+the request was not met. `remove_formula_of_a_missing_formula_is_a_refusal`
+pins it, with a present formula as the control.

context/code/clients.md

@@ -447,9 +447,10 @@ the status after.
 
 - `toggle-prune-empty` words its status off `View::prune_empty`, which is in
   the `Workbook`, in no cache, and not in `ViewState`.
-- `delete-formula-at-cursor` needs `(target, target_category)`; `FormulaCache`
-  carries the formula's raw *text*, and recovering the two from it means
-  reimplementing `parse_formula` on the client.
+- `delete-formula-at-cursor` needed `(target, target_category)` while
+  `FormulaCache` carried only the formula's raw *text*. **Closed by
+  improvise-tej1**: since km71 the cache carries a `FormulaId` per formula, and
+  `ModelCommand::RemoveFormula` now takes that id.
 - `view-back` was listed here too, and **improvise-cfly removed it**: once
   PR #115 put `Command::ViewBack` in the view half there was nothing left to
   enrich. See "The three improvise-e24v listed as free" below.
@@ -689,8 +690,9 @@ The classes that remain:
   and `group_of(idx)` is exactly `ToggleGroup`'s `(category, group)`. The one
   thing to get right is that the frame is **windowed** and the cursor is
   absolute, so a client must reconcile the scroll offset first, as `render.rs`
-  does for the selection. `delete-formula-at-cursor` stays: it names a position
-  in the formula panel, not in the grid.
+  does for the selection. `delete-formula-at-cursor` left this list with
+  improvise-tej1: the position it names is in the formula panel, and the id
+  the panel's cache now carries is enough to resolve it.
 - **Anything touching storage or an arbitrary command line.** The host's by
   definition.
 - **The records-mode commit** — *except the two creating columns*. A wide-records
@@ -1066,8 +1068,14 @@ recoverable without reimplementing `parse_formula` on the client — the gap
 `decision(improvise-e24v)` recorded for `delete-formula-at-cursor`. km71 closed
 it the gotcha 47 way, by **enriching the projection**: `FormulaCache::formulas`
 is `Vec<FormulaEntry { id: FormulaId, raw }>`. The id is opaque on purpose
-(`context/code/core.md` §14). `delete-formula-at-cursor` is still declined in
-the browser; it now needs only its resolver.
+(`context/code/core.md` §18). improvise-tej1 then composed
+`delete-formula-at-cursor` in the browser on the same id. `RemoveFormula`
+names its formula by `FormulaId` on the wire and in the effect, and a delete
+whose id no longer resolves is **refused with the batch stopped**, where it was
+a silent no-op: a frame that named a formula someone else removed is rolled
+back whole (`Host::mutating_frame`) and answered `Rejected(MutationRefused)`.
+The script command `remove-formula <category> <target>` resolves its names to
+an id in `execute` and refuses out loud when nothing matches.
 
 One divergence is pinned rather than closed: a rename. The host's
 `ReplaceFormula` rewrites the status to say what was renamed and which formulas

context/code/core.md

@@ -1334,7 +1334,8 @@ outside `improvise-formula` can read a name out of it. That is deliberate. The
 stable-identity work (improvise-sboz, ADR 005) will make a formula's identity
 something other than its names, and every signature that already says
 `FormulaId` — `Model::formula`, `Model::replace_formula`,
-`ModelCommand::ReplaceFormula`, `FormulaSlot::Existing`, `FormulaEntry` — then
+`ModelCommand::ReplaceFormula`, `ModelCommand::RemoveFormula` (since improvise-tej1),
+`Model::delete_formula`, `FormulaSlot::Existing`, `FormulaEntry` — then
 changes by changing this one struct. A name pair spread across those signatures
 would have made the swap a sweep.
 

context/test-evidence/clients.md

@@ -4869,6 +4869,25 @@ disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the
 cell B cleared. Under that rival, the 0j0f test above failed as well. Both
 files were restored.
 
+### deleting_a_formula_from_the_panel_agrees_with_the_host
+
+improvise-tej1. Three formulas with the cursor on index 1, so "the one under the
+cursor" is neither the first nor the last. Rivals: the resolver deletes
+`formulas.first()` instead of the clamped cursor row, and this test fails on the
+document. The resolver never steps the cursor back, and this test fails on the
+cursor comparison in `both_ends`, which is the only thing that step decides.
+
+### deleting_a_formula_another_client_already_removed_is_refused_cleanly
+
+improvise-tej1. The client composes its delete, then the host loses `Tax` before
+the frame arrives. Rival: `effect::RemoveFormula` ignores a missing id, which is
+exactly what the old name-keyed `Model::remove_formula` did. The frame is then
+accepted with nothing removed, and this test fails because no
+`Rejected(MutationRefused)` comes back. The assertion that the document is
+unchanged is the other half: a refusal that also removed a neighbour would fail
+there.
+
+
 ### tile_select_opens_on_a_regular_category_on_both_ends
 
 **What broke (improvise-oplp).** `EnterTileSelect` and the browser's

context/test-evidence/command.md

@@ -6299,6 +6299,15 @@ flag. This test fails alone. Every other test stays green, which was the gap.
 Once improvise-fy9o re-seeds the reopened editor, a stale `touched` would make
 every untouched Tab or Enter write its seed.
 
+### remove_formula_of_a_missing_formula_is_a_refusal
+
+improvise-9trl, from the review of #223. The script command's `None` arm refuses,
+which is what makes `main.rs` exit 1, and nothing tested it. Rival: `None =>
+vec![]`, the silent no-op the command had before. With it, the whole suite
+stayed green, and this test now fails on `last_batch_refused`. The control
+arm, a formula that is present, must not refuse and must remove it, so a
+command that refused every call cannot pass.
+
 ### `enter_enter_leaves_every_cell_unchanged`
 
 **improvise-fy9o.** The keymap welded `clear-buffer` after every commit key, so

crates/improvise-command/src/command/cmd/effect_cmds.rs

@@ -67,6 +67,34 @@ mod tests {
         assert_eq!(view.categories_on(Axis::Column), vec!["Type", "Month"]);
     }
 
+    /// improvise-9trl: a script naming a formula that is not there is refused,
+    /// so `main.rs` exits 1. It was a silent no-op that exited 0.
+    #[test]
+    fn remove_formula_of_a_missing_formula_is_a_refusal() {
+        let mut m = two_cat_model();
+        let tax = crate::formula::parse_formula("Tax = Food * 0.1", "Type").unwrap();
+        m.model.add_formula(tax);
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let ctx = make_ctx(&m, &layout, &reg);
+        let run = |args: [&str; 2]| {
+            let args = args.iter().map(|s| s.to_string()).collect();
+            let mut app = crate::ui::app::App::new(m.clone(), None);
+            app.apply_effects(RemoveFormulaCmd(args).execute(&ctx));
+            app
+        };
+        let missing = run(["Type", "Profit"]);
+        assert!(
+            missing.last_batch_refused(),
+            "{}",
+            missing.view_state.status_msg
+        );
+        assert_eq!(missing.model_state.workbook.model.formulas().len(), 1);
+        let present = run(["Type", "Tax"]);
+        assert!(!present.last_batch_refused(), "the control must not refuse");
+        assert!(present.model_state.workbook.model.formulas().is_empty());
+    }
+
     #[test]
     fn write_cmd_without_args_saves() {
         let m = two_cat_model();
@@ -251,11 +279,19 @@ effect_cmd!(
     RemoveFormulaCmd,
     "remove-formula",
     |args: &[String]| require_args(args, 2),
-    |args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
-        vec![Box::new(effect::RemoveFormula {
-            target_category: args[0].clone(),
-            target: args[1].clone(),
-        })]
+    |args: &Vec<String>, ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
+        let (category, target) = (&args[0], &args[1]);
+        let named = ctx
+            .model
+            .formulas()
+            .iter()
+            .find(|f| &f.target_category == category && &f.target == target);
+        match named {
+            Some(f) => vec![Box::new(effect::RemoveFormula { id: f.id() })],
+            None => vec![effect::refuse(format!(
+                "No formula for {target} in {category}"
+            ))],
+        }
     }
 );
 

crates/improvise-command/src/command/cmd/panel.rs

@@ -402,7 +402,7 @@ mod tests {
         let effects = DeleteFormulaAtCursor.execute(&ctx);
         let dbg = effects_debug(&effects);
         assert!(
-            dbg.contains("RemoveFormula { target: \"Tax\", target_category: \"Type\" }"),
+            dbg.contains("target: \"Tax\", target_category: \"Type\""),
             "must remove the formula AT THE CURSOR (Tax, index 1), got: {dbg}"
         );
         assert!(
@@ -668,10 +668,8 @@ impl Cmd for DeleteFormulaAtCursor {
         let cursor = ctx.formula_cursor.min(formulas.len().saturating_sub(1));
         if cursor < formulas.len() {
             let f = &formulas[cursor];
-            let mut effects: Vec<Box<dyn Effect>> = vec![Box::new(effect::RemoveFormula {
-                target: f.target.clone(),
-                target_category: f.target_category.clone(),
-            })];
+            let mut effects: Vec<Box<dyn Effect>> =
+                vec![Box::new(effect::RemoveFormula { id: f.id() })];
             if cursor > 0 {
                 effects.push(Box::new(effect::SetPanelCursor {
                     panel: Panel::Formula,

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

@@ -231,17 +231,17 @@ impl Effect for AddFormula {
     }
 }
 
+/// Remove the formula `id` names. A formula already gone is refused and stops
+/// the batch, so a frame that named it is rolled back (improvise-tej1).
 #[derive(Debug)]
 pub struct RemoveFormula {
-    pub target: String,
-    pub target_category: String,
+    pub id: FormulaId,
 }
 impl Effect for RemoveFormula {
     fn apply(&self, app: &mut App) {
-        app.model_state
-            .workbook
-            .model
-            .remove_formula(&self.target, &self.target_category);
+        if let Err(e) = app.model_state.workbook.model.delete_formula(&self.id) {
+            refuse_and_stop(app, e.to_string());
+        }
     }
     fn kind(&self) -> EffectKind {
         EffectKind::Model(ModelChange::CellData)
@@ -1982,8 +1982,9 @@ mod tests {
         .apply(&mut app);
         assert!(!app.model_state.workbook.model.formulas().is_empty());
         RemoveFormula {
-            target: "Clothing".to_string(),
-            target_category: "Type".to_string(),
+            id: crate::formula::parse_formula("Clothing = Food * 2", "Type")
+                .unwrap()
+                .id(),
         }
         .apply(&mut app);
         assert!(app.model_state.workbook.model.formulas().is_empty());
@@ -2958,8 +2959,9 @@ mod tests {
             }),
             ("RemoveFormula", || {
                 Box::new(RemoveFormula {
-                    target: "Profit".into(),
-                    target_category: "_Measure".into(),
+                    id: crate::formula::parse_formula("Profit = Revenue - Cost", "_Measure")
+                        .unwrap()
+                        .id(),
                 })
             }),
         ];
@@ -2982,8 +2984,9 @@ mod tests {
             }
             if label == "AddFormula" {
                 app.apply_effects(vec![Box::new(RemoveFormula {
-                    target: "Profit".into(),
-                    target_category: "_Measure".into(),
+                    id: crate::formula::parse_formula("Profit = Revenue - Cost", "_Measure")
+                        .unwrap()
+                        .id(),
                 })]);
             }
             app.model_state.dirty = false;

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

@@ -3115,8 +3115,9 @@ mod tests {
                 target_category: "_Measure".into(),
             },
             ModelCommand::RemoveFormula {
-                target: "Profit".into(),
-                target_category: "_Measure".into(),
+                id: crate::formula::parse_formula("Profit = Revenue", "_Measure")
+                    .unwrap()
+                    .id(),
             },
         ]
     }

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

@@ -173,13 +173,7 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec<Box<dyn Effect>> {
             raw: raw.clone(),
             target_category: target_category.clone(),
         }),
-        ModelCommand::RemoveFormula {
-            target,
-            target_category,
-        } => Box::new(effect::RemoveFormula {
-            target: target.clone(),
-            target_category: target_category.clone(),
-        }),
+        ModelCommand::RemoveFormula { id } => Box::new(effect::RemoveFormula { id: id.clone() }),
         ModelCommand::ReplaceFormula { id, was, raw } => Box::new(effect::ReplaceFormula {
             id: id.clone(),
             was: was.clone(),
@@ -691,8 +685,9 @@ pub(crate) mod tests {
                 target_category: "_Measure".into(),
             },
             ModelCommand::RemoveFormula {
-                target: "Profit".into(),
-                target_category: "_Measure".into(),
+                id: crate::formula::parse_formula("Profit = Revenue", "_Measure")
+                    .unwrap()
+                    .id(),
             },
             ModelCommand::ReplaceFormula {
                 id: crate::formula::parse_formula("Profit = Revenue", "_Measure")

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

@@ -562,6 +562,18 @@ impl Model {
         &self.formulas
     }
 
+    /// Remove the formula `id` names, refusing if none does — a delete that
+    /// finds nothing is a stale claim, not a no-op (improvise-tej1).
+    pub fn delete_formula(&mut self, id: &FormulaId) -> Result<()> {
+        let pos = self
+            .formulas
+            .iter()
+            .position(|f| f.id() == *id)
+            .ok_or_else(|| anyhow!("That formula no longer exists"))?;
+        self.formulas.remove(pos);
+        Ok(())
+    }
+
     pub fn formula(&self, id: &FormulaId) -> Option<&Formula> {
         self.formulas.iter().find(|f| f.id() == *id)
     }
@@ -2352,6 +2364,16 @@ mod formula_tests {
         assert_eq!(targets(&m), vec!["Profit", "Total", "Tax", "Margin"]);
     }
 
+    #[test]
+    fn delete_formula_removes_the_one_its_id_names_and_refuses_a_stale_id() {
+        let mut m = three_formulas();
+        let tax = m.formulas[1].id();
+        m.delete_formula(&tax).unwrap();
+        assert_eq!(targets(&m), vec!["Profit", "Margin"]);
+        assert!(m.delete_formula(&tax).is_err(), "a second delete is stale");
+        assert_eq!(targets(&m), vec!["Profit", "Margin"]);
+    }
+
     #[test]
     fn a_formula_mentions_what_its_expression_or_its_where_names() {
         let margin = parse_formula("Margin = Profit / Revenue", "_Measure").unwrap();

crates/improvise-protocol/src/reduce.rs

@@ -107,9 +107,9 @@ pub enum ModelCommand {
         raw: String,
         target_category: String,
     },
+    /// Remove the formula `id` names; refused if it is already gone.
     RemoveFormula {
-        target: String,
-        target_category: String,
+        id: FormulaId,
     },
     /// Edit the formula `id` names in place, if it still reads `was` — an edit
     /// of that formula, not a remove-then-add (improvise-km71).
@@ -634,10 +634,7 @@ mod tests {
                 raw,
                 target_category,
             } => name(raw) + name(target_category),
-            ModelCommand::RemoveFormula {
-                target,
-                target_category,
-            } => name(target) + name(target_category),
+            ModelCommand::RemoveFormula { id } => formula(id),
             ModelCommand::ReplaceFormula { id, was, raw } => formula(id) + name(was) + name(raw),
             ModelCommand::CleanEmptyRecords | ModelCommand::SortData => 0,
             ModelCommand::AddCategory(category)

crates/improvise-protocol/src/resolve.rs

@@ -340,7 +340,6 @@ fn declined(name: &str, args: &[String]) -> Option<String> {
         "toggle-prune-empty" => "hiding empty rows and columns",
         "toggle-records-mode" => "records mode",
         "add-record-row" => "adding a record row",
-        "delete-formula-at-cursor" => "deleting a formula",
         // Reachable only behind a prefix, and a chord must be answered or it
         // strands its own disarm — improvise-hs0u, `context/code/clients.md`.
         "toggle-col-group-under-cursor" => "collapsing a column group",
@@ -503,6 +502,7 @@ fn composed(
         }),
         // ── the formula panel ────────────────────────────────────────
         "edit-formula-at-cursor" => Some(edit_formula_at(state, state.view.formula_cursor)),
+        "delete-formula-at-cursor" => delete_formula_at_cursor(state),
 
         "open-item-add-at-cursor" => Some(match cat_at_cursor(state) {
             Some(cat) => vec![Command::ChangeMode(AppMode::item_add(cat))],
@@ -1405,6 +1405,32 @@ fn switch_view_at(state: &ClientState, index: usize) -> Option<Vec<Command>> {
     ])
 }
 
+/// `DeleteFormulaAtCursor::execute`: the cursor clamped to the list, the
+/// formula named by its id, and the step back only away from the top.
+fn delete_formula_at_cursor(state: &ClientState) -> Option<Vec<Command>> {
+    let formulas = &state.formulas.formulas;
+    let cursor = state
+        .view
+        .formula_cursor
+        .min(formulas.len().saturating_sub(1));
+    let Some(entry) = formulas.get(cursor) else {
+        return Some(Vec::new());
+    };
+    let mut commands = vec![model(
+        state,
+        ModelCommand::RemoveFormula {
+            id: entry.id.clone(),
+        },
+    )?];
+    if cursor > 0 {
+        commands.push(Command::SetPanelCursor {
+            panel: Panel::Formula,
+            cursor: cursor - 1,
+        });
+    }
+    Some(commands)
+}
+
 /// `DeleteViewAtCursor::execute`, including the cursor step back that only
 /// happens away from the top of the list.
 fn delete_view_at_cursor(state: &ClientState) -> Option<Vec<Command>> {
@@ -2540,7 +2566,6 @@ mod tests {
             "EXPORT: Enter commit-export Silent",
             "SAVE AS: Enter commit-save Silent",
             // Honestly unavailable, and saying so in `declined`'s words.
-            "FORMULAS: Del/d delete-formula-at-cursor Declines",
             "NORMAL: > drill-into-cell Declines",
             "NORMAL: : enter-mode Declines",
             "NORMAL: P toggle-prune-empty Declines",

crates/improvise-wasm-client/tests/lockstep.rs

@@ -2892,3 +2892,66 @@ fn renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host() {
     assert_eq!(client.view.status_msg, "Formula updated");
     assert_eq!(app.view_state.status_msg, "Renamed Tax to Levy");
 }
+
+/// `d` in the formula panel, at a NONZERO cursor over three formulas: the
+/// client names the formula by its id, and both ends step the cursor back.
+#[test]
+fn deleting_a_formula_from_the_panel_agrees_with_the_host() {
+    let mut app = furnished_host();
+    add_formulas(&mut app);
+    app.view_state.formula_panel_open = true;
+    app.view_state.mode = AppMode::FormulaPanel;
+    app.view_state.formula_cursor = 1;
+    let mut twin = twin_of(&app);
+    add_formulas(&mut twin);
+    let mut client = furnished_client(&app, (2, 1));
+    client.formulas = improvise_command::ui::project::formulas_of(&app);
+
+    both_ends(&mut app, &mut twin, &mut client, "d");
+    assert_eq!(
+        raws(&app),
+        vec![FORMULAS[0], FORMULAS[2]],
+        "`d` must remove the formula under cursor 1, not the first or the last"
+    );
+    assert_eq!(app.view_state.formula_cursor, 0, "and step the cursor back");
+}
+
+/// The id the client names was deleted by someone else first: the frame is
+/// refused whole and the document keeps what it had.
+#[test]
+fn deleting_a_formula_another_client_already_removed_is_refused_cleanly() {
+    let mut app = furnished_host();
+    add_formulas(&mut app);
+    app.view_state.formula_panel_open = true;
+    app.view_state.mode = AppMode::FormulaPanel;
+    app.view_state.formula_cursor = 1;
+    let mut client = furnished_client(&app, (2, 1));
+    client.formulas = improvise_command::ui::project::formulas_of(&app);
+    let event = BrowserKey::new("d").to_key_event().unwrap();
+    let KeyOutcome::Local(cmds) = resolve(&client, event) else {
+        panic!("`d` in the formula panel resolves here");
+    };
+
+    let mut host = improvise_command::ui::host::Host::new(app);
+    let (id, _) = host.connect(2, 1);
+    let model = &mut host.app_mut().model_state.workbook.model;
+    model.remove_formula("Tax", "_Measure");
+    let before = document_text(host.app());
+
+    let replies = host.handle(id, ClientFrame::Commands(cmds));
+    assert!(
+        replies.iter().any(|a| matches!(
+            a.msg,
+            improvise_protocol::ServerFrame::Rejected(
+                improvise_protocol::Rejection::MutationRefused
+            )
+        )),
+        "a delete of a formula that no longer exists must be refused: {replies:?}"
+    );
+    assert_eq!(
+        document_text(host.app()),
+        before,
+        "nothing else was removed"
+    );
+    assert_eq!(raws(host.app()), vec![FORMULAS[0], FORMULAS[2]]);
+}