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