neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merge: integrate main @ e003b81 (PR #190) into the Stage 1 lane 

One conflict, `docs/active-work.md`, with three strands rather than the
usual one: main gained #190's lane, this branch carries its own Stage 1
lane and a relabel of the #188 framing lane, and main had removed the
documentation lane while this branch still had it.

Resolved by construction. Main's file taken whole; this branch's Stage
1 lane reinserted at its own position ahead of the bottom-panel lane;
this branch's relabelled framing lane ("MERGED AS PR #188") kept in
place of main's stale "OPEN, PROPOSED" version; main's removal of the
documentation lane preserved.

Verified against both parents rather than by inspection: the Stage 1
block is byte-identical to this branch's, the documentation lane is
gone, no conflict markers survive, and the update-protocol rule 6 seam
check finds no double blanks.

Note for whoever absorbs next: main now carries three lanes describing
merged PRs (#190, #188, #194). This merge keeps this branch's more
accurate labelling of the #188 one but does not remove any of them ---
rule 4 permits removal only once durable facts reach
`docs/agent-handoff.md`, and none of those three PRs touched it.
This commit is contained in:
Levi Neuwirth 2026-07-29 15:12:13 -04:00
parent a07767d310 e003b81cdd
commit 7350cfdd8a
11 changed files with 3061 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
.github/workflows/ci.yml vendored
View File

@ -85,12 +85,69 @@ jobs:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- uses: Swatinem/rust-cache@v2
# Arm the external-tool-gated tests (see `TEST_IMPROVEMENT.md`
# §1.2). Before this step nothing installed these tools, so every
# test guarded on them returned early and reported GREEN without
# executing its body — a whole block of real-language-server and
# multi-shell coverage that had never once run in CI. Installing
# them is only half the fix; the `PMACS_REQUIRE_*` variables below
# are what turn a future missing tool back into a failure instead
# of silently restoring the vacuum.
#
# Linux only for now, deliberately. macOS would need the brew
# equivalents and roughly doubles the install cost on the slowest
# leg of the matrix; arming one platform already converts these
# from never-executed to executed, and the second is incremental.
# The tests still skip cleanly on macOS because the variables are
# unset there.
- name: Install external tools that gate acceptance tests (Linux)
if: runner.os == 'Linux'
run: |
sudo apt-get update
sudo apt-get install -y clangd zsh fish lua5.4
# `locate_lua` looks for `lua` or `luajit` by name; the
# distro package installs `lua5.4` only.
sudo ln -sf "$(command -v lua5.4)" /usr/local/bin/lua
# rust-analyzer belongs HERE, not on the shared toolchain
# step. `components:` there applies to every matrix leg, so it
# would install the binary on macOS too — and *presence*, not
# PMACS_REQUIRE_LSP, is what decides whether a gated test body
# runs. That would have executed the rust-analyzer tests on
# macOS for the first time ever, on the legs that are both the
# CI critical path and the documented flake surface, while
# this lane's text claimed Linux only.
rustup component add rust-analyzer
# Versions are PINNED. `@latest` and bare `npm install -g`
# make CI behaviour drift with upstream releases: a bad gopls
# or yaml-language-server publish then breaks CI with no
# commit in this repository to bisect against.
go install golang.org/x/tools/gopls@v0.16.2
echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH"
npm install -g vscode-langservers-extracted@4.10.0 \
yaml-language-server@1.15.0
- run: cargo build --all-targets --no-default-features --features ${{ matrix.lua }}
# Several acceptance binaries spawn real daemon / PTY child
# processes. Keep the harness serial so macOS runners do not
# expose cross-test process lifecycle races that are unrelated
# to the behavior under test.
#
# PMACS_REQUIRE_* make a missing tool fatal rather than a silent
# skip, exactly as PMACS_REQUIRE_GPU already does for the headless
# render job. Set only where the install step ran.
#
# PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright
# is deliberately NOT installed: that test has no timeout and
# hangs forever (root cause is the non-interruptible reader-thread
# join in `RuntimeHandles::drop`, already a named deferral in
# `src/process.rs`). This job has no `timeout-minutes`, so arming
# it today would trade a vacuous green for a six-hour hang on four
# legs. It gets armed after the hang fix and the CI timeouts land,
# and its own variable exists so that flip is one line.
- run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1
env:
PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }}
PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }}
PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }}
- run: cargo test --doc --no-default-features --features ${{ matrix.lua }}
# The workspace default member is only the root `pmacs` package, so
# the runs above never execute pmacs-protocol's own tests — the

249
builtin/runtime/lsp.lua
View File

