[etirelli]

What

Follow-ups to #15 (exclusion-only ContractSets), from the review of that PR.

Changes

• contracts: with no value no longer crashes coverage. YAML parses it as null. Coverage read spec.contracts before feat(coverage): allow ContractSets with no OpenAPI contracts #15's empty-list handling, in the yq version check and in baseline_overview, and died with a TypeError. As the schema requires, contracts: [] is the only exclusion-only shape: an omitted, blank, or non-list value is an integrity error (exit 2) that points at contracts: [].

• yq is verified even with no contracts. The pinned version lives on each contract's policy.yqVersion, so an exclusion-only descriptor pinned nothing and the binary went unchecked.

  • coverage now accepts --yq-version, which must agree with any contract's yqVersion.
  • The reusable coverage workflow passes the version it installed (yq-version input → YQ_VERSION → --yq-version).
  • With --yq-version, coverage verifies the binary before yq parses the descriptor, then checks the descriptor's pin agrees.
  • With no version from either source, coverage fails closed (exit 2, both modes), per docs/architecture.md. Every contract must pin policy.yqVersion, so this only applies to contracts: [].

• Warnings are kept. The classification step was overwriting report["warnings"]; it now appends to it.

• One shape validator for both commands. validate_descriptor() in modules/openapi-compat/checker.py runs inside load_descriptor(), so check and coverage reject the same malformed descriptors with the same exit-2 error before doing anything else, instead of crashing or silently accepting them. It follows the schema's types for:

  • spec, spec.contracts and each contract
  • each contract's policy, which must be a mapping with string oasdiffVersion and yqVersion, and its severityOverrides
  • the optional candidate, candidate.generator, spec.baseline, contract baseline, contract generator and exclusions blocks

  An optional block may be omitted, but if present it must have the schema's type. null and other falsy values ([], "", 0, false) are no longer treated as absent. Semantic rules (full Git SHAs, reasons, registered adapters, exclusion entries) stay in the commands, which give more specific errors. The per-field guards in both checkers are gone.

• Docs fix. An exclusion-only descriptor carries only the exclusions. The CRD globs remain the workflow's crd-globs input, or --crd-glob locally; they are not descriptor fields. cmd/contract-tests/README.md documents --yq-version.

• Onboarding skill and checklist. They describe the shape: contracts: [] plus exclusions, CRD and coverage jobs only, and confirming with the owner that no document is a servable API.

• Tests:

  • contracts: [] is complete; contracts: with no value, null, omitted, or a mapping is an error
  • a blank, null, list, or omitted spec (coverage and check)
  • falsy and non-mapping candidate, spec.baseline, policy, contract generator/baseline, and severityOverrides; missing or unquoted pins. Both commands, and check writes report.json for each
  • one valid pin no longer hides another contract's policy: null; candidate is validated even with contracts: []
  • contracts falling back to spec.candidate.generator (coverage and check)
  • --yq-version verified before the descriptor is parsed
  • modules/coverage/tests/test_schema.py: a table-driven parity test. Over 80 single-field mutations of a schema-valid descriptor assert that jsonschema and validate_descriptor() agree (jsonschema is test-only)
  • --yq-version verification and mismatch with the descriptor
  • no pinned yq fails closed in enforce and report-only
  • the workflow step passing YQ_VERSION
  • check refusing a descriptor with no contracts

Verification

All 424 Python tests in the CI directories pass, mypy is clean, and the action-pin check and actionlint pass.

Compatibility

No change for valid descriptors with contracts unless --yq-version is passed and disagrees with their yqVersion. Descriptors that are already invalid against the schema are affected:

• An exclusion-only descriptor that omits contracts passed coverage on main; it must now write contracts: [], and local runs of it must pass --yq-version.
• A descriptor whose contracts do not declare policy.yqVersion passed coverage on main without verifying yq. It now fails unless --yq-version is given. check already rejected it.
• A blank contracts: or spec:, a null or non-mapping policy (in check), or a non-mapping spec.baseline crashed with a traceback on main. Each is now a structured exit-2 error.
• A contract whose policy is missing, lacks oasdiffVersion, or has an unquoted version passed coverage on main (check already rejected it). It now fails both.
• candidate: or baseline: written with no value, or another falsy non-mapping value, is rejected. spec.baseline: null was accepted by both commands on main. candidate: null crashed.
• exclusions: null was an error on main and still is.

The workflow always passes the yq-version input it already required, and it installs that same version, so existing callers agree by construction.

🤖 Generated with Claude Code

[coderabbitai]

Important

• Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e7bc1234-14af-4080-b803-aee86da56bf0

[antowaddle]

