test(2/7): Verify-style characterization snapshots - #28
Merged
Conversation
Reviews the consumer-facing surface of ts-autocode and its four sibling packages against the API-design rules already stated in CONTRIBUTING.md, and records the remediation plan. Covers eight defects (README code that does not compile, grounding codegen targeting a nonexistent training.define, an inaccurate sideEffects declaration, a silently ignored fanOut, documented-but-unexported symbols, a placeholder threshold reaching the judge, a fail-open evolve kill switch, and an unreachable execution timeout), two missing capabilities (model selection and a CLI), and the consistency, error-model, and boilerplate backlog behind them. The remediation is additive: renamed or reshaped APIs are added alongside the existing ones, which keep working and are marked deprecated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Each of these was reachable by a user following the documentation. - README snippets did not compile: `activation.promotion.snapshot.candidateId` does not exist on `Activation`, and the quickstart referenced an undefined `deploymentPolicy` and used its token before defining it. Every TypeScript block in all four READMEs is now self-contained and compiled by test/docs.test.ts. - The grounding package generated `training.define(...)`, which `Training` has no such method for, so every generated registration file failed to compile. Codegen now emits `defineGrounding` from the package that owns the concept, and the scan test typechecks the generated source instead of string-matching it. - `sideEffects: false` was wrong: importing the root package wires the engine, executor, loop and promotion applier at import time, so a tree-shaking bundler could legally drop that and leave a consumer with "no training engine is configured" after importing the package that configures it. - `TrainInput.fanOut` was documented but silently ignored by the default harness loop, whose judge/adversary/rubric sequence is serial. It now refuses a fan-out above 1 and names the loop that supports one. - The root package re-exported a hand-maintained subset of its siblings that had drifted, leaving README-documented `trainingRounds` and `sequentialLoop` unreachable along with `defaultPromotionGates`. test/surface.test.ts now enforces exhaustiveness. The harness's colliding `defaultMaxRounds` is renamed `defaultHarnessRounds`, keeping the old name as a deprecated alias. - The promotion rubric handed to the judge printed the literal string "evaluation default" instead of the threshold. The defaults are now exported as `defaultMinScore` and `defaultMinPassRate` and the rubric resolves them. - `TS_AUTOCODE_EVOLVE` failed open: only "0", "false" and "off" disabled source-rewriting evolution, so "no" enabled it. It now fails closed and throws on an unrecognized value. - Candidate execution timeout had no path through settings; added `TrainingSettings.execution.timeoutMs`, distinct from the retrying `resilience.evaluate` policy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
…ream Errors were bare Error/TypeError/SyntaxError at ~40 sites, carrying a good message and nothing else, so the only way to tell "not enough traces" from "no engine configured" from "gate rejected" was to match on message text -- which is what the tests had to do. Adds TsAutocodeError with a `code` discriminant and concrete subclasses that carry the facts a caller would otherwise re-derive: InsufficientTracesError holds `required`/`found`, PromotionRejectedError holds the decision and its failures, TrainingIncompleteError holds the outcome. Nothing breaks. Every message string is preserved byte for byte, so existing catch blocks and substring assertions keep working. Errors that were TypeError or SyntaxError still are: family membership is decided by a brand rather than the prototype chain, so `instanceof TsAutocodeError` recognizes them without changing their existing type. Zod failures are wrapped as InvalidSettingsError instead of escaping as a schema-library type. Also: - TrainingRun.canActivate() reports whether the final candidate can be applied without provoking an exception. `outcome` already distinguished "stalled" from "exhausted"; a caller should not have needed try/catch to read it. - TrainingSettings.onEvent reports background work as a discriminated union, including evolution.started/applied/skipped/failed, which had no observable signal at all. onError is retained, deprecated, and implemented as a projection of the same stream, so both can be configured without a failure being delivered twice to one handler. - The evolution sad path documented in the README now has a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Four conventions disagreed with themselves; each is now settled additively,
with the old spelling kept, deprecated, and covered by test/deprecated.test.ts
so the compatibility promise is enforced rather than asserted.
- Runtime scoping. `configureTraining` configured a module singleton and
replaced it wholesale, so a second call silently discarded the first's
settings and nothing could hold an isolated runtime. Adds
`createTrainingRuntime(settings)`, which registers nothing globally, and
`resetTraining()` for tests. `configureTraining` keeps replacing by default
-- silently carrying settings between unrelated calls would be a worse
surprise than the one it fixes -- and takes `{ merge: true }` to opt in.
Named apart from `createTraining`, which training.test.ts deliberately
asserts the package must not export.
- Grouped train options. `TrainInput` grouped `evaluation` but flattened six
round and gate options, and `policy` was a `PromotionGate` in disguise --
the evaluator wrapped it into one -- so two spellings expressed one concept.
Adds `rounds: { max, fanOut }` and `promotion: { minScore, minPassRate,
gates }`. Both forms are honored; gates from both run rather than one
shadowing the other.
- Opt-in that reads as opt-in. `evolution.enabled` was the only opt-in switch
among three identically named ones, and it is the one that rewrites your
source. Adds `evolution.auto`; `enabled` still works.
- Name collisions. Grounding's `digest` hashed normalized text while rewrite's
canonicalizes an arbitrary value; both emit a `sha256:` prefix, so swapping
them silently changes every hash. Renamed `textDigest`. Grounding's three
SCREAMING_SNAKE exports, unique in this workspace, gain camelCase aliases.
- Root surface. Adds `ts-autocode/internal` for the author-level seams
(`captureTrainable`, `provideTrainingDefaults`, the rewrite primitives), so
what an application imports is what an application needs. All of it stays
exported from the root.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Choosing a model is the first thing most users change, and it was the one thing the zero-config path could not do. The default engine hardcoded OpenAI; using anything else meant importing createAxEngine from the ts-autocode/ax subpath -- mentioned once in the README with no example anywhere in the repo -- constructing an AxAIService, and handing a whole replacement engine to configureTraining. Adds TrainingSettings.model, a provider-neutral ModelSelection carrying provider, model name, an optional apiKey, and an optional stronger teacher model. ts-autocode-training stays provider-agnostic: it forwards the descriptor to whatever engine is configured through EngineContext, exactly as it already forwards secrets and variables, and the default Ax engine interprets provider as an Ax provider name. Credentials resolve in order: an explicit model.apiKey, the configured secret provider, then the environment variable conventional for that provider. Previously only OPENAI_API_KEY was ever consulted, so a user who named another provider would have been told to set the wrong variable; an unlisted provider falls back to <PROVIDER>_API_KEY rather than failing. Also documents ts-autocode/ax with a real example for the Ax-specific tuning the neutral slot does not cover, which had none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The product is "instrument your app and let it rewrite itself", but inspecting
what is trainable, what has been captured, or what a run would change required
writing a script that imports discoverTrainables. No package declared a bin.
`ts-autocode discover` lists every marked method with its signature and, more
importantly, the exact identity to pass to defineTrainable. That is the one
place an otherwise type-safe design falls back to a string -- a typo in
`defineTrainable("Router.route")` silently yields a different symbol -- so
printing real ids is what makes the marker design usable without reading the
source scanner.
`ts-autocode status` reports captured traces per trainable, which is what
background evolution counts against evolution.minTraces. Both take --cwd,
--project, --file, --output-dir and --json.
The CLI is a function returning {code, stdout, stderr} with a thin bin wrapper,
so it is tested without spawning a process, and library failures are reported
by message rather than as a stack trace.
Also makes examples/optimize.ts real, per CONTRIBUTING's own rule. It imported
"../src/index.js" rather than the package name, exported rather than ran, and
was referenced by no test or script, so nothing would have noticed it breaking.
It now imports by package name (tsconfig.test.json maps the specifier to src/),
runs directly under node, and executes on every check against a stub engine so
CI needs no provider key.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Five things a consumer had to work around.
- Types they had to supply but could not construct. Implementing a custom
TrainingLoop means returning a CandidateReview containing a
TrainableEvalRun, which only the internals could produce -- so this repo's
own tests wrote `{...} as unknown as TrainableEvalRun` and `{} as never`, and
a consumer had no better option. Adds createEvalRun,
createPromotionDecision and createCandidateReview, and uses them in the two
tests that needed the casts, which now have none.
- Uninferrable generics. defineTrainingHarness takes three type parameters but
its settings mention only TCandidate, so a bare call inferred `unknown`
three times and every documented call site wrote them all out. TChallenge was
already scoped to `run` and inferred correctly; `inferringHarness()` gives
the other two the same treatment.
- A lossy argument guess with no way out. Eval inputs were JSON.parsed and
spread as arguments, so a trainable taking the literal string "[1,2]"
received two numbers. Adds ExecutionSettings.decodeArgs, with the previous
behavior exported as `evaluationArgs` and still the default.
- `...(x === undefined ? {} : { x })`, written out about twenty-five times
because exactOptionalPropertyTypes forbids assigning an explicit undefined,
plus a one-off maybeSignal() doing the same for one field. Adds
`optional(key, value)` and `defined(values)` and applies them.
- Effect as a root runtime dependency to express two try/catch statements.
attempt/attemptAsync are now plain try/catch and `effect` is dropped from the
root package. It stays where it earns its place: resilience.ts, whose
timeout/retry/interruption composition is genuinely hard by hand.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Adds the packages/grounding README its own package.json homepage has always linked to and that never existed, and extends the documentation typecheck to cover it and docs/architecture.md, so every fenced TypeScript block in the repo now compiles. Updates prose the preceding commits made stale: onEvent alongside the deprecated onError, rounds.fanOut and promotion.gates, the fail-closed evolve switch, model selection as a neutral descriptor rather than a provider-specific option, and the consumer/author surface split. Records in docs/dx-review.md what shipped, plus two places the remediation deliberately departed from the plan -- configureTraining still replaces by default, and evolution's opt-in polarity was renamed rather than flipped -- and corrects one claim the review got wrong about test/wiring.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The review claimed a bundler honoring `sideEffects: false` could drop the package's import-time wiring and leave a consumer with "no training engine is configured". Bundling a trivial consumer with esbuild --tree-shaking=true produces byte-identical output with the flag set either way, so that failure was asserted rather than observed. The fix stands and is still correct: the declaration was factually untrue, since importing the root module registers four providers and configures rewrite capture. But the finding now says what it is -- a latent correctness bug in a promise made to bundlers -- and records that esbuild does not collect on it, rather than implying a reproduced breakage. Also drops the planned tree-shaking bundle test, which would pass either way and prove nothing. The manifest assertion in test/tier1.test.ts is what actually guards the declaration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
CI was red on every commit in this branch while `npm run check` passed locally. Two causes, both mine, plus a real pre-existing bug the failure uncovered. **examples/optimize.ts could not resolve `ts-autocode`.** It imports by package name, as a consumer would, but the package entry points at `dist/`, which `npm run check` does not build until *after* the tests run, and Vitest does not read tsconfig `paths`. It passed locally only because a stale `dist/` happened to be lying around -- exactly the works-on-my-machine trap. Mirrors the tsconfig paths as Vitest resolve aliases so the example resolves from source, deterministically. **test/tier1.test.ts imported src/register.ts** to reach a string-parsing function, and importing that module installs a load hook. That surfaced the real bug: **`ts-autocode/register` crashed on Node 20.** `module.registerHooks` is the synchronous in-thread loader API, added in Node 22.15. `engines` declares `node >= 20`, and the README's headline zero-config command is `node --import ts-autocode/register`, so the flagship feature was broken on the minimum supported version -- and failed with an internal `TypeError: registerHooks is not a function` rather than anything actionable. Nothing had ever imported that module in a test: test/register.test.ts exercises only the pure `augmentSource`, so the side-effecting entry was never loaded under test on any Node. The guard now names the requirement, says the rest of the package still works on Node 20, and points at the decorator, which needs no load hook. The README says so too. Also splits the evolve kill switch (src/evolve.ts) and the load-hook guard (src/load-hook.ts) out of the side-effecting entry, so both are testable without installing anything. Verified with a clean dist/ on Node 20.20.2 -- the exact version CI failed on -- and on Node 22: 218 tests pass on both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The suite had 218 tests and no coverage measurement at all, which made "we
have tests" unfalsifiable. Measuring it first showed 85.35% statements and
73.14% branches -- and that the three files added in the previous PR were
among the worst covered in the workspace: errors.ts at 16.66% branch,
builders.ts at 55%, optional.ts at 75%.
Adds @vitest/coverage-v8, wires coverage into `npm run check`, and sets
thresholds as a ratchet (90/78/93/93 -- what is actually achieved now, to be
raised as later suites land, never lowered).
New atomic unit suites, 184 tests:
- errors.ts: every constructor, static factory, payload accessor and message
string; both directions of the brand-based `hasInstance`, including that a
hand-rolled look-alike is not admitted and that subclass `instanceof` stays
exact; and the Zod boundary.
- optional.ts / defined(): asserts key *presence*, not deep equality --
`{a: undefined}` and `{}` compare equal under toEqual, so the distinction the
helpers exist for would otherwise go unchecked.
- builders.ts: every defaulting rule a consumer will rely on without reading
the source, plus that supplied evaluations are copied rather than aliased.
- token.ts: the normalization and rejection rules standing between a typo and
a silently different identity.
- canonical.ts, component.ts: pre-existing gaps at 46% and 50% branch. The
class-instance case matters -- if `isRecord` wrongly accepted a Date, every
Date would hash identically.
- attempt.ts (both copies), and the CLI's status and option paths.
Also adds test/digest-protocol.test.ts. Training and rewrite each implement the
body digest and never import each other, so guarded application depends on two
independent implementations agreeing; that agreement was assumed, and is now
asserted.
Boy-scout fix: test/docs.test.ts built one ts.Program per snippet, taking ~36s
and relying on the default 5s per-test timeout. Under coverage instrumentation
it blew that timeout and failed 15 of 26. It now builds one program for all
snippets: 3s, and robust under instrumentation. Verified it still fails when a
README snippet breaks rather than having gone vacuous.
218 -> 402 tests; 85.35 -> 90.15% statements, 73.14 -> 79% branches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The workspace had no snapshot tests at all. For a library whose product is
rewritten source, the generated text *is* the product, and a diff of it is the
only review that shows what actually changed -- `toContain("return input")`
says almost nothing about an emitted module.
test/support/verify.ts follows the Verify model rather than Vitest's inline
snapshots: one named file per subject under test/snapshots/, committed and
reviewed like any other artifact. An inline .snap blob keyed by test name is
hard to read in a diff and a rename silently orphans it. `scrub()` removes
digests, UUIDs, timestamps and absolute paths, because a snapshot that churns
is one everyone learns to re-approve without reading.
Approved: discovered source targets (the whole contract handed to an
optimizer), emitted instrumentation and the augmented module, the synthetic
candidate declaration for sync and async targets, applied rewrites, grounding
codegen, promotion decisions, CLI usage/discover/status, the export surface of
all seven entry points with each export's kind, the error message catalogue,
the promotion rubric read by the judge, and the Ax program signature.
The last two are the ones nothing else could pin: both are read by a model
rather than by code, and neither has a natural assertion. The rubric is where
the literal string "evaluation default" once shipped in place of a threshold.
Verified the snapshots actually fail on a real change rather than passing
vacuously -- which also surfaced that the root suite runs against siblings'
built dist/, so a sibling source edit needs a rebuild before it is visible.
Boy-scout fix, found by the Ax snapshot: a parameter with a literal default and
no annotation (`retries = 2`) was reported as type `unknown`, which the Ax field
mapper turned into `json` -- so the optimizer was told a plainly numeric
argument had an opaque shape. Source discovery now infers string, number,
boolean, bigint and uniform array types from a literal initializer, exactly
where TypeScript would. Anything non-literal stays `unknown`. The snapshot now
shows `{"name": "number"}` where it showed `{"name": "json"}`.
402 -> 443 tests; branches 79 -> 80.29%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #27. The workspace had zero snapshot tests.
Why this matters most for this library
The product is rewritten source. The generated text is the deliverable, and a diff of it is the only review that shows what actually changed —
toContain("return input")says almost nothing about an emitted module.The Verify model, not inline snapshots
test/support/verify.tsfollows Verify rather than Vitest's inlinetoMatchSnapshot: one named file per subject undertest/snapshots/, committed and reviewed like any other artifact. An inline.snapblob keyed by test name is hard to read in a diff, and a test rename silently orphans it.scrub()strips digests, UUIDs, timestamps and absolute paths first — a snapshot that churns is one everyone learns to re-approve without reading.What's approved
Discovered source targets · emitted instrumentation · the augmented module · the register-hook output · synthetic candidate declarations (sync + async) · applied rewrites · grounding codegen · promotion decisions · CLI usage/discover/status · the export surface of all seven entry points, with each export's kind · the error message catalogue · the promotion rubric the judge reads · the Ax program signature.
The last two are the ones nothing else could pin — both are read by a model rather than by code, so neither has a natural assertion. The rubric is exactly where the literal string
evaluation defaultonce shipped in place of a real threshold.A bug the snapshots found
The Ax program snapshot showed this:
{ "name": "methodArgumentRetries", "type": { "name": "json" }, "description": "retries = 2" }A parameter with a literal default and no annotation was reported as
unknown, which the Ax field mapper turned intojson— the optimizer was being told a plainly numeric argument had an opaque shape. Source discovery now infersstring/number/boolean/bigint/uniform-array types from a literal initializer, exactly where TypeScript would; anything non-literal staysunknown. The snapshot now reads{"name": "number"}.This is what characterization tests are for: nothing was failing, and no assertion would have been written for it.
Verified non-vacuous
I changed
instrumentKeyand confirmed three snapshots fail with a readable diff. That also surfaced a workflow detail worth knowing: the root suite runs against siblings' builtdist/, so a sibling source edit needs a rebuild before it's visible.npm run checkbuilds first, so CI is unaffected.Verification
npm run checkgreen. 402 → 443 tests; branches 79 → 80.29%.Snapshots are excluded from
tsconfig.test.json— they're generated artifacts, and the emitted instrumentation deliberately references names from the module it's appended to, so it doesn't typecheck standalone.🤖 Generated with Claude Code
https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Generated by Claude Code