From b0fb208b89f6f5dbc74fd895f4e288d33ca03209 Mon Sep 17 00:00:00 2001 From: installer-7 Date: Thu, 30 Jul 2026 20:38:06 -0500 Subject: [PATCH] =?UTF-8?q?fix(git):=20push-guard=20=E2=80=94=20resolve=20?= =?UTF-8?q?5=20review=20blockers=20+=20a=206th=20self-found=20instance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01YKj59Qadrb2WBLaePvkM7H --- .../mosaic/framework/tools/git/push-guard.sh | 196 +++++++++++++++++- .../framework/tools/git/test-push-guard.sh | 119 ++++++++++- .../framework/tools/git/verify-clean-clone.sh | 75 +++++++ 3 files changed, 377 insertions(+), 13 deletions(-) mode change 100644 => 100755 packages/mosaic/framework/tools/git/push-guard.sh mode change 100644 => 100755 packages/mosaic/framework/tools/git/test-push-guard.sh create mode 100755 packages/mosaic/framework/tools/git/verify-clean-clone.sh diff --git a/packages/mosaic/framework/tools/git/push-guard.sh b/packages/mosaic/framework/tools/git/push-guard.sh old mode 100644 new mode 100755 index adf3ed4d..17ebe760 --- a/packages/mosaic/framework/tools/git/push-guard.sh +++ b/packages/mosaic/framework/tools/git/push-guard.sh @@ -143,6 +143,49 @@ require_repo() { # ---------------------------------------------------------------- check-staged +# staged_files_z [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 diff --git a/packages/mosaic/framework/tools/git/test-push-guard.sh b/packages/mosaic/framework/tools/git/test-push-guard.sh old mode 100644 new mode 100755 index 7212abe9..cbe57e52 --- a/packages/mosaic/framework/tools/git/test-push-guard.sh +++ b/packages/mosaic/framework/tools/git/test-push-guard.sh @@ -37,11 +37,25 @@ new_repo() { } # write_config -# 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 +# 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 [--out ] -- @@ -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 diff --git a/packages/mosaic/framework/tools/git/verify-clean-clone.sh b/packages/mosaic/framework/tools/git/verify-clean-clone.sh new file mode 100755 index 00000000..3a806985 --- /dev/null +++ b/packages/mosaic/framework/tools/git/verify-clean-clone.sh @@ -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 "clean@example.invalid" +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'