diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 9ace580..2ef706c 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1924,7 +1924,8 @@ end local FILE_WATCH_INTERVAL_MS = 250 -- file_watchers[tostring(sid)][registrationId] = list of watch records --- ({ cancelled = bool, _sleep = handle? }), one per glob watcher. +-- ({ cancelled = bool, form = "relative"|"absolute", _sleep = handle? }), +-- one per glob watcher. local file_watchers = {} -- WatchKind is a bitmask (Create=1, Change=2, Delete=4); test it @@ -2058,7 +2059,18 @@ end local FC_CREATED, FC_CHANGED, FC_DELETED = 1, 2, 3 local function start_file_watcher(sid, base, glob, kind_mask, record) - local matches = glob_matcher(glob) + -- Per LSP, a plain-string glob matches the file's ABSOLUTE path, + -- while a RelativePattern's pattern is relative to its base — the + -- record's `form` (from resolve_watcher) picks the match subject. + -- scan_tree always walks in relative terms; only the string handed + -- to the matcher changes. + local match_glob = glob_matcher(glob) + local matches = match_glob + if record.form == "absolute" then + matches = function(rel) + return match_glob(base .. "/" .. rel) + end + end pmacs.async(function() local prev = scan_tree(base, matches) while not record.cancelled and server_is_live(sid) do @@ -2069,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] @@ -2097,22 +2132,44 @@ local function start_file_watcher(sid, base, glob, kind_mask, record) end -- Resolve a GlobPattern (string | { baseUri, pattern }) to --- (base_dir, pattern). A bare string with no base falls back to the --- directory of an attached file on `sid` (best effort). +-- (base_dir, pattern, form). The form must travel with the pair: a +-- 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 "**" + return pmacs.lsp.path_for_uri(gp.baseUri), gp.pattern or "**", "relative" end if type(gp) == "string" then for _, rec in pairs(attachments) do 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 end + if dir then + return dir, gp, (gp:sub(1, 1) == "/") and "absolute" or "relative" + end end end end - return nil, nil + return nil, nil, nil +end + +local function cancel_watch_records(recs) + for _, r in ipairs(recs or {}) do + r.cancelled = true + if r._sleep then pcall(function() r._sleep:cancel() end) end + end end local function register_file_watchers(sid, registrations) @@ -2120,11 +2177,16 @@ local function register_file_watchers(sid, registrations) file_watchers[skey] = file_watchers[skey] or {} for _, reg in ipairs(registrations or {}) do if reg.method == "workspace/didChangeWatchedFiles" then + -- Re-registering a live id supersedes it (rust-analyzer does + -- this): cancel the outgoing records first, because the table + -- write below drops the only reference to them and an + -- uncancelled record polls until the server dies. + cancel_watch_records(file_watchers[skey][reg.id]) local recs = {} for _, w in ipairs((reg.registerOptions or {}).watchers or {}) do - local base, pat = resolve_watcher(sid, w.globPattern) + local base, pat, form = resolve_watcher(sid, w.globPattern) if base and pat then - local r = { cancelled = false } + local r = { cancelled = false, form = form } recs[#recs + 1] = r start_file_watcher(sid, base, pat, w.kind or 7, r) end @@ -2139,10 +2201,7 @@ local function unregister_file_watchers(sid, unregs) if not byid then return end for _, u in ipairs(unregs or {}) do if u.method == "workspace/didChangeWatchedFiles" and byid[u.id] then - for _, r in ipairs(byid[u.id]) do - r.cancelled = true - if r._sleep then pcall(function() r._sleep:cancel() end) end - end + cancel_watch_records(byid[u.id]) byid[u.id] = nil end end diff --git a/docs/active-work.md b/docs/active-work.md index c2b19b3..53df4de 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -210,6 +210,108 @@ hazard in a shape that looks committed. **A documented error message that never appears is worse than no documentation**, because the reader waits for a signal that is not coming. +## LSP file watcher (issue #233) — PR #234 OPEN, awaiting review + +**Issue #233** — https://github.com/levineuwirth/pmacs/issues/233. +**PR #234** — https://github.com/levineuwirth/pmacs/pull/234, opened +2026-08-10 at `ed3033c` (the implementation commit atop the framing). +**URGENT by user ruling 2026-08-10: PR #227 is held unmerged until this +is resolved**, even though #227 is green and cleared. + +- **Branch `lsp-file-watcher`**, base `githubsucks/main` @ `0e4c58d`. + **`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 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"` (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. +- **Verification**: three new discriminating tests beside `m4_24` + (`filewatchabs` / `filewatchflat` / `filewatchrereg` fake-LSP modes), + each mutation-tested against the defect it names: reverting D1 fails + only the plain-string test; matching every form absolutely fails only + the RelativePattern guard; reverting D2 fails only the + re-registration test, with the leaked watcher's `.old` event visible + in `.received`. `m4_24` stayed green under all three mutations — + its recorded insensitivity, confirmed. + +**Not a #232 regression.** The statusline activity indicator is correct; +it renders real in-flight jobs. What changed on 2026-08-09 is +**visibility**. The polling watcher has been there since `1c25730` +(2026-05-19). Quieting the indicator would delete the only instrument +that found this, and is explicitly not the fix. + +- **Scope: D1 and D2 only** — plain-string globs are matched against + relative paths so they never match (rust-analyzer sees **no** file + changes at all; gopls sees `go.mod` but never `.go`), and + re-registering the same id leaks the previous coroutines + (rust-analyzer registers twice under one id → 12 pollers, 6 + permanently uncancellable). +- **Two findings from framing that the issue does not carry**, both of + which change the fix: + 1. **The existing test cannot discriminate this fix in either + direction.** The fake LSP's `**/*.txt` compiles to `^.-[^/]*%.txt$` + and `.-` spans `/`, so it matches relative *and* absolute subjects. + `m4_24` passes whether D1 is fixed or broken. New coverage must use + a pattern whose two readings disagree. + 2. **"Match the absolute path" alone would break `RelativePattern`.** + Measured: `*.txt` matches `a.txt` but not `/base/a.txt`. + `resolve_watcher` returns `(base, pattern)` and discards which form + it came from, so its contract has to change — not just the match + subject. +- **RULING ANSWERED 2026-08-10:** D1 and D2 fix correctness and the + leak but **do not stop the walking** — `walk` recurses + unconditionally and `matches` gates only recording. After this lane + rust-analyzer still walks the whole tree every 250 ms, six times per + tick instead of twelve. **The user accepted that scope**: the + acceptance bar for this lane is correctness and the leak, not "the + flipping stops". The walking is D3's, framed separately. +- **D3 deferred** with what was checked: there is **no `notify`/inotify + dependency in the tree**, so a real filesystem-notification primitive + is a new crate plus a Rust primitive plus its binding; and there is + **no ignore-list infrastructure** to reuse (`src/project.rs` knows + `.git` as a marker, not as something to skip). D3 is a + `COHERENCE.md` §9 concern and needs its own framing. +- **Gates:** `./scripts/gate --acceptance m4_acceptance` — **all nine + steps green, 2026-08-10** (lib 1,928; lib-crdt 2,113; m4 154 with the + standing `basedpyright` skip; GPU 243 under `PMACS_REQUIRE_GPU=1`; + full-workspace sweep 3,891 passed / 0 failed across 118 targets; + `diff --check` clean). The sweep covers the eleven other acceptance + suites that spawn the fake LSP; the three new modes are additive and + no existing mode's behaviour changes. No `--protocol` — no wire + change. + ## `scripts/gate` — PR #225 OPEN (build tooling) **PR #225** — https://github.com/levineuwirth/pmacs/pull/225. Written diff --git a/docs/lsp-file-watcher-framing.md b/docs/lsp-file-watcher-framing.md new file mode 100644 index 0000000..846cbd1 --- /dev/null +++ b/docs/lsp-file-watcher-framing.md @@ -0,0 +1,240 @@ +# LSP file watcher — framing + +**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 +gets its own framing. + +## What is and is not a regression + +**#232 is not at fault and nothing about it should be reverted.** The +statusline activity indicator it added is *correct*: it renders real +in-flight jobs from `AsyncRuntime::activity_summary`, and the jobs it +names (`sleep 250ms`, `read_dir `) are real. What changed on +2026-08-09 is **visibility**, not behaviour. + +The behaviour has been there since `1c25730` (2026-05-19). So the user- +facing report — "the modeline flips several times a second" — is a +three-month-old defect that became observable last week, and the fix +belongs to the watcher, not the indicator. + +Recorded plainly because the tempting move is to quiet the indicator, +and that would delete the only instrument that found this. + +## Verified against the tree at `0e4c58d` + +Every claim below was read or executed this session, not carried from +the issue. + +- `FILE_WATCH_INTERVAL_MS = 250` (`lsp.lua:1924`); each watcher is one + `pmacs.async` coroutine looping sleep → `scan_tree` + (`lsp.lua:2060-2097`). +- `scan_tree` builds `rel` from an empty prefix and calls + `matches(rel)` — **relative** paths (`lsp.lua:2035-2056`). +- **`walk` recurses into every directory unconditionally.** `matches` + gates only whether an entry is *recorded*. A watcher that can never + match still walks the whole tree every tick. +- `resolve_watcher`'s string branch returns the pattern **unchanged** + with the base guessed from an attached file's directory + (`lsp.lua:2102-2116`). +- `register_file_watchers` ends `file_watchers[skey][reg.id] = recs` + with no cancellation of the outgoing list (`lsp.lua:2132`). +- Job purposes are `format!("sleep {}ms", …)` (`async_runtime.rs:1027`) + and `format!("read_dir {}", …)` (`:1178`). +- The fake LSP registers **one** watcher, a `RelativePattern` + `{ baseUri, pattern: "**/*.txt" }`, id `watch-1` + (`pmacs_fake_lsp.rs:312-331`). + +### The glob table, reproduced + +Ran the tree's own `expand_braces` / `glob_one_to_pattern` / +`glob_matcher` under LuaJIT. Output matches the issue exactly, compiled +patterns included: + +| glob | compiled | `main.go` | `go.mod` | absolute | +|---|---|---|---|---| +| `**/*.{mod,work}` | `^.-[^/]*%.mod$` | false | **true** | true | +| `/goproj/**/*.{go,…}` | `^/tmp/goproj/.-[^/]*%.go$` | false | false | true | +| `/rsproj/**/*.rs` | `^/tmp/rsproj/.-[^/]*%.rs$` | false | false | true | + +## Two findings the issue does not carry, both of which shape the fix + +### F1 — the existing test cannot discriminate this fix, in either direction + +`**/*.txt` compiles to `^.-[^/]*%.txt$`, and `.-` spans `/`. Measured: +it matches `a.txt`, `sub/a.txt`, `/base/a.txt` **and** +`/base/sub/a.txt`. So `m4_24_workspace_did_change_watched_files` passes +whether the matching subject is relative or absolute. + +The issue says the tested path and the exercised path are disjoint. The +sharper statement is that the existing test is **insensitive**: it +cannot fail for D1 and it cannot confirm D1's fix. New coverage must use +a pattern whose two readings disagree, or it will inherit the same +blindness. + +### F2 — the fix cannot simply "match absolute"; the form must be carried + +Per LSP, a plain-string glob matches the **absolute** path while a +`RelativePattern`'s pattern is relative to **its base**. Matching +everything absolutely breaks the second. Measured on `*.txt`: + +| subject | matches | +|---|---| +| `a.txt` (relative, correct for RelativePattern) | **true** | +| `/base/a.txt` (absolute) | **false** | + +`resolve_watcher` returns `(base, pattern)` and **discards which form it +came from**, so both callers below it are already unable to tell. The +fix therefore changes that function's contract — a third return value or +an explicit record field — rather than only changing the subject string +at the match site. A fix that ignores this trades rust-analyzer's six +broken globs for every `RelativePattern` whose pattern does not begin +`**/`. + +## D1 — plain-string globs never match + +**Consequences, as measured in the issue and confirmed by the table +above:** rust-analyzer is never told about any file change (all six +globs absolute); gopls is told about `go.mod`/`go.work` but never `.go` +sources (only its relative glob matches). + +**Fix:** match a plain-string glob against `base .. "/" .. rel`; keep a +`RelativePattern` matched against `rel`. `resolve_watcher` gains the +form in its return, and the record carries it. + +The leading `**/` in gopls' relative glob compiles to `.-`, which spans +`/`, so that glob keeps matching under the absolute subject — which is +why one server's working case does not regress. + +## D2 — re-registration leaks the previous coroutines + +`file_watchers[skey][reg.id] = recs` replaces the record list without +setting `cancelled` or cancelling the in-flight `_sleep`. The old +coroutines poll until the server dies and are unreachable by +`unregister_file_watchers`, which can only see what the table now holds. + +**Reachable today**: rust-analyzer registers +`workspace/didChangeWatchedFiles` **twice under the same id**, six +watchers each, with no intervening unregister — 12 concurrent +coroutines, six permanently uncancellable. The issue's 44.1/s dir-open +rate against a ~270 ms period implies 12 watchers, so the leak is +measured from outside the process, not only read from the source. + +**Fix:** cancel the outgoing list before replacing it, with the same +treatment `unregister_file_watchers` already applies. + +## What this lane does NOT fix, stated so the report is not mistaken for closed + +**The poll cost survives both fixes.** D1 makes matching correct and D2 +halves rust-analyzer's watcher count; neither stops the walk. After this +lane, rust-analyzer still walks the entire tree every 250 ms — six times +per tick instead of twelve — including `.git`, `target` and +`node_modules`, at one async job per directory. + +So the modeline will still show activity, at roughly half the rate. **If +the acceptance bar for this lane is "the flipping stops", this lane does +not meet it** and should not be started until D3 is framed. That is a +ruling for the user, not an assumption to make quietly. + +**Answered 2026-08-10: the user accepted this scope.** D1 and D2 +proceed; the walking is D3's problem, framed separately. + +## D3 — deferred, with what was checked + +Options named in the issue: coalesce a server's watchers into one scan; +root the scan at the workspace rather than an attached file's directory; +an ignore list; back off when nothing changes; or a real +filesystem-notification primitive. + +Checked while framing: **there is no `notify`/inotify dependency in the +tree**, so the last option is a new crate *and* a new Rust primitive +plus its Lua binding — not a small change. There is also **no existing +ignore-list infrastructure** to reuse; `src/project.rs` knows `.git` as +a *marker* name, not as something to skip. + +D3 is a `COHERENCE.md` §9 concern — background work with no ownership +model — and §9's own Stage 1 is the indicator that surfaced it. + +## Verification + +The suite must fail without each fix, which the existing suite cannot +(F1). Planned: + +- **A fake-LSP mode registering a plain-string ABSOLUTE glob**, with a + pattern whose relative and absolute readings **disagree** — so the + test fails today and passes after D1. +- **A fake-LSP mode registering a `RelativePattern` whose pattern does + not begin `**/`** (e.g. `*.txt` at the base). This is F2's guard: it + passes today, and fails against a fix that matches everything + absolutely. Without it, the obvious wrong fix is green. +- **A re-registration mode**: the same id twice, no unregister. The + witness is that the superseded watchers **stop**, asserted on + observable polling rather than on internal table shape, since the + defect is precisely that the old records are unreachable. +- Existing `m4_24` kept and expected **unchanged** — it covers the + working branch and its insensitivity is now recorded rather than + mistaken for coverage. + +Each new test is mutation-tested against the fix it names. + +## Coherence impact (§20) + +- **Journey steps**: none added; step 5's editing surface is affected + only in that a correct watcher makes servers see edits they currently + miss. +- **Interaction islands**: none. +- **Config registry**: no new setting. The interval stays a module + constant; making it configurable would offer the user a knob for a + defect rather than a preference, and D3 may remove the poll entirely. +- **Background-work attribution (§9)**: this lane *reduces* unattributed + background work but does not model it. D3 owns that, and the honest + statement is that the indicator worked — it made three months of + invisible churn visible on its first week. + +## Gates + +`./scripts/gate --acceptance m4_acceptance` plus the touched LSP +acceptance suites; no `--protocol` (no wire change, no +`PROTOCOL_VERSION` bump). diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 19dc0b6..c7393a3 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -332,6 +332,106 @@ fn main() { }); write_frame(&mut stdout, &req); } + // Issue #233 D1: `filewatchabs` registers the same watcher + // as a PLAIN-STRING glob — `/**/*.txt`, the form + // rust-analyzer and gopls actually send. Per LSP it matches + // the file's ABSOLUTE path; its relative reading matches + // nothing, so the mode discriminates the match subject. + ("initialized", _) if mode == "filewatchabs" => { + let base = std::env::var("PMACS_FAKE_LSP_WATCH_BASE").unwrap_or_default(); + let req = serde_json::json!({ + "jsonrpc": "2.0", + "id": 9301, + "method": "client/registerCapability", + "params": { "registrations": [{ + "id": "watch-abs", + "method": "workspace/didChangeWatchedFiles", + "registerOptions": { "watchers": [{ + "globPattern": format!("{base}/**/*.txt"), + "kind": 7 + }] } + }] } + }); + 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 + // RELATIVELY and no absolute path at all, so a fix that + // matches every form against the absolute path goes red. + ("initialized", _) if mode == "filewatchflat" => { + let base = std::env::var("PMACS_FAKE_LSP_WATCH_BASE").unwrap_or_default(); + let req = serde_json::json!({ + "jsonrpc": "2.0", + "id": 9302, + "method": "client/registerCapability", + "params": { "registrations": [{ + "id": "watch-flat", + "method": "workspace/didChangeWatchedFiles", + "registerOptions": { "watchers": [{ + "globPattern": { + "baseUri": format!("file://{base}"), + "pattern": "*.txt" + }, + "kind": 7 + }] } + }] } + }); + write_frame(&mut stdout, &req); + } + // Issue #233 D2: `filewatchrereg` registers the SAME id + // twice with no unregister between — `**/*.old` then + // `**/*.new` — exactly rust-analyzer's shape. The second + // registration must supersede the first: only `.new` + // events may ever reach `.received`. + ("initialized", _) if mode == "filewatchrereg" => { + let base = std::env::var("PMACS_FAKE_LSP_WATCH_BASE").unwrap_or_default(); + for (rid, pattern) in [(9303, "**/*.old"), (9304, "**/*.new")] { + let req = serde_json::json!({ + "jsonrpc": "2.0", + "id": rid, + "method": "client/registerCapability", + "params": { "registrations": [{ + "id": "watch-re", + "method": "workspace/didChangeWatchedFiles", + "registerOptions": { "watchers": [{ + "globPattern": { + "baseUri": format!("file://{base}"), + "pattern": pattern + }, + "kind": 7 + }] } + }] } + }); + write_frame(&mut stdout, &req); + } + } ("initialized", _) => {} // T M4.5: the client's file-watch notifications. Append // `type uri` lines to `/.received` as a test diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index d706305..db79602 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -5351,6 +5351,416 @@ fn m4_24_workspace_did_change_watched_files() { ); } +/// Issue #233 D1 — a PLAIN-STRING `GlobPattern` matches the file's +/// ABSOLUTE path (LSP 3.17), not the walk's relative path. The +/// `filewatchabs` fake registers `/**/*.txt` as a bare string — +/// the form rust-analyzer and gopls actually send. Its relative +/// reading matches nothing (an anchored `^/…` can never match +/// `foo.txt`), so before the fix no event could ever be reported. +/// The watcher's base is guessed from the attached file's directory — +/// the tempdir here, and the production path for bare-string globs. +#[test] +fn m4_24_plain_string_glob_matches_absolute_path() { + 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 = 'filewatchabs', + 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" + ); + + // Same warm-up as m4_24: let registerCapability land and the + // watcher take its empty baseline before files appear. + 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"); + std::fs::write(base.join("bar.md"), b"md\n").expect("write bar.md"); + assert!( + pump_until_file_contains(&mut state, &received, &format!("1 {foo_uri}"), 6), + "CREATED for foo.txt never reported under a plain-string glob; \ + .received = {:?}", + std::fs::read_to_string(&received).unwrap_or_default() + ); + assert!( + !std::fs::read_to_string(&received) + .unwrap_or_default() + .contains("bar.md"), + "non-matching .md must be filtered out" + ); +} + +/// Issue #233 F2 guard — a `RelativePattern` stays relative to its +/// base. The `filewatchflat` fake registers `{ baseUri, pattern = +/// "*.txt" }`, whose pattern has no leading `**/`: it matches +/// base-level files RELATIVELY and cannot match any absolute path +/// (`[^/]*` spans no `/`). Green before and after D1's fix; red +/// against the obvious wrong fix that matches every form absolutely. +/// `sub/nested.txt` pins the other half of the same contract: a +/// base-level pattern must not match into subdirectories. +#[test] +fn m4_24_relative_pattern_without_globstar_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()); + std::fs::create_dir(base.join("sub")).expect("mkdir sub"); + + 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 = 'filewatchflat', + 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)); + } + + // nested.txt is written BEFORE foo.txt, so a watcher that wrongly + // matched it would report it no later than foo.txt's event — the + // negative assertion after the positive one is race-free. + std::fs::write(base.join("sub").join("nested.txt"), b"deep\n").expect("write nested.txt"); + 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 base-level foo.txt never reported under a \ + RelativePattern without `**/`; .received = {:?}", + std::fs::read_to_string(&received).unwrap_or_default() + ); + assert!( + !std::fs::read_to_string(&received) + .unwrap_or_default() + .contains("nested.txt"), + "a base-level `*.txt` RelativePattern must not match into \ + subdirectories" + ); +} + +/// 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 +/// shape rust-analyzer sends. The superseded watchers must STOP, +/// asserted on observable polling rather than on table shape (the +/// defect is precisely that the replaced records become unreachable +/// while still polling): `f.old` exists on disk before either `.new` +/// event lands, so a leaked first-registration watcher, polling at +/// the same 250 ms cadence, would have reported it by the time the +/// second `.new` positive arrives. +#[test] +fn m4_24_reregistration_supersedes_previous_watchers() { + 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 f_old_uri = format!("file://{}", base.join("f.old").display()); + let f_new_uri = format!("file://{}", base.join("f.new").display()); + let g_new_uri = format!("file://{}", base.join("g.new").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 = 'filewatchrereg', + 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("f.old"), b"old\n").expect("write f.old"); + std::fs::write(base.join("f.new"), b"new\n").expect("write f.new"); + assert!( + pump_until_file_contains(&mut state, &received, &format!("1 {f_new_uri}"), 6), + "CREATED for f.new never reported by the superseding watcher; \ + .received = {:?}", + std::fs::read_to_string(&received).unwrap_or_default() + ); + // A second positive puts at least one more full poll cycle between + // f.old appearing on disk and the negative assertion below. + std::fs::write(base.join("g.new"), b"new\n").expect("write g.new"); + assert!( + pump_until_file_contains(&mut state, &received, &format!("1 {g_new_uri}"), 6), + "CREATED for g.new never reported by the superseding watcher" + ); + assert!( + !std::fs::read_to_string(&received) + .unwrap_or_default() + .contains(&f_old_uri), + "the superseded `**/*.old` watcher is still polling after \ + re-registration under the same id; .received = {:?}", + std::fs::read_to_string(&received).unwrap_or_default() + ); +} + /// Tier 1 single-binary language servers ship pre-configured in the /// default bundle. Binary-independent: we don't spawn anything, just /// assert the `pmacs.lsp.config` tables and the `pmacs.lsp.filetypes`