From 4f6135263b9663258da44f6e3adb51dec841ede9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 17:38:21 -0400 Subject: [PATCH 01/12] wip(stage2a): name provenance plus the View rename hook `BufferNameOrigin` records where a buffer's name came from instead of inferring it from the string: a path-backed buffer's name is the path *as given*, so a relative open is named `foo.rs` while its stored path is absolute, and a user may legitimately choose a name that normalizes to its own file's path. Rename reconciliation asks the bit. Every path-backed creation site is audited onto the new `set_path_derived_name` door: `EditorCore::get_or_load_buffer`, the `NotFound` arm of `resolve_target_buffer`, `pmacs.buffer.from_file`, and `pmacs.buffer.find_or_open`. Ordinary `Buffer::set_name` records `Explicit`. `View::rename_resource` is the seam that re-roots a URI-keyed overlay in place, so it keeps its position in the window's composition order; `DiagnosticView` overrides it, whose `uri` is private and set once at construction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/buffer.rs | 60 ++++++++++++++++++++++++++++++++++++++++- src/diag.rs | 11 ++++++++ src/editor_core.rs | 16 +++++++++-- src/lua_bindings/mod.rs | 9 +++++++ src/view.rs | 14 ++++++++++ 5 files changed, 107 insertions(+), 3 deletions(-) diff --git a/src/buffer.rs b/src/buffer.rs index a9c01e9..3ccb5e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -148,6 +148,26 @@ struct EditDescription { inserted_len: u64, } +/// Provenance of a [`Buffer`]'s name (dired Stage 2a, Q#DR30). +/// +/// A rename must move a name that merely *renders* the file's path and +/// must leave a name the user chose alone. String inspection cannot +/// tell those apart — a user may legitimately name a buffer with a +/// string that normalizes to its own path — so the fact is recorded at +/// the moment the name is written instead of being reconstructed +/// later. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum BufferNameOrigin { + /// A caller named this buffer: `Buffer::new`/`from_bytes`, an + /// ordinary [`Buffer::set_name`], or the `pmacs.buffer.set_name` + /// binding. A rename leaves the name alone. + Explicit, + /// The name was derived from the buffer's backing path by a + /// path-backed creation site, through + /// [`Buffer::set_path_derived_name`]. A rename rewrites it. + PathDerived, +} + /// The unit of editable content: rope + identity + views + undo. /// /// # Threading @@ -159,6 +179,14 @@ pub struct Buffer { id: BufferId, rope: Rope, name: String, + /// Where [`Self::name`] came from. Recorded rather than inferred, + /// because a path-backed buffer's name is **not** reliably its + /// path: `get_or_load_buffer` takes the name from the path *as + /// given* and normalizes only the stored `file_path`, so a + /// relative open is named `foo.rs` while its path is absolute. + /// Rename reconciliation asks this bit, never the string + /// (dired Stage 2a, Q#DR30). + name_origin: BufferNameOrigin, /// The buffer's single active major mode, if one has been selected. major_mode: Option, is_modified: bool, @@ -247,6 +275,10 @@ impl Buffer { id, rope, name: name.into(), + // Construction names a buffer explicitly. A path-backed + // creation site re-records provenance through + // `set_path_derived_name` right after binding the path. + name_origin: BufferNameOrigin::Explicit, major_mode: None, is_modified: false, read_only: false, @@ -449,9 +481,35 @@ impl Buffer { &self.name } - /// Set the buffer's name. Used by save-as and rename operations. + /// Set the buffer's name, recording it as **explicitly chosen** + /// ([`BufferNameOrigin::Explicit`]). + /// + /// This is the user-facing door — `pmacs.buffer.set_name` and + /// save-as go through it — and it is deliberately explicit even + /// when the string happens to denote the file: naming a buffer + /// `notes` for `${cwd}/notes` is still a naming operation, and a + /// later rename must not overwrite it. Path-backed creation sites + /// use [`Self::set_path_derived_name`] instead. pub fn set_name(&mut self, name: impl Into) { self.name = name.into(); + self.name_origin = BufferNameOrigin::Explicit; + } + + /// Set the buffer's name **and** record that it was derived from + /// the buffer's backing path ([`BufferNameOrigin::PathDerived`]). + /// + /// Every site that creates or re-binds a path-backed buffer uses + /// this door, including rename reconciliation itself — so a second + /// rename still follows the path. + pub fn set_path_derived_name(&mut self, name: impl Into) { + self.name = name.into(); + self.name_origin = BufferNameOrigin::PathDerived; + } + + /// Where this buffer's name came from (dired Stage 2a, Q#DR30). + #[must_use] + pub fn name_origin(&self) -> BufferNameOrigin { + self.name_origin } /// This buffer's active major mode, if any. diff --git a/src/diag.rs b/src/diag.rs index 88aa6cc..ba67450 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -489,6 +489,17 @@ impl DiagnosticView { } impl View for DiagnosticView { + /// Re-root this view when the buffer's file was renamed (dired + /// Stage 2a, §5). The URI field is private and `View` has no + /// downcast, so this hook is the only way an outside sweep can + /// reach it — and mutating in place preserves this overlay's + /// position in the window's composition order. + fn rename_resource(&mut self, old_uri: &str, new_uri: &str) { + if self.uri == old_uri { + self.uri = new_uri.to_owned(); + } + } + fn kind(&self) -> &'static str { "diagnostic" } diff --git a/src/editor_core.rs b/src/editor_core.rs index 661b767..56ccfbf 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -938,11 +938,18 @@ impl EditorCore { } let normalized = normalize_buffer_path(path.to_path_buf()); let (bytes, meta) = crate::file_io::load_file(path)?; + // The name is the path **as given** — a relative open is named + // `foo.rs` while `file_path` below is absolute. Recording the + // provenance (Q#DR30) is what lets rename reconciliation move + // this name without having to guess from the string. let display_name = path.display().to_string(); let id = self .registry .borrow_mut() - .create_from_bytes(display_name, &bytes); + .create_from_bytes(display_name.clone(), &bytes); + if let Ok(b) = self.registry.borrow_mut().get_mut(id) { + b.set_path_derived_name(display_name); + } self.set_buffer_path(id, Some(normalized)); self.set_buffer_meta(id, Some(meta)); Ok((id, true)) @@ -1001,7 +1008,12 @@ impl EditorCore { }), Err(error) if error.kind() == std::io::ErrorKind::NotFound => { let display_path = path.display().to_string(); - let buffer_id = self.registry.borrow_mut().create(display_path); + let buffer_id = self.registry.borrow_mut().create(display_path.clone()); + // Path-backed creation site (Q#DR30): the name is the + // path, so a later rename may move it. + if let Ok(b) = self.registry.borrow_mut().get_mut(buffer_id) { + b.set_path_derived_name(display_path); + } self.set_buffer_path(buffer_id, Some(path.to_path_buf())); "[new file]".clone_into(&mut self.status); Ok(ResolvedTarget::Buffer { diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index d5a2b4b..bbdb7fa 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3244,6 +3244,11 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) @@ -3307,6 +3312,10 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) diff --git a/src/view.rs b/src/view.rs index 424fd9e..e3e4b6c 100644 --- a/src/view.rs +++ b/src/view.rs @@ -310,6 +310,20 @@ pub trait View { fn clone_for_split(&self) -> Option> { None } + + /// Retarget this overlay from `old_uri` to `new_uri` after a + /// resource rename (dired Stage 2a, §5). Default: no-op — a view + /// that renders nothing URI-keyed is unaffected. + /// + /// Mutates **in place**, so the overlay keeps its position in the + /// window's composition order. That is the reason this is a trait + /// hook rather than a remove-and-re-push at the call site: overlays + /// are an ordered `Vec` merged in sequence, and re-pushing would + /// move a diagnostic underline to the end of the stack. It is also + /// how *passive* windows are reached at all — the Lua attach path + /// (`pmacs.diag._attach_view`) can only touch the active window, + /// while the sweep that drives this walks every window. + fn rename_resource(&mut self, _old_uri: &str, _new_uri: &str) {} } // --------------------------------------------------------------------------- From f294942ef56dc3735a9bea392df17f97a6a50244 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:01:45 -0400 Subject: [PATCH 02/12] wip(stage2a): the reconciliation transaction and the URI teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One shared walk query (`buffers_bound_under`), lifted out of #190's `delete_verdict` so the guard and both reconciliation seams cannot disagree about which buffers an operation touches: every buffer, both sides normalized, component-aware containment. `EditorCore::reconcile_rename` moves the stored path and — only for a `PathDerived` name — the buffer name. `EditorCore::reconcile_delete` composes the same two removal phases `pmacs.buffer.kill` composes, preflighting `editing_in_progress` because a `ConcurrentEdit` refusal arrives after `kill_buffer` has already moved windows. Phase 2 stays with the caller; `EditorCore` gains no Lua handle. `AsyncRuntime::tick` now returns a `TickOutcome` carrying the settled ids plus the successful resource mutations, in bus-arrival order, which is documented as not being execution order. `PendingJob.resource` retains the paths the dispatchers move into the worker closure. `LspManager::forget_uri` purges the routes carrying a URI, drains the awaiters joined to them on the rid, and clears all fourteen stores plus `documents`. A generation-scoped exact-pair tombstone gates the two uncorrelated writers that can otherwise resurrect what it cleared: `publishDiagnostics` and `mark_document_stale`, which now takes a server id. `ResponseRoute::scoped_uri` is the one variant list, with `uri()` delegating to it. New Lua surface: `pmacs.buffer.set_name`, `pmacs.lsp.forget_uri`, `pmacs.diag._rename_resource`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/async_runtime.rs | 127 ++++++++++++++- src/diag.rs | 16 ++ src/editor_core.rs | 262 ++++++++++++++++++++++++++++++ src/lsp.rs | 332 +++++++++++++++++++++++++++++++++++++-- src/lua_bindings/diag.rs | 25 +++ src/lua_bindings/mod.rs | 264 +++++++++++++++++++++++++++---- 6 files changed, 975 insertions(+), 51 deletions(-) diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 493d993..551458f 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -391,6 +391,70 @@ struct PendingJob { /// When the job was registered. Used to compute "age" in the /// `*workers*` buffer. dispatched_at: Instant, + /// The filesystem mutation this job performs, retained so the + /// main-thread drain can reconcile the editor's path owners once + /// the syscall lands (dired Stage 2a, §5). + /// + /// The paths have to live here because the dispatchers **move** + /// them into the worker closure and nothing else retains them, and + /// because the reply is undifferentiated — rename and remove both + /// settle as `ReplyKind::FsUnit`, so a drain cannot key on the + /// reply and must key on the pending job. + /// + /// One enum field rather than a pair of `Option`s: two would admit + /// a both-`Some` state that cannot occur, which every consumer + /// would then have to rule out by hand. `COHERENCE.md` §9 is why + /// this is a field on the job and not a side map — the parse + /// job→buffer link already lives in a side map and §9 names that as + /// the defect. + resource: Option, +} + +/// A settled filesystem mutation, with the paths the worker consumed +/// (dired Stage 2a, §5). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ResourceOp { + /// A successful `rename(from, to)`. + Rename { + /// Source path, as the caller spelled it. + from: PathBuf, + /// Destination path, as the caller spelled it. + to: PathBuf, + }, + /// A successful `remove(path)`. + Remove { + /// The path that was removed. + path: PathBuf, + }, +} + +/// What one [`AsyncRuntime::tick`] observed. +/// +/// Settle identity and resource metadata come out of **one** +/// transaction — the post-drain loop already borrows `pending` to +/// record completions — so a consumer cannot see a settle without its +/// resource, or the reverse. +#[derive(Clone, Debug, Default)] +pub struct TickOutcome { + /// Ids that transitioned from `Running` to a terminal state during + /// this tick. The Lua runtime resumes coroutines parked on these. + pub settled: Vec, + /// Successful resource mutations, **in bus-arrival order. This is + /// not filesystem execution order.** + /// + /// [`AsyncRuntime::tick`] drains the reply bus with `try_recv` and + /// the runtime establishes no execution token, so a worker can + /// complete, be descheduled before sending, and have a later + /// mutation's reply arrive first. A consumer that reads "in settle + /// order" and infers causality is wrong; reconciliation is + /// deliberately order-independent (Q#DR29), and the primitive's + /// contract is that a caller with overlapping source/target paths + /// serializes by awaiting each op before dispatching the next. + /// + /// Carries **only** jobs that settled + /// [`PendingState::Complete`] — a failed or cancelled mutation + /// reconciles nothing and fires no hook. + pub resources: Vec, } /// Snapshot of a job's terminal state, returned by @@ -684,6 +748,18 @@ impl AsyncRuntime { kind: JobKind, supersede_key: Option<&str>, stream: Option, + ) -> (JobId, CancellationToken) { + self.allocate_with_resource(kind, supersede_key, stream, None) + } + + /// [`Self::allocate`], plus the filesystem mutation this job + /// performs. Only the two mutating fs dispatchers pass `resource`. + fn allocate_with_resource( + &self, + kind: JobKind, + supersede_key: Option<&str>, + stream: Option, + resource: Option, ) -> (JobId, CancellationToken) { let id = self.next_job_id.fetch_add(1, Ordering::Relaxed); let cancel = CancellationToken::new(); @@ -711,6 +787,7 @@ impl AsyncRuntime { max_batch: stream.unwrap_or(0), kind, dispatched_at: Instant::now(), + resource, }, ); (id, cancel) @@ -869,7 +946,17 @@ impl AsyncRuntime { /// Dispatch a `rename(from, to)` job. Settles to /// [`JobResult::Unit`] on success. T M8.1. pub fn dispatch_fs_rename(&self, from: PathBuf, to: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRename, supersede, None); + // The closure below MOVES both paths; the pending entry is the + // only thing that still knows them when the reply lands. + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRename, + supersede, + None, + Some(ResourceOp::Rename { + from: from.clone(), + to: to.clone(), + }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_rename(&cancel, &from, &to); @@ -891,7 +978,12 @@ impl AsyncRuntime { /// Dispatch a `remove(path)` job. T M8.1. pub fn dispatch_fs_remove(&self, path: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRemove, supersede, None); + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRemove, + supersede, + None, + Some(ResourceOp::Remove { path: path.clone() }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_remove(&cancel, &path); @@ -996,11 +1088,16 @@ impl AsyncRuntime { } } - /// Drain every queued reply on the main-thread bus, update - /// pending entries, and return the list of ids that *transitioned - /// from Running to a terminal state* during this tick. The Lua - /// runtime resumes coroutines parked on these ids. - pub fn tick(&self) -> Vec { + /// Drain every queued reply on the main-thread bus, update pending + /// entries, and report what settled. + /// + /// [`TickOutcome::settled`] is the ids that transitioned from + /// `Running` to a terminal state during this tick — the Lua runtime + /// resumes coroutines parked on these. + /// [`TickOutcome::resources`] is the filesystem mutations among them + /// that **succeeded**, in **bus-arrival order** (see the field's + /// own documentation: that is not execution order). + pub fn tick(&self) -> TickOutcome { let mut newly_settled = Vec::new(); while let Ok(env) = self.main.try_recv() { let Ok(reply): Result = self.main.decode(&env) else { @@ -1071,6 +1168,7 @@ impl AsyncRuntime { // a successor that came in mid-flight will have overwritten // the entry already, and that successor's pending lifetime // is what owns the slot now. + let mut resources = Vec::new(); if !newly_settled.is_empty() { let pending = self.pending.borrow(); let mut sup = self.supersede.borrow_mut(); @@ -1078,6 +1176,16 @@ impl AsyncRuntime { let now = Instant::now(); for id in &newly_settled { if let Some(job) = pending.get(id) { + // The harvest (§5): one more read in a loop that + // already borrows `pending` and reads `job.kind`, + // so settle identity and resource metadata come out + // of one transaction. Gated on `Complete` — a + // failed or cancelled mutation reconciles nothing. + if let Some(resource) = &job.resource + && matches!(job.state, PendingState::Complete(_)) + { + resources.push(resource.clone()); + } if let Some(key) = &job.supersede_key && sup.get(key) == Some(id) { @@ -1106,7 +1214,10 @@ impl AsyncRuntime { completed.pop_back(); } } - newly_settled + TickOutcome { + settled: newly_settled, + resources, + } } /// Snapshot the runtime's job tables for the `*workers*` diff --git a/src/diag.rs b/src/diag.rs index ba67450..f773321 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -266,6 +266,22 @@ impl DiagnosticStore { *self.epochs.entry(uri.to_owned()).or_insert(0) += 1; } + /// Drop **every** trace of `uri`, epoch included (dired Stage 2a, + /// §5 finding 4). + /// + /// Distinct from [`Self::clear`] on purpose: `clear` *creates* an + /// `epochs` entry (`or_insert(0) += 1`) because a consumer caching + /// against the epoch must observe that the diagnostics went away. + /// Forgetting is the opposite intent — the editor no longer holds + /// this URI at all — so leaving the counter behind would be a + /// URI-keyed leak in the one map nothing else prunes. + pub fn forget(&mut self, uri: &str) { + self.by_uri.remove(uri); + self.severity_counts.remove(uri); + self.stale_uris.remove(uri); + self.epochs.remove(uri); + } + /// Monotonic per-URI change counter: how many times `set` / /// `clear` ran for this URI. `0` for a URI never written. /// Consumers cache against this to detect republishes that no diff --git a/src/editor_core.rs b/src/editor_core.rs index 56ccfbf..732e19a 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -4840,6 +4840,191 @@ impl EditorCore { .map_err(|e| e.to_string()) } + /// Rebind every buffer affected by a successful rename of `old` to + /// `new` (dired Stage 2a, Q#DR14). Returns one + /// [`RenameRebind`] per buffer moved. + /// + /// A rename is a **transaction across path owners**, not a field + /// update. This method owns the two owners that live in the buffer: + /// the stored path and — subject to the provenance rule below — the + /// name. Everything else keyed by the path (URI-keyed LSP stores, + /// diagnostic overlays, dired's pathless handles, a package's own + /// URI table) reconciles off the `resource.renamed` hook that the + /// caller fires, because no buffer-keyed rebind can reach them. + /// + /// Both rename paths call this — the drain harvest for + /// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the + /// two cannot drift apart. + /// + /// # The name + /// + /// The name is rewritten only for a buffer whose name is + /// [`crate::buffer::BufferNameOrigin::PathDerived`]. String + /// inspection cannot substitute for that bit in either direction: a + /// relative open is named `foo.rs` (so an equality test leaves it + /// stale), and a user may name a buffer with a string that + /// normalizes to its own path (so a path-equivalence test + /// overwrites a chosen name). When it does fire, the new name is + /// the **normalized** new path — a buffer opened relatively + /// therefore acquires an absolute name, because no buffer records + /// which base its name was relative to. Reconciliation re-records + /// `PathDerived`, so a second rename still follows. + pub fn reconcile_rename(&mut self, old: &Path, new: &Path) -> Vec { + let old_n = normalize_buffer_path(old.to_path_buf()); + let new_n = normalize_buffer_path(new.to_path_buf()); + // A directory rename moves its whole subtree by construction, + // so descendants are always in scope here. + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, &old_n, true) + }; + let mut rebinds = Vec::with_capacity(affected.len()); + for (id, bound) in affected { + // Rebuild the path under the new root. An exact match maps + // to `new` itself; a descendant keeps its relative tail. + let target = if bound == old_n { + new_n.clone() + } else { + match bound.strip_prefix(&old_n) { + Ok(tail) => new_n.join(tail), + // Unreachable: `buffers_bound_under` matched on + // exactly this prefix. Skip rather than guess. + Err(_) => continue, + } + }; + let name_followed = { + let mut reg = self.registry.borrow_mut(); + let Ok(buf) = reg.get_mut(id) else { continue }; + buf.set_file_path(Some(target.clone())); + // The file behind this buffer moved, so metadata + // captured against the old path no longer describes + // it. Clearing is what `set_buffer_path`'s callers do + // via `set_buffer_meta`; leaving it would make + // external-change detection compare against a stat of + // a path that is gone. + buf.set_file_meta(None); + if buf.name_origin() == crate::buffer::BufferNameOrigin::PathDerived { + buf.set_path_derived_name(target.display().to_string()); + true + } else { + false + } + }; + rebinds.push(RenameRebind { + buffer_id: id, + old_path: bound, + new_path: target, + name_followed, + }); + } + rebinds + } + + /// Reconcile the buffers a successful delete of `path` orphaned + /// (dired Stage 2a, Q#DR18). + /// + /// Walks the whole registry by normalized equality **or** + /// component-aware prefix, so descendants of a deleted directory + /// are included and a second buffer on one path is not missed. + /// Descendants are unconditionally in scope here, unlike in + /// `delete_verdict`: a recursive delete destroyed them, and a + /// non-recursive one only succeeds on an *empty* directory, so a + /// buffer still bound underneath it was already an orphan. + /// + /// Policy, per buffer: + /// + /// * **modified** — kept alive and reported. The buffer keeps its + /// contents; only the file is gone. This is the half of the + /// promise that is robust, because it runs at drain time against + /// whatever state exists then. + /// * **mid-edit** — skipped entirely and reported in `refused`, + /// **preflighted** rather than discovered. A refusal from + /// `BufferRegistry::remove` is *not* inert: by the time it + /// returns `ConcurrentEdit`, [`Self::kill_buffer`] has already + /// dropped the id from `round_trip_buffers`, closed any side + /// window showing the buffer, and redirected every remaining + /// window onto a fallback with cursor, selection, overlays and + /// scroll position reset. The preflight is *sound*, not merely + /// cheap: phase 1 is entirely `EditorCore`, which holds no Lua + /// handle, so nothing between the check and the removal can + /// re-enter Lua and begin an edit. + /// * otherwise — killed through the full phase 1 above. + /// + /// Neither refusal aborts the rest: a directory delete reaching + /// twelve descendants must not stop at the one that is mid-edit. + /// + /// # Phase 2 is the caller's + /// + /// Buffer removal is two phases and the only place they are + /// composed today is a Lua binding (`pmacs.buffer.kill`). Phase 2 — + /// buffer-scoped keymaps, buffer-local config, folds, and the + /// registered `on_removed` callbacks — lives in `lua_bindings` and + /// needs `&Lua`, so this returns [`DeleteReconcile::killed`] and + /// its caller runs phase 2 over those ids. `EditorCore` does not + /// gain a Lua handle. + pub fn reconcile_delete(&mut self, path: &Path) -> DeleteReconcile { + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, path, true) + }; + let mut out = DeleteReconcile::default(); + for (id, _bound) in affected { + let preflight = { + let reg = self.registry.borrow(); + let Ok(buf) = reg.get(id) else { continue }; + let name = buf.name().to_owned(); + if buf.is_modified() { + Some(Err((true, name))) + } else if buf.editing_in_progress() { + Some(Err((false, name))) + } else { + Some(Ok(())) + } + }; + match preflight { + Some(Ok(())) => {} + Some(Err((true, name))) => { + out.kept_modified.push((id, name)); + continue; + } + Some(Err((false, name))) => { + out.refused.push(( + id, + format!("buffer {name:?} is mid-edit; finish the edit first"), + )); + continue; + } + None => continue, + } + match self.kill_buffer(id) { + Ok(()) => out.killed.push(id), + Err(message) => out.refused.push((id, message)), + } + } + out + } + + /// Re-root every URI-keyed overlay in **every** window from + /// `old_uri` to `new_uri` (dired Stage 2a, §5). + /// + /// The traversal mirrors overlay disposal's + /// (`lua_bindings`'s `retain` over `overlay_identity`), with the + /// `retain` replaced by [`View::rename_resource`]. That reaches + /// passive windows as well as the active one — which the Lua attach + /// path cannot, since `pmacs.diag._attach_view` takes the active + /// window and errors otherwise — and preserves composition order, + /// because nothing is removed or re-pushed. + /// + /// A window that never received the overlay still has none; + /// renaming cannot re-root an overlay that was never attached. + pub fn rename_resource_in_views(&mut self, old_uri: &str, new_uri: &str) { + for win in self.windows.values_mut() { + for overlay in &mut win.overlays { + overlay.rename_resource(old_uri, new_uri); + } + } + } + /// Switch one frontend's active window to a different buffer, allocating /// a fresh [`TextView`] for it without changing global active state. pub fn switch_active_buffer_for( @@ -5097,6 +5282,83 @@ fn backward_word(buf: &Buffer, mut pos: Position) -> Position { pos } +/// Every path-bound buffer an operation on `target` affects, paired +/// with its **normalized** stored path (dired Stage 2a; the shared walk +/// query #190 introduced for `delete_verdict`, lifted so rename +/// reconciliation and delete reconciliation cannot drift from it). +/// +/// Three properties, each of which a naive lookup gets wrong: +/// +/// * It scans **every** buffer. +/// [`crate::buffer_registry::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 first match +/// can hide a second buffer on the same path, which then survives +/// pointing at a path that no longer exists. +/// * Both sides are normalized. Stored paths are normalized on write +/// (`set_buffer_path`) while an op names its target however the +/// caller spelled it, so a raw comparison misses the match entirely. +/// * Containment is **component-aware** ([`Path::starts_with`]), never +/// a string prefix: `/foo` is not an ancestor of `/foobar`. +/// +/// `include_descendants` is the caller's decision because the two +/// consumers legitimately differ. A delete *guard* scopes descendants +/// to `recursive` (#190: a non-recursive delete destroys nothing +/// beneath the target, so a buffer under it must not refuse the op), +/// whereas a **rename** always moves its whole subtree and a +/// post-delete reconciliation is looking at a directory that is +/// already gone. +pub fn buffers_bound_under( + reg: &crate::buffer_registry::BufferRegistry, + target: &Path, + include_descendants: bool, +) -> Vec<(BufferId, PathBuf)> { + let target = normalize_buffer_path(target.to_path_buf()); + let mut out = Vec::new(); + for id in reg.ids() { + let Ok(buf) = reg.get(*id) else { continue }; + let Some(bound) = buf.file_path() else { continue }; + let bound = normalize_buffer_path(bound.to_path_buf()); + if bound == target || (include_descendants && bound.starts_with(&target)) { + out.push((*id, bound)); + } + } + out +} + +/// One buffer moved by [`EditorCore::reconcile_rename`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct RenameRebind { + /// The buffer that moved. + pub buffer_id: BufferId, + /// Its normalized path before the rename. + pub old_path: PathBuf, + /// Its normalized path after the rename. + pub new_path: PathBuf, + /// Whether the buffer's **name** followed the path, per + /// [`crate::buffer::BufferNameOrigin`]. Reported rather than + /// inferred so a consumer does not have to re-derive the + /// provenance rule. + pub name_followed: bool, +} + +/// Outcome of [`EditorCore::reconcile_delete`]. +/// +/// Three lists rather than two, because "kept on purpose" and "could +/// not be removed" are different events: collapsing them makes a +/// failure look like a policy decision. +#[derive(Clone, Debug, Default)] +pub struct DeleteReconcile { + /// Buffers whose phase 1 (core-side removal) completed. The + /// caller **must** run phase 2 (`after_buffer_removed`) over + /// these — `EditorCore` holds no Lua handle. + pub killed: Vec, + /// Modified buffers kept alive deliberately, with their names. + pub kept_modified: Vec<(BufferId, String)>, + /// Buffers that could not be removed, with the reason. + pub refused: Vec<(BufferId, String)>, +} + /// Normalize a buffer path to an absolute, lexically-clean form: /// /// 1. expand a leading `~` / `~/…` against `$HOME`, diff --git a/src/lsp.rs b/src/lsp.rs index 1eb8024..2d78f4a 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -821,6 +821,37 @@ pub struct LspManager { /// per-server [`crate::lsp_status::LspStatus`] for the modeline / /// `*lsp*` buffer. status_tracker: crate::lsp_status::LspStatusTracker, + /// dired Stage 2a §5 — exact `(server, uri)` pairs this editor + /// **explicitly forgot**, so a later *uncorrelated* write cannot + /// resurrect them. + /// + /// [`Self::forget_uri`] purges `pending_routes` and drains their + /// awaiters, which covers every write that is matched to a request + /// id. It cannot cover the writers that never go near a route, and + /// there are two that create state: + /// `textDocument/publishDiagnostics`, absorbed unconditionally — + /// and note that `diag_store` has **zero** correlated writers, so + /// the one store the purge most needs to protect is the one it + /// cannot help at all — and [`Self::mark_document_stale`], which + /// creates URI keys in three stores. + /// + /// Deliberately not the cheaper membership gate ("absorb only if + /// `(sid, uri)` is in `documents`"): servers legitimately publish + /// diagnostics for files the editor never opened — a crate-wide + /// push naming a dependency — and a membership gate drops every + /// one. A tombstone drops only what we forgot. `handle_response` + /// already uses this shape for late arrivals + /// (`client.cancelled_rids`); this is the same pattern with a + /// `(server, URI)` key instead of a request id. + /// + /// **Reclaimed and generation-scoped, not size-bounded.** + /// `did_open(sid, uri)` clears that exact pair; + /// [`Self::start_generation`] and [`Self::forget`] remove every pair + /// for their server and retain every other server's. A capacity or + /// LRU eviction would let an arbitrarily late notification + /// resurrect an evicted key, which is the whole failure this gate + /// exists to stop. + forgotten_documents: std::collections::HashSet<(LspServerId, String)>, /// T M4.9: `(project_root, language_id)` → server id. Drives the /// "LSP runs per-project, not per-buffer" invariant. Roots are /// stored as [`PathBuf`] so callers don't have to canonicalise @@ -901,9 +932,21 @@ enum ResponseRoute { } impl ResponseRoute { - /// The document URI this route targets — the key for the position - /// codec's document/encoding lookup. - fn uri(&self) -> &str { + /// The document URI this route is **scoped to**, if any (dired + /// Stage 2a, §5). + /// + /// Fourteen of the fifteen variants carry a `uri`. The fifteenth, + /// `WorkspaceSymbol`, carries a **query** and no URI at all — its + /// own comment explains that the query stands in for the doc URI in + /// the supersede key — so it answers `None`, and + /// [`LspManager::forget_uri`]'s purge retains it: a + /// workspace-symbol query is not scoped to any document and a + /// rename does not invalidate it. + /// + /// Exhaustive on purpose. A new URI-bearing variant must not + /// silently default to "not scoped", which would leave an in-flight + /// response able to repopulate a forgotten key. + fn scoped_uri(&self) -> Option<&str> { match self { ResponseRoute::Completion { uri } | ResponseRoute::Hover { uri } @@ -918,14 +961,25 @@ impl ResponseRoute { | ResponseRoute::SemanticTokensDelta { uri } | ResponseRoute::Locations { uri, .. } | ResponseRoute::DocumentSymbol { uri } - | ResponseRoute::DocumentHighlight { uri } => uri, - // workspace/symbol results span arbitrary files we have - // not cached — no doc to convert against, so the inbound - // codec must pass coordinates through untouched (same - // non-destructive rule as cross-file definition). - ResponseRoute::WorkspaceSymbol { .. } => "", + | ResponseRoute::DocumentHighlight { uri } => Some(uri), + ResponseRoute::WorkspaceSymbol { .. } => None, } } + + /// The document URI this route targets — the key for the position + /// codec's document/encoding lookup. + /// + /// Delegates to [`Self::scoped_uri`] so the variant list exists + /// once: two near-identical matches over fifteen variants is how + /// one of them ends up missing a variant the other has. + /// `workspace/symbol` results span arbitrary files we have not + /// cached, so there is no doc to convert against and the inbound + /// codec must pass coordinates through untouched (the same + /// non-destructive rule as cross-file definition) — which the empty + /// string already expressed. + fn uri(&self) -> &str { + self.scoped_uri().unwrap_or("") + } } /// One Lua-visible awaiter bound to an in-flight LSP request. Mirrors @@ -1021,6 +1075,7 @@ impl LspManager { semantic_token_store: crate::semantic_tokens::make_shared_store(), pending_routes: HashMap::new(), status_tracker: crate::lsp_status::LspStatusTracker::new(), + forgotten_documents: std::collections::HashSet::new(), project_servers: HashMap::new(), } } @@ -1329,6 +1384,10 @@ impl LspManager { // T M4.5 Option B: drop cached docs; the fresh server gets a // new `did_open` from the editor's reattach path. self.documents.retain(|(s, _), _| *s != id); + // dired Stage 2a §5 — the tombstone is generation-scoped: this + // generation's forgotten pairs go, every other server's stay. + // The reattach path re-`did_open`s whatever it still holds. + self.forgotten_documents.retain(|(s, _)| *s != id); client.state = LspClientState::Starting; let proc_spec = client.spec.to_process_spec(); let pid = self.supervisor.borrow_mut().spawn(proc_spec)?; @@ -2901,6 +2960,18 @@ impl LspManager { let Some(uri) = params.get("uri").and_then(Value::as_str).map(str::to_owned) else { return; }; + // dired Stage 2a §5 — the uncorrelated-write gate. This + // notification carries no request id, so `forget_uri`'s route + // purge cannot see it, and `diag_store` has no correlated + // writers at all: without this check a late publish for a + // renamed-away URI silently reinstates the state we just + // forgot. The gate is the exact `(server, uri)` pair, which is + // available here even though `DiagnosticStore.by_uri` is keyed + // by URI alone — so provenance is retained for selective + // teardown without changing the store's key. + if self.forgotten_documents.contains(&(sid, uri.clone())) { + return; + } // T M4.5 Option B: byte-normalise diagnostic ranges before the // store parses them, so the gutter renders correct spans on // non-ASCII lines. @@ -3031,6 +3102,9 @@ impl LspManager { // between exit and forget. Idempotent. self.drain_external_cancelled(sid); self.documents.retain(|(s, _), _| *s != sid); + // dired Stage 2a §5 — terminal removal drops every tombstone + // this server owned; other servers' pairs are retained. + self.forgotten_documents.retain(|(s, _)| *s != sid); self.status_tracker.forget(sid); // T M4.9: drop the project scoping so the next // ensure_server_for_project call spawns a fresh server. @@ -3038,6 +3112,223 @@ impl LspManager { Ok(()) } + /// Drop **every** trace of `uri` under `sid` (dired Stage 2a, §5). + /// + /// One manager-level method rather than fourteen call sites at the + /// Lua layer, because fourteen call sites is how one gets + /// forgotten. Four ordered steps: + /// + /// 1. **Tombstone `(sid, uri)` first**, before clearing anything. + /// Main-thread execution already makes the rest atomic with + /// respect to another manager tick, but putting the gate first + /// means every later call observes the forgotten state even if a + /// future refactor introduces an early return. + /// 2. **Purge `pending_routes`** whose route carries this URI. + /// `WorkspaceSymbol` is retained unconditionally: it carries no + /// URI at all — its query stands in for the doc URI in the + /// supersede key — and a workspace-symbol query is not scoped to + /// any document, so a rename does not invalidate it. Clearing + /// the stores *without* this purge is a race that reintroduces + /// exactly the state it removed: a response already in flight + /// routes on arrival and repopulates the old key after the clear. + /// 3. **Drain-cancel their awaiters.** `pending_external` holds the + /// `Handle:await()` side, and its contract is explicit that it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped — both range over `sid` — so this + /// joins route to awaiter on the `rid`, which is the only index + /// between them. The model is + /// [`Self::drain_external_cancelled`], which is *unconditional*; + /// modelling on `drain_cancelled_externals` instead would drain + /// nothing, because it removes only awaiters whose cancellation + /// token was flipped or which outlived the request timeout, and + /// **a rename flips no token** — leaving any coroutine awaiting + /// against the old URI parked forever. + /// 4. **Clear all fourteen stores plus `documents`.** Two keys are + /// irregular: `locations_store` is *kind*-keyed, so all four + /// kinds must go, and `symbol_store` is *scope*-keyed and holds + /// workspace symbols too, so only the document-scoped entry is + /// dropped — the same asymmetry that makes `WorkspaceSymbol` + /// route-exempt above. Diagnostics go through + /// [`crate::diag::DiagnosticStore::forget`], not `clear`: `clear` + /// *increments* the epoch it is meant to forget. + /// + /// Takes the **old** URI, so calling it after `did_open` of the new + /// one is safe and order-independent. + /// + /// Note there is **no precedent to copy for the store half**: + /// neither server-scoped teardown clears the fourteen result stores. + /// `start_generation` clears deferred notifications, routes, + /// documents and externals; `forget` clears routes, documents, + /// externals, the status tracker and project scoping. Whether stale + /// results should survive a restart is a separate pre-existing + /// question, and this method deliberately does not answer it. + /// + /// # Errors + /// + /// Unknown `sid`, matching [`Self::forget`]'s behaviour for the same + /// input. A URI with **no** state under a known server is an + /// idempotent **success**: the caller runs per attachment, an + /// attachment need not have any pending route or populated result + /// store, and cleanup can be repeated after an earlier partial + /// teardown. + pub fn forget_uri(&mut self, sid: LspServerId, uri: &str) -> Result<(), String> { + if !self.clients.contains_key(&sid) { + return Err(format!("unknown server: {sid}")); + } + // Step 1 — the gate, first. + self.forgotten_documents.insert((sid, uri.to_owned())); + + // Step 2 — collect the rids this URI owns, then purge. + let doomed_rids: Vec = self + .pending_routes + .iter() + .filter(|((s, _), route)| *s == sid && route.scoped_uri() == Some(uri)) + .map(|((_, rid), _)| *rid) + .collect(); + for rid in &doomed_rids { + self.pending_routes.remove(&(sid, *rid)); + } + + // Step 3 — settle the awaiters joined to those rids cancelled. + for rid in &doomed_rids { + if let Some(p) = self.pending_external.remove(&(sid, *rid)) { + for a in &p.awaiters { + self.runtime.complete_external_cancelled(a.job_id); + } + } + } + + // Step 4 — the fourteen stores plus `documents`. + let server_key = sid.raw().to_string(); + self.diag_store + .lock() + .expect("diag store mutex poisoned") + .forget(uri); + self.completion_store + .lock() + .expect("completion store mutex poisoned") + .clear(&crate::completion::CompletionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.hover_store + .lock() + .expect("hover store mutex poisoned") + .clear(&crate::hover::HoverKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.signature_store + .lock() + .expect("signature store mutex poisoned") + .clear(&crate::signature::SignatureKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.definition_store + .lock() + .expect("definition store mutex poisoned") + .clear(&crate::definition::DefinitionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + { + let mut guard = self + .locations_store + .lock() + .expect("locations store mutex poisoned"); + // Kind-keyed: all four have to go. + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + guard.clear(&crate::locations::LocationsKey { + server: server_key.clone(), + uri: uri.to_owned(), + kind, + }); + } + } + self.symbol_store + .lock() + .expect("symbol store mutex poisoned") + // Scope-keyed, and the store also holds workspace symbols: + // only the document-scoped entry is dropped. + .clear(&crate::symbol::SymbolKey { + server: server_key.clone(), + scope: crate::symbol::SymbolScope::Document(uri.to_owned()), + }); + self.document_highlight_store + .lock() + .expect("document highlight store mutex poisoned") + .clear(&crate::document_highlight::DocumentHighlightKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.formatting_store + .lock() + .expect("formatting store mutex poisoned") + .clear(&crate::formatting::FormattingKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.rename_store + .lock() + .expect("rename store mutex poisoned") + .clear(&crate::rename::RenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.prepare_rename_store + .lock() + .expect("prepare rename store mutex poisoned") + .clear(&crate::prepare_rename::PrepareRenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.code_action_store + .lock() + .expect("code action store mutex poisoned") + .clear(&crate::code_action::CodeActionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.inlay_hint_store + .lock() + .expect("inlay hint store mutex poisoned") + .clear(&crate::inlay_hint::InlayHintKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.semantic_token_store + .lock() + .expect("semantic token store mutex poisoned") + .clear(&crate::semantic_tokens::SemanticTokenKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.documents.remove(&(sid, uri.to_owned())); + Ok(()) + } + + /// Whether `(sid, uri)` is currently tombstoned (dired Stage 2a). + /// Read surface for tests; production code consults the set + /// directly at its two gates. + #[must_use] + pub fn is_forgotten(&self, sid: LspServerId, uri: &str) -> bool { + self.forgotten_documents.contains(&(sid, uri.to_owned())) + } + + /// How many `(server, uri)` pairs are tombstoned. Read surface for + /// the reclamation tests — the set must not grow without bound + /// across teardowns. + #[must_use] + pub fn forgotten_document_count(&self) -> usize { + self.forgotten_documents.len() + } + /// Convenience: send `textDocument/didOpen` to `sid`. pub fn did_open( &mut self, @@ -3053,6 +3344,12 @@ impl LspManager { .ok_or_else(|| format!("unknown server: {sid}"))?; let uri = uri.into(); let text = text.into(); + // dired Stage 2a §5 — reclaim the tombstone for THIS exact pair + // and no other. Reopening the document is the editor saying it + // holds the URI again, so a later publish or stale-mark for it + // must be admitted; another server's tombstone for the same URI + // is untouched. + self.forgotten_documents.remove(&(sid, uri.clone())); // T M4.5 Option B: mirror the document so the position codec // can convert per-line between the server's `character` units // and pmacs byte offsets. @@ -3081,7 +3378,7 @@ impl LspManager { let uri = uri.into(); let text = text.into(); self.documents.insert((sid, uri.clone()), text.clone()); - self.mark_document_stale(&uri); + self.mark_document_stale(sid, &uri); let params = json!({ "textDocument": { "uri": uri, @@ -3105,7 +3402,20 @@ impl LspManager { /// mark staleness at *edit* time even while the (full-document, /// O(file)) didChange notification itself is debounced — per-edit /// staleness is what keeps stale-position artifacts off screen. - pub fn mark_document_stale(&self, uri: &str) { + /// + /// **Takes `sid` since dired Stage 2a.** It previously took no + /// server id while *creating* URI keys in three stores for every + /// server at once, which made it the second uncorrelated writer able + /// to resurrect a forgotten URI — and made an exact tombstone + /// impossible. Every caller already owns the attachment's server id, + /// so the parameter costs nothing. + pub fn mark_document_stale(&self, sid: LspServerId, uri: &str) { + // The second uncorrelated-write gate (§5 finding 2). Returns + // before touching any of the three stores, so a forgotten URI + // cannot regain a stale flag either. + if self.forgotten_documents.contains(&(sid, uri.to_owned())) { + return; + } self.diag_store .lock() .expect("diag store mutex poisoned") diff --git a/src/lua_bindings/diag.rs b/src/lua_bindings/diag.rs index c462f44..338dfc3 100644 --- a/src/lua_bindings/diag.rs +++ b/src/lua_bindings/diag.rs @@ -232,6 +232,31 @@ pub fn install_diag( )?; } + // dired Stage 2a §5 step 6 — re-root every attached + // `DiagnosticView` from `old_uri` to `new_uri` after a rename. + // + // `DiagnosticView.uri` is set once at construction and is private, + // and `View` has no downcast, so nothing outside `diag.rs` can + // reach it; the `View::rename_resource` hook is the seam. The sweep + // walks EVERY window, which is what `_attach_view` above cannot do + // — it takes the active window and errors otherwise — so a passive + // split that already holds the overlay is re-rooted too. It mutates + // in place, so each overlay keeps its position in the window's + // composition order; a remove-and-re-push would move the underline + // to the end of the stack and pass a one-window test anyway. + { + diag_mod.set( + "_rename_resource", + lua.create_function(move |lua, (old_uri, new_uri): (String, String)| { + let Some(core) = lua.app_data_ref::() else { + return Ok(false); + }; + core.borrow_mut().rename_resource_in_views(&old_uri, &new_uri); + Ok(true) + })?, + )?; + } + pmacs.set("diag", diag_mod)?; Ok(()) } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index bbdb7fa..212e962 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1671,16 +1671,11 @@ fn delete_verdict( } }; - 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; - } + // The shared walk (dired Stage 2a): one enumeration, so this guard + // and the two reconciliation seams cannot disagree about which + // buffers an operation on `path` touches. + for (id, _bound) in crate::editor_core::buffers_bound_under(reg, path, scan_descendants) { + let Ok(buf) = reg.get(id) else { 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 @@ -1714,6 +1709,131 @@ fn delete_verdict( DeleteVerdict::Clear } +/// Reconcile a successful rename and fire `resource.renamed` (dired +/// Stage 2a, §5). +/// +/// Both rename paths land here — the drain harvest for +/// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the two +/// can no longer drift, which is how the raw-lookup trap survived being +/// "fixed" once already. +/// +/// The hook carries the **paths**, normalized absolute, not the rebind +/// list: dired's buffers are pathless, so a path-keyed consumer must be +/// able to reconcile from `(old, new)` alone. And the Rust side 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 — +/// so the hook is the mechanism that scales, not a convenience. +/// +/// Returns the rebinds, for a caller that wants to report. +fn reconcile_rename_and_fire( + lua: &Lua, + from: &std::path::Path, + to: &std::path::Path, +) -> Vec { + let (rebinds, old_n, new_n) = { + let Some(core) = lua.app_data_ref::() else { + return Vec::new(); + }; + let mut core = core.borrow_mut(); + let rebinds = core.reconcile_rename(from, to); + ( + rebinds, + crate::editor_core::normalize_buffer_path(from.to_path_buf()), + crate::editor_core::normalize_buffer_path(to.to_path_buf()), + ) + }; + // The borrow is released before re-entering Lua: subscribers call + // back into the core (dired reverts a listing, the LSP subscriber + // re-attaches), and a live borrow would panic. + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String( + match lua.create_string(old_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + args.push_back(mlua::Value::String( + match lua.create_string(new_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + run_hook_if_defined(lua, "resource.renamed", args); + rebinds +} + +/// Reconcile a successful delete and fire `resource.deleted` (dired +/// Stage 2a, §6). +/// +/// Composes the **same two removal phases** `pmacs.buffer.kill` +/// composes. Phase 1 (`EditorCore::reconcile_delete`) closes side +/// windows showing a doomed buffer, redirects every other window to a +/// fallback, and removes the id from the registry; phase 2 — +/// buffer-scoped keymaps, buffer-local config, folds, and the +/// registered `on_removed` callbacks — runs here, because it needs +/// `&Lua` and `EditorCore` has no Lua handle. +/// +/// `apply_resource_op`'s delete arm previously ran +/// `remove_buffer_and_fire`, i.e. phase 2 **without** phase 1, leaving +/// any window displaying that buffer pointing at a removed id. Routing +/// both paths through here is what makes that go away as a property of +/// the seam rather than as a separate patch. +fn reconcile_delete_and_fire( + lua: &Lua, + path: &std::path::Path, +) -> crate::editor_core::DeleteReconcile { + let (outcome, normalized) = { + let Some(core) = lua.app_data_ref::() else { + return crate::editor_core::DeleteReconcile::default(); + }; + let mut core = core.borrow_mut(); + let outcome = core.reconcile_delete(path); + ( + outcome, + crate::editor_core::normalize_buffer_path(path.to_path_buf()), + ) + }; + // Phase 2, over exactly the ids phase 1 removed. + for id in &outcome.killed { + after_buffer_removed(lua, *id); + } + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String( + match lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return outcome, + }, + )); + run_hook_if_defined(lua, "resource.deleted", args); + outcome +} + +/// Drive [`crate::async_runtime::TickOutcome::resources`] through +/// reconciliation, one settled mutation at a time (dired Stage 2a, +/// Q#DR29). +/// +/// **Each settled mutation reconciles on its own, and nothing here +/// depends on the relative order of two mutations that were in flight +/// simultaneously** — `resources` is bus-arrival order and the runtime +/// establishes no execution token. That is safe rather than merely +/// honest: independent mutations commute, and the primitive's contract +/// (`builtin/runtime/fs.lua`) requires a caller with overlapping +/// source/target paths to serialize by awaiting each op before +/// dispatching the next. +fn reconcile_settled_resources(lua: &Lua, resources: &[crate::async_runtime::ResourceOp]) { + use crate::async_runtime::ResourceOp; + for op in resources { + match op { + ResourceOp::Rename { from, to } => { + reconcile_rename_and_fire(lua, from, to); + } + ResourceOp::Remove { path } => { + reconcile_delete_and_fire(lua, path); + } + } + } +} + fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> { registry .borrow_mut() @@ -3220,6 +3340,37 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result*` buffer can follow a renamed + // directory is for dired's own `resource.renamed` subscriber to + // rename it. The alternative — kill and recreate under the new + // name — loses window placement, the cursor, the read-only + // intercept, round-trip input and the major mode, each of which + // would have to be re-established in the right order. + // + // Uniqueness stays the CALLER's job, matching the Rust setter; + // dired reuses its existing `<2>`-variant uniquifier. + // + // This records `BufferNameOrigin::Explicit` (Q#DR30): it is a + // naming operation even when the string happens to denote the + // file, so a later rename must not overwrite it. + let reg = registry.clone(); + buffer.set( + "set_name", + lua.create_function(move |_, (id, name): (BufferIdLua, String)| { + reg.borrow_mut() + .get_mut(id.0) + .map_err(mlua::Error::external)? + .set_name(name); + Ok(()) + })?, + )?; + } + { let reg = registry.clone(); buffer.set( @@ -3437,12 +3588,18 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() - { - core.borrow_mut().set_buffer_path(id, Some(to.clone())); - } + // dired Stage 2a: the raw, first-match, + // un-normalized `find_by_path` lookup this arm + // used is replaced by the shared transaction. + // Three defects went with it — stored paths are + // normalized on write while the op names its + // target raw, so the lookup could miss the + // buffer entirely; a directory rename has many + // affected buffers by construction and only the + // first moved; and the buffer's *name* stayed + // stale, so the statusline and buffer list kept + // the old filename. + reconcile_rename_and_fire(lua, &from, &to); } "delete" => { // Four ordered phases (Q#RD2): stat/no-op @@ -3499,18 +3656,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { return Err(mlua::Error::external(format!( @@ -7367,9 +7525,15 @@ pub fn install_async( async_mod.set( "_tick", lua.create_function(move |lua, ()| { - let ids = rt.tick(); - let t = lua.create_table_with_capacity(ids.len(), 0)?; - for (i, id) in ids.into_iter().enumerate() { + let outcome = rt.tick(); + // Reconcile BEFORE the settled ids reach Lua. The Lua + // runtime resumes parked coroutines from the table this + // returns, so a coroutine that renamed and then + // inspects a buffer would otherwise see pre-rename + // state. Ordering here is by construction, not by luck. + reconcile_settled_resources(lua, &outcome.resources); + let t = lua.create_table_with_capacity(outcome.settled.len(), 0)?; + for (i, id) in outcome.settled.into_iter().enumerate() { t.set(i + 1, id)?; } Ok(t) @@ -10001,11 +10165,18 @@ pub fn install_lsp( // `builtin/runtime/lsp.lua` calls this per edit so stale // suppression stays keystroke-accurate while the O(file) // full-document notification is coalesced. + // + // **Takes the server id since dired Stage 2a.** It previously + // took the URI alone while creating URI keys in three stores + // for every server at once, which made it the second + // uncorrelated writer able to resurrect a URI `forget_uri` had + // just cleared. The sole production caller already holds + // `rec.server`. let m = manager.clone(); lsp_mod.set( "_mark_document_stale", - lua.create_function(move |_, uri: String| { - m.borrow().mark_document_stale(&uri); + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow().mark_document_stale(id.0, &uri); Ok(()) })?, )?; @@ -10529,6 +10700,35 @@ pub fn install_lsp( )?; } + { + // dired Stage 2a §5 — the per-document teardown the + // `resource.renamed` subscriber needs. Modelled on `forget` + // above: a closure over the shared manager that calls through + // and maps the error with `mlua::Error::external`. + // + // Error contract: **raises** for an unknown server id, matching + // `forget`'s behaviour for the same input, and **succeeds + // silently** when the URI has no state under a known server. + // The second arm is the one that matters — the subscriber runs + // per attachment, an attachment need not have any pending route + // or populated result store, and cleanup can be repeated after + // an earlier partial teardown. An over-strict binding would turn + // that ordinary idempotent case into an error inside a hook. + // + // Takes the **old** URI, so calling it after `did_open` of the + // new one is safe and order-independent. + let m = manager.clone(); + lsp_mod.set( + "forget_uri", + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow_mut() + .forget_uri(id.0, &uri) + .map_err(mlua::Error::external)?; + Ok(()) + })?, + )?; + } + { let m = manager.clone(); lsp_mod.set( From 7e3e630c3b35042ce10e8647a9526540483245d7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:09:03 -0400 Subject: [PATCH 03/12] =?UTF-8?q?wip(stage2a):=20the=20Lua=20half=20?= =?UTF-8?q?=E2=80=94=20hooks,=20LSP=20subscribers,=20applier=20origin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resource.renamed` and `resource.deleted` are declared `all-must-succeed`, so one raising subscriber does not stop the rest from reconciling. `lsp.lua` gains the two subscribers. Rename runs the ordered teardown per attachment — flush the pending didChange, didClose the old URI, `forget_uri` against the OLD server, re-run `ensure_server` (a rename across project roots needs a different one), didOpen the new URI, then re-root the diagnostic overlays. Delete tears the attachment down, because the buffer may be gone entirely and a retained record is a dangling handle. The workspace-edit applier captures the origin BUFFER instead of its path, and restores nothing when that buffer is gone. A captured Lua local is unreachable to any transaction, and the old path fallback is what materialized a phantom empty buffer at the renamed-away path. `fs.lua` states the overlapping-mutation serialization precondition as a correctness rule, with the counterexample showing why no static ordering rule substitutes for it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- builtin/hooks/default.lua | 32 +++++++++ builtin/runtime/fs.lua | 24 +++++++ builtin/runtime/lsp.lua | 143 +++++++++++++++++++++++++++++++++++--- 3 files changed, 191 insertions(+), 8 deletions(-) diff --git a/builtin/hooks/default.lua b/builtin/hooks/default.lua index 4fabfe9..69b04c3 100644 --- a/builtin/hooks/default.lua +++ b/builtin/hooks/default.lua @@ -16,6 +16,12 @@ -- format-on-save subscribe here. -- * editor.before-quit --- short-circuit. A callback may veto quit -- (e.g. "buffer modified --- save first?"). +-- * resource.renamed --- all-must-succeed (dired Stage 2a). Fired +-- after a successful rename, with (old, new) +-- canonical absolute paths. +-- * resource.deleted --- all-must-succeed (dired Stage 2a). Fired +-- after a successful delete, with the +-- canonical absolute path. -- -- These are *defined* here so user config can attach callbacks via -- pmacs.hook.add. Run sites are in Rust (after-load, after-edit) and in @@ -76,6 +82,32 @@ define { kind = "short-circuit", } +define { + name = "resource.renamed", + description = "Fired once per SUCCESSFUL filesystem rename, with the old " .. + "and new paths as canonical absolute strings. The core " .. + "reconciles what it can reach -- buffer paths and names, the " .. + "URI-keyed LSP stores, attached diagnostic overlays -- but a " .. + "package that keys its own state by path or URI is invisible " .. + "to that, so this hook is the mechanism that scales. It " .. + "carries PATHS rather than a rebind list precisely because " .. + "dired's listing buffers are pathless: a path-keyed consumer " .. + "must be able to reconcile from (old, new) alone. Does not " .. + "fire for a rename that failed or was cancelled.", + kind = "all-must-succeed", +} + +define { + name = "resource.deleted", + description = "Fired once per SUCCESSFUL filesystem delete, with the " .. + "canonical absolute path. Buffers on the path and beneath it " .. + "have already been reconciled: unmodified ones killed " .. + "through both removal phases, modified ones kept alive. " .. + "Subscribers drop their own path-keyed state. Does not fire " .. + "for a delete that failed or was cancelled.", + kind = "all-must-succeed", +} + define { name = "editor.before-quit", description = "Fired before the editor exits. Return false to veto.", diff --git a/builtin/runtime/fs.lua b/builtin/runtime/fs.lua index 39e3baa..35b25a2 100644 --- a/builtin/runtime/fs.lua +++ b/builtin/runtime/fs.lua @@ -163,6 +163,30 @@ end -- If a package needs at-most-one-pending semantics for mutations, -- it should serialize on the package side (await each op before -- dispatching the next). The fs primitive can't enforce that. +-- +-- **And that is a CORRECTNESS precondition, not only a cancellation +-- one (dired Stage 2a, Q#DR29).** A successful `rename` or `remove` +-- reconciles the editor's path owners in the main-thread drain — buffer +-- paths and names, the URI-keyed LSP state, the `resource.renamed` / +-- `resource.deleted` hooks. That reconciliation is deliberately +-- order-INDEPENDENT: the runtime drains the reply bus with `try_recv` +-- and establishes no execution token, so a worker can finish first and +-- be descheduled before sending, and reply order therefore does not +-- recover filesystem execution order. +-- +-- Independent mutations commute, so nothing is owed for them. But +-- **mutations whose source/target paths overlap must be serialized by +-- dispatching the next only after the previous handle settles.** There +-- is no static ordering rule that would substitute: rename `dir` -> +-- `newdir` racing delete `dir/child.txt` needs delete-then-rename if +-- the delete ran first on disk and rename-then-delete if the rename +-- did, and a fixed "deletes before renames" rule gets one of the two +-- wrong — the kill misses, the rename then rebinds the buffer onto a +-- path whose file is gone, and it survives pointing at nothing. +-- +-- A caller that ignores this owns the residue: a buffer left bound to a +-- stale path, or killed when it should have been rebound. Recoverable +-- and visible, not data loss — but real. function fs.rename(from, to) if type(from) ~= "string" then diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index f17827f..679d573 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1124,7 +1124,7 @@ pmacs.hook.add("buffer.after-edit", function() -- Stale suppression must stay keystroke-accurate even though the -- O(file) didChange send below is coalesced: render families -- anchored to pre-edit positions are hidden from this edit on. - pcall(pmacs.lsp._mark_document_stale, rec.uri) + pcall(pmacs.lsp._mark_document_stale, rec.server, rec.uri) -- Arc 1d: was this edit a typed character? The input-origin signal -- (see the trigger block below). local typed = pmacs.editor.this_command @@ -1437,19 +1437,33 @@ local function apply_workspace_edit(ops) end end if #plan == 0 then return 0, 0, 0 end - local origin = active_buffer_path() + -- G1 — capture the origin BUFFER, not its path. A path captured here + -- is a plain Lua local, and no amount of reconciliation can reach an + -- already-captured local: when the batch renames the active file, the + -- old path no longer resolves, `find_or_open` hits + -- `resolve_target_buffer`'s NotFound arm, and that arm CREATES an + -- empty path-backed buffer and selects it. The user was returned to a + -- phantom file that never existed. The handle follows the rename for + -- free, because the buffer is what moved. + local origin_buf = pmacs.window.buffer() local edit_total, files, res_ops = 0, 0, 0 -- Plan items fully applied before a failure. Q#RD3 permits partial -- application, so this is what stops a caller claiming "nothing was -- mutated" when something was. local applied_ops = 0 - -- Return the user to where they invoked from — best-effort, since - -- that path may have just been renamed or deleted. Runs on the - -- FAILURE path too (Q#RD7): previously this ran only after a - -- successful loop, so a mid-batch refusal stranded the user in - -- whatever buffer the last applied op left active. + -- Return the user to where they invoked from. 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. + -- + -- **No path fallback (G1).** If the origin buffer is gone — the batch + -- deleted its file and reconciliation killed it — restore NOTHING. + -- The old code's path fallback is exactly what fabricated a phantom + -- buffer; "return the user somewhere plausible" is not worth inventing + -- a file that does not exist. local function restore_origin() - if origin then pcall(pmacs.buffer.find_or_open, origin) end + if not origin_buf then return end + pcall(pmacs.window.switch_buffer, origin_buf) end for _, item in ipairs(plan) do local ok, err @@ -2882,3 +2896,116 @@ pmacs.command.define { pmacs.keymap.bind { scope = "global", sequence = "M-g n", command = "diag.next" } pmacs.keymap.bind { scope = "global", sequence = "M-g p", command = "diag.previous" } + +-- Resource reconciliation --------------------------------------------------- +-- +-- dired Stage 2a, §5. A rename or delete moves or destroys a path that +-- FOURTEEN URI-keyed store families, the `documents` mirror, the pending +-- response routes and the attached diagnostic overlays are all keyed by. +-- `EditorCore` reconciles the buffer's own path and name; these two +-- subscribers reconcile the LSP layer, which is buffer-keyed here +-- (`rec.uri` is cached per buffer and read at dozens of sites, so ONE +-- rebind reaches all of them) and URI-keyed in Rust. +-- +-- These subscribers are independent of every other `resource.renamed` +-- consumer by construction: this one touches URI-keyed state, dired's +-- touches its own handle table, and neither reads what the other wrote. +-- That matters because `all-must-succeed` does NOT abort the fan-out — +-- `run_all_must_succeed` collects each callback's error and continues — +-- so a subscriber may not rely on a raising peer to stop the sequence, +-- and the ordered teardown below is ordered INTERNALLY rather than by +-- registration. + +-- Every attachment whose document is `path` or lies beneath it, as +-- `{ key, rec, path }`. Resolved through `path_for_uri` and compared +-- with `paths_related`, so the comparison is component-aware and runs on +-- the same canonical form the buffer registry keys on. +local function attachments_under(path) + local out = {} + for key, rec in pairs(attachments) do + local rec_path = rec.uri and pmacs.lsp.path_for_uri(rec.uri) + if rec_path and paths_related(rec_path, path) then + out[#out + 1] = { key = key, rec = rec, path = rec_path } + end + end + return out +end + +pmacs.hook.add("resource.renamed", function(old_path, new_path) + if type(old_path) ~= "string" or type(new_path) ~= "string" then return end + for _, hit in ipairs(attachments_under(old_path)) do + local key, rec, old_uri = hit.key, hit.rec, hit.rec.uri + -- The buffer's own path was rebound before this hook fired, so ask + -- it rather than reconstructing the tail ourselves. A buffer that + -- somehow lost its path (killed, unbound) cannot be re-opened, and + -- falls through to the teardown-only path below. + local ok_path, new_buf_path = pcall(function() return rec.buffer:path() end) + local new_uri = (ok_path and new_buf_path) and file_uri_for(new_buf_path) or nil + + -- 1. Flush any pending didChange for the OLD uri, so the server is + -- not left holding an edit it can no longer attribute. + flush_did_change_for(rec) + pending_did_change[key] = nil + + -- 2. didClose the old uri — this removes the open-document + -- registration and nothing else. + pcall(pmacs.lsp.did_close, rec.server, old_uri) + + -- 3. Purge the routes, drain their awaiters, and clear all fourteen + -- stores plus `documents` for the old key. Runs against the OLD + -- server, which matters when step 4 picks a different one. + pcall(pmacs.lsp.forget_uri, rec.server, old_uri) + + if not new_uri then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 4. Re-run ensure_server. Server affinity keys on the detected + -- project root, so a rename ACROSS roots needs a different + -- server; a same-root rename reuses the existing one. + local sid = ensure_server(rec.language, new_buf_path) + if not sid then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 5. didOpen the new uri with the buffer's current text and a + -- fresh version. This also reclaims the tombstone for + -- exactly (server, new uri). + rec.server = sid + rec.uri = new_uri + rec.version = 1 + local ok_text, text = pcall(buffer_text, rec.buffer) + pcall(pmacs.lsp.did_open, sid, new_uri, rec.version, + ok_text and text or "") + -- 6. Re-root the diagnostic overlays. `DiagnosticView.uri` is + -- set once at construction and is private, so this is the + -- only way to move it — and the sweep reaches PASSIVE + -- windows, which the attach path cannot, while preserving + -- each overlay's position in the composition order. + pcall(pmacs.diag._rename_resource, old_uri, new_uri) + end + end + end +end) + +pmacs.hook.add("resource.deleted", function(path) + if type(path) ~= "string" then return end + for _, hit in ipairs(attachments_under(path)) do + local key, rec = hit.key, hit.rec + -- No flush: the document is gone, and shipping a didChange for a + -- file the server can no longer read buys nothing. + pending_did_change[key] = nil + pcall(pmacs.lsp.did_close, rec.server, rec.uri) + pcall(pmacs.lsp.forget_uri, rec.server, rec.uri) + -- Drop the record unconditionally. The buffer may be gone entirely + -- (an unmodified visited file is killed), in which case a retained + -- record is a dangling handle that `repull_for_attachments` would + -- iterate; and a modified buffer kept alive has no file to analyze + -- until it is saved, which re-attaches through the ordinary path. + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + end +end) From aa813ec3b62958595ca125630d66e0dee246e346 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:23:41 -0400 Subject: [PATCH 04/12] test(stage2a): unit pins for the URI teardown and the bus order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/lsp.rs` gains nine: the fourteen-family store inventory with a 17-entry precondition so it cannot pass vacuously, the route purge with `workspace/symbol` and another server's route both surviving, the awaiter drain joined on the rid, the error contract's two arms, the late-publish drop with its does-not-over-reach companion, the `mark_document_stale` gate across all three stale stores, exact-pair tombstone identity, and reclamation under both `start_generation` and terminal `forget`. `src/diag.rs` pins that `forget` drops the epoch while `clear` deliberately bumps it — the leak a `clear`-based forget would leave in the one map nothing prunes. `src/async_runtime.rs` injects two resource replies onto the private bus in each order and asserts `TickOutcome.resources` reports arrival order, not allocation order; plus that a failed or cancelled mutation is not harvested at all. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/async_runtime.rs | 120 ++++++++ src/diag.rs | 37 +++ src/lsp.rs | 654 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 811 insertions(+) diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 551458f..3620a32 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -1860,6 +1860,126 @@ mod tests { } } + /// dired Stage 2a, acceptance 54 (controlled-bus layer). Allocate + /// two resource jobs **without dispatching workers**, inject their + /// successful replies in a chosen order, and assert + /// `TickOutcome.resources` reports exactly that order. + /// + /// This is the honest statement of what the runtime guarantees: + /// `tick` drains the reply bus with `try_recv` and establishes no + /// execution token, so what a consumer sees is bus-arrival order. + /// The test fails against sorting by job id or kind, and against any + /// claim that the order recovers dispatch or filesystem-execution + /// order — because the injection order here is *deliberately* the + /// reverse of the allocation order in the first case. + #[test] + fn tick_reports_resources_in_bus_arrival_order_not_allocation_order() { + fn run(reverse: bool) -> Vec { + let rt = AsyncRuntime::with_pool_size(1); + let (a, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }), + ); + let (b, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }), + ); + let order = if reverse { [b, a] } else { [a, b] }; + for id in order { + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: id, + kind: ReplyKind::FsUnit, + }, + ) + .expect("inject reply"); + } + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both jobs settled"); + outcome.resources + } + + let a_first = ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }; + let b_first = ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }; + + assert_eq!( + run(true), + vec![b_first.clone(), a_first.clone()], + "B injected first must be reported first, even though A was \ + allocated first" + ); + assert_eq!( + run(false), + vec![a_first, b_first], + "and the reverse arrival order reverses the report" + ); + } + + /// A failed or cancelled mutation reconciles nothing, so it must not + /// appear in `resources` at all (acceptance 37's runtime half). + #[test] + fn a_failed_or_cancelled_resource_job_is_not_harvested() { + let rt = AsyncRuntime::with_pool_size(1); + let (failed, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/nope"), + to: PathBuf::from("/tmp/also-nope"), + }), + ); + let (cancelled, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/never"), + }), + ); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: failed, + kind: ReplyKind::Error("ENOENT".to_owned()), + }, + ) + .expect("inject"); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: cancelled, + kind: ReplyKind::Cancelled, + }, + ) + .expect("inject"); + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both settled"); + assert!( + outcome.resources.is_empty(), + "only Complete mutations are harvested; got {:?}", + outcome.resources + ); + } + #[test] fn dispatch_sum_completes_with_correct_value() { let rt = AsyncRuntime::with_pool_size(2); diff --git a/src/diag.rs b/src/diag.rs index f773321..0471c2e 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -772,6 +772,43 @@ mod tests { } } + /// dired Stage 2a §5, finding 4. `clear` *creates* an `epochs` + /// entry, because a consumer caching against the epoch has to see + /// that the diagnostics went away; nothing ever removes one. So a + /// `forget_uri` that called `clear` would leave a URI-keyed leak + /// behind in the one map nothing prunes — which is why the forget + /// path is its own store method. + #[test] + fn forget_drops_the_epoch_while_clear_deliberately_bumps_it() { + let mut store = DiagnosticStore::new(); + store.set("file:///a.rs", vec![diag(0, DiagnosticSeverity::Error, "boom")]); + store.mark_stale("file:///a.rs"); + assert_eq!(store.epoch_for("file:///a.rs"), 1); + + store.clear("file:///a.rs"); + assert_eq!( + store.epoch_for("file:///a.rs"), + 2, + "clear announces the removal to epoch-keyed caches" + ); + + store.set("file:///a.rs", vec![diag(0, DiagnosticSeverity::Error, "boom")]); + store.mark_stale("file:///a.rs"); + store.forget("file:///a.rs"); + assert!(store.for_uri("file:///a.rs").is_empty(), "diagnostics"); + assert!(!store.is_stale("file:///a.rs"), "stale flag"); + assert_eq!( + store.severity_counts_for("file:///a.rs"), + (0, 0, 0, 0), + "severity counts" + ); + assert_eq!( + store.epoch_for("file:///a.rs"), + 0, + "forget leaves no trace at all, epoch included" + ); + } + #[test] fn from_lsp_value_parses_minimal_diagnostic() { let v = json!({ diff --git a/src/lsp.rs b/src/lsp.rs index 2d78f4a..fefc687 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -4074,3 +4074,657 @@ mod tests { assert_eq!(resolve_config_section(&s, Some("")), s); } } + +// --------------------------------------------------------------------------- +// dired Stage 2a — `forget_uri`, and the tombstone that gates the +// uncorrelated resurrection paths (§5, acceptance 31 / 31b / 31c / 31d). +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod resource_reconciliation_tests { + use super::*; + use crate::async_runtime::JobOutcome; + + /// A manager plus two live-enough clients. `/bin/cat` blocks on + /// stdin, so both stay in `Starting` for the whole test and every + /// notification is deferred rather than written — which is exactly + /// what these tests want: they assert on manager-owned state, not on + /// wire traffic. + fn manager_with_two_servers() -> (LspManager, LspServerId, LspServerId) { + use std::cell::RefCell; + use std::rc::Rc; + let sup = Rc::new(RefCell::new(crate::process::ProcessSupervisor::new())); + let runtime = Rc::new(crate::async_runtime::AsyncRuntime::with_pool_size(1)); + let mut mgr = LspManager::new(sup, runtime); + let mut spec_a = LspServerSpec::new("a", "rust", "/bin/cat"); + spec_a.restart = LspRestartPolicy::Never; + let mut spec_b = LspServerSpec::new("b", "rust", "/bin/cat"); + spec_b.restart = LspRestartPolicy::Never; + let a = mgr.spawn(spec_a).expect("spawn a"); + let b = mgr.spawn(spec_b).expect("spawn b"); + (mgr, a, b) + } + + fn publish(mgr: &mut LspManager, sid: LspServerId, uri: &str, message: &str) { + mgr.handle_notification( + sid, + "textDocument/publishDiagnostics".to_owned(), + json!({ + "uri": uri, + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + }, + "severity": 1, + "message": message, + }], + }), + Instant::now(), + ); + } + + fn diag_messages(mgr: &LspManager, uri: &str) -> Vec { + mgr.diag_store + .lock() + .expect("diag store") + .for_uri(uri) + .iter() + .map(|d| d.message.clone()) + .collect() + } + + /// Populate every one of the fourteen URI-keyed store families plus + /// the `documents` mirror for `(sid, uri)`. + fn populate_all_stores(mgr: &mut LspManager, sid: LspServerId, uri: &str) { + let server = sid.raw().to_string(); + publish(mgr, sid, uri, "a diagnostic"); + mgr.completion_store.lock().unwrap().set( + crate::completion::CompletionKey::new(server.clone(), uri), + crate::completion::CompletionResponse::from_lsp_value(&json!([{ "label": "x" }])), + ); + mgr.hover_store.lock().unwrap().set( + crate::hover::HoverKey::new(server.clone(), uri), + crate::hover::Hover::from_lsp_value(&json!({ "contents": "doc" })) + .expect("a hover payload with contents parses"), + ); + mgr.signature_store.lock().unwrap().set( + crate::signature::SignatureKey::new(server.clone(), uri), + crate::signature::SignatureHelp::from_lsp_value( + &json!({ "signatures": [{ "label": "f()" }] }), + ), + ); + mgr.definition_store.lock().unwrap().set( + crate::definition::DefinitionKey::new(server.clone(), uri), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + mgr.locations_store.lock().unwrap().set( + crate::locations::LocationsKey::new(server.clone(), uri, kind), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + } + mgr.symbol_store.lock().unwrap().set( + crate::symbol::SymbolKey::document(server.clone(), uri), + crate::symbol::SymbolResponse::from_lsp_value( + &json!([{ + "name": "S", "kind": 5, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "selectionRange": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + }]), + uri, + ), + ); + mgr.document_highlight_store.lock().unwrap().set( + crate::document_highlight::DocumentHighlightKey::new(server.clone(), uri), + crate::document_highlight::DocumentHighlightResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 } }, + }])), + ); + mgr.formatting_store.lock().unwrap().set( + crate::formatting::FormattingKey::new(server.clone(), uri), + crate::formatting::FormattingResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "newText": "x", + }])), + ); + mgr.rename_store.lock().unwrap().set( + crate::rename::RenameKey::new(server.clone(), uri), + crate::rename::WorkspaceEditResponse::from_lsp_value(&json!({ "changes": {} })), + ); + mgr.prepare_rename_store.lock().unwrap().set( + crate::prepare_rename::PrepareRenameKey::new(server.clone(), uri), + crate::prepare_rename::PrepareRenameResponse::from_lsp_value(&json!({ + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + })), + ); + mgr.code_action_store.lock().unwrap().set( + crate::code_action::CodeActionKey::new(server.clone(), uri), + crate::code_action::CodeActionResponse::from_lsp_value(&json!([{ "title": "fix" }])), + ); + mgr.inlay_hint_store.lock().unwrap().set( + crate::inlay_hint::InlayHintKey::new(server.clone(), uri), + crate::inlay_hint::InlayHintResponse::from_lsp_value(&json!([{ + "position": { "line": 0, "character": 0 }, + "label": ": i32", + }])), + ); + mgr.semantic_token_store.lock().unwrap().set( + crate::semantic_tokens::SemanticTokenKey::new(server, uri), + crate::semantic_tokens::SemanticTokensResponse::from_lsp_value(&json!({ + "data": [0, 0, 1, 0, 0], + })), + ); + mgr.documents.insert((sid, uri.to_owned()), "text".to_owned()); + } + + /// Which of the fourteen families still hold an entry for + /// `(sid, uri)`, by name. An empty vector is the post-forget + /// expectation; naming the survivors is what makes a failure + /// actionable instead of "assert!(false)". + fn populated_families(mgr: &LspManager, sid: LspServerId, uri: &str) -> Vec<&'static str> { + let server = sid.raw().to_string(); + let mut out = Vec::new(); + if !diag_messages(mgr, uri).is_empty() { + out.push("diag"); + } + if mgr + .completion_store + .lock() + .unwrap() + .get(&crate::completion::CompletionKey::new(server.clone(), uri)) + .is_some() + { + out.push("completion"); + } + if mgr + .hover_store + .lock() + .unwrap() + .get(&crate::hover::HoverKey::new(server.clone(), uri)) + .is_some() + { + out.push("hover"); + } + if mgr + .signature_store + .lock() + .unwrap() + .get(&crate::signature::SignatureKey::new(server.clone(), uri)) + .is_some() + { + out.push("signature"); + } + if mgr + .definition_store + .lock() + .unwrap() + .get(&crate::definition::DefinitionKey::new(server.clone(), uri)) + .is_some() + { + out.push("definition"); + } + for (kind, label) in [ + (crate::locations::LocationKind::References, "references"), + (crate::locations::LocationKind::Declaration, "declaration"), + ( + crate::locations::LocationKind::TypeDefinition, + "typeDefinition", + ), + ( + crate::locations::LocationKind::Implementation, + "implementation", + ), + ] { + if mgr + .locations_store + .lock() + .unwrap() + .get(&crate::locations::LocationsKey::new( + server.clone(), + uri, + kind, + )) + .is_some() + { + out.push(label); + } + } + if mgr + .symbol_store + .lock() + .unwrap() + .get(&crate::symbol::SymbolKey::document(server.clone(), uri)) + .is_some() + { + out.push("symbol"); + } + if mgr + .document_highlight_store + .lock() + .unwrap() + .get(&crate::document_highlight::DocumentHighlightKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("documentHighlight"); + } + if mgr + .formatting_store + .lock() + .unwrap() + .get(&crate::formatting::FormattingKey::new(server.clone(), uri)) + .is_some() + { + out.push("formatting"); + } + if mgr + .rename_store + .lock() + .unwrap() + .get(&crate::rename::RenameKey::new(server.clone(), uri)) + .is_some() + { + out.push("rename"); + } + if mgr + .prepare_rename_store + .lock() + .unwrap() + .get(&crate::prepare_rename::PrepareRenameKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("prepareRename"); + } + if mgr + .code_action_store + .lock() + .unwrap() + .get(&crate::code_action::CodeActionKey::new(server.clone(), uri)) + .is_some() + { + out.push("codeAction"); + } + if mgr + .inlay_hint_store + .lock() + .unwrap() + .get(&crate::inlay_hint::InlayHintKey::new(server.clone(), uri)) + .is_some() + { + out.push("inlayHint"); + } + if mgr + .semantic_token_store + .lock() + .unwrap() + .get(&crate::semantic_tokens::SemanticTokenKey::new(server, uri)) + .is_some() + { + out.push("semanticTokens"); + } + out + } + + /// Acceptance 31, store half — every one of the fourteen families + /// plus `documents` loses its entry. The `populated_families` + /// precondition is what makes this bite: an assertion that the + /// stores are empty afterwards passes vacuously if nothing filled + /// them. + #[test] + fn forget_uri_clears_all_fourteen_store_families_and_the_document_mirror() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let uri = "file:///tmp/old.rs"; + populate_all_stores(&mut mgr, a, uri); + let before = populated_families(&mgr, a, uri); + assert_eq!( + before.len(), + 17, + "precondition: every family must hold an entry before the forget \ + (14 families, of which `locations` counts four kinds); got {before:?}" + ); + assert!(mgr.documents.contains_key(&(a, uri.to_owned()))); + + mgr.forget_uri(a, uri).expect("forget a known server"); + + let after = populated_families(&mgr, a, uri); + assert!( + after.is_empty(), + "these families survived the forget: {after:?}" + ); + assert!( + !mgr.documents.contains_key(&(a, uri.to_owned())), + "the `documents` mirror is what didChange diffs against, so a \ + stale entry under the old URI is a correctness problem" + ); + } + + /// Acceptance 31, route half, plus W3's exemption. A response + /// already in flight at rename time must not repopulate the old key + /// after the clear — and a `workspace/symbol` route, which carries a + /// query and no URI at all, must survive. + #[test] + fn forget_uri_purges_routes_for_the_uri_and_retains_workspace_symbol() { + let (mut mgr, a, b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + mgr.pending_routes.insert( + (a, 1), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 2), + ResponseRoute::Locations { + uri: old.to_owned(), + kind: crate::locations::LocationKind::References, + }, + ); + mgr.pending_routes.insert( + (a, 3), + ResponseRoute::WorkspaceSymbol { + query: "Widget".to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 4), + ResponseRoute::Hover { + uri: other.to_owned(), + }, + ); + // Same URI, different server: another server's in-flight work is + // not ours to cancel. + mgr.pending_routes.insert( + (b, 5), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + + mgr.forget_uri(a, old).expect("forget"); + + assert!(!mgr.pending_routes.contains_key(&(a, 1)), "hover for old"); + assert!( + !mgr.pending_routes.contains_key(&(a, 2)), + "locations for old" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 3)), + "workspace/symbol carries no URI and is not scoped to any \ + document, so a rename does not invalidate it" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 4)), + "an unrelated document's route must survive" + ); + assert!( + mgr.pending_routes.contains_key(&(b, 5)), + "another server's route for the same URI must survive" + ); + } + + /// Acceptance 31, drain half. `pending_external` holds the + /// `Handle:await()` side, and its own contract says it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped, and the one with the similar name + /// (`drain_cancelled_externals`) removes only awaiters whose token + /// was flipped or which timed out — **a rename flips no token**, so + /// modelling on it would drain nothing and park the coroutine + /// forever. + #[test] + fn forget_uri_settles_the_awaiters_joined_to_the_purged_routes() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + let runtime = mgr.runtime.clone(); + + let mut register = |rid: u64, uri: &str| { + let (job_id, token) = runtime.register_external(JobKind::LspRequest, None); + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + mgr.pending_external.insert( + (a, rid), + PendingExternal { + method: "textDocument/hover".to_owned(), + awaiters: vec![Awaiter { job_id, token }], + dispatched_at: Instant::now(), + }, + ); + job_id + }; + let doomed = register(1, old); + let survivor = register(2, other); + + // Nothing has settled yet: the drain, not the registration, is + // what must produce the outcome. + let _ = runtime.tick(); + assert!(!runtime.is_complete(doomed)); + assert!(!runtime.is_complete(survivor)); + + mgr.forget_uri(a, old).expect("forget"); + let _ = runtime.tick(); + + assert!( + matches!(runtime.take_result(doomed), Some(JobOutcome::Cancelled)), + "an awaiter parked on a route we just purged must wake cancelled" + ); + assert!( + !mgr.pending_external.contains_key(&(a, 1)), + "and its entry must be gone, not merely settled" + ); + assert!( + runtime.take_result(survivor).is_none(), + "an unrelated document's awaiter keeps waiting" + ); + assert!(mgr.pending_external.contains_key(&(a, 2))); + } + + /// Acceptance 31c — the error contract, both arms. The second is the + /// one that matters: the subscriber runs per attachment, an + /// attachment need not have any pending route or populated result, + /// and repeated cleanup after a partial teardown must stay safe. + #[test] + fn forget_uri_raises_for_an_unknown_server_and_succeeds_with_no_state() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let unknown = LspServerId::next(); + let err = mgr + .forget_uri(unknown, "file:///tmp/x.rs") + .expect_err("unknown server must raise, matching `forget`"); + assert!(err.contains("unknown server"), "{err}"); + + mgr.forget_uri(a, "file:///tmp/never-touched.rs") + .expect("a URI with no state under a known server is an \ + idempotent success, not an error"); + mgr.forget_uri(a, "file:///tmp/never-touched.rs") + .expect("and repeating it stays safe"); + } + + /// Acceptance 31b — the uncorrelated write. This notification + /// carries no request id, so the route purge cannot see it, and + /// `diag_store` has no correlated writers at all. The companion + /// assertion is that the tombstone does **not** over-reach: a + /// publish for a different, never-opened URI is still absorbed, + /// which is exactly what a `documents` membership gate would have + /// broken. + #[test] + fn a_late_publish_for_a_forgotten_uri_is_dropped_and_an_unopened_uri_is_not() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + publish(&mut mgr, a, old, "before"); + assert_eq!(diag_messages(&mgr, old), vec!["before".to_owned()]); + + mgr.forget_uri(a, old).expect("forget"); + assert!(diag_messages(&mgr, old).is_empty(), "cleared by the forget"); + + publish(&mut mgr, a, old, "late arrival"); + assert!( + diag_messages(&mgr, old).is_empty(), + "a publish naming a URI we explicitly forgot must be dropped" + ); + + // Servers legitimately publish for files the editor never + // opened — a crate-wide push naming a dependency. + let never_opened = "file:///tmp/dependency.rs"; + publish(&mut mgr, a, never_opened, "third-party"); + assert_eq!( + diag_messages(&mgr, never_opened), + vec!["third-party".to_owned()], + "the tombstone drops only what we forgot, never everything \ + outside `documents`" + ); + } + + /// Acceptance 31b, second gate. `mark_document_stale` creates URI + /// keys in three stores, so without its own check a forgotten URI + /// regains a stale flag in all three. + #[test] + fn mark_document_stale_cannot_flag_a_forgotten_pair_in_any_of_the_three_stores() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + mgr.forget_uri(a, old).expect("forget"); + + mgr.mark_document_stale(a, old); + + assert!( + !mgr.diag_store.lock().unwrap().is_stale(old), + "diagnostics" + ); + assert!( + !mgr.semantic_token_store.lock().unwrap().is_stale(old), + "semantic tokens" + ); + assert!( + !mgr.inlay_hint_store.lock().unwrap().is_stale(old), + "inlay hints" + ); + + // And it still works for a URI we did not forget, so the gate is + // the tombstone and not a blanket disable. + let live = "file:///tmp/live.rs"; + mgr.mark_document_stale(a, live); + assert!(mgr.diag_store.lock().unwrap().is_stale(live)); + } + + /// Acceptance 31d — identity and reclamation are exact. Tombstone + /// one URI under two servers; `did_open(A, uri)` clears only A's + /// pair, so an A write is admitted while a later B write is dropped. + #[test] + fn the_tombstone_is_keyed_by_the_exact_server_uri_pair() { + let (mut mgr, a, b) = manager_with_two_servers(); + let uri = "file:///tmp/shared.rs"; + mgr.forget_uri(a, uri).expect("forget under a"); + mgr.forget_uri(b, uri).expect("forget under b"); + assert!(mgr.is_forgotten(a, uri)); + assert!(mgr.is_forgotten(b, uri)); + + mgr.did_open(a, uri, 1, "text").expect("reopen under a"); + assert!( + !mgr.is_forgotten(a, uri), + "reopening is the editor saying it holds the URI again" + ); + assert!( + mgr.is_forgotten(b, uri), + "and it says nothing about another server's tombstone" + ); + + publish(&mut mgr, a, uri, "from A"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "A's write is admitted after A reopened" + ); + publish(&mut mgr, b, uri, "from B"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "B is still tombstoned for this URI, so its later write is \ + dropped and A's payload survives untouched" + ); + } + + /// Acceptance 31d — a restart generation flip drops every pair for + /// its own server and retains every other server's. + #[test] + fn start_generation_reclaims_only_the_flipped_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(a, "file:///tmp/a2.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 3); + + let mut client = mgr.clients.remove(&b).expect("client b"); + mgr.start_generation(b, &mut client).expect("restart b"); + mgr.clients.insert(b, client); + + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(mgr.is_forgotten(a, "file:///tmp/a2.rs")); + assert!( + !mgr.is_forgotten(b, "file:///tmp/b1.rs"), + "B's generation is gone, so B's tombstones go with it" + ); + assert_eq!(mgr.forgotten_document_count(), 2); + } + + /// Acceptance 31d — terminal `forget` likewise, and the set is empty + /// once the only owning generation is torn down. This is what makes + /// the set reclaimed rather than a leak; it deliberately is **not** + /// size-bounded, because a capacity or LRU eviction would let an + /// arbitrarily late notification resurrect an evicted key. + #[test] + fn terminal_forget_reclaims_only_its_own_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 2); + + if let Some(client) = mgr.clients.get_mut(&b) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(b).expect("forget b"); + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(!mgr.is_forgotten(b, "file:///tmp/b1.rs")); + assert_eq!(mgr.forgotten_document_count(), 1); + + if let Some(client) = mgr.clients.get_mut(&a) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(a).expect("forget a"); + assert_eq!( + mgr.forgotten_document_count(), + 0, + "the set is empty once the owning generations are gone" + ); + } +} From 3c66370b8c2f2fb03933aa99e21635f5f81a1d54 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:40:38 -0400 Subject: [PATCH 05/12] test(stage2a): the reconciliation acceptance suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tests/resource_reconciliation_acceptance.rs`, 23 rows, no dired content — items 23–37 and 50–55 driven through the real entry points: `pmacs.fs.rename` / `pmacs.fs.remove` fire-and-forget for the drain harvest, `pmacs.buffer.apply_resource_op` for the synchronous arm, and the fake server's `workspace/applyEdit` for the applier. The rows that took design rather than transcription: Item 27 opens two descendants AND two buffers on one exact path, since one child would not defeat a first-match lookup. Item 29 tests name provenance in both directions, including a name explicitly set to a string that normalizes to the file's own path — the case a path-equivalence heuristic gets wrong. Item 30 paints a real frame and counts diagnostic underlines per window rect, because `DiagnosticView.uri` is private and a store assertion would prove nothing about re-rooting; it also pins each overlay's index in the composition order, which is what a remove-and-re-push breaks. Item 53b states its three assertions individually, since a compound check can pass on two of the three. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/diag.rs | 12 +- src/lsp.rs | 23 +- tests/resource_reconciliation_acceptance.rs | 1733 +++++++++++++++++++ 3 files changed, 1757 insertions(+), 11 deletions(-) create mode 100644 tests/resource_reconciliation_acceptance.rs diff --git a/src/diag.rs b/src/diag.rs index 0471c2e..b81a4dc 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -512,7 +512,7 @@ impl View for DiagnosticView { /// position in the window's composition order. fn rename_resource(&mut self, old_uri: &str, new_uri: &str) { if self.uri == old_uri { - self.uri = new_uri.to_owned(); + new_uri.clone_into(&mut self.uri); } } @@ -781,7 +781,10 @@ mod tests { #[test] fn forget_drops_the_epoch_while_clear_deliberately_bumps_it() { let mut store = DiagnosticStore::new(); - store.set("file:///a.rs", vec![diag(0, DiagnosticSeverity::Error, "boom")]); + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); store.mark_stale("file:///a.rs"); assert_eq!(store.epoch_for("file:///a.rs"), 1); @@ -792,7 +795,10 @@ mod tests { "clear announces the removal to epoch-keyed caches" ); - store.set("file:///a.rs", vec![diag(0, DiagnosticSeverity::Error, "boom")]); + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); store.mark_stale("file:///a.rs"); store.forget("file:///a.rs"); assert!(store.for_uri("file:///a.rs").is_empty(), "diagnostics"); diff --git a/src/lsp.rs b/src/lsp.rs index fefc687..ba32fb2 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -3171,6 +3171,10 @@ impl LspManager { /// attachment need not have any pending route or populated result /// store, and cleanup can be repeated after an earlier partial /// teardown. + #[allow( + clippy::too_many_lines, + reason = "the fourteen store families are a flat inventory; splitting it is how one of them gets forgotten, which is the defect this method exists to prevent" + )] pub fn forget_uri(&mut self, sid: LspServerId, uri: &str) -> Result<(), String> { if !self.clients.contains_key(&sid) { return Err(format!("unknown server: {sid}")); @@ -4233,13 +4237,18 @@ mod resource_reconciliation_tests { "data": [0, 0, 1, 0, 0], })), ); - mgr.documents.insert((sid, uri.to_owned()), "text".to_owned()); + mgr.documents + .insert((sid, uri.to_owned()), "text".to_owned()); } /// Which of the fourteen families still hold an entry for /// `(sid, uri)`, by name. An empty vector is the post-forget /// expectation; naming the survivors is what makes a failure /// actionable instead of "assert!(false)". + #[allow( + clippy::too_many_lines, + reason = "one probe per store family, mirroring the inventory under test" + )] fn populated_families(mgr: &LspManager, sid: LspServerId, uri: &str) -> Vec<&'static str> { let server = sid.raw().to_string(); let mut out = Vec::new(); @@ -4560,9 +4569,10 @@ mod resource_reconciliation_tests { .expect_err("unknown server must raise, matching `forget`"); assert!(err.contains("unknown server"), "{err}"); - mgr.forget_uri(a, "file:///tmp/never-touched.rs") - .expect("a URI with no state under a known server is an \ - idempotent success, not an error"); + mgr.forget_uri(a, "file:///tmp/never-touched.rs").expect( + "a URI with no state under a known server is an \ + idempotent success, not an error", + ); mgr.forget_uri(a, "file:///tmp/never-touched.rs") .expect("and repeating it stays safe"); } @@ -4613,10 +4623,7 @@ mod resource_reconciliation_tests { mgr.mark_document_stale(a, old); - assert!( - !mgr.diag_store.lock().unwrap().is_stale(old), - "diagnostics" - ); + assert!(!mgr.diag_store.lock().unwrap().is_stale(old), "diagnostics"); assert!( !mgr.semantic_token_store.lock().unwrap().is_stale(old), "semantic tokens" diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs new file mode 100644 index 0000000..90b4e34 --- /dev/null +++ b/tests/resource_reconciliation_acceptance.rs @@ -0,0 +1,1733 @@ +//! dired Stage 2a acceptance — rename and delete reconciliation. +//! +//! `docs/dired-stage2-framing.md` §5, §6, §10; acceptance items 23–38 +//! and 50–55. +//! +//! **This suite contains no dired content.** Stage 2a ships no dired +//! surface at all: it is the substrate transaction that Stage 2b's `R`, +//! `D` and `x` then stand on, and it closes three defects on `main` +//! that need no dired to be worth fixing — the workspace-edit phantom +//! buffer, the raw first-match registry lookup both `apply_resource_op` +//! arms used, and the incomplete removal lifecycle. +//! +//! Two disciplines the framing forces on every row here: +//! +//! * **Drive the real entry point.** A reconciliation with no +//! production caller passes every direct-call test, so the rename +//! rows go through `pmacs.fs.rename` (worker-dispatched, harvested in +//! the drain) or through `pmacs.buffer.apply_resource_op` (synchronous, +//! main-thread), never through `EditorCore::reconcile_rename`. +//! * **Pump to quiescence, never to a frame count**, because every +//! mutation is worker-dispatched. + +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; + +use pmacs::editor::EditorState; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn exec(state: &EditorState, source: &str) { + state + .lua_host + .lua() + .load(source.to_owned()) + .exec() + .unwrap_or_else(|e| panic!("lua exec failed: {e}\n--- source ---\n{source}")); +} + +fn eval(state: &EditorState, source: &str) -> T { + state + .lua_host + .lua() + .load(source.to_owned()) + .eval() + .unwrap_or_else(|e| panic!("lua eval failed: {e}\n--- source ---\n{source}")) +} + +/// Escape a path for embedding in a Lua double-quoted string. +fn lua_str(path: &Path) -> String { + path.display() + .to_string() + .replace('\\', "\\\\") + .replace('"', "\\\"") +} + +/// Pump the async runtime until `predicate` holds or the deadline +/// lapses. Quiescence, not a frame count: the whole point of items 24 +/// and 25 is that the reconciliation happens in the drain, and the drain +/// runs whenever a reply arrives. +fn pump_until bool>(state: &mut EditorState, what: &str, predicate: F) { + let deadline = Instant::now() + Duration::from_secs(5); + while !predicate(state) { + assert!(Instant::now() < deadline, "pump deadline exceeded: {what}"); + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// Pump a fixed number of times without any expectation. Used only to +/// give a dispatched job every chance to settle before asserting that +/// something did **not** happen. +fn pump_a_while(state: &mut EditorState) { + for _ in 0..80 { + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// A canonicalized temp directory. Canonicalized because the buffer +/// registry stores lexically-normalized absolute paths and macOS's +/// `/var` is a symlink to `/private/var`; without this the expected +/// paths below would differ from the stored ones by a symlink hop. +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +impl Fixture { + fn new() -> Self { + let dir = tempfile::tempdir().unwrap(); + let root = std::fs::canonicalize(dir.path()).unwrap(); + Self { _dir: dir, root } + } + + fn write(&self, rel: &str, contents: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, contents).unwrap(); + path + } + + fn dir(&self, rel: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(&path).unwrap(); + path + } + + fn at(&self, rel: &str) -> PathBuf { + self.root.join(rel) + } +} + +fn editor() -> EditorState { + let state = EditorState::new(); + // No language server may spawn from these fixtures. The LSP rows + // that DO want one configure it explicitly. + exec(&state, "pmacs.lsp.config = {}"); + state +} + +/// Open `path` into a buffer and return the Lua global name holding its +/// handle. Buffers are held on globals so a test can re-read their path +/// and name after the reconciliation moved them. +fn open_as(state: &EditorState, global: &str, path: &Path) { + exec( + state, + &format!( + "_G.{global} = pmacs.buffer.find_or_open(\"{}\")", + lua_str(path) + ), + ); +} + +fn buffer_path(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:path() or nil"), + ) +} + +fn buffer_name(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:name() or nil"), + ) +} + +fn buffer_is_valid(state: &EditorState, global: &str) -> bool { + eval( + state, + &format!("local b = _G.{global}; return (b ~= nil) and b:is_valid()"), + ) +} + +/// Dispatch a rename **without awaiting** the handle, then pump. +/// Fire-and-forget is the shape item 25 pins: the reconciliation must +/// not live at result consumption. +fn rename_fire_and_forget(state: &mut EditorState, from: &Path, to: &Path) { + exec( + state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(from), + lua_str(to) + ), + ); + pump_until(state, "rename lands on disk", |_| to.exists()); + // The rename landing on disk and the reply reaching the main thread + // are two events; pump past the first to reach the second. + pump_a_while(state); +} + +fn remove_fire_and_forget(state: &mut EditorState, path: &Path) { + exec(state, &format!("pmacs.fs.remove(\"{}\")", lua_str(path))); + pump_until(state, "remove lands on disk", |_| !path.exists()); + pump_a_while(state); +} + +// --------------------------------------------------------------------------- +// 25 — no-await rename +// --------------------------------------------------------------------------- + +/// Acceptance 25. Dispatch `pmacs.fs.rename`, **never take the +/// result**, pump: the open buffer's path has moved. +/// +/// Bite: fails if the reconciliation lives at result consumption +/// (`_take_result`) rather than in the drain, because nothing here ever +/// consumes the handle. +#[test] +fn acc25_a_never_awaited_rename_still_moves_the_open_buffers_path() { + let fx = Fixture::new(); + let old = fx.write("notes.txt", "hello\n"); + let new = fx.at("renamed.txt"); + let mut state = editor(); + open_as(&state, "B", &old); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()) + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must follow a fire-and-forget rename" + ); +} + +// --------------------------------------------------------------------------- +// 26, 27, 28 — the walk +// --------------------------------------------------------------------------- + +/// Acceptance 26. A buffer open on `dir/child.txt` follows +/// `dir` → `newdir`. +#[test] +fn acc26_a_buffer_under_a_renamed_directory_follows_it() { + let fx = Fixture::new(); + fx.dir("tree"); + let child = fx.write("tree/child.txt", "x\n"); + let mut state = editor(); + open_as(&state, "B", &child); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(fx.at("newtree/child.txt").to_str().unwrap()), + "a descendant keeps its relative tail under the new root" + ); +} + +/// Acceptance 27. **Every** match, not the first: two descendant +/// buffers under the renamed directory *and* two buffers visiting the +/// same exact path all move. +/// +/// Bite: fails against `find_by_path`'s first match. One child buffer +/// would not defeat a first-match implementation; this does, twice over +/// — and the duplicate-path pair is the case `find_by_path` cannot even +/// see, because it returns on the first hit. +#[test] +fn acc27_every_affected_buffer_moves_not_only_the_first() { + let fx = Fixture::new(); + fx.dir("tree"); + let one = fx.write("tree/one.txt", "1\n"); + let two = fx.write("tree/nested/two.txt", "2\n"); + let mut state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &two); + // Two buffers on the SAME exact path. `pmacs.buffer.from_file` + // creates a fresh buffer without deduping, which is how a duplicate + // path binding is reachable from public Lua. + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + let dup_first: String = eval(&state, "return _G.ONE:path()"); + let dup_second: String = eval(&state, "return _G.DUP:path()"); + assert_eq!( + dup_first, dup_second, + "precondition: two distinct buffers bound to one path" + ); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "ONE").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "first descendant" + ); + assert_eq!( + buffer_path(&state, "TWO").as_deref(), + Some(fx.at("newtree/nested/two.txt").to_str().unwrap()), + "second, more deeply nested descendant" + ); + assert_eq!( + buffer_path(&state, "DUP").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "the second buffer on the same path — invisible to a first-match \ + lookup, and left pointing at nothing by one" + ); +} + +/// Acceptance 28. Renaming `/…/foo` must not rebind a buffer on +/// `/…/foobar`. +/// +/// Bite: fails against a string `starts_with` instead of a +/// path-component prefix. +#[test] +fn acc28_a_false_string_prefix_is_not_a_path_prefix() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let mut state = editor(); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + rename_fire_and_forget(&mut state, &fx.at("foo"), &fx.at("renamed")); + + assert_eq!( + buffer_path(&state, "IN").as_deref(), + Some(fx.at("renamed/a.txt").to_str().unwrap()), + "the real descendant moves" + ); + assert_eq!( + buffer_path(&state, "OUT").as_deref(), + Some(sibling.to_str().unwrap()), + "`foobar.txt` shares a string prefix with `foo` and is not under it" + ); +} + +// --------------------------------------------------------------------------- +// 29 — name provenance, both directions +// --------------------------------------------------------------------------- + +/// Acceptance 29(a). A buffer opened by a **relative** path is named +/// `foo.rs` while its stored path is absolute, and its name follows the +/// rename because its load site recorded `PathDerived`. +/// +/// Bite: rev 7's string-equality rule fails this — the name never +/// equalled the normalized path, so it would have been left stale while +/// insisting it was user-chosen. +#[test] +fn acc29a_a_relative_opens_name_follows_the_rename() { + let fx = Fixture::new(); + let old = fx.write("relative.txt", "x\n"); + let new = fx.at("moved.txt"); + let mut state = editor(); + // Open by a path whose *spelling* is not the stored path: a `.` + // component is folded by normalization but kept in the name, which + // reproduces the relative-open shape without depending on the + // process cwd. + let as_given = fx.at("./relative.txt"); + open_as(&state, "B", &as_given); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(as_given.to_str().unwrap()), + "precondition: the name is the path AS GIVEN, not the stored path" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: only the stored path is normalized" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()) + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the name follows because the load site recorded path provenance, \ + not because the old name happened to match the old path" + ); +} + +/// Acceptance 29(b). A name set explicitly through +/// `pmacs.buffer.set_name` survives the rename **even when that string +/// normalizes to the file's own stored path**. +/// +/// Bite: rev 8's path-equivalence heuristic fails this — the chosen +/// name normalizes to the exact stored path, so the heuristic would +/// overwrite it. +#[test] +fn acc29b_an_explicitly_set_name_survives_even_when_it_denotes_the_file() { + let fx = Fixture::new(); + let old = fx.write("notes", "x\n"); + let new = fx.at("notes-renamed"); + let mut state = editor(); + open_as(&state, "B", &old); + // The chosen name IS the file's absolute path. Under a + // path-equivalence rule this is indistinguishable from a + // path-derived name; under recorded provenance it is not. + exec( + &state, + &format!("pmacs.buffer.set_name(_G.B, \"{}\")", lua_str(&old)), + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: the explicit name normalizes to the stored path" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the PATH always follows" + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "and the explicitly chosen name does not, however much it looks \ + like a path-derived one" + ); +} + +// --------------------------------------------------------------------------- +// 36, 37 — the synchronous arm, and failure +// --------------------------------------------------------------------------- + +/// Acceptance 36. `apply_resource_op`'s rename finds a buffer whose +/// stored path is normalized but whose op names it **un-normalized**. +/// +/// Bite: fails against the raw `find_by_path(&from)` this arm used — +/// stored paths are normalized on write, so a raw lookup with a `.` +/// component in it misses the buffer entirely and the rename silently +/// reconciles nothing. +#[test] +fn acc36_the_synchronous_arm_matches_an_un_normalized_op_path() { + let fx = Fixture::new(); + let old = fx.write("sync.txt", "x\n"); + let new = fx.at("sync-moved.txt"); + let state = editor(); + open_as(&state, "B", &old); + + let unnormalized = fx.at("./sync.txt"); + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"rename\", \ + old_path = \"{}\", new_path = \"{}\" }}", + lua_str(&unnormalized), + lua_str(&new) + ), + ); + + assert!(new.exists(), "the rename happened on disk"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must be found even though the op spelled the source \ + path differently from the stored one" + ); +} + +/// Acceptance 37. A **failed** rename reconciles nothing — and fires no +/// hook. +#[test] +fn acc37_a_failed_rename_reconciles_nothing_and_fires_no_hook() { + let fx = Fixture::new(); + let present = fx.write("present.txt", "x\n"); + let missing = fx.at("does-not-exist.txt"); + let mut state = editor(); + open_as(&state, "B", &present); + exec( + &state, + "_G.FIRED = 0 + pmacs.hook.add('resource.renamed', function() _G.FIRED = _G.FIRED + 1 end)", + ); + + // Renaming a path that does not exist fails in the worker. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&missing), + lua_str(&fx.at("target.txt")) + ), + ); + pump_a_while(&mut state); + + let fired: i64 = eval(&state, "return _G.FIRED"); + assert_eq!(fired, 0, "a failed mutation reconciles nothing"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(present.to_str().unwrap()), + "and no unrelated buffer moved" + ); +} + +// --------------------------------------------------------------------------- +// 50, 55 — the hooks +// --------------------------------------------------------------------------- + +/// Acceptance 50. `resource.renamed` fires **exactly once** per +/// successful rename, with `(old, new)` as normalized absolute paths, +/// and does not fire for a rename that failed. The symmetric assertion +/// for `resource.deleted` accompanies it. +/// +/// Bite: fails if the hook fires for a failed rename, or fires with the +/// un-normalized path a caller happened to spell. +#[test] +fn acc50_the_hooks_fire_once_with_normalized_paths() { + let fx = Fixture::new(); + let old = fx.write("hooked.txt", "x\n"); + let new = fx.at("hooked-moved.txt"); + let doomed = fx.write("doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.RENAMES = {} + _G.DELETES = {} + pmacs.hook.add('resource.renamed', function(a, b) + _G.RENAMES[#_G.RENAMES + 1] = tostring(a) .. ' -> ' .. tostring(b) + end) + pmacs.hook.add('resource.deleted', function(p) + _G.DELETES[#_G.DELETES + 1] = tostring(p) + end)", + ); + + // Spell BOTH paths un-normalized, so the hook's arguments can only + // be canonical if the fire site normalizes them. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&fx.at("./hooked.txt")), + lua_str(&fx.at("./hooked-moved.txt")) + ), + ); + pump_until(&mut state, "rename hook", |s| { + let n: i64 = eval(s, "return #_G.RENAMES"); + n > 0 + }); + pump_a_while(&mut state); + + let renames: String = eval(&state, "return table.concat(_G.RENAMES, '|')"); + assert_eq!( + renames, + format!("{} -> {}", old.display(), new.display()), + "exactly one row, and both paths canonical — a path-keyed \ + subscriber needs the form the registry keys on" + ); + + exec( + &state, + &format!("pmacs.fs.remove(\"{}\")", lua_str(&fx.at("./doomed.txt"))), + ); + pump_until(&mut state, "delete hook", |s| { + let n: i64 = eval(s, "return #_G.DELETES"); + n > 0 + }); + pump_a_while(&mut state); + let deletes: String = eval(&state, "return table.concat(_G.DELETES, '|')"); + assert_eq!( + deletes, + doomed.display().to_string(), + "one row, canonical path" + ); +} + +/// Acceptance 55. Both hooks are `all-must-succeed`, not +/// short-circuit: with two subscribers registered and the **first one +/// raising**, the second still runs and the error is reported rather +/// than swallowed. +/// +/// Bite: fails against a `short-circuit` registration, where the first +/// subscriber's return would stop the fan-out and silently prevent every +/// later one from reconciling — which no test asserting only "the hook +/// fired" would catch. +#[test] +fn acc55_a_raising_subscriber_does_not_stop_the_fan_out() { + let fx = Fixture::new(); + let old = fx.write("fanout.txt", "x\n"); + let new = fx.at("fanout-moved.txt"); + let doomed = fx.write("fanout-doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.SECOND_RAN = 0 + _G.SECOND_DELETED = 0 + pmacs.hook.add('resource.renamed', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.renamed', function() _G.SECOND_RAN = _G.SECOND_RAN + 1 end) + pmacs.hook.add('resource.deleted', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.deleted', function() _G.SECOND_DELETED = _G.SECOND_DELETED + 1 end)", + ); + + rename_fire_and_forget(&mut state, &old, &new); + let ran: i64 = eval(&state, "return _G.SECOND_RAN"); + assert_eq!( + ran, 1, + "`all-must-succeed` collects the first callback's error and \ + continues; a short-circuit registration would have stopped here" + ); + + remove_fire_and_forget(&mut state, &doomed); + let deleted: i64 = eval(&state, "return _G.SECOND_DELETED"); + assert_eq!(deleted, 1, "same for `resource.deleted`"); + + // The error is reported, not swallowed: the hook error log is the + // `*errors*` buffer. + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("first subscriber explodes"), + "the raising subscriber's error must be reported; *errors* held: \ + {errors:?}" + ); +} + +// --------------------------------------------------------------------------- +// 23, 24 — the delete lookups +// --------------------------------------------------------------------------- + +/// Acceptance 23. `apply_resource_op`'s delete reaches **descendants** +/// and a **second buffer on the same path** — the raw first-match lookup +/// replaced by the shared prefix-aware, normalizing query. +/// +/// (#190 owns the modified-buffer refusal; it refuses before disk, so by +/// the time this lane's reconciliation runs there is no modified buffer +/// on the synchronous path to spare. This row asserts the lookup fix.) +#[test] +fn acc23_the_synchronous_delete_reaches_descendants_and_duplicates() { + let fx = Fixture::new(); + fx.dir("tree/nested"); + let one = fx.write("tree/one.txt", "1\n"); + fx.write("tree/nested/two.txt", "2\n"); + let state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &fx.at("tree/nested/two.txt")); + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + // Keep an unrelated buffer alive so the last-buffer refusal is not + // what this row measures. + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("./tree")) + ), + ); + + assert!(!fx.at("tree").exists(), "the tree is gone from disk"); + assert!( + !buffer_is_valid(&state, "ONE"), + "a buffer directly under the deleted directory" + ); + assert!( + !buffer_is_valid(&state, "TWO"), + "a more deeply nested descendant" + ); + assert!( + !buffer_is_valid(&state, "DUP"), + "the second buffer on the same path — the one a first-match \ + lookup cannot see, which #190 deliberately left in place \ + because it had no two-phase kill to route it through" + ); + assert!(buffer_is_valid(&state, "KEEP"), "an unrelated buffer"); +} + +/// Acceptance 24. A **fire-and-forget** `pmacs.fs.remove` reconciles +/// too: never taking the handle still kills the unmodified buffer, which +/// is what makes the drain harvest the right seam rather than dired +/// firing the hook itself. +#[test] +fn acc24_a_never_awaited_remove_still_kills_the_unmodified_buffer() { + let fx = Fixture::new(); + let doomed = fx.write("gone.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + assert!(buffer_is_valid(&state, "B")); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + !buffer_is_valid(&state, "B"), + "the harvest must reconcile a delete no one awaited" + ); + assert!(buffer_is_valid(&state, "KEEP")); +} + +/// Acceptance 18's substrate half, and §6's modified case: a **modified** +/// buffer whose file is deleted out from under it keeps its contents. The +/// buffer half is the part of the promise that is robust, because it runs +/// at drain time against whatever state exists then. +#[test] +fn a_modified_buffer_survives_a_delete_with_its_contents() { + let fx = Fixture::new(); + let doomed = fx.write("dirty.txt", "original\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "_G.B:insert(0, 'edited ')"); + let modified: bool = eval(&state, "return _G.B:is_modified()"); + assert!(modified, "precondition"); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + buffer_is_valid(&state, "B"), + "a modified buffer is kept alive deliberately, not killed" + ); + let contents: String = eval(&state, "return _G.B:slice(0, _G.B:len())"); + assert_eq!(contents, "edited original\n", "with its contents intact"); + assert!(!doomed.exists(), "while the file is gone"); +} + +// --------------------------------------------------------------------------- +// 51, 52, 53 — the removal lifecycle +// --------------------------------------------------------------------------- + +/// Acceptance 51. A killed buffer completes **both** removal phases: +/// after a delete reconciles, an `on_removed` callback registered for +/// that buffer has fired and its buffer-local keymap entries are gone. +/// +/// Bite: fails against an implementation that calls only +/// `EditorCore::kill_buffer`, which does no phase-2 cleanup at all. +/// +/// Note 51 and 52 are a matched pair and **neither alone is +/// sufficient** — each pre-existing removal path passes one and fails +/// the other, which is exactly why both phases had to be named. +#[test] +fn acc51_a_killed_buffer_completes_both_removal_phases() { + let fx = Fixture::new(); + let doomed = fx.write("phase2.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec( + &state, + "_G.ON_REMOVED = 0 + pmacs.buffer.on_removed(_G.B, function() _G.ON_REMOVED = _G.ON_REMOVED + 1 end) + pmacs.command.define { name = 'test.noop', description = 'x', fn = function() end } + pmacs.keymap.bind { scope = 'buffer', buffer = _G.B, + sequence = 'C-c C-1', command = 'test.noop' } + -- `pmacs.keymap.lookup` is deliberately raw-global, so the + -- buffer-scoped row is only visible through `list()`. + function _G.BOUND_ROWS() + local n = 0 + for _, e in ipairs(pmacs.keymap.list()) do + if e.command == 'test.noop' then n = n + 1 end + end + return n + end", + ); + let bound_before: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_before, 1, + "precondition: the buffer-local binding exists" + ); + + remove_fire_and_forget(&mut state, &doomed); + + assert!(!buffer_is_valid(&state, "B"), "phase 1 removed the buffer"); + let fired: i64 = eval(&state, "return _G.ON_REMOVED"); + assert_eq!( + fired, 1, + "phase 2 must fire the registered on_removed callback; 0 means the \ + reconciliation called `EditorCore::kill_buffer` alone" + ); + let bound_after: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_after, 0, + "phase 2 must purge the buffer-scoped keymap, so a later buffer \ + cannot inherit a dead one's bindings" + ); +} + +/// Acceptance 52. A window displaying the deleted buffer is +/// **redirected**, not left dangling: no window holds a removed id. +/// +/// Bite: fails against `remove_buffer_and_fire`, which is what +/// `apply_resource_op` used — phase 2 without phase 1, so +/// `BufferRegistry::remove` runs while every window showing the buffer +/// keeps pointing at the id it just dropped. +#[test] +fn acc52_a_window_showing_the_deleted_buffer_is_redirected() { + let fx = Fixture::new(); + let doomed = fx.write("shown.txt", "x\n"); + let state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let doomed_id = state.core.borrow().active_buffer_id(); + assert!( + state + .core + .borrow() + .windows + .values() + .any(|w| w.buffer_id == doomed_id), + "precondition: a window shows the doomed buffer" + ); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", path = \"{}\" }}", + lua_str(&doomed) + ), + ); + + let core = state.core.borrow(); + assert!( + !core.registry.borrow().contains(doomed_id), + "the buffer was removed" + ); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "no window may hold a removed buffer id; dangling: {dangling:?}" + ); + assert!( + !core.windows.values().any(|w| w.buffer_id == doomed_id), + "and specifically not the deleted one" + ); +} + +/// Acceptance 53. The last-buffer and mid-edit refusals are +/// **reported, not silent**, and neither aborts the reconciliation of +/// other buffers. +#[test] +fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { + // Half one: the file behind the only open buffer. `kill_buffer` + // refuses to remove the last remaining buffer, so the file goes and + // the buffer stays. + let fx = Fixture::new(); + let only = fx.write("only.txt", "x\n"); + let mut state = editor(); + // Drop every other buffer so the target really is the last one. + exec( + &state, + &format!( + "_G.ONLY = pmacs.buffer.find_or_open(\"{}\") + for _, b in ipairs(pmacs.buffer.list()) do + if tostring(b) ~= tostring(_G.ONLY) then + pcall(pmacs.buffer.kill, b) + end + end + return #pmacs.buffer.list()", + lua_str(&only) + ), + ); + let count: i64 = eval(&state, "return #pmacs.buffer.list()"); + assert_eq!(count, 1, "precondition: exactly one buffer is open"); + + remove_fire_and_forget(&mut state, &only); + assert!( + buffer_is_valid(&state, "ONLY"), + "the last remaining buffer cannot be killed, so it survives the \ + deletion of its file" + ); + + // Half two: a directory of buffers where one refuses removal. The + // rest must still reconcile. + let fx2 = Fixture::new(); + fx2.dir("batch"); + let a = fx2.write("batch/a.txt", "a\n"); + let b = fx2.write("batch/b.txt", "b\n"); + let c = fx2.write("batch/c.txt", "c\n"); + let mut state2 = editor(); + open_as(&state2, "KEEP", &fx2.write("keep.txt", "k\n")); + open_as(&state2, "A", &a); + open_as(&state2, "B", &b); + open_as(&state2, "C", &c); + // B refuses: it is modified. + exec(&state2, "_G.B:insert(0, 'dirty ')"); + + remove_fire_and_forget(&mut state2, &fx2.at("batch/a.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/b.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/c.txt")); + + assert!(!buffer_is_valid(&state2, "A"), "A reconciled"); + assert!( + buffer_is_valid(&state2, "B"), + "B was kept because it is modified" + ); + assert!( + !buffer_is_valid(&state2, "C"), + "and C still reconciled afterwards — one refusal must not abort \ + the rest" + ); +} + +// --------------------------------------------------------------------------- +// 53b — a mid-edit refusal leaves editor state UNCHANGED +// --------------------------------------------------------------------------- + +/// Acceptance 53b, all three assertions, **stated individually**. +/// +/// With the buffer `editing_in_progress`, displayed in an ordinary +/// window, shown in a side window, and present in `round_trip_buffers`, +/// a delete reconciling it must leave each of the following provably +/// untouched. Each fails independently against the same one-line bite — +/// removing the `editing_in_progress` preflight — which is the point: a +/// single compound assertion can pass on two of the three and hide the +/// third. +/// +/// | # | Assertion | What the missing preflight breaks | +/// |---|---|---| +/// | i | the ordinary window still shows the buffer, cursor/selection/`view_top` intact | `kill_buffer` redirects the window to the fallback before `BufferRegistry::remove` refuses | +/// | ii | the side window is still open and still shows the buffer | `remove_side_window` collapses it first | +/// | iii | the buffer is still in `round_trip_buffers` | `round_trip_buffers.remove` runs first — the **first** thing `kill_buffer` does, and the easiest to miss | +#[test] +#[allow( + clippy::too_many_lines, + reason = "three independent assertions, each with its own precondition; a compound check is exactly what this row exists to avoid" +)] +fn acc53b_a_mid_edit_refusal_leaves_window_side_and_round_trip_state_untouched() { + let fx = Fixture::new(); + let doomed = fx.write("midedit.txt", "0123456789\nsecond line\n"); + let mut state = editor(); + // A grid frontend's real frame size is its declaration, and a side + // window needs one before it can be placed. + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + exec(&state, "pmacs.buffer.set_round_trip_input(_G.B, true)"); + + let doomed_id = state.core.borrow().active_buffer_id(); + let ordinary_window = state.core.borrow().active_window_id(); + // Seat a distinctive cursor + selection + scroll position on the + // ORDINARY window, so a redirect is detectable as more than "the + // window moved". + { + let mut core = state.core.borrow_mut(); + let win = core + .windows + .get_mut(&ordinary_window) + .expect("ordinary window"); + win.cursor = 4; + win.selection = Some(pmacs::window::Selection { anchor: 2 }); + win.view_top = 1; + } + + // A SIDE window showing the same buffer, so the collapse the + // preflight prevents has something to collapse. + exec( + &state, + "pmacs.window.display(_G.B, { side = \"bottom\", height = 4 })", + ); + let side_windows: Vec<_> = state + .core + .borrow() + .side_window_for(pmacs::protocol::FrontendId::LOCAL) + .into_iter() + .collect(); + assert!( + !side_windows.is_empty(), + "precondition: a side window exists" + ); + assert_eq!( + state + .core + .borrow() + .windows + .get(&side_windows[0]) + .expect("side window") + .buffer_id, + doomed_id, + "precondition: the side window shows the doomed buffer" + ); + assert_ne!( + side_windows[0], ordinary_window, + "precondition: the side window is a second window" + ); + assert!( + state.core.borrow().buffer_round_trips(doomed_id), + "precondition: the buffer round-trips input" + ); + + // Put the buffer mid-edit. `begin_edit` is the flag + // `BufferRegistry::remove` refuses on, and the whole point of the + // preflight is that the refusal arrives too late. + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(doomed_id) + .expect("doomed buffer") + .begin_edit() + .expect("begin edit"); + } + + remove_fire_and_forget(&mut state, &doomed); + + let core = state.core.borrow(); + // (i) the ordinary window, with its seated state. + let win = core + .windows + .get(&ordinary_window) + .expect("the ordinary window still exists"); + assert_eq!( + win.buffer_id, doomed_id, + "(i) the ordinary window must still show the buffer" + ); + assert_eq!(win.cursor, 4, "(i) cursor"); + assert_eq!( + win.selection, + Some(pmacs::window::Selection { anchor: 2 }), + "(i) selection" + ); + assert_eq!(win.view_top, 1, "(i) view_top"); + + // (ii) the side window. + for side in &side_windows { + let side_win = core.windows.get(side).unwrap_or_else(|| { + panic!( + "(ii) side window {side:?} was collapsed by a kill that should never have started" + ) + }); + assert_eq!( + side_win.buffer_id, doomed_id, + "(ii) the side window must still show the buffer" + ); + } + + // (iii) round-trip membership. + assert!( + core.buffer_round_trips(doomed_id), + "(iii) the buffer must still round-trip input — this is the FIRST \ + thing `kill_buffer` drops and the easiest to miss" + ); + + assert!( + core.registry.borrow().contains(doomed_id), + "and the buffer itself is still in the registry" + ); +} + +// --------------------------------------------------------------------------- +// 54 — independent mutations both reconcile, in either arrival order +// --------------------------------------------------------------------------- + +/// Acceptance 54, integration layer. Dispatch a rename and a delete on +/// **disjoint** paths, wait for both, and assert both registry effects +/// occurred. It fails against dropping or deduplicating one resource +/// kind. +/// +/// The disjoint end state is confidence coverage, **not** a claimed bite +/// against interdependent sequencing: disjoint paths necessarily +/// commute. The controlled-bus layer that does pin arrival order lives +/// in `src/async_runtime.rs`, and no test here pretends to pin an order +/// the mechanism does not establish. +#[test] +fn acc54_a_rename_and_a_delete_on_disjoint_paths_both_reconcile() { + for reverse in [false, true] { + let fx = Fixture::new(); + let renamed_from = fx.write("moves.txt", "m\n"); + let renamed_to = fx.at("moved.txt"); + let deleted = fx.write("goes.txt", "g\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "MOVES", &renamed_from); + open_as(&state, "GOES", &deleted); + + let rename = format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&renamed_from), + lua_str(&renamed_to) + ); + let remove = format!("pmacs.fs.remove(\"{}\")", lua_str(&deleted)); + if reverse { + exec(&state, &remove); + exec(&state, &rename); + } else { + exec(&state, &rename); + exec(&state, &remove); + } + + pump_until(&mut state, "both mutations", |s| { + renamed_to.exists() && !deleted.exists() && !buffer_is_valid(s, "GOES") + }); + pump_a_while(&mut state); + + assert_eq!( + buffer_path(&state, "MOVES").as_deref(), + Some(renamed_to.to_str().unwrap()), + "the rename reconciled (dispatch order reversed: {reverse})" + ); + assert!( + !buffer_is_valid(&state, "GOES"), + "the delete reconciled (dispatch order reversed: {reverse})" + ); + } +} + +// --------------------------------------------------------------------------- +// The LSP-facing rows (30, 31c, 32, 34, 35) +// --------------------------------------------------------------------------- +// +// Driven against `pmacs_fake_lsp` so nothing here needs a real toolchain +// on PATH. The fake publishes two synthetic diagnostics (one Error at +// line 0, one Warning at line 2) on every `didOpen`, which is what makes +// "the new URI's diagnostics" observable at all. + +fn fake_lsp_path() -> String { + env!("CARGO_BIN_EXE_pmacs_fake_lsp").to_owned() +} + +/// Configure `language` to spawn the fake, and pin the project-marker +/// walk to `root` so a stray `.git` above the tempdir cannot silently +/// turn a markerless fixture into a detected one. +fn configure_fake(state: &EditorState, root: &Path, language: &str) { + exec( + state, + &format!( + "pmacs.project.set_search_boundary(\"{}\") + pmacs.lsp.config.{language} = {{ command = \"{}\" }}", + lua_str(root), + fake_lsp_path() + ), + ); +} + +/// Pump the real frame order — processes, LSP, async — until `predicate` +/// holds. All three are needed: the fake's frames arrive through the +/// supervisor, the manager parses them, and the rename settles on the +/// async bus. +fn settle_until bool>( + state: &mut EditorState, + what: &str, + predicate: F, +) { + let deadline = Instant::now() + Duration::from_secs(20); + while !predicate(state) { + assert!( + Instant::now() < deadline, + "settle deadline exceeded: {what}" + ); + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(5)); + } +} + +fn settle_a_while(state: &mut EditorState) { + for _ in 0..120 { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(3)); + } +} + +fn server_count(state: &EditorState) -> i64 { + eval(state, "return #pmacs.lsp.list()") +} + +/// `language|root_uri|state` per live server, sorted, so assertions do +/// not depend on spawn order. +fn server_rows(state: &EditorState) -> Vec { + let joined: String = eval( + state, + r#" + local out = {} + for _, s in ipairs(pmacs.lsp.list()) do + out[#out + 1] = table.concat({ + s.language_id or "", s.root_uri or "", + (s.state and s.state.kind) or "", + }, "|") + end + table.sort(out) + return table.concat(out, "\n") + "#, + ); + if joined.is_empty() { + Vec::new() + } else { + joined.lines().map(str::to_owned).collect() + } +} + +fn diag_count(state: &EditorState, uri: &str) -> i64 { + eval( + state, + &format!( + "local e, w, i, h = pmacs.diag.count(\"{uri}\") + return (e or 0) + (w or 0) + (i or 0) + (h or 0)" + ), + ) +} + +/// Mirror of `file_uri_for` in `builtin/runtime/lsp.lua`. Reimplemented +/// rather than imported, so the test states the expected encoding +/// independently of the code under test. +fn file_uri(path: &Path) -> String { + let mut out = String::from("file://"); + for byte in path.display().to_string().as_bytes() { + match byte { + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'/' | b'-' | b'_' | b'.' | b'~' | b':' => { + out.push(*byte as char); + } + _ => { + use std::fmt::Write as _; + let _ = write!(out, "%{byte:02X}"); + } + } + } + out +} + +/// Every window's overlay kinds, in composition order, keyed by window. +fn overlay_kinds_per_window(state: &EditorState) -> Vec<(u64, Vec<&'static str>)> { + let core = state.core.borrow(); + let mut rows: Vec<(u64, Vec<&'static str>)> = core + .windows + .iter() + .map(|(id, w)| (id.raw(), w.overlay_kinds())) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Count cells carrying a diagnostic **error** underline colour, per +/// window rect, by painting one real frame. +/// +/// This is the only per-window, view-level observation available: +/// `DiagnosticView.uri` is private and `View` has no downcast, so +/// asserting on the store would prove nothing about whether the overlay +/// was re-rooted. A view still pointing at the old URI renders nothing, +/// because `forget_uri` emptied that key. +fn error_underlines_per_window(state: &EditorState) -> Vec<(u64, usize)> { + use pmacs::cell::{Cell, CellGrid, CellSize, Color}; + use pmacs::protocol::FrontendId; + use pmacs::window::Rect; + + let size = CellSize::new(24, 80); + let mut cells = vec![Cell::default(); (size.rows * size.cols) as usize]; + let mut grid = CellGrid { + cells: &mut cells, + stride: size.cols, + size, + }; + pmacs::editor::paint_frame( + state, + FrontendId::LOCAL, + &std::collections::HashMap::new(), + &mut grid, + size, + ); + let placements = { + let core = state.core.borrow(); + let view = core.views.get(&FrontendId::LOCAL).expect("LOCAL view"); + let area = Rect::new(0, 0, size.rows - 1, size.cols); + let fixed = core.panel_fixed_rows(FrontendId::LOCAL, area.size.rows); + view.layout.compute(area, &fixed) + }; + let error = Color::Indexed(1); + let mut rows: Vec<(u64, usize)> = placements + .into_iter() + .map(|(win, rect)| { + let mut n = 0; + for row in rect.origin.row..rect.origin.row + rect.size.rows { + for col in rect.origin.col..rect.origin.col + rect.size.cols { + let idx = (row * size.cols + col) as usize; + if cells.get(idx).map(|c| c.style.underline_color) == Some(error) { + n += 1; + } + } + } + (win.raw(), n) + }) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Acceptance 30. An attached LSP buffer with **diagnostics present +/// before** the rename, shown in **at least two windows**: afterwards +/// both windows render the **new** URI's diagnostics, the old URI's +/// store is empty, and each window's overlay keeps its **position in +/// the composition order**. +/// +/// Bite, two mutations: `rec.uri` updated without re-rooting the +/// diagnostic view (both windows then render nothing, because the old +/// key is empty); and a remove-and-re-push, which would pass a +/// one-window render test while moving the diagnostic overlay to the end +/// of the stack — caught by the composition-order assertion. +#[test] +fn acc30_diagnostics_re_root_in_every_window_and_keep_their_stack_position() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write( + "proj/src/main.rs", + "fn main() {}\n// second\n// third line here\n", + ); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &old); + + let old_uri = file_uri(&old); + let new_uri = file_uri(&new); + settle_until(&mut state, "diagnostics for the old URI", |s| { + diag_count(s, &old_uri) > 0 + }); + + // A second window showing the same buffer, with its own + // `DiagnosticView`. A split alone does not carry one — the view + // does not implement `clone_for_split` — so the switch hook is what + // attaches it, and that path only ever touches the ACTIVE window. + exec(&state, "pmacs.window.split_horizontal()"); + // `try_split_active` leaves focus where it was, so the switch hook — + // which can only reach the ACTIVE window — has to be given the new + // one explicitly. + exec(&state, "pmacs.window.focus_next()"); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + settle_a_while(&mut state); + + let before_kinds = overlay_kinds_per_window(&state); + let before_paint = error_underlines_per_window(&state); + assert_eq!( + before_paint.len(), + 2, + "precondition: two windows are placed; got {before_paint:?}" + ); + for (win, n) in &before_paint { + assert!( + *n > 0, + "precondition: window {win} must already paint diagnostic \ + underlines; got {before_paint:?} with overlays \ + {before_kinds:?}" + ); + } + let diag_positions_before: Vec<(u64, Option)> = before_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert!( + diag_positions_before.iter().all(|(_, p)| p.is_some()), + "precondition: every window carries a diagnostic overlay; got \ + {before_kinds:?}" + ); + + rename_fire_and_forget(&mut state, &old, &new); + settle_until(&mut state, "diagnostics for the new URI", |s| { + diag_count(s, &new_uri) > 0 + }); + settle_a_while(&mut state); + + assert_eq!( + diag_count(&state, &old_uri), + 0, + "the old URI's store must be empty" + ); + assert!( + diag_count(&state, &new_uri) > 0, + "and the new URI's must be populated" + ); + + let after_kinds = overlay_kinds_per_window(&state); + let diag_positions_after: Vec<(u64, Option)> = after_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert_eq!( + diag_positions_after, diag_positions_before, + "each window's diagnostic overlay must keep its position in the \ + composition order; a remove-and-re-push would move it to the end \ + ({before_kinds:?} -> {after_kinds:?})" + ); + + let after_paint = error_underlines_per_window(&state); + assert_eq!(after_paint.len(), 2, "still two windows: {after_paint:?}"); + for (win, n) in &after_paint { + assert!( + *n > 0, + "window {win} must render the NEW URI's diagnostics; 0 means \ + its overlay is still keyed under the old URI, whose store the \ + forget emptied ({after_paint:?})" + ); + } +} + +/// Acceptance 31c, at the Lua binding. Raises for an unknown server id; +/// **succeeds** for a URI with no state under a known server. +/// +/// The second arm is the one that matters: the `resource.renamed` +/// subscriber calls this per attachment, and an attachment need not have +/// any pending route or populated result store. An over-strict binding +/// would turn that ordinary idempotent case into an error inside a hook. +#[test] +fn acc31c_the_forget_uri_binding_raises_for_an_unknown_server_and_not_for_an_unknown_uri() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let file = fx.write("proj/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &file); + settle_until(&mut state, "one live server", |s| server_count(s) == 1); + + // An unknown id has to be a real handle to a server the manager no + // longer holds: `LspServerIdLua` is opaque and cannot be forged from + // an integer, which is itself the binding's first line of defence. + let raised: String = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + assert(sid, 'no server to stale out') + _G.STALE = sid + pmacs.lsp.stop(sid) + return 'stopped'", + ); + assert_eq!(raised, "stopped"); + settle_until(&mut state, "the server is forgotten", |s| { + let gone: bool = eval( + s, + "local ok = pcall(pmacs.lsp.forget, _G.STALE) + return #pmacs.lsp.list() == 0", + ); + gone + }); + let raised: String = eval( + &state, + "local ok, err = pcall(pmacs.lsp.forget_uri, _G.STALE, 'file:///nope.rs') + if ok then return 'DID NOT RAISE' end + return tostring(err)", + ); + assert!( + raised.contains("unknown server"), + "an unknown server id must raise, matching `pmacs.lsp.forget`; got \ + {raised:?}" + ); + + // And the success arm, against a live server. + open_as( + &state, + "C", + &fx.write("proj/src/second.rs", "fn second() {}\n"), + ); + settle_until(&mut state, "a live server again", |s| server_count(s) == 1); + let ok: bool = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + local a = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + local b = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + return a and b", + ); + assert!( + ok, + "a URI with no state under a known server is an idempotent \ + success, and repeating it stays safe" + ); +} + +/// Acceptance 32. A rename **across project roots** re-runs +/// `ensure_server` and the buffer ends up attached to a **different** +/// server; a same-root rename reuses the existing one (#161's affinity +/// key is the detected project root). +#[test] +fn acc32_a_cross_root_rename_re_runs_ensure_server_and_a_same_root_one_reuses() { + // Same root first: renaming within one package must not spawn a + // second server. + let fx = Fixture::new(); + fx.write("a/Cargo.toml", "[package]\nname = \"a\"\n"); + let inside = fx.write("a/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &inside); + settle_until(&mut state, "one initialized server", |s| { + server_rows(s) == vec![format!("rust|{}|initialized", file_uri(&fx.at("a")))] + }); + let root_a = file_uri(&fx.at("a")); + assert_eq!( + server_rows(&state), + vec![format!("rust|{root_a}|initialized")], + "precondition: one server, rooted at package a" + ); + + rename_fire_and_forget(&mut state, &inside, &fx.at("a/src/moved.rs")); + settle_a_while(&mut state); + assert_eq!( + server_count(&state), + 1, + "a same-root rename reuses the existing server: {:?}", + server_rows(&state) + ); + + // Now across roots: `b` is its own package, so its file needs its + // own server. + fx.write("b/Cargo.toml", "[package]\nname = \"b\"\n"); + std::fs::create_dir_all(fx.at("b/src")).unwrap(); + rename_fire_and_forget( + &mut state, + &fx.at("a/src/moved.rs"), + &fx.at("b/src/moved.rs"), + ); + settle_until(&mut state, "a second server for package b", |s| { + server_count(s) == 2 + }); + settle_a_while(&mut state); + + let root_b = file_uri(&fx.at("b")); + let rows = server_rows(&state); + assert!( + rows.iter().any(|r| r.contains(&root_b)), + "the cross-root rename must spawn a server rooted at package b; \ + rows were {rows:?}" + ); + assert_eq!( + rows.len(), + 2, + "exactly one new server, not one per reconciliation pass: {rows:?}" + ); +} + +/// Point the `rust` server at the `applyeditplan` fake carrying `plan`, +/// and hand back the sink the client's response to the server-initiated +/// `workspace/applyEdit` lands in. Must run before the first `.rs` file +/// is opened — that open is what launches the server. +fn plan_server(state: &EditorState, dir: &Path, plan: &serde_json::Value) -> PathBuf { + let plan_path = dir.join("plan.json"); + std::fs::write(&plan_path, serde_json::to_vec(plan).unwrap()).unwrap(); + let sink = dir.join("applyedit-response.json"); + exec( + state, + &format!( + "pmacs.lsp.config.rust = {{ + command = \"{}\", + env = {{ + PMACS_FAKE_LSP_MODE = 'applyeditplan', + PMACS_FAKE_LSP_EDIT_PLAN = '{}', + PMACS_FAKE_LSP_APPLYEDIT_SINK = '{}', + }}, + }}", + fake_lsp_path(), + plan_path.display(), + sink.display() + ), + ); + sink +} + +/// Ask the fake to deliver its planned `workspace/applyEdit`. Driven by +/// an `executeCommand` rather than fired at `initialized`, so the test +/// controls *when* the batch arrives — these fixtures depend on a +/// specific buffer being active first, and a server-timed request would +/// race that setup. +fn trigger_apply_edit(state: &EditorState) { + exec( + state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do + if row.state and row.state.kind == 'initialized' then sid = row.id end + end + assert(sid, 'no initialized server') + pmacs.lsp.request_execute_command(sid, 'pmacs.fake.applyEdit', {})", + ); +} + +fn wait_for_apply_response(state: &mut EditorState, sink: &Path) -> serde_json::Value { + let deadline = Instant::now() + Duration::from_secs(20); + loop { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if let Ok(raw) = std::fs::read(sink) + && let Ok(v) = serde_json::from_slice::(&raw) + { + assert!( + v.get("fakeError").is_none(), + "the fixture itself failed: {v:?}" + ); + return v; + } + assert!( + Instant::now() < deadline, + "the client never answered the server's workspace/applyEdit" + ); + std::thread::sleep(Duration::from_millis(10)); + } +} + +/// Acceptance 34. Renaming the **active** file through the full +/// `apply_workspace_edit` path leaves **no phantom empty buffer** at the +/// obsolete path, and the user is returned to the **same buffer** (now +/// under its new path). +/// +/// Bite: the applier restoring by path instead of by buffer handle. A +/// captured path no longer resolves after its own batch renamed it, so +/// `find_or_open` reaches `resolve_target_buffer`'s `NotFound` arm, +/// which *creates* an empty path-backed buffer and selects it. No +/// reconciliation can reach the string a Lua local already captured. +#[test] +fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write("proj/src/main.rs", "fn main() {}\n"); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [{ + "kind": "rename", + "oldUri": file_uri(&old), + "newUri": file_uri(&new), + }], + }), + ); + open_as(&state, "B", &old); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + // The applier restores the buffer that was active when the batch + // began, so make that the file being renamed. + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let active_before = state.core.borrow().active_buffer_id(); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!(new.exists(), "the rename landed on disk"); + assert!(!old.exists()); + assert_eq!( + state.core.borrow().active_buffer_id(), + active_before, + "the user must be returned to the SAME buffer, now under its new \ + path — not to a freshly created one" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "and that buffer's path followed the rename" + ); + + let phantom: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&old) + ), + ); + assert!( + !phantom, + "no buffer may remain bound to the obsolete path — that buffer is \ + the phantom the old path fallback materialized" + ); +} + +/// Acceptance 35. When the origin buffer is **gone** after the edit, the +/// applier restores **nothing** rather than falling back to the old +/// path. +#[test] +fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let doomed = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [{ + "kind": "delete", + "uri": file_uri(&doomed), + }], + }), + ); + // `other` keeps the registry non-empty so the delete's kill is not + // refused for being the last buffer. + open_as(&state, "OTHER", &other); + open_as(&state, "B", &doomed); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!(!doomed.exists(), "the file is gone"); + assert!( + !buffer_is_valid(&state, "B"), + "and its clean buffer was reconciled away" + ); + let phantom: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&doomed) + ), + ); + assert!( + !phantom, + "the applier must restore NOTHING rather than re-opening the path \ + it just deleted — a path fallback would recreate it as an empty \ + buffer, and the next C-x C-s would resurrect the file" + ); + let active_valid = { + let core = state.core.borrow(); + let id = core.active_buffer_id(); + core.registry.borrow().contains(id) + }; + assert!( + active_valid, + "and the window it left behind must sit on a live buffer" + ); +} From edfb52f29cf749378e73a292759eb77afd7c3e4d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:51:41 -0400 Subject: [PATCH 06/12] test(stage2a): make three bites bite, and correct one framing claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the bites found three that did not falsify anything. Item 28's rename row cannot pin the walk's containment rule: `reconcile_rename` calls `Path::strip_prefix` to rebuild a descendant's tail, and that is component-aware too, so a string-prefix walk is silently corrected a second time. Deletion has no such second guard — the walk's verdict IS the kill list — so the row moves there, and a string prefix now provably destroys a buffer on `foobar.txt` when `foo/` is deleted. Item 30's composition-order assertion was a tautology: the LSP attach leaves `diagnostic` LAST in the stack, and moving the last element to the end is a no-op, so a remove-and-re-push was indistinguishable from an in-place mutation. The row now pushes one more overlay after it and asserts that precondition explicitly. Item 34 needed both a restructure and a correction. §5's G1 says a stale captured path "materializes a phantom" via `resolve_target_buffer`'s `NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls `file_io::load_file` directly and maps the error, so a missing path RAISES, and the `NotFound` arm belongs to `resolve_target_buffer`, which serves `pmacs.window.display_file` and the startup target rather than this binding. The real defect is smaller and still real — the `pcall` swallows the raise and the user is stranded wherever the last applied op left them — so the plan now edits another file first, which is what makes the restore observable at all. The correction is recorded at the test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/editor_core.rs | 4 +- src/lua_bindings/diag.rs | 3 +- tests/resource_reconciliation_acceptance.rs | 144 +++++++++++++++++--- 3 files changed, 130 insertions(+), 21 deletions(-) diff --git a/src/editor_core.rs b/src/editor_core.rs index 732e19a..f902418 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -5317,7 +5317,9 @@ pub fn buffers_bound_under( let mut out = Vec::new(); for id in reg.ids() { let Ok(buf) = reg.get(*id) else { continue }; - let Some(bound) = buf.file_path() else { continue }; + let Some(bound) = buf.file_path() else { + continue; + }; let bound = normalize_buffer_path(bound.to_path_buf()); if bound == target || (include_descendants && bound.starts_with(&target)) { out.push((*id, bound)); diff --git a/src/lua_bindings/diag.rs b/src/lua_bindings/diag.rs index 338dfc3..b9e632a 100644 --- a/src/lua_bindings/diag.rs +++ b/src/lua_bindings/diag.rs @@ -251,7 +251,8 @@ pub fn install_diag( let Some(core) = lua.app_data_ref::() else { return Ok(false); }; - core.borrow_mut().rename_resource_in_views(&old_uri, &new_uri); + core.borrow_mut() + .rename_resource_in_views(&old_uri, &new_uri); Ok(true) })?, )?; diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs index 90b4e34..716ce76 100644 --- a/tests/resource_reconciliation_acceptance.rs +++ b/tests/resource_reconciliation_acceptance.rs @@ -312,6 +312,55 @@ fn acc28_a_false_string_prefix_is_not_a_path_prefix() { ); } +/// Acceptance 28, delete side — **and this is the row that bites.** +/// +/// The rename row above cannot falsify a string-prefix walk on its own: +/// `reconcile_rename` calls `Path::strip_prefix` to rebuild the +/// descendant's tail, and that is component-aware too, so a false +/// prefix match is silently dropped a second time and the buffer stays +/// put. Deletion has no such second guard — the walk's verdict IS the +/// kill list — so the containment rule has to be pinned here. +/// +/// Bite: a string `starts_with` instead of `Path::starts_with` kills a +/// buffer on `foobar.txt` when `foo/` is deleted. +#[test] +fn acc28_delete_a_false_string_prefix_does_not_widen_the_kill_list() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("foo")) + ), + ); + + assert!(!fx.at("foo").exists(), "the directory is gone"); + assert!( + !buffer_is_valid(&state, "IN"), + "the real descendant is reconciled away" + ); + assert!( + buffer_is_valid(&state, "OUT"), + "`foobar.txt` shares a string prefix with `foo` and is not under \ + it — killing it destroys an unrelated buffer whose file still \ + exists" + ); + assert!( + sibling.exists(), + "and that file is indeed still on disk, which is what makes the \ + kill wrong rather than merely early" + ); +} + // --------------------------------------------------------------------------- // 29 — name provenance, both directions // --------------------------------------------------------------------------- @@ -1320,7 +1369,32 @@ fn acc30_diagnostics_re_root_in_every_window_and_keep_their_stack_position() { exec(&state, "pmacs.window.switch_buffer(_G.B)"); settle_a_while(&mut state); + // Push one more overlay AFTER the diagnostic in each window. + // Without this the composition-order assertion below cannot bite: + // the LSP attach leaves `diagnostic` LAST in the stack, and moving + // the last element to the end is a no-op, so a remove-and-re-push + // would be indistinguishable from an in-place mutation. + // `_attach_highlight` uses `push_overlay` (no dedup), so a second + // call appends. + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + exec(&state, "pmacs.window.focus_next()"); + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + let before_kinds = overlay_kinds_per_window(&state); + assert!( + before_kinds + .iter() + .all(|(_, kinds)| kinds.iter().position(|k| *k == "diagnostic") + < Some(kinds.len() - 1)), + "precondition: the diagnostic overlay must NOT be last, or \ + \"keeps its stack position\" is unfalsifiable; got {before_kinds:?}" + ); let before_paint = error_underlines_per_window(&state); assert_eq!( before_paint.len(), @@ -1586,20 +1660,40 @@ fn wait_for_apply_response(state: &mut EditorState, sink: &Path) -> serde_json:: } /// Acceptance 34. Renaming the **active** file through the full -/// `apply_workspace_edit` path leaves **no phantom empty buffer** at the -/// obsolete path, and the user is returned to the **same buffer** (now -/// under its new path). +/// `apply_workspace_edit` path returns the user to the **same buffer** +/// (now under its new path), and leaves no buffer bound to the obsolete +/// path. +/// +/// The plan deliberately edits *another* file first. Without that the +/// row cannot bite at all: the applier only has to restore the origin if +/// something moved the active buffer away, and a lone rename op does not. /// /// Bite: the applier restoring by path instead of by buffer handle. A /// captured path no longer resolves after its own batch renamed it, so -/// `find_or_open` reaches `resolve_target_buffer`'s `NotFound` arm, -/// which *creates* an empty path-backed buffer and selects it. No -/// reconciliation can reach the string a Lua local already captured. +/// `find_or_open` raises, the `pcall` swallows it, and the user is +/// stranded in whatever buffer the last applied op left active. No +/// reconciliation can reach a string a Lua local already captured. +/// +/// **One framing claim corrected here.** §5's G1 says the stale path +/// "materializes a phantom": that `find_or_open(origin)` reaches +/// `resolve_target_buffer`'s `NotFound` arm, which creates an empty +/// path-backed buffer and selects it. It does not. +/// `pmacs.buffer.find_or_open` (`src/lua_bindings/mod.rs`) calls +/// `crate::file_io::load_file` directly and maps the error, so a missing +/// path **raises**; the `NotFound` arm belongs to +/// `EditorCore::resolve_target_buffer`, which serves +/// `pmacs.window.display_file` and the startup/daemon target, not this +/// binding. The defect is real but smaller than G1 states — a silently +/// swallowed restore, not a fabricated file — and this row asserts the +/// half that is true. The no-buffer-at-the-old-path assertion is kept as +/// a cheap guard against a future fallback that *would* create one, and +/// is not the biting half. #[test] -fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { +fn acc34_renaming_the_active_file_through_the_applier_returns_the_same_buffer() { let fx = Fixture::new(); fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); let old = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); let new = fx.at("proj/src/renamed.rs"); let mut state = editor(); configure_fake(&state, &fx.root, "rust"); @@ -1607,11 +1701,25 @@ fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { &state, &fx.root, &serde_json::json!({ - "documentChanges": [{ - "kind": "rename", - "oldUri": file_uri(&old), - "newUri": file_uri(&new), - }], + "documentChanges": [ + { + // Moves the active buffer away, so the restore has + // something to undo. + "textDocument": { "uri": file_uri(&other), "version": 1 }, + "edits": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "// touched\n", + }], + }, + { + "kind": "rename", + "oldUri": file_uri(&old), + "newUri": file_uri(&new), + }, + ], }), ); open_as(&state, "B", &old); @@ -1637,7 +1745,9 @@ fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { state.core.borrow().active_buffer_id(), active_before, "the user must be returned to the SAME buffer, now under its new \ - path — not to a freshly created one" + path — a path-based restore raises on the renamed-away path, the \ + pcall swallows it, and the user is left wherever the last applied \ + op put them" ); assert_eq!( buffer_path(&state, "B").as_deref(), @@ -1645,7 +1755,7 @@ fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { "and that buffer's path followed the rename" ); - let phantom: bool = eval( + let stale: bool = eval( &state, &format!( "for _, b in ipairs(pmacs.buffer.list()) do @@ -1655,11 +1765,7 @@ fn acc34_renaming_the_active_file_through_the_applier_leaves_no_phantom() { lua_str(&old) ), ); - assert!( - !phantom, - "no buffer may remain bound to the obsolete path — that buffer is \ - the phantom the old path fallback materialized" - ); + assert!(!stale, "no buffer may remain bound to the obsolete path"); } /// Acceptance 35. When the origin buffer is **gone** after the edit, the From 5e30e97f17e078b145300aacb9ec148f2beee565 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:54:22 -0400 Subject: [PATCH 07/12] style(stage2a): drop an unused mut the new delete row left behind Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- tests/resource_reconciliation_acceptance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs index 716ce76..f1ba6f9 100644 --- a/tests/resource_reconciliation_acceptance.rs +++ b/tests/resource_reconciliation_acceptance.rs @@ -329,7 +329,7 @@ fn acc28_delete_a_false_string_prefix_does_not_widen_the_kill_list() { fx.dir("foo"); let inside = fx.write("foo/a.txt", "in\n"); let sibling = fx.write("foobar.txt", "out\n"); - let mut state = editor(); + let state = editor(); open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); open_as(&state, "IN", &inside); open_as(&state, "OUT", &sibling); From f6af3f73351feaeaf848748afe8728951d8ab94b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 19:05:51 -0400 Subject: [PATCH 08/12] test(m4): re-pin two rows whose parked behaviour Stage 2a discharges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rd9` and `rd14` pinned #190's deliberate restraint on the `apply_resource_op` delete arm: descendants stay orphaned, and only the first of two duplicate path-bound buffers is reconciled. Both doc comments gave the same reason — widening would have routed N buffers through `remove_buffer_and_fire`, which is phase 2 without phase 1, so a tree delete would have left up to N windows on removed ids. `EditorCore::reconcile_delete` composes both phases, so that constraint is discharged and the old assertions are no longer merely obsolete: an orphaned buffer whose next `C-x C-s` recreates a file the user deleted is the defect. Each row now asserts the new contract in BOTH directions — the buffer is reconciled away, AND no window holds a removed id — so neither an exact-path/first-match regression nor a widening that skips phase 1 can pass. Each direction is bite-verified. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- tests/m4_acceptance.rs | 100 ++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 21 deletions(-) diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 220ae99..64323e0 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8549,17 +8549,26 @@ fn rd8_recursive_delete_refuses_for_a_modified_descendant() { assert!(tree.exists(), "including the directory itself"); } -/// Criterion 9 — a *clean* recursive delete leaves descendant buffers -/// orphaned, not removed. +/// Criterion 9 — a *clean* recursive delete reconciles descendant +/// buffers, through both removal phases. /// -/// This pin deliberately asserts today's imperfect behaviour. Widening -/// reconciliation to the tree would route N buffers through -/// `remove_buffer_and_fire` — phase 2 without phase 1 — promoting the -/// parked dangling-window defect from exact-path to tree-wide. +/// **Rewritten by dired Stage 2a** (`docs/dired-stage2-framing.md` §6, +/// Q#RD27 / acceptance 23). This row previously pinned the opposite — +/// that the descendant buffer stayed orphaned — and gave the reason: +/// widening reconciliation would have routed N buffers through +/// `remove_buffer_and_fire`, which is phase 2 *without* phase 1, so a +/// tree delete would have left up to N windows pointing at removed ids. +/// That constraint is discharged: `EditorCore::reconcile_delete` +/// composes the same two phases `pmacs.buffer.kill` composes, and the +/// delete arm routes through it. The old assertion is not merely +/// obsolete, it is now the defect — an orphaned buffer whose next +/// `C-x C-s` recreates a file the user deleted. /// -/// Bite: fails against an implementation that widens reconciliation. +/// Bite, both directions: fails against an exact-path reconciliation +/// (the descendant survives) **and** against a widening that skips +/// phase 1 (a window keeps a removed id). #[test] -fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { +fn rd9_clean_recursive_delete_reconciles_descendants_through_both_phases() { let dir = tempfile::tempdir().expect("tempdir"); let tree = dir.path().join("tree"); std::fs::create_dir(&tree).expect("mkdir"); @@ -8568,6 +8577,13 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { let mut state = pmacs::editor::EditorState::new(); rd_open(&mut state, "B", &inner); + // Display it, so the phase-1 window redirect has something to do. + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(B)") + .exec() + .expect("show the descendant"); let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); assert!(ok, "a clean tree deletes: {err}"); @@ -8580,9 +8596,23 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { .eval() .expect("validity probe"); assert!( - still, - "THE BITE: reconciliation stays exact-path, so the descendant \ - buffer is orphaned rather than removed" + !still, + "THE BITE: a buffer under a recursively deleted directory must be \ + reconciled away, not left bound to a path whose file is gone" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: widening the reconciliation must not promote the \ + dangling-window defect from exact-path to tree-wide; dangling: \ + {dangling:?}" ); } @@ -8633,15 +8663,22 @@ fn rd10_absent_plus_ignore_does_not_destroy_a_modified_buffer() { assert_eq!(text, "unsavedcontent\n", "the unsaved edit survives"); } -/// Criterion 14 — clean duplicates: exactly one match reconciled. +/// Criterion 14 — clean duplicates: **every** match reconciled. /// -/// Bite: fails against an implementation that removes **all** matches. -/// It pins the reconciliation half of Q#RD10 and *only* that: with both -/// buffers clean there is no verdict difference between consulting one -/// match and consulting all, so this setup cannot see validation -/// breadth. Criterion 6 is what detects incomplete validation. +/// **Rewritten by dired Stage 2a** (§6, acceptance 23). This row +/// previously pinned "exactly one", which was Q#RD10's deliberate +/// restraint: removing them all would have routed N buffers through +/// `remove_buffer_and_fire` — phase 2 without phase 1 — so the second +/// duplicate was left alive rather than have its window dangle. +/// `reconcile_delete` composes both phases, so the restraint is gone and +/// the surviving duplicate is now the defect: it is bound to a path +/// whose file no longer exists, and `find_by_path` cannot even see it. +/// +/// Bite: fails against a first-match implementation (one duplicate +/// survives) and against a widening that skips phase 1 (a window keeps +/// a removed id). #[test] -fn rd14_clean_duplicates_reconcile_exactly_one() { +fn rd14_clean_duplicates_all_reconcile() { let dir = tempfile::tempdir().expect("tempdir"); let f = dir.path().join("twin.rs"); std::fs::write(&f, b"twin\n").expect("write"); @@ -8658,6 +8695,13 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .exec() .expect("two clean buffers on one path"); + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(SECOND)") + .exec() + .expect("show the second duplicate"); + let (ok, err) = rd_delete(&mut state, &f, ""); assert!(ok, "two clean duplicates must not block: {err}"); @@ -8668,9 +8712,23 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .eval() .expect("validity probe"); assert!( - first != second, - "THE BITE: exactly one duplicate is reconciled away, not both \ - and not neither (first={first}, second={second})" + !first && !second, + "THE BITE: both buffers bound to the deleted path must be \ + reconciled away; a survivor points at a file that is gone and is \ + invisible to `find_by_path` (first={first}, second={second})" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: removing every match must not leave a window on \ + a removed id; dangling: {dangling:?}" ); } From d5fadf4120e64dcda2b5f653fb90370e59b83903 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 19:12:15 -0400 Subject: [PATCH 09/12] docs(active-work): add the dired Stage 2a lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rides this branch rather than a standalone ledger PR: with several PRs open, a lane written on `main` for work that lands elsewhere re-conflicts on every merge. Records the measured merge-base as pasted output, what 2b and 2c still owe so the split boundary is auditable, the two re-pinned m4 rows, the one framing claim found wrong, the two bites that were vacuous as specified and why, the gate numbers, and the §16 ownership warning against starting Journey Stage 1b while this is open. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 111 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 334ef3a..0529a2e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -655,6 +655,117 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-rd-impl -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. +## dired Stage 2a — rename/delete reconciliation — PR OPEN + +- Portable branch: `githubsucks/dired-stage2-impl`, worktree + `../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged + as #171 (`docs/dired-stage2-framing.md` rev 9, §5/§6/§10 — the + substrate transaction, no dired surface). Position against `main`, as + pasted command output rather than a remembered constant: + + ``` + $ git merge-base HEAD githubsucks/main + e003b81cdd577140fc77330bd4578d3090696877 + ``` + + That base is the #190 merge, and #190 matters here specifically: + Stage 2a **adopts** its `delete_verdict` refusal rather than + reinventing one, and lifts its walk query out into + `editor_core::buffers_bound_under` so the guard and both + reconciliation seams cannot disagree about which buffers an operation + touches. **Re-measure the merge-base before relying on it** — + `main` has branch protection, 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 a move reads as current when it is not. +- **What 2b and 2c still owe, stated so the split boundary is auditable.** + 2a ships **no user-visible surface at all** and no dired code: the + `dired_acceptance` count is deliberately unchanged at **25**, and a + moved count there would mean it touched something it should not have. + 2b owes the mark and operation layer (`m u U t d x D R w M`), + `pmacs.minibuffer.confirm` plus its `src/editor.rs` load-sequence line, + `pmacs.killring.push`, dired's own `resource.renamed` subscriber, and + acceptance 1–22, 33, 39–41. 2c owes `mkdir`/`copy`/`remove_dir_all`, + `JobKind` 12 → 15, `dired.recursive-deletes`, and acceptance 42–47. +- **The split boundary has not moved since rev 9.** It was re-checked + against this tree: #188 (generated-buffer immutability Stage 1) did not + convert dired's `paint`, so §3.1's coordination note is still an + obligation of that lane rather than a collision with this one, and + nothing in this diff touches `builtin/runtime/dired.lua`. +- **Two m4 rows were re-pinned, and that is a behaviour change to a + landed lane's assertions.** `rd9` and `rd14` pinned #190's deliberate + restraint on the `apply_resource_op` delete arm — descendants stay + orphaned, only the first of two duplicate path-bound buffers is + reconciled — and both doc comments gave the same reason: widening + would have routed N buffers through `remove_buffer_and_fire`, phase 2 + without phase 1, leaving up to N windows on removed ids. + `EditorCore::reconcile_delete` composes both phases, so the constraint + is discharged and the old assertions became the defect. Each row now + asserts BOTH directions — reconciled away **and** no window holding a + removed id — and each direction is bite-verified. +- **One framing claim is wrong and is corrected at the test, not + silently worked around.** §5's G1 says a stale captured path + "materializes a phantom" by reaching `resolve_target_buffer`'s + `NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls + `crate::file_io::load_file` directly and maps the error, so a missing + path **raises**, and the `NotFound` arm belongs to + `resolve_target_buffer`, which serves `pmacs.window.display_file` and + the startup/daemon target rather than that binding. The defect is real + and smaller: the `pcall` swallows the raise, so the user is stranded + wherever the last applied op left them. Acceptance 34 is restructured + to bite on that (its plan edits another file first, which is what makes + the restore observable at all) and the correction is recorded in the + test's own doc comment. +- **Two bites were vacuous as the framing specified them, and both + reasons are worth keeping.** Item 28's *rename* row cannot pin the + walk's containment rule: `reconcile_rename` calls + `Path::strip_prefix` to rebuild a descendant's tail, and that is + component-aware too, so a string-prefix walk is silently corrected a + second time. The row moved to the **delete** side, where the walk's + verdict IS the kill list. Item 30's composition-order assertion was a + tautology: the LSP attach leaves `diagnostic` **last** in the stack, and + moving the last element to the end is a no-op, so a remove-and-re-push + was indistinguishable from an in-place mutation; the row now pushes one + more overlay after it and asserts that precondition explicitly. +- **23 acceptance criteria are bite-verified by executed mutation**, each + labelled `OK (assertion)` — none merely `OK (COMPILE)`, and none + vacuous. Items 25, 27, 28, 29 (both directions), 30 (both mutations), + 31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51, + 52, 53b, 54, 55, plus the two re-pinned m4 rows in three + configurations. +- Verification at this head, each gate run to its own file and its own + exit code checked (never through a pipe): `cargo fmt --check` clean; + `cargo clippy --workspace --all-targets -- -D warnings` clean; + `cargo test --lib` **1,875** passed / 3 ignored; `--lib --features + crdt` **2,060** / 4 ignored; the new + `resource_reconciliation_acceptance` **24** default and **24** crdt; + `dired_acceptance` **25** and **25** crdt, deliberately unmoved; the + frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**, + all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed / + 3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**; + `lsp_dispatch_seams_acceptance` **15**; `journey_acceptance` **24** + (the ratchet floor, asserted as a count rather than a colour); + `gpu_invocation_acceptance` **15** crdt — **and that number is only + real with `pmacs` and `pmacs-gpu` built first**, which is the `a37` + trap in §5: the same command reported 12 failures before the build and + 15 passes after, so a red run there is not evidence of a regression + until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p + pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with + `--no-fail-fast` **3,557** passed across **104** suites, 19 ignored, 0 + failed; `git diff --check` clean. +- **Ownership, per the framing's own warning.** §16 says 2a must not run + concurrently with **Journey Stage 1b**, because 1b's LSP + spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s + attachment lifecycle and 1b's compile/binding half touches + `src/editor_core.rs` — the same two files 2a rewrites, where the + conflicts are semantic rather than textual so a clean `git merge` + proves nothing. **1b must not be started while this PR is open.** No + other lane in flight touches them: #188 is `dired.lua`/`buffer.rs` + generated-buffer writes, and the bottom-panel and CI lanes are + elsewhere. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-dired-s2 + -b dired-stage2-impl githubsucks/dired-stage2-impl`. + ## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED - Portable branch: `githubsucks/generated-buffer-immutability`; worktree From 44135fcd73bb67de8e57b75390020521b700de95 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 21:28:16 -0400 Subject: [PATCH 10/12] =?UTF-8?q?fix(stage2a):=20close=20review=20round=20?= =?UTF-8?q?1=20=E2=80=94=20four=20unreported=20failures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four findings were the same shape: a failure that left state wrong and told nobody. **Delete refusals reach the user.** `reconcile_delete_and_fire` returned `kept_modified` and `refused` and both production callers discarded them, so a last-buffer refusal or the asynchronous modified-buffer race left the file gone and the buffer still bound to it — and the next `C-x C-s` recreates the deleted file. Reporting now happens inside the shared seam, not at its call sites, for the same reason the reconciliation does: a caller that has to remember to report is a caller that will forget. The message names the buffers (capped, with a count for the rest) and states the consequence, and it is written to `EditorCore::status`, not `pmacs.error` — that channel is defined only by a test stub, so a report there would be the same silence. `reconcile_delete` now prefixes `kill_buffer`'s reason with the buffer name, because "cannot kill the last remaining buffer" does not say which buffer is now bound to a deleted path. **The LSP subscribers stop swallowing their own failures.** Ignored `pcall`s around `did_close`, `forget_uri`, `did_open` and overlay re-rooting made the callback return successfully, so the `all-must-succeed` logger had nothing to log — concretely, a stale server made `forget_uri` raise while the callback carried on with the old stores, routes and `documents` entry all live. A shared failure sink attributes each step, reports on both channels, and raises **after** the loop, so one unreachable server cannot leave every other attachment unreconciled. **`forget_uri` abandons requests through the established path.** It purged `pending_routes` and `pending_external` but not the same ids `send_request` put in `LspClient.pending`, and recorded nothing in `cancelled_rids`. The per-rid work is extracted from `drain_cancelled_externals` as `abandon_request` and reused, rather than a second incomplete copy: route, client pending, cancelled record and `$/cancelRequest` now happen together. **Acceptance 35 is pinned.** With a plain delete the forbidden fallback was unobservable — `find_or_open` raises out of `load_file` and the `pcall` swallows it — so both assertions passed with the fallback present. The plan now deletes the origin's file and recreates it, which gives the fallback something to open and makes "restores nothing" falsifiable. The corrected G1 explanation also reaches the production comments, which still repeated the false `resolve_target_buffer::NotFound` story. New pins: acceptance 53 and 53b assert the status channel; a stale-server row asserts attribution on both channels *and* that the healthy attachment still reconciles; an `lsp.rs` unit test asserts the client-side abandonment with an unrelated request as its control. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- builtin/runtime/lsp.lua | 138 +++++++++--- src/editor_core.rs | 16 +- src/lsp.rs | 113 ++++++++-- src/lua_bindings/mod.rs | 108 +++++++++- tests/resource_reconciliation_acceptance.rs | 220 ++++++++++++++++++-- 5 files changed, 535 insertions(+), 60 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 679d573..d23409b 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1439,12 +1439,23 @@ local function apply_workspace_edit(ops) if #plan == 0 then return 0, 0, 0 end -- G1 — capture the origin BUFFER, not its path. A path captured here -- is a plain Lua local, and no amount of reconciliation can reach an - -- already-captured local: when the batch renames the active file, the - -- old path no longer resolves, `find_or_open` hits - -- `resolve_target_buffer`'s NotFound arm, and that arm CREATES an - -- empty path-backed buffer and selects it. The user was returned to a - -- phantom file that never existed. The handle follows the rename for - -- free, because the buffer is what moved. + -- already-captured local: once the batch renames or deletes the active + -- file, that string names something that is no longer there. The + -- handle follows a rename for free, because the buffer is what moved. + -- + -- The framing's G1 described the failure as a "phantom buffer" created + -- by `resolve_target_buffer`'s NotFound arm. **That is not what + -- happens on this path, and the wrong explanation is recorded here + -- rather than left to be rediscovered.** `pmacs.buffer.find_or_open` + -- calls `crate::file_io::load_file` directly and maps the error, so a + -- missing path RAISES; the NotFound arm belongs to + -- `EditorCore::resolve_target_buffer`, which serves + -- `pmacs.window.display_file` and the startup/daemon target, not this + -- binding. The real defect is quieter: `restore_origin` runs under a + -- `pcall`, so the raise is swallowed and the user is left in whatever + -- buffer the last applied op made active. And when the old path DOES + -- still resolve -- a batch that deletes and then recreates it -- the + -- fallback silently opens a file the user asked to delete. local origin_buf = pmacs.window.buffer() local edit_total, files, res_ops = 0, 0, 0 -- Plan items fully applied before a failure. Q#RD3 permits partial @@ -1458,9 +1469,9 @@ local function apply_workspace_edit(ops) -- -- **No path fallback (G1).** If the origin buffer is gone — the batch -- deleted its file and reconciliation killed it — restore NOTHING. - -- The old code's path fallback is exactly what fabricated a phantom - -- buffer; "return the user somewhere plausible" is not worth inventing - -- a file that does not exist. + -- "Return the user somewhere plausible" is not worth re-opening a path + -- the batch just destroyed, and when that path has been recreated the + -- fallback would drop the user into a file they asked to delete. local function restore_origin() if not origin_buf then return end pcall(pmacs.window.switch_buffer, origin_buf) @@ -2931,30 +2942,95 @@ local function attachments_under(path) return out end +-- How many attributed failures one status line spells out before +-- collapsing the rest into a count. +local RESOURCE_REPORT_LIMIT = 2 + +-- A failure collector for a reconciliation fan-out. +-- +-- **Why this exists rather than a bare `pcall` per step.** Every step +-- below is fallible for reasons outside this file's control -- a stale +-- server id makes `forget_uri` raise, a stopped server makes `did_close` +-- raise -- and an IGNORED `pcall` makes the hook callback RETURN +-- SUCCESSFULLY. `resource.renamed` and `resource.deleted` are +-- `all-must-succeed`, so the registry's error logger is the mechanism +-- that surfaces a failing subscriber; a callback that swallows its own +-- failures gives that logger nothing to log, and the concrete outcome is +-- silent: `forget_uri` fails, the callback carries on, and the old +-- stores, routes and `documents` entry stay live under a URI the editor +-- no longer holds. +-- +-- It must NOT abort the loop. One unreachable server must not leave +-- every other attachment unreconciled, so failures accumulate and are +-- raised once, after every attachment has been processed. +local function failure_sink(hook_name) + local sink = { hook = hook_name, items = {} } + + -- Run `fn(...)`, and on a raise record it attributed to `what`. + -- Returns `ok, value` like `pcall`, so a caller can branch. + function sink:step(what, fn, ...) + local ok, value = pcall(fn, ...) + if not ok then + self.items[#self.items + 1] = string.format("%s: %s", what, tostring(value)) + end + return ok, value + end + + -- Report everything collected, on BOTH channels, and raise. + -- + -- The raise is what the `all-must-succeed` logger needs in order to + -- write an attributed record to *errors*; the status line is what the + -- user actually sees, because stale LSP state looks like the editor + -- quietly breaking. `pmacs.error` is deliberately not used: it is + -- defined only by a test stub, so writing there would reproduce the + -- silence this replaces. + function sink:finish() + if #self.items == 0 then return end + local shown, n = {}, #self.items + for i = 1, math.min(n, RESOURCE_REPORT_LIMIT) do shown[i] = self.items[i] end + local summary = table.concat(shown, "; ") + if n > #shown then + summary = summary .. string.format("; and %d more", n - #shown) + end + pcall(pmacs.editor.set_status, + string.format("LSP %s: %d reconciliation failure%s -- %s", + self.hook, n, (n == 1 and "" or "s"), summary)) + error(string.format("%s: %s", self.hook, table.concat(self.items, "; ")), 0) + end + + return sink +end + pmacs.hook.add("resource.renamed", function(old_path, new_path) if type(old_path) ~= "string" or type(new_path) ~= "string" then return end + local sink = failure_sink("resource.renamed") for _, hit in ipairs(attachments_under(old_path)) do local key, rec, old_uri = hit.key, hit.rec, hit.rec.uri -- The buffer's own path was rebound before this hook fired, so ask -- it rather than reconstructing the tail ourselves. A buffer that -- somehow lost its path (killed, unbound) cannot be re-opened, and - -- falls through to the teardown-only path below. + -- falls through to the teardown-only path below. Not routed through + -- the sink: a pathless buffer is a legitimate state here, not a + -- reconciliation failure. local ok_path, new_buf_path = pcall(function() return rec.buffer:path() end) local new_uri = (ok_path and new_buf_path) and file_uri_for(new_buf_path) or nil -- 1. Flush any pending didChange for the OLD uri, so the server is -- not left holding an edit it can no longer attribute. - flush_did_change_for(rec) + sink:step("flush didChange for " .. old_uri, flush_did_change_for, rec) pending_did_change[key] = nil -- 2. didClose the old uri — this removes the open-document -- registration and nothing else. - pcall(pmacs.lsp.did_close, rec.server, old_uri) + sink:step("didClose " .. old_uri, pmacs.lsp.did_close, rec.server, old_uri) -- 3. Purge the routes, drain their awaiters, and clear all fourteen -- stores plus `documents` for the old key. Runs against the OLD -- server, which matters when step 4 picks a different one. - pcall(pmacs.lsp.forget_uri, rec.server, old_uri) + -- A failure here is the one that most needs reporting: the + -- callback would otherwise continue with the old stores, routes + -- and `documents` entry all still live. + sink:step("forget_uri " .. old_uri, pmacs.lsp.forget_uri, rec.server, old_uri) if not new_uri then attachments[key] = nil @@ -2964,8 +3040,9 @@ pmacs.hook.add("resource.renamed", function(old_path, new_path) -- 4. Re-run ensure_server. Server affinity keys on the detected -- project root, so a rename ACROSS roots needs a different -- server; a same-root rename reuses the existing one. - local sid = ensure_server(rec.language, new_buf_path) - if not sid then + local ok_sid, sid = sink:step("ensure_server for " .. new_buf_path, + ensure_server, rec.language, new_buf_path) + if not (ok_sid and sid) then attachments[key] = nil styled_buffers[key] = nil diag_viewed_buffers[key] = nil @@ -2976,36 +3053,45 @@ pmacs.hook.add("resource.renamed", function(old_path, new_path) rec.server = sid rec.uri = new_uri rec.version = 1 - local ok_text, text = pcall(buffer_text, rec.buffer) - pcall(pmacs.lsp.did_open, sid, new_uri, rec.version, - ok_text and text or "") + local ok_text, text = sink:step("read " .. new_uri, buffer_text, rec.buffer) + sink:step("didOpen " .. new_uri, pmacs.lsp.did_open, + sid, new_uri, rec.version, ok_text and text or "") -- 6. Re-root the diagnostic overlays. `DiagnosticView.uri` is -- set once at construction and is private, so this is the -- only way to move it — and the sweep reaches PASSIVE -- windows, which the attach path cannot, while preserving -- each overlay's position in the composition order. - pcall(pmacs.diag._rename_resource, old_uri, new_uri) + sink:step("re-root diagnostics to " .. new_uri, + pmacs.diag._rename_resource, old_uri, new_uri) end end end + -- Raised only after EVERY attachment has been processed: one + -- unreachable server must not leave the rest unreconciled. + sink:finish() end) pmacs.hook.add("resource.deleted", function(path) if type(path) ~= "string" then return end + local sink = failure_sink("resource.deleted") for _, hit in ipairs(attachments_under(path)) do local key, rec = hit.key, hit.rec -- No flush: the document is gone, and shipping a didChange for a -- file the server can no longer read buys nothing. pending_did_change[key] = nil - pcall(pmacs.lsp.did_close, rec.server, rec.uri) - pcall(pmacs.lsp.forget_uri, rec.server, rec.uri) - -- Drop the record unconditionally. The buffer may be gone entirely - -- (an unmodified visited file is killed), in which case a retained - -- record is a dangling handle that `repull_for_attachments` would - -- iterate; and a modified buffer kept alive has no file to analyze - -- until it is saved, which re-attaches through the ordinary path. + sink:step("didClose " .. rec.uri, pmacs.lsp.did_close, rec.server, rec.uri) + sink:step("forget_uri " .. rec.uri, pmacs.lsp.forget_uri, rec.server, rec.uri) + -- Drop the record unconditionally, INCLUDING after a failure above. + -- The buffer may be gone entirely (an unmodified visited file is + -- killed), in which case a retained record is a dangling handle that + -- `repull_for_attachments` would iterate; and a modified buffer kept + -- alive has no file to analyze until it is saved, which re-attaches + -- through the ordinary path. Keeping a record whose teardown failed + -- would be strictly worse than dropping it: the failure is reported + -- either way, and a retained one is re-swept every refresh. attachments[key] = nil styled_buffers[key] = nil diag_viewed_buffers[key] = nil end + sink:finish() end) diff --git a/src/editor_core.rs b/src/editor_core.rs index f902418..9b8b5d3 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -4998,7 +4998,21 @@ impl EditorCore { } match self.kill_buffer(id) { Ok(()) => out.killed.push(id), - Err(message) => out.refused.push((id, message)), + // Named, because the reason alone is not actionable: + // `kill_buffer`'s "cannot kill the last remaining + // buffer" says nothing about *which* buffer is now + // bound to a path whose file is gone, and that buffer's + // name is what the user needs in order to save it + // somewhere else. + Err(message) => { + let name = self + .registry + .borrow() + .get(id) + .map_or_else(|_| format!("{id:?}"), |b| b.name().to_owned()); + out.refused + .push((id, format!("buffer {name:?}: {message}"))); + } } } out diff --git a/src/lsp.rs b/src/lsp.rs index ba32fb2..f5630b6 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -1699,15 +1699,35 @@ impl LspManager { ); } for rid in abandoned_rids { - self.pending_routes.remove(&(sid, rid)); - if let Some(client) = self.clients.get_mut(&sid) { - client.pending.remove(&rid); - client.cancelled_rids.insert(rid); - } - self.send_cancel_request(sid, rid); + self.abandon_request(sid, rid); } } + /// Abandon one in-flight request: drop its response route, drop the + /// client's `pending` entry, record the rid so a late reply is + /// dropped silently rather than surfacing as an unmatched response, + /// and ask the server to stop working on it. + /// + /// Extracted from [`Self::drain_cancelled_externals`] by dired Stage + /// 2a so [`Self::forget_uri`] reuses it instead of being a second, + /// incomplete copy. **All four steps are load-bearing together.** + /// Removing only the route and the awaiter — which is what + /// `forget_uri` originally did — leaves `client.pending` holding the + /// rid forever when the server never replies, and leaves + /// `cancelled_rids` without it, so a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename that is worse than a leak: the old server keeps the entry + /// and no attachment drains it afterwards, so the entries + /// accumulate. + fn abandon_request(&mut self, sid: LspServerId, rid: u64) { + self.pending_routes.remove(&(sid, rid)); + if let Some(client) = self.clients.get_mut(&sid) { + client.pending.remove(&rid); + client.cancelled_rids.insert(rid); + } + self.send_cancel_request(sid, rid); + } + /// Send `$/cancelRequest { id }` to `sid`, best-effort. A server /// that is not accepting writes (stopped / crashed) is skipped by /// [`Self::send_notification`]'s state guard; the `Err` is @@ -3123,7 +3143,11 @@ impl LspManager { /// respect to another manager tick, but putting the gate first /// means every later call observes the forgotten state even if a /// future refactor introduces an early return. - /// 2. **Purge `pending_routes`** whose route carries this URI. + /// 2. **Abandon every in-flight request scoped to this URI**, through + /// [`Self::abandon_request`] — the same path the per-tick + /// cancellation sweep uses, so the route, the client's `pending` + /// entry, the `cancelled_rids` record and `$/cancelRequest` all + /// happen together rather than only the first of the four. /// `WorkspaceSymbol` is retained unconditionally: it carries no /// URI at all — its query stands in for the doc URI in the /// supersede key — and a workspace-symbol query is not scoped to @@ -3182,24 +3206,26 @@ impl LspManager { // Step 1 — the gate, first. self.forgotten_documents.insert((sid, uri.to_owned())); - // Step 2 — collect the rids this URI owns, then purge. + // Steps 2 and 3 — collect the rids this URI owns, settle their + // awaiters cancelled, then abandon each request through the + // SAME path the per-tick sweep uses + // ([`Self::abandon_request`]): route, `client.pending`, + // `cancelled_rids`, `$/cancelRequest`. Purging the route alone + // would leave the request live in the client and a late reply + // unrecognised. let doomed_rids: Vec = self .pending_routes .iter() .filter(|((s, _), route)| *s == sid && route.scoped_uri() == Some(uri)) .map(|((_, rid), _)| *rid) .collect(); - for rid in &doomed_rids { - self.pending_routes.remove(&(sid, *rid)); - } - - // Step 3 — settle the awaiters joined to those rids cancelled. for rid in &doomed_rids { if let Some(p) = self.pending_external.remove(&(sid, *rid)) { for a in &p.awaiters { self.runtime.complete_external_cancelled(a.job_id); } } + self.abandon_request(sid, *rid); } // Step 4 — the fourteen stores plus `documents`. @@ -4556,6 +4582,67 @@ mod resource_reconciliation_tests { assert!(mgr.pending_external.contains_key(&(a, 2))); } + /// Review round 1 — `forget_uri` must abandon the request in the + /// **client**, not only in the route table. + /// + /// `pending_routes` and `pending_external` are two of four places an + /// in-flight request lives. `LspClient.pending` (written by + /// `send_request`) and `cancelled_rids` are the other two, and + /// dropping only the first two leaves the entry live forever when the + /// server never replies, while a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename the old server keeps those entries and no attachment + /// drains it afterwards, so they accumulate. + /// + /// Bite: fails against a `forget_uri` that purges routes and + /// awaiters without going through `abandon_request`. + #[test] + fn forget_uri_abandons_the_request_in_the_client_too() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + + for (rid, uri) in [(11u64, old), (12u64, other)] { + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + let client = mgr.clients.get_mut(&a).expect("client a"); + client.pending.insert(rid, "textDocument/hover".to_owned()); + } + let client = mgr.clients.get(&a).expect("client a"); + assert!(client.pending.contains_key(&11), "precondition"); + assert!(client.pending.contains_key(&12), "precondition"); + assert!( + client.cancelled_rids.is_empty(), + "precondition: nothing abandoned yet" + ); + + mgr.forget_uri(a, old).expect("forget"); + + let client = mgr.clients.get(&a).expect("client a"); + assert!( + !client.pending.contains_key(&11), + "the purged request must leave `client.pending`, or it leaks \ + for the lifetime of a server that never replies" + ); + assert!( + client.cancelled_rids.contains(&11), + "and must be recorded, or a late reply surfaces as a generic \ + unrouted response instead of being dropped" + ); + assert!( + client.pending.contains_key(&12), + "an unrelated document's request must survive" + ); + assert!( + !client.cancelled_rids.contains(&12), + "and must not be marked abandoned" + ); + } + /// Acceptance 31c — the error contract, both arms. The second is the /// one that matters: the subscriber runs per attachment, an /// attachment need not have any pending route or populated result, diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 212e962..a8a130b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1797,17 +1797,109 @@ fn reconcile_delete_and_fire( for id in &outcome.killed { after_buffer_removed(lua, *id); } - let mut args = mlua::MultiValue::new(); - args.push_back(mlua::Value::String( - match lua.create_string(normalized.as_os_str().as_encoded_bytes()) { - Ok(s) => s, - Err(_) => return outcome, - }, - )); - run_hook_if_defined(lua, "resource.deleted", args); + if let Ok(path_arg) = lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String(path_arg)); + run_hook_if_defined(lua, "resource.deleted", args); + } + // Reported AFTER the fan-out, deliberately: a subscriber may set its + // own status, and this message must be the last word because it is + // the data-loss-adjacent one. Unconditional, so a path that cannot + // cross into Lua still gets its refusal reported rather than losing + // both the hook and the report. + report_delete_reconcile(lua, &normalized, &outcome); outcome } +/// Cap on how many buffer names one status line spells out before +/// collapsing the rest into a count. A directory delete can reach +/// dozens; a status line that scrolls off is a message nobody reads. +const DELETE_REPORT_NAMED_LIMIT: usize = 3; + +/// Render the buffers a delete could not reconcile, and put it on the +/// status channel. +/// +/// **Silence here is the defect this exists to close.** Both outcomes +/// leave a buffer alive and still bound to a path whose file is gone, so +/// the next `C-x C-s` recreates the file the user just deleted. That is +/// recoverable only if the user knows it happened: +/// +/// * `kept_modified` — a modified buffer, kept on purpose. On the +/// synchronous path #190 refuses before disk so this cannot arise, but +/// `pmacs.fs.remove` dispatches a worker, and a buffer modified in the +/// interval between the caller's check and the syscall reaches here. +/// * `refused` — could not be removed at all: the last remaining buffer +/// (`kill_buffer` refuses to empty the registry), or a buffer that was +/// mid-edit when the reconciliation ran. +/// +/// The channel is `EditorCore::status`, which is what +/// `pmacs.editor.set_status` writes. **Not `pmacs.error`** — that +/// channel is defined only by a test stub, so all fifteen of its guarded +/// call sites are dead, and a report written there would be exactly the +/// silence being fixed. +/// +/// Lives inside the shared seam rather than at its two call sites, for +/// the same reason the reconciliation does: a caller that has to +/// remember to report is a caller that will forget. The first version of +/// this function's callers both discarded the outcome. +fn report_delete_reconcile( + lua: &Lua, + path: &std::path::Path, + outcome: &crate::editor_core::DeleteReconcile, +) { + if outcome.kept_modified.is_empty() && outcome.refused.is_empty() { + return; + } + let name_of = |p: &std::path::Path| { + p.file_name() + .map_or_else(|| p.display().to_string(), |n| n.to_string_lossy().into()) + }; + let mut parts: Vec = Vec::new(); + if !outcome.kept_modified.is_empty() { + let n = outcome.kept_modified.len(); + let named: Vec<&str> = outcome + .kept_modified + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, name)| name.as_str()) + .collect(); + parts.push(format!( + "{n} buffer{} with unsaved changes kept ({}{}) — saving {} will RECREATE the deleted file", + if n == 1 { "" } else { "s" }, + named.join(", "), + if n > named.len() { + format!(", and {} more", n - named.len()) + } else { + String::new() + }, + if n == 1 { "it" } else { "them" }, + )); + } + if !outcome.refused.is_empty() { + let n = outcome.refused.len(); + let named: Vec = outcome + .refused + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, why)| why.clone()) + .collect(); + parts.push(format!( + "{n} buffer{} could not be closed ({}{})", + if n == 1 { "" } else { "s" }, + named.join("; "), + if n > named.len() { + format!("; and {} more", n - named.len()) + } else { + String::new() + }, + )); + } + let message = format!("deleted {}: {}", name_of(path), parts.join("; ")); + if let Some(core) = lua.app_data_ref::() { + core.borrow_mut().status = message; + } +} + /// Drive [`crate::async_runtime::TickOutcome::resources`] through /// reconciliation, one settled mutation at a time (dired Stage 2a, /// Q#DR29). diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs index f1ba6f9..63870a0 100644 --- a/tests/resource_reconciliation_acceptance.rs +++ b/tests/resource_reconciliation_acceptance.rs @@ -147,6 +147,10 @@ fn buffer_name(state: &EditorState, global: &str) -> Option { ) } +fn status(state: &EditorState) -> String { + state.core.borrow().status.clone() +} + fn buffer_is_valid(state: &EditorState, global: &str) -> bool { eval( state, @@ -903,6 +907,21 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { "the last remaining buffer cannot be killed, so it survives the \ deletion of its file" ); + // **Reported, not silent.** Survival alone is not the criterion: the + // buffer is still bound to a path whose file is gone, so the next + // `C-x C-s` recreates the file the user deleted. That is recoverable + // only if the user is told. + let said = status(&state); + assert!( + said.contains("could not be closed"), + "the refusal must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("only.txt"), + "and must name the buffer, because `cannot kill the last \ + remaining buffer` alone does not say WHICH buffer is now bound \ + to a deleted path; status was {said:?}" + ); // Half two: a directory of buffers where one refuses removal. The // rest must still reconcile. @@ -933,6 +952,26 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { "and C still reconciled afterwards — one refusal must not abort \ the rest" ); + // The kept-modified case reports too, and says what the consequence + // is. This is the asynchronous race the framing's H1 leaves open: + // #190 refuses before disk on the synchronous path, but + // `pmacs.fs.remove` dispatches a worker, so a buffer modified in the + // interval reaches the drain with its file already gone. + let said2 = status(&state2); + assert!( + said2.contains("unsaved changes kept"), + "a modified buffer kept alive over a deleted file must be \ + reported; status was {said2:?}" + ); + assert!( + said2.contains("RECREATE"), + "and the report must state the consequence — saving it puts the \ + deleted file back; status was {said2:?}" + ); + assert!( + said2.contains("b.txt"), + "naming the buffer; status was {said2:?}" + ); } // --------------------------------------------------------------------------- @@ -1082,6 +1121,16 @@ fn acc53b_a_mid_edit_refusal_leaves_window_side_and_round_trip_state_untouched() core.registry.borrow().contains(doomed_id), "and the buffer itself is still in the registry" ); + drop(core); + // And the refusal is REPORTED. Leaving state untouched is only half + // the contract: the file is gone, so a user who is not told keeps a + // buffer bound to a path that no longer exists. + let said = status(&state); + assert!( + said.contains("mid-edit") && said.contains("could not be closed"), + "a mid-edit refusal must reach the status channel; status was \ + {said:?}" + ); } // --------------------------------------------------------------------------- @@ -1771,6 +1820,22 @@ fn acc34_renaming_the_active_file_through_the_applier_returns_the_same_buffer() /// Acceptance 35. When the origin buffer is **gone** after the edit, the /// applier restores **nothing** rather than falling back to the old /// path. +/// +/// **The plan deletes the origin's file and then RECREATES it, and that +/// is what makes the row bite at all.** With a plain delete the forbidden +/// fallback is unobservable: `find_or_open` on a path that no longer +/// exists raises straight out of `file_io::load_file`, the surrounding +/// `pcall` swallows it, and nothing happens — so "no buffer at the old +/// path" and "the active buffer is live" both hold with the fallback +/// present. Recreating the path gives the fallback something to open, and +/// it is not a contrived shape: a `documentChanges` batch that deletes +/// and recreates a file is ordinary LSP refactoring output. +/// +/// Bite: the applier restoring by path instead of by buffer handle. The +/// handle is invalid (reconciliation killed the buffer) so a +/// handle-based restore does nothing; a path-based one loads the +/// recreated file into a NEW buffer and switches the user into it — +/// dropping them, silently, into a file they asked to delete. #[test] fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { let fx = Fixture::new(); @@ -1783,10 +1848,18 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { &state, &fx.root, &serde_json::json!({ - "documentChanges": [{ - "kind": "delete", - "uri": file_uri(&doomed), - }], + "documentChanges": [ + { + "kind": "delete", + "uri": file_uri(&doomed), + }, + { + // Recreates the path, so a path-based restore has a + // file to open and the fallback becomes observable. + "kind": "create", + "uri": file_uri(&doomed), + }, + ], }), ); // `other` keeps the registry non-empty so the delete's kill is not @@ -1806,12 +1879,17 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { ); settle_a_while(&mut state); - assert!(!doomed.exists(), "the file is gone"); + assert!( + doomed.exists(), + "precondition for the bite: the batch recreated the path, so a \ + path-based restore CAN open it" + ); assert!( !buffer_is_valid(&state, "B"), - "and its clean buffer was reconciled away" + "the origin buffer was reconciled away by the delete" ); - let phantom: bool = eval( + + let reopened: bool = eval( &state, &format!( "for _, b in ipairs(pmacs.buffer.list()) do @@ -1822,10 +1900,20 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { ), ); assert!( - !phantom, - "the applier must restore NOTHING rather than re-opening the path \ - it just deleted — a path fallback would recreate it as an empty \ - buffer, and the next C-x C-s would resurrect the file" + !reopened, + "the applier must restore NOTHING. A path-based restore loads the \ + recreated file into a fresh buffer, which is the editor silently \ + re-opening a file the user asked to delete" + ); + + let active_path: Option = eval( + &state, + "local b = pmacs.window.buffer(); return b and b:path() or nil", + ); + assert_ne!( + active_path.as_deref(), + Some(doomed.to_str().unwrap()), + "and the user must not be sitting in it either" ); let active_valid = { let core = state.core.borrow(); @@ -1834,6 +1922,114 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { }; assert!( active_valid, - "and the window it left behind must sit on a live buffer" + "the window it left behind must still sit on a live buffer" + ); +} + +/// Review round 1 — a reconciliation failure inside the LSP subscriber +/// must be **reported and attributed**, and must not stop the remaining +/// attachments from reconciling. +/// +/// The scenario is the reviewer's: a **stale server id**. The attachment +/// record still names a server the manager has forgotten, so +/// `did_close` and `forget_uri` both raise. With ignored `pcall`s the +/// callback returned successfully, so the `all-must-succeed` hook logger +/// had nothing to log, and the old stores, routes and `documents` entry +/// stayed live under a URI the editor no longer held — silently. +/// +/// Two packages under one parent directory give two servers, and only +/// one is staled out, so the row can assert both halves at once: the +/// failure is surfaced, **and** the healthy attachment still moves. +/// +/// Bite: fails against ignored `pcall`s (nothing on either channel), and +/// against a fix that lets the first failure `error()` out of the loop +/// (the healthy attachment would never reconcile). +#[test] +fn a_subscriber_reconciliation_failure_is_reported_and_the_rest_still_reconcile() { + let fx = Fixture::new(); + fx.write("w/a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("w/b/Cargo.toml", "[package]\nname = \"b\"\n"); + let file_a = fx.write("w/a/src/main.rs", "fn main() {}\n"); + let file_b = fx.write("w/b/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "A", &file_a); + settle_until(&mut state, "server for package a", |s| server_count(s) == 1); + open_as(&state, "B", &file_b); + settle_until(&mut state, "server for package b", |s| server_count(s) == 2); + + // Stale out the server serving package `a` only: stop it, then forget + // it, leaving `attachments[a].server` naming a server the manager no + // longer holds. `forget_uri` raises for an unknown server id, which + // is exactly the failure mode under test. + let root_a = file_uri(&fx.at("w/a")); + exec( + &state, + &format!( + "local victim + for _, row in ipairs(pmacs.lsp.list()) do + if row.root_uri == \"{root_a}\" then victim = row.id end + end + assert(victim, 'no server rooted at package a') + pcall(pmacs.lsp.stop, victim) + _G.VICTIM = victim" + ), + ); + settle_until(&mut state, "the victim is forgotten", |s| { + let gone: bool = eval( + s, + "pcall(pmacs.lsp.forget, _G.VICTIM) + for _, row in ipairs(pmacs.lsp.list()) do + if row.id == _G.VICTIM then return false end + end + return true", + ); + gone + }); + exec(&state, "pmacs.editor.set_status('')"); + + // Rename the parent, so BOTH attachments are in the fan-out. + let new_uri_b = file_uri(&fx.at("w2/b/src/main.rs")); + rename_fire_and_forget(&mut state, &fx.at("w"), &fx.at("w2")); + settle_a_while(&mut state); + + // Half one: the failure is surfaced, on both channels, attributed to + // the operation that failed. + let said = status(&state); + assert!( + said.contains("resource.renamed") && said.contains("reconciliation failure"), + "the failure must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("forget_uri"), + "and must name WHICH step failed — an unattributed count does not \ + tell anyone that the URI-keyed stores were left live; status was \ + {said:?}" + ); + + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("resource.renamed") && errors.contains("forget_uri"), + "the callback must RAISE, so the all-must-succeed hook logger has \ + something to record; *errors* held {errors:?}" + ); + + // Half two: the healthy attachment still reconciled. The fake + // republishes diagnostics on every `didOpen`, so diagnostics under + // the NEW uri prove the whole ordered teardown ran for package b + // after package a's failed. + settle_until(&mut state, "package b reattached at its new uri", |s| { + diag_count(s, &new_uri_b) > 0 + }); + assert!( + diag_count(&state, &new_uri_b) > 0, + "one unreachable server must not leave every other attachment \ + unreconciled — the raise has to come after the loop, not inside it" ); } From a131c880e2b67eb88ac076e6e343765bd0c96ae3 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 21:34:42 -0400 Subject: [PATCH 11/12] test(stage2a): make acceptance 53's attribution assertion bite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bite-verifying the round-1 pins caught one of them passing with the bug restored. `contains("only.txt")` was satisfied by the status message's own `deleted only.txt:` prefix — the deleted path's basename — so stripping the `buffer "…"` attribution changed nothing the assertion could see. Both halves now assert the buffer's OWN name, which for a path-backed buffer is the full path and which only the attribution can produce. Dropping either name — the refusal reason's or the kept-modified list's — now fails. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- tests/resource_reconciliation_acceptance.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs index 63870a0..c9beba4 100644 --- a/tests/resource_reconciliation_acceptance.rs +++ b/tests/resource_reconciliation_acceptance.rs @@ -916,11 +916,18 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { said.contains("could not be closed"), "the refusal must reach the status channel; status was {said:?}" ); + // Asserted as the buffer's OWN name, not as the basename. The + // message opens with `deleted only.txt:` — the *path* — so a + // `contains("only.txt")` check passes with the attribution stripped, + // which is exactly how this assertion was vacuous when first + // written. A path-backed buffer's name is the full path, and only + // the `buffer "…"` prefix can produce it. + let expect_named = format!("buffer {:?}", only.display().to_string()); assert!( - said.contains("only.txt"), - "and must name the buffer, because `cannot kill the last \ + said.contains(&expect_named), + "the refusal must name the buffer, because `cannot kill the last \ remaining buffer` alone does not say WHICH buffer is now bound \ - to a deleted path; status was {said:?}" + to a deleted path; wanted {expect_named:?} in {said:?}" ); // Half two: a directory of buffers where one refuses removal. The @@ -968,9 +975,12 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { "and the report must state the consequence — saving it puts the \ deleted file back; status was {said2:?}" ); + // Same discipline: the full path is the buffer's name, while the + // message's `deleted b.txt:` prefix is only the basename. assert!( - said2.contains("b.txt"), - "naming the buffer; status was {said2:?}" + said2.contains(&b.display().to_string()), + "the kept buffer must be named, and by its own name rather than \ + the deleted path's basename; status was {said2:?}" ); } From e11c3d4ab8b26a1a3b8e21533e795ba7c2254d8c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 21:47:32 -0400 Subject: [PATCH 12/12] docs(active-work): record dired Stage 2a review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four findings were one shape — a failure that left state wrong and told nobody — so the lane records them as one lesson rather than four bugs: every one was a `pcall` or a discarded return value, and each looked like defensive coding. Also records the round-1 pin that passed with its own bug restored (acceptance 53's attribution assertion was satisfied by the deleted path's basename appearing elsewhere in the same message), the refreshed gate numbers, and that `main` was re-measured after the round and had not moved. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 76 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 0529a2e..bdd0f00 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -655,7 +655,7 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-rd-impl -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. -## dired Stage 2a — rename/delete reconciliation — PR OPEN +## dired Stage 2a — rename/delete reconciliation — PR #196 OPEN, review round 1 closed - Portable branch: `githubsucks/dired-stage2-impl`, worktree `../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged @@ -676,7 +676,9 @@ has **no branch and no framing yet**. touches. **Re-measure the merge-base before relying on it** — `main` has branch protection, 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 a move reads as current when it is not. + from before a move reads as current when it is not. **Re-measured after + round 1: `main` had not moved, so no integration was needed** — that is + a reading of the tree, not a standing fact. - **What 2b and 2c still owe, stated so the split boundary is auditable.** 2a ships **no user-visible surface at all** and no dired code: the `dired_acceptance` count is deliberately unchanged at **25**, and a @@ -732,17 +734,72 @@ has **no branch and no framing yet**. 31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51, 52, 53b, 54, 55, plus the two re-pinned m4 rows in three configurations. +- **Review round 1 found four defects; all four are fixed, and all four + were the same shape — a failure that left state wrong and told nobody.** + Worth keeping as one lesson rather than four bugs: every one of them + was a `pcall` or a discarded return value, and each *looked* like + defensive coding. + - **P1 — delete refusals were silent.** `reconcile_delete_and_fire` + returned `kept_modified` and `refused` and both production callers + discarded them, so a last-buffer refusal or the asynchronous + modified-buffer race left the file gone and the buffer still bound to + it — and the next `C-x C-s` recreates the deleted file. Reporting + moved **inside the shared seam**, for the same reason the + reconciliation lives there: a caller that has to remember to report + is a caller that will forget. Channel is `EditorCore::status`; + **not `pmacs.error`**, which is defined only by a test stub, so a + report there would have been the same silence. + - **P2 — the LSP subscribers swallowed their own reconciliation + failures.** Ignored `pcall`s made the callback return successfully, + so the `all-must-succeed` logger had nothing to log. A shared + failure sink now attributes each step and raises **after** the loop, + because a fix that aborts on the first failure would leave every + other attachment unreconciled — that wrong fix is itself a + bite-verified mutation. + - **P2 — `forget_uri` left purged requests live in the client.** It + dropped `pending_routes` and `pending_external` but not the ids + `send_request` puts in `LspClient.pending`, and recorded nothing in + `cancelled_rids`, so a server that never replies leaked the entry and + a late reply surfaced as a generic unrouted response. The per-rid + work is now extracted from `drain_cancelled_externals` as + `abandon_request` and **reused** rather than copied. + - **P2 — acceptance 35 was unpinned even after the G1 correction.** + With a plain delete the forbidden path fallback is unobservable: + `find_or_open` raises out of `load_file` and the `pcall` swallows it, + so both assertions passed with the fallback present. The plan now + deletes the origin's file **and recreates it**, which gives the + fallback something to open. The corrected G1 explanation also reached + the production comments, which still repeated the false + `resolve_target_buffer::NotFound` story — *a correction that stops at + the test comment has only half landed.* +- **One round-1 pin passed with its own bug restored, and the reason is + reusable.** Acceptance 53 asserted `contains("only.txt")` for the + buffer-name attribution — but the status line opens with + `deleted only.txt:`, the deleted path's **basename**, so stripping the + attribution changed nothing the assertion could see. Both halves now + assert the buffer's *own* name, which for a path-backed buffer is the + full path and which only the attribution can produce. **A pin written + to close a review finding is exactly the kind that passes with the bug + restored**, and the detector was running the bite rather than reading + the assertion. +- **31 bites now, all executed, every one labelled `OK (assertion)`** — + the original 23 plus 8 for round 1 (report call removed; refusal reason + unattributed; kept-modified name dropped; subscriber failures + swallowed; the wrong fix that aborts the loop; `forget_uri` skipping + `abandon_request`; and the forbidden path fallback restored, which must + fail acceptance 34 **and** 35 independently). - Verification at this head, each gate run to its own file and its own exit code checked (never through a pipe): `cargo fmt --check` clean; `cargo clippy --workspace --all-targets -- -D warnings` clean; - `cargo test --lib` **1,875** passed / 3 ignored; `--lib --features - crdt` **2,060** / 4 ignored; the new - `resource_reconciliation_acceptance` **24** default and **24** crdt; + `cargo test --lib` **1,876** passed / 3 ignored; `--lib --features + crdt` **2,061** / 4 ignored; the new + `resource_reconciliation_acceptance` **25** default and **25** crdt; `dired_acceptance` **25** and **25** crdt, deliberately unmoved; the frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**, all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed / 3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**; - `lsp_dispatch_seams_acceptance` **15**; `journey_acceptance` **24** + `lsp_dispatch_seams_acceptance` **15**; + `typed_edit_chain_acceptance` **13**; `journey_acceptance` **24** (the ratchet floor, asserted as a count rather than a colour); `gpu_invocation_acceptance` **15** crdt — **and that number is only real with `pmacs` and `pmacs-gpu` built first**, which is the `a37` @@ -750,8 +807,11 @@ has **no branch and no framing yet**. 15 passes after, so a red run there is not evidence of a regression until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with - `--no-fail-fast` **3,557** passed across **104** suites, 19 ignored, 0 - failed; `git diff --check` clean. + `--no-fail-fast` **3,559** passed across **104** suites, 19 ignored, 0 + failed; `git diff --check` clean. Every one of those was run as its own + step with its own exit status checked — never `cmd | tail` inside an + `&&` chain, which returns *tail's* status and has masked a real failure + in this repo before. - **Ownership, per the framing's own warning.** §16 says 2a must not run concurrently with **Journey Stage 1b**, because 1b's LSP spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s