Pin the renames half of the qz9p check; say every staged edit was discarded (improvise-6r2y, improvise-o9q7)
6r2y: a sparse drill row with no Type coordinate that stages a Type rename after Type is removed reaches the unwritable check only through its renames. Red with .chain(renames.keys()) removed: an orphan Drink/Jan/Cost cell is written. o9q7: when any row is unwritable, merged keeps only the unwritable rows' reasons, as one refusal saying every staged edit was discarded, so a later writable row's blank-coordinate refusal cannot overwrite it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
+14
-3
@@ -997,7 +997,9 @@ coordinates nothing owns. The fix has two parts.
|
||||
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
|
||||
the row names still exists, both its coordinates and its renames (a sparse
|
||||
row with no coordinate for a category it renames reaches the check only
|
||||
through the renames; improvise-6r2y). 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.
|
||||
@@ -1005,8 +1007,17 @@ coordinates nothing owns. The fix has two parts.
|
||||
(`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.
|
||||
contract rules out. So when any row is unwritable, `merged` writes nothing.
|
||||
- **What the user is told (improvise-o9q7).** `merged` first kept every row's
|
||||
refusals. Refusals are applied in row order and each one overwrites the
|
||||
status line, so a later writable row's own refusal, such as a blank
|
||||
coordinate, replaced the one that mattered. The user read "Record coordinates
|
||||
cannot be empty" when in fact every staged edit had been thrown away. So when
|
||||
any row is unwritable, `merged` keeps only the unwritable rows' reasons and
|
||||
combines them into one refusal that says so:
|
||||
`Unknown category 'Type': every staged edit was discarded`. Sibling refusals
|
||||
are dropped because nothing those rows staged is written, so their reasons
|
||||
describe edits that are no longer pending.
|
||||
- **`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`,
|
||||
|
||||
@@ -5629,3 +5629,44 @@ 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.
|
||||
|
||||
## `ui/effect.rs` — the renames half and the refusal wording of the qz9p check (improvise-6r2y, improvise-o9q7)
|
||||
|
||||
### a_sparse_drill_row_renamed_into_a_removed_category_is_refused_and_writes_nothing
|
||||
|
||||
improvise-6r2y. The qz9p check asks whether every category a row names still
|
||||
exists, and it collects those names from **two** sources: the row's coordinates
|
||||
and the categories it renames. Before this test, deleting the renames half
|
||||
(`.chain(renames.keys().copied())`) left the lib green. The rename tests all
|
||||
use rows that already carry a `Type` coordinate, so the coordinates half caught
|
||||
them anyway.
|
||||
|
||||
This test uses the one row that reaches the renames half only: a sparse record
|
||||
`Jan/Cost = 17` with no `Type` coordinate, which stages `Type → Drink` after
|
||||
`Type` is removed.
|
||||
|
||||
Fault injected: the `.chain(...)` line commented out. The test failed on the
|
||||
store assertion, because the commit wrote an orphan `Drink/Jan/Cost = 17`. The
|
||||
refusal assertion alone would not have caught it: `AddItem` into the missing
|
||||
category refuses by itself, so `refused` is true under the fault. The store
|
||||
assertion is what makes the test work.
|
||||
|
||||
### an_unwritable_drill_commit_reports_the_discard_not_a_later_rows_refusal
|
||||
|
||||
improvise-o9q7. Row 0 names the removed `Type` and stages `Cost = 5`. Row 1, the
|
||||
sparse record, stages a blank `Month`. `merged` used to keep both rows'
|
||||
refusals. They were applied in row order, so the status line ended on row 1's
|
||||
"Record coordinates cannot be empty". That hid the fact that the whole commit
|
||||
had been discarded.
|
||||
|
||||
This test was red before the fix: the status line was exactly
|
||||
`Record coordinates cannot be empty`. It asserts three things:
|
||||
- the status names `Unknown category 'Type'`;
|
||||
- the status says `every staged edit was discarded`;
|
||||
- the status does **not** carry the blank-coordinate refusal.
|
||||
|
||||
Reading from those three assertions, not by injecting them, each of these
|
||||
partial fixes would fail the test:
|
||||
- one that only rewords the message;
|
||||
- one that only filters out the writable rows' refusals;
|
||||
- one that emits the combined refusal first and the siblings' refusals after it.
|
||||
|
||||
@@ -803,21 +803,32 @@ impl RecordRewrite {
|
||||
/// 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 {
|
||||
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,
|
||||
let rewrites: Vec<Self> = rewrites.into_iter().collect();
|
||||
match rewrites.iter().any(|one| one.unwritable) {
|
||||
true => Self::discarded(rewrites.into_iter().filter(|one| one.unwritable)),
|
||||
false => rewrites.into_iter().fold(Self::default(), |mut all, one| {
|
||||
all.refusals.extend(one.refusals);
|
||||
all.items.extend(one.items);
|
||||
all.clears.extend(one.clears);
|
||||
all.sets.extend(one.sets);
|
||||
all
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
/// One refusal naming every gone category and saying the whole commit was dropped,
|
||||
/// so no writable row's refusal can overwrite it (improvise-o9q7).
|
||||
fn discarded(unwritable: impl Iterator<Item = Self>) -> Self {
|
||||
let why: std::collections::BTreeSet<String> =
|
||||
unwritable.flat_map(|one| one.refusals).collect();
|
||||
let why: Vec<String> = why.into_iter().collect();
|
||||
Self {
|
||||
refusals: vec![format!(
|
||||
"{}: every staged edit was discarded",
|
||||
why.join("; ")
|
||||
)],
|
||||
unwritable: true,
|
||||
..Self::default()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3514,8 +3525,9 @@ mod tests {
|
||||
assert_eq!(store, std::collections::BTreeSet::new(), "no orphan cell");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows() {
|
||||
/// A drill over `Food/Jan/Cost = 2` (row 0) and a sparse `Jan/Cost = 17` naming no
|
||||
/// Type (row 1).
|
||||
fn typed_and_untyped_drill() -> App {
|
||||
let mut app = test_app();
|
||||
let typed = drill_cell("Food", "Jan", "Cost");
|
||||
let untyped = CellKey::new(vec![
|
||||
@@ -3533,9 +3545,43 @@ mod tests {
|
||||
assert_eq!(records.len(), 2);
|
||||
assert_eq!(records[1].coords.get("Type"), None, "row 1 names no Type");
|
||||
StartDrill(records).apply(&mut app);
|
||||
app
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_sparse_drill_row_renamed_into_a_removed_category_is_refused_and_writes_nothing() {
|
||||
let (store, refused) = commit_after_removing_type(
|
||||
app,
|
||||
typed_and_untyped_drill(),
|
||||
&[(1, category("Type"), "Drink")],
|
||||
);
|
||||
assert!(refused, "the commit must say it could not be applied");
|
||||
assert_eq!(
|
||||
store,
|
||||
["-/Jan/Cost = 17".to_string()].into_iter().collect(),
|
||||
"row 1 names Type only through its rename; no orphan Drink cell"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unwritable_drill_commit_reports_the_discard_not_a_later_rows_refusal() {
|
||||
let mut app = typed_and_untyped_drill();
|
||||
app.model_state.workbook.remove_category("Type");
|
||||
stage(&mut app, 0, measure("Cost"), "5");
|
||||
stage(&mut app, 1, category("Month"), "");
|
||||
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
|
||||
let status = &app.view_state.status_msg;
|
||||
assert!(status.contains("Unknown category 'Type'"), "{status}");
|
||||
assert!(
|
||||
status.contains("every staged edit was discarded"),
|
||||
"{status}"
|
||||
);
|
||||
assert!(!status.contains(RECORD_COORDS_CANNOT_BE_EMPTY), "{status}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows() {
|
||||
let (store, refused) = commit_after_removing_type(
|
||||
typed_and_untyped_drill(),
|
||||
&[(0, measure("Cost"), "5"), (1, measure("Cost"), "999")],
|
||||
);
|
||||
assert!(refused, "the commit must say it could not be applied");
|
||||
|
||||
Reference in New Issue
Block a user