Skip to content

ci: cargo-semver-checks gate for the pg-core and pg-wasm public APIs - #270

Open
dobby-coder[bot] wants to merge 8 commits into
mainfrom
ci/253-cargo-semver-checks
Open

ci: cargo-semver-checks gate for the pg-core and pg-wasm public APIs#270
dobby-coder[bot] wants to merge 8 commits into
mainfrom
ci/253-cargo-semver-checks

Conversation

@dobby-coder

@dobby-coder dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Closes #253. Part of #248 (workstream D of #247).

Adds a cargo-semver-checks gate over the two surfaces external consumers build against, so a breaking public-API change cannot land without being declared.

What is in the diff

scripts/semver-checks.sh, which runs two checks and reports both before exiting:

crate invocation baseline
pg-core --only-explicit-features --features test,rust,stream crates.io 0.6.1
pg-wasm --target wasm32-unknown-unknown, RUSTFLAGS=--cap-lints=warn origin/main

scripts/semver-checks-test.sh covers the script's exit-code contract and its two environment variables. It stubs cargo, so it needs neither cargo-semver-checks nor a wasm32 toolchain and runs in 0.04 seconds.

And one line of .gitignore: pg-wasm is under the root manifest's exclude, so --manifest-path pg-wasm/Cargo.toml resolves it on its own and leaves an untracked lockfile next to the manifest. Now that running the gate is a routine step, a later git add -A would otherwise commit a file the repo does not track.

The semver-checks job that calls the script is in a comment below rather than in the diff: dobby-coder[bot] has no workflows: write on this repo, so a push touching .github/workflows/ is rejected at the remote. Until a maintainer applies that patch, nothing here gates a PR; the script has to be run by hand.

Which exit code means what

cargo-semver-checks splits its failures across two exit codes, and the distinction is the whole safety of the gate:

exit meaning
100 a real semver violation
101 the tool or the build failed: unresolvable baseline rev, missing rustup target, registry fetch failure, compile error in the crate

Only 100 may be reported as a breaking change. The advice a gate prints for a break is "declare it", which on this repo means a ! in the PR title and a major release of pg-core cut by release-plz. Printing that for a build failure would produce a wrong title, a spurious major bump, and a check that stays red anyway, because --release-type major leaves a 101 non-zero. So run_check records a crate only on 100 and lets every other exit out with the tool's own code.

The feature quirk, and how it reaches semver-checks

cargo test --workspace failing does not carry over: semver-checks documents the library, not the test harness, so pg-core's test-gated test items never have to compile. The feature list does carry over, for a different reason.

Left to itself, cargo-semver-checks enables every feature that does not look unstable. On pg-core that includes web, and the crate refuses to build:

error: "web" feature should only be enabled on wasm32 targets
  --> pg-core/src/client/web/mod.rs:19:1

So the script spells the set out, the same one the Test workspace and Clippy workspace matrices use. test is in it because the module it gates is public API: pg-compat and pg-wasm's dev-dependency both build against pg_core::test.

pg-core's web,stream surface is left out on purpose. On that combination Unsealer has two unseal methods 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 across versions by name alone. Checking the pg-core-v0.6.1 tag against the published 0.6.1 crate, byte-identical sources, reports:

--- failure method_receiver_mut_ref_became_owned: method receiver changed from mutable reference to owned value ---
  pg_core::client::Unsealer::unseal now takes Self, not &mut Self, in pg-core/src/client/web/mod.rs:187

The rust,stream set does not trip it, because there both unseal methods take an owned receiver. pg-wasm covers the surface those web consumers actually call, so the gap is the pg-core-level web API only.

Two things pg-wasm needs

It is not on crates.io (only @e4a/pg-wasm on npm, versioned from pg-core), so there is no registry baseline and the script compares against origin/main. That needs fetch-depth: 0 in the job.

It also has to be built for wasm32, and that hits a cargo bug. cargo-semver-checks puts --cap-lints allow in RUSTFLAGS, which silences the "dropping unsupported crate type" warnings that cargo reads back when it probes rustc for wasm32 target info; cargo then aborts:

error: output of --print=file-names missing when learning about target-specific information from rustc

Setting RUSTFLAGS ourselves keeps cargo-semver-checks from adding its own cap, and the probe goes through. Both workarounds are commented in the script.

Declaring a break

