ci: breaking-change gate on the pg-pkg OpenAPI contract (oasdiff)

[dobby-coder]

Adds the oasdiff breaking-change gate for pg-pkg/api-description.yaml from #249, mirroring the one that landed in encryption4all/cryptify#201.

.github/workflows/api-diff.yml is in the diff because @rubenhensen applied it in 8f26cf7; the App has no workflows: write here, so the YAML started life in a comment below. The rest of the diff is pg-pkg/tests/api_gate.rs, which pins what the gate's settings actually deliver, and the COMPATIBILITY.md rule the WARN setting adds.

Two deviations from the spec on #249

fail-on is WARN, not ERR. The request pinned fail-on: ERR. I measured what that gives us and it is not what the issue asks for ("removed route/field, type change, new required field"), so the gate ships at WARN. Reverting is a one-line change; the table below is the measurement.

Only one of the two escape hatches is implemented. #249 asks for "a new versioned route with the old one left running, or an explicit override label". This ships the first and drops the second. A label that bypasses a required check is a footgun: it makes the gate advisory for anyone who can apply a label, and the whole point of #249 is that the gate becomes a required check for #247. Because pg-pkg's routes are versioned, /v3 is a real escape hatch rather than a formality, which is what makes dropping the label affordable here (cryptify, with unversioned routes, has no such option). Worth a maintainer's explicit yes or no, since it is a narrowing of the brief and not just a setting. If you want the label anyway, it is if: "!contains(github.event.pull_request.labels.*.name, 'api-break-approved')" on the job, and it needs your hands: the App has no workflows: write.

Measured against oasdiff v1.26.1 on the real spec, with the flags the action's entrypoint builds. ERR+inc is fail-on: ERR with the two opt-in checks named:

change	ERR	ERR+inc	WARN+inc
route removed	fails	fails	fails
request property becomes required	fails	fails	fails
parameter type narrowed	fails	fails	fails
required response property removed	fails	fails	fails
401 becomes 403	passes	fails	fails
response enum value dropped	passes	fails	fails
response enum value added	passes	passes	fails
401 response removed	passes	fails	fails
optional response property removed (key)	passes	passes	fails
optional response property renamed	passes	passes	fails
required path parameter removed	passes	passes	fails

Three things came out of this:

Two checks are opt-in. response-non-success-status-removed and response-property-enum-value-removed rate ERR but only run when named, so include-checks is not optional for us.

