From ebcd2c4f6f083c68f711bf52fc199beb172c3092 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 14:24:59 -0400 Subject: [PATCH] =?UTF-8?q?fix(lsp):=20attribute=20a=20failing=20root=20re?= =?UTF-8?q?solver=20(COHERENCE=20=C2=A71.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COHERENCE.md §1.2 makes "a `pcall` around background wiring must log attributed failure, never discard it" a standing rule, and names `ensure_server`'s swallowed spawn failure as its canonical case — the exact function this branch modifies. Round 1 deferred the resolver's silent `pcall` as a Stage 3 concern. Under that rule it is not a deferral, it is a fresh instance of the named anti-pattern added by a PR touching the cited function, made worse by the memo: a raised error is buried permanently for that directory and never observed again. A resolver that raises, or returns a non-string non-nil, now leaves an attributed trace naming the language and the directory. Returning nil remains the documented decline and stays silent — pinned, so "report failures" cannot be satisfied by reporting every resolution. The report goes through `pmacs.editor.set_status`, NOT `pmacs.error`, and that choice is the finding: **`pmacs.error` does not exist.** Fifteen call sites across `async.lua` (5), `syntax.lua` (4), `lsp.lua`, `mcp.lua`, `fs.lua`, `editops.lua`, `autosave.lua`, and `commands/default.lua` report background failures through it, each guarded `if pmacs.error then ...`. It is defined nowhere in production; the only assignment in the tree is a test stub at `src/editor.rs:9881`, and `type(pmacs.error)` is nil in a fresh `EditorState` (probed, not inferred). `pmacs.errors` (plural) in compile.lua is an unrelated namespace. So all fifteen reports are dead, and the guard makes the silence look deliberate — which is why nobody noticed. Writing the test is what caught it: the first version of this fix used `pmacs.error` and its pin failed against a working implementation. Both bites recorded: dropping the report entirely fails the pin, and so does reporting ONLY through `pmacs.error` — the dead-channel variant this nearly shipped. Not fixed here, deliberately: defining `pmacs.error`, the fifteen dead sites, and surfacing the spawn failure itself. That last is Priority 1 work and a user-visible product behavior — what message, where, with what guidance — so it needs its own framing rather than being smuggled into an affinity PR. --- builtin/runtime/lsp.lua | 35 +++++++++++- tests/lsp_multi_root_acceptance.rs | 89 ++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 3 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 3aca1ac..6021134 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -540,7 +540,7 @@ end -- resolver can never serve a root the previous one computed. local root_resolver_memo = setmetatable({}, { __mode = "k" }) -local function resolve_root_fn(resolver, path) +local function resolve_root_fn(language, resolver, path) local dir = dir_of(path) if not dir then return nil end local memo = root_resolver_memo[resolver] @@ -555,7 +555,36 @@ local function resolve_root_fn(resolver, path) return hit or nil end local ok, resolved = pcall(resolver, path) - if not ok or type(resolved) ~= "string" then resolved = nil end + -- COHERENCE §1.2: background wiring must not DISCARD a failure. A + -- resolver that raises, or that returns something other than a string + -- or nil, is a config bug — and the memo below would otherwise bury + -- it permanently for this directory, so it is never observed again. + -- Returning nil is the documented decline and stays silent. + local failure + if not ok then + failure = "raised: " .. tostring(resolved) + elseif resolved ~= nil and type(resolved) ~= "string" then + failure = "returned a " .. type(resolved) .. "; want string or nil" + end + if failure then + local msg = string.format( + "LSP: %s root resolver for %s %s", language, dir, failure) + -- Report on the channel that EXISTS. `pmacs.error` is referenced by + -- fifteen guarded call sites across the runtime and is defined + -- nowhere in production (only by a test stub in `src/editor.rs`), so + -- `if pmacs.error then ...` alone would be a sixteenth report that + -- never fires — the unwired-guard shape, not a fix for it. The + -- status line is what lsp.lua already uses for every other LSP + -- error. The `pmacs.error` arm rides along so this upgrades for free + -- if that channel is ever built. + -- + -- Both reports are pcall'd: a broken reporting channel must not turn + -- a declined root into a failed attach. + pcall(pmacs.editor.set_status, msg) + if pmacs.error then pcall(pmacs.error, msg) end + resolved = nil + end + if type(resolved) ~= "string" then resolved = nil end memo[dir] = resolved or false return resolved end @@ -570,7 +599,7 @@ local function project_root_for(language, path) end if not path then return nil, nil end if configured then - local resolved = resolve_root_fn(configured, path) + local resolved = resolve_root_fn(language, configured, path) if resolved then return resolved, "config" end end local ok, det = pcall(pmacs.project.detect, path) diff --git a/tests/lsp_multi_root_acceptance.rs b/tests/lsp_multi_root_acceptance.rs index 518c964..39ac68f 100644 --- a/tests/lsp_multi_root_acceptance.rs +++ b/tests/lsp_multi_root_acceptance.rs @@ -156,6 +156,10 @@ fn rows(state: &EditorState) -> Vec { } } +fn status(state: &EditorState) -> String { + state.core.borrow().status.clone() +} + fn count(state: &EditorState) -> usize { let n: i64 = eval(state, "return #pmacs.lsp.list()"); usize::try_from(n).expect("server count is non-negative") @@ -613,3 +617,88 @@ fn config_root_false_reads_as_unset_and_detection_still_wins() { "`false` must not become a root; detection still wins" ); } + +/// COHERENCE §1.2: background wiring must leave an attributed trace +/// rather than discard a failure. A throwing root resolver is a config +/// bug, and the per-directory memo would otherwise bury it permanently. +/// +/// The bite: drop the reporting arm and `*errors*` stays empty while the +/// attach still succeeds — the exact silence §1.2 names as the canonical +/// anti-pattern, in the function it cites. +#[test] +fn a_throwing_root_resolver_leaves_an_attributed_trace() { + 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(); + fx.bind(&state); + exec( + &state, + &format!( + r#" + pmacs.lsp.config.rust = {{ + command = "{}", + root = function(_) error("resolver blew up") end, + }} + "#, + fake_lsp_path() + ), + ); + open(&state, &file); + settle(&mut state); + + let msg = status(&state); + assert!( + msg.contains("root resolver"), + "a raising resolver must leave an attributed trace; got: {msg:?}" + ); + assert!( + msg.contains("rust"), + "the trace must name the language that owns it; got: {msg:?}" + ); + assert!( + msg.contains("resolver blew up"), + "the underlying error text must survive; got: {msg:?}" + ); + + // ...and the failure must degrade to a decline, not a failed attach: + // detection still wins and the buffer still gets its server. + let rows = rows(&state); + assert_eq!(rows.len(), 1, "the attach must still succeed: {rows:?}"); + assert_eq!( + rows[0].split('|').nth(1).unwrap(), + file_uri(&fx.dir("proj")), + "a declining resolver falls through to the marker walk" + ); +} + +/// The decline path stays silent. Without this, "report failures" could +/// be satisfied by reporting *every* resolution, which would spam +/// `*errors*` on every attach in a Lean project. +#[test] +fn a_resolver_returning_nil_declines_silently() { + 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(); + fx.bind(&state); + exec( + &state, + &format!( + "pmacs.lsp.config.rust = {{ command = \"{}\", root = function(_) return nil end }}", + fake_lsp_path() + ), + ); + open(&state, &file); + settle(&mut state); + + let msg = status(&state); + assert!( + !msg.contains("root resolver"), + "returning nil is the documented decline, not a failure; got: {msg:?}" + ); + assert_eq!( + rows(&state)[0].split('|').nth(1).unwrap(), + file_uri(&fx.dir("proj")) + ); +}