From 1ae5963e9d9a16b8228559b8d37359cf4ac4aeda Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 10:36:11 -0400 Subject: [PATCH 1/5] feat(lsp): one server per detected project root (Q#LN15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensure_server` reused any live server whose `language_id` matched, regardless of project root — its own comment documented this as a known post-v0.1 limitation. For project-model-strict servers that is a correctness failure, not a rough edge: `lake serve` is bound to one Lake package, rust-analyzer and gopls to one workspace, so the second project a user opens gets a server that cannot resolve its imports. Server affinity is now keyed on the project root, with one rule that keeps the change from regressing every other language: The affinity key is the root only when a root was actually FOUND. `project_root_for` never returns nil for a file that has a path — its last resort is the file's own directory — so a naive `(language_id, root)` key would give every directory of loose scratch files its own server, for every language: two stray .py files in different directories would spawn two pyrights where today they share one. It now returns `root, source` with source one of "config" / "detected" / "fallback", and only the first two become an affinity key. Matching is on the spawned spec's `root_uri`, nil matching nil, so the fallback spawn passes `root_uri = nil` for the key and the stored spec to agree. `cwd` still carries the directory, and `build_initialize` (src/lsp.rs) derives the identical `rootUri` from `cwd` when the field is None — using a percent-encoder with the same allowed set as Lua's `file_uri_for`. The initialize payload for that case is therefore byte-identical to before; only what the reuse loop matches on changes. `build_initialize` is the only reader of `spec.root_uri` in the tree. Two consequences, both deliberate and both asserted rather than discovered: - A server hand-spawned from init.lua with only `cwd` set also reads back nil, so a root-bearing attach will not adopt it. We cannot know which root it was meant to serve, and guessing wrongly routes a project's files to the wrong server. - Opening files across N project roots spawns N servers. rust-analyzer has the same property and no editor caps it by default; `pmacs.lsp.stop` is the manual escape and an LRU reaping policy stays deferred. `config[language].root` may now be a `function(path) -> string|nil` as well as a string, for languages whose root rule the shared marker walk cannot express — an innermost-wins walk cannot find an *outermost* marker. A resolver returning nil declines and falls through to the marker walk. Results are memoized per directory because hoisting the root computation above the reuse loop puts it on every attach rather than every spawn; the memo is keyed weakly by the resolver function itself, so replacing `config[lang].root` cannot serve a root the old one computed. `pmacs.lsp.list()` rows gain `root_uri` and `cwd`. `root_uri` is the spec field verbatim, deliberately not the URI the server was initialized with. No protocol change. No Lean content: this is the shared affinity function for every LSP language, so it ships as its own PR and is exercised through rust, python, go and typescript against `pmacs_fake_lsp`. tests/lsp_multi_root_acceptance.rs covers acceptance 13-21. Every fixture sets `pmacs.project.set_search_boundary` at its own tempdir root: without it the marker walk climbs to the filesystem root, and a stray `.git` above the temp directory would turn the markerless cases into detected ones — the assertions would still pass while testing nothing. Refs docs/lean4-mode-framing.md Q#LN15, acceptance 13-21. --- builtin/runtime/lsp.lua | 108 +++++- src/lua_bindings/mod.rs | 16 +- tests/lsp_multi_root_acceptance.rs | 511 +++++++++++++++++++++++++++++ 3 files changed, 618 insertions(+), 17 deletions(-) create mode 100644 tests/lsp_multi_root_acceptance.rs diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 4181156..07d0aeb 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -30,9 +30,13 @@ pmacs.lsp = pmacs.lsp or {} -- env (table) extra environment -- init_options (table) `initializationOptions` -- settings (table) answered to `workspace/configuration` --- root (string) optional explicit project root; overrides +-- root (string|function) optional explicit project root; overrides -- the `pmacs.project.detect` marker walk used --- to set `rootUri`/`cwd` (see project_root_for) +-- to set `rootUri`/`cwd`. A `function(path) -> +-- string|nil` is resolved per file and +-- memoized per directory; returning nil +-- declines and falls through to the marker +-- walk (see project_root_for) pmacs.lsp.config = pmacs.lsp.config or {} -- Default rust-analyzer config. Users replace any field from init.lua @@ -507,34 +511,106 @@ end -- the rest of the editor uses, honoring set_search_boundary, -- 3. the file's own directory (a lone file still gets a sane root -- rather than leaking the editor cwd). --- This is single-root: it fixes which root the one per-language server --- uses, NOT one-server-per-root scoping (still deferred post-v0.1). +-- Returns `root, source`, where `source` is "config", "detected", or +-- "fallback" — and nil alongside a nil root. The source matters because +-- only the first two mean a root was actually *found*; `ensure_server` +-- keys server affinity on those and treats the fallback as rootless. +-- +-- `config[language].root` may be a `function(path) -> string|nil` as +-- well as a plain string, for languages whose root rule the shared +-- marker walk cannot express (an innermost-wins walk cannot find an +-- *outermost* marker). A resolver that returns nil declines, and +-- resolution falls through to the marker walk. +-- +-- Resolver results are memoized per directory, because `ensure_server` +-- resolves the root on the *reuse* path as well as the spawn path — so +-- an unmemoized filesystem-walking resolver would re-walk on every +-- attach rather than once per project. The memo is keyed by the +-- resolver function itself, weakly: replacing `config[lang].root` +-- installs a new key and the old memo is collected, so a swapped +-- 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 dir = dir_of(path) + if not dir then return nil end + local memo = root_resolver_memo[resolver] + if not memo then + memo = {} + root_resolver_memo[resolver] = memo + end + local hit = memo[dir] + -- `false` is the memoized form of "this resolver declined"; nil means + -- "not yet asked", so the two must stay distinguishable. + if hit ~= nil then + return hit or nil + end + local ok, resolved = pcall(resolver, path) + if not ok or type(resolved) ~= "string" then resolved = nil end + memo[dir] = resolved or false + return resolved +end + local function project_root_for(language, path) local cfg = pmacs.lsp.config[language] - if cfg and cfg.root then return cfg.root end - if not path then return nil end + local configured = cfg and cfg.root + -- Truthiness, not `~= nil`: `root = false` has always read as "unset", + -- and a `false` leaking through as a root would reach `file_uri_for`. + if configured and type(configured) ~= "function" then + return configured, "config" + end + if not path then return nil, nil end + if configured then + local resolved = resolve_root_fn(configured, path) + if resolved then return resolved, "config" end + end local ok, det = pcall(pmacs.project.detect, path) - if ok and det and det.root then return det.root end - return dir_of(path) + if ok and det and det.root then return det.root, "detected" end + return dir_of(path), "fallback" end local function ensure_server(language, path) local cfg = pmacs.lsp.config[language] if not cfg or not cfg.command then return nil end - -- Reuse an existing same-language server if one is up. Multi-root - -- scoping (one server per project root) ships post-v0.1, so the - -- first file that attaches a given language fixes that server's - -- root; later files of the same language reuse it regardless of - -- their own project (known, documented limitation). + -- Reuse an existing same-language server *serving the same root*. + -- One server per project root: `lake serve` is bound to one Lake + -- package and rust-analyzer/gopls to one workspace, so handing the + -- second project's files to the first project's server yields + -- unresolvable imports and empty diagnostics. + -- + -- The affinity key is the root only when a root was actually FOUND + -- (config override or marker walk). `project_root_for` never returns + -- nil for a file that has a path — its last resort is the file's own + -- directory — so keying on the fallback would give every directory + -- of loose scratch files its own server, for every language: two + -- stray .py files in different directories would spawn two pyrights + -- where today they share one. The fallback therefore keys on nil. + -- + -- Matching is on the spawned spec's `root_uri`, nil matching nil, so + -- the fallback spawn must pass `root_uri = nil` for the key and the + -- stored spec to agree. `cwd` still carries the directory and + -- `build_initialize` derives the identical `rootUri` from it when the + -- field is None (src/lsp.rs), so the initialize payload is unchanged + -- for that case — only what this loop matches on changes. + -- + -- Consequence, deliberate: a server hand-spawned from `init.lua` with + -- only `cwd` set also reads back nil, so a root-bearing attach will + -- not adopt it. We cannot know which root it was meant to serve, and + -- guessing wrongly routes a project's files to the wrong server. + local root, source = project_root_for(language, path) + local key_uri = nil + if source == "config" or source == "detected" then + key_uri = file_uri_for(root) + end for _, info in ipairs(pmacs.lsp.list()) do - if info.language_id == language and info.state then + if info.language_id == language and info.state + and info.root_uri == key_uri then local kind = info.state.kind if kind ~= "crashed" and kind ~= "stopped" then return info.id end end end - local root = project_root_for(language, path) local ok, sid = pcall(pmacs.lsp.spawn, { label = "default-" .. language, language_id = language, @@ -544,7 +620,7 @@ local function ensure_server(language, path) init_options = cfg.init_options, settings = cfg.settings, cwd = root, - root_uri = root and file_uri_for(root) or nil, + root_uri = key_uri, }) if ok then return sid end return nil diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 29e3b47..3a92520 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -9923,12 +9923,26 @@ pub fn install_lsp( let ids: Vec = mgr.ids().collect(); let out = lua.create_table_with_capacity(ids.len(), 0)?; for (i, id) in ids.iter().enumerate() { - let row = lua.create_table_with_capacity(0, 5)?; + let row = lua.create_table_with_capacity(0, 7)?; row.set("id", LspServerIdLua(*id))?; if let Some(spec) = mgr.spec(*id) { row.set("label", spec.label.as_str())?; row.set("language_id", spec.language_id.as_str())?; row.set("command", spec.command.as_str())?; + // Server *affinity* fields. `root_uri` is the spec + // field verbatim — deliberately NOT the URI the + // server was initialized with, which `build_initialize` + // derives from `cwd` when the field is `None`. Lua's + // `ensure_server` matches on this exact value, so a + // server that never asked for a specific root must + // read back as nil rather than as its cwd; see the + // affinity-key comment in `builtin/runtime/lsp.lua`. + if let Some(root_uri) = spec.root_uri.as_deref() { + row.set("root_uri", root_uri)?; + } + if let Some(cwd) = spec.cwd.as_deref() { + row.set("cwd", cwd.display().to_string())?; + } } if let Some(state) = mgr.state(*id) { row.set("state", lsp_state_to_lua(lua, state)?)?; diff --git a/tests/lsp_multi_root_acceptance.rs b/tests/lsp_multi_root_acceptance.rs new file mode 100644 index 0000000..b354283 --- /dev/null +++ b/tests/lsp_multi_root_acceptance.rs @@ -0,0 +1,511 @@ +//! Arc 8 Stage 2 acceptance — multi-root LSP server affinity. +//! +//! `docs/lean4-mode-framing.md` Q#LN15, acceptance 13–21. +//! +//! This suite deliberately contains **no Lean content**. `ensure_server` +//! (`builtin/runtime/lsp.lua`) is the single server-affinity function for +//! every LSP language in pmacs, so the change is exercised through the +//! four languages that already shipped attach paths — rust, python, go, +//! typescript — driven against `pmacs_fake_lsp` so nothing here needs a +//! real toolchain on PATH. +//! +//! Every fixture calls `pmacs.project.set_search_boundary` at its own +//! tempdir root. Without it the marker walk climbs to the filesystem +//! root, and a stray `.git` above the temp directory would silently turn +//! the "markerless" cases into detected ones — the assertions would still +//! pass while testing nothing. + +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use pmacs::editor::EditorState; + +fn exec(state: &EditorState, source: &str) { + state.lua_host.lua().load(source.to_owned()).exec().unwrap(); +} + +fn eval(state: &EditorState, source: &str) -> T { + state.lua_host.lua().load(source.to_owned()).eval().unwrap() +} + +fn fake_lsp_path() -> String { + env!("CARGO_BIN_EXE_pmacs_fake_lsp").to_owned() +} + +/// A fresh editor with the shipped language configs cleared, so the only +/// server any test can spawn is the fake one it configures itself. +fn editor() -> EditorState { + let state = EditorState::new(); + exec(&state, "pmacs.lsp.config = {}"); + state +} + +fn lua_str(path: &Path) -> String { + path.display() + .to_string() + .replace('\\', "\\\\") + .replace('"', "\\\"") +} + +/// Mirror of `file_uri_for` in `builtin/runtime/lsp.lua` and +/// `path_to_file_uri` in `src/lsp.rs`. 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 ch in path.display().to_string().chars() { + match ch { + 'a'..='z' | 'A'..='Z' | '0'..='9' | '/' | '-' | '_' | '.' | '~' | ':' => out.push(ch), + _ => { + use std::fmt::Write as _; + let mut buf = [0u8; 4]; + for byte in ch.encode_utf8(&mut buf).as_bytes() { + let _ = write!(out, "%{byte:02X}"); + } + } + } + } + out +} + +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +impl Fixture { + /// Canonicalized so the expected roots below compare equal to what + /// `pmacs.project.detect` returns (it canonicalizes before walking, + /// which matters on macOS where `/var` is a symlink to `/private/var`). + 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 { + self.root.join(rel) + } + + fn bind(&self, state: &EditorState) { + exec( + state, + &format!( + "pmacs.project.set_search_boundary(\"{}\")", + lua_str(&self.root) + ), + ); + } +} + +fn configure(state: &EditorState, language: &str) { + exec( + state, + &format!( + "pmacs.lsp.config.{language} = {{ command = \"{}\" }}", + fake_lsp_path() + ), + ); +} + +fn open(state: &EditorState, path: &Path) { + exec( + state, + &format!("pmacs.buffer.find_or_open(\"{}\")", lua_str(path)), + ); +} + +fn settle(state: &mut EditorState) { + for _ in 0..8 { + state.tick_processes(); + state.tick_lsp(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// One `language_id|root_uri|cwd|state` row per live server, sorted so +/// assertions do not depend on spawn order. Absent fields read as "". +fn 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.cwd 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 count(state: &EditorState) -> usize { + let n: i64 = eval(state, "return #pmacs.lsp.list()"); + usize::try_from(n).expect("server count is non-negative") +} + +// --------------------------------------------------------------------------- +// Acceptance 13 — `lsp.list()` rows carry `root_uri` and `cwd`. +// --------------------------------------------------------------------------- + +#[test] +fn acc13_list_rows_carry_root_uri_and_cwd() { + 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); + configure(&state, "rust"); + open(&state, &file); + settle(&mut state); + + let proj = fx.dir("proj"); + let rows = rows(&state); + assert_eq!(rows.len(), 1, "{rows:?}"); + let fields: Vec<&str> = rows[0].split('|').collect(); + assert_eq!(fields[0], "rust"); + assert_eq!(fields[1], file_uri(&proj), "root_uri must be the project root"); + assert_eq!(fields[2], proj.display().to_string(), "cwd must be the root"); +} + +// --------------------------------------------------------------------------- +// Acceptance 14 — two roots, same language, two servers. +// --------------------------------------------------------------------------- + +#[test] +fn acc14_two_project_roots_of_one_language_spawn_two_servers() { + let fx = Fixture::new(); + fx.write("a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("b/Cargo.toml", "[package]\nname = \"b\"\n"); + let first = fx.write("a/src/main.rs", "fn main() {}\n"); + let second = fx.write("b/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + open(&state, &first); + settle(&mut state); + open(&state, &second); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 2, "one server per project root: {rows:?}"); + let roots: Vec<&str> = rows.iter().map(|r| r.split('|').nth(1).unwrap()).collect(); + assert!(roots.contains(&file_uri(&fx.dir("a")).as_str()), "{roots:?}"); + assert!(roots.contains(&file_uri(&fx.dir("b")).as_str()), "{roots:?}"); +} + +// --------------------------------------------------------------------------- +// Acceptance 15 — same root, two files, one server. The pre-change +// behavior, pinned so the fix cannot degrade into "always spawn". +// --------------------------------------------------------------------------- + +#[test] +fn acc15_two_files_in_one_root_reuse_a_single_server() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let first = fx.write("proj/src/main.rs", "fn main() {}\n"); + let second = fx.write("proj/src/other.rs", "pub fn other() {}\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + open(&state, &first); + settle(&mut state); + open(&state, &second); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 1, "same root must reuse: {rows:?}"); + assert_eq!(rows[0].split('|').nth(1).unwrap(), file_uri(&fx.dir("proj"))); +} + +// --------------------------------------------------------------------------- +// Acceptance 16 — per-language regression pin. The single-root case is +// all the shipped attach paths ever exercised; it must be untouched. +// --------------------------------------------------------------------------- + +#[test] +fn acc16_shipped_languages_are_unchanged_for_the_single_root_case() { + // (language id, project marker, two source files under it) + let cases: [(&str, &str, &str, &str); 4] = [ + ("rust", "Cargo.toml", "one.rs", "two.rs"), + ("python", "pyproject.toml", "one.py", "two.py"), + ("go", "go.mod", "one.go", "two.go"), + ("typescript", "package.json", "one.ts", "two.ts"), + ]; + for (language, marker, first_name, second_name) in cases { + let fx = Fixture::new(); + fx.write(&format!("proj/{marker}"), "{}\n"); + let first = fx.write(&format!("proj/src/{first_name}"), "\n"); + let second = fx.write(&format!("proj/src/{second_name}"), "\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, language); + open(&state, &first); + settle(&mut state); + open(&state, &second); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 1, "{language}: expected one server, got {rows:?}"); + let fields: Vec<&str> = rows[0].split('|').collect(); + assert_eq!(fields[0], language, "{language}: language_id"); + assert_eq!( + fields[1], + file_uri(&fx.dir("proj")), + "{language}: root must be the marker directory" + ); + } +} + +// --------------------------------------------------------------------------- +// Acceptance 17 — hoist pin. `project_root_for` now runs on the *reuse* +// path, and a function-valued `root` is memoized per directory. +// --------------------------------------------------------------------------- + +#[test] +fn acc17_function_root_runs_on_the_reuse_path_and_memoizes_per_directory() { + let fx = Fixture::new(); + let shared = fx.dir("shared"); + std::fs::create_dir_all(&shared).unwrap(); + let a1 = fx.write("one/a.rs", "fn a() {}\n"); + let a2 = fx.write("one/b.rs", "fn b() {}\n"); + let b1 = fx.write("two/c.rs", "fn c() {}\n"); + let mut state = editor(); + fx.bind(&state); + // A resolver that answers the same root for every directory: the + // second directory therefore REUSES the first directory's server, + // which is exactly the path the hoist put the resolver on. + exec( + &state, + &format!( + r#" + _G.ROOT_CALLS = 0 + pmacs.lsp.config.rust = {{ + command = "{}", + root = function(_) + _G.ROOT_CALLS = _G.ROOT_CALLS + 1 + return "{}" + end, + }} + "#, + fake_lsp_path(), + lua_str(&shared) + ), + ); + + open(&state, &a1); + settle(&mut state); + assert_eq!(eval::(&state, "return _G.ROOT_CALLS"), 1, "spawn path"); + + // Same directory: served from the memo, so the count does not move. + open(&state, &a2); + settle(&mut state); + assert_eq!( + eval::(&state, "return _G.ROOT_CALLS"), + 1, + "second file in the same directory must hit the memo" + ); + + // Different directory: the resolver runs again — proving the reuse + // path resolves at all — but resolves to the same root, so no second + // server appears. + open(&state, &b1); + settle(&mut state); + assert_eq!( + eval::(&state, "return _G.ROOT_CALLS"), + 2, + "a new directory must consult the resolver on the reuse path" + ); + let rows = rows(&state); + assert_eq!(rows.len(), 1, "one resolved root, one server: {rows:?}"); + assert_eq!(rows[0].split('|').nth(1).unwrap(), file_uri(&shared)); +} + +// --------------------------------------------------------------------------- +// Acceptance 18 — a hand-spawned server carrying only `cwd` is not +// adopted by a root-bearing attach. A deliberate behavior change. +// --------------------------------------------------------------------------- + +#[test] +fn acc18_hand_spawned_server_without_root_uri_is_not_adopted() { + 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 proj = fx.dir("proj"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + // Exactly what an init.lua would write: cwd, no root_uri. + exec( + &state, + &format!( + r#" + pmacs.lsp.spawn({{ + label = "hand-rolled", + language_id = "rust", + command = "{}", + cwd = "{}", + }}) + "#, + fake_lsp_path(), + lua_str(&proj) + ), + ); + settle(&mut state); + assert_eq!(count(&state), 1, "the hand-spawned server is up"); + + open(&state, &file); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 2, "the attach must not adopt it: {rows:?}"); + let roots: Vec<&str> = rows.iter().map(|r| r.split('|').nth(1).unwrap()).collect(); + assert!(roots.contains(&""), "hand-spawned reads back nil: {roots:?}"); + assert!( + roots.contains(&file_uri(&proj).as_str()), + "the attach's own server carries the root: {roots:?}" + ); +} + +// --------------------------------------------------------------------------- +// Acceptance 19 — a dead server in the matching root is not reused. +// --------------------------------------------------------------------------- + +#[test] +fn acc19_stopped_server_in_the_matching_root_is_not_reused() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let first = fx.write("proj/src/main.rs", "fn main() {}\n"); + let second = fx.write("proj/src/other.rs", "pub fn other() {}\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + open(&state, &first); + settle(&mut state); + let original: i64 = eval(&state, "return pmacs.lsp.list()[1].id:raw()"); + + exec(&state, "pmacs.lsp.stop(pmacs.lsp.list()[1].id)"); + for _ in 0..200 { + settle(&mut state); + let dead: bool = eval( + &state, + r#" + for _, s in ipairs(pmacs.lsp.list()) do + local k = s.state and s.state.kind + if k == "stopped" or k == "crashed" then return true end + end + return false + "#, + ); + if dead { + break; + } + } + + open(&state, &second); + settle(&mut state); + let live: i64 = eval( + &state, + r#" + for _, s in ipairs(pmacs.lsp.list()) do + local k = s.state and s.state.kind + if k ~= "stopped" and k ~= "crashed" then + return s.id:raw() + end + end + return -1 + "#, + ); + assert_ne!(live, -1, "a replacement server must exist"); + assert_ne!(live, original, "the dead server must not be reused"); +} + +// --------------------------------------------------------------------------- +// Acceptance 20 — the loose-file pin (Q#LN15 part 2). This is the +// no-change case, and the one a naive `(language_id, root)` key breaks. +// --------------------------------------------------------------------------- + +#[test] +fn acc20_markerless_files_in_different_directories_share_one_server() { + let fx = Fixture::new(); + let first = fx.write("loose_a/one.rs", "fn one() {}\n"); + let second = fx.write("loose_b/two.rs", "fn two() {}\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + open(&state, &first); + settle(&mut state); + open(&state, &second); + settle(&mut state); + + let rows = rows(&state); + assert_eq!( + rows.len(), + 1, + "loose files must keep sharing one server: {rows:?}" + ); + let fields: Vec<&str> = rows[0].split('|').collect(); + assert_eq!(fields[1], "", "the fallback root is not an affinity key"); + assert_eq!( + fields[2], + fx.dir("loose_a").display().to_string(), + "cwd still carries the first file's directory" + ); +} + +// --------------------------------------------------------------------------- +// Acceptance 21 — detected and fallback are different servers, and the +// fallback one still carries its directory as `cwd`. +// --------------------------------------------------------------------------- + +#[test] +fn acc21_detected_root_and_markerless_file_get_different_servers() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let inside = fx.write("proj/src/main.rs", "fn main() {}\n"); + let loose = fx.write("loose/stray.rs", "fn stray() {}\n"); + let mut state = editor(); + fx.bind(&state); + configure(&state, "rust"); + open(&state, &inside); + settle(&mut state); + open(&state, &loose); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 2, "detected and fallback must differ: {rows:?}"); + let detected = rows + .iter() + .find(|r| r.split('|').nth(1).unwrap() == file_uri(&fx.dir("proj"))) + .unwrap_or_else(|| panic!("no server rooted at the project: {rows:?}")); + assert_eq!( + detected.split('|').nth(2).unwrap(), + fx.dir("proj").display().to_string() + ); + let fallback = rows + .iter() + .find(|r| r.split('|').nth(1).unwrap().is_empty()) + .unwrap_or_else(|| panic!("no rootless server: {rows:?}")); + assert_eq!( + fallback.split('|').nth(2).unwrap(), + fx.dir("loose").display().to_string(), + "the markerless server keeps the fallback directory as cwd" + ); +} From 92f57d8894366c4c4b478bf71ba824a3591dc043 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 11:03:54 -0400 Subject: [PATCH 2/5] docs: record Stage 1's landing and the Stage 2 affinity lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 1 merged as #160 (`main` @ `0827dd1`); the Lean lane header and branch line now say so, and Stage 2 gets its own subsection. Edits stay inside the Lean lane. PR #156 is still open against both this file and `docs/agent-handoff.md`, and it rewrites the snapshot header, the canonical-base line, and the whole bottom-panel lane — so those are left alone rather than merged twice. `agent-handoff.md` is untouched for the same reason plus its own: §1 describes what is on `main`, so it updates at merge, not during review. Records the one finding this stage turned up but did not fix: `ensure_server` never forwards `cfg.restart` to `pmacs.lsp.spawn`, so a `restart` in `pmacs.lsp.config[lang]` is silently dropped on the auto-attach path. Pre-existing, and out of scope for a PR whose acceptance 16 pins existing attach behavior as unchanged. --- docs/active-work.md | 70 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 13 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index f55627e..5bacaa1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -54,10 +54,11 @@ git status --short --branch The `git log` command must expose `0dd16a5` or a newer intentional main. If it does not, stop and repair the remote/fetch configuration. -## Lean 4 lane (Arc 8) — Stage 1 IN REVIEW (PR #160) +## Lean 4 lane (Arc 8) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #TBD) -- Portable branch: `githubsucks/lean4-stage1`, worked in the shared - checkout (no sibling worktree), based on `githubsucks/main` @ `e745068`. +- Stage 1 **merged as #160** (`main` @ `0827dd1`, 2026-07-25, one review + round, all twelve checks green). Branch `githubsucks/lean4-stage1` + retained; it was worked in the shared checkout (no sibling worktree). - Approved framing: `docs/lean4-mode-framing.md` revision 4, committed as the branch's first commit (`a382965`) after three review rounds. **Seven stages**, 19 decisions (Q#LN1–19), 64 acceptance criteria. North star: @@ -113,16 +114,59 @@ If it does not, stop and repair the remote/fetch configuration. 152; **isolated-config workspace sweep 3,150 across 90 suites**; `git diff --check` clean. The sweep needs an isolated `XDG_CONFIG_HOME` for the reason recorded in the bottom-panel lane below. -- **Stage 2 is multi-root LSP server affinity** — pure substrate, no Lean - content, and it changes `ensure_server`, which every LSP language - shares. It is sequenced next because Lean is the language that makes its - absence a correctness failure rather than an inconvenience. Two - corrections the framing already carries for it: `root` is computed at - `lsp.lua:537`, **after** the reuse loop, so the fix must hoist it; and - `project_root_for` never returns nil for a file with a path, so the - affinity key must be the root only when a root was actually *detected*, - or markerless scratch files fragment into one server per directory for - every language. +### Stage 2 — multi-root LSP server affinity (Q#LN15) + +- Portable branch: `githubsucks/lsp-multi-root-affinity`, shared checkout, + based on `githubsucks/main` @ `0827dd1`. Named for the substrate, not + for Lean: **the diff contains no Lean content**, because `ensure_server` + is the one server-affinity function every LSP language shares and a + cross-cutting change to it must not be reviewable only as a Lean + feature. +- Three files, no protocol change: `src/lua_bindings/mod.rs` (the + `lsp.list()` row builder gains `root_uri` + `cwd`), + `builtin/runtime/lsp.lua` (`project_root_for` returns `root, source`; + `ensure_server` hoists it above the reuse loop and matches on it), + `tests/lsp_multi_root_acceptance.rs` (9 tests, acceptance 13–21). +- **The rule that keeps this from regressing every other language: the + affinity key is the root only when a root was actually FOUND.** + `project_root_for` never returns nil for a file with a path — its last + resort is the file's own directory — so a naive `(language_id, root)` + key gives every directory of loose scratch files its own server, for + every language. `source` is `"config" | "detected" | "fallback"` and + only the first two become a key. +- **Wire-identical for the fallback case, and that is provable rather + than hoped.** Matching is on the spawned spec's `root_uri` (nil matching + nil), so the fallback spawn passes `root_uri = nil`; `cwd` still carries + the directory and `build_initialize` derives the identical `rootUri` + from `cwd` when the field is None, using a percent-encoder with the same + allowed set as Lua's `file_uri_for`. `build_initialize` (`src/lsp.rs`) + is the **only** reader of `spec.root_uri` in the tree. +- Deliberate behavior change, asserted not discovered: a server + hand-spawned from `init.lua` with only `cwd` set also reads back nil, so + a root-bearing attach will not adopt it. +- `config[language].root` may now be a `function(path) -> string|nil`, + memoized per directory — needed because the hoist puts root resolution + on every attach rather than every spawn. The memo is keyed **weakly by + the resolver function itself**, so replacing `config[lang].root` cannot + serve a root the previous resolver computed. This is Q#LN8's + generalization landing early; the Lean resolver that uses it is Stage 3. +- Bite-verified three ways: 5/9 fail against the pre-change `lsp.lua`, + 8/9 against the pre-change `mod.rs`, and — the one that matters most — + installing the naive always-key-on-root variant fails acceptance 20 and + 21 exactly as Q#LN15 part 2 predicts. The four that survive the first + bite (13, 15, 16, 19) are the regression pins; passing on both sides is + their job. +- Every fixture sets `pmacs.project.set_search_boundary` at its own + tempdir root. Without it the marker walk climbs to the filesystem root + and a stray `.git` above the temp directory turns the markerless cases + into detected ones — the assertions would still pass while testing + nothing. +- **Found but not fixed here (pre-existing, own lane):** `ensure_server` + never forwards `cfg.restart` to `pmacs.lsp.spawn`, so a + `restart = "never"` in `pmacs.lsp.config[lang]` is silently dropped on + the auto-attach path. At least one existing test sets it believing it + takes effect. Out of scope for a PR whose acceptance 16 pins existing + attach behavior as unchanged. ## Bottom-panel lane (window placement + side windows) — Stage 1 IN REVIEW From 35085b54d1f24f35759390859f72f80d95180e54 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 11:19:43 -0400 Subject: [PATCH 3/5] fix: rustfmt the acceptance suite and pin two untested arms (round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocker was process, not design. The test file was committed before `cargo fmt` ran, so the reflow of five over-width assertions sat uncommitted in the working tree while the branch as pushed failed the first gate in CLAUDE.md. The "fmt clean" reported on the PR described the worktree, not the branch. Gate results are only meaningful run against the pushed tree, so this commit lands the formatting first and the gates are re-run against it. Two pins review asked for, each covering a branch the nine acceptance tests left untested: - A **string** `config.root` as an affinity key. acc17 covers only the function form, so `return configured, "config"` had no test. The bite puts both files in their own marked project: drop the config arm and they key on their own detected roots and spawn two servers, so one server on the configured root is only reachable if the override wins. - `root = false` reads as unset. Defended by a truthiness check rather than `~= nil`, previously by comment alone. Under `~= nil` the config arm returns `false, "config"` and `file_uri_for(false)` returns nil, so the file lands on a rootless server instead of its detected project. Each was falsified against exactly the mutation it targets and neither against the other. Also documents an asymmetry review caught: `project_root_for`'s "detected" arm is canonicalized for free because `pmacs.project.detect` canonicalizes before walking, but a **configured** root — string or resolver return — is fed to `file_uri_for` exactly as written, and the affinity key is that URI. On macOS a resolver returning `/var/…` and a detected `/private/var/…` are therefore different keys for one directory, silently yielding two servers for one project. There is no Lua-side canonicalizer to normalize it, and Stage 3's Lean resolver is the first real consumer, so the obligation is stated in the `config.root` doc comment where that resolver's author will read it. --- builtin/runtime/lsp.lua | 9 +++ docs/active-work.md | 22 +++++- tests/lsp_multi_root_acceptance.rs | 118 +++++++++++++++++++++++++++-- 3 files changed, 141 insertions(+), 8 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 07d0aeb..3aca1ac 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -522,6 +522,15 @@ end -- *outermost* marker). A resolver that returns nil declines, and -- resolution falls through to the marker walk. -- +-- **A configured root — string or resolver return — MUST be a canonical +-- absolute path.** The `"detected"` arm is canonicalized for free +-- (`pmacs.project.detect` canonicalizes before walking), but a +-- configured one is fed to `file_uri_for` exactly as written, and the +-- affinity key is that URI. On macOS a resolver returning `/var/…` and +-- a detected `/private/var/…` are different keys for the same +-- directory, which silently yields two servers for one project. There +-- is no Lua-side canonicalizer to normalize this for you. +-- -- Resolver results are memoized per directory, because `ensure_server` -- resolves the root on the *reuse* path as well as the spawn path — so -- an unmemoized filesystem-walking resolver would re-walk on every diff --git a/docs/active-work.md b/docs/active-work.md index 5bacaa1..c3fd5f9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -54,7 +54,7 @@ git status --short --branch The `git log` command must expose `0dd16a5` or a newer intentional main. If it does not, stop and repair the remote/fetch configuration. -## Lean 4 lane (Arc 8) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #TBD) +## Lean 4 lane (Arc 8) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #161) - Stage 1 **merged as #160** (`main` @ `0827dd1`, 2026-07-25, one review round, all twelve checks green). Branch `githubsucks/lean4-stage1` @@ -167,6 +167,26 @@ If it does not, stop and repair the remote/fetch configuration. the auto-attach path. At least one existing test sets it believing it takes effect. Out of scope for a PR whose acceptance 16 pins existing attach behavior as unchanged. +- **Review round 1 addressed.** The blocker was process, not design: the + test file was committed *before* `cargo fmt` ran, so the fix sat + uncommitted in the working tree and the branch as pushed failed the + first gate. The reported "fmt clean" described the worktree, not the + branch — gate results are only meaningful when run against the pushed + tree. Also added the two pins review asked for (a **string** `config + .root` as an affinity key — acc17 only covered the function form; and + `root = false` reading as unset), each bite-verified against exactly + the mutation it targets and neither against the other. And documented + the canonicalization obligation: the `"detected"` arm is canonicalized + for free, a **configured** root is not, so on macOS a resolver + returning `/var/…` and a detected `/private/var/…` are different keys + for one directory. Stage 3's Lean resolver is the first real consumer, + so the obligation is written at the point of use. +- Verification on this branch: `cargo fmt --check` clean; strict + workspace Clippy clean; 1,826 default + 2,003 CRDT library tests; + multi-root 11/11; M4 121; statusline 7; completion popup 9; auto-pair + 45; required GPU 155; **isolated-config workspace sweep 3,164 across 91 + suites**; `git diff --check` clean. The sweep needs an isolated + `XDG_CONFIG_HOME` and `-- --skip basedpyright`. ## Bottom-panel lane (window placement + side windows) — Stage 1 IN REVIEW diff --git a/tests/lsp_multi_root_acceptance.rs b/tests/lsp_multi_root_acceptance.rs index b354283..518c964 100644 --- a/tests/lsp_multi_root_acceptance.rs +++ b/tests/lsp_multi_root_acceptance.rs @@ -181,8 +181,16 @@ fn acc13_list_rows_carry_root_uri_and_cwd() { assert_eq!(rows.len(), 1, "{rows:?}"); let fields: Vec<&str> = rows[0].split('|').collect(); assert_eq!(fields[0], "rust"); - assert_eq!(fields[1], file_uri(&proj), "root_uri must be the project root"); - assert_eq!(fields[2], proj.display().to_string(), "cwd must be the root"); + assert_eq!( + fields[1], + file_uri(&proj), + "root_uri must be the project root" + ); + assert_eq!( + fields[2], + proj.display().to_string(), + "cwd must be the root" + ); } // --------------------------------------------------------------------------- @@ -207,8 +215,14 @@ fn acc14_two_project_roots_of_one_language_spawn_two_servers() { let rows = rows(&state); assert_eq!(rows.len(), 2, "one server per project root: {rows:?}"); let roots: Vec<&str> = rows.iter().map(|r| r.split('|').nth(1).unwrap()).collect(); - assert!(roots.contains(&file_uri(&fx.dir("a")).as_str()), "{roots:?}"); - assert!(roots.contains(&file_uri(&fx.dir("b")).as_str()), "{roots:?}"); + assert!( + roots.contains(&file_uri(&fx.dir("a")).as_str()), + "{roots:?}" + ); + assert!( + roots.contains(&file_uri(&fx.dir("b")).as_str()), + "{roots:?}" + ); } // --------------------------------------------------------------------------- @@ -232,7 +246,10 @@ fn acc15_two_files_in_one_root_reuse_a_single_server() { let rows = rows(&state); assert_eq!(rows.len(), 1, "same root must reuse: {rows:?}"); - assert_eq!(rows[0].split('|').nth(1).unwrap(), file_uri(&fx.dir("proj"))); + assert_eq!( + rows[0].split('|').nth(1).unwrap(), + file_uri(&fx.dir("proj")) + ); } // --------------------------------------------------------------------------- @@ -263,7 +280,11 @@ fn acc16_shipped_languages_are_unchanged_for_the_single_root_case() { settle(&mut state); let rows = rows(&state); - assert_eq!(rows.len(), 1, "{language}: expected one server, got {rows:?}"); + assert_eq!( + rows.len(), + 1, + "{language}: expected one server, got {rows:?}" + ); let fields: Vec<&str> = rows[0].split('|').collect(); assert_eq!(fields[0], language, "{language}: language_id"); assert_eq!( @@ -377,7 +398,10 @@ fn acc18_hand_spawned_server_without_root_uri_is_not_adopted() { let rows = rows(&state); assert_eq!(rows.len(), 2, "the attach must not adopt it: {rows:?}"); let roots: Vec<&str> = rows.iter().map(|r| r.split('|').nth(1).unwrap()).collect(); - assert!(roots.contains(&""), "hand-spawned reads back nil: {roots:?}"); + assert!( + roots.contains(&""), + "hand-spawned reads back nil: {roots:?}" + ); assert!( roots.contains(&file_uri(&proj).as_str()), "the attach's own server carries the root: {roots:?}" @@ -509,3 +533,83 @@ fn acc21_detected_root_and_markerless_file_get_different_servers() { "the markerless server keeps the fallback directory as cwd" ); } + +// --------------------------------------------------------------------------- +// Review-round-1 pins. Neither is a numbered acceptance criterion; both +// cover a branch the nine above leave untested. +// --------------------------------------------------------------------------- + +/// A *string* `config.root` is an affinity key. acc17 covers the function +/// form; without this the `return configured, "config"` arm has no test. +/// +/// The bite: both files sit in their own marked project, so if the config +/// arm were dropped they would key on their own detected roots and spawn +/// two servers. One server keyed on the configured root is only possible +/// if the override wins. +#[test] +fn config_string_root_overrides_detection_as_the_affinity_key() { + let fx = Fixture::new(); + fx.write("a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("b/Cargo.toml", "[package]\nname = \"b\"\n"); + let first = fx.write("a/src/main.rs", "fn main() {}\n"); + let second = fx.write("b/src/main.rs", "fn main() {}\n"); + let shared = fx.dir("shared"); + std::fs::create_dir_all(&shared).unwrap(); + let mut state = editor(); + fx.bind(&state); + exec( + &state, + &format!( + "pmacs.lsp.config.rust = {{ command = \"{}\", root = \"{}\" }}", + fake_lsp_path(), + lua_str(&shared) + ), + ); + open(&state, &first); + settle(&mut state); + open(&state, &second); + settle(&mut state); + + let rows = rows(&state); + assert_eq!( + rows.len(), + 1, + "a configured root outranks both detected roots: {rows:?}" + ); + let fields: Vec<&str> = rows[0].split('|').collect(); + assert_eq!(fields[1], file_uri(&shared), "keyed on the configured root"); + assert_eq!(fields[2], shared.display().to_string()); +} + +/// `root = false` reads as unset, as it always has. Defended in +/// `project_root_for` by a truthiness check rather than `~= nil`; this +/// pins the behavior instead of trusting the comment. +/// +/// The bite: under a `~= nil` test the config arm would return +/// `false, "config"`, and `file_uri_for(false)` returns nil — so the file +/// would land on a rootless server instead of its detected project. +#[test] +fn config_root_false_reads_as_unset_and_detection_still_wins() { + 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 = false }}", + fake_lsp_path() + ), + ); + open(&state, &file); + settle(&mut state); + + let rows = rows(&state); + assert_eq!(rows.len(), 1, "{rows:?}"); + assert_eq!( + rows[0].split('|').nth(1).unwrap(), + file_uri(&fx.dir("proj")), + "`false` must not become a root; detection still wins" + ); +} From ebcd2c4f6f083c68f711bf52fc199beb172c3092 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 14:24:59 -0400 Subject: [PATCH 4/5] =?UTF-8?q?fix(lsp):=20attribute=20a=20failing=20root?= =?UTF-8?q?=20resolver=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")) + ); +} From b5bea3b2f536d20ac5d7029cafe0796be7e54760 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 14:28:56 -0400 Subject: [PATCH 5/5] docs(coherence): record the dead reporting channel and #161's slice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rides this PR per COHERENCE.md §25 ("when a PR changes any audited claim here, updating this file rides that PR"). §0 and §7: multi-root LSP affinity moves from in-flight-branch to PR #161, and §7 gains the rule the slice actually establishes — a *fallback* root is deliberately NOT an identity, so markerless files keep sharing one server per language. That is the part a reader would otherwise assume went the other way. §1.2 gains the finding this PR turned up, which sharpens the audit rather than restating it. The section recorded that background failures produce "no `*errors*` entry"; the sharper fact is that **the channel does not exist**. Fifteen call sites — `async.lua` (5), `syntax.lua` (4), and one each in `lsp.lua`, `mcp.lua`, `fs.lua`, `editops.lua`, `autosave.lua`, `commands/default.lua` — report through `pmacs.error`, each guarded `if pmacs.error then ...`. It is defined nowhere in production; the only assignment in the tree is a test stub (`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. All fifteen are dead, and the guard is what kept it unnoticed — it makes the silence read as deliberate. Hence the corollary now recorded beside the rule: report through a channel with a **test that observes it**, or the guard is indistinguishable from the silence it was meant to fix. Also a frequency note: per-root affinity makes the preconfigured-but-missing-server failure fire once per project root rather than once per language per session. Unchanged in kind, strictly more frequent. Surfacing it stays Priority 1 work needing its own framing — it is user-visible product behavior (what message, where, with what guidance), not a substrate fix to smuggle into an affinity PR. Line-number citations in the touched sections re-verified per §25; symbols are authoritative where they drifted. --- COHERENCE.md | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 1594b69..954e621 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -120,9 +120,8 @@ asymmetry**, and **per-arc coherence debt**. Coherence-shaped work already in flight at audit time: find-file / dired Stage 0 (`C-x C-f`, PR #162, `docs/dired-framing.md`), bottom -panel Stage 1 (merged #155), multi-root LSP affinity (branch -`lsp-multi-root-affinity`), the config registry foundation (merged -#127). +panel Stage 1 (merged #155), multi-root LSP affinity (PR #161), the +config registry foundation (merged #127). --- @@ -230,9 +229,37 @@ This directly contradicts the product thesis (§23): the "without freezing" half is delivered; the "without becoming opaque" half is currently false for exactly the failures a new user will hit first. +**The reporting channel the runtime believes it has does not exist.** +Fifteen call sites — `async.lua` (5), `syntax.lua` (4), and one each in +`lsp.lua`, `mcp.lua`, `fs.lua`, `editops.lua`, `autosave.lua`, and +`commands/default.lua` — report background failures through +`pmacs.error`, each guarded as `if pmacs.error then pmacs.error(...)`. +**`pmacs.error` is never defined in production** — the only assignment +in the tree is a test stub (`src/editor.rs:9881`), and +`type(pmacs.error)` is `nil` in a fresh `EditorState`. So every one of +those fifteen reports is dead: the guard makes the silence look +deliberate and keeps it from ever being noticed. `pmacs.errors` (plural, +`builtin/runtime/compile.lua:45`) is an unrelated namespace and is not +it. This is the silence asymmetry one level deeper than §1.2 first +recorded — not "the failure isn't surfaced" but "the surface was +written, guarded, and never built." Found while landing PR #161, which +nearly added a sixteenth; that one reports via +`pmacs.editor.set_status` (which exists) with the `pmacs.error` arm +riding along for when the channel is built. + **Rule to adopt:** anything that fails automatically must leave a user-visible trace with a named owner. A `pcall` around background -wiring must log attributed failure, never discard it. +wiring must log attributed failure, never discard it. Corollary from the +above: report through a channel with a **test that observes it**, or the +guard is indistinguishable from the silence it was meant to fix. + +**Frequency note (PR #161):** per-root server affinity means the +preconfigured-but-missing-server failure now fires **once per project +root** rather than once per language per session. The silence is +unchanged in kind; it is strictly more frequent. Surfacing it stays +Priority 1 work with its own framing — it is a user-visible product +behavior (what message, where, with what guidance), not a substrate fix +to smuggle into an affinity PR. ### 1.3 Ground truth: coherence debt compounds per-arc @@ -726,12 +753,14 @@ per-subsystem conventions.** nil/`"."`. Nothing owns the set {roots, servers, terminals, tasks, layout} — which is why desktop-save under a daemon had nothing principled to attach to (Q#DS9, §2 step 12). -- **First slice in flight**: the multi-root LSP server-affinity work - (branch `lsp-multi-root-affinity`) makes *(language, found-root)* the - server identity — the first time a root functions as an identity key - rather than a spawn parameter. Note it is again per-subsystem: LSP - learns roots; compile, search, index, and trust do not share the - object. +- **First slice landed (PR #161)**: the multi-root LSP server-affinity + work makes *(language, found-root)* the server identity — the first + time a root functions as an identity key rather than a spawn + parameter. It also establishes the rule that a *fallback* root (the + file's own directory, when no marker was found) is deliberately **not** + an identity, so markerless files keep sharing one server per language. + Note it is again per-subsystem: LSP learns roots; compile, search, + index, and trust do not share the object. A workspace entity is a **model gap** (real arc), not wiring. It is also the prerequisite that keeps §8 (locations), §9 (task ownership), §11