ci: tune the oasdiff gate's severity settings and pin them with a test

[dobby-coder]

The mutation table for cryptify's spec, and the settings it implies. Same measurement as encryption4all/postguard#269, redone here rather than assumed: the answer is the same on the settings, and different on the one WARN check that fires on a permitted change.

Closes #202. Part of encryption4all/postguard#247 (workstream C).

The workflow file is in a comment below, not in the diff: the App has no workflows: write on this repo, so .github/workflows/api-diff.yml needs a maintainer to apply it. What is committed here is mod api_gate_tests in src/main.rs, which pins what the gate's settings actually deliver, plus the CLAUDE.md section that records the measurement.

This PR is red on purpose, and applying that comment onto this branch is what turns it green. the_workflow_uses_the_settings_this_module_pins reads the committed workflow and asserts its fail-on and include-checks are the constants the module pins. Right now they are not, so cargo test fails with

.github/workflows/api-diff.yml and this module disagree about what the gate does:
  fail-on: the gate runs with ERR, this module pins WARN
  include-checks: the gate runs with nothing, this module pins response-non-success-status-removed,response-property-enum-value-removed

which is the point: without it the branch would merge green while certifying settings the gate does not use, and the gate would go on passing the ten forbidden mutations below. The maintainer's commit and the settings land together or neither does.

The mutation table

Measured against oasdiff v1.26.1 (the engine [email protected] pins) on the real api-description.yaml, with the flags the action's entrypoint builds. ERR+inc is fail-on: ERR with the two opt-in checks named; WARN+inc is what this PR moves to.

Forbidden by the contract, so the gate should stop all of these:

mutation	ERR (shipped)	ERR+inc	WARN+inc
route removed	fails	fails	fails
request property becomes required	fails	fails	fails
required response property removed	fails	fails	fails
response property type narrowed	fails	fails	fails
response media type removed	fails	fails	fails
required response header removed	fails	fails	fails
401 becomes 403	passes	fails	fails
non-success response removed	passes	fails	fails
response enum value dropped	passes	fails	fails
optional response property removed	passes	passes	fails
optional response property renamed	passes	passes	fails
required request parameter removed	passes	passes	fails
optional request parameter removed	passes	passes	fails
request property removed	passes	passes	fails
request parameter constraint narrowed (maxLength)	passes	passes	fails

Additive, so the gate must let them through:

mutation	ERR (shipped)	ERR+inc	WARN+inc
new endpoint	passes	passes	passes
new optional response property	passes	passes	passes
new optional request property	passes	passes	passes
new required response property	passes	passes	passes
new optional query parameter	passes	passes	passes
new response status	passes	passes	passes
new optional response header	passes	passes	passes
new request enum value	passes	passes	passes
edited description	passes	passes	passes
versioned route beside the unversioned one	passes	passes	passes
new response enum value	passes	passes	fails

What the table says

Nine of the fifteen forbidden changes pass at the shipped setting. Three of those are the opt-in checks; the other six are WARN-rated. So fail-on: ERR with no include-checks leaves the request side of "no removing or narrowing" completely unguarded, and the optional half of the response side too.

The lower exposure this repo was expected to have is real, but partial. #202 noted 34 required: occurrences here against pg-pkg's one, and that does buy something: a removed required response property is an ERR here, where in pg-pkg it was a WARN that slipped through. But UploadStatus.prev_token and prev_offset are optional, and they are exactly what a resuming client reads on the recovery path. Request parameters and request properties have no required:-driven escape at all: request-parameter-removed and request-property-removed are WARN whatever the spec looks like. The *-set family behaves the same way, confirmed here with a maxLength: 64 added to /usage's email query parameter.

And unversioned routes make each of these land harder than in pg-pkg. There is no /v3 to redirect a breaking change to, so the gate is the only thing between a spec edit and every pinned client.

The one WARN check that fires on a permitted change

Same check as pg-pkg: response-property-enum-value-added. Ten additive mutations ran through WARN+inc and only that one flagged. On this spec it hits UploadSessionNotFound.reason and PayloadTooLarge.limit.

The pg-pkg answer does not transfer cleanly, so here is what I found before landing on the same decision.

Nothing in this org actually switches on those codes today. pg-js reads the field as parsed.reason ?? 'unknown' (packages/pg-js/src/api/cryptify.ts) and hands it to UploadSessionExpiredError as a plain string; postguard-tb-addon passes it straight through to one UI message. pg-pkg's SessionStatus drives client control flow, so "a new value breaks an exhaustive switch" was concrete there. Here it is hypothetical. On the face of it that argues for suppressing the check.

