From 50fd9a08e4c723976cf4d7f8e97c6955cf004f1a Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 22 Jul 2026 14:49:23 -0400 Subject: [PATCH] fix(vterm): address stage 3 review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all addressed. One was a real defect; one prediction did not reproduce and is documented as such rather than papered over. Hover no longer claims durable terminal control (finding 2, the real one). apply_terminal_gesture claimed the controller before dispatching, including for Move, which does nothing. A semantic frontend reports motion at pixel rate, so sweeping the mouse across a passive split's terminal took durable control, and the next layout sync resized the shared PTY to that background view's geometry — precisely the theft the controller rule exists to prevent. Bare motion no longer claims; every deliberate gesture still does. scripts/bite HEAD src/editor.rs on the new test is a clean behavioral bite. The terminal-mode presence-sweep skip is removed (finding 1), but the predicted failure did NOT reproduce. The review reasoned that skipping the sweep freezes last_broadcast at the abandoned document position. It does not: the buffer-follow clears the terminal declaration when it ships the snapshot, so terminal_active is false on the tick a window first shows a terminal, and the declaration cannot arrive until a later tick — the frontend learns the buffer id from that very snapshot. One truthful sweep always lands first. The real-daemon two-frontend test written to catch the freeze passes against the pre-fix tree; the bite is vacuous and the test is labelled a regression guard, not fix evidence. The skip goes anyway: it was load-bearing on tick ordering and bought nothing, and removing it makes "presence follows the frontend" structural. Terminal motion is deduplicated by cell (finding 3). Sub-cell motion resolved to the same coordinate and still crossed the wire, where every event is a daemon-side gesture. Press and release re-arm the memo so the first drag after a press still reports. Its unit test cannot bite — the seam did not exist pre-fix — and says so. Declarations record only once sent (finding 4). terminal_declaration_if_changed is now a pure query; note_terminal_declaration_sent records. A failed write is retried instead of suppressed as already-declared. The existing a35 test caught the contract change and now pins both halves. Unchanged frames skip revalidation (finding 5). The complete-payload comparison runs before validate; only validated frames are ever stored, so a frame equal to the baseline has already passed. The chrome tail is factored into terminal_chrome so both exits emit it identically. Gates: fmt; strict workspace clippy; 1,757 default + 1,933 CRDT library tests; Stage 1 9/10, Stage 2 4/4, Stage 3 5/7, statusline 7/8 (default/CRDT); M4 120; required GPU 128; workspace sweep 2,921 across 83 suites; diff check clean. --- docs/active-work.md | 18 +- docs/vterm-framing.md | 48 ++++++ pmacs-gpu/src/main.rs | 135 +++++++++++++-- src/daemon.rs | 40 +++-- src/editor.rs | 20 ++- src/semantic_render.rs | 45 ++++- tests/vterm_stage3_acceptance.rs | 272 +++++++++++++++++++++++++++++++ 7 files changed, 537 insertions(+), 41 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 9697065..eed0dcf 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -84,11 +84,19 @@ If it does not, stop and repair the remote/fetch configuration. - `cargo test --workspace -- --skip basedpyright`: 2,919 passed across 83 suites (19 ignored), one invocation; - `git diff --check`. -- Open caveat: the required-GPU suite failed ONCE mid-session and did not - reproduce across eight subsequent runs or the full sweep. The failing - test's identity was not captured. Re-run `PMACS_REQUIRE_GPU=1 cargo test - -p pmacs-gpu` a few times on review; if it recurs, capture the name. -- Next: user review rounds on the PR. +- Review round 1 addressed (framing §0.10): hover no longer claims durable + terminal control (real defect, bite-verified); terminal motion dedupes by + cell; declarations record only once sent; unchanged frames skip + revalidation; the terminal-mode presence-sweep skip is removed. The + review's predicted presence FREEZE did not reproduce — the buffer-follow + clears the declaration before `render_frame`, so a truthful sweep always + precedes terminal mode; that test is a labelled regression guard, not + fix evidence. +- Post-review gates: required GPU 128; workspace sweep 2,921 across 83 + suites; Stage 3 acceptance 5 default / 7 CRDT; everything else as above. +- Closed caveat: the once-seen required-GPU failure did not reproduce in + eight author runs plus four reviewer runs. Treated as environmental. +- Next: further user review rounds on the PR. Recovery worktree: diff --git a/docs/vterm-framing.md b/docs/vterm-framing.md index fcbd471..ee0b59d 100644 --- a/docs/vterm-framing.md +++ b/docs/vterm-framing.md @@ -381,6 +381,54 @@ required GPU 127; one-invocation workspace sweep 2,919 passed across 83 suites required-GPU suite occurred once mid-session and did not reproduce across eight subsequent runs including the full sweep; its identity was not captured. +### 0.10 Stage 3 review round 1 + +PR #135's first review found no correctness blocker, confirmed the required-GPU +suite clean across four runs (twelve total with the author's), and raised two +design questions plus three minor notes. All five are addressed. + +- **Presence while in terminal mode (finding 1) — fix kept, prediction not + reproduced.** The review predicted that skipping the presence sweep freezes + `last_broadcast` at the abandoned document position, leaving peers painting a + stale caret. It does not: the buffer-follow clears the terminal declaration + when it ships the snapshot, so `terminal_active` is false on the tick a window + first shows a terminal, and the declaration cannot arrive until a later tick + (the frontend learns the buffer id FROM that snapshot). One truthful sweep + always lands first. A real-daemon two-frontend test written to catch the + freeze passes against the pre-fix tree — the bite is VACUOUS, and it is + labelled a regression guard rather than fix evidence. The skip is removed + anyway: it was load-bearing on tick ordering and bought nothing, and its + removal makes "presence follows the frontend" structural. +- **Hover claimed durable control (finding 2) — real, fixed, bite-verified.** + `apply_terminal_gesture` claimed the controller before dispatching, including + for `Move`, which does nothing. A semantic frontend reports motion at pixel + rate, so sweeping the mouse across a PASSIVE split's terminal took durable + control and the next layout sync resized the shared PTY to that background + view's geometry — exactly the theft the controller rule exists to prevent. + Bare motion no longer claims; every deliberate gesture still does. + `scripts/bite HEAD src/editor.rs` on + `hover_does_not_steal_terminal_control_from_the_active_frontend` is a clean + behavioral bite (assertion failure, not a compile error). +- **Terminal motion is deduplicated by cell (finding 3).** Sub-cell motion + resolved to the same coordinate and still crossed the wire, where each event + is a daemon-side gesture. `State::terminal_motion_is_new` now gates it, and + press/release re-arm the memo so the first drag after a press still reports. + Its unit test cannot bite — the seam did not exist pre-fix — and says so. +- **Declarations record only once sent (finding 4).** + `terminal_declaration_if_changed` is now a pure query and + `note_terminal_declaration_sent` records, so a failed write is retried instead + of suppressed as already-declared. The existing `a35` test caught the contract + change and now pins both halves. +- **Unchanged frames are no longer re-validated (finding 5).** The complete + payload comparison runs before `validate`; only validated frames are ever + stored, so a frame equal to the baseline has already passed. The chrome tail + is factored into `terminal_chrome` so both exits emit it identically. + +Post-review gates: `cargo fmt --check`; strict workspace Clippy; 1,757 default + +1,933 CRDT library tests; Stage 1 acceptance 9/10, Stage 2 4/4, Stage 3 5/7, +statusline 7/8 (default/CRDT); M4 120; required GPU 128; workspace sweep 2,921 +across 83 suites (19 ignored); `git diff --check` clean. + ## 1. Problem and ownership boundary Pmacs can supervise a PTY and can parse enough ANSI to turn command output into diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index e18ce27..b6ef17d 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -683,9 +683,11 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { 0, ); } - if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() { + if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() + && client.send_terminal_resize(buffer_id, size).is_ok() + { facts.declarations += 1; - let _ = client.send_terminal_resize(buffer_id, size); + state.note_terminal_declaration_sent(buffer_id, size); } } if state.terminal.is_some() { @@ -706,11 +708,13 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { if !sent_resize && facts.frames >= 2 { sent_resize = true; state.resize(700, 500); - if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() { + if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() + && client.send_terminal_resize(buffer_id, size).is_ok() + { facts.declarations += 1; facts.resized_cols = size.cols; facts.resized_rows = size.rows; - let _ = client.send_terminal_resize(buffer_id, size); + state.note_terminal_declaration_sent(buffer_id, size); } } if facts.resized_cols > 0 && facts.last_frame_cols == facts.resized_cols { @@ -1217,6 +1221,12 @@ struct State { /// Whether an invalid terminal frame has already been reported. /// Bounds the log while a bad producer keeps sending. terminal_frame_error_latched: bool, + /// Last terminal cell a motion or drag was reported at. Pixel-rate + /// motion inside ONE cell is not new information for the daemon — + /// the document drag path dedupes by hit byte for the same reason. + /// Cleared on press, release, and every exit from terminal mode, so + /// a gesture that returns to the same cell still reports. + last_terminal_pointer_cell: Option, /// One shaped buffer per planned text run. Rebuilt only when the /// plan changes, never per frame. terminal_text_buffers: Vec, @@ -1496,8 +1506,9 @@ impl App { let Some((buffer_id, size)) = state.terminal_declaration_if_changed() else { return; }; - if let Err(e) = client.send_terminal_resize(buffer_id, size) { - eprintln!("pmacs-gpu: send_terminal_resize failed: {e}"); + match client.send_terminal_resize(buffer_id, size) { + Ok(()) => state.note_terminal_declaration_sent(buffer_id, size), + Err(e) => eprintln!("pmacs-gpu: send_terminal_resize failed: {e}"), } } @@ -1818,13 +1829,22 @@ impl ApplicationHandler for App { if let Some((buffer_id, coord)) = self.terminal_pointer_hit(position.x, position.y) { - let mods = translate_mods(self.modifiers); - let kind = if dragging { - ProtocolMouseKind::Drag(ProtocolMouseButton::Left) - } else { - ProtocolMouseKind::Move - }; - self.send_terminal_pointer(buffer_id, coord, kind, mods); + // Sub-cell motion resolves to the same cell and + // carries nothing new. Report only on a cell + // change, matching the document drag path's + // hit-byte dedupe — otherwise pixel-rate motion + // becomes pixel-rate wire traffic, and every one + // of those is a daemon-side gesture. + let state = self.state.as_mut().expect("checked above"); + if state.terminal_motion_is_new(coord) { + let mods = translate_mods(self.modifiers); + let kind = if dragging { + ProtocolMouseKind::Drag(ProtocolMouseButton::Left) + } else { + ProtocolMouseKind::Move + }; + self.send_terminal_pointer(buffer_id, coord, kind, mods); + } } return; } @@ -1904,6 +1924,11 @@ impl ApplicationHandler for App { ProtocolMouseKind::Up(ProtocolMouseButton::Left) } }; + // A press or release always reports, and it re-arms + // the motion dedupe: the first drag after a press + // must reach the daemon even at the cell the press + // landed on. + state.last_terminal_pointer_cell = None; if let Some((buffer_id, coord)) = self.terminal_pointer_hit(x, y) { self.send_terminal_pointer(buffer_id, coord, kind, mods); } @@ -2813,6 +2838,7 @@ impl State { terminal: None, last_terminal_size_sent: None, terminal_frame_error_latched: false, + last_terminal_pointer_cell: None, terminal_text_buffers: Vec::new(), terminal_text_renderer, }; @@ -3884,6 +3910,7 @@ impl State { self.terminal_text_buffers.clear(); self.terminal_frame_error_latched = false; self.last_terminal_size_sent = None; + self.last_terminal_pointer_cell = None; } /// Drop shaping and geometry caches without leaving terminal mode. @@ -4159,10 +4186,32 @@ impl State { if self.last_terminal_size_sent == Some((buffer_id, size)) { return None; } - self.last_terminal_size_sent = Some((buffer_id, size)); Some((buffer_id, size)) } + /// Whether terminal motion at `coord` is new information. + /// + /// Records the cell as reported either way, so a caller that skips + /// the send still advances the memo. Press and release reset it + /// (`last_terminal_pointer_cell = None`), which is what lets the + /// first drag after a press reach the daemon even at the cell the + /// press landed on. + fn terminal_motion_is_new(&mut self, coord: CellCoord) -> bool { + let changed = self.last_terminal_pointer_cell != Some(coord); + self.last_terminal_pointer_cell = Some(coord); + changed + } + + /// Record a declaration the caller actually put on the wire. + /// + /// Separate from [`Self::terminal_declaration_if_changed`] so a + /// FAILED send does not leave a size believed-declared: the daemon + /// would still hold the old geometry while this frontend suppressed + /// every retry as unchanged. + fn note_terminal_declaration_sent(&mut self, buffer_id: BufferId, size: CellSize) { + self.last_terminal_size_sent = Some((buffer_id, size)); + } + /// True while the completion popup is open **for the buffer this /// window currently shows** — the predicate the key gates (Esc, /// RET/TAB) and the render path share, so a stale mirror can @@ -13117,9 +13166,20 @@ mod tests { .expect("a 400x300 window admits a cell grid"); assert_eq!(first.0, buffer_id); assert!(first.1.rows >= 1 && first.1.cols >= 1); + + // Review round 1, finding 4: the query does not record. Until + // the caller confirms the send, the declaration stays PENDING, + // so a failed write is retried rather than suppressed as + // already-declared. + assert_eq!( + state.terminal_declaration_if_changed(), + Some(first), + "an unsent declaration must still be offered" + ); + state.note_terminal_declaration_sent(first.0, first.1); assert!( state.terminal_declaration_if_changed().is_none(), - "an unchanged size must be silent" + "an unchanged size must be silent once it has been sent" ); // A real geometry change re-declares exactly once. @@ -13128,6 +13188,7 @@ mod tests { .terminal_declaration_if_changed() .expect("a wider window is a new size"); assert!(widened.1.cols > first.1.cols); + state.note_terminal_declaration_sent(widened.0, widened.1); assert!(state.terminal_declaration_if_changed().is_none()); // A buffer switch forces a fresh declaration even at the same size. @@ -13372,6 +13433,50 @@ mod tests { ); } + /// Review round 1, finding 3: terminal motion reports only on a cell + /// change. + /// + /// A unit test on the memo rather than on the wire: the send site + /// lives in `window_event`, which needs a real winit event and a + /// live attach client. This cannot bite against the pre-fix tree — + /// the seam it calls did not exist there — so it is a contract pin, + /// not fix evidence. + #[test] + fn terminal_motion_reports_once_per_cell_and_rearms_on_press() { + let Some(mut state) = headless_or_skip(400, 300, "doc") else { + return; + }; + let buffer_id = BufferId::next(); + state.current_buffer_id = Some(buffer_id); + state.apply_terminal_frame(plain_terminal_frame(buffer_id, "abcd", 8)); + + let cell = CellCoord::new(0, 2); + assert!(state.terminal_motion_is_new(cell), "first sight of a cell"); + assert!( + !state.terminal_motion_is_new(cell), + "sub-cell motion inside one cell is not new information" + ); + assert!( + state.terminal_motion_is_new(CellCoord::new(0, 3)), + "crossing into another cell reports" + ); + + // A press/release re-arms the memo, so the first drag after a + // press reaches the daemon even at the press cell. + let press_cell = CellCoord::new(1, 1); + assert!(state.terminal_motion_is_new(press_cell)); + state.last_terminal_pointer_cell = None; + assert!( + state.terminal_motion_is_new(press_cell), + "a press re-arms the memo at its own cell" + ); + + // Leaving terminal mode drops it too: the next terminal's cells + // are a different grid entirely. + state.exit_terminal_mode(); + assert!(state.last_terminal_pointer_cell.is_none()); + } + /// Acceptance 36: the terminal statusline metadata reaches the band /// as text, never as a host-title or control effect. #[test] diff --git a/src/daemon.rs b/src/daemon.rs index 8ae640f..46ed1d1 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1087,10 +1087,9 @@ fn dispatcher_loop( }; // Vterm Stage 3 — a semantic frontend showing a terminal has - // no document cursor: the identity buffer is empty, so both - // presence and `CursorByte` would describe byte 0 of a - // buffer with no text, painting a phantom peer caret over - // the cell grid. + // no document cursor: the identity buffer is empty, so a + // `CursorByte` would describe byte 0 of a buffer with no + // text and scroll the frontend's document view against it. let terminal_mode = semantic_states .get(fid) .is_some_and(crate::semantic_render::SemanticRenderState::in_terminal_mode); @@ -1098,12 +1097,33 @@ fn dispatcher_loop( // T M10.6 per-frontend presence sweep. The snapshot is // computed from this frontend's view; the sweep then // produces broadcasts to OTHER multi-frontend recipients. - let broadcasts = if terminal_mode { - Vec::new() - } else { - let snapshot = build_presence_snapshot(editor, *fid); - session_registry.sweep(&[(*fid, snapshot)]) - }; + // + // Terminal mode does NOT skip this (PR #135 review finding 1). + // `sweep` is diff-keyed on `last_broadcast`, so a skip can + // only ever FREEZE a frontend's presence — it can never + // retract it. Sweeping truthfully moves the presence into + // the terminal identity buffer, which is what takes this + // frontend's caret off every peer showing the document. + // + // Honest note on why the skip was not a live bug: the + // buffer-follow above clears the terminal declaration when + // it ships the snapshot, so `render_frame` reports + // `terminal_active == false` on the tick a window first + // shows a terminal, and the declaration cannot arrive until + // a later tick (the frontend learns the buffer id FROM that + // snapshot). Every entry into terminal mode is therefore + // already preceded by one truthful sweep. The skip was + // load-bearing on that ordering and bought nothing; removing + // it makes "presence follows the frontend" structural + // instead of a property of tick sequencing. + // + // The framing's "suppress presence for the terminal identity + // buffer" is a RENDER rule — no peer overlay is painted + // inside a terminal — which the GPU honors by not preparing + // the decoration batch in terminal mode. It was never a + // reason to stop telling peers where this frontend went. + let snapshot = build_presence_snapshot(editor, *fid); + let broadcasts = session_registry.sweep(&[(*fid, snapshot)]); // T M11.6 — DispatchIdle signal. `crdt_replica` frontends // gate their optimistic-apply path on this; we ship it diff --git a/src/editor.rs b/src/editor.rs index 8c844f9..bcba37c 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1951,6 +1951,18 @@ impl EditorState { (status.at_bottom, modes, screen_size) }; + // Hover is not an act of taking over a terminal (PR #135 review + // finding 2). Every other gesture is deliberate — a press, a + // release, a drag, a wheel tick, a right-click — but bare motion + // happens whenever a pointer crosses a window. A semantic + // frontend reports motion at pixel rate, so claiming on `Move` + // let merely sweeping the mouse across a PASSIVE split's + // terminal take durable control, and the next layout sync then + // resized the shared PTY to that background view's geometry. + // That is precisely the theft the controller rule exists to + // prevent. + let claims_control = !matches!(kind, TerminalMouseKind::Move); + if !shift && at_bottom && modes.mouse_sgr @@ -1958,12 +1970,16 @@ impl EditorState { && coord.col < screen_size.cols && let Some(bytes) = crate::terminal::input::encode_mouse(kind, coord, modifiers, modes) { - self.claim_terminal_controller(key); + if claims_control { + self.claim_terminal_controller(key); + } self.send_terminal_bytes(key.buffer_id, &bytes); return; } - self.claim_terminal_controller(key); + if claims_control { + self.claim_terminal_controller(key); + } let mut manager = self.terminal_manager.borrow_mut(); match kind { TerminalMouseKind::ScrollUp => { diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 2ba2f56..85b9753 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -853,17 +853,26 @@ impl SemanticRenderState { let mut out = Vec::new(); let frame = snapshot.into_terminal_frame(); + // Complete-payload comparison FIRST, and not on + // `screen_generation`: a scroll, a selection change, or a + // process exit must reach the frontend even though the screen + // itself is byte-identical. + // + // Comparing before validating is also what keeps the steady + // state cheap. Only validated frames are ever stored, so a frame + // equal to the baseline has already passed — re-running the + // per-cell width and topology checks every tick would recompute + // a verdict we hold. + if self.last_terminal_frame.as_ref() == Some(&frame) { + self.terminal_error_latched = false; + out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation)); + return Some(out); + } match frame.validate() { Ok(()) => { self.terminal_error_latched = false; - // Complete-payload comparison, not `screen_generation`: - // a scroll, a selection change, or a process exit must - // reach the frontend even though the screen itself is - // byte-identical. - if self.last_terminal_frame.as_ref() != Some(&frame) { - self.last_terminal_frame = Some(frame.clone()); - out.push(InstanceMessage::TerminalFrame(frame)); - } + self.last_terminal_frame = Some(frame.clone()); + out.push(InstanceMessage::TerminalFrame(frame)); } Err(error) => { // Never emit a malformed or truncated frame. The peer @@ -880,15 +889,33 @@ impl SemanticRenderState { } } + out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation)); + Some(out) + } + + /// The buffer-independent chrome a terminal-mode frontend still + /// needs, in the order the document path emits it. + /// + /// Shared by both terminal-pass exits so an unchanged frame and a + /// changed one ship exactly the same chrome — the suppression is + /// about the FRAME, never about the status band going quiet. + fn terminal_chrome( + &mut self, + state: &EditorState, + buffer_id: BufferId, + statusline_evaluation: Option, + ) -> Vec { + let mut out = Vec::new(); out.extend(self.status_facts_msg(state, buffer_id)); out.extend(self.menu_prompt_msg(state, buffer_id)); out.extend(self.minibuffer_prompt_msg(state, buffer_id)); out.extend(self.theme_facts_msg(state)); out.extend(self.font_facts_msg(state)); + // Q#SL6/Q#SL8: face inventory must precede segment text. if let Some(evaluation) = statusline_evaluation { self.emit_statusline_segments(evaluation, &mut out); } - Some(out) + out } /// Apply the lead evaluator's publication outcome to the v18 wire diff --git a/tests/vterm_stage3_acceptance.rs b/tests/vterm_stage3_acceptance.rs index 80ef188..4a170af 100644 --- a/tests/vterm_stage3_acceptance.rs +++ b/tests/vterm_stage3_acceptance.rs @@ -767,6 +767,278 @@ fn a37_real_daemon_real_pty_and_headless_gpu_render_one_terminal_session() { ); } +/// Review round 1, finding 1: a frontend that enters terminal mode must +/// still tell its peers it left the document. +/// +/// **This is a regression guard, not a bite-verified fix.** The review +/// predicted that skipping the presence sweep in terminal mode freezes +/// `last_broadcast` at the abandoned document position. It does not, and +/// this test passes against the pre-fix tree: the buffer-follow clears +/// the terminal declaration when it ships the snapshot, so +/// `terminal_active` is false on the tick a window first shows a +/// terminal, and the declaration cannot arrive until a later tick — one +/// truthful sweep always lands first. The skip was load-bearing on that +/// ordering and bought nothing, so it is gone; this test pins the +/// resulting invariant against a future reordering that would make the +/// predicted freeze real. +/// +/// Real daemon, real wire, two real frontends: presence delivery is a +/// property of the dispatcher loop, not of any function it calls. +#[cfg(feature = "crdt")] +#[test] +fn terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret() { + use pmacs::protocol::{ + AttachRequest, FrontendCapabilities, Hello, Key, KeyEvent, PROTOCOL_VERSION, read_message, + write_message, + }; + use std::os::unix::net::UnixStream; + + fn attach(daemon: &common::daemon::TestDaemon, semantic: bool) -> (Hello, UnixStream) { + let mut stream = daemon.connect(); + stream + .set_read_timeout(Some(Duration::from_secs(2))) + .expect("read timeout"); + let hello: Hello = read_message(&mut stream).expect("read Hello"); + let req = AttachRequest { + protocol_version: hello.protocol_version, + frontend_capabilities: FrontendCapabilities { + synchronized_output: false, + unicode_smp: true, + true_color: true, + mouse: false, + bracketed_paste: false, + terminal_kind: Some("acceptance".into()), + multi_frontend: true, + crdt_replica: true, + semantic_render: semantic, + }, + initial_size: CellSize::new(24, 80), + }; + write_message(&mut stream, &req).expect("write AttachRequest"); + (hello, stream) + } + + /// Pump one stream until `want` returns a value, or time out. + fn pump( + stream: &mut UnixStream, + what: &str, + mut want: impl FnMut(&InstanceMessage) -> Option, + ) -> T { + let deadline = Instant::now() + Duration::from_secs(20); + while Instant::now() < deadline { + match read_message::(stream) { + Ok(msg) => { + if let Some(found) = want(&msg) { + return found; + } + } + Err(_) => continue, + } + } + panic!("timed out waiting for {what}"); + } + + assert_eq!(PROTOCOL_VERSION, 19); + let daemon = common::daemon::TestDaemon::spawn_with_env_and_init( + &[ + ("PMACS_INSTANCE_SEMANTIC_RENDER", "1"), + ("PMACS_INSTANCE_MULTI_FRONTEND", "1"), + ], + PROBE_INIT_LUA, + ); + + // B attaches FIRST. The presence sweep is diff-keyed, so A's very + // first snapshot is its only guaranteed broadcast — if B were not + // yet a registered recipient, that broadcast would reach nobody and + // A's presence would sit unchanged (and unsent) forever after. + let (_hello_b, mut b) = attach(&daemon, false); + // A is the semantic frontend that will open a terminal; B is the + // peer whose document view would keep painting A's stale caret. + let (hello_a, mut a) = attach(&daemon, true); + let a_id = hello_a.assigned_frontend_id; + + // A declares a byte viewport so it is a live semantic session. + let document = pump(&mut a, "A's first BufferSnapshot", |msg| match msg { + InstanceMessage::BufferSnapshot { buffer_id, .. } => Some(*buffer_id), + _ => None, + }); + write_message( + &mut a, + &pmacs::protocol::FrontendEvent::Viewport { + frontend_id: a_id, + buffer_id: document, + visible: pmacs::protocol::ByteRange { start: 0, end: 0 }, + generation: 0, + }, + ) + .expect("A declares a viewport"); + + // B sees A in the document. Without this the later assertion could + // pass vacuously against a peer that never had presence at all. + let seen_in_document = pump(&mut b, "A's presence in the document", |msg| match msg { + InstanceMessage::PresenceUpdate { + frontend_id, + buffer_id, + .. + } if *frontend_id == a_id => Some(*buffer_id), + _ => None, + }); + assert_eq!(seen_in_document, document); + + // A opens a terminal through the bound chord and declares its cells. + write_message( + &mut a, + &pmacs::protocol::FrontendEvent::Key(KeyEvent { + frontend_id: a_id, + key: Key::Char('t'), + mods: Modifiers::CTRL | Modifiers::ALT, + timestamp_ns: 0, + }), + ) + .expect("A opens a terminal"); + let terminal = pump(&mut a, "A's terminal BufferSnapshot", |msg| match msg { + InstanceMessage::BufferSnapshot { buffer_id, .. } if *buffer_id != document => { + Some(*buffer_id) + } + _ => None, + }); + write_message( + &mut a, + &pmacs::protocol::FrontendEvent::TerminalResize { + frontend_id: a_id, + buffer_id: terminal, + size: CellSize::new(12, 40), + }, + ) + .expect("A declares terminal cells"); + // A really is in terminal mode once a frame arrives. + let framed = pump(&mut a, "A's first TerminalFrame", |msg| match msg { + InstanceMessage::TerminalFrame(frame) => Some(frame.buffer_id), + _ => None, + }); + assert_eq!(framed, terminal); + + // The finding: B must learn that A left the document. + let seen_after = pump( + &mut b, + "A's presence leaving the document", + |msg| match msg { + InstanceMessage::PresenceUpdate { + frontend_id, + buffer_id, + .. + } if *frontend_id == a_id && *buffer_id != document => Some(*buffer_id), + _ => None, + }, + ); + assert_eq!( + seen_after, terminal, + "A's presence must move into the terminal identity buffer, not freeze \ + at the document position it abandoned" + ); +} + +/// Review round 1, finding 2: hover must not claim durable control. +/// +/// A semantic frontend reports motion at pixel rate, so if bare `Move` +/// claimed the controller, sweeping the mouse across a PASSIVE split's +/// terminal would take it — and the next layout sync would resize the +/// shared PTY to that background view's geometry. Every deliberate +/// gesture still claims; only motion does not. +#[test] +fn hover_does_not_steal_terminal_control_from_the_active_frontend() { + let mut state = EditorState::new(); + let owner = FrontendId(71); + let bystander = FrontendId(72); + let terminal_buffer = open_terminal(&mut state, "sleep 30", 6, 20); + tick_until(&mut state, Duration::from_secs(5), |state| { + state + .terminal_manager + .borrow() + .snapshot(terminal_buffer) + .is_some() + }); + let owner_window = attach_view(&state, owner, terminal_buffer); + let bystander_window = attach_view(&state, bystander, terminal_buffer); + + let owner_size = CellSize::new(6, 20); + let bystander_size = CellSize::new(4, 12); + let owner_key = TerminalViewKey::new(owner, owner_window, terminal_buffer); + let bystander_key = TerminalViewKey::new(bystander, bystander_window, terminal_buffer); + { + let mut manager = state.terminal_manager.borrow_mut(); + assert!(manager.record_view_size(owner_key, owner_size)); + assert!(manager.record_view_size(bystander_key, bystander_size)); + } + + // The owner takes control with a real press. + assert!(state.dispatch_semantic_terminal_pointer( + owner, + terminal_buffer, + CellCoord::new(0, 0), + MouseKind::Down(MouseButton::Left), + Modifiers::NONE, + )); + assert_eq!( + state.terminal_manager.borrow().controller(terminal_buffer), + Some(pmacs::terminal::TerminalController::from_view(owner_key)), + "a press claims control" + ); + + // The bystander merely hovers, repeatedly. Control must not move. + for col in 0..4 { + assert!(state.dispatch_semantic_terminal_pointer( + bystander, + terminal_buffer, + CellCoord::new(0, col), + MouseKind::Move, + Modifiers::NONE, + )); + } + assert_eq!( + state.terminal_manager.borrow().controller(terminal_buffer), + Some(pmacs::terminal::TerminalController::from_view(owner_key)), + "hovering a passive view must not take durable control" + ); + + // And the shared PTY keeps the controller's geometry: a stolen + // controller would resize it to the bystander's smaller view. + assert!(!state.sync_semantic_terminal_layout(bystander, terminal_buffer, bystander_size)); + assert_eq!( + state + .terminal_manager + .borrow() + .snapshot(terminal_buffer) + .expect("snapshot") + .size, + owner_size, + "a hovered-over passive view must not resize the shared screen" + ); + + // A deliberate gesture from the bystander still claims, so the + // hover exemption is narrow rather than a dead controller path. + assert!(state.dispatch_semantic_terminal_pointer( + bystander, + terminal_buffer, + CellCoord::new(0, 1), + MouseKind::Down(MouseButton::Left), + Modifiers::NONE, + )); + assert_eq!( + state.terminal_manager.borrow().controller(terminal_buffer), + Some(pmacs::terminal::TerminalController::from_view( + bystander_key + )), + "a press still claims control" + ); + + state + .terminal_manager + .borrow_mut() + .terminate(terminal_buffer, &mut state.process_supervisor.borrow_mut()) + .expect("terminate child"); +} + /// Acceptance 30 (v18 half) and 28: a peer that negotiated v18 receives /// no terminal message at all and keeps the ordinary document path over /// the empty identity buffer.