diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2bb834..c9a9983 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -203,18 +203,27 @@ jobs: # render job. Set only where the install step ran. # # PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright - # is deliberately NOT installed: that test has no timeout and - # hangs forever (root cause is the non-interruptible reader-thread - # join in `RuntimeHandles::drop`, already a named deferral in - # `src/process.rs`). This job has no `timeout-minutes`, so arming - # it today would trade a vacuous green for a six-hour hang on four - # legs. It gets armed after the hang fix and the CI timeouts land, - # and its own variable exists so that flip is one line. + # is deliberately NOT installed. Both original reasons are now + # gone: the hang's root cause was the stdin-field drop ordering in + # `RuntimeHandles::drop` and is fixed, and this job now carries + # `timeout-minutes`, so a hang could no longer burn six hours. + # The ONE remaining reason is the plain one --- basedpyright is not + # installed here, so arming the variable would fail rather than + # test anything. Installing it (a uv + bundled-node download on + # every leg) is its own decision, not a rider on the hang fix. + # + # PMACS_REQUIRE_SETSID arms the teardown-deadlock unit test. Its + # fixture orphans a grandchild with `setsid --fork`, which is + # util-linux rather than coreutils, so the test skips when the + # binary is absent (a minimal container must not fail `--lib` + # without ever testing pmacs) and this variable is what makes the + # skip fatal where the tool is guaranteed. - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 env: PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_SETSID: ${{ runner.os == 'Linux' && '1' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/README.md b/README.md index 515c259..cd91a87 100644 --- a/README.md +++ b/README.md @@ -220,6 +220,13 @@ translation) are routed through trampolines that exec these tools. shell-locator helper to find `bash` / `zsh` / `fish` for per-shell integration tests. The M7.2 fetcher's timeout test uses `sleep`. +- **`setsid`** (util-linux, Linux only, **optional**). The process + teardown-deadlock test uses `setsid --fork` to orphan a grandchild, + which is the only way to reproduce that deadlock without depending on + shell `&` semantics (they differ between `bash` and `dash`). The test + **skips** when `setsid` is absent, so a minimal or BusyBox environment + still runs `cargo test --lib`; set `PMACS_REQUIRE_SETSID=1` to make + that skip a failure, as CI does on Linux. - **`git`** (added in M7.2). Required for any package operation: the package fetcher shells out to `git` to clone, fetch, and resolve refs, with a deterministic environment diff --git a/builtin/hooks/default.lua b/builtin/hooks/default.lua index 4fabfe9..69b04c3 100644 --- a/builtin/hooks/default.lua +++ b/builtin/hooks/default.lua @@ -16,6 +16,12 @@ -- format-on-save subscribe here. -- * editor.before-quit --- short-circuit. A callback may veto quit -- (e.g. "buffer modified --- save first?"). +-- * resource.renamed --- all-must-succeed (dired Stage 2a). Fired +-- after a successful rename, with (old, new) +-- canonical absolute paths. +-- * resource.deleted --- all-must-succeed (dired Stage 2a). Fired +-- after a successful delete, with the +-- canonical absolute path. -- -- These are *defined* here so user config can attach callbacks via -- pmacs.hook.add. Run sites are in Rust (after-load, after-edit) and in @@ -76,6 +82,32 @@ define { kind = "short-circuit", } +define { + name = "resource.renamed", + description = "Fired once per SUCCESSFUL filesystem rename, with the old " .. + "and new paths as canonical absolute strings. The core " .. + "reconciles what it can reach -- buffer paths and names, the " .. + "URI-keyed LSP stores, attached diagnostic overlays -- but a " .. + "package that keys its own state by path or URI is invisible " .. + "to that, so this hook is the mechanism that scales. It " .. + "carries PATHS rather than a rebind list precisely because " .. + "dired's listing buffers are pathless: a path-keyed consumer " .. + "must be able to reconcile from (old, new) alone. Does not " .. + "fire for a rename that failed or was cancelled.", + kind = "all-must-succeed", +} + +define { + name = "resource.deleted", + description = "Fired once per SUCCESSFUL filesystem delete, with the " .. + "canonical absolute path. Buffers on the path and beneath it " .. + "have already been reconciled: unmodified ones killed " .. + "through both removal phases, modified ones kept alive. " .. + "Subscribers drop their own path-keyed state. Does not fire " .. + "for a delete that failed or was cancelled.", + kind = "all-must-succeed", +} + define { name = "editor.before-quit", description = "Fired before the editor exits. Return false to veto.", 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/fs.lua b/builtin/runtime/fs.lua index 39e3baa..35b25a2 100644 --- a/builtin/runtime/fs.lua +++ b/builtin/runtime/fs.lua @@ -163,6 +163,30 @@ end -- If a package needs at-most-one-pending semantics for mutations, -- it should serialize on the package side (await each op before -- dispatching the next). The fs primitive can't enforce that. +-- +-- **And that is a CORRECTNESS precondition, not only a cancellation +-- one (dired Stage 2a, Q#DR29).** A successful `rename` or `remove` +-- reconciles the editor's path owners in the main-thread drain — buffer +-- paths and names, the URI-keyed LSP state, the `resource.renamed` / +-- `resource.deleted` hooks. That reconciliation is deliberately +-- order-INDEPENDENT: the runtime drains the reply bus with `try_recv` +-- and establishes no execution token, so a worker can finish first and +-- be descheduled before sending, and reply order therefore does not +-- recover filesystem execution order. +-- +-- Independent mutations commute, so nothing is owed for them. But +-- **mutations whose source/target paths overlap must be serialized by +-- dispatching the next only after the previous handle settles.** There +-- is no static ordering rule that would substitute: rename `dir` -> +-- `newdir` racing delete `dir/child.txt` needs delete-then-rename if +-- the delete ran first on disk and rename-then-delete if the rename +-- did, and a fixed "deletes before renames" rule gets one of the two +-- wrong — the kill misses, the rename then rebinds the buffer onto a +-- path whose file is gone, and it survives pointing at nothing. +-- +-- A caller that ignores this owns the residue: a buffer left bound to a +-- stale path, or killed when it should have been rebound. Recoverable +-- and visible, not data loss — but real. function fs.rename(from, to) if type(from) ~= "string" then diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 6a6d717..3c83119 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,14 +255,18 @@ 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 {} 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, } @@ -174,7 +275,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/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index f17827f..d23409b 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1124,7 +1124,7 @@ pmacs.hook.add("buffer.after-edit", function() -- Stale suppression must stay keystroke-accurate even though the -- O(file) didChange send below is coalesced: render families -- anchored to pre-edit positions are hidden from this edit on. - pcall(pmacs.lsp._mark_document_stale, rec.uri) + pcall(pmacs.lsp._mark_document_stale, rec.server, rec.uri) -- Arc 1d: was this edit a typed character? The input-origin signal -- (see the trigger block below). local typed = pmacs.editor.this_command @@ -1437,19 +1437,44 @@ local function apply_workspace_edit(ops) end end if #plan == 0 then return 0, 0, 0 end - local origin = active_buffer_path() + -- G1 — capture the origin BUFFER, not its path. A path captured here + -- is a plain Lua local, and no amount of reconciliation can reach an + -- already-captured local: once the batch renames or deletes the active + -- file, that string names something that is no longer there. The + -- handle follows a rename for free, because the buffer is what moved. + -- + -- The framing's G1 described the failure as a "phantom buffer" created + -- by `resolve_target_buffer`'s NotFound arm. **That is not what + -- happens on this path, and the wrong explanation is recorded here + -- rather than left to be rediscovered.** `pmacs.buffer.find_or_open` + -- calls `crate::file_io::load_file` directly and maps the error, so a + -- missing path RAISES; the NotFound arm belongs to + -- `EditorCore::resolve_target_buffer`, which serves + -- `pmacs.window.display_file` and the startup/daemon target, not this + -- binding. The real defect is quieter: `restore_origin` runs under a + -- `pcall`, so the raise is swallowed and the user is left in whatever + -- buffer the last applied op made active. And when the old path DOES + -- still resolve -- a batch that deletes and then recreates it -- the + -- fallback silently opens a file the user asked to delete. + local origin_buf = pmacs.window.buffer() local edit_total, files, res_ops = 0, 0, 0 -- Plan items fully applied before a failure. Q#RD3 permits partial -- application, so this is what stops a caller claiming "nothing was -- mutated" when something was. local applied_ops = 0 - -- Return the user to where they invoked from — best-effort, since - -- that path may have just been renamed or deleted. Runs on the - -- FAILURE path too (Q#RD7): previously this ran only after a - -- successful loop, so a mid-batch refusal stranded the user in - -- whatever buffer the last applied op left active. + -- Return the user to where they invoked from. Runs on the FAILURE + -- path too (Q#RD7): previously this ran only after a successful loop, + -- so a mid-batch refusal stranded the user in whatever buffer the last + -- applied op left active. + -- + -- **No path fallback (G1).** If the origin buffer is gone — the batch + -- deleted its file and reconciliation killed it — restore NOTHING. + -- "Return the user somewhere plausible" is not worth re-opening a path + -- the batch just destroyed, and when that path has been recreated the + -- fallback would drop the user into a file they asked to delete. local function restore_origin() - if origin then pcall(pmacs.buffer.find_or_open, origin) end + if not origin_buf then return end + pcall(pmacs.window.switch_buffer, origin_buf) end for _, item in ipairs(plan) do local ok, err @@ -2882,3 +2907,191 @@ pmacs.command.define { pmacs.keymap.bind { scope = "global", sequence = "M-g n", command = "diag.next" } pmacs.keymap.bind { scope = "global", sequence = "M-g p", command = "diag.previous" } + +-- Resource reconciliation --------------------------------------------------- +-- +-- dired Stage 2a, §5. A rename or delete moves or destroys a path that +-- FOURTEEN URI-keyed store families, the `documents` mirror, the pending +-- response routes and the attached diagnostic overlays are all keyed by. +-- `EditorCore` reconciles the buffer's own path and name; these two +-- subscribers reconcile the LSP layer, which is buffer-keyed here +-- (`rec.uri` is cached per buffer and read at dozens of sites, so ONE +-- rebind reaches all of them) and URI-keyed in Rust. +-- +-- These subscribers are independent of every other `resource.renamed` +-- consumer by construction: this one touches URI-keyed state, dired's +-- touches its own handle table, and neither reads what the other wrote. +-- That matters because `all-must-succeed` does NOT abort the fan-out — +-- `run_all_must_succeed` collects each callback's error and continues — +-- so a subscriber may not rely on a raising peer to stop the sequence, +-- and the ordered teardown below is ordered INTERNALLY rather than by +-- registration. + +-- Every attachment whose document is `path` or lies beneath it, as +-- `{ key, rec, path }`. Resolved through `path_for_uri` and compared +-- with `paths_related`, so the comparison is component-aware and runs on +-- the same canonical form the buffer registry keys on. +local function attachments_under(path) + local out = {} + for key, rec in pairs(attachments) do + local rec_path = rec.uri and pmacs.lsp.path_for_uri(rec.uri) + if rec_path and paths_related(rec_path, path) then + out[#out + 1] = { key = key, rec = rec, path = rec_path } + end + end + return out +end + +-- How many attributed failures one status line spells out before +-- collapsing the rest into a count. +local RESOURCE_REPORT_LIMIT = 2 + +-- A failure collector for a reconciliation fan-out. +-- +-- **Why this exists rather than a bare `pcall` per step.** Every step +-- below is fallible for reasons outside this file's control -- a stale +-- server id makes `forget_uri` raise, a stopped server makes `did_close` +-- raise -- and an IGNORED `pcall` makes the hook callback RETURN +-- SUCCESSFULLY. `resource.renamed` and `resource.deleted` are +-- `all-must-succeed`, so the registry's error logger is the mechanism +-- that surfaces a failing subscriber; a callback that swallows its own +-- failures gives that logger nothing to log, and the concrete outcome is +-- silent: `forget_uri` fails, the callback carries on, and the old +-- stores, routes and `documents` entry stay live under a URI the editor +-- no longer holds. +-- +-- It must NOT abort the loop. One unreachable server must not leave +-- every other attachment unreconciled, so failures accumulate and are +-- raised once, after every attachment has been processed. +local function failure_sink(hook_name) + local sink = { hook = hook_name, items = {} } + + -- Run `fn(...)`, and on a raise record it attributed to `what`. + -- Returns `ok, value` like `pcall`, so a caller can branch. + function sink:step(what, fn, ...) + local ok, value = pcall(fn, ...) + if not ok then + self.items[#self.items + 1] = string.format("%s: %s", what, tostring(value)) + end + return ok, value + end + + -- Report everything collected, on BOTH channels, and raise. + -- + -- The raise is what the `all-must-succeed` logger needs in order to + -- write an attributed record to *errors*; the status line is what the + -- user actually sees, because stale LSP state looks like the editor + -- quietly breaking. `pmacs.error` is deliberately not used: it is + -- defined only by a test stub, so writing there would reproduce the + -- silence this replaces. + function sink:finish() + if #self.items == 0 then return end + local shown, n = {}, #self.items + for i = 1, math.min(n, RESOURCE_REPORT_LIMIT) do shown[i] = self.items[i] end + local summary = table.concat(shown, "; ") + if n > #shown then + summary = summary .. string.format("; and %d more", n - #shown) + end + pcall(pmacs.editor.set_status, + string.format("LSP %s: %d reconciliation failure%s -- %s", + self.hook, n, (n == 1 and "" or "s"), summary)) + error(string.format("%s: %s", self.hook, table.concat(self.items, "; ")), 0) + end + + return sink +end + +pmacs.hook.add("resource.renamed", function(old_path, new_path) + if type(old_path) ~= "string" or type(new_path) ~= "string" then return end + local sink = failure_sink("resource.renamed") + for _, hit in ipairs(attachments_under(old_path)) do + local key, rec, old_uri = hit.key, hit.rec, hit.rec.uri + -- The buffer's own path was rebound before this hook fired, so ask + -- it rather than reconstructing the tail ourselves. A buffer that + -- somehow lost its path (killed, unbound) cannot be re-opened, and + -- falls through to the teardown-only path below. Not routed through + -- the sink: a pathless buffer is a legitimate state here, not a + -- reconciliation failure. + local ok_path, new_buf_path = pcall(function() return rec.buffer:path() end) + local new_uri = (ok_path and new_buf_path) and file_uri_for(new_buf_path) or nil + + -- 1. Flush any pending didChange for the OLD uri, so the server is + -- not left holding an edit it can no longer attribute. + sink:step("flush didChange for " .. old_uri, flush_did_change_for, rec) + pending_did_change[key] = nil + + -- 2. didClose the old uri — this removes the open-document + -- registration and nothing else. + sink:step("didClose " .. old_uri, pmacs.lsp.did_close, rec.server, old_uri) + + -- 3. Purge the routes, drain their awaiters, and clear all fourteen + -- stores plus `documents` for the old key. Runs against the OLD + -- server, which matters when step 4 picks a different one. + -- A failure here is the one that most needs reporting: the + -- callback would otherwise continue with the old stores, routes + -- and `documents` entry all still live. + sink:step("forget_uri " .. old_uri, pmacs.lsp.forget_uri, rec.server, old_uri) + + if not new_uri then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 4. Re-run ensure_server. Server affinity keys on the detected + -- project root, so a rename ACROSS roots needs a different + -- server; a same-root rename reuses the existing one. + local ok_sid, sid = sink:step("ensure_server for " .. new_buf_path, + ensure_server, rec.language, new_buf_path) + if not (ok_sid and sid) then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 5. didOpen the new uri with the buffer's current text and a + -- fresh version. This also reclaims the tombstone for + -- exactly (server, new uri). + rec.server = sid + rec.uri = new_uri + rec.version = 1 + local ok_text, text = sink:step("read " .. new_uri, buffer_text, rec.buffer) + sink:step("didOpen " .. new_uri, pmacs.lsp.did_open, + sid, new_uri, rec.version, ok_text and text or "") + -- 6. Re-root the diagnostic overlays. `DiagnosticView.uri` is + -- set once at construction and is private, so this is the + -- only way to move it — and the sweep reaches PASSIVE + -- windows, which the attach path cannot, while preserving + -- each overlay's position in the composition order. + sink:step("re-root diagnostics to " .. new_uri, + pmacs.diag._rename_resource, old_uri, new_uri) + end + end + end + -- Raised only after EVERY attachment has been processed: one + -- unreachable server must not leave the rest unreconciled. + sink:finish() +end) + +pmacs.hook.add("resource.deleted", function(path) + if type(path) ~= "string" then return end + local sink = failure_sink("resource.deleted") + for _, hit in ipairs(attachments_under(path)) do + local key, rec = hit.key, hit.rec + -- No flush: the document is gone, and shipping a didChange for a + -- file the server can no longer read buys nothing. + pending_did_change[key] = nil + sink:step("didClose " .. rec.uri, pmacs.lsp.did_close, rec.server, rec.uri) + sink:step("forget_uri " .. rec.uri, pmacs.lsp.forget_uri, rec.server, rec.uri) + -- Drop the record unconditionally, INCLUDING after a failure above. + -- The buffer may be gone entirely (an unmodified visited file is + -- killed), in which case a retained record is a dangling handle that + -- `repull_for_attachments` would iterate; and a modified buffer kept + -- alive has no file to analyze until it is saved, which re-attaches + -- through the ordinary path. Keeping a record whose teardown failed + -- would be strictly worse than dropping it: the failure is reported + -- either way, and a retained one is re-swept every refresh. + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + end + sink:finish() +end) diff --git a/docs/active-work.md b/docs/active-work.md index 3dce858..a863a4f 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -245,6 +245,139 @@ 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 7, approved and merged to `main` as `27b1185`. #188 owns the +acceptance contract; this lane adopts it.** On 2026-07-29 the user +directed #191 to fold its review corrections into this branch and then +merged #188, settling the implementation authority and merge ordering. +The contract is now +`docs/generated-buffer-immutability-framing.md` on canonical `main`. + +- **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 + 27b1185 + $ git log --oneline -1 githubsucks/main + 27b1185 Merge pull request #188 from levineuwirth/generated-buffer-immutability + $ git merge-base --is-ancestor githubsucks/main HEAD && echo "main IS integrated" + main IS integrated + ``` + + **That is a reading, not a constant, and it went stale inside this + lane's own review round.** `main` moved four times while the lane was + open: #187 -> #192 -> #193 -> #188. 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 + Q#GB13 ownership-by-handle with `<2>`..`<99>` disambiguation and + Q#GB18's identity-routed `panels` list in the **same** commit; + 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".** + `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`. +- **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; + `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 `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**, + `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**. 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 + 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 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); 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 + 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-3 OPEN; Stage 2 is COMPLETE with it Stage 1, the Stage 2 framing, Stage 2A, Stage 2B-1, and Stage 2B-2 are @@ -764,21 +897,193 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-rd-impl -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. -## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED +## dired Stage 2a — rename/delete reconciliation — PR #196 OPEN, review round 1 closed + +- Portable branch: `githubsucks/dired-stage2-impl`, worktree + `../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged + as #171 (`docs/dired-stage2-framing.md` rev 9, §5/§6/§10 — the + substrate transaction, no dired surface). Position against `main`, as + pasted command output rather than a remembered constant: + + ``` + $ git merge-base HEAD githubsucks/main + e003b81cdd577140fc77330bd4578d3090696877 + ``` + + That base is the #190 merge, and #190 matters here specifically: + Stage 2a **adopts** its `delete_verdict` refusal rather than + reinventing one, and lifts its walk query out into + `editor_core::buffers_bound_under` so the guard and both + reconciliation seams cannot disagree about which buffers an operation + touches. **Re-measure the merge-base before relying on it** — + `main` has branch protection, all 12 checks must pass on the merging + head, and a conflicting PR builds no merge ref at all, so a green run + from before a move reads as current when it is not. **Re-measured after + round 1: `main` had not moved, so no integration was needed** — that is + a reading of the tree, not a standing fact. +- **What 2b and 2c still owe, stated so the split boundary is auditable.** + 2a ships **no user-visible surface at all** and no dired code: the + `dired_acceptance` count is deliberately unchanged at **25**, and a + moved count there would mean it touched something it should not have. + 2b owes the mark and operation layer (`m u U t d x D R w M`), + `pmacs.minibuffer.confirm` plus its `src/editor.rs` load-sequence line, + `pmacs.killring.push`, dired's own `resource.renamed` subscriber, and + acceptance 1–22, 33, 39–41. 2c owes `mkdir`/`copy`/`remove_dir_all`, + `JobKind` 12 → 15, `dired.recursive-deletes`, and acceptance 42–47. +- **The split boundary has not moved since rev 9.** It was re-checked + against this tree: #188 (generated-buffer immutability Stage 1) did not + convert dired's `paint`, so §3.1's coordination note is still an + obligation of that lane rather than a collision with this one, and + nothing in this diff touches `builtin/runtime/dired.lua`. +- **Two m4 rows were re-pinned, and that is a behaviour change to a + landed lane's assertions.** `rd9` and `rd14` pinned #190's deliberate + restraint on the `apply_resource_op` delete arm — descendants stay + orphaned, only the first of two duplicate path-bound buffers is + reconciled — and both doc comments gave the same reason: widening + would have routed N buffers through `remove_buffer_and_fire`, phase 2 + without phase 1, leaving up to N windows on removed ids. + `EditorCore::reconcile_delete` composes both phases, so the constraint + is discharged and the old assertions became the defect. Each row now + asserts BOTH directions — reconciled away **and** no window holding a + removed id — and each direction is bite-verified. +- **One framing claim is wrong and is corrected at the test, not + silently worked around.** §5's G1 says a stale captured path + "materializes a phantom" by reaching `resolve_target_buffer`'s + `NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls + `crate::file_io::load_file` directly and maps the error, so a missing + path **raises**, and the `NotFound` arm belongs to + `resolve_target_buffer`, which serves `pmacs.window.display_file` and + the startup/daemon target rather than that binding. The defect is real + and smaller: the `pcall` swallows the raise, so the user is stranded + wherever the last applied op left them. Acceptance 34 is restructured + to bite on that (its plan edits another file first, which is what makes + the restore observable at all) and the correction is recorded in the + test's own doc comment. +- **Two bites were vacuous as the framing specified them, and both + reasons are worth keeping.** Item 28's *rename* row cannot pin the + walk's containment rule: `reconcile_rename` calls + `Path::strip_prefix` to rebuild a descendant's tail, and that is + component-aware too, so a string-prefix walk is silently corrected a + second time. The row moved to the **delete** side, where the walk's + verdict IS the kill list. Item 30's composition-order assertion was a + tautology: the LSP attach leaves `diagnostic` **last** in the stack, and + moving the last element to the end is a no-op, so a remove-and-re-push + was indistinguishable from an in-place mutation; the row now pushes one + more overlay after it and asserts that precondition explicitly. +- **23 acceptance criteria are bite-verified by executed mutation**, each + labelled `OK (assertion)` — none merely `OK (COMPILE)`, and none + vacuous. Items 25, 27, 28, 29 (both directions), 30 (both mutations), + 31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51, + 52, 53b, 54, 55, plus the two re-pinned m4 rows in three + configurations. +- **Review round 1 found four defects; all four are fixed, and all four + were the same shape — a failure that left state wrong and told nobody.** + Worth keeping as one lesson rather than four bugs: every one of them + was a `pcall` or a discarded return value, and each *looked* like + defensive coding. + - **P1 — delete refusals were silent.** `reconcile_delete_and_fire` + returned `kept_modified` and `refused` and both production callers + discarded them, so a last-buffer refusal or the asynchronous + modified-buffer race left the file gone and the buffer still bound to + it — and the next `C-x C-s` recreates the deleted file. Reporting + moved **inside the shared seam**, for the same reason the + reconciliation lives there: a caller that has to remember to report + is a caller that will forget. Channel is `EditorCore::status`; + **not `pmacs.error`**, which is defined only by a test stub, so a + report there would have been the same silence. + - **P2 — the LSP subscribers swallowed their own reconciliation + failures.** Ignored `pcall`s made the callback return successfully, + so the `all-must-succeed` logger had nothing to log. A shared + failure sink now attributes each step and raises **after** the loop, + because a fix that aborts on the first failure would leave every + other attachment unreconciled — that wrong fix is itself a + bite-verified mutation. + - **P2 — `forget_uri` left purged requests live in the client.** It + dropped `pending_routes` and `pending_external` but not the ids + `send_request` puts in `LspClient.pending`, and recorded nothing in + `cancelled_rids`, so a server that never replies leaked the entry and + a late reply surfaced as a generic unrouted response. The per-rid + work is now extracted from `drain_cancelled_externals` as + `abandon_request` and **reused** rather than copied. + - **P2 — acceptance 35 was unpinned even after the G1 correction.** + With a plain delete the forbidden path fallback is unobservable: + `find_or_open` raises out of `load_file` and the `pcall` swallows it, + so both assertions passed with the fallback present. The plan now + deletes the origin's file **and recreates it**, which gives the + fallback something to open. The corrected G1 explanation also reached + the production comments, which still repeated the false + `resolve_target_buffer::NotFound` story — *a correction that stops at + the test comment has only half landed.* +- **One round-1 pin passed with its own bug restored, and the reason is + reusable.** Acceptance 53 asserted `contains("only.txt")` for the + buffer-name attribution — but the status line opens with + `deleted only.txt:`, the deleted path's **basename**, so stripping the + attribution changed nothing the assertion could see. Both halves now + assert the buffer's *own* name, which for a path-backed buffer is the + full path and which only the attribution can produce. **A pin written + to close a review finding is exactly the kind that passes with the bug + restored**, and the detector was running the bite rather than reading + the assertion. +- **31 bites now, all executed, every one labelled `OK (assertion)`** — + the original 23 plus 8 for round 1 (report call removed; refusal reason + unattributed; kept-modified name dropped; subscriber failures + swallowed; the wrong fix that aborts the loop; `forget_uri` skipping + `abandon_request`; and the forbidden path fallback restored, which must + fail acceptance 34 **and** 35 independently). +- Verification at this head, each gate run to its own file and its own + exit code checked (never through a pipe): `cargo fmt --check` clean; + `cargo clippy --workspace --all-targets -- -D warnings` clean; + `cargo test --lib` **1,876** passed / 3 ignored; `--lib --features + crdt` **2,061** / 4 ignored; the new + `resource_reconciliation_acceptance` **25** default and **25** crdt; + `dired_acceptance` **25** and **25** crdt, deliberately unmoved; the + frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**, + all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed / + 3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**; + `lsp_dispatch_seams_acceptance` **15**; + `typed_edit_chain_acceptance` **13**; `journey_acceptance` **24** + (the ratchet floor, asserted as a count rather than a colour); + `gpu_invocation_acceptance` **15** crdt — **and that number is only + real with `pmacs` and `pmacs-gpu` built first**, which is the `a37` + trap in §5: the same command reported 12 failures before the build and + 15 passes after, so a red run there is not evidence of a regression + until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p + pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with + `--no-fail-fast` **3,559** passed across **104** suites, 19 ignored, 0 + failed; `git diff --check` clean. Every one of those was run as its own + step with its own exit status checked — never `cmd | tail` inside an + `&&` chain, which returns *tail's* status and has masked a real failure + in this repo before. +- **Ownership, per the framing's own warning.** §16 says 2a must not run + concurrently with **Journey Stage 1b**, because 1b's LSP + spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s + attachment lifecycle and 1b's compile/binding half touches + `src/editor_core.rs` — the same two files 2a rewrites, where the + conflicts are semantic rather than textual so a clean `git merge` + proves nothing. **1b must not be started while this PR is open.** No + other lane in flight touches them: #188 is `dired.lua`/`buffer.rs` + generated-buffer writes, and the bottom-panel and CI lanes are + elsewhere. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-dired-s2 + -b dired-stage2-impl githubsucks/dired-stage2-impl`. + +## Generated-buffer immutability framing lane — MERGED AS PR #188 - Portable branch: `githubsucks/generated-buffer-immutability`; worktree - `../pmacs-generated-immutability`. **PR #188**, base `main`, forked from - `githubsucks/main` @ `ad41cf1`, **integrated through `5e186c7`** — + `../pmacs-generated-immutability`. **PR #188 landed on `main` as + `27b1185` on 2026-07-29**, after forking from + `githubsucks/main` @ `ad41cf1` and integrating through `5e186c7` — #189 (clean), then #186 and #171 (`docs/active-work.md` conflict), then #187 (the same file again, after it removed the two landed framing lanes), #192 at merge commit `76cfaac`, and #193 (`docs/active-work.md` conflict again) after revision 7's first push. Revision 6 was reviewed at head `55c3061`; revision 7 closes that - round. Framing only — + round. The retained branch is provenance only. Framing only — `docs/generated-buffer-immutability-framing.md`, revision 7, plus this lane. **No runtime code, no protocol change.** -- **PROPOSED — six review rounds closed (thirty-two findings, - twenty-two P1, ten P2). Not approved. Do not implement, do not merge.** +- **APPROVED and merged after six review rounds** (thirty-two findings, + twenty-two P1, ten P2). Revision 7 is the governing contract. - **Stage 1 implementation is PR #191, open. The boundary is explicit and has already been needed twice:** #188 owns the **acceptance contract**; #191 **adopts** criteria and may not restate, narrow, or @@ -855,7 +1160,7 @@ has **no branch and no framing yet**. panel's four, compile/search ownership + routing, the path-backed refusal plus `mark_clean`, and the terminal-only `identity_protected` guard. **No Lua unlock ships.** -- **Nine facts from this lane that other lanes need before it merges:** +- **Nine facts this lane landed for other lanes:** - **`bypass_intercept` is the wrong inventory key.** It misses `*buffer-list*`, `*help*` and `*workers*`, which are generated with plain writes and no intercept at all. `docs/agent-handoff.md` §4's @@ -1062,6 +1367,7 @@ has **no branch and no framing yet**. at 42 insertions against 88 deletions — merging it would *revert* current documentation. The section said "whoever confirms the branch carries nothing unique removes the section"; this is that. + ## Test-improvement arc, lane 3a — CI timeouts and concurrency - Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree @@ -1148,6 +1454,96 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-ci3 -b ci-timeouts-concurrency githubsucks/ci-timeouts-concurrency`. +## Test-improvement arc, lane 4 — process teardown stdin deadlock + +- Portable branch: `githubsucks/process-teardown-stdin-deadlock`, + worktree `../pmacs-hang`. Implements + `docs/process-teardown-stdin-deadlock-framing.md` (rev 3: one review + round, then a CI round that falsified the reproduction). +- **Base, measured rather than quoted:** + + ``` + $ git log --oneline -1 githubsucks/main + e003b81 Merge pull request #190 from levineuwirth/resource-op-delete-guard-impl + ``` + +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-hang + -b process-teardown-stdin-deadlock + githubsucks/process-teardown-stdin-deadlock`. +- **The defect:** `RuntimeHandles::drop` joined its reader threads in + the `Drop` **body**, which runs before any field drops. The + `ChildStdin` sink lives in the `stdin` **field**, so it could only be + released after the join returned — and the join waited on readers + blocked in `read()` on pipes whose write ends the child still held, + because the child never got the stdin EOF that would have made it + exit. A closed cycle inside one function; teardown hung forever. +- **This is the root cause of the `m4_5_basedpyright` hang** that has + parked `--workspace` sweeps (once for 2h26m) and forced + `-- --skip basedpyright` into every gate recipe. The handoff's §3 + claim that the desktop's binary was broken is **retired by this PR**: + the binary was fine. `basedpyright-langserver` is a uv console script + that runs bundled `node` via `subprocess.run` and **waits**; at + teardown `shutdown()` SIGTERMs the recorded pid (the wrapper), which + dies without forwarding, and **that** orphans node to `PPid: 1` + holding the pipes. A direct binary like `clangd` is a genuine child + whose pipes close on reap. That is the whole of the "intermittent" + story. +- **Corrected in review round 2:** rev 1–3 said the wrapper "spawns node + and exits". Wrong — and refutable from evidence already in hand, since + the initialize handshake succeeds, which a wrapper that exited at spawn + could not have done. The `PPid: 1` observation was taken *after* + `shutdown()` had killed the wrapper. **We create the orphan.** The fix + is unaffected; the parked follow-up changes from "tolerate + self-orphaning servers" to "stop orphaning them" (signal the group). +- **Diagnosis method, because reproduce-first was the instruction:** + gdb thread stacks plus `/proc` fd forensics on a live wedged process, + both pipe ends identified in both processes, reproduced 5/5. Three + earlier reproductions were vacuous — see the handoff §5 lesson; the + shipped test carries two positive controls because of it. +- Verification (each gate its own step, real exit status, no + `cmd | tail`): fmt 0; `git diff --check` 0; clippy 0; `--lib` 1864 + passed; `--lib --features crdt` 2049 passed; **`m4_acceptance` + without the skip 150 passed in 2.66s with the basedpyright test + `ok`**; the **eleven** PTY/REPL/worker/panel suites of the framing's + Bet 2 all 0 (144 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202 + passed. Bite verified by revert: `ok` in 2.03s with the fix, FAILED on + timeout at 10.00s without it, both controls passing first. +- **CI round 1 falsified the reproduction, and the control is what + caught it.** Three Test legs failed on `9b1cf3d`'s predecessor: the + synthetic child used `sh -c 'cat <&0 & exit 0'`, and `<&0` does not + defeat the `/dev/null` rule it was chosen for — the rule applies + *before explicit redirections*, so fd 0 is already `/dev/null` and the + redirect duplicates it onto itself. `bash` skips the default when a + stdin redirect is present; **`dash`, which is Ubuntu's and CI's + `/bin/sh`, does not.** Local probing through `/bin/sh` could not see + it. Now `setsid --fork cat`, with no shell at all. **Lesson recorded in + the handoff §5: never probe shell behaviour through `/bin/sh` — name + the implementation.** +- **`acc28` on macos/lua54 was a flake, established not assumed.** + `bottom_panel_stage1_acceptance::acc28` failed once on that leg; + rerunning the same job on the *identical* head passed, and the suite is + 46/46 locally. It is now in Bet 2's falsifier list — its absence from + rev 1 was a real gap, since it drives real child input through a PTY in + a panel and this PR changes PTY-mode teardown ordering. +- **Not fixed here, parked in the framing §5:** cancellable non-group + `read` (covers a child that ignores EOF, and one that stops draining + while `write_all` is blocked); the orphaned-server **leak** — post-fix + the server exits by cooperation, not enforcement. +- `CLAUDE.md`'s `--skip basedpyright` entry is deliberately untouched. + Dropping it is a separate proposal owed evidence of repeated green. + The timeout precondition is **already satisfied** — #195 (this PR's + base) gave every job a `timeout-minutes` — so the only remaining reason + `PMACS_REQUIRE_PYRIGHT` stays unarmed is that CI does not install + basedpyright at all; arming it would fail rather than test anything. +- Adds `PMACS_REQUIRE_SETSID`, armed on Linux. The teardown test's + fixture needs `setsid --fork`, which is util-linux rather than + coreutils, so it **skips** when absent (the standard `--lib` gate must + not hard-fail a minimal container on an undeclared tool) and the + variable makes that skip fatal where the tool is guaranteed. Both arms + verified against a PATH with `setsid` genuinely removed: unarmed skips, + armed FAILS. README's test-dependency list declares it. + ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 9df0d8d..e381de0 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1377,10 +1377,32 @@ git diff --check Machine-specific caveats — re-verify on a machine you haven't used before trusting them: -- **basedpyright**: the DESKTOP's local binary is broken and HANGS the - `m4_5_basedpyright` tests — hence the `--skip` there. The LAPTOP has - a working basedpyright 1.39.9 (verified 2026-07-10: the m4_5 test - passes in 0.18s), so the skip is droppable on the laptop. +- **basedpyright**: the desktop binary was **never broken** — this was a + real code defect, diagnosed and fixed 2026-07-29 (see §5, "A `Drop` + body runs before its fields"). `RuntimeHandles::drop` joined its reader + threads before the `stdin` field dropped, so the server never got stdin + EOF, never exited, and kept the output pipe the readers were blocked + on. Deterministic on the desktop, invisible on the laptop and in CI, + which is why it read as a broken local binary for weeks. + **How the orphan is actually made — WE make it.** basedpyright's + console script runs bundled `node` through `subprocess.run` and + **waits** (`nodejs_wheel/executable.py:50`, verified in 1.39.6). At + teardown `shutdown()` SIGTERMs the *recorded* pid — the Python wrapper + — which dies without forwarding the signal, orphaning node to `PPid 1` + holding the pipes. An earlier revision of this entry said the wrapper + "spawns node and exits"; that was wrong, and the refutation was already + in hand, since the initialize handshake succeeds, which a + wrapper that exited at spawn could not have done. The consequence is + for the follow-up, not the fix: the orphan-management work is **stop + orphaning them** (signal the group), not tolerate self-orphaning. + The `--skip` above stays for now: it is still correct on any tree + predating the fix, and — the one live reason — **CI never installs + basedpyright at all**, so arming `PMACS_REQUIRE_PYRIGHT` would fail + rather than test anything. The two original reasons are both gone: the + hang is fixed, and #195 gave every job a `timeout-minutes`, so a hang + can no longer burn six hours. Installing basedpyright in CI (a uv plus + bundled-node download per leg) and dropping the local skip are two + separate proposals, each owed its own evidence. - **GPU on the laptop**: AMD Radeon 780M (RADV) — native Vulkan, `PMACS_REQUIRE_GPU=1` works without lavapipe. - **Flaky-under-load tests — rerun isolated before treating a sweep @@ -1547,6 +1569,47 @@ round-trip cannot detect a discriminant shift. asks you to keep. Same family as the skip-reports-`ok` lesson below and the double-invocation traps: **the thing that summarizes a gate must not be able to lose the gate's verdict.** +- **A reproduction is a measurement, and needs its own positive control.** + The basedpyright-hang lane wrote **four** reproductions that passed + against the *unfixed* tree, each vacuous for a different reason: the + child exited before the join; the child never read stdin at all; the + child's stdin was silently rebound to `/dev/null` (POSIX XCU §2.9.3 + assigns `/dev/null` to an asynchronous list's stdin when job control is + off, so `sh -c 'cat & exit 0'` EOFs instantly); and then **the repair + for that was also wrong** — the rule applies *before explicit + redirections*, so `<&0` duplicates `/dev/null` onto itself. `bash` + skips the default when a stdin redirect is present, `dash` does not, so + `<&0` passed locally and failed in CI. The shipped test uses + `setsid --fork`, removing the shell from the reproduction entirely. + Every one of the four looked obviously right when written, and the + fourth was verified locally before it failed. Note what a narrower rule + would have missed: "check the child is still alive" catches only the + first. Only the general form catches all four — **and the ones nobody + has invented yet.** Note also which mechanism caught the fourth: not a + reviewer, but the control itself, failing loudly in CI and naming its + own cause. So: assert the precondition your reproduction + depends on, in the test, before exercising the thing under test. In + `teardown_closes_stdin_before_joining_readers` that is two controls + (the recorded child has exited; both readers are still blocked in + `read`), each with a failure message naming what its absence means — + and a `/bin/sh` that is `bash` locally and `dash` in CI is exactly the + sort of divergence no amount of local verification reaches. + This is the same rule that produced #192's bite positive control and + #194's re-read-the-artifact lesson, stated at full generality: **a + measurement you have not controlled is a claim, not evidence.** +- **A `Drop` body runs before its fields, whatever the declaration + order.** Cost a multi-week misattribution: `RuntimeHandles::drop` + joined its reader threads in the drop *body*, while the `stdin` sink it + needed to close first sat in a *field* — reachable only after that body + returned. The child never got EOF, never exited, and kept the output + pipe the readers were blocked on, so teardown hung forever. Reordering + the struct's fields cannot fix this shape; the operation has to move + into the body. Generally: **if a `Drop` body waits on anything, check + what the waited-on party needs that only a field drop will release.** + Corollary from the same investigation — `cancel`-flag style wake-outs + only work where the thread actually polls them; a thread blocked in a + raw `read` never sees one, so a flag next to a blocking syscall is + documentation, not a mechanism. - **A test that skips on a missing precondition reports `ok`, and a gate log cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the only acceptance driving a real daemon, a real PTY and a real wgpu render diff --git a/docs/generated-buffer-immutability-framing.md b/docs/generated-buffer-immutability-framing.md index c8c6b63..71439cb 100644 --- a/docs/generated-buffer-immutability-framing.md +++ b/docs/generated-buffer-immutability-framing.md @@ -1,7 +1,7 @@ # Generated-buffer immutability -**PROPOSED — needs explicit user approval before implementation. DO NOT -implement, DO NOT merge.** +**APPROVED and MERGED as PR #188** (`main` @ `27b1185`, 2026-07-29). +Stage 1 implementation is PR #191. **Revision 7 — answers review round 6 on `55c3061`, authored against canonical `githubsucks/main` @ `64883eb` and integrated through diff --git a/docs/process-teardown-stdin-deadlock-framing.md b/docs/process-teardown-stdin-deadlock-framing.md new file mode 100644 index 0000000..d21edb9 --- /dev/null +++ b/docs/process-teardown-stdin-deadlock-framing.md @@ -0,0 +1,561 @@ +# Framing — close child stdin before joining readers (process teardown deadlock) + +A pipe-mode child that exits on stdin EOF can deadlock the supervisor's +teardown forever. `RuntimeHandles::drop` joins its reader threads in the +`Drop` body, which runs **before** the `stdin` field drops, so the child +never receives the EOF that would make it close the very pipe write ends +those readers are blocked on. The fix is a two-line reorder that reuses a +mechanism already present in this file. + +This is the diagnosed root cause of +`m4_5_basedpyright_initializes_and_negotiates_encoding` hanging +indefinitely — the hazard that has parked `cargo test --workspace` runs +(once for 2h26m) and forced `-- --skip basedpyright` into every gate +recipe. + +**Scope: `src/process.rs` only. No protocol change. No Lua surface. No +new primitive.** + +--- + +## Revision history + +- **rev 1** — initial framing. Root cause established by live diagnosis + (gdb stacks + `/proc` fd forensics on a wedged process), reproduced + 5/5 deterministically at `e003b81`. +- **rev 2** — review round 1. rev 1's synthetic child was **itself + vacuous** (the third in this lane): POSIX assigns `/dev/null` to a + background job's stdin when job control is off, so `sh -c 'cat & + exit 0'` EOFs instantly and exits against the *unfixed* tree. + Q#TD6 now uses the explicit-redirect form and criterion 2 gains a + positive control. Also: Q#TD3's bound widened to cover a blocked + stdin writer (a child that read stdin but stopped draining it), + criterion 5 extended to `docs/agent-handoff.md` §3, Bet 4 marked as + lane-stopping. +- **rev 4** — review round 2. §1.7's causal account was **wrong**: the + basedpyright wrapper uses `subprocess.run` and *waits*; the orphan is + created by pmacs SIGTERMing the wrapper at shutdown, not by the wrapper + exiting at spawn. Corrected here, in the handoff and in the ledger, and + §5's P2 restated — the follow-up is "stop orphaning them", not "tolerate + self-orphaning". Also: the `setsid` dependency is now skip-unless-armed + rather than a hard assert, since it is util-linux and the standard + `--lib` gate must not fail on an undeclared tool. +- **rev 3** — CI falsified rev 2's repair. `<&0` is defeated on `dash` + (the rule applies *before* explicit redirections, so `<&0` duplicates + `/dev/null` onto itself); it passed locally only because `/bin/sh` here + is `bash`. **Control 2 caught it in CI and named its own cause** — the + fourth vacuity in this lane, and the first one a control found instead + of a reviewer. The reproduction now uses `setsid --fork cat`, removing + the shell entirely. Bet 2's falsifier list also gained + `bottom_panel_stage1_acceptance`, which holds PTY-in-panel tests and + was a genuine gap in rev 1's list. + +--- + +## 0. Coherence impact (COHERENCE §20) + +This is a defect fix, not coherence work, and it should not claim +otherwise. + +- **Journey steps touched:** none directly. It protects the steps that + depend on a live language server (§2 step 6 onward) from an unbounded + teardown, but it adds no journey surface. +- **Interaction islands added:** none. +- **Config registry:** no new options. +- **Background-work attribution:** unchanged. The supervisor's process + model is untouched; only the order of two teardown operations moves. +- **Protocol:** unchanged. + +The one genuine coherence connection is indirect and worth stating +plainly: the hang parks `cargo test --workspace`, which is the ratchet +every COHERENCE priority is verified against (§19, §25). A gate that can +hang forever degrades every other lane's evidence. That is the argument +for doing this now rather than parking it — not a claim that it advances +a priority. + +--- + +## 1. Ground truth (scouted @ `e003b81`) + +Line numbers are hints; symbols are authoritative. + +### 1.1 The reproduction is deterministic, not intermittent + +`docs/agent-handoff.md` and the test-improvement audit both describe this +hang as intermittent. On a machine where `basedpyright-langserver` +resolves to a uv-installed shim it is **completely reliable**: 5 runs, 5 +hangs, via + +``` +cargo test --test m4_acceptance -- --exact \ + m4_5_basedpyright_initializes_and_negotiates_encoding +``` + +§1.7 explains why it looks intermittent across machines. The practical +consequence: this defect is directly testable, and any fix has a +revert-bite. + +### 1.2 The cycle, in five links + +Observed stack of the wedged test thread (gdb, `sudo` required — +`ptrace_scope=1`): + +``` +tests/m4_acceptance.rs:1374 Rc> dropped +→ ProcessSupervisor::drop src/process.rs:1545 +→ ProcessSupervisor::shutdown src/process.rs:1463 +→ ProcessSupervisor::tick src/process.rs:1188 +→ ProcessSupervisor::poll_one src/process.rs:1268 (drop site: :1337) +→ RuntimeHandles::drop src/process.rs:631 +→ JoinHandle::join ← blocked, indefinitely +``` + +The links: + +1. **`RuntimeHandles::drop` (`:631`)** sets `cancel`, then joins every + handle in `self.readers`. +2. **Rust runs a type's `Drop::drop` body before dropping its fields.** + `stdin: Option` is a *field* (`:505`), so it cannot drop + until the body returns. The body never returns. +3. **`StdinWriter` (`:556`)** holds the `Sender`; `StdinWriter::spawn` + (`:568`) moves the `ChildStdin` sink into its thread, which drops the + sink only once `rx.recv()` errors. Sender alive ⇒ sink alive ⇒ **the + child's stdin write end never closes.** +4. **The child therefore never sees EOF**, stays alive, and keeps the + stdout/stderr **write** ends it inherited. +5. **The readers are blocked in `read()`** at `:1886` inside + `spawn_reader` (`:1874`). `cancel` is consulted only at the loop top + (`:1883`) and around `send_timeout` — **never while `read` is + blocked.** + +Verified on the live process: the test held fd 4 (child stdin, WRONLY) +and fds 5 and 7 (stdout/stderr, RDONLY); the server process held the +matching opposite ends on fds 0, 1, 2. Two reader threads sat in +`anon_pipe_read`, and the stdin-writer thread sat parked in +`Receiver::recv` at `:577` — alive, still owning the sink. + +Confirmation from the other direction: when the wedged test process was +killed, its fd 4 closed, the server immediately saw stdin EOF and +exited. The cycle's load-bearing link is exactly the one the fix cuts. + +### 1.3 The existing comment names the false premise + +`RuntimeHandles::drop` documents its own reasoning: + +> Wake any reader thread blocked in a bounded `send` — dropping the +> master closes the kernel pipe and unblocks `read`, but does nothing +> for a reader stuck on a full channel […] + +The premise is true for a **PTY master** and false for **pipe mode**, +where `read` unblocks only when *every* write end closes. `cancel` was +introduced for the full-channel case and is correct for it; the comment +mistakenly treats the `read` case as already handled. + +### 1.4 `shutdown()`'s SIGKILL phase is unreachable on this path + +`shutdown()` (`:1463`) sends SIGTERM to all ids, then runs a bounded +grace loop (`deadline` at `:1476`) that calls `tick()`, and *then* +escalates to SIGKILL. The stack shows the deadlock occurs **inside that +grace loop's `tick()`**, because `poll_one` drops `RuntimeHandles` the +moment it observes the recorded pid exited. The SIGKILL phase is never +reached. + +So "shutdown force-kills everything first" is not true of this path. +(An earlier working assumption of mine said it did; the stack refutes +it.) Even if reached, SIGKILL targets the *recorded* pid, which per +§1.7 is not the surviving process. + +### 1.5 Only pipe-mode, non-group spawns are affected + +`spawn_pipes` (~`:1712`) chooses per stream: + +| `spec.group` | reader | cancellable mid-`read`? | +| --- | --- | --- | +| `true` | `spawn_group_reader` (`:1941`) — `O_NONBLOCK` + `poll` | **yes** | +| `false` | `spawn_reader` (`:1874`) — blocking `read` | **no** | + +PTY mode (~`:1724`) also uses `spawn_reader`, but there §1.3's premise +holds: dropping the master genuinely ends the read. The `spawn_ansi_parser` +reader also lives in `readers`, and reads a channel rather than an fd, so +it is unaffected. + +Non-group pipe consumers are, per `spawn_reader`'s own doc comment, the +**REPL and LSP** paths. This defect is therefore reachable by every LSP +server and every REPL — not by terminals. + +### 1.6 The fix mechanism already exists in this file + +`close_stdin` (`:1611`) already does precisely what is needed, and +already documents the semantics and the idempotence: + +```rust +// Dropping the writer closes the pipe at the kernel +// level. `take()` is idempotent — second call sees None. +let _ = runtime.stdin.take(); +``` + +The fix is applying an existing, already-reviewed mechanism at the one +site that is missing it. It introduces no new concept. + +### 1.7 Why basedpyright wedges and clangd/gopls do not + +`basedpyright-langserver` is a uv-installed **Python console script**: + +```python +from basedpyright.langserver import main +sys.exit(main()) +``` + +`main()` reaches `run_node.run`, which calls `nodejs_wheel`'s `node(...)` +— and that is **`subprocess.run`** (`nodejs_wheel/executable.py:50`). It +**waits**. Verified in the installed 1.39.6 source, not assumed. + +So the wrapper does *not* exit at spawn time, and **pmacs creates the +orphan itself**: + +1. The wrapper runs `node …/langserver.index.js --stdio` and blocks. Node + is a genuine grandchild; the initialize handshake completes normally. +2. At teardown, `shutdown()` sends **SIGTERM to the recorded pid** — the + Python wrapper — before entering its grace loop. +3. The wrapper dies on the default disposition and **does not forward the + signal**. Node is reparented to `PPid: 1`, still holding the inherited + pipes, idle in `ep_poll`. +4. `poll_one` then observes the recorded pid terminated, drops + `RuntimeHandles`, and enters the deadlock. + +**rev 1–3 of this doc said the wrapper "spawns node and exits".** That was +wrong, and the evidence against it was already in hand: the test's +assertions all pass *before* teardown, so the handshake succeeded — which +is impossible if the wrapper had exited at spawn. The observation that +generated the claim (`PPid: 1`, wrapper gone) was taken **after** +`shutdown()` had already killed it. + +This matters for the parked work, not for the fix. The follow-up is not +"tolerate servers that self-orphan" — it is **stop orphaning them**: +signal the process group rather than a wrapper pid that swallows the +signal. P2 in §5 is restated accordingly. + +`clangd` and `gopls` are real binaries: genuine children, reaped +normally, write ends closed, blocking `read` returns `Ok(0)` cleanly. The +"intermittency" in the handoff is not timing — it is *which server binary +is installed how*. + +### 1.8 Limits of the evidence + +- The deterministic reproduction is **one machine, one server**. The + causal chain is verified there link by link; its generality to other + shim-launched servers is reasoned, not measured. +- The gdb capture is a single sample of a state that was stable across a + four-minute window and identical across two independent runs. That is + strong for a deadlock and would be weak for a race. +- Nothing here establishes how often the hang has fired in CI. CI never + installs basedpyright (`PMACS_REQUIRE_PYRIGHT` is deliberately never + set, #194), so in CI this test skips and the defect is **dark**. Every + observation is local. + +--- + +## 2. Decisions + +### Q#TD1 — the fix is a reorder inside `Drop`, not a new primitive + +```rust +impl Drop for RuntimeHandles { + fn drop(&mut self) { + self.cancel.store(true, Ordering::Relaxed); + // Close the child's stdin BEFORE joining. A stdio child exits + // on EOF and closes its stdout/stderr write ends, and that — + // not `cancel` — is what unblocks a reader parked in `read` + // (`cancel` is only observed between reads and around `send`). + // The sink lives in the `stdin` field, which cannot drop until + // this body returns, so joining first deadlocks against it. + let _ = self.stdin.take(); + for h in std::mem::take(&mut self.readers) { + let _ = h.join(); + } + } +} +``` + +Rejected alternative: reordering the struct's *fields*. Field order does +not help — the explicit `Drop::drop` body runs before **all** fields +regardless of their declaration order. This is the trap that makes the +bug non-obvious, and it belongs in the comment. + +### Q#TD2 — the reorder is unconditional across modes + +Applying it only to pipe+non-group would require `RuntimeHandles::drop` +to learn which mode it is in, which it currently does not need to know. +Closing stdin before teardown is correct in both modes, so the reorder is +unconditional. + +This is a uniformity change, and uniformity changes in this repo have +made total functions partial before. It is therefore carried as a **bet +with a named falsifier** (§3, Bet 2), not as an assumption: PTY-mode +`stdin` is the pty *writer*, and dropping it while `pair.master` and the +cloned reader still exist must not end the read early. + +### Q#TD3 — the fix assumes the child drains stdin to EOF, and covers nothing outside that + +Stated up front because it bounds the claim: the fix works by making the +child exit. A child that never reads stdin — or reads it and ignores EOF +— keeps its write ends open and still wedges the join. + +There is a third member of that family, and it is not covered by the +wording above because such a child *did* read stdin: **the EOF is only +delivered if the writer thread reaches the end of its queue.** Its body +is a blocking `sink.write_all(&bytes)` (`:578`), so a child that has +stopped draining stdin while queued bytes remain blocks the writer +indefinitely — the sink never drops, EOF never arrives, and the join +re-wedges. This needs only a full stdin pipe buffer at teardown time, not +a misbehaving child. For LSP teardown the queue is near-empty and the +practical risk is nil, but the bound belongs in the claim: **the fix +assumes the child keeps draining stdin until EOF.** A full stdin pipe +with a non-draining child is P1's case as well. + +Covering *that* case requires making the blocking `read` itself +cancellable, i.e. moving non-group readers onto `spawn_group_reader`'s +`O_NONBLOCK` + `poll` mechanism. `spawn_reader`'s doc comment already +names this as a deferral from the compile-mode framing. It stays parked +(§5, P1) rather than riding this PR, because it is a behavioural change +to every REPL and LSP ingest path and deserves its own review. + +The honest claim for this PR is therefore: **it fixes the observed +deadlock for stdio children that honour EOF, which is what LSP servers +are, and narrows — not eliminates — the class.** + +### Q#TD4 — queued stdin writes are not lost, and the writer is not joined + +`crossbeam`'s `Receiver::recv` drains buffered items before reporting +disconnection, so dropping the `Sender` still lets the writer thread +write everything already queued. The writer thread is **not** joined +here, so there remains no guarantee the final flush completes before the +process is signalled. That is pre-existing, unchanged by this PR, and +noted rather than fixed (P3, §5). + +Draining is also the mechanism by which the fix can fail to deliver EOF +at all when the child has stopped reading — see Q#TD3's third case. + +### Q#TD5 — the leaked orphan server is not fixed here + +After the fix, the wedge is gone but a shim-launched server is still an +orphaned grandchild that teardown's recorded pid cannot signal. It exits +here only because it honours stdin EOF — by cooperation, not by +enforcement. A server that ignores EOF leaks. Parked (§5, P2). + +### Q#TD6 — the synthetic reproduction must model EOF-honouring, not sleeping, and needs an explicit stdin redirect + +Two distinct traps here, and this lane has now walked into **three** +vacuous reproductions, so the reasoning is recorded rather than the +conclusion alone. + +**Trap 1 — a sleeping child models the wrong defect.** +`sh -c 'sleep 300 & exit 0'` orphans a grandchild that holds the write +ends but **never reads stdin**, so closing stdin does not free it. That +reproduces a hang this fix does *not* address; it belongs to P1 (§5), not +here. + +**Trap 2 — a background job does not inherit stdin.** POSIX XCU §2.9.3: + +> If job control is disabled, the standard input of an asynchronous +> list, before any explicit redirections, shall be assigned to +> `/dev/null`. + +Job control is off in every non-interactive `sh`, so in +`sh -c 'cat & exit 0'` the background `cat` gets **`/dev/null`**, not the +inherited pipe. It EOFs immediately and exits **against the unfixed +tree** — the test would pass either way and Bet 3's revert-bite would +report VACUOUS. + +Measured on this machine (`/bin/sh` → `bash`), stdin attached to a +held-open fifo, checking the orphan's `/proc//fd/0`: + +| form | grandchild | fd 0 | +| --- | --- | --- | +| `sh -c 'cat & exit 0'` | **gone** | — (EOF'd from `/dev/null`) | +| `sh -c 'cat <&0 & exit 0'` | alive | the real pipe | + +**Trap 3 — `<&0` does not repair it, and the obvious fix is wrong.** rev 2 +proposed `sh -c 'cat <&0 & exit 0'`, verified on this machine. **CI +falsified it.** Re-read the rule: `/dev/null` is assigned *before any +explicit redirections*, so by the time `<&0` runs, fd 0 already **is** +`/dev/null`, and the redirect faithfully duplicates it onto itself. +`bash` happens to skip the default when a stdin redirect is present; +`dash` — Ubuntu's `/bin/sh`, and CI's — does not. Measured: + +| shell | form | grandchild | fd 0 | +| --- | --- | --- | --- | +| bash | `cat & exit 0` | gone | — | +| bash | `cat <&0 & exit 0` | alive | real pipe | +| dash | `cat <&0 & exit 0` | **gone** | — (CI: control 2 failed) | + +The local probe could not have caught this: `/bin/sh` here is `bash`. + +**The model is therefore `setsid --fork cat`, with no shell at all.** +`setsid --fork` forks, the parent exits, and the child inherits +stdin/stdout/stderr untouched — no asynchronous list, no `/dev/null` +rule, no implementation variance. The recorded pid (`setsid`) terminates +promptly so `poll_one` reaches the teardown path, while `cat` survives +holding the inherited pipes and exits on EOF exactly as a stdio language +server does. Unfixed, this deadlocks; fixed, teardown completes. + +`setsid(1)` is util-linux, which the Linux gate already assumes. +Presence is **asserted, not skipped** — a skip would reintroduce the +silent-green shape lane 2 removed. + +The controls are what make this recoverable rather than a silent +regression: control 2 failed loudly in CI and named its own cause. That +is #192's lesson one level down — the bite needs a control, and so does +the reproduction. + +--- + +## 3. Bets (falsifiable) + +1. **The reorder resolves the observed hang.** Falsified if + `m4_5_basedpyright_initializes_and_negotiates_encoding` still fails to + terminate after the change. +2. **The reorder is safe for PTY mode.** Falsified by any regression in + `vterm_stage1/2/3_acceptance`, `terminal_config_acceptance`, + `terminal_copy_mode_acceptance`, `m6_4/m6_5_repl_acceptance`, + `m6_7_scrollback_acceptance`, `m6_8_multi_repl_acceptance`, + `worker_shutdown_acceptance`, or **`bottom_panel_stage1_acceptance`** + — added in rev 3: it holds PTY-in-panel tests (`acc28` drives real + child input and the `C-c` escape) and its absence from rev 1's list + was a real gap, not a judgement call. +3. **The synthetic test bites.** Falsified if the new test passes with + `let _ = self.stdin.take();` removed. This must be checked by actual + revert, per the standing rule that a new pin needs its own bite. +4. **The basedpyright test passes rather than merely terminating.** The + hang is at teardown (`m4_acceptance.rs:1374`), *after* the body's + assertions, so it should now pass outright. Falsified if it terminates + with a failure — which would mean a second, independent defect. + **If falsified, stop the lane and frame that defect separately.** Do + not paper over it: "terminates" was never the goal, and a failing + assertion here is new information, not a loose end. + +--- + +## 4. Acceptance + +1. `RuntimeHandles::drop` takes `stdin` before joining readers, with a + comment naming the drop-body-before-fields trap. +2. New unit test in `src/process.rs` (so it runs under the standard + `cargo test --lib` gate, not only an acceptance suite): + `teardown_closes_stdin_before_joining_readers`. + - Spawns `setsid --fork cat` as a **non-group pipe** process. The + choice of `setsid` over a shell background job is load-bearing + (Q#TD6) and gets a comment saying so. `setsid` presence is + **asserted, not skipped.** + - **Two positive controls, before teardown starts:** (1) the recorded + child has actually exited — while it lives it holds the output pipe + itself, so control 2 would pass for the wrong reason; (2) both + readers are still blocked in `read`, which is only true while + something still holds the write ends. Without these the test + silently degrades into modelling the wrong thing and reports green + while doing it — which is exactly what happened on `dash`, and + control 2 is what caught it. + - `#[cfg(target_os = "linux")]`: the controls read `/proc`, and + `setsid(1)` is util-linux (absent on macOS). Gate it explicitly and + say why, rather than letting it be incidentally Linux-only. (Same + reasoning as the APFS gate — `cfg(unix)` would be wrong here.) + - Performs the full reap-and-drop sequence on a helper thread and + asserts completion via `recv_timeout`, so a regression **fails** + within a bounded window instead of hanging. A test that hangs on + regression would reproduce the exact hazard this PR removes. + - Bound: 10s (default `grace_period` is 2s, `:927`). + - On the failure path the helper thread stays wedged and the `cat` + survives until the harness's fds close at process exit. That is + bounded and acceptable — but the test comment must **say so**, or a + future reviewer correctly flags a leaked thread as a defect. +3. The bite is demonstrated by revert, and the result recorded in the PR + body — pass/fail both ways, per Bet 3. +4. `cargo test --test m4_acceptance` runs **without** + `-- --skip basedpyright` and completes, locally, on the machine where + it currently hangs 5/5. +5. Docs, in **both** places the superseded cause lives — replacing it, + not appending to it: + - `docs/agent-handoff.md` §5 gains the drop-body-before-fields lesson + and the corrected cause, replacing "no timeout on the initialize + handshake". + - `docs/agent-handoff.md` §3's machine caveat currently says the + desktop's **local binary is broken and hangs**. §1.7 shows the + binary was never broken: the shim architecture plus this defect + was. Left alone, §3 keeps steering readers toward a false model — + and toward keeping the skip forever. + +**Deliberately not a criterion:** removing `-- --skip basedpyright` from +`CLAUDE.md`'s standing gate list. It is a separate call that is the +user's to make, and it changes only *local* behaviour — CI skips the test +regardless (§1.8). I will propose it with evidence after the fix has been +green repeatedly, rather than fold a process change into a defect fix. + +When that proposal comes it owes two things beyond the green runs: the +`docs/agent-handoff.md` §3 caveat updated (criterion 5 covers it here, +but the *skip* rationale lives with it), and an explicit note that +`PMACS_REQUIRE_PYRIGHT` stays **unarmed** in CI until the per-test +timeout lane (3a) merges — the ordering #194 established, where presence +of the variable decides execution and arming without a timeout would give +CI the same unbounded hang this PR removes locally. + +--- + +## 5. Parked (each needs its own evidence) + +- **P1 — cancellable non-group `read`.** Move `spawn_reader` onto + `spawn_group_reader`'s `O_NONBLOCK` + `poll` mechanism so `cancel` is + observed within `READER_SEND_POLL_INTERVAL` (`:421`, 50ms) even + mid-`read`. Bounds teardown unconditionally, including for children + that ignore EOF (Q#TD3) — **and** the blocked-writer case, where EOF is + never delivered because `write_all` is stuck on a full pipe. Already + named as a deferral by `spawn_reader`'s own doc comment. Tests: the + `sleep 300` shape from Q#TD6 (child never reads stdin), plus a + fill-the-pipe-then-stop-reading shape for the writer case. +- **P2 — stop orphaning wrapper-launched servers (Q#TD5).** Restated in + rev 4, because the corrected §1.7 changes the target: the orphan is not + self-inflicted by the server, it is created by **us** SIGTERMing a + wrapper that does not forward the signal. Spawn stdio servers in their + own process group and signal the group, reusing the machinery the group + path and `reap_ledger` already have. Fixes a real leak: every + basedpyright-backed session currently leaves a `node` process behind. + Note the ordering consequence — a group-directed SIGTERM would reach + node directly, so this also removes the condition the present fix works + around, rather than merely tolerating it. +- **P3 — join the stdin writer thread** so the final flush is ordered + against child termination (Q#TD4). +- **P4 — re-audit the "intermittent" label** in `docs/agent-handoff.md` + and the audit now that §1.7 explains it. Rides this PR's doc update + only insofar as criterion 5 requires; a broader sweep is separate. + +--- + +## 6. Gates + +Per `CLAUDE.md`, each as its own step with a real exit status checked +(never `cmd | tail` — a pipe returns the tail's status and has masked a +real failure here before): + +- `cargo fmt --check` +- `cargo clippy --workspace --all-targets -- -D warnings` +- `cargo test --lib` +- `cargo test --lib --features crdt` +- `cargo test --test m4_acceptance` — **without** the basedpyright skip +- The PTY/REPL suites named in Bet 2 +- `cargo test --test worker_shutdown_acceptance` +- `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` +- `git diff --check` + +Commit before gating, so the results describe the pushed tree. + +--- + +## 7. Branch plan + +One branch, one PR: `process-teardown-stdin-deadlock`, from `main` @ +`e003b81` or later. Worktree `pmacs-hang` (already clean at that SHA). + +Small diff — the reorder, one unit test, one comment, the handoff +update. P1–P4 do not ride it. + +`docs/active-work.md` is integrated **late**, immediately before pushing, +to avoid the ledger-contention treadmill with the other open lanes. diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 493d993..3620a32 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -391,6 +391,70 @@ struct PendingJob { /// When the job was registered. Used to compute "age" in the /// `*workers*` buffer. dispatched_at: Instant, + /// The filesystem mutation this job performs, retained so the + /// main-thread drain can reconcile the editor's path owners once + /// the syscall lands (dired Stage 2a, §5). + /// + /// The paths have to live here because the dispatchers **move** + /// them into the worker closure and nothing else retains them, and + /// because the reply is undifferentiated — rename and remove both + /// settle as `ReplyKind::FsUnit`, so a drain cannot key on the + /// reply and must key on the pending job. + /// + /// One enum field rather than a pair of `Option`s: two would admit + /// a both-`Some` state that cannot occur, which every consumer + /// would then have to rule out by hand. `COHERENCE.md` §9 is why + /// this is a field on the job and not a side map — the parse + /// job→buffer link already lives in a side map and §9 names that as + /// the defect. + resource: Option, +} + +/// A settled filesystem mutation, with the paths the worker consumed +/// (dired Stage 2a, §5). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ResourceOp { + /// A successful `rename(from, to)`. + Rename { + /// Source path, as the caller spelled it. + from: PathBuf, + /// Destination path, as the caller spelled it. + to: PathBuf, + }, + /// A successful `remove(path)`. + Remove { + /// The path that was removed. + path: PathBuf, + }, +} + +/// What one [`AsyncRuntime::tick`] observed. +/// +/// Settle identity and resource metadata come out of **one** +/// transaction — the post-drain loop already borrows `pending` to +/// record completions — so a consumer cannot see a settle without its +/// resource, or the reverse. +#[derive(Clone, Debug, Default)] +pub struct TickOutcome { + /// Ids that transitioned from `Running` to a terminal state during + /// this tick. The Lua runtime resumes coroutines parked on these. + pub settled: Vec, + /// Successful resource mutations, **in bus-arrival order. This is + /// not filesystem execution order.** + /// + /// [`AsyncRuntime::tick`] drains the reply bus with `try_recv` and + /// the runtime establishes no execution token, so a worker can + /// complete, be descheduled before sending, and have a later + /// mutation's reply arrive first. A consumer that reads "in settle + /// order" and infers causality is wrong; reconciliation is + /// deliberately order-independent (Q#DR29), and the primitive's + /// contract is that a caller with overlapping source/target paths + /// serializes by awaiting each op before dispatching the next. + /// + /// Carries **only** jobs that settled + /// [`PendingState::Complete`] — a failed or cancelled mutation + /// reconciles nothing and fires no hook. + pub resources: Vec, } /// Snapshot of a job's terminal state, returned by @@ -684,6 +748,18 @@ impl AsyncRuntime { kind: JobKind, supersede_key: Option<&str>, stream: Option, + ) -> (JobId, CancellationToken) { + self.allocate_with_resource(kind, supersede_key, stream, None) + } + + /// [`Self::allocate`], plus the filesystem mutation this job + /// performs. Only the two mutating fs dispatchers pass `resource`. + fn allocate_with_resource( + &self, + kind: JobKind, + supersede_key: Option<&str>, + stream: Option, + resource: Option, ) -> (JobId, CancellationToken) { let id = self.next_job_id.fetch_add(1, Ordering::Relaxed); let cancel = CancellationToken::new(); @@ -711,6 +787,7 @@ impl AsyncRuntime { max_batch: stream.unwrap_or(0), kind, dispatched_at: Instant::now(), + resource, }, ); (id, cancel) @@ -869,7 +946,17 @@ impl AsyncRuntime { /// Dispatch a `rename(from, to)` job. Settles to /// [`JobResult::Unit`] on success. T M8.1. pub fn dispatch_fs_rename(&self, from: PathBuf, to: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRename, supersede, None); + // The closure below MOVES both paths; the pending entry is the + // only thing that still knows them when the reply lands. + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRename, + supersede, + None, + Some(ResourceOp::Rename { + from: from.clone(), + to: to.clone(), + }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_rename(&cancel, &from, &to); @@ -891,7 +978,12 @@ impl AsyncRuntime { /// Dispatch a `remove(path)` job. T M8.1. pub fn dispatch_fs_remove(&self, path: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRemove, supersede, None); + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRemove, + supersede, + None, + Some(ResourceOp::Remove { path: path.clone() }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_remove(&cancel, &path); @@ -996,11 +1088,16 @@ impl AsyncRuntime { } } - /// Drain every queued reply on the main-thread bus, update - /// pending entries, and return the list of ids that *transitioned - /// from Running to a terminal state* during this tick. The Lua - /// runtime resumes coroutines parked on these ids. - pub fn tick(&self) -> Vec { + /// Drain every queued reply on the main-thread bus, update pending + /// entries, and report what settled. + /// + /// [`TickOutcome::settled`] is the ids that transitioned from + /// `Running` to a terminal state during this tick — the Lua runtime + /// resumes coroutines parked on these. + /// [`TickOutcome::resources`] is the filesystem mutations among them + /// that **succeeded**, in **bus-arrival order** (see the field's + /// own documentation: that is not execution order). + pub fn tick(&self) -> TickOutcome { let mut newly_settled = Vec::new(); while let Ok(env) = self.main.try_recv() { let Ok(reply): Result = self.main.decode(&env) else { @@ -1071,6 +1168,7 @@ impl AsyncRuntime { // a successor that came in mid-flight will have overwritten // the entry already, and that successor's pending lifetime // is what owns the slot now. + let mut resources = Vec::new(); if !newly_settled.is_empty() { let pending = self.pending.borrow(); let mut sup = self.supersede.borrow_mut(); @@ -1078,6 +1176,16 @@ impl AsyncRuntime { let now = Instant::now(); for id in &newly_settled { if let Some(job) = pending.get(id) { + // The harvest (§5): one more read in a loop that + // already borrows `pending` and reads `job.kind`, + // so settle identity and resource metadata come out + // of one transaction. Gated on `Complete` — a + // failed or cancelled mutation reconciles nothing. + if let Some(resource) = &job.resource + && matches!(job.state, PendingState::Complete(_)) + { + resources.push(resource.clone()); + } if let Some(key) = &job.supersede_key && sup.get(key) == Some(id) { @@ -1106,7 +1214,10 @@ impl AsyncRuntime { completed.pop_back(); } } - newly_settled + TickOutcome { + settled: newly_settled, + resources, + } } /// Snapshot the runtime's job tables for the `*workers*` @@ -1749,6 +1860,126 @@ mod tests { } } + /// dired Stage 2a, acceptance 54 (controlled-bus layer). Allocate + /// two resource jobs **without dispatching workers**, inject their + /// successful replies in a chosen order, and assert + /// `TickOutcome.resources` reports exactly that order. + /// + /// This is the honest statement of what the runtime guarantees: + /// `tick` drains the reply bus with `try_recv` and establishes no + /// execution token, so what a consumer sees is bus-arrival order. + /// The test fails against sorting by job id or kind, and against any + /// claim that the order recovers dispatch or filesystem-execution + /// order — because the injection order here is *deliberately* the + /// reverse of the allocation order in the first case. + #[test] + fn tick_reports_resources_in_bus_arrival_order_not_allocation_order() { + fn run(reverse: bool) -> Vec { + let rt = AsyncRuntime::with_pool_size(1); + let (a, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }), + ); + let (b, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }), + ); + let order = if reverse { [b, a] } else { [a, b] }; + for id in order { + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: id, + kind: ReplyKind::FsUnit, + }, + ) + .expect("inject reply"); + } + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both jobs settled"); + outcome.resources + } + + let a_first = ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }; + let b_first = ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }; + + assert_eq!( + run(true), + vec![b_first.clone(), a_first.clone()], + "B injected first must be reported first, even though A was \ + allocated first" + ); + assert_eq!( + run(false), + vec![a_first, b_first], + "and the reverse arrival order reverses the report" + ); + } + + /// A failed or cancelled mutation reconciles nothing, so it must not + /// appear in `resources` at all (acceptance 37's runtime half). + #[test] + fn a_failed_or_cancelled_resource_job_is_not_harvested() { + let rt = AsyncRuntime::with_pool_size(1); + let (failed, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/nope"), + to: PathBuf::from("/tmp/also-nope"), + }), + ); + let (cancelled, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/never"), + }), + ); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: failed, + kind: ReplyKind::Error("ENOENT".to_owned()), + }, + ) + .expect("inject"); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: cancelled, + kind: ReplyKind::Cancelled, + }, + ) + .expect("inject"); + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both settled"); + assert!( + outcome.resources.is_empty(), + "only Complete mutations are harvested; got {:?}", + outcome.resources + ); + } + #[test] fn dispatch_sum_completes_with_correct_value() { let rt = AsyncRuntime::with_pool_size(2); diff --git a/src/buffer.rs b/src/buffer.rs index a9c01e9..3ccb5e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -148,6 +148,26 @@ struct EditDescription { inserted_len: u64, } +/// Provenance of a [`Buffer`]'s name (dired Stage 2a, Q#DR30). +/// +/// A rename must move a name that merely *renders* the file's path and +/// must leave a name the user chose alone. String inspection cannot +/// tell those apart — a user may legitimately name a buffer with a +/// string that normalizes to its own path — so the fact is recorded at +/// the moment the name is written instead of being reconstructed +/// later. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum BufferNameOrigin { + /// A caller named this buffer: `Buffer::new`/`from_bytes`, an + /// ordinary [`Buffer::set_name`], or the `pmacs.buffer.set_name` + /// binding. A rename leaves the name alone. + Explicit, + /// The name was derived from the buffer's backing path by a + /// path-backed creation site, through + /// [`Buffer::set_path_derived_name`]. A rename rewrites it. + PathDerived, +} + /// The unit of editable content: rope + identity + views + undo. /// /// # Threading @@ -159,6 +179,14 @@ pub struct Buffer { id: BufferId, rope: Rope, name: String, + /// Where [`Self::name`] came from. Recorded rather than inferred, + /// because a path-backed buffer's name is **not** reliably its + /// path: `get_or_load_buffer` takes the name from the path *as + /// given* and normalizes only the stored `file_path`, so a + /// relative open is named `foo.rs` while its path is absolute. + /// Rename reconciliation asks this bit, never the string + /// (dired Stage 2a, Q#DR30). + name_origin: BufferNameOrigin, /// The buffer's single active major mode, if one has been selected. major_mode: Option, is_modified: bool, @@ -247,6 +275,10 @@ impl Buffer { id, rope, name: name.into(), + // Construction names a buffer explicitly. A path-backed + // creation site re-records provenance through + // `set_path_derived_name` right after binding the path. + name_origin: BufferNameOrigin::Explicit, major_mode: None, is_modified: false, read_only: false, @@ -449,9 +481,35 @@ impl Buffer { &self.name } - /// Set the buffer's name. Used by save-as and rename operations. + /// Set the buffer's name, recording it as **explicitly chosen** + /// ([`BufferNameOrigin::Explicit`]). + /// + /// This is the user-facing door — `pmacs.buffer.set_name` and + /// save-as go through it — and it is deliberately explicit even + /// when the string happens to denote the file: naming a buffer + /// `notes` for `${cwd}/notes` is still a naming operation, and a + /// later rename must not overwrite it. Path-backed creation sites + /// use [`Self::set_path_derived_name`] instead. pub fn set_name(&mut self, name: impl Into) { self.name = name.into(); + self.name_origin = BufferNameOrigin::Explicit; + } + + /// Set the buffer's name **and** record that it was derived from + /// the buffer's backing path ([`BufferNameOrigin::PathDerived`]). + /// + /// Every site that creates or re-binds a path-backed buffer uses + /// this door, including rename reconciliation itself — so a second + /// rename still follows the path. + pub fn set_path_derived_name(&mut self, name: impl Into) { + self.name = name.into(); + self.name_origin = BufferNameOrigin::PathDerived; + } + + /// Where this buffer's name came from (dired Stage 2a, Q#DR30). + #[must_use] + pub fn name_origin(&self) -> BufferNameOrigin { + self.name_origin } /// This buffer's active major mode, if any. diff --git a/src/diag.rs b/src/diag.rs index 88aa6cc..b81a4dc 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -266,6 +266,22 @@ impl DiagnosticStore { *self.epochs.entry(uri.to_owned()).or_insert(0) += 1; } + /// Drop **every** trace of `uri`, epoch included (dired Stage 2a, + /// §5 finding 4). + /// + /// Distinct from [`Self::clear`] on purpose: `clear` *creates* an + /// `epochs` entry (`or_insert(0) += 1`) because a consumer caching + /// against the epoch must observe that the diagnostics went away. + /// Forgetting is the opposite intent — the editor no longer holds + /// this URI at all — so leaving the counter behind would be a + /// URI-keyed leak in the one map nothing else prunes. + pub fn forget(&mut self, uri: &str) { + self.by_uri.remove(uri); + self.severity_counts.remove(uri); + self.stale_uris.remove(uri); + self.epochs.remove(uri); + } + /// Monotonic per-URI change counter: how many times `set` / /// `clear` ran for this URI. `0` for a URI never written. /// Consumers cache against this to detect republishes that no @@ -489,6 +505,17 @@ impl DiagnosticView { } impl View for DiagnosticView { + /// Re-root this view when the buffer's file was renamed (dired + /// Stage 2a, §5). The URI field is private and `View` has no + /// downcast, so this hook is the only way an outside sweep can + /// reach it — and mutating in place preserves this overlay's + /// position in the window's composition order. + fn rename_resource(&mut self, old_uri: &str, new_uri: &str) { + if self.uri == old_uri { + new_uri.clone_into(&mut self.uri); + } + } + fn kind(&self) -> &'static str { "diagnostic" } @@ -745,6 +772,49 @@ mod tests { } } + /// dired Stage 2a §5, finding 4. `clear` *creates* an `epochs` + /// entry, because a consumer caching against the epoch has to see + /// that the diagnostics went away; nothing ever removes one. So a + /// `forget_uri` that called `clear` would leave a URI-keyed leak + /// behind in the one map nothing prunes — which is why the forget + /// path is its own store method. + #[test] + fn forget_drops_the_epoch_while_clear_deliberately_bumps_it() { + let mut store = DiagnosticStore::new(); + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); + store.mark_stale("file:///a.rs"); + assert_eq!(store.epoch_for("file:///a.rs"), 1); + + store.clear("file:///a.rs"); + assert_eq!( + store.epoch_for("file:///a.rs"), + 2, + "clear announces the removal to epoch-keyed caches" + ); + + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); + store.mark_stale("file:///a.rs"); + store.forget("file:///a.rs"); + assert!(store.for_uri("file:///a.rs").is_empty(), "diagnostics"); + assert!(!store.is_stale("file:///a.rs"), "stale flag"); + assert_eq!( + store.severity_counts_for("file:///a.rs"), + (0, 0, 0, 0), + "severity counts" + ); + assert_eq!( + store.epoch_for("file:///a.rs"), + 0, + "forget leaves no trace at all, epoch included" + ); + } + #[test] fn from_lsp_value_parses_minimal_diagnostic() { let v = json!({ diff --git a/src/editor_core.rs b/src/editor_core.rs index 661b767..6a8ec90 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -938,11 +938,18 @@ impl EditorCore { } let normalized = normalize_buffer_path(path.to_path_buf()); let (bytes, meta) = crate::file_io::load_file(path)?; + // The name is the path **as given** — a relative open is named + // `foo.rs` while `file_path` below is absolute. Recording the + // provenance (Q#DR30) is what lets rename reconciliation move + // this name without having to guess from the string. let display_name = path.display().to_string(); let id = self .registry .borrow_mut() - .create_from_bytes(display_name, &bytes); + .create_from_bytes(display_name.clone(), &bytes); + if let Ok(b) = self.registry.borrow_mut().get_mut(id) { + b.set_path_derived_name(display_name); + } self.set_buffer_path(id, Some(normalized)); self.set_buffer_meta(id, Some(meta)); Ok((id, true)) @@ -1001,7 +1008,12 @@ impl EditorCore { }), Err(error) if error.kind() == std::io::ErrorKind::NotFound => { let display_path = path.display().to_string(); - let buffer_id = self.registry.borrow_mut().create(display_path); + let buffer_id = self.registry.borrow_mut().create(display_path.clone()); + // Path-backed creation site (Q#DR30): the name is the + // path, so a later rename may move it. + if let Ok(b) = self.registry.borrow_mut().get_mut(buffer_id) { + b.set_path_derived_name(display_path); + } self.set_buffer_path(buffer_id, Some(path.to_path_buf())); "[new file]".clone_into(&mut self.status); Ok(ResolvedTarget::Buffer { @@ -1833,22 +1845,66 @@ 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. + /// + /// 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(); 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); } + 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; + } } } } + /// Clamp `win`'s cursor and selection anchor to `len`, clearing the + /// selection only when the clamp collapses it. + /// + /// 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 /// its [`TextView`] from scratch. /// @@ -1862,6 +1918,8 @@ impl EditorCore { /// /// Cursor and `view_top` are clamped to the new buffer extent so /// 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 { @@ -1871,9 +1929,7 @@ 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; @@ -4828,6 +4884,205 @@ impl EditorCore { .map_err(|e| e.to_string()) } + /// Rebind every buffer affected by a successful rename of `old` to + /// `new` (dired Stage 2a, Q#DR14). Returns one + /// [`RenameRebind`] per buffer moved. + /// + /// A rename is a **transaction across path owners**, not a field + /// update. This method owns the two owners that live in the buffer: + /// the stored path and — subject to the provenance rule below — the + /// name. Everything else keyed by the path (URI-keyed LSP stores, + /// diagnostic overlays, dired's pathless handles, a package's own + /// URI table) reconciles off the `resource.renamed` hook that the + /// caller fires, because no buffer-keyed rebind can reach them. + /// + /// Both rename paths call this — the drain harvest for + /// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the + /// two cannot drift apart. + /// + /// # The name + /// + /// The name is rewritten only for a buffer whose name is + /// [`crate::buffer::BufferNameOrigin::PathDerived`]. String + /// inspection cannot substitute for that bit in either direction: a + /// relative open is named `foo.rs` (so an equality test leaves it + /// stale), and a user may name a buffer with a string that + /// normalizes to its own path (so a path-equivalence test + /// overwrites a chosen name). When it does fire, the new name is + /// the **normalized** new path — a buffer opened relatively + /// therefore acquires an absolute name, because no buffer records + /// which base its name was relative to. Reconciliation re-records + /// `PathDerived`, so a second rename still follows. + pub fn reconcile_rename(&mut self, old: &Path, new: &Path) -> Vec { + let old_n = normalize_buffer_path(old.to_path_buf()); + let new_n = normalize_buffer_path(new.to_path_buf()); + // A directory rename moves its whole subtree by construction, + // so descendants are always in scope here. + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, &old_n, true) + }; + let mut rebinds = Vec::with_capacity(affected.len()); + for (id, bound) in affected { + // Rebuild the path under the new root. An exact match maps + // to `new` itself; a descendant keeps its relative tail. + let target = if bound == old_n { + new_n.clone() + } else { + match bound.strip_prefix(&old_n) { + Ok(tail) => new_n.join(tail), + // Unreachable: `buffers_bound_under` matched on + // exactly this prefix. Skip rather than guess. + Err(_) => continue, + } + }; + let name_followed = { + let mut reg = self.registry.borrow_mut(); + let Ok(buf) = reg.get_mut(id) else { continue }; + buf.set_file_path(Some(target.clone())); + // The file behind this buffer moved, so metadata + // captured against the old path no longer describes + // it. Clearing is what `set_buffer_path`'s callers do + // via `set_buffer_meta`; leaving it would make + // external-change detection compare against a stat of + // a path that is gone. + buf.set_file_meta(None); + if buf.name_origin() == crate::buffer::BufferNameOrigin::PathDerived { + buf.set_path_derived_name(target.display().to_string()); + true + } else { + false + } + }; + rebinds.push(RenameRebind { + buffer_id: id, + old_path: bound, + new_path: target, + name_followed, + }); + } + rebinds + } + + /// Reconcile the buffers a successful delete of `path` orphaned + /// (dired Stage 2a, Q#DR18). + /// + /// Walks the whole registry by normalized equality **or** + /// component-aware prefix, so descendants of a deleted directory + /// are included and a second buffer on one path is not missed. + /// Descendants are unconditionally in scope here, unlike in + /// `delete_verdict`: a recursive delete destroyed them, and a + /// non-recursive one only succeeds on an *empty* directory, so a + /// buffer still bound underneath it was already an orphan. + /// + /// Policy, per buffer: + /// + /// * **modified** — kept alive and reported. The buffer keeps its + /// contents; only the file is gone. This is the half of the + /// promise that is robust, because it runs at drain time against + /// whatever state exists then. + /// * **mid-edit** — skipped entirely and reported in `refused`, + /// **preflighted** rather than discovered. A refusal from + /// `BufferRegistry::remove` is *not* inert: by the time it + /// returns `ConcurrentEdit`, [`Self::kill_buffer`] has already + /// dropped the id from `round_trip_buffers`, closed any side + /// window showing the buffer, and redirected every remaining + /// window onto a fallback with cursor, selection, overlays and + /// scroll position reset. The preflight is *sound*, not merely + /// cheap: phase 1 is entirely `EditorCore`, which holds no Lua + /// handle, so nothing between the check and the removal can + /// re-enter Lua and begin an edit. + /// * otherwise — killed through the full phase 1 above. + /// + /// Neither refusal aborts the rest: a directory delete reaching + /// twelve descendants must not stop at the one that is mid-edit. + /// + /// # Phase 2 is the caller's + /// + /// Buffer removal is two phases and the only place they are + /// composed today is a Lua binding (`pmacs.buffer.kill`). Phase 2 — + /// buffer-scoped keymaps, buffer-local config, folds, and the + /// registered `on_removed` callbacks — lives in `lua_bindings` and + /// needs `&Lua`, so this returns [`DeleteReconcile::killed`] and + /// its caller runs phase 2 over those ids. `EditorCore` does not + /// gain a Lua handle. + pub fn reconcile_delete(&mut self, path: &Path) -> DeleteReconcile { + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, path, true) + }; + let mut out = DeleteReconcile::default(); + for (id, _bound) in affected { + let preflight = { + let reg = self.registry.borrow(); + let Ok(buf) = reg.get(id) else { continue }; + let name = buf.name().to_owned(); + if buf.is_modified() { + Some(Err((true, name))) + } else if buf.editing_in_progress() { + Some(Err((false, name))) + } else { + Some(Ok(())) + } + }; + match preflight { + Some(Ok(())) => {} + Some(Err((true, name))) => { + out.kept_modified.push((id, name)); + continue; + } + Some(Err((false, name))) => { + out.refused.push(( + id, + format!("buffer {name:?} is mid-edit; finish the edit first"), + )); + continue; + } + None => continue, + } + match self.kill_buffer(id) { + Ok(()) => out.killed.push(id), + // Named, because the reason alone is not actionable: + // `kill_buffer`'s "cannot kill the last remaining + // buffer" says nothing about *which* buffer is now + // bound to a path whose file is gone, and that buffer's + // name is what the user needs in order to save it + // somewhere else. + Err(message) => { + let name = self + .registry + .borrow() + .get(id) + .map_or_else(|_| format!("{id:?}"), |b| b.name().to_owned()); + out.refused + .push((id, format!("buffer {name:?}: {message}"))); + } + } + } + out + } + + /// Re-root every URI-keyed overlay in **every** window from + /// `old_uri` to `new_uri` (dired Stage 2a, §5). + /// + /// The traversal mirrors overlay disposal's + /// (`lua_bindings`'s `retain` over `overlay_identity`), with the + /// `retain` replaced by [`View::rename_resource`]. That reaches + /// passive windows as well as the active one — which the Lua attach + /// path cannot, since `pmacs.diag._attach_view` takes the active + /// window and errors otherwise — and preserves composition order, + /// because nothing is removed or re-pushed. + /// + /// A window that never received the overlay still has none; + /// renaming cannot re-root an overlay that was never attached. + pub fn rename_resource_in_views(&mut self, old_uri: &str, new_uri: &str) { + for win in self.windows.values_mut() { + for overlay in &mut win.overlays { + overlay.rename_resource(old_uri, new_uri); + } + } + } + /// Switch one frontend's active window to a different buffer, allocating /// a fresh [`TextView`] for it without changing global active state. pub fn switch_active_buffer_for( @@ -5085,6 +5340,85 @@ fn backward_word(buf: &Buffer, mut pos: Position) -> Position { pos } +/// Every path-bound buffer an operation on `target` affects, paired +/// with its **normalized** stored path (dired Stage 2a; the shared walk +/// query #190 introduced for `delete_verdict`, lifted so rename +/// reconciliation and delete reconciliation cannot drift from it). +/// +/// Three properties, each of which a naive lookup gets wrong: +/// +/// * It scans **every** buffer. +/// [`crate::buffer_registry::BufferRegistry::find_by_path`] is +/// first-match-only, and duplicate path-bound buffers are reachable +/// from public Lua via `pmacs.buffer.from_file` — so a first match +/// can hide a second buffer on the same path, which then survives +/// pointing at a path that no longer exists. +/// * Both sides are normalized. Stored paths are normalized on write +/// (`set_buffer_path`) while an op names its target however the +/// caller spelled it, so a raw comparison misses the match entirely. +/// * Containment is **component-aware** ([`Path::starts_with`]), never +/// a string prefix: `/foo` is not an ancestor of `/foobar`. +/// +/// `include_descendants` is the caller's decision because the two +/// consumers legitimately differ. A delete *guard* scopes descendants +/// to `recursive` (#190: a non-recursive delete destroys nothing +/// beneath the target, so a buffer under it must not refuse the op), +/// whereas a **rename** always moves its whole subtree and a +/// post-delete reconciliation is looking at a directory that is +/// already gone. +pub fn buffers_bound_under( + reg: &crate::buffer_registry::BufferRegistry, + target: &Path, + include_descendants: bool, +) -> Vec<(BufferId, PathBuf)> { + let target = normalize_buffer_path(target.to_path_buf()); + let mut out = Vec::new(); + for id in reg.ids() { + let Ok(buf) = reg.get(*id) else { continue }; + let Some(bound) = buf.file_path() else { + continue; + }; + let bound = normalize_buffer_path(bound.to_path_buf()); + if bound == target || (include_descendants && bound.starts_with(&target)) { + out.push((*id, bound)); + } + } + out +} + +/// One buffer moved by [`EditorCore::reconcile_rename`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct RenameRebind { + /// The buffer that moved. + pub buffer_id: BufferId, + /// Its normalized path before the rename. + pub old_path: PathBuf, + /// Its normalized path after the rename. + pub new_path: PathBuf, + /// Whether the buffer's **name** followed the path, per + /// [`crate::buffer::BufferNameOrigin`]. Reported rather than + /// inferred so a consumer does not have to re-derive the + /// provenance rule. + pub name_followed: bool, +} + +/// Outcome of [`EditorCore::reconcile_delete`]. +/// +/// Three lists rather than two, because "kept on purpose" and "could +/// not be removed" are different events: collapsing them makes a +/// failure look like a policy decision. +#[derive(Clone, Debug, Default)] +pub struct DeleteReconcile { + /// Buffers whose phase 1 (core-side removal) completed. The + /// caller **must** run phase 2 (`after_buffer_removed`) over + /// these — `EditorCore` holds no Lua handle. + pub killed: Vec, + /// Modified buffers kept alive deliberately, with their names. + pub kept_modified: Vec<(BufferId, String)>, + /// Buffers that could not be removed, with the reason. + pub refused: Vec<(BufferId, String)>, +} + /// Normalize a buffer path to an absolute, lexically-clean form: /// /// 1. expand a leading `~` / `~/…` against `$HOME`, diff --git a/src/lsp.rs b/src/lsp.rs index 1eb8024..f5630b6 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -821,6 +821,37 @@ pub struct LspManager { /// per-server [`crate::lsp_status::LspStatus`] for the modeline / /// `*lsp*` buffer. status_tracker: crate::lsp_status::LspStatusTracker, + /// dired Stage 2a §5 — exact `(server, uri)` pairs this editor + /// **explicitly forgot**, so a later *uncorrelated* write cannot + /// resurrect them. + /// + /// [`Self::forget_uri`] purges `pending_routes` and drains their + /// awaiters, which covers every write that is matched to a request + /// id. It cannot cover the writers that never go near a route, and + /// there are two that create state: + /// `textDocument/publishDiagnostics`, absorbed unconditionally — + /// and note that `diag_store` has **zero** correlated writers, so + /// the one store the purge most needs to protect is the one it + /// cannot help at all — and [`Self::mark_document_stale`], which + /// creates URI keys in three stores. + /// + /// Deliberately not the cheaper membership gate ("absorb only if + /// `(sid, uri)` is in `documents`"): servers legitimately publish + /// diagnostics for files the editor never opened — a crate-wide + /// push naming a dependency — and a membership gate drops every + /// one. A tombstone drops only what we forgot. `handle_response` + /// already uses this shape for late arrivals + /// (`client.cancelled_rids`); this is the same pattern with a + /// `(server, URI)` key instead of a request id. + /// + /// **Reclaimed and generation-scoped, not size-bounded.** + /// `did_open(sid, uri)` clears that exact pair; + /// [`Self::start_generation`] and [`Self::forget`] remove every pair + /// for their server and retain every other server's. A capacity or + /// LRU eviction would let an arbitrarily late notification + /// resurrect an evicted key, which is the whole failure this gate + /// exists to stop. + forgotten_documents: std::collections::HashSet<(LspServerId, String)>, /// T M4.9: `(project_root, language_id)` → server id. Drives the /// "LSP runs per-project, not per-buffer" invariant. Roots are /// stored as [`PathBuf`] so callers don't have to canonicalise @@ -901,9 +932,21 @@ enum ResponseRoute { } impl ResponseRoute { - /// The document URI this route targets — the key for the position - /// codec's document/encoding lookup. - fn uri(&self) -> &str { + /// The document URI this route is **scoped to**, if any (dired + /// Stage 2a, §5). + /// + /// Fourteen of the fifteen variants carry a `uri`. The fifteenth, + /// `WorkspaceSymbol`, carries a **query** and no URI at all — its + /// own comment explains that the query stands in for the doc URI in + /// the supersede key — so it answers `None`, and + /// [`LspManager::forget_uri`]'s purge retains it: a + /// workspace-symbol query is not scoped to any document and a + /// rename does not invalidate it. + /// + /// Exhaustive on purpose. A new URI-bearing variant must not + /// silently default to "not scoped", which would leave an in-flight + /// response able to repopulate a forgotten key. + fn scoped_uri(&self) -> Option<&str> { match self { ResponseRoute::Completion { uri } | ResponseRoute::Hover { uri } @@ -918,14 +961,25 @@ impl ResponseRoute { | ResponseRoute::SemanticTokensDelta { uri } | ResponseRoute::Locations { uri, .. } | ResponseRoute::DocumentSymbol { uri } - | ResponseRoute::DocumentHighlight { uri } => uri, - // workspace/symbol results span arbitrary files we have - // not cached — no doc to convert against, so the inbound - // codec must pass coordinates through untouched (same - // non-destructive rule as cross-file definition). - ResponseRoute::WorkspaceSymbol { .. } => "", + | ResponseRoute::DocumentHighlight { uri } => Some(uri), + ResponseRoute::WorkspaceSymbol { .. } => None, } } + + /// The document URI this route targets — the key for the position + /// codec's document/encoding lookup. + /// + /// Delegates to [`Self::scoped_uri`] so the variant list exists + /// once: two near-identical matches over fifteen variants is how + /// one of them ends up missing a variant the other has. + /// `workspace/symbol` results span arbitrary files we have not + /// cached, so there is no doc to convert against and the inbound + /// codec must pass coordinates through untouched (the same + /// non-destructive rule as cross-file definition) — which the empty + /// string already expressed. + fn uri(&self) -> &str { + self.scoped_uri().unwrap_or("") + } } /// One Lua-visible awaiter bound to an in-flight LSP request. Mirrors @@ -1021,6 +1075,7 @@ impl LspManager { semantic_token_store: crate::semantic_tokens::make_shared_store(), pending_routes: HashMap::new(), status_tracker: crate::lsp_status::LspStatusTracker::new(), + forgotten_documents: std::collections::HashSet::new(), project_servers: HashMap::new(), } } @@ -1329,6 +1384,10 @@ impl LspManager { // T M4.5 Option B: drop cached docs; the fresh server gets a // new `did_open` from the editor's reattach path. self.documents.retain(|(s, _), _| *s != id); + // dired Stage 2a §5 — the tombstone is generation-scoped: this + // generation's forgotten pairs go, every other server's stay. + // The reattach path re-`did_open`s whatever it still holds. + self.forgotten_documents.retain(|(s, _)| *s != id); client.state = LspClientState::Starting; let proc_spec = client.spec.to_process_spec(); let pid = self.supervisor.borrow_mut().spawn(proc_spec)?; @@ -1640,15 +1699,35 @@ impl LspManager { ); } for rid in abandoned_rids { - self.pending_routes.remove(&(sid, rid)); - if let Some(client) = self.clients.get_mut(&sid) { - client.pending.remove(&rid); - client.cancelled_rids.insert(rid); - } - self.send_cancel_request(sid, rid); + self.abandon_request(sid, rid); } } + /// Abandon one in-flight request: drop its response route, drop the + /// client's `pending` entry, record the rid so a late reply is + /// dropped silently rather than surfacing as an unmatched response, + /// and ask the server to stop working on it. + /// + /// Extracted from [`Self::drain_cancelled_externals`] by dired Stage + /// 2a so [`Self::forget_uri`] reuses it instead of being a second, + /// incomplete copy. **All four steps are load-bearing together.** + /// Removing only the route and the awaiter — which is what + /// `forget_uri` originally did — leaves `client.pending` holding the + /// rid forever when the server never replies, and leaves + /// `cancelled_rids` without it, so a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename that is worse than a leak: the old server keeps the entry + /// and no attachment drains it afterwards, so the entries + /// accumulate. + fn abandon_request(&mut self, sid: LspServerId, rid: u64) { + self.pending_routes.remove(&(sid, rid)); + if let Some(client) = self.clients.get_mut(&sid) { + client.pending.remove(&rid); + client.cancelled_rids.insert(rid); + } + self.send_cancel_request(sid, rid); + } + /// Send `$/cancelRequest { id }` to `sid`, best-effort. A server /// that is not accepting writes (stopped / crashed) is skipped by /// [`Self::send_notification`]'s state guard; the `Err` is @@ -2901,6 +2980,18 @@ impl LspManager { let Some(uri) = params.get("uri").and_then(Value::as_str).map(str::to_owned) else { return; }; + // dired Stage 2a §5 — the uncorrelated-write gate. This + // notification carries no request id, so `forget_uri`'s route + // purge cannot see it, and `diag_store` has no correlated + // writers at all: without this check a late publish for a + // renamed-away URI silently reinstates the state we just + // forgot. The gate is the exact `(server, uri)` pair, which is + // available here even though `DiagnosticStore.by_uri` is keyed + // by URI alone — so provenance is retained for selective + // teardown without changing the store's key. + if self.forgotten_documents.contains(&(sid, uri.clone())) { + return; + } // T M4.5 Option B: byte-normalise diagnostic ranges before the // store parses them, so the gutter renders correct spans on // non-ASCII lines. @@ -3031,6 +3122,9 @@ impl LspManager { // between exit and forget. Idempotent. self.drain_external_cancelled(sid); self.documents.retain(|(s, _), _| *s != sid); + // dired Stage 2a §5 — terminal removal drops every tombstone + // this server owned; other servers' pairs are retained. + self.forgotten_documents.retain(|(s, _)| *s != sid); self.status_tracker.forget(sid); // T M4.9: drop the project scoping so the next // ensure_server_for_project call spawns a fresh server. @@ -3038,6 +3132,233 @@ impl LspManager { Ok(()) } + /// Drop **every** trace of `uri` under `sid` (dired Stage 2a, §5). + /// + /// One manager-level method rather than fourteen call sites at the + /// Lua layer, because fourteen call sites is how one gets + /// forgotten. Four ordered steps: + /// + /// 1. **Tombstone `(sid, uri)` first**, before clearing anything. + /// Main-thread execution already makes the rest atomic with + /// respect to another manager tick, but putting the gate first + /// means every later call observes the forgotten state even if a + /// future refactor introduces an early return. + /// 2. **Abandon every in-flight request scoped to this URI**, through + /// [`Self::abandon_request`] — the same path the per-tick + /// cancellation sweep uses, so the route, the client's `pending` + /// entry, the `cancelled_rids` record and `$/cancelRequest` all + /// happen together rather than only the first of the four. + /// `WorkspaceSymbol` is retained unconditionally: it carries no + /// URI at all — its query stands in for the doc URI in the + /// supersede key — and a workspace-symbol query is not scoped to + /// any document, so a rename does not invalidate it. Clearing + /// the stores *without* this purge is a race that reintroduces + /// exactly the state it removed: a response already in flight + /// routes on arrival and repopulates the old key after the clear. + /// 3. **Drain-cancel their awaiters.** `pending_external` holds the + /// `Handle:await()` side, and its contract is explicit that it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped — both range over `sid` — so this + /// joins route to awaiter on the `rid`, which is the only index + /// between them. The model is + /// [`Self::drain_external_cancelled`], which is *unconditional*; + /// modelling on `drain_cancelled_externals` instead would drain + /// nothing, because it removes only awaiters whose cancellation + /// token was flipped or which outlived the request timeout, and + /// **a rename flips no token** — leaving any coroutine awaiting + /// against the old URI parked forever. + /// 4. **Clear all fourteen stores plus `documents`.** Two keys are + /// irregular: `locations_store` is *kind*-keyed, so all four + /// kinds must go, and `symbol_store` is *scope*-keyed and holds + /// workspace symbols too, so only the document-scoped entry is + /// dropped — the same asymmetry that makes `WorkspaceSymbol` + /// route-exempt above. Diagnostics go through + /// [`crate::diag::DiagnosticStore::forget`], not `clear`: `clear` + /// *increments* the epoch it is meant to forget. + /// + /// Takes the **old** URI, so calling it after `did_open` of the new + /// one is safe and order-independent. + /// + /// Note there is **no precedent to copy for the store half**: + /// neither server-scoped teardown clears the fourteen result stores. + /// `start_generation` clears deferred notifications, routes, + /// documents and externals; `forget` clears routes, documents, + /// externals, the status tracker and project scoping. Whether stale + /// results should survive a restart is a separate pre-existing + /// question, and this method deliberately does not answer it. + /// + /// # Errors + /// + /// Unknown `sid`, matching [`Self::forget`]'s behaviour for the same + /// input. A URI with **no** state under a known server is an + /// idempotent **success**: the caller runs per attachment, an + /// attachment need not have any pending route or populated result + /// store, and cleanup can be repeated after an earlier partial + /// teardown. + #[allow( + clippy::too_many_lines, + reason = "the fourteen store families are a flat inventory; splitting it is how one of them gets forgotten, which is the defect this method exists to prevent" + )] + pub fn forget_uri(&mut self, sid: LspServerId, uri: &str) -> Result<(), String> { + if !self.clients.contains_key(&sid) { + return Err(format!("unknown server: {sid}")); + } + // Step 1 — the gate, first. + self.forgotten_documents.insert((sid, uri.to_owned())); + + // Steps 2 and 3 — collect the rids this URI owns, settle their + // awaiters cancelled, then abandon each request through the + // SAME path the per-tick sweep uses + // ([`Self::abandon_request`]): route, `client.pending`, + // `cancelled_rids`, `$/cancelRequest`. Purging the route alone + // would leave the request live in the client and a late reply + // unrecognised. + let doomed_rids: Vec = self + .pending_routes + .iter() + .filter(|((s, _), route)| *s == sid && route.scoped_uri() == Some(uri)) + .map(|((_, rid), _)| *rid) + .collect(); + for rid in &doomed_rids { + if let Some(p) = self.pending_external.remove(&(sid, *rid)) { + for a in &p.awaiters { + self.runtime.complete_external_cancelled(a.job_id); + } + } + self.abandon_request(sid, *rid); + } + + // Step 4 — the fourteen stores plus `documents`. + let server_key = sid.raw().to_string(); + self.diag_store + .lock() + .expect("diag store mutex poisoned") + .forget(uri); + self.completion_store + .lock() + .expect("completion store mutex poisoned") + .clear(&crate::completion::CompletionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.hover_store + .lock() + .expect("hover store mutex poisoned") + .clear(&crate::hover::HoverKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.signature_store + .lock() + .expect("signature store mutex poisoned") + .clear(&crate::signature::SignatureKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.definition_store + .lock() + .expect("definition store mutex poisoned") + .clear(&crate::definition::DefinitionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + { + let mut guard = self + .locations_store + .lock() + .expect("locations store mutex poisoned"); + // Kind-keyed: all four have to go. + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + guard.clear(&crate::locations::LocationsKey { + server: server_key.clone(), + uri: uri.to_owned(), + kind, + }); + } + } + self.symbol_store + .lock() + .expect("symbol store mutex poisoned") + // Scope-keyed, and the store also holds workspace symbols: + // only the document-scoped entry is dropped. + .clear(&crate::symbol::SymbolKey { + server: server_key.clone(), + scope: crate::symbol::SymbolScope::Document(uri.to_owned()), + }); + self.document_highlight_store + .lock() + .expect("document highlight store mutex poisoned") + .clear(&crate::document_highlight::DocumentHighlightKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.formatting_store + .lock() + .expect("formatting store mutex poisoned") + .clear(&crate::formatting::FormattingKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.rename_store + .lock() + .expect("rename store mutex poisoned") + .clear(&crate::rename::RenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.prepare_rename_store + .lock() + .expect("prepare rename store mutex poisoned") + .clear(&crate::prepare_rename::PrepareRenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.code_action_store + .lock() + .expect("code action store mutex poisoned") + .clear(&crate::code_action::CodeActionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.inlay_hint_store + .lock() + .expect("inlay hint store mutex poisoned") + .clear(&crate::inlay_hint::InlayHintKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.semantic_token_store + .lock() + .expect("semantic token store mutex poisoned") + .clear(&crate::semantic_tokens::SemanticTokenKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.documents.remove(&(sid, uri.to_owned())); + Ok(()) + } + + /// Whether `(sid, uri)` is currently tombstoned (dired Stage 2a). + /// Read surface for tests; production code consults the set + /// directly at its two gates. + #[must_use] + pub fn is_forgotten(&self, sid: LspServerId, uri: &str) -> bool { + self.forgotten_documents.contains(&(sid, uri.to_owned())) + } + + /// How many `(server, uri)` pairs are tombstoned. Read surface for + /// the reclamation tests — the set must not grow without bound + /// across teardowns. + #[must_use] + pub fn forgotten_document_count(&self) -> usize { + self.forgotten_documents.len() + } + /// Convenience: send `textDocument/didOpen` to `sid`. pub fn did_open( &mut self, @@ -3053,6 +3374,12 @@ impl LspManager { .ok_or_else(|| format!("unknown server: {sid}"))?; let uri = uri.into(); let text = text.into(); + // dired Stage 2a §5 — reclaim the tombstone for THIS exact pair + // and no other. Reopening the document is the editor saying it + // holds the URI again, so a later publish or stale-mark for it + // must be admitted; another server's tombstone for the same URI + // is untouched. + self.forgotten_documents.remove(&(sid, uri.clone())); // T M4.5 Option B: mirror the document so the position codec // can convert per-line between the server's `character` units // and pmacs byte offsets. @@ -3081,7 +3408,7 @@ impl LspManager { let uri = uri.into(); let text = text.into(); self.documents.insert((sid, uri.clone()), text.clone()); - self.mark_document_stale(&uri); + self.mark_document_stale(sid, &uri); let params = json!({ "textDocument": { "uri": uri, @@ -3105,7 +3432,20 @@ impl LspManager { /// mark staleness at *edit* time even while the (full-document, /// O(file)) didChange notification itself is debounced — per-edit /// staleness is what keeps stale-position artifacts off screen. - pub fn mark_document_stale(&self, uri: &str) { + /// + /// **Takes `sid` since dired Stage 2a.** It previously took no + /// server id while *creating* URI keys in three stores for every + /// server at once, which made it the second uncorrelated writer able + /// to resurrect a forgotten URI — and made an exact tombstone + /// impossible. Every caller already owns the attachment's server id, + /// so the parameter costs nothing. + pub fn mark_document_stale(&self, sid: LspServerId, uri: &str) { + // The second uncorrelated-write gate (§5 finding 2). Returns + // before touching any of the three stores, so a forgotten URI + // cannot regain a stale flag either. + if self.forgotten_documents.contains(&(sid, uri.to_owned())) { + return; + } self.diag_store .lock() .expect("diag store mutex poisoned") @@ -3764,3 +4104,721 @@ mod tests { assert_eq!(resolve_config_section(&s, Some("")), s); } } + +// --------------------------------------------------------------------------- +// dired Stage 2a — `forget_uri`, and the tombstone that gates the +// uncorrelated resurrection paths (§5, acceptance 31 / 31b / 31c / 31d). +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod resource_reconciliation_tests { + use super::*; + use crate::async_runtime::JobOutcome; + + /// A manager plus two live-enough clients. `/bin/cat` blocks on + /// stdin, so both stay in `Starting` for the whole test and every + /// notification is deferred rather than written — which is exactly + /// what these tests want: they assert on manager-owned state, not on + /// wire traffic. + fn manager_with_two_servers() -> (LspManager, LspServerId, LspServerId) { + use std::cell::RefCell; + use std::rc::Rc; + let sup = Rc::new(RefCell::new(crate::process::ProcessSupervisor::new())); + let runtime = Rc::new(crate::async_runtime::AsyncRuntime::with_pool_size(1)); + let mut mgr = LspManager::new(sup, runtime); + let mut spec_a = LspServerSpec::new("a", "rust", "/bin/cat"); + spec_a.restart = LspRestartPolicy::Never; + let mut spec_b = LspServerSpec::new("b", "rust", "/bin/cat"); + spec_b.restart = LspRestartPolicy::Never; + let a = mgr.spawn(spec_a).expect("spawn a"); + let b = mgr.spawn(spec_b).expect("spawn b"); + (mgr, a, b) + } + + fn publish(mgr: &mut LspManager, sid: LspServerId, uri: &str, message: &str) { + mgr.handle_notification( + sid, + "textDocument/publishDiagnostics".to_owned(), + json!({ + "uri": uri, + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + }, + "severity": 1, + "message": message, + }], + }), + Instant::now(), + ); + } + + fn diag_messages(mgr: &LspManager, uri: &str) -> Vec { + mgr.diag_store + .lock() + .expect("diag store") + .for_uri(uri) + .iter() + .map(|d| d.message.clone()) + .collect() + } + + /// Populate every one of the fourteen URI-keyed store families plus + /// the `documents` mirror for `(sid, uri)`. + fn populate_all_stores(mgr: &mut LspManager, sid: LspServerId, uri: &str) { + let server = sid.raw().to_string(); + publish(mgr, sid, uri, "a diagnostic"); + mgr.completion_store.lock().unwrap().set( + crate::completion::CompletionKey::new(server.clone(), uri), + crate::completion::CompletionResponse::from_lsp_value(&json!([{ "label": "x" }])), + ); + mgr.hover_store.lock().unwrap().set( + crate::hover::HoverKey::new(server.clone(), uri), + crate::hover::Hover::from_lsp_value(&json!({ "contents": "doc" })) + .expect("a hover payload with contents parses"), + ); + mgr.signature_store.lock().unwrap().set( + crate::signature::SignatureKey::new(server.clone(), uri), + crate::signature::SignatureHelp::from_lsp_value( + &json!({ "signatures": [{ "label": "f()" }] }), + ), + ); + mgr.definition_store.lock().unwrap().set( + crate::definition::DefinitionKey::new(server.clone(), uri), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + mgr.locations_store.lock().unwrap().set( + crate::locations::LocationsKey::new(server.clone(), uri, kind), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + } + mgr.symbol_store.lock().unwrap().set( + crate::symbol::SymbolKey::document(server.clone(), uri), + crate::symbol::SymbolResponse::from_lsp_value( + &json!([{ + "name": "S", "kind": 5, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "selectionRange": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + }]), + uri, + ), + ); + mgr.document_highlight_store.lock().unwrap().set( + crate::document_highlight::DocumentHighlightKey::new(server.clone(), uri), + crate::document_highlight::DocumentHighlightResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 } }, + }])), + ); + mgr.formatting_store.lock().unwrap().set( + crate::formatting::FormattingKey::new(server.clone(), uri), + crate::formatting::FormattingResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "newText": "x", + }])), + ); + mgr.rename_store.lock().unwrap().set( + crate::rename::RenameKey::new(server.clone(), uri), + crate::rename::WorkspaceEditResponse::from_lsp_value(&json!({ "changes": {} })), + ); + mgr.prepare_rename_store.lock().unwrap().set( + crate::prepare_rename::PrepareRenameKey::new(server.clone(), uri), + crate::prepare_rename::PrepareRenameResponse::from_lsp_value(&json!({ + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + })), + ); + mgr.code_action_store.lock().unwrap().set( + crate::code_action::CodeActionKey::new(server.clone(), uri), + crate::code_action::CodeActionResponse::from_lsp_value(&json!([{ "title": "fix" }])), + ); + mgr.inlay_hint_store.lock().unwrap().set( + crate::inlay_hint::InlayHintKey::new(server.clone(), uri), + crate::inlay_hint::InlayHintResponse::from_lsp_value(&json!([{ + "position": { "line": 0, "character": 0 }, + "label": ": i32", + }])), + ); + mgr.semantic_token_store.lock().unwrap().set( + crate::semantic_tokens::SemanticTokenKey::new(server, uri), + crate::semantic_tokens::SemanticTokensResponse::from_lsp_value(&json!({ + "data": [0, 0, 1, 0, 0], + })), + ); + mgr.documents + .insert((sid, uri.to_owned()), "text".to_owned()); + } + + /// Which of the fourteen families still hold an entry for + /// `(sid, uri)`, by name. An empty vector is the post-forget + /// expectation; naming the survivors is what makes a failure + /// actionable instead of "assert!(false)". + #[allow( + clippy::too_many_lines, + reason = "one probe per store family, mirroring the inventory under test" + )] + fn populated_families(mgr: &LspManager, sid: LspServerId, uri: &str) -> Vec<&'static str> { + let server = sid.raw().to_string(); + let mut out = Vec::new(); + if !diag_messages(mgr, uri).is_empty() { + out.push("diag"); + } + if mgr + .completion_store + .lock() + .unwrap() + .get(&crate::completion::CompletionKey::new(server.clone(), uri)) + .is_some() + { + out.push("completion"); + } + if mgr + .hover_store + .lock() + .unwrap() + .get(&crate::hover::HoverKey::new(server.clone(), uri)) + .is_some() + { + out.push("hover"); + } + if mgr + .signature_store + .lock() + .unwrap() + .get(&crate::signature::SignatureKey::new(server.clone(), uri)) + .is_some() + { + out.push("signature"); + } + if mgr + .definition_store + .lock() + .unwrap() + .get(&crate::definition::DefinitionKey::new(server.clone(), uri)) + .is_some() + { + out.push("definition"); + } + for (kind, label) in [ + (crate::locations::LocationKind::References, "references"), + (crate::locations::LocationKind::Declaration, "declaration"), + ( + crate::locations::LocationKind::TypeDefinition, + "typeDefinition", + ), + ( + crate::locations::LocationKind::Implementation, + "implementation", + ), + ] { + if mgr + .locations_store + .lock() + .unwrap() + .get(&crate::locations::LocationsKey::new( + server.clone(), + uri, + kind, + )) + .is_some() + { + out.push(label); + } + } + if mgr + .symbol_store + .lock() + .unwrap() + .get(&crate::symbol::SymbolKey::document(server.clone(), uri)) + .is_some() + { + out.push("symbol"); + } + if mgr + .document_highlight_store + .lock() + .unwrap() + .get(&crate::document_highlight::DocumentHighlightKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("documentHighlight"); + } + if mgr + .formatting_store + .lock() + .unwrap() + .get(&crate::formatting::FormattingKey::new(server.clone(), uri)) + .is_some() + { + out.push("formatting"); + } + if mgr + .rename_store + .lock() + .unwrap() + .get(&crate::rename::RenameKey::new(server.clone(), uri)) + .is_some() + { + out.push("rename"); + } + if mgr + .prepare_rename_store + .lock() + .unwrap() + .get(&crate::prepare_rename::PrepareRenameKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("prepareRename"); + } + if mgr + .code_action_store + .lock() + .unwrap() + .get(&crate::code_action::CodeActionKey::new(server.clone(), uri)) + .is_some() + { + out.push("codeAction"); + } + if mgr + .inlay_hint_store + .lock() + .unwrap() + .get(&crate::inlay_hint::InlayHintKey::new(server.clone(), uri)) + .is_some() + { + out.push("inlayHint"); + } + if mgr + .semantic_token_store + .lock() + .unwrap() + .get(&crate::semantic_tokens::SemanticTokenKey::new(server, uri)) + .is_some() + { + out.push("semanticTokens"); + } + out + } + + /// Acceptance 31, store half — every one of the fourteen families + /// plus `documents` loses its entry. The `populated_families` + /// precondition is what makes this bite: an assertion that the + /// stores are empty afterwards passes vacuously if nothing filled + /// them. + #[test] + fn forget_uri_clears_all_fourteen_store_families_and_the_document_mirror() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let uri = "file:///tmp/old.rs"; + populate_all_stores(&mut mgr, a, uri); + let before = populated_families(&mgr, a, uri); + assert_eq!( + before.len(), + 17, + "precondition: every family must hold an entry before the forget \ + (14 families, of which `locations` counts four kinds); got {before:?}" + ); + assert!(mgr.documents.contains_key(&(a, uri.to_owned()))); + + mgr.forget_uri(a, uri).expect("forget a known server"); + + let after = populated_families(&mgr, a, uri); + assert!( + after.is_empty(), + "these families survived the forget: {after:?}" + ); + assert!( + !mgr.documents.contains_key(&(a, uri.to_owned())), + "the `documents` mirror is what didChange diffs against, so a \ + stale entry under the old URI is a correctness problem" + ); + } + + /// Acceptance 31, route half, plus W3's exemption. A response + /// already in flight at rename time must not repopulate the old key + /// after the clear — and a `workspace/symbol` route, which carries a + /// query and no URI at all, must survive. + #[test] + fn forget_uri_purges_routes_for_the_uri_and_retains_workspace_symbol() { + let (mut mgr, a, b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + mgr.pending_routes.insert( + (a, 1), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 2), + ResponseRoute::Locations { + uri: old.to_owned(), + kind: crate::locations::LocationKind::References, + }, + ); + mgr.pending_routes.insert( + (a, 3), + ResponseRoute::WorkspaceSymbol { + query: "Widget".to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 4), + ResponseRoute::Hover { + uri: other.to_owned(), + }, + ); + // Same URI, different server: another server's in-flight work is + // not ours to cancel. + mgr.pending_routes.insert( + (b, 5), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + + mgr.forget_uri(a, old).expect("forget"); + + assert!(!mgr.pending_routes.contains_key(&(a, 1)), "hover for old"); + assert!( + !mgr.pending_routes.contains_key(&(a, 2)), + "locations for old" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 3)), + "workspace/symbol carries no URI and is not scoped to any \ + document, so a rename does not invalidate it" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 4)), + "an unrelated document's route must survive" + ); + assert!( + mgr.pending_routes.contains_key(&(b, 5)), + "another server's route for the same URI must survive" + ); + } + + /// Acceptance 31, drain half. `pending_external` holds the + /// `Handle:await()` side, and its own contract says it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped, and the one with the similar name + /// (`drain_cancelled_externals`) removes only awaiters whose token + /// was flipped or which timed out — **a rename flips no token**, so + /// modelling on it would drain nothing and park the coroutine + /// forever. + #[test] + fn forget_uri_settles_the_awaiters_joined_to_the_purged_routes() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + let runtime = mgr.runtime.clone(); + + let mut register = |rid: u64, uri: &str| { + let (job_id, token) = runtime.register_external(JobKind::LspRequest, None); + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + mgr.pending_external.insert( + (a, rid), + PendingExternal { + method: "textDocument/hover".to_owned(), + awaiters: vec![Awaiter { job_id, token }], + dispatched_at: Instant::now(), + }, + ); + job_id + }; + let doomed = register(1, old); + let survivor = register(2, other); + + // Nothing has settled yet: the drain, not the registration, is + // what must produce the outcome. + let _ = runtime.tick(); + assert!(!runtime.is_complete(doomed)); + assert!(!runtime.is_complete(survivor)); + + mgr.forget_uri(a, old).expect("forget"); + let _ = runtime.tick(); + + assert!( + matches!(runtime.take_result(doomed), Some(JobOutcome::Cancelled)), + "an awaiter parked on a route we just purged must wake cancelled" + ); + assert!( + !mgr.pending_external.contains_key(&(a, 1)), + "and its entry must be gone, not merely settled" + ); + assert!( + runtime.take_result(survivor).is_none(), + "an unrelated document's awaiter keeps waiting" + ); + assert!(mgr.pending_external.contains_key(&(a, 2))); + } + + /// Review round 1 — `forget_uri` must abandon the request in the + /// **client**, not only in the route table. + /// + /// `pending_routes` and `pending_external` are two of four places an + /// in-flight request lives. `LspClient.pending` (written by + /// `send_request`) and `cancelled_rids` are the other two, and + /// dropping only the first two leaves the entry live forever when the + /// server never replies, while a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename the old server keeps those entries and no attachment + /// drains it afterwards, so they accumulate. + /// + /// Bite: fails against a `forget_uri` that purges routes and + /// awaiters without going through `abandon_request`. + #[test] + fn forget_uri_abandons_the_request_in_the_client_too() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + + for (rid, uri) in [(11u64, old), (12u64, other)] { + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + let client = mgr.clients.get_mut(&a).expect("client a"); + client.pending.insert(rid, "textDocument/hover".to_owned()); + } + let client = mgr.clients.get(&a).expect("client a"); + assert!(client.pending.contains_key(&11), "precondition"); + assert!(client.pending.contains_key(&12), "precondition"); + assert!( + client.cancelled_rids.is_empty(), + "precondition: nothing abandoned yet" + ); + + mgr.forget_uri(a, old).expect("forget"); + + let client = mgr.clients.get(&a).expect("client a"); + assert!( + !client.pending.contains_key(&11), + "the purged request must leave `client.pending`, or it leaks \ + for the lifetime of a server that never replies" + ); + assert!( + client.cancelled_rids.contains(&11), + "and must be recorded, or a late reply surfaces as a generic \ + unrouted response instead of being dropped" + ); + assert!( + client.pending.contains_key(&12), + "an unrelated document's request must survive" + ); + assert!( + !client.cancelled_rids.contains(&12), + "and must not be marked abandoned" + ); + } + + /// Acceptance 31c — the error contract, both arms. The second is the + /// one that matters: the subscriber runs per attachment, an + /// attachment need not have any pending route or populated result, + /// and repeated cleanup after a partial teardown must stay safe. + #[test] + fn forget_uri_raises_for_an_unknown_server_and_succeeds_with_no_state() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let unknown = LspServerId::next(); + let err = mgr + .forget_uri(unknown, "file:///tmp/x.rs") + .expect_err("unknown server must raise, matching `forget`"); + assert!(err.contains("unknown server"), "{err}"); + + mgr.forget_uri(a, "file:///tmp/never-touched.rs").expect( + "a URI with no state under a known server is an \ + idempotent success, not an error", + ); + mgr.forget_uri(a, "file:///tmp/never-touched.rs") + .expect("and repeating it stays safe"); + } + + /// Acceptance 31b — the uncorrelated write. This notification + /// carries no request id, so the route purge cannot see it, and + /// `diag_store` has no correlated writers at all. The companion + /// assertion is that the tombstone does **not** over-reach: a + /// publish for a different, never-opened URI is still absorbed, + /// which is exactly what a `documents` membership gate would have + /// broken. + #[test] + fn a_late_publish_for_a_forgotten_uri_is_dropped_and_an_unopened_uri_is_not() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + publish(&mut mgr, a, old, "before"); + assert_eq!(diag_messages(&mgr, old), vec!["before".to_owned()]); + + mgr.forget_uri(a, old).expect("forget"); + assert!(diag_messages(&mgr, old).is_empty(), "cleared by the forget"); + + publish(&mut mgr, a, old, "late arrival"); + assert!( + diag_messages(&mgr, old).is_empty(), + "a publish naming a URI we explicitly forgot must be dropped" + ); + + // Servers legitimately publish for files the editor never + // opened — a crate-wide push naming a dependency. + let never_opened = "file:///tmp/dependency.rs"; + publish(&mut mgr, a, never_opened, "third-party"); + assert_eq!( + diag_messages(&mgr, never_opened), + vec!["third-party".to_owned()], + "the tombstone drops only what we forgot, never everything \ + outside `documents`" + ); + } + + /// Acceptance 31b, second gate. `mark_document_stale` creates URI + /// keys in three stores, so without its own check a forgotten URI + /// regains a stale flag in all three. + #[test] + fn mark_document_stale_cannot_flag_a_forgotten_pair_in_any_of_the_three_stores() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + mgr.forget_uri(a, old).expect("forget"); + + mgr.mark_document_stale(a, old); + + assert!(!mgr.diag_store.lock().unwrap().is_stale(old), "diagnostics"); + assert!( + !mgr.semantic_token_store.lock().unwrap().is_stale(old), + "semantic tokens" + ); + assert!( + !mgr.inlay_hint_store.lock().unwrap().is_stale(old), + "inlay hints" + ); + + // And it still works for a URI we did not forget, so the gate is + // the tombstone and not a blanket disable. + let live = "file:///tmp/live.rs"; + mgr.mark_document_stale(a, live); + assert!(mgr.diag_store.lock().unwrap().is_stale(live)); + } + + /// Acceptance 31d — identity and reclamation are exact. Tombstone + /// one URI under two servers; `did_open(A, uri)` clears only A's + /// pair, so an A write is admitted while a later B write is dropped. + #[test] + fn the_tombstone_is_keyed_by_the_exact_server_uri_pair() { + let (mut mgr, a, b) = manager_with_two_servers(); + let uri = "file:///tmp/shared.rs"; + mgr.forget_uri(a, uri).expect("forget under a"); + mgr.forget_uri(b, uri).expect("forget under b"); + assert!(mgr.is_forgotten(a, uri)); + assert!(mgr.is_forgotten(b, uri)); + + mgr.did_open(a, uri, 1, "text").expect("reopen under a"); + assert!( + !mgr.is_forgotten(a, uri), + "reopening is the editor saying it holds the URI again" + ); + assert!( + mgr.is_forgotten(b, uri), + "and it says nothing about another server's tombstone" + ); + + publish(&mut mgr, a, uri, "from A"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "A's write is admitted after A reopened" + ); + publish(&mut mgr, b, uri, "from B"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "B is still tombstoned for this URI, so its later write is \ + dropped and A's payload survives untouched" + ); + } + + /// Acceptance 31d — a restart generation flip drops every pair for + /// its own server and retains every other server's. + #[test] + fn start_generation_reclaims_only_the_flipped_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(a, "file:///tmp/a2.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 3); + + let mut client = mgr.clients.remove(&b).expect("client b"); + mgr.start_generation(b, &mut client).expect("restart b"); + mgr.clients.insert(b, client); + + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(mgr.is_forgotten(a, "file:///tmp/a2.rs")); + assert!( + !mgr.is_forgotten(b, "file:///tmp/b1.rs"), + "B's generation is gone, so B's tombstones go with it" + ); + assert_eq!(mgr.forgotten_document_count(), 2); + } + + /// Acceptance 31d — terminal `forget` likewise, and the set is empty + /// once the only owning generation is torn down. This is what makes + /// the set reclaimed rather than a leak; it deliberately is **not** + /// size-bounded, because a capacity or LRU eviction would let an + /// arbitrarily late notification resurrect an evicted key. + #[test] + fn terminal_forget_reclaims_only_its_own_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 2); + + if let Some(client) = mgr.clients.get_mut(&b) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(b).expect("forget b"); + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(!mgr.is_forgotten(b, "file:///tmp/b1.rs")); + assert_eq!(mgr.forgotten_document_count(), 1); + + if let Some(client) = mgr.clients.get_mut(&a) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(a).expect("forget a"); + assert_eq!( + mgr.forgotten_document_count(), + 0, + "the set is empty once the owning generations are gone" + ); + } +} diff --git a/src/lua_bindings/diag.rs b/src/lua_bindings/diag.rs index c462f44..b9e632a 100644 --- a/src/lua_bindings/diag.rs +++ b/src/lua_bindings/diag.rs @@ -232,6 +232,32 @@ pub fn install_diag( )?; } + // dired Stage 2a §5 step 6 — re-root every attached + // `DiagnosticView` from `old_uri` to `new_uri` after a rename. + // + // `DiagnosticView.uri` is set once at construction and is private, + // and `View` has no downcast, so nothing outside `diag.rs` can + // reach it; the `View::rename_resource` hook is the seam. The sweep + // walks EVERY window, which is what `_attach_view` above cannot do + // — it takes the active window and errors otherwise — so a passive + // split that already holds the overlay is re-rooted too. It mutates + // in place, so each overlay keeps its position in the window's + // composition order; a remove-and-re-push would move the underline + // to the end of the stack and pass a one-window test anyway. + { + diag_mod.set( + "_rename_resource", + lua.create_function(move |lua, (old_uri, new_uri): (String, String)| { + let Some(core) = lua.app_data_ref::() else { + return Ok(false); + }; + core.borrow_mut() + .rename_resource_in_views(&old_uri, &new_uri); + Ok(true) + })?, + )?; + } + pmacs.set("diag", diag_mod)?; Ok(()) } 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/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index d5a2b4b..a8a130b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1671,16 +1671,11 @@ fn delete_verdict( } }; - let target = crate::editor_core::normalize_buffer_path(path.to_path_buf()); - for id in reg.ids() { - let Ok(buf) = reg.get(*id) else { continue }; - let Some(bound) = buf.file_path() else { - continue; - }; - let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf()); - if bound != target && !(scan_descendants && bound.starts_with(&target)) { - continue; - } + // The shared walk (dired Stage 2a): one enumeration, so this guard + // and the two reconciliation seams cannot disagree about which + // buffers an operation on `path` touches. + for (id, _bound) in crate::editor_core::buffers_bound_under(reg, path, scan_descendants) { + let Ok(buf) = reg.get(id) else { continue }; // "Modified" is `Buffer::is_modified()`. No new notion of // dirtiness, and a *clean* open buffer is deliberately not // guarded — refusing there would fail legitimate deletes for @@ -1714,6 +1709,223 @@ fn delete_verdict( DeleteVerdict::Clear } +/// Reconcile a successful rename and fire `resource.renamed` (dired +/// Stage 2a, §5). +/// +/// Both rename paths land here — the drain harvest for +/// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the two +/// can no longer drift, which is how the raw-lookup trap survived being +/// "fixed" once already. +/// +/// The hook carries the **paths**, normalized absolute, not the rebind +/// list: dired's buffers are pathless, so a path-keyed consumer must be +/// able to reconcile from `(old, new)` alone. And the Rust side is +/// structurally incapable of being complete — any package may key state +/// by URI in its own module table and the LSP manager will never know — +/// so the hook is the mechanism that scales, not a convenience. +/// +/// Returns the rebinds, for a caller that wants to report. +fn reconcile_rename_and_fire( + lua: &Lua, + from: &std::path::Path, + to: &std::path::Path, +) -> Vec { + let (rebinds, old_n, new_n) = { + let Some(core) = lua.app_data_ref::() else { + return Vec::new(); + }; + let mut core = core.borrow_mut(); + let rebinds = core.reconcile_rename(from, to); + ( + rebinds, + crate::editor_core::normalize_buffer_path(from.to_path_buf()), + crate::editor_core::normalize_buffer_path(to.to_path_buf()), + ) + }; + // The borrow is released before re-entering Lua: subscribers call + // back into the core (dired reverts a listing, the LSP subscriber + // re-attaches), and a live borrow would panic. + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String( + match lua.create_string(old_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + args.push_back(mlua::Value::String( + match lua.create_string(new_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + run_hook_if_defined(lua, "resource.renamed", args); + rebinds +} + +/// Reconcile a successful delete and fire `resource.deleted` (dired +/// Stage 2a, §6). +/// +/// Composes the **same two removal phases** `pmacs.buffer.kill` +/// composes. Phase 1 (`EditorCore::reconcile_delete`) closes side +/// windows showing a doomed buffer, redirects every other window to a +/// fallback, and removes the id from the registry; phase 2 — +/// buffer-scoped keymaps, buffer-local config, folds, and the +/// registered `on_removed` callbacks — runs here, because it needs +/// `&Lua` and `EditorCore` has no Lua handle. +/// +/// `apply_resource_op`'s delete arm previously ran +/// `remove_buffer_and_fire`, i.e. phase 2 **without** phase 1, leaving +/// any window displaying that buffer pointing at a removed id. Routing +/// both paths through here is what makes that go away as a property of +/// the seam rather than as a separate patch. +fn reconcile_delete_and_fire( + lua: &Lua, + path: &std::path::Path, +) -> crate::editor_core::DeleteReconcile { + let (outcome, normalized) = { + let Some(core) = lua.app_data_ref::() else { + return crate::editor_core::DeleteReconcile::default(); + }; + let mut core = core.borrow_mut(); + let outcome = core.reconcile_delete(path); + ( + outcome, + crate::editor_core::normalize_buffer_path(path.to_path_buf()), + ) + }; + // Phase 2, over exactly the ids phase 1 removed. + for id in &outcome.killed { + after_buffer_removed(lua, *id); + } + if let Ok(path_arg) = lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String(path_arg)); + run_hook_if_defined(lua, "resource.deleted", args); + } + // Reported AFTER the fan-out, deliberately: a subscriber may set its + // own status, and this message must be the last word because it is + // the data-loss-adjacent one. Unconditional, so a path that cannot + // cross into Lua still gets its refusal reported rather than losing + // both the hook and the report. + report_delete_reconcile(lua, &normalized, &outcome); + outcome +} + +/// Cap on how many buffer names one status line spells out before +/// collapsing the rest into a count. A directory delete can reach +/// dozens; a status line that scrolls off is a message nobody reads. +const DELETE_REPORT_NAMED_LIMIT: usize = 3; + +/// Render the buffers a delete could not reconcile, and put it on the +/// status channel. +/// +/// **Silence here is the defect this exists to close.** Both outcomes +/// leave a buffer alive and still bound to a path whose file is gone, so +/// the next `C-x C-s` recreates the file the user just deleted. That is +/// recoverable only if the user knows it happened: +/// +/// * `kept_modified` — a modified buffer, kept on purpose. On the +/// synchronous path #190 refuses before disk so this cannot arise, but +/// `pmacs.fs.remove` dispatches a worker, and a buffer modified in the +/// interval between the caller's check and the syscall reaches here. +/// * `refused` — could not be removed at all: the last remaining buffer +/// (`kill_buffer` refuses to empty the registry), or a buffer that was +/// mid-edit when the reconciliation ran. +/// +/// The channel is `EditorCore::status`, which is what +/// `pmacs.editor.set_status` writes. **Not `pmacs.error`** — that +/// channel is defined only by a test stub, so all fifteen of its guarded +/// call sites are dead, and a report written there would be exactly the +/// silence being fixed. +/// +/// Lives inside the shared seam rather than at its two call sites, for +/// the same reason the reconciliation does: a caller that has to +/// remember to report is a caller that will forget. The first version of +/// this function's callers both discarded the outcome. +fn report_delete_reconcile( + lua: &Lua, + path: &std::path::Path, + outcome: &crate::editor_core::DeleteReconcile, +) { + if outcome.kept_modified.is_empty() && outcome.refused.is_empty() { + return; + } + let name_of = |p: &std::path::Path| { + p.file_name() + .map_or_else(|| p.display().to_string(), |n| n.to_string_lossy().into()) + }; + let mut parts: Vec = Vec::new(); + if !outcome.kept_modified.is_empty() { + let n = outcome.kept_modified.len(); + let named: Vec<&str> = outcome + .kept_modified + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, name)| name.as_str()) + .collect(); + parts.push(format!( + "{n} buffer{} with unsaved changes kept ({}{}) — saving {} will RECREATE the deleted file", + if n == 1 { "" } else { "s" }, + named.join(", "), + if n > named.len() { + format!(", and {} more", n - named.len()) + } else { + String::new() + }, + if n == 1 { "it" } else { "them" }, + )); + } + if !outcome.refused.is_empty() { + let n = outcome.refused.len(); + let named: Vec = outcome + .refused + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, why)| why.clone()) + .collect(); + parts.push(format!( + "{n} buffer{} could not be closed ({}{})", + if n == 1 { "" } else { "s" }, + named.join("; "), + if n > named.len() { + format!("; and {} more", n - named.len()) + } else { + String::new() + }, + )); + } + let message = format!("deleted {}: {}", name_of(path), parts.join("; ")); + if let Some(core) = lua.app_data_ref::() { + core.borrow_mut().status = message; + } +} + +/// Drive [`crate::async_runtime::TickOutcome::resources`] through +/// reconciliation, one settled mutation at a time (dired Stage 2a, +/// Q#DR29). +/// +/// **Each settled mutation reconciles on its own, and nothing here +/// depends on the relative order of two mutations that were in flight +/// simultaneously** — `resources` is bus-arrival order and the runtime +/// establishes no execution token. That is safe rather than merely +/// honest: independent mutations commute, and the primitive's contract +/// (`builtin/runtime/fs.lua`) requires a caller with overlapping +/// source/target paths to serialize by awaiting each op before +/// dispatching the next. +fn reconcile_settled_resources(lua: &Lua, resources: &[crate::async_runtime::ResourceOp]) { + use crate::async_runtime::ResourceOp; + for op in resources { + match op { + ResourceOp::Rename { from, to } => { + reconcile_rename_and_fire(lua, from, to); + } + ResourceOp::Remove { path } => { + reconcile_delete_and_fire(lua, path); + } + } + } +} + fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> { registry .borrow_mut() @@ -3220,6 +3432,37 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result*` buffer can follow a renamed + // directory is for dired's own `resource.renamed` subscriber to + // rename it. The alternative — kill and recreate under the new + // name — loses window placement, the cursor, the read-only + // intercept, round-trip input and the major mode, each of which + // would have to be re-established in the right order. + // + // Uniqueness stays the CALLER's job, matching the Rust setter; + // dired reuses its existing `<2>`-variant uniquifier. + // + // This records `BufferNameOrigin::Explicit` (Q#DR30): it is a + // naming operation even when the string happens to denote the + // file, so a later rename must not overwrite it. + let reg = registry.clone(); + buffer.set( + "set_name", + lua.create_function(move |_, (id, name): (BufferIdLua, String)| { + reg.borrow_mut() + .get_mut(id.0) + .map_err(mlua::Error::external)? + .set_name(name); + Ok(()) + })?, + )?; + } + { let reg = registry.clone(); buffer.set( @@ -3244,6 +3487,11 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) @@ -3307,6 +3555,10 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) @@ -3428,12 +3680,18 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() - { - core.borrow_mut().set_buffer_path(id, Some(to.clone())); - } + // dired Stage 2a: the raw, first-match, + // un-normalized `find_by_path` lookup this arm + // used is replaced by the shared transaction. + // Three defects went with it — stored paths are + // normalized on write while the op names its + // target raw, so the lookup could miss the + // buffer entirely; a directory rename has many + // affected buffers by construction and only the + // first moved; and the buffer's *name* stayed + // stale, so the statusline and buffer list kept + // the old filename. + reconcile_rename_and_fire(lua, &from, &to); } "delete" => { // Four ordered phases (Q#RD2): stat/no-op @@ -3490,18 +3748,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { return Err(mlua::Error::external(format!( @@ -7358,9 +7617,15 @@ pub fn install_async( async_mod.set( "_tick", lua.create_function(move |lua, ()| { - let ids = rt.tick(); - let t = lua.create_table_with_capacity(ids.len(), 0)?; - for (i, id) in ids.into_iter().enumerate() { + let outcome = rt.tick(); + // Reconcile BEFORE the settled ids reach Lua. The Lua + // runtime resumes parked coroutines from the table this + // returns, so a coroutine that renamed and then + // inspects a buffer would otherwise see pre-rename + // state. Ordering here is by construction, not by luck. + reconcile_settled_resources(lua, &outcome.resources); + let t = lua.create_table_with_capacity(outcome.settled.len(), 0)?; + for (i, id) in outcome.settled.into_iter().enumerate() { t.set(i + 1, id)?; } Ok(t) @@ -9992,11 +10257,18 @@ pub fn install_lsp( // `builtin/runtime/lsp.lua` calls this per edit so stale // suppression stays keystroke-accurate while the O(file) // full-document notification is coalesced. + // + // **Takes the server id since dired Stage 2a.** It previously + // took the URI alone while creating URI keys in three stores + // for every server at once, which made it the second + // uncorrelated writer able to resurrect a URI `forget_uri` had + // just cleared. The sole production caller already holds + // `rec.server`. let m = manager.clone(); lsp_mod.set( "_mark_document_stale", - lua.create_function(move |_, uri: String| { - m.borrow().mark_document_stale(&uri); + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow().mark_document_stale(id.0, &uri); Ok(()) })?, )?; @@ -10520,6 +10792,35 @@ pub fn install_lsp( )?; } + { + // dired Stage 2a §5 — the per-document teardown the + // `resource.renamed` subscriber needs. Modelled on `forget` + // above: a closure over the shared manager that calls through + // and maps the error with `mlua::Error::external`. + // + // Error contract: **raises** for an unknown server id, matching + // `forget`'s behaviour for the same input, and **succeeds + // silently** when the URI has no state under a known server. + // The second arm is the one that matters — the subscriber runs + // per attachment, an attachment need not have any pending route + // or populated result store, and cleanup can be repeated after + // an earlier partial teardown. An over-strict binding would turn + // that ordinary idempotent case into an error inside a hook. + // + // Takes the **old** URI, so calling it after `did_open` of the + // new one is safe and order-independent. + let m = manager.clone(); + lsp_mod.set( + "forget_uri", + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow_mut() + .forget_uri(id.0, &uri) + .map_err(mlua::Error::external)?; + Ok(()) + })?, + )?; + } + { let m = manager.clone(); lsp_mod.set( diff --git a/src/process.rs b/src/process.rs index 9e02629..f0b851c 100644 --- a/src/process.rs +++ b/src/process.rs @@ -636,6 +636,26 @@ impl Drop for RuntimeHandles { // channel because the consumer fell behind. Cancel flag // unwedges that case before we join. T M6.2. self.cancel.store(true, Ordering::Relaxed); + // Close the child's stdin BEFORE joining. `cancel` covers a + // reader stuck in `send`; it does NOT cover one stuck in + // `read`, which is only consulted between reads. What actually + // unblocks that reader is the child exiting and closing its + // output pipe --- and a stdio child exits on stdin EOF. + // + // The premise in the comment above ("dropping the master + // closes the kernel pipe") holds for a PTY master but NOT for + // pipe mode, where `read` unblocks only once *every* write end + // closes. An escaped descendant holding one (a shim-launched + // language server that orphans its real process) keeps the + // reader blocked indefinitely. + // + // The sink lives in the `stdin` FIELD, and a type's `Drop::drop` + // body runs before *all* of its fields regardless of their + // declaration order --- so reordering the struct cannot fix + // this. Joining first deadlocks against the very EOF that would + // have ended the join. `take()` is idempotent, matching + // `close_stdin`. + let _ = self.stdin.take(); for h in std::mem::take(&mut self.readers) { let _ = h.join(); } @@ -3204,6 +3224,177 @@ mod tests { handle.join().expect("test thread should exit cleanly"); } + /// The stdin sink lives in a *field* of [`RuntimeHandles`], so it + /// cannot drop until `Drop::drop`'s body returns --- and a type's + /// drop body runs before *all* of its fields, whatever their + /// declaration order (so reordering the struct cannot fix this). + /// Joining readers inside that body therefore deadlocks against any + /// child that exits on stdin EOF while still holding the output + /// pipe: no EOF, so no exit, so no pipe close, so a blocking + /// `spawn_reader` never returns. + /// + /// This is the root cause of + /// `m4_5_basedpyright_initializes_and_negotiates_encoding` hanging + /// forever. Modelled with an orphaned grandchild, which is exactly + /// what a shim-launched language server is: the basedpyright + /// console script spawns bundled `node` and exits, leaving the real + /// server at `PPid 1` holding the inherited pipes. + /// + /// `setsid --fork` is used rather than a shell background job, and + /// that choice is LOAD-BEARING. POSIX XCU 2.9.3 assigns `/dev/null` + /// to an asynchronous list's stdin when job control is off --- i.e. + /// in every non-interactive `sh` --- so `sh -c 'cat & exit 0'` reads + /// EOF immediately and exits *against the unfixed tree*, giving a + /// test that passes either way and proves nothing. The obvious + /// repair does not work either: the rule applies **before explicit + /// redirections**, so by the time `<&0` runs, fd 0 already *is* + /// `/dev/null` and the redirect faithfully duplicates it onto + /// itself. `bash` happens to skip the default when a stdin redirect + /// is present; `dash` --- Ubuntu's `/bin/sh`, and CI's --- does not, + /// so `<&0` passed locally and failed in CI. + /// + /// `setsid --fork` sidesteps all of it: it forks, the parent exits, + /// and the child inherits stdin/stdout/stderr untouched by any shell. + /// No async list, no `/dev/null` rule, no implementation variance. + /// + /// Linux-gated deliberately rather than incidentally: the controls + /// read `/proc`, and `setsid(1)` is util-linux (absent on macOS). + /// + /// On the failure path this leaks a wedged worker thread, and `cat` + /// survives until the harness's fds close at process exit. Bounded + /// and intentional --- a test that *hung* on regression would + /// reproduce the very hazard it exists to catch. + #[cfg(target_os = "linux")] + #[test] + fn teardown_closes_stdin_before_joining_readers() { + use std::sync::mpsc; + + /// `sh` becomes a zombie when it exits, because this test + /// deliberately never ticks (a tick runs `poll_one`, which is + /// the teardown path under test). `kill(pid, None)` succeeds on + /// a zombie, so liveness has to come from the process state + /// rather than from signal 0. + fn reaped_or_zombie(pid: u32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Err(_) => true, + Ok(s) => s + .rsplit_once(')') + .and_then(|(_, rest)| rest.split_whitespace().next()) + .is_some_and(|state| state == "Z"), + } + } + + // setsid(1) is util-linux, not coreutils, and the standard + // `cargo test --lib` gate must not hard-fail on a tool the + // README does not require --- a minimal or BusyBox container + // would fail without ever testing pmacs. So: skip when absent, + // but FAIL when `PMACS_REQUIRE_SETSID` is set, which CI sets on + // Linux. That is the arming pattern from the silent-skip lane, + // and it is what keeps this from becoming a test that reports + // `ok` having never run. Presence decides, so an empty value + // counts as unset (a `${{ cond && '1' || '' }}` expression sets + // the empty string, not nothing). + let armed = std::env::var_os("PMACS_REQUIRE_SETSID").is_some_and(|v| !v.is_empty()); + if !binary_available("setsid") { + assert!( + !armed, + "PMACS_REQUIRE_SETSID is set but setsid(1) is not on PATH: \ + install util-linux, or unset the variable to allow the skip" + ); + eprintln!( + "setsid(1) not on PATH; skipping \ + teardown_closes_stdin_before_joining_readers" + ); + return; + } + + let (done_tx, done_rx) = mpsc::channel(); + let handle = std::thread::spawn(move || { + let mut sup = ProcessSupervisor::new(); + sup.set_grace_period(Duration::from_millis(300)); + let mut spec = ProcessSpec::new("orphan-holds-pipe", "setsid"); + // `setsid --fork` forks and the parent exits, so the + // *recorded* pid terminates promptly (letting `poll_one` + // reach the teardown path) while `cat` survives holding the + // inherited pipes. `cat` reads stdin and exits on EOF, + // exactly as a stdio language server does. + spec.args = vec!["--fork".into(), "cat".into()]; + // The default, restated because it is the whole point: with + // `StdinMode::Null` there is no sink to drop and no EOF to + // deliver. + spec.stdin = StdinMode::Piped; + let id = sup.spawn(spec).expect("spawn"); + + let sh_pid = sup + .processes + .get(&id) + .and_then(|p| p.runtime.as_ref()) + .map(|rt| rt.pid) + .expect("runtime records the spawned pid"); + + // CONTROL 1: the recorded child must actually exit. Until it + // does, *it* holds the output pipe, and control 2 would pass + // for the wrong reason. (`setsid` without `--fork` may exec + // directly instead of forking, in which case there is no + // grandchild and this is the control that notices.) + let deadline = Instant::now() + Duration::from_secs(5); + while Instant::now() < deadline && !reaped_or_zombie(sh_pid) { + std::thread::sleep(Duration::from_millis(10)); + } + assert!( + reaped_or_zombie(sh_pid), + "control 1 failed: the recorded child (`sh`) should exit \ + promptly, leaving the grandchild orphaned. While `sh` is \ + alive it holds the output pipe itself, so control 2 would \ + pass without the grandchild modelling anything" + ); + + // CONTROL 2: both readers must still be blocked in `read`, + // which is only true while something still holds the output + // pipe's write ends. If the grandchild never inherited the + // real stdin, it has already read EOF and exited, the write + // ends are closed, the readers have finished --- and the + // deadlock is not being modelled at all. This control is + // what caught the shell form failing on dash after it + // passed on bash. + let readers = sup + .processes + .get(&id) + .and_then(|p| p.runtime.as_ref()) + .map(|rt| { + ( + rt.readers.len(), + rt.readers.iter().filter(|h| !h.is_finished()).count(), + ) + }) + .expect("runtime still present before teardown"); + assert_eq!( + readers, + (2, 2), + "control 2 failed: both readers must still be blocked in \ + `read`, i.e. an escaped grandchild still holds the output \ + pipe. Finished readers mean `cat` read EOF and exited \ + already, so it never inherited the real stdin --- check \ + that `setsid --fork` still forks and passes fds 0/1/2 \ + through untouched on this runner" + ); + + // The deadlock, if present, is here: + // shutdown -> tick -> poll_one -> RuntimeHandles::drop -> join. + drop(sup); + let _ = done_tx.send(()); + }); + + done_rx.recv_timeout(Duration::from_secs(10)).expect( + "supervisor drop should complete within 10s --- if hung, \ + `RuntimeHandles::drop` is joining its readers before dropping \ + the `stdin` field, so the child never receives EOF, never \ + exits, and never closes the output pipe the readers are \ + blocked on", + ); + handle.join().expect("test thread should exit cleanly"); + } + // ----------------------------------------------------------------- // Compile-mode group lifecycle (Q#CM3; framing acceptance 34) // ----------------------------------------------------------------- diff --git a/src/view.rs b/src/view.rs index 424fd9e..e3e4b6c 100644 --- a/src/view.rs +++ b/src/view.rs @@ -310,6 +310,20 @@ pub trait View { fn clone_for_split(&self) -> Option> { None } + + /// Retarget this overlay from `old_uri` to `new_uri` after a + /// resource rename (dired Stage 2a, §5). Default: no-op — a view + /// that renders nothing URI-keyed is unaffected. + /// + /// Mutates **in place**, so the overlay keeps its position in the + /// window's composition order. That is the reason this is a trait + /// hook rather than a remove-and-re-push at the call site: overlays + /// are an ordered `Vec` merged in sequence, and re-pushing would + /// move a diagnostic underline to the end of the stack. It is also + /// how *passive* windows are reached at all — the Lua attach path + /// (`pmacs.diag._attach_view`) can only touch the active window, + /// while the sweep that drives this walks every window. + fn rename_resource(&mut self, _old_uri: &str, _new_uri: &str) {} } // --------------------------------------------------------------------------- diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index 7d0f2c5..bff9134 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,277 @@ 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" + ); +} + +/// 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. +/// +/// 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_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); + + type_char(&mut s, 'z'); + 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); + 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.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:?}" + ); +} + +/// **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` +/// 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"); + 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..8c15629 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,142 @@ 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); +} + +/// 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 @@ -136,3 +279,479 @@ 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" + ); +} + +/// Stage 1 criterion 5 [`main`, and also fix-shape] — the rope lock +/// refuses an ordinary edit first, and the named intercept survives +/// behind it. +/// +/// 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_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); + + press(&mut s, KeyCode::Char('z')); + 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); + 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.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:?}" + ); +} + +/// 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" + ); +} + +/// **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`. +/// +/// `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(); + 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). +/// +/// 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"); +} + +/// 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*. +/// +/// 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 +/// `"*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" + ); +} + +/// 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]`. +/// +/// 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 +/// 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/m4_acceptance.rs b/tests/m4_acceptance.rs index 220ae99..64323e0 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8549,17 +8549,26 @@ fn rd8_recursive_delete_refuses_for_a_modified_descendant() { assert!(tree.exists(), "including the directory itself"); } -/// Criterion 9 — a *clean* recursive delete leaves descendant buffers -/// orphaned, not removed. +/// Criterion 9 — a *clean* recursive delete reconciles descendant +/// buffers, through both removal phases. /// -/// This pin deliberately asserts today's imperfect behaviour. Widening -/// reconciliation to the tree would route N buffers through -/// `remove_buffer_and_fire` — phase 2 without phase 1 — promoting the -/// parked dangling-window defect from exact-path to tree-wide. +/// **Rewritten by dired Stage 2a** (`docs/dired-stage2-framing.md` §6, +/// Q#RD27 / acceptance 23). This row previously pinned the opposite — +/// that the descendant buffer stayed orphaned — and gave the reason: +/// widening reconciliation would have routed N buffers through +/// `remove_buffer_and_fire`, which is phase 2 *without* phase 1, so a +/// tree delete would have left up to N windows pointing at removed ids. +/// That constraint is discharged: `EditorCore::reconcile_delete` +/// composes the same two phases `pmacs.buffer.kill` composes, and the +/// delete arm routes through it. The old assertion is not merely +/// obsolete, it is now the defect — an orphaned buffer whose next +/// `C-x C-s` recreates a file the user deleted. /// -/// Bite: fails against an implementation that widens reconciliation. +/// Bite, both directions: fails against an exact-path reconciliation +/// (the descendant survives) **and** against a widening that skips +/// phase 1 (a window keeps a removed id). #[test] -fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { +fn rd9_clean_recursive_delete_reconciles_descendants_through_both_phases() { let dir = tempfile::tempdir().expect("tempdir"); let tree = dir.path().join("tree"); std::fs::create_dir(&tree).expect("mkdir"); @@ -8568,6 +8577,13 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { let mut state = pmacs::editor::EditorState::new(); rd_open(&mut state, "B", &inner); + // Display it, so the phase-1 window redirect has something to do. + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(B)") + .exec() + .expect("show the descendant"); let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); assert!(ok, "a clean tree deletes: {err}"); @@ -8580,9 +8596,23 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { .eval() .expect("validity probe"); assert!( - still, - "THE BITE: reconciliation stays exact-path, so the descendant \ - buffer is orphaned rather than removed" + !still, + "THE BITE: a buffer under a recursively deleted directory must be \ + reconciled away, not left bound to a path whose file is gone" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: widening the reconciliation must not promote the \ + dangling-window defect from exact-path to tree-wide; dangling: \ + {dangling:?}" ); } @@ -8633,15 +8663,22 @@ fn rd10_absent_plus_ignore_does_not_destroy_a_modified_buffer() { assert_eq!(text, "unsavedcontent\n", "the unsaved edit survives"); } -/// Criterion 14 — clean duplicates: exactly one match reconciled. +/// Criterion 14 — clean duplicates: **every** match reconciled. /// -/// Bite: fails against an implementation that removes **all** matches. -/// It pins the reconciliation half of Q#RD10 and *only* that: with both -/// buffers clean there is no verdict difference between consulting one -/// match and consulting all, so this setup cannot see validation -/// breadth. Criterion 6 is what detects incomplete validation. +/// **Rewritten by dired Stage 2a** (§6, acceptance 23). This row +/// previously pinned "exactly one", which was Q#RD10's deliberate +/// restraint: removing them all would have routed N buffers through +/// `remove_buffer_and_fire` — phase 2 without phase 1 — so the second +/// duplicate was left alive rather than have its window dangle. +/// `reconcile_delete` composes both phases, so the restraint is gone and +/// the surviving duplicate is now the defect: it is bound to a path +/// whose file no longer exists, and `find_by_path` cannot even see it. +/// +/// Bite: fails against a first-match implementation (one duplicate +/// survives) and against a widening that skips phase 1 (a window keeps +/// a removed id). #[test] -fn rd14_clean_duplicates_reconcile_exactly_one() { +fn rd14_clean_duplicates_all_reconcile() { let dir = tempfile::tempdir().expect("tempdir"); let f = dir.path().join("twin.rs"); std::fs::write(&f, b"twin\n").expect("write"); @@ -8658,6 +8695,13 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .exec() .expect("two clean buffers on one path"); + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(SECOND)") + .exec() + .expect("show the second duplicate"); + let (ok, err) = rd_delete(&mut state, &f, ""); assert!(ok, "two clean duplicates must not block: {err}"); @@ -8668,9 +8712,23 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .eval() .expect("validity probe"); assert!( - first != second, - "THE BITE: exactly one duplicate is reconciled away, not both \ - and not neither (first={first}, second={second})" + !first && !second, + "THE BITE: both buffers bound to the deleted path must be \ + reconciled away; a survivor points at a file that is gone and is \ + invisible to `find_by_path` (first={first}, second={second})" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: removing every match must not leave a window on \ + a removed id; dangling: {dangling:?}" ); } diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs new file mode 100644 index 0000000..c9beba4 --- /dev/null +++ b/tests/resource_reconciliation_acceptance.rs @@ -0,0 +1,2045 @@ +//! dired Stage 2a acceptance — rename and delete reconciliation. +//! +//! `docs/dired-stage2-framing.md` §5, §6, §10; acceptance items 23–38 +//! and 50–55. +//! +//! **This suite contains no dired content.** Stage 2a ships no dired +//! surface at all: it is the substrate transaction that Stage 2b's `R`, +//! `D` and `x` then stand on, and it closes three defects on `main` +//! that need no dired to be worth fixing — the workspace-edit phantom +//! buffer, the raw first-match registry lookup both `apply_resource_op` +//! arms used, and the incomplete removal lifecycle. +//! +//! Two disciplines the framing forces on every row here: +//! +//! * **Drive the real entry point.** A reconciliation with no +//! production caller passes every direct-call test, so the rename +//! rows go through `pmacs.fs.rename` (worker-dispatched, harvested in +//! the drain) or through `pmacs.buffer.apply_resource_op` (synchronous, +//! main-thread), never through `EditorCore::reconcile_rename`. +//! * **Pump to quiescence, never to a frame count**, because every +//! mutation is worker-dispatched. + +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; + +use pmacs::editor::EditorState; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn exec(state: &EditorState, source: &str) { + state + .lua_host + .lua() + .load(source.to_owned()) + .exec() + .unwrap_or_else(|e| panic!("lua exec failed: {e}\n--- source ---\n{source}")); +} + +fn eval(state: &EditorState, source: &str) -> T { + state + .lua_host + .lua() + .load(source.to_owned()) + .eval() + .unwrap_or_else(|e| panic!("lua eval failed: {e}\n--- source ---\n{source}")) +} + +/// Escape a path for embedding in a Lua double-quoted string. +fn lua_str(path: &Path) -> String { + path.display() + .to_string() + .replace('\\', "\\\\") + .replace('"', "\\\"") +} + +/// Pump the async runtime until `predicate` holds or the deadline +/// lapses. Quiescence, not a frame count: the whole point of items 24 +/// and 25 is that the reconciliation happens in the drain, and the drain +/// runs whenever a reply arrives. +fn pump_until bool>(state: &mut EditorState, what: &str, predicate: F) { + let deadline = Instant::now() + Duration::from_secs(5); + while !predicate(state) { + assert!(Instant::now() < deadline, "pump deadline exceeded: {what}"); + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// Pump a fixed number of times without any expectation. Used only to +/// give a dispatched job every chance to settle before asserting that +/// something did **not** happen. +fn pump_a_while(state: &mut EditorState) { + for _ in 0..80 { + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// A canonicalized temp directory. Canonicalized because the buffer +/// registry stores lexically-normalized absolute paths and macOS's +/// `/var` is a symlink to `/private/var`; without this the expected +/// paths below would differ from the stored ones by a symlink hop. +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +impl Fixture { + fn new() -> Self { + let dir = tempfile::tempdir().unwrap(); + let root = std::fs::canonicalize(dir.path()).unwrap(); + Self { _dir: dir, root } + } + + fn write(&self, rel: &str, contents: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, contents).unwrap(); + path + } + + fn dir(&self, rel: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(&path).unwrap(); + path + } + + fn at(&self, rel: &str) -> PathBuf { + self.root.join(rel) + } +} + +fn editor() -> EditorState { + let state = EditorState::new(); + // No language server may spawn from these fixtures. The LSP rows + // that DO want one configure it explicitly. + exec(&state, "pmacs.lsp.config = {}"); + state +} + +/// Open `path` into a buffer and return the Lua global name holding its +/// handle. Buffers are held on globals so a test can re-read their path +/// and name after the reconciliation moved them. +fn open_as(state: &EditorState, global: &str, path: &Path) { + exec( + state, + &format!( + "_G.{global} = pmacs.buffer.find_or_open(\"{}\")", + lua_str(path) + ), + ); +} + +fn buffer_path(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:path() or nil"), + ) +} + +fn buffer_name(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:name() or nil"), + ) +} + +fn status(state: &EditorState) -> String { + state.core.borrow().status.clone() +} + +fn buffer_is_valid(state: &EditorState, global: &str) -> bool { + eval( + state, + &format!("local b = _G.{global}; return (b ~= nil) and b:is_valid()"), + ) +} + +/// Dispatch a rename **without awaiting** the handle, then pump. +/// Fire-and-forget is the shape item 25 pins: the reconciliation must +/// not live at result consumption. +fn rename_fire_and_forget(state: &mut EditorState, from: &Path, to: &Path) { + exec( + state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(from), + lua_str(to) + ), + ); + pump_until(state, "rename lands on disk", |_| to.exists()); + // The rename landing on disk and the reply reaching the main thread + // are two events; pump past the first to reach the second. + pump_a_while(state); +} + +fn remove_fire_and_forget(state: &mut EditorState, path: &Path) { + exec(state, &format!("pmacs.fs.remove(\"{}\")", lua_str(path))); + pump_until(state, "remove lands on disk", |_| !path.exists()); + pump_a_while(state); +} + +// --------------------------------------------------------------------------- +// 25 — no-await rename +// --------------------------------------------------------------------------- + +/// Acceptance 25. Dispatch `pmacs.fs.rename`, **never take the +/// result**, pump: the open buffer's path has moved. +/// +/// Bite: fails if the reconciliation lives at result consumption +/// (`_take_result`) rather than in the drain, because nothing here ever +/// consumes the handle. +#[test] +fn acc25_a_never_awaited_rename_still_moves_the_open_buffers_path() { + let fx = Fixture::new(); + let old = fx.write("notes.txt", "hello\n"); + let new = fx.at("renamed.txt"); + let mut state = editor(); + open_as(&state, "B", &old); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()) + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must follow a fire-and-forget rename" + ); +} + +// --------------------------------------------------------------------------- +// 26, 27, 28 — the walk +// --------------------------------------------------------------------------- + +/// Acceptance 26. A buffer open on `dir/child.txt` follows +/// `dir` → `newdir`. +#[test] +fn acc26_a_buffer_under_a_renamed_directory_follows_it() { + let fx = Fixture::new(); + fx.dir("tree"); + let child = fx.write("tree/child.txt", "x\n"); + let mut state = editor(); + open_as(&state, "B", &child); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(fx.at("newtree/child.txt").to_str().unwrap()), + "a descendant keeps its relative tail under the new root" + ); +} + +/// Acceptance 27. **Every** match, not the first: two descendant +/// buffers under the renamed directory *and* two buffers visiting the +/// same exact path all move. +/// +/// Bite: fails against `find_by_path`'s first match. One child buffer +/// would not defeat a first-match implementation; this does, twice over +/// — and the duplicate-path pair is the case `find_by_path` cannot even +/// see, because it returns on the first hit. +#[test] +fn acc27_every_affected_buffer_moves_not_only_the_first() { + let fx = Fixture::new(); + fx.dir("tree"); + let one = fx.write("tree/one.txt", "1\n"); + let two = fx.write("tree/nested/two.txt", "2\n"); + let mut state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &two); + // Two buffers on the SAME exact path. `pmacs.buffer.from_file` + // creates a fresh buffer without deduping, which is how a duplicate + // path binding is reachable from public Lua. + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + let dup_first: String = eval(&state, "return _G.ONE:path()"); + let dup_second: String = eval(&state, "return _G.DUP:path()"); + assert_eq!( + dup_first, dup_second, + "precondition: two distinct buffers bound to one path" + ); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "ONE").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "first descendant" + ); + assert_eq!( + buffer_path(&state, "TWO").as_deref(), + Some(fx.at("newtree/nested/two.txt").to_str().unwrap()), + "second, more deeply nested descendant" + ); + assert_eq!( + buffer_path(&state, "DUP").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "the second buffer on the same path — invisible to a first-match \ + lookup, and left pointing at nothing by one" + ); +} + +/// Acceptance 28. Renaming `/…/foo` must not rebind a buffer on +/// `/…/foobar`. +/// +/// Bite: fails against a string `starts_with` instead of a +/// path-component prefix. +#[test] +fn acc28_a_false_string_prefix_is_not_a_path_prefix() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let mut state = editor(); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + rename_fire_and_forget(&mut state, &fx.at("foo"), &fx.at("renamed")); + + assert_eq!( + buffer_path(&state, "IN").as_deref(), + Some(fx.at("renamed/a.txt").to_str().unwrap()), + "the real descendant moves" + ); + assert_eq!( + buffer_path(&state, "OUT").as_deref(), + Some(sibling.to_str().unwrap()), + "`foobar.txt` shares a string prefix with `foo` and is not under it" + ); +} + +/// Acceptance 28, delete side — **and this is the row that bites.** +/// +/// The rename row above cannot falsify a string-prefix walk on its own: +/// `reconcile_rename` calls `Path::strip_prefix` to rebuild the +/// descendant's tail, and that is component-aware too, so a false +/// prefix match is silently dropped a second time and the buffer stays +/// put. Deletion has no such second guard — the walk's verdict IS the +/// kill list — so the containment rule has to be pinned here. +/// +/// Bite: a string `starts_with` instead of `Path::starts_with` kills a +/// buffer on `foobar.txt` when `foo/` is deleted. +#[test] +fn acc28_delete_a_false_string_prefix_does_not_widen_the_kill_list() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("foo")) + ), + ); + + assert!(!fx.at("foo").exists(), "the directory is gone"); + assert!( + !buffer_is_valid(&state, "IN"), + "the real descendant is reconciled away" + ); + assert!( + buffer_is_valid(&state, "OUT"), + "`foobar.txt` shares a string prefix with `foo` and is not under \ + it — killing it destroys an unrelated buffer whose file still \ + exists" + ); + assert!( + sibling.exists(), + "and that file is indeed still on disk, which is what makes the \ + kill wrong rather than merely early" + ); +} + +// --------------------------------------------------------------------------- +// 29 — name provenance, both directions +// --------------------------------------------------------------------------- + +/// Acceptance 29(a). A buffer opened by a **relative** path is named +/// `foo.rs` while its stored path is absolute, and its name follows the +/// rename because its load site recorded `PathDerived`. +/// +/// Bite: rev 7's string-equality rule fails this — the name never +/// equalled the normalized path, so it would have been left stale while +/// insisting it was user-chosen. +#[test] +fn acc29a_a_relative_opens_name_follows_the_rename() { + let fx = Fixture::new(); + let old = fx.write("relative.txt", "x\n"); + let new = fx.at("moved.txt"); + let mut state = editor(); + // Open by a path whose *spelling* is not the stored path: a `.` + // component is folded by normalization but kept in the name, which + // reproduces the relative-open shape without depending on the + // process cwd. + let as_given = fx.at("./relative.txt"); + open_as(&state, "B", &as_given); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(as_given.to_str().unwrap()), + "precondition: the name is the path AS GIVEN, not the stored path" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: only the stored path is normalized" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()) + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the name follows because the load site recorded path provenance, \ + not because the old name happened to match the old path" + ); +} + +/// Acceptance 29(b). A name set explicitly through +/// `pmacs.buffer.set_name` survives the rename **even when that string +/// normalizes to the file's own stored path**. +/// +/// Bite: rev 8's path-equivalence heuristic fails this — the chosen +/// name normalizes to the exact stored path, so the heuristic would +/// overwrite it. +#[test] +fn acc29b_an_explicitly_set_name_survives_even_when_it_denotes_the_file() { + let fx = Fixture::new(); + let old = fx.write("notes", "x\n"); + let new = fx.at("notes-renamed"); + let mut state = editor(); + open_as(&state, "B", &old); + // The chosen name IS the file's absolute path. Under a + // path-equivalence rule this is indistinguishable from a + // path-derived name; under recorded provenance it is not. + exec( + &state, + &format!("pmacs.buffer.set_name(_G.B, \"{}\")", lua_str(&old)), + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: the explicit name normalizes to the stored path" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the PATH always follows" + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "and the explicitly chosen name does not, however much it looks \ + like a path-derived one" + ); +} + +// --------------------------------------------------------------------------- +// 36, 37 — the synchronous arm, and failure +// --------------------------------------------------------------------------- + +/// Acceptance 36. `apply_resource_op`'s rename finds a buffer whose +/// stored path is normalized but whose op names it **un-normalized**. +/// +/// Bite: fails against the raw `find_by_path(&from)` this arm used — +/// stored paths are normalized on write, so a raw lookup with a `.` +/// component in it misses the buffer entirely and the rename silently +/// reconciles nothing. +#[test] +fn acc36_the_synchronous_arm_matches_an_un_normalized_op_path() { + let fx = Fixture::new(); + let old = fx.write("sync.txt", "x\n"); + let new = fx.at("sync-moved.txt"); + let state = editor(); + open_as(&state, "B", &old); + + let unnormalized = fx.at("./sync.txt"); + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"rename\", \ + old_path = \"{}\", new_path = \"{}\" }}", + lua_str(&unnormalized), + lua_str(&new) + ), + ); + + assert!(new.exists(), "the rename happened on disk"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must be found even though the op spelled the source \ + path differently from the stored one" + ); +} + +/// Acceptance 37. A **failed** rename reconciles nothing — and fires no +/// hook. +#[test] +fn acc37_a_failed_rename_reconciles_nothing_and_fires_no_hook() { + let fx = Fixture::new(); + let present = fx.write("present.txt", "x\n"); + let missing = fx.at("does-not-exist.txt"); + let mut state = editor(); + open_as(&state, "B", &present); + exec( + &state, + "_G.FIRED = 0 + pmacs.hook.add('resource.renamed', function() _G.FIRED = _G.FIRED + 1 end)", + ); + + // Renaming a path that does not exist fails in the worker. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&missing), + lua_str(&fx.at("target.txt")) + ), + ); + pump_a_while(&mut state); + + let fired: i64 = eval(&state, "return _G.FIRED"); + assert_eq!(fired, 0, "a failed mutation reconciles nothing"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(present.to_str().unwrap()), + "and no unrelated buffer moved" + ); +} + +// --------------------------------------------------------------------------- +// 50, 55 — the hooks +// --------------------------------------------------------------------------- + +/// Acceptance 50. `resource.renamed` fires **exactly once** per +/// successful rename, with `(old, new)` as normalized absolute paths, +/// and does not fire for a rename that failed. The symmetric assertion +/// for `resource.deleted` accompanies it. +/// +/// Bite: fails if the hook fires for a failed rename, or fires with the +/// un-normalized path a caller happened to spell. +#[test] +fn acc50_the_hooks_fire_once_with_normalized_paths() { + let fx = Fixture::new(); + let old = fx.write("hooked.txt", "x\n"); + let new = fx.at("hooked-moved.txt"); + let doomed = fx.write("doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.RENAMES = {} + _G.DELETES = {} + pmacs.hook.add('resource.renamed', function(a, b) + _G.RENAMES[#_G.RENAMES + 1] = tostring(a) .. ' -> ' .. tostring(b) + end) + pmacs.hook.add('resource.deleted', function(p) + _G.DELETES[#_G.DELETES + 1] = tostring(p) + end)", + ); + + // Spell BOTH paths un-normalized, so the hook's arguments can only + // be canonical if the fire site normalizes them. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&fx.at("./hooked.txt")), + lua_str(&fx.at("./hooked-moved.txt")) + ), + ); + pump_until(&mut state, "rename hook", |s| { + let n: i64 = eval(s, "return #_G.RENAMES"); + n > 0 + }); + pump_a_while(&mut state); + + let renames: String = eval(&state, "return table.concat(_G.RENAMES, '|')"); + assert_eq!( + renames, + format!("{} -> {}", old.display(), new.display()), + "exactly one row, and both paths canonical — a path-keyed \ + subscriber needs the form the registry keys on" + ); + + exec( + &state, + &format!("pmacs.fs.remove(\"{}\")", lua_str(&fx.at("./doomed.txt"))), + ); + pump_until(&mut state, "delete hook", |s| { + let n: i64 = eval(s, "return #_G.DELETES"); + n > 0 + }); + pump_a_while(&mut state); + let deletes: String = eval(&state, "return table.concat(_G.DELETES, '|')"); + assert_eq!( + deletes, + doomed.display().to_string(), + "one row, canonical path" + ); +} + +/// Acceptance 55. Both hooks are `all-must-succeed`, not +/// short-circuit: with two subscribers registered and the **first one +/// raising**, the second still runs and the error is reported rather +/// than swallowed. +/// +/// Bite: fails against a `short-circuit` registration, where the first +/// subscriber's return would stop the fan-out and silently prevent every +/// later one from reconciling — which no test asserting only "the hook +/// fired" would catch. +#[test] +fn acc55_a_raising_subscriber_does_not_stop_the_fan_out() { + let fx = Fixture::new(); + let old = fx.write("fanout.txt", "x\n"); + let new = fx.at("fanout-moved.txt"); + let doomed = fx.write("fanout-doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.SECOND_RAN = 0 + _G.SECOND_DELETED = 0 + pmacs.hook.add('resource.renamed', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.renamed', function() _G.SECOND_RAN = _G.SECOND_RAN + 1 end) + pmacs.hook.add('resource.deleted', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.deleted', function() _G.SECOND_DELETED = _G.SECOND_DELETED + 1 end)", + ); + + rename_fire_and_forget(&mut state, &old, &new); + let ran: i64 = eval(&state, "return _G.SECOND_RAN"); + assert_eq!( + ran, 1, + "`all-must-succeed` collects the first callback's error and \ + continues; a short-circuit registration would have stopped here" + ); + + remove_fire_and_forget(&mut state, &doomed); + let deleted: i64 = eval(&state, "return _G.SECOND_DELETED"); + assert_eq!(deleted, 1, "same for `resource.deleted`"); + + // The error is reported, not swallowed: the hook error log is the + // `*errors*` buffer. + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("first subscriber explodes"), + "the raising subscriber's error must be reported; *errors* held: \ + {errors:?}" + ); +} + +// --------------------------------------------------------------------------- +// 23, 24 — the delete lookups +// --------------------------------------------------------------------------- + +/// Acceptance 23. `apply_resource_op`'s delete reaches **descendants** +/// and a **second buffer on the same path** — the raw first-match lookup +/// replaced by the shared prefix-aware, normalizing query. +/// +/// (#190 owns the modified-buffer refusal; it refuses before disk, so by +/// the time this lane's reconciliation runs there is no modified buffer +/// on the synchronous path to spare. This row asserts the lookup fix.) +#[test] +fn acc23_the_synchronous_delete_reaches_descendants_and_duplicates() { + let fx = Fixture::new(); + fx.dir("tree/nested"); + let one = fx.write("tree/one.txt", "1\n"); + fx.write("tree/nested/two.txt", "2\n"); + let state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &fx.at("tree/nested/two.txt")); + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + // Keep an unrelated buffer alive so the last-buffer refusal is not + // what this row measures. + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("./tree")) + ), + ); + + assert!(!fx.at("tree").exists(), "the tree is gone from disk"); + assert!( + !buffer_is_valid(&state, "ONE"), + "a buffer directly under the deleted directory" + ); + assert!( + !buffer_is_valid(&state, "TWO"), + "a more deeply nested descendant" + ); + assert!( + !buffer_is_valid(&state, "DUP"), + "the second buffer on the same path — the one a first-match \ + lookup cannot see, which #190 deliberately left in place \ + because it had no two-phase kill to route it through" + ); + assert!(buffer_is_valid(&state, "KEEP"), "an unrelated buffer"); +} + +/// Acceptance 24. A **fire-and-forget** `pmacs.fs.remove` reconciles +/// too: never taking the handle still kills the unmodified buffer, which +/// is what makes the drain harvest the right seam rather than dired +/// firing the hook itself. +#[test] +fn acc24_a_never_awaited_remove_still_kills_the_unmodified_buffer() { + let fx = Fixture::new(); + let doomed = fx.write("gone.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + assert!(buffer_is_valid(&state, "B")); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + !buffer_is_valid(&state, "B"), + "the harvest must reconcile a delete no one awaited" + ); + assert!(buffer_is_valid(&state, "KEEP")); +} + +/// Acceptance 18's substrate half, and §6's modified case: a **modified** +/// buffer whose file is deleted out from under it keeps its contents. The +/// buffer half is the part of the promise that is robust, because it runs +/// at drain time against whatever state exists then. +#[test] +fn a_modified_buffer_survives_a_delete_with_its_contents() { + let fx = Fixture::new(); + let doomed = fx.write("dirty.txt", "original\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "_G.B:insert(0, 'edited ')"); + let modified: bool = eval(&state, "return _G.B:is_modified()"); + assert!(modified, "precondition"); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + buffer_is_valid(&state, "B"), + "a modified buffer is kept alive deliberately, not killed" + ); + let contents: String = eval(&state, "return _G.B:slice(0, _G.B:len())"); + assert_eq!(contents, "edited original\n", "with its contents intact"); + assert!(!doomed.exists(), "while the file is gone"); +} + +// --------------------------------------------------------------------------- +// 51, 52, 53 — the removal lifecycle +// --------------------------------------------------------------------------- + +/// Acceptance 51. A killed buffer completes **both** removal phases: +/// after a delete reconciles, an `on_removed` callback registered for +/// that buffer has fired and its buffer-local keymap entries are gone. +/// +/// Bite: fails against an implementation that calls only +/// `EditorCore::kill_buffer`, which does no phase-2 cleanup at all. +/// +/// Note 51 and 52 are a matched pair and **neither alone is +/// sufficient** — each pre-existing removal path passes one and fails +/// the other, which is exactly why both phases had to be named. +#[test] +fn acc51_a_killed_buffer_completes_both_removal_phases() { + let fx = Fixture::new(); + let doomed = fx.write("phase2.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec( + &state, + "_G.ON_REMOVED = 0 + pmacs.buffer.on_removed(_G.B, function() _G.ON_REMOVED = _G.ON_REMOVED + 1 end) + pmacs.command.define { name = 'test.noop', description = 'x', fn = function() end } + pmacs.keymap.bind { scope = 'buffer', buffer = _G.B, + sequence = 'C-c C-1', command = 'test.noop' } + -- `pmacs.keymap.lookup` is deliberately raw-global, so the + -- buffer-scoped row is only visible through `list()`. + function _G.BOUND_ROWS() + local n = 0 + for _, e in ipairs(pmacs.keymap.list()) do + if e.command == 'test.noop' then n = n + 1 end + end + return n + end", + ); + let bound_before: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_before, 1, + "precondition: the buffer-local binding exists" + ); + + remove_fire_and_forget(&mut state, &doomed); + + assert!(!buffer_is_valid(&state, "B"), "phase 1 removed the buffer"); + let fired: i64 = eval(&state, "return _G.ON_REMOVED"); + assert_eq!( + fired, 1, + "phase 2 must fire the registered on_removed callback; 0 means the \ + reconciliation called `EditorCore::kill_buffer` alone" + ); + let bound_after: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_after, 0, + "phase 2 must purge the buffer-scoped keymap, so a later buffer \ + cannot inherit a dead one's bindings" + ); +} + +/// Acceptance 52. A window displaying the deleted buffer is +/// **redirected**, not left dangling: no window holds a removed id. +/// +/// Bite: fails against `remove_buffer_and_fire`, which is what +/// `apply_resource_op` used — phase 2 without phase 1, so +/// `BufferRegistry::remove` runs while every window showing the buffer +/// keeps pointing at the id it just dropped. +#[test] +fn acc52_a_window_showing_the_deleted_buffer_is_redirected() { + let fx = Fixture::new(); + let doomed = fx.write("shown.txt", "x\n"); + let state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let doomed_id = state.core.borrow().active_buffer_id(); + assert!( + state + .core + .borrow() + .windows + .values() + .any(|w| w.buffer_id == doomed_id), + "precondition: a window shows the doomed buffer" + ); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", path = \"{}\" }}", + lua_str(&doomed) + ), + ); + + let core = state.core.borrow(); + assert!( + !core.registry.borrow().contains(doomed_id), + "the buffer was removed" + ); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "no window may hold a removed buffer id; dangling: {dangling:?}" + ); + assert!( + !core.windows.values().any(|w| w.buffer_id == doomed_id), + "and specifically not the deleted one" + ); +} + +/// Acceptance 53. The last-buffer and mid-edit refusals are +/// **reported, not silent**, and neither aborts the reconciliation of +/// other buffers. +#[test] +fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { + // Half one: the file behind the only open buffer. `kill_buffer` + // refuses to remove the last remaining buffer, so the file goes and + // the buffer stays. + let fx = Fixture::new(); + let only = fx.write("only.txt", "x\n"); + let mut state = editor(); + // Drop every other buffer so the target really is the last one. + exec( + &state, + &format!( + "_G.ONLY = pmacs.buffer.find_or_open(\"{}\") + for _, b in ipairs(pmacs.buffer.list()) do + if tostring(b) ~= tostring(_G.ONLY) then + pcall(pmacs.buffer.kill, b) + end + end + return #pmacs.buffer.list()", + lua_str(&only) + ), + ); + let count: i64 = eval(&state, "return #pmacs.buffer.list()"); + assert_eq!(count, 1, "precondition: exactly one buffer is open"); + + remove_fire_and_forget(&mut state, &only); + assert!( + buffer_is_valid(&state, "ONLY"), + "the last remaining buffer cannot be killed, so it survives the \ + deletion of its file" + ); + // **Reported, not silent.** Survival alone is not the criterion: the + // buffer is still bound to a path whose file is gone, so the next + // `C-x C-s` recreates the file the user deleted. That is recoverable + // only if the user is told. + let said = status(&state); + assert!( + said.contains("could not be closed"), + "the refusal must reach the status channel; status was {said:?}" + ); + // Asserted as the buffer's OWN name, not as the basename. The + // message opens with `deleted only.txt:` — the *path* — so a + // `contains("only.txt")` check passes with the attribution stripped, + // which is exactly how this assertion was vacuous when first + // written. A path-backed buffer's name is the full path, and only + // the `buffer "…"` prefix can produce it. + let expect_named = format!("buffer {:?}", only.display().to_string()); + assert!( + said.contains(&expect_named), + "the refusal must name the buffer, because `cannot kill the last \ + remaining buffer` alone does not say WHICH buffer is now bound \ + to a deleted path; wanted {expect_named:?} in {said:?}" + ); + + // Half two: a directory of buffers where one refuses removal. The + // rest must still reconcile. + let fx2 = Fixture::new(); + fx2.dir("batch"); + let a = fx2.write("batch/a.txt", "a\n"); + let b = fx2.write("batch/b.txt", "b\n"); + let c = fx2.write("batch/c.txt", "c\n"); + let mut state2 = editor(); + open_as(&state2, "KEEP", &fx2.write("keep.txt", "k\n")); + open_as(&state2, "A", &a); + open_as(&state2, "B", &b); + open_as(&state2, "C", &c); + // B refuses: it is modified. + exec(&state2, "_G.B:insert(0, 'dirty ')"); + + remove_fire_and_forget(&mut state2, &fx2.at("batch/a.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/b.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/c.txt")); + + assert!(!buffer_is_valid(&state2, "A"), "A reconciled"); + assert!( + buffer_is_valid(&state2, "B"), + "B was kept because it is modified" + ); + assert!( + !buffer_is_valid(&state2, "C"), + "and C still reconciled afterwards — one refusal must not abort \ + the rest" + ); + // The kept-modified case reports too, and says what the consequence + // is. This is the asynchronous race the framing's H1 leaves open: + // #190 refuses before disk on the synchronous path, but + // `pmacs.fs.remove` dispatches a worker, so a buffer modified in the + // interval reaches the drain with its file already gone. + let said2 = status(&state2); + assert!( + said2.contains("unsaved changes kept"), + "a modified buffer kept alive over a deleted file must be \ + reported; status was {said2:?}" + ); + assert!( + said2.contains("RECREATE"), + "and the report must state the consequence — saving it puts the \ + deleted file back; status was {said2:?}" + ); + // Same discipline: the full path is the buffer's name, while the + // message's `deleted b.txt:` prefix is only the basename. + assert!( + said2.contains(&b.display().to_string()), + "the kept buffer must be named, and by its own name rather than \ + the deleted path's basename; status was {said2:?}" + ); +} + +// --------------------------------------------------------------------------- +// 53b — a mid-edit refusal leaves editor state UNCHANGED +// --------------------------------------------------------------------------- + +/// Acceptance 53b, all three assertions, **stated individually**. +/// +/// With the buffer `editing_in_progress`, displayed in an ordinary +/// window, shown in a side window, and present in `round_trip_buffers`, +/// a delete reconciling it must leave each of the following provably +/// untouched. Each fails independently against the same one-line bite — +/// removing the `editing_in_progress` preflight — which is the point: a +/// single compound assertion can pass on two of the three and hide the +/// third. +/// +/// | # | Assertion | What the missing preflight breaks | +/// |---|---|---| +/// | i | the ordinary window still shows the buffer, cursor/selection/`view_top` intact | `kill_buffer` redirects the window to the fallback before `BufferRegistry::remove` refuses | +/// | ii | the side window is still open and still shows the buffer | `remove_side_window` collapses it first | +/// | iii | the buffer is still in `round_trip_buffers` | `round_trip_buffers.remove` runs first — the **first** thing `kill_buffer` does, and the easiest to miss | +#[test] +#[allow( + clippy::too_many_lines, + reason = "three independent assertions, each with its own precondition; a compound check is exactly what this row exists to avoid" +)] +fn acc53b_a_mid_edit_refusal_leaves_window_side_and_round_trip_state_untouched() { + let fx = Fixture::new(); + let doomed = fx.write("midedit.txt", "0123456789\nsecond line\n"); + let mut state = editor(); + // A grid frontend's real frame size is its declaration, and a side + // window needs one before it can be placed. + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + exec(&state, "pmacs.buffer.set_round_trip_input(_G.B, true)"); + + let doomed_id = state.core.borrow().active_buffer_id(); + let ordinary_window = state.core.borrow().active_window_id(); + // Seat a distinctive cursor + selection + scroll position on the + // ORDINARY window, so a redirect is detectable as more than "the + // window moved". + { + let mut core = state.core.borrow_mut(); + let win = core + .windows + .get_mut(&ordinary_window) + .expect("ordinary window"); + win.cursor = 4; + win.selection = Some(pmacs::window::Selection { anchor: 2 }); + win.view_top = 1; + } + + // A SIDE window showing the same buffer, so the collapse the + // preflight prevents has something to collapse. + exec( + &state, + "pmacs.window.display(_G.B, { side = \"bottom\", height = 4 })", + ); + let side_windows: Vec<_> = state + .core + .borrow() + .side_window_for(pmacs::protocol::FrontendId::LOCAL) + .into_iter() + .collect(); + assert!( + !side_windows.is_empty(), + "precondition: a side window exists" + ); + assert_eq!( + state + .core + .borrow() + .windows + .get(&side_windows[0]) + .expect("side window") + .buffer_id, + doomed_id, + "precondition: the side window shows the doomed buffer" + ); + assert_ne!( + side_windows[0], ordinary_window, + "precondition: the side window is a second window" + ); + assert!( + state.core.borrow().buffer_round_trips(doomed_id), + "precondition: the buffer round-trips input" + ); + + // Put the buffer mid-edit. `begin_edit` is the flag + // `BufferRegistry::remove` refuses on, and the whole point of the + // preflight is that the refusal arrives too late. + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(doomed_id) + .expect("doomed buffer") + .begin_edit() + .expect("begin edit"); + } + + remove_fire_and_forget(&mut state, &doomed); + + let core = state.core.borrow(); + // (i) the ordinary window, with its seated state. + let win = core + .windows + .get(&ordinary_window) + .expect("the ordinary window still exists"); + assert_eq!( + win.buffer_id, doomed_id, + "(i) the ordinary window must still show the buffer" + ); + assert_eq!(win.cursor, 4, "(i) cursor"); + assert_eq!( + win.selection, + Some(pmacs::window::Selection { anchor: 2 }), + "(i) selection" + ); + assert_eq!(win.view_top, 1, "(i) view_top"); + + // (ii) the side window. + for side in &side_windows { + let side_win = core.windows.get(side).unwrap_or_else(|| { + panic!( + "(ii) side window {side:?} was collapsed by a kill that should never have started" + ) + }); + assert_eq!( + side_win.buffer_id, doomed_id, + "(ii) the side window must still show the buffer" + ); + } + + // (iii) round-trip membership. + assert!( + core.buffer_round_trips(doomed_id), + "(iii) the buffer must still round-trip input — this is the FIRST \ + thing `kill_buffer` drops and the easiest to miss" + ); + + assert!( + core.registry.borrow().contains(doomed_id), + "and the buffer itself is still in the registry" + ); + drop(core); + // And the refusal is REPORTED. Leaving state untouched is only half + // the contract: the file is gone, so a user who is not told keeps a + // buffer bound to a path that no longer exists. + let said = status(&state); + assert!( + said.contains("mid-edit") && said.contains("could not be closed"), + "a mid-edit refusal must reach the status channel; status was \ + {said:?}" + ); +} + +// --------------------------------------------------------------------------- +// 54 — independent mutations both reconcile, in either arrival order +// --------------------------------------------------------------------------- + +/// Acceptance 54, integration layer. Dispatch a rename and a delete on +/// **disjoint** paths, wait for both, and assert both registry effects +/// occurred. It fails against dropping or deduplicating one resource +/// kind. +/// +/// The disjoint end state is confidence coverage, **not** a claimed bite +/// against interdependent sequencing: disjoint paths necessarily +/// commute. The controlled-bus layer that does pin arrival order lives +/// in `src/async_runtime.rs`, and no test here pretends to pin an order +/// the mechanism does not establish. +#[test] +fn acc54_a_rename_and_a_delete_on_disjoint_paths_both_reconcile() { + for reverse in [false, true] { + let fx = Fixture::new(); + let renamed_from = fx.write("moves.txt", "m\n"); + let renamed_to = fx.at("moved.txt"); + let deleted = fx.write("goes.txt", "g\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "MOVES", &renamed_from); + open_as(&state, "GOES", &deleted); + + let rename = format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&renamed_from), + lua_str(&renamed_to) + ); + let remove = format!("pmacs.fs.remove(\"{}\")", lua_str(&deleted)); + if reverse { + exec(&state, &remove); + exec(&state, &rename); + } else { + exec(&state, &rename); + exec(&state, &remove); + } + + pump_until(&mut state, "both mutations", |s| { + renamed_to.exists() && !deleted.exists() && !buffer_is_valid(s, "GOES") + }); + pump_a_while(&mut state); + + assert_eq!( + buffer_path(&state, "MOVES").as_deref(), + Some(renamed_to.to_str().unwrap()), + "the rename reconciled (dispatch order reversed: {reverse})" + ); + assert!( + !buffer_is_valid(&state, "GOES"), + "the delete reconciled (dispatch order reversed: {reverse})" + ); + } +} + +// --------------------------------------------------------------------------- +// The LSP-facing rows (30, 31c, 32, 34, 35) +// --------------------------------------------------------------------------- +// +// Driven against `pmacs_fake_lsp` so nothing here needs a real toolchain +// on PATH. The fake publishes two synthetic diagnostics (one Error at +// line 0, one Warning at line 2) on every `didOpen`, which is what makes +// "the new URI's diagnostics" observable at all. + +fn fake_lsp_path() -> String { + env!("CARGO_BIN_EXE_pmacs_fake_lsp").to_owned() +} + +/// Configure `language` to spawn the fake, and pin the project-marker +/// walk to `root` so a stray `.git` above the tempdir cannot silently +/// turn a markerless fixture into a detected one. +fn configure_fake(state: &EditorState, root: &Path, language: &str) { + exec( + state, + &format!( + "pmacs.project.set_search_boundary(\"{}\") + pmacs.lsp.config.{language} = {{ command = \"{}\" }}", + lua_str(root), + fake_lsp_path() + ), + ); +} + +/// Pump the real frame order — processes, LSP, async — until `predicate` +/// holds. All three are needed: the fake's frames arrive through the +/// supervisor, the manager parses them, and the rename settles on the +/// async bus. +fn settle_until bool>( + state: &mut EditorState, + what: &str, + predicate: F, +) { + let deadline = Instant::now() + Duration::from_secs(20); + while !predicate(state) { + assert!( + Instant::now() < deadline, + "settle deadline exceeded: {what}" + ); + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(5)); + } +} + +fn settle_a_while(state: &mut EditorState) { + for _ in 0..120 { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(3)); + } +} + +fn server_count(state: &EditorState) -> i64 { + eval(state, "return #pmacs.lsp.list()") +} + +/// `language|root_uri|state` per live server, sorted, so assertions do +/// not depend on spawn order. +fn server_rows(state: &EditorState) -> Vec { + let joined: String = eval( + state, + r#" + local out = {} + for _, s in ipairs(pmacs.lsp.list()) do + out[#out + 1] = table.concat({ + s.language_id or "", s.root_uri or "", + (s.state and s.state.kind) or "", + }, "|") + end + table.sort(out) + return table.concat(out, "\n") + "#, + ); + if joined.is_empty() { + Vec::new() + } else { + joined.lines().map(str::to_owned).collect() + } +} + +fn diag_count(state: &EditorState, uri: &str) -> i64 { + eval( + state, + &format!( + "local e, w, i, h = pmacs.diag.count(\"{uri}\") + return (e or 0) + (w or 0) + (i or 0) + (h or 0)" + ), + ) +} + +/// Mirror of `file_uri_for` in `builtin/runtime/lsp.lua`. Reimplemented +/// rather than imported, so the test states the expected encoding +/// independently of the code under test. +fn file_uri(path: &Path) -> String { + let mut out = String::from("file://"); + for byte in path.display().to_string().as_bytes() { + match byte { + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'/' | b'-' | b'_' | b'.' | b'~' | b':' => { + out.push(*byte as char); + } + _ => { + use std::fmt::Write as _; + let _ = write!(out, "%{byte:02X}"); + } + } + } + out +} + +/// Every window's overlay kinds, in composition order, keyed by window. +fn overlay_kinds_per_window(state: &EditorState) -> Vec<(u64, Vec<&'static str>)> { + let core = state.core.borrow(); + let mut rows: Vec<(u64, Vec<&'static str>)> = core + .windows + .iter() + .map(|(id, w)| (id.raw(), w.overlay_kinds())) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Count cells carrying a diagnostic **error** underline colour, per +/// window rect, by painting one real frame. +/// +/// This is the only per-window, view-level observation available: +/// `DiagnosticView.uri` is private and `View` has no downcast, so +/// asserting on the store would prove nothing about whether the overlay +/// was re-rooted. A view still pointing at the old URI renders nothing, +/// because `forget_uri` emptied that key. +fn error_underlines_per_window(state: &EditorState) -> Vec<(u64, usize)> { + use pmacs::cell::{Cell, CellGrid, CellSize, Color}; + use pmacs::protocol::FrontendId; + use pmacs::window::Rect; + + let size = CellSize::new(24, 80); + let mut cells = vec![Cell::default(); (size.rows * size.cols) as usize]; + let mut grid = CellGrid { + cells: &mut cells, + stride: size.cols, + size, + }; + pmacs::editor::paint_frame( + state, + FrontendId::LOCAL, + &std::collections::HashMap::new(), + &mut grid, + size, + ); + let placements = { + let core = state.core.borrow(); + let view = core.views.get(&FrontendId::LOCAL).expect("LOCAL view"); + let area = Rect::new(0, 0, size.rows - 1, size.cols); + let fixed = core.panel_fixed_rows(FrontendId::LOCAL, area.size.rows); + view.layout.compute(area, &fixed) + }; + let error = Color::Indexed(1); + let mut rows: Vec<(u64, usize)> = placements + .into_iter() + .map(|(win, rect)| { + let mut n = 0; + for row in rect.origin.row..rect.origin.row + rect.size.rows { + for col in rect.origin.col..rect.origin.col + rect.size.cols { + let idx = (row * size.cols + col) as usize; + if cells.get(idx).map(|c| c.style.underline_color) == Some(error) { + n += 1; + } + } + } + (win.raw(), n) + }) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Acceptance 30. An attached LSP buffer with **diagnostics present +/// before** the rename, shown in **at least two windows**: afterwards +/// both windows render the **new** URI's diagnostics, the old URI's +/// store is empty, and each window's overlay keeps its **position in +/// the composition order**. +/// +/// Bite, two mutations: `rec.uri` updated without re-rooting the +/// diagnostic view (both windows then render nothing, because the old +/// key is empty); and a remove-and-re-push, which would pass a +/// one-window render test while moving the diagnostic overlay to the end +/// of the stack — caught by the composition-order assertion. +#[test] +fn acc30_diagnostics_re_root_in_every_window_and_keep_their_stack_position() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write( + "proj/src/main.rs", + "fn main() {}\n// second\n// third line here\n", + ); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &old); + + let old_uri = file_uri(&old); + let new_uri = file_uri(&new); + settle_until(&mut state, "diagnostics for the old URI", |s| { + diag_count(s, &old_uri) > 0 + }); + + // A second window showing the same buffer, with its own + // `DiagnosticView`. A split alone does not carry one — the view + // does not implement `clone_for_split` — so the switch hook is what + // attaches it, and that path only ever touches the ACTIVE window. + exec(&state, "pmacs.window.split_horizontal()"); + // `try_split_active` leaves focus where it was, so the switch hook — + // which can only reach the ACTIVE window — has to be given the new + // one explicitly. + exec(&state, "pmacs.window.focus_next()"); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + settle_a_while(&mut state); + + // Push one more overlay AFTER the diagnostic in each window. + // Without this the composition-order assertion below cannot bite: + // the LSP attach leaves `diagnostic` LAST in the stack, and moving + // the last element to the end is a no-op, so a remove-and-re-push + // would be indistinguishable from an in-place mutation. + // `_attach_highlight` uses `push_overlay` (no dedup), so a second + // call appends. + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + exec(&state, "pmacs.window.focus_next()"); + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + + let before_kinds = overlay_kinds_per_window(&state); + assert!( + before_kinds + .iter() + .all(|(_, kinds)| kinds.iter().position(|k| *k == "diagnostic") + < Some(kinds.len() - 1)), + "precondition: the diagnostic overlay must NOT be last, or \ + \"keeps its stack position\" is unfalsifiable; got {before_kinds:?}" + ); + let before_paint = error_underlines_per_window(&state); + assert_eq!( + before_paint.len(), + 2, + "precondition: two windows are placed; got {before_paint:?}" + ); + for (win, n) in &before_paint { + assert!( + *n > 0, + "precondition: window {win} must already paint diagnostic \ + underlines; got {before_paint:?} with overlays \ + {before_kinds:?}" + ); + } + let diag_positions_before: Vec<(u64, Option)> = before_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert!( + diag_positions_before.iter().all(|(_, p)| p.is_some()), + "precondition: every window carries a diagnostic overlay; got \ + {before_kinds:?}" + ); + + rename_fire_and_forget(&mut state, &old, &new); + settle_until(&mut state, "diagnostics for the new URI", |s| { + diag_count(s, &new_uri) > 0 + }); + settle_a_while(&mut state); + + assert_eq!( + diag_count(&state, &old_uri), + 0, + "the old URI's store must be empty" + ); + assert!( + diag_count(&state, &new_uri) > 0, + "and the new URI's must be populated" + ); + + let after_kinds = overlay_kinds_per_window(&state); + let diag_positions_after: Vec<(u64, Option)> = after_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert_eq!( + diag_positions_after, diag_positions_before, + "each window's diagnostic overlay must keep its position in the \ + composition order; a remove-and-re-push would move it to the end \ + ({before_kinds:?} -> {after_kinds:?})" + ); + + let after_paint = error_underlines_per_window(&state); + assert_eq!(after_paint.len(), 2, "still two windows: {after_paint:?}"); + for (win, n) in &after_paint { + assert!( + *n > 0, + "window {win} must render the NEW URI's diagnostics; 0 means \ + its overlay is still keyed under the old URI, whose store the \ + forget emptied ({after_paint:?})" + ); + } +} + +/// Acceptance 31c, at the Lua binding. Raises for an unknown server id; +/// **succeeds** for a URI with no state under a known server. +/// +/// The second arm is the one that matters: the `resource.renamed` +/// subscriber calls this per attachment, and an attachment need not have +/// any pending route or populated result store. An over-strict binding +/// would turn that ordinary idempotent case into an error inside a hook. +#[test] +fn acc31c_the_forget_uri_binding_raises_for_an_unknown_server_and_not_for_an_unknown_uri() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let file = fx.write("proj/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &file); + settle_until(&mut state, "one live server", |s| server_count(s) == 1); + + // An unknown id has to be a real handle to a server the manager no + // longer holds: `LspServerIdLua` is opaque and cannot be forged from + // an integer, which is itself the binding's first line of defence. + let raised: String = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + assert(sid, 'no server to stale out') + _G.STALE = sid + pmacs.lsp.stop(sid) + return 'stopped'", + ); + assert_eq!(raised, "stopped"); + settle_until(&mut state, "the server is forgotten", |s| { + let gone: bool = eval( + s, + "local ok = pcall(pmacs.lsp.forget, _G.STALE) + return #pmacs.lsp.list() == 0", + ); + gone + }); + let raised: String = eval( + &state, + "local ok, err = pcall(pmacs.lsp.forget_uri, _G.STALE, 'file:///nope.rs') + if ok then return 'DID NOT RAISE' end + return tostring(err)", + ); + assert!( + raised.contains("unknown server"), + "an unknown server id must raise, matching `pmacs.lsp.forget`; got \ + {raised:?}" + ); + + // And the success arm, against a live server. + open_as( + &state, + "C", + &fx.write("proj/src/second.rs", "fn second() {}\n"), + ); + settle_until(&mut state, "a live server again", |s| server_count(s) == 1); + let ok: bool = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + local a = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + local b = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + return a and b", + ); + assert!( + ok, + "a URI with no state under a known server is an idempotent \ + success, and repeating it stays safe" + ); +} + +/// Acceptance 32. A rename **across project roots** re-runs +/// `ensure_server` and the buffer ends up attached to a **different** +/// server; a same-root rename reuses the existing one (#161's affinity +/// key is the detected project root). +#[test] +fn acc32_a_cross_root_rename_re_runs_ensure_server_and_a_same_root_one_reuses() { + // Same root first: renaming within one package must not spawn a + // second server. + let fx = Fixture::new(); + fx.write("a/Cargo.toml", "[package]\nname = \"a\"\n"); + let inside = fx.write("a/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &inside); + settle_until(&mut state, "one initialized server", |s| { + server_rows(s) == vec![format!("rust|{}|initialized", file_uri(&fx.at("a")))] + }); + let root_a = file_uri(&fx.at("a")); + assert_eq!( + server_rows(&state), + vec![format!("rust|{root_a}|initialized")], + "precondition: one server, rooted at package a" + ); + + rename_fire_and_forget(&mut state, &inside, &fx.at("a/src/moved.rs")); + settle_a_while(&mut state); + assert_eq!( + server_count(&state), + 1, + "a same-root rename reuses the existing server: {:?}", + server_rows(&state) + ); + + // Now across roots: `b` is its own package, so its file needs its + // own server. + fx.write("b/Cargo.toml", "[package]\nname = \"b\"\n"); + std::fs::create_dir_all(fx.at("b/src")).unwrap(); + rename_fire_and_forget( + &mut state, + &fx.at("a/src/moved.rs"), + &fx.at("b/src/moved.rs"), + ); + settle_until(&mut state, "a second server for package b", |s| { + server_count(s) == 2 + }); + settle_a_while(&mut state); + + let root_b = file_uri(&fx.at("b")); + let rows = server_rows(&state); + assert!( + rows.iter().any(|r| r.contains(&root_b)), + "the cross-root rename must spawn a server rooted at package b; \ + rows were {rows:?}" + ); + assert_eq!( + rows.len(), + 2, + "exactly one new server, not one per reconciliation pass: {rows:?}" + ); +} + +/// Point the `rust` server at the `applyeditplan` fake carrying `plan`, +/// and hand back the sink the client's response to the server-initiated +/// `workspace/applyEdit` lands in. Must run before the first `.rs` file +/// is opened — that open is what launches the server. +fn plan_server(state: &EditorState, dir: &Path, plan: &serde_json::Value) -> PathBuf { + let plan_path = dir.join("plan.json"); + std::fs::write(&plan_path, serde_json::to_vec(plan).unwrap()).unwrap(); + let sink = dir.join("applyedit-response.json"); + exec( + state, + &format!( + "pmacs.lsp.config.rust = {{ + command = \"{}\", + env = {{ + PMACS_FAKE_LSP_MODE = 'applyeditplan', + PMACS_FAKE_LSP_EDIT_PLAN = '{}', + PMACS_FAKE_LSP_APPLYEDIT_SINK = '{}', + }}, + }}", + fake_lsp_path(), + plan_path.display(), + sink.display() + ), + ); + sink +} + +/// Ask the fake to deliver its planned `workspace/applyEdit`. Driven by +/// an `executeCommand` rather than fired at `initialized`, so the test +/// controls *when* the batch arrives — these fixtures depend on a +/// specific buffer being active first, and a server-timed request would +/// race that setup. +fn trigger_apply_edit(state: &EditorState) { + exec( + state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do + if row.state and row.state.kind == 'initialized' then sid = row.id end + end + assert(sid, 'no initialized server') + pmacs.lsp.request_execute_command(sid, 'pmacs.fake.applyEdit', {})", + ); +} + +fn wait_for_apply_response(state: &mut EditorState, sink: &Path) -> serde_json::Value { + let deadline = Instant::now() + Duration::from_secs(20); + loop { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if let Ok(raw) = std::fs::read(sink) + && let Ok(v) = serde_json::from_slice::(&raw) + { + assert!( + v.get("fakeError").is_none(), + "the fixture itself failed: {v:?}" + ); + return v; + } + assert!( + Instant::now() < deadline, + "the client never answered the server's workspace/applyEdit" + ); + std::thread::sleep(Duration::from_millis(10)); + } +} + +/// Acceptance 34. Renaming the **active** file through the full +/// `apply_workspace_edit` path returns the user to the **same buffer** +/// (now under its new path), and leaves no buffer bound to the obsolete +/// path. +/// +/// The plan deliberately edits *another* file first. Without that the +/// row cannot bite at all: the applier only has to restore the origin if +/// something moved the active buffer away, and a lone rename op does not. +/// +/// Bite: the applier restoring by path instead of by buffer handle. A +/// captured path no longer resolves after its own batch renamed it, so +/// `find_or_open` raises, the `pcall` swallows it, and the user is +/// stranded in whatever buffer the last applied op left active. No +/// reconciliation can reach a string a Lua local already captured. +/// +/// **One framing claim corrected here.** §5's G1 says the stale path +/// "materializes a phantom": that `find_or_open(origin)` reaches +/// `resolve_target_buffer`'s `NotFound` arm, which creates an empty +/// path-backed buffer and selects it. It does not. +/// `pmacs.buffer.find_or_open` (`src/lua_bindings/mod.rs`) calls +/// `crate::file_io::load_file` directly and maps the error, so a missing +/// path **raises**; the `NotFound` arm belongs to +/// `EditorCore::resolve_target_buffer`, which serves +/// `pmacs.window.display_file` and the startup/daemon target, not this +/// binding. The defect is real but smaller than G1 states — a silently +/// swallowed restore, not a fabricated file — and this row asserts the +/// half that is true. The no-buffer-at-the-old-path assertion is kept as +/// a cheap guard against a future fallback that *would* create one, and +/// is not the biting half. +#[test] +fn acc34_renaming_the_active_file_through_the_applier_returns_the_same_buffer() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [ + { + // Moves the active buffer away, so the restore has + // something to undo. + "textDocument": { "uri": file_uri(&other), "version": 1 }, + "edits": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "// touched\n", + }], + }, + { + "kind": "rename", + "oldUri": file_uri(&old), + "newUri": file_uri(&new), + }, + ], + }), + ); + open_as(&state, "B", &old); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + // The applier restores the buffer that was active when the batch + // began, so make that the file being renamed. + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let active_before = state.core.borrow().active_buffer_id(); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!(new.exists(), "the rename landed on disk"); + assert!(!old.exists()); + assert_eq!( + state.core.borrow().active_buffer_id(), + active_before, + "the user must be returned to the SAME buffer, now under its new \ + path — a path-based restore raises on the renamed-away path, the \ + pcall swallows it, and the user is left wherever the last applied \ + op put them" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "and that buffer's path followed the rename" + ); + + let stale: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&old) + ), + ); + assert!(!stale, "no buffer may remain bound to the obsolete path"); +} + +/// Acceptance 35. When the origin buffer is **gone** after the edit, the +/// applier restores **nothing** rather than falling back to the old +/// path. +/// +/// **The plan deletes the origin's file and then RECREATES it, and that +/// is what makes the row bite at all.** With a plain delete the forbidden +/// fallback is unobservable: `find_or_open` on a path that no longer +/// exists raises straight out of `file_io::load_file`, the surrounding +/// `pcall` swallows it, and nothing happens — so "no buffer at the old +/// path" and "the active buffer is live" both hold with the fallback +/// present. Recreating the path gives the fallback something to open, and +/// it is not a contrived shape: a `documentChanges` batch that deletes +/// and recreates a file is ordinary LSP refactoring output. +/// +/// Bite: the applier restoring by path instead of by buffer handle. The +/// handle is invalid (reconciliation killed the buffer) so a +/// handle-based restore does nothing; a path-based one loads the +/// recreated file into a NEW buffer and switches the user into it — +/// dropping them, silently, into a file they asked to delete. +#[test] +fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let doomed = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [ + { + "kind": "delete", + "uri": file_uri(&doomed), + }, + { + // Recreates the path, so a path-based restore has a + // file to open and the fallback becomes observable. + "kind": "create", + "uri": file_uri(&doomed), + }, + ], + }), + ); + // `other` keeps the registry non-empty so the delete's kill is not + // refused for being the last buffer. + open_as(&state, "OTHER", &other); + open_as(&state, "B", &doomed); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!( + doomed.exists(), + "precondition for the bite: the batch recreated the path, so a \ + path-based restore CAN open it" + ); + assert!( + !buffer_is_valid(&state, "B"), + "the origin buffer was reconciled away by the delete" + ); + + let reopened: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&doomed) + ), + ); + assert!( + !reopened, + "the applier must restore NOTHING. A path-based restore loads the \ + recreated file into a fresh buffer, which is the editor silently \ + re-opening a file the user asked to delete" + ); + + let active_path: Option = eval( + &state, + "local b = pmacs.window.buffer(); return b and b:path() or nil", + ); + assert_ne!( + active_path.as_deref(), + Some(doomed.to_str().unwrap()), + "and the user must not be sitting in it either" + ); + let active_valid = { + let core = state.core.borrow(); + let id = core.active_buffer_id(); + core.registry.borrow().contains(id) + }; + assert!( + active_valid, + "the window it left behind must still sit on a live buffer" + ); +} + +/// Review round 1 — a reconciliation failure inside the LSP subscriber +/// must be **reported and attributed**, and must not stop the remaining +/// attachments from reconciling. +/// +/// The scenario is the reviewer's: a **stale server id**. The attachment +/// record still names a server the manager has forgotten, so +/// `did_close` and `forget_uri` both raise. With ignored `pcall`s the +/// callback returned successfully, so the `all-must-succeed` hook logger +/// had nothing to log, and the old stores, routes and `documents` entry +/// stayed live under a URI the editor no longer held — silently. +/// +/// Two packages under one parent directory give two servers, and only +/// one is staled out, so the row can assert both halves at once: the +/// failure is surfaced, **and** the healthy attachment still moves. +/// +/// Bite: fails against ignored `pcall`s (nothing on either channel), and +/// against a fix that lets the first failure `error()` out of the loop +/// (the healthy attachment would never reconcile). +#[test] +fn a_subscriber_reconciliation_failure_is_reported_and_the_rest_still_reconcile() { + let fx = Fixture::new(); + fx.write("w/a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("w/b/Cargo.toml", "[package]\nname = \"b\"\n"); + let file_a = fx.write("w/a/src/main.rs", "fn main() {}\n"); + let file_b = fx.write("w/b/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "A", &file_a); + settle_until(&mut state, "server for package a", |s| server_count(s) == 1); + open_as(&state, "B", &file_b); + settle_until(&mut state, "server for package b", |s| server_count(s) == 2); + + // Stale out the server serving package `a` only: stop it, then forget + // it, leaving `attachments[a].server` naming a server the manager no + // longer holds. `forget_uri` raises for an unknown server id, which + // is exactly the failure mode under test. + let root_a = file_uri(&fx.at("w/a")); + exec( + &state, + &format!( + "local victim + for _, row in ipairs(pmacs.lsp.list()) do + if row.root_uri == \"{root_a}\" then victim = row.id end + end + assert(victim, 'no server rooted at package a') + pcall(pmacs.lsp.stop, victim) + _G.VICTIM = victim" + ), + ); + settle_until(&mut state, "the victim is forgotten", |s| { + let gone: bool = eval( + s, + "pcall(pmacs.lsp.forget, _G.VICTIM) + for _, row in ipairs(pmacs.lsp.list()) do + if row.id == _G.VICTIM then return false end + end + return true", + ); + gone + }); + exec(&state, "pmacs.editor.set_status('')"); + + // Rename the parent, so BOTH attachments are in the fan-out. + let new_uri_b = file_uri(&fx.at("w2/b/src/main.rs")); + rename_fire_and_forget(&mut state, &fx.at("w"), &fx.at("w2")); + settle_a_while(&mut state); + + // Half one: the failure is surfaced, on both channels, attributed to + // the operation that failed. + let said = status(&state); + assert!( + said.contains("resource.renamed") && said.contains("reconciliation failure"), + "the failure must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("forget_uri"), + "and must name WHICH step failed — an unattributed count does not \ + tell anyone that the URI-keyed stores were left live; status was \ + {said:?}" + ); + + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("resource.renamed") && errors.contains("forget_uri"), + "the callback must RAISE, so the all-must-succeed hook logger has \ + something to record; *errors* held {errors:?}" + ); + + // Half two: the healthy attachment still reconciled. The fake + // republishes diagnostics on every `didOpen`, so diagnostics under + // the NEW uri prove the whole ordered teardown ran for package b + // after package a's failed. + settle_until(&mut state, "package b reattached at its new uri", |s| { + diag_count(s, &new_uri_b) > 0 + }); + assert!( + diag_count(&state, &new_uri_b) > 0, + "one unreachable server must not leave every other attachment \ + unreconciled — the raise has to come after the loop, not inside it" + ); +} diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index ed3f6d7..aeba1ce 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -992,3 +992,311 @@ 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"); +} + +/// 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 +/// `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`. +/// +/// 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_clamps_or_clears_the_selection() { + 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 + .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, + "a cursor clamp that collapses the region clears the selection" + ); + assert!( + !core.clipboard_copy(), + "there is no collapsed region to copy" + ); +} + +/// 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 +/// 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. +/// +/// 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_clamps_or_clears_the_selection() { + 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 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, + "an anchor clamp that collapses the region clears the selection" + ); + assert!( + !core.clipboard_copy(), + "the collapsed region is not retained as active-but-empty" + ); +}