From e9bfe9a14cd4c17c4d9e1489415376e471bee82c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 13:46:18 +0200 Subject: [PATCH 1/7] =?UTF-8?q?docs:=20frame=20Discovery=20Stage=202=20(re?= =?UTF-8?q?vision=202)=20=E2=80=94=20M-x=20rows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COHERENCE.md section 5 grades discoverability Partial after Stage 1 and names "M-x rows are still bare names". The descriptions ALREADY EXIST --- `Command.description` is required, and `help.list-commands` renders them --- so this is substrate without surface in its purest form: the information is present, surfaced elsewhere, and absent from the one moment it would change a decision. TWO REVISION-1 CLAIMS WERE WRONG, both checkable in the tree: - "Change `candidates` in place and gate at >= 23." Postcard is NOT self-describing: fields encode positionally, so a v22 peer decoding `Vec` where it expects `Vec` mis-reads the bytes rather than skipping them. And gating would not have rescued it --- with only one variant to gate, a v12-v22 peer would have received NO MINIBUFFER AT ALL. That variant goes to every peer negotiated >= 12 (src/daemon.rs:1472). Revision 2 is additive: `MinibufferPromptRows` APPENDED to the enum (indices are positional; inserting renumbers everything), `MinibufferPrompt` frozen for v12-v22, per-peer selection, per- variant cache keys, and close matching the open's family --- a mismatched close is how a popup stays on screen forever. - "Both frontends render label + detail." The grid TUI never reads `MinibufferPrompt`; src/editor.rs contains ZERO references to it. It paints from `core.minibuffer` and renders the selected candidate as `format!(" [{cand}]")` (src/editor.rs:5484). The rich wire reaches pmacs-gpu only. So the TUI half is a LOCAL formatting change --- it is in-process with the core and reads `Command.description` from the registry directly, with no wire involvement. The contract is pinned including clipping: THE NAME SURVIVES AND THE DESCRIPTION IS DROPPED at narrow widths, because a clipped name is strictly worse than today's bare one. A multi-row TUI chooser is explicitly not this lane. This lane HOLDS THE BUMP SLOT. Git Stage 1 is no-wire and runs beside it; git Stage 2 needs a bump and must wait. Q#D2-5 records a trap that arrives with the feature: richer rows make M-x LOOK like a closed set, inviting someone to make acceptance reject unmatched input. Completion is assistance, not validation --- `resolve_accepted_value` returns literal typed text by design --- so that would be a behaviour change, not a rendering one, and it is out of scope. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 33 ++++ docs/discovery-stage2-framing.md | 295 +++++++++++++++++++++++++++++++ 2 files changed, 328 insertions(+) create mode 100644 docs/discovery-stage2-framing.md diff --git a/docs/active-work.md b/docs/active-work.md index 59783a1..352eb53 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,6 +265,38 @@ 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 — BRANCHED, framing in review + +**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 2**, in + review. 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 ` — + the strengthened two-configuration sweep, which is what `--protocol` + exists for. + ## LSP LaTeX coverage — IMPLEMENTED, gates green, no PR yet **Written with the lane's first commit**, per the standing correction @@ -603,6 +635,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/discovery-stage2-framing.md b/docs/discovery-stage2-framing.md new file mode 100644 index 0000000..b2a12af --- /dev/null +++ b/docs/discovery-stage2-framing.md @@ -0,0 +1,295 @@ +# Discovery Stage 2 — M-x rows stop being bare names + +**Status: framing pass, revision 2. Pre-implementation. Awaiting +approval.** + +**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. +- **Close and cache ordering.** The prompt is cached-compare + suppressed, so the cache key must be **per variant**, or a v23 peer + that reconnects at v22 (or vice versa across a restart) can have its + first message suppressed as a duplicate of one it never received. + **The close message must use the same variant family as the open** — + a `MinibufferPromptRows` session closed by a legacy clear is exactly + the kind of mismatch that leaves a popup on screen forever. + +### 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 is explicit**: the suffix is already written against + `max = term_size.cols` with a running `written` count. The **name + must survive clipping and the description is what gets truncated** — + a row that clips to `[buffer.sa…]` would be strictly worse than + today. If the terminal is too narrow for `name — ` plus one + character of description, **the description is dropped entirely** + rather than shown as an ellipsis stub. +- **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 rev 2, 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, per-peer +selection, per-variant cache keys, and matched open/close families. + +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. +- **A round-trip encode/decode of the frozen `MinibufferPrompt`** + pins its shape, so a later field addition to it fails a test rather + than silently breaking v12–v22. +- **The cache key is per variant**: a session that opens for a v23 peer + and a later one for a v22 peer are not suppressed as duplicates of + each other (§3.2). +- **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 preserves the NAME and drops the description** at + narrow widths — witnessed at a width where both cannot fit, because + a clipped name is worse than today's bare name. +- **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). From b3a6da2416c25187fa9c5890fc3bdc5bd5f7c121 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 13:51:32 +0200 Subject: [PATCH 2/7] docs: Discovery Stage 2 framing, revision 3 Three corrections, each to something revision 2 asserted without checking the mechanism it was reasoning about. All three verified in the tree. A ROUND-TRIP FREEZES NOTHING. Revision 2 proposed pinning the legacy variant with an encode/decode round-trip. That test uses the SAME types on both sides, so adding a field to `MinibufferPrompt` leaves it passing while every v12-v22 peer in the field breaks. The existing `minibuffer_prompt_round_trips_through_postcard` (src/protocol.rs:2363) is precisely that shape, and there are NO literal byte fixtures anywhere in the protocol tests today --- checked, not assumed. Only `assert_eq!(encoded, LEGACY_BYTES)` against bytes captured now can fail when the encoding changes. Two fixtures, open and clear, matching the shapes the semantic test already covers so the corpus is not a fresh judgement call. THE CACHE HAZARD I DESCRIBED CANNOT HAPPEN. Revision 2 argued for a per-variant cache key on the grounds that a peer reconnecting at a different version could have its first message suppressed as a duplicate. `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), so a cache never spans two versions and reconnecting creates a fresh state. One per-peer cache stands; the selection is a producer gate, `peer_knows_minibuffer_rows`, alongside the four such gates that already exist (:1410-1435). The test for the impossible condition is REMOVED rather than written --- a test that cannot fail passes forever and teaches the next reader that the hazard is real. The matched open/close family requirement is independent of caching and stands unchanged. "THE NAME MUST SURVIVE" IS NOT ACHIEVABLE. Prompt plus typed input consume the width budget first, so the remaining suffix space can be too small for even the bare name --- and revision 2's rule would then have forced a partial name, which reads as a DIFFERENT command. The rule is now ordered: if the whole name does not fit, omit the suffix entirely; only once it fits is a description attempted; if the description does not fit whole, drop it. The guarantee becomes "never a partial name", which is achievable, rather than "the name always survives", which is not. Witnessed at three widths. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/discovery-stage2-framing.md | 87 ++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 21 deletions(-) diff --git a/docs/discovery-stage2-framing.md b/docs/discovery-stage2-framing.md index b2a12af..a7b461a 100644 --- a/docs/discovery-stage2-framing.md +++ b/docs/discovery-stage2-framing.md @@ -1,8 +1,14 @@ # Discovery Stage 2 — M-x rows stop being bare names -**Status: framing pass, revision 2. Pre-implementation. Awaiting +**Status: framing pass, revision 3. Pre-implementation. Awaiting approval.** +**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 @@ -107,13 +113,27 @@ So: - **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. -- **Close and cache ordering.** The prompt is cached-compare - suppressed, so the cache key must be **per variant**, or a v23 peer - that reconnects at v22 (or vice versa across a restart) can have its - first message suppressed as a duplicate of one it never received. - **The close message must use the same variant family as the open** — - a `MinibufferPromptRows` session closed by a legacy clear is exactly - the kind of mismatch that leaves a popup on screen forever. +- **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 @@ -140,13 +160,22 @@ M-x buffer.sa [buffer.save — Write the buffer to its file] the bump. - **Only the selected candidate**, as today. This is a formatting change to an existing suffix, not a new surface. -- **Clipping is explicit**: the suffix is already written against - `max = term_size.cols` with a running `written` count. The **name - must survive clipping and the description is what gets truncated** — - a row that clips to `[buffer.sa…]` would be strictly worse than - today. If the terminal is too narrow for `name — ` plus one - character of description, **the description is dropped entirely** - rather than shown as an ellipsis stub. +- **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. @@ -255,9 +284,22 @@ the temptation arrives with the feature. observing "no error". - **A v23 peer receives `MinibufferPromptRows` and NOT the legacy variant** — the double-render guard. -- **A round-trip encode/decode of the frozen `MinibufferPrompt`** - pins its shape, so a later field addition to it fails a test rather - than silently breaking v12–v22. +- **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. - **The cache key is per variant**: a session that opens for a v23 peer and a later one for a v22 peer are not suppressed as duplicates of each other (§3.2). @@ -265,9 +307,12 @@ the temptation arrives with the feature. 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 preserves the NAME and drops the description** at - narrow widths — witnessed at a width where both cannot fit, because - a clipped name is worse than today's bare name. +- **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 From 0e260e03773920fadb473c5ba124c60d905b218f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 13:55:17 +0200 Subject: [PATCH 3/7] docs: clear three stale references revision 3 left behind Revision 3 corrected the cache model in section 3.2 and the clipping rule in 3.4, then failed to propagate either. Internal contradictions in a framing document are worse than the original errors: a later reader resolves them by picking whichever statement they read first. - Q#D2-4 still summarised the design as "per-variant cache keys", contradicting 3.2's corrected one-per-peer model. Now names the `peer_knows_minibuffer_rows` producer gate and the single per-peer cache, with a parenthetical recording that the sentence was stale rather than silently rewriting history. - Section 6 still required the cross-version cache test that revision 3 removed as unfalsifiable. Replaced with what IS assertable --- a v22 and a v23 peer attached simultaneously each receive their own variant and only their own --- and the bullet says why the other test is absent, so it does not get "restored" later by someone who notices the gap. - The active-work lane still called the framing revision 2. Swept for the same class of residue from the other two corrections ("the name must survive", "both frontends render"); the remaining hits are the notes ABOUT those corrections, which are deliberate. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 2 +- docs/discovery-stage2-framing.md | 20 ++++++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 352eb53..23c7d20 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -275,7 +275,7 @@ from #171 and #215. tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout discovery-stage2`. -- **Framing `docs/discovery-stage2-framing.md`, revision 2**, in +- **Framing `docs/discovery-stage2-framing.md`, revision 3**, in review. 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 diff --git a/docs/discovery-stage2-framing.md b/docs/discovery-stage2-framing.md index a7b461a..993ab75 100644 --- a/docs/discovery-stage2-framing.md +++ b/docs/discovery-stage2-framing.md @@ -248,14 +248,19 @@ later. 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 rev 2, in §3.1–3.2** +### 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, per-peer -selection, per-variant cache keys, and matched open/close families. +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 @@ -300,9 +305,12 @@ the temptation arrives with the feature. 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. -- **The cache key is per variant**: a session that opens for a v23 peer - and a later one for a v22 peer are not suppressed as duplicates of - each other (§3.2). +- **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** From 6eecbfcca112374e71adb38e5efd7836b4900b7e Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 14:00:53 +0200 Subject: [PATCH 4/7] docs: record discovery Stage 2 framing approval Revision 3 was approved 2026-08-09 at 6a9a25d, after the three stale internal references were cleared. The lane heading and the framing status line both still said "in review", which is the kind of contradiction that makes a reader trust whichever line they hit first. The lane entry now also records WHAT the three review rounds found, rather than only how many there were. A count says the doc was reviewed; the list says what class of error it was reviewed for, which is the part a later reader can act on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 12 +++++++++--- docs/discovery-stage2-framing.md | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 23c7d20..b13f606 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,7 +265,7 @@ 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 — BRANCHED, framing in review +## Discovery Stage 2 — BRANCHED, pre-implementation **Written with the lane's first commit**, per the standing correction from #171 and #215. @@ -275,8 +275,14 @@ from #171 and #215. tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout discovery-stage2`. -- **Framing `docs/discovery-stage2-framing.md`, revision 3**, in - review. Scope: `COHERENCE.md` §5's "M-x rows are still bare names". +- **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. diff --git a/docs/discovery-stage2-framing.md b/docs/discovery-stage2-framing.md index 993ab75..56da2bf 100644 --- a/docs/discovery-stage2-framing.md +++ b/docs/discovery-stage2-framing.md @@ -1,7 +1,7 @@ # Discovery Stage 2 — M-x rows stop being bare names -**Status: framing pass, revision 3. Pre-implementation. Awaiting -approval.** +**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 From 70e578142004734de97f21ca57afb192048d6a05 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 14:51:00 +0200 Subject: [PATCH 5/7] =?UTF-8?q?feat(discovery):=20M-x=20rows=20carry=20des?= =?UTF-8?q?criptions=20=E2=80=94=20protocol=20v22=20->=20v23?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Command.description` has always been required and has always been rendered by `help.list-commands`. It was missing at the one moment it would change a decision: the M-x row. This carries it there. COHERENCE.md §5's clause "M-x rows are still bare names", per docs/discovery-stage2-framing.md revision 3. ## The wire half is additive, and the old variant is FROZEN postcard is not self-describing: enum variants encode by index and fields by position. Widening `MinibufferPrompt.candidates` in place would make every v12–v22 peer MIS-DECODE the bytes rather than ignore them — and gating the widened form at `>= 23` would not rescue them either, because with only one variant to gate they would receive no minibuffer message at all. Compatibility requires the old shape to still exist AND still be sent. So `MinibufferPrompt` is retained unchanged for `12..=22`, and `MinibufferPromptRows { prompt, input, cursor, rows, selected, total }` is APPENDED as the final variant, carrying `MinibufferRow { label, detail: Option }`. A new row type, not `CompletionPopupRow`, whose `kind` is an LSP `CompletionItemKind` code with no honest value for a command (Q#D2-1). Exactly one of the two reaches any peer, ever. The producer selects on the session's negotiated version, so the CLOSE necessarily uses the same family as the OPEN — a rows session closed by a legacy clear leaves the dropdown on screen forever. The daemon's write loop gates both directions again, with the legacy gate written as a RANGE (`12..MINIBUFFER_ROWS_MIN_VERSION`) rather than a floor, so a v23 peer cannot receive both and double-render. `ADVERTISED_PROTOCOL_VERSION` stays 20, untouched. ## The TUI half involves no wire at all `src/editor.rs` contains zero references to `MinibufferPrompt`: `paint_minibuffer` reads `core.minibuffer` directly. So it reads `Command.description` from the registry in-process, which is why this half is independent of the bump. Clipping is three ORDERED steps (§3.4), and the guarantee is "never a PARTIAL name", not "the name always survives" — the prompt and typed input consume the budget first, so the remainder can be too small even for the bare name. If the whole name does not fit, the suffix is omitted entirely; only once it fits is a description attempted; a description that does not fit whole is dropped, leaving today's `[name]`. No ellipsis stub, and no prefix of a name is ever emitted. ## Verification `src/protocol.rs` gains this repo's FIRST literal postcard byte fixtures: `minibuffer_prompt_v12_wire_bytes_are_frozen`, open and cleared. A round-trip freezes nothing — it encodes and decodes with the same types, so a field addition leaves it passing while every shipped peer breaks. Bite-verified: reordering two fields of `MinibufferPrompt` leaves `minibuffer_prompt_round_trips_through_postcard` green and fails the fixture. `line_wrap_facts_encoding_is_unchanged_by_the_v23_build` pins the PREVIOUS final variant, per the handoff §4 rule that an appended variant's own round-trip cannot detect a discriminant shift. `tests/discovery_stage2_acceptance.rs` runs ONE daemon serving a v22 and a v23 session simultaneously, through the real M-x key path, and asserts each receives its own variant AND ONLY its own — open and close alike — by collecting every minibuffer message rather than filtering for the expected one. No cross-version cache test, deliberately (§3.2): `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. Five version assertions updated, each read before editing: `src/protocol.rs` (the `PROTOCOL_VERSION` tripwire, renamed; and the v6-floor ladder's accepted/rejected ranges), `tests/statusline_segments_acceptance.rs`, `tests/bottom_panel_stage2b_gpu_acceptance.rs`, `tests/vterm_stage3_acceptance.rs`. No `ADVERTISED_PROTOCOL_VERSION` assertion fired. Gates: `scripts/gate --protocol --acceptance discovery_stage2_acceptance` — all ten green, including the strengthened two-configuration sweep. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- COHERENCE.md | 31 +- docs/active-work.md | 33 +- docs/agent-handoff.md | 21 +- pmacs-gpu/src/main.rs | 209 +++++++- pmacs-protocol/src/lib.rs | 11 +- pmacs-protocol/src/message.rs | 122 ++++- src/daemon.rs | 34 +- src/editor.rs | 64 ++- src/frontend.rs | 4 + src/protocol.rs | 145 ++++- src/semantic_render.rs | 269 +++++++++- tests/bottom_panel_stage2b_gpu_acceptance.rs | 13 +- tests/discovery_stage2_acceptance.rs | 537 +++++++++++++++++++ tests/statusline_segments_acceptance.rs | 9 +- tests/vterm_stage3_acceptance.rs | 5 +- 15 files changed, 1406 insertions(+), 101 deletions(-) create mode 100644 tests/discovery_stage2_acceptance.rs 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 b13f606..3323bca 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,7 +265,7 @@ 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 — BRANCHED, pre-implementation +## Discovery Stage 2 — IMPLEMENTED, no PR yet **Written with the lane's first commit**, per the standing correction from #171 and #215. @@ -299,9 +299,34 @@ tip** — the ref, not a SHA. Recover with `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 ` — - the strengthened two-configuration sweep, which is what `--protocol` - exists for. +- **Gates:** `scripts/gate --protocol --acceptance + discovery_stage2_acceptance` — the strengthened two-configuration + sweep, which is what `--protocol` exists for. +- **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. +- **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 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/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 52a2a29..cbb58e3 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,101 @@ 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 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 +16121,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 +16339,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 +17262,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/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..7050023 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,26 @@ 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. + let suffix = match session.selected.and_then(|idx| session.candidates.get(idx)) { + Some(cand) => { + let detail = matches!( + session.source, + crate::minibuffer::CompletionSource::Commands + ) + .then(|| commands.get(cand).map(|c| c.description.as_str())) + .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..2e62952 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,44 @@ 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| { + let detail = commands + .get(&label) + .map(|command| command.description.clone()); + 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 +1760,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 +5860,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 +5904,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 +5913,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..f290368 --- /dev/null +++ b/tests/discovery_stage2_acceptance.rs @@ -0,0 +1,537 @@ +// 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, 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, InstanceMessage, Key, KeyEvent, + Modifiers, SessionBootstrapRequest, +}; +#[cfg(feature = "crdt")] +use pmacs_protocol::transport::{read_message, write_message}; +#[cfg(feature = "crdt")] +use pmacs_protocol::{ByteRange, MinibufferRow}; + +#[cfg(feature = "crdt")] +use common::daemon::{TestDaemon, build_default_caps}; + +// --------------------------------------------------------------------------- +// 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 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:?}" + ); +} + +#[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"), From bf561ff29644afd7de7daadf184ceda609012e5b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 15:19:58 +0200 Subject: [PATCH 6/7] docs: record PR #228 and its merge block The lane heading said "no PR yet". It also needs to carry WHY the PR is merge-blocked, because a reader who finds only "blocked" will treat it as backlog hygiene and unblock it by rerunning the gate. The problem is gate integrity. --protocol promises the CRDT workspace sweep, that sweep has a documented precondition (handoff section 5), and the script does not run it --- confirmed by reading the plan emitter, not inferred from the failure. So a --protocol result can be decided by whether the build directory happened to contain pmacs-gpu rather than by the diff under test. It was latent until #225 gave each worktree its own target directory. A shared target dir usually already had pmacs-gpu built, which satisfied the precondition by accident and hid the omission. Unblocking needs both halves recorded: the scripts/gate repair as its own framing and PR, and then a fresh-target rerun of this branch protocol gate under the repaired script. A rerun alone would reproduce the same accident. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/docs/active-work.md b/docs/active-work.md index 3323bca..57788bc 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,7 +265,29 @@ 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 — IMPLEMENTED, no PR yet +## 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. From 5cf1d61b92e0515ea561a888e6dcbc26691037da Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:31:22 +0200 Subject: [PATCH 7/7] fix(discovery): clip command descriptions at the single-row surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #228 review found a correctness gap this lane made reachable. The GPU dropdown derives its height, its visible window and its selection-highlight offset from `rows.len()` — ONE logical row per candidate — while a detail carrying a line break shapes into more physical lines than that. One such row misaligns every row below it and the highlight with it. The grid TUI has the same exposure from the other side: it writes the description into a single-row suffix on the minibuffer band. ## Why not reject CR/LF at registration That was the obvious fix. It was implemented, measured, and abandoned on evidence. MCP tool registration renders a whole schema block into `Command.description` — tool text, blank line, `Arguments:`, then one line per argument (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, a `table.concat(lines, "\n")`, used at `:496`). And `tests/m9_6_acceptance.rs:583-598` ASSERTS four of those lines. A one-line guard in `CommandRegistry::define` fails 36 tests across `m9_6` (19/25), `m9_7` (16/19) and `m9_8` (1/17), in both feature configurations, and could only be made green by deleting a shipped acceptance criterion. So the one-line constraint goes where the constraint actually is: the surfaces that have one row. `Command.description` stays free-form, which it legitimately is. ## The change `Command::description_first_line` clips to the first CR **or** LF — a lone CR ends a line too, and an LF-only clip would pass a bare `\r` straight through to the same surface. Both single-row consumers call it: the semantic producer filling `MinibufferRow.detail` (`src/semantic_render.rs`) and the TUI suffix (`src/editor.rs`). A first line that is empty ships as `None` rather than `Some("")`, which would draw trailing padding. No ellipsis or truncation marker, matching the in-tree precedent and the minibuffer's own width rule. `describe-command` and `help.list-commands` are untouched and still report every line. That is what makes this a rendering decision rather than data loss, and it is asserted, not assumed. ## Precedent, already in this tree The same MCP fixture clips a tool RESULT to its first line because "a multi-line set_status would corrupt the row layout" (`init.lua:277-285`), leaving width clipping to the frontend. Same hazard class, same resolution. ## Verification `src/command.rs`: a schema block registers AND clips, in all three break forms; a single-line description is byte-identical after the clip; an empty first line clips to empty. `tests/discovery_stage2_acceptance.rs`: an MCP-shaped description reaches the TUI band and the GPU row as one line, through the real prompt path — with the full text still reachable via `describe-command` asserted alongside, so a clip that deleted the schema block everywhere would fail rather than pass. `pmacs-gpu`: one physical shaped line per logical candidate row — the geometry invariant the dropdown depends on. Mutation-checked: neutering `first_line` to the identity fails all four new break-handling tests (`a_multi_line_description_registers_and_clips_to_its_first_line`, `a_description_whose_first_line_is_empty_clips_to_empty`, `a_multi_line_description_reaches_the_tui_band_as_one_line`, `a_multi_line_description_reaches_the_gpu_row_as_one_physical_line`) and leaves the two "did not tighten past purpose" tests green. `Command.description`'s doc comment claimed "one-line", which the MCP path openly violates. It now states the real contract and records why a registration guard must not be re-proposed. `m9_6`/`m9_7`/`m9_8` pass COMPLETELY UNTOUCHED, and are now named gate suites so that stays on the record. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 41 ++++++- pmacs-gpu/src/main.rs | 11 ++ src/command.rs | 147 ++++++++++++++++++++++++- src/editor.rs | 11 +- src/semantic_render.rs | 19 +++- tests/discovery_stage2_acceptance.rs | 157 ++++++++++++++++++++++++++- 6 files changed, 376 insertions(+), 10 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 57788bc..ad3851a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -322,13 +322,50 @@ tip** — the ref, not a SHA. Recover with local formatting change reading the registry directly. A multi-row TUI chooser is explicitly NOT this lane. - **Gates:** `scripts/gate --protocol --acceptance - discovery_stage2_acceptance` — the strengthened two-configuration - sweep, which is what `--protocol` exists for. + discovery_stage2_acceptance --acceptance m9_6_acceptance --acceptance + m9_7_acceptance --acceptance m9_8_acceptance` — the strengthened + two-configuration sweep, which is what `--protocol` exists for. The + three m9 suites are named because the PR #228 review round measured + them as this change's blast radius (see the description-clip bullet); + their continued passing is on the record rather than assumed. + **`--protocol` does NOT run its own documented precondition** + (`cargo build --workspace --no-default-features --features + luajit,crdt`, handoff §5) — run it by hand first or twelve + `gpu_invocation_acceptance` tests fail on a missing `pmacs-gpu` + binary. That omission is the `gate-protocol-build` lane's, not this + one's. - **IMPLEMENTED.** `PROTOCOL_VERSION` is 23, `ADVERTISED_PROTOCOL_VERSION` is untouched at 20. New suite `tests/discovery_stage2_acceptance.rs`; the daemon half is `crdt`-gated (a semantic session is necessarily a text replica) and runs one daemon serving a v22 and a v23 session simultaneously. +- **Multi-line descriptions are clipped AT THE SURFACE, and + registration-level rejection was investigated and REJECTED ON + EVIDENCE — do not re-propose it.** PR #228 review found the real + hazard: the GPU dropdown derives its height, visible window and + highlight offset from `rows.len()` (one logical row per candidate), + so a detail carrying a line break misaligns every row below it; the + TUI writes into a single-row band. The obvious fix — reject CR/LF in + `CommandRegistry::define` — was implemented and measured, and it + **fails 36 tests across `m9_6`/`m9_7`/`m9_8`**, because MCP tool + registration renders a whole schema block into `description` + (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, + `table.concat(lines, "\n")`, used at `:496`) and + **`tests/m9_6_acceptance.rs:583-598` asserts four separate lines of + it** — tool text, `Arguments:`, and two per-argument lines. No + single-line rendering satisfies those assertions, so a registry guard + could only go green by deleting a shipped acceptance criterion. + The one-line constraint belongs to the surfaces that have it: + `Command::description_first_line` clips, both single-row consumers + call it, and the full text still reaches `describe-command` / + `help.list-commands` untouched. Precedent already in-tree — the same + MCP fixture clips a tool RESULT to its first line because *"a + multi-line set_status would corrupt the row layout"* (`:277-285`). + **A startup census is not a corpus census**: booting an + `EditorState` and scanning all 180 registered descriptions found zero + offenders, because MCP registers at RUNTIME and builds the string by + concatenation — invisible to both that census and a grep for literals. + The workspace sweep is what caught it. - **The freeze is enforced by LITERAL byte fixtures**, not a round-trip — `minibuffer_prompt_v12_wire_bytes_are_frozen` in `src/protocol.rs`, the first such fixture in this repo. Bite-verified: reordering two diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index cbb58e3..78eb64a 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -15482,6 +15482,17 @@ mod tests { Some("buffer.kill"), "a row with no detail renders the bare label, exactly as before v23: {lines:?}" ); + // The geometry invariant the dropdown depends on: it derives + // its height, its visible window and its selection-highlight + // offset from `rows.len()`, so ONE physical line per logical + // row is what keeps those aligned. The daemon clips a detail to + // its first line (`Command::description_first_line`) precisely + // so this holds for an MCP schema block. + assert_eq!( + lines.len(), + state.minibuffer.as_ref().map_or(0, |mb| mb.rows.len()), + "one physical line per candidate row: {lines:?}" + ); // The frozen `12..=22` form, which an older daemon still sends: // bare strings become detail-free rows. diff --git a/src/command.rs b/src/command.rs index 2b21d7a..ec93899 100644 --- a/src/command.rs +++ b/src/command.rs @@ -66,7 +66,28 @@ impl SourceLocation { pub struct Command { /// Unique name (e.g. `buffer.save`). pub name: String, - /// One-line human-readable description (R42, required). + /// Human-readable description. Required and non-empty after trim + /// (R42), but otherwise **free-form, and legitimately multi-line**. + /// + /// # Do not add a registration-time one-line guard + /// + /// This doc used to read "one-line human-readable description", + /// which was an aspiration rather than the contract: MCP tool + /// registration renders a whole schema block in here — the tool's + /// text, a blank line, `Arguments:`, then one line per argument + /// (`tests/fixtures/pmacs-mcp-tools/init.lua:272`, a + /// `table.concat(lines, "\n")`) — and `m9_6_acceptance.rs:583-598` + /// asserts all four of those lines. Rejecting CR/LF in + /// [`CommandRegistry::define`] was tried, measured, and abandoned: + /// it fails 36 tests across `m9_6`/`m9_7`/`m9_8` and could only be + /// made green by deleting a shipped acceptance criterion. + /// + /// The one-line constraint belongs to the **surfaces that have + /// it**, so a consumer rendering into a single row clips with + /// [`Self::description_first_line`] — the minibuffer band and the + /// completion dropdown both do. The full text stays intact for + /// `describe-command` and `help.list-commands`, which is what keeps + /// this a rendering decision rather than data loss. pub description: String, /// Where the command was defined. pub source: SourceLocation, @@ -79,6 +100,53 @@ pub struct Command { pub predicate: Option, } +impl Command { + /// [`Self::description`] clipped to its first line, for a consumer + /// rendering into a surface that has exactly one row. + /// + /// The description is free-form and may carry a whole schema block + /// (see that field). Two surfaces cannot show one: the grid TUI + /// writes the selected candidate into a single-row suffix on the + /// minibuffer band, and the GPU dropdown derives its height, its + /// visible window and its selection-highlight offset from + /// `rows.len()` — **one logical row per candidate** — so a detail + /// that shapes into more physical lines than that misaligns every + /// row below it and the highlight with it. + /// + /// Clipping here rather than refusing at registration follows the + /// precedent already in this tree: the MCP fixture's result + /// delivery keeps only the first line of a tool result because + /// *"a multi-line `set_status` would corrupt the row layout"* + /// (`tests/fixtures/pmacs-mcp-tools/init.lua:277-285`), leaving + /// width clipping to the frontend. Same hazard class, same + /// resolution. + /// + /// **No ellipsis or truncation marker**, matching that precedent + /// and the minibuffer's own width rule, which rejects stub markers + /// for the same reason: the full text is one `describe-command` + /// away, and a marker in a candidate row reads as part of the + /// candidate. + #[must_use] + pub fn description_first_line(&self) -> &str { + first_line(&self.description) + } +} + +/// The prefix of `text` before its first line break. +/// +/// Breaks on CR **or** LF, not LF alone: a lone CR ends a line on +/// classic-Mac-era input and is the leading half of a CRLF, so an +/// LF-only clip would pass a bare `\r` straight through to a +/// single-row surface — and a CR-only clip would do the same for `\n`. +/// Splitting on the first of either handles all three forms with one +/// scan, since CRLF's `\r` comes first. +fn first_line(text: &str) -> &str { + match text.find(['\n', '\r']) { + Some(break_at) => &text[..break_at], + None => text, + } +} + /// Errors raised by the command registry. #[derive(Debug, Error)] pub enum CommandError { @@ -271,6 +339,83 @@ mod tests { )); } + #[test] + fn a_multi_line_description_registers_and_clips_to_its_first_line() { + // Registration accepts it — MCP tool registration renders a + // whole schema block into `description` and + // `m9_6_acceptance.rs:583-598` asserts four of its lines, so a + // one-line guard here would delete a shipped contract. The + // one-line constraint lives at the single-row surfaces, which + // read `description_first_line`. + // + // All three break forms: a clip that split on `\n` alone would + // pass a bare `\r` through, and one that split on `\r` alone + // would pass `\n` through. + let lua = Lua::new(); + for (label, description) in [ + ( + "LF", + "Greet someone.\n\nArguments:\n name (string, required)", + ), + ( + "CR", + "Greet someone.\r\rArguments:\r name (string, required)", + ), + ( + "CRLF", + "Greet someone.\r\n\r\nArguments:\r\n name (string, required)", + ), + ] { + let mut r = CommandRegistry::new(); + r.define(make_command(&lua, "mcp.greet", description)) + .unwrap_or_else(|e| panic!("{label}: a schema block must still register: {e}")); + let cmd = r.get("mcp.greet").expect("registered"); + assert_eq!( + cmd.description, description, + "{label}: the registry stores the description verbatim — the clip is a \ + rendering decision, so `describe-command` must still see every line" + ); + assert_eq!( + cmd.description_first_line(), + "Greet someone.", + "{label}: a single-row surface gets the first line only" + ); + assert!( + !cmd.description_first_line().contains(['\n', '\r']), + "{label}: the clipped form must carry no break at all" + ); + } + } + + #[test] + fn a_single_line_description_is_byte_identical_after_the_clip() { + // The other half: the clip must not tighten past its purpose. + // Interior whitespace, punctuation and non-ASCII all survive, + // and there is no ellipsis or truncation marker. + let lua = Lua::new(); + let mut r = CommandRegistry::new(); + let description = "Write the buffer to its file — with a dash, and \ttabs."; + r.define(make_command(&lua, "buffer.save", description)) + .expect("registers"); + assert_eq!( + r.get("buffer.save").unwrap().description_first_line(), + description, + "a description with no break is returned unchanged" + ); + } + + #[test] + fn a_description_whose_first_line_is_empty_clips_to_empty() { + // The case the producer turns into `None` rather than + // `Some("")`: a leading break leaves nothing to render, and a + // `Some("")` detail would draw trailing padding after the label. + let lua = Lua::new(); + let mut r = CommandRegistry::new(); + r.define(make_command(&lua, "x", "\nArguments:\n a (string)")) + .expect("registers"); + assert_eq!(r.get("x").unwrap().description_first_line(), ""); + } + #[test] fn empty_name_is_rejected() { let lua = Lua::new(); diff --git a/src/editor.rs b/src/editor.rs index 7050023..0e78fe8 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -5575,13 +5575,22 @@ fn paint_minibuffer( // // Q#D2-2: only the command source has a detail. A file-path or // buffer-name prompt renders exactly as it did before. + // + // FIRST LINE ONLY: this band is a single row, and + // `Command.description` is free-form — MCP registration renders a + // whole schema block into it. The full text stays reachable through + // `describe-command`. let suffix = match session.selected.and_then(|idx| session.candidates.get(idx)) { Some(cand) => { let detail = matches!( session.source, crate::minibuffer::CompletionSource::Commands ) - .then(|| commands.get(cand).map(|c| c.description.as_str())) + .then(|| { + commands + .get(cand) + .map(crate::command::Command::description_first_line) + }) .flatten(); minibuffer_candidate_suffix(cand, detail, max.saturating_sub(col)) } diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 2e62952..3359504 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -1725,9 +1725,26 @@ impl SemanticRenderState { labels .into_iter() .map(|label| { + // FIRST LINE ONLY. `Command.description` + // is free-form and MCP registration puts + // a whole schema block in it, while the + // dropdown sizes itself from + // `rows.len()` — one logical row per + // candidate. Shipping the block would + // shape into more physical lines than + // the geometry accounts for and + // misalign every row below it. The full + // text stays reachable through + // `describe-command`. let detail = commands .get(&label) - .map(|command| command.description.clone()); + .map(|command| command.description_first_line().to_owned()) + // A description whose first line is + // empty (`"\nArguments:…"`) carries + // nothing to render, so it ships as + // absent rather than as `Some("")`, + // which would draw trailing padding. + .filter(|detail| !detail.is_empty()); MinibufferRow { label, detail } }) .collect() diff --git a/tests/discovery_stage2_acceptance.rs b/tests/discovery_stage2_acceptance.rs index f290368..571c605 100644 --- a/tests/discovery_stage2_acceptance.rs +++ b/tests/discovery_stage2_acceptance.rs @@ -34,7 +34,8 @@ use std::path::Path; use pmacs::bootstrap::BootstrapRoots; use pmacs::editor::EditorState; use pmacs_protocol::{ - ADVERTISED_PROTOCOL_VERSION, PROTOCOL_VERSION, is_supported_protocol_version, + ADVERTISED_PROTOCOL_VERSION, ByteRange, InstanceMessage, MinibufferRow, PROTOCOL_VERSION, + is_supported_protocol_version, }; #[cfg(feature = "crdt")] @@ -46,13 +47,11 @@ use std::time::{Duration, Instant}; use pmacs_protocol::cell::CellSize; #[cfg(feature = "crdt")] use pmacs_protocol::message::{ - AttachRequest, FrontendCapabilities, FrontendEvent, Hello, InstanceMessage, Key, KeyEvent, - Modifiers, SessionBootstrapRequest, + AttachRequest, FrontendCapabilities, FrontendEvent, Hello, Key, KeyEvent, Modifiers, + SessionBootstrapRequest, }; #[cfg(feature = "crdt")] use pmacs_protocol::transport::{read_message, write_message}; -#[cfg(feature = "crdt")] -use pmacs_protocol::{ByteRange, MinibufferRow}; #[cfg(feature = "crdt")] use common::daemon::{TestDaemon, build_default_caps}; @@ -148,6 +147,36 @@ fn bottom_row(s: &EditorState, rows: u32, cols: u32) -> String { row.into_iter().collect::().trim_end().to_owned() } +/// Open `M-x` narrowed to `zzprobe` and return the candidate rows the +/// semantic producer ships to a current-wire peer. +/// +/// Through `SemanticRenderState` and the real minibuffer session rather +/// than by constructing a message: the clip lives in the producer, so a +/// hand-built row would skip the thing under test. +fn mx_rows(s: &EditorState) -> Vec { + let bid = s.core.borrow().active_buffer_id(); + let mut render = pmacs::semantic_render::SemanticRenderState::for_peer( + pmacs::protocol::FrontendId::LOCAL, + PROTOCOL_VERSION, + ); + render.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); + let _ = render.render_frame(s); + + exec( + s, + "pmacs.minibuffer.read{ prompt = 'M-x ', source = 'commands', on_accept = function() end }", + ); + exec(s, "pmacs.minibuffer.set_contents('zzprobe')"); + render + .render_frame(s) + .into_iter() + .find_map(|msg| match msg { + InstanceMessage::MinibufferPromptRows { rows, .. } => Some(rows), + _ => None, + }) + .expect("the producer ships a rows prompt") +} + /// Open `M-x`, narrowed to exactly one command with a known /// description, and report the bottom row at `cols` columns. fn mx_bottom_row(s: &EditorState, cols: u32) -> String { @@ -254,6 +283,124 @@ fn a_source_with_no_detail_renders_exactly_as_before_in_the_tui() { ); } +// --------------------------------------------------------------------------- +// Multi-line descriptions reach single-row surfaces as ONE line +// --------------------------------------------------------------------------- + +/// An MCP-shaped description: tool text, blank line, `Arguments:`, then +/// one line per argument. +/// +/// This is the real shape, not an invented one — +/// `tests/fixtures/pmacs-mcp-tools/init.lua:272` builds it with +/// `table.concat(lines, "\n")` and `m9_6_acceptance.rs:583-598` asserts +/// four of its lines, which is why registration accepts it and the +/// SURFACES clip instead. +const MCP_SHAPED: &str = "Greet someone.\\n\\nArguments:\\n name (string, required)"; + +fn define_multiline_probe(s: &EditorState, name: &str, description: &str) { + exec( + s, + &format!( + "pmacs.command.define{{ name = '{name}', description = \"{description}\", \ + fn = function() end }}" + ), + ); +} + +#[test] +fn a_multi_line_description_reaches_the_tui_band_as_one_line() { + let s = session("tui-multiline"); + define_multiline_probe(&s, "zzprobe", MCP_SHAPED); + let row = mx_bottom_row(&s, 200); + assert!( + row.contains("[zzprobe — Greet someone.]"), + "the band shows the first line only: {row:?}" + ); + assert!( + !row.contains("Arguments:"), + "the schema block must not reach a single-row band: {row:?}" + ); + // `bottom_row` reads one grid row, so anything below would be lost + // rather than visibly wrong — assert on the registry-side clip too, + // which is what the painter consumed. + let clipped: String = eval(&s, "return pmacs.describe.command('zzprobe').description"); + assert!( + clipped.contains("Arguments:"), + "describe-command must still see the WHOLE description, or the clip \ + silently deleted the schema block everywhere: {clipped:?}" + ); +} + +#[test] +fn a_multi_line_description_reaches_the_gpu_row_as_one_physical_line() { + // The geometry hazard, through the real prompt path: the dropdown + // sizes itself from `rows.len()` — one logical row per candidate — + // so a detail carrying a break would shape into more physical lines + // than the geometry accounts for. + // + // All three break forms, since a clip handling only LF would pass a + // bare CR through to the same surface. + for (label, description, tail) in [ + ("LF", MCP_SHAPED, "Arguments:"), + ( + "CR", + "Greet someone.\\r\\rArguments:\\r name (string, required)", + "Arguments:", + ), + ( + "CRLF", + "Greet someone.\\r\\n\\r\\nArguments:\\r\\n name (string, required)", + "Arguments:", + ), + ] { + let s = session(&format!("gpu-multiline-{label}")); + define_multiline_probe(&s, "zzprobe", description); + let rows = mx_rows(&s); + let probe = rows + .iter() + .find(|row| row.label == "zzprobe") + .unwrap_or_else(|| panic!("{label}: the probe command is a candidate")); + let detail = probe + .detail + .as_deref() + .unwrap_or_else(|| panic!("{label}: the row carries a detail")); + assert_eq!( + detail, "Greet someone.", + "{label}: the wire row carries the first line only" + ); + assert!( + !detail.contains(['\n', '\r']), + "{label}: a row detail must carry no line break: {detail:?}" + ); + assert!( + !detail.contains(tail), + "{label}: the schema block must not reach the dropdown" + ); + + // And the full text is still there for the discoverability + // path, which is what makes this a rendering decision. + let full: String = eval(&s, "return pmacs.describe.command('zzprobe').description"); + assert!( + full.contains("name (string, required)"), + "{label}: describe-command must still report every line: {full:?}" + ); + } +} + +#[test] +fn a_single_line_description_is_unchanged_on_the_wire() { + // The clip did not tighten past its purpose: a description with no + // break reaches the row byte-identical, with no truncation marker. + let s = session("wire-single-line"); + define_probe(&s); + let rows = mx_rows(&s); + let probe = rows + .iter() + .find(|row| row.label == "zzprobe") + .expect("the probe command is a candidate"); + assert_eq!(probe.detail.as_deref(), Some(PROBE_DESCRIPTION)); +} + #[test] fn typed_but_unmatched_input_is_still_accepted() { // Q#D2-5, the trap this lane arrives with: richer rows make `M-x`