From e1df0cff4d4e67ac2dc5ee4af07c0c81c075a244 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:57:02 -0400 Subject: [PATCH] =?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