ci: verify the client's wire shapes against the committed openapi.json - #3
Closed
MattJackson wants to merge 3 commits into
Closed
ci: verify the client's wire shapes against the committed openapi.json#3MattJackson wants to merge 3 commits into
MattJackson wants to merge 3 commits into
Conversation
busbar-admin is a hand-rolled mirror of another repo's wire shapes, and nothing
checked that mirror. The spec-drift job compares `jq -r .info.version
openapi.json` against the latest busbar release tag: a VERSION STRING. It says
nothing about whether src/client.rs still matches the schemas inside the very
document sitting in the same commit, and it degrades to a warning when the
GitHub API is unreachable. A struct could drift arbitrarily and every gate
stayed green.
tests/openapi_conformance.rs closes that. It include_str!s the real committed
openapi.json (not a copy) and, for every request/response type the client
carries, asserts:
- the Rust WIRE key set (taken from a real serialize) equals the schema's
`properties`. A Rust field with no property fails; a property with no Rust
field fails unless listed in that case's `unmodelled` allowlist, which is
itself checked for staleness in both directions.
- optionality, proven behaviourally: a required non-nullable property must
REJECT an explicit null (so it cannot be an Option), and a nullable one must
ACCEPT it (so it must be).
- round trip: a schema-shaped value deserializes, re-serializes, and every
modelled property keeps its value.
- endpoint wiring: every path the client calls exists with that method and its
2xx response still points at the schema the client decodes into.
- the error-code vocabulary, the only closed enum on any shape this client
touches (the client models it as a String, so the enum set is pinned here).
- the untyped POST /config/apply body, which has no Rust type at all: its
required `config` key and its refusal of unknown top-level keys are pinned,
since nothing else in the crate would notice a change.
Adding a property to the schema, renaming a Rust field, or flipping a required
field to Option each fail this suite; all three were proven red before green.
Three drift findings the mapping surfaced, recorded as `unmodelled` entries with
reasons rather than silently tolerated: PluginView omits target,
interface_version, schema_url and schema_error; PluginInstallView omits
interface_version; HookView omits settings_keys.
Supporting changes:
- add a lib target (src/lib.rs) so tests/ can import the real types. A
[[bin]]-only crate cannot be imported by an integration test, which is why
these shapes could previously only be tested from an inline #[cfg(test)]
module.
- move the two inline #[cfg(test)] modules into tests/wire_shapes.rs and
tests/cli_args.rs, per the fleet convention that tests live in their own
file. The pure CLI mappings they cover move to src/argmap.rs so the tests
call the same functions main.rs does.
- publish ErrorEnvelope/ErrorDetail so the error envelope is checked like
every other shape.
- README: correct the spec version to 1.5.3 (matches openapi.json) and
describe the new gate.
The spec-drift job compares `jq -r .info.version openapi.json` against the latest busbar release tag. A mirror is a copy, so it carries core's version string along with everything else: a stale copy reports the version of the spec it was copied from, not the version of the spec it still matches. The check is therefore green precisely when it is least informative. Proven, not asserted. Core's `ci/close-structural-gaps` adds `phase`, `fires_at` and `groups` to HookView without changing info.version, since 1.5.3 was already the version. Against that spec the committed openapi.json is missing three properties and three required entries, and spec-drift stays green throughout, because both documents say 1.5.3. So add a gate that compares the DOCUMENT. It walks components.schemas schema-by-schema and property-by-property and paths path-by-path and method-by-method, and every finding names the schema and the property: which schemas are missing, which the mirror has and core does not, per-schema property and `required` differences, enum variant differences, type differences, and the same for paths, methods, parameters, request bodies and responses. A residual sweep then reports anything the named walk did not cover, by JSON pointer, so the gate cannot be green about a part of the document it never compared. That is the whole defect, and it would be absurd to reintroduce it inside its own fix. It FAILS CLOSED. spec-drift prints ::warning:: and exits 0 when the GitHub API is unreachable, so a rate-limited runner reports "no drift" without having looked. This exits 2 instead. An unknown is not a pass. It compares against `latest-release`, not a branch, because this client is built and integration-tested against the latest RELEASED busbar; committing a branch's spec would document fields no released engine serves. `--selftest` proves each finding class red against synthetic fixtures, plus the two fail-closed paths and the exact false-green above: a mirror carrying core's own info.version with three properties removed must still go red. CI runs the self-test BEFORE it trusts the verdict. This is an additional axis, not a replacement. tests/openapi_conformance.rs still checks src/client.rs against this repo's own committed spec, which is the complementary question and stays exactly as it was.
spec_mirror_selftest.py had no __main__ guard. `python3 spec_mirror_selftest.py` therefore defined run() and exited 0 having asserted NOTHING. That is the exact shape of failure this gate exists to remove: a green that proves nothing ran, from a file whose name invites being run directly. CI was never affected, because it invokes the canonical entry point `spec_mirror_gate.py --selftest`. The risk was a human checking the gate by hand, getting a clean exit 0, and concluding the self-test passed. Proven both ways: with a finding class disabled in the gate, the direct invocation now exits 1 and prints "SELF-TEST FAILED: 1 of 19 checks did not hold"; restored, it exits 0.
Contributor
Author
|
Landed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
busbar-admin is a hand-rolled mirror of another repo's wire shapes, and nothing checked the mirror.
The
spec-driftCI job comparesjq -r .info.version openapi.jsonagainst the latest busbar release tag. That is a version string. It says nothing about whethersrc/client.rsstill matches the schemas inside the very document sitting in the same commit, and it degrades to a warning when the GitHub API is unreachable. A struct could drift arbitrarily from the committed spec and every gate stayed green.The gate
tests/openapi_conformance.rsinclude_str!s the real committedopenapi.json(not a copy) and, for every request/response type insrc/client.rs, asserts:properties. A Rust field with no property fails. A property with no Rust field fails unless it is listed in that case'sunmodelledallowlist, and that allowlist is itself checked for staleness in both directions (an entry that is no longer a property, or that the Rust type now carries, fails).null, which is only true if the Rust field is not anOption. A nullable property must accept it, which is only true if it is. The two directions together pin every field.pathswith that method, and its 2xx response still points at the schema the client's method decodes into.codeenum is the only closed enum on any shape this client touches; the client models it as aString, so the set is pinned here and every member is decoded.POST /config/applyhas no Rust type (the CLI passes the file through as aserde_json::Value), so its requiredconfigkey and its refusal of unknown top-level keys are pinned directly against the spec. Nothing else in the crate would notice a change.Red before green
Each check class was proven to be a real gate, then restored.
1. Rename a Rust field (
PluginInstallView.publisher->publisher_name):2. Flip a schema-required field to
Option(HookView.on_error: String->Option<String>):3. Add a property to the schema (
RevokeView.revocation_reasonadded toopenapi.json):Drift the mapping surfaced
Every type mapped cleanly to a
components.schemasentry. Three types read fewer properties than the schema carries; each is now recorded as anunmodelledentry with a reason rather than silently tolerated, so it is a deliberate, reviewed omission:PluginViewPluginViewtarget,interface_version,schema_url,schema_errorPluginInstallViewPluginInstallViewinterface_versionHookViewHookViewsettings_keysGET /config(EffectiveConfigView) and thePOST /config/applyrequest body have no Rust type by design; both are still checked at the endpoint/body level.Supporting changes
src/lib.rs). A[[bin]]-only crate cannot be imported by an integration test, which is why these shapes could previously only be tested from an inline#[cfg(test)]module.#[cfg(test)]modules intotests/wire_shapes.rsandtests/cli_args.rs, per the convention that tests live in their own file. The pure CLI mappings they cover move tosrc/argmap.rsso the tests call the same functionsmain.rsdoes.ErrorEnvelope/ErrorDetailso the error envelope is checked like every other shape.openapi.json) and describe the new gate..github/workflows/ci.ymlalready rancargo test --locked --verbose, so the suite is gated on every push and PR; the job comments now say which check answers which question.Local run:
cargo fmt --check,cargo clippy --all-targets -- -D warnings, andcargo test --locked(36 tests) all pass.