@etirelli Thanks for these changes, added some comments.

[antowaddle]

descriptor_contracts() accepts omitted and null contracts as [] (modules/coverage/checker.py:515), and the new tests assert those descriptors are complete (modules/coverage/tests/test_coverage.py:359). But schemas/contract-set.schema.json:20 still requires contracts, and lines 27–30 type it as an array. The onboarding guide explicitly says descriptors are validated against that schema. Choose one direction: make contracts: [] the only valid exclusion-only shape and reject omitted/null values, or update the schema to make contracts optional/nullable and align the docs. An omitted key otherwise becomes a silent exclusion-only descriptor rather than a schema error. A schema-validation test would prevent this drifting again.

[etirelli]

Went with the schema: spec.contracts stays required, and contracts: [] is the only exclusion-only shape. descriptor_contracts() now rejects an omitted, blank, null, or non-list value with an integrity error (exit 2) that points at contracts: []. Added modules/coverage/tests/test_schema.py (jsonschema, test-only): it validates the selftest descriptor and the exclusion-only fixture, and asserts the omitted, null and mapping shapes fail. That mirrors the checker tests. Fixed in cb6a9cc.

[antowaddle]

A blank spec: still produces a traceback. load_exclusions() assumes spec is a mapping (modules/coverage/checker.py:133). With spec: containing no value, coverage exits 1 with AttributeError: 'NoneType' object has no attribute 'get' and no structured JSON report, instead of the documented integrity-error exit code 2. The new or {} in baseline_overview() does not help because classify_openapi() calls load_exclusions() first. Validate spec once and raise CheckError when it is not a mapping; the same pattern remains in generator_for() and pinned_yq_version(). This is pre-existing, but directly adjacent to the hardening goal.

[etirelli]

Fixed in cb6a9cc. load_descriptor() (shared by check and coverage) now requires spec to be a mapping. Coverage reads it through a new descriptor_spec() helper in load_exclusions, generator_for, baseline_overview and descriptor_contracts, so a blank spec: produces the structured JSON report with exit 2. A blank candidate: was a second crash in generator_for and in the openapi-compat check; both now treat it as absent. Tests cover a blank, null, list and omitted spec for both commands.

[antowaddle]

When neither the descriptor nor --yq-version supplies a version, coverage records a warning and exits 0 (modules/coverage/checker.py:558-563). The reusable workflow is safe because it always passes YQ_VERSION, but direct CLI or custom workflow callers can pass with an unverified parser. docs/architecture.md:39-41 says unpinned tools must fail closed. If warning-only behavior is intentional for local use, document that exception explicitly; otherwise make it an integrity error in enforce mode.

[etirelli]

Agreed. I made it fail closed instead of documenting an exception. Every contract must pin policy.yqVersion, so this only arises for contracts: [] without --yq-version. That case is now an integrity error (exit 2) in both enforce and report-only mode. The reusable workflow always passes YQ_VERSION, so CI callers are unaffected. The README and onboarding docs are updated. Fixed in cb6a9cc.

[antowaddle]

describes yq-version as “declared by the provider descriptor,” which is not true for exclusion-only descriptors. The generic local command in docs/onboarding/README.md:65 also omits --yq-version; add a note or flag for the exclusion-only case.

[etirelli]

Fixed in cb6a9cc. The input now reads "Exact mikefarah/yq version to install and verify. Must match every contract's policy.yqVersion; it is the only pin for a descriptor with no contracts." The local coverage commands in docs/onboarding/README.md (step 5) and openapi-compat.md now pass --yq-version, with a note that it is required for contracts: [].

[etirelli]

Update since the review replies (bfec70e):

A further self-review pass found and fixed some gaps:

• exclusions: null is rejected again, as on main and in the schema. The earlier commit had accepted it; leaving the key out is still fine.
• No more tracebacks for non-mapping spec.baseline or contract policy. Both now give a structured exit-2 error. policy: [] no longer shows up as "no yq pinned".
• With --yq-version, coverage verifies yq before parsing the descriptor, then checks that the descriptor's pin agrees. CI always passes it.
• Clearer unpinned-yq error. It asks for policy.yqVersion when the descriptor has contracts, and for --yq-version only when it has none.
• Tests: added for candidate (blank, non-mapping, and generator fallback; coverage and check), null and mapping exclusions in the checker and the schema, and verify-before-parse. The wording now says contracts: [] in the guide, the CLI help, and the schema.

The PR description's Compatibility section lists every descriptor shape that behaves differently from main. All of them are already invalid against the schema. 319 tests, mypy, action-pins, and actionlint pass locally.

[antowaddle]

