diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 87adde0..838dc93 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,149 @@ 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 | + +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 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), 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` +(`.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. + +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 + - 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` 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) 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..b59b4b3 --- /dev/null +++ b/.github/bump-callers/preflight.sh @@ -0,0 +1,418 @@ +#!/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. +# +# 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 +# (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). +# 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. Also a literal 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 an input we cannot trust (malformed SHA, glob-shaped +# 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), 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). +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-}" + +# --- 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 +# 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. + # 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. (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 + # 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 ! 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 + # 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" + # 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 + # 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 + 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 + resolve_oid "$fetched_tip:$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 [[ -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::$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 + 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 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. + # + # 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 + +# 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..15205a8 --- /dev/null +++ b/.github/bump-callers/tests/test_preflight.sh @@ -0,0 +1,529 @@ +#!/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"; } + +# 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 + 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 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 +# 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 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 +# 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\"" + +# --------------------------------------------------------------------------- +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 ]] 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.