feat: make snapshot recordings deliberate and retire the hand-rolled harness - #227
Open
systemfsoftware-maker wants to merge 29 commits into
Open
feat: make snapshot recordings deliberate and retire the hand-rolled harness#227systemfsoftware-maker wants to merge 29 commits into
systemfsoftware-maker wants to merge 29 commits into
Conversation
…nd-rolled harness
The suite hand-rolled snapshot testing and got the central case backwards: `snapshots.integration.test.ts:72-75` wrote the recording and returned without asserting when the file was absent, so deleting a golden made the suite green. It carried a bespoke UPDATE_SNAPSHOTS/U and TEST_FILTER/T env protocol and a .mjs file-IO helper to do it, and because that branch never consulted Vitest's update state, no runner configuration could reach it. The 36 recordings now go through `toMatchFileSnapshot` at the same paths with the same serialization, so every recorded byte is unchanged (`git diff --exit-code` on the snapshots dir is clean, and an accidental re-mint during verification reproduced the committed bytes exactly). The step body is wrapped in `Effect.tryPromise` rather than written as a bare async closure. `tapStep` (specs/gherkin/effect/src/DoNotation.ts:43-46) tests `Effect.isEffect(raw)` and otherwise returns `Effect.succeed(scope)`, so a returned Promise is discarded — the same silent-pass defect this commit removes, in a Gherkin costume. Renamed, because the old name was the same category error the recordings invited: it named the assertion mechanism where all three sibling suites name a capability. A snapshot is a technique, not a test kind, and the file is named for the feature it drives. Adds an orphan reconciliation scenario. Vitest derives obsolete snapshots from the `.snap` manifest only (@vitest/snapshot/dist/index.js:530, 623-627, 463-466), and file snapshots never enter it, so a recording orphaned by a deleted fixture would otherwise sit unread forever. Verified: 114 pass; corrupting a recording exits 1 with a field-level diff; adding an orphan exits 1; `pnpm check:local` exits 0.
Vitest resolves `updateSnapshot` from its own environment detection (vitest/dist/chunks/coverage.DM_a_rWm.js:365-369) to `none` when std-env reports CI and `new` otherwise, and under `new` a missing recording is written while the assertion passes. The forge was therefore already fail-closed, and the whole exposure was the developer's shell and the agent's, the two run classes where an unreviewed recording actually gets committed. Observed on this tree with CI, AGENT and GITHUB_ACTIONS unset, by deleting one recording and running the suite. Before, exit 0 with the suite green and the recording silently re-minted; after, exit 1 with nothing written. `update` is a NonProjectOptions key (reporters.d.DtoKVV2s.d.ts:3572), root config only, which is where `sharedConfig.test` is spread. The deliberate path is unaffected, since a CLI `-u` sets `resolved.update = true` and wins over the config value, confirmed by re-recording the deleted file and getting back byte-identical content. Ships alone per CONST-E4, because this is the surface that judges the work rather than the work. Ten first-party vitest configs spread no shared config at all and inherit nothing from here. None carries a recording today, and that they inherit no shared policy of any kind is a separate defect
Adds a `Recorded outcome` entry under `## Test execution`, beside `Run class` and `Contract lane`. The entry settles the question this work kept tripping over. A snapshot is an assertion technique; the kinds are altitudes. Asking which suffix a recording earns is a category error, so the file holding one is named for the feature it drives and no suffix keys on the recording. It also states which gate decides what. Whether a recording is honest is decided by the runner's update state and not by any lint rule, because whether a recorded value was reviewed is a semantic property no depth-0 check can read. The reviewable-size rule is labelled review-only, since nothing mechanical enforces it
Two vocabulary claims were false of the tree. `Cell` said "no rule keys on a filename, no config enumerates a sanctioned suffix set", and both clauses are falsified by test-placement, where `path.config.ts` enumerates the sanctioned test-kind suffixes and `test-suffix-outside-src.ts` keys on them. What retired on 2026-08-16 was the thirteen cell-role suffixes, so the sentence is now scoped to that. `Flagged ambiguities` said a suffix "never scopes a check", which the three `behaviour-*` rules falsify by scoping on `.integration.test.ts`. The correction names the distinction that actually holds. What the measurement condemns is an elective label, where an author who declines the name escapes the rule and its precision runs against nothing. A compulsory one is a different instrument, and `test-suffix-outside-src.ts:18-24` makes the suffix compulsory for every test outside `src/`, leaving no unlabeled member for a suffix-scoped rule to miss. This corrects a doctrine surface the rest of the change cites as authority, so it lands on its own. The plan carries the same correction plus the rename it forced
The matcher's path and the directory the corpus-parity scenario lists were two separate spellings of the same location. A drift between them would have left the parity check certifying a directory the matcher never reads, which is the one failure that scenario exists to prevent. Both now come from a single constant, and the recorded filename is built by one helper the matcher and the parity assertion share. Behaviour is unchanged: the resolved path is identical, all 114 tests pass, and corrupting a recording still fails against the same file
Pinning `update: 'none'` in the shared config only bound the packages that spread it. Ten first-party configs imported `defineConfig` straight from `vitest/config` and kept the runner's writing default, so the guard was half-built: a recording written in any of them was still silently re-minted on a developer or agent shell. Measured in `oxlint-plugin-test-placement` with CI, AGENT and GITHUB_ACTIONS unset, using a throwaway `toMatchFileSnapshot` test whose recording was deleted before each run: before this change - exit 0, suite green, recording silently minted after this change - exit 1, recording not written Each config now spreads `sharedConfig` and `sharedConfig.test` and keeps its own include, plugins, resolve conditions, timeouts and projects array. The three `testResources/` fixtures stay out deliberately; they are inputs to the mutation runner's own tests, not suites this policy should govern. Collection is unchanged in all ten, compared per package before and after with `vitest list`: 75, 71, 247, 2, 25, 152, 61, 210, 147 and 3 tests, with the same file counts. This completes the reach half of the evaluator change. The commit that pinned the value recorded these ten as an open residual; they no longer are
The entry said replacing a recording takes "an explicit flag" without saying which one. The runner's own failure text names no flag either, so a first reader had nowhere to learn it. It now names `vitest -u` and says why the pointer has to live in doctrine rather than in the failure
Measuring the blast radius of the one-line shared test-config change turned up a non-obvious property of the task graph. A change to a file inside a workspace package's published file set moves a global cache input, so it invalidates every task in the repository rather than the dependents' tasks only. A package upstream of the changed one had its build hash move, which no dependency path can explain. The document records the falsified hypothesis alongside the finding. The root-relative input that points the test task at the shared config looks like the carrier and is not; removing it moved no hash, and deleting it would drop a real correctness declaration while fixing nothing. It also records the two controls the attribution rests on, because without them "everything invalidates" cannot be told apart from an instrument that measures file metadata: a null control rewriting identical bytes moved 0 of 17 sampled hashes, and an unrelated edit outside any published file set moved none
…hing The shared test-runner configuration is a workspace package, and a change to a file inside a workspace package's published file set moves a global cache input. Every publishable package's build hash therefore moved, and the release gate keys on that hash rather than on which files were touched. Thirty-six packages are named at `none`. Nothing any of them exports, declares or does changed; only development-time test configuration did
A stash pop left `AGENTS.md` with both halves of a conflict still in the file: two contradictory REPO-R2 rules, a duplicated REPO-R3 whose copy had been mangled into a heading, and the markers around them. The file is the repo's governing law, so an agent reading it got two incompatible answers to "when is a changeset owed" and no way to choose. The surviving REPO-R2 is the one the gate implements. The gate compares the per-package turbo build hash between the pinned base and the head, and its own fixtures pin a touched-but-unchanged package as demanding nothing. The deleted copy keyed on whether a path under a publishable package changed, and describes a globbing gate that no longer exists. This resolves what was already committed on main; nothing here is new law
# Conflicts: # packages/testing/type-testing/arethetypeswrong/core/tests/published-package-analysis.integration.test.ts
…date everything The document said a workspace package's declared file set decides whether an edit invalidates the whole repository. A negative control falsifies that: editing the manifest of two packages — declared files by any reading — moved no global term and no unrelated task hash, while the same edit to the shared test-runner config moved both. The carrier is membership in the transitive workspace closure of the root manifest's own dependencies. Twenty-three of forty-two packages are inside it. A leaf with no dependents and a package with three both sit outside and invalidate nothing globally; the config package with thirty-three sits inside and invalidates every task, including packages upstream of it. Dependent count is separated from membership by the controls, and the document now records the remedy that follows: the root manifest is the lever, and a workspace dependency there that nothing imports still costs full reach
Inheriting the shared config brought its coverage block along, and that block enables the v8 provider whenever CI is set. The v8 provider cannot load inside browser mode, so the run died fetching it and the suite exited non-zero while its three stories had actually passed. The trap only springs under CI, which is why the migration measured clean locally and failed on the first push. An agent shell sets AGENT, the shared config reads that as a developer run, and coverage stays off; the runner sets CI alone and turns it on. This suite never collected coverage before it began inheriting the shared config, and the package's own rule frames it as a proof that specs compile, import and run. Restoring that costs no dependency, where switching providers would have added one against the leaf's catalog rule. Observed with CI set and AGENT unset, the run exits 1 with three coverage errors before and 0 with three stories passing after. The only other browser-mode config in the tree already pins its own provider
…ilesystem The tests reached disk through a hand-written `.mjs` shim paired with a `.d.mts` that declared what it wanted to be true rather than what the code did. `readFileSync` returns a Buffer; the declaration beside it said Uint8Array. Nothing compared the two, because a hand-written declaration is its own author's claim and no gate reads it against the implementation. Both call sites then wrapped that synchronous IO in `Effect.sync`, which is the shape this package exists to avoid: the edge doing blocking work inside a value that promises it is describing work. Reading now goes through the platform filesystem and path services, so the byte type is whatever the service returns rather than whatever a declaration claimed, and the suites carry those services as a layer. The two helpers move into a typed module beside the existing fixture helper, which is where the directory already kept this kind of thing. Behaviour is unchanged and measured: 115 pass, the 36 recordings stay byte-identical, and deleting one still fails the run without re-minting it
The scan spends 98% of a nine-package law suite searching for witnesses that cannot exist. One measured pass: 141 arms yield 2 obligations, and two schemas hold 99.7% of the time while producing none. Every one of their arms comes from a decoding default, which widens acceptance, so the value the search wants - accepted by the weakened schema and rejected by the original - is absent by construction. The plan's first draft was wrong in two ways that review caught. It proposed memoising arbitraries on AST identity; Effect already memoises that call on that key, and every weakened AST is a fresh clone, so the cache misses twice over. It also claimed the AST exposes nothing to tell a total default from a fallible transform; the link's transformation does carry a discriminating tag, and the real obstacle is narrower - within one tag, a default built from a succeeding effect is indistinguishable from one built from a failing one. Correcting those closed enough doors that the remaining constraints admitted no mechanism at all, so the budget moves from forbidden lever to candidate, gated on evidence the first unit collects rather than on convenience
…that came with it Taking the whole shared config to reach one setting also switched on v8 coverage under CI, and this package runs a schema-law sweep that instruments badly: 90s became 258s locally and ran past the 300s budget on a runner, failing the only task of 208 that failed. The setting this package needed was the update pin. It now takes that and nothing else, and the sweep is back to 90s with every test passing.
# Conflicts: # packages/testing/type-testing/arethetypeswrong/core/tests/compiler-host-cache.integration.test.ts # packages/testing/type-testing/arethetypeswrong/core/tests/published-package-analysis.integration.test.ts
…breached it A stated ceiling nothing measures is a ceiling nothing keeps. The doctrine puts a reviewable recording at a few dozen lines; the tree holds fourteen totalling five and a half thousand, the smallest nearly three times the limit, and no check anywhere reads their size. The first draft of this plan deleted them and then added the gate, which review rejected on the repository's own terms: a gate must name a wrong thing that specifically happened, and deleting every instance before the gate can witness one leaves a check whose only red is a fixture its author planted. The order is now reversed, so the gate is seen failing on all fourteen before any is removed. Two further corrections came from review. The sibling test named as already covering the deleted capability turned out to hold one scenario over one of sixteen fixtures, so the deletion now carries a replacement for each. And the rescued completeness claim cannot live beside the registry it describes, because that registry is exported and the placement rule admits an in-source block only where it reaches something private
…r will read The doctrine puts a reviewable recording at a few dozen lines and nothing measured it, so the tree accumulated fourteen totalling five and a half thousand, the smallest near three times the limit and the largest fifteen. Run against the tree as it stands, the gate exits 1 on fifteen findings: all fourteen recordings with their sizes, plus the call that reaches them, whose path is built by interpolation and so cannot be resolved without running the suite. That last one is reported rather than skipped, because a recording the gate cannot measure is the one an author would reach for next. It finds a recording the way a reader does - through the assertion that reads it. Nothing keys on a directory or a chosen filename, so moving a recording somewhere unconventional does not hide it. The one filename it does trust is the one the runner derives rather than the author, and each pooled entry in such a file is measured separately so the size is blamed on the assertion that grew, not on its neighbours. The overloaded matchers take their recording from whichever argument is string-shaped, since passing property matchers first would otherwise measure an object and pass everything
…omy can build The contribution gate named three suffixes. Two of them are cell-role names from a taxonomy retired in August, whose retirement says plainly that no config enumerates those roles, and the placement rule sanctions exactly one test file under a package source tree, so neither class can be constructed here at all. Zero exist. The gate has been claiming to police two file kinds that cannot be written. Only a workflow is a business decision worth a property, which is what the remaining suffix names. An adopter is not this repository, and that is what makes this a break rather than a tidy-up. The placement rule ships in a different package and is not a dependency of this one, so someone who installs the runner alone can hold those files today and is gated on them. After this they are not, silently, which is the worst way to lose a check BREAKING CHANGE: requireTestContribution now defaults to the workflow property suffix alone. A consumer relying on the previous default to gate policy or kernel property tests must set the option explicitly to keep them covered
… in for One feature block held three unrelated things: a claim about the fixture registry, sixteen comparisons against recorded JSON, and a failure path. The rule meant to catch that shape counts feature blocks rather than what the scenarios assert, so eighteen of them under one block went through. The assertion worth keeping was already there, sitting under the recordings and reachable only when the fixture's name happened to be a problem kind, so for four of the sixteen it ran and asserted nothing. It now derives its subjects from the schema instead of filtering them, which removes the branch entirely and makes a newly declared kind fail rather than quietly go unexercised. The four that are not named for a kind each say what they are for instead. The registry claim moved to its own file, since it describes the fixture set rather than the analysis, and putting it back beside the analysis scenarios would rebuild the drawer. It cannot live beside the registry either: that binding is exported, and an in-source block is admitted only where it reaches something private. Five and a half thousand lines of recorded JSON are gone with it. What they pinned beyond the reported kind was never read by anyone, and the size gate now refuses that trade on the next author's behalf
…e rule The guard reached for its parser through inline specifiers and pinned two lint suppressions on top to keep the rule quiet. Suppressing that rule is what the rule exists to stop, and the specifiers were the reason it fired. They also took the guard outside the config that governs everything under this directory, and that config's comment already says what happens next: without it Deno migrates the workspace definition into the package manifest. It did. A workspace list, a catalog and a second catalog block appeared in the root manifest, alongside a lock file at a root that has no Deno config at all, and every one of them was a duplicate of something the workspace file already owned. Declared as ordinary entries and imported by name, the parser resolves through that config again. The manifest keeps its own shape, the stray lock does not come back, and both suppressions are gone rather than justified
Two ways past it, both silent. A recording supplied by interpolation was measured at its static parts, so an empty pair of backticks around a variable scored zero lines and passed whatever it expanded to at runtime. The path side of the same gate already refused that shape; only the content side concatenated the fragments and called the result the recording. Removing the concatenation makes that helper identical to the one beside it, so it is gone and both callers share the survivor. Interpolation now reaches the same unmeasurable verdict from either side. The second was the matcher reached by bracket rather than dot, which the walk skipped because it only recognised one of the two accessor forms the language offers for the same call. Both are in the selftest, which fails on either without the fix
… we depend on The handler spread the incoming content into its reply, so its return type was inferred as the host's own content union. Those members live in a package this one never declares and cannot resolve from its own directory, reached only through the agent package it does declare. A consumer reading the emitted types would be sent somewhere their install need not have. The agent package already exports the return type for this handler, and the file already lists it among the shapes a dispatch can produce. Annotating the reply with it keeps the same runtime value and leaves the emitted type reachable from the dependency that supplies it
Nine sites each spelled out the same record and rebuilt the same location prefix by hand, so the structured fields and the text a reader sees were free to drift apart with nothing to notice. The selftest only ever asserted which kind a case produced, never what it said, so the drift would have shipped. One constructor takes the kind and the detail and derives the rest. The one finding that reports a path other than the file it was reached from keeps its own message and borrows only the location helper. Output over a corpus exercising all nine paths is byte-identical before and after
It inherited the base for a library that publishes declarations, so the compiler was checking every inferred type for whether it could be written down in a `.d.ts`. This package emits no `.d.ts` at all — the bundler is set to skip them — so that check was grading it against an artifact it does not produce, and the failure it raised was about a consumer who cannot exist. Last commit answered that by annotating the one return the check objected to. That accepted a false premise and left the wrong base in place to catch the next inferred type, so the annotation goes and the base changes instead. The inferred type is what it always was. Nothing project-references this package, so the composite flag the library base carried has no dependent to lose BREAKING CHANGE: the plugin no longer emits declaration output or a composite build info file. A consumer importing its types rather than running it must depend on the packages those types come from
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
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.
What
arethetypeswrong-corehand-rolled snapshot testing and got the central case backwards: theThenstep wrote the recording and returned without asserting when the file was absent, so deleting a golden made the suite green. It carried a bespokeUPDATE_SNAPSHOTS/U+TEST_FILTER/Tenv protocol and a.mjsfile-IO helper, and because that branch never consulted Vitest's update state, no runner configuration could reach it.This replaces it with the runner's own mechanism and makes recording a deliberate act repo-wide.
Evidence
The defect and the fix were both observed, not argued. With
CI,AGENTandGITHUB_ACTIONSunset, deleting one recording:The same red/green was measured independently in
oxlint-plugin-test-placement, one of the ten packages that previously inherited no shared policy.Other probes: corrupting a recording exits 1 with a field-level diff; adding an orphaned recording exits 1;
-ustill re-records and returns byte-identical content; all 36 recordings are byte-identical;pnpm check:localexits 0.Changes
toMatchFileSnapshotat the same paths with the same serialization. The step body is wrapped inEffect.tryPromisebecausetapStepchecksEffect.isEffectand otherwise discards the value — a bare async closure would be the same silent-pass defect in a Gherkin costume..snapmanifest only, and file snapshots never enter it, so a recording orphaned by a deleted fixture would sit unread forever. That check is now explicit.nonein the shared runner config, and the ten configs that spread no shared config at all were migrated so it actually reaches them. Collection verified unchanged per package.Recorded outcomeentry: a snapshot is an assertion technique, never a test kind — the kinds are altitudes. TwoCONCEPTS.mdclaims about filename suffixes that the tree falsified are corrected.Renamed
snapshots.integration.test.ts->published-package-analysis.integration.test.ts. Every sibling suite names a capability; this one named its assertion mechanism. An integration test drives a feature, and a snapshot is not one.Also fixes, on main and unrelated to the feature
AGENTS.mdhad a stash-pop conflict committed into it: both halves still present, two contradictory REPO-R2 rules, a duplicated REPO-R3 mangled into a heading, and the markers around them. The governing law gave two incompatible answers to "when is a changeset owed".Resolved in favour of the rule the gate actually implements — the hash verdict. The deleted copy keyed on paths under a publishable package and describes a globbing gate that no longer exists.
Release intent
Corrected during review. An earlier revision of this description claimed no changeset was owed, reasoning that
tests/**is not a build input. That is true and irrelevant: the shared test-runner config is itself a workspace package, and a change inside a workspace package's published file set moves a global turbo cache input. Every publishable package'sbuildhash therefore moved, and the gate keys on that hash.36 packages are named at
none— nothing any of them exports, declares or does changed. The gate exits 0.Review
Simplify pass (3 reviewers): 2 reuse findings applied, 6 skipped with reasons. Code review (4 reviewers): correctness returned zero findings with all five load-bearing claims verified against installed source; three reviewers independently converged on the ten-config bypass, now fixed.
One advisory stands by design:
-uoverrides the pin. That is the deliberate escape hatch, and it is now named in doctrine because the runner's failure text names no flag.Notes
Mutation runs are advisory and CI-owned per
REPO-D3; none started locally.🤖 Generated with Claude Code