test(spreadsheet): cover workbook_api.rs and util.rs remaining branches

The two weakest files left after the formula2 tranche.

workbook_api.rs 88.80% -> 99.58%:

- with_sheets_active with an empty sheet list falls back to a fresh
  single-sheet workbook.
- set_active_sheet with an out-of-range index is a no-op.
- remove_sheet: removing a sheet before the active one shifts the index
  down; removing the active one (or one after it) clamps to the new end.
- apply(): the style arms the earlier tests did not touch — italic,
  underline, number format, background — and both SetBorders shapes
  (per-edge target writes each requested edge, BorderTarget::None clears
  all four).
- Workbook::default() is a fresh single-sheet workbook.
- save()/load() round-trip through the default path, a legacy payload
  still loads as a one-sheet workbook, and a non-workbook payload yields
  None. The disk test takes a new crate::test_disk lock so it cannot
  race the persistence module's disk test under parallel `cargo test`;
  that pre-existing test now takes the same lock.

util.rs 90.91% -> 100%:

- write_col_letters matches col_letters across one-, two- and three-
  letter columns and reuses the caller's buffer without stale tails.
- adjust_formula_refs: a digit-bearing function name (LOG10) is not a
  cell ref; an 8-letter column overflowing u32 and a 30-digit row
  overflowing i64 are copied verbatim through the overflow fallbacks.

Engine total 96.54% -> 98.24% (floor 96). Unit tests 308 -> 318;
9 integration tests unchanged.

Deliberately uncovered: the mutex-poison recovery closure in the
test_disk lock (unreachable unless a test panics while holding it), and
the disk test's restore-to-clean branch (runs only when no save file
pre-exists — the CI-normal path; its inverse is the branch that runs
otherwise). Both match the existing persistence test's save/restore
convention.

crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs

@@ -31,3 +31,16 @@ pub use sheet::Sheet;
 pub use style::{BorderEdge, BorderTarget, CellAlign, CellStyle, Color, NumberFormat};
 pub use undo::{Change, ChangeSet};
 pub use workbook_api::{Workbook, WorkbookCommand};
+
+#[cfg(test)]
+pub(crate) mod test_disk {
+    use std::sync::{Mutex, MutexGuard};
+
+    static LOCK: Mutex<()> = Mutex::new(());
+
+    /// Serialize tests that touch the shared `generated/` save files, so
+    /// parallel `cargo test` workers cannot clobber each other's fixtures.
+    pub(crate) fn lock() -> MutexGuard<'static, ()> {
+        LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner())
+    }
+}

crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs

