fix(lsp): attribute a failing root resolver (COHERENCE §1.2)

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.
This commit is contained in:
Levi Neuwirth 2026-07-25 14:24:59 -04:00
parent 35085b54d1
commit ebcd2c4f6f
2 changed files with 121 additions and 3 deletions

View File

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

View File

@ -156,6 +156,10 @@ fn rows(state: &EditorState) -> Vec<String> {
}
}
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"))
);
}