From cb7fe818bd4b53652091db2ba4b3e9c86c7196a5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:28:24 -0400 Subject: [PATCH] fix(lsp): close review gaps in workspace edit reporting Normalize batch dependency paths through the registry's lexical canonical form so equivalent URI spellings do not revive the initial-state preflight bug. Separate execution-started state from the count of completed plan items. Preflight failures retain the no-mutation guarantee, while runtime failures conservatively acknowledge that the failing item may itself have changed a buffer or the filesystem. Add real-server-pump acceptance for dot-path dependency aliases, partial text edits within one item, and resource-operation side effects, and record the review-round corrections in the framing. --- builtin/runtime/lsp.lua | 87 ++++++----- docs/resource-op-delete-guard-framing.md | 121 ++++++++++++++-- tests/m4_acceptance.rs | 177 +++++++++++++++++++++++ 3 files changed, 341 insertions(+), 44 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 2048a3e..f17827f 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1298,27 +1298,32 @@ end -- (best-effort: it may itself have been renamed/deleted), on the -- failure path as well as the success path. Returns -- `edit_count, file_count, resource_op_count` on success, or --- `nil, message, applied_op_count` if the preflight rejected the edit --- OR any op failed while executing. No exception escapes this function --- (Q#RD7) — the three callers all handle `nil, message` already, and a --- raise reaching them meant an unattended server request went --- unanswered. +-- `nil, message, applied_op_count, execution_started` if the preflight +-- rejected the edit OR any op failed while executing. No exception +-- escapes this function (Q#RD7) — the three callers all handle +-- `nil, message` already, and a raise reaching them meant an unattended +-- server request went unanswered. -- -- The third failure value is load-bearing, not decoration: Q#RD3 -- permits partial application, so `applied_op_count > 0` means earlier -- plan items ARE still applied and no caller may say otherwise. - --- Strip trailing separators so a path compares by components. -local function strip_trailing_slash(p) - while #p > 1 and p:sub(-1) == "/" do p = p:sub(1, #p - 1) end - return p -end +-- `execution_started` is independently load-bearing: a plan item can +-- mutate before it fails (multiple text edits are sequential, and a +-- resource primitive may have intermediate filesystem effects), so +-- zero fully-applied items does NOT prove that nothing was mutated. -- True when `a` and `b` name the same path, or one lies beneath the -- other. Component-aware, like the Rust side's `Path::starts_with`: a -- raw string prefix would make `/tree` an ancestor of `/tree-sibling`. +-- +-- Compare on the buffer registry's lexical canonical form, not the raw +-- decoded URI spelling. `file:///tree/./x` and `file:///tree/x` reach +-- the same filesystem entry; treating them as unrelated would judge a +-- later delete against the initial filesystem and fabricate NotFound +-- for an earlier create. This is deliberately lexical — resolving +-- symlinks would change filesystem identity and fail for a new path. local function paths_related(a, b) - a, b = strip_trailing_slash(a), strip_trailing_slash(b) + a, b = pmacs.path.canonicalize(a), pmacs.path.canonicalize(b) if a == b then return true end if #a < #b then a, b = b, a end return a:sub(1, #b) == b and (b == "/" or a:sub(#b + 1, #b + 1) == "/") @@ -1358,12 +1363,16 @@ local function apply_workspace_edit(ops) if op.op == "edit" then if op.edits and #op.edits > 0 then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + end plan[#plan + 1] = { kind = "edit", path = path, edits = op.edits } end elseif op.op == "create" then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + end plan[#plan + 1] = { kind = "create", path = path, overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists, @@ -1374,7 +1383,7 @@ local function apply_workspace_edit(ops) local to = pmacs.lsp.path_for_uri(op.new_uri) if not from or not to then return nil, "cannot resolve rename " .. - tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0 + tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0, false end plan[#plan + 1] = { kind = "rename", old_path = from, new_path = to, @@ -1384,7 +1393,9 @@ local function apply_workspace_edit(ops) batch_changes[#batch_changes + 1] = to elseif op.op == "delete" then local path = pmacs.lsp.path_for_uri(op.uri) - if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end + if not path then + return nil, "cannot resolve " .. tostring(op.uri), 0, false + end -- Delete precondition check (Q#RD3). This is a FILTER, not a -- transaction. It catches, before anything in the batch is -- mutated: a plan-time modified or mid-edit buffer, a known @@ -1414,7 +1425,9 @@ local function apply_workspace_edit(ops) recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists, } - if verdict.kind == "refuse" then return nil, verdict.message, 0 end + if verdict.kind == "refuse" then + return nil, verdict.message, 0, false + end end plan[#plan + 1] = { kind = "delete", path = path, @@ -1456,7 +1469,7 @@ local function apply_workspace_edit(ops) end if not ok then restore_origin() - return nil, tostring(err), applied_ops + return nil, tostring(err), applied_ops, true end applied_ops = applied_ops + 1 end @@ -1470,16 +1483,22 @@ end -- -- Q#RD3 explicitly permits partial application: an earlier text edit -- can apply and dirty a buffer before a later delete refuses. So --- "nothing was mutated" is a claim about `applied`, not a constant — --- asserting it unconditionally is a false statement about the user's --- files in precisely the case the framing predicted. -local function workspace_edit_failure(message, applied) +-- "nothing was mutated" is reserved for failures before execution. +-- `applied == 0` after execution began proves only that no whole plan +-- item finished; a multi-edit item or resource primitive can still +-- have changed state before its error. +local function workspace_edit_failure(message, applied, execution_started) applied = applied or 0 if applied > 0 then return string.format( - "failed after %d operation%s — those earlier changes remain applied: %s", + "failed after %d operation%s completed — those earlier changes remain " .. + "applied; the failing operation may also have changed state: %s", applied, (applied == 1 and "" or "s"), tostring(message)) end + if execution_started then + return "failed during the first operation — it may have changed state " .. + "before failing: " .. tostring(message) + end return "aborted, nothing was mutated: " .. tostring(message) end @@ -1971,7 +1990,7 @@ local function handle_server_requests() -- being fixed, one line out of scope. The wrap costs -- nothing and makes the boundary uniform regardless of -- which call fails. - local ok, a, b, c = pcall(function() + local ok, a, b, c, d = pcall(function() local parsed = pmacs.lsp._parse_workspace_edit(edit) return apply_workspace_edit(parsed.ops) end) @@ -1981,11 +2000,11 @@ local function handle_server_requests() elseif a then applied = true else - -- `c` is the count of plan items already applied. The - -- server is told so, because `applied = false` alone - -- reads as "the workspace is unchanged" and Q#RD3 says - -- it need not be. - reason = workspace_edit_failure(b, c) + -- `c` is the count of plan items already applied; `d` + -- says execution began at all. The server needs both, + -- because a failing plan item can itself mutate before + -- returning an error. + reason = workspace_edit_failure(b, c, d) end else reason = "missing edit" @@ -2473,7 +2492,7 @@ function pmacs.lsp.rename() pmacs.editor.set_status("LSP: rename produced no edits") return end - local n, files, res = apply_workspace_edit(ops) + local n, files, res, execution_started = apply_workspace_edit(ops) if not n then -- On failure the second value is the message and the third -- is how many plan items already applied. It is NOT always @@ -2481,7 +2500,8 @@ function pmacs.lsp.rename() -- unconditionally — that was false in exactly the -- edit-then-delete case the framing predicted. pmacs.editor.set_status( - "LSP: rename " .. workspace_edit_failure(files, res)) + "LSP: rename " .. + workspace_edit_failure(files, res, execution_started)) return end local msg = string.format( @@ -2540,13 +2560,14 @@ end local function apply_code_action(rec, act) local bits = {} if act.has_edit then - local n, files, res = apply_workspace_edit(act.edit) + local n, files, res, execution_started = apply_workspace_edit(act.edit) if not n then -- Same failure shape as the rename caller: `files` is the -- message, `res` the applied-op count (Q#RD3 permits partial -- application, so it can be non-zero). pmacs.editor.set_status( - "LSP: code action " .. workspace_edit_failure(files, res)) + "LSP: code action " .. + workspace_edit_failure(files, res, execution_started)) return end local b = string.format("%d edit(s) / %d file(s)", n, files) diff --git a/docs/resource-op-delete-guard-framing.md b/docs/resource-op-delete-guard-framing.md index 66fe675..9746039 100644 --- a/docs/resource-op-delete-guard-framing.md +++ b/docs/resource-op-delete-guard-framing.md @@ -1,13 +1,13 @@ # Framing — `apply_resource_op` delete destroys unsaved work -**Revision 5, plus §9.** Status: **APPROVED and MERGED as #186 +**Revision 5, plus §§9–10.** Status: **APPROVED and MERGED as #186 (framing only); the implementation is PR #190** on branch `resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. The revision-5 body below is unchanged except for the two bookkeeping -edits §9.6 names and makes in place; **§9 records the corrections -implementation review round 1 found**, including two corrections to -this document. The "DO NOT implement, DO NOT merge" banner this line -replaces was true when revision 5 was written and is not now. +edits §9.6 names and makes in place; **§§9–10 record the corrections +implementation review rounds 1–2 found**, including corrections to this +document. The "DO NOT implement, DO NOT merge" banner this line replaces +was true when revision 5 was written and is not now. Revision 5's lane header — `resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`, based on `githubsucks/main` @ @@ -1543,10 +1543,10 @@ that passes against its pre-image has no bite and is rejected. 17. **Every new test is checked with `scripts/bite`** and none reports VACUOUS. -**Criteria 18, 19a–19c and 20 are added by §9**, after implementation -review round 1. They are listed there, with their pre-images, rather -than interleaved here, so this section stays readable as the record of -what revision 5 asked for. +**Criteria 18, 19a–19c and 20 are added by §9; criteria 21 and +22a–22b by §10**, after implementation review rounds 1 and 2. They are +listed there, with their pre-images, rather than interleaved here, so +this section stays readable as the record of what revision 5 asked for. ## 6. Parked — not deferred-and-forgotten @@ -1600,7 +1600,7 @@ suites; `cargo test --test m4_acceptance -- --skip basedpyright`; Touched suite: **`m4_acceptance`** — the resource-op home (§1.14) and the home of every criterion, 1–16 including 11a–11d, plus §9's 18, -19a–19c and 20. +19a–19c and 20, and §10's 21 and 22a–22b. *Amended at implementation (§9.6).* Revision 5 also named **`lsp_dispatch_seams_acceptance`**, for criterion 15's throwing parse @@ -1647,7 +1647,7 @@ the gate list below and §5, which revision 3 left disagreeing: |---|---| | `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) | | `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) | -| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) | +| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 and §10's 21 and 22a–22b — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) | | `src/bin/pmacs_fake_lsp.rs` | **one parameterized mode, `applyeditplan`**, whose `WorkspaceEdit` is read from a test-written file, plus a sink for the client's response — see §9.6 for why one mode replaced the eight named below | ~~`tests/lsp_dispatch_seams_acceptance.rs`~~ — struck at implementation @@ -1875,3 +1875,102 @@ never considered. The rest of the guard was swept for that shape. **Nothing else in this lane decides an affected set.** The reporting path names a buffer only inside a refusal it already computed, and `restore_origin` is best-effort by construction. + + +## 10. Corrections found during implementation — review round 2 + +This section records the two remaining findings on the round-1 repair. +Neither changes the feature boundary, Q#RD1's refusal, Q#RD2's phase +order, or Q#RD3's choice of a filter rather than a transaction. Both +make the round-1 correction true for inputs its first acceptance set did +not enumerate. + +### 10.1 Batch dependency comparison uses the registry's lexical path form + +Section 9.3 correctly required component-aware comparison, but its first +implementation compared **raw decoded URI strings** after stripping +only trailing slashes. That is component-aware without being +path-equivalence-aware: `file:///tree/./x` and +`file:///tree/x` reach the same filesystem entry while comparing +unequal. A legal ordered `create /tree/./x → delete /tree/x` was +therefore refused at plan time with the same fabricated `NotFound` +§9.3 had just fixed for identical spellings. Reproduced through the +real server pump. + +**Decision:** dependency comparison routes both operands through +`pmacs.path.canonicalize`, which is +`editor_core::normalize_buffer_path` itself — absolute, lexically clean, +redundant-separator and `.` / `..` folding, with no filesystem access +and no symlink resolution. This reuses the buffer registry's canonical +form rather than growing a Lua mirror (the COHERENCE §14 / dired Q#DR2 +rule). + +Only the **comparison** is normalized. The plan item retains the decoded +path for execution, so this correction does not silently widen Q#RD10's +raw phase-4 reconciliation or resolve symlinks. Section 9.7's two raw +execution-path findings remain exactly as scoped there. + +### 10.2 Zero completed items does not prove zero mutation + +Section 9.4's `applied_op_count` reports plan items that completed +before a failure. Its first renderer treated `0` as proof that nothing +was mutated. That inference is false **inside one failing item**: + +- a `TextDocumentEdit` contains multiple buffer edits applied + sequentially, so an intercept can accept the first and reject the + second after the first edit changed the buffer; +- a resource primitive can have intermediate filesystem effects before + its terminal error — today the rename arm creates destination parents + before attempting the rename, so a missing source can leave a new + directory behind. + +Both cases were reproduced through the real server pump with the failing +item first in the plan. The response said `aborted, nothing was +mutated` while the buffer or filesystem visibly disagreed. + +**Decision:** the failure result now carries +`execution_started` independently of `applied_op_count`. Only parse and +plan-time failures render `nothing was mutated`. Once execution starts, +the renderer is deliberately conservative: + +- with completed items, it says those earlier changes remain applied + and the failing item may also have changed state; +- with zero completed items, it says the first operation may have + changed state before failing. + +This does not claim that every failing primitive mutates. It refuses to +make a stronger recovery claim than the applier can prove, and one +renderer still serves the server response, rename status, and code +action status. + +### 10.3 Acceptance added by this round + +21. **Lexically equivalent dependency paths are related** (§10.1). + The server sends `create /dir/./x → delete /dir/x → create witness`; + the batch succeeds, `x` is gone, and the witness exists. + *Bite:* fails against raw-string `paths_related`, which preflights + `/dir/x` against the initial filesystem and refuses with `NotFound`. + +22a. **A failing multi-edit item is reported conservatively** (§10.2). + One `TextDocumentEdit` carries two replacements; a deterministic + intercept accepts the higher-offset edit and rejects the second. + The first edit remains in the buffer, `applied` is false, and the + reason must not say nothing was mutated. + *Bite:* fails when `applied_op_count == 0` alone selects the + no-mutation message. + +22b. **A failing resource item is reported conservatively** (§10.2). + A rename with an absent source and a destination under a new parent + fails after creating that parent. The directory remains and the + reason must acknowledge possible state change. + *Bite:* fails against a text-edit-only repair, or any renderer that + still equates zero completed resource items with zero mutation. + +### 10.4 Coherence and scope + +This round still serves **COHERENCE §1.2's silence asymmetry** and +§23's requirement that background computation not become opaque: it +makes the already-added server/user failure trace truthful. It touches +no new golden-journey step, adds no interaction island, adds no setting, +and creates no background work. No config-registry, ownership, or +protocol change follows. diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 2ebba86..d42aae5 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -9669,3 +9669,180 @@ fn rd20_the_user_facing_message_reports_partial_application() { ); assert!(a.exists(), "the file survives the refused delete"); } + +/// Criterion 21 (review round 2) — batch dependency comparison uses +/// the same lexical path form as the buffer registry. Distinct URI +/// spellings such as `/tree/./x` and `/tree/x` reach the same +/// filesystem entry and therefore must be treated as the same target. +/// +/// Bite: fails against raw-string `paths_related`, which judges the +/// delete against the initial filesystem, fabricates `NotFound`, and +/// refuses the legal ordered batch before its create runs. +#[test] +fn rd21_equivalent_dot_path_create_then_delete_is_not_preflight_refused() { + let dir = tempfile::tempdir().expect("tempdir"); + let anchor = dir.path().join("anchor.rs"); + let victim = dir.path().join("victim.rs"); + let witness = dir.path().join("witness.rs"); + std::fs::write(&anchor, b"anchor\n").expect("write anchor"); + let dot_uri = format!("file://{}/./victim.rs", dir.path().display()); + assert_ne!( + dot_uri, + rd_uri(&victim), + "fixture: the URI spellings must differ" + ); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [ + { "kind": "create", "uri": dot_uri }, + { "kind": "delete", "uri": rd_uri(&victim) }, + { "kind": "create", "uri": rd_uri(&witness) } + ] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &anchor); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!( + rd_applied(&response), + "lexically equivalent paths name the same target, so the ordered \ + create/delete batch is legal: {response:?}" + ); + assert!(!victim.exists(), "the created target was deleted"); + assert!( + witness.exists(), + "THE BITE: the batch ran past the delete, so success is not vacuous" + ); +} + +/// Criterion 22a (review round 2) — zero COMPLETED plan items does not +/// imply zero mutation. Text edits within one `TextDocumentEdit` run +/// sequentially, so a later edit can reject after an earlier one +/// changed the buffer. +/// +/// Bite: fails against a renderer that keys "nothing was mutated" only +/// on `applied_op_count == 0`. +#[test] +fn rd22a_partial_edits_inside_one_item_are_reported_conservatively() { + let dir = tempfile::tempdir().expect("tempdir"); + let target = dir.path().join("target.rs"); + std::fs::write(&target, b"abcdef\n").expect("write target"); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [{ + "textDocument": { "uri": rd_uri(&target), "version": 1 }, + "edits": [ + { + "range": { + "start": { "line": 0, "character": 4 }, + "end": { "line": 0, "character": 5 } + }, + "newText": "X" + }, + { + "range": { + "start": { "line": 0, "character": 1 }, + "end": { "line": 0, "character": 2 } + }, + "newText": "Y" + } + ] + }] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &target); + rd_wait_initialized(&mut state); + state + .lua_host + .lua() + .load( + "N = 0 \ + pmacs.buffer.add_intercept(B, function(op) \ + N = N + 1 \ + if N == 2 then error('second edit rejected') end \ + return op \ + end)", + ) + .exec() + .expect("install deterministic second-edit failure"); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "the second edit rejects"); + let text: String = state + .lua_host + .lua() + .load("return B:slice(0, B:len())") + .eval() + .expect("read back"); + assert_eq!( + text, "abcdXf\n", + "THE BITE: the first edit in the same item remains applied" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("first operation") && reason.contains("changed state"), + "the response must conservatively report the failing item's possible \ + mutation: {reason:?}" + ); + assert!( + !reason.contains("nothing was mutated"), + "the response must not deny the mutation visible in the buffer: {reason:?}" + ); +} + +/// Criterion 22b (review round 2) — the same conservative reporting +/// covers a resource primitive. The rename arm creates destination +/// parents before attempting the rename, so a missing source can leave +/// a directory behind even though the first plan item failed. +/// +/// Bite: fails against a text-edit-only repair that still reports +/// "nothing was mutated" for a failing resource operation. +#[test] +fn rd22b_a_failing_resource_item_can_leave_filesystem_state() { + let dir = tempfile::tempdir().expect("tempdir"); + let anchor = dir.path().join("anchor.rs"); + let missing = dir.path().join("missing.rs"); + let created_parent = dir.path().join("created-parent"); + let destination = created_parent.join("destination.rs"); + std::fs::write(&anchor, b"anchor\n").expect("write anchor"); + assert!(!missing.exists(), "fixture: rename source must be absent"); + assert!( + !created_parent.exists(), + "fixture: destination parent must start absent" + ); + + let mut state = pmacs::editor::EditorState::new(); + let plan = serde_json::json!({ + "documentChanges": [{ + "kind": "rename", + "oldUri": rd_uri(&missing), + "newUri": rd_uri(&destination) + }] + }); + let sink = rd_plan_server(&mut state, dir.path(), &plan); + rd_open(&mut state, "B", &anchor); + rd_wait_initialized(&mut state); + + rd_trigger_apply_edit(&mut state); + let response = rd_wait_response(&mut state, &sink, 10); + assert!(!rd_applied(&response), "renaming an absent source fails"); + assert!( + created_parent.is_dir(), + "THE BITE: the primitive created its destination parent before failing" + ); + let reason = rd_reason(&response); + assert!( + reason.contains("first operation") && reason.contains("changed state"), + "the response must conservatively report possible filesystem effects: \ + {reason:?}" + ); + assert!( + !reason.contains("nothing was mutated"), + "the response must not deny the directory left on disk: {reason:?}" + ); +}