Skip to content

Versioning primitives v2: client pinning, error shaping, introspection, behavior catalogs#4

Open
mahmoudimus wants to merge 58 commits into
mainfrom
tests/failing-issue-coverage
Open

Versioning primitives v2: client pinning, error shaping, introspection, behavior catalogs#4
mahmoudimus wants to merge 58 commits into
mainfrom
tests/failing-issue-coverage

Conversation

@mahmoudimus
Copy link
Copy Markdown
Owner

Summary

46 commits extending tsadwyn with the primitives real production adopters were hand-rolling on top of the framework: Stripe-style per-client pinning, server-driven error envelope versioning, introspection/debugging tools, typed per-version behavior catalogs, and a pre-wired /versioning upgrade resource. Consolidates the patterns from consumer integrations into first-class library surface.

Fixes #1 #2 #3.

⚠️ Breaking change

  • migrateHttpErrors now defaults to true (was false). Matches Stripe's actual production behavior: response migrations apply to 4xx/5xx error bodies unless explicitly opted out via { migrateHttpErrors: false }. Rationale and migration path in the README's Versioning error responses section. Consumers currently on false need to pass it explicitly or accept the new behavior.

New primitives

Client pinning (the Stripe model, productized)

  • perClientDefaultVersion({ identify, resolvePin, fallback, pinOnFirstResolve?, supportedVersions?, onStalePin? }) — canonical DB-backed default-version resolver
  • cachedPerClientDefaultVersion({ ..., ttlMs }) — high-QPS variant: TTL'd map, single-flight first-miss, explicit { resolver, invalidate, invalidateAll } handles
  • preVersionPick option on Tsadwyn — auth middleware runs before the default-version resolver
  • onUnsupportedVersion: 'reject' | 'fallback' | 'passthrough' on versionPickingMiddleware
  • createVersioningRoutes({ identify, loadVersion, saveVersion, supportedVersions }) — pre-wired RESTful /versioning resource with optimistic concurrency via {from, to} payloads
  • validateVersionUpgrade({ from, to, supported, ... }) — pure upgrade-policy helper with discriminated-union result

Per-version behavior (not just schemas)

  • createVersionedBehavior({ head, changes, initialVersion? }) — typed overlay primitive; behaviorHad: Partial<Behavior> is compile-time-checked against the head shape
  • buildBehaviorResolver(map, fallback, opts?) — low-level escape hatch for one-off maps
  • VersionChangeWithSideEffects.isApplied — boolean toggle for single-axis behavior changes

Error shaping

  • errorMapper option on Tsadwyn — translate domain exceptions → HttpError inside the handler catch block
  • exceptionMap({ ErrorClass: { status, body } }) + isExceptionMapFn — declarative, introspectable map form
  • ValidationError extends HttpError — Zod validation failures flow through errorMapper + migrateHttpErrors like any other error

Handler-level escape hatches

  • currentRequest() / currentRequestOrNull() — read the raw Express Request from inside any handler or migration callback. Auto-captured by the dispatcher — no middleware to mount.
  • requestContextStorage — underlying ALS, exported for advanced use

Generation-time diagnostics

  • detectRouteShadows(routes) + reportRouteShadows(shadows, policy, logger?) — wildcard-before-literal detector
  • onRouteShadowing: 'warn' | 'throw' | 'silent' + routeShadowingLogger on Tsadwyn
  • Generation-time lint now also catches: statusCode: 204 with body schema, body-mutating migrations on body-less responses, response migrations against raw() routes, GET/HEAD duplicates

Non-JSON response helpers

  • raw({ mimeType, supportsRanges? }) — schema marker for binary/streaming routes; marks body-migrations against this route as dead code
  • deletedResponseSchema(objectName, extraFields?) — Stripe-style {id, object, deleted: true} helper (pair with statusCode: 200 — 204 strips the body at the wire)
  • Router HEAD method support + tags: string[] in RouteOptions
  • 204/HEAD short-circuit in the dispatcher skips body migrations and wire emission

Outbound payload migration

  • migratePayloadToVersion(schemaName, payload, targetVersion, bundle) — standalone helper for webhooks and internal events (same migrations as in-flight dispatch, but invoked outside the request path)

Introspection / debugging trio

  • dumpRouteTable(app, opts?) — per-version route enumeration
  • inspectMigrationChain(app, { schemaName, clientVersion, direction }) — ordered migration chain for a schema + pin
  • simulateRoute(app, { method, path, version, body? }) — simulate a request without dispatching: matched route, candidate diagnostics, fallthrough reasons, up-migrated body preview

CLI

New subcommands on the tsadwyn binary:

  • routes --app <path> — route table with version filtering
  • migrations --app <path> --schema <name> --version <value> — migration chain for a schema+pin
  • simulate --app <path> --method <M> --path <p> [--version <v>] [--body <json>] — incident-triage tool
  • exceptions --app <path> — render the errorMapper / exceptionMap table
  • new version --date <YYYY-MM-DD> — scaffold a new VersionChange file

Docs

README grew from ~265 → ~996 lines. New sections:

  • Client pinning — explicit header / per-client default / cachedPerClientDefaultVersion high-traffic variant / stale pins rationale
  • Versioning error responses — the migrateHttpErrors default flip + errorMapper + exceptionMap interaction
  • Versioning behavior changesVersionChangeWithSideEffects, createVersionedBehavior typed overlay, buildBehaviorResolver escape hatch
  • Upgrade semantics — the /versioning resource — optional, persistence-free adapter with {from, to} optimistic-concurrency
  • Reading the raw request — currentRequest() — stripped-handler-view rationale + escape-hatch pattern
  • Route shadowing — the first-match-wins trap — detection heuristic + policy table
  • Adopting tsadwyn incrementally alongside existing Express routes
  • API Reference tables rebuilt to cover all new exports

Working end-to-end examples in examples/stripe-api.ts and examples/task-api.ts.

Test plan

  • npm run typecheck — clean
  • npm run build — dual ESM/CJS + .d.ts + .d.mts emission successful
  • npm test — 860/860 passing (56 test files). 2 supertest-related tests exhibit pre-existing intermittent flake at ~20% per-file rate under parallel forks; flake predates this branch and is documented in CLAUDE.md (root cause: supertest + Node HTTP keep-alive state; ecosystem pattern, not regression). See _gitless/testing-flakiness-research.md.
  • Full GPG-signed commit history
  • CI to be verified post-push (Node 20/22/24 matrix)
  • Manual verification of CLI subcommands against examples/stripe-api.ts

Migration notes for existing consumers

  1. migrateHttpErrors: false where needed — grep convertResponseToPreviousVersionFor call sites. If a migration should NOT apply to error bodies, add { migrateHttpErrors: false } explicitly.
  2. Null-safety on schema-scoped migrations — they may now fire on error-envelope shapes ({ detail: ... } validation errors) that don't match the schema. Null-check fields before mutating. See the defensive pattern snippet in the README.
  3. headerOnly: true — a new orthogonal flag for migrations that only mutate headers (not body). Runs on 204/304/HEAD body-less responses without NPE'ing.

🤖 Generated with Claude Code

Per-issue design specs (tsadwyn-issue-*.md), brainstorming artifacts
under docs/superpowers/, and the downstream-repo consumer-integration
tracking doc are kept in working dir but not tracked. The repo should
contain only runnable artifacts — source, tests, config, README.
Design specs have a half-life measured in weeks (the code becomes the
truth once it lands) and clutter PR review; they live adjacent to the
code but stay local. Matches how CLAUDE.md and _gitless/ are already
handled.
Encodes the contract for TsadwynOptions.errorMapper (maps domain
exceptions → HttpError so response migrations apply):
  - mapper returns HttpError → handler returns that status + body
  - mapper returns null → existing next(err) behavior
  - mapped HttpError flows through migrateHttpErrors: true migrations
  - a throwing mapper doesn't crash — tsadwyn returns 500

Paired with local design doc (not committed per repo convention).
…e ordering

Registers /widgets/:id with a UUID validator then /widgets/archived.
On main today the wildcard shadows the literal silently. Test passes
when EITHER a registration-time warning is emitted naming both routes
OR routes are auto-sorted so the literal is reachable.

Paired with local design doc (not committed per repo convention).
…elper

buildBehaviorResolver(map, fallback, opts) standardizes the per-version
behavior-flag lookup that every tsadwyn adopter hand-rolls. Tests
cover known-version lookup, unknown-version fallback, warn-once /
warn-every / silent telemetry modes, and the supportedVersions context
in warning output.

Paired with local design doc (not committed per repo convention).
…bhook migration

migratePayloadToVersion(schemaName, payload, targetVersion, bundle)
reuses convertResponseToPreviousVersionFor migrations to shape an
outbound payload for a pinned client. Tests cover shape transforms,
target==head no-op, input-payload immutability, multi-change walks,
and unknown-target throws.

Each it() declares its own VersionChange subclasses because of the
bind-once-per-bundle rule (T-1602).

