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" + ); +}