diff --git a/src/api/contract.integration.test.ts b/src/api/contract.integration.test.ts index c8ce2d98..af1ce559 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, @@ -16,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', @@ -53,9 +63,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 +79,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 +109,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 +118,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', @@ -647,13 +653,35 @@ 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/); + // #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`); @@ -700,10 +728,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' }, @@ -712,6 +739,203 @@ 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/); + // #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 + // 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/); + // #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` + // 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/); + // #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 () => { + const { specId, projectId } = fixture; + // 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' }, + 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`. + // + // 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/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'); } ); 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 }); diff --git a/src/test-utils/contract/schema-refs.ts b/src/test-utils/contract/schema-refs.ts new file mode 100644 index 00000000..7ee4e482 --- /dev/null +++ b/src/test-utils/contract/schema-refs.ts @@ -0,0 +1,110 @@ +// 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 { + 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 { + 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..a278a54f 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,37 @@ 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. 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 + // 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 +226,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.test.ts b/src/test-utils/contract/validate-response.test.ts index 9765f27a..2e1a066e 100644 --- a/src/test-utils/contract/validate-response.test.ts +++ b/src/test-utils/contract/validate-response.test.ts @@ -8,11 +8,61 @@ 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; +} + +/** 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. @@ -151,12 +201,53 @@ 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. + // 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 + // 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 +302,199 @@ 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. + // (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, + 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/); + }); +}); + +// #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. @@ -269,6 +553,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: { @@ -300,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 — @@ -374,6 +691,73 @@ 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'); + }); +}); + +// 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 @@ -404,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 828e5877..fae727fd 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; @@ -35,33 +45,61 @@ 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() 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, or +// assertResponse/assertResponseExact themselves, 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 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 +112,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 +144,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; } @@ -117,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. @@ -179,7 +243,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 @@ -242,10 +314,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; @@ -256,17 +330,41 @@ 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 ?? []) { - for (const key of Object.keys(branch.properties ?? {})) keys.add(key); +// 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 [index, rawBranch] of (schema.oneOf ?? []).entries()) { + const branch = RequestBodyBranchObject.parse(resolveIfRef(doc, rawBranch)); + // Per-BRANCH fail-loud, not just the whole-body one in operationParamKeys + // below. That guard fires only when the derived set ends up completely + // empty, so a union whose OTHER branches still contribute keys keeps + // `body.size` non-zero and hides an underivable branch entirely — INV-4 + // then checks a partial key set with no signal, which is the same vacuity + // this reader exists to prevent, one level down. RequestBodyBranchObject + // is a plain z.object (not .strict()), so an allOf/nested-oneOf branch + // parses cleanly and silently yields `properties: undefined` rather than + // throwing — hence an explicit check. An explicitly empty + // `properties: {}` is DERIVABLE and legal; only "could not derive" fails. + if (branch.properties === undefined) { + throw new Error( + `a requestBody \`oneOf\` branch (index ${index}) declares no top-level \`properties\` — ` + + 'an unsupported composition (allOf, a nested oneOf, a non-object branch) contributes ' + + 'zero keys and would weaken INV-4 without emptying the key set. Extend ' + + 'bodyPropertyKeys() to handle it.' + ); + } + for (const key of Object.keys(branch.properties)) keys.add(key); } return keys; } @@ -274,11 +372,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 +394,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