release-plz owns the version numbers here, so the PR that makes a breaking change is not the PR that bumps the version, and cannot be. Bumping pg-core on its own does not even resolve:

error: failed to select a version for the requirement `pg-core = "^0.6.1"`
candidate versions found which didn't match: 0.7.0
required by package `pg-cli v0.3.6`

What produces the bump is the conventional-commit ! in the PR title, which release-plz reads. So that is what the gate accepts as the declaration, and the only thing it accepts: the job matches ^type(scope)!: on the title and passes SEMVER_RELEASE_TYPE=major, which the script turns into --release-type major. edited is already in this workflow's trigger list, so adding the ! re-runs the gate. Without it, the check derives the release type from the version numbers, sees no bump, and fails on anything breaking.

A BREAKING CHANGE: footer in the PR body is deliberately not accepted. This repo's squash_merge_commit_message is COMMIT_MESSAGES, so the body never reaches the squashed commit, and release-plz reads commits. Accepting a body-only footer let a PR titled fix(pg-core): tidy up the unsealer set --release-type major, go green, and then get published as 0.6.2, a patch release breaking every pg-core = "0.6" consumer. Write the footer anyway where it helps the changelog; the gate simply does not read it.

Two properties of the merge settings matter when you declare a break, neither introduced here:

  • squash_merge_commit_title is COMMIT_OR_PR_TITLE, which GitHub resolves to the PR title on a multi-commit PR but to the commit's subject when the PR has exactly one commit. The gate reads the PR title in both cases, so on a single-commit PR the ! belongs in the commit subject too. Written down in the CLAUDE.md bullet.
  • --release-type major does not merely permit a larger bump, it leaves nothing to check. Every lint exists to demand a bump the declaration already grants, so all of them skip: Checked [0.000s] 0 checks: 0 pass, 253 skip on each surface, measured here against cargo-semver-checks 0.49.0. And it reaches both invocations, so a ! declared for a pg-wasm break also passes an unrelated pg-core break in the same PR. Inherent to the flag; recorded in the script header and CLAUDE.md so a reviewer reading a green gate on a ! PR knows it checked nothing.

The title is passed to the step through the environment, not interpolated into the shell.

Verification

Run on aarch64 with cargo-semver-checks 0.49.0, rustc 1.97.1.

scripts/semver-checks-test.sh, 28 assertions, all passing:

both surfaces clean
pg-core reports a semver violation (100)
both surfaces report a semver violation (100)
pg-core fails to build (101)
pg-wasm baseline does not resolve (101)
pg-wasm fails to build (101) with SEMVER_RELEASE_TYPE=major
pg-core violation (100) plus pg-wasm build failure (101)
an unrecognised exit code is not swallowed
SEMVER_RELEASE_TYPE reaches both invocations
the default run passes no --release-type
SEMVER_WASM_BASELINE reaches the pg-wasm invocation
run from outside a git repository

all assertions passed

Eleven of those fail against 23701f5~1, on three counts: a 101 was reported as a breaking change, its exit code was flattened to 1, and a run from outside a git repository exited 0 with No undeclared breaking public-API changes. after checking relative manifest paths against whatever the CWD happened to be. The pg-core violation (100) plus pg-wasm build failure (101) case is new in b5f3dcb and fails against b5f3dcb~1, where the recorded pg-core break was discarded:

pg-core violation (100) plus pg-wasm build failure (101)
  ok   propagates the tool's exit code
  FAIL still reports the recorded break: output does not contain 'Breaking public-API changes in: pg-core'

Against real cargo-semver-checks, clean tree, both surfaces green, 66 seconds cold:

==> pg-core (native, features test,rust,stream)
     Checked [   0.038s] 196 checks: 196 pass, 57 skip
     Summary no semver update required
==> pg-wasm (wasm32-unknown-unknown, baseline origin/main)
     Checked [   0.028s] 196 checks: 196 pass, 57 skip
     Summary no semver update required
No undeclared breaking public-API changes.

Then with a break planted in each crate (pub const VERSION_V1 made pub(crate), StreamUnsealer::public_identity renamed), reverted afterwards:

--- failure pub_module_level_const_missing: pub module-level const is missing ---
  VERSION_V1 in file pg-core-0.6.1/src/consts.rs:7
--- failure inherent_method_missing: pub method removed or renamed ---
  StreamUnsealer::public_identity, previously in file pg-wasm/src/lib.rs:259