@ -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
for _, item in ipairs(plan) do
if item.kind == "edit" then
pmacs.buffer.find_or_open(item.path)
edit_total = edit_total + apply_text_edits(item.edits)
files = files + 1
else
pmacs.buffer.apply_resource_op(item)
res_ops = res_ops + 1
end
end
-- 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.
if origin then pcall(pmacs.buffer.find_or_open, origin) end
-- 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
-- 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
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
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
-- 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)
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)

262
docs/active-work.md
View File

@ -654,6 +654,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 — MERGED AS PR #188
- Portable branch: `githubsucks/generated-buffer-immutability`; worktree
@ -851,6 +985,108 @@ has **no branch and no framing yet**.
warns against quoting a stale figure; it does not replace that
section's per-target census, which was not re-derived.
## Test-improvement arc, lane 2 — silent-skip arming
- Portable branch: `githubsucks/silent-skip-arming`, worktree
`../pmacs-skiparm`. Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4.
- **Base, measured at write time rather than quoted:**
```
$ git log --oneline -1 githubsucks/main
5e186c7 Merge pull request #193 from levineuwirth/test-improvement-audit
```
The previous revision of this entry said "base measured at write
time, pasted below" and then pasted nothing: the script meant to
substitute it reported success and silently matched no text, and the
claim was not re-read. Recorded because it is the same defect this
ledger keeps catching one level up — **asserting a measurement is not
making one, and a tool reporting success is not the measurement
either.**
- Recovery from a clean checkout:
`git fetch githubsucks && git worktree add ../pmacs-skiparm
-b silent-skip-arming githubsucks/silent-skip-arming`.
- **The defect:** `let Ok(_) = which_binary(x) else { eprintln!(..);
return; }` reports GREEN when the tool is absent, and CI installed
none of the tools. A block of real-language-server and multi-shell
tests had therefore **never once executed their bodies** in CI while
reporting success. A suite that cannot distinguish "passed" from
"never ran" is worse than a missing one, because it reads as
coverage.
- **The fix is the project's own pattern.** `PMACS_REQUIRE_*` already
makes a missing GPU fatal for `vterm_stage3_acceptance`; this adds
`PMACS_REQUIRE_LSP`, `PMACS_REQUIRE_SHELLS` and `PMACS_REQUIRE_LUA`,
plus the CI step that installs the tools. Per-tool variables, not one
blanket flag, so a tool that must stay unarmed keeps its decision
visible at the call site.
- **`basedpyright` is deliberately NOT installed and NOT armed.** Its
test has no timeout and hangs forever — root cause is the
non-interruptible reader-thread join in `RuntimeHandles::drop`,
already a named deferral in `src/process.rs`. The `test` job has no
`timeout-minutes` either. Arming it today would trade a vacuous green
for a six-hour hang across four legs. `PMACS_REQUIRE_PYRIGHT` exists
and is never set, so the flip is one line once lane 4 (the hang) and
lane 3 (timeouts) land. **Do not arm it before both.**
- **A trap found while writing the workflow, not after:** the natural
Actions idiom `${{ runner.os == 'Linux' && '1' || '' }}` sets the
variable to the EMPTY STRING elsewhere, and `var_os().is_some()` is
true for `Some("")`. That would have armed the guard on exactly the
runners with no tools installed. The helper therefore treats empty as
unset. `PMACS_REQUIRE_GPU` has the same latent shape and is safe only
because it is set literally.
- **Verified by execution in all three states**, on a tool genuinely
absent from this machine (`vscode-json-language-server`): unset ->
skips green; armed -> hard failure naming the CI step; empty string
-> skips green. The armed failure is the bite, and on `main` it
cannot occur because no guard exists.
- **The tests pass when they actually run** — which was the open
question, since none of them had. Armed locally: 11 `m6_5` + 8 `m6_8`
REPL tests green, and all six real-LSP tests (clangd x2, gopls x2,
rust-analyzer x2) green individually.
- **rust-analyzer is installed in the Linux-gated step, not via the
toolchain action's `components:`.** The first revision put it there,
which applies to *every* matrix leg — and **presence, not
`PMACS_REQUIRE_LSP`, is what decides whether a gated test body
runs**. That would have executed the two rust-analyzer tests on macOS
for the first time ever, on the legs that are simultaneously the CI
critical path and the documented flake surface, while this entry
claimed Linux only. The variables not being set there would only have
meant absence was tolerated; it would not have kept the tests
skipped. Text and workflow now agree.
- **Tool versions are pinned** (`gopls@v0.16.2`,
`vscode-langservers-extracted@4.10.0`,
`yaml-language-server@1.15.0`). `@latest` and bare `npm install -g`
make CI drift with upstream releases, so a bad publish breaks CI with
no commit here to bisect against. Caching the built `gopls` on the
pinned version is a follow-up, not done here.
- **§1.2 is NOT fully closed by this lane.** The guards arm the
*entry* skip only. `tests/m4_acceptance.rs`'s mid-test rust-analyzer
bail ("workspace likely still indexing; skipping") survives, so even
armed, that test's only assertion can still vanish under load —
precisely when a regression would show. Mid-test skips are their own
shape and want their own pass.
- **Not this lane's to fix, recorded so it is not mistaken for
oversight:** the generated-buffer immutability lane above still reads
"PR #188 OPEN, PROPOSED" and #188 has merged. Rule 4 forbids
relabelling it and permits removal only once its durable facts reach
`docs/agent-handoff.md`, which #188 did not touch — it changed the
framing and this ledger only. So the absorption is genuinely owed,
and the natural carrier is the arc's own next PR (#191, Stage 1),
not a testing lane reaching across into someone else's arc.
- **Follow-up owed after this merges:** delete
`githubsucks/handoff-2026-07-20`. Removing the documentation lane
removes the only pointer to that branch, so nothing will otherwise
remind anyone it still exists on the remote.
- Linux only for now, deliberately: macOS needs the brew equivalents
and roughly doubles install cost on the slowest matrix leg. The
variables stay unset there, so those tests skip cleanly.
- Also removes the **documentation lane**, whose disposition the ledger
left undecided pending confirmation that its branch carried nothing
unique. Confirmed by measurement: `githubsucks/handoff-2026-07-20` is
**1 ahead, 365 behind**, and its entire unique diff is four doc files
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`
@ -873,32 +1109,6 @@ git worktree add --track \
githubsucks/kill-ring-browser
```
## Documentation lane — STALE, AND ITS DISPOSITION IS UNDECIDED
> **Measured 2026-07-28, not inferred:** `githubsucks/handoff-2026-07-20`
> is at `c11d7e7`, **1 commit ahead of `main` and 320 behind**. Its
> whole diff against `main` is four documentation files
> (`docs/active-work.md`, `docs/agent-handoff.md`,
> `docs/roadmap-2026-07.md`, `docs/vterm-framing.md`), every one of
> which has been rewritten repeatedly since by the landed-doc PRs
> #156/#168/#169/#172/#180. Rule 4 removes a lane on merge *or
> abandonment*, and this one looks abandoned in substance — but "looks
> abandoned" is not the same as a decision, and no PR was ever opened
> for it. **This snapshot deliberately annotates rather than deletes:
> whoever confirms the branch carries nothing unique removes the
> section.** The bullets below are its original claims, preserved as
> written and now unverified.
- Portable branch: `githubsucks/handoff-2026-07-20`
- Carries synchronized `AGENTS.md` / `CLAUDE.md`, this ledger, the
durable handoff refresh, and the keybinding reference correction.
- It changes no runtime code.
- Review and merge this documentation branch separately; it must not be
folded into a feature framing branch.
- Now also absorbs both landed arcs: Vterm Stage 1 (#126) and the config
registry (#127). Canonical `main` is merged into it up to `2e37c04`,
so its diff against `main` is documentation only.
## Closed since the last snapshot
- **Terminal configuration + copy mode arc — BOTH STAGES MERGED, lane

16
docs/agent-handoff.md
View File

@ -1443,6 +1443,22 @@ round-trip cannot detect a discriminant shift.
## 5. Hard-won ops lessons
- **A gate summary assembled through a pipe can report success over a
failure.** `cmd | tail -2` returns **`tail`'s** exit status, not
`cmd`'s — in `fish` and `bash` alike — so a chain of
`cargo test ... | tail -2 && cargo test ... | tail -2 && echo "ALL
GATES CLEAN"` prints the clean line even when a suite failed. This
is not carelessness that closer reading would catch: the failure is
**structurally invisible** in the summary the PR then cites. It
happened while gating the silent-skip lane, and a `pmacs-gpu`
failure was reported as clean.
Either check `$pipestatus[1]` in fish (`${PIPESTATUS[0]}` in bash),
or — better — redirect each gate to a file and read the file
afterwards, which also preserves the full log this section already
asks you to keep. Same family as the skip-reports-`ok` lesson below
and the double-invocation traps: **the thing that summarizes a gate
must not be able to lose the gate's verdict.**
- **A test that skips on a missing precondition reports `ok`, and a gate log
cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the
only acceptance driving a real daemon, a real PTY and a real wgpu render

353
docs/resource-op-delete-guard-framing.md
View File

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

99
src/bin/pmacs_fake_lsp.rs
View File

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

291
src/lua_bindings/mod.rs
View File

@ -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,31 +3436,68 @@ 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) => {
let r = if md.is_dir() {
if recursive {
std::fs::remove_dir_all(&pb)
} else {
std::fs::remove_dir(&pb)
}
} else {
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));
// Phases 1 and 2, as one shared query.
let md = {
let verdict =
delete_verdict(&reg.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 => {}
}
Err(e) => return Err(io_err("delete (stat)", e)),
}
// `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)
} else {
std::fs::remove_dir(&pb)
}
} else {
std::fs::remove_file(&pb)
};
r.map_err(|e| io_err("delete", 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, &reg, 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(&reg.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(

1704
tests/m4_acceptance.rs
View File

File diff suppressed because it is too large Load Diff

21
tests/m6_5_repl_acceptance.rs
View File

@ -35,6 +35,9 @@ use std::path::PathBuf;
use std::sync::{Mutex, MutexGuard};
use std::time::{Duration, Instant};
#[path = "support/mod.rs"]
mod support;
static PUMP_TEST_LOCK: Mutex<()> = Mutex::new(());
fn pump_test_guard() -> MutexGuard<'static, ()> {
@ -196,7 +199,7 @@ fn m6_5_ret_submits_input_to_process() {
#[test]
fn m6_5_ctrl_d_on_empty_prompt_closes_stdin() {
let Some(bash) = locate_shell("bash") else {
eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)");
support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH");
return;
};
let setup = format!(
@ -290,7 +293,7 @@ fn m6_5_ctrl_d_on_nonempty_input_deletes_char_forward() {
)]
fn m6_5_ctrl_c_sends_sigint() {
let Some(sleep) = locate_shell("sleep") else {
eprintln!("skipping: sleep not on PATH (set PMACS_TEST_SLEEP to override)");
support::skip_or_fail_overridable("sleep", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_SLEEP");
return;
};
let setup = format!(
@ -344,7 +347,7 @@ fn m6_5_ctrl_c_sends_sigint() {
)]
fn m6_5_exit_marker_uses_basename_with_leading_newline() {
let Some(false_bin) = locate_shell("false") else {
eprintln!("skipping: false not on PATH (set PMACS_TEST_FALSE to override)");
support::skip_or_fail_overridable("false", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FALSE");
return;
};
let setup = format!(
@ -425,7 +428,7 @@ fn run_shell_smoke_test(shell_path: &std::path::Path, argv_extra: &[&str]) {
#[test]
fn m6_5_repl_spawns_bash() {
let Some(bash) = locate_shell("bash") else {
eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)");
support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH");
return;
};
run_shell_smoke_test(&bash, &["-i"]);
@ -435,7 +438,7 @@ fn m6_5_repl_spawns_bash() {
#[test]
fn m6_5_repl_spawns_zsh() {
let Some(zsh) = locate_shell("zsh") else {
eprintln!("skipping: zsh not on PATH (set PMACS_TEST_ZSH to override)");
support::skip_or_fail_overridable("zsh", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_ZSH");
return;
};
run_shell_smoke_test(&zsh, &["-i"]);
@ -446,7 +449,7 @@ fn m6_5_repl_spawns_zsh() {
#[test]
fn m6_5_repl_spawns_fish() {
let Some(fish) = locate_shell("fish") else {
eprintln!("skipping: fish not on PATH (set PMACS_TEST_FISH to override)");
support::skip_or_fail_overridable("fish", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FISH");
return;
};
run_shell_smoke_test(&fish, &["-i"]);
@ -458,8 +461,10 @@ fn m6_5_repl_spawns_fish() {
#[test]
fn m6_5_repl_spawns_lua() {
let Some(lua) = locate_shell("lua").or_else(|| locate_shell("luajit")) else {
eprintln!(
"skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)"
support::skip_or_fail_overridable(
"lua/luajit",
"PMACS_REQUIRE_LUA",
"PMACS_TEST_LUA or PMACS_TEST_LUAJIT",
);
return;
};

9
tests/m6_8_multi_repl_acceptance.rs
View File

@ -68,6 +68,9 @@ use pmacs::editor::EditorState;
use std::path::{Path, PathBuf};
use std::time::{Duration, Instant};
#[path = "support/mod.rs"]
mod support;
// ---------------------------------------------------------------------------
// Test harness
// ---------------------------------------------------------------------------
@ -94,8 +97,10 @@ fn locate_lua() -> Option<PathBuf> {
}
}
}
eprintln!(
"skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)"
support::skip_or_fail_overridable(
"lua/luajit",
"PMACS_REQUIRE_LUA",
"PMACS_TEST_LUA or PMACS_TEST_LUAJIT",
);
None
}

104
tests/support/mod.rs Normal file
View File

@ -0,0 +1,104 @@
//! Shared test-support helpers.
//!
//! Included by `#[path = "support/mod.rs"] mod support;` rather than
//! copied. Files under `tests/` subdirectories are not compiled as
//! their own test binaries, so this costs nothing — and
//! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying
//! cross-test-binary sharing "would need a fixture crate", which is not
//! so. A correct helper in one file and a degraded copy in another is
//! this suite's most repeated defect shape; sharing removes the way it
//! happens.
//!
//! **Why this is separate from `tests/common/`, which also exists.**
//! `tests/common/mod.rs` re-exports `daemon` and `pty` — real daemon
//! spawning and PTY plumbing. Including it to reach a six-line
//! environment check would compile that machinery into three test
//! binaries that spawn neither, for no benefit. `support` is the
//! dependency-free half: helpers any test binary can take without
//! taking a subsystem with them. Two directories is a cost worth
//! naming rather than leaving to be rediscovered; if a third appears,
//! consolidate instead of continuing the pattern.
#![allow(dead_code)]
/// Report a missing external tool, and turn the skip into a HARD
/// FAILURE when the environment has promised the tool is present.
///
/// The bare shape this replaces —
///
/// ```ignore
/// let Ok(_) = which_binary("gopls") else {
/// eprintln!("gopls not on PATH; skipping");
/// return;
/// };
/// ```
///
/// passes GREEN when the tool is absent, and is why a large block of
/// external-tool-gated tests had never once executed their bodies in
/// CI: nothing installed the tools, so every one of them reported
/// success without running. A suite that cannot tell "passed" from
/// "never ran" is worse than a missing suite, because it reads as
/// coverage.
///
/// `PMACS_REQUIRE_*` is the project's own fix, already load-bearing for
/// `PMACS_REQUIRE_GPU` in `vterm_stage3_acceptance`: CI installs the
/// tool, sets the variable, and absence becomes a failure that names
/// the step that should have provided it. Locally the variable is
/// unset, so the skip still works and nobody needs the whole toolchain
/// to run the suite.
///
/// Deliberately per-tool rather than one blanket variable: a tool that
/// must stay unarmed (because arming it would hang, or because CI does
/// not install it yet) keeps its own variable that CI never sets, and
/// that decision is then visible at the call site instead of buried in
/// a workflow file.
/// True when `var` is set to a non-empty value.
///
/// Emptiness matters, and the reason is a trap rather than a nicety.
/// The natural GitHub Actions idiom for a conditional environment
/// variable —
///
/// ```yaml
/// PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }}
/// ```
///
/// sets the variable to the EMPTY STRING on every other platform, not
/// to nothing. A bare `var_os(..).is_some()` is therefore true there,
/// which would arm the guard on exactly the runners that have none of
/// the tools installed and fail every one of them. Treating empty as
/// unset makes the common workflow spelling safe instead of subtly
/// wrong.
fn armed(var: &str) -> bool {
std::env::var_os(var).is_some_and(|v| !v.is_empty())
}
#[track_caller]
pub fn skip_or_fail(tool: &str, require_var: &str) {
assert!(
!armed(require_var),
"{require_var} is set, but `{tool}` is not on PATH. \
The CI step that installs it did not run, or installed it \
somewhere not on PATH. This is a hard failure precisely so \
the test cannot report green without executing."
);
eprintln!("{tool} not on PATH; skipping (set {require_var} to make this fatal)");
}
/// As [`skip_or_fail`], for tools whose PATH lookup can be overridden
/// by a `PMACS_TEST_*` variable. The skip notice keeps naming that
/// override, because losing it would make the local escape hatch
/// undiscoverable — the REPL suites are routinely run on machines
/// without zsh or fish.
#[track_caller]
pub fn skip_or_fail_overridable(tool: &str, require_var: &str, override_var: &str) {
assert!(
!armed(require_var),
"{require_var} is set, but `{tool}` is not on PATH and {override_var} \
is unset or points at nothing. The CI step that installs it did not \
run, or installed it somewhere not on PATH."
);
eprintln!(
"skipping: {tool} not on PATH (set {override_var} to override, \
or {require_var} to make this fatal)"
);
}