mcpx testing: guard tests that close a whole bug class #1971

Relocated from dezren39/darwin-nix#129 on 2026-10-28 (placement audit): mcpx's single source lives in developing-today/code (pkgs/mcpx); re-home tracked at #309. Original body below; issue references inside it are historical (dezren39/nix numbering).

Parent: #99

Summary

When a bug is one instance of a class -- a declaration and its use live apart, and the same shape has shipped before -- the fix adds a guard test that fails for every member of the class, present and future, not just the instance found. mcpx already has more of these than the strategy doc lists; this issue inventories them, finds where the existing ones leak, picks how guards are written, and lists the guards worth adding next. It also sets the rule for when a bug "is a class". It owns #181 collision 15: a setting must be able to change behaviour, not merely be read. That guard is due before or with #172.

Original notes

• testing
  • agents should make at least 1 simple, new test for every bug observed from any source, when cheap to do so-- aggresively test many permutations.

(Guards are the per-class extension of "at least 1 simple, new test for every bug": the per-instance policy is #128.)

Current state: guards that exist at 05c78b2

There are about twenty-five, not three. The ones that matter here, by mechanism:

The "declared but never read" audit landed as #171. Its inverse ("read but never declared", 44 MCPX_* variables) is #172 and is in flight. TestEverySettingSaysWhoReadsIt checks only that a setting declares a scope; TestEverySettingIsReadSomewhere checks that something reads it. Nothing checks that reading it can change anything (leak 5 below).

Where the existing guards leak

1. TestEveryExecFlagChangesSomething does not enumerate the flags. It has four rows (--export, --launcher, --prefix, --typecheck) and a comment "Add a row when you add a flag". The exec/run flag set has 23 flags (internal/cli/commands.go:435-481), including three that Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75 fixed (--no-launcher, --allow-repeat, --no-capture-console) with no row. A new flag with no row passes. The class is not closed; it relies on memory, the thing the guard exists to replace. Its probe half is the model to copy: each row asserts the outcome with the flag and its absence without, and that half caught Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75's own first --typecheck assertion, which matched output present either way.
2. Regex source scanning misses spellings. PR chore(deps): bump rustix from 0.37.19 to 0.37.25 in /sources/pavex/examples/realworld #56: TestNoPackageRedeclaresADefault matched "5 * time.Minute" while gofmt writes 5*time.Minute in call arguments, so nine live constants were invisible to it. A guard that silently passes is worse than none; it was found by accident.
3. Allowlists are one-way doors. outOfScope in guard_test.go still exempts seven whole packages and one file (guard_test.go:86-90) "owned by work happening in parallel"; nothing makes it shrink.
4. Nothing fails when a test reaches the network (Update Rust crate axum to 0.6.20 #65 fixed the one instance via MCPX_REGISTRY_URL, e2e_test.go:57). chore(deps): bump build/buck/preludes/buck2-prelude from d956689 to e7457f6 #52 is fixed (Phase 5: tag event bodies must travel with the event #182): cleanup is stop --all with the error logged, not discarded (e2e_test.go:71-85), and TestEnvCleanupStopsTheDaemonItStarted checks one env's daemon dies. Still unguarded: a daemon started outside newEnv, and testsupport's mcpx-bin-*/mcpx-fake-* dirs, which are never removed (internal/testsupport/build.go:27,57).
5. consumed_test.go proves a setting is read, not that reading it matters. registry.pageSize is read (internal/cli/registry.go:265, internal/daemon/ops.go:285) but consulted only inside if limit <= 0 (internal/registry/registry.go:154-155). Every ordinary caller passes a positive limit: --limit defaults to 20 (internal/cli/registry.go:26), and the daemon falls back to registry.limit (internal/daemon/ops.go:275-276). So it passed Phase 2: delegated Manage cannot add members #171's sweep while inert (Phase 3: /id-sync/1 is used by both range and LogSync handshakes #179). The scan sees a read; it cannot see that the read is unreachable. unread_test.go has the right shape, a with/without probe, but covers nine hand-picked settings out of 149. This is Phase 3: sync, blob gate, and relays #181 collision 15, owned here (next section).
6. A guard's name can overclaim. TestEverySettingIsReachableFromAllThreeSurfaces (internal/settings/registry_test.go:27-39) checks that a flag name exists, that the variable is MCPX_-prefixed and that the setting is documented. That is name shape, not reachability. The e2e TestEverySettingIsReachableOverV1 is the one that probes.
7. An allowlist entry's reason is prose nothing checks, and one is false. consumed_test.go:67-70 exempts paths.config as "read by config.SearchPath from MCPX_PATHS_CONFIG directly" (repeated in docs/declared-but-unread.md:93-96). Nothing reads MCPX_PATHS_CONFIG; the search path reads MCPX_CONFIG (internal/config/config.go:549). The exemption hid a setting that is dead on all three channels (Bump build/buck/preludes/buck2-prelude from 3a02ea3 to 370cd4d #5). An allowlist reason that names a mechanism should be a mechanism a test can check (the variable is read somewhere, the function exists), or say "declared, not read" plainly.

Owns #181 collision 15: a setting must be able to change behaviour

Owns #181 collision 15: the stronger settings guard. Every registry setting must be able to change an observable outcome through an ordinary caller, not merely be read. #179 (registry.pageSize) is the instance; this issue owns the class.

• Model: Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75's exec flag table (e2e_test.go:2877) asserts, per row, the outcome with the flag and its absence without. Phase 2: delegated Manage cannot add members #171's unread_test.go already does the same for nine settings by hand (set the value the way a user would, as an env var or config key and not a Go field, then assert a change).
• Shape (Approach C below, applied to settings.Registry()): a unit-speed completeness test fails when a setting has neither an effect probe nor an exemption with a reason. Exemptions ratchet down (e.g. plugin-scoped settings the Go side cannot observe, and paths.config, Bump build/buck/preludes/buck2-prelude from 3a02ea3 to 370cd4d #5). The e2e probes set a non-default value through an ordinary caller and assert a with/without difference. A negative test plants an inert setting, read only behind an unreachable branch as registry.pageSize is, and expects the guard to fail.
• Timing: apply it before or with Phase 2: membership, delegation, groups, and server config #172 (in flight), which will move up to 44 more MCPX_* variables into the registry (its recommended option declares the inbound ones and allowlists the outbound). Each should arrive with a probe row or a reasoned exemption, not be audited afterwards. Of the whole tree, this is the one part of Bump unsafe-libyaml from 0.2.8 to 0.2.10 in /sources/pavex/examples/realworld #99 that is time-sensitive.
• Scale: 149 settings at 05c78b2 (unique Path: entries in internal/settings/*.go), with more to come from Phase 2: membership, delegation, groups, and server config #172. A probe per setting in e2e is too slow for every PR. Keep the completeness half at unit speed (T3 in Update Rust crate tracing to v0.1.40 [SECURITY] #127). Group probes by surface, so that one daemon checks many settings. Update Rust crate tracing-subscriber to v0.3.20 [SECURITY] - autoclosed #124's settings.precedence generator can supply the non-default values.

When a bug "is a class"

Add a guard (in the fix PR, or file testing + implement and link it) when any of these holds:

• the same shape has shipped before (Update Rust crate petgraph to 0.6.4 - autoclosed #74 was the third "accepted and ignored");
• the bug is drift between a declaration and its use (registry vs reader, op table vs route, flag vs options struct, docs vs code);
• the fix edits a table, registry or allowlist;
• the bug depended on the environment the test ran in (network, OS, leftover processes).

Approaches: how guards are written

A -- source scanning with regexes (least recommended)

Walk .go files and match text, as TestNoPackageRedeclaresADefault, daemonRoutes and #171's consumed_test.go do. Quick to write and needs no registry. Risks: formatting changes the text (the gofmt miss above), comments and strings produce false hits, renames produce false passes, a match proves a call exists but not that it is reachable (leak 5), and failures read as "line 212 matched a pattern" rather than "setting X is never read". Keep only where nothing better exists, and if so parse with go/parser/go/ast (or golang.org/x/tools/go/packages for type info) rather than regexes.

B -- enumerate the registry (reflection over data)

Iterate settings.Registry(), api.Ops(), flag.FlagSet.VisitAll, the command table, and assert properties per entry. Unit-speed, exact, cannot be fooled by spelling -- most of the table above is this. Limitation: it proves declarations are consistent, not that they work; a registry entry nothing reads passes every B guard, and so does one that is read but cannot matter (registry.pageSize).

C -- enumerate, require a probe per entry, prove the guard can fail (recommended)

1. Enumerate the registry (B) as the source of truth.
2. Completeness: a unit test fails when an entry has neither a probe row nor an exemption with a reason (the allowed map style in guard_test.go). Fixes leak 1: a new flag without a row fails in milliseconds, locally, at T1/T3 of Update Rust crate tracing to v0.1.40 [SECURITY] #127.
3. Probe: each row proves an observable effect in both directions (the Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75 with/without pattern); probes that need the binary live in e2e and run in CI.
4. Ratchet: exemption lists may only shrink -- a test pins their length, or CI compares it to main.
5. Negative test: every guard's checker is a function with its own test that feeds it a planted violation and expects a failure (fixes leak 2's lesson: a guard that cannot fail is not tested).
6. Use AST scanning (not regex) only for classes with no registry. (Phase 2: delegated Manage cannot add members #171's "declared but never read" guard used a regex scan, which is enough to find a read but, as leak 5 shows, not to prove it matters; step 3's probe is what closes that.)

Recommended because it closes the gap the current exec guard has (enumeration), keeps the cheap half (completeness) in unit time so agents actually run it, and makes the guards themselves falsifiable.

Candidate new guards

Sub-choice: enforcing "no network"

• Env only (weakest): set HTTP_PROXY/HTTPS_PROXY to a dead port (http://127.0.0.1:9) in newEnv and a unit TestMain. Catches Go HTTP clients that honour proxy env (including the spawned mcpx binary); misses raw dials and anything in NO_PROXY.
• In-process dialer guard: replace http.DefaultTransport's DialContext in TestMain to refuse non-loopback. Precise for unit tests; cannot reach the mcpx subprocess or the JS runtimes.
• No network in the CI test step (recommended, plus the env layer locally): run tests in a network namespace (unshare -rn / systemd-run --property=PrivateNetwork=yes / a container with --network none) after go mod download. Catches everything, including deno/bun/node. Linux-only, so it lives in CI (Bump golang.org/x/crypto from 0.16.0 to 0.17.0 in /sources/launcher #95); macOS keeps the env layer.

Tasks

• Implement: settings effect guard (Phase 3: sync, blob gate, and relays #181 collision 15) -- completeness over settings.Registry(), probes grouped by surface, a planted-inert negative test. Before or with Phase 2: membership, delegation, groups, and server config #172.
• Implement: enumerate the exec/run flag set in TestEveryExecFlagChangesSomething (completeness + exemptions), rows for the Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75 flags.
• Implement: negative tests for existing checker-style guards (defaults, parity, consumed).
• Implement: no-network (env layer now; namespace via Bump golang.org/x/crypto from 0.16.0 to 0.17.0 in /sources/launcher #95).
• Implement: process/temp-dir sweep TestMain (chore(deps): bump build/buck/preludes/buck2-prelude from d956689 to e7457f6 #52 itself is fixed by Phase 5: tag event bodies must travel with the event #182; this is the suite-wide backstop and the temp-dir cleanup).
• Decide which other candidates become issues.

Open questions

• Should the completeness half of an e2e guard be a separate unit test (runs at T1), or stay inside the e2e test (runs only in CI)? Proposed: separate.
• Does the "declared but never read" audit produce a registry-level guard (B/C) or an AST scan (A with go/ast)? Answered by Phase 2: delegated Manage cannot add members #171: a regex scan (A's mechanism, consumed_test.go:54,60) over registry enumeration. The effect guard above is the C half it lacks.
• Which settings cannot be probed from Go at all (plugin-scoped ones read only by TypeScript, paths.config)? Do they get a TypeScript-side probe in the plugin's bun test, which CI does not run yet (Bump dezren39/nix flake input to 3499ed3 — opencode now defaults to v2 #141)?
• Where do exemption lists live: in the test file (current style) or in a data file several guards share?

Ease · Impact · Related

Ease M (policy S; each guard S-M) · Impact high
Depends on / related: #74, #75, #76, #65, #52 (closed, fixed in #182), #56 (the defaults-guard miss), #69/#70/#73 (the shared-path class), #97 (integration inventory), #95 (no-network CI step), #93 (generators for probe inputs); #171 (merged: the read guard), #172 (in flight: its inverse; lands with or after the effect guard), #179 (in flight: the registry.pageSize instance), #181 (collision 15, owned here); siblings #127 (T3 guards list), #128, #130, #131.

Security/auth deferred; see tracker.

Acceptance criteria

• The existing guard tests are inventoried on this issue, with where each leaks (class members it cannot catch) named.
• The rule for when a bug "is a class" is recorded, and the house style for writing guards is pinned.
• The Phase 3: sync, blob gate, and relays #181 collision-15 guard — a setting must be able to change behaviour, not merely be read — is landed, or scheduled before/with Phase 2: membership, delegation, groups, and server config #172 as the body requires.
• The guards worth adding next are listed, with follow-up issues filed for any not landed here.
• The work described in the Summary/Scope above is delivered via a branch + PR (no direct push to main); verification runs in GitHub Actions CI, not on the sandbox.
• Evidence (CI links or guard output as applicable) is recorded on this issue before it is closed.