Paired with local design doc (not committed per repo convention).
…sion policy

Option onUnsupportedVersion on versionPickingMiddleware with three
modes: 'reject' (400 structured body), 'fallback' (substitute default
+ warn), 'passthrough' (current default behavior). Default remains
'passthrough' for backwards compatibility.

Paired with local design doc (not committed per repo convention).
…y helper

validateVersionUpgrade(args) returns a structured decision:
  {ok: true, previous, next} | {ok: false, reason: 'unsupported' |
  'downgrade-blocked' | 'no-change'}. Defaults block downgrade and
  no-change; allowDowngrade / allowNoChange opt-outs. ISO-date lex
  compare is the default; custom comparator option for semver / etc.

Paired with local design doc (not committed per repo convention).
VersionedRouter exposes .get/.post/.put/.patch/.delete but no .head().
HEAD requests auto-mirror GET via Express, but response-body
migrations run against a body that's then discarded. Tests cover:
  - explicit .head() registration overrides auto-mirror
  - body migrations skipped on HEAD; header migrations still fire
  - migrateHttpErrors applies (status + headers; no body)
  - Content-Length matches equivalent GET
  - 405 + Allow when HEAD hits a method-only route
  - registration-time warn when .get() + .head() share a path

Paired with local design doc (not committed per repo convention).
When statusCode=204 or handler returns null/undefined:
  - no body emitted
  - body-mutating response migrations skipped (no NPE on res.body)
  - headerOnly:true migrations still fire
  - migrateHttpErrors on error paths unaffected
  - registration-time warn for body-mutating migrations against a 204 route
  - TsadwynStructureError when a 204-declared handler returns a body

Paired with local design doc (not committed per repo convention).
RouteDefinition.tags and OpenAPI operation.tags plumbing already
exists; only the registration-time RouteOptions.tags field is missing.
Tests cover: registration-time tag flow into OpenAPI output, grouping
multiple routes under a shared tag, endpoint().had({tags}) replacement
at older versions, reserved _TSADWYN prefix warning, deduplication.

Two-line change in RouteOptions + a small lint in route-generation —
highest-leverage tier-1 item for non-RESTful APIs.

Paired with local design doc (not committed per repo convention).
TsadwynOptions.preVersionPick runs middleware BEFORE
versionPickingMiddleware so the default-version resolver can read
req.user. Tests cover:
  - req.user set by hook is visible inside apiVersionDefaultValue
  - errors propagate via next(err)
  - async middleware supported
  - mutual-exclusion TsadwynStructureError when combined with
    versioningMiddleware full-override
  - composes correctly with VersionedRouter.use() per-version middleware
  - scoped to versioned dispatch (utility endpoints bypass the hook)

Paired with local design doc (not committed per repo convention).
…default helper

perClientDefaultVersion({identify, resolvePin, fallback, onStalePin,
cache, supportedVersions, logger}) returns an apiVersionDefaultValue-
compatible function. Tests cover: happy resolver chain, fallback on
null identity / null stored pin, explicit X-Api-Version override,
per-request caching via WeakMap, onStalePin: fallback/reject
behaviors, async support, error propagation.

Pairs with preVersionPick — auth typically runs first so identify
can read req.user.

Paired with local design doc (not committed per repo convention).
…wyn routes CLI

dumpRouteTable(app, {version, method, pathMatches, includePrivate})
returns every registered route per version. Tests cover: basic
enumeration, includeInSchema:false exclusion/inclusion, method +
pathMatches filtering, per-version sections when version omitted,
endpoint().existed visibility, combined filter AND semantics.

Top-priority tier-1 debugging tool. Pairs with the migration-chain
inspector and the route simulator.

Paired with local design doc (not committed per repo convention).
…spector

inspectMigrationChain(app, {schemaName, clientVersion, direction,
path?, method?, includeErrorMigrations?}) returns the ordered
migrations that would fire. Tests cover: response direction
(head→client), request direction (client→head), empty-when-no-match,
schema + path-based composition, error-migration filter, throws on
unknown schema / unknown version, entry shape
(changeClassName/kind/order).

Paired with local design doc (not committed per repo convention).
exceptionMap(config) returns an errorMapper-compatible function with
three mapping forms (function / static / static-with-transform) keyed
by err.name (not instanceof — survives module identity drift). Tests
cover: all three mapping forms, introspection API
(registeredNames/has/lookup/describe), construction validation
(duplicate-key detection via merge, non-4xx/5xx rejection),
end-to-end Tsadwyn integration including migrateHttpErrors
composition, and tsadwyn exceptions CLI subcommand (JSON/table/
markdown formats + --filter regex).

Completes the debugging introspection quartet with routes /
migrations / simulator.

Paired with local design doc (not committed per repo convention).
simulateRoute(app, {method, path, version?, headers?, body?}) answers
'is tsadwyn responsible for this request, and what would it do?'
without actually dispatching. Tests cover: matchedRoute with captured
params, fallthrough with closest-miss suggestions, candidate table
with per-candidate match reasons (method mismatch / extra segments /
shadowed wildcard), version resolution precedence (explicit > header
> default), request+response migration chains surfaced in order,
upMigratedBody preview when a legacy body is supplied,
availableAtOtherVersions diagnostic via endpoint().existed lifecycle.

Paired with local design doc (not committed per repo convention).
… perClientDefaultVersion

Three helpers + one middleware fix:

  src/version-upgrade.ts
    validateVersionUpgrade({current, target, supported, ...})
    Discriminated-union result (ok: true | {ok: false, reason}).
    iso-date (default) / semver / custom comparator.
    Closes tests/issue-validate-version-upgrade.test.ts (8/8 green).

  src/behavior-resolver.ts
    buildBehaviorResolver(map, fallback, opts?)
    Reads version from apiVersionStorage; silent fallback when no
    version is set; warn-once / warn-every / silent telemetry on
    unknown versions.
    Closes tests/issue-build-behavior-resolver.test.ts (7/7 green).

  src/per-client-default.ts
    perClientDefaultVersion({identify, resolvePin, fallback,
    onStalePin, cache, logger, supportedVersions})
    Per-request WeakMap cache; stale-pin policy (fallback/passthrough/reject).
    Closes tests/issue-per-client-default-version.test.ts (9/9 green).

  src/middleware.ts
    Wrap await apiVersionDefaultValue(req) in try/catch → next(err)
    so resolver rejections propagate to Express error handling
    instead of hanging the request.

All three exported from src/index.ts.
src/middleware.ts
  VersionPickingOptions gets onUnsupportedVersion ('reject' | 'fallback'
  | 'passthrough', default 'passthrough') and optional logger.
  - 'reject': 400 {error: 'unsupported_api_version', sent, supported}
  - 'fallback': substitute apiVersionDefaultValue + warn
  - 'passthrough': current behavior, verbatim storage
  Closes tests/issue-on-unsupported-version.test.ts (4/4 green).

src/application.ts
  TsadwynOptions.preVersionPick hook runs before versionPickingMiddleware
  for requests destined for versioned dispatch. Scoped out from utility
  endpoints (openApiUrl, docsUrl, redocUrl, changelogUrl) via path-check.
  Constructor throws TsadwynStructureError if preVersionPick and
  versioningMiddleware are both supplied (mutual exclusion).
  Closes tests/issue-pre-version-pick-hook.test.ts (7/7 green).
…s CLI

src/exception-map.ts
  exceptionMap(config) returns a function compatible with errorMapper:
    - function form: (err) => new HttpError(...)
    - static form: {status, code, message?}
    - static-with-transform: {status, code, transform: (err) => body}
  Introspection: registeredNames / has() / lookup() / describe().
  exceptionMap.merge(...) throws on overlapping keys.
  isExceptionMapFn(v) type guard for CLI detection.

src/application.ts
  TsadwynOptions.errorMapper stored on instance._errorMapper and passed
  through generateAndIncludeVersionedRouters.

src/route-generation.ts
  Handler catch block invokes errorMapper BEFORE the _isHttpLikeError
  check. If it returns an HttpError, the existing response-migration
  pipeline runs against it. If it returns null, next(err) preserves
  the original behavior. If it throws, next(err) with the ORIGINAL err
  — mapper failures never mask handler failures.

src/cli.ts
  runExceptions({app, format?, filter?}) returns {stdout, stderr,
  exitCode}. Renders table / json / markdown formats. Non-zero exit
  when the app has no errorMapper or a non-introspectable one.
   subcommand registered in createProgram.

tests/fixtures/cli-exception-map-app.ts
  Test fixture with exceptionMap-based errorMapper for CLI smoke tests.

Closes tests/issue-error-mapper.test.ts (4/4 green) + tests/issue-exception-map.test.ts (19/19 green).
…t-circuit

src/router.ts
  RouteOptions.tags registered at construction; dedup preserves order;
  warn for reserved _TSADWYN prefix.
  New VersionedRouter.head() method mirrors the other verbs and returns
  void (HEAD carries no body per HTTP spec).

src/structure/data.ts
  AlterResponseBySchemaInstruction + AlterResponseByPathInstruction both
  gain a headerOnly flag. ResponseMigrationOptions.headerOnly:boolean
  plumbed through the schema-based and path-based forms of
  convertResponseToPreviousVersionFor.

src/route-generation.ts
  - Migration short-circuit for null/undefined handler returns and HEAD:
    skip body-mutating migrations; only run headerOnly: true or
    migrateHttpErrors: true migrations.
  - HEAD on a successful response: still compute Content-Length but
    emit no body (res.end() without buffer).
  - Route reorder within each version: HEAD before GET for the same
    path so explicit HEAD handlers win over Express's HEAD→GET
    auto-mirror.
  - 405 + Allow header for HEAD requests on paths that have other
    methods but no GET and no explicit HEAD.
  - Registration-time warn when both GET and HEAD are explicitly
    registered for the same path.
  - Lint: path-based body migrations targeting 204/304 routes without
    headerOnly=true emit a warn at generation time.

tests/issue-no-content-shortcircuit.test.ts
  Per user feedback: 204+body is permissive by default (Stripe does
  this; RFC §15.3.5 is stricter than production reality). Removed the
  strict-throw assertion; test now verifies 204 status passes through
  unchanged even when the handler returns content.

Closes tests/issue-head-requests.test.ts (8/8 green) +
tests/issue-no-content-shortcircuit.test.ts (7/7 green) +
tests/issue-route-options-tags.test.ts (7/7 green).

628 existing tests remain green — no regressions.
…loadToVersion

src/migrate-payload.ts
  migratePayloadToVersion(schemaName, payload, targetVersion, versions):
  standalone helper that replays the schema-based response migrations
  registered between head and target against a head-shape payload and
  returns the reshaped result. Primary use case: outbound webhook
  dispatch that needs to honor the destination client's pinned version.
  Deep-clones input; throws when targetVersion is not in the bundle.

src/route-generation.ts
  Registration-time lint: for each pair of routes (same method) where
  an earlier wildcard-containing path would match a later literal
  sibling, emit a warn naming both paths. path-to-regexp is
  first-match-wins; the wildcard silently intercepts the literal
  unless the consumer reorders. This is the failure mode that
  precipitated the original production incident.

Closes tests/issue-wildcard-route-collision.test.ts (1/1 green) +
tests/issue-migrate-payload-to-version.test.ts (5/5 green).
…imulateRoute

src/route-table.ts
  dumpRouteTable(app, opts) enumerates registered routes per version.
  Filters by method / pathMatches (regex or substring) / includePrivate.
  Entries expose handler name, request/response schema names,
  statusCode, deprecated, tags (sans _TSADWYN internals), middleware.
  Uses per-version snapshots from Tsadwyn._versionedRoutes so
  endpoint().didntExist / .existed / .had mutations are reflected per
  version.

src/migration-chain.ts
  inspectMigrationChain(app, {schemaName, clientVersion, direction,
  path?, method?, includeErrorMigrations?}) returns ordered
  MigrationChainEntry list. Response direction walks head → client;
  request direction walks client → head. Entries mark kind
  (schema-based vs path-based), changeClassName, functionName, order.
  Throws TsadwynStructureError for unknown schema / unknown version.

src/route-simulation.ts
  simulateRoute(app, {method, path, version?, headers?, body?}) —
  synchronous SimulationResult with matchedRoute + every candidate's
  match reason + fallthrough diagnostics + request/response migration
  chain summaries + up-migrated body preview.
  Version resolution: explicit > header > apiVersionDefaultValue (when
  string) > head. Async default resolvers fall back to head so the
  function stays sync.
  In-house path matcher mirrors path-to-regexp's literal+:param
  subset; avoids pinning the consumer's path-to-regexp version.
  First-match-wins semantics preserved; later matching candidates are
  marked 'shadowed by earlier match'.

tests/issue-route-simulation.test.ts
tests/issue-route-table-dump.test.ts
  Minor test fixups: use prefixed paths with onlyExistsInOlderVersions
  (matches real stored path); replace require() with top-level import
  of endpoint.

All 15 issue test files + 31 existing test files green —
744/744 tests pass. typecheck + build clean.
…y 200+body)

Previously the body-less short-circuit called res.end() without the
buffer even when a migration populated responseInfo.body. That dropped
the body for the exact 'head returns 204, legacy wants 200+envelope'
scenario consumers most want to version.

Fix in src/route-generation.ts:
  - Body-less short-circuit (null/undefined handler return): after
    migrations run, if responseInfo.body is populated, emit it with
    the migrated statusCode + recomputed content-length. Otherwise
    keep the response body-less.
  - Normal migration path: if a migration cleared the body (head
    200+body → legacy 204+empty), emit an empty response instead of
    attempting JSON.stringify(undefined) which would throw on
    Buffer.from.

Test added in tests/issue-no-content-shortcircuit.test.ts:
  'migrations on 204 routes can add a body + change status' —
  head returns 204+empty for DELETE /users/:id; legacy version uses
  a headerOnly migration to rewrite statusCode to 200 AND populate
  res.body with a {deleted, id} envelope. Legacy client sees 200+body;
  head client still sees 204+empty.

745 tests pass (8/8 in the 204 suite, 737 elsewhere).
…n-first-call semantic

