diff --git a/COHERENCE.md b/COHERENCE.md index 4e7361c..0ba2ac8 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 / @@ -1182,7 +1214,22 @@ 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. **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 143a663..ef2fea4 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,248 @@ 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") + +-- An ARRAY of `{ terminal = , buffer = }`, scanned linearly and +-- compared with `==`, following dired's handle table (F7). +-- +-- 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) + if ok and described then return described.name end + return nil +end + +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 + 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_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. +local function render_snapshot(record) + local text = raw_copy_retained(record.terminal) or "" + -- 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. 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 + +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 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 protection is now LAYERED. Review + -- round 2 changed what each one is for. + -- + -- `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) + 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 — `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 + -- 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 ok, valid = pcall(record.buffer.is_valid, record.buffer) + if ok and valid then pcall(pmacs.buffer.kill, record.buffer) end + end) + + return record +end + +-- The snapshot record whose buffer the active window shows, or nil. +local function snapshot_for_current_buffer() + return handle_for_snapshot(pmacs.window.buffer()) +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 = claim_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 bd85ca1..7df3466 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -468,7 +468,7 @@ If it does not, stop and repair the remote/fetch configuration. **not** `crdt`-gated and do run under CI's exact flags, including the controller-release pin whose only job is catching the plausible wrong fix. -## Terminal config + copy mode arc — Stage 1 MERGED; Stage 2 is next +## 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 @@ -481,9 +481,188 @@ If it does not, stop and repair the remote/fetch configuration. binding; no protocol change. Main was integrated **twice** during the single review round (`ccf29e3`, then `c93f9ee` after the first merge left the PR conflicting) — see the no-CI-while-conflicting fact below. -- **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 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 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 + "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. +- **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. +- **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. +- **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. +- **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 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 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 48b75d8..15878cf 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -1,9 +1,31 @@ # 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, plus 18a and 18b added in review +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, 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 +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 @@ -330,9 +352,36 @@ 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 +> 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 @@ -368,7 +417,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 @@ -494,6 +544,44 @@ 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 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 ### Stage 1 — `terminal-config` @@ -563,6 +651,32 @@ 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. +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 @@ -572,18 +686,85 @@ 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: + 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 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. 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 @@ -632,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/src/buffer.rs b/src/buffer.rs index 7f048d5..a9c01e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -504,6 +504,67 @@ 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. 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 { + self.read_only = false; + 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.clear_history(); + self.read_only = true; + result + } + + /// 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(); + } + } + fn ensure_writable(&self) -> Result<(), BufferError> { if self.read_only { Err(BufferError::ReadOnly { @@ -1955,6 +2016,99 @@ 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" + ); + } + + /// 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] + 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/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 a43a195..aa5de88 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3065,6 +3065,36 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result 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..ed3f6d7 --- /dev/null +++ b/tests/terminal_copy_mode_acceptance.rs @@ -0,0 +1,994 @@ +//! 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 +} + +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. +/// 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, 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 { + 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 **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(); + 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 buffer is genuinely `read_only` at the rope boundary, so the +/// protection does not depend on which key or command was used. +#[test] +fn acc16b_the_snapshot_is_immutable_at_the_rope_not_merely_intercepted() { + 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"); + + 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(), + "an intercept guards the dispatch path only; `Buffer::undo` reaches \ + the rope through `ensure_writable` without consulting it, so the \ + snapshot must be read-only at the rope" + ); + drop(registry); + drop(core); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 16c (review round 2, P1): **undo cannot empty the snapshot**, +/// through the chord *or* through the command. +/// +/// The chord half alone would be a false pass. `M-x buffer.undo` and the +/// menu reach `Buffer::undo` without passing through any buffer-local +/// keymap, so rebinding `C-/` to a no-op — the existing `*compilation*` +/// idiom, which documents that "command/menu undo stays dispatchable" — +/// leaves the buffer emptiable. Only rope-level `read_only` closes both. +#[test] +fn acc16c_undo_cannot_empty_the_snapshot_by_chord_or_by_command() { + 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 rendered = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + assert!( + rendered.contains("LINE200"), + "precondition: the snapshot has content to lose" + ); + + // The command path — reachable regardless of any buffer-local binding. + let _: Value = state + .lua_host + .lua() + .load(r"return pcall(pmacs.command.invoke_interactive, 'buffer.undo')") + .eval() + .expect("invoke_interactive is callable"); + assert_eq!( + buffer_text_by_name(&state, SNAPSHOT_NAME).as_deref(), + Some(rendered.as_str()), + "M-x buffer.undo must not empty the snapshot" + ); + + // The chord path. + press(&mut state, KeyCode::Char('/'), KeyModifiers::CONTROL); + assert_eq!( + buffer_text_by_name(&state, SNAPSHOT_NAME).as_deref(), + Some(rendered.as_str()), + "C-/ must not empty the snapshot" + ); + + // Redo is the same door. + let _: Value = state + .lua_host + .lua() + .load(r"return pcall(pmacs.command.invoke_interactive, 'buffer.redo')") + .eval() + .expect("invoke_interactive is callable"); + assert_eq!( + buffer_text_by_name(&state, SNAPSHOT_NAME).as_deref(), + Some(rendered.as_str()), + "buffer.redo must not alter the snapshot either" + ); + + // ...and the owner's own refresh still works, which is the whole + // reason plain `read_only` was not enough on its own. + emit_into_child(&mut state, terminal, "STILLREFRESHES"); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("STILLREFRESHES"), + "the owner-authorized write path must survive immutability" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Snapshot buffer id, by name, from the Rust side. +#[cfg(feature = "crdt")] +fn snapshot_buffer_id(state: &EditorState) -> 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] +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); + 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, + "...and it 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" + ); + + // `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" + ); + + // 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(); +} + +/// 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); + 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. + 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 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!( + 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(); +} + +/// 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(); +} + +/// 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] +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}" + ); +}