mcpx testing: a failing-first test for every bug, and a review of recent fixes #1969

Relocated from dezren39/darwin-nix#128 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

Two parts. Policy: every bug observed from any source gets at least one new test, in the same PR as the fix, that fails without the fix -- when that is cheap -- and the test covers the bug's permutations, not just the one input that tripped it. Review: audit the recently fixed mcpx bugs against that policy, and file the missing tests. The checklist that enforces it (PR template vs CI check) is decided here too.

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.
    • you can absolutely rely on fuzzing anywhere you can fit it and you should whenever you can-- but you must use a single seed per-run to generate all inputs. (this could eventually be discussed with careful consideration.) it should be appended into a test tracking structured file in the repo and it can be committed. ideally main branch commit's seed or pr merge seed should make it back into file eventually. to prevent churn, maybe ci appends the seed into a gha var (race/overwrite risk?), or attached as artifact to each workflow in init job before tests begin-- a collector job can scoop them up sometime later. once a day/week/on-release/every-100-commits. im thinking touch tests/seeds/ or uv has a seed generate job. anyways ci it should end up in that dir, then as an artifact if gha and main branch, then passed into the test matrix. if uv test without arg- warn, generate and write it yourself pass it to all tests, if some other testing method there should not be a static seed, throw for empty or setup should make the seed. avoid changing how seed generates data. im not sure if a convenient uuid with time, v7?, is valid, but as long as its enough entropy whatever works. either put nothing in their or compact json, consider seeds/v1/id/<first 2 char>/<--symlink: seeds/yyyy/mm/dd/ avoiding conflicts is important.

The seed bullet is quoted here because it is the second half of this policy ("aggresively test many permutations" is how, "a single seed per-run" is the constraint). Its design -- seed format, storage under tests/seeds/, GHA artifacts, the collector job, uv -- is owned by #93 and is not decided here. This issue only commits to: permutation generators in regression tests take their randomness from the run seed #93 defines, never from their own clock or a hardcoded constant.

Current state

• No Fuzz*, property tests, math/rand or testing/quick anywhere; permutations today are hand-written table tests (e.g. TestEveryExecFlagChangesSomething, internal/e2e/e2e_test.go:2877, four rows).
• No PR template (.github/ holds only workflows/). Agents open PRs with gh pr create --body ..., which does not apply a template, so a template checkbox alone would not reach them.
• The best existing example of the policy is PR chore(deps): bump build/buck/preludes/buck2-prelude from d956689 to ba020c6 #62: "The new test exercises the script path. Verified it fails without the fix and passes with it." -- and it names why the old test missed it ("only the first direction was tested"). Two later PRs are as good or better. Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75 found that its own guard's first --typecheck assertion matched output present either way; the table's without-flag check caught it. Phase 5: tag event bodies must travel with the event #182 reverted its cleanup fix to watch the new leak test fail before trusting it.