src/per-client-default.ts
  Two new optional fields on PerClientDefaultVersionOptions:
    saveVersion:       (clientId, version) => void | Promise<void>;
    pinOnFirstResolve: boolean  // default false

  When pinOnFirstResolve: true AND a client has no stored pin (null
  from resolvePin), the resolver persists the fallback via
  saveVersion(clientId, fallback) BEFORE returning it. Subsequent
  calls find the stored pin and behave like any pinned client.
  Matches Stripe's 'new accounts pin to current latest at signup'
  behavior (from the new-account perspective — the pin materializes
  on first authenticated call rather than at DB insert, but from the
  client's POV the end state is identical).

  Construction throws TsadwynStructureError when pinOnFirstResolve is
  true but saveVersion is missing.

  pinOnFirstResolve explicitly does NOT overwrite existing stored
  pins, including stale (out-of-bundle) ones — those continue to
  flow through onStalePin policy. 'Healing' stale pins is a separate
  consumer concern; keeping these orthogonal avoids surprise
  overwrites.

  saveVersion errors surface as 500 via the standard pipeline (the
  promise rejection propagates through versionPickingMiddleware's
  try/catch, which calls next(err)).

tests/issue-per-client-default-version.test.ts
  Six new tests in a 'pinOnFirstResolve' describe block covering:
    - persist fallback on first call + no extra call on second
    - does NOT auto-pin unauthenticated (identify returns null)
    - does NOT overwrite stale stored pins
    - default (pinOnFirstResolve: false) never calls saveVersion
    - pinOnFirstResolve: true without saveVersion throws at construction
    - saveVersion errors surface as 500

  15/15 total in the file (up from 9), 811 overall.

examples/stripe-api.ts
  Enables pinOnFirstResolve + saveVersion, with fallback changed
  from '2024-01-15' (initial) to SUPPORTED_VERSIONS[0] (latest).
  createVersioningRoutes fallback matches so GET /versioning reports
  the effective version consistently.

  Smoke-tested live — first-call from unpinned account auto-pins to
  latest, second GET confirms the stored pin matches. Full Stripe
  semantic reproduced end-to-end.

README.md
  perClientDefaultVersion helpers table entry expanded to mention
  pinOnFirstResolve. Client-pinning section example updated to use
  fallback: bundle.versionValues[0] + pinOnFirstResolve: true
  pattern.

811 tests, typecheck clean.
New 'Stale pins' subsection in the Client Pinning section covering:

  - Definition: pin points at a version no longer in the VersionBundle
  - Four concrete scenarios where it happens:
      1. Version retirement (team sunsets an old version)
      2. Data seed / backfill drift (imported pins, typos, wrong env)
      3. Cross-environment pin drift (staging vs prod bundle skew)
      4. Rollback (version was in the bundle, deploy reverted)
  - Why tsadwyn doesn't auto-heal (overwrite stale pins):
      - Typos are bugs not fixes — coercion hides the corruption
      - Retirement-with-coercion violates the versioning contract
      - Most operators want telemetry, not silent mutation
      - Healing is consumer policy (upgrade to next, force-pin, flag
        for manual review) — tsadwyn stays out of that write path
  - What tsadwyn provides: onStalePin 'fallback' | 'passthrough' |
    'reject' with structured warn logs via the logger option

Placed between the per-client default resolver recipe and the
/versioning resource docs — flows from 'here's how pins are stored'
→ 'here's what happens when the stored value is out of bundle'
→ 'here's the helper that manages upgrades explicitly'.
…apper interaction

Closes the last documentation gap — a dedicated 'Versioning error
responses' section in the Client Pinning chapter covers:

  - Default behavior: error responses bypass response migrations
  - Per-migration opt-in: migrateHttpErrors: true
  - Why it's opt-in (not default): stable error envelope contract
  - What to flip when your error envelope does drift (Stripe pattern)
  - errorMapper ↔ migrateHttpErrors interaction:
       domain throw → errorMapper → HttpError (head-shape body)
                    → migrateHttpErrors migrations → client-shape body
  - ValidationError flows through the same pipeline
  - headerOnly: true cousin for body-less responses (HEAD/204/304/null
    handler return)

This rounds out the three outstanding issues on the repo:

  #1 (Discoverability of path-based migrations) — addressed via
     inspectMigrationChain() programmatic API + 'tsadwyn migrations'
     CLI subcommand. Both surface the migration chain tsadwyn
     actually picked up, turning 'nothing happens' into a one-command
     check ('tsadwyn migrations --app ... --schema ... --version ...').
     The issue proposed either a decorator API OR a boot-time
     diagnostic; we shipped the diagnostic.

  #2 (errorMapper option) — shipped as specified, plus exceptionMap()
     declarative helper and 'tsadwyn exceptions' CLI for introspection.
     ValidationError subclass lets consumers key on validation errors
     specifically in their exceptionMap config. All four failing-
     test-plan items from the issue pass.

  #3 (Docs for migrateHttpErrors + errorMapper interaction) — this
     commit plus earlier changes. The API Reference tables call out
     migrateHttpErrors on ResponseMigrationOptions, and the new
     narrative section explains the full flow including the
     errorMapper interaction the issue specifically asked for.

Fixes #1
Fixes #2
Fixes #3
…ning by default

BREAKING: response migrations now fire on error responses (4xx/5xx)
by default. Previously opt-in via migrateHttpErrors: true; now opt-
out via migrateHttpErrors: false.

Rationale (per the recommendation in #3 marked as 'v2 consideration',
plus user confirmation that the one active consumer can absorb the
change): tsadwyn's stated goal is Stripe-style versioning, and
Stripe's version-pin contract applies to error envelopes too. The
prior opt-in default made it silently easy to ship error-body field
additions that leaked to legacy-pinned clients without notice.

src/structure/data.ts
  Path-based + schema-based forms of convertResponseToPreviousVersionFor
  now default migrateHttpErrors to true.

src/route-generation.ts
  Body-less + HEAD short-circuits now gate on headerOnly (the flag
  that cleanly means 'safe on undefined body'), not migrateHttpErrors
  (the flag that means 'applies to error-status responses'). Keeps
  the two concerns orthogonal:
    - migrateHttpErrors: true  → migration fires on 4xx/5xx (default)
    - migrateHttpErrors: false → migration skips 4xx/5xx (opt-out)
    - headerOnly: true         → migration fires on body-less too
    - headerOnly: false        → migration skips body-less (default)

Test updates (no behavior surprises, all 811 tests green):
  - tests/basic.test.ts: null-check the UserResource migration so it
    doesn't NPE when the 422 validation body (shape {detail: [...]})
    runs through it. Demonstrates the new defensive pattern consumers
    should apply to schema-targeted migrations.
  - tests/http-errors.test.ts + migration-coverage.test.ts +
    response-types.test.ts: the 'skips when false' / 'skips non-
    flagged migration' tests now explicitly pass
    {migrateHttpErrors: false} to preserve their success-only assertion.
  - tests/issue-head-requests.test.ts: the header-migration test
    switches from migrateHttpErrors: true to headerOnly: true —
    matches the clarified semantic (headerOnly is for body-less
    contexts).
  - tests/issue-migration-chain-inspector.test.ts: the 'success-only'
    migration explicitly opts out so the includeErrorMigrations
    filter test has something to filter against.
  - tests/data-coverage.test.ts: default-value assertions updated
    (false → true) and the it() titles renamed to match.

README.md
  'Versioning error responses' section rewritten around the new
  default. Highlights the defensive null-check pattern for schema-
  targeted migrations (since error bodies may not match the schema).

No migration guide needed — the single known consumer updates per the
summary above. Future: if a second consumer hits this, a compat mode
TsadwynOptions.errorMigrationMode: 'opt-in' | 'opt-out' can be added
without breakage.
New 'Versioning behavior changes (not just schemas)' section
addressing the half of versioning schema migrations don't cover:
same shape, different side effects, policy, or semantics.

Five concrete real-world examples:
  - auto-capture vs deferred-capture charges
  - rate-limit tier changes per version
  - cascading vs soft-delete semantics
  - idempotency-key TTL drift
  - webhook retry policy changes

Pattern 1 — VersionChangeWithSideEffects.isApplied:
  Good for boolean toggles ('v2 does X differently'). Named &
  centralized, auto-documents into the changelog, lint-grep-able.
  Static isApplied getter enforces 'at-or-newer-than' semantic
  correctly so handlers never hand-roll date comparisons.

Pattern 2 — buildBehaviorResolver(map, fallback):
  Good for per-version configurable values (rate limits, timeouts,
  retry counts, feature configs). One resolver, many callsites, zero
  scattered version-string comparisons. onUnknown telemetry mode
  surfaces typos / stale pins without breaking the request.

Decision table maps the shape of the change to the appropriate
primitive. Notes that both primitives read from the same
apiVersionStorage the dispatch pipeline writes — no extra wiring.

Placed right after 'Versioning error responses' in the Client
Pinning section since it completes the 'what a client's pin
controls' story: shape (schemas), error envelope
(migrateHttpErrors), and now behavior (isApplied /
buildBehaviorResolver).
…s Request inside dispatch

tsadwyn handlers receive a deliberately stripped view —
{ body, params, query, headers } — so handlers can't silently reach into
arbitrary parts of the Express Request. Real apps still need to read
middleware-injected state (req.user, claims, req.tenantId, trace IDs)
that isn't part of the wire contract, and the workarounds consumers
landed on (captureRequestContext as LAST middleware + a local
currentRequest() helper) were both fragile and per-consumer boilerplate.

This ships the escape hatch as a framework primitive:

- currentRequest() / currentRequestOrNull() — read the raw Request from
  inside any handler or migration callback.
- requestContextStorage — the underlying AsyncLocalStorage, exported for
  advanced use (instrumentation, tests).
- Capture happens inside the dispatch wrapper in createVersionedHandler
  right before the handler's try block, so no middleware-ordering
  discipline is required on the consumer side.

Handlers AND migration callbacks see the correct req via ALS propagation;
concurrent requests are isolated. currentRequest() throws when called
outside a dispatch scope (loud failure on misuse); currentRequestOrNull()
returns null for library-internal helpers where absence is valid.
… behavior overlay

Schema migrations cover the wire shape. The other half of API versioning
is behavior — same wire shape, different side effects or defaults.
tsadwyn already had VersionChangeWithSideEffects (on/off flags) and
buildBehaviorResolver (raw Map<version, value>), but production adopters
kept rolling their own typed-shape + per-version-overlay scaffolding.
This ships that pattern as a first-class primitive.

createVersionedBehavior({ head, changes, initialVersion? }) takes:

- head: the latest (HEAD) behavior snapshot, typed by the caller.
- changes: ReadonlyArray<VersionBehaviorChange<B>>, each declaring
  `version` (when the change took effect) and `behaviorHad: Partial<B>`
  (the pre-change value). Partial<B> is a compile-time contract — typo'd
  field names fail tsc.
- initialVersion: optional oldest-supported version key. When supplied,
  every `behaviorHad` collapses onto that key to reconstruct the
  "before any tracked change" snapshot.

Returns { get, at, map }:

- get() reads apiVersionStorage and returns the snapshot (falls back to
  `fallback` on unknown version; delegates to buildBehaviorResolver for
  the ALS read + unknown-version telemetry).
- at(version) is an explicit lookup — throws on unknown, for tests and
  admin UIs where absence is a bug.
- map is a readonly snapshot of every resolved version → behavior, for
  changelog rendering / admin introspection.

Same-version duplicates merge partials; conflicting field writes trigger
a logger.warn with last-write-wins semantics.
… high-QPS default resolution

perClientDefaultVersion calls resolvePin on every authenticated request
that doesn't send an explicit version header — fine for modest load,
dominant cost on a high-QPS API with DB-backed pin storage. Production
adopters layered their own TTL'd map + invalidation hook on top.
cachedPerClientDefaultVersion ships that pattern verbatim, with a few
correctness details baked in so consumers don't have to get them right
themselves.

Returns { resolver, invalidate, invalidateAll }:

- resolver: Promise<string>-returning function suitable for
  apiVersionDefaultValue on Tsadwyn / versionPickingMiddleware. Reads
  from the in-memory Map<clientId, entry> before consulting resolvePin.
- invalidate(clientId): drops one entry. Wire this into the upgrade
  endpoint so a client who just upgraded doesn't serve stale for
  up to ttlMs.
- invalidateAll(): nuke every entry. For rolling deploys / test
  teardowns.

Correctness details handled by the primitive:

- Single-flight on cold cache: concurrent first-misses for the same
  clientId share one resolvePin promise — the other N-1 await instead
  of firing duplicate queries.
- Error bypass: if resolvePin rejects, the cache entry is deleted on
  rejection so the next request retries fresh instead of serving
  fallback for the full TTL.
- pinOnFirstResolve interaction: when a genuinely-unpinned client hits
  the resolver and saveVersion persists the fallback, the newly-saved
  pin is cached immediately.
- ttlMs: 0 disables caching entirely (every call falls through);
  negative values throw at construction.
…ral routes

path-to-regexp resolves routes in registration order and takes the first
pattern that matches. Registering GET /users/:id before GET /users/search
silently routes /users/search to the :id handler, which then 400s with
"search is not a valid UUID" from deep inside a validator — a mystery-
bug pattern every real-world adopter eventually hits.

There was already an inline console.warn in generateVersionedRouters for
this case. This commit promotes it to a proper module with a configurable
policy, exposes the detection helpers publicly (so CLI tools and direct
callers of generateVersionedRouters can use them), and removes the old
inline check — the enclosing Tsadwyn application now runs the detector
once at initialization time before binding.

New exports:

- detectRouteShadows(routes) -> RouteShadow[]
  Pure scan; groups routes by method, returns every (earlier wildcard,
  later literal) pair on the same method. Heuristic conservatively
  treats :param(\\d+) constraints as catch-alls so we warn on rare
  edge cases rather than miss them.
- reportRouteShadows(shadows, policy, logger?)
  Applies the 'warn' | 'throw' | 'silent' policy; structured logger
  falls back to console.warn.

TsadwynOptions gains:

- onRouteShadowing: 'warn' (default) | 'throw' | 'silent'
  'warn' matches prior behavior (one log line per shadow, boot continues).
  'throw' is the CI-enforcement mode: surfaces as TsadwynStructureError.
  'silent' is for acknowledging an intentional shadow.
- routeShadowingLogger: optional structured logger.

Overlapping-wildcard cases (/a/:x then /a/:y) are deliberately ignored —
those are either Express-detected duplicates or ambiguous enough that
warning would be noise.
…hing, route shadowing

Flesh out the README for the four new primitives. Each section focuses
on WHAT, WHEN, and HOW IT WORKS rather than just listing the API shape:

- 'Versioning behavior changes' section updated to use the new
  createVersionedBehavior primitive instead of the four-file hand-rolled
  walker. Collapses ~60 lines of example into one block; buildBehaviorResolver
  is called out as the low-level escape hatch. Updated 'Which to reach for'
  table entries.
- New '### High-traffic caching — cachedPerClientDefaultVersion' section
  under Client pinning. Covers the amortized-reads story, single-flight
  cold-cache dedup, the explicit invalidation contract (and what happens
  if you forget to invalidate after an upgrade), the TTL's real job as a
  safety net for multi-pod deploys, error bypass semantics, and when NOT
  to use it (when upstream auth already resolves the pin).
- New '### Reading the raw request — currentRequest()' section explaining
  the stripped-handler-view design, the escape hatch for middleware-
  injected state, how auto-capture works without mount-order discipline,
  concurrent-request isolation, and the anti-pattern (if a value belongs
  in the versioned contract, put it on the schema).
- New '### Route shadowing — the first-match-wins trap' section with a
  concrete before/after snippet of the /users/:id then /users/search bug,
  a per-method detection heuristic explainer, and a policy table
  (warn/throw/silent) with when-to-pick-which guidance.
- API Reference tables gain entries for currentRequest /
  currentRequestOrNull, cachedPerClientDefaultVersion, createVersionedBehavior,
  and the onRouteShadowing policy option.

gitignore: add migrate-to-latest.md alongside the other local-only
spec/scratchpad artifacts (tsadwyn-issue-*.md,
consumer-integration-followups.md, docs/superpowers/).
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR expands tsadwyn’s versioning surface to include Stripe-style client pinning, error-envelope shaping, per-version behavior catalogs, and a set of introspection/debugging tools (plus a pre-wired /versioning resource), while also flipping the default for migrateHttpErrors to true.

Changes:

  • Add new primitives: per-client default version resolvers (including cached variant), per-version behavior overlays, /versioning routes + upgrade-policy helper, raw/binary response marker, and deleted-response schema helper.
  • Add introspection tooling: route-table dump, migration-chain inspection, route-shadowing detection/reporting, and request-context accessors.
  • Update middleware/error pipeline semantics (notably migrateHttpErrors default = true) and extend route registration options (tags, HEAD).

Reviewed changes

Copilot reviewed 58 out of 70 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
tests/versioned-behavior.test.ts New tests for createVersionedBehavior overlay semantics and unknown-version behavior.
tests/route-shadowing.test.ts Tests for route-shadowing detection + Tsadwyn integration policies.
tests/response-types.test.ts Adjusts tests for new migrateHttpErrors default (explicit opt-out).
tests/migration-coverage.test.ts Adjusts error-migration coverage tests for new default (explicit opt-out).
tests/issue-wildcard-route-collision.test.ts Regression test covering wildcard-before-literal collisions (shadowing).
tests/issue-validation-error-envelope.test.ts Tests ValidationError flowing through error pipeline + migrations/mapper.
tests/issue-validate-version-upgrade.test.ts Tests for validateVersionUpgrade policy helper.
tests/issue-route-table-dump.test.ts Tests for dumpRouteTable route enumeration/filtering.
tests/issue-route-options-tags.test.ts Tests for RouteOptions.tags registration + OpenAPI emission/mutation.
tests/issue-raw-response.test.ts Tests for raw() marker behavior + dead-migration warning semantics.
tests/issue-pre-version-pick-hook.test.ts Tests for preVersionPick ordering, error propagation, and composition.
tests/issue-on-unsupported-version.test.ts Tests for onUnsupportedVersion behavior in versionPickingMiddleware.
tests/issue-migration-chain-inspector.test.ts Tests for inspectMigrationChain ordering and filtering semantics.
tests/issue-migrate-payload-to-version.test.ts Tests for migratePayloadToVersion (out-of-band payload reshaping).
tests/issue-head-requests.test.ts Tests for explicit HEAD support + HEAD migration short-circuit semantics.
tests/issue-error-mapper.test.ts Tests for TsadwynOptions.errorMapper mapping domain errors to HttpError.
tests/issue-cli-introspection-subcommands.test.ts Tests for CLI runners for routes/migrations/simulate.
tests/issue-build-behavior-resolver.test.ts Tests for buildBehaviorResolver unknown-version telemetry behaviors.
tests/issue-204-body-lint.test.ts Tests generation-time lint for 204-with-body schema footgun.
tests/http-errors.test.ts Updates response-migration tests for migrateHttpErrors default flip.
tests/fixtures/cli-migrations-app.ts CLI fixture app with real migration for migrations/inspection commands.
tests/fixtures/cli-exception-map-app.ts CLI fixture app for exception-map introspection command.
tests/delete-response-helper.test.ts Tests deletedResponseSchema helper + version-migration behavior.
tests/data-coverage.test.ts Updates expectations for migrateHttpErrors default = true.
tests/current-request.test.ts Tests currentRequest()/ALS request context plumbing and isolation.
tests/cached-per-client-default.test.ts Tests TTL cache, invalidation, and single-flight per-client default resolver.
tests/basic.test.ts Defensive migration update acknowledging error-body migrations now default on.
src/versioning-routes.ts Adds createVersioningRoutes helper implementing /versioning GET/POST contract.
src/versioned-behavior.ts Adds createVersionedBehavior typed overlay builder + snapshot map.
src/version-upgrade.ts Adds validateVersionUpgrade helper with iso-date/semver/custom comparator support.
src/structure/data.ts Adds headerOnly option; flips migrateHttpErrors default to true; threads flags into instructions.
src/router.ts Adds RouteOptions.tags + tag warnings/dedup; adds explicit .head() registration.
src/route-table.ts Adds dumpRouteTable introspection helper with filtering options.
src/route-shadowing.ts Adds route-shadowing detection + reporting with warn/throw/silent policies.
src/request-context.ts Adds currentRequest()/currentRequestOrNull() and exported ALS storage.
src/raw-response.ts Adds raw() marker + detection helper for binary/streaming responses.
src/per-client-default.ts Adds perClientDefaultVersion resolver helper with caching + stale-pin policies.
src/migration-chain.ts Adds inspectMigrationChain introspector for request/response migrations.
src/migrate-payload.ts Adds migratePayloadToVersion helper for outbound payload reshaping.
src/middleware.ts Adds onUnsupportedVersion policy + error handling around default resolver.
src/index.ts Exports new primitives/helpers/introspection utilities.
src/exceptions.ts Adds ValidationError as HttpError subclass for validation failures.
src/exception-map.ts Adds exceptionMap + isExceptionMapFn for declarative/introspectable error mapping.
src/delete-response.ts Adds deletedResponseSchema helper for Stripe-style delete responses.
src/cached-per-client-default.ts Adds cached per-client resolver variant with TTL + invalidation.
src/behavior-resolver.ts Adds buildBehaviorResolver helper for per-version behavior lookup.
src/application.ts Wires preVersionPick hook, errorMapper injection, and route-shadowing policy into Tsadwyn initialization/generation.
coverage/sorter.js Removes committed coverage artifact.
coverage/prettify.css Removes committed coverage artifact.
coverage/index.html Removes committed coverage artifact.
coverage/block-navigation.js Removes committed coverage artifact.
coverage/base.css Removes committed coverage artifact.
.gitignore Adds ignores for local-only design/spec artifacts (and related docs).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/versioning-routes.ts
Comment on lines +145 to +149
throw new HttpError(409, {
error: "version_mismatch",
expected: from,
actual: effective,
});
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

In the 409 version_mismatch response, the expected/actual fields look reversed: expected is currently set to the client-sent from, and actual is set to the server’s effective stored/fallback version. Typically expected should be the server’s effective version and actual should be what the client sent, so clients can reconcile and retry correctly.

Copilot uses AI. Check for mistakes.
Comment thread src/versioning-routes.ts
Comment on lines +114 to +118
version: effective,
supported: [...opts.supportedVersions],
// supportedVersions is newest-first per tsadwyn convention, so [0] is head.
latest: opts.supportedVersions[0],
};
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

