Skip to content

feat: make snapshot recordings deliberate and retire the hand-rolled harness - #227

Open
systemfsoftware-maker wants to merge 29 commits into
mainfrom
snapshot-testing
Open

feat: make snapshot recordings deliberate and retire the hand-rolled harness#227
systemfsoftware-maker wants to merge 29 commits into
mainfrom
snapshot-testing

Conversation

@systemfsoftware-maker

@systemfsoftware-maker systemfsoftware-maker commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

What

arethetypeswrong-core hand-rolled snapshot testing and got the central case backwards: the Then step 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 + TEST_FILTER/T env protocol and a .mjs file-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, AGENT and GITHUB_ACTIONS unset, deleting one recording:

exit recording
before 0 (green) silently re-minted
after 1 not written

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; -u still re-records and returns byte-identical content; all 36 recordings are byte-identical; pnpm check:local exits 0.

Changes

  • Assertions go through the runner. 36 recorded analyses now compared with toMatchFileSnapshot at the same paths with the same serialization. The step body is wrapped in Effect.tryPromise because tapStep checks Effect.isEffect and otherwise discards the value — a bare async closure would be the same silent-pass defect in a Gherkin costume.
  • A parity scenario. Vitest derives obsolete snapshots from the .snap manifest only, and file snapshots never enter it, so a recording orphaned by a deleted fixture would sit unread forever. That check is now explicit.
  • The update state is pinned to none in 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.
  • Dead machinery deleted — the private snapshot schema and five unused IO helpers. Zero surviving references.
  • Doctrine. A Recorded outcome entry: a snapshot is an assertion technique, never a test kind — the kinds are altitudes. Two CONCEPTS.md claims 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.md had 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's build hash 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: -u overrides 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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant