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.
This commit is contained in:
Levi Neuwirth 2026-07-29 12:28:24 -04:00
parent c804dd523c
commit cb7fe818bd
3 changed files with 341 additions and 44 deletions

View File

@ -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)

View File

@ -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 §§910.** 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; **§§910 record the corrections
implementation review rounds 12 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, 19a19c 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, 19a19c and 20 are added by §9; criteria 21 and
22a22b 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, 116 including 11a11d, plus §9's 18,
19a19c and 20.
19a19c and 20, and §10's 21 and 22a22b.
*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 116 including 11a11d, plus §9's 18, 19a19c and 20 — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) |
| `tests/m4_acceptance.rs` | criteria 116 including 11a11d, plus §9's 18, 19a19c and 20 and §10's 21 and 22a22b **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.

View File

@ -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:?}"
);
}