From 89ee2f9985483815eea2da155379a422104792bc Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:14:58 -0400 Subject: [PATCH 1/4] docs(tests): frame the ambient-config isolation defect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integration tests read the developer's real `~/.config/pmacs/init.lua`. The suite is green in CI and deterministically red on any machine with a real config, attributed to whatever branch is checked out — 11 of 67 in `compile_mode_acceptance` on 2026-07-30, 67/67 with an isolated `XDG_CONFIG_HOME`. The mechanism is that `src/editor.rs:770` guards config loading with `#[cfg(not(test))]`, which is set only when compiling the crate's own unit tests. An integration test links pmacs as an ordinary dependency, so the guard is inactive for all 96 of them. The hazard was identified and a mitigation was written; its scope does not match the threat. The obvious fix is unavailable: `std::env::set_var` is unsafe and the crate forbids unsafe, which the repo already knows — `Installer::root_override` exists for exactly this reason. Framing only, awaiting review round 1. No code changes. --- docs/test-ambient-config-isolation-framing.md | 237 ++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 docs/test-ambient-config-isolation-framing.md diff --git a/docs/test-ambient-config-isolation-framing.md b/docs/test-ambient-config-isolation-framing.md new file mode 100644 index 0000000..fff963e --- /dev/null +++ b/docs/test-ambient-config-isolation-framing.md @@ -0,0 +1,237 @@ +# Framing — integration tests read the developer's real config + +**Revision 1.** Status: awaiting review round 1. Proposed lane: +`test-ambient-config-isolation`, worktree `../pmacs-test-isolation`, +based on `githubsucks/main` @ `4cd4a7b` (a reading; re-measure at branch +time). + +**The suite is green in CI and red on a developer machine that has a +real `~/.config/pmacs/init.lua`.** Not flaky — deterministic, and +attributed to whatever branch happens to be checked out. + +## 0. Coherence impact (COHERENCE §20) + +- **No journey step, no user-facing behaviour.** This is test + infrastructure. +- **Serves §9 (worker model) indirectly**: a gate that fails for reasons + unrelated to the change under test destroys the signal the gate exists + to give. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**; under §25 no COHERENCE + edit rides this PR. + + +## 1. Ground truth (verified at `4cd4a7b`) + +### 1.1 The observation + +On 2026-07-30, `cargo test --all-targets --no-default-features --features +lua54` on a developer machine failed **11 of 67** in +`compile_mode_acceptance`, every one with: + +``` +[@/home/jeans/.config/pmacs/init.lua] command "find-file" is already +defined (refusing to overwrite) +stack traceback: + [C]: in field 'define' + /home/jeans/.config/pmacs/init.lua:3: in main chunk +``` + +Failing tests: `acc14`, `acc24`, `acc25a`, `acc27`, `acc29`, `r1f2`, +`r1f5`, `r2f1`, `r3f1`, `r4f1` (×2). With `XDG_CONFIG_HOME` pointed at an +empty directory the same suite is **67/67**. + +### 1.2 The mechanism, and why the existing guard misses + +`src/editor.rs:770` guards user-config loading: + +```rust +#[cfg(not(test))] +{ + crate::config::load_user_config(&mut lua_host); + lua_host.set_init_complete(); +``` + +with a comment stating the intent exactly: *"Skipped under `cfg(test)` so +the lib's own test suite doesn't pick up the developer's real +`~/.config/pmacs/init.lua` and turn into a flaky environment-dependent +run."* + +**`cfg(test)` is set only when compiling the crate's own unit tests.** +An integration test in `tests/` links `pmacs` as an ordinary dependency, +compiled *without* `cfg(test)` — so the guard is inactive for every one +of them. `cargo test --lib` is protected; `cargo test --test ` is +not. + +The hazard was identified, a mitigation was written, and its scope does +not match the threat. That is the finding — not that nobody thought +about it. + +### 1.3 There are two populations, and they need different fixes + +- **Spawned.** The test launches `pmacs --daemon` as a child process. + Isolation works by setting the child's environment. +- **In-process.** The test constructs `pmacs::editor::EditorState` + directly in the test binary. `compile_mode_acceptance` is this kind + (`tests/compile_mode_acceptance.rs:14`). + +**The spawned case is already solved, once.** `tests/m5_7_acceptance.rs:132`: + +```rust +fn spawn_pmacs_daemon(socket_path: &Path) -> Child { + // Isolate user config: HOME and XDG_CONFIG_HOME both point at the + // (currently empty) socket parent directory, so the daemon won't + // try to read the developer's real `init.lua`. + ... + .env("HOME", isolated_home) + .env("XDG_CONFIG_HOME", isolated_home) +``` + +Both variables, not just one — `config_dir` falls back from +`XDG_CONFIG_HOME` to `$HOME/.config`, so setting one alone leaves the +other path live. + +### 1.4 In-process tests cannot isolate themselves + +The obvious fix — set the variable in test setup — **is unavailable**. +`std::env::set_var` is `unsafe` in the 2024 edition and the crate is +`#![forbid(unsafe_code)]`. + +This is already established in the codebase rather than inferred: +`src/packages/installer.rs:368` carries a `root_override` field +explicitly *because* of it — *"the project forbids `unsafe_code`, so +mutating `XDG_DATA_HOME` directly is not an option"*. + +So an in-process test can only be isolated by (a) the environment it is +launched with, or (b) an injection point in the code under test. There +is precedent for (b) in the same repository. + +### 1.5 Scale + +96 files in `tests/`. **18** reference `EditorState`, `Editor::new`, +`load_user_config`, or `TestDaemon` and are therefore candidates. Only a +handful set any isolating variable today. + +**The 18 is a candidate count from a name-based grep, not a census.** +It is an upper bound on nothing and a lower bound on nothing; §3 Bet 1 +replaces it with a real classification. Recorded this way deliberately — +a previous lane in this repo built a classification from a truncated +grep and misclassified five rows. + +### 1.6 What is NOT established + +- **Whether any test *writes* into the developer's real directories.** + `XDG_DATA_HOME` and `XDG_STATE_HOME` back autosave + (`src/autosave.rs`), minibuffer history (`src/minibuffer.rs:724`), + builtin packages (`src/builtin_packages.rs:142`) and the package + installer (`src/packages/installer.rs:96`) — all with `HOME` + fallbacks. Read-only pollution is a failed gate; *write* pollution + would touch real user data. **No such write has been observed**, and + this lane does not claim one. Bet 2 goes looking, because the cost of + being wrong is asymmetric. +- **Whether CI is genuinely unaffected**, as opposed to merely having no + config today. A CI image that ever grows a `$HOME/.config/pmacs` would + break the same way, silently. +- **Whether the 11 failures are the whole blast radius.** The run + aborted at the first failing binary, so every suite ordered after + `compile_mode_acceptance` never executed. + + +## 2. Questions + +- **Q#TI1** — Should the `cfg(test)` guard be widened, or should + isolation be the test harness's job? *Proposed: the harness's. + Widening the guard means production code deciding it is under test, + which is exactly the shape that lets a test pass against behaviour + production never runs.* +- **Q#TI2** — For in-process tests, injection point or launched + environment? *Proposed: an explicit constructor that does not load + user config, following `Installer::root_override`'s precedent. A + wrapper script that sets the variable fixes the symptom for whoever + remembers to use it.* +- **Q#TI3** — Should CI arm a check that the isolation is real? + *Proposed: yes — otherwise this recurs the moment a CI image grows a + config file, and recurs invisibly.* +- **Q#TI4** — Does anything write outside its temp dir? *Unknown; Bet 2.* + + +## 3. Bets + +- **Bet 1 — the population is classifiable.** Every file in `tests/` + is classified as spawned, in-process, or neither, by reading each + candidate's construction site rather than by grepping for a name. + - *Falsified if* a file is both, or constructs the editor indirectly + through a helper that hides which it is. Then the classification is + reported with that ambiguity rather than forced. + +- **Bet 2 — the exposure is read-only.** A test run under an + instrumented `HOME`/`XDG_*` pointing at a fresh directory leaves no + writes behind. + - *Falsified if* anything appears there — which upgrades this lane's + priority sharply, from "gates lie locally" to "tests touch real user + data". + - **A clean result is evidence about the suites that ran**, not a + guarantee; the run must be recorded with which binaries executed. + +- **Bet 3 — isolation is verifiable by a test that fails without it.** + A positive control: a fixture writes an `init.lua` that would break a + known assertion, and the suite stays green because the isolation holds. + - *Falsified if* no such fixture can be built without the very env + mutation §1.4 rules out. Then isolation is asserted structurally + (no candidate constructs an editor without the non-loading path) and + labelled as the weaker check it is. + +- **Bet 4 — the fix does not change production behaviour.** The + non-loading constructor is additive; the existing one is untouched. + - *Falsified if* any production call site has to change. + + +## 4. Acceptance + +1. A classification of every `tests/*.rs` file into spawned / + in-process / neither, with counts stated and the method named + (read, not grepped). +2. In-process tests construct the editor through a path that does not + load user config, by explicit choice at the call site rather than by + a `cfg` the caller cannot see. +3. Spawned tests set **both** `HOME` and `XDG_CONFIG_HOME`, following + `m5_7_acceptance.rs:132`. Any that set only one are fixed. +4. `compile_mode_acceptance` passes with a real + `~/.config/pmacs/init.lua` present that defines `find-file` — the + exact condition that produced §1.1. +5. Bet 2's write-probe result recorded, listing which binaries ran. +6. `cfg(test)`-only guards are not widened into a + production-decides-it-is-under-test shape (Q#TI1). +7. The `src/editor.rs:770` comment is corrected: it claims a protection + it does not provide for integration tests. +8. README or the handoff records that a local full-suite run needs an + isolated `XDG_CONFIG_HOME` until this lands, so the next person does + not spend the afternoon I did attributing 11 failures to their branch. + + +## 5. Parked + +- **Any change to how production resolves config paths.** Out of scope; + the defect is in the tests. +- **The `crdt` half of the corpus being dark in CI** — a different + coverage hole with its own lane in `docs/active-work.md`. +- **Whether CI should install a hostile `init.lua` deliberately** to + keep this honest. Attractive, but it is a CI-policy decision and this + lane is already load-bearing enough. + + +## 6. Gates + +Standard suite, each its own step with a real exit status and nothing +after the command that could mask it. **Run twice: once with an isolated +`XDG_CONFIG_HOME`, and once with a deliberately hostile `init.lua` in +place.** A lane about ambient state that is only ever verified in a clean +environment has not been verified at all. + + +## 7. Branch plan + +One branch, one PR. Bet 1's classification first and alone — it decides +how large the change is, and its answer belongs in review before any +mechanical edit rides on it. From f2071ea02200d02d7ae10fe63c2064697666682c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:36:23 -0400 Subject: [PATCH 2/4] =?UTF-8?q?docs(tests):=20framing=20revision=202=20?= =?UTF-8?q?=E2=80=94=20close=20review=20round=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four blocking and two major findings, all accepted, all verified in the code before acceptance. The lane's scope changes: it is about ambient roots, not about init.lua. - The read-only assumption was already false. `EditorState::new` materializes bundled packages unconditionally and before config loading, into `XDG_DATA_HOME` or `$HOME/.local/share`, and `materialize_all` creates directories. Confirmed on the development machine: `~/.local/share/pmacs/builtin-packages/` exists with v0.1.0 and v1.0.0. This upgrades the lane from "local gates lie" to "tests write into real user data". - The population count was wrong and its stated method did not match the command that produced it: 18 from a grep for `Editor::new`, which does not match the real constructor `EditorState::new`. 66 of 96 files construct an editor directly. - File-level classification cannot work: 5 files are both in-process and spawned. Classification moves to construction sites. - The seam must cover `EditorState::open`, which calls `Self::new()` directly, while `journey_acceptance` requires that exact public entry point to avoid a dead-production-path test. Resolved by isolating journey through its launched environment rather than a different call. - Isolated construction must still return `is_init_complete() == true`. Config loading and `set_init_complete()` share one block, and `m8_2_acceptance.rs:75` documents its dependence on integration-test construction being init-complete — the `cfg(test)` gap is load-bearing in that one respect. - Revision 1 both proposed and parked a hostile-config CI leg. Resolved in favour of a test-binary self-spawn, which travels with the test rather than the workflow file, and which now also asserts the hostile root is unmodified afterwards. - The lane is recorded in `docs/active-work.md`, which revision 1 omitted despite the volatile-work protocol requiring it. Also documents six ambient roots where the shared daemon harness sets two, and why setting HOME only isolates a root whose XDG variable is unset — the harness's apparent adequacy is a property of one developer's environment. Framing only. No code. --- docs/active-work.md | 36 ++ docs/test-ambient-config-isolation-framing.md | 318 +++++++++++++----- 2 files changed, 268 insertions(+), 86 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index a863a4f..6828f81 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -743,6 +743,42 @@ review correction. projection.** Whichever is framed second re-scouts the other's landed state. +## Test ambient-root isolation — FRAMING OPEN, revision 2 + +- **Branch `test-ambient-config-isolation`**, worktree + `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b`. + **Framing only; no code, no PR yet.** + `docs/test-ambient-config-isolation-framing.md` revision 2, one review + round closed (four blocking, two major, all accepted). +- **What it is.** Integration tests use the developer's real ambient + roots. `#[cfg(not(test))]` guards config loading against the crate's + own unit tests only, so all 96 files in `tests/` load the real + `init.lua` — and, separately, `EditorState::new` materializes bundled + packages unconditionally into the real `XDG_DATA_HOME`/`$HOME`. + **It is not read-only**: `~/.local/share/pmacs/builtin-packages/` + exists on the development machine. +- **Why it matters now.** `cargo test` is red on any machine with a real + `~/.config/pmacs/init.lua` (11 of 67 in `compile_mode_acceptance`) and + green in CI, so the failure is attributed to whatever branch is + checked out. Every local gate run in this repo currently needs an + isolated `XDG_CONFIG_HOME` as a workaround. +- **Round 1's four blocking findings, all confirmed in code:** the + read-only assumption was already false; the population count was 18 + when 66 of 96 files construct an editor, from a grep that did not + match `EditorState::new`; 5 files are both in-process and spawned, so + a file-level partition cannot work; and `EditorState::open` calls + `Self::new()` while `journey_acceptance` requires that exact entry + point. +- **Two constraints any fix must respect.** `std::env::set_var` is + `unsafe` and the crate forbids it, so in-process tests cannot isolate + themselves (precedent: `Installer::root_override`). And config loading + shares one block with `set_init_complete()`, which + `m8_2_acceptance.rs:75` explicitly depends on — skipping the block + would leave integration tests permanently in the init phase. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-test-isolation + test-ambient-config-isolation`. + ## Folding lane (Arc 6) — Stages 1 and 2 MERGED; Stage 3 (GPU) is next Both shipped stages are on `main`; nothing in this arc is in flight. Stage 3 diff --git a/docs/test-ambient-config-isolation-framing.md b/docs/test-ambient-config-isolation-framing.md index fff963e..d244223 100644 --- a/docs/test-ambient-config-isolation-framing.md +++ b/docs/test-ambient-config-isolation-framing.md @@ -1,6 +1,6 @@ -# Framing — integration tests read the developer's real config +# Framing — integration tests use the developer's real ambient roots -**Revision 1.** Status: awaiting review round 1. Proposed lane: +**Revision 2.** Status: awaiting review round 2. Proposed lane: `test-ambient-config-isolation`, worktree `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b` (a reading; re-measure at branch time). @@ -9,6 +9,41 @@ time). real `~/.config/pmacs/init.lua`.** Not flaky — deterministic, and attributed to whatever branch happens to be checked out. +**And it is not only reads.** Review round 1 falsified revision 1's +central assumption: integration tests **write into the real user data +directory**. The lane is therefore about *ambient roots*, not about +`init.lua`. See §1.6. + +## Revision history + +**Revision 1 → 2**, after review round 1 (four blocking, two major). All +six accepted; all six verified in the code before acceptance. + +- **Rev 1's "the exposure is read-only" was already false** (§1.6). + `EditorState::new` materializes bundled packages *before* config + loading and unconditionally, and `materialize_all` creates + directories. Confirmed on the development machine: + `~/.local/share/pmacs/builtin-packages/v1.0.0` exists. A + non-config-loading constructor does not fix this. +- **Rev 1's population count was wrong, and its stated method did not + match the command that produced it** (§1.5). It reported 18 candidate + files from a grep for `Editor::new`, while its prose described a + broader pattern — and the real constructor is `EditorState::new`, + which `Editor::new` does not match. **66 of 96** test files construct + an editor directly. +- **File-level classification cannot work** (§1.5). At least **5** files + are both in-process and spawned. +- **The seam must cover `EditorState::open`, not only `new`** (§1.7). + `open` calls `Self::new()` directly, and `journey_acceptance` requires + that exact public entry point on purpose. +- **Isolated construction must still finish initialization** (§1.8). + Config loading and `set_init_complete()` share one conditional block, + and `m8_2_acceptance` *documents* its dependence on integration-test + construction being init-complete. +- **Rev 1 contradicted itself on the regression guard** (§2 Q#TI3 vs §5) + and **never added its lane to `docs/active-work.md`**, which the + repository's volatile-work protocol requires. + ## 0. Coherence impact (COHERENCE §20) - **No journey step, no user-facing behaviour.** This is test @@ -107,118 +142,229 @@ So an in-process test can only be isolated by (a) the environment it is launched with, or (b) an injection point in the code under test. There is precedent for (b) in the same repository. -### 1.5 Scale +### 1.5 Scale, measured -96 files in `tests/`. **18** reference `EditorState`, `Editor::new`, -`load_user_config`, or `TestDaemon` and are therefore candidates. Only a -handful set any isolating variable today. +96 files in `tests/`. **66** construct an editor directly +(`EditorState::new` or `EditorState::open`). **5** are *both* +in-process and spawned — `vterm_stage3_acceptance` constructs an editor +at `:159` and spawns a daemon at `:665`. -**The 18 is a candidate count from a name-based grep, not a census.** -It is an upper bound on nothing and a lower bound on nothing; §3 Bet 1 -replaces it with a real classification. Recorded this way deliberately — -a previous lane in this repo built a classification from a truncated -grep and misclassified five rows. +**Revision 1 said 18, and its stated method did not match the command +that produced it.** The prose named a broad pattern; the command grepped +`Editor::new`, which does not match `EditorState::new` — the actual +constructor. So the number was ~4x low *and* described a different +search than the one run. Rev 1 hedged the number as "a candidate count, +not a census" while leaving both the figure and the description wrong; +a disclaimer on a bad measurement does not make it a good one. -### 1.6 What is NOT established +**Consequence for the design:** classification must be per *construction +site* or per *test case*, never per file. A mutually exclusive file +partition cannot represent the 5 mixed files, and acceptance 1 of +revision 1 required exactly that partition. + +### 1.6 The exposure is NOT read-only — this is established + +`EditorState::new` materializes bundled packages **before** config +loading and **unconditionally** — outside any `cfg` guard +(`src/editor.rs:730`): + +```rust +let bundled_root = crate::builtin_packages::bundled_runtime_dir(); +let bundled_packages = crate::builtin_packages::materialize_all(&bundled_root) + .expect("materialize bundled packages"); +``` + +`bundled_runtime_dir()` resolves `XDG_DATA_HOME`, else `$HOME/.local/share` +(`src/builtin_packages.rs:142,148`), and `materialize_all` **creates +directories and writes package files** (`:174`). + +**Confirmed on the development machine:** +`~/.local/share/pmacs/builtin-packages/` exists containing `v0.1.0` and +`v1.0.0`. Whatever produced those, the write path is live and reachable +from every in-process test, none of which override `HOME` or +`XDG_DATA_HOME` at all. + +So revision 1's Bet 2 was not a question to investigate — it was already +answered, in the direction that matters. This upgrades the lane from +"local gates lie" to **"tests write into real user data"**. + +### 1.6a Six ambient roots, and the harness covers two + +`src/` reads: `HOME` (9 sites), `XDG_DATA_HOME`, `XDG_CONFIG_HOME`, +`XDG_STATE_HOME`, `XDG_CACHE_HOME`, `XDG_RUNTIME_DIR`, and +`PMACS_STATE_HOME`. + +The shared harness `spawn_daemon_process_with_env` +(`tests/common/daemon.rs:154`) sets **`HOME` and `XDG_CONFIG_HOME` +only**. A spawned daemon therefore inherits the developer's real +`XDG_DATA_HOME`, `XDG_STATE_HOME`, `XDG_CACHE_HOME` and +`PMACS_STATE_HOME`. + +**Setting `HOME` isolates a root only when the corresponding `XDG_*` +variable is unset**, because `HOME` is the *fallback*. On this machine +`XDG_DATA_HOME` happens to be unset, so `HOME` does cover it — which +means the harness's apparent adequacy is a property of one developer's +environment, not of the harness. On a machine that exports +`XDG_DATA_HOME`, spawned daemons write to the real one. + +### 1.7 The seam must cover `open`, not only `new` + +`EditorState::open` calls `Self::new()` directly (`src/editor.rs:944`), +so a non-loading *constructor* leaves every open-path test unisolated. + +It cannot simply be bypassed. `tests/journey_acceptance.rs:16` requires +that exact public entry point, and says why: *"A directory arm with no +production caller passes every direct-call test, so step 3 goes through +`EditorState::open` — the same function `pmacs FILE` calls."* The +golden-journey ratchet and the isolation seam pull in opposite +directions, and the framing must resolve it rather than pick one. + +### 1.8 Isolated construction must still finish initialization + +Config loading and `set_init_complete()` share **one** conditional block +(`src/editor.rs:769-773`). Factoring by skipping the block would leave +every integration test permanently in the init phase, changing package +APIs and startup-only config behaviour. + +**This is not hypothetical — it is already depended upon.** +`tests/m8_2_acceptance.rs:75` documents it: + +> `EditorState::new()` sets the init-complete flag during startup (the +> integration-test build doesn't get the `cfg(test)` guard that lib +> tests do), so we reopen the init phase before `install_local`. + +So the `cfg(test)` gap §1.2 calls a defect is, in this one respect, +load-bearing behaviour another suite was written against. Any fix must +skip **ambient reads** while still returning `is_init_complete() == true`. + +### 1.9 What is still NOT established -- **Whether any test *writes* into the developer's real directories.** - `XDG_DATA_HOME` and `XDG_STATE_HOME` back autosave - (`src/autosave.rs`), minibuffer history (`src/minibuffer.rs:724`), - builtin packages (`src/builtin_packages.rs:142`) and the package - installer (`src/packages/installer.rs:96`) — all with `HOME` - fallbacks. Read-only pollution is a failed gate; *write* pollution - would touch real user data. **No such write has been observed**, and - this lane does not claim one. Bet 2 goes looking, because the cost of - being wrong is asymmetric. - **Whether CI is genuinely unaffected**, as opposed to merely having no - config today. A CI image that ever grows a `$HOME/.config/pmacs` would + config and no prior data dir today. A CI image that grows either would break the same way, silently. - **Whether the 11 failures are the whole blast radius.** The run aborted at the first failing binary, so every suite ordered after `compile_mode_acceptance` never executed. +- **What wrote `v0.1.0` and `v1.0.0`** on the development machine — a + test run, or ordinary use of pmacs. The write *path* is proven; the + provenance of those two directories is not, and this lane does not + claim it. ## 2. Questions -- **Q#TI1** — Should the `cfg(test)` guard be widened, or should - isolation be the test harness's job? *Proposed: the harness's. - Widening the guard means production code deciding it is under test, - which is exactly the shape that lets a test pass against behaviour - production never runs.* -- **Q#TI2** — For in-process tests, injection point or launched - environment? *Proposed: an explicit constructor that does not load - user config, following `Installer::root_override`'s precedent. A - wrapper script that sets the variable fixes the symptom for whoever - remembers to use it.* -- **Q#TI3** — Should CI arm a check that the isolation is real? - *Proposed: yes — otherwise this recurs the moment a CI image grows a - config file, and recurs invisibly.* -- **Q#TI4** — Does anything write outside its temp dir? *Unknown; Bet 2.* +- **Q#TI1** — Widen the `cfg(test)` guard, or make isolation the + harness's job? *Proposed: neither alone. The guard must not widen + (production deciding it is under test is how a test passes against + behaviour production never runs), and §1.4 shows the harness cannot + set env in-process. The answer is an explicit **bootstrap-roots + parameter** threaded through construction.* +- **Q#TI2** — What is the seam, exactly? *Proposed: a + `BootstrapRoots` value naming the config root and the data/state/cache + roots, with a `BootstrapRoots::ambient()` used by production and a + test constructor taking an explicit one. It must cover **both** + `EditorState::new` and `EditorState::open` (§1.7), following + `Installer::root_override`'s precedent (§1.4).* +- **Q#TI3** — How does `open`-path isolation coexist with the + golden-journey ratchet? *Proposed: `open` gains a roots-taking sibling + and `journey_acceptance` keeps calling the ambient `open`, because its + purpose is to prove the production entry point is wired. Journey is + then isolated by the **environment its binary is launched with**, not + by a different call — which is the only option that preserves what the + ratchet exists to prove.* +- **Q#TI4** — Must isolated construction remain init-complete? **Yes** + (§1.8), and it is a criterion rather than an assumption. +- **Q#TI5** — What is the durable regression guard? *Proposed: a + **test-binary self-spawn under a controlled environment**, not a CI + leg. Revision 1 proposed a hostile-config CI leg in Q#TI3 and parked + that same mechanism in §5 — a contradiction. A CI leg also proves only + the branch that adds it. A self-spawning test carries its own hostile + environment and fails wherever it runs.* ## 3. Bets -- **Bet 1 — the population is classifiable.** Every file in `tests/` - is classified as spawned, in-process, or neither, by reading each - candidate's construction site rather than by grepping for a name. - - *Falsified if* a file is both, or constructs the editor indirectly - through a helper that hides which it is. Then the classification is - reported with that ambiguity rather than forced. +- **Bet 1 — the population is classifiable by construction site.** + Every `EditorState::new`/`open` call site in `tests/`, and every + daemon spawn, is classified. Files are not the unit: 5 are both + (§1.5). + - *Falsified if* sites are reached through helpers that obscure which + kind they are. Then the helper is the unit and that is stated. -- **Bet 2 — the exposure is read-only.** A test run under an - instrumented `HOME`/`XDG_*` pointing at a fresh directory leaves no - writes behind. - - *Falsified if* anything appears there — which upgrades this lane's - priority sharply, from "gates lie locally" to "tests touch real user - data". - - **A clean result is evidence about the suites that ran**, not a - guarantee; the run must be recorded with which binaries executed. +- **Bet 2 — every ambient root can be redirected without `unsafe`.** + A `BootstrapRoots` parameter covers config, data, state and cache; the + spawned harness sets all six variables of §1.6a. + - *Falsified if* any root is resolved somewhere that cannot accept the + parameter — e.g. behind a `OnceLock` initialised before construction. + **That is a real risk and is checked first**, because it decides + whether this design is possible at all. -- **Bet 3 — isolation is verifiable by a test that fails without it.** - A positive control: a fixture writes an `init.lua` that would break a - known assertion, and the suite stays green because the isolation holds. - - *Falsified if* no such fixture can be built without the very env - mutation §1.4 rules out. Then isolation is asserted structurally - (no candidate constructs an editor without the non-loading path) and - labelled as the weaker check it is. +- **Bet 3 — isolation is provable by a hostile-environment test.** A + test spawns the test binary itself with `HOME`/`XDG_*` pointing at a + directory containing an `init.lua` that would break a known assertion, + and a pre-seeded data dir. The suite stays green, and the hostile + directory is **unmodified afterwards**. + - *Falsified if* the child cannot be given a controlled environment + without the in-process `set_var` §1.4 rules out. (It can: `Command` + takes `.env`. This bet is cheap and its failure would be + informative.) + - **The unmodified-afterwards half is the write half of the + check** — a green suite that still wrote into the hostile root has + not demonstrated isolation. -- **Bet 4 — the fix does not change production behaviour.** The - non-loading constructor is additive; the existing one is untouched. - - *Falsified if* any production call site has to change. +- **Bet 4 — the fix does not change production behaviour.** Ambient + resolution stays the default; isolated construction still returns + `is_init_complete() == true` (§1.8). + - *Falsified if* any production call site changes, or if + `m8_2_acceptance`'s `reopen_init_phase_for_testing` dance stops + working. ## 4. Acceptance -1. A classification of every `tests/*.rs` file into spawned / - in-process / neither, with counts stated and the method named - (read, not grepped). -2. In-process tests construct the editor through a path that does not - load user config, by explicit choice at the call site rather than by - a `cfg` the caller cannot see. -3. Spawned tests set **both** `HOME` and `XDG_CONFIG_HOME`, following - `m5_7_acceptance.rs:132`. Any that set only one are fixed. -4. `compile_mode_acceptance` passes with a real - `~/.config/pmacs/init.lua` present that defines `find-file` — the - exact condition that produced §1.1. -5. Bet 2's write-probe result recorded, listing which binaries ran. -6. `cfg(test)`-only guards are not widened into a - production-decides-it-is-under-test shape (Q#TI1). -7. The `src/editor.rs:770` comment is corrected: it claims a protection - it does not provide for integration tests. -8. README or the handoff records that a local full-suite run needs an - isolated `XDG_CONFIG_HOME` until this lands, so the next person does - not spend the afternoon I did attributing 11 failures to their branch. +1. A classification of every editor-construction site and daemon spawn + in `tests/`, by **site**, with mixed files represented explicitly and + counts stated. Method named (read, not grepped) — §1.5 is what a + grep-shaped answer costs. +2. A `BootstrapRoots`-style parameter covering config, data, state and + cache roots, reachable from **both** `EditorState::new` and + `EditorState::open` (§1.7). +3. Isolated construction returns `is_init_complete() == true`, pinned by + a test that fails if the init flip is skipped along with the ambient + reads (§1.8). +4. `journey_acceptance` still drives the ambient `EditorState::open`; + its isolation comes from the launched environment. The ratchet's + production-entry-point discipline is unweakened, and this is asserted + rather than asserted-to-be-obvious. +5. The shared harness `spawn_daemon_process_with_env` sets **all six** + roots of §1.6a, not two. +6. `compile_mode_acceptance` passes with a real + `~/.config/pmacs/init.lua` defining `find-file` present — the exact + condition of §1.1. +7. A hostile-environment self-spawn test (Bet 3) that asserts both + green **and** an unmodified hostile root. +8. `cfg(test)`-only guards are not widened (Q#TI1). +9. The `src/editor.rs:770` comment is corrected: it claims a protection + it does not provide for integration tests, and says nothing about the + unconditional package materialization above it. +10. This lane is recorded in `docs/active-work.md` with its branch and + worktree, per the volatile-work protocol — revision 1 omitted it. +11. README or handoff records that a local full-suite run needs isolated + roots until this lands. ## 5. Parked -- **Any change to how production resolves config paths.** Out of scope; - the defect is in the tests. -- **The `crdt` half of the corpus being dark in CI** — a different - coverage hole with its own lane in `docs/active-work.md`. -- **Whether CI should install a hostile `init.lua` deliberately** to - keep this honest. Attractive, but it is a CI-policy decision and this - lane is already load-bearing enough. +- **Any change to how production resolves ambient roots.** The default + stays ambient; only construction gains a parameter. +- **A hostile-config CI leg.** Superseded by Q#TI5's self-spawn, which + is strictly stronger — it travels with the test rather than with the + workflow file. Recorded because revision 1 both proposed and parked + it. +- **Cleaning up whatever already wrote into + `~/.local/share/pmacs/builtin-packages/`.** Out of scope; this lane + stops the writes, it does not audit the past. +- **The `crdt` half of the corpus being dark in CI** — separate lane. ## 6. Gates From e9613c7d947f89cc6969fec410d728d7c77f8361 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:45:15 -0400 Subject: [PATCH 3/4] =?UTF-8?q?docs(tests):=20framing=20revision=203=20?= =?UTF-8?q?=E2=80=94=20close=20review=20round=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blocking and two major findings, all accepted. Two were verified by running the thing rather than reading it. - **Journey isolation had no executable mechanism.** Cargo launches each integration-test binary with the caller's environment, and a binary cannot re-point its own roots before its tests run — the `set_var` prohibition applies to itself. Revision 2's "isolated by its launched environment" therefore assumed the external wrapper this lane exists to delete. Now named and pinned: each journey test re-execs `current_exe()` with `--exact`, a marker, and controlled roots; the child runs the real body against the ambient `EditorState::open`, so the production-entry-point ratchet is untouched while the roots are contained. The same helper serves the hostile-environment check. - **One self-spawning test is not a ratchet.** It proves the seam works and cannot notice a raw `EditorState::new()` added to another binary later. A checked source inventory, falsifiable by adding an ambient constructor, is now acceptance 12. - **The root list and the gates disagreed with the audit.** The scope is now explicit — bootstrap STORAGE roots only (config, data, state, cache). `HOME`'s non-storage semantics are excluded by decision, not omission: `expand_tilde` resolves user-entered `~` and `find_file_acceptance` pins it deliberately, so redirecting `HOME` would retarget a user-facing feature. `XDG_RUNTIME_DIR` addresses sockets, not stored data. - **The gate instruction itself was insufficient**, and this is the finding with immediate consequences: isolating only `XDG_CONFIG_HOME` stops the reads and leaves the write path open. Every local gate run in this repository today had that hole. - **The count was one high and the ledger overstated it further.** 65 files call the constructor; 66 mention it. The 66th, `m5_6_acceptance`, mentions it only to say it deliberately does not use it — making it the third place in the suite documenting the `cfg(test)` gap. The ledger's "all 96 test files load the real config" was false. - **The recovery command did not work**, verified by running it: `git worktree add ` fails with `fatal: invalid reference` after a bare fetch. Replaced with the explicit tracking-branch form. Framing only. No code. --- docs/active-work.md | 24 ++-- docs/test-ambient-config-isolation-framing.md | 127 ++++++++++++++++-- 2 files changed, 132 insertions(+), 19 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 6828f81..c57c82f 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -748,12 +748,12 @@ review correction. - **Branch `test-ambient-config-isolation`**, worktree `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b`. **Framing only; no code, no PR yet.** - `docs/test-ambient-config-isolation-framing.md` revision 2, one review - round closed (four blocking, two major, all accepted). + `docs/test-ambient-config-isolation-framing.md` revision 3, two review + rounds closed (seven blocking, four major, all accepted). - **What it is.** Integration tests use the developer's real ambient roots. `#[cfg(not(test))]` guards config loading against the crate's - own unit tests only, so all 96 files in `tests/` load the real - `init.lua` — and, separately, `EditorState::new` materializes bundled + own unit tests only, so the **65** files in `tests/` that construct an + editor load the real `init.lua` — and, separately, `EditorState::new` materializes bundled packages unconditionally into the real `XDG_DATA_HOME`/`$HOME`. **It is not read-only**: `~/.local/share/pmacs/builtin-packages/` exists on the development machine. @@ -764,7 +764,7 @@ review correction. isolated `XDG_CONFIG_HOME` as a workaround. - **Round 1's four blocking findings, all confirmed in code:** the read-only assumption was already false; the population count was 18 - when 66 of 96 files construct an editor, from a grep that did not + when 65 of 96 files construct an editor, from a grep that did not match `EditorState::new`; 5 files are both in-process and spawned, so a file-level partition cannot work; and `EditorState::open` calls `Self::new()` while `journey_acceptance` requires that exact entry @@ -775,9 +775,17 @@ review correction. shares one block with `set_init_complete()`, which `m8_2_acceptance.rs:75` explicitly depends on — skipping the block would leave integration tests permanently in the init phase. -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-test-isolation - test-ambient-config-isolation`. +- Recovery from a clean checkout — **the two-argument form does not + work**, verified by running it (`git worktree add + ` fails with `fatal: invalid reference`, because + after a bare fetch no local branch exists): + + ```sh + git fetch githubsucks + git worktree add ../pmacs-test-isolation \ + -b test-ambient-config-isolation \ + githubsucks/test-ambient-config-isolation + ``` ## Folding lane (Arc 6) — Stages 1 and 2 MERGED; Stage 3 (GPU) is next diff --git a/docs/test-ambient-config-isolation-framing.md b/docs/test-ambient-config-isolation-framing.md index d244223..107316f 100644 --- a/docs/test-ambient-config-isolation-framing.md +++ b/docs/test-ambient-config-isolation-framing.md @@ -1,6 +1,6 @@ # Framing — integration tests use the developer's real ambient roots -**Revision 2.** Status: awaiting review round 2. Proposed lane: +**Revision 3.** Status: awaiting review round 3. Proposed lane: `test-ambient-config-isolation`, worktree `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b` (a reading; re-measure at branch time). @@ -16,6 +16,32 @@ directory**. The lane is therefore about *ambient roots*, not about ## Revision history +**Revision 2 → 3**, after review round 2 (three blocking, two major). +All five accepted; all five verified before acceptance, two of them by +running the thing rather than reading it. + +- **Journey isolation had no executable mechanism** (§1.10). `cargo test + --test journey_acceptance` launches with the caller's environment and + the binary cannot isolate itself before its own tests run. Rev 2 said + journey "is isolated by its launched environment" and nothing arranged + that — which quietly assumed the external wrapper this lane exists to + delete. Now named and pinned: per-test parent/child re-exec. +- **One self-spawning test is not a ratchet** (§4.7). It proves the seam + works; it cannot notice a raw `EditorState::new()` added to a + different binary next month. A checked source inventory is now an + acceptance criterion. +- **The root list and the gates disagreed with the audit** (§1.6a, §6). + `BootstrapRoots` covered four roots while §1.6a named six, and the + revised gate still isolated only `XDG_CONFIG_HOME` — so the + "isolated" gate could still write through the real data root. **Every + local gate run in this repo today had exactly that hole.** +- **The count was one high and the ledger overstated it further** + (§1.5). 65 files, not 66; and "all 96 test files load the real config" + was false. +- **The recovery command did not work.** Verified by running it: + `git worktree add ` fails with `fatal: + invalid reference`. + **Revision 1 → 2**, after review round 1 (four blocking, two major). All six accepted; all six verified in the code before acceptance. @@ -144,10 +170,19 @@ is precedent for (b) in the same repository. ### 1.5 Scale, measured -96 files in `tests/`. **66** construct an editor directly -(`EditorState::new` or `EditorState::open`). **5** are *both* -in-process and spawned — `vterm_stage3_acceptance` constructs an editor -at `:159` and spawns a daemon at `:665`. +96 files in `tests/`. **65** contain an actual `EditorState::new()` or +`EditorState::open(` **call**. **5** are *both* in-process and spawned — +`vterm_stage3_acceptance` constructs an editor at `:159` and spawns a +daemon at `:665`. + +**66 files match the bare name; 65 call it.** The 66th is +`tests/m5_6_acceptance.rs`, which mentions `EditorState::new` only to +say it deliberately does *not* use it (`:94`): *"Tests can't go through +`EditorState::new` here because the integration-test build doesn't have +`cfg(test)` set on the lib."* That makes it the **third** place in the +suite documenting the §1.2 gap — after `m8_2_acceptance` and the +`src/editor.rs` comment itself. A grep for a name counts mentions; only +reading counts calls. **Revision 1 said 18, and its stated method did not match the command that produced it.** The prose named a broad pattern; the command grepped @@ -207,6 +242,51 @@ means the harness's apparent adequacy is a property of one developer's environment, not of the harness. On a machine that exports `XDG_DATA_HOME`, spawned daemons write to the real one. +### 1.6b Scope: bootstrap STORAGE roots only + +`BootstrapRoots` covers **config, data, state and cache** — the roots +that decide *where pmacs stores things at startup*. It does **not** +cover: + +- **`HOME`'s non-storage semantics.** `expand_tilde` + (`src/editor_core.rs:5457`) resolves a leading `~` for ordinary path + entry, and `find_file_acceptance.rs:344` consumes `HOME` on purpose to + pin that expansion (skipping when unset). Blanket-overriding `HOME` + would silently retarget a user-facing path feature and its test. +- **`XDG_RUNTIME_DIR`**, which addresses sockets rather than stored + data. + +**Rev 2 listed six roots and proposed covering four without saying so.** +The two exclusions are deliberate and named here so the gap is a +decision rather than an oversight. A later lane may take `HOME` +semantics; this one must not, because the fix for a storage root +(redirect it) is the wrong fix for a path-expansion root (leave it and +isolate the *file* instead). + +### 1.10 Journey needs a mechanism, not an intention + +Rev 2 said `journey_acceptance` keeps the ambient `EditorState::open` +and "is isolated by the environment its binary is launched with". That +is not a mechanism. **Cargo launches each integration-test binary with +the caller's environment**, and a binary cannot re-point its own roots +before its ordinary tests run — §1.4's `set_var` prohibition applies to +itself. So under a plain `cargo test --test journey_acceptance` the +suite is ambient, and the only thing that made rev 2's sentence true was +an external environment wrapper — the very workaround this lane exists +to delete. + +**The mechanism, named:** each journey test becomes a thin parent that +re-execs `std::env::current_exe()` with `--exact `, a marker +variable, and controlled roots; the child, seeing the marker, runs the +real body and calls the **ambient** `EditorState::open`. + +That is the only shape found that satisfies both constraints at once: +the child drives the true production entry point, so +`journey_acceptance`'s ratchet discipline is untouched, while the +child's roots are controlled, so nothing reaches the developer's. The +same shape serves Bet 3's hostile-environment check, so it is one +helper, not two. + ### 1.7 The seam must cover `open`, not only `new` `EditorState::open` calls `Self::new()` directly (`src/editor.rs:944`), @@ -349,8 +429,19 @@ skip **ambient reads** while still returning `is_init_complete() == true`. unconditional package materialization above it. 10. This lane is recorded in `docs/active-work.md` with its branch and worktree, per the volatile-work protocol — revision 1 omitted it. -11. README or handoff records that a local full-suite run needs isolated - roots until this lands. +11. README or handoff records that a local full-suite run needs **all + four storage roots** isolated until this lands — naming them, since + isolating only `XDG_CONFIG_HOME` leaves the write path open. +12. **A durable adoption ratchet**, not a one-time census: a checked + source inventory that fails when a new ambient + `EditorState::new()`/`open(` appears in `tests/` outside a narrow, + named allowlist. Acceptance 1 proves today's state; this keeps it. + Falsified by adding an ambient constructor to any suite and + observing the inventory test fail. +13. The journey mechanism of §1.10 is implemented and pinned: a plain + `cargo test --test journey_acceptance`, with a hostile `init.lua` + present and no external wrapper, is green and leaves the hostile + root unmodified. ## 5. Parked @@ -370,11 +461,25 @@ skip **ambient reads** while still returning `is_init_complete() == true`. ## 6. Gates Standard suite, each its own step with a real exit status and nothing -after the command that could mask it. **Run twice: once with an isolated -`XDG_CONFIG_HOME`, and once with a deliberately hostile `init.lua` in -place.** A lane about ambient state that is only ever verified in a clean -environment has not been verified at all. +after the command that could mask it. +**Isolate every storage root, not just the config one.** Rev 2's gate +set only `XDG_CONFIG_HOME`, which stops the `init.lua` reads and leaves +the data root live — so an "isolated" run could still write to +`~/.local/share/pmacs`. **Every local gate run in this repository today +had that hole.** The correct invocation sets, to a fresh directory: + +``` +XDG_CONFIG_HOME XDG_DATA_HOME XDG_STATE_HOME XDG_CACHE_HOME +``` + +`HOME` is deliberately left alone (§1.6b). + +**Run the suite twice: once isolated, once with a hostile environment** +— an `init.lua` that would break a known assertion, plus a pre-seeded +data root — and assert the hostile root is **byte-identical +afterwards**. A lane about ambient state verified only in a clean +environment has not been verified. ## 7. Branch plan From e1df0cff4d4e67ac2dc5ee4af07c0c81c075a244 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:57:02 -0400 Subject: [PATCH 4/4] =?UTF-8?q?docs(tests):=20framing=20revision=204=20?= =?UTF-8?q?=E2=80=94=20close=20review=20round=203?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One blocking and two major findings, all accepted. - The isolated gate still inherited `PMACS_STATE_HOME`, which wins over `XDG_STATE_HOME` in `user_state_dir` (`src/state.rs:47-68`), so controlling the four XDG storage variables did not isolate state on a machine that exports it. The contract now names the exact five variables rather than counting roots. - Q#TI5 still called self-spawn the durable regression guard after revision 3 had accepted that it proves behaviour, not continued adoption. It now names both guards and their different jobs: self-spawn is the behavioural proof, the checked source inventory is the adoption proof. - The active-work lane still said revision 2 and still prescribed an isolated `XDG_CONFIG_HOME` alone; both are synchronized, and the stale config-only gating note elsewhere in the ledger is corrected to say it isolates the observed symptom rather than the gate. Framing only. No code. --- docs/active-work.md | 24 ++++--- docs/test-ambient-config-isolation-framing.md | 65 ++++++++++++++----- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index c57c82f..d8326fe 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -743,13 +743,13 @@ review correction. projection.** Whichever is framed second re-scouts the other's landed state. -## Test ambient-root isolation — FRAMING OPEN, revision 2 +## Test ambient-root isolation — FRAMING OPEN, revision 4 - **Branch `test-ambient-config-isolation`**, worktree `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b`. **Framing only; no code, no PR yet.** - `docs/test-ambient-config-isolation-framing.md` revision 3, two review - rounds closed (seven blocking, four major, all accepted). + `docs/test-ambient-config-isolation-framing.md` revision 4, three review + rounds closed (eight blocking, six major, all accepted). - **What it is.** Integration tests use the developer's real ambient roots. `#[cfg(not(test))]` guards config loading against the crate's own unit tests only, so the **65** files in `tests/` that construct an @@ -760,8 +760,10 @@ review correction. - **Why it matters now.** `cargo test` is red on any machine with a real `~/.config/pmacs/init.lua` (11 of 67 in `compile_mode_acceptance`) and green in CI, so the failure is attributed to whatever branch is - checked out. Every local gate run in this repo currently needs an - isolated `XDG_CONFIG_HOME` as a workaround. + checked out. Every local gate run in this repo currently needs all + five bootstrap-storage variables controlled as a workaround: + `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, + `XDG_CACHE_HOME`, and `PMACS_STATE_HOME`. - **Round 1's four blocking findings, all confirmed in code:** the read-only assumption was already false; the population count was 18 when 65 of 96 files construct an editor, from a grep that did not @@ -1746,11 +1748,13 @@ git worktree add --track \ acting frontend made a total function partial, and six runtime modules silently dropped operations (`kill_ring_acceptance` 30/30 → 25/5). Fixed in `9110f9f` before merge. - - Gating fact found on the way: **the workspace sweep must run with an - isolated `XDG_CONFIG_HOME`**, because the real user `init.lua` - installs a local package and the losing race leaks a status message - into painted-frame comparisons. There is also a latent pre-existing - `main` bug in the buffer CRDT undo path, unrelated to this arc. + - Gating fact found on the way: an isolated `XDG_CONFIG_HOME` prevents + the real user `init.lua` from installing a local package and leaking + a status message into painted-frame comparisons. **That isolates the + observed config symptom only, not the gate:** the ambient-root lane + above establishes that data/state/cache must be controlled too. + There is also a latent pre-existing `main` bug in the buffer CRDT + undo path, unrelated to this arc. - `compile_mode_acceptance` is load-sensitive under default parallelism (~1 run in 3, a different test each time); verified pre-existing by swapping in `main`'s `compile.lua`. It is 67/67 at diff --git a/docs/test-ambient-config-isolation-framing.md b/docs/test-ambient-config-isolation-framing.md index 107316f..d9971ba 100644 --- a/docs/test-ambient-config-isolation-framing.md +++ b/docs/test-ambient-config-isolation-framing.md @@ -1,6 +1,6 @@ # Framing — integration tests use the developer's real ambient roots -**Revision 3.** Status: awaiting review round 3. Proposed lane: +**Revision 4.** Status: awaiting review round 4. Proposed lane: `test-ambient-config-isolation`, worktree `../pmacs-test-isolation`, based on `githubsucks/main` @ `4cd4a7b` (a reading; re-measure at branch time). @@ -16,6 +16,21 @@ directory**. The lane is therefore about *ambient roots*, not about ## Revision history +**Revision 3 → 4**, after review round 3 (one blocking, two major). All +three accepted. + +- **The isolated gate still inherited `PMACS_STATE_HOME`** (§1.6a, + §6). That override wins over `XDG_STATE_HOME`, so setting the four XDG + storage variables did not isolate state on a machine that exports it. + The contract now names the exact five variables rather than counting + roots. +- **Q#TI5 still called self-spawn the durable regression guard** after + revision 3 had accepted that it proves behaviour, not continued + adoption. Q#TI5 now names both guards and their different jobs. +- **The active-work lane still said revision 2 and still prescribed + isolated `XDG_CONFIG_HOME` alone.** Both are synchronized with this + revision. + **Revision 2 → 3**, after review round 2 (three blocking, two major). All five accepted; all five verified before acceptance, two of them by running the thing rather than reading it. @@ -223,12 +238,19 @@ So revision 1's Bet 2 was not a question to investigate — it was already answered, in the direction that matters. This upgrades the lane from "local gates lie" to **"tests write into real user data"**. -### 1.6a Six ambient roots, and the harness covers two +### 1.6a Ambient variables, and the harness's incomplete storage coverage `src/` reads: `HOME` (9 sites), `XDG_DATA_HOME`, `XDG_CONFIG_HOME`, `XDG_STATE_HOME`, `XDG_CACHE_HOME`, `XDG_RUNTIME_DIR`, and `PMACS_STATE_HOME`. +The four bootstrap storage roots resolve through **five variables**: +`XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, +`XDG_CACHE_HOME`, and `PMACS_STATE_HOME`. The fifth is not redundant: +`PMACS_STATE_HOME` wins over `XDG_STATE_HOME` +(`src/state.rs:47-68`), so redirecting the latter while inheriting the +former leaves the real state root live. + The shared harness `spawn_daemon_process_with_env` (`tests/common/daemon.rs:154`) sets **`HOME` and `XDG_CONFIG_HOME` only**. A spawned daemon therefore inherits the developer's real @@ -354,12 +376,15 @@ skip **ambient reads** while still returning `is_init_complete() == true`. ratchet exists to prove.* - **Q#TI4** — Must isolated construction remain init-complete? **Yes** (§1.8), and it is a criterion rather than an assumption. -- **Q#TI5** — What is the durable regression guard? *Proposed: a - **test-binary self-spawn under a controlled environment**, not a CI - leg. Revision 1 proposed a hostile-config CI leg in Q#TI3 and parked - that same mechanism in §5 — a contradiction. A CI leg also proves only - the branch that adds it. A self-spawning test carries its own hostile - environment and fails wherever it runs.* +- **Q#TI5** — What are the durable regression guards? *Proposed: two + guards with different jobs. A **test-binary self-spawn under a + controlled environment** is the behavioural proof: the isolated seam + ignores hostile ambient roots and leaves them unmodified. A **checked + source inventory** is the adoption proof: a raw ambient constructor + added to another test binary fails the suite. Revision 1 proposed a + hostile-config CI leg in Q#TI3 and parked that same mechanism in §5 — + a contradiction; self-spawn replaces that workflow leg, while the + inventory prevents migration drift.* ## 3. Bets @@ -371,9 +396,10 @@ skip **ambient reads** while still returning `is_init_complete() == true`. - *Falsified if* sites are reached through helpers that obscure which kind they are. Then the helper is the unit and that is stated. -- **Bet 2 — every ambient root can be redirected without `unsafe`.** +- **Bet 2 — every bootstrap storage root can be redirected without + `unsafe`.** A `BootstrapRoots` parameter covers config, data, state and cache; the - spawned harness sets all six variables of §1.6a. + spawned harness controls all five storage variables of §1.6a. - *Falsified if* any root is resolved somewhere that cannot accept the parameter — e.g. behind a `OnceLock` initialised before construction. **That is a real risk and is checked first**, because it decides @@ -416,8 +442,10 @@ skip **ambient reads** while still returning `is_init_complete() == true`. its isolation comes from the launched environment. The ratchet's production-entry-point discipline is unweakened, and this is asserted rather than asserted-to-be-obvious. -5. The shared harness `spawn_daemon_process_with_env` sets **all six** - roots of §1.6a, not two. +5. The shared harness `spawn_daemon_process_with_env` controls all + **five storage variables** of §1.6a: + `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, + `XDG_CACHE_HOME`, and `PMACS_STATE_HOME`. 6. `compile_mode_acceptance` passes with a real `~/.config/pmacs/init.lua` defining `find-file` present — the exact condition of §1.1. @@ -429,9 +457,11 @@ skip **ambient reads** while still returning `is_init_complete() == true`. unconditional package materialization above it. 10. This lane is recorded in `docs/active-work.md` with its branch and worktree, per the volatile-work protocol — revision 1 omitted it. -11. README or handoff records that a local full-suite run needs **all - four storage roots** isolated until this lands — naming them, since - isolating only `XDG_CONFIG_HOME` leaves the write path open. +11. README or handoff records that a local full-suite run needs all + **five storage variables** controlled until this lands: + `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, + `XDG_CACHE_HOME`, and `PMACS_STATE_HOME`. Naming only the four XDG + variables leaves a higher-precedence state override live. 12. **A durable adoption ratchet**, not a one-time census: a checked source inventory that fails when a new ambient `EditorState::new()`/`open(` appears in `tests/` outside a narrow, @@ -471,9 +501,12 @@ had that hole.** The correct invocation sets, to a fresh directory: ``` XDG_CONFIG_HOME XDG_DATA_HOME XDG_STATE_HOME XDG_CACHE_HOME +PMACS_STATE_HOME ``` -`HOME` is deliberately left alone (§1.6b). +`PMACS_STATE_HOME` must be redirected or explicitly removed; it takes +precedence over `XDG_STATE_HOME`. `HOME` is deliberately left alone +(§1.6b). **Run the suite twice: once isolated, once with a hostile environment** — an `init.lua` that would break a known assertion, plus a pre-seeded