Merge pull request #201 from levineuwirth/test-ambient-config-isolation
docs(tests): frame the ambient-root isolation defect
This commit is contained in:
commit
22df6ab11a
|
|
@ -118,69 +118,6 @@ declares canonical will pass on a tree the rest of this file does not
|
|||
describe.
|
||||
If it does not, stop and repair the remote/fetch configuration.
|
||||
|
||||
## Process-signal diagnostic completeness — IMPLEMENTED, PR OPEN
|
||||
|
||||
- **Portable branch:** `githubsucks/process-signal-diagnostic-completeness`;
|
||||
worktree `../pmacs-signal-identity`. Governing document:
|
||||
`docs/process-signal-diagnostic-completeness-framing.md`, **revision 6**;
|
||||
the contract was approved at revision 4 and has been revised twice
|
||||
since to match what shipped.
|
||||
- **State:** **PR #200 open**, four review rounds closed, held for
|
||||
review. Canonical `githubsucks/main` @ `b8e18f6` (the ledger
|
||||
absorption #199) is integrated. Four commits: Bet 1 alone, Bets 2–4,
|
||||
Bet 1's fallback after CI falsified it, then round 4's corrections.
|
||||
- **Boundary, unchanged:** evidence collection only. No signal
|
||||
retargeting, tolerance, disposition change, or reap-ledger repair.
|
||||
**Group identity remains unprovable** (framing §1.5): the measured
|
||||
group is read in the same read-then-act window, and no portable
|
||||
mechanism closes that for a *group* — `pidfd` covers a process, and
|
||||
macOS has neither. The ledger's silent cancellation is parked as its
|
||||
own lane.
|
||||
- **What shipped.** The PTY foreground-group lookup now reports four
|
||||
distinct outcomes instead of one string, through a safe
|
||||
`filedescriptor::OwnedHandle::dup` bridge that keeps the errno
|
||||
`portable-pty` discards — with no `unsafe` in pmacs. The report names
|
||||
the signal. A `measured_group` field from `getpgid` is the only field
|
||||
in the report able to disagree with its input. The reap-ledger comment
|
||||
claiming "EPERM cannot happen for our own children" is corrected
|
||||
**without** claiming the child itself received EPERM.
|
||||
- **Verification at the merged tree** (not inherited from the pre-merge
|
||||
head): 11 gates, **4471 tests, zero failures** — fmt, diff-check,
|
||||
clippy, `--lib`, `--lib --features crdt`, compile-mode, copy-mode in
|
||||
both feature configurations, bottom-panel Stage 1, M4 with the
|
||||
basedpyright skip, and required GPU. The job-control divergence
|
||||
fixture was run 20/20. All local runs used an isolated
|
||||
`XDG_CONFIG_HOME`.
|
||||
- **Four bites, each by an actual revert, all observed to fail:**
|
||||
collapsing the PTY fallback back into a bare `leader-pid`; dropping
|
||||
`signal=` from the report; making the measured group restate the pid
|
||||
it was handed; and — as a positive control on the fixture itself —
|
||||
replacing the `bash -m` job-control child with a plain `sleep`.
|
||||
Separately, the **pre-Stage-B test was restored verbatim under the
|
||||
substitution mutation and PASSED**, which is the finding that
|
||||
justified rewriting it rather than adding to it.
|
||||
- **Bet 1 was falsified by CI and the framing's fallback shipped.**
|
||||
`bash -m` diverges on Linux and **never on macOS**, where both legs
|
||||
observed the terminal stay with the leader for a full 10s wait. The
|
||||
divergent case is now pinned by **injecting** the foreground group
|
||||
(runs everywhere, weaker); a Linux-only corroboration drives a real
|
||||
shell and is the **only** test exercising `pty_foreground_group`
|
||||
end-to-end. `/bin/bash` is a declared optional test dependency armed
|
||||
by `PMACS_REQUIRE_BASH` on **Linux only** — macOS ships bash but
|
||||
cannot produce the precondition, so arming it there would make a
|
||||
missing binary fatal for a test that can never run.
|
||||
- **Recovery from a clean checkout:**
|
||||
|
||||
```sh
|
||||
git fetch githubsucks --prune
|
||||
git worktree add ../pmacs-signal-identity \
|
||||
-b process-signal-diagnostic-completeness \
|
||||
githubsucks/process-signal-diagnostic-completeness
|
||||
cd ../pmacs-signal-identity
|
||||
git merge-base --is-ancestor b8e18f6 HEAD
|
||||
git status --short --branch
|
||||
```
|
||||
|
||||
## The CRDT half of the test corpus is dark in CI — NEEDS A LANE
|
||||
|
||||
- **No branch, no framing yet.** Found while gating #166, then measured
|
||||
|
|
@ -350,6 +287,52 @@ compatible.
|
|||
- **DAP waits for Stage 2, not Stage 1** — that dependency is now
|
||||
satisfied.
|
||||
|
||||
## 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 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
|
||||
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.
|
||||
- **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 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
|
||||
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 — **the two-argument form does not
|
||||
work**, verified by running it (`git worktree add <path>
|
||||
<remote-only-branch>` 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
|
||||
|
||||
Both shipped stages are on `main`; nothing in this arc is in flight. Stage 3
|
||||
|
|
@ -394,6 +377,27 @@ git worktree add --track \
|
|||
|
||||
## Closed since the last snapshot
|
||||
|
||||
- **Process-signal diagnostic completeness — MERGED as #200**
|
||||
(`main` @ `a2a92bb`), atop Stage A #176. `docs/process-signal-diagnostic-completeness-framing.md`
|
||||
revision 6; framing approved at revision 4 after three rounds, then
|
||||
five review rounds on the implementation. Durable facts are in
|
||||
`docs/agent-handoff.md` §1. **Evidence collection only** — no
|
||||
tolerance rule, no retargeting, no disposition change.
|
||||
- **Bet 1 was falsified by CI and the framing's own fallback shipped.**
|
||||
`bash -m` diverges the terminal's foreground group on Linux and
|
||||
never on macOS. The divergent case is pinned by injection
|
||||
everywhere; a Linux-only corroboration drives a real shell and is
|
||||
the **only** test exercising `pty_foreground_group` end-to-end, so
|
||||
**on macOS that lookup has no end-to-end coverage**.
|
||||
- **Group identity remains unprovable**, and the pre-kill sample does
|
||||
not change that: moving `getpgid` before the `kill` removed a
|
||||
post-hoc reading, it did not make the reading contemporaneous.
|
||||
- **Still parked, each needing its own lane:** the reap ledger's
|
||||
silent cancellation (an EPERM probe drops the entry; a failed
|
||||
`SIGKILL` is marked killed) — **being scoped next**; retargeting to
|
||||
the measured pgid; any EPERM/ESRCH tolerance rule; Q#PS6; and
|
||||
`signal_target`'s read-then-kill of `tcgetpgrp` on the PTY path.
|
||||
|
||||
- **Six lanes removed by the 2026-07-30 absorption pass**, all merged,
|
||||
all with their durable facts in `docs/agent-handoff.md` §1:
|
||||
**#190** resource-op delete-guard implementation; **#196** dired
|
||||
|
|
@ -547,11 +551,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
|
||||
|
|
|
|||
|
|
@ -0,0 +1,521 @@
|
|||
# Framing — integration tests use the developer's real ambient roots
|
||||
|
||||
**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).
|
||||
|
||||
**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.
|
||||
|
||||
**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 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.
|
||||
|
||||
- **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 <path> <remote-only-branch>` 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.
|
||||
|
||||
- **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
|
||||
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 <name>` 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, measured
|
||||
|
||||
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
|
||||
`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.
|
||||
|
||||
**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 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
|
||||
`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.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 <test name>`, 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`),
|
||||
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 CI is genuinely unaffected**, as opposed to merely having no
|
||||
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** — 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 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
|
||||
|
||||
- **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 — every bootstrap storage root can be redirected without
|
||||
`unsafe`.**
|
||||
A `BootstrapRoots` parameter covers config, data, state and cache; the
|
||||
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
|
||||
whether this design is possible at all.
|
||||
|
||||
- **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.** 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 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` 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.
|
||||
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 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,
|
||||
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
|
||||
|
||||
- **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
|
||||
|
||||
Standard suite, each its own step with a real exit status and nothing
|
||||
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
|
||||
PMACS_STATE_HOME
|
||||
```
|
||||
|
||||
`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
|
||||
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
|
||||
|
||||
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.
|
||||
Loading…
Reference in New Issue