latest: opts.supportedVersions[0] assumes supportedVersions is non-empty. If a consumer passes an empty list (or misorders it), this returns undefined but the Zod schema requires a string. Consider validating supportedVersions.length > 0 up front (and/or deriving latest more defensively).

Copilot uses AI. Check for mistakes.
Comment on lines +12 to +16

// GAP: validateVersionUpgrade is not exported from tsadwyn yet.
// @ts-expect-error — intentional: drives the failing-import signal
import { validateVersionUpgrade } from "../src/index.js";

Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

This file still claims the export is missing and uses @ts-expect-error to force a failing import, but validateVersionUpgrade is now exported from src/index.ts. Please remove the stale “FAILING TEST”/“GAP” comments and the @ts-expect-error so the test reflects the current intended behavior.

Copilot uses AI. Check for mistakes.
Comment thread tests/issue-route-table-dump.test.ts Outdated
Comment on lines +21 to +26
} from "../src/index.js";

// GAP: not exported
// @ts-expect-error — intentional
import { dumpRouteTable } from "../src/index.js";

Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

dumpRouteTable is now exported from src/index.ts, but this test still includes “GAP: not exported” and an @ts-expect-error on the import. Please remove the @ts-expect-error and update the header comment (it no longer represents a failing-gap test).

Copilot uses AI. Check for mistakes.
Comment thread tests/issue-raw-response.test.ts Outdated
Comment on lines +26 to +31
} from "../src/index.js";

