From ad80d22085fa9a3a08231b4e72eef0efa295da17 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 22:22:37 -0700 Subject: [PATCH 01/16] Add fixture-completeness guard: manifest + coverage invariant Introduces spec/fixtures/manifest.yaml and scripts/check-fixture-coverage.rb (wired into `make check`), guarding the shared JSON fixtures under spec/fixtures/. The guard enforces two things a validate-only check cannot: - Every manifest target (operation- or pointer-based) validates its selected instance against its schema via the recursive required/$ref/array walk, reused from conformance/runner/ruby/schema-walker.rb (not reimplemented). Pointer entries carry an explicit component schema so discriminator-lying projections validate correctly (recordings/get.json as generic Recording, search results as SearchResult, a /content_attachments/0 element as RichTextAttachment). - A coverage invariant: each covered_schemas entry must keep >=1 active target resolving to a concrete, non-null instance declared as that component. Empty arrays and transitive reachability do not count. So a NEW required field on a covered schema is forced into a fixture, closing the gap #408 exposed. Initial covered set: 17 schemas (Comment, Message, Document, Card, Todolist, Upload, ScheduleEntry, Forward, ForwardReply, ClientApproval, ClientCorrespondence, ClientReply, QuestionAnswer, Recording, Todo, SearchResult, RichTextAttachment). Gauge/GaugeNeedle excluded with reason + tracking issue #429 (no shared fixture; inline in gauges_test.go). Completes the Forward/ForwardReply fixtures, which were missing the required title/inherits_status/visible_to_clients fields the guard surfaced. Clarifies the AGENTS.md changed-path stub rule verbatim. --- AGENTS.md | 12 +- Makefile | 13 +- scripts/check-fixture-coverage.rb | 255 ++++++++++++++++++++++++++ scripts/check-fixture-coverage.sh | 14 ++ spec/fixtures/forwards/get.json | 3 + spec/fixtures/forwards/reply_get.json | 3 + spec/fixtures/manifest.yaml | 168 +++++++++++++++++ 7 files changed, 464 insertions(+), 4 deletions(-) create mode 100755 scripts/check-fixture-coverage.rb create mode 100755 scripts/check-fixture-coverage.sh create mode 100644 spec/fixtures/manifest.yaml diff --git a/AGENTS.md b/AGENTS.md index a23b4cf81..9dd45e2e4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -309,9 +309,19 @@ Use `make sync-status` to see upstream diffs since last sync. ### Every Changed Field/Path Requires -1. **Existing tests updated** — every test stubbing a changed path must be updated +1. **Existing tests updated** — every test stubbing a changed path must be updated. + Inline stubs may omit unrelated response fields, but every response stub for a + changed path must include any newly required or behaviorally changed field. 2. **New field tests** — at least one test fixture should include new fields to verify they flow through +> **Automated backstop.** The shared JSON fixtures under `spec/fixtures/` are +> guarded by `make check-fixture-coverage` (`spec/fixtures/manifest.yaml`): every +> manifest'd fixture must carry all required fields of its schema, and every +> `covered_schemas` entry must keep a concrete representative. A new required +> field on a covered schema is therefore forced into a fixture. This guard covers +> only the manifest'd shared fixtures — one-off inline stubs remain the reviewer's +> responsibility under the rule above. + ### Pre-Merge Verification Run `make go-check-drift`, `make kt-check-drift`, and `make py-check-drift` (all included in `make check`) and verify: diff --git a/Makefile b/Makefile index d54185389..6d2882f0a 100644 --- a/Makefile +++ b/Makefile @@ -743,7 +743,7 @@ tools: # Spec-shape lints #------------------------------------------------------------------------------ -.PHONY: check-bucket-flat-parity validate-api-gaps check-deprecation-parity +.PHONY: check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage # Verify every bucket-scoped GET list operation has a flat-path counterpart # (or is justified in spec/bucket-scoped-allowlist.txt). Cross-project SDK @@ -765,6 +765,13 @@ check-deprecation-parity: rb-build validate-api-gaps: @./scripts/validate-api-gaps.sh +# Fixture-completeness guard: every spec/fixtures/manifest.yaml target validates +# against its schema, and every covered schema keeps a concrete active +# representative — so a new required field on a covered schema is forced into a +# fixture. Reuses the conformance schema-walker. +check-fixture-coverage: + @./scripts/check-fixture-coverage.sh + #------------------------------------------------------------------------------ # Combined targets #------------------------------------------------------------------------------ @@ -787,7 +794,7 @@ generate: @echo "==> Generation complete" # Run all checks (Smithy + Go + TypeScript + Ruby + Kotlin + Swift + Python + Behavior Model + Conformance + Provenance + Actions lint) -check: lint-actions sync-spec-version-check smithy-check behavior-model-check provenance-check sync-api-version-check go-check-drift go-check-wrapper-drift auth-routable-check kt-check-drift go-check ts-check rb-check kt-check swift-check py-check conformance check-bucket-flat-parity validate-api-gaps check-deprecation-parity +check: lint-actions sync-spec-version-check smithy-check behavior-model-check provenance-check sync-api-version-check go-check-drift go-check-wrapper-drift auth-routable-check kt-check-drift go-check ts-check rb-check kt-check swift-check py-check conformance check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage @echo "==> All checks passed" # Clean all build artifacts @@ -900,6 +907,6 @@ help: @echo "" @echo "Combined:" @echo " generate Regenerate every machine-derived artifact (Smithy + per-language SDKs + provenance)" - @echo " check Run all checks (Smithy + behavior-model/drift + Go + TypeScript + Ruby + Swift + Kotlin + Python + Conformance + Provenance + API version sync + parity lint + api-gaps + Actions lint)" + @echo " check Run all checks (Smithy + behavior-model/drift + Go + TypeScript + Ruby + Swift + Kotlin + Python + Conformance + Provenance + API version sync + parity lint + api-gaps + fixture-coverage + Actions lint)" @echo " clean Remove all build artifacts" @echo " help Show this help" diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb new file mode 100755 index 000000000..57170be23 --- /dev/null +++ b/scripts/check-fixture-coverage.rb @@ -0,0 +1,255 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# Fixture-completeness guard. +# +# Validates spec/fixtures/manifest.yaml: +# 1. Every `targets` entry resolves — fixture exists, JSON pointer selects a +# concrete non-null instance, the named schema is a real component — and +# the selected instance carries every required field of its schema (the +# recursive $ref/array/required walk, reused from the conformance walker). +# 2. The coverage invariant: every schema in `covered_schemas` has >= 1 active +# target that resolves to a concrete, non-null instance whose declared root +# schema is that component (the concrete-instance rule — transitive +# reachability and empty arrays do not count). +# 3. `excluded_schemas` entries each carry a reason + tracking issue, name real +# components, and do not overlap `covered_schemas`. +# +# Reuses conformance/runner/ruby/schema-walker.rb for validation — the required +# walk is not reimplemented here. Wired into `make check` in the +# scripts/validate-api-gaps.rb style (stdlib only). + +require "json" +require "yaml" + +require_relative "../conformance/runner/ruby/schema-walker" + +PROJECT_ROOT = File.expand_path("..", __dir__) +MANIFEST_FILE = File.join(PROJECT_ROOT, "spec/fixtures/manifest.yaml") +FIXTURES_DIR = File.join(PROJECT_ROOT, "spec/fixtures") +OPENAPI_FILE = File.join(PROJECT_ROOT, "openapi.json") + +errors = [] + +def fail_with(errors, message) + errors << message +end + +# RFC 6901 JSON pointer resolution. Returns [value, found]. "" selects the whole +# document. Missing keys / out-of-range indices / non-container traversal -> +# [nil, false]. +def resolve_pointer(doc, pointer) + return [doc, true] if pointer.nil? || pointer.empty? + return [nil, false] unless pointer.start_with?("/") + + current = doc + pointer.split("/", -1).drop(1).each do |raw| + token = raw.gsub("~1", "/").gsub("~0", "~") + case current + when Hash + return [nil, false] unless current.key?(token) + + current = current[token] + when Array + return [nil, false] unless token.match?(/\A\d+\z/) + + idx = token.to_i + return [nil, false] if idx >= current.length + + current = current[idx] + else + return [nil, false] + end + end + [current, true] +end + +unless File.file?(MANIFEST_FILE) + warn "ERROR: manifest not found at #{MANIFEST_FILE}" + exit 2 +end +unless File.file?(OPENAPI_FILE) + warn "ERROR: openapi.json not found at #{OPENAPI_FILE}" + exit 2 +end + +manifest = YAML.safe_load(File.read(MANIFEST_FILE)) +walker = Basecamp::Conformance::SchemaWalker.new(OPENAPI_FILE) +openapi = JSON.parse(File.read(OPENAPI_FILE)) +components = openapi.dig("components", "schemas") || {} + +targets = manifest["targets"] || [] +covered = manifest["covered_schemas"] || {} +excluded = manifest["excluded_schemas"] || {} + +# Fixture cache so a fixture referenced by several targets is read once. +fixture_cache = {} +load_fixture = lambda do |rel| + fixture_cache[rel] ||= begin + path = File.join(FIXTURES_DIR, rel) + File.file?(path) ? JSON.parse(File.read(path)) : :missing + end +end + +# --- 1. Targets ---------------------------------------------------------------- + +by_id = {} +# Records, per target id, the concrete root schema it resolved to (or nil) so +# the coverage pass can apply the concrete-instance rule without re-resolving. +resolved_root = {} + +targets.each_with_index do |entry, i| + id = entry["id"] + if id.nil? || id.to_s.empty? + fail_with(errors, "targets[#{i}]: missing `id`") + next + end + if by_id.key?(id) + fail_with(errors, "target `#{id}`: duplicate id") + next + end + by_id[id] = entry + + operation = entry["operation"] + fixture_rel = entry["fixture"] + pointer = entry["pointer"] || "" + schema_name = entry["schema"] + + if operation && (schema_name || !fixture_rel) + # operation entry + if schema_name + fail_with(errors, "target `#{id}`: `operation` and `schema` are mutually exclusive") + next + end + end + + # Locate the schema to validate against. + schema = + if operation + s = walker.find_response_schema(operation) + unless s + fail_with(errors, "target `#{id}`: no 2xx/default response schema for operation `#{operation}`") + next + end + s + else + unless schema_name + fail_with(errors, "target `#{id}`: pointer entry needs an explicit `schema`") + next + end + unless components.key?(schema_name) + fail_with(errors, "target `#{id}`: unknown component schema `#{schema_name}`") + next + end + { "$ref" => "#/components/schemas/#{schema_name}" } + end + + # Load fixture (operation entries default pointer ""). + unless fixture_rel + fail_with(errors, "target `#{id}`: missing `fixture`") + next + end + doc = load_fixture.call(fixture_rel) + if doc == :missing + fail_with(errors, "target `#{id}`: fixture not found: spec/fixtures/#{fixture_rel}") + next + end + + instance, found = resolve_pointer(doc, pointer) + unless found + fail_with(errors, "target `#{id}`: JSON pointer `#{pointer}` does not resolve in #{fixture_rel}") + next + end + if instance.nil? + fail_with(errors, "target `#{id}`: pointer `#{pointer}` resolves to null (not a concrete instance) in #{fixture_rel}") + next + end + + # Record the declared root schema for the coverage pass. For pointer entries + # it is the named component; for operation entries, the top-level $ref name. + resolved_root[id] = + if schema_name + { name: schema_name, concrete: instance.is_a?(Hash) } + else + root = schema.is_a?(Hash) ? schema["$ref"] : nil + m = root&.match(%r{/components/schemas/(.+)\z}) + { name: m && m[1], concrete: instance.is_a?(Hash) } + end + + missing = walker.missing_required(instance, schema) + next if missing.empty? + + where = pointer.empty? ? fixture_rel : "#{fixture_rel} at pointer `#{pointer}`" + missing.each do |path| + fail_with(errors, "target `#{id}`: #{where} missing required field `#{path}`") + end +end + +# --- 2. Coverage invariant ----------------------------------------------------- + +covered.each do |schema_name, ids| + unless components.key?(schema_name) + fail_with(errors, "covered_schemas: unknown component schema `#{schema_name}`") + next + end + ids = Array(ids) + if ids.empty? + fail_with(errors, "covered_schemas[`#{schema_name}`]: no target ids listed") + next + end + + concrete_rep = false + ids.each do |id| + unless by_id.key?(id) + fail_with(errors, "covered_schemas[`#{schema_name}`]: references unknown target `#{id}`") + next + end + root = resolved_root[id] + next unless root # target failed to resolve; already reported above + + if root[:name] == schema_name && root[:concrete] + concrete_rep = true + end + end + + unless concrete_rep + fail_with(errors, + "covered_schemas[`#{schema_name}`]: no active target resolves to a concrete, " \ + "non-null instance declared as `#{schema_name}` (concrete-instance rule)") + end +end + +# --- 3. Exclusions ------------------------------------------------------------- + +excluded.each do |schema_name, meta| + unless components.key?(schema_name) + fail_with(errors, "excluded_schemas: unknown component schema `#{schema_name}`") + end + if covered.key?(schema_name) + fail_with(errors, "excluded_schemas[`#{schema_name}`]: schema is also in covered_schemas") + end + meta ||= {} + reason = meta["reason"] + if reason.nil? || reason.to_s.strip.empty? + fail_with(errors, "excluded_schemas[`#{schema_name}`]: missing `reason`") + end + issue = meta["issue"] + if issue.nil? || issue.to_s.strip.empty? + fail_with(errors, "excluded_schemas[`#{schema_name}`]: missing tracking `issue`") + end +end + +# --- Report -------------------------------------------------------------------- + +if errors.empty? + covered_n = covered.size + target_n = targets.size + excluded_n = excluded.size + puts "==> Fixture coverage clean — #{covered_n} covered schemas, " \ + "#{target_n} manifest targets, #{excluded_n} tracked exclusions" + exit 0 +else + warn "Fixture-coverage check failed:" + errors.sort.each { |e| warn " - #{e}" } + exit 1 +end diff --git a/scripts/check-fixture-coverage.sh b/scripts/check-fixture-coverage.sh new file mode 100755 index 000000000..3a7e3a5d2 --- /dev/null +++ b/scripts/check-fixture-coverage.sh @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +# Fixture-completeness guard: validate spec/fixtures/manifest.yaml — every +# manifest target validates against its schema and every covered schema keeps a +# concrete active representative. See scripts/check-fixture-coverage.rb. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +if ! command -v ruby >/dev/null 2>&1; then + echo "ERROR: ruby is required for check-fixture-coverage" >&2 + exit 2 +fi + +exec ruby "$SCRIPT_DIR/check-fixture-coverage.rb" diff --git a/spec/fixtures/forwards/get.json b/spec/fixtures/forwards/get.json index 60b25627e..d07be72b0 100644 --- a/spec/fixtures/forwards/get.json +++ b/spec/fixtures/forwards/get.json @@ -4,6 +4,9 @@ "created_at": "2024-01-16T14:30:00.000Z", "updated_at": "2024-01-16T14:30:00.000Z", "subject": "Project proposal from client", + "title": "Project proposal from client", + "inherits_status": true, + "visible_to_clients": false, "content": "
Hi team,

Please review the attached proposal for the new project.
", "from": "client@example.com", "type": "Inbox::Forward", diff --git a/spec/fixtures/forwards/reply_get.json b/spec/fixtures/forwards/reply_get.json index cfbdb61e9..ca9fd9cc5 100644 --- a/spec/fixtures/forwards/reply_get.json +++ b/spec/fixtures/forwards/reply_get.json @@ -3,6 +3,9 @@ "status": "active", "created_at": "2024-01-16T15:00:00.000Z", "updated_at": "2024-01-16T15:00:00.000Z", + "title": "Re: Project proposal from client", + "inherits_status": true, + "visible_to_clients": false, "content": "
Thanks for forwarding this. I'll review and get back to you by EOD.
", "type": "Inbox::Forward::Reply", "url": "https://3.basecampapi.com/195539477/buckets/2085958499/inbox_forwards/1069479380/replies/1069479400.json", diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml new file mode 100644 index 000000000..3b065fff3 --- /dev/null +++ b/spec/fixtures/manifest.yaml @@ -0,0 +1,168 @@ +# Fixture-completeness manifest + coverage invariant. +# +# Purpose: guarantee that every schema in `covered_schemas` has at least one +# live, validated representative among the shared JSON fixtures — so that when a +# NEW required (or behaviorally-changed) field is added to a covered schema, a +# fixture is forced to carry it and the guard catches the omission. A checker +# that validated only whatever entries happen to exist cannot make that +# guarantee; the coverage invariant closes the gap. +# +# Enforced by scripts/check-fixture-coverage.rb (wired into `make check`). The +# checker reuses conformance/runner/ruby/schema-walker.rb for the recursive +# required-field / $ref / array walk — it does not reimplement validation. +# +# --- Entry semantics ----------------------------------------------------------- +# +# Each `targets` entry is one of: +# +# operation entry — { id, operation: } +# Validates the COMPLETE response body of `operation` (response-schema +# lookup prefers 200, then any 2xx, then default) against the fixture named +# by `fixture` (pointer defaults to ""). +# +# pointer entry — { id, fixture, pointer, schema } +# Validates the object selected by JSON pointer `pointer` within `fixture` +# against the explicit component `schema`. Use this whenever the response +# schema cannot be inferred from an operation, or the discriminator lies: +# * recordings/get.json declares type "Message" on the wire but MUST +# validate as the generic Recording projection, not Message. +# * search/results.json elements declare type "Todo"/"Message"/… but MUST +# validate as SearchResult. +# * a `/content_attachments/0` element is a RichTextAttachment, not the +# array's parent schema. +# `pointer: ""` selects the whole document. `schema` names a component under +# components/schemas. +# +# --- Coverage invariant -------------------------------------------------------- +# +# `covered_schemas` maps each schema this guard promises to keep represented to +# the target id(s) that represent it. The checker enforces, per covered schema: +# * >= 1 non-excluded (active) target maps to it; +# * the concrete-instance rule: at least one mapped active target resolves to +# a concrete, non-null instance whose DECLARED ROOT schema is that component. +# Transitive reachability and empty arrays do NOT count — a Todo fixture with +# "description_attachments": [] reaches RichTextAttachment but instantiates +# none, so it does not cover it; a populated element (pointer +# .../content_attachments/0 declared RichTextAttachment) does. +# +# `excluded_schemas` records schemas deliberately NOT covered this round, each +# with a reason + tracking issue. Exclusions do NOT satisfy coverage. +# +# A missing fixture, unresolvable pointer, unknown schema, or covered schema +# with no active concrete representative -> the check FAILS. + +targets: + # 15 concrete resource GETs — pointer "" -> component schema. + - id: comment-get + fixture: comments/get.json + pointer: "" + schema: Comment + - id: message-get + fixture: messages/get.json + pointer: "" + schema: Message + - id: document-get + fixture: documents/get.json + pointer: "" + schema: Document + - id: card-get + fixture: cards/get.json + pointer: "" + schema: Card + - id: todolist-get + fixture: todolists/get.json + pointer: "" + schema: Todolist + - id: upload-get + fixture: uploads/get.json + pointer: "" + schema: Upload + - id: schedule-entry-get + fixture: schedules/entry_get.json + pointer: "" + schema: ScheduleEntry + - id: forward-get + fixture: forwards/get.json + pointer: "" + schema: Forward + - id: forward-reply-get + fixture: forwards/reply_get.json + pointer: "" + schema: ForwardReply + - id: client-approval-get + fixture: client_approvals/get.json + pointer: "" + schema: ClientApproval + - id: client-correspondence-get + fixture: client_correspondences/get.json + pointer: "" + schema: ClientCorrespondence + - id: client-reply-get + fixture: client_replies/get.json + pointer: "" + schema: ClientReply + - id: question-answer-get + fixture: checkins/answer.json + pointer: "" + schema: QuestionAnswer + - id: recording-get + # Discriminator lies: wire type is "Message"; validate as generic Recording. + fixture: recordings/get.json + pointer: "" + schema: Recording + - id: todo-get + fixture: todos/get.json + pointer: "" + schema: Todo + + # Polymorphic search projection: element declares its recording type, but + # validates as SearchResult. + - id: search-result-0 + fixture: search/results.json + pointer: "/0" + schema: SearchResult + + # Two concrete RichTextAttachment instances (populated elements, not empty + # arrays) — the only way to cover RichTextAttachment per the concrete-instance + # rule. + - id: richtext-comment-0 + fixture: comments/get.json + pointer: "/content_attachments/0" + schema: RichTextAttachment + - id: richtext-upload-0 + fixture: uploads/get.json + pointer: "/description_attachments/0" + schema: RichTextAttachment + +covered_schemas: + Comment: [comment-get] + Message: [message-get] + Document: [document-get] + Card: [card-get] + Todolist: [todolist-get] + Upload: [upload-get] + ScheduleEntry: [schedule-entry-get] + Forward: [forward-get] + ForwardReply: [forward-reply-get] + ClientApproval: [client-approval-get] + ClientCorrespondence: [client-correspondence-get] + ClientReply: [client-reply-get] + QuestionAnswer: [question-answer-get] + Recording: [recording-get] + Todo: [todo-get] + SearchResult: [search-result-0] + RichTextAttachment: [richtext-comment-0, richtext-upload-0] + +excluded_schemas: + Gauge: + reason: >- + No shared JSON fixture. Gauge is exercised only inline in + go/pkg/basecamp/gauges_test.go, so its optional non-nullable + description_attachments array has no manifest'd representative. + issue: 429 + GaugeNeedle: + reason: >- + No shared JSON fixture (same Gauge inline-only situation). GaugeNeedle + carries a @required description_attachments array not represented in any + shared fixture. + issue: 429 From 83ce34f81a4d02401171204c221e12f01bce8208 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 22:36:07 -0700 Subject: [PATCH 02/16] Source TS/Ruby/Python test helpers from shared fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract 3 of the fixture-completeness guard: the named response-body helpers in the non-validating SDKs (TS/Ruby/Python) now derive their body from the manifest'd shared fixture under spec/fixtures/ — one validated source of truth — instead of an independent inline literal that could silently drift from the schema. Each helper keeps its parameterized overrides (id, and the text field tests vary), defaulted to the fixture's own value; assertions that pinned a now-sourced value are updated to the fixture's truth. - TS: static JSON import (resolveJsonModule already on), { ...fixture, id }. - Ruby: load_fixture helper in test_helper.rb, fixture.merge(overrides). - Python: inlined load_fixture (parents[3]), {**fixture, **overrides}; _todo keeps its meaningful assignees/completion_subscribers overlays so the PUT-body flow-through assertions stay strong on top of the fixture shape. Kotlin/Swift are strict decoders — a missing @required field already throws (MissingFieldException / keyNotFound), and their builders already carry the @required *_attachments arrays; the only behaviorally-changed optional fields (Recording's projection arrays) are already asserted in RecordingsServiceTest (absent->null, empty->[], populated). Per Contract 3 they need no change. make ts-check / rb-check / py-check green. --- python/tests/services/test_todos.py | 16 ++++++++-- python/tests/services/test_uploads.py | 13 ++++++-- .../basecamp/services/cards_service_test.rb | 17 +++------- .../services/checkins_service_test.rb | 15 +++------ .../services/client_approvals_service_test.rb | 18 ++++------- .../client_correspondences_service_test.rb | 18 ++++------- .../services/client_replies_service_test.rb | 17 ++++------ .../services/comments_service_test.rb | 15 +++------ .../services/documents_service_test.rb | 17 +++------- .../services/forwards_service_test.rb | 31 ++++++------------- .../services/messages_service_test.rb | 19 ++++-------- .../services/recordings_service_test.rb | 30 +++++++++--------- .../services/schedules_service_test.rb | 18 ++++------- ruby/test/test_helper.rb | 7 +++++ typescript/tests/services/cards.test.ts | 16 ++++------ .../tests/services/client-visibility.test.ts | 13 +++----- typescript/tests/services/comments.test.ts | 15 ++++----- typescript/tests/services/messages.test.ts | 17 ++++------ typescript/tests/services/todolists.test.ts | 17 ++++------ 19 files changed, 134 insertions(+), 195 deletions(-) diff --git a/python/tests/services/test_todos.py b/python/tests/services/test_todos.py index 1531b7565..ffa264542 100644 --- a/python/tests/services/test_todos.py +++ b/python/tests/services/test_todos.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from pathlib import Path import httpx import pytest @@ -14,9 +15,19 @@ BASE = "https://3.basecampapi.com/12345" +_FIXTURES = Path(__file__).resolve().parents[3] / "spec" / "fixtures" + + +def load_fixture(rel: str) -> dict: + return json.loads((_FIXTURES / rel).read_text()) + def _todo(todo_id: int = 42, **overrides) -> dict: - todo = { + # Source the full validated fixture for shape (every required Todo field is + # present), then keep the test-critical override values that the assertions + # verify flow through to the PUT body. + return { + **load_fixture("todos/get.json"), "id": todo_id, "content": "Buy milk", "description": "

