From 0ed4e0ae11912a8c9b4ad5f2ee73b130c3ff19b5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 20:47:22 -0700 Subject: [PATCH 1/3] =?UTF-8?q?ci(bump-callers):=20add=20preflight.sh=20?= =?UTF-8?q?=E2=80=94=20one=20tested=20staleness/decommission=20guard=20(BE?= =?UTF-8?q?-6475)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every bump-*-callers.yml entrypoint carries an inline copy of the staleness/decommission guard that runs ahead of bump-callers.sh, and the eight copies have drifted: five skip on a bare tip mismatch (throwing away the only run for a change), one compares content but never re-points the pin at the verified tip, one is the hardened #117 version, and pr-risk has grown a different hardening again. Extract the #117 guard as .github/bump-callers/preflight.sh, generalized with an optional WATCHED_ASSETS input so a multi-path fleet compares the asset directory too — which is exactly what the "COUPLED TO THE PATH FILTER" note in #117 requires before its re-point can be reused. Emits proceed/new_sha as step outputs (never $GITHUB_ENV, which a step-level env: NEW_SHA: would silently override). Phase 1 of 2: script + tests + docs only. No entrypoint is swapped over yet, so no fleet behaviour changes and no bump fleet fires on this merge. --- .github/bump-callers/README.md | 82 ++++++- .github/bump-callers/preflight.sh | 190 +++++++++++++++ .github/bump-callers/tests/test_preflight.sh | 235 +++++++++++++++++++ .github/workflows/test-bump-callers.yml | 12 +- AGENTS.md | 9 +- 5 files changed, 520 insertions(+), 8 deletions(-) create mode 100755 .github/bump-callers/preflight.sh create mode 100755 .github/bump-callers/tests/test_preflight.sh diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 87adde0..9399276 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -16,7 +16,11 @@ forward automatically instead of silently drifting commits behind. @SHORT" diff) and, if a bump PR is already open, refreshes its title/body to the new SHA rather than opening another. A fresh PR is opened only when none is open (first bump, or the prior one merged/closed since the last run). -- **`tests/`** — a `bash` functional suite (stubs `gh`, no network), run by +- **`preflight.sh`** — the staleness/decommission guard that runs *before* the + bump script (see [Preflight](#preflight) below). Also one file on purpose: it + was an inline copy in every entrypoint, and the copies drifted. +- **`tests/`** — `bash` functional suites (stubs `gh` / builds throwaway local + repos; no network), run by [`test-bump-callers.yml`](../workflows/test-bump-callers.yml) plus shellcheck. ## The fleets @@ -104,6 +108,82 @@ otherwise they are left as found and the run logs a warning. Inert for every caller today (all call exactly one reusable); it exists so a caller that starts calling two cannot be corrupted. +## Preflight + +Before an entrypoint may bump anything it has to answer two questions: is this +run **stale** (has a later commit already touched the watched surface, so *that* +commit has its own run?), and has the watched surface been **decommissioned** +(deleted, so pinning callers to this SHA would break every one of them)? + +`preflight.sh` is that guard. It used to be an inline copy in each +`bump-*-callers.yml`, and the copies drifted — several skipped on a bare tip +mismatch, which throws away the only run for a change and freezes every caller, +and the one that compared content forgot to re-point the pin at the verified tip. +The extracted script deliberately adopts the hardened semantics: exact-refname +tip parse (a branch literally named `foo/refs/heads/main` matches the ls-remote +pattern at component boundaries and must not be consumed), `FETCH_HEAD` +verification before any object is read out of it, deletion tested through the +`$WATCHED` **variable** rather than a second copy of the literal path, and the +re-point that pins callers to the verified tip instead of a stale `github.sha`. + +| Input (env) | | +|---|---| +| `WATCHED` | **required** — repo-relative path of the watched reusable workflow (e.g. `.github/workflows/groom.yml`) | +| `WATCHED_ASSETS` | optional — the watched asset directory (e.g. `.github/groom`). Empty/unset means the fleet is single-path | +| `NEW_SHA` | the candidate SHA, normally `github.sha` | +| `GITHUB_SHA`, `GITHUB_OUTPUT` | provided by Actions | + +| Output (step output) | | +|---|---| +| `proceed` | `true` → run `bump-callers.sh`; `false` → stale or decommissioned, do nothing | +| `new_sha` | the SHA to pin — `NEW_SHA`, or the verified main tip when the run was re-pointed forward | + +Both outputs are written on **every** exit-0 path. The script exits non-zero only +for a lookup it could not perform (failed `ls-remote`, failed fetch, unresolvable +`FETCH_HEAD`): a lookup we couldn't perform is not evidence of staleness, so it +fails loudly rather than silently no-opping the fleet. + +**A multi-path fleet must pass `WATCHED_ASSETS`.** The re-point is only sound +because every entry in the fleet's `paths:` trigger is covered by the comparison +— for `cursor-review` / `groom` / `pr-size` that includes the asset directory the +reusable loads its prompts/scripts/briefs from at run time. Compare `WATCHED` +alone on one of those and a commit touching only the assets reads as "unchanged", +so callers get pinned to a tip whose other relevant content was never verified. +If you widen a fleet's path filter, widen these inputs in the same change. + +Consumption is two steps — the guard, then the bump gated on its output: + +```yaml + - name: Preflight (staleness / decommission guard) + id: preflight + env: + WATCHED: .github/workflows/groom.yml + WATCHED_ASSETS: .github/groom # omit for a single-path fleet + NEW_SHA: ${{ github.sha }} + run: bash .github/bump-callers/preflight.sh + + - name: Bump SHA in caller repos + if: steps.preflight.outputs.proceed == 'true' + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + NEW_SHA: ${{ steps.preflight.outputs.new_sha }} + # …VAR_NAME / TAG / WORKFLOW_FILE / CALLERS_JSON as before + run: bash .github/bump-callers/bump-callers.sh +``` + +`new_sha` is a **step output**, not a `$GITHUB_ENV` export, and the consuming +step reads it through its own `env:` binding. That is deliberate: a step-level +`env: NEW_SHA:` takes precedence over the job environment, so a `$GITHUB_ENV` +write would be silently overridden by the very binding it is meant to correct. + +> **The entrypoints still carry their inline copies.** Swapping them over to this +> script is a separate change. `bump-pr-risk-callers.yml` needs a decision rather +> than a swap: its copy has hardening this one does not implement — a +> `git rev-list` "did a later *commit* touch a watched path" test (rather than a +> net-content comparison) and an is-ancestor check that refuses to pin an +> orphaned commit — so folding it in, or keeping that fleet on its own guard, has +> to be chosen deliberately, not by deleting the checks. + ## How the pin rewrite is scoped (and why it asserts afterwards) The rewrite targets the **pin token**, not "any 40-hex on a line that mentions diff --git a/.github/bump-callers/preflight.sh b/.github/bump-callers/preflight.sh new file mode 100755 index 0000000..5f056f0 --- /dev/null +++ b/.github/bump-callers/preflight.sh @@ -0,0 +1,190 @@ +#!/usr/bin/env bash +# +# Staleness / decommission preflight for the bump-* caller fleets. +# +# Every `bump-*-callers.yml` entrypoint has to answer the same two questions +# before it hands control to bump-callers.sh: +# +# 1. Is this run STALE — i.e. has a *later* commit already touched the watched +# surface, so that commit has its own run and will pin the newer content? +# 2. Has the watched surface been DECOMMISSIONED — deleted — so that pinning +# callers to this SHA would break every one of them? +# +# Both questions are answered today by an inline copy of this logic in each +# bump-* entrypoint. Eight near-copies is exactly the drift pattern this +# directory exists to prevent (see bump-callers.sh's header), and they HAVE +# drifted: five skip on a bare tip mismatch, which throws away the ONLY run for a +# change; bump-auto-label-callers.yml compares content but forgets to re-point +# the pin at the verified tip; bump-detect-unreviewed-merge-callers.yml is the +# hardened one; and bump-pr-risk-callers.yml has since grown a different +# hardening again (a `git rev-list` "did a later COMMIT touch a watched path" +# test plus an is-ancestor orphan check, and no re-point). +# +# This script is the one implementation, and it deliberately adopts the +# bump-detect-unreviewed-merge-callers.yml semantics (PR #117) — exact-refname +# tip parse, FETCH_HEAD verification, `$WATCHED`-variable deletion guard, and the +# NEW_SHA re-point — generalized to multi-path fleets. Nothing consumes it yet; +# swapping the entrypoints over is a separate change, and the pr-risk swap in +# particular has to decide what to do with that fleet's extra checks rather than +# drop them. +# +# Required environment: +# WATCHED Repo-relative path of the watched reusable workflow file +# (e.g. .github/workflows/groom.yml). +# NEW_SHA The candidate commit to pin callers to (normally github.sha). +# GITHUB_SHA This run's own commit (provided by Actions). +# GITHUB_OUTPUT Step-output file (provided by Actions). +# Optional: +# WATCHED_ASSETS Watched asset directory (e.g. .github/groom) for a fleet whose +# `paths:` filter has more than one entry. Empty/unset means the +# fleet is single-path. +# +# Outputs (written to $GITHUB_OUTPUT on every exit-0 path): +# proceed "true" → the caller should run bump-callers.sh +# "false" → stale or decommissioned; the caller should do nothing +# new_sha the SHA to pin callers to — NEW_SHA, or the verified main tip when +# this run was re-pointed forward (see the re-point block below) +# +# Exits non-zero ONLY for a lookup we could not perform (failed ls-remote, failed +# fetch, unresolvable FETCH_HEAD). A lookup we couldn't perform is not evidence +# of staleness — it fails loudly rather than silently no-opping the fleet. +# +# Run from the repository root (the final decommission check tests the run's own +# checked-out tree). +set -euo pipefail + +: "${WATCHED:?WATCHED is required}" +: "${NEW_SHA:?NEW_SHA is required}" +: "${GITHUB_SHA:?GITHUB_SHA is required}" +: "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" +WATCHED_ASSETS="${WATCHED_ASSETS-}" + +# Both outputs are written on EVERY exit-0 path, so a consuming step never reads +# an empty `new_sha` off a skip. NEW_SHA is deliberately a step OUTPUT and not a +# $GITHUB_ENV export: a step-level `env: NEW_SHA:` binding in the consuming step +# takes precedence over the job environment, so a $GITHUB_ENV write would be +# silently overridden by the very binding it is meant to correct. +emit() { + printf 'proceed=%s\n' "$1" >> "$GITHUB_OUTPUT" + printf 'new_sha=%s\n' "$2" >> "$GITHUB_OUTPUT" +} + +# The main-only ref guard in the entrypoints cannot catch a manual RE-RUN of an +# older main run: github.ref is refs/heads/main but github.sha is that run's +# original (now stale) commit, and the bumper would force-repin every caller to +# it. So establish the current main tip first; the guard below decides whether +# this run is genuinely stale. +# Don't pipe into `cut`: the pipeline would report cut's status, so a failed +# ls-remote (network blip, remote hiccup) yields an EMPTY main_tip that then +# compares unequal to github.sha and silently no-ops the whole fleet bump as if +# the run were stale. A lookup we couldn't perform is not evidence of staleness +# — fail loudly. +# `--refs` plus an exact-refname match, not just the first line: git matches ref +# patterns at component boundaries, so a branch literally named +# `foo/refs/heads/main` also matches this pattern and could be the line a bare +# `%%\t*` parse consumes. +if ! ls_remote=$(git ls-remote --refs origin refs/heads/main); then + echo "::error::Could not look up the current main tip (git ls-remote failed)" + exit 1 +fi +main_tip=$(awk '$2 == "refs/heads/main" { print $1; exit }' <<<"$ls_remote") +if [[ -z "$main_tip" ]]; then + echo "::error::git ls-remote returned no SHA for refs/heads/main" + exit 1 +fi + +if [[ "$main_tip" != "$GITHUB_SHA" ]]; then + # main has moved on. That alone does NOT make this run stale: the push trigger + # is path-filtered to the watched surface, so an unrelated commit landing in + # the seconds between this run's trigger and this check starts NO run of its + # own. Skipping on a bare SHA mismatch would discard the only run for this + # change and leave every caller frozen — the exact pin-drift this fleet exists + # to prevent. (That bare-mismatch skip is what five of the entrypoints did + # before this script existed.) + # What actually distinguishes a stale re-run is that the watched surface has + # CHANGED since: then a later commit did touch a filtered path and does have + # its own run, which will pin the newer content. + if ! git fetch --depth=1 origin main; then + echo "::error::Could not fetch the current main tip to compare $WATCHED" + exit 1 + fi + # Prove FETCH_HEAD resolves to a real commit BEFORE reading objects out of it. + # `git rev-parse --verify --quiet` returns empty both for "that path is absent + # from this tree" and for "this revision could not be resolved at all" (a + # partial fetch, an unexpected FETCH_HEAD state). Without this guard the second + # case is indistinguishable from deletion and would exit 0 as "decommissioned" + # — the same "a lookup we couldn't perform is not evidence" anti-pattern the + # ls-remote guard above rejects, but silently no-opping the whole fleet. With + # it, an empty tip_blob genuinely means absent-from-tree. + if ! main_tip=$(git rev-parse --verify --quiet "FETCH_HEAD^{commit}"); then + echo "::error::Fetched main but FETCH_HEAD does not resolve to a commit — cannot compare $WATCHED" + exit 1 + fi + tip_blob=$(git rev-parse --verify --quiet "FETCH_HEAD:$WATCHED" || true) + # HEAD is this run's own checkout, so it must resolve. An empty here_blob means + # $WATCHED is absent at github.sha, which is the deletion-commit case the final + # guard handles — don't let it fall into the "changed since" branch below and + # be reported as a stale re-run, which would be a misleading log for a real + # decommission. + if ! here_blob=$(git rev-parse --verify --quiet "HEAD:$WATCHED"); then + echo "::warning::$WATCHED is absent at this run's own commit $GITHUB_SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + emit false "$NEW_SHA" + exit 0 + fi + # Multi-path fleets pin a second surface: the asset directory the reusable + # loads its prompts/scripts/briefs from at run time. Compare its TREE OID the + # same way — see the COUPLED TO THE PATH FILTER note on the re-point below. + tip_assets="" + here_assets="" + if [[ -n "$WATCHED_ASSETS" ]]; then + tip_assets=$(git rev-parse --verify --quiet "FETCH_HEAD:$WATCHED_ASSETS" || true) + here_assets=$(git rev-parse --verify --quiet "HEAD:$WATCHED_ASSETS" || true) + fi + if [[ -z "$tip_blob" ]] && { [[ -z "$WATCHED_ASSETS" ]] || [[ -z "$tip_assets" ]]; }; then + # ::warning:: not a bare echo: if the reusable was deleted while live callers + # still pin it, they all hard-fail at startup and a silently-green run here + # is the fleet's only chance to say so. + echo "::warning::$WATCHED no longer exists on main ($main_tip) — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + emit false "$NEW_SHA" + exit 0 + fi + if [[ "$tip_blob" != "$here_blob" ]] || [[ "$tip_assets" != "$here_assets" ]]; then + echo "github.sha $GITHUB_SHA is behind main ($main_tip) and the watched surface changed since — stale run/re-run; the newer commit has its own run. Nothing to bump" + emit false "$NEW_SHA" + exit 0 + fi + # Pin callers to the VERIFIED TIP, not to this run's stale github.sha. We have + # just proved every watched object is byte-identical at both, so the tip is the + # same reusable content at a commit that is actually current — pinning the + # older SHA would hand every caller a non-tip commit (and, on a + # land-then-revert, re-pin them backwards). + # + # COUPLED TO THE PATH FILTER — this is only sound because every entry in the + # fleet's `paths:` trigger is covered by the comparison above. A single-path + # fleet passes WATCHED alone; a fleet whose filter also watches an asset + # directory (cursor-review / groom / pr-size) MUST pass WATCHED_ASSETS too, or + # the comparison silently under-verifies and callers get pinned to a tip whose + # other relevant content was never compared. If you widen a fleet's filter + # again, widen the inputs here in the same change. + echo "main moved to $main_tip since $GITHUB_SHA, but the watched surface is unchanged — this run is still the only one for that change; pinning callers to $main_tip and proceeding" + NEW_SHA="$main_tip" +fi + +# The push path filter also matches a commit that DELETES the reusable workflow; +# bumping callers to a SHA where it is gone would break every caller. Deletion +# means decommissioning — no-op. +# Test "$WATCHED", not a second copy of the literal path: two literals drift +# apart on a rename, and the stale one would name a file that never exists, +# making this test always true and the whole fleet a permanent silent no-op. +if [[ ! -f "$WATCHED" ]]; then + echo "::warning::$WATCHED absent at this SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + emit false "$NEW_SHA" + exit 0 +fi +if [[ -n "$WATCHED_ASSETS" ]] && [[ ! -d "$WATCHED_ASSETS" ]]; then + echo "::warning::$WATCHED_ASSETS absent at this SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + emit false "$NEW_SHA" + exit 0 +fi + +emit true "$NEW_SHA" diff --git a/.github/bump-callers/tests/test_preflight.sh b/.github/bump-callers/tests/test_preflight.sh new file mode 100755 index 0000000..f505e23 --- /dev/null +++ b/.github/bump-callers/tests/test_preflight.sh @@ -0,0 +1,235 @@ +#!/usr/bin/env bash +# +# Functional tests for the bump-fleet staleness/decommission preflight +# (preflight.sh). +# +# The guard this script replaces is copy-pasted into every bump-*-callers.yml +# entrypoint, where nothing can test it — and the copies drifted: five skip on a +# bare tip mismatch (throwing away the only run for a change), one compares blobs +# but forgets to re-point the pin at the verified tip, and only one of the two +# content-comparing copies covers the asset directory that multi-path fleets also +# watch. Now that the logic is one script, it gets the same treatment as +# bump-callers.sh: drive the REAL script and assert the behavior each entrypoint +# depends on. +# +# No network and no GitHub: each case builds a throwaway bare repo as `origin` +# and a clone as the run's workspace, drives preflight.sh with $GITHUB_OUTPUT +# pointed at a temp file, and asserts the exit code, both step outputs, and the +# presence/absence of ::error::/::warning:: annotations. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PREFLIGHT="${SCRIPT_DIR}/../preflight.sh" +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +PASS=0 +FAIL=0 +ok() { PASS=$((PASS+1)); echo " ok: $1"; } +bad() { FAIL=$((FAIL+1)); echo " FAIL: $1"; } +check(){ if eval "$2"; then ok "$1"; else bad "$1 [$2]"; fi; } + +# The paths a real multi-path fleet (groom) watches. Any pair would do; using the +# real ones keeps the fixtures recognizable. +WATCHED_PATH=".github/workflows/groom.yml" +ASSETS_PATH=".github/groom" + +CASE=""; SRC=""; ORIGIN=""; WORKDIR=""; OUTFILE=""; OUT=""; RC=0; P=""; N="" + +# --- fixture: a bare repo as `origin`, a clone as the run workspace ----------- +# SRC is the scratch tree used to author commits; ORIGIN is the bare repo the +# script's `git ls-remote` / `git fetch` talk to; WORKDIR is the checkout the +# script runs in (Actions' own `actions/checkout` of github.sha). `file://` URLs +# so `git fetch --depth=1` really does a shallow fetch instead of being ignored +# as a local-path clone. +new_case() { + echo + echo "== $2 ==" + CASE="${WORK}/$1" + SRC="${CASE}/src"; ORIGIN="${CASE}/origin.git"; WORKDIR="${CASE}/work" + OUTFILE="${CASE}/gh_output" + mkdir -p "$SRC" + git -c init.defaultBranch=main init -q "$SRC" + git -C "$SRC" config user.email preflight-tests@example.invalid + git -C "$SRC" config user.name 'Preflight Tests' + mkdir -p "${SRC}/.github/workflows" "${SRC}/${ASSETS_PATH}" + printf 'name: Groom\non:\n workflow_call:\n' > "${SRC}/${WATCHED_PATH}" + printf 'finder brief v1\n' > "${SRC}/${ASSETS_PATH}/finder.md" + printf 'unrelated file\n' > "${SRC}/README.md" + git -C "$SRC" add -A + git -C "$SRC" commit -qm 'initial' + git clone -q --bare "$SRC" "$ORIGIN" + git -C "$SRC" remote add origin "file://${ORIGIN}" + clone_work +} + +clone_work() { rm -rf "$WORKDIR"; git clone -q "file://${ORIGIN}" "$WORKDIR"; } + +# Commit whatever is staged in SRC and advance origin/main to it. +push_src() { + git -C "$SRC" add -A + git -C "$SRC" commit -qm "$1" + git -C "$SRC" push -q origin main +} + +origin_tip() { git -C "$ORIGIN" rev-parse main; } +work_head() { git -C "$WORKDIR" rev-parse HEAD; } + +# Run the real script in WORKDIR. Extra `VAR=value` arguments are appended to the +# environment (so a case can add WATCHED_ASSETS or override anything above). +run_preflight() { + : > "$OUTFILE" + # shellcheck disable=SC2034 # OUT/RC/P/N are read by the `check` assertions below + OUT=$(cd "$WORKDIR" && env \ + WATCHED="$WATCHED_PATH" \ + GITHUB_OUTPUT="$OUTFILE" \ + "$@" bash "$PREFLIGHT" 2>&1) + RC=$? + P=$(grep '^proceed=' "$OUTFILE" 2>/dev/null | tail -1 | cut -d= -f2-) + N=$(grep '^new_sha=' "$OUTFILE" 2>/dev/null | tail -1 | cut -d= -f2-) +} + +# --------------------------------------------------------------------------- +new_case decoy 'a decoy refs/heads/foo/refs/heads/main is not the main tip' +# `git ls-remote origin refs/heads/main` matches ref patterns at COMPONENT +# BOUNDARIES, so a branch literally named foo/refs/heads/main also matches — and +# it sorts FIRST (f < m), so a bare "first line" parse consumes it. Point the +# decoy at an older commit: if the parse picks it up, the script thinks main +# moved and logs the re-point. It must not. +printf 'unrelated file, edited\n' > "${SRC}/README.md" +push_src 'second commit' +clone_work +DECOY_SHA=$(git -C "$ORIGIN" rev-parse 'main^') +git -C "$ORIGIN" update-ref refs/heads/foo/refs/heads/main "$DECOY_SHA" +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha is the real main tip" "[[ \"$N\" == \"$TIP\" ]]" +check "decoy ref was not consumed" "! grep -q \"main moved\" <<<\"\$OUT\"" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" +check "no ::warning::" "! grep -q \"::warning::\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case lsremote 'a failed ls-remote is a hard error, not a silent no-op' +# "A lookup we couldn't perform is not evidence of staleness" — the whole reason +# the tip is not parsed through a pipe. An unreachable origin must fail the job, +# never quietly leave every caller un-bumped. +git -C "$WORKDIR" remote set-url origin "file://${CASE}/does-not-exist.git" +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" +check "exit 1" "[[ $RC -eq 1 ]]" +check "::error:: annotation" "grep -q \"::error::\" <<<\"\$OUT\"" +check "proceed is not true" "[[ \"$P\" != \"true\" ]]" + +# --------------------------------------------------------------------------- +new_case stale_blob 'stale re-run: the watched workflow changed on main since' +# A later commit touched the watched path, so that commit has its own run and +# will pin the newer content. This run is a stale re-run — skip. +BEHIND=$(work_head) +printf 'name: Groom\non:\n workflow_call:\n inputs: {}\n' > "${SRC}/${WATCHED_PATH}" +push_src 'edit the watched workflow' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "logged as a stale run" "grep -q \"stale run/re-run\" <<<\"\$OUT\"" +check "no ::warning::" "! grep -q \"::warning::\" <<<\"\$OUT\"" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case repoint 'main moved but the watched surface is unchanged: re-point' +# An unrelated commit landed between the trigger and this check. The path filter +# means it started NO run of its own, so skipping here would discard the only run +# for this change. Proceed — but pin to the VERIFIED TIP, not this run's stale +# github.sha (which would hand callers a non-tip commit). +BEHIND=$(work_head) +printf 'unrelated file, edited\n' > "${SRC}/README.md" +push_src 'unrelated commit' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha re-pointed to the tip" "[[ \"$N\" == \"$TIP\" ]]" +check "new_sha is not the stale sha" "[[ \"$N\" != \"$BEHIND\" ]]" +check "re-point logged" "grep -q \"pinning callers to $TIP\" <<<\"\$OUT\"" +check "no ::warning::" "! grep -q \"::warning::\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case decommissioned 'the watched workflow was deleted on main: decommissioned' +# The push path filter also matches the commit that DELETES the reusable. Bumping +# callers to a SHA where it is gone would break every one of them, so this is a +# warned no-op, not a bump. +BEHIND=$(work_head) +git -C "$SRC" rm -rq "${WATCHED_PATH}" "${ASSETS_PATH}" +push_src 'retire the groom reusable' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "::warning:: annotation" "grep -q \"::warning::\" <<<\"\$OUT\"" +check "decommission message" "grep -q \"no longer exists on main\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case assets 'multi-path fleet: only the asset dir changed — still stale' +# The case a naive single-blob port gets WRONG. groom/cursor-review/pr-size +# callers pin an asset directory too (the briefs/prompts/scripts loaded at run +# time), so a commit that touches only that directory DOES have its own run — +# comparing $WATCHED alone would re-point and double-bump. +BEHIND=$(work_head) +printf 'finder brief v2\n' > "${SRC}/${ASSETS_PATH}/finder.md" +push_src 'edit the finder brief only' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "logged as a stale run" "grep -q \"stale run/re-run\" <<<\"\$OUT\"" +# ...and the same commit WITHOUT WATCHED_ASSETS is the under-verifying single-path +# comparison, which proves the widened comparison is what makes the difference. +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" +check "single-path config would re-point" "[[ \"$P\" == \"true\" ]]" + +# --------------------------------------------------------------------------- +new_case own_commit 'the watched workflow is absent at this run OWN commit' +# An unresolvable HEAD:$WATCHED is the deletion-commit case; it must not fall +# into the "changed since" branch and be reported as a stale re-run. +git -C "$SRC" rm -rq "${WATCHED_PATH}" +push_src 'retire the groom reusable' +clone_work +BEHIND=$(work_head) +printf 'unrelated file, edited again\n' > "${SRC}/README.md" +push_src 'unrelated commit on top' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "::warning:: annotation" "grep -q \"::warning::\" <<<\"\$OUT\"" +check "own-commit message" "grep -q \"absent at this run.s own commit\" <<<\"\$OUT\"" +check "not reported as stale" "! grep -q \"stale run/re-run\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case current_tip 'happy path: this run IS the current main tip' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha is github.sha" "[[ \"$N\" == \"$TIP\" ]]" +check "no fetch/compare happened" "! grep -q \"main moved\" <<<\"\$OUT\"" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" +check "no ::warning::" "! grep -q \"::warning::\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case missing_dir 'current tip, but the watched asset dir is gone locally' +# The final decommission check tests "$WATCHED"/"$WATCHED_ASSETS" — the +# VARIABLES, never a second copy of the literal path. Two literals drift apart on +# a rename and the stale one names a file that never exists, making the test +# always true and the whole fleet a permanent silent no-op. +git -C "$SRC" rm -rq "${ASSETS_PATH}" +push_src 'retire the groom briefs' +clone_work +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "::warning:: names the assets" "grep -q \"::warning::${ASSETS_PATH} absent\" <<<\"\$OUT\"" + +echo +echo "== $PASS passed, $FAIL failed ==" +[[ $FAIL -eq 0 ]] diff --git a/.github/workflows/test-bump-callers.yml b/.github/workflows/test-bump-callers.yml index 31a5de3..8009ba1 100644 --- a/.github/workflows/test-bump-callers.yml +++ b/.github/workflows/test-bump-callers.yml @@ -1,7 +1,8 @@ name: Test bump-callers script -# Runs the functional tests + shellcheck for the shared caller-bump script -# (.github/bump-callers/bump-callers.sh). That one script drives the SHA-bump +# Runs the functional tests + shellcheck for the shared caller-bump scripts +# (.github/bump-callers/bump-callers.sh and its preflight.sh staleness guard). +# That one bump script drives the SHA-bump # fan-out for the cursor-review, cursor-review-auto-label, agents-md-integrity, # pr-size, pr-risk, assign-reviewers, groom AND detect-unreviewed-merge caller fleets, # so a regression here silently breaks every consumer @@ -49,8 +50,11 @@ jobs: with: persist-credentials: false - - name: ShellCheck the bump script + tests - run: shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh + - name: ShellCheck the bump scripts + tests + run: shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/preflight.sh .github/bump-callers/tests/test_bump_callers.sh .github/bump-callers/tests/test_preflight.sh - name: Run bump-callers functional tests run: bash .github/bump-callers/tests/test_bump_callers.sh + + - name: Run preflight functional tests + run: bash .github/bump-callers/tests/test_preflight.sh diff --git a/AGENTS.md b/AGENTS.md index 568bc36..d5225f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,8 +27,9 @@ python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v # bump-callers shell tests + lint (gh is stubbed; no network) -shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh +shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/preflight.sh .github/bump-callers/tests/test_bump_callers.sh .github/bump-callers/tests/test_preflight.sh bash .github/bump-callers/tests/test_bump_callers.sh +bash .github/bump-callers/tests/test_preflight.sh # run the AGENTS.md integrity checker against any repo tree python3 .github/agents-md-integrity/check_agents_md.py --root . @@ -71,8 +72,10 @@ tests — run the matching command above for whatever you touched. (decayed commit touches, assigner-parity globs, collaborator-only) and surgically rewrites just the reviewer lists for a drift PR. Tests in `tests/`. - `.github/bump-callers/` — `bump-callers.sh`, the ONE fleet-agnostic script - that opens SHA-bump PRs in consumer repos when a reusable workflow changes. - Tests in `tests/`. + that opens SHA-bump PRs in consumer repos when a reusable workflow changes, + plus `preflight.sh` (BE-6475), the ONE staleness/decommission guard that runs + ahead of it — `proceed` / `new_sha` step outputs, `WATCHED` + + `WATCHED_ASSETS` inputs. Tests in `tests/`. - `README.md` — the public workflow catalog: per-workflow purpose, the SHA-pin usage pattern, and the versioning policy. Keep its table in sync when you add a workflow. From 13c540f76d611f16d054ef0c2f294b019325e28f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 04:31:56 -0700 Subject: [PATCH 2/3] fix(bump-callers): harden preflight.sh inputs, tip fetch and decommission verdict (BE-6475) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor review panel findings on the phase-1 extraction. Behaviour-affecting fixes, all with functional coverage in tests/test_preflight.sh: - Decommission at the tip is now EITHER watched surface being gone, not both. Retirement is normally staged (delete the reusable, clean up its asset directory later), so the `&&` let the common case fall through to the "stale run/re-run" branch and exit green — suppressing the ::warning:: that is the fleet's only signal that live callers now hard-fail at startup. It also disagreed with the local -f/-d guards, which already used OR semantics. The message now names the surface that went away. - NEW_SHA is validated as a full 40-hex lowercase SHA. It is the one value never derived from a lookup, yet it is emitted verbatim into $GITHUB_OUTPUT and handed to bump-callers.sh's pin rewrite: a newline in it injects extra output lines, and an injected `proceed=true` would win over the `proceed=false` this script wrote. - HEAD is asserted equal to $GITHUB_SHA. Every "here" side is read from HEAD while every decision is keyed off GITHUB_SHA; a `ref:` override in the consuming checkout would have the script compare main against itself, so every comparison reads "unchanged" and every stale re-run proceeds. - The tip is fetched as `refs/heads/main`, not the bare name. Refspec resolution consults refs/tags/ BEFORE refs/heads/, and this repo routinely creates and force-moves major tags — a tag named `main` would silently become the FETCH_HEAD every comparison and the re-point run against. A tip that advances between the ls-remote and the fetch is now logged as the benign race it is. - WATCHED / WATCHED_ASSETS are rejected when glob-shaped or slash-terminated. The guidance to widen them "to match the fleet's paths: filter" points straight at `.github/groom/**`, which resolves to nothing and would make every comparison verify nothing behind a green run. - rev-parse lookups distinguish "absent from the tree" (exit 1) from "the lookup failed" (any other status); `|| true` collapsed both into a silent decommissioned/stale verdict. - An asset tree already gone at the run's own commit is a decommission, not a "changed since", matching the existing here_blob guard. Docs: the multi-path fleet list was wrong in both directions — pr-size IS multi-path, and agents-md-integrity and pr-risk were missing. Script comment and README now list all five and tell you to read the entrypoint's `paths:` rather than trust the list. Tests: 74 assertions (was 42), including staged-retirement in both orders, the local -f decommission branch, the tag-shadowing fixture, and the three input guards. Verified discriminating by mutation: reverting the OR semantics fails 4 assertions, reverting the branch refspec fails 3. The direction/ancestry check (a fetched tip that is BEHIND HEAD) is not fixed here: `--depth=1` leaves FETCH_HEAD parentless, so `merge-base --is-ancestor` would reject the legitimate re-point. Doing it right needs pr-risk's unshallow probe, which is exactly the phase-2 decision this PR defers by design. Deferred to a follow-up. --- .github/bump-callers/README.md | 33 +++- .github/bump-callers/preflight.sh | 166 ++++++++++++++++--- .github/bump-callers/tests/test_preflight.sh | 119 +++++++++++++ 3 files changed, 288 insertions(+), 30 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 9399276..deb8765 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -133,23 +133,40 @@ re-point that pins callers to the verified tip instead of a stale `github.sha`. | `NEW_SHA` | the candidate SHA, normally `github.sha` | | `GITHUB_SHA`, `GITHUB_OUTPUT` | provided by Actions | +Both watched paths are **literal paths, not the globs from the `paths:` filter** — +`.github/groom`, never `.github/groom/**` and never a trailing slash. A glob +resolves to nothing (`[[ -d '.github/groom/**' ]]` is false, +`git rev-parse 'HEAD:.github/groom/**'` is empty), so it would make every +comparison verify nothing and the fleet a permanent silent no-op. The script +rejects that shape up front rather than reporting it as a decommission, and it +likewise rejects a `NEW_SHA` that is not a full 40-character lowercase SHA (it is +emitted verbatim into `$GITHUB_OUTPUT`, so a newline in it injects output lines) +and a `HEAD` that is not `GITHUB_SHA` (a `ref:` override in the consuming +checkout would have it compare main against itself). + | Output (step output) | | |---|---| | `proceed` | `true` → run `bump-callers.sh`; `false` → stale or decommissioned, do nothing | | `new_sha` | the SHA to pin — `NEW_SHA`, or the verified main tip when the run was re-pointed forward | Both outputs are written on **every** exit-0 path. The script exits non-zero only -for a lookup it could not perform (failed `ls-remote`, failed fetch, unresolvable -`FETCH_HEAD`): a lookup we couldn't perform is not evidence of staleness, so it -fails loudly rather than silently no-opping the fleet. +for an input it cannot trust (the shape checks above) or a lookup it could not +perform (failed `ls-remote`, failed fetch, unresolvable `FETCH_HEAD`, a +`rev-parse` that *failed* rather than reporting absence): neither is evidence of +staleness, so it fails loudly rather than silently no-opping the fleet. **A multi-path fleet must pass `WATCHED_ASSETS`.** The re-point is only sound because every entry in the fleet's `paths:` trigger is covered by the comparison -— for `cursor-review` / `groom` / `pr-size` that includes the asset directory the -reusable loads its prompts/scripts/briefs from at run time. Compare `WATCHED` -alone on one of those and a commit touching only the assets reads as "unchanged", -so callers get pinned to a tip whose other relevant content was never verified. -If you widen a fleet's path filter, widen these inputs in the same change. +— for `agents-md-integrity` (`.github/agents-md-integrity/**`), `cursor-review` +(`.github/cursor-review/**`), `groom` (`.github/groom/**`) and `pr-size` +(`scripts/check-pr-size/**`) that includes the asset directory the reusable loads +its prompts/scripts/briefs from at run time. (`pr-risk` is multi-path too, but its +filter also carries `:(exclude)` entries that one `WATCHED_ASSETS` string cannot +express — see the note below.) Compare `WATCHED` alone on one of those and a +commit touching only the assets reads as "unchanged", so callers get pinned to a +tip whose other relevant content was never verified. Read the entrypoint's +`paths:` rather than trusting this list, and if you widen a fleet's path filter, +widen these inputs in the same change. Consumption is two steps — the guard, then the bump gated on its output: diff --git a/.github/bump-callers/preflight.sh b/.github/bump-callers/preflight.sh index 5f056f0..2a896fd 100755 --- a/.github/bump-callers/preflight.sh +++ b/.github/bump-callers/preflight.sh @@ -30,14 +30,16 @@ # # Required environment: # WATCHED Repo-relative path of the watched reusable workflow file -# (e.g. .github/workflows/groom.yml). +# (e.g. .github/workflows/groom.yml). A LITERAL path, never a +# `paths:`-filter glob — see the shape validation below. # NEW_SHA The candidate commit to pin callers to (normally github.sha). -# GITHUB_SHA This run's own commit (provided by Actions). +# Must be a full 40-character lowercase SHA. +# GITHUB_SHA This run's own commit (provided by Actions). Must match HEAD. # GITHUB_OUTPUT Step-output file (provided by Actions). # Optional: # WATCHED_ASSETS Watched asset directory (e.g. .github/groom) for a fleet whose # `paths:` filter has more than one entry. Empty/unset means the -# fleet is single-path. +# fleet is single-path. Also a literal path. # # Outputs (written to $GITHUB_OUTPUT on every exit-0 path): # proceed "true" → the caller should run bump-callers.sh @@ -45,9 +47,11 @@ # new_sha the SHA to pin callers to — NEW_SHA, or the verified main tip when # this run was re-pointed forward (see the re-point block below) # -# Exits non-zero ONLY for a lookup we could not perform (failed ls-remote, failed -# fetch, unresolvable FETCH_HEAD). A lookup we couldn't perform is not evidence -# of staleness — it fails loudly rather than silently no-opping the fleet. +# Exits non-zero ONLY for an input we cannot trust (malformed SHA, glob-shaped +# watched path, a HEAD that is not GITHUB_SHA) or a lookup we could not perform +# (failed ls-remote, failed fetch, unresolvable FETCH_HEAD, a rev-parse that +# failed rather than reporting absence). Neither is evidence of staleness — it +# fails loudly rather than silently no-opping the fleet. # # Run from the repository root (the final decommission check tests the run's own # checked-out tree). @@ -59,6 +63,78 @@ set -euo pipefail : "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" WATCHED_ASSETS="${WATCHED_ASSETS-}" +# --- input shape validation -------------------------------------------------- +# A watched path is a LITERAL repo-relative path, never the glob from the fleet's +# `paths:` filter. Every instruction to "widen these inputs to match the fleet's +# path filter" points a maintainer straight at `.github/groom/**`, and a glob +# resolves to NOTHING here: `[[ -d '.github/groom/**' ]]` is false and +# `git rev-parse 'HEAD:.github/groom/**'` returns empty. A trailing slash +# (`.github/groom/`) does the same. Either one would make every comparison +# silently verify nothing and turn the whole fleet into a permanent no-op behind +# a green run — reject the shape instead of reporting it as a decommission. +validate_path() { # $1 = input name, $2 = value ("" = unset, skip) + [[ -n "$2" ]] || return 0 + if [[ "$2" == *'*'* || "$2" == *'?'* || "$2" == *'['* ]]; then + echo "::error::$1 must be a literal path, not a glob (got '$2') — pass the directory itself, e.g. .github/groom, not .github/groom/**" + exit 1 + fi + if [[ "$2" == */ ]]; then + echo "::error::$1 must not end in a slash (got '$2') — a trailing slash resolves to nothing, so the comparison would silently verify nothing" + exit 1 + fi +} +validate_path WATCHED "$WATCHED" +validate_path WATCHED_ASSETS "$WATCHED_ASSETS" + +# NEW_SHA is the one value here that is never derived from a lookup — every check +# below validates GITHUB_SHA/HEAD, while NEW_SHA is emitted verbatim into +# $GITHUB_OUTPUT and handed to bump-callers.sh's pin rewrite. Its SHAPE is +# therefore load-bearing: a value containing a newline injects extra output lines +# (an injected `proceed=true` would win over the `proceed=false` this script +# wrote), and any non-SHA silently becomes what every caller in the fleet is +# pinned to. +require_sha() { # $1 = input name, $2 = value + if [[ ! "$2" =~ ^[0-9a-f]{40}$ ]]; then + # Strip CR/LF before logging: an untrusted multi-line value would otherwise + # spread across the log as forged annotation lines of its own. + echo "::error::$1 must be a full 40-character lowercase commit SHA (got '${2//[$'\n'$'\r']/ }')" + exit 1 + fi +} +require_sha NEW_SHA "$NEW_SHA" +require_sha GITHUB_SHA "$GITHUB_SHA" + +# The staleness decision is keyed off $GITHUB_SHA, but the "here" side of every +# comparison below is read from HEAD (and from the working tree by the final +# -f/-d guards) — nothing otherwise asserts the two agree. A consuming job whose +# `actions/checkout` uses a `ref:` override, or any earlier step that moves HEAD, +# would have this script compare main against itself: every comparison reads +# "unchanged", so every stale re-run proceeds and re-points. +if ! head_sha=$(git rev-parse --verify --quiet 'HEAD^{commit}'); then + echo "::error::Could not resolve HEAD — preflight must run from the root of this run's own checkout" + exit 1 +fi +if [[ "$head_sha" != "$GITHUB_SHA" ]]; then + echo "::error::HEAD ($head_sha) is not this run's commit GITHUB_SHA ($GITHUB_SHA) — the checkout must not use a ref: override. Refusing to compare main against itself." + exit 1 +fi + +# Resolve a rev to an object id, distinguishing "absent from that tree" +# (rev-parse exit 1, empty result — a real answer this script acts on) from "the +# lookup itself failed" (any other status: a missing promisor object, a corrupt +# pack). A bare `|| true` collapses both into "absent", which turns a lookup we +# could not perform into a silent decommissioned/stale verdict — the same +# not-evidence anti-pattern the ls-remote guard below rejects. +RESOLVED="" +resolve_oid() { # $1 = rev; result in $RESOLVED, empty when absent from the tree + local rc=0 + RESOLVED=$(git rev-parse --verify --quiet "$1") || rc=$? + if (( rc > 1 )); then + echo "::error::Could not look up $1 (git rev-parse exited $rc) — refusing to read a failed lookup as a deletion" + exit 1 + fi +} + # Both outputs are written on EVERY exit-0 path, so a consuming step never reads # an empty `new_sha` off a skip. NEW_SHA is deliberately a step OUTPUT and not a # $GITHUB_ENV export: a step-level `env: NEW_SHA:` binding in the consuming step @@ -104,7 +180,15 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then # What actually distinguishes a stale re-run is that the watched surface has # CHANGED since: then a later commit did touch a filtered path and does have # its own run, which will pin the newer content. - if ! git fetch --depth=1 origin main; then + # Fetch the BRANCH ref explicitly. `git fetch origin main` resolves the bare + # name through the refspec rules, which consult `refs/tags/` BEFORE + # `refs/heads/` — and this repo routinely creates and force-moves major + # tags. A tag named `main` would shadow the branch, silently making an + # arbitrary tagged commit the FETCH_HEAD that the blob comparison and the + # re-point below both run against — and therefore what every caller gets + # pinned to. `refs/heads/main` can only ever be the branch, which is also the + # ref the exact-refname `ls-remote` match above resolved. + if ! git fetch --depth=1 origin refs/heads/main; then echo "::error::Could not fetch the current main tip to compare $WATCHED" exit 1 fi @@ -116,17 +200,27 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then # — the same "a lookup we couldn't perform is not evidence" anti-pattern the # ls-remote guard above rejects, but silently no-opping the whole fleet. With # it, an empty tip_blob genuinely means absent-from-tree. - if ! main_tip=$(git rev-parse --verify --quiet "FETCH_HEAD^{commit}"); then + if ! fetched_tip=$(git rev-parse --verify --quiet "FETCH_HEAD^{commit}"); then echo "::error::Fetched main but FETCH_HEAD does not resolve to a commit — cannot compare $WATCHED" exit 1 fi - tip_blob=$(git rev-parse --verify --quiet "FETCH_HEAD:$WATCHED" || true) - # HEAD is this run's own checkout, so it must resolve. An empty here_blob means - # $WATCHED is absent at github.sha, which is the deletion-commit case the final - # guard handles — don't let it fall into the "changed since" branch below and - # be reported as a stale re-run, which would be a misleading log for a real - # decommission. - if ! here_blob=$(git rev-parse --verify --quiet "HEAD:$WATCHED"); then + # A benign race, not an error: main can advance in the seconds between the + # ls-remote and the fetch. Compare against — and re-point to — the tip whose + # objects we actually read, and say so in the log. + if [[ "$fetched_tip" != "$main_tip" ]]; then + echo "main advanced from $main_tip to $fetched_tip between the tip lookup and the fetch — comparing against the fetched tip" + fi + main_tip="$fetched_tip" + resolve_oid "FETCH_HEAD:$WATCHED" + tip_blob="$RESOLVED" + # HEAD is this run's own checkout, so the lookup itself must succeed (a failure + # exits inside resolve_oid). An EMPTY here_blob means $WATCHED is absent at + # github.sha, which is the deletion-commit case the final guard handles — don't + # let it fall into the "changed since" branch below and be reported as a stale + # re-run, which would be a misleading log for a real decommission. + resolve_oid "HEAD:$WATCHED" + here_blob="$RESOLVED" + if [[ -z "$here_blob" ]]; then echo "::warning::$WATCHED is absent at this run's own commit $GITHUB_SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." emit false "$NEW_SHA" exit 0 @@ -137,14 +231,37 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then tip_assets="" here_assets="" if [[ -n "$WATCHED_ASSETS" ]]; then - tip_assets=$(git rev-parse --verify --quiet "FETCH_HEAD:$WATCHED_ASSETS" || true) - here_assets=$(git rev-parse --verify --quiet "HEAD:$WATCHED_ASSETS" || true) + resolve_oid "FETCH_HEAD:$WATCHED_ASSETS" + tip_assets="$RESOLVED" + resolve_oid "HEAD:$WATCHED_ASSETS" + here_assets="$RESOLVED" + # Same reasoning as the here_blob guard above: an asset tree that is already + # gone at this run's own commit is a decommission, not a "changed since". + if [[ -z "$here_assets" ]]; then + echo "::warning::$WATCHED_ASSETS is absent at this run's own commit $GITHUB_SHA — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + emit false "$NEW_SHA" + exit 0 + fi + fi + # EITHER watched surface being gone at the tip is a decommission — not both. + # Retirement is normally staged (delete the reusable, clean up its asset + # directory in a later commit), so an AND here would let the common case fall + # through to the "stale run/re-run" branch and exit green, suppressing the + # ::warning:: below. It would also disagree with the local -f/-d guards at the + # bottom of this script, which already treat either surface missing as a + # decommission — the same situation must not get two different verdicts + # depending on which branch reached it. + tip_gone="" + if [[ -z "$tip_blob" ]]; then + tip_gone="$WATCHED" + elif [[ -n "$WATCHED_ASSETS" ]] && [[ -z "$tip_assets" ]]; then + tip_gone="$WATCHED_ASSETS" fi - if [[ -z "$tip_blob" ]] && { [[ -z "$WATCHED_ASSETS" ]] || [[ -z "$tip_assets" ]]; }; then + if [[ -n "$tip_gone" ]]; then # ::warning:: not a bare echo: if the reusable was deleted while live callers # still pin it, they all hard-fail at startup and a silently-green run here # is the fleet's only chance to say so. - echo "::warning::$WATCHED no longer exists on main ($main_tip) — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." + echo "::warning::$tip_gone no longer exists on main ($main_tip) — treating as decommissioned and bumping nothing. If any caller still pins it, retire those callers." emit false "$NEW_SHA" exit 0 fi @@ -162,9 +279,14 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then # COUPLED TO THE PATH FILTER — this is only sound because every entry in the # fleet's `paths:` trigger is covered by the comparison above. A single-path # fleet passes WATCHED alone; a fleet whose filter also watches an asset - # directory (cursor-review / groom / pr-size) MUST pass WATCHED_ASSETS too, or - # the comparison silently under-verifies and callers get pinned to a tip whose - # other relevant content was never compared. If you widen a fleet's filter + # directory MUST pass WATCHED_ASSETS too, or the comparison silently + # under-verifies and callers get pinned to a tip whose other relevant content + # was never compared. Today that is agents-md-integrity + # (.github/agents-md-integrity/**), cursor-review (.github/cursor-review/**), + # groom (.github/groom/**) and pr-size (scripts/check-pr-size/**) — plus + # pr-risk, whose filter also carries `:(exclude)` entries that a single + # WATCHED_ASSETS string cannot express (see the header). Read the entrypoint's + # `paths:` rather than trusting this list, and if you widen a fleet's filter # again, widen the inputs here in the same change. echo "main moved to $main_tip since $GITHUB_SHA, but the watched surface is unchanged — this run is still the only one for that change; pinning callers to $main_tip and proceeding" NEW_SHA="$main_tip" diff --git a/.github/bump-callers/tests/test_preflight.sh b/.github/bump-callers/tests/test_preflight.sh index f505e23..1f9ce07 100755 --- a/.github/bump-callers/tests/test_preflight.sh +++ b/.github/bump-callers/tests/test_preflight.sh @@ -169,6 +169,32 @@ check "proceed=false" "[[ \"$P\" == \"false\" ]]" check "::warning:: annotation" "grep -q \"::warning::\" <<<\"\$OUT\"" check "decommission message" "grep -q \"no longer exists on main\" <<<\"\$OUT\"" +# --------------------------------------------------------------------------- +new_case decommissioned_staged 'the workflow was deleted but its assets survive' +# The real retirement sequence: delete the reusable now, clean up its asset +# directory in a later commit. EITHER surface being gone at the tip has to read +# as a decommission — an AND would let this (the common case) fall through to the +# stale branch and exit green, suppressing the ::warning:: that is the fleet's +# only chance to say that live callers now hard-fail at startup. +BEHIND=$(work_head) +git -C "$SRC" rm -rq "${WATCHED_PATH}" +push_src 'retire the groom reusable, briefs cleaned up later' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "::warning:: annotation" "grep -q \"::warning::\" <<<\"\$OUT\"" +check "names the deleted workflow" "grep -q \"::warning::${WATCHED_PATH} no longer exists on main\" <<<\"\$OUT\"" +check "not reported as stale" "! grep -q \"stale run/re-run\" <<<\"\$OUT\"" +# ...and the mirror image: the asset dir goes first, the workflow file survives. +new_case decommissioned_assets 'the asset dir was deleted but the workflow survives' +BEHIND=$(work_head) +git -C "$SRC" rm -rq "${ASSETS_PATH}" +push_src 'retire the groom briefs, workflow cleaned up later' +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "names the deleted asset dir" "grep -q \"::warning::${ASSETS_PATH} no longer exists on main\" <<<\"\$OUT\"" + # --------------------------------------------------------------------------- new_case assets 'multi-path fleet: only the asset dir changed — still stale' # The case a naive single-blob port gets WRONG. groom/cursor-review/pr-size @@ -230,6 +256,99 @@ check "exit 0" "[[ $RC -eq 0 ]]" check "proceed=false" "[[ \"$P\" == \"false\" ]]" check "::warning:: names the assets" "grep -q \"::warning::${ASSETS_PATH} absent\" <<<\"\$OUT\"" +# --------------------------------------------------------------------------- +new_case missing_file 'current tip, but the watched workflow is gone locally' +# The fleet's PRIMARY decommission path — the `[[ ! -f "$WATCHED" ]]` guard — is +# reached when the DELETING commit is itself the tip, so none of the "main moved" +# comparisons run at all. Its WATCHED_ASSETS variant is covered above; this is +# the one every single-path fleet depends on. +git -C "$SRC" rm -rq "${WATCHED_PATH}" +push_src 'retire the groom reusable at the tip' +clone_work +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=false" "[[ \"$P\" == \"false\" ]]" +check "::warning:: names the file" "grep -q \"::warning::${WATCHED_PATH} absent\" <<<\"\$OUT\"" +check "no fetch/compare happened" "! grep -q \"main moved\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case tag_shadow 'a TAG named main does not shadow refs/heads/main' +# `git fetch origin main` resolves the bare name through refs/tags/ BEFORE +# refs/heads/, and this repo routinely creates and force-moves major tags. +# A tag named `main` would silently become the FETCH_HEAD that the comparison and +# the re-point run against — i.e. what every caller gets pinned to. Point the tag +# at a commit whose watched file DIFFERS, so consuming it reads as "stale" while +# the correct branch fetch re-points. +BEHIND=$(work_head) +printf 'name: Groom\non:\n workflow_call:\n inputs: {}\n' > "${SRC}/${WATCHED_PATH}" +push_src 'a decoy commit that edits the watched workflow' +DECOY_SHA=$(origin_tip) +git -C "$SRC" checkout -q -- . 2>/dev/null || true +printf 'name: Groom\non:\n workflow_call:\n' > "${SRC}/${WATCHED_PATH}" +printf 'unrelated file, edited\n' > "${SRC}/README.md" +push_src 'restore the watched workflow, edit something unrelated' +TIP=$(origin_tip) +git -C "$ORIGIN" tag main "$DECOY_SHA" +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "re-pointed to the BRANCH tip" "[[ \"$N\" == \"$TIP\" ]]" +check "did not consume the tag" "[[ \"$N\" != \"$DECOY_SHA\" ]]" +check "not reported as stale" "! grep -q \"stale run/re-run\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case glob_input 'a glob-shaped watched path is rejected, not read as absent' +# The README tells maintainers to widen these inputs to match the fleet's +# `paths:` filter, which points straight at `.github/groom/**`. A glob resolves +# to NOTHING — `[[ -d '.github/groom/**' ]]` is false and +# `git rev-parse 'HEAD:.github/groom/**'` is empty — so left unvalidated it makes +# every comparison verify nothing and the whole fleet a permanent silent no-op +# behind a green run. +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="${ASSETS_PATH}/**" +check "exit 1" "[[ $RC -eq 1 ]]" +check "::error:: annotation" "grep -q \"::error::WATCHED_ASSETS must be a literal path\" <<<\"\$OUT\"" +check "proceed is not true" "[[ \"$P\" != \"true\" ]]" +# A trailing slash is the same footgun with no glob character in sight. +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="${ASSETS_PATH}/" +check "trailing slash: exit 1" "[[ $RC -eq 1 ]]" +check "trailing slash: ::error::" "grep -q \"must not end in a slash\" <<<\"\$OUT\"" +check "trailing slash: not proceed" "[[ \"$P\" != \"true\" ]]" + +# --------------------------------------------------------------------------- +new_case bad_new_sha 'a malformed NEW_SHA is rejected before it reaches an output' +# NEW_SHA is the one value never derived from a lookup: it is emitted verbatim +# into $GITHUB_OUTPUT and handed to bump-callers.sh's pin rewrite. A newline in +# it injects extra output lines — and an injected `proceed=true` would win over +# the `proceed=false` this script wrote, since a consuming step reads the LAST +# value of a repeated key. +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA=$'0000000000000000000000000000000000000000\nproceed=true' +check "exit 1" "[[ $RC -eq 1 ]]" +check "::error:: annotation" "grep -q \"::error::NEW_SHA must be a full 40\" <<<\"\$OUT\"" +check "no injected proceed=true" "! grep -q \"^proceed=true\$\" \"$OUTFILE\"" +check "nothing written to output" "[[ ! -s \"$OUTFILE\" ]]" +# A short/abbreviated SHA is the other way a bad pin reaches every caller. +run_preflight GITHUB_SHA="$TIP" NEW_SHA="${TIP:0:12}" +check "short sha: exit 1" "[[ $RC -eq 1 ]]" +check "short sha: not proceed" "[[ \"$P\" != \"true\" ]]" + +# --------------------------------------------------------------------------- +new_case head_mismatch 'HEAD that is not GITHUB_SHA is a hard error' +# Every "here" side is read from HEAD while every decision is keyed off +# GITHUB_SHA. A consuming checkout with a `ref:` override (or any earlier step +# that moves HEAD) would have the script compare main against itself: every +# comparison reads "unchanged", so every stale re-run proceeds and re-points. +BEHIND=$(work_head) +printf 'name: Groom\non:\n workflow_call:\n inputs: {}\n' > "${SRC}/${WATCHED_PATH}" +push_src 'edit the watched workflow' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" # HEAD is still $BEHIND +check "exit 1" "[[ $RC -eq 1 ]]" +check "::error:: annotation" "grep -q \"::error::HEAD ($BEHIND) is not this run\" <<<\"\$OUT\"" +check "proceed is not true" "[[ \"$P\" != \"true\" ]]" + echo echo "== $PASS passed, $FAIL failed ==" [[ $FAIL -eq 0 ]] From 7e8490a985982f4a5ccc5bb47f18d9960593cbbe Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 11 Aug 2026 12:11:02 -0700 Subject: [PATCH 3/3] ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) preflight.sh's tip-mismatch branch never checked that the fetched main tip DESCENDS from GITHUB_SHA. If main moved backwards — a force-push, a revert-reset, or a stale replica answering ls-remote/fetch — a legitimate fresh run was silently wrong in both directions: with the watched content differing at the older tip it exited GREEN as a stale re-run ("the newer commit has its own run", about an older commit), freezing the fleet; with the content identical, the re-point pinned every caller BACKWARDS. Gate both on `git merge-base --is-ancestor HEAD FETCH_HEAD`, loud (::error:: + exit 1), placed after the FETCH_HEAD verification and before any blob/tree comparison. A lookup that contradicts history is not evidence of staleness. The guard needs real history: against a --depth=1 graft --is-ancestor returns false even for a legitimate forward move, so the naive one-liner would hard-fail every re-point. Probe for .git/shallow and add --unshallow only then (--unshallow errors on a complete clone). Keeps refs/heads/main rather than pr-risk's bare `main` — the tag-shadowing comment above it explains why. Settles the swap decision the README deferred (BE-6670): direction guard adopted for all fleets; pr-risk's `git rev-list` test deliberately not ported — it compensates for pr-risk having no re-point, and the re-point already pins the verified TIP, which on a land-then-revert is the revert commit. Tests: four new cases — force-moved-backwards with content differing and with content identical, a happy-path re-point from a SHALLOW workdir (Actions' real checkout shape, which the full-clone helper never exercised), and land-then-revert. 94 passed, 0 failed; shellcheck clean. * fix(bump-callers): measure the direction of BOTH tip moves in preflight (BE-6675) Review follow-ups on the ancestry direction guard. The guard only proved the fetched tip descends from THIS RUN's commit, which is the weaker half: `ls-remote` and the fetch are two round trips, and a rewind landing in that window — or a stale replica answering the second one — can return a commit older than the tip just reported yet still ahead of this run. That sails through the HEAD check and gets compared and pinned anyway. Require the observed tip to be an ancestor of the fetched one before logging the move as an advance; a `--is-ancestor` that cannot be performed at all lands there too, and correctly (in a forward move the observed tip is present by definition, so its absence is itself evidence). Also: * Probe shallowness with `git rev-parse --is-shallow-repository` instead of statting `$(git rev-parse --git-dir)/shallow`. In a linked worktree the marker lives in the common git dir while `--git-dir` names the per-worktree one, so the hand-rolled form false-negatives there and skips the deepening. The verdict still came out right (a plain fetch into a shallow clone reaches the existing boundary), but that rested the guards on fetch boundary behavior rather than on `--unshallow`. Also drops the empty-array expansion, which is an unbound variable under `set -u` on bash 3.2. * Read `$fetched_tip`, the OID already resolved and verified, rather than the mutable `FETCH_HEAD` ref, in the ancestry check and both tree lookups. * Name both readings in the two `::error::` lines instead of asserting a diagnosis `--is-ancestor` cannot distinguish. * Record that not porting `git rev-list` settles land-then-revert only, and is not a claim that object comparison expresses a rev-list pathspec: an over-broad `WATCHED_ASSETS` on an EXCLUDING fleet lets a commit that starts no run of its own read as "changed since" and freeze that fleet green. * Drop the hardcoded ~6 MB / 137-commit cost figure that would drift. Tests: two cases that fail against the previous script — a rewind between the two lookups (a `git` shim fires it in the real window; previously exit 0, and callers pinned to the rewound tip), and a shallow LINKED WORKTREE asserting the fetch actually deepened. Suite verified under stock bash 3.2 as well. --- .github/bump-callers/README.md | 66 ++++++- .github/bump-callers/preflight.sh | 132 ++++++++++++-- .github/bump-callers/tests/test_preflight.sh | 175 +++++++++++++++++++ 3 files changed, 352 insertions(+), 21 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index deb8765..838dc93 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -150,11 +150,41 @@ checkout would have it compare main against itself). | `new_sha` | the SHA to pin — `NEW_SHA`, or the verified main tip when the run was re-pointed forward | Both outputs are written on **every** exit-0 path. The script exits non-zero only -for an input it cannot trust (the shape checks above) or a lookup it could not +for an input it cannot trust (the shape checks above), a lookup it could not perform (failed `ls-remote`, failed fetch, unresolvable `FETCH_HEAD`, a -`rev-parse` that *failed* rather than reporting absence): neither is evidence of +`rev-parse` that *failed* rather than reporting absence), or an answer that +contradicts history (the **direction guard** below): none of those is evidence of staleness, so it fails loudly rather than silently no-opping the fleet. +**The direction guard.** When main has moved, the fetched tip must *descend from* +this run's commit before anything is compared against it or pinned to it. If main +moved BACKWARDS — a force-push, a revert-reset, or a stale replica answering the +tip lookup — both outcomes are silently wrong: with the watched content differing +at the older commit the run reads as a stale re-run and exits green ("the newer +commit has its own run" — about an *older* commit), freezing every caller behind a +run that will never come; with the content identical, the re-point pins the whole +fleet BACKWARDS. So `git merge-base --is-ancestor` gates both, and a failure is an +`::error::`, not a skip. + +It is measured **twice**, because descending from this run's commit is the weaker +half. `ls-remote` and the fetch are two round trips, and a rewind landing in that +window — or a stale replica answering the second one — can hand back a commit that +is older than the tip just reported yet still *ahead* of this run, which sails +through a HEAD-only check and gets compared and pinned anyway. So the tip +`ls-remote` reported must be an ancestor of the fetched one before that move is +logged as an advance, and the fetched one must be a descendant of this run's +commit before anything is read out of it. + +Both need real history to answer: against a `--depth=1` graft `--is-ancestor` +returns false even for a legitimate forward move, so the fetch adds `--unshallow` +when the checkout is shallow (which `actions/checkout` makes it) — `--unshallow` +errors out on an already-complete clone, hence a probe rather than an +unconditional flag. The probe is `git rev-parse --is-shallow-repository`, not a +stat of `$(git rev-parse --git-dir)/shallow`: inside a **linked worktree** that +marker lives in the common git dir while `--git-dir` names the per-worktree one, +so the hand-rolled form false-negatives exactly there and silently skips the +deepening the guard is supposed to rest on. + **A multi-path fleet must pass `WATCHED_ASSETS`.** The re-point is only sound because every entry in the fleet's `paths:` trigger is covered by the comparison — for `agents-md-integrity` (`.github/agents-md-integrity/**`), `cursor-review` @@ -168,6 +198,15 @@ tip whose other relevant content was never verified. Read the entrypoint's `paths:` rather than trusting this list, and if you widen a fleet's path filter, widen these inputs in the same change. +They must not be **wider** than the filter either, which is the direction an +excluding fleet gets wrong. A commit touching only an excluded path (pr-risk's +`scripts/pr-risk/tests`, its `README.md`) starts no run of its own, but it does +change the tree OID of an over-broad `WATCHED_ASSETS` — so this run reports "the +watched surface changed since", skips green as a stale re-run, and waits on a +later run that will never exist. An exclusion is a reason to narrow the inputs, or +to leave that fleet on its own guard; never to point `WATCHED_ASSETS` at the whole +directory. + Consumption is two steps — the guard, then the bump gated on its output: ```yaml @@ -194,12 +233,23 @@ step reads it through its own `env:` binding. That is deliberate: a step-level write would be silently overridden by the very binding it is meant to correct. > **The entrypoints still carry their inline copies.** Swapping them over to this -> script is a separate change. `bump-pr-risk-callers.yml` needs a decision rather -> than a swap: its copy has hardening this one does not implement — a -> `git rev-list` "did a later *commit* touch a watched path" test (rather than a -> net-content comparison) and an is-ancestor check that refuses to pin an -> orphaned commit — so folding it in, or keeping that fleet on its own guard, has -> to be chosen deliberately, not by deleting the checks. +> script is a separate change. `bump-pr-risk-callers.yml` carried two checks this +> script did not, and **BE-6670 decided both** rather than leaving the swap to +> choose: +> +> - Its **is-ancestor check is adopted here, for every fleet** (BE-6675) — see +> the direction guard above. It was never pr-risk-specific. +> - Its **`git rev-list` "did a later *commit* touch a watched path" test is +> deliberately not ported.** It exists because pr-risk has no re-point, so a +> land-then-revert (net content change of zero) makes that fleet call this run +> the only one for the change and pin callers *backwards* to `github.sha`. The +> re-point already answers that case by pinning the verified tip — which on a +> land-then-revert is the revert commit, i.e. forward. Adding rev-list on top +> would only skip a run whose content the tip still needs pinned. That settles +> land-then-revert; it is not a claim that comparing objects expresses +> everything a rev-list *pathspec* can. Swapping an excluding fleet across means +> narrowing its inputs to what its filter really watches (above), not adding +> rev-list back. ## How the pin rewrite is scoped (and why it asserts afterwards) diff --git a/.github/bump-callers/preflight.sh b/.github/bump-callers/preflight.sh index 2a896fd..b59b4b3 100755 --- a/.github/bump-callers/preflight.sh +++ b/.github/bump-callers/preflight.sh @@ -24,9 +24,30 @@ # bump-detect-unreviewed-merge-callers.yml semantics (PR #117) — exact-refname # tip parse, FETCH_HEAD verification, `$WATCHED`-variable deletion guard, and the # NEW_SHA re-point — generalized to multi-path fleets. Nothing consumes it yet; -# swapping the entrypoints over is a separate change, and the pr-risk swap in -# particular has to decide what to do with that fleet's extra checks rather than -# drop them. +# swapping the entrypoints over is a separate change. +# +# What to do with bump-pr-risk-callers.yml's two extra checks was that swap's one +# open decision. BE-6670 made it, and both halves are settled: +# * The is-ancestor DIRECTION GUARD is ADOPTED here, for every fleet (BE-6675). +# It was never pr-risk-specific: without it, a main that moved BACKWARDS — +# a force-push, a revert-reset, or a stale replica answering the tip lookup — +# either reads as a stale re-run and freezes the whole fleet behind a green +# run, or re-points every caller to the OLDER tip. See the guard below. +# * The `git rev-list` "did a later COMMIT touch a watched path" test is +# deliberately NOT ported. It exists because pr-risk has no re-point: there, a +# land-then-revert nets to zero content change, so a content comparison calls +# this run the only one for the change and pins callers BACKWARDS to +# github.sha. The re-point below already answers that case — it pins the +# verified TIP, which on a land-then-revert IS the revert commit, i.e. +# forward. Porting rev-list on top would only add a stricter staleness +# verdict that skips a run whose content the tip still needs pinned. +# That decision is about the land-then-revert case, and it is NOT a claim +# that an object comparison expresses everything a rev-list PATHSPEC can: +# pr-risk's filter carries `:(exclude)` entries, and an over-broad watched +# surface here has its own failure mode — see the COUPLED TO THE PATH FILTER +# note on the re-point below. Swapping an excluding fleet onto this script +# means narrowing its inputs to what the filter really watches (or keeping +# it on its own guard), not adding rev-list. # # Required environment: # WATCHED Repo-relative path of the watched reusable workflow file @@ -48,10 +69,12 @@ # this run was re-pointed forward (see the re-point block below) # # Exits non-zero ONLY for an input we cannot trust (malformed SHA, glob-shaped -# watched path, a HEAD that is not GITHUB_SHA) or a lookup we could not perform +# watched path, a HEAD that is not GITHUB_SHA), a lookup we could not perform # (failed ls-remote, failed fetch, unresolvable FETCH_HEAD, a rev-parse that -# failed rather than reporting absence). Neither is evidence of staleness — it -# fails loudly rather than silently no-opping the fleet. +# failed rather than reporting absence), or an answer that contradicts history +# (a fetched main tip that does not descend from this run's commit, or from the +# tip the ls-remote reported moments earlier). None of those is evidence of +# staleness — it fails loudly rather than silently no-opping the fleet. # # Run from the repository root (the final decommission check tests the run's own # checked-out tree). @@ -187,8 +210,39 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then # arbitrary tagged commit the FETCH_HEAD that the blob comparison and the # re-point below both run against — and therefore what every caller gets # pinned to. `refs/heads/main` can only ever be the branch, which is also the - # ref the exact-refname `ls-remote` match above resolved. - if ! git fetch --depth=1 origin refs/heads/main; then + # ref the exact-refname `ls-remote` match above resolved. (bump-pr-risk's copy + # fetches a bare `main` — do not "align" with it; that is the shadowing bug.) + # + # Fetch REAL history, not just the tip. The direction guards below ask whether + # one commit DESCENDS from another, and `git merge-base --is-ancestor` cannot + # answer that against a `--depth=1` graft: a parentless FETCH_HEAD makes even a + # legitimate forward move read as not-an-ancestor, so the naive one-liner would + # hard-fail every re-point (verified empirically, spike BE-6670). Deepening is + # what makes the guards sound. actions/checkout clones shallow, and + # `--unshallow` errors out on an already-complete clone — hence the probe + # rather than an unconditional flag. + # Ask git whether the REPOSITORY is shallow; don't stat `$(git rev-parse + # --git-dir)/shallow`. Inside a linked worktree `--git-dir` is that worktree's + # own directory while the `shallow` marker lives in the COMMON git dir, so the + # hand-rolled probe false-negatives there and skips the deepening entirely. + # The verdict still comes out right today — a plain fetch into a shallow clone + # sends the new commits down to the existing boundary, so HEAD stays reachable + # (measured; test_preflight.sh's shallow_worktree case) — but that leaves the + # guards resting on fetch's boundary behavior instead of on the `--unshallow` + # this comment says makes them sound, and one refspec away from the shape that + # does break it. `--is-shallow-repository` is correct in both layouts (and, + # being a non-empty array either way, the form below is also safe under + # `set -u` on bash 3.2, where an empty `"${arr[@]}"` is an unbound variable). + # Cost: bounded by this repo's own history — a full clone of it measures well + # under 2s today — and this branch only runs on a tip mismatch. Deliberately no + # `--deepen` ceiling: a partial deepening cannot answer the ancestry question, + # which is the whole point of fetching here. The caller job's `timeout-minutes` + # is the backstop for an origin that hangs. + fetch_args=(origin refs/heads/main) + if [[ "$(git rev-parse --is-shallow-repository)" == "true" ]]; then + fetch_args=(--unshallow "${fetch_args[@]}") + fi + if ! git fetch "${fetch_args[@]}"; then echo "::error::Could not fetch the current main tip to compare $WATCHED" exit 1 fi @@ -204,14 +258,56 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then echo "::error::Fetched main but FETCH_HEAD does not resolve to a commit — cannot compare $WATCHED" exit 1 fi - # A benign race, not an error: main can advance in the seconds between the - # ls-remote and the fetch. Compare against — and re-point to — the tip whose - # objects we actually read, and say so in the log. + # main can move in the seconds between the ls-remote and the fetch. Moving + # FORWARD is a benign race: compare against — and re-point to — the tip whose + # objects we actually read, and say so in the log. But "advanced" is a + # DIRECTION, and it has to be measured rather than assumed: the fetch can + # equally land on a commit OLDER than the one ls-remote reported moments + # earlier (a force-push landing in that window, or a stale read replica + # answering the fetch). The HEAD check below does NOT catch that — a rewind to + # a commit that is still ahead of this run passes it, and the fleet is then + # silently pinned to a tip we already know main was ahead of. So require the + # observed tip to be an ancestor of the fetched one. + # A `--is-ancestor` that cannot be performed at all (exit >1: the observed tip + # is not even present locally after the fetch) lands here too, and correctly — + # in a forward move the observed tip IS an ancestor of the fetched one and + # therefore present, so its absence is itself evidence the move was not + # forward. The message names both readings. if [[ "$fetched_tip" != "$main_tip" ]]; then + if ! git merge-base --is-ancestor "$main_tip" "$fetched_tip"; then + echo "::error::the fetched main tip ($fetched_tip) does not descend from the tip the lookup reported moments earlier ($main_tip) — main moved backwards, or a stale replica answered the fetch (or the ancestry could not be checked at all); refusing to compare or re-point" + exit 1 + fi echo "main advanced from $main_tip to $fetched_tip between the tip lookup and the fetch — comparing against the fetched tip" fi main_tip="$fetched_tip" - resolve_oid "FETCH_HEAD:$WATCHED" + # Refuse to act on a tip that is not a descendant of this run's commit: a + # force-push back, a revert-reset, or a stale replica answering the lookup. + # Proceeding either way is wrong, and BOTH ways are silent today — if the + # watched content DIFFERS at the older tip, the comparison below reports a + # false "stale run/re-run; the newer commit has its own run" and exits green, + # freezing every caller behind a run that will never come; if it MATCHES, the + # re-point hands `new_sha` the OLDER tip and pins the whole fleet BACKWARDS. + # Loud, not a green skip — a lookup that contradicts history is not evidence of + # staleness. (Ported from bump-pr-risk-callers.yml, BE-6670.) + # + # Unlike resolve_oid, this deliberately does NOT split "not an ancestor" (exit + # 1) from "the check itself errored" (exit >1). There the distinction is + # load-bearing because the two verdicts DIVERGE — absence exits 0 and skips + # silently, a failed lookup must not. Here they converge: both are an + # `::error::` and exit 1, so splitting them would only reword a log line — and + # the line already names both readings rather than asserting the diagnosis. + # + # Compare the OID we resolved and verified above, not the FETCH_HEAD ref: that + # ref is mutable, and anything rewriting it in between (a retry wrapper, a + # concurrent git operation in the same workspace, a hook) would let a commit + # other than the one checked here be the one whose blobs are compared and + # emitted as new_sha. Same reason the reads below take $fetched_tip. + if ! git merge-base --is-ancestor "$head_sha" "$fetched_tip"; then + echo "::error::the fetched main tip ($main_tip) does not descend from this run's commit $GITHUB_SHA — main moved backwards, or a stale replica answered (or the ancestry could not be checked at all); refusing to compare or re-point" + exit 1 + fi + resolve_oid "$fetched_tip:$WATCHED" tip_blob="$RESOLVED" # HEAD is this run's own checkout, so the lookup itself must succeed (a failure # exits inside resolve_oid). An EMPTY here_blob means $WATCHED is absent at @@ -231,7 +327,7 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then tip_assets="" here_assets="" if [[ -n "$WATCHED_ASSETS" ]]; then - resolve_oid "FETCH_HEAD:$WATCHED_ASSETS" + resolve_oid "$fetched_tip:$WATCHED_ASSETS" tip_assets="$RESOLVED" resolve_oid "HEAD:$WATCHED_ASSETS" here_assets="$RESOLVED" @@ -288,6 +384,16 @@ if [[ "$main_tip" != "$GITHUB_SHA" ]]; then # WATCHED_ASSETS string cannot express (see the header). Read the entrypoint's # `paths:` rather than trusting this list, and if you widen a fleet's filter # again, widen the inputs here in the same change. + # + # The inputs must not be WIDER than the filter either, and that direction is + # the one an excluding fleet gets wrong. A commit touching only an EXCLUDED + # path (pr-risk's `scripts/pr-risk/tests`, its `README.md`) starts no run of + # its own — but it does change the tree OID of an over-broad WATCHED_ASSETS, + # so the comparison above reports "the watched surface changed since" and this + # run skips green as a stale re-run, waiting on a later run that will never + # exist. That freezes the fleet exactly as a bare tip mismatch used to. An + # exclusion is therefore a reason to narrow the inputs (or leave that fleet on + # its own guard), never to point WATCHED_ASSETS at the whole directory. echo "main moved to $main_tip since $GITHUB_SHA, but the watched surface is unchanged — this run is still the only one for that change; pinning callers to $main_tip and proceeding" NEW_SHA="$main_tip" fi diff --git a/.github/bump-callers/tests/test_preflight.sh b/.github/bump-callers/tests/test_preflight.sh index 1f9ce07..15205a8 100755 --- a/.github/bump-callers/tests/test_preflight.sh +++ b/.github/bump-callers/tests/test_preflight.sh @@ -66,6 +66,25 @@ new_case() { clone_work() { rm -rf "$WORKDIR"; git clone -q "file://${ORIGIN}" "$WORKDIR"; } +# Actions' own `actions/checkout` is SHALLOW by default, so the deepening arm of +# the script's `--unshallow` probe is the one that runs in production — and a +# full-clone fixture never exercises it. This is the variant that does. +clone_work_shallow() { + rm -rf "$WORKDIR" + git clone -q --depth=1 "file://${ORIGIN}" "$WORKDIR" +} + +# The same shallow checkout, but reached through a LINKED WORKTREE. Git keeps the +# `shallow` marker in the COMMON git dir, while `git rev-parse --git-dir` inside a +# linked worktree answers with that worktree's own directory — so a hand-rolled +# `[[ -f "$(git rev-parse --git-dir)/shallow" ]]` probe reports "not shallow" +# here and skips the deepening the ancestry guard depends on. +clone_work_shallow_worktree() { + rm -rf "$WORKDIR" "${CASE}/common" + git clone -q --depth=1 "file://${ORIGIN}" "${CASE}/common" + git -C "${CASE}/common" worktree add -q --detach "$WORKDIR" HEAD +} + # Commit whatever is staged in SRC and advance origin/main to it. push_src() { git -C "$SRC" add -A @@ -155,6 +174,162 @@ check "new_sha is not the stale sha" "[[ \"$N\" != \"$BEHIND\" ]]" check "re-point logged" "grep -q \"pinning callers to $TIP\" <<<\"\$OUT\"" check "no ::warning::" "! grep -q \"::warning::\" <<<\"\$OUT\"" +# --------------------------------------------------------------------------- +new_case backwards_differs 'origin main force-moved BACKWARDS, watched content differs' +# The direction guard. Nothing above ever checks that the fetched tip DESCENDS +# from this run's commit, so a main that moved backwards (force-push, a +# revert-reset, or a stale replica answering the tip lookup) lands in the content +# comparison with the older commit on the "tip" side. Here that content differs, +# so the pre-guard script logged "the newer commit has its own run" — about a +# commit that is OLDER — and exited GREEN, freezing every caller behind a run +# that will never come. It has to be loud instead. +BACK=$(origin_tip) +printf 'name: Groom\non:\n workflow_call:\n inputs: {}\n' > "${SRC}/${WATCHED_PATH}" +push_src 'edit the watched workflow' +clone_work # the run's checkout IS the new tip +TIP=$(work_head) +git -C "$ORIGIN" update-ref refs/heads/main "$BACK" # force-push main backwards +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" +check "exit 1" "[[ $RC -ne 0 ]]" +check "::error:: names the direction" "grep -q \"::error::.*does not descend\" <<<\"\$OUT\"" +check "NOT the silent stale verdict" "! grep -q \"stale run/re-run\" <<<\"\$OUT\"" +check "proceed is not false" "[[ \"$P\" != \"false\" ]]" +check "nothing written to output" "[[ ! -s \"$OUTFILE\" ]]" + +# --------------------------------------------------------------------------- +new_case backwards_equal 'origin main force-moved BACKWARDS, watched content identical' +# The other half of the same bug, and the worse one. With the watched surface +# byte-identical at both commits, the pre-guard script fell straight through to +# the re-point and handed `new_sha` the OLDER tip — pinning every caller in the +# fleet BACKWARDS, which is exactly what the re-point exists to avoid. +BACK=$(origin_tip) +printf 'unrelated file, edited\n' > "${SRC}/README.md" +push_src 'a commit that touches only the unrelated file' +clone_work # the run's checkout IS the new tip +TIP=$(work_head) +git -C "$ORIGIN" update-ref refs/heads/main "$BACK" +run_preflight GITHUB_SHA="$TIP" NEW_SHA="$TIP" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 1" "[[ $RC -ne 0 ]]" +check "::error:: names the direction" "grep -q \"::error::.*does not descend\" <<<\"\$OUT\"" +check "did not re-point backwards" "[[ \"$N\" != \"$BACK\" ]]" +check "no backwards re-point logged" "! grep -q \"pinning callers to $BACK\" <<<\"\$OUT\"" +check "nothing written to output" "[[ ! -s \"$OUTFILE\" ]]" + +# --------------------------------------------------------------------------- +new_case shallow_repoint 'a SHALLOW workdir still re-points: the --unshallow arm' +# The guard above needs REAL history: `git merge-base --is-ancestor` against a +# `--depth=1` graft returns false even for a legitimate forward move, so a naive +# port would hard-fail every re-point in production — where actions/checkout is +# shallow. This is the happy path run against that real-world shape. +printf 'unrelated file, v1\n' > "${SRC}/README.md" +push_src 'a second commit, so a depth=1 clone really truncates' +clone_work_shallow +BEHIND=$(work_head) +check "workdir really is shallow" "[[ -f \"${WORKDIR}/.git/shallow\" ]]" +printf 'unrelated file, v2\n' > "${SRC}/README.md" +push_src 'unrelated commit' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha re-pointed to the tip" "[[ \"$N\" == \"$TIP\" ]]" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" + +# --------------------------------------------------------------------------- +new_case shallow_worktree 'a shallow LINKED WORKTREE deepens too, and re-points' +# Same happy path as above, one layout over: the checkout is a linked worktree of +# a shallow clone. Git stores the `shallow` marker in the COMMON git dir, but +# `git rev-parse --git-dir` inside a linked worktree answers with the per-worktree +# directory — so probing for `$(git rev-parse --git-dir)/shallow` false-negatives +# exactly here and skips the deepening. Asking git (`--is-shallow-repository`) is +# correct in both layouts. +# The verdict below still comes out right either way (a plain fetch into a shallow +# clone sends the new commits down to the existing boundary, so HEAD stays +# reachable) — which is precisely why this needs an assertion on the DEEPENING and +# not just on the outputs. Left unfixed, the guard's soundness quietly depends on +# that boundary behavior instead of on the `--unshallow` its comment says makes it +# sound, and the shape that does break it (fetch grafting a parentless tip, which +# is what the original `--depth=1` fetch did) is one refspec away. +printf 'unrelated file, v1\n' > "${SRC}/README.md" +push_src 'a second commit, so a depth=1 clone really truncates' +clone_work_shallow_worktree +BEHIND=$(work_head) +check "the marker is NOT in --git-dir" \ + "[[ ! -f \"\$(git -C \"$WORKDIR\" rev-parse --git-dir)/shallow\" ]]" +check "but the repo really is shallow" \ + "[[ \"\$(git -C \"$WORKDIR\" rev-parse --is-shallow-repository)\" == true ]]" +printf 'unrelated file, v2\n' > "${SRC}/README.md" +push_src 'unrelated commit' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha re-pointed to the tip" "[[ \"$N\" == \"$TIP\" ]]" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" +check "the fetch really did deepen" \ + "[[ \"\$(git -C \"$WORKDIR\" rev-parse --is-shallow-repository)\" == false ]]" + +# --------------------------------------------------------------------------- +new_case rewound_between 'main REWOUND between the tip lookup and the fetch' +# The direction guard above only proves the fetched tip descends from THIS RUN's +# commit — it says nothing about the tip `ls-remote` reported moments earlier. A +# rewind that lands on a commit still AHEAD of this run therefore sails through +# it: the objects compared, and the SHA every caller is pinned to, come from a +# commit main was already known to be ahead of. Measure the direction of that +# move too. +# The fixture stages C0 (this run) → B → A on origin, then rewinds main to B in +# the window between the two lookups, via a `git` shim on PATH that fires right +# after the ls-remote. B still descends from C0, so ONLY the observed-tip +# comparison can catch this. +BEHIND=$(work_head) +printf 'unrelated file, b\n' > "${SRC}/README.md" +push_src 'commit B' +REWOUND_TO=$(origin_tip) +printf 'unrelated file, a\n' > "${SRC}/README.md" +push_src 'commit A — what ls-remote reports' +SHIM="${CASE}/shim"; mkdir -p "$SHIM" +REAL_GIT="$(command -v git)" +cat > "${SHIM}/git" < "${SRC}/${WATCHED_PATH}" +push_src 'land a watched change' +printf 'name: Groom\non:\n workflow_call:\n' > "${SRC}/${WATCHED_PATH}" +push_src 'revert it' +TIP=$(origin_tip) +run_preflight GITHUB_SHA="$BEHIND" NEW_SHA="$BEHIND" WATCHED_ASSETS="$ASSETS_PATH" +check "exit 0" "[[ $RC -eq 0 ]]" +check "proceed=true" "[[ \"$P\" == \"true\" ]]" +check "new_sha is the TIP" "[[ \"$N\" == \"$TIP\" ]]" +check "not this run's stale sha" "[[ \"$N\" != \"$BEHIND\" ]]" +check "no ::error::" "! grep -q \"::error::\" <<<\"\$OUT\"" + # --------------------------------------------------------------------------- new_case decommissioned 'the watched workflow was deleted on main: decommissioned' # The push path filter also matches the commit that DELETES the reusable. Bumping