From e1b859fd4fcc8dbd646f27207d0142defa4e01d5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 09:50:09 -0400 Subject: [PATCH 1/7] feat(generated-buffers): dired and listview adopt the authorized write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 1 of generated-buffer immutability (docs/generated-buffer-immutability-framing.md, revision 5). Closes the two families the bug is reachable on WITHOUT `M-x`: `compile.lua` and the search panel rebind all seven undo chords to a no-op, but `dired.lua` and `listview.lua` rebind nothing, so a bare `C-/` emptied a listing and a panel. The cheap half is also the exposed half. An intercept is not read-only. `Buffer::undo` reaches the rope through `ensure_writable` and never consults the intercept chain, so the erroring-intercept-plus-`bypass_intercept`-over-a-writable-rope idiom guarded the edit path and left the history path open. Rebinding chords does not close it: `M-x buffer.undo` is dispatchable on every buffer in the tree. - `dired.lua`'s `paint` and `listview.lua`'s `render` write through `pmacs.buffer.set_generated_contents` — lift the lock, whole-buffer replace skipping intercepts, discard history, re-assert the lock, fan the `Edit` out. Zero `bypass_intercept` writes remain in either file. - Both keep their named erroring intercept and `set_round_trip_input`. The layering at `terminal.lua:351-366` is unchanged: the rope lock protects the daemon copy, round-trip input protects a semantic frontend's own mirror, and neither substitutes for the other. - Q#GB13 — `listview.ensure_panel` stops adopting a same-named foreign buffer. Ownership is the `panels` table; a collision disambiguates `<2>`..`<99>` and raises at the limit, matching `dired.lua:476-504`. This is a prerequisite of the lock, not a follow-up: the arc removes the `M-x buffer.undo` that was the only recovery from a clobber. - Q#GB18 — `panels` becomes a compacting list keyed by identity. It was written under the requested name and read back under the actual name, which a disambiguated panel breaks: `RET`, `g` and `q` fail closed and silently, and `listview.open`'s capture guard fails OPEN, capturing a panel as its own `q` target — the chained-panel loop its comment says it prevents. Ships in the same commit as the disambiguation by the framing's ordering constraint. - Q#GB6 — `EditorCore::notify_buffer_edit` clamps each window coordinate against its own post-edit bound, unconditionally. `cursor` is a byte position bounded by `Buffer::len`; `view_top` is a line index bounded by `TextView::line_count`, and a replace can grow in bytes while collapsing lines, so "the buffer shrank" is not a usable trigger. This fixes a shipped defect that reaches terminal copy mode. - Q#GB16(a) — locking these families disables fold CREATION on them, because `document_bytes` is spelled `is_read_only()`. Accepted and stated rather than shipped silently; the status string now names the read-only lock instead of claiming "not a document buffer". Acceptance: 10 new criteria in `listview_acceptance` (16 total), 6 in `dired_acceptance` (31 total), 2 in `terminal_copy_mode_acceptance`. Every criterion's falsifying mutation was run: 5 bite by revert against `githubsucks/main`, 9 by a named one-line mutation. Two framing corrections, both recorded in the tests rather than worked around silently: - Stage 1 criterion 5 is unreachable as written. `Buffer::apply_edit` (`src/buffer.rs:773`) and `begin_edit` (`:725`) call `ensure_writable` as their FIRST statement while the intercept chain runs later inside `apply_edit_inner` (`:1072`), so once this arc's lock is installed an ordinary edit can never reach the intercept. Restated at the one point where the two are distinguishable — the lock lifted — which is the state the intercept genuinely still covers. - Criterion 7 cannot bite at the listview adopter. `listview.refresh` and `listview.open` both follow `render` with `window.switch_buffer`, which rebuilds the `TextView` from scratch and masks a dropped fan-out. `dired.revert` does not, so the dired half carries the bite; it fails under the mutation with the reported `assertion failed: end <= self.len()`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- builtin/runtime/dired.lua | 20 +- builtin/runtime/listview.lua | 157 ++++++-- src/editor_core.rs | 21 + src/lua_bindings/fold.rs | 18 +- tests/dired_acceptance.rs | 346 +++++++++++++++- tests/listview_acceptance.rs | 526 +++++++++++++++++++++++++ tests/terminal_copy_mode_acceptance.rs | 116 ++++++ 7 files changed, 1166 insertions(+), 38 deletions(-) diff --git a/builtin/runtime/dired.lua b/builtin/runtime/dired.lua index c8054fe..11d78ce 100644 --- a/builtin/runtime/dired.lua +++ b/builtin/runtime/dired.lua @@ -364,11 +364,23 @@ local function render_text(handle) return table.concat(lines, "\n") end --- Dired's own writes are the only ones that reach the buffer: the --- read-only intercept rejects everything else, and this bypasses it. +-- Dired's own writes are the only ones that reach the buffer, and this +-- is the one authorized door (Q#GB1, +-- docs/generated-buffer-immutability-framing.md). +-- +-- `set_generated_contents` lifts the rope's `read_only`, replaces the +-- whole buffer skipping intercepts, discards the resulting history and +-- re-asserts the lock --- all inside one registry borrow, so the buffer +-- is never observably unlocked. The erroring intercept this replaces a +-- bypass write beside is KEPT: it guards the edit path with a named +-- error, but `Buffer::undo` reaches the rope through `ensure_writable` +-- and never consults the intercept chain, so a listing protected by an +-- intercept alone was emptied by a bare `C-/` --- dired rebinds no undo +-- chord --- and by `M-x buffer.undo`, which no rebinding can remove. +-- Only rope-level `read_only` closes that, and only the pairing keeps +-- this repaint working after it. local function paint(handle) - local text = render_text(handle) - handle.buf:replace(0, handle.buf:len(), text, { bypass_intercept = true }) + pmacs.buffer.set_generated_contents(handle.buf, render_text(handle)) end -- --------------------------------------------------------------------------- diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 6a6d717..49f19e5 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -5,11 +5,16 @@ -- wholesale re-render, buffer-local RET/n/p/g/q keymap, a -- line->item map, previous-buffer capture + `q` restore, and the two -- disciplines the hand-rolled original lacks --- a read-only --- intercept (Q#P3; the panel's own renders write with --- bypass_intercept) and the Q#P6 round-trip-input mark, so a +-- intercept (Q#P3) and the Q#P6 round-trip-input mark, so a -- semantic frontend's RET dispatches into the visit binding instead -- of optimistically inserting a newline. -- +-- Generated-buffer immutability (Q#GB1, docs/generated-buffer-immutability-framing.md): +-- a panel's rope is genuinely read-only, and `render` is its owner's one +-- authorized door through the lock. The intercept alone protected the +-- edit path and left the history path open, so `C-/` emptied a panel. +-- Ownership is the `panels` table, never a name match (Q#GB13/Q#GB18). +-- -- Panels are buffers, so both frontends render them with zero -- protocol change (Q#P2: switch-in-place; the GPU cannot show -- splits). Framing: docs/lsp-panels-framing.md. @@ -24,9 +29,37 @@ pmacs.listview = pmacs.listview or {} --- name -> { buffer, prev, header, line_to_item, on_visit, on_refresh } +-- panels: array of +-- { requested_name, buffer, prev, header, line_to_item, on_visit, on_refresh } +-- +-- A LIST scanned by identity, not a name-keyed map (Q#GB18). `panels` +-- used to be written under the name the CALLER asked for and read back +-- under the buffer's ACTUAL name; those are the same string only while +-- `ensure_panel` adopts whatever buffer already carries the name. Once +-- ownership disambiguates a collision to `*references*<2>` (Q#GB13), a +-- name-keyed lookup can never find its own record, and every consumer +-- below fails: `RET`, `g` and `q` fail closed and silently, while +-- `open`'s capture guard fails OPEN and captures a panel as its own `q` +-- target --- the chained-panel loop its comment says it prevents. +-- +-- Keyed by linear scan over `BufferIdLua.__eq` rather than by table key +-- for the same reason dired's `handles` is (dired.lua:120-140): two +-- BufferIdLua values for the same buffer are distinct userdata, so a +-- `panels[buf]` lookup would miss. `compile.lua`'s `slot_for_buffer` +-- is the third instance of this shape; listview adopts it rather than +-- inventing a fourth. +-- +-- Dead panels are compacted out on every scan. A map held at most one +-- entry per name and self-limited; a list does not, so killing and +-- reopening `*references*` ten times would otherwise leave nine dead +-- records for every scan to walk. local panels = {} +-- How far the `<2>`, `<3>`, ... disambiguation walks before giving up. +-- dired.lua:474's constant, same value, same give-up-rather-than-adopt +-- rule. +local NAME_VARIANT_LIMIT = 99 + local function find_buffer_by_name(name) for _, id in ipairs(pmacs.buffer.list()) do local ok, d = pcall(pmacs.describe.buffer, id) @@ -35,18 +68,52 @@ local function find_buffer_by_name(name) return nil end +local function live_panels() + local live = {} + for _, p in ipairs(panels) do + local ok, valid = pcall(p.buffer.is_valid, p.buffer) + if ok and valid then live[#live + 1] = p end + end + panels = live + return live +end + +-- The record for the panel `spec.name` asked for. Stable across +-- disambiguation: a repeated `listview.open{ name = "*references*" }` +-- must reach the same panel even when its buffer is called +-- `*references*<2>`. +local function panel_for_requested_name(name) + for _, p in ipairs(live_panels()) do + if p.requested_name == name then return p end + end + return nil +end + +-- The record that owns `buf`, or nil. This is the identity question +-- every command below actually asks. +local function panel_for_buffer(buf) + if buf == nil then return nil end + for _, p in ipairs(live_panels()) do + if p.buffer == buf then return p end + end + return nil +end + -- The panel record whose buffer the active window shows, or nil. -local function panel_for_current_buffer() - local buf = pmacs.window.buffer() - if not buf then return nil end - local ok, d = pcall(pmacs.describe.buffer, buf) - if not (ok and d) then return nil end - return panels[d.name] +local function active_panel() + return panel_for_buffer(pmacs.window.buffer()) end -- Wholesale re-render: header + one line per row, rebuilding the -- line->item map (data lines are 1-based; the header is line 0). --- Panel writes bypass the read-only intercept. +-- +-- One `set_generated_contents` (the owner-authorized write) rather than +-- a delete-all + insert-all pair through `bypass_intercept`. The +-- intercept guarded the edit path and left the HISTORY path open, so a +-- bare `C-/` --- listview rebinds no undo chord --- emptied the panel; +-- `M-x buffer.undo` did too, and no rebinding can remove that. The +-- primitive lifts the rope lock, writes, discards the history and +-- re-asserts the lock, all inside one registry borrow. local function render(p, rows) local lines = { p.header } p.line_to_item = {} @@ -54,11 +121,7 @@ local function render(p, rows) lines[#lines + 1] = row.text p.line_to_item[#lines - 1] = row.item end - local body = table.concat(lines, "\n") - local buf = p.buffer - local len = buf:len() - if len > 0 then buf:delete(0, len, { bypass_intercept = true }) end - if #body > 0 then buf:insert(0, body, { bypass_intercept = true }) end + pmacs.buffer.set_generated_contents(p.buffer, table.concat(lines, "\n")) end -- Re-seat the cursor on data line `line` (1-based, clamped). @@ -87,19 +150,53 @@ local function bind_local_keymap(buf) bind("q", "listview.quit") end --- Build (or adopt) the persistent panel record for `name`. Handles a --- user-killed panel buffer by recreating it. +-- Build the persistent panel record for `name`. A user-killed panel +-- buffer is compacted out by `live_panels`, so the next `open` builds a +-- fresh record rather than resurrecting a dead one. +-- +-- Q#GB13: found-by-name is NOT adoption. `pmacs.buffer.create` takes any +-- caller-chosen name, so a foreign buffer may already be called +-- `*references*`; this used to adopt it, clobber the user's bytes, and +-- install an erroring intercept whose handle it discarded --- leaving +-- the user's buffer permanently un-editable. Rendering through +-- `set_generated_contents` would additionally lock its rope and clear +-- the history, removing the `M-x buffer.undo` that is currently the only +-- way back. So ownership is "this buffer is in `panels`", a name +-- collision disambiguates `<2>`..`<99>`, and exhausting the limit raises +-- rather than adopting --- the rule terminal.lua:300-305 states and +-- dired.lua:476-504 already implements. local function ensure_panel(name) - local p = panels[name] - if p and p.buffer:is_valid() then return p end - local buf = find_buffer_by_name(name) or pmacs.buffer.create(name) - p = { buffer = buf, line_to_item = {} } - panels[name] = p - -- Read-only (Q#P3): every non-bypass edit is rejected. The - -- intercept lives as long as the buffer; no teardown (the - -- buffer-list precedent for its keymap). + local p = panel_for_requested_name(name) + if p then return p end + + local actual = name + if find_buffer_by_name(actual) then + local unique = nil + for i = 2, NAME_VARIANT_LIMIT do + local candidate = string.format("%s<%d>", name, i) + if find_buffer_by_name(candidate) == nil then + unique = candidate + break + end + end + if unique == nil then + error(string.format("listview: %s is taken and no free variant remains", name)) + end + actual = unique + end + + local buf = pmacs.buffer.create(actual) + p = { requested_name = name, buffer = buf, line_to_item = {} } + panels[#panels + 1] = p + -- Read-only (Q#P3): every non-bypass edit is rejected, with a NAMED + -- error. Kept beside the rope lock, not replaced by it: the layering + -- at terminal.lua:351-366 --- the rope lock protects the daemon copy, + -- this and the round-trip mark protect a semantic frontend's own + -- mirror, and neither substitutes for the other. The intercept lives + -- as long as the buffer; no teardown (the buffer-list precedent for + -- its keymap). pmacs.buffer.add_intercept(buf, function() - error(name .. " is read-only") + error(actual .. " is read-only") end) -- Q#P6: semantic frontends must round-trip keys while this panel -- is focused (RET = visit, not an optimistic newline). @@ -119,7 +216,7 @@ function pmacs.listview.open(spec) -- (chained panels would trap `q` in a loop; restore targets the -- last real buffer). local active = pmacs.window.buffer() - if active and not panel_for_current_buffer() then + if active and not panel_for_buffer(active) then p.prev = active end render(p, spec.rows or {}) @@ -147,7 +244,7 @@ pmacs.command.define { name = "listview.visit", description = "Visit the list-panel item under the cursor.", fn = function() - local p = panel_for_current_buffer() + local p = active_panel() if not p then return end local item = p.line_to_item[pmacs.editor.cursor_line()] if item ~= nil and p.on_visit then p.on_visit(item) end @@ -158,7 +255,7 @@ pmacs.command.define { name = "listview.refresh", description = "Re-run the list panel's data source and re-render.", fn = function() - local p = panel_for_current_buffer() + local p = active_panel() if not (p and p.on_refresh) then return end local saved = pmacs.editor.cursor_line() local rows = p.on_refresh() or {} @@ -174,7 +271,7 @@ pmacs.command.define { name = "listview.quit", description = "Leave the list panel, restoring the previous buffer.", fn = function() - local p = panel_for_current_buffer() + local p = active_panel() if not p then return end -- Bottom-panel arc (Q#BP11b): `q` keeps its name and its -- user-visible behavior, delegating to `window.quit` only when the diff --git a/src/editor_core.rs b/src/editor_core.rs index 661b767..28fa0f4 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1833,18 +1833,39 @@ impl EditorCore { /// and translate the live origin — otherwise accepted-match /// highlights and the session origin survive at pre-edit offsets /// for every Lua mutator edit and applied CRDT op. + /// + /// Q#GB6: each window coordinate is also clamped against **its own** + /// post-edit bound, which [`Self::rebuild_views_for`] already does + /// (`:1853-1857`) and this path did not. A generated refresh that + /// shrinks its buffer otherwise leaves `cursor` past the end of the + /// rope indefinitely — neither paint nor a motion command recovers + /// it, because motion is computed from the stale value. The two + /// coordinates fail on different axes and are therefore clamped + /// separately and **unconditionally**: `cursor` is a byte position + /// bounded by [`Buffer::len`], while `view_top` is a line index + /// bounded by [`TextView::line_count`]. A replace can grow in bytes + /// while collapsing many lines into one, so "the buffer shrank" is + /// not a usable trigger for the second. pub fn notify_buffer_edit(&mut self, buffer_id: BufferId, edit: &Edit) { self.search_invalidate_for_edit(buffer_id, edit); let reg = self.registry.borrow(); let Ok(buffer) = reg.get(buffer_id) else { return; }; + let len = buffer.len(); for win in self.windows.values_mut() { if win.buffer_id == buffer_id { let _ = win.text_view.on_edit(buffer, edit); for overlay in &mut win.overlays { let _ = overlay.on_edit(buffer, edit); } + if win.cursor > len { + win.cursor = len; + } + let max_top = win.text_view.line_count().saturating_sub(1); + if win.view_top > max_top { + win.view_top = max_top; + } } } } diff --git a/src/lua_bindings/fold.rs b/src/lua_bindings/fold.rs index 0d4664c..1faf654 100644 --- a/src/lua_bindings/fold.rs +++ b/src/lua_bindings/fold.rs @@ -65,7 +65,13 @@ pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Resu let id = buf.id(); let requested = range_from_table(&range)?; let Some(bytes) = document_bytes(lua, id)? else { - set_status(lua, "fold rejected: not a document buffer"); + // Q#GB16(a): the guard is spelled `is_read_only()`, + // so this is the message it can actually justify. + // Its author meant "terminal"; generated-buffer + // immutability makes dired listings and listview + // panels read-only too, and "not a document buffer" + // would be a false explanation for those. + set_status(lua, "fold rejected: buffer is read-only"); return Ok(false); }; if requested.start > bytes.len() as u64 @@ -307,6 +313,16 @@ fn range_to_table(lua: &Lua, r: ByteRange) -> mlua::Result { /// The buffer's bytes if it is a normal document buffer, or `None` if it is /// read-only (a terminal identity buffer or other non-document buffer — /// the Q#FD11 "normal document buffer" guard). +/// +/// Q#GB16: the guard's author meant "terminal", and `read_only` is what +/// they had. Generated-buffer immutability widens the flag's population +/// — a dired listing and a listview panel are read-only from their first +/// paint — so fold **creation** is now refused on those families too. +/// That is accepted rather than worked around (option (a)): a generated +/// buffer's contents are replaced wholesale on every refresh, which +/// invalidates any stored range anyway. What is *not* accepted is +/// explaining the refusal with a sentence that is no longer true, hence +/// the status text at the `fold` call site. fn document_bytes(lua: &Lua, buf: BufferId) -> mlua::Result>> { with_registry(lua, |r| { let buffer = resolve(r, buf)?; diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index 7d0f2c5..b2011bb 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -26,6 +26,7 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, Instant, SystemTime}; use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; +use pmacs::buffer::BufferId; use pmacs::cell::{CellGrid, CellSize, Glyph}; use pmacs::editor::EditorState; use pmacs::editor_core::normalize_buffer_path; @@ -70,6 +71,39 @@ fn type_str(s: &mut EditorState, text: &str) { } } +fn alt(s: &mut EditorState, c: char) { + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Char(c), KeyModifiers::ALT)); +} + +/// `M-x RET` through the real minibuffer. `buffer.undo` is +/// reachable this way on every buffer in the tree and no buffer-local +/// rebinding can remove it (generated-buffer immutability, §0). +fn m_x(s: &mut EditorState, name: &str) { + alt(s, 'x'); + type_str(s, name); + press(s, KeyCode::Enter); +} + +fn active_buffer_id(s: &EditorState) -> BufferId { + s.core.borrow().active_buffer_id() +} + +/// Q#GB14: the rope lock has no Lua surface, so every "is it locked" +/// assertion goes through Rust. +fn is_read_only(s: &EditorState, id: BufferId) -> bool { + let core = s.core.borrow(); + let reg = core.registry.borrow(); + reg.get(id).expect("buffer in registry").is_read_only() +} + +fn set_read_only(s: &EditorState, id: BufferId, value: bool) { + let core = s.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(id) + .expect("buffer in registry") + .set_read_only(value); +} + fn exec(s: &EditorState, src: &str) { s.lua_host.lua().load(src.to_string()).exec().unwrap(); } @@ -958,13 +992,21 @@ fn dired_does_not_adopt_a_foreign_buffer_with_its_name() { // 5 --- read-only discipline // --------------------------------------------------------------------------- -/// An ordinary self-insert is rejected by the intercept and leaves the -/// text byte-identical, while dired's own repaint succeeds through -/// `bypass_intercept`. `set_round_trip_input` is pinned through the +/// An ordinary self-insert is rejected and leaves the text +/// byte-identical, while dired's own repaint still succeeds through the +/// owner-authorized write. `set_round_trip_input` is pinned through the /// **production** seam a semantic frontend reads (`dispatch_idle_for`, /// published as `DispatchIdle`) rather than by a direct-call assertion: /// without it, a GPU session would optimistically apply `g` as an /// insert instead of letting it reach the revert binding. +/// +/// **This test is NOT coverage of the generated-buffer adoption**, and +/// the `contains("read-only")` assertion below is the reason to say so: +/// `BufferError::ReadOnly` renders as ``buffer `{name}` (id {id:?}) is +/// read-only`` and the intercept's own message ends in `is read-only` +/// too, so that substring passes on both sides of the change. What the +/// adoption adds here is the explicit `is_read_only` assertion and +/// criterion 6(c); the undo criteria are separate tests below. #[test] fn dired_buffer_is_read_only_and_round_trips_input() { let td = fixture_dir(); @@ -988,6 +1030,33 @@ fn dired_buffer_is_read_only_and_round_trips_input() { "a round-trip buffer must turn optimistic apply OFF" ); + // Generated-buffer immutability Stage 1 criterion 6(c), the positive + // control: `dispatch_idle_for` has SIX ways to return false, so the + // assertion above is satisfied by any of them. Switching the same + // window to a plain buffer must flip the gate back ON --- a stuck + // minibuffer, a pending chord, an open menu or a live search would + // keep it off across the switch, so this failing is the signal that + // the assertion above passed for the wrong reason. + let listing = active_buffer_id(&s); + exec( + &s, + "DIRED_LISTING = pmacs.window.buffer()\n\ + pmacs.window.switch_buffer(pmacs.buffer.create('*plain*'))", + ); + assert!( + s.dispatch_idle_for(FrontendId::LOCAL), + "and back ON for a plain buffer" + ); + exec(&s, "pmacs.window.switch_buffer(DIRED_LISTING)"); + + // The listing's rope is genuinely locked, not merely intercepted + // (generated-buffer immutability Stage 1). Asserted Rust-side + // because `describe.buffer` carries no `read_only` field. + assert!( + is_read_only(&s, listing), + "the first paint must leave the listing's rope read-only" + ); + // `z` is bound nowhere in dired mode, so it reaches self-insert. type_char(&mut s, 'z'); assert_eq!( @@ -1654,3 +1723,274 @@ fn dired_renders_10k_entries_within_200ms() { "10K entries must render within 200ms; took {elapsed:?}" ); } + +// --------------------------------------------------------------------------- +// Generated-buffer immutability, Stage 1 +// (docs/generated-buffer-immutability-framing.md §6, Stage 1) +// --------------------------------------------------------------------------- + +/// Criterion 3 [`main`] --- neither `C-/` nor `M-x buffer.undo` can empty +/// a dired listing. +/// +/// Both halves in one test because they are one claim about one buffer, +/// and both are needed: dired rebinds **no** undo chord, so `C-/` is the +/// whole distance from a keystroke to an empty listing, while +/// `M-x buffer.undo` is the half that no rebinding could ever close. +/// The assertion is on the listing's own content --- the header line and +/// a real entry --- not on `!is_empty()`. +/// +/// *Bite:* measured on the pre-image --- one undo takes the listing to +/// `""`. `scripts/bite githubsucks/main builtin/runtime/dired.lua` +/// falsifies it: `paint`'s `bypass_intercept` replace pushed a poppable +/// undo entry over a writable rope, and `Buffer::undo` reaches that rope +/// through `ensure_writable` without ever consulting the intercept chain. +#[test] +fn dired_undo_cannot_empty_the_listing() { + let td = fixture_dir(); + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let before = active_text(&s); + let name = active_name(&s); + assert!( + before.contains("a.txt") && before.contains(&canon(td.path())), + "precondition: a real listing, got {before:?}" + ); + + ctrl(&mut s, '/'); + assert_eq!( + active_text(&s), + before, + "C-/ must leave the listing's content intact" + ); + + m_x(&mut s, "buffer.undo"); + assert_eq!( + active_name(&s), + name, + "the minibuffer round trip lands back in the listing" + ); + assert_eq!( + active_text(&s), + before, + "M-x buffer.undo must leave the listing's content intact" + ); +} + +/// Criterion 4 [fix-shape] --- the owner's own repaint still works after +/// the lock, and the listing is still locked afterwards. +/// +/// *Bite:* a bare `set_read_only(true)` would pass criterion 3 and fail +/// here, because it refuses the refresh the buffer exists for; the +/// falsifying one-line mutation on the shipped primitive is deleting +/// `self.read_only = false` from `Buffer::set_generated_contents` +/// (`src/buffer.rs:546`), after which `g` raises. Asserted on the content +/// the repaint produced, never on the absence of an error. +#[test] +fn dired_revert_still_repaints_after_the_lock() { + let td = fixture_dir(); + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let listing = active_buffer_id(&s); + assert!( + is_read_only(&s, listing), + "precondition: the first paint locked the rope" + ); + assert!(!active_text(&s).contains("c.txt")); + + std::fs::write(td.path().join("c.txt"), b"new\n").expect("write c"); + type_char(&mut s, 'g'); + pump(&mut s); + + assert!( + active_text(&s).contains("c.txt"), + "the owner's repaint must land through the lock: {:?}", + active_text(&s) + ); + assert!( + is_read_only(&s, listing), + "and leave the listing locked afterwards" + ); +} + +/// Criterion 5 [fix-shape] --- the named intercept survives adoption and +/// is still what refuses an edit whenever the rope does not. +/// +/// **Restated against the framing**, which asked for an ordinary edit +/// "refused by the INTERCEPT, not by the rope" and asserted on the +/// message text. That state is unreachable once the arc's lock is +/// installed: `Buffer::apply_edit` (`src/buffer.rs:773`) and +/// `Buffer::begin_edit` (`:725`) call `ensure_writable()` as their FIRST +/// statement, while the intercept chain runs later inside +/// `apply_edit_inner` (`:1072`), so the rope always answers first. The +/// criterion is therefore driven with the lock lifted --- the state the +/// intercept genuinely still covers, including the window between +/// `pmacs.buffer.create` and the first paint. +/// +/// *Bite:* unchanged --- an adopter that drops `add_intercept` and relies +/// on the rope alone passes criteria 3 and 4 and fails here. +#[test] +fn dired_keeps_the_named_intercept_beside_the_rope_lock() { + let td = fixture_dir(); + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let listing = active_buffer_id(&s); + let before = active_text(&s); + + // With the lock on, the ROPE answers first, and its message is the + // one with the buffer id in it. Pinned so the restatement above + // cannot rot silently. + type_char(&mut s, 'z'); + assert!( + status(&s).contains("(id BufferId("), + "with the lock on the rope refuses first; got {:?}", + status(&s) + ); + + set_read_only(&s, listing, false); + type_char(&mut s, 'z'); + set_read_only(&s, listing, true); + + assert_eq!( + active_text(&s), + before, + "the intercept must refuse the edit even with the rope writable" + ); + let st = status(&s); + assert!( + st.contains("dired.lua") && st.contains("is read-only"), + "and refuse it by NAME, not with the rope's message; got {st:?}" + ); +} + +/// Criterion 7 [mutation] --- a repaint reaches the **window**, not just +/// the rope, pinned by painting a shrinking listing. +/// +/// A rope write is only half of an edit: the window holds a `TextView` +/// line index that only `on_edit` maintains, so a write that reaches the +/// rope without the fan-out leaves the two disagreeing, and the next +/// paint indexes the new rope with the old offsets. `dired.revert` is +/// the right driver because it paints and does **not** follow the paint +/// with a `window.switch_buffer` --- which rebuilds the `TextView` from +/// scratch and would mask the mutation. (`listview.refresh` and +/// `listview.open` both do switch, so the listview half of this +/// criterion cannot bite; the primitive's own pin is +/// `terminal_copy_mode_acceptance::acc16d`.) +/// +/// *Bite:* delete the `notify_buffer_edit_to_windows` call in the +/// `set_generated_contents` binding (`src/lua_bindings/mod.rs:3092`) and +/// the painted frame keeps rows the listing no longer has. +#[test] +fn dired_a_shrinking_repaint_reaches_the_window() { + let td = tempfile::tempdir().expect("tempdir"); + for name in ["a.txt", "b.txt", "c.txt", "d.txt", "e.txt"] { + std::fs::write(td.path().join(name), b"x").expect("write"); + } + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let rows = painted_rows(&s); + assert!( + rows[5].contains("e.txt"), + "precondition: five entries paint on rows 1-5, got {:?}", + &rows[..7] + ); + + for name in ["b.txt", "c.txt", "d.txt", "e.txt"] { + std::fs::remove_file(td.path().join(name)).expect("remove"); + } + type_char(&mut s, 'g'); + pump(&mut s); + + let rows = painted_rows(&s); + assert!( + rows[1].contains("a.txt"), + "the one surviving entry paints: {:?}", + &rows[..7] + ); + assert_eq!( + rows[2], + "", + "and nothing of the four rows it replaced: {:?}", + &rows[..7] + ); +} + +/// Criterion 13a [`main`] --- a locked generated buffer is not foldable +/// (Q#GB16, option (a)). +/// +/// This is a regression pin **for the intended change**: sweep C found +/// that `document_bytes` (`src/lua_bindings/fold.rs`) is spelled +/// `if buffer.is_read_only() { return Ok(None) }`, so locking dired +/// listings silently disables fold *creation* on them. The decision is to +/// accept that --- a generated buffer's contents are replaced wholesale +/// on every refresh, which invalidates any stored range --- and to state +/// it rather than let it ship silently. +/// +/// *Bite:* this **fails on `main`**, where the same call returns `true`. +/// `scripts/bite githubsucks/main builtin/runtime/dired.lua` falsifies +/// it. +#[test] +fn dired_a_locked_listing_is_not_foldable() { + let td = fixture_dir(); + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let text = active_text(&s); + let first_nl = text.find('\n').expect("header line"); + let second_nl = text[first_nl + 1..] + .find('\n') + .map(|i| first_nl + 1 + i) + .expect("at least two entry lines"); + + let folded: bool = eval( + &s, + &format!( + "return pmacs.fold.fold(pmacs.window.buffer(), \ + {{ start = {first_nl}, ['end'] = {second_nl} }})" + ), + ); + + assert!(!folded, "a locked generated buffer is not foldable"); + let n: i64 = eval(&s, "return #pmacs.fold.folds(pmacs.window.buffer())"); + assert_eq!(n, 0, "and no fold is stored"); +} + +/// Criterion 13b [mutation] --- ...and the refusal says why. +/// +/// Separate from 13a because the two halves have different pre-images: +/// on `main` the call *succeeds* and sets no status at all, so this +/// cannot share 13a's `main` pre-image. The shape that ships if 13a is +/// written alone is correct behaviour with a false explanation --- the +/// guard's author meant "terminal", and "not a document buffer" is not +/// true of a dired listing. +/// +/// *Bite:* revert `src/lua_bindings/fold.rs`'s status string to +/// `"fold rejected: not a document buffer"`. +#[test] +fn dired_the_fold_refusal_names_the_read_only_lock() { + let td = fixture_dir(); + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let text = active_text(&s); + let first_nl = text.find('\n').expect("header line"); + let second_nl = text[first_nl + 1..] + .find('\n') + .map(|i| first_nl + 1 + i) + .expect("at least two entry lines"); + + let _: bool = eval( + &s, + &format!( + "return pmacs.fold.fold(pmacs.window.buffer(), \ + {{ start = {first_nl}, ['end'] = {second_nl} }})" + ), + ); + + let st = status(&s); + assert!( + st.contains("read-only"), + "the refusal must name the lock; got {st:?}" + ); + assert!( + !st.contains("not a document buffer"), + "and not the sentence that is no longer true; got {st:?}" + ); +} diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 9bb0a45..5740081 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -7,8 +7,15 @@ //! substrate hermetically. //! //! Framing: docs/lsp-panels-framing.md. +//! +//! Generated-buffer immutability Stage 1 +//! (docs/generated-buffer-immutability-framing.md §6) adds the +//! criteria below `refresh_reruns_the_source_and_reseats`: the undo +//! paths the Q#P3 intercept never guarded, the Q#GB13 ownership rule, +//! and the Q#GB18 identity routing that ownership makes load-bearing. use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; +use pmacs::buffer::BufferId; use pmacs::editor::EditorState; use pmacs::protocol::FrontendId; @@ -25,6 +32,97 @@ fn press(s: &mut EditorState, code: KeyCode) { s.dispatch_key(FrontendId::LOCAL, key(code, KeyModifiers::NONE)); } +fn ctrl(s: &mut EditorState, c: char) { + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char(c), KeyModifiers::CONTROL), + ); +} + +fn alt(s: &mut EditorState, c: char) { + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Char(c), KeyModifiers::ALT)); +} + +fn type_str(s: &mut EditorState, text: &str) { + for ch in text.chars() { + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char(ch), KeyModifiers::NONE), + ); + } +} + +/// `M-x RET` through the **real** minibuffer, not +/// `pmacs.command.invoke`: `buffer.undo` is reachable that way on every +/// buffer in the tree and no buffer-local rebinding can remove it, which +/// is the whole reason the intercept idiom did not close this hole. +fn m_x(s: &mut EditorState, name: &str) { + alt(s, 'x'); + type_str(s, name); + press(s, KeyCode::Enter); +} + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +fn status(s: &EditorState) -> String { + s.core.borrow().status.clone() +} + +fn buffer_names(s: &EditorState) -> Vec { + eval( + s, + "local out = {}\n\ + for _, id in ipairs(pmacs.buffer.list()) do\n\ + out[#out + 1] = pmacs.describe.buffer(id).name\n\ + end\n\ + return out", + ) +} + +fn active_name(s: &EditorState) -> String { + eval( + s, + "return pmacs.describe.buffer(pmacs.window.buffer()).name", + ) +} + +fn active_text(s: &EditorState) -> String { + eval( + s, + "local b = pmacs.window.buffer()\nreturn b:slice(0, b:len())", + ) +} + +fn id_of(s: &EditorState, name: &str) -> BufferId { + let core = s.core.borrow(); + let reg = core.registry.borrow(); + reg.find_by_name(name) + .unwrap_or_else(|| panic!("no buffer named {name:?} in {:?}", buffer_names(s))) +} + +/// Q#GB14: the rope lock is not observable from Lua --- `describe.buffer` +/// carries `name`, `length`, `modified`, `view_count` and nothing else --- +/// so every "is it locked" assertion goes through Rust. +fn is_read_only(s: &EditorState, id: BufferId) -> bool { + let core = s.core.borrow(); + let reg = core.registry.borrow(); + reg.get(id).expect("buffer in registry").is_read_only() +} + +fn set_read_only(s: &EditorState, id: BufferId, value: bool) { + let core = s.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(id) + .expect("buffer in registry") + .set_read_only(value); +} + /// Open a three-row test panel whose visits record into `_G.VISITED`. fn open_test_panel(s: &mut EditorState) { s.lua_host @@ -136,3 +234,431 @@ fn refresh_reruns_the_source_and_reseats() { let (_, _, _, visited) = probe(&s); assert_eq!(visited.as_deref(), Some("D"), "the refreshed row visits"); } + +// --------------------------------------------------------------------------- +// Generated-buffer immutability, Stage 1 +// (docs/generated-buffer-immutability-framing.md §6, Stage 1) +// --------------------------------------------------------------------------- + +/// The exact bytes `open_test_panel` renders. Asserted by value, not by +/// `is_empty()`: "the panel is not empty" is the assertion shape the +/// framing's §0.1 shows passing with the bug live on other families. +const PANEL_TEXT: &str = "3 items RET visit q quit\nalpha\nbeta\ngamma"; + +/// Criterion 1 [`main`] --- `C-/` cannot empty a listview panel. +/// +/// Driven by a real chord through `dispatch_key`, because listview +/// rebinds **no** undo chord (`grep -n 'C-/\|C-_\|C-x u\|undo' +/// builtin/runtime/listview.lua` is empty), so this is the whole +/// distance from a keystroke to an empty panel. +/// +/// *Bite:* measured on the pre-image --- `"H\nrow-one\nrow-two"` -> `""`. +/// `scripts/bite githubsucks/main builtin/runtime/listview.lua` falsifies +/// it: the panel's own render pushes a poppable undo entry, and +/// `Buffer::undo` reaches the rope through `ensure_writable` without ever +/// consulting the intercept chain. +#[test] +fn s1_1_the_undo_chord_cannot_empty_a_listview_panel() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + assert_eq!(active_text(&s), PANEL_TEXT, "precondition: rendered"); + + ctrl(&mut s, '/'); + + assert_eq!( + active_text(&s), + PANEL_TEXT, + "C-/ must leave the panel's content intact" + ); +} + +/// Criterion 2 [`main`] --- `M-x buffer.undo` cannot empty a listview +/// panel, driven through the **real** minibuffer. +/// +/// Separate from criterion 1 on purpose: a fix that only rebound the +/// chords would pass 1 and fail this. `compile.lua`'s own comment already +/// concedes the point ("command/menu undo stays dispatchable"). +/// +/// *Bite:* same empty result on the pre-image. +#[test] +fn s1_2_m_x_buffer_undo_cannot_empty_a_listview_panel() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + + m_x(&mut s, "buffer.undo"); + + assert_eq!( + active_name(&s), + "*test-panel*", + "the minibuffer round trip must land back in the panel" + ); + assert_eq!( + active_text(&s), + PANEL_TEXT, + "M-x buffer.undo must leave the panel's content intact" + ); +} + +/// Criterion 4 [fix-shape] --- the owner's own refresh still works after +/// the lock, and the buffer is still locked afterwards. +/// +/// *Bite:* a naive `set_read_only(true)` at panel creation passes +/// criteria 1-3 and fails here, because it refuses the refresh the panel +/// exists for. That is the failure mode `Buffer::set_generated_contents` +/// exists to prevent, and it is why the **pairing** is the primitive. +/// The assertion is on the content `g` produced, not on the call not +/// raising. +#[test] +fn s1_4_the_owners_refresh_still_works_after_the_lock() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + let panel = id_of(&s, "*test-panel*"); + assert!( + is_read_only(&s, panel), + "precondition: the first render locked the rope" + ); + + press(&mut s, KeyCode::Char('g')); + + let text = active_text(&s); + assert!(text.contains("delta"), "g must re-render: {text:?}"); + assert!( + !text.contains("alpha"), + "and replace the old rows: {text:?}" + ); + assert!( + is_read_only(&s, panel), + "and the panel must still be locked afterwards" + ); +} + +/// Criterion 5 [fix-shape] --- the named intercept survives adoption and +/// is still the thing that refuses an edit whenever the rope does not. +/// +/// **Restated against the framing, which specified this criterion in a +/// form the tree cannot reach.** §6 Stage 1 criterion 5 asks for an +/// ordinary edit refused "by the INTERCEPT, not by the rope", asserted on +/// the message text. Once the arc's own lock is installed that state is +/// unreachable: `Buffer::apply_edit` (`src/buffer.rs:773`) and +/// `Buffer::begin_edit` (`:725`) both call `ensure_writable()` as their +/// FIRST statement, while the intercept chain runs later, inside +/// `apply_edit_inner` (`:1072`). Measured here: a self-insert on an +/// adopted panel now reports +/// ``insert failed: buffer `*test-panel*` (id BufferId(n)) is read-only`` +/// --- the rope's message --- and can never report the intercept's. +/// +/// So the criterion is driven at the one point where the two are +/// distinguishable, which is also the state it is actually protecting: +/// the rope lock lifted. That covers the real window between +/// `pmacs.buffer.create` and the first render, and any future Rust-side +/// lift. +/// +/// *Bite:* unchanged from the framing's --- an adopter that deletes the +/// `add_intercept` call and relies on the rope alone passes criteria 1-4 +/// and fails here, because with the lock lifted the `z` lands. +#[test] +fn s1_5_an_ordinary_edit_is_refused_by_the_named_intercept_not_only_the_rope() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + let panel = id_of(&s, "*test-panel*"); + + // With the lock ON, the rope answers first and the message is its + // own. Pinned so the restatement above cannot rot silently. + press(&mut s, KeyCode::Char('z')); + assert!( + status(&s).contains("(id BufferId("), + "with the lock on, the ROPE refuses first; got {:?}", + status(&s) + ); + + set_read_only(&s, panel, false); + press(&mut s, KeyCode::Char('z')); + set_read_only(&s, panel, true); + + assert_eq!( + active_text(&s), + PANEL_TEXT, + "the intercept must refuse the edit even with the rope writable" + ); + let st = status(&s); + assert!( + st.contains("listview.lua") && st.contains("*test-panel* is read-only"), + "and refuse it by NAME, not with the rope's message; got {st:?}" + ); +} + +/// Criterion 6 [fix-shape] --- `set_round_trip_input` survives adoption, +/// asserted so that only the round-trip mark can make it pass. +/// +/// `dispatch_idle_for` (`src/editor.rs:1126-1155`) returns `false` for +/// **six** independent reasons, so `!dispatch_idle_for(..)` alone is +/// satisfied by any of them. All three halves are required: +/// +/// * **(a)** the document-window premise (`!window.is_side()`), so a +/// fixture that later displays the panel in a side window fails loudly +/// rather than passing vacuously --- `tests/dired_acceptance.rs:975`'s +/// shape; +/// * **(b)** the gate itself while the panel is focused; +/// * **(c)** the positive control --- switching the same window to a +/// plain buffer must flip the gate back to `true`. A stuck minibuffer, +/// a pending chord, an open menu or a live search would keep it `false` +/// across the switch, so (c) failing is the signal that (b) passed for +/// the wrong reason. +/// +/// *Bite:* delete the `set_round_trip_input` call in `listview.lua` and +/// criteria 1-5 all still pass; only (b) fails. A daemon-side rope +/// refusal does nothing for a replica's own mirror, which is why this is +/// pinned through `dispatch_idle_for` rather than through `read_only`. +#[test] +fn s1_6_round_trip_input_survives_the_adoption() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + + // (a) the premise. + { + let core = s.core.borrow(); + let active = core.active_window_id(); + assert!( + !core.windows.get(&active).expect("live window").is_side(), + "fixture premise: the panel is in a document window here" + ); + } + // (b) the gate. + assert!( + !s.dispatch_idle_for(FrontendId::LOCAL), + "a round-trip buffer must turn optimistic apply OFF" + ); + // (c) the positive control. + exec( + &s, + "pmacs.window.switch_buffer(pmacs.buffer.create('*plain*'))", + ); + assert!( + s.dispatch_idle_for(FrontendId::LOCAL), + "and back ON for a plain buffer --- otherwise (b) passed for one \ + of the other five reasons" + ); +} + +/// Criterion 9 [`main`] --- a foreign buffer that happens to share the +/// panel's name is never adopted (Q#GB13). +/// +/// Both halves, because the second is what fails if adoption is merely +/// made "safe" by skipping the render: the user's bytes survive **and** +/// an ordinary edit to the user's buffer still lands. Adoption installed +/// an erroring intercept whose handle it discarded, so the pre-image left +/// the clobbered buffer permanently un-editable --- and this arc removes +/// the `M-x buffer.undo` that was the only way back. +/// +/// *Bite:* measured on the pre-image --- `"my precious notes"` -> +/// `"H\nr1"`, one buffer where there should be two, and the user's buffer +/// left un-editable. `scripts/bite githubsucks/main +/// builtin/runtime/listview.lua` falsifies it. +#[test] +fn s1_9_a_foreign_buffer_with_the_panels_name_is_never_adopted() { + let mut s = EditorState::new(); + exec( + &s, + "FOREIGN = pmacs.buffer.create('*test-panel*')\n\ + FOREIGN:insert(0, 'my precious notes')", + ); + + open_test_panel(&mut s); + + let foreign: String = eval(&s, "return FOREIGN:slice(0, FOREIGN:len())"); + assert_eq!( + foreign, "my precious notes", + "the user's bytes must survive the panel opening" + ); + let landed: String = eval( + &s, + "FOREIGN:insert(0, 'still mine: ')\n\ + return FOREIGN:slice(0, FOREIGN:len())", + ); + assert_eq!( + landed, "still mine: my precious notes", + "and an ordinary edit to it must still land" + ); + assert_eq!( + active_name(&s), + "*test-panel*<2>", + "the panel opens under a disambiguated name" + ); + let names = buffer_names(&s); + assert!( + names.iter().any(|n| n == "*test-panel*") && names.iter().any(|n| n == "*test-panel*<2>"), + "two buffers, not one: {names:?}" + ); +} + +/// Criterion 10 [fix-shape] --- exhausting the disambiguation limit +/// raises rather than falling back to adoption, matching +/// `dired.lua:493-503` and `terminal.lua:309-315`. +/// +/// *Bite:* an implementation that adopts once `<99>` is taken passes +/// criterion 9 and fails here --- and it fails in the worst direction, +/// because the buffer it would adopt is by construction one a user +/// created. +#[test] +fn s1_10_the_disambiguation_limit_raises_rather_than_adopting() { + let s = EditorState::new(); + exec( + &s, + "MINE = pmacs.buffer.create('*test-panel*')\n\ + MINE:insert(0, 'mine')\n\ + for i = 2, 99 do pmacs.buffer.create(string.format('*test-panel*<%d>', i)) end", + ); + + let (ok, err): (bool, String) = eval( + &s, + "local ok, err = pcall(pmacs.listview.open, { name = '*test-panel*', rows = {} })\n\ + return ok, tostring(err)", + ); + + assert!(!ok, "the open must raise, not adopt"); + assert!( + err.contains("no free variant remains"), + "and say why; got {err:?}" + ); + let mine: String = eval(&s, "return MINE:slice(0, MINE:len())"); + assert_eq!(mine, "mine", "and touch nothing"); +} + +/// Criterion 11 [`main`] --- a **disambiguated** panel still answers +/// `RET`, `g` and `q` (Q#GB18). +/// +/// This is the criterion that fails against Q#GB13 landed without +/// Q#GB18: disambiguation alone leaves the old lookup reading +/// `panels["*test-panel*<2>"]` for a record stored under +/// `"*test-panel*"`, so all three commands return early. Every one of +/// them fails **silently**, so the assertion is on the content each +/// command produced, never on "it did not raise". +#[test] +fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { + let mut s = EditorState::new(); + exec( + &s, + "FOREIGN = pmacs.buffer.create('*test-panel*')\n\ + ORIGIN = pmacs.buffer.create('*origin*')\n\ + pmacs.window.switch_buffer(ORIGIN)", + ); + open_test_panel(&mut s); + assert_eq!(active_name(&s), "*test-panel*<2>", "premise: disambiguated"); + + // g --- re-render from the data source. + press(&mut s, KeyCode::Char('g')); + let text = active_text(&s); + assert!(text.contains("delta"), "g must re-render: {text:?}"); + + // RET --- fire on_visit for the row under the cursor. + press(&mut s, KeyCode::Enter); + let visited: Option = eval(&s, "return _G.VISITED"); + assert_eq!( + visited.as_deref(), + Some("D"), + "RET must visit the refreshed row" + ); + + // q --- restore the buffer the panel was opened from. + press(&mut s, KeyCode::Char('q')); + assert_eq!( + active_name(&s), + "*origin*", + "q must restore the previous buffer" + ); +} + +/// Criterion 12 [`main`] --- the `q`-target capture is not inverted +/// (Q#GB18), which needs its own criterion because it fails **open** +/// rather than closed. +/// +/// `listview.open`'s guard reads "capture the current buffer as the `q` +/// target, but never another panel (chained panels would trap `q` in a +/// loop)". When the lookup cannot recognise a disambiguated panel it +/// returns nil, the guard reads "not a panel", and the panel is captured +/// as the next panel's `q` target --- producing exactly the loop the +/// guard exists to prevent. Criterion 11 passes with that bug live, +/// because each command works in isolation; only the two-panel sequence +/// shows it. +/// +/// *Bite:* restore the name-keyed `panels[d.name]` lookup while keeping +/// the disambiguation and `q` lands back in `*test-panel*<2>`. +#[test] +fn s1_12_the_q_target_capture_is_not_inverted_across_two_panels() { + let mut s = EditorState::new(); + exec( + &s, + "FOREIGN = pmacs.buffer.create('*test-panel*')\n\ + ORIGIN = pmacs.buffer.create('*origin*')\n\ + pmacs.window.switch_buffer(ORIGIN)", + ); + open_test_panel(&mut s); + assert_eq!(active_name(&s), "*test-panel*<2>", "premise: disambiguated"); + + exec( + &s, + "pmacs.listview.open { name = '*other-panel*', header = 'O', \ + rows = { { text = 'x', item = 'X' } } }", + ); + assert_eq!(active_name(&s), "*other-panel*", "premise: second panel"); + + press(&mut s, KeyCode::Char('q')); + + assert_ne!( + active_name(&s), + "*test-panel*<2>", + "q must never return into another panel --- the chained-panel loop" + ); + assert_eq!( + active_name(&s), + "*scratch*", + "with no capturable previous buffer, q falls back to *scratch*" + ); +} + +/// Criterion 14 [structural] --- rides **alongside** 1-13, never instead: +/// a structural comparison of two authorities does not catch a misrouted +/// consumer, which is why 11 and 12 assert through `dispatch_key`. +/// +/// Three claims, each keyed to a decision: no `bypass_intercept` write +/// survives in either Stage 1 adopter (§1.1's arithmetic is the +/// reference, so the check is per non-comment line rather than a +/// substring sweep that the explanatory comments would trip); +/// `ensure_panel` contains no find-by-name adoption (Q#GB13); and every +/// `panels[` subscript is an append, so none can be keyed by a name +/// derived from `describe.buffer` (Q#GB18). +#[test] +fn s1_14_no_bypass_write_or_name_keyed_identity_remains() { + const LISTVIEW: &str = include_str!("../builtin/runtime/listview.lua"); + const DIRED: &str = include_str!("../builtin/runtime/dired.lua"); + + for (file, src) in [("listview.lua", LISTVIEW), ("dired.lua", DIRED)] { + let writes: Vec<&str> = src + .lines() + .filter(|l| !l.trim_start().starts_with("--") && l.contains("bypass_intercept")) + .collect(); + assert!( + writes.is_empty(), + "{file} must contain no bypass_intercept write; found {writes:?}" + ); + assert!( + src.contains("set_generated_contents"), + "{file} must write through the authorized primitive" + ); + } + + assert!( + !LISTVIEW.contains("find_buffer_by_name(name) or pmacs.buffer.create"), + "ensure_panel must not adopt a same-named foreign buffer" + ); + let subscripts: Vec<&str> = LISTVIEW + .lines() + .map(str::trim) + .filter(|line| !line.starts_with("--") && line.contains("panels[")) + .filter(|line| !line.starts_with("panels[#panels + 1]")) + .collect(); + assert!( + subscripts.is_empty(), + "every `panels[` subscript must be an append; found {subscripts:?}" + ); +} diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index ed3f6d7..c572166 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -992,3 +992,119 @@ fn copy_mode_refuses_a_non_terminal_buffer() { "the refusal must say why: {err}" ); } + +/// Generated-buffer immutability Stage 1, criterion 8 [`main`] — Q#GB6's +/// cursor clamp. +/// +/// `EditorCore::notify_buffer_edit` — the fan-out every generated write +/// goes through — updated each window's `TextView` and overlays but +/// clamped neither window coordinate; only `rebuild_views_for` did, and +/// its doc comment said so. So a shrinking generated refresh left +/// `win.cursor` past the end of the rope **indefinitely**: paint does not +/// crash, and a motion command does not recover it, because motion is +/// computed from the stale value. +/// +/// *Bite:* measured on the pre-image — cursor 29, len 2, and `C-p` leaves +/// it at 29. This ships today for terminal copy mode: refresh a snapshot +/// to a shorter one with the point low in the buffer and this is the +/// state. Falsify by deleting the `win.cursor > len` clamp. +#[test] +fn acc16f_a_shrinking_generated_write_clamps_the_window_cursor() { + let 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) + pmacs.editor.goto_byte(30) + ", + ); + let (cursor, len): (i64, i64) = eval( + &state, + "return pmacs.editor.cursor(), pmacs.window.buffer():len()", + ); + assert_eq!( + (cursor, len), + (30, 31), + "precondition: point low in a 31-byte buffer" + ); + + exec(&state, r"pmacs.buffer.set_generated_contents(GEN, 'x\n')"); + + let (cursor, len): (i64, i64) = eval( + &state, + "return pmacs.editor.cursor(), pmacs.window.buffer():len()", + ); + assert_eq!(len, 2, "precondition: the buffer shrank"); + assert!( + cursor <= len, + "the cursor must be clamped into the new rope; got {cursor} for len {len}" + ); + + // And motion works from there: `C-p` reaches line 0, which it cannot + // do from a dangling offset. + exec(&state, "pmacs.editor.move_up()"); + let cursor: i64 = eval(&state, "return pmacs.editor.cursor()"); + assert_eq!(cursor, 0, "C-p must move to the first line"); +} + +/// Generated-buffer immutability Stage 1, criterion 8b [`main`] — Q#GB6's +/// `view_top` clamp, on a buffer that GREW. +/// +/// The two coordinates fail on different axes: `cursor` is a byte +/// position bounded by `Buffer::len`, while `view_top` is a **line +/// index** bounded by `TextView::line_count`. A replace can grow in bytes +/// while collapsing many lines into one, so a clamp gated on "the buffer +/// shrank" passes criterion 8 and fails here — which is why the clamp +/// runs unconditionally, each coordinate against its own bound, exactly +/// as `rebuild_views_for` already does. +/// +/// *Bite:* falsify by gating the clamp on a byte-length comparison, or by +/// deleting the `view_top` half. Unlike criterion 8 this case is argued +/// from the types rather than measured on `main`; the assertion below is +/// the measurement. +#[test] +fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { + let state = EditorState::new(); + exec( + &state, + r" + GEN = pmacs.buffer.create('*viewtop-probe*') + pmacs.buffer.set_generated_contents(GEN, 'a\nb\nc\nd\ne\nf\n') + pmacs.window.switch_buffer(GEN) + pmacs.editor.set_view_top(5) + ", + ); + let (top, len): (i64, i64) = eval( + &state, + "return pmacs.editor.view_top(), pmacs.window.buffer():len()", + ); + assert_eq!( + (top, len), + (5, 12), + "precondition: scrolled to line 5 of a 12-byte, 7-line buffer" + ); + + // 20 bytes on ONE line: longer than what it replaces, so any + // "the buffer shrank" trigger is false here. + exec( + &state, + r"pmacs.buffer.set_generated_contents(GEN, '0123456789abcdefghij')", + ); + + let len: i64 = eval(&state, "return pmacs.window.buffer():len()"); + assert_eq!(len, 20, "precondition: the buffer GREW in bytes"); + + let (top, lines) = { + let core = state.core.borrow(); + let active = core.active_window_id(); + let win = core.windows.get(&active).expect("active window"); + (win.view_top, win.text_view.line_count()) + }; + assert!( + top < lines, + "view_top must be clamped into the new line count; got {top} of {lines}" + ); + assert_eq!(top, 0, "the collapsed buffer has exactly one line"); +} From e64bebc9c103043af531ecaecf5460b7c6adc7e7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 09:59:00 -0400 Subject: [PATCH 2/7] docs(active-work): the generated-buffer immutability Stage 1 lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An open PR is exactly the volatile work this file records, so it gets a lane the moment it is opened — and the lane rides on its own branch, not on `main`, because with several PRs open a lane written there re-conflicts on every merge. Records the branch and its base (`githubsucks/main` @ `300cbc4`), the code checkpoint the verification numbers describe, what Stage 1 ships, what Stage 2 still owes, the two framing criteria that turned out to be unimplementable as written and what replaced them, the bite result for every criterion, and the recovery commands. The canonical-base line above is deliberately left at `7586905`: this lane names its own base, which is the case that paragraph already covers ("lanes below that name an older base have not been re-based"), and editing the shared snapshot line while other PRs are open is the contention this file warns about. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 89 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 2dfbaf0..258f408 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -245,6 +245,95 @@ If it does not, stop and repair the remote/fetch configuration. never been enforced. Any CI job that compiles the `crdt` targets has to fix them first or it will be red on arrival. +## Generated-buffer immutability lane (Arc: workbench primitives) — STAGE 1 OPEN + +**Framing: [PR #188](https://github.com/levineuwirth/pmacs/pull/188), +revision 5, APPROVED and still open.** Read it as +`git show githubsucks/generated-buffer-immutability:docs/generated-buffer-immutability-framing.md` +until it merges. Stage 1 is branched from **landed `main`**, not stacked +on the framing branch, so the two merge in either order. + +- **Branch `generated-buffer-immutability-stage1`, based on + `githubsucks/main` @ `300cbc4`.** Worktree + `../pmacs-gbi-stage1`. Code checkpoint `e1b859f`; this ledger commit + rides on top of it. +- **What Stage 1 ships.** `dired.lua`'s `paint` and `listview.lua`'s + `render` write through `pmacs.buffer.set_generated_contents` (zero + `bypass_intercept` writes remain in either file); `listview` gains + Q#GB13 ownership-by-handle with `<2>`..`<99>` disambiguation and + Q#GB18's identity-routed `panels` list in the **same** commit; + Q#GB6's per-coordinate window clamp in + `EditorCore::notify_buffer_edit`; and Q#GB16(a)'s corrected fold + status string. No protocol change, no new Lua surface, no new + interaction island. +- **Why these two families first, and it is not "the cheap half".** + `compile.lua:219` and `builtin/commands/default.lua:855` rebind all + seven undo chords to a no-op; `dired.lua` and `listview.lua` rebind + **nothing**, so a bare `C-/` emptied a listing and a panel. Stage 1 + closes the only two families reachable without `M-x`. +- **Two framing criteria were wrong and the tests say so rather than + working around them.** + - Stage 1 criterion 5 ("an ordinary edit is refused by the INTERCEPT, + not by the rope") is **unreachable** once this arc's lock exists. + `Buffer::apply_edit` (`src/buffer.rs:773`) and `begin_edit` + (`:725`) call `ensure_writable` as their FIRST statement, while the + intercept chain runs later inside `apply_edit_inner` (`:1072`), so + the rope always answers first. Measured: a self-insert on an + adopted panel reports ``insert failed: buffer `*test-panel*` (id + BufferId(n)) is read-only``. Restated in both suites as "the + intercept still refuses with its named error **when the rope is + lifted**", which keeps the framing's own bite (delete + `add_intercept`) and is the state the intercept genuinely covers. + - Criterion 7 ("a refresh reaches the window") **cannot bite at the + listview adopter**: `listview.refresh` and `listview.open` both + follow `render` with `window.switch_buffer`, which rebuilds the + `TextView` from scratch (`src/editor_core.rs:4859-4868`) and masks + a dropped fan-out. `dired.revert` and `dired.sort-cycle` paint + without a switch, so the dired half carries it and fails the + mutation with the reported `assertion failed: end <= self.len()`. +- **Stage 2 still owes everything with new Rust in it**, per the + framing's cut: `Buffer::apply_generated_edit` + `GeneratedOutcome` + + the `{ generated = true }` option + its own `run_buffer_edit` arm; + `set_generated_contents` reimplemented over it; Q#GB10's path-backed + refusal and `mark_clean`; Q#GB15's `identity_protected`; Q#GB13/GB18 + for `compile.lua` and the search panel; Q#GB5's `ensure_slot` lock; + conversion of the remaining 13 write sites; and the three + `compile_mode_acceptance` intruder tests converted per Q#GB12. +- **Verification at code checkpoint `e1b859f`.** `cargo fmt --check`; + `cargo clippy --workspace --all-targets -- -D warnings`; library + **1,863 passed + 3 ignored** default and **2,048 passed + 4 ignored** + CRDT; `listview_acceptance` **16**, `dired_acceptance` **31**, + `folding_acceptance` **21**, `terminal_copy_mode_acceptance` **16** + default and **17** with `--features crdt` (the extra one is + `acc16e`, which a default run never compiles — judge that step by the + count, not the verdict); M4 **121 passed + 3 ignored + 1 filtered** + with `--skip basedpyright`; required GPU **202/202**; isolated-config + full workspace sweep **3,511 passed across 103 binaries, exit 0**; + `git diff --check` clean. +- **Bites, all executed.** Five criteria are falsified by revert against + `githubsucks/main` (`scripts/bite` on `builtin/runtime/listview.lua` + and `builtin/runtime/dired.lua`): the two undo criteria, the + no-adoption criterion, the disambiguated-panel criterion, and the + fold-refusal pair. Nine more are falsified by a named one-line + mutation, each run and each observed to fail: dropping the fan-out in + the `set_generated_contents` binding; deleting the cursor clamp; + gating the `view_top` clamp on "the buffer shrank"; deleting + `self.read_only = false` from `set_generated_contents`; deleting + `add_intercept` and `set_round_trip_input` at each adopter; restoring + a name-keyed `panel_for_buffer`; adopting at the variant limit; and + restoring the old fold status string. **The `view_top` and `cursor` + clamps each fail only their own criterion**, which is the + discrimination review round 2's P2-4 asked for. +- **Recovery:** + + ```sh + git fetch githubsucks + git worktree add ../pmacs-gbi-stage1 generated-buffer-immutability-stage1 + cd ../pmacs-gbi-stage1 + cargo test --test listview_acceptance --test dired_acceptance + cargo test --test terminal_copy_mode_acceptance --features crdt + ``` + ## Bottom-panel lane (Arc 7) — 2B-2 MERGED; 2B-3 IS NEXT Stage 1, the Stage 2 framing, Stage 2A, Stage 2B-1, and **Stage 2B-2 are From 4da4830f806a5d50991722d576391c50f543a503 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:27:17 -0400 Subject: [PATCH 3/7] fix(editor-core): drop a selection anchor a rewrite invalidated Review finding 1 on PR #191. `notify_buffer_edit` clamped `cursor` and `view_top` but not `win.selection.anchor`, and `rebuild_views_for` had the same gap. Clamping the cursor is not enough to make the region safe: `Window::region` orders `(anchor, cursor)`, so a stale anchor above a clamped cursor is still the region's high end and `region_bytes` slices the rope with it. Reproduced before the fix as `assertion failed: end <= self.len()` at `src/rope.rs:145`, reached from `EditorCore::clipboard_copy` after a generated rewrite. The anchor is DROPPED, not clamped. A window must always have a cursor, so clamping one is the only available answer; a window need not have a selection, and a clamped anchor asserts a region boundary the user never placed --- after a wholesale rewrite the surviving offsets address unrelated bytes. This is not a new rule: `window.quit`'s restore already answers the same question the same way with `selection.filter(|sel| sel.anchor <= len)` (`src/editor_core.rs:3259`). One rule, now three call sites. Both exits are pinned separately, because fixing one and trusting the other is how the gap arose: `acc16h` drives `notify_buffer_edit` through a generated write, `acc16i` drives `rebuild_views_for` through `pmacs.help.show_command`, which is the `*help*` renderer's real path. Deleting either call site fails only its own test. The pin also discriminates DROP from CLAMP, because that is the decision a revised Q#GB6 could overturn. The wording is marked PROVISIONAL in both the implementation and the pins. The rule belongs to Q#GB6, and PR #188's approved revision 5 does not mention the anchor; a revision request carrying this defect is with that lane. If the landed revision says clamp or translate, this changes to match rather than standing as a third description. Also in this commit, review findings 2 and 3 --- the tree asserting what the record does not support: - Criterion 5's restatement is withdrawn in BOTH suites. The tests now quote the approved criterion, are renamed `*_provisional_*`, and say they do not satisfy it; the evidence (`ensure_writable` precedes the intercept chain, with the measured `ReadOnly` message) is recorded as what was sent to #188, not as a replacement contract. The framing's own bite is unchanged and still fails them. - Criterion 7's "for each adopter" is restored: the listview half now exists as its own test. Its inability to carry the framing's mutation bite --- `window.switch_buffer` rebuilds the `TextView`, verified by applying the mutation and watching this half stay green while the dired half fails --- is recorded in the test and filed with #188, not resolved here. - Criteria 11 and 12 are relabelled from `main` bites to mutation bites. Both fail on `main` only at their disambiguation premise and never reach the assertions they exist for, so a revert is not evidence for what they assert. How a restated contract passed the previous gate run, since the next lane can use this: nothing in the gate suite reads a framing document, so a test that quietly narrows its criterion is indistinguishable from one that satisfies it --- both are green, and `scripts/bite` only proves an assertion bites some pre-image, never that the assertion is the one that was approved. The gate can catch a test that does not bite; it cannot catch a test that bites the wrong contract, so that check has to happen where the criterion is read. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/editor_core.rs | 50 ++++++- tests/dired_acceptance.rs | 71 +++++----- tests/listview_acceptance.rs | 178 ++++++++++++++++++++----- tests/terminal_copy_mode_acceptance.rs | 130 ++++++++++++++++++ 4 files changed, 366 insertions(+), 63 deletions(-) diff --git a/src/editor_core.rs b/src/editor_core.rs index 28fa0f4..ba4da86 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1846,6 +1846,10 @@ impl EditorCore { /// bounded by [`TextView::line_count`]. A replace can grow in bytes /// while collapsing many lines into one, so "the buffer shrank" is /// not a usable trigger for the second. + /// + /// The selection anchor is a **third** coordinate and it is dropped, + /// not clamped, when it no longer fits — see + /// [`Self::drop_stale_selection`] for why. pub fn notify_buffer_edit(&mut self, buffer_id: BufferId, edit: &Edit) { self.search_invalidate_for_edit(buffer_id, edit); let reg = self.registry.borrow(); @@ -1866,10 +1870,47 @@ impl EditorCore { if win.view_top > max_top { win.view_top = max_top; } + Self::drop_stale_selection(win, len); } } } + /// Drop `win`'s selection when its anchor no longer fits a buffer of + /// `len` bytes. + /// + /// **The anchor is dropped rather than clamped, and that asymmetry + /// with `cursor` is the point.** A window must always have a cursor, + /// so clamping one into range is the only available answer. A window + /// need not have a selection, and a *clamped* anchor asserts a region + /// boundary the user never placed — after a wholesale generated + /// rewrite the surviving offsets address unrelated bytes, so the + /// clamped region would be a selection of text nobody selected. + /// + /// This is not a new rule: `window.quit`'s restore already answers + /// exactly this question the same way, with + /// `selection.filter(|sel| sel.anchor <= len)` and a comment giving + /// this reason (`:3259`). Two call sites, one rule. + /// + /// Without it, `cursor`'s clamp is not enough to make the region + /// safe. `Window::region` orders `(anchor, cursor)`, so a stale + /// anchor above a clamped cursor still yields `hi > len`, and + /// `region_bytes` slices with it: reproduced as + /// `assertion failed: end <= self.len()` at `src/rope.rs:145` from + /// `EditorCore::clipboard_copy`. + /// + /// **PROVISIONAL WORDING.** The rule this implements belongs to + /// Q#GB6, and PR #188's revision 5 — the approved text at the time of + /// writing — does not mention the anchor at all. A revision request + /// carrying this defect is with that lane. If the landed revision + /// specifies clamping or translation instead, this function and its + /// pin change to match it; it must not be left as a third, + /// independently-worded description of the same rule. + fn drop_stale_selection(win: &mut Window, len: Position) { + if win.selection.is_some_and(|sel| sel.anchor > len) { + win.selection = None; + } + } + /// Force every window currently showing `buffer_id` to rebuild /// its [`TextView`] from scratch. /// @@ -1882,7 +1923,13 @@ impl EditorCore { /// what an end-to-end rewrite cost anyway. /// /// Cursor and `view_top` are clamped to the new buffer extent so - /// they don't dangle past the end after a shrinking rewrite. + /// they don't dangle past the end after a shrinking rewrite, and a + /// selection whose anchor no longer fits is dropped + /// ([`Self::drop_stale_selection`]). This function had the same + /// anchor gap [`Self::notify_buffer_edit`] did, and for the same + /// reason: clamping the cursor is not enough to make + /// [`Self::region_bytes`] safe, because `Window::region` orders the + /// pair and a stale anchor can still be the high end. pub fn rebuild_views_for(&mut self, buffer_id: BufferId) { let reg = self.registry.borrow(); let Ok(buffer) = reg.get(buffer_id) else { @@ -1899,6 +1946,7 @@ impl EditorCore { if win.view_top > max_top { win.view_top = max_top; } + Self::drop_stale_selection(win, len); } } } diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index b2011bb..261a073 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -1812,33 +1812,40 @@ fn dired_revert_still_repaints_after_the_lock() { ); } -/// Criterion 5 [fix-shape] --- the named intercept survives adoption and -/// is still what refuses an edit whenever the rope does not. +/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an +/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert +/// on the message text, which distinguishes them.* Bite: *an adopter +/// that deletes the intercept and relies on the rope passes 1-4 and +/// fails this.* /// -/// **Restated against the framing**, which asked for an ordinary edit -/// "refused by the INTERCEPT, not by the rope" and asserted on the -/// message text. That state is unreachable once the arc's lock is -/// installed: `Buffer::apply_edit` (`src/buffer.rs:773`) and -/// `Buffer::begin_edit` (`:725`) call `ensure_writable()` as their FIRST -/// statement, while the intercept chain runs later inside -/// `apply_edit_inner` (`:1072`), so the rope always answers first. The -/// criterion is therefore driven with the lock lifted --- the state the -/// intercept genuinely still covers, including the window between -/// `pmacs.buffer.create` and the first paint. +/// **PROVISIONAL --- this test does not currently satisfy that +/// criterion, and does not claim to.** The criterion's state proved +/// unreachable during Stage 1; a revision request carrying the evidence +/// is with PR #188, which owns this acceptance contract. Until that +/// revision lands and is re-approved this test stands in for criterion 5 +/// at the only point where the tree can express the distinction, and its +/// wording follows #188 rather than replacing it. **The framing's own +/// bite is preserved unchanged**: deleting `add_intercept` fails this +/// test. /// -/// *Bite:* unchanged --- an adopter that drops `add_intercept` and relies -/// on the rope alone passes criteria 3 and 4 and fails here. +/// The evidence handed to #188: `Buffer::apply_edit` +/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call +/// `ensure_writable()` as their FIRST statement, while the intercept +/// chain runs later inside `apply_edit_inner` (`:1072`), so once the +/// arc's lock is installed the rope always answers first. The stand-in +/// lifts the lock Rust-side, which is the state the intercept still +/// covers, including the window between `pmacs.buffer.create` and the +/// first paint. #[test] -fn dired_keeps_the_named_intercept_beside_the_rope_lock() { +fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { let td = fixture_dir(); let mut s = editor(); open_ok(&mut s, td.path(), "nil"); let listing = active_buffer_id(&s); let before = active_text(&s); - // With the lock on, the ROPE answers first, and its message is the - // one with the buffer id in it. Pinned so the restatement above - // cannot rot silently. + // The measurement reported to #188, pinned so it cannot rot while + // the revision is outstanding: with the lock on, the ROPE answers. type_char(&mut s, 'z'); assert!( status(&s).contains("(id BufferId("), @@ -1862,23 +1869,25 @@ fn dired_keeps_the_named_intercept_beside_the_rope_lock() { ); } -/// Criterion 7 [mutation] --- a repaint reaches the **window**, not just -/// the rope, pinned by painting a shrinking listing. +/// **Stage 1 criterion 7 [mutation]**, the dired half: *a refresh +/// reaches the window, not just the rope --- pinned by painting a +/// shrinking render (many rows -> one) and asserting row 1 is empty, for +/// each adopter.* Bite: *delete the `notify_buffer_edit_to_windows` call +/// in the `set_generated_contents` binding +/// (`src/lua_bindings/mod.rs:3092`).* +/// +/// The criterion says **for each adopter**, so the listview half is +/// `listview_acceptance::s1_7_a_shrinking_refresh_reaches_the_window`. +/// This half is the one that carries the framing's bite; the note on the +/// listview half records why, and that observation is with PR #188 as a +/// revision request rather than being settled here. /// /// A rope write is only half of an edit: the window holds a `TextView` /// line index that only `on_edit` maintains, so a write that reaches the /// rope without the fan-out leaves the two disagreeing, and the next -/// paint indexes the new rope with the old offsets. `dired.revert` is -/// the right driver because it paints and does **not** follow the paint -/// with a `window.switch_buffer` --- which rebuilds the `TextView` from -/// scratch and would mask the mutation. (`listview.refresh` and -/// `listview.open` both do switch, so the listview half of this -/// criterion cannot bite; the primitive's own pin is -/// `terminal_copy_mode_acceptance::acc16d`.) -/// -/// *Bite:* delete the `notify_buffer_edit_to_windows` call in the -/// `set_generated_contents` binding (`src/lua_bindings/mod.rs:3092`) and -/// the painted frame keeps rows the listing no longer has. +/// paint indexes the new rope with the old offsets. `dired.revert` +/// paints and does **not** follow the paint with a +/// `window.switch_buffer`. #[test] fn dired_a_shrinking_repaint_reaches_the_window() { let td = tempfile::tempdir().expect("tempdir"); diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 5740081..c5db6ed 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -123,6 +123,51 @@ fn set_read_only(s: &EditorState, id: BufferId, value: bool) { .set_read_only(value); } +/// Render the active window's text view into a cell grid. Criterion 7 is +/// pinned by PAINTING, because that is where a rope/window disagreement +/// bites: the rope is right and the screen is not. +fn paint_active_window(s: &EditorState, rows: u32, cols: u32) -> Vec { + use pmacs::cell::{Cell, CellGrid, CellSize}; + use pmacs::view::{View, Viewport}; + use pmacs::window::Rect; + + let mut core = s.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 { + use pmacs::cell::Glyph; + (0..cols) + .map(|c| match cells[(row * cols + c) as usize].glyph { + Glyph::Char(ch) => ch, + _ => ' ', + }) + .collect::() + .trim_end() + .to_owned() +} + /// Open a three-row test panel whose visits record into `_G.VISITED`. fn open_test_panel(s: &mut EditorState) { s.lua_host @@ -332,38 +377,41 @@ fn s1_4_the_owners_refresh_still_works_after_the_lock() { ); } -/// Criterion 5 [fix-shape] --- the named intercept survives adoption and -/// is still the thing that refuses an edit whenever the rope does not. +/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an +/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert +/// on the message text, which distinguishes them.* Bite: *an adopter +/// that deletes the intercept and relies on the rope passes 1-4 and +/// fails this.* /// -/// **Restated against the framing, which specified this criterion in a -/// form the tree cannot reach.** §6 Stage 1 criterion 5 asks for an -/// ordinary edit refused "by the INTERCEPT, not by the rope", asserted on -/// the message text. Once the arc's own lock is installed that state is -/// unreachable: `Buffer::apply_edit` (`src/buffer.rs:773`) and -/// `Buffer::begin_edit` (`:725`) both call `ensure_writable()` as their -/// FIRST statement, while the intercept chain runs later, inside -/// `apply_edit_inner` (`:1072`). Measured here: a self-insert on an -/// adopted panel now reports +/// **PROVISIONAL --- this test does not currently satisfy that +/// criterion, and does not claim to.** Implementing Stage 1 found the +/// criterion's state unreachable, with the evidence below; a revision +/// request is with PR #188, which owns this acceptance contract. Until +/// that revision lands and is re-approved, this test stands in for +/// criterion 5 by driving the same distinction at the only point where +/// the tree can express it, and its wording follows #188 rather than +/// replacing it. **The framing's own bite is preserved unchanged**: +/// deleting `add_intercept` fails this test. +/// +/// The evidence handed to #188: `Buffer::apply_edit` +/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call +/// `ensure_writable()` as their FIRST statement, while the intercept +/// chain runs later inside `apply_edit_inner` (`:1072`), so once the +/// arc's lock is installed the rope always answers first. Measured on +/// this branch, a self-insert on an adopted panel reports /// ``insert failed: buffer `*test-panel*` (id BufferId(n)) is read-only`` -/// --- the rope's message --- and can never report the intercept's. -/// -/// So the criterion is driven at the one point where the two are -/// distinguishable, which is also the state it is actually protecting: -/// the rope lock lifted. That covers the real window between -/// `pmacs.buffer.create` and the first render, and any future Rust-side -/// lift. -/// -/// *Bite:* unchanged from the framing's --- an adopter that deletes the -/// `add_intercept` call and relies on the rope alone passes criteria 1-4 -/// and fails here, because with the lock lifted the `z` lands. +/// and can never report the intercept's message. The stand-in lifts the +/// lock Rust-side first --- the state the intercept still covers, +/// including the window between `pmacs.buffer.create` and the first +/// render. #[test] -fn s1_5_an_ordinary_edit_is_refused_by_the_named_intercept_not_only_the_rope() { +fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { let mut s = EditorState::new(); open_test_panel(&mut s); let panel = id_of(&s, "*test-panel*"); - // With the lock ON, the rope answers first and the message is its - // own. Pinned so the restatement above cannot rot silently. + // The measurement reported to #188, pinned so it cannot rot while + // the revision is outstanding: with the lock on, the ROPE answers. press(&mut s, KeyCode::Char('z')); assert!( status(&s).contains("(id BufferId("), @@ -440,6 +488,55 @@ fn s1_6_round_trip_input_survives_the_adoption() { ); } +/// **Stage 1 criterion 7 [mutation]**, the listview half: *a refresh +/// reaches the window, not just the rope --- pinned by painting a +/// shrinking render (many rows -> one) and asserting row 1 is empty, for +/// each adopter.* Bite: *delete the `notify_buffer_edit_to_windows` call +/// in the `set_generated_contents` binding +/// (`src/lua_bindings/mod.rs:3092`).* +/// +/// The criterion says **for each adopter**, so both halves exist; the +/// dired half is `dired_acceptance::dired_a_shrinking_repaint_reaches_the_window`. +/// +/// **This half asserts the content produced but does NOT carry the +/// framing's mutation bite, and says so rather than being quietly +/// dropped.** `listview.refresh` and `listview.open` both follow +/// `render` with `pmacs.window.switch_buffer`, which rebuilds the +/// window's `TextView` from scratch (`src/editor_core.rs:4854-4868`) and +/// so repaints correctly even with the fan-out deleted --- on `main` +/// with its `bypass_intercept` writes just as much as here. Verified by +/// applying the mutation: this test stays green, while the dired half +/// fails with `assertion failed: end <= self.len()`. That observation is +/// filed with PR #188, which owns the criterion; it is recorded here, +/// not resolved here. +#[test] +fn s1_7_a_shrinking_refresh_reaches_the_window() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + let painted = paint_active_window(&s, 6, 24); + assert_eq!( + grid_row(&painted, 1, 24), + "alpha", + "precondition: three data rows paint" + ); + assert_eq!(grid_row(&painted, 3, 24), "gamma"); + + // `g` re-renders from three rows to one. + press(&mut s, KeyCode::Char('g')); + + let painted = paint_active_window(&s, 6, 24); + assert_eq!( + grid_row(&painted, 1, 24), + "delta", + "the refreshed row must paint" + ); + assert_eq!( + grid_row(&painted, 2, 24), + "", + "and nothing of the rows it replaced" + ); +} + /// Criterion 9 [`main`] --- a foreign buffer that happens to share the /// panel's name is never adopted (Q#GB13). /// @@ -524,11 +621,22 @@ fn s1_10_the_disambiguation_limit_raises_rather_than_adopting() { assert_eq!(mine, "mine", "and touch nothing"); } -/// Criterion 11 [`main`] --- a **disambiguated** panel still answers -/// `RET`, `g` and `q` (Q#GB18). +/// **Stage 1 criterion 11** --- a **disambiguated** panel still answers +/// `RET`, `g` and `q` (Q#GB18). The framing labels it `[main]` and names +/// its bite as *Q#GB13 landed without Q#GB18*. /// -/// This is the criterion that fails against Q#GB13 landed without -/// Q#GB18: disambiguation alone leaves the old lookup reading +/// **Recorded here as a MUTATION bite, because that is what it is.** On +/// `main` this test fails at its disambiguation *premise* --- `main` +/// adopts the foreign buffer, so the panel is never called +/// `*test-panel*<2>` and the `RET`/`g`/`q` assertions are never reached. +/// A revert therefore proves nothing about what the criterion asserts. +/// The bite the framing actually names is a mutation of this branch: +/// keep the disambiguation, restore a name-keyed `panel_for_buffer`. +/// Verified --- under that mutation the test fails at the `g` assertion. +/// The `[main]` label belongs to #188 and is reported to it; what the +/// tree claims is corrected here either way. +/// +/// Disambiguation alone leaves the old lookup reading /// `panels["*test-panel*<2>"]` for a record stored under /// `"*test-panel*"`, so all three commands return early. Every one of /// them fails **silently**, so the assertion is on the content each @@ -568,9 +676,17 @@ fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { ); } -/// Criterion 12 [`main`] --- the `q`-target capture is not inverted +/// **Stage 1 criterion 12** --- the `q`-target capture is not inverted /// (Q#GB18), which needs its own criterion because it fails **open** -/// rather than closed. +/// rather than closed. The framing labels it `[main]`. +/// +/// **Recorded here as a MUTATION bite**, for the same reason as +/// criterion 11: on `main` this test fails at its disambiguation +/// premise and never reaches the `q`-target assertion, so a revert is +/// not evidence for what it asserts. Under the mutation the framing +/// actually names --- a name-keyed `panel_for_buffer` beside the +/// disambiguation --- it fails at the assertion it exists for, +/// `q must never return into another panel`. Verified. /// /// `listview.open`'s guard reads "capture the current buffer as the `q` /// target, but never another panel (chained panels would trap `q` in a diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index c572166..fa3fd7d 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -1108,3 +1108,133 @@ fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { ); assert_eq!(top, 0, "the collapsed buffer has exactly one line"); } + +/// Generated-buffer immutability Stage 1 — the **selection anchor** is a +/// third window coordinate a generated rewrite invalidates, and clamping +/// the cursor alone does not make the region safe. +/// +/// **PROVISIONAL, and named as such.** This pins a defect found in +/// review of PR #191; the rule belongs to Q#GB6, whose approved text +/// (PR #188 revision 5) does not mention the anchor. A revision request +/// carrying this defect is with that lane. This test stands in for the +/// anchor clause of a revised Q#GB6 and must be reconciled with it — +/// including its verdict of *drop* rather than *clamp* — when the +/// revision lands. It is not an independent contract. +/// +/// `Window::region` orders `(anchor, cursor)`, so a stale anchor above a +/// clamped cursor is still the high end of the region, and +/// `region_bytes` slices the rope with it. Reproduced on this branch +/// before the fix: `assertion failed: end <= self.len()` at +/// `src/rope.rs:145`, reached from `EditorCore::clipboard_copy`. +/// +/// *Bite:* delete the `drop_stale_selection` call from +/// `notify_buffer_edit` and this panics rather than failing an +/// assertion. Note the anchor must be the **high** end: with the anchor +/// low and the cursor high the cursor clamp already covers it, so a +/// forward selection passes with the bug live. +#[test] +fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { + let state = EditorState::new(); + exec( + &state, + r" + GEN = pmacs.buffer.create('*anchor-probe*') + pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n') + pmacs.window.switch_buffer(GEN) + ", + ); + { + // A BACKWARD selection: anchor at the far end, point at the + // start. The forward one is not a discriminator. + let mut core = state.core.borrow_mut(); + core.begin_selection(30); + core.set_cursor_byte(0); + assert_eq!( + core.active_region(), + Some((0, 30)), + "precondition: a live 30-byte region" + ); + } + + exec(&state, r"pmacs.buffer.set_generated_contents(GEN, 'xy')"); + + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_buffer_len(), + 2, + "precondition: the buffer shrank" + ); + assert_eq!( + core.active_window().selection, + None, + "an anchor that no longer fits is dropped, not clamped" + ); + assert_eq!(core.active_region(), None, "so there is no region left"); + // The production consumer, not just the field: this is the call that + // panicked before the fix. + assert!( + !core.clipboard_copy(), + "copy must report 'no region' rather than slice past the rope" + ); +} + +/// The same anchor gap in the **other** function, driven through its own +/// real Lua path. +/// +/// `EditorCore::rebuild_views_for` had the identical defect and is a +/// separate exit: the `*help*` renderer rewrites end to end and calls it +/// rather than `notify_buffer_edit` (`src/lua_bindings/mod.rs:1650`, via +/// `pmacs.help.show_command`). Fixing one function and not the other +/// would leave a live panic reachable from `M-x` help, so this pins the +/// second exit rather than trusting that one call site implies the +/// other. +/// +/// Same PROVISIONAL status as `acc16h`: the rule is Q#GB6's and its +/// approved text does not yet carry the anchor. +/// +/// *Bite:* delete the `drop_stale_selection` call from +/// `rebuild_views_for` and this panics at `src/rope.rs:145`. `acc16h` +/// stays green under that mutation, which is why this test exists +/// separately. +#[test] +fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { + let state = EditorState::new(); + // 286 bytes, then 154: a real shrink through the help renderer. + exec( + &state, + r" + HELP = pmacs.help.show_command('cursor.down') + pmacs.window.switch_buffer(HELP) + ", + ); + let long_len: i64 = eval(&state, "return HELP:len()"); + { + let mut core = state.core.borrow_mut(); + let anchor = u64::try_from(long_len).expect("non-negative"); + core.begin_selection(anchor); + core.set_cursor_byte(0); + assert_eq!( + core.active_region(), + Some((0, anchor)), + "precondition: a live region anchored at the end" + ); + } + + exec(&state, "pmacs.help.show_command('editor.quit')"); + + let short_len: i64 = eval(&state, "return HELP:len()"); + assert!( + short_len < long_len, + "precondition: the help buffer shrank ({long_len} -> {short_len})" + ); + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_window().selection, + None, + "rebuild_views_for must drop an anchor that no longer fits" + ); + assert!( + !core.clipboard_copy(), + "copy must report 'no region' rather than slice past the rope" + ); +} From 99d026fa226b40e2a65e666f27f646d9bde9610d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:40:35 -0400 Subject: [PATCH 4/7] docs(active-work): rewrite the Stage 1 lane after review round 1 The lane itself carried two instances of the defect the review found in the tests, so it is rewritten rather than appended to. - Its "two framing criteria were wrong and the tests say so" bullet was a restated contract in the ledger, which is the same shape as the one in the test comments. Replaced by a statement of the boundary --- #188 owns the acceptance contract, this lane adopts it and files findings there --- and by what each finding actually was. - Its bite list said "five criteria are falsified by revert" over six enumerated items, and counted the disambiguated-panel criterion among them. That criterion fails on `main` only at its fixture premise, so it is a mutation bite. Both errors corrected, and the list is now split by falsification method rather than by a single count. The base is measured in the lane with the command output pasted, and labelled a reading rather than a constant: `main` moved twice while this lane was open, so a SHA quoted forward from an earlier message would already be wrong. Adds the sweep result (7 sites, 4 named by the review, 3 found by the sweep), the re-run bite results under `scripts/bite`'s new positive control, and the one deliberately-recorded VACUOUS result --- criterion 7's listview half, which passes under the criterion's own mutation because `window.switch_buffer` rebuilds the `TextView`. That is measured, not inferred: the same mutation reports VACUOUS against the listview half alone and BITES against the dired half. Also records that the dired 200 ms perf test is load-sensitive rather than regressed, with the pre-image comparison: 0.09 s either way over five runs each, so the whole-buffer conversion costs nothing measurable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 161 ++++++++++++++++++++++++++++++-------------- 1 file changed, 109 insertions(+), 52 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 32b69b5..d0d48f5 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -248,15 +248,30 @@ If it does not, stop and repair the remote/fetch configuration. ## Generated-buffer immutability lane (Arc: workbench primitives) — STAGE 1 OPEN **Framing: [PR #188](https://github.com/levineuwirth/pmacs/pull/188), -revision 5, APPROVED and still open.** Read it as +approved and still open. #188 owns the acceptance contract; this lane +adopts it.** Where implementing Stage 1 found a criterion impossible or +mislabelled, the finding goes to #188 as a revision request and this lane +waits — it does not restate, narrow or reclassify a criterion locally. +Read the framing as `git show githubsucks/generated-buffer-immutability:docs/generated-buffer-immutability-framing.md` -until it merges. Stage 1 is branched from **landed `main`**, not stacked -on the framing branch, so the two merge in either order. +until it merges. -- **Branch `generated-buffer-immutability-stage1`, based on - `githubsucks/main` @ `300cbc4`.** Worktree - `../pmacs-gbi-stage1`. Code checkpoint `e1b859f`; this ledger commit - rides on top of it. +- **Branch `generated-buffer-immutability-stage1`**, worktree + `../pmacs-gbi-stage1`. `githubsucks/main` is integrated into it. + Measured when this line was written: + + ``` + $ git rev-parse --short githubsucks/main + 64883eb + $ git log --oneline -1 githubsucks/main + 64883eb Merge pull request #192 from levineuwirth/bite-positive-control + $ git merge-base --is-ancestor githubsucks/main HEAD && echo "main IS integrated" + main IS integrated + ``` + + **That is a reading, not a constant.** `main` moved twice while this + lane was open (#187 -> #192, with #193 behind it). Re-measure before + quoting it; do not copy the SHA forward. - **What Stage 1 ships.** `dired.lua`'s `paint` and `listview.lua`'s `render` write through `pmacs.buffer.set_generated_contents` (zero `bypass_intercept` writes remain in either file); `listview` gains @@ -271,26 +286,42 @@ on the framing branch, so the two merge in either order. seven undo chords to a no-op; `dired.lua` and `listview.lua` rebind **nothing**, so a bare `C-/` emptied a listing and a panel. Stage 1 closes the only two families reachable without `M-x`. -- **Two framing criteria were wrong and the tests say so rather than - working around them.** - - Stage 1 criterion 5 ("an ordinary edit is refused by the INTERCEPT, - not by the rope") is **unreachable** once this arc's lock exists. - `Buffer::apply_edit` (`src/buffer.rs:773`) and `begin_edit` - (`:725`) call `ensure_writable` as their FIRST statement, while the - intercept chain runs later inside `apply_edit_inner` (`:1072`), so - the rope always answers first. Measured: a self-insert on an - adopted panel reports ``insert failed: buffer `*test-panel*` (id - BufferId(n)) is read-only``. Restated in both suites as "the - intercept still refuses with its named error **when the rope is - lifted**", which keeps the framing's own bite (delete - `add_intercept`) and is the state the intercept genuinely covers. - - Criterion 7 ("a refresh reaches the window") **cannot bite at the - listview adopter**: `listview.refresh` and `listview.open` both - follow `render` with `window.switch_buffer`, which rebuilds the - `TextView` from scratch (`src/editor_core.rs:4859-4868`) and masks - a dropped fan-out. `dired.revert` and `dired.sort-cycle` paint - without a switch, so the dired half carries it and fails the - mutation with the reported `assertion failed: end <= self.len()`. +- **Review round 1 closed three findings.** All three are the same + class in the end: *the tree asserting something the record does not + support.* + - **[P1] The selection anchor.** `notify_buffer_edit` clamped + `cursor` and `view_top` but not `win.selection.anchor`, and + `rebuild_views_for` had the same gap. `Window::region` orders + `(anchor, cursor)`, so a stale anchor above a clamped cursor is + still the region's high end: reproduced as + `assertion failed: end <= self.len()` (`src/rope.rs:145`) from + `EditorCore::clipboard_copy`. Fixed by **dropping** the selection, + not clamping it — `window.quit`'s restore already answers this + question the same way (`src/editor_core.rs:3259`). Both exits are + pinned separately (`acc16h`, `acc16i`) and each mutation fails only + its own. **The wording is provisional**: the rule is Q#GB6's, whose + approved text does not mention the anchor, and a revision request is + with #188. + - **[P1] Criteria 5 and 7 were restated locally.** Withdrawn. Both + suites now quote the approved criterion, the criterion-5 tests are + renamed `*_provisional_*` and say they do not satisfy it, and + criterion 7's **"for each adopter"** is restored with the listview + half added back. The evidence stays as evidence and is filed with + #188. + - **[P2] Criterion 12 was labelled a `main` bite.** It is a mutation + bite; on `main` it fails at its disambiguation premise and never + reaches its assertion. Relabelled. +- **Sweep for that class across this branch: 7 sites, 4 named by the + review and 3 found by the sweep.** Class A, a restated or narrowed + contract — the listview criterion-5 doc, **the dired criterion-5 doc** + (a second file the review did not cite), criterion 7's narrowing plus + its **missing listview half**, and this lane's own "two framing + criteria were wrong" bullet. Class B, a bite recorded as a `main` + failure that is really a mutation bite — criterion 12, **criterion 11 + (same defect, not reported)**, and this lane's own revert list, which + said "five criteria" over six items and counted criterion 11 among + them. All 7 corrected. Every remaining `[main]` label was re-run and + fails at its own assertion, not at a premise. - **Stage 2 still owes everything with new Rust in it**, per the framing's cut: `Buffer::apply_generated_edit` + `GeneratedOutcome` + the `{ generated = true }` option + its own `run_buffer_edit` arm; @@ -299,31 +330,57 @@ on the framing branch, so the two merge in either order. for `compile.lua` and the search panel; Q#GB5's `ensure_slot` lock; conversion of the remaining 13 write sites; and the three `compile_mode_acceptance` intruder tests converted per Q#GB12. -- **Verification at code checkpoint `e1b859f`.** `cargo fmt --check`; - `cargo clippy --workspace --all-targets -- -D warnings`; library - **1,863 passed + 3 ignored** default and **2,048 passed + 4 ignored** - CRDT; `listview_acceptance` **16**, `dired_acceptance` **31**, - `folding_acceptance` **21**, `terminal_copy_mode_acceptance` **16** - default and **17** with `--features crdt` (the extra one is - `acc16e`, which a default run never compiles — judge that step by the - count, not the verdict); M4 **121 passed + 3 ignored + 1 filtered** - with `--skip basedpyright`; required GPU **202/202**; isolated-config - full workspace sweep **3,511 passed across 103 binaries, exit 0**; - `git diff --check` clean. -- **Bites, all executed.** Five criteria are falsified by revert against - `githubsucks/main` (`scripts/bite` on `builtin/runtime/listview.lua` - and `builtin/runtime/dired.lua`): the two undo criteria, the - no-adoption criterion, the disambiguated-panel criterion, and the - fold-refusal pair. Nine more are falsified by a named one-line - mutation, each run and each observed to fail: dropping the fan-out in - the `set_generated_contents` binding; deleting the cursor clamp; - gating the `view_top` clamp on "the buffer shrank"; deleting - `self.read_only = false` from `set_generated_contents`; deleting - `add_intercept` and `set_round_trip_input` at each adopter; restoring - a name-keyed `panel_for_buffer`; adopting at the variant limit; and - restoring the old fold status string. **The `view_top` and `cursor` - clamps each fail only their own criterion**, which is the - discrimination review round 2's P2-4 asked for. +- **Verification.** Run on the merged tree, at the code checkpoint the + next commit records; the ledger commit on top is docs-only and + `cargo fmt --check` + `git diff --check` were re-run after it. + `cargo fmt --check` clean; `cargo clippy --workspace --all-targets -- + -D warnings` clean; library **1,863 passed + 3 ignored** default and + **2,048 passed + 4 ignored** CRDT; `listview_acceptance` **17**, + `dired_acceptance` **31**, `folding_acceptance` **21**, + `terminal_copy_mode_acceptance` **18** default and **19** with + `--features crdt` — judge that step by the count, because `acc16e` is + `#[cfg(feature = "crdt")]` and a default run never compiles it; M4 + **121 passed + 3 ignored + 1 filtered** with `--skip basedpyright`; + required GPU **202/202**; isolated-config full workspace sweep + **3,514 passed across 103 binaries, exit 0**; `git diff --check` + clean. +- **The dired 200 ms perf test is load-sensitive, and the conversion + costs it nothing.** Review saw `dired_renders_10k_entries_within_200ms` + take 241 ms in a combined run and pass alone. Measured here: 0.09 s + isolated over five runs, and the whole 31-test suite finishes in + 0.12 s, so 241 ms was contention rather than a regression. Measured + against the pre-image as well, by swapping in `main`'s `dired.lua` + (the `bypass_intercept` paint): **0.09 s either way over five runs + each**. A whole-buffer `set_generated_contents` costs the same as the + bypass replace it replaces, which discharges Q#GB4's measurement + obligation for the whole-buffer case only — the streaming case is + Stage 2's and is not touched here. +- **Bites, re-run under `scripts/bite`'s positive control (#192).** + A bare `bite: OK` from the pre-#192 script is weaker than it looks, so + every result below is from the current script or from a mutation + harness carrying the same control (named tests must pass on the + working tree and at least one must have run). + - **Falsified by revert, all `OK (assertion)` — not `OK (COMPILE)`:** + `builtin/runtime/listview.lua` for criteria 1, 2, 9 and 10; + `builtin/runtime/dired.lua` for criteria 3 and 13a; + `src/lua_bindings/fold.rs` for 13b; `src/editor_core.rs` for 8, 8b + and both anchor pins. + - **Falsified by a named mutation, each observed to fail:** the + fan-out drop in the `set_generated_contents` binding (criterion 7); + deleting `self.read_only = false` (criterion 4, both adopters); + deleting `add_intercept` and `set_round_trip_input` at each adopter + (criteria 5 and 6); the name-keyed `panel_for_buffer` (criteria 11 + and 12); adopting at the variant limit (criterion 10); the old fold + status string (13b); deleting each clamp (8, 8b); and **clamping + instead of dropping** the anchor. + - **One recorded VACUOUS result, deliberately.** Criterion 7's + listview half passes under the criterion's own mutation, because + `listview.refresh` and `listview.open` follow `render` with + `window.switch_buffer`, which rebuilds the `TextView` + (`src/editor_core.rs:4854-4868`). Measured, not inferred: the same + mutation run against the listview half alone reports VACUOUS while + the dired half reports BITES. Filed with #188; the half is kept and + labelled rather than deleted. - **Recovery:** ```sh From dab0c184e14d918e746471841c39d38efea7af22 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:47:55 -0400 Subject: [PATCH 5/7] docs(active-work): re-measure the base after #193, and say why once was not enough The previous commit pasted `git rev-parse`/`git log`/`merge-base` output showing `main` at `64883eb` and "main IS integrated". #193 merged between writing that and pushing it, so the pasted output was false in the tree that carried it --- and "main is integrated" is exactly the kind of claim this review round was about: the tree asserting something the record does not support. Re-measured and re-integrated, and the bullet now says the thing the first attempt got wrong. Pasting command output is necessary and NOT sufficient: a base measured before the last push is a claim about a tree nobody has, so it has to be re-measured at push time and treated as expired on sight. `main` moved three times inside this one lane (#187 -> #192 -> #193). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index dcb32cd..48ff54a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -262,16 +262,21 @@ until it merges. ``` $ git rev-parse --short githubsucks/main - 64883eb + 5e186c7 $ git log --oneline -1 githubsucks/main - 64883eb Merge pull request #192 from levineuwirth/bite-positive-control + 5e186c7 Merge pull request #193 from levineuwirth/test-improvement-audit $ git merge-base --is-ancestor githubsucks/main HEAD && echo "main IS integrated" main IS integrated ``` - **That is a reading, not a constant.** `main` moved twice while this - lane was open (#187 -> #192, with #193 behind it). Re-measure before - quoting it; do not copy the SHA forward. + **That is a reading, not a constant, and it went stale inside this + lane's own review round.** `main` moved three times while the lane was + open: #187 -> #192 -> #193. An earlier revision of this bullet pasted + the same three commands with `64883eb` and the same `main IS + integrated` line, and #193 merged between writing it and pushing it --- + so the pasted output was false in the tree that carried it. Pasting + command output is necessary and **not sufficient**: re-measure at push + time, and treat any base SHA in this file as expired on sight. - **What Stage 1 ships.** `dired.lua`'s `paint` and `listview.lua`'s `render` write through `pmacs.buffer.set_generated_contents` (zero `bypass_intercept` writes remain in either file); `listview` gains From 5d92348054c11062295e048ffa006d42860d8b86 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:26:08 -0400 Subject: [PATCH 6/7] fix(editor-core): normalize rewritten selections Adopt Q#GB6's clamp-or-clear rule in both window-coordinate normalization paths. Preserve shortened selections, clear only those collapsed by a moved endpoint, and pin both outcomes through the real generated-write and view-rebuild callers. Make listview refresh rely on the generated-write notification before reseating, so Stage 1 criterion 7's fan-out mutation bites both adopters. Align criteria 5, 11, and 12 with framing revision 7. --- builtin/runtime/listview.lua | 10 +- src/editor_core.rs | 77 +++++---------- tests/dired_acceptance.rs | 58 +++++------ tests/listview_acceptance.rs | 107 ++++++++------------ tests/terminal_copy_mode_acceptance.rs | 130 ++++++++++++++++++------- 5 files changed, 197 insertions(+), 185 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 49f19e5..3c83119 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -260,9 +260,13 @@ pmacs.command.define { local saved = pmacs.editor.cursor_line() local rows = p.on_refresh() or {} render(p, rows) - -- The wholesale rewrite leaves the window cursor at a stale byte - -- offset; re-enter the buffer to reset, then re-seat. - pmacs.window.switch_buffer(p.buffer) + -- `set_generated_contents` has already refreshed this window's + -- TextView. Re-seat through the editor primitives instead of + -- switching to the buffer it already shows: that redundant switch + -- rebuilt the TextView and hid a missing edit notification. + pmacs.editor.clear_selection() + pmacs.editor.set_view_top(0) + pmacs.editor.move_to_line(0) seat_cursor(p, saved) end, } diff --git a/src/editor_core.rs b/src/editor_core.rs index ba4da86..2a5691e 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1847,9 +1847,10 @@ impl EditorCore { /// while collapsing many lines into one, so "the buffer shrank" is /// not a usable trigger for the second. /// - /// The selection anchor is a **third** coordinate and it is dropped, - /// not clamped, when it no longer fits — see - /// [`Self::drop_stale_selection`] for why. + /// The selection anchor is a **third** coordinate. It is clamped to + /// the buffer extent, and the selection is cleared only when moving + /// an endpoint collapses it — see + /// [`Self::clamp_cursor_and_selection`]. pub fn notify_buffer_edit(&mut self, buffer_id: BufferId, edit: &Edit) { self.search_invalidate_for_edit(buffer_id, edit); let reg = self.registry.borrow(); @@ -1863,52 +1864,33 @@ impl EditorCore { for overlay in &mut win.overlays { let _ = overlay.on_edit(buffer, edit); } - if win.cursor > len { - win.cursor = len; - } + Self::clamp_cursor_and_selection(win, len); let max_top = win.text_view.line_count().saturating_sub(1); if win.view_top > max_top { win.view_top = max_top; } - Self::drop_stale_selection(win, len); } } } - /// Drop `win`'s selection when its anchor no longer fits a buffer of - /// `len` bytes. + /// Clamp `win`'s cursor and selection anchor to `len`, clearing the + /// selection only when the clamp collapses it. /// - /// **The anchor is dropped rather than clamped, and that asymmetry - /// with `cursor` is the point.** A window must always have a cursor, - /// so clamping one into range is the only available answer. A window - /// need not have a selection, and a *clamped* anchor asserts a region - /// boundary the user never placed — after a wholesale generated - /// rewrite the surviving offsets address unrelated bytes, so the - /// clamped region would be a selection of text nobody selected. - /// - /// This is not a new rule: `window.quit`'s restore already answers - /// exactly this question the same way, with - /// `selection.filter(|sel| sel.anchor <= len)` and a comment giving - /// this reason (`:3259`). Two call sites, one rule. - /// - /// Without it, `cursor`'s clamp is not enough to make the region - /// safe. `Window::region` orders `(anchor, cursor)`, so a stale - /// anchor above a clamped cursor still yields `hi > len`, and - /// `region_bytes` slices with it: reproduced as - /// `assertion failed: end <= self.len()` at `src/rope.rs:145` from - /// `EditorCore::clipboard_copy`. - /// - /// **PROVISIONAL WORDING.** The rule this implements belongs to - /// Q#GB6, and PR #188's revision 5 — the approved text at the time of - /// writing — does not mention the anchor at all. A revision request - /// carrying this defect is with that lane. If the landed revision - /// specifies clamping or translation instead, this function and its - /// pin change to match it; it must not be left as a third, - /// independently-worded description of the same rule. - fn drop_stale_selection(win: &mut Window, len: Position) { - if win.selection.is_some_and(|sel| sel.anchor > len) { - win.selection = None; - } + /// This mirrors terminal selection normalization: a surviving, + /// shortened region remains selected, while a region whose content + /// disappeared does not become an accidental active-but-empty + /// selection. Looking at whether either endpoint moved distinguishes + /// that case from a zero-width selection the user created. + fn clamp_cursor_and_selection(win: &mut Window, len: Position) { + let old_cursor = win.cursor; + win.cursor = win.cursor.min(len); + win.selection = win.selection.and_then(|mut selection| { + let old_anchor = selection.anchor; + selection.anchor = selection.anchor.min(len); + let collapsed_by_clamp = selection.anchor == win.cursor + && (selection.anchor != old_anchor || win.cursor != old_cursor); + (!collapsed_by_clamp).then_some(selection) + }); } /// Force every window currently showing `buffer_id` to rebuild @@ -1923,13 +1905,9 @@ impl EditorCore { /// what an end-to-end rewrite cost anyway. /// /// Cursor and `view_top` are clamped to the new buffer extent so - /// they don't dangle past the end after a shrinking rewrite, and a - /// selection whose anchor no longer fits is dropped - /// ([`Self::drop_stale_selection`]). This function had the same - /// anchor gap [`Self::notify_buffer_edit`] did, and for the same - /// reason: clamping the cursor is not enough to make - /// [`Self::region_bytes`] safe, because `Window::region` orders the - /// pair and a stale anchor can still be the high end. + /// they don't dangle past the end after a shrinking rewrite. + /// Selection normalization uses the same clamp-or-clear rule as + /// [`Self::notify_buffer_edit`]. pub fn rebuild_views_for(&mut self, buffer_id: BufferId) { let reg = self.registry.borrow(); let Ok(buffer) = reg.get(buffer_id) else { @@ -1939,14 +1917,11 @@ impl EditorCore { for win in self.windows.values_mut() { if win.buffer_id == buffer_id { win.text_view = TextView::new(buffer); - if win.cursor > len { - win.cursor = len; - } + Self::clamp_cursor_and_selection(win, len); let max_top = win.text_view.line_count().saturating_sub(1); if win.view_top > max_top { win.view_top = max_top; } - Self::drop_stale_selection(win, len); } } } diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index 261a073..bff9134 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -1812,45 +1812,33 @@ fn dired_revert_still_repaints_after_the_lock() { ); } -/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an -/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert -/// on the message text, which distinguishes them.* Bite: *an adopter -/// that deletes the intercept and relies on the rope passes 1-4 and -/// fails this.* +/// Stage 1 criterion 5 [`main`, and also fix-shape] — the rope lock +/// refuses an ordinary edit first, and the named dired intercept +/// survives behind it. /// -/// **PROVISIONAL --- this test does not currently satisfy that -/// criterion, and does not claim to.** The criterion's state proved -/// unreachable during Stage 1; a revision request carrying the evidence -/// is with PR #188, which owns this acceptance contract. Until that -/// revision lands and is re-approved this test stands in for criterion 5 -/// at the only point where the tree can express the distinction, and its -/// wording follows #188 rather than replacing it. **The framing's own -/// bite is preserved unchanged**: deleting `add_intercept` fails this -/// test. -/// -/// The evidence handed to #188: `Buffer::apply_edit` -/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call -/// `ensure_writable()` as their FIRST statement, while the intercept -/// chain runs later inside `apply_edit_inner` (`:1072`), so once the -/// arc's lock is installed the rope always answers first. The stand-in -/// lifts the lock Rust-side, which is the state the intercept still -/// covers, including the window between `pmacs.buffer.create` and the -/// first paint. +/// The rope half asserts the exact `BufferError::ReadOnly` rendering and +/// byte identity. The lifted half distinguishes the intercept by its +/// `intercept rejected the edit` message, so deleting `add_intercept` +/// still fails with the rope guard intact. #[test] -fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { +fn dired_rope_lock_and_named_intercept_refuse_in_order() { let td = fixture_dir(); let mut s = editor(); open_ok(&mut s, td.path(), "nil"); let listing = active_buffer_id(&s); + let name = active_name(&s); let before = active_text(&s); - // The measurement reported to #188, pinned so it cannot rot while - // the revision is outstanding: with the lock on, the ROPE answers. type_char(&mut s, 'z'); - assert!( - status(&s).contains("(id BufferId("), - "with the lock on the rope refuses first; got {:?}", - status(&s) + assert_eq!( + status(&s), + format!("insert failed: buffer `{name}` (id {listing:?}) is read-only"), + "with the lock on, the rope must provide the exact refusal" + ); + assert_eq!( + active_text(&s), + before, + "the rope refusal leaves every byte unchanged" ); set_read_only(&s, listing, false); @@ -1864,8 +1852,14 @@ fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { ); let st = status(&s); assert!( - st.contains("dired.lua") && st.contains("is read-only"), - "and refuse it by NAME, not with the rope's message; got {st:?}" + st.starts_with("insert failed: intercept rejected the edit:") + && st.contains("dired.lua") + && st.contains("is read-only"), + "the lifted path must carry the named intercept refusal; got {st:?}" + ); + assert!( + !st.contains(&format!("buffer `{name}` (id {listing:?}) is read-only")), + "the lifted path must not masquerade as the rope refusal: {st:?}" ); } diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index c5db6ed..8c15629 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -377,46 +377,32 @@ fn s1_4_the_owners_refresh_still_works_after_the_lock() { ); } -/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an -/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert -/// on the message text, which distinguishes them.* Bite: *an adopter -/// that deletes the intercept and relies on the rope passes 1-4 and -/// fails this.* +/// Stage 1 criterion 5 [`main`, and also fix-shape] — the rope lock +/// refuses an ordinary edit first, and the named intercept survives +/// behind it. /// -/// **PROVISIONAL --- this test does not currently satisfy that -/// criterion, and does not claim to.** Implementing Stage 1 found the -/// criterion's state unreachable, with the evidence below; a revision -/// request is with PR #188, which owns this acceptance contract. Until -/// that revision lands and is re-approved, this test stands in for -/// criterion 5 by driving the same distinction at the only point where -/// the tree can express it, and its wording follows #188 rather than -/// replacing it. **The framing's own bite is preserved unchanged**: -/// deleting `add_intercept` fails this test. -/// -/// The evidence handed to #188: `Buffer::apply_edit` -/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call -/// `ensure_writable()` as their FIRST statement, while the intercept -/// chain runs later inside `apply_edit_inner` (`:1072`), so once the -/// arc's lock is installed the rope always answers first. Measured on -/// this branch, a self-insert on an adopted panel reports -/// ``insert failed: buffer `*test-panel*` (id BufferId(n)) is read-only`` -/// and can never report the intercept's message. The stand-in lifts the -/// lock Rust-side first --- the state the intercept still covers, -/// including the window between `pmacs.buffer.create` and the first -/// render. +/// Both halves are required. The first asserts the exact +/// `BufferError::ReadOnly` rendering and byte identity. The second lifts +/// the lock Rust-side and distinguishes the intercept by its +/// `intercept rejected the edit` message. Deleting `add_intercept` +/// therefore passes the rope half and fails the lifted half. #[test] -fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { +fn s1_5_the_rope_lock_and_named_intercept_refuse_in_order() { let mut s = EditorState::new(); open_test_panel(&mut s); let panel = id_of(&s, "*test-panel*"); + let before = active_text(&s); - // The measurement reported to #188, pinned so it cannot rot while - // the revision is outstanding: with the lock on, the ROPE answers. press(&mut s, KeyCode::Char('z')); - assert!( - status(&s).contains("(id BufferId("), - "with the lock on, the ROPE refuses first; got {:?}", - status(&s) + assert_eq!( + status(&s), + format!("insert failed: buffer `*test-panel*` (id {panel:?}) is read-only"), + "with the lock on, the rope must provide the exact refusal" + ); + assert_eq!( + active_text(&s), + before, + "the rope refusal leaves every byte unchanged" ); set_read_only(&s, panel, false); @@ -430,8 +416,16 @@ fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { ); let st = status(&s); assert!( - st.contains("listview.lua") && st.contains("*test-panel* is read-only"), - "and refuse it by NAME, not with the rope's message; got {st:?}" + st.starts_with("insert failed: intercept rejected the edit:") + && st.contains("listview.lua") + && st.contains("*test-panel* is read-only"), + "the lifted path must carry the named intercept refusal; got {st:?}" + ); + assert!( + !st.contains(&format!( + "buffer `*test-panel*` (id {panel:?}) is read-only" + )), + "the lifted path must not masquerade as the rope refusal: {st:?}" ); } @@ -498,17 +492,10 @@ fn s1_6_round_trip_input_survives_the_adoption() { /// The criterion says **for each adopter**, so both halves exist; the /// dired half is `dired_acceptance::dired_a_shrinking_repaint_reaches_the_window`. /// -/// **This half asserts the content produced but does NOT carry the -/// framing's mutation bite, and says so rather than being quietly -/// dropped.** `listview.refresh` and `listview.open` both follow -/// `render` with `pmacs.window.switch_buffer`, which rebuilds the -/// window's `TextView` from scratch (`src/editor_core.rs:4854-4868`) and -/// so repaints correctly even with the fan-out deleted --- on `main` -/// with its `bypass_intercept` writes just as much as here. Verified by -/// applying the mutation: this test stays green, while the dired half -/// fails with `assertion failed: end <= self.len()`. That observation is -/// filed with PR #188, which owns the criterion; it is recorded here, -/// not resolved here. +/// `listview.refresh` re-seats through the already-notified `TextView`; +/// it deliberately does not rebuild the view by switching to the buffer +/// it already shows. Deleting the notification fan-out therefore leaves +/// the old line index live and this paint assertion bites. #[test] fn s1_7_a_shrinking_refresh_reaches_the_window() { let mut s = EditorState::new(); @@ -621,20 +608,14 @@ fn s1_10_the_disambiguation_limit_raises_rather_than_adopting() { assert_eq!(mine, "mine", "and touch nothing"); } -/// **Stage 1 criterion 11** --- a **disambiguated** panel still answers +/// Stage 1 criterion 11 [`main`] — a **disambiguated** panel still answers /// `RET`, `g` and `q` (Q#GB18). The framing labels it `[main]` and names /// its bite as *Q#GB13 landed without Q#GB18*. /// -/// **Recorded here as a MUTATION bite, because that is what it is.** On -/// `main` this test fails at its disambiguation *premise* --- `main` -/// adopts the foreign buffer, so the panel is never called -/// `*test-panel*<2>` and the `RET`/`g`/`q` assertions are never reached. -/// A revert therefore proves nothing about what the criterion asserts. -/// The bite the framing actually names is a mutation of this branch: -/// keep the disambiguation, restore a name-keyed `panel_for_buffer`. -/// Verified --- under that mutation the test fails at the `g` assertion. -/// The `[main]` label belongs to #188 and is reported to it; what the -/// tree claims is corrected here either way. +/// On `main` the test first fails at the disambiguation premise because +/// ownership is absent. The framing's narrower pre-image is also pinned: +/// keep disambiguation but restore a name-keyed `panel_for_buffer`, and +/// the test reaches the consumer checks and fails at `g`. /// /// Disambiguation alone leaves the old lookup reading /// `panels["*test-panel*<2>"]` for a record stored under @@ -676,17 +657,13 @@ fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { ); } -/// **Stage 1 criterion 12** --- the `q`-target capture is not inverted +/// Stage 1 criterion 12 [`main`] — the `q`-target capture is not inverted /// (Q#GB18), which needs its own criterion because it fails **open** /// rather than closed. The framing labels it `[main]`. /// -/// **Recorded here as a MUTATION bite**, for the same reason as -/// criterion 11: on `main` this test fails at its disambiguation -/// premise and never reaches the `q`-target assertion, so a revert is -/// not evidence for what it asserts. Under the mutation the framing -/// actually names --- a name-keyed `panel_for_buffer` beside the -/// disambiguation --- it fails at the assertion it exists for, -/// `q must never return into another panel`. Verified. +/// On `main` the ownership premise fails first. With disambiguation kept +/// and only `panel_for_buffer` restored to name-keyed lookup, the test +/// reaches and fails the `q`-target assertion the criterion exists for. /// /// `listview.open`'s guard reads "capture the current buffer as the `q` /// target, but never another panel (chained panels would trap `q` in a diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index fa3fd7d..aeba1ce 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -1109,17 +1109,9 @@ fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { assert_eq!(top, 0, "the collapsed buffer has exactly one line"); } -/// Generated-buffer immutability Stage 1 — the **selection anchor** is a -/// third window coordinate a generated rewrite invalidates, and clamping -/// the cursor alone does not make the region safe. -/// -/// **PROVISIONAL, and named as such.** This pins a defect found in -/// review of PR #191; the rule belongs to Q#GB6, whose approved text -/// (PR #188 revision 5) does not mention the anchor. A revision request -/// carrying this defect is with that lane. This test stands in for the -/// anchor clause of a revised Q#GB6 and must be reconciled with it — -/// including its verdict of *drop* rather than *clamp* — when the -/// revision lands. It is not an independent contract. +/// Generated-buffer immutability Stage 1 criterion 8c [`main`] — the +/// selection anchor is a third window coordinate normalized by Q#GB6's +/// clamp-or-clear rule. /// /// `Window::region` orders `(anchor, cursor)`, so a stale anchor above a /// clamped cursor is still the high end of the region, and @@ -1127,13 +1119,16 @@ fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { /// before the fix: `assertion failed: end <= self.len()` at /// `src/rope.rs:145`, reached from `EditorCore::clipboard_copy`. /// -/// *Bite:* delete the `drop_stale_selection` call from -/// `notify_buffer_edit` and this panics rather than failing an -/// assertion. Note the anchor must be the **high** end: with the anchor -/// low and the cursor high the cursor clamp already covers it, so a -/// forward selection passes with the bug live. +/// Both outcomes matter: clamping a backward selection from 0..30 into +/// 0..2 preserves the shortened region, while clamping a forward +/// selection from 2..30 collapses both endpoints at 2 and clears it. +/// Clearing every stale anchor passes the crash check but fails the +/// first half; clamping without the collapsed check fails the second. +/// +/// *Bite:* delete `clamp_cursor_and_selection` from +/// `notify_buffer_edit`; the first copy reaches the stale-anchor panic. #[test] -fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { +fn acc16h_a_shrinking_generated_write_clamps_or_clears_the_selection() { let state = EditorState::new(); exec( &state, @@ -1164,22 +1159,55 @@ fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { 2, "precondition: the buffer shrank" ); + assert_eq!( + core.active_window() + .selection + .map(|selection| selection.anchor), + Some(2), + "the stale anchor is clamped into the new extent" + ); + assert_eq!( + core.active_region(), + Some((0, 2)), + "a non-collapsed selection survives as the shortened region" + ); + assert!( + core.clipboard_copy(), + "the production consumer copies the valid shortened region" + ); + drop(core); + + // The other result: cursor clamping moves 30 to the anchor at 2, so + // the selected content is gone and no empty active selection remains. + exec( + &state, + r"pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n')", + ); + { + let mut core = state.core.borrow_mut(); + core.begin_selection(2); + core.set_cursor_byte(30); + assert_eq!( + core.active_region(), + Some((2, 30)), + "precondition: a forward 28-byte region" + ); + } + exec(&state, r"pmacs.buffer.set_generated_contents(GEN, 'xy')"); + let mut core = state.core.borrow_mut(); assert_eq!( core.active_window().selection, None, - "an anchor that no longer fits is dropped, not clamped" + "a cursor clamp that collapses the region clears the selection" ); - assert_eq!(core.active_region(), None, "so there is no region left"); - // The production consumer, not just the field: this is the call that - // panicked before the fix. assert!( !core.clipboard_copy(), - "copy must report 'no region' rather than slice past the rope" + "there is no collapsed region to copy" ); } -/// The same anchor gap in the **other** function, driven through its own -/// real Lua path. +/// Criterion 8c's second clamp site, driven through its own real Lua +/// path. /// /// `EditorCore::rebuild_views_for` had the identical defect and is a /// separate exit: the `*help*` renderer rewrites end to end and calls it @@ -1189,15 +1217,13 @@ fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { /// second exit rather than trusting that one call site implies the /// other. /// -/// Same PROVISIONAL status as `acc16h`: the rule is Q#GB6's and its -/// approved text does not yet carry the anchor. -/// -/// *Bite:* delete the `drop_stale_selection` call from -/// `rebuild_views_for` and this panics at `src/rope.rs:145`. `acc16h` -/// stays green under that mutation, which is why this test exists -/// separately. +/// This site also asserts both halves: a clamp can preserve the +/// shortened region, and an anchor that clamps exactly onto the cursor +/// clears it. *Bite:* delete `clamp_cursor_and_selection` from +/// `rebuild_views_for`; the first copy panics at `src/rope.rs:145` while +/// `acc16h` stays green. #[test] -fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { +fn acc16i_a_shrinking_view_rebuild_clamps_or_clears_the_selection() { let state = EditorState::new(); // 286 bytes, then 154: a real shrink through the help renderer. exec( @@ -1227,14 +1253,50 @@ fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { short_len < long_len, "precondition: the help buffer shrank ({long_len} -> {short_len})" ); + let short = u64::try_from(short_len).expect("non-negative"); + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_window() + .selection + .map(|selection| selection.anchor), + Some(short), + "the anchor is clamped to the shorter help buffer" + ); + assert_eq!( + core.active_region(), + Some((0, short)), + "the non-collapsed region survives the rebuild" + ); + assert!( + core.clipboard_copy(), + "copy consumes the clamped region without slicing past the rope" + ); + drop(core); + + // Grow the same help buffer, then choose an anchor that the next + // short render will clamp exactly onto the cursor. + exec(&state, "pmacs.help.show_command('cursor.down')"); + { + let mut core = state.core.borrow_mut(); + let long = u64::try_from(long_len).expect("non-negative"); + core.begin_selection(long); + core.set_cursor_byte(short); + assert_eq!( + core.active_region(), + Some((short, long)), + "precondition: a region whose anchor exceeds the next extent" + ); + } + exec(&state, "pmacs.help.show_command('editor.quit')"); + let mut core = state.core.borrow_mut(); assert_eq!( core.active_window().selection, None, - "rebuild_views_for must drop an anchor that no longer fits" + "an anchor clamp that collapses the region clears the selection" ); assert!( !core.clipboard_copy(), - "copy must report 'no region' rather than slice past the rope" + "the collapsed region is not retained as active-but-empty" ); } From 4bc37ab3864932bb8f89697808f7f7924a9e3b7d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:27:03 -0400 Subject: [PATCH 7/7] docs(active-work): close PR 191 review round 2 Record the revision-7 selection and acceptance reconciliation, the exact code checkpoint, the non-vacuous fan-out bite, and the final gate results. Keep PR 188's proposed status and merge ordering explicit. --- docs/active-work.md | 104 +++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 60 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 48ff54a..7d30f17 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -248,11 +248,12 @@ If it does not, stop and repair the remote/fetch configuration. ## Generated-buffer immutability lane (Arc: workbench primitives) — STAGE 1 OPEN **Framing: [PR #188](https://github.com/levineuwirth/pmacs/pull/188), -approved and still open. #188 owns the acceptance contract; this lane -adopts it.** Where implementing Stage 1 found a criterion impossible or -mislabelled, the finding goes to #188 as a revision request and this lane -waits — it does not restate, narrow or reclassify a criterion locally. -Read the framing as +revision 7, proposed and still open. #188 owns the acceptance contract; +this lane adopts it.** On 2026-07-29 the user explicitly directed #191 +to fold its review corrections into this branch; that authorizes the +Stage 1 implementation delta recorded below, but does not approve or +merge #188 itself. #191 must not merge ahead of the framing's explicit +approval. Read the current contract as `git show githubsucks/generated-buffer-immutability:docs/generated-buffer-immutability-framing.md` until it merges. @@ -282,8 +283,10 @@ until it merges. `bypass_intercept` writes remain in either file); `listview` gains Q#GB13 ownership-by-handle with `<2>`..`<99>` disambiguation and Q#GB18's identity-routed `panels` list in the **same** commit; - Q#GB6's per-coordinate window clamp in - `EditorCore::notify_buffer_edit`; and Q#GB16(a)'s corrected fold + Q#GB6's cursor/view-top clamp plus selection clamp-or-clear in both + `EditorCore::notify_buffer_edit` and `rebuild_views_for`; + listview refresh reseating through the already-notified view rather + than a redundant same-buffer switch; and Q#GB16(a)'s corrected fold status string. No protocol change, no new Lua surface, no new interaction island. - **Why these two families first, and it is not "the cheap half".** @@ -291,42 +294,27 @@ until it merges. seven undo chords to a no-op; `dired.lua` and `listview.lua` rebind **nothing**, so a bare `C-/` emptied a listing and a panel. Stage 1 closes the only two families reachable without `M-x`. -- **Review round 1 closed three findings.** All three are the same - class in the end: *the tree asserting something the record does not - support.* - - **[P1] The selection anchor.** `notify_buffer_edit` clamped - `cursor` and `view_top` but not `win.selection.anchor`, and - `rebuild_views_for` had the same gap. `Window::region` orders - `(anchor, cursor)`, so a stale anchor above a clamped cursor is - still the region's high end: reproduced as - `assertion failed: end <= self.len()` (`src/rope.rs:145`) from - `EditorCore::clipboard_copy`. Fixed by **dropping** the selection, - not clamping it — `window.quit`'s restore already answers this - question the same way (`src/editor_core.rs:3259`). Both exits are - pinned separately (`acc16h`, `acc16i`) and each mutation fails only - its own. **The wording is provisional**: the rule is Q#GB6's, whose - approved text does not mention the anchor, and a revision request is - with #188. - - **[P1] Criteria 5 and 7 were restated locally.** Withdrawn. Both - suites now quote the approved criterion, the criterion-5 tests are - renamed `*_provisional_*` and say they do not satisfy it, and - criterion 7's **"for each adopter"** is restored with the listview - half added back. The evidence stays as evidence and is filed with - #188. - - **[P2] Criterion 12 was labelled a `main` bite.** It is a mutation - bite; on `main` it fails at its disambiguation premise and never - reaches its assertion. Relabelled. -- **Sweep for that class across this branch: 7 sites, 4 named by the - review and 3 found by the sweep.** Class A, a restated or narrowed - contract — the listview criterion-5 doc, **the dired criterion-5 doc** - (a second file the review did not cite), criterion 7's narrowing plus - its **missing listview half**, and this lane's own "two framing - criteria were wrong" bullet. Class B, a bite recorded as a `main` - failure that is really a mutation bite — criterion 12, **criterion 11 - (same defect, not reported)**, and this lane's own revert list, which - said "five criteria" over six items and counted criterion 11 among - them. All 7 corrected. Every remaining `[main]` label was re-run and - fails at its own assertion, not at a premise. +- **Review round 1 found the stale selection anchor and four acceptance + contract mismatches.** Its provisional drop-on-stale fix stopped the + crash but intentionally waited on #188 to decide the selection rule; + criteria 5 and 7 likewise recorded evidence without claiming to + replace the framing. That evidence produced #188 revision 7. +- **Review round 2 closes both remaining P1 findings against revision + 7.** + - **Q#GB6 now matches at both sites.** Cursor and anchor clamp to the + new extent; a selection survives shortened unless an endpoint + movement collapses it, in which case it clears. `acc16h` and + `acc16i` each drive a real caller and assert both the surviving + region and collapsed case. Unconditional drop and bare clamp are + separately falsified. + - **The Stage 1 criteria are adopted without local substitutes.** + Criterion 5 has the exact rope-refusal + byte-identity half and the + Rust-lifted named-intercept half for both adopters. Criterion 7 now + bites the named fan-out mutation for both adopters: listview refresh + no longer rebuilds the view with a redundant same-buffer switch. + Criteria 11 and 12 carry the framing's `[main]` classification and + also record where its narrower Q#GB13-without-Q#GB18 pre-image + fails. - **Stage 2 still owes everything with new Rust in it**, per the framing's cut: `Buffer::apply_generated_edit` + `GeneratedOutcome` + the `{ generated = true }` option + its own `run_buffer_edit` arm; @@ -335,9 +323,9 @@ until it merges. for `compile.lua` and the search panel; Q#GB5's `ensure_slot` lock; conversion of the remaining 13 write sites; and the three `compile_mode_acceptance` intruder tests converted per Q#GB12. -- **Verification.** Run on the merged tree, at the code checkpoint the - next commit records; the ledger commit on top is docs-only and - `cargo fmt --check` + `git diff --check` were re-run after it. +- **Verification at code checkpoint `5d92348`.** The ledger commit on + top is docs-only; `cargo fmt --check` and `git diff --check` are + re-run after it. `cargo fmt --check` clean; `cargo clippy --workspace --all-targets -- -D warnings` clean; library **1,863 passed + 3 ignored** default and **2,048 passed + 4 ignored** CRDT; `listview_acceptance` **17**, @@ -346,9 +334,10 @@ until it merges. `--features crdt` — judge that step by the count, because `acc16e` is `#[cfg(feature = "crdt")]` and a default run never compiles it; M4 **121 passed + 3 ignored + 1 filtered** with `--skip basedpyright`; - required GPU **202/202**; isolated-config full workspace sweep - **3,514 passed across 103 binaries, exit 0**; `git diff --check` - clean. + required GPU **202/202**. The first GPU attempt inside the tool + sandbox failed three managed-attach socket tests and left the + closed-outbox reader blocked; the authoritative rerun outside that + socket sandbox passed all 202. `git diff --check` clean. - **The dired 200 ms perf test is load-sensitive, and the conversion costs it nothing.** Review saw `dired_renders_10k_entries_within_200ms` take 241 ms in a combined run and pass alone. Measured here: 0.09 s @@ -369,23 +358,18 @@ until it merges. `builtin/runtime/listview.lua` for criteria 1, 2, 9 and 10; `builtin/runtime/dired.lua` for criteria 3 and 13a; `src/lua_bindings/fold.rs` for 13b; `src/editor_core.rs` for 8, 8b - and both anchor pins. + and both selection-normalization pins. - **Falsified by a named mutation, each observed to fail:** the fan-out drop in the `set_generated_contents` binding (criterion 7); deleting `self.read_only = false` (criterion 4, both adopters); deleting `add_intercept` and `set_round_trip_input` at each adopter (criteria 5 and 6); the name-keyed `panel_for_buffer` (criteria 11 and 12); adopting at the variant limit (criterion 10); the old fold - status string (13b); deleting each clamp (8, 8b); and **clamping - instead of dropping** the anchor. - - **One recorded VACUOUS result, deliberately.** Criterion 7's - listview half passes under the criterion's own mutation, because - `listview.refresh` and `listview.open` follow `render` with - `window.switch_buffer`, which rebuilds the `TextView` - (`src/editor_core.rs:4854-4868`). Measured, not inferred: the same - mutation run against the listview half alone reports VACUOUS while - the dired half reports BITES. Filed with #188; the half is kept and - labelled rather than deleted. + status string (13b); deleting each clamp (8, 8b); deleting the + selection helper from either site; unconditionally dropping a stale + anchor; and retaining a selection that an endpoint clamp collapsed. + Criterion 7's fan-out drop now fails by assertion in **both** + listview and dired. - **Recovery:** ```sh