fix(git): push-guard — resolve 5 review blockers + a 6th self-found instance
ci/woodpecker/pr/ci Pipeline failed

Authored by installer-7; committed by mos-claude (installer-7 has no credential for this remote).
All five blockers from rev-974's review were REPRODUCED before any fix.

B1 mode 100644: the artifact was never executable, so an untouched clone could not run the
   suite at all (exit 126) — both prior 32/32 runs used a local exec bit set at creation.
   Fixed in the INDEX (git update-index --chmod=+x), plus verify-clean-clone.sh which asserts
   the mode via 'git ls-files -s' (not disk) and executes the suite DIRECTLY ('bash script'
   masks a missing bit). Negative control: chmod 644 makes the verifier refuse.
B2 mapfile < <(git diff) observed mapfile's status, not the producer's — a fatal pathspec
   error reported a clean scan, exit 0. pipefail governs PIPELINES; a process substitution is
   an async child whose status is never collected.
B3 OFF accepted from an untracked working-tree config, defeating the committed-artifact
   asymmetry — and write_config() wrote untracked configs, so every opt-out control asserted
   the forbidden provenance and passed green. OFF is now re-read from HEAD; untracked,
   staged-uncommitted, committed-symlink, and committed-ON-flipped-locally are all refused.
B4 --since-head proved inequality, not ancestry: an unrelated pre-existing commit passed as
   new work.
B5 empty ROOT commit exempted by parent-count, then reported PUSH CONFIRMED. Emptiness is now
   defined as 'tree identical to every parent' rather than exempted by category.
B6 (self-found by sweeping for the CONSTRUCT, not the report) same < <(git diff) in
   cmd_check_staged — fails closed but published a wrong diagnosis. Both callers now route
   through one staged_files_z().

