scripts/check-release-needed.sh

@@ -21,6 +21,27 @@ if [[ "${PRE_COMMIT_REMOTE_BRANCH:-}" != "$TARGET_BRANCH" ]]; then
   exit 0
 fi
 
+# What is actually being pushed, which is only HEAD for the common
+# `git push <remote> <current-branch>` case. pre-commit's pre-push hook-impl
+# exports the local sha of each pushed ref as PRE_COMMIT_TO_REF; a
+# `git push <remote> topic:main` from a different checkout would otherwise be
+# gated on the wrong tip — a false negative when HEAD is behind the pushed ref
+# (unreleased changes sail through), a false positive when it is ahead.
+# PRE_COMMIT_FROM_REF, the *remote's* current tip, is deliberately not used
+# anywhere here: the baseline is the last release tag, not what the remote
+# already has. Diffing from the remote tip would let an untagged
+# release-relevant commit already on main excuse the next push from cutting a
+# tag, which is precisely the drift this gate exists to catch.
+PUSHED_REF="${PRE_COMMIT_TO_REF:-HEAD}"
+
+# pre-commit passes an all-zeros sha (40 hex zeros under sha1, 64 under sha256)
+# as the "to" ref when the push deletes a branch. Nothing is being shipped, and
+# every rev-taking command below would fail on an unresolvable sha, so bail out
+# rather than turning a branch deletion into a confusing "could not diff".
+if [[ "$PUSHED_REF" =~ ^0+$ ]]; then
+  exit 0
+fi
+
 REPO_ROOT="$(git rev-parse --show-toplevel)"
 cd "$REPO_ROOT"
 
@@ -31,8 +52,10 @@ if [[ ! -f "$HOOKS_MANIFEST" ]]; then
 fi
 
 # Only vX.Y.Z release tags count as a baseline — an incidental checkpoint or
-# experiment tag reachable from HEAD must not shift the diff baseline.
-LAST_TAG="$(git describe --tags --abbrev=0 --match 'v[0-9]*.[0-9]*.[0-9]*' 2>/dev/null || true)"
+# experiment tag reachable from the pushed ref must not shift the diff baseline.
+# The tag is resolved from $PUSHED_REF, not HEAD, for the same reason the diff
+# is: a tag reachable only from HEAD is not part of the history being pushed.
+LAST_TAG="$(git describe --tags --abbrev=0 --match 'v[0-9]*.[0-9]*.[0-9]*' "$PUSHED_REF" 2>/dev/null || true)"
 
 if [[ -z "$LAST_TAG" ]]; then
   echo "FAIL: no release tag exists yet, but .pre-commit-hooks.yaml already exposes hooks to external consumers." >&2
@@ -68,16 +91,93 @@ add_release_path() {
   RELEASE_PATHS+=("$candidate")
 }
 
+# Emits one "<hook id><TAB><entry value>" line per hook so a rejected entry can
+# name the hook a human has to go fix. The id sits on its own line above its
+# entry: in YAML, so it is carried forward and then cleared; a hook that somehow
+# has no id still reports something printable rather than an empty name. Kept in
+# bash rather than awk: matching `[[:space:]]` inside a bracket expression is
+# reliable in bash's own globs but not in the BWK awk macOS ships. `read -r` with
+# a single variable is the trimmer — it strips leading and trailing whitespace
+# while preserving anything in between, so a multi-token entry survives intact
+# for the error message to quote back.
+manifest_entries() {
+  local line id="" value
+  while IFS= read -r line; do
+    # Drop the indentation and the optional list dash, so that `-   id: x` and
+    # `    entry: y` both reduce to the same bare "key: value" shape.
+    line="${line#"${line%%[![:space:]]*}"}"
+    if [[ "$line" == -* ]]; then
+      line="${line#-}"
+      line="${line#"${line%%[![:space:]]*}"}"
+    fi
+    case "$line" in
+      id:*)
+        read -r id <<< "${line#id:}"
+        ;;
+      entry:*)
+        read -r value <<< "${line#entry:}"
+        printf '%s\t%s\n' "${id:-(unnamed hook)}" "$value"
+        id=""
+        ;;
+    esac
+  done
+}
+
+# A hook's script is legitimate if it exists in the working tree *or* at
+# $LAST_TAG — the same union the pathspec itself spans. Checking per-scope
+# instead would reject exactly the case this gate exists to flag: a script
+# deleted since the tag while its entry survives (see the no -e filtering note
+# further down) is a real deletion to report, not a malformed manifest.
+entry_path_exists() {
+  local candidate="$1"
+  [[ -e "$candidate" ]] && return 0
+  git cat-file -e "$LAST_TAG:$candidate" 2>/dev/null && return 0
+  return 1
+}
+
 # $1 selects where the "does this hook bundle an assets/ tree?" guard looks:
 # "worktree" probes the filesystem, anything else is a rev whose tree is probed
 # with git plumbing. Reading entry lines from stdin keeps one derivation for
 # both the tagged manifest and the current one.
 collect_release_paths() {
-  local scope="$1" entry bundle_root
+  local scope="$1" line hook_id entry bundle_root where
   local -a tokens
-  while IFS= read -r entry; do
+  if [[ "$scope" == "worktree" ]]; then
+    where="the working tree's $HOOKS_MANIFEST"
+  else
+    where="$HOOKS_MANIFEST at $scope"
+  fi
+  while IFS= read -r line; do
+    hook_id="${line%%$'\t'*}"
+    entry="${line#*$'\t'}"
     read -ra tokens <<< "$entry"
     [[ ${#tokens[@]} -eq 0 ]] && continue
+    # ADR-0014 binds every entry to a bare script path and nothing else, because
+    # pre-commit rewrites only entry[0] into the hook-repo clone. That is a
+    # constraint nothing else enforces, and the sibling .pre-commit-config.yaml
+    # already ships the multi-token `bash <script>` shape one copy-paste away —
+    # so an entry like `bash scripts/foo.sh` would add "bash" as a pathspec that
+    # matches nothing and derive a bundle root of ".", dropping that hook's
+    # entire surface out of the gate silently. Both malformed shapes below fail
+    # loudly instead: silent degradation here is the same class of defect as the
+    # --config token already recorded in LESSONS.md.
+    if [[ ${#tokens[@]} -gt 1 ]]; then
+      echo "FAIL: hook '$hook_id' in $where has a multi-token entry: $entry" >&2
+      echo "  Why: pre-commit rewrites only entry[0] into the hook-repo clone, so every later" >&2
+      echo "       token resolves against the *consuming* repo and can never name a file this" >&2
+      echo "       repo ships — and this gate would derive its release paths from '${tokens[0]}'." >&2
+      echo "  Fix: make the entry a bare script path and have the script self-locate anything" >&2
+      echo "       else from \${BASH_SOURCE[0]} (see ADR-0014, 'Consequences')." >&2
+      exit 1
+    fi
+    if ! entry_path_exists "${tokens[0]}"; then
+      echo "FAIL: hook '$hook_id' in $where names a path that exists neither in the working tree nor at $LAST_TAG: ${tokens[0]}" >&2
+      echo "  Why: this gate derives its release-relevant pathspec from that path, so a name" >&2
+      echo "       that resolves to no file silently drops the hook's whole surface from the diff." >&2
+      echo "  Fix: point the entry at a script path this repo actually ships (see ADR-0014," >&2
+      echo "       'Consequences'); a bare command name is not a valid entry here." >&2
+      exit 1
+    fi
     add_release_path "${tokens[0]}"
     bundle_root="$(dirname "$(dirname "${tokens[0]}")")"
     [[ "$bundle_root" == "." ]] && continue
@@ -91,7 +191,7 @@ collect_release_paths() {
 }
 
 # The worktree alone is not enough: a path is release-relevant if it was part of
-# the contract at $LAST_TAG *or* is part of it at HEAD, so both trees have to be
+# the contract at $LAST_TAG *or* is part of it now, so both trees have to be
 # derived and unioned. Deriving only from the worktree meant that deleting a
 # hook's entire assets/ tree made the `-d` guard drop the path from the pathspec
 # altogether, and the deletion — which breaks every consumer at the next rev —
@@ -102,7 +202,7 @@ collect_release_paths() {
 # surface they cannot reach without a new tag. The union never over-fires on its
 # own, either — any manifest edit that makes the two disagree already changes
 # $HOOKS_MANIFEST, which is itself a release-relevant path.
-collect_release_paths worktree < <(sed -n 's/^[[:space:]]*entry:[[:space:]]*//p' "$HOOKS_MANIFEST")
+collect_release_paths worktree < <(manifest_entries < "$HOOKS_MANIFEST")
 
 # A missing manifest at the tag is legitimate (the manifest was added since) but
 # is indistinguishable from an unreadable tagged tree by its exit status alone,
@@ -110,20 +210,21 @@ collect_release_paths worktree < <(sed -n 's/^[[:space:]]*entry:[[:space:]]*//p'
 # shallow clone, a truncated fetch — fails closed exactly like a `git diff`
 # failure does, rather than silently degrading to worktree-only derivation.
 if MANIFEST_AT_TAG="$(git cat-file -p "$LAST_TAG:$HOOKS_MANIFEST" 2>/dev/null)"; then
-  collect_release_paths "$LAST_TAG" < <(printf '%s\n' "$MANIFEST_AT_TAG" | sed -n 's/^[[:space:]]*entry:[[:space:]]*//p')
+  collect_release_paths "$LAST_TAG" < <(printf '%s\n' "$MANIFEST_AT_TAG" | manifest_entries)
 elif ! git cat-file -e "$LAST_TAG^{tree}" 2>/dev/null; then
   echo "FAIL: could not read the tree at $LAST_TAG to determine which paths that release exposed." >&2
   echo "  Fix: ensure full tag history is available (e.g. git fetch --unshallow) and retry." >&2
   exit 1
 fi
 
-# No -e/existence filtering: a path deleted since $LAST_TAG is exactly the case
-# that must be caught (external consumers pinning the old tag would hit a
-# missing file), and `git diff` reports deletions fine without it existing at
-# HEAD. A git failure (e.g. a shallow clone missing $LAST_TAG's history) must
-# fail closed, not be swallowed into an empty, falsely-clean diff.
-if ! CHANGED="$(git diff --name-only "$LAST_TAG"..HEAD -- "${RELEASE_PATHS[@]}")"; then
-  echo "FAIL: could not diff $LAST_TAG..HEAD to check for release-relevant changes (see git error above)." >&2
+# No -e/existence filtering on the pathspec: a path deleted since $LAST_TAG is
+# exactly the case that must be caught (external consumers pinning the old tag
+# would hit a missing file), and `git diff` reports deletions fine without it
+# existing at the pushed ref. A git failure (e.g. a shallow clone missing
+# $LAST_TAG's history) must fail closed, not be swallowed into an empty,
+# falsely-clean diff.
+if ! CHANGED="$(git diff --name-only "$LAST_TAG".."$PUSHED_REF" -- "${RELEASE_PATHS[@]}")"; then
+  echo "FAIL: could not diff $LAST_TAG..$PUSHED_REF to check for release-relevant changes (see git error above)." >&2
   echo "  Fix: ensure full tag history is available (e.g. git fetch --unshallow) and retry." >&2
   exit 1
 fi

tests/test-check-release-needed.sh

@@ -52,9 +52,37 @@ make_tagged_fixture() {
   echo "$dir"
 }
 
+# Helper: a fixture whose manifest carries one malformed entry: at the tag *and*
+# at HEAD, plus a post-tag change to the file that entry was meant to cover.
+# Committing the bad entry before the tag is what makes the assertion sharp — an
+# edited manifest is itself release-relevant, so the gate would fail for the
+# wrong reason and hide a parser that degrades silently.
+make_malformed_fixture() {
+  local entry="$1" dir
+  dir="$(mktemp -d)"
+  (cd "$dir" && git init -q && git config user.email t@t.t && git config user.name t)
+  write_release_paths "$dir"
+  cat > "$dir/.pre-commit-hooks.yaml" <<EOF
+-   id: fake-size-check
+    entry: $entry
+    language: script
+EOF
+  (cd "$dir" && git add -A && git commit -q -m "initial" && git tag v1.0.0)
+  echo "v2" > "$dir/scripts/skill-size-check.sh"
+  (cd "$dir" && git add -A && git commit -q -m "change the file the malformed entry should cover")
+  echo "$dir"
+}
+
+# $3 is optional: pre-commit's PRE_COMMIT_TO_REF, the local sha being pushed.
+# Left off entirely, the variable stays unset and the script falls back to HEAD,
+# exactly as a plain `git push <remote> <current-branch>` behaves.
 run_check() {
   local dir="$1" branch="$2"
-  (cd "$dir" && PRE_COMMIT_REMOTE_BRANCH="$branch" bash "$SCRIPT" 2>&1)
+  if [[ $# -ge 3 ]]; then
+    (cd "$dir" && PRE_COMMIT_REMOTE_BRANCH="$branch" PRE_COMMIT_TO_REF="$3" bash "$SCRIPT" 2>&1)
+  else
+    (cd "$dir" && PRE_COMMIT_REMOTE_BRANCH="$branch" bash "$SCRIPT" 2>&1)
+  fi
 }
 
 CLEANUP_DIRS=()
@@ -268,6 +296,120 @@ else
   fail "reported only the manifest change and hid which shipped paths the retirement removed"
 fi
 
+# --- 15. A multi-token entry: is rejected loudly, not silently mis-parsed ---
+# ADR-0014 binds entries to a bare script path, but nothing enforced it, and the
+# sibling .pre-commit-config.yaml already ships `entry: bash <script>`. Under the
+# old parser tokens[0] became "bash": a pathspec matching nothing (which git diff
+# accepts in silence) and a bundle root of "." (skipped), so the hook's whole
+# surface dropped out of the gate and the post-tag change below diffed clean.
+echo ""
+echo "--- exits 1 naming the hook when an entry: carries more than one token ---"
+# The entry is quoted back verbatim, not just its first token: that is what makes
+# the diagnostic point at the argument the author has to remove, and what
+# distinguishes this from the unresolvable-path rejection test 16 covers.
+FIXTURE15="$(make_malformed_fixture "bash scripts/skill-size-check.sh")"; track "$FIXTURE15"
+OUT15=$(run_check "$FIXTURE15" "refs/heads/main" || true)
+if run_check "$FIXTURE15" "refs/heads/main" > /dev/null; then
+  fail "silently exited 0 on a multi-token entry, dropping that hook's paths from the gate"
+elif echo "$OUT15" | grep -q "fake-size-check" \
+  && echo "$OUT15" | grep -q "bash scripts/skill-size-check.sh" \
+  && echo "$OUT15" | grep -q "ADR-0014"; then
+  pass "rejects a multi-token entry, quoting it back and naming the hook and ADR-0014"
+else
+  fail "rejected the multi-token entry without naming the hook, the entry, and ADR-0014"
+fi
+
+# --- 16. An entry naming no file this repo ships is rejected loudly ---
+# The token-count guard alone still lets a single bare command name (`entry:
+# vale`, valid for language: system) through as a pathspec matching nothing.
+# Existence is checked against the union of the worktree and $LAST_TAG, so this
+# cannot misfire on the deletion cases tests 12-14 pin.
+echo ""
+echo "--- exits 1 naming the hook when an entry: names no file in the worktree or at the tag ---"
+FIXTURE16="$(make_malformed_fixture "vale")"; track "$FIXTURE16"
+OUT16=$(run_check "$FIXTURE16" "refs/heads/main" || true)
+if run_check "$FIXTURE16" "refs/heads/main" > /dev/null; then
+  fail "silently exited 0 on an entry that names no shipped file"
+elif echo "$OUT16" | grep -q "fake-size-check" && echo "$OUT16" | grep -q "ADR-0014"; then
+  pass "rejects an entry that resolves to no file, naming the hook and the ADR-0014 constraint"
+else
+  fail "rejected the unresolvable entry without naming the hook and the ADR-0014 constraint"
+fi
+
+# --- 17. The pushed ref, not HEAD, is what gets gated ---
+# pre-commit exports the local sha of each pushed ref as PRE_COMMIT_TO_REF.
+# `git push <remote> pushed-tip:main` from a checkout sitting on an older commit
+# is the false-negative direction: HEAD is still at the tag and diffs clean while
+# the branch actually landing on main carries an untagged, release-relevant
+# change. HEAD is reset back to the tag so the two genuinely differ.
+echo ""
+echo "--- exits 1 on a release-relevant change reachable only from PRE_COMMIT_TO_REF ---"
+FIXTURE17="$(make_tagged_fixture)"; track "$FIXTURE17"
+echo "v2" > "$FIXTURE17/scripts/skill-size-check.sh"
+(cd "$FIXTURE17" && git add -A && git commit -q -m "release-relevant change" \
+  && git branch pushed-tip && git reset -q --hard v1.0.0)
+OUT17=$(run_check "$FIXTURE17" "refs/heads/main" "pushed-tip" || true)
+if echo "$OUT17" | grep -q "skill-size-check.sh"; then
+  pass "gates the pushed ref's tip, not HEAD, when HEAD is behind it"
+else
+  fail "diffed HEAD instead of PRE_COMMIT_TO_REF and missed a release-relevant change"
+fi
+
+# --- 18. Neither the diff tip nor the tag baseline may come from a newer HEAD ---
+# The false-positive direction: HEAD has moved past a v2.0.0 that the pushed ref
+# never saw. Reading either end of the diff off HEAD fails a push that is clean
+# since its own baseline — diffing v2.0.0..HEAD flags HEAD's untagged commit, and
+# resolving the tag from HEAD while diffing pushed-tip flags v2.0.0's change.
+echo ""
+echo "--- exits 0 when the pushed ref is clean since its own tag but HEAD has moved on ---"
+FIXTURE18="$(make_tagged_fixture)"; track "$FIXTURE18"
+(cd "$FIXTURE18" && git branch pushed-tip)
+echo "v2" > "$FIXTURE18/scripts/skill-size-check.sh"
+(cd "$FIXTURE18" && git add -A && git commit -q -m "released change" && git tag v2.0.0)
+echo "v3" > "$FIXTURE18/scripts/skill-size-check.sh"
+(cd "$FIXTURE18" && git add -A && git commit -q -m "unreleased change on HEAD's line")
+if run_check "$FIXTURE18" "refs/heads/main" "pushed-tip" > /dev/null; then
+  pass "exits 0 for a pushed ref clean since the tag reachable from it, ignoring HEAD's line"
+else
+  fail "gated HEAD's tag or tip and falsely demanded a release for a clean pushed ref"
+fi
+
+# --- 19. A branch deletion is a no-op, not a confusing git failure ---
+# pre-commit sets PRE_COMMIT_TO_REF to an all-zeros sha when the push deletes a
+# branch. Nothing is being shipped, and the sha resolves to nothing, so without
+# an explicit guard the gate reports "could not diff" on an unrelated operation.
+echo ""
+echo "--- exits 0 when PRE_COMMIT_TO_REF is the all-zeros branch-deletion sha ---"
+FIXTURE19="$(make_tagged_fixture)"; track "$FIXTURE19"
+echo "v2" > "$FIXTURE19/scripts/skill-size-check.sh"
+(cd "$FIXTURE19" && git add -A && git commit -q -m "release-relevant change")
+if run_check "$FIXTURE19" "refs/heads/main" "0000000000000000000000000000000000000000" > /dev/null; then
+  pass "treats an all-zeros PRE_COMMIT_TO_REF as a branch deletion and exits 0"
+else
+  fail "turned a branch deletion into a failure instead of a no-op"
+fi
+
+# --- 20. The repo's own .pre-commit-hooks.yaml satisfies the entry constraints ---
+# The parser guards above are only safe to ship if the manifest actually in tree
+# passes them. It is replayed into a fixture (with the paths its entries name
+# created) rather than run against the real repo, which has no release tag yet.
+echo ""
+echo "--- accepts the real .pre-commit-hooks.yaml this repo ships ---"
+FIXTURE20="$(mktemp -d)"; track "$FIXTURE20"
+(cd "$FIXTURE20" && git init -q && git config user.email t@t.t && git config user.name t)
+cp "$REPO_ROOT/.pre-commit-hooks.yaml" "$FIXTURE20/.pre-commit-hooks.yaml"
+while IFS= read -r real_entry; do
+  mkdir -p "$FIXTURE20/$(dirname "$real_entry")"
+  echo "v1" > "$FIXTURE20/$real_entry"
+done < <(sed -n 's/^[[:space:]]*entry:[[:space:]]*//p' "$REPO_ROOT/.pre-commit-hooks.yaml")
+(cd "$FIXTURE20" && git add -A && git commit -q -m "initial" && git tag v1.0.0)
+OUT20=$(run_check "$FIXTURE20" "refs/heads/main" || true)
+if [[ -z "$OUT20" ]]; then
+  pass "parses every entry in the repo's real .pre-commit-hooks.yaml without complaint"
+else
+  fail "the repo's own .pre-commit-hooks.yaml no longer satisfies the entry constraints: $OUT20"
+fi
+
 echo ""
 echo "Results: $PASS passed, $FAIL failed"
 [[ $FAIL -eq 0 ]]