Breaking public-API changes in: pg-core pg-wasm
SCRIPT EXIT: 1

Same tree with SEMVER_RELEASE_TYPE=major exits 0, checking nothing on either surface:

==> pg-core (native, features test,rust,stream)
     Checked [   0.000s] 0 checks: 0 pass, 253 skip
==> pg-wasm (wasm32-unknown-unknown, baseline origin/main)
     Checked [   0.000s] 0 checks: 0 pass, 253 skip
No undeclared breaking public-API changes.

An unresolvable baseline exits 101 instead:

$ SEMVER_WASM_BASELINE=refs/heads/does-not-exist ./scripts/semver-checks.sh
error: couldn't parse revision: "refs/heads/does-not-exist^{tree}"
cargo-semver-checks failed on pg-wasm (exit 101): tool or build
failure, not a semver break. Adding '!' to the PR title will not
clear this.
SCRIPT EXIT: 101

And the two together, a planted pg-core break with pg-wasm's baseline unresolvable, which is what b5f3dcb changed: the break is named before the script leaves with the tool's code.

==> pg-core (native, features test,rust,stream)
     Checked [   0.011s] 196 checks: 195 pass, 1 fail, 0 warn, 57 skip
     Summary semver requires new major version: 1 major and 0 minor checks failed
==> pg-wasm (wasm32-unknown-unknown, baseline refs/heads/does-not-exist)
error: couldn't parse revision: "refs/heads/does-not-exist^{tree}"

Breaking public-API changes in: pg-core
Either keep the change additive, or declare the break: give the PR a
conventional-commit title with '!' (e.g. feat(pg-core)!: ...), which is
what release-plz reads to cut a major release.

cargo-semver-checks failed on pg-wasm (exit 101): tool or build
failure, not a semver break. Adding '!' to the PR title will not
clear this.
SCRIPT EXIT: 101

cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream and cargo fmt --all -- --check both pass, and a full gate run now leaves a clean git status.

The workflow patch itself has not run; the job cannot exist until a maintainer applies it. Its YAML parses and the semver-checks job resolves to seven steps with shell: bash on every run:, the diff applies clean against build.yml at b5f3dcb, and the release tarball's SHA-256 matches the digest pinned in the step. The declaration step's logic was exercised on eight titles: feat(pg-core)!:, fix!: and chore(deps)!: emit release_type=major; feat:, ci:, fix(pg-core): tidy up the unsealer (the case from the blocker), feat(pg-core)! : and a title that is itself BREAKING CHANGE: emit nothing. The step no longer reads PR_BODY at all, so a body footer cannot affect the outcome.

Not fixed here

