suaveplan/openspec/archive/2026-04-27-validation-critical-bugfix/proposal.md

# OpenSpec Change: validation-critical-bugfix

- **Change ID**: validation-critical-bugfix
- **Package**: `@suaveplan/validation`
- **Status**: PROPOSED
- **Date**: 2026-04-27

## Summary

Fix three correctness gaps in `@suaveplan/validation` and bring the spec into
conformance:

1. **Critical security footgun** — `parse()` / `safeParse()` previously
   silently skipped async refinements. A schema such as
   `string().refine(async (token) => await checkRevoked(token))` consumed via
   `.parse()` would return `valid` for revoked tokens. Synchronous parse paths
   now refuse to run when any refinement is async, surfacing
   `VALIDATION.ASYNC_REFINEMENT_REQUIRES_PARSEASYNC` instead.
2. **Ghost API (VL-10)** — `BaseSchema` now implements `.withHooks(hooks)`
   with `beforeValidate` / `afterValidate` / `onError` invocation at every
   terminal branch of `parse` and `parseAsync`. Hooks are awaited on async
   paths and their return value is discarded on sync paths.
3. **Ghost OTel emission (VL-11)** — `recordParseSuccess` /
   `recordParseFailure` and the duration histogram are now emitted from
   `BaseSchema.parse` / `BaseSchema.parseAsync` on every terminal branch,
   tagged with `schema.type = this.constructor.name`.

The change also closes one VL-09 gap (`coerceString` now returns `null` for
non-coercible `symbol` inputs so the wrapper can produce a `ParseFailure`
rather than yielding the misleading `"Symbol(x)"` token), folds the existing
`validation-spec-conformance` work into a single shipping change, and adds
`**Verified by:**` lines to every requirement in
`openspec/specs/validation/spec.md`.

## Current state

`@suaveplan/validation` ships 12 schema types, an immutable builder pattern,
and 100% line coverage at baseline. Three documented MUST-clauses had drifted
from the implementation:

- **Async refinement footgun.** `BaseSchema.parse` called
  `_applyRefinementsSync` which contained `if (r.async) continue`. The async
  predicate's promise was discarded and the value was reported `valid`
  regardless of the predicate's eventual result. The original suite even
  codified this as expected behaviour in `schema.test.ts::"sync parse skips
  async refinements without error"` — flipping that test is part of this
  change.
- **VL-10 hooks unwired.** `ValidationHookMap` and `ValidationHookData` were
  exported as types, but no `BaseSchema.withHooks` method existed and no
  parse path invoked any hook.
- **VL-11 OTel unwired.** `recordParseSuccess` / `recordParseFailure` lived
  in `src/otel/validation-otel.ts` but no `src/` call site ever invoked
  them; only the unit test for the otel module exercised the recorders.
- **VL-09 coerceString.** Returned a string for every input including
  `symbol` (where `String()` throws), forcing the wrapper into a
  `_inner._parse` round-trip that could never succeed and yielding misleading
  `"Symbol(x)"` text. The spec mandates failed coercion produce
  `ParseFailure`.
- **Spec annotations.** All 12 requirements lacked `**Verified by:**` lines
  required by `bun scripts/openspec-conformance.ts`.

## What changes

1. **schema.ts.**
   - Add `_assertNoAsyncRefinementsForSyncParse` that throws a
     `ValidationError` with the new code
     `VALIDATION.ASYNC_REFINEMENT_REQUIRES_PARSEASYNC` when invoked from
     `parse()`. `parseAsync()` is unchanged.
   - Add `withHooks(hooks)` builder that returns an immutable clone with
     hooks merged onto pre-existing hooks (per-key replace semantics).
   - Invoke hooks in `parse` / `parseAsync` at `beforeValidate` /
     `afterValidate` / `onError` boundaries.
   - Wire `recordParseSuccess` / `recordParseFailure` (with duration via
     `performance.now()`) at every terminal branch of `parse` / `parseAsync`.
   - Re-implement `safeParse` as a `try { return parse(input) } catch` shim
     so OTel + hooks fire on the safe path too.
   - Add a `_phantomInput: TInput` declared field so `InferInput<S>` resolves
     to the schema's actual input type instead of degrading to `unknown`.
2. **errors.ts / common.ts.** Register the new
   `VALIDATION.ASYNC_REFINEMENT_REQUIRES_PARSEASYNC` code in both the runtime
   `defineCodes` registry and the `ValidationCode` string-union type.
3. **utils/coerce.ts.** `coerceString` now returns `string | null`; returns
   `null` for `symbol` inputs.
4. **Tests.** Add scenarios in `schema.test.ts` (sync-throws,
   safeParse-fails-cleanly) and one in `coerce.test.ts` (symbol → null).
   Existing async-refinement-skips test is replaced.
5. **Spec.** Add `**Verified by:**` lines to every VL-NN requirement and add
   one new requirement (VL-13) for the async-refinement loud-fail behaviour.

## Why now

The three drift items above are the highest-severity gaps in the package: the
async-refinement footgun is a security-grade silent-failure bug in the core
parse path, and the unwired VL-10 / VL-11 mean any consumer reading the spec
gets a fictional picture of the API. Bundling them into a single shipping
change is cheaper than three sequential rounds of the 5-gate.

## Impact

- Public API gains `BaseSchema.withHooks(hooks)` (additive) and the
  `VALIDATION.ASYNC_REFINEMENT_REQUIRES_PARSEASYNC` error code (additive).
  `coerceString` return type narrows from `string` to `string | null`
  (callers must now handle `null`; the only in-repo caller is
  `coerce()`'s wrapper, which already guards on the symbol branch).
- `parse()` / `safeParse()` start surfacing failures for schemas that
  previously silently accepted invalid values via async refinements. This is
  the intended correctness fix; downstream code that relied on the bug must
  switch to `parseAsync` / `safeParseAsync`.
- One new spec requirement (VL-13). All eleven existing requirements
  unchanged in wording; only `**Verified by:**` lines are added.
- Coverage stays at 100 / 100 / 100 / 100.

## Deferred

The following Zod-API surface items are out of scope and tracked here so
follow-up work can pick them up:

- `discriminatedUnion(key, schemas)` — typed-tagged-union short-circuit.
- `lazy(() => schema)` / recursive schemas via thunks.
- `brand<TBrand>()` — nominal types.
- `default(value)` — fall back to a value when input is `undefined`.
- `catch(value)` — fall back to a value on parse failure.
- `superRefine((value, ctx) => ...)` — multi-issue refinement with explicit
  `ctx.addIssue`.

These are additive features; the current schema engine is internally
expressive enough to host them once a follow-up change formalises them.