diff --git a/docs/dired-stage2-framing.md b/docs/dired-stage2-framing.md index bce84cd..eab6b7c 100644 --- a/docs/dired-stage2-framing.md +++ b/docs/dired-stage2-framing.md @@ -1,28 +1,254 @@ # Dired Stage 2 — marks and operations — framing -**Revision 4 — 2026-07-25. Status: PROPOSED; review rounds 1–3 -addressed. Round 3: three design blockers, one merge-readiness blocker, -four cleanups — and the staging call taken (§10: three PRs, not two).** +**Revision 5 — 2026-07-28. Status: PROPOSED — NOT APPROVED. This +document has never received a formal framing approval, and it needs one +from the user before any implementation branch is cut.** Its four +commits embody three rounds of review findings; that is not the same as +approval, and GitHub records no review on PR #171. + +**Ground truth: re-scouted 2026-07-28 against canonical `main` @ +`6bee09d`** (`Merge pull request #184 from levineuwirth/bottom-panel-stage2b`). +Rev 4 was scouted at `c8ec8f3` — which is dired Stage 1's *own* merge +commit — and `main` has moved **153 commits** since. Rev 5 is that +re-scout: §0's round-4 section states exactly what moved, what it +invalidated, and what survived unchanged. + Continues `docs/dired-framing.md` (rev 7, approved; Stage 0 merged as #162, Stage 1 as #165). That document's §6 and §7 carry the *approved* shape of marks and operations; this one re-verifies every claim in them -against `main` @ `c8ec8f3`, corrects what has drifted or was wrong, and -adds what a Stage 2 implementation needs and the parent did not decide: -the batch-execution contract, the confirmation surface, the staging cut, -and acceptance. Decisions continue the parent's scheme from **Q#DR12**. +against the tree, corrects what has drifted or was wrong, and adds what a +Stage 2 implementation needs and the parent did not decide: the +batch-execution contract, the confirmation surface, the staging cut, and +acceptance. Decisions continue the parent's scheme from **Q#DR12**. The parent framing's ground truth (§2) was scouted at `main` @ `e745068` and re-verified at `0827dd1`. Stage 1 then changed three of the files Stage 2 depends on most (`src/fs.rs`, `src/async_runtime.rs`, -`src/lua_bindings/mod.rs`), so **§2 below supersedes the parent's line -references for everything Stage 2 touches.** +`src/lua_bindings/mod.rs`), and #178/#179/#181/#182/#183/#184 have since +changed more, so **§2 below supersedes the parent's line references for +everything Stage 2 touches.** Line numbers throughout are given for +navigation only; every claim is anchored to a **symbol**, because line +numbers drift and this document has now watched them drift twice. --- ## 0. Revision history +### Re-scout round 4 (rev 4 → rev 5) — `c8ec8f3` → `6bee09d`, 153 commits + +No reviewer produced these; a re-scout did. Rev 4's design survives, but +**one new dependency changes what Stage 2 must build, one changes the +shape of a mechanism rev 4 described, and seven of rev 4's own claims +about pmacs were wrong** — which is the failure mode this arc has been +burned by before (a framing that verifies its *external* facts and gets +its *internal* ones wrong). Every claim below was read on the tree at +`6bee09d`, not inferred. + +#### What arrived that Stage 2 must now answer + +- **N1 (new scope, blocking the mark layer) — #178 landed + `Buffer::set_generated_contents`, and dired has not adopted it.** + `src/buffer.rs:545` is now *the* authorized write path for a generated + buffer: lift `read_only`, one whole-buffer `Replace` skipping + intercepts, `clear_history`, re-assert `read_only`, and **return the + `Edit`**. `docs/agent-handoff.md` §4 names `builtin/runtime/dired.lua:371` + as one of **four writer mechanisms that have not adopted it**, and + `COHERENCE.md` §14 records the same. Dired still pairs an erroring + intercept (`dired.lua:509-511`) with a `bypass_intercept` whole-buffer + replace (`paint`, `dired.lua:369-372`) **over a still-writable rope**, + so **`M-x buffer.undo` empties a dired listing today** — the command + exists (`builtin/commands/default.lua:179`), needs no keybinding, and + `Buffer::undo` reaches the rope through `ensure_writable` + (`src/buffer.rs:568-577`) without ever consulting the intercept chain. + Stage 2 writes that buffer on every mark, every unmark, every toggle, + and after every batch, so it multiplies the exposure rather than + inheriting it quietly. Rev 5 adopts the primitive as **Q#DR25** (§3, + §10) and states the three traps the handoff attaches to it. +- **N2 (shape change) — #182 (Journey Stage 1a) demoted dired to a + replaceable slot, and rewrote the function Stage 2's operations run + inside.** `resolve_target_buffer` gained a `ResolvedTarget::Directory` + arm **ahead of** the load (`src/editor_core.rs:964-970`); + `EditorState::open` became a caller rather than a parallel + implementation (`src/editor.rs:953-959`); and which surface handles a + directory is now the `path.open-directory` hook chain with dired as a + **replaceable fallback slot**, `pmacs.path.directory_handler` + (`dired.lua:736-738`, registered via + `pmacs.path.set_directory_handler`, `src/lua_bindings/mod.rs:3679`) — + deliberately **not** a hook subscriber, because `HookRegistry::add` + only appends and builtins load before `init.lua`, so a dired + subscription would always claim before any user listener. The + consequence for Stage 2 is in `open_directory`, which gained + `opts.dest` and a `commit()` closure run under + `pmacs.window.commit_to` (`dired.lua:656-707`), and that scope + **refuses an `await` inside it** (Q#JR14b). §9's serialize-and-await + batch therefore has a constraint rev 4 could not have known about. +- **N3 (a ratchet Stage 2 must not break) — `tests/journey_acceptance.rs`.** + **24 `#[test]`s**, 0 ignored, 0 feature-gated, 2 gated + `#[cfg(target_os = "linux")]`. Its module doc states the rule + verbatim at `:12`: *"**This file is a ratchet: stages add rows, none + removes them.**"* Seven of its tests assert on dired directly, and its + step-3 rows pin that a directory launch produces a `*dired:*` + buffer. **The ratchet is split across two files**: #183 put the GPU + journey row in `tests/gpu_invocation_acceptance.rs` + (`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`), + not in `journey_acceptance.rs`. Both are gates now (§14). + *(Correcting the re-scout brief: #183 did **not** extend + `journey_acceptance.rs` — `git diff c2d56ff 7fd646d -- tests/journey_acceptance.rs` + is empty.)* +- **N4 (a fan-out lesson Stage 2's two new hooks inherit) — #179/#181 + landed the typed-edit consumer chain.** Dired participates in + **neither** the chain nor `buffer.after-edit` (grep of `dired.lua` for + `typed_edit|after-edit`: **0**), and `set_generated_contents` fires no + hook — its Lua binding routes only through + `notify_buffer_edit_to_windows` (`src/lua_bindings/mod.rs:3092`), + which runs no Lua. So N1 does not drag dired into the chain. But + `resource.renamed` / `resource.deleted` are new fan-outs and inherit + the chain's three hard-won lessons: a consumer cannot both edit and + let a later consumer act meaningfully (the record is a **copy** taken + before any consumer ran); `buffer.after-edit` fan-outs **nest**; and + the thing that counts fan-outs must be **unskippable**. §5 states + which of these bind the new hooks and which do not. + +#### Rev 4 claims that are wrong about pmacs itself + +- **W1 (load-bearing) — §5 step 2 names one function and cites a + different one's line.** Rev 4: *"`drain_external_cancelled` (`:1596`) + is the existing sweep."* These are **two distinct functions**: + `LspManager::drain_external_cancelled` (`src/lsp.rs:1561-1576`) is the + **server-scoped, unconditional** drain — settle every awaiter for + `sid` cancelled and drop its `pending_external` entries; that is the + right precedent. `LspManager::drain_cancelled_externals` + (`src/lsp.rs:1596-1645`) is a **per-tick, per-awaiter cancellation and + timeout sweep** that removes only awaiters whose `CancellationToken` + was flipped or which outlived `request_timeout`. An implementer + following rev 4's line number reaches the second: **a rename flips no + token, so the drain half of `forget_uri` would be a silent no-op** and + a coroutine awaiting a request against the old URI hangs forever — + precisely the failure step 2 exists to prevent. Corrected in §5 and + Q#DR23. +- **W2 — there is no `fn restart`.** Rev 4 models `forget_uri` on *"the + server-scoped teardown that already exists (`restart`, `:1316-1331`)"*. + The teardown is **`LspManager::start_generation`** + (`src/lsp.rs:1307-1345`); its route purge is at `:1324`, which is the + one number rev 4 got right. And there is a **second** precedent rev 4 + never names: **`LspManager::forget`** (`src/lsp.rs:3015-3042`), which + does the same three things (`pending_routes.retain`, + `drain_external_cancelled`, `documents.retain`) plus + `status_tracker.forget` and `project_servers`. Rev 4's genuinely + surprising observation survives and now applies twice: **neither** + server-scoped teardown clears the fourteen result stores. +- **W3 — the route inventory is off, and the gap is the part an + implementer trips on.** `ResponseRoute` (`src/lsp.rs:835-859`) has + **15 variants, of which 14 carry a `uri`**; the fifteenth is + `WorkspaceSymbol { query }`. There are **16** insert sites, all in + `request_*` methods spanning `src/lsp.rs:1683-2132` — and **15 of the + 16 insert a URI-bearing route**. Rev 4's enumeration of fifteen was + therefore *correct as a list of URI-bearing inserts* and *wrong as a + claim about how many insert sites exist*. The arithmetic matters + concretely: a purge predicate written as *"retain unless the route's + uri equals the old one"* has to say something about a variant that has + no `uri` field at all, and `request_workspace_symbol` + (`src/lsp.rs:1877-1891`) is that variant. Stated in §5. +- **W4 — the Lua-side blast radius is ~3× what rev 4 said.** F1 put + `rec.uri` at *"~20 sites"*. The real figure on this tree is **57 + lines** in `builtin/runtime/lsp.lua` (`grep -c "rec\.uri"` = 57, one + per line). Rev 4's design is unaffected — the point of caching + `rec.uri` in the attachment record is that one rebind reaches all of + them — but the magnitude was understated and the arc has a standing + rule against sizing anything from a partial count. +- **W5 — the path-owner census has grown to SIX.** Rev 4's five owners + all still hold (re-verified individually in §5). A **sixth** arrived + with the Lean 4 stages: **`lean.lua`'s `M.file_progress`** + (`builtin/runtime/lean.lua:646`, written at `:651`), a **URI-keyed + Lua module table** populated from the `$/lean/fileProgress` + notification and, by its own comment, read by Stage 5's goal view. It + lives in **no Rust store**, so `forget_uri` cannot reach it; it *is* + reachable from a `resource.renamed` Lua subscriber, which is an + argument for the hook rev 4 already proposed. Two weaker, **persisted** + owners are also now named rather than fixed: `saveplace.lua`'s + path-keyed places file and `recentf.lua`'s MRU list (§11). +- **W6 — the ledger note names a merged PR.** §16 said + `docs/active-work.md` and `docs/agent-handoff.md` are held by open PR + #169. #169 merged as `74301d1`. The current holder is **PR #185** + (`docs-landed-state-184`). The instruction is unchanged; the number + was stale. +- **W7 — the C1 seam moved, and it has two namesakes.** + `pmacs._async._tick` is at `src/lua_bindings/mod.rs:7104-7115`, inside + `install_async`, not `:6911-6924`. Its closure is + `move |lua, ()|`, so `lua.app_data_ref::()` is reachable + and C1's decision stands. Worth naming because `mod.rs` defines + **three** `_tick` bindings in different classes — + `install_async:7104` (`pmacs._async._tick`), + `install_process:8469` (`pmacs.process._tick`), and + `install_lsp:10271` (`pmacs.lsp._tick`) — and only the first has + `lua` in scope. + +#### What the re-scout confirmed unchanged + +Listed because "still true at `6bee09d`" is the load-bearing half of a +re-scout, and because each of these was read again rather than carried +forward: the whole `pmacs.fs` surface and its five `*_blocking` +implementations, at **identical line numbers** (§2); the absence of +`mkdir`, `copy`, and any recursive remove (the four `create_dir` hits in +`src/fs.rs` are all inside `#[cfg(test)]` bodies); `PendingJob`'s seven +fields and `JobKind`'s **12** variants; `tick`'s signature +(`pub fn tick(&self) -> Vec`, `src/async_runtime.rs:1003`) and +the `Sleep | FsUnit` collapse at `:1046`; `dispatch_fs_rename` moving +both paths into the closure (`:871`); `LspManager`'s **fourteen** +URI-bearing store families (`src/lsp.rs:753-793`), exactly the fourteen +rev 4 tabulated, in the same order; `documents` at `:810` and +`pending_external` at `:804` with the *"drained-cancelled wherever +`pending_routes` is purged"* contract at `:801-803`; +`DiagnosticView.uri` private, set once at construction, its doc still +reading *"M5 may add re-rooting if a buffer is renamed"* +(`src/diag.rs:456-457`); `View` still carrying `overlay_identity` and +`clone_for_split` and **no** `rename_resource` and no downcast +(`src/view.rs:300`, `:310`, `:285`); the overlay-disposal sweep over +`core.windows.values_mut()` (moved to `src/lua_bindings/mod.rs:2044-2046`); +`_attach_view` taking `active_window_mut()` and erroring +(`src/lua_bindings/diag.rs:211-232`); `apply_resource_op`'s rename arm +still doing a raw-path `find_by_path` first-match rebind +(`src/lua_bindings/mod.rs:3306`) and its delete arm still killing +through `remove_buffer_and_fire` with **no modified check** +(`:3339-3341`) — **so an LSP-authored delete still destroys unsaved work +on `main` today**; `apply_workspace_edit` still capturing `origin` as a +**string** and restoring with `find_or_open` (`builtin/runtime/lsp.lua:1338`, +`:1352`), its own comment still conceding the path *"may have just been +renamed or deleted"*; `resolve_target_buffer`'s `NotFound` arm still +materializing a phantom path-backed buffer (`src/editor_core.rs:980-989`); +`Buffer::set_name` existing with the doc *"Used by save-as and rename +operations"* (`src/buffer.rs:453`) and **no** `pmacs.buffer.set_name` +Lua binding; `killring.lua`'s `push_entry` still a `local function` +(`:93`) and `copy()` still region-required (`:184-194`); **no** +`builtin/runtime/minibuffer.lua` and **no** `y_or_n`/`yes_or_no`/ +`yes-or-no` anywhere in `src/` or `builtin/`; `pmacs.fs.rename` still +having **zero production callers** (only `tests/fixtures/pmacs-dired/init.lua:1200,1210`, +`tests/m8_1_acceptance.rs:359`, and `tests/m8_3_acceptance.rs:182-184`'s +monkeypatch) — and the same is now separately confirmed for +`pmacs.fs.remove` (only `tests/m8_1_acceptance.rs:438,439,472`); and +every one of dired's own Stage 1 seams at **unchanged line numbers** +(`render_entry:334`, `paint:369-372`, `entry_at_cursor:381-385`, +`seat_cursor:405-416`, the handle shape `:521-528`). The stale `*errors*` +comment rev 4 offered to fix in passing is still there, having moved +from `:665` to `:718`. + +Two dired references in rev 4 **did** drift and are corrected in §2: the +mode-scoped `bind` helper is at `dired.lua:933-935` with the bindings at +`:937-946` (was `:866-868`), and `pmacs.dired._layout` is at `:954-961` +(was `:887-895`). + +--- + ### Review round 1 (rev 1 → rev 2) +> **The three round sections below are a historical record and keep the +> line numbers they were written with (`c8ec8f3` / `c93f9ee`).** Their +> *findings* all still hold — each was re-verified at `6bee09d` — but +> for a current reference use §2 and §5, which carry the re-scouted +> positions. Where a round-section claim was found to be wrong about +> pmacs, §0's round-4 list says so and the live section is corrected; +> the round text is left as written rather than retconned. + Every checkable claim in the review was verified against `c8ec8f3` before being acted on; all seven held. @@ -196,6 +422,13 @@ Five corrections, one of them load-bearing. `lua.app_data_ref::()`, exactly as `apply_resource_op` does (`:3251`). §5 specifies the split: `AsyncRuntime` *harvests*, the binding *rebinds*. + *(Rev 5, W7: the decision stands, verified again at `6bee09d`. The + seam is now `src/lua_bindings/mod.rs:7104-7115`, inside + `install_async`; its closure is `move |lua, ()|`, so + `lua.app_data_ref::()` is still reachable. Note that + `mod.rs` defines **three** `_tick` bindings in different classes — + `install_async:7104`, `install_process:8469`, `install_lsp:10271` — + and only the async one has `lua` in scope.)* - **C2.** The parent's line references have drifted. `tick` is now `:1003` (was 991); the `Sleep | FsUnit → JobResult::Unit` arm is `:1046-1048` (was 1022-1025); `apply_resource_op`'s raw lookup is @@ -229,25 +462,54 @@ Five corrections, one of them load-bearing. ## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) -- **Journey steps.** Touches **step 7's file half**, which #165 moved from - "missing" to "fixed but unadvertised". Stage 2 does not change that - grade: it deepens a surface the user must still already know about. - Discoverability is §20 Priority 4's job, not this stage's, and this - framing does not claim otherwise. +**Sections served: §20 Priority 1 (protect the golden product journey) +and §14 (coherent workbench primitives).** Rev 4 cited neither by +number; the re-scout makes both concrete, and Priority 1 is a *new* +claim that only became true when #182 landed. + +- **Journey steps — and this changed under rev 4.** Rev 4 said Stage 2 + touches "step 7's file half" only, and that dired "deepens a surface + the user must still already know about". Half of that is now false: + **#182 put dired on step 3.** `COHERENCE.md` §20 Priority 1 records + directory-argument handling as **done** and says it "routes `pmacs .` + into #165's dired buffer rather than growing a second directory + surface", and `tests/journey_acceptance.rs` pins exactly that + (`journey_step3_opening_a_directory_lists_it`). So dired is no longer + only a surface the user must know to ask for — for anyone who runs + `pmacs .`, **it is the first thing they see**, and step 5's row + (`journey_step5_editing_a_file_reached_through_the_directory`) reaches + the editable file *through* it. That raises the stakes on Stage 2's + correctness rather than changing its scope, and it is the strongest + argument for N1: a step-3 surface that `M-x buffer.undo` can empty is + a journey regression waiting to be filed. Discoverability of the + *marks* remains §20 Priority 4's job, not this stage's, and this + framing still does not claim otherwise. +- **Workbench convergence (§14, Priority 5).** `COHERENCE.md` §14 names + dired explicitly in the generated-buffer adoption gap — "four writer + mechanisms have not yet adopted it and remain emptiable … and dired + buffers" — and classifies dired, with listview, as **"the cheap + half"**, because both already write whole-buffer replaces and so need + none of the streaming variant the three appending buffers require. + Q#DR25 closes dired's quarter of that gap. It does **not** close the + other three, and this framing does not claim progress on them. + Separately, §14's tree-primitive point is untouched: Stage 2 adds no + tree, and `i` (insert subdirectory) stays deferred (§11) precisely so + it can land on a shared primitive rather than inventing one. - **Interaction islands: adds none.** Every new key is an entry in the - existing mode-scoped `dired` keymap (`dired.lua:866-879`), and every new + existing mode-scoped `dired` keymap (`dired.lua:937-946`), and every new command is an ordinary `pmacs.command.define`, so all of it is reachable from `M-x` and describable by `describe.key`. The confirmation prompt uses the existing minibuffer rather than a new modal surface — that is the *reason* §7 spends a section on it. -- **Config registry.** Stage 2a adds no keys. Stage 2b adds +- **Config registry.** Stage 2a and 2b add no keys. Stage 2c adds `dired.recursive-deletes` (Emacs's `dired-recursive-deletes`), through - `pmacs.config.define` like `dired.kill-when-opening`. + `pmacs.config.define` like `dired.kill-when-opening` + (`dired.lua:78-84`). - **Background-work attribution (§9): this stage makes a real, and honest, dent in the wrong direction unless it is deliberate.** `COHERENCE.md` §9 grades the worker model "mechanism without identity" and names the prerequisite precisely: `PendingJob` - (`src/async_runtime.rs:367-394`) carries no owner, purpose, buffer + (`src/async_runtime.rs:367-375`) carries no owner, purpose, buffer association, or parent, and `JobKind` is a **closed 12-variant enum**. Stage 2 pushes on both: - It must make a pending rename **retain its from/to paths** (§5). That @@ -293,13 +555,23 @@ Two small public surfaces come with it, both because the operations have nowhere to land otherwise: **`pmacs.buffer.set_name`** (Q#DR21) and **`pmacs.killring.push`** (Q#DR22). +**And one correctness fix to Stage 1 that Stage 2 may not skip +(Q#DR25, new in rev 5): dired's listing becomes a genuinely immutable +generated buffer**, written through `pmacs.buffer.set_generated_contents` +rather than an erroring intercept over a writable rope. On `main` today +`M-x buffer.undo` empties a dired listing — no keybinding required — and +every mark, unmark, toggle and post-batch revert Stage 2 adds is another +write to that undo stack. §3 states the change; §10 places it at the +head of 2b. + Plus, invisibly: **renaming or deleting a path starts reconciling every -consumer that holds it** — buffer path *and* name, the five URI-keyed LSP -stores and the attached diagnostic view, dired's own pathless handles, -and the workspace-edit applier (§5, §6). That is a correctness fix to -shared substrate; it is the reason `R` is safe on a directory at all, and -it closes a path on which an LSP-authored delete currently destroys -unsaved work. +consumer that holds it** — buffer path *and* name, the fourteen +URI-keyed LSP store families and the attached diagnostic view, dired's +own pathless handles, `lean.lua`'s URI-keyed progress table, and the +workspace-edit applier (§5, §6). That is a correctness fix to shared +substrate; it is the reason `R` is safe on a directory at all, and it +closes a path on which an LSP-authored delete currently destroys unsaved +work. Not in Stage 2: `wdired` (Stage 3), subdirectory insertion (`i`), shell commands on marks (`!`), regexp marking (`% m`), and @@ -307,9 +579,12 @@ compress/symlink/hardlink ops. §11 names them. --- -## 2. Ground truth (scouted 2026-07-25, `main` @ `c8ec8f3`) +## 2. Ground truth (re-scouted 2026-07-28, `main` @ `6bee09d`) -Everything below was read or executed on this tree, not inferred. +Everything below was read or executed on this tree, not inferred. Where +a fact was already in rev 4 and survived the 153-commit gap unchanged, it +is left as written; where a line number moved it is corrected in place +without comment; where a *claim* changed, the change is called out. ### The filesystem surface @@ -355,7 +630,7 @@ Everything below was read or executed on this tree, not inferred. - `dispatch_fs_rename` (`async_runtime.rs:871-879`) **moves `from` and `to` into the worker closure.** Nothing retains them, which is exactly why §5 needs a new field. -- `PendingJob` (`:367-394`) carries `{cancel, state, supersede_key, +- `PendingJob` (`:367-375`) carries `{cancel, state, supersede_key, stream_buffer, max_batch, kind, dispatched_at}` — `kind` is there, the paths are not. - Rename settles as an **undifferentiated `ReplyKind::FsUnit`**: the @@ -366,23 +641,26 @@ Everything below was read or executed on this tree, not inferred. needs.** `:1074-1108` iterates `newly_settled`, borrows `pending`, and reads `job.kind`, `job.state`, `job.dispatched_at`, and `job.supersede_key` to push a `CompletedSlot`. The harvest is one more - read in a loop that already exists. + read in a loop that already exists. *(Unchanged at `6bee09d`, line + numbers included.)* - `find_by_path` (`buffer_registry.rs:168-174`) is exact `Path` equality over `self.order`, **first match only**. No prefix logic, and two buffers on one path means one of them is invisible to it. -- `apply_resource_op`'s rename arm (`mod.rs:3234-3255`) does a +- `apply_resource_op`'s rename arm (`mod.rs:3291-3312`) does a **synchronous** `std::fs::rename` on the main thread and then - `reg.borrow().find_by_path(&from)` with the **raw** path (`:3249`), - while stored paths are normalized on write (`editor_core.rs:819`) and - the normalizing wrapper `find_buffer_for_path` (`:864-867`) exists and - is bypassed. This is a **second**, LSP-facing rename path with the same - two defects; §5 fixes both in one change. + `reg.borrow().find_by_path(&from)` with the **raw** path (`:3306`), + while stored paths are normalized on write (`editor_core.rs:889-890`) + and the normalizing wrapper `find_buffer_for_path` (`:935-938`) exists + and is bypassed. This is a **second**, LSP-facing rename path with the + same two defects; §5 fixes both in one change. *(Rev 4 cited + `:3234-3255` / `:3249` / `:819` / `:864-867`; the code is unchanged, + only its position.)* ### A verified pre-existing defect Stage 2 must not lean on **A fire-and-forget non-stream job leaks its pending entry forever.** The only two removals from `pending` are stream eviction of closed streams -(`:1220-1226`) and `take_result` (`:1262-1282`); the Lua `Handle` is a +(`:1225`) and `take_result` (`:1262-1282`); the Lua `Handle` is a bare `setmetatable({_id = id}, Handle)` (`async.lua:55`) with **no `__gc`**, so dropping a handle reaps nothing. @@ -414,7 +692,7 @@ and **prunes marks whose target no longer exists** on refresh Stage 0's finding is that a selected candidate shadows typed text. The boundary is sharper than the parent framing records: -`resolve_accepted_value` (`src/minibuffer.rs:564-574`) **short-circuits +`resolve_accepted_value` (`src/minibuffer.rs:564-575`) **short-circuits on `CompletionSource::None` at `:565-567` and returns the typed text before the candidate branch is reached.** @@ -434,18 +712,168 @@ Rust-only. The nearest existing confirm is `autosave.lua:219-224`, a (`:521-528`) — **no mark state; §3 adds it.** `render_entry` (`:334-349`) hardcodes `BLANK_MARK` in column 0. `paint` (`:369-372`) is a wholesale `buf:replace` with `bypass_intercept = true`; the read-only intercept -(`:509-511`) rejects everything else. `seat_cursor` (`:405-416`) re-seats -by basename and carries the warning that `move_to_line` is **ambient** — -every post-`await` seat must first check `pmacs.window.buffer()`. +(`:509-511`) rejects everything else. **Both of those are what Q#DR25 +replaces** (§3). `seat_cursor` (`:405-416`) re-seats by basename and +carries the warning that `move_to_line` is **ambient** — every +post-`await` seat must first check `pmacs.window.buffer()`. `entry_at_cursor` (`:381-385`) maps cursor line *n* to `entries[n]`, returning nil on the header and the footer. Keys are mode-scoped through -one helper (`:866-868`), and `pmacs.dired._layout` (`:887-895`) exports -the column contract. +one helper (`bind`, `:933-935`, with the nine bindings at `:937-946`), +and `pmacs.dired._layout` (`:954-961`) exports the column contract. +`tests/dired_acceptance.rs` carries **25** tests. -One stale comment to fix in passing: `:665` still says an uncaught raise +One stale comment to fix in passing: `:718` still says an uncaught raise lands "in \*errors\*", which #161's COHERENCE finding falsified and which the module doc at `:56-72` already corrects. Same file, two -answers. +answers. *(It was `:665` in rev 4; #182 moved it, and it is still +wrong.)* + +### The generated-buffer write invariant, which dired has not adopted (N1) + +`Buffer::set_generated_contents` (`src/buffer.rs:545-556`) is, per +`docs/agent-handoff.md` §4, **the one authorized write** for a generated +buffer. It does four things as a unit: clear `read_only`, apply one +whole-buffer `EditOp::Replace` through `apply_edit_skip_intercepts`, +`clear_history()`, re-assert `read_only` — and **return the `Edit`**. + +The three traps the handoff attaches to it, each verified here: + +- **An intercept is not read-only.** `Buffer::undo` reaches the rope via + `ensure_writable` (`src/buffer.rs:568-577`), which consults only the + `read_only` flag and never the intercept chain. `M-x buffer.undo` is a + defined command (`builtin/commands/default.lua:179`) reachable from + M-x with no binding at all, so rebinding `C-/` buffer-locally would + not close it. **Dired sets no `read_only`** — `claim_handle` installs + an intercept and `set_round_trip_input`, nothing more + (`dired.lua:506-519`) — so a dired listing is emptiable today. +- **A rope write is only half an edit.** The `Edit` must be fanned out + or a displaying window keeps a `TextView` line index describing the + previous contents. The Lua binding already discharges this: + `pmacs.buffer.set_generated_contents` + (`src/lua_bindings/mod.rs:3079-3095`) releases the registry borrow and + then calls `notify_buffer_edit_to_windows` (`:3092`). **So a Lua + caller inherits the fan-out for free** — this is not something + `dired.lua` has to arrange, and the comment at `:3088-3091` says why + the borrow is dropped first. +- **"Discard history" means whichever history exists.** `clear_history` + (`src/buffer.rs:559-566`) clears the v0.1 `undo`/`redo` stacks **and**, + under `#[cfg(feature = "crdt")]`, calls `crdt.clear_undo_history()` — + because CRDT mode bypasses the v0.1 stacks entirely. Consequence for + §14: the `crdt`-featured `dired_acceptance` run is not a formality + here; it is the only run in which the CRDT half of Q#DR25 is live. + +There is deliberately **no** Lua `set_read_only` +(`src/lua_bindings/mod.rs:3074-3078` states the reason: it would let a +caller lock a buffer with no way to refresh it). Pairing the lock with +the write **is** the primitive, which is why adoption is a swap of +`paint`, not an addition to it. + +And it does **not** replace `set_round_trip_input`. The handoff is +explicit that the protection is layered across two copies: rope-level +`read_only` refuses the op at the daemon, while round-trip input stops a +semantic frontend applying optimistically to its **own mirror**, which a +daemon-side refusal arrives too late to prevent. `dired.lua:516` stays. + +### How a directory reaches dired, after #182 (N2) + +Rev 4 predates Journey Stage 1a. Three facts a Stage 2 implementer will +otherwise get wrong: + +- **`resolve_target_buffer` resolves a directory before it loads.** The + `ResolvedTarget::Directory { path }` arm is first + (`src/editor_core.rs:964-970`), ahead of `get_or_load_buffer`, and its + `path` is **normalized** — the type's own doc says so + (`:118-130`) and warns that this is not free, because normalization + otherwise happens inside `set_buffer_path` (`:889-890`), which never + runs on this arm. **A handler keying state by path gets the canonical + form**, which is what makes dired's `handle_for_path` dedup agree with + it. Stage 2's reconciliation must normalize on the same seam or it + will miss dired handles. +- **Dired is a replaceable fallback slot, not a hook subscriber.** The + chain is the `path.open-directory` hook (`builtin/hooks/default.lua:65`), + and dired registers through `pmacs.path.set_directory_handler` + (`dired.lua:736-738`; binding at `src/lua_bindings/mod.rs:3679`, + readable back as `pmacs.path.directory_handler`, `:3677`). The + module's own comment states the reason: `HookRegistry::add` only + appends and builtins load before `init.lua`, so a dired subscription + would always claim first and no user listener could ever win. + **Setting the slot to `nil` disables directory opening entirely**, and + `journey_unclaimed_directory_starts_successfully_with_a_status` pins + that this is a status message rather than a failure. Nothing in Stage + 2 may assume dired is what opened a directory. +- **`open_directory` now commits under a captured destination, and that + scope refuses an `await`.** The mutating half of `open_directory` is a + `commit()` closure (`dired.lua:656-693`) run through + `pmacs.window.commit_to` when `opts.dest` is present (`:703-707`); the + `read_listing` await is deliberately **outside** it, and the comment + at `:653-655` records why: awaiting inside a commit is refused + (Q#JR14b), because a yield would restore the scope while the coroutine + is still parked. `journey_acceptance.rs`'s + `commit_to_refuses_an_await_and_restores` pins it. **§9's + serialize-and-await batch therefore cannot run inside a commit + scope** — see §9. + +### The journey ratchet (N3) + +`tests/journey_acceptance.rs` — **24** `#[test]`s, 0 `#[ignore]`, 0 +feature-gated, 2 under `#[cfg(target_os = "linux")]`. Its module doc at +`:12` is the rule: *"**This file is a ratchet: stages add rows, none +removes them.**"* Two further disciplines it states, both of which bind +Stage 2's own acceptance: + +- **Drive the real entry point** (`:16-19`): "a directory arm with no + production caller passes every direct-call test". The suite goes + through `EditorState::open` / `open_directory_target` + (`src/editor.rs:999`), never `resolve_target_buffer`. +- **Pump to quiescence, never to a frame count** (`:20-22`), because + every listing is worker-dispatched. Stage 2's batches are more + worker-dispatched still. + +Seven rows assert on dired directly, including that a failed listing +leaves **no** `*dired:` buffer behind, that a declining resolver chain +still falls back to dired, and that `q` returns to the *destination* +window's origin buffer. **The GPU row is in a different file** — +`tests/gpu_invocation_acceptance.rs`'s +`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`, +added by #183 through a real daemon child process. Stage 2 must keep +both green and must not remove a row from either (§14). + +### The typed-edit chain, and why Stage 2 stays outside it (N4) + +Dired participates in neither `pmacs.typed_edit` nor +`buffer.after-edit`: a grep of `dired.lua` for +`typed_edit|after-edit|after_edit` returns **0**, and it registers no +`pmacs.hook.add` at all. Nor does the generated-buffer write drag it in +— `set_generated_contents`'s binding runs `notify_buffer_edit_to_windows` +and nothing else, and `EditorCore::notify_buffer_edit` +(`src/editor_core.rs:1814-1828`) runs no Lua hook. **So Q#DR25 does not +put dired writes on the chain**, which is the answer to the obvious +worry about N1 and N4 interacting. + +What *does* inherit from the chain is §5's two new hooks, because they +are new fan-outs. The relevant facts: + +- `resource.renamed` / `resource.deleted` will be declared in + `builtin/hooks/default.lua`. The existing edit hook there is + `all-must-succeed`, and **`run_all_must_succeed` (`src/hook.rs:332`) + iterates every callback and collects errors — it does not abort the + fan-out.** A reconciliation subscriber may therefore not rely on a + raising peer to stop the sequence, and §5's ordered LSP teardown must + be internally ordered rather than ordered-by-registration. +- **A chain consumer cannot both edit and let a later consumer act + meaningfully**, because the record handed to consumers is a copy taken + before any ran (`builtin/runtime/typed_edit.lua:132`, `:145-159`). + The analogue for §5 is direct: `resource.renamed` carries **paths**, + not a rebind list, and a subscriber that re-reads editor state sees + whatever earlier subscribers did. §5's LSP and dired subscribers are + independent by construction — one touches URI-keyed stores, the other + touches handles — and this framing asserts that independence rather + than assuming it. +- **`buffer.after-edit` fan-outs nest** (`builtin/runtime/lean_input.lua:232-241`). + Stage 2 fires no `buffer.after-edit`, so this does not bind directly; + it is recorded because the reconciliation runs inside + `pmacs._async._tick`, and a subscriber that edits a buffer there is + one `pmacs.hook.run` away from re-entering a fan-out it is inside. --- @@ -479,6 +907,43 @@ draw: behavior, and the alternative silently converts flags into marks. `U` clears both. +### 3.1 The listing becomes a genuinely immutable generated buffer (Q#DR25, new in rev 5) + +Stage 1 protected the listing with an erroring intercept over a writable +rope, which was the best available idiom when #165 landed. #178 replaced +that idiom. Stage 2 adopts the replacement, in `dired.lua` only: + +- `paint` becomes + `pmacs.buffer.set_generated_contents(handle.buf, render_text(handle))`, + dropping the `bypass_intercept` replace entirely. +- `claim_handle`'s `pmacs.buffer.add_intercept` (`dired.lua:509-511`) + is **removed**. It becomes redundant: the rope-level `read_only` the + primitive asserts refuses ordinary edits, undo, redo, and remote CRDT + imports alike, which is strictly more than the intercept refused. +- `pmacs.buffer.set_round_trip_input(buf, true)` (`:516`) **stays**, per + §2 — the two protections cover different copies and neither implies + the other. + +**Why this belongs in Stage 2 rather than a standalone fix.** It is a +three-line change to one file with no Rust at all, and it touches the +exact function every Stage 2 operation calls. Landing it separately +would mean two PRs racing on `paint`. It goes at the **head of 2b** +(§10), before the mark layer, so that every mark-layer acceptance +exercises the new write path rather than the old one. If the user would +rather see it land on its own — it is, after all, a live defect on a +step-3 journey surface — it detaches cleanly and 2b rebases onto it; +this framing states the preference, not a constraint. + +**The acceptance trap, stated because it would otherwise be missed.** +`tests/dired_acceptance.rs`'s +`dired_buffer_is_read_only_and_round_trips_input` (`:969`) asserts +`status(&s).contains("read-only")`. `BufferError::ReadOnly` renders as +``buffer `{name}` (id {id:?}) is read-only`` (`src/buffer.rs:1794`), so +**that test passes both before and after the swap** — it has no bite +against Q#DR25 and must not be mistaken for coverage of it. The pin that +bites is a new one: **`M-x buffer.undo` on a dired listing leaves the +listing intact** (§13 item 48), which fails against `main` today. + --- ## 4. Target sets: which operations are set-based (Q#DR13) @@ -531,13 +996,28 @@ a **transaction across every consumer that holds the path**. ### The owners, all verified -| Owner | Held as | Stale after a rev-1 rebind | -|---|---|---| -| Buffer path | `Buffer.file_path` | fixed | -| Buffer **name** | `Buffer.name` | **yes** — `set_buffer_path` never calls `set_name`, which documents itself as for "save-as and rename operations" (`buffer.rs:452`). Statusline and buffer list keep the old filename | -| LSP attachment | `rec.uri`, cached per buffer (`lsp.lua:826-833`) | **yes** — didChange (`:418`), semantic tokens (`:776-790`), diagnostics (`:953`), signature (`:1016`), definition (`:1648`), references (`:1757`) all keep firing at the old URI | -| dired handles | `handle.path` in Lua; the buffers are **pathless** | **yes, and unreachable** — no buffer-keyed rebind can ever find them | -| Workspace-edit origin | `origin = active_buffer_path()` (`lsp.lua:1252`) — a **string** (`:471-473`) | **yes, and it materializes a phantom** — `find_or_open(origin)` (`:1266`) on a renamed-away path hits `resolve_target_buffer`'s `NotFound` arm, which creates "an empty path-backed buffer" (`editor_core.rs:876-878`) and selects it. **No transaction can fix this one**, because the stale value is a captured Lua local, not editor state (G1) | +Rev 4 listed five. The re-scout confirms all five and finds a **sixth** +(W5); two further owners are *persisted* rather than in-memory and are +named in §11 rather than fixed here. + +| # | Owner | Held as | Stale after a rev-1 rebind | +|---|---|---|---| +| 1 | Buffer path | `Buffer.file_path` | fixed | +| 2 | Buffer **name** | `Buffer.name` | **yes** — `set_buffer_path` never calls `set_name`, which documents itself as for "save-as and rename operations" (`buffer.rs:453`). Statusline and buffer list keep the old filename | +| 3 | LSP attachment | `rec.uri`, cached per **buffer** in the `attachments` table (`lsp.lua:340`, record built at `:861-868`) | **yes** — read at **57 lines** in `lsp.lua` (W4), among them didChange (`:418`), semantic tokens (`:799-813`), the diagnostic attach (`:1039`), signature (`:1102-1108`), definition (`:1922-1937`), references (`:2031-2047`), all firing at the old URI. Buffer-keyed, so **one** rebind reaches all 57 | +| 4 | dired handles | `handle.path` in Lua; the buffers are **pathless** | **yes, and unreachable** — no buffer-keyed rebind can ever find them | +| 5 | Workspace-edit origin | `origin = active_buffer_path()` (`lsp.lua:1338`) — a **string** (`:471-473`) | **yes, and it materializes a phantom** — `find_or_open(origin)` (`:1352`) on a renamed-away path hits `resolve_target_buffer`'s `NotFound` arm, which creates an empty path-backed buffer (`editor_core.rs:980-989`) and selects it. **No transaction can fix this one**, because the stale value is a captured Lua local, not editor state (G1) | +| 6 | **Lean file progress** (NEW, W5) | `M.file_progress[uri]`, a **URI-keyed Lua module table** (`lean.lua:646`, written `:651`) | **yes, and `forget_uri` cannot reach it** — it is in no Rust store. Populated from `$/lean/fileProgress`; its own comment says Stage 5's goal view reads it to tell "no goals" from "not done yet". Reachable only from a **`resource.renamed` Lua subscriber**, which is an independent argument for the hook | + +**Owner 6 is the load-bearing addition, and not because Lean matters +here.** It is evidence that the Rust-side `forget_uri` (below) is +structurally incapable of being complete: any package may key state by +URI in its own module table, and the LSP manager will never know. The +hook is not a convenience for dired — it is the only mechanism that +scales, and owner 6 is the first case that proves it outside dired. +`lean.lua`'s subscriber is **not** in Stage 2's scope (Lean's arc owns +it); what Stage 2 owes is a hook whose contract makes writing one +possible, which is why the hook carries `(old, new)` paths. ### One transaction, two callers, one notification @@ -563,8 +1043,8 @@ survived being "fixed" once already. **Then one hook, fired once per rename: `resource.renamed(old_path, new_path)`.** The mechanism exists — -`run_hook_if_defined` (`mod.rs:1596`) fires Rust-side, `pmacs.hook.run` -(`:5883`) fires Lua-side — but **no rename or delete hook exists today**; +`run_hook_if_defined` (`mod.rs:1623`) fires Rust-side, `pmacs.hook.run` +(`:6005`) fires Lua-side — but **no rename or delete hook exists today**; the whole set is `buffer.{after-edit,after-load,after-save,after-switch, before-save,save,self-insert}`, `editor.before-quit`, `frontend.detached`, `process.after-tick`. Stage 2 adds this one. @@ -577,7 +1057,7 @@ reconcile from `(old, new)` alone. Rev 2 said "recompute `rec.uri` and didClose/didOpen". Rev 3 added five stores. Both were sized from a partial inventory: `LspManager` -(`src/lsp.rs:741-819`) holds **fourteen** URI-bearing store families, and +(`src/lsp.rs:741-830`) holds **fourteen** URI-bearing store families, and two more things keyed the same way. **The complete inventory**, from the struct itself: @@ -603,43 +1083,83 @@ Plus **`documents: HashMap<(LspServerId, String), String>`** (`:810`), the latest full text per `(server, uri)` — which is what `didChange` diffs against, so a stale entry under the old URI is a *correctness* problem, not just a leak. And **`pending_routes`** (`:797`), whose `ResponseRoute` -variants **carry the URI**: fifteen insert sites at `:1684-2133` -(`Completion`, `Hover`, `Signature`, `Definition`, `Locations`, -`DocumentSymbol`, `DocumentHighlight`, `Formatting`, `Rename`, -`PrepareRename`, `CodeAction`, `InlayHint`, `SemanticTokens` ×2, -`SemanticTokensDelta`). +variants carry the URI. -**That last one is why "clear the stores" is not enough** (H2): a response +**The route arithmetic, corrected (W3).** `ResponseRoute` +(`src/lsp.rs:835-859`) has **15 variants**, of which **14 carry a +`uri`**: `Completion`, `Hover`, `Signature`, `Definition`, `Formatting`, +`Rename`, `PrepareRename`, `CodeAction`, `InlayHint`, `SemanticTokens`, +`SemanticTokensDelta`, `Locations { uri, kind }`, `DocumentSymbol`, +`DocumentHighlight`. The fifteenth is **`WorkspaceSymbol { query }`**, +which carries **no URI at all** — its own comment explains that the +query stands in for the doc URI in the supersede key +(`src/lsp.rs:1887-1888`). There are **16** insert sites, every one in a +`request_*` method, spanning `src/lsp.rs:1683-2132`; **15 of the 16 +insert a URI-bearing route**, the two `SemanticTokens` inserts coming +from `request_semantic_tokens` (`:2073`) and +`request_semantic_tokens_range` (`:2108`), and the sixteenth being +`request_workspace_symbol` (`:1889`). *Rev 4 said "fifteen insert +sites"; the fifteen was a correct count of the URI-bearing ones and an +incorrect count of the sites.* **The purge predicate must therefore +answer for a variant with no `uri` field** — `WorkspaceSymbol` is +retained unconditionally, because a workspace-symbol query is not scoped +to any document and a rename does not invalidate it. + +**And that is why "clear the stores" is not enough** (H2): a response already in flight when the rename happens routes on arrival and **repopulates the old key after the clear**. Clearing without purging in-flight routes is a race that reintroduces exactly the state it removed. **One manager-level method, `forget_uri(sid, uri)`**, doing all of it — because fourteen call sites at the Lua layer is how one gets forgotten. -Its shape is modelled directly on the **server-scoped** teardown that -already exists (`restart`, `:1316-1331`), which does the same three things -one axis over: +Its shape is modelled on the **server-scoped** teardowns that already +exist. **There are two of them, and rev 4 named neither correctly (W2):** +`LspManager::start_generation` (`src/lsp.rs:1307-1345`, the restart- +generation flip) and `LspManager::forget` (`src/lsp.rs:3015-3042`, the +terminal-state removal). There is no `fn restart`. Both do the same +three things one axis over: 1. **Purge `pending_routes`** whose route carries this URI — - `retain`, mirroring `:1324`'s `retain(|(sid, _), _| *sid != id)`. -2. **Drain-cancel their awaiters.** `pending_external` holds the - `Handle:await()` side, and the existing contract is explicit that it is - "drained-cancelled wherever `pending_routes` is purged" (`:799-803`); - `drain_external_cancelled` (`:1596`) is the existing sweep. Skipping - this hangs any coroutine awaiting a request against the old URI. + `retain`, mirroring `start_generation`'s `:1324` + `retain(|(sid, _), _| *sid != id)` and `forget`'s `:3028`. + Per W3, the predicate must retain `WorkspaceSymbol` explicitly. +2. **Drain-cancel their awaiters — and this is where rev 4 pointed at + the wrong function (W1).** `pending_external` (`:804`) holds the + `Handle:await()` side, and the contract at `:801-803` is explicit + that it is *"drained-cancelled wherever `pending_routes` is purged"*. + The sweep to model on is **`drain_external_cancelled` + (`src/lsp.rs:1561-1576`)** — server-scoped and **unconditional**, + settling every awaiter for `sid` cancelled. It is **not** + `drain_cancelled_externals` (`src/lsp.rs:1596-1645`), which despite + the near-identical name is a per-tick, per-awaiter sweep that removes + only awaiters whose `CancellationToken` was flipped or which outlived + `request_timeout`. **A rename flips no token**, so modelling on the + second yields a drain that silently does nothing and leaves any + coroutine awaiting against the old URI parked forever. + Note that **neither existing sweep is URI-scoped** — both range over + `sid`. `forget_uri` needs a new shape: collect the `rid`s whose + `pending_routes` entry carries the old URI, then settle + `pending_external[(sid, rid)]`'s awaiters cancelled via + `runtime.complete_external_cancelled`, exactly as + `drain_external_cancelled` does per key. The route→awaiter join is + the `rid`; there is no other index. 3. **Clear all fourteen stores plus `documents`** for the old key. Each store already has a keyed `clear` (`diag.rs:262`, `hover.rs:160`, `completion.rs:331`, `semantic_tokens.rs:263`, …). Note the two irregular keys: `locations_store` is **kind**-keyed, so all four kinds must go; `symbol_store` is **scope**-keyed and holds workspace symbols - too, so only the document-scoped entry is dropped. + too, so only the document-scoped entry is dropped — the same + asymmetry that makes `WorkspaceSymbol` route-exempt in step 1. -**Worth stating because it is surprising:** the server-restart teardown -clears `deferred_notifications`, `pending_routes`, `documents`, and drains -externals — but **not** the fourteen result stores. Rev 4 does not change -that (it is a separate pre-existing question about whether stale results -should survive a restart), but it does mean `forget_uri` has no per-server -precedent to copy for the store half, only the route/document half. +**Worth stating because it is surprising, and it is now true twice:** +*neither* server-scoped teardown clears the fourteen result stores. +`start_generation` clears `deferred_notifications`, `pending_routes`, +`documents`, and drains externals; `forget` clears `pending_routes`, +`documents`, externals, `status_tracker`, and `project_servers`. Rev 5 +does not change that (it is a separate pre-existing question about +whether stale results should survive a restart), but it does mean +`forget_uri` has **no** precedent to copy for the store half, only for +the route/document/drain half. Then the ordered sequence, per attachment: @@ -660,7 +1180,7 @@ Then the ordered sequence, per attachment: `DiagnosticView.uri` is **set once at construction** and the field's own doc anticipates exactly this: *"M5 may add re-rooting if a buffer is -renamed"* (`src/diag.rs:455-457`). Rev 3 left it as "either a `set_uri` or +renamed"* (`src/diag.rs:456-457`). Rev 3 left it as "either a `set_uri` or tear down and re-attach", which round 3 rightly rejected: neither option explained how **passive** windows are reached, and the field is private while `View` has **no downcast**, so an outside caller cannot reach it at @@ -668,7 +1188,7 @@ all. Two further facts make the naive options worse: - **`_attach_view` reaches only the active window.** It takes `core_borrow.active_window_mut()` and **errors** if that window is not - showing the buffer (`lua_bindings/diag.rs:216-224`). So a + showing the buffer (`lua_bindings/diag.rs:218-224`). So a re-attach-per-window loop cannot be driven from Lua, and a passive split never got its overlay from this path in the first place. - **A remove-and-re-push loses composition order.** Overlays are an @@ -704,7 +1224,7 @@ for win in core.windows.values_mut() { } ``` -(`lua_bindings/mod.rs:2016-2019`.) The rename sweep is the same traversal +(`lua_bindings/mod.rs:2044-2046`.) The rename sweep is the same traversal with `retain` replaced by a call to `rename_resource` — so **every** window showing the buffer is reached, active or passive, in one pass, and order is preserved because nothing is removed or re-pushed. @@ -735,7 +1255,7 @@ it is the right call over the alternative: - `Buffer::set_name` **already exists** in Rust and already documents itself as "used by save-as and **rename operations**" - (`src/buffer.rs:452`). Nothing new is being invented; an existing, + (`src/buffer.rs:453`). Nothing new is being invented; an existing, purpose-built setter is being exposed. - §5 needs the same capability anyway for the `Buffer.name` half of the transaction, so the Rust-side name update is in scope regardless. The @@ -756,7 +1276,7 @@ that `handle.path` changed. #### The workspace-edit applier (G1) `apply_workspace_edit` captures `origin` as a **string** and restores with -`find_or_open(origin)` (`lsp.lua:1252`, `:1266`). No amount of +`find_or_open(origin)` (`lsp.lua:1338`, `:1352`). No amount of reconciliation reaches an already-captured Lua local, so the phantom survives every design above. The applier itself must change: @@ -776,7 +1296,7 @@ resumes**, or a coroutine that renamed and then inspects a buffer sees the pre-rename state. Verified: `pmacs._async.tick` calls `async_mod._tick()` first and only then walks the settled ids firing `on_complete` callbacks and resuming parked coroutines -(`async.lua:389-391`). So doing the work inside `_tick`'s Rust closure is +(`async.lua:401-421`). So doing the work inside `_tick`'s Rust closure is correctly ordered by construction, not by luck. ### The harvest (F6) @@ -833,7 +1353,7 @@ exists — and **saving it recreates the file the user just deleted**. Rev 2 wrote dired's policy and stopped, leaving the LSP path untouched and unmentioned. Verified: `apply_resource_op`'s delete arm -(`mod.rs:3256-3285`) deletes, then kills the buffer via +(`mod.rs:3313-3341`) deletes, then kills the buffer via `reg.borrow().find_by_path(&pb)` — the **same raw-path, first-match** lookup §5 fixes for rename, with **no descendant handling and no modified check**. So an LSP-authored delete **destroys unsaved work @@ -902,7 +1422,7 @@ save-or-discard and retry. ### Checked twice — and still only best-effort (G4, narrowed H1) -`buf:is_modified()` is exposed to Lua (`mod.rs:1234`), so dired decides +`buf:is_modified()` is exposed to Lua (`mod.rs:1261`), so dired decides this itself. It checks **twice**: - **Before the confirm**, so the prompt states the skip up front: @@ -1025,7 +1545,7 @@ confirm. > **It needs a public kill-ring entry point, which does not exist (G5).** > `push_entry` is a `local function` (`killring.lua:93`) and the public > `copy()` requires a region — it calls `ed.region()` and fails with "no -> region" otherwise (`:182-190`). Rev 3 adds +> region" otherwise (`:184-194`). Rev 3 adds > **`pmacs.killring.push(text)`** (Q#DR22) with exactly the semantics > `copy()` already establishes for non-kill text: `push_entry`, mirror to > the OS clipboard via `ed.clipboard_set`, and **break the kill chain** @@ -1097,7 +1617,22 @@ The contract: wholesale repaint and N repaints for N deletions is both slower and visibly wrong. - **Every post-`await` re-seat checks `pmacs.window.buffer()` first** - (`dired.lua:399-404`), the rule #165's review round produced. + (`dired.lua:400-404`), the rule #165's review round produced. +- **No part of a batch may run inside `pmacs.window.commit_to`** (new in + rev 5, from N2). The commit scope **refuses an `await`** (Q#JR14b) — + `commit_to_refuses_an_await_and_restores` pins the raise, whose + message contains both `"cannot await inside"` and `"commit_to"` — and + a batch is defined by awaiting each op in turn. Stage 1 already solved + the same problem the same way: `open_directory` does its + `read_listing` await **outside** the commit and puts only the + non-yielding mutation inside (`dired.lua:640-655`). Stage 2's + operations follow that shape — await the whole batch first, then, if a + destination was captured, do the repaint and re-seat inside a commit. + Stage 2's operations are all initiated from an already-active dired + buffer rather than from a captured destination, so in practice + `opts.dest` is nil on these paths and the ambient rule above governs; + the constraint is stated because the *shape* must not be copied + wrongly if that ever changes. --- @@ -1109,19 +1644,27 @@ called it, and the three rounds of findings on the reconciliation half are the evidence — every one of G1, G3, H1, H2, and H3 was about the transaction, not about marks. +**Rev 5 keeps the three-PR cut unchanged** and adds Q#DR25 to the head +of 2b. The cut was re-examined against the re-scout and still holds: 2a +is still substrate-only, 2b is still the dired surface, 2c is still the +three additive primitives, and Stage 3 is still wdired. Nothing that +landed in the 153 commits moves work across those lines — #178 lands +inside 2b (it is dired-local), #182 changed the *shape* of dired's entry +point but added no Stage 2 work, and #179/#181 added none. + | | **2a — reconciliation** | **2b — marks and operations** | **2c — new primitives** | |---|---|---|---| -| User-visible surface | **none** | `m u U t d x D R w M` | `+ C`, recursive delete | -| Rust | `reconcile_rename`, `reconcile_delete`, `TickOutcome`, `PendingJob` paths, `forget_uri` (14 stores + `documents` + route purge + drain), `View::rename_resource` + the window sweep, `apply_resource_op` (rename **and** delete arms), `apply_workspace_edit` origin, `pmacs.buffer.set_name` | `pmacs.killring.push` | `mkdir`, `copy`, `remove_dir_all`; `JobKind` 12 → 15 | -| Lua | the two hook subscribers in `lsp.lua` | all of `dired.lua`'s mark/op layer, `minibuffer.lua` | two ops | +| User-visible surface | **none** | `m u U t d x D R w M`, plus a listing that survives `M-x buffer.undo` | `+ C`, recursive delete | +| Rust | `reconcile_rename`, `reconcile_delete`, `TickOutcome`, `PendingJob` paths, `forget_uri` (14 stores + `documents` + route purge + URI-scoped drain), `View::rename_resource` + the window sweep, `apply_resource_op` (rename **and** delete arms), `apply_workspace_edit` origin, `pmacs.buffer.set_name` | `pmacs.killring.push` | `mkdir`, `copy`, `remove_dir_all`; `JobKind` 12 → 15 | +| Lua | the two hook subscribers in `lsp.lua` | **Q#DR25 (`paint` → `set_generated_contents`, drop the intercept)**, then all of `dired.lua`'s mark/op layer, plus `minibuffer.lua` | two ops | | Config keys | none | none | `dired.recursive-deletes` | -| Acceptance | items 25–38 | 1–24, 39–41 | 42–47 | +| Acceptance | items 25–38 | 1–24, 39–41, **48–49** | 42–47 | **Why 2a first, with no dired surface at all.** It is a self-contained substrate correctness fix that stands on its own merits: it closes a path where an LSP-authored delete **destroys unsaved work today** -(`mod.rs:3256-3285`), and one where renaming the active file through a -workspace edit **materializes a phantom buffer** (`lsp.lua:1266`). Neither +(`mod.rs:3313-3341`), and one where renaming the active file through a +workspace edit **materializes a phantom buffer** (`lsp.lua:1352`). Neither needs dired to be worth fixing, and neither is dired's fault. Landing it alone also means the LSP lifecycle work — a fourteen-store inventory, an in-flight route purge, an awaiter drain, and a new `View` hook swept across @@ -1179,12 +1722,12 @@ that deserve an undivided reviewer. deciding what a partially-applied workspace edit does. - **A passive window that never received a `DiagnosticView` still has none** (H3). `_attach_view` takes `active_window_mut()` and errors - otherwise (`lua_bindings/diag.rs:216-224`); `ensure_overlay` + + otherwise (`lua_bindings/diag.rs:218-224`); `ensure_overlay` + `clone_for_split` cover split-from-attached, not attach-while-passive. Pre-existing, and rename cannot re-root an overlay that was never attached. - **The server-restart teardown does not clear the fourteen result - stores** (`lsp.rs:1316-1331` clears routes, documents, and deferred + stores** (`lsp.rs:1307-1345` clears routes, documents, and deferred notifications only). Pre-existing; whether stale results should survive a restart is its own question, and `forget_uri` deliberately does not answer it. @@ -1192,18 +1735,49 @@ that deserve an undivided reviewer. same class as the config-sink race fixed in #174, not observed failing, and the obvious fix would trade a precise regression diff for a vague timeout. Needs a record terminator in the fake server first. -- **Recursive copy** — 2b refuses directory sources; a real `copy -r` +- **Recursive copy** — 2c refuses directory sources; a real `copy -r` primitive is separate. +- **The other three non-adopters of `set_generated_contents`** (new in + rev 5). Q#DR25 closes dired's quarter of the handoff §4 / `COHERENCE.md` + §14 inventory. The remaining three — listview panels + (`builtin/runtime/listview.lua:60-61`), `compile.lua`'s `ensure_slot` + serving `*compilation*` **and** `*shell-command*`, and the independent + `*search-results*` panel (`builtin/commands/default.lua:869`) — stay + emptiable. Listview is the other cheap half (whole-buffer replace); + the three appending writers need a **streaming variant of the + primitive that does not exist**, which is why this is a lane and not a + rider. Note the handoff's warning: `ensure_slot` does **not** cover + `*search-results*`, and `*workers*`/`*help*`/`*buffer-list*` are + generated but do not use this idiom, so they are not in scope either. +- **Two persisted path owners, named not fixed** (rev 5, W5). + `saveplace.lua` keys its places file by path (`load_places`, `:31-45`; + `restore_active`, `:69-79`) and `recentf.lua` keeps a path MRU list + (`record`, `:36-46`). Neither is an in-memory owner — both rebuild + their index from `pmacs.state` on every call — so a rename leaves a + stale *line on disk*, not stale editor state. The consequence is + bounded and self-correcting: entries are capped and LRU-evicted, and + the worst case is a future unrelated file at the old path restoring a + wrong cursor. Reconciling them would mean teaching the reconciliation + transaction to rewrite persisted state, which is a persistence-arc + question. A `resource.renamed` subscriber in each is the cheap fix + whenever someone wants it. +- **`lean.lua`'s `M.file_progress` is not re-rooted** (rev 5, owner 6). + Stage 2 supplies the hook that makes a subscriber possible and does + not write one — Lean's arc owns that file, and Stage 5's goal view is + the consumer that would notice. --- ## 12. Bets -- **B1.** 2a needs **no** new `pmacs.fs` op. Falsified if any of +- **B1.** **2b** needs **no** new `pmacs.fs` op. Falsified if any of `m u U t d x D R w M` cannot be built on the existing five. *(Rests on - C5, which was read off `remove_blocking` directly.)* + C5, which was read off `remove_blocking` directly.)* *(Rev 5: rev 4 + wrote "2a" here, which contradicted its own §10 table — 2a ships no + dired surface at all, so it cannot need an op for a key it does not + bind. Corrected label; the bet is unchanged.)* - **B1b.** The two new hooks need no new hook machinery — - `run_hook_if_defined` (`mod.rs:1596`) and `pmacs.hook.run` (`:5883`) + `run_hook_if_defined` (`mod.rs:1623`) and `pmacs.hook.run` (`:6005`) already exist, and `resource.renamed`/`resource.deleted` are ordinary names in that registry. Falsified if firing a hook from the reconcile path needs a new dispatch mechanism. @@ -1226,6 +1800,23 @@ that deserve an undivided reviewer. - **B5.** `describe_key_identifies_every_default_binding` stays green without further surgery — #165 already taught it per-binding mode context, and Stage 2 only adds more bindings in the same mode. +- **B6** *(new in rev 5, N1)*. Q#DR25 needs **no Rust**: the primitive, + its Lua binding, and the fan-out all exist + (`src/buffer.rs:545`, `src/lua_bindings/mod.rs:3079-3095`). Falsified + if adopting it requires a new binding, a `set_read_only` exposure, or + any change outside `builtin/runtime/dired.lua`. *Not obvious in one + respect:* dropping the intercept removes the only thing that currently + produces dired's refusal **message**, so if any Stage 1 acceptance + depends on the intercept's exact wording rather than on the substring + `read-only`, this bet fails. §3.1 checked the one test that looked + like a risk and it does not. +- **B7** *(new in rev 5, N3)*. The journey ratchet stays at **≥ 24** + rows in `tests/journey_acceptance.rs` and keeps its GPU row in + `tests/gpu_invocation_acceptance.rs`, with no row weakened. Falsified + by any deletion, `#[ignore]`, or assertion relaxation in either — and + the ratchet's own doc says a green test that cannot fail must be + deleted rather than kept, so "still green" is not the bet; "still + biting" is. --- @@ -1382,6 +1973,33 @@ that deserve an undivided reviewer. 47. Recursive delete happens only with `dired.recursive-deletes` enabled **and** a confirm; disabled, the non-empty directory still fails. +**2b — the generated-buffer invariant (§3.1, Q#DR25, new in rev 5)** + +48. **`M-x buffer.undo` on a dired listing leaves the listing intact.** + Driven through `pmacs.command.invoke` (or the M-x path), not through + a chord — the command is reachable with no binding, which is the + whole point. *(This is the pin with bite: it fails against `main` + today, where the listing is emptied.)* Its sibling asserts **redo** + is equally refused, since `Buffer::redo` takes the same + `ensure_writable` path. +49. **A repaint after the swap still reaches a displaying window.** Mark + an entry in a dired buffer shown in **two** windows and assert the + mark column changed in **both** — the fan-out obligation the + handoff attaches to the primitive. *(An implementation that called + `Buffer::set_generated_contents` from Rust and swallowed the `Edit` + would paint stale ranges; the Lua binding discharges this at + `mod.rs:3092`, and this pins that the Lua path is the one used.)* + Run under **both** default and `crdt` features: `clear_history` + clears loro's `UndoManager` only under `crdt`, so the default run + does not exercise that half at all. + +Note explicitly what does **not** count as coverage of Q#DR25: +`dired_acceptance.rs`'s existing +`dired_buffer_is_read_only_and_round_trips_input` (`:969`) passes both +before and after the swap, because `BufferError::ReadOnly` renders with +the substring `is read-only` and the test asserts +`status(&s).contains("read-only")` (§3.1). + **Bite obligations.** Each of these must fail against a stated mutation: | Item | Mutation it must catch | @@ -1398,6 +2016,8 @@ that deserve an undivided reviewer. | 34 | the applier restoring by path instead of by buffer handle | | 39 | a completion source added to `confirm` | | 40 | the batch changed to dispatch-all-then-await | +| 48 | `paint` reverted to `bypass_intercept` over a writable rope (i.e. `main` today) | +| 49 | the `Edit` returned by `set_generated_contents` swallowed instead of fanned out | `dired.lua` is an existing file now, so `scripts/bite`'s swap-over-`git show` mode applies — but per #165's lesson, **commit @@ -1421,6 +2041,34 @@ crdt`; `dired_acceptance` (default **and** `crdt`); **`m8_1`, `m8_2`, -p pmacs-gpu`; the isolated-`XDG_CONFIG_HOME` workspace sweep with `--no-fail-fast`; `git diff --check`. +**Two gates rev 4 could not have listed, both required of every PR in +this stage (N3):** + +- **`journey_acceptance` at ≥ 24 tests, all passing.** It is a ratchet + by its own declaration, seven of its rows assert on dired, and its + step-3 and step-5 rows run *through* the surface Stage 2 modifies. + **Assert the count, not just the colour** — a row silently dropped is + exactly what the ratchet exists to prevent, and Q#DR25 changes how + dired's buffer is written underneath those rows. +- **`gpu_invocation_acceptance`**, which is where #183 put the GPU + journey row + (`public_gpu_directory_target_reaches_dired_and_leaves_the_daemon_usable`). + It drives a **real daemon child process**, so it is in the blast + radius of any change to how a dired buffer is written or protected — + Q#DR25 makes the listing rope-level `read_only`, and a daemon-side + refusal is exactly what that test's frontend would see. Note this row + is **not** in `journey_acceptance.rs`; running only that file leaves + the GPU half of the journey unpinned. + +Also add **`typed_edit_chain_acceptance` (13 tests)** to 2b's list, not +because dired joins the chain — it does not (§2, N4) — but because +Q#DR25 changes a `buffer.after-edit`-adjacent write path and the chain +is the tree's most order-sensitive consumer of edits. + +2b's `dired_acceptance` run is expected to **grow** by items 48–49 and +otherwise hold at its current **25**; a *drop* there means Q#DR25 broke +a Stage 1 pin rather than superseding one, and the two are not the same. + --- ## 15. Numbered decisions @@ -1516,21 +2164,35 @@ crdt`; `dired_acceptance` (default **and** `crdt`); **`m8_1`, `m8_2`, rename, which its own module doc says is why buffer-per-directory exists. Uniqueness stays the caller's job, matching the Rust setter. (§5) -- **Q#DR23** *(new in rev 4, H2)* The LSP rename teardown is **one - manager-level `forget_uri(sid, uri)`**, not per-store calls at the Lua - layer: fourteen call sites is how one gets forgotten. It purges - `pending_routes` by URI, **drain-cancels the matching awaiters** (the - existing contract at `lsp.rs:799-803` already requires this wherever - routes are purged), and clears all fourteen stores plus `documents`, - handling `locations_store`'s **kind** key and `symbol_store`'s **scope** - key specially. Modelled on the server-scoped teardown at - `lsp.rs:1316-1331`. (§5) +- **Q#DR23** *(new in rev 4, H2; corrected in rev 5, W1/W2/W3)* The LSP + rename teardown is **one manager-level `forget_uri(sid, uri)`**, not + per-store calls at the Lua layer: fourteen call sites is how one gets + forgotten. It purges `pending_routes` by URI, **drain-cancels the + matching awaiters** (the existing contract at `lsp.rs:801-803` already + requires this wherever routes are purged), and clears all fourteen + stores plus `documents`, handling `locations_store`'s **kind** key and + `symbol_store`'s **scope** key specially. + *(Rev 5 corrects three facts underneath it, none of which change the + decision but any of which would break the implementation:* the + teardowns to model on are **`LspManager::start_generation`** + (`lsp.rs:1307-1345`) and **`LspManager::forget`** (`lsp.rs:3015-3042`) + — there is no `fn restart`; the drain to model on is + **`drain_external_cancelled`** (`lsp.rs:1561-1576`), the unconditional + server-scoped one, **not** the near-namesake + `drain_cancelled_externals` (`lsp.rs:1596-1645`), which only reaps + token-cancelled and timed-out awaiters and would therefore drain + nothing on a rename; and the route purge must **retain + `WorkspaceSymbol` unconditionally**, since it is the one + `ResponseRoute` variant of fifteen that carries a `query` rather than + a `uri`. Neither existing drain is URI-scoped, so `forget_uri`'s drain + joins route to awaiter on the **`rid`**, which is the only index + available.*) (§5) - **Q#DR24** *(new in rev 4, H3)* Diagnostic re-rooting is a **`View::rename_resource(&mut self, old, new)`** default-no-op hook, joining `overlay_identity` and `clone_for_split` in the family #113 added for cross-window overlay problems, swept over `core.windows.values_mut()` exactly as overlay disposal already is - (`mod.rs:2016-2019`). Chosen over exposing `set_uri` (the field stays + (`mod.rs:2044-2046`). Chosen over exposing `set_uri` (the field stays private, and `View` has no downcast) and over tear-down-and-re-attach (which loses composition order, and `_attach_view` cannot reach a passive window at all). (§5) @@ -1539,6 +2201,24 @@ crdt`; `dired_acceptance` (default **and** `crdt`); **`m8_1`, `m8_2`, non-region text: push the entry, mirror to the OS clipboard, and **break the kill chain**. `push_entry` is private and `copy()` requires a region, so `w` has no surface without it. (§8) +- **Q#DR25** *(new in rev 5, N1)* **Dired's listing becomes a genuinely + immutable generated buffer.** `paint` writes through + `pmacs.buffer.set_generated_contents` instead of a + `bypass_intercept` `buf:replace`, and `claim_handle`'s erroring + intercept is **removed** as redundant — rope-level `read_only` refuses + strictly more (ordinary edits, **undo, redo**, and remote CRDT + imports). `set_round_trip_input` **stays**: the two protections cover + different copies, and a daemon-side refusal arrives after a semantic + frontend has already painted its own mirror. This is a correctness fix + to **Stage 1**, not new Stage 2 surface: on `main` today `M-x + buffer.undo` empties a dired listing, and dired is now a **step-3 + journey surface** (§0.5). It lands at the head of 2b because it is + dired-local, needs no Rust, and touches the one function every Stage 2 + operation calls — but it detaches cleanly if the user prefers it + standalone. The fan-out obligation is discharged by the existing Lua + binding (`mod.rs:3092`); the history-clearing obligation covers the + CRDT `UndoManager` too, which is why the `crdt` acceptance run is + load-bearing rather than routine. (§3.1, §10, §13 items 48–49) ## 16. Branch and PR plan @@ -1564,7 +2244,34 @@ closes on `main` today — the LSP-authored delete that destroys unsaved work, and the workspace-edit phantom buffer — because neither needs dired to be worth fixing. -**Ledger note:** this framing branch deliberately touches **only** this -file. `docs/active-work.md` and `docs/agent-handoff.md` are held by the -open docs PR #169, and a second edit to them here would conflict for no -benefit; the Stage 2 lane goes in once #169 has landed. +**Ledger note (corrected in rev 5, W6):** this framing branch +deliberately touches **only** this file. Rev 4 said the durable records +were held by open PR #169; **#169 merged** as `74301d1`. The current +holder is **PR #185** (`docs-landed-state-184`), which owns +`docs/active-work.md`, `docs/agent-handoff.md`, and `COHERENCE.md`. A +second edit to any of the three from here would conflict for no benefit, +so the Stage 2 lane goes in once #185 has landed. *(This is the ledger +contention treadmill the ops lessons name: with several PRs open, every +merge re-conflicts the rest in `docs/active-work.md`. Integrate late.)* + +### Ownership warning: 2a must not run concurrently with Journey Stage 1b + +**2a overlaps `src/editor_core.rs`, `builtin/runtime/lsp.lua`, and the +URI-keyed LSP state with other coherence work in flight.** +`COHERENCE.md` §20 names Journey Stage 1b as Priority 1's remainder — +compile defaults, **LSP spawn-failure surfacing**, bindings, and a +welcome buffer — and the LSP-failure half lands in the same files 2a +rewrites, while 1b's compile/binding half touches `editor_core.rs`. Two +branches editing `lsp.lua`'s attachment lifecycle at once is the +worst-case shape for this repo: the conflicts are semantic rather than +textual, so a clean `git merge` proves nothing. + +**Neither stage may start until those files are assigned.** The +resolution is cheap if taken up front — 1b's LSP work is a *reporting* +change (surface the spawn failure with guidance) and 2a's is a +*lifecycle* change (teardown and re-attach on rename), so they can be +sequenced in either order provided only one is open at a time. Taken +late it is three merge rounds, which this arc has already paid twice. + +Stage 2b and 2c carry no such overlap: 2b is `dired.lua` plus one +killring binding, and 2c is three additive `pmacs.fs` primitives.