cargo doc -p pg-wasm fails on a broken intra-doc link (Unsealer::inspect_header at pg-wasm/src/lib.rs:211, denied by the crate's rustdoc::broken_intra_doc_links). It does not block this gate (cargo-semver-checks caps lints while generating rustdoc JSON), so it is left alone rather than folded into a CI change.

scripts/semver-checks.sh runs cargo-semver-checks over the two surfaces
external consumers build against and fails on a breaking change that the
PR has not declared. The workflow job that calls it ships as a patch in a
PR comment; the bot cannot push .github/workflows.

Refs #253.
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Workflow patch for a maintainer to apply

dobby-coder[bot] has no workflows: write on this repo, so the job that calls scripts/semver-checks.sh cannot be in the diff. Save the diff below to a file and git apply it onto this branch before merging, or the script ships with nothing calling it.

Updated for b5f3dcb. The change from the previous version is the declaration step: the BREAKING CHANGE: footer branch and the PR_BODY env entry are gone, so the title ! is the only declaration. Cycle 2 was right that the two are not equivalent here. I confirmed the setting myself:

$ gh api repos/encryption4all/postguard -q '{squash_title: .squash_merge_commit_title, squash_message: .squash_merge_commit_message}'
{"squash_message":"COMMIT_MESSAGES","squash_title":"COMMIT_OR_PR_TITLE"}

COMMIT_MESSAGES keeps the PR body out of the squashed commit, and release-plz reads commits, so a body-only footer would have set --release-type major, turned the gate green, and left release-plz cutting 0.6.2 for a breaking change. The step now reads PR_TITLE and nothing else. Exercised on eight titles: feat(pg-core)!:, fix!: and chore(deps)!: emit release_type=major; feat:, ci:, fix(pg-core): tidy up the unsealer, feat(pg-core)! : and a title that is literally BREAKING CHANGE: emit nothing.

GitHub cannot anchor an inline comment to a path outside the diff, so four notes on the YAML itself:

  • fetch-depth: 0 is load-bearing. pg-wasm has no crates.io release, so its baseline is origin/main, and cargo-semver-checks clones that revision. Dropping it used to make the gate report a breaking change in pg-wasm; since 23701f5 it fails as the tool error it is (exit 101).
  • The Test the gate script step stubs cargo, so it runs before the toolchain install and finishes in well under a second. It is what pins the 100-vs-101 exit-code mapping the gate depends on, now including the case where a 101 on one surface must not discard a 100 already found on the other.
  • The install step is a pinned release tarball plus its SHA-256, rather than a fourth third-party action. taiki-e/install-action@<sha> with tool: cargo-semver-checks does the same job and takes the digest maintenance off your hands, if you would rather have that. Digest re-verified against the v0.49.0 x86_64 release: sha256sum gives 72f6834d...40e4a, matching the pin.
  • The PR title reaches the script through env:, never through ${{ }} interpolation inside a run: block, so a crafted title cannot inject shell. Every run: block declares shell: bash, which is what gets pipefail.

One thing this does not close, called out in the cycle-2 review and now written into CLAUDE.md rather than into the job: squash_merge_commit_title is COMMIT_OR_PR_TITLE, which resolves to the commit's subject when a PR has exactly one commit. The gate reads the PR title in both cases, so on a single-commit PR the ! has to be in the commit subject too, or the gate goes green off the title while release-plz cuts a patch. Closing that in the job is possible: the step could compare github.event.pull_request.commits against the head commit's subject and fail when they disagree. I left it out because it is YAML I cannot run in CI here, and the failure mode is narrow. Say the word and I will add it.

Verified against b5f3dcb: git apply --check is clean, the YAML parses, and the semver-checks job resolves to seven steps with shell: bash on every run:.

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 1e0be8b..f49320d 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -192,3 +192,102 @@ jobs:
       - name: Clippy pg-compat
         if: steps.changes.outputs.wire == 'true'
         run: cargo clippy --manifest-path pg-compat/Cargo.toml --all-targets --locked -- -D warnings
+
+  # ---------------------------------------------------------------------------
+  # Public-API semver gate (#253). Fails a PR that breaks pg-core's or pg-wasm's
+  # public API without declaring the break.
+  #
+  # Versions here are bumped by release-plz, not by the PR that makes the
+  # change, so "declared" means the conventional-commit `!` marker that
+  # release-plz turns into a major bump. The marker is read off the PR title
+  # into SEMVER_RELEASE_TYPE; `edited` is already in this file's trigger list,
+  # so adding the `!` to the title re-runs the gate.
+  #
+  # Same always-reports shape as wire-compat-rust: the path filter is a step,
+  # not an `on: paths:` key, so a required check never sits pending.
+  # ---------------------------------------------------------------------------
+  semver-checks:
+    name: Public API semver
+    if: github.event_name == 'pull_request'
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      pull-requests: read # paths-filter reads the PR's changed files
+    steps:
+      # pg-wasm is not published to crates.io, so its baseline is origin/main
+      # rather than a registry version, and the full history has to be here.
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+
+      - uses: dorny/paths-filter@7b450fff21473bca461d4b92ce414b9d0420d706 # v4.0.2
+        id: changes
+        with:
+          filters: |
+            api:
+              - 'pg-core/**'
+              - 'pg-wasm/**'
+              - 'Cargo.toml'
+              - 'Cargo.lock'
+              - 'scripts/semver-checks.sh'
+              - 'scripts/semver-checks-test.sh'
+              - '.github/workflows/build.yml'
+
+      # Stubs cargo, so it needs no toolchain and runs in under a second. It
+      # covers the 100-vs-101 exit-code mapping the gate depends on: 100 is a
+      # semver violation, 101 is the tool or the build failing.
+      - name: Test the gate script
+        if: steps.changes.outputs.api == 'true'
+        shell: bash
+        run: ./scripts/semver-checks-test.sh
+
+      - if: steps.changes.outputs.api == 'true'
+        uses: dtolnay/rust-toolchain@stable
+        with:
+          targets: wasm32-unknown-unknown
+
+      # Pinned release binary rather than `cargo install` (a five-minute build)
+      # or a fourth third-party action. Bump the version and the digest together.
+      - name: Install cargo-semver-checks
+        if: steps.changes.outputs.api == 'true'
+        shell: bash
+        env:
+          VERSION: 0.49.0
+          SHA256: 72f6834d75d28a66e02c9fd6a230ce901bb30eee6067b85867a97445df040e4a
+        run: |
+          curl -sSfL -o "$RUNNER_TEMP/cargo-semver-checks.tar.gz" \
+            "https://github.com/obi1kenobi/cargo-semver-checks/releases/download/v${VERSION}/cargo-semver-checks-x86_64-unknown-linux-gnu.tar.gz"
+          echo "${SHA256}  $RUNNER_TEMP/cargo-semver-checks.tar.gz" | sha256sum -c -
+          tar -xzf "$RUNNER_TEMP/cargo-semver-checks.tar.gz" -C "$HOME/.cargo/bin" cargo-semver-checks
+
+      # The PR title, not the commit subject: PRs are squash-merged here, so the
+      # title is what release-plz and the Conventional Commit check read. Passed
+      # through the environment rather than interpolated into the script, so a
+      # PR title cannot inject shell.
+      #
+      # The title `!` is the only accepted declaration. A `BREAKING CHANGE:`
+      # footer in the PR *body* is deliberately not accepted: this repo's
+      # squash_merge_commit_message is COMMIT_MESSAGES, so the body never reaches
+      # the squashed commit, and release-plz reads commits. Honouring a body-only
+      # footer would pass the gate on a `fix(pg-core):` PR that release-plz then
+      # publishes as a patch release of a breaking change.
+      - name: Read the breaking-change declaration off the PR title
+        id: declared
+        if: steps.changes.outputs.api == 'true'
+        shell: bash
+        env:
+          PR_TITLE: ${{ github.event.pull_request.title }}
+        run: |
+          if [[ "$PR_TITLE" =~ ^[a-zA-Z]+(\([^\)]*\))?!: ]]; then
+            echo "Breaking change declared in the PR title; a major bump is allowed."
+            echo "release_type=major" >> "$GITHUB_OUTPUT"
+          else
+            echo "No '!' in the PR title; any breaking change will fail the gate."
+          fi
+
+      - name: Check the public API
+        if: steps.changes.outputs.api == 'true'
+        shell: bash
+        env:
+          SEMVER_RELEASE_TYPE: ${{ steps.declared.outputs.release_type }}
+        run: ./scripts/semver-checks.sh