Two measurements argue the other way:

• --warn-ignore cannot express "ignore this check". It takes a file, and matching is by asking whether an ignore line contains the rendered change text, so an entry has to spell out the full lowercase string per affected operation and name the specific new value. response-property-enum-value-added as an ignore line suppresses nothing. Neither does .*. Verified on v1.26.1. There is no standing setting to add.
• x-extensible-enum, which oasdiff's message suggests, makes it skip the property entirely. Adding a value passes, and so does removing one. That is response-property-enum-value-removed, which is one of the two checks this PR turns on. It trades a false positive for a genuine gap.

So there is no cheap suppression, and the check costs one red run on the day someone adds an error code. I kept it, and CLAUDE.md now says a new response enum value is a breaking change here and why. It is also the only rule the gate has that WARN alone enforces, so it is the first thing a revert to ERR drops, silently; mod api_gate_tests pins it so it cannot go quietly.

If you would rather have the check gone, the honest version is the three ignore lines at the time you need them, not a setting. Happy to switch if you disagree.

The regression test

mod api_gate_tests in src/main.rs, inline per this repo's convention (no library target, so no tests/ dir). It mutates the real spec one way per rule, shells out to oasdiff with the flags the action's entrypoint builds, and asserts stop-or-pass. Its verdict is CI's verdict.

On the shipped settings it fails and names every gap:

the gate's verdict on 10 of 27 changes is not what fail-on=ERR and include-checks= are supposed to deliver:
  a changed status code: the gate should stop it, it passed it
  a removed non-success status: the gate should stop it, it passed it
  a removed response enum value: the gate should stop it, it passed it
  a new response enum value: the gate should stop it, it passed it
  a removed optional response property: the gate should stop it, it passed it
  a renamed optional response property: the gate should stop it, it passed it
  a narrowed request parameter constraint: the gate should stop it, it passed it
  a removed required request parameter: the gate should stop it, it passed it
  a removed optional request parameter: the gate should stop it, it passed it
  a removed request property: the gate should stop it, it passed it

On the new ones it passes. It rides the existing Rust quality job rather than a new one, since that runs cargo test --all-targets. It needs the oasdiff binary, which no runner has, so it skips there with a message saying how to install it. The module's other two tests need nothing but the repo tree and so do run on the runner: every_api_gate_mutation_still_applies fails if a spec edit strands one of the anchors, and the_workflow_uses_the_settings_this_module_pins fails if the workflow and the constants disagree. Locally, with go install github.com/oasdiff/[email protected], all three do the real work.

Kept from the current workflow, deliberately

review: false, the unpinned-but-tagged @v0.1.10, the plain on: pull_request trigger, and both checkouts as they are. postguard#269's copy differs on three of those (sha-pinned action, types: [..., edited] for base retargets, persist-credentials: false), and each is defensible, but none of them is this issue. The edited trigger one is worth its own issue: it has already bitten this repo once, on #201 itself. Say the word and I will file it.

Verified locally

cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings are clean. cargo test --all-targets is 156 passed and 1 failed, the one being the workflow-coupling test described above; with the comment's YAML copied over .github/workflows/api-diff.yml in the working tree it is 157 passed, and the tree was restored afterwards. Both runs were done with and without oasdiff on PATH, so the skip path is exercised too. Its three other failure modes were each provoked rather than reasoned about: a duplicated fail-on:, a workflow that no longer uses the action, and the settings matching. actionlint 1.7.7 is clean on the YAML in the comment; it has no run: blocks, so there is no shell to guard.

Review round 1

• The blocker, the workflow/constants drift: fixed as described above.
• CLAUDE.md in the present tense about settings no committed workflow has: the section now says the gate needs those settings, that the committed file is still fail-on: ERR, and where the patch is. It says to tighten the wording back in this same PR once the maintainer's commit is on the branch.
• 213/31 off by one: re-measured here as well, -f json gives 212 error and 30 warning (the table output adds a header row and a trailing blank, which is where 213/31 came from). Corrected in CLAUDE.md and in the workflow header comment.
• Byte-exact anchors on a Windows checkout: root .gitattributes with api-description.yaml -text.
• Two bare replacens: both go through once now, so a renamed getUsage fails loudly.

[dobby-coder]

.github/workflows/api-diff.yml (replaces the current file)

Two changes from what is on main: fail-on goes from ERR to WARN, and the two opt-in ERR checks are named. Everything else, including the header comment's structure, is the existing file. The comment is rewritten because the current one promises "fails on anything oasdiff rates ERR. Additive changes pass", and neither half is true once the settings change.

