Skip to content

refactor(steps): give every step its own packaging, benchmark, docs, and tested contract - #136

Merged
DevilTea merged 54 commits into
mainfrom
refactor/step-unit-standardization
Jul 31, 2026
Merged

refactor(steps): give every step its own packaging, benchmark, docs, and tested contract#136
DevilTea merged 54 commits into
mainfrom
refactor/step-unit-standardization

Conversation

@DevilTea

@DevilTea DevilTea commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Closes #134. Phase 4's remainder is #135.

Every built-in step becomes one self-contained unit that carries its own implementation, tests,
benchmark, and documentation — and the docs site, the performance gate, and the completeness gates all
read from those units instead of from a second hand-maintained copy.

Phase 1 — one canonical step shape

The standard is written down in .claude/skills/valchecker-dev/references/step-unit.md and enforced by
pnpm steps:complete, applied to all 114 steps with no behaviour change. Fifteen auxiliary test files
folded into their step's suite; twenty-two implementation files reordered; four steps de-prefixed. The
sorted multiset of it/test/describe title lines under steps/ is identical before and after, and
so is the total assertion count — that is the no-behaviour-change evidence, checked independently of
the agents that did the work.

The ordering rule is an allow-list: only erased syntax may precede PluginDef. An adversarial
review of the first version closed four ways past the rules, each now with a test named after it — a
.types.test.ts holding only runtime assertions, a one-line helper re-admitting a folded suite, a
namespace without declare smuggling a value above PluginDef, and a cross-step test whose family
was also a step name.

scripts/generate-bench-files.ts is deleted: nothing referenced it, and its output would have been
rejected by the gate that recommended it.

Phase 2 — per-step benches drive Performance Impact