@dobby-coder
dobby-coder Bot requested a review from rubenhensen July 28, 2026 11:30

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: request-changes

Rules Dobby 2 — consolidated review (cycle 1)

Rule sweep: 14 rules selected as plausibly applicable to a bash-script + CLAUDE.md diff on a Rust repo (patch-in-comment/CI, cargo-semver-checks, workflow-trigger and path-filter shape, prose, PR hygiene). No rule violations found. Two things I checked and confirmed clean rather than assumed:

  • The YAML in the patch comment is compliant with the two workflow rules it could plausibly trip. Every run: block declares shell: bash, so the echo … | sha256sum -c - pipe in the install step actually gets pipefail. The path filter is a step with per-step if, not an on: paths: key, so the job always reports and can become a required check. edited is already in build.yml's pull_request types, so the PR-title escape hatch re-runs the gate — I verified that against the file on the branch, not just the PR body's claim.
  • CI is fully green with nothing in flight (all 4 crates × test/fmt/clippy, both Docker arches, all three browser lanes, wire-compat, Conventional Commit).

One handed-forward nit I am dropping, and the coder should not "fix" it: the CLAUDE.md line saying the gate is NOT yet a CI gate: the semver-checks job for build.yml sits in the patch comment on PR #270 and needs a maintainer to apply it. That is not transient state leaking into durable docs — it is verbatim what the binding patch-in-comment-gate-is-not-live-yet rule prescribes while the patch is unapplied, and I confirmed the PR HEAD tree still has no .github/workflows/ change. Rewording it to present tense is the exact defect that rule exists to prevent. What the rule does require is tightening it back to present tense in this same PR once a maintainer commits the patch onto the branch, which the patch comment already promises.

What needs fixing

One bug, three nits. I reproduced all of them on 62cf526 with cargo-semver-checks 0.49.0 rather than taking them on trust; the exit-code one is the only blocker.

