From 1873be614195137bf52be5a5c659d9f83ac49d9b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 09:47:35 -0400 Subject: [PATCH] feat(lsp): refuse a resource-op delete that would destroy unsaved work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the framing merged as #186. On `main` today, `pmacs.buffer.apply_resource_op`'s delete arm removes a file and then removes any buffer bound to it, with no dirty check at any link in the chain — so a server-driven delete destroys unsaved edits, and the `ignore_if_not_exists` arm destroys them having done no filesystem work at all. Layer 1 — the primitive. The delete arm becomes four ordered phases: stat/no-op decision, enumerate and validate, mutate the filesystem, reconcile the registry. Validation inspects and removes nothing, so a filesystem failure leaves every buffer intact automatically rather than by compensation, and `on_removed` still observes the path already gone because reconciliation stays last. `delete_verdict` is the single shared query. It scans *every* path-bound buffer rather than the first match, because `find_by_path` is first-match-only and `pmacs.buffer.from_file` makes duplicates reachable — a clean first match could otherwise hide a modified second. It normalizes both sides before comparing and uses component-aware `starts_with`, so `/tree` does not match `/tree-sibling`. It stats with `symlink_metadata`, not `canonicalize`, which reports a dangling symlink as absent and would disagree with the primitive on exactly the input `ignore_if_not_exists` turns on. Layer 2 — the applier and the server-request boundary. `apply_workspace_edit` gains a plan-time delete precondition check driven by the same Rust helper, so the two layers cannot drift. It is a filter, not a transaction, and the code says so: `documentChanges` are sequential, so an earlier edit can dirty a buffer a later op deletes. The applier is now total — every failure becomes `nil, message`, and the origin buffer is restored on the failure path as well as the success path. At the boundary, parse *and* apply are wrapped: `_parse_workspace_edit` sits one line above the applier and is fallible, so a parse failure previously escaped, was swallowed by `pcall(handle_server_requests)`, and left the server unanswered — the defect being fixed, one line out of scope. Failures now also append one labelled record to `*errors*`. Scope, stated plainly rather than implied by what is present: * Acceptance criteria 1-10, 14 and 16 land here — 11 tests driving the primitive directly. Criteria 11, 11a-11d, 12, 13 and 15 do NOT: they exercise Layer 2 through a real server pump and need `pmacs_fake_lsp` modes that do not exist yet. Criterion 13 explicitly rejects a direct-call test as insufficient, so the Layer 2 code currently has no production-path pin. That is a real gap and the reason this is not the whole lane. * The framing's §8 branch plan said the implementation would land on #186 itself. #186 merged as framing-only, so it gets its own branch and PR. No decision changes. * Criterion 3's stated bite in the framing is wrong. It claims to fail against buffer-first ordering; it does not, because the deleted path is a directory no buffer is bound to, so the reordering never fires on that input. It does fail against validation that removes rather than inspects. Checked by mutation rather than trusted, and the test comment carries the correction. Bite: criteria 1, 5, 6, 8 and 10 fail against `githubsucks/main` under `scripts/bite`. Criteria 3 and 4 pin phase ordering against designs never committed, so `main` cannot falsify them; both were verified by hand mutation instead. Criteria 2, 7, 9 and 14 assert preserved or deliberately-unchanged behaviour and pass against `main` by design — 2 is criterion 1's opposite direction, 9 pins today's imperfect orphaning so widening cannot happen silently. Gates: fmt; clippy -D warnings; --lib 1863; --lib --features crdt 2048; m4_acceptance 132; lsp_dispatch_seams_acceptance 15; dired_acceptance 25 and autosave_acceptance 29 (the framing's watch items); PMACS_REQUIRE_GPU=1 -p pmacs-gpu 202; git diff --check clean. No protocol change. --- builtin/runtime/lsp.lua | 102 +++++++-- docs/active-work.md | 50 ++++ src/lua_bindings/mod.rs | 272 ++++++++++++++++++++-- tests/m4_acceptance.rs | 494 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 886 insertions(+), 32 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index bf56a4c..edf5d6c 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1295,9 +1295,13 @@ end -- `find_or_open` makes the target active. Resource ops go through -- `pmacs.buffer.apply_resource_op` (filesystem + buffer-registry -- reconciliation). The buffer the user invoked from is restored last --- (best-effort: it may itself have been renamed/deleted). Returns +-- (best-effort: it may itself have been renamed/deleted), on the +-- failure path as well as the success path. Returns -- `edit_count, file_count, resource_op_count` on success, or --- `nil, message` if the preflight rejected the edit. +-- `nil, message` if the preflight rejected the edit OR any op failed +-- while executing. No exception escapes this function (Q#RD7) — the +-- three callers all handle `nil, message` already, and a raise +-- reaching them meant an unattended server request went unanswered. local function apply_workspace_edit(ops) local plan = {} for _, op in ipairs(ops or {}) do @@ -1328,6 +1332,29 @@ local function apply_workspace_edit(ops) elseif op.op == "delete" then local path = pmacs.lsp.path_for_uri(op.uri) if not path then return nil, "cannot resolve " .. tostring(op.uri) end + -- Delete precondition check (Q#RD3). This is a FILTER, not a + -- transaction. It catches, before anything in the batch is + -- mutated: a plan-time modified or mid-edit buffer, a known + -- missing target without `ignore_if_not_exists`, and a stat we + -- could not answer. + -- + -- What it deliberately does not catch: `documentChanges` are + -- sequential, so an earlier text edit can dirty a clean buffer + -- and an earlier rename can move a modified buffer into a later + -- delete's subtree, both after this snapshot. The primitive then + -- refuses mid-batch, leaving earlier ops applied. Reporting that + -- honestly is Q#RD7's job, not this check's to prevent. + -- + -- The verdict comes from the same Rust helper the primitive + -- uses, so the two layers cannot disagree. `no-op` and `clear` + -- both pass: rejecting `no-op` would refuse an op the primitive + -- treats as doing nothing. + local verdict = pmacs.buffer._delete_verdict { + path = path, + recursive = op.recursive, + ignore_if_not_exists = op.ignore_if_not_exists, + } + if verdict.kind == "refuse" then return nil, verdict.message end plan[#plan + 1] = { kind = "delete", path = path, recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists, @@ -1337,19 +1364,36 @@ local function apply_workspace_edit(ops) if #plan == 0 then return 0, 0, 0 end local origin = active_buffer_path() local edit_total, files, res_ops = 0, 0, 0 + -- Return the user to where they invoked from — best-effort, since + -- that path may have just been renamed or deleted. Runs on the + -- FAILURE path too (Q#RD7): previously this ran only after a + -- successful loop, so a mid-batch refusal stranded the user in + -- whatever buffer the last applied op left active. + local function restore_origin() + if origin then pcall(pmacs.buffer.find_or_open, origin) end + end for _, item in ipairs(plan) do + local ok, err if item.kind == "edit" then - pmacs.buffer.find_or_open(item.path) - edit_total = edit_total + apply_text_edits(item.edits) - files = files + 1 + ok, err = pcall(function() + pmacs.buffer.find_or_open(item.path) + edit_total = edit_total + apply_text_edits(item.edits) + files = files + 1 + end) else - pmacs.buffer.apply_resource_op(item) - res_ops = res_ops + 1 + -- Every failure becomes a value. The primitive raises for a + -- refusal or an I/O error; converting here is what lets all + -- three callers keep using the existing `nil, message` shape + -- instead of each growing its own pcall. + ok, err = pcall(pmacs.buffer.apply_resource_op, item) + if ok then res_ops = res_ops + 1 end + end + if not ok then + restore_origin() + return nil, tostring(err) end end - -- Return the user to where they invoked from — best-effort, since - -- that path may have just been renamed or deleted. - if origin then pcall(pmacs.buffer.find_or_open, origin) end + restore_origin() return edit_total, files, res_ops end @@ -1832,14 +1876,44 @@ local function handle_server_requests() local edit = ev.params and ev.params.edit local applied, reason = false, nil if edit then - local parsed = pmacs.lsp._parse_workspace_edit(edit) - local n, info = apply_workspace_edit(parsed.ops) - if n then applied = true else reason = info end + -- Wrap parse AND apply, not apply alone (Q#RD7). + -- `_parse_workspace_edit` sits one line above the applier + -- and is itself fallible, so a parse failure escaped the + -- old wrap entirely, was swallowed by the + -- `pcall(handle_server_requests)` at the bottom of this + -- file, and left the server unanswered — the exact defect + -- being fixed, one line out of scope. The wrap costs + -- nothing and makes the boundary uniform regardless of + -- which call fails. + local ok, a, b = pcall(function() + local parsed = pmacs.lsp._parse_workspace_edit(edit) + return apply_workspace_edit(parsed.ops) + end) + if not ok then + reason = a + elseif a then + applied = true + else + reason = b + end else reason = "missing edit" end local result = { applied = applied } - if not applied then result.failureReason = tostring(reason) end + if not applied then + result.failureReason = tostring(reason) + -- The durable trace (Q#RD7): one call site, one label, + -- written at the layer that actually knows the outcome. A + -- Lua preflight rejection never reaches the Rust + -- primitive, so logging there would miss the common + -- unattended case entirely. + pcall(pmacs.buffer._append_error_record, + "lsp:workspace/applyEdit", tostring(reason)) + end + -- Always ATTEMPT a response while the response channel + -- remains live. `send_response` is itself under an ignored + -- pcall, so whether it lands is the transport's business and + -- is not observable from here. pcall(pmacs.lsp.send_response, sid, ev.request_id, result) elseif ev.kind == "request" and ev.method == "workspace/inlayHint/refresh" then diff --git a/docs/active-work.md b/docs/active-work.md index 2dfbaf0..ff72c92 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,6 +521,56 @@ 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 implementation — PR OPEN, PARTIAL + +- Portable branch: `githubsucks/resource-op-delete-guard-impl`, worktree + `../pmacs-rd-impl`, based on `main` @ `300cbc4`. Implements the + framing merged as #186 (`docs/resource-op-delete-guard-framing.md`, + revision 5). +- **The framing's §8 branch plan is superseded and cannot be followed.** + It says "one PR — #186, which becomes the implementation PR", written + when #186 was still open. #186 merged as framing-only, so the + implementation necessarily gets its own branch and PR. Nothing about + the decisions changes; only the branch plan. +- **Layer 1 (the primitive) is complete and tested.** The delete arm is + four ordered phases; `delete_verdict` is the single shared query, used + by the primitive and exposed to Lua as `pmacs.buffer._delete_verdict` + so the two layers cannot drift. +- **Layer 2 (the applier + server-request boundary) is implemented but + NOT yet covered.** `builtin/runtime/lsp.lua` has the plan-time + preflight, the parse-plus-apply wrap, origin restore on the failure + path, and the `*errors*` trace. Criteria 11-15 exercise those through + a real server pump and need new `pmacs_fake_lsp` modes that do not + exist yet. **Criterion 13 explicitly rejects a direct-call test as + insufficient**, so this is a real gap, not a formality: today the + Layer 2 code has no production-path pin. +- Acceptance status: criteria **1-10, 14 and 16 land here** (11 tests in + `tests/m4_acceptance.rs`, prefixed `rd`). Criteria **11, 11a-11d, 12, + 13, 15 do not** — they are the fake-LSP modes above. +- **Criterion 3's stated bite in the framing is wrong**, found by + checking rather than trusting it. The framing says it fails against + buffer-first ordering; it does not, because the deleted path is a + directory no buffer is bound to, so reconciliation never fires on + that input. It *does* fail against validation that removes rather + than inspects — verified by mutation. The test comment carries the + correction; the framing wants amending on its next revision. +- Bite verification: the five refusal criteria (1, 5, 6, 8, 10) fail + against `githubsucks/main` via `scripts/bite`. Criteria 3 and 4 pin + phase *ordering* against designs never committed, so `main` cannot + falsify them; both were verified by hand mutation instead (4 catches + buffer-first ordering, 3 catches removing-validation). Criteria 2, 7, + 9 and 14 assert preserved or deliberately-unchanged behaviour and are + expected to pass against `main` — that is what they are for. +- Gates green at this tree: fmt; clippy `-D warnings`; `--lib` **1863**; + `--lib --features crdt` **2048**; `m4_acceptance` **132**; + `lsp_dispatch_seams_acceptance` **15**; `dired_acceptance` **25** and + `autosave_acceptance` **29** (framing watch items); required GPU + **202**; `git diff --check`. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-rd-impl + -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. + + ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index b624a00..be948d4 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1589,6 +1589,119 @@ fn remove_buffer_removed_callback(lua: &Lua, handle: &BufferRemoveCallbackHandle callbacks.remove(handle.buffer, handle.callback_id) } +/// Verdict for a pending `delete` resource op (resource-op delete +/// guard, Q#RD12). +/// +/// Computed once and consumed by two callers in different forms — the +/// `apply_resource_op` delete arm and the Lua applier's plan-time +/// preflight — so the two layers cannot disagree about the same path. +enum DeleteVerdict { + /// Path absent **and** `ignore_if_not_exists` set: the op will do + /// nothing, and the preflight must not reject it (Q#RD4). + NoOp, + /// Path present, and every affected buffer is clean and not + /// mid-edit. The delete may proceed. + Clear, + /// The delete must not proceed. `message` always states why; + /// `buffer_name` is set only when a buffer caused the refusal. + Refuse { + message: String, + buffer_name: Option, + }, +} + +/// The single shared query behind both delete layers (Q#RD6, Q#RD12). +/// +/// Scans **every** path-bound buffer rather than the first match. +/// [`BufferRegistry::find_by_path`] is first-match-only, and duplicate +/// path-bound buffers are reachable from public Lua via +/// `pmacs.buffer.from_file`, so a clean first match could otherwise +/// hide a modified second — a silent guard bypass. Paths are +/// normalized on both sides before comparison, so a raw-path lookup +/// cannot miss a stored normalized path, and directory containment +/// uses component-aware [`std::path::Path::starts_with`] so `/tree` +/// does not match `/tree-sibling`. +/// +/// Uses the same `symlink_metadata` call the primitive uses, **not** +/// `canonicalize`: the latter resolves symlinks and reports a dangling +/// one as absent, which is the single input on which the two disagree +/// and exactly the input `ignore_if_not_exists` turns on. +/// +/// `recursive` is deliberately not a parameter. Inspection is +/// prefix-aware whenever the target is a directory, because a +/// non-recursive delete of a non-empty directory fails at the +/// filesystem anyway — so widening inspection there costs nothing and +/// narrowing it would leave the recursive arm's bypass reachable. +fn delete_verdict( + reg: &BufferRegistry, + path: &std::path::Path, + ignore_if_not_exists: bool, +) -> DeleteVerdict { + let is_dir = match std::fs::symlink_metadata(path) { + Ok(md) => md.is_dir(), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return if ignore_if_not_exists { + DeleteVerdict::NoOp + } else { + DeleteVerdict::Refuse { + message: format!("apply_resource_op delete: {e}"), + buffer_name: None, + } + }; + } + // Never report safe on the strength of a question we could not + // answer: an unanswerable stat refuses rather than proceeding. + Err(e) => { + return DeleteVerdict::Refuse { + message: format!("apply_resource_op delete (stat): {e}"), + buffer_name: None, + }; + } + }; + + let target = crate::editor_core::normalize_buffer_path(path.to_path_buf()); + for id in reg.ids() { + let Ok(buf) = reg.get(*id) else { continue }; + let Some(bound) = buf.file_path() else { + continue; + }; + let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf()); + if bound != target && !(is_dir && bound.starts_with(&target)) { + continue; + } + // "Modified" is `Buffer::is_modified()`. No new notion of + // dirtiness, and a *clean* open buffer is deliberately not + // guarded — refusing there would fail legitimate deletes for + // anyone who merely has the file open. + if buf.is_modified() { + return DeleteVerdict::Refuse { + message: format!( + "apply_resource_op delete: refusing to delete {} — \ + buffer {:?} has unsaved changes; save or revert it first", + path.display(), + buf.name() + ), + buffer_name: Some(buf.name().to_string()), + }; + } + // Checked here rather than at removal time: today a + // `ConcurrentEdit` refusal from `BufferRegistry::remove` + // arrives *after* the file is already gone. + if buf.editing_in_progress() { + return DeleteVerdict::Refuse { + message: format!( + "apply_resource_op delete: refusing to delete {} — \ + buffer {:?} is mid-edit; finish the edit first", + path.display(), + buf.name() + ), + buffer_name: Some(buf.name().to_string()), + }; + } + } + DeleteVerdict::Clear +} + fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> { registry .borrow_mut() @@ -3311,31 +3424,67 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { + // Four ordered phases (Q#RD2): stat/no-op + // decision -> enumerate and validate affected + // buffers -> mutate the filesystem -> reconcile + // the registry. + // + // Validation inspects and removes nothing, so a + // filesystem failure leaves every buffer intact + // automatically rather than by compensation, and + // `on_removed` still observes the path already + // gone because reconciliation is last. let path: String = spec.get("path")?; let pb = std::path::PathBuf::from(&path); let recursive: bool = spec.get("recursive").unwrap_or(false); let ignore_if_not_exists: bool = spec.get("ignore_if_not_exists").unwrap_or(false); - match std::fs::symlink_metadata(&pb) { - Ok(md) => { - let r = if md.is_dir() { - if recursive { - std::fs::remove_dir_all(&pb) - } else { - std::fs::remove_dir(&pb) - } - } else { - std::fs::remove_file(&pb) - }; - r.map_err(|e| io_err("delete", e))?; - } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - if !ignore_if_not_exists { - return Err(io_err("delete", e)); + + // Phases 1 and 2, as one shared query. + let md = { + let verdict = delete_verdict(®.borrow(), &pb, ignore_if_not_exists); + match verdict { + // Absent plus ignore is a no-op: return + // without touching the registry, which + // is the `create` arm's existing idiom. + // Falling through here is the bug that + // let mode (b) destroy a modified buffer + // with zero filesystem work. + DeleteVerdict::NoOp => return Ok(()), + DeleteVerdict::Refuse { message, .. } => { + return Err(mlua::Error::external(message)); } + DeleteVerdict::Clear => {} } - Err(e) => return Err(io_err("delete (stat)", e)), - } + // `Clear` implies the path is present; re-stat + // for the dir/file decision. A race between + // the two calls degrades to an ordinary + // filesystem error below, never to data loss. + std::fs::symlink_metadata(&pb) + .map_err(|e| io_err("delete (stat)", e))? + }; + + // Phase 3 — the irreversible step, reached only + // after validation cleared it. + let r = if md.is_dir() { + if recursive { + std::fs::remove_dir_all(&pb) + } else { + std::fs::remove_dir(&pb) + } + } else { + std::fs::remove_file(&pb) + }; + r.map_err(|e| io_err("delete", e))?; + + // Phase 4 — reconcile exactly as before + // (Q#RD10): the single first exact-path match is + // removed and additional clean duplicates are + // left in place. Removing them all would route N + // buffers through `remove_buffer_and_fire`, + // which is phase 2 without phase 1, creating up + // to N dangling windows — the parked lifecycle + // defect this lane must not enlarge. let bid = reg.borrow().find_by_path(&pb); if let Some(id) = bid { remove_buffer_and_fire(lua, ®, id)?; @@ -3352,6 +3501,93 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result mlua::Result { + let path: String = spec.get("path")?; + let ignore_if_not_exists: bool = spec.get("ignore_if_not_exists").unwrap_or(false); + let pb = std::path::PathBuf::from(&path); + let out = lua.create_table()?; + match delete_verdict(®.borrow(), &pb, ignore_if_not_exists) { + DeleteVerdict::NoOp => out.set("kind", "no-op")?, + DeleteVerdict::Clear => out.set("kind", "clear")?, + DeleteVerdict::Refuse { + message, + buffer_name, + } => { + out.set("kind", "refuse")?; + out.set("message", message)?; + if let Some(name) = buffer_name { + out.set("buffer_name", name)?; + } + } + } + Ok(out) + })?, + )?; + } + + { + // Q#RD7 — the durable trace, written at the server-request + // boundary rather than in the primitive. `LuaHost:: + // append_to_errors_buffer` 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. Hence a narrow surface reachable from the layer + // that actually knows the outcome. + let reg = registry.clone(); + buffer.set( + "_append_error_record", + lua.create_function(move |lua, (label, message): (String, String)| { + let line = format!("[{label}] {message}\n"); + let (id, edit) = { + let mut r = reg.borrow_mut(); + let id = match r.find_by_name(crate::lua::ERRORS_BUFFER_NAME) { + Some(id) => id, + None => r.create(crate::lua::ERRORS_BUFFER_NAME), + }; + let Ok(buf) = r.get_mut(id) else { + return Ok(()); + }; + let pos = buf.len(); + let Ok(edit) = buf.apply_edit(EditOp::Insert { + pos, + bytes: line.as_bytes(), + }) else { + return Ok(()); + }; + (id, edit) + }; + // Window TextViews are not attached views, so they miss + // `Buffer::apply_edit`'s broadcast; a window displaying + // `*errors*` would paint a stale line cache without + // this. The CRDT queue matters for the same reason it + // does on the host path: `*errors*` is upgraded at every + // replica attach. + if let Some(core) = lua.app_data_ref::() { + let mut core = core.borrow_mut(); + core.notify_buffer_edit(id, &edit); + core.queue_daemon_origin_crdt_op(id, &edit); + } + Ok(()) + })?, + )?; + } + { let reg = registry.clone(); buffer.set( diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 691d746..9dd3177 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8163,3 +8163,497 @@ fn hover_doc_panel_shows_full_contents_via_binding() { .expect("post-q probe"); assert!(name.ends_with("h.rs"), "q restores the source buffer"); } + +// --------------------------------------------------------------- +// Resource-op delete guard (framing `docs/resource-op-delete-guard- +// framing.md`, revision 5). Criteria 1-10 and 14 drive the primitive +// directly; criteria 11-15 drive the applier and the server-request +// boundary and live below these. +// +// Every criterion names the pre-image it must fail against. A test +// that passes against its pre-image has no bite and is worthless +// here: the whole lane exists because the unguarded arm looks fine +// from the buffer's side. +// --------------------------------------------------------------- + +/// Open `path`, returning the Lua global name the buffer is bound to. +fn rd_open(state: &mut pmacs::editor::EditorState, global: &str, path: &std::path::Path) { + let p = path.display().to_string(); + state + .lua_host + .lua() + .load(format!("{global} = pmacs.buffer.find_or_open('{p}')")) + .exec() + .unwrap_or_else(|e| panic!("open {p}: {e}")); +} + +/// `pcall` a delete resource op, returning `(ok, message)`. +fn rd_delete( + state: &mut pmacs::editor::EditorState, + path: &std::path::Path, + extra: &str, +) -> (bool, String) { + let p = path.display().to_string(); + state + .lua_host + .lua() + .load(format!( + "local ok, err = pcall(pmacs.buffer.apply_resource_op, \ + {{ kind = 'delete', path = '{p}'{extra} }}) \ + return ok, tostring(err)" + )) + .eval() + .expect("delete pcall") +} + +/// Criterion 1 — a delete targeting a modified buffer refuses, and the +/// file survives. +/// +/// Bite: fails against `main` before this lane. Asserting only that +/// the buffer survived would be VACUOUS — that is already mode (c)'s +/// behaviour today. **The `exists()` assertion carries the bite.** +#[test] +fn rd1_delete_refuses_when_a_bound_buffer_is_modified() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("a.rs"); + std::fs::write(&f, b"original\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &f); + state + .lua_host + .lua() + .load("B:insert(0, 'X')") + .exec() + .expect("dirty the buffer"); + + let (ok, err) = rd_delete(&mut state, &f, ""); + assert!(!ok, "delete must refuse; it returned success: {err}"); + assert!( + err.contains("unsaved changes"), + "the message must name the reason, got {err:?}" + ); + assert!(err.contains("a.rs"), "and name the buffer, got {err:?}"); + assert!( + f.exists(), + "THE BITE: the file must still be on disk after a refusal" + ); + + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!(text, "Xoriginal\n", "the unsaved edit must be intact"); +} + +/// Criterion 2 — a delete targeting a *clean* open buffer still +/// succeeds: file removed, buffer removed. +/// +/// Bite: fails against an over-broad guard that refuses whenever any +/// buffer is open. Criterion 1 and this one are the two directions of +/// the same rule and neither is sufficient alone. +#[test] +fn rd2_delete_still_succeeds_for_a_clean_open_buffer() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("clean.rs"); + std::fs::write(&f, b"untouched\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &f); + + let (ok, err) = rd_delete(&mut state, &f, ""); + assert!(ok, "a clean open buffer must not block the delete: {err}"); + assert!(!f.exists(), "the file must be gone"); + + let still: bool = state + .lua_host + .lua() + .load("return B:is_valid()") + .eval() + .expect("validity probe"); + assert!(!still, "the buffer must have been reconciled away"); +} + +/// Criterion 3 — a filesystem failure preserves the clean buffer. +/// +/// Bite — **corrected against the framing, which states this wrongly.** +/// The framing says this criterion "fails against revision 1's +/// buffer-first ordering". It does not, and the claim was checked +/// rather than trusted: moving reconciliation ahead of the filesystem +/// mutation leaves this test PASSING, because the deleted path here is +/// a *directory* and no buffer is bound to it — `find_by_path` matches +/// nothing, so the reordering never fires on this input. +/// +/// What does falsify it is the shape revision 1 actually proposed: +/// validation that **removes** the affected set instead of inspecting +/// it. Verified by mutation — with validation removing every buffer at +/// or beneath the target, this test fails on exactly its stated +/// assertion. That is the pre-image; the framing's wording is the one +/// that needs amending, not this setup. +#[test] +fn rd3_filesystem_failure_leaves_the_clean_buffer_intact() { + let dir = tempfile::tempdir().expect("tempdir"); + let sub = dir.path().join("subdir"); + std::fs::create_dir(&sub).expect("mkdir"); + let inner = sub.join("inner.rs"); + std::fs::write(&inner, b"inner\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &inner); + + // Non-empty directory without `recursive` — the fs mutation fails + // after validation has already cleared the (clean) buffer. + let (ok, err) = rd_delete(&mut state, &sub, ""); + assert!(!ok, "removing a non-empty dir without recursive must fail"); + assert!( + err.to_lowercase().contains("delete"), + "the failure should be the delete's own, got {err:?}" + ); + + let still: bool = state + .lua_host + .lua() + .load("return B:is_valid()") + .eval() + .expect("validity probe"); + assert!( + still, + "THE BITE: nothing is removed before the filesystem mutation succeeds" + ); + assert!(inner.exists(), "and the file is untouched"); +} + +/// Criterion 4 — `on_removed` observes the path already absent. +/// +/// Bite: fails against buffer-first ordering, under which the callback +/// would see the file still present. This is the pin that stops the +/// phase order from silently regressing, so it asserts what the +/// callback *saw*, not merely that it ran. +#[test] +fn rd4_on_removed_observes_the_path_already_gone() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("watched.rs"); + std::fs::write(&f, b"bye\n").expect("write"); + let p = f.display().to_string(); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &f); + state + .lua_host + .lua() + .load(format!( + "SAW = 'callback never ran' \ + pmacs.buffer.on_removed(B, function() \ + SAW = pmacs.fs.exists and 'unexpected' or nil \ + local fh = io and io.open and io.open('{p}', 'r') \ + if fh then fh:close(); SAW = 'present' else SAW = 'absent' end \ + end)" + )) + .exec() + .expect("register on_removed"); + + let (ok, err) = rd_delete(&mut state, &f, ""); + assert!(ok, "clean delete should succeed: {err}"); + + let saw: String = state + .lua_host + .lua() + .load("return tostring(SAW)") + .eval() + .expect("read observation"); + assert_eq!( + saw, "absent", + "THE BITE: reconciliation is the last phase, so the callback \ + must observe the path already gone" + ); +} + +/// Criterion 5 — a delete invoked from inside the target buffer's own +/// edit intercept refuses *before* touching disk. +/// +/// Bite: fails against `main`, where `ConcurrentEdit` is discovered +/// only at `BufferRegistry::remove` — i.e. after `remove_file` has +/// already run. The `exists()` assertion is what separates the two. +#[test] +fn rd5_delete_from_inside_the_targets_own_intercept_refuses_before_disk() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("busy.rs"); + std::fs::write(&f, b"busy\n").expect("write"); + let p = f.display().to_string(); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &f); + state + .lua_host + .lua() + .load(format!( + "DELETE_OK, DELETE_ERR = nil, nil \ + pmacs.buffer.add_intercept(B, function(ev) \ + if not DELETE_OK then \ + DELETE_OK, DELETE_ERR = pcall(pmacs.buffer.apply_resource_op, \ + {{ kind = 'delete', path = '{p}' }}) \ + end \ + return ev \ + end)" + )) + .exec() + .expect("register intercept"); + + state + .lua_host + .lua() + .load("pcall(function() B:insert(0, 'z') end)") + .exec() + .expect("drive an edit through the intercept"); + + let (ran, err): (bool, String) = state + .lua_host + .lua() + .load("return DELETE_OK ~= nil, tostring(DELETE_ERR)") + .eval() + .expect("intercept observation"); + assert!(ran, "the intercept must have attempted the delete"); + assert!( + err.contains("mid-edit"), + "the refusal must name the mid-edit reason, got {err:?}" + ); + assert!( + f.exists(), + "THE BITE: the file must survive a mid-edit refusal" + ); +} + +/// Criterion 6 — duplicate path-bound buffers cannot hide a modified +/// copy. **This is the criterion that pins validation breadth**; +/// criterion 14 pins the reconciliation half and cannot see breadth. +/// +/// Bite: fails against any first-match lookup, including +/// `EditorCore::find_buffer_for_path`, which is exactly what revision 1 +/// specified. The first match here is deliberately clean. +#[test] +fn rd6_a_clean_first_match_cannot_hide_a_modified_duplicate() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("dup.rs"); + std::fs::write(&f, b"shared\n").expect("write"); + let p = f.display().to_string(); + + let mut state = pmacs::editor::EditorState::new(); + state + .lua_host + .lua() + .load(format!( + "FIRST = pmacs.buffer.from_file('{p}') \ + SECOND = pmacs.buffer.from_file('{p}') \ + SECOND:insert(0, 'dirty')" + )) + .exec() + .expect("two buffers on one path, second dirtied"); + + let (ok, err) = rd_delete(&mut state, &f, ""); + assert!( + !ok, + "a modified SECOND match must refuse even though the first is clean" + ); + assert!(err.contains("unsaved changes"), "got {err:?}"); + assert!(f.exists(), "THE BITE: the file must survive"); +} + +/// Criterion 7 — component-prefix false positives are rejected. 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. +#[test] +fn rd7_a_sibling_directory_sharing_a_name_prefix_does_not_block() { + let dir = tempfile::tempdir().expect("tempdir"); + let tree = dir.path().join("tree"); + let sibling = dir.path().join("tree-sibling"); + std::fs::create_dir(&tree).expect("mkdir tree"); + std::fs::create_dir(&sibling).expect("mkdir sibling"); + std::fs::write(tree.join("in.rs"), b"in\n").expect("write in"); + let outside = sibling.join("out.rs"); + std::fs::write(&outside, b"out\n").expect("write out"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &outside); + state + .lua_host + .lua() + .load("B:insert(0, 'dirty')") + .exec() + .expect("dirty the sibling's buffer"); + + let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); + assert!( + ok, + "THE BITE: `tree-sibling` is not beneath `tree`; a string-prefix \ + implementation would wrongly refuse here. got {err}" + ); + assert!(!tree.exists(), "the tree must be gone"); + assert!(outside.exists(), "and the sibling untouched"); +} + +/// Criterion 8 — `recursive = true` over a directory containing a +/// modified buffer's file refuses, and the whole tree survives. +/// +/// Bite: fails against exact-path-equality validation. Asserting the +/// *inner file* still exists is what carries it — the buffer surviving +/// is already true today (mode (c)). +#[test] +fn rd8_recursive_delete_refuses_for_a_modified_descendant() { + let dir = tempfile::tempdir().expect("tempdir"); + let tree = dir.path().join("tree"); + let nested = tree.join("nested"); + std::fs::create_dir_all(&nested).expect("mkdir -p"); + let inner = nested.join("deep.rs"); + std::fs::write(&inner, b"deep\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &inner); + state + .lua_host + .lua() + .load("B:insert(0, 'dirty')") + .exec() + .expect("dirty the descendant"); + + let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); + assert!( + !ok, + "a modified descendant must refuse the recursive delete" + ); + assert!(err.contains("unsaved changes"), "got {err:?}"); + assert!( + inner.exists(), + "THE BITE: the whole tree survives, not just the buffer" + ); + assert!(tree.exists(), "including the directory itself"); +} + +/// Criterion 9 — a *clean* recursive delete leaves descendant buffers +/// orphaned, not removed. +/// +/// This pin deliberately asserts today's imperfect behaviour. Widening +/// reconciliation to the tree would route N buffers through +/// `remove_buffer_and_fire` — phase 2 without phase 1 — promoting the +/// parked dangling-window defect from exact-path to tree-wide. +/// +/// Bite: fails against an implementation that widens reconciliation. +#[test] +fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { + let dir = tempfile::tempdir().expect("tempdir"); + let tree = dir.path().join("tree"); + std::fs::create_dir(&tree).expect("mkdir"); + let inner = tree.join("kept.rs"); + std::fs::write(&inner, b"kept\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &inner); + + let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); + assert!(ok, "a clean tree deletes: {err}"); + assert!(!tree.exists(), "the tree is gone"); + + let still: bool = state + .lua_host + .lua() + .load("return B:is_valid()") + .eval() + .expect("validity probe"); + assert!( + still, + "THE BITE: reconciliation stays exact-path, so the descendant \ + buffer is orphaned rather than removed" + ); +} + +/// Criterion 10 — `ignore_if_not_exists = true` on an absent path +/// leaves a modified buffer intact, reproducing mode (b): the file is +/// removed behind pmacs's back first, then the op runs. +/// +/// Bite: fails against `main`, where the `NotFound` + ignore branch +/// does **not** return and falls through to buffer removal — destroying +/// unsaved work with zero filesystem work done. +#[test] +fn rd10_absent_plus_ignore_does_not_destroy_a_modified_buffer() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("vanished.rs"); + std::fs::write(&f, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &f); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + + // The file disappears behind pmacs's back. + std::fs::remove_file(&f).expect("remove behind our back"); + + let (ok, err) = rd_delete(&mut state, &f, ", ignore_if_not_exists = true"); + assert!(ok, "absent + ignore is a no-op, not an error: {err}"); + + let still: bool = state + .lua_host + .lua() + .load("return B:is_valid()") + .eval() + .expect("validity probe"); + assert!( + still, + "THE BITE: the no-op must return before touching the registry" + ); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!(text, "unsavedcontent\n", "the unsaved edit survives"); +} + +/// Criterion 14 — clean duplicates: exactly one match reconciled. +/// +/// Bite: fails against an implementation that removes **all** matches. +/// It pins the reconciliation half of Q#RD10 and *only* that: with both +/// buffers clean there is no verdict difference between consulting one +/// match and consulting all, so this setup cannot see validation +/// breadth. Criterion 6 is what detects incomplete validation. +#[test] +fn rd14_clean_duplicates_reconcile_exactly_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let f = dir.path().join("twin.rs"); + std::fs::write(&f, b"twin\n").expect("write"); + let p = f.display().to_string(); + + let mut state = pmacs::editor::EditorState::new(); + state + .lua_host + .lua() + .load(format!( + "FIRST = pmacs.buffer.from_file('{p}') \ + SECOND = pmacs.buffer.from_file('{p}')" + )) + .exec() + .expect("two clean buffers on one path"); + + let (ok, err) = rd_delete(&mut state, &f, ""); + assert!(ok, "two clean duplicates must not block: {err}"); + + let (first, second): (bool, bool) = state + .lua_host + .lua() + .load("return FIRST:is_valid(), SECOND:is_valid()") + .eval() + .expect("validity probe"); + assert!( + first != second, + "THE BITE: exactly one duplicate is reconciled away, not both \ + and not neither (first={first}, second={second})" + ); +}