diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index d247d06..2ef706c 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -2081,6 +2081,29 @@ local function start_file_watcher(sid, base, glob, kind_mask, record) if record.cancelled or not server_is_live(sid) then break end local cur = scan_tree(base, matches) + -- The seam that makes the recheck below WITNESSABLE. `scan_tree` + -- suspends on `read_dir` once per directory, and the race is a + -- cancel arriving during one of those suspensions --- which no + -- arrangement of real timing can be made to happen on demand. + -- Same reason `git.lua` exposes `_deliver_status`: the contract is + -- about an interleaving the caller does not choose. Unset in + -- production, so this costs one nil test per tick. + -- `cur` is handed over so a test can cancel on THE SCAN THAT + -- OBSERVED a given change. Cancelling on any other scan is not a + -- witness: the loop would break at the post-sleep check on the + -- next iteration and emit nothing anyway, so the assertion would + -- pass with the recheck below deleted. + if pmacs.lsp._after_scan_for_tests then + pcall(pmacs.lsp._after_scan_for_tests, record, cur) + end + -- RECHECKED AFTER THE SCAN, not only after the sleep (review P2). + -- The coroutine is suspended for most of a tick with `_sleep` + -- already cleared, so a cancel landing there sets `cancelled` and + -- has no sleep to interrupt. Without this line the resumed scan + -- runs on to `did_change_watched_files` below and a SUPERSEDED + -- watcher emits one last batch under its OLD pattern. One batch is + -- enough: it is a wrong-pattern notification the server acts on. + if record.cancelled or not server_is_live(sid) then break end local changes = {} for rel, sig in pairs(cur) do local was = prev[rel] @@ -2110,11 +2133,20 @@ end -- Resolve a GlobPattern (string | { baseUri, pattern }) to -- (base_dir, pattern, form). The form must travel with the pair: a --- RelativePattern's pattern is relative to its baseUri, but a bare --- string matches the file's ABSOLUTE path (real servers send absolute --- globs), and dropping the distinction here is what made those globs --- unable to match anything. A bare string with no base falls back to --- the directory of an attached file on `sid` (best effort). +-- RelativePattern's pattern is relative to its baseUri, and dropping +-- that distinction is what made absolute server globs unable to match +-- anything. A bare string with no base falls back to the directory of +-- an attached file on `sid` (best effort). +-- +-- THE FORM COMES FROM THE PATTERN, NOT FROM THE UNION ARM (review P1). +-- The first fix for #233 returned `"absolute"` for every string, which +-- is a different bug wearing the same shape: LSP 3.17 defines `Pattern` +-- relative to a base path, and VS Code treats a string watcher as +-- applying across workspace folders, so a bare `*.txt` is a VALID +-- relative pattern. Classifying it absolute matched it against +-- `/foo.txt`, which `^[^/]*%.txt$` can never match --- so that +-- fix silently broke a case that worked before it. A leading `/` is +-- what makes a pattern absolute; the arm it arrived in is not. local function resolve_watcher(sid, gp) if type(gp) == "table" and gp.baseUri then return pmacs.lsp.path_for_uri(gp.baseUri), gp.pattern or "**", "relative" @@ -2124,7 +2156,9 @@ local function resolve_watcher(sid, gp) if rec.server == sid and rec.uri then local p = pmacs.lsp.path_for_uri(rec.uri) local dir = p and p:match("^(.*)/[^/]*$") - if dir then return dir, gp, "absolute" end + if dir then + return dir, gp, (gp:sub(1, 1) == "/") and "absolute" or "relative" + end end end end diff --git a/docs/active-work.md b/docs/active-work.md index aa8bcae..53df4de 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -222,16 +222,38 @@ is resolved**, even though #227 is green and cleared. **`githubsucks/lsp-file-watcher` is the authoritative tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout lsp-file-watcher`. -- **Framing `docs/lsp-file-watcher-framing.md`, revision 1, APPROVED - 2026-08-10.** The ruling that blocked implementation is ANSWERED (see - below); D1 and D2 are implemented on the branch atop the framing - commit. Committed at the branch's first commit so it is portable +- **Framing `docs/lsp-file-watcher-framing.md`, revision 2** — design + approved 2026-08-10; revision 2 records a review round **against the + implementation** that found two correctness defects, both now fixed + (see below). Committed at the branch's first commit so it is portable during review — the standing lesson from the GUI arc framing, which spent two review rounds as an untracked file in one worktree. +- **Review round 1 on the code found two defects, and both are cases + where the first fix was itself wrong:** + 1. **P1 — a bare-string `*.txt` stopped working.** + `resolve_watcher` classified **every** string as absolute, so a + valid relative pattern was matched against `/foo.txt` and + could never fire. That path worked **before** this lane, so the + repair broke a live case while fixing another. The form now comes + from the pattern (a leading `/`), not from the union arm. The + flat-pattern test did not catch it because it exercises the + RelativePattern **object** arm — F1's tested-vs-exercised split, + repeating inside the lane that named it. + 2. **P2 — a scan finishing after cancellation still emitted one + batch.** `scan_tree` suspends on `read_dir` per directory with + `_sleep` already cleared, so a cancel landing there had nothing to + interrupt and the resumed scan ran on to + `did_change_watched_files` under the superseded pattern. + Cancellation and liveness are rechecked after the scan. +- **A test seam was added**: `pmacs.lsp._after_scan_for_tests`, nil in + production, handed the scan result. P2's race cannot be produced by + timing; the seam is the same device as `git.lua`'s `_deliver_status`. + It takes the scan result because a test cancelling on any *other* + scan would pass with the fix deleted. - **What the implementation is**: `resolve_watcher` returns `(base, pattern, form)` and the watch record carries the form — - `"absolute"` (plain string) matches `base .. "/" .. rel`, - `"relative"` (RelativePattern) matches `rel`; and + `"absolute"` (pattern begins `/`) matches `base .. "/" .. rel`, + `"relative"` matches `rel`; and `register_file_watchers` cancels the outgoing record list through the new `cancel_watch_records` (shared with `unregister_file_watchers`) before the table write drops the only reference to it. diff --git a/docs/lsp-file-watcher-framing.md b/docs/lsp-file-watcher-framing.md index 2fe8e0d..846cbd1 100644 --- a/docs/lsp-file-watcher-framing.md +++ b/docs/lsp-file-watcher-framing.md @@ -1,9 +1,47 @@ # LSP file watcher — framing -**Status: revision 1 — APPROVED 2026-08-10.** The user ruled that D1 -and D2 proceed with the walking explicitly surviving this lane; D3 gets -its own framing. The acceptance bar for this lane is correctness and -the leak, not "the flipping stops". +**Status: revision 2 — approved design (2026-08-10), plus two +correctness findings from review OF THE IMPLEMENTATION.** The user ruled +that D1 and D2 proceed with the walking explicitly surviving this lane; +D3 gets its own framing. The acceptance bar for this lane is correctness +and the leak, not "the flipping stops". + +**Revision 2 records a review round against the code, not the design. +Both findings are cases where the first fix was itself wrong**, and both +were confirmed against the tree before being acted on: + +- **P1 — the form must be read from the PATTERN, not from the union + arm.** `resolve_watcher` returned `"absolute"` for *every* string, so + a bare `*.txt` — a valid relative pattern under LSP 3.17, and how VS + Code treats string watchers across workspace folders — was matched + against `/foo.txt` and could never fire. **That case worked + before this lane touched it**, so the repair for #233 silently broke a + live path while fixing another. A leading `/` is what makes a pattern + absolute; classification now reads the string. +- **P2 — a scan completing after cancellation still emitted.** + `scan_tree` awaits `read_dir` once per directory, so the coroutine + sits suspended for most of a tick with `_sleep` already cleared. A + cancel arriving there sets `cancelled` and has no sleep to interrupt, + and the resumed scan ran on to `did_change_watched_files` — one stale + batch under the superseded pattern, which is a wrong-pattern + notification the server acts on. Cancellation and liveness are now + rechecked after the scan. + +**F1's lesson repeated itself inside this lane.** The flat-pattern test +constrains the RelativePattern **object** arm, so it said nothing about +the **string** arm P1's regression lived in — the same +tested-path/exercised-path split this framing opened by naming. Both +findings now have tests, and both tests were mutation-checked: each +fails only its own defect. + +**A test seam was added, and is recorded here rather than buried.** +`pmacs.lsp._after_scan_for_tests` is a production hook, nil in normal +operation, that P2's witness requires: the race is a cancel landing +during one of the scan's suspensions, which no arrangement of real +timing produces on demand. Same device and justification as `git.lua`'s +`_deliver_status`. It is handed the scan result deliberately — a test +that cancels on any *other* scan passes with the fix deleted, because +the loop would break at the post-sleep check and emit nothing anyway. Answers issue #233. **Scope is D1 and D2 only** — the two bug-shaped defects. D3 (the polling cost) is named here, deferred with reasons, and diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 9a051ac..c7393a3 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -354,6 +354,32 @@ fn main() { }); write_frame(&mut stdout, &req); } + // Issue #233 review P1 guard: `filewatchbare` registers a + // BARE STRING with no base and no leading `/` — `*.txt`. + // The string arm and the `filewatchflat` arm below carry the + // same pattern deliberately: `flat` proves a + // RelativePattern stays relative, and this proves the + // classification is read from THE PATTERN rather than from + // the union arm it arrived in. The first fix for #233 + // called every string absolute, which matched this against + // `/foo.txt` and broke a case that had worked since + // May. Without this mode that regression is invisible. + ("initialized", _) if mode == "filewatchbare" => { + let req = serde_json::json!({ + "jsonrpc": "2.0", + "id": 9304, + "method": "client/registerCapability", + "params": { "registrations": [{ + "id": "watch-bare", + "method": "workspace/didChangeWatchedFiles", + "registerOptions": { "watchers": [{ + "globPattern": "*.txt", + "kind": 7 + }] } + }] } + }); + write_frame(&mut stdout, &req); + } // Issue #233 F2 guard: `filewatchflat` registers a // RelativePattern whose pattern has no leading `**/` // (`*.txt` at the base). It matches base-level files diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index b082738..db79602 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -5506,6 +5506,174 @@ fn m4_24_relative_pattern_without_globstar_stays_relative() { ); } +/// Issue #233 review P2 — a scan that completes AFTER cancellation +/// must not emit. +/// +/// `scan_tree` awaits `read_dir` once per directory, so the watcher +/// coroutine spends most of a tick suspended with `_sleep` already +/// cleared. A cancel arriving there — re-registration or unregistration +/// — sets `cancelled` and has no sleep to interrupt, so before the fix +/// the resumed scan ran on and emitted one last batch under the +/// superseded pattern. +/// +/// No arrangement of real timing produces that interleaving on demand, +/// so it is driven through `pmacs.lsp._after_scan_for_tests`, the same +/// device `git.lua` uses for out-of-order completions. The hook is +/// handed the scan result and cancels **only on the scan that observed +/// `foo.txt`** — cancelling on any other scan would pass with the fix +/// deleted, because the loop would break at the post-sleep check and +/// emit nothing regardless. +#[test] +fn m4_24_a_scan_finishing_after_cancellation_emits_nothing() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let base = dir.path().to_path_buf(); + let base_disp = base.display().to_string(); + let a_path = base.join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n").expect("write a"); + let a_disp = a_path.display().to_string(); + let received = base.join(".received"); + + let mut state = EditorState::new_with_roots(&crate::iso::roots()); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'filewatch', + PMACS_FAKE_LSP_WATCH_BASE = '{base_disp}' }} }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + assert!( + pump_lua_flag( + &mut state, + "(function() for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end return false end)()", + 5, + ), + "fake never initialized" + ); + + // Armed BEFORE the file exists, so the cancel cannot land early: + // the hook fires on every scan and only cancels once the scan it is + // inspecting actually contains foo.txt. + state + .lua_host + .lua() + .load( + "pmacs.lsp._after_scan_for_tests = function(record, cur) + if cur and cur['foo.txt'] then record.cancelled = true end + end", + ) + .exec() + .expect("install scan hook"); + + let warm = Instant::now() + Duration::from_millis(900); + while Instant::now() < warm { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(15)); + } + + std::fs::write(base.join("foo.txt"), b"one\n").expect("write foo.txt"); + + let deadline = Instant::now() + Duration::from_secs(4); + while Instant::now() < deadline { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(15)); + } + + let got = std::fs::read_to_string(&received).unwrap_or_default(); + assert!( + !got.contains("foo.txt"), + "a watcher cancelled during its scan emitted a stale batch \ + anyway; .received = {got:?}" + ); +} + +/// Issue #233 review P1 — a BARE-STRING glob with no leading `/` is a +/// relative pattern and must stay one. +/// +/// The first fix for #233 classified every string-arm pattern as +/// absolute, so `*.txt` was matched against `/foo.txt` and could +/// never fire — silently breaking a case that had worked since May +/// while fixing the absolute one. `m4_24_relative_pattern_without_globstar_stays_relative` +/// does not cover it: that mode sends the `RelativePattern` OBJECT form, +/// so it constrains the object arm only. This sends the same pattern +/// through the STRING arm, which is the arm the regression lived in. +#[test] +fn m4_24_bare_string_glob_stays_relative() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let base = dir.path().to_path_buf(); + let base_disp = base.display().to_string(); + let a_path = base.join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n").expect("write a"); + let a_disp = a_path.display().to_string(); + let received = base.join(".received"); + let foo_uri = format!("file://{}", base.join("foo.txt").display()); + + let mut state = EditorState::new_with_roots(&crate::iso::roots()); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'filewatchbare', + PMACS_FAKE_LSP_WATCH_BASE = '{base_disp}' }} }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + assert!( + pump_lua_flag( + &mut state, + "(function() for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end return false end)()", + 5, + ), + "fake never initialized" + ); + + let warm = Instant::now() + Duration::from_millis(900); + while Instant::now() < warm { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(15)); + } + + std::fs::write(base.join("foo.txt"), b"one\n").expect("write foo.txt"); + assert!( + pump_until_file_contains(&mut state, &received, &format!("1 {foo_uri}"), 6), + "CREATED for foo.txt never reported under a bare-string `*.txt` \ + glob — the string arm is being classified absolute again; \ + .received = {:?}", + std::fs::read_to_string(&received).unwrap_or_default() + ); +} + /// Issue #233 D2 — re-registering a live id supersedes it. The /// `filewatchrereg` fake registers `watch-re` TWICE with no /// unregister between — `**/*.old`, then `**/*.new` — exactly the