Normalize the extension surface: user-supplied model services, Promoter, enforced defaults - #35
Merged
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
The workspace had neither. Properties state the law and let fast-check hunt for a counterexample; fuzzing feeds the parsers input they were not written for. Both found real defects on the first run. Properties cover identity round-trips, digest canonicalization (load-bearing: guarded rewriting refuses a candidate whose body digest changed), the spread helpers, evaluation-argument decoding, and promotion-gate aggregation. Fuzzing covers source discovery, the register load hook, ambient declaration scanning, the evolve kill switch, and the CLI. Four findings, all fixed here: 1. `toTrainableToken` was not exported from either barrel. It takes the public `TrainableIdentity` type and is the canonical validator, so anyone implementing a loop, engine or store needs it. Now exported; the surface snapshot shows the single added line. 2. `minScore: Infinity` reported "expected number, received number" -- Zod's base schema rejected it before the .finite() message could apply, so a user who passed a bad threshold was told nothing useful. Every rejection now reports the range. 3. Discovery could report `bodyEnd` past the end of the source. TypeScript's error recovery synthesizes a body for an unterminated block whose `end` sits past EOF, so a truncated file produced a target claiming offsets outside its own source. Slicing clamps, so nothing was corrupted, but publishing an out-of-range range is malformed data crossing a public boundary. Now clamped -- a no-op for source that parses. 4. `TrainableTarget`'s body fields had undocumented and subtly different relationships to the source: `implementation` is trimmed, `bodyDigest` hashes the raw slice, and guarded application depends on the digest side. Nothing said so until a property asked. Now documented and pinned. The fuzz corpus is itself tested. An early version used random punctuation; instrumenting it showed 1 input in 3000 produced a discovered target, so every property about offsets and rewriting was passing vacuously. test/support/sources.ts generates structurally plausible marked modules and then damages them, and the suite asserts the corpus still reaches real work. One property documents a limitation rather than a bug: `-0` cannot round-trip through an eval input, because JSON.stringify(-0) is "0". 443 -> 500 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
…ound The provider-neutral design says any structurally compatible implementation works. That was only ever checked against the implementations shipped here, through whatever paths happened to exercise them -- which is not a claim about anyone else's, and not a stated contract at all. Adds conformance suites for all five injected seams, shipped in ts-autocode-training so an implementer can run them against their own provider. They are framework-agnostic on purpose: a list of named checks that throw on violation, driven by whatever runner the consumer has. They state what types cannot: a store preserves append order and does not alias its internal state; an executor surfaces a throwing body as a rejection; a loop returns the winning round last, because the runtime activates `rounds.at(-1)`; an applier refuses a decision bound to a different candidate. test/contract.test.ts runs every provider this repo ships through them, plus a second, deliberately different store -- a suite that only ever sees one shape describes that shape rather than a contract. packages/training/test/conformance.test.ts proves each suite *rejects* an implementation violating the rule it names, so the kit cannot pass everything. Two defects fixed, both found while writing this: 1. `defined()` built its result with `result[key] = value`, which goes through the `__proto__` setter on Object.prototype. A `__proto__` key was silently dropped, and with an object value it replaced the result's prototype instead of adding a key. Now built with Object.fromEntries, which defines own properties. Found by a property test over arbitrary dictionaries; `optional()` was already safe because a computed key in an object literal defines rather than assigns. 2. One conformance check was written vacuously -- it asserted `rejected || resolved`, which is always true. It now counts proposals, so it genuinely rejects a loop that keeps calling the engine after an abort, and a test proves it does. Also corrects an assertion in the property suite: `key in spread` walks the prototype chain, so any key named after an Object.prototype member read as present whether or not it was added. fast-check found that mistake in my own test within a few hundred runs. 500 -> 573 tests; thresholds raised to 91/80/93/93. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
CI on this branch failed with `Counterexample: ["toString", undefined]`, seed 1960220664. Two separate defects, both here: 1. The assertion was wrong. `key in spread` walks the prototype chain, so any key named after an Object.prototype member -- "toString", "constructor", "valueOf" -- read as present whether or not it had been added. It takes fast-check about 87 runs to find one, which is why it passed locally and failed in CI: the seed is random per run. Now `Object.hasOwn`. 2. `defined()` was genuinely broken for the same class of key. It built its result with `result[key] = value`, an assignment that goes through the `__proto__` setter on Object.prototype: a `__proto__` key was silently dropped, and with an object value it replaced the result's prototype instead of adding a key. Now built with Object.fromEntries, which defines own properties. `optional()` was already safe, because a computed key in an object literal defines rather than assigns. The second is the reason this belongs in this PR rather than a later one: the dictionary property that finds it was introduced here, so leaving the fix downstream would leave this branch intermittently red. Reproduced the original failure with CI's exact seed before fixing, then confirmed 2000 runs of that seed and 5000 of the dictionary property pass, plus eight full suite runs on random seeds. Adds regression tests for every key that shadows Object.prototype, including that Object.prototype itself is never polluted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The __proto__ fix and both hasOwn assertion corrections moved to the PR whose properties found them, so this branch takes them from its base rather than carrying them itself.
Every other suite exercises the path where nothing goes wrong. But the engine
is a network call to a model, the executor runs code that model wrote, the
store is I/O, and the file being rewritten is one a developer may be editing
at the same moment.
Two properties are asserted repeatedly, because violating them would be
unacceptable rather than merely annoying:
- a failure in capture or evolution never breaks, delays or alters the
application call being traced -- the method's own error propagates
unchanged, not the store's;
- a failure during promotion never leaves a partially rewritten file.
Injected: stores that throw, throw intermittently, hang, or need retries;
engines that fail, rate-limit, hang, return invalid TypeScript, or return
nothing; executors that throw or hang; signals aborting mid-round; files edited
between discovery and application and between application and rollback; and
event handlers, serializers and capture mappers that throw.
One check earns its place on its own: an operation whose caller has already
aborted is not retried, because retrying cancelled work spends money on a
result nobody wants.
Boy-scout fix found by writing these: `Training.capture`.
`captureTrainable` routes to the process-wide runtime, which is what installed
instrumentation calls. A runtime built with `createTrainingRuntime` registers
nothing globally -- by design -- so it could train and evaluate but never
capture. That is half a runtime, and precisely the multi-tenant case isolation
was added for. `Training.capture(identity, methodName, this, method, args)`
closes it, delegating to the same internal path, with unit tests covering
`this`, thrown errors, async settling, and that two isolated runtimes do not
see each other's records.
573 -> 607 tests; thresholds now met at 91.41/80.69/94.01/94.11.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Every other suite is organized around units and failure modes. These are organized around what a user is trying to do, in the vocabulary the README uses -- mark, capture, train, gate, activate, roll back -- so this is the suite that fails when a documented promise stops being true even though every unit still passes. Each `it` is one sentence of the contract the library offers. test/support/scenario.ts is a given/when/then builder rather than a BDD framework: Gherkin's parser buys little when the steps are TypeScript anyway, and a plain builder keeps the spec and its assertions in one file. Specified: marking observes without intercepting, and never swallows the method's own error; nothing is written until something is activated; a passing candidate promotes and a failing one is refused with its gate failures; an engine failure is a typed error rather than a stalled run; captured traffic becomes eval cases, and repeated inputs count once; activation rewrites only the marked body and leaves line count and directive intact; rollback restores byte for byte, and refuses to overwrite an edit made after activation; and evolution trains, skips, or fails without ever disturbing an application call. The load-bearing spec is "refuses a candidate that changes the behavior the traffic demonstrated". That is the entire safety story for unattended rewriting -- evolution replays captured traffic as equality cases, so a candidate that alters observed behavior cannot pass the gate however confident the model was -- and it deserves a spec that reads like the claim. Writing it is what showed the first draft of the "applies" spec was proposing a behavior-changing candidate and would never have promoted. 607 -> 625 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Coverage says a line ran. Mutation testing says a test would notice if that line were wrong. Stryker rewrites the source -- flips a comparison, deletes a branch, replaces a function with one that returns nothing -- and reports how many of those changes the suite catches. Scoped to the modules where a surviving mutant is alarming rather than untidy: the promotion gate, which decides whether generated code is written into someone's source file, and the rewrite guard, which decides whether it lands on the body it was verified against. Mutating everything would take hours and mostly re-measure line coverage, which vitest already enforces. The first run scored 72.31%, and the detail mattered more than the number: promotion.ts 68.42 -> 100 digest.ts 13.89 -> 100 apply.ts 66.00 -> 90 builders/optional/token already 100 Replacing an entire gate rule with `() => undefined` -- so that rule can never refuse a candidate -- left the suite green. The tests asserted that a bad candidate was refused, but never which rule refused it, so any single rule could have been deleted undetected. packages/training/test/gates.test.ts now pins each rule with evidence where it, and only it, is the reason. Two survivors were genuine off-by-one risks no example-based test would have found. `.every(...)` and `.some(...)` are indistinguishable when the evidence is a single evaluation, so the binding rules needed mixed evidence. And `score >= threshold` versus `>` only differs when a score sits exactly on the boundary, which nothing tested -- in the rule that decides whether a candidate counts as passing. digest.ts scored 13.89% because its only tests lived in other packages; it now has a package-internal suite covering key sorting, the plain-object check that keeps every Date from hashing alike, and whitespace sensitivity, which is why the raw body slice is hashed rather than the trimmed one. Two small consistency fixes found along the way: `evaluatePromotionGate` now freezes its `failures` array, which `createPromotionDecision` already did, so the two producers of a PromotionDecision no longer differ in mutability; and the reindentation tests were computing `bodyEnd` differently from source discovery, which produced a stray blank line and looked like a library bug. Final score 98.05%, with the promotion gate at 100%. Runs in about a minute, so it runs on every pull request rather than on a schedule nobody reads. Thresholds are a ratchet: break at 95, raise as suites improve. 625 -> 672 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Every seam resolved `settings.X ?? defaultProviders.X` except `promote`,
which read the process-wide provider only. `TrainingSettings` had no slot
for it, so an applier could be registered exactly one way: globally, via
`provideTrainingDefaults`.
That left `createTrainingRuntime` — the isolation feature — sharing one
applier between runtimes, and the shared component is the one that writes
generated code into a user's source file. It is the same hole found in
`Training.capture`, in the same feature: a runtime that could train and
evaluate but could not be given the seam it needed.
The corroborating evidence is `packages/training/test/wiring.ts`, which
exists only to call `provideTrainingDefaults({ promote })` at module
scope. That file is named as finding E1 in the DX review, and
`createTrainingRuntime` was supposed to retire it; it survives because
there was nowhere else to put an applier.
Additive: `src/index.ts` still registers the global default, and a
runtime without the setting behaves exactly as before.
Found while writing the provider authoring guide — the wiring example
would not compile.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
Five seams are injected and any structurally compatible implementation works, but nothing said how to write one. The README documented custom engines and loops; `ImplementationExecutor`, `PromotionApplier` and `TrainingStore` had no worked example anywhere, and the conformance kit -- the thing that makes the provider-neutral claim checkable by someone outside this repo -- had zero mentions in any markdown file. Its only usage guidance was a source comment. The guide opens with a table mapping intent to answer, because the most common wrong turn is writing an engine to change models: that is a setting. Then one section per seam, each carrying the rules the types cannot state -- a loop returns the winning round last, an executor surfaces a throwing body as a rejection, a store preserves append order and does not alias internal state, an applier refuses a decision bound to another candidate. The engine and loop sections link to the README's existing examples rather than copying them; two copies of one example drift. Every snippet is compiled by `test/docs.test.ts` (37 snippets, up from 26). Verified non-vacuous: breaking one fails that snippet's own case with a real diagnostic. Also: - `test/docs.test.ts` named a missing doc instead of throwing an unhandled ENOENT at module load, which took the whole file down. - `packages/training/README.md` listed four injected seams; there are five. It omitted `TrainingStore`. - `docs/testing.md` described the docs layer as README and architecture snippets only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
…riptor
`TrainingSettings.model` only accepted a `{ provider, name, apiKey }`
descriptor. The descriptor routes through the configured engine's own
provider registry -- Ax's, for the default -- but anything that registry
does not cover (a self-hosted endpoint, a proxy with its own auth, a
client the application already built) required constructing a whole
replacement engine via `ts-autocode/ax`. That put this library in the
business of brokering providers when it should accept an adapter, the
way every other AI library does.
`ModelSelection.service` carries a pre-built client -- or a factory
returning one -- opaquely through the training package, which never
calls it, to whichever engine is configured. The default Ax engine
accepts any `AxAIService` and rejects anything else with an
`InvalidSettingsError` naming the setting and the expected shape, at
configuration depth rather than mid-optimization. `teacher.service`
does the same for the teacher role. When `service` is set, `provider`,
`name` and `apiKey` are the client's concern, not this library's.
The provider/name descriptor stays as sugar over the engine's registry;
its docs now say that is what it is, rather than reading like an
internal support matrix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
…nforced defaults Three normalizations, all additive: **`PromotionApplier` -> `Promoter`.** Four of the five seams were agent nouns (engine, executor, loop, store); "applier" was the outlier and reads as invented language. `Promoter` is the agent noun of the thing it does. The old name stays as a deprecated structural alias, and `promotionApplierContract` likewise aliases `promoterContract` — no implementation changes. **`directExecutor` is exported.** The no-isolation `new Function` executor was reimplemented by hand in ten files in this repo alone — the clearest sign consumers would be forced to reproduce it too. It now ships once, documented as trusted-candidates-only, and the in-repo copies import it. Writing an executor is now only for isolation you actually own. **Every seam's default is enforced, not asserted.** The provider-neutral design only holds its weight if implementing a seam is opt-in, never homework. `test/defaults.test.ts` pins it: a zero-config runtime must fail on the missing credential (`MissingSecretError`) and never on a seam's own NotConfigured error. If a future change unwires a default, this test names the regression. Hardening: `execution.timeoutMs` is validated at the boundary (`InvalidSettingsError` naming the setting) instead of flowing into the executor as a nonsense timeout. `CONTRIBUTING.md` now states the naming and shape rules the surface follows — one factory verb, the Settings/Options split, agent-noun seams with defaults and conformance suites, provider choices as settings, boundary validation — so the next addition has a rule to follow rather than a precedent to guess from. 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 |
…cted] cases, bare-function engines
The seam types were already optional; the ceremony lived at every call
site. Three cuts, all additive, each defined as sugar over an existing
semantics rather than a second one:
**Identities are plain strings.** `TrainableIdentity` now admits the id
`ts-autocode discover` prints, so `train({ trainable: "Router.route" })`
replaces `defineTrainable("Router.route").symbol`. Strings were rejected
on the theory that the branded token was safer -- but defineTrainable(id)
is itself an unchecked string, so the rejection bought no safety, only
ceremony. Symbols and tokens still work everywhere.
**`TrainInput.cases` is `[input, expected]` pairs.** Each becomes an
equality eval case and the baseline task is a lookup over the pairs --
the exact mechanism replayed live traffic already uses, so explicit
cases and replay are one evaluation semantics with two sources.
Non-strings are JSON-encoded the way outputs are compared; repeated
inputs keep the last expected value, as replay does. Explicit
`evaluation.tests` still win; `evaluation` still carries `outputDir` and
friends alongside `cases`.
**An engine can be a bare function.** `(request, context) => body`
(or a full candidate) is accepted anywhere a `TrainingEngine` is; the id
is derived from the function name. The `{ id, optimize }` object form
remains for engines with an identity worth publishing. `asEngine`
normalizes either spelling and is exported.
Also fixed here, found while writing the examples: the authoring guide
claimed `promotion.gates` REPLACES the standard gate set and told
readers to spread `defaultPromotionGates`. promotion.ts:110 appends
configured gates after the standard set, so the advice would have run
the default gates twice. The guide now states the append semantics and
its example passes only the extra rule. README quickstart and the CLI
discover hint rewritten to the short forms; the long forms stay
documented as the escape hatch.
Snapshot approvals in this commit: surface gains exactly `asEngine`;
the CLI discover snapshot carries the new hint.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
… method now works
The previous commit admitted plain strings as trainable identities. That
contradicted an ADR the maintainer has rejected multiple times: a plain
string is not a sufficient identity to guarantee uniqueness, and every
call site becomes an unchecked retyping of it. This commit treats that
as the failure it was.
**Reverted completely.** `TrainableIdentity` no longer admits `string`;
`trainable: "Router.route"` is a compile error again, and the runtime
rejection message is restored byte-for-byte. The rejection is now
pinned at the ADR level: `test/adr.test.ts` holds a `@ts-expect-error`
on the string spelling, so re-admitting it ever again fails typecheck
with an unused suppression — the decision is enforced, not remembered.
`CONTRIBUTING.md` records the ADR next to the other surface rules.
**The alternative the ADR allows: the marked method itself.** The
marking machinery — the `@trainable()` decorator, `wrapTrainable`, and
`instrumentTrainable` — now stamps the callable it marks with the
identity it declared, under the registry symbol
`Symbol.for("ts-autocode.trainable.id")`. `TrainableIdentity` gains
that third arm:
train({ trainable: Router.prototype.route })
resolves the identity the instrumentation wrote, so nothing is retyped
anywhere. An unmarked function is refused with an error naming the fix.
`stampTrainable` and `trainableStamp` are exported for instrumentation
authors; the surface snapshot moved by exactly those two names.
The other two shorthands from the previous commit stand: `cases` pairs
and bare-function engines touched no identity guarantees. README, the
authoring guide, and the CLI discover hint are restored to the
symbol/token forms, with the marked-method form documented where
instrumentation makes it available.
The `!` marks reverting the string acceptance pushed to this branch
earlier today; it was never released.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
…mbol), train(symbol)
The maintainer's design, now implemented as stated: the application
declares a `unique symbol` it owns; the `@trainable(symbol)` decorator
on the trainable code registers the declaration under that symbol; and
`training.train(symbol)` reuses the same symbol, so discovery is plain
symbol-key indexing. The symbol's object identity is the uniqueness
guarantee. The durable string id the machinery needs — for stores and
source rewriting — is derived from the declaring class and method; the
user types no name anywhere.
export const route: unique symbol = Symbol("route");
class Router {
@trainable(route)
route(input: string): string { "use training"; return input; }
}
await training.train(route, { cases: [["abc", "ABC"]] });
What changed:
- `registerTrainable(symbol, token)`: the in-process symbol index the
decorator writes and every identity lookup reads first. A symbol keys
exactly one trainable; rebinding is refused with a named error.
- `@trainable(symbol)` with a unique symbol registers it against the
inferred declaration id. Registry (`Symbol.for`) symbols keep their
existing behavior for the zero-config directive flow, where ids are
machinery-derived from parsed source.
- `train(symbol, options?)` positional overload, as the design reads;
the object form remains.
- **`defineTrainable("...")` is purged from every README, doc, snippet,
and CLI suggestion.** It remains as machinery (source discovery,
replay, the register hook derive ids from the AST — machinery-derived
ids are allowed; user-typed ones are not). `test/adr.test.ts` now
scans every documentation snippet and fails CI if the banned pattern
reappears, alongside the existing compile-time string pin and a new
end-to-end unique-symbol test.
- `AGENTS.md` (mirrored as `CLAUDE.md`) states the rule as a hard
failure for any agent working in this repo, with the full intended
design and the enforcement tests by name. `CONTRIBUTING.md` updated
to match.
The `!` marks correcting unreleased commits on this branch; the string
form of this mistake was already reverted in eacf55c, and this commit
removes its declaration-site cousin from the taught surface.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
The mutation job caught what the unit run missed: `registerTrainable` landed with 10 uncovered mutants and 4 survivors in token.ts, dropping the score to 92.98 against the 95 ratchet. The ADR suite exercises the index end to end, but it lives outside the mutation config's test set; identity code needs its pins in the package-internal token suite the mutation run actually executes. Added there: index-beats-description resolution (a unique symbol whose description would derive a different id must resolve through the registration, or it silently forks), idempotent re-registration, rebind refusal that leaves the original binding intact, frozen bound tokens, stamp-only-functions, non-enumerable stamping, restamping semantics, the anonymous-function refusal message, and the published registry key `ts-autocode.trainable.id` pinned as cross-package protocol. token.ts 80% -> 100%; overall 92.98% -> 98.25%, over the 95 ratchet, which stays where it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
A high-effort review of the combined stack before merging surfaced
eight findings; six are fixed here, two were judged working-as-decided
(the fanOut refusal implements review finding A4's sanctioned "reject
with a clear error, never ignore", and the evolve kill-switch change
was an intentional fail-closed fix).
- **The frozen `training` facade dropped train's second argument.**
`training.train(route, { cases })` forwarded only the identity, so
the documented call silently fell back to replay evaluation. Fixed
and pinned by a test that distinguishes the two paths by which error
they reach (verified non-vacuous by reverting the fix).
- **canActivate() could promise a doomed activate().** It checked only
the gate decision; a runtime with no promotion applier reported
ready and then threw PromotionApplierNotConfiguredError. Readiness
now reports the missing applier, pinned in a test file that
deliberately skips the global wiring.
- **An unregistered unique symbol fell back to description-derived
ids** -- string identity by the back door, able to silently target a
different trainable. It now fails loudly, naming the fix; registry
symbols keep their machinery path. Pinned to 100% mutation score.
- **directExecutor could not run await-bearing candidates** for async
targets: a sync Function constructor throws on `await` that the
engine itself validated. Async targets now compile via
AsyncFunction.
- **examples/optimize.ts still taught defineTrainable("...")** -- the
ADR's banned pattern, escaping the doc scan because it is a .ts
file. The example now declares a unique symbol and registers via
`instrumentTrainable(Router, "route", route)`, which gains a symbol
overload: exactly `@trainable(symbol)` without decorator syntax, for
runtimes whose transforms cannot lower TC39 decorators (Node
strip-types, and the oxc pipeline vitest 4 uses -- both verified to
reject the syntax). The ADR scan now also covers examples/*.ts.
- **Prose in docs/architecture.md still directed applications at
defineTrainable**; rewritten to the symbol-key flow.
- **`ts-autocode status` read an artifact nothing ships writes.** Its
usage text now says exactly what it reads and that the default
in-memory store does not persist it, instead of silently reporting
zero captures as if that were the truth.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QchTdrzVJeexx1sjWjkJVb
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 #34. Responds to design feedback on the extension surface: the model descriptor read like an internal provider support matrix, the seam types read like required boilerplate,
PromotionApplieris invented language — and the call sites still carried too much ceremony. One change in this PR's history was itself rejected as an ADR violation and is reverted here with the rejection now enforced at compile time (see §3).1. Providers are the user's to supply, never this library's to bless
{ provider, name }descriptor was already pass-through — it goes to the configured engine's own registry (Ax's, by default), with a<PROVIDER>_API_KEYfallback for unlisted providers. The docs made it look like an internal allow-list; they no longer do.ts-autocode/ax. Nowmodel.service(andmodel.teacher.service) carries any pre-built client — or a factory receiving the engine context — opaquely to whichever engine is configured. The default Ax engine accepts anyAxAIServiceand rejects anything else withInvalidSettingsErrornaming the setting, at configuration depth rather than mid-optimization.2. The seam types are not homework
test/defaults.test.tspins that a zero-config runtime fails only on the missing credential — never on any seam's ownNotConfigurederror.directExecutorships — the no-isolation executor that ten files in this repo had hand-reimplemented; documented trusted-candidates-only.3. Identity: the ADR, violated and then enforced
A commit on this branch briefly admitted
trainable: "Router.route". That violated an ADR the maintainer has rejected multiple times — a plain string is not a sufficient identity to guarantee uniqueness — and is fully reverted:TrainableIdentitydoes not admitstring, the spelling is a compile error, and the runtime message is restored byte-for-byte.The rejection is now enforced, not remembered:
test/adr.test.tsholds a@ts-expect-erroron the string spelling, so any future re-admission failsnpm run typecheckwith an unused suppression.CONTRIBUTING.mdrecords the ADR.What the ADR allows instead — the marked method itself. The marking machinery (
@trainable(),wrapTrainable,instrumentTrainable) now stamps the callable it marks with the identity it declared (Symbol.for("ts-autocode.trainable.id")), so:resolves the identity the instrumentation wrote — nothing retyped anywhere. An unmarked function is refused with an error naming the fix.
stampTrainable/trainableStampare exported for instrumentation authors.4. Call-site ceremony cut (identity-preserving)
cases: [[input, expected], ...]replaces hand-built{ id, input, assert }objects for the equality case — sugar over the exact lookup-task mechanism live-traffic replay already uses.evaluation.testsstill wins.engine: (request, context) => bodyworks anywhere aTrainingEnginedoes;asEnginenormalizes.discoverhint showdefineTrainable(id).symbol(the string appears exactly once, insidedefineTrainable) with the marked-method form documented where instrumentation provides it.5. Naming + hardening
PromotionApplier→Promoter(deprecated structural aliases retained). Conventions inCONTRIBUTING.md: one factory verb,Settings/Optionssplit, agent-noun seams with defaults + conformance suites, provider choices as settings, boundary validation, and the identity ADR.execution.timeoutMsvalidated at the boundary.promotion.gatesreplaces the standard set;promotion.ts:110appends them, so the old advice ran defaults twice.Verification
npm run checkgreen: 684 → 705 tests, coverage 92.2% / 81.84%.@ts-expect-errorADR pin is verified live: typecheck passes only because the string spelling genuinely fails to compile.wrapTrainable,instrumentTrainable) plus the unmarked-function refusal.directExecutor,promoterContract,asEngine,stampTrainable,trainableStamp) and nothing else.