Skip to content

Fixture-completeness guard: manifest + coverage invariant#432

Open
jeremy wants to merge 13 commits into
mainfrom
track-b-fixture-coverage-guard
Open

Fixture-completeness guard: manifest + coverage invariant#432
jeremy wants to merge 13 commits into
mainfrom
track-b-fixture-coverage-guard

Conversation

@jeremy

@jeremy jeremy commented Jul 25, 2026

Copy link
Copy Markdown
Member

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 into
make check. It reuses conformance/runner/ruby/schema-walker.rb
(find_response_schema + the recursive required/$ref/array walk) rather than
reimplementing validation.

It enforces two things a validate-only check cannot:

  1. Every manifest target validates. Entries are either an operation entry
    (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.json is wire-type
    "Message" but must validate as the generic Recording; search results
    validate as SearchResult; a /content_attachments/0 element is a
    RichTextAttachment.
  2. A coverage invariant. Each covered_schemas entry must keep ≥1 active
    target 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: [] reaches RichTextAttachment
    but 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.json
and forwards/reply_get.json were missing the required title,
inherits_status, and visible_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:

  • TS (5 helpers) — static JSON import, { ...fixture, id }.
  • Ruby (12 helpers) — load_fixture in test_helper.rb, fixture.merge(overrides).
  • Python (2 helpers) — inlined load_fixture, {**fixture, **overrides}.

Kotlin/Swift are strict decoders: a missing @required field already throws,
their builders already carry the @required *_attachments arrays, and the only
behaviorally-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.
  • Negative paths confirmed: drop a required field from a manifest'd fixture →
    fails with path+pointer; remove a covered schema's only concrete representative
    → coverage invariant fails; add a new @required field with no representative
    → fails.
  • make ts-check (746), make rb-check (727), make py-check (393) green; Go
    forwards 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

    • Added spec/fixtures/manifest.yaml and scripts/check-fixture-coverage.rb; run via make check-fixture-coverage, included in make check, and as a CI job (fixture-coverage) under LC_ALL=C with a kill‑safe self‑test hardened against a timeout‑after‑reap race.
    • Validator merges $ref (incl. 3.1 siblings) and allOf with conjunctive nullability, intersects allOf type constraints, requires ≥1 matching anyOf/oneOf branch 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.
    • Coverage invariant: every covered_schemas entry keeps a concrete representative; rich‑text emitter inventory is derived from openapi.json (traverses $ref with siblings and allOf/anyOf/oneOf, matches aliased/composed RichTextAttachment items). Every emitter must be covered or excluded. Completed Forward/ForwardReply fixtures with required title, inherits_status, and visible_to_clients.
  • Refactors

    • TS/Ruby/Python tests now load bodies from the shared fixtures; GET tests pin the id to the requested resource. Ruby/Python loaders and the schema walker read as UTF‑8. Vitest server.fs.allow is pinned to the repo root so JSON fixture imports resolve reliably.

Written for commit 170eb10. Summary will update on new commits.

Review in cubic

jeremy added 2 commits July 24, 2026 22:22
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.
Copilot AI review requested due to automatic review settings July 25, 2026 05:37
@github-actions github-actions Bot added documentation Improvements or additions to documentation typescript Pull requests that update TypeScript code ruby Pull requests that update the Ruby SDK spec Changes to the Smithy spec or OpenAPI labels Jul 25, 2026
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Spec Change Impact

  • Modified: Added fixture-completeness guard to manifest and enforced a coverage invariant.
  • No operations or resources were added or removed.
  • This is not a breaking API change as it does not remove operations or fields.
  • All SDKs require regeneration to incorporate the changes.

Checklist of SDKs needing updates:

  • Go
  • TypeScript
  • Ruby
  • Kotlin
  • Swift

@github-actions github-actions Bot added enhancement New feature or request and removed documentation Improvements or additions to documentation labels Jul 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 wire check-fixture-coverage into make 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.

Comment thread ruby/test/basecamp/services/schedules_service_test.rb Outdated
Comment thread ruby/test/basecamp/services/recordings_service_test.rb Outdated
Comment thread ruby/test/basecamp/services/messages_service_test.rb Outdated
Comment thread spec/fixtures/manifest.yaml Outdated
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).
Copilot AI review requested due to automatic review settings July 25, 2026 05:49
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 25, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread scripts/check-fixture-coverage.rb Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/check-fixture-coverage.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Comment thread scripts/check-fixture-coverage.rb Outdated
jeremy added a commit that referenced this pull request Jul 25, 2026
…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.
Copilot AI review requested due to automatic review settings July 25, 2026 06:05
@github-actions

Copy link
Copy Markdown

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

  • .github/workflows/test.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

@github-actions github-actions Bot added documentation Improvements or additions to documentation github-actions Pull requests that update GitHub Actions labels Jul 25, 2026
…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.
@github-actions github-actions Bot removed the documentation Improvements or additions to documentation label Jul 25, 2026
@jeremy
jeremy force-pushed the track-b-fixture-coverage-guard branch from 7b66dfd to 72614b7 Compare July 25, 2026 06:06
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 25, 2026
…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).
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 25, 2026 07:45

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/check-fixture-coverage.rb
Comment thread scripts/check-fixture-coverage.rb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.

Comment thread typescript/tests/services/todolists.test.ts
Comment thread typescript/tests/services/messages.test.ts
Comment thread typescript/tests/services/comments.test.ts
Comment thread typescript/tests/services/cards.test.ts
Comment thread typescript/tests/services/client-visibility.test.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread scripts/check-fixture-coverage.rb
…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.
Copilot AI review requested due to automatic review settings July 25, 2026 07:57
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 25, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/check-fixture-coverage.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

Comment thread scripts/test-check-fixture-coverage.rb
…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 AI review requested due to automatic review settings July 25, 2026 08:03
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 25, 2026
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.
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 25, 2026 08:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +114 to +115
when String
[Set.new([t]), schema["nullable"] == true]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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|

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conformance Conformance test suite enhancement New feature or request github-actions Pull requests that update GitHub Actions ruby Pull requests that update the Ruby SDK spec Changes to the Smithy spec or OpenAPI typescript Pull requests that update TypeScript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants