ci: add the oasdiff breaking-change gate - #201
Conversation
|
/dobby please review — this is your api-diff.yml from the #200 comment, split onto a stacked PR after the gate flagged #200's own spec corrections (spec-to-spec diff can't see that service behavior was unchanged). Base is the audit branch, so it retargets to main when #200 merges and arms against a truthful spec. |
(cherry picked from commit b2a0cda)
b2a0cda to
0f5fc7c
Compare
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approving. The gate is correct, minimally scoped, and now genuinely armed against a truthful base. Three non-blocking notes inline (1 style, 2 nits) — none need to hold up the merge.
State I verified at HEAD 0f5fc7c (the branch was force-pushed at 14:30 after my first read, so I re-reviewed against the real head — content is byte-identical to b2a0cda, it was a rebase onto main):
#200merged at 14:28:57 and GitHub'sautomatic_base_change_succeededfired at 14:29:00, so this PR now targets main @ef51da2. The ordering plan in the description worked as written.api-description.yamlis byte-identical at base and head (blobfff6004), so the gate's own PR is a clean no-change case rather than an accidental self-trip.oasdiff breaking changespasses in 13s in run30275393634, which started at 14:30:20 — after the retarget. So the reported green really was computed against main, not against the deleted audit branch. All other checks pass; nothing in flight.- Two-dot diff against the current base is just this one new file, so it is not a no-op stacked PR.
On the security-relevant settings, this matches what we require of oasdiff-action: review: false (nothing uploaded to oasdiff.com), allow-external-refs left at its false default, permissions: contents: read, and the tag pins the engine — v0.1.10's Dockerfile is FROM tufin/oasdiff:v1.26.1, which is what lets a local run reproduce CI's verdict.
One earlier finding I dropped as stale: a note that retargeting this PR to main by hand before #200 landed would make the gate fail on its own PR with those four ERRs. That risk has passed — #200 merged first, GitHub retargeted automatically, and the resulting run is green.
| ref: ${{ github.event.pull_request.base.sha }} | ||
| path: base | ||
| - name: Diff the spec against the base branch | ||
| uses: oasdiff/oasdiff-action/breaking@v0.1.10 |
There was a problem hiding this comment.
style — third-party Docker action pinned to a mutable tag.
oasdiff/oasdiff-action/breaking@v0.1.10 is a runs: using: docker action (confirmed at that tag), so CI builds and runs code from that repo (62 stars) with the workspace mounted. I resolved the tag: v0.1.10 → 0ab8ad204b00d25acc5ae87106281433e288d0c1, a commit on the default branch — a tag that can be repointed.
Every other action here is first-party or well-known (actions/*, docker/*, dtolnay, Swatinem, release-plz), so this is the first genuinely small third-party action in the repo. Suggestion:
uses: oasdiff/oasdiff-action/breaking@0ab8ad204b00d25acc5ae87106281433e288d0c1 # v0.1.10Blast radius is already small (permissions: contents: read, review: false, no secrets on a pull_request trigger), so this is hardening rather than a hole — hence style, not blocking. Note the SHA pin also makes the engine pin exact, since the tag is what currently guarantees tufin/oasdiff:v1.26.1.
| # oasdiff rates ERR. Additive changes pass. | ||
| # | ||
| # To land a genuinely breaking change: add a new versioned route, leave the | ||
| # old one in place, and deprecate it once privacybydesign/postguard-ops#64 |
There was a problem hiding this comment.
nit — dead cross-reference.
privacybydesign/postguard-ops#64 does not resolve: the repo 404s on the API and is absent from a full repo list of both orgs. The string appears nowhere else in cryptify.
This is the one pointer a future maintainer follows to answer "when may I drop the old route", so a 404 there costs real time. #196 and encryption4all/postguard#247 both resolve if you want an existing tracker, or create the ops issue and update the line.
| # | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
nit — a bare pull_request: will not re-run this gate when a PR is retargeted.
The default activity types are opened, synchronize, reopened. Changing a PR's base branch fires edited, which is not in that set. Since the verdict is derived from github.event.pull_request.base.sha (line 33), a retargeted PR keeps the check result computed against its old base.
This PR is the live example. automatic_base_change_succeeded fired at 14:29:00 when #200 merged, and it triggered no API-diff run — the gate only re-evaluated against main because of the unrelated force-push at 14:30:14 twenty seconds later. Without that push, the green you would be merging on was computed against the deleted audit branch.
The substance happens to be fine here (the spec blob is identical at both bases), and the stacked-PR flow the description recommends is exactly the flow that hits this. It matters more once this becomes a required check, because a stale green is still a passing required check. Fix is one line:
on:
pull_request:
types: [opened, synchronize, reopened, edited]edited also fires on title/body edits, so this costs a few extra 13-second runs. Worth it for a gate whose answer depends on the base.
|
Approved #201 — the gate is correct and minimally scoped. Two things worth knowing before the details: the branch got force-pushed while I was reviewing ( Left 3 non-blocking notes inline:
Nothing here should hold up the merge. |
#203) * ci: pin the oasdiff gate's settings with a mutation test `fail-on: ERR` with no `include-checks`, the setting the gate landed with in #201, fails open on ten of the changes the spec's contract forbids. Measured against oasdiff v1.26.1 by mutating the real spec one way per rule: a `401` that becomes a `403` and a dropped response enum value rate ERR but are opt-in, and a removed or renamed optional response property, a removed request parameter, a removed request property and the constraint-narrowing `*-set` family all rate WARN. Cryptify's routes are unversioned, so each of those reaches every pinned client the moment it deploys. Adds `mod api_gate_tests` in `src/main.rs`, which runs the engine with the flags the action's entrypoint builds and asserts stop-or-pass per mutation. It fails on the shipped settings and names every gap; it passes on `fail-on: WARN` plus the two opt-in checks. The engine is not on any runner, so it skips in CI while `every_api_gate_mutation_still_applies` still runs there. The workflow itself is in a PR comment: the App has no `workflows: write`. Refs #202. Part of encryption4all/postguard#247 (workstream C). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: assert the workflow runs the settings the gate test pins Review on #203: `FAIL_ON` and `INCLUDE_CHECKS` described settings nothing tied to the committed workflow, which still says `fail-on: ERR` with no `include-checks`, and `cargo test` was green anyway. So the branch shipped a passing test certifying a configuration the gate does not use, which is the fail-open the module exists to catch. `the_workflow_uses_the_settings_this_module_pins` reads `.github/workflows/api-diff.yml` and asserts its two step inputs are the two constants. It needs no `oasdiff`, so it runs on every runner. It is red until a maintainer applies the workflow patch from the PR comment, which makes the merge order enforced rather than documented. Also from the review: CLAUDE.md now says the settings are pending rather than present-tense fact and names where the patch lives; the tier counts are the `-f json` ones (WARN adds 30 on top of ERR's 212, not 31 on 213 — the table output has a header row and a trailing blank); the versioned-route mutation goes through `once` like every other anchor, so a renamed `getUsage` fails loudly instead of no-op'ing; and a root `.gitattributes` marks the spec `-text` so the multi-line anchors survive a Windows checkout. Refs #202. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: assert the api gate still runs, not only what it runs with `the_workflow_uses_the_settings_this_module_pins` pinned the gate's two inputs but not whether the job is ever triggered, which is the one fail-open it could not see: settings that read correctly on a gate that never fires. The workflow's own header comment names the foreseeable edit ("There is deliberately no `on: paths:` filter"), and a later `paths:` would skip the gate on every PR that does not touch the spec with the test still green. Asserts a `pull_request` trigger, no `paths:`/`paths-ignore:` filter, and no `if:` on the job. The trigger check is line-based over the `on:` block rather than a byte-exact match, so a rewrite between the mapping, list and scalar forms of `on:` does not go red for nothing. * ci: tune the oasdiff gate to WARN with the two opt-in ERR checks Dobby's YAML from the #203 comment, applied verbatim (the App cannot push workflows). This is the half its guard test was failing on: the_workflow_uses_the_settings_this_module_pins reads this file and asserts fail-on and include-checks match the constants the test module pins, so the settings and the mutation test that certifies them cannot land apart. At fail-on=ERR with no include-checks the gate passed ten of the changes this spec's contract forbids. --------- Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ruben Hensen <ruben.hensen@protonmail.com>
1 participant
The gate from #196, split out of #200 after it (correctly) flagged #200's own spec corrections as breaking: oasdiff compares spec-to-spec, so fixing the spec to match the service's real behavior — the always-required
cryptifytokenheader, plain-text 400s, the actual download media type — reads as four ERR findings even though the wire contract is unchanged.Ordering fix: the spec correction (#200) merges first; this PR is stacked on it (GitHub retargets to main automatically when #200 lands), so the gate arms against a truthful base and its first verdict on a real PR is meaningful.
Workflow YAML is dobby's, unchanged from the #200 comment:
review: false(nothing uploaded to oasdiff.com), no path filter (so it can become a required check), base pinned topull_request.base.sha,fail-on: ERR.Part of encryption4all/postguard#247 (workstream C). Refs #196.