From 35085b54d1f24f35759390859f72f80d95180e54 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 11:19:43 -0400 Subject: [PATCH] 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" + ); +}