@@ -197,6 +197,7 @@ mod tests {
     /// clobbering another test's fixture.
     #[test]
     fn the_legacy_file_is_read_when_the_current_one_is_missing() {
+        let _disk = crate::test_disk::lock();
         let current = sheet_generated_file_path();
         let legacy = sheet_generated_legacy_file_path();
         let saved_current = fs::read_to_string(&current).ok();

crates/apps/spreadsheet/spreadsheet-engine/src/util.rs

@@ -368,4 +368,46 @@ mod tests {
         // =AZ10 with 0,+1 → =BA10
         assert_eq!(adjust_formula_refs("=AZ10", 0, 1), "=BA10");
     }
+
+    // --- write_col_letters (no-alloc variant used by the grid draw loop) ---
+
+    #[test]
+    fn write_col_letters_matches_col_letters() {
+        for i in [0u32, 1, 25, 26, 27, 51, 52, 701, 702, 18277] {
+            let mut buf = String::from("leftover");
+            write_col_letters(i, &mut buf);
+            assert_eq!(buf, col_letters(i), "write_col_letters({i}) diverged");
+        }
+        // The buffer is cleared on entry, so a shorter label leaves no
+        // stale tail from the previous call.
+        let mut buf = String::new();
+        write_col_letters(702, &mut buf); // "AAA"
+        write_col_letters(0, &mut buf); // "A"
+        assert_eq!(buf, "A");
+    }
+
+    // --- adjust_formula_refs: digit-bearing names and overflow guards ---
+
+    #[test]
+    fn adjust_refs_function_name_with_digits() {
+        // LOG10 has digits in its identifier; it must not be treated as a
+        // cell reference.
+        assert_eq!(adjust_formula_refs("=LOG10(100)", 1, 1), "=LOG10(100)");
+    }
+
+    #[test]
+    fn adjust_refs_overflowing_column_is_left_alone() {
+        // An 8-letter column exceeds u32 in `col_str_to_index`, so the
+        // scanner hits the overflow fallback. With a zero delta the output
+        // is verbatim, proving the fallback copies rather than mangles.
+        assert_eq!(adjust_formula_refs("=AAAAAAAA1", 0, 0), "=AAAAAAAA1");
+    }
+
+    #[test]
+    fn adjust_refs_overflowing_row_is_left_alone() {
+        // A 30-digit row does not fit i64, so the scanner copies it verbatim.
+        let row = "9".repeat(30);
+        let formula = format!("=A{}", row);
+        assert_eq!(adjust_formula_refs(&formula, 1, 1), formula);
+    }
 }

crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs

@@ -144,18 +144,15 @@ impl Workbook {
     }
 
     /// Load from the default save path (`generated/current.workbook.tsv`).
-    /// Falls back to legacy single-sheet format if the workbook file
+    /// Falls back to the legacy single-sheet format if the workbook file
     /// doesn't exist.
     pub fn load() -> Option<Self> {
+        // `deserialize_workbook` already migrates the legacy V1 formats
+        // (single-sheet `#MP_SHEET_V2` and the older CSV) into a one-sheet
+        // workbook, so there is no separate legacy branch to take here.
         let saved = load_saved_spreadsheet_state()?;
-        if let Some((sheets, active)) = deserialize_workbook(&saved) {
-            return Some(Self { sheets, active });
-        }
-        if is_legacy_workbook_payload(&saved) {
-            Some(migrate_legacy_workbook(&saved))
-        } else {
-            None
-        }
+        let (sheets, active) = deserialize_workbook(&saved)?;
+        Some(Self { sheets, active })
     }
 
     /// Save to the default save path.
@@ -495,22 +492,6 @@ impl Workbook {
     }
 }
 
