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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
266bc6e4f3
commit
97da79f136
|
|
@ -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,
|
||||
];
|
||||
|
|
|
|||
|
|
@ -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}");
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<QueryReplaceSession>,
|
||||
/// 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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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<T: mlua::FromLuaMulti>(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;
|
||||
Loading…
Reference in New Issue