diff --git a/docs/active-work.md b/docs/active-work.md index dc8852d..8564532 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -606,57 +606,340 @@ has **no branch and no framing yet**. warns against quoting a stale figure; it does not replace that section's per-target census, which was not re-derived. -## dired Stage 2 framing lane — PR #171 OPEN, STALE, DO NOT MERGE AS-IS +## Resource-op delete guard lane — PR #186 OPEN, PROPOSED, DO NOT MERGE -- Portable branch: `githubsucks/dired-stage2-framing` (head `ab42a79`, - four framing commits); worktree `../pmacs-dired-stage1`. **PR #171**, - base `main`. Framing only — `docs/dired-stage2-framing.md`, 1,570 - lines, no runtime code. -- **Measured 2026-07-28: 4 commits ahead of `main`, 153 behind**, merge - base `c8ec8f3`. GitHub reports it mergeable, and its old CI run is - green — **both facts are about a tree nobody has looked at in 153 - commits**, and the document still says PROPOSED. -- **The commit history embodies three review rounds. That is not the - same as approval**, and GitHub records no formal review or comment on - it. Do not read the round count as a green light. -- **Its dependencies moved materially underneath it**, which is the real - reason not to merge. Note that dired Stage 1 (#165) and find-file - (#162) are its *base*, not new arrivals — the merge base `c8ec8f3` - **is** #165's merge commit. Eighteen PRs landed after it, and at least - three change ground the framing stands on: - - **#178 gave generated buffers a write invariant** - (`Buffer::set_generated_contents`). Dired's listing is a generated - buffer, and dired is named in the handoff as one of the writer - mechanisms that has **not** adopted it. Stage 2's marks and - operations write that buffer constantly. - - **#182 (Journey Stage 1a) made `resolve_target_buffer` the single - directory-open path**, with dired demoted to a *replaceable slot* - (`pmacs.path.directory_handler`) rather than a hook subscriber. Any - Stage 2 claim about how a directory reaches dired is now describing - a mechanism that no longer exists in that form. - - **#179/#181 landed the typed-edit consumer chain**, which is the - fan-out a rename transaction has to survive. +- Portable branch: `githubsucks/resource-op-delete-guard`; worktree + `../pmacs-resource-op-delete`. **PR #186**, base `main`. Currently + framing only — `docs/resource-op-delete-guard-framing.md`, **revision + 5** — plus this lane entry. No runtime code yet. +- **Measured 2026-07-28, `main` @ `7586905`:** - Re-scout against `6bee09d`, publish a new revision, and get explicit - framing approval before any implementation. **The re-scout is under - way** on the existing branch, so PR #171 keeps its three-round - history; the product is a revision 5, not a new document. (`main` has - since advanced to `0442d78`, but the only difference is the test-only - #174, so no re-scout conclusion turns on it.) -- **The rename problem the framing must still answer**, restated because - it is the hard part: a rename is a transaction across **five** path - owners — the buffer path, the buffer name, the URI-keyed LSP stores - plus `DiagnosticView` (whose URI is set once at construction), dired's - pathless handles, and a captured Lua local that no transaction can - reach. + ``` + $ git rev-list --left-right --count HEAD...githubsucks/main + 5 0 + ``` + + Five commits ahead, **0 behind** at the pushed revision-5 head. This + count includes the revision commit itself; revision 4 recorded the + pre-commit count and was therefore one short. +- **This PR becomes the implementation PR.** Revision 2 dropped rev 1's + framing-PR-then-implementation-PR plan as a one-feature/one-branch/ + one-PR violation. The framing is revised in place; implementation + commits land on this same branch **only after explicit user + approval**. +- **Live data-loss bug, reproduced four ways against `ad41cf1`.** + `pmacs.buffer.apply_resource_op`'s delete arm removes the path from + disk and *then* drops any buffer bound to it, with no dirty check at + any link — not the arm, not `remove_buffer_and_fire`, and not + `BufferRegistry::remove`, whose only guard is `editing_in_progress`. + Reachable through any language server's `WorkspaceEdit`. The four + modes: (a) the plain case returns `Ok(())` with file and buffer both + gone; (b) `ignore_if_not_exists = true` does **zero** filesystem work + and still destroys the buffer; (c) `recursive = true` reconciles + **nothing**, so a whole tree leaves orphaned buffers — the most + destructive arm does the least reconciliation, and it bypasses any + exact-path guard; (d) removal is not `kill_buffer`, so windows are + left bound to a removed `BufferId` and the registry can be driven to + **empty**. +- **Approved in principle after review round 1; revision 5 closes round + 4's two contract P1s and the ledger-ownership P1. Still PROPOSED, + still not approved for implementation.** Settled: refuse + unconditionally; take the delete side now. Withdrawn: rev 1's + buffer-first ordering. The design is `stat/no-op/refuse → enumerate + and validate → mutate filesystem → reconcile`, which keeps + `on_removed`'s "path already gone" invariant and makes a failed + deletion leave buffers intact automatically. +- **The stable cross-lane ownership split with #171:** + + > #186 owns the urgent **pre-filesystem refusal** for synchronous + > `apply_resource_op`. #171 later owns **full post-delete lifecycle + > reconciliation**, including the **async race where a buffer becomes + > modified after dired dispatch**. + + #186 additionally **owns the shared walk query** (scan every + path-bound buffer, normalize once, component-aware `Path::starts_with`) + under the boundary's "whichever lands first owns the query"; #171 + adopts it and extends it to `reconcile_rename`. **Neither lane guards + `pmacs.fs.remove`** — zero production callers today, named out of + scope by both. +- **#171 owns its own lane entry.** Revision 4 rewrote that sibling + block and was stale before push when #171 revision 8 landed 67 seconds + earlier. Revision 5 restores the block to `main`'s tree, so #186's + diff no longer changes it. The one fact this lane depends on is the + policy split above, which is stable through #171's pushed revision 8 + and independent of its commit count. +- **Standing rule this lane learned the expensive way.** A census is a + reading, not a constant. **Do not write an ahead/behind count, a line + count, or a call-site count into this file that you have not just + produced with a command whose output you can paste.** #186 shipped a + stale line count, then a stale commit count, then a stale ledger + citation, in three consecutive revisions — each time by carrying a + measurement across a base change instead of re-running it. The + specific trap: a count taken against `ad41cf1` was reported in + present tense after `main` had moved to `7586905`, which silently + converted "0 behind" into a falsehood. +- **Four facts a re-scout should not have to rediscover**, all verified + at `ad41cf1`: + - **No caller reliably surfaces a raise.** The server pump runs under + `pcall(handle_server_requests)` (`builtin/runtime/lsp.lua:1892`), so + a raise unwinds past the `send_response` and the server is never + answered; and the two user-initiated paths route uncaught coroutine + errors through `pmacs.error`, which is **undefined** (11 call sites + in `builtin/`, zero definitions). Refusals must travel as values. + - **A partial batch is already the status quo** — verified in-repo: + two delete ops, the second raises, the first stayed applied. Any + framing claiming batch atomicity here is wrong. **Do not justify + this from the LSP spec.** Revision 2 of #186 wrote that LSP 3.18 + "assigns `FailureHandlingKind.Abort` to resource-op-bearing edits"; + **it does not** — recovery is described by the client's advertised + `workspace.workspaceEdit.failureHandling`, `Abort` is one of four + strategies, only `TextOnlyTransactional` degrades to abort for + resource changes, and **pmacs advertises none of them**. The + justification is repository evidence plus the judgement that a + visible partial refactor beats unrecoverable unsaved work. + - **`find_by_path` is singular and duplicates are reachable.** + `BufferRegistry::find_by_path` returns the first match in insertion + order, `EditorCore::find_buffer_for_path` inherits that, and + `pmacs.buffer.from_file` creates path-bound buffers with **no + dedup** — so a clean first match can hide a modified second. The + guard needs a full scan with component-aware `Path::starts_with`. + - **pmacs advertises no `workspace.workspaceEdit` capability at all** — + `"applyEdit": true` but no `documentChanges`, no + `resourceOperations`, no `failureHandling`; `grep -rn + failureHandling` returns 0. Parked, not fixed here. +- **Ownership claim, concretely.** For this lane's duration #186 owns: + the pre-filesystem refusal inside synchronous `apply_resource_op`; the + **shared walk query**; and `builtin/runtime/lsp.lua`'s + `apply_workspace_edit` plus the `workspace/applyEdit` server-request + boundary. It does **not** own: full post-delete lifecycle + reconciliation, the dired async race between dispatch and + `remove_blocking`, the rename side of the walk, or `pmacs.fs.remove`. + Do not run the two lanes concurrently over `builtin/runtime/lsp.lua` + without re-splitting that claim. +- **Two residues #186 deliberately leaves for #171**, both named rather + than silent: after a successful *clean* recursive delete, descendant + buffers stay orphaned-and-clean (widening removal would promote the + dangling-window/empty-registry defect from exact-path to tree-wide); + and after a successful delete with several clean duplicates on one + path, only the first is reconciled. #186 validates **every** match but + reconciles **one**, which is today's behaviour preserved on purpose. +- Files the implementation will touch: `src/lua_bindings/mod.rs`, + `builtin/runtime/lsp.lua`, `tests/m4_acceptance.rs`, + `tests/lsp_dispatch_seams_acceptance.rs`, + `src/bin/pmacs_fake_lsp.rs`. **Not** `src/daemon.rs`, + `pmacs-protocol/`, `builtin/runtime/dired.lua`, + `docs/agent-handoff.md` or `COHERENCE.md`. No protocol change. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-resource-op-delete + -b resource-op-delete-guard githubsucks/resource-op-delete-guard`. + +## dired Stage 2 framing lane — PR #171 AT REVISION 9, AWAITING APPROVAL + +- Portable branch: `githubsucks/dired-stage2-framing`; + worktree `../pmacs-dired-stage1`. **PR #171**, base `main`, integrated + up to canonical `main` @ `7586905`. Framing only — + `docs/dired-stage2-framing.md`, **3,844 lines measured at this + revision**, no runtime code. This lane rides that PR — it has since + rev 6, when #185 merged; do not open a standalone ledger PR. +- **Status: PROPOSED, never approved.** Revision 5 was reviewed and NOT + approved (six findings, four P1); revision 6 answered them; **revision + 7 resolved a cross-lane conflict with PR #186**; revision 8 was + reviewed and NOT approved (six findings, four P1); revision 9 answers + them. The framing is at revision 9 after seven review rounds; **that + is not the same as approval**, and it must not be read as one. +- **Round 8 (rev 9) made every rev-8 correction singular and + executable:** + - Q#DR26 no longer retains the withdrawn execution-order claim; it + owns only the homogeneous enum/vector shape and delegates sequence + semantics to Q#DR29: **bus arrival, not filesystem execution**. + - 2a owns acceptance **23–38 and 50–55, including every suffixed + item**, so rev 8's new tests cannot fall between implementation PRs. + - The LSP gate is one manager-owned, generation-scoped + `HashSet<(LspServerId, String)>`. Both + `absorb_publish_diagnostics` and the now-server-aware + `mark_document_stale` check the exact pair; `did_open` clears one + pair, and server teardown clears all and only that server's pairs. + It is reclaimed, not falsely called size-bounded. Acceptance covers + both writers and both reclamation paths. + - Buffer-name following now uses explicit + `BufferNameOrigin::{Explicit, PathDerived}` state. The acceptance + collision is a file `${cwd}/notes` explicitly named `notes`, which + defeats rev 8's path-equivalence heuristic. +- **Round 7 (rev 8) found five contract defects of ONE family, and the + family is the transferable lesson: a guarantee assumed from a + mechanism whose implementation was never read.** Reply order assumed + to be execution order; a refusal assumed to be inert; `forget_uri` + assumed to cover every writer. Three of the five were literally that. + The decisions: + - **Reply order is NOT execution order.** `AsyncRuntime::tick` + (`src/async_runtime.rs:1003`) is `while let Ok(env) = + self.main.try_recv()` — a bus drain with **no execution token**, so + a worker can finish, be descheduled before sending, and have a later + reply arrive first. The ordering guarantee was **withdrawn, not + engineered**: reconciliation is order-independent, and `fs.lua:155-165` + already recommends serialization. Rev 9 makes the public contract + exact: overlapping mutations dispatch serially, awaiting one before + the next. A lock was rejected — it would serialize every fs mutation + for a hazard absent from shipped in-tree callers. + - **`EditorCore::kill_buffer` damages editor state BEFORE it can + refuse.** It clears `round_trip_buffers`, closes side windows and + redirects ordinary windows, and only then can + `BufferRegistry::remove` return `ConcurrentEdit`. So "refused" is + not "nothing happened". Fixed by preflighting `editing_in_progress` + — sound because phase 1 is pure `EditorCore` and holds no Lua + handle, so nothing can re-enter between check and removal. + - **Buffer names are set from `path.display()` AS GIVEN** while only + `file_path` is normalized (`src/editor_core.rs:917`, + `src/lua_bindings/mod.rs:3112`), so a relative open leaves a short + name that a string-equality rule mistakes for user-chosen. Rev 8 + moved to path-equivalence; rev 9 replaces that inference with + explicit provenance because a custom name can equal the path. + - **`diag_store` has ZERO correlated writers**, and + `DiagnosticStore.by_uri` is keyed by **URI alone** with no server + component (`src/diag.rs:198`) — so the store most needing protection + is the one a `pending_routes` purge cannot help. Rev 8 incorrectly + inferred that the tombstone must share the store's URI-only key; + rev 9 gates in `LspManager`, where `sid` is still available. + - **A first pass of rev 8 checked only `handle_notification` and + concluded `publishDiagnostics` was the only uncorrelated writer. + Wrong lens boundary** — `pub fn mark_document_stale` + (`src/lsp.rs:3108`) takes **no `LspServerId`** and creates URI keys + across three stores for every server. Rev 9 changes that surface and + its private Lua binding to `(sid, uri)` and pins it independently. +- **Both new hooks are `all-must-succeed`, not short-circuit** — a gap + the review did not raise and the sweep found. `src/hook.rs` defines + three `HookKind`s; registered short-circuit, one `resource.renamed` + subscriber returning falsey would silently stop every later + subscriber reconciling. +- **CROSS-LANE SPLIT WITH PR #186 — settled, recorded verbatim so the + two lanes cannot diverge again:** + + > #186 owns the urgent **pre-filesystem refusal** for synchronous + > `apply_resource_op`. #171 later owns **full post-delete lifecycle + > reconciliation**, including the **async race where a buffer becomes + > modified after dired dispatch**. #171's revision 7 adopts the + > refusal and stops saying LSP intentionally deletes modified files. + + **PR #186 (`resource-op-delete-guard`) owns the pre-filesystem + refusal.** Do not implement a competing dirty check from #171. Rev 7 + adopts #186's Q#RD1 (refuse, do not prompt, do not save, do not back + up) and its Q#RD6 shared query (scan every path-bound buffer, not + first-match; normalize once; component-aware `Path::starts_with`); + whichever lane lands first owns that query and the other adopts it. + The two fit deliberately: #186's Q#RD5 keeps reconciliation + **exact-path** so it does not promote the dangling-window and + last-buffer defects tree-wide, and #171's Q#DR27 is the lane that + makes widening safe. +- **What rev 7 removed.** Rev 6 said an LSP-authored delete removes the + file and orphans the modified buffer as accepted residue. That is + withdrawn — **20 matched lines across 13 passages** were censused and + reclassified: 4 ownership claims reassigned to #186, 4 policy claims + deleted, 3 ground-truth statements kept but attributed, 2 async-race + claims kept and narrowed. +- **What rev 7 kept, and it is now one path not two.** The async race — + a buffer modified between dired's pre-dispatch check and + `remove_blocking`'s syscall — survives #186's refusal, because **dired + never goes through `apply_resource_op`**: it calls `pmacs.fs.remove`, + which dispatches a worker no in-applier guard can see. Rev 6 carried + this as "two paths, LSP and dired"; after #186 the LSP path has no + residue at all. +- **A gap neither lane closes, named in rev 7 §11:** `pmacs.fs.remove` + is public Lua API with **no dirty check of its own**. After both lanes + land the guards sit one layer above it on each side — at + `apply_resource_op` (#186) and in dired's policy layer (#171) — so a + third caller inherits neither. Latent, not live: it has zero + production callers today. +- **LSP failure-handling, verified against the 3.18 spec** because #186 + got it wrong and this lane was checked for the same error. **This lane + never made the claim** — it asserts nothing about `WorkspaceEdit` + recovery. For the record: only `textOnlyTransactional` degrades to + abort for resource operations; `transactional` covers them; and the + specification states **no default** for a client that does not + advertise `failureHandling` (pmacs advertises none). So "Abort applies + by omission" does not follow. +- **Round 5's single theme, worth carrying because it will recur:** rev + 5 changed the slice split and the ownership of a decision, and the + prose did not follow. Four of the six findings were that same defect + in different places — stale slice labels, an acceptance allocation + that contradicted the code split, and a PR body still describing + revision *1*'s two-slice plan. **When a cut changes, sweep for the + label, do not patch the instance.** Rev 6's own sweep found four more + the review had not cited, one of which was wrong in both halves + (`x`'s report tagged 2a, `remove_dir_all` tagged 2b). +- **Rev 4 was scouted at `c8ec8f3`, which is dired Stage 1's own merge + commit (#165)** — so dired Stage 1 and find-file (#162) were always + its base, not new arrivals. Eighteen PRs landed on top before the + re-scout. Rev 5 is that re-scout, against `6bee09d` and then + integrated forward. +- **Seven of rev 4's own claims about pmacs were wrong**, which is the + lane's most transferable lesson: a framing can verify all its external + facts and still be wrong about the codebase it is for. The load-bearing + one: §5 named `drain_external_cancelled` but cited `lsp.rs:1596`. + **Those are two different functions 35 lines apart** — + `drain_external_cancelled` (`:1561`) is the unconditional + server-scoped drain and is the right precedent; + `drain_cancelled_externals` (`:1596`) is a per-tick token/timeout + sweep. A rename flips no token, so following the line number yields a + `forget_uri` whose drain half is a silent no-op and the awaiting + coroutine hangs forever — exactly the failure that step exists to + prevent. +- **The rename path-owner census is SIX, not five.** The sixth is + `lean.lua`'s `M.file_progress`, a URI-keyed Lua module table living in + no Rust store, so `forget_uri` structurally cannot reach it. That is + the first case outside dired proving the **hook**, not the Rust + method, is the mechanism that scales. The other five: buffer path, + buffer name, the URI-keyed LSP stores plus `DiagnosticView` (URI set + once at construction), dired's pathless handles, and a captured Lua + local no transaction can reach. +- **The journey ratchet is split across two files**, and both are gates: + `tests/journey_acceptance.rs` (24 tests, "stages add rows, none + removes them", seven rows asserting on dired) and + `gpu_invocation_acceptance.rs`, where #183 actually put the GPU row. + A scout that checks only the first will think #183 added nothing. +- **`open_directory` now commits under `pmacs.window.commit_to`, whose + scope REFUSES an `await`** (#182). That directly constrains the + serialize-and-await batch contract the framing proposes. +- **The typed-edit chain does not reach dired** — verified rather than + assumed. Its lessons bind the framing's two *new* hooks instead. +- **Scope MOVED OUT of this lane, and rev 6 records the handoff.** + Rev 5 added Q#DR25 to adopt `set_generated_contents` at the head of + 2b, because dired's listing is a generated buffer whose paint bypasses + an intercept over a still writable rope. That turned out to be a + **class bug, not dired's** — the same idiom is in listview, compile + and search/grep, and **no Lua caller anywhere sets `read_only`** + because there is no Lua `set_read_only` to call — so it is owned by + the generated-buffer immutability lane. **Done: rev 6 withdraws + Q#DR25**, and §3.1 hands that lane what this one's re-scout found, + including the trap that + `dired_buffer_is_read_only_and_round_trips_input` passes **either + way** and is therefore not coverage of the adoption. Do not implement + Q#DR25 from this document. +- **Two substrate facts rev 6 added that outlive this lane.** (1) Buffer + removal is **two phases** and **no existing Rust path composes them**: + `EditorCore::kill_buffer` (`src/editor_core.rs:4590`) does windows and + registry, `after_buffer_removed` (`src/lua_bindings/mod.rs:1602`) does + keymaps/config/folds/`on_removed`, and only the Lua binding + `pmacs.buffer.kill` (`mod.rs:5476-5491`) runs both — while + `apply_resource_op`'s delete uses `remove_buffer_and_fire`, which + skips phase 1, **so a window displaying the deleted buffer keeps a + removed id**. That is a third defect on that arm beside the missing + dirty check and the first-match lookup. (2) The drain outcome must + carry deletions as well as renames in one `Vec` so every + kind is expressible, but its sequence is **bus-arrival order, not + filesystem execution order**; Q#DR29 requires callers with + interdependent mutations to serialize. - Intended serial implementation once approved: **2a** rename/delete reconciliation substrate with no dired UI, **2b** marks and operations, **2c** mkdir/copy/recursive-delete primitives, then Stage - 3 wdired. + 3 wdired. The re-scout re-examined this cut and it holds unchanged. - **Ownership warning:** dired 2a overlaps `src/editor_core.rs`, `builtin/runtime/lsp.lua`, and the URI-keyed LSP state with other coherence work. Do not run it concurrently with Journey Stage 1b without assigning those files to one lane first. +- **Two live bugs on `main` this lane confirmed but does NOT fix**, both + now owned elsewhere: `apply_resource_op`'s delete arm destroys unsaved + work (no dirty check anywhere on the path), and `View` still lacks + `rename_resource`. ## Parked lane: kill-ring browser + persistence diff --git a/docs/dired-stage2-framing.md b/docs/dired-stage2-framing.md new file mode 100644 index 0000000..5f8287e --- /dev/null +++ b/docs/dired-stage2-framing.md @@ -0,0 +1,3844 @@ +# Dired Stage 2 — marks and operations — framing + +**Revision 9 — 2026-07-28. Status: PROPOSED — NOT APPROVED. This +document has never received a formal framing approval, and it needs one +from the user before any implementation branch is cut.** Its commits +embody seven rounds of findings; that is not the same as approval. +Revision 7 resolved the cross-lane split with PR #186 — accepted, and +not reopened here. Revision 8 answered review round 7's five contract +defects and two documentation fixes. **Revision 9 answers review round +8: four blocking contract/acceptance defects and two accuracy defects.** +§0's round-8 section says what each decided. + +**Ground truth: re-scouted 2026-07-28 against canonical `main` @ +`6bee09d`** (`Merge pull request #184 from levineuwirth/bottom-panel-stage2b`). +Rev 4 was scouted at `c8ec8f3` — which is dired Stage 1's *own* merge +commit — and `main` had moved **153 commits** since. Rev 5 is that +re-scout: §0's round-4 section states exactly what moved, what it +invalidated, and what survived unchanged. + +**`main` then moved again mid-re-scout, to `0442d78`** (#174, +`fix-m4-sink-races`), and this branch was merged up to it. #174 touches +`tests/m4_acceptance.rs` only, changes no source, and intersects this +document at exactly one point — a §11 deferral, which it **confirms** +rather than invalidates. Every scouting claim below therefore holds at +`0442d78` as well as at `6bee09d`; the `6bee09d` anchor is kept as the +one the census was actually read against. + +**The branch is now integrated through canonical `main` at `7586905`.** +The intervening first-parent changes are #185 and #189, both +documentation-only ledger/coherence updates; neither changes the +runtime substrate scouted here. The current PR base and merge parent are +therefore `7586905`, while `6bee09d` remains the explicit code-census +anchor. + +Continues `docs/dired-framing.md` (rev 7, approved; Stage 0 merged as +#162, Stage 1 as #165). That document's §6 and §7 carry the *approved* +shape of marks and operations; this one re-verifies every claim in them +against the tree, corrects what has drifted or was wrong, and adds what a +Stage 2 implementation needs and the parent did not decide: the +batch-execution contract, the confirmation surface, the staging cut, and +acceptance. Decisions continue the parent's scheme from **Q#DR12**. + +The parent framing's ground truth (§2) was scouted at `main` @ `e745068` +and re-verified at `0827dd1`. Stage 1 then changed three of the files +Stage 2 depends on most (`src/fs.rs`, `src/async_runtime.rs`, +`src/lua_bindings/mod.rs`), and #178/#179/#181/#182/#183/#184 have since +changed more, so **§2 below supersedes the parent's line references for +everything Stage 2 touches.** Line numbers throughout are given for +navigation only; every claim is anchored to a **symbol**, because line +numbers drift and this document has now watched them drift twice. + +--- + +## 0. Revision history + +### Review round 8 (rev 8 → rev 9) — make the revised contracts singular and executable + +Round 8 accepted the substance of rev 8's mid-edit preflight, +three-outcome confirmation, Lua binding, and hook-kind decisions. It +found six places where the document had not carried its new decisions +through to one implementable contract. + +#### F1 — the withdrawn ordering guarantee remained authoritative → **supersede it everywhere** + +Verified: Q#DR26 still called `TickOutcome.resources` an ordered +execution sequence, the proposed Rust doc still said "settle order", +and `docs/active-work.md` repeated the same claim, while Q#DR29 and §6 +said the opposite. Rev 9 rewrites Q#DR26 to retain only the decisions +that survived — one homogeneous `Vec` and one +`Option` — and explicitly delegates sequence semantics to +Q#DR29. The proposed Rust doc and volatile ledger now both say +**bus-arrival order, not filesystem execution order**. +Because the async primitives are public, rev 9 also corrects "no +production path" to "no shipped in-tree path" and assigns 2a an explicit +`fs.lua` contract requiring overlapping mutations to serialize. + +#### F2 — rev 8's acceptance additions were not assigned to 2a → **the cut owns 23–38 and 50–55** + +Verified: §13 added 31b, 31c, 53b, 54 and 55, but §10 and the PR body +still assigned 2a only `23–38, 50–53`. The numeric ranges include the +31 suffixes, but not 54–55, and leave 53b needlessly ambiguous. Rev 9 +assigns **23–38 and 50–55, including every suffixed item**, to 2a. The +PR body carries the same allocation. + +#### F3 — the tombstone had two keys and no implementable teardown → **one manager-owned `(sid, uri)` set** + +Verified: rev 8 called the tombstone both "per-server `(sid, uri)`" and +"URI-keyed to match `DiagnosticStore`". The latter inference was wrong: +the gate sits in `LspManager::absorb_publish_diagnostics`, which +receives `sid`, **before** the URI-only store write. A URI-only +tombstone also cannot clear one server's entries at teardown without +retaining the server provenance it discarded. + +Rev 9 chooses one state shape: + +```rust +forgotten_documents: HashSet<(LspServerId, String)> +``` + +`forget_uri(sid, uri)` inserts the exact pair. +`absorb_publish_diagnostics(sid, …)` checks that pair before touching +`DiagnosticStore`. `mark_document_stale` becomes +`mark_document_stale(sid, uri)`, and the private Lua binding becomes +`pmacs.lsp._mark_document_stale(server_id, uri)`, so the second +state-creating writer checks the same pair instead of requiring a +global URI projection. `did_open(sid, uri)` removes only that pair; +`start_generation(sid, …)` and `forget(sid)` retain entries for every +other server and remove all entries for `sid`. + +The set is **generation-scoped and reclaimed, not size-bounded**. A live +server can accumulate distinct forgotten URIs until reopen or teardown; +rev 8's word "bounded" promised a cap that does not exist. An arbitrary +capacity/LRU eviction would reopen the late-notification race, so rev 9 +states the honest lifetime instead. + +Acceptance now covers both state-creating writers, exact-pair reopen, +and server-selective teardown. The fifteen synchronous Lua `clear` +bindings are not late arrivals and cannot repopulate result data; the +diagnostic forget primitive itself removes `epochs` rather than calling +the existing `clear`, which increments it. + +#### F4 — `mark_document_stale` had no bite → **test the writer and both reclamation paths** + +Item 31b now calls both late paths: `publishDiagnostics(sid, old_uri)` +must not recreate diagnostics, and +`_mark_document_stale(sid, old_uri)` must not recreate any of the three +stale flags. New item 31d proves that `did_open` clears only the exact +pair and that each teardown clears all and only that server's pairs. +The binding error/idempotence contract remains item 31c. + +#### F5 — path equivalence is not provenance → **record provenance** + +Verified with the review's collision: for a file `${cwd}/notes`, a user +who explicitly chooses the buffer name `notes` produces the same +normalized path as the file. Rev 8 would overwrite it while claiming +custom names survive. + +Rev 9 adds explicit private buffer state: +`BufferNameOrigin::{Explicit, PathDerived}`. Generic constructors and +`set_name` produce `Explicit`; every audited file-load/save-as site uses +a dedicated path-derived setter; reconciliation updates only +`PathDerived` names and keeps that origin for later renames. +`pmacs.buffer.set_name` is explicitly user naming and therefore sets +`Explicit`. Acceptance uses the collision case itself, so replacing +provenance with the rev-8 normalization heuristic fails. + +#### F6 — the volatile ledger was not volatile enough + +`docs/active-work.md` now records revision 9, seven review rounds, and +integration through canonical `main` at `7586905`; its old +`ad41cf1` anchor and rev-6 ordering residue are removed. The measured +line count is refreshed after this revision's final edit. + +### Review round 7 (rev 7 → rev 8) — five contract defects, one family + +The split with #186 was accepted and **none of it is reopened**. What +round 7 found instead is five places where this framing **asserted a +guarantee it had derived from a mechanism whose implementation it never +read**. Findings 1, 2 and 4 are literally that: reply order assumed to +be execution order, a refusal assumed to be inert, `forget_uri` assumed +to cover every writer. Findings 3 and 5 are the same shape one level +out — a naming rule assumed from how *some* buffers are named, and a +callback contract assumed from a helper signature that cannot express +it. + +Every citation was re-verified here before being acted on. **All seven +hold.** One is understated (F2); nothing in the review was wrong. + +#### F1 — "settle order" is not filesystem mutation order → **the ordering guarantee is WITHDRAWN, not engineered** + +Verified: `AsyncRuntime::tick` (`src/async_runtime.rs:1003`) is +`while let Ok(env) = self.main.try_recv()` — it drains the bus and +records **arrival** order. Mutations run on separate pool workers, so a +worker can finish, be descheduled before sending, and have a later +mutation's reply arrive first. `TickOutcome.resources` therefore cannot +carry execution order, and **rev 7's justification for a single ordered +vector was false.** + +**The cheaper question first, as directed: does reconciliation need to +be order-dependent?** Working it through: + +- For **independent** mutations — disjoint paths, disjoint subtrees — + reconciliation commutes. Order is irrelevant. +- For **interdependent** ones it genuinely is not, and *no static rule + rescues it*. Take rename `dir`→`newdir` racing delete `dir/child.txt`. + If the delete ran first on disk, correct reconciliation is delete-then- + rename. If the rename ran first (so the delete was really of + `newdir/child.txt`), correct reconciliation is rename-then-delete. A + fixed rule such as "deletes before renames" gets the first case right + and the second wrong: the kill misses, the rename then rebinds the + buffer onto a path whose file is gone, and it survives pointing at + nothing. **So it is a real execution-order token or nothing** — this + was worked out rather than assumed, because the review asked which. +- **But no production path can produce the interdependent case.** dired + **serializes** — one coroutine per batch, awaiting each op before + dispatching the next (§9). `apply_resource_op` is **synchronous on the + main thread** and never enters the drain at all. And §2 verified that + `pmacs.fs.rename` and `pmacs.fs.remove` have **zero production + callers** besides tests. The hazard requires a third-party package to + fire-and-forget two interdependent mutations. +- **And the substrate already tells it not to.** `fs.lua:155-165` states + the contract: *"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)."* §9 already cites this as the reason + dired serializes. + +**Decision: the reconciliation contract is order-independent by +construction, and promises nothing about two mutations in flight +simultaneously.** Each settled mutation reconciles on its own; the +framing states explicitly that `resources` is in **bus-arrival order, +which is not execution order**, and does not build on the sequence. +Where interdependence exists, the caller owes serialization, and the +primitive's own documentation already says so. + +**Why the token was rejected.** An execution-order token established +while holding a mutation lock would serialize every filesystem mutation +through one lock — undoing the concurrency the worker pool exists for — +to close a hazard with no production reachability and a documented +caller-side remedy. It is named as a deferral (§11) with its trigger +stated: **the first production caller that fire-and-forgets two +interdependent mutations.** That is a condition someone can check, not a +vague "if it becomes a problem". + +Consequences carried through: rev 7's one-vector-because-ordered +argument is replaced (§5); §6's "via the same ordered sequence" is +reworded; a new acceptance pins that two **independent** concurrent +mutations reconcile correctly **regardless of arrival order**, and §13 +states explicitly that there is **no** item for interdependent ordering, +with the reason — a test cannot pin an order the mechanism does not +establish. That is the same discipline item 20 already applies to H1's +open interval. + +#### F2 — a refused mid-edit kill is already destructive → **preflight, and the review understates it by one** + +Verified in `EditorCore::kill_buffer` (`src/editor_core.rs:4590`), in +execution order: the last-buffer and unknown-id guards return early +(safe); then `self.round_trip_buffers.remove(&buffer_id)`; then the +fallback is computed; then side windows showing the victim are +**closed** via `remove_side_window`; then every remaining window on the +victim is redirected, resetting `text_view`, `cursor`, `selection`, +`overlays`, `view_top`, `goal_col`; and **only then** does +`BufferRegistry::remove` (`src/buffer_registry.rs:127`) get the chance +to refuse with `ConcurrentEdit`. So "refused" is not "nothing happened". + +**Where rev 7 was worse than the review says:** rev 7 did not merely +mis-describe the refusal, it wrote *"the reconcile must treat a phase-1 +failure as 'keep the buffer' and not run phase 2"* — which reads as +though skipping phase 2 restores the buffer. It does not. Skipping +phase 2 only avoids firing `on_removed` for a buffer that still exists; +the window and round-trip damage is already done and nothing in the +described flow undoes it. + +**Decision: preflight `editing_in_progress` before phase 1.** Not a +transactional kill. Three reasons: + +- **The preflight is sound, not merely cheap.** Phase 1 is entirely + `EditorCore`, which holds **no Lua handle** (the layering fact C1 + established), so nothing between the check and + `BufferRegistry::remove` can re-enter Lua and start an edit. The + window that would make a preflight racy does not exist on this path. +- **It matches #186.** Its Q#RD2 already moves `editing_in_progress` + out of discovery and into validation, for the same reason — *"Today a + `ConcurrentEdit` refusal from `BufferRegistry::remove` arrives after + the file is gone."* Both lanes then check the same predicate in the + same phase. +- **A transactional kill would need a compensation path** that + snapshots and restores window layout, side-window collapse and + round-trip membership — and compensation can itself fail. A check that + makes the failure unreachable is strictly smaller than a rollback that + handles it. + +Acceptance 53 is split into **three separately-asserted properties with +individual bites** (§13). + +#### F3 — the name-provenance rule is false for relative opens → **path-equivalence, stated** *(superseded by rev 9 F5)* + +Verified: `get_or_load_buffer` (`src/editor_core.rs:917`) sets +`display_name = path.display().to_string()` — **the path as given** — +and normalizes only the stored `file_path` via `set_buffer_path`. +`pmacs.buffer.from_file` (`src/lua_bindings/mod.rs:3112`) does the same: +`create_from_bytes(path.clone(), …)` with the raw string. So opening +`foo.rs` relatively yields name `foo.rs` and path `/abs/dir/foo.rs`. +Rev 7's rule — update the name only when it **equals** the normalized +old path — leaves that buffer's name stale, and it is not user-renamed. + +**Rev-8 decision, superseded in rev 9: the rule is path-equivalence, not +string equality.** The +name is treated as path-derived — and therefore updated — **iff the +stored name, parsed as a path and normalized by +`normalize_buffer_path`, equals the buffer's stored normalized old +path.** In words: *the name still denotes this file.* + +- A full-path name (Stage 1's and dired's shape) qualifies. +- A relative name `foo.rs` qualifies, because it normalizes to the same + absolute path. +- A genuinely custom name — `notes`, `*scratch*`, anything the user + chose — does not, and is preserved. That direction is what stops the + rule over-correcting, and §13 tests it. + +**One consequence stated rather than hidden:** when the rule fires the +new name is written as the **normalized new path**, so a buffer opened +by a relative path acquires an absolute name after a rename. Preserving +the relative rendering would need a record of *what base the name was +relative to*, which no buffer carries. The effect is confined to the +statusline and buffer list. If that is judged wrong, the alternative is +an explicit provenance flag on `Buffer` — real state, every creation +site audited — and this framing does not think a corner of the +statusline earns that. + +#### F4 — late `publishDiagnostics` can repopulate the old URI + +Verified: the manager absorbs `textDocument/publishDiagnostics` +unconditionally (`src/lsp.rs:2878`); it is a server-initiated +notification with **no request id**, so `pending_routes` — which is what +`forget_uri`'s purge and awaiter drain operate on — cannot see it. A +notification queued before `didClose` can therefore arrive after the +store is cleared and recreate the entry under the old URI. + +This is the five-owner census in a new lens, and the review is right +that it is the same shape. **The re-run is reported in full in §5**, and +it changed the design four times over — most sharply: `diag_store` has +**zero** correlated writers, so the store `forget_uri` most needs to +protect is the one its route purge cannot help; and +`DiagnosticStore.by_uri` is keyed by **URI alone**, with no server +component, so a `(sid, uri)` tombstone would not match it. + +**Answering the question as asked: no, `publishDiagnostics` is not the +only one.** It is the only uncorrelated **notification** writer — 1 of +1, and `handle_notification` has no default arm — but +`pub fn mark_document_stale` (`src/lsp.rs:3108`) is a second +uncorrelated writer that takes **no `LspServerId`** and creates URI keys +across three stores for every server at once. **A first pass of this +revision checked only `handle_notification` and concluded it was the +only one. That was the wrong lens boundary, and it is this round's own +defect class occurring inside the round** — recorded rather than +quietly corrected, because "I checked the mechanism" and "I checked the +right mechanism" are different claims. + +**Rev-8 decision, superseded in rev 9 F3: a bounded per-server +tombstone, checked at absorb.** +`forget_uri(sid, uri)` records `(sid, uri)` as forgotten; an +uncorrelated absorb whose URI is tombstoned is **dropped**; the +tombstone is cleared when that URI is next `did_open`ed, and wholesale +at the existing server-teardown sites (`start_generation`, `forget`). + +**Why a tombstone and not the obvious membership gate.** The tempting +version is "absorb only if `(sid, uri)` is currently in `documents`", +which needs no new state at all. **It would be a behaviour regression:** +servers legitimately publish diagnostics for files the editor never +opened — a crate-wide push naming a dependency — and a membership gate +drops all of them. The tombstone drops **only what this editor +explicitly forgot**, which is the narrow claim that is actually true. +Residue stated: a server that legitimately publishes for the old path +*after* we renamed away from it is also dropped until the tombstone +clears. That is the conservative direction, and it is the same +information-loss trade `forget_uri` already makes for the correlated +stores. + +#### F5 — `confirm { prompt, on_yes }` cannot express decline → **three outcomes, because the substrate already has three** + +Verified: the copy contract says a declined overwrite **continues**, +copying the non-colliding entries, while the helper exposes only +`on_yes`. There is no negative continuation to hang that on. + +And the substrate is richer than the helper: `pmacs.minibuffer.read` +takes a required `on_accept` and an optional `on_cancel` +(`src/lua_bindings/mod.rs:13403-13417`), and the session carries both +(`src/minibuffer.rs:403-405`), with `cancel()` returning the +on-cancel callback (`:353`). So a prompt has **three** outcomes, not +two: accept-affirmative, accept-negative, and cancel — and rev 7's +helper collapsed all three into "call `on_yes` or do nothing". + +**Decision: `pmacs.minibuffer.confirm { prompt, on_yes, on_no, +on_cancel }`**, with decline and cancel **distinguishable**, because the +copy flow needs exactly that distinction and the substrate already +draws it: + +- **`on_yes`** — `y`/`yes`, case-insensitive. +- **`on_no`** — any other accepted text, **including an empty `RET`**. + The fail-closed default from Q#DR15 is unchanged; it now has somewhere + to go. +- **`on_cancel`** — `C-g`/Escape. **If omitted, cancelling does + nothing at all** — it does *not* fall through to `on_no`. + +That last point is the decision worth arguing with, so here is why. For +`x` and `D` the two are equivalent: declining and cancelling both mean +"delete nothing". For `C` they are not — declining means *copy the +non-colliding entries* (a partial action), cancelling means *abandon +the batch*. Defaulting cancel to `on_no` would make `C-g` perform a +partial copy, which is the one thing a user pressing `C-g` is trying to +avoid. So cancel is inert unless a caller explicitly opts in. + +Acceptance 39 is extended to exercise the negative path and the cancel +path separately — the current item cannot, because there was nothing to +observe. + +#### P2 fixes + +- **The Lua surface for `forget_uri` is now named.** Rev 7 told a Lua + subscriber to call a manager-level method with no binding behind it. + Lua reaches only explicit bindings; the nearest precedent is + `pmacs.lsp.forget` (`src/lua_bindings/mod.rs:10257`), a four-line + closure that calls through and maps the error with + `mlua::Error::external`. §5 now specifies `pmacs.lsp.forget_uri`, its + error contract, and a pin. +- **The ledger note is rewritten and its numbers re-measured.** It + claimed this branch touches only the framing file — false since rev 6, + which is when the lane moved onto this PR — and quoted a line count + from a previous revision. Counts in §16 and in the ledger are now + measured at this revision. + +#### The sweep for the same defect class + +Findings 1, 2 and 4 share one shape: **a guarantee assumed from a +mechanism whose implementation was not read.** Swept the rest of the +framing for it. Three candidates, honestly reported: + +- **`run_all_must_succeed` "collects errors and does not abort the + fan-out"** (§2). This had been carried from a project lesson rather + than read. **Read now** (`src/hook.rs:332-348`): it is a plain + `for cb in callbacks` loop pushing into `errors` with no early return, + and `proceed` is `errors.is_empty()`. **Claim holds.** +- **`pmacs.window.commit_to` refuses an `await`** (§2), cited to a test + by name. **Verified**: `commit_to_refuses_an_await_and_restores` + exists at `tests/journey_acceptance.rs:746`. **Claim holds.** +- **One genuine new finding, which the review did not raise: the + framing never says which `HookKind` the two new hooks register + with.** `src/hook.rs` defines **three** — `ShortCircuit`, + `AllMustSucceed`, `Accumulate` — and every declared hook picks one in + `builtin/hooks/default.lua` (`buffer.after-edit` is + `all-must-succeed`; `path.open-directory` and `editor.before-quit` + are `short-circuit`). Rev 7's B1b asserted only that the *mechanism* + exists. **That gap matters**: registered `short-circuit`, one + `resource.renamed` subscriber returning falsey would stop the fan-out + and silently prevent every later subscriber from reconciling — the + precise trap the typed-edit chain lesson names, arriving through a + door this framing left unlabelled. **Decision: both hooks are + `all-must-succeed`** (Q#DR28), so a failing subscriber is reported and + the rest still run, and no subscriber can claim the event. + +Nothing else in the framing rests on an unread mechanism. Recorded as a +count rather than a shrug: **three candidates, two cleared by reading, +one real.** + +--- + +### Round 6 (rev 6 → rev 7) — cross-lane reconciliation with PR #186 + +Not a review of this document. **PR #186** +(`docs/resource-op-delete-guard-framing.md`, branch +`resource-op-delete-guard`, based on the same `ad41cf1`) frames a guard +for `apply_resource_op`'s delete arm that **refuses before touching +disk** when any affected buffer is modified. Rev 6 said the opposite for +the same event: the file *is* deleted, the modified buffer survives +orphaned, and §11 named that orphaning as accepted residue. Two lanes, +two answers, same call site. + +**The settled split, recorded verbatim so the two lanes cannot diverge +again:** + +> #186 owns the urgent **pre-filesystem refusal** for synchronous +> `apply_resource_op`. #171 later owns **full post-delete lifecycle +> reconciliation**, including the **async race where a buffer becomes +> modified after dired dispatch**. #171's revision 7 adopts the refusal +> and stops saying LSP intentionally deletes modified files. + +Rev 7 adopts it. **This was not re-litigated**, and the changes below +are what adopting it costs this document. + +#### The census of orphaning claims, and its arithmetic + +Built by grep over the whole file, then classified by reading each site's +enclosing passage rather than the matched line. The pattern set was +`orphan`, `destroys/destroy/destroying unsaved`, `unsaved work`, +`keeps/keep modified`, `kept_modified`, `still deletes the file`, +`no longer destroys`, `accepted the refactor`. + +**20 matched lines, resolving to 13 distinct passages**, classified: + +| Class | Count | What it said | Rev 7 | +|---|---|---|---| +| **A — ownership** | **4** | that *this lane* closes the LSP-delete data-loss defect | **Reassigned to #186.** §1, §10, §13 item 23, §16 | +| **B — policy** | **4** | that an LSP-authored delete *intentionally* removes the file and orphans the buffer, as accepted residue | **Deleted.** §6's asymmetric split, §6's residue paragraph, §11's deferral, Q#DR18 | +| **C — ground truth** | **3** | that the defect exists on `main` today | **Kept — it is still true — but attributed to #186 as the fixer.** §0's round-4 list, §2, §6 | +| **D — the async race** | **2** | that a buffer modified after dispatch is orphaned | **Kept, and narrowed** — see below | + +Class C is deliberately not deleted: `main` at `ad41cf1` really does +destroy unsaved work on that arm, and a framing that stopped saying so +because another lane is fixing it would be describing a tree that does +not exist yet. What changed is the attribution, not the fact. + +#### What changed in Q#DR18 + +The decision is **rewritten, not withdrawn** — `reconcile_delete` +survives, and the coordinator's instruction is explicit that the shared +seam is not cancelled. Three changes: + +- **The synchronous path refuses before disk.** For `apply_resource_op`, + a modified buffer in the affected set means the delete **fails**, + changing nothing on disk and nothing in the registry (#186 Q#RD1). + Rev 6's "still deletes the file, because the user accepted the + refactor" is gone. +- **`reconcile_delete`'s modified arm is no longer *reached* on that + path** — not removed from the seam. `DeleteReconcile.kept_modified` + stays, because the **asynchronous** path still reaches it (Class D). + Saying "the field is unreachable" would be wrong; saying "the sync + caller can no longer produce it" is right. +- **The walk rule is adopted from #186, not reinvented.** #186's Q#RD6 + already specifies the shared query — scans **every** path-bound + buffer rather than first-match, normalizes once, and matches with + component-aware `Path::starts_with` so `/tree` does not match + `/tree-sibling`. That is character-for-character the rule rev 6 wrote + for `reconcile_delete`, arrived at independently. **Whichever lands + first owns the query and the other adopts it**, which is #186's own + Q#RD5 boundary wording. `reconcile_rename` then uses the same query — + #186's §6 explicitly parks "the rename side of prefix-aware, + normalizing lookup" as dired Stage 2's. + +#### What residue survives the refusal, and why + +- **The async race is real, and rev 7 narrows whose it is.** #186's + refusal is synchronous and lives inside `apply_resource_op`. **Dired + never goes through `apply_resource_op`** — it calls `pmacs.fs.remove`, + which dispatches a worker (§2), so the refusal cannot reach it at any + strength. Dired's own pre-dispatch check closes the window up to + dispatch; between dispatch and `remove_blocking`'s `remove_file` the + interval stays open, and a buffer modified in it is orphaned. + **Correcting rev 6's framing of this:** rev 6 called it "the residue + rev 3 scoped to the LSP path applies to dired too — one deferral, two + paths". After #186 that is backwards. The LSP path has no residue, + because it refuses. **It is one deferral, one path, and the path is + dired's** — it always was, and rev 6 obscured it by bundling. +- **The full post-delete lifecycle** stays this lane's, and #186 hands + it over explicitly rather than by omission. Its Q#RD8 parks "the + window/last-buffer defects" and states the trap rev 6's R4 found + independently: `kill_buffer` and `remove_buffer_and_fire` clean + **disjoint** sets, so neither is a superset and "just call + `kill_buffer`" silently regresses four cleanups. Its Q#RD5 then + deliberately keeps reconciliation **exact-path** — leaving descendant + buffers orphaned-and-clean after a recursive delete — precisely + because widening it "would promote mode (d)'s dangling-window and + last-buffer defects from an exact-path defect to a tree-wide one". + **That is this lane's Q#DR27.** #186 narrows on purpose so that #171 + can widen safely once the lifecycle is correct; the two decisions fit, + and neither is complete alone. + +#### The LSP failure-handling claim — checked, and not inherited + +The coordinator flagged that #186 mis-assigns `Abort` to edits +containing resource operations and warned this document might carry the +same error. **It does not.** `grep` over this file for +`failureHandling|failure handling|Abort|transactional` returns **10 +lines, none of which is a claim about the LSP specification** — every +one is about hook fan-out or dired's own batch semantics ("a per-entry +failure does not abort the batch"). Rev 6 asserted nothing about +`WorkspaceEdit` recovery. + +The one adjacent claim was §6's *"refusing mid-edit leaves a +half-applied refactor"*, offered as the reason the LSP arm should +delete anyway. That was a claim about consequence rather than about the +protocol — but it was the load-bearing support for Class B, and it goes +with it. + +Verified against the **LSP 3.18** specification so the record is +accurate for both lanes: + +- `abort` — *"Applying the workspace change is simply aborted if one of + the changes provided fails. All operations executed before the failing + operation stay executed."* **No mention of resource operations.** +- `transactional` — *"All operations are executed transactionally. That + means they either all succeed or no changes at all are applied to the + workspace."* This **does** cover resource operations. +- `textOnlyTransactional` — *"If the workspace edit contains only + textual file changes they are executed transactionally. If resource + changes (create, rename or delete file) are part of the change the + failure handling strategy is abort."* **This is the only kind that + degrades for resource ops**, and it is the sentence that gets + misattributed to `abort`. +- `undo` — *"The client tries to undo the operations already executed. + But there is no guarantee that this is succeeding."* +- The `failureHandling` capability is described only as *"The failure + handling strategy of a client if applying the workspace edit fails."* + **The specification states no default for a client that does not + advertise it.** + +So "pmacs advertises none, therefore `Abort` semantics apply by +omission" does not follow, and neither does "the protocol declines to +promise transactionality for resource-op edits" — `transactional` +promises exactly that. Recorded here rather than only in a review +comment because this document's §6 will sit next to #186's §1.7 in the +same arc, and the corrected reading should be findable from either. + +#### One supporting citation in #186 that has gone stale + +Not a disagreement with the split, and not grounds to revisit it — +#186's Q#RD5 justifies taking the delete side now partly by citing "the +ledger's own assessment of that lane: PR #171 is **OPEN, STALE, DO NOT +MERGE AS-IS**, 153 commits behind at the last snapshot, under re-scout." +That was accurate when written and is no longer: #171 completed the +re-scout, is integrated to `ad41cf1`, and is at revision 7. The +*conclusion* stands on urgency alone — #186 fixes live data loss and +this lane is not implementable until its framing is approved — so +nothing about the split changes. Recorded so #186's next revision can +drop a citation that would otherwise read as current. + +--- + +### Review round 5 (rev 5 → rev 6) — six findings, four P1, none approved + +Round 5's theme is **one theme, not six**: rev 5 changed the slice split +and the ownership of a decision, and the prose did not follow. Four of +the six findings are that same defect in different places. Every cited +line was verified against the tree before being acted on; **all six +hold**, and two of them are *understated* — see R2 and R4. + +- **R1 (P1) — rev 5 was superseded by its own ledger entry, and by a + PR body older still.** Verified: `docs/active-work.md:507` records + that Q#DR25 has **moved out of this lane** — dired's + `bypass_intercept`-over-a-writable-rope paint turned out to be a + **class** bug, the same idiom appearing in listview, `compile.lua` and + the search/grep panel, with **zero Lua callers anywhere setting + `read_only`** — and that a separate lane now owns it (branch + `generated-buffer-immutability`, worktree + `../pmacs-generated-immutability`, framing in progress). Meanwhile + rev 5 still called the adoption a fix "Stage 2 may not skip" (§1) and + assigned it to 2b (§10), and the **GitHub PR body** still described + revision *1*'s two-slice plan against `c8ec8f3`. Three sources, three + different stories. **Rev 6 reconciles all three:** §1 and §3.1 now + defer to the lane, §10's table drops the row, Q#DR25 is restated as a + deferral in §11 and §15, and the PR body is rewritten. *Dired's + adoption is not cancelled — it is owned elsewhere*, and this document + now says where rather than deleting the problem. **Correcting one + thing rev 5 got wrong in the other direction:** rev 5's §0.5 claimed + Q#DR25 "closes dired's quarter of that gap". That was already + arguable, and the class-bug finding settles it — dired was never a + quarter of anything; it was one instance of one idiom. +- **R2 (P1) — the acceptance allocation contradicted the code split, + and the review understates it by one.** Verified: items **23–24** test + `apply_resource_op`'s delete arm and fire-and-forget `pmacs.fs.remove` + — both pure **2a** substrate — yet sat under a `**2b** — deletion + policy` header and inside 2b's `1–24` table range. Conversely item + **33** asserts an open dired buffer follows a directory rename, which + needs a `dired.lua` subscriber, while §10 and §16 both define 2a as + containing **no dired code at all**. Rev 6 **moves 23–24 into 2a** and + **moves 33 into 2b**, and justifies the direction rather than leaving + both readings open (§13). **The review asked for a choice on 33 and + the choice is 2b**, because 2a's entire review rationale — §10's "with + no dired surface at all" and §16's "its diff contains no dired code" — + is what makes it reviewable as substrate rather than as a dired + feature, and admitting one dired subscriber to satisfy one acceptance + item would spend that. The cost is stated: between 2a and 2b a + directory rename leaves dired handles stale, which is **exactly the + status quo** and so is not a regression. **The undercount:** moving 33 + out leaves 2a shipping `resource.renamed` with no acceptance on the + hook itself, so rev 6 adds **item 50** pinning the hook's own contract + (fires once per *successful* rename, with normalized absolute paths), + which is what 2b's subscriber attaches to. +- **R3 (P1) — `TickOutcome` could not carry what §6 asked of it.** + Verified: the struct as specified held `settled` and `renames` only, + while §6 required deletion to emerge "via the same `TickOutcome`". As + written `_tick` could not learn which settled `FsRemove` to reconcile + or which path to pass to `resource.deleted`. Rev 6 replaces the two + ad-hoc vectors with **one ordered `Vec`** and makes + `PendingJob` carry a single `Option` rather than rev 5's + `rename_paths` (§5). Ordering is the reason it is one vector and not + two: a directory rename and a delete beneath it can settle in the same + tick, and reconciling them out of order reconciles the wrong path. + A single enum field also refuses the impossible both-`Some` state, the + argument `ResolvedTarget`'s own doc makes at `src/editor_core.rs:100-102`. + *(The ordering rationale is historical here and is superseded by rev + 9 F1/Q#DR29; the enum-shape rationale survives.)* +- **R4 (P1) — `reconcile_delete` stopped short of the real removal + lifecycle, and the substrate is worse than the review says.** The + review is right that removal is two phases — `EditorCore::kill_buffer` + (`src/editor_core.rs:4590`) for window and side-window cleanup, and + `after_buffer_removed` (`src/lua_bindings/mod.rs:1602`) for keymaps, + buffer-local config, folds and `on_removed` callbacks. **What it does + not say is that no existing Rust path composes them, and the one + `apply_resource_op` uses is the incomplete one.** Verified: + `pmacs.buffer.kill` (`install_buffer_kill`, `mod.rs:5476-5491`) *does* + compose both, and its doc comment says it is late-bound precisely + because it "needs an `EditorCore` handle to redirect any windows + showing the doomed buffer before removal". But `apply_resource_op`'s + delete arm calls **`remove_buffer_and_fire`** (`mod.rs:1592`) = + `registry.remove` + `after_buffer_removed`, with **no window cleanup** + — and `BufferRegistry::remove` touches only `buffers` and `order`. So + **an LSP-authored delete leaves any window displaying that buffer + pointing at a removed id**, which is a *third* defect on that arm that + rev 5 never named, alongside the missing dirty check and the + first-match lookup. §6 now specifies both phases, the failure modes + (`ConcurrentEdit` on a mid-edit buffer, and the refusal to kill the + **last** remaining buffer), and acceptance items 51–53. +- **R5 (P2) — the "every consumer" claim was false for Lean.** Verified + the contradiction: §1 listed `lean.lua`'s progress table among the + consumers reconciliation reaches, while §11 correctly said Stage 2 + does not write that subscriber. **The deferral was right and the + summary was wrong**, which is the direction rev 6 fixes: Stage 2 + supplies the hook; Lean's URI-keyed state stays stale until its owner + adopts it. Owner 6 remains in §5's census — it is still evidence that + `forget_uri` cannot be complete — it is just no longer claimed as + fixed. +- **R6 (P2) — pre-three-slice text in the file inventory.** Verified + both halves: §7 requires `builtin/runtime/minibuffer.lua` to join + `src/editor.rs`'s explicit `include_str!` load sequence (which is a + real edit to `src/editor.rs` — the sequence is ~30 entries at + `src/editor.rs:395-660`), contradicting §16's "2b is `dired.lua` plus + one killring binding"; and `+` and `C` were still tagged **2b** in §1's + table though §10 puts them in 2c. + +**The sweep the review asked for caught four more instances of the same +defect**, none of them cited: + +| Where | Said | Should say | +|---|---|---| +| §4, the set-based class list | "`C` (copy, **2b**)" | 2c | +| §7, the confirmation surface | "`x`, `D`, and (in **2b**) a recursive delete and an overwriting copy" | 2c | +| §8, `d`/`x` | "reported as such in **2a**; **2b**'s `remove_dir_all`" | **wrong twice** — `x`'s report is a **2b** surface, and `remove_dir_all` is **2c** | +| §8, the op sections | "**`+`** (create directory, **2b**)" and "**`C`** (copy, **2b**)" | 2c | + +The §8 line is the clearest survivor of the two-slice era: under rev 1's +plan "2a" was the mark layer and "2b" was the primitives, so a +sentence written then reads one slice off in *both* halves after the +three-way cut. All are corrected. The three historical round sections +keep their original labels, where "2b" means the old primitives slice — +flagged in the note at the head of round 1 rather than retconned. + +--- + +### Re-scout round 4 (rev 4 → rev 5) — `c8ec8f3` → `6bee09d`, 153 commits + +No reviewer produced these; a re-scout did. Rev 4's design survives, but +**one new dependency changes what Stage 2 must build, one changes the +shape of a mechanism rev 4 described, and seven of rev 4's own claims +about pmacs were wrong** — which is the failure mode this arc has been +burned by before (a framing that verifies its *external* facts and gets +its *internal* ones wrong). Every claim below was read on the tree at +`6bee09d`, not inferred. + +#### What arrived that Stage 2 must now answer + +- **N1 (new scope, blocking the mark layer) — #178 landed + `Buffer::set_generated_contents`, and dired has not adopted it.** + `src/buffer.rs:545` is now *the* authorized write path for a generated + buffer: lift `read_only`, one whole-buffer `Replace` skipping + intercepts, `clear_history`, re-assert `read_only`, and **return the + `Edit`**. `docs/agent-handoff.md` §4 names `builtin/runtime/dired.lua:371` + as one of **four writer mechanisms that have not adopted it**, and + `COHERENCE.md` §14 records the same. Dired still pairs an erroring + intercept (`dired.lua:509-511`) with a `bypass_intercept` whole-buffer + replace (`paint`, `dired.lua:369-372`) **over a still-writable rope**, + so **`M-x buffer.undo` empties a dired listing today** — the command + exists (`builtin/commands/default.lua:179`), needs no keybinding, and + `Buffer::undo` reaches the rope through `ensure_writable` + (`src/buffer.rs:568-577`) without ever consulting the intercept chain. + Stage 2 writes that buffer on every mark, every unmark, every toggle, + and after every batch, so it multiplies the exposure rather than + inheriting it quietly. Rev 5 adopted the primitive as **Q#DR25**. + **Round 5 withdrew that** — the same idiom turned out to be in + `listview.lua`, `compile.lua` and the search/grep panel, so it is a + class bug with its own lane (R1). The re-scout finding stands; only + its owner changed. See §3.1. +- **N2 (shape change) — #182 (Journey Stage 1a) demoted dired to a + replaceable slot, and rewrote the function Stage 2's operations run + inside.** `resolve_target_buffer` gained a `ResolvedTarget::Directory` + arm **ahead of** the load (`src/editor_core.rs:964-970`); + `EditorState::open` became a caller rather than a parallel + implementation (`src/editor.rs:953-959`); and which surface handles a + directory is now the `path.open-directory` hook chain with dired as a + **replaceable fallback slot**, `pmacs.path.directory_handler` + (`dired.lua:736-738`, registered via + `pmacs.path.set_directory_handler`, `src/lua_bindings/mod.rs:3679`) — + deliberately **not** a hook subscriber, because `HookRegistry::add` + only appends and builtins load before `init.lua`, so a dired + subscription would always claim before any user listener. The + consequence for Stage 2 is in `open_directory`, which gained + `opts.dest` and a `commit()` closure run under + `pmacs.window.commit_to` (`dired.lua:656-707`), and that scope + **refuses an `await` inside it** (Q#JR14b). §9's serialize-and-await + batch therefore has a constraint rev 4 could not have known about. +- **N3 (a ratchet Stage 2 must not break) — `tests/journey_acceptance.rs`.** + **24 `#[test]`s**, 0 ignored, 0 feature-gated, 2 gated + `#[cfg(target_os = "linux")]`. Its module doc states the rule + verbatim at `:12`: *"**This file is a ratchet: stages add rows, none + removes them.**"* Seven of its tests assert on dired directly, and its + step-3 rows pin that a directory launch produces a `*dired:*` + buffer. **The ratchet is split across two files**: #183 put the GPU + journey row in `tests/gpu_invocation_acceptance.rs` + (`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`), + not in `journey_acceptance.rs`. Both are gates now (§14). + *(Correcting the re-scout brief: #183 did **not** extend + `journey_acceptance.rs` — `git diff c2d56ff 7fd646d -- tests/journey_acceptance.rs` + is empty.)* +- **N4 (a fan-out lesson Stage 2's two new hooks inherit) — #179/#181 + landed the typed-edit consumer chain.** Dired participates in + **neither** the chain nor `buffer.after-edit` (grep of `dired.lua` for + `typed_edit|after-edit`: **0**), and `set_generated_contents` fires no + hook — its Lua binding routes only through + `notify_buffer_edit_to_windows` (`src/lua_bindings/mod.rs:3092`), + which runs no Lua. So N1 does not drag dired into the chain. But + `resource.renamed` / `resource.deleted` are new fan-outs and inherit + the chain's three hard-won lessons: a consumer cannot both edit and + let a later consumer act meaningfully (the record is a **copy** taken + before any consumer ran); `buffer.after-edit` fan-outs **nest**; and + the thing that counts fan-outs must be **unskippable**. §5 states + which of these bind the new hooks and which do not. + +#### Rev 4 claims that are wrong about pmacs itself + +- **W1 (load-bearing) — §5 step 2 names one function and cites a + different one's line.** Rev 4: *"`drain_external_cancelled` (`:1596`) + is the existing sweep."* These are **two distinct functions**: + `LspManager::drain_external_cancelled` (`src/lsp.rs:1561-1576`) is the + **server-scoped, unconditional** drain — settle every awaiter for + `sid` cancelled and drop its `pending_external` entries; that is the + right precedent. `LspManager::drain_cancelled_externals` + (`src/lsp.rs:1596-1645`) is a **per-tick, per-awaiter cancellation and + timeout sweep** that removes only awaiters whose `CancellationToken` + was flipped or which outlived `request_timeout`. An implementer + following rev 4's line number reaches the second: **a rename flips no + token, so the drain half of `forget_uri` would be a silent no-op** and + a coroutine awaiting a request against the old URI hangs forever — + precisely the failure step 2 exists to prevent. Corrected in §5 and + Q#DR23. +- **W2 — there is no `fn restart`.** Rev 4 models `forget_uri` on *"the + server-scoped teardown that already exists (`restart`, `:1316-1331`)"*. + The teardown is **`LspManager::start_generation`** + (`src/lsp.rs:1307-1345`); its route purge is at `:1324`, which is the + one number rev 4 got right. And there is a **second** precedent rev 4 + never names: **`LspManager::forget`** (`src/lsp.rs:3015-3042`), which + does the same three things (`pending_routes.retain`, + `drain_external_cancelled`, `documents.retain`) plus + `status_tracker.forget` and `project_servers`. Rev 4's genuinely + surprising observation survives and now applies twice: **neither** + server-scoped teardown clears the fourteen result stores. +- **W3 — the route inventory is off, and the gap is the part an + implementer trips on.** `ResponseRoute` (`src/lsp.rs:835-859`) has + **15 variants, of which 14 carry a `uri`**; the fifteenth is + `WorkspaceSymbol { query }`. There are **16** insert sites, all in + `request_*` methods spanning `src/lsp.rs:1683-2132` — and **15 of the + 16 insert a URI-bearing route**. Rev 4's enumeration of fifteen was + therefore *correct as a list of URI-bearing inserts* and *wrong as a + claim about how many insert sites exist*. The arithmetic matters + concretely: a purge predicate written as *"retain unless the route's + uri equals the old one"* has to say something about a variant that has + no `uri` field at all, and `request_workspace_symbol` + (`src/lsp.rs:1877-1891`) is that variant. Stated in §5. +- **W4 — the Lua-side blast radius is ~3× what rev 4 said.** F1 put + `rec.uri` at *"~20 sites"*. The real figure on this tree is **57 + lines** in `builtin/runtime/lsp.lua` (`grep -c "rec\.uri"` = 57, one + per line). Rev 4's design is unaffected — the point of caching + `rec.uri` in the attachment record is that one rebind reaches all of + them — but the magnitude was understated and the arc has a standing + rule against sizing anything from a partial count. +- **W5 — the path-owner census has grown to SIX.** Rev 4's five owners + all still hold (re-verified individually in §5). A **sixth** arrived + with the Lean 4 stages: **`lean.lua`'s `M.file_progress`** + (`builtin/runtime/lean.lua:646`, written at `:651`), a **URI-keyed + Lua module table** populated from the `$/lean/fileProgress` + notification and, by its own comment, read by Stage 5's goal view. It + lives in **no Rust store**, so `forget_uri` cannot reach it; it *is* + reachable from a `resource.renamed` Lua subscriber, which is an + argument for the hook rev 4 already proposed. Two weaker, **persisted** + owners are also now named rather than fixed: `saveplace.lua`'s + path-keyed places file and `recentf.lua`'s MRU list (§11). +- **W6 — the ledger note names a merged PR.** §16 said + `docs/active-work.md` and `docs/agent-handoff.md` are held by open PR + #169. #169 merged as `74301d1`. The current holder is **PR #185** + (`docs-landed-state-184`). The instruction is unchanged; the number + was stale. +- **W7 — the C1 seam moved, and it has two namesakes.** + `pmacs._async._tick` is at `src/lua_bindings/mod.rs:7104-7115`, inside + `install_async`, not `:6911-6924`. Its closure is + `move |lua, ()|`, so `lua.app_data_ref::()` is reachable + and C1's decision stands. Worth naming because `mod.rs` defines + **three** `_tick` bindings in different classes — + `install_async:7104` (`pmacs._async._tick`), + `install_process:8469` (`pmacs.process._tick`), and + `install_lsp:10271` (`pmacs.lsp._tick`) — and only the first has + `lua` in scope. + +#### What the re-scout confirmed unchanged + +Listed because "still true at `6bee09d`" is the load-bearing half of a +re-scout, and because each of these was read again rather than carried +forward: the whole `pmacs.fs` surface and its five `*_blocking` +implementations, at **identical line numbers** (§2); the absence of +`mkdir`, `copy`, and any recursive remove (the four `create_dir` hits in +`src/fs.rs` are all inside `#[cfg(test)]` bodies); `PendingJob`'s seven +fields and `JobKind`'s **12** variants; `tick`'s signature +(`pub fn tick(&self) -> Vec`, `src/async_runtime.rs:1003`) and +the `Sleep | FsUnit` collapse at `:1046`; `dispatch_fs_rename` moving +both paths into the closure (`:871`); `LspManager`'s **fourteen** +URI-bearing store families (`src/lsp.rs:753-793`), exactly the fourteen +rev 4 tabulated, in the same order; `documents` at `:810` and +`pending_external` at `:804` with the *"drained-cancelled wherever +`pending_routes` is purged"* contract at `:801-803`; +`DiagnosticView.uri` private, set once at construction, its doc still +reading *"M5 may add re-rooting if a buffer is renamed"* +(`src/diag.rs:456-457`); `View` still carrying `overlay_identity` and +`clone_for_split` and **no** `rename_resource` and no downcast +(`src/view.rs:300`, `:310`, `:285`); the overlay-disposal sweep over +`core.windows.values_mut()` (moved to `src/lua_bindings/mod.rs:2044-2046`); +`_attach_view` taking `active_window_mut()` and erroring +(`src/lua_bindings/diag.rs:211-232`); `apply_resource_op`'s rename arm +still doing a raw-path `find_by_path` first-match rebind +(`src/lua_bindings/mod.rs:3306`) and its delete arm still killing +through `remove_buffer_and_fire` with **no modified check** +(`:3339-3341`) — **so an LSP-authored delete still destroys unsaved work +on `main` today** (still true at `ad41cf1`; **PR #186 is what fixes it**, +not this lane — see round 6); `apply_workspace_edit` still capturing `origin` as a +**string** and restoring with `find_or_open` (`builtin/runtime/lsp.lua:1338`, +`:1352`), its own comment still conceding the path *"may have just been +renamed or deleted"*; `resolve_target_buffer`'s `NotFound` arm still +materializing a phantom path-backed buffer (`src/editor_core.rs:980-989`); +`Buffer::set_name` existing with the doc *"Used by save-as and rename +operations"* (`src/buffer.rs:453`) and **no** `pmacs.buffer.set_name` +Lua binding; `killring.lua`'s `push_entry` still a `local function` +(`:93`) and `copy()` still region-required (`:184-194`); **no** +`builtin/runtime/minibuffer.lua` and **no** `y_or_n`/`yes_or_no`/ +`yes-or-no` anywhere in `src/` or `builtin/`; `pmacs.fs.rename` still +having **zero production callers** (only `tests/fixtures/pmacs-dired/init.lua:1200,1210`, +`tests/m8_1_acceptance.rs:359`, and `tests/m8_3_acceptance.rs:182-184`'s +monkeypatch) — and the same is now separately confirmed for +`pmacs.fs.remove` (only `tests/m8_1_acceptance.rs:438,439,472`); and +every one of dired's own Stage 1 seams at **unchanged line numbers** +(`render_entry:334`, `paint:369-372`, `entry_at_cursor:381-385`, +`seat_cursor:405-416`, the handle shape `:521-528`). The stale `*errors*` +comment rev 4 offered to fix in passing is still there, having moved +from `:665` to `:718`. + +Two dired references in rev 4 **did** drift and are corrected in §2: the +mode-scoped `bind` helper is at `dired.lua:933-935` with the bindings at +`:937-946` (was `:866-868`), and `pmacs.dired._layout` is at `:954-961` +(was `:887-895`). + +--- + +### Review round 1 (rev 1 → rev 2) + +> **The three round sections below are a historical record and keep the +> line numbers they were written with (`c8ec8f3` / `c93f9ee`).** Their +> *findings* all still hold — each was re-verified at `6bee09d` — but +> for a current reference use §2 and §5, which carry the re-scouted +> positions. Where a round-section claim was found to be wrong about +> pmacs, §0's round-4 list says so and the live section is corrected; +> the round text is left as written rather than retconned. + +Every checkable claim in the review was verified against `c8ec8f3` +before being acted on; all seven held. + +- **F1 (blocking) — the rename contract reached only one path owner.** + Rev 1 rebound `Buffer.file_path` and stopped. Verified: `Buffer::set_name` + documents itself as "used by save-as and **rename operations**" + (`src/buffer.rs:452`) and `set_buffer_path` never calls it, so the + statusline and buffer list keep the old filename; `rec.uri` is cached + per attachment in `lsp.lua` and read at ~20 sites (didChange `:418`, + semantic tokens `:776-790`, diagnostics `:953`, signature `:1016`, + definition `:1648`, references `:1757`); dired's own buffers are + **pathless** so no buffer-keyed rebind can ever reach them; and the + workspace-edit path captures `origin = active_buffer_path()` + (`lsp.lua:1252`) then calls `find_or_open(origin)` (`:1266`), which for + a since-renamed path does not fail gracefully — `resolve_target_buffer` + turns `NotFound` into "an empty path-backed buffer" + (`editor_core.rs:876-878`), i.e. it **materializes a phantom buffer at + the obsolete path and selects it**. Rev 2 replaces the rebind with a + shared reconciliation transaction plus a `resource.renamed` hook (§5). +- **F2 (blocking) — deletion of visited paths had no policy.** Correct, + and rev 1 simply did not consider it. §6 is new and decides all four + cases; the modified-buffer case blocks rather than orphans. +- **F3 (high) — the key table silently changed approved scope.** Verified + against the parent's table (`dired-framing.md:963-966`): it lists `w` + (copy filename to the kill ring) in Stage 2 and contains **no** `M`. + Rev 1 dropped `w` and added `M` without saying so. Worse, the parent + states at `:543-546` that the fixture "rejects symlink perms edits at + intercept time ... **that decision carries over unchanged**", and rev 1's + `M` chose to chmod the symlink's target with a warning — a direct + contradiction of approved text. `w` is restored (§8, Q#DR20) and `M` + is now an explicit new decision that **refuses symlinks** (Q#DR19). +- **F4 (high) — Q#DR13 contradicted the `R` contract.** It did: "every + operation targets the marked set ... except `x`", then `R` point-only. + Q#DR13 is narrowed to name the set-based operations explicitly, with + `R` and `w` as point-based by construction (§4), plus acceptance for + `R` with unrelated marks present. +- **F5 (high) — load-bearing decisions lacked falsifying acceptance.** + All five additions taken; see §13 items 14, 27, 39, 40, and 41 + (renumbered again in rev 4). +- **F6 (medium) — `take_settled_renames()` was underspecified.** Correct: + a no-argument drain either needs a second queue or a scan of every + settled entry, neither of which rev 1 named. Rev 2 takes the reviewer's + preferred shape — `tick` returns a structured outcome so settle + identity and rename metadata stay in one transaction (§5). +- **F7 (2b) — overwrite and recursive-delete safety were incomplete.** + §8 now defines the `C` command flow including per-collision handling in + a multi-source batch, pins `remove_dir_all`'s lstat safety at the + primitive, and states `dired.recursive-deletes` as boolean, default + **false**. + +One thing the review asked for that rev 2 does **not** do: it does not +widen `R` to the marked set. Multi-file rename needs a target-directory +concept that does not exist, so `R` stays point-based and is *named* as +such rather than being an unstated exception. §11 keeps it deferred. + +--- + +### Review round 3 (rev 3 → rev 4) + +Round 3's theme: rev 3 named the right seams but two of them were sized +from a partial inventory, and one promise was still stronger than the +mechanism behind it. All three verified against `c93f9ee`. + +- **H1 (blocking) — the modified check still races the syscall.** + Correct, and rev 3's "immediately before each syscall" was wrong about + where the boundary is: `pmacs.fs.remove` **dispatches a worker**, so the + interval between the Lua check and `remove_blocking`'s + `remove_file`/`remove_dir` is wide open, and acceptance 20 (edit before + `y`) could never detect it. Rev 4 **narrows the promise** to a + TOCTOU-bounded pre-dispatch check — the same honest framing G6 forced on + `R` — rather than inventing a reservation primitive inside a dired + stage. The residue is stated: reconciliation preserves the + newly-modified *buffer*, but the *file* is gone, so **the orphan + deferral rev 3 scoped to the LSP path applies to dired too** (§6, §11). +- **H2 (blocking) — the LSP teardown inventory was a third of the real + one.** Rev 3 cleared five stores. `LspManager` (`src/lsp.rs:741-819`) + holds **fourteen** URI-bearing store families, plus the `documents` text + map that `didChange` diffs against, and `pending_routes` — whose + `ResponseRoute` variants **carry the URI** at fifteen insert sites + (`:1684-2133`), so an in-flight response repopulates a key *after* any + clear. §5 now carries the full inventory and a purge/drain policy + modelled on the server-scoped teardown that already exists (§5). +- **H3 (blocking) — the diagnostic-view seam was still "either/or".** + Verified: `DiagnosticView.uri` is private and immutable, `View` has no + downcast, and `_attach_view` (`lua_bindings/diag.rs:211-232`) takes + `active_window_mut()` and **errors** if the active window is not showing + the buffer — so it reaches one window. Rev 4 **chooses** the seam, and it + has direct precedent: a `View::rename_resource` default-no-op hook + alongside `overlay_identity` and `clone_for_split` (the hook family + #113 added for this exact class), swept over + `core.windows.values_mut()` the way overlay disposal already is + (`mod.rs:2016-2019`). In-place mutation, so **overlay order is + preserved by construction** (§5). +- **H4 (merge-readiness)** — #169's problem, not this PR's; fixed there by + merging `c93f9ee` and aligning its recovery threshold with its own + canonical anchor. +- **H5 — four cleanups**, all taken: the §4 item-35 → item-40 reference, + the §5 acceptance 27 → 30 and 28 → 32 references, and the §10 table's + obsolete "rename-only Rust" description. + +**And the staging call, taken as directed: three PRs, not two** (§10, +Q#DR17). The reconciliation transaction lands first, on its own — after +three rounds the LSP lifecycle and multi-window diagnostic work are +substantial enough that reviewing them beside a mark layer would waste +the round. + +### Review round 2 (rev 2 → rev 3) + +Round 2's finding: rev 2 widened the rename fix into a *resource +transaction*, and four consumers of that transaction were named but not +actually reachable by it. All six substantive claims verified against +`c8ec8f3`. + +- **G1 (blocking) — acceptance 29 was unimplementable by the proposed + design.** `apply_workspace_edit` captures `origin` as a **string** + (`active_buffer_path()` is `pmacs.editor.file_path()`, + `lsp.lua:471-473`), so neither `reconcile_rename` nor `resource.renamed` + can reach an already-captured Lua local; the phantom survives. Fixed by + changing the applier: capture the **buffer handle** and restore with + `pmacs.window.switch_buffer`, with **no path fallback** (§5). +- **G2 (blocking) — the dired subscriber could not rename its own + buffer.** `dired.lua:34-38` records in its own module doc that **there + is no `pmacs.buffer.set_name`**, so "updates its handle and renames its + buffer" was not implementable. Rev 3 adds the narrow setter, which §5 + needs anyway for the `Buffer.name` half of the transaction (§5, Q#DR21). +- **G3 (blocking) — `rec.uri` was not the last LSP path owner.** + `DiagnosticView` captures its URI **at construction** and the field's + own doc says so: *"Set once at construction; M5 may add re-rooting if a + buffer is renamed"* (`src/diag.rs:455-457`). Five more stores are + URI-keyed (`pmacs.diag`, `semantic_tokens`, `signature`, `definition`, + `references`). §5 now carries a complete teardown/re-attach contract. +- **G4 (blocking) — Q#DR18 had no shared seam and was racy across the + prompt.** Verified: `apply_resource_op`'s delete arm + (`mod.rs:3256-3285`) kills via `find_by_path` — raw path, **first match + only, no descendants, and no modified check**, so an LSP-driven delete + **destroys unsaved work today**. Rev 3 defines one shared + `reconcile_delete` seam and revalidates modified state immediately + before each syscall (§6). +- **G5 (high) — `w` had no implementation surface and the wrong + semantics.** `push_entry` is a `local function` (`killring.lua:93`) and + the public `copy()` requires a region, failing with "no region" + otherwise (`:182-190`). Rev 3 adds `pmacs.killring.push` and — taking + the reviewer's point that the parent approved the *binding*, and Emacs + copies marked filenames — makes `w` **set-based**, so `R` is now the + only point-based operation (§4, Q#DR20). +- **G6 (high) — `R`'s no-clobber was only a preflight.** `rename_blocking` + calls plain `std::fs::rename` (`fs.rs:492-499`), which silently + replaces an existing target, so a target appearing between the check and + the syscall is overwritten. Rev 3 **narrows the claim** rather than + overstating it, and names a no-replace primitive as deferred (§8). +- **G7 — four cleanups**, all taken: `lsp_multi_root` added to §14, the + §13/§7 cross-reference slips fixed, and the stale "2a's only Rust is the + rename rebind" line corrected — it is no longer true after the widened + transaction. + +### Corrections to the parent framing (rev 1, unchanged) + +Five corrections, one of them load-bearing. + +- **C1 (load-bearing).** The parent says the rename rebind belongs "in + the main-thread completion drain, `AsyncRuntime::tick` + (`async_runtime.rs:991`)". The *decision* is right and this framing + keeps it, but **it cannot be implemented there**: `AsyncRuntime` + (`src/async_runtime.rs:513-549`) holds a worker pool, a bus, and job + tables — it has no buffer registry, no `EditorCore`, and no Lua handle. + Putting a buffer rebind inside it would invert the layering. The actual + seam is one level up: **`pmacs._async._tick` + (`src/lua_bindings/mod.rs:6911-6924`)**, the production binding that + calls `rt.tick()` and is already a `lua.create_function` closure with + `lua` in scope — so it can reach `SharedCore` by + `lua.app_data_ref::()`, exactly as `apply_resource_op` + does (`:3251`). §5 specifies the split: `AsyncRuntime` *harvests*, the + binding *rebinds*. + *(Rev 5, W7: the decision stands, verified again at `6bee09d`. The + seam is now `src/lua_bindings/mod.rs:7104-7115`, inside + `install_async`; its closure is `move |lua, ()|`, so + `lua.app_data_ref::()` is still reachable. Note that + `mod.rs` defines **three** `_tick` bindings in different classes — + `install_async:7104`, `install_process:8469`, `install_lsp:10271` — + and only the async one has `lua` in scope.)* +- **C2.** The parent's line references have drifted. `tick` is now + `:1003` (was 991); the `Sleep | FsUnit → JobResult::Unit` arm is + `:1046-1048` (was 1022-1025); `apply_resource_op`'s raw lookup is + `:3249` (was 3248). Stage 1's `ReadDir` variants account for the shift. + Three references were already exact and still are: `find_by_path` + (`buffer_registry.rs:168-174`), `find_buffer_for_path` + (`editor_core.rs:864-867`), and the normalize-on-write in + `set_buffer_path` (`:819`). +- **C3.** The parent says the fixture's 45 behavioral tests are the + reference for dired behavior. For Stage 2 that is **false and worth + knowing**: the frozen fixture has **no mark-and-operate layer at all**. + It defines eight commands — `open-line`, `parent`, three sorts, and + three `wdired` — and binds exactly two keys (`RET`, `Backspace`, + `tests/fixtures/pmacs-dired/init.lua:381-388`). Its "marks" are + `pmacs.buffer.mark_create` *text position* marks used by wdired + (`:716-770`), not dired mark flags. **Stage 2 has no in-repo reference + implementation and no existing test coverage to match.** Stage 3 does. +- **C4.** The parent's `y_or_n` claim is confirmed stronger than stated: + grep across `builtin/` and `src/` finds **no** `y_or_n`, `yes_or_no`, + or `yes-or-no` anywhere, and there is no `builtin/runtime/minibuffer.lua` + at all — `pmacs.minibuffer` is entirely Rust-provided. §7 decides where + the helper lives. +- **C5.** The parent's list of three missing primitives is correct, and + `remove` covers more than it implies: `remove_blocking` + (`src/fs.rs:561-583`) already deletes **files and empty directories**, + and correctly unlinks a symlink-to-a-directory rather than following it. + So `remove_dir_all` is needed only for **non-empty** directories. This + is what makes the staging cut in §10 possible. + +--- + +## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) + +**Sections served: §20 Priority 1 (protect the golden product journey) +and §14 (coherent workbench primitives).** Rev 4 cited neither by +number; the re-scout makes both concrete, and Priority 1 is a *new* +claim that only became true when #182 landed. + +- **Journey steps — and this changed under rev 4.** Rev 4 said Stage 2 + touches "step 7's file half" only, and that dired "deepens a surface + the user must still already know about". Half of that is now false: + **#182 put dired on step 3.** `COHERENCE.md` §20 Priority 1 records + directory-argument handling as **done** and says it "routes `pmacs .` + into #165's dired buffer rather than growing a second directory + surface", and `tests/journey_acceptance.rs` pins exactly that + (`journey_step3_opening_a_directory_lists_it`). So dired is no longer + only a surface the user must know to ask for — for anyone who runs + `pmacs .`, **it is the first thing they see**, and step 5's row + (`journey_step5_editing_a_file_reached_through_the_directory`) reaches + the editable file *through* it. That raises the stakes on Stage 2's + correctness rather than changing its scope, and it is the strongest + argument for N1: a step-3 surface that `M-x buffer.undo` can empty is + a journey regression waiting to be filed. Discoverability of the + *marks* remains §20 Priority 4's job, not this stage's, and this + framing still does not claim otherwise. +- **Workbench convergence (§14, Priority 5).** `COHERENCE.md` §14 names + dired explicitly in the generated-buffer adoption gap — "four writer + mechanisms have not yet adopted it and remain emptiable … and dired + buffers" — and classifies dired, with listview, as **"the cheap + half"**, because both already write whole-buffer replaces and so need + none of the streaming variant the three appending buffers require. + **Rev 5 claimed Q#DR25 closed "dired's quarter" of that gap; rev 6 + withdraws both the decision and the claim** (R1). Dired was never a + quarter of anything — it is one instance of one idiom, and the missing + capability (no Lua `set_read_only`, so no Lua caller can assert + `read_only` at all) is shared by all four. **Stage 2 therefore makes + no progress on §14's generated-buffer gap, and no longer says it + does**; the `generated-buffer-immutability` lane owns it (§3.1). + Separately, §14's tree-primitive point is untouched: Stage 2 adds no + tree, and `i` (insert subdirectory) stays deferred (§11) precisely so + it can land on a shared primitive rather than inventing one. +- **Interaction islands: adds none.** Every new key is an entry in the + existing mode-scoped `dired` keymap (`dired.lua:937-946`), and every new + command is an ordinary `pmacs.command.define`, so all of it is reachable + from `M-x` and describable by `describe.key`. The confirmation prompt + uses the existing minibuffer rather than a new modal surface — that is + the *reason* §7 spends a section on it. +- **Config registry.** Stage 2a and 2b add no keys. Stage 2c adds + `dired.recursive-deletes` (Emacs's `dired-recursive-deletes`), through + `pmacs.config.define` like `dired.kill-when-opening` + (`dired.lua:78-84`). +- **Background-work attribution (§9): this stage makes a real, and + honest, dent in the wrong direction unless it is deliberate.** + `COHERENCE.md` §9 grades the worker model "mechanism without identity" + and names the prerequisite precisely: `PendingJob` + (`src/async_runtime.rs:367-375`) carries no owner, purpose, buffer + association, or parent, and `JobKind` is a **closed 12-variant enum**. + Stage 2 pushes on both: + - It must make a pending rename **retain its from/to paths** (§5). That + is a new `PendingJob` field which is *not* an owner or a purpose — a + one-off. The alternative shape, a `job_id → (from, to)` side map on + `AsyncRuntime`, is **worse by §9's own diagnosis**: §9 singles out the + existing parse-job→buffer link for living "in a `SyntaxCoordinator` + side map, invisible to the workers view". So the field is the + coherent choice of the two, and this framing takes it — while + recording that a general `purpose`/`owner` field should later subsume + it, and that Stage 2 is not the place to design that. + - Stage 2b grows `JobKind` from 12 to 15 variants. That is additive and + matches every existing fs op, but the closedness §9 objects to is not + something this stage fixes, and the framing does not pretend the + growth is progress. + +--- + +## 1. What Stage 2 ships + +The Emacs dired working loop: select a set of files, then act on it. + +| Key | Command | Effect | +|---|---|---| +| `m` | `dired.mark` | Mark entry under cursor `*`, advance | +| `u` | `dired.unmark` | Clear the mark, advance | +| `U` | `dired.unmark-all` | Clear every mark in this listing | +| `t` | `dired.toggle-marks` | Invert `*` marks across the listing | +| `d` | `dired.flag-delete` | Flag `D`, advance | +| `x` | `dired.execute-flags` | Delete every `D`-flagged entry, after confirming | +| `D` | `dired.do-delete` | Delete the marked set (or entry at point) now, after confirming | +| `R` | `dired.do-rename` | Rename the entry **at point** (§4) | +| `w` | `dired.copy-filename` | Copy the marked filenames to the kill ring | +| `M` | `dired.do-chmod` | Mode bits on the marked set; **refuses symlinks** (NEW — Q#DR19) | +| `+` | `dired.create-directory` | Create a subdirectory (**2c**) | +| `C` | `dired.do-copy` | Copy the marked set (or entry at point) (**2c**) | + +`w` is carried forward from the parent's approved table; **`M` is new +scope** and needs explicit approval, since the parent listed neither it +nor any chmod surface for Stage 2 (F3). + +Two small public surfaces come with it, both because the operations have +nowhere to land otherwise: **`pmacs.buffer.set_name`** (Q#DR21) and +**`pmacs.killring.push`** (Q#DR22). + +**Not in Stage 2, and no longer this document's decision (Q#DR25, +withdrawn in rev 6 — R1): dired's listing becoming a genuinely +immutable generated buffer.** Rev 5 added it here as a fix "Stage 2 may +not skip". It is a real defect — `M-x buffer.undo` empties a dired +listing on `main` today — but it is **not dired's defect**: the same +erroring-intercept-over-a-writable-rope idiom is in `listview.lua`, +`compile.lua` and the search/grep panel, and **no Lua caller anywhere +sets `read_only`**. A class bug gets a class fix, so it now belongs to +the **generated-buffer immutability lane** (branch +`generated-buffer-immutability`). §3.1 records what that lane inherits +from this one; §11 and §15 carry the deferral. **Stage 2 must not +implement it**, and Stage 2's acceptance must not pin it. + +Plus, invisibly: **renaming or deleting a path starts reconciling the +consumers this stage can reach** — buffer path *and* name, the fourteen +URI-keyed LSP store families and the attached diagnostic view, dired's +own pathless handles, and the workspace-edit applier (§5, §6) — **and +fires a hook so the ones it cannot reach are able to reconcile +themselves.** That distinction is load-bearing and rev 5 blurred it +(R5): `lean.lua`'s URI-keyed progress table is **not** reconciled by +this stage. Stage 2 supplies `resource.renamed`; Lean's state stays +stale until Lean's owner subscribes (§11). What Stage 2 *does* deliver +is a correctness fix to shared substrate: it is the reason `R` is safe +on a directory at all, and it gives rename and delete **one** registry +walk instead of the raw first-match lookup each arm does today. + +**What this lane no longer claims** (rev 7, round 6): closing the +LSP-authored delete that destroys unsaved work. That defect is real on +`main` and is **PR #186's**, which refuses the delete *before disk* +rather than reconciling after it. This lane inherits the refusal and +owns what remains once it is in place — the full post-delete lifecycle, +and the async race #186's synchronous guard cannot reach (§6). + +Not in Stage 2: `wdired` (Stage 3), subdirectory insertion (`i`), +shell commands on marks (`!`), regexp marking (`% m`), and +compress/symlink/hardlink ops. §11 names them. + +--- + +## 2. Ground truth (re-scouted 2026-07-28, `main` @ `6bee09d`) + +Everything below was read or executed on this tree, not inferred. Where +a fact was already in rev 4 and survived the 153-commit gap unchanged, it +is left as written; where a line number moved it is corrected in place +without comment; where a *claim* changed, the change is called out. + +### The filesystem surface + +`pmacs.fs` is exactly **five ops plus `watch`** +(`builtin/runtime/fs.lua`): `read_dir` (:124), `stat` (:133), `rename` +(:167), `chmod` (:177), `remove` (:187), `watch` (:267). Rust side: +`read_dir_blocking` (`src/fs.rs:289`), `stat_blocking` (:449), +`rename_blocking` (:492), `chmod_blocking` (:521), `remove_blocking` +(:561). **No `mkdir`, no `copy`, no recursive remove exists anywhere.** + +- `remove_blocking` (`:561-583`) lstats first, then `remove_dir` for a + **real** directory (non-recursive) and `remove_file` otherwise, with an + explicit comment that a symlink-to-a-directory is not a directory per + `lstat` and so gets unlinked rather than followed. **So `remove` + already handles files and empty directories, and deleting a symlink + never touches its target.** +- Mutating ops deliberately take **no** `supersede`, and `fs.lua:155-165` + states the reason (a cancelled mutation may still have completed) and + the prescribed alternative: *"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)."* §9 takes that instruction + literally. +- `chmod` **follows symlinks** (`fs.lua:145-153`) while `read_dir`/`stat` + lstat. So `M` on a symlink line changes the *target's* mode and a + refresh shows the link's own, unchanged mode. This is documented + substrate behavior, not a bug to fix here; §8 decides what `M` does + about it. +- `read_opts` (`:82-105`) rejects unknown keys. Any new op that takes + opts must go through it or repeat that discipline. + +### Rename, and why the buffer rebind is not where you would put it + +- `pmacs.fs.rename` has **zero production callers.** Repo-wide the only + callers are the frozen fixture (`init.lua:1200`, `:1210`), + `tests/m8_1_acceptance.rs:359`, and `tests/m8_3_acceptance.rs:182-184` + — which *monkeypatches the Lua function*. Changing rename's contract + therefore breaks nobody, and the monkeypatch matters for test design: + a Lua-level replacement bypasses a Rust-level rebind entirely. +- The fixture's wdired commit renames in **two phases through unique temp + names** (`init.lua:1197-1215`), awaiting each. A prefix-aware rebind + will therefore rebind an affected buffer twice (real→temp→final), + landing correctly. Worth knowing before reading a confusing trace. +- `dispatch_fs_rename` (`async_runtime.rs:871-879`) **moves `from` and + `to` into the worker closure.** Nothing retains them, which is exactly + why §5 needs a new field. +- `PendingJob` (`:367-375`) carries `{cancel, state, supersede_key, + stream_buffer, max_batch, kind, dispatched_at}` — `kind` is there, the + paths are not. +- Rename settles as an **undifferentiated `ReplyKind::FsUnit`**: the + `Sleep | FsUnit` arm maps both to `JobResult::Unit` (`:1046-1048`). + There is no `Rename` reply variant, so a drain **cannot key on the + reply**; it must key on the pending job's own `JobKind::FsRename`. +- **`tick`'s post-loop block already does exactly the read the harvest + needs.** `:1074-1108` iterates `newly_settled`, borrows `pending`, + and reads `job.kind`, `job.state`, `job.dispatched_at`, and + `job.supersede_key` to push a `CompletedSlot`. The harvest is one more + read in a loop that already exists. *(Unchanged at `6bee09d`, line + numbers included.)* +- `find_by_path` (`buffer_registry.rs:168-174`) is exact `Path` equality + over `self.order`, **first match only**. No prefix logic, and two + buffers on one path means one of them is invisible to it. +- `apply_resource_op`'s rename arm (`mod.rs:3291-3312`) does a + **synchronous** `std::fs::rename` on the main thread and then + `reg.borrow().find_by_path(&from)` with the **raw** path (`:3306`), + while stored paths are normalized on write (`editor_core.rs:889-890`) + and the normalizing wrapper `find_buffer_for_path` (`:935-938`) exists + and is bypassed. This is a **second**, LSP-facing rename path with the + same two defects; §5 fixes both in one change. *(Rev 4 cited + `:3234-3255` / `:3249` / `:819` / `:864-867`; the code is unchanged, + only its position.)* + +### A verified pre-existing defect Stage 2 must not lean on + +**A fire-and-forget non-stream job leaks its pending entry forever.** The +only two removals from `pending` are stream eviction of closed streams +(`:1225`) and `take_result` (`:1262-1282`); the Lua `Handle` is a +bare `setmetatable({_id = id}, Handle)` (`async.lua:55`) with **no +`__gc`**, so dropping a handle reaps nothing. + +Executed on this tree to confirm rather than infer — dispatch a rename, +pump until complete, never take the result: + +``` +PROBE renamed_on_disk=true pending_len_after_settle=1 +PROBE is_complete=true snapshot_active=0 snapshot_completed=1 +``` + +Two consequences. Good: **the settled job is still in `pending` when the +harvest runs**, so §5's design is sound. Bad: this is a real leak, it is +**pre-existing and not Stage 2's to fix**, and Stage 2 must not make it +routine. §9's serialize-and-await contract means dired's own ops reap +every entry they create; the rebind exists for *other* callers, who leak +today regardless. Named as a deferral (§11) with the note that +`pending_len` is the observable. + +### Marks: the precedent already in the tree + +`*buffer-list*` keys deletion marks by **stable id, not line index** +(`builtin/commands/default.lua:373`, set at `:507`, cleared at `:520`), +and **prunes marks whose target no longer exists** on refresh +(`:444-452`). That is the exact shape §3 adopts, one substitution +(basename for buffer id). + +### Prompts: the shadowing trap has a precise boundary + +Stage 0's finding is that a selected candidate shadows typed text. The +boundary is sharper than the parent framing records: +`resolve_accepted_value` (`src/minibuffer.rs:564-575`) **short-circuits +on `CompletionSource::None` at `:565-567` and returns the typed text +before the candidate branch is reached.** + +So: **a prompt with no `source` returns typed text verbatim, always.** +Every Stage 2 free-text prompt (a new name, an octal mode, a directory +name) omits `source` and is immune. A prompt that *wants* candidates +re-enters the trap deliberately — which is what §7 is about. + +There is no `builtin/runtime/minibuffer.lua`; `pmacs.minibuffer` is +Rust-only. The nearest existing confirm is `autosave.lua:219-224`, a +`source = function() return {"yes","no"} end` prompt tested with +`answer ~= "yes"`. + +### What Stage 1 left for Stage 2 to build on + +`dired.lua`'s handle is `{buf, path, entries, errors, sort_mode, prev}` +(`:521-528`) — **no mark state; §3 adds it.** `render_entry` (`:334-349`) +hardcodes `BLANK_MARK` in column 0. `paint` (`:369-372`) is a wholesale +`buf:replace` with `bypass_intercept = true`; the read-only intercept +(`:509-511`) rejects everything else. **Both of those are what the +`generated-buffer-immutability` lane replaces** — rev 5 proposed to do +it here and rev 6 withdrew that (§3.1, R1); Stage 2 leaves `paint` and +the intercept exactly as Stage 1 built them. `seat_cursor` (`:405-416`) re-seats by basename and +carries the warning that `move_to_line` is **ambient** — every +post-`await` seat must first check `pmacs.window.buffer()`. +`entry_at_cursor` (`:381-385`) maps cursor line *n* to `entries[n]`, +returning nil on the header and the footer. Keys are mode-scoped through +one helper (`bind`, `:933-935`, with the nine bindings at `:937-946`), +and `pmacs.dired._layout` (`:954-961`) exports the column contract. +`tests/dired_acceptance.rs` carries **25** tests. + +One stale comment to fix in passing: `:718` still says an uncaught raise +lands "in \*errors\*", which #161's COHERENCE finding falsified and +which the module doc at `:56-72` already corrects. Same file, two +answers. *(It was `:665` in rev 4; #182 moved it, and it is still +wrong.)* + +### The generated-buffer write invariant, which dired has not adopted (N1) + +`Buffer::set_generated_contents` (`src/buffer.rs:545-556`) is, per +`docs/agent-handoff.md` §4, **the one authorized write** for a generated +buffer. It does four things as a unit: clear `read_only`, apply one +whole-buffer `EditOp::Replace` through `apply_edit_skip_intercepts`, +`clear_history()`, re-assert `read_only` — and **return the `Edit`**. + +The three traps the handoff attaches to it, each verified here: + +- **An intercept is not read-only.** `Buffer::undo` reaches the rope via + `ensure_writable` (`src/buffer.rs:568-577`), which consults only the + `read_only` flag and never the intercept chain. `M-x buffer.undo` is a + defined command (`builtin/commands/default.lua:179`) reachable from + M-x with no binding at all, so rebinding `C-/` buffer-locally would + not close it. **Dired sets no `read_only`** — `claim_handle` installs + an intercept and `set_round_trip_input`, nothing more + (`dired.lua:506-519`) — so a dired listing is emptiable today. +- **A rope write is only half an edit.** The `Edit` must be fanned out + or a displaying window keeps a `TextView` line index describing the + previous contents. The Lua binding already discharges this: + `pmacs.buffer.set_generated_contents` + (`src/lua_bindings/mod.rs:3079-3095`) releases the registry borrow and + then calls `notify_buffer_edit_to_windows` (`:3092`). **So a Lua + caller inherits the fan-out for free** — this is not something + `dired.lua` has to arrange, and the comment at `:3088-3091` says why + the borrow is dropped first. +- **"Discard history" means whichever history exists.** `clear_history` + (`src/buffer.rs:559-566`) clears the v0.1 `undo`/`redo` stacks **and**, + under `#[cfg(feature = "crdt")]`, calls `crdt.clear_undo_history()` — + because CRDT mode bypasses the v0.1 stacks entirely. Consequence for + §14: whichever lane adopts the primitive, the `crdt`-featured run is + the only one in which that half is live — a default-feature run + exercises the v0.1 stacks alone. + +There is deliberately **no** Lua `set_read_only` +(`src/lua_bindings/mod.rs:3074-3078` states the reason: it would let a +caller lock a buffer with no way to refresh it). Pairing the lock with +the write **is** the primitive, which is why adoption is a swap of +`paint`, not an addition to it. + +And it does **not** replace `set_round_trip_input`. The handoff is +explicit that the protection is layered across two copies: rope-level +`read_only` refuses the op at the daemon, while round-trip input stops a +semantic frontend applying optimistically to its **own mirror**, which a +daemon-side refusal arrives too late to prevent. `dired.lua:516` stays. + +### How a directory reaches dired, after #182 (N2) + +Rev 4 predates Journey Stage 1a. Three facts a Stage 2 implementer will +otherwise get wrong: + +- **`resolve_target_buffer` resolves a directory before it loads.** The + `ResolvedTarget::Directory { path }` arm is first + (`src/editor_core.rs:964-970`), ahead of `get_or_load_buffer`, and its + `path` is **normalized** — the type's own doc says so + (`:118-130`) and warns that this is not free, because normalization + otherwise happens inside `set_buffer_path` (`:889-890`), which never + runs on this arm. **A handler keying state by path gets the canonical + form**, which is what makes dired's `handle_for_path` dedup agree with + it. Stage 2's reconciliation must normalize on the same seam or it + will miss dired handles. +- **Dired is a replaceable fallback slot, not a hook subscriber.** The + chain is the `path.open-directory` hook (`builtin/hooks/default.lua:65`), + and dired registers through `pmacs.path.set_directory_handler` + (`dired.lua:736-738`; binding at `src/lua_bindings/mod.rs:3679`, + readable back as `pmacs.path.directory_handler`, `:3677`). The + module's own comment states the reason: `HookRegistry::add` only + appends and builtins load before `init.lua`, so a dired subscription + would always claim first and no user listener could ever win. + **Setting the slot to `nil` disables directory opening entirely**, and + `journey_unclaimed_directory_starts_successfully_with_a_status` pins + that this is a status message rather than a failure. Nothing in Stage + 2 may assume dired is what opened a directory. +- **`open_directory` now commits under a captured destination, and that + scope refuses an `await`.** The mutating half of `open_directory` is a + `commit()` closure (`dired.lua:656-693`) run through + `pmacs.window.commit_to` when `opts.dest` is present (`:703-707`); the + `read_listing` await is deliberately **outside** it, and the comment + at `:653-655` records why: awaiting inside a commit is refused + (Q#JR14b), because a yield would restore the scope while the coroutine + is still parked. `journey_acceptance.rs`'s + `commit_to_refuses_an_await_and_restores` pins it. **§9's + serialize-and-await batch therefore cannot run inside a commit + scope** — see §9. + +### The journey ratchet (N3) + +`tests/journey_acceptance.rs` — **24** `#[test]`s, 0 `#[ignore]`, 0 +feature-gated, 2 under `#[cfg(target_os = "linux")]`. Its module doc at +`:12` is the rule: *"**This file is a ratchet: stages add rows, none +removes them.**"* Two further disciplines it states, both of which bind +Stage 2's own acceptance: + +- **Drive the real entry point** (`:16-19`): "a directory arm with no + production caller passes every direct-call test". The suite goes + through `EditorState::open` / `open_directory_target` + (`src/editor.rs:999`), never `resolve_target_buffer`. +- **Pump to quiescence, never to a frame count** (`:20-22`), because + every listing is worker-dispatched. Stage 2's batches are more + worker-dispatched still. + +Seven rows assert on dired directly, including that a failed listing +leaves **no** `*dired:` buffer behind, that a declining resolver chain +still falls back to dired, and that `q` returns to the *destination* +window's origin buffer. **The GPU row is in a different file** — +`tests/gpu_invocation_acceptance.rs`'s +`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`, +added by #183 through a real daemon child process. Stage 2 must keep +both green and must not remove a row from either (§14). + +### The typed-edit chain, and why Stage 2 stays outside it (N4) + +Dired participates in neither `pmacs.typed_edit` nor +`buffer.after-edit`: a grep of `dired.lua` for +`typed_edit|after-edit|after_edit` returns **0**, and it registers no +`pmacs.hook.add` at all. Nor does the generated-buffer write drag it in +— `set_generated_contents`'s binding runs `notify_buffer_edit_to_windows` +and nothing else, and `EditorCore::notify_buffer_edit` +(`src/editor_core.rs:1814-1828`) runs no Lua hook. **So adopting the +generated-buffer primitive would not put dired writes on the chain +either** — recorded for the lane that will do it (§3.1), since it is the +obvious worry about N1 and N4 interacting and the answer is no. + +What *does* inherit from the chain is §5's two new hooks, because they +are new fan-outs. The relevant facts: + +- `resource.renamed` / `resource.deleted` will be declared in + `builtin/hooks/default.lua`. The existing edit hook there is + `all-must-succeed`, and **`run_all_must_succeed` (`src/hook.rs:332`) + iterates every callback and collects errors — it does not abort the + fan-out.** A reconciliation subscriber may therefore not rely on a + raising peer to stop the sequence, and §5's ordered LSP teardown must + be internally ordered rather than ordered-by-registration. +- **A chain consumer cannot both edit and let a later consumer act + meaningfully**, because the record handed to consumers is a copy taken + before any ran (`builtin/runtime/typed_edit.lua:132`, `:145-159`). + The analogue for §5 is direct: `resource.renamed` carries **paths**, + not a rebind list, and a subscriber that re-reads editor state sees + whatever earlier subscribers did. §5's LSP and dired subscribers are + independent by construction — one touches URI-keyed stores, the other + touches handles — and this framing asserts that independence rather + than assuming it. +- **`buffer.after-edit` fan-outs nest** (`builtin/runtime/lean_input.lua:232-241`). + Stage 2 fires no `buffer.after-edit`, so this does not bind directly; + it is recorded because the reconciliation runs inside + `pmacs._async._tick`, and a subscriber that edits a buffer there is + one `pmacs.hook.run` away from re-entering a fan-out it is inside. + +--- + +## 3. Marks (Q#DR12, continuing the parent's Q#DR4) + +`handle.marks` is a table **keyed by basename**, valued by the mark +character: `{ ["foo.txt"] = "*", ["old/"] = "D" }`. Two characters only, +per the parent: `*` (general, consumed by operations) and `D` (deletion +flag, consumed by `x`). + +Basename keying is the parent's decision and it is right for the reason +it gives — a sort or a revert reorders lines, so a line-indexed set +retargets onto a different file. Three consequences the parent does not +draw: + +- **Marks are per-directory-buffer, which falls out for free.** The mark + table lives on the handle, and there is one handle per directory + (Q#DR2). Nothing to decide, but it is why a basename key is + sufficient — the directory is implied by the table it is in. +- **Marks must be pruned on every re-read**, following + `*buffer-list*`'s `:444-452`. A vanished name's mark is dropped + *before* it can be counted by the next operation, so the mark set can + never name something the listing does not. +- **`render_entry` becomes mark-aware**, which means it needs the mark + for the entry it is rendering. It currently takes only `entry`; it + grows a second parameter rather than reaching for the handle, so it + stays a pure function of its inputs and the acceptance can call it + directly. + +`t` inverts only `*` marks and leaves `D` flags alone — Emacs's +behavior, and the alternative silently converts flags into marks. +`U` clears both. + +### 3.1 The listing's write path — deferred to the generated-buffer lane (Q#DR25, withdrawn in rev 6) + +Rev 5 decided here that dired's `paint` would adopt +`pmacs.buffer.set_generated_contents`, dropping the erroring intercept. +**Round 5 withdrew that decision from this document** (R1), and the +reason is worth stating precisely because it changes what the fix *is*, +not merely who does it. + +The defect is real and rev 5 characterised it correctly: dired sets no +rope-level `read_only` — `claim_handle` installs an intercept and +`set_round_trip_input`, nothing more (`dired.lua:506-519`) — while +`Buffer::undo` reaches the rope through `ensure_writable` +(`src/buffer.rs:568-577`) without consulting the intercept chain, and +`M-x buffer.undo` (`builtin/commands/default.lua:179`) is reachable from +M-x with no binding. So a dired listing is emptiable today. + +**What rev 5 got wrong is the scope.** It framed this as dired's +quarter of a four-writer inventory. It is not a quarter of anything: the +same idiom — erroring intercept, `bypass_intercept` write, writable rope +— is in `listview.lua`, `compile.lua` and the search/grep panel, and +**no Lua caller anywhere sets `read_only`**, because there is +deliberately no Lua `set_read_only` to call +(`src/lua_bindings/mod.rs:3074-3078` says why). One idiom, one missing +capability, four instances. Fixing dired's instance inside a dired stage +would have produced a fourth bespoke adoption and left the shared +question — what a Lua-owned generated buffer is *supposed* to do — +unanswered for the fourth time. + +**It is therefore owned by the `generated-buffer-immutability` lane**, +whose framing is in progress. Stage 2 does not implement it, does not +gate on it, and carries no acceptance for it. + +**What that lane inherits from this one**, recorded here so the re-scout +is not repeated: + +- The three obligations the primitive carries, all verified at + `6bee09d`: an intercept is not `read_only`; the returned `Edit` **must** + be fanned out (the Lua binding already does it, + `src/lua_bindings/mod.rs:3092`, releasing the registry borrow first); + and `clear_history` (`src/buffer.rs:559-566`) must clear **whichever** + history exists, since CRDT mode bypasses the v0.1 stacks and keeps its + own in loro's `UndoManager` — so any acceptance for this must run + under **both** default and `crdt` features or it exercises one half. +- Adoption does **not** replace `set_round_trip_input`. The protections + cover different copies: rope-level `read_only` refuses the op at the + daemon, while round-trip input stops a semantic frontend applying + optimistically to its **own mirror**, which a daemon-side refusal + arrives too late to prevent. `dired.lua:516` stays either way. +- **A trap for that lane's acceptance.** + `tests/dired_acceptance.rs:969`'s + `dired_buffer_is_read_only_and_round_trips_input` asserts + `status(&s).contains("read-only")`, and `BufferError::ReadOnly` + renders as ``buffer `{name}` (id {id:?}) is read-only`` + (`src/buffer.rs:1794`). **That test passes both before and after the + swap**, so it is not coverage of the adoption and must not be counted + as such. The pin with bite is `M-x buffer.undo` leaving the listing + intact, which fails against `main` today. + +**What Stage 2 owes it: nothing but non-interference.** Stage 2 changes +`paint`'s *callers* (every mark and batch repaints) but not `paint` +itself, so the two lanes touch `dired.lua` in different places. If the +immutability lane lands first, Stage 2b rebases onto a `paint` that +already writes through the primitive and needs no change; if Stage 2b +lands first, the lane adopts a `paint` with more callers and still needs +no change to them. **Neither ordering creates a conflict**, which is why +this is a deferral rather than a dependency. + +## 4. Target sets: which operations are set-based (Q#DR13) + +Rev 1 stated one rule with one exception and then contradicted it (F4). +Operations fall into **three** classes, and the class is a property of +the command, not a special case: + +**Set-based** — `D` (delete), `M` (chmod), `w` (copy filename), and `C` +(copy, 2c) target: + +> **the marked set, or — if nothing is marked — the entry at point.** + +Emacs's rule, and it is why none of them needs a separate at-point +binding. + +**Flag-based** — `x` alone. It consumes **`D` flags only**, never `*` +marks, and **never falls back to point**: with nothing flagged, `x` does +nothing and says so. A `d`-then-`x` sequence and an `m`-then-`D` sequence +are different gestures, and collapsing them makes `x` unpredictable after +a stray `m`. + +**Point-based** — `R` (rename) **alone**. It acts on the entry at point +**regardless of what is marked**, and leaves marks untouched. This is a +real limitation, not a preference: renaming a *set* means renaming into a +target directory, which needs a concept Stage 2 does not build (§11). +Naming it here is the point — rev 1 left it as an unstated exception to a +rule that claimed to have only one. + +*(Rev 2 also put `w` here. Round 2 was right that this was an unapproved +narrowing: the parent approved the `w` **binding**, and Emacs copies the +marked filenames when marks exist. `w` is set-based in rev 3.)* + +A basename in a set that has vanished from disk since it was marked is +**dropped from the batch and reported**, not silently skipped and not +fatal to the rest — the parent framing's §6 rule, kept. Note this is a +distinct event from a revert pruning the mark (§3): a target can vanish +*between* the last revert and the operation, and §13 item 40 pins that +the batch reports it rather than silently shrinking. + +--- + +## 5. Rename reconciliation (Q#DR14, superseding Q#DR5's rebind) + +The parent's decision — fix it at the primitive, in the main-thread +drain, unconditionally on success — is kept. Rev 1's *scope* was wrong: +it updated `Buffer.file_path` and nothing else, leaving four other owners +of the same path stale (F1). A rename is not a buffer-field update; it is +a **transaction across every consumer that holds the path**. + +### The owners, all verified + +Rev 4 listed five. The re-scout confirms all five and finds a **sixth** +(W5); two further owners are *persisted* rather than in-memory and are +named in §11 rather than fixed here. + +| # | Owner | Held as | Stale after a rev-1 rebind | +|---|---|---|---| +| 1 | Buffer path | `Buffer.file_path` | fixed | +| 2 | Buffer **name** | `Buffer.name` | **yes** — `set_buffer_path` never calls `set_name`, which documents itself as for "save-as and rename operations" (`buffer.rs:453`). Statusline and buffer list keep the old filename | +| 3 | LSP attachment | `rec.uri`, cached per **buffer** in the `attachments` table (`lsp.lua:340`, record built at `:861-868`) | **yes** — read at **57 lines** in `lsp.lua` (W4), among them didChange (`:418`), semantic tokens (`:799-813`), the diagnostic attach (`:1039`), signature (`:1102-1108`), definition (`:1922-1937`), references (`:2031-2047`), all firing at the old URI. Buffer-keyed, so **one** rebind reaches all 57 | +| 4 | dired handles | `handle.path` in Lua; the buffers are **pathless** | **yes, and unreachable** — no buffer-keyed rebind can ever find them | +| 5 | Workspace-edit origin | `origin = active_buffer_path()` (`lsp.lua:1338`) — a **string** (`:471-473`) | **yes, and it materializes a phantom** — `find_or_open(origin)` (`:1352`) on a renamed-away path hits `resolve_target_buffer`'s `NotFound` arm, which creates an empty path-backed buffer (`editor_core.rs:980-989`) and selects it. **No transaction can fix this one**, because the stale value is a captured Lua local, not editor state (G1) | +| 6 | **Lean file progress** (NEW, W5) | `M.file_progress[uri]`, a **URI-keyed Lua module table** (`lean.lua:646`, written `:651`) | **yes, and `forget_uri` cannot reach it** — it is in no Rust store. Populated from `$/lean/fileProgress`; its own comment says Stage 5's goal view reads it to tell "no goals" from "not done yet". Reachable only from a **`resource.renamed` Lua subscriber**, which is an independent argument for the hook | + +**Owner 6 is the load-bearing addition, and not because Lean matters +here.** It is evidence that the Rust-side `forget_uri` (below) 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. The +hook is not a convenience for dired — it is the only mechanism that +scales, and owner 6 is the first case that proves it outside dired. +`lean.lua`'s subscriber is **not** in Stage 2's scope (Lean's arc owns +it); what Stage 2 owes is a hook whose contract makes writing one +possible, which is why the hook carries `(old, new)` paths. + +### One transaction, two callers, one notification + +**`EditorCore::reconcile_rename(old, new) -> Vec`**, where +`RenameRebind { buffer_id, old_path, new_path }`: + +- walks the **whole** registry, not `find_by_path`'s first match — a + directory rename has many affected buffers by construction, and two + buffers can visit one path; +- matches normalized stored paths against normalized `old` by **equality + or path-component prefix** (`/foo` must not match `/foobar`); +- sets the new path **and** sets the name, but only when the buffer's + explicit name-origin state is **path-derived**, by the provenance + rule below; +- returns every rebind it performed. + +**The name-provenance rule (Q#DR30, rewritten in rev 9 — F5).** Rev 7 +updated the name only when it **equalled** the normalized old path, +assuming path-backed buffers carry full-path names. **They do not.** +`get_or_load_buffer` (`src/editor_core.rs:917`) sets +`display_name = path.display().to_string()` — the path **as given** — +and normalizes only the stored `file_path`; `pmacs.buffer.from_file` +(`src/lua_bindings/mod.rs:3112`) passes the raw string the same way. So +`pmacs foo.rs` yields name `foo.rs`, path `/abs/dir/foo.rs`, and rev 7's +rule would have left that name stale while insisting it was +user-chosen. + +Rev 8 replaced string equality with normalized path-equivalence. That +fixed relative opens but still was not provenance: for a file +`${cwd}/notes`, a user-chosen name `notes` normalizes to the file's path +and would be overwritten. **Rev 9 records the fact instead of inferring +it from the string:** + +```rust +enum BufferNameOrigin { + Explicit, + PathDerived, +} +``` + +The field is private buffer state and is set only through two explicit +doors: + +- `Buffer::new`, `Buffer::from_bytes`, and ordinary `Buffer::set_name` + create/set an **`Explicit`** name. The new + `pmacs.buffer.set_name` binding goes through this door: it is a user + naming operation even when its string happens to denote the file. +- A dedicated `set_path_derived_name` writes the name and marks it + **`PathDerived`**. Every path-backed creation site is audited to use + it: `EditorCore::get_or_load_buffer`, the `NotFound` arm of + `resolve_target_buffer`, `pmacs.buffer.from_file`, and + `pmacs.buffer.find_or_open`. Save-as/visit sites that establish a path + must use the same door. Reconciliation uses it again after a rename, + so a second rename still follows. + +`reconcile_rename` tests only `BufferNameOrigin`; it does not parse or +normalize the display name. A relative open therefore follows because +its creator recorded path provenance, while a user who explicitly names +`${cwd}/notes` as `notes` keeps that name. §13 item 29 uses exactly that +collision rather than a custom string that happens not to resemble the +path. + +**One consequence, stated rather than hidden:** when the rule fires the +new name is written as the **normalized new path**, so a buffer opened +by a relative path acquires an absolute name after a rename. Preserving +the relative rendering would require knowing *which base the name was +relative to*, which no buffer records. The effect is confined to the +statusline and the buffer list. The origin bit records only whether the +name may follow; it deliberately does not attempt to preserve the +relative spelling or its base. + +**Both rename paths call it**: the async harvest below, and +`apply_resource_op`'s rename arm (`mod.rs:3234-3255`), whose raw +first-match lookup at `:3249` is deleted in favour of it. One function, +two callers — so the two can no longer drift, which is how the trap +survived being "fixed" once already. + +**Then one hook, fired once per rename: +`resource.renamed(old_path, new_path)`.** The mechanism exists — +`run_hook_if_defined` (`mod.rs:1623`) fires Rust-side, `pmacs.hook.run` +(`:6005`) fires Lua-side — but **no rename or delete hook exists today**; +the whole set is `buffer.{after-edit,after-load,after-save,after-switch, +before-save,save,self-insert}`, `editor.before-quit`, +`frontend.detached`, `process.after-tick`. Stage 2 adds this one. + +The hook carries the **paths**, not the rebind list, precisely because +dired's buffers are pathless: a path-keyed consumer must be able to +reconcile from `(old, new)` alone. + +#### The LSP subscriber, in full (G3, corrected H2) + +Rev 2 said "recompute `rec.uri` and didClose/didOpen". Rev 3 added five +stores. Both were sized from a partial inventory: `LspManager` +(`src/lsp.rs:741-830`) holds **fourteen** URI-bearing store families, and +two more things keyed the same way. + +**The complete inventory**, from the struct itself: + +| # | Field | Keyed by | +|---|---|---| +| 1 | `diag_store` | uri | +| 2 | `completion_store` | (server, uri) | +| 3 | `hover_store` | (server, uri) | +| 4 | `signature_store` | (server, uri) | +| 5 | `definition_store` | (server, uri) | +| 6 | `locations_store` | (server, uri, **kind**) — references / declaration / typeDefinition / implementation | +| 7 | `symbol_store` | **scope**-keyed — documentSymbol *and* workspace symbol | +| 8 | `document_highlight_store` | (server, uri) | +| 9 | `formatting_store` | (server, uri) | +| 10 | `rename_store` | (server, uri) | +| 11 | `prepare_rename_store` | (server, uri) | +| 12 | `code_action_store` | (server, uri) | +| 13 | `inlay_hint_store` | (server, uri) | +| 14 | `semantic_token_store` | (server, uri) — plus the retained raw int stream and `result_id` | + +Plus **`documents: HashMap<(LspServerId, String), String>`** (`:810`), the +latest full text per `(server, uri)` — which is what `didChange` diffs +against, so a stale entry under the old URI is a *correctness* problem, +not just a leak. And **`pending_routes`** (`:797`), whose `ResponseRoute` +variants carry the URI. + +**The route arithmetic, corrected (W3).** `ResponseRoute` +(`src/lsp.rs:835-859`) has **15 variants**, of which **14 carry a +`uri`**: `Completion`, `Hover`, `Signature`, `Definition`, `Formatting`, +`Rename`, `PrepareRename`, `CodeAction`, `InlayHint`, `SemanticTokens`, +`SemanticTokensDelta`, `Locations { uri, kind }`, `DocumentSymbol`, +`DocumentHighlight`. The fifteenth is **`WorkspaceSymbol { query }`**, +which carries **no URI at all** — its own comment explains that the +query stands in for the doc URI in the supersede key +(`src/lsp.rs:1887-1888`). There are **16** insert sites, every one in a +`request_*` method, spanning `src/lsp.rs:1683-2132`; **15 of the 16 +insert a URI-bearing route**, the two `SemanticTokens` inserts coming +from `request_semantic_tokens` (`:2073`) and +`request_semantic_tokens_range` (`:2108`), and the sixteenth being +`request_workspace_symbol` (`:1889`). *Rev 4 said "fifteen insert +sites"; the fifteen was a correct count of the URI-bearing ones and an +incorrect count of the sites.* **The purge predicate must therefore +answer for a variant with no `uri` field** — `WorkspaceSymbol` is +retained unconditionally, because a workspace-symbol query is not scoped +to any document and a rename does not invalidate it. + +**And that is why "clear the stores" is not enough** (H2): a response +already in flight when the rename happens routes on arrival and +**repopulates the old key after the clear**. Clearing without purging +in-flight routes is a race that reintroduces exactly the state it removed. + +**One manager-level method, `forget_uri(sid, uri)`**, doing all of it — +because fourteen call sites at the Lua layer is how one gets forgotten. + +**And it needs a Lua binding, which rev 7 never named** (rev 8, P2). +The subscriber that calls this lives in `lsp.lua`, and Lua reaches only +explicit bindings. The surface is **`pmacs.lsp.forget_uri(server_id, +uri)`**, modelled on `pmacs.lsp.forget` (`src/lua_bindings/mod.rs:10257`) +— a closure over the shared manager that calls through and maps the +error with `mlua::Error::external`. Its **error contract**: + +- **Raises** on an unknown `server_id`, matching `forget`'s existing + behaviour for the same input. +- **Succeeds silently** when the URI has no state under that server. + The subscriber runs per attachment, but an attachment need not have + any pending route or populated result store, and cleanup can be + repeated after an earlier partial teardown. "Nothing to forget" is + therefore an idempotent success, not an error. +- Takes the **old** URI. Calling it after `didOpen` of the new URI is + therefore safe and order-independent with respect to step 5. + +Pinned by §13 item 31c, which asserts both arms — the raise and the +silent success. +Its shape is modelled on the **server-scoped** teardowns that already +exist. **There are two of them, and rev 4 named neither correctly (W2):** +`LspManager::start_generation` (`src/lsp.rs:1307-1345`, the restart- +generation flip) and `LspManager::forget` (`src/lsp.rs:3015-3042`, the +terminal-state removal). There is no `fn restart`. Both do the same +teardown work one axis over; `forget_uri` does four things in this +order: + +1. **Record `(sid, uri)` in `forgotten_documents` before clearing + anything.** Main-thread execution makes the following steps 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. **Purge `pending_routes`** whose route carries this URI — + `retain`, mirroring `start_generation`'s `:1324` + `retain(|(sid, _), _| *sid != id)` and `forget`'s `:3028`. + Per W3, the predicate must retain `WorkspaceSymbol` explicitly. +3. **Drain-cancel their awaiters — and this is where rev 4 pointed at + the wrong function (W1).** `pending_external` (`:804`) holds the + `Handle:await()` side, and the contract at `:801-803` is explicit + that it is *"drained-cancelled wherever `pending_routes` is purged"*. + The sweep to model on is **`drain_external_cancelled` + (`src/lsp.rs:1561-1576`)** — server-scoped and **unconditional**, + settling every awaiter for `sid` cancelled. It is **not** + `drain_cancelled_externals` (`src/lsp.rs:1596-1645`), which despite + the near-identical name is a per-tick, per-awaiter sweep that removes + only awaiters whose `CancellationToken` was flipped or which outlived + `request_timeout`. **A rename flips no token**, so modelling on the + second yields a drain that silently does nothing and leaves any + coroutine awaiting against the old URI parked forever. + Note that **neither existing sweep is URI-scoped** — both range over + `sid`. `forget_uri` needs a new shape: collect the `rid`s whose + `pending_routes` entry carries the old URI, then settle + `pending_external[(sid, rid)]`'s awaiters cancelled via + `runtime.complete_external_cancelled`, exactly as + `drain_external_cancelled` does per key. The route→awaiter join is + the `rid`; there is no other index. +4. **Clear all fourteen stores plus `documents`** for the old key. Each + store already has a keyed `clear` (`diag.rs:262`, `hover.rs:160`, + `completion.rs:331`, `semantic_tokens.rs:263`, …). Note the two + irregular keys: `locations_store` is **kind**-keyed, so all four kinds + must go; `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 in step 1. + +#### The uncorrelated resurrection paths, and the tombstone that gates them (rev 9, F3/F4) + +`forget_uri`'s route purge and awaiter drain operate on +`pending_routes` — the **correlated** path, where a response is matched +to a request id — and its store clear handles only state already +present. Neither guards a later writer that never goes near a request +route. + +**The census, re-run with "uncorrelated writers" as the lens.** Counted +over production code only, with `#[cfg(test)]` boundaries read per file +rather than inferred from filenames. **41 writes total**: 16 correlated, +19 uncorrelated store writes, 6 `documents` writes. + +- **Correlated: 16**, every one inside + `LspManager::absorb_routed_response` (`src/lsp.rs:2667`), which has + exactly one caller — `handle_response` (`:2523`) behind + `pending_routes.remove(&(sid, rid))` at `:2627`. These are + purged by construction if the route drain runs first. +- **Uncorrelated to a request id: 19** = **1** server-initiated + notification + **3** `mark_document_stale` writes + **15** + Lua-callable `clear` bindings. Only the first four create result/stale + state and can resurrect a forgotten URI; the synchronous `clear` + bindings remove result data. + +**Four findings that change the design, not just the prose:** + +1. **`diag_store` has ZERO correlated writers.** The 16 correlated + writes cover 13 of the 14 stores; diagnostics are populated *only* + by the uncorrelated path. So the one store `forget_uri` most needs + to protect is the one its route purge cannot help at all. +2. **`publishDiagnostics` is the only uncorrelated *notification* + writer — but not the only uncorrelated writer.** `handle_notification` + (`src/lsp.rs:2878`) is 22 lines, matches exactly one method string, + and has no default arm; everything else reaches only `push_event`, + which writes the event queue and status tracker and no URI-keyed + store. So among notifications it is **1 of 1**. But + **`pub fn mark_document_stale(&self, uri)` (`src/lsp.rs:3108`)** is a + second uncorrelated writer: today it takes `&self` and **no + `LspServerId`**, marks `stale_uris` in three stores **for every + server at once**, *creates* URI keys, and is exposed to Lua as + `pmacs.lsp._mark_document_stale` (`src/lua_bindings/mod.rs:9742`). + Rev 9 changes both surfaces to `(sid, uri)`, because every caller + already owns the attachment/server id and the tombstone cannot be + exact without it. + **An earlier pass of this framing checked only `handle_notification` + and concluded "publishDiagnostics is the only one in Rust". That was + the wrong lens boundary and the answer was wrong** — recorded because + it is the round's own defect class caught inside the round. +3. **`DiagnosticStore.by_uri` is keyed by URI *alone*** (`src/diag.rs:198`), + with **no `LspServerId` component**, unlike the other thirteen which + key on `(server, uri)`. Rev 8 drew the wrong conclusion from that + fact: the tombstone does **not** live in `DiagnosticStore` and need + not share its key. `absorb_publish_diagnostics(sid, params)` has the + server id before it calls `guard.set(uri, parsed)`, so a manager-owned + `(sid, uri)` gate protects the URI-only write while retaining the + provenance needed for selective teardown. +4. **`epochs` is never pruned.** `DiagnosticStore::clear` *creates* an + `epochs` entry (`src/diag.rs:266`, `or_insert(0) += 1`), as does + `set` (`:250`), and nothing removes them. A `forget_uri` clearing + only `by_uri` leaves a URI-keyed leak behind, so the store's own + forget path must drop the epoch too. + +**And the lens reproduces owner 6 independently.** `lean.lua` writes +`M.file_progress[uri]` from +`pmacs.lsp.on_notification("$/lean/fileProgress", …)` +(`builtin/runtime/lean.lua:648,651`) through a **second, Lua-side +notification dispatch table** (`dispatch_notification`, +`builtin/runtime/lsp.lua:1719`, driven from `:1867`). It is +uncorrelated by the same mechanism, `forget_uri` cannot reach it, and it +is exactly §5's sixth path owner. The census arriving at it from a +different direction is the strongest evidence yet for §5's conclusion: +**the Rust method handles what it can see, and the hook is the +mechanism that scales.** + +**The gate: one manager-owned, generation-scoped exact-pair tombstone.** +`LspManager` gains +`forgotten_documents: HashSet<(LspServerId, String)>`. +`forget_uri(sid, uri)` inserts the exact pair; +`absorb_publish_diagnostics(sid, params)` drops a matching pair before +touching the URI-only diagnostic store; and +`mark_document_stale(sid, uri)` returns before touching any of its three +stores when the same pair is present. The private Lua surface changes +to `pmacs.lsp._mark_document_stale(server_id, uri)`, and its sole +production call in `lsp.lua` passes `rec.server, rec.uri`. + +The tombstone clears **for that exact pair** when +`did_open(sid, uri)` reopens it. `start_generation(sid, …)` and +`forget(sid)` remove all pairs for `sid` and retain every other server's +pairs. This is **reclaimed and generation-scoped, not size-bounded**: a +live generation can accumulate distinct forgotten URIs, and imposing a +capacity/LRU eviction would allow an arbitrarily late notification to +resurrect an evicted key. Per finding 4, the diagnostic forget path is +a distinct store method that removes `by_uri`, `severity_counts`, +`stale_uris`, **and `epochs`**; it does not call today's `clear`, which +increments the epoch it is meant to forget. + +**Why not the cheaper membership gate.** The tempting version needs no +new state: absorb only if `(sid, uri)` is in `documents`. **It would be +a behaviour regression** — servers legitimately publish diagnostics for +files the editor never opened, a crate-wide push naming a dependency, +and a membership gate drops every one. The tombstone drops **only what +this editor explicitly forgot**. There is precedent for exactly this +shape: `handle_response` already drops late arrivals via +`client.cancelled_rids.remove(&rid)` (`src/lsp.rs:2549`) — a +"forget-then-drop-late-arrivals" set. The tombstone is that pattern with +a `(server, URI)` key instead of a request id. + +**Why the existing epoch cannot serve.** `epochs` is bumped +unconditionally by `set` with no epoch parameter and is read only by +`epoch_for` for render-cache invalidation (`src/semantic_render.rs:2107`). +Nothing compares an incoming write's epoch to a stored one, so a late +notification simply bumps it and looks current. `stale_uris` is likewise +a one-bit render-suppression flag that gates the **read** path only. And +`LspClient::attempt` (`:3009`) is a real generation counter but +**per-server**, useless for a closed URI on a live server. **There is no +tombstone in the tree today** — zero occurrences — so this is new state, +and the framing says so rather than implying it reuses something. + +**Verified absence of any existing guard**, stated with evidence rather +than as a claim: `grep -rn "documents.contains_key" src/` returns +**zero**, and the only `documents` membership test anywhere is +`inbound_converted` (`src/lsp.rs:1498`), which on a miss returns the +value **unconverted and proceeds** rather than dropping it. +`absorb_publish_diagnostics` (`:2900`) guards only that `uri` is a +string and `diagnostics` is an array, then writes. + +**Residue, stated:** a server that legitimately publishes for the old +path *after* we renamed away from it is dropped until the tombstone +clears — the conservative direction, and the same trade `forget_uri` +already makes for the correlated stores. Two further holes are named, +not closed, because they are wider than this lane (§11): the fourteen +`pub fn *_store()` accessors hand out `Arc>` clones conferring +unrestricted write access, and the `workspace/*/refresh` handlers +re-pull by iterating `attachments` with no cross-check against +`documents`, so a purge can be followed by a fresh, correlated request +for a URI the editor no longer holds. + +**Worth stating because it is surprising, and it is now true twice:** +*neither* server-scoped teardown clears the fourteen result stores. +`start_generation` clears `deferred_notifications`, `pending_routes`, +`documents`, and drains externals; `forget` clears `pending_routes`, +`documents`, externals, `status_tracker`, and `project_servers`. Rev 5 +does not change that (it is a separate pre-existing question about +whether stale results should survive a restart), but it does mean +`forget_uri` has **no** precedent to copy for the store half, only for +the route/document/drain half. + +Then the ordered sequence, per attachment: + +1. **Flush any pending `didChange`** for the old URI, so the server is not + left with an edit it can no longer attribute. +2. **`didClose` the old URI** — which removes the open-document + registration and nothing else. +3. **`forget_uri(sid, old)`** — the purge/drain/clear above. +4. **Re-run `ensure_server`.** Since #161 affinity keys on the detected + project root, a rename *across roots* needs a **different server**; + same-root renames reuse. When the server changes, step 3 runs against + the **old** server and `didOpen` goes to the new one. +5. **`didOpen` the new URI** with the buffer's current text and a fresh + version. +6. **Re-root the diagnostic views** (below). + +#### Re-rooting the diagnostic views: the seam, chosen (H3) + +`DiagnosticView.uri` is **set once at construction** and the field's own +doc anticipates exactly this: *"M5 may add re-rooting if a buffer is +renamed"* (`src/diag.rs:456-457`). Rev 3 left it as "either a `set_uri` or +tear down and re-attach", which round 3 rightly rejected: neither option +explained how **passive** windows are reached, and the field is private +while `View` has **no downcast**, so an outside caller cannot reach it at +all. Two further facts make the naive options worse: + +- **`_attach_view` reaches only the active window.** It takes + `core_borrow.active_window_mut()` and **errors** if that window is not + showing the buffer (`lua_bindings/diag.rs:218-224`). So a + re-attach-per-window loop cannot be driven from Lua, and a passive split + never got its overlay from this path in the first place. +- **A remove-and-re-push loses composition order.** Overlays are an + ordered `Vec>` (`window.rs:365`) merged in sequence, so + re-pushing puts the diagnostic underline at the end of the stack rather + than where it was. + +**The seam: a `View::rename_resource` hook.** `View` (`src/view.rs`) +already carries exactly this family of cross-window overlay hooks, added +by #113 round 6 for the same class of problem — `overlay_identity`, whose +own doc says it "lets disposal remove **every window copy**", and +`clone_for_split`. Rev 4 adds a third: + +```rust +/// Retarget this overlay from `old_uri` to `new_uri` after a resource +/// rename. 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. +fn rename_resource(&mut self, _old_uri: &str, _new_uri: &str) {} +``` + +`DiagnosticView` overrides it to swap its own private field when the URI +matches. The field **stays private**; no downcast is needed; and any +future URI-bearing overlay opts in by overriding the same hook instead of +growing another special case. + +**The sweep has precedent too.** Overlay disposal already walks every +window and acts on identity: + +```rust +for win in core.windows.values_mut() { + win.overlays.retain(|v| v.overlay_identity() != Some(id)); +} +``` + +(`lua_bindings/mod.rs:2044-2046`.) The rename sweep is the same traversal +with `retain` replaced by a call to `rename_resource` — so **every** window +showing the buffer is reached, active or passive, in one pass, and order is +preserved because nothing is removed or re-pushed. + +**One thing this deliberately does not fix.** A passive split that never +received a `DiagnosticView` still has none — `_attach_view`'s +active-window-only restriction is a **pre-existing** gap (`ensure_overlay` ++ `clone_for_split` cover the split-from-an-attached-window case, not the +attach-while-passive case). Renaming cannot re-root an overlay that was +never attached, and pretending otherwise would make acceptance 30 pass for +the wrong reason. Named in §11. + +Acceptance 30 is correspondingly stronger: **diagnostics present before +the rename**, **at least two windows** showing the buffer, and afterwards +only the **new** URI's diagnostics are visible and countable in *both*, +with the old URI's store empty. + +#### The dired subscriber, and the setter it needs (G2) + +`dired.lua` updates any handle whose `path` equals or is under `old`, +**renames its buffer**, and reverts it. Rev 2 wrote that without noticing +that the module's own doc says it is impossible: *"there is no +`pmacs.buffer.set_name`"* (`dired.lua:34-38`), which is precisely why +Stage 1 chose buffer-per-directory over in-place repaint. + +**Rev 3 adds `pmacs.buffer.set_name(buf, name)`** (Q#DR21). Three reasons +it is the right call over the alternative: + +- `Buffer::set_name` **already exists** in Rust and already documents + itself as "used by save-as and **rename operations**" + (`src/buffer.rs:453`). Nothing new is being invented; an existing, + purpose-built setter is being exposed. +- §5 needs the same capability anyway for the `Buffer.name` half of the + transaction, so the Rust-side name update is in scope regardless. The + Lua binding is the increment, and it is what makes the *pathless* case + reachable. +- The alternative — kill the dired buffer, recreate it under the new + name, and replace it in every window showing it — is a far larger + transaction that loses window placement, the cursor, the read-only + intercept, `set_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 `claim_handle`'s existing `<2>`-variant uniquifier before setting. +Acceptance 28 therefore asserts the **buffer name** and that +`handle_for_path` dedup finds the buffer under its new path — not merely +that `handle.path` changed. + +#### The workspace-edit applier (G1) + +`apply_workspace_edit` captures `origin` as a **string** and restores with +`find_or_open(origin)` (`lsp.lua:1338`, `:1352`). No amount of +reconciliation reaches an already-captured Lua local, so the phantom +survives every design above. The applier itself must change: + +- capture the **buffer handle** (`pmacs.window.buffer()`), not the path; +- restore with `pmacs.window.switch_buffer(origin_buf)` when the handle is + still valid; +- **no path fallback.** If the origin buffer is gone, do nothing — the + current code's fallback is what creates the phantom, and "return the + user somewhere plausible" is not worth inventing a file that does not + exist. The existing comment there already concedes the path "may have + just been renamed or deleted"; it simply drew the wrong conclusion. + +### Ordering, which is already guaranteed + +The reconciliation must complete **before any awaiting coroutine +resumes**, or a coroutine that renamed and then inspects a buffer sees +the pre-rename state. Verified: `pmacs._async.tick` calls +`async_mod._tick()` first and only then walks the settled ids firing +`on_complete` callbacks and resuming parked coroutines +(`async.lua:401-421`). So doing the work inside `_tick`'s Rust closure is +correctly ordered by construction, not by luck. + +### The harvest (F6) + +Rev 1 proposed a no-argument `take_settled_renames()` that drains "this +tick's" renames, which — as the review notes — needs either a second +queue or a scan of every settled entry. Neither was named, and both are +the one-off side channel `COHERENCE.md` §9 objects to. + +Instead, **`tick` returns a structured outcome**. Rev 5's shape carried +`settled` and `renames` only, which §6 then asked to also deliver +deletions — an impossible read (R3). Rev 6 fixes the shape: + +```rust +/// A settled filesystem mutation, with the paths the worker consumed. +pub enum ResourceOp { + Rename { from: PathBuf, to: PathBuf }, + Remove { path: PathBuf }, +} + +pub struct TickOutcome { + pub settled: Vec, + /// Successful resource mutations, in bus-arrival order. + /// This is not filesystem execution order. + pub resources: Vec, +} +``` + +**One vector, not two fields — but NOT because it is ordered** (rev 8, +F1). Rev 7 justified the single vector by claiming it preserved the +relative order of mutations settling in the same tick. **That claim was +false and is withdrawn**: `tick` (`src/async_runtime.rs:1003`) is +`while let Ok(env) = self.main.try_recv()`, a drain of the reply bus +with **no execution token**, so `resources` is in **bus-arrival order, +which is not filesystem-mutation order** — a worker can complete, be +descheduled before sending, and have a later mutation's reply arrive +first. + +The vector stays because it is one homogeneous list of settled +mutations that consumers dispatch on by kind, which keeps +`PendingJob.resource` and the outcome the same shape. **Nothing in the +reconciliation may depend on its sequence**, and §6 states the contract +that makes that safe. + +`resources` carries **only** jobs that settled `PendingState::Complete` +— a failed or cancelled mutation reconciles nothing, and fires no hook. +Its documentation must say **bus-arrival order, not execution order**, +in those words: a future consumer that reads "in settle order" and +infers causality is the exact mistake rev 7 made. +Settle identity and resource metadata still come out of **one** +transaction, from the loop at `:1074-1108` that already borrows +`pending` and reads `job.kind`. The ~17 in-crate `let _ = rt.tick();` +call sites are unaffected; `_tick` reads `.settled` for the Lua table it +already builds and `.resources` for the reconciliation. + +**`PendingJob` gains one field, `resource: Option`** — +rather than rev 5's `rename_paths` plus the separate remove-path §6 +implied. `dispatch_fs_rename` **moves** both paths into the worker +closure (`:871-879`) and `dispatch_fs_remove` (`:893`) moves its one, +so nothing retains them either way. A single enum field rather than two +`Option`s is deliberate: two would admit a both-`Some` state that cannot +occur, which is exactly the argument `ResolvedTarget`'s own doc makes +against a shape whose "three states that cannot occur" every caller +would have to re-establish by hand (`src/editor_core.rs:100-102`). +§0.5 states why carrying this on the job at all is the coherent choice +over a side map, and that a general `purpose`/`owner` should later +subsume it. + +### Additivity + +`ReplyKind` and the worker↔main wire are untouched; the new field and the +outcome struct are main-thread-only. `pmacs-protocol` is **not** involved +and does not bump — Stage 1 set that precedent by adding `ReadDir` +without one. `m8_3` **monkeypatches the Lua `pmacs.fs.rename`** +(`m8_3_acceptance.rs:182-184`), so it never reaches the Rust path and its +count must not move; the fixture's two-phase temp-name rename +(`init.lua:1197-1215`) will reconcile twice, real→temp→final, landing +correctly. + +--- + +## 6. Deleting a path something is holding (Q#DR18) + +New in rev 2 (F2); given a shared seam and a revalidation rule in rev 3 +(G4). `pmacs.fs.remove` changes disk and nothing else, so without a +policy a deleted file's buffer stays bound to a path that no longer +exists — and **saving it recreates the file the user just deleted**. + +### There are already two divergent deletion behaviors + +Rev 2 wrote dired's policy and stopped, leaving the LSP path untouched +and unmentioned. Verified: `apply_resource_op`'s delete arm +(`mod.rs:3313-3341`) deletes, then kills the buffer via +`reg.borrow().find_by_path(&pb)` — the **same raw-path, first-match** +lookup §5 fixes for rename, with **no descendant handling and no +modified check**. So an LSP-authored delete **destroys unsaved work +today**, silently, and a second buffer on the same path survives. +**That data-loss half is PR #186's to fix** (rev 7, round 6): it refuses +the delete before disk. What this lane takes from the same arm is the +lookup — descendants and duplicate path-bound buffers — and the removal +lifecycle (Q#DR27). + +### One seam + +**`EditorCore::reconcile_delete(path) -> DeleteReconcile { killed, +kept_modified, refused }`**, symmetric with `reconcile_rename`: + +- walks the **whole** registry by normalized equality **or + path-component prefix**, so descendants of a deleted directory are + included and a second buffer on one path is not missed; +- **kills unmodified** buffers, through the full two-phase lifecycle + below; +- **keeps modified ones alive** and returns them, so a caller can + report. *(Rev 7, round 6: the **synchronous** caller can no longer + produce this outcome — #186 refuses the delete before disk when any + affected buffer is modified, so `apply_resource_op` never reaches a + live file with a dirty buffer. The arm is retained for the + **asynchronous** path, which `pmacs.fs.remove` puts on a worker where + no in-applier guard can see it.)*; +- returns anything it **could not** kill separately from what it kept + deliberately — the two are different events and collapsing them makes + a failure look like a policy decision. + +#### "Kills" means two phases, and no existing Rust path does both (R4) + +Rev 5 said "kills" and stopped. Buffer removal in this substrate is two +distinct phases, and the important part is that **the only place they +are composed is a Lua binding**: + +| Phase | What it does | Where | +|---|---|---| +| 1. Core | Refuses unknown ids and **refuses to kill the last remaining buffer**; drops the id from `round_trip_buffers`; **closes** side windows showing it (Q#BP10a — closed, not redirected, or an unrelated buffer is stranded in the panel slot); redirects every other window to a fallback (`*scratch*` or any other buffer), resetting `text_view`, `cursor`, `selection`, `overlays`, `view_top` and `goal_col`; then `registry.remove` | `EditorCore::kill_buffer`, `src/editor_core.rs:4590` | +| 2. Lua-side | Clears buffer-scoped keymaps, buffer-local config, folds, and fires the registered `on_removed` callbacks | `after_buffer_removed`, `src/lua_bindings/mod.rs:1602` | + +**`pmacs.buffer.kill` composes both** (`install_buffer_kill`, +`mod.rs:5476-5491`), and its doc comment says exactly why it has to be +late-bound: it "needs an `EditorCore` handle to redirect any windows +showing the doomed buffer before removal". + +**`apply_resource_op`'s delete arm does not.** It calls +`remove_buffer_and_fire` (`mod.rs:1592`), which is `registry.remove` + +`after_buffer_removed` — **phase 2 without phase 1**. And +`BufferRegistry::remove` touches only `self.buffers` and `self.order`; +nothing in it looks at windows. So **an LSP-authored delete leaves any +window displaying that buffer pointing at a removed id.** That is a +*third* defect on that arm, alongside the missing dirty check and the +raw first-match lookup, and rev 5 named neither it nor the phase split. + +**So `reconcile_delete` composes the same pair `pmacs.buffer.kill` +composes**, for every id it kills, and `apply_resource_op`'s delete arm +is rerouted through it — which is what makes the window bug go away as a +side effect of the seam rather than as a separate patch. Note the +layering consequence: phase 2 lives in `lua_bindings` and needs `&Lua`, +so `reconcile_delete` returns the killed ids and its **caller** runs +phase 2 over them, exactly as `pmacs.buffer.kill` does. `EditorCore` does +not gain a Lua handle. + +**Failure modes, all of which a batch must survive (§9):** + +- **The last buffer cannot be killed.** `kill_buffer` returns + `"cannot kill the last remaining buffer"`. Deleting the file behind + the only open buffer therefore deletes the file and **keeps** the + buffer — reported, not silently ignored, and returned in `refused`. +- **A mid-edit buffer refuses removal — and the refusal is NOT inert + (rev 8, F2).** `BufferRegistry::remove` returns + `RegistryError::ConcurrentEdit` when `editing_in_progress()` and + leaves the *registry* untouched — but by then `kill_buffer` has + already, in this order, dropped the id from `round_trip_buffers`, + **closed** any side window showing the buffer, and redirected every + remaining window onto a fallback with `text_view`, `cursor`, + `selection`, `overlays`, `view_top` and `goal_col` all reset. Rev 7 + said to "treat a phase-1 failure as keep the buffer", which reads as + though skipping phase 2 restored something. **It does not** — it only + avoids firing `on_removed` for a buffer that still exists. + + **So `reconcile_delete` preflights `editing_in_progress` before phase + 1** and skips the buffer entirely, returning it in `refused`. The + preflight is *sound*, not merely cheap: phase 1 is entirely + `EditorCore`, which holds **no Lua handle** (C1), so nothing between + the check and `BufferRegistry::remove` can re-enter Lua and begin an + edit — the window that would make a preflight racy does not exist on + this path. #186's Q#RD2 moves the same predicate into its validation + phase for the same reason, so both lanes check it before mutating. + + A transactional kill was rejected: it needs a compensation path + snapshotting and restoring window layout, side-window collapse and + round-trip membership, and compensation can itself fail. A check that + makes the failure unreachable is smaller than a rollback that handles + it. +- **Neither failure aborts the reconciliation of other buffers.** A + directory delete reaching twelve descendants must not stop at the one + that is mid-edit. + +**Both paths call it**: the drain harvest for `pmacs.fs.remove`, and +`apply_resource_op`'s delete arm, replacing its first-match lookup. + +**Both paths refuse. The policy is symmetric** (rev 7 — round 6; rev 6 +had them asymmetric, and that half is withdrawn): + +- **dired refuses the whole entry** when a visited buffer is modified — + the file is never deleted. A direct user gesture on a file with + unsaved changes should stop, not proceed-and-cope. +- **`apply_resource_op` refuses the operation** when any affected buffer + is modified — **before touching disk**, so nothing is deleted and + nothing is removed from the registry. That is **PR #186's Q#RD1**, not + this lane's decision, and this lane adopts it. + +Rev 6 argued the opposite for the LSP arm: that the delete should +proceed because "the user already accepted the refactor" and refusing +mid-edit "leaves a half-applied refactor", with the orphaned buffer as +accepted residue. **That is withdrawn.** Two reasons it was wrong beyond +the cross-lane conflict: + +- Accepting a *refactor* is not accepting the loss of edits made after + it was requested, and pmacs cannot tell the two apart from inside the + applier. +- The "half-applied refactor" argument leaned on an assumption about + `WorkspaceEdit` recovery that this document never actually checked. + Checked now, against **LSP 3.18** (§0, round 6): only + `textOnlyTransactional` degrades to abort for resource operations, + `transactional` covers them, and the specification states **no + default** for a client that does not advertise `failureHandling` — + which pmacs does not. So the protocol does not license "deleting + anyway is the safer half", and the argument had no support. + +**So on the synchronous path there is no orphan to reconcile.** The +residue that remains is the asynchronous one, below — and it is dired's +alone. + +### Deletion is harvested, not hand-fired (G4) + +Rev 2 left this ambiguous, and the two options really do produce +different primitive contracts. Rev 3 chooses the one symmetric with +rename: **`remove` is harvested in the drain**, through the same +`TickOutcome.resources` list, firing **`resource.deleted(path)`**. +`PendingJob` carries the path for `JobKind::FsRemove` in the same +`Option` field it carries a rename's pair in (§5, R3) — one +field, one enum, so the two cannot both be set. + +The reason is the same as Q#DR14's: a **fire-and-forget** `pmacs.fs.remove` +must reconcile too, and dired firing the hook itself after its own +`:await()` would protect only dired. The policy (what to delete) is +dired's preflight; the mechanism (what to reconcile once the syscall +lands) is the primitive's. + +### Reconciliation is order-independent, and says so (Q#DR29, new in rev 8) + +**Each settled mutation reconciles on its own. Nothing depends on the +relative order of two mutations that were in flight simultaneously**, and +the framing promises nothing about it — because `TickOutcome.resources` +is bus-arrival order and the runtime establishes no execution order (§5, +F1). + +That is safe rather than merely honest, for three verified reasons: + +- **Independent mutations commute.** Disjoint paths and disjoint + subtrees reconcile to the same registry state in either order, which + is every case the shipped consumers can produce. +- **No shipped in-tree path produces interdependent concurrent + mutations.** dired **serializes** — one coroutine per batch, awaiting + each op before dispatching the next (§9). `apply_resource_op` is + **synchronous on the main thread** and never enters the drain. And §2 + verified `pmacs.fs.rename` and `pmacs.fs.remove` have **zero in-tree + production callers** besides tests. They remain public package APIs, + so this is not a claim that third-party reachability is zero. +- **The primitive currently recommends serialization, and 2a makes the + correctness precondition explicit.** + `fs.lua:155-165`: *"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)."* That comment today explains + cancellation/supersede, not reconciliation. 2a extends it in the same + block: **mutations whose source/target paths overlap must be + serialized by dispatching the next only after the previous handle + settles**, because reply order does not recover filesystem execution + order. §9 already follows this rule. A caller that ignores the + explicit contract owns the visible stale-binding residue below. + +**Why no static ordering rule is offered.** It was worked out rather +than waved away: 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. A fixed "deletes before renames" +rule gets one right and the other wrong — the kill misses, the rename +then rebinds the buffer onto a path whose file is gone, and it survives +pointing at nothing. **There is no rule short of a real execution-order +token**, which §11 defers with a checkable trigger. + +**The residue, stated:** a third-party package that fire-and-forgets two +interdependent mutations, against the explicit 2a contract, can leave +a buffer bound to a stale path or kill one that should have been +rebound. Recoverable, visible, and not data loss — but real. §13 pins +the **independent** case in both arrival orders and deliberately pins no +interdependent one, because a test cannot pin an order the mechanism +does not establish. + +### The four cases + +| Case | Policy | +|---|---| +| Unmodified visited file | Delete, then kill the buffer | +| **Modified** visited file | **Refuse that entry.** Report it; the rest of the batch proceeds | +| Open buffers *under* a deleted directory | Same two rules, applied to every buffer whose path is under it | +| Open dired handles on the path or under it | Killed, via `resource.deleted` | + +**Refusing on modified is a deliberate divergence from Emacs**, which +deletes and leaves an orphaned buffer. The reasoning: the orphan is +indistinguishable from a normal buffer, and the next `C-x C-s` silently +resurrects the file. A refusal is visible, recoverable, and the user can +save-or-discard and retry. **PR #186 reached the same conclusion +independently for the LSP arm** (its Q#RD1 — "refuse, do not prompt, do +not save, do not back up"), which is why rev 7's policy is symmetric +rather than split. + +### Checked twice — and still only best-effort (G4, narrowed H1) + +`buf:is_modified()` is exposed to Lua (`mod.rs:1261`), so dired decides +this itself. It checks **twice**: + +- **Before the confirm**, so the prompt states the skip up front: + `Delete 3 entries? (1 has unsaved changes and will be skipped) (y/n) `. +- **Again immediately before dispatching each removal.** The prompt is not + modal against the world: another frontend attached to the same daemon + can edit a buffer while it is open, and the batch is serialized (§9), so + there is a real window between the answer and the *n*-th dispatch. + +**And that is where the guarantee stops. It is a pre-dispatch check, not a +lock (H1).** Rev 3 said "immediately before each syscall", which was wrong +about where the boundary is: `pmacs.fs.remove` **dispatches a worker**, so +the syscall happens later, on another thread, in `remove_blocking`. An +edit landing between dispatch and `remove_file`/`remove_dir` is not +detected by anything, and **no acceptance test can pin an interval that is +not closed** — which is why rev 3's acceptance 20 (edit before `y`) could +never have detected this. + +So the promise is stated at its real strength, the same way G6 forced for +`R`: **a TOCTOU-bounded pre-dispatch refusal.** What survives the race is +worth being precise about: + +- the **buffer** survives with its contents — `reconcile_delete` keeps a + modified buffer rather than killing it, and that half *is* robust, + because it runs at drain time on whatever state exists then; +- the **file** is gone. + +Which means the orphaned-modified-buffer residue is **dired's, and only +dired's** (rev 7 — round 6). Rev 6 called it "the residue rev 3 scoped +to the LSP path applies to dired too — one deferral, two paths". After +#186 that is backwards: **the LSP path has no residue, because it +refuses before disk.** Dired's remains because **dired never goes +through `apply_resource_op`** — it calls `pmacs.fs.remove`, which +dispatches a worker (§2), so a synchronous guard inside the applier +cannot reach it at any strength. §11 carries it as **one deferral, one +path**. + +Closing it properly needs one of two things this stage should not invent: +a **reservation** (some lock or generation the worker re-validates before +the syscall), or a **synchronous** delete path. The synchronous option is +tempting because `apply_resource_op` already does main-thread +`std::fs::remove_*`, and it would remove the need for the delete harvest +entirely — but it puts N blocking syscalls on the main thread for an +N-entry batch, and a fire-and-forget `pmacs.fs.remove` from any *other* +caller would still need the harvest. Named in §11. + +## 7. Confirmation (Q#DR15) + +Destructive operations confirm: `x`, `D`, and (in 2c) a recursive delete +and an overwriting copy. There is no helper to do it with (C4). + +**Stage 2 adds `builtin/runtime/minibuffer.lua` defining +`pmacs.minibuffer.confirm { prompt, on_yes, on_no, on_cancel }`**, +loaded before `dired.lua` in `editor.rs`'s explicit sequence. + +**Three outcomes, because the substrate already has three** (rev 8, +F5). Rev 7 exposed only `on_yes`, which cannot express §8's copy +contract — where declining an overwrite **continues**, copying the +non-colliding entries. There was no negative continuation to hang that +on. And the underlying primitive is richer than the helper was: +`pmacs.minibuffer.read` takes a required `on_accept` and an optional +`on_cancel` (`src/lua_bindings/mod.rs:13403-13417`), and the session +carries both (`src/minibuffer.rs:403-405`), with `cancel()` returning +the on-cancel callback (`:353`). A prompt therefore has +accept-affirmative, accept-negative and cancel — and rev 7 collapsed all +three into "call `on_yes` or do nothing". + +| Outcome | Trigger | Runs | +|---|---|---| +| **yes** | `y` / `yes`, case-insensitive | `on_yes` | +| **no** | any other accepted text, **including empty `RET`** | `on_no` | +| **cancel** | `C-g` / Escape | `on_cancel` — **and nothing if it is absent** | + +The fail-closed default from Q#DR15 is unchanged: an empty `RET` is +still not affirmative. It now has somewhere to go instead of being +indistinguishable from silence. + +**Cancel does NOT fall through to `on_no`, and that is the decision +worth arguing with.** For `x` and `D` the two are equivalent — decline +and cancel both mean "delete nothing". For `C` they are not: declining +means *copy the non-colliding entries*, a partial action, while +cancelling means *abandon the batch*. Defaulting cancel to `on_no` +would make `C-g` perform a partial copy, which is precisely what a user +pressing `C-g` is trying to avoid. So cancel is inert unless a caller +opts in, and §8's `C` flow is the only caller that does. + +**That sequence is a real edit to `src/editor.rs`** — around thirty +`include_str!` entries at `src/editor.rs:395-660`, each naming its +chunk — so **2b touches a Rust file**, which §16 now says and rev 5 did +not (R6). Ordering is load-bearing, not cosmetic: `dired.lua` calls +`pmacs.minibuffer.confirm` at load-time-registered command bodies, so +the defining chunk must run first. + +It takes **no completion source**, and that is the decision, not an +omission: + +- With no source, `resolve_accepted_value` returns typed text verbatim + (`:565-567`), so the answer is exactly what the user typed and the + shadowing question does not arise. +- Affirmative is `y` or `yes`, case-insensitively. **Everything else, + including an empty `RET`, is no.** A destructive prompt must fail + closed. +- The alternative — `autosave.lua`'s two-candidate source — happens to + be safe today only by lexicographic luck. `fuzzy_score` returns + `Some(0)` for an empty needle (`minibuffer.rs:638-640`) so **both** + candidates match, and `filter_and_sort` breaks the score tie on + `a.1.cmp(b.1)` — plain ascending string order (`:678`). `"no" < "yes"`, + so `selected = Some(0)` resolves to `"no"`. The right default, reached + by an accident of spelling: the safe answer wins **only because it + happens to sort first**. Relabel the pair and it inverts — `{"delete", + "keep"}` or `{"apply", "cancel"}` both put the *destructive* answer at + index 0 and make an empty `RET` execute. (Note it is spelling, not list + order, that decides: the tie-break is lexicographic, so writing + `{"no", "yes"}` changes nothing.) Not a foundation for four new + destructive call sites. + +The prompt states the count and the operation — `Delete 3 marked +entries? (y/n) ` — because a confirmation that does not say what it is +confirming is decoration. + +**Migrating `autosave.lua` to the helper is a named follow-up (§11), not +part of this stage.** Both shapes already answer "no" to an empty `RET`, +so the migration is behavior-preserving; it still touches the +crash-recovery prompt, which does not belong in a dired PR. + +--- + +## 8. The operations, individually + +**`d` / `x` (flag and execute).** `d` sets `D` and advances. `x` collects +`D`-flagged basenames, applies §6's visited-path policy, confirms with the +count, then deletes serially (§9) via `pmacs.fs.remove`, which handles +files and empty directories (C5). A non-empty directory fails with the +kernel's `ENOTEMPTY`, **reported as such** in 2b; 2c's `remove_dir_all` +plus `dired.recursive-deletes` addresses it. Then revert once. + +**`D` (delete now).** Same deletion path and same §6 policy, targeting +the §4 set rather than the flags. + +**`R` (rename).** The entry **at point**, regardless of marks (§4). +Prompts with `initial` = the current basename and **no source**, so the +typed name arrives verbatim (`minibuffer.rs:565-567`). A bare name +resolves against `handle.path`; a name containing `/` is a path, relative +to `handle.path` unless absolute. Reconciles every path owner by §5, +including on a directory. + +> **The no-clobber guarantee is a preflight, and rev 3 says so (G6).** +> `rename_blocking` calls plain `std::fs::rename` (`fs.rs:492-499`), +> which on Unix **silently replaces** an existing target. Dired stats the +> destination and refuses if it exists, but that check and the syscall are +> not atomic: a target created in between is overwritten. Rev 2's +> "refuses an existing target" overstated a **best-effort, TOCTOU-bounded +> refusal**, and acceptance 12 is reworded to promise only what is +> delivered. Closing it needs a no-replace primitive — +> `renameat2(RENAME_NOREPLACE)` on Linux, `renamex_np` on macOS, with a +> link/unlink fallback elsewhere — which is a portability question of its +> own and is deferred (§11). + +**`w` (copy filename).** Carried forward from the parent's approved +table, and **set-based** (§4): with marks, it copies every marked name, +newline-separated; with none, the entry at point. Non-destructive, no +confirm. + +> **It needs a public kill-ring entry point, which does not exist (G5).** +> `push_entry` is a `local function` (`killring.lua:93`) and the public +> `copy()` requires a region — it calls `ed.region()` and fails with "no +> region" otherwise (`:184-194`). Rev 3 adds +> **`pmacs.killring.push(text)`** (Q#DR22) with exactly the semantics +> `copy()` already establishes for non-kill text: `push_entry`, mirror to +> the OS clipboard via `ed.clipboard_set`, and **break the kill chain** +> (`fail_kill`), because a filename copy is not an appendable kill and +> must not merge into an adjacent `C-k` run. + +**`M` (chmod). New scope — needs explicit approval (F3).** Prompts for an +**octal** mode string, no source, validated to `[0, 07777]` before +dispatch to match `fs.chmod`'s own guard (`fs.lua:181-183`). Applies to +the §4 set. + +> **It refuses symlink entries.** `chmod` follows symlinks +> (`fs.lua:145-153`) while the listing is lstat-based, so chmodding a +> symlink line silently changes a *different file's* mode and the +> refreshed listing shows the link's own unchanged bits — the change +> appears to have done nothing. The parent framing already decided this +> for the wdired surface: the fixture "rejects symlink perms edits at +> intercept time for exactly this reason", and that decision "carries +> over unchanged" (`dired-framing.md:543-546`). Rev 1 proposed warning +> after the fact instead, which is materially different and contradicted +> approved text. A refusal is reported per entry and does not abort the +> batch. + +**`+` (create directory, 2c).** Prompts for a name, no source, resolved +against `handle.path`. `opts.parents` for `create_dir_all`. + +**`C` (copy, 2c) — full command flow (F7).** Targets the §4 set. + +1. **Destination prompt.** One source prompts for a destination path; + **several sources require an existing directory** and the command + refuses before dispatching anything if the answer is not one. +2. **Collision scan, before any copy.** Resolve every source to its + destination path and stat each. This happens up front so the user is + asked once, not once per file mid-batch. +3. **Confirm.** With no collisions and one source, no prompt — a copy + onto free space is not destructive. With collisions, **one** confirm + naming the count: `Overwrite 2 existing files? (y/n) `. **Declining** + (`on_no`, including an empty `RET`) **skips the colliding entries and + copies the rest**, rather than abandoning the batch, which matches + §9's per-entry failure rule. **Cancelling** (`on_cancel`, `C-g`) + **abandons the whole batch and copies nothing** — the distinction §7 + exists to preserve, and the reason `C` is the one caller that passes + all three callbacks. +4. **Dispatch** with `opts.overwrite` set only for the entries the user + confirmed. The primitive still refuses an existing target without it, + so the guard is enforced at both layers. +5. **A directory source is refused** — the parent's decision, and the + honest one while no recursive-copy primitive exists. + +Mode bits are preserved. + +## 9. Execution: serialize, report, then revert (Q#DR16) + +`fs.lua:155-165` instructs packages needing at-most-one-pending +mutation semantics to *"await each op before dispatching the next"*. +Stage 2 does that, for three reasons beyond obedience: a batch is +user-initiated and small; interleaved failures are attributable to a +specific entry; and awaiting reaps the pending entry, keeping dired out +of the leak in §2. + +The contract: + +- **One coroutine per batch**, `pmacs.async`, awaiting each op in turn. +- **A per-entry failure does not abort the batch.** Collect + `{basename, message}`, continue. +- **One status line at the end**, naming counts: + `dired: deleted 2, failed 1 (report.log: Directory not empty)`. Not one + status per entry — the last would be the only one visible. +- **Marks consumed by a successful operation are cleared; marks on + entries that failed are kept**, so `x` again retries exactly the + failures. +- **Revert once, after the batch**, not per entry: the listing is a + wholesale repaint and N repaints for N deletions is both slower and + visibly wrong. +- **Every post-`await` re-seat checks `pmacs.window.buffer()` first** + (`dired.lua:400-404`), the rule #165's review round produced. +- **No part of a batch may run inside `pmacs.window.commit_to`** (new in + rev 5, from N2). The commit scope **refuses an `await`** (Q#JR14b) — + `commit_to_refuses_an_await_and_restores` pins the raise, whose + message contains both `"cannot await inside"` and `"commit_to"` — and + a batch is defined by awaiting each op in turn. Stage 1 already solved + the same problem the same way: `open_directory` does its + `read_listing` await **outside** the commit and puts only the + non-yielding mutation inside (`dired.lua:640-655`). Stage 2's + operations follow that shape — await the whole batch first, then, if a + destination was captured, do the repaint and re-seat inside a commit. + Stage 2's operations are all initiated from an already-active dired + buffer rather than from a captured destination, so in practice + `opts.dest` is nil on these paths and the ambient rule above governs; + the constraint is stated because the *shape* must not be copied + wrongly if that ever changes. + +--- + +## 10. Staging: three PRs (Q#DR17, revised in rev 4) + +**Rev 4 takes the further cut, as directed in round 3.** Rev 3 already +recorded that 2a's Rust had outgrown "one narrowly-scoped change"; round 3 +called it, and the three rounds of findings on the reconciliation half are +the evidence — every one of G1, G3, H1, H2, and H3 was about the +transaction, not about marks. + +**Rev 6 keeps the three-PR cut unchanged.** The cut was re-examined +against the round-4 re-scout and again after round 5, and still holds: +2a is substrate-only, 2b is the dired surface, 2c is the three additive +primitives, and Stage 3 is wdired. Nothing that landed in the 153 +commits moves work across those lines — #182 changed the *shape* of +dired's entry point but added no Stage 2 work, and #179/#181 added none. +**Q#DR25 has left the table entirely** (R1): it is the +`generated-buffer-immutability` lane's, not 2b's (§3.1). And the +acceptance row is corrected (R2) — rev 5's allocation put two +substrate items in 2b and one dired item in 2a. + +| | **2a — reconciliation** | **2b — marks and operations** | **2c — new primitives** | +|---|---|---|---| +| User-visible surface | **none** | `m u U t d x D R w M` | `+ C`, recursive delete | +| Rust | `reconcile_rename`, `reconcile_delete` (both kill phases), `BufferNameOrigin`, `ResourceOp` + `TickOutcome`, `PendingJob.resource`, `forget_uri` (14 stores + `documents` + route purge + URI-scoped drain + exact-pair tombstone), server-aware `mark_document_stale`, `View::rename_resource` + the window sweep, `apply_resource_op` (rename **and** delete arms), `apply_workspace_edit` origin, `pmacs.buffer.set_name` | `pmacs.killring.push` | `mkdir`, `copy`, `remove_dir_all`; `JobKind` 12 → 15 | +| Lua | the two hook subscribers in `lsp.lua`, the server-aware `_mark_document_stale` call, and `fs.lua`'s explicit overlapping-mutation serialization contract | all of `dired.lua`'s mark/op layer, **including its `resource.renamed` subscriber**, plus `minibuffer.lua` | two ops | +| Other files | — | **`src/editor.rs`**, to add `minibuffer.lua` to the explicit load sequence (R6) | — | +| Config keys | none | none | `dired.recursive-deletes` | +| Acceptance | **23–38 and 50–55, including every suffixed item** | 1–22, 39–41 | 42–47 | + +**Why 2a first, with no dired surface at all.** It is a self-contained +substrate correctness fix that stands on its own merits: it closes the +path where renaming the active file through a workspace edit +**materializes a phantom buffer** (`lsp.lua:1352`); it replaces the +raw first-match, un-normalized registry lookup that **both** +`apply_resource_op` arms use with one shared prefix-aware query; and it +supplies the **full removal lifecycle** that neither existing path +performs (§6, Q#DR27). None of that needs dired to be worth fixing, and +none of it is dired's fault. *(Rev 6 also claimed 2a closes the +LSP-authored delete that destroys unsaved work. **PR #186 owns that** — +it refuses before disk rather than reconciling after — so rev 7 drops +the claim. What is left is still substrate-level and still worth its own +review round.)* Landing it +alone also means the LSP lifecycle work — a fourteen-store inventory, an +in-flight route purge, an awaiter drain, and a new `View` hook swept across +every window — gets a review round of its own rather than sharing one with +a mark column. + +**Why marks second rather than first.** The mark layer is the more visible +work and the more pleasant to review, which is exactly the argument for +*not* putting it in the same PR as the substrate change: it would absorb +the attention. It also genuinely depends on 2a — `R` is unsafe on a +directory without the transaction, and `D`/`x` are unsafe on a visited file +without `reconcile_delete`. + +**Why 2c last and separate.** Three additive fs primitives with their own +overwrite and lstat-safety semantics (§8, §13 items 42–47). Nothing in 2b +needs them; `remove` already covers files and empty directories (C5). + +**The cost, stated.** Three PRs is three review cycles instead of two, and +2a ships nothing a user can see — its acceptance is entirely +substrate-level (a no-await rename, a two-window diagnostic re-root, an +LSP store inventory). That is the trade round 3 accepted, and the reason it +is worth it: a bug in 2a is a silent data-loss bug, and those are the ones +that deserve an undivided reviewer. + +## 11. Deferred (named) + +- **`wdired`** — Stage 3, with the frozen fixture as its reference (C3). +- **The fire-and-forget pending-entry leak** (§2). Pre-existing, verified, + orthogonal, observable as `pending_len` growth. Needs its own lane; the + candidate fix is a settled-entry sweep with a reap policy, which is a + decision about handle lifetime, not a patch. +- **A general `purpose`/`owner` field on `PendingJob`**, per + `COHERENCE.md` §9, which should subsume §5's `resource` field. +- **An execution-order token for overlapping fire-and-forget filesystem + mutations** (Q#DR29). 2a documents the public caller precondition: + overlapping mutations serialize by awaiting one before dispatching + the next. The implementation trigger is the first shipped in-tree + caller that cannot satisfy that rule; at that point the runtime needs + a real execution token rather than a static reorder of bus replies. +- **Migrating `autosave.lua` to `pmacs.minibuffer.confirm`** (§7). +- **Multi-file `R` into a target directory**, and `%`-regexp marking — + both need a target/pattern concept Stage 2 does not build. This is why + `R` is point-based in §4 rather than an unstated exception. +- **Symbolic chmod** (`u+x`), needing a mode-expression parser. +- **`i` (insert subdirectory)** — the recursive in-buffer case, already a + named deferral in `docs/dired-framing.md` §13, and the place a shared + tree primitive (`COHERENCE.md` §14) would land. +- **`!` shell command on marks**, compress, symlink, hardlink. +- **A no-replace rename primitive** (G6) — `renameat2(RENAME_NOREPLACE)` + on Linux, `renamex_np` on macOS, link/unlink elsewhere. Until then `R`'s + refusal is a TOCTOU-bounded preflight, which §8 states plainly. +- **The async race: a buffer modified after dired dispatch is still + orphaned** — **one deferral, one path** (H1; narrowed in rev 7, round + 6). Rev 6 carried this as two paths, LSP and dired. It is dired's + alone: **#186 refuses the synchronous `apply_resource_op` delete + before disk**, so that path orphans nothing, while dired calls + `pmacs.fs.remove`, which **dispatches a worker** — the interval + between dired's pre-dispatch check and `remove_blocking`'s + `remove_file` is not closed by anything, and a buffer modified inside + it loses its file. Closing it needs a **reservation** the worker + re-validates before the syscall, or a **synchronous** delete path — + the latter would also make the delete harvest unnecessary for dired, + but puts N blocking syscalls on the main thread for an N-entry batch. + Note this is the *only* thing left of rev 6's orphaning story; the + LSP half went away with #186 rather than being solved here. +- **Two wider LSP-store holes the tombstone does not close** (new in + rev 8, F4). The fourteen `pub fn *_store()` accessors + (`src/lsp.rs:1031-1114`) each hand out an `Arc>` clone, + conferring unrestricted `set`/`clear`/`mark_stale` on any holder — the + fifteen Lua `clear` bindings already prove the write capability + crosses the FFI boundary, and nothing in the type system stops a + `set`. Separately, the `workspace/inlayHint/refresh` and + `workspace/semanticTokens/refresh` handlers + (`builtin/runtime/lsp.lua:1845,1853`) re-pull by iterating + `attachments` with **no cross-check against `documents`**, so a purge + can be followed by a fresh — and route-correlated, therefore + ungated — request for a URI the editor no longer holds. Both are + pre-existing, both are wider than a dired stage, and both would be + in scope for an LSP-lifecycle lane rather than this one. +- **`pmacs.fs.remove` itself is guarded by neither lane** (new in rev 7, + round 6). After both land, the refusal exists at the + `apply_resource_op` primitive (#186) and in dired's policy layer + (§6) — but `pmacs.fs.remove` is public Lua API with **no dirty check + of its own**, so a third caller inherits neither guard. This is + latent rather than live: §2 verified `pmacs.fs.remove` has **zero + production callers** (only `tests/m8_1_acceptance.rs:438,439,472`). + Naming it because the natural reading of "both lanes guard deletion" + is that the primitive is guarded, and it is not — the guards are one + layer above it on each side. +- **A passive window that never received a `DiagnosticView` still has + none** (H3). `_attach_view` takes `active_window_mut()` and errors + otherwise (`lua_bindings/diag.rs:218-224`); `ensure_overlay` + + `clone_for_split` cover split-from-attached, not attach-while-passive. + Pre-existing, and rename cannot re-root an overlay that was never + attached. +- **The server-restart teardown does not clear the fourteen result + stores** (`lsp.rs:1307-1345` clears routes, documents, and deferred + notifications only). Pre-existing; whether stale results should survive + a restart is its own question, and `forget_uri` deliberately does not + answer it. +- **The rooturi sink's weak wait predicate** (`m4_acceptance.rs:5499`) — + same class as the config-sink race fixed in #174, not observed failing, + and the obvious fix would trade a precise regression diff for a vague + timeout. Needs a record terminator in the fake server first. + **Confirmed in rev 5, and now backed in-tree:** #174 merged during + this re-scout (`main` @ `0442d78`), fixed the *config* sink by waiting + for a complete JSONL record (`ends_with('\n')` rather than + `contains("probe")`), and **deliberately left the rooturi predicate + alone**, adding a comment at `m4_acceptance.rs:5486-5496` giving the + same three reasons this deferral gives. So this item is no longer a + claim of this framing; it is a claim of the tree. +- **Recursive copy** — 2c refuses directory sources; a real `copy -r` + primitive is separate. +- **The whole generated-buffer immutability class, including dired's + instance** (Q#DR25, withdrawn from this document in rev 6 — R1). + Dired's `paint` writes through `bypass_intercept` over a writable + rope behind an erroring intercept, so `M-x buffer.undo` empties a + listing. So do `listview.lua`, `compile.lua`'s `ensure_slot` + (`*compilation*` **and** `*shell-command*`) and the independent + `*search-results*` panel — one idiom, four instances, and **no Lua + caller anywhere sets `read_only`** because there is deliberately no + Lua `set_read_only`. Owned by the **`generated-buffer-immutability` + lane**; §3.1 records what that lane inherits from this re-scout, and + why neither landing order conflicts with Stage 2b. Note the two + sub-shapes that lane must reconcile and this one did not have to: + dired and listview write **whole-buffer replaces** and are the cheap + half, while `*compilation*`, `*shell-command*` and `*search-results*` + **append** per batch and need a streaming variant of the primitive + that does not exist. +- **Two persisted path owners, named not fixed** (rev 5, W5). + `saveplace.lua` keys its places file by path (`load_places`, `:31-45`; + `restore_active`, `:69-79`) and `recentf.lua` keeps a path MRU list + (`record`, `:36-46`). Neither is an in-memory owner — both rebuild + their index from `pmacs.state` on every call — so a rename leaves a + stale *line on disk*, not stale editor state. The consequence is + bounded and self-correcting: entries are capped and LRU-evicted, and + the worst case is a future unrelated file at the old path restoring a + wrong cursor. Reconciling them would mean teaching the reconciliation + transaction to rewrite persisted state, which is a persistence-arc + question. A `resource.renamed` subscriber in each is the cheap fix + whenever someone wants it. +- **`lean.lua`'s `M.file_progress` is not re-rooted** (rev 5, owner 6; + reaffirmed in rev 6, R5). Stage 2 supplies the hook that makes a + subscriber possible and **does not write one** — Lean's arc owns that + file, and Stage 5's goal view is the consumer that would notice. + Round 5 found §1 claiming the opposite; **this deferral was the + correct half and §1 was corrected to match it**, not the other way + round. Until Lean subscribes, a rename leaves its progress entry under + the old URI, and nothing in Stage 2 detects that. + +--- + +## 12. Bets + +- **B1.** **2b** needs **no** new `pmacs.fs` op. Falsified if any of + `m u U t d x D R w M` cannot be built on the existing five. *(Rests on + C5, which was read off `remove_blocking` directly.)* *(Rev 5: rev 4 + wrote "2a" here, which contradicted its own §10 table — 2a ships no + dired surface at all, so it cannot need an op for a key it does not + bind. Corrected label; the bet is unchanged.)* +- **B1b.** The two new hooks need no new hook machinery — + `run_hook_if_defined` (`mod.rs:1623`) and `pmacs.hook.run` (`:6005`) + already exist, and `resource.renamed`/`resource.deleted` are ordinary + names in that registry. Falsified if firing a hook from the reconcile + path needs a new dispatch mechanism. +- **B2.** The rename rebind needs **no** protocol bump and leaves the + worker↔main wire untouched. Falsified by any change to `ReplyKind` or + `SUPPORTED`. +- **B3.** The frozen `m8_1`/`m8_2`/`m8_3` counts are unchanged by the + reconciliation. *Not obvious, and less obvious in rev 2:* the fixture + renames files it lists, so a test holding a buffer on a renamed path + would newly see its path **and now its name** move, and `m8_3` + monkeypatches the Lua `rename` so it never reaches the Rust path at + all. The additivity gate is a real check, not a formality. +- **B3b.** No existing LSP test changes behavior. *Not obvious:* the + `resource.renamed` subscriber issues didClose/didOpen and may re-run + `ensure_server`, so any suite that renames a file with a server + attached is in the blast radius. `m4`/`lsp_multi_root` counts are + gated for exactly this reason. +- **B4.** No GPU or TUI frontend change. Marks are buffer text and the + keymap is mode-scoped; `set_round_trip_input` is already set by Stage 1. +- **B5.** `describe_key_identifies_every_default_binding` stays green + without further surgery — #165 already taught it per-binding mode + context, and Stage 2 only adds more bindings in the same mode. +- **B6** *(added in rev 5; **retired** in rev 6, R1 — it was a bet + about Q#DR25, which is no longer this document's.* Handed to §3.1 for + the `generated-buffer-immutability` lane, where it is worth keeping: + the primitive, its Lua binding and the fan-out all exist + (`src/buffer.rs:545`, `src/lua_bindings/mod.rs:3079-3095`), so + dired's adoption should need no Rust — falsified if it requires a new + binding, a `set_read_only` exposure, or any change outside + `builtin/runtime/dired.lua`. *Not obvious in one + respect:* dropping the intercept removes the only thing that currently + produces dired's refusal **message**, so if any Stage 1 acceptance + depends on the intercept's exact wording rather than on the substring + `read-only`, this bet fails. §3.1 checked the one test that looked + like a risk and it does not. +- **B8** *(new in rev 7, round 6)*. Adopting #186's refusal costs this + lane **no design change beyond deletion**: `reconcile_delete`'s + signature, its callers, the drain harvest and `resource.deleted` are + all unaffected, because the refusal happens *before* the seam rather + than inside it. Falsified if 2a turns out to need a different seam + shape once #186 has landed — most plausibly if #186's validation phase + ends up owning the affected-set query in a form `reconcile_delete` + cannot reuse, in which case the two must be reconciled before 2a is + cut rather than after. +- **B7** *(new in rev 5, N3)*. The journey ratchet stays at **≥ 24** + rows in `tests/journey_acceptance.rs` and keeps its GPU row in + `tests/gpu_invocation_acceptance.rs`, with no row weakened. Falsified + by any deletion, `#[ignore]`, or assertion relaxation in either — and + the ratchet's own doc says a green test that cannot fail must be + deleted rather than kept, so "still green" is not the bet; "still + biting" is. + +--- + +## 13. Acceptance + +**2b — marks** *(stage labels follow §10's three-PR cut)* + +1. `m` marks the entry at point and advances; the mark renders at + `_layout.MARK_START` and **no other column moves** (asserted against + the exported layout, not hardcoded offsets). +2. `u` clears and advances; `U` clears every mark; `t` inverts `*` and + leaves `D` untouched. +3. Marks survive a **sort** (`s`) — the basename-keyed set must follow + its entries to their new lines. +4. A mark on a basename that vanishes is **pruned by a revert** and is + absent from the next batch. +5. Marks are per-buffer: two dired buffers on two directories keep + independent sets. +6. **`R` with unrelated marks present** renames the entry at point, not + the marked set, and **leaves every mark intact** (F4 — pins that §4's + point-based class is real and not an accident). + +**2b — operations** + +7. `d` then `x` deletes the flagged file, after a confirm; declining + deletes nothing. +8. `x` consumes `D` flags **only** — a `*`-marked entry survives it — + and with nothing flagged it is a reported no-op. +9. `D` with nothing marked targets the entry at point (§4). +10. `x` on an empty directory succeeds; on a **non-empty** directory it + fails, the message names the entry, and **the rest of the batch still + runs**. +11. A batch with one failure reports both counts in one status line, and + the failed entry **keeps its mark** while the successful one loses it. +12. `R` renames; a bare name resolves against the listing's directory; + a target **observed to exist at preflight is refused** — the honest + contract (G6), since `std::fs::rename` would replace one appearing + afterwards. +13. `M` applies an octal mode to the marked set; an out-of-range mode is + refused before dispatch. +14. **`M` refuses a symlink entry** (Q#DR19, F3/F5): the mode is + reported unchanged, the target file's mode is **asserted untouched**, + and the batch continues. *(A warning-after-the-fact implementation + fails this.)* +15. `w` copies the **marked** filenames to the kill ring, + newline-separated, and the entry at point when nothing is marked; + the OS clipboard mirrors it and the **kill chain is broken**, so a + following `C-k` does not append to it. +16. The listing reverts **once** after a batch, not per entry. + +**2b — deletion policy (§6)** + +17. Deleting an **unmodified** visited file kills its buffer. +18. Deleting a **modified** visited file is **refused**; the buffer + survives with its contents, and the file is still on disk. +19. The confirm prompt **states the skip before the user answers**, not + after. +20. **A buffer modified after the prompt appears but before `y`** is + skipped and reported: the check is re-run before each **dispatch**, + because another frontend can edit during the prompt and the batch is + serialized. *(An implementation that checks only once, up front, fails + this.)* **This is the whole of the guarantee** — the dispatch-to-syscall + interval is open (H1) and deliberately has no test, because no test can + pin an interval that is not closed. What §13 item 24 pins instead is + the half that *is* robust: the modified buffer survives. +21. Deleting a directory kills buffers on its **descendants**. +22. An open dired handle on a deleted directory is closed + (`resource.deleted`). +*(Items 23–24 moved to 2a in rev 6 — R2. They exercise +`apply_resource_op` and the drain harvest, neither of which 2b builds.)* + +**2a — reconciliation (§5, §6)** + +23. **`apply_resource_op`'s delete reaches descendants and a second + buffer on the same path** — the raw-path first-match lookup replaced + by the shared prefix-aware, normalizing query (G4). *(Rev 7, round + 6: the **modified**-buffer half of this item moved to **PR #186**, + which refuses the delete before disk, so by the time this lane's + reconciliation runs there is no modified buffer on the synchronous + path to spare. Assert the lookup fix here; #186 asserts the + refusal. If #186 has not landed when 2a is implemented, this item + still stands on the lookup alone.)* +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. + +25. **No-await rename**: dispatch `pmacs.fs.rename`, never take the + result, pump — the open buffer's path has moved. *(Fails if the + reconciliation lives at result-consumption.)* +26. **Directory rename**: a buffer open on `dir/child.txt` follows + `dir` → `newdir`. +27. **Every match, not the first** (F5): **two** descendant buffers under + the renamed directory **and two buffers visiting the same exact + path** all move. *(One child buffer does not defeat a first-match + implementation; this does.)* +28. **False prefix**: renaming `/…/foo` does **not** rebind a buffer on + `/…/foobar`. +29. **Buffer name follows explicit provenance — tested in BOTH + directions** (rev-9 F5). + (a) A buffer opened by a **relative** path (name `foo.rs`, stored + path `/abs/dir/foo.rs`) gets its name updated because its load site + records `BufferNameOrigin::PathDerived`. *(Rev 7's string-equality + rule fails this.)* (b) Open the file **`${cwd}/notes`**, then + explicitly set its name to **`notes`** through + `pmacs.buffer.set_name`; a rename keeps `notes` because that call + records `Explicit`. *(Rev 8's path-equivalence heuristic fails this: + the custom string normalizes to the exact stored path.)* Both arms + are required; together they bite against both superseded heuristics. +30. **An attached LSP buffer with diagnostics present before the rename, + shown in at least TWO windows** (H3): 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** — not + merely `rec.uri` updated (`DiagnosticView.uri` is set once at + construction, and a remove-and-re-push would pass a one-window test + while reordering the stack). +31. **The store inventory** (H2): after a rename, an entry that existed + under the old URI in each of the fourteen stores plus `documents` is + gone, and **a response already in flight at rename time does not + repopulate the old key** — the `pending_routes` purge and awaiter + drain, without which the clear is undone by arrival. +31b. **A late `publishDiagnostics` for the OLD URI does not resurrect + it** (F4): after the rename completes, feed the manager a + `textDocument/publishDiagnostics` notification naming the **old** + URI; the old key stays empty and the new URI's diagnostics are + unaffected. *(This is uncorrelated — it carries no request id — so + the `pending_routes` purge cannot see it, and item 31 passes with + the bug present. Fails against a `forget_uri` with no tombstone.)* + Its companion asserts the tombstone **does not over-reach**: a + `publishDiagnostics` for a **different**, never-opened URI is still + absorbed, which is what a membership gate would have broken. + **The second state-creating writer is in the same acceptance:** call + `pmacs.lsp._mark_document_stale(server_id, old_uri)` after the + forget and assert that **none** of diagnostic, semantic-token, or + inlay-hint `stale_uris` regains the old URI. Removing either gate + must fail its own assertion. +31c. **`pmacs.lsp.forget_uri`'s error contract** (P2): it **raises** for + an unknown server id, and **succeeds** for a URI with no state under + a known server. *(The second arm is the one that matters: the + subscriber runs per attachment, but a valid attachment can have no + pending route or populated result, and repeated cleanup must remain + safe. An over-strict binding would turn that ordinary idempotent case + into an error inside a hook.)* +31d. **Tombstone identity and reclamation are exact** (rev-9 F3/F4). + Tombstone the same URI under servers A and B. `did_open(A, uri)` + clears only A, asserted directly against the manager-owned set in an + `lsp.rs` unit test. Diagnostics with distinct payloads then prove an + A write is admitted while a later B write is dropped; a second URI + tombstoned only under B proves B's `mark_document_stale` cannot set + any of the three global stale flags. Exercise + `start_generation(B, …)` and terminal `forget(B)` in separate cases: + direct set assertions prove each removes every pair for B while + preserving A's. The set is empty after the owning generation is + torn down. *(Fails against rev 8's URI-only gate, against a global + clear on any teardown, and against never reclaiming the set.)* +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). +34. **The workspace-edit origin**: renaming the *active* file through + the full `apply_workspace_edit` path leaves **no phantom empty + buffer** at the obsolete path, and the user is returned to the + **same buffer** (now under its new path). *(G1 — this is a change to + the applier, which must capture the buffer handle; no reconciliation + can reach the string it captures today.)* +35. When the origin buffer is **gone** after the edit, the applier + restores nothing rather than falling back to the old path. +36. `apply_resource_op`'s rename finds a buffer whose stored path is + normalized but whose op names it un-normalized (the `:3249` fix). +37. A **failed** rename reconciles nothing. +38. Additivity: `m8_1`, `m8_2`, `m8_3` at unchanged counts. + +**2b — the shared helpers** + +39. **`pmacs.minibuffer.confirm` — all three outcomes** (Q#DR15, F5). + (a) An **empty `RET` does not call `on_yes`**; `y`, `Y`, `yes`, + `YES` all do; `n` and arbitrary text do not. *(A typed-`n` test + alone would not catch a completion source being reintroduced — the + empty-`RET` arm is the one that detects it.)* (b) **`on_no` runs** + for `n`, for arbitrary text, and for empty `RET` — the negative + continuation §8's `C` flow needs, which rev 7's helper could not + express and rev 7's acceptance could not observe. (c) **`on_cancel` + runs for `C-g`, and `on_no` does NOT**; with `on_cancel` omitted, + cancelling runs **nothing**. *(Fails against an implementation that + routes cancel through `on_no` — which would make `C-g` perform a + partial copy.)* +40. **Serialization** (Q#DR16, F5): in a batch of N mutations, the second + is **not dispatched until the first has settled**. Asserted by + observing at most one in-flight fs job at any pump step — *not* by + the end state, which is identical if all N were dispatched at once + and awaited afterwards. +41. A marked target that vanishes **between the last revert and the + operation** is **reported**, not silently dropped (F5 — distinct from + item 4's revert-time pruning). + +**2c — new primitives** + +42. `+` creates a subdirectory, which appears on the next listing. +43. `C` copies a file and preserves mode bits; refuses a directory + source. +44. `C` with several marked entries requires an existing directory + destination and refuses otherwise **before copying anything**. +45. `C` onto existing targets confirms **once** with the collision count; + **declining copies the non-colliding entries and skips the rest** + (F7). +46. `remove_dir_all` **unlinks a symlink-to-a-directory rather than + traversing it** — pinned at the primitive, mirroring + `remove_blocking`'s lstat guard (F7). +47. Recursive delete happens only with `dired.recursive-deletes` enabled + **and** a confirm; disabled, the non-empty directory still fails. + +**2b — the dired subscriber (moved from 2a in rev 6, R2)** + +33. **An open dired buffer on the renamed directory** follows it: its + `handle.path`, **its buffer name** (`*dired:*`), and + `handle_for_path` dedup under the new path all move together (G2 — + asserting `handle.path` alone would pass with the name still stale). + *Lives in 2b because the subscriber it tests lives in `dired.lua`, + and 2a ships no dired code (§10, §16). 2a pins the hook it attaches + to instead — item 50.* + +**2a — the hook and the removal lifecycle (new in rev 6)** + +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 or was cancelled. *(R2 — without + this, moving item 33 to 2b would leave 2a shipping a hook with no + acceptance at all. The normalization half matters because + `ResolvedTarget::Directory`'s doc warns that normalization does not + happen on every path into the core, so a path-keyed subscriber + needs the canonical form.)* The symmetric assertion for + `resource.deleted` accompanies it. +51. **A killed buffer completes BOTH removal phases** (R4): after a + delete reconciles, an `on_removed` callback registered for that + buffer **has fired**, and its buffer-local keymap entries and folds + are gone. *(Fails against an implementation that calls only + `EditorCore::kill_buffer`, which does no phase-2 cleanup.)* +52. **A window displaying the deleted buffer is redirected, not left + dangling** (R4): with the buffer shown in a window, deleting the + file leaves that window on a valid fallback buffer, and **no window + holds a removed id**. *(Fails against `remove_buffer_and_fire`, + which is what `apply_resource_op` uses today — it does no window + cleanup at all.)* +53. **The last-buffer and mid-edit refusals are reported, not silent** + (R4): deleting the file behind the **only** open buffer keeps the + buffer and says so; and a delete reaching a directory of buffers + where one refuses removal still reconciles the rest. +53b. **A mid-edit refusal leaves editor state UNCHANGED — three + separate assertions** (F2). 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, asserted individually rather + than as one compound check: + + | # | Assertion | Bite — the mutation that must falsify it | + |---|---|---| + | i | the ordinary window still shows the buffer, with its `cursor`, `selection` and `view_top` intact | drop the preflight, so `kill_buffer` redirects the window to the fallback before `BufferRegistry::remove` refuses | + | ii | the **side** window is still open and still shows the buffer | drop the preflight, so `remove_side_window` collapses it first | + | iii | the buffer is still in `round_trip_buffers` (a semantic frontend still round-trips its keys) | drop the preflight, so `round_trip_buffers.remove` runs first — this is the **first** thing `kill_buffer` does and the easiest to miss | + + Each bite is the same one-line deletion, but each assertion fails + independently, which is the point: a single compound assertion can + pass on two of the three and hide the third. *(Rev 7 specified none + of this — it said to treat the refusal as "keep the buffer", which + reads as though skipping phase 2 restored something. It does not.)* +54. **Bus order is reported honestly, and independent mutations both + reconcile** (F1, Q#DR29), in two layers: + + - An `async_runtime.rs` unit test allocates two pending resource jobs + without dispatching workers, injects their successful + `WorkerReply`s B-then-A, and asserts `TickOutcome.resources` is + B-then-A; repeat A-then-B. This uses the module test's access to the + private bus and fails against sorting by job id/kind or claiming + dispatch/execution order. + - An integration test dispatches a rename and delete on **disjoint** + paths, waits for both, and asserts both registry effects occurred. + It fails against dropping/deduplicating one resource kind. + + The disjoint end state is confidence coverage, **not a claimed bite + against interdependent sequencing**: disjoint paths necessarily + commute. Rev 8's text said resolving a delete after an independent + rename would falsify the test, but the rename cannot change that + delete target. The unsupported overlapping-path case remains the + explicit residue below; no test pretends to solve it. +55. **`resource.renamed` and `resource.deleted` are `all-must-succeed`, + not short-circuit** (Q#DR28): with **two** subscribers registered + and the **first one raising**, the second still runs, and the error + is reported rather than swallowed. *(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.)* + +**No acceptance pins interdependent ordering, deliberately.** A test +cannot pin an order the mechanism does not establish (§6, F1): the +runtime records bus-arrival order and no execution token exists, so an +"interdependent mutations reconcile in execution order" test would +either be flaky or would pass by accident on a scheduler that happens to +cooperate. This is the same discipline item 20 applies to H1's open +dispatch-to-syscall interval — **state the gap, do not fake a pin for +it.** + +**Bite obligations.** Each of these must fail against a stated mutation: + +| Item | Mutation it must catch | +|---|---| +| 8 | `x` widened to consume `*` marks | +| 14 | `M`'s symlink refusal downgraded to a warning | +| 20 | the modified check run only once, before the prompt | +| 25 | the reconciliation moved to `_take_result` | +| 27 | `find_by_path`'s first match instead of every match | +| 28 | a string `starts_with` instead of a path-component prefix | +| 29 | explicit `BufferNameOrigin` replaced by rev 8's normalized-path heuristic | +| 30 | `rec.uri` updated without re-rooting the diagnostic view — and a remove-and-re-push, which passes a one-window test | +| 31 | the store clear without the `pending_routes` purge, so an in-flight response repopulates the old key | +| 31b | either late-write gate removed: diagnostics repopulate or one of the three stale flags returns | +| 31d | the exact-pair set replaced by a URI-only set, or teardown clears every server's tombstones | +| 33 | `handle.path` updated without the buffer name | +| 34 | the applier restoring by path instead of by buffer handle | +| 39 | a completion source added to `confirm` | +| 40 | the batch changed to dispatch-all-then-await | +| 50 | the hook fired for a **failed** rename, or fired with the un-normalized path | +| 51 | `reconcile_delete` calling only `EditorCore::kill_buffer`, so phase 2 never runs | +| 52 | `reconcile_delete` calling only `remove_buffer_and_fire`, so windows keep a removed id (this is `apply_resource_op` today) | +| 53b | the `editing_in_progress` preflight removed, exposing each of the three pre-refusal mutations | +| 54 | resources sorted by job id/kind instead of preserving injected bus order, or one resource kind dropped | +| 55 | either hook registered `short-circuit` | + +*(Rev 5's items 48–49 and their bites left with Q#DR25 — R1. They belong +to the `generated-buffer-immutability` lane, and §3.1 hands over the one +that matters: `M-x buffer.undo` leaving the listing intact is the pin +with bite, and the existing `dired_buffer_is_read_only_and_round_trips_input` +is **not** coverage because it passes either way.)* + +Note that **51 and 52 are a matched pair, and neither alone is +sufficient** — each existing removal path passes one and fails the +other, which is exactly why the framing had to name both phases (R4). + +`dired.lua` is an existing file now, so `scripts/bite`'s +swap-over-`git show` mode applies — but per #165's lesson, **commit +before biting**. Items 30, 33, and 34 came from round 2, item 31 from +round 3, items 50–53 from round 5, items 53b–55 from round 7, and item +31d plus 54's controlled-bus layer from round 8; each is a case where a +previous revision's design would have passed a weaker test. Note that +**item 20 has no bite for the interval it cannot close** (H1) — only for +the check it does make. + +## 14. Gates (per PR) + +The standard suite from `CLAUDE.md`, plus what this work touches. **2a's + gates are the widest of the three** — it changes `buffer.rs`, + `async_runtime.rs`, `lsp.rs`, the LSP stores, `view.rs`, `window.rs`, + `editor_core.rs`, and `lua_bindings`, so every LSP suite is in its + blast radius, not just the dired one: +`cargo fmt --check`; `cargo clippy --workspace --all-targets -- -D +warnings` as its own step; `cargo test --lib` and `--lib --features +crdt`; `dired_acceptance` (default **and** `crdt`); **`m8_1`, `m8_2`, +`m8_3` at unchanged counts** (the additivity gate, B3); +`m4_acceptance -- --skip basedpyright`; **`lsp_multi_root_acceptance`** +(B3b's gate, omitted from rev 2's list); `PMACS_REQUIRE_GPU=1 cargo test +-p pmacs-gpu`; the isolated-`XDG_CONFIG_HOME` workspace sweep with +`--no-fail-fast`; `git diff --check`. + +**Two gates rev 4 could not have listed, both required of every PR in +this stage (N3):** + +- **`journey_acceptance` at ≥ 24 tests, all passing.** It is a ratchet + by its own declaration, seven of its rows assert on dired, and its + step-3 and step-5 rows run *through* the surface Stage 2 modifies. + **Assert the count, not just the colour** — a row silently dropped is + exactly what the ratchet exists to prevent, and 2b changes the + contents those rows read. +- **`gpu_invocation_acceptance`**, which is where #183 put the GPU + journey row + (`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`). + It drives a **real daemon child process**, so it is in the blast + radius of any change to how a dired buffer is written or protected. + That makes it a **required gate for the + `generated-buffer-immutability` lane too**, which will make the + listing rope-level `read_only` — a daemon-side refusal is exactly what + this test's frontend would see. Note this row + is **not** in `journey_acceptance.rs`; running only that file leaves + the GPU half of the journey unpinned. + +*(Rev 5 also added `typed_edit_chain_acceptance` to 2b's list on +Q#DR25's account. With Q#DR25 gone (R1) that reason goes with it — dired +joins neither the chain nor `buffer.after-edit` (§2, N4), and 2b changes +no write path. It is not a 2b gate.)* + +**2a's dired-facing gate is `dired_acceptance` at an unchanged 25**, not +a grown one: 2a ships no dired code, so a moved count there means it +touched something it should not have. **2b's** is expected to grow by the +mark, operation and subscriber items. + +--- + +## 15. Numbered decisions + +- **Q#DR12** Marks are a per-handle table **keyed by basename**, values + `*` or `D`, pruned on every re-read following `*buffer-list*`'s + precedent. `render_entry` takes the mark as a second argument. (§3) +- **Q#DR13** *(narrowed in rev 2, F4; `w` moved in rev 3, G5)* + Operations fall into three classes: **set-based** (`D`, `M`, `w`, `C`) + target the marked set or the entry at point; **flag-based** (`x`) + consumes `D` flags only and never falls back to point; **point-based** + (`R` alone) acts at point regardless of marks and leaves marks + untouched. A vanished basename is dropped and reported. (§4) +- **Q#DR14** *(widened in rev 2, F1)* A rename is a **transaction across + every path owner**, not a buffer-field update. + `EditorCore::reconcile_rename` walks the whole registry, matches by + equality or **path-component** prefix, and updates both `file_path` + **and** `name`; **both** `pmacs.fs.rename`'s drain and + `apply_resource_op` call it, so they cannot drift. A new + **`resource.renamed(old, new)`** hook then lets path-keyed Lua + consumers reconcile — `lsp.lua` recomputes `rec.uri`, re-runs + `ensure_server` for a cross-root move, and issues didClose/didOpen; + `dired.lua` follows its handles. `tick` returns a **structured + `TickOutcome`** so settle identity and rename metadata stay in one + transaction (F6). Ordering is guaranteed: `_tick` runs before any + coroutine resumes. Pinned by a **no-await** rename. + *(Widened again in rev 3, G1/G3:* the LSP subscriber owns a full + teardown/re-attach — flush pending `didChange`, `didClose`, **drop all + five URI-keyed stores**, re-run `ensure_server`, `didOpen`, and + **re-root `DiagnosticView`**, whose URI is set once at construction; + and `apply_workspace_edit` must capture the **buffer handle** rather + than the path string, restoring with `switch_buffer` and **no path + fallback**, since no transaction can reach a captured Lua local.*) + *(Completed in rev 4, H2/H3:* the LSP half is one manager-level + **`forget_uri(sid, uri)`** covering **fourteen** URI-bearing stores plus + `documents`, **plus a `pending_routes` purge and awaiter drain** — an + in-flight response otherwise repopulates the old key after the clear; + and the view half is a **`View::rename_resource`** default-no-op hook + swept over `core.windows.values_mut()`, chosen over `set_uri` or + re-attach because it reaches passive windows and **preserves overlay + order by mutating in place**.*) (§5) +- **Q#DR15** *(widened in rev 8, F5)* Confirmation is + `pmacs.minibuffer.confirm { prompt, on_yes, on_no, on_cancel }` in a + new `builtin/runtime/minibuffer.lua`, with **no completion source**; + affirmative is `y`/`yes` case-insensitively and everything else — + including empty `RET` — is no. **Three outcomes, because + `pmacs.minibuffer.read` already has three**: accept-affirmative, + accept-negative and cancel. Rev 7 exposed only `on_yes` and so could + not express §8's copy contract, where declining **continues**. + **Cancel does not fall through to `on_no`** — with `on_cancel` + omitted, `C-g` runs nothing — because for `C` declining means *copy + the non-colliding entries* while cancelling means *abandon*, and + defaulting cancel to decline would make `C-g` perform a partial copy. + (§7, §8, §13 item 39) +- **Q#DR16** Batches **serialize** (await each op), a per-entry failure + does not abort, successful marks clear while failed marks persist, and + the listing reverts **once** at the end. (§9) +- **Q#DR17** *(revised in rev 4)* Stage 2 ships as **three** PRs, not + two: **2a** the resource-reconciliation transaction with **no dired + surface at all** (rename + delete reconciliation, `forget_uri` over + fourteen stores, `View::rename_resource`, the applier fix, + `pmacs.buffer.set_name`); **2b** the mark layer plus + `m u U t d x D R w M` on the five existing fs ops; **2c** + `mkdir`/`copy`/`remove_dir_all` with `+ C` and recursive delete. 2a + first because a bug in it is silent data loss, and because every + blocking finding across three rounds landed on it. (§10) +- **Q#DR18** *(new in rev 2, F2; seam in rev 3, G4; **policy rewritten + in rev 7, round 6**)* Deleting a path something holds. + + **The seam is unchanged and is not cancelled.** One shared + `EditorCore::reconcile_delete`, symmetric with `reconcile_rename`, + called by both the drain harvest and `apply_resource_op`, replacing + the latter's raw first-match lookup. Its walk is the **shared + prefix-aware, normalizing query** — every path-bound buffer, not the + first match; normalized once; component-aware `Path::starts_with` so + `/tree` does not match `/tree-sibling`. **That query is #186's Q#RD6**; + whichever lane lands first owns it and the other adopts it, which is + #186's own boundary wording, and `reconcile_rename` then shares it. + `remove` is still **harvested in the drain** like rename, firing + **`resource.deleted(path)`**, so a fire-and-forget remove reconciles + too. + + **The policy is now symmetric: both paths refuse.** Rev 6 had dired + refusing while an LSP-authored delete proceeded and orphaned the + buffer as accepted residue. **Withdrawn.** For the synchronous + `apply_resource_op` path a modified buffer in the affected set means + the delete is **refused before disk** — nothing removed, nothing + deleted — which is **PR #186's Q#RD1**, adopted here rather than + re-decided. dired refuses the entry on the same condition. + + **`DeleteReconcile.kept_modified` stays, and is not dead.** The + synchronous caller can no longer produce it, because the refusal + happens earlier; the **asynchronous** path still can, since dired + goes through `pmacs.fs.remove`, which dispatches a worker that #186's + in-applier guard never sees. dired's modified check therefore still + runs **before** the confirm *and again before each dispatch* — but it + is a **pre-dispatch check, not a lock** (H1), the interval from + dispatch to `remove_blocking` stays open, and the promise is stated at + that strength, as G6 forced for `R`. That interval is the whole of + this lane's remaining orphaning residue (§11). + + Deliberately diverges from Emacs, which orphans the buffer and lets + the next save resurrect the file — a conclusion **#186 reached + independently** for the LSP arm (its Q#RD1: refuse, do not prompt, do + not save, do not back up). (§6) +- **Q#DR19** *(new in rev 2, F3)* `M` (chmod) is **new scope** beyond the + parent's approved table and needs explicit approval. It **refuses + symlink entries**, because `chmod` follows links while the listing is + lstat-based, so the operation would change a different file and appear + to do nothing — the same reasoning the parent already applied to the + fixture's wdired perms edits and said "carries over unchanged". (§8) +- **Q#DR20** *(restored in rev 2, F3; corrected in rev 3, G5)* `w` (copy + filename to the kill ring) is carried forward from the parent's + approved Stage 2 table, which rev 1 dropped without saying so. It is + **set-based** — the parent approved the binding, and Emacs copies the + marked filenames; rev 2's point-only narrowing was an unapproved change + of its own. Non-destructive. (§8) +- **Q#DR21** *(new in rev 3, G2)* **`pmacs.buffer.set_name(buf, name)`** + is exposed. `Buffer::set_name` already exists in Rust and already + documents itself as for "save-as and **rename operations**"; §5 needs + the same capability for the `Buffer.name` half of the transaction; and + without it dired's **pathless** buffer cannot follow a directory + rename, which its own module doc says is why buffer-per-directory + exists. Uniqueness stays the caller's job, matching the Rust setter. + (§5) +- **Q#DR23** *(new in rev 4, H2; corrected in rev 5, W1/W2/W3)* The LSP + rename teardown is **one manager-level `forget_uri(sid, uri)`**, not + per-store calls at the Lua layer: fourteen call sites is how one gets + forgotten. It purges `pending_routes` by URI, **drain-cancels the + matching awaiters** (the existing contract at `lsp.rs:801-803` already + requires this wherever routes are purged), and clears all fourteen + stores plus `documents`, handling `locations_store`'s **kind** key and + `symbol_store`'s **scope** key specially. + *(Rev 5 corrects three facts underneath it, none of which change the + decision but any of which would break the implementation:* the + teardowns to model on are **`LspManager::start_generation`** + (`lsp.rs:1307-1345`) and **`LspManager::forget`** (`lsp.rs:3015-3042`) + — there is no `fn restart`; the drain to model on is + **`drain_external_cancelled`** (`lsp.rs:1561-1576`), the unconditional + server-scoped one, **not** the near-namesake + `drain_cancelled_externals` (`lsp.rs:1596-1645`), which only reaps + token-cancelled and timed-out awaiters and would therefore drain + nothing on a rename; and the route purge must **retain + `WorkspaceSymbol` unconditionally**, since it is the one + `ResponseRoute` variant of fifteen that carries a `query` rather than + a `uri`. Neither existing drain is URI-scoped, so `forget_uri`'s drain + joins route to awaiter on the **`rid`**, which is the only index + available.*) (§5) +- **Q#DR24** *(new in rev 4, H3)* Diagnostic re-rooting is a + **`View::rename_resource(&mut self, old, new)`** default-no-op hook, + joining `overlay_identity` and `clone_for_split` in the family #113 + added for cross-window overlay problems, swept over + `core.windows.values_mut()` exactly as overlay disposal already is + (`mod.rs:2044-2046`). Chosen over exposing `set_uri` (the field stays + private, and `View` has no downcast) and over tear-down-and-re-attach + (which loses composition order, and `_attach_view` cannot reach a + passive window at all). (§5) +- **Q#DR22** *(new in rev 3, G5)* **`pmacs.killring.push(text)`** is + exposed, with the semantics `copy()` already establishes for + non-region text: push the entry, mirror to the OS clipboard, and + **break the kill chain**. `push_entry` is private and `copy()` requires + a region, so `w` has no surface without it. (§8) +- **Q#DR25** *(added in rev 5; **withdrawn from this document** in rev + 6, R1)* Dired's listing becoming a genuinely immutable generated + buffer is **not Stage 2's decision to make**. The defect is real — + `M-x buffer.undo` empties a dired listing on `main` today — but it is + a **class** defect, not dired's: the same erroring-intercept-over-a- + writable-rope idiom appears in `listview.lua`, `compile.lua` and the + search/grep panel, and **no Lua caller anywhere sets `read_only`**, + because no Lua `set_read_only` exists to call. It is owned by the + **`generated-buffer-immutability`** lane. Stage 2 does not implement + it, does not gate on it, and carries no acceptance for it; §3.1 + records what that lane inherits from this document's re-scout, and why + the two lanes can land in either order without conflicting. (§3.1, + §11) +- **Q#DR26** *(new in rev 6, R3; ordering half superseded in rev 9, + F1)* The drain outcome is **one homogeneous sequence of settled + resource mutations**, not a field per mutation kind: + `TickOutcome { settled, resources: Vec }` with + `ResourceOp::{Rename { from, to }, Remove { path }}`, and `PendingJob` + carrying a single `Option`. One enum rather than two + `Option`s because two would admit a both-`Some` state that cannot + occur — `ResolvedTarget`'s own doc makes that argument at + `src/editor_core.rs:100-102`. Rev 5's `renames`-only shape could not + express deletion at all, though §6 required it to. **This decision + makes no ordering promise; Q#DR29 exclusively defines sequence + semantics as bus-arrival, not filesystem execution order.** (§5, §6) +- **Q#DR27** *(new in rev 6, R4)* "Kill a buffer" means **both** removal + phases, and `reconcile_delete` composes them for every id it kills: + `EditorCore::kill_buffer` (`src/editor_core.rs:4590` — last-buffer and + unknown-id refusals, side-window close, window redirect to a fallback, + registry removal) **then** `after_buffer_removed` + (`src/lua_bindings/mod.rs:1602` — keymaps, buffer-local config, folds, + `on_removed` callbacks). The composition already exists in exactly one + place, `pmacs.buffer.kill` (`mod.rs:5476-5491`), whose doc says it is + late-bound precisely because it needs the core to redirect windows; + `apply_resource_op` uses the **other**, incomplete path + (`remove_buffer_and_fire`, `mod.rs:1592`) and therefore leaves a + displaying window pointing at a removed id — a third defect on that + arm beside the missing dirty check and the first-match lookup. Phase 2 + needs `&Lua`, so `reconcile_delete` returns the killed ids and its + caller runs phase 2; `EditorCore` gains no Lua handle. Both refusals + (last buffer, mid-edit `ConcurrentEdit`) are reported and neither + aborts the rest of the batch. **And the mid-edit case is preflighted, + not caught**: `kill_buffer` clears `round_trip_buffers`, closes side + windows and redirects ordinary windows **before** + `BufferRegistry::remove` can refuse, so a caught `ConcurrentEdit` + leaves editor state already damaged. `reconcile_delete` therefore + checks `editing_in_progress` **before phase 1** and skips the buffer + — sound because phase 1 is pure `EditorCore` and holds no Lua handle, + so nothing can re-enter and begin an edit between check and removal. + (§6, §13 items 51–53, 53b) +- **Q#DR28** *(new in rev 8, from the round-7 sweep)* Both new hooks + register **`all-must-succeed`**, not `short-circuit`. `src/hook.rs` + defines three `HookKind`s and every declared hook picks one in + `builtin/hooks/default.lua`; rev 7 asserted only that the mechanism + existed and never said which. It matters: registered `short-circuit`, + a single `resource.renamed` subscriber returning falsey would stop the + fan-out and silently prevent every later subscriber from reconciling + — the typed-edit chain's trap arriving through an unlabelled door. + `all-must-succeed` reports a failing subscriber and still runs the + rest, and lets no subscriber claim the event. (§13 item 55) +- **Q#DR29** *(new in rev 8, F1)* **Reconciliation is + order-independent, and the framing promises nothing about the relative + order of two mutations in flight simultaneously.** + `TickOutcome.resources` is **bus-arrival order, not execution order** + — `tick` is a `try_recv` drain with no execution token — so rev 7's + ordered-sequence justification is withdrawn. This is safe because + independent mutations commute and no shipped in-tree path produces + interdependent concurrent ones (dired serializes; `apply_resource_op` + is synchronous; the fs primitives have zero in-tree production + callers). Because the primitives are public, 2a extends + `fs.lua:155-165` with the explicit rule that overlapping mutations + must be serialized by awaiting before the next dispatch. + **An execution-order token + under a mutation lock was rejected**: it would serialize every fs + mutation through one lock to close a hazard absent from shipped + callers and covered by a public caller-side precondition. Deferred + with a checkable trigger (§11). No static ordering rule is offered + because none works — the correct order depends on which mutation + actually ran first, which is exactly what is unknown. (§5, §6, §13 + item 54) +- **Q#DR30** *(new in rev 8, F3; rewritten in rev 9, F5)* A buffer's + name is path-derived only when its private + `BufferNameOrigin::{Explicit, PathDerived}` state says so; no string + or normalized-path heuristic infers provenance. Generic constructors, + `Buffer::set_name`, and `pmacs.buffer.set_name` set `Explicit`. + Audited path-backed load/new-file/save-as sites use + `set_path_derived_name`; reconciliation updates only that origin and + preserves it for later renames. This handles relative opens without + clobbering the collision `${cwd}/notes` explicitly named `notes`. + A path-derived relative name becomes the normalized absolute new path + after rename — stated rather than hidden, and confined to the + statusline and buffer list. (§5, §13 item 29) +- **Q#DR31** *(new in rev 8, F4; corrected in rev 9, F3/F4)* + State-creating writers uncorrelated to a request id are gated by one + **manager-owned, generation-scoped + `HashSet<(LspServerId, String)>`**, not by document membership. + `forget_uri` records the exact pair; + `absorb_publish_diagnostics(sid, …)` and + `mark_document_stale(sid, uri)` drop a matching late write; + `did_open` clears only that pair; and `start_generation`/`forget` + remove all and only the owning server's pairs. The private Lua stale + binding also takes `(sid, uri)`. `DiagnosticStore.by_uri` remains + URI-only, but that does not determine the tombstone key: the manager + has `sid` and gates **before** the store write. The diagnostic forget + path also drops the URI's never-pruned `epochs` entry. + A membership gate (`absorb only if in documents`) was rejected because + servers legitimately publish for files never opened, and it would drop + all of them; the precedent for the chosen shape is + `client.cancelled_rids` (`src/lsp.rs:2549`), the same + forget-then-drop-late-arrivals set with a request id instead of a URI. + `DiagStore`'s existing `epochs` cannot serve: `set()` bumps it on + every write with no epoch parameter, so a late notification looks + current. The set is reclaimed at reopen/generation teardown but has no + arbitrary size cap; calling it "bounded" would be false and eviction + would reopen the race. (§5, §13 items 31b–31d) + +## 16. Branch and PR plan + +Framing on `dired-stage2-framing`, kept after merge per the repo's +`-framing` convention. Then **three** implementation branches, each cut +fresh from `main` after the previous one merges — **not** stacked, since +each needs only a merged `main` and the arc has already paid for a stacked +retarget once (#104 → #105): + +1. **`dired-stage2a-reconciliation`** — the transaction, no dired surface. +2. **`dired-stage2b-marks`** — the mark and operation layer. +3. **`dired-stage2c-primitives`** — `mkdir`/`copy`/`remove_dir_all`. + +One feature, one branch, one PR; gates green before each; the ledger lane +and `docs/agent-handoff.md` §1 updated per their own protocols as each +lands. + +**Naming 2a for the substrate, not for dired**, following #161's +precedent: its diff contains no dired code, and a cross-cutting +correctness fix to rename/delete reconciliation must not be reviewable +only as a dired feature. The PR body should lead with the defects it +closes on `main` — the **workspace-edit phantom buffer**, the **raw +first-match lookup** shared by both `apply_resource_op` arms, and the +**incomplete removal lifecycle** (Q#DR27) — because none needs dired to +be worth fixing. *(Rev 7: it should **not** lead with the +LSP-authored delete that destroys unsaved work. That is **PR #186's**, +and 2a's body should say so and cite it rather than appearing to claim +it.)* + +**Ledger note (rewritten in rev 8 — P2; the rev 5 text was stale and +self-contradictory).** Two things it got wrong and this corrects: + +- **This branch no longer touches only the framing file.** It has + carried `docs/active-work.md`'s dired Stage 2 lane since **rev 6**, + which is when the ledger PR (#185) merged and the lane moved onto this + PR. The rev 5 text still claimed single-file scope while the very same + revision was editing the ledger. The reason the lane rides this PR + rather than a standalone refresh is unchanged: with several PRs open, + a separate ledger PR re-conflicts on every merge, and the ledger's own + protocol puts a lane refresh with the work. +- **The line counts were quoted, not measured.** Rev 5 described the + document as *1,570 lines* — a figure from two revisions earlier — and + the ledger in turn reported *~2,630*. **Measured at this revision: + `docs/dired-stage2-framing.md` is 3,844 lines and + `docs/active-work.md` is 1,003.** *A census is a reading, not a + constant*, and that applies to a framing's count of its own size + exactly as it applies to its count of the tree. Neither number above + is copied from anywhere; both were run against the tree being pushed. + +`docs/agent-handoff.md` and `COHERENCE.md` remain untouched by this +branch. + +### Ownership warning: 2a must not run concurrently with Journey Stage 1b + +**2a overlaps `src/editor_core.rs`, `builtin/runtime/lsp.lua`, and the +URI-keyed LSP state with other coherence work in flight.** +`COHERENCE.md` §20 names Journey Stage 1b as Priority 1's remainder — +compile defaults, **LSP spawn-failure surfacing**, bindings, and a +welcome buffer — and the LSP-failure half lands in the same files 2a +rewrites, while 1b's compile/binding half touches `editor_core.rs`. Two +branches editing `lsp.lua`'s attachment lifecycle at once is the +worst-case shape for this repo: the conflicts are semantic rather than +textual, so a clean `git merge` proves nothing. + +**Neither stage may start until those files are assigned.** The +resolution is cheap if taken up front — 1b's LSP work is a *reporting* +change (surface the spawn failure with guidance) and 2a's is a +*lifecycle* change (teardown and re-attach on rename), so they can be +sequenced in either order provided only one is open at a time. Taken +late it is three merge rounds, which this arc has already paid twice. + +Stage 2b and 2c carry no such overlap, but **2b's file list is wider +than rev 5 said** (R6). It is `dired.lua`'s mark, operation and +subscriber layer; the new `builtin/runtime/minibuffer.lua`; the +`pmacs.killring.push` binding; **and `src/editor.rs`**, because +`minibuffer.lua` has to join the explicit `include_str!` load sequence +there (`src/editor.rs:395-660`, ~30 entries) and must be ordered +**before** `dired.lua`. That is a one-line addition to a file no other +lane in flight is editing, so it changes no ownership conclusion — but +"2b is `dired.lua` plus one killring binding" was simply false, and a +reader sizing the ownership warning off it would have sized it wrong. +2c is three additive `pmacs.fs` primitives plus their two dired +commands. diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md new file mode 100644 index 0000000..8706554 --- /dev/null +++ b/docs/resource-op-delete-guard-framing.md @@ -0,0 +1,1649 @@ +# Framing — `apply_resource_op` delete destroys unsaved work + +**Revision 5.** Status: **PROPOSED — needs explicit user approval before +implementation. DO NOT implement, DO NOT merge.** Lane: +`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, +based on `githubsucks/main` @ `7586905`. + +Revision 5 removes volatile sibling-branch counts from the normative +contract. A count is a reading, not a dependency; where history retains +one, it names the revision at which it was measured. + +This is a live data-loss bug, reproduced four ways against `ad41cf1` +(§1.1). A language server can destroy a buffer's unsaved edits *and* +the file that would have held them, with no prompt, no status message, +and no error return. + +**This PR is the implementation PR.** Revision 2 drops revision 1's +framing-PR-then-implementation-PR plan, which conflicted with +one-feature/one-branch/one-PR. The framing is revised in place; once the +design is approved, the implementation lands on this same branch and in +this same PR (§8). + +## Revision history + +### Revision 4 → 5, after review round 4 + +Round 4 accepted the core and both surfaced decisions — the defensive +parse stub and fail-closed filesystem uncertainty — but found two +contract defects plus a ledger-ownership defect. All accepted. + +**P1 — the narrowed reporting promise had one stale consumer.** Q#RD7 +correctly says the unattended path always **attempts** a response while +the channel remains live, but §2.1 still said it **always answers**. +That normative consumer now uses the exact Q#RD7 promise. The revision +4 audit missed it because the literal search `always answers` did not +match Markdown's `**always** answers`. §1.15 therefore adds one more +procedural rule: search normalized prose or term stems, not only an +exact rendered phrase containing markup. + +**P1 — Q#RD12 called a three-row verdict total when it was not.** It +omitted absent-without-ignore, defined `conflict` as a named modified +buffer while also assigning stat failures with no buffer to it, and did +not carry Q#RD2's `editing_in_progress` condition into the shared query. +The verdict is now `no-op` / `clear` / `refuse`, with a required message +on every refusal and an optional buffer name only for buffer-caused +refusals. The total mapping is explicit: absent-plus-ignore is `no-op`; +absent without ignore and an unanswerable stat are `refuse`; a modified +or mid-edit affected buffer is `refuse`; only a present target with a +clean, quiescent affected set is `clear`. Criteria 11c and 11d pin the +two filesystem refusal directions before any earlier batch op mutates. + +**P1 — merge order was being used as ledger ownership.** Revision 4 +rewrote #171's full lane entry from #186. #171 revision 8 landed 67 +seconds earlier with different state, making #186's copy stale before +it was pushed; #186's own `3 / 0` count was also one short because the +revision commit itself had not yet been counted. The sibling block is +restored to `main`'s version, so #186 no longer changes it. #171 owns +its entry on its branch. This lane records only the stable split it +depends on and checks the pushed sibling framing for semantic changes, +without copying its volatile head/count/line state. + +**Coordination decision.** #186 lands before #171 for a product reason, +not a merge convenience: it closes live data loss, and #171 explicitly +adopts its refusal and shared query. #171 integrates the result and +reconciles its own ledger entry. The generated-buffer lane is +independent; `journey-stage1a-directory-open` has no unmerged ledger +delta. + +### Revision 3 → 4, after review round 3 + +Round 3 accepted the core — pre-filesystem refusal, four-phase ordering, +the #171 split, Q#RD10 — and raised four P1s and a P2. All accepted. + +**P1 — the lane state and ledger were still false.** Re-measured: + +``` +$ git rev-list --left-right --count fd7ae37...7586905 +13 2 +``` + +#171 is **two commits behind**, not zero. Revision 3's "0 behind" was +measured against `ad41cf1` and reported in present tense after `main` +had moved. The ledger additionally still described #186 as revision 2 on +`ad41cf1` and retained the superseded `ab42a79` / 153-behind entry for +#171. **Both lane entries are now corrected in place** rather than +having a correction layered above stale ground truth, which is what +produced a self-contradicting ledger. §1.12 carries its count as pasted +command output. + +**P1 — the withdrawn LSP claim survived in the normative decision.** +Q#RD3 still called partial application "`FailureHandlingKind.Abort`, the +strategy the spec itself assigns to any edit containing resource +changes", and §1.11 called `Abort` the default "by omission". Both are +withdrawn. Both sites now say only that **verified pmacs behaviour +resembles abort-style application**, with the justification resting on +§1.6's reproduction. This mattered more than an ordinary error because +§1.15's audit had certified the document clean while the claim was still +load-bearing three sections away. + +**P1 — acceptance 15 had no reachable payload.** +`WorkspaceEditResponse::from_lsp_value` (`src/rename.rs:95`) returns +`Self`, its doc says "A `null` / shapeless result yields an empty +response", and the binding's only `?` is `lua_to_json` over a value that +arrived through `json_to_lua`. **No server payload can make the parse +fail**, so the criterion could not fail either. **Decision (Q#RD11): +keep the wrap, drive the test with an explicit throwing stub, and label +it defensive.** Q#RD7's promise is correspondingly narrowed to **"always +attempts a response while the response channel remains live"** — +`send_response` is itself under an ignored `pcall` +(`builtin/runtime/lsp.lua:1843`). + +**P1 — absent-plus-ignore had no synchronous seam.** `pmacs.fs.stat` +(`builtin/runtime/fs.lua:133`) dispatches async; the only synchronous +filesystem binding is `canonicalize` +(`src/lua_bindings/mod.rs:6743`), which resolves symlinks and returns +`nil` for a dangling one — so it disagrees with the primitive's +`symlink_metadata` on precisely the input this query turns on. **New +Q#RD12** specifies a structured Rust-backed verdict (`no-op` / `clear` / +`conflict`) evaluated with the same `symlink_metadata` call, with an +error contract that fails toward refusal. New criteria **11a** +(present + ignore + modified ⇒ still refused) and **11b** (dangling +symlink counts as present) supply the missing opposite direction. + +**P2 — acceptance and file-scope bookkeeping.** Criterion 14's +"fails in both directions" was **false**: with both duplicate buffers +clean, the setup cannot distinguish first-match validation from full +validation. **The claim is fixed, not the setup** — criterion 6 already +pins validation breadth, and duplicating it would add no bite; the two +are now labelled by which half of Q#RD10 each covers. §8's touch table +and §7's gate list are reconciled: `lsp_dispatch_seams_acceptance` was +named in one and omitted from the other, and the parse-stub work was +missing from both. + +**Sweep — corrections applied at one site while a dependent site kept +the old claim.** Whole-document pass over every claim withdrawn or +revised in revisions 2 and 3, checked at each consuming site rather than +only where defined. **Count: 4.** Two were the P1-2 sites above (Q#RD3, +§1.11). Two were the stale #171 count, which had propagated into both +the revision-history entry and §1.12. Claims checked and found clean at +every consuming site: rev 1's buffer-first ordering; "whole-batch +atomicity"; `find_buffer_for_path` as the lookup; primitive-only +`ignore_if_not_exists`; the three withdrawn impossibility claims about +prompting; "no `*Messages*`/`*warnings*` buffer"; "only path 1 is +unattended"; mode (d) as unowned; and Q#RD9. Each of those appears only +in withdrawal text or in correctly-scoped ground truth. + +### Revision 2 → 3, after review round 2 + +Round 2 confirmed everything central from round 1 as fixed and raised +four P1s. All four accepted; two sweeps run. + +**P1-1 — the ownership boundary was stale.** Revision 2 described PR +#171 as "OPEN, STALE, 153 commits behind, under re-scout" and said it +claimed the **rename** side only. Re-checked directly: #171 is at +**revision 7, `fd7ae37`, merge-base `ad41cf1`** — not stale. *(That +sentence originally read "0 commits behind"; it was measured before +`main` moved and is corrected in the rev 3 → 4 section above.)* Revision 6 had assigned **both** rename and delete to Stage 2a, +with the **opposite** policy: its `reconcile_delete` "kills unmodified +buffers and keeps modified ones alive", i.e. the file is deleted and the +modified buffer orphaned, and its §11 named that orphaning as accepted +residue. Two lanes, opposite answers, same event. §1.12 and Q#RD5 now +carry the settled split verbatim, and #171 revision 7 has adopted it +from the other side. + +**P1-2 — the LSP failure-handling claim was wrong.** Revision 2 said the +spec "assigns `Abort` to any edit containing resource operations" and +rested B2 on it. **It does not.** Recovery is described by the client's +advertised `failureHandling`; `Abort` is one of four strategies, and +only `TextOnlyTransactional` degrades to abort when resource changes are +present. pmacs advertises **none** (§1.11, established by revision 2's +own sweep), so the spec assigns pmacs no strategy at all. §1.7 and B2 are +rewritten to stand on **verified pmacs behaviour** — §1.6's reproduced +partial batch — rather than on borrowed protocol authority. This was the +document's second external-spec overclaim; every external claim now +carries a direct quote or is marked not established (§1.15). + +**P1-3 — Q#RD7 had no implementable, tested reporting seam.** Three gaps +confirmed by reading: `_parse_workspace_edit` is called at +`builtin/runtime/lsp.lua:1835`, **outside** the `apply_workspace_edit` +call revision 2 proposed to wrap, and it is fallible +(`lua_to_json(edit)?`, `src/lua_bindings/mod.rs:10161`), so "always +answers" was false for a parse failure; `append_to_errors_buffer` +(`src/lua.rs:401`) is **private**, so revision 2's promise to log +through it was not implementable from where it was made, and a Lua +preflight rejection never reaches Rust anyway; and acceptance 13 tested +the response but not the promised `*errors*` trace. Q#RD7 is rewritten +around **one seam at the server-request boundary**, and of the two +options offered, this revision **picks wrapping parse-plus-apply** +rather than narrowing the claim — the fix is one line up from the +existing wrap and it makes "always answers" true rather than qualified. + +**P1-4 — clean duplicate reconciliation was unspecified.** Revision 2 +said validation scans every match but never said what reconciliation +does afterwards. Now an explicit decision, **Q#RD10**, taking the user's +steer: **validate every match, reconcile only today's first exact-path +match.** Widening would enlarge the parked lifecycle defect that Q#RD5 +exists to contain; the surviving clean duplicate is named as residue +handed to #171, not left silent. Acceptance 14 pins it in both +directions. + +**Sweep — external claims.** Every non-repo claim re-audited (§1.15). +One was a paraphrase standing in for a quote: revision 2 asserted that +"in Emacs `kill-buffer` on a modified file-visiting buffer prompts" +without establishing it. It is true, but **not for the reason a reader +would assume**, and the precise version matters to the argument — see +§1.13, which now quotes `Fkill_buffer` and the `INTERACTIVE` macro. + +**Sweep — cross-lane claims.** Beyond P1-1: revision 2's Q#RD8 said mode +(d) "needs its own lane and a census". **It has one** — #171's +`reconcile_delete` composes both removal phases and adopts the trap +verbatim. Q#RD8 and §6 now name the owner instead of describing the +defect as unowned. Q#RD6 additionally **claims the shared query +explicitly**, per the boundary's "whichever lands first owns the query", +so the duplicate resolves in one direction. And a gap neither lane +closes — `pmacs.fs.remove` has no dirty check of its own — is now named +as explicitly out of scope with its owner (§6), because "both lanes +guard deletion" otherwise reads as the primitive being guarded. + +### Revision 1 → 2, after review round 1 + +The refusal strategy was approved in principle; revision 1 as written +was not. Q#RD1 (refuse unconditionally) and Q#RD5 (take prefix-awareness +now) are **settled yes**; Q#RD9 is **settled no** and is withdrawn. +Six blocking points, all accepted, plus three further overclaims found +by the sweep the review asked for. + +1. **Q#RD2 conflated inspection with removal — rewritten.** Revision 1 + proposed removing the buffer *before* the filesystem call. That fires + arbitrary Lua `on_removed` callbacks while the file still exists, + destroying today's useful invariant that a subscriber observes the + path already gone, and it accepts losing the buffer if the deletion + then fails. The correct sequence separates the two: + `stat/no-op → enumerate and validate → mutate filesystem → reconcile`. + Validation needs no removal, so a failed deletion leaves the buffer + intact automatically. **The review is right and revision 1 was + wrong.** `Buffer::editing_in_progress()` (`src/buffer.rs:747`) is a + public getter, so the re-entrancy condition can also be checked + during validation rather than discovered during removal. +2. **Q#RD3 overclaimed whole-batch atomicity — rewritten and + downgraded.** Revision 1 called the preflight "whole-batch + atomicity" and said "**nothing** in the batch is mutated". That is + false for a sequential batch: an earlier text edit can dirty a + clean buffer, and an earlier rename can move a modified buffer + *into* a later delete's subtree, after the snapshot was taken. The + preflight is now described as an **early conflict check** — a + cheap, honest first filter, not a transaction (§2.1, Q#RD3). The + real robustness comes from per-op `pcall` and an always-sent + response (Q#RD7). +3. **The lookup cannot be `EditorCore::find_buffer_for_path` — accepted + and independently re-verified.** It normalizes but delegates to + `BufferRegistry::find_by_path` (`src/buffer_registry.rs:168`), whose + own doc says "First buffer bound to `path`" — singular, insertion + order. And `pmacs.buffer.from_file` (`src/lua_bindings/mod.rs:3112`) + calls `create_from_bytes` with no dedup check, so duplicate + path-bound buffers are reachable from public Lua. A clean first match + hides a modified second. Q#RD6 now requires a full scan. +4. **Do not expand the parked lifecycle defect — accepted.** Revision 1 + left it ambiguous whether a recursive delete should reconcile + descendants. It must not: mode (d)'s dangling-window and + last-buffer defects would be promoted from exact-path to tree-wide. + Q#RD5 now says explicitly that the tree is **inspected** but only the + exact path is **reconciled**. +5. **Q#RD4 must hold at both layers — accepted.** Revision 1 applied the + `ignore_if_not_exists` early return only to the primitive. The Lua + preflight must not reject an absent path merely because a modified + buffer still names it. +6. **The prompt argument was overclaimed — rewritten, and this was my + error.** Revision 1 said prompting was "architecturally + unavailable", "the only option that is *possible*", and that the + server-initiated path "cannot produce that answer". All three are + wrong. `pmacs.lsp.send_response` (`src/lua_bindings/mod.rs:9680`) + takes `request_id` as an ordinary value, so a `workspace/applyEdit` + **can** be answered on a later tick; and a callback continuation + would reuse the **existing** minibuffer shadow (rung 4), not add a + seventh rung. The honest claim is that prompting is *expensive and + separately scoped*, and §2.2 now argues only that, on evidence + (§1.8). + +**Three further overclaims found by the sweep** (the review asked for +the defect class, not just the three cited instances — all three are +the same shape: revision 1 asserted an absence or a guarantee it had +not established): + +7. **"There is no `*Messages*` buffer and no `*warnings*` buffer" was + misleading by omission.** There is a durable append-only error + surface: `LuaHost::append_to_errors_buffer` (`src/lua.rs:401`) + writing `*errors*` (`ERRORS_BUFFER_NAME`, `src/lua.rs:32`; 49 + references across `src/` and `builtin/`), already used by + `log_hook_error` (`src/lua_bindings/mod.rs:6061`), + `log_statusline_provider_error` (`:6099`) and `log_buffer_removed_error`. + This **improves the design**: Q#RD7 now records the refusal there + too, so it survives the status line being cleared and leaves a trace + on the unattended path. +8. **"Only path 1 is fully unattended" understated the problem.** + Revision 1 said a raise on paths 2 and 3 "propagates out of the + `pmacs.async` coroutine" without establishing where it lands. It + lands nowhere useful: `step` (`builtin/runtime/async.lua:196`) routes + an uncaught coroutine error to `pmacs.error`, **which is undefined** + — 11 call sites in `builtin/`, zero definitions — so the `error(...)` + fallback re-raises at the spawn site. **No caller reliably reports a + raise**, which strengthens rather than weakens point 2's requirement. +9. **pmacs advertises no `workspace.workspaceEdit` capability at all.** + `default_client_capabilities` (`src/lsp.rs:3242`) sends + `"applyEdit": true` but no `workspaceEdit` object, so no + `documentChanges`, no `resourceOperations`, and no `failureHandling` + — `grep -rn failureHandling` over the tree returns **0**. Revision 1 + discussed batch semantics without noting that pmacs declares no + failure-handling strategy. Named as ground truth (§1.11); **not + fixed here** (§6). + +### Revision 1 + +First cut. Established the bug and the four modes, the caller +inventory, the Emacs prior art, and the refusal recommendation. + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 6, "Receive language intelligence"** (§2), and by + consequence **step 5, "Edit immediately"** — the loss is of exactly + the edits step 5 grades as "genuinely excellent". §2's verdict table + grades step 6 **Partial**; this lane does not raise that grade, it + removes a way the step can destroy the user's work. Serves + **Priority 1** ("treat regressions as release blockers") as a + correctness floor rather than a feature. +- **Interaction islands: none added.** The recommended design adds no + modal surface at all — it refuses and reports. *Revision 2 correction:* + revision 1 additionally claimed that the rejected prompt option would + have added a seventh dispatcher rung. It would not; a callback + continuation reuses the existing minibuffer shadow (rung 4). The + count stays at six either way, and §6's island budget is **not** an + argument against prompting (§2.2). +- **Config registry: not adopted.** No knob is proposed; the refusal is + unconditional (Q#RD1). An `lsp.confirm-server-edits`-style setting is + the natural future adopter and is parked in §6. +- **Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. The `docs/active-work.md` lane for this + PR does ride it, per that file's "When a PR is opened, give it a + lane." + + +## 1. Ground truth (scouted and verified @ `ad41cf1`) + +### 1.1 The bug, reproduced + +`pmacs.buffer.apply_resource_op` with `kind = "delete"` destroys a +modified buffer and the file backing it. Reproduced in this worktree by +throwaway acceptance tests against `ad41cf1`, written, run, then removed +— they are the model for §5's pins, not shipped artefacts. Four modes: + +**(a) The reported bug.** + +``` +PRE: modified=true text="UNSAVED EDIT ORIGINAL ON DISK\n" +apply_resource_op result: Ok(()) +buffers before=3 after=2 +file still on disk? false +``` + +The call returns `Ok(())`. The file is gone. The buffer is gone. The +text existed in exactly one place and now exists nowhere. + +**(b) `ignore_if_not_exists = true` destroys the buffer having done no +filesystem work at all.** When the path is already absent, the delete +arm skips the `remove_file` — and falls through to the buffer +reconciliation anyway: + +``` +file gone; buffers=3; buffer holds the only copy +result: Ok(()) +buffers after=2 +CONFIRMED: ignore_if_not_exists=true did ZERO fs work yet still destroyed the only copy. +``` + +The `create` arm returns early (`return Ok(())`) under the analogous +`ignore_if_exists` condition; the delete arm's `Err(NotFound)` branch +does not return. + +**(c) `recursive = true` fails in the opposite direction.** + +``` +recursive delete result: Ok(()) +inner file exists? false +buffers before=3 after=3 +inner buffer still in registry? true +``` + +The tree goes; no buffer is reconciled, because the lookup is for the +directory path and buffers hold file paths. The most destructive arm +does the least reconciliation. Here the data survives — in an orphaned +buffer — which is strictly safer than (a), and is why the fix must not +be "make delete behave like the recursive case". + +**(d) Removal is not `kill_buffer`.** + +``` +victim is the active buffer? true +window.buffer():is_valid() after delete => Ok(Boolean(false)) +editor.file_path() after delete => Ok(Nil) +``` +``` +buffers now: 1 +delete of the LAST buffer: Ok(()) +buffers after: 0 (0 => registry driven empty) +``` + +`EditorCore::kill_buffer` (`src/editor_core.rs:4590`) refuses the last +buffer and rebinds every window to a fallback. This path does neither. +**(d) is parked** (§6, Q#RD8) and, per Q#RD5, must not be *widened* by +this lane. + +### 1.2 There is no dirty check at any link in the chain + +1. **The delete arm** (`src/lua_bindings/mod.rs:3313`) — stats, deletes, + then `find_by_path`, then `remove_buffer_and_fire`. No `is_modified`. +2. **`remove_buffer_and_fire`** (`:1592`) — `registry.remove(id)` then + `after_buffer_removed` (`:1602`), which clears keymaps, config, folds + and fires `on_removed`. No dirty check. +3. **`BufferRegistry::remove`** (`src/buffer_registry.rs:127`) — one + guard, and not this one: + + ```rust + if let Some(buf) = self.buffers.get(&id) + && buf.editing_in_progress() + { + return Err(RegistryError::ConcurrentEdit { ... }); + } + ``` + + That refuses re-entrant removal from inside an edit intercept + (T M7.4). It says nothing about unsaved content. + +The report's description is **accurate at every link**. Modes (b), (c) +and (d) are additional. + +### 1.3 The ordering is disk-first, so no guard placed later can help + +The arm performs the irreversible filesystem operation **before** it has +looked for a buffer. **A fix that adds a dirty check to the existing +buffer-reconcile block is not a fix** — it converts mode (a) into mode +(c), and the user's file is still gone. The check must happen in a phase +that precedes the filesystem call, which is what Q#RD2 introduces. + +### 1.4 `is_modified` is available, singular, and the lookup around it is not + +- One field, `Buffer::is_modified: bool` (`src/buffer.rs:164`); one + accessor (`:473`); one public mutator `mark_clean` (`:488`). + `Buffer::editing_in_progress()` (`:747`) is likewise a public getter, + so both conditions Q#RD2 needs are inspectable without mutating. +- **Zero features refuse or confirm on unsaved state today.** + `kill_buffer`, `editor.quit` (whose `editor.before-quit` veto hook has + no subscriber) and `dired.revert` all ignore it; the only behavioural + consumer is autosave's `gather` filter (`src/autosave.rs:363`), which + *includes* rather than refuses. **This lane introduces the first + refusal keyed on unsaved state** and should be read as setting that + precedent. +- **The registry lookup is singular and duplicates are reachable.** + `BufferRegistry::find_by_path` (`src/buffer_registry.rs:168`) returns + the *first* match in insertion order — its own doc says "First buffer + bound to `path`". `EditorCore::find_buffer_for_path` + (`src/editor_core.rs:935`) normalizes and then delegates to it, so it + inherits the singularity. And duplicates are creatable from public + Lua: `pmacs.buffer.find_or_open` (`src/lua_bindings/mod.rs:3162`) + dedups via `find_by_path`, but **`pmacs.buffer.from_file` (`:3112`) + does not** — it calls `create_from_bytes` unconditionally and then + `set_buffer_path`. Two `from_file` calls on one path yield two + path-bound buffers, and a clean first match hides a modified second. + This is why Q#RD6 requires a full scan rather than the existing + wrapper. +- Lua reaches modified state as `buf:is_modified()` (`:1261`) and + `pmacs.describe.buffer(id).modified` (`:6359`). It is **not** a key on + the `pmacs.buffer` module table, so the preflight needs a new query + (Q#RD3). + +### 1.5 Who calls this, and what happens to a raised error + +One production caller: `apply_workspace_edit` +(`builtin/runtime/lsp.lua:1301`), at `:1346`. Three callers of that: + +| # | Call site | Origin | Disposition of a raise | +|---|---|---|---| +| 1 | `handle_server_requests` (`lsp.lua:1815`), call at `:1836` | **server-initiated** `workspace/applyEdit` | **Swallowed.** The pump runs under `pcall(handle_server_requests)` (`:1892`); the raise unwinds past the `pcall(pmacs.lsp.send_response, ...)` that answers the request, so the user sees nothing **and the server is never answered**. | +| 2 | LSP rename (`lsp.lua:2311`) | user, `M-x` | Raises out of the `pmacs.async` coroutine — see below. | +| 3 | code action apply (`lsp.lua:2373`) | user, `M-x` | as #2. | + +**Revision 2 correction.** Revision 1 called only path 1 unattended. +Paths 2 and 3 are no better: `step` (`builtin/runtime/async.lua:196`) +handles an uncaught coroutine error by calling `pmacs.error` if it +exists and `error(...)` otherwise — + +```lua +if not ok then + if pmacs.error then + pmacs.error("pmacs.async: coroutine raised: " .. tostring(yielded)) + else + error("pmacs.async: coroutine raised: " .. tostring(yielded)) + end + return +end +``` + +— and **`pmacs.error` is undefined**: 11 call sites across `builtin/`, +zero definitions. So the fallback always runs and re-raises at the spawn +site. **No caller reliably surfaces a raise to the user.** Hence Q#RD7: +the refusal travels as a value, never as an exception alone. + +`src/rename.rs:25` documents the division of labour — `rename.rs` parses +and never mutates; Lua drives the primitives "so the application +strategy stays configurable". That strategy is what this framing picks. + +### 1.6 A partial batch is already possible today — verified + +The applier's loop (`lsp.lua:1340-1349`) calls the primitive +unprotected. Two delete ops where the second raises: + +``` +batch result: Err(... "apply_resource_op delete: No such file or directory (os error 2)") +a.txt still exists? false (false ⇒ partial batch) +``` + +The first op stayed applied. **Partial application on I/O error is the +status quo**, not something a refusal introduces. Data loss is strictly +worse than a failure class the code already tolerates. + +It also shows the preflight's contract is narrower than its comment +implies. `lsp.lua:1287-1291` says the applier "refuses to mutate +*anything* unless every URI it touches resolves to a real file path +first". True — but URI resolution is the *only* precondition; the plan +loop (`:1302-1336`) validates nothing about the filesystem or the +registry. That loop is where Q#RD3's conflict check goes. + +### 1.7 The batch is sequential; the protocol assigns pmacs no failure strategy + +Claims about **the LSP specification** (3.18), each a direct quote, not +a paraphrase — and note carefully what they do *not* say. + +**Sequential execution** — this is the load-bearing one, and it is +unconditional: + +> "If resource operations are present, clients need to execute the +> operations in the order in which they are provided." + +**Failure recovery is the client's declared choice, not a fixed rule:** + +> "How the client recovers from the failure is described by the client +> capability: `workspace.workspaceEdit.failureHandling`" + +`FailureHandlingKind` has four values, quoted from the spec's own +namespace block: + +| Value | Doc comment (verbatim) | +|---|---| +| `Abort` | "Applying the workspace change is simply aborted if one of the changes provided fails. All operations executed before the failing operation stay executed." | +| `Transactional` | "All operations are executed transactionally. That means they either all succeed or no changes at all are applied to the workspace." | +| `TextOnlyTransactional` | "If the workspace edit contains only textual file changes they are executed transactionally. If resource changes (create, rename or delete file) are part of the change the failure handling strategy is abort." | +| `Undo` | "The client tries to undo the operations already executed. But there is no guarantee that this is succeeding." | + +**Revision 3 correction.** Revision 2 read this as "the protocol assigns +`Abort` to any edit containing resource operations". **That is wrong.** +`Abort` is one of four strategies a *client* may advertise; +`Transactional` covers all operations and `Undo` attempts rollback. Only +`TextOnlyTransactional` degrades to abort in the presence of resource +changes, and that degradation is a property of *that* strategy, not of +resource operations in general. + +**And pmacs advertises none of them** (§1.11). So the specification does +not tell us what pmacs should do here; it tells us the question is the +client's to answer. Revision 2 borrowed authority it did not have. + +What survives, and is sufficient: **sequential execution is +unconditional**, which is why a snapshot preflight is necessarily +incomplete — an earlier op can change the facts a later op's +precondition was evaluated against. That, plus §1.6's *verified* pmacs +behaviour (a partial batch already happens today on I/O error), is the +whole basis for Q#RD3 and B2. The justification is that **partial +application is already what pmacs does and is safer than data loss** — +not that the protocol blesses it. + +### 1.8 What prompting would actually cost — corrected + +Revision 1 called prompting impossible. It is not. Establishing what is +and is not true: + +**True, and verified:** +- The primitive cannot suspend. `apply_resource_op` is a synchronous + Rust closure performing its `std::fs` calls inline; there is no yield + point. A prompt therefore cannot be issued *from inside it* — the + applier would have to be restructured into a continuation chain. +- `pmacs.minibuffer.read` (`src/lua_bindings/mod.rs:13380`) is + asynchronous-by-callback, and `Minibuffer::accept` + (`src/minibuffer.rs:334`) deliberately *returns* the callback rather + than invoking it, because "firing user code from inside the minibuffer + would re-enter the registry" (`:332`). +- **The minibuffer is a single slot that replaces without asking.** + `Minibuffer::session: Option` (`src/minibuffer.rs:71`), + and `begin` (`:106`) is documented "**Replaces any existing + session**". A prompt raised mid-batch while the user has a minibuffer + open silently destroys the in-flight prompt and its callbacks. +- **There is no `y_or_n` helper in the tree** — a named deferral + (`docs/dired-framing.md:854`). + +**False, as revision 1 had it:** +- *"The server-initiated path cannot produce that answer."* It can. + `pmacs.lsp.send_response` (`src/lua_bindings/mod.rs:9680`) takes + `(server_id, request_id, result, err)` as ordinary values; nothing + binds it to the pump's call frame, and `request_id` arrives on the + event as a plain Lua value that can be stashed. A `workspace/applyEdit` + **can** be answered on a later tick. +- *"It costs a seventh dispatcher shadow."* It does not. The minibuffer + is already rung 4; a continuation reuses it. + +**So the honest case against prompting** (§2.2) is scope, not +possibility: queuing, cancellation, collision with an already-active +single-slot minibuffer, and revalidation of every precondition after the +user turn — because the world moves during the turn, which is §1.7's +problem again, only worse. + +### 1.9 Autosave cannot serve as a pre-delete backup + +Verified against `src/autosave.rs`: there is no per-buffer write entry +point (the only public writer is `sweep`, `:261`, which walks the whole +registry); `sweep` skips clean buffers (`:363`); **removing a buffer +purges its recovery file** — the `on_removed` callback registered at +`builtin/runtime/autosave.lua:167` calls `discard_buffer` (`:511`), with +a sweep-time GC backstop (`:290-306`), pinned by +`tests/autosave_acceptance.rs:702`; and deleting the file flips the +recovery to `Stale`, which is never auto-offered. + +### 1.10 Report channels — corrected + +- `pmacs.editor.set_status` (`src/lua_bindings/mod.rs:13036`) is + transient; it is cleared at the top of every `dispatch_key`. +- **Revision 2 correction: a durable surface exists.** + `LuaHost::append_to_errors_buffer` (`src/lua.rs:401`) appends to + `*errors*` (`ERRORS_BUFFER_NAME`, `src/lua.rs:32`), creating it on + first use, and is the established idiom for "a callback failed and the + user was not watching" — `log_hook_error` + (`src/lua_bindings/mod.rs:6061`), `log_statusline_provider_error` + (`:6099`), `log_buffer_removed_error`, and the config error path + (`src/lua_bindings/config.rs:511`). Revision 1 claimed no such channel + existed. It does, it is Rust-side, and Q#RD7 now uses it. + +### 1.11 pmacs advertises no `workspace.workspaceEdit` capability + +`default_client_capabilities` (`src/lsp.rs:3242`) sends `"applyEdit": +true` (`:3259`) inside its `"workspace"` block (`:3253`) but **no +`workspaceEdit` object at all**. So pmacs declares neither +`documentChanges` ("The client supports versioned document changes in +`WorkspaceEdit`s"), nor `resourceOperations` ("The resource operations +the client supports"), nor `failureHandling` ("The failure handling +strategy of a client if applying the workspace edit fails") — +`grep -rn "failureHandling"` over the tree returns **0**. + +Two consequences worth stating plainly. pmacs applies resource +operations it never declared support for; and it declares no failure +strategy at all — **which is not the same as defaulting to one.** The +spec establishes no default for a client that advertises nothing, so +pmacs's actual behaviour is simply whatever its code does, which §1.6 +verified **resembles** abort-style application without being licensed as +it. *(Revision 3 wrote "`Abort` semantics are the de facto behaviour by +omission"; that smuggled the withdrawn claim back as a default and is +itself withdrawn at revision 4.)* **Neither is fixed by this lane** — +declaring capabilities changes what servers send, which is a behavioural +change needing its own evidence (§6). It is recorded because a framing +about batch failure semantics that did not notice pmacs declares none +would be describing half the system. + +### 1.12 The rename arm is more careful, and differently careful + +`"rename"` (`src/lua_bindings/mod.rs:3291`) does `std::fs::rename`, then +`find_by_path`, then `set_buffer_path` — it **rebinds**, preserving +contents and modified state. Delete **destroys**. Rename treats the +buffer as the valuable thing and the path as a mutable attribute; delete +treats the buffer as a cache of the file. + +Both arms share the §1.4 lookup defects. + +**Cross-lane contract, rechecked at revision 5.** PR #171's pushed +revision 8 (`7ecea94`) retains revision 7's split unchanged. This +document deliberately does **not** copy its ahead/behind count, line +count, or full lane status: those are volatile state owned by #171's +branch, and revision 4 proved that a sibling copy can be false before +the copying commit is pushed. The historical correction still matters: +revision 2 described #171 as stale and rename-only, while its revision 6 +had assigned rename *and* delete reconciliation to Stage 2a with the +opposite policy — `reconcile_delete` killing unmodified buffers and +keeping modified ones alive, so the file was deleted and the modified +buffer orphaned. Revision 7 withdrew that policy and established the +split below; revision 8 does not reopen it. + +**The stable ownership split carried by both lanes:** + +> #186 owns the urgent **pre-filesystem refusal** for synchronous +> `apply_resource_op`. #171 later owns **full post-delete lifecycle +> reconciliation**, including the **async race where a buffer becomes +> modified after dired dispatch**. + +#171 has adopted this from its side: its Q#DR18 takes this +document's Q#RD1 refusal rather than re-deciding it, and it records the +reason the refusal cannot simply be extended to cover dired — **dired +never calls `apply_resource_op`**. It calls `pmacs.fs.remove`, which +dispatches a worker, so a synchronous refusal inside the primitive +cannot reach it at any strength. That asynchronous window is #171's, and +naming it here is what keeps this lane from appearing to close a defect +it does not close. + +The older in-tree note at `docs/dired-framing.md:807-819` (Stage 1-era) +still describes the rename-side lookup defect accurately, but it is +superseded as a statement of plan by #171's Stage 2 document, which +exists only on that branch. + +### 1.13 Prior art — claims about **Emacs**, not pmacs + +Verified against `lisp/progmodes/eglot.el`, `emacs-mirror/emacs` +`master`. + +**Eglot orders the operations the other way round.** Its `do-delete`: + +```elisp +(do-delete (path &key recursive ignoreIfNotExists &allow-other-keys) + (let ((exists (file-exists-p path))) + (when (and (not exists) (not ignoreIfNotExists)) + (eglot--error "File %s does not exist" path)) + (when exists + ;; Kill buffer if the file is visited + (let ((buf (find-buffer-visiting path))) + (when buf (kill-buffer buf))) + (delete-file path recursive)))) +``` + +The buffer is killed **before** the file is deleted. Note also that the +`exists` guard means `ignoreIfNotExists` does **not** fall through to +the buffer kill — the asymmetry mode (b) exposes in pmacs. + +**Revision 3 precision.** Revision 2 asserted that "in Emacs +`kill-buffer` on a modified file-visiting buffer prompts", which was a +paraphrase carrying real weight in the argument. It is true, but the +mechanism is not the obvious one and the difference matters. From +`Fkill_buffer` (`src/buffer.c`): + +```c + /* Is this a modified buffer that's visiting a file? */ + modified = !NILP (BVAR (b, filename)) + && BUF_MODIFF (b) > BUF_SAVE_MODIFF (b); + + /* Query if the buffer is still modified. */ + if (INTERACTIVE && modified) + { + /* Ask whether to kill the buffer, and exit if the user says + "no". */ + if (NILP (calln (Qkill_buffer__possibly_save, buffer))) + return unbind_to (count, Qnil); +``` + +and `INTERACTIVE` is (`src/commands.h`): + +```c +/* Nonzero if input is coming from the keyboard. */ + +#define INTERACTIVE (NILP (Vexecuting_kbd_macro) && !noninteractive) +``` + +So the gate is **"Emacs has a keyboard"**, not "this function was +reached through `call-interactively`". Eglot's `do-delete` calls +`kill-buffer` programmatically from Lisp and **still prompts** in a +normal session — but **does not** in batch mode or while a keyboard +macro is executing. Revision 2's sentence was right for a reason it +never established, and false in two environments it never considered. + +Two riders, both verified: eglot ignores `kill-buffer`'s return value, +so declining the kill still deletes the file — the buffer and its text +survive, which is milder than pmacs's failure but is not a refusal. And +the prompt is **not** `buffer-offer-save`, whose own docstring says so: +"Note that this option has no effect on `kill-buffer'; if you want to +control what happens when a buffer is killed, use +`kill-buffer-query-functions'." + +*Ordering note (rev 2, sharpened at rev 3):* Emacs's ordering is **not** +what Q#RD2 adopts. Emacs can afford buffer-first because `kill-buffer` +is itself the consent gate — conditionally, per the `INTERACTIVE` gate +above. pmacs has no such gate at all, so it validates first and +reconciles last (Q#RD2), which yields the same safety unconditionally +and without firing callbacks against a file that still exists. + +**Eglot confirms server-initiated edits by default, as a whole-batch +decision taken before anything is applied.** `eglot-confirm-server-edits` +defaults to `'((t . maybe-summary))`; `prepare` builds closures touching +nothing, then the decision, then `apply-all`. The `maybe-*` decisions +skip the prompt only when the batch is `peaceful`: + +```elisp +(peaceful + (and + all-text-edits + (cl-loop for op in prepared + always (find-buffer-visiting (cadddr op))))) +``` + +`all-text-edits` is a conjunction over the whole batch, so a batch +containing any create/rename/delete **always** prompts under the +default. + +### 1.14 `apply_resource_op` has no direct test coverage + +`grep -rn "apply_resource_op" tests/ src/` returns **4 lines**: one doc +comment in `src/rename.rs` and three inside the binding's own +definition. Zero tests name it. + +One indirect acceptance exercises it: +`m4_15_workspace_edit_resource_ops_apply_in_order` +(`tests/m4_acceptance.rs:4014`), driven by the `resourceops` mode of the +fake server (`src/bin/pmacs_fake_lsp.rs:834`). **Its deleted `c.rs` is +never opened**, so the entire buffer-reconciliation half is untested. +That suite and that fake are where §5's pins belong. + +### 1.15 External-claim audit (revision 3) + +Two external-spec overclaims in two revisions is a pattern, not an +accident, so every claim in this document that is **not** about this +repository is listed here with its evidence. The standing rule: an +external claim carries a direct quote or it is marked not established. + +**This audit itself failed at revision 3, and the failure mode is +recorded because the table is now something readers trust.** Revision 3 +marked the `Abort` claim WITHDRAWN in row 4 while Q#RD3 — the normative +decision — still asserted it, and §1.11 still called it a default. The +audit checked each claim **where it was defined**, not at every site +that **consumed** it, so it certified a document that was internally +contradictory. A withdrawal recorded in an audit while the claim stays +load-bearing elsewhere is worse than no withdrawal, because the audit +converts an error into a false assurance. + +**So the audit procedure is, from revision 5:** for each row, search the +whole document for the claim's terms and check every hit, not only the +defining section. Search normalized prose or multiple term stems as +well as exact phrases: revision 4's literal `always answers` search +missed §2.1's `**always** answers` because Markdown markup split the +phrase. Revision 4 found the two surviving `Abort` consumers; revision +5 found and fixed that reporting consumer. + +| # | Claim | Source | Status | +|---|---|---|---| +| 1 | Resource ops execute in provided order | LSP 3.18 `WorkspaceEdit` | **Quoted**, §1.7. Unconditional. | +| 2 | Recovery is described by the client's `failureHandling` | LSP 3.18 | **Quoted**, §1.7. | +| 3 | The four `FailureHandlingKind` doc comments | LSP 3.18 | **Quoted verbatim**, §1.7 table. | +| 4 | ~~The spec assigns `Abort` to resource-op edits~~ | — | **WITHDRAWN** (P1-2). Never supported; it conflated one client-selectable strategy with a protocol rule. | +| 5 | `documentChanges` / `resourceOperations` / `failureHandling` capability doc comments | LSP 3.18 | **Quoted**, §1.11. | +| 6 | eglot's `do-delete` body | `lisp/progmodes/eglot.el`, emacs-mirror master | **Quoted from source**, §1.13. | +| 7 | `eglot-confirm-server-edits` default and the `peaceful` conjunction | same | **Quoted from source**, §1.13. | +| 8 | Emacs prompts when killing a modified file-visiting buffer | `src/buffer.c` + `src/commands.h` | **Quoted at rev 3**, §1.13. Was a bare paraphrase at rev 2; the real gate is `INTERACTIVE`, i.e. keyboard present — not `call-interactively` — so it does **not** hold in batch or during a keyboard macro. | +| 9 | `buffer-offer-save` does not affect `kill-buffer` | `lisp/files.el` docstring | **Quoted**, §1.13. | + +Not established, and therefore not claimed anywhere in this document: +what `lsp-mode` (as distinct from eglot) does with `DeleteFile`; and the +exact `ApplyWorkspaceEditResult` field list beyond `applied` and +`failureReason`, which this document uses only because pmacs's own code +already sends them (`builtin/runtime/lsp.lua:1841`). + + +## 2. The decision space + +### 2.1 Recommended — validate before mutating, at the primitive; conflict-check early, in the applier + +**The primitive refuses before touching disk. The applier catches what +it can early, and reports honestly what it cannot.** + +**Layer 1 — the primitive (the invariant).** The delete arm becomes four +ordered phases: + +``` +stat / no-op decision → enumerate and validate affected buffers + → mutate the filesystem + → reconcile the registry +``` + +Validation inspects; it does not remove. If any affected buffer is +modified — or is mid-edit (`editing_in_progress`) — the op returns an +error having touched nothing. Because validation removes nothing, a +filesystem failure leaves every buffer intact automatically, and +`on_removed` still fires only in the reconcile phase, i.e. with the path +already gone, preserving today's invariant. + +**Layer 2 — the applier (early conflict check + robust reporting).** +`apply_workspace_edit`'s existing plan loop gains a delete-precondition +check and returns its existing `nil, message`. This is a **filter, not a +transaction** (§1.7): it catches the plan-time buffer conflict and +filesystem refusals cheaply, before anything is mutated, and it is +honest that a sequential batch can still refuse mid-flight. What makes +mid-flight refusal survivable is Q#RD7: each primitive call is wrapped, +every failure becomes `nil, message`, the origin buffer is restored +best-effort, and the unattended caller always **attempts** a response +while the response channel remains live. + +Neither layer is redundant. Layer 1 alone leaves every batch failure +reported through a channel that does not work (§1.5). Layer 2 alone +leaves the primitive armed for direct callers — `pmacs.buffer.apply_resource_op` +is public Lua API, and dired Stage 2's own plan names delete +reconciliation as its 2a substrate. + +**Why this beats the runners-up, in one sentence each:** it is the only +option that puts the check strictly before the irreversible step without +either firing callbacks into a half-changed world (buffer-first) or +inventing a recovery surface (backup), and it is the only one whose cost +is bounded by this lane. + +### 2.2 Prompt the user — rejected on scope, not on possibility + +**Revision 2 rewrite.** Revision 1 argued impossibility on three +grounds; two were wrong (§1.8) and are withdrawn. The surviving argument +is narrower and is about cost: + +- **The applier must become a continuation chain.** The primitive cannot + suspend (§1.8), so the remaining plan has to be carried as a closure + across the user turn — with cancellation, and with **revalidation of + every precondition afterwards**, because the world moves during the + turn. That is §1.7's sequential-batch problem with a human-scale delay + inserted into it. +- **The minibuffer is a single slot that replaces without asking** + (`Minibuffer::begin`, "Replaces any existing session"). A prompt + raised while the user is mid-`M-x` destroys their in-flight prompt. + Queuing is therefore a prerequisite, and no queue exists. +- **Deferred answers need a pending-request ledger.** Answering + `workspace/applyEdit` later is possible (§1.8) but means retaining + `(server_id, request_id)` across ticks and deciding what happens if + the server dies first. `purge_dead_pending` exists for *client* + requests; there is no equivalent for held server requests. + +None of that is impossible; all of it is a separate lane with its own +framing. **Refusing is the correct move for a live data-loss bug**, and +a prompt can later *loosen* an unconditional refusal without either +change invalidating the other. + +### 2.3 Save first, then delete — rejected + +Silently converts an unsaved edit into a committed one and then destroys +it — more destructive, not less, because it overwrites the on-disk +original immediately before removing the file. It also cannot be relied +on: `save_inner` (`src/editor_core.rs:1908`) refuses at `:1917` when the +file changed on disk since it was read, so the fallback question is +unanswered and we are back to refusing. + +### 2.4 Back up the contents somewhere recoverable — rejected + +Rejected on evidence (§1.9): the decisive fact is that **removing the +buffer deletes the recovery file**, so the backup is destroyed by the +operation it exists to survive. Building a side-store outside +`autosave/` means a second recovery surface with its own discovery, GC +and lifecycle, to make a destructive operation *feel* safe. + +### 2.5 Key the behaviour on LSP-versus-user provenance — rejected + +`apply_resource_op` takes no provenance argument and there is no ambient +caller identity. Adding one makes the primitive's safety depend on a +caller-supplied flag — any caller that omits it is unguarded, which is +the failure mode the lane exists to remove. COHERENCE §10 (extension +trust classes) is unbuilt, so there is no trust dimension to key on. +**The refusal is unconditional and provenance-blind.** + + +## 3. Decisions + +### Q#RD1 — Refuse. Do not prompt, do not save, do not back up — **SETTLED YES** + +A delete whose target set contains a modified buffer **fails**, changing +nothing on disk and nothing in the registry. This is the first refusal +in the codebase keyed on unsaved state (§1.4) and is intended as the +precedent for `kill_buffer` and `editor.quit`, which have the same gap. + +### Q#RD2 — Validate before mutating; reconcile last — **REWRITTEN at rev 2** + +Four phases, in order: **stat/no-op decision → enumerate and validate +affected buffers → mutate the filesystem → reconcile the registry.** + +- **Validation inspects only.** It checks `Buffer::is_modified()` and + `Buffer::editing_in_progress()` (`src/buffer.rs:473`, `:747`) across + the affected set. Nothing is removed, so nothing can be lost if a + later phase fails. +- **`editing_in_progress` moves from discovery to validation.** Today a + `ConcurrentEdit` refusal from `BufferRegistry::remove` arrives *after* + the file is gone. Checking it during validation means a delete invoked + from inside the target's own edit intercept refuses before disk. +- **`on_removed` still observes the path already gone.** Reconciliation + is the last phase, so the invariant revision 1 would have broken is + preserved. This is the specific defect revision 1's buffer-first + ordering introduced, and it is why that ordering is withdrawn. +- **A filesystem failure leaves buffers untouched**, automatically + rather than by compensation. + +### Q#RD3 — The preflight is an early conflict check, **not** a transaction — **DOWNGRADED at rev 2** + +`apply_workspace_edit`'s plan loop gains a delete-precondition check and +returns its existing `nil, message`. It is described in the code comment +and here as a **filter**: + +- **What it guarantees:** a plan-time modified/mid-edit buffer, a known + missing target without `ignore_if_not_exists`, or an unanswerable stat + refuses before anything in the batch is mutated, with one clear + message. +- **What it does not guarantee, stated plainly:** `documentChanges` are + sequential (§1.7). An earlier text edit can dirty a clean buffer, and + an earlier rename can move a modified buffer *into* a later delete's + subtree, after the snapshot. Then the preflight passes and the + primitive refuses mid-batch, leaving earlier operations applied. + That outcome **resembles abort-style application**, and it is what + pmacs already does today on an I/O error (§1.6, verified). It is + **not** licensed by the specification: the spec assigns no strategy + to a client that advertises none (§1.7), so the justification is + observed pmacs behaviour plus the judgement that a visible partial + refactor beats unrecoverable unsaved work — nothing more. +- Revision 1 called this "whole-batch atomicity" and said "nothing in + the batch is mutated". **That was false and is withdrawn.** + +The check needs a synchronous filesystem-and-buffer query that Lua lacks +(§1.4, Q#RD12). It must be **one** query shared with the primitive's +stat/validation phases, so the two cannot drift apart. + +### Q#RD4 — `ignore_if_not_exists` short-circuits at **both** layers — **WIDENED at rev 2** + +When the path is absent and `ignore_if_not_exists` is set, the op is a +no-op: + +- **Primitive:** return early without touching the registry — the + `create` arm's existing idiom and Eglot's `exists` guard (§1.13). +- **Preflight:** must **not** reject the batch merely because a modified + buffer still names that absent path. Revision 1 applied this only to + the primitive, which would have made the preflight refuse an op the + primitive treats as a no-op — a refusal with no underlying + destruction, i.e. a false positive that blocks legitimate edits. + +Mode (b) is not a special case of the main bug; it is a missing early +return, and a fix aimed only at the "we actually deleted something" +branch leaves it live. + +### Q#RD5 — Recursive deletes are **inspected** tree-wide but **reconciled** exact-path — **SETTLED YES, NARROWED at rev 2** + +Mode (c) proves `recursive = true` reconciles nothing, so an exact-path +guard is bypassed by the most destructive arm. Therefore: + +- **Validation is prefix-aware**: every buffer whose path lies beneath + the deleted directory is inspected, and any modified one refuses the + op. Without this the guard has a trivial reachable bypass. +- **Reconciliation is not widened**: after a successful *clean* + recursive delete, descendant buffers are left exactly as today — + orphaned and clean. **Removing them now would promote mode (d)'s + dangling-window and last-buffer defects from an exact-path defect to a + tree-wide one**, which is precisely the parked lifecycle work this + lane must not expand into (Q#RD8). + +The asymmetry is deliberate and is the point: **inspect widely, mutate +narrowly.** + +**Boundary with dired — restated at rev 5.** The settled split (§1.12, +also carried by #171) is: + +> #186 owns the urgent **pre-filesystem refusal** for synchronous +> `apply_resource_op`. #171 later owns **full post-delete lifecycle +> reconciliation**, including the **async race where a buffer becomes +> modified after dired dispatch**. + +**The stale justification is withdrawn.** Revision 2 supported taking +the delete side now by citing the ledger's "OPEN, STALE, 153 commits +behind, under re-scout" assessment of #171. That re-scout has finished; +#171 has completed the re-scout and retains the settled split through +its pushed revision 8. **The conclusion is unchanged and rests on +urgency alone** — this is a live data-loss bug +with a reproduction, and a refusal that must precede the filesystem call +cannot be deferred to a lane that acts after it. It no longer rests on +any claim about #171's freshness, and it must not be re-argued from one. + +### Q#RD6 — The shared query scans **all** path-bound buffers — **REWRITTEN at rev 2** + +Revision 1 said the guard would use `EditorCore::find_buffer_for_path`. +**That is wrong** and is withdrawn: it normalizes but delegates to the +singular, first-match-only `find_by_path` (§1.4), and duplicate +path-bound buffers are reachable from public Lua via +`pmacs.buffer.from_file`. A clean first match would hide a modified +second — a silent guard bypass. + +The shared query therefore: + +- **scans every path-bound buffer**, returning all matches rather than + the first; +- **normalizes once** and compares normalized forms, so a raw-path + lookup cannot miss a stored normalized path; +- **matches with component-aware `Path::starts_with`**, not string + prefix — so `/tree` does not match `/tree-sibling`; +- is the **single** query used by both the primitive's validation phase + and the Lua preflight (Q#RD3). + +**This lane claims the query.** The boundary's rule is "whichever lands +first owns the query and the other adopts it", and #171's current +framing records that this rule's four clauses are character-for-character what +it had written independently for `reconcile_delete`. To stop both lanes +asserting ownership: **#186 owns and implements the shared walk**, #171 +adopts it and extends it to `reconcile_rename`. If #171 lands first the +claim inverts and this decision is what gets deleted — but it is stated +in one direction so the duplicate resolves rather than persisting. + +"Modified" is `Buffer::is_modified()`. No new notion of dirtiness. + +Explicitly **not** guarded: a clean buffer. A delete whose target is +open but unmodified proceeds and removes the buffer, as today. +Overreach would break `m4_15` and would fail legitimate deletes for +users who merely have the file open. + +### Q#RD7 — One reporting seam at the server-request boundary — **REWRITTEN at rev 3** + +§1.5 established that **no** caller reliably surfaces a raise. Revision +2 answered that with three promises that did not compose into anything +implementable; revision 3 replaces them with **one seam**. + +**Where the seam is: the server-request boundary**, i.e. the +`workspace/applyEdit` arm of `handle_server_requests` +(`builtin/runtime/lsp.lua:1833-1843`). Everything below hangs off that +single point. + +- **Wrap parse *and* apply, not apply alone.** Revision 2 wrapped "every + primitive call inside `apply_workspace_edit`", which does not cover + `pmacs.lsp._parse_workspace_edit` — it is called at `lsp.lua:1835`, + one line **above** `apply_workspace_edit`, and it is fallible + (`lua_to_json(edit)?`, `src/lua_bindings/mod.rs:10161`). A parse + failure therefore escaped, was swallowed by + `pcall(handle_server_requests)`, and left the server unanswered — the + exact defect being fixed, one line out of scope. + + **Of the two options offered in review, this revision picks wrapping + parse-plus-apply** rather than narrowing the wrap to applier execution + failures. Reason: the wrap moves up exactly one line and costs + nothing, so the boundary is uniform regardless of which call fails. + + **Revision 4 correction to the strength of the claim.** Revision 3 + said this made "always answers" true *without qualification*. It does + not, for two independent reasons, and the honest wording is **"always + attempts a response while the response channel remains live"**: + - `send_response` is itself called under an ignored `pcall` + (`builtin/runtime/lsp.lua:1843`), so its failure is unobservable to + the applier. A dead or wedged transport cannot be answered by any + amount of wrapping upstream. + - The parse call is, on the evidence, **not reachably fallible** — + see acceptance 15 and Q#RD11. +- **Every failure becomes a value.** Refusal, I/O error, and parse + failure all converge on the existing `nil, message` shape, which all + three callers already handle. No exception escapes the applier. +- **The unattended caller always *attempts* a response**: `{ applied = + false, failureReason = ... }` is constructed and sent in every failure + case the applier can observe. Whether it lands is the transport's + business, and the applier cannot tell (see above). +- **The durable trace is written at this boundary, not in the + primitive.** Revision 2 promised logging through + `LuaHost::append_to_errors_buffer` (`src/lua.rs:401`). Two problems, + both confirmed: it is **private**, so it is not callable from where + the promise was made; and a **Lua preflight** rejection never reaches + the Rust primitive at all, so primitive-side logging would miss the + common unattended case entirely. So: a **narrow Lua-callable surface** + that appends one attributed record to `*errors*`, invoked at the + server-request boundary **after any `applied = false`**, with the + label `lsp:workspace/applyEdit`. One call site, one label, reachable + from the layer that actually knows the outcome. +- **The origin buffer is restored best-effort on the failure path too.** + Today `pcall(pmacs.buffer.find_or_open, origin)` runs only after a + successful loop; an early failure return would strand the user in + whatever buffer the last op left active. +- The message **names the buffer** and says what to do. Not a bare + errno. + +Acceptance 13 tests **both** halves of the boundary — the response the +server receives *and* the `*errors*` record — because revision 2 tested +only the first while promising the second. + +### Q#RD8 — The window/last-buffer defects do **not** land here + +Mode (d) is real and parked (§6). It is a different failure from data +loss, and it is shared with `pmacs.buffer.remove`. + +**The trap that makes the obvious fix wrong:** the two removal paths +clean *disjoint* sets. `kill_buffer` handles the last-buffer refusal, +`round_trip_buffers`, side-window collapse and window rebinding, but +**not** keymaps, config, folds or `on_removed` callbacks; +`remove_buffer_and_fire` handles exactly the latter and none of the +former. Neither is a superset, so "just call `kill_buffer` instead" +would silently regress four cleanups. + +**Revision 3 — that lane now exists.** Revision 2 said mode (d) "needs +its own lane and a census", which was true when written and is not now. +#171's `reconcile_delete` composes both phases for every id it kills and +reroutes `apply_resource_op`'s delete arm through it, and #171 revision +7 records the disjoint-set trap independently. So mode (d) is **owned, +not unowned**, and per Q#RD5 this lane's job is narrower than it looked: +not merely "don't fix it here" but **don't enlarge the surface #171 has +to fix**. + +### Q#RD9 — **WITHDRAWN at rev 2** + +Revision 1 proposed that, after a buffer-first removal, a filesystem +failure would leave the buffer unrestored. Q#RD2's phase ordering makes +the situation unreachable: nothing is removed before the filesystem +mutation succeeds, so there is no lost buffer to restore. The decision +number is retained rather than reused, so review can see it went away +rather than being renumbered. + +### Q#RD10 — Validate every match; reconcile today's first match only — **NEW at rev 3** + +Q#RD6 makes validation scan **all** path-bound buffers. Revision 2 never +said what *reconciliation* does afterwards when several match, which +left the common duplicate case undefined. It is now decided: + +- **Validation: every match.** If any buffer bound to the path — or + beneath it, for a recursive delete — is modified, the op refuses. + A clean first match must not be able to hide a modified second (§1.4). +- **Reconciliation: exactly today's behaviour.** After a *successful* + delete, the single first exact-path match is removed, as + `find_by_path` does now. Additional clean duplicates are left in + place. + +**Why not remove them all.** Every extra removal goes through +`remove_buffer_and_fire`, which is phase 2 without phase 1 (Q#RD8) — so +removing N duplicates creates up to N dangling windows and brings the +registry N steps closer to empty. That is precisely the parked defect +Q#RD5 is written to contain, and widening it here would hand #171 a +larger problem in exchange for tidiness this lane does not need. + +**The honest cost**, stated rather than buried: a surviving clean +duplicate is left bound to a path that no longer exists — the same +orphan shape as mode (c), on a narrower trigger. It is **residue handed +to #171**, whose lifecycle transaction can then remove all matches +safely because it composes both phases. This lane's contract is that no +*unsaved* work is lost, not that the registry ends tidy. + +Acceptance 14 pins both directions, so neither widening nor narrowing +can happen silently. + +### Q#RD11 — The parse wrap is a defensive boundary, tested with a stub — **NEW at rev 4** + +Revision 3 justified wrapping `_parse_workspace_edit` by asserting it +was reachably fallible. **On the evidence it is not**, and acceptance 15 +as written could not fail: + +- `WorkspaceEditResponse::from_lsp_value` (`src/rename.rs:95`) returns + `Self`, not a `Result`. Its own doc comment says "A `null` / + shapeless result yields an empty response." +- The binding's only `?` is `lua_to_json(edit)?` + (`src/lua_bindings/mod.rs:10161`), and its input arrived as JSON + through `json_to_lua`. Every value that round-trip produces is + accepted going back. + +So no fake server can send a payload that makes the parse fail. +**Decision — the first of the two options offered: keep the wrap, and +label it a defensive boundary test driven by an explicit throwing test +stub for `pmacs.lsp._parse_workspace_edit`.** Reasons: the wrap costs +one line and makes the boundary uniform, so a future parse that *does* +become fallible is covered by construction rather than by remembering; +and the promise in Q#RD7 is narrowed to match reality rather than +propped up by an unfalsifiable criterion. + +**What this explicitly is not:** a claim that a server can trigger it. +Acceptance 15 is labelled defensive, and it substitutes the stub rather +than dressing up a reachable payload — a criterion that cannot fail is +not a pin, and pretending otherwise is the defect this decision exists +to avoid. + +### Q#RD12 — The preflight needs a total Rust-backed verdict, not a registry walk alone — **REWRITTEN at rev 5** + +Q#RD4 requires the Lua preflight to distinguish **absent + ignore** (a +no-op the preflight must let through) from **present + ignore** (a real +delete the preflight must judge). Q#RD6 specifies only a registry walk, +which answers a question about *buffers*, not about *the filesystem*. +Nothing in Lua closes that gap today: + +- `pmacs.fs.stat` (`builtin/runtime/fs.lua:133`) dispatches through + `async_mod._dispatch_fs_stat` and returns a handle — asynchronous, and + the applier is synchronous. +- The only synchronous filesystem binding is `canonicalize` + (`src/lua_bindings/mod.rs:6743`), which is realpath-like and **not** + equivalent to the primitive's `symlink_metadata`: it resolves symlinks + and returns `nil` for a dangling one, so it reports "absent" for a + broken symlink that `symlink_metadata` reports as **present**. That is + exactly the case this query turns on, so using it would produce a + preflight that disagrees with the primitive on the one input that + matters. + +**The seam: one synchronous internal Rust binding, +`pmacs.buffer._delete_verdict(spec)`, returning a structured verdict.** +It accepts the same `path`, `recursive`, and `ignore_if_not_exists` +fields as the delete primitive and delegates to the same Rust helper, +including the same `symlink_metadata` call and affected-set walk, so the +two layers cannot disagree by construction. The Lua-visible shape is +`{ kind = "...", message = ..., buffer_name = ... }`; `message` is +required and non-empty for `refuse`, and `buffer_name` is present only +when a buffer caused the refusal: + +| Verdict | Meaning | +|---|---| +| `no-op` | path absent **and** `ignore_if_not_exists` set — the op will do nothing; the preflight must not reject it | +| `clear` | path present, and every affected buffer is clean and not mid-edit — the delete may proceed | +| `refuse` | delete must not proceed: missing without ignore, stat uncertainty, or an affected buffer modified/mid-edit; `message` states which, and buffer-caused refusals name it | + +**Total mapping and error contract.** + +1. `symlink_metadata == NotFound` plus `ignore_if_not_exists` yields + `no-op`. +2. `NotFound` without ignore yields `refuse` with the ordinary delete + I/O message. Catching this deterministic failure in the plan makes + that case more atomic than today without claiming the batch is a + transaction; dynamic failures remain possible. +3. Any other stat error (for example `EACCES` or `NotADirectory`) yields + `refuse` carrying that I/O reason. The preflight never reports safe + on the strength of a question it could not answer. +4. A present path whose affected set contains a modified or + `editing_in_progress` buffer yields `refuse` naming that buffer. +5. Only a present path with a clean, quiescent affected set yields + `clear`. + +The binding raises only on argument-type violations, matching the rest +of the `pmacs.buffer` surface. Ordinary filesystem conditions and buffer +refusals are values. + +The callers consume the same Rust enum in different forms. The +primitive returns `Ok(())` for `no-op`, turns `refuse` into its ordinary +Lua error carrying the verdict message, and reaches filesystem mutation +only for `clear`. The Lua plan lets `no-op` and `clear` through and +returns its existing `nil, message` for `refuse`. + +This helper **is** the single shared query of Q#RD6: the walk is its +buffer half, `symlink_metadata` its filesystem half, the binding +serializes its result, and the primitive consumes it directly. Drift is +impossible rather than merely discouraged. + +## 4. Bets (falsifiable) + +- **B1 — Refusing breaks no legitimate server workflow.** A server + deleting a file the user has unsaved edits in is a conflict the user + must resolve. Falsified by a real server whose normal operation + deletes files the user is actively editing. +- **B2 — Mid-batch refusal is acceptable because partial application is + already what pmacs does, and is safer than data loss.** + *Rewritten at rev 3 (P1-2).* Revision 2 rested this on the protocol + "assigning `Abort`" to resource-op edits, which it does not (§1.7): + recovery is the client's advertised choice and pmacs advertises none. + The bet now stands on repository evidence — §1.6 **verified** that an + op failing mid-batch leaves earlier ops applied on `main` today — plus + the ordering judgement that a partial refactor the user can see and + redo beats unsaved work they cannot recover. Falsified if a server is + found that requires transactional application and degrades badly under + partial application. Acceptance 12 pins the observable behaviour + either way. +- **B3 — Prefix-aware validation does not over-refuse.** Falsified if a + common workflow deletes a directory while an unrelated modified buffer + sits beneath it and the refusal is judged unhelpful. +- **B4 — Leaving clean descendants orphaned is the lesser evil** + (Q#RD5). Falsified if orphaned clean buffers after a recursive delete + prove more disruptive than the tree-wide lifecycle defect that + removing them would create. + + +## 5. Acceptance + +Each criterion states the **pre-image it must fail against**. A test +that passes against its pre-image has no bite and is rejected. + +1. **A delete op targeting a modified buffer refuses, and the file + survives.** Assert together: the call fails, the buffer is still in + the registry with its exact unsaved text, and `path.exists()` is + still true. + *Bite:* fails against `ad41cf1` unmodified. **Asserting only that the + buffer survived is vacuous** — that is mode (c)'s existing behaviour. + The `exists()` assertion carries the bite. + +2. **A delete op targeting a *clean* open buffer still succeeds**, file + removed and buffer removed. + *Bite:* fails against an over-broad guard that refuses whenever a + buffer is open. Assert **both directions**. + +3. **A filesystem failure preserves the clean buffer** (Q#RD2). Force + the fs mutation to fail (e.g. a non-empty directory without + `recursive`) and assert the buffer is still present and intact. + *Bite:* fails against revision 1's buffer-first ordering, which would + have removed the buffer and then failed. + +4. **`on_removed` observes the path absent** (Q#RD2). Register an + `on_removed` callback that stats the path and records the result; + assert it saw the path already gone. + *Bite:* fails against revision 1's buffer-first ordering, under which + the callback would observe the file still present. This is the pin + that keeps the phase order from silently regressing. + +5. **A delete called from inside the target's own edit intercept refuses + before disk** (Q#RD2). Assert the file still exists. + *Bite:* fails against `ad41cf1`, where `ConcurrentEdit` is discovered + only at removal time — after `remove_file` has already run. + +6. **Duplicate path-bound buffers cannot hide a modified copy** (Q#RD6). + Create two buffers on one path via `pmacs.buffer.from_file`, leave + the first clean and modify the second, then delete. + *Bite:* fails against any first-match lookup, including + `EditorCore::find_buffer_for_path` — which is exactly what revision 1 + specified. **This is the criterion that pins validation breadth**; + criterion 14 pins the reconciliation half and cannot see breadth + (Q#RD10). + +7. **Component-prefix false positives are rejected** (Q#RD6). A modified + buffer under `/tree-sibling` must **not** block a recursive delete of + `/tree`. + *Bite:* fails against a string-prefix implementation. Pairs with + criterion 8 so both directions of the prefix rule are pinned. + +8. **`recursive = true` over a directory containing a modified buffer's + file refuses, and the whole tree survives** (Q#RD5). Assert the inner + file still exists — not merely that the buffer does, which is already + true today (mode (c)). + *Bite:* fails against exact-path-equality validation. + +9. **A clean recursive delete leaves descendant buffers orphaned, not + removed** (Q#RD5). Assert the descendant buffer is still in the + registry after a successful recursive delete. + *Bite:* fails against an implementation that widens reconciliation to + the tree. This pin exists specifically to stop the parked defect from + being enlarged, and it is expected to look odd — it asserts today's + imperfect behaviour deliberately. + +10. **`ignore_if_not_exists = true` on an absent path leaves a modified + buffer intact** (Q#RD4), reproducing mode (b): file removed behind + pmacs's back first, then the op. + *Bite:* fails against a fix guarding only the branch where the fs + delete actually ran. + +11. **Absent-plus-ignore succeeds through the real server pump** + (Q#RD4, layer 2). Drive it end to end and assert the batch is + **not** refused and the server is told `applied = true`. + *Bite:* fails against a preflight that rejects on the presence of a + modified buffer without consulting `ignore_if_not_exists` — the + false-positive Q#RD4 exists to prevent. + +11a. **Present-plus-ignore with a modified buffer is still REFUSED in + the preflight** (Q#RD4, Q#RD12) — the opposite direction of 11. The + target **exists** on disk, `ignore_if_not_exists = true`, and a + modified buffer is bound to it. Assert the batch is refused, the + file still exists, and the buffer keeps its text. + *Bite:* fails against a preflight that treats + `ignore_if_not_exists` as an unconditional bypass rather than + consulting the filesystem — i.e. against any implementation that + reads the flag without the `symlink_metadata` verdict Q#RD12 + specifies. **Revision 3 shipped only direction 11**, and + one-direction coverage on a two-direction rule is exactly how that + gap survived a round; the pair is now explicit, as with 7/8. + +11b. **A dangling symlink counts as present** (Q#RD12). Target is a + symlink whose destination does not exist, `ignore_if_not_exists = + true`, modified buffer bound to the link path. Assert refusal. + *Bite:* fails against a preflight built on `canonicalize` + (`src/lua_bindings/mod.rs:6743`), which returns `nil` for a broken + symlink and would therefore mis-classify this as absent — the one + input on which realpath and `symlink_metadata` disagree, and the + reason Q#RD12 specifies the latter. + +11c. **Absent without ignore refuses in the plan, before earlier ops** + (Q#RD3, Q#RD12). A batch contains a text edit followed by a delete + of a missing target with `ignore_if_not_exists = false`. Assert + `applied = false`, a non-empty NotFound-style `failureReason`, and + that the earlier text edit was not applied. + *Bite:* fails if the verdict maps this state to `clear` and leaves + the primitive to discover it mid-batch; that implementation would + partially apply the text edit before returning the known error. + +11d. **An unanswerable stat fails closed in the plan** (Q#RD12). Use a + regular file as a would-be parent and target its child, producing + `NotADirectory` on the supported CI platforms. Assert + `applied = false`, the earlier batch op did not apply, and the + `failureReason` carries the filesystem cause. + *Bite:* fails if a non-NotFound stat error is collapsed to `clear` + or if the binding raises past the value-returning boundary. + +12. **Edit-then-delete and rename-into-delete still answer the server** + (Q#RD3, Q#RD7). Two batches that defeat the snapshot preflight: one + where an earlier text edit dirties the buffer a later op deletes, + one where an earlier rename moves a modified buffer into a later + delete's subtree. Assert in both cases that the server receives + `applied = false` with a non-empty `failureReason`. + *Bite:* fails against `ad41cf1` (the raise is swallowed at + `lsp.lua:1892` and no response is sent) **and** against a + preflight-only fix that claims atomicity — these are the cases + revision 1's atomicity claim asserted could not happen. + +13. **The refusal reaches the server on the unattended path AND leaves + the durable trace** (Q#RD7). Assert **both**: the server receives + `applied = false` with a non-empty `failureReason`, *and* `*errors*` + contains a record carrying the `lsp:workspace/applyEdit` label. The + user-initiated paths additionally report on the status line naming + the buffer. + *Bite:* fails against a fix that refuses by raising. A direct-call + test on `apply_resource_op` does **not** satisfy this and is + rejected as insufficient — the guard must be pinned through the + outermost user-reachable seam. **The `*errors*` half fails against + revision 2**, which promised the trace and tested only the response; + asserting the response alone is what let that gap survive a round. + +14. **Clean duplicates: one match reconciled** (Q#RD10). Two clean + buffers bound to one path; delete succeeds. Assert exactly one is + removed and one remains. + *Bite:* fails against an implementation that removes **all** matches + — i.e. it pins the reconciliation half of Q#RD10, and only that. + *Correction at rev 4:* revision 3 claimed this criterion "fails in + both directions", including against first-match-only *validation*. + **That was false.** With both buffers clean there is no verdict + difference between consulting one match and consulting all, so the + setup cannot see validation breadth. **Criterion 6 is the one that + detects incomplete validation** (clean first, modified second), and + the two are now labelled by which half of Q#RD10 each pins. The + claim is fixed rather than the setup, because criterion 6 already + covers the other half and duplicating it here would add no bite. + +15. **Defensive: a parse failure still attempts a response** (Q#RD7, + Q#RD11). Substitute an explicit **throwing test stub** for + `pmacs.lsp._parse_workspace_edit`, then assert the server still + receives `applied = false` with a `failureReason`. + *Bite:* fails against a wrap that covers `apply_workspace_edit` + only, leaving the parse one line outside. + **Labelled defensive, and here is why the label is load-bearing:** + revision 3 specified this as a *payload* test, which **could not + fail** — `from_lsp_value` (`src/rename.rs:95`) returns `Self` and + its doc says a shapeless result yields an empty response, and the + binding's only `?` is `lua_to_json` over a value that arrived + through `json_to_lua`. No server payload reaches the failure. The + stub is therefore substituted deliberately, and the criterion claims + only what a stub can establish: that the boundary reports rather + than that a server can provoke it. + +16. **`m4_15_workspace_edit_resource_ops_apply_in_order` stays green + unmodified**, pinning no-regression from outside. Its `c.rs` is + never opened, so it exercises exactly the unguarded case that must + keep working (Q#RD6). + +17. **Every new test is checked with `scripts/bite`** and none reports + VACUOUS. + + +## 6. Parked — not deferred-and-forgotten + +- **Mode (d): the dangling window and the emptiable registry** (§1.1, + Q#RD8). **Owned by #171** as of its revision 7 — `reconcile_delete` + composes both removal phases and reroutes `apply_resource_op`'s delete + arm through it. Revision 2 of this document called it unowned; that + was true when written and is not now. Q#RD5 and Q#RD10 are written to + avoid enlarging what that lane must fix. +- **`pmacs.fs.remove` is guarded by neither lane — explicitly out of + scope here** (named in #171 revision 7 §11). After both lanes land, + the refusal sits at the `apply_resource_op` primitive (this lane) and + in dired's policy layer (#171), but `pmacs.fs.remove` is public Lua + API with **no dirty check of its own**, so a third caller inherits + neither guard — the guards are one layer *above* it on each side. + Verified latent rather than live: `pmacs.fs.remove` + (`builtin/runtime/fs.lua:187`) has **zero production callers**, its + only references being `tests/m8_1_acceptance.rs:438`, `:439`, `:472`. + **This lane does not extend scope to cover it.** It belongs with the + primitive-level fs guards, i.e. #171's `pmacs.fs.*` work or a + successor lane — recorded here because "both lanes guard deletion" + otherwise reads as a claim that the primitive is guarded, and it is + not. +- **`kill_buffer` and `editor.quit` have the same gap** (§1.4). + `editor.before-quit` exists as a veto channel with no subscriber. This + lane sets the precedent; those are separate lanes. +- **Declaring `workspace.workspaceEdit` capabilities** — `documentChanges`, + `resourceOperations`, `failureHandling` (§1.11). Declaring them changes + what servers send, so it needs its own evidence and its own lane. +- **`pmacs.error` is undefined** (§1.5) — 11 dead call sites in + `builtin/`, including the one that is supposed to surface every + uncaught async coroutine error. A known standing defect, widened in + relevance by this framing but not fixed by it. +- **A `y_or_n` helper**, minibuffer queuing, and a held-server-request + ledger — the three prerequisites that would make §2.2 cheap rather + than merely expensive. +- **`lsp.confirm-server-edits`**, the config-registry adopter that would + let a user loosen Q#RD1 once a prompt mechanism exists. +- **The rename side of prefix-aware, normalizing lookup** — dired + Stage 2's, per Q#RD5. + + +## 7. Gates + +Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy +--workspace --all-targets -- -D warnings` as its own step; `cargo test +--lib`; `cargo test --lib --features crdt`; the touched acceptance +suites; `cargo test --test m4_acceptance -- --skip basedpyright`; +`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; `git diff --check`. + +Touched suites: **`m4_acceptance`** (the resource-op home, §1.14, and +the home of criteria 1–14 including 11a–11d, and 16) and +**`lsp_dispatch_seams_acceptance`** +(criterion 15's throwing parse stub, Q#RD11). Both appear in §8's touch +table; revision 3 named the second here but omitted it there, and the +two lists are now maintained together. + +`dired_acceptance` and `autosave_acceptance` are watch items, not +touched files — the former for Q#RD6's shared lookup, the latter because +`on_removed` ordering (criterion 4) is where autosave's +`discard_buffer` hangs. + +Gate the pushed tree, not the worktree — commit first, then gate. + +While the document is still PROPOSED the gate is `git diff --check` plus +a docs read; the full suite runs once implementation lands on this +branch. + + +## 8. Branch plan + +`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, one +branch, **one PR — #186, which becomes the implementation PR.** +Revision 2 withdraws revision 1's two-PR plan, which conflicted with +one-feature/one-branch/one-PR: the framing is revised in place, and once +approved the implementation commits land on this same branch. + +**Implementation does not begin until the user approves this revision.** + +**Files the implementation will touch** — reconciled at rev 5 against +the gate list below and §5, which revision 3 left disagreeing: + +| File | Why | +|---|---| +| `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) | +| `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) | +| `tests/m4_acceptance.rs` | criteria 1–14 including 11a–11d, and 16 | +| `tests/lsp_dispatch_seams_acceptance.rs` | criterion 15's throwing parse stub (Q#RD11) — this file was named in the gate list but omitted from revision 3's touch list | +| `src/bin/pmacs_fake_lsp.rs` | fake modes: blocked delete; edit-then-delete; rename-into-delete; absent-plus-ignore; present-plus-ignore (11a); dangling-symlink (11b); absent-without-ignore (11c); unanswerable-stat (11d) | + +It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, +`builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`. +No protocol change. + +If criterion 15's stub proves cleaner to host in `m4_acceptance` +alongside the rest, that is a permitted simplification — but then +`lsp_dispatch_seams_acceptance` drops out of the gate list too, and the +two lists move together. Revision 3's defect was that they did not. + +**Ownership note — rechecked at rev 5 against #171's pushed revision +8.** The settled split is quoted in §1.12 and Q#RD5 and is carried by +both lanes. Concretely, this lane claims for its duration: + +- the **pre-filesystem refusal** inside synchronous `apply_resource_op`; +- the **shared walk query** of Q#RD6 (`whichever lands first owns the + query`), which #171 then adopts for `reconcile_rename`; +- `builtin/runtime/lsp.lua`'s `apply_workspace_edit` and the + `workspace/applyEdit` server-request boundary. + +It explicitly does **not** claim: full post-delete lifecycle +reconciliation, the dired async race between dispatch and +`remove_blocking`, the rename side of the walk, or `pmacs.fs.remove` +(§6). Revision 2's version of this note was written against a stale +reading of #171 and is superseded.