From bf6792f383e45af6392f44c3dd913f1ebd81ba0f Mon Sep 17 00:00:00 2001 From: westerberg Date: Mon, 10 Aug 2026 17:18:07 +0000 Subject: [PATCH 1/4] ci: guard the exported API against accidental breaking changes The build and the tests do not catch every break. Adding a trailing variadic parameter to an exported function changes that function's type: every ordinary call site still compiles, so nothing goes red, while any caller that held the function as a value stops compiling. That reached a release, and a test written specifically to pin backward compatibility did not catch it either, because it was an ordinary call expression. The check runs apidiff over every exported package and fails on incompatible changes. Adding to the API is always allowed. Two details that matter. It compares against the merge base rather than the target branch tip, because a branch cut before a recent change to the target would otherwise be reported as removing whatever the target gained in the meantime; the first draft did exactly that and produced two convincing false positives. And internal packages are excluded, since they are not public API. Verified end to end rather than assumed: run against the commit that introduced the variadics it reports both constructors and exits 1, and against the commit that restores them it exits 0 with no findings. --- .github/scripts/apidiff.sh | 77 +++++++++++++++++++++++++++++++++++ .github/workflows/apidiff.yml | 43 +++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100755 .github/scripts/apidiff.sh create mode 100644 .github/workflows/apidiff.yml diff --git a/.github/scripts/apidiff.sh b/.github/scripts/apidiff.sh new file mode 100755 index 000000000..7250aab95 --- /dev/null +++ b/.github/scripts/apidiff.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# Reports incompatible changes to the module's exported API. +# +# Compares every exported package against a base revision and fails when +# apidiff calls a change incompatible. Adding to the API is always allowed. +# +# Usage: apidiff.sh +# +# Why this exists: a trailing variadic parameter added to an exported function +# changes that function's type. Every ordinary call site still compiles, so +# neither the build nor the tests notice, while any caller holding the function +# as a value stops compiling. That shipped once. +set -euo pipefail + +TARGET_REF="${1:?usage: apidiff.sh }" + +# Compare against the merge base, not the target's tip. A branch cut before a +# recent change to the target would otherwise be reported as having removed +# whatever the target gained in the meantime, which is a false positive and the +# quickest way to get a check like this ignored. +BASE_REF="$(git merge-base HEAD "$TARGET_REF")" +echo "Target $TARGET_REF, comparing against merge base ${BASE_REF:0:12}" + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# Exported packages only. internal/ is not public API, and neither is a package +# with no importable identifiers. +packages() { + go list ./... | grep -v '/internal/' | grep -v '/internal$' +} + +echo "Collecting the API at $BASE_REF" +BASE_TREE="$WORK/base" +git worktree add --detach --quiet "$BASE_TREE" "$BASE_REF" +cleanup_worktree() { git worktree remove --force "$BASE_TREE" >/dev/null 2>&1 || true; } +trap 'cleanup_worktree; rm -rf "$WORK"' EXIT + +mkdir -p "$WORK/api" +( + cd "$BASE_TREE" + go work init >/dev/null 2>&1 || true + go work use -r . >/dev/null 2>&1 || true + for pkg in $(packages); do + # A package that does not build at the base has no baseline to compare + # against, which is normal for one this change introduces. + apidiff -w "$WORK/api/$(echo "$pkg" | tr '/' '_').api" "$pkg" >/dev/null 2>&1 || true + done +) + +echo "Comparing HEAD against it" +status=0 +for pkg in $(packages); do + snapshot="$WORK/api/$(echo "$pkg" | tr '/' '_').api" + [ -f "$snapshot" ] || continue # new package, nothing to break + + out="$(apidiff "$snapshot" "$pkg" 2>/dev/null || true)" + # apidiff prints an "Incompatible changes:" section only when there are some. + if printf '%s' "$out" | grep -q '^Incompatible changes:'; then + echo + echo "=== $pkg ===" + printf '%s\n' "$out" | sed -n '/^Incompatible changes:/,/^$/p' + status=1 + fi +done + +if [ "$status" -ne 0 ]; then + cat <<'MSG' + +Incompatible API changes found. + +If the break is intended, say so in the pull request description and a +maintainer can override this check. Note that adding a variadic parameter to an +existing exported function is a break even though call sites still compile. +MSG +fi +exit "$status" diff --git a/.github/workflows/apidiff.yml b/.github/workflows/apidiff.yml new file mode 100644 index 000000000..f711801cb --- /dev/null +++ b/.github/workflows/apidiff.yml @@ -0,0 +1,43 @@ +# Guards the exported API of the module against accidental breaking changes. +# +# The build and the tests do not catch every break. Adding a trailing variadic +# parameter to an exported function changes that function's type: every ordinary +# call site still compiles, so nothing here went red, while any caller holding +# the function as a value stops compiling. That reached a release once, which is +# why this check exists. +name: API diff + +on: + pull_request: + branches: [ "main", "v1" ] + + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + apidiff: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # apidiff compares against the merge base, which needs the history + # rather than the single commit a default checkout fetches. + fetch-depth: 0 + + - name: Setup + uses: ./.github/actions/setup + + - name: Initialize workspace + run: go work init && go work use -r . + + - name: Install apidiff + run: go install golang.org/x/exp/cmd/apidiff@v0.0.0-20250911091902-df9299821621 + + - name: Compare the exported API against the base branch + run: .github/scripts/apidiff.sh origin/${{ github.base_ref }} From 35f5025371441111edbf8900e4b90466a155285a Mon Sep 17 00:00:00 2001 From: westerberg Date: Mon, 10 Aug 2026 17:37:26 +0000 Subject: [PATCH 2/4] ci: pass the base ref to apidiff through the environment zizmor's template-injection audit flagged the run step, correctly. A ${{ }} expansion inside run: is substituted into the shell text before the shell sees it, so whatever the value holds becomes code rather than an argument. Passing it as an environment variable and quoting it in the script keeps it data. Caught by CI on the pull request that added the workflow, which is a reasonable advertisement for the check that job performs. --- .github/workflows/apidiff.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/apidiff.yml b/.github/workflows/apidiff.yml index f711801cb..e1d260c45 100644 --- a/.github/workflows/apidiff.yml +++ b/.github/workflows/apidiff.yml @@ -40,4 +40,10 @@ jobs: run: go install golang.org/x/exp/cmd/apidiff@v0.0.0-20250911091902-df9299821621 - name: Compare the exported API against the base branch - run: .github/scripts/apidiff.sh origin/${{ github.base_ref }} + # The ref goes through the environment rather than being interpolated + # into the command. A ${{ }} expansion inside run: is substituted into + # the shell text before the shell sees it, so anything controllable in + # the value becomes code; passing it as a quoted variable keeps it data. + env: + BASE_REF: ${{ github.base_ref }} + run: .github/scripts/apidiff.sh "origin/$BASE_REF" From d4d5e38e1f0b891b3bea8c8ea8c37d34b4f85e23 Mon Sep 17 00:00:00 2001 From: westerberg Date: Tue, 11 Aug 2026 08:35:36 +0000 Subject: [PATCH 3/4] ci: allow a deliberate API break behind a label The check shipped with no way to land an intended break. The failure message said a maintainer could override it, but the only mechanism was disabling a required check, which is how checks get removed rather than overridden. A pull request labelled breaking-change is still compared and still prints what changed, so the break is on the record instead of waved through invisibly. It just stops failing. --- .github/scripts/apidiff.sh | 24 ++++++++++++++++++------ .github/workflows/apidiff.yml | 6 ++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/scripts/apidiff.sh b/.github/scripts/apidiff.sh index 7250aab95..771f860e9 100755 --- a/.github/scripts/apidiff.sh +++ b/.github/scripts/apidiff.sh @@ -64,14 +64,26 @@ for pkg in $(packages); do fi done -if [ "$status" -ne 0 ]; then - cat <<'MSG' +if [ "$status" -eq 0 ]; then + exit 0 +fi + +cat <<'MSG' Incompatible API changes found. -If the break is intended, say so in the pull request description and a -maintainer can override this check. Note that adding a variadic parameter to an -existing exported function is a break even though call sites still compile. +Adding to the API is always allowed; the changes above remove something or +change its type. Note that adding a variadic parameter to an existing exported +function counts, even though every call site still compiles. + +If the break is deliberate, label the pull request "breaking-change". The +comparison still runs and still prints what changed, so the break stays on the +record, but it stops failing the check. MSG + +if [ "${ALLOW_BREAKING:-false}" = "true" ]; then + echo + echo 'Labelled "breaking-change", so this is reported rather than enforced.' + exit 0 fi -exit "$status" +exit 1 diff --git a/.github/workflows/apidiff.yml b/.github/workflows/apidiff.yml index e1d260c45..cbdf21683 100644 --- a/.github/workflows/apidiff.yml +++ b/.github/workflows/apidiff.yml @@ -46,4 +46,10 @@ jobs: # the value becomes code; passing it as a quoted variable keeps it data. env: BASE_REF: ${{ github.base_ref }} + # A pull request labelled breaking-change still gets compared, and + # still prints what changed, so the break is recorded rather than + # waved through invisibly. It just does not fail the check. Without + # an escape hatch the only way to land a deliberate break is to + # disable the check, which is how checks get removed entirely. + ALLOW_BREAKING: ${{ contains(github.event.pull_request.labels.*.name, 'breaking-change') }} run: .github/scripts/apidiff.sh "origin/$BASE_REF" From 3c1d2e21f33d139cdda287f0bc00783b497d61f5 Mon Sep 17 00:00:00 2001 From: westerberg Date: Tue, 11 Aug 2026 13:14:37 +0000 Subject: [PATCH 4/4] ci: compare whole modules instead of one package at a time The check took eleven minutes warm and over fifteen cold. It invoked apidiff once per exported package, and every invocation reloads and type-checks the entire dependency graph. apidiff -m reads a whole module in one pass and reports the same findings in seven seconds. Module mode also reaches plugin/agentanalytics, which go list ./... never saw. That module is public API and was going unchecked. And it skips internal/ on its own, so the grep filtering is gone with it. Separately, a module that cannot be read now fails. Every apidiff call carried a trailing || true, so a base revision that failed to build wrote no snapshots, every package was then skipped for want of one, and the job went green having compared nothing at all. apidiff exits non-zero only when it could not do its job, so that case is easy to separate from a run that found a break, and the breaking-change label does not cover it: a comparison that did not happen is not a break anyone decided to make. Verified against the same cases as before. A trailing variadic added to runner.New and to NewBatchProcessor is reported and exits 1, a clean tree exits 0, and a module that does not compile fails even with the label set. --- .github/scripts/apidiff.sh | 113 +++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 29 deletions(-) diff --git a/.github/scripts/apidiff.sh b/.github/scripts/apidiff.sh index 771f860e9..0d61a0e4b 100755 --- a/.github/scripts/apidiff.sh +++ b/.github/scripts/apidiff.sh @@ -1,10 +1,10 @@ #!/usr/bin/env bash -# Reports incompatible changes to the module's exported API. +# Reports incompatible changes to the exported API of the repository's modules. # -# Compares every exported package against a base revision and fails when -# apidiff calls a change incompatible. Adding to the API is always allowed. +# Compares every module against a base revision and fails when apidiff calls a +# change incompatible. Adding to the API is always allowed. # -# Usage: apidiff.sh +# Usage: apidiff.sh # # Why this exists: a trailing variadic parameter added to an exported function # changes that function's type. Every ordinary call site still compiles, so @@ -22,49 +22,104 @@ BASE_REF="$(git merge-base HEAD "$TARGET_REF")" echo "Target $TARGET_REF, comparing against merge base ${BASE_REF:0:12}" WORK="$(mktemp -d)" -trap 'rm -rf "$WORK"' EXIT +BASE_TREE="$WORK/base" +cleanup() { + git worktree remove --force "$BASE_TREE" >/dev/null 2>&1 || true + rm -rf "$WORK" +} +trap cleanup EXIT +mkdir -p "$WORK/api" -# Exported packages only. internal/ is not public API, and neither is a package -# with no importable identifiers. -packages() { - go list ./... | grep -v '/internal/' | grep -v '/internal$' +# Every module in the repository, as a path relative to the root. +modules() { + find . -not -path '*/.git/*' -name go.mod -exec dirname {} \; | sort } -echo "Collecting the API at $BASE_REF" -BASE_TREE="$WORK/base" -git worktree add --detach --quiet "$BASE_TREE" "$BASE_REF" -cleanup_worktree() { git worktree remove --force "$BASE_TREE" >/dev/null 2>&1 || true; } -trap 'cleanup_worktree; rm -rf "$WORK"' EXIT +# The module path declared by a directory. GOWORK=off because inside a +# workspace `go list -m` reports every module in the workspace, not this one. +module_path() { (cd "$1" && GOWORK=off go list -m); } -mkdir -p "$WORK/api" +snapshot_for() { echo "$WORK/api/$(echo "$1" | tr '/' '_').api"; } + +# apidiff narrates every internal package it skips, on stderr, which buries the +# one line that says what actually went wrong under eighty that do not. +report_error() { grep -v '^Ignoring internal package ' "$WORK/err" | sed 's/^/ /' || true; } + +# apidiff is asked for a whole module at a time. -m loads every package in the +# module in one pass and skips internal/ by itself, where invoking it once per +# package costs minutes on a module this size and reports the same thing. +# +# It exits non-zero only when it could not do its job; finding an incompatible +# change is a successful run that prints to stdout. Keeping those two apart is +# the difference between "nothing broke" and "nothing was compared", which look +# identical from the outside. +failed=0 +breaks=0 + +echo "Collecting the API at the base revision" +git worktree add --detach --quiet "$BASE_TREE" "$BASE_REF" ( cd "$BASE_TREE" go work init >/dev/null 2>&1 || true go work use -r . >/dev/null 2>&1 || true - for pkg in $(packages); do - # A package that does not build at the base has no baseline to compare - # against, which is normal for one this change introduces. - apidiff -w "$WORK/api/$(echo "$pkg" | tr '/' '_').api" "$pkg" >/dev/null 2>&1 || true - done ) +while IFS= read -r dir; do + mod="$(module_path "$dir")" + snap="$(snapshot_for "$mod")" + + # A module this change introduces has no baseline, which is not a problem. + if [ ! -d "$BASE_TREE/$dir" ]; then + echo " $mod is new; nothing to compare against" + continue + fi + + if (cd "$BASE_TREE/$dir" && apidiff -m -w "$snap" "$mod") 2>"$WORK/err"; then + echo " $mod" + else + echo " ERROR: could not read the API of $mod at the base revision" + report_error + failed=1 + fi +done < <(modules) + echo "Comparing HEAD against it" -status=0 -for pkg in $(packages); do - snapshot="$WORK/api/$(echo "$pkg" | tr '/' '_').api" - [ -f "$snapshot" ] || continue # new package, nothing to break +while IFS= read -r dir; do + mod="$(module_path "$dir")" + snap="$(snapshot_for "$mod")" + [ -f "$snap" ] || continue + + if ! out="$( (cd "$dir" && apidiff -m "$snap" "$mod") 2>"$WORK/err" )"; then + echo " ERROR: could not read the API of $mod at HEAD" + report_error + failed=1 + continue + fi - out="$(apidiff "$snapshot" "$pkg" 2>/dev/null || true)" # apidiff prints an "Incompatible changes:" section only when there are some. if printf '%s' "$out" | grep -q '^Incompatible changes:'; then echo - echo "=== $pkg ===" + echo "=== $mod ===" printf '%s\n' "$out" | sed -n '/^Incompatible changes:/,/^$/p' - status=1 + breaks=1 fi -done +done < <(modules) + +# A module that could not be read was not checked at all. Reporting that as a +# clean run is the one outcome worse than not having the check, so it fails +# here, and the breaking-change label below does not cover it. +if [ "$failed" -ne 0 ]; then + cat <<'MSG' + +The comparison did not complete: the errors above mean some module's API was +never read, so nothing about it was verified. Fix those before reading this +check as "no breaking changes". +MSG + exit 1 +fi -if [ "$status" -eq 0 ]; then +if [ "$breaks" -eq 0 ]; then + echo "No incompatible changes." exit 0 fi