diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 02fc441e..0d25d431 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -6784,6 +6784,37 @@ and cursor are back where they were. It is asserted by value (`Normal`, `(0, 0)`), not only as a difference from the snapshot, so it reads as a claim about what the user sees. The ticket's rival fails it too. +### a_frame_after_a_rolled_back_reshape_resolves_against_the_restored_grid + +improvise-8wy9, found in the adversarial review of PR #254. `App::restore` ends +with `self.rebuild_layout()`, because `app.layout` is derived state and not part +of `AppSnapshot`. Deleting that line kept the whole workspace green: +`snapshot_restore`'s tests compare only the snapshot's own fields, and nothing +drove a frame after a rollback. Yet `Host::undo_frame` calls `restore` and does +not rebuild. `App::resolve_aggregate`, `record_in_layout` and `unwritable` +all read `self.layout`, so the next frame would resolve its surrogates against +the grid of the frame that was rolled back. + +The test drives the host's own path. The fixture stores **no** cells, so South is +a record known only by its position in the layout. A frame +`[RemoveItem(South), SetCell(South)]` reshapes the grid, is refused +`ChainBroken`, and is rolled back. The test then asserts two things: + +- the host's layout has two rows again (the direct check); +- a next frame writing South's empty cell is accepted and stores 7. This is the + consequence the bead names. + +**Rival** (the bead's: delete `self.rebuild_layout();` from `App::restore`). Run +across the whole workspace, it fails **only** this test, at the row count (1 +against 2). With the row-count assertion removed, the behavioural half fails +alone: the write to South is refused `RecordGone`, resolved against the grid in +which South had been removed. So each half discriminates on its own, and the +behavioural one is the reason the rebuild exists. + +Placed in `ui/host.rs` rather than in `ui/app.rs`'s `snapshot_restore` module +(which PR #254 adds), both to avoid that conflict and because the consequence is +the host's. + ## `ui/host.rs` — the abort arm of `Host::mutating_frame` (improvise-we1a) ### every_failing_model_effect_that_aborts_also_refuses diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 051d5209..27a5aa50 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -968,6 +968,71 @@ mod tests { ); } + /// After a refused frame reshaped the grid, the NEXT frame resolves against the + /// restored layout (improvise-8wy9). Evidence: `context/test-evidence/command.md`. + #[test] + fn a_frame_after_a_rolled_back_reshape_resolves_against_the_restored_grid() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); + assert_eq!(host.app().layout.row_count(), 2, "setup: North and South"); + let basis = frame_basis(&host, &["South"]); + let refused = host.handle( + asker, + ClientFrame::Commands(vec![ + Command::Model { + basis: basis.clone(), + cmd: ModelCommand::RemoveItem { + category: "Region".into(), + item: "South".into(), + }, + }, + Command::Model { + basis, + cmd: ModelCommand::SetCell { + cell: addressed("South"), + value: CellValue::Number(5.0), + }, + }, + ]), + ); + assert!( + refused + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::ChainBroken(_)))), + "setup: the frame that removed South must be refused and rolled back: {refused:?}" + ); + assert_eq!( + host.app().layout.row_count(), + 2, + "the restored document has South again, so its grid must too" + ); + + // South stores nothing: it is known only by its position in the layout. + let answer = host.model( + asker, + ModelCommand::SetCell { + cell: addressed("South"), + value: CellValue::Number(7.0), + }, + ); + assert!( + !answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), + "a write to South was refused against the rolled-back grid: {answer:?}" + ); + assert_eq!( + host.app() + .model_state + .workbook + .model + .get_cell(&cell("South")), + Some(&CellValue::Number(7.0)) + ); + } + /// The rollback reaches the MIRRORS, not only the document — the cost the /// ticket called its least obvious. See `context/test-evidence/command.md`. #[test]