fix(lsp): close review round 1 on the resource-op delete guard
Four findings, all reproduced by the reviewer, all accepted. Two of
them are one defect class — a guard whose scope was REASONED ABOUT
rather than enumerated — so both are recorded in the framing's new §9
together with a sweep of every other place this lane decides something
is "affected".
P1 — the delete preflight broke ordered resource operations
(§9.3). Every delete was judged against the filesystem's INITIAL
state, at plan-construction time, so a valid `create X -> delete X`
was refused with a fabricated NotFound about a path the batch was
about to create; likewise `rename A -> B -> delete B`. This was a
regression this lane introduced, not a pre-existing defect.
Decision: DEFER, do not simulate. A delete whose target is related by
component-aware path containment to a path an EARLIER op in the same
plan creates, renames onto, renames away from, or removes is not
judged at plan time; the primitive judges it when it runs. Q#RD3
already calls this check a filter, not a transaction, so declining to
judge is inside its contract and refusing a legal batch is not.
Simulating instead would mean modelling filesystem presence AND the
registry's path bindings across create/rename/edit — the transaction
Q#RD3 declines to build — and a wrong simulation emits false `clear`
verdicts, which is the dangerous direction. `edit` ops are excluded
from the deferral set on purpose: an edit changes no path's existence,
so it can only turn a plan-time `clear` into a primitive-time
refusal, which Q#RD3 already documents and accepts. The
buffer-and-filesystem half therefore still fires early for any target
no prior op touches, which is what criterion 11c pins.
P1 — the production-boundary acceptances are landed (§9.5).
Criteria 11, 11a-11d, 12 (both directions), 13 and 15 now drive a real
`pmacs_fake_lsp` child over a real transport. One parameterized mode,
`applyeditplan`, replaces the eight the framing named: it reads its
whole WorkspaceEdit from a test-written file and publishes the
client's response to a sink, so each of the eight fixtures sits next
to the assertions that depend on it instead of being mirrored across
two files. Fail-closed — an unreadable plan sends no applyEdit and
reports itself through the sink, so a broken fixture cannot read as a
pass — and the sink is written `.part`-then-rename so a polling reader
never sees a partial record. There is no skip-and-return-ok arm
anywhere: `fake_lsp_path` resolves through `env!("CARGO_BIN_EXE_...")`,
a compile-time constant, so a missing binary is a build failure.
P1 — mid-batch failures were misreported as complete aborts (§9.4).
`apply_workspace_edit` now returns `nil, message, applied_op_count`,
and ONE renderer serves both the user-facing status line and the
server's `failureReason`, so the two cannot disagree. All three
callers are updated, not only the rename one.
P2 — non-recursive deletes inspected descendants (§9.2). `recursive`
is now a parameter of the shared query and descendant matching is
reserved for recursive deletes. The old doc comment argued at length
for the wrong behaviour and is replaced by the counterexample that
falsifies it: a modified buffer at `tree/gone.rs` whose file is
already gone blocked a non-recursive delete of the now-EMPTY `tree/`,
an op that would have succeeded and that removes none of that
buffer's contents. This narrows the Q#RD6 query #171 adopts.
Criterion 3's stated bite: fixed by fixing the SETUP, not the doc.
The first commit's test comment carried a correction saying the
framing's wording was wrong. It was wrong only against that setup —
and §9.2's narrowing would then have left the setup with no bite at
all, since a non-recursive delete no longer inspects a descendant.
So the buffer is now bound to the EXACT deleted path: a file is
opened, then replaced on disk by a non-empty directory, and
`remove_dir` fails with ENOTEMPTY deterministically under any uid.
Both of the framing's stated pre-images now bite, so the framing
needed no amendment there. The correction is recorded in §9.1 rather
than only in a test comment, which is where the review asked for it.
WHY THE SHIPPED SUITE PASSED WHILE FINDINGS 1 AND 4 WERE LIVE — two
coverage facts for the next lane. Every delete criterion drove the
PRIMITIVE directly, so nothing in the suite ever built a multi-op plan
and the preflight's plan-time behaviour had no test at all; the only
batch test, `m4_15`, happens to delete a path no earlier op touches.
And every recursive-delete criterion (7, 8, 9) passes `recursive =
true`, while every non-recursive one binds its buffer to the exact
target, so no test in the suite ever combined a non-recursive delete
with a descendant buffer — the exact cell finding 4 lives in.
Sweep, per the review's request. Seven sites decide something is
"affected"; the table is in framing §9.7. Three were the defects
above. Two are unchanged by design and named so they are not mistaken
for oversights: phase-4 reconciliation compares paths RAW via
`BufferRegistry::find_by_path`, which Q#RD10 pins as "exactly today's
behaviour" and which correcting would widen reconciliation — the one
thing Q#RD5 forbids; and `delete_verdict` stats the raw path while
comparing normalized ones, a latent inconsistency whose every branch
fails safe and which matches the primitive's own `remove_file`. Two
are consistent: the `_delete_verdict` binding defaults `recursive` and
`ignore_if_not_exists` the same way the primitive does, and the
deferral set is enumerated (create: 1 path; rename: 2; delete: 1;
edit: excluded, with the argument written down) rather than reasoned
about. Nothing else in the lane decides an affected set.
Bites. Every row was RUN, with the positive control `scripts/bite`
gained in #192 (merged into this lane), and every ref-based row below
reports `OK (assertion)` rather than `OK (COMPILE)`. `1873be6` is this
lane's own first commit: findings 1, 3 and 4 were introduced by it, so
`main` cannot falsify their pins.
rd11a builtin/runtime/lsp.lua @ main OK (assertion)
rd11b builtin/runtime/lsp.lua @ main OK (assertion)
rd11c builtin/runtime/lsp.lua @ main OK (assertion)
rd11d builtin/runtime/lsp.lua @ main OK (assertion)
rd12a builtin/runtime/lsp.lua @ main OK (assertion)
rd12b builtin/runtime/lsp.lua @ main OK (assertion)
rd13 builtin/runtime/lsp.lua @ main OK (assertion)
rd15 builtin/runtime/lsp.lua @ main OK (assertion)
rd18 src/lua_bindings/mod.rs @ 1873be6 OK (assertion)
rd19a builtin/runtime/lsp.lua @ 1873be6 OK (assertion)
rd19b builtin/runtime/lsp.lua @ 1873be6 OK (assertion)
rd19c builtin/runtime/lsp.lua @ 1873be6 OK (assertion)
rd20 builtin/runtime/lsp.lua @ 1873be6 OK (assertion)
Two rows need their weakness stated rather than hidden.
rd11 is VACUOUS against `main`'s `lsp.lua` and the script says so — a
preflight-less applier passes it, which is expected, because rd11 is
the direction that asserts the guard does NOT over-refuse (the same
shape as criteria 2, 7, 9 and 14). It bites two other ways, both run:
`OK (assertion)` against `main`'s `src/lua_bindings/mod.rs`, where the
primitive's absent-plus-ignore branch destroys the buffer; and against
a hand mutation dropping `ignore_if_not_exists` from the preflight
call, which is the pre-image the framing actually names for it.
rd3's two pre-images are designs never committed, so no ref carries
them and `scripts/bite` cannot be used. Hand-mutated instead:
reconciliation moved ahead of the filesystem mutation makes rd3 fail
on exactly its stated assertion (and rd4 with it). On this setup that
mutation and "validation that removes rather than inspects" are the
same mutation, because the buffer is bound to the exact deleted path —
stated because the first shipped setup could see neither.
The eight rows against `main`'s `lsp.lua` all fail by TIMEOUT rather
than by a value assertion, and that is the pre-image behaviour, not a
flaky harness: on `main` the primitive's raise escapes the applier,
escapes `handle_server_requests`, is swallowed by the
`pcall(handle_server_requests)` at the bottom of the file, and the
server is never answered at all. The sink is therefore never written.
That unanswered request is the defect criterion 13 exists to pin.
Gates: fmt; clippy -D warnings; --lib 1863; --lib --features crdt
2048; m4_acceptance 146 (was 132); lsp_dispatch_seams_acceptance 15;
dired_acceptance 25 and autosave_acceptance 29 (the framing's watch
items); PMACS_REQUIRE_GPU=1 -p pmacs-gpu 202; git diff --check clean.
No protocol change.
This commit is contained in:
parent
30750310fc
commit
6b21e93e93
|
|
@ -1298,40 +1298,93 @@ end
|
|||
-- (best-effort: it may itself have been renamed/deleted), on the
|
||||
-- failure path as well as the success path. Returns
|
||||
-- `edit_count, file_count, resource_op_count` on success, or
|
||||
-- `nil, message` if the preflight rejected the edit OR any op failed
|
||||
-- while executing. No exception escapes this function (Q#RD7) — the
|
||||
-- three callers all handle `nil, message` already, and a raise
|
||||
-- reaching them meant an unattended server request went unanswered.
|
||||
-- `nil, message, applied_op_count` if the preflight rejected the edit
|
||||
-- OR any op failed while executing. No exception escapes this function
|
||||
-- (Q#RD7) — the three callers all handle `nil, message` already, and a
|
||||
-- raise reaching them meant an unattended server request went
|
||||
-- unanswered.
|
||||
--
|
||||
-- The third failure value is load-bearing, not decoration: Q#RD3
|
||||
-- permits partial application, so `applied_op_count > 0` means earlier
|
||||
-- plan items ARE still applied and no caller may say otherwise.
|
||||
|
||||
-- Strip trailing separators so a path compares by components.
|
||||
local function strip_trailing_slash(p)
|
||||
while #p > 1 and p:sub(-1) == "/" do p = p:sub(1, #p - 1) end
|
||||
return p
|
||||
end
|
||||
|
||||
-- True when `a` and `b` name the same path, or one lies beneath the
|
||||
-- other. Component-aware, like the Rust side's `Path::starts_with`: a
|
||||
-- raw string prefix would make `/tree` an ancestor of `/tree-sibling`.
|
||||
local function paths_related(a, b)
|
||||
a, b = strip_trailing_slash(a), strip_trailing_slash(b)
|
||||
if a == b then return true end
|
||||
if #a < #b then a, b = b, a end
|
||||
return a:sub(1, #b) == b and (b == "/" or a:sub(#b + 1, #b + 1) == "/")
|
||||
end
|
||||
|
||||
local function apply_workspace_edit(ops)
|
||||
local plan = {}
|
||||
-- Paths an EARLIER op in this same batch creates, renames onto,
|
||||
-- renames away from, or removes. A delete whose target is related to
|
||||
-- one of them cannot be judged from the filesystem's *initial*
|
||||
-- state, which is the only state the plan loop can see.
|
||||
--
|
||||
-- Why defer rather than simulate. Q#RD3 already calls this check a
|
||||
-- FILTER, not a transaction, so declining to judge an op is within
|
||||
-- its contract; refusing a legal batch is not. Simulating instead
|
||||
-- would mean modelling filesystem presence AND the buffer registry's
|
||||
-- path bindings across create/rename/edit — the transaction Q#RD3
|
||||
-- declines to build — and a simulation that got it wrong would
|
||||
-- produce false `clear` verdicts, which is the dangerous direction.
|
||||
-- Deferring only forgoes the early, cheap report; the primitive's
|
||||
-- own four-phase guard is untouched and is what actually stands
|
||||
-- between a server and unsaved work.
|
||||
--
|
||||
-- `edit` ops are deliberately NOT in this set. An edit changes no
|
||||
-- path's existence; it can only dirty a buffer, i.e. only turn a
|
||||
-- plan-time `clear` into a primitive-time refusal. That is the
|
||||
-- under-refusal Q#RD3 documents and accepts, and adding edits here
|
||||
-- would merely delay a refusal that is already certain.
|
||||
local batch_changes = {}
|
||||
local function batch_will_change(path)
|
||||
for _, other in ipairs(batch_changes) do
|
||||
if paths_related(path, other) then return true end
|
||||
end
|
||||
return false
|
||||
end
|
||||
for _, op in ipairs(ops or {}) do
|
||||
if op.op == "edit" then
|
||||
if op.edits and #op.edits > 0 then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end
|
||||
plan[#plan + 1] = { kind = "edit", path = path, edits = op.edits }
|
||||
end
|
||||
elseif op.op == "create" then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end
|
||||
plan[#plan + 1] = {
|
||||
kind = "create", path = path,
|
||||
overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = path
|
||||
elseif op.op == "rename" then
|
||||
local from = pmacs.lsp.path_for_uri(op.old_uri)
|
||||
local to = pmacs.lsp.path_for_uri(op.new_uri)
|
||||
if not from or not to then
|
||||
return nil, "cannot resolve rename " ..
|
||||
tostring(op.old_uri) .. " -> " .. tostring(op.new_uri)
|
||||
tostring(op.old_uri) .. " -> " .. tostring(op.new_uri), 0
|
||||
end
|
||||
plan[#plan + 1] = {
|
||||
kind = "rename", old_path = from, new_path = to,
|
||||
overwrite = op.overwrite, ignore_if_exists = op.ignore_if_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = from
|
||||
batch_changes[#batch_changes + 1] = to
|
||||
elseif op.op == "delete" then
|
||||
local path = pmacs.lsp.path_for_uri(op.uri)
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri) end
|
||||
if not path then return nil, "cannot resolve " .. tostring(op.uri), 0 end
|
||||
-- Delete precondition check (Q#RD3). This is a FILTER, not a
|
||||
-- transaction. It catches, before anything in the batch is
|
||||
-- mutated: a plan-time modified or mid-edit buffer, a known
|
||||
|
|
@ -1349,21 +1402,34 @@ local function apply_workspace_edit(ops)
|
|||
-- uses, so the two layers cannot disagree. `no-op` and `clear`
|
||||
-- both pass: rejecting `no-op` would refuse an op the primitive
|
||||
-- treats as doing nothing.
|
||||
local verdict = pmacs.buffer._delete_verdict {
|
||||
path = path,
|
||||
recursive = op.recursive,
|
||||
ignore_if_not_exists = op.ignore_if_not_exists,
|
||||
}
|
||||
if verdict.kind == "refuse" then return nil, verdict.message end
|
||||
--
|
||||
-- Skipped entirely when an earlier op in this batch can change
|
||||
-- this target (see `batch_changes`). Judging `delete X` against
|
||||
-- the initial filesystem when an earlier `create X` or
|
||||
-- `rename A -> X` has not run yet reports a NotFound that the
|
||||
-- batch itself was about to fix, and refuses a legal edit.
|
||||
if not batch_will_change(path) then
|
||||
local verdict = pmacs.buffer._delete_verdict {
|
||||
path = path,
|
||||
recursive = op.recursive,
|
||||
ignore_if_not_exists = op.ignore_if_not_exists,
|
||||
}
|
||||
if verdict.kind == "refuse" then return nil, verdict.message, 0 end
|
||||
end
|
||||
plan[#plan + 1] = {
|
||||
kind = "delete", path = path,
|
||||
recursive = op.recursive, ignore_if_not_exists = op.ignore_if_not_exists,
|
||||
}
|
||||
batch_changes[#batch_changes + 1] = path
|
||||
end
|
||||
end
|
||||
if #plan == 0 then return 0, 0, 0 end
|
||||
local origin = active_buffer_path()
|
||||
local edit_total, files, res_ops = 0, 0, 0
|
||||
-- Plan items fully applied before a failure. Q#RD3 permits partial
|
||||
-- application, so this is what stops a caller claiming "nothing was
|
||||
-- mutated" when something was.
|
||||
local applied_ops = 0
|
||||
-- Return the user to where they invoked from — best-effort, since
|
||||
-- that path may have just been renamed or deleted. Runs on the
|
||||
-- FAILURE path too (Q#RD7): previously this ran only after a
|
||||
|
|
@ -1390,13 +1456,33 @@ local function apply_workspace_edit(ops)
|
|||
end
|
||||
if not ok then
|
||||
restore_origin()
|
||||
return nil, tostring(err)
|
||||
return nil, tostring(err), applied_ops
|
||||
end
|
||||
applied_ops = applied_ops + 1
|
||||
end
|
||||
restore_origin()
|
||||
return edit_total, files, res_ops
|
||||
end
|
||||
|
||||
-- Render an `apply_workspace_edit` failure for a human or for a
|
||||
-- server's `failureReason`. One renderer for both, so the two cannot
|
||||
-- disagree about what happened.
|
||||
--
|
||||
-- Q#RD3 explicitly permits partial application: an earlier text edit
|
||||
-- can apply and dirty a buffer before a later delete refuses. So
|
||||
-- "nothing was mutated" is a claim about `applied`, not a constant —
|
||||
-- asserting it unconditionally is a false statement about the user's
|
||||
-- files in precisely the case the framing predicted.
|
||||
local function workspace_edit_failure(message, applied)
|
||||
applied = applied or 0
|
||||
if applied > 0 then
|
||||
return string.format(
|
||||
"failed after %d operation%s — those earlier changes remain applied: %s",
|
||||
applied, (applied == 1 and "" or "s"), tostring(message))
|
||||
end
|
||||
return "aborted, nothing was mutated: " .. tostring(message)
|
||||
end
|
||||
|
||||
-- Re-pull a per-`(server, uri)` store for every buffer attached to
|
||||
-- `sid`. Fire-and-forget: the response absorbs into its store via the
|
||||
-- request's route, exactly like the explicit command path — no await
|
||||
|
|
@ -1885,16 +1971,21 @@ local function handle_server_requests()
|
|||
-- being fixed, one line out of scope. The wrap costs
|
||||
-- nothing and makes the boundary uniform regardless of
|
||||
-- which call fails.
|
||||
local ok, a, b = pcall(function()
|
||||
local ok, a, b, c = pcall(function()
|
||||
local parsed = pmacs.lsp._parse_workspace_edit(edit)
|
||||
return apply_workspace_edit(parsed.ops)
|
||||
end)
|
||||
if not ok then
|
||||
reason = a
|
||||
-- A raise from the parse: nothing in the batch ran.
|
||||
reason = workspace_edit_failure(a, 0)
|
||||
elseif a then
|
||||
applied = true
|
||||
else
|
||||
reason = b
|
||||
-- `c` is the count of plan items already applied. The
|
||||
-- server is told so, because `applied = false` alone
|
||||
-- reads as "the workspace is unchanged" and Q#RD3 says
|
||||
-- it need not be.
|
||||
reason = workspace_edit_failure(b, c)
|
||||
end
|
||||
else
|
||||
reason = "missing edit"
|
||||
|
|
@ -2384,8 +2475,13 @@ function pmacs.lsp.rename()
|
|||
end
|
||||
local n, files, res = apply_workspace_edit(ops)
|
||||
if not n then
|
||||
-- Preflight rejected it; nothing was mutated.
|
||||
pmacs.editor.set_status("LSP: rename aborted: " .. tostring(files))
|
||||
-- On failure the second value is the message and the third
|
||||
-- is how many plan items already applied. It is NOT always
|
||||
-- zero (Q#RD3), so this must not say "nothing was mutated"
|
||||
-- unconditionally — that was false in exactly the
|
||||
-- edit-then-delete case the framing predicted.
|
||||
pmacs.editor.set_status(
|
||||
"LSP: rename " .. workspace_edit_failure(files, res))
|
||||
return
|
||||
end
|
||||
local msg = string.format(
|
||||
|
|
@ -2446,7 +2542,11 @@ local function apply_code_action(rec, act)
|
|||
if act.has_edit then
|
||||
local n, files, res = apply_workspace_edit(act.edit)
|
||||
if not n then
|
||||
pmacs.editor.set_status("LSP: code action aborted: " .. tostring(files))
|
||||
-- Same failure shape as the rename caller: `files` is the
|
||||
-- message, `res` the applied-op count (Q#RD3 permits partial
|
||||
-- application, so it can be non-zero).
|
||||
pmacs.editor.set_status(
|
||||
"LSP: code action " .. workspace_edit_failure(files, res))
|
||||
return
|
||||
end
|
||||
local b = string.format("%d edit(s) / %d file(s)", n, files)
|
||||
|
|
|
|||
|
|
@ -521,55 +521,105 @@ has **no branch and no framing yet**.
|
|||
`FrontendView.fold_projection` to `true` for semantic frontends, which
|
||||
Stage 2 deliberately left `false` (Q#FD21).
|
||||
|
||||
## Resource-op delete guard implementation — PR OPEN, PARTIAL
|
||||
## Resource-op delete guard implementation — PR #190 OPEN, review round 1 closed
|
||||
|
||||
- Portable branch: `githubsucks/resource-op-delete-guard-impl`, worktree
|
||||
`../pmacs-rd-impl`, based on `main` @ `300cbc4`. Implements the
|
||||
framing merged as #186 (`docs/resource-op-delete-guard-framing.md`,
|
||||
revision 5).
|
||||
`../pmacs-rd-impl`. Implements the framing merged as #186
|
||||
(`docs/resource-op-delete-guard-framing.md`, revision 5 plus its new
|
||||
§9). Position against `main`, as pasted command output rather than a
|
||||
remembered constant — **`main` moved while this lane was being
|
||||
written**:
|
||||
|
||||
```
|
||||
$ git merge-base HEAD githubsucks/main
|
||||
64883ebe0c1785b8188d2dee7c8e6f8ea4518512
|
||||
$ git rev-list --left-right --count HEAD...githubsucks/main
|
||||
2 0
|
||||
```
|
||||
|
||||
Re-measure before quoting this anywhere: `main` has branch protection
|
||||
now, and #193 was open behind #192 when this was written.
|
||||
- **The framing's §8 branch plan is superseded and cannot be followed.**
|
||||
It says "one PR — #186, which becomes the implementation PR", written
|
||||
when #186 was still open. #186 merged as framing-only, so the
|
||||
implementation necessarily gets its own branch and PR. Nothing about
|
||||
the decisions changes; only the branch plan.
|
||||
- **Layer 1 (the primitive) is complete and tested.** The delete arm is
|
||||
four ordered phases; `delete_verdict` is the single shared query, used
|
||||
by the primitive and exposed to Lua as `pmacs.buffer._delete_verdict`
|
||||
so the two layers cannot drift.
|
||||
- **Layer 2 (the applier + server-request boundary) is implemented but
|
||||
NOT yet covered.** `builtin/runtime/lsp.lua` has the plan-time
|
||||
preflight, the parse-plus-apply wrap, origin restore on the failure
|
||||
path, and the `*errors*` trace. Criteria 11-15 exercise those through
|
||||
a real server pump and need new `pmacs_fake_lsp` modes that do not
|
||||
exist yet. **Criterion 13 explicitly rejects a direct-call test as
|
||||
insufficient**, so this is a real gap, not a formality: today the
|
||||
Layer 2 code has no production-path pin.
|
||||
- Acceptance status: criteria **1-10, 14 and 16 land here** (11 tests in
|
||||
`tests/m4_acceptance.rs`, prefixed `rd`). Criteria **11, 11a-11d, 12,
|
||||
13, 15 do not** — they are the fake-LSP modes above.
|
||||
- **Criterion 3's stated bite in the framing is wrong**, found by
|
||||
checking rather than trusting it. The framing says it fails against
|
||||
buffer-first ordering; it does not, because the deleted path is a
|
||||
directory no buffer is bound to, so reconciliation never fires on
|
||||
that input. It *does* fail against validation that removes rather
|
||||
than inspects — verified by mutation. The test comment carries the
|
||||
correction; the framing wants amending on its next revision.
|
||||
- Bite verification: the five refusal criteria (1, 5, 6, 8, 10) fail
|
||||
against `githubsucks/main` via `scripts/bite`. Criteria 3 and 4 pin
|
||||
phase *ordering* against designs never committed, so `main` cannot
|
||||
falsify them; both were verified by hand mutation instead (4 catches
|
||||
buffer-first ordering, 3 catches removing-validation). Criteria 2, 7,
|
||||
9 and 14 assert preserved or deliberately-unchanged behaviour and are
|
||||
expected to pass against `main` — that is what they are for.
|
||||
- Gates green at this tree: fmt; clippy `-D warnings`; `--lib` **1863**;
|
||||
`--lib --features crdt` **2048**; `m4_acceptance` **132**;
|
||||
`lsp_dispatch_seams_acceptance` **15**; `dired_acceptance` **25** and
|
||||
`autosave_acceptance` **29** (framing watch items); required GPU
|
||||
**202**; `git diff --check`.
|
||||
implementation got its own branch and PR. Nothing about the decisions
|
||||
changes; only the branch plan. Both the framing's header and its §8 now
|
||||
say so on their own pages.
|
||||
- **Layer 1 (the primitive) and Layer 2 (the applier + server-request
|
||||
boundary) are both complete and both pinned through their production
|
||||
paths.** The Layer 2 gap the first commit named — criteria 11, 11a-11d,
|
||||
12, 13, 15 having no production-path pin — is closed.
|
||||
- **Review round 1 found four defects; all four are fixed and all four
|
||||
are recorded in the framing's new §9**, because two of them were
|
||||
corrections *to that document*, and a correction living only in a test
|
||||
comment is invisible to the next reader of the framing:
|
||||
- **P1 §9.3 — the preflight broke ordered resource ops.** Every delete
|
||||
was judged against the filesystem's *initial* state, so a valid
|
||||
`create X -> delete X` (or `rename A -> B -> delete B`) was refused
|
||||
with a fabricated `NotFound` about a path the batch was about to
|
||||
create. A regression this lane introduced. **Decision: defer, do not
|
||||
simulate** — a delete whose target is related by component-aware path
|
||||
containment to a path an *earlier* op creates, renames, or removes is
|
||||
left to the primitive. Q#RD3 already calls the check a filter, not a
|
||||
transaction. `edit` ops are deliberately excluded, so the
|
||||
buffer-and-filesystem half still fires early for untouched targets
|
||||
(criterion 11c depends on exactly that).
|
||||
- **P1 §9.5 — the required production-boundary acceptances were
|
||||
missing.** Landed: 11, 11a-11d, 12 (both directions), 13, 15.
|
||||
- **P1 §9.4 — mid-batch failures were misreported as complete aborts.**
|
||||
`apply_workspace_edit` now returns `nil, message, applied_op_count`,
|
||||
and ONE renderer serves both the status line and the server's
|
||||
`failureReason`. All three callers updated.
|
||||
- **P2 §9.2 — non-recursive deletes inspected descendants.** `recursive`
|
||||
is now a parameter of the shared query. The counterexample is an
|
||||
orphan: a modified buffer at `tree/gone.rs` whose file is already gone
|
||||
blocked a non-recursive delete of the now-*empty* `tree/`.
|
||||
- **`delete_verdict` is narrowed, and #171 inherits the narrowed
|
||||
version.** Q#RD6's shared query is this lane's to own; descendant
|
||||
matching is now reserved for recursive deletes. Q#RD5's "inspect widely,
|
||||
mutate narrowly" is unchanged in substance — "widely" means the set the
|
||||
op can actually destroy.
|
||||
- **Criterion 3's stated bite: fixed by fixing the SETUP, not the doc.**
|
||||
The framing says it fails against buffer-first ordering. Against the
|
||||
first shipped setup it did not (a directory target with no buffer bound
|
||||
to it), and §9.2's narrowing would then have left that setup with no
|
||||
bite at all. The buffer is now bound to the *exact* deleted path — a
|
||||
file opened, then replaced on disk by a non-empty directory, so a
|
||||
non-recursive `remove_dir` fails with `ENOTEMPTY` deterministically and
|
||||
under any uid. Both stated pre-images now bite, so the framing's wording
|
||||
needed no amendment after all.
|
||||
- **The fake is one parameterized mode, not eight.**
|
||||
`PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit` from
|
||||
`PMACS_FAKE_LSP_EDIT_PLAN` and publishes the client's response to
|
||||
`PMACS_FAKE_LSP_APPLYEDIT_SINK` (written `.part`-then-rename, so a
|
||||
polling reader never sees a partial record). Fail-closed: an unreadable
|
||||
plan sends no `applyEdit` and reports itself through the sink.
|
||||
`pmacs_fake_lsp` is a cargo BIN resolved through
|
||||
`env!("CARGO_BIN_EXE_...")`, so every CI leg builds it and a missing
|
||||
binary is a build failure — there is deliberately no
|
||||
skip-and-return-ok arm.
|
||||
- **Criterion 15's stub is hosted in `m4_acceptance`, and the gate list
|
||||
moved with it.** `lsp_dispatch_seams_acceptance` is struck from the
|
||||
framing's §7 gate list AND its §8 touch table in the same edit, under
|
||||
§8's permitted simplification. It is still *run* as a gate, because
|
||||
`builtin/runtime/lsp.lua` changed.
|
||||
- Acceptance: criteria 1-16 plus §9's 18, 19a-19c and 20, all in
|
||||
`tests/m4_acceptance.rs` and prefixed `rd`. 28 tests.
|
||||
- Bite verification uses `scripts/bite` **with the positive control** it
|
||||
gained in #192, merged into this lane. The pre-image for the round-1
|
||||
fixes is this lane's own first commit `1873be6`, not `main` — those
|
||||
defects were introduced by it. Per-criterion results are in the commit
|
||||
message.
|
||||
- Gates green at the pushed tree: fmt; clippy `-D warnings`; `--lib`
|
||||
**1863**; `--lib --features crdt` **2048**; `m4_acceptance` **146**
|
||||
(was 132); `lsp_dispatch_seams_acceptance` **15**; `dired_acceptance`
|
||||
**25** and `autosave_acceptance` **29** (the framing's watch items);
|
||||
required GPU **202**; `git diff --check` clean.
|
||||
- Recovery from a clean checkout:
|
||||
`git fetch githubsucks && git worktree add ../pmacs-rd-impl
|
||||
-b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`.
|
||||
|
||||
|
||||
## Test-improvement arc, lane 6 — `scripts/bite` positive control
|
||||
|
||||
- Portable branch: `githubsucks/bite-positive-control`, worktree
|
||||
|
|
|
|||
|
|
@ -1,9 +1,19 @@
|
|||
# Framing — `apply_resource_op` delete destroys unsaved work
|
||||
|
||||
**Revision 5.** Status: **PROPOSED — needs explicit user approval before
|
||||
implementation. DO NOT implement, DO NOT merge.** Lane:
|
||||
`resource-op-delete-guard`, worktree `../pmacs-resource-op-delete`,
|
||||
based on `githubsucks/main` @ `7586905`.
|
||||
**Revision 5, plus §9.** Status: **APPROVED and MERGED as #186
|
||||
(framing only); the implementation is PR #190** on branch
|
||||
`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`. The
|
||||
revision-5 body below is unchanged except for the two bookkeeping
|
||||
edits §9.6 names and makes in place; **§9 records the corrections
|
||||
implementation review round 1 found**, including two corrections to
|
||||
this document. The "DO NOT implement, DO NOT merge" banner this line
|
||||
replaces was true when revision 5 was written and is not now.
|
||||
|
||||
Revision 5's lane header — `resource-op-delete-guard`, worktree
|
||||
`../pmacs-resource-op-delete`, based on `githubsucks/main` @
|
||||
`7586905` — describes the framing branch, which merged. §8's
|
||||
one-PR-for-both branch plan is superseded for the same reason and is
|
||||
annotated there.
|
||||
|
||||
Revision 5 removes volatile sibling-branch counts from the normative
|
||||
contract. A count is a reading, not a dependency; where history retains
|
||||
|
|
@ -1533,6 +1543,11 @@ that passes against its pre-image has no bite and is rejected.
|
|||
17. **Every new test is checked with `scripts/bite`** and none reports
|
||||
VACUOUS.
|
||||
|
||||
**Criteria 18, 19a–19c and 20 are added by §9**, after implementation
|
||||
review round 1. They are listed there, with their pre-images, rather
|
||||
than interleaved here, so this section stays readable as the record of
|
||||
what revision 5 asked for.
|
||||
|
||||
|
||||
## 6. Parked — not deferred-and-forgotten
|
||||
|
||||
|
|
@ -1583,12 +1598,18 @@ Full suite per `CLAUDE.md`: `cargo fmt --check`; `cargo clippy
|
|||
suites; `cargo test --test m4_acceptance -- --skip basedpyright`;
|
||||
`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; `git diff --check`.
|
||||
|
||||
Touched suites: **`m4_acceptance`** (the resource-op home, §1.14, and
|
||||
the home of criteria 1–14 including 11a–11d, and 16) and
|
||||
**`lsp_dispatch_seams_acceptance`**
|
||||
(criterion 15's throwing parse stub, Q#RD11). Both appear in §8's touch
|
||||
table; revision 3 named the second here but omitted it there, and the
|
||||
two lists are now maintained together.
|
||||
Touched suite: **`m4_acceptance`** — the resource-op home (§1.14) and
|
||||
the home of every criterion, 1–16 including 11a–11d, plus §9's 18,
|
||||
19a–19c and 20.
|
||||
|
||||
*Amended at implementation (§9.6).* Revision 5 also named
|
||||
**`lsp_dispatch_seams_acceptance`**, for criterion 15's throwing parse
|
||||
stub. §8 permits hosting that stub in `m4_acceptance` instead
|
||||
**provided the gate list moves with it**, and it does: criterion 15
|
||||
drives the same server pump as 11–13, so splitting it across two
|
||||
suites would have duplicated the whole fixture. The suite is therefore
|
||||
struck from this list **and** from §8's touch table, in the same edit.
|
||||
The lists are maintained together, which is what revision 3 got wrong.
|
||||
|
||||
`dired_acceptance` and `autosave_acceptance` are watch items, not
|
||||
touched files — the former for Q#RD6's shared lookup, the latter because
|
||||
|
|
@ -1612,6 +1633,13 @@ approved the implementation commits land on this same branch.
|
|||
|
||||
**Implementation does not begin until the user approves this revision.**
|
||||
|
||||
**Superseded in fact, not by decision.** #186 merged as framing-only,
|
||||
so the implementation necessarily got its own branch
|
||||
(`resource-op-delete-guard-impl`, worktree `../pmacs-rd-impl`) and its
|
||||
own PR, #190. No decision in this document changes; only the branch
|
||||
plan, which described a PR that no longer existed by the time
|
||||
implementation started.
|
||||
|
||||
**Files the implementation will touch** — reconciled at rev 5 against
|
||||
the gate list below and §5, which revision 3 left disagreeing:
|
||||
|
||||
|
|
@ -1619,9 +1647,18 @@ the gate list below and §5, which revision 3 left disagreeing:
|
|||
|---|---|
|
||||
| `src/lua_bindings/mod.rs` | the delete arm's four phases (Q#RD2); the shared query binding and its structured verdict (Q#RD6, Q#RD12); the narrow `*errors*` append surface (Q#RD7) |
|
||||
| `builtin/runtime/lsp.lua` | the preflight conflict check (Q#RD3); the parse-plus-apply wrap, origin restore, and boundary logging (Q#RD7) |
|
||||
| `tests/m4_acceptance.rs` | criteria 1–14 including 11a–11d, and 16 |
|
||||
| `tests/lsp_dispatch_seams_acceptance.rs` | criterion 15's throwing parse stub (Q#RD11) — this file was named in the gate list but omitted from revision 3's touch list |
|
||||
| `src/bin/pmacs_fake_lsp.rs` | fake modes: blocked delete; edit-then-delete; rename-into-delete; absent-plus-ignore; present-plus-ignore (11a); dangling-symlink (11b); absent-without-ignore (11c); unanswerable-stat (11d) |
|
||||
| `tests/m4_acceptance.rs` | criteria 1–16 including 11a–11d, plus §9's 18, 19a–19c and 20 — **including criterion 15's throwing parse stub**, per the permitted simplification below (§9.6) |
|
||||
| `src/bin/pmacs_fake_lsp.rs` | **one parameterized mode, `applyeditplan`**, whose `WorkspaceEdit` is read from a test-written file, plus a sink for the client's response — see §9.6 for why one mode replaced the eight named below |
|
||||
|
||||
~~`tests/lsp_dispatch_seams_acceptance.rs`~~ — struck at implementation
|
||||
(§9.6), together with its entry in §7's gate list.
|
||||
|
||||
The eight fake modes revision 5 named — blocked delete;
|
||||
edit-then-delete; rename-into-delete; absent-plus-ignore;
|
||||
present-plus-ignore (11a); dangling-symlink (11b);
|
||||
absent-without-ignore (11c); unanswerable-stat (11d) — are the eight
|
||||
*fixtures*, and they all still exist. They are payloads now, not modes
|
||||
(§9.6).
|
||||
|
||||
It will **not** touch `src/daemon.rs`, `pmacs-protocol/`,
|
||||
`builtin/runtime/dired.lua`, `docs/agent-handoff.md` or `COHERENCE.md`.
|
||||
|
|
@ -1647,3 +1684,194 @@ reconciliation, the dired async race between dispatch and
|
|||
`remove_blocking`, the rename side of the walk, or `pmacs.fs.remove`
|
||||
(§6). Revision 2's version of this note was written against a stale
|
||||
reading of #171 and is superseded.
|
||||
|
||||
|
||||
## 9. Corrections found during implementation — review round 1
|
||||
|
||||
This section is written **after** revision 5 was approved and
|
||||
implemented, and it changes no design decision. It records four
|
||||
corrections review round 1 found in the implementation, the acceptance
|
||||
criteria they added, and two bookkeeping edits made in place above.
|
||||
It lives here because two of the four are corrections *to this
|
||||
document*, and a correction that lives only in a test comment is
|
||||
invisible to the next reader of the framing.
|
||||
|
||||
### 9.1 Criterion 3's stated bite was wrong — fixed by fixing the setup
|
||||
|
||||
Criterion 3 says it "fails against revision 1's buffer-first
|
||||
ordering". Against the first shipped setup it did **not**, and the
|
||||
claim was found by checking rather than trusting it: that setup bound
|
||||
the clean buffer to a file *beneath* the deleted directory, so no
|
||||
buffer was bound to the deleted path, `find_by_path` matched nothing,
|
||||
and reordering reconciliation ahead of the filesystem mutation left
|
||||
the test passing.
|
||||
|
||||
The first fix considered was to amend the criterion's wording.
|
||||
**That is not what was done.** §9.2 then narrowed the affected set to
|
||||
recursive deletes only, which would have left that setup with *no*
|
||||
bite at all — the pre-image it did catch (validation that removes
|
||||
rather than inspects) stops touching a descendant buffer on a
|
||||
non-recursive delete. So the **setup** is what changed: the buffer is
|
||||
now bound to the **exact** deleted path. A file is opened, the path is
|
||||
then replaced on disk by a non-empty directory, and a non-recursive
|
||||
`remove_dir` fails deterministically with `ENOTEMPTY` — no permission
|
||||
trickery and nothing that behaves differently under a root CI.
|
||||
|
||||
Against that setup the criterion fails against **both** pre-images,
|
||||
which is what revision 5 claimed all along. **The framing's wording
|
||||
needed no amendment; the test did.** Both directions verified by
|
||||
mutation.
|
||||
|
||||
### 9.2 The affected set is scoped by `recursive` (Q#RD12, Q#RD6)
|
||||
|
||||
The shipped `delete_verdict` ignored `recursive` and scanned
|
||||
descendants for **every** directory target, justified in a doc comment
|
||||
on the grounds that a non-recursive delete of a non-empty directory
|
||||
fails at the filesystem anyway, so widening inspection cost nothing.
|
||||
|
||||
**That reasoning is wrong**, and the counterexample is an orphan: a
|
||||
modified buffer at `tree/gone.rs` whose file is already deleted blocks
|
||||
a non-recursive delete of the now-**empty** `tree/` — an op that would
|
||||
have succeeded and that removes none of that buffer's contents.
|
||||
Reproduced in review.
|
||||
|
||||
`recursive` is therefore a parameter of the shared query, and
|
||||
descendant matching is reserved for recursive deletes. This
|
||||
**narrows** the query Q#RD6 hands to #171, which adopts the narrowed
|
||||
version. Q#RD5's "inspect widely, mutate narrowly" is unchanged in
|
||||
substance: "widely" means *the set the op can actually destroy*, which
|
||||
for a non-recursive delete is the target entry alone.
|
||||
|
||||
A symlink to a directory is correctly excluded by the same rule:
|
||||
`symlink_metadata` reports it as not-a-directory, and the primitive
|
||||
`remove_file`s the link without walking through it.
|
||||
|
||||
### 9.3 The preflight defers for targets the batch itself changes (Q#RD3)
|
||||
|
||||
The shipped preflight judged **every** delete against the filesystem's
|
||||
**initial** state, at plan-construction time. A valid `create X →
|
||||
delete X` batch was therefore rejected because X was absent when the
|
||||
plan was built, with a fabricated `NotFound` about a path the batch
|
||||
was about to create; likewise `rename A → B → delete B`. This was a
|
||||
regression introduced by the implementation, not a pre-existing
|
||||
defect.
|
||||
|
||||
**Decision — defer, do not simulate.** A delete whose target is
|
||||
related by path containment to a path an **earlier** op in the same
|
||||
plan creates, renames onto, renames away from, or removes is not
|
||||
judged at plan time; the primitive judges it when it runs. Comparison
|
||||
is component-aware, like the Rust side's `Path::starts_with`.
|
||||
|
||||
Why this is the right half of the choice Q#RD3 already made:
|
||||
|
||||
- Q#RD3 calls this check a **filter, not a transaction**. Declining to
|
||||
judge an op the snapshot cannot see is inside that contract;
|
||||
refusing a legal batch is not.
|
||||
- Simulating instead would mean modelling filesystem presence **and**
|
||||
the buffer registry's path bindings across create / rename / edit —
|
||||
the transaction Q#RD3 declines to build — and a simulation that got
|
||||
it wrong would emit false `clear` verdicts, which is the dangerous
|
||||
direction. Deferral only forgoes the early, cheap report.
|
||||
- The primitive's four-phase guard is untouched and is the thing that
|
||||
actually stands between a server and unsaved work. Criterion 19c
|
||||
pins that deferring is not skipping.
|
||||
|
||||
**`edit` ops are deliberately not in the deferral set.** An edit
|
||||
changes no path's existence; it can only dirty a buffer, i.e. only
|
||||
turn a plan-time `clear` into a primitive-time refusal. That is the
|
||||
under-refusal Q#RD3 documents and accepts, and adding edits would
|
||||
merely delay a refusal that is already certain. Criterion 11c depends
|
||||
on this: its delete target is touched by no earlier op, so the
|
||||
buffer-and-filesystem half of the check still fires before anything is
|
||||
mutated.
|
||||
|
||||
### 9.4 Failure reporting must not deny partial application (Q#RD3, Q#RD7)
|
||||
|
||||
`apply_workspace_edit` discarded, on failure, whether earlier ops had
|
||||
succeeded, and returned a bare `nil, message`. The rename caller then
|
||||
said "rename aborted" under a comment reading "nothing was mutated" —
|
||||
false in exactly the case Q#RD3 predicts, where an earlier text edit
|
||||
applies and dirties the buffer a later delete refuses.
|
||||
|
||||
The applier now returns `nil, message, applied_op_count`, and **one
|
||||
renderer** serves both the user-facing status line and the server's
|
||||
`failureReason`, so the two cannot disagree about what happened. All
|
||||
three callers are updated: the server-request boundary, the rename
|
||||
caller, and the code-action caller.
|
||||
|
||||
### 9.5 Acceptance added by this round
|
||||
|
||||
18. **A non-recursive delete is not blocked by a buffer beneath its
|
||||
target** (§9.2). Modified buffer at `tree/gone.rs` whose file is
|
||||
already gone; non-recursive delete of the now-empty `tree/`
|
||||
succeeds and the buffer is untouched.
|
||||
*Bite:* fails against a `delete_verdict` that ignores `recursive`.
|
||||
|
||||
19a. **`create X → delete X` is not refused at plan time** (§9.3).
|
||||
Assert `applied = true`, and that a later `create` in the same
|
||||
batch produced its file, so the success is not vacuous.
|
||||
*Bite:* fails against the initial-state preflight, which reports
|
||||
`NotFound` for X and rejects the batch before anything runs.
|
||||
|
||||
19b. **`rename A → B → delete B` is not refused at plan time** (§9.3).
|
||||
*Bite:* as 19a; the source file surviving is what carries it,
|
||||
because a plan-time rejection leaves the rename unapplied.
|
||||
|
||||
19c. **Deferring the check is not skipping it** (§9.3).
|
||||
`create X → edit X → delete X` gets past the plan and then refuses
|
||||
at the primitive, naming the unsaved changes the edit created —
|
||||
and reports that two operations remain applied.
|
||||
*Bite:* fails against the initial-state preflight (which reports
|
||||
`NotFound` instead) **and** against dropping the primitive's guard
|
||||
for deferred targets (which would report `applied = true`).
|
||||
|
||||
20. **The user-facing message reports partial application** (§9.4).
|
||||
Driven through `M-x lsp.rename`, because a status line is where a
|
||||
user reads it and the applier's return value alone would not pin
|
||||
the caller.
|
||||
*Bite:* fails against any caller that renders the failure without
|
||||
consulting the applied-op count — i.e. against the shipped
|
||||
"rename aborted".
|
||||
|
||||
All of 11, 11a–11d, 12, 13 and 15 also land in this round; they were
|
||||
specified by revision 5 and were the named gap in the first
|
||||
implementation commit.
|
||||
|
||||
### 9.6 Bookkeeping edits made in place
|
||||
|
||||
- **§7's gate list and §8's touch table both lose
|
||||
`lsp_dispatch_seams_acceptance`**, in the same edit, under §8's
|
||||
permitted simplification. Criterion 15 drives the same server pump
|
||||
as 11–13, so hosting it anywhere else would duplicate the fixture.
|
||||
- **§8's fake-mode row is one parameterized mode, not eight.**
|
||||
`PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit`
|
||||
from the file named by `PMACS_FAKE_LSP_EDIT_PLAN` and publishes the
|
||||
client's response to `PMACS_FAKE_LSP_APPLYEDIT_SINK`. The eight
|
||||
fixtures revision 5 named all exist; they are payloads the test
|
||||
writes rather than modes the fake hardcodes, which keeps each
|
||||
payload next to the assertions that depend on it instead of mirrored
|
||||
across two files. The mode is **fail-closed**: an unreadable or
|
||||
unparsable plan sends no `applyEdit` and reports itself through the
|
||||
sink, so a broken fixture cannot read as a pass. The sink is written
|
||||
to a `.part` and renamed, so a polling reader never sees a partial
|
||||
record — the wait predicate cannot be weaker than the assertion.
|
||||
|
||||
### 9.7 Sweep — every place the guard decides something is "affected"
|
||||
|
||||
§9.2 and §9.3 are one defect class: **a guard whose scope was reasoned
|
||||
about rather than enumerated**, over-refusing on inputs the reasoning
|
||||
never considered. The rest of the guard was swept for that shape.
|
||||
|
||||
| Site | Decision | Verdict |
|
||||
|---|---|---|
|
||||
| `delete_verdict` filesystem classification | is the target a directory whose descendants are at risk? | **Fixed** (§9.2). Now `is_dir() && recursive`. A symlink-to-directory is excluded, matching the primitive's `remove_file`. |
|
||||
| `delete_verdict` buffer matching | which buffers are in the affected set? | **Fixed** (§9.2), exact path always plus descendants only when recursive. |
|
||||
| Lua plan-time preflight | which deletes can be judged from the initial snapshot? | **Fixed** (§9.3). |
|
||||
| `delete_verdict` path normalization | the stat uses the **raw** path; the buffer comparison uses the **normalized** one | **Latent inconsistency, fails safe, not fixed here.** A `~`-prefixed argument stats as a literal `~` directory (absent) while matching buffers bound under `$HOME`. Every branch of that disagreement is safe: without `ignore_if_not_exists` it refuses, and with it the primitive returns early having touched nothing. It matches the primitive's own `remove_file`, which also takes the raw path, so the two layers still agree with each other. |
|
||||
| Phase 4 reconciliation | which buffer is removed after a successful delete? | **Unchanged by design** (Q#RD10). `BufferRegistry::find_by_path` compares paths **raw**, with no normalization, so a buffer stored under a differently-spelled path is not reconciled. This is `main`'s behaviour, Q#RD10 pins "exactly today's", and correcting it would *widen* reconciliation — the one thing Q#RD5 and Q#RD10 forbid. Named so it is not mistaken for an oversight; it belongs to #171. |
|
||||
| `_delete_verdict` argument handling | `recursive` / `ignore_if_not_exists` defaults | **Consistent.** Both default to `false` on the binding and on the primitive, so an omitted `options` object means the same thing at both layers. |
|
||||
| Preflight `batch_changes` membership | which prior ops can change a delete's target? | **Enumerated, not reasoned:** create (1 path), rename (both paths), delete (1 path); edit excluded with the argument in §9.3. |
|
||||
|
||||
**Nothing else in this lane decides an affected set.** The reporting
|
||||
path names a buffer only inside a refusal it already computed, and
|
||||
`restore_origin` is best-effort by construction.
|
||||
|
|
|
|||
|
|
@ -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{}");
|
||||
|
|
|
|||
|
|
@ -1627,18 +1627,30 @@ enum DeleteVerdict {
|
|||
/// one as absent, which is the single input on which the two disagree
|
||||
/// and exactly the input `ignore_if_not_exists` turns on.
|
||||
///
|
||||
/// `recursive` is deliberately not a parameter. Inspection is
|
||||
/// prefix-aware whenever the target is a directory, because a
|
||||
/// non-recursive delete of a non-empty directory fails at the
|
||||
/// filesystem anyway — so widening inspection there costs nothing and
|
||||
/// narrowing it would leave the recursive arm's bypass reachable.
|
||||
/// **Descendant matching is reserved for `recursive` deletes.** The
|
||||
/// affected set is what the op can actually destroy: a non-recursive
|
||||
/// delete removes the target entry and nothing beneath it, so a buffer
|
||||
/// under the target is not at risk and must not refuse the op. The
|
||||
/// earlier revision of this helper ignored `recursive` and scanned
|
||||
/// descendants for every directory, on the reasoning that a
|
||||
/// non-recursive delete of a *non-empty* directory fails at the
|
||||
/// filesystem anyway. That reasoning was wrong, and the counterexample
|
||||
/// is an orphan: a modified buffer at `tree/gone.rs` whose file is
|
||||
/// already deleted blocks a non-recursive delete of the now-**empty**
|
||||
/// `tree/`, which would have succeeded and would have removed none of
|
||||
/// that buffer's contents.
|
||||
fn delete_verdict(
|
||||
reg: &BufferRegistry,
|
||||
path: &std::path::Path,
|
||||
recursive: bool,
|
||||
ignore_if_not_exists: bool,
|
||||
) -> DeleteVerdict {
|
||||
let is_dir = match std::fs::symlink_metadata(path) {
|
||||
Ok(md) => md.is_dir(),
|
||||
let scan_descendants = match std::fs::symlink_metadata(path) {
|
||||
// A symlink to a directory reports `is_dir() == false` here, and
|
||||
// that is correct: the primitive `remove_file`s the link and
|
||||
// never walks through it, so nothing beneath the link's target
|
||||
// is at risk either.
|
||||
Ok(md) => md.is_dir() && recursive,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
|
||||
return if ignore_if_not_exists {
|
||||
DeleteVerdict::NoOp
|
||||
|
|
@ -1666,7 +1678,7 @@ fn delete_verdict(
|
|||
continue;
|
||||
};
|
||||
let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf());
|
||||
if bound != target && !(is_dir && bound.starts_with(&target)) {
|
||||
if bound != target && !(scan_descendants && bound.starts_with(&target)) {
|
||||
continue;
|
||||
}
|
||||
// "Modified" is `Buffer::is_modified()`. No new notion of
|
||||
|
|
@ -3442,7 +3454,8 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
|||
|
||||
// Phases 1 and 2, as one shared query.
|
||||
let md = {
|
||||
let verdict = delete_verdict(®.borrow(), &pb, ignore_if_not_exists);
|
||||
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
|
||||
|
|
@ -3518,10 +3531,16 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
|||
"_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, ignore_if_not_exists) {
|
||||
match delete_verdict(®.borrow(), &pb, recursive, ignore_if_not_exists) {
|
||||
DeleteVerdict::NoOp => out.set("kind", "no-op")?,
|
||||
DeleteVerdict::Clear => out.set("kind", "clear")?,
|
||||
DeleteVerdict::Refuse {
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load Diff
Loading…
Reference in New Issue