From 9b364adc267bff6c7df75cb4c7bc8c19318ed64d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 17:31:04 -0400 Subject: [PATCH] =?UTF-8?q?fix(panel):=20review=20round=201=20=E2=80=94=20?= =?UTF-8?q?buffer=5Fid,=20the=20transport=20ratchet,=20shared=20bounds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — `PanelPointer` was missing the approved `buffer_id`. Q#BP16 gives it and `panel_epoch` different jobs and neither subsumes the other: `buffer_id` catches an A->B buffer replacement, `panel_epoch` catches close/hide/reopen of the SAME persistent buffer, which a buffer id alone cannot see. Added in the framing's field order, with a pin asserting each field independently reaches the wire. P1 — added parent criterion 39's transport-safety ratchet. It builds the maximum legal panel payload, asserts the fixture actually spends the whole aggregate glyph budget (otherwise the ratchet measures something smaller than the worst case), asserts one byte more is rejected, and pins the encoded `InstanceMessage::PanelFrame` below `MAX_FRAME_BYTES`. Shaped `1 x MAX_PANEL_VISIBLE_CELLS` deliberately: no per-axis cap makes that a legal panel geometry a terminal cannot express, so it is the worst case the terminal's own ratchet never measured. Bitten by tripling the glyph budget — 30,342,696 bytes against the 16 MiB cap. P2 — the shared bounds were duplicated literals. `MAX_TERMINAL_GRAPHEME_BYTES` now aliases `MAX_WIRE_GRID_GRAPHEME_BYTES`, so the terminal screen's truncation (`src/terminal/screen.rs:697`, `:777`) and the validator cannot drift. Two more had the same defect and are aliased too: `MAX_TERMINAL_VISIBLE_CELLS` and `MAX_TERMINAL_FRAME_GLYPH_BYTES`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RuhVYUPHXMHG8r2z4tsDPR --- pmacs-protocol/src/message.rs | 8 + pmacs-protocol/src/panel.rs | 2 +- pmacs-protocol/src/terminal.rs | 16 +- pmacs-protocol/src/wire_grid.rs | 8 + ...ottom_panel_stage2b_protocol_acceptance.rs | 180 +++++++++++++++++- 5 files changed, 208 insertions(+), 6 deletions(-) diff --git a/pmacs-protocol/src/message.rs b/pmacs-protocol/src/message.rs index 4103cd4..e3795a3 100644 --- a/pmacs-protocol/src/message.rs +++ b/pmacs-protocol/src/message.rs @@ -491,6 +491,12 @@ pub enum FrontendEvent { /// Carries both epochs so a gesture aimed at a panel that has since /// been replaced or reopened cannot be applied to its successor. /// Unlike [`Self::Pointer`], accepting this **activates the panel**. + /// + /// `buffer_id` and `panel_epoch` close different holes and neither + /// subsumes the other: `buffer_id` catches an A→B buffer + /// replacement, while `panel_epoch` catches close/hide/reopen of the + /// **same** persistent buffer — which a buffer id alone cannot + /// distinguish — without putting a `WindowId` on the wire. PanelPointer { /// Which frontend produced the gesture (untrusted, as above). frontend_id: FrontendId, @@ -498,6 +504,8 @@ pub enum FrontendEvent { geometry_epoch: u64, /// Presentation identity this gesture addresses. panel_epoch: u64, + /// Buffer the frontend believed the panel was displaying. + buffer_id: crate::BufferId, /// Cell the pointer is over, within the declared panel grid. coord: CellCoord, /// Which gesture step this is. diff --git a/pmacs-protocol/src/panel.rs b/pmacs-protocol/src/panel.rs index d6e19d7..e571ca4 100644 --- a/pmacs-protocol/src/panel.rs +++ b/pmacs-protocol/src/panel.rs @@ -22,7 +22,7 @@ use crate::wire_grid::{ /// /// Identical to the terminal bound: it is the transport-safety limit, /// not a PTY policy, so both messages answer to it. -pub const MAX_PANEL_VISIBLE_CELLS: usize = 262_144; +pub const MAX_PANEL_VISIBLE_CELLS: usize = crate::wire_grid::MAX_WIRE_GRID_VISIBLE_CELLS; /// Bounds a panel frame enforces on its cell grid. /// diff --git a/pmacs-protocol/src/terminal.rs b/pmacs-protocol/src/terminal.rs index b1b0016..8f7f507 100644 --- a/pmacs-protocol/src/terminal.rs +++ b/pmacs-protocol/src/terminal.rs @@ -37,10 +37,20 @@ pub const MAX_TERMINAL_COLS: u16 = 512; /// Maximum visible terminal cells accepted at creation, resize, or on /// the wire. -pub const MAX_TERMINAL_VISIBLE_CELLS: usize = 262_144; +/// +/// An alias of the shared wire-grid bound: this is transport safety, not +/// a PTY policy, so it must not drift from the panel's. +pub const MAX_TERMINAL_VISIBLE_CELLS: usize = crate::wire_grid::MAX_WIRE_GRID_VISIBLE_CELLS; /// Maximum UTF-8 bytes retained in one terminal grapheme cluster. -pub const MAX_TERMINAL_GRAPHEME_BYTES: usize = 256; +/// +/// An **alias** of the shared wire-grid bound, not an independent value. +/// The terminal screen truncates clusters to this constant while +/// [`crate::wire_grid`] validates against its own; if the two were +/// separate literals, raising one would make the producer emit clusters +/// its own validator rejects — or, worse, accept clusters no frontend +/// budgeted for. Keeping this a re-export means they cannot drift. +pub const MAX_TERMINAL_GRAPHEME_BYTES: usize = crate::wire_grid::MAX_WIRE_GRID_GRAPHEME_BYTES; /// Shared cap for terminal title and process-outcome metadata. pub const MAX_TERMINAL_METADATA_BYTES: usize = 1_024; @@ -56,7 +66,7 @@ pub const MAX_TERMINAL_METADATA_BYTES: usize = 1_024; /// protocol test `maximum_legal_terminal_frame_encodes_below_the_transport_cap` /// measures the largest legal frame this bound admits and pins it below /// the unchanged 16 MiB cap. -pub const MAX_TERMINAL_FRAME_GLYPH_BYTES: usize = 8 * 1024 * 1024; +pub const MAX_TERMINAL_FRAME_GLYPH_BYTES: usize = crate::wire_grid::MAX_WIRE_GRID_GLYPH_BYTES; // --------------------------------------------------------------------------- // Payload types diff --git a/pmacs-protocol/src/wire_grid.rs b/pmacs-protocol/src/wire_grid.rs index 1ff40c9..a9449df 100644 --- a/pmacs-protocol/src/wire_grid.rs +++ b/pmacs-protocol/src/wire_grid.rs @@ -39,6 +39,14 @@ pub const MAX_WIRE_GRID_GLYPH_BYTES: usize = 8 * 1024 * 1024; /// Per-cell grapheme-cluster byte ceiling shared by every wire grid. pub const MAX_WIRE_GRID_GRAPHEME_BYTES: usize = 256; +/// Visible-cell ceiling shared by every wire grid. +/// +/// This is the transport-safety bound, not a per-message policy: it is +/// what keeps `rows * cols * per-cell` inside the transport frame limit, +/// so both the terminal and the panel answer to it even though they +/// carry different per-axis caps. +pub const MAX_WIRE_GRID_VISIBLE_CELLS: usize = 262_144; + /// Bounds a particular wire grid enforces. /// /// `max_rows` / `max_cols` are per-message policy. `max_visible_cells` diff --git a/tests/bottom_panel_stage2b_protocol_acceptance.rs b/tests/bottom_panel_stage2b_protocol_acceptance.rs index 91a8d48..c3ec239 100644 --- a/tests/bottom_panel_stage2b_protocol_acceptance.rs +++ b/tests/bottom_panel_stage2b_protocol_acceptance.rs @@ -5,12 +5,16 @@ //! projection, the epoch state machine, and the GPU band are later //! slices of this stage and are not exercised here. -use pmacs_protocol::cell::{Cell, CellCoord, CellSize, Glyph, Style}; +use pmacs_protocol::cell::{Cell, CellCoord, CellSize, Color, Glyph, Style, UnderlineStyle}; use pmacs_protocol::message::{FrontendEvent, InstanceMessage, Modifiers, MouseButton, MouseKind}; -use pmacs_protocol::panel::{PanelFrame, PanelFrameError, PanelFramePayload}; +use pmacs_protocol::panel::{ + MAX_PANEL_VISIBLE_CELLS, PanelFrame, PanelFrameError, PanelFramePayload, +}; use pmacs_protocol::terminal::{ MAX_TERMINAL_COLS, TerminalFrame, TerminalFrameError, TerminalProcessState, }; +use pmacs_protocol::transport::MAX_FRAME_BYTES; +use pmacs_protocol::wire_grid::{MAX_WIRE_GRID_GLYPH_BYTES, MAX_WIRE_GRID_GRAPHEME_BYTES}; use pmacs_protocol::{BufferId, FrontendId, PROTOCOL_VERSION, SUPPORTED_PROTOCOL_VERSIONS}; fn cell(ch: char) -> Cell { @@ -21,6 +25,43 @@ fn cell(ch: char) -> Cell { } } +/// The style whose postcard encoding is as long as a legal `Style` gets. +fn maximal_style() -> Style { + Style { + fg: Color::Rgb(0xff, 0xee, 0xdd), + bg: Color::Rgb(0x11, 0x22, 0x33), + bold: true, + italic: true, + underline: UnderlineStyle::Dashed, + reverse: true, + underline_color: Color::Rgb(0x44, 0x55, 0x66), + } +} + +fn maximal_cell(glyph: Glyph) -> Cell { + Cell { + glyph, + style: maximal_style(), + attachment: None, + } +} + +/// A single-column cluster of exactly `len` UTF-8 bytes. +fn cluster_of_len(len: usize) -> Vec { + assert!((1..=MAX_WIRE_GRID_GRAPHEME_BYTES).contains(&len)); + let mut text = String::with_capacity(len); + if len % 2 == 1 { + text.push(' '); + } else { + text.push('\u{e9}'); + } + while text.len() < len { + text.push('\u{301}'); + } + assert_eq!(text.len(), len); + text.into_bytes() +} + fn panel_frame(rows: u32, cols: u32) -> PanelFrame { PanelFrame { buffer_id: BufferId::from_raw(9), @@ -121,6 +162,7 @@ fn the_three_panel_events_round_trip() { frontend_id: fid, geometry_epoch: 2, panel_epoch: 7, + buffer_id: BufferId::from_raw(21), coord: CellCoord::new(3, 9), kind: MouseKind::Down(MouseButton::Left), mods: Modifiers::default(), @@ -134,6 +176,45 @@ fn the_three_panel_events_round_trip() { } } +#[test] +fn panel_pointer_carries_buffer_id_distinctly_from_panel_epoch() { + // The two fields close different holes and neither subsumes the + // other: `buffer_id` catches an A->B buffer replacement, while + // `panel_epoch` catches close/hide/reopen of the SAME buffer, which + // a buffer id alone cannot see. So each must independently reach the + // wire — a field silently dropped from the encoding would let one of + // those two stale gestures through. + let base = |buffer: u64, panel_epoch: u64| FrontendEvent::PanelPointer { + frontend_id: FrontendId(4), + geometry_epoch: 2, + panel_epoch, + buffer_id: BufferId::from_raw(buffer), + coord: CellCoord::new(1, 1), + kind: MouseKind::Down(MouseButton::Left), + mods: Modifiers::default(), + }; + let encode = |e: &FrontendEvent| postcard::to_allocvec(e).expect("encode"); + + // Same panel epoch, different buffer: must differ on the wire. + assert_ne!(encode(&base(1, 7)), encode(&base(2, 7))); + // Same buffer, different panel epoch: must also differ. + assert_ne!(encode(&base(1, 7)), encode(&base(1, 8))); + + // And both survive decode rather than being defaulted. + let event = base(31, 7); + let decoded: FrontendEvent = postcard::from_bytes(&encode(&event)).expect("decode"); + let FrontendEvent::PanelPointer { + buffer_id, + panel_epoch, + .. + } = decoded + else { + panic!("expected a PanelPointer, got {decoded:?}"); + }; + assert_eq!(buffer_id, BufferId::from_raw(31)); + assert_eq!(panel_epoch, 7); +} + // --------------------------------------------------------------------------- // 37 — byte pins on the previous final variant of each extended enum // --------------------------------------------------------------------------- @@ -201,6 +282,7 @@ fn appending_panel_events_does_not_move_the_previous_final_event_discriminant() frontend_id: fid, geometry_epoch: 1, panel_epoch: 1, + buffer_id: BufferId::from_raw(1), coord: CellCoord::new(0, 0), kind: MouseKind::Down(MouseButton::Left), mods: Modifiers::default(), @@ -349,3 +431,97 @@ fn terminal_frames_are_unchanged_by_the_factoring() { }) )); } + +// --------------------------------------------------------------------------- +// 39 — the transport-safety ratchet +// --------------------------------------------------------------------------- + +/// The largest legal panel frame, plus the same frame one glyph byte over. +/// +/// Deliberately shaped `1 x MAX_PANEL_VISIBLE_CELLS`: a panel carries no +/// per-axis cap, so this is a legal panel geometry a terminal frame +/// cannot express, and it is therefore the worst case the terminal's own +/// ratchet never measured. +fn panel_budget_boundary_frames() -> (PanelFrame, PanelFrame) { + /// Shortest cluster length postcard encodes with a two-byte length + /// prefix, which is what makes a cluster cell maximally expensive. + const WIDE_PREFIX_LEN: usize = 128; + let area = MAX_PANEL_VISIBLE_CELLS; + + // Every cell owes at least one glyph byte; the rest of the budget is + // spent on as many two-byte-prefix clusters as it affords. + let spare = MAX_WIRE_GRID_GLYPH_BYTES - area; + let wide_cells = spare / (WIDE_PREFIX_LEN - 1); + let remainder = spare % (WIDE_PREFIX_LEN - 1); + assert!(wide_cells + usize::from(remainder > 0) <= area); + + let wide = cluster_of_len(WIDE_PREFIX_LEN).into_boxed_slice(); + let single = cluster_of_len(1).into_boxed_slice(); + let mut cells = Vec::with_capacity(area); + for index in 0..area { + let glyph = if index < wide_cells { + Glyph::Cluster(wide.clone()) + } else if index == wide_cells && remainder > 0 { + Glyph::Cluster(cluster_of_len(remainder + 1).into_boxed_slice()) + } else { + Glyph::Cluster(single.clone()) + }; + cells.push(maximal_cell(glyph)); + } + + let cols = u32::try_from(area).expect("area fits u32"); + let exact = PanelFrame { + buffer_id: BufferId::from_raw(u64::MAX), + panel_epoch: u64::MAX, + geometry_epoch: u64::MAX, + size: CellSize::new(1, cols), + cells, + cursor: Some(CellCoord::new(0, cols - 1)), + focused: true, + }; + + let mut over = exact.clone(); + // One more byte of glyph, nothing else changed. + let last = over.cells.len() - 1; + over.cells[last] = maximal_cell(Glyph::Cluster(cluster_of_len(3).into_boxed_slice())); + + (exact, over) +} + +#[test] +fn maximum_legal_panel_frame_encodes_below_the_transport_cap() { + let (exact, over) = panel_budget_boundary_frames(); + assert_eq!(exact.validate(), Ok(())); + + // The fixture must actually sit ON the boundary, or the ratchet + // below measures something smaller than the worst case and would + // stay green while a real maximum frame overran the transport. + let mut glyph_bytes = 0usize; + for cell in &exact.cells { + glyph_bytes += match &cell.glyph { + Glyph::Char(ch) => ch.len_utf8(), + Glyph::Cluster(bytes) => bytes.len(), + Glyph::Continuation => 0, + }; + } + assert_eq!( + glyph_bytes, MAX_WIRE_GRID_GLYPH_BYTES, + "the measured fixture must spend the whole aggregate budget" + ); + + // One byte over is rejected, which is what makes `exact` maximal. + assert!(matches!( + over.validate(), + Err(PanelFrameError::GlyphBudget { .. }) + )); + + let msg = InstanceMessage::PanelFrame(PanelFramePayload::Present(exact)); + let bytes = postcard::to_allocvec(&msg).expect("encode"); + assert!( + bytes.len() < MAX_FRAME_BYTES, + "largest legal panel frame encodes to {} bytes, at or above the \ + {MAX_FRAME_BYTES}-byte transport cap; the aggregate glyph bound no \ + longer keeps panel traffic inside the existing transport limit", + bytes.len() + ); +}