docs(tests): framing revision 3 — close review round 2
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 <path> <remote-only-branch>` fails with `fatal: invalid reference` after a bare fetch. Replaced with the explicit tracking-branch form. Framing only. No code.
This commit is contained in:
parent
f2071ea022
commit
e9613c7d94
|
|
@ -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 <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
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <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.
|
||||
|
||||
|
|
@ -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 <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`),
|
||||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue