From aae113ba764a12aa7ae1f5a9dce6f041b2374b28 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 17:54:05 -0400 Subject: [PATCH 1/5] docs(framing): guard the resource-op delete arm against data loss `pmacs.buffer.apply_resource_op`'s delete arm removes the file from disk and then unconditionally drops any buffer bound to that path. There is no dirty check at any link in the chain --- not in the arm, not in `remove_buffer_and_fire`, and not in `BufferRegistry::remove`, whose only guard is `editing_in_progress`. A buffer with unsaved edits is destroyed and the file that would have held them is already gone. Reachable today through any language server's `WorkspaceEdit`. Reproduced four ways against `ad41cf1`, by throwaway tests run in this worktree and removed before commit: a. the reported bug --- op returns `Ok(())`, file gone, buffer gone; b. `ignore_if_not_exists = true` does zero filesystem work and still destroys the buffer (the `create` arm's early return was never applied to delete); 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`: windows are left bound to a removed `BufferId` and the registry can be driven to empty. Recommends refusing before touching disk, at two layers: the Rust primitive reconciles the registry first so the guard is expressible at all, and the applier's existing URI preflight gains a second precondition so the whole batch aborts with its documented `nil, message` contract rather than half-applying. Prompting is rejected on evidence: prompts are callback-continuations resumed by a later keystroke, so a synchronous Rust binding cannot issue one; the alternative is a seventh dispatcher shadow; and the server-initiated path must answer `workspace/applyEdit` synchronously with no user turn available. Backing up is rejected because removing the buffer purges its autosave recovery file. Emacs prior art (eglot's `do-delete`) kills the buffer before deleting the file and confirms server-initiated edits as a whole-batch decision taken before any mutation --- the same shape, in the phase pmacs already has. Framing only. No runtime code. PROPOSED --- needs explicit user approval before implementation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/resource-op-delete-guard-framing.md | 729 +++++++++++++++++++++++ 1 file changed, 729 insertions(+) create mode 100644 docs/resource-op-delete-guard-framing.md diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md new file mode 100644 index 0000000..02414a7 --- /dev/null +++ b/docs/resource-op-delete-guard-framing.md @@ -0,0 +1,729 @@ +# Framing — `apply_resource_op` delete destroys unsaved work + +**Revision 1.** 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` @ `ad41cf1`. + +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. + +## Revision history + +**Revision 1.** First cut. Every claim about pmacs in §1 was verified by +reading the tree at `ad41cf1` and, for §1.1, by running throwaway tests +in this worktree (removed before commit — this lane ships no runtime +code). Every claim about Emacs is marked as such in §1.9 and was +verified against `lisp/progmodes/eglot.el` on `emacs-mirror/emacs` +`master`, not from recollection. + + +## 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. A journey that + eats unsaved edits at step 6 is not a journey worth protecting, so + this serves **Priority 1** ("treat regressions as release blockers") + as a correctness floor rather than a feature. +- **Interaction islands: none added.** This is the load-bearing + constraint, not an afterthought. §6 grades islands "weak, and growing + by one island per modal feature", and the count is **six**. A + confirmation prompt for this op would need a seventh (§2.2), which is + the single strongest argument against the prompt option. +- **Config registry: not adopted.** No knob is proposed. The refusal is + unconditional. An `lsp.confirm-server-edits`-style setting is the + natural future adopter and is parked in §6, not shipped here. +- **Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 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 — no runtime code ships in this lane). Four distinct modes: + +**(a) The reported bug.** Open a file, edit it without saving, apply a +delete op for its path: + +``` +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 then 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. +``` + +This is the sub-case where the buffer is *most* certainly the only +copy, and it is the one arm where the op explicitly decided to do +nothing. Compare the `create` arm, which returns early +(`return Ok(())`) under the analogous `ignore_if_exists` condition; the +delete arm's `Err(NotFound)` branch does not return, it falls through. + +**(c) `recursive = true` fails in the opposite direction.** A recursive +directory delete removes the whole tree from disk but reconciles *no* +buffer, because the lookup is for the directory path and buffers hold +file paths: + +``` +recursive delete result: Ok(()) +inner file exists? false +buffers before=3 after=3 +inner buffer still in registry? true +``` + +The most destructive arm does the least reconciliation. Here the data +survives — in an orphaned buffer pointing at a path that no longer +exists — which is strictly safer than (a) and is why the fix cannot be +"make delete behave like the recursive case". + +**(d) Removal is not `kill_buffer`.** The delete path leaves a window +bound to a removed `BufferId`, and can drive the registry to empty: + +``` +victim is the active buffer? true +delete result: Ok(()) +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`) explicitly refuses +the last buffer and rebinds every window to a fallback. This path does +neither. **(d) is named here for the record and is parked** — see §6 and +Q#RD8; it is a different failure from data loss and it is shared with +`pmacs.buffer.remove`. + +### 1.2 There is no dirty check at any link in the chain + +Three links, verified by reading each: + +1. **`apply_resource_op`'s delete arm** (`src/lua_bindings/mod.rs:3313`) + — stats the path, calls `remove_file` / `remove_dir` / + `remove_dir_all`, then `find_by_path`, then `remove_buffer_and_fire`. + No `is_modified` anywhere in the arm. +2. **`remove_buffer_and_fire`** (`src/lua_bindings/mod.rs:1592`) — four + lines: `registry.borrow_mut().remove(id)`, then + `after_buffer_removed`. `after_buffer_removed` + (`src/lua_bindings/mod.rs:1602`) clears keymaps, config, folds and + fires `on_removed` callbacks. No dirty check. +3. **`BufferRegistry::remove`** (`src/buffer_registry.rs:127`) — has + exactly one guard, and it is 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 user's description of the bug is **accurate at every link**. Modes +(b), (c) and (d) are additional and were not in the report. + +### 1.3 The ordering is disk-first, so no guard placed later can help + +The arm performs the irreversible filesystem operation **before** it has +even looked for a buffer. By the time any `is_modified` check could run, +the file is gone. This is the single most important structural fact in +this document: **a fix that adds a dirty check to the buffer-reconcile +block is not a fix.** It converts "file deleted, buffer destroyed" into +"file deleted, buffer orphaned" — mode (c) — which is better, but the +user's on-disk file is still gone and they never consented. + +### 1.4 `is_modified` is available, singular, and not on `pmacs.buffer` + +- One field, `Buffer::is_modified: bool` (`src/buffer.rs:164`); one + accessor `Buffer::is_modified()` (`src/buffer.rs:473`); one public + mutator `Buffer::mark_clean()` (`src/buffer.rs:488`). There is no + `dirty`, no `saved_revision`, no public setter to true. +- **Zero features refuse or confirm on it today.** `kill_buffer` + (`src/editor_core.rs:4590`) does not check it; `editor.quit` + (`builtin/commands/default.lua:250`) has a veto hook + (`editor.before-quit`) that nothing subscribes to with a modified + check; `dired.revert` does not check it. The one behavioural consumer + is autosave's `gather` filter (`src/autosave.rs:363`), which *includes* + rather than refuses. **So this lane introduces the first refusal in the + codebase keyed on unsaved state**, and should be read as setting that + precedent. +- Lua reaches it as the userdata method `buf:is_modified()` + (`src/lua_bindings/mod.rs:1261`) and as + `pmacs.describe.buffer(id).modified` (`:6359`). It is **not** a key on + the `pmacs.buffer` module table. Relevant because the batch preflight + (Q#RD3) is Lua and needs a path-keyed query it does not have today. + +### 1.5 Who calls this, and what happens to a raised error + +`pmacs.buffer.apply_resource_op` has exactly one production caller: +`apply_workspace_edit` (`builtin/runtime/lsp.lua:1301`), at `:1346`. +That function has three callers: + +| # | Call site | Origin | Error disposition | +|---|---|---|---| +| 1 | `handle_server_requests` (`lsp.lua:1815`), call at `:1836` | **server-initiated** `workspace/applyEdit` | **Swallowed silently.** The pump is driven by `pcall(handle_server_requests)` (`lsp.lua:1892`). A 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` | `apply_workspace_edit` never returns `nil` for an op failure, so a raise propagates out of the `pmacs.async` coroutine. | +| 3 | code action apply (`lsp.lua:2373`) | user, `M-x` | as #2. | + +Only path 1 is fully unattended. This matters: **the refusal cannot be +delivered by raising**, or the most dangerous path reports nothing and +hangs the server's request. That is a design constraint, not a nicety +(Q#RD7). + +Note also `src/rename.rs:25`, which documents the division of labour: +`rename.rs` parses and never mutates; Lua drives `pmacs.buffer.*` +"so the application strategy stays configurable". The application +strategy is the thing this framing is choosing. + +### 1.6 A partial batch is already possible today — verified + +The applier's loop (`lsp.lua:1340-1349`) calls `apply_resource_op` +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. So **partial application on I/O error is +the status quo**, not something a refusal would introduce. This +substantially weakens the "is a partially-applied WorkspaceEdit worse +than the data loss?" objection — the partial batch already exists and +data loss is strictly worse than a class of failure the code already +tolerates. + +It also shows the preflight's documented 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* thing preflighted. The +plan loop (`:1302-1336`) validates nothing about the filesystem or the +buffer registry. **The preflight phase exists and is the natural place +to add a second precondition** (Q#RD3). + +### 1.7 The rename arm is more careful, and differently careful + +Directly above the delete arm, `"rename"` (`mod.rs:3291`) does +`std::fs::rename`, then `find_by_path`, then +`core.borrow_mut().set_buffer_path(id, Some(to))`. It **rebinds** the +buffer and preserves its contents and its modified state. Delete +**destroys**. The asymmetry is the whole bug: rename treats the buffer +as the valuable thing and the path as the mutable attribute; delete +treats the buffer as a cache of the file. + +Both arms share two latent defects, already recorded against the dired +arc: `find_by_path` (`src/buffer_registry.rs:168`) is exact `Path` +equality, first match only, called with the raw path while stored paths +are normalized on write. `EditorCore::find_buffer_for_path` +(`src/editor_core.rs:935`) is the normalizing wrapper that exists and is +bypassed. See `docs/dired-framing.md:807-819` and the ledger note at +`docs/active-work.md:605`, both of which claim the **rename** side of +this for dired Stage 2. §6 draws the boundary. + +### 1.8 What pmacs cannot do, established rather than assumed + +- **A prompt cannot be issued from inside this binding.** + `pmacs.minibuffer.read` (`src/lua_bindings/mod.rs:13380`) is + asynchronous-by-callback: it registers `on_accept`/`on_cancel` and + returns immediately. `Minibuffer::accept` + (`src/minibuffer.rs:334`) deliberately *returns* the callback rather + than calling it, with the reason stated at `:332` — "firing user code + from inside the minibuffer would re-enter the registry". The answer + arrives on a later keystroke, through the event loop. + `apply_resource_op` is a synchronous Rust closure that performs its + `std::fs` calls inline and returns; there is no point at which it can + suspend. **There is no `y_or_n` helper in the tree at all** — a named + deferral in `docs/dired-framing.md:854`. +- **Autosave cannot be used 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-169` calls + `discard_buffer` (`src/autosave.rs:511`), and a sweep-time GC + (`:290-306`) catches whatever the callback misses. Pinned by + `tests/autosave_acceptance.rs:702`. + - deleting the file flips the recovery's status to `Stale`, and + `Stale` is never auto-offered. + + So "back up, then delete" is self-defeating four times over: no + entry point, wrong filter, the backup is deleted by the very removal + it was protecting against, and what survives is never surfaced. +- **`pmacs.editor.set_status` is the available report channel** + (`src/lua_bindings/mod.rs:13036`), cleared at the top of every + `dispatch_key`. There is no `*Messages*` buffer and no `*warnings*` + buffer. `pmacs.error` is referenced in `async.lua` and **never + defined**. + +### 1.9 Prior art — claims about **Emacs**, not pmacs + +Verified against `lisp/progmodes/eglot.el`, `emacs-mirror/emacs` +`master`. Everything in this subsection is a statement about Emacs. + +**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. In Emacs, +`kill-buffer` on a modified file-visiting buffer prompts, so the consent +gate sits ahead of the irreversible step by construction. (Eglot does +not check `kill-buffer`'s return value, so declining the kill still +deletes the file — but the buffer, and therefore the text, survives. +Even Eglot's failure mode is strictly milder than pmacs's.) + +Note also the `exists` guard: Eglot's `ignoreIfNotExists` path does +**not** fall through to the buffer kill. That is exactly the asymmetry +mode (b) exposes in pmacs. + +**Eglot confirms server-initiated edits by default, as a whole-batch +decision made before anything is applied.** `eglot-confirm-server-edits` +defaults to `'((t . maybe-summary))`. The prepare/decide/apply structure +is explicit: `prepare` builds a list of closures touching nothing, then +`eglot--confirm-server-edits` decides, then `apply-all` runs. 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 is never `peaceful` and always +prompts** under the default. Confirmation is all-or-nothing and +strictly precedes mutation; there is no mid-batch interrupt. + +**The transferable lessons** (now claims about what pmacs should do): +the consent gate belongs *before* the irreversible step; the decision is +made over the *whole* batch during a preparation phase; and +`ignoreIfNotExists` must short-circuit the buffer half too. + +### 1.10 `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 in `src/lua_bindings/mod.rs`. Zero tests name it. + +It is exercised indirectly by exactly one acceptance, +`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`), which emits an ordered +create / edit / rename / delete. **Its deleted file `c.rs` is never +opened in a buffer**, so the entire buffer-reconciliation half of the +delete arm is untested. That suite and that fake are where this lane's +acceptances belong. + + +## 2. The decision space + +### 2.1 Recommended — refuse before touching disk, at both layers + +**Refuse the delete when it would destroy unsaved work, and refuse it +before the filesystem call. Do this at the Rust primitive (the +invariant) *and* in the applier's existing preflight (batch +atomicity).** + +Concretely: + +- **Layer 1, the primitive.** The delete arm reconciles the buffer + registry *first*. If the path resolves to a modified buffer, it + returns an error and touches nothing. Inverting the order is what + makes the guard possible at all (§1.3), and it also closes a smaller + hole: today a `ConcurrentEdit` refusal from `BufferRegistry::remove` + arrives after the file is already gone. +- **Layer 2, the applier preflight.** `apply_workspace_edit`'s plan loop + (`lsp.lua:1302-1336`) gains a second precondition alongside URI + resolution, and returns its existing `nil, message` when any delete op + targets a modified buffer — so **nothing** in the batch is mutated. + +Both layers are needed, and neither is redundant: + +- Layer 1 alone leaves partial batches (§1.6): op 1 deletes, op 2 + refuses, and the user is left mid-edit with no way back. +- Layer 2 alone leaves the primitive armed. `pmacs.buffer.apply_resource_op` + is public Lua API; dired Stage 2 and any package can call it directly. + A guard that lives only in one caller is exactly the shape the project + has been burned by — "pin the guard through the real path". + +**Why this beats the runners-up, in one sentence each:** + +- It is the only option that is *possible* — prompting is architecturally + unavailable (§1.8) — and it is the option Emacs's structure already + endorses (§1.9): consent gate before the irreversible step, decision + taken over the whole batch during preparation. + +### 2.2 Prompt the user — rejected + +Rejected on three independent grounds, any one sufficient. + +1. **Architecturally unavailable.** `apply_resource_op` is synchronous + Rust; prompts are callback-continuations resumed by a later keystroke + (§1.8). Making this work means restructuring the applier into a + callback chain carrying the remaining plan as a closure upvalue, with + revalidation of every precondition at each resumption — a large, + independently-risky change to LSP edit application, for a guard. +2. **It costs an interaction island.** The alternative to a callback + chain is a Rust modal shadow (the query-replace shape). That is a + **seventh** dispatcher shadow. COHERENCE §6 grades this area "weak, + and growing by one island per modal feature" and records that + terminal copy mode was deliberately engineered *not* to become the + seventh. Spending that budget on an error path would be a poor trade. +3. **It cannot serve the most dangerous caller.** The server-initiated + path (§1.5, row 1) must answer `workspace/applyEdit` synchronously + with `applied: true | false`. There is no user turn available inside + it, and a prompt that resolves three keystrokes later cannot produce + that answer. + +Note that "what happens to the rest of the batch?" — the question that +makes prompting genuinely hard — is dissolved by the recommendation +rather than answered: the decision is taken in the preflight, before any +op runs, so there is no rest-of-batch to strand. + +### 2.3 Save first, then delete — rejected + +Silently converts an unsaved edit into a committed one and then destroys +it. It is *more* destructive than refusing, not less: it overwrites the +user's on-disk original — the copy they might have wanted — immediately +before removing the file. It also cannot be relied on: `save_inner` +refuses when the file changed on disk since it was read +(`src/editor_core.rs:1908`, guard at `:1917`), so the fallback question is unanswered and +we are back to refusing. + +### 2.4 Back up the contents somewhere recoverable — rejected + +Rejected on evidence, not taste. §1.8 establishes that the existing +autosave machinery defeats this four ways, the decisive one being that +**removing the buffer deletes the recovery file** — the backup is +destroyed by the very operation it exists to survive. Building a +parallel side-store outside `autosave/` means inventing a second +recovery surface with its own discovery, GC and lifecycle, to make a +destructive operation *feel* safe. Refusing is cheaper and honest. + +### 2.5 Key the behaviour on LSP-versus-user provenance — rejected + +Superficially attractive because Eglot's default keys on exactly this +(`eglot-confirm-server-edits`). But: `apply_resource_op` takes no +provenance argument and there is no ambient caller identity to read. +Adding one makes the primitive's safety depend on a caller-supplied +flag — any caller that omits it, or passes the permissive value, is +unguarded, which is the failure mode the whole lane exists to remove. +COHERENCE §10 (extension trust classes) is unbuilt, so there is no +existing trust dimension to key on either. **The refusal is +unconditional and provenance-blind.** A future +`lsp.confirm-server-edits` setting can *loosen* it once a prompt +mechanism exists; §6. + + +## 3. Decisions + +### Q#RD1 — Refuse. Do not prompt, do not save, do not back up + +An `apply_resource_op` delete whose target resolves to a modified buffer +**fails**, changing nothing on disk and nothing in the registry. +Rationale: §2.2–§2.5. 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 — Reconcile the registry before touching the filesystem + +The delete arm's order inverts: resolve the buffer set, decide, and only +then call `remove_file` / `remove_dir` / `remove_dir_all`. This is what +makes the guard expressible (§1.3) and it also moves the pre-existing +`ConcurrentEdit` refusal ahead of the irreversible step. + +**What inverting breaks, considered:** the current order means a +successful `remove_buffer_and_fire` implies the file is already gone, +so `on_removed` subscribers observe a consistent "file and buffer both +gone" world. After inversion, `on_removed` fires with the file still +present for the remainder of the call. Two mitigations: the fs call +follows immediately with no yield point in between (Lua callbacks run +synchronously inside `after_buffer_removed`), and if the fs call then +fails the correct end state is *ambiguous either way* — today it cannot +happen because the buffer removal is unreachable on fs failure. Q#RD9 +records the resolution. + +### Q#RD3 — The batch aborts in the preflight, before any op runs + +`apply_workspace_edit`'s plan loop gains a modified-buffer precondition +for delete ops and returns `nil, message`. This reuses the applier's +existing, documented abort contract — "aborts the whole edit cleanly, +origin buffer untouched, rather than half-applying" +(`lsp.lua:1287-1291`) — and all three callers already handle the +`nil, message` shape (§1.5). It is Eglot's prepare/decide/apply shape +(§1.9) implemented in the phase pmacs already has. + +The preflight needs a path-keyed modified query, which Lua lacks today +(§1.4). The minimal addition is a `pmacs.buffer` surface answering +"is there a modified buffer at or beneath this path"; its exact shape is +an implementation choice, but it must be **one** query so the preflight +and the primitive cannot drift apart. + +### Q#RD4 — `ignore_if_not_exists` short-circuits the buffer half too + +When the path is absent and `ignore_if_not_exists` is set, the arm +returns early **without** touching the registry — matching the `create` +arm's existing `return Ok(())` idiom and Eglot's `exists` guard (§1.9). +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 prefix-aware, or the guard has a documented bypass + +Mode (c) proves that `recursive = true` reconciles nothing, so an +exact-path guard is bypassed entirely by the most destructive arm: a +server that sends `{kind: "delete", uri: , recursive: true}` walks +straight past it. A guard with a trivial, reachable bypass is not a +guard, so the check must cover every buffer whose path lies **beneath** +the deleted directory, not merely one whose path equals it. + +**This overlaps dired Stage 2 and the boundary is drawn explicitly.** +`docs/dired-framing.md:807-819` and `docs/active-work.md:605` claim +prefix-aware, normalize-before-lookup rebinding for the **rename** side. +This lane takes the **delete** side only, because without it this lane +ships nothing. The two want the same helper; whichever lands second +adopts the first's. If the user prefers, the alternative is to sequence +this lane after dired Stage 2 and consume its helper — but the bug is +live and dired Stage 2 is unframed for this, so shipping first is +recommended. + +### Q#RD6 — Lookups normalize; "modified" means `Buffer::is_modified` + +The guard resolves paths through the normalizing wrapper +(`EditorCore::find_buffer_for_path`, `src/editor_core.rs:935`) rather +than raw `find_by_path`, because a lookup miss is a *silent* guard +bypass (§1.7). "Modified" is `Buffer::is_modified()` — the single +existing predicate (§1.4). No new notion of dirtiness is introduced. + +Explicitly **not** guarded: a clean buffer. A delete whose target is +open but unmodified proceeds and removes the buffer, exactly as today. +Overreach here would break `m4_15` and, more importantly, would make the +LSP's legitimate deletes fail for users who merely have the file open. + +### Q#RD7 — The refusal is reported on every path, and never by raising alone + +Per §1.5, a raise on the server-initiated path is swallowed by +`pcall(handle_server_requests)` and the server is left unanswered. So: + +- **Preflight refusal (all three callers)** returns `nil, message`. + Callers 2 and 3 already render that to `set_status`; caller 1 already + turns it into `{ applied = false, failureReason = ... }` and sends the + response. +- **Primitive refusal** still raises — it must, being a Rust binding — + but that is now a defence-in-depth path reached only by direct callers, + because the preflight catches the LSP path first. + +The message names the buffer and says what to do: save it, or use the +buffer-level command to discard. It must not be a bare errno. + +### Q#RD8 — The window/last-buffer defects do **not** land here + +Mode (d) is real and is parked (§6). Two reasons. It is a different +failure (a dangling window and an empty registry, not data loss), and it +is shared with `pmacs.buffer.remove` rather than specific to delete. + +**And there is a 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 of the other, so "just call `kill_buffer` +instead" would silently regress four cleanups. Unifying them is its own +lane with its own census. + +### Q#RD9 — On filesystem failure after the buffer is removed, the buffer wins + +Given Q#RD2's inversion, a delete can now fail *after* the buffer is +gone. The buffer is not restored. Rationale: the buffer removal is only +reached for a clean buffer (Q#RD1), so nothing unsaved is at stake, and +re-inserting a buffer would need a new registry primitive and would +resurrect it with a fresh `BufferId` that no window, keymap or callback +refers to. The op reports the fs error as it does today. This is a +deliberate, narrow widening of the failure surface and is called out so +review can reject it rather than discover it. + + +## 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; no server needs that delete to succeed silently. + Falsified by a real server whose normal operation deletes files the + user is actively editing. +- **B2 — The preflight is the right layer for batch atomicity.** + Falsified if a `WorkspaceEdit` legitimately depends on a delete whose + precondition can only be evaluated after an earlier op runs (e.g. a + rename that moves the modified buffer out of the delete's path first). + **This is the sharpest risk in the design** and acceptance 8 pins the + behaviour so the failure is loud rather than silent. +- **B3 — Prefix-aware checking 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 — Inverting the order breaks no `on_removed` subscriber.** + Evidence: the fs call follows synchronously with no yield in between. + Falsified by a subscriber that stats the path. + + +## 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 three things 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 (today: `Ok(())`, file + gone, buffer gone). **Asserting only that the buffer survived is + vacuous** — that is mode (c)'s behaviour, which this lane must not + ship. The `exists()` assertion is the load-bearing one. + +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**, per the bottom-panel + lesson that a blanket rewrite passes an "everything moved" test. + +3. **`ignore_if_not_exists = true` on an absent path leaves a modified + buffer intact** (Q#RD4). + *Bite:* fails against a fix that guards only the branch where the fs + delete actually ran — reproduce mode (b) exactly: file removed behind + pmacs's back first, then the op. + +4. **`recursive = true` on a directory containing a modified buffer's + file refuses, and the whole tree survives** (Q#RD5). + *Bite:* fails against an exact-path-equality guard. Assert the inner + file still exists — not just that the buffer does, which is already + true today (mode (c)). + +5. **Whole-batch atomicity: a `documentChanges` whose *second* op is a + blocked delete leaves the *first* op unapplied** (Q#RD3). + *Bite:* fails against a primitive-only fix. §1.6 verified that op 1 + currently stays applied, so this is a real behaviour change and the + assertion must name op 1's effect (e.g. a file that must still exist, + or must not yet have been created). + +6. **The refusal reaches the server on the unattended path.** Drive a + server-initiated `workspace/applyEdit` carrying a blocked delete and + assert the server receives a response with `applied = false` and a + non-empty `failureReason`. + *Bite:* fails against a fix that refuses by raising — §1.5 established + that `pcall(handle_server_requests)` (`lsp.lua:1892`) swallows the + raise and the response is never sent. This is the "pin the guard + through the outermost user-reachable seam" obligation; a direct-call + test on `apply_resource_op` does not satisfy it and is rejected as + insufficient for this criterion. + +7. **The user-initiated paths report on the status line.** LSP rename + and code action each surface a message naming the buffer. + *Bite:* fails against a fix that returns `nil` without a message, or + whose message is a bare errno. + +8. **The B2 risk is pinned:** a batch that renames the modified buffer's + file *and then* deletes the old path is refused by the preflight + rather than silently mis-evaluated. Whichever behaviour review + chooses, it is asserted, so the limitation is a documented decision + rather than an accident. + +9. **`m4_15_workspace_edit_resource_ops_apply_in_order` stays green** + unmodified, pinning "no regression to the ordered-resource-op path" + from outside. Its `c.rs` is never opened, so it exercises exactly the + unguarded case that must keep working (Q#RD6). + +10. **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). Real, reachable, shared with `pmacs.buffer.remove`. Needs its + own lane and a census, because the two removal paths clean disjoint + sets and the obvious unification regresses four cleanups. +- **`kill_buffer` and `editor.quit` have the same gap** (§1.4): both + discard unsaved work with no check. `editor.before-quit` exists as a + veto channel with no subscriber. This lane sets the precedent; those + are separate lanes. +- **A `y_or_n` helper**, and with it any confirm-instead-of-refuse + option. Already a named deferral (`docs/dired-framing.md:854`). +- **`lsp.confirm-server-edits`**, the config-registry adopter that would + let a user loosen Q#RD1 once a prompt mechanism exists (§2.5). +- **The rename side of prefix-aware, normalizing lookup** — dired + Stage 2's, per Q#RD5. +- **A general transient-keymap layer** (COHERENCE §6), the prerequisite + that would make §2.2 cheap rather than impossible. + + +## 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.10) and +`lsp_dispatch_seams_acceptance`. `dired_acceptance` is a watch item for +Q#RD5's shared lookup change. + +Gate the pushed tree, not the worktree — commit first, then gate. + +This framing-only PR ships no runtime code, so its own gate is +`git diff --check` plus a docs read. + + +## 8. Branch plan + +`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, one +PR. **This framing is the entire first PR.** Implementation does not +begin until the user approves the design — specifically Q#RD1 (refuse +rather than prompt), Q#RD5 (take the delete side of prefix-awareness now +rather than sequencing behind dired Stage 2), and Q#RD9 (the buffer is +not restored on fs failure). + +Files this lane will touch when approved: `src/lua_bindings/mod.rs` +(the delete arm and a modified-at-or-beneath query), +`builtin/runtime/lsp.lua` (the preflight), `tests/m4_acceptance.rs` and +`src/bin/pmacs_fake_lsp.rs` (a fake mode carrying a blocked delete). +It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, or +`builtin/runtime/dired.lua`. No protocol change. From e1e9b441545257cd6ad14e1104a34b45c4dd5046 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 18:23:52 -0400 Subject: [PATCH 2/5] docs(framing): revision 2 of the resource-op delete guard Review round 1 approved the refusal strategy in principle and rejected revision 1 as written. Six blocking points, all accepted, plus three further overclaims found by the requested sweep. Q#RD1 and Q#RD5 are settled yes; Q#RD9 is settled no and is withdrawn. Q#RD2 conflated inspection with removal. Revision 1 removed the buffer before the filesystem call, which fires arbitrary Lua `on_removed` callbacks while the file still exists and accepts losing the buffer if the deletion then fails. The sequence is now `stat/no-op -> enumerate and validate -> mutate filesystem -> reconcile`. Validation inspects `is_modified` and `editing_in_progress` without removing anything, so a failed deletion leaves buffers intact automatically and `on_removed` still observes the path already gone. Q#RD3 overclaimed whole-batch atomicity. `documentChanges` are sequential, so an earlier edit can dirty a clean buffer and an earlier rename can move a modified buffer into a later delete's subtree, after the snapshot. LSP 3.18 assigns `FailureHandlingKind.Abort` to any edit containing resource changes --- "all operations executed before the failing operation stay executed" --- so the protocol itself declines to promise what revision 1 claimed. The preflight is now described as an early conflict check, with robustness coming from per-op `pcall`, an always-sent server response, and best-effort origin restore. The lookup cannot be `EditorCore::find_buffer_for_path`: it normalizes but delegates to the first-match-only `find_by_path`, and `pmacs.buffer.from_file` creates path-bound buffers with no dedup, so a clean first match can hide a modified second. Q#RD6 now requires a full scan with component-aware `Path::starts_with`. Recursive deletion now inspects the tree but reconciles only the exact path, so the parked lifecycle defect stays exact-path rather than becoming tree-wide. Q#RD4 holds at both layers, so the preflight cannot reject an absent path the primitive treats as a no-op. The prompt argument was overclaimed and that was my error. `pmacs.lsp.send_response` 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 rather than add a seventh dispatcher rung. Prompting is expensive and separately scoped, not impossible; the section now claims only what the evidence carries. Sweep found three more of the same defect class --- an absence or a guarantee asserted rather than established: * a durable error surface does exist (`append_to_errors_buffer` -> `*errors*`), so Q#RD7 now records the refusal there as well; * no caller reliably surfaces a raise, because the async path routes uncaught coroutine errors through the undefined `pmacs.error`; * pmacs advertises no `workspace.workspaceEdit` capability at all --- no `documentChanges`, no `resourceOperations`, no `failureHandling`. Adds seven acceptance pins with their bite obligations, adds the `docs/active-work.md` lane the ledger requires for every open PR, and drops the two-PR plan: #186 is revised in place and becomes the implementation PR. Still PROPOSED. No runtime code. Implementation begins only after explicit user approval. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 72 ++ docs/resource-op-delete-guard-framing.md | 1164 +++++++++++++--------- 2 files changed, 781 insertions(+), 455 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index a303151..7bac9fc 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -459,6 +459,78 @@ has **no branch and no framing yet**. `FrontendView.fold_projection` to `true` for semantic frontends, which Stage 2 deliberately left `false` (Q#FD21). +## Resource-op delete guard lane — PR #186 OPEN, PROPOSED, DO NOT MERGE + +- Portable branch: `githubsucks/resource-op-delete-guard`; worktree + `../pmacs-resource-op-delete`. **PR #186**, base `main`, forked from + `ad41cf1` with **no drift** (`main` is still `ad41cf1`). Currently + framing only — `docs/resource-op-delete-guard-framing.md`, revision 2 + — plus this lane entry. No runtime code yet. +- **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 1 rejected. + Settled: refuse unconditionally; take delete-side prefix-awareness now + rather than waiting on #171. Withdrawn: rev 1's buffer-first ordering. + The design is now `stat/no-op → enumerate and validate → mutate + filesystem → reconcile`, which keeps `on_removed`'s "path already + gone" invariant and makes a failed deletion leave buffers intact + automatically. +- **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: two delete + ops, the second raises, the first stayed applied. LSP 3.18 says so + too: resource-op-bearing edits get `FailureHandlingKind.Abort`, + "all operations executed before the failing operation stay + executed". Any framing claiming batch atomicity here is wrong. + - **`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, per the dired lane's own warning below:** dired + Stage 2a is "rename/delete reconciliation substrate" and overlaps + `builtin/runtime/lsp.lua`. **This lane claims the delete half of that + substrate and the `apply_workspace_edit` applier for its duration**; + dired Stage 2 keeps the rename half. Whichever lands second adopts the + first's shared lookup helper. Do not run the two concurrently over + `builtin/runtime/lsp.lua` without re-splitting that claim. +- Files the implementation will touch: `src/lua_bindings/mod.rs`, + `builtin/runtime/lsp.lua`, `tests/m4_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 OPEN, STALE, DO NOT MERGE AS-IS - Portable branch: `githubsucks/dired-stage2-framing` (head `ab42a79`, diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 02414a7..352f30b 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,23 +1,120 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 1.** Status: **PROPOSED — needs explicit user approval before +**Revision 2.** 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` @ `ad41cf1`. +based on `githubsucks/main` @ `ad41cf1` (re-checked at revision 2: no +drift, `main` is still `ad41cf1`). 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 1.** First cut. Every claim about pmacs in §1 was verified by -reading the tree at `ad41cf1` and, for §1.1, by running throwaway tests -in this worktree (removed before commit — this lane ships no runtime -code). Every claim about Emacs is marked as such in §1.9 and was -verified against `lisp/progmodes/eglot.el` on `emacs-mirror/emacs` -`master`, not from recollection. +### 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) @@ -26,21 +123,24 @@ verified against `lisp/progmodes/eglot.el` on `emacs-mirror/emacs` 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. A journey that - eats unsaved edits at step 6 is not a journey worth protecting, so - this serves **Priority 1** ("treat regressions as release blockers") - as a correctness floor rather than a feature. -- **Interaction islands: none added.** This is the load-bearing - constraint, not an afterthought. §6 grades islands "weak, and growing - by one island per modal feature", and the count is **six**. A - confirmation prompt for this op would need a seventh (§2.2), which is - the single strongest argument against the prompt option. -- **Config registry: not adopted.** No knob is proposed. The refusal is - unconditional. An `lsp.confirm-server-edits`-style setting is the - natural future adopter and is parked in §6, not shipped here. + 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. + 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`) @@ -49,11 +149,10 @@ verified against `lisp/progmodes/eglot.el` on `emacs-mirror/emacs` `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 — no runtime code ships in this lane). Four distinct modes: +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.** Open a file, edit it without saving, apply a -delete op for its path: +**(a) The reported bug.** ``` PRE: modified=true text="UNSAVED EDIT ORIGINAL ON DISK\n" @@ -67,7 +166,7 @@ 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 then falls through to the buffer +arm skips the `remove_file` — and falls through to the buffer reconciliation anyway: ``` @@ -77,16 +176,11 @@ buffers after=2 CONFIRMED: ignore_if_not_exists=true did ZERO fs work yet still destroyed the only copy. ``` -This is the sub-case where the buffer is *most* certainly the only -copy, and it is the one arm where the op explicitly decided to do -nothing. Compare the `create` arm, which returns early -(`return Ok(())`) under the analogous `ignore_if_exists` condition; the -delete arm's `Err(NotFound)` branch does not return, it falls through. +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.** A recursive -directory delete removes the whole tree from disk but reconciles *no* -buffer, because the lookup is for the directory path and buffers hold -file paths: +**(c) `recursive = true` fails in the opposite direction.** ``` recursive delete result: Ok(()) @@ -95,17 +189,16 @@ buffers before=3 after=3 inner buffer still in registry? true ``` -The most destructive arm does the least reconciliation. Here the data -survives — in an orphaned buffer pointing at a path that no longer -exists — which is strictly safer than (a) and is why the fix cannot be -"make delete behave like the recursive case". +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`.** The delete path leaves a window -bound to a removed `BufferId`, and can drive the registry to empty: +**(d) Removal is not `kill_buffer`.** ``` victim is the active buffer? true -delete result: Ok(()) window.buffer():is_valid() after delete => Ok(Boolean(false)) editor.file_path() after delete => Ok(Nil) ``` @@ -115,27 +208,20 @@ delete of the LAST buffer: Ok(()) buffers after: 0 (0 => registry driven empty) ``` -`EditorCore::kill_buffer` (`src/editor_core.rs:4590`) explicitly refuses -the last buffer and rebinds every window to a fallback. This path does -neither. **(d) is named here for the record and is parked** — see §6 and -Q#RD8; it is a different failure from data loss and it is shared with -`pmacs.buffer.remove`. +`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 -Three links, verified by reading each: - -1. **`apply_resource_op`'s delete arm** (`src/lua_bindings/mod.rs:3313`) - — stats the path, calls `remove_file` / `remove_dir` / - `remove_dir_all`, then `find_by_path`, then `remove_buffer_and_fire`. - No `is_modified` anywhere in the arm. -2. **`remove_buffer_and_fire`** (`src/lua_bindings/mod.rs:1592`) — four - lines: `registry.borrow_mut().remove(id)`, then - `after_buffer_removed`. `after_buffer_removed` - (`src/lua_bindings/mod.rs:1602`) clears keymaps, config, folds and - fires `on_removed` callbacks. No dirty check. -3. **`BufferRegistry::remove`** (`src/buffer_registry.rs:127`) — has - exactly one guard, and it is not this one: +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) @@ -148,65 +234,87 @@ Three links, verified by reading each: That refuses re-entrant removal from inside an edit intercept (T M7.4). It says nothing about unsaved content. -The user's description of the bug is **accurate at every link**. Modes -(b), (c) and (d) are additional and were not in the report. +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 -even looked for a buffer. By the time any `is_modified` check could run, -the file is gone. This is the single most important structural fact in -this document: **a fix that adds a dirty check to the buffer-reconcile -block is not a fix.** It converts "file deleted, buffer destroyed" into -"file deleted, buffer orphaned" — mode (c) — which is better, but the -user's on-disk file is still gone and they never consented. +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 not on `pmacs.buffer` +### 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 `Buffer::is_modified()` (`src/buffer.rs:473`); one public - mutator `Buffer::mark_clean()` (`src/buffer.rs:488`). There is no - `dirty`, no `saved_revision`, no public setter to true. -- **Zero features refuse or confirm on it today.** `kill_buffer` - (`src/editor_core.rs:4590`) does not check it; `editor.quit` - (`builtin/commands/default.lua:250`) has a veto hook - (`editor.before-quit`) that nothing subscribes to with a modified - check; `dired.revert` does not check it. The one behavioural consumer - is autosave's `gather` filter (`src/autosave.rs:363`), which *includes* - rather than refuses. **So this lane introduces the first refusal in the - codebase keyed on unsaved state**, and should be read as setting that + 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. -- Lua reaches it as the userdata method `buf:is_modified()` - (`src/lua_bindings/mod.rs:1261`) and as +- **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. Relevant because the batch preflight - (Q#RD3) is Lua and needs a path-keyed query it does not have today. + 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 -`pmacs.buffer.apply_resource_op` has exactly one production caller: -`apply_workspace_edit` (`builtin/runtime/lsp.lua:1301`), at `:1346`. -That function has three callers: +One production caller: `apply_workspace_edit` +(`builtin/runtime/lsp.lua:1301`), at `:1346`. Three callers of that: -| # | Call site | Origin | Error disposition | +| # | Call site | Origin | Disposition of a raise | |---|---|---|---| -| 1 | `handle_server_requests` (`lsp.lua:1815`), call at `:1836` | **server-initiated** `workspace/applyEdit` | **Swallowed silently.** The pump is driven by `pcall(handle_server_requests)` (`lsp.lua:1892`). A 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` | `apply_workspace_edit` never returns `nil` for an op failure, so a raise propagates out of the `pmacs.async` coroutine. | +| 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. | -Only path 1 is fully unattended. This matters: **the refusal cannot be -delivered by raising**, or the most dangerous path reports nothing and -hangs the server's request. That is a design constraint, not a nicety -(Q#RD7). +**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 — -Note also `src/rename.rs:25`, which documents the division of labour: -`rename.rs` parses and never mutates; Lua drives `pmacs.buffer.*` -"so the application strategy stays configurable". The application -strategy is the thing this framing is choosing. +```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 `apply_resource_op` +The applier's loop (`lsp.lua:1340-1349`) calls the primitive unprotected. Two delete ops where the second raises: ``` @@ -214,80 +322,140 @@ batch result: Err(... "apply_resource_op delete: No such file or directory (os e a.txt still exists? false (false ⇒ partial batch) ``` -The first op stayed applied. So **partial application on I/O error is -the status quo**, not something a refusal would introduce. This -substantially weakens the "is a partially-applied WorkspaceEdit worse -than the data loss?" objection — the partial batch already exists and -data loss is strictly worse than a class of failure the code already -tolerates. +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 documented contract is narrower than its -comment implies. `lsp.lua:1287-1291` says the applier "refuses to mutate +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* thing preflighted. The -plan loop (`:1302-1336`) validates nothing about the filesystem or the -buffer registry. **The preflight phase exists and is the natural place -to add a second precondition** (Q#RD3). +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 rename arm is more careful, and differently careful +### 1.7 The batch is sequential, and the protocol says so -Directly above the delete arm, `"rename"` (`mod.rs:3291`) does -`std::fs::rename`, then `find_by_path`, then -`core.borrow_mut().set_buffer_path(id, Some(to))`. It **rebinds** the -buffer and preserves its contents and its modified state. Delete -**destroys**. The asymmetry is the whole bug: rename treats the buffer -as the valuable thing and the path as the mutable attribute; delete +Claims about **the LSP specification** (3.18), not about pmacs: + +- "If resource operations are present, clients need to execute the + operations in the order in which they are provided." +- `FailureHandlingKind.Abort`: "All operations executed before the + failing operation stay executed." +- `FailureHandlingKind.TextOnlyTransactional`: "If the workspace edit + contains only textual file changes they are executed transactionally. + **If resource changes are part of the change the failure handling + strategy is abort.**" + +So the protocol itself declines to promise transactionality for exactly +the edits this lane is about. **This is the evidence that revision 1's +"whole-batch atomicity" claim was unsupportable**, and the reason Q#RD3 +now describes an early conflict check instead. Sequential execution is +also why a snapshot preflight is necessarily incomplete: an earlier op +can change the facts a later op's precondition was evaluated against. + +### 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, so §1.7's `Abort` semantics are the de facto behaviour by +omission rather than by choice. **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 two latent defects, already recorded against the dired -arc: `find_by_path` (`src/buffer_registry.rs:168`) is exact `Path` -equality, first match only, called with the raw path while stored paths -are normalized on write. `EditorCore::find_buffer_for_path` -(`src/editor_core.rs:935`) is the normalizing wrapper that exists and is -bypassed. See `docs/dired-framing.md:807-819` and the ledger note at -`docs/active-work.md:605`, both of which claim the **rename** side of -this for dired Stage 2. §6 draws the boundary. +Both arms share the §1.4 lookup defects. `docs/dired-framing.md:807-819` +and the dired Stage 1 entry under "Closed since the last snapshot" in +`docs/active-work.md` claim the **rename** side for dired Stage 2. §6 +draws the boundary; the dired lane is recorded as **OPEN, STALE, DO NOT +MERGE AS-IS** and under re-scout, which is why this lane does not wait +on it. -### 1.8 What pmacs cannot do, established rather than assumed - -- **A prompt cannot be issued from inside this binding.** - `pmacs.minibuffer.read` (`src/lua_bindings/mod.rs:13380`) is - asynchronous-by-callback: it registers `on_accept`/`on_cancel` and - returns immediately. `Minibuffer::accept` - (`src/minibuffer.rs:334`) deliberately *returns* the callback rather - than calling it, with the reason stated at `:332` — "firing user code - from inside the minibuffer would re-enter the registry". The answer - arrives on a later keystroke, through the event loop. - `apply_resource_op` is a synchronous Rust closure that performs its - `std::fs` calls inline and returns; there is no point at which it can - suspend. **There is no `y_or_n` helper in the tree at all** — a named - deferral in `docs/dired-framing.md:854`. -- **Autosave cannot be used 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-169` calls - `discard_buffer` (`src/autosave.rs:511`), and a sweep-time GC - (`:290-306`) catches whatever the callback misses. Pinned by - `tests/autosave_acceptance.rs:702`. - - deleting the file flips the recovery's status to `Stale`, and - `Stale` is never auto-offered. - - So "back up, then delete" is self-defeating four times over: no - entry point, wrong filter, the backup is deleted by the very removal - it was protecting against, and what survives is never surfaced. -- **`pmacs.editor.set_status` is the available report channel** - (`src/lua_bindings/mod.rs:13036`), cleared at the top of every - `dispatch_key`. There is no `*Messages*` buffer and no `*warnings*` - buffer. `pmacs.error` is referenced in `async.lua` and **never - defined**. - -### 1.9 Prior art — claims about **Emacs**, not pmacs +### 1.13 Prior art — claims about **Emacs**, not pmacs Verified against `lisp/progmodes/eglot.el`, `emacs-mirror/emacs` -`master`. Everything in this subsection is a statement about Emacs. +`master`. **Eglot orders the operations the other way round.** Its `do-delete`: @@ -303,23 +471,26 @@ Verified against `lisp/progmodes/eglot.el`, `emacs-mirror/emacs` (delete-file path recursive)))) ``` -The buffer is killed **before** the file is deleted. In Emacs, -`kill-buffer` on a modified file-visiting buffer prompts, so the consent -gate sits ahead of the irreversible step by construction. (Eglot does -not check `kill-buffer`'s return value, so declining the kill still -deletes the file — but the buffer, and therefore the text, survives. -Even Eglot's failure mode is strictly milder than pmacs's.) +The buffer is killed **before** the file is deleted, and in Emacs +`kill-buffer` on a modified file-visiting buffer prompts — so the +consent gate precedes the irreversible step. (Eglot ignores +`kill-buffer`'s return value, so declining still deletes the file, but +the buffer and its text survive. Even that failure mode is milder than +pmacs's.) Note also that the `exists` guard means `ignoreIfNotExists` +does **not** fall through to the buffer kill — the asymmetry mode (b) +exposes in pmacs. -Note also the `exists` guard: Eglot's `ignoreIfNotExists` path does -**not** fall through to the buffer kill. That is exactly the asymmetry -mode (b) exposes in pmacs. +*Revision 2 note:* Emacs's ordering is **not** what Q#RD2 adopts. +Emacs can afford buffer-first because `kill-buffer` is itself the +consent gate; pmacs has no such gate, so it validates first and +reconciles last (Q#RD2), which yields the same safety without firing +callbacks against a file that still exists. **Eglot confirms server-initiated edits by default, as a whole-batch -decision made before anything is applied.** `eglot-confirm-server-edits` -defaults to `'((t . maybe-summary))`. The prepare/decide/apply structure -is explicit: `prepare` builds a list of closures touching nothing, then -`eglot--confirm-server-edits` decides, then `apply-all` runs. The -`maybe-*` decisions skip the prompt only when the batch is `peaceful`: +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 @@ -329,279 +500,309 @@ is explicit: `prepare` builds a list of closures touching nothing, then always (find-buffer-visiting (cadddr op))))) ``` -`all-text-edits` is a conjunction over the whole batch, so **a batch -containing any create/rename/delete is never `peaceful` and always -prompts** under the default. Confirmation is all-or-nothing and -strictly precedes mutation; there is no mid-batch interrupt. +`all-text-edits` is a conjunction over the whole batch, so a batch +containing any create/rename/delete **always** prompts under the +default. -**The transferable lessons** (now claims about what pmacs should do): -the consent gate belongs *before* the irreversible step; the decision is -made over the *whole* batch during a preparation phase; and -`ignoreIfNotExists` must short-circuit the buffer half too. - -### 1.10 `apply_resource_op` has no direct test coverage +### 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 in `src/lua_bindings/mod.rs`. Zero tests name it. +comment in `src/rename.rs` and three inside the binding's own +definition. Zero tests name it. -It is exercised indirectly by exactly one acceptance, +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`), which emits an ordered -create / edit / rename / delete. **Its deleted file `c.rs` is never -opened in a buffer**, so the entire buffer-reconciliation half of the -delete arm is untested. That suite and that fake are where this lane's -acceptances belong. +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. ## 2. The decision space -### 2.1 Recommended — refuse before touching disk, at both layers +### 2.1 Recommended — validate before mutating, at the primitive; conflict-check early, in the applier -**Refuse the delete when it would destroy unsaved work, and refuse it -before the filesystem call. Do this at the Rust primitive (the -invariant) *and* in the applier's existing preflight (batch -atomicity).** +**The primitive refuses before touching disk. The applier catches what +it can early, and reports honestly what it cannot.** -Concretely: +**Layer 1 — the primitive (the invariant).** The delete arm becomes four +ordered phases: -- **Layer 1, the primitive.** The delete arm reconciles the buffer - registry *first*. If the path resolves to a modified buffer, it - returns an error and touches nothing. Inverting the order is what - makes the guard possible at all (§1.3), and it also closes a smaller - hole: today a `ConcurrentEdit` refusal from `BufferRegistry::remove` - arrives after the file is already gone. -- **Layer 2, the applier preflight.** `apply_workspace_edit`'s plan loop - (`lsp.lua:1302-1336`) gains a second precondition alongside URI - resolution, and returns its existing `nil, message` when any delete op - targets a modified buffer — so **nothing** in the batch is mutated. +``` +stat / no-op decision → enumerate and validate affected buffers + → mutate the filesystem + → reconcile the registry +``` -Both layers are needed, and neither is redundant: +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 1 alone leaves partial batches (§1.6): op 1 deletes, op 2 - refuses, and the user is left mid-edit with no way back. -- Layer 2 alone leaves the primitive armed. `pmacs.buffer.apply_resource_op` - is public Lua API; dired Stage 2 and any package can call it directly. - A guard that lives only in one caller is exactly the shape the project - has been burned by — "pin the guard through the real path". +**Layer 2 — the applier (early conflict check + robust reporting).** +`apply_workspace_edit`'s existing plan loop gains a modified-buffer +conflict check for delete ops and returns its existing `nil, message`. +This is a **filter, not a transaction** (§1.7): it catches the common +case 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** answers the server. -**Why this beats the runners-up, in one sentence each:** +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. -- It is the only option that is *possible* — prompting is architecturally - unavailable (§1.8) — and it is the option Emacs's structure already - endorses (§1.9): consent gate before the irreversible step, decision - taken over the whole batch during preparation. +**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 +### 2.2 Prompt the user — rejected on scope, not on possibility -Rejected on three independent grounds, any one sufficient. +**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: -1. **Architecturally unavailable.** `apply_resource_op` is synchronous - Rust; prompts are callback-continuations resumed by a later keystroke - (§1.8). Making this work means restructuring the applier into a - callback chain carrying the remaining plan as a closure upvalue, with - revalidation of every precondition at each resumption — a large, - independently-risky change to LSP edit application, for a guard. -2. **It costs an interaction island.** The alternative to a callback - chain is a Rust modal shadow (the query-replace shape). That is a - **seventh** dispatcher shadow. COHERENCE §6 grades this area "weak, - and growing by one island per modal feature" and records that - terminal copy mode was deliberately engineered *not* to become the - seventh. Spending that budget on an error path would be a poor trade. -3. **It cannot serve the most dangerous caller.** The server-initiated - path (§1.5, row 1) must answer `workspace/applyEdit` synchronously - with `applied: true | false`. There is no user turn available inside - it, and a prompt that resolves three keystrokes later cannot produce - that answer. +- **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. -Note that "what happens to the rest of the batch?" — the question that -makes prompting genuinely hard — is dissolved by the recommendation -rather than answered: the decision is taken in the preflight, before any -op runs, so there is no rest-of-batch to strand. +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. It is *more* destructive than refusing, not less: it overwrites the -user's on-disk original — the copy they might have wanted — immediately -before removing the file. It also cannot be relied on: `save_inner` -refuses when the file changed on disk since it was read -(`src/editor_core.rs:1908`, guard at `:1917`), so the fallback question is unanswered and -we are back to refusing. +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, not taste. §1.8 establishes that the existing -autosave machinery defeats this four ways, the decisive one being that -**removing the buffer deletes the recovery file** — the backup is -destroyed by the very operation it exists to survive. Building a -parallel side-store outside `autosave/` means inventing a second -recovery surface with its own discovery, GC and lifecycle, to make a -destructive operation *feel* safe. Refusing is cheaper and honest. +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 -Superficially attractive because Eglot's default keys on exactly this -(`eglot-confirm-server-edits`). But: `apply_resource_op` takes no -provenance argument and there is no ambient caller identity to read. -Adding one makes the primitive's safety depend on a caller-supplied -flag — any caller that omits it, or passes the permissive value, is -unguarded, which is the failure mode the whole lane exists to remove. -COHERENCE §10 (extension trust classes) is unbuilt, so there is no -existing trust dimension to key on either. **The refusal is -unconditional and provenance-blind.** A future -`lsp.confirm-server-edits` setting can *loosen* it once a prompt -mechanism exists; §6. +`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 +### Q#RD1 — Refuse. Do not prompt, do not save, do not back up — **SETTLED YES** -An `apply_resource_op` delete whose target resolves to a modified buffer -**fails**, changing nothing on disk and nothing in the registry. -Rationale: §2.2–§2.5. 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. +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 — Reconcile the registry before touching the filesystem +### Q#RD2 — Validate before mutating; reconcile last — **REWRITTEN at rev 2** -The delete arm's order inverts: resolve the buffer set, decide, and only -then call `remove_file` / `remove_dir` / `remove_dir_all`. This is what -makes the guard expressible (§1.3) and it also moves the pre-existing -`ConcurrentEdit` refusal ahead of the irreversible step. +Four phases, in order: **stat/no-op decision → enumerate and validate +affected buffers → mutate the filesystem → reconcile the registry.** -**What inverting breaks, considered:** the current order means a -successful `remove_buffer_and_fire` implies the file is already gone, -so `on_removed` subscribers observe a consistent "file and buffer both -gone" world. After inversion, `on_removed` fires with the file still -present for the remainder of the call. Two mitigations: the fs call -follows immediately with no yield point in between (Lua callbacks run -synchronously inside `after_buffer_removed`), and if the fs call then -fails the correct end state is *ambiguous either way* — today it cannot -happen because the buffer removal is unreachable on fs failure. Q#RD9 -records the resolution. +- **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 batch aborts in the preflight, before any op runs +### Q#RD3 — The preflight is an early conflict check, **not** a transaction — **DOWNGRADED at rev 2** -`apply_workspace_edit`'s plan loop gains a modified-buffer precondition -for delete ops and returns `nil, message`. This reuses the applier's -existing, documented abort contract — "aborts the whole edit cleanly, -origin buffer untouched, rather than half-applying" -(`lsp.lua:1287-1291`) — and all three callers already handle the -`nil, message` shape (§1.5). It is Eglot's prepare/decide/apply shape -(§1.9) implemented in the phase pmacs already has. +`apply_workspace_edit`'s plan loop gains a modified-buffer conflict +check for delete ops and returns its existing `nil, message`. It is +described in the code comment and here as a **filter**: -The preflight needs a path-keyed modified query, which Lua lacks today -(§1.4). The minimal addition is a `pmacs.buffer` surface answering -"is there a modified buffer at or beneath this path"; its exact shape is -an implementation choice, but it must be **one** query so the preflight -and the primitive cannot drift apart. +- **What it guarantees:** when the conflict is visible at plan time, + nothing in the batch is mutated at all, and the user gets 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 — + which is `FailureHandlingKind.Abort`, the strategy the spec itself + assigns to any edit containing resource changes. +- Revision 1 called this "whole-batch atomicity" and said "nothing in + the batch is mutated". **That was false and is withdrawn.** -### Q#RD4 — `ignore_if_not_exists` short-circuits the buffer half too +The check needs a path-keyed modified query that Lua lacks (§1.4). It +must be **one** query shared with the primitive's validation phase, 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. -When the path is absent and `ignore_if_not_exists` is set, the arm -returns early **without** touching the registry — matching the `create` -arm's existing `return Ok(())` idiom and Eglot's `exists` guard (§1.9). 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 prefix-aware, or the guard has a documented bypass +### Q#RD5 — Recursive deletes are **inspected** tree-wide but **reconciled** exact-path — **SETTLED YES, NARROWED at rev 2** -Mode (c) proves that `recursive = true` reconciles nothing, so an -exact-path guard is bypassed entirely by the most destructive arm: a -server that sends `{kind: "delete", uri: , recursive: true}` walks -straight past it. A guard with a trivial, reachable bypass is not a -guard, so the check must cover every buffer whose path lies **beneath** -the deleted directory, not merely one whose path equals it. +Mode (c) proves `recursive = true` reconciles nothing, so an exact-path +guard is bypassed by the most destructive arm. Therefore: -**This overlaps dired Stage 2 and the boundary is drawn explicitly.** -`docs/dired-framing.md:807-819` and `docs/active-work.md:605` claim -prefix-aware, normalize-before-lookup rebinding for the **rename** side. -This lane takes the **delete** side only, because without it this lane -ships nothing. The two want the same helper; whichever lands second -adopts the first's. If the user prefers, the alternative is to sequence -this lane after dired Stage 2 and consume its helper — but the bug is -live and dired Stage 2 is unframed for this, so shipping first is -recommended. +- **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). -### Q#RD6 — Lookups normalize; "modified" means `Buffer::is_modified` +The asymmetry is deliberate and is the point: **inspect widely, mutate +narrowly.** -The guard resolves paths through the normalizing wrapper -(`EditorCore::find_buffer_for_path`, `src/editor_core.rs:935`) rather -than raw `find_by_path`, because a lookup miss is a *silent* guard -bypass (§1.7). "Modified" is `Buffer::is_modified()` — the single -existing predicate (§1.4). No new notion of dirtiness is introduced. +**Boundary with dired.** `docs/dired-framing.md:807-819` and the ledger +claim prefix-aware, normalize-before-lookup rebinding for the **rename** +side. This lane takes the **delete** side only. Taking it now rather +than consuming dired Stage 2's helper is settled, and is supported by +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. Whichever lands second adopts the first's helper. + +### 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). + +"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, exactly as today. -Overreach here would break `m4_15` and, more importantly, would make the -LSP's legitimate deletes fail for users who merely have the file open. +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 — The refusal is reported on every path, and never by raising alone +### Q#RD7 — Failures travel as values, are always answered, and leave a durable trace — **WIDENED at rev 2** -Per §1.5, a raise on the server-initiated path is swallowed by -`pcall(handle_server_requests)` and the server is left unanswered. So: +§1.5 established that **no** caller reliably surfaces a raise. So: -- **Preflight refusal (all three callers)** returns `nil, message`. - Callers 2 and 3 already render that to `set_status`; caller 1 already - turns it into `{ applied = false, failureReason = ... }` and sends the - response. -- **Primitive refusal** still raises — it must, being a Rust binding — - but that is now a defence-in-depth path reached only by direct callers, - because the preflight catches the LSP path first. - -The message names the buffer and says what to do: save it, or use the -buffer-level command to discard. It must not be a bare errno. +- **Every primitive call inside `apply_workspace_edit` is wrapped**, and + every execution failure — refusal or I/O error — is converted to the + existing `nil, message` return. No exception escapes the applier. +- **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 unattended caller always answers.** Path 1 must send + `{ applied = false, failureReason = ... }` in every failure case. + Today the raise unwinds past the send and the server waits forever. +- **The refusal is also recorded in `*errors*`** via the existing + Rust-side `append_to_errors_buffer` idiom (§1.10), so it survives the + status line being cleared on the next keystroke and leaves a trace on + the path the user was never watching. +- The message **names the buffer** and says what to do. Not a bare + errno. ### Q#RD8 — The window/last-buffer defects do **not** land here -Mode (d) is real and is parked (§6). Two reasons. It is a different -failure (a dangling window and an empty registry, not data loss), and it -is shared with `pmacs.buffer.remove` rather than specific to delete. +Mode (d) is real and parked (§6). It is a different failure from data +loss, and it is shared with `pmacs.buffer.remove`. -**And there is a 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; +**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 of the other, so "just call `kill_buffer` -instead" would silently regress four cleanups. Unifying them is its own -lane with its own census. +former. Neither is a superset, so "just call `kill_buffer` instead" +would silently regress four cleanups. Unifying them needs its own census +and its own lane — and per Q#RD5 this lane must not enlarge the surface +that lane will have to fix. -### Q#RD9 — On filesystem failure after the buffer is removed, the buffer wins +### Q#RD9 — **WITHDRAWN at rev 2** -Given Q#RD2's inversion, a delete can now fail *after* the buffer is -gone. The buffer is not restored. Rationale: the buffer removal is only -reached for a clean buffer (Q#RD1), so nothing unsaved is at stake, and -re-inserting a buffer would need a new registry primitive and would -resurrect it with a fresh `BufferId` that no window, keymap or callback -refers to. The op reports the fs error as it does today. This is a -deliberate, narrow widening of the failure surface and is called out so -review can reject it rather than discover it. +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. ## 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; no server needs that delete to succeed silently. - Falsified by a real server whose normal operation deletes files the - user is actively editing. -- **B2 — The preflight is the right layer for batch atomicity.** - Falsified if a `WorkspaceEdit` legitimately depends on a delete whose - precondition can only be evaluated after an earlier op runs (e.g. a - rename that moves the modified buffer out of the delete's path first). - **This is the sharpest risk in the design** and acceptance 8 pins the - behaviour so the failure is loud rather than silent. -- **B3 — Prefix-aware checking does not over-refuse.** Falsified if a + must resolve. Falsified by a real server whose normal operation + deletes files the user is actively editing. +- **B2 — Mid-batch refusal is acceptable because the protocol already + specifies it.** §1.7's `Abort` semantics are the spec's own answer for + resource-op-bearing edits. Falsified if a server is found that + requires transactional application and degrades badly under `Abort`. + 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 — Inverting the order breaks no `on_removed` subscriber.** - Evidence: the fs call follows synchronously with no yield in between. - Falsified by a subscriber that stats the path. +- **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 @@ -610,88 +811,126 @@ 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 three things 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 (today: `Ok(())`, file - gone, buffer gone). **Asserting only that the buffer survived is - vacuous** — that is mode (c)'s behaviour, which this lane must not - ship. The `exists()` assertion is the load-bearing one. + 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**, per the bottom-panel - lesson that a blanket rewrite passes an "everything moved" test. + buffer is open. Assert **both directions**. -3. **`ignore_if_not_exists = true` on an absent path leaves a modified - buffer intact** (Q#RD4). - *Bite:* fails against a fix that guards only the branch where the fs - delete actually ran — reproduce mode (b) exactly: file removed behind - pmacs's back first, then the op. +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. **`recursive = true` on a directory containing a modified buffer's - file refuses, and the whole tree survives** (Q#RD5). - *Bite:* fails against an exact-path-equality guard. Assert the inner - file still exists — not just that the buffer does, which is already +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. + +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. -5. **Whole-batch atomicity: a `documentChanges` whose *second* op is a - blocked delete leaves the *first* op unapplied** (Q#RD3). - *Bite:* fails against a primitive-only fix. §1.6 verified that op 1 - currently stays applied, so this is a real behaviour change and the - assertion must name op 1's effect (e.g. a file that must still exist, - or must not yet have been created). +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. -6. **The refusal reaches the server on the unattended path.** Drive a - server-initiated `workspace/applyEdit` carrying a blocked delete and - assert the server receives a response with `applied = false` and a - non-empty `failureReason`. - *Bite:* fails against a fix that refuses by raising — §1.5 established - that `pcall(handle_server_requests)` (`lsp.lua:1892`) swallows the - raise and the response is never sent. This is the "pin the guard - through the outermost user-reachable seam" obligation; a direct-call - test on `apply_resource_op` does not satisfy it and is rejected as - insufficient for this criterion. +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. -7. **The user-initiated paths report on the status line.** LSP rename - and code action each surface a message naming the buffer. - *Bite:* fails against a fix that returns `nil` without a message, or - whose message is a bare errno. +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. -8. **The B2 risk is pinned:** a batch that renames the modified buffer's - file *and then* deletes the old path is refused by the preflight - rather than silently mis-evaluated. Whichever behaviour review - chooses, it is asserted, so the limitation is a documented decision - rather than an accident. +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. -9. **`m4_15_workspace_edit_resource_ops_apply_in_order` stays green** - unmodified, pinning "no regression to the ordered-resource-op path" - from outside. Its `c.rs` is never opened, so it exercises exactly the - unguarded case that must keep working (Q#RD6). +13. **The refusal reaches the server on the unattended path**, and the + user-initiated paths 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. -10. **Every new test is checked with `scripts/bite`** and none reports +14. **`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). + +15. **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). Real, reachable, shared with `pmacs.buffer.remove`. Needs its - own lane and a census, because the two removal paths clean disjoint - sets and the obvious unification regresses four cleanups. -- **`kill_buffer` and `editor.quit` have the same gap** (§1.4): both - discard unsaved work with no check. `editor.before-quit` exists as a - veto channel with no subscriber. This lane sets the precedent; those - are separate lanes. -- **A `y_or_n` helper**, and with it any confirm-instead-of-refuse - option. Already a named deferral (`docs/dired-framing.md:854`). + Q#RD8). Needs its own lane and a census, because the two removal paths + clean disjoint sets and the obvious unification regresses four + cleanups. Q#RD5 is written to avoid enlarging it. +- **`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 (§2.5). + 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. -- **A general transient-keymap layer** (COHERENCE §6), the prerequisite - that would make §2.2 cheap rather than impossible. ## 7. Gates @@ -702,28 +941,43 @@ Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy 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.10) and -`lsp_dispatch_seams_acceptance`. `dired_acceptance` is a watch item for -Q#RD5's shared lookup change. +Touched suites: **`m4_acceptance`** (the resource-op home, §1.14) and +`lsp_dispatch_seams_acceptance`. `dired_acceptance` and +`autosave_acceptance` are watch items — the former for Q#RD6's shared +lookup, the latter because `on_removed` ordering (acceptance 4) is where +autosave's `discard_buffer` hangs. Gate the pushed tree, not the worktree — commit first, then gate. -This framing-only PR ships no runtime code, so its own gate is -`git diff --check` plus a docs read. +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 -PR. **This framing is the entire first PR.** Implementation does not -begin until the user approves the design — specifically Q#RD1 (refuse -rather than prompt), Q#RD5 (take the delete side of prefix-awareness now -rather than sequencing behind dired Stage 2), and Q#RD9 (the buffer is -not restored on fs failure). +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. -Files this lane will touch when approved: `src/lua_bindings/mod.rs` -(the delete arm and a modified-at-or-beneath query), -`builtin/runtime/lsp.lua` (the preflight), `tests/m4_acceptance.rs` and -`src/bin/pmacs_fake_lsp.rs` (a fake mode carrying a blocked delete). -It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, or -`builtin/runtime/dired.lua`. No protocol change. +**Implementation does not begin until the user approves this revision.** + +Files the implementation will touch: `src/lua_bindings/mod.rs` (the +delete arm's four phases and the shared all-buffers query), +`builtin/runtime/lsp.lua` (the conflict check, per-op wrapping, origin +restore, always-answer), `tests/m4_acceptance.rs` and +`src/bin/pmacs_fake_lsp.rs` (fake modes for the blocked delete, the +edit-then-delete and rename-into-delete batches, and absent-plus-ignore). +It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, +`builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`. +No protocol change. + +**Ownership note.** `docs/active-work.md` records that dired Stage 2a — +"rename/delete reconciliation substrate" — overlaps +`builtin/runtime/lsp.lua` and warns against running it concurrently with +other work touching those files "without assigning those files to one +lane first". This lane claims the **delete** half of that substrate and +`builtin/runtime/lsp.lua`'s applier for its duration; the lane entry in +`docs/active-work.md` records the claim. From aea50978900c2b40c477de60681f022d8369980e Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 19:01:19 -0400 Subject: [PATCH 3/5] docs(framing): revision 3 of the resource-op delete guard Review round 2 confirmed everything central from round 1 as fixed and raised four P1s. All four accepted; both requested sweeps run. Rebased onto `7586905`. P1-1, the ownership boundary was stale. Revision 2 described PR #171 as "OPEN, STALE, 153 commits behind, under re-scout" claiming the rename side only. Re-checked directly: #171 is at revision 7, `fd7ae37`, merge-base `ad41cf1`, zero commits behind, and its revision 6 had assigned rename AND delete to Stage 2a with the opposite policy --- file deleted, modified buffer kept alive and orphaned, that orphaning named as accepted residue. Two lanes, opposite answers, same event. The settled split is now carried verbatim in the framing and the ledger, and #171 revision 7 has adopted it from its side. P1-2, the LSP failure-handling claim was wrong. The spec does not assign `Abort` to resource-op-bearing edits. Recovery is described by the client's advertised `workspace.workspaceEdit.failureHandling`; `Abort` is one of four strategies, `Transactional` covers all operations, `Undo` attempts rollback, and only `TextOnlyTransactional` degrades to abort when resource changes are present. pmacs advertises none, so the spec assigns pmacs no strategy at all. Section 1.7 and B2 now rest on verified repository behaviour --- the reproduced partial batch --- not on borrowed protocol authority. P1-3, Q#RD7 had no implementable seam. Three gaps confirmed by reading: `_parse_workspace_edit` is called one line above `apply_workspace_edit` and is fallible, so the proposed wrap left "always answers" untrue; `append_to_errors_buffer` is private and a Lua preflight rejection never reaches Rust, so the promised logging was not implementable from where it was promised; and acceptance 13 tested the response but not the 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. P1-4, clean duplicate reconciliation is now Q#RD10: validate every match, reconcile today's first exact-path match only. Widening would enlarge the parked lifecycle defect Q#RD5 exists to contain; the surviving clean duplicate is named as residue handed to #171. External-claim sweep. Every non-repo claim is now listed in a new section 1.15 with its evidence. One was a paraphrase standing in for a quote: "in Emacs `kill-buffer` on a modified file-visiting buffer prompts". It is true, but the gate is `INTERACTIVE`, defined as `(NILP (Vexecuting_kbd_macro) && !noninteractive)` --- keyboard present, not `call-interactively` --- so eglot's programmatic kill does prompt in a normal session and does not in batch or during a keyboard macro. The revision 2 sentence was right for a reason it never established and false in two environments it never considered. Cross-lane sweep. Q#RD8 said mode (d) needs its own lane; it has one now --- #171's `reconcile_delete` composes both removal phases. Q#RD6 claims the shared walk query explicitly so the duplicate resolves in one direction. And `pmacs.fs.remove`, guarded by neither lane and verified to have zero production callers, is named as explicitly out of scope with its owner rather than left to read as covered. Still PROPOSED. No runtime code. Implementation begins only after explicit user approval. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 75 +++- docs/resource-op-delete-guard-framing.md | 492 +++++++++++++++++++---- 2 files changed, 468 insertions(+), 99 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 7bac9fc..e0e8692 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -485,13 +485,35 @@ has **no branch and no framing yet**. 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 1 rejected. - Settled: refuse unconditionally; take delete-side prefix-awareness now - rather than waiting on #171. Withdrawn: rev 1's buffer-first ordering. - The design is now `stat/no-op → enumerate and validate → mutate - filesystem → reconcile`, which keeps `on_removed`'s "path already - gone" invariant and makes a failed deletion leave buffers intact - automatically. +- **Approved in principle after review round 1; revision 2 raised four + P1s; revision 3 answers them. 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 → 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 settled cross-lane split with #171 — identical wording in both + lanes, do not paraphrase:** + + > #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. + + #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's real state, re-checked 2026-07-28:** revision 7, head + `fd7ae37`, merge-base `ad41cf1`, **0 commits behind**. It is **not** + the "153 commits behind, under re-scout" lane described further down + this file and in #186's revision 2 — that re-scout has finished. + **This cross-lane fact rotted twice in one arc** because the two lanes + were briefed hours apart; re-read the other lane's head before citing + its state, never a summary of it. - **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 @@ -500,11 +522,17 @@ has **no branch and no framing yet**. 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: two delete - ops, the second raises, the first stayed applied. LSP 3.18 says so - too: resource-op-bearing edits get `FailureHandlingKind.Abort`, - "all operations executed before the failing operation stay - executed". Any framing claiming batch atomicity here is wrong. + - **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 @@ -515,13 +543,22 @@ has **no branch and no framing yet**. `"applyEdit": true` but no `documentChanges`, no `resourceOperations`, no `failureHandling`; `grep -rn failureHandling` returns 0. Parked, not fixed here. -- **Ownership claim, per the dired lane's own warning below:** dired - Stage 2a is "rename/delete reconciliation substrate" and overlaps - `builtin/runtime/lsp.lua`. **This lane claims the delete half of that - substrate and the `apply_workspace_edit` applier for its duration**; - dired Stage 2 keeps the rename half. Whichever lands second adopts the - first's shared lookup helper. Do not run the two concurrently over - `builtin/runtime/lsp.lua` without re-splitting that claim. +- **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`, `src/bin/pmacs_fake_lsp.rs`. **Not** `src/daemon.rs`, diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 352f30b..b92e6c3 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,10 +1,10 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 2.** Status: **PROPOSED — needs explicit user approval before +**Revision 3.** 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` @ `ad41cf1` (re-checked at revision 2: no -drift, `main` is still `ad41cf1`). +rebased at revision 3 onto `githubsucks/main` @ `7586905` (PR #189, +COHERENCE.md only — no bearing on any decision here). 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* @@ -19,6 +19,77 @@ this same PR (§8). ## Revision history +### 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`, 0 commits behind** — not +stale. 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 @@ -333,25 +404,52 @@ 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, and the protocol says so +### 1.7 The batch is sequential; the protocol assigns pmacs no failure strategy -Claims about **the LSP specification** (3.18), not about pmacs: +Claims about **the LSP specification** (3.18), each a direct quote, not +a paraphrase — and note carefully what they do *not* say. -- "If resource operations are present, clients need to execute the - operations in the order in which they are provided." -- `FailureHandlingKind.Abort`: "All operations executed before the - failing operation stay executed." -- `FailureHandlingKind.TextOnlyTransactional`: "If the workspace edit - contains only textual file changes they are executed transactionally. - **If resource changes are part of the change the failure handling - strategy is abort.**" +**Sequential execution** — this is the load-bearing one, and it is +unconditional: -So the protocol itself declines to promise transactionality for exactly -the edits this lane is about. **This is the evidence that revision 1's -"whole-batch atomicity" claim was unsupportable**, and the reason Q#RD3 -now describes an early conflict check instead. Sequential execution is -also why a snapshot preflight is necessarily incomplete: an earlier op -can change the facts a later op's precondition was evaluated against. +> "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 @@ -445,12 +543,40 @@ 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. `docs/dired-framing.md:807-819` -and the dired Stage 1 entry under "Closed since the last snapshot" in -`docs/active-work.md` claim the **rename** side for dired Stage 2. §6 -draws the boundary; the dired lane is recorded as **OPEN, STALE, DO NOT -MERGE AS-IS** and under re-scout, which is why this lane does not wait -on it. +Both arms share the §1.4 lookup defects. + +**Cross-lane state, re-checked directly at revision 3 rather than +inherited.** PR #171 (dired Stage 2) is at **revision 7, `fd7ae37`, +merge-base `ad41cf1`, 0 commits behind `main`**. Revision 2 of this +document described it as "OPEN, STALE, 153 commits behind, under +re-scout" and said it claimed the rename side only; **both halves of +that were out of date**. Its revision 6 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 +is deleted and the modified buffer orphaned, with that orphaning named +as accepted residue. + +**The settled split** (identical wording 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's revision 7 adopts the refusal +> and stops saying LSP intentionally deletes modified files. + +#171 revision 7 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 @@ -471,20 +597,59 @@ Verified against `lisp/progmodes/eglot.el`, `emacs-mirror/emacs` (delete-file path recursive)))) ``` -The buffer is killed **before** the file is deleted, and in Emacs -`kill-buffer` on a modified file-visiting buffer prompts — so the -consent gate precedes the irreversible step. (Eglot ignores -`kill-buffer`'s return value, so declining still deletes the file, but -the buffer and its text survive. Even that failure mode is milder than -pmacs's.) Note also that the `exists` guard means `ignoreIfNotExists` -does **not** fall through to the buffer kill — the asymmetry mode (b) -exposes in pmacs. +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 2 note:* Emacs's ordering is **not** what Q#RD2 adopts. -Emacs can afford buffer-first because `kill-buffer` is itself the -consent gate; pmacs has no such gate, so it validates first and -reconciles last (Q#RD2), which yields the same safety without firing -callbacks against a file that still exists. +**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` @@ -517,6 +682,32 @@ 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 for +revision 4 onward: an external claim carries a direct quote or it is +marked not established. + +| # | 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 @@ -704,13 +895,22 @@ guard is bypassed by the most destructive arm. Therefore: The asymmetry is deliberate and is the point: **inspect widely, mutate narrowly.** -**Boundary with dired.** `docs/dired-framing.md:807-819` and the ledger -claim prefix-aware, normalize-before-lookup rebinding for the **rename** -side. This lane takes the **delete** side only. Taking it now rather -than consuming dired Stage 2's helper is settled, and is supported by -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. Whichever lands second adopts the first's helper. +**Boundary with dired — restated at rev 3.** The settled split (§1.12, +quoted there verbatim and carried identically 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 is at revision 7, integrated to `ad41cf1`. **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** @@ -732,6 +932,15 @@ The shared query therefore: - 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 revision 7 +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 @@ -739,27 +948,58 @@ 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 — Failures travel as values, are always answered, and leave a durable trace — **WIDENED at rev 2** +### Q#RD7 — One reporting seam at the server-request boundary — **REWRITTEN at rev 3** -§1.5 established that **no** caller reliably surfaces a raise. So: +§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**. -- **Every primitive call inside `apply_workspace_edit` is wrapped**, and - every execution failure — refusal or I/O error — is converted to the - existing `nil, message` return. No exception escapes the applier. +**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 "always answers" to applier + execution failures. Reason: the narrow option documents a hole instead + of closing one, and the wrap already exists — it moves up one line. + With it, **"always answers" is true without qualification**. +- **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 answers**: `{ applied = false, + failureReason = ... }` in every failure case, including parse failure. +- **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 unattended caller always answers.** Path 1 must send - `{ applied = false, failureReason = ... }` in every failure case. - Today the raise unwinds past the send and the server waits forever. -- **The refusal is also recorded in `*errors*`** via the existing - Rust-side `append_to_errors_buffer` idiom (§1.10), so it survives the - status line being cleared on the next keystroke and leaves a trace on - the path the user was never watching. - 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 @@ -771,9 +1011,16 @@ clean *disjoint* sets. `kill_buffer` handles the last-buffer refusal, **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. Unifying them needs its own census -and its own lane — and per Q#RD5 this lane must not enlarge the surface -that lane will have to fix. +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** @@ -784,6 +1031,36 @@ 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. ## 4. Bets (falsifiable) @@ -791,11 +1068,18 @@ rather than being renumbered. 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 the protocol already - specifies it.** §1.7's `Abort` semantics are the spec's own answer for - resource-op-bearing edits. Falsified if a server is found that - requires transactional application and degrades badly under `Abort`. - Acceptance 12 pins the observable behaviour either way. +- **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. @@ -892,28 +1176,68 @@ that passes against its pre-image has no bite and is rejected. 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 the - user-initiated paths report on the status line naming the buffer. +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. + 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. **`m4_15_workspace_edit_resource_ops_apply_in_order` stays green +14. **Clean duplicates: every match validated, one match reconciled** + (Q#RD10). Two clean buffers bound to one path; delete succeeds. + Assert exactly one is removed and one remains. + *Bite:* fails in **both** directions — against an implementation + that removes all matches (widening the parked defect) and against + one whose validation only consulted the first match. Pair with + criterion 6, which covers the modified-second case; this one covers + the all-clean case that criterion 6 cannot see. + +15. **A parse failure still answers the server** (Q#RD7). Feed the + `workspace/applyEdit` arm an edit payload that makes + `_parse_workspace_edit` fail, and assert the server still receives + `applied = false` with a `failureReason`. + *Bite:* fails against `ad41cf1` **and** against revision 2's + proposed wrap, which covered `apply_workspace_edit` only and left + the parse one line outside — the concrete reason "always answers" + was untrue as written. + +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). -15. **Every new test is checked with `scripts/bite`** and none reports +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). Needs its own lane and a census, because the two removal paths - clean disjoint sets and the obvious unification regresses four - cleanups. Q#RD5 is written to avoid enlarging it. + 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. @@ -974,10 +1298,18 @@ It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, `builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`. No protocol change. -**Ownership note.** `docs/active-work.md` records that dired Stage 2a — -"rename/delete reconciliation substrate" — overlaps -`builtin/runtime/lsp.lua` and warns against running it concurrently with -other work touching those files "without assigning those files to one -lane first". This lane claims the **delete** half of that substrate and -`builtin/runtime/lsp.lua`'s applier for its duration; the lane entry in -`docs/active-work.md` records the claim. +**Ownership note — restated at rev 3 against #171 revision 7.** The +settled split is quoted in §1.12 and Q#RD5 and is carried identically 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. From 46b3c8db1a07227b6f8d5a9772285be126d78125 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 20:31:36 -0400 Subject: [PATCH 4/5] docs(framing): revision 4 of the resource-op delete guard Review 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. The external-claim audit added in revision 3 certified this document as clean while Q#RD3, the normative decision, still asserted the very LSP claim the audit's own row 4 marked WITHDRAWN. It did so because it checked each claim where it was DEFINED and never at the sites that CONSUMED it, so a withdrawal recorded in one section coexisted with the same claim doing load-bearing work three sections away. That is a defect in the audit procedure rather than a missed line, and since the table is now an artifact readers trust, section 1.15 records both the failure and the corrected procedure: grep the whole document per row and check every hit. P1, lane state. Re-measured: `git rev-list --left-right --count fd7ae37...7586905` returns `13 2`, so #171 is two commits behind, not zero, and those two are exactly PR #189's COHERENCE change. Revision 3's "0 behind" was taken against `ad41cf1` and reported in present tense after main had moved. Both ledger lane entries are corrected in place --- #186's revision and base, and #171's superseded `ab42a79` / 153-behind / merge-base-`c8ec8f3` ground truth --- rather than having a correction layered above stale text, which is what made the ledger self-contradicting. Every count now appears as pasted command output. P1, withdrawn claim surviving. Q#RD3 called partial application `FailureHandlingKind.Abort`, "the strategy the spec itself assigns to any edit containing resource changes", and section 1.11 called `Abort` the default "by omission". The spec establishes no default for a client advertising no strategy. Both sites now say only that verified pmacs behaviour resembles abort-style application, resting on the reproduction in section 1.6. P1, acceptance 15. `WorkspaceEditResponse::from_lsp_value` returns `Self`, 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` --- so no server payload can make the parse fail and the criterion could not fail. Decision, new Q#RD11: keep the wrap, drive the test with an explicit throwing stub, label it defensive. Q#RD7's promise narrows to "always attempts a response while the response channel remains live", since `send_response` is itself under an ignored `pcall`. P1, absent-plus-ignore. `pmacs.fs.stat` dispatches async and `canonicalize` resolves symlinks and returns nil for a dangling one, so it disagrees with the primitive's `symlink_metadata` on exactly 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 and 11b supply the missing opposite direction. P2. Criterion 14's "fails in both directions" was false: with both duplicates clean the setup cannot distinguish first-match validation from full validation. The claim is fixed rather than the setup, because criterion 6 already pins validation breadth. Section 8's touch table and section 7's gate list are reconciled. Sweep for corrections applied at one site while a dependent site kept the old claim: count 4 --- the two `Abort` sites above, and the stale #171 count in both the revision history and section 1.12. Nine other withdrawn or revised claims were checked at every consuming site and found clean. Still PROPOSED. No runtime code. Implementation begins only after explicit user approval. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 105 ++++--- docs/resource-op-delete-guard-framing.md | 354 +++++++++++++++++++---- 2 files changed, 375 insertions(+), 84 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index e0e8692..5333375 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -462,10 +462,19 @@ has **no branch and no framing yet**. ## Resource-op delete guard lane — PR #186 OPEN, PROPOSED, DO NOT MERGE - Portable branch: `githubsucks/resource-op-delete-guard`; worktree - `../pmacs-resource-op-delete`. **PR #186**, base `main`, forked from - `ad41cf1` with **no drift** (`main` is still `ad41cf1`). Currently - framing only — `docs/resource-op-delete-guard-framing.md`, revision 2 - — plus this lane entry. No runtime code yet. + `../pmacs-resource-op-delete`. **PR #186**, base `main`. Currently + framing only — `docs/resource-op-delete-guard-framing.md`, **revision + 4** — plus this lane entry. No runtime code yet. +- **Measured 2026-07-28, `main` @ `7586905`:** + + ``` + $ git rev-list --left-right --count HEAD...githubsucks/main + 3 0 + ``` + + Three commits ahead, **0 behind**. Re-measure before quoting; the + count below for #171 was wrong in three consecutive revisions of this + lane because it was carried forward instead of re-run. - **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 @@ -507,13 +516,20 @@ has **no branch and no framing yet**. 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's real state, re-checked 2026-07-28:** revision 7, head - `fd7ae37`, merge-base `ad41cf1`, **0 commits behind**. It is **not** - the "153 commits behind, under re-scout" lane described further down - this file and in #186's revision 2 — that re-scout has finished. - **This cross-lane fact rotted twice in one arc** because the two lanes - were briefed hours apart; re-read the other lane's head before citing - its state, never a summary of it. +- **#171's real state — measured, not summarised.** Its own lane entry + below carries the numbers; do not duplicate them here, because two + copies is how they diverged. The one fact this lane depends on is the + policy split above, which is independent of #171's 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 @@ -568,24 +584,46 @@ has **no branch and no framing yet**. `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 OPEN, STALE, DO NOT MERGE AS-IS +## dired Stage 2 framing lane — PR #171 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: +*Ground-truth lines below refreshed by the #186 lane on 2026-07-28 +because they had gone stale and were contradicting the #186 entry; the +lane's own narrative and plan remain #171's to write.* + +- Portable branch: `githubsucks/dired-stage2-framing` (head `fd7ae37`); + worktree `../pmacs-dired-stage1`. **PR #171**, base `main`. Framing + only — `docs/dired-stage2-framing.md`, **revision 7**, no runtime code. +- **Measured 2026-07-28, `main` @ `7586905`:** + + ``` + $ git rev-list --left-right --count fd7ae37...7586905 + 13 2 + $ git merge-base fd7ae37 7586905 + ad41cf15c2f3905bd8b6e177af824f846b66b085 + $ git show fd7ae37:docs/dired-stage2-framing.md | wc -l + 2897 + ``` + + Thirteen commits ahead, **2 behind** — and those two are exactly the + COHERENCE change (`0dd0bf2`, `7586905`, PR #189), so no framing + conclusion turns on them. The re-scout that the previous entry + described as pending has **finished**: this is no longer the + "`ab42a79`, 4 ahead, 153 behind, merge base `c8ec8f3`" tree, and it is + no longer stale. +- **The commit history embodies five review rounds plus a cross-lane + reconciliation. That is not the same as approval**, and the document + still says PROPOSED — NOT APPROVED. Do not read the round count as a + green light. +- **Revision 7 reconciles with #186.** It adopts #186's pre-filesystem + refusal rather than its own rev-6 policy (which deleted the file and + kept the modified buffer orphaned), and it adopts #186's shared walk + query. See the #186 lane above for the split, quoted verbatim in both + places. +- **Its dependencies had moved materially underneath it** — the reason + the re-scout was needed. This list is **historical**: revisions 5–7 + answered it, and the merge base is now `ad41cf1`, not `c8ec8f3`. + Retained because the three items name substrate any future revision + still 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 @@ -599,12 +637,11 @@ has **no branch and no framing yet**. - **#179/#181 landed the typed-edit consumer chain**, which is the fan-out a rename transaction has to survive. - 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.) + **That re-scout is done** — revision 5 answered it, and revisions 6 + and 7 followed, all on the existing branch so PR #171 keeps its + history. What remains outstanding is **explicit framing approval + before any implementation**, which the document itself still says it + has never received. - **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 diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index b92e6c3..1b4da37 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,10 +1,15 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 3.** Status: **PROPOSED — needs explicit user approval before +**Revision 4.** Status: **PROPOSED — needs explicit user approval before implementation. DO NOT implement, DO NOT merge.** Lane: `resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, -rebased at revision 3 onto `githubsucks/main` @ `7586905` (PR #189, -COHERENCE.md only — no bearing on any decision here). +based on `githubsucks/main` @ `7586905`. + +Every count in this document was produced by a command run at revision +4, with its output pasted at the point of use. That is a reaction to +this lane's own record: it shipped a stale line count, then a stale +commit count, then a stale ledger citation, in three consecutive +revisions — each by carrying a measurement across a base change. 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* @@ -19,6 +24,84 @@ this same PR (§8). ## Revision history +### 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 @@ -27,8 +110,9 @@ 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`, 0 commits behind** — not -stale. Revision 6 had assigned **both** rename and delete to Stage 2a, +**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 @@ -528,8 +612,13 @@ strategy of a client if applying the workspace edit fails") — Two consequences worth stating plainly. pmacs applies resource operations it never declared support for; and it declares no failure -strategy, so §1.7's `Abort` semantics are the de facto behaviour by -omission rather than by choice. **Neither is fixed by this lane** — +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 @@ -545,9 +634,19 @@ treats the buffer as a cache of the file. Both arms share the §1.4 lookup defects. -**Cross-lane state, re-checked directly at revision 3 rather than -inherited.** PR #171 (dired Stage 2) is at **revision 7, `fd7ae37`, -merge-base `ad41cf1`, 0 commits behind `main`**. Revision 2 of this +**Cross-lane state, re-measured at revision 4.** PR #171 (dired +Stage 2) is at **revision 7, `fd7ae37`, merge-base `ad41cf1`**: + +``` +$ git rev-list --left-right --count fd7ae37...7586905 +13 2 +``` + +Thirteen ahead, **two behind** — and those two are exactly PR #189's +COHERENCE change, so no cross-lane conclusion turns on them. Revision 3 +of this document said "0 commits behind"; that was measured against +`ad41cf1` and reported after `main` had moved to `7586905`, which is the +third instance in this lane of quoting a census as a constant. Revision 2 of this document described it as "OPEN, STALE, 153 commits behind, under re-scout" and said it claimed the rename side only; **both halves of that were out of date**. Its revision 6 assigned rename *and* delete @@ -686,9 +785,23 @@ That suite and that fake are where §5's pins belong. 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 for -revision 4 onward: an external claim carries a direct quote or it is -marked not established. +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 4:** for each row, grep the +whole document for the claim's terms and check every hit, not the +defining section. Revision 4 ran that and found two surviving +consumers (§1.15 is the audit; the fix is in Q#RD3 and §1.11). | # | Claim | Source | Status | |---|---|---|---| @@ -850,9 +963,13 @@ described in the code comment and here as a **filter**: 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 — - which is `FailureHandlingKind.Abort`, the strategy the spec itself - assigns to any edit containing resource changes. + 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.** @@ -969,15 +1086,27 @@ single point. 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 "always answers" to applier - execution failures. Reason: the narrow option documents a hole instead - of closing one, and the wrap already exists — it moves up one line. - With it, **"always answers" is true without qualification**. + 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 answers**: `{ applied = false, - failureReason = ... }` in every failure case, including parse failure. +- **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, @@ -1062,6 +1191,79 @@ safely because it composes both phases. This lane's contract is that no 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 structured Rust-backed seam, not a registry walk alone — **NEW at rev 4** + +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 Rust binding returning a structured verdict**, +evaluated with the *same* `symlink_metadata` call the primitive uses, so +the two layers cannot disagree by construction: + +| Verdict | Meaning | +|---|---| +| `no-op` | path absent **and** `ignore_if_not_exists` set — the op will do nothing; the preflight must not reject it | +| `clear` | the delete may proceed: no matching buffer is modified | +| `conflict` | at least one matching buffer (at the path, or beneath it for a recursive delete) is modified — refuse, with the buffer named | + +**Error contract.** The binding is total over its inputs and does not +raise for ordinary filesystem conditions — absence is a verdict, not an +error. It raises only on argument-type violations, matching the rest of +the `pmacs.buffer` surface. A stat error that is neither success nor +`NotFound` (e.g. `EACCES` on a parent directory) yields `conflict`, not +`clear`: the preflight must never report "safe to delete" on the +strength of a question it could not answer. That asymmetry is +deliberate — the failure direction is toward refusing. + +This binding **is** the single shared query of Q#RD6: the walk is its +buffer half, the `symlink_metadata` call its filesystem half, and the +primitive's validation phase calls the same function so drift is +impossible rather than merely discouraged. + ## 4. Bets (falsifiable) - **B1 — Refusing breaks no legitimate server workflow.** A server @@ -1130,7 +1332,9 @@ that passes against its pre-image has no bite and is rejected. 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. + 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 @@ -1165,6 +1369,28 @@ that passes against its pre-image has no bite and is rejected. 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. + 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, @@ -1189,23 +1415,36 @@ that passes against its pre-image has no bite and is rejected. 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: every match validated, one match reconciled** - (Q#RD10). Two clean buffers bound to one path; delete succeeds. - Assert exactly one is removed and one remains. - *Bite:* fails in **both** directions — against an implementation - that removes all matches (widening the parked defect) and against - one whose validation only consulted the first match. Pair with - criterion 6, which covers the modified-second case; this one covers - the all-clean case that criterion 6 cannot see. +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. **A parse failure still answers the server** (Q#RD7). Feed the - `workspace/applyEdit` arm an edit payload that makes - `_parse_workspace_edit` fail, and assert the server still receives - `applied = false` with a `failureReason`. - *Bite:* fails against `ad41cf1` **and** against revision 2's - proposed wrap, which covered `apply_workspace_edit` only and left - the parse one line outside — the concrete reason "always answers" - was untrue as written. +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 @@ -1265,11 +1504,16 @@ Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy 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 -`lsp_dispatch_seams_acceptance`. `dired_acceptance` and -`autosave_acceptance` are watch items — the former for Q#RD6's shared -lookup, the latter because `on_removed` ordering (acceptance 4) is where -autosave's `discard_buffer` hangs. +Touched suites: **`m4_acceptance`** (the resource-op home, §1.14, and +the home of criteria 1–14 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. @@ -1288,16 +1532,26 @@ approved the implementation commits land on this same branch. **Implementation does not begin until the user approves this revision.** -Files the implementation will touch: `src/lua_bindings/mod.rs` (the -delete arm's four phases and the shared all-buffers query), -`builtin/runtime/lsp.lua` (the conflict check, per-op wrapping, origin -restore, always-answer), `tests/m4_acceptance.rs` and -`src/bin/pmacs_fake_lsp.rs` (fake modes for the blocked delete, the -edit-then-delete and rename-into-delete batches, and absent-plus-ignore). +**Files the implementation will touch** — reconciled at rev 4 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, 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) | + 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 — restated at rev 3 against #171 revision 7.** The settled split is quoted in §1.12 and Q#RD5 and is carried identically by both lanes. Concretely, this lane claims for its duration: From c7ad4f491f31517d0caa36f3ae450f6eddeada7d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 20:55:01 -0400 Subject: [PATCH 5/5] docs(framing): revision 5 of the resource-op delete guard Review round 4 accepted the core design and the two surfaced choices, then found two remaining contract defects and one ledger-ownership defect. Close all three without starting runtime implementation. Narrow the last normative "always answers" consumer to Q#RD7's honest "always attempts while the response channel remains live" contract. Strengthen the audit procedure to search normalized prose and term stems so Markdown emphasis cannot hide a consuming site from a literal phrase sweep. Replace Q#RD12's incomplete no-op/clear/conflict table with a total no-op/clear/refuse verdict exposed through one named internal binding. The shared Rust helper now has an explicit mapping for missing with and without ignore, non-NotFound stat errors, modified buffers, and editing-in-progress buffers. Specify how both the primitive and Lua preflight consume the same result. Add acceptance 11c for known NotFound before earlier batch mutation and 11d for fail-closed stat uncertainty. Stop rewriting #171's lane from #186. Restore the sibling block exactly to main, retain only the stable cross-lane ownership split, and record that #171 owns its volatile state. Sequence #186 before #171 because the urgent data-loss guard is the dependency #171 adopts, not to make a textual conflict cheaper. Correct #186's own ahead count to include this revision commit and add the previously omitted lsp_dispatch_seams_acceptance file to the lane's touch list. Still PROPOSED. No runtime code. Implementation begins only after explicit user approval. --- docs/active-work.md | 109 +++++------ docs/resource-op-delete-guard-framing.md | 238 +++++++++++++++-------- 2 files changed, 204 insertions(+), 143 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 5333375..ba6efba 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -464,17 +464,17 @@ has **no branch and no framing yet**. - 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 - 4** — plus this lane entry. No runtime code yet. + 5** — plus this lane entry. No runtime code yet. - **Measured 2026-07-28, `main` @ `7586905`:** ``` $ git rev-list --left-right --count HEAD...githubsucks/main - 3 0 + 5 0 ``` - Three commits ahead, **0 behind**. Re-measure before quoting; the - count below for #171 was wrong in three consecutive revisions of this - lane because it was carried forward instead of re-run. + 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 @@ -494,21 +494,20 @@ has **no branch and no framing yet**. 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 2 raised four - P1s; revision 3 answers them. 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 → 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 settled cross-lane split with #171 — identical wording in both - lanes, do not paraphrase:** +- **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**. #171's revision 7 adopts the - > refusal and stops saying LSP intentionally deletes modified files. + > modified after dired dispatch**. #186 additionally **owns the shared walk query** (scan every path-bound buffer, normalize once, component-aware `Path::starts_with`) @@ -516,10 +515,12 @@ has **no branch and no framing yet**. 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's real state — measured, not summarised.** Its own lane entry - below carries the numbers; do not duplicate them here, because two - copies is how they diverged. The one fact this lane depends on is the - policy split above, which is independent of #171's commit count. +- **#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 @@ -577,6 +578,7 @@ has **no branch and no framing yet**. 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. @@ -584,46 +586,24 @@ has **no branch and no framing yet**. `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 OPEN, PROPOSED, DO NOT MERGE +## dired Stage 2 framing lane — PR #171 OPEN, STALE, DO NOT MERGE AS-IS -*Ground-truth lines below refreshed by the #186 lane on 2026-07-28 -because they had gone stale and were contradicting the #186 entry; the -lane's own narrative and plan remain #171's to write.* - -- Portable branch: `githubsucks/dired-stage2-framing` (head `fd7ae37`); - worktree `../pmacs-dired-stage1`. **PR #171**, base `main`. Framing - only — `docs/dired-stage2-framing.md`, **revision 7**, no runtime code. -- **Measured 2026-07-28, `main` @ `7586905`:** - - ``` - $ git rev-list --left-right --count fd7ae37...7586905 - 13 2 - $ git merge-base fd7ae37 7586905 - ad41cf15c2f3905bd8b6e177af824f846b66b085 - $ git show fd7ae37:docs/dired-stage2-framing.md | wc -l - 2897 - ``` - - Thirteen commits ahead, **2 behind** — and those two are exactly the - COHERENCE change (`0dd0bf2`, `7586905`, PR #189), so no framing - conclusion turns on them. The re-scout that the previous entry - described as pending has **finished**: this is no longer the - "`ab42a79`, 4 ahead, 153 behind, merge base `c8ec8f3`" tree, and it is - no longer stale. -- **The commit history embodies five review rounds plus a cross-lane - reconciliation. That is not the same as approval**, and the document - still says PROPOSED — NOT APPROVED. Do not read the round count as a - green light. -- **Revision 7 reconciles with #186.** It adopts #186's pre-filesystem - refusal rather than its own rev-6 policy (which deleted the file and - kept the modified buffer orphaned), and it adopts #186's shared walk - query. See the #186 lane above for the split, quoted verbatim in both - places. -- **Its dependencies had moved materially underneath it** — the reason - the re-scout was needed. This list is **historical**: revisions 5–7 - answered it, and the merge base is now `ad41cf1`, not `c8ec8f3`. - Retained because the three items name substrate any future revision - still stands on: +- 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 @@ -637,11 +617,12 @@ lane's own narrative and plan remain #171's to write.* - **#179/#181 landed the typed-edit consumer chain**, which is the fan-out a rename transaction has to survive. - **That re-scout is done** — revision 5 answered it, and revisions 6 - and 7 followed, all on the existing branch so PR #171 keeps its - history. What remains outstanding is **explicit framing approval - before any implementation**, which the document itself still says it - has never received. + 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 diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 1b4da37..8706554 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,15 +1,13 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 4.** Status: **PROPOSED — needs explicit user approval before +**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`. -Every count in this document was produced by a command run at revision -4, with its output pasted at the point of use. That is a reaction to -this lane's own record: it shipped a stale line count, then a stale -commit count, then a stale ledger citation, in three consecutive -revisions — each by carrying a measurement across a base change. +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* @@ -24,6 +22,50 @@ 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, @@ -634,36 +676,27 @@ treats the buffer as a cache of the file. Both arms share the §1.4 lookup defects. -**Cross-lane state, re-measured at revision 4.** PR #171 (dired -Stage 2) is at **revision 7, `fd7ae37`, merge-base `ad41cf1`**: +**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. -``` -$ git rev-list --left-right --count fd7ae37...7586905 -13 2 -``` - -Thirteen ahead, **two behind** — and those two are exactly PR #189's -COHERENCE change, so no cross-lane conclusion turns on them. Revision 3 -of this document said "0 commits behind"; that was measured against -`ad41cf1` and reported after `main` had moved to `7586905`, which is the -third instance in this lane of quoting a census as a constant. Revision 2 of this -document described it as "OPEN, STALE, 153 commits behind, under -re-scout" and said it claimed the rename side only; **both halves of -that were out of date**. Its revision 6 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 -is deleted and the modified buffer orphaned, with that orphaning named -as accepted residue. - -**The settled split** (identical wording carried by both lanes): +**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's revision 7 adopts the refusal -> and stops saying LSP intentionally deletes modified files. +> modified after dired dispatch**. -#171 revision 7 has adopted this from its side: its Q#DR18 takes this +#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 @@ -798,10 +831,13 @@ 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 4:** for each row, grep the -whole document for the claim's terms and check every hit, not the -defining section. Revision 4 ran that and found two surviving -consumers (§1.15 is the audit; the fix is in Q#RD3 and §1.11). +**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 | |---|---|---|---| @@ -846,14 +882,15 @@ filesystem failure leaves every buffer intact automatically, and already gone, preserving today's invariant. **Layer 2 — the applier (early conflict check + robust reporting).** -`apply_workspace_edit`'s existing plan loop gains a modified-buffer -conflict check for delete ops and returns its existing `nil, message`. -This is a **filter, not a transaction** (§1.7): it catches the common -case 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** answers the server. +`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 @@ -952,12 +989,13 @@ affected buffers → mutate the filesystem → reconcile the registry.** ### Q#RD3 — The preflight is an early conflict check, **not** a transaction — **DOWNGRADED at rev 2** -`apply_workspace_edit`'s plan loop gains a modified-buffer conflict -check for delete ops and returns its existing `nil, message`. It is -described in the code comment and here as a **filter**: +`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:** when the conflict is visible at plan time, - nothing in the batch is mutated at all, and the user gets one clear +- **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 @@ -973,9 +1011,9 @@ described in the code comment and here as a **filter**: - 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 path-keyed modified query that Lua lacks (§1.4). It -must be **one** query shared with the primitive's validation phase, so -the two cannot drift apart. +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** @@ -1012,8 +1050,8 @@ guard is bypassed by the most destructive arm. Therefore: The asymmetry is deliberate and is the point: **inspect widely, mutate narrowly.** -**Boundary with dired — restated at rev 3.** The settled split (§1.12, -quoted there verbatim and carried identically by #171) is: +**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 @@ -1023,8 +1061,9 @@ quoted there verbatim and carried identically by #171) is: **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 is at revision 7, integrated to `ad41cf1`. **The conclusion is -unchanged and rests on urgency alone** — this is a live data-loss bug +#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. @@ -1050,8 +1089,8 @@ The shared query therefore: 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 revision 7 -records that this rule's four clauses are character-for-character what +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 @@ -1220,7 +1259,7 @@ 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 structured Rust-backed seam, not a registry walk alone — **NEW at rev 4** +### 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 @@ -1240,28 +1279,51 @@ Nothing in Lua closes that gap today: preflight that disagrees with the primitive on the one input that matters. -**The seam: one synchronous Rust binding returning a structured verdict**, -evaluated with the *same* `symlink_metadata` call the primitive uses, so -the two layers cannot disagree by construction: +**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` | the delete may proceed: no matching buffer is modified | -| `conflict` | at least one matching buffer (at the path, or beneath it for a recursive delete) is modified — refuse, with the buffer named | +| `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 | -**Error contract.** The binding is total over its inputs and does not -raise for ordinary filesystem conditions — absence is a verdict, not an -error. It raises only on argument-type violations, matching the rest of -the `pmacs.buffer` surface. A stat error that is neither success nor -`NotFound` (e.g. `EACCES` on a parent directory) yields `conflict`, not -`clear`: the preflight must never report "safe to delete" on the -strength of a question it could not answer. That asymmetry is -deliberate — the failure direction is toward refusing. +**Total mapping and error contract.** -This binding **is** the single shared query of Q#RD6: the walk is its -buffer half, the `symlink_metadata` call its filesystem half, and the -primitive's validation phase calls the same function so drift is +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) @@ -1391,6 +1453,23 @@ that passes against its pre-image has no bite and is rejected. 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, @@ -1505,7 +1584,8 @@ 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 and 16) and **`lsp_dispatch_seams_acceptance`** +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. @@ -1532,16 +1612,16 @@ 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 4 against +**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, 16 | +| `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) | +| `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`. @@ -1552,8 +1632,8 @@ 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 — restated at rev 3 against #171 revision 7.** The -settled split is quoted in §1.12 and Q#RD5 and is carried identically by +**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`;