fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(command): one basis per frame, then the chain runs in order

Browse Source 
The user has settled the contract, and it rejects the "at most one document
mutation per frame" rule this branch shipped. Verbatim: *"a sequence of
commands sent to the server should assume that it's played in order from
start to end and that each command sees the state after the previous command
runs"*, sent as *"a single transaction from a given base"*. Four clauses:

1. A frame states **one basis**, describing the base the client composed
   against.
2. That basis is checked **once**, against that base, before anything runs.
3. Commands are applied **in order**, and command N sees the state command
   N-1 left. Data dependence between them is expected, not an error.
4. **All or nothing.**

This commit lands clauses 1–3 and removes the rejected rule. **Clause 4 is
not implemented** and is documented as an open hole rather than a closed one;
the design note on PR #26 covers what rollback costs.

**Per-command basis checking is not merely awkward, it is incapable**, which
is the finding that makes clause 3 necessary rather than preferable. A client
cannot claim a counter for a record its own chain is about to create, because
`BasisCache::claim` filters to records the cache has been told about — so
`[AddItem("East"), SetCell(East)]`, the archetypal chain, is refused
`RecordGone`/`RecordUnstated` by a per-command check *in any order*. The base
describes what the client **saw**; the chain's internal coherence is the
transaction's job, not the basis check's.

So `CheckedBatch::against` answers the basis on the first command that
carries one and admits the rest through a private
`Checked::within_an_answered_frame`. Private is what stops it being a bypass:
from outside the module the only doors are still `Checked::against` and
`CheckedBatch::against`.

**Two things the rejected rule got wrong, kept as the lesson.** *Arity is not
a property of a variant* — a future `Command::RunScript` is one variant
performing unbounded mutations, so no count over the vocabulary can ever be
right. And *the transaction boundary was never the frame*: because one
command may already be a chain, clause 4 is needed inside a command as much
as across a frame of them, which is why the design note puts the mechanism at
`App::apply_effects`.

`mutates_the_document` is **deleted rather than kept**, which is a deliberate
deviation from the instruction to keep it: with the rule gone it has no
caller, and dead production code behind an `#[allow]` is worse than none. The
reason it was made exhaustive — a `matches!` answers `false` for a future
variant in silence — is preserved in the `CheckedBatch` doc, where clause 4
will read it when the classifier returns.

`Rejection::FrameNotAtomic` goes with the rule; nothing can produce it. The
taxonomies on `Rejection` and `ServerFrame::Rejected` return to two classes
and name the third that arrives with rollback.

Evidence, both faults injected:

- `a_data_dependent_chain_lands_in_order` — `[AddItem("East"), SetCell(East)]`
  through a real `Host`. Restoring the per-command check fails it, which is
  the whole point. It asserts **both** commands landed, because asserting
  only the first would pass against a host that dropped the second.
- `a_frame_whose_later_mutation_fails_is_not_yet_undone` characterizes the
  open hole with `[RemoveItem("North"), SetCell(North)]`: the removal lands,
  the write resolves to nothing and `model_effects` returns an **empty**
  batch, and the frame is reported accepted having applied half of itself.
  Both the partial application and the silence are open. **This test inverts
  when clause 4 lands**; until then its job is to fail loudly if anyone
  believes the hole is closed.
- The worker's `a_partly_refused_batch_…` fixture moved its injected
  staleness to the **first** command, since a later one no longer carries an
  answer of its own. Removing the injection still fails it.

The hole is unreachable from any client in this workspace — the only producer
of a multi-command frame is `keys::commit`, one mutation among view commands
— which is what makes it safe to document rather than rush.

Merged `main` (3915994). One semantic conflict: PR #27 added a
`mark_dirty()` call in `commit.rs::add_category`, which no longer exists —
dropped, since `AddCategory` is a `Model` effect and `apply_effects` raises
the flag.

