From b9a7e40855bf24c85aacf38b43ed304c79b24837 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 22 Jul 2026 09:09:44 -0400 Subject: [PATCH] fix(vterm): harden view anchors and interactive authority Clamp anchors into partially evicted wrapped lines, require authenticated interactive origins, and avoid per-mouse cell snapshots. Restore dispatcher rationale, named context errors, and focused regressions for the corrected contracts. Co-authored-by: OpenAI Codex --- docs/vterm-framing.md | 9 ++++ src/editor.rs | 46 +++++++++++++---- src/editor_core.rs | 2 +- src/lua_bindings/mod.rs | 79 +++++++++++++++++++++------- src/terminal/screen.rs | 3 ++ src/terminal/view.rs | 89 +++++++++++++++++++++++++++++--- tests/m9_6_acceptance.rs | 24 +++++---- tests/vterm_stage2_acceptance.rs | 40 ++++++++++++++ 8 files changed, 245 insertions(+), 47 deletions(-) diff --git a/docs/vterm-framing.md b/docs/vterm-framing.md index f10ad74..84b50aa 100644 --- a/docs/vterm-framing.md +++ b/docs/vterm-framing.md @@ -873,6 +873,12 @@ that frontend's dispatcher, terminal views, controller claims, and bell baseline. These are Stage 2 changes to existing v18 grid behavior; no protocol bump or semantic/GPU terminal surface is added in this PR. +One intentional Stage 2 boundary remains until Stage 3: an authenticated v18 +semantic frontend can send a key while its active buffer is a terminal, claim +that terminal's controller, and feed the PTY, but cannot display the screen. +The next accepted TUI terminal input reclaims control; v19 removes the invisible +interval by adding the semantic terminal surface. + ## 6. Stage 3 — protocol v19 and GPU integration ### 6.1 Wire additions @@ -1234,6 +1240,9 @@ Not part of these three PRs: alternate-screen switches; - legacy X10 mouse byte encoding when a child enables mouse tracking without SGR mode; Stage 2 sends no report for that unsupported combination; +- bracketed-paste payload filtering: Stage 2 forwards exact paste bytes as + framed, so embedded `ESC[201~` can terminate the wrapper early; xterm-style + filtering/escaping requires a separate input-policy decision; - nonstandard `CSI 3 K` ignore semantics (the current core clears the line); - the ASCII fast path that avoids grapheme-candidate allocation and segmentation for every printable character after another ASCII character, diff --git a/src/editor.rs b/src/editor.rs index 693a56e..368b1c2 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -719,6 +719,12 @@ impl EditorState { } /// Whether `frontend_id` may optimistically self-insert its next key. + /// + /// `false` while a prefix, terminal escape, modal surface, or round-trip + /// buffer owns input. The daemon publishes this as `DispatchIdle`; returning + /// `true` while one of those surfaces is active would let a CRDT frontend + /// edit the document locally while the daemon routes the same key elsewhere + /// (M10.10, Q#SR5, Q#CM1, Q#QR1, Arc 1b Q#P6). #[must_use] pub fn dispatch_idle_for(&self, frontend_id: FrontendId) -> bool { if self @@ -735,7 +741,7 @@ impl EditorState { && !core.menu_is_open() && core .active_window_for(frontend_id) - .is_some_and(|window| !core.active_buffer_round_trips_for(window.buffer_id)) + .is_some_and(|window| !core.buffer_round_trips(window.buffer_id)) } /// Local-frontend compatibility wrapper. @@ -767,6 +773,9 @@ impl EditorState { if !matches!(key.kind, KeyEventKind::Press | KeyEventKind::Repeat) { return; } + // Authenticate every path through this input event, including modal + // callbacks such as M-x minibuffer acceptance. + let _origin = self.interactive_origin.enter(frontend_id); let chord = key_event_to_chord(key); { let mut core = self.core.borrow_mut(); @@ -782,6 +791,11 @@ impl EditorState { } } + // Modal surfaces beat both terminal transport and the completion popup. + // Menu/search/query-replace/minibuffer are full keymap shadows shared by + // grid and semantic input; each returns before the ordinary post-command + // edit check, so shadow handlers own any required hook fan-out (Q#CM1, + // Q#SR5, Q#QR1). // Global modal surfaces own input before terminal transport. if self.core.borrow().menu_is_open() { if let Some(chord) = chord { @@ -808,6 +822,9 @@ impl EditorState { return; } + // Completion is the one partial modal shadow (Q#C3): only its control + // chords are intercepted. A pending per-frontend prefix owns those + // chords instead, and ordinary keys continue to terminal/keymap dispatch. let dispatcher_pending = self .dispatchers .get(&frontend_id) @@ -820,6 +837,8 @@ impl EditorState { return; } + // Terminal transport precedes ordinary buffer/global bindings. `C-c` + // opens a fixed one-key editor escape; all unescaped keys go to the child. let terminal_key = self.active_terminal_key(frontend_id); let escaped = self .dispatchers @@ -878,9 +897,10 @@ impl EditorState { let pre_revision = self.active_buffer_revision(); match action { + // Kill ring Q#KR2: stamp the authenticated frontend before the + // body so nested interactive calls inherit the same origin. Action::Run { command, .. } => { self.core.borrow_mut().rotate_command(frontend_id, &command); - let _origin = self.interactive_origin.enter(frontend_id); if let Err(e) = self .lua_host .invoke_command(&command, mlua::MultiValue::new()) @@ -889,10 +909,14 @@ impl EditorState { format!("error in {command}: {}", first_line(&e.to_string())); } } + // A prefix is rendered from dispatcher state; dismissing the + // popup prevents its partial shadow from stealing continuation. Action::Pending { .. } => { self.core.borrow_mut().completion_popup_close(); } Action::Unbound { sequence } => { + // Self-insert is an interactive command boundary (Q#KR2). + // Arm Q#AP9 typed-edit metadata only across this dispatch. if let Some(ch) = printable_char(&sequence) { self.core .borrow_mut() @@ -900,12 +924,12 @@ impl EditorState { self.core.borrow_mut().typed_edit_arm(frontend_id, ch); let mut args = mlua::MultiValue::new(); args.push_back(mlua::Value::Integer(ch as i64)); - let _origin = self.interactive_origin.enter(frontend_id); if let Err(e) = self.lua_host.invoke_command("buffer.self-insert", args) { self.core.borrow_mut().status = format!("self-insert failed: {}", first_line(&e.to_string())); } } else { + // Emacs `undefined` is still a command boundary (Q#KR2). self.core.borrow_mut().break_command_chain(frontend_id); self.core.borrow_mut().status = format!("{}: not bound", display_sequence(&sequence)); @@ -1736,14 +1760,12 @@ impl EditorState { let shift = modifiers.contains(TerminalModifiers::SHIFT); let (at_bottom, modes, screen_size) = { let mut manager = self.terminal_manager.borrow_mut(); - let Some(snapshot) = manager.snapshot_for_view(key, viewport_size) else { + let Some(status) = manager.view_status_for_size(key, viewport_size) else { return; }; let modes = manager.modes_for_view(key).unwrap_or_default(); - let screen_size = manager - .snapshot(key.buffer_id) - .map_or(viewport_size, |snapshot| snapshot.size); - (snapshot.at_bottom, modes, screen_size) + let screen_size = manager.screen_size_for_view(key).unwrap_or(viewport_size); + (status.at_bottom, modes, screen_size) }; if !shift @@ -2543,11 +2565,15 @@ impl CompletionPopupKey { /// future non-crossterm frontend) can drive it directly against a /// Vec-backed [`crate::cell::CellGrid`] without going through a /// `RenderState`. +#[allow( + clippy::implicit_hasher, + reason = "the public renderer contract uses the canonical snapshot HashMap" +)] #[allow(clippy::too_many_lines, reason = "linear paint pipeline")] -pub fn paint_frame( +pub fn paint_frame( state: &EditorState, frontend_id: FrontendId, - terminal_snapshots: &HashMap, + terminal_snapshots: &HashMap, grid: &mut crate::cell::CellGrid<'_>, term_size: CellSize, ) -> Option { diff --git a/src/editor_core.rs b/src/editor_core.rs index 4625de9..ded8143 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -2828,7 +2828,7 @@ impl EditorCore { /// Whether an explicit buffer requires daemon-owned round-trip input. #[must_use] - pub fn active_buffer_round_trips_for(&self, buffer_id: BufferId) -> bool { + pub fn buffer_round_trips(&self, buffer_id: BufferId) -> bool { self.round_trip_buffers.contains(&buffer_id) } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 44b7707..d35967b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -5176,6 +5176,26 @@ fn install_buffer_kill(lua: &Lua, core: &SharedCore) -> mlua::Result<()> { Ok(()) } +fn rotate_interactive_command(lua: &Lua, name: &str) -> mlua::Result<()> { + let origin = lua + .app_data_ref::() + .ok_or_else(|| { + mlua::Error::external( + "pmacs.command.invoke_interactive: interactive frontend context is unavailable", + ) + })?; + let frontend_id = origin.current().ok_or_else(|| { + mlua::Error::external( + "pmacs.command.invoke_interactive: requires an active interactive frontend context", + ) + })?; + let core = lua.app_data_ref::().ok_or_else(|| { + mlua::Error::external("pmacs.command.invoke_interactive: editor core is unavailable") + })?; + core.borrow_mut().rotate_command(frontend_id, name); + Ok(()) +} + fn install_command_module(lua: &Lua, commands: &SharedCommandRegistry) -> mlua::Result { let command = lua.create_table()?; @@ -5248,18 +5268,7 @@ fn install_command_module(lua: &Lua, commands: &SharedCommandRegistry) -> mlua:: command.set( "invoke_interactive", lua.create_function(move |lua, (name, args): (String, Variadic)| { - let frontend_id = lua.app_data_ref::().map(|core| { - let mut core = core.borrow_mut(); - let frontend_id = core.active_frontend; - core.rotate_command(frontend_id, &name); - frontend_id - }); - let origin = lua - .app_data_ref::() - .map(|origin| origin.clone()); - let _origin_guard = frontend_id - .zip(origin.as_ref()) - .map(|(frontend_id, origin)| origin.enter(frontend_id)); + rotate_interactive_command(lua, &name)?; let body = { let r = cmds.borrow(); r.get(&name) @@ -8194,6 +8203,41 @@ fn active_terminal_view_key( )) } +fn terminal_context_integer(context: &Table, field: &str) -> mlua::Result { + match context.raw_get::(field)? { + Value::Integer(value) => u64::try_from(value).map_err(|_| { + mlua::Error::external(format!( + "pmacs.terminal.view_state: `{field}` must be nonnegative" + )) + }), + Value::Nil => Err(mlua::Error::external(format!( + "pmacs.terminal.view_state: missing field `{field}`" + ))), + other => Err(mlua::Error::external(format!( + "pmacs.terminal.view_state: `{field}` must be an integer, got {}", + other.type_name() + ))), + } +} + +fn terminal_context_buffer(context: &Table) -> mlua::Result { + match context.raw_get::("buffer")? { + Value::UserData(buffer) => buffer + .borrow::() + .map(|buffer| buffer.0) + .map_err(|_| { + mlua::Error::external("pmacs.terminal.view_state: `buffer` must be a buffer id") + }), + Value::Nil => Err(mlua::Error::external( + "pmacs.terminal.view_state: missing field `buffer`", + )), + other => Err(mlua::Error::external(format!( + "pmacs.terminal.view_state: `buffer` must be a buffer id, got {}", + other.type_name() + ))), + } +} + fn terminal_view_key_from_context( core: &SharedCore, context: &Table, @@ -8219,14 +8263,9 @@ fn terminal_view_key_from_context( "pmacs.terminal.view_state: unknown field `{field}`" ))); } - let frontend_raw = context.get::("frontend")?; - let frontend_id = crate::protocol::FrontendId(u64::try_from(frontend_raw).map_err(|_| { - mlua::Error::external("pmacs.terminal.view_state: `frontend` must be nonnegative") - })?); - let window_raw = u64::try_from(context.get::("window")?).map_err(|_| { - mlua::Error::external("pmacs.terminal.view_state: `window` must be nonnegative") - })?; - let buffer_id = context.get::("buffer")?.0; + let frontend_id = crate::protocol::FrontendId(terminal_context_integer(context, "frontend")?); + let window_raw = terminal_context_integer(context, "window")?; + let buffer_id = terminal_context_buffer(context)?; let core = core.borrow(); let Some(view) = core.views.get(&frontend_id) else { diff --git a/src/terminal/screen.rs b/src/terminal/screen.rs index 67c8bd7..11c92ef 100644 --- a/src/terminal/screen.rs +++ b/src/terminal/screen.rs @@ -120,6 +120,7 @@ pub struct ScreenProjection { #[allow(missing_docs)] #[derive(Clone, Copy)] pub(crate) struct BorrowedScreenProjection<'a> { + pub size: CellSize, pub alternate_active: bool, pub history_head: &'a [TerminalRow], pub history_tail: &'a [TerminalRow], @@ -138,6 +139,7 @@ impl BorrowedScreenProjection<'_> { impl ScreenProjection { pub(crate) fn as_borrowed(&self) -> BorrowedScreenProjection<'_> { BorrowedScreenProjection { + size: self.size, alternate_active: self.alternate_active, history_head: &self.history, history_tail: &[], @@ -582,6 +584,7 @@ impl TerminalScreen { self.main.history.as_slices() }; BorrowedScreenProjection { + size: self.size, alternate_active: self.alt_active, history_head, history_tail, diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 7740cce..b8cd2ad 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -212,15 +212,29 @@ impl TerminalManager { self.scroll_view(key, size, rows.saturating_mul(direction.signum())) } - /// Return fresh geometric status for one registered view. + /// Register or refresh one view at `viewport_size` and return its geometry + /// without allocating an owned cell snapshot. #[must_use] - pub fn view_status(&mut self, key: TerminalViewKey) -> Option { - let projection = self.sessions.get(&key.buffer_id)?.screen.projection_ref(); - let state = self.views.get_mut(&key)?; + pub(crate) fn view_status_for_size( + &mut self, + key: TerminalViewKey, + viewport_size: CellSize, + ) -> Option { + if !valid_viewport(viewport_size) { + return None; + } + let session = self.sessions.get(&key.buffer_id)?; + let projection = session.screen.projection_ref(); + let bell_count = session.screen.bell_count(); + let state = self.views.entry(key).or_insert_with(|| TerminalViewState { + alternate_active: Some(projection.alternate_active), + last_bell_count: bell_count, + ..TerminalViewState::default() + }); normalize_state(state, projection); + state.viewport_size = Some(viewport_size); let rows = retained_rows(projection); - let size = state.viewport_size?; - let geometry = view_geometry(&rows, state, size.rows); + let geometry = view_geometry(&rows, state, viewport_size.rows); Some(TerminalViewStatus { at_bottom: geometry.scroll_offset == 0, scroll_offset: geometry.scroll_offset, @@ -228,6 +242,21 @@ 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.sessions + .get(&key.buffer_id) + .map(|session| session.screen.projection_ref().size) + } + + /// Return fresh geometric status for one registered view. + #[must_use] + pub fn view_status(&mut self, key: TerminalViewKey) -> Option { + let size = self.views.get(&key)?.viewport_size?; + self.view_status_for_size(key, size) + } + /// Clear selection and resume live-tail following for one view. pub fn scroll_to_bottom(&mut self, key: TerminalViewKey) -> bool { let Some(state) = self.views.get_mut(&key) else { @@ -510,7 +539,10 @@ fn clamp_or_clear(rows: &RetainedRows<'_>, anchor: LogicalCellAnchor) -> Option< return Some(anchor_for(rows, resolved)); } let first = rows.first()?; - (anchor.logical_line_id < first.logical_line_id).then(|| row_lead(first)) + (anchor.logical_line_id < first.logical_line_id + || (anchor.logical_line_id == first.logical_line_id + && anchor.cell_offset < first.cell_offset)) + .then(|| row_lead(first)) } fn normalize_state(state: &mut TerminalViewState, projection: BorrowedScreenProjection<'_>) { @@ -920,6 +952,49 @@ mod tests { assert_eq!(bytes, "界".as_bytes()); } + #[test] + fn partially_evicted_wrapped_anchor_clamps_to_first_surviving_cell() { + let source = projection( + vec![row(7, 4, "tail", true)], + vec![row(8, 0, "next", false)], + ); + let first_survivor = LogicalCellAnchor { + logical_line_id: 7, + cell_offset: 4, + }; + let mut state = TerminalViewState { + top: Some(LogicalCellAnchor { + logical_line_id: 7, + cell_offset: 1, + }), + selection: Some(TerminalSelection { + anchor: LogicalCellAnchor { + logical_line_id: 7, + cell_offset: 2, + }, + head: LogicalCellAnchor { + logical_line_id: 8, + cell_offset: 1, + }, + }), + ..TerminalViewState::default() + }; + + normalize_state(&mut state, source.as_borrowed()); + + assert_eq!(state.top, Some(first_survivor)); + assert_eq!( + state.selection, + Some(TerminalSelection { + anchor: first_survivor, + head: LogicalCellAnchor { + logical_line_id: 8, + cell_offset: 1, + }, + }) + ); + } + #[test] fn alternate_switch_clears_view_anchors_and_selection() { let source = ScreenProjection { diff --git a/tests/m9_6_acceptance.rs b/tests/m9_6_acceptance.rs index 39c4182..4b5166d 100644 --- a/tests/m9_6_acceptance.rs +++ b/tests/m9_6_acceptance.rs @@ -26,8 +26,11 @@ use std::path::PathBuf; use std::time::{Duration, Instant}; +use crossterm::event::{KeyCode, KeyModifiers}; use pmacs::editor::EditorState; +use pmacs::frontend::KeyEvent; use pmacs::lua_bindings::PackageInstallOverride; +use pmacs::protocol::FrontendId; use tempfile::TempDir; fn fake_mcp_path() -> String { @@ -813,17 +816,19 @@ fn m9_6_mx_palette_invokes_mcp_tool_through_minibuffer_reentry() { .lua_host .eval( Some("type-cmd-name"), - r#" - pmacs.minibuffer.set_contents("m9_6-echo") - pmacs.minibuffer.accept() - "#, + r#"pmacs.minibuffer.set_contents("m9_6-echo")"#, ) .expect("accept command name"); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE), + ); + // Step 3: the tool's argument prompt must now be active. This is // the re-entrant minibuffer behavior: outer session was taken, // inner session was begun, and the begin happened from inside - // the outer accept's on_accept callback. + // the outer accept's authenticated dispatch callback. let inner_active: bool = state .lua_host .lua() @@ -841,12 +846,13 @@ fn m9_6_mx_palette_invokes_mcp_tool_through_minibuffer_reentry() { .lua_host .eval( Some("type-arg"), - r#" - pmacs.minibuffer.set_contents("through M-x") - pmacs.minibuffer.accept() - "#, + r#"pmacs.minibuffer.set_contents("through M-x")"#, ) .expect("accept arg"); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE), + ); assert!( pump_until_status_contains(&mut state, "through M-x", Duration::from_secs(2)), "M-x → arg-prompt → dispatch must reach status; status={:?}", diff --git a/tests/vterm_stage2_acceptance.rs b/tests/vterm_stage2_acceptance.rs index 86d1201..017a838 100644 --- a/tests/vterm_stage2_acceptance.rs +++ b/tests/vterm_stage2_acceptance.rs @@ -238,6 +238,46 @@ fn lua_surface_is_strict_fresh_transactional_and_context_safe() { .expect("pcall implicit scroll"); assert!(!ok); assert!(error.contains("interactive frontend context")); + + let (ok, error): (bool, String) = lua + .load( + r" + local ok, err = pcall(function() + pmacs.command.invoke_interactive('terminal.scroll-up') + end) + return ok, tostring(err) + ", + ) + .eval() + .expect("pcall ambient interactive invoke"); + assert!(!ok); + assert!(error.contains("active interactive frontend context")); + + for (field, source) in [ + ( + "frontend", + "pmacs.terminal.view_state { window = 1, buffer = TERM_BUFFER, active = true }", + ), + ( + "window", + "pmacs.terminal.view_state { frontend = 1, buffer = TERM_BUFFER, active = true }", + ), + ( + "buffer", + "pmacs.terminal.view_state { frontend = 1, window = 1, active = true }", + ), + ] { + let (_, error): (bool, String) = lua + .load(format!( + "local ok, err = pcall(function() {source} end); return ok, tostring(err)" + )) + .eval() + .expect("pcall incomplete explicit context"); + assert!( + error.contains(&format!("missing field `{field}`")), + "missing `{field}` surfaced as {error:?}" + ); + } } state