From c2dad6c88e62ba29db4d4ec85fa33d2f14800040 Mon Sep 17 00:00:00 2001 From: "claude-ai-fix[bot]" Date: Sun, 23 Aug 2026 14:20:00 +0200 Subject: [PATCH] ci: fail a PR that changes an add-on without bumping its version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supervisor only offers a rebuild when `version` changes, so an add-on fix merged without one reaches nobody: the image stays put, the issue looks closed. Nothing in CI checked this. Not hypothetical — PR #2972 changed birdnet-pi's 99-run.sh with no bump and sat green. Worse, it also hid the rest of CI. check-addon-changes derives `changedAddons` from `^/config\.(json|ya?ml)$` alone, and the CHANGELOG gate, add-on linter and Docker build are all matrixed over it — so a PR that edits only an add-on's scripts produces an empty set and every one of them *skips*. #2972 was never linted or built, and birdnet-pi-zach turned out to have been failing to build for five weeks behind that same gap. This check therefore does NOT reuse `changedAddons`: gating on it would make the check a no-op in exactly the case it exists for. It runs its own scan and changes nothing about which add-ons get linted or built — widening that detection is a separate change with real build-time cost. Significance is an allowlist of ignorable paths anchored at the add-on root (CHANGELOG, top-level *.md, icon/logo/stats png, images/, updater.json) with everything else counting, so a new kind of file defaults to needing a bump. Note addon/images/ is artwork but addon/rootfs/**/images/ ships, hence the anchoring. Reads the manifest at HEAD by its own resolved filename, so renaming config.yaml -> config.yml mid-PR cannot slip through; parses config.json with jq so a minified file is read correctly; anchors the YAML `version:` match at column 0 so an indented key in a nested mapping is not mistaken for the manifest's; strips a trailing YAML comment so `version: "1.2.3" # note` is not read as a change. An unreadable version is an error, not a warning — the check refuses to pass where it could not be performed. The bypass label is read live inside the job rather than in its `if:`, because the workflow deliberately does not listen for `labeled`: adding that activity type would re-run the ~3 h add-on builds on every label change. Labelling the PR and re-running this one job is the intended sequence. Verified across 22 cases against real history and synthetic diffs: fails #2972 as originally opened and passes it after the bump, passes #2973/#2974 and the current #2972; stays quiet on CHANGELOG-, stats.png-, updater.json-, README-, .templates- and addon/images-only diffs; and fails on a changed Dockerfile/rootfs, a config.yaml option change, rootfs/**/images, a comment-only version edit, an indented nested version key, a mid-PR manifest rename, a config.json add-on, and a missing version key. Co-Authored-By: Claude Opus 5 --- .github/scripts/check_version_bump.sh | 143 ++++++++++++++++++++++++++ .github/workflows/onpr_check-pr.yaml | 63 ++++++++++++ 2 files changed, 206 insertions(+) create mode 100755 .github/scripts/check_version_bump.sh diff --git a/.github/scripts/check_version_bump.sh b/.github/scripts/check_version_bump.sh new file mode 100755 index 0000000000..38b8f9a741 --- /dev/null +++ b/.github/scripts/check_version_bump.sh @@ -0,0 +1,143 @@ +#!/usr/bin/env bash +# Destination: .github/scripts/check_version_bump.sh +# +# Fails a pull request that changes what an add-on ships without bumping that +# add-on's `version`. Supervisor only offers a rebuild when `version` changes, +# so a fix merged without one leaves every user on the old image: merged, inert, +# and the issue looks closed. Nothing else in CI checks this. +# +# It deliberately does NOT reuse check-addon-changes' `changedAddons`. That +# output is built from `^/config\.(json|ya?ml)$` alone, so it is empty +# for exactly the pull requests this check exists to catch — an add-on whose +# scripts changed while config.yaml did not. Reusing it would make this a no-op. +# This scan is local to this check and does not affect which add-ons get linted +# or built. +# +# Env: +# BASE_SHA (required) — commit this PR is diffed against +# HEAD_SHA (required) — the PR's merge commit +# +# Exit 0 = every add-on that needs a bump got one (or nothing relevant changed). + +set -euo pipefail + +: "${BASE_SHA:?BASE_SHA must be set}" +: "${HEAD_SHA:?HEAD_SHA must be set}" + +# Files that ship to users only as repo metadata, or that bots rewrite on their +# own schedule. Changing one of these alone does not require anybody to receive +# a new image, so it must not demand a version bump — otherwise every changelog +# or stats-graph commit would fail CI. +# +# Everything else under an add-on directory counts: Dockerfile, rootfs/, +# build.json|yaml (base images), apparmor.txt and translations/ (Supervisor +# re-reads them on update), root-level *.sh that Dockerfiles COPY, and +# config.yaml itself — an added option or changed port needs the update offered +# just as much as a code change does. Allowlisting the ignorable and treating +# the remainder as significant fails closed: a new kind of file defaults to +# "needs a bump" rather than silently escaping the check. +is_ignorable() { + local rel="$1" # path relative to the add-on directory + # addon/images/** is artwork; addon/rootfs/**/images/** is shipped content, + # so this must be anchored at the add-on root rather than matching any + # path that happens to contain an "images" segment. + case "$rel" in + images/*) return 0 ;; + esac + # Anything else nested ships inside the image (rootfs/, translations/, ...). + case "$rel" in + */*) return 1 ;; + esac + case "$rel" in + CHANGELOG.md | updater.json | stats.png | icon.png | logo.png | *.md) return 0 ;; + esac + return 1 +} + +# An add-on is a top-level directory with a config file. Tested against the +# BASE tree so a directory deleted by this PR is still recognised (and then +# skipped below), and .github/, .templates/ and .claude/ are excluded for free +# by simply not having one. +addon_config_at() { + local ref="$1" addon="$2" f + for f in config.yaml config.yml config.json; do + if git cat-file -e "${ref}:${addon}/${f}" 2> /dev/null; then + printf '%s' "$f" + return 0 + fi + done + return 1 +} + +# Reads the add-on's declared version. JSON goes through jq so a minified or +# reordered config.json is read correctly rather than silently returning empty. +# YAML is matched at column 0 on purpose: an indented `version:` belongs to a +# nested mapping (a schema entry, an option literally named version) and +# comparing it would compare the wrong value. A trailing YAML comment is +# stripped before quotes so `version: "1.2.3" # note` does not read as a bump. +version_at() { + local ref="$1" path="$2" content + content=$(git show "${ref}:${path}" 2> /dev/null) || return 1 + case "$path" in + *.json) + printf '%s' "$content" | jq -r '.version // empty' 2> /dev/null + ;; + *) + printf '%s\n' "$content" | + grep -m1 -E '^version[[:space:]]*:' | + sed -E 's/^version[[:space:]]*:[[:space:]]*//; s/[[:space:]]+#.*$//; s/^["'\'']//; s/["'\'']$//; s/[[:space:]]*$//' + ;; + esac +} + +mapfile -t CHANGED < <(git diff --name-only "$BASE_SHA" "$HEAD_SHA") + +declare -A NEEDS_BUMP=() +for file in "${CHANGED[@]}"; do + [ -n "$file" ] || continue + case "$file" in */*) ;; *) continue ;; esac # top-level files are not add-ons + addon="${file%%/*}" + is_ignorable "${file#*/}" && continue + addon_config_at "$BASE_SHA" "$addon" > /dev/null 2>&1 || continue + NEEDS_BUMP["$addon"]=1 +done + +if [ "${#NEEDS_BUMP[@]}" -eq 0 ]; then + echo "No add-on changes that require a version bump." + exit 0 +fi + +FAILED=0 +for addon in $(printf '%s\n' "${!NEEDS_BUMP[@]}" | sort); do + base_cfg=$(addon_config_at "$BASE_SHA" "$addon") + # Resolved separately at HEAD: an add-on that renames its manifest between + # supported names (config.yaml -> config.yml) while changing shipped files + # would otherwise be read at the old path, come back empty, and slip through. + if ! head_cfg=$(addon_config_at "$HEAD_SHA" "$addon"); then + echo " $addon: removed by this PR, skipping" + continue + fi + old=$(version_at "$BASE_SHA" "$addon/$base_cfg" || true) + new=$(version_at "$HEAD_SHA" "$addon/$head_cfg" || true) + # Fail closed. An unreadable version used to warn and skip, which let the + # job go green on exactly the add-ons whose manifest this check could not + # understand — the opposite of what it is for. + if [ -z "$old" ] || [ -z "$new" ]; then + echo "::error file=$addon/$head_cfg::$addon: could not read a version from $base_cfg (base) or $head_cfg (head). Refusing to pass a check that could not be performed." + FAILED=1 + continue + fi + if [ "$old" = "$new" ]; then + echo "::error file=$addon/$head_cfg::$addon ships changed files but version is still $old. Supervisor only offers a rebuild when version changes, so this would merge without reaching anyone. Bump it following this add-on's own convention: for a local patch counter take the boundary from updater.json's upstream_version (append .1 when version equals it, otherwise increment the digits after it); date-based and LSIO-style versions have no counter and follow their own scheme." + FAILED=1 + else + echo " $addon: $old -> $new" + fi +done + +if [ "$FAILED" -ne 0 ]; then + echo "::error::One or more add-ons changed without a version bump. Add the 'skip-version-check' label and re-run this job if that is deliberate." + exit 1 +fi + +echo "All changed add-ons have a version bump." diff --git a/.github/workflows/onpr_check-pr.yaml b/.github/workflows/onpr_check-pr.yaml index 0a7c6d61d3..bfd1afce4c 100644 --- a/.github/workflows/onpr_check-pr.yaml +++ b/.github/workflows/onpr_check-pr.yaml @@ -58,6 +58,69 @@ jobs: echo "Changed addons: $changed_addons" echo "changed_addons=$changed_addons" >> "$GITHUB_OUTPUT" + # 1b. A pull request that changes what an add-on ships must bump that add-on's + # version. Supervisor only offers a rebuild when `version` changes, so without + # one the fix merges and no user ever receives it - inert, while the issue + # looks closed. Nothing else in CI checks this (the add-on linter validates + # the schema, not that the value moved). + # + # Deliberately independent of check-addon-changes above: that job derives + # `changedAddons` from `^/config.(json|ya?ml)$` alone, so it is empty + # for precisely the pull requests this catches - an add-on whose scripts + # changed while config.yaml did not. Gating this on its output would make it + # a no-op. It does its own scan and changes nothing about which add-ons are + # linted or built. + check-version-bump: + name: Check add-on version bumped + if: ${{ github.repository_owner == 'alexbelgium' }} + runs-on: ubuntu-latest + # Only reads git history and the PR's labels. + permissions: + contents: read + pull-requests: read + steps: + # The bypass label is read LIVE here rather than from the job's `if:`. + # The event payload is a snapshot from trigger time, and this workflow + # deliberately does not listen for `labeled` — adding that activity type + # would re-run the ~3 h add-on builds on every label change. Reading it + # at run time instead means "label the PR, then re-run this one job" + # works, which is the sequence the failure message asks for. + - name: Check for the bypass label + id: bypass + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + skip=$(gh pr view "$PR" --repo "$REPO" --json labels \ + --jq '[.labels[].name] | index("skip-version-check") != null') + echo "skip=$skip" >> "$GITHUB_OUTPUT" + [ "$skip" = "true" ] && echo "skip-version-check label present; skipping." || true + + - name: Checkout repo + if: steps.bypass.outputs.skip != 'true' + uses: actions/checkout@v7.0.1 + with: + # Same reason as check-addon-changes: HEAD^1 must be resolvable. + fetch-depth: 2 + # Nothing here pushes, and the repo is public, so an anonymous fetch + # of the base commit is enough - do not leave a token in .git/config. + persist-credentials: false + + - name: Check every changed add-on bumped its version + if: steps.bypass.outputs.skip != 'true' + env: + HEAD_SHA: ${{ github.sha }} + run: | + set -euo pipefail + # HEAD^1, not pull_request.base.sha, for the same reason as above: the + # event payload's base can be stale if master advanced since trigger. + BASE_SHA=$(git rev-parse HEAD^1) + git fetch origin "$BASE_SHA" + export BASE_SHA + bash .github/scripts/check_version_bump.sh + check-changed-changelog: name: Check if CHANGELOG.md changed needs: check-addon-changes