The App has no workflows: write on this repo, so this needs a maintainer to commit it, onto this branch rather than onto main. cargo test is red until it lands: the_workflow_uses_the_settings_this_module_pins reads this file and asserts its two inputs are the constants mod api_gate_tests pins, so the settings and the test that certifies them cannot land apart. Applying it here turns the suite green, and it is the last thing this PR needs.

actionlint 1.7.7 is clean on it. There are no run: blocks, so no shell to guard for the pipefail trap.

name: API diff
#
# Breaking-change gate on the OpenAPI contract (#196, #202).
#
# Cryptify's routes are unversioned, so there is no way to keep an old shape
# running next to a new one: any breaking change to api-description.yaml
# breaks the clients pinned to it (pg-js, pg-dotnet, the add-ins). This job
# diffs the PR's spec against the branch it targets and fails on anything
# oasdiff rates WARN or ERR.
#
# 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
# telemetry shows nobody is calling it. A versioned route added beside the
# unversioned one reads as additive, so this gate passes it. Reach for that
# before reaching for err-ignore.
#
# Why WARN and not ERR
# --------------------
# `fail-on: ERR` left several of the changes this contract forbids passing
# silently, because oasdiff rates them WARN. Measured on this spec against
# oasdiff v1.26.1:
#
#   mutation                                  fail-on ERR   fail-on WARN
#   optional response property removed        passes        fails
#   optional response property renamed        passes        fails
#   request parameter removed                 passes        fails
#   request property removed                  passes        fails
#   request parameter constraint narrowed     passes        fails
#   response enum value added                 passes        fails
#
# The first five are the "no removing or narrowing" rule, and at ERR the
# request side of it and the optional half of the response side were both
# unguarded. This spec marks most fields `required`, which does make a removed
# *required* response property an ERR, but that is not the whole rule.
#
# WARN adds 30 checks on top of ERR's 212. All but one are changes the contract
# already forbids (request-parameter-removed, request-property-removed,
# response-body-media-type-schema-removed, the constraint-narrowing *-set
# family). The exception is the sixth row above,
# response-property-enum-value-added: adding a value to
# UploadSessionNotFound.reason or PayloadTooLarge.limit fails this gate even
# though a wider response enum is additive on paper. That is deliberate.
# Today's consumers do tolerate it (pg-js reads `reason` as
# `parsed.reason ?? 'unknown'`), but nothing stops a future client from
# switching on those codes, and a gate that asks for a decision beats one that
# passes it silently. It is the one rule here that only WARN enforces, so it is
# also the first casualty of a revert to ERR; mod api_gate_tests in
# src/main.rs pins it.
#
# There is no standing way to switch just that check off, so do not go looking
# for one: warn-ignore takes a file whose lines must each contain the whole
# rendered change text, per operation and naming the new value, not a check id.
# CLAUDE.md has the details and the x-extensible-enum trade-off.
#
# Two more checks are opt-in: they rate ERR but only run when named, so they
# need the include-checks input below. Without it, changing a 401 to a 403 and
# dropping an enum value from a response both pass. mod api_gate_tests reads
# this file and asserts fail-on and include-checks are the constants it pins,
# so editing one side alone fails `cargo test`.
#
# There is deliberately no `on: paths:` filter. A path-filtered job reports no
# status on the PRs it skips, so as a required check it would leave every PR
# that does not touch the spec pending forever. The job is two checkouts and
# one container, so it just always runs.
#

on:
  pull_request:

permissions:
  contents: read

jobs:

  breaking-changes:
    name: oasdiff breaking changes
    runs-on: ubuntu-latest
    steps:
      - name: Check out the pull request
        uses: actions/checkout@v6
      - name: Check out the base spec
        uses: actions/checkout@v6
        with:
          ref: ${{ github.event.pull_request.base.sha }}
          path: base
      - name: Diff the spec against the base branch
        uses: oasdiff/oasdiff-action/[email protected]
        with:
          base: base/api-description.yaml
          revision: api-description.yaml
          fail-on: WARN
          # Both of these rate ERR but are opt-in, so they do not run unless
          # named: a changed non-success status (401 -> 403) and an enum value
          # dropped from a response property.
          include-checks: response-non-success-status-removed,response-property-enum-value-removed
          # Do not upload the spec to oasdiff.com for a side-by-side review
          # page. The default is `true`; detection and annotations work
          # without it, so nothing leaves CI.
          review: false

Apply with:

gh pr checkout 203
# paste the YAML above over .github/workflows/api-diff.yml
git add .github/workflows/api-diff.yml
git commit -m 'ci: fail the oasdiff gate on WARN and name the two opt-in checks'
git push
cargo test --all-targets api_gate   # green once the file and the constants agree

