Merge pull request #190 from levineuwirth/resource-op-delete-guard-impl
feat(lsp): refuse a resource-op delete that would destroy unsaved work
This commit is contained in:
GitHub
commit
e003b81cdd
|
|
@ -1295,64 +1295,213 @@ end
|
|||
-- `find_or_open` makes the target active. Resource ops go through
|
||||
-- `pmacs.buffer.apply_resource_op` (filesystem + buffer-registry
|
||||
-- reconciliation). The buffer the user invoked from is restored last
|
||||
-- (best-effort: it may itself have been renamed/deleted). Returns
|
||||
-- (best-effort: it may itself have been renamed/deleted), on the
|
||||
-- failure path as well as the success path. Returns
|
||||
-- `edit_count, file_count, resource_op_count` on success, or
|
||||
-- `nil, message` if the preflight rejected the edit.
|
||||
-- `nil, message, applied_op_count, execution_started` if the preflight
|
||||
-- rejected the edit OR any op failed while executing. No exception
|
||||
-- escapes this function (Q#RD7) — the three callers all handle
|
||||
-- `nil, message` already, and a raise reaching them meant an unattended
|
||||
-- server request went unanswered.
|
||||
--
|
||||
-- The third failure value is load-bearing, not decoration: Q#RD3
|
||||
-- permits partial application, so `applied_op_count > 0` means earlier
|
||||
-- plan items ARE still applied and no caller may say otherwise.
|
||||
-- `execution_started` is independently load-bearing: a plan item can
|
||||
-- mutate before it fails (multiple text edits are sequential, and a
|
||||
-- resource primitive may have intermediate filesystem effects), so
|
||||
-- zero fully-applied items does NOT prove that nothing was mutated.
|
||||
|
||||
-- True when `a` and `b` name the same path, or one lies beneath the
|
||||
-- other. Component-aware, like the Rust side's `Path::starts_with`: a
|
||||
-- raw string prefix would make `/tree` an ancestor of `/tree-sibling`.
|
||||
--
|
||||
-- Compare on the buffer registry's lexical canonical form, not the raw
|
||||
-- decoded URI spelling. `file:///tree/./x` and `file:///tree/x` reach
|
||||
-- the same filesystem entry; treating them as unrelated would judge a
|
||||
-- later delete against the initial filesystem and fabricate NotFound
|
||||
-- for an earlier create. This is deliberately lexical — resolving
|
||||
-- symlinks would change filesystem identity and fail for a new path.
|
||||
local function paths_related(a, b)
|
||||
a, b = pmacs.path.canonicalize(a), pmacs.path.canonicalize(b)
|
||||
if a == b then return true end
|
||||
if #a < #b then a, b = b, a end
|
||||
return a:sub(1, #b) == b and (b == "/" or a:sub(#b + 1, #b + 1) == "/")
|
||||
end
|
||||
|
||||
local function apply_workspace_edit(ops)
|
||||
local plan = {}
|
||||
-- Paths an EARLIER op in this same batch creates, renames onto,
|
||||
-- renames away from, or removes. A delete whose target is related to
|
||||
-- one of them cannot be judged from the filesystem's *initial*
|
||||
-- state, which is the only state the plan loop can see.
|
||||
--
|
||||
-- Why defer rather than simulate. Q#RD3 already calls this check a
|
||||
-- FILTER, not a transaction, so declining to judge an op is within
|
||||
-- its contract; refusing a legal batch is not. Simulating instead
|
||||
-- would mean modelling filesystem presence AND the buffer registry's
|
||||
-- path bindings across create/rename/edit — the transaction Q#RD3
|
||||
-- declines to build — and a simulation that got it wrong would
|
||||
-- produce false `clear` verdicts, which is the dangerous direction.
|
||||
-- Deferring only forgoes the early, cheap report; the primitive's
|
||||
-- own four-phase guard is untouched and is what actually stands
|
||||
-- between a server and unsaved work.
|
||||
--
|
||||
-- `edit` ops are deliberately NOT in this set. An edit changes no
|
||||
-- path's existence; it can only dirty a buffer, i.e. only turn a
|
||||
-- plan-time `clear` into a primitive-time refusal. That is the
|
||||
-- under-refusal Q#RD3 documents and accepts, and adding edits here
|
||||
-- would merely delay a refusal that is already certain.
|
||||
local batch_changes = {}
|
||||
local function batch_will_change(path)
|
||||
for _, other in ipairs(batch_changes) do
|
||||
if paths_related(path, other) then return true end
|
||||
end
|
||||
return false
|
||||
end
|
||||
for _, op in ipairs(ops or {}) do
|
||||
if op.op == "edit" then
|
||||
if op.edits and #op.edits > 0 then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then
|
||||
return nil, "cannot resolve " .. tostring(op.uri), 0, false
|
||||
end
|
||||
plan[#plan + 1] = { kind = "edit", path = path, edits = op.edits }
|
||||
end
|
||||
elseif op.op == "create" then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then
|
||||
return nil, "cannot resolve " .. tostring(op.uri), 0, false
|
||||
end
|
||||
plan[#plan + 1] = {
|
||||
kind = "create", path = path,
|
||||
overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = path
|
||||
elseif op.op == "rename" then
|
||||
local from = pmacs.lsp.path_for_uri(op.old_uri)
|
||||
local to = pmacs.lsp.path_for_uri(op.new_uri)
|
||||
if not from or not to then
|
||||
return nil, "cannot resolve rename " ..
|
||||
tostring(op.old_uri) .. " -> " .. tostring(op.new_uri)
|
||||
tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0, false
|
||||
end
|
||||
plan[#plan + 1] = {
|
||||
kind = "rename", old_path = from, new_path = to,
|
||||
overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = from
|
||||
batch_changes[#batch_changes + 1] = to
|
||||
elseif op.op == "delete" then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then
|
||||
return nil, "cannot resolve " .. tostring(op.uri), 0, false
|
||||
end
|
||||
-- Delete precondition check (Q#RD3). This is a FILTER, not a
|
||||
-- transaction. It catches, before anything in the batch is
|
||||
-- mutated: a plan-time modified or mid-edit buffer, a known
|
||||
-- missing target without `ignore_if_not_exists`, and a stat we
|
||||
-- could not answer.
|
||||
--
|
||||
-- What it deliberately does not catch: `documentChanges` are
|
||||
-- sequential, so an earlier text edit can dirty a clean buffer
|
||||
-- and an earlier rename can move a modified buffer into a later
|
||||
-- delete's subtree, both after this snapshot. The primitive then
|
||||
-- refuses mid-batch, leaving earlier ops applied. Reporting that
|
||||
-- honestly is Q#RD7's job, not this check's to prevent.
|
||||
--
|
||||
-- The verdict comes from the same Rust helper the primitive
|
||||
-- uses, so the two layers cannot disagree. `no-op` and `clear`
|
||||
-- both pass: rejecting `no-op` would refuse an op the primitive
|
||||
-- treats as doing nothing.
|
||||
--
|
||||
-- Skipped entirely when an earlier op in this batch can change
|
||||
-- this target (see `batch_changes`). Judging `delete X` against
|
||||
-- the initial filesystem when an earlier `create X` or
|
||||
-- `rename A -> X` has not run yet reports a NotFound that the
|
||||
-- batch itself was about to fix, and refuses a legal edit.
|
||||
if not batch_will_change(path) then
|
||||
local verdict = pmacs.buffer._delete_verdict {
|
||||
path = path,
|
||||
recursive = op.recursive,
|
||||
ignore_if_not_exists = op.ignore_if_not_exists,
|
||||
}
|
||||
if verdict.kind == "refuse" then
|
||||
return nil, verdict.message, 0, false
|
||||
end
|
||||
end
|
||||
plan[#plan + 1] = {
|
||||
kind = "delete", path = path,
|
||||
recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = path
|
||||
end
|
||||
end
|
||||
if #plan == 0 then return 0, 0, 0 end
|
||||
local origin = active_buffer_path()
|
||||
local edit_total, files, res_ops = 0, 0, 0
|
||||
-- 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
|
||||
-- successful loop, so a mid-batch refusal stranded the user in
|
||||
-- whatever buffer the last applied op left active.
|
||||
local function restore_origin()
|
||||
if origin then pcall(pmacs.buffer.find_or_open, origin) end
|
||||
end
|
||||
for _, item in ipairs(plan) do
|
||||
local ok, err
|
||||
if item.kind == "edit" then
|
||||
ok, err = pcall(function()
|
||||
pmacs.buffer.find_or_open(item.path)
|
||||
edit_total = edit_total + apply_text_edits(item.edits)
|
||||
files = files + 1
|
||||
end)
|
||||
else
|
||||
pmacs.buffer.apply_resource_op(item)
|
||||
res_ops = res_ops + 1
|
||||
-- Every failure becomes a value. The primitive raises for a
|
||||
-- refusal or an I/O error; converting here is what lets all
|
||||
-- three callers keep using the existing `nil, message` shape
|
||||
-- instead of each growing its own pcall.
|
||||
ok, err = pcall(pmacs.buffer.apply_resource_op, item)
|
||||
if ok then res_ops = res_ops + 1 end
|
||||
end
|
||||
if not ok then
|
||||
restore_origin()
|
||||
return nil, tostring(err), applied_ops, true
|
||||
end
|
||||
-- Return the user to where they invoked from — best-effort, since
|
||||
-- that path may have just been renamed or deleted.
|
||||
if origin then pcall(pmacs.buffer.find_or_open, origin) end
|
||||
applied_ops = applied_ops + 1
|
||||
end
|
||||
restore_origin()
|
||||
return edit_total, files, res_ops
|
||||
end
|
||||
|
||||
-- Render an `apply_workspace_edit` failure for a human or for a
|
||||
-- server's `failureReason`. One renderer for both, so the two cannot
|
||||
-- disagree about what happened.
|
||||
--
|
||||
-- Q#RD3 explicitly permits partial application: an earlier text edit
|
||||
-- can apply and dirty a buffer before a later delete refuses. So
|
||||
-- "nothing was mutated" is reserved for failures before execution.
|
||||
-- `applied == 0` after execution began proves only that no whole plan
|
||||
-- item finished; a multi-edit item or resource primitive can still
|
||||
-- have changed state before its error.
|
||||
local function workspace_edit_failure(message, applied, execution_started)
|
||||
applied = applied or 0
|
||||
if applied > 0 then
|
||||
return string.format(
|
||||
"failed after %d operation%s completed — those earlier changes remain " ..
|
||||
"applied; the failing operation may also have changed state: %s",
|
||||
applied, (applied == 1 and "" or "s"), tostring(message))
|
||||
end
|
||||
if execution_started then
|
||||
return "failed during the first operation — it may have changed state " ..
|
||||
"before failing: " .. tostring(message)
|
||||
end
|
||||
return "aborted, nothing was mutated: " .. tostring(message)
|
||||
end
|
||||
|
||||
-- Re-pull a per-`(server, uri)` store for every buffer attached to
|
||||
-- `sid`. Fire-and-forget: the response absorbs into its store via the
|
||||
-- request's route, exactly like the explicit command path — no await
|
||||
|
|
@ -1832,14 +1981,49 @@ local function handle_server_requests()
|
|||
local edit = ev.params and ev.params.edit
|
||||
local applied, reason = false, nil
|
||||
if edit then
|
||||
-- Wrap parse AND apply, not apply alone (Q#RD7).
|
||||
-- `_parse_workspace_edit` sits one line above the applier
|
||||
-- and is itself fallible, so a parse failure escaped the
|
||||
-- old wrap entirely, was swallowed by the
|
||||
-- `pcall(handle_server_requests)` at the bottom of this
|
||||
-- file, and left the server unanswered — the exact defect
|
||||
-- being fixed, one line out of scope. The wrap costs
|
||||
-- nothing and makes the boundary uniform regardless of
|
||||
-- which call fails.
|
||||
local ok, a, b, c, d = pcall(function()
|
||||
local parsed = pmacs.lsp._parse_workspace_edit(edit)
|
||||
local n, info = apply_workspace_edit(parsed.ops)
|
||||
if n then applied = true else reason = info end
|
||||
return apply_workspace_edit(parsed.ops)
|
||||
end)
|
||||
if not ok then
|
||||
-- A raise from the parse: nothing in the batch ran.
|
||||
reason = workspace_edit_failure(a, 0)
|
||||
elseif a then
|
||||
applied = true
|
||||
else
|
||||
-- `c` is the count of plan items already applied; `d`
|
||||
-- says execution began at all. The server needs both,
|
||||
-- because a failing plan item can itself mutate before
|
||||
-- returning an error.
|
||||
reason = workspace_edit_failure(b, c, d)
|
||||
end
|
||||
else
|
||||
reason = "missing edit"
|
||||
end
|
||||
local result = { applied = applied }
|
||||
if not applied then result.failureReason = tostring(reason) end
|
||||
if not applied then
|
||||
result.failureReason = tostring(reason)
|
||||
-- The durable trace (Q#RD7): one call site, one label,
|
||||
-- written at the layer that actually knows the outcome. A
|
||||
-- Lua preflight rejection never reaches the Rust
|
||||
-- primitive, so logging there would miss the common
|
||||
-- unattended case entirely.
|
||||
pcall(pmacs.buffer._append_error_record,
|
||||
"lsp:workspace/applyEdit", tostring(reason))
|
||||
end
|
||||
-- Always ATTEMPT a response while the response channel
|
||||
-- remains live. `send_response` is itself under an ignored
|
||||
-- pcall, so whether it lands is the transport's business and
|
||||
-- is not observable from here.
|
||||
pcall(pmacs.lsp.send_response, sid, ev.request_id, result)
|
||||
elseif ev.kind == "request"
|
||||
and ev.method == "workspace/inlayHint/refresh" then
|
||||
|
|
@ -2308,10 +2492,16 @@ function pmacs.lsp.rename()
|
|||
pmacs.editor.set_status("LSP: rename produced no edits")
|
||||
return
|
||||
end
|
||||
local n, files, res = apply_workspace_edit(ops)
|
||||
local n, files, res, execution_started = apply_workspace_edit(ops)
|
||||
if not n then
|
||||
-- Preflight rejected it; nothing was mutated.
|
||||
pmacs.editor.set_status("LSP: rename aborted: " .. tostring(files))
|
||||
-- On failure the second value is the message and the third
|
||||
-- is how many plan items already applied. It is NOT always
|
||||
-- zero (Q#RD3), so this must not say "nothing was mutated"
|
||||
-- unconditionally — that was false in exactly the
|
||||
-- edit-then-delete case the framing predicted.
|
||||
pmacs.editor.set_status(
|
||||
"LSP: rename " ..
|
||||
workspace_edit_failure(files, res, execution_started))
|
||||
return
|
||||
end
|
||||
local msg = string.format(
|
||||
|
|
@ -2370,9 +2560,14 @@ end
|
|||
local function apply_code_action(rec, act)
|
||||
local bits = {}
|
||||
if act.has_edit then
|
||||
local n, files, res = apply_workspace_edit(act.edit)
|
||||
local n, files, res, execution_started = apply_workspace_edit(act.edit)
|
||||
if not n then
|
||||
pmacs.editor.set_status("LSP: code action aborted: " .. tostring(files))
|
||||
-- Same failure shape as the rename caller: `files` is the
|
||||
-- message, `res` the applied-op count (Q#RD3 permits partial
|
||||
-- application, so it can be non-zero).
|
||||
pmacs.editor.set_status(
|
||||
"LSP: code action " ..
|
||||
workspace_edit_failure(files, res, execution_started))
|
||||
return
|
||||
end
|
||||
local b = string.format("%d edit(s) / %d file(s)", n, files)
|
||||
|
|
|
|||
|
|
@ -521,6 +521,140 @@ has **no branch and no framing yet**.
|
|||
`FrontendView.fold_projection` to `true` for semantic frontends, which
|
||||
Stage 2 deliberately left `false` (Q#FD21).
|
||||
|
||||
## Resource-op delete guard implementation — PR #190 OPEN, review round 2 closed
|
||||
|
||||
- Portable branch: `githubsucks/resource-op-delete-guard-impl`, worktree
|
||||
`../pmacs-rd-impl`. Implements the framing merged as #186
|
||||
(`docs/resource-op-delete-guard-framing.md`, revision 5 plus its new
|
||||
§§9-10). Position against `main`, as pasted command output rather than a
|
||||
remembered constant — **`main` moved while this lane was being
|
||||
written**:
|
||||
|
||||
```
|
||||
$ git merge-base HEAD githubsucks/main
|
||||
b7bf2c664475c36b60cf7c0361ea75cd3c3b2315
|
||||
```
|
||||
|
||||
That is the whole durable fact, and it is deliberately the ONLY
|
||||
number pasted here. **An ahead-count cannot be recorded in the file
|
||||
it counts**: writing it is a commit, so the value is stale by one the
|
||||
instant it is written, and the previous attempt at this entry proved
|
||||
it — a pasted `4 0` read `5 0` at the pushed head. Run
|
||||
`git rev-list --left-right --count HEAD...githubsucks/main` when you
|
||||
need it; the merge-base above is what tells you whether the answer is
|
||||
still meaningful.
|
||||
|
||||
**Re-measure the merge-base too before relying on it.** `main` moved
|
||||
twice while this lane's round-1 fixes were being written (#192 and
|
||||
#193), then twice more during round 2 (#188 and #194). This branch
|
||||
integrates through #194. `main` has branch protection now, so a stale
|
||||
base is not merely untidy: all 12 checks must pass on the merging head,
|
||||
and a conflicting PR builds no merge ref at all, so a green run from
|
||||
before the move reads as current when it is not.
|
||||
- **The framing's §8 branch plan is superseded and cannot be followed.**
|
||||
It says "one PR — #186, which becomes the implementation PR", written
|
||||
when #186 was still open. #186 merged as framing-only, so the
|
||||
implementation got its own branch and PR. Nothing about the decisions
|
||||
changes; only the branch plan. Both the framing's header and its §8 now
|
||||
say so on their own pages.
|
||||
- **Layer 1 (the primitive) and Layer 2 (the applier + server-request
|
||||
boundary) are both complete and both pinned through their production
|
||||
paths.** The Layer 2 gap the first commit named — criteria 11, 11a-11d,
|
||||
12, 13, 15 having no production-path pin — is closed.
|
||||
- **Review round 1 found four defects; all four are fixed and all four
|
||||
are recorded in the framing's new §9**, because two of them were
|
||||
corrections *to that document*, and a correction living only in a test
|
||||
comment is invisible to the next reader of the framing:
|
||||
- **P1 §9.3 — the preflight broke ordered resource ops.** Every delete
|
||||
was judged against the filesystem's *initial* state, so a valid
|
||||
`create X -> delete X` (or `rename A -> B -> delete B`) was refused
|
||||
with a fabricated `NotFound` about a path the batch was about to
|
||||
create. A regression this lane introduced. **Decision: defer, do not
|
||||
simulate** — a delete whose target is related by component-aware path
|
||||
containment to a path an *earlier* op creates, renames, or removes is
|
||||
left to the primitive. Q#RD3 already calls the check a filter, not a
|
||||
transaction. `edit` ops are deliberately excluded, so the
|
||||
buffer-and-filesystem half still fires early for untouched targets
|
||||
(criterion 11c depends on exactly that).
|
||||
- **P1 §9.5 — the required production-boundary acceptances were
|
||||
missing.** Landed: 11, 11a-11d, 12 (both directions), 13, 15.
|
||||
- **P1 §9.4 — mid-batch failures were misreported as complete aborts.**
|
||||
`apply_workspace_edit` now returns `nil, message, applied_op_count`,
|
||||
and ONE renderer serves both the status line and the server's
|
||||
`failureReason`. All three callers updated.
|
||||
- **P2 §9.2 — non-recursive deletes inspected descendants.** `recursive`
|
||||
is now a parameter of the shared query. The counterexample is an
|
||||
orphan: a modified buffer at `tree/gone.rs` whose file is already gone
|
||||
blocked a non-recursive delete of the now-*empty* `tree/`.
|
||||
- **Review round 2 found two more defects; both are fixed and recorded in
|
||||
the framing's new §10:**
|
||||
- **P1 §10.1 — dependency filtering compared raw path spellings.**
|
||||
`create dir/./x -> delete dir/x` was wrongly preflight-refused even
|
||||
though both operations name the same lexical path. The comparison
|
||||
now runs both sides through the registry's existing lexical
|
||||
`pmacs.path.canonicalize` normalizer before component-aware
|
||||
containment. This is deliberately comparison-only: operation
|
||||
execution still receives the server's original path, and no
|
||||
filesystem/symlink canonicalization was added.
|
||||
- **P1 §10.2 — a failing first plan item could mutate while reporting
|
||||
“nothing was mutated.”** `apply_workspace_edit` now returns an
|
||||
`execution_started` fact in addition to the completed-item count.
|
||||
Only parse/plan/preflight failures claim that nothing changed. Once
|
||||
execution starts, the shared renderer conservatively says the
|
||||
failing operation may have changed state. Criteria 22a and 22b pin
|
||||
both forms: a multi-edit text item whose first edit lands before its
|
||||
second edit fails, and a resource rename that creates destination
|
||||
parents before the filesystem rename fails.
|
||||
- **`delete_verdict` is narrowed, and #171 inherits the narrowed
|
||||
version.** Q#RD6's shared query is this lane's to own; descendant
|
||||
matching is now reserved for recursive deletes. Q#RD5's "inspect widely,
|
||||
mutate narrowly" is unchanged in substance — "widely" means the set the
|
||||
op can actually destroy.
|
||||
- **Criterion 3's stated bite: fixed by fixing the SETUP, not the doc.**
|
||||
The framing says it fails against buffer-first ordering. Against the
|
||||
first shipped setup it did not (a directory target with no buffer bound
|
||||
to it), and §9.2's narrowing would then have left that setup with no
|
||||
bite at all. The buffer is now bound to the *exact* deleted path — a
|
||||
file opened, then replaced on disk by a non-empty directory, so a
|
||||
non-recursive `remove_dir` fails with `ENOTEMPTY` deterministically and
|
||||
under any uid. Both stated pre-images now bite, so the framing's wording
|
||||
needed no amendment after all.
|
||||
- **The fake is one parameterized mode, not eight.**
|
||||
`PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit` from
|
||||
`PMACS_FAKE_LSP_EDIT_PLAN` and publishes the client's response to
|
||||
`PMACS_FAKE_LSP_APPLYEDIT_SINK` (written `.part`-then-rename, so a
|
||||
polling reader never sees a partial record). Fail-closed: an unreadable
|
||||
plan sends no `applyEdit` and reports itself through the sink.
|
||||
`pmacs_fake_lsp` is a cargo BIN resolved through
|
||||
`env!("CARGO_BIN_EXE_...")`, so every CI leg builds it and a missing
|
||||
binary is a build failure — there is deliberately no
|
||||
skip-and-return-ok arm.
|
||||
- **Criterion 15's stub is hosted in `m4_acceptance`, and the gate list
|
||||
moved with it.** `lsp_dispatch_seams_acceptance` is struck from the
|
||||
framing's §7 gate list AND its §8 touch table in the same edit, under
|
||||
§8's permitted simplification. It is still *run* as a gate, because
|
||||
`builtin/runtime/lsp.lua` changed.
|
||||
- Acceptance: criteria 1-16, §9's 18, 19a-19c and 20, plus §10's 21 and
|
||||
22a-22b, all in `tests/m4_acceptance.rs` and prefixed `rd`. 28 tests.
|
||||
- Bite verification uses `scripts/bite` **with the positive control** it
|
||||
gained in #192, merged into this lane. The pre-image for the round-1
|
||||
fixes is this lane's own first commit `1873be6`, not `main` — those
|
||||
defects were introduced by it. Per-criterion results are in the commit
|
||||
message. The round-2 criteria 21, 22a, and 22b each pass on the
|
||||
round-2 code checkpoint `cb7fe81` and produce a clean assertion
|
||||
failure against its pre-image `c804dd5`.
|
||||
- Gates green at the round-2 tree: fmt; clippy `-D warnings`; `--lib`
|
||||
**1863**; `--lib --features crdt` **2048**; `m4_acceptance` **149**
|
||||
passed, **3** ignored, **1** filtered; `lsp_dispatch_seams_acceptance`
|
||||
**15**; `dired_acceptance`
|
||||
**25** and `autosave_acceptance` **29** (the framing's watch items);
|
||||
required GPU **202**; full isolated-config workspace sweep; `git diff
|
||||
--check` clean. The only warning in the non-Clippy CRDT build is the
|
||||
pre-existing `unused_mut` in `src/daemon.rs`; strict Clippy is clean.
|
||||
- Recovery from a clean checkout:
|
||||
`git fetch githubsucks && git worktree add ../pmacs-rd-impl
|
||||
-b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`.
|
||||
|
||||
## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED
|
||||
|
||||
- Portable branch: `githubsucks/generated-buffer-immutability`; worktree
|
||||
|
|
@ -819,7 +953,6 @@ has **no branch and no framing yet**.
|
|||
at 42 insertions against 88 deletions — merging it would *revert*
|
||||
current documentation. The section said "whoever confirms the branch
|
||||
carries nothing unique removes the section"; this is that.
|
||||
|
||||
## Parked lane: kill-ring browser + persistence
|
||||
|
||||
- Portable branch: `githubsucks/kill-ring-browser`
|
||||
|
|
|
|||
|
|
@ -1,9 +1,19 @@
|
|||
# Framing — `apply_resource_op` delete destroys unsaved work
|
||||
|
||||
**Revision 5.** Status: **PROPOSED — needs explicit user approval before
|
||||
implementation. DO NOT implement, DO NOT merge.** Lane:
|
||||
`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`,
|
||||
based on `githubsucks/main` @ `7586905`.
|
||||
**Revision 5, plus §§9–10.** Status: **APPROVED and MERGED as #186
|
||||
(framing only); the implementation is PR #190** on branch
|
||||
`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. The
|
||||
revision-5 body below is unchanged except for the two bookkeeping
|
||||
edits §9.6 names and makes in place; **§§9–10 record the corrections
|
||||
implementation review rounds 1–2 found**, including corrections to this
|
||||
document. The "DO NOT implement, DO NOT merge" banner this line replaces
|
||||
was true when revision 5 was written and is not now.
|
||||
|
||||
Revision 5's lane header — `resource-op-delete-guard`, worktree
|
||||
`../pmacs-resource-op-delete`, based on `githubsucks/main` @
|
||||
`7586905` — describes the framing branch, which merged. §8's
|
||||
one-PR-for-both branch plan is superseded for the same reason and is
|
||||
annotated there.
|
||||
|
||||
Revision 5 removes volatile sibling-branch counts from the normative
|
||||
contract. A count is a reading, not a dependency; where history retains
|
||||
|
|
@ -1533,6 +1543,11 @@ that passes against its pre-image has no bite and is rejected.
|
|||
17. **Every new test is checked with `scripts/bite`** and none reports
|
||||
VACUOUS.
|
||||
|
||||
**Criteria 18, 19a–19c and 20 are added by §9; criteria 21 and
|
||||
22a–22b by §10**, after implementation review rounds 1 and 2. They are
|
||||
listed there, with their pre-images, rather than interleaved here, so
|
||||
this section stays readable as the record of what revision 5 asked for.
|
||||
|
||||
|
||||
## 6. Parked — not deferred-and-forgotten
|
||||
|
||||
|
|
@ -1583,12 +1598,18 @@ Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy
|
|||
suites; `cargo test --test m4_acceptance -- --skip basedpyright`;
|
||||
`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; `git diff --check`.
|
||||
|
||||
Touched suites: **`m4_acceptance`** (the resource-op home, §1.14, and
|
||||
the home of criteria 1–14 including 11a–11d, and 16) and
|
||||
**`lsp_dispatch_seams_acceptance`**
|
||||
(criterion 15's throwing parse stub, Q#RD11). Both appear in §8's touch
|
||||
table; revision 3 named the second here but omitted it there, and the
|
||||
two lists are now maintained together.
|
||||
Touched suite: **`m4_acceptance`** — the resource-op home (§1.14) and
|
||||
the home of every criterion, 1–16 including 11a–11d, plus §9's 18,
|
||||
19a–19c and 20, and §10's 21 and 22a–22b.
|
||||
|
||||
*Amended at implementation (§9.6).* Revision 5 also named
|
||||
**`lsp_dispatch_seams_acceptance`**, for criterion 15's throwing parse
|
||||
stub. §8 permits hosting that stub in `m4_acceptance` instead
|
||||
**provided the gate list moves with it**, and it does: criterion 15
|
||||
drives the same server pump as 11–13, so splitting it across two
|
||||
suites would have duplicated the whole fixture. The suite is therefore
|
||||
struck from this list **and** from §8's touch table, in the same edit.
|
||||
The lists are maintained together, which is what revision 3 got wrong.
|
||||
|
||||
`dired_acceptance` and `autosave_acceptance` are watch items, not
|
||||
touched files — the former for Q#RD6's shared lookup, the latter because
|
||||
|
|
@ -1612,6 +1633,13 @@ approved the implementation commits land on this same branch.
|
|||
|
||||
**Implementation does not begin until the user approves this revision.**
|
||||
|
||||
**Superseded in fact, not by decision.** #186 merged as framing-only,
|
||||
so the implementation necessarily got its own branch
|
||||
(`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`) and its
|
||||
own PR, #190. No decision in this document changes; only the branch
|
||||
plan, which described a PR that no longer existed by the time
|
||||
implementation started.
|
||||
|
||||
**Files the implementation will touch** — reconciled at rev 5 against
|
||||
the gate list below and §5, which revision 3 left disagreeing:
|
||||
|
||||
|
|
@ -1619,9 +1647,18 @@ the gate list below and §5, which revision 3 left disagreeing:
|
|||
|---|---|
|
||||
| `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) |
|
||||
| `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) |
|
||||
| `tests/m4_acceptance.rs` | criteria 1–14 including 11a–11d, and 16 |
|
||||
| `tests/lsp_dispatch_seams_acceptance.rs` | criterion 15's throwing parse stub (Q#RD11) — this file was named in the gate list but omitted from revision 3's touch list |
|
||||
| `src/bin/pmacs_fake_lsp.rs` | fake modes: blocked delete; edit-then-delete; rename-into-delete; absent-plus-ignore; present-plus-ignore (11a); dangling-symlink (11b); absent-without-ignore (11c); unanswerable-stat (11d) |
|
||||
| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 and §10's 21 and 22a–22b — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) |
|
||||
| `src/bin/pmacs_fake_lsp.rs` | **one parameterized mode, `applyeditplan`**, whose `WorkspaceEdit` is read from a test-written file, plus a sink for the client's response — see §9.6 for why one mode replaced the eight named below |
|
||||
|
||||
~~`tests/lsp_dispatch_seams_acceptance.rs`~~ — struck at implementation
|
||||
(§9.6), together with its entry in §7's gate list.
|
||||
|
||||
The eight fake modes revision 5 named — blocked delete;
|
||||
edit-then-delete; rename-into-delete; absent-plus-ignore;
|
||||
present-plus-ignore (11a); dangling-symlink (11b);
|
||||
absent-without-ignore (11c); unanswerable-stat (11d) — are the eight
|
||||
*fixtures*, and they all still exist. They are payloads now, not modes
|
||||
(§9.6).
|
||||
|
||||
It will **not** touch `src/daemon.rs`, `pmacs-protocol/`,
|
||||
`builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`.
|
||||
|
|
@ -1647,3 +1684,293 @@ reconciliation, the dired async race between dispatch and
|
|||
`remove_blocking`, the rename side of the walk, or `pmacs.fs.remove`
|
||||
(§6). Revision 2's version of this note was written against a stale
|
||||
reading of #171 and is superseded.
|
||||
|
||||
|
||||
## 9. Corrections found during implementation — review round 1
|
||||
|
||||
This section is written **after** revision 5 was approved and
|
||||
implemented, and it changes no design decision. It records four
|
||||
corrections review round 1 found in the implementation, the acceptance
|
||||
criteria they added, and two bookkeeping edits made in place above.
|
||||
It lives here because two of the four are corrections *to this
|
||||
document*, and a correction that lives only in a test comment is
|
||||
invisible to the next reader of the framing.
|
||||
|
||||
### 9.1 Criterion 3's stated bite was wrong — fixed by fixing the setup
|
||||
|
||||
Criterion 3 says it "fails against revision 1's buffer-first
|
||||
ordering". Against the first shipped setup it did **not**, and the
|
||||
claim was found by checking rather than trusting it: that setup bound
|
||||
the clean buffer to a file *beneath* the deleted directory, so no
|
||||
buffer was bound to the deleted path, `find_by_path` matched nothing,
|
||||
and reordering reconciliation ahead of the filesystem mutation left
|
||||
the test passing.
|
||||
|
||||
The first fix considered was to amend the criterion's wording.
|
||||
**That is not what was done.** §9.2 then narrowed the affected set to
|
||||
recursive deletes only, which would have left that setup with *no*
|
||||
bite at all — the pre-image it did catch (validation that removes
|
||||
rather than inspects) stops touching a descendant buffer on a
|
||||
non-recursive delete. So the **setup** is what changed: the buffer is
|
||||
now bound to the **exact** deleted path. A file is opened, the path is
|
||||
then replaced on disk by a non-empty directory, and a non-recursive
|
||||
`remove_dir` fails deterministically with `ENOTEMPTY` — no permission
|
||||
trickery and nothing that behaves differently under a root CI.
|
||||
|
||||
Against that setup the criterion fails against **both** pre-images,
|
||||
which is what revision 5 claimed all along. **The framing's wording
|
||||
needed no amendment; the test did.** Both directions verified by
|
||||
mutation.
|
||||
|
||||
### 9.2 The affected set is scoped by `recursive` (Q#RD12, Q#RD6)
|
||||
|
||||
The shipped `delete_verdict` ignored `recursive` and scanned
|
||||
descendants for **every** directory target, justified in a doc comment
|
||||
on the grounds that a non-recursive delete of a non-empty directory
|
||||
fails at the filesystem anyway, so widening inspection cost nothing.
|
||||
|
||||
**That reasoning is wrong**, and the counterexample is an orphan: a
|
||||
modified buffer at `tree/gone.rs` whose file is already deleted blocks
|
||||
a non-recursive delete of the now-**empty** `tree/` — an op that would
|
||||
have succeeded and that removes none of that buffer's contents.
|
||||
Reproduced in review.
|
||||
|
||||
`recursive` is therefore a parameter of the shared query, and
|
||||
descendant matching is reserved for recursive deletes. This
|
||||
**narrows** the query Q#RD6 hands to #171, which adopts the narrowed
|
||||
version. Q#RD5's "inspect widely, mutate narrowly" is unchanged in
|
||||
substance: "widely" means *the set the op can actually destroy*, which
|
||||
for a non-recursive delete is the target entry alone.
|
||||
|
||||
A symlink to a directory is correctly excluded by the same rule:
|
||||
`symlink_metadata` reports it as not-a-directory, and the primitive
|
||||
`remove_file`s the link without walking through it.
|
||||
|
||||
### 9.3 The preflight defers for targets the batch itself changes (Q#RD3)
|
||||
|
||||
The shipped preflight judged **every** delete against the filesystem's
|
||||
**initial** state, at plan-construction time. A valid `create X →
|
||||
delete X` batch was therefore rejected because X was absent when the
|
||||
plan was built, with a fabricated `NotFound` about a path the batch
|
||||
was about to create; likewise `rename A → B → delete B`. This was a
|
||||
regression introduced by the implementation, not a pre-existing
|
||||
defect.
|
||||
|
||||
**Decision — defer, do not simulate.** A delete whose target is
|
||||
related by path containment to a path an **earlier** op in the same
|
||||
plan creates, renames onto, renames away from, or removes is not
|
||||
judged at plan time; the primitive judges it when it runs. Comparison
|
||||
is component-aware, like the Rust side's `Path::starts_with`.
|
||||
|
||||
Why this is the right half of the choice Q#RD3 already made:
|
||||
|
||||
- Q#RD3 calls this check a **filter, not a transaction**. Declining to
|
||||
judge an op the snapshot cannot see is inside that contract;
|
||||
refusing a legal batch is not.
|
||||
- Simulating instead would mean modelling filesystem presence **and**
|
||||
the buffer registry's path bindings across create / rename / edit —
|
||||
the transaction Q#RD3 declines to build — and a simulation that got
|
||||
it wrong would emit false `clear` verdicts, which is the dangerous
|
||||
direction. Deferral only forgoes the early, cheap report.
|
||||
- The primitive's four-phase guard is untouched and is the thing that
|
||||
actually stands between a server and unsaved work. Criterion 19c
|
||||
pins that deferring is not skipping.
|
||||
|
||||
**`edit` ops are deliberately not in the deferral set.** An edit
|
||||
changes no path's existence; it can only dirty a buffer, i.e. only
|
||||
turn a plan-time `clear` into a primitive-time refusal. That is the
|
||||
under-refusal Q#RD3 documents and accepts, and adding edits would
|
||||
merely delay a refusal that is already certain. Criterion 11c depends
|
||||
on this: its delete target is touched by no earlier op, so the
|
||||
buffer-and-filesystem half of the check still fires before anything is
|
||||
mutated.
|
||||
|
||||
### 9.4 Failure reporting must not deny partial application (Q#RD3, Q#RD7)
|
||||
|
||||
`apply_workspace_edit` discarded, on failure, whether earlier ops had
|
||||
succeeded, and returned a bare `nil, message`. The rename caller then
|
||||
said "rename aborted" under a comment reading "nothing was mutated" —
|
||||
false in exactly the case Q#RD3 predicts, where an earlier text edit
|
||||
applies and dirties the buffer a later delete refuses.
|
||||
|
||||
The applier now returns `nil, message, applied_op_count`, and **one
|
||||
renderer** serves both the user-facing status line and the server's
|
||||
`failureReason`, so the two cannot disagree about what happened. All
|
||||
three callers are updated: the server-request boundary, the rename
|
||||
caller, and the code-action caller.
|
||||
|
||||
### 9.5 Acceptance added by this round
|
||||
|
||||
18. **A non-recursive delete is not blocked by a buffer beneath its
|
||||
target** (§9.2). Modified buffer at `tree/gone.rs` whose file is
|
||||
already gone; non-recursive delete of the now-empty `tree/`
|
||||
succeeds and the buffer is untouched.
|
||||
*Bite:* fails against a `delete_verdict` that ignores `recursive`.
|
||||
|
||||
19a. **`create X → delete X` is not refused at plan time** (§9.3).
|
||||
Assert `applied = true`, and that a later `create` in the same
|
||||
batch produced its file, so the success is not vacuous.
|
||||
*Bite:* fails against the initial-state preflight, which reports
|
||||
`NotFound` for X and rejects the batch before anything runs.
|
||||
|
||||
19b. **`rename A → B → delete B` is not refused at plan time** (§9.3).
|
||||
*Bite:* as 19a; the source file surviving is what carries it,
|
||||
because a plan-time rejection leaves the rename unapplied.
|
||||
|
||||
19c. **Deferring the check is not skipping it** (§9.3).
|
||||
`create X → edit X → delete X` gets past the plan and then refuses
|
||||
at the primitive, naming the unsaved changes the edit created —
|
||||
and reports that two operations remain applied.
|
||||
*Bite:* fails against the initial-state preflight (which reports
|
||||
`NotFound` instead) **and** against dropping the primitive's guard
|
||||
for deferred targets (which would report `applied = true`).
|
||||
|
||||
20. **The user-facing message reports partial application** (§9.4).
|
||||
Driven through `M-x lsp.rename`, because a status line is where a
|
||||
user reads it and the applier's return value alone would not pin
|
||||
the caller.
|
||||
*Bite:* fails against any caller that renders the failure without
|
||||
consulting the applied-op count — i.e. against the shipped
|
||||
"rename aborted".
|
||||
|
||||
All of 11, 11a–11d, 12, 13 and 15 also land in this round; they were
|
||||
specified by revision 5 and were the named gap in the first
|
||||
implementation commit.
|
||||
|
||||
### 9.6 Bookkeeping edits made in place
|
||||
|
||||
- **§7's gate list and §8's touch table both lose
|
||||
`lsp_dispatch_seams_acceptance`**, in the same edit, under §8's
|
||||
permitted simplification. Criterion 15 drives the same server pump
|
||||
as 11–13, so hosting it anywhere else would duplicate the fixture.
|
||||
- **§8's fake-mode row is one parameterized mode, not eight.**
|
||||
`PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit`
|
||||
from the file named by `PMACS_FAKE_LSP_EDIT_PLAN` and publishes the
|
||||
client's response to `PMACS_FAKE_LSP_APPLYEDIT_SINK`. The eight
|
||||
fixtures revision 5 named all exist; they are payloads the test
|
||||
writes rather than modes the fake hardcodes, which keeps each
|
||||
payload next to the assertions that depend on it instead of mirrored
|
||||
across two files. The mode is **fail-closed**: an unreadable or
|
||||
unparsable plan sends no `applyEdit` and reports itself through the
|
||||
sink, so a broken fixture cannot read as a pass. The sink is written
|
||||
to a `.part` and renamed, so a polling reader never sees a partial
|
||||
record — the wait predicate cannot be weaker than the assertion.
|
||||
|
||||
### 9.7 Sweep — every place the guard decides something is "affected"
|
||||
|
||||
§9.2 and §9.3 are one defect class: **a guard whose scope was reasoned
|
||||
about rather than enumerated**, over-refusing on inputs the reasoning
|
||||
never considered. The rest of the guard was swept for that shape.
|
||||
|
||||
| Site | Decision | Verdict |
|
||||
|---|---|---|
|
||||
| `delete_verdict` filesystem classification | is the target a directory whose descendants are at risk? | **Fixed** (§9.2). Now `is_dir() && recursive`. A symlink-to-directory is excluded, matching the primitive's `remove_file`. |
|
||||
| `delete_verdict` buffer matching | which buffers are in the affected set? | **Fixed** (§9.2), exact path always plus descendants only when recursive. |
|
||||
| Lua plan-time preflight | which deletes can be judged from the initial snapshot? | **Fixed** (§9.3). |
|
||||
| `delete_verdict` path normalization | the stat uses the **raw** path; the buffer comparison uses the **normalized** one | **Latent inconsistency, fails safe, not fixed here.** A `~`-prefixed argument stats as a literal `~` directory (absent) while matching buffers bound under `$HOME`. Every branch of that disagreement is safe: without `ignore_if_not_exists` it refuses, and with it the primitive returns early having touched nothing. It matches the primitive's own `remove_file`, which also takes the raw path, so the two layers still agree with each other. |
|
||||
| Phase 4 reconciliation | which buffer is removed after a successful delete? | **Unchanged by design** (Q#RD10). `BufferRegistry::find_by_path` compares paths **raw**, with no normalization, so a buffer stored under a differently-spelled path is not reconciled. This is `main`'s behaviour, Q#RD10 pins "exactly today's", and correcting it would *widen* reconciliation — the one thing Q#RD5 and Q#RD10 forbid. Named so it is not mistaken for an oversight; it belongs to #171. |
|
||||
| `_delete_verdict` argument handling | `recursive` / `ignore_if_not_exists` defaults | **Consistent.** Both default to `false` on the binding and on the primitive, so an omitted `options` object means the same thing at both layers. |
|
||||
| Preflight `batch_changes` membership | which prior ops can change a delete's target? | **Enumerated, not reasoned:** create (1 path), rename (both paths), delete (1 path); edit excluded with the argument in §9.3. |
|
||||
|
||||
**Nothing else in this lane decides an affected set.** The reporting
|
||||
path names a buffer only inside a refusal it already computed, and
|
||||
`restore_origin` is best-effort by construction.
|
||||
|
||||
|
||||
## 10. Corrections found during implementation — review round 2
|
||||
|
||||
This section records the two remaining findings on the round-1 repair.
|
||||
Neither changes the feature boundary, Q#RD1's refusal, Q#RD2's phase
|
||||
order, or Q#RD3's choice of a filter rather than a transaction. Both
|
||||
make the round-1 correction true for inputs its first acceptance set did
|
||||
not enumerate.
|
||||
|
||||
### 10.1 Batch dependency comparison uses the registry's lexical path form
|
||||
|
||||
Section 9.3 correctly required component-aware comparison, but its first
|
||||
implementation compared **raw decoded URI strings** after stripping
|
||||
only trailing slashes. That is component-aware without being
|
||||
path-equivalence-aware: `file:///tree/./x` and
|
||||
`file:///tree/x` reach the same filesystem entry while comparing
|
||||
unequal. A legal ordered `create /tree/./x → delete /tree/x` was
|
||||
therefore refused at plan time with the same fabricated `NotFound`
|
||||
§9.3 had just fixed for identical spellings. Reproduced through the
|
||||
real server pump.
|
||||
|
||||
**Decision:** dependency comparison routes both operands through
|
||||
`pmacs.path.canonicalize`, which is
|
||||
`editor_core::normalize_buffer_path` itself — absolute, lexically clean,
|
||||
redundant-separator and `.` / `..` folding, with no filesystem access
|
||||
and no symlink resolution. This reuses the buffer registry's canonical
|
||||
form rather than growing a Lua mirror (the COHERENCE §14 / dired Q#DR2
|
||||
rule).
|
||||
|
||||
Only the **comparison** is normalized. The plan item retains the decoded
|
||||
path for execution, so this correction does not silently widen Q#RD10's
|
||||
raw phase-4 reconciliation or resolve symlinks. Section 9.7's two raw
|
||||
execution-path findings remain exactly as scoped there.
|
||||
|
||||
### 10.2 Zero completed items does not prove zero mutation
|
||||
|
||||
Section 9.4's `applied_op_count` reports plan items that completed
|
||||
before a failure. Its first renderer treated `0` as proof that nothing
|
||||
was mutated. That inference is false **inside one failing item**:
|
||||
|
||||
- a `TextDocumentEdit` contains multiple buffer edits applied
|
||||
sequentially, so an intercept can accept the first and reject the
|
||||
second after the first edit changed the buffer;
|
||||
- a resource primitive can have intermediate filesystem effects before
|
||||
its terminal error — today the rename arm creates destination parents
|
||||
before attempting the rename, so a missing source can leave a new
|
||||
directory behind.
|
||||
|
||||
Both cases were reproduced through the real server pump with the failing
|
||||
item first in the plan. The response said `aborted, nothing was
|
||||
mutated` while the buffer or filesystem visibly disagreed.
|
||||
|
||||
**Decision:** the failure result now carries
|
||||
`execution_started` independently of `applied_op_count`. Only parse and
|
||||
plan-time failures render `nothing was mutated`. Once execution starts,
|
||||
the renderer is deliberately conservative:
|
||||
|
||||
- with completed items, it says those earlier changes remain applied
|
||||
and the failing item may also have changed state;
|
||||
- with zero completed items, it says the first operation may have
|
||||
changed state before failing.
|
||||
|
||||
This does not claim that every failing primitive mutates. It refuses to
|
||||
make a stronger recovery claim than the applier can prove, and one
|
||||
renderer still serves the server response, rename status, and code
|
||||
action status.
|
||||
|
||||
### 10.3 Acceptance added by this round
|
||||
|
||||
21. **Lexically equivalent dependency paths are related** (§10.1).
|
||||
The server sends `create /dir/./x → delete /dir/x → create witness`;
|
||||
the batch succeeds, `x` is gone, and the witness exists.
|
||||
*Bite:* fails against raw-string `paths_related`, which preflights
|
||||
`/dir/x` against the initial filesystem and refuses with `NotFound`.
|
||||
|
||||
22a. **A failing multi-edit item is reported conservatively** (§10.2).
|
||||
One `TextDocumentEdit` carries two replacements; a deterministic
|
||||
intercept accepts the higher-offset edit and rejects the second.
|
||||
The first edit remains in the buffer, `applied` is false, and the
|
||||
reason must not say nothing was mutated.
|
||||
*Bite:* fails when `applied_op_count == 0` alone selects the
|
||||
no-mutation message.
|
||||
|
||||
22b. **A failing resource item is reported conservatively** (§10.2).
|
||||
A rename with an absent source and a destination under a new parent
|
||||
fails after creating that parent. The directory remains and the
|
||||
reason must acknowledge possible state change.
|
||||
*Bite:* fails against a text-edit-only repair, or any renderer that
|
||||
still equates zero completed resource items with zero mutation.
|
||||
|
||||
### 10.4 Coherence and scope
|
||||
|
||||
This round still serves **COHERENCE §1.2's silence asymmetry** and
|
||||
§23's requirement that background computation not become opaque: it
|
||||
makes the already-added server/user failure trace truthful. It touches
|
||||
no new golden-journey step, adds no interaction island, adds no setting,
|
||||
and creates no background work. No config-registry, ownership, or
|
||||
protocol change follows.
|
||||
|
|
|
|||
|
|
@ -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: Write>(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<serde_json::Value, String> {
|
||||
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{}");
|
||||
|
|
|
|||
|
|
@ -1589,6 +1589,131 @@ fn remove_buffer_removed_callback(lua: &Lua, handle: &BufferRemoveCallbackHandle
|
|||
callbacks.remove(handle.buffer, handle.callback_id)
|
||||
}
|
||||
|
||||
/// Verdict for a pending `delete` resource op (resource-op delete
|
||||
/// guard, Q#RD12).
|
||||
///
|
||||
/// Computed once and consumed by two callers in different forms — the
|
||||
/// `apply_resource_op` delete arm and the Lua applier's plan-time
|
||||
/// preflight — so the two layers cannot disagree about the same path.
|
||||
enum DeleteVerdict {
|
||||
/// Path absent **and** `ignore_if_not_exists` set: the op will do
|
||||
/// nothing, and the preflight must not reject it (Q#RD4).
|
||||
NoOp,
|
||||
/// Path present, and every affected buffer is clean and not
|
||||
/// mid-edit. The delete may proceed.
|
||||
Clear,
|
||||
/// The delete must not proceed. `message` always states why;
|
||||
/// `buffer_name` is set only when a buffer caused the refusal.
|
||||
Refuse {
|
||||
message: String,
|
||||
buffer_name: Option<String>,
|
||||
},
|
||||
}
|
||||
|
||||
/// The single shared query behind both delete layers (Q#RD6, Q#RD12).
|
||||
///
|
||||
/// Scans **every** path-bound buffer rather than the first match.
|
||||
/// [`BufferRegistry::find_by_path`] is first-match-only, and duplicate
|
||||
/// path-bound buffers are reachable from public Lua via
|
||||
/// `pmacs.buffer.from_file`, so a clean first match could otherwise
|
||||
/// hide a modified second — a silent guard bypass. Paths are
|
||||
/// normalized on both sides before comparison, so a raw-path lookup
|
||||
/// cannot miss a stored normalized path, and directory containment
|
||||
/// uses component-aware [`std::path::Path::starts_with`] so `/tree`
|
||||
/// does not match `/tree-sibling`.
|
||||
///
|
||||
/// Uses the same `symlink_metadata` call the primitive uses, **not**
|
||||
/// `canonicalize`: the latter resolves symlinks and reports a dangling
|
||||
/// one as absent, which is the single input on which the two disagree
|
||||
/// and exactly the input `ignore_if_not_exists` turns on.
|
||||
///
|
||||
/// **Descendant matching is reserved for `recursive` deletes.** The
|
||||
/// affected set is what the op can actually destroy: a non-recursive
|
||||
/// delete removes the target entry and nothing beneath it, so a buffer
|
||||
/// under the target is not at risk and must not refuse the op. The
|
||||
/// earlier revision of this helper ignored `recursive` and scanned
|
||||
/// descendants for every directory, on the reasoning that a
|
||||
/// non-recursive delete of a *non-empty* directory fails at the
|
||||
/// filesystem anyway. That reasoning was wrong, and the counterexample
|
||||
/// is an orphan: a modified buffer at `tree/gone.rs` whose file is
|
||||
/// already deleted blocks a non-recursive delete of the now-**empty**
|
||||
/// `tree/`, which would have succeeded and would have removed none of
|
||||
/// that buffer's contents.
|
||||
fn delete_verdict(
|
||||
reg: &BufferRegistry,
|
||||
path: &std::path::Path,
|
||||
recursive: bool,
|
||||
ignore_if_not_exists: bool,
|
||||
) -> DeleteVerdict {
|
||||
let scan_descendants = match std::fs::symlink_metadata(path) {
|
||||
// A symlink to a directory reports `is_dir() == false` here, and
|
||||
// that is correct: the primitive `remove_file`s the link and
|
||||
// never walks through it, so nothing beneath the link's target
|
||||
// is at risk either.
|
||||
Ok(md) => md.is_dir() && recursive,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
|
||||
return if ignore_if_not_exists {
|
||||
DeleteVerdict::NoOp
|
||||
} else {
|
||||
DeleteVerdict::Refuse {
|
||||
message: format!("apply_resource_op delete: {e}"),
|
||||
buffer_name: None,
|
||||
}
|
||||
};
|
||||
}
|
||||
// Never report safe on the strength of a question we could not
|
||||
// answer: an unanswerable stat refuses rather than proceeding.
|
||||
Err(e) => {
|
||||
return DeleteVerdict::Refuse {
|
||||
message: format!("apply_resource_op delete (stat): {e}"),
|
||||
buffer_name: None,
|
||||
};
|
||||
}
|
||||
};
|
||||
|
||||
let target = crate::editor_core::normalize_buffer_path(path.to_path_buf());
|
||||
for id in reg.ids() {
|
||||
let Ok(buf) = reg.get(*id) else { continue };
|
||||
let Some(bound) = buf.file_path() else {
|
||||
continue;
|
||||
};
|
||||
let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf());
|
||||
if bound != target && !(scan_descendants && bound.starts_with(&target)) {
|
||||
continue;
|
||||
}
|
||||
// "Modified" is `Buffer::is_modified()`. No new notion of
|
||||
// dirtiness, and a *clean* open buffer is deliberately not
|
||||
// guarded — refusing there would fail legitimate deletes for
|
||||
// anyone who merely has the file open.
|
||||
if buf.is_modified() {
|
||||
return DeleteVerdict::Refuse {
|
||||
message: format!(
|
||||
"apply_resource_op delete: refusing to delete {} — \
|
||||
buffer {:?} has unsaved changes; save or revert it first",
|
||||
path.display(),
|
||||
buf.name()
|
||||
),
|
||||
buffer_name: Some(buf.name().to_string()),
|
||||
};
|
||||
}
|
||||
// Checked here rather than at removal time: today a
|
||||
// `ConcurrentEdit` refusal from `BufferRegistry::remove`
|
||||
// arrives *after* the file is already gone.
|
||||
if buf.editing_in_progress() {
|
||||
return DeleteVerdict::Refuse {
|
||||
message: format!(
|
||||
"apply_resource_op delete: refusing to delete {} — \
|
||||
buffer {:?} is mid-edit; finish the edit first",
|
||||
path.display(),
|
||||
buf.name()
|
||||
),
|
||||
buffer_name: Some(buf.name().to_string()),
|
||||
};
|
||||
}
|
||||
}
|
||||
DeleteVerdict::Clear
|
||||
}
|
||||
|
||||
fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> {
|
||||
registry
|
||||
.borrow_mut()
|
||||
|
|
@ -3311,13 +3436,49 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
|||
}
|
||||
}
|
||||
"delete" => {
|
||||
// Four ordered phases (Q#RD2): stat/no-op
|
||||
// decision -> enumerate and validate affected
|
||||
// buffers -> mutate the filesystem -> reconcile
|
||||
// the registry.
|
||||
//
|
||||
// Validation inspects and removes nothing, so a
|
||||
// filesystem failure leaves every buffer intact
|
||||
// automatically rather than by compensation, and
|
||||
// `on_removed` still observes the path already
|
||||
// gone because reconciliation is last.
|
||||
let path: String = spec.get("path")?;
|
||||
let pb = std::path::PathBuf::from(&path);
|
||||
let recursive: bool = spec.get("recursive").unwrap_or(false);
|
||||
let ignore_if_not_exists: bool =
|
||||
spec.get("ignore_if_not_exists").unwrap_or(false);
|
||||
match std::fs::symlink_metadata(&pb) {
|
||||
Ok(md) => {
|
||||
|
||||
// Phases 1 and 2, as one shared query.
|
||||
let md = {
|
||||
let verdict =
|
||||
delete_verdict(®.borrow(), &pb, recursive, ignore_if_not_exists);
|
||||
match verdict {
|
||||
// Absent plus ignore is a no-op: return
|
||||
// without touching the registry, which
|
||||
// is the `create` arm's existing idiom.
|
||||
// Falling through here is the bug that
|
||||
// let mode (b) destroy a modified buffer
|
||||
// with zero filesystem work.
|
||||
DeleteVerdict::NoOp => return Ok(()),
|
||||
DeleteVerdict::Refuse { message, .. } => {
|
||||
return Err(mlua::Error::external(message));
|
||||
}
|
||||
DeleteVerdict::Clear => {}
|
||||
}
|
||||
// `Clear` implies the path is present; re-stat
|
||||
// for the dir/file decision. A race between
|
||||
// the two calls degrades to an ordinary
|
||||
// filesystem error below, never to data loss.
|
||||
std::fs::symlink_metadata(&pb)
|
||||
.map_err(|e| io_err("delete (stat)", e))?
|
||||
};
|
||||
|
||||
// Phase 3 — the irreversible step, reached only
|
||||
// after validation cleared it.
|
||||
let r = if md.is_dir() {
|
||||
if recursive {
|
||||
std::fs::remove_dir_all(&pb)
|
||||
|
|
@ -3328,14 +3489,15 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
|||
std::fs::remove_file(&pb)
|
||||
};
|
||||
r.map_err(|e| io_err("delete", e))?;
|
||||
}
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
|
||||
if !ignore_if_not_exists {
|
||||
return Err(io_err("delete", e));
|
||||
}
|
||||
}
|
||||
Err(e) => return Err(io_err("delete (stat)", e)),
|
||||
}
|
||||
|
||||
// Phase 4 — reconcile exactly as before
|
||||
// (Q#RD10): the single first exact-path match is
|
||||
// removed and additional clean duplicates are
|
||||
// left in place. Removing them all would route N
|
||||
// buffers through `remove_buffer_and_fire`,
|
||||
// which is phase 2 without phase 1, creating up
|
||||
// to N dangling windows — the parked lifecycle
|
||||
// defect this lane must not enlarge.
|
||||
let bid = reg.borrow().find_by_path(&pb);
|
||||
if let Some(id) = bid {
|
||||
remove_buffer_and_fire(lua, ®, id)?;
|
||||
|
|
@ -3352,6 +3514,99 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
|||
)?;
|
||||
}
|
||||
|
||||
{
|
||||
// Q#RD12 — the synchronous, structured verdict the Lua
|
||||
// preflight needs and cannot otherwise get. `pmacs.fs.stat` is
|
||||
// asynchronous and the applier is synchronous; the only
|
||||
// synchronous filesystem binding is `canonicalize`, which is
|
||||
// realpath-like and disagrees with `symlink_metadata` on
|
||||
// exactly the dangling-symlink input this query turns on.
|
||||
//
|
||||
// Delegates to the same helper the primitive uses, so the two
|
||||
// layers cannot drift apart. Raises only on argument-type
|
||||
// violations, matching the rest of the `pmacs.buffer` surface;
|
||||
// ordinary filesystem conditions and buffer refusals are values.
|
||||
let reg = registry.clone();
|
||||
buffer.set(
|
||||
"_delete_verdict",
|
||||
lua.create_function(move |lua, spec: Table| -> mlua::Result<Table> {
|
||||
let path: String = spec.get("path")?;
|
||||
// `recursive` is read here for the same reason the
|
||||
// primitive reads it: it decides whether descendants are
|
||||
// part of the affected set at all. Dropping it would put
|
||||
// the preflight and the primitive back into disagreement
|
||||
// on every non-recursive directory delete.
|
||||
let recursive: bool = spec.get("recursive").unwrap_or(false);
|
||||
let ignore_if_not_exists: bool = spec.get("ignore_if_not_exists").unwrap_or(false);
|
||||
let pb = std::path::PathBuf::from(&path);
|
||||
let out = lua.create_table()?;
|
||||
match delete_verdict(®.borrow(), &pb, recursive, ignore_if_not_exists) {
|
||||
DeleteVerdict::NoOp => out.set("kind", "no-op")?,
|
||||
DeleteVerdict::Clear => out.set("kind", "clear")?,
|
||||
DeleteVerdict::Refuse {
|
||||
message,
|
||||
buffer_name,
|
||||
} => {
|
||||
out.set("kind", "refuse")?;
|
||||
out.set("message", message)?;
|
||||
if let Some(name) = buffer_name {
|
||||
out.set("buffer_name", name)?;
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(out)
|
||||
})?,
|
||||
)?;
|
||||
}
|
||||
|
||||
{
|
||||
// Q#RD7 — the durable trace, written at the server-request
|
||||
// boundary rather than in the primitive. `LuaHost::
|
||||
// append_to_errors_buffer` is private, so it is not callable
|
||||
// from where the promise was made; and a *Lua preflight*
|
||||
// rejection never reaches the Rust primitive at all, so
|
||||
// primitive-side logging would miss the common unattended case
|
||||
// entirely. Hence a narrow surface reachable from the layer
|
||||
// that actually knows the outcome.
|
||||
let reg = registry.clone();
|
||||
buffer.set(
|
||||
"_append_error_record",
|
||||
lua.create_function(move |lua, (label, message): (String, String)| {
|
||||
let line = format!("[{label}] {message}\n");
|
||||
let (id, edit) = {
|
||||
let mut r = reg.borrow_mut();
|
||||
let id = match r.find_by_name(crate::lua::ERRORS_BUFFER_NAME) {
|
||||
Some(id) => id,
|
||||
None => r.create(crate::lua::ERRORS_BUFFER_NAME),
|
||||
};
|
||||
let Ok(buf) = r.get_mut(id) else {
|
||||
return Ok(());
|
||||
};
|
||||
let pos = buf.len();
|
||||
let Ok(edit) = buf.apply_edit(EditOp::Insert {
|
||||
pos,
|
||||
bytes: line.as_bytes(),
|
||||
}) else {
|
||||
return Ok(());
|
||||
};
|
||||
(id, edit)
|
||||
};
|
||||
// Window TextViews are not attached views, so they miss
|
||||
// `Buffer::apply_edit`'s broadcast; a window displaying
|
||||
// `*errors*` would paint a stale line cache without
|
||||
// this. The CRDT queue matters for the same reason it
|
||||
// does on the host path: `*errors*` is upgraded at every
|
||||
// replica attach.
|
||||
if let Some(core) = lua.app_data_ref::<SharedCore>() {
|
||||
let mut core = core.borrow_mut();
|
||||
core.notify_buffer_edit(id, &edit);
|
||||
core.queue_daemon_origin_crdt_op(id, &edit);
|
||||
}
|
||||
Ok(())
|
||||
})?,
|
||||
)?;
|
||||
}
|
||||
|
||||
{
|
||||
let reg = registry.clone();
|
||||
buffer.set(
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load Diff
Loading…
Reference in New Issue