The blocker is that the script treats every non-zero exit from cargo-semver-checks as a breaking API change. Exit 100 means a semver violation; exit 101 means the tool or the build failed. I confirmed both on this branch: a planted pubpub(crate) on VERSION_V1 gives 100, and an unresolvable baseline rev gives 101. || failed+=(…) on lines 60 and 74 collapses them, so a dropped fetch-depth: 0, a missing wasm32 target, a transient crates.io fetch failure, or any future compile error in pg-core all print Breaking public-API changes in: … and advise adding a ! to the title. That advice is actively harmful: it makes release-plz cut a spurious major release of pg-core, and I verified it does not even turn the check green, because the tool still exits 101 with --release-type major passed.

The nits are non-blocking, but they are one-liners and the branch is already open, so they may as well ride along.

Comment thread scripts/semver-checks.sh Outdated
Comment thread scripts/semver-checks.sh Outdated
Comment thread scripts/semver-checks.sh Outdated
Comment thread scripts/semver-checks.sh Outdated
dobby-coder Bot added 3 commits July 28, 2026 12:09
cargo-semver-checks answers 100 for a semver violation and 101 for the
tool or the build failing. The script treated every non-zero exit as a
breaking change, so an unresolvable baseline rev, a missing wasm32
target, a registry fetch failure or a compile error all printed
"Breaking public-API changes in: ..." and advised adding a `!` to the PR
title. On this repo that makes release-plz cut a spurious major release
of pg-core, and it does not even clear the check: --release-type major
leaves a 101 non-zero.

run_check now records only 100 and lets anything else out with the
tool's own exit code.

Two shell fixes ride along. `cd "$(git rev-parse --show-toplevel)"` did
not abort outside a git repository, because the substitution fails but
`cd ""` succeeds: the gate went on to check relative manifest paths
against whatever the CWD happened to be, and reported success. And
`"${release_type[@]}"` on an empty array aborts under `set -u` on bash
before 4.4, which is macOS system bash.

scripts/semver-checks-test.sh covers the exit-code mapping, the two
environment variables and the outside-a-repo abort. It stubs cargo, so
it needs neither cargo-semver-checks nor a wasm32 toolchain and runs in
under a second. Eleven of its assertions fail against the previous
script.
pg-wasm is under the root manifest's `exclude`, so running the gate with
`--manifest-path pg-wasm/Cargo.toml` resolves it on its own and leaves an
untracked lockfile next to the manifest. Now that running the gate is a
routine step, a later `git add -A` would commit a file the repo does not
track.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: request-changes

Rules Dobby 2 — consolidated review (cycle 2)

Rule sweep: 12 rules selected against a bash + CLAUDE.md + .gitignore diff on a Rust repo (CI gate shape, the cargo-semver-checks traps, patch-in-comment docs accuracy, prose, PR and release hygiene), run as three Haiku batches with the rule text and the diff embedded. No rule violations.

I re-verified all four cycle-1 blockers against the branch rather than trusting the report:

  • exit 100 and exit 101 are now separated in run_check, and 101 leaves with the tool's own code.
  • root=$(git rev-parse --show-toplevel) then cd "$root", so a run outside a repo aborts instead of exiting 0 with a success message.
  • ${release_type[@]+"${release_type[@]}"} for the empty-array expansion under set -u on bash 3.2.
  • pg-wasm/Cargo.lock is gitignored, with the reason written next to it.

scripts/semver-checks-test.sh passes on this branch, 25 assertions, and I reproduced the exit-code mapping myself by running the gate against a stubbed cargo. CI is green on ae446bf with nothing in flight: all four crates across test/fmt/clippy, pg-compat lint, wire-compat, three browser lanes, both Docker arches, Conventional Commit.

One blocker remains, and it is in the workflow patch rather than the diff, so GitHub cannot anchor it as an inline comment.

Blocker: the PR-body escape hatch opens a hole through the gate

In the Read the breaking-change declaration off the PR step (patch lines 106-107), the declaration accepts a BREAKING CHANGE: footer in the PR body as an alternative to the ! in the title:

if [[ "$PR_TITLE" =~ ^[a-zA-Z]+(\([^\)]*\))?!: ]] \
  || grep -qE '^BREAKING[ -]CHANGE:' <<<"$PR_BODY"; then