Two things only a maintainer can do afterwards: keep oasdiff breaking changes as a required check for main (branch protection is out of the App's reach), and confirm oasdiff/oasdiff-action is allowed if this repo has an Actions allowlist.

Once it is on the branch, the CLAUDE.md paragraph that currently says the settings are pending wants tightening back to plain present tense in the same PR. Ping me and I will push that, or edit it in place; it is the paragraph under "The oasdiff gate's settings, and the test that pins them".

One note for the first run after this lands: it is green on this PR, which does not touch the spec. The first PR that does touch it may go red on something that used to pass, and that is the point rather than a defect.

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 1. Rule sweep over the rules that can fire on this diff (Rust test module, CLAUDE.md prose, a CI gate delivered as a patch-in-comment, PR hygiene), merged with the review findings. One blocker, four nits.

The blocker is one root cause with two faces. This branch says the gate's settings are fail-on: WARN plus the two opt-in checks — in FAIL_ON/INCLUDE_CHECKS, in the module doc comment, and in CLAUDE.md as present-tense fact. The committed .github/workflows/api-diff.yml on this branch still says fail-on: ERR with no include-checks (line 40), and cargo test --all-targets is green anyway. So on merge the repo ships a passing test certifying settings the gate does not use, plus agent-loaded docs asserting the same, while the real gate keeps passing the ten forbidden mutations this PR measured. That is the fail-open the module says it exists to prevent. Nothing here needs oasdiff, so the assertion that closes it runs in CI.

Verified rather than argued, on 0af8069:

• git show pr203:.github/workflows/api-diff.yml → fail-on: ERR, no include-checks. review: false and the pinned @v0.1.10 tag are both correctly kept, in the committed file and in the proposed YAML.
• The new test's CRLF failure was reproduced, not reasoned about: sed -i 's/$/\r/' api-description.yaml then cargo test --all-targets api_gate → every_api_gate_mutation_still_applies panics at src/main.rs:3945 on the /metrics:\n anchor. Reverted with the inverse sed, working tree clean. rustc on a CRLF source file confirms raw-string literals are normalised to LF, so the anchors and the file on disk disagree in exactly one direction.
• The check counts were re-measured on a freshly installed oasdiff v1.26.1: -s error is 212 and -s warn is 30 by JSON length; the text output is 214 and 32 rows, which is where 213/31 comes from (header row plus trailing newline). The reviewer's arithmetic holds.
• All CI green, nothing in flight: Build amd64/arm64, Conventional Commit, Rust quality, oasdiff breaking changes.

Rules checked and clean: oasdiff-action-uploads-specs-by-default (review: false present, tag pinned, allow-external-refs left at its default, WARN + the two opt-in checks is the prescribed setting), actions-default-shell-has-no-pipefail (the proposed YAML has no run: blocks at all), conventional-commit-pr-titles, pr-close-issue-keywords (Closes #202 in the body, same repo, targeting main), default-prs-to-ready (draft is justified and the reason is stated — a maintainer must apply the workflow), single-repo-lessons-belong-in-that-repos-claude-md (the measurement went to this repo's CLAUDE.md, nothing to shared rules/), avoid-self-trigger-loops, rust-run-cargo-fmt-before-push.

Not flipping this out of draft, and not touching the nits myself, since the blocker needs a code change in the same file.

[dobby-coder]

VERDICT: approve

Rules Dobby 2, cycle 2. Rule sweep over the rules that can fire on this diff (oasdiff gate settings, patch-in-comment docs accuracy, Actions YAML, Rust/test hygiene, byte-exact anchors, PR hygiene, prose), merged with the re-review findings. No blocking issues. The one new nit is fixed on the branch.

Cycle 1's five findings: all verified fixed

Re-measured against HEAD rather than taken on trust:

• Workflow/constants drift — the_workflow_uses_the_settings_this_module_pins now include_str!s the workflow and compares its two inputs against FAIL_ON/INCLUDE_CHECKS. Confirmed it is the only failing test (156 passed, 1 failed) and that its message names both sides.
• CLAUDE.md present tense — the section now says the committed file is still fail-on: ERR, names where the patch lives, and says to tighten the wording in this same PR once the maintainer's commit is on the branch. That is what patch-in-comment-gate-is-not-live-yet asks for.
• 213/31 off by one — CLAUDE.md and the YAML header both read 212/30 now, matching oasdiff checks -f json.
• Byte-exact anchors on Windows — root .gitattributes with api-description.yaml -text. Checked whether the workflow YAML needs the same treatment, since the module include_str!s it too: it does not. str::lines() strips a trailing \r and every extracted value is trim()ed, and the one substring assert (uses: oasdiff/oasdiff-action/breaking@) contains no newline. So the single -text entry is the right scope, not an omission.
• Two bare replacens — both go through once, which asserts the anchor occurs exactly once.

Also spot-checked against the gate rules: review: false and the pinned @v0.1.10 tag survive into the proposed YAML, allow-external-refs stays at its false default, the YAML has no run: blocks so there is no pipefail exposure, and response-property-enum-value-added — the one rule fail-on: ERR cannot enforce at all — is pinned as a Gate::Stops mutation rather than left out.

The new nit, fixed here rather than sent back (d9e71fa)

The coupling test pinned the gate's two inputs but not that the job is ever triggered, so the one fail-open it could not see was a gate that is present and correctly configured but never fires. Not hypothetical drift: the proposed YAML's own header names the foreseeable edit ("There is deliberately no on: paths: filter"), and a later paths: filter would skip the gate on every PR that does not touch the spec with the test still green.

Added three asserts — a pull_request trigger, no paths:/paths-ignore:, no if: on the job — and provoked each rather than reasoning about it:

probe	result
on: pull_request → on: workflow_dispatch	fails, "no longer triggers on pull_request"
paths: [api-description.yaml] added to the trigger	fails, "has a path filter on its trigger"
if: false on the breaking-changes job	fails, "has an if: condition"

The trigger check reads the on: block line by line instead of matching the mapping form byte-for-byte, because on: is legal as a mapping, a list or a bare scalar and a rewrite between those forms changes nothing about when the gate runs. A check that cries wolf is the one that gets deleted.

Verified after the change: cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings clean, cargo test --all-targets still 156 passed / 1 failed with the same settings-mismatch message, and 157 passed / 0 failed with the comment's YAML applied — so the "apply the comment and it goes green" promise still holds with the new asserts in place. oasdiff v1.26.1 was on PATH for these runs, so the mutation test did the real work rather than skipping.

Non-blocking, not acted on

The body's "the gate would go on passing the ten forbidden mutations below" is loose: nine of those ten are in the "forbidden by the contract" table, and the tenth (a new response enum value) sits in the additive table as a change this gate deliberately blocks anyway. The same phrasing is in the test's failure message. Left alone — the body devotes a whole section to exactly that distinction, so a reader is not misled, and churning a failure message over one word is not worth a commit.

On prose, the formatting flags did not survive a recount. PR body: 1,799 words, zero em dashes, 5 inline-header bullets, and of 27 bold spans 16 are **passes**/**fails** cells inside the two tables marking which verdicts are the gaps — 11 prose bold spans, against 11 in the base CLAUDE.md's 1,712 words. Added CLAUDE.md prose: 633 words, zero em dashes, one bold span. No banned vocabulary in any of the three artifacts. The **X:** bullets in "Review round 1" label five distinct findings with their fixes, which is a table, not decoration.

Ready for review, while red

Flipping this out of draft even though Rust quality is red, because the red is the deliverable and only a maintainer can clear it. default-prs-to-ready keeps drafts for work that is unfinished or unsure; this is neither, and leaving it draft would deadlock — nobody applies the YAML, so CI never greens, so it never gets flipped.

What is left is one maintainer action: commit the YAML from the comment onto this branch (not onto main), which turns the suite green. Two things after that which are out of the App's reach: keeping oasdiff breaking changes a required check for main, and tightening the CLAUDE.md paragraph back to plain present tense in this same PR once the workflow file and the docs line sit in the same tree.

[rubenhensen]

Workflow half applied verbatim — that was the whole failure. the_workflow_uses_the_settings_this_module_pins was red because the test module pins fail-on: WARN plus the two opt-in checks while .github/workflows/api-diff.yml on this branch still said ERR with none, which is exactly the disagreement it exists to catch. All three api-gate tests pass locally now, mutation test included, and cargo fmt --check is clean.

Worth calling out that this test is a better pattern than the hand-off we have been using elsewhere. Every other repo's workflow patch sits in a PR comment and nothing notices if it is never applied — which is how postguard#265 merged with an inert crate and postguard-js#137 shipped with a release path nobody had wired. Here the missing half turns cargo test red and names both sides of the disagreement. Worth copying to the other gates; I will raise that separately rather than widening this PR.

Also noting your measurement: ten forbidden changes passed at ERR on this spec, against four on pg-pkg's. Cryptify's routes are unversioned, so there is no /v3 escape hatch for any of them — this gate was the weakest one in the fleet and is now the strictest.