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"]