diff --git a/COHERENCE.md b/COHERENCE.md index e9cb970..c12acfc 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -570,9 +570,11 @@ descriptions, indexed by `M-x help`. It needed **no Rust** — the data was all reachable from Lua, and even the settings completion source is a Lua function through `CompletionSource::Custom`. -**What is still missing** is itemized below and unchanged by that stage: -`Command` has no title/category/aliases/flags/arg-schema; the predicate -is still never evaluated; M-x rows are still bare name strings; the Rust +**What is still missing** is itemized below. Discovery Stage 2 took one +item — **M-x rows now carry each command's description** — and the rest +is unchanged by both stages: `Command` has no +title/category/aliases/flags/arg-schema; the predicate +is still never evaluated; the Rust help layer is still orphaned (Stage 1 funnels every command through one Lua seam so the eventual migration is enumerated per subject rather than per call site); **packages** have no discovery surface, and workers, @@ -621,12 +623,21 @@ the sharpest instance of §1.1.** M-x filtering, or the menu. The doc comment's claim that "the command palette (T M2.7) uses it to gray out unavailable entries" describes something that never shipped. -- **M-x shows bare name strings.** `CompletionSource::Commands` returns - `Vec` of names; the wire type `MinibufferPrompt.candidates` - is `Vec` (`pmacs-protocol/src/message.rs:994-1006`). No - description, no keybinding, no category alongside candidates — while - `CompletionPopupRow` (`:1231`) already carries `kind` and `detail`, - proving richer rows are a solved wire problem in this codebase. +- **M-x rows carry a description — Discovery Stage 2 (protocol v23).** + `CompletionSource::Commands` still returns `Vec` of names, but + the row the user reads is no longer one. The GPU receives + `InstanceMessage::MinibufferPromptRows` — `MinibufferRow { label, + detail }`, a new type rather than a borrowed `CompletionPopupRow`, + whose `kind` is an LSP code with no honest value for a command — and + the grid TUI renders `[name — description]` inline from the registry + **in-process**, since `src/editor.rs` never consumed the wire variant + at all. The bump is additive: `MinibufferPrompt` is FROZEN and still + sent to every `12..=22` peer (postcard is positional, so widening it + would mis-decode there rather than be ignored), and exactly one of the + two variants reaches any peer. + **What is still missing here:** no keybinding and no category + alongside the candidate — those wait on `Command` gaining the fields + at all. - **The entire Rust help layer is orphaned** (§1.1). Consequence: two parallel `*help*` implementations exist — `help.rs`'s cross-referenced renderer and the Lua `show_help_text` in @@ -684,6 +695,8 @@ the sharpest instance of §1.1.** provenance in the config registry, (c) a dozen interactive commands and richer M-x candidate rows over introspection that **already exists**. This is the highest payoff-per-effort concern in the document. +*(c) is done: Stage 1 shipped the command family, Stage 2 the richer +rows. (a) and (b) remain.* --- diff --git a/docs/active-work.md b/docs/active-work.md index 59783a1..ad3851a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,6 +265,128 @@ also removed: this branch's "R8 NEEDS A LANE" investigation block, and durable facts are in the retired registry row and the handoff §6 census. +## Discovery Stage 2 — PR #228 OPEN, **MERGE-BLOCKED** + +**PR #228** — https://github.com/levineuwirth/pmacs/pull/228. Opened +2026-08-09 at `2d298dd`. **Open for review, not for merge.** + +**The block is a gate-integrity problem, not backlog hygiene.** This +lane's gate is `scripts/gate --protocol`, which promises the CRDT +workspace sweep. That sweep's documented precondition is +`cargo build --workspace --no-default-features --features luajit,crdt` +(handoff §5), and **the script does not run it** — confirmed by reading +its plan emitter. On a fresh per-worktree target directory the sweep +fails on twelve `gpu_invocation_acceptance` tests missing the +`pmacs-gpu` binary, so a `--protocol` result can be decided by the +state of the build directory rather than by the diff. + +Latent until #225 gave each worktree its own target dir — a shared one +usually already had `pmacs-gpu` built, satisfying the precondition by +accident. It surfaced on this branch's first gate run. + +**Unblocking requires both:** the `scripts/gate` repair, in its own +narrow framing and its own PR (explicitly **not** folded into this +feature branch), and then a **fresh-target rerun of this branch's +protocol gate** under the repaired script. + +**Written with the lane's first commit**, per the standing correction +from #171 and #215. + +**Branch `discovery-stage2`**, base `githubsucks/main` @ `4bc55e8` +(the #225 merge). **`githubsucks/discovery-stage2` is the authoritative +tip** — the ref, not a SHA. Recover with +`git fetch githubsucks && git checkout discovery-stage2`. + +- **Framing `docs/discovery-stage2-framing.md`, revision 3, APPROVED + 2026-08-09** after three review rounds. Each round found the previous + one reasoning about a mechanism instead of reading it — an in-place + field change that postcard cannot make compatible, a TUI that never + reads the message at all, a round-trip test that freezes nothing, a + cache hazard the per-peer render state makes impossible, and a + clipping rule unachievable at narrow widths. + Scope: `COHERENCE.md` §5's "M-x rows are still bare names". + Descriptions already exist on `Command` and are already rendered by + `help.list-commands`; they are missing at the one moment they would + change a decision. +- **PROTOCOL BUMP v22 → v23, and this lane HOLDS THE BUMP SLOT.** + Additive: a new `MinibufferPromptRows` variant **appended** to the + enum, with `MinibufferPrompt` **frozen** for v12–v22. An in-place + field change is a wire break — postcard encodes positionally, and + that variant is sent to every peer `>= 12` (`src/daemon.rs:1472`). +- **Git Stage 2 (gutter markers) also needs a bump and must wait for + this to land.** Git Stage 1 is no-wire and runs beside it. +- **Two halves, only one of which is wire work.** `pmacs-gpu` renders + the new variant. **The grid TUI never reads `MinibufferPrompt` at + all** — it paints from `core.minibuffer` and renders + `format!(" [{cand}]")` (`src/editor.rs:5484`), so its half is a + 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 --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 + fields of `MinibufferPrompt` leaves + `minibuffer_prompt_round_trips_through_postcard` **passing** and fails + the fixture, which is exactly the hazard a round-trip cannot see. +- **Version assertions updated (five, each read before editing):** + `src/protocol.rs` — the `PROTOCOL_VERSION == 22` tripwire (renamed + `protocol_version_is_twenty_three_for_minibuffer_prompt_rows`) and + `supported_protocol_versions_resume_ladder_on_v6_floor`'s + accepted/rejected ranges; `tests/statusline_segments_acceptance.rs` + (version + supported range + the `!supported` ceiling); + `tests/bottom_panel_stage2b_gpu_acceptance.rs`; + `tests/vterm_stage3_acceptance.rs`. **No `ADVERTISED_PROTOCOL_VERSION` + assertion fired**, which is the pin doing its job. +- **No cross-version cache test, deliberately** (framing §3.2/§6): + `SemanticRenderState::for_peer` bakes the negotiated version in at + attach and is dropped at detach, so a cache cannot span two versions. + A test for an impossible condition passes forever while teaching the + next reader that the hazard is real. + ## LSP LaTeX coverage — IMPLEMENTED, gates green, no PR yet **Written with the lane's first commit**, per the standing correction @@ -603,6 +725,7 @@ authoritative tip** — the ref, not a SHA. Recover with — added in the second round — a **rename of either** the build or the sweep step each fail the suite. ||||||| parent of 72bbb96 (docs: LSP LaTeX coverage framing revision 2, on a branch at last) +||||||| parent of 312ec7a (docs: frame Discovery Stage 2 (revision 2) — M-x rows) ## QoL arc retirement — PR #224 OPEN (docs only) diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 65e7374..ca53c70 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -2598,11 +2598,30 @@ cannot advertise 21 without stranding existing v20 clients before `AttachRequest`. v15 = `CompletionPopup` + `StatusFacts.message`; v16 = `ThemeFacts`; v17 = `FontFacts`; v18 = `StatuslineSegments`; v19 = the vterm terminal family; v20 = semantic `SessionBootstrapRequest` plus appended -`InitialTargetResult`; v21 reserves the panel frame/event family. New wire +`InitialTargetResult`; v21 reserves the panel frame/event family; +v22 = `LineWrapFacts`; v23 = `MinibufferPromptRows`. New wire surface ⇒ bump + both-frontends support + acceptance. An APPENDED variant must be guarded by a byte pin on the PREVIOUS final variant — its own round-trip cannot detect a discriminant shift. +**A SUPERSEDED variant can be frozen rather than widened, and v23 is the +first case.** Discovery Stage 2 needed richer minibuffer rows. +Widening `MinibufferPrompt` in place was not an option — postcard +encodes fields positionally, so every v12–v22 peer would **mis-decode** +the bytes rather than ignore them — and gating the widened form at +`>= 23` would have left those peers with **no minibuffer message at +all**, because there would have been only one variant to gate. +Compatibility requires the old shape to still exist *and still be sent*. +So `MinibufferPrompt` is retained unchanged for `12..=22`, +`MinibufferPromptRows` is appended for `>= 23`, and the daemon gate is a +**range on both sides** so exactly one variant reaches any peer. +Two consequences worth carrying forward: a frozen variant needs a +**literal byte fixture** (`assert_eq!(encoded, LEGACY_BYTES)`), because a +round-trip encodes and decodes with the same types and so freezes +nothing; and the CLOSE message must use the same variant family as the +OPEN, or a session closed by the other family's clear leaves its surface +on screen forever. + **Fake LSP** (`src/bin/pmacs_fake_lsp.rs`) modes: `fullonly`, `rangeonly`, `rangeonly16` (UTF-16 + fail-closed bounds validation), `sighelp`. Use these for capability-matrix tests, not real servers. diff --git a/docs/discovery-stage2-framing.md b/docs/discovery-stage2-framing.md new file mode 100644 index 0000000..56da2bf --- /dev/null +++ b/docs/discovery-stage2-framing.md @@ -0,0 +1,348 @@ +# Discovery Stage 2 — M-x rows stop being bare names + +**Status: revision 3, APPROVED 2026-08-09. Implementation may +proceed.** + +**Revision 3 fixes three things revision 2 asserted without checking +the mechanism it was reasoning about**: a "frozen-shape" test that +freezes nothing, a cache hazard that this architecture makes +impossible, and a clipping rule that is unachievable at narrow enough +widths. All three verified in the tree. + +**Revision 2 fixes two claims revision 1 made about compatibility and +about the TUI, both wrong, both checkable.** An in-place field change +cannot preserve v22 — postcard is not self-describing — and "both +frontends render it" was false, because the grid TUI never reads that +message at all. Verified in the tree, not reasoned about. + +--- + +## 1. The gap, stated exactly + +`COHERENCE.md` §5 grades unified discoverability **Partial** after +Stage 1 (#207), and names three things left. This lane takes one: + +> `Command` still has no title/category/flags, **M-x rows are still +> bare names**, and the Rust help layer is still orphaned. + +**The descriptions already exist.** `Command.description` is a required +field (`src/command.rs:69`), and `help.list-commands` already renders +"every registered command **with its description**" +(`builtin/runtime/help.lua:339`). A user who runs `M-x help` can read +what everything does. + +**What they cannot do is see it at the moment of choosing.** `M-x` +shows names alone — so the information exists, is already surfaced +elsewhere, and is missing from the one place it would change a +decision. That is §1.1's *substrate without surface* in its purest +form, and it is felt every time the editor is used. + +## 2. Ground truth + +Scouted: + +- **The wire asymmetry is a single field.** + `InstanceMessage::MinibufferPrompt` carries + `candidates: Vec` (`pmacs-protocol/src/message.rs:1113`). +- **The rich pattern is already proven in a sibling variant.** + `CompletionPopup` carries `rows: Vec` — `label`, + `kind: u8`, `detail: Option` (`:1387`) — and both frontends + already render it. + + *(Revision note: an earlier read of mine reported two bare-string + sites. There is one. The second grep hit was `CompletionPopup`'s + doc comment, which says "candidates" while the field is `rows`.)* +- **`Command` needs no change for this lane.** `description` is + already there and already required. Title/category/aliases — the + lane's other Stage-2 candidate — would enrich these rows further and + are **deliberately not** in scope: they are a ~175-site change and + this lane can deliver the felt improvement without them. +- **`ADVERTISED_PROTOCOL_VERSION` is pinned at 20** + (`pmacs-protocol/src/message.rs:1767`) and **must not be edited**, + per handoff §3/§5. +- **The transport is postcard** (`pmacs-protocol/src/transport.rs:1`), + which is **not self-describing**: enum variants encode by index and + fields by position. **Changing a field's type in place is a wire + break**, not a compatible evolution — a v22 peer would mis-decode the + bytes rather than ignore them. +- **`MinibufferPrompt` is sent to every peer negotiated `>= 12`** + (`src/daemon.rs:1472`, "Q#MB1 — MinibufferPrompt gated at v12"). So + the population that would break is every frontend from v12 to v22. +- **The grid TUI never reads `MinibufferPrompt`.** `src/editor.rs` + contains **zero** references to it; `paint_minibuffer` reads + `core.minibuffer` directly and renders the selected candidate as an + inline suffix, `format!(" [{cand}]")` (`src/editor.rs:5484`), with + its own `ui.minibuffer.candidate` face. **The rich wire reaches + `pmacs-gpu` only.** + +## 3. The change + +**This is a protocol change: v22 → v23**, and it is **additive**, not +an edit. + +### 3.1 A new variant, because an in-place change cannot be compatible + +Revision 1 proposed changing `candidates` in place. **That breaks every +frontend from v12 to v22**: postcard encodes fields positionally, so a +v22 peer decoding a `Vec` where it expects +`Vec` mis-reads the bytes — it does not skip them. + +And gating the changed variant at `>= 23` does not rescue it: the peer +would then receive **no minibuffer message at all**, because there is +only one variant to send. Compatibility means *sending the old shape*, +which requires the old shape to still exist. + +So: + +- **`MinibufferPrompt` is retained, unchanged, for v12–v22.** Its + encoding is frozen. +- **`MinibufferPromptRows` is a NEW variant appended to the enum**, + carrying `rows: Vec` and otherwise mirroring + `MinibufferPrompt`'s fields. +- **Appended, not inserted.** Variant indices are positional in + postcard; inserting anywhere but the end renumbers every later + variant and breaks everything at once. + +### 3.2 Per-session selection, and the ordering that matters + +- **Selection is per peer, decided from its negotiated version**: + `>= 23` receives `MinibufferPromptRows`; `12..=22` receives + `MinibufferPrompt`. This mirrors the existing gates in + `src/daemon.rs:1472`, which already suppress `MenuPrompt`, + `MinibufferPrompt` and `LineNumbers` per peer. +- **Exactly one of the two is sent to any given peer, ever.** Sending + both to a v23 peer would double-render; sending neither is the bug + gating alone would have caused. +- **The selection is a producer gate**, named + `peer_knows_minibuffer_rows`, alongside the existing + `peer_knows_minibuffer_prompt` / `peer_knows_menu_prompt` / + `peer_knows_completion_popup` (`src/daemon.rs:1410-1435`). One new + gate in an established pattern, not a new mechanism. +- **ONE per-peer minibuffer cache, not a per-variant key.** + + **Revision 2's rationale for a per-variant key was false**, and the + architecture is why: `SemanticRenderState::for_peer(frontend_id, + negotiated_protocol_version)` is created **per peer, with its version + baked in, on attach** (`src/daemon.rs:2080`) and **removed on + detach** (`:1591`). A cache therefore never spans two negotiated + versions — the v23→v22 reconnect suppression I described **cannot + occur**, because reconnecting creates a fresh state. The + corresponding test is removed rather than written; a test for an + impossible condition passes forever and teaches the next reader that + the hazard is real. +- **The close message must still use the same variant family as the + open** — a `MinibufferPromptRows` session closed by a legacy clear is + the mismatch that leaves a popup on screen forever. That one is + independent of caching and stands. + +### 3.3 What each frontend does + +- **`pmacs-gpu`** renders label + detail from the new variant. +- **The grid TUI does not consume this message at all** and is + addressed separately in §3.4. + +### 3.4 The TUI presentation contract + +Revision 1 said "both frontends render label + detail". **The grid TUI +does not read `MinibufferPrompt`** — it paints from `core.minibuffer` +and renders the selected candidate as `format!(" [{cand}]")` +(`src/editor.rs:5484`). The wire change reaches it not at all. + +*My vote: **an inline selected form, matching what is already there***: + +``` +M-x buffer.sa [buffer.save — Write the buffer to its file] +``` + +- **Source: local.** The TUI is in-process with the core, so it reads + `Command.description` from the registry directly. **No wire + involvement**, which is why this half of the lane is independent of + the bump. +- **Only the selected candidate**, as today. This is a formatting + change to an existing suffix, not a new surface. +- **Clipping, in three ordered steps.** The suffix is already written + against `max = term_size.cols` with a running `written` count, and + the prompt plus typed input consume that budget first — so the + remaining width can be **too small even for the bare name**. + Revision 2 said "the name must survive", which is not achievable at + arbitrary widths and would have forced a partial name. The rule: + + 1. **If the remaining suffix width cannot fit the WHOLE name, omit + the suffix entirely.** Never emit a partial name — `[buffer.sa…]` + is worse than nothing, because it reads as a different command. + 2. **Only once the whole name fits** is a description attempted. + 3. **If the description does not fit whole, drop the description**, + leaving today's `[name]`. No ellipsis stub. + + So the guarantee is *"never a partial name"*, which is achievable, + rather than *"the name always survives"*, which is not. +- **The `ui.minibuffer.candidate` face already exists** and continues + to cover the suffix. + +**A multi-row TUI chooser is explicitly NOT this lane.** It would be a +new interaction surface, a §6 island risk, and materially larger than +the wire work — it is named here so that "make the TUI match the GPU" +does not quietly become that. + +### 3.5 Scheduling consequence, which is not incidental + +`PROTOCOL_VERSION` is a strict serialization point — two lanes bumping +it collide, and this session recorded eight broken version assertions +from a single bump. So: + +- **This lane holds the bump slot.** Git Stage 1 is deliberately + no-wire and runs beside it without contention. +- **Git Stage 2 (gutter markers) also needs a bump and must therefore + wait for this to land.** That ordering should be explicit in the + ledger rather than discovered when the two collide. + +## 4. Coherence impact (§20) + +- **§5 unified discoverability — the direct target**, and the specific + clause "M-x rows are still bare names". +- **Journey step 4** ("understand the interface"): `COHERENCE.md` P4 + says most of it "rides on" discovery. This improves the step without + adding one. +- **§16 semantic frontend:** a clean instance of the architecture — + the instance states *what a candidate is*, each frontend decides how + to draw it. Degradation is the established practice (Q#D2-4). +- **Interaction islands (§6): none added.** No new key interception; + this changes what an existing prompt carries. +- **Config registry:** no new setting. Whether detail rendering is + optional is Q#D2-3, and my vote is no setting at all. +- **Background-work attribution (§9): untouched.** No new background + work. + +## 5. Open questions + +### Q#D2-1 — reuse `CompletionPopupRow`, or a new type? + +Reuse is tempting and I think wrong. `CompletionPopupRow.kind` is an +**LSP `CompletionItemKind` code (1..=25)** with a documented contract; +an M-x command is not an LSP completion item and has no honest value +for that field. Reusing it would mean either inventing a fake kind or +declaring 0/unknown everywhere — a type whose invariant is +"meaningless in half its uses". + +*My vote: **a new `MinibufferRow { label, detail: Option }`*** +— no `kind`. If a category field is wanted later it arrives with +`Command.category` (the other Stage-2 candidate), typed as what it +actually is rather than borrowed from LSP. + +### Q#D2-2 — which prompts get rows? + +`pmacs.minibuffer.read` serves many sources, not just M-x: file paths, +buffer names, apropos substrings, settings. Only some have a natural +`detail`. + +*My vote: **the field is `Option` per row and the daemon fills +it where it has one.*** Commands get their description; a file-path +prompt leaves it `None` and renders exactly as today. No source is +obliged to invent a detail, and none is prevented from gaining one +later. + +### Q#D2-3 — is detail rendering configurable? + +*My vote: **no setting.*** §11 grades the registry "partial +(foundation only)"; adding a speculative toggle for a feature nobody +has yet asked to disable is how a registry becomes noise. If somebody +wants it off, that is use evidence and a later one-line addition. + +### Q#D2-4 — older frontends — **RESOLVED, in §3.1–3.2** + +No longer open, and the revision-1 answer was wrong. "Gate the richer +form at `>= 23`" would have **removed the minibuffer entirely** from +every v12–v22 peer, because there would have been only one variant to +gate. Compatibility requires the legacy shape to still exist and still +be sent — hence the additive `MinibufferPromptRows` variant, a +per-peer `peer_knows_minibuffer_rows` producer gate, **one per-peer +minibuffer cache**, and matched open/close families. + +*(Revision 2 said "per-variant cache keys" here. §3.2 corrected that in +revision 3 — the render state is per peer with its version baked in, so +a cache cannot span two versions — and this sentence was left stale.)* + +The `CompletionPopup` gate I proposed copying (`daemon-gated >= 15`) +**is** the right precedent for *how to select per peer*; it is not a +precedent for changing a live variant's shape, because that variant was +new when it was gated. + +### Q#D2-5 — does this tempt closed-set acceptance? **(a trap)** + +The discovery lane's own handoff note warns: **completion is +assistance, not validation** — `resolve_accepted_value` returns the +literal typed text when no candidate is selected, so closed-set +acceptance is unbuilt Rust work. + +Richer rows make M-x *look* like a closed set, which invites someone to +make acceptance reject unmatched input. **That is out of scope and +would be a behaviour change**, not a rendering one. Stated here because +the temptation arrives with the feature. + +## 6. Verification + +- **A command's description reaches the GPU row**, asserted through + the real prompt path rather than by constructing a message. +- **A v22 peer still receives `MinibufferPrompt`, with its old + encoding** — the case revision 1 would have broken. Asserted by + negotiating v22 and observing the legacy variant arrive, **not** by + observing "no error". +- **A v23 peer receives `MinibufferPromptRows` and NOT the legacy + variant** — the double-render guard. +- **LITERAL POSTCARD BYTE FIXTURES for the legacy variant**, open and + clear: `assert_eq!(encoded, LEGACY_BYTES)` against a constant. + + **Revision 2 proposed a round-trip and that freezes nothing.** A + round-trip encodes and decodes with the *same* types, so adding a + field to `MinibufferPrompt` leaves it passing — both sides simply + learn the new shape, while every v12–v22 peer in the field breaks. + The existing `minibuffer_prompt_round_trips_through_postcard` + (`src/protocol.rs:2363`) is exactly that kind of test, and **there + are no literal byte fixtures anywhere in the protocol tests today** — + checked, not assumed. + + Only comparing against bytes captured *now* can fail when the + encoding changes. Two fixtures: an open prompt with candidates and a + selection, and a cleared band — the two shapes the existing semantic + test already covers, so the corpus is not a new judgement call. +- **No cross-version cache test.** Revision 2 required one; it asserts + a condition this architecture makes impossible (§3.2), and a test + that cannot fail passes forever while teaching the next reader that + the hazard is real. What *is* asserted is the producer gate: a v22 + peer and a v23 peer attached simultaneously each receive their own + variant and only their own. +- **Close matches open**: a `MinibufferPromptRows` session is closed by + its own family, witnessed by the popup actually clearing. +- **The TUI renders `name — description` for the selected candidate** + (§3.4), from the local registry, with **no wire involvement**. +- **TUI clipping is witnessed at THREE widths** (§3.4): wide enough + for name + description; wide enough for the name only (description + dropped, `[name]` as today); and **too narrow for even the whole + name — the suffix vanishes entirely**. The last is the case revision + 2's rule could not express, and the assertion is that no *prefix* of + a name is ever emitted. +- **A source with no detail renders exactly as before** — the + file-path prompt is the witness (Q#D2-2). +- **Typed-but-unmatched input is still accepted** (Q#D2-5) — the + guard against this lane quietly becoming a validation change. +- **The version-bump discipline**: `ADVERTISED_PROTOCOL_VERSION` + unchanged at 20, and the tripwire assertions updated **knowingly**. + Handoff §3 requires the strengthened two-configuration sweep for a + `PROTOCOL_VERSION` change — `scripts/gate --protocol`, which exists + precisely for this. + +**What this will not prove:** that `Command` carries title or category +(not in scope), or that predicates are evaluated (Stage 3+). + +## 7. Not in scope + +`Command` gaining title/category/aliases/flags/arg-schema — the +~175-site change, and the lane's next candidate. **A multi-row TUI +chooser** (§3.4) — a new interaction surface and materially larger than +this lane. **Changing `MinibufferPrompt`'s existing shape** — it is +frozen for v12–v22. Predicate evaluation, +which makes commands stop being invocable and needs its own decision at +each call site. Help-layer unification (`src/help.rs` is still +orphaned). The help prefix key — `C-h` is **not** free, since non-kitty +terminals cannot disambiguate Ctrl+Backspace from Ctrl+H (both are +byte 0x08). Closed-set acceptance (Q#D2-5). diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 52a2a29..78eb64a 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -44,9 +44,10 @@ use pmacs_protocol::{ CompletionPopupRow, CrdtOp, Decoration, DecorationKind, DecorationSegment, FrontendId, InlineAdornment, InstanceMessage, InstanceSignal, Key as ProtocolKey, LineNumberMode, MAX_STATUSLINE_FACE_BYTES, MAX_STATUSLINE_PROVIDERS, MAX_STATUSLINE_SEGMENT_BYTES, - MAX_STATUSLINE_TOTAL_TEXT_BYTES, MenuPromptRow, Modifiers, MouseButton as ProtocolMouseButton, - MouseKind as ProtocolMouseKind, PointerKind, SelectionSnapshot, StatuslineSegment, - StyleSegment, StyleSpan, TAB_STOP_COLUMNS, TerminalFrame, UnderlineStyle, + MAX_STATUSLINE_TOTAL_TEXT_BYTES, MenuPromptRow, MinibufferRow, Modifiers, + MouseButton as ProtocolMouseButton, MouseKind as ProtocolMouseKind, PointerKind, + SelectionSnapshot, StatuslineSegment, StyleSegment, StyleSpan, TAB_STOP_COLUMNS, TerminalFrame, + UnderlineStyle, cell::{Color as CellColor, Style as CellStyle}, is_builtin_pair_char, is_modeline_face_name, panel::{PANEL_MIN_VERSION, PanelFrame, PanelFramePayload}, @@ -2259,16 +2260,25 @@ struct SearchPromptLocal { invalid: bool, } -/// The live minibuffer (Q#MB1, protocol v12), mirrored from a -/// `MinibufferPrompt` whose `prompt` was `Some`. The prompt+input draw -/// in the bottom band with a caret; `candidates` (a windowed slice) feed -/// the dropdown. +/// The live minibuffer (Q#MB1, protocol v12), mirrored from whichever +/// minibuffer variant this session's negotiated version carries, when +/// its `prompt` was `Some`. The prompt+input draw in the bottom band +/// with a caret; `rows` (a windowed slice) feed the dropdown. +/// +/// **One local shape for two wire variants.** A `>= 23` daemon sends +/// `MinibufferPromptRows` with per-row details; a `12..=22` daemon sends +/// the frozen `MinibufferPrompt` with bare strings, which land here as +/// rows whose `detail` is `None`. Both are live: this binary offers its +/// own `PROTOCOL_VERSION` only when the daemon advertises the current +/// baseline, and echoes an older baseline verbatim — so an older daemon +/// still negotiates an older session, and the legacy arm is reachable +/// rather than dead code. #[derive(Clone, Debug, PartialEq)] struct MinibufferLocal { prompt: String, input: String, cursor: u32, - candidates: Vec, + rows: Vec, selected: Option, total: u32, } @@ -5085,7 +5095,10 @@ impl State { None } // Q#MB1 — the minibuffer prompt/input/candidates. `prompt: - // None` closes it. + // None` closes it. This is the FROZEN legacy variant, which + // only a `12..=22` daemon sends; its candidates carry no + // detail, so they become rows with `detail: None` and render + // exactly as they did before v23. InstanceMessage::MinibufferPrompt { prompt, input, @@ -5098,7 +5111,38 @@ impl State { prompt, input, cursor, - candidates, + rows: candidates + .into_iter() + .map(|label| MinibufferRow { + label, + detail: None, + }) + .collect(), + selected, + total, + }); + self.request_redraw(); + None + } + // Discovery Stage 2 — the v23 rows form of the same surface, + // carrying an optional per-row detail (a command's + // description). `prompt: None` closes it, and the close + // arrives in THIS family because the daemon picks the family + // per peer: a rows session closed by a legacy clear would + // leave the dropdown on screen forever. + InstanceMessage::MinibufferPromptRows { + prompt, + input, + cursor, + rows, + selected, + total, + } => { + self.minibuffer = prompt.map(|prompt| MinibufferLocal { + prompt, + input, + cursor, + rows, selected, total, }); @@ -7619,11 +7663,22 @@ impl State { /// Re-shape the minibuffer dropdown candidates (Q#MB1), one line per /// candidate, best match first. Empty when there are no candidates. + /// + /// Discovery Stage 2: a row with a `detail` renders `label detail`, + /// the same two-space form the completion dropdown already uses. A + /// row without one renders the bare label, so a file-path or + /// buffer-name prompt looks exactly as it did before v23. fn refresh_mb_buffer(&mut self) { - let text = self - .minibuffer - .as_ref() - .map_or_else(String::new, |mb| mb.candidates.join("\n")); + let text = self.minibuffer.as_ref().map_or_else(String::new, |mb| { + mb.rows + .iter() + .map(|row| match row.detail.as_deref() { + Some(detail) => format!("{} {detail}", row.label), + None => row.label.clone(), + }) + .collect::>() + .join("\n") + }); let family = self.resolved_family.clone(); self.mb_buffer.set_text( &mut self.font_system, @@ -7644,7 +7699,7 @@ impl State { let mb = self.minibuffer.as_ref()?; let band_top = status_band_top(self.config.height, self.fm); mb_dropdown_window( - mb.candidates.len(), + mb.rows.len(), mb.selected.map_or(0, |s| s as usize), band_top, self.fm, @@ -10868,6 +10923,7 @@ fn instance_message_label(msg: &InstanceMessage) -> &'static str { InstanceMessage::SearchPrompt { .. } => "SearchPrompt", InstanceMessage::MenuPrompt { .. } => "MenuPrompt", InstanceMessage::MinibufferPrompt { .. } => "MinibufferPrompt", + InstanceMessage::MinibufferPromptRows { .. } => "MinibufferPromptRows", InstanceMessage::BlockAdornments { .. } => "BlockAdornments", InstanceMessage::FoldState { .. } => "FoldState", InstanceMessage::ResourceOffer { .. } => "ResourceOffer", @@ -13766,6 +13822,22 @@ mod tests { // They skip (not fail) when no wgpu adapter is available — a dev box // without working Vulkan, or CI without lavapipe. + /// Detail-free minibuffer rows from bare labels — what a `12..=22` + /// daemon's frozen `MinibufferPrompt` lands as. + fn detailless_rows(labels: I) -> Vec + where + I: IntoIterator, + S: Into, + { + labels + .into_iter() + .map(|label| MinibufferRow { + label: label.into(), + detail: None, + }) + .collect() + } + /// Build a headless `State`, or return `None` and log when there's no /// adapter so the caller can skip. When `PMACS_REQUIRE_GPU` is set /// (CI, where lavapipe is installed) a missing adapter is a hard @@ -14888,7 +14960,7 @@ mod tests { prompt: "M-x ".to_owned(), input: "find".to_owned(), cursor: 4, - candidates: Vec::new(), + rows: Vec::new(), selected: None, total: 0, }); @@ -15291,7 +15363,7 @@ mod tests { prompt: "M-x ".into(), input: "theme".into(), cursor: 5, - candidates: Vec::new(), + rows: Vec::new(), selected: None, total: 0, }); @@ -15335,7 +15407,7 @@ mod tests { prompt: "M-x ".into(), input: "the".into(), cursor: 3, - candidates: vec!["theme-set".into(), "theme-clear".into()], + rows: detailless_rows(["theme-set", "theme-clear"]), selected: Some(0), total: 2, }); @@ -15357,6 +15429,112 @@ mod tests { ); } + /// Discovery Stage 2: a row's `detail` reaches the shaped dropdown + /// line, and BOTH wire families land in the same local shape. + /// + /// Driven through `apply_attach_message` rather than by assigning + /// `state.minibuffer` — the mapping from wire variant to local row + /// is exactly what this asserts, so constructing the local value + /// would skip the thing under test. The shaped `layout_runs()` text + /// is what glyphon rasterizes, so a description present there is a + /// description on screen. + #[test] + fn a_minibuffer_row_detail_reaches_the_shaped_dropdown_line() { + let Some(mut state) = headless_or_skip(600, 400, "hello") else { + return; + }; + + // The v23 rows form: a row with a detail, and a row without. + let _ = state.apply_attach_message(InstanceMessage::MinibufferPromptRows { + prompt: Some("M-x ".into()), + input: "buf".into(), + cursor: 3, + rows: vec![ + MinibufferRow { + label: "buffer.save".into(), + detail: Some("Write the buffer to its file".into()), + }, + MinibufferRow { + label: "buffer.kill".into(), + detail: None, + }, + ], + selected: Some(0), + total: 2, + }); + state.refresh_mb_buffer(); + let lines: Vec = state + .mb_buffer + .layout_runs() + .map(|run| run.text.to_owned()) + .collect(); + assert!( + lines + .iter() + .any(|l| l.contains("buffer.save") && l.contains("Write the buffer to its file")), + "the detail must be shaped into the row: {lines:?}" + ); + assert_eq!( + lines + .iter() + .find(|l| l.contains("buffer.kill")) + .map(String::as_str), + 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. + let _ = state.apply_attach_message(InstanceMessage::MinibufferPrompt { + prompt: Some("M-x ".into()), + input: "buf".into(), + cursor: 3, + candidates: vec!["buffer.save".into()], + selected: Some(0), + total: 1, + }); + assert_eq!( + state.minibuffer.as_ref().map(|mb| mb.rows.clone()), + Some(vec![MinibufferRow { + label: "buffer.save".into(), + detail: None, + }]), + "the legacy variant lands as a detail-free row" + ); + state.refresh_mb_buffer(); + let legacy: Vec = state + .mb_buffer + .layout_runs() + .map(|run| run.text.to_owned()) + .collect(); + assert_eq!(legacy, vec!["buffer.save".to_owned()]); + + // Either family closes the surface with `prompt: None`. + let _ = state.apply_attach_message(InstanceMessage::MinibufferPromptRows { + prompt: None, + input: String::new(), + cursor: 0, + rows: Vec::new(), + selected: None, + total: 0, + }); + assert!( + state.minibuffer.is_none(), + "a rows clear closes the surface" + ); + } + #[test] fn headless_diag_face_recolors_band_counter_despite_unchanged_text() { // Acceptance 22 — the round-1 finding-3 bite. The E: counter @@ -15954,7 +16132,7 @@ mod tests { prompt: "P: ".into(), input: String::new(), cursor: 0, - candidates: vec![long.clone(), long.clone()], + rows: detailless_rows([long.clone(), long.clone()]), selected: Some(1), total: 2, }); @@ -16172,7 +16350,7 @@ mod tests { prompt: "M-x ".into(), input: String::new(), cursor: 0, - candidates: (0..30).map(|i| format!("candidate-{i}")).collect(), + rows: detailless_rows((0..30).map(|i| format!("candidate-{i}"))), selected: Some(1), total: 30, }); @@ -17095,7 +17273,7 @@ mod tests { prompt: ":".into(), input: String::new(), cursor: 0, - candidates: Vec::new(), + rows: Vec::new(), selected: None, total: 0, }); diff --git a/pmacs-protocol/src/lib.rs b/pmacs-protocol/src/lib.rs index 9a0a2dc..6976a8c 100644 --- a/pmacs-protocol/src/lib.rs +++ b/pmacs-protocol/src/lib.rs @@ -65,11 +65,12 @@ pub use message::{ InstanceMessage, InstanceSignal, Key, KeyEvent, LineNumberMode, MAX_INITIAL_TARGET_ERROR_BYTES, MAX_INITIAL_TARGET_PATH_BYTES, MAX_STATUSLINE_FACE_BYTES, MAX_STATUSLINE_PROVIDER_NAME_BYTES, MAX_STATUSLINE_PROVIDERS, MAX_STATUSLINE_SEGMENT_BYTES, MAX_STATUSLINE_TOTAL_TEXT_BYTES, - MenuPromptRow, Modifiers, MouseButton, MouseEvent, MouseKind, NegotiatedCapabilities, - PROTOCOL_VERSION, PointerKind, ResourceBody, SUPPORTED_PROTOCOL_VERSIONS, SelectionSnapshot, - SessionBootstrapRequest, StatuslineSegment, StyleSegment, StyleSpan, ThemeFace, - is_builtin_pair_char, is_modeline_face_name, is_supported_protocol_version, is_ui_face_name, - negotiate_capabilities, negotiated_session_version, requested_protocol_version, + MenuPromptRow, MinibufferRow, Modifiers, MouseButton, MouseEvent, MouseKind, + NegotiatedCapabilities, PROTOCOL_VERSION, PointerKind, ResourceBody, + SUPPORTED_PROTOCOL_VERSIONS, SelectionSnapshot, SessionBootstrapRequest, StatuslineSegment, + StyleSegment, StyleSpan, ThemeFace, is_builtin_pair_char, is_modeline_face_name, + is_supported_protocol_version, is_ui_face_name, negotiate_capabilities, + negotiated_session_version, requested_protocol_version, }; pub use panel::{ MAX_PANEL_VISIBLE_CELLS, PANEL_MIN_VERSION, PanelFrame, PanelFrameError, PanelFramePayload, diff --git a/pmacs-protocol/src/message.rs b/pmacs-protocol/src/message.rs index b4c8e7e..8516d4d 100644 --- a/pmacs-protocol/src/message.rs +++ b/pmacs-protocol/src/message.rs @@ -1098,7 +1098,24 @@ pub enum InstanceMessage { /// v12). The minibuffer is a single *global* core instance, so this /// is bufferless; the producer still emits it from the active-buffer /// viewport. `prompt: None` clears the GUI. Cached-compare - /// suppressed like `SearchPrompt`; daemon-gated `>= 12`. + /// suppressed like `SearchPrompt`; daemon-gated `12..=22`. + /// + /// # FROZEN — this variant's encoding must not move + /// + /// Discovery Stage 2 (v23) needed richer rows, and postcard is not + /// self-describing: enum variants encode by index and fields by + /// position, so widening `candidates` in place would make every + /// v12–v22 peer **mis-decode** these bytes rather than ignore them. + /// Gating the widened shape at `>= 23` would not rescue them either + /// — with only one variant to send, they would receive no minibuffer + /// message at all. So the rich form went into a new appended + /// variant, [`Self::MinibufferPromptRows`], and this one is retained + /// unchanged as what a `12..=22` peer receives. + /// + /// Its bytes are pinned literally by + /// `minibuffer_prompt_v12_wire_bytes_are_frozen` in + /// `src/protocol.rs` — a round-trip cannot detect a field addition, + /// because both sides simply learn the new shape. MinibufferPrompt { /// The prompt string (e.g. `"M-x "`), or `None` when no /// minibuffer is open. @@ -1299,6 +1316,59 @@ pub enum InstanceMessage { /// Whether that buffer's long lines wrap. wrap: bool, }, + /// Discovery Stage 2 (protocol v23): the minibuffer prompt with + /// **structured rows** — a label and an optional one-line detail — + /// instead of bare candidate strings. + /// + /// # Why a second variant rather than a wider `MinibufferPrompt` + /// + /// `Command.description` already exists and is already rendered by + /// `help.list-commands`; it is missing at the one moment it would + /// change a decision, which is the `M-x` row. Carrying it means + /// widening the minibuffer's candidate shape — and postcard encodes + /// fields **positionally**, so changing `candidates: Vec` in + /// place is a wire break, not an evolution: a v22 peer mis-decodes + /// the bytes rather than skipping them. Gating the changed variant + /// at `>= 23` does not rescue it either, because a `12..=22` peer + /// would then receive no minibuffer message at all. Compatibility + /// requires the old shape to still exist *and still be sent*, so + /// [`Self::MinibufferPrompt`] is frozen and this is appended beside + /// it. + /// + /// # Exactly one of the two reaches any peer + /// + /// The producer selects on the session's negotiated version and the + /// daemon's write loop gates both directions: `>= 23` receives this + /// and never the legacy variant; `12..=22` receives the legacy + /// variant and never this. Sending both would double-render; sending + /// neither is the bug gating alone would have caused. The close + /// message must use the same family as the open — a rows session + /// closed by a legacy clear leaves a popup on screen forever. + /// + /// Otherwise this mirrors [`Self::MinibufferPrompt`] exactly: + /// bufferless (one global core minibuffer), `prompt: None` clears + /// the GUI, cached-compare suppressed, emitted from the + /// active-buffer viewport. + /// + /// Appended after [`Self::LineWrapFacts`], the final v22 variant, so + /// no existing postcard discriminant moves. + MinibufferPromptRows { + /// The prompt string (e.g. `"M-x "`), or `None` when no + /// minibuffer is open. + prompt: Option, + /// The text typed so far. + input: String, + /// Codepoints before the cursor within `input` (the caret + /// position). + cursor: u32, + /// A windowed slice of the completion candidates (best-first, + /// already filtered/sorted by the core), `<= MB_VISIBLE`. + rows: Vec, + /// Highlighted row *within* `rows`, or `None`. + selected: Option, + /// Total candidate count (the window is a slice of this). + total: u32, + }, } /// One resolved UI face for [`InstanceMessage::ThemeFacts`]: a full @@ -1394,6 +1464,35 @@ pub struct CompletionPopupRow { pub detail: Option, } +/// One row of the minibuffer's candidate list on the wire +/// ([`InstanceMessage::MinibufferPromptRows`], Discovery Stage 2, +/// protocol v23). +/// +/// # Why this is not `CompletionPopupRow` +/// +/// Reuse was tempting and is wrong. [`CompletionPopupRow::kind`] is an +/// LSP `CompletionItemKind` code with a documented contract, and an +/// `M-x` command is not an LSP completion item — it has no honest value +/// for that field. Reusing it would mean inventing a fake kind or +/// declaring unknown everywhere: a type whose invariant is "meaningless +/// in half its uses". If a category is wanted later it arrives with +/// `Command.category`, typed as what it actually is rather than +/// borrowed from LSP. +/// +/// `detail` is optional **per row** because `pmacs.minibuffer.read` +/// serves many sources — file paths, buffer names, settings — and only +/// some have a natural detail. A source with none leaves it `None` and +/// renders exactly as it did before v23. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq, Eq)] +pub struct MinibufferRow { + /// Display label — the candidate itself, and the value acceptance + /// resolves to. + pub label: String, + /// Optional one-line detail rendered after the label (a command's + /// description, for `M-x`). + pub detail: Option, +} + /// Flat selection state for the wire. /// /// Mirrors [`crate::window::Selection`] but as a self-contained pair @@ -1731,7 +1830,17 @@ pub enum ResourceBody { /// directions: a v20 peer neither receives `PanelFrame` nor is placed in /// a side window, because denying only the events would leave its /// window invisible. -pub const PROTOCOL_VERSION: u32 = 22; +/// +/// Discovery Stage 2: bumped 22 → 23 for +/// [`InstanceMessage::MinibufferPromptRows`] — the minibuffer's +/// candidate rows gaining an optional per-row detail. Appended after +/// `LineWrapFacts`, the final v22 variant, so no existing discriminant +/// moves; [`InstanceMessage::MinibufferPrompt`] is retained **frozen** +/// and still sent to `12..=22` peers, because postcard's positional +/// encoding makes an in-place widening a wire break rather than an +/// evolution, and gating the widened form would have left those peers +/// with no minibuffer message at all. +pub const PROTOCOL_VERSION: u32 = 23; /// Protocol version placed in the daemon's server-first [`Hello`]. /// @@ -1905,8 +2014,15 @@ pub fn negotiated_session_version(frontend_offer: u32) -> u32 { /// [`ADVERTISED_PROTOCOL_VERSION`] does not move — a v21 frontend /// negotiates v21, never receives the variant, and keeps its own /// behavior. +/// +/// Discovery Stage 2: extended to `[6, ..., 23]` for +/// [`InstanceMessage::MinibufferPromptRows`]. Additive and daemon-gated, +/// and unusually the gate is a **range on both sides**: a `12..=22` peer +/// keeps receiving the frozen [`InstanceMessage::MinibufferPrompt`], a +/// `>= 23` peer receives only the rows form, and no peer ever receives +/// both. [`ADVERTISED_PROTOCOL_VERSION`] does not move. pub const SUPPORTED_PROTOCOL_VERSIONS: &[u32] = &[ - 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, + 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, ]; /// T M10.5: predicate for the handshake check. Returns `true` if 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/daemon.rs b/src/daemon.rs index fff5e21..e5c5a94 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1420,9 +1420,23 @@ fn dispatcher_loop( let peer_knows_menu_prompt = session_registry .session_state(*fid) .is_some_and(|s| s.negotiated_protocol_version >= 11); - let peer_knows_minibuffer_prompt = session_registry - .session_state(*fid) - .is_some_and(|s| s.negotiated_protocol_version >= 12); + // Q#MB1 / Discovery Stage 2 — the minibuffer is the one + // surface with TWO live variants, and the gate is a + // RANGE on both sides rather than a floor. The legacy + // `MinibufferPrompt` is frozen and belongs to `12..=22`; + // `MinibufferPromptRows` belongs to `>= 23`. Writing the + // legacy gate as a bare `>= 12` would let a v23 peer + // receive both and double-render its dropdown. + let peer_knows_minibuffer_prompt = + session_registry.session_state(*fid).is_some_and(|s| { + (12..crate::semantic_render::MINIBUFFER_ROWS_MIN_VERSION) + .contains(&s.negotiated_protocol_version) + }); + let peer_knows_minibuffer_rows = + session_registry.session_state(*fid).is_some_and(|s| { + s.negotiated_protocol_version + >= crate::semantic_render::MINIBUFFER_ROWS_MIN_VERSION + }); // UX gutter — `LineNumbers` carries a `LineNumberMode` since // v14 (was `enabled: bool` in v13); a peer below 14 keeps // its gutter off rather than mis-decoding the wider shape. @@ -1470,12 +1484,24 @@ fn dispatcher_loop( continue; } // Q#MB1 — MinibufferPrompt gated at v12; a v11 peer - // simply can't render the GUI minibuffer. + // simply can't render the GUI minibuffer. Discovery + // Stage 2 closed the range at the top: a v23 peer + // gets the rows form instead, never both. if !peer_knows_minibuffer_prompt && matches!(msg, InstanceMessage::MinibufferPrompt { .. }) { continue; } + // Discovery Stage 2 — MinibufferPromptRows gated at + // v23. A `12..=22` peer keeps the frozen legacy + // variant above, which is why gating alone was never + // enough: with one variant it would have lost the + // minibuffer entirely. + if !peer_knows_minibuffer_rows + && matches!(msg, InstanceMessage::MinibufferPromptRows { .. }) + { + continue; + } if !peer_knows_line_numbers && matches!(msg, InstanceMessage::LineNumbers { .. }) { diff --git a/src/editor.rs b/src/editor.rs index 8d80a6c..0e78fe8 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -4732,7 +4732,10 @@ pub fn paint_frame( paint_search_prompt(grid, core, term_size, &theme); None } else if core.minibuffer.is_active() { - Some(paint_minibuffer(grid, core, term_size, &theme)) + // The command registry is a separate `RefCell` from the core, so + // this borrow does not contend with the one held above. + let commands = state.lua_host.commands().borrow(); + Some(paint_minibuffer(grid, core, &commands, term_size, &theme)) } else { None }; @@ -5464,9 +5467,42 @@ fn minibuffer_style(theme: &crate::highlight::Theme) -> crate::cell::Style { }) } +/// The inline candidate suffix for the minibuffer's bottom row, given +/// the columns still free after the prompt and the typed input. +/// +/// Discovery Stage 2 §3.4 — three ORDERED steps, and the guarantee is +/// **"never a partial name"**, not "the name always survives". The +/// latter is unachievable: the prompt and the typed input consume the +/// budget first, so the remainder can be too small even for the bare +/// name. +/// +/// 1. If the whole name does not fit, emit **nothing**. A truncated +/// `[buffer.sa…]` is worse than no suffix, because it reads as a +/// different command. +/// 2. Only once the whole name fits is a description attempted. +/// 3. If the description does not fit whole, drop it — leaving exactly +/// today's `[name]`. No ellipsis stub. +/// +/// Measured in `char`s, matching the painter below: it writes one cell +/// per `char`. +fn minibuffer_candidate_suffix(name: &str, detail: Option<&str>, remaining: u32) -> String { + let bare = format!(" [{name}]"); + if bare.chars().count() as u32 > remaining { + return String::new(); + } + if let Some(detail) = detail.map(str::trim).filter(|d| !d.is_empty()) { + let full = format!(" [{name} — {detail}]"); + if full.chars().count() as u32 <= remaining { + return full; + } + } + bare +} + fn paint_minibuffer( grid: &mut crate::cell::CellGrid<'_>, core: &EditorCore, + commands: &crate::command::CommandRegistry, term_size: crate::cell::CellSize, theme: &crate::highlight::Theme, ) -> u32 { @@ -5477,12 +5513,6 @@ fn paint_minibuffer( .expect("called only when active"); let prompt = &session.prompt; let contents = core.minibuffer.contents(); - let mut suffix = String::new(); - if let Some(idx) = session.selected - && let Some(cand) = session.candidates.get(idx) - { - suffix = format!(" [{cand}]"); - } let row = term_size.rows - 1; let mut col: u32 = 0; let mut written: u32 = 0; @@ -5537,6 +5567,35 @@ fn paint_minibuffer( cursor_col = prompt_end; } + // Discovery Stage 2 (§3.4): the selected candidate's suffix now + // carries the command's DESCRIPTION, read from the registry + // in-process. The grid TUI never consumes `MinibufferPrompt` — it + // paints from `core.minibuffer` — so this half of the lane involves + // no wire at all and is independent of the v23 bump. + // + // 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(crate::command::Command::description_first_line) + }) + .flatten(); + minibuffer_candidate_suffix(cand, detail, max.saturating_sub(col)) + } + None => String::new(), + }; for ch in suffix.chars() { if col >= max { break; diff --git a/src/frontend.rs b/src/frontend.rs index 1d04e9f..60d7449 100644 --- a/src/frontend.rs +++ b/src/frontend.rs @@ -406,6 +406,10 @@ impl Frontend { // surface; the TUI paints the minibuffer via its own bottom // row, so it drops this silently too. | InstanceMessage::MinibufferPrompt { .. } + // Discovery Stage 2 — the v23 rows form of the same surface. + // The TUI reads `Command.description` from the registry + // in-process instead, so this reaches it not at all. + | InstanceMessage::MinibufferPromptRows { .. } // UX gutter — LineNumbers is the semantic-frontend gutter // toggle; the cell-grid TUI reads its window's mode directly, // so it drops this silently like the other semantic families. diff --git a/src/protocol.rs b/src/protocol.rs index 1a1c723..33beee0 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -1683,7 +1683,7 @@ mod tests { // --- M5.5a handshake & postcard round-trips --- #[test] - fn protocol_version_is_twenty_two_for_line_wrap_facts() { + fn protocol_version_is_twenty_three_for_minibuffer_prompt_rows() { // Pin the value: T M10.5 bumped 1→2 (v1.0 wire: CrdtOp / // PresenceUpdate). T M11.1 bumped 2→3 (v1.1 wire: the // SemanticFrame family + FrontendEvent::Viewport). T M11.6 @@ -1732,7 +1732,15 @@ mod tests { // daemon-gated, appended after the final v21 variant). The // GPU lays out locally and would otherwise never hear the wrap // setting; the advertised baseline is deliberately unmoved. - assert_eq!(PROTOCOL_VERSION, 22); + // Discovery Stage 2 bumps 22→23 (`InstanceMessage:: + // MinibufferPromptRows`, daemon-gated, appended after the final + // v22 variant). The first bump to leave the SUPERSEDED variant + // live rather than widening it: postcard is positional, so + // widening `MinibufferPrompt` would break every v12–v22 peer, + // and gating the wider form would have left them with no + // minibuffer at all. `MinibufferPrompt` is therefore frozen and + // pinned by literal bytes below. + assert_eq!(PROTOCOL_VERSION, 23); } #[test] @@ -1809,17 +1817,18 @@ mod tests { // (`CompletionPopup`), v16 (`ThemeFacts`), v17 (`FontFacts`), // v18 (`StatuslineSegments`), v19 (the vterm terminal family), // v20 (semantic initial-target bootstrap), v21 (the bottom - // panel band), and v22 (`LineWrapFacts`) all interoperate. - for accepted in 6..=22 { + // panel band), v22 (`LineWrapFacts`), and v23 + // (`MinibufferPromptRows`) all interoperate. + for accepted in 6..=23 { assert!( is_supported_protocol_version(accepted), "v{accepted} must be accepted" ); } - for rejected in [0, 1, 2, 3, 4, 5, 23, u32::MAX] { + for rejected in [0, 1, 2, 3, 4, 5, 24, u32::MAX] { assert!( !is_supported_protocol_version(rejected), - "v{rejected} must be rejected by a v22 binary" + "v{rejected} must be rejected by a v23 binary" ); } } @@ -2406,6 +2415,130 @@ mod tests { } } + #[test] + fn minibuffer_prompt_v12_wire_bytes_are_frozen() { + // Discovery Stage 2 (v23) froze `MinibufferPrompt` and put the + // richer shape in an appended `MinibufferPromptRows`. THIS is + // what makes the freeze real, and the round-trip above is not: + // a round-trip encodes and decodes with the SAME types, so + // adding a field to `MinibufferPrompt` leaves it passing while + // every v12–v22 peer in the field mis-decodes the bytes. Only a + // comparison against bytes captured now can fail when the + // encoding changes. + // + // Two fixtures, the two shapes the producer emits: an open + // prompt with a windowed candidate list and a selection, and a + // cleared band. Discriminant 20, then the fields positionally + // (postcard is not self-describing). + let open = InstanceMessage::MinibufferPrompt { + prompt: Some("M-x ".to_owned()), + input: "ed".to_owned(), + cursor: 2, + candidates: vec!["edit.copy".to_owned(), "edit.cut".to_owned()], + selected: Some(1), + total: 7, + }; + assert_eq!( + postcard::to_allocvec(&open).expect("encode open"), + [ + 20, // InstanceMessage::MinibufferPrompt + 1, 4, b'M', b'-', b'x', b' ', // prompt: Some("M-x ") + 2, b'e', b'd', // input: "ed" + 2, // cursor + 2, 9, b'e', b'd', b'i', b't', b'.', b'c', b'o', b'p', b'y', 8, b'e', b'd', b'i', + b't', b'.', b'c', b'u', b't', // candidates + 1, 1, // selected: Some(1) + 7, // total + ], + "MinibufferPrompt's v12 wire bytes changed. It is FROZEN for \ + v12..=22 — a widening here mis-decodes on every already-shipped \ + frontend rather than being ignored. Richer minibuffer rows \ + belong in MinibufferPromptRows." + ); + + let clear = InstanceMessage::MinibufferPrompt { + prompt: None, + input: String::new(), + cursor: 0, + candidates: Vec::new(), + selected: None, + total: 0, + }; + assert_eq!( + postcard::to_allocvec(&clear).expect("encode clear"), + [20, 0, 0, 0, 0, 0, 0], + "MinibufferPrompt's cleared-band v12 wire bytes changed — see the \ + open-prompt fixture above" + ); + } + + #[test] + fn line_wrap_facts_encoding_is_unchanged_by_the_v23_build() { + // Discovery Stage 2 placement pin: `MinibufferPromptRows` must + // be APPENDED after `LineWrapFacts` — the final v22 variant, + // whose ordinal moves if anything is inserted before any v22 + // variant. The new variant's own round-trip cannot detect a + // shift, which is why the pin sits on the PREVIOUS final variant + // (handoff §4). + let msg = InstanceMessage::LineWrapFacts { + buffer_id: pmacs_protocol::BufferId::from_raw(4), + wrap: true, + }; + let bytes = postcard::to_allocvec(&msg).expect("encode"); + assert_eq!( + bytes, + [29, 4, 1], + "LineWrapFacts' v22 wire bytes changed — a variant was \ + inserted before it; append new InstanceMessage variants \ + at the end" + ); + } + + #[test] + fn minibuffer_prompt_rows_round_trips_and_appends_after_line_wrap_facts() { + // The v23 variant itself: both shapes, a detail present and a + // detail absent (Q#D2-2 — a source with no detail leaves it + // `None` and renders as it always did), plus the cleared band. + let cases = [ + ( + Some("M-x ".to_owned()), + "ed".to_owned(), + 2u32, + vec![ + MinibufferRow { + label: "edit.copy".to_owned(), + detail: Some("Copy the region".to_owned()), + }, + MinibufferRow { + label: "notes.txt".to_owned(), + detail: None, + }, + ], + Some(1u32), + 7u32, + ), + (None, String::new(), 0, Vec::new(), None, 0), + ]; + for (prompt, input, cursor, rows, selected, total) in cases { + let msg = InstanceMessage::MinibufferPromptRows { + prompt: prompt.clone(), + input: input.clone(), + cursor, + rows: rows.clone(), + selected, + total, + }; + let bytes = postcard::to_allocvec(&msg).expect("encode"); + assert_eq!( + bytes.first(), + Some(&30), + "MinibufferPromptRows must be appended after v22 LineWrapFacts" + ); + let decoded: InstanceMessage = postcard::from_bytes(&bytes).expect("decode"); + assert_eq!(decoded, msg); + } + } + #[test] fn key_event_to_crossterm_round_trips() { // Build a protocol KeyEvent, translate to crossterm, translate diff --git a/src/semantic_render.rs b/src/semantic_render.rs index de9f417..3359504 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -38,7 +38,7 @@ use crate::cell::{CellSize, Style}; use crate::editor::EditorState; use crate::protocol::{ AdornmentContent, AdornmentPlacement, ByteRange, Decoration, DecorationKind, DecorationSegment, - FrontendId, InlineAdornment, InstanceMessage, MenuPromptRow, PANEL_MIN_VERSION, + FrontendId, InlineAdornment, InstanceMessage, MenuPromptRow, MinibufferRow, PANEL_MIN_VERSION, StatuslineSegment, StyleSegment, StyleSpan, }; use crate::statusline::{ @@ -95,10 +95,26 @@ type SearchPromptFacts = (Option, Option, u32, bool, bool); /// menu. type MenuPromptFacts = (Vec, Option); -/// Cached `MinibufferPrompt` payload for cached-compare suppression -/// (Q#MB1): `(prompt, input, cursor, candidates-window, selected, total)`. -/// A `None` prompt means the minibuffer is closed. -type MinibufferFacts = (Option, String, u32, Vec, Option, u32); +/// Cached minibuffer payload for cached-compare suppression (Q#MB1): +/// `(prompt, input, cursor, rows-window, selected, total)`. A `None` +/// prompt means the minibuffer is closed. +/// +/// **ONE cache per peer, not one per variant.** [`SemanticRenderState`] +/// is constructed by [`SemanticRenderState::for_peer`] with the +/// session's negotiated version baked in on attach and dropped on +/// detach, so a cache can never span two negotiated versions and a +/// per-variant key would guard nothing. The rows are the cached form +/// either way: for a `12..=22` peer every `detail` is `None` (the +/// producer does not resolve details it cannot ship), so the cache +/// describes exactly what that peer received. +type MinibufferFacts = ( + Option, + String, + u32, + Vec, + Option, + u32, +); /// Cached `CompletionPopup` payload for cached-compare suppression /// (Arc 1a Q#C5): `(anchor, prefix_len, rows-window, selected, total)`. @@ -115,10 +131,20 @@ type CompletionPopupFacts = ( /// scrolled window around the selection, not the full (≤1024) list. const MB_VISIBLE: usize = 10; +/// The first protocol version that carries +/// [`InstanceMessage::MinibufferPromptRows`] (Discovery Stage 2). +/// +/// Named rather than written as a literal `23` at each site, and NOT +/// derived from `PROTOCOL_VERSION`: the contract is "the version this +/// variant was introduced at", which is an absolute fact, while +/// `PROTOCOL_VERSION` moves with every later bump. Handoff §5 records +/// five defects of exactly that shape from one previous bump. +pub const MINIBUFFER_ROWS_MIN_VERSION: u32 = 23; + /// A window of up to [`MB_VISIBLE`] candidates around `selected`, plus /// the selection's index *within* that window. Keeps the selected row /// visible as the user cycles a long list. -fn minibuffer_window(candidates: &[String], selected: Option) -> (Vec, Option) { +fn minibuffer_window(candidates: &[T], selected: Option) -> (Vec, Option) { if candidates.is_empty() { return (Vec::new(), None); } @@ -216,10 +242,18 @@ pub struct SemanticRenderState { /// Last emitted `MenuPrompt` payload per buffer (Q#CM1), for /// cached-compare suppression (see [`MenuPromptFacts`]). last_menu_prompt: HashMap, - /// Last emitted `MinibufferPrompt` payload (Q#MB1) — a single value, - /// not per-buffer, because the minibuffer is one global core - /// instance. + /// Last emitted minibuffer payload (Q#MB1) — a single value, not + /// per-buffer, because the minibuffer is one global core instance, + /// and a single value across both wire variants, because this state + /// belongs to one peer at one negotiated version (see + /// [`MinibufferFacts`]). last_minibuffer: Option, + /// Whether the peer negotiated protocol >= 23 (Discovery Stage 2). + /// `true` ⇒ it receives `MinibufferPromptRows` and never the legacy + /// variant; `false` ⇒ the frozen `MinibufferPrompt` and never the + /// rows form. Also gates the per-row detail lookup: a peer that + /// cannot carry a detail does not pay to resolve one. + peer_knows_minibuffer_rows: bool, /// Last emitted `CompletionPopup` payload per buffer (Arc 1a /// Q#C5), for cached-compare suppression (see /// [`CompletionPopupFacts`]). @@ -478,6 +512,7 @@ impl SemanticRenderState { s.peer_knows_theme_facts = negotiated_protocol_version >= 16; s.peer_knows_font_facts = negotiated_protocol_version >= 17; s.peer_knows_line_wrap = negotiated_protocol_version >= 22; + s.peer_knows_minibuffer_rows = negotiated_protocol_version >= MINIBUFFER_ROWS_MIN_VERSION; s.peer_knows_statusline_segments = negotiated_protocol_version >= 18; s.peer_knows_terminal_frames = negotiated_protocol_version >= 19; s.peer_knows_panel_frames = negotiated_protocol_version >= PANEL_MIN_VERSION; @@ -500,6 +535,7 @@ impl SemanticRenderState { last_search_prompt: HashMap::new(), last_menu_prompt: HashMap::new(), last_minibuffer: None, + peer_knows_minibuffer_rows: true, last_completion_popup: HashMap::new(), last_summary: HashMap::new(), last_status: HashMap::new(), @@ -1637,11 +1673,20 @@ impl SemanticRenderState { Some(msg) } - /// The `MinibufferPrompt` message for this frame, or `None` when the + /// The minibuffer message for this frame, or `None` when the /// (global) minibuffer state is unchanged (Q#MB1). Emitted only from /// the active buffer's viewport so the bufferless message ships once /// per frame. Closed = `prompt: None`; first sight while closed stays - /// silent. The daemon keeps the variant off wires negotiated `< 12`. + /// silent. + /// + /// **Exactly one variant, chosen by the peer's negotiated version** + /// (Discovery Stage 2). `>= 23` gets `MinibufferPromptRows` with + /// per-row details; `12..=22` gets the frozen `MinibufferPrompt` + /// carrying bare labels. Because the choice is made here, the CLOSE + /// necessarily uses the same family as the OPEN — a rows session + /// closed by a legacy clear would leave a popup on screen forever. + /// The daemon's write loop gates both directions again as + /// belt-and-braces. fn minibuffer_prompt_msg( &mut self, state: &EditorState, @@ -1662,13 +1707,61 @@ impl SemanticRenderState { .take_while(|(i, _)| *i < cursor_byte) .count() as u32; let total = session.candidates.len() as u32; - let (candidates, selected) = + let (labels, selected) = minibuffer_window(&session.candidates, session.selected); + // Q#D2-2: the detail is per row and optional. Only + // the command source has one today; a file-path or + // buffer-name prompt leaves it `None` and renders + // exactly as it did before v23. Resolved only for a + // peer that can carry it, so the cached facts + // describe what that peer actually received. + let detail_source = self.peer_knows_minibuffer_rows + && matches!( + session.source, + crate::minibuffer::CompletionSource::Commands + ); + let rows = if detail_source { + let commands = state.lua_host.commands().borrow(); + 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_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() + } else { + labels + .into_iter() + .map(|label| MinibufferRow { + label, + detail: None, + }) + .collect() + }; ( Some(session.prompt.clone()), input, cursor, - candidates, + rows, selected, total, ) @@ -1684,13 +1777,24 @@ impl SemanticRenderState { self.last_minibuffer = Some(facts); return None; } - let msg = InstanceMessage::MinibufferPrompt { - prompt: facts.0.clone(), - input: facts.1.clone(), - cursor: facts.2, - candidates: facts.3.clone(), - selected: facts.4, - total: facts.5, + let msg = if self.peer_knows_minibuffer_rows { + InstanceMessage::MinibufferPromptRows { + prompt: facts.0.clone(), + input: facts.1.clone(), + cursor: facts.2, + rows: facts.3.clone(), + selected: facts.4, + total: facts.5, + } + } else { + InstanceMessage::MinibufferPrompt { + prompt: facts.0.clone(), + input: facts.1.clone(), + cursor: facts.2, + candidates: facts.3.iter().map(|row| row.label.clone()).collect(), + selected: facts.4, + total: facts.5, + } }; self.last_minibuffer = Some(facts); Some(msg) @@ -5773,9 +5877,28 @@ mod tests { let short: Vec = vec!["a".into(), "b".into(), "c".into()]; assert_eq!(minibuffer_window(&short, Some(2)), (short.clone(), Some(2))); // Empty. - assert_eq!(minibuffer_window(&[], Some(0)), (Vec::new(), None)); + assert_eq!( + minibuffer_window::(&[], Some(0)), + (Vec::new(), None) + ); } + /// The v23 rows form: `(prompt, input, rows)`. + fn minibuffer_rows_of( + msgs: &[InstanceMessage], + ) -> Option<(Option, String, Vec)> { + msgs.iter().find_map(|m| match m { + InstanceMessage::MinibufferPromptRows { + prompt, + input, + rows, + .. + } => Some((prompt.clone(), input.clone(), rows.clone())), + _ => None, + }) + } + + /// The frozen `12..=22` form: `(prompt, input, candidates)`. fn minibuffer_prompt_of( msgs: &[InstanceMessage], ) -> Option<(Option, String, Vec)> { @@ -5798,7 +5921,7 @@ mod tests { s.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); // No minibuffer: the producer stays silent on first sight. - assert!(minibuffer_prompt_of(&s.render_frame(&state)).is_none()); + assert!(minibuffer_rows_of(&s.render_frame(&state)).is_none()); // Open an `M-x` prompt (command completion) via the Lua API. state @@ -5807,28 +5930,133 @@ mod tests { .load("pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', on_accept = function() end }") .exec() .expect("open minibuffer"); - let (prompt, input, cands) = - minibuffer_prompt_of(&s.render_frame(&state)).expect("minibuffer prompt emitted"); + let (prompt, input, rows) = + minibuffer_rows_of(&s.render_frame(&state)).expect("minibuffer prompt emitted"); assert_eq!(prompt.as_deref(), Some("M-x ")); assert_eq!(input, ""); // Empty input matches every command; the wire carries a window. - assert!(!cands.is_empty(), "M-x seeds command candidates"); - assert!(cands.len() <= MB_VISIBLE, "candidates ship windowed"); + assert!(!rows.is_empty(), "M-x seeds command candidates"); + assert!(rows.len() <= MB_VISIBLE, "candidates ship windowed"); // Unchanged → suppressed (cached-compare). - assert!(minibuffer_prompt_of(&s.render_frame(&state)).is_none()); + assert!(minibuffer_rows_of(&s.render_frame(&state)).is_none()); - // Cancel: the prompt clears (None). + // Cancel: the prompt clears (None), in the SAME family as the + // open — a rows session closed by a legacy clear would leave the + // dropdown on screen forever. state .lua_host .lua() .load("pmacs.minibuffer.cancel()") .exec() .expect("cancel"); - let (prompt, _, _) = minibuffer_prompt_of(&s.render_frame(&state)).expect("clear emitted"); + let frame = s.render_frame(&state); + assert!( + minibuffer_prompt_of(&frame).is_none(), + "a v23 peer must never see the legacy variant, not even to close" + ); + let (prompt, _, _) = minibuffer_rows_of(&frame).expect("clear emitted"); assert!(prompt.is_none(), "cancel clears the minibuffer band"); } + #[test] + fn a_v22_peer_gets_the_frozen_variant_and_a_v23_peer_gets_rows_with_details() { + // The producer half of the exclusivity guarantee, at the two + // versions that straddle the boundary. The real-daemon half — + // two sessions negotiating simultaneously — is in + // `tests/discovery_stage2_acceptance.rs`. + let state = empty_state(); + let bid = active_buffer(&state); + state + .lua_host + .lua() + .load( + "pmacs.command.define{ name = 'mb.probe', description = 'Probe the row detail.', \ + fn = function() end }", + ) + .exec() + .expect("define probe command"); + + let mut v22 = SemanticRenderState::for_peer(FrontendId::LOCAL, 22); + let mut v23 = SemanticRenderState::for_peer(FrontendId::LOCAL, 23); + for s in [&mut v22, &mut v23] { + s.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); + let _ = s.render_frame(&state); + } + + state + .lua_host + .lua() + .load( + "pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', \ + on_accept = function() end }", + ) + .exec() + .expect("open minibuffer"); + state + .lua_host + .lua() + .load("pmacs.minibuffer.set_contents('mb.probe')") + .exec() + .expect("narrow to the probe command"); + + let v22_frame = v22.render_frame(&state); + assert!( + minibuffer_rows_of(&v22_frame).is_none(), + "a v22 peer must never receive the v23 rows variant" + ); + let (_, _, candidates) = + minibuffer_prompt_of(&v22_frame).expect("v22 gets the frozen variant"); + assert!( + candidates.iter().any(|c| c == "mb.probe"), + "the frozen variant still carries the candidate names: {candidates:?}" + ); + + let v23_frame = v23.render_frame(&state); + assert!( + minibuffer_prompt_of(&v23_frame).is_none(), + "a v23 peer must never receive the frozen variant" + ); + let (_, _, rows) = minibuffer_rows_of(&v23_frame).expect("v23 gets the rows variant"); + let probe = rows + .iter() + .find(|r| r.label == "mb.probe") + .expect("the probe command is a candidate"); + assert_eq!( + probe.detail.as_deref(), + Some("Probe the row detail."), + "the row carries the command's registered description" + ); + } + + #[test] + fn a_source_with_no_detail_ships_rows_with_none() { + // Q#D2-2: only the command source has a detail today. A + // buffer-name prompt leaves it `None`, and the GPU then renders + // exactly what it rendered before v23. + let state = empty_state(); + let mut s = local(); + let bid = active_buffer(&state); + s.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); + let _ = s.render_frame(&state); + + state + .lua_host + .lua() + .load( + "pmacs.minibuffer.read{ prompt = 'Buffer: ', source = 'buffers', \ + on_accept = function() end }", + ) + .exec() + .expect("open buffer prompt"); + let (_, _, rows) = minibuffer_rows_of(&s.render_frame(&state)).expect("prompt emitted"); + assert!(!rows.is_empty(), "the buffer registry seeds candidates"); + assert!( + rows.iter().all(|r| r.detail.is_none()), + "a source with no detail leaves every row's detail None: {rows:?}" + ); + } + #[test] fn status_facts_emit_on_change_and_freeze_counts_while_stale() { let state = empty_state(); diff --git a/tests/bottom_panel_stage2b_gpu_acceptance.rs b/tests/bottom_panel_stage2b_gpu_acceptance.rs index 257680a..32c197d 100644 --- a/tests/bottom_panel_stage2b_gpu_acceptance.rs +++ b/tests/bottom_panel_stage2b_gpu_acceptance.rs @@ -337,8 +337,9 @@ fn one_daemon_serves_a_v21_panel_session_and_a_shipped_v20_client() { #[test] fn the_baseline_stays_and_the_counter_offer_activates() { // A deliberate tripwire: bumping the wire must be a conscious edit - // here, not a silent one. v22 is `LineWrapFacts` (long-lines Stage 3). - assert_eq!(PROTOCOL_VERSION, 22); + // here, not a silent one. v23 is `MinibufferPromptRows` (Discovery + // Stage 2); v22 was `LineWrapFacts` (long-lines Stage 3). + assert_eq!(PROTOCOL_VERSION, 23); assert_eq!( ADVERTISED_PROTOCOL_VERSION, 20, "moving this is the incompatible act the mechanism exists to avoid" @@ -352,10 +353,10 @@ fn the_baseline_stays_and_the_counter_offer_activates() { // This replaces `assert_eq!(PANEL_MIN_VERSION, PROTOCOL_VERSION)`, // which asserted a **coincidence**: panel frames were the newest // feature when it was written, so their minimum happened to equal - // the current wire. Any later feature falsifies that — v22 is the - // first, and the equality would have had to be edited on every - // subsequent bump while telling a reader something that was never - // the contract. + // the current wire. Any later feature falsifies that — v22 was the + // first and v23 the second, and the equality would have had to be + // edited on every subsequent bump while telling a reader something + // that was never the contract. // `const` blocks, matching the line above: these are compile-time // constants, so a runtime `assert!` is both a clippy error and a // weaker check than the language already offers. diff --git a/tests/discovery_stage2_acceptance.rs b/tests/discovery_stage2_acceptance.rs new file mode 100644 index 0000000..571c605 --- /dev/null +++ b/tests/discovery_stage2_acceptance.rs @@ -0,0 +1,684 @@ +// discovery_stage2_acceptance.rs --- Discovery Stage 2 +// (docs/discovery-stage2-framing.md §6). + +//! `M-x` rows stop being bare names. +//! +//! `Command.description` already existed and was already rendered by +//! `help.list-commands`; it was missing at the one moment it would +//! change a decision. Carrying it to the row is two independent halves, +//! and this suite keeps them separate because they fail separately: +//! +//! - **The wire half** is a protocol bump, v22 → v23, and it is +//! *additive*. `MinibufferPrompt` is FROZEN and still sent to every +//! `12..=22` peer, because postcard encodes fields positionally — a +//! widened `candidates` would make those peers mis-decode rather than +//! ignore, and gating the widened form would have left them with no +//! minibuffer message at all. The rich shape lives in an appended +//! `MinibufferPromptRows`, and **exactly one of the two reaches any +//! peer, ever**. +//! - **The TUI half involves no wire at all.** `src/editor.rs` contains +//! zero references to `MinibufferPrompt`: `paint_minibuffer` reads +//! `core.minibuffer` directly and renders the selected candidate as an +//! inline suffix. So it reads `Command.description` from the registry +//! in-process, which is why this half is independent of the bump. +//! +//! The daemon fixtures are `crdt`-gated because a semantic session is +//! necessarily a text replica: a non-CRDT build advertises no +//! `semantic_render` and cannot host one. They run in the +//! `--features crdt` sweep that `scripts/gate --protocol` adds. + +mod common; + +use std::path::Path; + +use pmacs::bootstrap::BootstrapRoots; +use pmacs::editor::EditorState; +use pmacs_protocol::{ + ADVERTISED_PROTOCOL_VERSION, ByteRange, InstanceMessage, MinibufferRow, PROTOCOL_VERSION, + is_supported_protocol_version, +}; + +#[cfg(feature = "crdt")] +use std::os::unix::net::UnixStream; +#[cfg(feature = "crdt")] +use std::time::{Duration, Instant}; + +#[cfg(feature = "crdt")] +use pmacs_protocol::cell::CellSize; +#[cfg(feature = "crdt")] +use pmacs_protocol::message::{ + AttachRequest, FrontendCapabilities, FrontendEvent, Hello, Key, KeyEvent, Modifiers, + SessionBootstrapRequest, +}; +#[cfg(feature = "crdt")] +use pmacs_protocol::transport::{read_message, write_message}; + +#[cfg(feature = "crdt")] +use common::daemon::{TestDaemon, build_default_caps}; + +// --------------------------------------------------------------------------- +// Version-bump discipline (§6, last bullet) +// --------------------------------------------------------------------------- + +/// The bump is deliberate, and the advertised baseline does NOT move. +/// +/// `ADVERTISED_PROTOCOL_VERSION` is pinned at 20 and is the one constant +/// that must never be edited (handoff §3/§5): the handshake is +/// server-first, so moving it locks out every already-shipped frontend +/// before it can counter-offer. An additive family never needs it. +#[test] +fn the_wire_is_v23_and_the_advertised_baseline_is_unmoved() { + assert_eq!( + PROTOCOL_VERSION, 23, + "v23 is MinibufferPromptRows (Discovery Stage 2)" + ); + assert_eq!( + ADVERTISED_PROTOCOL_VERSION, 20, + "moving this is the incompatible act the counter-offer mechanism exists to avoid" + ); + // The whole v12..=22 population this lane is compatible with is + // still supported, and the set ends at the new wire — a widened set + // is a failure rather than a silent pass. + for version in 6..=23 { + assert!( + is_supported_protocol_version(version), + "v{version} must still be supported" + ); + } + assert!(!is_supported_protocol_version(24)); +} + +// --------------------------------------------------------------------------- +// The TUI half: no wire involvement (§3.4, §6) +// --------------------------------------------------------------------------- + +fn session(name: &str) -> EditorState { + let base = Path::new(env!("CARGO_TARGET_TMPDIR")) + .join("discovery-stage2") + .join(name); + let _ = std::fs::remove_dir_all(&base); + let roots = BootstrapRoots::isolated_under(&base); + for (_, dir) in roots.child_env() { + std::fs::create_dir_all(&dir).expect("create controlled root"); + } + let state = EditorState::new_with_roots(&roots); + state.install_state_dirs(); + state +} + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +/// Render one frame at `cols` columns and return the bottom row's text. +/// +/// Through `RenderState` and the wire rather than by calling the painter +/// directly: the spans are what the TUI actually consumes, so this +/// asserts on the cells that reach a screen. +fn bottom_row(s: &EditorState, rows: u32, cols: u32) -> String { + use std::collections::HashMap; + + let size = pmacs::cell::CellSize::new(rows, cols); + let mut rs = pmacs::instance_render::RenderState::new(size); + let msgs = rs.render_frame(s, pmacs::protocol::FrontendId::LOCAL, &HashMap::new(), &[]); + + let mut row = vec![' '; cols as usize]; + for msg in &msgs { + if let pmacs_protocol::InstanceMessage::CellDelta { spans, .. } = msg { + for span in spans { + if span.start.row != rows - 1 { + continue; + } + for (i, cell) in span.cells.iter().enumerate() { + let c = span.start.col as usize + i; + if c < cols as usize + && let pmacs::cell::Glyph::Char(ch) = cell.glyph + { + row[c] = ch; + } + } + } + } + } + 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 { + exec( + s, + "pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', on_accept = function() end }", + ); + exec(s, "pmacs.minibuffer.set_contents('zzprobe')"); + bottom_row(s, 24, cols) +} + +const PROBE_DESCRIPTION: &str = "Probe the description row."; + +fn define_probe(s: &EditorState) { + exec( + s, + &format!( + "pmacs.command.define{{ name = 'zzprobe', description = '{PROBE_DESCRIPTION}', \ + fn = function() end }}" + ), + ); +} + +#[test] +fn the_tui_renders_the_description_beside_the_selected_name() { + let s = session("tui-wide"); + define_probe(&s); + let row = mx_bottom_row(&s, 120); + assert!( + row.contains(&format!("[zzprobe — {PROBE_DESCRIPTION}]")), + "the selected candidate carries its description: {row:?}" + ); +} + +#[test] +fn the_tui_drops_the_description_then_the_whole_suffix_as_width_shrinks() { + // §3.4's three ORDERED steps, at the three widths that separate + // them. The guarantee is "never a PARTIAL name", which is + // achievable; "the name always survives" is not, because the prompt + // and the typed input consume the budget first. + let s = session("tui-clip"); + define_probe(&s); + + // 1. Wide: name + description. + let wide = mx_bottom_row(&s, 120); + assert!( + wide.contains(&format!("[zzprobe — {PROBE_DESCRIPTION}]")), + "wide: {wide:?}" + ); + + // 2. Room for the whole name but not the whole description: the + // description is dropped, leaving exactly today's `[name]`. No + // ellipsis stub, and no prefix of the description either. + let medium = mx_bottom_row(&s, 30); + assert!(medium.contains("[zzprobe]"), "medium: {medium:?}"); + assert!( + !medium.contains('—'), + "a description that does not fit whole is dropped entirely: {medium:?}" + ); + + // 3. Too narrow for even the whole name: the suffix vanishes. The + // assertion is that no PREFIX of the name is emitted — `[zzpr` + // would read as a different command, which is worse than nothing. + let narrow = mx_bottom_row(&s, 18); + assert!( + !narrow.contains('['), + "a suffix that cannot hold the whole name is omitted entirely: {narrow:?}" + ); + assert!( + narrow.starts_with("M-x zzprobe"), + "the prompt and the typed input still own the row: {narrow:?}" + ); + for cut in 1.."zzprobe".len() { + assert!( + !narrow.contains(&format!("[{}", &"zzprobe"[..cut])), + "no prefix of the name may be emitted: {narrow:?}" + ); + } +} + +#[test] +fn a_source_with_no_detail_renders_exactly_as_before_in_the_tui() { + // Q#D2-2: the file-path prompt is the witness. It has no detail, so + // its suffix is the pre-v23 `[name]` and nothing else. + let s = session("tui-files"); + let dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("discovery-stage2-files"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create file-prompt dir"); + std::fs::write(dir.join("zznotes.txt"), b"x").expect("seed a file"); + exec( + &s, + &format!( + "pmacs.minibuffer.read{{ prompt = 'File: ', source = 'files', \ + source_root = '{}', on_accept = function() end }}", + dir.display() + ), + ); + exec(&s, "pmacs.minibuffer.set_contents('zznotes.txt')"); + let row = bottom_row(&s, 24, 120); + assert!(row.contains("[zznotes.txt]"), "file prompt row: {row:?}"); + assert!( + !row.contains('—'), + "a source with no detail gains no separator: {row:?}" + ); +} + +// --------------------------------------------------------------------------- +// 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` + // LOOK like a closed set, which invites making acceptance reject + // unmatched input. That would be a behaviour change, and it is out + // of scope. `resolve_accepted_value` still returns the literal typed + // text when nothing is selected. + let s = session("open-set"); + exec( + &s, + "_G.ACCEPTED = nil + pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', + on_accept = function(v) _G.ACCEPTED = v end }", + ); + exec( + &s, + "pmacs.minibuffer.set_contents('no-such-command-at-all')", + ); + assert_eq!( + eval::(&s, "return #pmacs.minibuffer.candidates()"), + 0, + "the probe input must match nothing, or this asserts the wrong thing" + ); + exec(&s, "pmacs.minibuffer.accept()"); + assert_eq!( + eval::(&s, "return _G.ACCEPTED"), + "no-such-command-at-all", + "completion is assistance, not validation" + ); +} + +// --------------------------------------------------------------------------- +// The wire half: one real daemon, two negotiated versions (§6) +// --------------------------------------------------------------------------- + +/// An `init.lua` that registers the probe command whose description the +/// wire must carry. +#[cfg(feature = "crdt")] +const PROBE_INIT: &str = r#" +pmacs.command.define { + name = "zzprobe", + description = "Probe the description row.", + fn = function() end, +} +"#; + +/// A minibuffer message, in whichever family it arrived. +#[cfg(feature = "crdt")] +#[derive(Debug)] +enum Mb { + Legacy { + prompt: Option, + candidates: Vec, + }, + Rows { + prompt: Option, + rows: Vec, + }, +} + +#[cfg(feature = "crdt")] +fn semantic_caps() -> FrontendCapabilities { + FrontendCapabilities { + multi_frontend: true, + crdt_replica: true, + semantic_render: true, + ..build_default_caps() + } +} + +/// Attach a semantic session offering exactly `offer`, declare a +/// viewport so the projection producer is live, and hand back the +/// stream plus this session's frontend id. +#[cfg(feature = "crdt")] +fn attach_semantic(daemon: &TestDaemon, offer: u32) -> (UnixStream, pmacs_protocol::FrontendId) { + let mut stream = daemon.connect(); + stream + .set_read_timeout(Some(Duration::from_secs(10))) + .expect("set read timeout"); + let hello: Hello = read_message(&mut stream).expect("read daemon Hello"); + assert_eq!( + hello.protocol_version, ADVERTISED_PROTOCOL_VERSION, + "the server-first Hello must stay at the compatibility baseline" + ); + let fid = hello.assigned_frontend_id; + write_message( + &mut stream, + &AttachRequest { + protocol_version: offer, + frontend_capabilities: semantic_caps(), + initial_size: CellSize::new(24, 80), + }, + ) + .expect("write AttachRequest"); + // A v20-or-later semantic session sends the bootstrap envelope; the + // daemon reads it unconditionally for those, so skipping it would + // desynchronize the stream. + if offer >= 20 { + write_message( + &mut stream, + &SessionBootstrapRequest { + initial_target: None, + }, + ) + .expect("write bootstrap"); + } + let document = pump(&mut stream, "first BufferSnapshot", |msg| match msg { + InstanceMessage::BufferSnapshot { buffer_id, .. } => Some(*buffer_id), + _ => None, + }); + write_message( + &mut stream, + &FrontendEvent::Viewport { + frontend_id: fid, + buffer_id: document, + visible: ByteRange { start: 0, end: 0 }, + generation: 0, + }, + ) + .expect("declare a viewport"); + (stream, fid) +} + +#[cfg(feature = "crdt")] +fn pump( + stream: &mut UnixStream, + what: &str, + mut want: impl FnMut(&InstanceMessage) -> Option, +) -> T { + let deadline = Instant::now() + Duration::from_secs(20); + while Instant::now() < deadline { + match read_message::(stream) { + Ok(msg) => { + if let Some(found) = want(&msg) { + return found; + } + } + Err(error) => panic!("{what}: read stopped: {error}"), + } + } + panic!("timed out waiting for {what}"); +} + +/// Collect every minibuffer message this session receives, up to and +/// including the first one `done` accepts. +/// +/// Collecting rather than filtering is the point: "a v23 peer receives +/// the rows form" is only half the guarantee, and the other half — that +/// it never receives the legacy form — can only be checked against +/// everything that arrived. +#[cfg(feature = "crdt")] +fn collect_minibuffer( + stream: &mut UnixStream, + what: &str, + mut done: impl FnMut(&Mb) -> bool, +) -> Vec { + let mut seen = Vec::new(); + let deadline = Instant::now() + Duration::from_secs(20); + while Instant::now() < deadline { + match read_message::(stream) { + Ok(InstanceMessage::MinibufferPrompt { + prompt, candidates, .. + }) => { + seen.push(Mb::Legacy { prompt, candidates }); + } + Ok(InstanceMessage::MinibufferPromptRows { prompt, rows, .. }) => { + seen.push(Mb::Rows { prompt, rows }); + } + Ok(_) => continue, + Err(error) => panic!("{what}: read stopped: {error}"), + } + if done(seen.last().expect("just pushed")) { + return seen; + } + } + panic!("timed out waiting for {what}; saw {seen:?}"); +} + +#[cfg(feature = "crdt")] +fn send_key(stream: &mut UnixStream, fid: pmacs_protocol::FrontendId, key: Key, mods: Modifiers) { + write_message( + stream, + &FrontendEvent::Key(KeyEvent { + frontend_id: fid, + key, + mods, + timestamp_ns: 0, + }), + ) + .expect("write key"); +} + +/// The whole exclusivity guarantee, on one live daemon: a v22 peer and a +/// v23 peer attached **simultaneously** each receive their own variant +/// and only their own — open and close alike. +/// +/// One daemon rather than two, and both directions in one fixture. Two +/// daemons could each pass their own half while the same build was +/// incapable of serving both, which is the only property that matters; +/// and a test that only proved "v23 gets rows" would pass with the +/// compatibility half broken. +#[cfg(feature = "crdt")] +#[test] +fn one_daemon_serves_a_v23_rows_session_and_a_frozen_v22_session() { + let daemon = TestDaemon::spawn_with_config(PROBE_INIT); + + // The compatibility half attaches FIRST, deliberately: it is the + // half an over-eager bump destroys, so a regression fails here + // rather than after the interesting half has already passed. + let (mut legacy, _legacy_fid) = attach_semantic(&daemon, 22); + let (mut current, current_fid) = attach_semantic(&daemon, PROTOCOL_VERSION); + assert_eq!(PROTOCOL_VERSION, 23); + + // Open the real `M-x` through the real key path, then narrow to the + // probe command by typing it — the candidate window is ten rows out + // of well over a hundred commands, so an unnarrowed prompt would + // assert nothing about the probe. + send_key(&mut current, current_fid, Key::Char('x'), Modifiers::ALT); + for ch in "zzprobe".chars() { + send_key(&mut current, current_fid, Key::Char(ch), Modifiers::NONE); + } + + let on_current = collect_minibuffer(&mut current, "v23 open", |mb| match mb { + Mb::Rows { prompt, rows } => { + prompt.is_some() && rows.iter().any(|row| row.label == "zzprobe") + } + Mb::Legacy { .. } => false, + }); + assert!( + on_current.iter().all(|mb| matches!(mb, Mb::Rows { .. })), + "a v23 peer must never receive the frozen legacy variant: {on_current:?}" + ); + let Some(Mb::Rows { rows, .. }) = on_current.last() else { + unreachable!("collect_minibuffer returns on a Rows match") + }; + 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), + "the description reaches the row through the real prompt path" + ); + + // The same session state, seen by the v22 peer, in the frozen shape. + let on_legacy = collect_minibuffer(&mut legacy, "v22 open", |mb| match mb { + Mb::Legacy { prompt, candidates } => { + prompt.is_some() && candidates.iter().any(|c| c == "zzprobe") + } + Mb::Rows { .. } => false, + }); + assert!( + on_legacy.iter().all(|mb| matches!(mb, Mb::Legacy { .. })), + "a v22 peer must never receive the v23 rows variant: {on_legacy:?}" + ); + + // The close must arrive in the SAME family as the open. A rows + // session closed by a legacy clear leaves the dropdown on screen + // forever, and the witness for "it actually cleared" is a `prompt: + // None` in the family the frontend is mirroring. + send_key(&mut current, current_fid, Key::Escape, Modifiers::NONE); + let closed_current = collect_minibuffer(&mut current, "v23 close", |mb| { + matches!(mb, Mb::Rows { prompt: None, .. }) + }); + assert!( + closed_current + .iter() + .all(|mb| matches!(mb, Mb::Rows { .. })), + "the v23 close must not arrive as a legacy clear: {closed_current:?}" + ); + let closed_legacy = collect_minibuffer(&mut legacy, "v22 close", |mb| { + matches!(mb, Mb::Legacy { prompt: None, .. }) + }); + assert!( + closed_legacy + .iter() + .all(|mb| matches!(mb, Mb::Legacy { .. })), + "the v22 close must stay in the frozen family: {closed_legacy:?}" + ); +} diff --git a/tests/statusline_segments_acceptance.rs b/tests/statusline_segments_acceptance.rs index b583c83..a4dc5f1 100644 --- a/tests/statusline_segments_acceptance.rs +++ b/tests/statusline_segments_acceptance.rs @@ -789,7 +789,8 @@ fn a13_17_26_protocol_semantic_init_late_join_and_version_cost() { // Vterm Stage 3 appended the terminal family as v19; GPU initial targets // appended the semantic bootstrap family as v20; bottom-panel Stage 2B-1 // appended the panel family as v21; long-lines Stage 3 appended - // `LineWrapFacts` as v22. This acceptance owns the STATUSLINE + // `LineWrapFacts` as v22; Discovery Stage 2 appended + // `MinibufferPromptRows` as v23. This acceptance owns the STATUSLINE // variant's placement and gate, so it tracks the current wire version // rather than pinning 18: the v18 floor it actually cares about is asserted // below and in `peer_accepts_statusline_message`. @@ -798,11 +799,11 @@ fn a13_17_26_protocol_semantic_init_late_join_and_version_cost() { // three lines on purpose. The ceiling assertion is the load-bearing // one — it says the supported set ENDS here, which is what makes an // accidentally-widened set a failure rather than a silent pass. - assert_eq!(PROTOCOL_VERSION, 22); - for version in 6..=22 { + assert_eq!(PROTOCOL_VERSION, 23); + for version in 6..=23 { assert!(is_supported_protocol_version(version)); } - assert!(!is_supported_protocol_version(23)); + assert!(!is_supported_protocol_version(24)); let sample = InstanceMessage::StatuslineSegments { buffer_id: BufferId::from_raw(9), left: vec![StatuslineSegment { diff --git a/tests/vterm_stage3_acceptance.rs b/tests/vterm_stage3_acceptance.rs index b5a71c5..92c287b 100644 --- a/tests/vterm_stage3_acceptance.rs +++ b/tests/vterm_stage3_acceptance.rs @@ -888,9 +888,10 @@ fn terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret() { panic!("timed out waiting for {what}"); } - // Tripwire: a wire bump must be a conscious edit here. v22 is + // Tripwire: a wire bump must be a conscious edit here. v23 is + // `MinibufferPromptRows` (Discovery Stage 2); v22 was // `LineWrapFacts` (long-lines Stage 3). - assert_eq!(PROTOCOL_VERSION, 22); + assert_eq!(PROTOCOL_VERSION, 23); let daemon = common::daemon::TestDaemon::spawn_with_env_and_init( &[ ("PMACS_INSTANCE_SEMANTIC_RENDER", "1"),