From 97da79f136d6d3eda863629758d50d1cff8cd9d1 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 12 Aug 2026 17:20:15 +0200 Subject: [PATCH] fix: 1a review round 2 --- provenance was armed but never completed **The P1 was real and my previous fix only did half of it.** Arming and completing a `TypedEditRecord` are separate steps: `typed_edit_complete` is called from `insert_char` / `insert_char_over_region` and NOWHERE else, so routing the single-scalar branch through the generic `insert_text_input` left the arm holding `None`. `this_command` rotated correctly and `buffer.after-edit` saw no record --- auto-pairing stayed broken while the command side looked right, which is the failure mode that hides longest. The single-scalar branch now goes through `insert_char_over_region(ch)`, which handles the no-region case itself by delegating to `insert_char`. `insert_text_input` is documented as the MULTI-scalar path only, with the trap named at the definition rather than left for the next caller to rediscover. **The witness consumes the record rather than inspecting `this_command`**, per the review. `single_scalar_text_input_produces_a_consumable_typed_edit_record` takes it through the same `pmacs.pair._last_record` seam `pair.lua` uses, and `single_scalar_text_input_auto_pairs_like_a_keypress` states the same fact in the terms a user would notice: typing `(` must produce `()`. **Mutation M-1a-1 reverts the fix and both rows fail**; the four others stay green, so they are discriminating rather than duplicated. **A ceiling tripwire proved less than it claimed.** The discovery acceptance looped `6..=23` and then rejected `PROTOCOL_VERSION + 1`, so a supported set that ENDED at 23 would have passed while `PROTOCOL_VERSION` was 24 --- the accepted half said nothing about the version the constant names. It runs to `PROTOCOL_VERSION` now. **The public protocol history stopped at v23 while both constants already included 24.** The rustdoc above `PROTOCOL_VERSION` and `SUPPORTED_PROTOCOL_VERSIONS` now carries the v24 bump, and states the thing that makes it unlike its predecessors: **it is the first FRONTEND->INSTANCE extension needing a gate in BOTH directions**, because the producer withholding is not enough when a peer compiled from this same crate can encode the variant whatever it negotiated. Three typed-edit doc sites said the arm is set by "the dispatch fallback only" or named two producers; there are three now, and the single-scalar `TextInput` path is one. `typed_edit_arm`'s own doc gains the warning that arming is only half. Also 1a's suite: A6 (one commit, one edit, one undo unit), A7 (a prompt accumulates scalars in order), and A9's boundary row --- a payload exactly at the cap lands intact, the complement of the rejection that is enforced where a test can reach it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- pmacs-protocol/src/message.rs | 26 ++++ src/editor.rs | 31 ++-- src/editor_core.rs | 34 ++++- tests/discovery_stage2_acceptance.rs | 9 +- tests/gui_stage1a_acceptance.rs | 208 +++++++++++++++++++++++++++ 5 files changed, 289 insertions(+), 19 deletions(-) create mode 100644 tests/gui_stage1a_acceptance.rs diff --git a/pmacs-protocol/src/message.rs b/pmacs-protocol/src/message.rs index 712eff5..517fe34 100644 --- a/pmacs-protocol/src/message.rs +++ b/pmacs-protocol/src/message.rs @@ -1895,6 +1895,24 @@ pub enum ResourceBody { /// encoding makes an in-place widening a wire break rather than an /// evolution, and gating the widened form would have left those peers /// with no minibuffer message at all. +/// +/// GUI arc Stage 1a: bumped 23 → 24 for +/// [`FrontendEvent::TextInput`] — committed text from a keypress or an +/// IME composition, carried as one event so that a multi-scalar +/// grapheme is one edit, one undo unit and one eligible CRDT op instead +/// of being truncated to its first scalar. Appended after +/// `PanelPointer`, the final v23 `FrontendEvent` variant, so no +/// existing discriminant moves. +/// +/// **This is the first FRONTEND→INSTANCE extension to need a gate in +/// both directions**, and the reason is that the direction of travel is +/// reversed: for an instance→frontend variant the daemon simply +/// withholds, but here the *frontend* must withhold below +/// [`TEXT_INPUT_MIN_VERSION`] **and** the daemon must refuse what a peer +/// below it nonetheless sends. A client built from this crate can encode +/// the variant whatever it negotiated, so the producer gate alone would +/// leave a v6–v23 session able to drive an edit through a variant its +/// own session never declared. pub const PROTOCOL_VERSION: u32 = 24; /// Protocol version placed in the daemon's server-first [`Hello`]. @@ -2076,6 +2094,14 @@ pub fn negotiated_session_version(frontend_offer: u32) -> u32 { /// keeps receiving the frozen [`InstanceMessage::MinibufferPrompt`], a /// `>= 23` peer receives only the rows form, and no peer ever receives /// both. [`ADVERTISED_PROTOCOL_VERSION`] does not move. +/// +/// GUI arc Stage 1a: extended to `[6, ..., 24]` for +/// [`FrontendEvent::TextInput`]. Additive, and gated in **both** +/// directions rather than only daemon-side — see [`PROTOCOL_VERSION`] +/// for why an inbound frontend→instance variant needs the receiving +/// check too. [`ADVERTISED_PROTOCOL_VERSION`] does not move: a v23 +/// frontend negotiates v23, never sends the variant, and keeps today's +/// first-scalar behaviour. pub const SUPPORTED_PROTOCOL_VERSIONS: &[u32] = &[ 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, ]; diff --git a/src/editor.rs b/src/editor.rs index 52bc0f7..b08c26a 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1950,16 +1950,29 @@ impl EditorState { let pre_revision = self.active_buffer_revision(); { let mut core = self.core.borrow_mut(); - match single { - Some(ch) => { - core.rotate_command(frontend_id, "buffer.self-insert"); - core.typed_edit_arm(frontend_id, ch); + if let Some(ch) = single { + core.rotate_command(frontend_id, "buffer.self-insert"); + core.typed_edit_arm(frontend_id, ch); + // **Must go through `insert_char_over_region`, not + // the generic byte insert.** Arming provenance is + // only half of it: `typed_edit_complete` is called + // from `insert_char` / `insert_char_over_region` and + // nowhere else, so a generic insert leaves the arm + // holding `None` and `buffer.after-edit` sees no + // record — `this_command` rotates correctly and + // auto-pairing still fails, which is a failure mode + // that looks like success from the command side. + // It handles the no-region case itself, by + // delegating to `insert_char`. + core.insert_char_over_region(ch); + } else { + core.break_command_chain(frontend_id); + // A6 — multi-scalar is ONE generic edit, and + // deliberately creates no typed provenance: it is not a + // keystroke. + if let Err(e) = core.insert_text_input(text) { + eprintln!("pmacs: text input failed: {e}"); } - None => core.break_command_chain(frontend_id), - } - // A6 — ONE edit, whichever branch armed it. - if let Err(e) = core.insert_text_input(text) { - eprintln!("pmacs: text input failed: {e}"); } } diff --git a/src/editor_core.rs b/src/editor_core.rs index 51f71b7..01e2fe6 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -478,8 +478,9 @@ pub struct CommandBoundary { /// the exact facts for the one consumer contract that needs them (the /// pairing hook): the decoded codepoint, the requested and effective /// ranges, and the post-edit cursor, plus a `clean` verdict (effective -/// triple equals the request). It is ephemeral — armed by the two -/// self-insert producers (dispatch fallback, optimistic CRDT arm) for +/// triple equals the request). It is ephemeral — armed by the three +/// self-insert producers (dispatch fallback, optimistic CRDT arm, and +/// the single-scalar `TextInput` path of GUI arc Stage 1a) for /// exactly one `buffer.after-edit` fan-out, consumable once via /// `pmacs.editor.take_typed_edit()`, and cleared when the fan-out /// returns. Paste, programmatic mutation, manual hook runs, and a @@ -687,8 +688,10 @@ pub struct EditorCore { /// query-replace twin of `search`; drives the fifth dispatcher /// shadow. query_replace: Option, - /// In-flight typed-edit arm (auto-pairing Q#AP9): set by the - /// dispatch fallback just before it invokes `buffer.self-insert`, + /// In-flight typed-edit arm (auto-pairing Q#AP9): set by a + /// self-insert producer just before the edit — the dispatch + /// fallback invoking `buffer.self-insert`, or 1a's single-scalar + /// `TextInput` path — /// completed by the insert primitives, taken back by the /// dispatcher via [`Self::typed_edit_finish`] in the same /// dispatch. Never survives a dispatch cycle. @@ -4808,9 +4811,19 @@ impl EditorCore { /// Declare that `fid`'s dispatch is about to invoke /// `buffer.self-insert` for `codepoint`: the next insert primitive /// whose character matches completes the [`TypedEditRecord`]. - /// Called by the dispatch fallback only — programmatic + /// Called by the self-insert producers: the dispatch fallback, and + /// the **single-scalar** `TextInput` path (GUI arc 1a), which must + /// be indistinguishable from a keypress downstream. Programmatic /// `pmacs.command.invoke("buffer.self-insert")` deliberately never - /// arms, so a hook run after it observes no record. + /// arms, so a hook run after it observes no record; nor does a + /// MULTI-scalar `TextInput`, which is not a keystroke. + /// + /// **Arming is only half.** Completion happens in the insert + /// primitives ([`Self::insert_char`] / + /// [`Self::insert_char_over_region`]) and nowhere else, so a caller + /// that arms and then performs a generic byte insert leaves the arm + /// holding `None` — `this_command` looks right and auto-pairing + /// silently stops working. pub fn typed_edit_arm(&mut self, fid: FrontendId, codepoint: char) { self.typed_edit_pending = Some(TypedEditPending { fid, @@ -5009,7 +5022,14 @@ impl EditorCore { self.insert_bytes_over_region(data) } - /// GUI arc Stage 1a / A6 — insert committed text as **one edit**. + /// GUI arc Stage 1a / A6 — insert **multi-scalar** committed text as + /// one edit. + /// + /// **Single-scalar text does NOT come here**; it goes through + /// [`Self::insert_char_over_region`], which is the only path (with + /// [`Self::insert_char`]) that completes a [`TypedEditRecord`]. + /// Routing a single scalar here would arm provenance and never + /// complete it — see `EditorState::dispatch_text_input`. /// /// Shares [`Self::insert_bytes_over_region`] with paste, which is /// what makes it a single `EditOp` and therefore a single undo diff --git a/tests/discovery_stage2_acceptance.rs b/tests/discovery_stage2_acceptance.rs index 8afa29a..db5f8cc 100644 --- a/tests/discovery_stage2_acceptance.rs +++ b/tests/discovery_stage2_acceptance.rs @@ -77,9 +77,12 @@ fn the_wire_is_v24_and_the_advertised_baseline_is_unmoved() { "moving this is the incompatible act the counter-offer mechanism exists to avoid" ); // The whole v12..=22 population this lane is compatible with is - // still supported, and the set ends at the new wire — a widened set - // is a failure rather than a silent pass. - for version in 6..=23 { + // still supported, AND the current wire is in the set. The loop + // must run to `PROTOCOL_VERSION`, not to a literal: stopping at 23 + // let a supported range that ended at 23 pass this test while + // `PROTOCOL_VERSION` was already 24 — the accepted half proved + // nothing about the version the constant names. + for version in 6..=PROTOCOL_VERSION { assert!( is_supported_protocol_version(version), "v{version} must still be supported" diff --git a/tests/gui_stage1a_acceptance.rs b/tests/gui_stage1a_acceptance.rs new file mode 100644 index 0000000..369e653 --- /dev/null +++ b/tests/gui_stage1a_acceptance.rs @@ -0,0 +1,208 @@ +//! GUI arc Stage 1a acceptance — `TextInput` at protocol v24. +//! +//! Framing: `docs/gui-stage1-input-framing.md` §5 (Q#S1-9 precedence) +//! and §6's A1–A9. +//! +//! **These rows drive the real dispatch, not the classifier.** 1a's +//! first review found A7 and A8 unreachable from the production +//! producer while `text_input_payload` was perfectly correct: the +//! intercept branch returned before classification, and a modal prompt +//! or a focused terminal is exactly what makes intercept true. A test +//! that exercises the pure function would have stayed green through +//! that, so the rows here go through `dispatch_text_input` and, for the +//! producer-side ones, through the real classifier at the real call +//! site. + +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +fn editor_with(body: &str) -> EditorState { + let s = EditorState::new_with_roots(&crate::iso::roots()); + if !body.is_empty() { + exec(&s, &format!("pmacs.window.buffer():insert(0, {body:?})")); + } + exec(&s, "pmacs.editor.goto_byte(0)"); + s +} + +fn buffer_text(s: &EditorState) -> String { + let b: mlua::String = eval( + s, + "local b = pmacs.window.buffer(); return b:slice(0, b:len())", + ); + String::from_utf8_lossy(&b.as_bytes()).into_owned() +} + +const FID: FrontendId = FrontendId::LOCAL; + +// --------------------------------------------------------------------- +// A6 — one commit is one edit, one undo unit, one hook fan-out +// --------------------------------------------------------------------- + +/// A6 — a multi-scalar commit is **one** edit and **one** undo unit. +/// +/// This is the failure 1a exists to fix: as separate keypresses the same +/// grapheme is two edits, so one undo leaves half of it behind. +#[test] +fn a6_a_multi_scalar_commit_is_one_edit_and_one_undo_unit() { + let mut s = editor_with(""); + exec( + &s, + "_G.edits = 0 + pmacs.hook.add('buffer.after-edit', function() _G.edits = _G.edits + 1 end)", + ); + + // A composed grapheme: base plus combining acute. Two scalars, one + // thing the user meant to type. + s.dispatch_text_input(FID, "e\u{301}"); + assert_eq!(buffer_text(&s), "e\u{301}"); + + let edits: i64 = eval(&s, "return _G.edits"); + assert_eq!(edits, 1, "one commit must fire ONE buffer.after-edit"); + + exec(&s, "pmacs.command.invoke('buffer.undo')"); + assert_eq!( + buffer_text(&s), + "", + "one undo must remove the whole commit, not its last scalar" + ); +} + +// --------------------------------------------------------------------- +// §5 provenance — the single/multi split +// --------------------------------------------------------------------- + +/// §5 — a SINGLE-scalar commit is indistinguishable from a keypress, so +/// it must produce a real, consumable `TypedEditRecord`. +/// +/// **Asserting `this_command` is not enough**, and that is the whole +/// point of this row: review round 2 found the code rotating the command +/// correctly while never completing the record, because arming and +/// completing are different steps and only the insert primitives +/// complete. `this_command` looked right and auto-pairing was broken. +/// So this consumes the record through the same seam `pair.lua` uses. +#[test] +fn single_scalar_text_input_produces_a_consumable_typed_edit_record() { + let mut s = editor_with(""); + exec(&s, "pmacs.pair._capture_records = true"); + + s.dispatch_text_input(FID, "("); + + let (cp, ch, clean, il): (i64, String, bool, i64) = eval( + &s, + "local r = pmacs.pair._last_record + return r.codepoint, r.char, r.clean, r.inserted_len", + ); + assert_eq!(cp, 40, "exact codepoint for '('"); + assert_eq!(ch, "("); + assert!(clean, "no intercept ran, so the effective triple is clean"); + assert_eq!(il, 1); + + let this_command: String = eval(&s, "return pmacs.editor.this_command() or ''"); + assert_eq!( + this_command, "buffer.self-insert", + "and the command rotates, which is the half that already worked" + ); +} + +/// §5 — a MULTI-scalar commit is **not** a keystroke: it creates no +/// typed provenance and breaks the command chain. +#[test] +fn multi_scalar_text_input_creates_no_typed_provenance() { + let mut s = editor_with(""); + exec(&s, "pmacs.pair._capture_records = true"); + + s.dispatch_text_input(FID, "e\u{301}"); + + let no_record: bool = eval(&s, "return pmacs.pair._last_record == nil"); + assert!( + no_record, + "a multi-scalar commit must not forge a typed-edit record" + ); + let this_command: String = eval(&s, "return pmacs.editor.this_command() or ''"); + assert_ne!( + this_command, "buffer.self-insert", + "a multi-scalar commit is not a self-insert" + ); +} + +/// The pairing consumer, end to end: a single-scalar `(` must auto-pair +/// exactly as a typed `(` does. This is the behaviour the missing record +/// silently disabled, stated in the terms a user would notice. +#[test] +fn single_scalar_text_input_auto_pairs_like_a_keypress() { + let mut s = editor_with(""); + s.dispatch_text_input(FID, "("); + assert_eq!( + buffer_text(&s), + "()", + "auto-pairing consumes the typed-edit record; without one the \ + closer is never inserted" + ); +} + +// --------------------------------------------------------------------- +// A7 — prompts consume scalars IN ORDER +// --------------------------------------------------------------------- + +/// A7 — a prompt accumulates the scalars in order. +/// +/// Order is the contract: a reversed or set-wise delivery would still +/// "consume" the text and would produce a different query. +#[test] +fn a7_a_prompt_consumes_scalars_in_order() { + let mut s = editor_with(""); + exec( + &s, + "pmacs.minibuffer.read({ prompt = 'x: ', on_accept = function() end })", + ); + assert!(s.core.borrow().minibuffer.is_active(), "prompt is up"); + + s.dispatch_text_input(FID, "abc"); + + let content: String = eval(&s, "return pmacs.minibuffer.contents() or ''"); + assert_eq!(content, "abc", "in order, not reversed or reordered"); + assert_eq!( + buffer_text(&s), + "", + "and the buffer underneath is untouched" + ); +} + +// --------------------------------------------------------------------- +// A9 — the cap rejects rather than truncates +// --------------------------------------------------------------------- + +/// A9 — a payload at the cap is accepted whole. The complement of the +/// rejection row: a cap that refused its own boundary value would be +/// off by one in the direction nobody notices until a long IME commit +/// vanishes. +/// +/// **The rejection half is witnessed where it is enforced** — at the +/// daemon boundary (`daemon.rs`, gated before any insert) and at the +/// producer (`AttachClient::send_text_input`, whose unit test lives +/// beside it in `pmacs-gpu`). Neither is reachable from an +/// `EditorState`, so asserting the constant here instead would be a row +/// that cannot fail for the right reason. +#[test] +fn a9_a_payload_at_the_cap_is_inserted_whole() { + let mut s = editor_with(""); + let at_cap = "a".repeat(pmacs_protocol::TEXT_INPUT_MAX_BYTES); + s.dispatch_text_input(FID, &at_cap); + assert_eq!( + buffer_text(&s).len(), + pmacs_protocol::TEXT_INPUT_MAX_BYTES, + "the boundary value is legal and must land intact" + ); +} + +#[path = "common/iso.rs"] +mod iso;