@etirelli
The four earlier concerns are addressed in bfec70e: spec.contracts is required, blank spec is a structured error, unpinned yq fails closed, and the workflow/docs now pass --yq-version. CI is green.

Two issues remain:

• [P2] Policy validation is still split between commands. coverage/checker.py:549 skips policy: null. A two-contract probe with one valid pin and one policy: null returned exit 0, despite the schema rejecting it. Meanwhile openapi-compat/checker.py:437 still calls .get() on a null, list, or string policy; check exits 1 with AttributeError and writes no report.json. Validate contract policy once in shared descriptor code and test null/non-mapping policies through both commands.

• [P2] Falsy non-mapping candidates bypass validation. coverage/checker.py:151 and openapi-compat/checker.py:535 use ... or {}, so candidate: [], "", 0, and false are treated as absent. Both commands exit 0 while jsonschema rejects those descriptors. With contracts: [], even candidate: "bad" is never inspected and coverage exits 0. Check for None explicitly, then require a mapping.
  candidate: null and baseline: null also remain accepted at runtime but rejected by the schema. Either make those nullable in the schema or reject them consistently, and extend the parity tests beyond contracts/exclusions/spec.

[etirelli]

@antowaddle thanks. Both items are addressed in 4adec37 by validating the descriptor's shape once, instead of field by field.

• One validator for both commands. validate_descriptor() in modules/openapi-compat/checker.py runs inside load_descriptor(), so check and coverage reject the same malformed descriptors with the same exit-2 error before doing anything else. It follows the schema's types for spec, contracts and each contract, each contract's policy (a mapping with string oasdiffVersion/yqVersion) and its severityOverrides, and the optional candidate, candidate.generator, spec.baseline, contract baseline/generator and exclusions blocks. The per-field guards in both checkers are removed.
• Policy [P2]. Your probe (one valid pin plus policy: null) now exits 2. check produces a structured error with report.json for a null, list, string or missing policy, a missing pin, or an unquoted version.
• Falsy candidates [P2]. An optional block may be omitted, but if present it must have the schema's type. null, [], "", 0 and false are no longer treated as absent. candidate is validated even with contracts: []. I kept candidate and baseline non-nullable, as the schema has them, rather than loosening the schema.
• Parity tests. test_schema.py is now table-driven: 86 single-field mutations of a schema-valid descriptor (falsy and wrong-type values for each block, missing and unquoted pins), each asserting that jsonschema and validate_descriptor() agree. CLI tests cover the same shapes through both commands.

The validator covers types and structure only. SHA format, reasons, adapter names and exclusion entries stay with the commands, which give more specific errors. The PR description's Compatibility section lists the two shapes main accepted that are now rejected: spec.baseline: null, and a contract missing policy or oasdiffVersion (coverage only; check already rejected it). 424 tests, mypy, action-pins and actionlint pass locally.

[antowaddle]

@etirelli thanks for making those changes, some additional comments:

[P2] matchPath is not validated. matchPath: null still produces a raw TypeError in both commands with no JSON report; matchPath: 1 does the same. This contradicts the statement that both commands reject the same malformed descriptors before doing anything else. A schema-valid but syntactically invalid regex (matchPath: "[") is caught by coverage with exit 2, but check dies with PatternError and no report. Validate the type and regex once, or catch re.error in check.

[P2] flattenAllOf is not validated as a boolean. flattenAllOf: null and "false" pass both commands with exit 0. Null makes policy.get("flattenAllOf", True) return None, so check silently omits --flatten-allof; the schema rejects this descriptor. Add a boolean check and a parity-table case.

[P3] The “same malformed descriptors” guarantee is still broader than the validator. Contract id, type, and declaredArtifact are not shape-checked. For example, both commands accept an omitted or list-valued id with exit 0, although the schema requires a string. The table at test_schema.py:74 covers selected mappings only, so it gives the appearance of full parity without testing these fields.

[antowaddle]

/lgtm
We can followup on the additional review comments as discussed.

[etirelli]

@antowaddle thanks, all three are valid. I checked them against main, and they predate this PR: coverage behaves identically on main and on 4adec37 for matchPath: null/1/"[", flattenAllOf: null/"false", and an omitted or list-valued id. This PR does not change how check reads matchPath, flattenAllOf, or the contract id/type/declaredArtifact. You're right that the "same malformed descriptors" wording overstates what the validator covers today.

We'll fix these in a new PR:

• Validate the matchPath type and regex once, which also stops check crashing on re.error.
• Check that flattenAllOf is a boolean.
• Check the type of the contract id, type, declaredArtifact, and other required strings.
• Extend the parity table to every schema property, with a test that fails if a property has no case.