// GAP: not exported
// @ts-expect-error — intentional
import { raw } from "../src/index.js";

Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

raw is now exported from src/index.ts, but this test still marks the import with @ts-expect-error and describes it as a missing export. Please drop the stale “GAP”/“FAILING TEST” framing and remove the @ts-expect-error so the test doesn’t mislead future readers.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +23
import { apiVersionStorage } from "../src/index.js";
// GAP: buildBehaviorResolver is not exported from tsadwyn yet. This import
// will fail at module load until the helper ships.
// @ts-expect-error — intentional: drives the failing-import signal
import { buildBehaviorResolver } from "../src/index.js";

Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

buildBehaviorResolver is now exported from src/index.ts, so the @ts-expect-error and “GAP: … not exported” comment are stale/misleading. Please remove them and treat this as a normal behavior test.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +19
// GAP: these three runners are not exported from cli.ts yet
// @ts-expect-error — intentional
import { runRoutes, runMigrations, runSimulate } from "../src/cli.js";

Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

runRoutes/runMigrations/runSimulate are exported from src/cli.ts, but this test still labels them as missing and uses @ts-expect-error on the import. Please remove the stale “GAP” framing and the @ts-expect-error so the test reflects the current public API.

Copilot uses AI. Check for mistakes.
…onstruction

An empty `supportedVersions: []` would let `latest: supportedVersions[0]`
resolve to `undefined`, failing the VersioningState Zod schema at
dispatch time with a cryptic "expected string, got undefined" error far
from the actual misconfiguration. Validate up-front so the problem
surfaces at `createVersioningRoutes(...)` construction with a message
pointing at the fix (typically `bundle.versionValues`).

Also covers the undefined case (caller omits the option entirely) via
the same guard — required by the interface but JS callers could still
pass undefined at runtime.

Addresses Copilot review feedback on PR #4.
…ssing gap tests

These test files were originally written as failing-import drivers to
prove missing primitives before the implementation landed — "FAILING
TEST" headers + `@ts-expect-error` pragmas on the imports, explicit
"GAP: not exported" comments. The primitives have all shipped, so the
stale framing is actively misleading for future readers.

Cleanup per file:
  - Replace "FAILING TEST — verifies the gap..." headers with a clear
    "Covers <primitive>..." description of what the test actually asserts.
  - Collapse the fake-failing separate import back into the main import
    block where the other tsadwyn exports are imported.
  - Drop the @ts-expect-error pragmas + "GAP" / "failing-import signal"
    comments.

Files touched:

  Flagged by Copilot review on PR #4:
    tests/issue-build-behavior-resolver.test.ts
    tests/issue-cli-introspection-subcommands.test.ts
    tests/issue-raw-response.test.ts
    tests/issue-route-table-dump.test.ts
    tests/issue-validate-version-upgrade.test.ts

  Same stale pattern, caught by grep while we were here:
    tests/issue-exception-map.test.ts
    tests/issue-migrate-payload-to-version.test.ts
    tests/issue-migration-chain-inspector.test.ts
    tests/issue-per-client-default-version.test.ts
    tests/issue-route-simulation.test.ts
    tests/issue-versioning-resource.test.ts

Preserved intentionally:
  - tests/versioned-behavior.test.ts:186 — a legitimate `@ts-expect-error`
    on `behaviorHad: { nonExistentField: true }` asserting the
    `Partial<Behavior>` type contract at compile time. This is a real
    contract guard, not stale framing.

No behavior change. All 862 tests still pass, typecheck clean.
@mahmoudimus
Copy link
Copy Markdown
Owner Author

Copilot review — triage

Addressed in two commits on this branch:

✅ Fixed — supertest: supportedVersions: [] produced latest: undefined

15c291b (fix) — Validates up-front in createVersioningRoutes(...); an empty or missing supportedVersions now throws a clear Error at construction with a message pointing at the typical fix (bundle.versionValues). Previously the misconfiguration would surface at dispatch as a cryptic Zod "expected string, got undefined" error far from the actual cause. Added two tests covering the guard (empty array + explicitly-undefined).

✅ Fixed — stale @ts-expect-error / "GAP" framing across 11 test files

e894bd8 (chore/tests) — Copilot flagged 5 files with stale @ts-expect-error pragmas on imports that now work. While we were in there I grepped for the same pattern across all tests and found 6 more with the identical stale framing; cleaned them all up for consistency. Each file now has an accurate "Covers <primitive> ..." header instead of the original "FAILING TEST — verifies the gap..." language, and the fake-failing import block has been folded back into the main import.

Preserved intentionally: tests/versioned-behavior.test.ts:186 has a legitimate @ts-expect-error that asserts a compile-time contract on behaviorHad: Partial<Behavior>. That one's a real type guard, not stale framing.

❌ Not changed — 409 response expected / actual field naming

Copilot suggested swapping the fields in the 409 version_mismatch response body — claiming expected should be the server's effective version and actual should be the client-sent from.

I looked at this carefully and it's not a bug, it's a convention choice. The current semantic is client-perspective naming: "I (the client) expected the current state to be X (my last-known); actually it's Y (server's truth)." That's explicitly documented in README line 529 with a concrete example and asserted by two tests in issue-versioning-resource.test.ts at lines 158–159 and 322–323 — the second has an explicit // effective, not null comment on actual, showing the semantic was deliberate, not accidental.

Both namings (client-perspective vs server-perspective) are defensible. Swapping now would:

  1. Invert the documented contract in the README
  2. Break two existing tests
  3. Be a silent breaking change for any consumer parsing these fields

Happy to revisit in a follow-up if there's a strong preference for server-perspective naming, but that'd be a deliberate v0.2.0 contract change, not a bug fix. Keeping the current shape here.


Result: npm run typecheck clean, npm test passing 862/862 (up from 860 — added 2 tests for the new guard). All commits GPG-signed with the same key as the rest of the branch.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 58 out of 70 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/versioning-routes.ts
Comment on lines +158 to +163
throw new HttpError(409, {
error: "version_mismatch",
expected: from,
actual: effective,
});
}
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

In the 409 "version_mismatch" response, the expected and actual fields appear swapped: expected is currently set to the client-sent from, while actual is set to the server-side effective version. For optimistic concurrency, expected should reflect what the server expected (the effective version), and actual should reflect what the client sent, so callers can diagnose and retry correctly.