1245 passing; clippy, fmt and the wasm gate clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:46:52 -07:00
parent 6b7a044a01
commit 90ea26e7fa
7 changed files with 242 additions and 300 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/improvise-command/src/command/cmd/commit.rs
-1
View File
@@ -379,7 +379,6 @@ fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
return;
}
effects.push(Box::new(effect::AddCategory(name.to_string())));
effects.push(effect::mark_dirty());
}
fn commit_cell_value(
crates/improvise-command/src/ui/host.rs
+96 -133
View File
@@ -734,165 +734,128 @@ mod tests {
);
}
/// improvise-955n, the half Copilot found on PR #26 — twice, because the
/// first answer fixed the example rather than the rule.
/// **The capability the contract is for**: a chain of data-dependent
/// commands, applied in order, each seeing what the previous one left.
///
/// Answering every command against the pre-frame `App` is right for the
/// claims a client can actually construct, and it is *only* right while
/// no command in the frame can invalidate another's. Two document
/// mutations break that, and **the failure is silent**:
/// `[RemoveItem("North"), SetCell(the North record)]` passes both checks,
/// then `RemoveItem` deletes the record, `model_effects` resolves the
/// already-answered `SetCell` against the mutated document, finds
/// nothing, and returns an empty effect batch. The frame is reported as
/// accepted having applied only its first command, and the client
/// believes its edit landed. Note this is a **regression the batch check
/// introduced**: under the old per-command check the `SetCell` would have
/// been refused with `RecordGone`.
/// `[AddItem("East"), SetCell(the East record)]` is the archetype, and it
/// is the case a per-command basis check **cannot** admit however it is
/// ordered: a client cannot claim a counter for a record its own chain is
/// about to create, because `BasisCache::claim` filters to records the
/// cache has been told about. So the basis is answered once, on the first
/// command that carries one, and the rest of the chain runs against what
/// its predecessors left.
///
/// So the rule is the count, not a list of dangerous variants: **a frame
/// may carry at most one document mutation**. The first fix refused only
/// the document-*replacing* case, which is the same bug seen through one
/// variant — `[LoadModel, TransposeAxes]` — instead of through the rule.
/// **Three** shapes are driven here, because a guard written for the one
/// that came to mind is what got this wrong three times running
/// (`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`).
/// The third is `SwitchView`, which a first version excluded on the
/// grounds that a record surrogate is view-independent — true of
/// `SetCell` and `ClearCell` and false of every other model command,
/// because they all act on the *active view*.
///
/// The accepting arms are the point of the test as much as the refusals:
/// one mutation surrounded by view commands must land whole, or
/// "refused" would be true for the wrong reason and the batch shape
/// improvise-bpq2 actually produces would be broken.
/// The second assertion is the one that earns the test. Asserting only
/// that the item exists would pass against a host that applied the first
/// command and dropped the second — which is exactly what the previous
/// rule did, loudly, and what the pre-frame check did, silently.
#[test]
fn a_frame_carrying_two_document_mutations_is_refused_whole() {
fn a_data_dependent_chain_lands_in_order() {
use improvise_protocol::ModelCommand;
// Shape 1: a structural command that invalidates its sibling's
// surrogate. The one that was silently accepted.
let mut structural = host();
let (asker, _) = structural.connect(10, 10);
let mut host = host();
let (asker, _) = host.connect(10, 10);
let add = crate::ui::reduce::caught_up(
host.app(),
ModelCommand::AddItem {
category: "Region".into(),
item: "East".into(),
},
);
let write = crate::ui::reduce::caught_up(
host.app(),
ModelCommand::SetCell {
cell: addressed("East"),
value: CellValue::Number(9.0),
},
);
let answer = host.handle(asker, ClientFrame::Commands(vec![add, write]));
assert!(
!answer
.iter()
.any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
"a data-dependent chain must not be refused: {answer:?}"
);
assert!(
host.app()
.model_state
.workbook
.model
.category("Region")
.is_some_and(|c| c.ordered_item_names().contains(&"East")),
"the first command must land"
);
assert_eq!(
host.app()
.model_state
.workbook
.model
.get_cell(&cell("East")),
Some(&CellValue::Number(9.0)),
"and the second must land on top of it — a write to a record the \
frame itself created is the whole point"
);
}
/// **Characterization of the open hole (clause 4), not an endorsement.**
///
/// The contract says a frame is all-or-nothing. It is not yet:
/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is
/// not a rollback), so a chain whose later mutation fails leaves the
/// earlier one standing. `[RemoveItem("North"), SetCell(the North
/// record)]` is the shape: the removal lands, the write then resolves to
/// nothing, `model_effects` returns an **empty** batch, and the frame is
/// reported accepted having applied half of itself. Both the partial
/// application and the silence are open.
///
/// It is unreachable from any client here — the only producer of a
/// multi-command frame is `keys::commit`, which emits one mutation among
/// view commands — which is what makes it safe to document rather than
/// rush.
///
/// **This test must be inverted when clause 4 lands.** Its job until then
/// is to make the hole fail loudly if anyone believes it is closed.
#[test]
fn a_frame_whose_later_mutation_fails_is_not_yet_undone() {
use improvise_protocol::ModelCommand;
let mut host = host();
let (asker, _) = host.connect(10, 10);
let remove = crate::ui::reduce::caught_up(
structural.app(),
host.app(),
ModelCommand::RemoveItem {
category: "Region".into(),
item: "North".into(),
},
);
let write = crate::ui::reduce::caught_up(
structural.app(),
host.app(),
ModelCommand::SetCell {
cell: addressed("North"),
value: CellValue::Number(5.0),
},
);
let answer = structural.handle(asker, ClientFrame::Commands(vec![remove, write]));
let answer = host.handle(asker, ClientFrame::Commands(vec![remove, write]));
assert!(
matches!(
to(&answer, asker).as_slice(),
[ServerFrame::Rejected(Rejection::FrameNotAtomic)]
),
"{answer:?}"
);
assert!(
structural
!host
.app()
.model_state
.workbook
.model
.category("Region")
.is_some_and(|c| c.ordered_item_names().contains(&"North")),
"nothing in the frame may have been applied"
);
// Shape 2: a document replacement followed by a command that names
// no surrogate at all, so the resolve-to-nothing argument that
// covered shape 1's sibling does not cover it.
let (mut swapper, path) = host_over_a_loadable_document();
let (asker, _) = swapper.connect(10, 10);
let load = crate::ui::reduce::caught_up(swapper.app(), ModelCommand::LoadModel(path));
let reshape = crate::ui::reduce::caught_up(swapper.app(), ModelCommand::TransposeAxes);
let answer = swapper.handle(asker, ClientFrame::Commands(vec![load, reshape]));
assert!(
matches!(
to(&answer, asker).as_slice(),
[ServerFrame::Rejected(Rejection::FrameNotAtomic)]
),
"{answer:?}"
);
assert_eq!(
swapper.app().model_state.workbook.model.name,
"Test",
"the document must not have been replaced"
);
// Shape 3: `SwitchView` is a document mutation too, although it
// carries no basis. Every model command *except* the two that name a
// record by surrogate acts on the **active view**, so a switch
// beside one of them redirects it: this pair transposed `Second`
// rather than the view the transpose was composed against.
let mut switcher = host();
switcher
.app_mut()
.model_state
.workbook
.views
.insert("Second".to_string(), crate::view::View::new("Second"));
let (asker, _) = switcher.connect(10, 10);
let reshape = crate::ui::reduce::caught_up(switcher.app(), ModelCommand::TransposeAxes);
let answer = switcher.handle(
asker,
ClientFrame::Commands(vec![Command::SwitchView("Second".into()), reshape]),
"TODAY: the first command lands. Under clause 4 it must be undone."
);
assert!(
matches!(
to(&answer, asker).as_slice(),
[ServerFrame::Rejected(Rejection::FrameNotAtomic)]
),
"{answer:?}"
);
assert_eq!(
switcher.app().model_state.workbook.active_view,
"Default",
"nothing in the frame may have been applied"
);
// The accepting arm: one mutation, surrounded by the view commands a
// locally-resolved keypress really does batch with it.
let mut accepted = host();
let (asker, _) = accepted.connect(10, 10);
let edit = crate::ui::reduce::caught_up(
accepted.app(),
ModelCommand::SetCell {
cell: addressed("North"),
value: CellValue::Number(7.0),
},
);
accepted.handle(
asker,
ClientFrame::Commands(vec![
Command::SetSelected { row: 1, col: 0 },
edit,
Command::SetRowOffset(1),
]),
);
assert_eq!(
accepted
.app()
.model_state
.workbook
.model
.get_cell(&cell("North")),
Some(&CellValue::Number(7.0)),
"one mutation among view commands must still land"
);
let mirror = accepted.sessions.get_mut(asker).unwrap().view().cursor();
assert_eq!(
(mirror.selected, mirror.row_offset),
((1, 0), 1),
"and so must the view commands around it"
!answer
.iter()
.any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
"TODAY: the frame is reported accepted. Under clause 4 it must be \
refused, and this assertion inverts: {answer:?}"
);
}
crates/improvise-command/src/ui/reduce.rs
+105 -104
View File
@@ -112,126 +112,116 @@ pub struct Checked<'a>(&'a Command);
/// *not even partially*, and that was true of a single command and false of
/// a batch.
///
/// # One basis, and therefore at most one document mutation
/// # One basis, checked once, and then the chain runs
///
/// The question that had to be answered before any code moved: what does a
/// frame's basis mean when its later commands were composed against its
/// earlier ones' *effects*? A client resolving one keypress emits
/// `SetSelected` plus the offsets it computed from the selection it just set.
/// The contract, as the user stated it: *"a sequence of commands sent to the
/// server should assume that it's played in order from start to end and that
/// each command sees the state after the previous command runs"*, sent as
/// *"a single transaction from a given base"*. Four clauses:
///
/// There is **one** basis and it describes the document the client was
/// looking at when it composed the frame. That is not a choice: a client
/// cannot claim anything else, because
/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) hands back
/// what the server last *published*, and the server publishes after a batch
/// and never during one. So no client can hold a basis describing the
/// document as it would be halfway through its own frame.
/// 1. A frame states **one basis**, describing the base the client composed
/// against.
/// 2. That basis is checked **once**, against that base, before anything is
/// applied.
/// 3. Commands are applied **in order**, and command N sees the state command
/// N-1 left. Data dependence between them is expected, not an error.
/// 4. **All or nothing.** If any command fails, the frame leaves the document
/// exactly as the base described.
///
/// **That forces the rule: a frame may carry at most one document
/// mutation** — a [`Command::Model`] or a [`Command::SwitchView`].
/// Checking every command against the pre-frame `App`
/// is right for the claims that can exist, and it is *only* right while no
/// command in the frame can invalidate another's. Two document mutations in
/// one frame break that, and the failure is silent rather than loud:
/// `[RemoveItem("North"), SetCell(the North record)]` passes both checks,
/// then `RemoveItem` deletes the record and `model_effects` resolves the
/// already-answered `SetCell` against the mutated document, finds nothing,
/// and returns an **empty** effect batch. The frame is reported as accepted
/// having applied only its first command, and the client believes its edit
/// landed. (Found by Copilot on PR #26, against a first version of this type
/// that refused only the document-*replacing* case — which was the same bug
/// spotted through one variant instead of through the rule.)
/// Clauses 1–3 are here. **Clause 4 is not yet implemented** — see the hole
/// below and the design note on PR #26.
///
/// The rule costs nothing real. Two model mutations in one frame could never
/// both be honestly based — the second's claim describes a document that
/// stopped existing when the first landed — so refusing them is not a
/// restriction, it is that incoherence said out loud. A client that wants
/// two sends two frames and composes the second against what the first
/// earns it. And the batch shape that actually exists — `SetSelected` plus
/// the offsets `viewport_effects` returns, optionally around one edit — is
/// untouched, because a view command carries no basis and cannot invalidate
/// one.
/// The basis is checked once and **later commands are not checked against
/// the pre-frame state**, which is not a relaxation but the only contract
/// under which a data-dependent chain can exist at all. The record tier
/// makes that concrete: a client cannot claim a counter for a record its own
/// chain is about to create, because
/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) filters to
/// records the cache has been told about. So `[AddItem(East), SetCell(East)]`
/// — the archetypal chain — is refused `RecordGone`/`RecordUnstated` by a
/// per-command check no matter what order it is checked in. Per-command
/// checking is not awkward here; it is incapable.
///
/// **`Command::SwitchView` counts too**, although it carries no basis of its
/// own (ADR 001 names that hole and leaves it open). A first version excluded
/// it, on the grounds that a record surrogate is view-independent so a switch
/// cannot invalidate an edit beside it — true of `SetCell` and `ClearCell`,
/// and **false of every other model command**, because `TransposeAxes`,
/// `SetAxis`, `HideItem`, `SetNumberFormat` and their siblings all act on the
/// *active view*. `[SwitchView("Second"), TransposeAxes]` passed both checks
/// and then transposed `Second` rather than the view the transpose was
/// composed against. Copilot found that one too, which makes three rounds of
/// the same mistake: an argument that holds for the command that came to mind.
/// The rule is now "a document mutation", not "a `Command::Model`", and what
/// is left uncounted is only what genuinely cannot reach the document.
/// What the base means is therefore: **what the client saw**. The transaction
/// promises the whole chain lands on top of it or none of it does. The
/// chain's internal coherence is the transaction's job, not the basis
/// check's.
///
/// With that, atomicity is not an argument: at most one command in a frame
/// can be refused, and it is refused before anything is applied.
/// # The hole clause 4 leaves, stated rather than hidden
///
/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a
/// rollback), so a frame whose second mutation fails leaves the first
/// applied. `[RemoveItem("North"), SetCell(the North record)]` is the
/// example: the removal lands, the write then resolves to nothing and
/// `model_effects` returns an **empty** batch, so the frame is reported
/// accepted having applied half of itself. Both halves of that are open —
/// the partial application and the silence.
///
/// It is **unreachable from any client in this workspace**: the only producer
/// of a multi-command frame is `improvise-wasm-client::keys::commit`, which
/// emits one mutation among view commands. That is what makes it safe to
/// document rather than rush, and it is why an earlier "at most one document
/// mutation per frame" rule — which closed the hole by forbidding the
/// capability — was rejected.
///
/// # Two things the rule that was rejected got wrong, worth keeping
///
/// **Arity is not a property of a variant.** A future `Command::RunScript` is
/// one variant performing an unbounded number of mutations, so no count over
/// the vocabulary can be right: a classifier can honestly answer *whether* a
/// command changes the document, never *how many times*. A
/// `mutates_the_document` predicate written for the rejected rule is deleted
/// with it rather than kept unused — but it was made an **exhaustive match**
/// on review, because a `matches!` answers `false` for a future variant in
/// silence, and clause 4 will need that classifier back. Write it exhaustive
/// when it returns.
///
/// **The transaction boundary is not the frame.** Because one command may
/// already be a chain, clause 4 is a property the system needs *inside* a
/// command as much as across a frame of them, which is why the design note
/// puts the mechanism at `App::apply_effects` rather than here.
pub struct CheckedBatch<'a>(Vec<Checked<'a>>);
impl<'a> CheckedBatch<'a> {
/// Answer for every command in a frame, against the state the frame was
/// composed against — which is the state *before* any of them runs.
///
/// The first refusal is the frame's answer and nothing is applied. It
/// is a `Result` rather than a per-command verdict for the same reason
/// The refusal is the frame's answer and nothing is applied. It is a
/// `Result` rather than a per-command verdict for the same reason
/// [`Checked`] is a type rather than a guard: the only enforceable
/// version of "answer for all of them first" is not being able to reach
/// the commands without having done so.
/// version of "answer before applying" is not being able to reach the
/// commands without having done so.
///
/// The count comes first, because a frame carrying two mutations is one
/// no basis can describe — there is nothing to check it *against* before
/// deciding that.
/// **The basis is answered once**, on the first command that carries one
/// — clause 2 — and every later command is admitted without one, which
/// is clause 3. A frame with no mutation at all answers trivially.
///
/// The `Basis` on a *later* `Command::Model` is ignored rather than
/// compared, and that is an honest interim rather than the end state:
/// clause 1 says the **frame** states one basis, but the wire still
/// carries a `Basis` inside each `Command::Model` — right when one
/// command was one transaction, and now one field per command describing
/// something that belongs to the frame. Moving it to
/// `ClientFrame::Commands` would make "one basis per frame"
/// unrepresentable-otherwise instead of a convention, and that protocol
/// change belongs with clause 4.
pub fn against(app: &App, cmds: &'a [Command]) -> Result<Self, Rejection> {
let mutations = cmds.iter().filter(|cmd| mutates_the_document(cmd)).count();
match mutations > 1 {
true => Err(Rejection::FrameNotAtomic),
false => cmds
.iter()
.map(|cmd| Checked::against(app, cmd))
.collect::<Result<Vec<_>, _>>()
.map(Self),
let mut answered = false;
let mut checked = Vec::with_capacity(cmds.len());
for cmd in cmds {
let one = match (answered, cmd) {
// Clause 3: the chain's later commands are not compared to a
// base that describes a document they are not running
// against.
(true, _) => Checked::within_an_answered_frame(cmd),
(false, _) => {
answered = matches!(cmd, Command::Model { .. });
Checked::against(app, cmd)?
}
};
checked.push(one);
}
}
}
/// Does this command change the shared document?
///
/// The two `Command` variants `Sessions::apply` routes away from the mirror
/// (gotchas 36, 38), and it is deliberately the *document* question rather
/// than "does it carry a basis": `SwitchView` carries none and still moves
/// `Workbook::active_view`, which every view-config model command is
/// relative to. Anything else in the vocabulary is the asking client's own
/// view slice and cannot invalidate a sibling's claim.
///
/// **Exhaustive on purpose**, like [`records_named`], and for the same
/// reason: a `matches!` would answer `false` for a future variant in
/// silence, and silence is exactly the failure mode here — an uncounted
/// document mutation walks past the atomicity guard and is applied against a
/// document a sibling has already changed. A new `Command` variant fails the
/// build until it says which it is (raised by Copilot on PR #26).
fn mutates_the_document(cmd: &Command) -> bool {
match cmd {
Command::Model { .. } | Command::SwitchView(_) => true,
Command::SetSelected { .. }
| Command::SetRowOffset(_)
| Command::SetColOffset(_)
| Command::ResetCursor
| Command::ChangeMode(_)
| Command::SetStatus(_)
| Command::SetYanked(_)
| Command::SetSearchQuery(_)
| Command::SetSearchMode(_)
| Command::SetBuffer { .. }
| Command::ClearBuffer(_)
| Command::SetTileCatIdx(_)
| Command::SetPanelOpen { .. }
| Command::SetPanelCursor { .. }
| Command::ToggleCatExpand(_)
| Command::HelpPage(_)
| Command::SetTransientPrefix(_)
| Command::Project(_) => false,
Ok(Self(checked))
}
}
@@ -271,6 +261,17 @@ impl<'a> Checked<'a> {
}
}
/// A command in a frame whose basis has **already been answered**
/// (clause 2), admitted without a second answer (clause 3).
///
/// Private, and that is what keeps it from being a bypass: from outside
/// this module the only doors are still [`Checked::against`] and
/// [`CheckedBatch::against`], and the only caller here is the loop that
/// has just answered the frame's basis.
fn within_an_answered_frame(cmd: &'a Command) -> Self {
Self(cmd)
}
/// The command this proof is about.
///
/// It hands back a `&'a Command` rather than one borrowed from `&self`,
crates/improvise-protocol/src/basis.rs
+7 -19
View File
@@ -103,7 +103,7 @@ impl Basis {
/// about the host, and a frame carrying two mutations is answered by
/// splitting it, not by claiming a fresher basis.
///
/// **Three classes live here**, and the doc enumerates them because the
/// **Two classes live here**, and the doc enumerates them because the
/// name says "basis" and only one of them is about a basis:
///
/// 1. **Staleness** (ADR 001) — [`Rejection::DocumentReplaced`],
@@ -114,10 +114,12 @@ impl Basis {
/// [`Rejection::NoDocumentPath`] and [`Rejection::SaveFailed`], answering
/// a [`ClientFrame::Save`](crate::ClientFrame::Save) (improvise-368h).
/// Nothing went stale; there is nothing to re-compose.
/// 3. **A frame that cannot be answered as one unit** —
/// [`Rejection::FrameNotAtomic`]. Not a fact about the document at all but
/// about the message: it carries two mutations, and one basis cannot
/// describe both.
///
/// A third class is coming and is not here yet: a **transaction that was
/// rolled back** because a later command in a chain could not be applied
/// (clause 4 of the frame contract — see `CheckedBatch` in
/// `improvise-command`). Until rollback exists there is nothing to report,
/// because nothing is undone.
///
/// They share this type rather than getting refusal channels of their own,
/// for the reason the type exists: a refusal is "nothing happened, here is
@@ -188,20 +190,6 @@ pub enum Rejection {
/// fact a client can act on: the document it is editing is not on disk.
/// Reporting a failed write as success is the thing this exists to stop.
SaveFailed,
/// A `ClientFrame::Commands` carrying **more than one** document
/// mutation, which no single basis can describe (improvise-955n).
///
/// Not staleness: nothing has moved, and re-sending the same frame would
/// be refused again. It is a fact about the *message* — the second
/// mutation's claim describes a document that stops existing when the
/// first one lands. What a client does is send them as separate frames
/// and compose each against what the previous one earns it.
///
/// View commands are not mutations and are unaffected: the batch a
/// locally-resolved keypress produces — a selection plus the scroll
/// offsets that follow it, optionally around one edit — is exactly what
/// this permits.
FrameNotAtomic,
}
/// The basis a client holds, folded from the stream like any other widget
crates/improvise-protocol/src/frame.rs
+14 -18
View File
@@ -86,19 +86,17 @@ pub enum ClientFrame {
/// Every command is answered against the document the *frame* was
/// composed against, which is the only claim a client can construct — a
/// basis is published between batches and never during one. And
/// therefore **a frame may carry at most one document mutation** — a
/// [`Command::Model`] or a [`Command::SwitchView`]. Two could not both be
/// honestly based, and the second would be applied against a document its
/// own sibling had already changed. A frame that carries two is refused
/// with [`Rejection::FrameNotAtomic`].
/// therefore the basis is answered **once**, on the first command that
/// carries one, and every later command is applied against the state its
/// predecessor left. Data dependence between them is expected: a chain
/// may create a record and then write it. Checking a later command
/// against the pre-frame base would refuse exactly those chains, because
/// a client cannot claim a counter for a record it is about to create.
///
/// `SwitchView` counts although it carries no basis of its own, because
/// every model command except the two that name a record by surrogate
/// acts on the **active view**: `[SwitchView("Second"), TransposeAxes]`
/// would transpose `Second` rather than the view the transpose was
/// composed against. View commands are not mutations, so the batch a
/// locally-resolved keypress produces — a selection, its scroll offsets
/// and at most one edit — is unaffected.
/// **All or nothing is the contract and is not yet delivered.** A chain
/// whose later command fails leaves its predecessors applied —
/// `App::apply_effects` aborts without undoing. `CheckedBatch` in
/// `improvise-command` carries the four clauses and the open hole.
Commands(Vec<Command>),
/// Write the document back to wherever this host keeps it
/// (improvise-368h).
@@ -183,15 +181,13 @@ pub enum ServerFrame {
Commands(Vec<Command>),
/// Nothing was applied, and here is why.
///
/// Three causes, all refusals: the batch was composed against a picture
/// of the document that had moved (ADR 001, improvise-fkfg); it was a
/// [`ClientFrame::Save`] this host cannot honour (improvise-368h); or it
/// was a [`ClientFrame::Commands`] carrying more than one document
/// mutation, which no single basis can describe (improvise-955n). They
/// Two causes, both refusals: the batch was composed against a picture of
/// the document that had moved (ADR 001, improvise-fkfg), or it was a
/// [`ClientFrame::Save`] this host cannot honour (improvise-368h). They
/// share a frame because they are the same message — nothing happened,
/// addressed to the asker alone — and the [`Rejection`] says which, so
/// its variants are the list to match exhaustively rather than this
/// sentence.
/// sentence. A third will arrive with transactional rollback.
///
/// A third variant because `Sessions::apply` grew a third case, exactly
/// as `Snapshot`/`Commands` is. It is addressed to the one client that
crates/improvise-wasm-client/src/socket.rs
-5
View File
@@ -59,11 +59,6 @@ fn rejection_message(rejection: &Rejection) -> String {
"this host has nowhere to write the document — it was not saved".into()
}
Rejection::SaveFailed => "the document could not be written".into(),
// Not staleness and not a host limitation but a badly composed
// frame, so the wording names what to do rather than what moved.
Rejection::FrameNotAtomic => {
"that batch carried two document changes at once — send them separately".into()
}
}
}
crates/improvise-worker-server/src/worker.rs
+20 -20
View File
@@ -510,36 +510,36 @@ mod tests {
/// (improvise-gv9r), so a batch that *did* change something would be
/// written whatever else the answer contained.
///
/// The fixture is the shape that had the bug — an accepted geometry
/// command followed by a stale one — and the assertion is that the axes
/// on disk and the axes in memory agree.
/// The fixture is a refused frame with a command behind the refusal, and
/// the assertion is that the axes on disk and the axes in memory agree.
/// Under the settled contract the refusal comes from the **frame's**
/// basis — answered once, on the first command that carries one — so
/// that is where the staleness is injected; a later command carries no
/// answer of its own.
#[test]
fn a_partly_refused_batch_does_not_leave_disk_and_memory_disagreeing() {
let (mut worker, path) = worker_over_storage();
connect(&mut worker);
let moves = reduce::caught_up(worker.app(), ModelCommand::TransposeAxes);
let Command::Model { mut basis, cmd } = reduce::caught_up(
worker.app(),
ModelCommand::SetCell {
cell: RecordCell {
record: improvise_core::model::record::RecordHash::of(
&improvise_core::model::cell::CellKey::new(vec![(
"Region".to_string(),
"North".to_string(),
)]),
),
measure: Some("Revenue".to_string()),
},
value: CellValue::Number(1.0),
},
) else {
// The frame's basis is the FIRST command's, answered once (clause 2),
// so that is where the staleness must be injected: a later command
// carries no answer of its own any more.
let Command::Model { mut basis, cmd } =
reduce::caught_up(worker.app(), ModelCommand::TransposeAxes)
else {
unreachable!("caught_up builds a Command::Model")
};
basis.document_generation += 1;
let stale = Command::Model { basis, cmd };
let follows = reduce::caught_up(
worker.app(),
ModelCommand::AddItem {
category: "Region".to_string(),
item: "East".to_string(),
},
);
let out = worker.on_frame(ClientFrame::Commands(vec![moves, stale]));
let out = worker.on_frame(ClientFrame::Commands(vec![stale, follows]));
assert!(
matches!(out.as_slice(), [ServerFrame::Rejected(_)]),
"the fixture must actually be refused, got {out:?}"