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`