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