test(1/7): coverage enforcement and atomic unit gaps - #27
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
|
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 #26. First of seven PRs adding the test types the project was missing.
Why this one first
The suite had 218 tests and no coverage measurement at all, which makes "we have tests" unfalsifiable. Measuring first showed the honest baseline — and that the three files I added in #26 were among the worst covered in the workspace:
errors.ts(added in #26)builders.ts(added in #26)optional.ts(added in #26)canonical.ts(pre-existing)component.ts(pre-existing)What's here
@vitest/coverage-v8wired intonpm run check, with thresholds set as a ratchet at what is actually achieved (90/78/93/93) — to be raised as later PRs land, never lowered to get green.errors,optional/defined,builders,token,canonical,component,attempt(both copies), and the CLI's status/option paths.test/digest-protocol.test.ts— training and rewrite each implement the body digest and deliberately never import each other, so guarded application depends on two independent implementations agreeing. That was assumed; it is now asserted.docs/testing.md— what each layer catches, and the conventions.Two things worth calling out
optional/definedneeded structural assertions, nottoEqual.{a: undefined}and{}compare equal undertoEqual, so the exact distinction those helpers exist for — key absent vs. key present-and-undefined, which is whatexactOptionalPropertyTypesforbids — would have gone unchecked by the obvious test.Boy-scout fix:
test/docs.test.tswas fragile and I wrote it. It built onets.Programper snippet (~36s) and relied on the default 5s per-test timeout. Under coverage instrumentation it blew that timeout and failed 15 of 26 — it only ever passed because the machine was fast enough. Now one program for all snippets: 36s → 3s, robust under instrumentation. Verified it still fails when a README snippet breaks rather than having gone vacuous.Verification
npm run checkgreen. 218 → 402 tests. 85.35 → 90.15% statements, 73.14 → 79% branches, 87.79 → 93.13% functions.The rest of the stack
2/7characterization ·3/7property+fuzz ·4/7contract ·5/7chaos ·6/7BDD ·7/7mutation.🤖 Generated with Claude Code
https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Generated by Claude Code