From 5cf1d61b92e0515ea561a888e6dcbc26691037da Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:31:22 +0200 Subject: [PATCH] fix(discovery): clip command descriptions at the single-row surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #228 review found a correctness gap this lane made reachable. The GPU dropdown derives its height, its visible window and its selection-highlight offset from `rows.len()` — ONE logical row per candidate — while a detail carrying a line break shapes into more physical lines than that. One such row misaligns every row below it and the highlight with it. The grid TUI has the same exposure from the other side: it writes the description into a single-row suffix on the minibuffer band. ## Why not reject CR/LF at registration That was the obvious fix. It was implemented, measured, and abandoned on evidence. MCP tool registration renders a whole schema block into `Command.description` — tool text, blank line, `Arguments:`, then one line per argument (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, a `table.concat(lines, "\n")`, used at `:496`). And `tests/m9_6_acceptance.rs:583-598` ASSERTS four of those lines. A one-line guard in `CommandRegistry::define` fails 36 tests across `m9_6` (19/25), `m9_7` (16/19) and `m9_8` (1/17), in both feature configurations, and could only be made green by deleting a shipped acceptance criterion. So the one-line constraint goes where the constraint actually is: the surfaces that have one row. `Command.description` stays free-form, which it legitimately is. ## The change `Command::description_first_line` clips to the first CR **or** LF — a lone CR ends a line too, and an LF-only clip would pass a bare `\r` straight through to the same surface. Both single-row consumers call it: the semantic producer filling `MinibufferRow.detail` (`src/semantic_render.rs`) and the TUI suffix (`src/editor.rs`). A first line that is empty ships as `None` rather than `Some("")`, which would draw trailing padding. No ellipsis or truncation marker, matching the in-tree precedent and the minibuffer's own width rule. `describe-command` and `help.list-commands` are untouched and still report every line. That is what makes this a rendering decision rather than data loss, and it is asserted, not assumed. ## Precedent, already in this tree The same MCP fixture clips a tool RESULT to its first line because "a multi-line set_status would corrupt the row layout" (`init.lua:277-285`), leaving width clipping to the frontend. Same hazard class, same resolution. ## Verification `src/command.rs`: a schema block registers AND clips, in all three break forms; a single-line description is byte-identical after the clip; an empty first line clips to empty. `tests/discovery_stage2_acceptance.rs`: an MCP-shaped description reaches the TUI band and the GPU row as one line, through the real prompt path — with the full text still reachable via `describe-command` asserted alongside, so a clip that deleted the schema block everywhere would fail rather than pass. `pmacs-gpu`: one physical shaped line per logical candidate row — the geometry invariant the dropdown depends on. Mutation-checked: neutering `first_line` to the identity fails all four new break-handling tests (`a_multi_line_description_registers_and_clips_to_its_first_line`, `a_description_whose_first_line_is_empty_clips_to_empty`, `a_multi_line_description_reaches_the_tui_band_as_one_line`, `a_multi_line_description_reaches_the_gpu_row_as_one_physical_line`) and leaves the two "did not tighten past purpose" tests green. `Command.description`'s doc comment claimed "one-line", which the MCP path openly violates. It now states the real contract and records why a registration guard must not be re-proposed. `m9_6`/`m9_7`/`m9_8` pass COMPLETELY UNTOUCHED, and are now named gate suites so that stays on the record. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 41 ++++++- pmacs-gpu/src/main.rs | 11 ++ src/command.rs | 147 ++++++++++++++++++++++++- src/editor.rs | 11 +- src/semantic_render.rs | 19 +++- tests/discovery_stage2_acceptance.rs | 157 ++++++++++++++++++++++++++- 6 files changed, 376 insertions(+), 10 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 57788bc..ad3851a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -322,13 +322,50 @@ tip** — the ref, not a SHA. Recover with local formatting change reading the registry directly. A multi-row TUI chooser is explicitly NOT this lane. - **Gates:** `scripts/gate --protocol --acceptance - discovery_stage2_acceptance` — the strengthened two-configuration - sweep, which is what `--protocol` exists for. + discovery_stage2_acceptance --acceptance m9_6_acceptance --acceptance + m9_7_acceptance --acceptance m9_8_acceptance` — the strengthened + two-configuration sweep, which is what `--protocol` exists for. The + three m9 suites are named because the PR #228 review round measured + them as this change's blast radius (see the description-clip bullet); + their continued passing is on the record rather than assumed. + **`--protocol` does NOT run its own documented precondition** + (`cargo build --workspace --no-default-features --features + luajit,crdt`, handoff §5) — run it by hand first or twelve + `gpu_invocation_acceptance` tests fail on a missing `pmacs-gpu` + binary. That omission is the `gate-protocol-build` lane's, not this + one's. - **IMPLEMENTED.** `PROTOCOL_VERSION` is 23, `ADVERTISED_PROTOCOL_VERSION` is untouched at 20. New suite `tests/discovery_stage2_acceptance.rs`; the daemon half is `crdt`-gated (a semantic session is necessarily a text replica) and runs one daemon serving a v22 and a v23 session simultaneously. +- **Multi-line descriptions are clipped AT THE SURFACE, and + registration-level rejection was investigated and REJECTED ON + EVIDENCE — do not re-propose it.** PR #228 review found the real + hazard: the GPU dropdown derives its height, visible window and + highlight offset from `rows.len()` (one logical row per candidate), + so a detail carrying a line break misaligns every row below it; the + TUI writes into a single-row band. The obvious fix — reject CR/LF in + `CommandRegistry::define` — was implemented and measured, and it + **fails 36 tests across `m9_6`/`m9_7`/`m9_8`**, because MCP tool + registration renders a whole schema block into `description` + (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, + `table.concat(lines, "\n")`, used at `:496`) and + **`tests/m9_6_acceptance.rs:583-598` asserts four separate lines of + it** — tool text, `Arguments:`, and two per-argument lines. No + single-line rendering satisfies those assertions, so a registry guard + could only go green by deleting a shipped acceptance criterion. + The one-line constraint belongs to the surfaces that have it: + `Command::description_first_line` clips, both single-row consumers + call it, and the full text still reaches `describe-command` / + `help.list-commands` untouched. Precedent already in-tree — the same + MCP fixture clips a tool RESULT to its first line because *"a + multi-line set_status would corrupt the row layout"* (`:277-285`). + **A startup census is not a corpus census**: booting an + `EditorState` and scanning all 180 registered descriptions found zero + offenders, because MCP registers at RUNTIME and builds the string by + concatenation — invisible to both that census and a grep for literals. + The workspace sweep is what caught it. - **The freeze is enforced by LITERAL byte fixtures**, not a round-trip — `minibuffer_prompt_v12_wire_bytes_are_frozen` in `src/protocol.rs`, the first such fixture in this repo. Bite-verified: reordering two diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index cbb58e3..78eb64a 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -15482,6 +15482,17 @@ mod tests { Some("buffer.kill"), "a row with no detail renders the bare label, exactly as before v23: {lines:?}" ); + // The geometry invariant the dropdown depends on: it derives + // its height, its visible window and its selection-highlight + // offset from `rows.len()`, so ONE physical line per logical + // row is what keeps those aligned. The daemon clips a detail to + // its first line (`Command::description_first_line`) precisely + // so this holds for an MCP schema block. + assert_eq!( + lines.len(), + state.minibuffer.as_ref().map_or(0, |mb| mb.rows.len()), + "one physical line per candidate row: {lines:?}" + ); // The frozen `12..=22` form, which an older daemon still sends: // bare strings become detail-free rows. diff --git a/src/command.rs b/src/command.rs index 2b21d7a..ec93899 100644 --- a/src/command.rs +++ b/src/command.rs @@ -66,7 +66,28 @@ impl SourceLocation { pub struct Command { /// Unique name (e.g. `buffer.save`). pub name: String, - /// One-line human-readable description (R42, required). + /// Human-readable description. Required and non-empty after trim + /// (R42), but otherwise **free-form, and legitimately multi-line**. + /// + /// # Do not add a registration-time one-line guard + /// + /// This doc used to read "one-line human-readable description", + /// which was an aspiration rather than the contract: MCP tool + /// registration renders a whole schema block in here — the tool's + /// text, a blank line, `Arguments:`, then one line per argument + /// (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, a + /// `table.concat(lines, "\n")`) — and `m9_6_acceptance.rs:583-598` + /// asserts all four of those lines. Rejecting CR/LF in + /// [`CommandRegistry::define`] was tried, measured, and abandoned: + /// it fails 36 tests across `m9_6`/`m9_7`/`m9_8` and could only be + /// made green by deleting a shipped acceptance criterion. + /// + /// The one-line constraint belongs to the **surfaces that have + /// it**, so a consumer rendering into a single row clips with + /// [`Self::description_first_line`] — the minibuffer band and the + /// completion dropdown both do. The full text stays intact for + /// `describe-command` and `help.list-commands`, which is what keeps + /// this a rendering decision rather than data loss. pub description: String, /// Where the command was defined. pub source: SourceLocation, @@ -79,6 +100,53 @@ pub struct Command { pub predicate: Option, } +impl Command { + /// [`Self::description`] clipped to its first line, for a consumer + /// rendering into a surface that has exactly one row. + /// + /// The description is free-form and may carry a whole schema block + /// (see that field). Two surfaces cannot show one: the grid TUI + /// writes the selected candidate into a single-row suffix on the + /// minibuffer band, and the GPU dropdown derives its height, its + /// visible window and its selection-highlight offset from + /// `rows.len()` — **one logical row per candidate** — so a detail + /// that shapes into more physical lines than that misaligns every + /// row below it and the highlight with it. + /// + /// Clipping here rather than refusing at registration follows the + /// precedent already in this tree: the MCP fixture's result + /// delivery keeps only the first line of a tool result because + /// *"a multi-line `set_status` would corrupt the row layout"* + /// (`tests/fixtures/pmacs-mcp-tools/init.lua:277-285`), leaving + /// width clipping to the frontend. Same hazard class, same + /// resolution. + /// + /// **No ellipsis or truncation marker**, matching that precedent + /// and the minibuffer's own width rule, which rejects stub markers + /// for the same reason: the full text is one `describe-command` + /// away, and a marker in a candidate row reads as part of the + /// candidate. + #[must_use] + pub fn description_first_line(&self) -> &str { + first_line(&self.description) + } +} + +/// The prefix of `text` before its first line break. +/// +/// Breaks on CR **or** LF, not LF alone: a lone CR ends a line on +/// classic-Mac-era input and is the leading half of a CRLF, so an +/// LF-only clip would pass a bare `\r` straight through to a +/// single-row surface — and a CR-only clip would do the same for `\n`. +/// Splitting on the first of either handles all three forms with one +/// scan, since CRLF's `\r` comes first. +fn first_line(text: &str) -> &str { + match text.find(['\n', '\r']) { + Some(break_at) => &text[..break_at], + None => text, + } +} + /// Errors raised by the command registry. #[derive(Debug, Error)] pub enum CommandError { @@ -271,6 +339,83 @@ mod tests { )); } + #[test] + fn a_multi_line_description_registers_and_clips_to_its_first_line() { + // Registration accepts it — MCP tool registration renders a + // whole schema block into `description` and + // `m9_6_acceptance.rs:583-598` asserts four of its lines, so a + // one-line guard here would delete a shipped contract. The + // one-line constraint lives at the single-row surfaces, which + // read `description_first_line`. + // + // All three break forms: a clip that split on `\n` alone would + // pass a bare `\r` through, and one that split on `\r` alone + // would pass `\n` through. + let lua = Lua::new(); + for (label, description) in [ + ( + "LF", + "Greet someone.\n\nArguments:\n name (string, required)", + ), + ( + "CR", + "Greet someone.\r\rArguments:\r name (string, required)", + ), + ( + "CRLF", + "Greet someone.\r\n\r\nArguments:\r\n name (string, required)", + ), + ] { + let mut r = CommandRegistry::new(); + r.define(make_command(&lua, "mcp.greet", description)) + .unwrap_or_else(|e| panic!("{label}: a schema block must still register: {e}")); + let cmd = r.get("mcp.greet").expect("registered"); + assert_eq!( + cmd.description, description, + "{label}: the registry stores the description verbatim — the clip is a \ + rendering decision, so `describe-command` must still see every line" + ); + assert_eq!( + cmd.description_first_line(), + "Greet someone.", + "{label}: a single-row surface gets the first line only" + ); + assert!( + !cmd.description_first_line().contains(['\n', '\r']), + "{label}: the clipped form must carry no break at all" + ); + } + } + + #[test] + fn a_single_line_description_is_byte_identical_after_the_clip() { + // The other half: the clip must not tighten past its purpose. + // Interior whitespace, punctuation and non-ASCII all survive, + // and there is no ellipsis or truncation marker. + let lua = Lua::new(); + let mut r = CommandRegistry::new(); + let description = "Write the buffer to its file — with a dash, and \ttabs."; + r.define(make_command(&lua, "buffer.save", description)) + .expect("registers"); + assert_eq!( + r.get("buffer.save").unwrap().description_first_line(), + description, + "a description with no break is returned unchanged" + ); + } + + #[test] + fn a_description_whose_first_line_is_empty_clips_to_empty() { + // The case the producer turns into `None` rather than + // `Some("")`: a leading break leaves nothing to render, and a + // `Some("")` detail would draw trailing padding after the label. + let lua = Lua::new(); + let mut r = CommandRegistry::new(); + r.define(make_command(&lua, "x", "\nArguments:\n a (string)")) + .expect("registers"); + assert_eq!(r.get("x").unwrap().description_first_line(), ""); + } + #[test] fn empty_name_is_rejected() { let lua = Lua::new(); diff --git a/src/editor.rs b/src/editor.rs index 7050023..0e78fe8 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -5575,13 +5575,22 @@ fn paint_minibuffer( // // Q#D2-2: only the command source has a detail. A file-path or // buffer-name prompt renders exactly as it did before. + // + // FIRST LINE ONLY: this band is a single row, and + // `Command.description` is free-form — MCP registration renders a + // whole schema block into it. The full text stays reachable through + // `describe-command`. let suffix = match session.selected.and_then(|idx| session.candidates.get(idx)) { Some(cand) => { let detail = matches!( session.source, crate::minibuffer::CompletionSource::Commands ) - .then(|| commands.get(cand).map(|c| c.description.as_str())) + .then(|| { + commands + .get(cand) + .map(crate::command::Command::description_first_line) + }) .flatten(); minibuffer_candidate_suffix(cand, detail, max.saturating_sub(col)) } diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 2e62952..3359504 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -1725,9 +1725,26 @@ impl SemanticRenderState { labels .into_iter() .map(|label| { + // FIRST LINE ONLY. `Command.description` + // is free-form and MCP registration puts + // a whole schema block in it, while the + // dropdown sizes itself from + // `rows.len()` — one logical row per + // candidate. Shipping the block would + // shape into more physical lines than + // the geometry accounts for and + // misalign every row below it. The full + // text stays reachable through + // `describe-command`. let detail = commands .get(&label) - .map(|command| command.description.clone()); + .map(|command| command.description_first_line().to_owned()) + // A description whose first line is + // empty (`"\nArguments:…"`) carries + // nothing to render, so it ships as + // absent rather than as `Some("")`, + // which would draw trailing padding. + .filter(|detail| !detail.is_empty()); MinibufferRow { label, detail } }) .collect() diff --git a/tests/discovery_stage2_acceptance.rs b/tests/discovery_stage2_acceptance.rs index f290368..571c605 100644 --- a/tests/discovery_stage2_acceptance.rs +++ b/tests/discovery_stage2_acceptance.rs @@ -34,7 +34,8 @@ use std::path::Path; use pmacs::bootstrap::BootstrapRoots; use pmacs::editor::EditorState; use pmacs_protocol::{ - ADVERTISED_PROTOCOL_VERSION, PROTOCOL_VERSION, is_supported_protocol_version, + ADVERTISED_PROTOCOL_VERSION, ByteRange, InstanceMessage, MinibufferRow, PROTOCOL_VERSION, + is_supported_protocol_version, }; #[cfg(feature = "crdt")] @@ -46,13 +47,11 @@ use std::time::{Duration, Instant}; use pmacs_protocol::cell::CellSize; #[cfg(feature = "crdt")] use pmacs_protocol::message::{ - AttachRequest, FrontendCapabilities, FrontendEvent, Hello, InstanceMessage, Key, KeyEvent, - Modifiers, SessionBootstrapRequest, + AttachRequest, FrontendCapabilities, FrontendEvent, Hello, Key, KeyEvent, Modifiers, + SessionBootstrapRequest, }; #[cfg(feature = "crdt")] use pmacs_protocol::transport::{read_message, write_message}; -#[cfg(feature = "crdt")] -use pmacs_protocol::{ByteRange, MinibufferRow}; #[cfg(feature = "crdt")] use common::daemon::{TestDaemon, build_default_caps}; @@ -148,6 +147,36 @@ fn bottom_row(s: &EditorState, rows: u32, cols: u32) -> String { row.into_iter().collect::().trim_end().to_owned() } +/// Open `M-x` narrowed to `zzprobe` and return the candidate rows the +/// semantic producer ships to a current-wire peer. +/// +/// Through `SemanticRenderState` and the real minibuffer session rather +/// than by constructing a message: the clip lives in the producer, so a +/// hand-built row would skip the thing under test. +fn mx_rows(s: &EditorState) -> Vec { + let bid = s.core.borrow().active_buffer_id(); + let mut render = pmacs::semantic_render::SemanticRenderState::for_peer( + pmacs::protocol::FrontendId::LOCAL, + PROTOCOL_VERSION, + ); + render.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); + let _ = render.render_frame(s); + + exec( + s, + "pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', on_accept = function() end }", + ); + exec(s, "pmacs.minibuffer.set_contents('zzprobe')"); + render + .render_frame(s) + .into_iter() + .find_map(|msg| match msg { + InstanceMessage::MinibufferPromptRows { rows, .. } => Some(rows), + _ => None, + }) + .expect("the producer ships a rows prompt") +} + /// Open `M-x`, narrowed to exactly one command with a known /// description, and report the bottom row at `cols` columns. fn mx_bottom_row(s: &EditorState, cols: u32) -> String { @@ -254,6 +283,124 @@ fn a_source_with_no_detail_renders_exactly_as_before_in_the_tui() { ); } +// --------------------------------------------------------------------------- +// Multi-line descriptions reach single-row surfaces as ONE line +// --------------------------------------------------------------------------- + +/// An MCP-shaped description: tool text, blank line, `Arguments:`, then +/// one line per argument. +/// +/// This is the real shape, not an invented one — +/// `tests/fixtures/pmacs-mcp-tools/init.lua:272` builds it with +/// `table.concat(lines, "\n")` and `m9_6_acceptance.rs:583-598` asserts +/// four of its lines, which is why registration accepts it and the +/// SURFACES clip instead. +const MCP_SHAPED: &str = "Greet someone.\\n\\nArguments:\\n name (string, required)"; + +fn define_multiline_probe(s: &EditorState, name: &str, description: &str) { + exec( + s, + &format!( + "pmacs.command.define{{ name = '{name}', description = \"{description}\", \ + fn = function() end }}" + ), + ); +} + +#[test] +fn a_multi_line_description_reaches_the_tui_band_as_one_line() { + let s = session("tui-multiline"); + define_multiline_probe(&s, "zzprobe", MCP_SHAPED); + let row = mx_bottom_row(&s, 200); + assert!( + row.contains("[zzprobe — Greet someone.]"), + "the band shows the first line only: {row:?}" + ); + assert!( + !row.contains("Arguments:"), + "the schema block must not reach a single-row band: {row:?}" + ); + // `bottom_row` reads one grid row, so anything below would be lost + // rather than visibly wrong — assert on the registry-side clip too, + // which is what the painter consumed. + let clipped: String = eval(&s, "return pmacs.describe.command('zzprobe').description"); + assert!( + clipped.contains("Arguments:"), + "describe-command must still see the WHOLE description, or the clip \ + silently deleted the schema block everywhere: {clipped:?}" + ); +} + +#[test] +fn a_multi_line_description_reaches_the_gpu_row_as_one_physical_line() { + // The geometry hazard, through the real prompt path: the dropdown + // sizes itself from `rows.len()` — one logical row per candidate — + // so a detail carrying a break would shape into more physical lines + // than the geometry accounts for. + // + // All three break forms, since a clip handling only LF would pass a + // bare CR through to the same surface. + for (label, description, tail) in [ + ("LF", MCP_SHAPED, "Arguments:"), + ( + "CR", + "Greet someone.\\r\\rArguments:\\r name (string, required)", + "Arguments:", + ), + ( + "CRLF", + "Greet someone.\\r\\n\\r\\nArguments:\\r\\n name (string, required)", + "Arguments:", + ), + ] { + let s = session(&format!("gpu-multiline-{label}")); + define_multiline_probe(&s, "zzprobe", description); + let rows = mx_rows(&s); + let probe = rows + .iter() + .find(|row| row.label == "zzprobe") + .unwrap_or_else(|| panic!("{label}: the probe command is a candidate")); + let detail = probe + .detail + .as_deref() + .unwrap_or_else(|| panic!("{label}: the row carries a detail")); + assert_eq!( + detail, "Greet someone.", + "{label}: the wire row carries the first line only" + ); + assert!( + !detail.contains(['\n', '\r']), + "{label}: a row detail must carry no line break: {detail:?}" + ); + assert!( + !detail.contains(tail), + "{label}: the schema block must not reach the dropdown" + ); + + // And the full text is still there for the discoverability + // path, which is what makes this a rendering decision. + let full: String = eval(&s, "return pmacs.describe.command('zzprobe').description"); + assert!( + full.contains("name (string, required)"), + "{label}: describe-command must still report every line: {full:?}" + ); + } +} + +#[test] +fn a_single_line_description_is_unchanged_on_the_wire() { + // The clip did not tighten past its purpose: a description with no + // break reaches the row byte-identical, with no truncation marker. + let s = session("wire-single-line"); + define_probe(&s); + let rows = mx_rows(&s); + let probe = rows + .iter() + .find(|row| row.label == "zzprobe") + .expect("the probe command is a candidate"); + assert_eq!(probe.detail.as_deref(), Some(PROBE_DESCRIPTION)); +} + #[test] fn typed_but_unmatched_input_is_still_accepted() { // Q#D2-5, the trap this lane arrives with: richer rows make `M-x`