diff --git a/docs/active-work.md b/docs/active-work.md index eed0dcf..f3396d1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -92,10 +92,18 @@ If it does not, stop and repair the remote/fetch configuration. 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. +- Review round 2 addressed (framing §0.11): a daemon disconnect now leaves + terminal mode so the notice is visible (real defect, hand-verified + because fix and test share a file); the per-tick full-grid clone is + gone; inbound terminal events require a negotiated v19 session; a + grid-missing press no longer arms a drag; roadmap/handoff Arc 5 lines + corrected. Named deferral: terminal wheel gestures discard scroll + magnitude. +- Post-round-2 gates: 1,758 default + 1,934 CRDT library tests; required + GPU 129; workspace sweep 2,923 across 83 suites; Stage 3 acceptance 5 + default / 7 CRDT; M4 120; fmt, clippy, diff check clean. - Closed caveat: the once-seen required-GPU failure did not reproduce in - eight author runs plus four reviewer runs. Treated as environmental. + eight author runs plus five reviewer runs. Treated as environmental. - Next: further user review rounds on the PR. Recovery worktree: diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 9648dfe..a4a857f 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -353,8 +353,8 @@ commands, read `docs/active-work.md` immediately after this file. - **Arc 4 (themes + extensibility) COMPLETE** — named UI faces (#120), live GPU font preferences (#124), statusline providers (#125). - **Arc 5 terminal stage ACTIVE** — compile mode (#113), Vterm terminal - core (#126), and Vterm TUI (#130) landed; protocol/GPU Stage 3 is framed - and awaiting approval. + core (#126), and Vterm TUI (#130) landed; protocol/GPU Stage 3 is + implemented and in review as PR #135. - **Config registry COMPLETE (#127)** — not a numbered arc; it was the cross-cutting substrate ranked first on `docs/side-quest-backlog.md`'s north star, and it unblocks the diff --git a/docs/roadmap-2026-07.md b/docs/roadmap-2026-07.md index 9c1ef1c..e93f4f7 100644 --- a/docs/roadmap-2026-07.md +++ b/docs/roadmap-2026-07.md @@ -80,7 +80,7 @@ preference at protocol v17; and composable per-window `pmacs.statusline` providers transported to semantic/GPU frontends by protocol-v18 `StatuslineSegments`. -### Arc 5 — Terminal, staged — VTERM STAGE 3 FRAMED +### Arc 5 — Terminal, staged — VTERM STAGE 3 IN REVIEW - **Compile mode landed in #113**: line-oriented PTY/ANSI output, error-regex navigation, and `M-x compile`. @@ -92,11 +92,11 @@ protocol-v18 `StatuslineSegments`. - **Vterm Stage 2 TUI landed in #130**: terminal-window composition, input/resize, per-context scroll/selection/copy, authenticated frontend ownership, BEL/clipboard drainage, and the strict Lua surface. -- **Vterm Stage 3 protocol/GPU is framed at Revision 8**: additive protocol - v19 complete frames/events, an aggregate glyph-byte bound under the - unchanged transport cap, dual viewport bootstrap, authenticated semantic - routing, and native fixed-cell GPU terminal rendering. Implementation waits - for explicit framing approval. +- **Vterm Stage 3 protocol/GPU is implemented and in review (PR #135)**: + additive protocol v19 complete frames/events, an aggregate glyph-byte bound + under the unchanged transport cap, dual viewport bootstrap, authenticated + semantic routing, and native fixed-cell GPU terminal rendering. Landing it + closes Arc 5's terminal stage. ### Arc 6 — Folding (keystone gutter rider) diff --git a/docs/vterm-framing.md b/docs/vterm-framing.md index ee0b59d..b1124c0 100644 --- a/docs/vterm-framing.md +++ b/docs/vterm-framing.md @@ -429,6 +429,57 @@ Post-review gates: `cargo fmt --check`; strict workspace Clippy; 1,757 default + statusline 7/8 (default/CRDT); M4 120; required GPU 128; workspace sweep 2,921 across 83 suites (19 ignored); `git diff --check` clean. +### 0.11 Stage 3 review round 2 + +The second review verified all five round-1 fixes in code, re-ran the +required-GPU suite clean (a thirteenth consecutive pass, closing the flake +caveat), and found one new low-severity defect plus minor items. + +- **A disconnect in terminal mode hid the notice (finding 1) — real, fixed, + hand-verified.** `AttachEvent::Disconnected` set the placeholder text but + never left terminal mode, where the document code layer is not prepared at + all and the terminal glyph layer keeps painting its last frame. The user was + left looking at a frozen, live-looking terminal that silently ignored input — + and with GPU auto-reconnect a named deferral, until relaunch. + `State::on_daemon_disconnected` now leaves terminal mode, forces a repaint + even when the notice text is byte-identical, and requests a redraw. Its test + lives in the same file as the fix, so `scripts/bite`'s file granularity + cannot bite it; the equivalent was done by hand — neutralizing only the + `exit_terminal_mode()` call makes the test fail, restoring it makes it pass. +- **Per-tick full-grid clone removed (finding 2).** + `sync_semantic_terminal_layout` compared geometry via `snapshot(..).size`, + cloning the whole visible cell grid every dispatcher tick to answer one + comparison. `TerminalManager::screen_size` reads it from the borrowed + projection instead. +- **Roadmap and handoff Arc 5 lines corrected (finding 3).** Both still said + Stage 3 was framed and awaiting approval, contradicting this PR's own ledger. +- **A press that misses the grid no longer arms a drag (nit).** It set + `pointer_drag_active` unconditionally, so a later in-grid motion sent a + `Drag` with no preceding `Down`. Daemon-side impact was nil + (`update_selection` bails without a drag anchor), but the state is now + honest. A release still always ends the drag, including one that wandered + outside the grid. +- **Inbound terminal events now require a negotiated v19 session (finding 5).** + The outbound `TerminalFrame` was gated twice while the inbound declarations + relied on the frontend's send gate alone. A pre-v19 peer cannot construct + these variants, so this only refuses a hand-rolled client — and the a32 + forgery tests already prove such an event reaches nothing but the sender's + own authenticated active view — but the asymmetry was not deliberate, and + "gated in both directions" should be true of the code rather than only of the + frontends we ship. + +Deferred from this round, named: **terminal wheel gestures discard scroll +magnitude.** One winit wheel event becomes one terminal gesture regardless of +the lines it accumulated, so a two-tick event scrolls the same distance as a +one-tick event, while the document path scrolls by `lines`. Closing it means +either sending N gestures (chattier) or widening the terminal pointer event +with a magnitude — a protocol change. Not worth either inside this stage. + +Post-round-2 gates: `cargo fmt --check`; strict workspace Clippy; 1,758 default ++ 1,934 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 129; workspace sweep 2,923 +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 b6ef17d..565e693 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -1914,12 +1914,21 @@ impl ApplicationHandler for App { } let mods = translate_mods(self.modifiers); if state.terminal.is_some() { + let hit = self.terminal_pointer_hit(x, y); + let state = self.state.as_mut().expect("checked above"); let kind = match button_state { ElementState::Pressed => { - state.pointer_drag_active = true; + // A press that MISSES the grid (the status + // band, the trailing padding) starts no + // drag: arming the flag there would make a + // later in-grid motion send a `Drag` with no + // preceding `Down`. + state.pointer_drag_active = hit.is_some(); ProtocolMouseKind::Down(ProtocolMouseButton::Left) } ElementState::Released => { + // A release always ends the drag, including + // one that wandered outside the grid. state.pointer_drag_active = false; ProtocolMouseKind::Up(ProtocolMouseButton::Left) } @@ -1929,7 +1938,7 @@ impl ApplicationHandler for App { // 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) { + if let Some((buffer_id, coord)) = hit { self.send_terminal_pointer(buffer_id, coord, kind, mods); } return; @@ -2219,7 +2228,7 @@ impl ApplicationHandler for App { } AppEvent::Attach(AttachEvent::Disconnected(reason)) => { eprintln!("pmacs-gpu: daemon disconnected ({reason})"); - state.set_text("(daemon disconnected)"); + state.on_daemon_disconnected("(daemon disconnected)"); } } } @@ -3904,6 +3913,26 @@ impl State { } } + /// Show a disconnect notice, leaving terminal mode first. + /// + /// Terminal mode prepares NO document code layer, and the terminal + /// glyph layer keeps painting the last frame it was given. Setting + /// the text without leaving terminal mode therefore writes into a + /// layer nothing draws, and the user is left looking at a frozen, + /// live-looking terminal that silently ignores input — with GPU + /// auto-reconnect a named deferral, until relaunch. The F-008 + /// "make the teardown visible" contract applies to terminal mode + /// too. + fn on_daemon_disconnected(&mut self, notice: &str) { + self.exit_terminal_mode(); + if !self.set_text(notice) { + // Byte-identical text still needs a repaint: the frame that + // is on screen is the terminal's, not this notice. + self.reshape(); + } + self.request_redraw(); + } + /// Leave terminal mode and drop every terminal-only cache. fn exit_terminal_mode(&mut self) { self.terminal = None; @@ -13477,6 +13506,54 @@ mod tests { assert!(state.last_terminal_pointer_cell.is_none()); } + /// Review round 2, finding 1: a disconnect must leave terminal mode, + /// or the notice is invisible. + /// + /// Terminal mode prepares no document code layer while the terminal + /// glyph layer keeps painting its last frame, so a notice written + /// without leaving terminal mode never reaches the screen and the + /// user sees a frozen terminal that ignores input. + #[test] + fn a_disconnect_leaves_terminal_mode_so_the_notice_is_visible() { + let Some(mut state) = headless_or_skip(400, 300, "document text") else { + return; + }; + let buffer_id = BufferId::next(); + state.current_buffer_id = Some(buffer_id); + state.apply_terminal_frame(plain_terminal_frame(buffer_id, "live", 8)); + assert!( + state.terminal.is_some(), + "the probe starts in terminal mode" + ); + let terminal_px = state.render_offscreen(); + + state.on_daemon_disconnected("(daemon disconnected)"); + assert!( + state.terminal.is_none(), + "a disconnect must leave terminal mode" + ); + assert!( + state.terminal_text_buffers.is_empty(), + "terminal-only caches go with it" + ); + assert_eq!(state.current_text, "(daemon disconnected)"); + + // The notice actually reaches the screen: the frame differs from + // the terminal frame it replaced, which is the whole point of + // the fix. + let notice_px = state.render_offscreen(); + let differing = terminal_px + .iter() + .zip(¬ice_px) + .filter(|(a, b)| a != b) + .count(); + assert!( + differing > 500, + "the disconnect notice must repaint over the frozen terminal \ + ({differing} bytes differ)" + ); + } + /// 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 46ed1d1..fe28112 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -799,6 +799,26 @@ fn peer_accepts_statusline_message(protocol_version: u32, message: &InstanceMess protocol_version >= 18 || !matches!(message, InstanceMessage::StatuslineSegments { .. }) } +/// Whether a session negotiated the v19 wire, and may therefore drive +/// terminal state inbound. +/// +/// The outbound `TerminalFrame` is gated twice — in the producer and in +/// the write loop — and the inbound direction is now symmetric. A +/// pre-v19 peer cannot construct these variants at all (its enum lacks +/// them), so this only ever refuses a hand-rolled client; the a32 +/// forgery tests already prove such an event can reach nothing but the +/// sender's own authenticated active view. It is defense in depth, and +/// it makes "gated in both directions" true of the code rather than +/// only of the frontends we ship. +fn peer_declared_terminal_support( + session_registry: &SessionRegistry, + frontend_id: FrontendId, +) -> bool { + session_registry + .session_state(frontend_id) + .is_some_and(|state| state.negotiated_protocol_version >= 19) +} + /// The same belt-and-braces write-loop gate for the additive /// protocol-v19 terminal frame. The semantic producer skips construction /// for an older peer; this filter independently prevents an unknown @@ -1735,6 +1755,7 @@ fn handle_dispatcher_event( // the durable controller's declaration reaches the // shared PTY — a declaration never claims control. if semantic_states.contains_key(&source) + && peer_declared_terminal_support(session_registry, source) && editor.semantic_terminal_declaration_is_active(source, buffer_id) { if let Some(sem) = semantic_states.get_mut(&source) { @@ -1757,7 +1778,9 @@ fn handle_dispatcher_event( // forged id, a stale buffer, a missing declaration, // or an out-of-bounds cell all drop before any view, // controller, selection, menu, or PTY mutation. - if semantic_states.contains_key(&source) { + if semantic_states.contains_key(&source) + && peer_declared_terminal_support(session_registry, source) + { editor.dispatch_semantic_terminal_pointer( source, buffer_id, coord, kind, mods, ); @@ -2854,6 +2877,38 @@ mod tests { )); } + #[test] + fn inbound_terminal_events_require_a_negotiated_v19_session() { + // Review round 2, finding 5: the outbound `TerminalFrame` was + // gated twice (producer + write loop) while the inbound + // declarations relied on the frontend's send gate alone. A + // pre-v19 peer cannot construct these variants, so this only + // refuses a hand-rolled client — but it makes "gated in both + // directions" true of the code, not just of the frontends we + // ship. + let mut registry = SessionRegistry::new(); + let semantic = crate::protocol::NegotiatedCapabilities { + multi_frontend: true, + crdt_replica: true, + semantic_render: true, + }; + let old_peer = FrontendId(2); + let new_peer = FrontendId(3); + registry.register_session( + old_peer, + crate::presence::SessionState::new(18, semantic, 0), + ); + registry.register_session( + new_peer, + crate::presence::SessionState::new(19, semantic, 1), + ); + + assert!(!peer_declared_terminal_support(®istry, old_peer)); + assert!(peer_declared_terminal_support(®istry, new_peer)); + // An unknown session is refused rather than defaulted open. + assert!(!peer_declared_terminal_support(®istry, FrontendId(99))); + } + #[test] fn build_identity_includes_version_and_uptime() { let s = DaemonState::new(Some("research".into())); diff --git a/src/editor.rs b/src/editor.rs index bcba37c..6ac3509 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1215,11 +1215,11 @@ impl EditorState { if !controls { return false; } - let old_size = self - .terminal_manager - .borrow() - .snapshot(buffer_id) - .map(|snapshot| snapshot.size); + // Read the size from the borrowed projection rather than a + // snapshot: this runs every dispatcher tick, and + // `snapshot(..).size` would clone the whole visible grid to + // answer one comparison. + let old_size = self.terminal_manager.borrow().screen_size(buffer_id); if old_size == Some(size) { return false; } diff --git a/src/terminal/view.rs b/src/terminal/view.rs index d79d8b6..5d9d311 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -248,8 +248,19 @@ impl TerminalManager { /// Return the publication-consistent child grid size for one view. #[must_use] pub(crate) fn screen_size_for_view(&self, key: TerminalViewKey) -> Option { + self.screen_size(key.buffer_id) + } + + /// The shared screen's current size, read from the borrowed + /// projection. + /// + /// Deliberately not `snapshot(..).size`: that clones the whole + /// visible cell grid, and geometry comparison runs on every + /// dispatcher tick for every frontend with a declared terminal. + #[must_use] + pub fn screen_size(&self, buffer_id: BufferId) -> Option { self.sessions - .get(&key.buffer_id) + .get(&buffer_id) .map(|session| session.screen.projection_ref().size) }