From abf3d76873f685784c012e851e039aa00b56c2d9 Mon Sep 17 00:00:00 2001 From: Alexandre <44178713+alexbelgium@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:05:42 +0200 Subject: [PATCH] birdnet-go-dev: add merge-prs.sh --check to catch build-breaking PR conflicts (#3053) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * birdnet-go-dev: add merge-prs.sh --check to catch build-breaking PR conflicts The add-on build merges every open non-draft fork PR onto an upstream-synced main. GitHub's `mergeable` field answers a different question: it compares a PR against its *own* base ref, which for the stacked dashboard PRs is another feature branch - sometimes one belonging to an already-closed PR. So a PR can read MERGEABLE/CLEAN and still fail the build. That is how run 34101694542 broke: PR #57 is MERGEABLE against the frozen branch of closed PR #56, but conflicts with main on DetectionCard.svelte. --check replays the exact same merge sequence, skips past conflicts instead of stopping at the first one, and reports every offender as !!! CONFLICT pr=#N conflicts-with= files=... title=... conflicts-with is probed in a throwaway worktree against the pristine synced main, so it separates a PR that is merely stale (fixable in its own branch) from one that only clashes with another open PR (needs a cross-PR decision). Build behaviour is unchanged; --check is a no-op unless asked for, so no version bump. Co-Authored-By: Claude Opus 5 * birdnet-go-dev: fix --check misclassification, arg handling and temp leak Review findings on #3053, all reproduced against git fixtures before fixing. 1. The probe did not apply the build's own merge policy (Codex). The real merge treats a *sole* frontend/package-lock.json conflict as non-fatal, but merges_onto_main() did a raw merge. A PR whose only clash with main is the generated lockfile was therefore reported as conflicts-with=main, which prints "the PR is stale against main; merge main into its branch" when in truth it merges onto main fine and only clashes with another open PR — the exact opposite remediation, from the feature whose entire job is to say which one it is. Extracted the policy into resolve_sole_lockfile() and routed both the real merge and the probe through it, so the two cannot drift apart again. 2. A second positional operand silently won (CodeRabbit). `merge-prs.sh a b` ran against b, where the pre-flag script used "${1}". A stray argument would have cloned into the wrong directory. Now exits 64. 3. mktemp parent directory leaked (CodeRabbit, Copilot). probe="$(mktemp -d)/probe" and only the child was removed, leaking one empty dir per checked conflict. Measured 3 leaked dirs over 3 calls; now 0. Verified with throwaway repos: a lockfile-only clash now classifies as "accumulated" (was "main"), a real source conflict with main still classifies as "main", a clean PR still classifies as "accumulated", 0 leaked temp dirs, 0 stray worktrees, and the argument matrix behaves. shellcheck clean. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 Co-authored-by: BirdNET-Go Addon Builder --- birdnet-go-dev/merge-prs.sh | 115 ++++++++++++++++++++++++++++++++---- 1 file changed, 103 insertions(+), 12 deletions(-) diff --git a/birdnet-go-dev/merge-prs.sh b/birdnet-go-dev/merge-prs.sh index b3430adc19..daa08b2033 100755 --- a/birdnet-go-dev/merge-prs.sh +++ b/birdnet-go-dev/merge-prs.sh @@ -11,6 +11,15 @@ # stamping) is written to the directory given as $1 so the Docker build can # compile it. # +# With --check the script does not build anything: it performs the exact same +# merge sequence, skips (instead of failing on) every conflicting PR, and prints +# one "!!! CONFLICT pr=#N conflicts-with=... files=... " line per offender before +# exiting 2. Use it to find conflicts *before* a build burns on them. This is a +# different question from GitHub's `mergeable` field, which compares a PR against +# its own base ref - for a stacked PR that base is another feature branch (often +# stale, sometimes belonging to a closed PR), so GitHub can report CLEAN for a PR +# that does not merge onto main at all. +# # Environment: # BIRDNET_FORK owner/repo of the fork (default alexbelgium/birdnet-go) # BIRDNET_UPSTREAM owner/repo of the upstream (default tphakala/birdnet-go) @@ -19,7 +28,26 @@ # set -euo pipefail -TARGET_DIR="${1:?usage: merge-prs.sh }" +CHECK_ONLY="${MERGE_PRS_CHECK:-0}" +TARGET_DIR="" +while [ "$#" -gt 0 ]; do + case "$1" in + --check) CHECK_ONLY=1 ;; + -*) echo "unknown option: $1" >&2; exit 64 ;; + *) + # Last-one-wins would silently clone into the wrong directory if a caller ever + # appended an argument; the pre-flag script used "${1}", so refuse rather than + # quietly change which operand counts. + if [ -n "${TARGET_DIR}" ]; then + echo "usage: merge-prs.sh [--check] " >&2 + exit 64 + fi + TARGET_DIR="$1" + ;; + esac + shift +done +: "${TARGET_DIR:?usage: merge-prs.sh [--check] }" FORK="${BIRDNET_FORK:-alexbelgium/birdnet-go}" UPSTREAM="${BIRDNET_UPSTREAM:-tphakala/birdnet-go}" @@ -33,6 +61,52 @@ GH_TOKEN="${GH_TOKEN:-${GITHUB_TOKEN:-}}" log() { echo ">>> $*"; } +# Conflicting PRs collected in --check mode: "number|scope|files|title". +conflicting=() + +LOCKFILE="frontend/package-lock.json" + +# The single place that decides whether a conflicted merge is still acceptable. +# package-lock.json is generated content and stacked PRs can carry an older copy even when +# their source changes merge cleanly, so keep the lockfile already assembled on the base side +# — but only when it is the sole conflict. Any source conflict stays fatal. +# Returns 0 when it resolved and committed such a merge, 1 when the conflict is real. +# BOTH the real merge and the --check probe must go through here: when only the real merge +# applied the policy, the probe called a PR "conflicts-with=main" that the build would have +# merged fine, and printed the opposite remediation to the true one. +resolve_sole_lockfile() { + local dir="$1" + local -a conflicted + mapfile -t conflicted < <(git -C "${dir}" diff --name-only --diff-filter=U) + if [ "${#conflicted[@]}" -ne 1 ] || [ "${conflicted[0]}" != "${LOCKFILE}" ]; then + return 1 + fi + log "Resolving generated ${LOCKFILE} conflict using the base tree" + git -C "${dir}" checkout --ours -- "${LOCKFILE}" || return 1 + git -C "${dir}" add "${LOCKFILE}" || return 1 + git -C "${dir}" commit --no-edit > /dev/null || return 1 +} + +# Does ${1} merge cleanly onto the pristine upstream-synced main? Probed in a +# throwaway worktree so the accumulated tree is left untouched. Tells apart a PR +# that is simply stale against main (fixable inside that PR's own branch) from +# one that only clashes with another open PR (needs a cross-PR decision). +merges_onto_main() { + local sha="$1" tmpdir probe rc=0 + tmpdir="$(mktemp -d)" + probe="${tmpdir}/probe" + git worktree add --quiet --detach "${probe}" "${MAIN_SYNCED}" + if ! git -C "${probe}" merge --no-edit --no-ff -m probe "${sha}" > /dev/null 2>&1; then + # Same policy as the real merge, or this misclassifies a lockfile-only clash. + resolve_sole_lockfile "${probe}" > /dev/null 2>&1 || rc=1 + fi + git worktree remove --force "${probe}" > /dev/null 2>&1 || true + # worktree remove only takes the child back; without this the mktemp parent is left behind + # on every checked conflict. + rmdir "${tmpdir}" > /dev/null 2>&1 || true + return "${rc}" +} + git config --global user.email "addon-builder@users.noreply.github.com" git config --global user.name "BirdNET-Go Addon Builder" git config --global advice.detachedHead false @@ -47,6 +121,7 @@ git remote add upstream "${UPSTREAM_URL}" git fetch --no-tags upstream main # --no-ff keeps an explicit sync commit; a no-op when main is already current. git merge --no-edit --no-ff upstream/main +MAIN_SYNCED="$(git rev-parse HEAD)" log "Querying open non-draft PRs from ${FORK}" auth_header=() @@ -79,27 +154,43 @@ for entry in "${prs[@]}"; do if ! git merge --no-edit --no-ff -m "Merge PR #${number}: ${title}" "${sha}"; then mapfile -t conflicted_files < <(git diff --name-only --diff-filter=U) - # package-lock.json is generated content and stacked PRs can carry an - # older copy even when their source changes merge cleanly. Keep the - # lockfile already assembled from upstream and earlier PRs, but only - # when it is the sole conflict. Any source conflict remains fatal. - if [ "${#conflicted_files[@]}" -eq 1 ] \ - && [ "${conflicted_files[0]}" = "frontend/package-lock.json" ]; then - log "Resolving generated frontend/package-lock.json conflict using the accumulated tree" - git checkout --ours -- frontend/package-lock.json - git add frontend/package-lock.json - git commit --no-edit + if resolve_sole_lockfile .; then + : # generated lockfile only - the merge is committed and the build continues else echo "!!! Merge conflict while merging PR #${number} (${title})." >&2 if [ "${#conflicted_files[@]}" -gt 0 ]; then printf '!!! Conflicting file: %s\n' "${conflicted_files[@]}" >&2 fi - echo "!!! Resolve the conflict in the fork or pause this PR, then rebuild." >&2 git merge --abort || true + + if [ "${CHECK_ONLY}" = "1" ]; then + scope="accumulated" + merges_onto_main "${sha}" || scope="main" + conflicting+=("${number}|${scope}|${conflicted_files[*]:-}|${title}") + log "check mode: skipping PR #${number}, continuing with the rest" + continue + fi + + echo "!!! Resolve the conflict in the fork or pause this PR, then rebuild." >&2 exit 1 fi fi done +if [ "${CHECK_ONLY}" = "1" ]; then + if [ "${#conflicting[@]}" -eq 0 ]; then + log "CHECK OK: every open non-draft PR merges into the combined build tree" + exit 0 + fi + echo "!!! CHECK FAILED: ${#conflicting[@]} PR(s) would break the add-on build" >&2 + for entry in "${conflicting[@]}"; do + IFS='|' read -r number scope files title <<<"${entry}" + echo "!!! CONFLICT pr=#${number} conflicts-with=${scope} files=${files} title=${title}" >&2 + done + echo "!!! conflicts-with=main -> the PR is stale against main; merge main into its branch and resolve there." >&2 + echo "!!! conflicts-with=accumulated -> the PR only clashes with another open PR; decide which one owns the hunk." >&2 + exit 2 +fi + log "Merged HEAD: $(git rev-parse --short HEAD)" log "Source tree ready at ${TARGET_DIR}"