diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index bf56a4c..f17827f 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1295,64 +1295,213 @@ 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, 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. +-- `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 = 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) == "/") +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, 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) 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, } + 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, false 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, 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 + -- 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. + -- + -- 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, false + 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 - for _, item in ipairs(plan) do - if item.kind == "edit" then - pmacs.buffer.find_or_open(item.path) - edit_total = edit_total + apply_text_edits(item.edits) - files = files + 1 - else - pmacs.buffer.apply_resource_op(item) - res_ops = res_ops + 1 - end - end + -- 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. - if origin then pcall(pmacs.buffer.find_or_open, origin) end + -- 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 + 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 + -- 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), applied_ops, true + 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 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 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 + -- 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 @@ -1832,14 +1981,49 @@ 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, c, d = pcall(function() + local parsed = pmacs.lsp._parse_workspace_edit(edit) + return apply_workspace_edit(parsed.ops) + end) + if not ok then + -- A raise from the parse: nothing in the batch ran. + reason = workspace_edit_failure(a, 0) + elseif a then + applied = true + else + -- `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" 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 @@ -2308,10 +2492,16 @@ 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 - -- 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, execution_started)) return end local msg = string.format( @@ -2370,9 +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 - 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, execution_started)) 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 5527ba7..17f73fe 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,6 +521,140 @@ 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 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-10). 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 + b7bf2c664475c36b60cf7c0361ea75cd3c3b2315 + ``` + + 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 + #193), then twice more during round 2 (#188 and #194). This branch + integrates through #194. `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 + 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/`. +- **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, + 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, §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. 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**; 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`. + ## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED - Portable branch: `githubsucks/generated-buffer-immutability`; worktree @@ -819,7 +953,6 @@ has **no branch and no framing yet**. at 42 insertions against 88 deletions — merging it would *revert* current documentation. The section said "whoever confirms the branch carries nothing unique removes the section"; this is that. - ## Test-improvement arc, lane 3a — CI timeouts and concurrency - Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree @@ -1213,10 +1346,4 @@ Whenever a listed lane changes materially: 3. keep durable architecture in `docs/agent-handoff.md`, not here; 4. remove the lane after merge or abandonment; 5. verify every recovery command from a clean worktree before calling - the transfer complete; -6. **read the seam back after inserting or removing a lane.** A block - that ends in a blank line, inserted above a heading already preceded - by one, leaves a double blank — three consecutive PRs shipped that - and each was caught in review rather than before it. It survives by - being beneath the level anyone reads at. `grep -n -B2 '^## '` over - the file, or just look at the two lines above the next heading. + the transfer complete. diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 8706554..9746039 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–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–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` @ +`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; 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 @@ -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, 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 +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 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 +(§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,293 @@ 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. + + +## 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/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 b624a00..d5a2b4b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1589,6 +1589,131 @@ 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. +/// +/// **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 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 + } 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 && !(scan_descendants && 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 +3436,68 @@ 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, recursive, 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 +3514,99 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> 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, recursive, 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 f56dfc7..220ae99 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8166,3 +8166,1686 @@ 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 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 +// 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. +/// +/// **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. +/// +/// 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 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", &target); + + // 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"), + "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!(target.is_dir(), "and the directory 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})" + ); +} + +// --------------------------------------------------------------- +// 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"); +} + +/// 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:?}" + ); +}