API compatibility: check the whole module, and fail on an incompatible change
Two defects in the api-compat job, and the second is what let the first survive. Review then found three more, and the third of those was evidence about the approach rather than another bug: the step is now apidiff -m over the module instead of a hand-rolled loop over its packages.
Scope
apidiff compares one package at a time, so the literal ./pkg/basecamp in this step was the gate's scope. Every other importable package in the module — pkg/basecamp/oauth, pkg/basecamp/otel, pkg/basecamp/prometheus, pkg/types, pkg/generated — has never been checked, and pkg/basecamp/eventfeed was about to enter the module the same way.
The gate is now the whole module: apidiff -m -w exports each revision, and -m compares them. A package added later is covered the day it appears rather than the day someone remembers to widen a literal, a package removed by the PR is classified incompatible, and a package added is classified compatible.
Failure
The step printed "Breaking Changes Detected" and passed anyway (# Don't fail the build, just warn). It now exits non-zero, with the existing breaking label ("Breaking change to public API") as the sanctioned override. The label suppresses the failure, never the report — the finding still prints in full. Without a hatch the gate could not block at all: this SDK is pre-1.0 and removes surface on purpose (#504 withdrew GetEverythingBoosts), and a hard fail with no override gets switched off the first time it is right.
set -eu, so a failed export aborts instead of leaving a missing .api file for the comparison to read as "nothing changed".
Verification — the widening is load-bearing
Driven against real compiling mutants, with the pre-change step reconstructed alongside. Recorded against 5a6d7cdc9; the widening it measures is unchanged since, and the module-mode rewrite is re-proved separately below.
| Scenario | This PR | Today |
|---|---|---|
| Control — no change | pass | — |
| New package added | pass, reported additive | — |
Break in pkg/basecamp | FAIL | warn, then pass |
Break in pkg/basecamp/oauth | FAIL | pass — "No breaking changes detected." |
Break in pkg/basecamp/eventfeed | FAIL | pass |
| Package removed | FAIL | invisible |
Break + breaking label | pass, finding still printed | — |
| Package does not compile | FAIL, loudly | silent / degraded export |
The oauth and eventfeed rows are the proof the widening is load-bearing: the identical break, in identical trees, is invisible to the gate as it stands.
The "does not compile" row was found by accident — the first oauth mutant renamed a method and its call site diverged. It exited 1 for the right verdict but the wrong reason, so that run was discarded as a vacuous kill and redone with a compiling mutant.
Sequencing
Lands before the event-feed foundations PR. Row 2 is why that is safe: a new package cannot be incompatible with a base that never had it, and the gate says so explicitly instead of staying silent.
The comparator could fail and the gate would pass (found in review)
The section above says a failed export must not be read as "nothing changed". The comparison had precisely that hole, and it is the more dangerous one, because it is the step that renders the verdict.
apidiff has three outcomes and this gate distinguished two:
| outcome | exit | output |
|---|---|---|
| clean | 0 | none |
| differences found | 0 | stdout |
| could not run | non-zero | stderr |
Both call sites tested apidiff ... && [ -s report ]. set -e does not fire for a command used as an if condition, so a non-zero apidiff short-circuited the &&, the branch was skipped, breaking stayed 0, and the step printed "No breaking changes detected." and exited 0. A merge gate failing open.
It was also silent: the comparator's stderr was redirected into a report file that is only ever printed on the differences path, so the reason was captured and then discarded. That is why it survived review.
Red proof
The comparison region was extracted verbatim from the run: block and driven against a stubbed apidiff. Before the fix, the first two rows are byte-identical reports:
########## HEAD (then-current, unfixed) — apidiff stub: tool-failure ##########
## API Compatibility Check
No breaking changes detected.
### All API Changes
No API changes detected in packages present in both revisions.
REAL_EXIT=0
apidiff stub | before | after |
|---|---|---|
tool failure, -incompatible site | No breaking changes detected. — exit 0 | names it a tool failure, prints the swallowed stderr — exit 1 |
| tool failure, all-changes site | No API changes detected… — exit 0 | same, exit 1 |
| clean | pass | unchanged |
| differences found | reported | unchanged |
A tool failure is reported as a tool failure and explicitly not as something the breaking label should wave through — the label answers "this break is intended", not "the toolchain is broken". Both comparisons still go through one helper after the rewrite, since the way this returns is the two drifting apart.
internal/… is not compared
An internal package has no external consumer to break — the same reason an added package is compatible by construction. This is not only tidiness: apidiff refuses to compare an internal package, says so on stderr, and exits 0. When the comparisons folded stderr into the report and read any non-empty report as a finding, the first internal package to exist in both revisions would have been announced as a breaking change and failed the gate:
>>> FALSE BREAKING reported for example.com/intmod/internal/secret:
Ignoring internal package example.com/intmod/internal/secret
Latent today (7 packages, none internal), and a false failure on something that cannot break anyone is how a breaking label ends up on a non-break — or how the gate ends up switched off.
The exclusion is now apidiff -m's own (filterInternal), and the report no longer merges stderr into stdout, so that message cannot reach the report at all. Separating the streams is what demotes the internal exclusion from a load-bearing requirement back to a nicety.
Module mode replaces the hand-rolled loop (found in review)
Three review rounds, three defects, all of them in the per-package machinery this step wrapped around apidiff and none in the policy around it. A fourth was sitting there unfound: two import paths whose underscore slugs collide — pkg/event/feed and pkg/event_feed both slug to pkg_event_feed — overwrite each other's export file, and a real incompatible change in one of them is silently missed.
Red proof
Against 1a179e76f, with the PR breaking Subscribe in pkg/event/feed:
########## RED: HEAD — slug collision (pkg/event/feed vs pkg/event_feed) ##########
## API Compatibility Check
No breaking changes detected.
### All API Changes
No API changes detected in packages present in both revisions.
REAL_EXIT=0
After:
### Breaking Changes Detected
- ./pkg/event/feed.Subscribe: changed from func(string) to func(string, int)
REAL_EXIT=1
None of that machinery had to be owned here. The pinned apidiff build has module mode, priced against a two-revision scratch module across every case:
| Case | -m module mode | hand-rolled loop |
|---|---|---|
| Clean change | exit 0, empty stdout | exit 0 |
| Incompatible change | exit 0, finding on stdout | exit 0, same finding |
| Added package | - package …: added, compatible | reported "additive, not compared" |
| Removed package | - package …: removed, incompatible | hand-written package-set check |
internal/ in both, changed incompatibly | not compared, not reported | not compared (needed an awk filter) |
| Tool failure | exit 1, reason on stderr | exit 1, reason on merged stream |
| Slug collision | correct | fail-open |
Gone with it: the go list ./... enumeration, the internal/ filter, the slugged filenames, the empty-directory prune, the package-set removal check, the added-package report, and both comparison loops. Kept, because it is policy rather than mechanism: the comparator-failure handling and the live label read.
Two things -m does not do, both handled. It writes Ignoring internal package … to stderr on every comparison, so the streams are kept apart. And it keys packages by their path relative to the module, so a module whose path changed compares as though nothing moved and reports nothing at all; that break is checked explicitly against go list -m on each side.
GOWORK=off for both exports: the repository root carries a go.work over three modules, so a bare go list -m prints all three and apidiff -m is handed a three-line "module path". It also measures the module as a consumer resolves it — its own go.mod and go.sum — and the two spellings export byte-identical data today.
The two exports no longer straddle a moving base (found in review)
The PR side is the checked-out refs/pull/N/merge commit. The base side was origin/${GITHUB_BASE_REF}, a moving ref that fetch-depth: 0 refreshes at job start, so the two could straddle a base that had advanced in between — and then a package the base gained reads as one this PR removed. Reading the breaking label live widened the window further, by making "label it and re-run" a normal flow that can happen long after the merge commit was cut.
Red proof
Against 1a179e76f, with a PR that changes nothing and a base that gains a package after the merge commit is cut:
########## RED: HEAD (1a179e76f) — case drift, PR changes NOTHING, base gained a package ##########
## API Compatibility Check
### Package removed: `example.com/mod/pkg/lateaddition`
::error::Incompatible public API changes. If they are intentional, label this PR `breaking` and re-run this job…
REAL_EXIT=1
After:
Comparing 11128b01e (this PR merged onto its base) against 12c6da22e (that base).
## API Compatibility Check
No breaking changes detected.
REAL_EXIT=0
The base is now the merge commit's own first parent, so both sides come from one fixed point. Verified against this PR's real merge ref rather than assumed:
$ git rev-list --parents -n 1 refs/pull/776/merge
ff245408e 0ef7fe38e 1a179e76f
^ base ^ this PR's head
A guard fails the step if HEAD is not a merge commit, so changing the checkout to fetch the branch head cannot silently narrow the gate to "did the last commit break the API".
The base revision is exported from git worktree add under RUNNER_TEMP, which retires the checkout dance with it: no checking the base over this tree, no removing the files the PR added, no removing the directories that left empty, no restoring — and no path by which this step leaves go/ dirty under the manifest guard that runs immediately after it.
The label residual, accepted deliberately
Removing breaking after a green run leaves that run green. Closing it needs a label-event trigger: labeled/unlabeled on this workflow re-runs all 23 Test jobs on every label event on every PR (and labeler.yml emits label events automatically on every push that moves the changed-path set), while splitting this gate into its own workflow file makes that one job but renames the required check, which needs a branch-protection change by an admin.
Accepted, because the residual is narrow: breaking is not in .github/labeler.yml, so no automation can remove it; and removing it by hand buys nothing, since the label acknowledges a break rather than approving a merge — anyone who can remove it can equally leave it on and merge. It is not free either: .github/release.yml files breaking PRs under "⚠️ Breaking Changes". The acknowledged-pass message now says the run is only as good as the label at the moment it read it, and the step records the trip-wire — revisit if breaking becomes automated, or if a merge queue lands.
Case matrix
Stub-driven against the extracted run: block on a real two-revision git repo with a real merge-shaped commit, because the label paths never execute in this PR's own CI (breaking=0).
| Case | exit |
|---|---|
| clean | 0 |
| incompatible, unlabelled | 1 |
| incompatible, labelled | 0 |
| added package | 0 |
| removed package | 1 |
internal/ changed incompatibly | 0 |
| slug collision | 1 |
| tool failure at comparison | 1 |
| tool failure at export | 1 |
| label API read fails, with a break | 1 |
| no PR number, with a break | 1 |
| HEAD not a merge commit | 1 |
| base drifted, PR changed nothing | 0 |
End-to-end against the real module at this PR's merge ref: clean, REAL_EXIT=0, worktree left clean. Two mutations, restored by cp + diff -q:
(*AccountService).GetAccountgains a parameter →- ./pkg/basecamp.(*AccountService).GetAccount: changed from func(context.Context) (*Account, error) to func(context.Context, int) (*Account, error),REAL_EXIT=1FlexInt.MarshalJSONgains a parameter →- ./pkg/types.FlexInt.MarshalJSON: changed from func() ([]byte, error) to func(int) ([]byte, error),REAL_EXIT=1
The second is the point of the PR: pkg/types is outside the gate entirely on main.
actionlint, zizmor and shellcheck on the extracted step, all under LC_ALL=C: REAL_EXIT=0 each.
Summary by cubic
Expands the API compatibility gate to the entire Go module and hard-fails on incompatible changes unless the PR is labeled breaking. Previously it checked only ./pkg/basecamp, warned, and passed.
- Compares module-to-module with
apidiff -m; added packages are additive, removed packages are breaking;internal/...is ignored byapidiff. - Detects a module path change and treats it as breaking (not visible to
apidiff -m). - Pins the baseline to the merge commit’s first parent and exports it from a detached worktree; sets
GOWORK=offto avoid workspace contamination. - Treats
apidifftool failures as errors and prints stderr; keeps stdout/stderr separate to prevent false positives. - Produces a single “All API Changes” report at module scope (includes package adds/removes).
- Reads the
breakinglabel live via the GitHub API; requirespull-requests: read. If the label read fails, the job fails and asks to re-run. - Required action: For intentional public API breaks, label the PR
breakingand document the change in the changelog.