test(config): freeze the records and add the configuration mutation gate (PLT-893, phase 1) - #3860
Closed
bdchatham wants to merge 3 commits into
Closed
test(config): freeze the records and add the configuration mutation gate (PLT-893, phase 1)#3860bdchatham wants to merge 3 commits into
bdchatham wants to merge 3 commits into
Conversation
…PLT-893) Three tests could not fail, each proven by mutating the code the test names and watching it pass. Test-only: no production file changes. sei-cosmos/server/config was red under `go test -count=2` because TestSetConfigTemplate wrote the package-global app.toml template and never restored it, so three later rows rendered a one-section template. The restore is now explicit and asserted, with t.Cleanup retained for the panic path -- SetConfigTemplate assigns text/template's (nil, err) to the global before it panics, so a recovered caller would leave it nil. The row also now asserts the template is installed, not merely that the call did not panic: a setter that parsed and discarded passed before, which would leave `seid init` writing an app.toml with no [state-commit], [state-store], [receipt-store] or [evm] sections at all. Event sinks were compared by len(sinks) at six sites, so "null replaces the whole set" held whichever sink won -- a node indexing every transaction and one indexing none agree on every count. They now compare identity. Both list orders are driven: selection ranges a map, and the first-listed name lands in the first of its eight slots while the range starts at a random slot, so the minority order appears about one time in eight (measured 12.4% over 400,000 runs). TestBaseAppTracingDisabledByDefault asserted only that construction did not panic, which is true whether tracing is on or off. It now asserts that neither the app's own tracing gate nor the process-global provider yields a live span, using SpanContext().IsValid() rather than IsRecording() so a stray OTEL_TRACES_SAMPLER cannot make the row lie. A companion row pins the enabled path: it measures the batch-processor goroutine the provider leaves behind, calls app.Close() to establish that production's lifecycle close does not release it, then shuts the provider down itself and asserts the count returns. Recorded, not repaired, per the standing decision not to change shipped behavior: EventSinksFromConfig opens the tx_index store and discards it without closing on the orders that reach kv first, holding a goleveldb LOCK and five goroutines for the process lifetime; and NewBaseApp writes the process-global tracer provider from a constructor, shadowing the handle so nothing can shut it down. Each row says what a fix looks like and asks for the record to be updated rather than the assertion widened. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…893, FR-001/FR-002) -update is one process-global switch over three independent records: the defaults record CheckDefaults compares, the key-name record CheckKeyNames compares, and requireKeyNameRecord's cross-check, which stands down rather than compare against a file being regenerated in the same run. No workflow inspected the tree afterwards, so a CI run holding the flag would have rewritten all three and reported success -- a test comparing against a record regenerated from the code passes by construction, which is the most convincing kind of test that proves nothing. The refusal sits at the single point every record write passes through, writeGolden, rather than at each caller. A guard repeated per call site is a convention the next caller can forget; a guard at the one writer is an invariant they cannot. keynames.go therefore needed no change at all. requireKeyNameRecord is the one caller with anything of its own, because it suppresses a comparison rather than performing a write and a suppressed comparison reads as a pass; it asks one named question. CI joins Isolate's allowlist. The refusal reads the environment and Isolate strips everything outside that list, so a check that isolated before writing would have found CI unset and written the record with nothing to notice. Verified the cost is nil rather than small: no key in the closed read-key set is named ci, and the env lane recorded no bare CI resolution, so the empty-prefix viper has nothing to resolve it to. TestRefusalSurvivesIsolate keeps the entry from being tidied away -- removing it fails with a message naming the fix. Both go-test jobs gain two steps. One asserts CI is set, because on a self-hosted runner that variable is the runner's to provide and without it the refusal is silently inert. The other fails the job when the suite leaves the tree dirty, which covers what the refusal cannot: a package with its own -update flag that never routes through writeGolden, as sei-tendermint/internal/p2p/conn does. Verified by removing each refusal route in turn and watching the contract test redden, with go vet reporting no build error so the failures are assertion failures rather than breakage. CI=true go test ./x/evm/querier/ -count=1 -update exits non-zero naming all three records it declined to rewrite and leaves git status --porcelain empty. The existing -update round-trip contract test still passes. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…-004/FR-005) A prior audit renamed 27 operator-facing app.toml key names with the whole suite green, so "the suite covers these keys" and "the suite would catch a change to them" turned out to be different claims. A test that cannot fail is indistinguishable from one that works, by reading. This settles the second claim by experiment: it breaks production code in a recorded way and observes what the suite does about it. Each row of expectations.tsv is a three-part observation. The named packages must PASS before the patch, because a non-zero exit alone cannot distinguish a real catch from a package already failing -- and this tree contains a live example, where one patch reddens evmrpc/config only through a hand-written require.NotNil unrelated to the configuration suite. Then the patch is applied and the packages must FAIL. Then the failure output must contain the row's recorded substring, since a package can go red for a reason other than the row's own assertion. First run, every verdict measured rather than predicted: 11 rows, 10 mutations observed, 4 caught and 6 not. The six are inventoried as EXPECTED-GREEN naming the requirement that closes each, because an inventoried gap and a hidden one behave identically on a green CI run and only one of them is being worked on. A row moving from EXPECTED-GREEN to EXPECTED-RED is the unit of progress for the coverage work. The gate refuses to report success over zero observations -- the failure it exists to detect, applied to itself. An absent or empty expectations file aborts, a file of rows that apply no patch fails, and a NOT-OBSERVABLE row naming no enabling change fails, because otherwise it is a one-line way to silence a row that started failing. Every mutation, revert and test run goes through one interface, so the verdict logic is driven by ordinary unit tests with a fake rather than by editing copies of the tool. Those tests are what prove the gate can fail: each of the three arms has a passing case and a failing case, and the fake matches an ordered script, so a gate that skips an arm produces the wrong call and fails rather than passing quietly. That ordering caught two defects during the port -- a failed git apply that never reverted, leaving a partial patch for the next row, and an unapplied patch counted as an observation and then judged against a run that never happened. The real implementation works inside a throwaway git worktree. An interrupted run therefore cannot leave a patch applied in anyone's checkout, and the gate always measures the committed state rather than whatever is being edited -- so a dirty checkout is a notice rather than a refusal, and the tool is usable mid-edit. Every subprocess has a deadline and is killed by process group, because go test spawns child test binaries that would otherwise outlive a cancel and a deadlocked test would hang the whole run. Wired as its own CI job with its own timeout, ordered so the unit tests run before the observation: an instrument nobody has shown can fail cannot certify anything. make mutation-gate is the same two steps locally. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3860 +/- ##
==========================================
- Coverage 61.61% 60.73% -0.89%
==========================================
Files 2363 2276 -87
Lines 199695 189595 -10100
==========================================
- Hits 123042 115144 -7898
+ Misses 65701 64344 -1357
+ Partials 10952 10107 -845
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — opening for discussion before publishing.
Phase 1 of the coverage spec in
bdchatham-designs. Two things, and the second is why the first can be trusted. Test and harness code only — no production resolution behaviour changes.The problem this exists to solve
A prior mutation-verified audit renamed 27 operator-facing
app.tomlkey names and the entire suite stayed green. So "the suite covers these keys" and "the suite would catch a change to them" turned out to be different claims — and a test that cannot fail is indistinguishable from one that works, by reading. Three concrete examples from this tree:len(sinks), so "null replaces the whole set" passed whichever sink won. A probe foundkvwinning 357/400 runs andnullthe other 43 — a node that indexes every transaction on most starts and nothing on the rest — green in both worlds.TestBaseAppTracingDisabledByDefaultasserted only that construction didn't panic. Flip the tracing read and it still passes, while installing a real OTEL provider with recording spans.TestNativeTracerSetIsNonJSInGethnever enumeratesnativeTraceTracers, the set the function answers from. Add a JS tracer and it passes. That's the security property the test exists for.1. The record refusal (
e151b2f04)-updatewas one process-global switch over three independent records, and no workflow inspected the tree afterward. A CI run holding it would have rewritten all three and reported success — a test comparing against a record regenerated from the code passes by construction.The refusal sits inside
writeGolden, the single point every record write passes through, not at each caller. A guard repeated per call site is a convention the next caller can forget; a guard at the one writer is an invariant they cannot — sokeynames.goneeded no change at all.requireKeyNameRecordis the one caller needing anything of its own, because it suppresses a comparison rather than performing a write, and a suppressed comparison reads as a pass.CIjoinsIsolate's allowlist, because the refusal reads the environment andIsolatestrips everything outside that list — a check that isolated before writing would have foundCIunset and written the record with nothing to notice. Verified the cost is nil rather than small: no key in the closed 481-key read set is namedci, and the env census recorded no bareCIresolution.2. The mutation gate (
fbf8a7138)Each row of
expectations.tsvis a three-part observation:evmrpc/configonly through an unrelatedrequire.NotNil.First run, every verdict measured rather than predicted: 11 rows, 10 mutations observed, 4 caught and 6 not. The six are inventoried as
EXPECTED-GREENnaming the requirement that closes each — an inventoried gap and a hidden one behave identically on a green CI run, and only one of them is being worked on. A row movingEXPECTED-GREEN→EXPECTED-REDis the unit of progress for the rest of the coverage work.The gate refuses to report success over zero observations — the failure it exists to detect, applied to itself. Missing or empty expectations file aborts; a file of rows that apply no patch fails; a
NOT-OBSERVABLErow naming no enabling change fails, because otherwise it's a one-line way to silence a row that started failing.Where review effort is best spent
The
Treeinterface and its ordered-script fake. Every effect goes through one interface, so the verdict logic is driven by unit tests rather than by editing copies of the tool. Each of the three arms has a passing case and a failing case, and the fake matches an ordered script — so a gate that skips an arm produces the wrong call and fails rather than passing quietly. That ordering caught two defects during development:git applythat never reverted, leaving a partial patch for the next rowBoth were sequencing bugs invisible to static review.
The worktree decision. The gate works inside a throwaway
git worktree, so an interrupted run cannot leave a patch applied in anyone's checkout, and it always measures the committed state. A dirty checkout is therefore a notice, not a refusal — the tool is usable mid-edit. Verified after a full run: 0 dirty files, 0 leftover worktrees, 0 leftover temp dirs.Subprocess lifecycle. Every
git/gocall has a deadline and is killed by process group, becausego testspawns child test binaries that would otherwise outlive a cancel. Without this a deadlocked test hangs the whole gate until CI's own kill, with no diagnostic.Provenance worth knowing
This tool was first written in shell and rewritten in Go inside this branch, so the shell version doesn't appear in the diff. Nine defects were found in 406 lines of shell — tab-collapse in the field reader shifting every column after an empty one, repo location by
$0arithmetic, aseddelimiter colliding with its own pattern, a missing-file redirect whose behaviour is shell-dependent on the tool's primary input,go testinheriting the read loop's stdin,git checkout -- .unable to revert file additions, a header prefix-match silently dropping any row namedpatch*, and unbounded output capture. Every one is a class Go doesn't have, and no shell linter exists in this repo, so none of it was machine-reviewed.Acceptance proof: the Go gate reproduces the shell's measured result exactly — same 11 rows, 10 observations, 4 red / 6 green / 1 not-observable, same substrings attributed.
Cost
make mutation-gateis ~6 minutes, dominated by./app/at 37.7s per run appearing in 5 of the 19go testinvocations, plus cache invalidation from patches to shared harness files. It runs as its own CI job with its own timeout, so it doesn't spend the test job's budget, and it is not in the localgo test ./...path. Deliberately not parallelized: the work is compile-bound on a shared build cache and concurrent runs would contend rather than overlap.Verification
gofmt/goimportsclean;golangci-lint(v2.8.0, CI's pin) 0 issuesgo test ./testutil/configtest/...green — 22 gate unit tests in 0.45smake mutation-gategreen end to end🤖 Generated with Claude Code