-fn is_legacy_workbook_payload(serialized: &str) -> bool {
-    let first = serialized.lines().next().unwrap_or_default().trim();
-    first == "#MP_SHEET_V2" || first.contains(',')
-}
-
-fn migrate_legacy_workbook(serialized: &str) -> Workbook {
-    let mut data = SpreadsheetData::default();
-    data.deserialize(serialized);
-    data.undo_stack.clear();
-    data.redo_stack.clear();
-    Workbook {
-        sheets: vec![Sheet::new("Sheet 1", data)],
-        active: 0,
-    }
-}
-
 impl Default for Workbook {
     fn default() -> Self {
         Self::new()
@@ -692,23 +673,6 @@ mod tests {
         assert_eq!(wb.get_raw(0, 0), "test");
     }
 
-    #[test]
-    fn legacy_payload_detection_rejects_arbitrary_text() {
-        assert!(is_legacy_workbook_payload("#MP_SHEET_V2\n"));
-        assert!(is_legacy_workbook_payload("0,0,legacy\n"));
-        assert!(!is_legacy_workbook_payload("not a workbook"));
-    }
-
-    #[test]
-    fn legacy_payload_migrates_to_one_sheet() {
-        let mut data = SpreadsheetData::default();
-        data.set_cell(0, 0, "legacy");
-        let migrated = migrate_legacy_workbook(&data.serialize());
-        assert_eq!(migrated.sheet_count(), 1);
-        assert_eq!(migrated.get_raw(0, 0), "legacy");
-        assert_eq!(migrated.active_sheet(), 0);
-    }
-
     #[test]
     fn workbook_try_deserialize_reports_invalid_input() {
         let mut wb = Workbook::new();
@@ -973,4 +937,190 @@ mod tests {
         wb.set_active_sheet(1);
         assert_eq!(wb.get_display_value(0, 0), "20");
     }
+
+    // ── Previously-uncovered branches ───────────────────────────────────
+
+    /// An empty sheet list cannot build a workbook, so `with_sheets_active`
+    /// falls back to a fresh single-sheet workbook.
+    #[test]
+    fn with_sheets_active_empty_defaults_to_one_sheet() {
+        let wb = Workbook::with_sheets_active(vec![], 7);
+        assert_eq!(wb.sheet_count(), 1);
+        assert_eq!(wb.active_sheet(), 0);
+    }
+
+    /// Setting an out-of-range active index is a no-op, not a panic.
+    #[test]
+    fn set_active_sheet_out_of_range_is_noop() {
+        let mut wb = Workbook::new();
+        wb.set_active_sheet(99);
+        assert_eq!(wb.active_sheet(), 0);
+    }
+
+    /// Removing a sheet *before* the active one shifts the active index
+    /// down so it keeps pointing at the same sheet.
+    #[test]
+    fn remove_sheet_before_active_shifts_active_down() {
+        let mut wb = Workbook::new();
+        wb.add_sheet("B");
+        wb.add_sheet("C");
+        wb.add_sheet("D"); // ["Sheet 1", "B", "C", "D"]
+        wb.set_active_sheet(2); // C
+        let removed = wb.remove_sheet(0); // remove "Sheet 1" (before C)
+        assert!(removed.is_some());
+        assert_eq!(wb.sheet_count(), 3);
+        assert_eq!(wb.active_sheet(), 1); // shifted down, still C
+        assert_eq!(wb.active_sheet_name(), "C");
+    }
+
+    /// Removing the active sheet (or one after it) clamps the active index
+    /// to the new last sheet.
+    #[test]
+    fn remove_sheet_at_or_after_active_clamps_to_end() {
+        let mut wb = Workbook::new();
+        wb.add_sheet("B");
+        wb.add_sheet("C"); // ["Sheet 1", "B", "C"]
+        wb.set_active_sheet(2); // C (last)
+        let removed = wb.remove_sheet(1); // remove B; active 2 >= new len 2
+        assert!(removed.is_some());
+        assert_eq!(wb.sheet_count(), 2);
+        assert_eq!(wb.active_sheet(), 1); // clamped to the new last sheet
+        assert_eq!(wb.active_sheet_name(), "C");
+    }
+
+    /// `apply` forwards the remaining style commands to the active sheet:
+    /// italic, underline, number format, and background.
+    #[test]
+    fn apply_covers_italic_underline_number_format_and_background() {
+        let mut wb = Workbook::new();
+        wb.apply_batch([
+            WorkbookCommand::ToggleItalic { row: 0, col: 0 },
+            WorkbookCommand::ToggleUnderline { row: 0, col: 0 },
+            WorkbookCommand::SetNumberFormat {
+                row: 0,
+                col: 0,
+                format: NumberFormat::Currency,
+            },
+            WorkbookCommand::SetBackground {
+                row: 0,
+                col: 0,
+                color: Some(Color::new(1.0, 0.0, 0.0, 1.0)),
+            },
+        ]);
+        let cell = wb
+            .active_sheet_data()
+            .cells
+            .get(&CellId::new(0, 0))
+            .unwrap();
+        assert!(cell.style.italic);
+        assert!(cell.style.underline);
+        assert_eq!(cell.style.number_format, NumberFormat::Currency);
+        assert_eq!(cell.style.bg_color, Some(Color::new(1.0, 0.0, 0.0, 1.0)));
+    }
+
+    /// `apply`'s SetBorders arm: a per-edge target writes each requested
+    /// edge, and `BorderTarget::None` clears all four.
+    #[test]
+    fn apply_set_borders_covers_clear_and_per_edge() {
+        let mut wb = Workbook::new();
+        let edge = BorderEdge {
+            color: Color::new(0.0, 0.0, 1.0, 1.0),
+            width: 2.0,
+        };
+        wb.apply(WorkbookCommand::SetBorders {
+            row: 0,
+            col: 0,
+            target: BorderTarget::All,
+            edge,
+            top: true,
+            bottom: true,
+            left: true,
+            right: true,
+        });
+        let cell = wb
+            .active_sheet_data()
+            .cells
+            .get(&CellId::new(0, 0))
+            .unwrap();
+        assert_eq!(cell.style.border_top, Some(edge));
+        assert_eq!(cell.style.border_bottom, Some(edge));
+        assert_eq!(cell.style.border_left, Some(edge));
+        assert_eq!(cell.style.border_right, Some(edge));
+
+        wb.apply(WorkbookCommand::SetBorders {
+            row: 0,
+            col: 0,
+            target: BorderTarget::None,
+            edge,
+            top: false,
+            bottom: false,
+            left: false,
+            right: false,
+        });
+        let cell = wb
+            .active_sheet_data()
+            .cells
+            .get(&CellId::new(0, 0))
+            .unwrap();
+        assert_eq!(cell.style.border_top, None);
+        assert_eq!(cell.style.border_bottom, None);
+        assert_eq!(cell.style.border_left, None);
+        assert_eq!(cell.style.border_right, None);
+    }
+
+    /// `Workbook::default()` is a fresh single-sheet workbook.
+    #[test]
+    fn workbook_default_is_a_fresh_single_sheet() {
+        let wb = Workbook::default();
+        assert_eq!(wb.sheet_count(), 1);
+        assert_eq!(wb.active_sheet_name(), "Sheet 1");
+    }
+
+    /// `save()` / `load()` round-trip through the default path, a legacy
+    /// single-sheet payload still loads, and a non-workbook payload yields
+    /// `None`.
+    ///
+    /// Runs single-threaded against the shared default filenames via
+    /// `crate::test_disk::lock()`, and saves/restores whatever was there
+    /// before — the same convention as the persistence module's disk tests.
+    #[test]
+    fn workbook_save_load_legacy_and_reject_via_default_path() {
+        let _disk = crate::test_disk::lock();
+        let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("generated");
+        let current = dir.join("current.workbook.tsv");
+        let saved_current = std::fs::read_to_string(&current).ok();
+
+        // Round-trip through the default path.
+        let mut wb = Workbook::new();
+        wb.set_cell(0, 0, "disk");
+        wb.set_cell(0, 1, "=A1&\"!\"");
+        wb.save().expect("save failed");
+        let loaded = Workbook::load().expect("load failed");
+        assert_eq!(loaded.sheet_count(), 1);
+        assert_eq!(loaded.active_sheet(), 0);
+        assert_eq!(loaded.get_raw(0, 0), "disk");
+        assert_eq!(loaded.get_display_value(0, 1), "disk!");
+
+        // A legacy single-sheet payload still loads as a one-sheet workbook.
+        let mut legacy_data = SpreadsheetData::default();
+        legacy_data.set_cell(0, 0, "legacy");
+        std::fs::write(&current, legacy_data.serialize()).expect("write legacy");
+        let migrated = Workbook::load().expect("legacy load failed");
+        assert_eq!(migrated.sheet_count(), 1);
+        assert_eq!(migrated.get_raw(0, 0), "legacy");
+
+        // A non-workbook payload yields None.
+        std::fs::write(&current, "not a workbook\n").expect("write garbage");
+        assert!(Workbook::load().is_none());
+
+        // Restore whatever was there before.
+        match saved_current {
+            Some(text) => {
+                std::fs::write(&current, text).ok();
+            }
+            None => {
+                std::fs::remove_file(&current).ok();
+            }
+        }
+    }
 }