On this repo those two are not equivalent. I checked the merge settings: squash_merge_commit_message is COMMIT_MESSAGES, not PR_BODY. So on a squash merge the PR body never reaches the commit message, and release-plz reads commits. A footer that exists only in the PR body is invisible to it.

I ran the step's exact logic on three cases. A PR titled fix(pg-core): tidy up the unsealer whose body contains BREAKING CHANGE: pg_core::consts::VERSION_V1 is now private. emits release_type=major:

=== case 1: no ! in title, footer ONLY in PR body ===
Breaking change declared on the PR; a major bump is allowed.
OUTPUT: [release_type=major]
=== case 2: no ! and no footer (control) ===
OUTPUT: []

That sets SEMVER_RELEASE_TYPE=major, which the script passes as --release-type major. The gate then goes green. release-plz, reading a bare fix(pg-core): subject, cuts a patch bump, 0.6.1 to 0.6.2, published to crates.io as a compatible release that breaks every downstream pg-core = "0.6" consumer. That is the exact outcome this gate exists to prevent, and the escape hatch is what produces it.

CLAUDE.md already states the rule the other way round: the ! is what the semantic-PR-title check and release-plz both key on. The fix is to drop the || grep -qE '^BREAKING[ -]CHANGE:' <<<"$PR_BODY" branch and the PR_BODY env entry, so the title ! is the single declaration mechanism, matching what release-plz actually reads.

Note the mismatch runs the other way too, harmlessly: a footer in a commit message with no ! in the title fails the gate while release-plz would cut a major.

One thing to confirm while making that change. squash_merge_commit_title on this repo is COMMIT_OR_PR_TITLE, which GitHub documents as the commit's title when the PR has a single commit and the PR title when it has more than one. So dropping the body branch makes the gate and release-plz agree on multi-commit PRs, and on a single-commit PR only when that commit's subject also carries the !. Worth a sentence in the CLAUDE.md bullet, since the gate reads the PR title in both cases. I am not treating this as a second blocker; it is a property of the repo setting rather than something this PR introduced.

Dropped from the handover

Nothing this cycle. Both nits below are carried forward as non-blocking, same as cycle 1 judged them.

Non-blocking nits

Two, both on scripts/semver-checks.sh, left inline. Neither hides a break: the first costs a round trip, the second is a docs-accuracy point about what a green gate on a ! PR actually checked.

Comment thread scripts/semver-checks.sh Outdated
Comment thread scripts/semver-checks.sh
Comment thread scripts/semver-checks.sh Outdated
dobby-coder Bot and others added 2 commits July 28, 2026 12:49
The gate's contract said the declaration was the conventional-commit `!`
in the PR title "plus the BREAKING CHANGE: footer", and the advice it
printed on a break told the author to supply both. The workflow step that
feeds it read those as alternatives and accepted a footer in the PR body
on its own.

Those are not equivalent on this repo. `squash_merge_commit_message` is
COMMIT_MESSAGES, so the PR body never reaches the squashed commit, and
release-plz reads commits: a PR titled `fix(pg-core): ...` with a footer
only in its body sets --release-type major, turns the gate green, and
gets a patch release of a breaking change published to crates.io. The
title `!` is the marker release-plz actually reads, so it is the only one
the gate accepts. The footer stays useful changelog prose.

Also in the script:

- report the violations recorded so far before exiting on a tool or build
  failure. A 101 on pg-wasm used to discard a 100 already found on
  pg-core, so fixing the baseline and re-running was the only way to
  learn about the break.
- write down what --release-type major does: it does not permit a larger
  bump, it leaves nothing to check (`0 checks: 0 pass, 253 skip`,
  measured on both surfaces), and it reaches both invocations, so a `!`
  declared for one crate passes an unrelated break in the other.

CLAUDE.md gains the same, plus the other half of the merge settings:
`squash_merge_commit_title` is COMMIT_OR_PR_TITLE, which is the commit's
subject when a PR has exactly one commit, so a single-commit PR needs the
`!` in the commit subject too.

Refs #253
The bullet said "Three things it encodes" and then listed four: the
exit-code item (4) was appended in ae446bf without updating the count.

Dropped the five bold spans from the two new bullets as well. Five in 528
words is 9.5 per 1,000, against the org writing rules' cap of 1 per 1,000
and the 1-in-1,287 the rest of this file sits at. The exit codes read as
`100` and `101` in backticks instead; the other three needed no emphasis.

Refs #253

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: approve

