diff --git a/docs/active-work.md b/docs/active-work.md index 439f9fe..3c585cf 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -245,6 +245,51 @@ the PR that retires other lanes. - **Retire this block in the next absorption after #224 merges.** It describes a docs PR; once merged there is nothing volatile left. +## R8 fixture boundary — PR #226 OPEN, HELD FOR REVIEW (test hermeticity) + +**PR #226** — https://github.com/levineuwirth/pmacs/pull/226. **Open, +awaiting review; no merge authorization.** + +**Branch `r8-fixture-boundary`**, base `githubsucks/main` @ `b833b13` +(the #224 merge). **`githubsucks/r8-fixture-boundary` is the +authoritative tip** — the ref, not a SHA. Recover with +`git fetch githubsucks && git checkout r8-fixture-boundary`. + +**Written with the lane's first commit, before the PR existed** — the +standing correction from #171 and #215, which the previous two lanes +both missed and review both caught. + +- **Framing `docs/r8-fixture-boundary-framing.md`**, revision 2, + approved 2026-08-08. +- **Scope:** `tests/m4_acceptance.rs` only — `open_against_fake` sets + `pmacs.project.set_search_boundary` to the fixture directory, plus a + portable planted-marker witness. **No `src/`, no runtime, no + product-behaviour change.** `display_path` and project detection are + deliberately untouched: shortening a location against its project + root is the feature. +- **Deliberately NOT branched from `gate-script`.** `scripts/gate` does + not exist on `main`, so naming it as a criterion would have made this + lane depend on an artifact absent from its own base. +- **Verification (all run):** the R8 test passes **with `/tmp/.git` + still present**, i.e. on the machine that reproduces it; the planted + marker witness passes, and also passes with `TMPDIR` outside `/tmp` + (the CI shape); reverting the boundary fails **both**, the witness + with `proj/r.rs:12:3` — relative to the *planted* marker, which is + what makes the bite portable; full `m4_acceptance` 151/0; `--lib` + 1920 and `--lib --features crdt` 2105; `-p pmacs-gpu` 241; fmt, + clippy, `git diff --check`; and the **full workspace sweep exit 0 + across 113 targets** — the first fully green local sweep of this + session. +- **`/tmp/.git` was NOT removed**, deliberately: deleting it would hide + the hermeticity defect, its provenance is unresolved, and it is the + only thing on this machine that reproduces the row. + +**Sequencing:** this lands first, on its own merits. **Then #225 rebases +onto it** and takes "`scripts/gate` runs green" as *its* re-gate +criterion. On that rebase, **the R8 documentation here is authoritative** +— #225 carries an earlier, pre-fix copy of the R8 registry row and lane +text from when it was still an open investigation, and those must lose. + ## Docs absorption after #217 — MERGED as #218 (2026-08-06 09:59Z) **PR #218** — https://github.com/levineuwirth/pmacs/pull/218. **This diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 8e296e0..96e25ce 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -2922,6 +2922,18 @@ round-trip cannot detect a discriminant shift. ## 6. Named deferrals (the standing backlog, consolidated) +**Fixture project-detection census — NOT STARTED, follow-on from R8.** +`tests/m4_acceptance.rs` alone constructs state through +`EditorState::new_with_roots` **113 times**, and an unknown number of +those never call `pmacs.project.set_search_boundary`. Unbounded +detection is harmless *only while the assertion does not render a +path* — which is why exactly one of them (R8) ever failed, and why the +next one will look like a fresh mystery rather than a known class. The +work is a census across the suites, not a rewrite: find the unbounded +constructions whose assertions are path-sensitive, and bound those. +`src/project.rs:208` documents the hazard and `detect_project_within` +is the mechanism; eight test files already use it. + **Capability-aware keymap resolution — CROSS-CUTTING, NOT STARTED, needs its own framing.** Named here because #220 hit its absence and worked around it, not because any of it is designed. Today diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 2b7f268..ad050e9 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -104,10 +104,11 @@ evidence is macOS / lua54 and that one is macOS / luajit. ## Live rows -**Four of the six evidenced rows are live.** R2 and R4 were retired on -2026-08-05 and are below, under "Retired rows", with their dispositions. -R5 and R6 were added on 2026-08-06 from an occurrence scan and a live -red; neither is diagnosed. +**Four of the seven evidenced rows are live.** R2 and R4 were retired +on 2026-08-05 and **R8 on 2026-08-08**; all three are below, under +"Retired rows", with their dispositions. R5 and R6 were added on +2026-08-06 from an occurrence scan and a live red; neither is +diagnosed. **Rate, as of 2026-08-06.** Of the last 25 `main` runs, **23 green and 2 red** — the two reds being R2's second occurrence (30710662474) and @@ -226,30 +227,6 @@ readiness helpers exist, whether they can be one, and what each promises. Patching this call site alone would leave the same question open under a fourth selector. -### R8 — LSP listview row renders a path relative to a root the test does not expect - -The first row here that is **not intermittent**. It reproduces on every -run, and the merge-base control has already been done — so the one -question this registry exists to answer is settled for it, and settled -against the branch that found it. - -| field | value | -|---|---| -| **selector** | `--test m4_acceptance flat_listview_consumers_render_byte_identically_after_the_tree_extension` | -| **job / flavor** | local (Linux), any invocation — isolated single-test runs included. Not load-sensitive | -| **required fragments** | `the flat references row renders verbatim` **and** a `left` value that is the `right` value with a **leading directory removed** | -| **status** | **deterministic locally, NOT attributed to the long-lines arc** | -| **what IS established** | it fails identically on `main` and on `gpu-horizontal-scroll` with the working tree stashed — a merge-base control, not an inference from "my diff looks unrelated". The rendered row is the expected path with `/tmp/` stripped: `.tmpPZsycN/r.rs:12:3` vs `/tmp/.tmpPZsycN/r.rs:12:3` | -| **what is NOT** | the mechanism. **This is a prefix strip, not a width truncation** — the fixture declares `CellSize::new(40, 100)`, so 25 characters fit with room to spare, and the missing text is at the front. The likeliest reading is that the row is rendered relative to a workspace root that resolves to `/tmp` on this machine, which would make it depend on `TMPDIR`. **Not verified**, and the alternative — that the renderer relativizes against something else entirely — is not excluded | -| **why CI is green** | unestablished. If the `TMPDIR` reading is right, a runner whose temp dir is not directly under the relativization root would never see it. That is a hypothesis, not a finding | -| **retirement** | a diagnosis of what the row is rendered relative to, then either fixing the renderer or making the assertion state the relativization it expects. A green run on a machine with a different `TMPDIR` retires nothing | - -**Not this lane's to fix, and deliberately not fixed here.** Stage 5 -touches `pmacs-gpu`, `pmacs-protocol::scroll`, and `horizontal_follow` -in `src/editor.rs`; it has no path to the LSP listview renderer. Fixing -it inside this branch would put an unrelated, undiagnosed change in a -viewport PR. - ## Retired rows **These stay here on purpose.** A retirement is a claim that a mechanism @@ -257,6 +234,67 @@ is gone; keeping the signature is what makes a recurrence recognisable as a falsification of that claim rather than as a fresh mystery. Both were retired **causally** — the mechanism removed, plus a discriminating witness that fails without the fix — never by a count of green runs. +**R8 joined them on 2026-08-08**, and unlike the other two it was never +intermittent — it was deterministic, and the "flake" reading was never +available to it. + +### R8 — LSP listview row rendered relative to a stray ancestor marker — RETIRED 2026-08-08 + +| field | value | +|---|---| +| **selector** | `--test m4_acceptance flat_listview_consumers_render_byte_identically_after_the_tree_extension` | +| **job / flavor** | local (Linux), any invocation — isolated single-test runs included. Not load-sensitive | +| **required fragments** | `the flat references row renders verbatim` **and** a `left` value that is the `right` value with a **leading directory removed** | +| **causal status** | **DIAGNOSED and FIXED — test hermeticity** | +| **evidence** | reproduced deterministically on one Linux workstation; merge-base control confirmed it on `main` | +| **disposition** | `tests/m4_acceptance.rs::open_against_fake` now sets `pmacs.project.set_search_boundary` to the fixture directory. `docs/r8-fixture-boundary-framing.md` | + +**Mechanism, established rather than guessed:** + +1. `builtin/runtime/lsp.lua:2397` `display_path` shortens a location + against the **detected project root** before rendering it. +2. `pmacs.project.detect` walks **upward** for a marker. From + `/tmp/.tmpXXXXXX/r.rs` it reached `/tmp`. +3. That machine had a stray **`/tmp/.git`** — an *empty directory*, not + a repository. The `.git` marker is directory-only, so an empty + directory still matched. +4. Root resolved to `/tmp`, the prefix was stripped, and the row + rendered as observed. + +Control at diagnosis time: the same test with `TMPDIR` outside `/tmp` +passed. + +**The product behaviour was never wrong and was not changed.** +Shortening a location against its project root is the feature. The +defect was that the fixture did not bound its own project detection, so +its assertion depended on what the developer's `/tmp` contained. +`src/project.rs:208` had documented this exact hazard — *"a developer's +`/tmp/.git`"*, in those words — and provided +`detect_project_within`; `open_against_fake` was one helper that missed +the pattern the same file already used five times. + +**Retired causally, and the witness is portable.** A new test, +`a_planted_ancestor_marker_does_not_reach_the_rendered_row`, **plants +an empty `.git` in a temporary ancestor** and asserts the rendered row +stays absolute. Removing the boundary fails it *deterministically on +every machine* — including CI, where no `/tmp/.git` exists — because +the planted marker is nearer than any real one. **The original +machine's stray directory was corroboration, never the proof**, and it +was deliberately left in place: deleting it would have hidden the +hermeticity defect. + +**Provenance of that `/tmp/.git` remains unresolved and is not needed.** +Observations of its timestamps disagreed, and `/tmp` is a tmpfs whose +entries are touched by inspection, so no timestamp is authoritative +here. It may have been created by the session that found the row. The +fix does not depend on the answer. + +**What this retirement does NOT claim:** that the rest of the suite is +hermetic. `EditorState::new_with_roots` is called **113 times** in +`m4_acceptance` alone, an unknown number of them equally unbounded — +harmless only while their assertions do not render a path. That census +is a named follow-on in `docs/agent-handoff.md` §6, not a completed +audit. ### R2 — USR1 delivered before the trap is installed — RETIRED 2026-08-05 diff --git a/docs/r8-fixture-boundary-framing.md b/docs/r8-fixture-boundary-framing.md new file mode 100644 index 0000000..4c50ed7 --- /dev/null +++ b/docs/r8-fixture-boundary-framing.md @@ -0,0 +1,213 @@ +# R8 — the fixture boundary the LSP panel tests never set + +**Status: revision 2 — APPROVED 2026-08-08 and IMPLEMENTED. PR #226 +open (`https://github.com/levineuwirth/pmacs/pull/226`), held for +review; not merged.** The verification in §5 is no longer a plan: every +item was run, and the results are recorded in the lane in +`docs/active-work.md` and in the retired R8 row of +`docs/ci-red-signatures.md`. + +**Revision 2 corrects two counts I got wrong by miscounting, adds the +portable witness revision 1 lacked, and bounds Q#R8-1's answer.** The +counts came from raw `grep -c`, which counted a function definition, a +comment, a test *name* and two assertion strings as if they were calls +— the same class of error as trusting a test-name match instead of a +signature, which this project has a whole registry about. + +Test hermeticity. **Not coherence-affecting** — no journey step, no +interaction island, no config-registry setting, no background work +(`COHERENCE.md` §20). Named so the absence is a statement. + +--- + +## 1. What is wrong, and what is not + +`docs/ci-red-signatures.md` **R8** fails deterministically on this +machine: + +``` +assertion `left == right` failed: the flat references row renders verbatim + left: ".tmpPZsycN/r.rs:12:3" + right: "/tmp/.tmpPZsycN/r.rs:12:3" +``` + +Diagnosed 2026-08-08: + +1. `builtin/runtime/lsp.lua:2397` `display_path` shortens a location + against the **detected project root** before rendering it. +2. `pmacs.project.detect` walks **upward** for a marker. From + `/tmp/.tmpXXXXXX/r.rs` it reaches `/tmp`. +3. This machine has a stray **`/tmp/.git`** — an empty directory, not a + repository. The `.git` marker is directory-only, so it matches. +4. Root resolves to `/tmp`; the prefix is stripped; the row renders as + observed. + +Control: the same test with `TMPDIR` outside `/tmp` **passes**. + +**The product behaviour is correct and is not being changed.** Shortening +a location against its project root is the feature. A file that really +does sit inside a project really should render relative to it. + +**The defect is that the fixture does not bound its own project +detection**, so its assertion depends on whether the developer's `/tmp` +happens to contain a `.git`. That is a hermeticity bug in the test, and +it is what this lane fixes. + +## 2. The mechanism already exists, and this suite already uses it + +`src/project.rs:208` documents `detect_project_within(start, markers, +stop_root)` as existing + +> *"so a stray marker in a temp-dir's ancestor (e.g. a developer's +> `/tmp/.git`) can't leak into a fixture that lives below it."* + +It is reachable from Lua as `pmacs.project.set_search_boundary(path)` +(`src/lua_bindings/mod.rs:12308`), and **eight test files already call +it** — fourteen real calls between them, of which +`tests/m4_acceptance.rs` holds **five**. One of those five carries the +comment *"(a developer's `/tmp/.git`, say) can't masquerade as the +root."* + +(Revision 1 said nine, from a raw `grep -c` that also counted a +comment, the name of a test *about* the binding, and two assertion +message strings.) + +So this is not a missing capability, a design question, or a new +pattern. **It is one helper that missed an established one**, and the +hazard it missed is documented by name in the same file. + +`open_against_fake` (`tests/m4_acceptance.rs:7985`) builds an +`EditorState`, declares frame geometry, points the `rust` server at the +fake, and opens the file — and never sets a boundary. + +## 3. The change + +Set the search boundary to the fixture's own temporary directory inside +`open_against_fake`, before the file is opened, matching the existing +call sites. + +**Three tests** call `open_against_fake` (`tests/m4_acceptance.rs:8048, +8137, 8261` — revision 1 said four, counting the definition at 7985). +All three must still pass, and the fix is expected to change the +rendering of exactly one — the one whose assertion spells the path out. + +## 4. Open questions + +### Q#R8-1 — the boundary's value: the file's parent, or the tempdir root? + +`open_against_fake` receives a *path*, not the `TempDir`. The parent +directory of that path is the fixture root in every current caller. + +*My vote: **the file's parent directory***, derived inside the helper. +It needs no signature change, and it is what the other call sites in +this file effectively use. + +**Its limitation, stated now rather than discovered later.** This is +correct only while fixtures put the file as a **direct child** of the +fixture root, which all three callers do. A future nested fixture — +say `/crate/src/r.rs` — that *wants* detection to reach its outer +root would not be served by this, and **passing a deeper path cannot +fix it**: the boundary is derived from the path's parent, so a deeper +path clamps the walk *sooner*, never later. That case needs an explicit +boundary argument or a second helper, and revision 1's suggestion that +a caller "can pass a deeper path" had the direction backwards. + +### Q#R8-2 — is this one helper, or a census? + +The suite constructs state through `EditorState::new_with_roots` **113 +times**. An unknown number of those are equally unbounded, and the same +stray marker would affect any of them **whose assertion renders a +path**. Most do not, which is why only this one fails. + +*My vote: **fix this helper in this lane; census separately.*** A +113-site audit is not a bug fix, and bundling it would make the +regression-fixing change unreviewable. But the census should be a named +follow-on rather than a good intention — this row cost real time twice, +and the next one will look like a new mystery. + +**Recommend: file the census as a named backlog item** in +`docs/agent-handoff.md` §6 when this lands. + +### Q#R8-3 — does anything depend on the unbounded walk? + +If some test's expectation quietly relies on detection escaping its +fixture, bounding it would break that test — and that would be a +finding worth having rather than an obstacle. + +*Expected: no.* To be established by running the suite, not asserted +here. + +## 5. Verification + +### 5.1 The primary witness is PORTABLE and plants its own marker + +Revision 1 rested the bite on `/tmp/.git`, which makes the proof a +property of **this machine** — the same mistake as a test that passes +only where the developer happens to be standing. A hermeticity fix +whose only evidence is one machine's stray directory is not +demonstrated; it is anecdotal. + +So the primary witness builds the hazard itself: + +``` +/ <- an empty `.git` is PLANTED here + proj/ <- the file's parent; becomes the boundary + r.rs +``` + +- **With the helper's boundary set to `proj`**, detection examines + `proj`, finds no marker, and stops — the planted `.git` one level up + is out of reach. `display_path` finds no root and falls back, so the + rendered row is the **absolute** path. +- **Reverting the boundary** lets the walk reach ``, match the + planted marker, and strip the prefix — **deterministically, on every + machine**, with no dependence on `/tmp` or `TMPDIR`. + +That pair is the bite. It runs in CI, where `/tmp/.git` does not exist, +and it fails for the right reason if the helper regresses. + +*This is a new test rather than a rewrite of the existing one*: the +existing assertion's value is that it renders a real path verbatim, and +changing its layout to carry a planted marker would blur two purposes +into one fixture. + +### 5.2 The machine observation stays, as confirmation only + +- **The existing test passes with `/tmp/.git` still present** — on the + machine that reproduces R8, not merely on a clean one. The stray + directory is deliberately **not** removed: deleting it would hide the + hermeticity defect, and it is an external directory with unresolved + provenance. +- This is corroboration for the portable witness above, **not the bite + itself**. + +### 5.3 The rest + +- **The other two `open_against_fake` tests still pass**, and the full + `m4_acceptance` suite passes (Q#R8-3). +- **The standard gate suite**, run by hand from `docs/agent-handoff.md` + §3. + +**`scripts/gate` is deliberately NOT a criterion here.** This lane +branches from `main`, where that script **does not exist** — it is +unmerged on `gate-script` (#225). Naming it would make this lane's +verification depend on an artifact absent from its own base, and would +quietly couple two lanes that are meant to land independently. + +The sequencing instead: **R8 lands first, on its own merits. Then #225 +rebases onto it, and "`scripts/gate` runs green" becomes #225's +re-gate criterion** — which is where it belongs, since a green gate run +is the thing #225 ships. + +**What this does not prove:** that the other 113 construction sites are +hermetic. Q#R8-2's census is why. + +## 6. Not in scope + +Removing or altering `/tmp/.git` — and equally, settling its provenance. +Observations of its timestamps have disagreed, `/tmp` is a tmpfs whose +entries are touched by inspection, and nothing in this lane depends on +the answer: §5.1's witness plants its own marker precisely so the fix +does not rest on that directory at all. Changing `display_path` or project +detection semantics — the product behaviour is correct. The 113-site +census (Q#R8-2). Rebasing or merging #225, which follows this landing. diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index b271d3e..b106aee 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -7997,11 +7997,36 @@ fn open_against_fake(path: &std::path::Path) -> pmacs::editor::EditorState { pmacs::protocol::FrontendId::LOCAL, pmacs::protocol::CellSize::new(40, 100), ); + // Clamp the marker walk to the fixture directory so a stray + // ancestor marker (a developer's /tmp/.git, say) can't masquerade + // as the root — the same guard the `go`/`rooturi` fixture above + // already applies, and the hazard `src/project.rs` names in + // `detect_project_within`'s own doc comment. + // + // Without this, `display_path` in lsp.lua shortens rendered + // locations against whatever root detection finds ABOVE the + // tempdir, so an assertion that spells a path out passes or fails + // according to what is in the developer's /tmp. That is registry + // row R8, and it is why this line exists. + // + // The file's PARENT is the boundary, which is correct while + // fixtures put the file as a direct child of the fixture root — all + // three callers do. A future nested fixture that wants detection to + // reach an outer root needs an explicit argument, not a deeper + // path: the boundary is derived from the parent, so a deeper path + // clamps the walk sooner, never later. + let boundary = path + .parent() + .expect("the fixture file has a parent directory"); let fake = fake_lsp_path(); state .lua_host .lua() - .load(format!("pmacs.lsp.config.rust = {{ command = '{fake}' }}")) + .load(format!( + "pmacs.project.set_search_boundary('{}') + pmacs.lsp.config.rust = {{ command = '{fake}' }}", + boundary.display() + )) .exec() .expect("override rust config"); state @@ -8023,6 +8048,84 @@ fn open_against_fake(path: &std::path::Path) -> pmacs::editor::EditorState { state } +/// **R8's portable witness: a planted ancestor marker must not reach a +/// rendered path.** +/// +/// The row above renders a location verbatim, so it is only meaningful +/// if project detection is bounded by the fixture. It was not, and the +/// consequence was registry row **R8** — the assertion passed or failed +/// according to whether the developer's `/tmp` happened to contain a +/// `.git`, which is a property of the machine rather than of pmacs. +/// +/// This test refuses to depend on that. **It plants the hazard itself:** +/// +/// ```text +/// / <- an empty `.git` is created HERE +/// proj/ <- the file's parent; `open_against_fake` bounds to it +/// r.rs +/// ``` +/// +/// With the boundary, detection examines `proj`, finds no marker, and +/// stops — the planted marker one level up is out of reach, so +/// `display_path` finds no root and falls back to the absolute path. +/// **Remove the boundary from `open_against_fake` and this fails +/// deterministically on every machine**, including CI, where no +/// `/tmp/.git` exists. That is the bite; the machine's own stray +/// directory is corroboration, not proof. +/// +/// The product behaviour is deliberately NOT under test here — a file +/// that really is inside a project really should render relative to it. +/// What is under test is that a fixture's rendering cannot be steered +/// by whatever sits above its tempdir. +#[test] +fn a_planted_ancestor_marker_does_not_reach_the_rendered_row() { + let dir = tempfile::tempdir().expect("tempdir"); + // The hazard, planted: an EMPTY `.git` directory, which is exactly + // what the machine that first exhibited R8 had in /tmp. The `.git` + // marker is directory-only, so emptiness does not save us. + std::fs::create_dir_all(dir.path().join(".git")).expect("plant ancestor marker"); + + let proj = dir.path().join("proj"); + std::fs::create_dir_all(&proj).expect("create fixture root"); + let a_path = proj.join("r.rs"); + std::fs::write(&a_path, b"fn main() {}\n").expect("write r"); + + let mut state = open_against_fake(&a_path); + let body = |state: &pmacs::editor::EditorState| -> String { + state + .lua_host + .lua() + .load("local b = pmacs.window.buffer() return b:slice(0, b:len())") + .eval() + .expect("panel text") + }; + + state + .lua_host + .lua() + .load("pmacs.lsp.find_references()") + .exec() + .expect("invoke find_references"); + assert!( + pump_lua_flag( + &mut state, + "pmacs.describe.buffer(pmacs.window.buffer()).name == '*references*'", + 5, + ), + "the references panel opened" + ); + + let refs = body(&state); + let (_header, rows) = refs.split_once('\n').expect("header then rows"); + assert_eq!( + rows, + format!("{}:12:3", a_path.display()), + "the planted `.git` one level above the fixture root must not \ + shorten the rendered path; an unbounded walk would render this \ + as `proj/r.rs:12:3`" + ); +} + /// The *outline* panel end-to-end against the fake server's /// hierarchical documentSymbol response ("Outer" class > "inner" /// method): open, depth-indented rows, RET jump-ring visit to the