From the store

", @@ -25,9 +36,8 @@ def _todo(todo_id: int = 42, **overrides) -> dict: "assignees": [{"id": 100, "name": "Jane Doe"}], "completion_subscribers": [{"id": 555, "name": "Sub Scriber"}], "completed": False, + **overrides, } - todo.update(overrides) - return todo def _put_body(route) -> dict: diff --git a/python/tests/services/test_uploads.py b/python/tests/services/test_uploads.py index a008526b5..d81cd5709 100644 --- a/python/tests/services/test_uploads.py +++ b/python/tests/services/test_uploads.py @@ -2,6 +2,9 @@ from __future__ import annotations +import json +from pathlib import Path + import httpx import pytest import respx @@ -9,14 +12,20 @@ from basecamp import AsyncClient, Client from basecamp.errors import UsageError +_FIXTURES = Path(__file__).resolve().parents[3] / "spec" / "fixtures" + + +def load_fixture(rel: str) -> dict: + return json.loads((_FIXTURES / rel).read_text()) + def _metadata(upload_id: int = 1069479400, *, download_url, filename="report.pdf") -> dict: - """Minimal upload metadata payload; callers override download_url/filename.""" + """Upload metadata sourced from the validated fixture; callers override download_url/filename.""" return { + **load_fixture("uploads/get.json"), "id": upload_id, "filename": filename, "download_url": download_url, - "description_attachments": [], } diff --git a/ruby/test/basecamp/services/cards_service_test.rb b/ruby/test/basecamp/services/cards_service_test.rb index e25d75a7f..7d669371b 100644 --- a/ruby/test/basecamp/services/cards_service_test.rb +++ b/ruby/test/basecamp/services/cards_service_test.rb @@ -14,16 +14,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_card(id: 1, title: "Task Card") - { - "id" => id, - "title" => title, - "content" => "

Card description

", - "description_attachments" => [], - "due_on" => "2024-12-31", - "completed" => false, - "assignees" => [] - } + def sample_card(id: nil, title: nil) + fixture = load_fixture("cards/get.json") + fixture.merge "id" => id || fixture["id"], "title" => title || fixture["title"] end def test_list_cards @@ -33,7 +26,7 @@ def test_list_cards cards = @account.cards.list(column_id: 200).to_a assert_equal 2, cards.length - assert_equal "Task Card", cards[0]["title"] + assert_equal "Implement user authentication", cards[0]["title"] assert_equal "Another Card", cards[1]["title"] end @@ -44,7 +37,7 @@ def test_get_card card = @account.cards.get(card_id: 200) assert_equal 200, card["id"] - assert_equal "Task Card", card["title"] + assert_equal "Implement user authentication", card["title"] end def test_create_card diff --git a/ruby/test/basecamp/services/checkins_service_test.rb b/ruby/test/basecamp/services/checkins_service_test.rb index 0a9018c01..b74db6298 100644 --- a/ruby/test/basecamp/services/checkins_service_test.rb +++ b/ruby/test/basecamp/services/checkins_service_test.rb @@ -32,14 +32,9 @@ def sample_question(id: 1, title: "What did you work on today?") } end - def sample_answer(id: 1, content: "

Making great progress!

") - { - "id" => id, - "content" => content, - "content_attachments" => [], - "creator" => { "id" => 1, "name" => "Test User" }, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_answer(id: nil, content: nil) + fixture = load_fixture("checkins/answer.json") + fixture.merge "id" => id || fixture["id"], "content" => content || fixture["content"] end def test_get_questionnaire @@ -107,7 +102,7 @@ def test_list_answers answers = @account.checkins.list_answers(question_id: 200).to_a assert_equal 2, answers.length - assert_equal "

Making great progress!

", answers[0]["content"] + assert_equal load_fixture("checkins/answer.json")["content"], answers[0]["content"] end def test_get_answer @@ -117,7 +112,7 @@ def test_get_answer answer = @account.checkins.get_answer(answer_id: 200) assert_equal 200, answer["id"] - assert_equal "

Making great progress!

", answer["content"] + assert_equal load_fixture("checkins/answer.json")["content"], answer["content"] end def test_create_answer diff --git a/ruby/test/basecamp/services/client_approvals_service_test.rb b/ruby/test/basecamp/services/client_approvals_service_test.rb index e3073adeb..7a7487dd0 100644 --- a/ruby/test/basecamp/services/client_approvals_service_test.rb +++ b/ruby/test/basecamp/services/client_approvals_service_test.rb @@ -14,15 +14,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_approval(id: 1, subject: "Design Review") - { - "id" => id, - "subject" => subject, - "approval_status" => "pending", - "content" => "

Please review the attached designs.

", - "content_attachments" => [], - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_approval(id: nil, subject: nil) + fixture = load_fixture("client_approvals/get.json") + fixture.merge "id" => id || fixture["id"], "subject" => subject || fixture["subject"] end def test_list_approvals @@ -32,7 +26,7 @@ def test_list_approvals approvals = @account.client_approvals.list.to_a assert_equal 2, approvals.length - assert_equal "Design Review", approvals[0]["subject"] + assert_equal "New logo for the website", approvals[0]["subject"] assert_equal "Budget Approval", approvals[1]["subject"] end @@ -43,7 +37,7 @@ def test_get_approval approval = @account.client_approvals.get(approval_id: 200) assert_equal 200, approval["id"] - assert_equal "Design Review", approval["subject"] - assert_equal "pending", approval["approval_status"] + assert_equal "New logo for the website", approval["subject"] + assert_equal "approved", approval["approval_status"] end end diff --git a/ruby/test/basecamp/services/client_correspondences_service_test.rb b/ruby/test/basecamp/services/client_correspondences_service_test.rb index fadea7ea5..f06dfb914 100644 --- a/ruby/test/basecamp/services/client_correspondences_service_test.rb +++ b/ruby/test/basecamp/services/client_correspondences_service_test.rb @@ -14,15 +14,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_correspondence(id: 1, subject: "Project Update") - { - "id" => id, - "subject" => subject, - "content" => "

Here is the latest update on the project.

", - "content_attachments" => [], - "replies_count" => 3, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_correspondence(id: nil, subject: nil) + fixture = load_fixture("client_correspondences/get.json") + fixture.merge "id" => id || fixture["id"], "subject" => subject || fixture["subject"] end def test_list_correspondences @@ -32,7 +26,7 @@ def test_list_correspondences correspondences = @account.client_correspondences.list.to_a assert_equal 2, correspondences.length - assert_equal "Project Update", correspondences[0]["subject"] + assert_equal "Project kickoff!", correspondences[0]["subject"] assert_equal "Invoice Query", correspondences[1]["subject"] end @@ -43,7 +37,7 @@ def test_get_correspondence correspondence = @account.client_correspondences.get(correspondence_id: 200) assert_equal 200, correspondence["id"] - assert_equal "Project Update", correspondence["subject"] - assert_equal 3, correspondence["replies_count"] + assert_equal "Project kickoff!", correspondence["subject"] + assert_equal 5, correspondence["replies_count"] end end diff --git a/ruby/test/basecamp/services/client_replies_service_test.rb b/ruby/test/basecamp/services/client_replies_service_test.rb index c56ddef75..74e6f6111 100644 --- a/ruby/test/basecamp/services/client_replies_service_test.rb +++ b/ruby/test/basecamp/services/client_replies_service_test.rb @@ -14,14 +14,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_reply(id: 1, content: "

Thank you for the update!

") - { - "id" => id, - "content" => content, - "content_attachments" => [], - "creator" => { "id" => 1, "name" => "Client User" }, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_reply(id: nil, content: nil) + fixture = load_fixture("client_replies/get.json") + fixture.merge "id" => id || fixture["id"], "content" => content || fixture["content"] end def test_list_replies @@ -31,7 +26,7 @@ def test_list_replies replies = @account.client_replies.list(recording_id: 200).to_a assert_equal 2, replies.length - assert_equal "

Thank you for the update!

", replies[0]["content"] + assert_equal load_fixture("client_replies/get.json")["content"], replies[0]["content"] assert_equal "

Looking forward to it!

", replies[1]["content"] end @@ -42,7 +37,7 @@ def test_get_reply reply = @account.client_replies.get(recording_id: 200, reply_id: 300) assert_equal 300, reply["id"] - assert_equal "

Thank you for the update!

", reply["content"] - assert_equal "Client User", reply["creator"]["name"] + assert_equal load_fixture("client_replies/get.json")["content"], reply["content"] + assert_equal "Annie Bryan", reply["creator"]["name"] end end diff --git a/ruby/test/basecamp/services/comments_service_test.rb b/ruby/test/basecamp/services/comments_service_test.rb index 8e2d98e8e..888029a33 100644 --- a/ruby/test/basecamp/services/comments_service_test.rb +++ b/ruby/test/basecamp/services/comments_service_test.rb @@ -15,14 +15,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_comment(id: 1, content: "

Great work!

") - { - "id" => id, - "content" => content, - "content_attachments" => [], - "creator" => { "id" => 1, "name" => "Test User" }, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_comment(id: nil, content: nil) + fixture = load_fixture("comments/get.json") + fixture.merge "id" => id || fixture["id"], "content" => content || fixture["content"] end def test_list_comments @@ -32,7 +27,7 @@ def test_list_comments comments = @account.comments.list(recording_id: 200).to_a assert_equal 2, comments.length - assert_equal "

Great work!

", comments[0]["content"] + assert_equal load_fixture("comments/get.json")["content"], comments[0]["content"] assert_equal "

I agree!

", comments[1]["content"] end @@ -43,7 +38,7 @@ def test_get_comment comment = @account.comments.get(comment_id: 200) assert_equal 200, comment["id"] - assert_equal "

Great work!

", comment["content"] + assert_equal load_fixture("comments/get.json")["content"], comment["content"] end def test_create_comment diff --git a/ruby/test/basecamp/services/documents_service_test.rb b/ruby/test/basecamp/services/documents_service_test.rb index 95648749e..13855586a 100644 --- a/ruby/test/basecamp/services/documents_service_test.rb +++ b/ruby/test/basecamp/services/documents_service_test.rb @@ -14,16 +14,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_document(id: 1, title: "Meeting Notes") - { - "id" => id, - "title" => title, - "content" => "

Notes from today's meeting...

", - "content_attachments" => [], - "status" => "active", - "comments_count" => 2, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_document(id: nil, title: nil) + fixture = load_fixture("documents/get.json") + fixture.merge "id" => id || fixture["id"], "title" => title || fixture["title"] end def test_list_documents @@ -33,7 +26,7 @@ def test_list_documents documents = @account.documents.list(vault_id: 200).to_a assert_equal 2, documents.length - assert_equal "Meeting Notes", documents[0]["title"] + assert_equal "Project Overview", documents[0]["title"] assert_equal "Project Plan", documents[1]["title"] end @@ -44,7 +37,7 @@ def test_get_document document = @account.documents.get(document_id: 200) assert_equal 200, document["id"] - assert_equal "Meeting Notes", document["title"] + assert_equal "Project Overview", document["title"] end def test_create_document diff --git a/ruby/test/basecamp/services/forwards_service_test.rb b/ruby/test/basecamp/services/forwards_service_test.rb index fbec6487a..74310889e 100644 --- a/ruby/test/basecamp/services/forwards_service_test.rb +++ b/ruby/test/basecamp/services/forwards_service_test.rb @@ -22,25 +22,14 @@ def sample_inbox(id: 1) } end - def sample_forward(id: 1, subject: "Client Question") - { - "id" => id, - "subject" => subject, - "from" => "client@example.com", - "content" => "

I have a question about the project.

", - "content_attachments" => [], - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_forward(id: nil, subject: nil) + fixture = load_fixture("forwards/get.json") + fixture.merge "id" => id || fixture["id"], "subject" => subject || fixture["subject"] end - def sample_reply(id: 1, content: "

Thanks for reaching out!

") - { - "id" => id, - "content" => content, - "content_attachments" => [], - "creator" => { "id" => 1, "name" => "Test User" }, - "created_at" => "2024-01-01T00:00:00Z" - } + def sample_reply(id: nil, content: nil) + fixture = load_fixture("forwards/reply_get.json") + fixture.merge "id" => id || fixture["id"], "content" => content || fixture["content"] end def test_get_inbox @@ -60,7 +49,7 @@ def test_list_forwards forwards = @account.forwards.list(inbox_id: 200).to_a assert_equal 2, forwards.length - assert_equal "Client Question", forwards[0]["subject"] + assert_equal "Project proposal from client", forwards[0]["subject"] assert_equal "Another Email", forwards[1]["subject"] end @@ -71,7 +60,7 @@ def test_get_forward forward = @account.forwards.get(forward_id: 200) assert_equal 200, forward["id"] - assert_equal "Client Question", forward["subject"] + assert_equal "Project proposal from client", forward["subject"] assert_equal "client@example.com", forward["from"] end @@ -82,7 +71,7 @@ def test_list_replies replies = @account.forwards.list_replies(forward_id: 200).to_a assert_equal 2, replies.length - assert_equal "

Thanks for reaching out!

", replies[0]["content"] + assert_equal load_fixture("forwards/reply_get.json")["content"], replies[0]["content"] end def test_get_reply @@ -92,7 +81,7 @@ def test_get_reply reply = @account.forwards.get_reply(forward_id: 200, reply_id: 300) assert_equal 300, reply["id"] - assert_equal "

Thanks for reaching out!

", reply["content"] + assert_equal load_fixture("forwards/reply_get.json")["content"], reply["content"] end def test_create_reply diff --git a/ruby/test/basecamp/services/messages_service_test.rb b/ruby/test/basecamp/services/messages_service_test.rb index 4191ed5da..9bad9a1e0 100644 --- a/ruby/test/basecamp/services/messages_service_test.rb +++ b/ruby/test/basecamp/services/messages_service_test.rb @@ -16,16 +16,9 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_message(id: 789, subject: "Test Message") - { - "id" => id, - "subject" => subject, - "content" => "

Message content

", - "content_attachments" => [], - "status" => "active", - "created_at" => "2024-01-01T00:00:00Z", - "creator" => { "id" => 1, "name" => "Test User" } - } + def sample_message(id: nil, subject: nil) + fixture = load_fixture("messages/get.json") + fixture.merge "id" => id || fixture["id"], "subject" => subject || fixture["subject"] end def test_list @@ -35,7 +28,7 @@ def test_list result = @account.messages.list(board_id: 456).to_a assert_equal 2, result.length - assert_equal "Test Message", result[0]["subject"] + assert_equal "We won Leto!", result[0]["subject"] end def test_list_with_sort_and_direction @@ -57,8 +50,8 @@ def test_get result = @account.messages.get(message_id: 789) - assert_equal 789, result["id"] - assert_equal "Test Message", result["subject"] + assert_equal 1_069_479_351, result["id"] + assert_equal "We won Leto!", result["subject"] end def test_create diff --git a/ruby/test/basecamp/services/recordings_service_test.rb b/ruby/test/basecamp/services/recordings_service_test.rb index 0f7509d35..c7898f80d 100644 --- a/ruby/test/basecamp/services/recordings_service_test.rb +++ b/ruby/test/basecamp/services/recordings_service_test.rb @@ -19,17 +19,13 @@ def setup @account = create_account_client(account_id: "12345") end - def sample_recording(id: 456, title: "Test Recording") - { - "id" => id, - "title" => title, - "status" => "active", - "type" => "Todo", - # The generic recording projection carries the matching type's rich-text - # companion array; a Todo recording surfaces description_attachments. - "description_attachments" => [], - "created_at" => "2024-01-01T00:00:00Z" - } + # Sourced from the shared recordings/get.json fixture (the validated source of + # truth). It is a Message recording, so the rich-text companion array it + # carries is content_attachments (one inline file); description_attachments is + # absent for this type. + def sample_recording(id: nil, title: nil) + fixture = load_fixture("recordings/get.json") + fixture.merge "id" => id || fixture["id"], "title" => title || fixture["title"] end def test_list @@ -41,7 +37,7 @@ def test_list result = @account.recordings.list(type: "Todo").to_a assert_equal 2, result.length - assert_equal "Test Recording", result[0]["title"] + assert_equal "We won Leto!", result[0]["title"] end def test_list_with_filters @@ -62,10 +58,12 @@ def test_get result = @account.recordings.get(recording_id: 456) - assert_equal 456, result["id"] - assert_equal "Test Recording", result["title"] - # The optional projection array surfaces on the matching-type recording. - assert_equal [], result["description_attachments"] + assert_equal 1_069_479_351, result["id"] + assert_equal "We won Leto!", result["title"] + # This Message recording surfaces content_attachments (one inline file); the + # description_attachments projection is absent for this type. + assert_equal 1, result["content_attachments"].length + assert_nil result["description_attachments"] end def test_archive diff --git a/ruby/test/basecamp/services/schedules_service_test.rb b/ruby/test/basecamp/services/schedules_service_test.rb index 0021dd630..dc8bbf4df 100644 --- a/ruby/test/basecamp/services/schedules_service_test.rb +++ b/ruby/test/basecamp/services/schedules_service_test.rb @@ -24,15 +24,9 @@ def sample_schedule(id: 456) } end - def sample_entry(id: 789, summary: "Team Meeting") - { - "id" => id, - "summary" => summary, - "description_attachments" => [], - "starts_at" => "2024-12-15T09:00:00Z", - "ends_at" => "2024-12-15T10:00:00Z", - "all_day" => false - } + def sample_entry(id: nil, summary: nil) + fixture = load_fixture("schedules/entry_get.json") + fixture.merge "id" => id || fixture["id"], "summary" => summary || fixture["summary"] end def test_get @@ -51,7 +45,7 @@ def test_list_entries result = @account.schedules.list_entries(schedule_id: 456).to_a assert_equal 2, result.length - assert_equal "Team Meeting", result[0]["summary"] + assert_equal "Project Kickoff Meeting", result[0]["summary"] end def test_get_entry @@ -59,8 +53,8 @@ def test_get_entry result = @account.schedules.get_entry(entry_id: 789) - assert_equal 789, result["id"] - assert_equal "Team Meeting", result["summary"] + assert_equal 1_069_479_400, result["id"] + assert_equal "Project Kickoff Meeting", result["summary"] end def test_create_entry diff --git a/ruby/test/test_helper.rb b/ruby/test/test_helper.rb index 22fa4c100..f38a94bca 100644 --- a/ruby/test/test_helper.rb +++ b/ruby/test/test_helper.rb @@ -184,6 +184,13 @@ def stub_delete(path, status: 204) .to_return(status: status, body: "") end + # Load a shared JSON fixture (the validated source of truth) as string-keyed + # hashes. test_helper.rb lives at ruby/test/, so "../../spec/fixtures" reaches + # the repo-root spec/fixtures directory. + def load_fixture(relative_path) + JSON.parse(File.read(File.expand_path("../../spec/fixtures/#{relative_path}", __dir__))) + end + # Sample project data def sample_project(id: 123, name: "Test Project") { diff --git a/typescript/tests/services/cards.test.ts b/typescript/tests/services/cards.test.ts index 42a8e7c3d..381c7788c 100644 --- a/typescript/tests/services/cards.test.ts +++ b/typescript/tests/services/cards.test.ts @@ -7,18 +7,14 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; +import cardFixture from "../../../spec/fixtures/cards/get.json"; const BASE_URL = "https://3.basecampapi.com/12345"; -const sampleCard = (id = 1) => ({ - id, - title: "Design mockups", - content: "

Create initial designs

", - description_attachments: [], - due_on: "2024-03-01", - created_at: "2024-01-15T10:00:00Z", - updated_at: "2024-01-15T10:00:00Z", -}); +// Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) +// so this helper cannot drift from the validated Card shape; `id` is overridable +// per call. +const sampleCard = (id = cardFixture.id) => ({ ...cardFixture, id }); describe("CardsService", () => { let client: BasecampClient; @@ -71,7 +67,7 @@ describe("CardsService", () => { const card = await client.cards.get(cardId); expect(card.id).toBe(cardId); - expect(card.title).toBe("Design mockups"); + expect(card.title).toBe(cardFixture.title); }); it("should throw not_found for missing card", async () => { diff --git a/typescript/tests/services/client-visibility.test.ts b/typescript/tests/services/client-visibility.test.ts index 02c6a8bee..750ab19b1 100644 --- a/typescript/tests/services/client-visibility.test.ts +++ b/typescript/tests/services/client-visibility.test.ts @@ -6,17 +6,14 @@ import { http, HttpResponse } from "msw"; import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import type { BasecampClient } from "../../src/client.js"; +import recordingFixture from "../../../spec/fixtures/recordings/get.json"; const BASE_URL = "https://3.basecampapi.com/12345"; -const sampleRecording = (id = 1) => ({ - id, - title: "Some recording", - type: "Todo", - visible_to_clients: true, - created_at: "2024-01-15T10:00:00Z", - updated_at: "2024-01-15T10:00:00Z", -}); +// Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) +// so this helper cannot drift from the validated Recording shape; `id` is +// overridable per call. +const sampleRecording = (id = recordingFixture.id) => ({ ...recordingFixture, id }); describe("ClientVisibilityService", () => { let client: BasecampClient; diff --git a/typescript/tests/services/comments.test.ts b/typescript/tests/services/comments.test.ts index fef0a261a..6b7c13672 100644 --- a/typescript/tests/services/comments.test.ts +++ b/typescript/tests/services/comments.test.ts @@ -7,17 +7,14 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; +import commentFixture from "../../../spec/fixtures/comments/get.json"; const BASE_URL = "https://3.basecampapi.com/12345"; -const sampleComment = (id = 1) => ({ - id, - content: "

Great work!

", - content_attachments: [], - created_at: "2024-01-15T10:00:00Z", - updated_at: "2024-01-15T10:00:00Z", - creator: { id: 100, name: "Jane Doe" }, -}); +// Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) +// so this helper cannot drift from the validated Comment shape; `id` is +// overridable per call. +const sampleComment = (id = commentFixture.id) => ({ ...commentFixture, id }); describe("CommentsService", () => { let client: BasecampClient; @@ -42,7 +39,7 @@ describe("CommentsService", () => { const comment = await client.comments.get(commentId); expect(comment.id).toBe(commentId); - expect(comment.content).toBe("

Great work!

"); + expect(comment.content).toBe(commentFixture.content); }); it("preserves float-spelled and null attachment dimensions at runtime", async () => { diff --git a/typescript/tests/services/messages.test.ts b/typescript/tests/services/messages.test.ts index a10801c81..f007b761b 100644 --- a/typescript/tests/services/messages.test.ts +++ b/typescript/tests/services/messages.test.ts @@ -7,19 +7,14 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; +import messageFixture from "../../../spec/fixtures/messages/get.json"; const BASE_URL = "https://3.basecampapi.com/12345"; -const sampleMessage = (id = 1) => ({ - id, - subject: "Weekly Update", - content: "

Here is the update

", - content_attachments: [], - status: "active", - created_at: "2024-01-15T10:00:00Z", - updated_at: "2024-01-15T10:00:00Z", - creator: { id: 100, name: "Jane Doe" }, -}); +// Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) +// so this helper cannot drift from the validated Message shape; `id` is +// overridable per call. +const sampleMessage = (id = messageFixture.id) => ({ ...messageFixture, id }); describe("MessagesService", () => { let client: BasecampClient; @@ -88,7 +83,7 @@ describe("MessagesService", () => { const message = await client.messages.get(messageId); expect(message.id).toBe(messageId); - expect(message.subject).toBe("Weekly Update"); + expect(message.subject).toBe(messageFixture.subject); }); it("should throw not_found for missing message", async () => { diff --git a/typescript/tests/services/todolists.test.ts b/typescript/tests/services/todolists.test.ts index 9967f8d8d..636a8f0fb 100644 --- a/typescript/tests/services/todolists.test.ts +++ b/typescript/tests/services/todolists.test.ts @@ -7,19 +7,14 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; +import todolistFixture from "../../../spec/fixtures/todolists/get.json"; const BASE_URL = "https://3.basecampapi.com/12345"; -const sampleTodolist = (id = 1) => ({ - id, - name: "Launch list", - description: "

Things to do before launch

", - description_attachments: [], - completed: false, - completed_ratio: "0/5", - created_at: "2024-01-15T10:00:00Z", - updated_at: "2024-01-15T10:00:00Z", -}); +// Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) +// so this helper cannot drift from the validated Todolist shape; `id` is +// overridable per call. +const sampleTodolist = (id = todolistFixture.id) => ({ ...todolistFixture, id }); describe("TodolistsService", () => { let client: BasecampClient; @@ -44,7 +39,7 @@ describe("TodolistsService", () => { const todolist = await client.todolists.get(id); expect(todolist.id).toBe(id); - expect(todolist.name).toBe("Launch list"); + expect(todolist.name).toBe(todolistFixture.name); }); it("should throw not_found for missing todolist", async () => { From 6addd730d1fd2bd7d831c40cea0f4f9fefcc3222 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 22:49:16 -0700 Subject: [PATCH 03/16] Address review: pin get-test ids to the requested resource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #432: the schedules/recordings/messages get-tests stubbed a path with a specific id (789/456/789) but returned the shared fixture's id and asserted that, weakening the id-alignment check. Override the fixture id to the requested id via the helper's existing `id:` param and assert it — the rest of the body stays fixture-sourced. Also correct the manifest's operation-entry shorthand to show `fixture` is required (the operation supplies the schema, the fixture the body). --- ruby/test/basecamp/services/messages_service_test.rb | 4 ++-- ruby/test/basecamp/services/recordings_service_test.rb | 4 ++-- ruby/test/basecamp/services/schedules_service_test.rb | 4 ++-- spec/fixtures/manifest.yaml | 5 +++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ruby/test/basecamp/services/messages_service_test.rb b/ruby/test/basecamp/services/messages_service_test.rb index 9bad9a1e0..27969183c 100644 --- a/ruby/test/basecamp/services/messages_service_test.rb +++ b/ruby/test/basecamp/services/messages_service_test.rb @@ -46,11 +46,11 @@ def test_list_with_sort_and_direction def test_get # Generated service: /messages/{id} without .json - stub_get("/12345/messages/789", response_body: sample_message) + stub_get("/12345/messages/789", response_body: sample_message(id: 789)) result = @account.messages.get(message_id: 789) - assert_equal 1_069_479_351, result["id"] + assert_equal 789, result["id"] assert_equal "We won Leto!", result["subject"] end diff --git a/ruby/test/basecamp/services/recordings_service_test.rb b/ruby/test/basecamp/services/recordings_service_test.rb index c7898f80d..b5cbc0d1f 100644 --- a/ruby/test/basecamp/services/recordings_service_test.rb +++ b/ruby/test/basecamp/services/recordings_service_test.rb @@ -54,11 +54,11 @@ def test_list_with_filters def test_get # Generated service: /recordings/{id} without .json - stub_get("/12345/recordings/456", response_body: sample_recording) + stub_get("/12345/recordings/456", response_body: sample_recording(id: 456)) result = @account.recordings.get(recording_id: 456) - assert_equal 1_069_479_351, result["id"] + assert_equal 456, result["id"] assert_equal "We won Leto!", result["title"] # This Message recording surfaces content_attachments (one inline file); the # description_attachments projection is absent for this type. diff --git a/ruby/test/basecamp/services/schedules_service_test.rb b/ruby/test/basecamp/services/schedules_service_test.rb index dc8bbf4df..59b45c390 100644 --- a/ruby/test/basecamp/services/schedules_service_test.rb +++ b/ruby/test/basecamp/services/schedules_service_test.rb @@ -49,11 +49,11 @@ def test_list_entries end def test_get_entry - stub_get("/12345/schedule_entries/789", response_body: sample_entry) + stub_get("/12345/schedule_entries/789", response_body: sample_entry(id: 789)) result = @account.schedules.get_entry(entry_id: 789) - assert_equal 1_069_479_400, result["id"] + assert_equal 789, result["id"] assert_equal "Project Kickoff Meeting", result["summary"] end diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml index 3b065fff3..53393fbb3 100644 --- a/spec/fixtures/manifest.yaml +++ b/spec/fixtures/manifest.yaml @@ -15,10 +15,11 @@ # # Each `targets` entry is one of: # -# operation entry — { id, operation: } +# operation entry — { id, operation: , fixture: } (pointer optional, defaults "") # Validates the COMPLETE response body of `operation` (response-schema # lookup prefers 200, then any 2xx, then default) against the fixture named -# by `fixture` (pointer defaults to ""). +# by `fixture`. `fixture` is required — the operation supplies the schema, +# the fixture supplies the body to validate against it. # # pointer entry — { id, fixture, pointer, schema } # Validates the object selected by JSON pointer `pointer` within `fixture` From 72614b7311017dcff9caf17e84930cc15dfcab81 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 23:05:54 -0700 Subject: [PATCH 04/16] Harden fixture guard: schema type/nullability validation, CI, non-self-erasing inventory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review (Copilot/Codex/Cubic + human) on #432: - Real schema validation (codex P2, human blocker): the guard now validates each target's instance for type AND nullability, not just required-key presence — a required field null against a non-nullable schema, or a value whose JSON type contradicts the declared type (string where an array is required), now fails. integer/number are interchangeable so float-spelled FlexInt (1024.0) still passes. The conformance walker still supplies the required-field walk. - Non-self-erasing inventory (human blocker): derive the RichTextAttachment- emitter set from openapi.json and require every emitter to be covered or excluded, so dropping a schema from covered_schemas fails instead of silently reducing coverage. 18 emitters, all accounted for. - Schema-type-aware concreteness (Copilot): an array-typed component is concrete when the instance is a non-empty Array (not only a Hash), so array-typed components can satisfy coverage. - RFC 6901 pointers (Cubic P3): reject leading-zero array tokens (e.g. /x/01) so a manifest typo fails instead of silently selecting element 1. - Operation entries reject a non-root pointer (they validate the whole body). - Wire the guard into CI as a dedicated `fixture-coverage` job in test.yml — it was only in `make check`, which CI never invoked. Docs (manifest header + AGENTS.md backstop note) updated to describe the type/nullability validation and inventory-completeness rules. --- .github/workflows/test.yml | 18 +++ AGENTS.md | 11 +- scripts/check-fixture-coverage.rb | 251 ++++++++++++++++++++++++------ spec/fixtures/manifest.yaml | 17 ++ 4 files changed, 242 insertions(+), 55 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 096dd0f57..7b3dabe91 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -45,6 +45,24 @@ jobs: - name: Verify embedded provenance is up to date run: diff -q spec/api-provenance.json go/pkg/basecamp/api-provenance.json + fixture-coverage: + name: Fixture coverage + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - name: Set up Ruby + uses: ruby/setup-ruby@d45b1a4e94b71acab930e56e79c6aa188764e7f9 # v1.316.0 + with: + ruby-version: "3.3" + + - name: Check fixture completeness + coverage invariant + run: ruby scripts/check-fixture-coverage.rb + test-go: name: Go Tests runs-on: ubuntu-latest diff --git a/AGENTS.md b/AGENTS.md index 9dd45e2e4..197a4d2bb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -316,10 +316,13 @@ Use `make sync-status` to see upstream diffs since last sync. > **Automated backstop.** The shared JSON fixtures under `spec/fixtures/` are > guarded by `make check-fixture-coverage` (`spec/fixtures/manifest.yaml`): every -> manifest'd fixture must carry all required fields of its schema, and every -> `covered_schemas` entry must keep a concrete representative. A new required -> field on a covered schema is therefore forced into a fixture. This guard covers -> only the manifest'd shared fixtures — one-off inline stubs remain the reviewer's +> manifest'd fixture is validated against its schema for both required-field +> presence and type/nullability (a required-null-against-non-nullable or a +> wrong-typed value fails), every `covered_schemas` entry must keep a concrete +> representative, and every rich-text emitter schema must be covered or explicitly +> excluded (so the inventory can't silently shrink). A new required field on a +> covered schema is therefore forced into a fixture. This guard covers only the +> manifest'd shared fixtures — one-off inline stubs remain the reviewer's > responsibility under the rule above. ### Pre-Merge Verification diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 57170be23..79d0caf7d 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -5,22 +5,29 @@ # # Validates spec/fixtures/manifest.yaml: # 1. Every `targets` entry resolves — fixture exists, JSON pointer selects a -# concrete non-null instance, the named schema is a real component — and -# the selected instance carries every required field of its schema (the -# recursive $ref/array/required walk, reused from the conformance walker). +# concrete instance, the named schema is a real component — and the selected +# instance both (a) carries every required field of its schema (the +# recursive $ref/array/required walk, reused from the conformance walker) +# and (b) matches the schema's declared types and nullability (no required +# field null against a non-nullable schema, no wrong container/scalar type). # 2. The coverage invariant: every schema in `covered_schemas` has >= 1 active -# target that resolves to a concrete, non-null instance whose declared root -# schema is that component (the concrete-instance rule — transitive -# reachability and empty arrays do not count). -# 3. `excluded_schemas` entries each carry a reason + tracking issue, name real +# target that resolves to a concrete instance whose declared root schema is +# that component (the concrete-instance rule — transitive reachability and +# empty arrays do not count; an object component wants a Hash, an array +# component a non-empty Array). +# 3. Inventory completeness: every RichTextAttachment-emitting component schema +# (the #408 class this guard exists to protect) must be either covered or +# excluded — so the inventory can't silently shrink by dropping a schema. +# 4. `excluded_schemas` entries each carry a reason + tracking issue, name real # components, and do not overlap `covered_schemas`. # -# Reuses conformance/runner/ruby/schema-walker.rb for validation — the required -# walk is not reimplemented here. Wired into `make check` in the -# scripts/validate-api-gaps.rb style (stdlib only). +# Reuses conformance/runner/ruby/schema-walker.rb for the required-field walk. +# The type/nullability walk is additional (the walker checks presence only). +# Wired into `make check` in the scripts/validate-api-gaps.rb style (stdlib only). require "json" require "yaml" +require "set" require_relative "../conformance/runner/ruby/schema-walker" @@ -36,8 +43,9 @@ def fail_with(errors, message) end # RFC 6901 JSON pointer resolution. Returns [value, found]. "" selects the whole -# document. Missing keys / out-of-range indices / non-container traversal -> -# [nil, false]. +# document. Array indices must be "0" or a non-zero-leading run of digits (RFC +# 6901 §4) — "01" is malformed and does NOT silently resolve element 1. Missing +# keys / out-of-range indices / non-container traversal -> [nil, false]. def resolve_pointer(doc, pointer) return [doc, true] if pointer.nil? || pointer.empty? return [nil, false] unless pointer.start_with?("/") @@ -51,7 +59,8 @@ def resolve_pointer(doc, pointer) current = current[token] when Array - return [nil, false] unless token.match?(/\A\d+\z/) + # RFC 6901 array index: "0" or [1-9][0-9]* — reject leading zeroes. + return [nil, false] unless token == "0" || token.match?(/\A[1-9]\d*\z/) idx = token.to_i return [nil, false] if idx >= current.length @@ -64,6 +73,142 @@ def resolve_pointer(doc, pointer) [current, true] end +# --- Schema helpers (local; the walker keeps its resolver private) ------------- + +# Follows $ref chains within components until a non-ref schema or a cycle. +def resolve_ref(schema, components) + seen = {} + current = schema + while current.is_a?(Hash) && current["$ref"].is_a?(String) + ref = current["$ref"] + break if seen[ref] + + seen[ref] = true + m = ref.match(%r{\A(?:openapi\.json)?#/components/schemas/(.+)\z}) + break unless m + + nxt = components[m[1]] + break unless nxt + + current = nxt + end + current +end + +# Returns [Set(declared non-null json-type strings), nullable?]. Handles OpenAPI +# 3.1 null-union (`type: [X, "null"]`) and 3.0 `nullable: true`. An empty set +# means the type is unconstrained/compositional (oneOf/allOf) — skip type checks. +def allowed_types(schema) + return [Set.new, false] unless schema.is_a?(Hash) + + t = schema["type"] + case t + when Array + members = t.compact + [Set.new(members - ["null"]), members.include?("null")] + when String + [Set.new([t]), schema["nullable"] == true] + else + [Set.new, schema["nullable"] == true] + end +end + +def json_type(value) + case value + when Hash then "object" + when Array then "array" + when String then "string" + when true, false then "boolean" + when Integer then "integer" + when Float then "number" + else "null" + end +end + +# Accepts an actual json-type against the schema's declared set. integer/number +# are interchangeable so float-spelled integers (e.g. FlexInt `1024.0`) pass. +def type_ok?(types, actual) + return true if types.empty? + return true if types.include?(actual) + + numeric = %w[integer number] + numeric.include?(actual) && types.any? { |t| numeric.include?(t) } +end + +# Recursive type/nullability validation. Reports (prefix-tagged) errors for: +# - a present value whose JSON type contradicts the schema's declared type; +# - a REQUIRED field present as null against a non-nullable schema. +# Optional-null and unknown/compositional schemas are left alone (forward-compat, +# matching the walker's extras philosophy). +def type_errors(prefix, value, schema, components, depth = 0) + return [] if depth > 20 + + resolved = resolve_ref(schema, components) + return [] unless resolved.is_a?(Hash) + + return [] if value.nil? # optional-null tolerated here; required-null caught below + + errs = [] + types, = allowed_types(resolved) + actual = json_type(value) + unless type_ok?(types, actual) + label = prefix.empty? ? "(root)" : prefix + return ["#{label}: expected #{types.to_a.sort.join('|')}, got #{actual}"] + end + + if value.is_a?(Hash) && resolved["properties"].is_a?(Hash) + props = resolved["properties"] + (resolved["required"] || []).each do |rk| + next unless value.key?(rk) && value[rk].nil? + + _, nullable = allowed_types(resolve_ref(props[rk] || {}, components)) + next if nullable + + field = prefix.empty? ? rk : "#{prefix}/#{rk}" + errs << "#{field}: required field is null but its schema is not nullable" + end + value.each do |k, v| + next unless props.key?(k) + + child = prefix.empty? ? k : "#{prefix}/#{k}" + errs.concat(type_errors(child, v, props[k], components, depth + 1)) + end + elsif value.is_a?(Array) && resolved["items"] + value.each_with_index do |item, i| + errs.concat(type_errors("#{prefix}[#{i}]", item, resolved["items"], components, depth + 1)) + end + end + + errs +end + +# True when the instance is a concrete representative of `schema_name`: an +# object component wants a non-null Hash; an array component a non-empty Array. +def concrete_for?(schema_name, instance, components) + types, = allowed_types(components[schema_name] || {}) + if types.include?("array") + instance.is_a?(Array) && !instance.empty? + else + instance.is_a?(Hash) + end +end + +# Component schemas that declare at least one array-of-RichTextAttachment member +# — the rich-text emitters the guard exists to keep represented. +def rich_text_emitters(components) + components.select do |_name, schema| + props = schema.is_a?(Hash) ? schema["properties"] : nil + next false unless props.is_a?(Hash) + + props.any? do |_pn, ps| + ps.is_a?(Hash) && ps["type"] == "array" && + ps.dig("items", "$ref").to_s.end_with?("/RichTextAttachment") + end + end.keys +end + +# --- Load ----------------------------------------------------------------------- + unless File.file?(MANIFEST_FILE) warn "ERROR: manifest not found at #{MANIFEST_FILE}" exit 2 @@ -94,8 +239,8 @@ def resolve_pointer(doc, pointer) # --- 1. Targets ---------------------------------------------------------------- by_id = {} -# Records, per target id, the concrete root schema it resolved to (or nil) so -# the coverage pass can apply the concrete-instance rule without re-resolving. +# Records, per target id, the concrete root schema it resolved to (or nil) so the +# coverage pass can apply the concrete-instance rule without re-resolving. resolved_root = {} targets.each_with_index do |entry, i| @@ -115,23 +260,27 @@ def resolve_pointer(doc, pointer) pointer = entry["pointer"] || "" schema_name = entry["schema"] - if operation && (schema_name || !fixture_rel) - # operation entry - if schema_name - fail_with(errors, "target `#{id}`: `operation` and `schema` are mutually exclusive") - next - end + if operation && schema_name + fail_with(errors, "target `#{id}`: `operation` and `schema` are mutually exclusive") + next + end + if operation && !pointer.empty? + fail_with(errors, + "target `#{id}`: operation entries validate the whole response body — " \ + "a non-root `pointer` (#{pointer}) is not allowed (use a pointer entry with an explicit schema)") + next end - # Locate the schema to validate against. - schema = + # Locate the schema to validate against, and the schema NAME for coverage. + schema, root_schema_name = if operation s = walker.find_response_schema(operation) unless s fail_with(errors, "target `#{id}`: no 2xx/default response schema for operation `#{operation}`") next end - s + m = s.is_a?(Hash) ? s["$ref"].to_s.match(%r{/components/schemas/(.+)\z}) : nil + [s, m && m[1]] else unless schema_name fail_with(errors, "target `#{id}`: pointer entry needs an explicit `schema`") @@ -141,10 +290,9 @@ def resolve_pointer(doc, pointer) fail_with(errors, "target `#{id}`: unknown component schema `#{schema_name}`") next end - { "$ref" => "#/components/schemas/#{schema_name}" } + [{ "$ref" => "#/components/schemas/#{schema_name}" }, schema_name] end - # Load fixture (operation entries default pointer ""). unless fixture_rel fail_with(errors, "target `#{id}`: missing `fixture`") next @@ -165,24 +313,17 @@ def resolve_pointer(doc, pointer) next end - # Record the declared root schema for the coverage pass. For pointer entries - # it is the named component; for operation entries, the top-level $ref name. - resolved_root[id] = - if schema_name - { name: schema_name, concrete: instance.is_a?(Hash) } - else - root = schema.is_a?(Hash) ? schema["$ref"] : nil - m = root&.match(%r{/components/schemas/(.+)\z}) - { name: m && m[1], concrete: instance.is_a?(Hash) } - end - - missing = walker.missing_required(instance, schema) - next if missing.empty? + # Record for the coverage pass: concreteness is schema-type-aware. + concrete = root_schema_name ? concrete_for?(root_schema_name, instance, components) : instance.is_a?(Hash) + resolved_root[id] = { name: root_schema_name, concrete: concrete } where = pointer.empty? ? fixture_rel : "#{fixture_rel} at pointer `#{pointer}`" - missing.each do |path| + walker.missing_required(instance, schema).each do |path| fail_with(errors, "target `#{id}`: #{where} missing required field `#{path}`") end + type_errors("", instance, schema, components).each do |msg| + fail_with(errors, "target `#{id}`: #{where} #{msg}") + end end # --- 2. Coverage invariant ----------------------------------------------------- @@ -207,19 +348,29 @@ def resolve_pointer(doc, pointer) root = resolved_root[id] next unless root # target failed to resolve; already reported above - if root[:name] == schema_name && root[:concrete] - concrete_rep = true - end + concrete_rep = true if root[:name] == schema_name && root[:concrete] end unless concrete_rep fail_with(errors, - "covered_schemas[`#{schema_name}`]: no active target resolves to a concrete, " \ - "non-null instance declared as `#{schema_name}` (concrete-instance rule)") + "covered_schemas[`#{schema_name}`]: no active target resolves to a concrete " \ + "instance declared as `#{schema_name}` (concrete-instance rule)") end end -# --- 3. Exclusions ------------------------------------------------------------- +# --- 3. Inventory completeness (non-self-erasing) ------------------------------ + +accounted = Set.new(covered.keys) | Set.new(excluded.keys) +rich_text_emitters(components).sort.each do |emitter| + next if accounted.include?(emitter) + + fail_with(errors, + "schema `#{emitter}` declares a RichTextAttachment array member but is neither " \ + "in covered_schemas nor excluded_schemas — the coverage inventory must account " \ + "for every rich-text emitter (add a covered target or an exclusion with a reason)") +end + +# --- 4. Exclusions ------------------------------------------------------------- excluded.each do |schema_name, meta| unless components.key?(schema_name) @@ -242,11 +393,9 @@ def resolve_pointer(doc, pointer) # --- Report -------------------------------------------------------------------- if errors.empty? - covered_n = covered.size - target_n = targets.size - excluded_n = excluded.size - puts "==> Fixture coverage clean — #{covered_n} covered schemas, " \ - "#{target_n} manifest targets, #{excluded_n} tracked exclusions" + puts "==> Fixture coverage clean — #{covered.size} covered schemas, " \ + "#{targets.size} manifest targets, #{excluded.size} tracked exclusions, " \ + "#{rich_text_emitters(components).size} rich-text emitters accounted for" exit 0 else warn "Fixture-coverage check failed:" diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml index 53393fbb3..8a93c3bfa 100644 --- a/spec/fixtures/manifest.yaml +++ b/spec/fixtures/manifest.yaml @@ -49,6 +49,23 @@ # `excluded_schemas` records schemas deliberately NOT covered this round, each # with a reason + tracking issue. Exclusions do NOT satisfy coverage. # +# --- Inventory completeness (non-self-erasing) -------------------------------- +# +# Every component schema that declares an array-of-RichTextAttachment member (the +# #408 rich-text-emitter class this guard exists to protect) MUST appear in +# covered_schemas or excluded_schemas. This is derived from openapi.json, so the +# inventory cannot silently shrink: dropping a rich-text emitter from +# covered_schemas fails the check instead of quietly reducing coverage. +# +# --- Per-target validation ---------------------------------------------------- +# +# Each target's selected instance is validated against its schema for BOTH +# required-field presence (the conformance walker) AND type/nullability: a +# required field that is null against a non-nullable schema, or a value whose +# JSON type contradicts the declared type (e.g. a string where an array is +# required), fails the target. So a fixture that no longer matches the generated +# types is caught, not just one missing a key. +# # A missing fixture, unresolvable pointer, unknown schema, or covered schema # with no active concrete representative -> the check FAILS. From b8bc41044b38bc97b7b212228d64fa28ef0c5fc8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 23:47:06 -0700 Subject: [PATCH 05/16] Fixture guard round 2: integral-int, null elements, robust emitters, self-test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on #432: - Integer validation rejects non-integral floats: `type_matches?` accepts a float for an integer field only when mathematically integral, so FlexInt `1024.0` passes but `1.5` fails (previously any Numeric passed). - Null array elements are caught: `type_errors` no longer returns early for a nil element — a null against a non-nullable item schema fails (so `content_attachments: [null]` no longer passes). Optional OBJECT-field nulls stay tolerated because the Smithy-derived OpenAPI under-marks some nullable optionals (e.g. Person.bio/location are `type:string` yet the wire sends null). - Emitter discovery resolves `$ref` and `allOf`/`anyOf`/`oneOf`, so an indirectly-declared companion array can't bypass the non-self-erasing inventory check. - RFC 6901: reject invalid escapes (`~` not followed by 0/1, e.g. `~2`) in addition to the leading-zero index fix. - Locale robustness: read openapi/manifest/fixtures as explicit UTF-8 (LC_ALL=C otherwise reads US-ASCII and chokes on UTF-8 bytes) — including the shared conformance schema-walker's own read. - Automated negative cases: scripts/test-check-fixture-coverage.rb drives the checker via FIXTURE_MANIFEST/FIXTURE_OPENAPI/FIXTURE_DIR env overrides against crafted-bad inputs and asserts each failure mode is caught (1 positive + 8 negative). Wired into `make check-fixture-coverage` and the CI job. --- .github/workflows/test.yml | 3 + Makefile | 9 +- conformance/runner/ruby/schema-walker.rb | 4 +- scripts/check-fixture-coverage.rb | 132 +++++++++++++++------- scripts/test-check-fixture-coverage.rb | 138 +++++++++++++++++++++++ 5 files changed, 239 insertions(+), 47 deletions(-) create mode 100755 scripts/test-check-fixture-coverage.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7b3dabe91..ee08643a7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -63,6 +63,9 @@ jobs: - name: Check fixture completeness + coverage invariant run: ruby scripts/check-fixture-coverage.rb + - name: Fixture-guard self-test (negative cases) + run: ruby scripts/test-check-fixture-coverage.rb + test-go: name: Go Tests runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 6d2882f0a..eb201db6d 100644 --- a/Makefile +++ b/Makefile @@ -766,11 +766,14 @@ validate-api-gaps: @./scripts/validate-api-gaps.sh # Fixture-completeness guard: every spec/fixtures/manifest.yaml target validates -# against its schema, and every covered schema keeps a concrete active -# representative — so a new required field on a covered schema is forced into a -# fixture. Reuses the conformance schema-walker. +# against its schema (required-field presence + type/nullability), every covered +# schema keeps a concrete active representative, and every rich-text emitter is +# accounted for — so a new required field on a covered schema is forced into a +# fixture. Reuses the conformance schema-walker. The self-test asserts the guard +# rejects each crafted failure mode (the live check only exercises the valid set). check-fixture-coverage: @./scripts/check-fixture-coverage.sh + @ruby ./scripts/test-check-fixture-coverage.rb #------------------------------------------------------------------------------ # Combined targets diff --git a/conformance/runner/ruby/schema-walker.rb b/conformance/runner/ruby/schema-walker.rb index 843556e8f..02d018d8d 100644 --- a/conformance/runner/ruby/schema-walker.rb +++ b/conformance/runner/ruby/schema-walker.rb @@ -30,7 +30,9 @@ class SchemaWalker MAX_DEPTH = 12 def initialize(openapi_path) - @doc = JSON.parse(File.read(openapi_path)) + # Read as UTF-8 regardless of process locale (LC_ALL=C would otherwise + # read as US-ASCII and choke on the spec's UTF-8 bytes). + @doc = JSON.parse(File.read(openapi_path, encoding: "UTF-8")) end # Returns the response schema for operation_id, or nil when none exists. diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 79d0caf7d..d8619be1a 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -9,7 +9,9 @@ # instance both (a) carries every required field of its schema (the # recursive $ref/array/required walk, reused from the conformance walker) # and (b) matches the schema's declared types and nullability (no required -# field null against a non-nullable schema, no wrong container/scalar type). +# field null against a non-nullable schema, no null array element against a +# non-nullable item schema, no wrong container/scalar type; a float is +# accepted for an integer field only when mathematically integral). # 2. The coverage invariant: every schema in `covered_schemas` has >= 1 active # target that resolves to a concrete instance whose declared root schema is # that component (the concrete-instance rule — transitive reachability and @@ -18,12 +20,18 @@ # 3. Inventory completeness: every RichTextAttachment-emitting component schema # (the #408 class this guard exists to protect) must be either covered or # excluded — so the inventory can't silently shrink by dropping a schema. +# Emitter discovery resolves `$ref` and `allOf`/`anyOf`/`oneOf` so an +# indirectly-declared companion array can't slip the net. # 4. `excluded_schemas` entries each carry a reason + tracking issue, name real # components, and do not overlap `covered_schemas`. # # Reuses conformance/runner/ruby/schema-walker.rb for the required-field walk. # The type/nullability walk is additional (the walker checks presence only). # Wired into `make check` in the scripts/validate-api-gaps.rb style (stdlib only). +# +# Paths default to the repo layout but honour FIXTURE_MANIFEST / FIXTURE_OPENAPI +# / FIXTURE_DIR env overrides so the negative-case self-test +# (scripts/test-check-fixture-coverage.rb) can point it at crafted inputs. require "json" require "yaml" @@ -32,9 +40,15 @@ require_relative "../conformance/runner/ruby/schema-walker" PROJECT_ROOT = File.expand_path("..", __dir__) -MANIFEST_FILE = File.join(PROJECT_ROOT, "spec/fixtures/manifest.yaml") -FIXTURES_DIR = File.join(PROJECT_ROOT, "spec/fixtures") -OPENAPI_FILE = File.join(PROJECT_ROOT, "openapi.json") +MANIFEST_FILE = ENV.fetch("FIXTURE_MANIFEST", File.join(PROJECT_ROOT, "spec/fixtures/manifest.yaml")) +FIXTURES_DIR = ENV.fetch("FIXTURE_DIR", File.join(PROJECT_ROOT, "spec/fixtures")) +OPENAPI_FILE = ENV.fetch("FIXTURE_OPENAPI", File.join(PROJECT_ROOT, "openapi.json")) + +# Read text as UTF-8 regardless of the process locale (LC_ALL=C would otherwise +# read as US-ASCII and choke on the spec's UTF-8 bytes). +def read_utf8(path) + File.read(path, encoding: "UTF-8") +end errors = [] @@ -43,15 +57,18 @@ def fail_with(errors, message) end # RFC 6901 JSON pointer resolution. Returns [value, found]. "" selects the whole -# document. Array indices must be "0" or a non-zero-leading run of digits (RFC -# 6901 §4) — "01" is malformed and does NOT silently resolve element 1. Missing -# keys / out-of-range indices / non-container traversal -> [nil, false]. +# document. Escapes: `~0`->`~`, `~1`->`/`; a `~` not followed by 0/1 is invalid. +# Array indices must be "0" or a non-zero-leading run of digits (§4) — "01" is +# malformed and does NOT silently resolve element 1. Missing keys / out-of-range +# indices / non-container traversal / bad escapes -> [nil, false]. def resolve_pointer(doc, pointer) return [doc, true] if pointer.nil? || pointer.empty? return [nil, false] unless pointer.start_with?("/") current = doc pointer.split("/", -1).drop(1).each do |raw| + return [nil, false] if raw.match?(/~(?![01])/) # invalid escape (e.g. ~2, trailing ~) + token = raw.gsub("~1", "/").gsub("~0", "~") case current when Hash @@ -59,7 +76,6 @@ def resolve_pointer(doc, pointer) current = current[token] when Array - # RFC 6901 array index: "0" or [1-9][0-9]* — reject leading zeroes. return [nil, false] unless token == "0" || token.match?(/\A[1-9]\d*\z/) idx = token.to_i @@ -75,7 +91,6 @@ def resolve_pointer(doc, pointer) # --- Schema helpers (local; the walker keeps its resolver private) ------------- -# Follows $ref chains within components until a non-ref schema or a cycle. def resolve_ref(schema, components) seen = {} current = schema @@ -97,7 +112,7 @@ def resolve_ref(schema, components) # Returns [Set(declared non-null json-type strings), nullable?]. Handles OpenAPI # 3.1 null-union (`type: [X, "null"]`) and 3.0 `nullable: true`. An empty set -# means the type is unconstrained/compositional (oneOf/allOf) — skip type checks. +# means the type is unconstrained/compositional — skip type checks. def allowed_types(schema) return [Set.new, false] unless schema.is_a?(Hash) @@ -125,37 +140,46 @@ def json_type(value) end end -# Accepts an actual json-type against the schema's declared set. integer/number -# are interchangeable so float-spelled integers (e.g. FlexInt `1024.0`) pass. -def type_ok?(types, actual) +# True when `value`'s JSON type satisfies the declared `types`. integer/number +# interchange with one guard: a float supplied for an integer-only field passes +# only when it is mathematically integral (so FlexInt `1024.0` passes but `1.5` +# fails). +def type_matches?(types, value) return true if types.empty? + + actual = json_type(value) return true if types.include?(actual) - numeric = %w[integer number] - numeric.include?(actual) && types.any? { |t| numeric.include?(t) } + if actual == "number" && types.include?("integer") && !types.include?("number") + return value.is_a?(Float) && value.finite? && value == value.truncate + end + return true if actual == "integer" && types.include?("number") + + false end # Recursive type/nullability validation. Reports (prefix-tagged) errors for: -# - a present value whose JSON type contradicts the schema's declared type; -# - a REQUIRED field present as null against a non-nullable schema. -# Optional-null and unknown/compositional schemas are left alone (forward-compat, -# matching the walker's extras philosophy). +# - a present value whose JSON type contradicts the declared type; +# - a REQUIRED object field present as null against a non-nullable schema; +# - a null ARRAY ELEMENT against a non-nullable item schema. +# Optional object-field nulls are tolerated: the OpenAPI generated from Smithy +# under-marks some nullable optionals (e.g. Person.bio/location are `type:string` +# yet the wire sends null), so flagging them would be a false positive, not a +# fixture defect. def type_errors(prefix, value, schema, components, depth = 0) return [] if depth > 20 resolved = resolve_ref(schema, components) return [] unless resolved.is_a?(Hash) + return [] if value.nil? # object-field optional-null tolerated (see note above) - return [] if value.nil? # optional-null tolerated here; required-null caught below - - errs = [] types, = allowed_types(resolved) - actual = json_type(value) - unless type_ok?(types, actual) + unless type_matches?(types, value) label = prefix.empty? ? "(root)" : prefix - return ["#{label}: expected #{types.to_a.sort.join('|')}, got #{actual}"] + return ["#{label}: expected #{types.to_a.sort.join('|')}, got #{json_type(value)}"] end + errs = [] if value.is_a?(Hash) && resolved["properties"].is_a?(Hash) props = resolved["properties"] (resolved["required"] || []).each do |rk| @@ -174,16 +198,21 @@ def type_errors(prefix, value, schema, components, depth = 0) errs.concat(type_errors(child, v, props[k], components, depth + 1)) end elsif value.is_a?(Array) && resolved["items"] + items = resolved["items"] + _, item_nullable = allowed_types(resolve_ref(items, components)) value.each_with_index do |item, i| - errs.concat(type_errors("#{prefix}[#{i}]", item, resolved["items"], components, depth + 1)) + ip = "#{prefix}[#{i}]" + if item.nil? + errs << "#{ip}: null array element but the item schema is not nullable" unless item_nullable + next + end + errs.concat(type_errors(ip, item, items, components, depth + 1)) end end errs end -# True when the instance is a concrete representative of `schema_name`: an -# object component wants a non-null Hash; an array component a non-empty Array. def concrete_for?(schema_name, instance, components) types, = allowed_types(components[schema_name] || {}) if types.include?("array") @@ -193,17 +222,39 @@ def concrete_for?(schema_name, instance, components) end end -# Component schemas that declare at least one array-of-RichTextAttachment member -# — the rich-text emitters the guard exists to keep represented. +# True when `schema` is (or composes/refs) an array whose items are the +# RichTextAttachment component. Resolves $ref and allOf/anyOf/oneOf so an +# indirectly-declared companion array is still recognized. +def references_rich_text_array?(schema, components, depth = 0) + return false if depth > 10 + + resolved = resolve_ref(schema, components) + return false unless resolved.is_a?(Hash) + + t = resolved["type"] + is_array = t == "array" || (t.is_a?(Array) && t.include?("array")) + if is_array + items = resolved["items"] + ref = items.is_a?(Hash) ? items["$ref"] : nil + if ref.is_a?(String) && ref.match(%r{/components/schemas/(.+)\z})&.captures&.first == "RichTextAttachment" + return true + end + end + + %w[allOf anyOf oneOf].each do |key| + (resolved[key] || []).each do |sub| + return true if references_rich_text_array?(sub, components, depth + 1) + end + end + false +end + def rich_text_emitters(components) components.select do |_name, schema| props = schema.is_a?(Hash) ? schema["properties"] : nil next false unless props.is_a?(Hash) - props.any? do |_pn, ps| - ps.is_a?(Hash) && ps["type"] == "array" && - ps.dig("items", "$ref").to_s.end_with?("/RichTextAttachment") - end + props.any? { |_pn, ps| references_rich_text_array?(ps, components) } end.keys end @@ -218,29 +269,26 @@ def rich_text_emitters(components) exit 2 end -manifest = YAML.safe_load(File.read(MANIFEST_FILE)) +manifest = YAML.safe_load(read_utf8(MANIFEST_FILE)) walker = Basecamp::Conformance::SchemaWalker.new(OPENAPI_FILE) -openapi = JSON.parse(File.read(OPENAPI_FILE)) +openapi = JSON.parse(read_utf8(OPENAPI_FILE)) components = openapi.dig("components", "schemas") || {} targets = manifest["targets"] || [] covered = manifest["covered_schemas"] || {} excluded = manifest["excluded_schemas"] || {} -# Fixture cache so a fixture referenced by several targets is read once. fixture_cache = {} load_fixture = lambda do |rel| fixture_cache[rel] ||= begin path = File.join(FIXTURES_DIR, rel) - File.file?(path) ? JSON.parse(File.read(path)) : :missing + File.file?(path) ? JSON.parse(read_utf8(path)) : :missing end end # --- 1. Targets ---------------------------------------------------------------- by_id = {} -# Records, per target id, the concrete root schema it resolved to (or nil) so the -# coverage pass can apply the concrete-instance rule without re-resolving. resolved_root = {} targets.each_with_index do |entry, i| @@ -271,7 +319,6 @@ def rich_text_emitters(components) next end - # Locate the schema to validate against, and the schema NAME for coverage. schema, root_schema_name = if operation s = walker.find_response_schema(operation) @@ -313,7 +360,6 @@ def rich_text_emitters(components) next end - # Record for the coverage pass: concreteness is schema-type-aware. concrete = root_schema_name ? concrete_for?(root_schema_name, instance, components) : instance.is_a?(Hash) resolved_root[id] = { name: root_schema_name, concrete: concrete } @@ -346,7 +392,7 @@ def rich_text_emitters(components) next end root = resolved_root[id] - next unless root # target failed to resolve; already reported above + next unless root concrete_rep = true if root[:name] == schema_name && root[:concrete] end diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb new file mode 100755 index 000000000..4ba9b141d --- /dev/null +++ b/scripts/test-check-fixture-coverage.rb @@ -0,0 +1,138 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# Negative-case self-test for scripts/check-fixture-coverage.rb. +# +# The guard's own `make check` run only exercises the VALID manifest. This test +# crafts each failure mode and asserts the guard rejects it (non-zero exit + an +# expected message fragment), driving the checker through its FIXTURE_MANIFEST / +# FIXTURE_OPENAPI / FIXTURE_DIR env overrides against tmp inputs. It also +# confirms the real manifest still passes (positive control). +# +# Run directly (`ruby scripts/test-check-fixture-coverage.rb`) or via +# `make check-fixture-coverage` (which runs it after the live check). + +require "json" +require "yaml" +require "tmpdir" +require "fileutils" +require "open3" + +ROOT = File.expand_path("..", __dir__) +CHECKER = File.join(__dir__, "check-fixture-coverage.rb") +REAL_OPENAPI = File.join(ROOT, "openapi.json") +REAL_FIXTURES = File.join(ROOT, "spec/fixtures") +REAL_MANIFEST = File.join(REAL_FIXTURES, "manifest.yaml") + +def read_utf8(path) = File.read(path, encoding: "UTF-8") + +# Run the checker with env overrides; returns [combined_output, status]. +def run_checker(manifest:, openapi: REAL_OPENAPI, fixtures: REAL_FIXTURES) + env = { + "FIXTURE_MANIFEST" => manifest, + "FIXTURE_OPENAPI" => openapi, + "FIXTURE_DIR" => fixtures, + } + Open3.capture2e(env, "ruby", CHECKER) +end + +failures = [] + +def expect_pass(failures, label, out, status) + return if status.success? + + failures << "#{label}: expected PASS but checker failed:\n#{out}" +end + +def expect_fail(failures, label, out, status, fragment) + if status.success? + failures << "#{label}: expected FAILURE but checker passed:\n#{out}" + elsif !out.include?(fragment) + failures << "#{label}: failed as expected but message missing #{fragment.inspect}:\n#{out}" + end +end + +# A tmp copy of the real fixtures tree, with one fixture file mutated in place. +def with_mutated_fixture(rel) + Dir.mktmpdir("fixture-cov-test") do |dir| + fixtures = File.join(dir, "fixtures") + FileUtils.cp_r(REAL_FIXTURES, fixtures) + path = File.join(fixtures, rel) + data = JSON.parse(read_utf8(path)) + yield data # mutate in place + File.write(path, JSON.generate(data)) + # Point the manifest's fixture lookups at the mutated tree; reuse the real + # manifest (paths are relative to FIXTURE_DIR). + out, status = run_checker(manifest: File.join(fixtures, "manifest.yaml"), fixtures: fixtures) + [out, status] + end +end + +# A tmp manifest (built from the real one, then mutated) against real fixtures. +def with_mutated_manifest + manifest = YAML.safe_load(read_utf8(REAL_MANIFEST)) + yield manifest + Dir.mktmpdir("fixture-cov-test") do |dir| + path = File.join(dir, "manifest.yaml") + File.write(path, YAML.dump(manifest)) + run_checker(manifest: path) + end +end + +# --- Positive control ---------------------------------------------------------- + +out, status = run_checker(manifest: REAL_MANIFEST) +expect_pass(failures, "real manifest passes", out, status) + +# --- Fixture-content mutations ------------------------------------------------- + +out, status = with_mutated_fixture("comments/get.json") { |d| d["id"] = "not-a-number" } +expect_fail(failures, "required field wrong type", out, status, "expected integer, got string") + +out, status = with_mutated_fixture("comments/get.json") { |d| d["id"] = nil } +expect_fail(failures, "required field null", out, status, "required field is null") + +out, status = with_mutated_fixture("comments/get.json") { |d| d["id"] = 1.5 } +expect_fail(failures, "non-integral float for integer", out, status, "expected integer, got number") + +out, status = with_mutated_fixture("comments/get.json") { |d| d["content_attachments"] = [nil] } +expect_fail(failures, "null array element", out, status, "null array element") + +# --- Manifest mutations -------------------------------------------------------- + +out, status = with_mutated_manifest do |m| + t = m["targets"].find { |e| e["id"] == "richtext-comment-0" } + t["pointer"] = "/content_attachments/01" +end +expect_fail(failures, "RFC 6901 leading-zero pointer", out, status, "does not resolve") + +out, status = with_mutated_manifest do |m| + t = m["targets"].find { |e| e["id"] == "richtext-comment-0" } + t["pointer"] = "/content_attachments/~2" +end +expect_fail(failures, "RFC 6901 bad escape", out, status, "does not resolve") + +out, status = with_mutated_manifest do |m| + m["covered_schemas"].delete("Todo") + m["targets"].reject! { |e| e["id"] == "todo-get" } +end +expect_fail(failures, "dropped emitter (self-erasing)", out, status, "neither in covered_schemas") + +out, status = with_mutated_manifest do |m| + t = m["targets"].find { |e| e["id"] == "todo-get" } + t.delete("schema") + t["operation"] = "GetTodo" + t["pointer"] = "/id" +end +expect_fail(failures, "operation entry with non-root pointer", out, status, "non-root") + +# --- Report -------------------------------------------------------------------- + +if failures.empty? + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative cases" + exit 0 +else + warn "Fixture-coverage self-test FAILED:" + failures.each { |f| warn " - #{f}" } + exit 1 +end From 844b66bb86ba9328fc0c35c64cd24859f4d414b8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 23:52:27 -0700 Subject: [PATCH 06/16] Address review: load_fixture reads UTF-8 (locale robustness) Copilot on #432: the Ruby test helper's load_fixture read fixtures without forcing UTF-8, so under a non-UTF-8 locale (LC_ALL=C -> US-ASCII) it raised on the emoji/non-ASCII bytes several fixtures carry (e.g. messages/get.json, boosts/get.json). Read as explicit UTF-8, matching the guard scripts and the conformance schema-walker. --- ruby/test/test_helper.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ruby/test/test_helper.rb b/ruby/test/test_helper.rb index f38a94bca..386a26ccd 100644 --- a/ruby/test/test_helper.rb +++ b/ruby/test/test_helper.rb @@ -186,9 +186,12 @@ def stub_delete(path, status: 204) # Load a shared JSON fixture (the validated source of truth) as string-keyed # hashes. test_helper.rb lives at ruby/test/, so "../../spec/fixtures" reaches - # the repo-root spec/fixtures directory. + # the repo-root spec/fixtures directory. Read as explicit UTF-8 so a non-UTF-8 + # process locale (LC_ALL=C) doesn't choke on the emoji/non-ASCII bytes several + # fixtures carry. def load_fixture(relative_path) - JSON.parse(File.read(File.expand_path("../../spec/fixtures/#{relative_path}", __dir__))) + path = File.expand_path("../../spec/fixtures/#{relative_path}", __dir__) + JSON.parse(File.read(path, encoding: "UTF-8")) end # Sample project data From 4492836908b8c5ae009f67d41d63c3957c9e066a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 00:04:56 -0700 Subject: [PATCH 07/16] Fixture guard: emitter discovery traverses composition + aliased items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last emitter-discovery hole on #432. Previously rich_text_emitters only scanned a component's top-level `properties` and only matched array items that DIRECTLY $ref RichTextAttachment, so an emitter reachable via component-level allOf/anyOf/oneOf, or one whose array items reference an alias/composed schema, could vanish from the derived (non-self-erasing) inventory. Now: - collect_property_schemas follows $ref and traverses component-level allOf/anyOf/oneOf, so inherited/composed properties are seen. - rich_text_attachment? resolves the item schema recursively through $ref chains and composition, so an aliased or composed RichTextAttachment item is matched. - Both walks keep a visited-name set (+ depth guard) so reference/composition cycles terminate. - Pure whole-component `$ref` aliases (the ~40 `*ResponseContent` response envelopes) are skipped: they introduce no decode surface of their own, and their target — if itself an emitter — is caught directly, so skipping creates no hole. Keeps the emitter set at the 18 concrete decode types. Self-test adds three synthetic cases proving it: a component-level allOf emitter and an aliased-item emitter are both discovered and flagged, and a reference/composition cycle terminates safely (Timeout-guarded) without misclassifying the cycle components as emitters. --- scripts/check-fixture-coverage.rb | 104 ++++++++++++++++++++----- scripts/test-check-fixture-coverage.rb | 64 ++++++++++++++- 2 files changed, 148 insertions(+), 20 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index d8619be1a..d470d756b 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -222,39 +222,105 @@ def concrete_for?(schema_name, instance, components) end end -# True when `schema` is (or composes/refs) an array whose items are the -# RichTextAttachment component. Resolves $ref and allOf/anyOf/oneOf so an -# indirectly-declared companion array is still recognized. -def references_rich_text_array?(schema, components, depth = 0) - return false if depth > 10 +# Ruby name of a `$ref`, or nil. +def ref_name(schema) + return nil unless schema.is_a?(Hash) && schema["$ref"].is_a?(String) - resolved = resolve_ref(schema, components) - return false unless resolved.is_a?(Hash) + schema["$ref"].match(%r{/components/schemas/(.+)\z})&.captures&.first +end - t = resolved["type"] - is_array = t == "array" || (t.is_a?(Array) && t.include?("array")) - if is_array - items = resolved["items"] - ref = items.is_a?(Hash) ? items["$ref"] : nil - if ref.is_a?(String) && ref.match(%r{/components/schemas/(.+)\z})&.captures&.first == "RichTextAttachment" - return true +# True when `schema` denotes (through $ref chains AND allOf/anyOf/oneOf +# composition) the RichTextAttachment component — so an alias +# (`{$ref: SomeAlias}` -> RichTextAttachment) or a composed item schema is still +# recognized. `visited` tracks resolved component names to terminate ref cycles. +def rich_text_attachment?(schema, components, visited = Set.new, depth = 0) + return false if depth > 40 || !schema.is_a?(Hash) + + name = ref_name(schema) + if name + return true if name == "RichTextAttachment" + return false if visited.include?(name) + + visited << name + return rich_text_attachment?(components[name], components, visited, depth + 1) + end + + %w[allOf anyOf oneOf].each do |key| + (schema[key] || []).each do |sub| + return true if rich_text_attachment?(sub, components, visited, depth + 1) end end + false +end + +# True when `schema` is (or composes/refs) an array whose items resolve to the +# RichTextAttachment component. Follows $ref chains and allOf/anyOf/oneOf on both +# the array schema and its items; `visited` guards ref cycles. +def references_rich_text_array?(schema, components, visited = Set.new, depth = 0) + return false if depth > 40 || !schema.is_a?(Hash) + + name = ref_name(schema) + if name + return false if visited.include?(name) + + visited << name + return references_rich_text_array?(components[name], components, visited, depth + 1) + end + + t = schema["type"] + if (t == "array" || (t.is_a?(Array) && t.include?("array"))) && schema["items"] + return true if rich_text_attachment?(schema["items"], components) + end %w[allOf anyOf oneOf].each do |key| - (resolved[key] || []).each do |sub| - return true if references_rich_text_array?(sub, components, depth + 1) + (schema[key] || []).each do |sub| + return true if references_rich_text_array?(sub, components, visited, depth + 1) end end false end +# Collects every property schema of a component, following $ref and traversing +# component-level allOf/anyOf/oneOf so properties inherited through composition +# are seen. `visited` guards ref cycles. +def collect_property_schemas(schema, components, out, visited = Set.new, depth = 0) + return if depth > 40 || !schema.is_a?(Hash) + + name = ref_name(schema) + if name + return if visited.include?(name) + + visited << name + return collect_property_schemas(components[name], components, out, visited, depth + 1) + end + + (schema["properties"] || {}).each_value { |ps| out << ps } + %w[allOf anyOf oneOf].each do |key| + (schema[key] || []).each do |sub| + collect_property_schemas(sub, components, out, visited, depth + 1) + end + end +end + +# A whole-component alias — a schema that is only a `$ref` to another component +# (bare annotations aside). It introduces no decode surface of its own, so it is +# NOT an independent emitter: covering its target covers it, and if that target +# is itself an emitter it is caught directly. Skipping these keeps the emitter +# set to the concrete decode types (the ~40 `*ResponseContent` response +# envelopes are pure aliases of already-covered components). +def pure_ref_alias?(schema) + schema.is_a?(Hash) && schema.key?("$ref") && + (schema.keys - %w[$ref description title]).empty? +end + def rich_text_emitters(components) components.select do |_name, schema| - props = schema.is_a?(Hash) ? schema["properties"] : nil - next false unless props.is_a?(Hash) + next false unless schema.is_a?(Hash) + next false if pure_ref_alias?(schema) - props.any? { |_pn, ps| references_rich_text_array?(ps, components) } + props = [] + collect_property_schemas(schema, components, props) + props.any? { |ps| references_rich_text_array?(ps, components) } end.keys end diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 4ba9b141d..9b6e2e20e 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -17,6 +17,7 @@ require "tmpdir" require "fileutils" require "open3" +require "timeout" ROOT = File.expand_path("..", __dir__) CHECKER = File.join(__dir__, "check-fixture-coverage.rb") @@ -126,10 +127,71 @@ def with_mutated_manifest end expect_fail(failures, "operation entry with non-root pointer", out, status, "non-root") +# --- Synthetic emitter-discovery cases ----------------------------------------- +# +# Exercise indirect rich-text-emitter shapes against a crafted OpenAPI (no +# targets, so no fixtures are read): the completeness check must still discover +# emitters declared through component-level composition and aliased array items, +# and must traverse reference/composition cycles without hanging. + +SYNTHETIC_OPENAPI = { + "components" => { "schemas" => { + "RichTextAttachment" => { "type" => "object", "properties" => { "id" => { "type" => "integer" } } }, + # A whole-component alias of RichTextAttachment (an aliased item target). + "RTAAlias" => { "$ref" => "#/components/schemas/RichTextAttachment" }, + "Base" => { "type" => "object", "properties" => { "title" => { "type" => "string" } } }, + # (1) Emitter whose companion array arrives via COMPONENT-LEVEL allOf. + "AllOfEmitter" => { "allOf" => [ + { "$ref" => "#/components/schemas/Base" }, + { "type" => "object", + "properties" => { "content_attachments" => { "type" => "array", + "items" => { "$ref" => "#/components/schemas/RichTextAttachment" } } } }, + ] }, + # (2) Emitter whose array ITEMS reference RichTextAttachment through an alias. + "AliasedItemEmitter" => { "type" => "object", + "properties" => { "description_attachments" => { "type" => "array", + "items" => { "$ref" => "#/components/schemas/RTAAlias" } } } }, + # (3a) Component-level composition cycle (no rich text) — must terminate. + "CycleA" => { "allOf" => [{ "$ref" => "#/components/schemas/CycleB" }] }, + "CycleB" => { "allOf" => [{ "$ref" => "#/components/schemas/CycleA" }] }, + # (3b) A self-referential item alias reached from an array — must terminate. + "SelfRefItem" => { "allOf" => [{ "$ref" => "#/components/schemas/SelfRefItem" }] }, + "CycleItemHolder" => { "type" => "object", + "properties" => { "things" => { "type" => "array", + "items" => { "$ref" => "#/components/schemas/SelfRefItem" } } } }, + } }, +}.freeze + +EMPTY_MANIFEST = { "targets" => [], "covered_schemas" => {}, "excluded_schemas" => {} }.freeze + +def run_synthetic(openapi:, manifest:) + Dir.mktmpdir("fixture-cov-synthetic") do |dir| + op = File.join(dir, "openapi.json") + mf = File.join(dir, "manifest.yaml") + File.write(op, JSON.generate(openapi)) + File.write(mf, YAML.dump(manifest)) + Timeout.timeout(30) { run_checker(manifest: mf, openapi: op, fixtures: dir) } + end +end + +begin + out, status = run_synthetic(openapi: SYNTHETIC_OPENAPI, manifest: EMPTY_MANIFEST) + # (1) + (2): both indirect emitters must be discovered and flagged as unaccounted. + expect_fail(failures, "component-level allOf emitter discovered", out, status, "`AllOfEmitter`") + expect_fail(failures, "aliased RichTextAttachment item discovered", out, status, "`AliasedItemEmitter`") + # (3): the cycle components must NOT be misclassified as emitters, and — proven + # by the run returning at all under Timeout — discovery terminated safely. + if out.include?("`CycleA`") || out.include?("`SelfRefItem`") || out.include?("`CycleItemHolder`") + failures << "cycle components should not be flagged as emitters:\n#{out}" + end +rescue Timeout::Error + failures << "emitter discovery did not terminate on a reference/composition cycle (hung)" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 3 synthetic emitter cases" exit 0 else warn "Fixture-coverage self-test FAILED:" From 1c3b523fd3f8e36dcbbce13e8ae3b92210d88216 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 00:27:45 -0700 Subject: [PATCH 08/16] Fixture guard round 4: $ref siblings, composition-aware validation, killable timeout, CI locale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round on #432: - $ref siblings (OpenAPI 3.1): the three discovery helpers no longer return immediately after following a $ref — they process the referenced target AND the node's local properties/composition, so a `{$ref: X, properties: {...}}` emitter is discovered. - Composition-aware validation: required/type/nullability validation now merges $ref (with siblings) and allOf via merged_constraints, replacing the walker's composition-blind required walk (the walker is kept only for find_response_schema). An accounted composed target whose empty fixture omits a branch's required field now fails. - Killable timeout: the self-test spawns the checker with a retained PID and KILLs+reaps it on timeout (Open3.capture2e's cleanup would block on a hung child, defeating Timeout). - Both Python loaders read_text(encoding="utf-8"). - CI pins LC_ALL=C on the fixture-coverage + self-test steps so the non-UTF-8 locale path is actually exercised. Self-test grows to 5 synthetic cases: allOf emitter, aliased-item emitter, $ref-with-siblings emitter, a reference/composition cycle that terminates safely, and a composed covered target whose empty fixture fails on both branch-contributed required fields. Docs (manifest header, AGENTS.md) updated to describe composition-aware validation accurately. --- .github/workflows/test.yml | 8 +- python/tests/services/test_todos.py | 2 +- python/tests/services/test_uploads.py | 2 +- scripts/check-fixture-coverage.rb | 196 ++++++++++++++----------- scripts/test-check-fixture-coverage.rb | 90 +++++++++++- spec/fixtures/manifest.yaml | 14 +- 6 files changed, 209 insertions(+), 103 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ee08643a7..8d0b5bbd0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -60,11 +60,17 @@ jobs: with: ruby-version: "3.3" + # Run under LC_ALL=C so CI actually exercises the non-UTF-8-locale path + # (the reads are pinned to UTF-8; this proves it stays that way). - name: Check fixture completeness + coverage invariant run: ruby scripts/check-fixture-coverage.rb + env: + LC_ALL: C - - name: Fixture-guard self-test (negative cases) + - name: Fixture-guard self-test (negative + synthetic cases) run: ruby scripts/test-check-fixture-coverage.rb + env: + LC_ALL: C test-go: name: Go Tests diff --git a/python/tests/services/test_todos.py b/python/tests/services/test_todos.py index ffa264542..af9fe9550 100644 --- a/python/tests/services/test_todos.py +++ b/python/tests/services/test_todos.py @@ -19,7 +19,7 @@ def load_fixture(rel: str) -> dict: - return json.loads((_FIXTURES / rel).read_text()) + return json.loads((_FIXTURES / rel).read_text(encoding="utf-8")) def _todo(todo_id: int = 42, **overrides) -> dict: diff --git a/python/tests/services/test_uploads.py b/python/tests/services/test_uploads.py index d81cd5709..1af3c9f28 100644 --- a/python/tests/services/test_uploads.py +++ b/python/tests/services/test_uploads.py @@ -16,7 +16,7 @@ def load_fixture(rel: str) -> dict: - return json.loads((_FIXTURES / rel).read_text()) + return json.loads((_FIXTURES / rel).read_text(encoding="utf-8")) def _metadata(upload_id: int = 1069479400, *, download_url, filename="report.pdf") -> dict: diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index d470d756b..d950e77f3 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -6,12 +6,13 @@ # Validates spec/fixtures/manifest.yaml: # 1. Every `targets` entry resolves — fixture exists, JSON pointer selects a # concrete instance, the named schema is a real component — and the selected -# instance both (a) carries every required field of its schema (the -# recursive $ref/array/required walk, reused from the conformance walker) -# and (b) matches the schema's declared types and nullability (no required -# field null against a non-nullable schema, no null array element against a -# non-nullable item schema, no wrong container/scalar type; a float is -# accepted for an integer field only when mathematically integral). +# instance validates against its schema: every required field present and +# non-null (unless the field schema is nullable), and every present value's +# JSON type matches the declared type (a float is accepted for an integer +# field only when mathematically integral; a null array element fails a +# non-nullable item schema). Validation is composition-aware — it merges +# `$ref` (including 3.1 `$ref`-with-siblings) and `allOf`, so a composed +# target whose required fields live in a branch is still enforced. # 2. The coverage invariant: every schema in `covered_schemas` has >= 1 active # target that resolves to a concrete instance whose declared root schema is # that component (the concrete-instance rule — transitive reachability and @@ -20,13 +21,14 @@ # 3. Inventory completeness: every RichTextAttachment-emitting component schema # (the #408 class this guard exists to protect) must be either covered or # excluded — so the inventory can't silently shrink by dropping a schema. -# Emitter discovery resolves `$ref` and `allOf`/`anyOf`/`oneOf` so an -# indirectly-declared companion array can't slip the net. +# Emitter discovery follows `$ref` (with siblings), traverses component-level +# `allOf`/`anyOf`/`oneOf`, and resolves aliased array items, guarding cycles. # 4. `excluded_schemas` entries each carry a reason + tracking issue, name real # components, and do not overlap `covered_schemas`. # -# Reuses conformance/runner/ruby/schema-walker.rb for the required-field walk. -# The type/nullability walk is additional (the walker checks presence only). +# Uses conformance/runner/ruby/schema-walker.rb only for `find_response_schema` +# (operation-entry response-schema lookup). The required/type/nullability walk is +# implemented here because it must be composition-aware, which the walker is not. # Wired into `make check` in the scripts/validate-api-gaps.rb style (stdlib only). # # Paths default to the repo layout but honour FIXTURE_MANIFEST / FIXTURE_OPENAPI @@ -89,30 +91,18 @@ def resolve_pointer(doc, pointer) [current, true] end -# --- Schema helpers (local; the walker keeps its resolver private) ------------- +# --- Schema helpers ------------------------------------------------------------ -def resolve_ref(schema, components) - seen = {} - current = schema - while current.is_a?(Hash) && current["$ref"].is_a?(String) - ref = current["$ref"] - break if seen[ref] - - seen[ref] = true - m = ref.match(%r{\A(?:openapi\.json)?#/components/schemas/(.+)\z}) - break unless m - - nxt = components[m[1]] - break unless nxt +# Ruby name of a `$ref`, or nil. +def ref_name(schema) + return nil unless schema.is_a?(Hash) && schema["$ref"].is_a?(String) - current = nxt - end - current + schema["$ref"].match(%r{/components/schemas/(.+)\z})&.captures&.first end -# Returns [Set(declared non-null json-type strings), nullable?]. Handles OpenAPI -# 3.1 null-union (`type: [X, "null"]`) and 3.0 `nullable: true`. An empty set -# means the type is unconstrained/compositional — skip type checks. +# Returns [Set(declared non-null json-type strings), nullable?] for a single +# schema node (no traversal). Handles OpenAPI 3.1 null-union (`type:[X,"null"]`) +# and 3.0 `nullable:true`. An empty set means the node declares no type. def allowed_types(schema) return [Set.new, false] unless schema.is_a?(Hash) @@ -158,55 +148,91 @@ def type_matches?(types, value) false end -# Recursive type/nullability validation. Reports (prefix-tagged) errors for: -# - a present value whose JSON type contradicts the declared type; -# - a REQUIRED object field present as null against a non-nullable schema; -# - a null ARRAY ELEMENT against a non-nullable item schema. -# Optional object-field nulls are tolerated: the OpenAPI generated from Smithy +# Merges a schema's effective constraints across `$ref` (including 3.1 +# `$ref`-with-siblings) and `allOf`, returning +# [required(Array), properties(Hash), types(Set), nullable(bool), items(schema)]. +# `anyOf`/`oneOf` are alternatives, so their required/properties are NOT merged +# (that would over-require). `visited` (component names) + depth guard terminate +# reference/composition cycles. +def merged_constraints(schema, components, visited = Set.new, depth = 0) + req = [] + props = {} + types = Set.new + nullable = false + items = nil + return [req, props, types, nullable, items] if depth > 40 || !schema.is_a?(Hash) + + absorb = lambda do |sub| + r2, p2, t2, n2, i2 = merged_constraints(sub, components, visited, depth + 1) + req.concat(r2) + p2.each { |k, v| props[k] ||= v } + types.merge(t2) + nullable ||= n2 + items ||= i2 + end + + name = ref_name(schema) + if name && !visited.include?(name) + visited << name + absorb.call(components[name]) + # fall through to local keywords (OpenAPI 3.1 permits $ref siblings) + end + + t, nn = allowed_types(schema) + types.merge(t) + nullable ||= nn + (schema["properties"] || {}).each { |k, v| props[k] ||= v } + (schema["required"] || []).each { |r| req << r } + items ||= schema["items"] + (schema["allOf"] || []).each { |sub| absorb.call(sub) } + + [req, props, types, nullable, items] +end + +# Composition-aware validation of `value` against `schema`. Reports +# (path-tagged) errors for a missing required field, a required field present as +# null against a non-nullable schema, a null array element against a non-nullable +# item schema, and a present value whose JSON type contradicts the declared type. +# Optional object-field nulls are tolerated: the Smithy-derived OpenAPI # under-marks some nullable optionals (e.g. Person.bio/location are `type:string` -# yet the wire sends null), so flagging them would be a false positive, not a -# fixture defect. -def type_errors(prefix, value, schema, components, depth = 0) - return [] if depth > 20 +# yet the wire sends null), so flagging them would be a false positive. +def instance_errors(prefix, value, schema, components, depth = 0) + return [] if depth > 60 + return [] if value.nil? # optional-null tolerated; required-/element-null handled in context - resolved = resolve_ref(schema, components) - return [] unless resolved.is_a?(Hash) - return [] if value.nil? # object-field optional-null tolerated (see note above) + req, props, types, _nullable, items = merged_constraints(schema, components) - types, = allowed_types(resolved) unless type_matches?(types, value) label = prefix.empty? ? "(root)" : prefix return ["#{label}: expected #{types.to_a.sort.join('|')}, got #{json_type(value)}"] end errs = [] - if value.is_a?(Hash) && resolved["properties"].is_a?(Hash) - props = resolved["properties"] - (resolved["required"] || []).each do |rk| - next unless value.key?(rk) && value[rk].nil? - - _, nullable = allowed_types(resolve_ref(props[rk] || {}, components)) - next if nullable - + if value.is_a?(Hash) + req.uniq.each do |rk| field = prefix.empty? ? rk : "#{prefix}/#{rk}" - errs << "#{field}: required field is null but its schema is not nullable" + if !value.key?(rk) + errs << "missing required field `#{field}`" + elsif value[rk].nil? + _, _, _, field_nullable, = merged_constraints(props[rk] || {}, components) + errs << "#{field}: required field is null but its schema is not nullable" unless field_nullable + end end value.each do |k, v| next unless props.key?(k) child = prefix.empty? ? k : "#{prefix}/#{k}" - errs.concat(type_errors(child, v, props[k], components, depth + 1)) + errs.concat(instance_errors(child, v, props[k], components, depth + 1)) end - elsif value.is_a?(Array) && resolved["items"] - items = resolved["items"] - _, item_nullable = allowed_types(resolve_ref(items, components)) + elsif value.is_a?(Array) && items + _, _, _, item_nullable, = merged_constraints(items, components) value.each_with_index do |item, i| ip = "#{prefix}[#{i}]" if item.nil? errs << "#{ip}: null array element but the item schema is not nullable" unless item_nullable next end - errs.concat(type_errors(ip, item, items, components, depth + 1)) + errs.concat(instance_errors(ip, item, items, components, depth + 1)) end end @@ -214,7 +240,7 @@ def type_errors(prefix, value, schema, components, depth = 0) end def concrete_for?(schema_name, instance, components) - types, = allowed_types(components[schema_name] || {}) + _, _, types, = merged_constraints({ "$ref" => "#/components/schemas/#{schema_name}" }, components) if types.include?("array") instance.is_a?(Array) && !instance.empty? else @@ -222,27 +248,22 @@ def concrete_for?(schema_name, instance, components) end end -# Ruby name of a `$ref`, or nil. -def ref_name(schema) - return nil unless schema.is_a?(Hash) && schema["$ref"].is_a?(String) - - schema["$ref"].match(%r{/components/schemas/(.+)\z})&.captures&.first -end - -# True when `schema` denotes (through $ref chains AND allOf/anyOf/oneOf -# composition) the RichTextAttachment component — so an alias +# True when `schema` denotes (through `$ref` chains + siblings and +# allOf/anyOf/oneOf composition) the RichTextAttachment component — so an alias # (`{$ref: SomeAlias}` -> RichTextAttachment) or a composed item schema is still -# recognized. `visited` tracks resolved component names to terminate ref cycles. +# recognized. `visited` (component names) terminates ref cycles. def rich_text_attachment?(schema, components, visited = Set.new, depth = 0) return false if depth > 40 || !schema.is_a?(Hash) name = ref_name(schema) if name return true if name == "RichTextAttachment" - return false if visited.include?(name) - visited << name - return rich_text_attachment?(components[name], components, visited, depth + 1) + unless visited.include?(name) + visited << name + return true if rich_text_attachment?(components[name], components, visited, depth + 1) + end + # fall through to local keywords ($ref siblings) end %w[allOf anyOf oneOf].each do |key| @@ -254,17 +275,17 @@ def rich_text_attachment?(schema, components, visited = Set.new, depth = 0) end # True when `schema` is (or composes/refs) an array whose items resolve to the -# RichTextAttachment component. Follows $ref chains and allOf/anyOf/oneOf on both -# the array schema and its items; `visited` guards ref cycles. +# RichTextAttachment component. Follows `$ref` (with siblings) and +# allOf/anyOf/oneOf on both the array schema and its items; `visited` guards +# cycles. def references_rich_text_array?(schema, components, visited = Set.new, depth = 0) return false if depth > 40 || !schema.is_a?(Hash) name = ref_name(schema) - if name - return false if visited.include?(name) - + if name && !visited.include?(name) visited << name - return references_rich_text_array?(components[name], components, visited, depth + 1) + return true if references_rich_text_array?(components[name], components, visited, depth + 1) + # fall through to local keywords ($ref siblings) end t = schema["type"] @@ -280,18 +301,17 @@ def references_rich_text_array?(schema, components, visited = Set.new, depth = 0 false end -# Collects every property schema of a component, following $ref and traversing -# component-level allOf/anyOf/oneOf so properties inherited through composition -# are seen. `visited` guards ref cycles. +# Collects every property schema of a component, following `$ref` (with siblings) +# and traversing component-level allOf/anyOf/oneOf so composed/inherited +# properties are seen. `visited` (component names) guards ref cycles. def collect_property_schemas(schema, components, out, visited = Set.new, depth = 0) return if depth > 40 || !schema.is_a?(Hash) name = ref_name(schema) - if name - return if visited.include?(name) - + if name && !visited.include?(name) visited << name - return collect_property_schemas(components[name], components, out, visited, depth + 1) + collect_property_schemas(components[name], components, out, visited, depth + 1) + # fall through to local keywords ($ref siblings) end (schema["properties"] || {}).each_value { |ps| out << ps } @@ -307,7 +327,8 @@ def collect_property_schemas(schema, components, out, visited = Set.new, depth = # NOT an independent emitter: covering its target covers it, and if that target # is itself an emitter it is caught directly. Skipping these keeps the emitter # set to the concrete decode types (the ~40 `*ResponseContent` response -# envelopes are pure aliases of already-covered components). +# envelopes are pure aliases of already-covered components). A `$ref` WITH local +# structure (3.1 siblings) is not a pure alias and is still examined. def pure_ref_alias?(schema) schema.is_a?(Hash) && schema.key?("$ref") && (schema.keys - %w[$ref description title]).empty? @@ -430,10 +451,7 @@ def rich_text_emitters(components) resolved_root[id] = { name: root_schema_name, concrete: concrete } where = pointer.empty? ? fixture_rel : "#{fixture_rel} at pointer `#{pointer}`" - walker.missing_required(instance, schema).each do |path| - fail_with(errors, "target `#{id}`: #{where} missing required field `#{path}`") - end - type_errors("", instance, schema, components).each do |msg| + instance_errors("", instance, schema, components).each do |msg| fail_with(errors, "target `#{id}`: #{where} #{msg}") end end diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 9b6e2e20e..59fb5e41b 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -37,6 +37,31 @@ def run_checker(manifest:, openapi: REAL_OPENAPI, fixtures: REAL_FIXTURES) Open3.capture2e(env, "ruby", CHECKER) end +# Like run_checker but spawns with a retained PID so a hung child can actually be +# killed on timeout (Open3.capture2e's cleanup would block waiting for the +# child). Raises Timeout::Error after `seconds`, having KILLed and reaped the +# process. Returns [combined_output, Process::Status]. +def run_checker_killable(manifest:, openapi:, fixtures:, seconds: 30) + env = { "FIXTURE_MANIFEST" => manifest, "FIXTURE_OPENAPI" => openapi, "FIXTURE_DIR" => fixtures } + reader, writer = IO.pipe + pid = Process.spawn(env, "ruby", CHECKER, out: writer, err: writer) + writer.close + out = +"" + pump = Thread.new { out = reader.read } + status = nil + begin + Timeout.timeout(seconds) { _, status = Process.wait2(pid) } + rescue Timeout::Error + Process.kill("KILL", pid) + Process.wait(pid) + raise + ensure + pump.join + reader.close + end + [out, status] +end + failures = [] def expect_pass(failures, label, out, status) @@ -151,6 +176,11 @@ def with_mutated_manifest "AliasedItemEmitter" => { "type" => "object", "properties" => { "description_attachments" => { "type" => "array", "items" => { "$ref" => "#/components/schemas/RTAAlias" } } } }, + # (2b) Emitter declared as a $ref WITH sibling local properties (OpenAPI 3.1) + # — the local content_attachments must be seen despite the $ref. + "SiblingEmitter" => { "$ref" => "#/components/schemas/Base", + "properties" => { "content_attachments" => { "type" => "array", + "items" => { "$ref" => "#/components/schemas/RichTextAttachment" } } } }, # (3a) Component-level composition cycle (no rich text) — must terminate. "CycleA" => { "allOf" => [{ "$ref" => "#/components/schemas/CycleB" }] }, "CycleB" => { "allOf" => [{ "$ref" => "#/components/schemas/CycleA" }] }, @@ -164,23 +194,58 @@ def with_mutated_manifest EMPTY_MANIFEST = { "targets" => [], "covered_schemas" => {}, "excluded_schemas" => {} }.freeze -def run_synthetic(openapi:, manifest:) +# A composed COVERED target: ComposedTarget's required fields (base_field, extra) +# live in $ref + allOf branches, and it is a rich-text emitter. An empty-object +# fixture must fail composition-aware required validation. +COMPOSED_OPENAPI = { + "components" => { "schemas" => { + "RichTextAttachment" => { "type" => "object", "properties" => { "id" => { "type" => "integer" } } }, + "Base" => { "type" => "object", "required" => ["base_field"], + "properties" => { "base_field" => { "type" => "string" } } }, + "ComposedTarget" => { "allOf" => [ + { "$ref" => "#/components/schemas/Base" }, + { "type" => "object", "required" => ["extra"], + "properties" => { "extra" => { "type" => "string" }, + "content_attachments" => { "type" => "array", + "items" => { "$ref" => "#/components/schemas/RichTextAttachment" } } } }, + ] }, + } }, +}.freeze + +COMPOSED_MANIFEST = { + "targets" => [{ "id" => "composed", "fixture" => "composed.json", "pointer" => "", "schema" => "ComposedTarget" }], + "covered_schemas" => { "ComposedTarget" => ["composed"] }, + "excluded_schemas" => {}, +}.freeze + +# Writes openapi + manifest (+ optional fixture files) to a tmp tree and runs the +# checker there, killably (so a cycle that hangs discovery is terminated, not +# left blocking the suite). +def run_synthetic(openapi:, manifest:, fixtures: {}) Dir.mktmpdir("fixture-cov-synthetic") do |dir| op = File.join(dir, "openapi.json") mf = File.join(dir, "manifest.yaml") + fx = File.join(dir, "fixtures") + FileUtils.mkdir_p(fx) File.write(op, JSON.generate(openapi)) File.write(mf, YAML.dump(manifest)) - Timeout.timeout(30) { run_checker(manifest: mf, openapi: op, fixtures: dir) } + fixtures.each do |rel, data| + p = File.join(fx, rel) + FileUtils.mkdir_p(File.dirname(p)) + File.write(p, JSON.generate(data)) + end + run_checker_killable(manifest: mf, openapi: op, fixtures: fx) end end begin out, status = run_synthetic(openapi: SYNTHETIC_OPENAPI, manifest: EMPTY_MANIFEST) - # (1) + (2): both indirect emitters must be discovered and flagged as unaccounted. + # Indirect emitters must all be discovered and flagged as unaccounted. expect_fail(failures, "component-level allOf emitter discovered", out, status, "`AllOfEmitter`") expect_fail(failures, "aliased RichTextAttachment item discovered", out, status, "`AliasedItemEmitter`") - # (3): the cycle components must NOT be misclassified as emitters, and — proven - # by the run returning at all under Timeout — discovery terminated safely. + expect_fail(failures, "$ref-with-siblings emitter discovered", out, status, "`SiblingEmitter`") + # The cycle components must NOT be misclassified as emitters, and — proven by + # the run returning at all under the killable timeout — discovery terminated. if out.include?("`CycleA`") || out.include?("`SelfRefItem`") || out.include?("`CycleItemHolder`") failures << "cycle components should not be flagged as emitters:\n#{out}" end @@ -188,10 +253,23 @@ def run_synthetic(openapi:, manifest:) failures << "emitter discovery did not terminate on a reference/composition cycle (hung)" end +# Composition-aware required validation: an empty fixture for a composed covered +# target must fail on the required fields contributed by its $ref/allOf branches. +begin + out, status = run_synthetic(openapi: COMPOSED_OPENAPI, manifest: COMPOSED_MANIFEST, + fixtures: { "composed.json" => {} }) + expect_fail(failures, "composed covered target with empty fixture fails", out, status, "missing required field") + %w[base_field extra].each do |f| + failures << "composed-target test did not report missing `#{f}`:\n#{out}" unless out.include?("`#{f}`") + end +rescue Timeout::Error + failures << "composed-target validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 3 synthetic emitter cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 5 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml index 8a93c3bfa..96e231add 100644 --- a/spec/fixtures/manifest.yaml +++ b/spec/fixtures/manifest.yaml @@ -8,8 +8,10 @@ # guarantee; the coverage invariant closes the gap. # # Enforced by scripts/check-fixture-coverage.rb (wired into `make check`). The -# checker reuses conformance/runner/ruby/schema-walker.rb for the recursive -# required-field / $ref / array walk — it does not reimplement validation. +# checker uses conformance/runner/ruby/schema-walker.rb for operation-entry +# response-schema lookup; the required/type/nullability walk is its own because +# it must be composition-aware ($ref-with-siblings + allOf), which the walker +# is not. # # --- Entry semantics ----------------------------------------------------------- # @@ -60,9 +62,11 @@ # --- Per-target validation ---------------------------------------------------- # # Each target's selected instance is validated against its schema for BOTH -# required-field presence (the conformance walker) AND type/nullability: a -# required field that is null against a non-nullable schema, or a value whose -# JSON type contradicts the declared type (e.g. a string where an array is +# required-field presence AND type/nullability, composition-aware ($ref-with- +# siblings + allOf are merged): a missing required field (including one +# contributed by an allOf branch), a required field null against a non-nullable +# schema, a null array element against a non-nullable item schema, or a value +# whose JSON type contradicts the declared type (e.g. a string where an array is # required), fails the target. So a fixture that no longer matches the generated # types is caught, not just one missing a key. # From cc89bd7286210c813e9888cb9b1693cfd2dd23b1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 00:41:27 -0700 Subject: [PATCH 09/16] Fixture guard: validate anyOf/oneOf alternatives (at-least-one-branch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last validation gap on #432. merged_constraints already excluded anyOf/oneOf from the merged required/properties (correct — they're alternatives, not a conjunction), but instance_errors then never checked them, so a fixture could omit every alternative's required fields and pass. Now merged_constraints also collects each anyOf/oneOf group (including groups inherited through $ref and allOf), and instance_errors requires the value to satisfy at least one branch of each group — reporting "matches none of the N allowed alternatives" otherwise. oneOf is validated as at-least-one (enforcing exactly-one would need full discriminator/enum/const validation to avoid false positives, out of scope for this partial validator). The real spec's only oneOf (TodolistOrGroup) is reached solely by two *ResponseContent aliases, not by any covered target, so real fixtures are unaffected. Self-test adds two cases (7 synthetic total): an empty object matching no alternative fails; a value matching a branch passes — with the group inherited via allOf -> $ref. --- scripts/check-fixture-coverage.rb | 35 ++++++++++++++++++++------ scripts/test-check-fixture-coverage.rb | 31 ++++++++++++++++++++++- spec/fixtures/manifest.yaml | 8 +++--- 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index d950e77f3..ed0ac8af3 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -150,25 +150,30 @@ def type_matches?(types, value) # Merges a schema's effective constraints across `$ref` (including 3.1 # `$ref`-with-siblings) and `allOf`, returning -# [required(Array), properties(Hash), types(Set), nullable(bool), items(schema)]. -# `anyOf`/`oneOf` are alternatives, so their required/properties are NOT merged -# (that would over-require). `visited` (component names) + depth guard terminate -# reference/composition cycles. +# [required(Array), properties(Hash), types(Set), nullable(bool), items(schema), +# alt_groups(Array of branch-arrays)]. +# `allOf` is a conjunction (merged into required/properties). `anyOf`/`oneOf` are +# alternatives: their branches are NOT merged (that would over-require) but each +# group is collected so validation can require at least one branch to match — +# including groups inherited through `$ref` and `allOf`. `visited` (component +# names) + depth guard terminate reference/composition cycles. def merged_constraints(schema, components, visited = Set.new, depth = 0) req = [] props = {} types = Set.new nullable = false items = nil - return [req, props, types, nullable, items] if depth > 40 || !schema.is_a?(Hash) + alt_groups = [] + return [req, props, types, nullable, items, alt_groups] if depth > 40 || !schema.is_a?(Hash) absorb = lambda do |sub| - r2, p2, t2, n2, i2 = merged_constraints(sub, components, visited, depth + 1) + r2, p2, t2, n2, i2, a2 = merged_constraints(sub, components, visited, depth + 1) req.concat(r2) p2.each { |k, v| props[k] ||= v } types.merge(t2) nullable ||= n2 items ||= i2 + alt_groups.concat(a2) end name = ref_name(schema) @@ -185,8 +190,11 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) (schema["required"] || []).each { |r| req << r } items ||= schema["items"] (schema["allOf"] || []).each { |sub| absorb.call(sub) } + %w[anyOf oneOf].each do |key| + alt_groups << schema[key] if schema[key].is_a?(Array) && !schema[key].empty? + end - [req, props, types, nullable, items] + [req, props, types, nullable, items, alt_groups] end # Composition-aware validation of `value` against `schema`. Reports @@ -200,7 +208,7 @@ def instance_errors(prefix, value, schema, components, depth = 0) return [] if depth > 60 return [] if value.nil? # optional-null tolerated; required-/element-null handled in context - req, props, types, _nullable, items = merged_constraints(schema, components) + req, props, types, _nullable, items, alt_groups = merged_constraints(schema, components) unless type_matches?(types, value) label = prefix.empty? ? "(root)" : prefix @@ -208,6 +216,17 @@ def instance_errors(prefix, value, schema, components, depth = 0) end errs = [] + + # anyOf/oneOf: the value must satisfy at least one branch of each group + # (oneOf is validated as "at least one" — enforcing exactly-one would need full + # discriminator/enum/const validation to avoid false positives). + alt_groups.each do |branches| + next if branches.any? { |branch| instance_errors(prefix, value, branch, components, depth + 1).empty? } + + label = prefix.empty? ? "(root)" : prefix + errs << "#{label}: value matches none of the #{branches.length} allowed alternatives (anyOf/oneOf)" + end + if value.is_a?(Hash) req.uniq.each do |rk| field = prefix.empty? ? rk : "#{prefix}/#{rk}" diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 59fb5e41b..0ffe4d0d7 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -266,10 +266,39 @@ def run_synthetic(openapi:, manifest:, fixtures: {}) failures << "composed-target validation hung" end +# Alternative-group (anyOf/oneOf) validation, with the group inherited through +# BOTH allOf and $ref (ComposedAlt -> allOf -> $ref AltBase -> oneOf). A value +# must satisfy at least one branch; an empty object satisfies neither. +ALT_OPENAPI = { + "components" => { "schemas" => { + "AltBase" => { "oneOf" => [ + { "type" => "object", "required" => ["a"], "properties" => { "a" => { "type" => "string" } } }, + { "type" => "object", "required" => ["b"], "properties" => { "b" => { "type" => "string" } } }, + ] }, + "ComposedAlt" => { "allOf" => [{ "$ref" => "#/components/schemas/AltBase" }] }, + } }, +}.freeze + +def alt_manifest + { "targets" => [{ "id" => "alt", "fixture" => "alt.json", "pointer" => "", "schema" => "ComposedAlt" }], + "covered_schemas" => { "ComposedAlt" => ["alt"] }, + "excluded_schemas" => {} } +end + +begin + out, status = run_synthetic(openapi: ALT_OPENAPI, manifest: alt_manifest, fixtures: { "alt.json" => {} }) + expect_fail(failures, "anyOf/oneOf: empty object matches no alternative", out, status, "matches none of") + + out, status = run_synthetic(openapi: ALT_OPENAPI, manifest: alt_manifest, fixtures: { "alt.json" => { "a" => "x" } }) + expect_pass(failures, "anyOf/oneOf: value matching a branch passes", out, status) +rescue Timeout::Error + failures << "alternative-group validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 5 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 7 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml index 96e231add..03c901b53 100644 --- a/spec/fixtures/manifest.yaml +++ b/spec/fixtures/manifest.yaml @@ -63,9 +63,11 @@ # # Each target's selected instance is validated against its schema for BOTH # required-field presence AND type/nullability, composition-aware ($ref-with- -# siblings + allOf are merged): a missing required field (including one -# contributed by an allOf branch), a required field null against a non-nullable -# schema, a null array element against a non-nullable item schema, or a value +# siblings + allOf are merged; anyOf/oneOf groups — including those inherited via +# $ref/allOf — require the value to satisfy at least one branch): a missing +# required field (including one contributed by an allOf branch), a required field +# null against a non-nullable schema, a null array element against a non-nullable +# item schema, a value matching none of an anyOf/oneOf's alternatives, or a value # whose JSON type contradicts the declared type (e.g. a string where an array is # required), fails the target. So a fixture that no longer matches the generated # types is caught, not just one missing a key. From 23d5dadcf8c4c11b7505830d4fcb5bf62698ee91 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 00:43:55 -0700 Subject: [PATCH 10/16] Fixture guard: allOf nullability is conjunctive (null needs every branch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on #432: merged_constraints OR-merged `nullable`, so a required field whose schema is allOf[nullable-branch, non-nullable-branch] was treated as nullable and a null value slipped through — but allOf is a conjunction, so null is valid only if EVERY branch (and the $ref target, and the node's own type) permits it. Now the merge accumulates `forbids_null` (a part with a concrete type that does not allow null forbids it) across $ref + allOf and returns its negation as the nullable flag. So the required-field-null and null-array-element checks reject null unless every conjunctive part allows it. Self-test adds an allOf[nullable, non-nullable] required field: null is rejected, a concrete value passes (9 synthetic cases total). --- scripts/check-fixture-coverage.rb | 16 +++++++----- scripts/test-check-fixture-coverage.rb | 35 +++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index ed0ac8af3..d3f7993a9 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -161,17 +161,20 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) req = [] props = {} types = Set.new - nullable = false + # `$ref`(+siblings) and allOf form a CONJUNCTION: null is allowed only if every + # part allows it, so a part that imposes a non-nullable type FORBIDS null. We + # accumulate `forbids_null` (OR) and return its negation as the nullable flag. + forbids_null = false items = nil alt_groups = [] - return [req, props, types, nullable, items, alt_groups] if depth > 40 || !schema.is_a?(Hash) + return [req, props, types, true, items, alt_groups] if depth > 40 || !schema.is_a?(Hash) absorb = lambda do |sub| - r2, p2, t2, n2, i2, a2 = merged_constraints(sub, components, visited, depth + 1) + r2, p2, t2, sub_nullable, i2, a2 = merged_constraints(sub, components, visited, depth + 1) req.concat(r2) p2.each { |k, v| props[k] ||= v } types.merge(t2) - nullable ||= n2 + forbids_null ||= !sub_nullable items ||= i2 alt_groups.concat(a2) end @@ -185,7 +188,8 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) t, nn = allowed_types(schema) types.merge(t) - nullable ||= nn + # A node that imposes a concrete type but does not permit null forbids null. + forbids_null ||= (!t.empty? && !nn) (schema["properties"] || {}).each { |k, v| props[k] ||= v } (schema["required"] || []).each { |r| req << r } items ||= schema["items"] @@ -194,7 +198,7 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) alt_groups << schema[key] if schema[key].is_a?(Array) && !schema[key].empty? end - [req, props, types, nullable, items, alt_groups] + [req, props, types, !forbids_null, items, alt_groups] end # Composition-aware validation of `value` against `schema`. Reports diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 0ffe4d0d7..a17da2332 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -295,10 +295,43 @@ def alt_manifest failures << "alternative-group validation hung" end +# allOf nullability is a conjunction: a required field whose schema is +# allOf[nullable-branch, non-nullable-branch] must reject null (one branch +# forbids it), not accept it because a single branch is nullable. +NULLABLE_ALLOF_OPENAPI = { + "components" => { "schemas" => { + "NullAllOfTarget" => { "type" => "object", "required" => ["f"], "properties" => { + "f" => { "allOf" => [ + { "type" => "string", "nullable" => true }, + { "type" => "string" }, + ] }, + } }, + } }, +}.freeze + +def null_allof_manifest + { "targets" => [{ "id" => "nallof", "fixture" => "nallof.json", "pointer" => "", "schema" => "NullAllOfTarget" }], + "covered_schemas" => { "NullAllOfTarget" => ["nallof"] }, + "excluded_schemas" => {} } +end + +begin + out, status = run_synthetic(openapi: NULLABLE_ALLOF_OPENAPI, manifest: null_allof_manifest, + fixtures: { "nallof.json" => { "f" => nil } }) + expect_fail(failures, "allOf nullability is conjunctive (null rejected)", out, status, + "required field is null but its schema is not nullable") + + out, status = run_synthetic(openapi: NULLABLE_ALLOF_OPENAPI, manifest: null_allof_manifest, + fixtures: { "nallof.json" => { "f" => "x" } }) + expect_pass(failures, "allOf field with a concrete value passes", out, status) +rescue Timeout::Error + failures << "allOf-nullability validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 7 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 9 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" From 35fc26c0df8a077c5dfb91c135809ed5083aa226 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 00:57:20 -0700 Subject: [PATCH 11/16] Fixture guard: fold anyOf/oneOf into nullability (reject null-typed-by-non-null-alts) Codex review on #432: a required field typed only by non-null alternatives (e.g. oneOf:[{type:string},{type:integer}]) still accepted null. merged_constraints collected the alternative groups but never folded them into forbids_null, so the schema reported nullable:true and the required-field-null check trusted it. Now, after collecting alt_groups, each group is folded into forbids_null: a group permits null only if at least one branch permits it; if every branch forbids null, the group forbids null (the value must satisfy some branch). Branch nullability is computed with a FRESH visited set so outer traversal state can't short-circuit it, and the result conjoins with the surrounding $ref/allOf constraints. Reproduced the reported false negative ({"f": null} against oneOf[string,integer]) now fails correctly. Self-test adds two cases (11 synthetic total): non-null alternatives reject required null; an explicitly-nullable alternative accepts it. --- scripts/check-fixture-coverage.rb | 13 ++++++++++ scripts/test-check-fixture-coverage.rb | 35 +++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index d3f7993a9..635f290b9 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -198,6 +198,19 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) alt_groups << schema[key] if schema[key].is_a?(Array) && !schema[key].empty? end + # An anyOf/oneOf group (local or inherited via $ref/allOf) permits null only if + # at least one branch does; if every branch forbids null, the group forbids it + # (the value must satisfy some branch). Conjoin that with the surrounding + # $ref/allOf constraints. Branch nullability is computed with a FRESH visited + # set so outer traversal state can't short-circuit it. + alt_groups.each do |branches| + group_allows_null = branches.any? do |branch| + _, _, _, branch_nullable, = merged_constraints(branch, components, Set.new, depth + 1) + branch_nullable + end + forbids_null ||= !group_allows_null + end + [req, props, types, !forbids_null, items, alt_groups] end diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index a17da2332..cfb6180ad 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -328,10 +328,43 @@ def null_allof_manifest failures << "allOf-nullability validation hung" end +# anyOf/oneOf nullability: a required field typed only by non-null alternatives +# must REJECT null (no branch permits it); an explicitly-nullable alternative +# must ACCEPT it. This is the path {}-fails / matching-object-passes can't reach. +ALT_NULL_OPENAPI = { + "components" => { "schemas" => { + "AltNullReject" => { "type" => "object", "required" => ["f"], "properties" => { + "f" => { "oneOf" => [{ "type" => "string" }, { "type" => "integer" }] }, + } }, + "AltNullAccept" => { "type" => "object", "required" => ["f"], "properties" => { + "f" => { "oneOf" => [{ "type" => "string", "nullable" => true }, { "type" => "integer" }] }, + } }, + } }, +}.freeze + +def alt_null_manifest(schema, id) + { "targets" => [{ "id" => id, "fixture" => "#{id}.json", "pointer" => "", "schema" => schema }], + "covered_schemas" => { schema => [id] }, + "excluded_schemas" => {} } +end + +begin + out, status = run_synthetic(openapi: ALT_NULL_OPENAPI, manifest: alt_null_manifest("AltNullReject", "reject"), + fixtures: { "reject.json" => { "f" => nil } }) + expect_fail(failures, "anyOf/oneOf: non-null alternatives reject required null", out, status, + "required field is null but its schema is not nullable") + + out, status = run_synthetic(openapi: ALT_NULL_OPENAPI, manifest: alt_null_manifest("AltNullAccept", "accept"), + fixtures: { "accept.json" => { "f" => nil } }) + expect_pass(failures, "anyOf/oneOf: an explicitly-nullable alternative accepts null", out, status) +rescue Timeout::Error + failures << "alternative-nullability validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 9 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 11 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" From c261cb15964198e19606de868874315f2634e33f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 01:03:12 -0700 Subject: [PATCH 12/16] Fixture guard: intersect allOf type constraints; pin Vitest fs allow-list Further review round on #432: - allOf type constraints now INTERSECT (Codex): a value must satisfy every conjunctive part's declared type, not the union. merged_constraints collects per-part type-sets and instance_errors checks the value against each; and a property declared in multiple allOf branches is conjoined (allOf-wrapped) rather than first-wins, so a field constrained by several branches must match all of them. A value matching only one contradictory branch now fails. - Vitest fs allow-list pinned to the repo root (Copilot x5): the service tests import JSON fixtures from /spec/fixtures (outside the typescript package). It worked via Vite's git-root workspace detection, but is now made explicit via server.fs.allow so a nested package.json or a strict downstream environment can't narrow it. Self-test grows to 12 synthetic cases (adds the allOf type-intersection case). The array-item alternative-nullability path (Codex) was already covered by the prior anyOf/oneOf-nullability fix and is exercised by the null-array-element handling. oneOf remains validated as at-least-one per the review's explicit guidance (exactly-one would need full discriminator/enum/const validation). --- scripts/check-fixture-coverage.rb | 35 +++++++++++++++++++------- scripts/test-check-fixture-coverage.rb | 26 ++++++++++++++++++- typescript/vitest.config.ts | 14 +++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 635f290b9..698e3b395 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -160,20 +160,31 @@ def type_matches?(types, value) def merged_constraints(schema, components, visited = Set.new, depth = 0) req = [] props = {} - types = Set.new + types = Set.new # union of all declared types (for messages + concrete_for?) + type_sets = [] # per-conjunctive-part declared type-sets — a value must + # satisfy EVERY one (allOf/$ref are a conjunction, so their + # type constraints INTERSECT, not union). # `$ref`(+siblings) and allOf form a CONJUNCTION: null is allowed only if every # part allows it, so a part that imposes a non-nullable type FORBIDS null. We # accumulate `forbids_null` (OR) and return its negation as the nullable flag. forbids_null = false items = nil alt_groups = [] - return [req, props, types, true, items, alt_groups] if depth > 40 || !schema.is_a?(Hash) + return [req, props, types, true, items, alt_groups, type_sets] if depth > 40 || !schema.is_a?(Hash) + + # When the same property is constrained by more than one conjunctive part + # (e.g. declared in two allOf branches), conjoin the schemas so the field must + # satisfy ALL of them — not just the first seen. + add_prop = lambda do |k, v| + props[k] = props.key?(k) ? { "allOf" => [props[k], v] } : v + end absorb = lambda do |sub| - r2, p2, t2, sub_nullable, i2, a2 = merged_constraints(sub, components, visited, depth + 1) + r2, p2, t2, sub_nullable, i2, a2, ts2 = merged_constraints(sub, components, visited, depth + 1) req.concat(r2) - p2.each { |k, v| props[k] ||= v } + p2.each { |k, v| add_prop.call(k, v) } types.merge(t2) + type_sets.concat(ts2) forbids_null ||= !sub_nullable items ||= i2 alt_groups.concat(a2) @@ -188,9 +199,10 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) t, nn = allowed_types(schema) types.merge(t) + type_sets << t unless t.empty? # A node that imposes a concrete type but does not permit null forbids null. forbids_null ||= (!t.empty? && !nn) - (schema["properties"] || {}).each { |k, v| props[k] ||= v } + (schema["properties"] || {}).each { |k, v| add_prop.call(k, v) } (schema["required"] || []).each { |r| req << r } items ||= schema["items"] (schema["allOf"] || []).each { |sub| absorb.call(sub) } @@ -211,7 +223,7 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) forbids_null ||= !group_allows_null end - [req, props, types, !forbids_null, items, alt_groups] + [req, props, types, !forbids_null, items, alt_groups, type_sets] end # Composition-aware validation of `value` against `schema`. Reports @@ -225,11 +237,16 @@ def instance_errors(prefix, value, schema, components, depth = 0) return [] if depth > 60 return [] if value.nil? # optional-null tolerated; required-/element-null handled in context - req, props, types, _nullable, items, alt_groups = merged_constraints(schema, components) + req, props, _types, _nullable, items, alt_groups, type_sets = merged_constraints(schema, components) + + # The value must satisfy EVERY conjunctive part's declared type (allOf/$ref + # intersect their type constraints — a value matching only one contradictory + # branch fails). + type_sets.each do |ts| + next if type_matches?(ts, value) - unless type_matches?(types, value) label = prefix.empty? ? "(root)" : prefix - return ["#{label}: expected #{types.to_a.sort.join('|')}, got #{json_type(value)}"] + return ["#{label}: expected #{ts.to_a.sort.join('|')}, got #{json_type(value)}"] end errs = [] diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index cfb6180ad..e17b48053 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -361,10 +361,34 @@ def alt_null_manifest(schema, id) failures << "alternative-nullability validation hung" end +# allOf type constraints INTERSECT: a property declared in two allOf branches +# with conflicting types must satisfy both. A value matching only one branch +# fails (union would have wrongly accepted it). +INTERSECT_OPENAPI = { + "components" => { "schemas" => { + "IntersectTarget" => { "allOf" => [ + { "type" => "object", "required" => ["f"], "properties" => { "f" => { "type" => "string" } } }, + { "type" => "object", "properties" => { "f" => { "type" => "integer" } } }, + ] }, + } }, +}.freeze + +begin + out, status = run_synthetic( + openapi: INTERSECT_OPENAPI, + manifest: { "targets" => [{ "id" => "isect", "fixture" => "isect.json", "pointer" => "", "schema" => "IntersectTarget" }], + "covered_schemas" => { "IntersectTarget" => ["isect"] }, "excluded_schemas" => {} }, + fixtures: { "isect.json" => { "f" => "x" } }, + ) + expect_fail(failures, "allOf type constraints intersect (conflicting branch)", out, status, "expected integer, got string") +rescue Timeout::Error + failures << "allOf type-intersection validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 11 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 12 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" diff --git a/typescript/vitest.config.ts b/typescript/vitest.config.ts index d32cdb3e1..12f128d11 100644 --- a/typescript/vitest.config.ts +++ b/typescript/vitest.config.ts @@ -1,6 +1,20 @@ import { defineConfig } from "vitest/config"; +import { fileURLToPath } from "node:url"; + +// Repo root (parent of typescript/). Service tests source their stub bodies from +// the shared, coverage-guarded fixtures under /spec/fixtures via JSON +// imports; pin the fs allow-list to the repo root so those out-of-package +// imports resolve regardless of Vite's workspace-root auto-detection (which a +// nested package.json or a strict downstream environment could otherwise +// narrow). +const repoRoot = fileURLToPath(new URL("..", import.meta.url)); export default defineConfig({ + server: { + fs: { + allow: [repoRoot], + }, + }, test: { globals: true, environment: "node", From 170eb1015cdd8014e5646cd467c4fe9562a4e080 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 01:05:07 -0700 Subject: [PATCH 13/16] Harden run_checker_killable against the Timeout-after-reap race Copilot on #432: Timeout can fire just after Process.wait2 already reaped the child, so the rescue's Process.kill/Process.wait would hit an already-gone PID. Tolerate Errno::ESRCH/ECHILD in the kill+reap so the timeout path can't itself raise a spurious error. --- scripts/test-check-fixture-coverage.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index e17b48053..6f62314bb 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -52,8 +52,14 @@ def run_checker_killable(manifest:, openapi:, fixtures:, seconds: 30) begin Timeout.timeout(seconds) { _, status = Process.wait2(pid) } rescue Timeout::Error - Process.kill("KILL", pid) - Process.wait(pid) + # Timeout can fire just after wait2 already reaped the child, so the kill/ + # reap may hit an already-gone process — tolerate ESRCH/ECHILD. + begin + Process.kill("KILL", pid) + Process.wait(pid) + rescue Errno::ESRCH, Errno::ECHILD + # already exited/reaped + end raise ensure pump.join From 67bd5ffa86152bda2bcd8e36562ab0c72aa352d9 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 10:13:24 -0700 Subject: [PATCH 14/16] Fixture guard: conjoin allOf array items; accept scalar type:"null" Further review on #432: - allOf now conjoins array `items` across branches (Codex/Cubic): `items` was first-write-wins, so an array composed from multiple allOf branches enforced only the first branch's item schema. It now allOf-wraps them (same as the property conjoin), so an element must satisfy every branch's item schema. - OpenAPI 3.1 scalar `type: "null"` is recognized as nullable (Codex): the `when String` branch treated "null" as an ordinary non-null type, so a required field typed `{type: "null"}` wrongly rejected a null value. It now imposes no non-null type and reports nullable. - merged_constraints doc contract updated to list all seven return values (adds type_sets) (Cubic). Self-test grows to 14 synthetic cases (array-items-conjoin negative; scalar type:"null" positive). --- scripts/check-fixture-coverage.rb | 32 ++++++++++++------ scripts/test-check-fixture-coverage.rb | 46 +++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 698e3b395..31fca4581 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -112,6 +112,10 @@ def allowed_types(schema) members = t.compact [Set.new(members - ["null"]), members.include?("null")] when String + # OpenAPI 3.1 scalar null type: the value must be null, so it imposes no + # non-null type constraint and IS nullable. + return [Set.new, true] if t == "null" + [Set.new([t]), schema["nullable"] == true] else [Set.new, schema["nullable"] == true] @@ -151,12 +155,15 @@ def type_matches?(types, value) # Merges a schema's effective constraints across `$ref` (including 3.1 # `$ref`-with-siblings) and `allOf`, returning # [required(Array), properties(Hash), types(Set), nullable(bool), items(schema), -# alt_groups(Array of branch-arrays)]. -# `allOf` is a conjunction (merged into required/properties). `anyOf`/`oneOf` are -# alternatives: their branches are NOT merged (that would over-require) but each -# group is collected so validation can require at least one branch to match — -# including groups inherited through `$ref` and `allOf`. `visited` (component -# names) + depth guard terminate reference/composition cycles. +# alt_groups(Array of branch-arrays), type_sets(Array of Sets)]. +# `allOf` is a conjunction: required is unioned, and properties AND array `items` +# constrained by more than one branch are conjoined (allOf-wrapped) so a value +# must satisfy all of them; `type_sets` holds each part's declared type-set (a +# value must match every one). `anyOf`/`oneOf` are alternatives: their branches +# are NOT merged (that would over-require) but each group is collected so +# validation can require at least one branch to match — including groups +# inherited through `$ref` and `allOf`. `visited` (component names) + depth guard +# terminate reference/composition cycles. def merged_constraints(schema, components, visited = Set.new, depth = 0) req = [] props = {} @@ -172,12 +179,15 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) alt_groups = [] return [req, props, types, true, items, alt_groups, type_sets] if depth > 40 || !schema.is_a?(Hash) - # When the same property is constrained by more than one conjunctive part - # (e.g. declared in two allOf branches), conjoin the schemas so the field must - # satisfy ALL of them — not just the first seen. + # When the same property (or array `items`) is constrained by more than one + # conjunctive part (e.g. declared in two allOf branches), conjoin the schemas + # so the value must satisfy ALL of them — not just the first seen. add_prop = lambda do |k, v| props[k] = props.key?(k) ? { "allOf" => [props[k], v] } : v end + add_items = lambda do |i| + items = items ? { "allOf" => [items, i] } : i + end absorb = lambda do |sub| r2, p2, t2, sub_nullable, i2, a2, ts2 = merged_constraints(sub, components, visited, depth + 1) @@ -186,7 +196,7 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) types.merge(t2) type_sets.concat(ts2) forbids_null ||= !sub_nullable - items ||= i2 + add_items.call(i2) if i2 alt_groups.concat(a2) end @@ -204,7 +214,7 @@ def merged_constraints(schema, components, visited = Set.new, depth = 0) forbids_null ||= (!t.empty? && !nn) (schema["properties"] || {}).each { |k, v| add_prop.call(k, v) } (schema["required"] || []).each { |r| req << r } - items ||= schema["items"] + add_items.call(schema["items"]) if schema["items"] (schema["allOf"] || []).each { |sub| absorb.call(sub) } %w[anyOf oneOf].each do |key| alt_groups << schema[key] if schema[key].is_a?(Array) && !schema[key].empty? diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 6f62314bb..7eaa8252d 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -391,10 +391,54 @@ def alt_null_manifest(schema, id) failures << "allOf type-intersection validation hung" end +# allOf conjoins array `items` from every branch: an element must satisfy all +# item schemas, not just the first branch's. +ITEMS_ALLOF_OPENAPI = { + "components" => { "schemas" => { + "ItemsTarget" => { "allOf" => [ + { "type" => "object", "required" => ["xs"], + "properties" => { "xs" => { "type" => "array", "items" => { "type" => "string" } } } }, + { "type" => "object", + "properties" => { "xs" => { "type" => "array", "items" => { "type" => "integer" } } } }, + ] }, + } }, +}.freeze + +begin + out, status = run_synthetic( + openapi: ITEMS_ALLOF_OPENAPI, + manifest: { "targets" => [{ "id" => "items", "fixture" => "items.json", "pointer" => "", "schema" => "ItemsTarget" }], + "covered_schemas" => { "ItemsTarget" => ["items"] }, "excluded_schemas" => {} }, + fixtures: { "items.json" => { "xs" => ["hello"] } }, + ) + expect_fail(failures, "allOf conjoins array items across branches", out, status, "expected integer, got string") +rescue Timeout::Error + failures << "allOf items-conjoin validation hung" +end + +# OpenAPI 3.1 scalar `type: "null"`: a required field so typed accepts null. +NULL_TYPE_OPENAPI = { + "components" => { "schemas" => { + "NullTypeTarget" => { "type" => "object", "required" => ["n"], "properties" => { "n" => { "type" => "null" } } }, + } }, +}.freeze + +begin + out, status = run_synthetic( + openapi: NULL_TYPE_OPENAPI, + manifest: { "targets" => [{ "id" => "nt", "fixture" => "nt.json", "pointer" => "", "schema" => "NullTypeTarget" }], + "covered_schemas" => { "NullTypeTarget" => ["nt"] }, "excluded_schemas" => {} }, + fixtures: { "nt.json" => { "n" => nil } }, + ) + expect_pass(failures, 'scalar type:"null" required field accepts null', out, status) +rescue Timeout::Error + failures << "null-type validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 12 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 14 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" From a5c6effcb84fd8c6049fe320c81feeac1c09b24a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 13:26:20 -0700 Subject: [PATCH 15/16] Fixture guard: scalar type:"null" imposes a real null constraint; document scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #432: the scalar-null handling returned an EMPTY type set, which imposed no constraint at all — so `type: "null"` accepted every value, not just null. Return Set["null"] so a non-null value fails ("expected null, got …") while a null value is still accepted (nullable). Self-test adds the negative case (non-null value rejected); 15 synthetic cases total. Also documents the checker as a deliberately PARTIAL structural validator — required/types/nullability/arrays/$ref/allOf/at-least-one-alternative — and lists the keywords intentionally out of scope (exact-one oneOf, enum, const, discriminators, pattern, format, numeric bounds, additionalProperties, …), to be declined unless a case is reached by the actual generated schemas. --- scripts/check-fixture-coverage.rb | 27 +++++++++++++++++++++++--- scripts/test-check-fixture-coverage.rb | 10 +++++++++- spec/fixtures/manifest.yaml | 5 +++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 31fca4581..3a8a2c9fe 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -34,6 +34,26 @@ # Paths default to the repo layout but honour FIXTURE_MANIFEST / FIXTURE_OPENAPI # / FIXTURE_DIR env overrides so the negative-case self-test # (scripts/test-check-fixture-coverage.rb) can point it at crafted inputs. +# +# --- Scope: a deliberately PARTIAL structural validator ------------------------ +# +# This is NOT a complete JSON-Schema / OpenAPI validator. It validates the subset +# that keeps a fixture structurally faithful to the generated types: +# * required-field presence +# * declared types (with integer/number integrality) and nullability +# * arrays and array-element typing/nullability +# * $ref (incl. 3.1 $ref-with-siblings) and allOf conjunction (required unioned; +# duplicate properties and array `items` conjoined; type constraints +# intersected; nullability = every part permits null) +# * anyOf/oneOf as AT-LEAST-ONE (the value must satisfy some branch; a group is +# nullable only if a branch is) +# +# Intentionally NOT implemented (out of scope unless a case is reached by the +# actual generated schemas): exact-one `oneOf` selection, `enum`, `const`, +# discriminators, `pattern`, `format`, numeric bounds, `additionalProperties`, +# `uniqueItems`, and other assertion keywords. Findings about these are declined +# unless they affect the current generated `openapi.json` or this documented +# subset. require "json" require "yaml" @@ -112,9 +132,10 @@ def allowed_types(schema) members = t.compact [Set.new(members - ["null"]), members.include?("null")] when String - # OpenAPI 3.1 scalar null type: the value must be null, so it imposes no - # non-null type constraint and IS nullable. - return [Set.new, true] if t == "null" + # OpenAPI 3.1 scalar null type: the value must BE null — a real "null" type + # constraint (so a non-null value fails), and it is nullable. Returning an + # empty type set would wrongly impose no constraint at all. + return [Set.new(["null"]), true] if t == "null" [Set.new([t]), schema["nullable"] == true] else diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index 7eaa8252d..e8d1823bd 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -431,6 +431,14 @@ def alt_null_manifest(schema, id) fixtures: { "nt.json" => { "n" => nil } }, ) expect_pass(failures, 'scalar type:"null" required field accepts null', out, status) + + out, status = run_synthetic( + openapi: NULL_TYPE_OPENAPI, + manifest: { "targets" => [{ "id" => "nt", "fixture" => "nt.json", "pointer" => "", "schema" => "NullTypeTarget" }], + "covered_schemas" => { "NullTypeTarget" => ["nt"] }, "excluded_schemas" => {} }, + fixtures: { "nt.json" => { "n" => "not-null" } }, + ) + expect_fail(failures, 'scalar type:"null" rejects a non-null value', out, status, "expected null, got string") rescue Timeout::Error failures << "null-type validation hung" end @@ -438,7 +446,7 @@ def alt_null_manifest(schema, id) # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 14 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 15 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:" diff --git a/spec/fixtures/manifest.yaml b/spec/fixtures/manifest.yaml index 03c901b53..e515ee22b 100644 --- a/spec/fixtures/manifest.yaml +++ b/spec/fixtures/manifest.yaml @@ -72,6 +72,11 @@ # required), fails the target. So a fixture that no longer matches the generated # types is caught, not just one missing a key. # +# This is a deliberately PARTIAL structural validator (required/types/nullability/ +# arrays/$ref/allOf/at-least-one-alternative) — not a full JSON-Schema +# implementation. See the "Scope" note in scripts/check-fixture-coverage.rb for +# the supported subset and the keywords intentionally out of scope. +# # A missing fixture, unresolvable pointer, unknown schema, or covered schema # with no active concrete representative -> the check FAILS. From 72d94ad87f1e672b03c1e677bb91dd6ce6f1c398 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 25 Jul 2026 13:41:27 -0700 Subject: [PATCH 16/16] Fixture guard: preserve null-only constraint for type: ["null"] union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on #432: the array union form type: ["null"] subtracted "null" leaving an empty type set, which type_matches? treats as unconstrained — so a non-null value passed. Keep "null" as the type when the union has no non-null members (mirrors the scalar type: "null" fix), so a non-null value fails while null is accepted. Self-test adds both directions (17 synthetic cases). --- scripts/check-fixture-coverage.rb | 6 +++++- scripts/test-check-fixture-coverage.rb | 22 +++++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/scripts/check-fixture-coverage.rb b/scripts/check-fixture-coverage.rb index 3a8a2c9fe..a0ba6c59c 100755 --- a/scripts/check-fixture-coverage.rb +++ b/scripts/check-fixture-coverage.rb @@ -130,7 +130,11 @@ def allowed_types(schema) case t when Array members = t.compact - [Set.new(members - ["null"]), members.include?("null")] + non_null = members - ["null"] + # A union of only ["null"] still constrains the value to null — keep "null" + # as the type so a non-null value fails (an empty set would be unconstrained). + types = non_null.empty? ? Set.new(["null"]) : Set.new(non_null) + [types, members.include?("null")] when String # OpenAPI 3.1 scalar null type: the value must BE null — a real "null" type # constraint (so a non-null value fails), and it is nullable. Returning an diff --git a/scripts/test-check-fixture-coverage.rb b/scripts/test-check-fixture-coverage.rb index e8d1823bd..8e54ecf3f 100755 --- a/scripts/test-check-fixture-coverage.rb +++ b/scripts/test-check-fixture-coverage.rb @@ -443,10 +443,30 @@ def alt_null_manifest(schema, id) failures << "null-type validation hung" end +# OpenAPI 3.1 union `type: ["null"]` (array form with only null) must keep its +# null-only constraint too — a non-null value fails, null passes. +NULL_ARRAY_TYPE_OPENAPI = { + "components" => { "schemas" => { + "NullArrTarget" => { "type" => "object", "required" => ["n"], "properties" => { "n" => { "type" => ["null"] } } }, + } }, +}.freeze + +begin + m = { "targets" => [{ "id" => "na", "fixture" => "na.json", "pointer" => "", "schema" => "NullArrTarget" }], + "covered_schemas" => { "NullArrTarget" => ["na"] }, "excluded_schemas" => {} } + out, status = run_synthetic(openapi: NULL_ARRAY_TYPE_OPENAPI, manifest: m, fixtures: { "na.json" => { "n" => nil } }) + expect_pass(failures, 'type:["null"] required field accepts null', out, status) + + out, status = run_synthetic(openapi: NULL_ARRAY_TYPE_OPENAPI, manifest: m, fixtures: { "na.json" => { "n" => "x" } }) + expect_fail(failures, 'type:["null"] rejects a non-null value', out, status, "expected null, got string") +rescue Timeout::Error + failures << "null-array-type validation hung" +end + # --- Report -------------------------------------------------------------------- if failures.empty? - puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 15 synthetic cases" + puts "==> Fixture-coverage self-test passed — 1 positive + 8 negative + 17 synthetic cases" exit 0 else warn "Fixture-coverage self-test FAILED:"