neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(vterm): address stage 3 review round 2 

One real defect, three cleanups, and a named deferral.

A daemon disconnect in terminal mode hid the disconnect notice. The
Disconnected arm 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 — so the user was left looking at a
frozen, live-looking terminal that silently ignored input. GPU auto-reconnect
is a named deferral, so that state persisted 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.

The fix and its test share a file, 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 on the "must leave terminal
mode" assertion; restoring it makes it pass.

sync_semantic_terminal_layout no longer clones the whole visible cell grid to
read one size. It ran every dispatcher tick for any semantic frontend with a
declared terminal; TerminalManager::screen_size reads the value from the
borrowed projection instead.

Inbound terminal events now require a negotiated v19 session. The outbound
TerminalFrame was gated twice while TerminalResize/TerminalPointer relied on
the frontend's send gate alone. A pre-v19 peer cannot construct those
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.

A terminal-mode press that misses the grid no longer arms a drag, so a later
in-grid motion cannot send a Drag with no preceding Down. Daemon-side impact
was nil; the state is now honest. A release still always ends the drag.

The roadmap and handoff Arc 5 lines still said Stage 3 was framed and
awaiting approval, contradicting this PR's own ledger. Both corrected.

Named deferral: terminal wheel gestures discard scroll magnitude. One winit
wheel event becomes one gesture regardless of the lines it accumulated, while
the document path scrolls by lines. Closing it means either N gestures
(chattier) or a magnitude field on the pointer event — a protocol change.
Neither belongs in this stage.

Gates: fmt; strict workspace clippy; 1,758 default + 1,934 CRDT library
tests; Stage 1 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; diff check clean.
This commit is contained in:
Levi Neuwirth 2026-07-22 15:33:26 -04:00
parent 50fd9a08e4
commit 9ff6a62623
8 changed files with 223 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
docs/active-work.md
View File

@ -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:

4
docs/agent-handoff.md
View File

@ -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

12
docs/roadmap-2026-07.md
View File

@ -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)

51
docs/vterm-framing.md
View File

@ -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

83
pmacs-gpu/src/main.rs
View File

@ -1914,12 +1914,21 @@ impl ApplicationHandler<AppEvent> 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<AppEvent> 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<AppEvent> 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(&notice_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]

57
src/daemon.rs
View File

@ -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(&registry, old_peer));
assert!(peer_declared_terminal_support(&registry, new_peer));
// An unknown session is refused rather than defaulted open.
assert!(!peer_declared_terminal_support(&registry, FrontendId(99)));
}
#[test]
fn build_identity_includes_version_and_uptime() {
let s = DaemonState::new(Some("research".into()));

10
src/editor.rs
View File

@ -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;
}

13
src/terminal/view.rs
View File

@ -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<CellSize> {
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<CellSize> {
self.sessions
.get(&key.buffer_id)
.get(&buffer_id)
.map(|session| session.screen.projection_ref().size)
}