First-pass review of recent fixes (from PR bodies and changed-file lists, merged PRs through #182 at 05c78b2; to be confirmed)

The policy (proposed wording for the contributor page, #131)

1. Sources that count: user reports, CI failures (including ones that "went green on retry"), an agent noticing something while doing other work, review comments, flaky runs, -race reports, and bugs in tests themselves (fix(deps): update rust crate axum to 0.6.20 #35).
2. Where: the test lands in the same PR as the fix. A fix without a test says why in the PR body (No test: <reason>).
3. Red first: the test must fail without the fix. Either commit the test before the fix (reviewable in history), or state "fails without the fix" in the PR body as chore(deps): bump build/buck/preludes/buck2-prelude from d956689 to ba020c6 #62 did. Better still, revert the fix and quote the failure, as Phase 5: tag event bodies must travel with the event #182 did. "Fails without the fix" means the assertion discriminates, not just that the test runs. Update Rust crate proc-macro2 to 1.0.81 - autoclosed #75's first --typecheck assertion matched text present with or without the flag and would have passed either way.
4. "Cheap" means roughly: a unit test, or an e2e test that adds < ~2 s. When it is not cheap (needs a real server, an OS CI does not have, minutes of setup), file an issue labelled testing with the reproduction instead of dropping it.
5. Permutations, aggressively. Test the bug's shape: a Go table test over the dimensions the bug lives in (runtime deno/bun/node x snippet/file x daemon/inline x socket/TCP is mcpx's usual cross-product; add "one configuration vs two" and "one run vs concurrent runs" after Update Rust crate bacon to 2.14.0 #69/Update Rust crate globwalk to 0.9.0 #70/Update Rust crate libtest-mimic to 0.8.0 #73), or a generator seeded from the run seed (Bump golang.org/x/crypto from 0.10.0 to 0.17.0 in /sources/graph #93). Keep the cross-product in unit or in-process tests; in e2e, pick the rows only the binary can prove, because every e2e row is a subprocess and CI is budgeted (Bump golang.org/x/crypto from 0.16.0 to 0.17.0 in /sources/launcher #95).
6. Name it for the behaviour, not the issue number (TestInlineScriptReachesTheDaemon, not TestIssue62), and put the issue number in a comment.
7. If the bug is one of a class (the same shape has shipped before, or a declaration and its use live apart), the fix also adds or extends a guard test -- Add omasnap upstream repository to flake inputs #129.

Approaches: how the policy is enforced

A -- a PR template checkbox (least recommended alone)

.github/pull_request_template.md with "[ ] regression test added / fails without the fix". Cheap. But the template is repo-wide (this is a whole nix config repo, most PRs are not mcpx), GitHub supports per-template files only via ?template= URLs, and gh pr create --body -- how agents open PRs -- skips templates entirely. A checkbox is also self-attested.

B -- the checklist in the contributor page and the review step

The rule lives in the contributor page (#131), and whoever reviews (human or review agent) checks it. Reaches agents where they actually read instructions, costs nothing in CI. Still self-attested, and misses PRs nobody reviews.

C -- B plus a cheap CI check (recommended)

B, plus a CI step (#95) on PRs touching pkgs/mcpx/**: if the PR is a fix (title starts with fix, body has Fixes #/Closes # on a bug issue, or a label says so) and changes no *_test.go / *.test.ts, the step fails unless the body contains No test: <reason>. It does not prove the test fails without the fix; that stays a review item (an optional later step could run the PR's new tests against the merge base and expect failure). Recommended because it catches the common miss for seconds of CI, leaves an escape hatch that is visible in history, and does not depend on templates agents never see.

Tasks

• Confirm or correct each row of the review table; for each "no"/"unclear", file or write the missing test (small PRs, one per bug).
• Settle the policy wording and hand it to Update Rust crate tracing-subscriber to v0.3.20 [SECURITY] #131.
• Specify the C check for Bump golang.org/x/crypto from 0.16.0 to 0.17.0 in /sources/launcher #95 (inputs, exact match rules, escape hatch).

Open questions

• Is "fails without the fix" proven by commit order, by PR-body statement, or by a CI job that runs new tests against the merge base? The last is the only non-attested option and the most expensive.
• Does a bug found and fixed inside the same unmerged PR count? (Proposed: yes if it reached a pushed commit or CI.)
• Label for flaky-found bugs: reuse testing + bug, or add a flaky label? (Label set is fixed today.)

Ease · Impact · Related

Ease S (policy) / M (review + missing tests) / S (CI check) · Impact high
Depends on / related: #93 (run seed, generators, the quoted seed bullet), #95 (the CI check, plugin tests), #97 (integration inventory), #35, #52 (closed, fixed in #182), #65, #74 (closed, fixed in #75), #75, #78, #46, #56, #58, #61, #62, #66, #69, #70, #73, #171, #179 (the setting #171's guard passed), #182; siblings #127, #129, #130, #131.

Security/auth deferred; see tracker.

Acceptance criteria

• The failing-first policy is recorded: every bug gets at least one new test in the same PR as its fix, failing without the fix (when cheap), covering the bug's permutations — and the enforcement checklist (PR template vs CI check) is decided and in place.
• Recently fixed mcpx bugs are audited against the policy, with the audit result recorded on this issue.
• Missing tests the audit finds are filed (issues or PRs) and linked 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 (audit list and links) is recorded on this issue before it is closed.