Copilot uses AI. Check for mistakes.
Comment thread src/behavior-resolver.ts Outdated
* - 'warn-every' — warn on every unknown lookup.
*/
onUnknown?: "silent" | "warn-once" | "warn-every";
/** Optional structured logger. Required if `onUnknown !== 'silent'`. */
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

The BuildBehaviorResolverOptions.logger docstring says the logger is "Required if onUnknown !== 'silent'", but the implementation treats it as optional (it simply skips warnings when logger is absent). Either enforce the requirement (throw when onUnknown is warn-* and no logger is provided) or update the documentation to match the actual behavior.

Suggested change
/** Optional structured logger. Required if `onUnknown !== 'silent'`. */
/**
* Optional structured logger. When provided and `onUnknown !== 'silent'`,
* unknown-version lookups may emit warnings; otherwise warnings are skipped.
*/

Copilot uses AI. Check for mistakes.
Comment thread src/versioned-behavior.ts
Comment on lines +73 to +82
/**
* Fallback when an unknown version is active at `.get()` time. Default: `head`.
*/
fallback?: B;
/** Telemetry policy for unknown-version lookups via `.get()`. */
onUnknown?: "silent" | "warn-once" | "warn-every";
/** Optional structured logger. Required when `onUnknown !== 'silent'`. */
logger?: {
warn: (ctx: Record<string, unknown>, msg: string) => void;
};
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