The rest are WARN, and on this spec that is most of the rule. COMPATIBILITY.md forbids "removing a route or a field". Only status is ever required in this spec, so key (the IBE user secret key /v2/request/key 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. At ERR the response side of that rule is close to unguarded.

WARN is nearly a restatement of COMPATIBILITY.md. It adds 31 checks on top of ERR's 213. Of the 31, exactly one fires on something the document did not already forbid: response-property-enum-value-added. I ran nine additive changes through WARN and that was the only one flagged; the other eight (new endpoint, new optional response property, new optional request property, new required response property, new optional query parameter, new response status, an edited description, and a /v3 route beside /v2) all pass. Rather than suppress it, COMPATIBILITY.md now names a new response enum value as non-additive, because it is: a client that switches on SessionStatus without a default branch breaks on a value it has never seen, which is a /v3 change.

If you would rather keep the brief exactly, set fail-on: ERR and flip the four Gate::Stops expectations in api_gate.rs that stop holding. The test will tell you which: the three optional-field removals in the table above, plus a new response enum value, which is the rule COMPATIBILITY.md gains in this PR and the one that only WARN enforces.

The regression test

pg-pkg/tests/api_gate.rs mutates the spec 19 ways and asserts, for each, whether the gate stops it. It runs the engine with the flags the action's entrypoint builds (--allow-external-refs=false --include-checks … --composed=false --fail-on …), so its verdict is CI's verdict.

On the old settings it fails and names every gap:

the gate's verdict on 7 of 20 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 removed request parameter: the gate should stop it, it passed it

On the new ones it passes. It is wired into a job that already exists rather than a new one: CI runs cargo test --manifest-path pg-pkg/Cargo.toml --all-features, which picks it up. The verdict test needs the oasdiff binary, which no runner has, so it skips there with a message saying how to install it. Two tests run regardless: every_mutation_still_applies, so a spec edit that strands one of the anchors still fails in CI, and the_workflow_step_matches_the_pinned_inputs, which reads fail-on and include-checks back out of the workflow and fails when they no longer match the constants the verdicts were measured against. Locally, with go install github.com/oasdiff/[email protected], all three do the real work.

Unchanged from cryptify's copy

review: false so nothing is uploaded to oasdiff.com, no path filter so the job always reports a status and can become a required check, base pinned to pull_request.base.sha.

Three deltas from cryptify's copy

• The trigger is types: [opened, synchronize, reopened, edited]. A base retarget fires edited, which is not in the default set, so without it a verdict computed against the old base stays attached to the unchanged head sha and still reports green. That happened on ci: add the oasdiff breaking-change gate cryptify#201 itself: docs: true up api-description.yaml with the mounted routes cryptify#200 merged, GitHub retargeted, no run fired, and the green came from an unrelated force-push twenty seconds later. build.yml already carries this trigger for the same reason.
• The action is sha-pinned (0ab8ad2 with the tag in a trailing comment), like dorny/paths-filter in build.yml. This step is the verdict.
• persist-credentials: false on both checkouts. The oasdiff action is a third-party container with the workspace mounted, and the spec is all it reads.

cryptify's gate has the same two gaps, for the same reason: it was written before any of this was measured, and it has neither include-checks nor a narrowed promise. Filed as encryption4all/cryptify#202.

Notes for the maintainer

• The gate is green on this PR, which does not touch the spec. It first ran after the merge conflict was resolved: a conflicted PR has no refs/pull/N/merge, so no pull_request workflow fired at all while the conflict stood.
• Making it a required check for EPIC: Fleet remediation — enforce the seams, kill silent failure (2026-07 audit) #247 needs branch protection, which the App can neither read nor set.
• allow-external-refs stays at its false default. Every $ref in the spec is internal, checked.
• Nothing needs pull-requests: write: with review: false the action never posts a comment.
• Two one-line YAML edits I cannot push (no workflows: write), neither of which changes behaviour:
  1. Add github-token: '' to the oasdiff step's with: block. The input defaults to ${{ github.token }}, so the job's token is handed to the third-party container as a positional argument for no reason. Confirmed in the run on this branch: the docker invocation carries -e "INPUT_GITHUB-TOKEN" and "***" as its last argument. Both post_review_comment call sites in the action's entrypoint at the pinned sha are guarded by [ "$review" != "false" ], so with review: false nothing reads it. It is the same reasoning as the persist-credentials: false two lines up, which is there because the workspace is mounted into that container.
  2. In the header comment, the "Reach for that before reaching for err-ignore" line points at err-ignore (a file of regexes) as the thing not to reach for, where ci: breaking-change gate on api-description.yaml (oasdiff) #249's wording was an override label. If you agree with dropping the label (see the deviations above), a clause saying so belongs in that comment, since the comment is where the next person looks.
• One thing the gate does not cover, now stated in both COMPATIBILITY.md and the workflow comment: it compares documented paths, and the spec documents the canonical /v2/request/... paths only, so dropping the /v2/irma/... alias handlers before shrink the promised surface: deprecate /v2/irma alias, drop dead format_version, unify x-postguard semantics #257's deprecation has run passes it. That stays a review rule.
• The docs lines calling the gate pending are all gone: 8f26cf7 tightened two of them, and the paragraph in COMPATIBILITY.md pointing at the YAML comment plus the same wording in api_gate.rs's header went with the conflict resolution.
• actions/checkout@v6 in the new file against @v4 throughout build.yml. Both resolve; worth aligning whenever you next touch either.
• Two review nits fixed on the branch after the conflict resolution. api_gate.rs now pins the action ref as well as fail-on/include-checks, because the ref is what selects the engine (v0.1.10 is FROM tufin/oasdiff:v1.26.1) that the verdicts were measured against, so a bump used to leave the suite green while the verdicts stopped describing CI. And root .gitattributes now marks the spec -text: the mutation anchors are multi-line raw strings, rustc normalises CRLF inside those but read_to_string does not, so a Windows checkout failed every_mutation_still_applies on a clean tree. CI is ubuntu/macos only and never saw it.

Refs #249, #242. Part of #247 (workstream C).

[dobby-coder]

.github/workflows/api-diff.yml (new file)

Updated in 56ce480: fail-on is now WARN and the two opt-in ERR checks are named. At ERR this gate passed six of the changes COMPATIBILITY.md forbids; the header comment explains which and why, and pg-pkg/tests/api_gate.rs asserts it.

The App has no workflows: write on this repo, so this needs a maintainer to commit it, either onto this branch (preferred, so the gate self-tests here and the two "not live yet" docs lines can tighten to present tense before merge) or onto main.

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

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

Apply with:

gh pr checkout 269
mkdir -p .github/workflows
# paste the YAML above into .github/workflows/api-diff.yml
git add .github/workflows/api-diff.yml
git commit -m 'ci: add the oasdiff breaking-change gate on the pg-pkg contract'
git push

Two things only a maintainer can do afterwards: add API breaking changes (oasdiff) to the required checks 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.

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 1. Rule sweep over 14 applicable rules plus Review Dobby 2's findings, all re-verified against HEAD 1593102.

One blocking issue: the gate does not catch three of the changes its own comment and COMPATIBILITY.md promise it catches. I reproduced this independently — built oasdiff v1.26.1 (the engine v0.1.10 ships) and mutated the real pg-pkg/api-description.yaml, using the step's exact flags:

mutation	--fail-on ERR, no include-checks	with include-checks
401 → 403 on GET /v2/request/key/{timestamp}	exit 0, no finding	response-non-success-status-removed ERR, exit 1
TIMEOUT dropped from SessionStatus	exit 0, no finding	response-property-enum-value-removed, 3 ERRs, exit 1
optional key removed from KeyResponse	exit 0 (response-optional-property-removed is WARN)	exit 1 only at --fail-on WARN

The workflow header comment promises "a changed status code for a condition a client already handles" and "a route or field removed or renamed". Neither holds today. The first two are one input away — add to the oasdiff step:

          fail-on: ERR
          include-checks: response-non-success-status-removed,response-property-enum-value-removed
          review: false

The third needs a decision rather than a flag, and it is the one worth a maintainer's attention: in this spec only status is ever required, so key, proofStatus, pubSignKey, privSignKey and the rest are all optional, and removing any of them is WARN. key is the IBE user secret key — the payload the endpoint exists to return. Two honest options:

• Raise to fail-on: WARN. That is 31 more checks, and most are things COMPATIBILITY.md already forbids (request-parameter-removed, response-body-media-type-schema-removed). Some are noisy.
• Keep ERR and narrow the promise to "a removed route, or a removed/renamed required response field" in both the workflow comment and COMPATIBILITY.md.

Either way the two places that currently overpromise should match what the gate actually does.

Also worth fixing in the same push (non-blocking)

• Two bare cross-repo refs in the PR body (cross-repo-link-format). Line 27 reads "That happened on cryptify#201 itself: #200 merged" — cryptify#201 does not auto-link across repos, and the bare #200 resolves to postguard#200, a real unrelated PR ("feat(pkg): proxy IRMA /statusevents SSE endpoint"), so it links a reader to the wrong thing. Use encryption4all/cryptify#201 and encryption4all/cryptify#200.
• The alias gap deserves one clause in CLAUDE.md. The gate only sees paths the document lists, and the spec deliberately documents canonical paths only (its own "Path prefix aliases" note, lines 16-20 — I confirmed no /v2/irma path appears anywhere in the spec). So dropping the /v2/irma/... handlers, the alias COMPATIBILITY.md promises keeps working until #257's deprecation runs, passes the gate silently. The enforcement paragraph sits directly under the alias paragraph, which reads as though it covers it.
• The pending wording is correct now and goes stale on apply. patch-in-comment-gate-is-not-live-yet requires the tightening to happen in this same PR once a maintainer commits the YAML onto the branch — the PR body already lists it as a pre-merge TODO, which is the right call. Flagging it so it does not merge with the pending tense baked in.

What the sweep cleared

review: false set and the action sha-pinned with the tag in a trailing comment (oasdiff-action-uploads-specs-by-default — the sha pin is stronger than a tag pin and matches dorny/paths-filter in build.yml); no on: paths: filter, so the job always reports and can be a required check (path-filtered-job-cannot-be-a-required-check); edited present in the trigger, which the base-sha-dependent verdict needs (pull-request-default-types-exclude-edited); no run: blocks, so no pipefail trap; allow-external-refs at its SSRF-guarding default; reviewer assigned; conventional-commit title green; repo knowledge in the repo's CLAUDE.md rather than shared rules. Refs #249 rather than Closes #249 is right here — merging this does not make the gate live.

Tests: not run, and correctly so — the committed diff is two markdown files. Repo CI is 23 pass / 0 fail.

[dobby-coder]

VERDICT: approve

Rules Dobby 2, cycle 2. Reviewed HEAD ffc5bd4, then pushed f0edf44 with the two nit fixes below. No blocking findings, so this is signed off and flipped out of draft.

Cycle 1's blocking finding is closed

The gate now delivers what its comment and COMPATIBILITY.md promise. I rebuilt the measurement rather than reading it off the PR body: installed oasdiff v1.26.1 locally and replayed all ten breaking mutations from api_gate.rs with the exact flags the action's entrypoint builds. At fail-on: ERR plus include-checks, three pass that should not (a removed optional response property, a renamed one, a removed request parameter). At WARN plus include-checks all ten fail. Running cargo test --test api_gate with the real engine on PATH: both tests pass, all 19 mutations plus the unmutated baseline.

Rules checked

14 rules apply to this diff. All compliant.

oasdiff-action-uploads-specs-by-default is the one with teeth here, and the YAML satisfies every part of it: review: false, action sha-pinned with the tag in a trailing comment, allow-external-refs left at its default, both opt-in checks named, and the tier pinned in a committed test rather than only in the workflow.

patch-in-comment-gate-is-not-live-yet: CLAUDE.md, COMPATIBILITY.md and the test header all say the gate is not running yet and name the comment it waits in. The PR body also commits to tightening those lines in this PR once a maintainer applies the YAML, which is what the rule asks for.

path-filtered-job-cannot-be-a-required-check: no on: paths: filter, and the comment says why.

pull-request-default-types-exclude-edited: edited is in the trigger, and the job does read pull_request.base.sha.

actions-default-shell-has-no-pipefail: no run: blocks, so there is no shell to guard. actionlint 1.7.7 is clean on the YAML after my edit to it.

ci-gate-can-ride-an-existing-build-script: the test rides Test workspace (pkg), which the matrix runs with --all-features. Confirmed against build.yml rather than taken from the PR body.

rust-run-cargo-fmt-before-push: cargo fmt and cargo clippy --all-targets --all-features -- -D warnings both clean on pg-pkg, before and after my commit.

writing-rules: zero banned words, zero em dashes in the added prose, three bold spans across 2945 added words.

Also checked and clean: conventional-commit-pr-titles (the repo runs the job and it passes), cross-repo-link-format, single-repo-lessons-belong-in-that-repos-claude-md, grep-a-construction-site-before-user-visible-claim, tests-required-on-fixes, promised-vs-delivered. No advisory or embargo is touched, so the ready flip is safe.

Two nits, both fixed on the branch

Neither blocks, so instead of sending this back I fixed them in f0edf44 and made the surrounding claims match.

The first is a missing mutation for the newest rule. Nothing covered adding a response enum value, which is the one rule COMPATIBILITY.md gains in this PR and the only one WARN alone enforces. Measured on the real spec: adding EXPIRED to SessionStatus exits 0 at ERR+include-checks and exits 1 at WARN+include-checks. So the revert path the PR body offers dropped the newest rule while the test stayed green, which is the drift the test exists to prevent. api_gate.rs now pins it as a 19th mutation.

The second is the revert sentence's count. It said four Gate::Stops expectations stop holding at ERR; before this commit it was three, and the workflow comment and CLAUDE.md both said three. Adding the enum case makes four correct, so I kept the number and named which four. I also re-measured the quoted failure output for the old settings: it is now 7 of 20, not 6 of 19.

Adding the 19th case moved two numbers written before it existed, so those moved too: CLAUDE.md's "18 ways" to 19, and in the workflow comment the "three of the changes COMPATIBILITY.md forbids" paragraph to four, with the enum row added to its table. The gate inputs in that comment are byte-identical to what cycle 1 reviewed; only comment lines changed.

For the maintainer

Nothing above changes what you need to do. Paste the YAML from the comment onto this branch, then the two "not live yet" doc lines tighten to present tense before merge, and API breaking changes (oasdiff) can go into branch protection for #247.

[rubenhensen]

Workflow applied onto this branch (so the gate self-tests here, as you suggested), and both doc lines tightened to present tense: COMPATIBILITY.md no longer says "written but not running yet", and CLAUDE.md no longer points at a spent PR comment.

Your fail-on: ERR finding travels — encryption4all/cryptify's gate has the same setting, which I landed in cryptify#201 before this analysis existed. Filed as encryption4all/cryptify#202 and linked under the epic, with the instruction to redo your mutation-table measurement against that spec rather than assume the same answer. Its exposure is lower (34 required: occurrences vs this spec's one, so removed required properties do rate ERR there) but the optional-property and request-parameter gaps are spec-independent, and cryptify's routes are unversioned so there is no /v3 escape hatch.

