From 1873be614195137bf52be5a5c659d9f83ac49d9b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 09:47:35 -0400 Subject: [PATCH 1/6] 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})" + ); +} From 6b21e93e932c29668782db3f63b64cee983ebb1c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:45:43 -0400 Subject: [PATCH 2/6] fix(lsp): close review round 1 on the resource-op delete guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all reproduced by the reviewer, all accepted. Two of them are one defect class — a guard whose scope was REASONED ABOUT rather than enumerated — so both are recorded in the framing's new §9 together with a sweep of every other place this lane decides something is "affected". P1 — the delete preflight broke ordered resource operations (§9.3). Every delete was judged against the filesystem's INITIAL state, at plan-construction time, so a valid `create X -> delete X` was refused with a fabricated NotFound about a path the batch was about to create; likewise `rename A -> B -> delete B`. This was a regression this lane introduced, not a pre-existing defect. Decision: DEFER, do not simulate. A delete whose target is related by component-aware path containment to a path an EARLIER op in the same plan creates, renames onto, renames away from, or removes is not judged at plan time; the primitive judges it when it runs. Q#RD3 already calls this check a filter, not a transaction, so declining to judge is inside its contract and refusing a legal batch is not. Simulating instead would mean modelling filesystem presence AND the registry's path bindings across create/rename/edit — the transaction Q#RD3 declines to build — and a wrong simulation emits false `clear` verdicts, which is the dangerous direction. `edit` ops are excluded from the deferral set on purpose: an edit changes no path's existence, so it can only turn a plan-time `clear` into a primitive-time refusal, which Q#RD3 already documents and accepts. The buffer-and-filesystem half therefore still fires early for any target no prior op touches, which is what criterion 11c pins. P1 — the production-boundary acceptances are landed (§9.5). Criteria 11, 11a-11d, 12 (both directions), 13 and 15 now drive a real `pmacs_fake_lsp` child over a real transport. One parameterized mode, `applyeditplan`, replaces the eight the framing named: it reads its whole WorkspaceEdit from a test-written file and publishes the client's response to a sink, so each of the eight fixtures sits next to the assertions that depend on it instead of being mirrored across two files. Fail-closed — an unreadable plan sends no applyEdit and reports itself through the sink, so a broken fixture cannot read as a pass — and the sink is written `.part`-then-rename so a polling reader never sees a partial record. There is no skip-and-return-ok arm anywhere: `fake_lsp_path` resolves through `env!("CARGO_BIN_EXE_...")`, a compile-time constant, so a missing binary is a build failure. P1 — mid-batch failures were misreported as complete aborts (§9.4). `apply_workspace_edit` now returns `nil, message, applied_op_count`, and ONE renderer serves both the user-facing status line and the server's `failureReason`, so the two cannot disagree. All three callers are updated, not only the rename one. P2 — non-recursive deletes inspected descendants (§9.2). `recursive` is now a parameter of the shared query and descendant matching is reserved for recursive deletes. The old doc comment argued at length for the wrong behaviour and is replaced by the counterexample that falsifies it: a modified buffer at `tree/gone.rs` whose file is already gone blocked a non-recursive delete of the now-EMPTY `tree/`, an op that would have succeeded and that removes none of that buffer's contents. This narrows the Q#RD6 query #171 adopts. Criterion 3's stated bite: fixed by fixing the SETUP, not the doc. The first commit's test comment carried a correction saying the framing's wording was wrong. It was wrong only against that setup — and §9.2's narrowing would then have left the setup with no bite at all, since a non-recursive delete no longer inspects a descendant. So the buffer is now bound to the EXACT deleted path: a file is opened, then replaced on disk by a non-empty directory, and `remove_dir` fails with ENOTEMPTY deterministically under any uid. Both of the framing's stated pre-images now bite, so the framing needed no amendment there. The correction is recorded in §9.1 rather than only in a test comment, which is where the review asked for it. WHY THE SHIPPED SUITE PASSED WHILE FINDINGS 1 AND 4 WERE LIVE — two coverage facts for the next lane. Every delete criterion drove the PRIMITIVE directly, so nothing in the suite ever built a multi-op plan and the preflight's plan-time behaviour had no test at all; the only batch test, `m4_15`, happens to delete a path no earlier op touches. And every recursive-delete criterion (7, 8, 9) passes `recursive = true`, while every non-recursive one binds its buffer to the exact target, so no test in the suite ever combined a non-recursive delete with a descendant buffer — the exact cell finding 4 lives in. Sweep, per the review's request. Seven sites decide something is "affected"; the table is in framing §9.7. Three were the defects above. Two are unchanged by design and named so they are not mistaken for oversights: phase-4 reconciliation compares paths RAW via `BufferRegistry::find_by_path`, which Q#RD10 pins as "exactly today's behaviour" and which correcting would widen reconciliation — the one thing Q#RD5 forbids; and `delete_verdict` stats the raw path while comparing normalized ones, a latent inconsistency whose every branch fails safe and which matches the primitive's own `remove_file`. Two are consistent: the `_delete_verdict` binding defaults `recursive` and `ignore_if_not_exists` the same way the primitive does, and the deferral set is enumerated (create: 1 path; rename: 2; delete: 1; edit: excluded, with the argument written down) rather than reasoned about. Nothing else in the lane decides an affected set. Bites. Every row was RUN, with the positive control `scripts/bite` gained in #192 (merged into this lane), and every ref-based row below reports `OK (assertion)` rather than `OK (COMPILE)`. `1873be6` is this lane's own first commit: findings 1, 3 and 4 were introduced by it, so `main` cannot falsify their pins. rd11a builtin/runtime/lsp.lua @ main OK (assertion) rd11b builtin/runtime/lsp.lua @ main OK (assertion) rd11c builtin/runtime/lsp.lua @ main OK (assertion) rd11d builtin/runtime/lsp.lua @ main OK (assertion) rd12a builtin/runtime/lsp.lua @ main OK (assertion) rd12b builtin/runtime/lsp.lua @ main OK (assertion) rd13 builtin/runtime/lsp.lua @ main OK (assertion) rd15 builtin/runtime/lsp.lua @ main OK (assertion) rd18 src/lua_bindings/mod.rs @ 1873be6 OK (assertion) rd19a builtin/runtime/lsp.lua @ 1873be6 OK (assertion) rd19b builtin/runtime/lsp.lua @ 1873be6 OK (assertion) rd19c builtin/runtime/lsp.lua @ 1873be6 OK (assertion) rd20 builtin/runtime/lsp.lua @ 1873be6 OK (assertion) Two rows need their weakness stated rather than hidden. rd11 is VACUOUS against `main`'s `lsp.lua` and the script says so — a preflight-less applier passes it, which is expected, because rd11 is the direction that asserts the guard does NOT over-refuse (the same shape as criteria 2, 7, 9 and 14). It bites two other ways, both run: `OK (assertion)` against `main`'s `src/lua_bindings/mod.rs`, where the primitive's absent-plus-ignore branch destroys the buffer; and against a hand mutation dropping `ignore_if_not_exists` from the preflight call, which is the pre-image the framing actually names for it. rd3's two pre-images are designs never committed, so no ref carries them and `scripts/bite` cannot be used. Hand-mutated instead: reconciliation moved ahead of the filesystem mutation makes rd3 fail on exactly its stated assertion (and rd4 with it). On this setup that mutation and "validation that removes rather than inspects" are the same mutation, because the buffer is bound to the exact deleted path — stated because the first shipped setup could see neither. The eight rows against `main`'s `lsp.lua` all fail by TIMEOUT rather than by a value assertion, and that is the pre-image behaviour, not a flaky harness: on `main` the primitive's raise escapes the applier, escapes `handle_server_requests`, is swallowed by the `pcall(handle_server_requests)` at the bottom of the file, and the server is never answered at all. The sink is therefore never written. That unanswered request is the defect criterion 13 exists to pin. Gates: fmt; clippy -D warnings; --lib 1863; --lib --features crdt 2048; m4_acceptance 146 (was 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 | 142 ++- docs/active-work.md | 130 ++- docs/resource-op-delete-guard-framing.md | 254 +++++- src/bin/pmacs_fake_lsp.rs | 99 +- src/lua_bindings/mod.rs | 39 +- tests/m4_acceptance.rs | 1062 +++++++++++++++++++++- 6 files changed, 1616 insertions(+), 110 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index edf5d6c..2048a3e 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1298,40 +1298,93 @@ end -- (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 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. +-- `nil, message, applied_op_count` 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. +-- +-- The third failure value is load-bearing, not decoration: Q#RD3 +-- permits partial application, so `applied_op_count > 0` means earlier +-- plan items ARE still applied and no caller may say otherwise. + +-- Strip trailing separators so a path compares by components. +local function strip_trailing_slash(p) + while #p > 1 and p:sub(-1) == "/" do p = p:sub(1, #p - 1) end + return p +end + +-- True when `a` and `b` name the same path, or one lies beneath the +-- other. Component-aware, like the Rust side's `Path::starts_with`: a +-- raw string prefix would make `/tree` an ancestor of `/tree-sibling`. +local function paths_related(a, b) + a, b = strip_trailing_slash(a), strip_trailing_slash(b) + if a == b then return true end + if #a < #b then a, b = b, a end + return a:sub(1, #b) == b and (b == "/" or a:sub(#b + 1, #b + 1) == "/") +end + local function apply_workspace_edit(ops) local plan = {} + -- Paths an EARLIER op in this same batch creates, renames onto, + -- renames away from, or removes. A delete whose target is related to + -- one of them cannot be judged from the filesystem's *initial* + -- state, which is the only state the plan loop can see. + -- + -- Why defer rather than simulate. Q#RD3 already calls this check a + -- FILTER, not a transaction, so declining to judge an op is within + -- its contract; refusing a legal batch is not. Simulating instead + -- would mean modelling filesystem presence AND the buffer registry's + -- path bindings across create/rename/edit — the transaction Q#RD3 + -- declines to build — and a simulation that got it wrong would + -- produce false `clear` verdicts, which is the dangerous direction. + -- Deferring only forgoes the early, cheap report; the primitive's + -- own four-phase guard is untouched and is what actually stands + -- between a server and unsaved work. + -- + -- `edit` ops are deliberately NOT in this set. An edit changes no + -- path's existence; it can only dirty a buffer, i.e. only turn a + -- plan-time `clear` into a primitive-time refusal. That is the + -- under-refusal Q#RD3 documents and accepts, and adding edits here + -- would merely delay a refusal that is already certain. + local batch_changes = {} + local function batch_will_change(path) + for _, other in ipairs(batch_changes) do + if paths_related(path, other) then return true end + end + return false + end for _, op in ipairs(ops or {}) do if op.op == "edit" then if op.edits and #op.edits > 0 then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri) end + if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end plan[#plan + 1] = { kind = "edit", path = path, edits = op.edits } end elseif op.op == "create" then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri) end + if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end plan[#plan + 1] = { kind = "create", path = path, overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists, } + batch_changes[#batch_changes + 1] = path elseif op.op == "rename" then local from = pmacs.lsp.path_for_uri(op.old_uri) local to = pmacs.lsp.path_for_uri(op.new_uri) if not from or not to then return nil, "cannot resolve rename " .. - tostring(op.old_uri) .. " -> " .. tostring(op.new_uri) + tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0 end plan[#plan + 1] = { kind = "rename", old_path = from, new_path = to, overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists, } + batch_changes[#batch_changes + 1] = from + batch_changes[#batch_changes + 1] = to 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 + if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 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 @@ -1349,21 +1402,34 @@ local function apply_workspace_edit(ops) -- 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 + -- + -- Skipped entirely when an earlier op in this batch can change + -- this target (see `batch_changes`). Judging `delete X` against + -- the initial filesystem when an earlier `create X` or + -- `rename A -> X` has not run yet reports a NotFound that the + -- batch itself was about to fix, and refuses a legal edit. + if not batch_will_change(path) then + 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, 0 end + end plan[#plan + 1] = { kind = "delete", path = path, recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists, } + batch_changes[#batch_changes + 1] = path end end if #plan == 0 then return 0, 0, 0 end local origin = active_buffer_path() local edit_total, files, res_ops = 0, 0, 0 + -- Plan items fully applied before a failure. Q#RD3 permits partial + -- application, so this is what stops a caller claiming "nothing was + -- mutated" when something was. + local applied_ops = 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 @@ -1390,13 +1456,33 @@ local function apply_workspace_edit(ops) end if not ok then restore_origin() - return nil, tostring(err) + return nil, tostring(err), applied_ops end + applied_ops = applied_ops + 1 end restore_origin() return edit_total, files, res_ops end +-- Render an `apply_workspace_edit` failure for a human or for a +-- server's `failureReason`. One renderer for both, so the two cannot +-- disagree about what happened. +-- +-- Q#RD3 explicitly permits partial application: an earlier text edit +-- can apply and dirty a buffer before a later delete refuses. So +-- "nothing was mutated" is a claim about `applied`, not a constant — +-- asserting it unconditionally is a false statement about the user's +-- files in precisely the case the framing predicted. +local function workspace_edit_failure(message, applied) + applied = applied or 0 + if applied > 0 then + return string.format( + "failed after %d operation%s — those earlier changes remain applied: %s", + applied, (applied == 1 and "" or "s"), tostring(message)) + end + return "aborted, nothing was mutated: " .. tostring(message) +end + -- Re-pull a per-`(server, uri)` store for every buffer attached to -- `sid`. Fire-and-forget: the response absorbs into its store via the -- request's route, exactly like the explicit command path — no await @@ -1885,16 +1971,21 @@ local function handle_server_requests() -- 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 ok, a, b, c = pcall(function() local parsed = pmacs.lsp._parse_workspace_edit(edit) return apply_workspace_edit(parsed.ops) end) if not ok then - reason = a + -- A raise from the parse: nothing in the batch ran. + reason = workspace_edit_failure(a, 0) elseif a then applied = true else - reason = b + -- `c` is the count of plan items already applied. The + -- server is told so, because `applied = false` alone + -- reads as "the workspace is unchanged" and Q#RD3 says + -- it need not be. + reason = workspace_edit_failure(b, c) end else reason = "missing edit" @@ -2384,8 +2475,13 @@ function pmacs.lsp.rename() end local n, files, res = apply_workspace_edit(ops) if not n then - -- Preflight rejected it; nothing was mutated. - pmacs.editor.set_status("LSP: rename aborted: " .. tostring(files)) + -- On failure the second value is the message and the third + -- is how many plan items already applied. It is NOT always + -- zero (Q#RD3), so this must not say "nothing was mutated" + -- unconditionally — that was false in exactly the + -- edit-then-delete case the framing predicted. + pmacs.editor.set_status( + "LSP: rename " .. workspace_edit_failure(files, res)) return end local msg = string.format( @@ -2446,7 +2542,11 @@ local function apply_code_action(rec, act) if act.has_edit then local n, files, res = apply_workspace_edit(act.edit) if not n then - pmacs.editor.set_status("LSP: code action aborted: " .. tostring(files)) + -- Same failure shape as the rename caller: `files` is the + -- message, `res` the applied-op count (Q#RD3 permits partial + -- application, so it can be non-zero). + pmacs.editor.set_status( + "LSP: code action " .. workspace_edit_failure(files, res)) return end local b = string.format("%d edit(s) / %d file(s)", n, files) diff --git a/docs/active-work.md b/docs/active-work.md index 7e419bc..d9e204d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,55 +521,105 @@ 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 +## Resource-op delete guard implementation — PR #190 OPEN, review round 1 closed - 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). + `../pmacs-rd-impl`. Implements the framing merged as #186 + (`docs/resource-op-delete-guard-framing.md`, revision 5 plus its new + §9). Position against `main`, as pasted command output rather than a + remembered constant — **`main` moved while this lane was being + written**: + + ``` + $ git merge-base HEAD githubsucks/main + 64883ebe0c1785b8188d2dee7c8e6f8ea4518512 + $ git rev-list --left-right --count HEAD...githubsucks/main + 2 0 + ``` + + Re-measure before quoting this anywhere: `main` has branch protection + now, and #193 was open behind #192 when this was written. - **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`. + implementation got its own branch and PR. Nothing about the decisions + changes; only the branch plan. Both the framing's header and its §8 now + say so on their own pages. +- **Layer 1 (the primitive) and Layer 2 (the applier + server-request + boundary) are both complete and both pinned through their production + paths.** The Layer 2 gap the first commit named — criteria 11, 11a-11d, + 12, 13, 15 having no production-path pin — is closed. +- **Review round 1 found four defects; all four are fixed and all four + are recorded in the framing's new §9**, because two of them were + corrections *to that document*, and a correction living only in a test + comment is invisible to the next reader of the framing: + - **P1 §9.3 — the preflight broke ordered resource ops.** Every delete + was judged against the filesystem's *initial* state, so a valid + `create X -> delete X` (or `rename A -> B -> delete B`) was refused + with a fabricated `NotFound` about a path the batch was about to + create. A regression this lane introduced. **Decision: defer, do not + simulate** — a delete whose target is related by component-aware path + containment to a path an *earlier* op creates, renames, or removes is + left to the primitive. Q#RD3 already calls the check a filter, not a + transaction. `edit` ops are deliberately excluded, so the + buffer-and-filesystem half still fires early for untouched targets + (criterion 11c depends on exactly that). + - **P1 §9.5 — the required production-boundary acceptances were + missing.** Landed: 11, 11a-11d, 12 (both directions), 13, 15. + - **P1 §9.4 — mid-batch failures were misreported as complete aborts.** + `apply_workspace_edit` now returns `nil, message, applied_op_count`, + and ONE renderer serves both the status line and the server's + `failureReason`. All three callers updated. + - **P2 §9.2 — non-recursive deletes inspected descendants.** `recursive` + is now a parameter of the shared query. The counterexample is an + orphan: a modified buffer at `tree/gone.rs` whose file is already gone + blocked a non-recursive delete of the now-*empty* `tree/`. +- **`delete_verdict` is narrowed, and #171 inherits the narrowed + version.** Q#RD6's shared query is this lane's to own; descendant + matching is now reserved for recursive deletes. Q#RD5's "inspect widely, + mutate narrowly" is unchanged in substance — "widely" means the set the + op can actually destroy. +- **Criterion 3's stated bite: fixed by fixing the SETUP, not the doc.** + The framing says it fails against buffer-first ordering. Against the + first shipped setup it did not (a directory target with no buffer bound + to it), and §9.2's narrowing would then have left that setup with no + bite at all. The buffer is now bound to the *exact* deleted path — a + file opened, then replaced on disk by a non-empty directory, so a + non-recursive `remove_dir` fails with `ENOTEMPTY` deterministically and + under any uid. Both stated pre-images now bite, so the framing's wording + needed no amendment after all. +- **The fake is one parameterized mode, not eight.** + `PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit` from + `PMACS_FAKE_LSP_EDIT_PLAN` and publishes the client's response to + `PMACS_FAKE_LSP_APPLYEDIT_SINK` (written `.part`-then-rename, so a + polling reader never sees a partial record). Fail-closed: an unreadable + plan sends no `applyEdit` and reports itself through the sink. + `pmacs_fake_lsp` is a cargo BIN resolved through + `env!("CARGO_BIN_EXE_...")`, so every CI leg builds it and a missing + binary is a build failure — there is deliberately no + skip-and-return-ok arm. +- **Criterion 15's stub is hosted in `m4_acceptance`, and the gate list + moved with it.** `lsp_dispatch_seams_acceptance` is struck from the + framing's §7 gate list AND its §8 touch table in the same edit, under + §8's permitted simplification. It is still *run* as a gate, because + `builtin/runtime/lsp.lua` changed. +- Acceptance: criteria 1-16 plus §9's 18, 19a-19c and 20, all in + `tests/m4_acceptance.rs` and prefixed `rd`. 28 tests. +- Bite verification uses `scripts/bite` **with the positive control** it + gained in #192, merged into this lane. The pre-image for the round-1 + fixes is this lane's own first commit `1873be6`, not `main` — those + defects were introduced by it. Per-criterion results are in the commit + message. +- Gates green at the pushed tree: fmt; clippy `-D warnings`; `--lib` + **1863**; `--lib --features crdt` **2048**; `m4_acceptance` **146** + (was 132); `lsp_dispatch_seams_acceptance` **15**; `dired_acceptance` + **25** and `autosave_acceptance` **29** (the framing's watch items); + required GPU **202**; `git diff --check` clean. - 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`. + ## Test-improvement arc, lane 6 — `scripts/bite` positive control - Portable branch: `githubsucks/bite-positive-control`, worktree diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 8706554..66fe675 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,9 +1,19 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 5.** Status: **PROPOSED — needs explicit user approval before -implementation. DO NOT implement, DO NOT merge.** Lane: -`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, -based on `githubsucks/main` @ `7586905`. +**Revision 5, plus §9.** Status: **APPROVED and MERGED as #186 +(framing only); the implementation is PR #190** on branch +`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. The +revision-5 body below is unchanged except for the two bookkeeping +edits §9.6 names and makes in place; **§9 records the corrections +implementation review round 1 found**, including two corrections to +this document. The "DO NOT implement, DO NOT merge" banner this line +replaces was true when revision 5 was written and is not now. + +Revision 5's lane header — `resource-op-delete-guard`, worktree +`../pmacs-resource-op-delete`, based on `githubsucks/main` @ +`7586905` — describes the framing branch, which merged. §8's +one-PR-for-both branch plan is superseded for the same reason and is +annotated there. Revision 5 removes volatile sibling-branch counts from the normative contract. A count is a reading, not a dependency; where history retains @@ -1533,6 +1543,11 @@ that passes against its pre-image has no bite and is rejected. 17. **Every new test is checked with `scripts/bite`** and none reports VACUOUS. +**Criteria 18, 19a–19c and 20 are added by §9**, after implementation +review round 1. They are listed there, with their pre-images, rather +than interleaved here, so this section stays readable as the record of +what revision 5 asked for. + ## 6. Parked — not deferred-and-forgotten @@ -1583,12 +1598,18 @@ Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy suites; `cargo test --test m4_acceptance -- --skip basedpyright`; `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; `git diff --check`. -Touched suites: **`m4_acceptance`** (the resource-op home, §1.14, and -the home of criteria 1–14 including 11a–11d, and 16) and -**`lsp_dispatch_seams_acceptance`** -(criterion 15's throwing parse stub, Q#RD11). Both appear in §8's touch -table; revision 3 named the second here but omitted it there, and the -two lists are now maintained together. +Touched suite: **`m4_acceptance`** — the resource-op home (§1.14) and +the home of every criterion, 1–16 including 11a–11d, plus §9's 18, +19a–19c and 20. + +*Amended at implementation (§9.6).* Revision 5 also named +**`lsp_dispatch_seams_acceptance`**, for criterion 15's throwing parse +stub. §8 permits hosting that stub in `m4_acceptance` instead +**provided the gate list moves with it**, and it does: criterion 15 +drives the same server pump as 11–13, so splitting it across two +suites would have duplicated the whole fixture. The suite is therefore +struck from this list **and** from §8's touch table, in the same edit. +The lists are maintained together, which is what revision 3 got wrong. `dired_acceptance` and `autosave_acceptance` are watch items, not touched files — the former for Q#RD6's shared lookup, the latter because @@ -1612,6 +1633,13 @@ approved the implementation commits land on this same branch. **Implementation does not begin until the user approves this revision.** +**Superseded in fact, not by decision.** #186 merged as framing-only, +so the implementation necessarily got its own branch +(`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`) and its +own PR, #190. No decision in this document changes; only the branch +plan, which described a PR that no longer existed by the time +implementation started. + **Files the implementation will touch** — reconciled at rev 5 against the gate list below and §5, which revision 3 left disagreeing: @@ -1619,9 +1647,18 @@ the gate list below and §5, which revision 3 left disagreeing: |---|---| | `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) | | `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) | -| `tests/m4_acceptance.rs` | criteria 1–14 including 11a–11d, and 16 | -| `tests/lsp_dispatch_seams_acceptance.rs` | criterion 15's throwing parse stub (Q#RD11) — this file was named in the gate list but omitted from revision 3's touch list | -| `src/bin/pmacs_fake_lsp.rs` | fake modes: blocked delete; edit-then-delete; rename-into-delete; absent-plus-ignore; present-plus-ignore (11a); dangling-symlink (11b); absent-without-ignore (11c); unanswerable-stat (11d) | +| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) | +| `src/bin/pmacs_fake_lsp.rs` | **one parameterized mode, `applyeditplan`**, whose `WorkspaceEdit` is read from a test-written file, plus a sink for the client's response — see §9.6 for why one mode replaced the eight named below | + +~~`tests/lsp_dispatch_seams_acceptance.rs`~~ — struck at implementation +(§9.6), together with its entry in §7's gate list. + +The eight fake modes revision 5 named — blocked delete; +edit-then-delete; rename-into-delete; absent-plus-ignore; +present-plus-ignore (11a); dangling-symlink (11b); +absent-without-ignore (11c); unanswerable-stat (11d) — are the eight +*fixtures*, and they all still exist. They are payloads now, not modes +(§9.6). It will **not** touch `src/daemon.rs`, `pmacs-protocol/`, `builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`. @@ -1647,3 +1684,194 @@ reconciliation, the dired async race between dispatch and `remove_blocking`, the rename side of the walk, or `pmacs.fs.remove` (§6). Revision 2's version of this note was written against a stale reading of #171 and is superseded. + + +## 9. Corrections found during implementation — review round 1 + +This section is written **after** revision 5 was approved and +implemented, and it changes no design decision. It records four +corrections review round 1 found in the implementation, the acceptance +criteria they added, and two bookkeeping edits made in place above. +It lives here because two of the four are corrections *to this +document*, and a correction that lives only in a test comment is +invisible to the next reader of the framing. + +### 9.1 Criterion 3's stated bite was wrong — fixed by fixing the setup + +Criterion 3 says it "fails against revision 1's buffer-first +ordering". Against the first shipped setup it did **not**, and the +claim was found by checking rather than trusting it: that setup bound +the clean buffer to a file *beneath* the deleted directory, so no +buffer was bound to the deleted path, `find_by_path` matched nothing, +and reordering reconciliation ahead of the filesystem mutation left +the test passing. + +The first fix considered was to amend the criterion's wording. +**That is not what was done.** §9.2 then narrowed the affected set to +recursive deletes only, which would have left that setup with *no* +bite at all — the pre-image it did catch (validation that removes +rather than inspects) stops touching a descendant buffer on a +non-recursive delete. So the **setup** is what changed: the buffer is +now bound to the **exact** deleted path. A file is opened, the path is +then replaced on disk by a non-empty directory, and a non-recursive +`remove_dir` fails deterministically with `ENOTEMPTY` — no permission +trickery and nothing that behaves differently under a root CI. + +Against that setup the criterion fails against **both** pre-images, +which is what revision 5 claimed all along. **The framing's wording +needed no amendment; the test did.** Both directions verified by +mutation. + +### 9.2 The affected set is scoped by `recursive` (Q#RD12, Q#RD6) + +The shipped `delete_verdict` ignored `recursive` and scanned +descendants for **every** directory target, justified in a doc comment +on the grounds that a non-recursive delete of a non-empty directory +fails at the filesystem anyway, so widening inspection cost nothing. + +**That reasoning is wrong**, and the counterexample is an orphan: a +modified buffer at `tree/gone.rs` whose file is already deleted blocks +a non-recursive delete of the now-**empty** `tree/` — an op that would +have succeeded and that removes none of that buffer's contents. +Reproduced in review. + +`recursive` is therefore a parameter of the shared query, and +descendant matching is reserved for recursive deletes. This +**narrows** the query Q#RD6 hands to #171, which adopts the narrowed +version. Q#RD5's "inspect widely, mutate narrowly" is unchanged in +substance: "widely" means *the set the op can actually destroy*, which +for a non-recursive delete is the target entry alone. + +A symlink to a directory is correctly excluded by the same rule: +`symlink_metadata` reports it as not-a-directory, and the primitive +`remove_file`s the link without walking through it. + +### 9.3 The preflight defers for targets the batch itself changes (Q#RD3) + +The shipped preflight judged **every** delete against the filesystem's +**initial** state, at plan-construction time. A valid `create X → +delete X` batch was therefore rejected because X was absent when the +plan was built, with a fabricated `NotFound` about a path the batch +was about to create; likewise `rename A → B → delete B`. This was a +regression introduced by the implementation, not a pre-existing +defect. + +**Decision — defer, do not simulate.** A delete whose target is +related by path containment to a path an **earlier** op in the same +plan creates, renames onto, renames away from, or removes is not +judged at plan time; the primitive judges it when it runs. Comparison +is component-aware, like the Rust side's `Path::starts_with`. + +Why this is the right half of the choice Q#RD3 already made: + +- Q#RD3 calls this check a **filter, not a transaction**. Declining to + judge an op the snapshot cannot see is inside that contract; + refusing a legal batch is not. +- Simulating instead would mean modelling filesystem presence **and** + the buffer registry's path bindings across create / rename / edit — + the transaction Q#RD3 declines to build — and a simulation that got + it wrong would emit false `clear` verdicts, which is the dangerous + direction. Deferral only forgoes the early, cheap report. +- The primitive's four-phase guard is untouched and is the thing that + actually stands between a server and unsaved work. Criterion 19c + pins that deferring is not skipping. + +**`edit` ops are deliberately not in the deferral set.** An edit +changes no path's existence; it can only dirty a buffer, i.e. only +turn a plan-time `clear` into a primitive-time refusal. That is the +under-refusal Q#RD3 documents and accepts, and adding edits would +merely delay a refusal that is already certain. Criterion 11c depends +on this: its delete target is touched by no earlier op, so the +buffer-and-filesystem half of the check still fires before anything is +mutated. + +### 9.4 Failure reporting must not deny partial application (Q#RD3, Q#RD7) + +`apply_workspace_edit` discarded, on failure, whether earlier ops had +succeeded, and returned a bare `nil, message`. The rename caller then +said "rename aborted" under a comment reading "nothing was mutated" — +false in exactly the case Q#RD3 predicts, where an earlier text edit +applies and dirties the buffer a later delete refuses. + +The applier now returns `nil, message, applied_op_count`, and **one +renderer** serves both the user-facing status line and the server's +`failureReason`, so the two cannot disagree about what happened. All +three callers are updated: the server-request boundary, the rename +caller, and the code-action caller. + +### 9.5 Acceptance added by this round + +18. **A non-recursive delete is not blocked by a buffer beneath its + target** (§9.2). Modified buffer at `tree/gone.rs` whose file is + already gone; non-recursive delete of the now-empty `tree/` + succeeds and the buffer is untouched. + *Bite:* fails against a `delete_verdict` that ignores `recursive`. + +19a. **`create X → delete X` is not refused at plan time** (§9.3). + Assert `applied = true`, and that a later `create` in the same + batch produced its file, so the success is not vacuous. + *Bite:* fails against the initial-state preflight, which reports + `NotFound` for X and rejects the batch before anything runs. + +19b. **`rename A → B → delete B` is not refused at plan time** (§9.3). + *Bite:* as 19a; the source file surviving is what carries it, + because a plan-time rejection leaves the rename unapplied. + +19c. **Deferring the check is not skipping it** (§9.3). + `create X → edit X → delete X` gets past the plan and then refuses + at the primitive, naming the unsaved changes the edit created — + and reports that two operations remain applied. + *Bite:* fails against the initial-state preflight (which reports + `NotFound` instead) **and** against dropping the primitive's guard + for deferred targets (which would report `applied = true`). + +20. **The user-facing message reports partial application** (§9.4). + Driven through `M-x lsp.rename`, because a status line is where a + user reads it and the applier's return value alone would not pin + the caller. + *Bite:* fails against any caller that renders the failure without + consulting the applied-op count — i.e. against the shipped + "rename aborted". + +All of 11, 11a–11d, 12, 13 and 15 also land in this round; they were +specified by revision 5 and were the named gap in the first +implementation commit. + +### 9.6 Bookkeeping edits made in place + +- **§7's gate list and §8's touch table both lose + `lsp_dispatch_seams_acceptance`**, in the same edit, under §8's + permitted simplification. Criterion 15 drives the same server pump + as 11–13, so hosting it anywhere else would duplicate the fixture. +- **§8's fake-mode row is one parameterized mode, not eight.** + `PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit` + from the file named by `PMACS_FAKE_LSP_EDIT_PLAN` and publishes the + client's response to `PMACS_FAKE_LSP_APPLYEDIT_SINK`. The eight + fixtures revision 5 named all exist; they are payloads the test + writes rather than modes the fake hardcodes, which keeps each + payload next to the assertions that depend on it instead of mirrored + across two files. The mode is **fail-closed**: an unreadable or + unparsable plan sends no `applyEdit` and reports itself through the + sink, so a broken fixture cannot read as a pass. The sink is written + to a `.part` and renamed, so a polling reader never sees a partial + record — the wait predicate cannot be weaker than the assertion. + +### 9.7 Sweep — every place the guard decides something is "affected" + +§9.2 and §9.3 are one defect class: **a guard whose scope was reasoned +about rather than enumerated**, over-refusing on inputs the reasoning +never considered. The rest of the guard was swept for that shape. + +| Site | Decision | Verdict | +|---|---|---| +| `delete_verdict` filesystem classification | is the target a directory whose descendants are at risk? | **Fixed** (§9.2). Now `is_dir() && recursive`. A symlink-to-directory is excluded, matching the primitive's `remove_file`. | +| `delete_verdict` buffer matching | which buffers are in the affected set? | **Fixed** (§9.2), exact path always plus descendants only when recursive. | +| Lua plan-time preflight | which deletes can be judged from the initial snapshot? | **Fixed** (§9.3). | +| `delete_verdict` path normalization | the stat uses the **raw** path; the buffer comparison uses the **normalized** one | **Latent inconsistency, fails safe, not fixed here.** A `~`-prefixed argument stats as a literal `~` directory (absent) while matching buffers bound under `$HOME`. Every branch of that disagreement is safe: without `ignore_if_not_exists` it refuses, and with it the primitive returns early having touched nothing. It matches the primitive's own `remove_file`, which also takes the raw path, so the two layers still agree with each other. | +| Phase 4 reconciliation | which buffer is removed after a successful delete? | **Unchanged by design** (Q#RD10). `BufferRegistry::find_by_path` compares paths **raw**, with no normalization, so a buffer stored under a differently-spelled path is not reconciled. This is `main`'s behaviour, Q#RD10 pins "exactly today's", and correcting it would *widen* reconciliation — the one thing Q#RD5 and Q#RD10 forbid. Named so it is not mistaken for an oversight; it belongs to #171. | +| `_delete_verdict` argument handling | `recursive` / `ignore_if_not_exists` defaults | **Consistent.** Both default to `false` on the binding and on the primitive, so an omitted `options` object means the same thing at both layers. | +| Preflight `batch_changes` membership | which prior ops can change a delete's target? | **Enumerated, not reasoned:** create (1 path), rename (both paths), delete (1 path); edit excluded with the argument in §9.3. | + +**Nothing else in this lane decides an affected set.** The reporting +path names a buffer only inside a refusal it already computed, and +`restore_origin` is best-effort by construction. diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 67d6620..19dc0b6 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -49,6 +49,22 @@ //! advertises `signatureHelpProvider` with `(` / `,` triggers, so a //! test can drive the Arc 1d auto-trigger. Every other mode omits the //! capability and therefore never auto-triggers. +//! * If launched with `PMACS_FAKE_LSP_MODE=applyeditplan`: the +//! `WorkspaceEdit` this server hands the client is read verbatim from +//! the JSON file named by `PMACS_FAKE_LSP_EDIT_PLAN`, both for the +//! `executeCommand`-driven server→client `workspace/applyEdit` (id +//! 9100) and for `textDocument/rename`. The client's **response** to +//! 9100 is then written, whole, to the file named by +//! `PMACS_FAKE_LSP_APPLYEDIT_SINK` (written to a sibling `.part` and +//! renamed, so a reader never sees half a record). +//! +//! One parameterized mode rather than one mode per fixture: the eight +//! delete-guard cases differ only in their payload, and a payload the +//! *test* writes sits next to the assertions that depend on it +//! instead of being mirrored across two files. Fail-closed — an +//! unreadable or unparsable plan sends no `applyEdit` at all and +//! reports itself through the sink, so a broken fixture cannot read +//! as a pass. //! * If `PMACS_FAKE_LSP_CHANGE_SINK` names a file (any mode): appends //! one `{"method", "text"}` JSON line per received didOpen / //! didChange, so a test can replay the exact document-sync sequence @@ -118,6 +134,18 @@ fn main() { write_frame(&mut stdout, &echo); continue; } + // `applyeditplan`: the client's reply to the server→client + // `workspace/applyEdit` (id 9100) is the whole observable — + // `applied` plus `failureReason`. Capture it for the test and + // stop, so the generic echo arm below does not answer a + // response as though it were a request. + if mode == "applyeditplan" + && method.is_empty() + && id.as_ref().and_then(serde_json::Value::as_u64) == Some(9100) + { + write_sink(&msg); + continue; + } // T M4.5 async-bridge failure-path test modes: // * `error` — answer every `textDocument/*` request with a // JSON-RPC error object (drives `Handle:await()` -> failed). @@ -730,7 +758,19 @@ fn main() { }, "newText": new_name }]); - let workspace_edit = if mode == "rename" { + let workspace_edit = if mode == "applyeditplan" { + // The user-initiated caller of the same applier. + // Whatever plan the test wrote drives `M-x + // lsp.rename`, so the status-line half of the + // failure contract is reachable from a test. + match edit_plan() { + Ok(plan) => plan, + Err(message) => { + write_sink(&serde_json::json!({ "fakeError": message })); + serde_json::json!({}) + } + } + } else if mode == "rename" { let second = std::env::var("PMACS_FAKE_LSP_RENAME_URI").unwrap_or_default(); serde_json::json!({ "documentChanges": [ @@ -831,6 +871,36 @@ fn main() { // sibling, and deletes another — paths derived // from the request URI's directory so the test // doesn't have to thread them through env. + if mode == "applyeditplan" { + // Fail-closed: with no readable plan there is + // nothing meaningful to apply, so send no + // `applyEdit` at all and report through the + // sink. Sending an empty edit instead would + // make the client answer `applied = false` for + // a fixture reason, which is indistinguishable + // from the refusal these tests are asserting. + match edit_plan() { + Ok(plan) => { + let apply = serde_json::json!({ + "jsonrpc": "2.0", + "id": 9100, + "method": "workspace/applyEdit", + "params": { "label": "fake plan", "edit": plan } + }); + write_frame(&mut stdout, &apply); + } + Err(message) => { + write_sink(&serde_json::json!({ "fakeError": message })); + } + } + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "result": serde_json::Value::Null + }); + write_frame(&mut stdout, &resp); + continue; + } let we = if mode == "resourceops" { let s = target.as_str().unwrap_or(""); let base = match s.rfind('/') { @@ -1187,6 +1257,33 @@ fn write_frame(w: &mut W, body: &serde_json::Value) { let _ = w.flush(); } +/// Read the `applyeditplan` payload — a whole `WorkspaceEdit` object, +/// written by the test. Every failure is a message rather than a panic, +/// so it can reach the test through the sink instead of dying as an +/// unexplained transport EOF. +fn edit_plan() -> Result { + let path = std::env::var("PMACS_FAKE_LSP_EDIT_PLAN") + .map_err(|_| "PMACS_FAKE_LSP_EDIT_PLAN is unset".to_owned())?; + let raw = std::fs::read(&path).map_err(|e| format!("reading {path}: {e}"))?; + serde_json::from_slice(&raw).map_err(|e| format!("parsing {path}: {e}")) +} + +/// Publish one JSON record to `PMACS_FAKE_LSP_APPLYEDIT_SINK`. +/// +/// Written to a sibling `.part` and renamed. A reader polling for the +/// file therefore never observes a partial record, so a test's wait +/// predicate cannot be weaker than its assertion — the `m4_5` config-sink +/// race in reverse. +fn write_sink(value: &serde_json::Value) { + let Ok(path) = std::env::var("PMACS_FAKE_LSP_APPLYEDIT_SINK") else { + return; + }; + let part = format!("{path}.part"); + if std::fs::write(&part, serde_json::to_vec(value).unwrap_or_default()).is_ok() { + let _ = std::fs::rename(&part, &path); + } +} + fn write_garbage() { let mut stdout = io::stdout().lock(); let _ = stdout.write_all(b"NotAValidLspFrame\r\nGarbageHeader\r\n\r\n{}"); diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index be948d4..d5a2b4b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1627,18 +1627,30 @@ enum DeleteVerdict { /// 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. +/// **Descendant matching is reserved for `recursive` deletes.** The +/// affected set is what the op can actually destroy: a non-recursive +/// delete removes the target entry and nothing beneath it, so a buffer +/// under the target is not at risk and must not refuse the op. The +/// earlier revision of this helper ignored `recursive` and scanned +/// descendants for every directory, on the reasoning that a +/// non-recursive delete of a *non-empty* directory fails at the +/// filesystem anyway. That reasoning was wrong, and the counterexample +/// is an orphan: a modified buffer at `tree/gone.rs` whose file is +/// already deleted blocks a non-recursive delete of the now-**empty** +/// `tree/`, which would have succeeded and would have removed none of +/// that buffer's contents. fn delete_verdict( reg: &BufferRegistry, path: &std::path::Path, + recursive: bool, ignore_if_not_exists: bool, ) -> DeleteVerdict { - let is_dir = match std::fs::symlink_metadata(path) { - Ok(md) => md.is_dir(), + let scan_descendants = match std::fs::symlink_metadata(path) { + // A symlink to a directory reports `is_dir() == false` here, and + // that is correct: the primitive `remove_file`s the link and + // never walks through it, so nothing beneath the link's target + // is at risk either. + Ok(md) => md.is_dir() && recursive, Err(e) if e.kind() == std::io::ErrorKind::NotFound => { return if ignore_if_not_exists { DeleteVerdict::NoOp @@ -1666,7 +1678,7 @@ fn delete_verdict( continue; }; let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf()); - if bound != target && !(is_dir && bound.starts_with(&target)) { + if bound != target && !(scan_descendants && bound.starts_with(&target)) { continue; } // "Modified" is `Buffer::is_modified()`. No new notion of @@ -3442,7 +3454,8 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result mlua::Result mlua::Result
{ let path: String = spec.get("path")?; + // `recursive` is read here for the same reason the + // primitive reads it: it decides whether descendants are + // part of the affected set at all. Dropping it would put + // the preflight and the primitive back into disagreement + // on every non-recursive directory delete. + let recursive: bool = spec.get("recursive").unwrap_or(false); 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) { + match delete_verdict(®.borrow(), &pb, recursive, ignore_if_not_exists) { DeleteVerdict::NoOp => out.set("kind", "no-op")?, DeleteVerdict::Clear => out.set("kind", "clear")?, DeleteVerdict::Refuse { diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 9dd3177..2ebba86 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8166,9 +8166,11 @@ fn hover_doc_panel_shows_full_contents_via_binding() { // --------------------------------------------------------------- // 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. +// framing.md`, revision 5 plus its §9 implementation-review +// corrections). Criteria 1-10, 14 and 18 drive the primitive +// directly; criteria 11, 11a-11d, 12, 13, 15, 19 and 20 drive the +// applier and the server-request boundary through a real +// `pmacs_fake_lsp` child, and live in the second block below. // // Every criterion names the pre-image it must fail against. A test // that passes against its pre-image has no bite and is worthless @@ -8278,34 +8280,45 @@ fn rd2_delete_still_succeeds_for_a_clean_open_buffer() { /// 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. +/// **The setup is deliberate and the framing's bite is restored by it.** +/// The first version of this test bound the buffer to a file *beneath* +/// the deleted directory, and against that setup the framing's stated +/// bite ("fails against revision 1's buffer-first ordering") was simply +/// false: no buffer was bound to the deleted path, so reconciliation — +/// wherever it sat in the order — matched nothing and the reordering +/// never fired. Round 1 of review then narrowed the affected set to +/// recursive deletes only, which would have left that setup with no +/// bite at all. /// -/// 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. +/// So the buffer is bound to the **exact** deleted path here: a file is +/// opened, and the path is then replaced on disk by a non-empty +/// directory. The buffer is clean and exact-path-bound, the delete is +/// non-recursive, and `remove_dir` fails deterministically with +/// `ENOTEMPTY` — no permission trickery, nothing that behaves +/// differently under a root CI. +/// +/// Bite: fails against **both** pre-images, as the framing intended. +/// Buffer-first ordering removes B and then fails at the filesystem; +/// validation that *removes* the affected set rather than inspecting it +/// removes B as well. Both verified by mutation. #[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 target = dir.path().join("target"); + std::fs::write(&target, b"was a file\n").expect("write"); let mut state = pmacs::editor::EditorState::new(); - rd_open(&mut state, "B", &inner); + rd_open(&mut state, "B", &target); - // 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, ""); + // The path changes type behind pmacs's back — the same class of + // drift mode (b) exposes, and the buffer keeps its binding. + std::fs::remove_file(&target).expect("unlink"); + std::fs::create_dir(&target).expect("mkdir"); + std::fs::write(target.join("occupant.rs"), b"occupant\n").expect("write occupant"); + + // Non-empty directory without `recursive`: validation clears the + // (clean, exact-path-bound) buffer, and the fs mutation then fails. + let (ok, err) = rd_delete(&mut state, &target, ""); assert!(!ok, "removing a non-empty dir without recursive must fail"); assert!( err.to_lowercase().contains("delete"), @@ -8322,7 +8335,7 @@ fn rd3_filesystem_failure_leaves_the_clean_buffer_intact() { still, "THE BITE: nothing is removed before the filesystem mutation succeeds" ); - assert!(inner.exists(), "and the file is untouched"); + assert!(target.is_dir(), "and the directory is untouched"); } /// Criterion 4 — `on_removed` observes the path already absent. @@ -8657,3 +8670,1002 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { and not neither (first={first}, second={second})" ); } + +// --------------------------------------------------------------- +// Layer 2 — the applier and the server-request boundary, driven +// through the REAL server pump. Criteria 11, 11a-11d, 12, 13 and 15, +// plus 18 and 19, which review round 1 added. +// +// Criterion 13 rejects a direct-call test on `apply_resource_op` as +// insufficient: the guard has to be pinned at the outermost +// user-reachable seam, which is a server-initiated +// `workspace/applyEdit`. These therefore all run a real +// `pmacs_fake_lsp` child over a real transport. +// +// `pmacs_fake_lsp` is a cargo BIN, so every CI leg builds it and +// `fake_lsp_path` resolves it through `env!("CARGO_BIN_EXE_...")` — a +// compile-time constant, not a runtime probe. There is deliberately +// no "binary missing, skip and return ok" arm anywhere in this file: +// that shape is how a suite reports green without running (the a37 +// precedent). A missing binary is a build failure here. +// --------------------------------------------------------------- + +/// `file://` URI for a path, as a server would send it. +fn rd_uri(path: &std::path::Path) -> String { + format!("file://{}", path.display()) +} + +/// One `documentChanges` text-edit entry replacing `line`'s columns +/// `[from, to)` with `new_text`. +fn rd_edit_op( + path: &std::path::Path, + line: u64, + from: u64, + to: u64, + new_text: &str, +) -> serde_json::Value { + serde_json::json!({ + "textDocument": { "uri": rd_uri(path), "version": 1 }, + "edits": [{ + "range": { + "start": { "line": line, "character": from }, + "end": { "line": line, "character": to } + }, + "newText": new_text + }] + }) +} + +/// Point the `rust` server at the `applyeditplan` fake carrying +/// `plan`, and hand back the sink path the client's response to the +/// server-initiated `workspace/applyEdit` will land in. +/// +/// Must run before the first `.rs` file is opened — that open is what +/// launches the server. +fn rd_plan_server( + state: &mut pmacs::editor::EditorState, + dir: &std::path::Path, + plan: &serde_json::Value, +) -> std::path::PathBuf { + let plan_path = dir.join("plan.json"); + std::fs::write( + &plan_path, + serde_json::to_vec(plan).expect("serialize the plan"), + ) + .expect("write the plan"); + let sink = dir.join("applyedit-response.json"); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ + PMACS_FAKE_LSP_MODE = 'applyeditplan', + PMACS_FAKE_LSP_EDIT_PLAN = '{plan_disp}', + PMACS_FAKE_LSP_APPLYEDIT_SINK = '{sink_disp}', + }}, + }}", + plan_disp = plan_path.display(), + sink_disp = sink.display(), + )) + .exec() + .expect("override rust config"); + sink +} + +/// Block until the fake has answered `initialize`. +fn rd_wait_initialized(state: &mut pmacs::editor::EditorState) { + assert!( + pump_lua_flag( + state, + "(function() for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end return false end)()", + 10, + ), + "fake never initialized" + ); +} + +/// Ask the fake to deliver its planned `workspace/applyEdit`. +/// +/// Driven by an `executeCommand` rather than fired at `initialized` +/// so the test controls *when* the batch arrives: every one of these +/// fixtures depends on buffers being open and dirty first, and a +/// server-timed request would race that setup. +fn rd_trigger_apply_edit(state: &mut pmacs::editor::EditorState) { + state + .lua_host + .lua() + .load( + "local sid \ + for _, row in ipairs(pmacs.lsp.list()) do \ + if row.state and row.state.kind == 'initialized' then sid = row.id end \ + end \ + assert(sid, 'no initialized server') \ + pmacs.lsp.request_execute_command(sid, 'pmacs.fake.applyEdit', {})", + ) + .exec() + .expect("dispatch the executeCommand that drives applyEdit"); +} + +/// Pump the real frame order until the fake has published the +/// client's whole response, and return it parsed. +/// +/// The fake writes a `.part` and renames, so observing the file at +/// all means observing a complete record — the wait predicate cannot +/// be weaker than the assertions that follow it. +fn rd_wait_response( + state: &mut pmacs::editor::EditorState, + sink: &std::path::Path, + secs: u64, +) -> serde_json::Value { + let deadline = Instant::now() + Duration::from_secs(secs); + loop { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if let Ok(raw) = std::fs::read(sink) + && let Ok(v) = serde_json::from_slice::(&raw) + { + assert!( + v.get("fakeError").is_none(), + "the fixture itself failed: {v:?}" + ); + return v; + } + assert!( + Instant::now() < deadline, + "the client never answered the server's workspace/applyEdit \ + (no record at {})", + sink.display() + ); + std::thread::sleep(Duration::from_millis(10)); + } +} + +/// `applied` out of an `ApplyWorkspaceEditResult`. +fn rd_applied(response: &serde_json::Value) -> bool { + response + .get("result") + .and_then(|r| r.get("applied")) + .and_then(serde_json::Value::as_bool) + .unwrap_or_else(|| panic!("response carried no boolean `applied`: {response:?}")) +} + +/// `failureReason`, which must be present and non-empty whenever +/// `applied` is false. +fn rd_reason(response: &serde_json::Value) -> String { + let reason = response + .get("result") + .and_then(|r| r.get("failureReason")) + .and_then(serde_json::Value::as_str) + .unwrap_or_else(|| panic!("response carried no `failureReason`: {response:?}")) + .to_owned(); + assert!(!reason.is_empty(), "failureReason must not be empty"); + reason +} + +/// The whole `*errors*` buffer, or `""` when it was never created. +fn rd_errors_text(state: &mut pmacs::editor::EditorState) -> String { + state + .lua_host + .lua() + .load( + "for _, id in ipairs(pmacs.buffer.list()) do \ + local ok, d = pcall(pmacs.describe.buffer, id) \ + if ok and d and d.name == '*errors*' then \ + return id:slice(0, id:len()) \ + end \ + end \ + return ''", + ) + .eval() + .expect("read *errors*") +} + +/// Criterion 11 — absent-plus-ignore succeeds through the real server +/// pump, and the modified buffer bound to that absent path survives. +/// +/// Bite: fails against a preflight that rejects on the presence of a +/// modified buffer without consulting `ignore_if_not_exists`, and +/// against `main`, where the primitive's `NotFound` + ignore branch +/// falls through and destroys the buffer. +#[test] +fn rd11_absent_plus_ignore_succeeds_through_the_server_pump() { + let dir = tempfile::tempdir().expect("tempdir"); + let victim = dir.path().join("victim.rs"); + std::fs::write(&victim, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "delete", "uri": rd_uri(&victim), + "options": { "ignoreIfNotExists": true } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &victim); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + rd_wait_initialized(&mut state); + + // The file goes behind pmacs's back, so the op is a genuine + // absent-plus-ignore no-op with a modified buffer still naming it. + std::fs::remove_file(&victim).expect("unlink behind pmacs's back"); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + rd_applied(&response), + "a no-op delete must not be refused: {response:?}" + ); + + let (valid, text): (bool, String) = state + .lua_host + .lua() + .load("return B:is_valid(), B:slice(0, B:len())") + .eval() + .expect("buffer probe"); + assert!(valid, "THE BITE: the buffer must survive a no-op delete"); + assert_eq!(text, "unsavedcontent\n", "and keep its unsaved text"); +} + +/// Criterion 11a — present-plus-ignore with a modified buffer is still +/// REFUSED. The opposite direction of criterion 11, and the pair is +/// the point: one-direction coverage on a two-direction rule is how +/// the gap survived a framing round. +/// +/// Bite: fails against a preflight that treats `ignore_if_not_exists` +/// as an unconditional bypass rather than consulting the filesystem. +#[test] +fn rd11a_present_plus_ignore_with_a_modified_buffer_is_refused() { + let dir = tempfile::tempdir().expect("tempdir"); + let victim = dir.path().join("victim.rs"); + std::fs::write(&victim, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "delete", "uri": rd_uri(&victim), + "options": { "ignoreIfNotExists": true } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &victim); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "the target EXISTS, so ignoreIfNotExists is irrelevant: {response:?}" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("unsaved changes"), + "the reason must name the conflict, got {reason:?}" + ); + assert!( + victim.exists(), + "THE BITE: the file survives a refused delete" + ); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!(text, "unsavedcontent\n", "and so does the unsaved text"); +} + +/// Criterion 11b — a dangling symlink counts as PRESENT. +/// +/// Bite: fails against a preflight built on `canonicalize`, which +/// resolves symlinks and returns `nil` for a broken one — it would +/// classify this as absent, take the `ignoreIfNotExists` no-op path, +/// and let the batch through. This is the single input on which +/// realpath and `symlink_metadata` disagree, and the reason Q#RD12 +/// specifies the latter. +#[cfg(unix)] +#[test] +fn rd11b_a_dangling_symlink_counts_as_present() { + let dir = tempfile::tempdir().expect("tempdir"); + let real = dir.path().join("real.rs"); + let link = dir.path().join("link.rs"); + std::fs::write(&real, b"content\n").expect("write"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "delete", "uri": rd_uri(&link), + "options": { "ignoreIfNotExists": true } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + // Opened through the LINK path, so the buffer binds to the link — + // `normalize_buffer_path` is lexical and resolves no symlinks. + rd_open(&mut state, "B", &link); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + rd_wait_initialized(&mut state); + + // Break the link. `canonicalize` now says absent; the primitive's + // `symlink_metadata` still says present. + std::fs::remove_file(&real).expect("unlink the destination"); + assert!( + std::fs::symlink_metadata(&link).is_ok(), + "fixture: the link itself must still exist" + ); + assert!( + std::fs::canonicalize(&link).is_err(), + "fixture: the link must be dangling, or this test proves nothing" + ); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "a dangling symlink is present, so the modified buffer refuses: {response:?}" + ); + assert!( + rd_reason(&response).contains("unsaved changes"), + "and refuses for the buffer, not for a missing path" + ); + assert!( + std::fs::symlink_metadata(&link).is_ok(), + "THE BITE: the link survives the refusal" + ); +} + +/// Criterion 11c — absent without `ignoreIfNotExists` refuses in the +/// PLAN, before the earlier op in the batch runs. +/// +/// This is also the half of the preflight that review round 1 +/// required to keep firing: the delete's target is touched by no +/// earlier op, so the deferral must not apply to it. +/// +/// Bite: fails if the verdict maps this state to `clear` and leaves +/// the primitive to discover it mid-batch — that implementation +/// applies the text edit first. +#[test] +fn rd11c_absent_without_ignore_refuses_before_the_earlier_op_runs() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + let missing = dir.path().join("never-existed.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + rd_edit_op(&a, 0, 3, 6, "EDITED"), + { "kind": "delete", "uri": rd_uri(&missing), + "options": { "ignoreIfNotExists": false } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "a known-missing target refuses"); + let reason = rd_reason(&response); + assert!( + reason.contains("os error 2") || reason.to_lowercase().contains("no such file"), + "the reason must carry the NotFound cause, got {reason:?}" + ); + assert!( + reason.contains("nothing was mutated"), + "a plan-time refusal applied nothing and must say so, got {reason:?}" + ); + + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcfooxyz\n", + "THE BITE: the earlier text edit must NOT have applied" + ); +} + +/// Criterion 11d — an unanswerable stat fails closed in the plan. +/// +/// A regular file stands in as the target's parent, so +/// `symlink_metadata` yields `NotADirectory` rather than `NotFound` on +/// every supported platform. +/// +/// Bite: fails if a non-`NotFound` stat error is collapsed to `clear`, +/// or if the binding raises past the value-returning boundary. +#[test] +fn rd11d_an_unanswerable_stat_fails_closed_in_the_plan() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + let not_a_dir = dir.path().join("regular.txt"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + std::fs::write(¬_a_dir, b"I am a file\n").expect("write the would-be parent"); + let target = not_a_dir.join("child.rs"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + rd_edit_op(&a, 0, 3, 6, "EDITED"), + { "kind": "delete", "uri": rd_uri(&target), + "options": { "ignoreIfNotExists": true } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "the preflight never reports safe on a question it could not answer" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("stat"), + "the reason must say the stat failed, got {reason:?}" + ); + assert!( + reason.contains("os error 20") || reason.to_lowercase().contains("not a directory"), + "and must carry the filesystem cause, got {reason:?}" + ); + + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcfooxyz\n", + "THE BITE: `ignoreIfNotExists` must not swallow a non-NotFound error, \ + and the earlier edit must not have applied" + ); +} + +/// Criterion 12, direction 1 — an earlier text edit dirties the buffer +/// a later delete targets. The preflight cannot see it (the snapshot +/// predates the edit), the primitive refuses mid-batch, and the server +/// is told **and told that earlier work stayed applied**. +/// +/// Bite: fails against `main`, where the raise is swallowed at the +/// pump's `pcall(handle_server_requests)` and no response is sent at +/// all; and fails against a reporter that says "nothing was mutated" +/// on every failure, which is the round-1 defect. +#[test] +fn rd12a_edit_then_delete_answers_the_server_and_reports_partial_work() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + rd_edit_op(&a, 0, 3, 6, "EDITED"), + { "kind": "delete", "uri": rd_uri(&a) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "the delete refuses mid-batch"); + let reason = rd_reason(&response); + assert!( + reason.contains("unsaved changes"), + "the reason names the conflict the edit created, got {reason:?}" + ); + assert!( + reason.contains("1 operation") && reason.contains("remain applied"), + "THE BITE: the earlier edit IS still applied and the server must be \ + told so — Q#RD3 permits exactly this. got {reason:?}" + ); + + assert!(a.exists(), "the file survives the refusal"); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcEDITEDxyz\n", + "and the earlier edit really is still there, so the message is true" + ); +} + +/// Criterion 12, direction 2 — an earlier rename moves a MODIFIED +/// buffer into a later delete's subtree, after the snapshot. +/// +/// Bite: as for direction 1. The rename-into shape additionally proves +/// the primitive's prefix-aware validation is what catches it, since +/// no plan-time verdict could have. +#[test] +fn rd12b_rename_into_delete_answers_the_server_and_reports_partial_work() { + let dir = tempfile::tempdir().expect("tempdir"); + let tree = dir.path().join("tree"); + std::fs::create_dir(&tree).expect("mkdir tree"); + let outside = dir.path().join("m.rs"); + let inside = tree.join("m.rs"); + std::fs::write(&outside, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "rename", "oldUri": rd_uri(&outside), "newUri": rd_uri(&inside) }, + { "kind": "delete", "uri": rd_uri(&tree), + "options": { "recursive": true } } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &outside); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "the recursive delete must refuse over the just-moved modified buffer" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("unsaved changes"), + "the reason names the conflict, got {reason:?}" + ); + assert!( + reason.contains("1 operation") && reason.contains("remain applied"), + "THE BITE: the rename IS still applied and the server must be told. \ + got {reason:?}" + ); + + assert!(inside.exists(), "the rename really did happen"); + assert!(!outside.exists(), "and is not undone by the later refusal"); + assert!(tree.is_dir(), "the tree survives the refusal"); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!(text, "unsavedcontent\n", "and the unsaved text survives"); +} + +/// Criterion 13 — the refusal reaches the server on the unattended +/// path AND leaves the durable `*errors*` trace. +/// +/// A direct-call test on `apply_resource_op` does not satisfy this and +/// the framing rejects it: on `main` the raise is swallowed by +/// `pcall(handle_server_requests)`, so the whole defect lives between +/// the primitive and this seam. +/// +/// Bite: fails against a fix that refuses by raising, and the +/// `*errors*` half fails against a fix that answers the server but +/// writes no trace — which is exactly what an earlier framing revision +/// promised and did not test. +#[test] +fn rd13_the_refusal_answers_the_server_and_leaves_a_durable_trace() { + let dir = tempfile::tempdir().expect("tempdir"); + let victim = dir.path().join("victim.rs"); + std::fs::write(&victim, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ { "kind": "delete", "uri": rd_uri(&victim) } ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &victim); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + rd_wait_initialized(&mut state); + // Asserted as a DELTA, not against an empty buffer: `*errors*` is a + // shared append-only surface and the user's own `init.lua` can have + // written to it before this test ran. Emptiness is an ambient fact; + // "this run appended the record" is the claim. + let errors_before = rd_errors_text(&mut state); + assert!( + !errors_before.contains("lsp:workspace/applyEdit"), + "fixture: the label must not already be present, or the trace \ + assertion is vacuous; *errors* was {errors_before:?}" + ); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "the delete is refused"); + let reason = rd_reason(&response); + assert!( + reason.contains("victim.rs"), + "the server is told which buffer blocked it, got {reason:?}" + ); + + let errors = rd_errors_text(&mut state); + let added = errors + .strip_prefix(errors_before.as_str()) + .unwrap_or(errors.as_str()); + assert!( + added.contains("lsp:workspace/applyEdit"), + "THE BITE (durable half): the trace must carry the boundary's label; \ + this run appended {added:?}" + ); + assert!( + added.contains("unsaved changes"), + "and the reason, not just the label; this run appended {added:?}" + ); + assert!(victim.exists(), "and the file survives"); +} + +/// Criterion 15 — DEFENSIVE. A parse failure still attempts a +/// response. +/// +/// Substituted with an explicit throwing stub, per Q#RD11, and +/// labelled defensive because no server payload can reach the failure: +/// `WorkspaceEditResponse::from_lsp_value` returns `Self`, and the +/// binding's only `?` is `lua_to_json` over a value that arrived +/// through `json_to_lua`. The criterion claims only what a stub can +/// establish — that the boundary reports — not that a server can +/// provoke it. +/// +/// Bite: fails against a wrap that covers `apply_workspace_edit` only, +/// leaving `_parse_workspace_edit` one line outside it. That is the +/// shape on `main`, where the raise escapes to +/// `pcall(handle_server_requests)` and the server is never answered. +#[test] +fn rd15_defensive_a_parse_failure_still_attempts_a_response() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + + let mut state = pmacs::editor::EditorState::new(); + // A plan that would otherwise succeed, so a response saying + // `applied = false` can only have come from the parse stub. + let plan = serde_json::json!({ + "documentChanges": [ rd_edit_op(&a, 0, 3, 6, "EDITED") ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + state + .lua_host + .lua() + .load( + "pmacs.lsp._parse_workspace_edit = \ + function() error('synthetic parse failure') end", + ) + .exec() + .expect("substitute the throwing parse stub"); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "a parse failure cannot report success" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("synthetic parse failure"), + "THE BITE: the parse raise must become the failureReason, not escape \ + the boundary unanswered. got {reason:?}" + ); + + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcfooxyz\n", + "and nothing was applied, since the parse never produced ops" + ); +} + +/// Criterion 18 (review round 1) — a NON-recursive delete inspects +/// only its exact path. +/// +/// The counterexample that falsified the original reasoning. A +/// modified buffer at `tree/gone.rs` whose file has already been +/// deleted blocks a non-recursive delete of the now-EMPTY `tree/` — +/// an op that would have succeeded and that removes none of that +/// buffer's contents, because a non-recursive delete removes the +/// directory entry and nothing beneath it. +/// +/// Bite: fails against a `delete_verdict` that ignores `recursive` and +/// scans descendants for every directory. +#[test] +fn rd18_non_recursive_delete_is_not_blocked_by_an_orphan_beneath_it() { + let dir = tempfile::tempdir().expect("tempdir"); + let tree = dir.path().join("tree"); + std::fs::create_dir(&tree).expect("mkdir"); + let gone = tree.join("gone.rs"); + std::fs::write(&gone, b"content\n").expect("write"); + + let mut state = pmacs::editor::EditorState::new(); + rd_open(&mut state, "B", &gone); + state + .lua_host + .lua() + .load("B:insert(0, 'unsaved')") + .exec() + .expect("dirty the buffer"); + // The file goes; the modified buffer is now an orphan under an + // empty directory. + std::fs::remove_file(&gone).expect("unlink"); + + let (ok, err) = rd_delete(&mut state, &tree, ""); + assert!( + ok, + "THE BITE: a non-recursive delete cannot destroy anything beneath \ + the target, so a buffer beneath it must not refuse the op. got {err}" + ); + assert!(!tree.exists(), "and the empty directory really is removed"); + + let (valid, text): (bool, String) = state + .lua_host + .lua() + .load("return B:is_valid(), B:slice(0, B:len())") + .eval() + .expect("buffer probe"); + assert!(valid, "the orphaned buffer is untouched"); + assert_eq!(text, "unsavedcontent\n", "and keeps its unsaved text"); +} + +/// Criterion 19a (review round 1) — a delete whose target an EARLIER +/// op in the same batch creates is not refused at plan time. +/// +/// Bite: fails against a preflight that judges every delete against +/// the filesystem's initial state. There it reports a `NotFound` the +/// batch itself was about to fix, and the whole legal batch is +/// rejected before anything runs. +#[test] +fn rd19a_create_then_delete_is_not_refused_by_the_plan_time_preflight() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + let transient = dir.path().join("transient.rs"); + let witness = dir.path().join("witness.rs"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "create", "uri": rd_uri(&transient) }, + { "kind": "delete", "uri": rd_uri(&transient) }, + { "kind": "create", "uri": rd_uri(&witness) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + rd_applied(&response), + "THE BITE: `create X` then `delete X` is a legal ordered batch and \ + must not be refused because X is absent when the plan is built. \ + got {response:?}" + ); + assert!( + witness.exists(), + "the batch really ran to the end, so `applied = true` is not vacuous" + ); + assert!(!transient.exists(), "and the delete really deleted"); +} + +/// Criterion 19b (review round 1) — the same for a target an earlier +/// RENAME produces. +/// +/// Bite: fails against the initial-state preflight, which reports +/// `NotFound` for the rename's destination and rejects the batch — +/// leaving the source file in place, which is what this asserts. +#[test] +fn rd19b_rename_then_delete_is_not_refused_by_the_plan_time_preflight() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + let source = dir.path().join("source.rs"); + let destination = dir.path().join("destination.rs"); + std::fs::write(&source, b"moving\n").expect("write source"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "rename", "oldUri": rd_uri(&source), "newUri": rd_uri(&destination) }, + { "kind": "delete", "uri": rd_uri(&destination) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + rd_applied(&response), + "`rename A -> B` then `delete B` is legal and ordered: {response:?}" + ); + assert!( + !source.exists(), + "THE BITE: the rename ran, so the batch was not rejected at plan time" + ); + assert!(!destination.exists(), "and the delete then removed it"); +} + +/// Criterion 19c (review round 1) — the deferral does not weaken the +/// guard. `create X -> edit X -> delete X` gets past the plan, and +/// then refuses at the primitive for the RIGHT reason: the edit +/// dirtied the just-created buffer. +/// +/// This is the pin that stops "defer the check" turning into "skip the +/// check". The distinction it draws is between a fabricated plan-time +/// `NotFound` about a path the batch creates, and a real refusal about +/// unsaved work. +/// +/// Bite: fails against the initial-state preflight (which reports +/// `NotFound` instead), and against dropping the primitive's guard for +/// deferred targets (which would report `applied = true`). +#[test] +fn rd19c_deferring_the_check_does_not_skip_it() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + let fresh = dir.path().join("fresh.rs"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "create", "uri": rd_uri(&fresh) }, + { + "textDocument": { "uri": rd_uri(&fresh), "version": 1 }, + "edits": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "newText": "NEW" + }] + }, + { "kind": "delete", "uri": rd_uri(&fresh) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + !rd_applied(&response), + "the edit dirtied the buffer, so the delete must still refuse" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("unsaved changes"), + "THE BITE: refused for the real reason, not a plan-time NotFound \ + about a path this batch created. got {reason:?}" + ); + assert!( + !reason.contains("os error 2)"), + "and specifically NOT a NotFound, got {reason:?}" + ); + assert!( + reason.contains("2 operations") && reason.contains("remain applied"), + "the create and the edit stayed applied, and the server is told: \ + got {reason:?}" + ); + assert!(fresh.exists(), "the created file survives the refusal"); +} + +/// Criterion 20 (review round 1) — the USER-facing message reports +/// partial application too. +/// +/// The rename caller is the one that was wrong: it said "rename +/// aborted" under a comment reading "Preflight rejected it; nothing +/// was mutated", which is false in exactly the case Q#RD3 predicts. +/// Driven through `M-x lsp.rename` because that is where a user reads +/// it; the applier's return value alone would not pin the caller. +/// +/// Bite: fails against any caller that renders the failure without +/// consulting the applied-op count. +#[test] +fn rd20_the_user_facing_message_reports_partial_application() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("a.rs"); + std::fs::write(&a, b"abcfooxyz\n").expect("write a"); + + let mut state = EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + rd_edit_op(&a, 0, 3, 6, "EDITED"), + { "kind": "delete", "uri": rd_uri(&a) } + ] + }); + let _sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &a); + rd_wait_initialized(&mut state); + + state + .lua_host + .lua() + .load("pmacs.lsp.rename()") + .exec() + .expect("invoke rename"); + assert!( + state + .lua_host + .lua() + .load("return pmacs.minibuffer.is_active()") + .eval::() + .unwrap(), + "rename should have opened a minibuffer prompt" + ); + state + .lua_host + .lua() + .load("pmacs.minibuffer.set_contents('BAR'); pmacs.minibuffer.accept()") + .exec() + .expect("accept the rename name"); + + let deadline = Instant::now() + Duration::from_secs(10); + let status = loop { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + let s = state.core.borrow().status.clone(); + if s.contains("LSP: rename") { + break s; + } + assert!( + Instant::now() < deadline, + "the rename never reported; status was {s:?}" + ); + std::thread::sleep(Duration::from_millis(10)); + }; + assert!( + status.contains("remain applied"), + "THE BITE: the earlier edit IS applied, so the user must not be told \ + the rename simply aborted. got {status:?}" + ); + assert!( + !status.contains("nothing was mutated"), + "and must not be told the opposite of what happened. got {status:?}" + ); + assert!(a.exists(), "the file survives the refused delete"); +} From 990bc3e8a76177c985ae4aa5c5fd85e0c07f3679 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:51:51 -0400 Subject: [PATCH 3/6] docs(active-work): re-take the lane position after the #193 merge The count in the previous commit was measured against `64883eb` and was stale the moment that merge landed. Re-measured and pasted as command output. `main` moved twice during this round; the entry now says so, and says why a stale base matters under branch protection. --- docs/active-work.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 6af5b2f..b231127 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -532,13 +532,18 @@ has **no branch and no framing yet**. ``` $ git merge-base HEAD githubsucks/main - 64883ebe0c1785b8188d2dee7c8e6f8ea4518512 + 5e186c798236e244a0a6f68d5df79458b4856106 $ git rev-list --left-right --count HEAD...githubsucks/main - 2 0 + 4 0 ``` - Re-measure before quoting this anywhere: `main` has branch protection - now, and #193 was open behind #192 when this was written. + **Re-measure before quoting this anywhere.** `main` moved TWICE while + this lane's round-1 fixes were being written — #192 and then #193 — + and each move invalidated the count above. `main` has branch + protection, so a stale base is not merely untidy: all 12 checks must + pass on the merging head, and a conflicting PR builds no merge ref at + all, so a green run from before the move reads as current when it is + not. - **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 From c804dd523cf9c3c79e27fb4907df0c700a77c594 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:58:11 -0400 Subject: [PATCH 4/6] docs(active-work): stop pasting a count that invalidates itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry pasted `4 0`, measured immediately before the commit that wrote it; at the pushed head the same command said `5 0`. An ahead-count is self-referential in the file that carries it, so the entry now pastes only the merge-base — which is stable — and says to run the count on demand. Behind-count stays the thing that matters, and the merge-base tells you whether it can still be zero. --- docs/active-work.md | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index b231127..97d77d5 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -533,17 +533,23 @@ has **no branch and no framing yet**. ``` $ git merge-base HEAD githubsucks/main 5e186c798236e244a0a6f68d5df79458b4856106 - $ git rev-list --left-right --count HEAD...githubsucks/main - 4 0 ``` - **Re-measure before quoting this anywhere.** `main` moved TWICE while - this lane's round-1 fixes were being written — #192 and then #193 — - and each move invalidated the count above. `main` has branch - protection, so a stale base is not merely untidy: all 12 checks must - pass on the merging head, and a conflicting PR builds no merge ref at - all, so a green run from before the move reads as current when it is - not. + That is the whole durable fact, and it is deliberately the ONLY + number pasted here. **An ahead-count cannot be recorded in the file + it counts**: writing it is a commit, so the value is stale by one the + instant it is written, and the previous attempt at this entry proved + it — a pasted `4 0` read `5 0` at the pushed head. Run + `git rev-list --left-right --count HEAD...githubsucks/main` when you + need it; the merge-base above is what tells you whether the answer is + still meaningful. + + **Re-measure the merge-base too before relying on it.** `main` moved + TWICE while this lane's round-1 fixes were being written — #192 and + then #193. `main` has branch protection now, so a stale base is not + merely untidy: all 12 checks must pass on the merging head, and a + conflicting PR builds no merge ref at all, so a green run from before + the move reads as current when it is not. - **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 From cb7fe818bd4b53652091db2ba4b3e9c86c7196a5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:28:24 -0400 Subject: [PATCH 5/6] fix(lsp): close review gaps in workspace edit reporting Normalize batch dependency paths through the registry's lexical canonical form so equivalent URI spellings do not revive the initial-state preflight bug. Separate execution-started state from the count of completed plan items. Preflight failures retain the no-mutation guarantee, while runtime failures conservatively acknowledge that the failing item may itself have changed a buffer or the filesystem. Add real-server-pump acceptance for dot-path dependency aliases, partial text edits within one item, and resource-operation side effects, and record the review-round corrections in the framing. --- builtin/runtime/lsp.lua | 87 ++++++----- docs/resource-op-delete-guard-framing.md | 121 ++++++++++++++-- tests/m4_acceptance.rs | 177 +++++++++++++++++++++++ 3 files changed, 341 insertions(+), 44 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 2048a3e..f17827f 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1298,27 +1298,32 @@ end -- (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, applied_op_count` 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. +-- `nil, message, applied_op_count, execution_started` 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. -- -- The third failure value is load-bearing, not decoration: Q#RD3 -- permits partial application, so `applied_op_count > 0` means earlier -- plan items ARE still applied and no caller may say otherwise. - --- Strip trailing separators so a path compares by components. -local function strip_trailing_slash(p) - while #p > 1 and p:sub(-1) == "/" do p = p:sub(1, #p - 1) end - return p -end +-- `execution_started` is independently load-bearing: a plan item can +-- mutate before it fails (multiple text edits are sequential, and a +-- resource primitive may have intermediate filesystem effects), so +-- zero fully-applied items does NOT prove that nothing was mutated. -- True when `a` and `b` name the same path, or one lies beneath the -- other. Component-aware, like the Rust side's `Path::starts_with`: a -- raw string prefix would make `/tree` an ancestor of `/tree-sibling`. +-- +-- Compare on the buffer registry's lexical canonical form, not the raw +-- decoded URI spelling. `file:///tree/./x` and `file:///tree/x` reach +-- the same filesystem entry; treating them as unrelated would judge a +-- later delete against the initial filesystem and fabricate NotFound +-- for an earlier create. This is deliberately lexical — resolving +-- symlinks would change filesystem identity and fail for a new path. local function paths_related(a, b) - a, b = strip_trailing_slash(a), strip_trailing_slash(b) + a, b = pmacs.path.canonicalize(a), pmacs.path.canonicalize(b) if a == b then return true end if #a < #b then a, b = b, a end return a:sub(1, #b) == b and (b == "/" or a:sub(#b + 1, #b + 1) == "/") @@ -1358,12 +1363,16 @@ local function apply_workspace_edit(ops) if op.op == "edit" then if op.edits and #op.edits > 0 then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + end plan[#plan + 1] = { kind = "edit", path = path, edits = op.edits } end elseif op.op == "create" then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + end plan[#plan + 1] = { kind = "create", path = path, overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists, @@ -1374,7 +1383,7 @@ local function apply_workspace_edit(ops) local to = pmacs.lsp.path_for_uri(op.new_uri) if not from or not to then return nil, "cannot resolve rename " .. - tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0 + tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0, false end plan[#plan + 1] = { kind = "rename", old_path = from, new_path = to, @@ -1384,7 +1393,9 @@ local function apply_workspace_edit(ops) batch_changes[#batch_changes + 1] = to 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), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + 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 @@ -1414,7 +1425,9 @@ local function apply_workspace_edit(ops) recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists, } - if verdict.kind == "refuse" then return nil, verdict.message, 0 end + if verdict.kind == "refuse" then + return nil, verdict.message, 0, false + end end plan[#plan + 1] = { kind = "delete", path = path, @@ -1456,7 +1469,7 @@ local function apply_workspace_edit(ops) end if not ok then restore_origin() - return nil, tostring(err), applied_ops + return nil, tostring(err), applied_ops, true end applied_ops = applied_ops + 1 end @@ -1470,16 +1483,22 @@ end -- -- Q#RD3 explicitly permits partial application: an earlier text edit -- can apply and dirty a buffer before a later delete refuses. So --- "nothing was mutated" is a claim about `applied`, not a constant — --- asserting it unconditionally is a false statement about the user's --- files in precisely the case the framing predicted. -local function workspace_edit_failure(message, applied) +-- "nothing was mutated" is reserved for failures before execution. +-- `applied == 0` after execution began proves only that no whole plan +-- item finished; a multi-edit item or resource primitive can still +-- have changed state before its error. +local function workspace_edit_failure(message, applied, execution_started) applied = applied or 0 if applied > 0 then return string.format( - "failed after %d operation%s — those earlier changes remain applied: %s", + "failed after %d operation%s completed — those earlier changes remain " .. + "applied; the failing operation may also have changed state: %s", applied, (applied == 1 and "" or "s"), tostring(message)) end + if execution_started then + return "failed during the first operation — it may have changed state " .. + "before failing: " .. tostring(message) + end return "aborted, nothing was mutated: " .. tostring(message) end @@ -1971,7 +1990,7 @@ local function handle_server_requests() -- 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, c = pcall(function() + local ok, a, b, c, d = pcall(function() local parsed = pmacs.lsp._parse_workspace_edit(edit) return apply_workspace_edit(parsed.ops) end) @@ -1981,11 +2000,11 @@ local function handle_server_requests() elseif a then applied = true else - -- `c` is the count of plan items already applied. The - -- server is told so, because `applied = false` alone - -- reads as "the workspace is unchanged" and Q#RD3 says - -- it need not be. - reason = workspace_edit_failure(b, c) + -- `c` is the count of plan items already applied; `d` + -- says execution began at all. The server needs both, + -- because a failing plan item can itself mutate before + -- returning an error. + reason = workspace_edit_failure(b, c, d) end else reason = "missing edit" @@ -2473,7 +2492,7 @@ function pmacs.lsp.rename() pmacs.editor.set_status("LSP: rename produced no edits") return end - local n, files, res = apply_workspace_edit(ops) + local n, files, res, execution_started = apply_workspace_edit(ops) if not n then -- On failure the second value is the message and the third -- is how many plan items already applied. It is NOT always @@ -2481,7 +2500,8 @@ function pmacs.lsp.rename() -- unconditionally — that was false in exactly the -- edit-then-delete case the framing predicted. pmacs.editor.set_status( - "LSP: rename " .. workspace_edit_failure(files, res)) + "LSP: rename " .. + workspace_edit_failure(files, res, execution_started)) return end local msg = string.format( @@ -2540,13 +2560,14 @@ end local function apply_code_action(rec, act) local bits = {} if act.has_edit then - local n, files, res = apply_workspace_edit(act.edit) + local n, files, res, execution_started = apply_workspace_edit(act.edit) if not n then -- Same failure shape as the rename caller: `files` is the -- message, `res` the applied-op count (Q#RD3 permits partial -- application, so it can be non-zero). pmacs.editor.set_status( - "LSP: code action " .. workspace_edit_failure(files, res)) + "LSP: code action " .. + workspace_edit_failure(files, res, execution_started)) return end local b = string.format("%d edit(s) / %d file(s)", n, files) diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 66fe675..9746039 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,13 +1,13 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 5, plus §9.** Status: **APPROVED and MERGED as #186 +**Revision 5, plus §§9–10.** Status: **APPROVED and MERGED as #186 (framing only); the implementation is PR #190** on branch `resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. The revision-5 body below is unchanged except for the two bookkeeping -edits §9.6 names and makes in place; **§9 records the corrections -implementation review round 1 found**, including two corrections to -this document. The "DO NOT implement, DO NOT merge" banner this line -replaces was true when revision 5 was written and is not now. +edits §9.6 names and makes in place; **§§9–10 record the corrections +implementation review rounds 1–2 found**, including corrections to this +document. The "DO NOT implement, DO NOT merge" banner this line replaces +was true when revision 5 was written and is not now. Revision 5's lane header — `resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, based on `githubsucks/main` @ @@ -1543,10 +1543,10 @@ that passes against its pre-image has no bite and is rejected. 17. **Every new test is checked with `scripts/bite`** and none reports VACUOUS. -**Criteria 18, 19a–19c and 20 are added by §9**, after implementation -review round 1. They are listed there, with their pre-images, rather -than interleaved here, so this section stays readable as the record of -what revision 5 asked for. +**Criteria 18, 19a–19c and 20 are added by §9; criteria 21 and +22a–22b by §10**, after implementation review rounds 1 and 2. They are +listed there, with their pre-images, rather than interleaved here, so +this section stays readable as the record of what revision 5 asked for. ## 6. Parked — not deferred-and-forgotten @@ -1600,7 +1600,7 @@ suites; `cargo test --test m4_acceptance -- --skip basedpyright`; Touched suite: **`m4_acceptance`** — the resource-op home (§1.14) and the home of every criterion, 1–16 including 11a–11d, plus §9's 18, -19a–19c and 20. +19a–19c and 20, and §10's 21 and 22a–22b. *Amended at implementation (§9.6).* Revision 5 also named **`lsp_dispatch_seams_acceptance`**, for criterion 15's throwing parse @@ -1647,7 +1647,7 @@ the gate list below and §5, which revision 3 left disagreeing: |---|---| | `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) | | `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) | -| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) | +| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 and §10's 21 and 22a–22b — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) | | `src/bin/pmacs_fake_lsp.rs` | **one parameterized mode, `applyeditplan`**, whose `WorkspaceEdit` is read from a test-written file, plus a sink for the client's response — see §9.6 for why one mode replaced the eight named below | ~~`tests/lsp_dispatch_seams_acceptance.rs`~~ — struck at implementation @@ -1875,3 +1875,102 @@ never considered. The rest of the guard was swept for that shape. **Nothing else in this lane decides an affected set.** The reporting path names a buffer only inside a refusal it already computed, and `restore_origin` is best-effort by construction. + + +## 10. Corrections found during implementation — review round 2 + +This section records the two remaining findings on the round-1 repair. +Neither changes the feature boundary, Q#RD1's refusal, Q#RD2's phase +order, or Q#RD3's choice of a filter rather than a transaction. Both +make the round-1 correction true for inputs its first acceptance set did +not enumerate. + +### 10.1 Batch dependency comparison uses the registry's lexical path form + +Section 9.3 correctly required component-aware comparison, but its first +implementation compared **raw decoded URI strings** after stripping +only trailing slashes. That is component-aware without being +path-equivalence-aware: `file:///tree/./x` and +`file:///tree/x` reach the same filesystem entry while comparing +unequal. A legal ordered `create /tree/./x → delete /tree/x` was +therefore refused at plan time with the same fabricated `NotFound` +§9.3 had just fixed for identical spellings. Reproduced through the +real server pump. + +**Decision:** dependency comparison routes both operands through +`pmacs.path.canonicalize`, which is +`editor_core::normalize_buffer_path` itself — absolute, lexically clean, +redundant-separator and `.` / `..` folding, with no filesystem access +and no symlink resolution. This reuses the buffer registry's canonical +form rather than growing a Lua mirror (the COHERENCE §14 / dired Q#DR2 +rule). + +Only the **comparison** is normalized. The plan item retains the decoded +path for execution, so this correction does not silently widen Q#RD10's +raw phase-4 reconciliation or resolve symlinks. Section 9.7's two raw +execution-path findings remain exactly as scoped there. + +### 10.2 Zero completed items does not prove zero mutation + +Section 9.4's `applied_op_count` reports plan items that completed +before a failure. Its first renderer treated `0` as proof that nothing +was mutated. That inference is false **inside one failing item**: + +- a `TextDocumentEdit` contains multiple buffer edits applied + sequentially, so an intercept can accept the first and reject the + second after the first edit changed the buffer; +- a resource primitive can have intermediate filesystem effects before + its terminal error — today the rename arm creates destination parents + before attempting the rename, so a missing source can leave a new + directory behind. + +Both cases were reproduced through the real server pump with the failing +item first in the plan. The response said `aborted, nothing was +mutated` while the buffer or filesystem visibly disagreed. + +**Decision:** the failure result now carries +`execution_started` independently of `applied_op_count`. Only parse and +plan-time failures render `nothing was mutated`. Once execution starts, +the renderer is deliberately conservative: + +- with completed items, it says those earlier changes remain applied + and the failing item may also have changed state; +- with zero completed items, it says the first operation may have + changed state before failing. + +This does not claim that every failing primitive mutates. It refuses to +make a stronger recovery claim than the applier can prove, and one +renderer still serves the server response, rename status, and code +action status. + +### 10.3 Acceptance added by this round + +21. **Lexically equivalent dependency paths are related** (§10.1). + The server sends `create /dir/./x → delete /dir/x → create witness`; + the batch succeeds, `x` is gone, and the witness exists. + *Bite:* fails against raw-string `paths_related`, which preflights + `/dir/x` against the initial filesystem and refuses with `NotFound`. + +22a. **A failing multi-edit item is reported conservatively** (§10.2). + One `TextDocumentEdit` carries two replacements; a deterministic + intercept accepts the higher-offset edit and rejects the second. + The first edit remains in the buffer, `applied` is false, and the + reason must not say nothing was mutated. + *Bite:* fails when `applied_op_count == 0` alone selects the + no-mutation message. + +22b. **A failing resource item is reported conservatively** (§10.2). + A rename with an absent source and a destination under a new parent + fails after creating that parent. The directory remains and the + reason must acknowledge possible state change. + *Bite:* fails against a text-edit-only repair, or any renderer that + still equates zero completed resource items with zero mutation. + +### 10.4 Coherence and scope + +This round still serves **COHERENCE §1.2's silence asymmetry** and +§23's requirement that background computation not become opaque: it +makes the already-added server/user failure trace truthful. It touches +no new golden-journey step, adds no interaction island, adds no setting, +and creates no background work. No config-registry, ownership, or +protocol change follows. diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 2ebba86..d42aae5 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -9669,3 +9669,180 @@ fn rd20_the_user_facing_message_reports_partial_application() { ); assert!(a.exists(), "the file survives the refused delete"); } + +/// Criterion 21 (review round 2) — batch dependency comparison uses +/// the same lexical path form as the buffer registry. Distinct URI +/// spellings such as `/tree/./x` and `/tree/x` reach the same +/// filesystem entry and therefore must be treated as the same target. +/// +/// Bite: fails against raw-string `paths_related`, which judges the +/// delete against the initial filesystem, fabricates `NotFound`, and +/// refuses the legal ordered batch before its create runs. +#[test] +fn rd21_equivalent_dot_path_create_then_delete_is_not_preflight_refused() { + let dir = tempfile::tempdir().expect("tempdir"); + let anchor = dir.path().join("anchor.rs"); + let victim = dir.path().join("victim.rs"); + let witness = dir.path().join("witness.rs"); + std::fs::write(&anchor, b"anchor\n").expect("write anchor"); + let dot_uri = format!("file://{}/./victim.rs", dir.path().display()); + assert_ne!( + dot_uri, + rd_uri(&victim), + "fixture: the URI spellings must differ" + ); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "create", "uri": dot_uri }, + { "kind": "delete", "uri": rd_uri(&victim) }, + { "kind": "create", "uri": rd_uri(&witness) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &anchor); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + rd_applied(&response), + "lexically equivalent paths name the same target, so the ordered \ + create/delete batch is legal: {response:?}" + ); + assert!(!victim.exists(), "the created target was deleted"); + assert!( + witness.exists(), + "THE BITE: the batch ran past the delete, so success is not vacuous" + ); +} + +/// Criterion 22a (review round 2) — zero COMPLETED plan items does not +/// imply zero mutation. Text edits within one `TextDocumentEdit` run +/// sequentially, so a later edit can reject after an earlier one +/// changed the buffer. +/// +/// Bite: fails against a renderer that keys "nothing was mutated" only +/// on `applied_op_count == 0`. +#[test] +fn rd22a_partial_edits_inside_one_item_are_reported_conservatively() { + let dir = tempfile::tempdir().expect("tempdir"); + let target = dir.path().join("target.rs"); + std::fs::write(&target, b"abcdef\n").expect("write target"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [{ + "textDocument": { "uri": rd_uri(&target), "version": 1 }, + "edits": [ + { + "range": { + "start": { "line": 0, "character": 4 }, + "end": { "line": 0, "character": 5 } + }, + "newText": "X" + }, + { + "range": { + "start": { "line": 0, "character": 1 }, + "end": { "line": 0, "character": 2 } + }, + "newText": "Y" + } + ] + }] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &target); + rd_wait_initialized(&mut state); + state + .lua_host + .lua() + .load( + "N = 0 \ + pmacs.buffer.add_intercept(B, function(op) \ + N = N + 1 \ + if N == 2 then error('second edit rejected') end \ + return op \ + end)", + ) + .exec() + .expect("install deterministic second-edit failure"); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "the second edit rejects"); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcdXf\n", + "THE BITE: the first edit in the same item remains applied" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("first operation") && reason.contains("changed state"), + "the response must conservatively report the failing item's possible \ + mutation: {reason:?}" + ); + assert!( + !reason.contains("nothing was mutated"), + "the response must not deny the mutation visible in the buffer: {reason:?}" + ); +} + +/// Criterion 22b (review round 2) — the same conservative reporting +/// covers a resource primitive. The rename arm creates destination +/// parents before attempting the rename, so a missing source can leave +/// a directory behind even though the first plan item failed. +/// +/// Bite: fails against a text-edit-only repair that still reports +/// "nothing was mutated" for a failing resource operation. +#[test] +fn rd22b_a_failing_resource_item_can_leave_filesystem_state() { + let dir = tempfile::tempdir().expect("tempdir"); + let anchor = dir.path().join("anchor.rs"); + let missing = dir.path().join("missing.rs"); + let created_parent = dir.path().join("created-parent"); + let destination = created_parent.join("destination.rs"); + std::fs::write(&anchor, b"anchor\n").expect("write anchor"); + assert!(!missing.exists(), "fixture: rename source must be absent"); + assert!( + !created_parent.exists(), + "fixture: destination parent must start absent" + ); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [{ + "kind": "rename", + "oldUri": rd_uri(&missing), + "newUri": rd_uri(&destination) + }] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &anchor); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "renaming an absent source fails"); + assert!( + created_parent.is_dir(), + "THE BITE: the primitive created its destination parent before failing" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("first operation") && reason.contains("changed state"), + "the response must conservatively report possible filesystem effects: \ + {reason:?}" + ); + assert!( + !reason.contains("nothing was mutated"), + "the response must not deny the directory left on disk: {reason:?}" + ); +} From 156567b5dd1a5639eb6615c2857a86221a69f7a0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 13:32:08 -0400 Subject: [PATCH 6/6] docs: record PR 190 review round 2 Record the path-normalization and partial-first-operation fixes, their acceptance criteria and bite pre-images, and the green round-2 gate results in the active-work ledger. --- docs/active-work.md | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 97d77d5..ad4a472 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,12 +521,12 @@ 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 #190 OPEN, review round 1 closed +## Resource-op delete guard implementation — PR #190 OPEN, review round 2 closed - Portable branch: `githubsucks/resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. Implements the framing merged as #186 (`docs/resource-op-delete-guard-framing.md`, revision 5 plus its new - §9). Position against `main`, as pasted command output rather than a + §§9-10). Position against `main`, as pasted command output rather than a remembered constant — **`main` moved while this lane was being written**: @@ -585,6 +585,25 @@ has **no branch and no framing yet**. is now a parameter of the shared query. The counterexample is an orphan: a modified buffer at `tree/gone.rs` whose file is already gone blocked a non-recursive delete of the now-*empty* `tree/`. +- **Review round 2 found two more defects; both are fixed and recorded in + the framing's new §10:** + - **P1 §10.1 — dependency filtering compared raw path spellings.** + `create dir/./x -> delete dir/x` was wrongly preflight-refused even + though both operations name the same lexical path. The comparison + now runs both sides through the registry's existing lexical + `pmacs.path.canonicalize` normalizer before component-aware + containment. This is deliberately comparison-only: operation + execution still receives the server's original path, and no + filesystem/symlink canonicalization was added. + - **P1 §10.2 — a failing first plan item could mutate while reporting + “nothing was mutated.”** `apply_workspace_edit` now returns an + `execution_started` fact in addition to the completed-item count. + Only parse/plan/preflight failures claim that nothing changed. Once + execution starts, the shared renderer conservatively says the + failing operation may have changed state. Criteria 22a and 22b pin + both forms: a multi-edit text item whose first edit lands before its + second edit fails, and a resource rename that creates destination + parents before the filesystem rename fails. - **`delete_verdict` is narrowed, and #171 inherits the narrowed version.** Q#RD6's shared query is this lane's to own; descendant matching is now reserved for recursive deletes. Q#RD5's "inspect widely, @@ -614,18 +633,23 @@ has **no branch and no framing yet**. framing's §7 gate list AND its §8 touch table in the same edit, under §8's permitted simplification. It is still *run* as a gate, because `builtin/runtime/lsp.lua` changed. -- Acceptance: criteria 1-16 plus §9's 18, 19a-19c and 20, all in - `tests/m4_acceptance.rs` and prefixed `rd`. 28 tests. +- Acceptance: criteria 1-16, §9's 18, 19a-19c and 20, plus §10's 21 and + 22a-22b, all in `tests/m4_acceptance.rs` and prefixed `rd`. 28 tests. - Bite verification uses `scripts/bite` **with the positive control** it gained in #192, merged into this lane. The pre-image for the round-1 fixes is this lane's own first commit `1873be6`, not `main` — those defects were introduced by it. Per-criterion results are in the commit - message. -- Gates green at the pushed tree: fmt; clippy `-D warnings`; `--lib` - **1863**; `--lib --features crdt` **2048**; `m4_acceptance` **146** - (was 132); `lsp_dispatch_seams_acceptance` **15**; `dired_acceptance` + message. The round-2 criteria 21, 22a, and 22b each pass on the + round-2 code checkpoint `cb7fe81` and produce a clean assertion + failure against its pre-image `c804dd5`. +- Gates green at the round-2 tree: fmt; clippy `-D warnings`; `--lib` + **1863**; `--lib --features crdt` **2048**; `m4_acceptance` **149** + passed, **3** ignored, **1** filtered; `lsp_dispatch_seams_acceptance` + **15**; `dired_acceptance` **25** and `autosave_acceptance` **29** (the framing's watch items); - required GPU **202**; `git diff --check` clean. + required GPU **202**; full isolated-config workspace sweep; `git diff + --check` clean. The only warning in the non-Clippy CRDT build is the + pre-existing `unused_mut` in `src/daemon.rs`; strict Clippy is clean. - 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`.