CreateVersionedBehaviorOptions.logger is documented as required when onUnknown !== 'silent', but createVersionedBehavior does not enforce this (it passes the optional logger through and warnings become a no-op). Consider either validating and throwing when onUnknown is warn-* without a logger, or relaxing the docstring to say warnings are only emitted when a logger is supplied.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +22
/**
* FAILING TEST — verifies the gap described in tsadwyn-issue-error-mapper.md
*
* Today, when a handler throws a domain exception that doesn't carry a
* `statusCode` property, tsadwyn's `_isHttpLikeError()` check fails and the
* error escapes via `next(err)` to Express's default error handler. That
* bypasses the response-migration pipeline entirely and forces consumers to
* couple their domain layer to tsadwyn's internal detection.
*
* The proposed fix adds an `errorMapper` option on `TsadwynOptions` — a pure
* function `(err: unknown) => HttpError | null` invoked inside the handler's
* catch block before `_isHttpLikeError`. When it returns an `HttpError`, the
* existing migration / status / header machinery picks up. When it returns
* `null`, current behavior (`next(err)`) is preserved.
*
* These tests will turn green when:
* 1. `TsadwynOptions.errorMapper` is accepted at construction
* 2. The mapper runs in the catch block before the HTTP-likeness check
* 3. Mapped HttpError flows through `migrateHttpErrors: true` migrations
* 4. A throwing mapper does not crash the response — tsadwyn returns 500
*
* Run: npx vitest run tests/issue-error-mapper.test.ts
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

This file header still says "FAILING TEST" / "will turn green when…", but the PR implements errorMapper and the overall test plan claims the suite is passing. Consider updating this header to reflect that it's now a regression/coverage test (or remove the outdated wording) to avoid confusion for future contributors.

Copilot uses AI. Check for mistakes.
…warn-*

The docstring on `BuildBehaviorResolverOptions.logger` has always said
the logger is "Required if onUnknown !== 'silent'" but the implementation
silently no-op'd warnings when the logger was missing. That's the worst
kind of bug — a user opts into telemetry (sets `onUnknown: 'warn-once'`),
forgets to pass a logger, and never finds out why their warnings don't
appear. Fail loudly at construction instead.

Throws `TsadwynStructureError` with an actionable message when
`onUnknown` is `'warn-once'` or `'warn-every'` and `logger` is absent.
`onUnknown: 'silent'` (the default) requires nothing — the behavior
there is unchanged.

`createVersionedBehavior` delegates telemetry options through to
`buildBehaviorResolver`, so the guard flows through without additional
code. Added delegation-locking tests in `tests/versioned-behavior.test.ts`
so a future refactor can't accidentally reintroduce the silent-no-op
footgun by changing the delegation.

TDD-verified by reverting the guard and running the 4 new enforcement
tests — all 4 fail as expected. With the guard restored, all 869 tests
pass. Addresses Copilot comments on src/behavior-resolver.ts:17 and
src/versioned-behavior.ts:82.
… coverage docs

Follow-up to e894bd8, which caught all the test files with the stale
@ts-expect-error pragma but missed 8 more that only had the narrative
"FAILING TEST — verifies the gap..." header language (no @ts-expect-error
to grep on). Copilot's second review flagged one of them
(issue-error-mapper.test.ts); grep-ing for "FAILING TEST" found the
other 7 while we were in there.

Files touched:

  Flagged by Copilot second review:
    tests/issue-error-mapper.test.ts

  Same stale pattern, caught by grep:
    tests/issue-204-body-lint.test.ts
    tests/issue-head-requests.test.ts
    tests/issue-no-content-shortcircuit.test.ts
    tests/issue-on-unsupported-version.test.ts
    tests/issue-pre-version-pick-hook.test.ts
    tests/issue-route-options-tags.test.ts
    tests/issue-wildcard-route-collision.test.ts

Each file's header rewritten to describe what the test actually asserts
("Covers <primitive> ...") with an Invariants / Under test list where
useful. Removed "will turn green when..." / "These tests turn green
when..." wording since the primitives all shipped. Dropped the inline
`(router.ts:188-246)` source-line pointers that rot with refactors.

No behavior change. All 869 tests still pass, typecheck clean.
@mahmoudimus
Copy link
Copy Markdown
Owner Author

Copilot review — second round triage

✅ Fixed — logger required when onUnknown !== 'silent'

dd464fd (fix) — Copilot caught a real docstring/code mismatch on BuildBehaviorResolverOptions.logger. The docs claimed the logger was "Required if `onUnknown !== 'silent'`" but the implementation silently no-op'd warnings when it was missing. That's the worst kind of bug: user opts into telemetry (onUnknown: 'warn-once'), forgets the logger, never sees a warning, never knows why.

Now throws TsadwynStructureError at construction with an actionable message. Same guard flows transitively into createVersionedBehavior since it delegates telemetry options through. Added 4 new tests locking in the enforcement on both APIs — TDD-verified by reverting the guard and confirming all 4 fail (the non-throwing happy-path tests still pass in both states, which proves the guard is the specific behavior being tested).

✅ Fixed — stale "FAILING TEST" header on issue-error-mapper.test.ts

3ed1f66 (chore/tests) — same stale-header pattern I cleaned in e894bd8, but without the @ts-expect-error pragma my earlier grep was keyed on. grep -l "FAILING TEST" tests/ found 7 more files with the same issue; cleaned all 8 in one sweep.

❌ Not changed — 409 expected / actual swap (second time flagged)

This is the same suggestion from the first review pass. My reply there still applies:

  • Current semantic is client-perspective naming: "I expected the current state to be X; actually it's Y."
  • Documented in README line 529 with a concrete example.
  • Asserted by 2 tests in issue-versioning-resource.test.ts, one with an explicit // effective, not null comment proving the choice was deliberate.
  • Both namings (client-perspective vs server-perspective) are defensible. Swapping now would break the documented contract + invert the tests + be a silent breaking change for any consumer parsing these fields.

Leaving unchanged. Happy to revisit in a follow-up v0.2.0 if there's a strong preference for server-perspective naming, but it's a deliberate API choice, not a bug.


Result: npm run typecheck clean, npm test passing 869/869 (up from 862 — +4 logger-enforcement tests and +3 other delegation-locking tests). All commits GPG-signed.

…g as unhandled

TDD cycle addressing review finding #1.

Test-first: tests/reviewer-findings.test.ts::Finding #1 registers a
process-level unhandledRejection listener, constructs a Tsadwyn with an
async onStartup that throws, and asserts no unhandled rejection is
captured. Before this fix, the test fails (the rejection escapes because
`this._onStartup()` at application.ts:666 was called without .catch,
discarding the returned Promise). On Node 20+ this would terminate the
process by default.

Fix: wrap the call in Promise.resolve().then(hook).catch(log). Matches
the existing handling pattern on the sibling onShutdown hook at
application.ts:434.

onStartup remains fire-and-forget for the caller — we don't await it
inside _performInitialization (which is still sync) because adding async
would ripple through every call site. The .catch guarantees the
exception is observable via console.error instead of crashing.
…ts.methods

TDD cycle addressing review finding #2.

Test-first: tests/reviewer-findings.test.ts::Finding #2 registered a
path-based response migration (`convertResponseToPreviousVersionFor(path,
methods)`) and called `migratePayloadToVersion` for the corresponding
schema. Before this fix, the payload returned unchanged — the helper
only iterated `_alterResponseBySchemaInstructions` and silently skipped
the path-based bucket entirely. Outbound webhooks dispatched from
background jobs could deliver unmigrated payloads to older-pinned
clients with no error and no log.

Fix: extend the signature with an optional fifth parameter
`MigratePayloadOptions { path?, methods? }`. When `opts.path` is
supplied, the function also iterates `_alterResponseByPathInstructions`
keyed on that path, applying each matching transformer. `opts.methods`
(optional) restricts to a method subset; omitted means "any method
registered at this path."

Why opt-in instead of automatic? The function has no access to the
route table — it only receives the `VersionBundle`. Path-based
instructions are keyed on path, not schema name, so there's no way to
know which path the raw payload would have originated from unless the
caller tells us. Forcing opt-in makes the contract explicit:
schema-based migrations fire on schemaName alone; path-based migrations
require the caller to name the path.

When `opts.path` is omitted, behavior is unchanged from HEAD — pure
schema-based dispatch. The 3rd test asserts the `methods` filter
actually excludes non-matching methods, so a GET-registered path-based
migration doesn't accidentally apply to a POST payload delivered under
the same path.

Docstring updated to describe both forms and the opt-in semantic.
…ring responses

TDD cycle addressing review finding #3.

Test-first: tests/reviewer-findings.test.ts::Finding #3 wraps res.end
via a preVersionPick hook and records every call's arguments. On HEAD
requests to a Buffer-returning handler, the pre-fix code called
res.end(buffer) — the test captures that and asserts args[0] is either
undefined or an empty buffer. Without the fix, args[0] is the actual
payload Buffer.

Why the bug matters (and why it's classified MEDIUM not HIGH): Node's
HTTP writer strips bodies from HEAD responses at the wire level per
RFC 7231 §4.3.2, so clients never actually receive the leaked bytes.
But the app CODE was writing them into the socket knowing they'd be
discarded — wasted work on large Buffers, and the bytes still pass
through any logging / tracing / observability middleware that wraps
res.end before the wire.

Fix: compute `isHead = req.method === "HEAD"` once at the top of the
dispatch body (moved up from line 1297 to before the non-JSON branch at
line 1240), and thread it into `sendNonJsonResponse` as a new parameter.
The helper now calls `res.end()` with no argument on HEAD for Buffer
and string responses, and skips `.pipe()` entirely for streams. Headers
(content-type, content-length) are still set so HEAD probes carry the
metadata.

Also patched the JSON-via-string-parse branch at line 1271 to use
`isHead ? undefined : jsonBody`, matching the JSON object path that
already had the guard.

Updated the plain-string HEAD test case to assert the same internal
behavior (res.end not called with body) rather than relying on Node's
wire-level stripping masking the issue.
… on TsadwynOptions

TDD cycle addressing review finding #4.

Test-first: two cases in tests/reviewer-findings.test.ts::Finding #4.
One sets onUnsupportedVersion: 'reject' and asserts 400 with structured
body; the other sets 'fallback' and asserts the configured logger's warn
is called. Before this fix, both failed at runtime — the option was
declared on VersionPickingOptions (src/middleware.ts) but never forwarded
into the pickingOpts object at application.ts:418-423, so consumers
using the Tsadwyn class had no way to reach the policy without
overriding the entire versioningMiddleware.

Fix: add `onUnsupportedVersion` + `versionPickingLogger` to
TsadwynOptions; store them on the instance as `_onUnsupportedVersion` /
`_versionPickingLogger`; forward into `pickingOpts` when present
(leaving passthrough as the historical default when omitted).

Verified with the revert-fix-rerun cycle:
  - With the forwarding removed, both tests fail (422 instead of 400 on
    'reject'; logger.warn never called on 'fallback').
  - With the forwarding restored, both pass.

No type cast needed in the test now that the option is on TsadwynOptions.
…n instances

TDD cycle addressing review finding #5.

Test-first: tests/reviewer-findings.test.ts::Finding #5 constructs 15
Tsadwyn instances each with an onShutdown hook and asserts the SIGTERM
listener-count delta stays ≤ 1. Before this fix, each instance called
`process.on("SIGTERM", ...)` + `process.on("SIGINT", ...)`, so 15
instances added 15 listeners per signal. Node's default
maxListeners=10 triggers MaxListenersExceededWarning at ~11, and more
subtly, a real SIGTERM would invoke all 15 handlers in parallel and each
would race to call process.exit — masking failures in test suites.

Fix: module-scoped `_tsadwynActiveInstances` Set + a single signal
handler installed exactly once via `_installTsadwynSignalHandlerOnce`.
On signal, the shared handler drains every registered instance's
onShutdown in parallel via Promise.allSettled, then calls process.exit.
Exit code is 1 if any instance's shutdown rejected, 0 otherwise.

Also adds a public `Tsadwyn#close()` method so tests (and consumers that
hot-swap instances) can unregister from the shared set. Idempotent —
calling it twice is a safe no-op.

Verified with revert-fix-rerun:
  - With per-instance process.on: 15-instance test shows 15 listeners
    added → test fails.
  - With module-scoped handler: 1 listener added total → test passes.

Second test locks in that close() is safe to call multiple times.
…orage.run

TDD cycle addressing review finding #7.

Test-first: tests/reviewer-findings.test.ts::Finding #7 registers an
unversioned route via `app.unversionedRouter.get(...)` whose handler
calls `currentRequest()` to read `req.user` injected by preVersionPick.
Before this fix, the test returned 500 because `_wrapHandlerWithOverrides`
invoked the handler directly — no AsyncLocalStorage scope, so
`currentRequest()` threw "called outside a tsadwyn handler scope".

Fix: match the versioned dispatcher's pattern. `_wrapHandlerWithOverrides`
now returns a sync outer wrapper that calls `requestContextStorage.run(req,
...)` before the async body; the async body was extracted into
`_dispatchUnversionedHandler` and does the same try/catch + handler
invocation as before. Functionally identical except that
`currentRequest()` now resolves to the live req inside unversioned
handlers.

Verified with revert-fix-rerun:
  - Without the run wrap, the test fails with 500 (throw from
    currentRequest).
  - With the wrap, 200 + the expected { user: "unversioned_user" } body.

No regression in the 880 pre-existing tests — the public behavior of
unversioned routes (dispatch, error handling, dependencyOverrides) is
preserved; the ALS wrap only adds capability.
TDD cycle addressing review finding #6.

Test-first: tests/reviewer-findings.test.ts::Finding #6 creates a named
schema via `.named("Finding6_MySchema")` (which populates both the
WeakMap registry and the legacy `._tsadwynName` property), then deletes
the legacy property to simulate a downstream transform (serializer,
clone, wrapper) that dropped it. The test asserts the schema still
appears in the OpenAPI `components.schemas` output — which requires
every schema-name read to fall back to the WeakMap.

Before this fix, the test failed because 4 call sites read
`._tsadwynName` directly instead of going through `getSchemaName()`:

- src/application.ts::_buildRegistryFromRoutes — builds the registry
  used for versioned OpenAPI generation (original reviewer target)
- src/schema-generation.ts::ZodSchemaRegistry#getVersioned — looks up
  the versioned copy of an original schema during routing
- src/schema-generation.ts::transformSchemaReferences — rewrites schema
  references across a tree during versioned schema generation
- src/openapi.ts::getSchemaName (local shadow) — used throughout
  OpenAPI builder for deciding when to inline vs $ref a schema
- src/structure/enums.ts::enum_ — reads the enum's registered name

All five now delegate to the canonical `getSchemaName` export from
zod-extend.ts, which checks the WeakMap first and falls back to the
legacy property. The WeakMap-first path is the documented
invariant per CLAUDE.md: "Use `getSchemaName` / `setSchemaName` rather
than reading `._tsadwynName` directly."

Post-fix: 882/882 tests pass, typecheck clean. The legacy `._tsadwynName`
property is still written by setSchemaName for backward compat; nothing
removed from the public API.
…ng interaction explicitly

Addressing review finding #8, which was classified as "missing test
assertion rather than code defect." The existing test at
tests/reviewer-findings.test.ts::Finding #8 (landed in commit 0358f18)
already locks in the per-call retry contract — 3 sequential calls for
a stale-pinned client produce 3 resolvePin invocations.

This commit adds the other half the reviewer asked for: explicit docs
so a future reader understands the behavior is intentional.

1. Expanded docstring on `onStalePin` in CachedPerClientDefaultVersionOptions:
   spells out that the 'fallback' / 'passthrough' resolutions get
   cached, while 'reject' throws and every subsequent request re-hits
   resolvePin (no back-off, no negative caching). Also notes the
   workaround (pick a different stale policy or narrow
   supportedVersions) so a surprised reader knows where to go.

2. Inline comment at getOrCreate's rejection branch warning against
   caching rejections as an "optimization" — cross-references the
   lock-in test so someone reaching for that change can see why it'd
   break the contract.

No behavior change — purely clarifying a promise the code already kept.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discoverability: path-based migrations register silently via arbitrary instance-property names

2 participants