diff --git a/COHERENCE.md b/COHERENCE.md index f172a21..7f8f456 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -120,7 +120,7 @@ asymmetry**, and **per-arc coherence debt**. Coherence-shaped work already in flight at audit time: find-file / dired Stage 0 (`C-x C-f`, merged #162, `docs/dired-framing.md`) and its -Stage 1 directory view (PR #165), bottom panel Stage 1 (merged #155), +Stage 1 directory view (merged #165), bottom panel Stage 1 (merged #155), multi-root LSP affinity (merged #161), the config registry foundation (merged #127). @@ -195,7 +195,7 @@ working, unreachable capability: time; a complete 1,384-line dired existed only as a frozen test fixture (`tests/fixtures/pmacs-dired/init.lua`). **Fixed:** dired Stage 0 opens a path (`C-x C-f`, merged #162) and Stage 1 ships the - browsing view as a builtin (`C-x d` / `C-x C-j`, PR #165). The fixture + browsing view as a builtin (`C-x d` / `C-x C-j`, merged #165). The fixture stays frozen — its `install_local` + `require` routing *is* the M8 package-universality proof (Q#DR1) — and shrinking it is scheduled after Stage 3. @@ -363,12 +363,12 @@ Full verdict table: |---|---|---|---| | 1 | Install | **Partial** | Source build only: `cargo build --release --workspace --features pmacs/crdt` (`README.md`). No binaries, no packaging. Runtime deps (`/bin/sh`, git, tar, coreutils) documented, never checked at runtime | | 2 | Launch unconfigured | **Works** | `EditorState::new()` → empty `*scratch*`; missing config is not an error (`src/config.rs:7-9`); recentf/saveplace/autosave default-on | -| 3 | Open real project | **Missing at the CLI** | `pmacs .` still exits 1 (above): `load_file` does `File::open` (which succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so `resolve_target_buffer`'s create-a-`[new file]` arm never fires. Dired Stage 1 (PR #165) supplies the buffer a directory should resolve *to*; routing `pmacs .` into it is Journey Stage 1's work, which must not invent a second directory surface | +| 3 | Open real project | **Missing at the CLI** | `pmacs .` still exits 1 (above): `load_file` does `File::open` (which succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so `resolve_target_buffer`'s create-a-`[new file]` arm never fires. Dired Stage 1 (merged #165) supplies the buffer a directory should resolve *to*; routing `pmacs .` into it is Journey Stage 1's work, which must not invent a second directory surface | | 4 | Understand interface | **Partial** | Mode line gives name/modified/L:C/scroll + mode/LSP/terminal segments; but no welcome text (`EditorCore::new` sets `status: String::new()`), no cheat sheet, and `C-h` deletes a word (§18) | | 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.* | +| 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing #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), 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) | @@ -467,7 +467,7 @@ level is the one missing. Audited level-by-level: **Beginner** (should see: files, buffers, search, diagnostics, terminal, build actions, menus, missing-tool guidance): -- files ✓ since #162 / PR #165 (`C-x C-f` opens a path, `C-x d` browses; +- files ✓ since #162 / #165 (`C-x C-f` opens a path, `C-x d` browses; neither is advertised anywhere but the keymap) · buffers ✓ (`C-x b`, `*buffer-list*`) · search ✓ (`C-s`/`C-r`/`C-M-s`; project.search is M-x-only) · diagnostics ✓ once a server runs · terminal ✓ but @@ -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 → @@ -1044,10 +1063,23 @@ 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 / - hooks+keymaps / packages), but with eight settings registered, level 1 + hooks+keymaps / packages), but with nine settings registered, level 1 is effectively empty — users need executable Lua for nearly every ordinary preference, which is the exact failure the section warns about. @@ -1185,7 +1217,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` @@ -1204,7 +1251,7 @@ Primitive-by-primitive against the list above: hierarchy, package dependency graph, worker trees, git status) will each need it; building it once *before* dired's directory view and the workers tree harden their own conventions is exactly this - section's point. Dired Stage 1 (PR #165) landed **without** inventing + section's point. Dired Stage 1 (merged #165) landed **without** inventing one: its listing is flat (Emacs parity), and the recursive in-buffer case — `i` insert-subdirectory — is a named deferral in `docs/dired-framing.md` §13, which is where a shared tree primitive @@ -1368,7 +1415,8 @@ greets a new user says nothing (`EditorCore::new` sets an empty status). Note the dependency: five of the ten onboarding steps above currently -lead somewhere broken or invisible (find a file — in flight; inspect a +lead somewhere broken or invisible (find a file — the mechanism is fixed +since #162/#165 but is advertised nowhere except the keymap; inspect a diagnostic — silent-failure risk; view workers — undiscoverable; setting provenance — unanswerable). Onboarding is correctly sequenced *after* the P1/P4 fixes, but the cheap floor — a welcome buffer in @@ -1432,7 +1480,7 @@ Establish the end-to-end workflow; treat regressions as release blockers. **State: broken at step 3 (§2). Mostly wiring, and unusually cheap:** directory-argument handling (the remaining half of step 3 — dired Stage 1 landed the buffer it should resolve to); a find-file -surface (**done**: #162 open-by-path, PR #165 browsing); surfacing the +surface (**done**: #162 open-by-path, #165 browsing); surfacing the LSP spawn failure with guidance (§1.2); a compile keybinding + `cargo build`/`test` default from the existing `ProjectKind::Cargo`; a terminal keybinding; a welcome buffer. The @@ -1502,7 +1550,9 @@ implementation — this list is direction, not commitment): 1. **Journey Stage 1** (P1): directory open + compile defaults + LSP-failure surfacing + bindings + welcome buffer + the first - journey acceptance suite. Rides alongside the in-flight dired arc. + journey acceptance suite. Dired Stage 1 has landed (#165), so the + buffer a directory resolves *to* already exists; this arc routes + `pmacs .` into it rather than growing a second directory surface. 2. **Discovery surface** (P4): the describe/list/where-is command family, M-x rich rows, help unification, help prefix. 3. **Transient keymap layer** (§6): the overlay scope + lifetime 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 7bb63ff..034b1fd 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -5,6 +5,15 @@ landed on `main`. Read it after `docs/agent-handoff.md`. Remove completed entries when their PR merges; do not let this become a second permanent backlog. +**Two lane headers below are stale on purpose**, pending the docs updates +their own lanes owe: multi-root LSP affinity **#161 has merged** (the +Lean 4 lane still says IN REVIEW; its continuation is PR #167) and GPU +terminal input **#166 has merged** (its lane still says IN REVIEW; PR +#168 records it). Trust the canonical-base line below over a lane header: +if a PR number appears in `git log --first-parent githubsucks/main`, it +has landed regardless of what its lane says. (The inline-math lane was +here too until #172 removed it — that is the update those two owe.) + ## Repository authority - Canonical development URL: @@ -14,13 +23,19 @@ backlog. machine-local: `origin` may name this canonical URL, a release mirror, or something else, and therefore has no authority by name alone. - Canonical base at this snapshot: - `githubsucks/main` @ `a27f646` (Lean 4 Stage 4a #179 atop Stage 3b - #170, Stage 3a #167, the bottom-panel landed-doc refresh #156, the inline-math slice - #158, dired Stage 1 #165, the GPU terminal input fix #166, Lean 4 - Stage 2 #161, the dired framing #164, COHERENCE.md #163, find-file - #162, Lean 4 Stage 1 #160, and the minimap blank-slab fix #159; - protocol v20). The previous snapshot named `d152120`; the recovery - check below accepts it or anything newer. + `githubsucks/main` @ `74301d1` (the dired Stage 1 landed-doc refresh + #169 atop Lean 4 Stage 4a #179, bottom-panel + Stage 2A #177, the bottom-panel Stage 2 framing #175, terminal + configuration Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a #167, the + CRDT undo repro #157, the inline-math landed-doc refresh #172, the + bottom-panel landed-doc refresh #156, the inline-math slice #158, + dired Stage 1 #165, the GPU terminal input fix #166, Lean 4 Stage 2 + #161, the dired framing #164, COHERENCE.md #163, find-file #162, Lean 4 + Stage 1 #160, and the minimap blank-slab fix #159; protocol v20). The + previous snapshot named `d152120`; the recovery check below accepts it + or anything newer. + **Lanes below that name an older base have not been re-based; derive + their integration surface from `git diff ..main`.** - On the transfer source, `origin/main` named a release mirror at `d3fa632` and lagged badly. On the current destination, `origin` names the canonical URL. This difference is why all recovery begins by @@ -54,9 +69,99 @@ git worktree list git status --short --branch ``` -The `git log` command must expose `d152120` or a newer intentional main. +The `git log` command must expose `c93f9ee` — the base named above — or a +newer intentional main. Keep this threshold and the canonical-base line in +step: a recovery check that accepts an older commit than the base it +declares canonical will pass on a tree the rest of this file does not +describe. If it does not, stop and repair the remote/fetch configuration. +## PTY terminate diagnostic lane — IN REVIEW (PR #176) + +- Portable branch: `githubsucks/pty-terminate-eperm`; worktree + `../pmacs-math-slice`. **PR #176**, base `main`, based on `ccf29e3` + with `c93f9ee` (#175) merged in. +- Approved framing: `docs/process-signal-tolerance-framing.md` + **revision 4**, after three review rounds. +- **Diagnostic only. No disposition change.** Every call that failed + before still fails, with no state transition and no reap-ledger + arming. `src/process.rs` is the only source file touched. +- **Why nothing is fixed:** revisions 1–3 each proposed a *tolerance* + rule and all three were rejected as unsound in the same way — each + concluded something about a process from something that was not about + that process. Rev 1 from an errno alone (EPERM means the caller lacks + permission, not that the id was recycled); rev 2 from `try_wait`, + which observes the spawned **leader** while a PTY signal targets + `-tcgetpgrp(...)`, entities that diverge exactly when job control has + moved the terminal; rev 3 from group-directed **ESRCH**, which proves + only that the selected foreground group vanished. +- **Two facts that killed the original argument.** `group = true` is + *rejected* for PTY mode at spawn (`src/process.rs:1428-1429`), so the + reap ledger never applies to the PTY path at all; and the ledger + comment (`:1075`) says EPERM "cannot happen for our own children" and + drops the entry for **bounded growth** — not a ruling that EPERM means + dead. +- **The CI evidence never established the child had exited.** The probe's + last source statement is a file write and CPython teardown does not + synchronise with it, so no tolerance rule could even be shown to fix + the symptom. That is the whole reason the lane is diagnostic. +- What ships: a failing `kill` now reports five separate facts — target + source, target kind/value, spawn-time group, errno, and the leader's + real `try_wait` state. The test seam injects the **kill result only**, + never the observation, so the real `ChildHandle::try_wait` runs against + the real child. +- **Not "strictly additive".** `try_wait` reaps and caches, so an exited + child may be reaped earlier than otherwise. Safe because + `portable-pty` 0.9.0 returns a `std::process::Child` on Unix and + delegates `try_wait` to it, so `poll_one` still sees the cached + status — pinned by an exactly-one-terminal-event test rather than + assumed. +- Round-1 review fixes: the exited-child tests no longer use a fixed + sleep as proof of exit (nix's `waitid` is unavailable on macOS and + `libc::waitid` needs `unsafe`, which the crate forbids), instead + driving the production diagnostic in a bounded loop until it observes + the exit; and every assertion is now exact message equality built from + the kernel-assigned pid, since the substring forms would have accepted + a hardcoded target or a wrong exit code. +- Bites, all verified rather than assumed: tolerating the failure fails + the disposition test; stubbing the leader observation fails three + tests including the one-event pin; a hardcoded target fails four; a + wrong exit code fails two. +- **The sweep found a real defect in these tests, not a flake.** + `observing_the_leader_does_not_consume_the_exit_event` failed with + "process ProcessId(26) is not running": the pid helper drained for + `Started`, and **`drain_until` ticks**. A tick can observe an + immediately-exiting child and move the record out of `Running`, after + which `signal` never reaches the diagnostic at all, so the bounded + loop spun to its limit. It passed standalone because the drain + returned on `Started` before `poll_one` saw the exit; only load lost + the race. Fast-exiting children now read the pid straight from the + supervisor record (no tick), and the loop fails fast if the record + left `Running`. **Verified under matched load: 0/15 with all 16 cores + saturated, while the old ticking helper fails 1/10 — the fix is + load-bearing.** +- Verification: fmt, `git diff --check`, strict workspace clippy clean; + lib 1,838 + CRDT 2,015 (both +6, exactly the new tests); GPU 202; M4 + 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; **isolated-config + `--no-fail-fast` sweep 3,258 across 93 suites, zero failures**. + Earlier sweeps on this branch showed two failures and then one; the + totals reconcile (3,256/2 → 3,257/1 → 3,258/0, same test count). The + two that were genuinely unrelated — + `read_dir_supersede_cancels_in_flight_predecessor` (known + pre-existing) and + `headless_snapshot_round_trip_summary_restores_the_minimap` — are + load-contention flakes; the second is structurally unreachable from + this diff, since `pmacs-gpu` depends on `pmacs-protocol` and never on + `pmacs`. +- **Parked, each with its reason:** all tolerance rules (need the + evidence this PR produces); `terminate` idempotence for an + already-reaped process (independent fix, different failure, one + feature per PR); and `signal_target`'s read-then-kill of `tcgetpgrp` + — still the most likely real fix site. +- **The lane closes when this merges.** It does not wait for the flake + to recur; the next occurrence carries its own evidence under whoever's + PR, and a Stage B framing follows then. + ## Lean 4 lane (Arc 8) — Stages 1–4a MERGED; Stage 4b IN REVIEW - **Stages 1, 2, 3a, 3b and 4a are MERGED** — #160 (`main` @ `0827dd1`), @@ -145,219 +250,291 @@ If it does not, stop and repair the remote/fetch configuration. fix it and also disables `dispatch_idle`, so RET would stop inserting a newline. -## Dired lane — Stage 0 MERGED; Stage 1 IN REVIEW (PR #165) +## The CRDT half of the test corpus is dark in CI — NEEDS A LANE -- Approved framing: `docs/dired-framing.md` **revision 6** — rev 5 is the - approved text (merged as its own docs PR #164), rev 6 adds §0's Stage 1 - implementation notes (S1-1…S1-9). Stages 2 (marks and operations) and 3 - (wdired) each get their own detailed framing after the prior stage lands. -- **Stage 0 (`C-x C-f` find-file) MERGED as #162** (`main` @ `2af1ab3`, - 2026-07-25, one review round, 12/12 CI green). Durable facts moved to - `docs/agent-handoff.md` §1 per rule 3 below. -- **Stage 1 branch: `githubsucks/dired-stage1`**, worktree - `../pmacs-dired-stage1`, based on `githubsucks/main` @ `8c86d34` (the - framing merge #164). **A fresh cut, not a rebase:** the older `dired` - branch (`ffdd642`, worktree `../pmacs-dired-arc`) was based on the - superseded `0827dd1` and carried only the framing content #164 already - put on `main`, so merging it would have reconciled two histories of one - document. It is left untouched and carries nothing unmerged. -- **Stage 1 implemented; no wire change (protocol stays v20).** What - landed on the branch: - - `builtin/runtime/dired.lua`: one buffer per directory named - `*dired:*` with the handle-table ownership check; - read-only intercept + `set_round_trip_input`; the `dired` major mode - and its mode-scoped keymap (`RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`); - basename cursor re-seating across every wholesale repaint; - `display_file` for file visits and same-window reuse for directory - descent; `C-x d` / `C-x C-j`; the `dired.kill-when-opening` setting. - Loaded after `window.lua`. - - `src/fs.rs`: `ReadDirTolerance`, `FsDirEntryError`, `FsDirListing`, - and one walk that either fails on a per-entry condition or records it - (Q#DR6). `src/async_runtime.rs` carries the listing in - `ReplyKind::ReadDir` / `JobResult::ReadDir`; `src/lua_bindings/mod.rs` - keys the Lua result **shape** on `errors.is_some()`, so the bare array - the frozen M8.2 fixture consumes with `ipairs` is untouched; - `builtin/runtime/fs.lua` validates read-op opts and **rejects unknown - keys** (a typo'd `tolerant` used to degrade silently to fatal). - - `src/editor_core.rs` + `src/lua_bindings/mod.rs`: - `normalize_buffer_path` is `pub` and exposed as - `pmacs.path.canonicalize` — Q#DR2's preferred end state, so no Lua - mirror exists and Stage 2 owes no mirror removal. This makes B2 - ("tolerant `read_dir` is the only Rust change") false by one small - binding, deliberately. - - `tests/dired_acceptance.rs`: 22 tests over framing items 1–16, - dispatch-driven; item 17 is the m8_1/m8_2/m8_3 additivity gate. -- **The framing claim the substrate falsified (S1-2):** R2-3 expected a - dedicated dired panel to carry its dedication across a descent. - `display_buffer` never replaces the buffer in a slot dedicated to - another one — it discards every side-specific parameter and falls back - to the document window (Q#BP3 2.iii), and the exact-window arm errors. - Dired does not unpin the user's panel; both arms are pinned. -- **The vacuity the bites found (S1-3):** acceptance 3c cannot pin the - descent *routing*. Dired holds focus in its own panel, so a raw - `switch_buffer` lands in the same window and every 3c assertion holds - either way. Dedication is the only discriminator, so the - dedicated-panel test is the real pin — and the vacuity is documented at - the assertion rather than relabelled. -- **The pre-existing test dired's first mode-scoped binding broke - (S1-4):** `describe_key_identifies_every_default_binding` asserted every - binding in the stack resolves through `describe.key` context-free, which - held only while the modes table was empty. It now sets the effective - context per binding and explicitly *clears* the mode for global ones, - because a leaked mode legitimately shadows a global chord of the same - name (dired's `RET` shadows `edit.newline-and-indent`). -- Durable substrate facts, independent of this arc: - - `pmacs.buffer.kill` (not `remove`) redirects windows off a doomed - buffer before removal, so `kill-when-opening` kills **after** the - replacement is displayed. - - Interactive origin does **not** survive an await: work resumed in - `tick_async` sees no `InteractiveCommandOrigin`, so `pmacs.window.*` - acts for the *ambient* active frontend (S1-9). - - Kinds are lstat-based in both `read_dir` and `stat`, so nothing in an - entry says whether a symlink points at a directory; `RET` probes by - trying to list it (S1-8). - - A path-backed buffer's *name* is its full path, not its basename — - worth knowing before writing any name assertion. - - `C-x d` takes **no** completion source on purpose (S1-5): with one, - RET on an empty field opens whatever sorts first, and - RET-on-where-you-are is the gesture the binding exists for. The field - is prefilled instead. -- **Bite verification:** 15 claims, each mutated in place and required to - fail the test that names it. `dired.lua` is new, so `scripts/bite`'s - file swap does not apply; every mutation was applied and reverted with - `git checkout --`. One came back VACUOUS and is recorded above. -- **Review round 1 addressed** (framing rev 7, S1-10…S1-12). Three - behavioral fixes, each bite-verified: `dired.revert`'s re-seat is - guarded on the active buffer (an ambient `move_to_line` after an await - moved an unrelated buffer's cursor — the buffer-level instance of - S1-9); `fmt_size` keeps the column width past ten digits, because - `_layout` is a contract Stage 3 is planned against; and the symlink - descent dropped its probe, since `open_directory`'s - changed-nothing-on-failure invariant *is* the probe (it was listing the - target directory twice). Plus a consecutive-`readdir`-error cap, because - **nothing cancels a dired listing** — it carries no supersede key, so - cancellation was never the backstop the tolerant loop implicitly relied - on. Naming/comment findings taken as-is. - - Durable process lesson, hit twice now: a mutation-bite helper restores - with `git checkout --`, which reverts to **HEAD** — so a fix must be - committed *before* it is bitten. Round 1's fixes were briefly wiped by - exactly that. -- **Canonical main integrated twice** — at `46a1b8f` (multi-root LSP - affinity #161) and again at `b889873` (GPU terminal input #166), both - merged rather than rebased per the #135/#137 precedent so the review - anchors stay addressable. Each conflict was a single doc hunk resolved - as the union: this lane owns COHERENCE's journey step 7 file half, #161 - owns the in-flight list, #166 owns step 8's GPU-terminal addendum. - Three things worth carrying: - - **A conflicting PR silently stops running CI.** GitHub builds - `pull_request` runs against the merge ref, which does not exist while - the PR conflicts, so no run is created and nothing reports a - failure — the checks list simply stays as it was. Three pushes to - this branch produced no CI at all before the cause was found. Watch - `mergeable` on a long-lived lane, not just the check list. - - #161's own COHERENCE finding **falsified a claim in this lane's - module doc**: `pmacs.error` is never defined in production, so an - uncaught raise inside a `pmacs.async` coroutine does not reach - `*errors*` as the comment said. It reaches a bare `error()` inside - `pmacs._async.tick()`, whose result `tick_async` discards with - `let _ =` — i.e. nowhere. That makes dired's per-coroutine `pcall` + - `set_status` load-bearing rather than tidy, and the comment now says - so. - - **A lane in review against a fast-moving `main` needs its gates rerun - per integration, not per push.** Main advanced twice inside this - review round, and the second time landed while the first - integration's sweep was still running. The numbers below describe the - twice-merged tree. -- Verification on the twice-merged tree (`main` @ `b889873`): - `cargo fmt --check` clean; strict workspace Clippy clean; **1,832 - default + 2,009 CRDT** library tests; dired acceptance **25 default + - 25 CRDT**; m8_1 10 / m8_2 15 / m8_3 32 unchanged; multi-root 13 and - vterm Stage 3 5 (both suites main added, green under this lane's - `mod.rs` and `editor.rs` changes); M4 121; required GPU 155; - **isolated-`XDG_CONFIG_HOME` workspace sweep 3,205 passed across 93 - suites, zero failures**; `git diff --check` clean. The sweep needs the - isolated config for the reason recorded in the bottom-panel lane - below. -- Coherence (framing §0.5, required since #163): serves `COHERENCE.md` §20 - Priority 1, which names this work explicitly; journey step 7's file half - goes from no surface to a surface; **adds no interaction island** — keys - are a mode-scoped keymap, and wdired will be a mode swap; adopts - `pmacs.config` for `dired.kill-when-opening`; inherits §9's - worker-attribution gap for its `read_dir` jobs without worsening it. The - audited claims this changes are updated in `COHERENCE.md` itself, per its - §25. -- **Boundary with the Journey Stage 1 arc** (`COHERENCE.md` §20 arc-cut - 1): CLI directory-argument handling (`pmacs .` exits 1) belongs there, - not here — Stage 1 does **not** fix it. The two meet at - `resolve_target_buffer`; dired supplies the buffer a directory should - resolve *to*, and `pmacs .` should route into it rather than growing a - second directory surface. +- **No branch, no framing yet.** Found while gating #166, then measured + properly during the vterm as-framed audit. Deliberately kept out of #166 so + a CI change would not arrive after review approval. +- **Root cause:** `.github/workflows/ci.yml` never enables the `crdt` feature + anywhere — zero hits across the workflow directory. The `test` job runs + `cargo test --all-targets --no-default-features --features luajit|lua54`. + Every `#[cfg(feature = "crdt")]` test is therefore **not compiled** in CI, + not merely skipped. +- **Measured, `--list` under CI's exact flags versus the same flags plus + `crdt`: 3,024 vs 3,288 — 264 tests dark.** Per target: -## GPU terminal input lane — IN REVIEW + | dark | CI | full | target | + |---:|---:|---:|---| + | 177 | 1,832 | 2,009 | **the library itself** (`src/lib.rs`) | + | 21 | 15 | 36 | `m5_5_acceptance` | + | 13 | 1 | 14 | `gpu_invocation_acceptance` | + | 13 | 1 | 14 | `gpu_initial_target_acceptance` | + | 8 | 0 | 8 | `m10_11_acceptance` | + | 6 | 0 | 6 | `auto_pair_crdt_acceptance` | + | 6 | 0 | 6 | `m10_2_perf` | + | 4 | 5 | 9 | `vterm_stage3_acceptance` | + | 4 | 0 | 4 | `m10_10_perf` | + | 3 | 0 | 3 | `compile_mode_crdt_acceptance` | + | 2 | 22 | 24 | `theme_faces_acceptance` | + | 2 | 0 | 2 | `m11_5_semantic_acceptance` | + | 1 | 9 | 10 | `vterm_stage1_acceptance` | + | 1 | 7 | 8 | `statusline_segments_acceptance` | + | 1 | 10 | 11 | `gpu_font_acceptance` | + | 1 | 0 | 1 | `auto_indent_crdt_acceptance` | + | 1 | 0 | 1 | `m10_11_perf` | -- Portable branch: `githubsucks/gpu-terminal-input`, worktree - `../pmacs-gui-term-input`, based on `githubsucks/main` @ `46a1b8f`. -- Approved framing: `docs/gpu-terminal-input-framing.md` revision 2, - committed as the branch's first commit (`9a0df21`). Bug fix, not a - feature; **no protocol change (stays v20)**. -- Reported as "text input within the terminal doesn't work on GUI, this is - fine in TUI". Root cause: the dispatcher applied **both** terminal-layout - syncs to **every** attached frontend, and a semantic session satisfies both - conditions (a `term_sizes` entry from `AttachRequest` *and* a terminal - declaration). Its PTY was resized twice per tick forever — grid arm installs - the TUI placement size, semantic arm installs the declared content - rectangle, each arm's idempotence guard seeing only what the other just - wrote — so the child took a `SIGWINCH` storm at tick cadence. -- **The fix is a split, not a guard.** The grid arm is also the only per-tick - controller-liveness release a semantic frontend gets, and - `sync_semantic_terminal_layout` cannot take that over: the buffer-follow - snapshot clears the viewport declaration (`on_buffer_snapshot_sent`), so - that arm stops running in exactly the switch-away case that needs the - release. `sync_terminal_layout` is therefore split into a - frontend-kind-neutral half (panel reconcile + liveness) and a grid-only - geometry half, with the loop body extracted to - `sync_terminal_layouts_for_tick` so the exclusivity is structural and tests - drive the real thing. -- **Trap for anyone touching this again:** the release at the "no - `window_placements` entry" arm reads like liveness and is grid geometry. A - semantic frontend has no placement entry at all, so moving it into the - neutral half releases a GPU controller every tick. -- Bite-verified against **two** pre-images, because the naive guard fixes the - storm and introduces the leak: +- **The single worst line is the library.** `cargo test --lib --features crdt` + is a REQUIRED local gate in `CLAUDE.md`, and CI has never run it. 177 + library tests — the whole CRDT half — are developer-machine-only. +- **Ten suites run zero or one test in CI**, including `gpu_initial_target` + (#148's entire acceptance, 1/14), `gpu_invocation` (#141's, 1/14), and + `a37`, the Vterm Stage 3 real-daemon/real-PTY/real-wgpu path that #135 + built specifically because "a decoded-message fixture would prove none of + the three fit together". +- **⚠ `a37` will report green in the new job without running, unless the + job builds `pmacs-gpu` AND sets `PMACS_REQUIRE_GPU=1`.** Measured + 2026-07-26 while gating #173. `a37_real_daemon_real_pty_and_headless_gpu_ + render_one_terminal_session` derives its sibling binary path from + `CARGO_BIN_EXE_pmacs`, and on a missing binary it `eprintln!`s a skip and + **returns `ok`**. A fresh worktree running + `cargo test --features crdt --test vterm_stage3_acceptance` reports **9/9 + in 0.17 s having never run it**; a real run takes ~4 s. Only + `PMACS_REQUIRE_GPU=1` promotes that skip to a failure, and `CLAUDE.md` + applies that flag to `cargo test -p pmacs-gpu` — a **different package**, + so the required local gate does not cover a37 either. The `gpu-render` + job already sets the flag, which is what makes fix-shape part 2 sound; + state it as a **requirement** of that job rather than inheriting it by + luck, because a `crdt` leg added to the plain `test` job would run a37 + vacuously. +- **`a37` is also load-sensitive, which changes how to read the expected + first-run failures.** It passed at `d152120` and failed at that *same + commit* twenty minutes later, with a second agent saturating the machine + with `rustc` in between; it then failed identically on `d152120`, + `04c5ad1`, and the #173 merge commit, which is how #173 established the + failure was not its own. The signature is `last_frame_text` all spaces + with `rendered_nonuniform_frames` nonzero — frames arrive, content does + not. `pmacs-gpu`'s own suite flaked the same way under the same load + (201/202, then 202/202 on immediate rerun). **So a red a37 on the first + CI run is ambiguous by construction**: before treating it as a real + failure, run the same command on the merge base, and prefer serialized + execution for this suite over retry-until-green. +- **Sort deliberate from accidental before proposing a fix.** Some of the 264 + are perf suites that are `#[ignore]`d by default and belong to their own + jobs (`m10_2_perf` 6, `m10_11_perf` 1). `m10_10_perf` has **no** `#[ignore]` + and no CI job naming it, so it looks accidental. This classification is not + finished and is the lane's first task. +- **Fix shape, two parts** (the flag combination is verified to work: + `--no-default-features --features luajit,crdt` lists 10 vterm Stage 1 tests + versus 9 without): + 1. a `crdt` leg on the `test` job for the non-GPU suites and the library; + 2. the GPU-requiring `crdt` suites onto the existing `gpu-render` job, which + already has lavapipe and `PMACS_REQUIRE_GPU=1` — + `vterm_stage3_acceptance`, `gpu_invocation_acceptance`, + `gpu_initial_target_acceptance`, `gpu_font_acceptance`. +- **Expect first-run failures, and budget for them.** These would execute in + CI for the first time ever: real PTY timing on CI runners, wgpu under + lavapipe, and daemon-socket tests at unfamiliar concurrency. Start + ubuntu-only and decide about macOS from evidence. A red first run is the + lane working, not the lane failing. +- Mitigating fact, verified rather than assumed: #166's three unit pins are + **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. - | pin | `main` | naive guard | the split | - |---|---|---|---| - | settle (acc 2+3) | FAIL | pass | pass | - | controller release (acc 6) | pass | FAIL | pass | - | grid still resizes (acc 5) | pass | pass | pass | - -- Real-path evidence: a quiet child trapping `SIGWINCH` reports **144 frames - in 4 s and `WINCH 1..12` on screen** against the pre-fix tree, versus a - settled screen with the fix. -- **Deliberately out of scope, named:** interactive-shell echo on a raw-mode - PTY (Q#GT5 — reproduces in-process too, so it is not the GUI/TUI - asymmetry), and a geometry change appearing to clear the visible screen - (reproduces pre-fix; why acceptance 4 latches its observation across - frames). -- Verification on this branch: `cargo fmt --check` clean; strict workspace - Clippy clean; 1,829 default + 2,006 CRDT library tests; vterm Stage 1/2/3 - 10 / 6 / 9 CRDT; bottom-panel Stage 1 46; M4 121; required GPU 155; - **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; - `git diff --check` clean. Gates were run against the committed tree. - -## Terminal config + copy mode arc — Stage 1 IN REVIEW +## Terminal config + copy mode arc — Stage 1 MERGED; Stage 2 IN REVIEW - Approved framing: `docs/terminal-config-and-copy-mode-framing.md` **revision 4** (four review rounds), committed as the first commit of Stage 1's branch. Two stages, two branches, two PRs; **no protocol change**. -- **Stage 1 = `githubsucks/terminal-config`**, worktree - `../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120` - and merged up to `c93f9ee` during review round 1. Profiles, - scrollback, escape key, and the `C-c t` opening binding. -- **Stage 2 = `terminal-copy-mode`, not started.** Branch it off `main` - after Stage 1 merges: no dependency, but both edit - `builtin/runtime/terminal.lua`. +- **Stage 1 MERGED as #173** (`main` @ `cf54270`, 2026-07-26, one review + round, all twelve checks green). Branch `githubsucks/terminal-config` + and worktree `../pmacs-terminal-config` retained. Profiles, scrollback, + a per-terminal configurable escape key, and the `C-c t` opening + 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 = `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`; @@ -439,22 +616,34 @@ If it does not, stop and repair the remote/fetch configuration. - `pmacs-gpu` itself failed 201/202 once under the same load and passed 202/202 on immediate rerun. -## Bottom-panel lane (Arc 7) — Stage 1 + framing MERGED; Stage 2A IN REVIEW +## Bottom-panel lane (Arc 7) — Stages 1, 2A + framing MERGED; 2B is next -Stage 1 and the Stage 2 framing are on `main`. **Stage 2A is -implemented and in review.** +Stage 1, the Stage 2 framing, and Stage 2A are all on `main`. **Stage 2B +has not started.** -- **Stage 2A — portable branch `githubsucks/bottom-panel-stage2a`**, - worktree `../pmacs-bp-stage2a`, **canonical `main` @ `cf54270` - integrated** (review round 1, finding 4 — the terminal-config lane - #173 also changes `src/editor.rs`, so gates were rerun on the merge - result, not the old combination). Five commits: the classified census - routing, the painter extraction + acceptance, the lane record, then - the round-1, round-2 and round-3 review fixes. **No protocol change; no behavior - change for any frontend today** — with `panel_capable = false` for - semantic sessions, `primary_document_window` returns `view.active` - in every existing configuration, so this is seam adoption that - becomes load-bearing in 2B. +- **Stage 2A MERGED as #177** (`main` @ `0a3fcd1`, 2026-07-26, all twelve + checks green at `8424172`, three review rounds). Branch + `githubsucks/bottom-panel-stage2a` and worktree `../pmacs-bp-stage2a` + are retained and carry nothing unmerged. Five commits: the classified + census routing, the painter extraction + acceptance, the lane record, + then the round-1, round-2 and round-3 review fixes. **No protocol + change; no behavior change for any frontend today** — with + `panel_capable = false` for semantic sessions, + `primary_document_window` returns `view.active` in every existing + configuration, so this is seam adoption that becomes load-bearing in + 2B. +- **Stage 2B is approved and unstarted.** It branches from `main`, **not + stacked on 2A**, per the framing §9. Scope: protocol v21, the daemon + panel projection, the GPU band, and the negotiated `panel_capable` + flip. `docs/bottom-panel-stage2-framing.md` §7.2 carries its five + acceptance criteria (A2B-1..5) plus the reassertion of parent + criterion 52, and §8 records **no open items**, so 2B needs no further + framing round. Its sharpest trap is §5.3's three-boundary split: the + GPU `text_area_bottom` is `status_band_top`, + `geometry_capacity_bottom` and `document_text_bottom` at once, and a + blanket rewrite moves the status chrome along with the document while + still satisfying an "everything moved" assertion — hence A2B-4's + contrast form. - Verification on the merge result: `cargo fmt --check` clean; strict workspace Clippy clean; **1,832 default + 2,015 CRDT** library tests; `bottom_panel_stage2a_acceptance` **17**; bottom-panel Stage 1 46; @@ -613,6 +802,55 @@ git worktree add --track \ ## Closed since the last snapshot +- **Dired Stage 1 (the directory view) — MERGED as #165** (`main` @ + `c8ec8f3`, 2026-07-25, after one review round). pmacs has a directory + surface: `C-x d` / `C-x C-j`, one read-only buffer per directory named + `*dired:*`, a `dired` major mode carrying + `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. No wire change (v20). The Rust is + two things — a per-entry-tolerant `read_dir` (Q#DR6), which had to be + Rust because `read_dir_blocking` fails a whole listing on any of five + per-entry conditions and a tolerant wrapper cannot be written in Lua at + all, and `normalize_buffer_path` going `pub` as + `pmacs.path.canonicalize` (Q#DR2's preferred end state, so no Lua mirror + exists and Stage 2 owes no mirror removal). The frozen m8_1/m8_2/m8_3 + counts are unchanged, which is the additivity gate. 15 claims + bite-verified; one came back VACUOUS (acceptance 3c cannot pin descent + routing — dired holds focus in its own panel, so dedication is the only + discriminator) and is documented at the assertion rather than + relabelled. Its branch (`dired-stage1`) and worktree + (`../pmacs-dired-stage1`) are done; the abandoned `dired` branch + (`ffdd642`, `../pmacs-dired-arc`) was superseded by a fresh cut and + carries nothing unmerged. **Stage 2 (marks and operations) and Stage 3 + (wdired) each still need their own framing**, and the frozen fixture + shrinks after Stage 3. Durable substrate facts and both new ops lessons + live in `docs/agent-handoff.md` §§1/5; the implementation notes are + `docs/dired-framing.md` §0, S1-1…S1-12. Two named forward items for + Stage 2: `apply_resource_op`'s rename rebind is exact-PathBuf-equality, + first-match-only, looked up with the raw path while stored paths are + normalized — so a directory rename strands every buffer under it, and + `pmacs.fs.rename` has zero production callers, so it can be fixed at + the primitive; and Q#DR5's seam is the main-thread drain + `AsyncRuntime::tick`, not `_take_result`, where rename settles as an + undifferentiated `ReplyKind::FsUnit` and so must be keyed on + `JobKind::FsRename`. + +- **GPU terminal input (the double terminal-layout sync) — MERGED as #166** + (`main` @ `b889873`, 2026-07-25, one review round, all twelve checks green + after a macOS PTY-timing rerun). The dispatcher applied **both** + terminal-layout syncs to **every** attached frontend; a semantic session + satisfies both conditions, so its PTY was resized twice per tick forever and + the child took a `SIGWINCH` storm that made a GPU terminal untypable while + output still flowed. `sync_terminal_layout` is now split into a + frontend-kind-neutral half (panel reconcile + controller liveness) and a + grid-only geometry half, with the loop body extracted to + `sync_terminal_layouts_for_tick` so the exclusivity is structural. No + protocol change (v20). Durable lessons are in `docs/agent-handoff.md` §5; + the framing (`docs/gpu-terminal-input-framing.md` rev 2) carries three + falsified hypotheses, the two-pre-image bite matrix, and two named + out-of-scope items (Q#GT5 interactive-shell echo on a raw PTY, which + reproduces in-process and so is not the GUI/TUI asymmetry; and a geometry + change appearing to clear the visible screen, which reproduces pre-fix). + Branch `gpu-terminal-input` and worktree `../pmacs-gui-term-input` retained. - **Inline-math slice — MERGED as #158** (`main` @ `5aa9044`, 2026-07-25). Detect → parse → layout → draw for `$…$`, entirely inside `pmacs-gpu`, no protocol change. Verified by the user's manual pass on diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 23108f4..32b1a10 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1,11 +1,18 @@ # Agent handoff — cross-machine continuity -**Last updated: 2026-07-26, after Lean 4 Stage 4a (#179) landed — the -typed-edit consumer chain, the substrate the Unicode input method -registers on — atop stages 3a and 3b (#167, #170), pmacs' first Lean -language server, and following the inline-math -slice (#158), the first mathematical typesetting in pmacs, and find-file (#162), -the dired arc's Stage 0, and COHERENCE.md (#163), Lean 4 Stage 1 (#160), the +**Last updated: 2026-07-26, after Lean 4 Stage 4a (#179) — the typed-edit +consumer chain — and bottom-panel Stage 2A (#177), the classified census +routing that makes every Projection-class consumer ask +`primary_document_window`; following the bottom-panel Stage 2 framing +(#175), terminal configuration Stage 1 (#173) — profiles, scrollback, a +per-terminal configurable escape key, and the `C-c t` opening binding — +Lean 4 stages 3a and 3b (#167, #170), pmacs' first Lean language server; +the GPU terminal input fix (#166), the double terminal-layout sync that +made a GPU terminal untypable; the CRDT undo repro (#157), the +inline-math landed-doc refresh (#172), the inline-math slice (#158), the +first mathematical typesetting in pmacs; dired Stage 1 (#165), Lean 4 +Stage 2 (#161), the dired framing pair (#163/#164), find-file (#162) — +the dired arc's Stage 0 — COHERENCE.md (#163), Lean 4 Stage 1 (#160), the minimap blank-slab fix (#159), bottom-panel Stage 1 (#155), the inline-math re-scout (#154), the vterm PTY-flake fix (#153), and the GPU initial-target doc refresh (#152); and before that GPU @@ -30,13 +37,18 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-07-26) -- `main` @ `d400f30` (Lean 4 Stage 3b #170 atop Stage 3a #167, the - bottom-panel landed-doc refresh #156, the inline-math slice #158, - dired Stage 1 #165, the GPU terminal input fix #166, Lean 4 Stage 2 - #161, the dired framing #164, COHERENCE.md #163, find-file #162, Lean - 4 Stage 1 #160, minimap blank-slab #159, bottom-panel Stage 1 #155). - Protocol unchanged at **v20**. The bullets below describe the arcs in - their own terms; this line is the head-of-`main` anchor. +- `main` @ `74301d1` (the dired Stage 1 landed-doc refresh #169 atop + Lean 4 Stage 4a #179, bottom-panel Stage 2A + #177, the bottom-panel Stage 2 framing #175, terminal configuration + Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a #167, the CRDT undo repro + #157, the inline-math landed-doc refresh #172, the bottom-panel + landed-doc refresh #156, the inline-math slice #158, dired Stage 1 + #165, the GPU terminal input fix #166, Lean 4 Stage 2 #161, the dired + framing #164, COHERENCE.md #163, find-file #162, Lean 4 Stage 1 #160, + minimap blank-slab #159, bottom-panel Stage 1 #155). Protocol unchanged + at **v20** — bottom-panel Stage 2A deliberately carries no wire change; + v21 arrives with Stage 2B. The bullets below describe the arcs in their + own terms; this line is the head-of-`main` anchor. - **`COHERENCE.md` is now required reading and a required framing input — #163.** It carries the product-coherence thesis, an audited scorecard, per-concern gaps, and §20's priority order, and it is the @@ -194,11 +206,78 @@ commands, read `docs/active-work.md` immediately after this file. against an open buffer yet fails to load one that is not open — find-file expands the tilde Lua-side. Loading through the normalized path is a named deferral. - - **Stage 1 (the directory view) is IN REVIEW as PR #165** — the - builtin `dired.lua`, the per-entry-tolerant `read_dir` opt, and - `pmacs.path.canonicalize`. Its branch state, substrate facts, and - verification live in `docs/active-work.md`; this section absorbs them - when it merges. +- **dired Stage 1 — the directory view — LANDED — #165** + (`docs/dired-framing.md` §0, S1-1…S1-12; merge `c8ec8f3`; one review + round). pmacs now has a directory surface: `C-x d` / `C-x C-j` open a + read-only listing, one buffer per directory named + `*dired:*`, with a `dired` major mode whose + mode-scoped keymap carries `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. + Protocol unchanged at **v20**. **Stage 2 (marks and operations) and + Stage 3 (wdired) each still need their own framing**; the frozen + fixture shrinks after Stage 3. + - **The Rust is confined to two things**: a per-entry-tolerant + `read_dir` (`ReadDirTolerance {Fatal, PerEntry}` → + `FsDirListing {entries, errors}`), because `read_dir_blocking` fails + a whole listing on any of five per-entry conditions and the tolerant + wrapper its own module doc delegates to package authors **cannot be + written in Lua** (one error value, no partial vec); and + `editor_core::normalize_buffer_path` becoming `pub`, exposed as + `pmacs.path.canonicalize`. Only non-UTF-8 **names** stay fatal — + byte-preserving paths would be needed. The Lua result **shape** keys + on `errors.is_some()`, so the bare array the frozen M8.2 fixture + consumes with `ipairs` is untouched. + - **Exposing a core normalizer beat mirroring it in Lua.** A Lua mirror + would have been a second canonical form — the same class of bug as + the five tab-width constants (#137). Applies to any future Lua-side + path reckoning. + - **A fixed-width column must be fixed-width for every input.** The + exported `pmacs.dired._layout` (MARK 0, KIND 2, PERMS 3–12, SIZE 13, + MTIME 24, NAME 41) is the contract Stage 3 reads offsets from, and + `%10d` overflows at ≥10 GB, silently shifting every column right of + it. Sizes now fall back to a width-clamped magnitude (K/M/G/T/P/E). + - **An ambient action must be gated on the buffer it assumes.** A + revert's cursor re-seat settles a tick or more later, by which time + the user may have switched buffers; the paint names its buffer and is + safe, but seating is ambient. This is the buffer-level instance of + the rule below that interactive origin does not survive an await. + - **A failure IS an answer — don't probe first.** Kinds are lstat-based + in both `read_dir` and `stat`, so nothing in an entry says whether a + symlink points at a directory. `RET` tries to list it and treats the + failure as the answer; an explicit probe was a second full + `read_dir`, so a descent listed twice. + - **Unbounded per-entry error collection needs a cap when nothing + cancels the work.** A dired listing carries no supersede key, so + cancellation was never the backstop the tolerant loop implicitly + relied on (`READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS = 1024`). + - **This is the first builtin with mode-scoped keys** (#129's first + non-detection consumer), which broke the pre-existing + `describe_key_identifies_every_default_binding`: it asserted every + binding resolves through `describe.key` context-free, which held only + while the modes table was empty. It now sets the effective context + per binding and explicitly **clears** the mode for global ones, + because a leaked mode legitimately shadows a global chord of the same + name (dired's `RET` shadows `edit.newline-and-indent`), plus a floor + assertion that at least one mode-scoped binding exists. + - **A dedicated panel does not carry its dedication across a descent** + — the framing expected it to. `display_buffer` never replaces the + buffer in a slot dedicated to another one; it discards every + side-specific parameter and falls back to the document window (Q#BP3 + 2.iii), and the exact-window arm errors. Dired does not unpin the + user's panel; both arms are pinned. + - Smaller facts worth knowing before touching this code: a path-backed + buffer's **name is its full path**, not its basename, which matters + for any name assertion; `pmacs.buffer.kill` (not `remove`) redirects + windows off a doomed buffer first, so `dired.kill-when-opening` kills + **after** the replacement is displayed; ownership is checked against + the handle table only, never the buffer name; and `C-x d` takes **no** + completion source on purpose (with one, `RET` on an empty field opens + whatever sorts first, and RET-where-you-are is the gesture the binding + exists for — the field is prefilled instead). + - Verification at merge: 1,832 default + 2,009 CRDT library tests; + dired acceptance 25 + 25 CRDT; the frozen m8_1 10 / m8_2 15 / m8_3 32 + unchanged, which is the additivity gate for the `read_dir` change; M4 + 121; required GPU 155; isolated-`XDG_CONFIG_HOME` workspace sweep + 3,205 across 93 suites. 15 claims bite-verified. - Protocol **v20** (`SUPPORTED=[6..=20]`; v16 = `ThemeFacts`, v17 = `FontFacts`, v18 = `StatuslineSegments`, v19 = terminal frames/events, v20 = the GPU initial-target semantic bootstrap family). @@ -786,6 +865,44 @@ commands, read `docs/active-work.md` immediately after this file. DAP, 8 GPU splits, plus the `.ipynb` arc (its JSON-grammar prerequisite shipped in #123). +- **GPU terminal input LANDED — #166** (`main` @ `b889873`; + `docs/gpu-terminal-input-framing.md` rev 2; one review round). The + dispatcher applied **both** terminal-layout syncs to **every** attached + frontend each tick. A semantic session satisfies both conditions — a + `term_sizes` entry from `AttachRequest` *and* a terminal declaration — so + its PTY was resized twice per tick forever: the grid arm installed the TUI + placement size, the semantic arm the declared content rectangle, each arm's + `old_size == size` guard seeing only what the other had just written. The + child took a `SIGWINCH` storm at tick cadence, which made typing into a GPU + terminal impossible while output kept flowing. TUI was structurally + unaffected. + - `EditorInstance::sync_terminal_layout` is split into + `sync_terminal_controller_liveness` (frontend-kind **neutral**: panel + reconcile + release of a controller whose window moved away — reads only + views/windows/controller, never a grid size) and + `sync_terminal_grid_geometry` (**grid only**: TUI placement + resize). + `sync_terminal_layout` survives as the composition, so `editor::run` and + `LOCAL` are byte-identical. + - `daemon::sync_terminal_layouts_for_tick` is the extracted loop body: + liveness for every frontend once per tick, then **exactly one** geometry + arm keyed on `semantic_states` membership — the same fact session + establishment uses, so the arms cannot both fire. + - **The trap, kept in a comment:** the release on a missing + `window_placements` entry reads like liveness and is grid geometry. A + semantic frontend has no placement entry at all, so moving it into the + neutral half would release a GPU controller every tick. + - Why not the one-line guard: the grid arm was also the **only** per-tick + controller-liveness release a semantic frontend got, and + `sync_semantic_terminal_layout` cannot take it over — the buffer-follow + snapshot clears the viewport declaration, so that arm stops running in + exactly the switch-away case that needs the release. + - No protocol change (v20). Gates: 1,829 default + 2,006 CRDT library + tests; vterm Stage 1/2/3 10/6/9 CRDT; bottom-panel 46; M4 121; required + GPU 155; isolated-config workspace sweep 3,177 across 92 suites. + - **Known gap, its own lane:** CI never enables `crdt`, so the Stage 3 + real-path acceptance (including `a37`) is not compiled there. #166's unit + pins are not `crdt`-gated and do run. See `docs/active-work.md`. + ## 2. How we work (the part that must not drift) The user is expert and reviews deeply — they falsify framings and find @@ -912,6 +1029,34 @@ final variant — its own round-trip cannot detect a discriminant shift. ## 5. Hard-won ops lessons +- **A test that skips on a missing precondition reports `ok`, and a gate log + cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the + only acceptance driving a real daemon, a real PTY and a real wgpu render + together — derives `pmacs-gpu` from `CARGO_BIN_EXE_pmacs` and, when that + binary is absent from the target directory, prints a skip and returns. + A fresh worktree reports the suite 9/9 **in 0.17 s having never run it**; + a real run takes ~4 s. `PMACS_REQUIRE_GPU=1` is what promotes the skip to + a failure, and the standing gate list applies that flag to + `cargo test -p pmacs-gpu`, a *different package*. Two habits follow: + build the workspace before believing any suite that reaches for a sibling + binary, and **judge such a suite by its elapsed time**, not its verdict. +- **Before attributing a red test to your branch, run it on the merge base.** + `a37` failed on the #173 branch, which looked like a regression; it failed + identically on the PR's own base and on two intermediate commits, and had + *passed* on that same base twenty minutes earlier. The variable was machine + load from a second agent compiling continuously. Load-sensitive tests make + both verdicts uninformative in isolation, so the base-commit run is the + cheapest way to tell a regression from weather — and it is much cheaper + than the bisect it replaces. +- **A daemon-side fix is not deployed until the daemon is restarted from a + tree that contains it.** #166's reporter rebuilt and saw no change: the + running daemon had been started from a shared checkout still on a pre-fix + branch, and `pmacs --gpu` attaches to whatever process already owns the + socket. Rebuilding a binary does nothing to a running process. When + validating a daemon-side fix by hand, check the running process's binary + path and start time against the tree you think you fixed — + `ps -eo pid,lstart,args | grep '[p]macs --daemon'` — before concluding the + fix failed. - **Two operations that must be alternatives are not made alternatives by being adjacent.** The dispatcher applied its grid and semantic terminal-layout syncs to every attached frontend; a semantic session @@ -963,6 +1108,24 @@ final variant — its own round-trip cannot detect a discriminant shift. trap-guarded one-file swap over read-only `git show`, with an inverted verdict (exit 0 iff the tests FAIL against the old version), making bite-verification machine-checkable. +- **A fix must be COMMITTED before it is bitten.** `scripts/bite` + restores by `git checkout --`, which reverts the file to **HEAD**, not + to the state it found — so any uncommitted work in a bitten file is + destroyed. A whole review round's fixes were wiped this way during + #165. Corollary for a NEW file: the swap-over-`git show` mode does not + apply at all, so its claims must be bitten by hand-editing, which makes + the commit-first rule load-bearing rather than hygienic. +- **A CONFLICTING PR silently runs no CI at all.** GitHub builds + `pull_request` workflow runs against the PR's **merge ref**, which it + does not create while the branch conflicts with its base. So pushes + land, the branch updates, no run is ever queued, and **nothing reports + the absence** — the checks list simply keeps showing the last + successful run, which reads as current. Three pushes to #165 produced + zero CI before the cause was found, and `gh pr checks` returns nothing + usable here. On any lane that lives through a moving `main`, check + `gh pr view --json mergeable,mergeStateStatus,headRefOid` and + confirm a run exists **for the current head sha**, not merely that a + recent run was green. - **Stacked PRs**: retarget the child to main BEFORE merging the parent — GitHub auto-closes a PR whose base branch is deleted and cannot reopen it (#104 → re-opened as #105). diff --git a/docs/dired-framing.md b/docs/dired-framing.md index ada853e..83e6f1a 100644 --- a/docs/dired-framing.md +++ b/docs/dired-framing.md @@ -1,7 +1,9 @@ # Dired — framing **Revision 7 — 2026-07-25. Status: APPROVED; Stage 0 MERGED as #162; -Stage 1 IN REVIEW as PR #165, review round 1 addressed.** +Stage 1 MERGED as #165 (`main` @ `c8ec8f3`, one review round). Stage 2 +(marks and operations) and Stage 3 (wdired) each still need their own +framing before implementation; the frozen fixture shrinks after Stage 3.** Rev 1 passed a ground-truth review; rev 2 fixed round 1's seven findings; rev 3 fixed round 2's six and was approved; rev 4 recorded what Stage 0's implementation falsified in the approved text (§0); rev 5 adds the diff --git a/docs/gpu-terminal-input-framing.md b/docs/gpu-terminal-input-framing.md index 0bbccef..aa1bea4 100644 --- a/docs/gpu-terminal-input-framing.md +++ b/docs/gpu-terminal-input-framing.md @@ -81,7 +81,11 @@ character: | | frames for a static screen | typed `Z` ever visible at the prompt | |---|---|---| | `main` today | **730** in a 20 s window | **no** | -| with the guard | **2** | (see Q#GT5 — a separate question) | +| with the fix | **2** | yes | + +Bet B2 is **scored TRUE**: with the fix deployed, the reporter confirmed +typing into a GPU terminal works. The earlier caveat here pointed at Q#GT5, +which is now retracted — see "Deferred (named)". The TUI is unaffected: a grid session has no semantic terminal declaration, so only one arm ever runs for it. This is a **frontend-kind** defect, which is @@ -281,11 +285,12 @@ change. Stays v20. snapshot that signals the switch-away). Hence the split in Q#GT1. Recorded rather than deleted: the failure mode is one a reviewer or a future simplification will re-propose. -- **B2.** The user's reported symptom is this defect. *Partially scored: the - storm is proven and GUI-only, and its shape (line editor unusable, output - still flowing) matches the report. Not fully scored until the user, or an - acceptance running the **user's own shell**, confirms typing works after the - fix. Q#GT5 is the reason this bet is stated rather than assumed.* +- **B2 — SCORED TRUE 2026-07-25.** "The user's reported symptom is this + defect." Confirmed in real use after the fix was deployed: typing into a GPU + terminal works. The confirmation needed a daemon **restart** built from a + tree containing the fix — the first attempt reported no change because a + pre-fix daemon still owned the socket, which is worth remembering whenever a + daemon-side fix is being validated by hand. - **B3.** No other pair of per-frontend-kind daemon operations is applied as siblings rather than alternatives. *Scored by an explicit audit of the dispatcher's per-frontend loop during implementation — this defect's shape @@ -294,7 +299,14 @@ change. Stays v20. ## Deferred (named) -- Interactive-shell echo on a raw-mode PTY (Q#GT5) — its own scout. +- ~~Interactive-shell echo on a raw-mode PTY (Q#GT5)~~ — **RETRACTED + 2026-07-25.** The observation behind it (a `bash --norc -i` fixture not + echoing typed characters) does not reproduce in real use: with the fix + deployed, typing into a GPU terminal echoes normally. The fixture was almost + certainly measuring its own timing — polling a published screen snapshot + before readline had finished initialising — not a product behaviour. Recorded + as retracted rather than deleted so nobody re-derives it from the framing's + earlier revision and spends a scout on it. - **A geometry change appears to clear the visible screen.** Observed while building acceptance 4: after the probe's deliberate 25×92 → 20×71 resize, the next frame's visible grid is entirely blank even though the content diff --git a/docs/process-signal-tolerance-framing.md b/docs/process-signal-tolerance-framing.md new file mode 100644 index 0000000..1d3c45d --- /dev/null +++ b/docs/process-signal-tolerance-framing.md @@ -0,0 +1,258 @@ +# Framing — make the PTY terminate failure self-describing (diagnostic only) + +**Revision 4.** Status: awaiting review round 4. Lane: +`pty-terminate-eperm`, worktree `../pmacs-math-slice`, based on +`githubsucks/main` @ `ccf29e3`. + +**Diagnostic only. No disposition changes, no tolerance rules, no +behavioural fix.** Every rule this document proposed across revisions 1 +to 3 is parked (§5). The lane's entire deliverable is that the next +occurrence of the failure explains itself. + +## Revision history + +**Revision 3 → 4**, after review round 3 (two blocking, one major) and +its scope call. All accepted. + +- **Group-directed ESRCH was also unsafe**, for the same reason EPERM + was: it proves the selected *foreground group* vanished, not that the + leader exited. A job-control race — foreground job exits after + `tcgetpgrp` and before `kill`, shell alive and not yet reclaiming the + terminal — would have been reported as success with the leader never + signalled. Rev 3's acceptance 7 pinned that unsafe behaviour. **All + tolerance is parked** (§5). +- **Rev 3's Stage A implemented Stage B.** It declared itself + diagnostic-only, then listed tolerance and bookkeeping acceptances. + Removed. +- **Q#PS6 (already-reaped `terminate` is `Ok`) is parked separately.** + It is an independent behavioural fix answering a different failure; + under one-feature/one-PR it does not ride with instrumentation. +- **"Strictly additive / cannot regress behaviour" was overstated** and + is narrowed (Q#PD3). +- The injected-kill seam is restored as an explicit decision (Q#PD4). + +**Rounds 1–3, for the record.** Rev 1 classified on errno alone and +claimed a live owned child cannot yield EPERM — false. Rev 2 gated on +`try_wait`, which observes the leader while a PTY signal targets the +foreground group — unsound whenever those diverge, and it could not be +shown to fix the observed failure at all. Rev 3 corrected EPERM but left +ESRCH unsafe and mixed the stages. **Three consecutive designs were +wrong in the same direction: each tried to conclude something about a +process from something that was not about that process.** + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half. **No grade + change and no behavioural change** — this lane only improves what a + failure reports. +- **Serves §9 (worker model), failure attribution**, in its most literal + sense: an error that names only an errno cannot be attributed. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 1. Ground truth (scouted @ `ccf29e3`, re-verified each revision) + +### 1.1 The failure reports an errno and nothing else + +`ProcessSupervisor::signal` (`src/process.rs:921`) maps the `kill` +failure to `format!("kill: {e}")` (`:931`). That string is everything a +reader gets. + +### 1.2 The signal target is not the observation target + +- **Signal target** — `signal_target` (`:687`) returns `-pgrp` for a + PTY, where `pgrp = master.process_group_leader()`: the tty's + **current foreground process group**, read at signal time. +- **Observation target** — `ChildHandle::try_wait` (`:668`) observes the + **spawned leader**. + +They coincide only while the leader owns the terminal. Job control is +precisely the mechanism that makes them diverge, and the PTY path is +**always group-directed by design** — spawn rejects `group = true` for +PTY mode with the rationale that "PTY children already lead their own +session and are signaled group-wide" (`:1428-1429`). + +**This is why every tolerance rule across rev 1–3 failed review**, and +why the diagnostic must record the target and the leader state as +*separate* facts. + +### 1.3 The reap ledger is disjoint from this path + +`tick_reap_ledger` (`:1075`) treats any probe error as "nothing left we +can reach" for **bounded growth**, asserting EPERM "cannot happen for our +own children". It is armed only for `proc.spec.group`, which PTY mode +cannot set. Rev 1's "asymmetry" argument was a misreading; withdrawn. + +### 1.4 The observed failure, and the limits of the evidence + +macOS CI, PR #172 (**docs-only** diff), `Test (macos-latest / luajit)`, +`acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel` +([attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30177276839/attempts/1)): + +``` +in function 'terminate' +cause: ExternalError(Process("kill: EPERM: Operation not permitted")) +``` + +**Established:** the errno, and the call path +(`pmacs.terminal.terminate` → `session.rs:566` → `signal`). + +**Not established:** that the child had exited (the probe's last source +statement is a file write at +`tests/bottom_panel_stage1_acceptance.rs:2239`; CPython teardown follows +and does not synchronise with it); that any pgid was recycled; or what +the signal target actually was. + +**This is the whole reason the lane is diagnostic.** Every candidate fix +needs at least one of those three facts, and none is available. + +### 1.5 Caller inventory + +| Caller | Disposition | +|---|---| +| `src/lsp.rs:1364`, `:2427` | discards (`let _ =`) | +| `src/mcp.rs:1229`, `:1239`, `:1915` | discards (`let _ =`) | +| `src/terminal/session.rs:319`, `:607`, `:635` | discards (`let _ =`) | +| **`src/terminal/session.rs:566`** (propagating at `:577`) | **propagates** as `TerminalError::Process` | +| supervisor-internal `shutdown` path | discards | +| `src/lua_bindings/mod.rs:8150`, `:8164` | propagates to Lua | +| `src/lua_bindings/mod.rs:8717` | propagates (via `session.rs:566`) | +| `src/daemon.rs:4162` | **test-only** `.expect`, not production | + +No test in the repository asserts either error string, so widening the +message breaks nothing. + +### 1.6 `portable-pty` caches the exit status on Unix + +Pinned `portable-pty 0.9.0`: `spawn_command` returns +`std::process::Child` (`unix.rs:228`), and `impl Child for +std::process::Child::try_wait` delegates to +`std::process::Child::try_wait` (`lib.rs:271-277`), which caches into +`self.status`. Both `ChildHandle` variants therefore cache. + + +## 2. Decisions + +### Q#PD1 — what the widened error records + +On a `kill` failure in `signal`, the error carries: + +| Field | Why | +|---|---| +| **target source** — `tcgetpgrp` vs `group` vs `leader-pid` fallback | which branch of `signal_target` (`:687`) ran | +| **target kind and value** — `-pgid` or `pid`, with the number | the entity actually signalled | +| **spawn-time pgid / leader pid** | a divergence from the target is the job-control hypothesis, visible only by comparison | +| **errno** | as today | +| **leader `try_wait` state** — `exited(status)` / `live` / `unobservable(e)` | separates "the leader is gone" from "the group we signalled is gone" — the distinction all three failed designs collapsed | + +Every candidate Stage B rule is decidable from these five together, and +none is decidable from the errno alone. + +### Q#PD2 — the disposition is preserved exactly + +The call still fails, with the same `Err`, in every case. No state +transition changes, no ledger arming changes, no tolerance. A reader +diffing behaviour should find none. + +### Q#PD3 — the honest claim is "no disposition change", not "strictly additive" + +Rev 3 said the diagnostic was only an error-string change and could not +regress behaviour. **That overstated it.** `try_wait` on an exited child +**reaps it and caches the status**, so consulting it in the failure path +is an internal state change: the child may be reaped earlier than it +otherwise would be. + +Observably safe, because both variants cache (§1.6) and `poll_one` +(`:1133`) will still see `Ok(Some(_))` and emit its event. But safe by +argument is not safe by assertion, so the terminate-failure-then-tick +event pin is retained (acceptance 5). + +### Q#PD4 — the injected-kill seam injects the KILL, never the observation + +Acceptance 5 needs a forced `kill` failure while the **real** +`ChildHandle::try_wait` runs against the **real** child. A stubbed +observation would bypass exactly the code path in question. + +So the seam is a test-only override of the *kill attempt's result*, +consumed once by the signal path; everything downstream — target +selection, the observation, the error construction — runs for real. This +also makes the diagnostic's own fields testable without racing the +kernel. + +### Q#PD5 — nothing else lands here + +No tolerance rule, no idempotence change, no `signal_target` change. See +§5. + + +## 3. Bets (falsifiable) + +- **B1 — The five fields are sufficient to discriminate the §1.4 + hypotheses.** Falsified if a recurrence carries all five and still + leaves the cause ambiguous — which would itself be a finding worth + having. +- **B2 — Widening the message breaks no caller.** Evidence: §1.5, and no + test asserts the string. + +*Retracted across revisions and not reinstated:* rev 1's "a live owned +child cannot yield EPERM"; rev 2's "exit observation suffices"; rev 2's +"this removes the failure class"; rev 3's "group ESRCH is safe to +tolerate". + + +## 4. Acceptance + +1. A group-directed `kill` failure produces an error carrying all five + Q#PD1 fields, with the target rendered as `-pgid` and the leader + state distinct from it. +2. A leader-directed `kill` failure does the same, with the target + rendered as `pid` and the target source recorded as the fallback + branch. +3. The leader state renders each of `exited(status)`, `live`, and + `unobservable(e)` correctly. +4. **The disposition is unchanged**: every injected failure still + returns `Err`, with no state transition and no ledger arming + (Q#PD2). Falsified by revert — flipping any arm to `Ok` fails this. +5. **Forced injected kill failure against the real PTY child + observation**, then tick: exactly one exit event, with the correct + status (Q#PD3/Q#PD4). A fully stubbed observation does not satisfy + this and is rejected as vacuous. +6. The existing suites stay green, pinning "no behavioural change" from + the outside. + + +## 5. Parked (not deferred-and-forgotten — each needs its own evidence) + +- **All tolerance rules.** Group-directed EPERM *and* ESRCH both fail on + the §1.2 entity split; leader-directed tolerance is plausible but + unmotivated until evidence shows the fallback branch is ever taken. + Needs Stage A evidence first. +- **Q#PS6, `terminate` on an already-reaped process returning `Ok`.** + Independent behavioural fix, different failure (§1.6 of rev 3), its + own lane under one-feature/one-PR. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — still the most + likely real fix site, still unframed. +- `terminate` cancelling pending restarts; PTYs in + `pmacs.process.list`; any change to `C-c` delivery. + + +## 6. Gates + +Full suite per `CLAUDE.md`. Touched suites: +`bottom_panel_stage1_acceptance`, the vterm stages, and +`compile_mode_acceptance`. Sweep with `-- --skip basedpyright`. + + +## 7. Branch plan + +`pty-terminate-eperm`, one PR, diagnostic only. This framing is its first +commit; the instrumentation and its tests are the second. + +**The lane then closes.** It does not wait for the flake to recur: the +next occurrence — whenever it happens, under whoever's PR — carries its +own evidence, and Stage B is framed then. Math work proceeds immediately +after this lands. 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/docs/vterm-framing.md b/docs/vterm-framing.md index b1124c0..7d4ca02 100644 --- a/docs/vterm-framing.md +++ b/docs/vterm-framing.md @@ -1674,6 +1674,64 @@ GPU assertions remain in `pmacs-protocol` and `pmacs-gpu` respectively. - **37:** one real-daemon/real-PTY/headless-wgpu acceptance path; it is not replaced by a decoded-message fixture. +### 0.12 As-framed audit, 2026-07-25 (after #166) + +Prompted by a GPU terminal input defect that shipped in Stage 3 and was fixed +in #166. The arc is structurally complete — all 37 criteria have +implementations, and every test named in the Stage 2 verification map exists — +but the audit found two gaps worth recording against the criteria themselves. + +**Criterion 22's "without thrash" was never pinned.** The criterion reads +"unchanged, zero, passive, and failed resize cases preserve prior geometry +*without thrash*". The word appears nowhere in `src/` or `tests/`. The suite +pinned the four enumerated single-arm cases and never the cross-arm +interaction — which is exactly where the thrash lived: the daemon applied +both the grid and the semantic terminal-layout sync to every attached +frontend, so a semantic session's PTY was resized twice per tick forever. +Criterion 31's "only the exact durable controller changes PTY geometry" was +violated in the same event, in spirit rather than letter: the controller was +the right frontend, but the geometry came from the grid projection. #166 adds +the settle pins; the gap was open from #135 (2026-07-22) until then. + +**Why the Stage 3 suite could not see it.** Of its nine tests, only three +drive a real daemon; the other six construct `EditorState` directly and never +execute the dispatcher loop where the defect lived. `a31`, which is about two +semantic frontends sharing one session, therefore passes on the broken tree. +The same structural blindness explains why `bottom_panel_stage1_acceptance` +was unaffected. A criterion about *dispatcher* behavior needs a test that +runs the dispatcher. + +**Four of the nine Stage 3 tests do not run in CI at all**, because they are +`#[cfg(feature = "crdt")]` and the workflow never enables that feature: +`a37`, the two added by #166, and +`terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret` — which +is Stage 3 review round 1's own regression guard. Stage 1's +`read_only_empty_crdt_bootstrap_is_immutable_against_remote_content`, the CRDT +half of criterion 14, is dark for the same reason. Stage 2 is fully covered +(6/6). This is not a vterm problem: 264 tests workspace-wide are dark, +including 177 in the library. It has its own lane in `docs/active-work.md`. + +**And `a37` is darker still than that count implies: it reports `ok` without +running whenever `pmacs-gpu` is absent from the same target directory** +(measured 2026-07-26 while gating #173). It derives the sibling binary from +`CARGO_BIN_EXE_pmacs` and, finding nothing, prints a skip notice and returns. +A fresh worktree reports the suite 9/9 in 0.17 s having executed the arc's +only real-daemon/real-PTY/real-wgpu path zero times; a genuine run takes +about four seconds. `PMACS_REQUIRE_GPU=1` is the only thing that turns that +skip into a failure, and the standing gate list applies that flag to +`cargo test -p pmacs-gpu`, a different package. So the audit's claim that +"only 3 of 9 Stage 3 tests drive a real daemon" was itself optimistic — +**on a target directory without the frontend binary the honest number is 2**, +and nothing in the gate log says so. It is also load-sensitive: it passed and +then failed at the same commit twenty minutes apart under machine +contention. Criterion 22's unpinned "without thrash" and this are the arc's +two standing verification gaps. + +**Not audited:** §11's blanket claim that "deferral means graceful ignore or +documented absence, never escape leakage, panic, unbounded allocation, or +child leak". That covers roughly twenty deferred items and none were +spot-checked. It remains an unproven claim rather than a known gap. + ## 10. Gates and bite verification Every PR runs the standing full gates from `AGENTS.md`, sequentially, plus its 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, } /// One armed group in the reap ledger. @@ -684,7 +691,50 @@ impl ChildHandle { } } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +/// Which branch of [`signal_target`] chose the target (Q#PD1). +/// +/// Recorded on failure because the branches differ in what a failing +/// `kill` can possibly mean: only [`Self::LeaderPid`] aims at the +/// spawned child itself. The other two aim at a *group*, which for a +/// PTY is read from the terminal and can belong to something the +/// supervisor never spawned. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TargetSource { + /// The tty's current foreground process group, read at signal + /// time. Diverges from the leader exactly when job control has + /// moved the terminal. + ForegroundGroup, + /// A `group = true` pipe child leading its own process group. + SpawnGroup, + /// The child's own pid. + LeaderPid, +} + +impl TargetSource { + fn as_str(self) -> &'static str { + match self { + Self::ForegroundGroup => "tcgetpgrp", + Self::SpawnGroup => "group", + Self::LeaderPid => "leader-pid", + } + } + + /// Whether the target is a process group rather than one process. + fn is_group(self) -> bool { + matches!(self, Self::ForegroundGroup | Self::SpawnGroup) + } +} + +/// The entity a signal was actually aimed at, plus the branch that +/// chose it. Carried so a failure can report the target as a fact +/// separate from the leader's state (Q#PD1). +#[derive(Debug, Clone, Copy)] +struct SignalTarget { + pid: Pid, + source: TargetSource, +} + +fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. @@ -692,7 +742,10 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { && let Some(pgrp) = master.process_group_leader() && pgrp > 0 { - return Ok(Pid::from_raw(-pgrp)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }); } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -700,11 +753,80 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { // (Q#CM3). if proc.spec.group { let pgid = i32::try_from(pid).map_err(|e| e.to_string())?; - return Ok(Pid::from_raw(-pgid)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgid), + source: TargetSource::SpawnGroup, + }); } - Ok(Pid::from_raw( - i32::try_from(pid).map_err(|e| e.to_string())?, - )) + Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::LeaderPid, + }) +} + +/// The spawned leader's state at the moment a `kill` failed (Q#PD1). +/// +/// Deliberately reported *beside* the target rather than folded into a +/// verdict: for a PTY the two are different entities whenever job +/// control has moved the terminal, and three successive designs for +/// this code were unsound precisely because they collapsed them. +enum LeaderObservation { + Exited(TermStatus), + Live, + Unobservable(String), + NoRuntime, +} + +impl LeaderObservation { + fn render(&self) -> String { + match self { + Self::Exited(TermStatus::Exited(code)) => format!("exited(code {code})"), + Self::Exited(TermStatus::Signaled(sig)) => format!("exited(signal {sig})"), + Self::Live => "live".to_owned(), + Self::Unobservable(e) => format!("unobservable({e})"), + Self::NoRuntime => "no-runtime".to_owned(), + } + } +} + +/// Observe the spawned leader. Note this *reaps* an exited child and +/// caches its status; that is why Q#PD3 claims "no disposition change" +/// rather than "strictly additive", and why an event-count test pins +/// that `poll_one` still emits exactly one exit event afterwards. +fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { + let Some(runtime) = proc.runtime.as_mut() else { + return LeaderObservation::NoRuntime; + }; + match runtime.child.try_wait() { + Ok(Some(status)) => LeaderObservation::Exited(status), + Ok(None) => LeaderObservation::Live, + Err(e) => LeaderObservation::Unobservable(e), + } +} + +/// Render a failing `kill` as the five facts of Q#PD1. The disposition +/// is unchanged (Q#PD2) — this only replaces a message that said +/// nothing but the errno. +fn signal_failure_report( + target: SignalTarget, + leader_pid: u32, + errno: nix::errno::Errno, + leader: &LeaderObservation, +) -> String { + let expected = if target.source.is_group() { + match i32::try_from(leader_pid) { + Ok(p) => format!(", expected_group=-{p}"), + Err(_) => String::new(), + } + } else { + String::new() + }; + format!( + "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + target.pid.as_raw(), + target.source.as_str(), + leader.render(), + ) } /// Termination status of one generation. Internal --- the supervisor @@ -807,9 +929,20 @@ impl ProcessSupervisor { shut_down: false, reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, + forced_kill_errno: None, } } + /// Q#PD4 test seam: make the next `kill(2)` attempt in + /// [`Self::signal`] report `errno` instead of calling the kernel. + /// Consumed by that one attempt. Everything downstream — target + /// selection, the leader observation against the real child, and + /// the error construction — runs unmodified. + #[cfg(test)] + fn force_next_kill_errno(&mut self, errno: nix::errno::Errno) { + self.forced_kill_errno = Some(errno); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -928,7 +1061,21 @@ impl ProcessSupervisor { return Err(format!("process {id} is not running")); }; let target = signal_target(proc, pid)?; - nix::sys::signal::kill(target, Some(signal)).map_err(|e| format!("kill: {e}"))?; + // Q#PD4: the seam injects the KILL attempt's result only — + // never the observation below — so target selection, the real + // `ChildHandle::try_wait` against the real child, and the error + // construction all run for real. Consumed once. + let kill_result = match self.forced_kill_errno.take() { + Some(errno) => Err(errno), + None => nix::sys::signal::kill(target.pid, Some(signal)), + }; + if let Err(errno) = kill_result { + // Q#PD1/Q#PD2: the failure describes itself; the + // disposition is unchanged — this still returns `Err`, + // with no state transition and no ledger arming. + let leader = observe_leader(proc); + return Err(signal_failure_report(target, pid, errno, &leader)); + } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { pid, @@ -2131,6 +2278,290 @@ mod tests { ); } + /// Spawn a PTY child that leads its own session and stays alive + /// until terminated, returning its id and OS pid. + /// + /// `/bin/sleep` directly rather than through a shell: a shell may + /// place the command in a different foreground process group, and + /// these tests assert the exact target the tty reports. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> (ProcessId, u32) { + let mut spec = ProcessSpec::new(name, "/bin/sleep"); + spec.args = vec!["30".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + (id, spawn_started_pid(sup, id)) + } + + /// The OS pid straight from the supervisor's own record, WITHOUT + /// ticking. + /// + /// `drain_until` ticks, and a tick can observe a fast child's exit + /// and transition the record out of `Running` — after which + /// `signal` returns "is not running" and never reaches the + /// diagnostic at all. Any test whose child exits promptly must read + /// the pid this way. (Found by the parallel workspace sweep: the + /// drain-based helper raced only under load.) + fn record_pid(sup: &ProcessSupervisor, id: ProcessId) -> u32 { + match sup.processes.get(&id).expect("record").state { + ProcessState::Running { pid, .. } | ProcessState::Exiting { pid, .. } => pid, + ProcessState::Starting => panic!("spawn has not reported a pid yet"), + ProcessState::Terminated(_) => { + panic!("the record already left Running; the pid is unavailable") + } + } + } + + /// Drain until `Started` and return the OS pid it carries. Safe + /// only for children that outlive the drain; see [`record_pid`]. + fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 { + let evs = drain_until(sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + evs.iter() + .find_map(|e| match e.kind { + ProcessEventKind::Started { pid } => Some(pid), + _ => None, + }) + .expect("Started carries a pid") + } + + /// Drive the production diagnostic until it observes the leader as + /// exited, bounded by `timeout`. + /// + /// A fixed sleep is NOT proof of exit — on a loaded runner the child + /// can still be live, which would turn these tests into false + /// failures. This synchronises on the very observation under test. + /// Each failing attempt leaves the record untouched, because the + /// failure path returns before any bookkeeping (Q#PD2), so looping + /// is side-effect free. + fn terminate_until_leader_exited( + sup: &mut ProcessSupervisor, + id: ProcessId, + timeout: Duration, + ) -> String { + let deadline = Instant::now() + timeout; + loop { + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + if err.contains("leader=exited(") { + return err; + } + assert!( + !err.contains("is not running"), + "the record left Running before the diagnostic could run, so \ + this test never exercised it: {err}" + ); + assert!( + Instant::now() < deadline, + "leader never observed as exited within {timeout:?}: {err}" + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + + /// Q#PD1 acceptance 1 — a group-directed failure names the target, + /// the branch that chose it, the expected group, the errno, and the + /// leader's own state, as five separate facts. + /// + /// Asserted as an exact message against the pid the kernel actually + /// assigned, so a hardcoded target could not satisfy it. The leader + /// field is the one that matters: for a PTY the signal goes to the + /// terminal's foreground group, a different entity from the spawned + /// child whenever job control has moved the terminal. Three rejected + /// designs for this code collapsed the two; the report keeps them + /// apart, and here they are asserted to agree only because nothing + /// has moved the terminal. + #[test] + fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + let mut sup = ProcessSupervisor::new(); + let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the report names the exact target the tty reported, the exact \ + leader pid, and observes the leader as live" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 2 — a leader-directed failure records the + /// fallback branch and a positive target, and omits the group field + /// that would be meaningless for it. Exact message again. + #[test] + fn a_leader_directed_kill_failure_reports_the_fallback_branch() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.force_next_kill_errno(nix::errno::Errno::ESRCH); + let err = sup.terminate(id).expect_err("injected ESRCH must fail"); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + nix::errno::Errno::ESRCH + ); + assert_eq!( + err, expected, + "a non-group pipe child targets its own pid, and the group \ + field is omitted where it has no meaning" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 3 — every leader state renders distinctly. The + /// `Unobservable` and `NoRuntime` arms cannot be produced by a real + /// child on demand, so they are pinned directly; `live` and `exited` + /// are pinned through the real path by the tests around this one. + #[test] + fn every_leader_observation_renders_distinctly() { + assert_eq!( + LeaderObservation::Exited(TermStatus::Exited(0)).render(), + "exited(code 0)" + ); + assert_eq!( + LeaderObservation::Exited(TermStatus::Signaled("SIGTERM".into())).render(), + "exited(signal SIGTERM)" + ); + assert_eq!(LeaderObservation::Live.render(), "live"); + assert_eq!( + LeaderObservation::Unobservable("try_wait: boom".into()).render(), + "unobservable(try_wait: boom)" + ); + assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); + } + + /// Q#PD1 acceptance 3, exited arm through the REAL path — the leader + /// has genuinely exited and the report carries its exact code, not + /// merely "some exit". + #[test] + fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 3".into()]; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and this child exits + // immediately. + let pid = record_pid(&sup, id); + + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the exact exit code is observed from the real child, not \ + inferred from the errno" + ); + } + + /// Q#PD2 acceptance 4 — **the disposition is unchanged.** An + /// injected failure still fails, and neither the state transition + /// nor the reap-ledger arming runs. This is the assertion that + /// separates a diagnostic from the tolerance rules three review + /// rounds rejected; flipping any arm to `Ok` fails it. + #[test] + fn an_injected_failure_changes_no_state_and_arms_no_ledger() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + assert!( + sup.reap_ledger.is_empty(), + "precondition: nothing armed before the attempt" + ); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "a group=true pipe child reports via group"); + + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed kill must not transition the record to Exiting" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed kill must not arm the reap ledger" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL + /// `ChildHandle::try_wait` on the REAL child, which reaps it and + /// caches the status. `poll_one` must still emit exactly one exit + /// event, carrying the exact code. + /// + /// A stubbed observation would bypass the double-`try_wait` path + /// entirely and pin nothing, so the injection replaces the kill + /// result only. + #[test] + fn observing_the_leader_does_not_consume_the_exit_event() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 7".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and a tick can reap + // this immediately-exiting child before the diagnostic runs. + let _ = record_pid(&sup, id); + + // Drives `observe_leader`, which try_waits the real PTY child + // for the first time and reaps it. + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + assert!( + err.contains("leader=exited(code 7)"), + "the real handle was consulted and carries the exact code: {err}" + ); + + // The supervisor's own try_wait must still see that status. + let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let terminal: Vec = evs + .iter() + .filter_map(|e| match e.kind { + ProcessEventKind::Exited { code, .. } => Some(code), + ProcessEventKind::Signaled { .. } => Some(-1), + _ => None, + }) + .collect(); + assert_eq!( + terminal, + vec![7], + "exactly one terminal event survives the diagnostic's try_wait, \ + carrying the child's real exit code" + ); + } #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new(); diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 1c0957d..b787dee 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -329,6 +329,29 @@ impl TerminalManager { copy_selection_bytes(&rows, selection) } + /// Serialize a session's ENTIRE retained range — scrollback plus the + /// visible screen — through the same path [`copy_selection`] uses. + /// + /// Q#TC7. This deliberately builds a whole-range *selection* and hands + /// it to the existing serializer rather than walking the rows itself. + /// Soft-wrap joining, wide-glyph continuation, cluster bytes, and + /// per-row trailing-blank trimming are Vterm Stage 2 criterion 21's + /// pinned behavior; a second walk would re-derive all four and the two + /// would drift. That inheritance is what acceptance 13 asserts, by + /// comparing this against a full-range `copy_selection` rather than + /// against a literal. + /// + /// Returns `None` for a non-terminal buffer and for a session whose + /// retained rows are all empty — there is no cell to anchor to. + /// Unlike `copy_selection` this needs no registered view, so copy mode + /// does not depend on the terminal being currently displayed. + #[must_use] + pub fn copy_retained(&self, buffer_id: BufferId) -> Option> { + let session = self.sessions.get(&buffer_id)?; + let projection = session.screen.projection_ref(); + retained_bytes(&retained_rows(projection)) + } + /// Start an editor-owned primary selection at a viewport coordinate. pub fn begin_selection( &mut self, @@ -540,6 +563,40 @@ fn retained_rows(projection: BorrowedScreenProjection<'_>) -> RetainedRows<'_> { RetainedRows { projection } } +/// Serialize every retained cell, through the selection-copy serializer. +/// +/// Split out from [`TerminalManager::copy_retained`] so the fidelity +/// claims — soft-wrap joining, per-row trailing-blank trimming, wide-glyph +/// continuation, cluster bytes — are testable against the same projection +/// fixtures that pin `copy_selection_bytes` itself. Those four are exactly +/// what a second, independently written walk would get wrong. +fn retained_bytes(rows: &RetainedRows<'_>) -> Option> { + copy_selection_bytes(rows, full_retained_selection(rows)?) +} + +/// The selection spanning every retained cell. +/// +/// Rows with no cells are skipped at both ends rather than clamped: an +/// anchor into a zero-width row cannot resolve (`resolve_anchor` requires +/// `cell_offset` to fall inside `cell_offset .. cell_offset + len`), so +/// including one would make the whole range unresolvable and silently +/// yield nothing. Interior empty rows are untouched, because trailing- and +/// interior-blank handling belongs to the serializer. +fn full_retained_selection(rows: &RetainedRows<'_>) -> Option { + let mut occupied = rows.iter().filter(|row| !row.cells.is_empty()); + let first = occupied.next()?; + // `RetainedRows::iter` is a chain of slice iterators exposed as + // `impl Iterator`, so it is not double-ended; scan forward. + let last = occupied.last().unwrap_or(first); + Some(TerminalSelection { + anchor: row_lead(first), + head: LogicalCellAnchor { + logical_line_id: last.logical_line_id, + cell_offset: last.cell_offset.saturating_add(last.cells.len() as u32 - 1), + }, + }) +} + fn row_lead(row: &TerminalRow) -> LogicalCellAnchor { LogicalCellAnchor { logical_line_id: row.logical_line_id, @@ -1001,6 +1058,92 @@ mod tests { assert_eq!(bytes, b"abcd\ne"); } + /// Stage 2 criteria 13 and 14. Every property here is one a second, + /// independently written whole-range walk would get wrong: a naive + /// walk emits a newline per physical row (breaking the soft wrap), + /// keeps trailing default blanks, and has to rediscover that history + /// precedes the visible screen. Asserting exact bytes is what makes + /// "it reuses the serializer" falsifiable. + #[test] + fn retained_copy_spans_history_joins_soft_wraps_and_trims_blanks() { + let source = projection( + vec![row(1, 0, "ab ", true), row(1, 3, "cd ", false)], + vec![row(2, 0, "e ", false), row(3, 0, " ", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let bytes = retained_bytes(&retained).expect("whole range resolves"); + // `ab`+`cd` joined across the soft wrap; `e` on its own hard row; + // the all-blank final row trimmed to nothing but still separated. + assert_eq!(bytes, b"abcd\ne\n"); + } + + /// The whole-range selection must not depend on a view existing, and + /// must agree with an explicit full-span selection through the public + /// serializer — the anti-drift half of criterion 13. + #[test] + fn retained_copy_agrees_with_an_explicit_full_span_selection() { + let source = projection( + vec![row(1, 0, "aaa", false)], + vec![row(2, 0, "bbb", false), row(3, 0, "ccc", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let explicit = copy_selection_bytes( + &retained, + TerminalSelection { + anchor: LogicalCellAnchor { + logical_line_id: 1, + cell_offset: 0, + }, + head: LogicalCellAnchor { + logical_line_id: 3, + cell_offset: 2, + }, + }, + ) + .expect("explicit selection resolves"); + assert_eq!(retained_bytes(&retained).expect("whole range"), explicit); + assert_eq!(explicit, b"aaa\nbbb\nccc"); + } + + /// A wide glyph must be copied once across the whole range too, not + /// once per cell it occupies. + #[test] + fn retained_copy_emits_a_wide_glyph_once() { + let wide = TerminalRow { + cells: vec![ + Cell { + glyph: Glyph::Char('界'), + style: Style::default(), + attachment: None, + }, + Cell { + glyph: Glyph::Continuation, + style: Style::default(), + attachment: None, + }, + Cell::default(), + ], + logical_line_id: 9, + cell_offset: 0, + soft_wrapped: false, + }; + let source = projection(Vec::new(), vec![wide]); + let retained = retained_rows(source.as_borrowed()); + assert_eq!( + retained_bytes(&retained).expect("whole range"), + "界".as_bytes() + ); + } + + /// A session with nothing retained yields `None` rather than an empty + /// string, so the caller can tell "no terminal" from "empty terminal". + #[test] + fn retained_copy_of_zero_width_rows_is_none() { + let source = projection(Vec::new(), vec![row(1, 0, "", false)]); + let retained = retained_rows(source.as_borrowed()); + assert!(retained_bytes(&retained).is_none()); + } + #[test] fn wide_continuation_canonicalizes_to_lead_and_copies_once() { let wide = TerminalRow { diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs new file mode 100644 index 0000000..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}" + ); +}