benchmarks/src/scenarios/* is no longer measured by the impact gate. It stays as the unit of
performance-comparison.yml and of check-benchmark-coverage.ts, because a competitor's spelling of
the same work cannot live inside a step's own bench file.

A step declares cells as data through stepBench('<step>', [cells]). One declaration, two drivers:
stepBench() calls vitest's describe/bench for the local pnpm bench loop and pushes the
resolved cells into a registry; the gate imports the same file in a plain Node process under two
resolution hooks — vitest to a no-op shim, and the '../..' each bench file already had to the dist
build under test — then reads the registry. There is no second copy to drift, and the measured artefact
is the 186 KB bundle rather than TypeScript source.

Cells: 341 → 245. Async cells 1 → 12, collectAllIssues cells 0 → 9, message cells 0 → 2,
JavaScript baselines 9 → 0. The old suite was not fit to be measured: object, strictObject and
looseObject all benched an empty shape, so the child-execute loop never iterated; bigint and
symbol had no success cell at all; 31 steps constructed the schema inside the timed region.

pnpm bench:cells enforces the contract by driving the cells, not by reading them — every cell
produces the issue codes it declares, every step has a success cell, every issue-owning step has a cell
producing its own code, construction is outside the timed region (AST-checked), and the import
allow-list holds. What it cannot decide, its messages say: whether an input is representative, whether
a batch measures work worth measuring.

The gate now shards four ways and merges before the verdict. The "deliberately NOT sharded"
rationale is replaced with the measurements that refute it: 55m12s of measurement against ~40s of
non-shardable fixed cost, so four shards is ~28.5 minutes.

Classification is interval-based. severeScenarios used to require row.stable, so an alarming but
imprecise cell was a silent pass — #124 named that failure mode and treated it by buying precision.
Now cleared / regression / severe / inconclusive are decided against the confidence interval,
stabilityThreshold is a reported diagnostic, and the group aggregate runs over decisive rows.

Two CI null runs (before == after) validated this with a real counter-example: run A classified
construct/tuple as a regression at −5.32% with pairedRme 3.12% — stable, precise, and false.
Its interval spans −5%, so the new rule returns inconclusive. The same runs measured a 32% flip
rate
in the stable verdict between identical runs, which is why it is no longer the decision input.

Phase 3 — the API reference is composed from the steps

Each step owns a <name>.doc.md opening with a <!-- step-doc --> block declaring category,
section, summary. Category is declared, never inferred, and a step with no entry, an unknown
category, or a section no template offers fails loudly — a step silently missing from the site with a
green gate is the failure this phase exists to remove.

pnpm docs:api / docs:api:update mirror the api:surface pair and run inside pnpm verify. Six
pages, the catalog, and the sidebar region are generated; the non-step prose lives in six templates.

A whole-corpus audit classified all 798 prose units of the old 1,629-line reference: 532 verbatim,
237 reworded, 28 deliberately dropped, 1 lost and restored. Review risk concentrates in the 51 steps
that previously had only a bullet, whose entries are newly written.

step-completeness no longer scans the site. It reads each step's own entry, which is a stronger rule
than the one it replaces — that one could be satisfied by a page saying the step does not exist.

Phase 4 — partial, stopped to conserve budget

Refuted repository-wide by mutation sweeps: all 110 owned issue codes are genuinely asserted, all 92
customMessage paths, all 92 payload shapes. The epic's central suspicion — an issue code appearing in
a string that never reaches an assertion — does not hold anywhere.

Real defects fixed, each with the regression test that proves it:

  • array() and tuple() re-read value.length on their asynchronous paths while the synchronous entry
    points snapshot it, so a child mutating the array it was validating extended its own traversal;
  • map() and set() misreported firstIndex once collectAllIssues collected past a failed entry;
  • isOneOf accepted undefined against a candidate list excluding it — found by mutation, not reading;
  • two unpunctuated public default messages, the only two in the library;
  • isRecoverableFailure deleted: git shows it was introduced uncalled and never had a caller.

Refuted rather than fixed: tuple's rest-schema TypeError does not escape the result type (the
core catches it and reports core:unknown_exception), and getter-counting cases already existed.

Mutant triage finished for three slices — 82 distinct mutants, about four fifths real, nearly all now
killed. That corrects the "~1/3 real" estimate in #135, along with two claims of mine withdrawn in
dcbfa80: see below.

Two withdrawn claims, and the process failure behind one

dcbfa80 withdraws a changelog entry and a source comment that measurement does not support.

The looseObject "defect" never existed. main has i < keysLen; the i <= keysLen I fixed was a
live mutation from a concurrent audit sweep, captured into eef3199 by my own
git add packages/internal/src over a shared checkout. Restoring it left the file identical to main.
The lesson is in the tooling, not the judgement: a mutation harness and a broad git add must not share
a working tree.

The record claim — a finite key domain collecting past an internal issue — does not reproduce. It
returns one internal issue on both paths; a second appears only when a recoverable issue preceded it,
which is the documented contract.

Verification

pnpm verifyexit 0: 9/9 quality gates, 147 test files / 2292 tests, per-file coverage policy
passed, api-surface matches, 27 documentation pages compile, docs site builds.
pnpm typeperf — all deterministic compiler-complexity metrics within budget.

Not validated: the sharded workflow end to end. The matrix, artifact names, merge wiring, and
download glob have local equivalents only — no CI run has exercised them, and this pull request's own
Performance Impact run is the first. Bundle-size is unchecked.

Known remainder

  • The targeted retry pass is not built. compareResults emits inconclusiveScenarios, but nothing
    re-runs them, so an inconclusive cell is reported and not resolved.
  • measured N / added M / removed K is not printed unconditionally; unmeasurable cells are named, but
    added/removed counts against the baseline are not computed.
  • The compare job's summary lost its before/after SHA table when the jobs were split.
  • benchmarks/README.md still describes scenarios as the impact unit.
  • Phase 2 extended the verdict decision in two places and flagged both as its reading rather than the
    decision's letter: improvements count as decisive in the group aggregate, and inconclusive gets its
    own verdict but does not fail the job.
  • Phase 4's audit remainder, the dead code at intersection.ts:522-532, two cross-step tests that do
    not discriminate what they claim, and one product decision about live re-yield in map/set are all
    in test: finish the step and core audit stopped part-way in #134 #135.

🤖 Generated with Claude Code


Update — hardening pass after external review

An external review of the phase 2 design was recorded on #134 and evaluated. Six items landed. This
section supersedes the "Verification" and "Known remainder" lists above where they disagree.

Two of the review's points reversed decisions recorded earlier in #134, and both reversals were
accepted with the reason stated:

  • The group verdict no longer filters to decisive rows. Filtering to rows whose own measurement
    happened to be decisive conditions the aggregate on the measurement outcome — cells with larger
    effects or lower noise are likelier to survive the filter, so the group estimate was biased toward
    the rows most likely to trigger it. The group is now its own estimator: per repetition, the scoped
    cells' log-ratios are averaged into G_r, and the Student-t interval is formed across repetitions.
  • The retry does not pool. Adding samples until an interval stops straddling a boundary is optional
    stopping, and "precision target, pre-declared" does not escape it once the set being extended is
    chosen by the first result. It is now two fixed stages — a 5-repetition screen for every selected
    cell, then an independent fixed confirmation batch for candidate regression, severe, and
    boundary-inconclusive rows — judged from the confirmation batch rather than from a pool.

Also landed: one coherent estimator (the interval was centred on the mean of paired ratios while
the reported and severe-triggering point estimate was their median — two estimators, one decision; now
a single log-ratio estimator), compare/report decoupled from the build under test (the catalog is a
hashed artifact; compare loads no production code and runs build: 'false'), the Windows
generated-reference mismatch fixed at the boundary where the difference enters, counterbalancing
confirmed to have survived the shard rewrite and now gated so the next rewrite cannot drop it, and
measured N / added M / removed K printed unconditionally.

The two-stage gate earned its keep on its first real run

json/invalid-json screened as severe at −21.86% and came back cleared at +0.44% in the
independent confirmation batch. Under the previous single-pass gate that would have failed the build.

An accepted regression, and the mechanism that records it

The map()/set() firstIndex correction in this pull request costs roughly 12–15% on
map/collect-all and set/collect-all. It lands only where collectAllIssues is on and an entry has
already failed — the success path is unchanged — and the owner accepted it rather than blocking the
correctness fix on optimizing the buffered path. set/collect-all measured −15.13% and −32.37% across
two screens, so the direction is solid and the magnitude is not.

benchmarks/src/accepted-regressions.mjs records it at both levels, with bounds taken from measurement
rather than picked: a bound tighter than the gate's own run-to-run spread fails on noise instead of on a
change. The group warm/failure/all is acknowledged at −12% because the accepted cells carry it —
set/collect-all −4.25pp and map/collect-all −0.88pp of the measured −6.40%.

It is an acknowledgement, not a suppression. The reported value stays the true one, an acknowledged row
appears in the summary as acknowledged with its bound, and severeGroups still reports the trigger
as measured — a separate unacknowledgedSevereGroups is what fails the gate. Rot is checked in both
directions: an entry whose cell or group the screen reports cleared fails, so the list shrinks as the
code improves; an entry naming something the catalog does not declare fails; and a group entry whose
member cells are all unacknowledged fails, so it cannot outlive the per-cell reasons it rests on.

Staleness deliberately does not read the confirmation batch — one batch clearing a cell the screen
called severe is this gate's noise diagnostic, not evidence a cost is gone.

Two limits are documented rather than left implicit: an acknowledgement cannot tell an accepted cost
from a regression someone got tired of, and a group bound cannot tell whether the cost is still the
accepted one.

The rejected alternative, recorded so it is not re-litigated

Excluding acknowledged cells from the group aggregate was rejected. It would condition the aggregate on
which cells someone previously forgave — the same defect the decisive-row filter had — and shrinking the
denominator would dilute a new regression landing in that group instead of catching it. The argument
is in the module.

Verification, superseding the list above

Every check on this pull request passes, including the full impact chain end to end: preflight, four
measure shards, compare, four confirm-measure shards, and verdict
(run 30547023911). That is the
evidence the earlier body listed as missing — the matrix, artifact names, merge wiring, and download
glob had local equivalents only, and those local equivalents are exactly what missed both original
failures.

pnpm verify exits 0. Windows now passes; it did not before.

What still remains

  • The compare job's before/after SHA table, lost when the jobs were split.
  • benchmarks/README.md still describes scenarios as the impact unit.
  • Optimizing the buffered path in map/set, which would let both acknowledgement entries be deleted —
    and the orphan rot check means deleting the cell entries forces the group entry out with them.
  • Phase 4's audit remainder, the dead code at intersection.ts:522-532, two cross-step tests that do
    not discriminate what they claim, and one product decision about live re-yield in map/set — all in
    test: finish the step and core audit stopped part-way in #134 #135, along with an external recommendation to replace the hand-rolled mutation harness with StrykerJS.
    Deliberately not in this pull request.

DevilTea and others added 30 commits July 28, 2026 22:13
Sixteen test files held one slice of a single step's contract under a name
that read like a separate subject, in five different naming schemes:
`collectAllIssues.test.ts` in seven directories, `lazy-output.test.ts`,
`native-snapshot.test.ts`, `check.narrow.test.ts`, `map.async.test.ts`,
`strictObject.async-missing.test.ts`, and three `intersection.*.test.ts`.
A reader listing a step directory could not tell which file carried the
step's own tests.

Each is now a further top-level `describe` in `<name>.test.ts`, moved
verbatim. Almost every folded file declared its own module-scope
`const v = createValchecker(…)` over a different step list — and two of
them a different fixture — so each kept its instance by declaring it
inside its own block rather than adopting the target's. No case runs
against an instance it did not run against before.

No behaviour change: `it`/`test` registrations under
`packages/internal/src/steps` total 1105 before and after, and the folded
blocks are byte-verbatim against their deleted sources.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A step file's job is to state a contract and then satisfy it, but twenty-two
of them put the machinery first: `isEmoji` spent 120 lines building a regular
expression before naming the step, and the nine format validators opened on a
bare `const pattern`. Opening any of those files showed how before what.

Every module-scope `const`, `let` and `function` that sat above
`interface PluginDef` now sits between it and the `implStepPlugin` export,
carrying its comments with it and keeping its order among its neighbours.
Nothing forward-references: the only statement that reads these values is the
last one in the file.

`isEmoji`'s `type RegisteredSupport` moved down too, next to the
`resolveRegisteredSupport` that is its only reader, which the standard leaves
to review rather than to the gate.

Pure relocation, verified per file: ignoring blank lines, the sorted line
multiset of each of the twenty-two is identical to `main`'s, so no line's
content changed. The 283 tests over the affected steps are unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`isAtLeast`, `isAtMost`, `isLengthAtLeast` and `isLengthAtMost` named their
three module-local declarations `AtLeastInternal`/`AtLeastMeta`/
`AtLeastPluginDef` and so on, while the other 110 steps use `Internal`, `Meta`
and `PluginDef`. Nothing outside each file can see them — `declare namespace`
and a non-exported type alias are module-scoped — so the prefix bought nothing
and cost every reader comparing two steps.

`check-step-jsdoc.ts` matched `interface \w*PluginDef` because of these four.
Its comment now records that the pattern is deliberately no longer
load-bearing: a step reintroducing a prefix is caught by the naming rule that
owns it rather than dropping out of the JSDoc scan the way it used to.

Pure identifier substitution: every changed line carries a renamed identifier
and nothing else, no identifier survives elsewhere in `packages/*/src`, and the
19 tests over the four steps are unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cross-step test asserts a contract spanning a family of steps and belongs to
no one of them, so it lives at the steps root rather than inside whichever
member happened to be open when it was written. Naming it
`<family>.<aspect>.test.ts` is what keeps a single step's test from sitting
among them looking like a family contract, and three of the ten used a single
hyphenated run instead: `structural-failure-semantics`,
`structural-issue-collection` and `structural-sync-fast-path` are now
`structural.failure-semantics`, `structural.issue-collection` and
`structural.sync-fast-path`, matching the seven that already had both parts.

`callbackErrorSentinel.ts` becomes `callback-error-sentinel.ts`: a module
shared across step directories follows the same kebab-case rule as a helper
module inside one. Its three importers — `toFiltered`, `toMapped`, `toSorted` —
change only the specifier.

Pure rename; no file content changed beyond those three import paths.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All 114 step directories already held `<name>.ts`, `<name>.test.ts`,
`<name>.bench.ts` and `index.ts`, and everything past that was discipline: the
in-file section order varied five ways, sixteen auxiliary tests used five
naming schemes, and nothing decided whether a new step matched any of it.
`scripts/step-completeness.ts` said so outright — the conventions were "held by
discipline and by `scripts/generate-bench-files.ts`, which creates a bench and
then never looks again".

The standard is now one reference,
`.claude/skills/valchecker-dev/references/step-unit.md`: the file set, the
auxiliary and helper naming patterns, the steps root, and the six in-file
sections. `step-completeness.ts` gains the rules for it — the allowed entries, a
one-line `index.ts`, `Meta` and `PluginDef` under those names with `Meta` first,
no value declaration above `PluginDef`, an `Internal` namespace, the plugin as
the last statement, and the cross-step test pattern at the steps root — driven
by 27 new tests over a synthetic repository, including the limit these rules
keep: a local *type* is the same syntax in either section, so where one belongs
stays review guidance and a test pins that it is accepted either way.

`scripts/generate-bench-files.ts` is deleted. No `package.json` script
referenced it, its heuristic output did not match what a step's benchmark has to
be, and the gate pointed a failing step at it as a starting point — which would
have produced a file the same gate then rejected.

`.claude/commands/new-step.md`, the dev skill, `architecture.md` and `AGENTS.md`
now point at the standard instead of restating a subset of it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifteen auxiliary test files were folded, not sixteen: `collectAllIssues.test.ts`
in seven directories plus eight one-off names. The count is checkable — the
repository held 161 `*.test.ts` files before and holds 146 now.

Records the invariant that stands behind the "no behaviour change" claim while
it is still cheap to state: `it`/`test` registrations under
`packages/internal/src/steps`, counted from the parsed AST, total 1105 both
before and after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adversarial review of the gate got a step directory to satisfy every new
rule while violating what each stood for. Four trees, each now a test named
after it:

- a one-line `lazy-output.ts` containing `export {}` re-admitted the 231-line
  `lazy-output.test.ts` beside it, because the file set required only that the
  named module exist. A helper module must now be one `<name>.ts` reaches,
  directly or through another helper;
- a `<name>.types.test.ts` full of runtime `expect` calls used the one named
  auxiliary test as a way around the fold. Since the whole justification for
  that exception is that `pnpm typecheck` decides its assertions, it must now
  call `expectTypeOf` or `assertType`;
- a `namespace` without `declare` emits an IIFE, so a `const` inside one was a
  runtime value above `PluginDef` that an enumeration of const/function/class/
  enum could not see — as were a bare expression statement, a top-level
  `await`, and `import x = require(…)`. The rule is now an allow-list: only
  erased syntax may precede `PluginDef`;
- `implStepPlugin` was tracked by overwriting, so a second, earlier call was
  neither position-checked nor counted as a value. It is counted now, and a
  step constructs exactly one plugin.

Also enforces what section 6 already claimed — the plugin is the file's only
export — and rejects a cross-step test whose `<family>` is a step, which left
`map.async.test.ts` accepted one directory up because every all-lowercase step
directory name is also a valid kebab-case family.

The wording follows the rules rather than leading them. `step-unit.md`, the
module header, and `successMessage` no longer say "outright" or "no value
declaration precedes"; they now name the three things still undecidable — which
section a type belongs to, a helper reached rather than used, and whether any
assertion says something true.

All 114 steps pass unchanged; the script suite is 182 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gate

`CONTRIBUTING.md` is declared self-contained for outside contributors, and it
still described the pre-branch gate. A contributor adding
`collectAllIssues.test.ts` or a `const` above `PluginDef` would have hit a
failure the document never mentioned. It now states the file set, the
declaration order, and the cross-step test name, in its own words.

The remaining pointers: `testing.md` names the cross-step pattern rather than
just the directory; `conventions.md` and `SKILL.md` stop advertising a "file
layout" section that `conventions.md` does not have and `step-unit.md` now owns;
`new-step.md` and `SKILL.md` list the file-set and order rules among what
`pnpm steps:complete` decides.

`AGENTS.md` no longer counts the weak rules ("Four of those…"), since the
count was wrong in both directions as the rules changed and a number is the
part of that sentence carrying no information. It names them instead, including
the two the hardening added.

The changelog drops a figure two independent reviews could not reproduce — an
`it`/`test` registration count — for one a reader can check: the sorted multiset
of `it`, `test` and `describe` title lines under `packages/internal/src/steps`
is identical before and after at 1217 lines, so no case was added, removed,
renamed or merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`docs/api/{overview,primitives,formats,structures,transforms,helpers}.md` was a
second, hand-maintained copy of the steps, and its only link back to an
implementation was that a name happened to be spelled on both sides. Adding a
step meant editing the step and then editing a page, and the gate could only
check that the name appeared somewhere — `check-step-completeness` said outright
that a page claiming `toTrimmedStart()` does not exist would satisfy it.

So the source moves into the step unit. Each of the 114 steps now owns a
`<name>.doc.md` holding its `###` entry — description, example, and the issue
codes it owns — and `scripts/docs-api.ts` composes the six pages, the overview
catalog, and the sidebar region in `docs/.vitepress/config.ts` from those
entries plus six page templates under `scripts/docs-api-templates/`. The
templates carry the prose that belongs to no single step: the import strategies,
the naming convention, how issues are collected across the structural steps, the
optional-field shorthand, the message-priority chain, the execution result.
`pnpm docs:api` fails when a committed page stops matching, `pnpm docs:api:update`
rewrites it, and every generated page carries a banner saying so.

