diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..107e94cf --- /dev/null +++ b/.gitattributes @@ -0,0 +1,7 @@ +# pg-pkg/tests/api_gate.rs locates blocks in this spec with multi-line raw-string +# anchors. rustc normalises CRLF to LF inside raw strings, but the file read off +# disk keeps whatever git wrote, so on a Windows checkout (core.autocrlf=true is +# the Git for Windows default) every anchor misses and the suite goes red on a +# clean tree. Keeping the stored bytes is also what makes the oasdiff gate's +# verdict reproducible off a local checkout. +pg-pkg/api-description.yaml -text diff --git a/.github/workflows/api-diff.yml b/.github/workflows/api-diff.yml new file mode 100644 index 00000000..7652d30e --- /dev/null +++ b/.github/workflows/api-diff.yml @@ -0,0 +1,107 @@ +name: API diff +# +# Breaking-change gate on the pg-pkg OpenAPI contract (#249). +# +# pg-pkg/api-description.yaml is the pinned v2 HTTP contract (#242), one of the +# three seams COMPATIBILITY.md guarantees. This job diffs the PR's spec against +# the branch the PR targets and fails on any change oasdiff rates WARN or ERR. +# +# Escape hatch, not a wall: pg-pkg's routes are versioned (unlike cryptify's, +# whose gate this mirrors), so a change /v2 cannot take additively goes under a +# new prefix (/v3/...) with /v2 left running, and /v2 is retired later through +# the deprecation process in COMPATIBILITY.md, once postguard-ops#64 telemetry +# shows nobody calls it. A /v3 route added next to /v2 reads as additive, so +# this gate passes it. Reach for that before reaching for err-ignore. +# +# Why WARN and not ERR +# -------------------- +# `fail-on: ERR` would leave four of the changes COMPATIBILITY.md forbids +# passing silently, because oasdiff rates them WARN, and on this spec that is +# most of the "no removing a route or a field" rule: only `status` is ever +# `required`, so `key` (the IBE user secret key the endpoint exists to return), +# `proofStatus`, `pubSignKey` and the rest are optional, and removing or +# renaming an optional response property is WARN. So is removing a request +# parameter. Measured on this spec against oasdiff v1.26.1: +# +# mutation fail-on ERR fail-on WARN +# optional response property removed (`key`) passes fails +# optional response property renamed passes fails +# required path parameter removed passes fails +# response enum value added passes fails +# +# The fourth is the rule COMPATIBILITY.md gains alongside this gate; see below. +# +# WARN adds 31 checks on top of ERR's 213. All but one are changes +# COMPATIBILITY.md already forbids (request-parameter-removed, +# request-property-removed, response-body-media-type-schema-removed, and the +# constraint-narrowing *-set family). The exception is +# response-property-enum-value-added: adding a value to SessionStatus or +# ProofStatus fails this gate. That is deliberate, and COMPATIBILITY.md now +# says so too. A client that switches on the enum without a default branch +# breaks on a value it has never seen, so a new status is a /v3 change (or an +# x-extensible-enum one), not an additive one. It is the one rule here that +# only WARN enforces, so it is also the first casualty of a revert to ERR; +# pg-pkg/tests/api_gate.rs pins it. +# +# 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. Keep include-checks and +# fail-on in step with the local-repro command in CLAUDE.md, or a local run +# quietly disagrees with CI. +# +# Not covered: the gate compares documented paths, and the spec documents the +# canonical /v2/request/... paths only (see its "Path prefix aliases" note), so +# dropping the /v2/irma/... alias handlers before #257's deprecation has run +# passes this gate. That one stays a review rule. +# +# 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: + # `edited` included for base retargets (e.g. a stacked PR's base merging): + # this job's verdict depends on the base sha, and without `edited` a stale + # verdict stays attached to the unchanged head sha. + types: [opened, synchronize, reopened, edited] + +permissions: + contents: read + +jobs: + + breaking-changes: + name: API breaking changes (oasdiff) + runs-on: ubuntu-latest + steps: + - name: Check out the pull request + uses: actions/checkout@v6 + with: + # The spec is all this job reads, and the oasdiff container gets the + # workspace mounted; do not leave a push token in .git/config for it. + persist-credentials: false + - name: Check out the base spec + uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.base.sha }} + path: base + persist-credentials: false + - name: Diff the spec against the base branch + # Pinned by sha because this step is the gate's verdict. The tag pins + # the engine: v0.1.10 is FROM tufin/oasdiff:v1.26.1, so a local + # `oasdiff v1.26.1` reproduces what CI decides here. + uses: oasdiff/oasdiff-action/breaking@0ab8ad204b00d25acc5ae87106281433e288d0c1 # v0.1.10 + with: + base: base/pg-pkg/api-description.yaml + revision: pg-pkg/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 two specs to oasdiff.com for a side-by-side + # review page. The default is `true`; the detection and the inline + # annotations work without it, so nothing leaves CI. + review: false diff --git a/CLAUDE.md b/CLAUDE.md index f706ca72..d51a2f83 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,10 +10,13 @@ Migrated from the dobby memory repo (`encryption4all/dobby`). This file is the s - JS-reader gotchas the Node gate ran into, all still true of `@e4a/pg-wasm` 0.6.1 and `@e4a/pg-js` 1.11.0/2.3.3: (a) `@e4a/pg-wasm`'s default (bundler) entry does `import * as wasm from "./index_bg.wasm"`, which plain Node cannot resolve — import `@e4a/pg-wasm/web` and pass the module bytes to its default export, resolving them via `new URL('index_bg.wasm', import.meta.resolve('@e4a/pg-wasm/web'))` since the `.wasm` file is not in the package's `exports`; (b) `Unsealer.unseal()`/`StreamUnsealer.unseal()` *consume* the unsealer (wasm-bindgen `__destroy_into_raw`), so a tidy `free()` afterwards is a double free reported as `null pointer passed to rust`, which reads exactly like a corrupt container; (c) `pg-js` is stream-mode only in both directions — `toBytes()` seals with `sealStream` and its decrypt path only ever builds a `StreamUnsealer`, so a memory-mode container fails with `mode is not supported: InMemory { size: N }`; (d) `pg-js` 1.x discards what `StreamUnsealer.unseal()` returns and reports `public_identity()` instead, so it never surfaces the private signing policy of a `*-privsig` container and its `sender.raw` is the bare header policy rather than 2.x's `{public, private?}`; (e) `pg-js`'s decrypt path needs a PKG at `pkgUrl` for exactly two GETs — `/v2/sign/parameters` for the verifying key and `/v2/irma/key/` (bearer token) for the USK — which is why the gate can drive it offline from the artifact's own `vk.json`/`usk-*.json`. - Appending a field at the *end* of `Header` really is additive: the header is a length-prefixed region and `bincode` ignores trailing bytes, so published `pg-core` 0.6.1 still opens it. A field inserted anywhere else, a changed field type, or a reorder shifts every following byte and the containers stop opening, but *not* with a decode error: 0.6.1 reads a garbage length prefix, attempts a ~20 GiB allocation, and the process aborts (SIGABRT). Expect `reader died on signal 6 ... memory allocation of N bytes failed`, not a message naming the header. This is also why `pg-compat` opens each case in a child process (its `pg-compat-case` binary): an abort is not a panic, `catch_unwind` cannot contain it, and in one process the first broken case would take the run down before the others were tried. Don't reason about "additive" from the struct alone; run the compat gate. - CI's `Format workspace` matrix runs `cargo fmt --manifest-path pg-/Cargo.toml --all -- --check` per crate over shared workspace files; always run `cargo fmt --all -- --check` from repo root before pushing, or one crate's drift fails the whole matrix. +- `Run wasm tests in browsers` flakes, and the error names the wrong culprit. `Error: missing field 'chunk'` is `wasm-bindgen-test-runner` failing to parse a truncated webdriver reply; the cause is the line above it, `[SEVERE]: Timed out receiving message from renderer: 30.000`. Read the driver stderr before suspecting the test. The matrix is fail-fast, so one browser timing out reports the other two as failures when they were cancelled: check each job's own conclusion, not the summary. Seen on the same sha passing at 07:45 and failing at 07:48 (runs 30432815599 and 30432995207 on #269, a docs-only commit). Re-run rather than debug, and note that `dobby-coder` cannot: `POST /actions/runs/{id}/rerun-failed-jobs` is 403 for the App, so a maintainer has to click it, or a fresh push has to supersede the run. - `scripts/semver-checks.sh` runs `cargo-semver-checks` over the two surfaces external consumers build against: `pg-core` against its crates.io release, and `pg-wasm` against `origin/main` (it has no crates.io release; the npm package is versioned from `pg-core`). The `semver-checks` job in `build.yml` calls it on any PR touching `pg-core`, `pg-wasm`, the root manifest or the script itself; run it yourself too before pushing such a change, since the job needs a wasm32 toolchain and a pinned cargo-semver-checks download and is therefore not the fastest feedback. Four things it encodes. (1) `pg-core` needs `--only-explicit-features --features test,rust,stream`, the same set the test and clippy matrices use: cargo-semver-checks otherwise enables everything that doesn't look unstable, which pulls in `web` and hits its `compile_error!`. (2) `pg-core`'s `web,stream` surface is deliberately not checked. `Unsealer` has two `unseal` methods there on different instantiations (owned `self` in `client/web/mod.rs`, `&mut self` in `client/web/stream.rs`) and cargo-semver-checks 0.49 pairs them by name alone, so it reports `method_receiver_mut_ref_became_owned` against byte-identical source; `rust,stream` is clean because both receivers are owned there. (3) Any wasm32 run needs `RUSTFLAGS=--cap-lints=warn`, because the `--cap-lints allow` cargo-semver-checks sets silences the "dropping unsupported crate type" warnings cargo reads back when probing rustc, and cargo then dies with "output of --print=file-names missing". (4) `cargo-semver-checks` splits its non-zero exits: `100` is a semver violation, `101` is the tool or the build failing (unresolvable baseline rev, missing rustup target, registry fetch failure, compile error in the crate). Never treat "non-zero" as "breaking change" here, because the advice a semver gate prints is "declare the break", and on this repo that means a `!` in the PR title and a spurious major release of `pg-core`. `scripts/semver-checks-test.sh` pins that mapping; it stubs `cargo`, so it runs in well under a second and needs neither cargo-semver-checks nor a wasm32 toolchain. Run it after touching the gate. - release-plz owns the version numbers, so the PR making a breaking change cannot bump the crate to match (bumping `pg-core` alone doesn't even resolve: `pg-cli` requires `^0.6.1`). What the semver gate accepts as the declaration is the conventional-commit `!` in the PR title, and only that; CI turns it into `SEMVER_RELEASE_TYPE=major`, which the script passes as `--release-type major`. A `BREAKING CHANGE:` footer in the PR body is not accepted and must not be: this repo's `squash_merge_commit_message` is `COMMIT_MESSAGES`, so the body never reaches the squashed commit, and release-plz reading a bare `fix(pg-core):` subject would cut a patch release of a break the gate had already waved through. Two consequences of the merge settings worth knowing when you declare a break. `squash_merge_commit_title` is `COMMIT_OR_PR_TITLE`, which is the PR title on a multi-commit PR but the commit's subject when the PR has exactly one commit — so on a single-commit PR put the `!` in the commit subject too, or the gate goes green off the PR title while release-plz cuts a patch. And `--release-type major` doesn't merely permit a bigger bump: every lint exists to demand a bump the declaration already grants, so all of them skip and the run checks nothing (`0 checks: 0 pass, 253 skip`) on both surfaces at once. A green gate on a `!` PR verified nothing; a `!` added for a pg-wasm break also passes any unrelated pg-core break in the same PR. - The Docker build (`Dockerfile`, `FROM rust:-slim`) pins an older or different Rust than the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the Dockerfile's current pin, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the Docker image. -- The `dobby-coder` GitHub App lacks `workflows: write` on this repo; any push touching `.github/workflows/*.yml` is rejected at the remote. Before treating a fix as blocked, check whether the same effect can be achieved in a pushable file (crate manifest, source, committed script); if a fix genuinely can only live in a workflow file, ship the pushable half and hand the maintainer ready-to-paste YAML in the PR body. The block covers *merge* commits too, which is easy to miss: once a branch carries its own `build.yml` change (typically a maintainer applying such a patch onto it), a later `git merge origin/main` that has to touch `build.yml` produces a commit updating a workflow file, and the push is rejected even when the resolution is only "keep both new jobs". Nothing can be split out of a merge commit, so that merge has to be landed by a maintainer, or the App needs `workflows: write`. +- `pg-pkg/api-description.yaml` is the pinned v2 HTTP contract and is additive-only (see `COMPATIBILITY.md`). Its breaking-change gate is `.github/workflows/api-diff.yml`, job `API breaking changes (oasdiff)`, which runs on every PR; `pg-pkg/tests/api_gate.rs` is its executable spec. That test is the executable record of what the gate stops; it mutates the spec 19 ways and asserts each verdict. The verdict test skips when `oasdiff` is not on `PATH`, which is the case in CI; the other two run everywhere, one checking that every mutation still finds its anchor and one reading `fail-on`/`include-checks`/the pinned action ref back out of the workflow YAML, so editing the step and the constants apart fails the suite instead of quietly weakening the gate. The action ref is pinned because it is what selects the engine version (`v0.1.10` is `FROM tufin/oasdiff:v1.26.1`), so bumping the action silently re-measures every verdict. Those anchors are multi-line raw strings matched against the spec read off disk, which is why root `.gitattributes` marks `pg-pkg/api-description.yaml -text`: rustc normalises CRLF inside raw strings but `read_to_string` does not, so without it a Windows checkout fails `every_mutation_still_applies` on a clean tree. Run it (and read it) before touching the gate's settings: `go install github.com/oasdiff/oasdiff@v1.26.1 && cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate`. +- The oasdiff gate's settings are **not** self-evident and `--fail-on ERR` alone fails open. `fail-on: WARN` is deliberate: oasdiff rates removing or renaming an *optional* response property, and removing a request parameter, as WARN, and this spec marks only `status` as `required`, so at ERR the gate silently passed a removed `key` (the IBE user secret key `/v2/request/key` exists to return), a renamed `proofStatus` and a dropped `timestamp` parameter. Two more, a changed non-success status (401 to 403) and a dropped response enum value, rate ERR but are **opt-in**, so they only run when named in `include-checks`. Of the 31 WARN checks, `response-property-enum-value-added` is the only one that fires on something `COMPATIBILITY.md` does not already forbid, which is why that document now names a new response enum value as non-additive too. Reproduce a verdict with the exact flags the action's entrypoint builds (`oasdiff/oasdiff-action/breaking@v0.1.10` is `FROM tufin/oasdiff:v1.26.1`, so the pinned tag is what makes a local run authoritative): `git show origin/main:pg-pkg/api-description.yaml > /tmp/base.yaml && oasdiff breaking /tmp/base.yaml pg-pkg/api-description.yaml --allow-external-refs=false --composed=false --fail-on WARN --include-checks response-non-success-status-removed,response-property-enum-value-removed`. Two traps: `oasdiff --version` prints `oasdiff version main` after a `go install` of a tag because the version comes from release ldflags (the code is still the tag), and `--fail-on` takes `ERR`/`WARN` while `oasdiff checks --severity` takes `error`/`warn`/`info`. `--severity ERR` is a usage error, so `oasdiff checks --severity ERR | wc -l` counts the help text instead and badly undercounts the tier (it is 213 error checks, 31 warn, 265 info). The spec has no external `$ref`s, so `allow-external-refs` stays at its safe (SSRF-guarding) default. The gate only sees paths the spec documents, and the spec documents canonical paths only, so dropping the `/v2/irma/...` alias handlers (#257) passes it. +- The `dobby-coder` GitHub App lacks `workflows: write` on this repo; any push touching `.github/workflows/*.yml` is rejected at the remote. Before treating a fix as blocked, check whether the same effect can be achieved in a pushable file (crate manifest, source, committed script); if a fix genuinely can only live in a workflow file, ship the pushable half and hand the maintainer ready-to-paste YAML in the PR body. The block covers *merge* commits too, which is easy to miss: once a branch carries its own `build.yml` change (typically a maintainer applying such a patch onto it), a later `git merge origin/main` that has to touch `build.yml` produces a commit updating a workflow file, and the push is rejected even when the resolution is only "keep both new jobs". Nothing can be split out of a merge commit, so that merge has to be landed by a maintainer, or the App needs `workflows: write`. Measured exception, worth trying before handing the sync over: the App pushed `ce0fc59` on this branch, a merge whose diff against its first parent added main's 64 new `build.yml` lines. That merge needed no resolution inside `build.yml` — it took main's side whole, so the blob it committed already existed in the repo. Try the merge and read the remote's answer; only escalate on an actual rejection. ## Dependencies diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 16f7237b..8434a72a 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -14,7 +14,9 @@ Changes to `/v2` are additive only. New endpoints, new optional request fields and new response fields are allowed, so a client written against an older revision of the spec keeps working. These are not allowed on `/v2`: removing a route or a field, renaming either, narrowing a type, making an optional field -required, or changing the status code for a condition a client already handles. +required, changing the status code for a condition a client already handles, or +adding a value to a response enum, since a client that switches on `status` +without a default branch breaks on a value it has never seen. A change that cannot be made additively ships under a new prefix (`/v3`), with `/v2` left running until its consumers are gone. @@ -26,8 +28,16 @@ removed once the deprecation process at the bottom of this file has run for it ([#257]). Until then it keeps working, so a deployed client on `/v2/irma/...` is not broken by this notice. -Planned enforcement: an oasdiff gate that diffs the spec against `main` and -fails on a breaking change ([#249]). Until that lands, this is a review rule. +Enforcement: the `API breaking changes (oasdiff)` job in +`.github/workflows/api-diff.yml` diffs the spec against the branch a PR targets +and fails on any change oasdiff rates WARN or ERR ([#249]). That covers every +rule above except one. The gate compares documented paths, and this spec +documents the canonical `/v2/request/...` paths only, so dropping the +`/v2/irma/...` alias handlers before the deprecation above has run passes it; +that one stays a review rule. A `/v3` route added next to `/v2` reads as +additive, so the gate passes the escape hatch. `pg-pkg/tests/api_gate.rs` pins +which changes the gate stops and which it lets through; run it before changing +what the gate checks. ## Stored artifacts diff --git a/pg-pkg/tests/api_gate.rs b/pg-pkg/tests/api_gate.rs new file mode 100644 index 00000000..c36393bb --- /dev/null +++ b/pg-pkg/tests/api_gate.rs @@ -0,0 +1,612 @@ +//! What the API breaking-change gate actually catches (issue #249). +//! +//! The gate is `.github/workflows/api-diff.yml`, which runs `oasdiff breaking` +//! over `api-description.yaml` and is what stops a careless edit from breaking a +//! deployed client. +//! +//! The gate's verdict is decided by two step inputs, `fail-on` and +//! `include-checks`, plus the pinned action ref that decides which engine +//! version reads them, and getting any of the three wrong fails open: the job +//! goes green and nobody learns that the change it was supposed to stop went +//! through. +//! `fail-on: ERR` alone passes a removed optional response property, a removed +//! request parameter, a changed status code and a dropped response enum value, +//! all of which COMPATIBILITY.md forbids. +//! +//! So the inputs are pinned here as well as in the workflow: this test mutates +//! the real spec, runs the real engine with the real flags, and asserts which +//! mutations the gate stops. [`the_workflow_step_matches_the_pinned_inputs`] +//! reads all three back out of the YAML, so editing one side without the other +//! fails here instead of silently weakening the gate. +//! +//! The engine is not vendored, so the verdict test needs `oasdiff` on `PATH` +//! (or `OASDIFF` pointing at it) and skips when it is absent, which is the case +//! in CI. Install the version the action pins, so a local verdict is CI's: +//! +//! ```text +//! go install github.com/oasdiff/oasdiff@v1.26.1 +//! cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate +//! ``` + +use std::env; +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// `fail-on` in the workflow's oasdiff step. WARN, not ERR: on this spec only +/// `status` is ever `required`, so removing or renaming any other response +/// property is WARN, and at ERR the gate would pass most of what +/// COMPATIBILITY.md forbids. +const FAIL_ON: &str = "WARN"; + +/// `include-checks` in the workflow's oasdiff step. Both rate ERR but are +/// opt-in, so they do not run unless named. +const INCLUDE_CHECKS: &str = + "response-non-success-status-removed,response-property-enum-value-removed"; + +/// The action ref in the workflow's oasdiff step. This is the third input the +/// verdicts depend on, and the least obvious: it pins the *engine*, because +/// `v0.1.10`'s Dockerfile is `FROM tufin/oasdiff:v1.26.1` and every verdict +/// below was measured against v1.26.1. A bump with no re-measurement is the +/// same fail-open `FAIL_ON` guards against, so it is pinned here too. +const ACTION_REF: &str = "oasdiff/oasdiff-action/breaking@0ab8ad204b00d25acc5ae87106281433e288d0c1"; + +/// Whether the gate stops a change, i.e. whether the job goes red. +#[derive(Debug, PartialEq, Eq)] +enum Gate { + /// Additive as far as a deployed client is concerned. + Passes, + /// Breaking under COMPATIBILITY.md's `/v2` rules. + Stops, +} + +impl Gate { + fn verb(&self) -> &'static str { + match self { + Gate::Passes => "pass", + Gate::Stops => "stop", + } + } + + fn past(&self) -> &'static str { + match self { + Gate::Passes => "passed", + Gate::Stops => "stopped", + } + } +} + +fn spec_path() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("api-description.yaml") +} + +fn workflow_path() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join(".github/workflows/api-diff.yml") +} + +/// The value of the `key:` mapping entry in the workflow, with the surrounding +/// comment lines ignored. Requires exactly one such entry, so a second oasdiff +/// step (or an input moved into a matrix) fails the test rather than having one +/// of the two verdicts silently go unchecked. +fn workflow_input(workflow: &str, key: &str) -> String { + let needle = format!("{key}:"); + let values: Vec<&str> = workflow + .lines() + .map(str::trim) + .filter(|line| !line.starts_with('#')) + .filter_map(|line| line.strip_prefix(&needle)) + .map(str::trim) + .collect(); + assert_eq!( + values.len(), + 1, + "expected exactly one `{key}:` in {}, found {}", + workflow_path().display(), + values.len() + ); + values[0].to_owned() +} + +/// The `oasdiff` binary, or `None` when it is not installed. +fn oasdiff() -> Option { + if let Some(explicit) = env::var_os("OASDIFF") { + return Some(PathBuf::from(explicit)); + } + let found = Command::new("oasdiff") + .arg("--help") + .output() + .is_ok_and(|out| out.status.success()); + found.then(|| PathBuf::from("oasdiff")) +} + +/// Replaces `old` with `new`, requiring `old` to occur exactly once so a spec +/// edit that moves an anchor fails loudly instead of silently mutating nothing. +fn once(text: &str, old: &str, new: &str) -> String { + assert_eq!( + text.matches(old).count(), + 1, + "anchor is not unique in the spec, so this mutation no longer means what it says: {old:?}" + ); + text.replacen(old, new, 1) +} + +/// The half-open byte range of the block starting at `start` and ending where +/// the next `end` begins. +fn block(text: &str, start: &str, end: &str) -> (usize, usize) { + let from = text.find(start).unwrap_or_else(|| panic!("no {start:?}")); + let to = text[from..] + .find(end) + .unwrap_or_else(|| panic!("no {end:?} after {start:?}")); + (from, from + to) +} + +/// Runs the gate exactly as the workflow's step does: the committed spec as the +/// base, `revision` as the PR's version. +/// +/// The entrypoint of `oasdiff/oasdiff-action/breaking@v0.1.10` turns the step +/// inputs into `--allow-external-refs=false --include-checks +/// --composed=false --fail-on `, so those are the flags used here. Exit 0 +/// is a clean diff and exit 1 is "breaking changes found"; anything else is the +/// engine refusing the input, which means the mutation produced a spec oasdiff +/// cannot load and any verdict read off it would be meaningless. +fn gate(oasdiff: &Path, revision: &Path) -> Gate { + let out = Command::new(oasdiff) + .arg("breaking") + .arg(spec_path()) + .arg(revision) + .arg("--allow-external-refs=false") + .args(["--include-checks", INCLUDE_CHECKS]) + .arg("--composed=false") + .args(["--fail-on", FAIL_ON]) + .output() + .expect("run oasdiff"); + + match out.status.code() { + Some(0) => Gate::Passes, + Some(1) => Gate::Stops, + other => panic!( + "oasdiff exited {other:?} instead of 0 or 1, so it never reached a verdict:\n{}\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr), + ), + } +} + +// The two response blocks the mutations below anchor on. Both are unique in the +// spec, and `once` fails the test if that ever stops being true. + +const TIMESTAMP_PARAMETER: &str = r##" - name: timestamp + in: path + required: true + description: "Unix seconds; the policy timestamp the key must match." + schema: + type: integer + format: int64 +"##; + +const RATE_LIMITED_THEN_IRMA_503: &str = r##" "429": + description: "Rate limited." + "503": + description: "IRMA verification key unavailable (IRMA server unreachable)." +"##; + +const JWT_401: &str = r##" "401": + description: "Missing/invalid JWT." + content: + application/json: + schema: + $ref: "#/components/schemas/Error" +"##; + +// --------------------------------------------------------------------------- +// Additive: allowed on `/v2`, so the gate must let these through. A gate that +// stops them is worse than no gate, because the way around it is to switch it +// off. +// --------------------------------------------------------------------------- + +fn add_endpoint(spec: &str) -> String { + once( + spec, + " /v2/request/key:\n", + r##" /v2/request/echo: + get: + tags: ["Keys"] + summary: "Echo" + operationId: "echo" + responses: + "200": + description: "ok" + /v2/request/key: +"##, + ) +} + +fn add_optional_response_property(spec: &str) -> String { + once( + spec, + " key:\n description: |\n Opaque pg-core-serialized IBE", + r##" issuedAt: + type: integer + format: int64 + description: "Unix seconds the key was derived." + key: + description: | + Opaque pg-core-serialized IBE"##, + ) +} + +fn add_optional_request_property(spec: &str) -> String { + once( + spec, + " required: [pubSignId]\n additionalProperties: false\n properties:\n", + r##" required: [pubSignId] + additionalProperties: false + properties: + clientHint: + type: string +"##, + ) +} + +fn add_required_response_property(spec: &str) -> String { + once( + spec, + " required: [tenant_id]\n properties:\n", + r##" required: [tenant_id, checked_at] + properties: + checked_at: + type: integer + format: int64 +"##, + ) +} + +fn add_optional_query_parameter(spec: &str) -> String { + once( + spec, + " operationId: \"key\"\n security:", + r##" operationId: "key" + parameters: + - name: verbose + in: query + required: false + schema: + type: boolean + security:"##, + ) +} + +fn add_response_status(spec: &str) -> String { + let with_404 = format!( + " \"404\":\n description: \"No such timestamp.\"\n{RATE_LIMITED_THEN_IRMA_503}" + ); + once(spec, RATE_LIMITED_THEN_IRMA_503, &with_404) +} + +fn edit_a_description(spec: &str) -> String { + once( + spec, + "description: \"Yivi session status.\"", + "description: \"Yivi session status (see the Yivi docs).\"", + ) +} + +/// The escape hatch COMPATIBILITY.md points at: a change `/v2` cannot take +/// additively ships under `/v3` with `/v2` left running. If the gate stopped +/// this there would be no way to make a breaking change at all. +fn add_v3_route_beside_v2(spec: &str) -> String { + let (from, to) = block(spec, " /v2/request/key:\n", " /v2/request/sign/key:\n"); + let v3 = spec[from..to] + .replacen(" /v2/request/key:", " /v3/request/key:", 1) + .replacen("operationId: \"key\"", "operationId: \"keyV3\"", 1); + once( + spec, + " /v2/request/sign/key:\n", + &format!("{v3} /v2/request/sign/key:\n"), + ) +} + +// --------------------------------------------------------------------------- +// Breaking: forbidden on `/v2` by COMPATIBILITY.md, so the gate must stop +// these. Each one breaks a client written against today's spec. +// --------------------------------------------------------------------------- + +fn remove_route(spec: &str) -> String { + let (from, to) = block( + spec, + " /v2/request/key/{timestamp}:\n", + " /v2/request/key:\n", + ); + format!("{}{}", &spec[..from], &spec[to..]) +} + +fn request_property_becomes_required(spec: &str) -> String { + once( + spec, + " required: [pubSignId]\n", + " required: [pubSignId, privSignId]\n", + ) +} + +/// A client that handles 401 by refreshing its JWT sees an unhandled 403. +fn change_a_status_code(spec: &str) -> String { + once( + spec, + &format!("{JWT_401}{RATE_LIMITED_THEN_IRMA_503}"), + &format!( + "{}{RATE_LIMITED_THEN_IRMA_503}", + JWT_401.replacen("\"401\":", "\"403\":", 1) + ), + ) +} + +fn remove_a_non_success_status(spec: &str) -> String { + once( + spec, + r##" "401": + description: "Unknown, expired or revoked key." + content: + application/json: + schema: + $ref: "#/components/schemas/Error" +"##, + "", + ) +} + +fn remove_a_response_enum_value(spec: &str) -> String { + once( + spec, + "enum: [INITIALIZED, PAIRING, CONNECTED, CANCELLED, DONE, TIMEOUT]", + "enum: [INITIALIZED, PAIRING, CONNECTED, CANCELLED, DONE]", + ) +} + +/// The rule COMPATIBILITY.md gained for this gate: the one WARN check that fires +/// on a change the document did not already forbid, kept rather than suppressed +/// because a client switching on `status` with no default branch breaks on a +/// value it has never seen. Pinned here because it is the newest rule and the +/// most fragile: it holds only at WARN, so a revert to `fail-on: ERR` drops it, +/// and without this case the test would stay green while it did. +fn add_a_response_enum_value(spec: &str) -> String { + once( + spec, + "enum: [INITIALIZED, PAIRING, CONNECTED, CANCELLED, DONE, TIMEOUT]", + "enum: [INITIALIZED, PAIRING, CONNECTED, CANCELLED, DONE, TIMEOUT, EXPIRED]", + ) +} + +/// `key` is the IBE user secret key: the payload the endpoint exists to return, +/// and optional only because it is absent until the session is `DONE`. +fn remove_optional_response_property(spec: &str) -> String { + once( + spec, + r##" key: + description: | + Opaque pg-core-serialized IBE user secret key. Present only when + `status` is `DONE` and `proofStatus` is `VALID`. +"##, + "", + ) +} + +fn rename_optional_response_property(spec: &str) -> String { + once( + spec, + " proofStatus:\n $ref: \"#/components/schemas/ProofStatus\"\n key:\n", + " proof_status:\n $ref: \"#/components/schemas/ProofStatus\"\n key:\n", + ) +} + +fn remove_required_response_property(spec: &str) -> String { + once( + spec, + r##" KeyResponse: + type: object + required: [status] + properties: + status: + $ref: "#/components/schemas/SessionStatus" +"##, + " KeyResponse:\n type: object\n properties:\n", + ) +} + +fn narrow_a_parameter_type(spec: &str) -> String { + once( + spec, + TIMESTAMP_PARAMETER, + &TIMESTAMP_PARAMETER.replacen("format: int64", "format: int32", 1), + ) +} + +fn remove_a_request_parameter(spec: &str) -> String { + once( + spec, + &format!(" parameters:\n{TIMESTAMP_PARAMETER}"), + "", + ) +} + +type Mutation = (&'static str, fn(&str) -> String, Gate); + +fn mutations() -> Vec { + vec![ + ("a new endpoint", add_endpoint, Gate::Passes), + ( + "a new optional response property", + add_optional_response_property, + Gate::Passes, + ), + ( + "a new optional request property", + add_optional_request_property, + Gate::Passes, + ), + ( + "a new required response property", + add_required_response_property, + Gate::Passes, + ), + ( + "a new optional query parameter", + add_optional_query_parameter, + Gate::Passes, + ), + ("a new response status", add_response_status, Gate::Passes), + ("an edited description", edit_a_description, Gate::Passes), + ( + "a /v3 route beside /v2", + add_v3_route_beside_v2, + Gate::Passes, + ), + ("a removed route", remove_route, Gate::Stops), + ( + "a request property becoming required", + request_property_becomes_required, + Gate::Stops, + ), + ("a changed status code", change_a_status_code, Gate::Stops), + ( + "a removed non-success status", + remove_a_non_success_status, + Gate::Stops, + ), + ( + "a removed response enum value", + remove_a_response_enum_value, + Gate::Stops, + ), + ( + "a new response enum value", + add_a_response_enum_value, + Gate::Stops, + ), + ( + "a removed optional response property", + remove_optional_response_property, + Gate::Stops, + ), + ( + "a renamed optional response property", + rename_optional_response_property, + Gate::Stops, + ), + ( + "a removed required response property", + remove_required_response_property, + Gate::Stops, + ), + ( + "a narrowed parameter type", + narrow_a_parameter_type, + Gate::Stops, + ), + ( + "a removed request parameter", + remove_a_request_parameter, + Gate::Stops, + ), + ] +} + +/// The verdicts below are only CI's verdicts if CI runs the engine with these +/// flags, and nothing else compares the two: `the_gate_stops_...` skips on every +/// runner, so a `fail-on` edited down to `ERR` in the workflow alone would land +/// green. This test needs no engine, so it runs in CI, where it is the only +/// thing standing between the workflow and the promises this file makes about +/// it. +#[test] +fn the_workflow_step_matches_the_pinned_inputs() { + let path = workflow_path(); + let workflow = + fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + + assert_eq!( + workflow_input(&workflow, "fail-on"), + FAIL_ON, + "the workflow's oasdiff step and FAIL_ON disagree, so the verdicts this \ + test pins are not the ones CI reaches" + ); + assert_eq!( + workflow_input(&workflow, "include-checks"), + INCLUDE_CHECKS, + "the workflow's oasdiff step and INCLUDE_CHECKS disagree, so the \ + verdicts this test pins are not the ones CI reaches" + ); + assert!( + workflow.contains(ACTION_REF), + "the workflow's oasdiff step does not use {ACTION_REF}, so CI is not \ + running the engine (tufin/oasdiff:v1.26.1) the verdicts this test pins \ + were measured against" + ); +} + +/// Every mutation must still edit the spec, whether or not oasdiff is +/// installed, so a spec edit that strands an anchor is caught in CI too. +#[test] +fn every_mutation_still_applies() { + let spec = fs::read_to_string(spec_path()).expect("read the spec"); + for (name, mutate, _) in mutations() { + assert_ne!( + mutate(&spec), + spec, + "the mutation for {name} changed nothing, so whatever it asserts is vacuous" + ); + } +} + +#[test] +fn the_gate_stops_breaking_changes_and_passes_additive_ones() { + let Some(oasdiff) = oasdiff() else { + eprintln!( + "skipping: oasdiff is not installed, which is the case on every runner. To run this \ + test, `go install github.com/oasdiff/oasdiff@v1.26.1` (the version the action pins), \ + or set OASDIFF." + ); + return; + }; + + let spec = fs::read_to_string(spec_path()).expect("read the spec"); + let dir = env::temp_dir().join(format!("pg-pkg-api-gate-{}", std::process::id())); + fs::create_dir_all(&dir).expect("create the scratch directory"); + + // The unmutated spec first: without this, a gate that stopped everything + // would satisfy every Stops case below and only look half broken. + let unchanged = dir.join("unchanged.yaml"); + fs::write(&unchanged, &spec).expect("write the spec"); + let baseline = gate(&oasdiff, &unchanged); + + let mut wrong = Vec::new(); + if baseline != Gate::Passes { + wrong.push(" no change at all: the gate should pass it, it stopped it".to_owned()); + } + for (name, mutate, expected) in mutations() { + let slug: String = name + .chars() + .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) + .collect(); + let revision = dir.join(format!("{slug}.yaml")); + fs::write(&revision, mutate(&spec)).expect("write the mutated spec"); + let actual = gate(&oasdiff, &revision); + if actual != expected { + wrong.push(format!( + " {name}: the gate should {} it, it {} it", + expected.verb(), + actual.past() + )); + } + } + + fs::remove_dir_all(&dir).ok(); + assert!( + wrong.is_empty(), + "the gate's verdict on {} of {} changes is not what fail-on={FAIL_ON} and \ + include-checks={INCLUDE_CHECKS} are supposed to deliver:\n{}", + wrong.len(), + mutations().len() + 1, + wrong.join("\n"), + ); +}