diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..fcbee537 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2382,6 +2382,31 @@ keystroke to land in. Injected fault: dropping `self.view_state = snapshot.view_state` from `App::restore` fails this test and only this test. +**Addendum (improvise-gho5).** Improvise-gho5 found that this +test and `a_rolled_back_load_tells_nobody_and_puts_every_mirror_back` passed +against a host that **never ran the load**. The review's fault, reproduced +here, was placed at the top of `Host::commands`: refuse any multi-command frame +that contains a `LoadModel` with `Rejected(DocumentReplaced)`, before +`CheckedBatch` runs. Both tests stayed green. A frame that never got that far +leaves the generation, the view, the status line and every mirror untouched, +which is exactly what the tests asserted. + +Both tests now assert that the asker's whole reply is +`the_load_ran_then_the_write_broke()`, which is `Rejected(ChainBroken(North))`. +They also assert, before the frame, that `North` resolves. Together these make +the rejection a witness that the load ran. `ChainBroken` comes only from +`reduce::broken_chain`, which runs inside `mutating_frame` just before each +mutation. It names `North` only once the document no longer holds that +record, and only an applied `LoadModel` in this frame can have removed it. + +With the same fault re-injected, both tests fail. The left side is +`[Rejected(DocumentReplaced)]` and the right is +`[Rejected(ChainBroken())]`. The fault was reverted. + +The residual: a rogue up-front check that forged `ChainBroken` specifically +would still pass. That is the same forgery a hand-rolled client could commit +against any basis check, and a test cannot guard against it. + ### a_refused_mutation_rolls_back_its_predecessor_and_the_abort_flag The `Rejection::MutationRefused` arm — the one the first hand-back wrongly called @@ -4885,9 +4910,14 @@ Three things make it non-vacuous: `LoadModel` silently failed, `Applied::Rebased` would never run and every assertion here would pass over nothing. The control asserts the generation moves and that *both* clients are owed a `Snapshot`. -- **The rollback is asserted before anything is concluded from it** — the - `document_generation` equality is checked first, so "the frame was rejected" - cannot be confused with "the frame never got that far". +- **The load is witnessed to have RUN before anything is concluded from the + rollback** (improvise-gho5). The asker's whole reply must be + `Rejected(ChainBroken(North))`. `North` is asserted to resolve before the + frame, so only `broken_chain` can give that answer, and only after the + `LoadModel` has replaced the document. `CheckedBatch` never produces + `ChainBroken`. This bullet used to say the `document_generation` equality + did the job, and that was false: generation equality is exactly what holds + when the frame never got that far. - **A bystander is seated where a wrong relay could move it.** A single-client fixture cannot tell "the mirrors were restored" from "there was one mirror and it happened to be the asker's". diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 50d7b392..166f16e0 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1049,6 +1049,12 @@ mod tests { let was_view = host.app().view_state.active_view.clone(); let was_status = host.app().view_state.status_msg.clone(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands, then the write names a record the incoming document // has never had — so the frame fails at its second command. @@ -1069,11 +1075,10 @@ mod tests { ]), ); - assert!( - answer - .iter() - .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), - "the fixture must actually refuse the frame: {answer:?}" + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" ); assert_eq!( host.app().view_state.active_view, @@ -1097,6 +1102,12 @@ mod tests { ); } + /// `North` resolves in the outgoing document, so only `broken_chain` AFTER the + /// load applied can answer this. Evidence: `context/test-evidence/command.md`. + fn the_load_ran_then_the_write_broke() -> ServerFrame { + ServerFrame::Rejected(Rejection::ChainBroken(addressed("North").record)) + } + /// Everything one client was told, flattened to the projections it carries. fn projections(frames: &[Addressed], id: SessionId) -> Vec { to(frames, id) @@ -1147,6 +1158,12 @@ mod tests { let (bystander, _) = host.connect(10, 10); let was_generation = host.app().document_generation(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands and rebases every mirror; the write then names a // record the incoming document has never had, so the frame fails. @@ -1167,6 +1184,11 @@ mod tests { ]), ); + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" + ); assert_eq!( host.app().document_generation(), was_generation,