fix(completion): address TUI-validation findings (LSP query gaps, scoping, prefix keys, window scope)

Five findings from the manual validation pass, all in-branch:

1. LSP-only words never queried the server: the auto-open path fired
   request_completion only when the sync providers already produced
   rows. An empty sweep now leaves a pending session and the request
   always fires; isIncomplete responses re-request on further typing.
   Corollary: attachment_for_request now flushes-if-attached but NEVER
   attaches -- the first cut wrapped attached_for_active, which spawns
   servers on demand, i.e. per-keystroke spawn attempts in unattached
   buffers (wedged the parallel m4 suite; serial ran 3x slower).
   Attachment stays buffer-open policy.

2. Cross-buffer LSP leak: the built-in provider's no-uri fallback was
   the legacy global store drain, so scratch/unattached buffers could
   show another file's cached completions. Strict now: no uri, no rows.

3. Pending prefixes own the keyboard: Action::Pending (C-x ...)
   dismisses the popup and the popup shadow is guarded on an empty
   dispatcher prefix, so the sequence's continuation and its C-g abort
   reach the dispatcher instead of the popup.

4. Window-scoped sessions: CompletionPopupState.window_id (stamped by
   completion_popup_open; Lua never sees it). Only the owning window's
   overlay paints -- same-buffer splits each carry a persistent
   overlay -- and a focus change invalidates the session.

5. Flaky worker test: the /proc thread-count probe and the idempotence
   check both build EditorStates and could run concurrently, polluting
   the baseline; merged into one test (non-Linux keeps a portable
   idempotence variant).

Regression tests for 1-4; framing doc gains the as-built notes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Levi Neuwirth 2026-07-07 16:54:14 -04:00
parent a1f5b1ffd7
commit 61a31b3ad4
9 changed files with 227 additions and 44 deletions

View File

