From 9a1e11c0c8ac99ef05fadc089b6e83455c201707 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 19:31:13 +0200 Subject: [PATCH] fix(lsp): a `/` search boundary no longer disables the LaTeX marker walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `latex_within_boundary` answered a question about path COMPONENTS with string arithmetic: dir:sub(1, #boundary + 1) == boundary .. "/" With a `/` boundary the needle is `"//"`, which no canonical path begins with. Every ancestor was therefore judged out of bounds, the marker walk never examined a single directory, and each chapter of a thesis got its own root — two texlab processes for one document tree. The lane's headline behaviour, silently off, with all fourteen shipped tests still green because every one of them clamps the boundary to its own tempdir. Fixed by comparing segments rather than characters, so the root is a boundary with zero segments — containing everything by construction instead of by a special case, and tolerating a trailing separator for free. The same root-is-special trap sat at two other points on the same path and is closed with it: * `latex_parent_of` returned nil for a top-level directory, making `/` the one directory the walk could never examine — the identical bug from the far end. It now yields `/`, matching `walk_for_marker`'s `Path::ancestors` on the Rust side, and still terminates because `/` has no component to strip. * `latex_root_for` sliced `/paper.tex` to an EMPTY directory, which canonicalizes to nothing and made the resolver DECLINE — and a decline is the one path that falls through to `pmacs.project.detect`, whose walk includes `.git`. A document at the filesystem root now roots at `/`. * `latex_marker_in`'s join is guarded for `dir == "/"`, which this change makes reachable for the first time; the naive form produces `//name`, the one spelling POSIX leaves implementation-defined. Two new pins plus a strengthened one, 16 tests: * `two_chapters_share_one_server_under_a_root_search_boundary` — the defect end to end through ATTACH, not on the predicate, because the symptom is two servers rather than a wrong string. Restoring the old comparison fails exactly this test, with the two-server output. * `latex_root_walk_stops_at_the_search_boundary` now asserts BOTH directions. "Stops at the boundary" is also satisfied by a walk that never runs — which is precisely what a `/` boundary produced — so the hermeticity property (R8's shape: a stray `latexmkrc` above the tempdir must stay invisible) is now paired with the walk still climbing to, and examining, the boundary directory itself. * `latex_root_for_a_document_at_the_filesystem_root_is_the_root`. Also corrects `docs/active-work.md`: §3 no longer awaits a revision 3 — `b5eaf27` IS revision 3 — and the lane entry now records that boundary handling has been this resolver's interesting part twice, so a reader weighing whether to trust it knows where to look first. Gates: ALL GREEN via `./scripts/gate --acceptance lsp_latex_acceptance`. `/tmp` on this machine is a near-full tmpfs and three grep tests fail there with `QuotaExceeded`; the green run used `TMPDIR=/home/jeans/build/pmacs-gate-tmp/lsp-latex`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/lsp.lua | 54 +++++++++++++++++++-- docs/active-work.md | 36 ++++++++++++-- tests/lsp_latex_acceptance.rs | 89 +++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 8 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index e0a7e81..9ace580 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -337,8 +337,14 @@ local LATEX_ROOT_MARKERS = { -- establishes it: content -> no error; empty file -> nil, no error; -- directory -> nil, "Is a directory"; missing -> `io.open` nil. local function latex_marker_in(dir) + -- Joining, not testing: `/` is the one directory that already ends in + -- a separator, and `dir .. "/" .. name` would give `//name` — the + -- exactly-two-leading-slashes spelling POSIX leaves implementation- + -- defined. `/` became reachable here once the walk stopped treating + -- the filesystem root as off-limits, so the join has to say so. + local base = (dir == "/") and "" or dir for _, name in ipairs(LATEX_ROOT_MARKERS) do - local f = io.open(dir .. "/" .. name, "r") + local f = io.open(base .. "/" .. name, "r") if f then local _, err = f:read(1) f:close() @@ -348,9 +354,18 @@ local function latex_marker_in(dir) return false end +-- `/` is a directory like any other. The pattern below yields the EMPTY +-- string for a top-level directory (`/tmp` -> ``), and treating that as +-- "no parent" would make the filesystem root the one directory this walk +-- can never examine — the same root-is-special bug the boundary test +-- below had, from the other end. `/` itself matches nothing (no +-- non-separator component to strip), so the walk still terminates there. +-- This matches `walk_for_marker`'s use of `Path::ancestors` +-- (`src/project.rs`), which likewise ends at `/` inclusive. local function latex_parent_of(dir) local up = dir:match("^(.*)/[^/]+$") - if up == nil or up == dir or up == "" then return nil end + if up == nil or up == dir then return nil end + if up == "" then return "/" end return up end @@ -360,9 +375,37 @@ end -- ignored the boundary would break that contract — and make this -- resolver's own acceptance fixtures non-hermetic against any -- `latexmkrc` sitting above the test's tempdir (R8's shape exactly). +-- +-- Containment is a question about PATH COMPONENTS, so it is answered by +-- comparing components. The previous string-prefix form +-- (`dir:sub(1, #boundary + 1) == boundary .. "/"`) silently disabled the +-- entire walk for a `/` boundary: the needle became `"//"`, which no +-- canonical path begins with, so every ancestor was judged out of +-- bounds, no marker was ever examined, and each chapter of a thesis got +-- its own server. Segment comparison makes the root boundary a boundary +-- with zero segments — containing everything, by construction rather +-- than by a special case — and absorbs a trailing separator for free. +-- +-- Both arguments are canonical absolute paths (`latex_root_for` +-- canonicalizes `dir`; `set_search_boundary` canonicalizes the boundary +-- at set time), so a leading-separator mismatch cannot arise. +local function latex_path_segments(path) + local segs = {} + for seg in path:gmatch("[^/]+") do + segs[#segs + 1] = seg + end + return segs +end + local function latex_within_boundary(dir, boundary) if not boundary then return true end - return dir == boundary or dir:sub(1, #boundary + 1) == boundary .. "/" + local want = latex_path_segments(boundary) + local have = latex_path_segments(dir) + if #have < #want then return false end + for i = 1, #want do + if have[i] ~= want[i] then return false end + end + return true end -- Returns the INNERMOST ancestor holding a texlab root marker, or the @@ -382,6 +425,11 @@ local function latex_root_for(path) if type(path) ~= "string" then return nil end local dir = path:match("^(.*)/[^/]*$") if not dir then return nil end + -- Same root-is-special trap as `latex_parent_of`: `/paper.tex` slices + -- to an EMPTY directory, which canonicalizes to nothing and would make + -- the resolver DECLINE — and a decline is the one path that reaches + -- `pmacs.project.detect`, whose walk includes `.git`. + if dir == "" then dir = "/" end dir = pmacs.fs.canonicalize(dir) if not dir then return nil end local boundary diff --git a/docs/active-work.md b/docs/active-work.md index 3327aad..59783a1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,11 +333,12 @@ authoritative tip** — the ref, not a SHA. Recover with - **`args = {}` is also observed**, not assumed: bare `texlab` answers `initialize` with `TexLab 5.25.1` over stdio, so the `run` subcommand is not needed. - - **§3 therefore reads slightly stale** — it frames marker 1 as - conditional on texlab honouring the file, when the operative fact - is that texlab honours the *client-supplied root*. Worth a revision - 3 by whoever next touches the document; not smuggled into this - lane's commit. + - **§3 said the wrong thing and has been corrected — `b5eaf27` IS + revision 3.** It framed marker 1 as conditional on texlab honouring + the `.texlabroot` *file*, when the operative fact is that texlab + honours the *client-supplied root* and never widens it. The caveat + was discharged by observation, and revision 3 records what that + established. Nothing about §3 is outstanding. - **`.git` exclusion needed more than omitting it from the list.** `project_root_for` falls through to `pmacs.project.detect` when a resolver returns nil, and **that** walk includes `.git` — so a @@ -360,6 +361,31 @@ authoritative tip** — the ref, not a SHA. Recover with (1), boundary ignored (1), `io.open` truthiness so a directory counts as a marker (1), marker set narrowed (4), command renamed with opinionated settings added (1). +- **The boundary has now been the interesting part twice, and the + second time it was a real defect (fixed in review).** First it was + hermeticity — every fixture sets `set_search_boundary` at its own + tempdir because R8's shape (a stray `latexmkrc` above the tempdir) + would make the markerless assertions pass while testing nothing. + Then review found `latex_within_boundary` answering a PATH question + with string arithmetic: `dir:sub(1, #boundary + 1) == boundary .. "/"` + compares against `"//"` when the boundary is `/`, which no canonical + path matches, so a root boundary judged **every** ancestor out of + bounds, ran no marker walk at all, and gave each chapter of a thesis + its own server — the lane's headline behaviour silently off, with + every shipped test still green because each one clamps to a tempdir. + The same trap sat at the other end (`/` was never a walk candidate, + and `/paper.tex` sliced to an empty directory and declined into the + `.git`-aware detector). Now segment comparison throughout: the root + is a boundary with zero segments, contained by construction rather + than by a special case. Pinned by an ATTACH-level test under a `/` + boundary — two chapters, one server, marker root — and the + hermeticity property asserts **both** directions, since "stops at the + boundary" is also satisfied by a walk that never runs. Suite is 16 + tests. **A reader + deciding whether to trust this resolver should read it as: the marker + set and the `.git` exclusion were settled by observation and are + solid; the boundary arithmetic around them was not, and is the place + to look first if roots come back wrong.** - **Trap for the next agent in this worktree:** this machine exports a shared `CARGO_TARGET_DIR`, so a bare `cargo test` compiles against a sibling worktree's artifacts and fails with errors from code that is diff --git a/tests/lsp_latex_acceptance.rs b/tests/lsp_latex_acceptance.rs index e62a03e..4ccd138 100644 --- a/tests/lsp_latex_acceptance.rs +++ b/tests/lsp_latex_acceptance.rs @@ -488,6 +488,54 @@ fn two_chapters_of_one_thesis_share_a_single_server() { ); } +#[test] +fn two_chapters_share_one_server_under_a_root_search_boundary() { + // A `/` boundary is "clamp nothing", spelled as a path — and it used + // to disable the marker walk OUTRIGHT. The containment test was + // string arithmetic (`dir:sub(1, #boundary + 1) == boundary .. "/"`), + // so a `/` boundary asked whether each ancestor began with `"//"`, + // which no canonical path does. Every ancestor was judged out of + // bounds, no marker was ever examined, and each chapter got its own + // root — the lane's headline behaviour, silently off, with the + // predicate's unit-level answers all still looking plausible. + // + // Pinned through ATTACH because that is where the symptom lives: two + // texlab processes for one thesis, not a wrong string. + // + // Still hermetic despite the unclamped boundary: innermost marker + // wins, and `thesis/` has one, so no `latexmkrc` above the tempdir + // can change the answer. + let fx = Fixture::new(); + let mut state = editor(); + exec(&state, "pmacs.project.set_search_boundary(\"/\")"); + let seen: String = eval(&state, "return pmacs.project.search_boundary() or \"\""); + assert_eq!( + seen, "/", + "fixture precondition: the boundary must be the filesystem root" + ); + point_command_at(&state, &fake_lsp_path()); + fx.write("thesis/latexmkrc", ""); + let one = fx.write("thesis/chapters/one.tex", "\\section{One}\n"); + let two = fx.write("thesis/appendix/two.tex", "\\section{Two}\n"); + open(&state, &one); + settle(&mut state); + open(&state, &two); + settle(&mut state); + + let rows = rows(&state); + assert_eq!( + rows.len(), + 1, + "a root boundary must behave like any other boundary: both \ + chapters resolve to the thesis root, so ONE server: {rows:?}" + ); + assert_eq!( + rows[0].split('|').nth(1).unwrap(), + file_uri(&fx.dir("thesis")), + "and that one server is rooted at the marker directory" + ); +} + #[test] fn two_markerless_documents_in_different_directories_do_not_share_a_server() { // The complement of the pin above: the fallback is the file's own @@ -604,6 +652,47 @@ fn latex_root_walk_stops_at_the_search_boundary() { "the walk must not climb past the search boundary to reach the \ marker above it" ); + + // The other direction, and it is not decoration: "stops at the + // boundary" is also satisfied by a walk that never runs at all — + // which is precisely what a `/` boundary used to produce. So assert + // that within the boundary the walk still CLIMBS, and that the + // boundary directory itself is a candidate (inclusive, matching + // `set_search_boundary`'s documented contract). + fx.write("inner/.texlabroot", ""); + assert_eq!( + resolve_root(&state, &doc).as_deref(), + Some(inner.display().to_string().as_str()), + "a marker AT the boundary directory is found, and the walk \ + climbs out of `chapters/` to reach it" + ); +} + +#[test] +fn latex_root_for_a_document_at_the_filesystem_root_is_the_root() { + // The same root-is-special trap one level up: `/paper.tex` slices to + // an EMPTY directory string, which canonicalizes to nothing, so the + // resolver DECLINED — and a decline is the one path that falls + // through to `pmacs.project.detect`, whose walk includes `.git`. + // Hermetic: the boundary is this fixture's tempdir, so `/` is out of + // bounds, no marker is examined, and the answer is the directory + // itself regardless of what sits at the filesystem root. + let fx = Fixture::new(); + let state = editor(); + fx.bind(&state); + let doc = Path::new("/pmacs-lsp-latex-no-such-document.tex"); + assert!( + !doc.exists(), + "fixture precondition: {} must not exist", + doc.display() + ); + + assert_eq!( + resolve_root(&state, doc).as_deref(), + Some("/"), + "a document at the filesystem root roots at `/`; it must not \ + decline into the shared `.git`-aware detector" + ); } #[path = "common/iso.rs"]