From 6bf1c548e9f62f58cb862ec517e352854e58ab77 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 14:35:37 -0700 Subject: [PATCH 01/10] fix(cross): revisions - drop undocumented specCount from GET /revisions/{id} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getPackageRevision built its RevisionWithTrees result as `{ ...mapSummary(...), specs }`, spreading mapSummary's full RevisionSummary shape (specCount included) and appending specs. TypeScript's excess-property check never fires on a spread's inferred return type, so every real GET /revisions/{id} response silently carried an undocumented specCount field. Invisible until issue #649 fixed the self-referential-schema stack overflow that had excluded this operation from response-schema validation entirely — the new exact-match check caught it immediately. Extracts the shared mapRevisionCore() so each caller builds the specific shape its return type promises (RevisionSummary keeps specCount via mapSummary(); RevisionWithTrees no longer carries it) instead of a superset of it. Co-Authored-By: Claude Sonnet 5 --- src/db/queries/revisions.ts | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/db/queries/revisions.ts b/src/db/queries/revisions.ts index f89948b4..1a5816a3 100644 --- a/src/db/queries/revisions.ts +++ b/src/db/queries/revisions.ts @@ -175,11 +175,19 @@ function readinessInputFrom(input: string | CreatePackageRevisionInput): Readine return { mode: input.mode, overrideReadinessGate: input.overrideReadinessGate }; } -export function mapSummary( +// The fields RevisionSummary and RevisionWithTrees share — everything except RevisionSummary's +// `specCount` and RevisionWithTrees's `specs`. #649: getPackageRevision used to build its +// RevisionWithTrees result as `{ ...mapSummary(...), specs }`, which spreads mapSummary's FULL +// RevisionSummary (specCount included) and appends `specs` — TypeScript's excess-property check +// never fires on a spread's inferred return type, so `specCount` silently rode along on every real +// GET /revisions/{id} response despite openapi.yaml never documenting it on RevisionWithTrees. The +// gap was invisible because that op had no response-schema validation at all until #649 fixed the +// self-referential-schema stack overflow that had excluded it. Extracting the shared core here +// makes each caller build the SPECIFIC shape its return type promises, not a superset of it. +function mapRevisionCore( row: RevisionRow, - profile: RevisionNomenclatureProfile | null, - specCount: number -): RevisionSummary { + profile: RevisionNomenclatureProfile | null +): Omit { const date = revisionDateString(row.revision_date); const attributes = parseAttributes(row.attributes); const display = getRevisionDisplayIdentity( @@ -200,12 +208,19 @@ export function mapSummary( number: display.number, attributes, issuedAt: row.issued_at.toISOString(), - specCount, parentRevisionId: row.parent_revision_id, baseRevisionId: row.base_revision_id, }; } +export function mapSummary( + row: RevisionRow, + profile: RevisionNomenclatureProfile | null, + specCount: number +): RevisionSummary { + return { ...mapRevisionCore(row, profile), specCount }; +} + async function profileForPackage( packageId: string, db: Queryable @@ -347,7 +362,7 @@ export async function getPackageRevision( position: snap.position, tree: validateTree(snap.tree, snap.spec_id), })); - return { ...mapSummary(row, profile, specs.length), specs }; + return { ...mapRevisionCore(row, profile), specs }; } catch (err) { if (err instanceof DatabaseError) throw err; throw new DatabaseError(`getPackageRevision: query failed for ${revisionId}`, { cause: err }); From 790b50b7b3d76da4995e42078d0093ce39acbbcb Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 14:36:02 -0700 Subject: [PATCH 02/10] fix(contract): bundle openapi.yaml instead of dereferencing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadSpec() used $RefParser.dereference(), which resolves every $ref into its literal target. For a self-referential component (SpecNode: children: SpecNode[]) that builds a real circular JS object, and ajv's compile-time schema traversal has no cycle guard — RangeError: maximum call stack size exceeded. Six operations whose success response embeds SpecNode or SpecTree were excluded from response-schema validation entirely as a result (GET /specs/{id}, POST .../paragraphs, PATCH .../paragraphs/{nodeId}, .../removal, .../reject, GET /revisions/{id}). Switches to $RefParser.bundle(), which preserves $ref pointers instead of inlining them (openapi.yaml has zero external-file refs, so bundle's output is structurally identical to the existing un-dereferenced loadRawSpec()). ajv then resolves $ref lazily at validate time instead of the walker eagerly materializing a circular object. This changes what every consumer of loadSpec()'s OpenApiDoc sees: $refs that used to be fully inlined objects are now literal `{ $ref }` pointers (response-level refs to components/responses/*, parameter refs to components/parameters/*, request-body refs to components/schemas/*). `resolveIfRef()` (schema-refs.ts) resolves one level of local $ref for any caller that needs the actual shape rather than just a validator; operationParamKeys()'s request-body/parameter reading now goes through it, or its INV-4 vacuity guard would false-positive on `post /packages/{id}/revisions` and any $ref-bodied write op. Design decisions (no ADR per sprint policy — recorded here and at the qualifyRef/markObject $ref-branch call sites): - getValidator() (assertResponse / INV-5 conformance path) registers the whole bundled doc once under one ajv $id and rewrites each response schema's $ref strings to point into it (qualifyRefs in schema-refs.ts). Never inlines a $ref target — that would reproduce the exact circular shape bundling exists to avoid. - assertResponseExact() (INV-6 exact-key-match) cannot reuse that approach: marking a component once, standalone, with unevaluatedProperties:false is wrong whenever the SAME component is referenced from both an in-place applicator (allOf/oneOf/anyOf branch) and a child position (properties/items) somewhere in openapi.yaml — confirmed for 12 real components including SuccessResponse and ErrorResponse. Marking SuccessResponse standalone made it reject the sibling `data` branch's own keys: a false rejection of a fully documented payload, not just a missed detection. Fixed by registering TWO component mirrors (CHILD_MIRROR_ID, IN_PLACE_MIRROR_ID) built eagerly for every component name, with nested $refs qualified by the LOCAL walking context they're found in (unevaluated-properties.ts's markObject, via the new optional RefQualifyOptions on markUnevaluatedPropertiesFalse). No dependency worklist needed — ajv resolves $ref lazily by id, so a mirror entry never needs its own dependencies pre-built. - OpenApiDocSchema was silently stripping `components` via Zod's default object behavior — harmless under dereference (nothing read it), but load-bearing now: ajv needs doc.components.schemas present in the registered document for any #/components/schemas/X pointer to resolve. Widened to retain components.{schemas,responses,parameters}. Co-Authored-By: Claude Sonnet 5 --- src/test-utils/contract/schema-refs.ts | 102 ++++++++++++++ .../contract/unevaluated-properties.ts | 124 ++++++++++++++---- src/test-utils/contract/validate-response.ts | 117 +++++++++++++---- 3 files changed, 293 insertions(+), 50 deletions(-) create mode 100644 src/test-utils/contract/schema-refs.ts diff --git a/src/test-utils/contract/schema-refs.ts b/src/test-utils/contract/schema-refs.ts new file mode 100644 index 00000000..fa04b6d6 --- /dev/null +++ b/src/test-utils/contract/schema-refs.ts @@ -0,0 +1,102 @@ +// src/test-utils/contract/schema-refs.ts +// +// #649 — validate-response.ts switched loadSpec() from $RefParser.dereference to .bundle so ajv +// can compile the self-referential SpecNode/SpecTree response schemas (dereference materializes a +// real circular JS object for them, which blows ajv's compile-time traversal stack). Bundling keeps +// every `$ref` as a literal `{ $ref: '#/...' }` pointer instead of inlining it, which shifts the +// work here: registering documents with ajv under stable ids so those pointers resolve at +// compile/validate time, and giving callers a way to resolve one level of `$ref` themselves when +// they need to inspect a component's actual shape (not just validate a body against it). Split out +// of validate-response.ts (400-line cap) as its own cohesive concern. +import type { AnySchemaObject } from 'ajv'; +import { markUnevaluatedPropertiesFalse, buildMirrorQualifyRef } from './unevaluated-properties.js'; +import { CHILD_MIRROR_ID, IN_PLACE_MIRROR_ID } from './unevaluated-properties.js'; +import type { OpenApiDoc } from './validate-response.js'; + +/** The bundled document itself, registered once with ajv so a plain-conformance (assertResponse) + * response schema's qualified `$ref` (see {@link qualifyRefs}) can resolve against it at compile + * time — the assertResponse counterpart to the two exact-match component mirrors below. */ +export const DOC_SCHEMA_ID = 'https://specr.internal/contract/validate-response/doc'; + +type ComponentKind = 'schemas' | 'responses' | 'parameters'; +const COMPONENT_REF_PATTERN = /^#\/components\/(schemas|responses|parameters)\/([^/]+)$/; + +/** Resolves ONE level of a local `#/components/{schemas,responses,parameters}/Name` pointer + * against `doc`, or returns `value` unchanged when it isn't a `$ref` object. Exported for + * consumers that need to inspect a component's actual shape directly (rather than validate a real + * response body against it) — #649: bundling leaves these as literal `{ $ref }` pointers instead + * of dereference's fully-inlined target. Throws on a non-local or dangling ref rather than + * returning `undefined`, matching this module's fail-loud posture for a gate that would otherwise + * silently stop checking anything. */ +export function resolveIfRef(doc: OpenApiDoc, value: unknown): unknown { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return value; + const ref = (value as Record)['$ref']; + if (typeof ref !== 'string') return value; + const match = COMPONENT_REF_PATTERN.exec(ref); + if (match === null) { + throw new Error(`resolveIfRef: unsupported $ref "${ref}" — only local #/components/* refs`); + } + const kind = match[1] as ComponentKind; + const name = match[2] as string; + const target = doc.components?.[kind]?.[name]; + if (target === undefined) { + throw new Error(`resolveIfRef: openapi.yaml has no components.${kind}.${name} (ref "${ref}")`); + } + return target; +} + +/** Deep-clones `schema` and rewrites every local `$ref` string (`#/...`) to `${toId}#/...`, so it + * resolves against whatever document was registered with ajv under `toId`. Never inlines/resolves + * the ref target itself — inlining a self-referential target (e.g. SpecNode) would reproduce the + * exact circular-JS-object shape that made ajv's traversal stack overflow before #649. Throws on a + * non-local ref instead of silently leaving it unqualified (which would let it accidentally resolve + * against the wrong document by URI-shape coincidence). */ +export function qualifyRefs(schema: AnySchemaObject, toId: string): AnySchemaObject { + return qualifyRefValue(schema, toId) as AnySchemaObject; +} + +function qualifyRefValue(value: unknown, toId: string): unknown { + if (Array.isArray(value)) return value.map((item) => qualifyRefValue(item, toId)); + if (typeof value !== 'object' || value === null) return value; + const out: Record = {}; + for (const [key, val] of Object.entries(value)) { + if (key === '$ref' && typeof val === 'string') { + if (!val.startsWith('#/')) { + throw new Error(`qualifyRefs: unsupported non-local $ref "${val}"`); + } + out[key] = `${toId}${val}`; + } else { + out[key] = qualifyRefValue(val, toId); + } + } + return out; +} + +/** Minimal slice of the ajv instance this module needs — kept narrow so this file doesn't have to + * re-declare the CJS-interop typing dance validate-response.ts does for the full instance. */ +export interface AjvSchemaRegistry { + addSchema(schema: AnySchemaObject, key: string): unknown; +} + +/** Builds and registers the two exact-match component mirrors (#649): every `components.schemas` + * entry marked once as if reached via a CHILD position (properties/items) and once as if reached + * via an IN_PLACE position (allOf/oneOf/anyOf branch). Both are built eagerly for every component + * name — no dependency worklist needed, since ajv resolves `$ref` lazily by registered id at + * compile/validate time, never at registration time. A single "mark once, context-agnostic" mirror + * was tried and rejected: at least 12 real components (SuccessResponse, ErrorResponse, and others) + * are referenced from BOTH contexts somewhere in openapi.yaml, and marking SuccessResponse + * standalone made it reject the sibling `data` branch's own keys — a false rejection of a fully + * documented payload, not just a missed detection. */ +export function registerComponentMirrors(ajv: AjvSchemaRegistry, doc: OpenApiDoc): void { + const componentSchemas = doc.components?.schemas ?? {}; + const qualifyRef = buildMirrorQualifyRef(); + const childMirror: Record = {}; + const inPlaceMirror: Record = {}; + for (const [name, componentSchema] of Object.entries(componentSchemas)) { + const schema = componentSchema as AnySchemaObject; + childMirror[name] = markUnevaluatedPropertiesFalse(schema, { inPlace: false, qualifyRef }); + inPlaceMirror[name] = markUnevaluatedPropertiesFalse(schema, { inPlace: true, qualifyRef }); + } + ajv.addSchema({ components: { schemas: childMirror } }, CHILD_MIRROR_ID); + ajv.addSchema({ components: { schemas: inPlaceMirror } }, IN_PLACE_MIRROR_ID); +} diff --git a/src/test-utils/contract/unevaluated-properties.ts b/src/test-utils/contract/unevaluated-properties.ts index 3ead2d89..3032d62e 100644 --- a/src/test-utils/contract/unevaluated-properties.ts +++ b/src/test-utils/contract/unevaluated-properties.ts @@ -38,8 +38,17 @@ function isPlainObject(value: unknown): value is Record { // its own sub-instance, so each is marked like any top-level schema. // // `not` is deliberately absent: it succeeds precisely when its subschema FAILS, its annotations are -// discarded, and injecting a marker inside it could flip the branch's result. `$ref` is absent -// because every schema reaching this walker is already dereferenced (see loadSpec()). +// discarded, and injecting a marker inside it could flip the branch's result. `$ref` is NOT a +// subschema-bearing applicator the walker recurses into — since loadSpec() switched from +// dereference to bundle (issue #649), a `$ref` pointer is a genuine terminal leaf: SpecNode's own +// self-reference (`children: SpecNode[]`) would otherwise make dereference-then-walk build a +// literal circular JS object and blow ajv's compile-time traversal stack. Instead `markObject` +// below rewrites the pointer string (via `qualifyRef`) to target whichever mirror document matches +// the LOCAL context of that specific `$ref` occurrence, and leaves resolving it to ajv at +// validate-time, which walks the recursive structure lazily instead of eagerly. A `$ref` node +// evaluates no properties of its OWN (no literal `properties`/`patternProperties` etc besides the +// pointer), so `shouldMark` never marks it directly — the marking lives on the mirror entry the +// pointer targets instead. const IN_PLACE_ARRAYS = ['allOf', 'oneOf', 'anyOf'] as const; const IN_PLACE_SINGLES = ['if', 'then', 'else'] as const; const IN_PLACE_MAPS = ['dependentSchemas'] as const; @@ -71,16 +80,51 @@ const CHILD_SINGLES = [ * contexts. */ type SeenContexts = Map>>; +/** Rewrites a bundled `$ref` pointer (e.g. `#/components/schemas/SpecNode`) to target whichever + * mirror document matches the LOCAL walking context (`inPlace`) the ref was encountered under. + * The default (used whenever a caller doesn't need mirror-qualified refs — every pre-#649 call + * site) is the identity function, so existing callers/tests are byte-for-byte unaffected. */ +export type QualifyRef = (ref: string, inPlace: boolean) => string; + +export interface RefQualifyOptions { + readonly inPlace?: boolean; + readonly qualifyRef?: QualifyRef; +} + +// The two mirror documents assertResponseExact/loadSpec register with ajv (#649): CHILD_MIRROR_ID +// holds every component schema marked as if reached via a properties/items position, +// IN_PLACE_MIRROR_ID holds every component marked as if reached via an allOf/oneOf/anyOf branch +// (i.e. never marked directly — see the applicator classification above). A component referenced +// from both contexts somewhere in openapi.yaml (confirmed: SuccessResponse, ErrorResponse, and 10 +// others) needs BOTH mirror entries, which is exactly why there are two documents rather than one. +export const CHILD_MIRROR_ID = 'https://specr.internal/contract/unevaluated-properties/child'; +export const IN_PLACE_MIRROR_ID = 'https://specr.internal/contract/unevaluated-properties/in-place'; + +const LOCAL_REF_PREFIX = '#/'; + +/** Standard `qualifyRef` for the two component mirrors: rewrites a local `#/...` pointer to + * `#/...`, choosing the mirror by the LOCAL context the pointer was found in — never the + * top-level call's own `inPlace`, which is what lets one component serve both contexts correctly. */ +export function buildMirrorQualifyRef(): QualifyRef { + return (ref, inPlace) => { + if (!ref.startsWith(LOCAL_REF_PREFIX)) { + throw new Error(`markUnevaluatedPropertiesFalse: unsupported non-local $ref "${ref}"`); + } + return `${inPlace ? IN_PLACE_MIRROR_ID : CHILD_MIRROR_ID}${ref}`; + }; +} + function walkMap( schema: Record, key: string, seen: SeenContexts, - inPlace: boolean + inPlace: boolean, + qualifyRef: QualifyRef ): void { const map = schema[key]; if (!isPlainObject(map)) return; const out: Record = {}; - for (const [name, sub] of Object.entries(map)) out[name] = mark(sub, seen, inPlace); + for (const [name, sub] of Object.entries(map)) out[name] = mark(sub, seen, inPlace, qualifyRef); schema[key] = out; } @@ -88,31 +132,37 @@ function walkArray( schema: Record, key: string, seen: SeenContexts, - inPlace: boolean + inPlace: boolean, + qualifyRef: QualifyRef ): void { const branches = schema[key]; if (!Array.isArray(branches)) return; - schema[key] = branches.map((branch) => mark(branch, seen, inPlace)); + schema[key] = branches.map((branch) => mark(branch, seen, inPlace, qualifyRef)); } function walkSingle( schema: Record, key: string, seen: SeenContexts, - inPlace: boolean + inPlace: boolean, + qualifyRef: QualifyRef ): void { const sub = schema[key]; if (!isPlainObject(sub)) return; - schema[key] = mark(sub, seen, inPlace); + schema[key] = mark(sub, seen, inPlace, qualifyRef); } -function walkSubschemas(schema: Record, seen: SeenContexts): void { - for (const key of CHILD_MAPS) walkMap(schema, key, seen, false); - for (const key of IN_PLACE_MAPS) walkMap(schema, key, seen, true); - for (const key of CHILD_ARRAYS) walkArray(schema, key, seen, false); - for (const key of IN_PLACE_ARRAYS) walkArray(schema, key, seen, true); - for (const key of CHILD_SINGLES) walkSingle(schema, key, seen, false); - for (const key of IN_PLACE_SINGLES) walkSingle(schema, key, seen, true); +function walkSubschemas( + schema: Record, + seen: SeenContexts, + qualifyRef: QualifyRef +): void { + for (const key of CHILD_MAPS) walkMap(schema, key, seen, false, qualifyRef); + for (const key of IN_PLACE_MAPS) walkMap(schema, key, seen, true, qualifyRef); + for (const key of CHILD_ARRAYS) walkArray(schema, key, seen, false, qualifyRef); + for (const key of IN_PLACE_ARRAYS) walkArray(schema, key, seen, true, qualifyRef); + for (const key of CHILD_SINGLES) walkSingle(schema, key, seen, false, qualifyRef); + for (const key of IN_PLACE_SINGLES) walkSingle(schema, key, seen, true, qualifyRef); } /** True when this schema object itself evaluates object properties — via its own @@ -137,25 +187,35 @@ function shouldMark(schema: Record, inPlace: boolean): boolean function markObject( node: Record, seen: SeenContexts, - inPlace: boolean + inPlace: boolean, + qualifyRef: QualifyRef ): Record { const contexts = seen.get(node); const cached = contexts?.get(inPlace); - // Same original node, same context: either a true cycle (dereferenced schemas can be - // self-referential — stop recursing) or a harmless duplicate reference already processed for this - // context. Either way the existing (in-progress or finished) clone is the right answer. + // Same original node, same context: either a true cycle (a schema that is its own ancestor + // through some OTHER shared-identity path — see the "two different composition contexts" tests) + // or a harmless duplicate reference already processed for this context. Either way the existing + // (in-progress or finished) clone is the right answer. `$ref` pointers themselves no longer create + // this kind of cycle post-#649 (see the applicator-classification comment above): a bundled `$ref` + // is a small, non-shared literal object rewritten in place, never resolved into its target here. if (cached !== undefined) return cached; // Always build a fresh SHALLOW clone from the pristine original `node` — never mutate it, and // never derive one context's clone from another context's already-processed output (that would // leak the sibling's mark across contexts). The shallow copy suffices because every nested key // touched below is REASSIGNED to a brand-new value from a recursive call, never mutated in place. const schema: Record = { ...node }; - // Register the in-progress clone under its context BEFORE recursing, so a self-referential schema - // reached again under the SAME context returns this clone instead of recursing forever. + // A bundled `$ref` pointer: rewrite it to the mirror-qualified id for THIS local context and stop + // — it has no other subschema-bearing keywords worth walking (siblings like `description` are + // plain data), and `shouldMark` below naturally leaves it unmarked since it evaluates no + // properties of its own (see the `$ref` note in the applicator-classification comment). + if (typeof schema['$ref'] === 'string') schema['$ref'] = qualifyRef(schema['$ref'], inPlace); + // Register the in-progress clone under its context BEFORE recursing, so a schema reached again + // under the SAME context (the shared-object-identity case above) returns this clone instead of + // recursing forever. const perNode = contexts ?? new Map>(); perNode.set(inPlace, schema); seen.set(node, perNode); - walkSubschemas(schema, seen); + walkSubschemas(schema, seen, qualifyRef); if (shouldMark(schema, inPlace)) schema['unevaluatedProperties'] = false; return schema; } @@ -164,13 +224,25 @@ function markObject( * properties at its own instance location (see the applicator classification above). Never mutates * its input — the returned tree is always a fresh clone, safe to compile through an uncached ajv * instance without corrupting any other reader of the original schema object. */ -function mark(node: unknown, seen: SeenContexts, inPlace: boolean): unknown { - return isPlainObject(node) ? markObject(node, seen, inPlace) : node; +function mark( + node: unknown, + seen: SeenContexts, + inPlace: boolean, + qualifyRef: QualifyRef +): unknown { + return isPlainObject(node) ? markObject(node, seen, inPlace, qualifyRef) : node; } -export function markUnevaluatedPropertiesFalse(schema: AnySchemaObject): AnySchemaObject { +const IDENTITY_QUALIFY_REF: QualifyRef = (ref) => ref; + +export function markUnevaluatedPropertiesFalse( + schema: AnySchemaObject, + options: RefQualifyOptions = {} +): AnySchemaObject { // No assertion at this boundary (CLAUDE.md): `markObject` is typed to return an object, and // `AnySchemaObject` is structurally a `Record`, so the return type is proven // by the signature rather than asserted over an `unknown`. - return markObject(structuredClone(schema), new Map(), false); + const inPlace = options.inPlace ?? false; + const qualifyRef = options.qualifyRef ?? IDENTITY_QUALIFY_REF; + return markObject(structuredClone(schema), new Map(), inPlace, qualifyRef); } diff --git a/src/test-utils/contract/validate-response.ts b/src/test-utils/contract/validate-response.ts index 828e5877..67e79301 100644 --- a/src/test-utils/contract/validate-response.ts +++ b/src/test-utils/contract/validate-response.ts @@ -4,11 +4,20 @@ import type { Router } from 'express'; import $RefParser from '@apidevtools/json-schema-ref-parser'; import type { ValidateFunction, AnySchemaObject } from 'ajv'; import { z } from 'zod'; -import { markUnevaluatedPropertiesFalse } from './unevaluated-properties.js'; +import { markUnevaluatedPropertiesFalse, buildMirrorQualifyRef } from './unevaluated-properties.js'; +import { + resolveIfRef, + qualifyRefs, + registerComponentMirrors, + DOC_SCHEMA_ID, +} from './schema-refs.js'; // Re-exported so the walker's boundary invariants (no input mutation, per-context marking) can be // pinned from the same module surface the contract tests already import. -export { markUnevaluatedPropertiesFalse }; +export { markUnevaluatedPropertiesFalse, buildMirrorQualifyRef }; +// Re-exported (#649): consumers that need a component's actual resolved shape — not just to +// validate a response body against it — reach for this instead of hand-rolling ref-resolution. +export { resolveIfRef }; // ajv and ajv-formats are CJS-only packages with no exports map; under // moduleResolution:NodeNext they must be loaded via createRequire. @@ -17,6 +26,7 @@ const require = createRequire(import.meta.url); interface AjvInstance { compile(schema: AnySchemaObject): ValidateFunction; errorsText(errors?: ValidateFunction['errors']): string; + addSchema(schema: AnySchemaObject, key: string): unknown; } interface AjvConstructor { new (opts: { strict: boolean; allErrors: boolean }): AjvInstance; @@ -40,28 +50,48 @@ const OperationObject = z.object({ }); // Request-body schema, narrowed only to the shape operationParamKeys() reads: either a direct -// `properties` map, or (for the 3 `oneOf`-composed bodies — the two general-spec PUTs and -// POST /packages/{id}/revisions) a `oneOf` array of branch schemas each with their own -// `properties`. Everything else (types, formats, nested schemas) is deliberately untyped here. +// `properties` map, or (for the request bodies that are a top-level `$ref` to a component, or +// `oneOf`-composed — the two general-spec PUTs and POST /packages/{id}/revisions) a `oneOf` array +// of branch schemas each with their own `properties`. Everything else (types, formats, nested +// schemas) is deliberately untyped here. Never itself sees a raw `$ref` key — callers resolve one +// level of top-level `$ref` via {@link resolveIfRef} BEFORE parsing into this shape (#649: bundling +// leaves component-referencing request bodies like MergeRequest as a literal `{ $ref }` pointer +// instead of dereference's fully-inlined object). +const RequestBodyBranchObject = z.object({ + properties: z.record(z.string(), z.unknown()).optional(), +}); +// `oneOf` branches are `z.unknown()` here, not narrowed directly: each may itself be a top-level +// `$ref` (post /packages/{id}/revisions' CreateRevisionLegacyBody/CreateRevisionStructuredBody, +// #649), resolved per-branch in bodyPropertyKeys() before parsing into RequestBodyBranchObject. const RequestBodySchemaObject = z.object({ properties: z.record(z.string(), z.unknown()).optional(), - oneOf: z.array(z.object({ properties: z.record(z.string(), z.unknown()).optional() })).optional(), + oneOf: z.array(z.unknown()).optional(), }); -const RequestBodyContent = z.record( - z.string(), - z.object({ schema: RequestBodySchemaObject.optional() }) -); +const MediaTypeObject = z.object({ schema: z.unknown().optional() }); +const RequestBodyContent = z.record(z.string(), MediaTypeObject); const ParameterObject = z.object({ name: z.string(), in: z.string(), }); +// Raw (possibly `$ref`'d — e.g. `#/components/parameters/SpecId`) parameter entries, resolved one +// at a time via {@link resolveIfRef} in operationParamKeys() before parsing into ParameterObject. const OperationWithParamsObject = z.object({ - parameters: z.array(ParameterObject).optional(), + parameters: z.array(z.unknown()).optional(), requestBody: z.object({ content: RequestBodyContent.optional() }).optional(), }); +const ComponentsObject = z.object({ + schemas: z.record(z.string(), z.unknown()).optional(), + responses: z.record(z.string(), z.unknown()).optional(), + parameters: z.record(z.string(), z.unknown()).optional(), +}); const OpenApiDocSchema = z.object({ servers: z.array(z.object({ url: z.string() })).optional(), paths: z.record(z.string(), z.record(z.string(), z.unknown())), + // Was silently stripped by Zod's default object behavior before #649 — harmless under + // dereference (nothing read it), but load-bearing under bundle: ajv needs + // `doc.components.schemas` present in the REGISTERED document for any + // `#/components/schemas/X` pointer left in `doc.paths` to resolve at compile/validate time. + components: ComponentsObject.optional(), }); export type OpenApiDoc = z.infer; @@ -74,7 +104,20 @@ function normalizePath(path: string): string { let specPromise: Promise | null = null; export function loadSpec(): Promise { - specPromise ??= $RefParser.dereference(SPEC_PATH).then((raw) => OpenApiDocSchema.parse(raw)); + // bundle, not dereference (#649): dereference resolves every `$ref` into its literal target, + // which for a self-referential component (SpecNode: `children: SpecNode[]`) builds a real + // circular JS object that blows ajv's compile-time schema traversal (RangeError: maximum call + // stack size exceeded) for every response whose success body embeds one. bundle preserves `$ref` + // pointers for openapi.yaml's purely-internal refs (confirmed: zero external-file refs in this + // single-file spec, so bundle's output is structurally identical to `loadRawSpec()`'s un- + // dereferenced parse) — ajv resolves them lazily at validate time instead, which is exactly what + // lets it handle real recursion without ever materializing a circular object. + specPromise ??= $RefParser.bundle(SPEC_PATH).then((raw) => { + const doc = OpenApiDocSchema.parse(raw); + ajv.addSchema(doc, DOC_SCHEMA_ID); + registerComponentMirrors(ajv, doc); + return doc; + }); return specPromise; } @@ -93,10 +136,16 @@ export function loadRawSpec(): Promise { // Exported so contract tests can compile+run an arbitrary component schema directly // (e.g. cross-checking a request-body schema against a hand-duplicated Zod shape), // not just a response schema reached through assertResponse. +// +// Cache key is the ORIGINAL, unqualified `schema` object (never the qualified clone) so repeated +// calls for the same op+status keep hitting the same compiled validator. A schema with no `$ref` at +// all (every hand-built schema in this module's own tests) round-trips through qualifyRefs as a +// harmless no-op clone, so callers never need `loadSpec()` to have registered {@link DOC_SCHEMA_ID} +// first unless their schema actually contains a `$ref`. export function getValidator(schema: AnySchemaObject): ValidateFunction { const cached = validators.get(schema); if (cached !== undefined) return cached; - const compiled = ajv.compile(schema); + const compiled = ajv.compile(qualifyRefs(schema, DOC_SCHEMA_ID)); validators.set(schema, compiled); return compiled; } @@ -179,7 +228,15 @@ export async function assertResponseExact( const doc = await loadSpec(); const schema = resolveResponseSchema(doc, method, pathTemplate, status); if (!schema) return; // documented non-JSON (binary / no-content / multipart) — out of scope - const exactSchema = markUnevaluatedPropertiesFalse(schema); + // Mark against the mirror-qualifying callback (#649), the SAME one loadSpec() used to build + // CHILD_MIRROR_ID/IN_PLACE_MIRROR_ID — so a `$ref` nested anywhere in this response's own tree + // (e.g. `data: { $ref: SpecTree }`) resolves into an already-marked mirror entry instead of an + // unresolvable dangling pointer or, worse, an unmarked one that would silently reopen #640 for + // exactly the operations this issue exists to close. + const exactSchema = markUnevaluatedPropertiesFalse(schema, { + inPlace: false, + qualifyRef: buildMirrorQualifyRef(), + }); const validate = ajv.compile(exactSchema); if (!validate(body)) { // The exact schema still enforces `required` and field types, so a failure here is not @@ -256,16 +313,22 @@ export interface ParamSet { readonly body: ReadonlySet; } -type RequestBodySchema = z.infer; - // Union of the direct `properties` map AND every `oneOf` branch's `properties` (the 3 composed-body // ops), so INV-4 stays meaningful instead of passing vacuously for exactly the operations this // helper exists to check. Unioning rather than letting a direct `properties` win outright matters // for a schema carrying both (a discriminator alongside branch-specific fields): short-circuiting -// on the base map would drop every branch field without any signal. -function bodyPropertyKeys(schema: RequestBodySchema | undefined): ReadonlySet { - const keys = new Set(Object.keys(schema?.properties ?? {})); - for (const branch of schema?.oneOf ?? []) { +// on the base map would drop every branch field without any signal. `rawSchema` is resolved via +// {@link resolveIfRef} BEFORE parsing into `RequestBodySchemaObject` — #649: many request bodies +// (MergeRequest, CreateAssociation, PutConventionBody, HeaderFooterComposition, …) are a top-level +// `$ref` to a component, which bundling leaves as a literal `{ $ref }` pointer instead of +// dereference's fully-inlined object; parsing that bare pointer through a schema with no `$ref` +// field would silently strip it and yield an empty (vacuously-passing) key set. +function bodyPropertyKeys(doc: OpenApiDoc, rawSchema: unknown): ReadonlySet { + if (rawSchema === undefined) return new Set(); + const schema = RequestBodySchemaObject.parse(resolveIfRef(doc, rawSchema)); + const keys = new Set(Object.keys(schema.properties ?? {})); + for (const rawBranch of schema.oneOf ?? []) { + const branch = RequestBodyBranchObject.parse(resolveIfRef(doc, rawBranch)); for (const key of Object.keys(branch.properties ?? {})) keys.add(key); } return keys; @@ -274,11 +337,12 @@ function bodyPropertyKeys(schema: RequestBodySchema | undefined): ReadonlySet | undefined ): ReadonlySet { const jsonSchema = content?.['application/json']?.schema; - if (jsonSchema !== undefined) return bodyPropertyKeys(jsonSchema); - return bodyPropertyKeys(content?.['multipart/form-data']?.schema); + if (jsonSchema !== undefined) return bodyPropertyKeys(doc, jsonSchema); + return bodyPropertyKeys(doc, content?.['multipart/form-data']?.schema); } /** Query-param names and request-body top-level property names an operation documents in @@ -295,10 +359,15 @@ export function operationParamKeys( const raw = doc.paths[pathTemplate]?.[method.toLowerCase()]; if (raw === undefined) throw new Error(`No OpenAPI operation: ${method} ${pathTemplate}`); const op = OperationWithParamsObject.parse(raw); + // Each entry may itself be a `$ref` (e.g. `#/components/parameters/SpecId`, #649) rather than an + // inline parameter object — resolve before narrowing into ParameterObject. + const parameters = (op.parameters ?? []).map((param) => + ParameterObject.parse(resolveIfRef(doc, param)) + ); const query = new Set( - (op.parameters ?? []).filter((param) => param.in === 'query').map((param) => param.name) + parameters.filter((param) => param.in === 'query').map((param) => param.name) ); - const body = requestBodyKeys(op.requestBody?.content); + const body = requestBodyKeys(doc, op.requestBody?.content); // Fail loud rather than hand back an empty set: a documented requestBody whose top-level keys // this narrow reader cannot derive (an unsupported composition — allOf, a nested anyOf, a // non-object body) would make INV-4 pass vacuously for that op, which is the failure mode the From a47ec3b6485bf0f7829f9ba81178c8ad7c103a23 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 14:36:15 -0700 Subject: [PATCH 03/10] fix(api): resolve $ref pointers in openapi-schema tests after bundling switch path-id-status-openapi.test.ts and revision-parent-openapi.test.ts read raw response/request schema shapes out of loadSpec()'s OpenApiDoc directly (400/404/500 responses $ref'd to components/responses/*, a RevisionWithTrees/RevisionSummary/array-items data schema $ref'd to components/schemas/*). Under the new bundle-based loadSpec() (#649) those stay literal `{ $ref }` pointers instead of dereference's fully-inlined objects, so both files now resolve one level via resolveIfRef() before narrowing into their local Zod shapes. Co-Authored-By: Claude Sonnet 5 --- src/api/path-id-status-openapi.test.ts | 28 +++++++++---- src/api/revision-parent-openapi.test.ts | 56 ++++++++++++++++--------- 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/src/api/path-id-status-openapi.test.ts b/src/api/path-id-status-openapi.test.ts index 3873e017..94a9d6d4 100644 --- a/src/api/path-id-status-openapi.test.ts +++ b/src/api/path-id-status-openapi.test.ts @@ -15,7 +15,11 @@ // disciplines .integration.test.ts, all tagged "(#568)"). import { describe, it, expect } from 'vitest'; import { z } from 'zod'; -import { loadSpec, type OpenApiDoc } from '../test-utils/contract/validate-response.js'; +import { + loadSpec, + resolveIfRef, + type OpenApiDoc, +} from '../test-utils/contract/validate-response.js'; const MediaObjectSchema = z.object({ schema: z.unknown() }); const ContentSchema = z.record(z.string(), MediaObjectSchema); @@ -23,8 +27,13 @@ const ResponseObjectSchema = z.object({ description: z.string().optional(), content: ContentSchema.optional(), }); +// `responses` values are `z.unknown()`, not narrowed directly: a status like 400/404/500 is +// typically `$ref`'d to a shared `components/responses/*` entry (e.g. BadRequest) rather than +// documented inline, which bundling (#649) leaves as a literal `{ $ref }` pointer instead of +// dereference's fully-inlined response object — resolved per-status in operation()/ +// expectErrorResponseSchema() before parsing into ResponseObjectSchema. const OperationSchema = z.object({ - responses: z.record(z.string(), ResponseObjectSchema).optional(), + responses: z.record(z.string(), z.unknown()).optional(), }); function operation(doc: OpenApiDoc, path: string, method: string): z.infer { @@ -48,14 +57,17 @@ const ErrorResponseShapeSchema = z.object({ }); function expectErrorResponseSchema( + doc: OpenApiDoc, responses: z.infer['responses'], status: string, label: string ): void { - const response = responses?.[status]; - expect(response, `${label} does not document a ${status} response`).toBeDefined(); - const schema = response?.content?.['application/json']?.schema; - expect(schema, `${label} ${status} has no application/json schema`).toBeDefined(); + const rawResponse = responses?.[status]; + expect(rawResponse, `${label} does not document a ${status} response`).toBeDefined(); + const response = ResponseObjectSchema.parse(resolveIfRef(doc, rawResponse)); + const rawSchema = response.content?.['application/json']?.schema; + expect(rawSchema, `${label} ${status} has no application/json schema`).toBeDefined(); + const schema = resolveIfRef(doc, rawSchema); const parsed = ErrorResponseShapeSchema.safeParse(schema); expect(parsed.success, `${label} ${status} is not the ErrorResponse shape`).toBe(true); @@ -100,7 +112,7 @@ describe('openapi.yaml documents the parsePathUuid 400 on every migrated operati const op = operation(doc, path, method); // ErrorResponse always requires `success: false` + `error: string` — // confirm the 400 targets that shape, not some ad hoc one-off object. - expectErrorResponseSchema(op.responses, '400', `${method} ${path}`); + expectErrorResponseSchema(doc, op.responses, '400', `${method} ${path}`); } ); }); @@ -109,6 +121,6 @@ describe('openapi.yaml documents the shared-schema 422 on POST /templates/import it('documents 422 (not a second, ad hoc 400) for a name that fails CreateTemplateBodySchema', async () => { const doc = await loadSpec(); const op = operation(doc, '/templates/import', 'post'); - expectErrorResponseSchema(op.responses, '422', 'post /templates/import'); + expectErrorResponseSchema(doc, op.responses, '422', 'post /templates/import'); }); }); diff --git a/src/api/revision-parent-openapi.test.ts b/src/api/revision-parent-openapi.test.ts index 77520598..6a6f035c 100644 --- a/src/api/revision-parent-openapi.test.ts +++ b/src/api/revision-parent-openapi.test.ts @@ -13,7 +13,11 @@ // those modules without a matching openapi.yaml edit fails here first. import { describe, it, expect } from 'vitest'; import { z } from 'zod'; -import { loadSpec, type OpenApiDoc } from '../test-utils/contract/validate-response.js'; +import { + loadSpec, + resolveIfRef, + type OpenApiDoc, +} from '../test-utils/contract/validate-response.js'; const JsonSchemaObjectSchema = z.object({ type: z.union([z.string(), z.array(z.string())]).optional(), @@ -46,26 +50,37 @@ function jsonSchemaOf(content: z.infer | undefined): unkno return media.schema; } -/** Picks the oneOf/allOf branch whose `required` list names `field` — order-independent. */ -function branchRequiring(branches: readonly unknown[], field: string): JsonSchemaObject { +/** Picks the oneOf/allOf branch whose `required` list names `field` — order-independent. Each + * branch is resolved via {@link resolveIfRef} before narrowing: bundling (#649) leaves a branch + * that is itself a top-level `$ref` (e.g. CreateRevisionLegacyBody/CreateRevisionStructuredBody) + * as a literal `{ $ref }` pointer instead of dereference's fully-inlined object. */ +function branchRequiring( + doc: OpenApiDoc, + branches: readonly unknown[], + field: string +): JsonSchemaObject { const match = branches - .map((b) => JsonSchemaObjectSchema.parse(b)) + .map((b) => JsonSchemaObjectSchema.parse(resolveIfRef(doc, b))) .find((b) => (b.required ?? []).includes(field)); if (match === undefined) throw new Error(`no schema branch requires "${field}"`); return match; } -/** Unwraps `data` out of the `allOf: [SuccessResponse, { data }]` envelope. */ -function dataSchemaOf(schema: unknown): unknown { +/** Unwraps `data` out of the `allOf: [SuccessResponse, { data }]` envelope, resolving a `data` + * schema that is itself a top-level `$ref` (e.g. `{ $ref: RevisionWithTrees }`, #649). */ +function dataSchemaOf(doc: OpenApiDoc, schema: unknown): unknown { const allOf = z.object({ allOf: z.array(z.unknown()) }).parse(schema).allOf; - const holder = branchRequiring(allOf, 'data'); + const holder = branchRequiring(doc, allOf, 'data'); const properties = holder.properties; if (properties === undefined) throw new Error('data-bearing branch has no properties'); - return properties['data']; + return resolveIfRef(doc, properties['data']); } -function itemsOf(schema: unknown): unknown { - return z.object({ items: z.unknown() }).parse(schema).items; +/** Resolves an array schema's `items` — itself commonly a top-level `$ref` (e.g. `#/.../ + * RevisionSummary`, #649) — to its actual component shape. */ +function itemsOf(doc: OpenApiDoc, schema: unknown): unknown { + const items = z.object({ items: z.unknown() }).parse(schema).items; + return resolveIfRef(doc, items); } function expectNullableUuidField(schema: JsonSchemaObject, field: string): void { @@ -82,7 +97,7 @@ describe('openapi.yaml — package_revisions.parent_revision_id (ADR-066 #389)', const oneOf = z .object({ oneOf: z.array(z.unknown()) }) .parse(jsonSchemaOf(op.requestBody?.content)).oneOf; - const structured = branchRequiring(oneOf, 'type'); + const structured = branchRequiring(doc, oneOf, 'type'); const prop = JsonSchemaObjectSchema.parse((structured.properties ?? {})['parentRevisionId']); expect(prop.type).toBe('string'); expect(prop.format).toBe('uuid'); @@ -95,7 +110,7 @@ describe('openapi.yaml — package_revisions.parent_revision_id (ADR-066 #389)', const oneOf = z .object({ oneOf: z.array(z.unknown()) }) .parse(jsonSchemaOf(op.requestBody?.content)).oneOf; - const legacy = branchRequiring(oneOf, 'label'); + const legacy = branchRequiring(doc, oneOf, 'label'); expect(legacy.properties ?? {}).not.toHaveProperty('parentRevisionId'); }); @@ -103,7 +118,7 @@ describe('openapi.yaml — package_revisions.parent_revision_id (ADR-066 #389)', const doc = await loadSpec(); const op = operation(doc, '/packages/{id}/revisions', 'post'); const revisionSummary = JsonSchemaObjectSchema.parse( - dataSchemaOf(jsonSchemaOf(op.responses?.['201']?.content)) + dataSchemaOf(doc, jsonSchemaOf(op.responses?.['201']?.content)) ); expectNullableUuidField(revisionSummary, 'parentRevisionId'); }); @@ -111,8 +126,8 @@ describe('openapi.yaml — package_revisions.parent_revision_id (ADR-066 #389)', it('RevisionSummary requires parentRevisionId on every list item (GET .../revisions 200)', async () => { const doc = await loadSpec(); const op = operation(doc, '/packages/{id}/revisions', 'get'); - const arraySchema = dataSchemaOf(jsonSchemaOf(op.responses?.['200']?.content)); - const itemSchema = JsonSchemaObjectSchema.parse(itemsOf(arraySchema)); + const arraySchema = dataSchemaOf(doc, jsonSchemaOf(op.responses?.['200']?.content)); + const itemSchema = JsonSchemaObjectSchema.parse(itemsOf(doc, arraySchema)); expectNullableUuidField(itemSchema, 'parentRevisionId'); }); @@ -120,7 +135,7 @@ describe('openapi.yaml — package_revisions.parent_revision_id (ADR-066 #389)', const doc = await loadSpec(); const op = operation(doc, '/revisions/{id}', 'get'); const revisionWithTrees = JsonSchemaObjectSchema.parse( - dataSchemaOf(jsonSchemaOf(op.responses?.['200']?.content)) + dataSchemaOf(doc, jsonSchemaOf(op.responses?.['200']?.content)) ); expectNullableUuidField(revisionWithTrees, 'parentRevisionId'); }); @@ -148,12 +163,12 @@ describe('openapi.yaml — package_revisions.base_revision_id (ADR-066 #390)', ( const branches = z .object({ oneOf: z.array(z.unknown()) }) .parse(jsonSchemaOf(op.requestBody?.content)).oneOf; - const structured = branchRequiring(branches, 'type'); + const structured = branchRequiring(doc, branches, 'type'); const base = JsonSchemaObjectSchema.parse((structured.properties ?? {})['baseRevisionId']); expect(base.type).toBe('string'); expect(base.format).toBe('uuid'); expect(structured.required ?? []).not.toContain('baseRevisionId'); - expect(branchRequiring(branches, 'label').properties ?? {}).not.toHaveProperty( + expect(branchRequiring(doc, branches, 'label').properties ?? {}).not.toHaveProperty( 'baseRevisionId' ); }); @@ -168,9 +183,12 @@ describe('openapi.yaml — package_revisions.base_revision_id (ADR-066 #390)', ( const doc = await loadSpec(); const op = operation(doc, path, method); const dataSchema = dataSchemaOf( + doc, jsonSchemaOf(op.responses?.[method === 'post' ? '201' : '200']?.content) ); - const responseSchema = JsonSchemaObjectSchema.parse(list ? itemsOf(dataSchema) : dataSchema); + const responseSchema = JsonSchemaObjectSchema.parse( + list ? itemsOf(doc, dataSchema) : dataSchema + ); expectNullableUuidField(responseSchema, 'baseRevisionId'); } ); From 178fcdf9adb0ace9cb9c830953028acdfde53d16 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 14:36:36 -0700 Subject: [PATCH 04/10] test(contract): validate the six previously-unvalidated response schemas (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves GET /specs/{id}, POST /specs/{id}/paragraphs, PATCH /specs/{id}/paragraphs/{nodeId} (and its /removal and /reject siblings), and GET /revisions/{id} from RESPONSE_ALLOWLIST to RESPONSE_COVERED now that loadSpec()'s bundle switch lets ajv compile their self-referential SpecNode/SpecTree schemas. Drives each op with a real request against the existing checkpoint fixture and a new package/revision fixture, asserts schema conformance (assertResponse) and exact-key-match (assertResponseExact) against the real response, and splices an undocumented key onto each real payload — at the top level and nested inside the recursive SpecNode/SpecTree structure — to prove INV-6 actually rejects it for all six ops, not just the previously-compilable ones. Adds unit-level regression coverage in validate-response.test.ts: - a hand-built `items` case in the applicator-coverage table (previously absent — the mutation-verify bar had nothing to go red for the CHILD keyword that recurses into SpecNode's own `children`, confirmed by temporarily removing it from CHILD_SINGLES and watching the new case fail; restored afterward) - a context-crossing case pinning that a $ref inside `items` nested in an allOf branch is qualified with CHILD context, never the branch's own IN_PLACE context (found during implementation: a real-openapi.yaml- driven version of this mutation did NOT reliably fail, because an unqualified/mis-qualified ref can accidentally self-resolve against the wrong mirror document by URI-shape coincidence — this hand-built, spec-independent case doesn't have that coincidence to hide behind) - getValidator compiling and validating a 200-deep synthetic self-referential SpecNode payload with zero RangeError - assertResponseExact accepting a clean SuccessResponse-enveloped SpecNode payload (the false-rejection pitfall the dual-mirror design exists to avoid) and rejecting a key nested inside SpecNode.children Also corrects a stale comment framing the "reached via two different composition contexts" tests as a dereference-object-identity artifact — that mechanism (SeenContexts) is orthogonal to #649 and still pins a genuinely different, ref-parser-independent case. Co-Authored-By: Claude Sonnet 5 --- src/api/contract.integration.test.ts | 215 ++++++++++++++++-- .../contract/validate-response.test.ts | 178 ++++++++++++++- 2 files changed, 367 insertions(+), 26 deletions(-) diff --git a/src/api/contract.integration.test.ts b/src/api/contract.integration.test.ts index 1935f61f..435589d0 100644 --- a/src/api/contract.integration.test.ts +++ b/src/api/contract.integration.test.ts @@ -5,6 +5,7 @@ import { router } from './router.js'; import { errorHandler } from './middleware/error.js'; import { assertResponse, + assertResponseExact, expressRouteManifest, specOperationManifest, successJsonOps, @@ -53,9 +54,7 @@ const RESPONSE_COVERED = new Set([ 'get /packages/{}/header-footer/resolved', 'get /revisions/{}/header-footer/resolved', // version-history checkpoints and pending summaries (ADR-052 D3/D4/D9, - // issue #380 task 11) — dedicated response-contract test below. Sibling op - // `patch /specs/{}/paragraphs/{}/reject` returns a SpecNode and is - // allowlisted instead (see the SpecNode-cycle comment above). + // issue #380 task 11) — dedicated response-contract test below. 'post /specs/{}/checkpoints', 'get /specs/{}/checkpoints', 'post /projects/{}/checkpoints', @@ -71,6 +70,16 @@ const RESPONSE_COVERED = new Set([ 'put /projects/{}/language-rules', 'delete /projects/{}/language-rules', 'get /projects/{}/language-findings', + // #649: loadSpec() switched from full $ref dereference to bundle, so ajv can compile the + // self-referential SpecNode/SpecTree response schemas these six operations' success bodies + // embed instead of stack-overflowing on a literal circular JS object. Response-verified (and + // INV-6 exact-match-verified) in the dedicated checkpoint/reject test block below. + 'get /specs/{}', + 'post /specs/{}/paragraphs', + 'patch /specs/{}/paragraphs/{}', + 'patch /specs/{}/paragraphs/{}/removal', + 'patch /specs/{}/paragraphs/{}/reject', + 'get /revisions/{}', ]); // Documented JSON ops not yet response-verified (burned down in PR2…N). @@ -91,8 +100,6 @@ const RESPONSE_ALLOWLIST = new Set([ 'get /projects/{}/references/broken', 'get /projects/{}/references/inbound', 'get /projects/{}/specs/{}/references', - 'get /revisions/{}', - 'get /specs/{}', 'get /specs/{}/hierarchy-report', 'get /specs/{}/lineage', 'get /specs/{}/paragraphs/{}/history', @@ -102,16 +109,6 @@ const RESPONSE_ALLOWLIST = new Set([ 'get /templates/{}', 'patch /libraries/{}', 'patch /specs/{}', - // SpecNode is self-referential (children: SpecNode[]); loadSpec()'s full - // $ref dereference turns that into a real object-identity cycle that blows - // ajv's schema-traversal stack (json-schema-traverse has no cycle guard). - // Every op whose success body embeds a SpecNode is allowlisted for that - // structural reason, not because it lacks a test — see each op's own - // integration test for real (non-schema) response assertions instead. - 'patch /specs/{}/paragraphs/{}', - 'patch /specs/{}/paragraphs/{}/removal', - 'post /specs/{}/paragraphs', - 'patch /specs/{}/paragraphs/{}/reject', // ADR-052 D4, issue #380 — same SpecNode cycle 'patch /templates/{}', 'post /clients', 'get /clients', @@ -642,13 +639,27 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D expect(getCp.status).toBe(200); await assertResponse('get', '/checkpoints/{id}', 200, await getCp.json()); - // 3. An edit made after the checkpoint is pending — content_version 1 -> 2. + // 3. An edit made after the checkpoint is pending — content_version 1 -> 2. PATCH + // .../paragraphs/{nodeId}'s response is a SpecNode (#649: previously unvalidated — its + // self-referential `children: SpecNode[]` schema stack-overflowed ajv's compile-time + // traversal under loadSpec()'s full dereference; bundling + mirror-qualified $refs fixes + // that). Exact-match + a spliced undocumented key proves INV-6 actually reaches this op. const edit = await fetch(`${baseUrl}/specs/${specId}/paragraphs/${paragraphId}`, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ text: 'Edited pending text.', actorLabel }), }); expect(edit.status).toBe(200); + const editBody = (await edit.json()) as { data: { id: string; text: string; meta: object } }; + expect(editBody.data.text).toBe('Edited pending text.'); + await assertResponse('patch', '/specs/{id}/paragraphs/{nodeId}', 200, editBody); + await assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, editBody); + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, { + ...editBody, + data: { ...editBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); // 4. The spec pending-summary reflects exactly that one pending paragraph. const specPending = await fetch(`${baseUrl}/specs/${specId}/pending-summary`); @@ -695,10 +706,9 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D // 7. Rejecting to the ORIGINAL (earlier) checkpoint restores the pre-edit // text — proving the boundary lookup targets checkpointId's own sealed // content_version (1), not the project checkpoint's later one (2). - // No assertResponse here: the response body is a SpecNode, whose - // self-referential schema stack-overflows ajv after loadSpec()'s full - // dereference (see the RESPONSE_ALLOWLIST comment above) — the same - // reason its sibling paragraph-mutation ops skip schema validation. + // PATCH .../reject's response is a SpecNode (#649 — same fix as step 3's PATCH; ADR-052 D4, + // issue #380 previously called out the identical self-referential-schema stack overflow as + // the reason this op skipped schema validation entirely). const reject = await fetch(`${baseUrl}/specs/${specId}/paragraphs/${paragraphId}/reject`, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, @@ -707,6 +717,171 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D expect(reject.status).toBe(200); const rejectBody = (await reject.json()) as { data: { text: string } }; expect(rejectBody.data.text).toBe(ORIGINAL_TEXT); + await assertResponse('patch', '/specs/{id}/paragraphs/{nodeId}/reject', 200, rejectBody); + await assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/reject', 200, rejectBody); + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/reject', 200, { + ...rejectBody, + data: { ...(rejectBody.data as object), rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + + // 8. GET /specs/{id} returns the full SpecTree — matches its documented schema exactly, and + // rejects a spliced undocumented key both at the top level (the data wrapper) and nested two + // levels deep inside `data.parts` (SpecTree's own recursive SpecNode `children` position) — + // #649's six unvalidated operations were exactly the ones whose response embeds this + // self-referential shape somewhere. + const getSpec = await fetch(`${baseUrl}/specs/${specId}`); + expect(getSpec.status).toBe(200); + const getSpecBody = (await getSpec.json()) as { + data: { id: string; parts: readonly { id: string; type: string }[] }; + }; + expect(getSpecBody.data.id).toBe(specId); + await assertResponse('get', '/specs/{id}', 200, getSpecBody); + await assertResponseExact('get', '/specs/{id}', 200, getSpecBody); + await expect( + assertResponseExact('get', '/specs/{id}', 200, { + ...getSpecBody, + data: { ...getSpecBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + const firstPart = getSpecBody.data.parts[0]; + if (firstPart === undefined) throw new Error('checkpoint fixture spec tree has no parts'); + await expect( + assertResponseExact('get', '/specs/{id}', 200, { + ...getSpecBody, + data: { + ...getSpecBody.data, + parts: [{ ...firstPart, rogueKey: 'nope' }, ...getSpecBody.data.parts.slice(1)], + }, + }) + ).rejects.toThrow(/does not document/); + + // 9. POST /specs/{id}/paragraphs inserts a new sibling paragraph — response is a SpecNode + // (201), same exact-match + splice-rejection proof as the PATCH ops above. + const insertParagraph = await fetch(`${baseUrl}/specs/${specId}/paragraphs`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + anchorNodeId: paragraphId, + text: 'Inserted paragraph text.', + actorLabel, + }), + }); + expect(insertParagraph.status).toBe(201); + const insertedBody = (await insertParagraph.json()) as { data: { id: string; text: string } }; + expect(insertedBody.data.text).toBe('Inserted paragraph text.'); + await assertResponse('post', '/specs/{id}/paragraphs', 201, insertedBody); + await assertResponseExact('post', '/specs/{id}/paragraphs', 201, insertedBody); + await expect( + assertResponseExact('post', '/specs/{id}/paragraphs', 201, { + ...insertedBody, + data: { ...insertedBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + const insertedNodeId = insertedBody.data.id; + + // 10. PATCH .../paragraphs/{nodeId}/removal toggles the just-inserted paragraph's `vanish` + // flag — response is again a SpecNode. + const removal = await fetch(`${baseUrl}/specs/${specId}/paragraphs/${insertedNodeId}/removal`, { + method: 'PATCH', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ removed: true, actorLabel }), + }); + expect(removal.status).toBe(200); + const removalBody = (await removal.json()) as { + data: { id: string; meta: { vanish?: boolean } }; + }; + expect(removalBody.data.meta.vanish).toBe(true); + await assertResponse('patch', '/specs/{id}/paragraphs/{nodeId}/removal', 200, removalBody); + await assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/removal', 200, removalBody); + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/removal', 200, { + ...removalBody, + data: { ...removalBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + }); + + it('GET /revisions/{id} matches its documented schema exactly, including its embedded SpecTree', async () => { + const { specId, projectId } = fixture; + const pkg = await fetch(`${baseUrl}/projects/${projectId}/packages`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ name: `contract-revision-pkg-${Date.now()}` }), + }); + expect(pkg.status).toBe(201); + const pkgBody = (await pkg.json()) as { data: { packageId: string } }; + const packageId = pkgBody.data.packageId; + // setPackageSpecs requires every member spec to already be on the project's TOC + // (project_specs) — the checkpoint fixture's spec carries a `project_id` column but was + // inserted directly via SQL, never added to project_specs. + await pool.query( + 'INSERT INTO project_specs (project_id, spec_id, position) VALUES ($1, $2, 1)', + [projectId, specId] + ); + try { + const setMembers = await fetch(`${baseUrl}/packages/${packageId}/specs`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ specIds: [specId] }), + }); + expect(setMembers.status).toBe(200); + + const issue = await fetch(`${baseUrl}/packages/${packageId}/revisions`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ label: 'Contract GET /revisions/{id} baseline' }), + }); + expect(issue.status).toBe(201); + const issueBody = (await issue.json()) as { data: { revisionId: string } }; + const revisionId = issueBody.data.revisionId; + + // GET /revisions/{id}'s `data` is RevisionWithTrees, which embeds `specs[].tree` — a full + // SpecTree, the same self-referential shape as GET /specs/{id}'s `data` (#649). Splice both + // at the top level and nested inside the frozen tree's `parts`. + const getRevision = await fetch(`${baseUrl}/revisions/${revisionId}`); + expect(getRevision.status).toBe(200); + const getRevisionBody = (await getRevision.json()) as { + data: { + revisionId: string; + specs: readonly { specId: string; tree: { parts: readonly unknown[] } }[]; + }; + }; + expect(getRevisionBody.data.revisionId).toBe(revisionId); + await assertResponse('get', '/revisions/{id}', 200, getRevisionBody); + await assertResponseExact('get', '/revisions/{id}', 200, getRevisionBody); + await expect( + assertResponseExact('get', '/revisions/{id}', 200, { + ...getRevisionBody, + data: { ...getRevisionBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + const firstSpecEntry = getRevisionBody.data.specs[0]; + if (firstSpecEntry === undefined) throw new Error('issued revision has no member specs'); + await expect( + assertResponseExact('get', '/revisions/{id}', 200, { + ...getRevisionBody, + data: { + ...getRevisionBody.data, + specs: [ + { ...firstSpecEntry, tree: { ...firstSpecEntry.tree, rogueKey: 'nope' } }, + ...getRevisionBody.data.specs.slice(1), + ], + }, + }) + ).rejects.toThrow(/does not document/); + } finally { + // package_revisions.package_id and package_revision_specs.revision_id both CASCADE from + // design_packages (021_create_package_revisions.ts), so deleting the package alone is enough + // there. project_specs.spec_id is RESTRICT (007_create_project_specs.ts), so it must be + // cleaned up explicitly before the outer afterAll's `DELETE FROM specs`. + await pool.query('DELETE FROM design_packages WHERE id = $1', [packageId]); + await pool.query('DELETE FROM project_specs WHERE project_id = $1 AND spec_id = $2', [ + projectId, + specId, + ]); + } }); }); diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index 9765f27a..cb4f8e7b 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -8,11 +8,27 @@ import { loadRawSpec, operationParamKeys, markUnevaluatedPropertiesFalse, + buildMirrorQualifyRef, resolveResponseSchema, getValidator, } from './validate-response.js'; import type { OpenApiDoc } from './validate-response.js'; +// A fixed-format UUID literal — ajv's `format: uuid` only checks shape, so every synthetic +// SpecNode below reuses this rather than pulling in a real uuid generator. +const NODE_ID = '11111111-1111-1111-1111-111111111111'; + +/** Builds a `depth`-deep, self-referential SpecNode chain (`children: [child]` at every level) — + * the exact shape that made $RefParser.dereference() build a literal circular JS object and blow + * ajv's compile-time traversal stack before #649. */ +function buildDeepSpecNode(depth: number): unknown { + let node: unknown = { id: NODE_ID, type: 'pr1', text: 'leaf', children: [], meta: {} }; + for (let i = 0; i < depth; i++) { + node = { id: NODE_ID, type: 'pr1', text: `depth-${i}`, children: [node], meta: {} }; + } + return node; +} + const HTTP_METHODS = ['get', 'post', 'put', 'patch', 'delete'] as const; // Narrow shape for the raw (un-dereferenced) request-body schema a #377 write op documents. @@ -151,12 +167,17 @@ describe('assertResponseExact (#640) — exact-key-match against openapi.yaml', expect(marked.properties.nested.unevaluatedProperties).toBe(false); }); - // A dereferenced schema can reach the SAME object identity (one $ref target reused by - // $RefParser.dereference for every pointer to it) through two different composition contexts - // in one tree: once as a raw allOf branch (must stay unmarked — see unevaluated-properties.ts's - // applicator classification) and once nested under a sibling's properties (must be marked). A visited-Set that - // only tracks "already seen" — not "seen under which context" — lets whichever context visits - // first silently decide for both, dropping the second context's mark. + // Pins the walker's SHARED-OBJECT-IDENTITY cycle guard (SeenContexts in unevaluated-properties.ts) + // — a mechanism orthogonal to, and unaffected by, #649's switch from dereference to bundle. It has + // nothing to do with `$ref` strings: it's a hand-built schema where ONE JS object (`shared`) is + // literally reused at two composition sites in the SAME tree, the way any hand-authored or + // programmatically-assembled schema legitimately can (bundling doesn't produce this shape — a + // bundled `$ref` is a small, non-shared literal pointer object, handled separately by the + // `qualifyRef`-rewrite tests below). Reached once as a raw allOf branch (must stay unmarked — see + // unevaluated-properties.ts's applicator classification) and once nested under a sibling's + // properties (must be marked). A visited-Set that only tracks "already seen" — not "seen under + // which context" — lets whichever context visits first silently decide for both, dropping the + // second context's mark. it('marks a schema reached via two different composition contexts independently, not first-visit-wins', () => { const shared = { type: 'object', properties: { name: { type: 'string' } } }; const schema = { @@ -211,6 +232,137 @@ describe('assertResponseExact (#640) — exact-key-match against openapi.yaml', }); }); +// #649 — loadSpec() switched from $RefParser.dereference to .bundle so ajv can compile the +// self-referential SpecNode/SpecTree response schemas six operations' success bodies embed +// (previously a real circular JS object blew ajv's compile-time traversal stack). These pin the +// mechanism directly: a bundled `$ref` is a literal pointer object, never inlined, and +// markUnevaluatedPropertiesFalse's new `qualifyRef`/`inPlace` option rewrites it per LOCAL context. +describe('markUnevaluatedPropertiesFalse — $ref qualification (#649)', () => { + it('rewrites a $ref via the qualifyRef callback and leaves the pointer node itself unmarked', () => { + const schema = { properties: { child: { $ref: '#/components/schemas/Foo' } } }; + const marked = markUnevaluatedPropertiesFalse(schema, { + qualifyRef: (ref, inPlace) => `${inPlace ? 'in' : 'child'}:${ref}`, + }) as { properties: { child: { $ref: string; unevaluatedProperties?: boolean } } }; + expect(marked.properties.child.$ref).toBe('child:#/components/schemas/Foo'); + // A $ref-only node evaluates no properties of its own — marking it directly would be a false + // positive (it isn't the node that actually owns the properties it points at). + expect(marked.properties.child.unevaluatedProperties).toBeUndefined(); + }); + + it('defaults qualifyRef to identity, matching every pre-#649 call site byte-for-byte', () => { + const schema = { properties: { child: { $ref: '#/x' } } }; + const marked = markUnevaluatedPropertiesFalse(schema) as { + properties: { child: { $ref: string } }; + }; + expect(marked.properties.child.$ref).toBe('#/x'); + }); + + // The sharpest correctness pitfall the dual-mirror design found (see the PR description): a + // CHILD-context keyword's `$ref` must be qualified with CHILD context even when the keyword + // itself sits inside an allOf branch being walked under IN_PLACE context — `walkSubschemas` + // hardcodes each keyword's context by category, never by the caller's own context, and this is + // what makes that true. Getting it backwards would let an unqualified/mis-qualified ref + // accidentally self-resolve against the WRONG mirror document by URI-shape coincidence rather + // than failing loudly (confirmed during implementation: this exact mutation didn't fail against + // real openapi.yaml schemas by coincidence, which is why this hand-built, ref-parser-independent + // case exists — a real-spec-driven test can pass by accident where a hand-built one can't). + it("qualifies a $ref inside `items` nested in an allOf branch using CHILD context, never leaking the branch's own IN_PLACE context", () => { + const contexts: boolean[] = []; + const qualifyRef = (ref: string, inPlace: boolean): string => { + contexts.push(inPlace); + return `${inPlace ? 'IN_PLACE' : 'CHILD'}:${ref}`; + }; + const schema = { + allOf: [ + { + type: 'object', + properties: { + list: { type: 'array', items: { $ref: '#/components/schemas/Foo' } }, + }, + }, + ], + }; + const marked = markUnevaluatedPropertiesFalse(schema, { qualifyRef }) as { + allOf: [{ properties: { list: { items: { $ref: string } } } }]; + }; + expect(marked.allOf[0].properties.list.items.$ref).toBe('CHILD:#/components/schemas/Foo'); + expect(contexts).toEqual([false]); // never called with inPlace:true despite the allOf branch + }); + + it('buildMirrorQualifyRef throws on a non-local $ref instead of silently mis-qualifying it', () => { + expect(() => + markUnevaluatedPropertiesFalse( + { properties: { child: { $ref: 'https://example.com/foo' } } }, + { qualifyRef: buildMirrorQualifyRef() } + ) + ).toThrow(/unsupported non-local/); + }); +}); + +// The core bug this issue fixes, pinned directly: loadSpec()'s bundled document + getValidator's +// $ref-qualification must compile and validate a genuinely self-referential SpecNode payload with +// zero RangeError — no DB, no HTTP, just the compiled ajv validator against the real openapi.yaml. +describe('#649: self-referential SpecNode/SpecTree schemas compile and validate', () => { + it('getValidator compiles POST /specs/{id}/paragraphs’s 201 schema and validates a 200-deep synthetic SpecNode without RangeError', async () => { + const doc = await loadSpec(); + const schema = resolveResponseSchema(doc, 'post', '/specs/{id}/paragraphs', 201); + if (schema === undefined) throw new Error('expected a documented application/json schema'); + const validate = getValidator(schema); + const body = { success: true, data: buildDeepSpecNode(200) }; + expect(() => validate(body)).not.toThrow(); + expect(validate(body)).toBe(true); + }); + + it.each([ + ['get', '/specs/{id}', 200] as const, + ['post', '/specs/{id}/paragraphs', 201] as const, + ['patch', '/specs/{id}/paragraphs/{nodeId}', 200] as const, + ['patch', '/specs/{id}/paragraphs/{nodeId}/removal', 200] as const, + ['patch', '/specs/{id}/paragraphs/{nodeId}/reject', 200] as const, + ['get', '/revisions/{id}', 200] as const, + ])( + 'the six previously-unvalidated operations (%s %s) each compile via getValidator', + async (method, path, status) => { + const doc = await loadSpec(); + const schema = resolveResponseSchema(doc, method, path, status); + if (schema === undefined) throw new Error('expected a documented application/json schema'); + expect(() => getValidator(schema)).not.toThrow(); + } + ); + + // The false-rejection pitfall the dual-mirror design exists to avoid: SuccessResponse is an + // allOf-envelope branch for EVERY response, so a single "mark every component once, standalone" + // mirror makes IN_PLACE_MIRROR's SuccessResponse entry ALSO carry `unevaluatedProperties: false` + // — and a standalone SuccessResponse only "sees" its own `success` key, never a sibling allOf + // branch's `data` key, so it would reject a fully clean, fully-documented payload. Driving a real + // op's exact-match check against a genuinely clean body proves the two-mirror split avoids this. + it('assertResponseExact accepts a clean SuccessResponse-enveloped SpecNode payload (no false rejection)', async () => { + const body = { + success: true, + data: { id: NODE_ID, type: 'pr1', text: 'clean', children: [], meta: {} }, + }; + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, body) + ).resolves.toBeUndefined(); + }); + + it('assertResponseExact rejects an undocumented key nested inside SpecNode.children, not just at the top level', async () => { + const body = { + success: true, + data: { + id: NODE_ID, + type: 'pr1', + text: 'parent', + children: [{ id: NODE_ID, type: 'pr2', text: 'child', children: [], meta: {}, rogue: 1 }], + meta: {}, + }, + }; + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, body) + ).rejects.toThrow(/does not document/); + }); +}); + // #640 adversarial-review follow-ups. The walker's job is to make the gate strict WITHOUT making it // wrong: a vacuous branch lets an undocumented key through silently (the bug #640 exists to kill), // while a false positive rejects a fully-documented payload and gets the gate weakened or reverted. @@ -269,6 +421,20 @@ describe('markUnevaluatedPropertiesFalse — JSON-Schema 2020-12 applicator cove valid: { any: { known: 'v' } }, extra: { any: { known: 'v', rogue: 1 } }, }, + { + // #649: `items` is the CHILD_SINGLES entry that actually matters most — it's what recurses + // into SpecNode's own `children: SpecNode[]` array — yet no case here exercised it directly + // (only its cousins prefixItems/contains/unevaluatedItems were covered), so the issue's + // mandated mutation-verify bar ("remove one entry from CHILD_SINGLES, confirm the pinning + // test goes red") had nothing to go red for `items` specifically. + keyword: 'items', + schema: { + type: 'array', + items: { type: 'object', properties: { a: { type: 'string' } } }, + }, + valid: [{ a: 'x' }], + extra: [{ a: 'x', rogue: 1 }], + }, { keyword: 'prefixItems', schema: { From 57616bec391ee202acaa6b3b61ef10a69f25cb18 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 15:06:54 -0700 Subject: [PATCH 05/10] test(contract): pin sweep-vacuity and per-op depth invariants for #649 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two invariants from the bundle-switch design weren't directly pinned yet: - successJsonOps() fed every 2xx response through OperationObject, which parses `$ref` responses into a content-less shape before the loop that checks for `application/json` ever sees them. No operation in today's openapi.yaml documents a 2xx response as a bare `$ref` (only 4xx/5xx do), but bundling leaves that door open — such an operation would silently drop out of successJsonOps' output and pass the "response-covered or allowlisted" sweep vacuously (never appearing in either RESPONSE_COVERED or the failing "uncovered" list). Fixed by resolving one level of `$ref` via resolveIfRef before the has2xxJson check, using a new raw-responses schema so the `$ref` key survives long enough to resolve. Pinned against a synthetic doc, matching this file's existing vacuity-guard pattern. - assertResponseExact's "rejects an undocumented key at any depth" claim was only integration-tested at the top level for four of the six #649 operations (POST .../paragraphs, PATCH .../nodeId, .../removal, .../reject) — only the two SpecTree-wrapping ops (GET /specs/{id}, GET /revisions/{id}) had a nested-depth splice. Extends each of the four direct-SpecNode-response ops with a splice nested one level down inside `data.children`, driven through the same real HTTP round trip as their existing top-level splice assertions. Co-Authored-By: Claude Sonnet 5 --- src/api/contract.integration.test.ts | 38 +++++++++++++++++ .../contract/validate-response.test.ts | 41 +++++++++++++++++++ src/test-utils/contract/validate-response.ts | 21 ++++++++-- 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/src/api/contract.integration.test.ts b/src/api/contract.integration.test.ts index 435589d0..aaeb58db 100644 --- a/src/api/contract.integration.test.ts +++ b/src/api/contract.integration.test.ts @@ -17,6 +17,15 @@ import { MAX_LITERAL_TERM_LENGTH } from '../ast/index.js'; // MCP is registered separately (not on `router`); exclude defensively. const EXCLUDE = new Set(['post /mcp', 'get /mcp', 'delete /mcp']); +// #649: a splice at the top level of `data` alone doesn't prove INV-6 reaches every nesting depth a +// SpecNode response can carry — its own `children: SpecNode[]` is self-referential. Each of the +// four direct-SpecNode-response ops below returns a fixture paragraph with an empty `children` +// array, so this synthesizes one leaf child (otherwise-valid) carrying the undocumented key, to +// prove assertResponseExact's recursive mirror walk reaches into it rather than stopping at `data`. +function rogueChild(id: string): Record { + return { id, type: 'pr1', text: 'nested', children: [], meta: {}, rogueKey: 'nope' }; +} + // Response bodies asserted in this file. const RESPONSE_COVERED = new Set([ 'delete /projects/{}/revision-nomenclature', @@ -660,6 +669,14 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D data: { ...editBody.data, rogueKey: 'nope' }, }) ).rejects.toThrow(/does not document/); + // #649: the same undocumented key, nested one level down inside `data.children` — proves the + // rejection isn't limited to the top level of this self-referential SpecNode response. + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, { + ...editBody, + data: { ...editBody.data, children: [rogueChild(paragraphId)] }, + }) + ).rejects.toThrow(/does not document/); // 4. The spec pending-summary reflects exactly that one pending paragraph. const specPending = await fetch(`${baseUrl}/specs/${specId}/pending-summary`); @@ -725,6 +742,13 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D data: { ...(rejectBody.data as object), rogueKey: 'nope' }, }) ).rejects.toThrow(/does not document/); + // #649: nested one level down inside `data.children` — see rogueChild's comment above. + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/reject', 200, { + ...rejectBody, + data: { ...(rejectBody.data as object), children: [rogueChild(paragraphId)] }, + }) + ).rejects.toThrow(/does not document/); // 8. GET /specs/{id} returns the full SpecTree — matches its documented schema exactly, and // rejects a spliced undocumented key both at the top level (the data wrapper) and nested two @@ -779,6 +803,13 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D data: { ...insertedBody.data, rogueKey: 'nope' }, }) ).rejects.toThrow(/does not document/); + // #649: nested one level down inside `data.children` — see rogueChild's comment above. + await expect( + assertResponseExact('post', '/specs/{id}/paragraphs', 201, { + ...insertedBody, + data: { ...insertedBody.data, children: [rogueChild(insertedBody.data.id)] }, + }) + ).rejects.toThrow(/does not document/); const insertedNodeId = insertedBody.data.id; // 10. PATCH .../paragraphs/{nodeId}/removal toggles the just-inserted paragraph's `vanish` @@ -801,6 +832,13 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D data: { ...removalBody.data, rogueKey: 'nope' }, }) ).rejects.toThrow(/does not document/); + // #649: nested one level down inside `data.children` — see rogueChild's comment above. + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/removal', 200, { + ...removalBody, + data: { ...removalBody.data, children: [rogueChild(insertedNodeId)] }, + }) + ).rejects.toThrow(/does not document/); }); it('GET /revisions/{id} matches its documented schema exactly, including its embedded SpecTree', async () => { diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index cb4f8e7b..243e322c 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -540,6 +540,47 @@ describe('resolveResponseSchema vacuity guards (#640)', () => { }); }); +// #649: loadSpec() switched from $RefParser.dereference to .bundle, which leaves every `$ref` — +// including a 2xx RESPONSE OBJECT itself, not just schema fields inside it — as a literal +// `{ $ref }` pointer instead of an inlined object. successJsonOps() feeds the "every success-JSON +// operation is response-covered or explicitly allowlisted" sweep (contract.integration.test.ts): +// an operation successJsonOps silently fails to count never appears in EITHER RESPONSE_COVERED or +// the "uncovered" failure list, so the sweep passes vacuously for it — exactly the failure mode +// #640's fail-loud guards elsewhere in this file exist to prevent. No operation in today's +// openapi.yaml documents a 2xx response as a bare `$ref` to `components/responses/*` (only 4xx/5xx +// do), so this is pinned against a synthetic doc, matching the vacuity-guard pattern above. +describe('successJsonOps — sweep vacuity under a $ref-pointer 2xx response (#649)', () => { + it('still counts a 2xx response that is itself a $ref to components/responses/*, not silently dropped by the bundle switch', () => { + const doc: OpenApiDoc = { + paths: { + '/synthetic': { + get: { responses: { '200': { $ref: '#/components/responses/Ok' } } }, + }, + }, + components: { + responses: { + Ok: { content: { 'application/json': { schema: { type: 'object' } } } }, + }, + }, + }; + expect(successJsonOps(doc)).toContain('get /synthetic'); + }); + + it('still excludes a $ref-pointer 2xx response with no application/json content', () => { + const doc: OpenApiDoc = { + paths: { + '/synthetic': { + get: { responses: { '200': { $ref: '#/components/responses/NoContent' } } }, + }, + }, + components: { + responses: { NoContent: {} }, + }, + }; + expect(successJsonOps(doc)).not.toContain('get /synthetic'); + }); +}); + // operationParamKeys() feeds INV-4; anything it silently under-reports becomes an INV-4 check that // passes vacuously. Synthetic docs (no such op exists in openapi.yaml yet) pin the two ways that // could happen: a body carrying BOTH a base `properties` map and `oneOf` branches, and a body diff --git a/src/test-utils/contract/validate-response.ts b/src/test-utils/contract/validate-response.ts index 67e79301..0baf9b5a 100644 --- a/src/test-utils/contract/validate-response.ts +++ b/src/test-utils/contract/validate-response.ts @@ -48,6 +48,17 @@ const ResponseObject = z.object({ const OperationObject = z.object({ responses: z.record(z.string(), ResponseObject).optional(), }); +// Raw (possibly `$ref`'d — e.g. `#/components/responses/BadRequest`) response entries, resolved +// one at a time via {@link resolveIfRef} in successJsonOps() before parsing into ResponseObject. +// OperationObject above parses every response through ResponseObject up front, which strips a +// `$ref` key immediately (ResponseObject has no `$ref` field) — successJsonOps needs the pointer +// to survive long enough to resolve it, so it narrows through this raw shape instead (#649: no 2xx +// response in today's openapi.yaml is `$ref`'d this way, only 4xx/5xx are, but bundling leaves the +// door open and a silently-dropped 2xx op would make the response-covered/allowlist sweep pass +// vacuously for it). +const RawOperationObject = z.object({ + responses: z.record(z.string(), z.unknown()).optional(), +}); // Request-body schema, narrowed only to the shape operationParamKeys() reads: either a direct // `properties` map, or (for the request bodies that are a top-level `$ref` to a component, or @@ -299,10 +310,12 @@ export function operationPathTemplates(doc: OpenApiDoc): ReadonlyMap status.startsWith('2') && r.content?.['application/json'] !== undefined - ); + const op = RawOperationObject.parse(raw); + const has2xxJson = Object.entries(op.responses ?? {}).some(([status, rawResponse]) => { + if (!status.startsWith('2')) return false; + const response = ResponseObject.parse(resolveIfRef(doc, rawResponse)); + return response.content?.['application/json'] !== undefined; + }); if (has2xxJson) out.push(`${method} ${normalizePath(path)}`); } return out; From c0c30680914348fc82c6380bc5c1b1ac527e07e8 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 15:15:30 -0700 Subject: [PATCH 06/10] test(contract): pin local $ref context + six-op assertResponseExact acceptance (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes two acceptance-criteria gaps left after the bundle switch: (1) the six-op it.each only proved each response schema COMPILES via getValidator, never that assertResponseExact actually accepts a real, fully-documented payload end-to-end for all six; (2) the LOCAL-walking-context invariant (a $ref's mirror qualification is decided per-occurrence, never by the component's identity) had only a hand-built regression, no real-spec-driven one. Adds a six-op assertResponseExact acceptance sweep (folding the prior single-op SuccessResponse+SpecNode accept test into it), and a dedicated describe block driving DELETE /specs/{id}/lock — a real op where SuccessResponse is referenced bare (CHILD context) instead of via its usual allOf branch (IN_PLACE context) — proving the same component is qualified correctly in both real contexts, not just the hand-built ones. Mutation-verified: inverting buildMirrorQualifyRef's inPlace ternary turned all 9 new tests red (plus the pre-existing /health accept test); reverting restored all 70/70 green. Co-Authored-By: Claude Sonnet 5 --- .../contract/validate-response.test.ts | 116 ++++++++++++++++-- 1 file changed, 106 insertions(+), 10 deletions(-) diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index 243e322c..9996d133 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -29,6 +29,40 @@ function buildDeepSpecNode(depth: number): unknown { return node; } +/** A minimal, fully-documented SpecNode — every field either required by the schema or a real + * optional the openapi.yaml SpecNode component declares. Shared by the six-op acceptance sweep + * below (#649) so each op's worked example stays legible instead of re-deriving the shape. */ +function cleanSpecNode(overrides: Record = {}): Record { + return { id: NODE_ID, type: 'pr1', text: 'clean', children: [], meta: {}, ...overrides }; +} + +/** A minimal, fully-documented SpecTree (GET /specs/{id}'s `data`) — required fields only, plus + * one real SpecNode child so the SpecTree → SpecNode $ref chain is actually exercised. */ +function cleanSpecTree(): Record { + return { id: NODE_ID, section: '09 91 26', title: 'Sample Section', parts: [cleanSpecNode()] }; +} + +/** A minimal, fully-documented RevisionWithTrees (GET /revisions/{id}'s `data`) — every required + * field, with one RevisionSpecEntry whose `tree` is a real SpecTree so the deepest real $ref chain + * in openapi.yaml (RevisionWithTrees → RevisionSpecEntry → SpecTree → SpecNode) is exercised. */ +function cleanRevisionWithTrees(): Record { + return { + revisionId: NODE_ID, + packageId: NODE_ID, + label: 'Rev A', + displayName: 'Revision A', + type: 'draft', + date: '2026-01-01', + sortOrder: 1, + number: null, + attributes: {}, + issuedAt: '2026-01-01T00:00:00Z', + specs: [{ specId: NODE_ID, position: 1, tree: cleanSpecTree() }], + parentRevisionId: null, + baseRevisionId: null, + }; +} + const HTTP_METHODS = ['get', 'post', 'put', 'patch', 'delete'] as const; // Narrow shape for the raw (un-dereferenced) request-body schema a #377 write op documents. @@ -336,16 +370,8 @@ describe('#649: self-referential SpecNode/SpecTree schemas compile and validate' // — and a standalone SuccessResponse only "sees" its own `success` key, never a sibling allOf // branch's `data` key, so it would reject a fully clean, fully-documented payload. Driving a real // op's exact-match check against a genuinely clean body proves the two-mirror split avoids this. - it('assertResponseExact accepts a clean SuccessResponse-enveloped SpecNode payload (no false rejection)', async () => { - const body = { - success: true, - data: { id: NODE_ID, type: 'pr1', text: 'clean', children: [], meta: {} }, - }; - await expect( - assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}', 200, body) - ).resolves.toBeUndefined(); - }); - + // (The accept case itself is now one row of the six-op sweep below; this test asserts the + // rejection half, which the sweep — accept-only by design — doesn't cover.) it('assertResponseExact rejects an undocumented key nested inside SpecNode.children, not just at the top level', async () => { const body = { success: true, @@ -363,6 +389,76 @@ describe('#649: self-referential SpecNode/SpecTree schemas compile and validate' }); }); +// #649 acceptance criterion 4: "Confirm the six ops above now actually validate — drive each and +// assert a real response passes." The it.each above only proves each schema COMPILES; this drives +// assertResponseExact — the strictest of the two checks (INV-6, #640) — against a real, +// fully-documented payload for every one of the six, closing the gap between "compiles" and +// "a real response is actually accepted end-to-end". A regression that mis-registered even one +// mirror entry, or mis-qualified a single `$ref` along one of these ops' real (sometimes multi-hop: +// RevisionWithTrees → RevisionSpecEntry → SpecTree → SpecNode) reference chains, would surface here +// as a false rejection — a failure mode a compile-only check cannot see. +describe('assertResponseExact accepts a real payload for all six previously-unvalidated operations (#649)', () => { + it.each([ + ['get', '/specs/{id}', 200, { success: true, data: cleanSpecTree() }] as const, + ['post', '/specs/{id}/paragraphs', 201, { success: true, data: cleanSpecNode() }] as const, + [ + 'patch', + '/specs/{id}/paragraphs/{nodeId}', + 200, + { success: true, data: cleanSpecNode() }, + ] as const, + [ + 'patch', + '/specs/{id}/paragraphs/{nodeId}/removal', + 200, + { success: true, data: cleanSpecNode() }, + ] as const, + [ + 'patch', + '/specs/{id}/paragraphs/{nodeId}/reject', + 200, + { success: true, data: cleanSpecNode() }, + ] as const, + ['get', '/revisions/{id}', 200, { success: true, data: cleanRevisionWithTrees() }] as const, + ])( + '%s %s (%i) accepts its fully-documented worked example', + async (method, path, status, body) => { + await expect(assertResponseExact(method, path, status, body)).resolves.toBeUndefined(); + } + ); +}); + +// #649 — the LOCAL-walking-context invariant, proven against a REAL dual-context $ref rather than +// a hand-built one: SuccessResponse is referenced from openapi.yaml in TWO genuinely different +// contexts. At ~130 other ops it is always an allOf BRANCH (IN_PLACE — never marked directly, see +// unevaluated-properties.ts's applicator classification). But DELETE /specs/{id}/lock's 200 +// response schema is a bare `$ref: SuccessResponse` with no allOf wrapper at all — SuccessResponse +// is the response schema itself (CHILD context — the top-level call), and MUST be marked directly +// there, or an undocumented key on this one op would silently pass. Qualification keyed on the +// component's identity alone (rather than the LOCAL context of each individual `$ref` occurrence) +// would make this op inherit whichever mirror the other ~130 in-place occurrences last touched, +// and either always reject (breaking every allOf-composed response) or always accept (reopening +// #640 for this op specifically) — this is the real-spec case the hand-built qualifyRef tests above +// exist to generalize from. +describe('assertResponseExact — SuccessResponse in two real contexts proves LOCAL $ref qualification (#649)', () => { + it('rejects an undocumented key where SuccessResponse is the bare top-level schema (CHILD context)', async () => { + await expect( + assertResponseExact('delete', '/specs/{id}/lock', 200, { success: true, extra: 'nope' }) + ).rejects.toThrow(/does not document/); + }); + + it('accepts the exact documented shape for that same bare-SuccessResponse op', async () => { + await expect( + assertResponseExact('delete', '/specs/{id}/lock', 200, { success: true }) + ).resolves.toBeUndefined(); + }); + + it("still permits SuccessResponse's allOf-branch sibling keys elsewhere — same component, IN_PLACE context, never marked directly", async () => { + const body = { success: true, data: { db: 'connected', uptime: 5 } }; + await expect(assertResponseExact('get', '/health', 200, body)).resolves.toBeUndefined(); + }); +}); + // #640 adversarial-review follow-ups. The walker's job is to make the gate strict WITHOUT making it // wrong: a vacuous branch lets an undocumented key through silently (the bug #640 exists to kill), // while a false positive rejects a fully-documented payload and gets the gate weakened or reverted. From 693215d1e50bfb9aa8bfa30850309be18e151b04 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 15:47:05 -0700 Subject: [PATCH 07/10] fix(test-utils): resolve $ref-pointer 2xx responses in resolveResponseSchema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveResponseSchema parsed a response entry straight into ResponseObject, which has no `$ref` field — a `$ref`-pointer 2xx response (e.g. `{ $ref: '#/components/responses/Ok' }`) silently lost its pointer and read as `{ content: undefined }`, falling into the "documented non-JSON" no-op branch instead of resolving to its real schema. That reopened the same vacuous-gate class #649 fixed for successJsonOps, one call site over: assertResponse/assertResponseExact would both silently no-op instead of validating the response body at all. Fixed by parsing through RawOperationObject and resolving the specific status's response via resolveIfRef before narrowing into ResponseObject, mirroring the pattern successJsonOps already uses. Pinned with a synthetic-doc regression test (no 2xx response in today's openapi.yaml is $ref'd this way, only 4xx/5xx are). Co-Authored-By: Claude Sonnet 5 --- .../contract/validate-response.test.ts | 26 +++++++++++++++++ src/test-utils/contract/validate-response.ts | 28 +++++++++++-------- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index 9996d133..0dc28fa0 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -677,6 +677,32 @@ describe('successJsonOps — sweep vacuity under a $ref-pointer 2xx response (#6 }); }); +// resolveResponseSchema shares successJsonOps' $ref-pointer 2xx exposure: it parsed the raw response +// entry straight into ResponseObject (no `$ref` field), so a $ref-pointer 2xx response silently lost +// its `$ref` key and read as `{ content: undefined }` — falling into the "documented non-JSON" no-op +// branch instead of resolving the pointer and validating against its real schema. That reopens the +// exact vacuous-gate class #649 fixed for successJsonOps, one call site over: assertResponse and +// assertResponseExact (both built on resolveResponseSchema) would silently no-op for such an +// operation instead of validating the response body at all. No 2xx response in today's openapi.yaml +// is `$ref`'d this way (only 4xx/5xx are), so this is pinned against a synthetic doc. +describe('resolveResponseSchema resolves a $ref-pointer 2xx response (#649)', () => { + it('resolves the $ref and returns its schema, instead of treating it as documented non-JSON', () => { + const doc: OpenApiDoc = { + paths: { + '/synthetic': { + get: { responses: { '200': { $ref: '#/components/responses/Ok' } } }, + }, + }, + components: { + responses: { + Ok: { content: { 'application/json': { schema: { type: 'object' } } } }, + }, + }, + }; + expect(resolveResponseSchema(doc, 'get', '/synthetic', 200)).toEqual({ type: 'object' }); + }); +}); + // operationParamKeys() feeds INV-4; anything it silently under-reports becomes an INV-4 check that // passes vacuously. Synthetic docs (no such op exists in openapi.yaml yet) pin the two ways that // could happen: a body carrying BOTH a base `properties` map and `oneOf` branches, and a body diff --git a/src/test-utils/contract/validate-response.ts b/src/test-utils/contract/validate-response.ts index 0baf9b5a..336d3556 100644 --- a/src/test-utils/contract/validate-response.ts +++ b/src/test-utils/contract/validate-response.ts @@ -45,17 +45,14 @@ const SchemaObject = z.record(z.string(), z.unknown()); const ResponseObject = z.object({ content: z.record(z.string(), z.object({ schema: SchemaObject.optional() })).optional(), }); -const OperationObject = z.object({ - responses: z.record(z.string(), ResponseObject).optional(), -}); // Raw (possibly `$ref`'d — e.g. `#/components/responses/BadRequest`) response entries, resolved -// one at a time via {@link resolveIfRef} in successJsonOps() before parsing into ResponseObject. -// OperationObject above parses every response through ResponseObject up front, which strips a -// `$ref` key immediately (ResponseObject has no `$ref` field) — successJsonOps needs the pointer -// to survive long enough to resolve it, so it narrows through this raw shape instead (#649: no 2xx +// one at a time via {@link resolveIfRef} in successJsonOps() and resolveResponseSchema() before +// parsing into ResponseObject. Parsing a response entry straight into ResponseObject strips a +// `$ref` key immediately (ResponseObject has no `$ref` field) — both callers need the pointer to +// survive long enough to resolve it, so they narrow through this raw shape instead (#649: no 2xx // response in today's openapi.yaml is `$ref`'d this way, only 4xx/5xx are, but bundling leaves the -// door open and a silently-dropped 2xx op would make the response-covered/allowlist sweep pass -// vacuously for it). +// door open and a silently-dropped 2xx op would make the response-covered/allowlist sweep, or +// assertResponse/assertResponseExact themselves, pass vacuously for it). const RawOperationObject = z.object({ responses: z.record(z.string(), z.unknown()).optional(), }); @@ -177,17 +174,24 @@ export function resolveResponseSchema( ): AnySchemaObject | undefined { const rawOp = doc.paths[pathTemplate]?.[method.toLowerCase()]; if (rawOp === undefined) throw new Error(`No OpenAPI operation: ${method} ${pathTemplate}`); - const op = OperationObject.parse(rawOp); + // Parsed via RawOperationObject so a `$ref`-pointer response entry — e.g. + // `{ $ref: '#/components/responses/Ok' }` — survives long enough to resolve below (#649: parsing + // straight into ResponseObject silently strips a `$ref` key, since ResponseObject has no `$ref` + // field, and the stripped entry would read as `{ content: undefined }` — falling into the + // documented-non-JSON no-op below instead of ever reaching the real schema; the same vacuous-gate + // shape successJsonOps was fixed against, one call site over). + const op = RawOperationObject.parse(rawOp); // An UNDOCUMENTED status must fail loud, not fall through to the no-JSON no-op below. Collapsing // the two means a caller pinned to a status openapi.yaml no longer documents (e.g. a 201 changed // to 200) silently validates NOTHING and stays green forever — the gate quietly stops gating. - const response = op.responses?.[String(status)]; - if (response === undefined) { + const rawResponse = op.responses?.[String(status)]; + if (rawResponse === undefined) { throw new Error( `${method} ${pathTemplate} documents no ${status} response in openapi.yaml — the assertion ` + 'would pass vacuously; fix the expected status or document it.' ); } + const response = ResponseObject.parse(resolveIfRef(doc, rawResponse)); const json = response.content?.['application/json']; // No `application/json` media type at all — a documented binary / no-content / multipart // response. There is genuinely nothing to validate, so both callers no-op. From 2f7347d5e1d2ebc1cca2d7cbc085a6b2df046afe Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 16:11:14 -0700 Subject: [PATCH 08/10] test(contract): pin no-mutation invariant for the qualifyRef/inPlace path (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit markUnevaluatedPropertiesFalse's existing no-mutation test only exercised the default (no-options) call shape. Production never calls it that way: assertResponseExact and registerComponentMirrors always pass { inPlace, qualifyRef: buildMirrorQualifyRef() } against doc-owned component schema objects shared for the lifetime of the cached loadSpec() document, so a regression that mutated the original only along that branch would go undetected and corrupt shared schema state process-wide. Adds a mutation-verified regression test exercising the exact qualifyRef/ inPlace shape production uses (confirmed red against an injected mutation bug — the default-path test stayed green under the same regression). Co-Authored-By: Claude Sonnet 5 --- .../contract/validate-response.test.ts | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index 0dc28fa0..6dc7d0dc 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -201,6 +201,42 @@ describe('assertResponseExact (#640) — exact-key-match against openapi.yaml', expect(marked.properties.nested.unevaluatedProperties).toBe(false); }); + // The no-mutation test above only exercises the DEFAULT (no-options) call shape. Production never + // calls it that way: assertResponseExact and registerComponentMirrors (schema-refs.ts) both always + // pass `{ inPlace, qualifyRef: buildMirrorQualifyRef() }` — and registerComponentMirrors runs this + // directly against every doc-owned `components.schemas` entry of the SINGLE cached `loadSpec()` + // document, shared for the lifetime of the process. A regression that mutated the original only + // along the qualifyRef/inPlace branch (e.g. rewriting `node['$ref']` instead of the cloned + // `schema['$ref']` inside markObject) would corrupt that shared component schema for every later + // reader, yet leave the default-options test above green — it never passes qualifyRef or inPlace, + // and its hand-built schema has no `$ref` at all, so it can't reach that branch. + it.each([false, true])( + 'never mutates its input via the qualifyRef/inPlace path production actually calls (inPlace=%s)', + (inPlace) => { + const original = { + type: 'object', + properties: { + child: { $ref: '#/components/schemas/Foo' }, + nested: { type: 'object', properties: { inner: { type: 'string' } } }, + }, + }; + const snapshot = structuredClone(original); + + const marked = markUnevaluatedPropertiesFalse(original, { + inPlace, + qualifyRef: buildMirrorQualifyRef(), + }) as { + properties: { child: { $ref: string }; nested: { unevaluatedProperties?: boolean } }; + }; + + expect(original).toEqual(snapshot); // input object graph is byte-for-byte untouched + expect(marked).not.toBe(original); // caller always gets an independent clone + // The clone itself IS rewritten — proving this isn't a no-op qualifyRef that happens to + // dodge the mutation question entirely. + expect(marked.properties.child.$ref).not.toBe('#/components/schemas/Foo'); + } + ); + // Pins the walker's SHARED-OBJECT-IDENTITY cycle guard (SeenContexts in unevaluated-properties.ts) // — a mechanism orthogonal to, and unaffected by, #649's switch from dereference to bundle. It has // nothing to do with `$ref` strings: it's a hand-built schema where ONE JS object (`shared`) is From 08bb5454300661125d5508617ca9873dc472cc66 Mon Sep 17 00:00:00 2001 From: thewrz Date: Tue, 4 Aug 2026 19:16:49 -0700 Subject: [PATCH 09/10] test(contract): kill the surviving CHILD_SINGLES mutant (unevaluatedProperties) The #649 mutation-verify sweep over the walker's applicator lists found exactly one surviving mutant: deleting 'unevaluatedProperties' from CHILD_SINGLES left the entire suite green, so that traversal edge was unpinned and could have been dropped without any gate going red. Every other keyword in CHILD_SINGLES/CHILD_MAPS/IN_PLACE_MAPS already had a case in the applicator-coverage table (items 2 red, contains/additionalProperties/ unevaluatedItems/dependentSchemas/patternProperties 1 red each). This adds the object-side twin of the existing unevaluatedItems case. The edge is worth pinning for a second reason beyond list completeness: shouldMark deliberately refuses to mark a node that DECLARES unevaluatedProperties (an openness decision openapi.yaml already made). That refusal is correct for the declaring node but must not stop the walker descending into the schema-valued subschema, which sits at a CHILD instance location and closes normally. Red/green verified: passes on current code (74/74); with the keyword removed from CHILD_SINGLES this exact case is the single failure (1 failed | 73 passed). Co-Authored-By: Claude Opus 5 (1M context) --- .../contract/validate-response.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index 6dc7d0dc..e5df7529 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -598,6 +598,25 @@ describe('markUnevaluatedPropertiesFalse — JSON-Schema 2020-12 applicator cove valid: ['head', { a: 'x' }], extra: ['head', { a: 'x', rogue: 1 }], }, + { + // #649 mutation-verify sweep: `unevaluatedProperties` was the ONE entry in CHILD_SINGLES + // whose removal left the whole suite green — the surviving mutant this case kills. It is the + // object-side twin of the `unevaluatedItems` case above (both are zero-occurrence in today's + // openapi.yaml), and it has a second, subtler reason to exist: `shouldMark` deliberately + // refuses to mark a node that DECLARES `unevaluatedProperties`, treating it as an openness + // decision openapi.yaml already made. That refusal is correct for the declaring node, but it + // must not stop the walker descending INTO the schema-valued subschema, which sits at a CHILD + // instance location and closes normally. Drop the keyword from CHILD_SINGLES and `extra` + // below starts passing — an undocumented key accepted one level down. + keyword: 'unevaluatedProperties (schema-valued)', + schema: { + type: 'object', + properties: { known: { type: 'string' } }, + unevaluatedProperties: { type: 'object', properties: { a: { type: 'string' } } }, + }, + valid: { known: 'head', other: { a: 'x' } }, + extra: { known: 'head', other: { a: 'x', rogue: 1 } }, + }, { // CodeRabbit #645: the walker already treated `dependentSchemas` as an in-place applicator // and `evaluatesProperties` already counted it, but nothing exercised that traversal — From 9857806d85f404e20adc3687d1b3e4903193fa32 Mon Sep 17 00:00:00 2001 From: thewrz Date: Wed, 5 Aug 2026 01:31:16 -0700 Subject: [PATCH 10/10] test(contract): close four review nitpicks; per-branch INV-4 vacuity guard (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. validate-response.ts — bodyPropertyKeys could hand INV-4 a PARTIAL key set with no signal. The whole-body fail-loud guard in operationParamKeys fires only when the derived set is completely empty, so a `oneOf` whose other branches still contribute keys masks an underivable branch entirely. RequestBodyBranchObject is a plain z.object (not .strict()), so an allOf/nested-oneOf branch parses cleanly and yields `properties: undefined` rather than throwing — confirmed reachable, not theoretical. Added a per-branch guard, keyed on "could not derive" (properties undefined), NOT on "derived nothing": an explicit `properties: {}` is legal and must still pass. Both directions pinned, and the new guard mutation-verified. 2. contract.integration.test.ts — the package POST and the project_specs INSERT ran BEFORE the try. The package is created first, so an INSERT failure (a re-run where the row already exists) skipped the finally and orphaned the design_packages row — a leak that compounds every run. Both now sit inside the try, with the finally guarding on packageId. 3. unevaluated-properties.ts — the comment claimed the `$ref` branch rewrites "and stop"; the code has no early return and walks on. Harmless (a $ref node carries no other subschema-bearing keywords) but the comment described control flow that does not exist. 4. schema-refs.ts — replaced the `as AnySchemaObject` cast with a real runtime narrowing check, per the repo's no-cross-boundary-assertions rule. DECLINED: extracting a RevisionCore interface. Its stated risk — that divergence would be "misleading before it makes it a compile error" — does not hold: a new RevisionSummary field lands in Omit<> and mapRevisionCore must return it, while a new RevisionWithTrees field breaks the {...core, specs} construction. Either direction is an immediate compile error, and the relationship is already spelled out in the comment above. pnpm lint clean, 3659/3659 unit, contract gate 22/22. Co-Authored-By: Claude Opus 5 --- src/api/contract.integration.test.ts | 43 ++++++++++++------- src/test-utils/contract/schema-refs.ts | 10 ++++- .../contract/unevaluated-properties.ts | 10 +++-- .../contract/validate-response.test.ts | 24 +++++++++++ src/test-utils/contract/validate-response.ts | 22 +++++++++- 5 files changed, 86 insertions(+), 23 deletions(-) diff --git a/src/api/contract.integration.test.ts b/src/api/contract.integration.test.ts index aaeb58db..cf1d7051 100644 --- a/src/api/contract.integration.test.ts +++ b/src/api/contract.integration.test.ts @@ -843,22 +843,27 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D it('GET /revisions/{id} matches its documented schema exactly, including its embedded SpecTree', async () => { const { specId, projectId } = fixture; - const pkg = await fetch(`${baseUrl}/projects/${projectId}/packages`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ name: `contract-revision-pkg-${Date.now()}` }), - }); - expect(pkg.status).toBe(201); - const pkgBody = (await pkg.json()) as { data: { packageId: string } }; - const packageId = pkgBody.data.packageId; - // setPackageSpecs requires every member spec to already be on the project's TOC - // (project_specs) — the checkpoint fixture's spec carries a `project_id` column but was - // inserted directly via SQL, never added to project_specs. - await pool.query( - 'INSERT INTO project_specs (project_id, spec_id, position) VALUES ($1, $2, 1)', - [projectId, specId] - ); + // Both the package creation and the project_specs INSERT sit INSIDE the try: the package is + // created first, so if the INSERT below throws (a re-run where the row already exists, say), + // a version of this test that opened the try afterwards would skip the finally entirely and + // orphan the design_packages row it had just created — a leak that compounds every run. + let packageId: string | undefined; try { + const pkg = await fetch(`${baseUrl}/projects/${projectId}/packages`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ name: `contract-revision-pkg-${Date.now()}` }), + }); + expect(pkg.status).toBe(201); + const pkgBody = (await pkg.json()) as { data: { packageId: string } }; + packageId = pkgBody.data.packageId; + // setPackageSpecs requires every member spec to already be on the project's TOC + // (project_specs) — the checkpoint fixture's spec carries a `project_id` column but was + // inserted directly via SQL, never added to project_specs. + await pool.query( + 'INSERT INTO project_specs (project_id, spec_id, position) VALUES ($1, $2, 1)', + [projectId, specId] + ); const setMembers = await fetch(`${baseUrl}/packages/${packageId}/specs`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, @@ -914,7 +919,13 @@ describe('checkpoint, pending-summary, and paragraph-reject endpoints (ADR-052 D // design_packages (021_create_package_revisions.ts), so deleting the package alone is enough // there. project_specs.spec_id is RESTRICT (007_create_project_specs.ts), so it must be // cleaned up explicitly before the outer afterAll's `DELETE FROM specs`. - await pool.query('DELETE FROM design_packages WHERE id = $1', [packageId]); + // + // packageId is undefined only when the package POST itself threw, in which case there is no + // row to remove; the project_specs delete runs unconditionally because it is id-scoped and + // a no-op when the INSERT never landed. + if (packageId !== undefined) { + await pool.query('DELETE FROM design_packages WHERE id = $1', [packageId]); + } await pool.query('DELETE FROM project_specs WHERE project_id = $1 AND spec_id = $2', [ projectId, specId, diff --git a/src/test-utils/contract/schema-refs.ts b/src/test-utils/contract/schema-refs.ts index fa04b6d6..7ee4e482 100644 --- a/src/test-utils/contract/schema-refs.ts +++ b/src/test-utils/contract/schema-refs.ts @@ -52,7 +52,15 @@ export function resolveIfRef(doc: OpenApiDoc, value: unknown): unknown { * non-local ref instead of silently leaving it unqualified (which would let it accidentally resolve * against the wrong document by URI-shape coincidence). */ export function qualifyRefs(schema: AnySchemaObject, toId: string): AnySchemaObject { - return qualifyRefValue(schema, toId) as AnySchemaObject; + const qualified = qualifyRefValue(schema, toId); + // qualifyRefValue is typed `unknown -> unknown` because it recurses over arbitrary schema + // values. Narrowing with a real runtime check rather than an `as` assertion: the input is an + // object schema and the walk preserves object-ness, so this never fires — but a genuine check + // costs nothing here and keeps the boundary free of a cast the repo's conventions reject. + if (typeof qualified !== 'object' || qualified === null || Array.isArray(qualified)) { + throw new Error('qualifyRefs: expected the qualified result to be an object schema'); + } + return qualified; } function qualifyRefValue(value: unknown, toId: string): unknown { diff --git a/src/test-utils/contract/unevaluated-properties.ts b/src/test-utils/contract/unevaluated-properties.ts index 3032d62e..a278a54f 100644 --- a/src/test-utils/contract/unevaluated-properties.ts +++ b/src/test-utils/contract/unevaluated-properties.ts @@ -204,10 +204,12 @@ function markObject( // leak the sibling's mark across contexts). The shallow copy suffices because every nested key // touched below is REASSIGNED to a brand-new value from a recursive call, never mutated in place. const schema: Record = { ...node }; - // A bundled `$ref` pointer: rewrite it to the mirror-qualified id for THIS local context and stop - // — it has no other subschema-bearing keywords worth walking (siblings like `description` are - // plain data), and `shouldMark` below naturally leaves it unmarked since it evaluates no - // properties of its own (see the `$ref` note in the applicator-classification comment). + // A bundled `$ref` pointer: rewrite it to the mirror-qualified id for THIS local context. The + // walk then CONTINUES through the rest of the node rather than returning early — harmlessly, as + // a `$ref` node carries no other subschema-bearing keywords (siblings like `description` are + // plain data), so the recursion below finds nothing more to rewrite. `shouldMark` also leaves it + // unmarked, since it evaluates no properties of its own (see the `$ref` note in the + // applicator-classification comment). if (typeof schema['$ref'] === 'string') schema['$ref'] = qualifyRef(schema['$ref'], inPlace); // Register the in-progress clone under its context BEFORE recursing, so a schema reached again // under the SAME context (the shared-object-identity case above) returns this clone instead of diff --git a/src/test-utils/contract/validate-response.test.ts b/src/test-utils/contract/validate-response.test.ts index e5df7529..2e1a066e 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -788,6 +788,30 @@ describe('operationParamKeys body-key derivation', () => { const doc = docWithBodySchema({ allOf: [{ properties: { hidden: {} } }] }); expect(() => operationParamKeys(doc, 'post', '/synthetic')).toThrow(/pass vacuously/); }); + + // The whole-body guard above only fires when the derived set ends up EMPTY. A union whose other + // branches still contribute keys keeps body.size non-zero, so an underivable branch would slip + // through and INV-4 would compare a PARTIAL key set with no signal — the same vacuity class, one + // level down. Without the per-branch check this doc derives {ok} and passes silently. + it('throws when only SOME oneOf branches are derivable, rather than checking a partial key set', () => { + const doc = docWithBodySchema({ + oneOf: [{ properties: { ok: {} } }, { allOf: [{ properties: { hidden: {} } }] }], + }); + expect(() => operationParamKeys(doc, 'post', '/synthetic')).toThrow( + /branch \(index 1\) declares no top-level `properties`/ + ); + }); + + // Guards the guard: an explicitly empty `properties: {}` branch IS derivable — it genuinely + // declares no keys — so it must not be mistaken for an underivable composition and must not + // throw. Pins that the check keys on "could not derive", never on "derived nothing". + it('accepts a oneOf branch that explicitly declares an empty properties map', () => { + const doc = docWithBodySchema({ + oneOf: [{ properties: { ok: {} } }, { properties: {} }], + }); + const { body } = operationParamKeys(doc, 'post', '/synthetic'); + expect([...body]).toEqual(['ok']); + }); }); // INV-5 (#403) drives an MCP tool, wraps its BARE payload as the REST envelope diff --git a/src/test-utils/contract/validate-response.ts b/src/test-utils/contract/validate-response.ts index 336d3556..fae727fd 100644 --- a/src/test-utils/contract/validate-response.ts +++ b/src/test-utils/contract/validate-response.ts @@ -344,9 +344,27 @@ function bodyPropertyKeys(doc: OpenApiDoc, rawSchema: unknown): ReadonlySet