From 5cbcb1cf03463225a922a704635ba2262787d823 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 10 Aug 2026 22:55:40 +0200 Subject: [PATCH 1/4] docs: frame the LSP file watcher fix (issue #233), and its lane Framing and lane at the branch's first commit, so the document is portable while it is reviewed. The GUI arc framing spent two review rounds as an untracked file in one worktree; that is the lesson being applied, not a preference. Revision 1 is a DRAFT and no implementation may start from it. Scope is D1 and D2, the two bug-shaped defects. Every claim in the framing was read or executed against `0e4c58d` rather than carried from the issue --- including re-running the tree's own `expand_braces` / `glob_one_to_pattern` / `glob_matcher` under LuaJIT, which reproduces the issue's glob table exactly, compiled patterns included. #232 is NOT at fault and nothing about it should be reverted. The activity indicator renders real in-flight jobs; what changed is visibility, not behaviour, and the watcher has polled since 1c25730 in May. Quieting the indicator would delete the instrument that found this. Two findings the issue does not carry, both of which change the fix: The existing test cannot discriminate this fix IN EITHER DIRECTION. The fake LSP registers `**/*.txt`, which compiles to `^.-[^/]*%.txt$`, and `.-` spans `/` --- so it matches relative and absolute subjects alike and `m4_24` passes whether D1 is fixed or broken. The issue calls the tested and exercised paths disjoint; the sharper statement is that the one existing test is INSENSITIVE. And "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. A fix that misses this trades six broken rust-analyzer globs for every RelativePattern that does not begin `**/`. The verification plan includes a test that fails against exactly that wrong fix. The framing also records what this lane does NOT fix, so the report is not mistaken for closed: neither D1 nor D2 stops the walking, because `walk` recurses unconditionally and `matches` gates only recording. The modeline will keep flipping at roughly half the rate. Whether that meets the acceptance bar is an open ruling for the user, stated as one rather than assumed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 57 +++++++++ docs/lsp-file-watcher-framing.md | 197 +++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 docs/lsp-file-watcher-framing.md diff --git a/docs/active-work.md b/docs/active-work.md index c2b19b3..c325c73 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -210,6 +210,63 @@ 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) — branch OPEN, framing revision 1 AWAITING REVIEW + +**Issue #233** — https://github.com/levineuwirth/pmacs/issues/233. +**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 1, DRAFT.** + 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. +- **NO IMPLEMENTATION MAY START** until revision 1 is approved *and* the + open ruling below is answered. + +**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. +- **OPEN RULING, blocking the start of implementation:** 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. If the acceptance bar is "the + modeline stops flipping", this lane does not meet it and **D3 must be + framed first**. +- **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` plus the + touched LSP acceptance suites. 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..04eae7c --- /dev/null +++ b/docs/lsp-file-watcher-framing.md @@ -0,0 +1,197 @@ +# LSP file watcher — framing + +**Status: revision 1 — DRAFT, awaiting review. No implementation may +begin from this document.** + +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. + +## 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). From ed3033c1fb70628cc2c65747588135cc4544f5ba Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 10 Aug 2026 23:29:12 +0200 Subject: [PATCH 2/4] fix(lsp): carry the GlobPattern form so plain-string globs can match, and cancel superseded watchers (#233) Implements D1 and D2 from docs/lsp-file-watcher-framing.md (revision 1, approved 2026-08-10). The user ruled that the walking survives this lane; D3 gets its own framing. D1. resolve_watcher now returns (base, pattern, form) and the watch record carries the form. Per LSP, a plain-string glob matches the file's ABSOLUTE path while a RelativePattern's pattern is relative to its base --- and resolve_watcher discarded the distinction, so every downstream consumer matched relatively. Real servers send absolute globs: rust-analyzer was never told about any file change (all six of its globs absolute), and gopls saw go.mod but never a .go edit. The match subject is chosen in start_file_watcher --- form "absolute" matches base .. "/" .. rel, form "relative" matches rel unchanged. scan_tree still walks in relative terms; only the string handed to the matcher changes. D2. register_file_watchers now cancels the outgoing record list before file_watchers[skey][reg.id] = recs drops the only reference to it. The cancel loop is factored into cancel_watch_records, shared with unregister_file_watchers, so the two paths cannot diverge. rust-analyzer registers the same id twice with no unregister between --- previously 12 concurrent pollers, 6 permanently uncancellable. Verification, per the framing's plan. Three new fake-LSP modes and tests beside m4_24, each mutation-tested against the defect it names: - filewatchabs registers a plain-string absolute glob whose relative reading matches nothing. Red before D1 (reverting the match subject fails exactly this test), green after. - filewatchflat registers a RelativePattern without a leading **/ (*.txt at the base) --- F2's guard. Matching every form absolutely fails exactly this test, so the obvious wrong fix cannot land green. It also pins that a base-level pattern does not match into subdirectories. - filewatchrereg registers the same id twice (rust-analyzer's shape). The witness is observable polling, not table shape: f.old exists on disk before either .new event lands, so a leaked watcher at the same 250ms cadence reports it before the second positive. Reverting D2 fails exactly this test, the leaked .old event visible in .received. m4_24 stayed green under all three mutations --- the framing's F1 finding (the existing test is insensitive to D1 in both directions), confirmed rather than assumed. It is kept unchanged. The framing doc records the approval and the answered ruling; the active-work lane moves to IMPLEMENTED with the verification results. Co-Authored-By: Claude Fable 5 --- builtin/runtime/lsp.lua | 51 +++++-- docs/active-work.md | 53 +++++-- docs/lsp-file-watcher-framing.md | 9 +- src/bin/pmacs_fake_lsp.rs | 74 ++++++++++ tests/m4_acceptance.rs | 242 +++++++++++++++++++++++++++++++ 5 files changed, 398 insertions(+), 31 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 9ace580..d247d06 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 @@ -2097,22 +2109,33 @@ 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, 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). 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, "absolute" 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 +2143,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 +2167,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 c325c73..d32a95b 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -210,7 +210,7 @@ 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) — branch OPEN, framing revision 1 AWAITING REVIEW +## LSP file watcher (issue #233) — IMPLEMENTED, awaiting PR review **Issue #233** — https://github.com/levineuwirth/pmacs/issues/233. **URGENT by user ruling 2026-08-10: PR #227 is held unmerged until this @@ -220,12 +220,27 @@ 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, DRAFT.** - 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. -- **NO IMPLEMENTATION MAY START** until revision 1 is approved *and* the - open ruling below is answered. +- **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 + during review — the standing lesson from the GUI arc framing, which + spent two review rounds as an untracked file in one worktree. +- **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 + `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 @@ -251,21 +266,27 @@ that found this, and is explicitly not the fix. `resolve_watcher` returns `(base, pattern)` and discards which form it came from, so its contract has to change — not just the match subject. -- **OPEN RULING, blocking the start of implementation:** 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. If the acceptance bar is "the - modeline stops flipping", this lane does not meet it and **D3 must be - framed first**. +- **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` plus the - touched LSP acceptance suites. No `--protocol` — no wire change. +- **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) diff --git a/docs/lsp-file-watcher-framing.md b/docs/lsp-file-watcher-framing.md index 04eae7c..2fe8e0d 100644 --- a/docs/lsp-file-watcher-framing.md +++ b/docs/lsp-file-watcher-framing.md @@ -1,7 +1,9 @@ # LSP file watcher — framing -**Status: revision 1 — DRAFT, awaiting review. No implementation may -begin from this document.** +**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". 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 @@ -138,6 +140,9 @@ 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; diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 19dc0b6..9a051ac 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -332,6 +332,80 @@ 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 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..b082738 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -5351,6 +5351,248 @@ 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 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` From 072301775439144257a5a1937cf1595ac4cb04f6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 10 Aug 2026 23:30:57 +0200 Subject: [PATCH 3/4] docs: record PR #234 in the file-watcher lane The lane header carries the PR number and the ref it was opened at, so the next machine can find the review without searching GitHub. Co-Authored-By: Claude Fable 5 --- docs/active-work.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/active-work.md b/docs/active-work.md index d32a95b..aa8bcae 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -210,9 +210,11 @@ 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) — IMPLEMENTED, awaiting PR review +## 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. From 2a16e0eed56f676e5ce90f8803c0d0e1bf6756ab Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 11 Aug 2026 09:00:15 +0200 Subject: [PATCH 4/4] fix(lsp): read the glob form from the pattern, and stop a cancelled scan emitting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the #233 implementation found two correctness defects. Both are cases where the FIRST fix for #233 was itself wrong, which is worth naming: this lane repaired absolute globs and, in the same change, broke a case that had worked since May. P1 --- `resolve_watcher` returned "absolute" for EVERY string, so the form was carried but derived from the union arm rather than from the pattern. A bare `*.txt` is a valid relative pattern (LSP 3.17 defines `Pattern` relative to a base path; VS Code treats string watchers as applying across workspace folders), and classifying it absolute matched it against `/foo.txt`, which `^[^/]*%.txt$` can never match. A leading `/` is what makes a pattern absolute. The `filewatchflat` test could not catch this: it sends the RelativePattern OBJECT form, so it constrains the object arm, never the string arm the regression lived in --- the framing's own F1 finding, repeating inside the lane that named it. P2 --- `scan_tree` awaits `read_dir` once per directory, so the watcher 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 rechecked after the scan. Both tests are mutation-checked and each bite fails only its own defect. Reverting P1 fails the bare-string test alone --- the absolute test still passes, so the two readings are independent. Deleting P2's recheck reproduces the defect verbatim: `.received = "1 file:///…/foo.txt"`, a batch from a watcher that was already cancelled. P2's witness needs a seam. The race is a cancel landing during one of the scan's suspensions, which no arrangement of real timing produces on demand --- the same situation, and the same device, as `git.lua`'s `_deliver_status`. `pmacs.lsp._after_scan_for_tests` is nil in production and is handed the SCAN RESULT deliberately: a test that cancels on any other scan passes with the recheck deleted, because the loop would break at the post-sleep check and emit nothing anyway. Gates: 9/9 green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/lsp.lua | 46 +++++++-- docs/active-work.md | 34 +++++-- docs/lsp-file-watcher-framing.md | 46 ++++++++- src/bin/pmacs_fake_lsp.rs | 26 +++++ tests/m4_acceptance.rs | 168 +++++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 16 deletions(-) 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