test: unify the test harness, discover suites, fix a real hermeticity leak #134

2026-09-08T00:59:07Z

rootiest commented

2026-09-08 00:59:07 +00:00

What

tests/ had three files in three idioms, hand-named in run-tests.fish. Unified into one tests/lib.fish assertion core (check, section, report) and a driver that discovers tests/test-*.fish by glob instead of naming each suite. 332 pre-existing assertions preserved with identical per-phase counts (317 vault, now 15→17 session — the count was mis-attributed before, see below).

The finding that mattered most

tests/test-agents-vault.fish was documented as hermetic. Run directly, it wasn't: it loaded the live config and real universal variables (registry entry count measured at 65 with config loaded, 0 with --no-config). This job's whole task was writing guard tests that manipulate exactly those variables (set -e __fish_config_op_logging, etc.) — so without this fix, the tests written to prove the guard system safe would have been capable of erasing a real universal variable from the maintainer's running shell. Fixed first, before any guard test was written (dd672de).

The obvious hardening — env -i HOME=$sandbox — was rejected: it would have made the vault suite's two existing hermeticity assertions vacuous (both snapshots read absent before and after, still green, asserting nothing). HOME stays real; only XDG_* is sandboxed. That reasoning is a code comment at both call sites, not just in the design doc.

Guard-system coverage

66 new cases covering __fish_variable_check's four exit codes, the subcategory→category→master cascade, C5 opt-in (including that a truthy master switch cannot enable it — the specific regression AGENTS.md §5 warns has recurred before), registry lookup, and always/on/always/off/AND-across-tags via a synthetic registry (the production registry has 65 entries, every one single-tag, with no always/* anywhere — those code paths are otherwise unreachable from real data).

All 66 pass. No defects found in the guard system itself.

332 → 398 total assertions.

Also fixed, found in passing

The runner called bare cp, which this config aliases to cp -i. On an existing destination with stdin at EOF it silently skips the copy and exits non-zero — but the runner never checked that status, so a partially-populated sandbox could report as a clean pass. Latent today only because the sandbox destination is always a fresh mktemp -d. All runner utilities now go through command.

Corrections to two claims that didn't survive verification

The runner does not leave sandboxes in the trash. An earlier pass suspected rm -rf was being intercepted by this config's trash wrapper; verified false — the wrapper bails to command rm for any non-recursive flag, and -f is such a flag.
There is no mod-256 false-green in exit $TESTS_FAILED. Fish clamps exit/return to 255 rather than wrapping (fish -c 'exit 256' → 255; POSIX sh gives 0). report's test $TESTS_FAILED -eq 0 ending is kept as an AGENTS.md §5 style-conformance change, not a bug fix.

Please merge before #refactor/config-settings-renderer

That PR's last commit hand-adds a phase to the old run-tests.fish. If this PR lands first, that commit becomes droppable entirely — this driver's glob-based discovery picks up tests/config-settings-render.fish with no changes needed.

Testing

fish tests/run-tests.fish — 226/226 lint (unchanged), 398/398 total assertions, exit 0. Every commit gated green; a dedicated task proves the harness goes red on a broken assertion, a broken mode marker, and reports exit 1 correctly before landing any new coverage.

## What `tests/` had three files in three idioms, hand-named in `run-tests.fish`. Unified into one `tests/lib.fish` assertion core (`check`, `section`, `report`) and a driver that discovers `tests/test-*.fish` by glob instead of naming each suite. 332 pre-existing assertions preserved with identical per-phase counts (317 vault, now 15→17 session — the count was mis-attributed before, see below). ## The finding that mattered most `tests/test-agents-vault.fish` was documented as hermetic. Run directly, it wasn't: it loaded the **live config and real universal variables** (registry entry count measured at 65 with config loaded, 0 with `--no-config`). This job's whole task was writing guard tests that manipulate exactly those variables (`set -e __fish_config_op_logging`, etc.) — so without this fix, the tests written to prove the guard system safe would have been capable of erasing a real universal variable from the maintainer's running shell. Fixed first, before any guard test was written (`dd672de`). The obvious hardening — `env -i HOME=$sandbox` — was rejected: it would have made the vault suite's two existing hermeticity assertions **vacuous** (both snapshots read `absent` before and after, still green, asserting nothing). `HOME` stays real; only `XDG_*` is sandboxed. That reasoning is a code comment at both call sites, not just in the design doc. ## Guard-system coverage 66 new cases covering `__fish_variable_check`'s four exit codes, the subcategory→category→master cascade, C5 opt-in (including that a truthy master switch **cannot** enable it — the specific regression `AGENTS.md` §5 warns has recurred before), registry lookup, and `always/on`/`always/off`/AND-across-tags via a synthetic registry (the production registry has 65 entries, every one single-tag, with no `always/*` anywhere — those code paths are otherwise unreachable from real data). **All 66 pass. No defects found in the guard system itself.** **332 → 398 total assertions.** ## Also fixed, found in passing The runner called bare `cp`, which this config aliases to `cp -i`. On an existing destination with stdin at EOF it silently skips the copy and exits non-zero — but the runner never checked that status, so a partially-populated sandbox could report as a clean pass. Latent today only because the sandbox destination is always a fresh `mktemp -d`. All runner utilities now go through `command`. ## Corrections to two claims that didn't survive verification - **The runner does not leave sandboxes in the trash.** An earlier pass suspected `rm -rf` was being intercepted by this config's trash wrapper; verified false — the wrapper bails to `command rm` for any non-recursive flag, and `-f` is such a flag. - **There is no mod-256 false-green in `exit $TESTS_FAILED`.** Fish clamps `exit`/`return` to 255 rather than wrapping (`fish -c 'exit 256'` → 255; POSIX `sh` gives 0). `report`'s `test $TESTS_FAILED -eq 0` ending is kept as an `AGENTS.md` §5 style-conformance change, not a bug fix. ## Please merge before #refactor/config-settings-renderer That PR's last commit hand-adds a phase to the *old* `run-tests.fish`. If this PR lands first, that commit becomes droppable entirely — this driver's glob-based discovery picks up `tests/config-settings-render.fish` with no changes needed. ## Testing `fish tests/run-tests.fish` — 226/226 lint (unchanged), 398/398 total assertions, exit 0. Every commit gated green; a dedicated task proves the harness goes red on a broken assertion, a broken mode marker, and reports exit 1 correctly before landing any new coverage.

