Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# `mod api_gate_tests` in src/main.rs locates blocks of api-description.yaml by
# multi-line anchors copied verbatim out of it. rustc normalises CRLF to LF
# inside raw strings, but a CRLF checkout of the spec keeps its `\r\n`, so on a
# Windows clone (git installs with core.autocrlf=true) every anchor would miss
# and the test suite would go red on a clean tree. `-text` keeps the checked-out
# bytes equal to the stored ones in both directions.
api-description.yaml -text
63 changes: 59 additions & 4 deletions .github/workflows/api-diff.yml
Original file line number Diff line number Diff line change
@@ -1,16 +1,67 @@
name: API diff
#
# Breaking-change gate on the OpenAPI contract (#196).
# 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 ERR. Additive changes pass.
# 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.
# 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:
Expand All @@ -37,7 +88,11 @@ jobs:
with:
base: base/api-description.yaml
revision: api-description.yaml
fail-on: ERR
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.
Expand Down
72 changes: 72 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,78 @@ The spec is also the external contract for pg-js, pg-dotnet, and the add-ins, so
a breaking edit to it needs a new versioned route rather than an in-place change
(cryptify's routes are unversioned, so there is no other escape hatch).

## The oasdiff gate's settings, and the test that pins them

`.github/workflows/api-diff.yml` diffs the PR's `api-description.yaml` against
the base branch. Its whole behaviour is two step inputs, and a wrong pair fails
open: the job goes green and nobody learns the change went through. So
`mod api_gate_tests` in `src/main.rs` mutates the real spec one way per rule,
runs the real engine with the flags the action's entrypoint builds, and asserts
stop-or-pass. It also reads the committed workflow and asserts its two inputs
are the constants the module pins, and that the job still triggers on
`pull_request` with no path filter and no `if:` — settings on a gate that never
runs fail open just as quietly. So the two cannot drift apart unnoticed.

The settings the gate needs are `fail-on: WARN` plus
`include-checks: response-non-success-status-removed,response-property-enum-value-removed`.
**The committed workflow does not have them yet**: it is still `fail-on: ERR`
with no `include-checks`. The new pair sits in the `api-diff.yml` patch on
PR #203 and needs a maintainer to apply it, because the App cannot push
`.github/workflows/`. Until that lands, the gate is passing everything in the
list below, and `the_workflow_uses_the_settings_this_module_pins` is red saying
so. Tighten this paragraph back to plain present tense in that same PR once the
maintainer's commit is on the branch.

Measured on this spec against oasdiff v1.26.1, `fail-on: ERR` on its own passes
several changes the contract forbids. A `401` that becomes a `403` and a
dropped response enum value rate ERR but are opt-in, so they never run unless
named, which is what `include-checks` is for. The rest rate WARN, not ERR: a removed
or renamed optional response property, a removed request parameter, a removed
request property, and the constraint-narrowing `*-set` family. Those gaps are
spec-independent, so they apply here even though this spec marks most fields
`required` (which does make a removed *required* response property an ERR).

WARN adds 30 checks on top of ERR's 212 (`oasdiff checks -s warn -f json`; the
table output has two rows more than that, a header and a trailing blank). All
but one of the 30 are changes the contract
already forbids. The exception is `response-property-enum-value-added`: adding
a value to `UploadSessionNotFound.reason` or `PayloadTooLarge.limit` fails the
gate even though a wider response enum is additive on paper, and today's
consumers do tolerate it (pg-js reads `reason` as `parsed.reason ?? 'unknown'`,
a plain string, and the tb-addon passes it through). It is kept anyway: nothing
stops a future client from switching on those codes, and a red gate that asks
for a decision beats a silent pass. It is also the only rule here that WARN
alone enforces, so it is the first casualty of a revert to ERR, which is why
the test pins it.

Two things to know before reaching for a suppression. `--warn-ignore` and
`--err-ignore` do not take check ids or partial regexes: the ignore file is
matched by asking whether an ignore line *contains* the rendered change text,
so a line has to spell out the whole thing in lowercase, per affected
operation, including the new value:

```
in api put /fileupload/{uuid} added the new `quota_exceeded` enum value to the `reason` response property for the response status `404`
```

A bare `response-property-enum-value-added`, or even `.*`, suppresses nothing
(verified on v1.26.1). So there is no standing "ignore this check" setting;
every future enum value needs its own lines. And `x-extensible-enum` in place
of `enum:`, which oasdiff's own message suggests, makes it skip that property
altogether: adding a value passes, but so does *removing* one, so that trade
buys the false positive back with a gap.

The mutation test needs the engine, which no runner has, so it skips in CI. The
other two do run there: `every_api_gate_mutation_still_applies` catches a spec
edit that strands an anchor, and
`the_workflow_uses_the_settings_this_module_pins` catches the workflow and the
constants disagreeing. To run the mutation test for real:

```
go install github.com/oasdiff/[email protected] # the version the action tag pins
cargo test --all-targets api_gate
```

## Content-Range end byte is EXCLUSIVE on the chunk PUT
`upload_chunk` rejects `start >= end` and takes the chunk length to be
`end - start`, so `bytes 200-1000/*` is 800 bytes at offset 200, not the 801 that
Expand Down
Loading