41/41 needles, executed directly from a clean clone. Known gap, stated: B2's second instance is
fixed but UNNEEDLED — a corrupt index aborts earlier at 128, so no fault injection reaches it.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01YKj59Qadrb2WBLaePvkM7H
This commit is contained in:
installer-7
2026-07-30 20:38:06 -05:00
co-authored by Claude Opus 5
parent 62e3bf9e28
commit b0fb208b89
3 changed files with 377 additions and 13 deletions
+186 -10
View File
@@ -143,6 +143,49 @@ require_repo() {
# ---------------------------------------------------------------- check-staged
# staged_files_z <array-name> [pathspec...]
#
# THE FAIL-OPEN THIS FUNCTION EXISTS TO REMOVE:
# mapfile -d '' -t files < <(git diff ...)
# observes MAPFILE's exit status, never the producer's. When git diff dies -- a
# bad pathspec, a corrupt index -- mapfile cheerfully reads zero bytes and
# succeeds, and the caller then reports that silence as "nothing to check".
#
# `set -o pipefail` does NOT cover this. pipefail applies to PIPELINES; a process
# substitution is an asynchronous child whose status is never collected. That is
# the precise reason every `cmd | cmd` in this file is safe and both `< <(cmd)`
# constructs were not -- a distinction worth stating, because "we set pipefail"
# reads like whole-file coverage and is not.
#
# Both callers now route through here, so the category is gone rather than the
# two known instances being patched.
staged_files_z() {
local -n _out="$1"; shift
local tmp err status=0
tmp="$(mktemp)"; err="$(mktemp)"
if (( $# )); then
git diff --cached --name-only --diff-filter=ACM --no-renames -z \
-- "$@" >"$tmp" 2>"$err" || status=$?
else
git diff --cached --name-only --diff-filter=ACM --no-renames -z \
>"$tmp" 2>"$err" || status=$?
fi
if (( status != 0 )); then
local msg; msg="$(tr '\n' ' ' <"$err")"
rm -f "$tmp" "$err"
fail "$EX_CONFIG" \
"CANNOT ENUMERATE STAGED FILES — git diff exited $status" \
"git said: ${msg:-(no message)}" \
"" \
"Refusing to report a result. An enumeration that failed returns no" \
"files, which is indistinguishable from a clean tree — reporting that" \
"as OK would be a silent pass on an unexamined index."
fi
_out=()
mapfile -d '' -t _out <"$tmp"
rm -f "$tmp" "$err"
}
check_unmerged() {
local unmerged
unmerged="$(git ls-files --unmerged | awk '{print $4}' | sort -u)"
@@ -347,6 +390,86 @@ resolve_json_config() {
ALLOW) CFG_ALLOW+=("$val") ;;
esac
done <<<"$out"
# ------------------------------------------------------------------
# THE ASYMMETRY, ENFORCED RATHER THAN INTENDED.
# ON may come from anywhere: turning a check on needs no review.
# OFF must come from a COMMITTED object, because the entire argument for
# requiring a written reason was that a reason is reviewable in a diff
# while an absence is not. An opt-out honoured from an untracked
# working-tree file is not reviewable either -- it is an ad-hoc local
# bypass wearing a config file's clothes, and it defeats the design.
# So the OFF decision is re-read from HEAD and the working tree gets no
# vote in it. A committed ON flipped to OFF locally is likewise refused.
# ------------------------------------------------------------------
[[ "$CFG_MODE" == "none" ]] || return 0
local rel cmode cblob cout cstatus=0
rel="$(git ls-files --full-name --error-unmatch -- "$cfg" 2>/dev/null)" || rel=""
if [[ -z "$rel" ]]; then
fail "$EX_CONFIG" \
"OPT-OUT IS NOT REVIEWABLE — $CONFIG_FILE is not tracked in git" \
"recorded reason was: ${CFG_REASON:-(none)}" \
"" \
"The opt-out requires a written reason precisely so it shows up in a" \
"diff. An untracked file shows up in nobody's review, so honouring it" \
"would rebuild the unreviewable bypass this design exists to prevent." \
"" \
"Commit $CONFIG_FILE, then run again. Turning the check ON needs no" \
"commit; only turning it OFF does."
fi
# A COMMITTED SYMLINK (mode 120000) is a mutable target: the reviewed blob
# is a path, and what it points at can change without any diff at all.
cmode="$(git ls-tree HEAD -- "$rel" 2>/dev/null | awk '{print $1}')"
if [[ -z "$cmode" ]]; then
fail "$EX_CONFIG" \
"OPT-OUT IS NOT REVIEWABLE — $CONFIG_FILE is staged but not yet in HEAD" \
"recorded reason was: ${CFG_REASON:-(none)}" \
"" \
"A staged-but-uncommitted opt-out has not been through review either." \
"Commit it first."
fi
if [[ "$cmode" == "120000" ]]; then
fail "$EX_CONFIG" \
"OPT-OUT IS NOT REVIEWABLE — $CONFIG_FILE is a committed SYMLINK" \
"" \
"The reviewed object would be a path, not a decision: what it points" \
"at can be changed later without producing any diff. Commit the" \
"config as a regular file."
fi
cblob="$(git show "HEAD:$rel" 2>/dev/null)" || cblob=""
cout="$(printf '%s' "$cblob" | python3 -c "$CONFIG_PARSER" /dev/stdin 2>&1)" || cstatus=$?
if (( cstatus != 0 )); then
fail "$EX_CONFIG" \
"COMMITTED $CONFIG_FILE is INVALID — $cout" \
"" \
"The working tree opts out, but the committed version does not parse," \
"so there is no reviewable decision to honour."
fi
local cmode_val="" creason=""
while IFS=$'\t' read -r key val; do
case "$key" in
MODE) cmode_val="$val" ;;
REASON) creason="$val" ;;
esac
done <<<"$cout"
if [[ "$cmode_val" != "none" ]]; then
fail "$EX_CONFIG" \
"LOCAL-ONLY OPT-OUT — the working tree turns the JSON check OFF but" \
"HEAD does not (committed decision: $cmode_val)" \
"working-tree reason: ${CFG_REASON:-(none)}" \
"" \
"An uncommitted edit that disables a committed check is exactly the" \
"unreviewable bypass this guard refuses. Commit the change if it is" \
"real; revert it if it was a local convenience."
fi
# Print the COMMITTED reason, never the working tree's: the reason anyone
# can actually review is the one in the object.
CFG_REASON="$creason"
}
check_staged_json() {
@@ -417,10 +540,7 @@ check_staged_json() {
done
local -a files=()
mapfile -d '' -t files < <(
git diff --cached --name-only --diff-filter=ACM --no-renames -z \
-- "${pathspec[@]}"
)
staged_files_z files "${pathspec[@]}"
for f in ${files[@]+"${files[@]}"}; do
[[ "$f" == *.json ]] || continue
@@ -467,9 +587,14 @@ cmd_check_staged() {
# decomposes each rename back into D + A so the new path is always seen.
# (Adding R to the filter also works, but --name-only -z emits two paths for
# a rename and is ambiguous to parse — this removes the category instead.)
mapfile -d '' -t files < <(
git diff --cached --name-only --diff-filter=ACM --no-renames -z
)
#
# SECOND INSTANCE OF THE SAME DEFECT, found by sweeping for the construct
# rather than fixing only the one that was reported. This one failed CLOSED
# (zero files hit "NOTHING IS STAGED"), so it was never a security hole --
# but it reported a confident WRONG DIAGNOSIS, sending anyone debugging it
# at their index instead of at the failing git invocation.
local -a files=()
staged_files_z files
if (( ${#files[@]} == 0 )); then
fail "$EX_NULL" \
"NOTHING IS STAGED" \
@@ -517,6 +642,12 @@ cmd_push() {
# (1) Did a new commit actually get created?
if [[ -n "$since_head" ]]; then
git rev-parse --verify --quiet "${since_head}^{commit}" >/dev/null \
|| fail "$EX_USAGE" \
"--since-head is not a commit in this repository: $since_head" \
"" \
"Cannot verify advancement against a start point that does not exist."
if [[ "$head" == "$since_head" ]]; then
fail "$EX_NULL" \
"HEAD DID NOT ADVANCE — no commit was created" \
@@ -525,10 +656,32 @@ cmd_push() {
"Your commit step failed and execution continued on stale state." \
"Pushing now would publish something you did not just build."
fi
log " ok HEAD advanced ${since_head:0:9} -> ${head:0:9}"
# INEQUALITY IS NOT ADVANCEMENT. "different from where I started" is
# satisfied by checking out any unrelated commit -- including one that
# existed long before this session -- which would let pre-existing work
# be published as something just built. The claim being made is that new
# commits were created ON TOP OF the recorded start point, and only
# ancestry states that.
if ! git merge-base --is-ancestor "$since_head" "$head"; then
fail "$EX_NULL" \
"HEAD IS NOT A DESCENDANT of the recorded start point" \
"start: $since_head" \
"head : $head" \
"" \
"HEAD differs from the start point but does not build on it, so" \
"this is a checkout of other history rather than work you just" \
"created. Being different is not the same as having advanced."
fi
log " ok HEAD advanced ${since_head:0:9} -> ${head:0:9} (descendant)"
fi
# (2) Is that commit non-empty?
# EXEMPTING BY PARENT COUNT WAS A FAIL-OPEN. An empty root commit sailed
# through "emptiness check not applicable" and was then reported as a
# CONFIRMED PUSH -- directly contradicting the non-empty requirement this
# step exists to enforce. Root and merge commits do have well-defined
# emptiness; the original code declined to define it, which is not the same
# as it being undefined.
local parents
parents="$(git rev-list --parents -n 1 HEAD | wc -w)"
if (( parents == 2 )); then # sha + exactly one parent
@@ -542,9 +695,32 @@ cmd_push() {
fi
log " ok HEAD is a non-empty commit"
elif (( parents > 2 )); then
log " -- HEAD is a merge commit — emptiness check not applicable"
# A merge is empty when its tree matches EVERY parent: it then carries
# no content of its own and no integration either.
local p all_same=yes
for p in $(git rev-list --parents -n 1 HEAD | cut -d' ' -f2-); do
git diff --quiet "$p" HEAD || { all_same=no; break; }
done
if [[ "$all_same" == yes ]]; then
fail "$EX_NULL" \
"HEAD IS AN EMPTY MERGE — its tree is identical to every parent" \
"commit $head" \
"" \
"It integrates nothing and introduces nothing. Refusing to push it."
fi
log " ok HEAD is a non-empty merge commit"
else
log " -- HEAD is a root commit — emptiness check not applicable"
# A root commit has no parent, so emptiness is measured against the
# empty tree: it is empty when it adds no paths at all.
if [[ -z "$(git diff-tree --root -r --name-only --no-commit-id HEAD)" ]]; then
fail "$EX_NULL" \
"HEAD IS AN EMPTY ROOT COMMIT — it introduces no files" \
"commit $head" \
"" \
"A root commit is empty when it adds nothing against the empty" \
"tree. Refusing to push it."
fi
log " ok HEAD is a non-empty root commit"
fi
# (3) Capture the remote BEFORE. This is the step whose absence made the
+116 -3
View File
@@ -37,11 +37,25 @@ new_repo() {
}
# write_config <repo> <json-text>
# The guard reads .push-guard.json from the WORKING TREE at the repo root, so it
# does not need to be staged — which also keeps the "N staged file(s)" counts in
# the older controls unchanged.
#
# THIS HELPER USED TO WRITE THE CONFIG UNTRACKED, and the comment that lived here
# justified it ("does not need to be staged"). That made every opt-out control in
# this file assert the FORBIDDEN provenance and pass — a control that does not
# merely test nothing, but tests the OPPOSITE of the requirement and goes green.
# The whole design is: ON from anywhere, OFF only from a committed reviewable
# artifact. A harness that opts out from an untracked file was proving the bypass
# worked. It now COMMITS, so the opt-out controls exercise the supported path.
write_config() {
printf '%s\n' "$2" > "$1/.push-guard.json"
git -C "$1" add .push-guard.json
git -C "$1" commit -q -m "push-guard config"
}
# write_config_untracked <repo> <json-text>
# Deliberately NOT committed — used only where the untracked config is the thing
# under test and the expected outcome is a REFUSAL.
write_config_untracked() {
printf '%s\n' "$2" > "$1/.push-guard.json"
}
# expect <kind> <expected_exit> <description> [--out <substring>] -- <command...>
@@ -467,6 +481,105 @@ expect CONTROL 0 "--since-head passes when a real commit was created" \
--out "HEAD advanced" -- \
bash -c "cd '$R' && '$GUARD' push --remote origin --branch main --since-head '$HEAD_BEFORE'"
# ---------------------------------------------------------------------------
# Needles for the five blockers rev-974 found from a clean checkout. Every one
# of these was reproduced before it was fixed; none is a hypothesis.
# ---------------------------------------------------------------------------
echo
echo "-- blocker 2: the enumerating producer's exit status --"
# FAULT INJECTION. mapfile < <(git diff) reported MAPFILE's status, so a git diff
# that died returned zero files and the guard published that silence as clean.
R="$(new_repo e1)"
mkdir -p "$R/data"; printf '{ not json' > "$R/data/bad.json"
git -C "$R" add -f data/bad.json
expect NEEDLE 6 "a failed file enumeration REFUSES instead of reporting clean" \
--out "CANNOT ENUMERATE STAGED FILES" -- \
bash -c "cd '$R' && '$GUARD' check-staged --json-path ':(this-magic-does-not-exist)data/*.json'"
# CONTROL: the same malformed file with a WORKING pathspec must still be caught,
# so the needle above is not passing merely because everything now refuses.
expect CONTROL 3 "a working pathspec still catches the malformed JSON" \
--out "data/bad.json" -- \
bash -c "cd '$R' && '$GUARD' check-staged --json-path 'data/*.json'"
echo
echo "-- blocker 3: OFF must come from a committed, reviewable object --"
R="$(new_repo e2)"
write_config_untracked "$R" '{"json_check": "none", "reason": "local unreviewed bypass"}'
mkdir -p "$R/data"; printf '{ not json' > "$R/data/bad.json"
git -C "$R" add -f data/bad.json
expect NEEDLE 6 "an UNTRACKED opt-out is refused as unreviewable" \
--out "OPT-OUT IS NOT REVIEWABLE" -- \
bash -c "cd '$R' && '$GUARD' check-staged"
# A committed ON silently flipped OFF in the working tree is the same bypass.
R="$(new_repo e3)"
write_config "$R" '{"json_paths": ["data/**/*.json"]}'
printf '%s\n' '{"json_check": "none", "reason": "local convenience"}' > "$R/.push-guard.json"
mkdir -p "$R/data"; printf '{ not json' > "$R/data/bad.json"
git -C "$R" add -f data/bad.json
expect NEEDLE 6 "a committed ON flipped OFF in the working tree is refused" \
--out "LOCAL-ONLY OPT-OUT" -- \
bash -c "cd '$R' && '$GUARD' check-staged"
# A committed SYMLINK is a mutable target: the reviewed blob is a path, and what
# it points at can change with no diff at all.
R="$(new_repo e4)"
printf '%s\n' '{"json_check": "none", "reason": "via symlink"}' > "$R/real-config.json"
ln -s real-config.json "$R/.push-guard.json"
git -C "$R" add real-config.json .push-guard.json
git -C "$R" commit -q -m "symlinked config"
mkdir -p "$R/data"; printf '{ not json' > "$R/data/bad.json"
git -C "$R" add -f data/bad.json
expect NEEDLE 6 "a committed SYMLINK config is refused as a mutable target" \
--out "committed SYMLINK" -- \
bash -c "cd '$R' && '$GUARD' check-staged"
# CONTROL: the supported path still works. Without this the whole opt-out feature
# could be dead and every needle above would still pass — a gate that can never
# say yes is not a gate.
R="$(new_repo e5)"
write_config "$R" '{"json_check": "none", "reason": "committed and reviewable"}'
mkdir -p "$R/data"; printf '{ not json' > "$R/data/bad.json"
git -C "$R" add -f data/bad.json
expect CONTROL 0 "a COMMITTED opt-out is honoured and prints its committed reason" \
--out "recorded reason: committed and reviewable" -- \
bash -c "cd '$R' && '$GUARD' check-staged"
echo
echo "-- blocker 4: advancement is ancestry, not inequality --"
R="$(new_repo e6)"
git -C "$R" checkout -qb other
printf 'o\n' > "$R/o.txt"; git -C "$R" add o.txt; git -C "$R" commit -q -m o
OTHER="$(git -C "$R" rev-parse HEAD)"
git -C "$R" checkout -q main 2>/dev/null || git -C "$R" checkout -q master
printf 'm\n' > "$R/m.txt"; git -C "$R" add m.txt; git -C "$R" commit -q -m m
MAINH="$(git -C "$R" rev-parse HEAD)"
git -C "$R" checkout -q "$OTHER"
expect NEEDLE 5 "a checkout of pre-existing diverged history is not 'advancement'" \
--out "NOT A DESCENDANT" -- \
bash -c "cd '$R' && '$GUARD' push --remote origin --branch other --since-head '$MAINH'"
echo
echo "-- blocker 5: root and merge commits have defined emptiness --"
R="$(new_repo e7)"
git -C "$R" checkout -q --orphan fresh
git -C "$R" rm -q -rf . >/dev/null 2>&1 || true
git -C "$R" commit -q --allow-empty -m "empty root"
expect NEEDLE 5 "an EMPTY ROOT commit is refused, not exempted" \
--out "EMPTY ROOT COMMIT" -- \
bash -c "cd '$R' && '$GUARD' push --remote origin --branch fresh"
# CONTROL: a real root commit must still push, or the fix is just a new wall.
R="$(new_repo e8)"
git -C "$R" checkout -q --orphan fresh2
git -C "$R" rm -q -rf . >/dev/null 2>&1 || true
printf 'real\n' > "$R/real.txt"; git -C "$R" add real.txt
git -C "$R" commit -q -m "real root"
expect CONTROL 0 "a NON-empty root commit still pushes" \
--out "non-empty root commit" -- \
bash -c "cd '$R' && '$GUARD' push --remote origin --branch fresh2"
echo
printf 'push-guard needles: %d passed, %d failed\n' "$PASS" "$FAIL"
(( FAIL == 0 )) || exit 1
+75
View File
@@ -0,0 +1,75 @@
#!/usr/bin/env bash
# verify-clean-clone.sh -- run the suite from a FRESH CLONE, never the working copy.
#
# BLOCKER 1 EXISTED BECAUSE EVERY RUN HAPPENED WHERE THE EXEC BIT WAS ALREADY SET.
# Two people ran 32/32 on files whose local mode had been set by hand at creation
# time; the COMMITTED mode was 100644, so the delivered artifact returned 126
# Permission denied and aborted before printing a summary. The artifact had never
# run for anyone.
#
# "Run it somewhere real" was not enough. The missing word is CLEAN: run it from a
# checkout that carries only what the repository actually stores. Local state you
# did not commit is invisible to you precisely because it is yours.
#
# This script commits the artifacts into a scratch repo, clones that repo, and
# runs the suite from the clone -- so the only modes in play are the ones git
# recorded. It also asserts the mode positively, so a future commit that drops the
# bit fails here instead of at the next reviewer.
set -euo pipefail
HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
WORK="$(mktemp -d)"
trap 'rm -rf "$WORK"' EXIT
ARTIFACTS=(push-guard.sh test-push-guard.sh)
printf '=== packaging from %s ===\n' "$HERE"
mkdir -p "$WORK/src"
for f in "${ARTIFACTS[@]}"; do
cp "$HERE/$f" "$WORK/src/$f"
done
[[ -f "$HERE/README.md" ]] && cp "$HERE/README.md" "$WORK/src/README.md"
cd "$WORK/src"
git init -q .
git config user.email "[email protected]"
git config user.name "Clean Clone"
git add -A
# THE ASSERTION THAT WOULD HAVE CAUGHT BLOCKER 1.
# Check the mode in the INDEX -- what git will actually store -- not the mode on
# disk, which is what misled us. These differ exactly when it matters.
rc=0
for f in "${ARTIFACTS[@]}"; do
mode="$(git ls-files -s -- "$f" | awk '{print $1}')"
if [[ "$mode" != "100755" ]]; then
printf ' FAIL %s is mode %s in the index (needs 100755)\n' "$f" "$mode"
printf ' fix with: git update-index --chmod=+x %s\n' "$f"
rc=1
else
printf ' ok %s is mode %s in the index\n' "$f" "$mode"
fi
done
(( rc == 0 )) || { printf '\nREFUSING TO CONTINUE: the committed modes are wrong.\n'; exit 1; }
git commit -q -m "artifacts under test"
printf '\n=== cloning (no local mode bits survive this) ===\n'
cd "$WORK"
git clone -q src clone
cd clone
for f in "${ARTIFACTS[@]}"; do
printf ' clone mode: %s %s\n' "$(stat -c '%a' "$f")" "$f"
done
printf '\n=== executing DIRECTLY (./script), not via `bash script` ===\n'
# Running `bash script` masks a missing exec bit completely -- it is how the
# original 32/32 was obtained. Direct execution is the thing under test.
if ! ./test-push-guard.sh; then
printf '\nCLEAN-CLONE RUN FAILED.\n'
exit 1
fi
printf '\n=== CLEAN CLONE: suite ran and passed from a fresh checkout ===\n'