Three properties are load-bearing, because the absence of each is a silent
failure. A step's category is declared, never inferred: `isXxx -> formats` would
file `isInteger` under string formats and would invent a category for a step
nobody has written yet. Anything unplaceable fails — no `.doc.md`, an unknown
category, a section no template offers, a section slot no step fills, a page no
entry claims, a category page offering no section, and either kind of page-level
anchor collision, which VitePress reports without naming a cause. And nothing is
hand-ordered, so check mode is stable: pages from one declaration, sections from
the order their slots appear, entries from a code-point sort of the public name.

`check-step-completeness` now reads a step's own entry instead of scanning
`docs/api` for its name, which is strictly stronger: the old pair asked that some
code span somewhere wrote the name in call form and that each code appeared on
some page, which an entry describing a different step satisfied. What stays
undecidable is unchanged and still stated in the failure text.

A `ts` example reaches its page verbatim, directives included, so
`check-docs-examples` compiles it there as before; because check mode makes the
pages byte-identical to what the sources compose, a broken example fails one gate
or the other and cannot reach the site.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AGENTS.md`, `CONTRIBUTING.md`, the dev skill's machine-checked table and
architecture reference, and the `new-step` command all described the docs rules
that were just replaced: a name in a code span on the catalog page and on one
further page, and an issue code appearing somewhere under `docs/api`. They now
describe the rule that ships — a step's own `<name>.doc.md` entry — and name the
gate that owns the rest, `pnpm docs:api` / `pnpm docs:api:update`.

The surfaces list in `AGENTS.md` gained the two files a public change now has to
touch: the step's entry, and the page template when the change is cross-cutting.
Nothing under `docs/api` is hand-edited any more, and each of these files says so,
because the generated banner only helps a reader who already opened the page.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four entries under Changed: the reference composed from 114 per-step entries and
six page templates; the declared category and section with the list of things that
now fail loudly instead of dropping a step from the site; the stronger
`check-step-completeness` docs rule; and how a `ts` example still reaches
`check-docs-examples`.

Also corrects the entry the previous pull request added for that gate. It has not
shipped, so the unreleased notes have to describe the rule that will ship rather
than the one it introduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A whole-corpus audit accounted for every prose unit of the six old pages as
migrated, reworded, deliberately dropped, or lost. One was lost: `toKeys()`,
`toValues()`, and `toEntries()` used to annotate their inferred output types
(`string[]`, `number[]`, `Array<[string, number]>`), and the new entries showed
only the runtime result. The `toEntries()` annotation is also what evidenced the
"mutable tuples" claim, so its loss cost a reader the only place that was visible.

Two grouping fixes from the same review. `isMimeType()` sat under "Collection size
and membership", where it is neither, and the section's "every membership form uses
SameValueZero equality" read as though it covered a step that compares strings; it
now has a section of its own. And the overview lost its way in to the optional-field
shorthand when that example moved to Structures, so it links there instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m two drivers

A step's `<name>.bench.ts` has to serve two readers that want different things.
`pnpm bench` is `vitest bench` over the TypeScript source, which is the local
loop; the Performance Impact gate compares two builds of
`packages/valchecker/dist/index.mjs`, one process per cell, and a vitest
`bench()` over source measures neither of them.

So a bench file now declares its cells as data through `stepBench()`, which
registers them with vitest for the local driver and with a registry the gate
reads. The gate driver imports the same file in a plain Node process under two
resolution hooks: `vitest` resolves to a shim, and the `'../..'` package entry
every bench file already has resolves to the dist build under test. There is one
declaration, so what the gate measures is what `pnpm bench` runs, cell for cell.

A cell carries three things a `bench(name, fn)` cannot: the group it aggregates
into, what executing it must produce, and how many iterations make up one
measured unit. The expectation is verified outside every timed region, which is
how a "success" cell that actually fails and a failure cell that fails earlier
in the chain than its own step become runtime errors rather than review
questions. The batch exists because `measure.mjs` reads the clock every 16
iterations at about 15 ns a read, which on a 2.6 ns cell is 88% of the
measurement.

The declaration helper is excluded from coverage for the same reason
`**/*.bench.*` already is: nothing ships it and no test executes it. It is not
unchecked — every declaration in the repository runs through it on `pnpm bench`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audit of 25 step suites against the assertions they actually make, not the
names they carry. The mutation sweeps behind it: replacing each `createIssue`
field under `steps/` with a sentinel and running the whole suite shows that
all 110 issue codes and all 92 `customMessage` paths are genuinely asserted,
that only `isEqualTo` publishes an unasserted payload, and that 22 steps
publish a default message nothing asserts.

Removed as unable to fail: `toEntries`'s `not.toBe([...input.entries()][0])`,
whose right-hand side allocates fresh tuples on every evaluation, so it passed
for any implementation. Removed the `Test Plan`/`Coverage Goals` headers and
`should …` names from the coverage-shaped suites, along with duplicate cases
and fixtures that asserted nothing a smaller case already did.

`use.doc.md` said `use()` preserves execution mode. It preserves it only for a
`'sync'` delegate; every other mode, including a `toAsync()` delegate's
unconditional `'async'`, becomes `'maybe-async'`. Corrected, and
`docs/api/helpers.md` regenerated from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`array`'s synchronous path snapshotted the length before traversing
(`const len = value.length`, then `new Array(len)`), but `continueAsync`
re-read `value.length` on every iteration. So an element schema that appended
to the array it was validating extended its own traversal, and wrote outputs
past the end of an output array allocated for the original length: an `async`
child pushing five entries turned `execute([1, 2])` into seven outputs, against
the two the same schema returns when the child is synchronous.

The asynchronous loop now bounds itself by `output.length`, which is that
snapshot and cannot drift, so both paths visit the same indexes.

Found by a probe comparing the two paths under a mutating child; the appending
case is the regression test, and a second case pins the shortening direction,
which the pre-allocated output already handled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three exemplars across the kinds the rewrite has to cover: a cheap primitive
validator, a structure, and a transform whose cells never reached their own step.

`isEmail` is the floor — a success cell, a failure cell producing the step's own
issue, construction hoisted, batched to a unit worth measuring.

`object` is the structure, and it is where the audit found the largest hole:
`object`, `strictObject`, and `looseObject` all benched `v.object({})`, an empty
shape whose child-execute loop never iterates. It now carries a four-key shape,
a `collectAllIssues` cell — the dual traversal policy of all nine structures was
unmeasured once scenarios stopped being this gate's unit — a designated
enclosing-message cell that puts the deferred message chain under measurement for
the first time, and the designated construction and cold cells that keep module
initialisation attributable at all.

`toNumber` shows what a transform failure cell has to prove: its old failure cell
converted `'invalid'` to `NaN` and succeeded, because the conversion delegates to
`Number()` and adopts no parsing policy. Reaching `toNumber:conversion_failed`
takes a symbol, and the runtime check is what says so.

`--steps` restricts the drive to a slice, so a file being converted can be
verified without importing the ones that have not been.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… throws

Sweeping every `defaultMessage` under `steps/` and in the core found exactly two
without terminal punctuation, both public output: `check:failed`'s
`'Check failed'` and `core:unknown_exception`'s `'An unexpected error occurred
during step execution'`. Each has a sibling issue in the same file that was
already punctuated, so this is an inconsistency rather than a style choice.

`record()`'s JSDoc claimed `'record:expected_object'` means "not a plain
object", while its guard admits a `Date`, a `Map`, and any class instance. The
reference entry was corrected in phase 3; the JSDoc had been missed.

`union()`, `variant()` and `record()` throw at construction and said nothing
about it — `union()`'s entry actively misled, describing an unregistered
shorthand provider as merely "not enabled" when the branch throws a `TypeError`
naming its index. All of those throws were already tested; only the prose was
missing.

`tuple()`'s execution-time rest guard is now documented and tested. It was
reported to me as the one runtime exception escaping the result type; it is not.
The core catches the `TypeError` and reports the fatal internal issue
`core:unknown_exception` with `payload.method` `'tuple'`, which the new test
pins.

`isRecoverableFailure` is deleted. It was introduced in bbed8db (#30), the only
commit ever to touch the symbol, already without a production caller; it is
absent from `api-surface.json` and from `core/index.ts`'s export list, so no
module outside `core.ts` could reach it. Its three assertions went with it,
while the sibling `hasInternalIssue` it wrapped keeps its own — that one has a
real caller in `fallback.ts`, and its test now also covers an internal issue
after a recoverable one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ude it

Found by mutating every comparison and logical operator in every step
implementation and re-running the suite: changing `index < expectedValues.length`
to `index <= expectedValues.length` left the whole repository green. With that
bound the scan reads one past the last candidate, which is `undefined`, and
`Object.is(value, undefined)` then accepts an `undefined` input against a list
that never contained it.

The new case fails under that mutation and passes without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `if (false)` wrapper made `tsc` report TS7027 on the block's first
statement, failing `pnpm typecheck`. The idiom works elsewhere only by
accident: TS7027 is raised once per unreachable region, and in
`collection-size-membership.types.test.ts` a `@ts-expect-error` sits directly
above that first statement and swallows it. Here the directives sit inside the
expressions, where the type errors are, so nothing covered the block's opening
line.

A never-invoked function keeps the body reachable, so the directives stay on the
lines that actually error and no suppression depends on statement order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The family contract asserting that an internal child failure stops traversal
covered `strictObject` and `looseObject` but omitted `object`, which has the
same contract and the same fatality rule.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
341 cells across 114 files become 245. The count fell because most of what went
was not measuring this library.

What each step now carries: one success cell on a non-degenerate input, one
failure cell producing one of that step's *own* issue codes, construction hoisted
above the timed region, and a batch sized so one measured unit is roughly 1-10 µs
rather than a few nanoseconds the harness's own clock reads dominate.

What was wrong before, from the audit and now fixed:

- cells that never reached their own step. `toString`, `toLowercase`,
  `toUppercase`, `toTrimmed{,Start,End}`, `transform`, `toLength`, `toSliced` and
  `toSorted` all had "invalid input" cells that failed inside `string`, `number`,
  or `array`. They measured the step that rejected the input;
- `object`, `strictObject`, and `looseObject` benched an empty shape, so the
  child-execute loop never iterated. `array` benched `v.array(v.any())`, where no
  element can fail. `strictObject`'s "valid input - large" actually produced
  `unexpected_keys` — a failure recorded as a success;
- `bigint` and `symbol` had no success cell at all: all three of each passed
  `undefined`;
- `literal` wrapped two 1,000-character allocations around one comparison;
- 31 steps constructed the schema inside the timed region;
- `toSize`, `toKeys`, `toValues`, `toEntries`, and `toArray` were 90% or more
  enclosing structure, so one `map`/`set` regression fired four false alarms
  elsewhere. They now sit on `as`, which installs no runtime step, so the unit is
  the transform.

What was added where the feature exists: 9 `collectAllIssues` cells, so the dual
traversal policy of the structures is measured at all; 12 async cells against the
1 the repository had; algorithm cells for `isEmoji({ registered })`,
`isIp({ version })`, `toString({ radix })`, `toSplit(RegExp)`,
`toSorted({ compareFn })`, `record`'s finite key domain, `tuple`'s rest region,
and the array/Set dispatch in `toMapped`/`toFiltered`/`isIncluding`; and the
designated cells that close the dark paths — two `message` cells for the deferred
message chain no cell anywhere exercised, `fallback`'s recovery, and five
construction/cold cells so module initialisation stays attributable.

What was removed: the 9 JavaScript baselines (two of them constant expressions
the JIT folds away), the byte-identical duplicates, and the size and option-value
variants that multiplied cells without measuring a distinct path.

Every cell's declared outcome is verified by executing it, which is how the two
`v.map`/`v.record` option-shape errors and every wrong issue-code set were found.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reading them

`benchmarks/src/step-audit.mjs` exists because a scenario's hand-maintained
`steps: []` was an unverifiable human claim: it drives every scenario's `build()`
against a recording instance and compares what was called with what was declared.
`pnpm bench:cells` is the same idea one level down.

It executes every cell once, outside any timed region, against the built dist —
through the same resolution hooks the impact gate measures through, so a cell that
works here works there — and then decides:

- every cell produces what its `expect` declares, issue codes included. This is
  the rule that separates a cell measuring its own step from one that fails
  earlier in the chain, which eight steps' cells did;
- every step has a cell that succeeds, with one allowlisted exception carrying its
  reason (`never` rejects every value by definition);
- every step owning an issue code has a cell producing one of *its own* codes;
- construction is outside the timed region, decided from the AST: a `run` is
  exactly one call on a reference built above the cells;
- a bench file imports nothing but the package entry and the declaration helper,
  which is what keeps the measured process holding the bundle rather than the
  TypeScript source;
- one measured unit is within two orders of magnitude of the 1-10 µs target.

Each message says what its rule cannot decide. The batch rule cannot decide
whether a unit is inside 1-10 µs, because it shares a machine with the rest of
`pnpm verify`. The own-issue rule cannot decide whether a failure input is
representative. None of them can decide whether a cell measures work worth
measuring — that is review's job, and the reason the cell set is small enough to
read.

`step-completeness.ts` asked for a `bench(` call, which the declaration form no
longer has. It now asks for the `stepBench(` declaration, which is what makes a
step's cells reachable by *both* drivers, and its message points at
`pnpm bench:cells` for the half it cannot decide.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate asked two questions — is this estimate precise, and is the point
estimate past the threshold — where it needed to ask one: can a regression be
ruled out? Asking it as two produced errors in both directions, and both are
measured rather than argued.

A hosted-runner null run (`before == after`, so every non-neutral result is false
by construction) returned `review` and called `construct/tuple` a regression at
-5.32% with a paired RME of 3.12%: precise by the rule that used to decide, and
wrong. Its interval is about [-8.3%, -2.4%], which spans -5%, so the honest answer
is that the run cannot tell a 5% regression from noise on that cell. In the other
direction, a row at -12% with 6% RME has an interval of about [-18%, -6%] — every
value in it a regression — and the old rule discarded it for imprecision and
passed it in silence, which is exactly what #124 named and did not fix.

So classification is taken against the 95% interval: `cleared` when the whole
interval is inside ±5%, `regression` when it is entirely at or below -5%, `severe`
when it is a regression and the point estimate is at or below -10%, and
`inconclusive` when it spans a threshold. `severe` is strictly more sensitive than
the rule it replaces, which demanded precision on top.

`inconclusive` is not a pass. It gets its own verdict so nothing downstream can
read an unsettled run as a clean sweep, and the rows are named in the report as
the retry pass's input. It does not fail the job by itself: two identical
hosted-runner runs moved 54 of 170 cells across the old precision threshold, so
failing on an unsettled row would turn a runner's noise into a red gate on pull
requests that changed nothing.

`stabilityThreshold` stays exported and reported as a diagnostic and decides
nothing. The group aggregate runs over decisive rows rather than precise ones, and
improvements count as decisive: leaving them out would compute a geometric mean
over regressions and cleared rows only, which is biased toward firing the group
trigger on what is really a trade-off.

Repetitions stay at 5 and no threshold moves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runner for the impact gate's new unit. It writes the same `raw.json` shape a
scenario run writes, which is what makes this a change of unit rather than a
second gate: `impact-verdict.mjs`, `comparability.mjs`, `sharding.mjs`, and
`merge.mjs` read a cell run with no knowledge that its rows are cells, so the
classification, the identity guards, and the shard merge are the ones already
tested.

One process per cell, for the reason the cross-library suite already measured: an
identical array pipeline measured 83.5 ns as the first array-carried scenario in a
process and 261.9 ns after three others, and a gate whose numbers depend on what
ran before them cannot attribute a change to a diff.

Cell definitions come from the checked-out ref only. The apparatus has always been
fixed while `before` and `after` are two builds it points at, and cells are part
of the apparatus — if each side read its own, an author editing a bench file would
be editing the measurement and `inert-change.ts` could not see it. The cost is
that a cell which cannot execute against the baseline build has no baseline
number; it is reported as unmeasurable, by name and with the reason, in the run's
log and in `unmeasurableCells`.

`compare` gains `--catalog cells`, defaulting to cells, so a group's denominator
is every cell every step declares rather than only the ones a scoped run selected.
That is what lets a group row read `5/113` instead of `5/5`. `--catalog scenarios`
still reads the cross-library suite, which remains the unit of
`performance-comparison.yml`.

Checked locally: five cells, three paired repetitions per side, both sides the
same build — the plumbing produces a verdict, and the group rows carry the full
catalog denominators. That run says nothing about noise; it used the smoke profile
on a loaded laptop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… them

The second half of the impact selection. The first half is untouched: a changed
file still maps through the internal import graph, rooted at `gateBuildEntry`,
following imports and never directories, with `inert-change.ts` dropping revision
pairs that differ only in comments. What changes is what a step then selects —
its own cells rather than the scenarios whose declared `steps` name it. That
removes the last place a hand-maintained declaration decided what the gate
measures: a cell's step is the directory it lives in.

`*.bench.ts` needed a third classification. It was `isNonShippingSourcePath`, so
it selected nothing, and that is now wrong in a specific way: a bench file cannot
change either build, but it declares the measurement. It selects its own step's
cells — neither a full run nor nothing — and `check-impact-triggers.ts` gained
the matching rule, because a rewritten cell that never starts the workflow is a
measurement change nothing looks at until some later diff inherits it. All 114
bench files start both events.

The canary is now cells: the construction and cold groups whole, plus thirteen
named core cells — the per-call floor, the string pipeline, the object walk,
issue construction with and without a path, both halves of the deferred message
chain, the collect-all traversal, and the asynchronous path. It stays a list here
rather than a flag on each cell, because a `canary: true` would be each step
author's claim about the core and a core path nobody flagged would leave the
canary silently. The test drives the real catalog and holds every group
triggerable.

`benchmarks/src/cells/catalog.mjs` exists because collecting cells needs Node's
own resolution hooks: inside a vitest worker `vitest` resolves to the real runner
and a bench file would register a suite from inside a test, so a caller under
another loader spawns this instead of importing `collect.mjs`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate measures each step's own bench cells instead of the cross-library
scenarios, and it shards, which the comment said it deliberately did not.

Both of that comment's objections were checked. The severe-group trigger becoming
a cross-runner aggregate is true as a description and breaks nothing: every input
to it is already a dimensionless machine-cancelled paired ratio, and
`summarizeGroups` computes a bare geometric mean with no confidence interval, so
there is no statistic for a between-machine term to enter. The claim that the
fixed cost does not shard was measured rather than argued: 55m12s of measurement
against about 40s of checkout, setup, both builds, and scoping, so wall time is
40s + measurement/N and four shards is about 28.5 minutes. The comment now carries
those numbers, because leaving a disproved rationale in place misleads the next
reader.

The merge happens before the verdict, so every group aggregate is computed once
over the complete cell set. `merge` already refuses an incomplete shard set, a
shape no positional round-robin could produce, and shards built from different
commits, so a missing shard fails rather than publishing a quarter of a run as a
whole one. Each shard resolves the same parameters and scope independently, which
is safe because the assignment is deterministic from the selection and count
alone.

Sharding is also the only answer that scales: the cell count grows with every step
added, while a timeout is a constant that gets consumed.

The competitor-adapter and fail-on-regression dispatch inputs are gone. A cell run
measures one library against itself by construction, the cross-library ranking is
Performance Comparison's job, and a gate that can be asked not to fail is not one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 4's remaining slices, landed before the audit was stopped: default
messages, single-`length`/`size` reads, format-validator grammars, and the
Standard Schema interop path each gain the assertion their suite described but
never made. What was missing in each case is not a registered case but a
mutation to the code under test leaving the repository green.

No implementation changes. The audit's ranked remainder is recorded on #134.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The benchmarking reference described scenarios as what the gate selects and
measures, the focused benchmark as a local-only tool, the gate as unsharded, and
the classifier as a precision test on a point estimate. All four are now false.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ll described

The scoping paragraph still mapped steps to scenarios, the canary was still
eleven scenario ids, and the decision rubric still described a precision test on
a point estimate. All three describe what the gate did before this phase.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`eef3199` added a test that casts an `ExecutionResult` union straight to the
issue shape it wants to reach, which TypeScript refuses: the union's `issues`
tuple may hold more than one element and none of its members is comparable to
the single-element target, so `tsc --project tsconfig.tests.json` failed and
`pnpm verify` could not reach exit 0. Widening through `unknown` is the same
escape the rest of the suite uses, and the assertion — that the reported
`protocols` array is frozen — is unchanged.

This is a phase 4 file. Phase 4 is stopped and its remainder is issue #135, so
this is fixed here rather than left as the one thing blocking the epic's gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DevilTea and others added 13 commits July 30, 2026 18:41
…heckout's line endings

The composition is canonically LF: every source is split on `/\r?\n/` and joined
with `\n`. The committed pages were compared as the bytes git left in the working
tree, which is CRLF on a Windows checkout, so `test (22, windows-latest)` and
`test (24, windows-latest)` reported all seven generated files stale on a tree
nobody had edited while ubuntu and macOS passed.

The comparison moves out of the CLI into `docs-api.ts` as `staleOutputs()`, where
a test over a synthetic tree can reach it — the existing CRLF test covered the
composition, which was already line-ending agnostic, and could not reach the one
rule that reads the committed pages. Two cases drive it: a committed page
differing only in line endings is accepted, and a content difference or a missing
page is still named.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four measurement shards of the first sharded run completed and merged 245
cells, and then `compare` failed with `VALCHECKER_DIST_URL is not set`: building
the coverage denominators re-entered the loader that resolves a bench file's
`'../..'` to a dist. Setting the variable in that job would have kept a reporting
stage executing the code it reports on.

The catalog becomes data. `cells --catalog-output <path>` writes it while
measuring, `compare --cell-catalog <path>` reads it, `catalog-artifact.mjs`
imports nothing that registers a hook, and the compare job now runs with
`build: 'false'` so the coupling cannot return unnoticed. Verified end to end
locally: six smoke runs measured, then compare produced its verdict with
`VALCHECKER_DIST_URL` unset.

Catalog identity is checked wherever a cell set could differ: each run records the
hash it measured against, `merge` refuses shards that disagree,
`measurementIdentity` carries it, and `compare` refuses a catalog file that is not
the one the runs were measured from. A cell run's `scenarioCatalog` now lists the
cells its shard was assigned rather than the ones it managed to measure, which is
what the cross-library runner already recorded and what keeps `p % count`
invertible when a build cannot execute a cell.

`measured N / added M / removed K` prints unconditionally, added and removed cells
named. That also removes a hard failure: an asymmetric cell set used to abort with
`Candidate contains scenarios absent from baseline`, so a pull request adding a
step could not be measured at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The confidence interval was centred on the mean of the paired ratios while the
reported and severe-triggering point estimate was their median. Two estimands, one
decision, and the deciding one was not the one on display.

Both are now `exp` of the mean of the per-repetition log ratios
`d_r = ln(candidate_r / baseline_r)`: the reported change is `exp(mean(d)) - 1` and
the interval is `exp(mean(d) ± t·sd(d)/√n) - 1`. Improvement and regression become
multiplicatively symmetric — a doubling and a halving are `±ln 2` rather than `+1.0`
and `-0.5` — which is what lets a group aggregate be a mean of the same numbers.
Student's t stays and no threshold moved.

`statistics.mjs` gains `confidenceHalfWidth` as the primitive, with
`relativeMarginOfError` defined from it, and `pairedLogRatioEstimate` as the
estimator; a relative half-width is the wrong shape in log space, where the mean of
a null comparison is near zero. `pairedRme` is now the log half-width in percent,
which agrees with the old figure to first order: the null run's `construct/tuple`
cell still reads 3.12%.

Every required case still holds, and the fixture is now symmetric in logs so each
is exact: -5.32% at 3.12% RME is `inconclusive`, and +14.88%/19.47,
-7.71%/15.72, -6.07%/6.15, -5.64%/11.09 are all `inconclusive`. Two new cases
drive the estimator itself — the interval's bounds are reciprocal multiples of the
point estimate, and a ratio and its reciprocal produce exact negatives.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ecisive rows

The group aggregate was a geometric mean over the rows whose own measurement
happened to be decisive, which conditions the estimate on the measurement outcome:
a row survives that filter when its effect is large or its noise is low, so the
mean ran over exactly the rows most likely to trigger it.

The group is now estimated from the same numbers a row is. Per repetition its
cells' log ratios are averaged into `G_r`, and the verdict is the Student-t
interval across `G_1 … G_5`, converted back with `exp`. Every cell selected into
the group contributes to every repetition whatever its own row said.
`exp(mean(G))` is still the geometric mean of every cell's ratio, so the aggregate
did not change shape — it gained an interval, and averaging within a repetition
before taking the spread across repetitions is what earns it.

The trigger's prerequisite becomes two *measured* rows rather than two decisive
ones: a property of the selection, decided before anything runs. `decisiveScenarios`
stays as a diagnostic. `minimumDecisiveScenariosPerGroup` is renamed
`minimumScenariosPerGroup`, group rows carry their own interval and verdict, and
`impact.json` is schema 9.

Three cases drive the change, replacing the two that encoded the old filter: an
inconclusive row's cell stays in its group's estimate and widens the interval, a
group whose interval spans −5% is not severe however far past it the point estimate
is, and a group of five cells each too noisy to decide alone is severe when their
noise falls on different repetitions — the case the old rule could not reach at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rder

The alternation by repetition parity survived the rewrite into four shard jobs — it
is in the measurement loop, identical in every shard, with both sides of a
repetition still measured back to back. Nothing tested that.

Pairing cancels machine speed, not monotonic drift over a 24-minute job, so a loop
that always measured the baseline first would put every repetition's drift on the
candidate. The rule goes in the check that already reads this workflow, and it
fails on the three ways the property can be lost: no parity branch, a parity
branch with no `else`, and two branches measuring the sides in the same order.
Verified by removing the alternation and observing the failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…econd batch

The recorded plan was to re-run every inconclusive cell for k more paired
repetitions, pool them with the first pass, and judge once. That is optional
stopping however carefully the rule is pre-declared: the set being extended is
chosen by the first result, so the second judgement rests on a sample that exists
because the first was unsettled.

Two fixed batches instead. The screen is five paired repetitions over every
selected cell; `confirm-measure` then measures a second five over the rows that
could block — every candidate regression, and every inconclusive row whose interval
reaches −5% — with its own seed, its own comparison, and no knowledge of the first.
`resolveConfirmation` combines two verdicts rather than lengthening one sample:
severe + severe or regression fails, severe + inconclusive is `unresolved` and not
a pass, regression + cleared passes with the screen's noise named, and a severe row
with no second batch is `unconfirmed` and still blocks. Nine cases drive it,
including the three the review's table names.

`compare` no longer carries the exit code; the new `verdict` job does, and it runs
even when the confirmation batch failed so a missing confirmation cannot read as a
clearing one. A group verdict is deliberately not confirmed: the confirmation set
is chosen by the screen's outcome, so a group aggregate over it would carry exactly
the conditioning `groupEstimate` removes, and confirming a group means re-measuring
all 124 cells of it. That limit is stated in the report, the README, and the module.

The counterbalance rule now checks every repetition loop rather than the first, so
the confirmation batch is held to it too. Verified by breaking the second loop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndings

`test (22, windows-latest)` and `test (24, windows-latest)` failed on the
counterbalance rule added in ee1ee1d: it matches a shell block ending in `done\n`,
which a CRLF checkout never contains, so the check reported that the workflow has
no measurement loop at all. Every other platform passed — the same shape as the
generated-reference failure fixed in cde7147, and the second time this file has
been caught by it, since `readEventPaths` once matched `on:` positionally and
failed the same way.

Normalized once at the read, where the difference enters, rather than in either
rule. Reproduced locally by converting the workflow to CRLF: the check fails
without the change and passes with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…'s flavour

`seq -s, 0 2` is `0,1,2,` under BSD and `0,1,2` under GNU coreutils, so the shard
list was valid JSON only on the runner that happens to ship the second one — and
`fromJSON` rejects the first, which would have failed the confirmation stage for
any count above one. Joined with `paste -sd, -` instead, which terminates nothing.

Found by running the compare job's shell locally: 3 cells produced `[0,1,2,]`.
Verified for counts 1 through 4 by parsing each result as JSON.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`benchmarks/README.md` still said the impact gate is deliberately not sharded,
directly above the two-stage description added in this branch and below a workflow
that has sharded four ways since the cell rewrite. Its two objections are answered
with what was measured rather than dropped: the group trigger does become a
cross-runner aggregate and nothing breaks, because a shard's machine is a constant
per cell rather than a per-repetition random effect and cannot inflate the spread
the group's interval is built from; and the fixed cost was 40s against 55m12s of
measurement, so four shards is about 28.5 minutes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…constants

The confirmation report printed `5 screened + 5 confirming` from constants in the
module while the workflow is what sets the repetition count, so a rehearsal at
three repetitions reported five and a dispatched run asking for seven would too.
Both numbers now come from the two comparisons' own `runCounts`, and a stage that
did not run reads as "no confirming" rather than as a count. The constants are
deleted rather than left unread: a number no code consults is a second copy waiting
to disagree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…locked

The gate's first real CI run judged `map/collect-all` severe in both batches
(-14.67%, then -11.59%) and `set/collect-all` inconclusive in the screen
(-42.9% … +26.1%) and severe at -30.20% in the confirmation batch. The first pair
resolved as `reproduced` and the second as `reproduced` too, while the same pair in
the other order — severe then inconclusive — resolved as `unresolved`. One severe
judgement against one non-judgement is the same evidence whichever stage produced
which, so the rule now reads both classifications: two regression claims are
`reproduced`, a claim against a non-judgement is `unresolved` in either direction.
Only a severe claim fails the build, as before; a reproduced plain regression is a
review. Two cases drive it, one of them the run's own numbers in both orders.

The failure was also illegible: the job's log ended in a one-line summary and
`ELIFECYCLE`, so the reason lived only in an artifact and the run was first read as
a wiring fault. The rows that decided are now printed where the failure is, with
both stages' classifications and deltas, and a severe group carried through from
the screen says that it was not confirmed here.

Replaying the run's own artifacts under the corrected rule reaches the same verdict
and exit code, with `set/collect-all` reported as unresolved rather than reproduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rot checks

The owner accepted the `firstIndex` correction's cost on the collect-all path. It is
declared rather than suppressed: `accepted-regressions.mjs` holds an entry per cell
with the depth accepted and prose saying what the cost bought, following the shape
`check-benchmark-coverage.ts` uses for the steps no competitor can express.

Bounds come from measurement, not from taste. Two hosted comparisons measured
`map/collect-all` at -14.67% and -7.67% (confirmations -11.59%, -6.70%) and
`set/collect-all` at -15.13% and -32.37% (confirmations -30.20%, -29.83%), so the
bounds are 25% and 45% — above the deepest observation, because a bound tighter
than the gate's own run-to-run spread fails on noise instead of on a change. A
later -60% on either cell still fails, with both numbers in the message.

Rot is checked in both directions, each where it can be decided. An entry whose
cell the screen now reports `cleared` fails the verdict, so the list shrinks as the
code improves; an entry naming a cell the catalog no longer declares fails in
`pnpm bench:cells`, which needs no measurement. Staleness reads the screen and not
the confirmation batch, whose clearing of a cell the screen called severe is this
gate's noise diagnostic rather than evidence a cost is gone. An acknowledged row is
printed with its measured depth, its bound, and its reason, never absent.

Two limits are documented rather than left to be discovered: an acknowledgement
never reaches a group verdict, and no rule here can tell an accepted cost from a
regression someone got tired of.

Replaying both real runs: the first resolves to `review` and exit 0 with both cells
acknowledged; the second acknowledges `set/collect-all` at -32.37% and stays red on
the `warm/failure/all` group trigger, which this deliberately does not cover.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, with a bound

`warm/failure/all` measured -6.40% with an interval of [-7.1%, -5.7%], and that
number is true: the group holds one collect-all cell per structure and two of its
nine carry a cost this repository accepted. `set/collect-all` accounts for -4.25pp
of it, `map/collect-all` for -0.88pp, the two together for -5.10pp, the remaining
seven cells for -1.37pp — decomposed from the run's own rows rather than asserted.

So the group is forgiven by an entry naming the group, bounded at 12%. Twelve rather
than seven because the group sits astride the -5% trigger with seven of nine rows
individually inconclusive — the previous comparison put it at -3.93% and called it
inconclusive — while still failing if the group effect roughly doubles.

Excluding acknowledged cells from the aggregate was rejected, and the reason is in
the module: it would condition the estimate on which cells someone previously
forgave, which is the bias `groupEstimate` was rebuilt to remove, and it would
shrink the denominator so a new regression in that group would be diluted rather
than caught. The reported group number stays the true one over every cell; a bound
says how much of it a person agreed to.

Rot checks, each where it can be decided: a group the screen now reports `cleared`
fails the verdict, a group no cell aggregates into fails `pnpm bench:cells`,
malformed entries fail, two unjudgeable batches leave the entry untouched — and the
one a cell entry does not need, a group entry with no acknowledged member cell left,
so it cannot outlive the per-cell reasons it rests on. Verified by emptying the cell
list and watching the group entry fail.

Replaying the run that was red: `review`, exit 0, `set/collect-all` acknowledged at
-32.37% against 45% and `warm/failure/all` at -6.40% against 12%, both printed with
their true measured values.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@DevilTea DevilTea left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT 5.6 sol — review of 4321be4

The hardening pass addressed the major issues I raised on #134: the row estimator is coherent, group estimation no longer conditions on decisive rows, confirmation is independent rather than pooled, compare is data-only, counterbalancing is preserved, and the full workflow now runs green end to end.

I still would not merge this HEAD yet. I see two remaining properties where the Performance Impact gate can make a stronger claim than the evidence supports; both are called out inline.

  1. Catalog deletion/rename is still invisible to the advertised added / removed audit. Candidate-owned definitions are the right measurement apparatus, but they cannot simultaneously tell us what the candidate deleted from the apparatus. That needs a separate static base-vs-head catalog diff.
  2. A blocking group CI currently ignores cross-runner heterogeneity. The latest green run itself mixed EPYC 7763 and EPYC 9V74 shards. Pairing cancels machine scale for a cell, but a CPU-dependent candidate/base ratio becomes a fixed shard effect; the CI across repetitions cannot see it. A triggered group therefore needs whole-group confirmation on one runner, or it should remain review-only on hosted sharded runners.

Non-blocking cleanup before calling #134 complete: please also sweep the remaining comments/docs that still describe the pre-hardening semantics (for example the workflow header's old decisive/stable-row group explanation, step-bench.ts saying the group trigger needs decisive cells, and the old pooled-retry comment in impact-verdict.mjs). Benchmark methodology comments are part of the maintenance contract here, and stale rules are particularly risky after a redesign this large.

I am submitting this as COMMENT rather than REQUEST_CHANGES because the connected GitHub identity is the PR author; semantically, I consider the two inline P1 findings merge-blocking.

Comment thread benchmarks/src/impact-verdict.mjs Outdated
Comment thread benchmarks/README.md Outdated
DevilTea and others added 2 commits July 31, 2026 00:26
…oup on one runner

Two merge-blocking findings from the review of 4321be4, both correct.

**`removed` advertised an audit it could not perform.** The apparatus comes from the
candidate ref, so a deleted cell is never collected, never sharded, and can never
appear in a baseline result: a pull request deleting a cell reported `removed 0`,
exactly the coverage loss the line existed to surface. Catalog addition and removal
now come from `pnpm bench:catalog-diff`, which reads both revisions with
`git show` and parses their `stepBench()` declarations with the TypeScript AST —
`scripts/bench-catalog-ids.test.ts` asserts over its own import graph that it
reaches neither the package entry nor the cell collector, because "executes no
build" is the property the approach rests on. The runtime difference stays under
the names it always measured, `candidate-only` and `baseline-only`. A revision whose
declarations cannot be read statically is an incomplete diff, not an empty catalog;
with no diff at all the report prints `n/a` rather than `0`.

**A cross-shard group interval has no between-runner component.** My earlier
argument was wrong and is corrected in place. Pairing cancels machine speed per
cell; it does not make that cell's candidate/base ratio invariant across
microarchitectures. Since a cell keeps its shard for all five repetitions, such an
effect is a *fixed* effect across repetitions — it shifts every `G_r` equally and
contributes zero variance — so the interval cannot widen for it and can be tight and
displaced at once. Run 30547023911 drew an EPYC 9V74 for shard 2 and 7763s for the
rest, so it is not hypothetical, and a group verdict blocks while a cell's is
independently confirmed.

Sharding stays; the decision boundary moved. A triggered group is remeasured in full
on one runner and blocks only if that batch agrees. The arithmetic: 613 cell-runs per
shard in 1429 s is 2.33 s each, so N cells cost 23.3·N seconds plus ~2 min overhead,
against 45 of the job's 60 minutes — about 110 cells. `warm/failure/all` (9 cells,
3.5 min) and `warm/async/success` (12) are confirmed; `warm/success` (124 cells,
48.2 min) falls back to `review`, and the report says so. Whether a group was
confirmed is read from the artifacts, so a sharded or partial confirmation cannot
support blocking whatever the workflow intended.

Also swept the comments the redesign left stale: the workflow header's group
argument, `step-bench.ts` on decisive cells, `impact-verdict.mjs` on the pooled
retry, and the README sentence the review quoted. The renderer no longer returns
early when there are no cell rows, which had been hiding the group section.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nt as to block on one

The rot check reported `warm/failure/all` stale at -3.44% `cleared` one run after
the same instrument reported it -6.40% `regression` with an interval of
[-7.1%, -5.7%], and a third run had it -3.93% `inconclusive`. Under the interval
rule `cleared` and `regression` cannot both describe one quantity, so the between-run
variation exceeds the within-run interval — the between-runner fixed effect the
review identified, now demonstrated by the acknowledgement machinery itself rather
than argued. That evidence is recorded in the module.

The rule follows symmetrically: a cross-shard screen that cannot support blocking a
group cannot support un-acknowledging one either. Otherwise the entry can neither
exist nor not exist without failing on some runs, which is a gate no author can
satisfy — `e4ed510` is exactly that. A group entry is now stale only when a
single-runner confirmation reports it cleared, and its bound is judged from that same
confirmation; nothing about an acknowledged group is decided from the screen.

The same argument applies to cells, checked rather than assumed: a cell keeps its
shard for every repetition too, so its interval cannot see a runner-dependent shift
in its own ratio, and a cleared screen is one reading. The risk is smaller — a -11%
cell needs a far larger displacement to read `cleared` than a group whose interval is
[-7.1%, -5.7%] — but the fix costs two cells, so the asymmetry had no reason to
stay: a cell entry is stale only when the screen and the confirmation agree.

Acknowledged cells and acknowledged groups are therefore always queued for the
confirmation batch, which is what keeps their entries falsifiable at all, and a check
with no evidence behind it is reported as `unassessed` — never a pass, never a
failure. `warm/failure/all` is 9 cells, 5.5 min of the 45-minute single-runner
budget, so its acknowledgement stays assessable; a 124-cell group would not fit, and
for such a group `review` is the honest instrument rather than a bounded
acknowledgement, which the report and the docs now say.

The catalog diff was already wired — the screen comparison logged
`catalog added 245 / removed 0` on e4ed510 — but the `n/a` in that log came from the
confirmation comparison, which audits nothing. It no longer prints a catalog line at
all, and the screen comparison passes `--require-catalog-diff` so an absent diff
fails loudly instead of printing an `n/a` a reader has to notice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DevilTea

Copy link
Copy Markdown
Owner Author

Both P1 findings are addressed on 0faf780. Every check on this pull request passes, including the full impact chain (run 30564818047).

P1 — catalog deletion is now visible, from a static diff

scripts/bench-catalog-diff.ts enumerates each revision with git ls-tree, reads each file with
git show <ref>:<path>, and parses the stepBench() call with the TypeScript AST — the same
read-a-revision-as-text idiom inert-change.ts already uses. Three things establish that it executes
no build: a test asserts its import graph reaches only typescript and node:*, never
benchmarks/src/cells/ (which is what registers the loader hook resolving '../..' to a dist); it
runs against main, a revision whose dist this checkout cannot build; and it needs no
VALCHECKER_DIST_URL.

The screen comparison passes --require-catalog-diff and fails outright when the diff is absent, so
the wiring is itself checkable rather than something a reader has to notice. Runtime presence is now
named candidate-only / baseline-only and never conflated with catalog membership. The confirmation
comparison measures a subset and audits nothing, so it prints no catalog line at all rather than an
n/a that reads as "the audit did not happen".

On this pull request the diff reports added 245 / removed 0 alongside 114 unreadable-file
problems
, because the base is main where no bench file uses stepBench() yet — an incomplete diff
reported as incomplete rather than as "everything was added".

P1 — a triggered group is confirmed on one runner, and your reasoning turned out to be measurable

Accepted, with the correction stated: the coordinator had earlier argued a cross-shard aggregate was
safe because each cell's paired ratio is machine-cancelled, and framed the residual as
heteroscedasticity. That was wrong for the reason you gave — a cell keeps its shard for all five
repetitions, so a runner-dependent effect on its ratio is a fixed effect across r, shifting every
G_r equally and contributing zero variance. The interval cannot widen for it.

Your finding then proved itself empirically, through the acknowledgement machinery rather than by
argument. Across three runs the same group reported −3.93% inconclusive, −6.40% regression with
interval [−7.1%, −5.7%]
, and −3.44% cleared. Under the interval scheme cleared and regression
cannot both describe one quantity, so the between-run variation exceeds the within-run interval. Those
three readings are recorded in the module.

A triggered group is now remeasured in full on one runner and blocks only if that batch also
classifies it a regression, read from the artifacts — a confirmation that ran sharded, or that did not
cover every cell of the group, cannot support blocking whatever the workflow intended. The budget is
45 of the job's 60 minutes at a measured 2.33 s per cell-run, so:

group cells single-runner cost treatment
warm/failure/all 9 5.5 min confirmed, may block
warm/async/success 12 4.7 min confirmed
warm/failure/library-default 95 36.9 min confirmed
warm/success 124 50.2 min review, not blocking, with the arithmetic printed

So your fallback applies exactly where the arithmetic forces it, and nowhere else.

The same argument made the rot rule symmetric

Your finding has a consequence for the accepted-regression mechanism that only surfaced when the rot
check fired: blocking required single-runner confirmation while staleness accepted a cross-shard
screen, so an acknowledged group could neither exist nor be removed without failing on some runs. If a
cross-shard screen cannot support blocking a group, it cannot support un-acknowledging one either.

Staleness now requires the same evidence as blocking — a group retires only on a single-runner
confirmation, a cell only when the screen and the confirmation agree — and where the evidence is
missing the check reports unassessed by name, which is neither a pass nor a failure. Acknowledged
cells and groups are always queued into the confirmation batch, because otherwise no entry could ever
produce the evidence needed to retire it and the rot check would be decoration.

The same reasoning applies to cells, which we had expected it would not: a cell also keeps its shard
across repetitions, so a cleared screen is a single reading. The risk is much smaller, but the fix
costs two cells in the batch, so it is symmetric rather than justified as a weaker rule.

Where a bounded acknowledgement cannot be falsified — a group too large to confirm on one runner —
review is the honest treatment, and that is now stated in the module, the README, the skill
reference, and the changelog rather than left to be discovered.

Non-blocking sweep

Corrected: the workflow header's group argument (which asserted the now-known-false "no statistic for a
between-machine term to enter" — the coordinator's own mistaken reasoning, committed), step-bench.ts
on "two decisive cells", impact-verdict.mjs's pooled-retry comment, and the README sentence you
quoted. Each says what the mechanism now does and why the old claim was wrong. A test written for this
also caught a live reporting bug: the renderer returned early with no cell rows and hid the group
section entirely.

What the gate reported on this run

verdict regression over 60 of 245 standard scenarios
cells measured 60, candidate-only 0 / baseline-only 0
[confirm] verdict review (screen regression): 0 reproduced, 14 not reproduced, 0 unresolved
[confirm] accepted regression: map/collect-all — measured -13.53%, accepted to -25%
[confirm] accepted regression: set/collect-all — measured -30.73%, accepted to -45%
[confirm] rot check not assessed: warm/failure/all — the single-runner confirmation reports it
          inconclusive, which neither confirms the accepted cost nor shows it gone

Fourteen screen regressions, none reproduced, so a screen regression resolved to review on
independent evidence rather than blocking. pnpm verify exits 0; 149 harness tests and 11
static-catalog tests pass.

@DevilTea DevilTea left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT 5.6 sol — second review of 0faf780

I re-reviewed the current HEAD rather than carrying the previous findings forward. The two earlier P1s are substantively addressed: the runtime presence counts are now separated from a static catalog instrument, and a blocking group now requires a whole-group single-runner confirmation. The latest Performance Impact run also exercises that new path end to end.

I found three follow-on issues in the hardening implementation, all called out inline. I would fix them before merging because each changes whether the gate is allowed to make or withhold a blocking claim:

  1. --require-catalog-diff currently requires the artifact to exist but does not require the audit to be complete. The latest green run proves this path can stay green with problems populated.
  2. Group confirmability is computed from the union of group cells and unrelated row/acknowledgement confirmations, so unrelated work can make an individually confirmable severe group fail open to review.
  3. Accepted-regression bounds are still judged from the deepest point estimate seen in either stage, bypassing the interval semantics and independent-reproduction rule the rest of the gate now uses.

One non-blocking follow-up after these: the static catalog audit currently diffs IDs only. group, batch, and async are measurement-contract fields too (the runtime catalog hash already treats them that way), so reporting metadata changes would make apparatus edits much easier to audit. I would not hold this PR solely for that if the three inline findings are fixed.

Because the connector is acting as the PR author, this is submitted as COMMENT; semantically the three inline findings are merge-blocking for me.

Comment thread scripts/bench-catalog-diff.ts
Comment thread benchmarks/src/confirmation.mjs Outdated
Comment thread benchmarks/src/accepted-regressions.mjs Outdated
DevilTea and others added 2 commits July 31, 2026 08:04
…owledgement gate

**An incomplete catalog audit no longer passes.** `bench:catalog-diff` printed its
problems and exited 0, so `--require-catalog-diff` checked only that a file existed:
the green run carried `baseCells: 0` and a problem per legacy file while the workflow
stayed green. The reviewer found the concrete escape — `check-bench-cells.ts` did not
require a literal cell `name`, and `stepBench()` accepts a computed one, so a pull
request could pass the quality gate, make the static reader unable to parse the head,
and leave the required check green. Literal names are now enforced; a head problem is
always fatal; a base problem is fatal too, except for one explicit self-retiring
migration case — the all-legacy baseline, which cannot fire once the base declares any
cell, asserted as a test rather than left as a comment. `compare` also refuses an
artifact carrying a fatal problem.

**A group's confirmability no longer depends on unrelated work.** One budget over the
union of boundary rows, every group's members and the rot-check workload was
all-or-nothing, so a nine-cell group lost its blocking confirmation when unrelated
rows pushed the union past the limit, and two groups that each fitted alone but not
together left neither able to block. Each group now gets its own single-runner batch
sized by its own cells; rows keep their sharded batch and acknowledged cells ride with
them. Driven by a test with 130 unrelated rows beside a two-cell group.

**A bound is judged like every other decision threshold.** This was the reviewer's
own correction of their earlier phrasing, and they were right: `deepestRegressionPercent`
reintroduced both failure modes the gate was hardened against. With a 45% bound a noisy
screen at -60% and a confirmation `cleared` at 0% was a breach that failed the
workflow — so adding an acknowledgement made the gate stricter than having none — while
a -40% point estimate whose interval ran past -45% was accepted. Each stage's interval
is now classified against the bound, and a breach blocks only when both batches
reproduce it.

The bounds were rechecked and none moved: every interval observed sits inside its bound
(map [-18.2%, -10.9%] against 25%, the group [-7.1%, -5.7%] against 12%, set
[-42.9%, +26.1%] against 45% by 2.1pp). The consequence is recorded rather than
smoothed over: a noisier run now reports `set/collect-all` as `unassessed` instead of
acknowledged, which blocks nothing, and the bound is deliberately not widened to make
it read as acknowledged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e planner

`compare` died with `TypeError: Cannot read properties of undefined (reading
'totalSeconds')` and took `confirm-measure` and `verdict` with it. When the planner
moved to independent per-group batches, the `--plan` branch of the CLI was not
updated: it still read `plan.budget` and `plan.shardCount`, both replaced by
per-batch and per-group figures. A second gap sat in the same branch — it emitted
`{ cells, shardCount, groups }` while the workflow had moved to reading
`plan.batches`, so even past the crash the next step would have read an undefined
`batches`.

The tests did not catch it because they call `confirmationPlan` directly and the CLI
was assumed to be a thin shell. It was not thin: formatting a plan is logic about the
plan. `planSummaryLines()` moves into `confirmation.mjs` beside the planner, the CLI
prints what it returns and emits the whole plan as JSON, and three cases cover the
shapes nobody had exercised — a plan with rows and a group, a plan with rows and no
group, and an empty plan — plus a group whose budget entry is missing, which cannot
arise today and must describe itself rather than throw.

Rehearsed against the real screen artifact from the failing run: 65 row cells over
four shards and `warm/failure/all` at 9 cells on one runner, 5.5 min of the 45 min
budget.

The verdict job's batch loop also dropped `mapfile`, which needs bash 4 and so cannot
be run on macOS's bash 3.2 before pushing — the same mistake as depending on GNU
`seq -s,`. It reads a file instead, and the loop was rehearsed locally under bash
3.2 against that same plan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DevilTea

Copy link
Copy Markdown
Owner Author

All three findings are addressed on e74e196. Every check passes, including the full impact chain (run 30594089941).

P1-a — completeness is in the contract, and the escape route is closed

check-bench-cells.ts now requires a literal cell name, which closes exactly the hole you found: a computed name passed the quality gate, made the static reader unable to parse the head, and left the required check green. bench:catalog-diff exits non-zero on a fatal problem, a head problem is always fatal, and a base problem is fatal too — with one explicit, named migration case for the all-legacy base. compare --require-catalog-diff refuses an artifact carrying a fatal problem rather than merely checking the file exists.

The follow-up condition is a test rather than a comment: isLegacyBaselineOnly cannot fire once baseCells > 0, and stops tolerating the legacy base the moment the base declares any cell pins that. So the toleration expires by construction when this pull request merges; nobody has to remember.

CI evidence: the compare line now reads cells measured 245, candidate-only 0 / baseline-only 0, catalog added 245 / removed 0 — the static audit and the runtime counts side by side and never conflated.

P1-b — group confirmability no longer depends on unrelated work

Each confirmed group gets its own single-runner batch sized by its own cells. Rows keep the sharded batch, and acknowledged cells ride with the rows, since they need no single runner and must not consume a group's budget. unrelated row confirmations cannot cost a group its confirmability puts 130 unrelated rows beside a two-cell group: the rows shard four ways, the group keeps its own batch, and nothing is downgraded.

Visible in this run as two separate comparisons — 69 cells for the rows and 9 for warm/failure/all on its own runner.

P1-c — the bound uses the gate's own semantics

Each stage's interval is classified against -maxRegressionPercent: wholly worse is a breach, wholly inside is within bound, spanning it is unassessed. A breach requires both batches, so your −60% screen followed by a cleared confirmation is unassessed rather than a red gate — and an acknowledgement can no longer make the gate stricter than having none.

The bounds did not move, and the arithmetic is on the record: map's widest interval [−18.2%, −10.9%] against 25%, the group's [−7.1%, −5.7%] against 12%, and set's widest [−42.9%, +26.1%] against 45% — inside by 2.1pp. All classify within under the stricter reading. The consequence is recorded rather than smoothed: on a noisier run set/collect-all will report unassessed instead of acknowledged. That blocks nothing, and widening the bound to make it read as acknowledged would be widening an acknowledgement to suit a measurement.

Two process failures worth stating, since they bear on how much the local evidence is worth

Fixing P1-b broke compare with a TypeError — the plan's shape changed and the CLI branch that formats it was not updated. The tests missed it because they call the planner directly on the assumption that the CLI is a thin I/O shell; it was not, since formatting a plan is logic about the plan. planSummaryLines() now lives beside the planner, so a shape change fails a test instead of a job.

Two smaller ones, both the same class: a mapfile in the verdict job that the local bash 3.2 cannot run, so that snippet was unverifiable locally rather than merely unverified — now replaced and exercised under 3.2 against the real plan; and, earlier, a GNU seq dependency. The honest summary is that a local check already built was twice not re-run after a later change. Where the local equivalent could not exist at all, it has been replaced by one that can.

Non-blocking metadata audit

Not done, and recorded on #134 with the reasoning rather than squeezed in unverified: the static reader would need per-field diffing and the report a vocabulary for a metadata change as distinct from an addition or removal. It is worth pairing with the base-problems rule above, which only becomes reliable once this pull request merges.

What the gate reported

verdict regression over 69 of 245 standard scenarios     (rows, sharded)
verdict regression over  9 of 245 standard scenarios     (warm/failure/all, one runner)
[confirm] verdict review (screen regression): 0 reproduced, 36 not reproduced, 0 unresolved
[confirm] accepted regression: set/collect-all — measured -32.38%, accepted to -45%
[confirm] rot check not assessed: warm/failure/all — the single-runner confirmation reports it
          inconclusive, which neither confirms the accepted cost nor shows it gone

Thirty-six screen regressions, none reproduced, so a screen regression resolved to review on independent evidence. 155 harness tests and 14 static-catalog tests pass; pnpm verify exits 0.

@DevilTea DevilTea left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT 5.6 sol — re-review of e74e196

The previous three findings were materially addressed: the static catalog audit now fails on unreadable head/current catalogs with an explicit one-time legacy-base exception; group confirmation is scheduled independently per group on one runner; and accepted-regression bounds are now interval-based with a reproduced-breach rule. The latest PR run (30594089941) also exercises the new fan-out end to end: four screen shards, 69 row-confirmation cells over four shards, a separate 9-cell warm/failure/all group batch, and a green final verdict.

I still would not merge this HEAD yet. I found two remaining P1 properties and one P2 workflow bug, called out inline:

  1. An unassessed acceptance can still fall through to ordinary severe/group blocking. The bound logic itself is now conservative, but the final verdict only exempts accepted.acknowledged; when the bound spans in one batch, the row/group becomes unacknowledged and the normal reproduced-severe path can fail even though no reproduced bound breach exists.
  2. The static contract diff audits cell identity but not group membership. Moving an existing cell between gate groups changes the load-bearing severe-group contract while still reporting zero catalog additions/removals; the runtime comparison cannot detect that history because the candidate apparatus supplies the group to both sides.
  3. The confirmation budget is still hard-coded to five repetitions. PR runs are five, but workflow_dispatch accepts a larger runs value and the actual confirmation jobs use it. The planner can therefore admit a group as a <45-minute single-runner batch that then exceeds the 60-minute job timeout.

The old inline threads being outdated is consistent with the fixes; I am not repeating those old findings. As before, this is submitted as COMMENT because the connected GitHub identity is the PR author; semantically the two P1 findings are merge-blocking, while the P2 should be fixed before treating manual dispatch as reliable.

Comment thread benchmarks/src/confirmation.mjs Outdated
Comment thread scripts/bench-catalog-ids.ts Outdated
Comment thread benchmarks/src/resolve-confirmation.mjs Outdated
…it group moves

**P1-1.** The exemption set held only `accepted.acknowledged`, so a bound the run
could not judge dropped the cell out of it and back to the ordinary rules. With a 45%
bound: a screen `severe` at [-60%, -20%] spans the bound, a confirmation `severe` at
[-35%, -25%] is wholly inside it, no breach is reproduced, the bound is `unassessed` —
and the cell then resolved as an ordinary reproduced severe row and turned the gate
red, although neither stage established the ceiling was breached. That is the P1-c
pattern again: an acknowledgement raises the threshold to the bound, so falling back
to 5% when the bound cannot be assessed is the stricter rule, and having the entry was
worse than not having it. Acceptance state is now modelled separately from ordinary
regression state — an entry exempts a row or group whatever the bound said, and only
the acceptance list can fail on it. The group path had the same interaction and the
same fix. Three final-verdict tests, including the reviewer's case at both levels and
the control that a reproduced breach still blocks.

**P1-2.** `group` needs a static instrument for the same structural reason a deletion
does: the apparatus comes from the candidate ref and supplies each cell's group to
*both* measured sides, so they agree on the new group by construction and no runtime
comparison can recover the history. `staticCatalog` retains `id -> group` including
`baseline`, the diff carries `changed: [{ id, baseGroup, headGroup, gateEffect }]`, and
the report prints `catalog added / removed / regrouped`. A `baseline` transition is
named as entering or leaving the gate so its accompanying addition or deletion reads as
a move rather than as a cell appearing from nowhere. `batch` and `async` stay deferred
on #134; only `group` is load-bearing for a blocking verdict.

**P2.** The planner prices a batch at the repetition count the screen used, read from
the comparison and refused when its two sides disagree. A default of five admitted a
job nobody scheduled: 100 cells is 40.8 min at five repetitions and 79.7 at ten, past
the timeout.

Re-ran every local check rather than only the new ones, which is what caught a `tsc`
error in a test fixture that the new tests alone passed straight through: 159 harness
tests, 239 script tests, `bench:cells`, both workflow gates, the catalog diff, the
`--plan` CLI against the real screen artifact, and the verdict job's loop under bash
3.2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DevilTea

Copy link
Copy Markdown
Owner Author

All three findings are addressed on 28a9cfc. Every check passes, including the full impact chain.

P1-1 — acceptance state is now independent of ordinary regression state

You were right that this was P1-c's pattern in a new place. An entry now exempts its cell or group from the ordinary rules whatever its bound turned out to say; only the acceptance list itself can fail on it — a reproduced breach, a stale entry, a malformed one. Rows resolve as acknowledged or acceptance-unassessed, and neither enters severeClaims.

Tests are your cases exactly: at cell level, a screen severe at [−60%, −20%] spanning a 45% bound with a confirm severe at [−35%, −25%] wholly within it resolves acceptance-unassessed and blocks nothing; at group level, a cross-shard screen spanning the bound with a single-runner confirmation within it stays out of unacknowledgedSevereGroups. A control asserts that a reproduced breach still blocks, so the exemption is not unconditional.

P1-2 — the static diff audits id → group

Accepted, and the escalation was right: this is the same structural argument that justified a static instrument at all, since the apparatus supplies each cell's group to both measured sides and they therefore agree on the new group by construction.

staticCatalog retains every cell's group including baseline. The diff carries changed: [{ id, baseGroup, headGroup, gateEffect }] and the report prints catalog added M / removed K / regrouped C with a named section. A baseline transition is labelled as having entered or left the gate, so an accompanying addition or deletion reads as a move rather than as two unrelated events. Tests cover a cell moved out of warm/failure/all under an unchanged id reporting added 0 / removed 0 / regrouped 1, and both baseline transitions.

batch and async remain deferred on #134, since only group is load-bearing for a blocking verdict.

P2 — the planner prices the batch the workflow will run

repetitions comes from screen.runCounts.baseline and is refused when the two sides disagree. The test asserts your figures: 100 cells fit at five repetitions and do not at ten, where the planner's own model gives 79.7 minutes.

An independent review of the surface none of these rounds has covered

Separately, the four runtime behaviour changes in this pull request — the only ones in 413 files — were reviewed independently, since all three of your passes have been on the gate rather than on the library. The reviewer could not fault any of the four, and the evidence is worth recording:

  • map/set materialization was probed exhaustively over 625 Set and 2,401 Map orderings of failure, duplicate and identity, including key-only, value-only and simultaneous failures and duplicates on both sides of a failure. NaN and signed-zero probes pass; native Map and Set canonicalize a stored -0 to +0 and the duplicate metadata retains the original index.
  • The five siblings a stopped audit had left unexamined — record, intersection, strictObject, looseObject, object — carry no sync/async divergence of the array/tuple class: each snapshots its key set or property count at construction.
  • isRecoverableFailure never had a production caller, was never exported from core/index.ts, and appears in no API surface.
  • Run against pre-fix source, the four regression cases fail and one passes; against this branch all five pass.

One process note

After the three fixes above, the new tests passed but pnpm verify exited 2 on a tsc error: StaticCatalog gained a field and a hand-built object in bench-catalog-ids.test.ts did not follow, which vitest runs but never typechecks. That is the third instance this session of a local check not being re-run after a later change, and the only reason it did not reach CI is a standing instruction to re-run everything rather than only what is new.

…ests on

Two gaps this pull request's own history exposed.

`pnpm verify` did not run `benchmarks`' harness suite. That suite decides how a
regression is classified, and it was reachable only from the Performance Impact
preflight job — so a change to the verdict logic could break its own tests and the
repository's complete gate would not notice. It is now the tenth quality gate, run
through `pnpm --dir benchmarks test` so it is the same command CI runs rather than a
second list that can drift. It needs no `benchmarks/node_modules`: the suite is
Node's own runner over modules whose graphs contain no bare specifier.

Nothing typechecked before a push. Three times while building this branch, a file
vitest executes but never typechecks carried a `tsc` error that only the full
`pnpm verify` found; a push here starts a benchmark comparison costing the better
part of an hour, so the cheap half of that gate belongs before it rather than after.
`pre-push` runs `pnpm verify:push` — typecheck plus the quality gates, nine seconds
measured.

What it cannot do is worth stating: of the failures that actually reached CI red on
this branch, a pre-push hook would have caught none. A Windows-only line-ending
difference, a job that needed an environment variable, and a crash in a shell branch
are all beyond it. This makes an instruction structural; it does not replace CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…found

An external pre-merge review went at the two gates nothing had reviewed —
`step-completeness.ts`, which all 114 step directories rest on, and `docs-api.ts`,
which generates the published reference — plus the catalog reader. Each finding is a
rule that could be satisfied without its requirement being met.

`step-completeness.ts` accepted a fake or shadowed registration, an unreachable
helper cycle, a locally redefined core utility, a prefix-only issue code, a malformed
plugin declaration, and documentation hidden inside a fence or an HTML comment. A
registration must now be the imported symbol at module scope, an issue code must
match as a complete token on both sides, and an entry's opening heading and its `ts`
example must both carry content.

`docs-api.ts` treated a marker inside a fence or a comment as a real slot, mishandled
CommonMark fence closers, and accepted a duplicated sidebar marker region — each a way
for a step to leave the site while the gate stayed green.

`bench-catalog-ids.ts` missed a duplicate id when either declaration was `baseline`,
which made group auditing depend on file order.

Twenty-seven adversarial regressions, each observed failing before its fix.

`AGENTS.md` follows the gates rather than describing what they used to do, including
one admission the strengthened rules add: a registration-shaped call may still sit
behind control flow the gate does not interpret. A gate that claims more than it
decides is the defect these findings all share.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

epic: every step owns its packaging, benchmark, docs, and tested contract

1 participant