rootiest added 10 commits 2026-09-08 00:59:07 +00:00

fix(tests): isolate the vault suite from the live config and universal variables dd672ded3b

The suite ran under a plain `fish`, which loads the user's real
~/.config/fish and their universal variables -- this repo doubles as that
config. A test manipulating a guard variable could erase a real universal
variable out of the running shell.

Override XDG_CONFIG_HOME/XDG_DATA_HOME and pass --no-config. HOME stays
real on purpose: overriding it makes the suite's two hermeticity
assertions vacuous. Reasoning recorded at the call site.

Vault suite still 317/317 with byte-identical stderr.

fix(tests): route the runner's utilities through command 8fc27fa9c2

run-tests.fish executes under the config it tests, which shadows cp, rm
and cat. The real hazard is cp: the config aliases it to 'cp -i', which on
a non-empty destination reads EOF in a non-interactive runner, silently
skips the copy and exits 0 -- a sandbox missing config files, reported as
success.

rm -rf and cat were measured and behave correctly as-is (the rm wrapper
bails to command rm on any non-recursive flag, so -rf really deletes and
does not trash). Prefixed anyway: a test runner must not depend on the
configuration under test.

refactor(tests): extract the shared assertion core into tests/lib.fish d3028d7703

check/section/report and the counters move out of the vault suite
unchanged. report also writes its counts to $FISH_CONFIG_TEST_COUNTS so a
driver can aggregate without parsing stdout, and ends on an explicit
boolean per AGENTS.md item 5.

All 317 vault assertions and every fixture helper are untouched.

refactor(tests): discover suites instead of naming them 0a9cfebcad

run-tests.fish globs tests/test-*.fish and runs each in the mode the suite
declares in its own header. Isolated is the default and no typo can
promote a suite to in-session: detection is case-insensitive so a
near-miss is caught, the value comparison is exact.

In-session suites share one sandboxed session; isolated suites each get a
--no-config child with temp XDG dirs. Counts come back through a file so
output keeps streaming.

functional.fish becomes test-session.fish, its 15 predicates rewritten as
check calls. 332 assertions, unchanged per-phase.

test(guards): cover __fish_variable_check's four exit codes a8d564803b

21 cases: a precondition asserting the fork's registry is loaded, then
truthy/falsy/unset/empty/unrecognized/no-argument/list.

Runs isolated, which is what lets these cases manipulate real guard
variable names without being able to reach the user's universal variables.

test(guards): cover the cascade's subcategory -> category -> master order 2b1e99ab47

13 cases on invented variable names, so nothing ambient can perturb them:
default-on, explicit truthy/falsy, unrecognized-defers, subcategory
overriding the category in both directions, and the four cases that
together pin the master as an off switch only.

test(guards): pin C5 logging as opt-in, including that the master cannot enable it 639d139799

8 cases on the real __fish_config_op_logging name. AGENTS.md records this
as a deliberate special case agents keep trying to 'fix' out; three of
these cases exist specifically to turn that into a test failure.

Includes the three subcategory cases proving the opt-in is inherited
through chain[-1] rather than special-cased per subcategory.

test(guards): cover registry lookup, including identity:site pairing 2efbcfc53f

5 cases. The wrong-site case pins that the key is the identity:site pair
rather than the identity alone.

test(guards): cover op_enabled against the real registry 7a6a7339dc

5 cases: default-on, .fish suffix stripping, category off, subcategory
overriding it, and the documented fail-open on a missing entry.

test(guards): cover always/on, always/off and AND via a synthetic registry 1f847bfce0

14 cases. The production registry has 65 entries all carrying exactly one
tag and no always/* anywhere, so these three documented semantics are
unreachable from real data -- the synthetic keys/values table is the only
way to reach them. Reasoning recorded at the fixture.

Also pins the degenerate no-slash tag and reaches C5 through
__fish_config_op_enabled, which is the path production code takes.

rootiest merged commit 0a2290640f into main

2026-09-08 05:26:30 +00:00

rootiest deleted branch test/test-harness-overhaul

2026-09-08 05:26:30 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rootiest/fish-config#134