Fixture-completeness guard: manifest + coverage invariant#432
Conversation
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.
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.
Spec Change Impact
Checklist of SDKs needing updates:
|
There was a problem hiding this comment.
Pull request overview
Adds a fixture-completeness guard for shared JSON fixtures (spec/fixtures/) so newly-required fields can’t be silently omitted, then refactors TS/Ruby/Python tests to source stub bodies from those guard-validated fixtures (single source of truth). This fits the repo’s “spec → fixtures/tests as contract” workflow by adding an explicit coverage invariant on top of schema validation.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Add
spec/fixtures/manifest.yaml+scripts/check-fixture-coverage.rb(+ shell wrapper) and wirecheck-fixture-coverageintomake check. - Complete Forward/ForwardReply fixtures with newly-required fields and track schema coverage/exclusions in the manifest.
- Update TS/Ruby/Python tests/helpers to load response bodies from shared fixtures and merge overrides.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/services/todolists.test.ts | Switch sample stub helper to spread the shared Todolist fixture. |
| typescript/tests/services/messages.test.ts | Switch sample stub helper to spread the shared Message fixture. |
| typescript/tests/services/comments.test.ts | Switch sample stub helper to spread the shared Comment fixture. |
| typescript/tests/services/client-visibility.test.ts | Switch sample stub helper to spread the shared Recording fixture. |
| typescript/tests/services/cards.test.ts | Switch sample stub helper to spread the shared Card fixture. |
| spec/fixtures/manifest.yaml | New manifest defining validation targets + coverage invariant + exclusions. |
| spec/fixtures/forwards/reply_get.json | Add required ForwardReply fields (title, inherits_status, visible_to_clients). |
| spec/fixtures/forwards/get.json | Add required Forward fields (title, inherits_status, visible_to_clients). |
| scripts/check-fixture-coverage.sh | Add wrapper to run the Ruby coverage checker. |
| scripts/check-fixture-coverage.rb | Implement manifest validation + coverage invariant using the conformance schema walker. |
| ruby/test/test_helper.rb | Add load_fixture helper to read shared fixtures for tests. |
| ruby/test/basecamp/services/schedules_service_test.rb | Use shared schedule entry fixture for stubs and update expectations. |
| ruby/test/basecamp/services/recordings_service_test.rb | Use shared recording fixture for stubs and update expectations. |
| ruby/test/basecamp/services/messages_service_test.rb | Use shared message fixture for stubs and update expectations. |
| ruby/test/basecamp/services/forwards_service_test.rb | Use shared forward/reply fixtures for stubs and update expectations. |
| ruby/test/basecamp/services/documents_service_test.rb | Use shared document fixture for stubs and update expectations. |
| ruby/test/basecamp/services/comments_service_test.rb | Use shared comment fixture for stubs and update expectations. |
| ruby/test/basecamp/services/client_replies_service_test.rb | Use shared client reply fixture for stubs and update expectations. |
| ruby/test/basecamp/services/client_correspondences_service_test.rb | Use shared client correspondence fixture for stubs and update expectations. |
| ruby/test/basecamp/services/client_approvals_service_test.rb | Use shared client approval fixture for stubs and update expectations. |
| ruby/test/basecamp/services/checkins_service_test.rb | Use shared question-answer fixture for stubs and update expectations. |
| ruby/test/basecamp/services/cards_service_test.rb | Use shared card fixture for stubs and update expectations. |
| python/tests/services/test_uploads.py | Add fixture loader and build upload metadata from shared Upload fixture + overrides. |
| python/tests/services/test_todos.py | Add fixture loader and build Todo payload from shared Todo fixture + overrides. |
| Makefile | Add check-fixture-coverage target and include it in make check. |
| AGENTS.md | Clarify changed-path stub rule and document the new automated fixture backstop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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).
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6addd730d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…f-erasing inventory 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 () 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.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
…f-erasing inventory 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.
7b66dfd to
72614b7
Compare
…nch) 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).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23d5dadcf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…y-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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35fc26c0df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…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 <repoRoot>/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).
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170eb1015c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| types.merge(t2) | ||
| type_sets.concat(ts2) | ||
| forbids_null ||= !sub_nullable | ||
| items ||= i2 |
There was a problem hiding this comment.
Conjoin array item schemas from allOf branches
When an array schema is composed from multiple allOf branches that each declare items, items ||= i2 retains only the first item schema, so elements are never checked against later conjuncts. For example, allOf: [{type: array, items: {type: string}}, {type: array, items: {type: integer}}] incorrectly accepts an array of strings even though it violates the second branch. The fresh remaining case beyond the duplicate-property fix is to combine every inherited item schema under allOf (or validate each independently).
AGENTS.md reference: AGENTS.md:L317-L323
Useful? React with 👍 / 👎.
| when String | ||
| [Set.new([t]), schema["nullable"] == true] |
There was a problem hiding this comment.
Recognize a scalar null schema as nullable
For an OpenAPI 3.1 schema written as type: "null", this branch records "null" as an ordinary non-null type and returns nullable: false. Consequently, a required field declared directly as null—or an anyOf/oneOf nullable branch using {type: "null"}—rejects the only valid value. The fresh case not covered by the prior nullable-alternative fix is the scalar type: "null" form; treat it like the null member already removed from type arrays.
AGENTS.md reference: AGENTS.md:L317-L321
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-fixture-coverage.rb">
<violation number="1" location="scripts/check-fixture-coverage.rb:164">
P3: `merged_constraints`' documented return contract still lists six values, but it now returns `type_sets` as a seventh. Updating that contract prevents future callers from destructuring this internal API incorrectly.</violation>
<violation number="2" location="scripts/check-fixture-coverage.rb:178">
P2: The allOf merge now conjoins duplicate object properties, but array `items` constraints are still first-write-wins (`items ||= i2`). That means an array schema composed from multiple allOf branches can accept elements that satisfy only the first branch and ignore later item constraints. Consider conjoining inherited `items` schemas as well (for example via `allOf`) so array elements must satisfy every branch.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # 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| |
There was a problem hiding this comment.
P2: The allOf merge now conjoins duplicate object properties, but array items constraints are still first-write-wins (items ||= i2). That means an array schema composed from multiple allOf branches can accept elements that satisfy only the first branch and ignore later item constraints. Consider conjoining inherited items schemas as well (for example via allOf) so array elements must satisfy every branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-fixture-coverage.rb, line 178:
<comment>The allOf merge now conjoins duplicate object properties, but array `items` constraints are still first-write-wins (`items ||= i2`). That means an array schema composed from multiple allOf branches can accept elements that satisfy only the first branch and ignore later item constraints. Consider conjoining inherited `items` schemas as well (for example via `allOf`) so array elements must satisfy every branch.</comment>
<file context>
@@ -160,20 +160,31 @@ def type_matches?(types, value)
+ # 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
</file context>
| req = [] | ||
| props = {} | ||
| types = Set.new # union of all declared types (for messages + concrete_for?) | ||
| type_sets = [] # per-conjunctive-part declared type-sets — a value must |
There was a problem hiding this comment.
P3: merged_constraints' documented return contract still lists six values, but it now returns type_sets as a seventh. Updating that contract prevents future callers from destructuring this internal API incorrectly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-fixture-coverage.rb, line 164:
<comment>`merged_constraints`' documented return contract still lists six values, but it now returns `type_sets` as a seventh. Updating that contract prevents future callers from destructuring this internal API incorrectly.</comment>
<file context>
@@ -160,20 +160,31 @@ def type_matches?(types, value)
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).
</file context>
Track B of the #408 follow-up: a fixture-completeness guard so the shared
JSON fixtures under
spec/fixtures/can't silently miss a newly-required field —the exact gap #408 exposed (a resource validates fine without an additive array,
so per-resource validation never flagged the omission).
Core guard (
make check-fixture-coverage)spec/fixtures/manifest.yaml+scripts/check-fixture-coverage.rb, wired intomake check. It reusesconformance/runner/ruby/schema-walker.rb(
find_response_schema+ the recursive required/$ref/array walk) rather thanreimplementing validation.
It enforces two things a validate-only check cannot:
(validates the complete response body via
operationId) or a pointer entry(JSON pointer + explicit component schema). Pointer entries carry the schema
explicitly because discriminators lie:
recordings/get.jsonis wire-type"Message"but must validate as the generic Recording; search resultsvalidate as SearchResult; a
/content_attachments/0element is aRichTextAttachment.
covered_schemasentry must keep ≥1 activetarget resolving to a concrete, non-null instance whose declared root
schema is that component. Empty arrays and transitive reachability don't
count — a Todo with
description_attachments: []reachesRichTextAttachmentbut instantiates none. So a new required field on a covered schema is forced
into a fixture.
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).The guard immediately caught two genuinely incomplete fixtures —
forwards/get.jsonand
forwards/reply_get.jsonwere missing the requiredtitle,inherits_status, andvisible_to_clients— now completed.AGENTS.md rule (the actual #408 lesson)
Clarifies the changed-path stub rule verbatim: "every response stub for a changed
path must include any newly required or behaviorally changed field."
Contract 3 — helpers source from the fixtures
The named response-body helpers in the non-validating SDKs now derive their
body from the manifest'd fixture (one source of truth), keeping parameterized
overrides:
{ ...fixture, id }.load_fixtureintest_helper.rb,fixture.merge(overrides).load_fixture,{**fixture, **overrides}.Kotlin/Swift are strict decoders: a missing
@requiredfield already throws,their builders already carry the
@required*_attachmentsarrays, and the onlybehaviorally-changed optional fields (Recording's projection arrays) are already
asserted in
RecordingsServiceTest— so per Contract 3 they need no change.Verify
make check-fixture-coverage— 17 covered, 18 targets, 2 exclusions.fails with path+pointer; remove a covered schema's only concrete representative
→ coverage invariant fails; add a new
@requiredfield with no representative→ fails.
make ts-check(746),make rb-check(727),make py-check(393) green; Goforwards test green after the fixture completion.
Summary by cubic
Adds a fixture-completeness guard with a manifest and a coverage invariant that validates required fields, types, and nullability. Validation is composition‑aware ($ref with siblings, allOf), enforces anyOf/oneOf (at‑least‑one branch) with nullability, and intersects allOf type constraints so values must satisfy every branch.
New Features
spec/fixtures/manifest.yamlandscripts/check-fixture-coverage.rb; run viamake check-fixture-coverage, included inmake check, and as a CI job (fixture-coverage) underLC_ALL=Cwith a kill‑safe self‑test hardened against a timeout‑after‑reap race.$ref(incl. 3.1 siblings) andallOfwith conjunctive nullability, intersectsallOftype constraints, requires ≥1 matchinganyOf/oneOfbranch and folds their nullability. Enforces required/type/nullability (floats allowed for integers only when integral), flags null array elements, rejects non‑root pointers for operation entries, and enforces RFC 6901 (no leading‑zero indices, bad~escapes). Array‑typed schemas require non‑empty arrays for coverage.covered_schemasentry keeps a concrete representative; rich‑text emitter inventory is derived fromopenapi.json(traverses$refwith siblings andallOf/anyOf/oneOf, matches aliased/composedRichTextAttachmentitems). Every emitter must be covered or excluded. Completed Forward/ForwardReply fixtures with requiredtitle,inherits_status, andvisible_to_clients.Refactors
idto the requested resource. Ruby/Python loaders and the schema walker read as UTF‑8. Vitestserver.fs.allowis pinned to the repo root so JSON fixture imports resolve reliably.Written for commit 170eb10. Summary will update on new commits.