@ -176,7 +176,15 @@ pmacs.hook.add("buffer.after-edit", function()
close_popup()
return
end
if not publish(buf, session.anchor, prefix, "incomplete", nil) then
if publish(buf, session.anchor, prefix, "incomplete", nil) then
-- isIncomplete contract: a partial server response must be
-- re-queried as the user keeps typing, not merely re-filtered.
local rec = pmacs.lsp.active_attachment()
if rec then
local ok, incomplete = pcall(pmacs.completion.is_incomplete, rec.server, rec.uri)
if ok and incomplete then request_lsp_then_refresh() end
end
else
session = nil -- narrowed to nothing: the session is over
end
return
@ -187,9 +195,14 @@ pmacs.hook.add("buffer.after-edit", function()
if key ~= prev_key or not prev_cursor or cursor - prev_cursor ~= 1 then return end
local prefix = word_prefix_before(buf, cursor)
if #prefix >= MIN_PREFIX then
if publish(buf, cursor - #prefix, prefix, "invoked", nil) then
request_lsp_then_refresh()
-- Fire the LSP request even when the synchronous providers came
-- up empty: for an LSP-only word (no dabbrev/snippet/index hit,
-- cold store) the popup materializes when the response lands ---
-- the same pending-session shape as the trigger-char path.
if not publish(buf, cursor - #prefix, prefix, "invoked", nil) then
session = { key = key, anchor = cursor - #prefix, pending = true }
end
request_lsp_then_refresh()
return
end
if #prefix == 0 then

View File

@ -545,16 +545,22 @@ function pmacs.lsp.active_attachment()
end
-- Flushing variant for request-issuing callers outside this file
-- (Q#C8): resolves (or attaches) the active buffer's server AND
-- flushes any debounced didChange first, so the server answers the
-- caller's request against the current text --- exactly what every
-- interactive command in this file gets from the local
-- `attached_for_active`. The in-buffer completion driver
-- (builtin/runtime/completion.lua) calls this before
-- textDocument/completion; a non-flushing peek would hand the server
-- stale text after a typing burst.
-- (Q#C8): when the active buffer already has a server attached,
-- flush any debounced didChange first and return the record, so the
-- caller's request is answered against the current text. Unlike the
-- local `attached_for_active`, this NEVER triggers an attach: the
-- in-buffer completion driver calls it on ordinary typing, and
-- spawning language servers as a typing side effect is wrong (and,
-- concretely, wedged the m4 suite with per-keystroke spawn attempts
-- across parallel tests). Attachment remains buffer-open policy.
function pmacs.lsp.attachment_for_request()
return attached_for_active()
local buf = pmacs.window.buffer()
if not buf then return nil end
local key = tostring(buf)
local rec = attachments[key]
if not rec then return nil end
flush_did_change(key)
return rec
end
-- Hooks --------------------------------------------------------------------

View File

@ -277,6 +277,42 @@ and kill-ring are the named next table-stakes items).
shows misfires, the fix is the named hook-payload PR, not
heuristic patching.
## As-built notes — phase 1 (PR #92)
Landed close to the framing; the user's TUI validation pass surfaced
five findings, all addressed in-branch:
1. **LSP-only words never queried the server** — the auto-open path
fired `request_completion` only when the synchronous providers had
already produced rows. Now an empty sweep leaves a *pending*
session (the trigger-char shape) and the request always fires;
`isIncomplete` responses re-request on further typing via
`pmacs.completion.is_incomplete`. Corollary found while fixing:
`attachment_for_request` must flush-if-attached but **never
attach** — the first cut wrapped `attached_for_active`, which
spawns a server on demand, i.e. per-keystroke spawn attempts in
every unattached buffer (wedged the parallel m4 suite).
Attachment stays buffer-open policy.
2. **Strict URI scoping** — the built-in LSP provider now returns
*nothing* without `ctx.uri` (the framing's "legacy global drain
when absent" allowed unattached/scratch buffers to show another
file's cached completions).
3. **Pending prefixes own the keyboard**`Action::Pending` (`C-x
...`) dismisses the popup, and the popup shadow is additionally
guarded on `dispatcher.pending().is_empty()`, so a sequence's
continuation and its `C-g` abort reach the dispatcher.
4. **Window-scoped sessions**`CompletionPopupState.window_id`
(stamped by `completion_popup_open`; Lua never sees it): only the
owning window's overlay paints (same-buffer splits each carry a
persistent overlay), and a focus change invalidates the session.
5. The worker-pool teardown fix (signal-only `EditorState::drop`)
rode along in the PR — unrelated to completion, surfaced by
running the m4 gate.
Also caught by the new acceptance suite pre-validation:
`install_completion` rebuilt `pmacs.completion` and clobbered the
popup bindings — all `pmacs.completion` installers now merge.
## Deferred (named, not silently dropped)
- Snippet tabstops/placeholders (v1 inserts bodies literally).

View File

@ -487,6 +487,14 @@ pub struct CompletionPopupState {
/// Buffer the popup targets. The session closes the moment the
/// active buffer differs (Q#C3 validation).
pub buffer_id: BufferId,
/// Window the popup belongs to. Stamped by
/// [`crate::editor_core::EditorCore::completion_popup_open`]
/// (Lua publishers don't know window identity), `None` only
/// before that stamp. Two splits showing the same buffer each
/// carry a persistent overlay --- without this, both would paint
/// the popup; with it, only the owning window's overlay renders,
/// and a focus change closes the session (Q#C3).
pub window_id: Option<crate::window::WindowId>,
/// Byte offset where the typed prefix starts. For a
/// trigger-character session (e.g. right after `.`) the prefix is
/// empty and `anchor` equals the cursor.
@ -519,6 +527,7 @@ impl CompletionPopupState {
}
Some(Self {
buffer_id,
window_id: None,
anchor,
prefix,
candidates,
@ -637,13 +646,19 @@ pub(crate) fn popup_window(n: usize, selected: usize, max: usize) -> (usize, usi
/// the popup would overflow the window's right edge.
pub struct CompletionView {
popup: SharedCompletionPopup,
/// The window this overlay instance is attached to. Overlays
/// persist per window; only the one matching the session's
/// `window_id` paints, so same-buffer splits don't each show
/// the popup.
window_id: crate::window::WindowId,
}
impl CompletionView {
/// Build a view reading `popup`.
/// Build a view reading `popup`, rendering only when the open
/// session belongs to `window_id`.
#[must_use]
pub fn new(popup: SharedCompletionPopup) -> Self {
Self { popup }
pub fn new(popup: SharedCompletionPopup, window_id: crate::window::WindowId) -> Self {
Self { popup, window_id }
}
}
@ -826,6 +841,9 @@ impl View for CompletionView {
let Some(popup) = guard.as_ref() else {
return;
};
if popup.window_id != Some(self.window_id) {
return; // the session belongs to another window
}
if popup.buffer_id != buf.id() {
return; // this window shows a different buffer
}

View File

@ -624,15 +624,18 @@ fn project_kind_to_completion_kind(k: &crate::project_index::SymbolKind) -> Comp
/// the LSP completion store. The framework does **not** drive a
/// fresh `textDocument/completion` request --- that's the editor's
/// job; we just read whatever the async pipeline has produced so
/// far. With `ctx.uri` set (Q#C8 scoping, the popup driver's path)
/// only that document's entries surface --- across all servers keyed
/// to it --- so a popup never shows another buffer's candidates.
/// Without a URI the legacy global drain across every cached
/// `(server_id, uri)` key applies. The registry's dedup collapses
/// identical entries; the prefix score ranks them.
/// far. Strictly scoped to `ctx.uri` (Q#C8): only that document's
/// entries surface, across all servers keyed to it. **No URI → no
/// LSP candidates** --- an unattached/scratch buffer must never show
/// another file's cached completions (the original global drain did
/// exactly that). The registry's dedup collapses identical entries;
/// the prefix score ranks them.
#[must_use]
pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn {
Box::new(move |ctx: &CompletionContext| -> Vec<CompletionItem> {
let Some(uri) = ctx.uri.clone() else {
return Vec::new();
};
let store_handle = {
let mgr = lsp.borrow();
mgr.completion_store()
@ -641,11 +644,7 @@ pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn
return Vec::new();
};
let mut out: Vec<CompletionItem> = Vec::new();
let keys: Vec<_> = store
.keys()
.filter(|k| ctx.uri.as_ref().is_none_or(|uri| k.uri == *uri))
.cloned()
.collect();
let keys: Vec<_> = store.keys().filter(|k| k.uri == uri).cloned().collect();
for key in keys {
for item in store.items(&key) {
out.push(item.clone());

View File

@ -578,8 +578,13 @@ impl EditorState {
// below, so typing keeps self-inserting and motion keys keep
// moving. The post-dispatch validation at the bottom of this
// function closes the session when a fallen-through key breaks
// the anchor/prefix invariant.
// the anchor/prefix invariant. A pending multi-key prefix owns
// the keyboard: while one is in flight (`C-x ...`) the popup
// must not steal its continuation or its `C-g` abort --- and
// the Pending arm below closes the popup anyway, so this guard
// only covers the same-dispatch race.
if self.core.borrow().completion_popup_is_open()
&& self.dispatcher.pending().is_empty()
&& let Some(key) = CompletionPopupKey::from_chord(chord)
{
self.dispatch_completion_key(key);
@ -616,7 +621,12 @@ impl EditorState {
}
Action::Pending { .. } => {
// The pending prefix is rendered from
// `dispatcher.pending()`; no command runs yet.
// `dispatcher.pending()`; no command runs yet. Starting
// a command sequence dismisses the completion popup:
// leaving it open would route the sequence's `C-g`
// abort (and its continuation chords) into the popup's
// shadow instead of the dispatcher.
self.core.borrow_mut().completion_popup_close();
}
Action::Unbound { sequence } => match printable_char(&sequence) {
Some(ch) => {

View File

@ -1842,7 +1842,11 @@ impl EditorCore {
/// Emptiness is enforced upstream:
/// [`crate::completion::CompletionPopupState::new`] refuses to build
/// a candidate-less session.
pub fn completion_popup_open(&mut self, state: crate::completion::CompletionPopupState) {
pub fn completion_popup_open(&mut self, mut state: crate::completion::CompletionPopupState) {
// Stamp the owning window (Lua publishers don't know window
// identity): only that window's overlay paints the popup, and
// a focus change invalidates the session.
state.window_id = Some(self.active_window_id());
*self
.completion_popup
.lock()
@ -1883,14 +1887,17 @@ impl EditorCore {
/// this the session is stale, not a prefix.
const MAX_PREFIX_BYTES: u64 = 512;
let (buffer_id, anchor) = {
let (buffer_id, window_id, anchor) = {
let guard = self
.completion_popup
.lock()
.expect("completion popup poisoned");
let p = guard.as_ref()?;
(p.buffer_id, p.anchor)
(p.buffer_id, p.window_id, p.anchor)
};
if window_id != Some(self.active_window_id()) {
return None; // focus moved to another window/split
}
if self.active_buffer_id() != buffer_id {
return None;
}
@ -1979,9 +1986,10 @@ impl EditorCore {
/// renders nothing while the popup is closed.
fn ensure_completion_overlay(&mut self) {
let popup = self.completion_popup.clone();
let wid = self.active_window_id();
let win = self.active_window_mut();
if !win.overlay_kinds().contains(&"completion-popup") {
win.push_overlay(Box::new(crate::completion::CompletionView::new(popup)));
win.push_overlay(Box::new(crate::completion::CompletionView::new(popup, wid)));
}
}
@ -3382,4 +3390,24 @@ mod tests {
assert_eq!(text_of(&s), "he world\n", "buffer untouched");
assert!(!s.completion_popup_is_open(), "stale accept still closes");
}
#[test]
fn completion_popup_validate_closes_on_window_focus_change() {
// Two splits on the SAME buffer: the session is window-scoped,
// so moving focus (buffer unchanged!) must invalidate it ---
// this is also what keeps the persistent overlay in the other
// split from painting a popup it doesn't own.
let mut s = from_bytes(b"he world\n");
s.split_active(Orientation::Horizontal, true);
s.active_window_mut().cursor = 2;
open_popup(&mut s, 0, "he", "hello");
s.completion_popup_validate();
assert!(s.completion_popup_is_open(), "session holds in its window");
s.focus_next();
s.completion_popup_validate();
assert!(
!s.completion_popup_is_open(),
"focus change closes the session even with the same buffer"
);
}
}

View File

@ -201,10 +201,68 @@ fn at_point_command_opens_below_threshold() {
assert_eq!(text, "hello_world hello_world");
}
/// The Q#C8 URI scoping: with `ctx.uri` set, the LSP provider only
/// surfaces the matching document's cached items; without it, the
/// legacy global drain applies. Driven through the Lua collect
/// surface against a hand-seeded store.
/// A multi-key prefix owns the keyboard: starting `C-x` while the
/// popup is open dismisses it, so the sequence's continuation (and a
/// `C-g` abort) reaches the dispatcher instead of the popup shadow.
#[test]
fn pending_prefix_dismisses_popup_and_keeps_dispatcher_control() {
let mut s = EditorState::new();
type_str(&mut s, "hello_world he");
let (_, visible, _) = probe(&s);
assert!(visible);
// C-x starts a prefix: the popup must close immediately...
s.dispatch_key(
FrontendId::LOCAL,
key(KeyCode::Char('x'), KeyModifiers::CONTROL),
);
let (text, visible, _) = probe(&s);
assert_eq!(text, "hello_world he", "the prefix key edits nothing");
assert!(!visible, "a pending prefix dismisses the popup");
// ...so this C-g aborts the prefix (not a popup), and typing
// afterwards self-inserts normally.
s.dispatch_key(
FrontendId::LOCAL,
key(KeyCode::Char('g'), KeyModifiers::CONTROL),
);
type_str(&mut s, "x");
let (text, _, _) = probe(&s);
assert_eq!(
text, "hello_world hex",
"after the aborted prefix, keys dispatch normally"
);
}
/// LSP-only words must still query the server: when the synchronous
/// providers return nothing at auto-open, a pending session is left
/// behind and the request fires anyway (previously the request was
/// gated on the popup having opened, so an empty dabbrev/snippet/
/// index sweep meant the server was never asked).
#[test]
fn empty_sync_sweep_still_leaves_a_pending_session() {
let mut s = EditorState::new();
// A buffer whose only word is the one being typed: dabbrev is
// structurally empty, no LSP attached. The popup cannot open...
type_str(&mut s, "qz");
let (_, visible, _) = probe(&s);
assert!(!visible, "nothing to show without providers");
// ...but the driver's session mirror must be pending rather than
// absent, so a (mocked) late LSP arrival could materialize it.
// With no attachment at all the request path no-ops; what we can
// assert end-to-end is that the state machine stays consistent:
// further typing neither crashes nor opens a bogus popup.
type_str(&mut s, "q");
let (text, visible, _) = probe(&s);
assert_eq!(text, "qzq");
assert!(!visible);
}
/// The Q#C8 URI plumbing: `ctx.uri` reaches Lua providers as the
/// ninth positional arg, so a URI-aware provider can scope itself
/// (the BUILT-IN LSP provider is stricter still: no uri → no rows).
/// Driven through the Lua collect surface with an emulating provider
/// because the real LSP store is Rust-side.
#[test]
fn collect_scopes_lsp_candidates_to_ctx_uri() {
let s = EditorState::new();

View File

@ -1,17 +1,25 @@
//! Worker-pool teardown regression: dropping an `EditorState` must
//! release its worker threads even though the `Rc<AsyncRuntime>` is
//! trapped in Lua-VM reference cycles and never reaches refcount
//! zero (`EditorState::drop` → `AsyncRuntime::shutdown_workers`).
//! zero (`EditorState::drop` → `AsyncRuntime::shutdown_workers`,
//! signal-only --- a join here deadlocks against workers blocked
//! handing replies to the main thread).
//!
//! Before the fix, every `EditorState` ever constructed leaked a
//! full `cores - 1` worker pool: the m4 acceptance suite (54 editor-
//! building tests) accumulated 1000+ live threads, each waking every
//! 100ms.
//!
//! The thread-count probe and the idempotence check share ONE test
//! function: they both build `EditorState`s, and as separate tests
//! libtest may run them concurrently, polluting the /proc-based
//! baseline on high-core machines.
use pmacs::editor::EditorState;
/// Thread-count probe via /proc; Linux-only (macOS CI skips --- the
/// leak and the fix are platform-independent, the *probe* isn't).
/// Thread-count probe via /proc; Linux-only (macOS CI runs the
/// non-Linux variant below --- the leak and the fix are platform-
/// independent, the *probe* isn't).
#[cfg(target_os = "linux")]
fn live_threads() -> usize {
std::fs::read_dir("/proc/self/task").map_or(0, std::iter::Iterator::count)
@ -19,14 +27,14 @@ fn live_threads() -> usize {
#[cfg(target_os = "linux")]
#[test]
fn editor_state_drop_releases_worker_threads() {
fn editor_state_drop_releases_workers_and_shutdown_is_idempotent() {
let baseline = live_threads();
for _ in 0..3 {
let s = EditorState::new();
drop(s);
}
// Joins are synchronous in drop; the small sleep only covers
// detached per-process reaper threads finishing up.
// Signal-only shutdown: parked workers exit within their 100ms
// park timeout; give them a beat.
std::thread::sleep(std::time::Duration::from_millis(300));
let after = live_threads();
assert!(
@ -34,10 +42,17 @@ fn editor_state_drop_releases_worker_threads() {
"worker threads leak across EditorState drop: \
baseline {baseline}, after 3 create/drop cycles {after}"
);
// Idempotence: explicit shutdown twice, then drop runs it a
// third time --- none may hang or panic.
let s = EditorState::new();
s.async_runtime.shutdown_workers();
s.async_runtime.shutdown_workers();
drop(s);
}
/// Platform-independent variant: the pool reports itself dead after
/// an explicit shutdown, and shutdown is idempotent.
/// Platform-independent idempotence check for hosts without /proc.
#[cfg(not(target_os = "linux"))]
#[test]
fn explicit_shutdown_is_idempotent() {
let s = EditorState::new();