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})" + ); +}