* Add API changelog tooling: semantic OpenAPI diff with severity ranking
Answering "has the API changelog been updated?" currently means reading a
10k-line JSON diff by hand, so it falls to whoever is asked in Slack. This adds
the mechanical part of that job as two mage tasks.
`./bin/mage openapi-diff OLD.json NEW.json [--severity breaking]`
resources/openapi/openapi.json is generated from the Malli endpoint schemas, so
the spec at any git ref is what the API was at that ref. The task parses two
specs into {METHOD /path -> shape} maps, inlines $refs (component schemas change
too), and compares params, request body, and response schemas.
Findings are classified BREAKING / ADDITIVE / DOC_ONLY and sorted breaking-first.
A change is breaking when it requires MORE from the caller or provides LESS to
the caller; anything else is just a change. That rule is directional and it
inverts between request and response, so severity is decided from both schemas
rather than from how a change happens to render:
request breaking: newly required, type/enum narrowed, schema closed, field removed
additive: new OPTIONAL field or param, type/enum widened, made nullable
response breaking: field removed, field may now be null
additive: new field returned
Adding an optional parameter is not breaking; returning extra data is not
breaking. A public diff of two Stripe specs found 663 of 679 changes additive,
which is why filtering matters more than summarizing.
`./bin/mage openapi-staleness`
The self-healing CI job runs only on PRs labelled `openapi-self-healing`, so the
committed spec drifts; it is currently ~38 API commits behind. A stale spec makes
the diff under-report, so this reports drift and names the commits rather than
letting a clean diff read as "no changes". The label gate is deliberate (#67854
introduced it to stage the rollout after #67294 removed the spec entirely over
merge conflicts), so it is left alone here.
Response coverage is partial: ~199 response entries declare a schema and can be
compared, ~1,700 are description-only 2XX/4XX/5XX stubs and cannot be.
Tests cover both directions of every request and response rule, $ref resolution
including the SCIM schemas whose property is literally named "$ref", a guard
against the failure mode in mozilla-ai/otari#1315 (breaking markers dropped at
render time), and a signal-to-noise case asserting additive bulk cannot drown
the breaking few.
* Generate specs from git refs, so the diff is not bound to the committed blob
`./bin/mage openapi-diff --refs <old-ref> <new-ref>` checks each ref out into a
throwaway worktree, runs that ref's own `generate-openapi-spec`, and diffs the
two results. Nothing is committed and no checkout is touched.
This removes the staleness problem rather than reporting it. The committed
`openapi.json` only updates on PRs labelled `openapi-self-healing`, so it lags
source badly: `--committed origin/release-x.63.x origin/master` reports 0
findings, because both refs carry the same stale blob. Generating from source
reports 670 -> 714 operations and 10 removed endpoints for the same pair.
It needs no support from the ref being diffed beyond `generate-openapi-spec`
already existing, so it works on historical refs. `--committed` keeps the fast
path and now prints a warning that its result under-reports, since an empty diff
from a stale spec is indistinguishable from an API that did not change.
Validated against the real v63 -> master delta. The top removals are the three
`GET .../execute` endpoints that the hand-written 0.64 changelog documents as
replaced by `POST .../execute/values`, and it independently surfaces a query
param rename (`pinned_state` -> `pinned-state`) that has no entry yet.
That run also shows a precision limit worth stating: it reports 239 breaking
findings, but 217 are the identical `additionalProperties: false` line from one
PR (#82447, closing mu/defn argument schemas). The classification is right --
closing a schema does require more of the caller -- but as changelog input that
is one entry, not 217. The skill now tells the drafter to collapse findings by
text and treat a repeated finding as a single systematic change.
* Group identical findings so a systematic change reads as one entry
`--grouped` collapses findings with identical text across endpoints and sorts by
blast radius, because the per-endpoint view is the wrong shape for drafting a
changelog.
On the real v63 -> master delta it turns 239 breaking findings into 113 distinct
changes. The largest spans 217 endpoints and is a single upstream PR (#82447,
closing mu/defn argument schemas): one changelog entry, reported once with its
endpoints, instead of 217 identical lines to scroll past.
Grouping is a separate render mode rather than a transformation of the default.
The per-endpoint view answers "what changed on this route?", the grouped view
answers "what distinct changes happened?", and collapsing the former into the
latter would lose the ability to check a specific endpoint.
The long tail is where the individually-interesting entries live: the same run
surfaces a `pinned_state` -> `pinned-state` query param rename that no changelog
entry covers yet.
* Fix review findings: two defects let breaking changes pass silently
Eric's review found 13 issues. The two that mattered both caused false
negatives - the tool reporting nothing where a caller would break.
The depth guard counted nesting levels, not `$ref` hops. `seen` already breaks
cycles, so the counter was doing a different job than its name claimed: past 12
levels both specs collapsed to the literal "<deep>" and compared equal, hiding
every change in 52 of 637 operations. It now advances only on a `$ref` hop, and
52 falls to 17.
`response-lines` compared only top-level properties and delegated anything deeper
to `(widening? new old)`. Swapping those arguments inverts value-set semantics -
which is why the nullable case read correctly - but not object-property
semantics, so a removed nested response field read as additive. It now recurses
the way `body-lines` does.
Chasing the second one surfaced a third of the same shape: `widening?` never
compared array `items`, so a narrowing inside an array was invisible. Found by a
test written for the SCIM `$ref`-named-property case.
Also fixed:
- `committed-spec!` built a shell command by interpolation, so a ref containing
`$`, a quote, or a space produced a wrong file. Values now ride as positional
arguments and the shell never parses them.
- The `--grouped` header counted severity-filtered-out changes, inflating the
count above the body printed beneath it.
- The removed and added endpoint lists were severity-gated, so `--severity
breaking` hid the added half of an endpoint move - at exactly the severity the
skill tells a drafter to start with. Both lists now print at every severity.
- The staleness check watched 66 of the 158 files that contain `defendpoint`,
missing `src/metabase/*/api/*.clj` and all of `enterprise/backend/src`, so it
could report `OK: spec is current` with dozens of newer endpoint files on disk.
It also compared `%cI` strings across a history carrying both `+08:00` and `Z`,
and listed commits from a narrower pathspec than the date came from.
- `DOC_ONLY` was unreachable: a description-only change produced no findings and
was dropped. Descriptions are what the skill reads to spot changes it cannot
explain, so a reworded docstring now surfaces as doc-only.
- A misspelled `--severity` fell through to nil and silently printed everything.
It now exits. `--committed` without `--refs` was accepted and ignored; it now
exits too.
- Each `--refs` run left two multi-megabyte specs in /tmp.
- `brief` truncated its non-map branch without the "..." its map branch adds.
- The first commit message claimed a test covered the SCIM `$ref`-named property.
No such test existed; one exists now, along with tests for the depth guard, the
nested response removal, and the doc-only path.
One correction to that commit message stands: `babashka.json` would have avoided
the `:discouraged-namespace` ratchet entirely, since `mage/doctor.clj` already
uses it unsuppressed. Keeping cheshire for consistency with the three other mage
namespaces that use it; the budget moves 68 -> 69 against current master.
The `de-slop` skill the review could not find does exist, at
~/.claude/skills/de-slop - it is a personal skill, not in the repo.
* Fix second review: four more directions where a finding was inverted or absent
All nine findings addressed. The reviewer supplied a failing test for each of the
first five; all five reproduced on the previous head, and all now pass.
Three of the four major findings were the same mistake in different places: a
schema shape that the output path handed to `widening?` with swapped arguments,
which inverts value-set semantics but not object-property semantics.
- A response array of objects went to the leaf branch, so a removed element field
read as additive and an added one read as breaking. 24 of the 199 comparable
response schemas in the committed spec are top-level arrays. Arrays now recurse
through `items` in the output direction.
- `schema-props` unwraps a nullable `oneOf`, so a nullable OBJECT reached the
object branch, where only keys and requiredness were compared and the type
change was invisible. The committed spec has 726 nullable object properties.
Both `body-lines` and `response-lines` now compare type sets there, with the
severity test pointing the correct way for each direction.
- `response-lines` discarded the required sets, so a response field the API may
now omit produced nothing - the same loss of guarantee as one that may now be
null, which was already breaking. 174 of 637 operations declare `required` in a
response schema.
The fourth major finding was a shortcut in `widening?`: any empty old schema was
treated as "no constraint", so a field going from Malli `:any` to a concrete type
read as additive. `operations` stored an absent body as `{}`, which made "no body"
and "any value" indistinguishable; an absent body is now nil and `{}` falls
through to the type and enum checks. After ref resolution the committed spec has
1,182 property schemas that are `{}`.
The depth limit is gone rather than re-tuned. `seen` already terminates every
path against a finite component set, so the limit only ever replaced deep nodes
with the same literal on both sides - which compares equal and hides the change.
The MBQL clause schemas form chains long enough to hit it: 17 operations carried
`<deep>`, and now none do while 18 terminate on genuine cycles via `<recursive>`.
A self-diff of the committed spec costs 2.9s instead of 1.4s and still reports 0
findings.
Also fixed:
- `format-epoch` shelled out to `date -r`, which only BSD and macOS read as epoch
seconds; on GNU coreutils it means "the mtime of this file" and printed nothing,
silently, because the call was `:quiet?`. Now `java.time`.
- `openapi-staleness` claimed the spec was "current" from endpoint files alone.
661 of 763 component schemas live outside `.api` namespaces, so a shared-schema
change moves the generated spec invisibly. The OK message now says what it
actually checked and points at `--refs`.
- A `--refs` run that could neither generate nor read the second ref leaked the
first spec: materialization happened in the `let` bindings, outside the `try`
whose `finally` removes the directory.
- The skill told an agent that two identical committed specs prove the API did not
change, and that the commit touching `openapi.json` is the PR that changed the
API. Both are wrong for the same reason - the committed spec only moves on
`openapi-self-healing` PRs. It now says to re-run without `--committed` before
reporting no changes, and to search endpoint source for attribution.
* Drop the ratchet bump: babashka.json instead of cheshire, and fix stale docs
The `:discouraged-namespace` increase was avoidable and the rationale I gave for
it was backwards. `babashka.json` is not in the discouraged list, and 6 mage
namespaces already use it unsuppressed against 5 on cheshire - so the
"consistency" argument pointed the other way. Swapping to it removes both the
68 -> 69 bump and the `:clj-kondo/ignore`, and this PR no longer touches
.clj-kondo/ratchets.edn at all. `read-str` keywordizes by default, so the two
spec reads pass `{:key-fn identity}`; this namespace is string-key throughout.
Verified unchanged behavior: self-diff still 0 findings, the real 0.64 change
still 1 breaking.
Three docstring counts were wrong. The `api-source-pathspecs` docstring claimed
238 `/api/ee/` paths (actual 106 - it looks like 106 paths plus 132 operations,
double-counted), 66 files matching `src/metabase/**/api.clj` (actual 65), and 158
files containing `defendpoint` (actual 160). The argument the docstring makes is
unaffected - the narrow pathspec still misses most endpoint files - but the
numbers are now the measured ones.
Also removed documentation that described code this branch already deleted:
- A comment in `resolve-refs` said "Only a $ref hop advances depth" while the
docstring 17 lines above correctly says there is no depth limit. There is no
depth counter left; the comment described the previous commit's version.
- Two test comments and a test name referenced a `<deep>` placeholder guard that
no longer exists, and cited a "52 of 637" measurement of the mechanism that was
removed. The tests themselves are valid - they assert deep and long-chain
changes are detected - so only the prose changed.
- SKILL.md quoted "`--committed` reports 0 findings" for v63 vs master as a fixed
figure. That was true when written and is not now: the blobs have since
diverged and it reports 1. The underlying point stands, so it is stated
qualitatively rather than as a number that drifts.
- SKILL.md said `--refs` "works on any ref". The code only attempts that and has
a documented fallback for refs that will not build with the current toolchain;
the text now says so.
* Fix third review: object unions and nullable arrays hid breaking changes
- A union of two or more object variants was compared by its first
variant only, so removing or editing any other variant produced no
finding. Such unions now compare whole: widening only when every old
variant survives unchanged.
- Nullable arrays (`oneOf [array, null]`) skipped the array branch, so a
change to their items was ranked additive. Both array branches now
unwrap the null first.
- The render test never rendered. It now asserts breaking operations
print under the breaking headings of both renderers, and a new test
covers the --severity filter.
* Rank changes to unmodeled schema keywords breaking, and compare bounds, allOf, and tuples
The old widening check returned "safe" for any keyword it did not
recognize, so each review round found another way for a breaking change
to read as additive: allOf, minimum/minLength, map-of value schemas,
prefixItems, nullable enums, closed nullable objects.
`compatible?` replaces it. It takes a direction (:request or :response),
compares unions variant by variant, compares bounds, pattern, and format,
and compares allOf and prefixItems by position. A change to any keyword
it does not model ranks breaking. A false breaking finding costs one
look; a missed one ships undocumented.
Documentation keys (description, title, examples, default) are stripped
before comparison, so a reworded field description is doc-only instead
of an additive schema change.
Also:
- When two summaries are identical, the finding names where the schemas
differ (e.g. `items.enum: +"measure"`), so distinct changes no longer
merge in the grouped view.
- Generation deletes the checked-out spec before running, so a generator
that logs and swallows an error no longer passes the committed spec off
as fresh. A timeout falls back to the committed spec instead of
throwing.
- An empty or non-spec input file exits 1 instead of reporting 0 BREAKING.
- openapi-staleness lists commits after the spec commit, not including it.
* Close the gaps an independent check found in the compatible? rewrite
- Only a nullable union unwraps to its object. `string | object` and
similar unions are compared whole, so a change to the non-object
variant is no longer hidden.
- Keywords beside an object's properties (allOf, a map-of
additionalProperties, minProperties) are compared.
- If the field walk finds nothing but the stripped schemas differ, the
whole schema is ranked instead of falling through to doc-only.
- Inclusive and exclusive numeric bounds compare as one bound.
- A response allOf may gain members (narrowing output); a request allOf
may lose them.
- An enum or const without a type widening to its type is additive.
- requestBody.required and request content types are compared, and a
response that declares a schema for the first time is reported as
additive.
- A missing input file says so.
* Match operations across path-variable renames; count only visible severities in the grouped header
- Operations and path params are keyed by position, not variable name.
Renaming `{id}` to `{card-id}` leaves the URL a caller sends unchanged,
so it is now a doc-only "path variable renamed" finding rather than a
removed endpoint plus an added one. Routes that would merge under
position-keying (`/api/database/{id}/schemas` and
`/api/database/{virtual-db}/schemas`) keep their names. Reports still
print the real spec paths.
- At --severity breaking, the grouped header no longer counts the
new-endpoint list, which is additive. The list still prints.
- The skill's attribution section no longer opens with the
openapi.json git log that it then says names the wrong author.
* Rank a changed or dropped request default as breaking
`default` was stripped with the documentation keys, so a default that
flipped (say a query param from false to true) ranked doc-only and
--severity breaking hid it. The schema still validates the same
requests, but a caller who omits the field now gets different
behavior.
Request defaults are now compared: changed or dropped is breaking,
added where there was none is additive. Response defaults are ignored.
* State one definition of breaking that covers every rule
A change is breaking when an existing caller, sending exactly what it
sent before, can now fail or get a different result. The old wording
(requires more / provides less) did not cover changed defaults, and it
presented two conservative rules - removed request fields and changes
to unmodeled keywords - as if they followed from it. Those are now
stated as what the tool does when it cannot tell.
* Skill: point to docs-write, and attribute with all six API source pathspecs
de-slop is a personal skill the repo does not ship; docs-write carries
the Metabase style guide. The attribution git log used three of the six
pathspecs in api-source-pathspecs and missed enterprise api/ dirs and
routes.clj files (35 tracked files, e.g. sso/api/saml.clj).
6cb8e9e904