# 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: