From 1b1e5990709842706d7b1b70054dd08d0095ff47 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 10:10:27 -0400 Subject: [PATCH 1/6] feat(terminal): copy mode over retained scrollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 2 of docs/terminal-config-and-copy-mode-framing.md (rev 4, approved). `M-x terminal.copy-mode`, or `C-t` in a terminal buffer — physically `C-c C-t`, since every unescaped key goes to the child — materializes the retained scrollback into an ordinary read-only, path-less buffer, with `g` to re-snapshot and `q` to return. No protocol change. Materializing is the whole design. isearch, motion, selection and the kill ring work with no new substrate because the snapshot is a rope, so SearchStore and the existing match painting apply unchanged. And "keys must not reach the child" dissolves structurally rather than being guarded: the transport arm keys on is_terminal(buffer_id), and a snapshot is not a terminal, so the arm never fires. The dispatch-shadow count stays at six and describe-key keeps telling the truth — asserted directly, since that is the observable difference between the buffer-local idiom and a shadow. One serializer, not two (Q#TC7). `copy_retained` builds a whole-range selection and hands it to `copy_selection_bytes`; a second walk would re-derive soft-wrap joining, wide-glyph continuation, cluster bytes and per-row trailing-blank trimming, and the two would drift. Four unit pins in view.rs assert exact bytes against the same projection fixtures that pin the serializer itself. Q#TC6a is implemented as two calls, and the second is the load-bearing one: an intercept guards dispatch only, and no Lua binding sets Buffer::read_only, so set_round_trip_input is what keeps a replica frontend from applying optimistically and emitting an op that would pass ensure_writable and mutate both sides. Acceptance 16 pins that UNGATED, because CI never compiles the crdt feature. Eight of nine criteria. Criterion 17's semantic-frontend end-to-end pin is deliberately absent: the optimistic apply lives only in pmacs-gpu/src/main.rs and the headless SemanticClient has no optimistic path, so a faithful test needs the real GPU binary — the a37 foundation, which CI never compiles, silently returns ok when the binary is unbuilt, and is load-sensitive. Both halves of the mechanism are pinned ungated instead (16, and 16b for the hazard); the wire-level half stays an explicit obligation of the CI crdt-coverage lane. Substrate fact found while wiring lifecycle: TerminalManager::prune REACTS to a buffer already gone from the registry rather than removing one, so a child exiting leaves both the terminal and its snapshot alive. That is why on_removed is a sound teardown hook, and why a finished command's output stays readable. Five bites, five different wrong implementations, each failing exactly one test: removing set_round_trip_input fails acceptance 16 in the DEFAULT configuration; a naive independent serializer fails all four unit pins, with the diffs naming each drift mode; making re-invoke create a fresh buffer fails 18; dropping the kill-with-terminal teardown fails 18; removing the intercept fails 16b. COHERENCE.md: §6 gains this as the worked example that a modal-looking feature need not become a shadow; §11 records the scope="global" deferral's second live case, making the argument for both registry deferrals cumulative; §2 step 8 gains copy mode and keeps the still-missing close command named. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 34 +- builtin/runtime/terminal.lua | 186 +++++++ docs/active-work.md | 49 +- docs/terminal-config-and-copy-mode-framing.md | 35 +- src/lua_bindings/mod.rs | 19 + src/terminal/view.rs | 143 +++++ tests/terminal_copy_mode_acceptance.rs | 516 ++++++++++++++++++ 7 files changed, 970 insertions(+), 12 deletions(-) create mode 100644 tests/terminal_copy_mode_acceptance.rs diff --git a/COHERENCE.md b/COHERENCE.md index 4e7361c..f707234 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -368,7 +368,7 @@ Full verdict table: | 5 | Edit | **Works** | Full CUA + Emacs keymap in 161 lines (`builtin/keymaps/default.lua`); isearch, query-replace, kill ring, undo/redo, auto-indent/pair/comment, atomic save. Genuinely excellent zero-config | | 6 | Language intelligence | **Partial** | Rust grammar bundled and auto-attaches; rust-analyzer preconfigured (`builtin/runtime/lsp.lua:44-52`) — but a missing binary fails silently (§1.2) and highlighting masks it. No LSP status command exists to diagnose | | 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing PR #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | -| 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173). Named limitation: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | +| 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173), and searchable through `M-x terminal.copy-mode` / `C-c C-t`, which materializes the retained scrollback into an ordinary read-only buffer (Stage 2). Named limitations: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there; and there is still **no close/kill command**, which is the remaining half of this step's discoverability gap. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | | 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | | 11 | See background work | **Works but undiscoverable** | `*workers*` view via `M-x editor.list-workers`; `C-c C-k` cancel-at-point. No keybinding, no statusline spinner/progress indicator anywhere (§9) | @@ -658,6 +658,25 @@ Facts that define the gap: a terminal buffer. Since #173 that chord is `terminal.escape-key` rather than a hardcoded `C-c`, so a user can *move* which prefix is eaten; they cannot make the shadow stop eating one. +- **A worked example that a modal-*looking* feature need not become a + shadow.** Terminal copy mode (Stage 2 of the terminal-config arc) is + the case that most invited a seventh rung: it wants motion, search and + its own `g`/`q` inside a surface where every unescaped key otherwise + goes to a child process. It resolves to the buffer-local keymap idiom + instead, by **materializing** the retained scrollback into an ordinary + read-only document buffer. The keys-must-not-reach-the-child problem + then dissolves structurally rather than being guarded: the transport + arm keys on `is_terminal(buffer_id)`, and a snapshot buffer is not a + terminal, so the arm never fires. No new precedence rung, no new + hand-synced guard-list entry, and `describe-key` keeps reporting the + truth — pinned by asserting exactly that for the snapshot's `g` and + `q`, which is the observable difference between the idiom and a + shadow. **The count stays at six.** + + The transferable rule: when a feature wants a keymap over *content*, + ask whether the content can become a buffer. The shadows that exist + are the cases where it genuinely cannot (a minibuffer prompt, a + live search prompt) — not the cases where nobody tried. - **No transient-keymap mechanism exists to migrate to.** `KeymapStack` has exactly three fixed scopes — `Buffer(BufferId)`, `Mode(String)`, `Global` (`src/keymap_stack.rs:37-44`); resolution order buffer → @@ -1041,6 +1060,19 @@ layering, provenance, and adoption have not followed.** scalars. It is the clearest evidence yet that table-valued settings are the blocking prerequisite: the terminal is now half-registered, and no settings UI can render the half that matters most. +- **The missing `scope = "global"` flag has its second live case.** After + `autosave.interval-ms`, the terminal's two *open-time* settings — + `terminal.default-profile` and `terminal.scrollback-rows` — are read + before their terminal's identity buffer exists, so a buffer-local + override can never be consulted. The registry accepts `set_local` on + them anyway, because `Live` mutability is all it can express. Nothing + breaks; the setting simply has no effect, which is the worst shape a + configuration surface can take. `terminal.escape-key` is the contrast + that shows this is a real distinction rather than a blanket wish: it + *deliberately* supports buffer-locals, and per-terminal escapes are a + feature. So the argument for both deferrals is now **cumulative and + concrete** rather than hypothetical — two adopters, two distinct + missing primitives, one feature. - **No persistence**: settings changed at runtime do not survive restart (the `custom-file` split-brain question is a named deferral). - The three-level separation holds in principle today (registry / diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index 143a663..b44e824 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -49,6 +49,18 @@ local function bind_terminal_keys(buffer) bind("C-v", "terminal.page-down") bind("M-<", "terminal.scroll-oldest") bind("M->", "terminal.scroll-bottom") + -- Q#TC8a/Q#TC9: copy mode is ADDITIVE. The live keys above are + -- unchanged; this is one more leaf beside them. `C-t` is globally + -- `edit.transpose-chars`, which is meaningless in a read-only + -- terminal buffer, and binding it buffer-locally is the scoped + -- idiom rather than a shadow — `keymap.bind`'s strictness rejects + -- binding a PREFIX of an existing sequence within a scope, not + -- cross-scope shadowing. + -- + -- Physically typed as `C-c C-t`: in a terminal every unescaped key + -- goes to the child, so terminal-local bindings are reached through + -- the escape. That also matches emacs-libvterm's own chord. + bind("C-t", "terminal.copy-mode") end -- Q#TC1: profiles are a raw Lua table, not a config setting. The @@ -190,6 +202,180 @@ pmacs.command.define { -- `C-c` is consumed as the escape. `M-x terminal` still works there. pmacs.keymap.bind { scope = "global", sequence = "C-c t", command = "terminal" } +-- === Copy mode (Stage 2, Q#TC6) ========================================= +-- +-- `terminal.copy-mode` MATERIALIZES the retained rows into an ordinary +-- read-only document buffer instead of adding a modal state to the +-- terminal. That choice is the whole design: +-- +-- * isearch, motion, selection, `M-w` and the kill ring all work with no +-- new substrate — the snapshot is a rope, so `SearchStore` and the +-- existing match painting apply unchanged; +-- * "keys must not reach the child" dissolves structurally rather than +-- being guarded: the transport arm keys on `is_terminal(buffer)`, and +-- a snapshot buffer is not a terminal, so it never fires; +-- * the dispatch-shadow count stays at SIX (`COHERENCE.md` §6) and +-- `describe-key` keeps telling the truth, because the bindings are +-- buffer-local and inspectable. + +local raw_copy_retained = assert(terminal._copy_retained, + "pmacs.terminal._copy_retained is required") + +-- snapshot buffer name -> { terminal = , buffer = } +-- +-- Keyed by NAME, not by buffer handle: handles are not stable table keys, +-- and a name survives the user killing the snapshot (listview precedent). +local snapshots = {} + +local function buffer_name(buf) + local ok, described = pcall(pmacs.describe.buffer, buf) + if ok and described then return described.name end + return nil +end + +local function find_buffer_by_name(name) + for _, id in ipairs(pmacs.buffer.list()) do + local ok, described = pcall(pmacs.describe.buffer, id) + if ok and described and described.name == name then return id end + end + return nil +end + +-- `*terminal:bash*` -> `*terminal-copy: terminal:bash*`. The surrounding +-- asterisks are stripped before nesting so the result reads as one +-- generated-buffer name rather than two. +local function snapshot_name_for(term_buf) + local name = buffer_name(term_buf) or "terminal" + return string.format("*terminal-copy: %s*", (name:gsub("^%*", ""):gsub("%*$", ""))) +end + +-- Q#TC7: the snapshot text comes from the SAME serializer selection-copy +-- uses, so soft wraps, wide glyphs, clusters and trailing blanks cannot +-- drift between the two. +local function render_snapshot(record) + local text = raw_copy_retained(record.terminal) or "" + local buf = record.buffer + local len = buf:len() + -- Snapshot writes bypass the read-only intercept; everything else is + -- rejected by it. + if len > 0 then buf:delete(0, len, { bypass_intercept = true }) end + if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end +end + +local function ensure_snapshot(term_buf) + local name = snapshot_name_for(term_buf) + local record = snapshots[name] + if record and record.buffer:is_valid() then + -- Q#TC8: re-invoking refreshes IN PLACE. Retarget the terminal too, + -- in case a terminal buffer was recreated under the same name. + record.terminal = term_buf + return record + end + + local buf = find_buffer_by_name(name) or pmacs.buffer.create(name) + record = { terminal = term_buf, buffer = buf } + snapshots[name] = record + + -- Q#TC6a — BOTH calls, and the second is the load-bearing one. + -- + -- An intercept guards the dispatch/edit path only. It does NOT set + -- `Buffer::read_only` (deliberately independent), and no Lua binding + -- sets that flag at all, so an optimistic CRDT op from a semantic + -- frontend bypasses the intercept AND passes `ensure_writable()` — + -- mutating the daemon buffer in lockstep with the mirror, with no + -- divergence to notice. `set_round_trip_input` prevents that at the + -- only point it can be prevented: `dispatch_idle_for` reports false + -- while this buffer is focused, so the frontend never applies + -- optimistically and never emits the op. It is the guard, not + -- hardening. + pmacs.buffer.add_intercept(buf, function() + error(name .. " is read-only") + end) + pmacs.buffer.set_round_trip_input(buf, true) + + pmacs.keymap.bind { scope = "buffer", buffer = buf, + sequence = "g", command = "terminal.copy-refresh" } + pmacs.keymap.bind { scope = "buffer", buffer = buf, + sequence = "q", command = "terminal.copy-quit" } + + -- Q#TC8 lifecycle, both directions. Killing the terminal takes its + -- snapshot with it; killing the snapshot alone leaves the terminal + -- running and merely forgets the record, so a later invoke rebuilds. + -- + -- `on_removed` is sound here because every user-facing kill path + -- routes through `pmacs.buffer.kill`, which fires the callbacks. The + -- terminal manager's own `prune` does not — but it never removes a + -- buffer either; it REACTS to one already gone from the registry. A + -- child exiting therefore leaves both the terminal and its snapshot + -- alive, which is what makes reading back a finished command's output + -- work at all. + pcall(pmacs.buffer.on_removed, term_buf, function() + local current = snapshots[name] + if current and current.buffer:is_valid() then + pcall(pmacs.buffer.kill, current.buffer) + end + snapshots[name] = nil + end) + pcall(pmacs.buffer.on_removed, buf, function() + snapshots[name] = nil + end) + + return record +end + +-- The snapshot record whose buffer the active window shows, or nil. +local function snapshot_for_current_buffer() + local buf = pmacs.window.buffer() + if not buf then return nil end + local name = buffer_name(buf) + if not name then return nil end + return snapshots[name] +end + +function terminal.copy_mode(term_buf) + term_buf = term_buf or pmacs.window.buffer() + assert(term_buf, "terminal.copy-mode: no active buffer") + if not terminal.is_terminal(term_buf) then + error("terminal.copy-mode: the current buffer is not a terminal", 0) + end + local record = ensure_snapshot(term_buf) + render_snapshot(record) + pmacs.window.switch_buffer(record.buffer) + return record.buffer +end + +pmacs.command.define { + name = "terminal.copy-mode", + description = "Open a searchable read-only snapshot of this terminal's scrollback.", + fn = function() return terminal.copy_mode() end, +} + +pmacs.command.define { + name = "terminal.copy-refresh", + description = "Re-snapshot the source terminal into this copy buffer.", + fn = function() + local record = snapshot_for_current_buffer() + if not record then return end + if not record.terminal:is_valid() then + pmacs.editor.set_status("terminal.copy-refresh: the source terminal is gone") + return + end + render_snapshot(record) + end, +} + +pmacs.command.define { + name = "terminal.copy-quit", + description = "Return to the terminal this copy buffer was taken from.", + fn = function() + local record = snapshot_for_current_buffer() + if not record then return end + if record.terminal:is_valid() then + pmacs.window.switch_buffer(record.terminal) + end + end, +} + pmacs.command.define { name = "terminal.copy-selection", description = "Copy the active terminal selection.", diff --git a/docs/active-work.md b/docs/active-work.md index 88e70df..bfdb7ff 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -389,19 +389,52 @@ If it does not, stop and repair the remote/fetch configuration. **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; `git diff --check` clean. Gates were run against the committed tree. -## Terminal config + copy mode arc — Stage 1 IN REVIEW +## Terminal config + copy mode arc — Stage 1 MERGED; Stage 2 IN REVIEW - Approved framing: `docs/terminal-config-and-copy-mode-framing.md` **revision 4** (four review rounds), committed as the first commit of Stage 1's branch. Two stages, two branches, two PRs; **no protocol change**. -- **Stage 1 = `githubsucks/terminal-config`**, worktree - `../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120` - and merged up to `c93f9ee` during review round 1. Profiles, - scrollback, escape key, and the `C-c t` opening binding. -- **Stage 2 = `terminal-copy-mode`, not started.** Branch it off `main` - after Stage 1 merges: no dependency, but both edit - `builtin/runtime/terminal.lua`. +- **Stage 1 MERGED as #173** (`main` @ `cf54270`, 2026-07-26, one review + round, twelve checks green). Branch `githubsucks/terminal-config` and + worktree `../pmacs-terminal-config` retained. +- **Stage 2 = `githubsucks/terminal-copy-mode`**, worktree + `../pmacs-terminal-copy-mode`, based on `githubsucks/main` @ + `cf54270`. Copy mode: `M-x terminal.copy-mode` / `C-c C-t`. +- **Stage 2 ships eight of nine criteria, and the missing one is named.** + Criterion 17 (a real semantic frontend proving neither daemon buffer + nor mirror mutates) is **not pinned**: the optimistic apply exists only + in `pmacs-gpu/src/main.rs`, and the headless `SemanticClient` every + other semantic test uses has no optimistic path, so a faithful test + must drive the real GPU binary — the `a37` foundation, which CI never + compiles, silently skips without the binary, and is load-sensitive. A + second test on that footing buys the appearance of coverage. Both + halves of the mechanism are pinned **ungated** instead: acceptance 16 + (the guard is armed — `dispatch_idle` false while the snapshot is + focused) and 16b (the hazard is real — the snapshot's `is_read_only()` + is **false** despite the intercept, so nothing at the rope/CRDT + boundary would stop an op that did arrive). The wire-level half is an + explicit obligation of the CI `crdt`-coverage lane. +- Load-bearing Stage 2 decisions: + - **The snapshot MATERIALIZES into an ordinary buffer**, so isearch, + motion, selection and the kill ring work with no new substrate, and + "keys must not reach the child" dissolves structurally — the + transport arm keys on `is_terminal(buffer_id)` and a snapshot is not + a terminal. **The dispatch-shadow count stays at six.** + - **One serializer, not two** (Q#TC7): `copy_retained` builds a + whole-range *selection* and hands it to `copy_selection_bytes`. + - **`prune` reacts to removal rather than causing it** — it filters on + `!registry.contains(buffer_id)`, so a child exiting does NOT remove + the terminal buffer. That is why `on_removed` is a sound teardown + hook, and why a finished command's output stays readable. +- **Five bites, five different wrong implementations.** Removing + `set_round_trip_input` fails acceptance 16 **in the default + configuration** (the whole reason that pin is ungated); a naive + independently-written serializer fails all four unit pins, with the + diffs naming each drift mode (broken soft wrap, untrimmed blanks, + trailing newline); making re-invoke create a fresh buffer fails 18; + dropping the kill-with-terminal teardown fails 18; removing the + intercept fails 16b. Each failed exactly one test. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 48b75d8..68a88dd 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -1,9 +1,16 @@ # Terminal configuration and copy mode **Revision 4 — scouted against canonical `main` @ `b889873` (protocol v20), -2026-07-25. APPROVED after four review rounds. Stage 1 is implemented on -branch `terminal-config` (PR #173); Stage 2 (`terminal-copy-mode`) is -framed but not started, and branches off `main` after Stage 1 merges.** +2026-07-25. APPROVED after four review rounds. Stage 1 MERGED as #173 +(`main` @ `cf54270`, 2026-07-26). Stage 2 implemented on branch +`terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** + +**Stage 2 ships eight of its nine criteria.** Criterion 17's semantic-frontend +end-to-end pin is deliberately absent — see the note under it — because a +faithful version requires the real `pmacs-gpu` optimistic path, and therefore +the `a37` foundation, which CI never compiles and which skips silently. Both +halves of the *mechanism* it guards are pinned ungated instead (16, 16b). No +other criterion is partial. Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) — revision 3 named the key but not the storage, and two implementations @@ -576,6 +583,28 @@ additive, on its own binding, and does not replace scroll-and-select. emitted, bypasses the Lua intercept, passes `ensure_writable()`, and mutates **both sides** — a buffer the editor calls read-only silently accepts an edit. + + **NOT PINNED as specified, deliberately, and this is the one gap in + Stage 2.** A faithful test has to drive the *real* `pmacs-gpu` binary: + the optimistic apply lives only in `pmacs-gpu/src/main.rs` + (`optimistic_crdt_insert` / `optimistic_insert_text`), and the headless + `SemanticClient` the other semantic tests use has no optimistic path at + all, so it cannot produce the op whose absence is the claim. That means + building on the `a37` foundation — which is `crdt`-gated so CI never + compiles it, **returns `ok` without running** when `pmacs-gpu` is absent + from the target directory, and is load-sensitive enough to pass and fail + at the same commit twenty minutes apart. A second test on that footing + would add the appearance of coverage without the substance. + + What IS pinned instead, ungated and in CI: acceptance 16 asserts the + guard is armed (`dispatch_idle` false while the snapshot is focused, so + no replica can apply optimistically or emit), and acceptance 16b asserts + the hazard is real by showing the snapshot buffer's `is_read_only()` is + **false** despite the intercept — i.e. nothing at the rope/CRDT boundary + would stop such an op if one arrived. Together those cover both halves of + Q#TC6a's *mechanism*. What remains unproven is only the end-to-end wire + behaviour of a real GPU frontend, and it stays an explicit obligation of + the CI `crdt`-coverage lane rather than being quietly dropped. 18. Re-invoking against the same terminal refreshes in place; the buffer count does not grow (Q#TC8). Killing the snapshot leaves the terminal running; killing the terminal removes the snapshot. diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 1e7ac04..37a1307 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -8836,6 +8836,25 @@ fn install_terminal( )?; } + { + let manager = manager.clone(); + terminal.set( + "_copy_retained", + // Q#TC7: returns the whole retained range as a string, through + // the same serializer selection-copy uses. Takes an explicit + // buffer rather than resolving the active view, because copy + // mode reads a terminal that may not be displayed — and + // because the caller already holds the handle it keyed its + // snapshot on. + lua.create_function(move |lua, buffer: BufferIdLua| { + let Some(bytes) = manager.borrow().copy_retained(buffer.0) else { + return Ok(None); + }; + Ok(Some(lua.create_string(&bytes)?)) + })?, + )?; + } + pmacs.set("terminal", terminal) } diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 1c0957d..b787dee 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -329,6 +329,29 @@ impl TerminalManager { copy_selection_bytes(&rows, selection) } + /// Serialize a session's ENTIRE retained range — scrollback plus the + /// visible screen — through the same path [`copy_selection`] uses. + /// + /// Q#TC7. This deliberately builds a whole-range *selection* and hands + /// it to the existing serializer rather than walking the rows itself. + /// Soft-wrap joining, wide-glyph continuation, cluster bytes, and + /// per-row trailing-blank trimming are Vterm Stage 2 criterion 21's + /// pinned behavior; a second walk would re-derive all four and the two + /// would drift. That inheritance is what acceptance 13 asserts, by + /// comparing this against a full-range `copy_selection` rather than + /// against a literal. + /// + /// Returns `None` for a non-terminal buffer and for a session whose + /// retained rows are all empty — there is no cell to anchor to. + /// Unlike `copy_selection` this needs no registered view, so copy mode + /// does not depend on the terminal being currently displayed. + #[must_use] + pub fn copy_retained(&self, buffer_id: BufferId) -> Option> { + let session = self.sessions.get(&buffer_id)?; + let projection = session.screen.projection_ref(); + retained_bytes(&retained_rows(projection)) + } + /// Start an editor-owned primary selection at a viewport coordinate. pub fn begin_selection( &mut self, @@ -540,6 +563,40 @@ fn retained_rows(projection: BorrowedScreenProjection<'_>) -> RetainedRows<'_> { RetainedRows { projection } } +/// Serialize every retained cell, through the selection-copy serializer. +/// +/// Split out from [`TerminalManager::copy_retained`] so the fidelity +/// claims — soft-wrap joining, per-row trailing-blank trimming, wide-glyph +/// continuation, cluster bytes — are testable against the same projection +/// fixtures that pin `copy_selection_bytes` itself. Those four are exactly +/// what a second, independently written walk would get wrong. +fn retained_bytes(rows: &RetainedRows<'_>) -> Option> { + copy_selection_bytes(rows, full_retained_selection(rows)?) +} + +/// The selection spanning every retained cell. +/// +/// Rows with no cells are skipped at both ends rather than clamped: an +/// anchor into a zero-width row cannot resolve (`resolve_anchor` requires +/// `cell_offset` to fall inside `cell_offset .. cell_offset + len`), so +/// including one would make the whole range unresolvable and silently +/// yield nothing. Interior empty rows are untouched, because trailing- and +/// interior-blank handling belongs to the serializer. +fn full_retained_selection(rows: &RetainedRows<'_>) -> Option { + let mut occupied = rows.iter().filter(|row| !row.cells.is_empty()); + let first = occupied.next()?; + // `RetainedRows::iter` is a chain of slice iterators exposed as + // `impl Iterator`, so it is not double-ended; scan forward. + let last = occupied.last().unwrap_or(first); + Some(TerminalSelection { + anchor: row_lead(first), + head: LogicalCellAnchor { + logical_line_id: last.logical_line_id, + cell_offset: last.cell_offset.saturating_add(last.cells.len() as u32 - 1), + }, + }) +} + fn row_lead(row: &TerminalRow) -> LogicalCellAnchor { LogicalCellAnchor { logical_line_id: row.logical_line_id, @@ -1001,6 +1058,92 @@ mod tests { assert_eq!(bytes, b"abcd\ne"); } + /// Stage 2 criteria 13 and 14. Every property here is one a second, + /// independently written whole-range walk would get wrong: a naive + /// walk emits a newline per physical row (breaking the soft wrap), + /// keeps trailing default blanks, and has to rediscover that history + /// precedes the visible screen. Asserting exact bytes is what makes + /// "it reuses the serializer" falsifiable. + #[test] + fn retained_copy_spans_history_joins_soft_wraps_and_trims_blanks() { + let source = projection( + vec![row(1, 0, "ab ", true), row(1, 3, "cd ", false)], + vec![row(2, 0, "e ", false), row(3, 0, " ", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let bytes = retained_bytes(&retained).expect("whole range resolves"); + // `ab`+`cd` joined across the soft wrap; `e` on its own hard row; + // the all-blank final row trimmed to nothing but still separated. + assert_eq!(bytes, b"abcd\ne\n"); + } + + /// The whole-range selection must not depend on a view existing, and + /// must agree with an explicit full-span selection through the public + /// serializer — the anti-drift half of criterion 13. + #[test] + fn retained_copy_agrees_with_an_explicit_full_span_selection() { + let source = projection( + vec![row(1, 0, "aaa", false)], + vec![row(2, 0, "bbb", false), row(3, 0, "ccc", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let explicit = copy_selection_bytes( + &retained, + TerminalSelection { + anchor: LogicalCellAnchor { + logical_line_id: 1, + cell_offset: 0, + }, + head: LogicalCellAnchor { + logical_line_id: 3, + cell_offset: 2, + }, + }, + ) + .expect("explicit selection resolves"); + assert_eq!(retained_bytes(&retained).expect("whole range"), explicit); + assert_eq!(explicit, b"aaa\nbbb\nccc"); + } + + /// A wide glyph must be copied once across the whole range too, not + /// once per cell it occupies. + #[test] + fn retained_copy_emits_a_wide_glyph_once() { + let wide = TerminalRow { + cells: vec![ + Cell { + glyph: Glyph::Char('界'), + style: Style::default(), + attachment: None, + }, + Cell { + glyph: Glyph::Continuation, + style: Style::default(), + attachment: None, + }, + Cell::default(), + ], + logical_line_id: 9, + cell_offset: 0, + soft_wrapped: false, + }; + let source = projection(Vec::new(), vec![wide]); + let retained = retained_rows(source.as_borrowed()); + assert_eq!( + retained_bytes(&retained).expect("whole range"), + "界".as_bytes() + ); + } + + /// A session with nothing retained yields `None` rather than an empty + /// string, so the caller can tell "no terminal" from "empty terminal". + #[test] + fn retained_copy_of_zero_width_rows_is_none() { + let source = projection(Vec::new(), vec![row(1, 0, "", false)]); + let retained = retained_rows(source.as_borrowed()); + assert!(retained_bytes(&retained).is_none()); + } + #[test] fn wide_continuation_canonicalizes_to_lead_and_copies_once() { let wide = TerminalRow { diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs new file mode 100644 index 0000000..5b62ad8 --- /dev/null +++ b/tests/terminal_copy_mode_acceptance.rs @@ -0,0 +1,516 @@ +//! Terminal copy-mode acceptance (Stage 2 of +//! `docs/terminal-config-and-copy-mode-framing.md`, criteria 13-21). +//! +//! **Deliberately NOT `#[cfg(feature = "crdt")]`.** CI never enables that +//! feature, so a gated suite is written and then never run — 264 tests are +//! dark workspace-wide for exactly that reason. Criterion 16, the +//! round-trip gate Q#TC6a's entire safety argument rests on, needs no CRDT +//! and must be caught by the default configuration. + +use std::thread; +use std::time::{Duration, Instant}; + +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use mlua::Value; +use pmacs::cell::{CellSize, Glyph}; +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; +use pmacs::terminal::TerminalViewKey; + +const SNAPSHOT_NAME: &str = "*terminal-copy: terminal:sh*"; + +fn exec(state: &EditorState, src: &str) { + state + .lua_host + .lua() + .load(src) + .exec() + .unwrap_or_else(|e| panic!("lua failed: {src}\n{e}")); +} + +fn eval(state: &EditorState, src: &str) -> T { + state + .lua_host + .lua() + .load(src) + .eval() + .unwrap_or_else(|e| panic!("lua eval failed: {src}\n{e}")) +} + +fn eval_err(state: &EditorState, src: &str) -> String { + let result: mlua::Result = state.lua_host.lua().load(src).eval(); + match result { + Ok(_) => panic!("expected an error from: {src}"), + Err(e) => e.to_string(), + } +} + +fn press(state: &mut EditorState, code: KeyCode, mods: KeyModifiers) { + state.dispatch_key(FrontendId::LOCAL, KeyEvent::new(code, mods)); +} + +/// The live terminal screen's text, used only to wait for the child. +fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String { + let manager = state.terminal_manager.borrow(); + let Some(snapshot) = manager.snapshot(buffer) else { + return String::new(); + }; + let mut text = String::new(); + for cell in &snapshot.cells { + match &cell.glyph { + Glyph::Char(c) => text.push(*c), + Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), + Glyph::Continuation => {} + } + } + text +} + +fn tick_until(state: &mut EditorState, needle: &str, buffer: pmacs::buffer::BufferId) -> bool { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + state.tick_processes(); + if screen_text(state, buffer).contains(needle) { + return true; + } + if Instant::now() >= deadline { + return false; + } + thread::sleep(Duration::from_millis(20)); + } +} + +fn terminal_buffers(state: &EditorState) -> Vec { + let manager = state.terminal_manager.borrow(); + state + .core + .borrow() + .registry + .borrow() + .ids() + .iter() + .copied() + .filter(|id| manager.is_terminal(*id)) + .collect() +} + +/// A child that overflows the 24-row screen and then goes quiet, so its +/// early lines exist ONLY in scrollback — which is what makes criterion +/// 15's "content only in scrollback" claim meaningful. +const FILL_PROFILE: &str = r#" +pmacs.terminal.profiles.fill = { + command = "/bin/sh", + args = { "-c", + "printf 'NEEDLE-IN-SCROLLBACK\r\n'; i=1; while [ $i -le 200 ]; do printf 'LINE%03d\r\n' $i; i=$((i+1)); done; printf 'DONE\r\n'; exec cat" }, +} +"#; + +/// Open the fill terminal, wait for the child to finish, and return its id. +fn open_fill_terminal(state: &mut EditorState) -> pmacs::buffer::BufferId { + exec(state, FILL_PROFILE); + let before = terminal_buffers(state); + exec( + state, + r#"TERM_BUF = pmacs.terminal.open { profile = "fill" }"#, + ); + let fresh: Vec<_> = terminal_buffers(state) + .into_iter() + .filter(|id| !before.contains(id)) + .collect(); + assert_eq!(fresh.len(), 1, "exactly one terminal must have opened"); + let buffer = fresh[0]; + assert!(tick_until(state, "DONE", buffer), "the child must finish"); + buffer +} + +/// Give LOCAL a window on the terminal and register/claim its view, which +/// is what makes `dispatch_key`'s terminal transport arm reachable. +fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) { + state.core.borrow_mut().switch_active_buffer(buffer).ok(); + let window = state.core.borrow().active_window_id(); + let key = TerminalViewKey::new(FrontendId::LOCAL, window, buffer); + let mut manager = state.terminal_manager.borrow_mut(); + manager.register_view(key); + manager.claim_controller(key); + let _ = manager.snapshot_for_view(key, CellSize::new(10, 40)); +} + +fn buffer_text_by_name(state: &EditorState, name: &str) -> Option { + eval( + state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {name:?} then + return id:slice(0, id:len()) + end + end + return nil + " + ), + ) +} + +fn active_buffer_name(state: &EditorState) -> String { + eval( + state, + r"local b = pmacs.window.buffer(); return (pmacs.describe.buffer(b)).name", + ) +} + +fn buffer_count(state: &EditorState) -> usize { + state.core.borrow().registry.borrow().ids().len() +} + +/// Acceptance 13: the snapshot's text is exactly the whole retained range +/// as the existing copy path serializes it. +/// +/// Compared against `_copy_retained` rather than a literal, so this cannot +/// pass by both sides drifting the same way; the exact-bytes fidelity +/// claims (criterion 14) are pinned at the unit level in +/// `src/terminal/view.rs`, against the same projection fixtures that pin +/// `copy_selection_bytes` itself. +#[test] +fn acc13_snapshot_is_the_whole_retained_range_through_the_shared_serializer() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + exec(&state, "SNAP = pmacs.terminal.copy_mode(TERM_BUF)"); + let snapshot_text = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot buffer exists"); + let serialized: String = eval( + &state, + r"return pmacs.terminal._copy_retained(TERM_BUF) or ''", + ); + + assert_eq!( + snapshot_text, serialized, + "the snapshot must be byte-identical to the shared serializer's output" + ); + assert!( + snapshot_text.contains("NEEDLE-IN-SCROLLBACK") && snapshot_text.contains("LINE200"), + "the range must span scrollback AND the visible screen" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 14 (end-to-end half): the snapshot really is a rope-backed +/// document buffer and not a terminal, which is what makes every +/// buffer-shaped consumer work and what removes the transport arm. +#[test] +fn acc14_the_snapshot_is_an_ordinary_non_terminal_buffer() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let is_terminal: bool = eval( + &state, + r"local b = pmacs.window.buffer(); return pmacs.terminal.is_terminal(b)", + ); + assert!( + !is_terminal, + "the snapshot must NOT be a terminal — that is what structurally \ + removes the transport arm rather than guarding it" + ); + assert_eq!(active_buffer_name(&state), SNAPSHOT_NAME); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 15: isearch finds content that exists ONLY in scrollback, +/// with no change to `src/search.rs` (B1). +#[test] +fn acc15_isearch_finds_content_only_in_scrollback() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + // The needle is off the visible screen: the live terminal cannot see it. + assert!( + !screen_text(&state, terminal).contains("NEEDLE-IN-SCROLLBACK"), + "precondition: the needle must have scrolled off the live screen" + ); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + state.core.borrow_mut().set_cursor_byte(0); + + // Drive real isearch: C-s then the needle. + press(&mut state, KeyCode::Char('s'), KeyModifiers::CONTROL); + for ch in "NEEDLE-IN-SCROLLBACK".chars() { + press(&mut state, KeyCode::Char(ch), KeyModifiers::NONE); + } + let cursor = state.core.borrow().cursor(); + press(&mut state, KeyCode::Enter, KeyModifiers::NONE); + + let text = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + let expected = text + .find("NEEDLE-IN-SCROLLBACK") + .expect("the needle is in the snapshot") as u64; + assert_eq!( + cursor, + expected, + "isearch must land on the scrollback-only match; text was {:?}", + &text[..text.len().min(80)] + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 16 — the load-bearing pin, and the reason this suite is +/// ungated. `set_round_trip_input` is the ONLY thing standing between a +/// replica frontend and unauthorized mutation (Q#TC6a), so its regression +/// must be caught in the configuration CI actually compiles. +#[test] +fn acc16_dispatch_idle_is_false_while_the_snapshot_is_focused() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + assert!( + !state.dispatch_idle(), + "a focused snapshot must round-trip keys, so no replica applies \ + optimistically and none emits a CRDT op" + ); + // ...and it is the SNAPSHOT that does it, not merely "some terminal + // buffer is around": switching to an ordinary buffer restores idle. + exec( + &state, + r#"pmacs.window.switch_buffer(pmacs.buffer.create("*plain*"))"#, + ); + assert!(state.dispatch_idle(), "an ordinary buffer is idle again"); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 16 (the other half): the intercept rejects ordinary edits, +/// and — the fact that makes round-trip load-bearing rather than defence +/// in depth — the buffer is **not** `read_only` at the rope boundary. +#[test] +fn acc16b_the_intercept_rejects_edits_but_is_not_rope_level_protection() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + press(&mut state, KeyCode::Char('z'), KeyModifiers::NONE); + let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + assert_eq!(before, after, "the read-only intercept rejects self-insert"); + + // Q#TC6a, stated as a test so the next reader does not mistake the + // intercept for real immutability: no Lua binding sets + // `Buffer::read_only`, so this buffer accepts rope/CRDT mutation and + // only the round-trip mark above keeps a replica from producing one. + let core = state.core.borrow(); + let registry = core.registry.borrow(); + let ids = registry.ids(); + let snapshot = ids + .iter() + .copied() + .find(|id| { + registry + .get(*id) + .is_ok_and(|buf| buf.name() == SNAPSHOT_NAME) + }) + .expect("snapshot buffer id"); + assert!( + !registry + .get(snapshot) + .expect("snapshot buffer") + .is_read_only(), + "the Lua intercept does NOT set Buffer::read_only — this is why \ + set_round_trip_input is the guard and not hardening" + ); + drop(registry); + drop(core); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 18: re-invoking refreshes in place, and the lifecycle runs +/// both directions. +#[test] +fn acc18_reinvoke_refreshes_in_place_and_lifecycle_runs_both_ways() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + let count_after_first = buffer_count(&state); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert_eq!( + buffer_count(&state), + count_after_first, + "re-invoking must refresh in place, not accumulate buffers" + ); + + // Killing the snapshot alone leaves the terminal running. + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {SNAPSHOT_NAME:?} then pmacs.buffer.kill(id) end + end + " + ), + ); + assert!( + state.terminal_manager.borrow().is_terminal(terminal), + "killing the snapshot must leave the terminal untouched" + ); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_none(), + "the snapshot buffer is gone" + ); + + // ...and it can be rebuilt afterwards. + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_some(), + "a later invoke rebuilds the snapshot" + ); + + // Killing the terminal takes its snapshot with it. + exec(&state, "pmacs.terminal.terminate(TERM_BUF)"); + exec(&state, "pmacs.buffer.kill(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_none(), + "killing the terminal must remove its snapshot" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 19: `C-t` in a terminal — physically `C-c C-t`, because every +/// unescaped key goes to the child — enters copy mode; `g` refreshes and +/// `q` returns to the source terminal. +#[test] +fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + let terminal_name = active_buffer_name(&state); + + // The escape, then the terminal-local binding. + press(&mut state, KeyCode::Char('c'), KeyModifiers::CONTROL); + press(&mut state, KeyCode::Char('t'), KeyModifiers::CONTROL); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "C-c C-t must enter copy mode" + ); + + // `g` re-snapshots in place. + let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); + let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + assert_eq!(before, after, "a quiet terminal re-snapshots identically"); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "g must not move us" + ); + + // `q` returns to the source terminal. + press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); + assert_eq!( + active_buffer_name(&state), + terminal_name, + "q must return to the terminal the snapshot was taken from" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 20: copy mode is additive — the live terminal's own keys are +/// unchanged while a snapshot exists, and the terminal still follows its +/// tail. +#[test] +fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + // Back to the terminal; its five live bindings must still resolve. + exec(&state, "pmacs.window.switch_buffer(TERM_BUF)"); + for (sequence, command) in [ + ("M-w", "terminal.copy-selection"), + ("M-v", "terminal.page-up"), + ("C-v", "terminal.page-down"), + ("M-<", "terminal.scroll-oldest"), + ("M->", "terminal.scroll-bottom"), + ] { + let resolved: Option = eval( + &state, + &format!(r"local d = pmacs.describe.key({sequence:?}); return d and d.command"), + ); + assert_eq!( + resolved.as_deref(), + Some(command), + "{sequence} must still be the live terminal binding" + ); + } + + // The terminal is still following its tail: the child's last output is + // visible without scrolling. + assert!( + screen_text(&state, terminal).contains("DONE"), + "the live terminal keeps following its tail" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 21: the dispatch-shadow count is unchanged at six, pinned by +/// the observable difference between a buffer-local keymap and a shadow — +/// `describe-key` telling the truth about `g` and `q` in the snapshot. +/// +/// A seventh shadow would decode these keys before `KeymapStack::resolve` +/// ever ran, so introspection would report whatever the global binding is +/// (or nothing) while the keys behaved differently. +#[test] +fn acc21_describe_key_reports_the_truth_for_the_snapshot_bindings() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + for (sequence, command) in [("g", "terminal.copy-refresh"), ("q", "terminal.copy-quit")] { + let resolved: Option = eval( + &state, + &format!(r"local d = pmacs.describe.key({sequence:?}); return d and d.command"), + ); + assert_eq!( + resolved.as_deref(), + Some(command), + "describe-key must report the buffer-local {sequence} binding" + ); + } + + // And the binding really is scoped: back in the terminal, `q` is not + // the copy-mode command. + exec(&state, "pmacs.window.switch_buffer(TERM_BUF)"); + let resolved: Option = eval( + &state, + r#"local d = pmacs.describe.key("q"); return d and d.command"#, + ); + assert_ne!( + resolved.as_deref(), + Some("terminal.copy-quit"), + "the snapshot's q must not leak into the terminal buffer" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Copy mode refuses a non-terminal buffer rather than producing an empty +/// snapshot of nothing. +#[test] +fn copy_mode_refuses_a_non_terminal_buffer() { + let state = EditorState::new(); + let err = eval_err(&state, "return pmacs.terminal.copy_mode()"); + assert!( + err.contains("not a terminal"), + "the refusal must say why: {err}" + ); +} From 2eb6218ccd3400e3aca640898b29c00b8055f3a6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 10:45:32 -0400 Subject: [PATCH 2/6] fix(terminal): close review round 1 on copy mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all real, and they rhyme in pairs. Two implementation defects and two vacuous pins, all four tracing to one root: a name is not an identity, and a context-free readout is not a state observation. A foreign buffer carrying the snapshot's name was adopted and then overwritten. `pmacs.buffer.create` accepts any caller-chosen name and snapshot writes use bypass_intercept, so found-by-name adoption clobbered user data — the reviewer reproduced "do not clobber" becoming 23 newlines. Now follows dired's F7 rule: ownership means "in copy mode's own handle table", never "found by name", and a taken name yields a `<2>` variant. Snapshot identity was keyed by terminal NAME. `TerminalManager::open` uniquifies only the derived name — an explicit `name = ...` is inserted verbatim — so two valid terminals can share one, and a name-keyed table handed them a single snapshot: the second invocation retargeted it, `q` returned to the wrong terminal, and killing either removed the shared buffer. Identity is now the terminal buffer, compared in an array, because BufferIdLua implements `__eq` but each wrapper is a distinct table key: comparison works, hashing does not. The kill-with-terminal callback now closes over its own record rather than looking the name up again. The refresh pins were vacuous. Acceptance 19 compared a quiet terminal's snapshot against itself and 18 counted buffers, so both passed with render_snapshot replaced by a no-op. The child is `exec cat`, so the tests now type a marker into the focused terminal, require it ABSENT from the existing snapshot, and only then refresh — via `g` and via re-invocation respectively. The tail-follow pin could not observe view state. `TerminalManager::snapshot(buffer_id)` is context-free and always returns the live screen, so it reported "at the tail" even for a view forced to the oldest retained row. Now read through `snapshot_for_view`'s at_bottom and its projected cells. Adds acceptance 18a (a foreign same-named buffer is never adopted or clobbered) and 18b (two same-named terminals get two independent snapshots, each `q` returns to its own source, and killing one leaves the other's snapshot alive). Four new bites, all discriminating: restoring adopt-by-name fails 18a AND 18b; restoring name-keyed identity fails 18b; making render_snapshot a no-op fails BOTH 18 and 19, which is the vacuity demonstrated rather than argued; and forcing the view off the tail fails 20. Criterion 17 stays a named follow-up, per review agreement, until the real GPU probe is non-skipping and CI-executed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- builtin/runtime/terminal.lua | 125 +++++--- docs/active-work.md | 29 ++ docs/terminal-config-and-copy-mode-framing.md | 48 ++- tests/terminal_copy_mode_acceptance.rs | 290 ++++++++++++++++-- 4 files changed, 432 insertions(+), 60 deletions(-) diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index b44e824..f2eca85 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -221,11 +221,56 @@ pmacs.keymap.bind { scope = "global", sequence = "C-c t", command = "terminal" } local raw_copy_retained = assert(terminal._copy_retained, "pmacs.terminal._copy_retained is required") --- snapshot buffer name -> { terminal = , buffer = } +-- An ARRAY of `{ terminal = , buffer = }`, scanned linearly and +-- compared with `==`, following dired's handle table (F7). -- --- Keyed by NAME, not by buffer handle: handles are not stable table keys, --- and a name survives the user killing the snapshot (listview precedent). -local snapshots = {} +-- Not `snapshots[name]`, and not `snapshots[buf]`, for two separate +-- reasons — both of which were live defects in review round 1: +-- +-- * **A terminal name is not a unique key.** `TerminalManager::open` +-- uniquifies only the DERIVED name; an explicitly passed +-- `name = "*same*"` is inserted verbatim +-- (`src/terminal/session.rs`, `if spec.name.is_some()`). Two valid +-- terminals can therefore share a name, and a name-keyed table gives +-- them one snapshot between them: the second invocation silently +-- retargets it, `q` returns to the wrong terminal, and killing either +-- one removes the shared buffer. +-- * **A buffer handle is not a stable table key.** `BufferIdLua` +-- implements `__eq` but each wrapper is a distinct table key, so +-- `snapshots[buf]` would miss on a freshly minted handle for the same +-- buffer. Comparison works; hashing does not. Hence the scan. +local handles = {} + +-- Compact dead entries first, so a command in a killed snapshot sees +-- "not in copy mode" rather than operating on dead state. +local function live_handles() + local live = {} + for _, h in ipairs(handles) do + local term_ok, term_valid = pcall(h.terminal.is_valid, h.terminal) + local snap_ok, snap_valid = pcall(h.buffer.is_valid, h.buffer) + if term_ok and term_valid and snap_ok and snap_valid then + live[#live + 1] = h + end + end + handles = live + return live +end + +local function handle_for_terminal(term_buf) + if term_buf == nil then return nil end + for _, h in ipairs(live_handles()) do + if h.terminal == term_buf then return h end + end + return nil +end + +local function handle_for_snapshot(buf) + if buf == nil then return nil end + for _, h in ipairs(live_handles()) do + if h.buffer == buf then return h end + end + return nil +end local function buffer_name(buf) local ok, described = pcall(pmacs.describe.buffer, buf) @@ -233,7 +278,7 @@ local function buffer_name(buf) return nil end -local function find_buffer_by_name(name) +local function buffer_named(name) for _, id in ipairs(pmacs.buffer.list()) do local ok, described = pcall(pmacs.describe.buffer, id) if ok and described and described.name == name then return id end @@ -244,11 +289,31 @@ end -- `*terminal:bash*` -> `*terminal-copy: terminal:bash*`. The surrounding -- asterisks are stripped before nesting so the result reads as one -- generated-buffer name rather than two. -local function snapshot_name_for(term_buf) +local function snapshot_base_name(term_buf) local name = buffer_name(term_buf) or "terminal" return string.format("*terminal-copy: %s*", (name:gsub("^%*", ""):gsub("%*$", ""))) end +-- How far the `<2>`, `<3>`, ... disambiguation walks before giving up. +local NAME_VARIANT_LIMIT = 99 + +-- `pmacs.buffer.create` takes any caller-chosen name, so a foreign buffer +-- may already be called `*terminal-copy: sh*` — and two same-named +-- terminals legitimately produce the same base name. Painting into a +-- buffer we did not create would clobber a user's data through +-- `bypass_intercept`, so **found-by-name is NOT adoption**: ownership +-- means "this buffer is in the handle table above", exactly as in dired. +local function unique_snapshot_name(term_buf) + local name = snapshot_base_name(term_buf) + if buffer_named(name) == nil then return name end + for i = 2, NAME_VARIANT_LIMIT do + local candidate = string.format("%s<%d>", name, i) + if buffer_named(candidate) == nil then return candidate end + end + error(string.format( + "terminal.copy-mode: %s is taken and no free variant remains", name), 0) +end + -- Q#TC7: the snapshot text comes from the SAME serializer selection-copy -- uses, so soft wraps, wide glyphs, clusters and trailing blanks cannot -- drift between the two. @@ -262,19 +327,17 @@ local function render_snapshot(record) if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end end -local function ensure_snapshot(term_buf) - local name = snapshot_name_for(term_buf) - local record = snapshots[name] - if record and record.buffer:is_valid() then - -- Q#TC8: re-invoking refreshes IN PLACE. Retarget the terminal too, - -- in case a terminal buffer was recreated under the same name. - record.terminal = term_buf - return record - end +local function claim_snapshot(term_buf) + -- Q#TC8: re-invoking against the same terminal refreshes IN PLACE. + -- Identity is the terminal BUFFER, so two same-named terminals get two + -- snapshots and neither can retarget the other's. + local existing = handle_for_terminal(term_buf) + if existing then return existing end - local buf = find_buffer_by_name(name) or pmacs.buffer.create(name) - record = { terminal = term_buf, buffer = buf } - snapshots[name] = record + local name = unique_snapshot_name(term_buf) + local buf = pmacs.buffer.create(name) + local record = { terminal = term_buf, buffer = buf } + handles[#handles + 1] = record -- Q#TC6a — BOTH calls, and the second is the load-bearing one. -- @@ -298,9 +361,11 @@ local function ensure_snapshot(term_buf) pmacs.keymap.bind { scope = "buffer", buffer = buf, sequence = "q", command = "terminal.copy-quit" } - -- Q#TC8 lifecycle, both directions. Killing the terminal takes its - -- snapshot with it; killing the snapshot alone leaves the terminal - -- running and merely forgets the record, so a later invoke rebuilds. + -- Q#TC8 lifecycle, both directions. Killing the terminal takes ITS + -- snapshot with it — `record`, captured here, not "whatever is + -- currently filed under this name"; killing the snapshot alone leaves + -- the terminal running, and `live_handles` compacts the entry out so a + -- later invoke rebuilds. -- -- `on_removed` is sound here because every user-facing kill path -- routes through `pmacs.buffer.kill`, which fires the callbacks. The @@ -310,14 +375,8 @@ local function ensure_snapshot(term_buf) -- alive, which is what makes reading back a finished command's output -- work at all. pcall(pmacs.buffer.on_removed, term_buf, function() - local current = snapshots[name] - if current and current.buffer:is_valid() then - pcall(pmacs.buffer.kill, current.buffer) - end - snapshots[name] = nil - end) - pcall(pmacs.buffer.on_removed, buf, function() - snapshots[name] = nil + local ok, valid = pcall(record.buffer.is_valid, record.buffer) + if ok and valid then pcall(pmacs.buffer.kill, record.buffer) end end) return record @@ -325,11 +384,7 @@ end -- The snapshot record whose buffer the active window shows, or nil. local function snapshot_for_current_buffer() - local buf = pmacs.window.buffer() - if not buf then return nil end - local name = buffer_name(buf) - if not name then return nil end - return snapshots[name] + return handle_for_snapshot(pmacs.window.buffer()) end function terminal.copy_mode(term_buf) @@ -338,7 +393,7 @@ function terminal.copy_mode(term_buf) if not terminal.is_terminal(term_buf) then error("terminal.copy-mode: the current buffer is not a terminal", 0) end - local record = ensure_snapshot(term_buf) + local record = claim_snapshot(term_buf) render_snapshot(record) pmacs.window.switch_buffer(record.buffer) return record.buffer diff --git a/docs/active-work.md b/docs/active-work.md index bfdb7ff..9ba9efa 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -435,6 +435,35 @@ If it does not, stop and repair the remote/fetch configuration. trailing newline); making re-invoke create a fresh buffer fails 18; dropping the kill-with-terminal teardown fails 18; removing the intercept fails 16b. Each failed exactly one test. +- **Review round 1 — four findings, all real, and they rhyme in pairs.** + Two P1 implementation defects and two P2 vacuous pins, all four tracing + to one root: **a name is not an identity, and a context-free readout is + not a state observation.** + - *P1 — a foreign same-named buffer was adopted and clobbered.* Snapshot + writes use `bypass_intercept`, so found-by-name adoption overwrote a + user's buffer; the reviewer reproduced "do not clobber" becoming 23 + newlines. Fixed by dired's F7 rule: **ownership means "in our own + handle table"**, and a taken name yields a `<2>` variant. + - *P1 — snapshot identity was keyed by terminal NAME.* + `TerminalManager::open` uniquifies only the *derived* name, so an + explicit `name = "*same*"` lets two valid terminals share one; they + then shared a snapshot, `q` returned to the wrong terminal, and + killing either removed it. Now keyed by comparing buffer handles in an + array — `BufferIdLua` implements `__eq` but each wrapper is a distinct + table key, so **comparison works and hashing does not**. + - *P2 — the refresh pins were vacuous.* 19 compared a quiet terminal's + snapshot against itself and 18 counted buffers, so both passed with + `render_snapshot` replaced by a no-op. Now the test types a marker + into the `cat` child, requires it **absent** first, then refreshes. + - *P2 — the tail-follow pin could not observe view state.* + `manager.snapshot(buffer_id)` is context-free and always reads the + live screen, so it reported "at the tail" for a view forced to the + oldest retained row. Now read through `snapshot_for_view`'s + `at_bottom` and projected cells. +- **Four more bites, all discriminating.** Restoring adopt-by-name fails + 18a *and* 18b; restoring name-keyed identity fails 18b; making + `render_snapshot` a no-op fails **both** 18 and 19 (the vacuity, + demonstrated); and forcing the view off the tail fails 20. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 68a88dd..b7ced8e 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -5,12 +5,19 @@ (`main` @ `cf54270`, 2026-07-26). Stage 2 implemented on branch `terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** -**Stage 2 ships eight of its nine criteria.** Criterion 17's semantic-frontend -end-to-end pin is deliberately absent — see the note under it — because a -faithful version requires the real `pmacs-gpu` optimistic path, and therefore -the `a37` foundation, which CI never compiles and which skips silently. Both -halves of the *mechanism* it guards are pinned ungated instead (16, 16b). No -other criterion is partial. +**Stage 2 ships eight of its nine criteria, plus 18a and 18b added in review +round 1.** Criterion 17's semantic-frontend end-to-end pin is deliberately +absent — see the note under it — because a faithful version requires the real +`pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never +compiles and which skips silently. Both halves of the *mechanism* it guards are +pinned ungated instead (16, 16b). No other criterion is partial. + +**Review round 1 found four defects, and the pair of them rhymes.** Two were +implementation (18a's foreign-buffer clobber, 18b's name-keyed identity) and +two were vacuous pins (18/19's refresh, 20's tail-follow) — and all four trace +to the same root: **a name is not an identity, and a context-free readout is +not a state observation.** The name mistake produced both P1s; the readout +mistake produced both P2s. Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) — revision 3 named the key but not the storage, and two implementations @@ -608,11 +615,40 @@ additive, on its own binding, and does not replace scroll-and-select. 18. Re-invoking against the same terminal refreshes in place; the buffer count does not grow (Q#TC8). Killing the snapshot leaves the terminal running; killing the terminal removes the snapshot. + + **The refresh half must be observed by CONTENT, not by buffer count** + (review round 1). Counting buffers, or comparing a quiet terminal's + snapshot against itself, passes with `render_snapshot` replaced by a + no-op. The child is `exec cat`, so the test types a marker into the + focused terminal, requires it **absent** from the existing snapshot, and + only then re-invokes — the "advance the world" discipline. +18a. **A foreign buffer carrying the snapshot's name is never adopted.** + `pmacs.buffer.create` accepts any caller-chosen name, and snapshot writes + use `bypass_intercept`, so found-by-name adoption silently overwrites a + user's data — reproduced in review round 1 as "do not clobber" becoming + 23 newlines. Ownership means **"in copy mode's own handle table"**, which + is dired's F7 rule; a taken name yields a `<2>` variant. +18b. **Snapshot identity is the terminal BUFFER, not its name.** + `TerminalManager::open` uniquifies only the *derived* name — an explicit + `name = ...` is inserted verbatim — so two valid terminals can share one. + A name-keyed table hands them a single snapshot: the second invocation + retargets it, `q` returns to the wrong terminal, and killing either one + removes the shared buffer. Keyed instead by comparing buffer handles in + an array, because `BufferIdLua` implements `__eq` but each wrapper is a + distinct table key — comparison works, hashing does not. 19. `C-t` in a terminal buffer (physically `C-c C-t`) enters copy mode; `g` refreshes the snapshot from the live terminal and `q` returns to the source terminal (Q#TC8a). 20. The live terminal's own keys are unchanged while a snapshot exists (Q#TC9), and the terminal keeps following its tail. + + **Tail-following must be read through the registered VIEW.** Review + round 1: `TerminalManager::snapshot(buffer_id)` is context-free and + always returns the live screen, so it reports "at the tail" even for a + view forced to the oldest retained row — falsified by doing exactly + that and watching the assertion still pass. `snapshot_for_view`'s + `at_bottom` plus its projected cells are the only observables that can + tell the two apart. 21. The dispatch-shadow count is **unchanged at six** — pinned by asserting `describe-key` reports the truth for the snapshot buffer's `g` and `q`, which is the observable difference between the buffer-local idiom and a diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index 5b62ad8..39c2753 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -123,16 +123,66 @@ fn open_fill_terminal(state: &mut EditorState) -> pmacs::buffer::BufferId { buffer } +fn viewport() -> CellSize { + CellSize::new(10, 40) +} + /// Give LOCAL a window on the terminal and register/claim its view, which /// is what makes `dispatch_key`'s terminal transport arm reachable. -fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) { +/// Returns the view key, so assertions can read the *projected* view +/// rather than the context-free live screen. +fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) -> TerminalViewKey { state.core.borrow_mut().switch_active_buffer(buffer).ok(); let window = state.core.borrow().active_window_id(); let key = TerminalViewKey::new(FrontendId::LOCAL, window, buffer); let mut manager = state.terminal_manager.borrow_mut(); manager.register_view(key); manager.claim_controller(key); - let _ = manager.snapshot_for_view(key, CellSize::new(10, 40)); + let _ = manager.snapshot_for_view(key, viewport()); + key +} + +/// Make the child produce NEW output, so a refresh has something to find. +/// +/// The child is `exec cat`, so typing into the focused terminal echoes +/// back. Without this, "refresh" tests compare a quiet terminal against +/// itself and pass with the render replaced by a no-op — the defect review +/// round 1 found in acceptance 18 and 19. +fn emit_into_child(state: &mut EditorState, terminal: pmacs::buffer::BufferId, marker: &str) { + focus_terminal(state, terminal); + for ch in marker.chars() { + press(state, KeyCode::Char(ch), KeyModifiers::NONE); + } + assert!( + tick_until(state, marker, terminal), + "the child must echo {marker:?} back onto the live screen" + ); +} + +/// What the registered VIEW currently projects — which, unlike +/// `manager.snapshot(buffer)`, depends on where the view is anchored. +fn view_text(state: &EditorState, key: TerminalViewKey) -> String { + let mut manager = state.terminal_manager.borrow_mut(); + let Some(snapshot) = manager.snapshot_for_view(key, viewport()) else { + return String::new(); + }; + let mut text = String::new(); + for cell in &snapshot.cells { + match &cell.glyph { + Glyph::Char(c) => text.push(*c), + Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), + Glyph::Continuation => {} + } + } + text +} + +fn view_at_bottom(state: &EditorState, key: TerminalViewKey) -> bool { + state + .terminal_manager + .borrow_mut() + .snapshot_for_view(key, viewport()) + .is_some_and(|snapshot| snapshot.at_bottom) } fn buffer_text_by_name(state: &EditorState, name: &str) -> Option { @@ -336,13 +386,30 @@ fn acc18_reinvoke_refreshes_in_place_and_lifecycle_runs_both_ways() { exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); let count_after_first = buffer_count(&state); + assert!( + !buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("REINVOKE"), + "precondition: the marker has not been emitted yet" + ); + // Advance the world, then re-invoke. Counting buffers alone is + // vacuous: it passes with the render replaced by a no-op, so the + // refresh must be observed by CONTENT that only exists after the + // first snapshot was taken. + emit_into_child(&mut state, terminal, "REINVOKE"); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("REINVOKE"), + "re-invoking must actually re-serialize, not just reuse the buffer" + ); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); assert_eq!( buffer_count(&state), count_after_first, - "re-invoking must refresh in place, not accumulate buffers" + "...and it must refresh IN PLACE, not accumulate buffers" ); // Killing the snapshot alone leaves the terminal running. @@ -402,17 +469,6 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { "C-c C-t must enter copy mode" ); - // `g` re-snapshots in place. - let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); - press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); - let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); - assert_eq!(before, after, "a quiet terminal re-snapshots identically"); - assert_eq!( - active_buffer_name(&state), - SNAPSHOT_NAME, - "g must not move us" - ); - // `q` returns to the source terminal. press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); assert_eq!( @@ -420,6 +476,45 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { terminal_name, "q must return to the terminal the snapshot was taken from" ); + + // Now advance the world and come back WITHOUT re-invoking copy mode, + // so the snapshot is genuinely stale. Comparing a quiet terminal's + // snapshot against itself is vacuous — it passes with `render_snapshot` + // replaced by a no-op. + emit_into_child(&mut state, terminal, "AFTER-G"); + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {SNAPSHOT_NAME:?} then + pmacs.window.switch_buffer(id) + end + end + " + ), + ); + assert!( + !buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("AFTER-G"), + "the snapshot must still be stale before `g` — otherwise the next \ + assertion proves nothing" + ); + + press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("AFTER-G"), + "`g` must re-snapshot from the live terminal" + ); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "g must not move us" + ); state.process_supervisor.borrow_mut().shutdown(); } @@ -430,7 +525,7 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { let mut state = EditorState::new(); let terminal = open_fill_terminal(&mut state); - focus_terminal(&state, terminal); + let key = focus_terminal(&state, terminal); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); // Back to the terminal; its five live bindings must still resolve. @@ -453,11 +548,25 @@ fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { ); } - // The terminal is still following its tail: the child's last output is - // visible without scrolling. + // The terminal still FOLLOWS ITS TAIL while a snapshot exists. + // + // Read through the registered view, not `manager.snapshot(buffer)`: + // that call is context-free and always returns the live screen, so it + // reports "at the tail" even for a view forced to the oldest retained + // row. The projected view is the only thing that can distinguish them. assert!( - screen_text(&state, terminal).contains("DONE"), - "the live terminal keeps following its tail" + view_at_bottom(&state, key), + "precondition: the view starts at the tail" + ); + emit_into_child(&mut state, terminal, "TAILMARK"); + assert!( + view_at_bottom(&state, key), + "new child output must not knock the view off the tail" + ); + assert!( + view_text(&state, key).contains("TAILMARK"), + "the freshest output must be visible in the PROJECTED view: {:?}", + view_text(&state, key) ); state.process_supervisor.borrow_mut().shutdown(); } @@ -503,6 +612,149 @@ fn acc21_describe_key_reports_the_truth_for_the_snapshot_bindings() { state.process_supervisor.borrow_mut().shutdown(); } +/// Acceptance 18a (review round 1, P1): a foreign buffer that happens to +/// carry the snapshot's name is **never adopted**. +/// +/// `pmacs.buffer.create` takes any caller-chosen name, and snapshot writes +/// use `bypass_intercept`, so found-by-name adoption clobbers a user's +/// data outright. Ownership means "in copy mode's own handle table" +/// (dired's F7 rule); a taken name gets a `<2>` variant instead. +#[test] +fn acc18a_a_foreign_same_named_buffer_is_never_adopted_or_clobbered() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + // A user's buffer, sitting exactly where the snapshot wants to go. + exec( + &state, + &format!( + r" + FOREIGN = pmacs.buffer.create({SNAPSHOT_NAME:?}) + FOREIGN:insert(0, 'do not clobber') + " + ), + ); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let foreign_text: String = eval(&state, r"return FOREIGN:slice(0, FOREIGN:len())"); + assert_eq!( + foreign_text, "do not clobber", + "the foreign buffer must be untouched" + ); + assert_ne!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "copy mode must not display the foreign buffer" + ); + assert_eq!( + active_buffer_name(&state), + format!("{SNAPSHOT_NAME}<2>"), + "a taken name must yield a unique variant" + ); + assert!( + buffer_text_by_name(&state, &format!("{SNAPSHOT_NAME}<2>")) + .expect("variant snapshot") + .contains("LINE200"), + "the variant is the real snapshot" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 18b (review round 1, P1): snapshot identity is the terminal +/// BUFFER, not its name. +/// +/// `TerminalManager::open` uniquifies only the *derived* name — an +/// explicit `name = ...` is inserted verbatim — so two valid terminals can +/// share a name. Keying snapshots by name gives them one buffer between +/// them: the second invocation retargets it, `q` returns to the wrong +/// terminal, and killing either one removes the shared snapshot. +#[test] +fn acc18b_two_same_named_terminals_get_two_independent_snapshots() { + let mut state = EditorState::new(); + exec(&state, FILL_PROFILE); + + let before = terminal_buffers(&state); + exec( + &state, + r#"TERM_A = pmacs.terminal.open { profile = "fill", name = "*same*" }"#, + ); + exec( + &state, + r#"TERM_B = pmacs.terminal.open { profile = "fill", name = "*same*" }"#, + ); + let fresh: Vec<_> = terminal_buffers(&state) + .into_iter() + .filter(|id| !before.contains(id)) + .collect(); + assert_eq!(fresh.len(), 2, "two terminals opened under one name"); + + // Distinguish them by content, since their names are identical. + emit_into_child(&mut state, fresh[0], "AAAA"); + emit_into_child(&mut state, fresh[1], "BBBB"); + + focus_terminal(&state, fresh[0]); + let snap_a: String = eval( + &state, + r"local b = pmacs.terminal.copy_mode(TERM_A); return (pmacs.describe.buffer(b)).name", + ); + focus_terminal(&state, fresh[1]); + let snap_b: String = eval( + &state, + r"local b = pmacs.terminal.copy_mode(TERM_B); return (pmacs.describe.buffer(b)).name", + ); + + assert_ne!( + snap_a, snap_b, + "two terminals must not share one snapshot buffer" + ); + let text_a = buffer_text_by_name(&state, &snap_a).expect("snapshot A"); + let text_b = buffer_text_by_name(&state, &snap_b).expect("snapshot B"); + assert!( + text_a.contains("AAAA") && !text_a.contains("BBBB"), + "snapshot A must hold only A's output: {:?}", + &text_a[text_a.len().saturating_sub(60)..] + ); + assert!( + text_b.contains("BBBB") && !text_b.contains("AAAA"), + "snapshot B must hold only B's output" + ); + + // `q` from each snapshot returns to ITS OWN terminal, which is only + // observable through the buffer id — the two names are the same. + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {snap_b:?} then pmacs.window.switch_buffer(id) end + end + " + ), + ); + press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); + let returned_is_b: bool = eval(&state, r"return pmacs.window.buffer() == TERM_B"); + assert!( + returned_is_b, + "q from B's snapshot must return to terminal B" + ); + + // Killing terminal A removes only A's snapshot. + exec(&state, "pmacs.terminal.terminate(TERM_A)"); + exec(&state, "pmacs.buffer.kill(TERM_A)"); + assert!( + buffer_text_by_name(&state, &snap_a).is_none(), + "A's snapshot dies with A" + ); + assert!( + buffer_text_by_name(&state, &snap_b).is_some(), + "B's snapshot must SURVIVE — a shared buffer would have gone too" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + /// Copy mode refuses a non-terminal buffer rather than producing an empty /// snapshot of nothing. #[test] From 8c5b39ef32afc05970b8a7f63c39bcdc8f064f26 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 13:12:08 -0400 Subject: [PATCH 3/6] fix(buffer): make generated buffers survive undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2, P1. Undo could empty the "read-only" snapshot. `render_snapshot` wrote with bypass_intercept, which leaves ordinary undo history behind, and `Buffer::undo` reaches the rope through `ensure_writable` without ever consulting the intercept chain. So a single `C-/` — or `M-x buffer.undo`, which needs no keymap at all — replaced a freshly rendered snapshot with an empty buffer. `set_round_trip_input` does not help: it routes the key into the daemon command path, which is exactly where undo runs. Rebinding the undo chords buffer-locally would not have closed this, and `compile.lua` already says so in a comment: "command/menu undo stays dispatchable". `*compilation*` and listview panels therefore carry the same latent defect today. Adds `Buffer::set_generated_contents` (Lua: `pmacs.buffer.set_generated_contents`): lift `read_only`, replace the contents skipping intercepts, discard the resulting history, re-assert `read_only`. This ships the framing's deferred immutability lane as ONE primitive rather than exposing the setter — a bare `set_read_only` would let a caller lock a buffer it can no longer refresh, which is precisely why that lane was deferred. Discarding history is load-bearing twice: it removes what undo would replay, and it stops a periodically refreshed buffer accumulating rope clones that `read_only` guarantees nothing can ever pop. New acceptance 16c drives the real M-x path (`command.invoke_interactive`), the chord, and redo, and asserts the owner's own refresh still works — the operation plain `read_only` would have broken. Acceptance 16b flips from asserting `is_read_only()` is false to true, because the property it documented is the one that was wrong. Three `buffer.rs` unit tests cover the primitive directly, including that ten refreshes leave an empty undo stack. Bite: restoring the delete+insert render reproduces the report exactly — `left: Some("")` against the full snapshot — failing 16c and 16b. Still open, and now named in the framing, COHERENCE.md §14 and the ledger: `*compilation*` and listview have not adopted the primitive and remain emptiable by `M-x buffer.undo`; a streaming-friendly variant is needed for the append case. In CRDT mode `read_only` is what refuses undo, since loro's UndoManager exposes no clear through `CrdtState`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 11 +- builtin/runtime/terminal.lua | 18 ++- docs/active-work.md | 33 +++++ docs/terminal-config-and-copy-mode-framing.md | 38 ++++++ src/buffer.rs | 115 ++++++++++++++++++ src/lua_bindings/mod.rs | 23 ++++ tests/terminal_copy_mode_acceptance.rs | 85 +++++++++++-- 7 files changed, 306 insertions(+), 17 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index f707234..93298fc 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1214,7 +1214,16 @@ Primitive-by-primitive against the list above: rebindable (§6's counter-example). - **Output channel** ✓ — the compile-mode `*compilation*` model (streamed, intercept-read-only, error-rule parsing), reused by grep - and shell-command. + and shell-command. **Caveat found in terminal copy mode's review + (Stage 2): "intercept-read-only" is not read-only.** `Buffer::undo` + reaches the rope through `ensure_writable` without consulting the + intercept chain, so `M-x buffer.undo` empties such a buffer — and + rebinding the undo *chords* buffer-locally does not close it, as + `compile.lua`'s own comment admits ("command/menu undo stays + dispatchable"). `Buffer::set_generated_contents` (write + discard + history + assert `read_only`, in one authorized call) now fixes this + for the terminal snapshot; `*compilation*` and listview panels have + not yet adopted it and remain emptiable. - **Diagnostics collection** ✓ — `DiagnosticStore` + signs + unified `error.next` source. - **Transient selector** ✓ — the minibuffer (though its `source` diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index f2eca85..5a912da 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -319,12 +319,18 @@ end -- drift between the two. local function render_snapshot(record) local text = raw_copy_retained(record.terminal) or "" - local buf = record.buffer - local len = buf:len() - -- Snapshot writes bypass the read-only intercept; everything else is - -- rejected by it. - if len > 0 then buf:delete(0, len, { bypass_intercept = true }) end - if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end + -- The owner-authorized write, and the ONLY one this buffer accepts. + -- + -- Not `delete`+`insert` with `bypass_intercept` (review round 2): that + -- leaves the buffer writable at the rope, and it leaves undo history + -- behind. `Buffer::undo` reaches the rope through `ensure_writable` + -- without consulting the intercept chain, so a single `C-/` — or + -- `M-x buffer.undo`, which no buffer-local rebinding can take away — + -- replaced a freshly rendered snapshot with an empty buffer. + -- `set_generated_contents` writes, discards the history, and leaves + -- `read_only` asserted, so undo/redo and remote CRDT imports are all + -- refused at the rope. + pmacs.buffer.set_generated_contents(record.buffer, text) end local function claim_snapshot(term_buf) diff --git a/docs/active-work.md b/docs/active-work.md index 362dd1c..1ac337d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -738,6 +738,39 @@ If it does not, stop and repair the remote/fetch configuration. 18a *and* 18b; restoring name-keyed identity fails 18b; making `render_snapshot` a no-op fails **both** 18 and 19 (the vacuity, demonstrated); and forcing the view off the tail fails 20. +- **Review round 2 — one P1, and its fix retires half a named deferral.** + **Undo emptied the "read-only" snapshot.** `render_snapshot` wrote with + `bypass_intercept`, leaving ordinary undo history, and **`Buffer::undo` + reaches the rope through `ensure_writable` without ever consulting the + intercept chain** — so `C-/` *or* `M-x buffer.undo` replaced a freshly + rendered snapshot with an empty buffer. `set_round_trip_input` does not + help: it routes the key into the daemon command path, which is where + undo runs. + - **Rebinding the undo chords would NOT have fixed it**, and + `compile.lua` already says so in a comment — "command/menu undo stays + dispatchable". `*compilation*` and listview panels therefore carry the + same latent defect today. + - Fixed with `Buffer::set_generated_contents` (Lua + `pmacs.buffer.set_generated_contents`): lift `read_only`, replace + skipping intercepts, **discard history**, re-assert `read_only`. This + ships the deferred lane's two halves *as one primitive* — a bare + `set_read_only` would let a caller lock a buffer it can no longer + refresh, which is exactly why that lane was deferred. Clearing history + also stops a periodically refreshed buffer accumulating rope clones + nothing can ever pop. + - New pins: **acc16c** drives the real M-x path + (`command.invoke_interactive`), the chord, and redo, and asserts the + owner's refresh still works; **acc16b** flipped from asserting + `is_read_only()` is *false* to *true*, because the property it + described is the one that was fixed; plus three `buffer.rs` unit tests. + - Bite: restoring the `delete`+`insert` render reproduces the report + exactly — `left: Some("")` against the full snapshot — failing acc16c + and acc16b. + - **Still open:** `*compilation*` and listview remain emptiable by + `M-x buffer.undo`; the primitive they need now exists and is proven, + so the remainder is adoption plus a streaming-friendly variant. In + CRDT mode `read_only` is what refuses undo, since loro's `UndoManager` + exposes no clear through `CrdtState`. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index b7ced8e..9d2a494 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -508,6 +508,35 @@ additive, on its own binding, and does not replace scroll-and-select. "skip the intercepts". Naming only the setter would have made it look like a one-line follow-up. + **PARTIALLY RETIRED in Stage 2, because review round 2 turned it from a + nice-to-have into a defect.** An intercept guards the dispatch path only, + and `Buffer::undo` reaches the rope through `ensure_writable` without ever + consulting the intercept chain — so a single `C-/` replaced a freshly + rendered snapshot with an empty buffer. Rebinding the undo chords + buffer-locally, which is `*compilation*`'s existing idiom, does **not** + close it: `compile.lua` says so itself ("command/menu undo stays + dispatchable"), and `M-x buffer.undo` needs no keymap. + + The fix ships the deferral's two halves together as **one** primitive + rather than exposing the setter: `Buffer::set_generated_contents` (Lua: + `pmacs.buffer.set_generated_contents`) lifts `read_only`, replaces the + contents skipping intercepts, **discards the history**, and re-asserts + `read_only`. Pairing the lock with the write is precisely what makes it + safe — a bare `set_read_only` would let a caller lock a buffer it can no + longer refresh, which is why the lane was deferred in the first place. + Discarding history is load-bearing twice: it removes the entries undo + would replay, and it stops a periodically refreshed buffer accumulating + rope clones that `read_only` guarantees nothing can ever pop. + + **What remains of the lane:** `*compilation*` and listview panels still + rely on intercept-plus-round-trip and are still emptiable by + `M-x buffer.undo`. The primitive they need now exists and is proven, so + the remaining work is adoption plus a streaming-friendly variant + (`*compilation*` appends rather than replacing wholesale). The CRDT half + is also still open: `set_generated_contents` clears the v0.1 stacks, and + in CRDT mode `read_only` is what refuses undo, since loro's + `UndoManager` has no clear exposed through `CrdtState`. + ## Acceptance ### Stage 1 — `terminal-config` @@ -577,6 +606,15 @@ additive, on its own binding, and does not replace scroll-and-select. them for selection copy. 15. isearch over the snapshot finds content that is **only in scrollback** (scrolled off the visible screen), with no change to `src/search.rs` (B1). +16c. **Undo cannot empty the snapshot, by chord OR by command** (review + round 2). `Buffer::undo` bypasses the intercept chain entirely, so the + snapshot must be `read_only` at the rope. Pinning only the chords would + be a false pass: `M-x buffer.undo` and the menu reach the command with + no keymap involved, which is why `*compilation*`'s chord-rebinding idiom + does not close this. Pinned through **`invoke_interactive`**, the real + M-x path, plus the chord, plus redo — and paired with an assertion that + the owner's own refresh still works, since that is what plain + `read_only` would have broken. 16. **Ungated, runs in CI:** focusing the snapshot buffer makes `dispatch_idle_for` report **false**. This is the whole mechanism Q#TC6a depends on, it needs no CRDT, and it fails the moment diff --git a/src/buffer.rs b/src/buffer.rs index 7f048d5..27a496a 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -504,6 +504,55 @@ impl Buffer { self.read_only = read_only; } + /// Replace a generated buffer's entire contents on behalf of its owner, + /// and leave it genuinely immutable. + /// + /// This is the **owner-authorized update path** that genuine + /// immutability for generated buffers requires. A snapshot, panel or + /// `*compilation*` buffer must reject ordinary edits, **undo, redo**, + /// and remote CRDT imports alike — and only [`read_only`] does that. + /// An edit intercept is not enough: it guards the dispatch/edit path + /// only, while [`Buffer::undo`] reaches the rope through + /// `ensure_writable` without ever consulting the intercept chain. A + /// buffer protected by an intercept alone can therefore be emptied by + /// `C-/`, by `M-x buffer.undo`, or by the menu — the command is + /// reachable even where the chords are rebound to no-ops. + /// + /// But `read_only` also blocks the owner's own refresh, which is the + /// operation such buffers exist for. So the owner needs exactly one + /// door, and this is it: lift the flag, replace the contents skipping + /// intercepts, **discard the resulting history**, re-assert the flag. + /// + /// Discarding history is not tidiness. Without it every refresh pushes + /// undo entries holding full rope clones that nothing can ever pop — + /// `read_only` guarantees they are unreachable — so a periodically + /// refreshed buffer would grow without bound. + /// + /// [`read_only`]: Self::set_read_only + pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + self.read_only = false; + let result = self.replace_whole_buffer(bytes); + // Cleared even on failure: a partial replace must not leave a + // half-applied edit reachable through an undo the owner cannot see. + self.undo.clear(); + self.redo.clear(); + self.read_only = true; + result + } + + fn replace_whole_buffer(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + let len = self.len(); + if len > 0 { + self.apply_edit_skip_intercepts(EditOp::Delete { + range: Range::new(0, len), + })?; + } + if !bytes.is_empty() { + self.apply_edit_skip_intercepts(EditOp::Insert { pos: 0, bytes })?; + } + Ok(()) + } + fn ensure_writable(&self) -> Result<(), BufferError> { if self.read_only { Err(BufferError::ReadOnly { @@ -1955,6 +2004,72 @@ mod tests { } ); + /// The whole point of the primitive: after an owner write the buffer + /// is immutable, and `undo` — which never consults the intercept + /// chain — cannot reach back past it. + #[test] + fn set_generated_contents_writes_then_locks_and_leaves_nothing_to_undo() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + buf.set_generated_contents(b"first render").expect("write"); + + assert_eq!(buf.len(), 12); + assert!(buf.is_read_only(), "the buffer ends immutable"); + assert!( + matches!(buf.undo(), Err(BufferError::ReadOnly { .. })), + "undo must be refused at the rope, not merely at dispatch" + ); + assert!(matches!(buf.redo(), Err(BufferError::ReadOnly { .. }))); + + // Even with the lock lifted there is no history to replay — the + // protection does not depend on the flag alone. + buf.set_read_only(false); + assert!(matches!(buf.undo(), Err(BufferError::NothingToUndo))); + assert!(matches!(buf.redo(), Err(BufferError::NothingToRedo))); + } + + /// Refreshing repeatedly must not accumulate unreachable history. + /// Each render would otherwise push entries holding full rope clones + /// that `read_only` guarantees nothing can ever pop. + #[test] + fn repeated_generated_writes_do_not_accumulate_history() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + for i in 0..10 { + buf.set_generated_contents(format!("render {i}").as_bytes()) + .expect("write"); + } + let mut bytes = vec![0u8; buf.len() as usize]; + buf.snapshot_rope().slice(0, buf.len(), &mut bytes); + assert_eq!(String::from_utf8(bytes).expect("utf8"), "render 9"); + + buf.set_read_only(false); + assert!( + matches!(buf.undo(), Err(BufferError::NothingToUndo)), + "ten renders must leave an empty undo stack, not ten entries" + ); + } + + /// An ordinary edit is still refused after a generated write, so the + /// primitive does not quietly leave the buffer writable. + #[test] + fn set_generated_contents_still_refuses_ordinary_edits() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + buf.set_generated_contents(b"content").expect("write"); + assert!(matches!( + buf.apply_edit(EditOp::Insert { + pos: 0, + bytes: b"x" + }), + Err(BufferError::ReadOnly { .. }) + )); + assert!(matches!( + buf.apply_edit_skip_intercepts(EditOp::Insert { + pos: 0, + bytes: b"x" + }), + Err(BufferError::ReadOnly { .. }) + )); + } + #[cfg(feature = "crdt")] #[test] fn read_only_rejects_remote_crdt_before_import_and_allows_empty_bootstrap() { diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index e27d9dd..cff5bda 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3065,6 +3065,29 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result Date: Sun, 26 Jul 2026 13:43:22 -0400 Subject: [PATCH 4/6] fix(buffer): fan out generated writes, and clear the history that exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3, on the round-2 primitive itself. One lesson covers all three findings: a rope write is only half of an edit, and "discard history" means whichever history the buffer actually has. P1 — the binding swallowed the edit. `set_generated_contents` returned `()`, so nothing reached `notify_buffer_edit_to_windows`. Two consequences, both reproduced by the reviewer. In the default build a window showing the buffer kept a `TextView` line index describing the PREVIOUS contents, and the next paint indexed the new rope with stale ranges — `assertion failed: end <= self.len()` in `src/rope.rs`. In the CRDT build `pending_crdt_ops` stayed empty, so replica mirrors never imported the owner's write and their optimistic edits were generated against content already replaced. The `delete`+`insert` pair this replaced had done that fan-out for free. Now applies ONE whole-buffer `Replace`, returns its `Edit`, and notifies from the binding. The doc comment states the obligation, because the next owner to adopt the primitive inherits it. P2 — "discard history" was false in CRDT mode. The v0.1 stacks are bypassed entirely there; the history lives in loro's `UndoManager`. `read_only` stops the replay but not the retention, which is the memory cost the contract claims to eliminate. `UndoManager` exposes no clear, but needs none: it records only what happens after it is constructed, the same property `CrdtState::from_bytes` already uses to keep the seed insert out of undo. `CrdtState::clear_undo_history` rebinds a fresh manager to the same doc. P2 — the docs described the pre-fix architecture. Q#TC6a said no Lua binding sets `read_only` and round-trip input is the only guard; the acceptance text still said `is_read_only() == false` while 16b had been flipped to true; `terminal.lua`'s comment repeated the obsolete claim. The architecture is layered and now says so: rope-level read-only protects the daemon copy, round-trip input protects the replica's optimistic mirror, and neither substitutes for the other. Q#TC6a keeps its analysis under a superseded-in-part box rather than being silently rewritten — its conclusion survives, two of its premises do not. New pins. acc16d paints the window after a SHRINKING generated write: stale offsets then point past the buffer end, so the failure is the reported crash rather than merely stale pixels. acc16e asserts the refresh is queued for mirrors, through the real copy-mode path; `crdt`-gated and therefore dark in CI, which is why 16d drives the binding rather than the terminal. Plus a CRDT unit test that ten renders leave the `UndoManager` with nothing recorded. Bites: dropping the notify panics acc16d at `rope.rs:145` and fails acc16e with `queued: []`; dropping the `UndoManager` rebind fails the new unit test on `can_undo`. Still open, and recorded in COHERENCE.md §14: the fan-out obligation makes `*compilation*`/listview adoption more than a one-line swap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 8 +- builtin/runtime/terminal.lua | 31 ++-- docs/active-work.md | 49 +++++- docs/terminal-config-and-copy-mode-framing.md | 78 ++++++++- src/buffer.rs | 69 ++++++-- src/crdt.rs | 20 +++ src/lua_bindings/mod.rs | 19 ++- tests/terminal_copy_mode_acceptance.rs | 154 ++++++++++++++++++ 8 files changed, 382 insertions(+), 46 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 93298fc..0ba2ac8 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1223,7 +1223,13 @@ Primitive-by-primitive against the list above: dispatchable"). `Buffer::set_generated_contents` (write + discard history + assert `read_only`, in one authorized call) now fixes this for the terminal snapshot; `*compilation*` and listview panels have - not yet adopted it and remain emptiable. + not yet adopted it and remain emptiable. **A second half of the same + caveat, found in round 3: a rope write is only half of an edit.** The + owner-authorized write must be fanned out to the windows showing the + buffer and queued for replica mirrors, or the displaying window keeps + a line index describing the previous contents and the next paint + indexes the new rope with stale ranges. Adoption is therefore not a + one-line swap. - **Diagnostics collection** ✓ — `DiagnosticStore` + signs + unified `error.next` source. - **Transient selector** ✓ — the minibuffer (though its `source` diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index 5a912da..ef2fea4 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -329,7 +329,10 @@ local function render_snapshot(record) -- replaced a freshly rendered snapshot with an empty buffer. -- `set_generated_contents` writes, discards the history, and leaves -- `read_only` asserted, so undo/redo and remote CRDT imports are all - -- refused at the rope. + -- refused at the rope. Its binding also fans the resulting edit out to + -- the windows showing this buffer and to replica mirrors (review round + -- 3) — a rope write alone leaves a displaying window indexing the new + -- contents with stale line offsets. pmacs.buffer.set_generated_contents(record.buffer, text) end @@ -345,18 +348,22 @@ local function claim_snapshot(term_buf) local record = { terminal = term_buf, buffer = buf } handles[#handles + 1] = record - -- Q#TC6a — BOTH calls, and the second is the load-bearing one. + -- Q#TC6a — BOTH calls, and the protection is now LAYERED. Review + -- round 2 changed what each one is for. -- - -- An intercept guards the dispatch/edit path only. It does NOT set - -- `Buffer::read_only` (deliberately independent), and no Lua binding - -- sets that flag at all, so an optimistic CRDT op from a semantic - -- frontend bypasses the intercept AND passes `ensure_writable()` — - -- mutating the daemon buffer in lockstep with the mirror, with no - -- divergence to notice. `set_round_trip_input` prevents that at the - -- only point it can be prevented: `dispatch_idle_for` reports false - -- while this buffer is focused, so the frontend never applies - -- optimistically and never emits the op. It is the guard, not - -- hardening. + -- `set_generated_contents` leaves `read_only` asserted at the rope, so + -- on the DAEMON side undo, redo, ordinary edits and imported CRDT ops + -- are all refused by `ensure_writable()`. The intercept below is no + -- longer the daemon's guard; it survives to give a dispatching edit a + -- named error instead of a bare refusal. + -- + -- `set_round_trip_input` still guards the half `read_only` cannot + -- reach: a semantic frontend applies optimistically in its own MIRROR + -- before the daemon ever sees the op. `dispatch_idle_for` reports + -- false while this buffer is focused, so the mirror never mutates and + -- no op is emitted to be refused. That is the layering — rope-level + -- read-only protects the daemon copy, round-trip input protects the + -- replica copy — and neither substitutes for the other. pmacs.buffer.add_intercept(buf, function() error(name .. " is read-only") end) diff --git a/docs/active-work.md b/docs/active-work.md index 1ac337d..dc8c306 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -768,9 +768,52 @@ If it does not, stop and repair the remote/fetch configuration. and acc16b. - **Still open:** `*compilation*` and listview remain emptiable by `M-x buffer.undo`; the primitive they need now exists and is proven, - so the remainder is adoption plus a streaming-friendly variant. In - CRDT mode `read_only` is what refuses undo, since loro's `UndoManager` - exposes no clear through `CrdtState`. + so the remainder is adoption plus a streaming-friendly variant. +- **Review round 3 — one P1 and two P2s, all on the round-2 primitive.** + The lesson: **a rope write is only half of an edit, and "discard + history" means whichever history the buffer actually has.** + - **P1 — the binding swallowed the edit.** `set_generated_contents` + returned `()`, so nothing called `notify_buffer_edit_to_windows`. + Two consequences, both reproduced by the reviewer: in the default + build a window showing the buffer kept a `TextView` line index + describing the *previous* contents, and the next paint indexed the + new rope with stale ranges — `assertion failed: end <= self.len()` + in `src/rope.rs`; in the CRDT build `pending_crdt_ops` stayed empty, + so replica mirrors never received the owner's write. The prior + `buf:delete`/`buf:insert` pair had done this fan-out for free. + Fixed by applying **one whole-buffer `Replace`**, returning its + `Edit`, and notifying from the binding. + - **P2 — "discard history" was false in CRDT mode.** The v0.1 stacks + are bypassed entirely there; the history lives in loro's + `UndoManager`. `read_only` stops the replay but not the retention, + which is the memory cost the contract claims to eliminate. + `UndoManager` has no `clear`, but needs none — it records only what + happens after construction, the property `CrdtState::from_bytes` + already uses to keep the seed insert out of undo. New + `CrdtState::clear_undo_history` rebinds a fresh manager to the + same doc. + - **P2 — the docs described the pre-fix architecture.** Q#TC6a said no + Lua binding sets `read_only` and round-trip input is the only guard; + the acceptance text still said `is_read_only() == false` while 16b + had been flipped to `true`; `terminal.lua`'s comment repeated the + obsolete claim. The architecture is **layered** and now says so: + rope-level read-only protects the daemon copy, round-trip input + protects the replica's optimistic mirror, and neither substitutes + for the other. Q#TC6a carries a superseded-in-part box rather than + being silently rewritten. + - New pins: **acc16d** paints the window after a *shrinking* generated + write (the stale offsets then point past the end, which is the + reported crash rather than stale pixels); **acc16e** asserts the + refresh is queued for mirrors through the real copy-mode path + (`crdt`-gated, therefore dark in CI — 16d is the half that runs); + plus a CRDT `buffer.rs` unit test that ten renders leave the + `UndoManager` with nothing recorded. + - Bites: dropping the notify panics acc16d at `rope.rs:145` and fails + acc16e with `queued: []`; dropping the `UndoManager` rebind fails + the new unit test on `can_undo`. + - **Still open:** the fan-out obligation makes `*compilation*`/listview + adoption more than a one-line swap — recorded in `COHERENCE.md` §14 + alongside the undo half. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 9d2a494..3d9891d 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -6,7 +6,10 @@ `terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** **Stage 2 ships eight of its nine criteria, plus 18a and 18b added in review -round 1.** Criterion 17's semantic-frontend end-to-end pin is deliberately +round 1 and 16c-16e in rounds 2-3.** Rounds 2 and 3 changed the design, not +just the code: the snapshot is now genuinely `read_only` at the rope, so +**Q#TC6a's analysis below is superseded in part** — read the box at its head +before the analysis. Q#TC6a's conclusion survives; two of its premises do not. Criterion 17's semantic-frontend end-to-end pin is deliberately absent — see the note under it — because a faithful version requires the real `pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never compiles and which skips silently. Both halves of the *mechanism* it guards are @@ -348,6 +351,32 @@ ordinary document buffer, so: and `set_round_trip_input` is the ONLY thing standing between a replica frontend and unauthorized mutation.** +> **SUPERSEDED IN PART BY IMPLEMENTATION (review rounds 2-3). Read this +> box before the analysis below it.** The reasoning is still the correct +> account of the substrate *as it stood when this was written*, and its +> conclusion about round-trip input still holds. Two of its premises no +> longer do: +> +> - "**No Lua binding sets `read_only` at all**" — one does now. +> `pmacs.buffer.set_generated_contents` leaves it asserted, so on the +> daemon side undo, redo, ordinary edits and imported CRDT ops are all +> refused by `ensure_writable()`. That closed a real defect: undo +> bypasses the intercept chain, so `M-x buffer.undo` emptied the +> snapshot. +> - "**`set_round_trip_input` is the ONLY thing**" — it is now the only +> thing standing between a replica and *mirror* mutation, which is the +> half `read_only` cannot reach. A semantic frontend applies +> optimistically in its own mirror before the daemon sees the op; a +> daemon-side refusal cannot prevent that, it can only make the two +> copies disagree. +> +> The protection is therefore **layered, not singular**: rope-level +> read-only protects the daemon copy, round-trip input protects the +> replica copy, and neither substitutes for the other. The intercept +> survives only to give a dispatching edit a named error. The Deferred +> lane below records what this leaves open for `*compilation*` and +> listview, which have **not** adopted the primitive. + The established idiom is two calls: `listview.lua:106` and `compile.lua:272` each pair `pmacs.buffer.add_intercept` with `pmacs.buffer.set_round_trip_input(buf, true)`. Revision 2 described the @@ -382,7 +411,8 @@ Two things follow, and both are recorded rather than fixed here: genuinely immutable at the rope/CRDT boundary the way terminal identity buffers are, turning round-trip back into real defence in depth. That is a substrate change affecting listview and compile as much as this snapshot, so - it is named in Deferred with its own lane. + it is named in Deferred with its own lane. **Done for this snapshot only**, + and not by exposing the setter — see the Deferred lane and the box above. **Q#TC7 — the materializer reuses the existing serializer.** A whole-range variant of `copy_selection_bytes` over `retained_rows` inherits the criterion @@ -532,10 +562,19 @@ additive, on its own binding, and does not replace scroll-and-select. rely on intercept-plus-round-trip and are still emptiable by `M-x buffer.undo`. The primitive they need now exists and is proven, so the remaining work is adoption plus a streaming-friendly variant - (`*compilation*` appends rather than replacing wholesale). The CRDT half - is also still open: `set_generated_contents` clears the v0.1 stacks, and - in CRDT mode `read_only` is what refuses undo, since loro's - `UndoManager` has no clear exposed through `CrdtState`. + (`*compilation*` appends rather than replacing wholesale). + + **The CRDT half is closed too** (review round 3). Clearing the v0.1 + stacks proves nothing in CRDT mode, where they are bypassed entirely and + the history lives in loro's `UndoManager`. `read_only` would stop that + history being *replayed* but not *retained* — a panel refreshed on a + timer still grows without bound, which is the condition the contract + says it eliminates. `UndoManager` exposes no `clear`, but it needs none: + a manager records only what happens after it is constructed, which + `CrdtState::from_bytes` already relies on to keep the seed insert out of + undo. `CrdtState::clear_undo_history` rebinds a fresh manager to the same + doc, and `set_generated_contents` clears whichever history the buffer + actually has. ## Acceptance @@ -615,6 +654,23 @@ additive, on its own binding, and does not replace scroll-and-select. M-x path, plus the chord, plus redo — and paired with an assertion that the owner's own refresh still works, since that is what plain `read_only` would have broken. +16d. **A generated write reaches the window, not just the rope** (review + round 3). `set_generated_contents` returns one whole-buffer `Replace` + and its binding fans it out; swallowing it leaves a displaying + window's `TextView` line index describing the *previous* contents. + Pinned by **painting** — a shrinking write, so the stale offsets point + past the buffer end and the next render trips + `assertion failed: end <= self.len()` in `src/rope.rs`, which is the + reported crash rather than merely stale pixels. Driven through the Lua + binding copy mode itself calls, so it covers every future owner of the + primitive. +16e. **The same write is queued for replica mirrors** (review round 3, + CRDT half). The dropped fan-out also skipped + `queue_daemon_origin_crdt_op`, so a replica's mirror never imports the + owner's write and its optimistic edits are generated against content + already replaced. Pinned through the real copy-mode refresh on an + upgraded snapshot. `crdt`-gated, therefore dark in CI — 16d is the half + that actually runs there. 16. **Ungated, runs in CI:** focusing the snapshot buffer makes `dispatch_idle_for` report **false**. This is the whole mechanism Q#TC6a depends on, it needs no CRDT, and it fails the moment @@ -644,9 +700,13 @@ additive, on its own binding, and does not replace scroll-and-select. What IS pinned instead, ungated and in CI: acceptance 16 asserts the guard is armed (`dispatch_idle` false while the snapshot is focused, so no replica can apply optimistically or emit), and acceptance 16b asserts - the hazard is real by showing the snapshot buffer's `is_read_only()` is - **false** despite the intercept — i.e. nothing at the rope/CRDT boundary - would stop such an op if one arrived. Together those cover both halves of + the buffer is `is_read_only()` **true** at the rope, so an op that did + arrive at the daemon would be refused by `ensure_writable()` rather + than applied. (Rounds 1-2 asserted **false** here, documenting the + hazard; round 2 closed it, and the assertion was flipped with it. + That does not make 17 redundant — a daemon-side refusal cannot stop a + replica mutating its own mirror, which is precisely what + `set_round_trip_input` is for.) Together those cover both halves of Q#TC6a's *mechanism*. What remains unproven is only the end-to-end wire behaviour of a real GPU frontend, and it stays an explicit obligation of the CI `crdt`-coverage lane rather than being quietly dropped. diff --git a/src/buffer.rs b/src/buffer.rs index 27a496a..a9c01e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -526,31 +526,43 @@ impl Buffer { /// Discarding history is not tidiness. Without it every refresh pushes /// undo entries holding full rope clones that nothing can ever pop — /// `read_only` guarantees they are unreachable — so a periodically - /// refreshed buffer would grow without bound. + /// refreshed buffer would grow without bound. In CRDT mode the same + /// retention lives in loro's `UndoManager`, so both are cleared. + /// + /// # The returned edit must be fanned out + /// + /// One whole-buffer [`EditOp::Replace`] is applied, and its [`Edit`] + /// is returned rather than swallowed, because a rope write is only + /// half of an edit. Callers **must** route the result through their + /// normal edit-notification path (for the Lua surface, + /// `notify_buffer_edit_to_windows`). A window already displaying the + /// buffer keeps a stale `TextView` line cache otherwise, and the next + /// paint indexes the new rope with old ranges; and in CRDT mode the + /// op never reaches replica mirrors, so their optimistic edits are + /// generated against content the owner has already replaced. /// /// [`read_only`]: Self::set_read_only - pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result { self.read_only = false; - let result = self.replace_whole_buffer(bytes); + let result = self.apply_edit_skip_intercepts(EditOp::Replace { + range: Range::new(0, self.len()), + bytes, + }); // Cleared even on failure: a partial replace must not leave a // half-applied edit reachable through an undo the owner cannot see. - self.undo.clear(); - self.redo.clear(); + self.clear_history(); self.read_only = true; result } - fn replace_whole_buffer(&mut self, bytes: &[u8]) -> Result<(), BufferError> { - let len = self.len(); - if len > 0 { - self.apply_edit_skip_intercepts(EditOp::Delete { - range: Range::new(0, len), - })?; + /// Drop undo and redo history in whichever mode this buffer is in. + fn clear_history(&mut self) { + self.undo.clear(); + self.redo.clear(); + #[cfg(feature = "crdt")] + if let Some(crdt) = self.crdt.as_ref() { + crdt.clear_undo_history(); } - if !bytes.is_empty() { - self.apply_edit_skip_intercepts(EditOp::Insert { pos: 0, bytes })?; - } - Ok(()) } fn ensure_writable(&self) -> Result<(), BufferError> { @@ -2048,6 +2060,33 @@ mod tests { ); } + /// Review round 3, P2. In CRDT mode the v0.1 stacks are bypassed + /// entirely, so clearing them proves nothing: the history the + /// primitive promises to discard lives in loro's `UndoManager`. + /// The lock is lifted deliberately — `read_only` stops the replay, + /// but the contract is that there is nothing left to replay. + #[cfg(feature = "crdt")] + #[test] + fn generated_writes_accumulate_no_crdt_history_either() { + let mut buf = + Buffer::new_with_crdt(BufferId::next(), "*generated*", 1).expect("crdt construction"); + for i in 0..10 { + buf.set_generated_contents(format!("render {i}").as_bytes()) + .expect("write"); + } + assert_eq!(rope_string(&buf), "render 9"); + assert!( + !buf.crdt_state().expect("crdt-backed").can_undo(), + "the UndoManager must have nothing recorded" + ); + + buf.set_read_only(false); + assert!( + matches!(buf.undo(), Err(BufferError::NothingToUndo)), + "CRDT-mode undo must find no history either" + ); + } + /// An ordinary edit is still refused after a generated write, so the /// primitive does not quietly leave the buffer writable. #[test] diff --git a/src/crdt.rs b/src/crdt.rs index 8cef9e3..ca68d1a 100644 --- a/src/crdt.rs +++ b/src/crdt.rs @@ -486,6 +486,26 @@ impl CrdtState { pub fn record_checkpoint(&self) -> LoroResult<()> { self.undo.borrow_mut().record_new_checkpoint() } + + /// Discard the bound peer's undo and redo history, keeping the + /// document itself untouched. + /// + /// Loro's `UndoManager` exposes no `clear`, but it does not need + /// one: a manager records only what happens **after** it is + /// constructed. [`Self::from_bytes`] already relies on exactly + /// that property to keep the seed insert out of undo. Replacing + /// the manager with a fresh one bound to the same doc therefore + /// leaves nothing to undo, and drops the old manager's retained + /// stacks with it. + /// + /// Used by [`crate::buffer::Buffer::set_generated_contents`], whose + /// contract is that a generated buffer accumulates no history + /// across refreshes. Marking the buffer read-only would stop the + /// history being *replayed*, but not being *retained* — a panel + /// refreshed on a timer would grow without bound. + pub fn clear_undo_history(&self) { + *self.undo.borrow_mut() = Self::create_undo_manager(&self.doc); + } } /// T M10.3: map a [`crate::protocol::FrontendId`] to the loro `PeerID` diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index cff5bda..aa5de88 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3077,12 +3077,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result pmacs::buffer::BufferId { + let core = state.core.borrow(); + let reg = core.registry.borrow(); + reg.ids() + .iter() + .copied() + .find(|id| reg.get(*id).is_ok_and(|b| b.name() == SNAPSHOT_NAME)) + .expect("snapshot buffer exists") +} + +/// Rendered cells of the active window (the `m4_acceptance` grid helper; +/// cross-crate test code can't import it). +fn render_active_window_to_grid( + state: &mut EditorState, + rows: u32, + cols: u32, +) -> Vec { + use pmacs::cell::{Cell, CellGrid}; + use pmacs::view::{View, Viewport}; + use pmacs::window::Rect; + + let mut core = state.core.borrow_mut(); + let active = core.active_window_id(); + let registry = core.registry.clone(); + let win = core.windows.get_mut(&active).expect("active window"); + let rect = Rect::new(0, 0, rows, cols); + let mut backing = vec![Cell::default(); (rows * cols) as usize]; + let reg = registry.borrow(); + let buf = reg.get(win.buffer_id).expect("buffer in registry"); + let viewport = Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: rect.origin, + cell_size: CellSize::new(rows, cols), + gutter_w: 0, + folds: None, + }; + let mut grid = CellGrid { + cells: &mut backing, + stride: cols, + size: CellSize::new(rows, cols), + }; + win.text_view.render(buf, viewport, &mut grid); + backing +} + +fn grid_row(cells: &[pmacs::cell::Cell], row: u32, cols: u32) -> String { + (0..cols) + .map(|c| match cells[(row * cols + c) as usize].glyph { + Glyph::Char(ch) => ch, + _ => ' ', + }) + .collect::() + .trim_end() + .to_owned() +} + +/// Review round 3, P1. A rope write is only half of an edit: the window +/// showing the buffer holds a `TextView` line index that only `on_edit` +/// maintains, so a write that reaches the rope without the notification +/// leaves the two disagreeing. +/// +/// Pinned by PAINTING, because that is where the disagreement bites: with +/// the fan-out dropped, the next render indexes the new rope with the old +/// line offsets. A shrinking write is used deliberately — stale offsets +/// then point past the buffer end, which is the reported crash rather than +/// merely stale pixels. +/// +/// Driven through `pmacs.buffer.set_generated_contents`, the seam copy +/// mode's refresh actually calls, so it also covers `*compilation*` and +/// any other owner that adopts the primitive later. +#[test] +fn acc16d_a_generated_write_notifies_the_window_that_displays_it() { + let mut state = EditorState::new(); + exec( + &state, + r" + GEN = pmacs.buffer.create('*generated-probe*') + pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n') + pmacs.window.switch_buffer(GEN) + ", + ); + let painted = render_active_window_to_grid(&mut state, 6, 20); + assert_eq!( + grid_row(&painted, 0, 20), + "alpha", + "precondition: the window paints the generated buffer" + ); + + exec( + &state, + r"pmacs.buffer.set_generated_contents(GEN, 'CHANGED\n')", + ); + let painted = render_active_window_to_grid(&mut state, 6, 20); + assert_eq!( + grid_row(&painted, 0, 20), + "CHANGED", + "the window must paint the refreshed contents" + ); + assert_eq!( + grid_row(&painted, 1, 20), + "", + "and nothing of the longer contents it replaced" + ); +} + +/// Review round 3, P1, CRDT half. The same dropped fan-out also skips +/// `queue_daemon_origin_crdt_op`, so replica mirrors never import the +/// owner's write and their optimistic edits are generated against content +/// the owner has already replaced. +/// +/// Gated because `upgrade_to_crdt` is — and therefore dark in CI, which +/// never enables the feature. The default-configuration half above is the +/// one that actually runs there. +#[cfg(feature = "crdt")] +#[test] +fn acc16e_a_refresh_queues_the_owners_write_for_replica_mirrors() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let snapshot = snapshot_buffer_id(&state); + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + let buffer = reg.get_mut(snapshot).expect("snapshot buffer"); + // `read_only` refuses the upgrade's own bookkeeping path the same + // way it refuses everything else, so lift it around the upgrade. + buffer.set_read_only(false); + buffer.upgrade_to_crdt(2).expect("upgrade"); + buffer.set_read_only(true); + } + state.core.borrow_mut().pending_crdt_ops.clear(); + + emit_into_child(&mut state, terminal, "MIRRORME"); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let queued: Vec<_> = state + .core + .borrow() + .pending_crdt_ops + .iter() + .map(|(_, id, _)| *id) + .collect(); + assert!( + queued.contains(&snapshot), + "the owner's refresh must be queued for broadcast; queued: {queued:?}" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + /// Acceptance 18: re-invoking refreshes in place, and the lifecycle runs /// both directions. #[test] From 23c966cc31142ad6ce2e4ba4cc32749b8eaff9be Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 14:38:22 -0400 Subject: [PATCH 5/6] docs(terminal): restate criterion 17's bite for the fixed substrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4, P2. A fix can invalidate a test that was never written. Criterion 17 still specified the pre-round-2 world: remove `set_round_trip_input` and the optimistic op "passes `ensure_writable()` and mutates BOTH sides, silently, with no divergence to notice". That was true while no Lua binding set `read_only`. Since `set_generated_contents` does, the daemon refuses the op — so only the frontend's own mirror mutates, and the copies diverge. The gap matters precisely because 17 is unpinned. A real-GPU test written to the old spec would hunt for a daemon-side edit that can no longer occur and pass for the wrong reason, quietly readmitting the round-2 regression through a test not yet built. The specification is the artifact under review here, not the code. Restated around unauthorized MIRROR mutation plus daemon refusal — divergence — in all four places carrying the obsolete claim: the criterion itself, the Q#TC6a heading, the acceptance-16 doc comment, and the bite roster. The heading's "ONLY thing" now says what it is the only thing FOR: the replica's own mirror. `docs/active-work.md` also still described acceptance 16b as asserting `is_read_only()` is false, which round 2 flipped. Why round-trip input stays load-bearing rather than redundant, now stated wherever the daemon guard is mentioned: a refusal arrives after the frontend has already applied optimistically and painted. It buys divergence instead of silent agreement; it does not prevent the mutation the user is looking at. Also recorded, after capturing it properly this time: the gate-run flake in `cargo test --lib --features crdt` is `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` (`active_reader_probe` -> None, "live runtime probe"), ~1 run in 5. Pre-existing and unrelated — this branch does not touch `src/process.rs`, the test passes 10/10 standalone and 2017/2017 at `--test-threads=1`, and it is another instance of the known `drain_until` trap: draining for `Started` also ticks, and a tick reaps the leader. That also explains the unattributed "2 failed" run noted in round 2. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- docs/active-work.md | 45 +++++++++++++++++-- docs/terminal-config-and-copy-mode-framing.md | 39 +++++++++++----- tests/terminal_copy_mode_acceptance.rs | 11 ++++- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index dc8c306..62a9b6d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -685,10 +685,15 @@ If it does not, stop and repair the remote/fetch configuration. second test on that footing buys the appearance of coverage. Both halves of the mechanism are pinned **ungated** instead: acceptance 16 (the guard is armed — `dispatch_idle` false while the snapshot is - focused) and 16b (the hazard is real — the snapshot's `is_read_only()` - is **false** despite the intercept, so nothing at the rope/CRDT - boundary would stop an op that did arrive). The wire-level half is an - explicit obligation of the CI `crdt`-coverage lane. + focused) and 16b (the daemon holds — `is_read_only()` is **true** at + the rope, so an op that did arrive is refused by `ensure_writable()`). + **Rounds 2-3 changed what 17 must show.** 16b asserted `false` through + round 1, documenting the hazard; round 2 closed it. So the eventual + real-GPU test must look for **mirror mutation plus daemon refusal — + divergence** — not the "mutates both sides, silently" the criterion + originally specified, which after the fix cannot happen and would pass + for the wrong reason. The wire-level half stays an explicit obligation + of the CI `crdt`-coverage lane. - Load-bearing Stage 2 decisions: - **The snapshot MATERIALIZES into an ordinary buffer**, so isearch, motion, selection and the kill ring work with no new substrate, and @@ -814,6 +819,38 @@ If it does not, stop and repair the remote/fetch configuration. - **Still open:** the fan-out obligation makes `*compilation*`/listview adoption more than a one-line swap — recorded in `COHERENCE.md` §14 alongside the undo half. +- **Review round 4 — one P2, docs only, and it is the interesting kind.** + **A fix can invalidate a test that was never written.** Criterion 17's + *bite* still described the pre-round-2 world: remove + `set_round_trip_input` and the op "mutates both sides, silently, with + no divergence to notice". True while nothing set `read_only` from Lua; + false once `set_generated_contents` did. A real-GPU test written to + that spec would hunt for a daemon-side edit that can no longer occur + and pass for the wrong reason — the specification would have leaked + the round-2 regression back in, through a test not yet built. + - Restated around **unauthorized mirror mutation plus daemon refusal = + divergence**, in all four places that carried the old claim: the + criterion, the Q#TC6a heading, the acceptance-16 doc comment, and the + bite roster. The heading's "ONLY thing" now says what it is the only + thing *for* — the replica's own mirror. + - Why round-trip input is still load-bearing rather than redundant: a + daemon refusal arrives after the frontend has already applied + optimistically and painted. It buys divergence instead of silent + agreement; it does not prevent the mutation the user sees. + - **Gate-run flake identified and attributed, not waved off.** + `cargo test --lib --features crdt` failed ~1 run in 5 on + `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` + — `active_reader_probe` returning `None` at `process.rs:3179` + ("live runtime probe"). **Pre-existing and unrelated:** this branch + does not touch `src/process.rs` (last changed by the Darwin PTY + signal-name fix), and the test passes 10/10 standalone, failing only + under full-suite parallelism. It is **another instance of the known + `drain_until` trap** — draining for `Started` to learn the pid also + ticks, and a tick reaps the leader, so the probe that follows finds + nothing live. Same module and same signature as the earlier + `signal`-says-"is not running" case. This also explains the + unattributed "2 failed" CRDT run recorded in round 2. Belongs to the + CI `crdt`-coverage lane, which is where the whole class lives. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 3d9891d..15878cf 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -9,7 +9,12 @@ round 1 and 16c-16e in rounds 2-3.** Rounds 2 and 3 changed the design, not just the code: the snapshot is now genuinely `read_only` at the rope, so **Q#TC6a's analysis below is superseded in part** — read the box at its head -before the analysis. Q#TC6a's conclusion survives; two of its premises do not. Criterion 17's semantic-frontend end-to-end pin is deliberately +before the analysis. Q#TC6a's conclusion survives; two of its premises do +not, and **criterion 17's bite was restated with them** — the daemon now +refuses the op, so the failure it must look for is mirror mutation plus +divergence, not silent agreement. + +Criterion 17's semantic-frontend end-to-end pin is deliberately absent — see the note under it — because a faithful version requires the real `pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never compiles and which skips silently. Both halves of the *mechanism* it guards are @@ -347,9 +352,10 @@ ordinary document buffer, so: inspectable — the idiom `COHERENCE.md` §6 identifies as the right side of the line. -**Q#TC6a — the snapshot is BOTH intercept-read-only AND round-trip-marked, -and `set_round_trip_input` is the ONLY thing standing between a replica -frontend and unauthorized mutation.** +**Q#TC6a — the snapshot is read-only at the rope AND round-trip-marked, and +each guard covers a copy the other cannot reach: `read_only` refuses the op +at the daemon, `set_round_trip_input` is the ONLY thing standing between a +replica frontend and unauthorized mutation of its own mirror.** > **SUPERSEDED IN PART BY IMPLEMENTATION (review rounds 2-3). Read this > box before the analysis below it.** The reasoning is still the correct @@ -680,10 +686,22 @@ additive, on its own binding, and does not replace scroll-and-select. 17. **Through a semantic frontend** (this one does need CRDT): keys typed in the snapshot buffer reach ordinary dispatch and never the child, and **neither the daemon buffer nor the frontend's mirror is mutated** - (Q#TC6a). Bite: with `set_round_trip_input` removed, the optimistic op is - emitted, bypasses the Lua intercept, passes `ensure_writable()`, and - mutates **both sides** — a buffer the editor calls read-only silently - accepts an edit. + (Q#TC6a). Bite: with `set_round_trip_input` removed, the frontend + applies the edit **optimistically to its own mirror** and emits the op; + the mirror now shows text the user was told is read-only. The daemon + refuses the op at `ensure_writable()` — `set_generated_contents` leaves + `read_only` asserted — so the two copies **diverge**, and the local + mirror is the one the user is looking at. + + **This bite changed in review round 3, and the direction matters.** + Rounds 1-2 specified it as "mutates *both sides*, silently, with no + divergence to notice" — true when nothing set `read_only` from Lua, + and false now. The eventual real-GPU test must assert **mirror + mutation plus daemon refusal**, not silent agreement; written the old + way it would look for a daemon-side edit that can no longer happen and + pass for the wrong reason. That the daemon now holds is exactly why + round-trip input is still load-bearing rather than redundant: a + refusal protects the daemon's copy and does nothing for the replica's. **NOT PINNED as specified, deliberately, and this is the one gap in Stage 2.** A faithful test has to drive the *real* `pmacs-gpu` binary: @@ -795,8 +813,9 @@ Full gate suite per `CLAUDE.md` for each PR separately, plus: implementations (epoch-only key, single last-entry, unpurged map), which is why one pin was not enough; **9** (a hardcoded `0x03` makes the configured chord unreachable); **10** (its failure mode is a terminal nobody can - escape); and **16/17** (a read-only buffer that silently accepts an edit on - both sides). + escape); and **16** (a read-only buffer whose replica mirror accepts an + edit the user is then looking at — 17's daemon half was closed in review + round 2, and its bite restated in round 3). - **The observation seams the cache pins need are `escape_parses` (how often) and `escape_caches` (how many are still held).** Neither is inferable from behavior: for a *valid* setting a correct per-session cache and a leaking diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index 9572cbc..ed3f6d7 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -308,8 +308,15 @@ fn acc15_isearch_finds_content_only_in_scrollback() { /// Acceptance 16 — the load-bearing pin, and the reason this suite is /// ungated. `set_round_trip_input` is the ONLY thing standing between a -/// replica frontend and unauthorized mutation (Q#TC6a), so its regression -/// must be caught in the configuration CI actually compiles. +/// replica frontend and unauthorized mutation **of its own mirror** +/// (Q#TC6a), so its regression must be caught in the configuration CI +/// actually compiles. +/// +/// Rope-level `read_only` does not substitute for it. Since review round 2 +/// the daemon refuses such an op at `ensure_writable()` — but a refusal +/// arrives after the frontend has already applied optimistically and +/// painted the result. What that buys is divergence instead of silent +/// agreement; what stops the mutation is this. #[test] fn acc16_dispatch_idle_is_false_while_the_snapshot_is_focused() { let mut state = EditorState::new(); From a58bb110bc871e95126381d97486e9388bdd50f5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 14:54:37 -0400 Subject: [PATCH 6/6] docs(terminal): qualify process flake attribution Separate the observed suite behavior from the suspected drain_until mechanism, and leave round-2 failures unattributed without test names. --- docs/active-work.md | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 62a9b6d..3a5bb88 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -837,20 +837,23 @@ If it does not, stop and repair the remote/fetch configuration. daemon refusal arrives after the frontend has already applied optimistically and painted. It buys divergence instead of silent agreement; it does not prevent the mutation the user sees. - - **Gate-run flake identified and attributed, not waved off.** + - **Gate-run flake observed and scoped without overclaiming its cause.** `cargo test --lib --features crdt` failed ~1 run in 5 on `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` — `active_reader_probe` returning `None` at `process.rs:3179` ("live runtime probe"). **Pre-existing and unrelated:** this branch does not touch `src/process.rs` (last changed by the Darwin PTY - signal-name fix), and the test passes 10/10 standalone, failing only - under full-suite parallelism. It is **another instance of the known - `drain_until` trap** — draining for `Started` to learn the pid also - ticks, and a tick reaps the leader, so the probe that follows finds - nothing live. Same module and same signature as the earlier - `signal`-says-"is not running" case. This also explains the - unattributed "2 failed" CRDT run recorded in round 2. Belongs to the - CI `crdt`-coverage lane, which is where the whole class lives. + signal-name fix), and the test passed 10/10 standalone; the observed + failures were during parallel full-suite runs. That localizes the + trigger to suite load or interaction, but does **not** distinguish + parallelism from another full-suite effect — no serial full-suite bite + was run. The leading code-path explanation is the known `drain_until` + trap: draining for `Started` also ticks, and a tick can reap the leader + before the following `active_reader_probe`. That is an inference from + the failure site and control flow, not yet a falsified root cause. + It belongs to the CI `crdt`-coverage lane for discrimination. The two + round-2 CRDT failures had no captured test names; this flake is a + plausible candidate for them, but they remain **unattributed**. - Load-bearing decisions, each forced by scouted ground truth: - profiles are a **raw Lua table** — `ConfigValue` is four scalars with no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`;