[rubenhensen]

/dobby fix conflicts and review if the workflow is implemented as intended

[dobby-coder]

Got it — I'm on PR #269 now. I'll resolve the merge conflicts, then review the changes to check the workflow is implemented as intended and push fixes if needed.

[dobby-coder]

Conflicts resolved and the workflow reviewed against the action's source. Branch is mergeable, all 26 checks pass, and the gate ran for the first time.

The conflict, and why no check had ever reported

One hunk: the link-reference block at the bottom of COMPATIBILITY.md, where main's [#262]/[#268] and this branch's [#269] landed on the same line. Resolved by keeping main's two and dropping [#269] along with the paragraph that used it, which said the workflow "sits in a comment on #269 until a maintainer applies it" and was left behind by 8f26cf7. api_gate.rs's header said the same thing; also dropped. pg-compat-js/test/manifest.test.mjs parses that file and still passes.

The conflict is why the gate had not run. GitHub cannot build refs/pull/269/merge for a conflicted PR, so nothing fired on 8f26cf7: not the new job, not build.yml, nothing at all. gh pr checks 269 answered "no checks reported on the branch", which reads like a missing job rather than like an unmergeable PR. First real run after the merge passed in 19s.

What I verified about the step

Against oasdiff/oasdiff-action at the pinned sha, not against its docs:

• 0ab8ad204b00d25acc5ae87106281433e288d0c1 is the commit tagged v0.1.10.
• Every input the step passes exists in that tag's breaking/action.yml. No typo silently defaulting.
• breaking/Dockerfile is FROM tufin/oasdiff:v1.26.1, so the version CLAUDE.md tells you to install locally is the engine CI runs.
• breaking/entrypoint.sh builds --allow-external-refs=false --include-checks <checks> --composed=false plus --fail-on WARN. The job log confirms it verbatim, and it is what gate() in api_gate.rs runs, so the test's verdicts are CI's verdicts.
• review: false returns before both the --open upload and post_review_comment, so no spec leaves CI and nothing is posted on the PR. permissions: contents: read is therefore sufficient; the step's github-token default is never reached.
• No run: blocks, so the missing-pipefail trap does not apply.
• The PR checkout is refs/pull/269/merge and the base checkout resolved base.sha to c4ce6e9, main's tip at event time. Both ends are what the header claims.

One gap, closed

api_gate.rs's header named a hole that stopped being theoretical once the YAML was committed. FAIL_ON and INCLUDE_CHECKS are what the test runs oasdiff with, the step is what CI runs it with, and nothing compared the two. The verdict test skips on every runner, so fail-on edited down to ERR in the workflow alone lands green with the whole suite passing. That is the exact failure the WARN analysis exists to prevent.

the_workflow_step_matches_the_pinned_inputs now reads both values back out of the YAML. It needs no engine, so unlike the verdict test it runs in CI. Checked it has teeth: with fail-on: ERR in the workflow it fails with left: "ERR", right: "WARN".

94e6ed5 dropped the gate's CLAUDE.md entry

The "Update branch" merge hit a conflict where main's two semver-checks bullets and this branch's two oasdiff bullets both sat under the Docker one, and took main's side whole. Both oasdiff bullets were gone from CLAUDE.md on the merge result: the api-description.yaml one and the settings one that records why WARN. Restored unchanged, next to the semver pair.

While there, one correction to flag rather than bury, since it is your sentence: the bullet now says a merge that has to touch build.yml is rejected for the App and has to be landed by a maintainer. ce0fc59 on this branch is the App pushing exactly that, and it went through. Its diff against its first parent adds main's 64 new build.yml lines. What it did not do is resolve anything inside build.yml; it took main's side whole, so the blob it committed already existed in the repo, and that may be the line the remote actually draws. I added the measurement and left your sentence standing. Revert the addition if you would rather keep the stricter rule.

Not changed

The step uses actions/checkout@v6; build.yml is on @v4 throughout. Both resolve, and the App cannot push workflow edits anyway, so this is a note for whenever you next touch the file rather than a request.

[dobby-coder]

Two things after the last push, both now settled.

The applied YAML is byte-identical to what was in the comment. Checked rather than assumed: extracted the fenced block from comment 5103413183 and diffed it against .github/workflows/api-diff.yml at HEAD. 107 lines, no difference. So the review above, which reads the file, is also a review of what was proposed.

The three red wasm jobs were a flake, and the branch is green now. Same sha e86c71b ran CI twice, once from the push and once from my PR-body edit (build.yml carries the edited trigger). The 07:45 run passed all three browsers; the 07:48 run failed. Only chrome actually failed, on [SEVERE]: Timed out receiving message from renderer: 30.000; firefox and safari were cancelled by the fail-fast matrix and reported as failures. Error: missing field 'chunk', the line the log leads with, is just the test runner choking on the truncated webdriver reply, so the error names the wrong culprit. Written up in CLAUDE.md with both run ids, including that dobby-coder gets a 403 on rerun-failed-jobs and cannot clear such a run itself.

All 26 checks pass on 6aedefd. Nothing here needs a rebase.

[dobby-coder]

VERDICT: approve

Rules Dobby 2, cycle 3. Reviewed HEAD 6aedefd, then pushed 3ff6d8e with two nit fixes. No blocking findings. All 26 checks pass on the new sha, and the PR is already out of draft.

Is the workflow implemented as intended?

Yes, with two deviations from #249, and until this cycle the PR body only admitted one.

The gate itself does what its comment and COMPATIBILITY.md say it does. Cycle 1's blocking finding (the prose promised more than fail-on: ERR delivered) was closed in cycle 2 by re-measuring against the real engine at WARN plus the two opt-in checks. Nothing since has touched api-diff.yml or the pinned verdicts: the delta from cycle 2 is 8f26cf7 (@rubenhensen applying the YAML), 44d6379 (the inputs test), two base merges, and two docs commits. The gate is green and the job reports on every PR, so it can go into branch protection for #247.

The second deviation is the one worth a decision from you. #249 asks for "a new versioned route with the old one left running, or an explicit override label". Only the first is implemented. Dropping a label bypass on a required check is defensible, and /v3 is a real escape hatch here because pg-pkg's routes are versioned, but it is a narrowing of the brief that the body did not mention. It does now.

The conflict resolution

Clean, and I checked it by patch-content identity rather than by reading the hunks. The branch's contribution before the merge and after it are the same +/- lines with one exception: 94e6ed5 took main's side of CLAUDE.md whole and so dropped all three of this branch's new bullets. e86c71b restored them, and both are present at HEAD. Worth recording because that is the failure mode of that merge, and nothing in the conflict hunks would have shown it.

git diff against the base is 769 lines, so this is not a no-op PR. ce0fc59 is also the measured exception to the workflows: write block that the CLAUDE.md bullet now documents: a merge that takes the base's build.yml side whole is pushable by the App, because the blob it commits already exists in the repo.

Rules checked

15 rules apply. All compliant after the two fixes below.

oasdiff-action-uploads-specs-by-default is the one with teeth, and the step satisfies it: review: false, action sha-pinned with the tag in a trailing comment, allow-external-refs at its default, both opt-in checks named, fail-on: WARN, and the tier pinned in a test that reads the workflow file rather than in a bare constant.

patch-in-comment-gate-is-not-live-yet now cuts the other way: the YAML is applied, so present-tense docs are correct. COMPATIBILITY.md and the api_gate.rs header were tightened accordingly, which is what the rule asks for in the same PR.

path-filtered-job-cannot-be-a-required-check: no on: paths: filter, and the comment says why. pull-request-default-types-exclude-edited: edited is in the trigger, and the job does read pull_request.base.sha. actions-default-shell-has-no-pipefail: no run: blocks, so no shell to guard.

Also checked and clean: conflict-resolution-check-noop, verify-a-merge-by-patch-content-identity, workflow-file-conflict-is-unpushable-and-kills-ci, single-repo-lessons-belong-in-that-repos-claude-md, rust-run-cargo-fmt-before-push, ci-gate-can-ride-an-existing-build-script, tests-required-on-fixes, no-justification-paragraphs-for-simple-changes, writing-rules, never-self-merge-prs. No advisory or embargo is touched.

Two nits fixed on the branch

Neither blocks, so I fixed them rather than sending the PR back.

The action ref was not pinned (4334f9a). the_workflow_step_matches_the_pinned_inputs read fail-on and include-checks back out of the workflow but not the action ref, which is the third thing the verdicts depend on: breaking@0ab8ad2 is v0.1.10, whose Dockerfile is FROM tufin/oasdiff:v1.26.1, and every verdict in the file was measured against v1.26.1. So bumping the action left all three tests green while the verdicts quietly stopped describing CI, which is the fail-open the fail-on pin exists to prevent. Verified by bumping the ref in the workflow: the test now fails and names it.

The spec's line endings were not pinned (3ff6d8e). The mutation anchors are multi-line raw strings matched against the spec read off disk. rustc normalises CRLF to LF inside raw strings, but fs::read_to_string does not, so on a Windows checkout (core.autocrlf=true is the Git for Windows default) every multi-line anchor misses. Reproduced rather than argued: converting the spec to CRLF fails every_mutation_still_applies with anchor is not unique in the spec, which is the one gate test that runs without the external engine, so a Windows contributor gets a red suite on a clean tree with a message blaming the spec. CI is ubuntu/macos only and never sees it. Root .gitattributes now marks the spec -text. This is the same defect found on the sibling gate in encryption4all/cryptify#203.

cargo fmt --all -- --check clean, cargo clippy --all-targets --all-features -- -D warnings clean on pg-pkg, and the full pg-pkg suite passes (59 + 3 + 2).

Two nits left, both in the workflow file

Inline below. Both are one-line YAML edits that need your hands, since the App has no workflows: write on this repo. Neither changes behaviour, and neither blocks the merge.

Loop limit

This is cycle 3, so the PR is not going back to the coder regardless. Everything I could fix from a pushable file is fixed and verified; the remainder is the two inline YAML lines and the label question above, all of which are yours to decide.

[dobby-coder]

Nit, needs your hands (the App has no workflows: write): add github-token: '' to this with: block.

The input is left at its action default, which is ${{ github.token }}, so the job's GITHUB_TOKEN is handed to the third-party oasdiff container even though nothing uses it. Confirmed in the real run on this branch: the docker invocation carries -e "INPUT_GITHUB-TOKEN" and "***" as its last positional argument. Both post_review_comment call sites in the action's entrypoint at the pinned sha are guarded by [ "$review" != "false" ], so with review: false the token is never read.

That makes it a free removal, and it is the same threat model this step already reasons about two lines up, where persist-credentials: false is set on both checkouts because the workspace is mounted into that container. No behaviour change.

[dobby-coder]

Nit, and a question rather than a defect.

This line presents /v3 as the escape hatch while pointing at err-ignore as the thing not to reach for. err-ignore is a file of regexes; what #249 actually offered as the alternative to a versioned route was "an explicit override label", and this PR does not implement one.

Dropping the label is defensible: on a required check it makes the gate advisory for anyone who can apply a label, and because pg-pkg's routes are versioned, /v3 is a real alternative rather than a formality. But it is a second deviation from #249, and the PR body claimed only one until this review. I have added it to the body; if you agree with the choice, a clause saying so belongs here too, since this comment is where the next person hitting a red gate will look.

If you would rather have the label, it is if: "!contains(github.event.pull_request.labels.*.name, 'api-break-approved')" on the job, and it needs a maintainer commit for the same permissions reason.

[dobby-coder]

Done — conflicts were already cleanly resolved on the branch (verified by patch-content identity, which caught that main's merge had silently dropped three CLAUDE.md bullets; a follow-up commit restored them), so I reviewed the workflow instead of redoing that work.

Implemented as intended? Mostly. The gate does what its comment and COMPATIBILITY.md claim, and all 26 checks are green. There are two deviations from #249, though — one I've now documented in the PR body that wasn't called out before: only the versioned-route (/v3) escape hatch is implemented, not the override-label alternative #249 asked for. Dropping the label is defensible since /v3 is a real escape hatch here, but it's a narrowing of the brief worth your sign-off.

Along the way I fixed and pushed two nits: pinned the previously-floating oasdiff action ref in the gate test (it silently affected the verdicts), and added a root .gitattributes to stop a CRLF-driven red suite on Windows.

Posted the review as approve with two small inline YAML nits I can't push myself (no workflows: write) — both one-liners, noted on the PR. This was cycle 3, so it's not going back to the coder; it's ready for you:

#269