Merge pull request #134 from fiddlerwoaroof/fix/jz6w-keyoutcome-frame

fix(protocol): a key can ask its host to save (improvise-jz6w)

context/code/clients.md

@@ -480,7 +480,7 @@ command crate is not on a browser's dependency list and never will be. The
 *keys* that reach it are no longer spelled twice. `scrolled_to` mirrors
 `navigation.rs::viewport_effects` over the window this client announced.
 
-### The three outcomes
+### The four outcomes
 
 `KeyOutcome::Local(Vec<Command>)` — resolved here; the caller folds them locally
 *and* sends them, as one frame and in this order (the asker's own fold is its
@@ -491,11 +491,45 @@ with. **Never empty** — a keypress this client can answer with nothing is
 `Ignored`, and `KeyOutcome::of` is the one constructor so that contract is a
 property of the type's users rather than a rule each resolver remembers.
 
+`Request(Request)` — resolved here, but as a **request to the host** rather than
+as state this client can compose. Today the one member is `Request::Save`.
 `Upstream(KeyEvent)` — bound to a command this client cannot compose.
 `Ignored` — nothing bound, or what is bound resolves to no change. **Nothing
 bound is not "the host might know better"**: the host looks the key up in this
 same table. That is new; it used to go upstream into an empty vec.
 
+### Why `Request` is its own enum and not a `ClientFrame` (improvise-jz6w)
+
+`ClientFrame::Save` and a `Host::handle` arm for it had existed since
+improvise-368h, and **no key could reach either**: `KeyOutcome` had no shape
+that said "send this frame", so `save` sat on the host-needed list, resolved to
+`Upstream`, and landed in the black hole. This is the gap a vocabulary audit
+cannot see — nothing was missing from `Command` or `ModelCommand`, and the
+enumeration that would have found it is over **`KeyOutcome`'s variants**, which
+are as much of the protocol surface as the frames are. Enumerate them too.
+
+The fourth variant could have carried a whole `ClientFrame`. It does not, and
+the reason is not tidiness: a resolver handing out arbitrary frames could
+compose a `Viewport`, whose extent is the *page's* fact and not the keymap's
+(gotcha 47), or a `Commands` batch — which would be **indistinguishable on the
+wire from a `Local`** while silently skipping the local fold that `Local`'s
+whole contract is. The narrow enum costs one type and one `frame()` mapping, and
+in exchange a request is exactly "ask the host to do a thing". `Request::frame`
+is the one place a resolver's decision becomes an envelope.
+
+Two consequences. `requested` is checked **first** in `step`, before `declined`
+and `composed`, so no name can be in two lists — the same layering the other two
+already have, and `composed` has no `"save"` arm to go stale. And a request
+**cannot travel inside a `Binding::Sequence`**: a sequence is one frame and a
+request is a frame of its own, so a sequence naming one is refused as
+`Upstream`. Nothing binds one there; `no_binding_puts_a_request_inside_a_
+sequence` is what keeps that arm unreachable rather than merely untested.
+
+What `Request` deliberately does **not** do is become a `Command`. Gotcha 45's
+argument stands: a `ClientFrame::Commands` batch promises all-or-nothing, and a
+runtime-fallible write inside one reported a failed save and then applied the
+edit that followed it.
+
 A `Binding::Sequence` is **all or nothing**: half of `Esc` — clearing the buffer
 without leaving the mode — is worse than none of it. Each step is resolved
 against the *unchanged* state, because the host resolves every step against one

context/test-evidence/clients.md

@@ -207,6 +207,12 @@ in a browser with no change to this crate. Each entry is driven through the key
 that reaches it, because a name that is not bound anywhere would otherwise sit
 in the list forever looking like a gap.
 
+**It is a live gate, and improvise-jz6w exercised it.** `save` left the list
+when Ctrl-S became a `Request`; putting the entry back fails with *"save
+(Char('s')) was resolved locally; if that is deliberate, this list is where it
+stops being a gap"*. So the mechanism gotcha 47 asks for was watched failing
+rather than assumed.
+
 ### a_prompt_this_client_cannot_commit_is_never_entered
 
 The refusal list above is only half the property. `Upstream` says the key
@@ -3744,3 +3750,65 @@ text before either later assertion is reached — the host writes `View 4` with
 resets it to `,.2`. That failure is the evidence that this file is where the
 one-derivation claim is witnessed, and that a test per front end would have
 passed on both sides of the divergence.
+
+### ctrl_s_in_the_browser_writes_the_hosts_document
+
+improvise-jz6w, end to end and in one test on purpose. `ClientFrame::Save` and
+`Host::handle`'s arm for it landed with improvise-368h and were covered by
+`a_client_can_ask_its_host_to_save` in `ui/host.rs` — which **constructs the
+frame itself**. Nothing anywhere pressed a key and watched a document get
+written, so a `KeyOutcome` with no shape for "send this frame" left the feature
+complete on both sides of a seam that did not exist. Two half-tests that never
+meet is the exact failure mode, which is why this one runs a real `Client`'s
+`key()` into a real `Host` over a real `App`.
+
+The fixture is `browser_round_trip.rs`'s own `host()` given a `file_path` and a
+`MemoryFs`, so `Host::save` has somewhere to land and the test can read it back
+through `App::storage`. The pre-assertion that storage is empty before the key
+is pressed is what stops a fixture that was written at construction time from
+passing this vacuously.
+
+**Against HEAD it failed** with `left: Key { code: Char('s'), modifiers:
+CONTROL } right: Save` — the black hole of gotcha 47, observed rather than
+argued.
+
+**Two rivals were injected.** Mapping `Request::Save` to
+`ClientFrame::Commands(vec![])` — the wide-variant mistake this ticket declined,
+a request smuggled into the batch envelope — fails on the frame assertion with
+`left: Commands([])`. That is the one the storage assertion alone could not see
+with certainty, because an empty batch writes nothing either way; asserting the
+**frame** is what separates "reached the host" from "reached the host as the
+right kind of thing". Deleting `requested`'s `"save"` arm restores `Upstream`
+and fails identically to HEAD.
+
+### ctrl_s_asks_the_host_to_save_and_composes_nothing
+
+The unit half of the same fix, and the one that states the *negative*: `composed`
+must return `None` for `"save"`. A resolver that grew a `"save"` arm composing
+some `Command` would satisfy the end-to-end test only by accident of ordering
+(`requested` is checked first in `step`), and would be dead code the moment that
+order changed. Asserting the absence is what keeps gotcha 45's reason — a
+runtime-fallible write does not belong in an all-or-nothing batch — enforced at
+the resolver rather than remembered.
+
+It also pins `Request::Save.frame() == ClientFrame::Save`, the one mapping from
+a resolver's decision to an envelope.
+
+**Rival:** renaming `requested`'s match arm so `"save"` no longer matches. The
+test fails at the first assertion with `Upstream(...)`, which is what the
+browser did before this ticket.
+
+### no_binding_puts_a_request_inside_a_sequence
+
+`sequence` refuses a `Step::Request` by returning `None`, which becomes
+`Upstream` — a black hole. That is only acceptable because nothing binds a
+request inside a `Binding::Sequence`, and "nothing does" is a claim about the
+keymap that nothing else checks. This test walks every mode's bindings,
+recursing into prefixes and parents exactly as `bound_commands` does, and
+requires that no sequence step is a `requested` name.
+
+It asserts the walk found **some** sequence steps before checking any of them:
+a walk that silently visited nothing would otherwise pass forever, which is the
+vacuous-assertion shape design-principles §7 names. The same test is what makes
+the refusal arm safe to leave unreachable instead of inventing a second frame
+for it.

crates/improvise-protocol/src/lib.rs

@@ -31,7 +31,9 @@ pub use render::{
     ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind,
     WizardProposal, WizardStepKind, header_level_widths,
 };
-pub use resolve::{Click, KeyOutcome, click, read_buffer, resolve, typed, typed_cell_command};
+pub use resolve::{
+    Click, KeyOutcome, Request, click, read_buffer, resolve, typed, typed_cell_command,
+};
 pub use viewmodel::{CatTreeEntry, flatten_cat_tree, view_axis_summary};
 // `improvise-core`'s, because the index that resolves one lives in
 // `DataStore`. Re-exported so a client depends on the protocol alone.

crates/improvise-protocol/src/resolve.rs

@@ -4,6 +4,7 @@
 use improvise_core::view::Axis;
 
 use crate::client::ClientState;
+use crate::frame::ClientFrame;
 use crate::keymap::{Binding, Keymaps, ModeKey};
 use crate::viewmodel::{CatTreeEntry, axis_change_status, flatten_cat_tree};
 use crate::{
@@ -21,6 +22,9 @@ pub enum KeyOutcome {
     /// Resolved here: the caller folds them locally *and* sends them, as one
     /// frame, in order. Never empty. See `context/code/clients.md`.
     Local(Vec<Command>),
+    /// Resolved here as a **request**: the caller sends the frame and folds
+    /// nothing. Why it is not a `Command`: `context/code/clients.md`.
+    Request(Request),
     /// Bound to a command this client cannot compose. Forwarded to the host,
     /// which today answers nothing — `context/code/clients.md`.
     Upstream(KeyEvent),
@@ -29,6 +33,24 @@ pub enum KeyOutcome {
     Ignored,
 }
 
+/// What a key may ask its **host** to do, as opposed to what it may compose.
+/// Narrow on purpose — the argument is in `context/code/clients.md`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Request {
+    /// Write the document back where the host keeps it (improvise-368h).
+    Save,
+}
+
+impl Request {
+    /// The frame that carries it. The one mapping, so a resolver never names
+    /// a [`ClientFrame`] and cannot compose one this outcome does not mean.
+    pub fn frame(self) -> ClientFrame {
+        match self {
+            Request::Save => ClientFrame::Save,
+        }
+    }
+}
+
 impl KeyOutcome {
     /// `Local` when there is something to say, `Ignored` when not — the one
     /// constructor, so "never empty" is a property rather than a rule.
@@ -59,6 +81,7 @@ pub fn resolve(state: &ClientState, event: KeyEvent) -> KeyOutcome {
         Binding::Prefix(_) => KeyOutcome::Upstream(event),
         Binding::Cmd { name, args } => match step(state, event, name, args) {
             Step::Local(commands) | Step::Declined(commands) => KeyOutcome::of(commands),
+            Step::Request(request) => KeyOutcome::Request(request),
             Step::Upstream => KeyOutcome::Upstream(event),
         },
         // All or nothing, each step resolved against the *unchanged* state.
@@ -205,7 +228,9 @@ fn sequence(
         match step(state, event, name, args) {
             Step::Local(more) => commands.extend(more),
             Step::Declined(said) => return Some(said),
-            Step::Upstream => return None,
+            // A request is a frame of its own and a sequence is one frame, so
+            // there is nowhere to put it. No binding does — clients.md.
+            Step::Request(_) | Step::Upstream => return None,
         }
     }
     Some(commands)
@@ -216,21 +241,34 @@ fn sequence(
 enum Step {
     Local(Vec<Command>),
     Declined(Vec<Command>),
+    Request(Request),
     Upstream,
 }
 
-/// One name, answered. A decline is checked first, so [`composed`] has no arm
-/// for any of them and the two lists cannot overlap.
+/// One name, answered. Each list is checked before the next, so no name is in
+/// two of them and [`composed`] has no arm for any earlier one.
 fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Step {
-    match declined(name, args) {
-        Some(said) => Step::Declined(vec![Command::SetStatus(said)]),
-        None => match composed(state, event, name, args) {
-            Some(commands) => Step::Local(commands),
-            None => Step::Upstream,
+    match requested(name) {
+        Some(request) => Step::Request(request),
+        None => match declined(name, args) {
+            Some(said) => Step::Declined(vec![Command::SetStatus(said)]),
+            None => match composed(state, event, name, args) {
+                Some(commands) => Step::Local(commands),
+                None => Step::Upstream,
+            },
         },
     }
 }
 
+/// The command names this client answers by **asking its host**, rather than
+/// by composing anything. See `context/code/clients.md` (improvise-jz6w).
+fn requested(name: &str) -> Option<Request> {
+    match name {
+        "save" => Some(Request::Save),
+        _ => None,
+    }
+}
+
 /// The commands this client cannot compose and answers **out loud**; the one
 /// home for the wording. Why silence was worse: clients.md (improvise-rmvx).
 fn declined(name: &str, args: &[String]) -> Option<String> {
@@ -2179,10 +2217,8 @@ mod tests {
             // `N` left this list with improvise-koh6: both its arms are
             // answered here, one of them out loud. See clients.md.
             (ev(KeyCode::Enter), "enter-advance"),
-            (
-                KeyEvent::new(KeyCode::Char('s'), KeyModifiers::CONTROL),
-                "save",
-            ),
+            // Ctrl-S left this list with improvise-jz6w: it is a `Request`,
+            // not a `Command`, and the host has answered it since 368h.
             (
                 KeyEvent::new(KeyCode::Char('q'), KeyModifiers::CONTROL),
                 "force-quit",
@@ -2198,6 +2234,68 @@ mod tests {
         }
     }
 
+    /// improvise-jz6w: the outcome a resolver had no way to name. See
+    /// `context/test-evidence/clients.md`.
+    #[test]
+    fn ctrl_s_asks_the_host_to_save_and_composes_nothing() {
+        let state = in_mode(AppMode::Normal);
+        let key = KeyEvent::new(KeyCode::Char('s'), KeyModifiers::CONTROL);
+        assert_eq!(
+            resolve(&state, key),
+            KeyOutcome::Request(Request::Save),
+            "Ctrl-S must resolve to the save REQUEST"
+        );
+        assert_eq!(
+            Request::Save.frame(),
+            crate::ClientFrame::Save,
+            "and the one mapping must name the frame the host already answers"
+        );
+        assert_eq!(
+            composed(&state, key, "save", &[]),
+            None,
+            "`save` must compose NO command: a Commands batch is all-or-\
+             nothing, which is why Save is a frame (gotcha 45)"
+        );
+    }
+
+    /// A request is one frame, so a sequence has nowhere to put one. The arm
+    /// that refuses is unreachable, and this is what keeps it so.
+    #[test]
+    fn no_binding_puts_a_request_inside_a_sequence() {
+        fn steps_of(keymap: &Keymap, out: &mut Vec<String>) {
+            for binding in keymap.bindings().values() {
+                match binding {
+                    Binding::Sequence(steps) => {
+                        out.extend(steps.iter().map(|(name, _)| name.to_string()))
+                    }
+                    // Prefixes are flat (gotcha 13), so this terminates.
+                    Binding::Prefix(sub) => steps_of(sub, out),
+                    Binding::Cmd { .. } => {}
+                }
+            }
+            if let Some(parent) = keymap.parent() {
+                steps_of(parent, out);
+            }
+        }
+
+        let mut names = Vec::new();
+        for (_, keymap) in Keymaps::shared().iter() {
+            steps_of(keymap, &mut names);
+        }
+        assert!(
+            !names.is_empty(),
+            "the walk found no sequence steps at all, so it proves nothing"
+        );
+        for name in &names {
+            assert_eq!(
+                requested(name),
+                None,
+                "`{name}` is bound inside a sequence, and a request cannot \
+                 travel in one: see clients.md"
+            );
+        }
+    }
+
     /// improvise-cfly. The two directions of the view history disagree in the
     /// `Cmd`, and the client mirrors the disagreement — evidence file.
     #[test]

crates/improvise-wasm-client/src/lib.rs

@@ -5,7 +5,7 @@ pub mod keys;
 pub mod render;
 pub mod socket;
 
-pub use improvise_protocol::{Click, ClientState, KeyOutcome, resolve};
+pub use improvise_protocol::{Click, ClientState, KeyOutcome, Request, resolve};
 pub use keys::BrowserKey;
 pub use render::{RenderCatRow, RenderCell, RenderModel, RenderRow};
 
@@ -214,6 +214,9 @@ impl Client {
                 }
                 Some(ClientFrame::Commands(cmds))
             }
+            // Nothing to fold: a request moves no state of this client's, and
+            // what it moves on the host comes back as a projection.
+            KeyOutcome::Request(request) => Some(request.frame()),
             KeyOutcome::Upstream(event) => Some(ClientFrame::key(event)),
             KeyOutcome::Ignored => None,
         }

crates/improvise-wasm-client/tests/browser_round_trip.rs

@@ -488,3 +488,75 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() {
         );
     }
 }
+
+// ── Ctrl-S, all the way to storage (improvise-jz6w) ─────────────────────
+
+/// The same document, over `MemoryFs` and knowing where it lives, so a save
+/// has somewhere to land and the test can read it back.
+fn host_with_a_path() -> (Host, std::path::PathBuf) {
+    let path = std::path::PathBuf::from("doc.improv");
+    let mut host = host();
+    host.app_mut().model_state.file_path = Some(path.clone());
+    host.app_mut().storage = Box::new(improvise_command::storage::MemoryFs::new());
+    (host, path)
+}
+
+/// Ctrl-S, as a browser reports it.
+fn ctrl_s() -> improvise_wasm_client::BrowserKey {
+    improvise_wasm_client::BrowserKey {
+        key: "s".to_string(),
+        ctrl: true,
+        alt: false,
+        shift: false,
+        meta: false,
+    }
+}
+
+/// improvise-jz6w: `ClientFrame::Save` existed and `Host::handle` answered it,
+/// but no `KeyOutcome` could name it. See `context/test-evidence/clients.md`.
+#[test]
+fn ctrl_s_in_the_browser_writes_the_hosts_document() {
+    let (mut host, path) = host_with_a_path();
+    let mut client = Client::new();
+    let handshake: ClientFrame =
+        serde_json::from_str(&client.set_viewport(10, 10)).expect("the viewport frame parses");
+    let ClientFrame::Viewport { rows, cols } = handshake else {
+        panic!("set_viewport must produce a Viewport frame");
+    };
+    let (id, snapshot) = host.connect(rows, cols);
+    deliver(&mut client, &[snapshot]);
+    assert!(
+        host.app().storage.read(&path).is_err(),
+        "nothing may be on storage before the key is pressed"
+    );
+
+    let frame = client
+        .key(&ctrl_s())
+        .expect("Ctrl-S must produce a frame to send");
+    assert_eq!(
+        frame,
+        ClientFrame::Save,
+        "the frame must be the save REQUEST — a Key frame is the black hole \
+         this ticket is about, and a Commands batch is the atomicity Save is \
+         a frame to escape"
+    );
+    let answer = send(&mut host, id, &serde_json::to_string(&frame).unwrap());
+
+    let written = String::from_utf8(
+        host.app()
+            .storage
+            .read(&path)
+            .expect("the keypress must have reached this host's storage"),
+    )
+    .expect("the document is text");
+    assert!(
+        written.contains("North"),
+        "and it must be this document, got:\n{written}"
+    );
+    assert!(
+        !answer
+            .iter()
+            .any(|a| matches!(a.msg, improvise_protocol::ServerFrame::Rejected(_))),
+        "a save that landed is not a rejection: {answer:?}"
+    );
+}