Rules Dobby 2 — consolidated review (cycle 3)

Rule sweep: 11 rules selected against a bash + CLAUDE.md + .gitignore diff on a Rust repo (the cargo-semver-checks traps, workflow shell semantics, patch-in-comment docs accuracy, CI gate shape, prose, PR and release hygiene), run as three Haiku batches with the rule text, both scripts and the workflow YAML pasted into each prompt. No rule violations. Review Dobby 2 handed over zero findings on the ae446bf..b5f3dcb delta.

Cycle 2's blocker is closed. I checked the fix rather than the report: the declaration step in the patch comment now reads PR_TITLE only, and the || grep -qE '^BREAKING[ -]CHANGE:' <<<"$PR_BODY" branch and the PR_BODY env entry are both gone. The repo setting that made those two non-equivalent is squash_merge_commit_message: COMMIT_MESSAGES, which I confirmed against the API myself.

What I verified rather than assumed

  • The 101-discards-100 nit from cycle 2 is fixed. report_breaks is now a function called from both the tail of the script and the early-exit path in run_check, with the tool-failure message printed last. A planted pg-core break plus an unresolvable pg-wasm baseline names the break before leaving with 101.
  • scripts/semver-checks-test.sh passes on this branch: 28 assertions, including the new pg-core violation (100) plus pg-wasm build failure (101) case, and the outside-a-repo case asserts on cargo never being invoked rather than only on the exit status.
  • Both shell traps are handled: root=$(git rev-parse --show-toplevel) then cd "$root", and ${release_type[@]+"${release_type[@]}"} for the empty-array expansion under set -u on bash 3.2.
  • Every run: block in the patch comment declares shell: bash, so the echo … | sha256sum -c - pipe in the install step gets pipefail. The default bash -e {0} would swallow it.
  • pg-wasm/Cargo.lock is untracked and gitignored, with the reason next to it.
  • The docs stay honest about the gate not existing yet. git grep -n semver .github/workflows/ at HEAD returns nothing, and CLAUDE.md says so in the same breath, which is what patch-in-comment-gate-is-not-live-yet asks for. Once a maintainer commits the patch onto this branch, that wording has to be tightened to present tense in this same PR.
  • Cycle 2's COMMIT_OR_PR_TITLE caveat is now written into CLAUDE.md, as asked.
  • CI is green on b5f3dcb: 26 checks pass, 4 skipped release and publish jobs, nothing failing. cargo fmt --all -- --check is clean.

Two nits, both fixed on the branch as 56feaac

Neither hides a break, and neither was worth another coder cycle, so I committed them directly.

  1. CLAUDE.md said "Three things it encodes" and then listed four. The exit-code item (4) was appended in ae446bf without updating the count.
  2. The two new CLAUDE.md bullets carried five bold spans in 528 words, or 9.5 per 1,000. The org writing rules cap bold at 1 per 1,000, and the rest of this file sits at 1 in 1,287, so it also broke the file's own idiom. The exit codes now read as 100 and 101 in backticks; the other three needed no emphasis. The single em dash I left alone: at 1.9 per 1,000 it matches the 2.3 per 1,000 already in the file.

Dropped from the handover

The see .github/workflows/build.yml pointer in the script header names a job that no workflow file defines yet. I am not filing it: the line immediately above it in CLAUDE.md says the job is unapplied and names the patch comment, and the pointer becomes correct the moment a maintainer applies it. Rewriting it now is the churn that patch-in-comment-gate-is-not-live-yet exists to prevent.

One thing for the maintainer, not a finding

Nothing in this PR gates anything until the semver-checks job from the patch comment is applied to .github/workflows/build.yml. dobby-coder[bot] has no workflows: write here, so that step needs a human. Until then scripts/semver-checks.sh has to be run by hand, which is what CLAUDE.md tells the next run to do.

Signing off. Loop limit of three cycles reached, and there is nothing left blocking.

@dobby-coder
dobby-coder Bot marked this pull request as ready for review July 28, 2026 13:19
Dobby's patch from the #270 comment, applied verbatim (the App cannot push workflows). Without it scripts/semver-checks.sh ships with nothing calling it.
@rubenhensen

Copy link
Copy Markdown
Contributor

Workflow patch applied verbatim onto this branch, so the job now actually calls scripts/semver-checks.sh. Please re-review the applied commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: cargo-semver-checks for pg-core and pg-wasm

1 participant