feat(scripts): SHA-pin tooling Suggested-tier follow-up (#176 review)

.claude/CLAUDE.md

@@ -12,8 +12,9 @@ for ByronWilliamsCPA. It contains:
 - Org-level profile (`profile/README.md`)
 
 There is no Python package and no build system in this repo. The Bats test suite
-under `tests/` covers `scripts/update-pinned-actions.sh` (run via `.github/workflows/shell-tests.yml`);
-the other scripts in `scripts/` do not yet have automated tests.
+under `tests/` covers both `scripts/update-pinned-actions.sh` and
+`scripts/fleet-audit-sha-pins.sh` (all `tests/*.bats` files run via
+`.github/workflows/shell-tests.yml`).
 
 ## Model Selection
 

.github/workflows/shell-tests.yml

@@ -9,7 +9,7 @@
 #
 # Run locally:
 #   git submodule update --init --recursive
-#   ./tests/libs/bats-core/bin/bats tests/update-pinned-actions.bats
+#   ./tests/libs/bats-core/bin/bats tests/*.bats
 # ============================================================================
 
 name: Shell Tests
@@ -52,4 +52,12 @@ jobs:
           submodules: recursive
 
       - name: Run bats tests
-        run: ./tests/libs/bats-core/bin/bats tests/update-pinned-actions.bats
+        # Discover all .bats files under tests/ rather than naming a single
+        # file. Naming tests/update-pinned-actions.bats hid a class of bugs:
+        # tests added under tests/fleet-audit-sha-pins.bats (and any future
+        # *.bats sibling) were never executed by CI, so STRICT_AUDIT and
+        # REPO_LIMIT regressions could ship green. find + -print0/-exec keeps
+        # this glob-injection-safe and POSIX-clean.
+        run: |
+          find tests -maxdepth 1 -name '*.bats' -print0 \
+            | xargs -0 ./tests/libs/bats-core/bin/bats

CHANGELOG.md

@@ -32,6 +32,20 @@ are no numbered releases.
   2026) makes the SBOM scanner itself a supply-chain risk; Grype (Anchore) is
   unaffected.
 
+- `scripts/fleet-audit-sha-pins.sh`: new `STRICT_AUDIT=1` environment toggle.
+  When set, the script exits with status 2 if any repo emitted a `repo,error`
+  row or if any org's repo count saturated `REPO_LIMIT`. CI gates that need
+  fail-closed semantics opt in via `STRICT_AUDIT=1`; the default flow remains
+  report-only (exit 0) so existing consumers are unaffected. Companion
+  change: `REPO_LIMIT` now reads from environment with a default of 1000
+  (`REPO_LIMIT="${REPO_LIMIT:-1000}"`), letting tests exercise the
+  saturation WARN with a small stub-friendly value and giving operators an
+  escape hatch for fleets that grow beyond the default.
+- `scripts/fleet-audit-sha-pins.sh`: `STRICT_AUDIT` now accepts the
+  case-insensitive truthy spellings `1`, `true`, and `yes`; previously only
+  the literal `"1"` was honoured and `STRICT_AUDIT=true` silently degraded
+  to report-only.
+
 ### Breaking Changes
 
 - `python-security-analysis.yml`: the `safety` SCA scanner has been
@@ -178,6 +192,14 @@ are no numbered releases.
 - `scripts/fleet-audit-sha-pins.sh`: distinguish legitimate HTTP 404 (no workflows directory) from real API failures (rate limit, 5xx, auth loss); real failures now emit a `repo,error` sentinel row and a stderr WARN line instead of silently appearing as `repo,0`. The 404 discriminator now requires the exact `(HTTP 404)` parenthesized form so an upstream 5xx response containing the substring "Not Found" cannot be misclassified as a missing resource. Replaces the regex-based `SKIP_OWNERS` check with a literal `case` match so operator-supplied values containing regex metacharacters (e.g., `.*`) cannot silently classify every owner as skipped. Adds a stderr WARN when a per-file 404 occurs mid-iteration (the directory listing succeeded but the specific file fetch returned 404, indicating a race or scope mismatch). Replaces the silent `|| true` masking on the base64 decode with explicit rc handling that propagates decode failures to the `repo,error` sentinel. Addresses the silent-failure and regex-injection findings from the PR #175 follow-up review.
 - `scripts/update-pinned-actions.sh`: `extract_tag_pins` and `extract_branch_pins` now accept refs that carry an inline `# comment` (e.g., `actions/checkout@v4  # stable`); the converter previously required the ref to be the last non-whitespace content on the line, leaving inline-annotated refs as lingering audit violations. The sed substitution in `--pin-tags --apply` now spans to end-of-line so any pre-existing inline comment is replaced cleanly by the new `# <tag>` comment. Escapes sed-replacement metacharacters (`\`, `&`, `|`) in the resolved tag name AND sed-pattern metacharacters in `current_tag` and `full_action` before splicing into the substitution; a semver `.` in `current_tag` no longer matches arbitrary characters and tags containing the sed delimiter `|` no longer terminate the pattern field. Replaces the regex-based `--owner-allowlist` filter in both extractors with an awk literal-string lookup, eliminating the regex-injection vector where `--owner-allowlist '.*'` would silently classify every action as first-party. Replaces the `{ pipeline; } || true` wrappers in both extractors with explicit grep rc handling so a real failure (rc>=2: unreadable directory, permission denied) surfaces as a stderr WARN and a non-zero return instead of a silent empty result. Replaces the two-call `resolve_tag_sha` pattern with a single composite-jq fetch, removing the race window where a force-push or rate-limit hit between two fetches of the same ref could return disagreeing values. Makes the EXIT trap BSD-compatible by using `${VAR:+"$VAR"}` so `rm -f` never receives an empty-string argument on macOS. Hoists the `CHANGE_LOG` temp file and its cleanup trap to script scope so `pin_tags_main` no longer relies on a function-local `RETURN` trap; both extractor pipelines tolerate zero matches under `set -euo pipefail` via the new rc-aware path.
 - `tests/update-pinned-actions.bats`, `tests/fleet-audit-sha-pins.bats`: add eight new test cases covering the hardening above. The five from the original PR #175 follow-up commit (inline-comment conversion, custom `--owner-allowlist`, two-step annotated-tag dereferencing, audit emits `repo,error` on non-404 failure, audit treats 404 on the workflows directory as a legitimate zero) are joined by three regression-vector tests: sed-escape of `&` and `|` characters in tag-name comments, `--skip-owners` with regex metacharacters still counts violations, and mixed-success inner-loop (one workflow file fetches OK, a second fails, repo row flips to `error` not a partial numeric count).
+- `scripts/update-pinned-actions.sh`: `resolve_tag_sha` now validates that the `gh api` composite jq response contains non-empty, non-`"null"` values for `.object.type` and `.object.sha`. The previous code accepted the literal string `"null"` (emitted by `jq -r` for missing fields) as a successful resolution, which produced patterns like `actions/checkout@null` that the caller wrote into the workflow file. The same guard now also fires after the second annotated-tag dereferencing call. Action refs that fail validation are marked SKIPPED rather than written with a malformed SHA.
+- `tests/fleet-audit-sha-pins.bats`, `tests/update-pinned-actions.bats`: add nine more regression-vector tests guarding the Suggested-tier fixes. fleet-audit gains: REPO_LIMIT saturation WARN with env-overridable REPO_LIMIT, STRICT_AUDIT=1 fail-closed on saturation, STRICT_AUDIT=1 fail-closed on per-repo error, STRICT_AUDIT=1 clean-run regression guard, SKIP_OWNERS edge cases (single value, empty, double internal commas). update-pinned-actions gains: extract_branch_pins reports comment-annotated branch refs, --pin-tags --apply succeeds with only-first-party fixtures, --pin-tags --apply skips when annotated-tag dereferencing returns missing `.object.sha`.
+- `scripts/fleet-audit-sha-pins.sh`: workflows-listing failure path now sets `audit_incomplete=true` before continuing, matching the per-file-fetch error path. The previous code emitted a `repo,error` sentinel but left the flag unset, so `STRICT_AUDIT=1` could exit 0 even when error rows were present. Verified by the new `STRICT_AUDIT=1 exits non-zero on workflows-listing failure` regression test. (Critical from the PR #177 self-review.)
+- `.github/workflows/shell-tests.yml`: bats runner now discovers all `tests/*.bats` files via `find ... -print0 | xargs -0`. Previously it named only `tests/update-pinned-actions.bats`, so the 13 cases in `tests/fleet-audit-sha-pins.bats` (including the STRICT_AUDIT and REPO_LIMIT regression cases added in PR #176 and PR #177) never ran in CI. (Critical from the PR #177 self-review.)
+- `scripts/fleet-audit-sha-pins.sh`: `gh repo list` invocation now captures stdout to a temp file rather than reading through process substitution, so the command's exit code surfaces. An auth-loss or rate-limit hit during enumeration now flags the audit as incomplete and emits a stderr WARN; previously the failure was discarded and the org silently contributed zero rows.
+- `scripts/fleet-audit-sha-pins.sh`: validate `REPO_LIMIT` as a positive integer up front and exit 1 with a clear error message when it is empty, zero, negative, or non-numeric. Previously a bad value would crash mid-loop inside `[[ ... -eq $REPO_LIMIT ]]` under `set -euo pipefail`.
+- `scripts/update-pinned-actions.sh`: `resolve_tag_sha` null guards now emit a stderr WARN before returning empty, so operators can distinguish a malformed GitHub API response from a missing tag. The action is still marked SKIPPED; only the diagnostic surface changes.
+- `scripts/fleet-audit-sha-pins.sh`: strip trailing CR from each line before regex matching in the violation counter, so workflow files checked out on Windows or proxied through CRLF-normalizing intermediaries no longer leak `\r` into `BASH_REMATCH[4]` and inflate violation counts.
 
 ### Security
 

scripts/fleet-audit-sha-pins.sh

@@ -13,10 +13,29 @@
 #                    message written to stderr
 # Exit code: 0 even when violations or errors are present (this is a report,
 # not a gate). Errors are surfaced via the sentinel row plus stderr.
+# Exception: STRICT_AUDIT (below) overrides this and exits 2 on any incomplete
+# audit. A bad REPO_LIMIT value exits 1 immediately before any audit work.
+#
+# Environment overrides:
+#   REPO_LIMIT     Max repos fetched per org via `gh repo list` (default 1000).
+#                  Lower it for testing; raise it if your fleet exceeds 1000
+#                  repos. Must be a positive integer; the script exits 1 with
+#                  an error if not. The script emits a stderr WARN when the
+#                  result count equals REPO_LIMIT (saturation, audit may be
+#                  truncated).
+#   STRICT_AUDIT   When truthy (1, true, or yes; case-insensitive), exit 2 at
+#                  the end of the run if any API failure produced a
+#                  `repo,error` row, any org's `gh repo list` failed, or any
+#                  org's repo count saturated REPO_LIMIT. Use this in CI
+#                  gates that should fail closed when the audit is
+#                  incomplete. The default (unset / "0" / "false" / "no")
+#                  preserves report-only semantics.
 #
 # Usage:
 #   ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA [--org williaby] \
 #     [--skip-owners ByronWilliamsCPA,williaby] [--output audit.csv]
+#   STRICT_AUDIT=1 ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA
+#   REPO_LIMIT=3 ./scripts/fleet-audit-sha-pins.sh --org ByronWilliamsCPA
 # =============================================================================
 
 set -euo pipefail
@@ -96,12 +115,39 @@ is_skipped_owner() {
     esac
 }
 
-REPO_LIMIT=1000
+# REPO_LIMIT defaults to 1000 but may be overridden via environment for tests
+# and for fleets that grow beyond the default. Validate as a positive integer
+# up front so the loop below does not blow up inside `[[ ... -eq $REPO_LIMIT ]]`
+# under `set -euo pipefail` when the operator passes a typo (`REPO_LIMIT=abc`).
+REPO_LIMIT="${REPO_LIMIT:-1000}"
+if [[ ! "$REPO_LIMIT" =~ ^[1-9][0-9]*$ ]]; then
+    echo "ERROR: REPO_LIMIT must be a positive integer (got: '$REPO_LIMIT')" >&2
+    exit 1
+fi
+
+# Tracks whether the audit was incomplete due to API errors or saturation;
+# STRICT_AUDIT consults this at exit to decide whether to fail closed.
+audit_incomplete=false
 
 for org in "${ORGS[@]}"; do
-    mapfile -t repos < <(gh repo list "$org" --limit "$REPO_LIMIT" --json name --jq '.[].name')
+    # Capture `gh repo list` rc via a tmpfile rather than process substitution.
+    # `mapfile -t repos < <(gh repo list ...)` would discard a non-zero rc
+    # (auth loss, rate-limit mid-run, transient API hiccup), leaving `repos`
+    # empty and the org silently contributing zero rows without flagging
+    # the audit as incomplete. That defeats STRICT_AUDIT.
+    repo_list_tmp="$(mktemp)"
+    if ! gh repo list "$org" --limit "$REPO_LIMIT" --json name --jq '.[].name' \
+            > "$repo_list_tmp" 2>/dev/null; then
+        echo "WARN: $org: gh repo list failed; org skipped, audit incomplete" >&2
+        audit_incomplete=true
+        rm -f "$repo_list_tmp"
+        continue
+    fi
+    mapfile -t repos < "$repo_list_tmp"
+    rm -f "$repo_list_tmp"
     if [[ ${#repos[@]} -eq $REPO_LIMIT ]]; then
         echo "WARN: $org returned exactly $REPO_LIMIT repos; audit may be truncated. Raise REPO_LIMIT or page through results." >&2
+        audit_incomplete=true
     fi
     for repo in "${repos[@]}"; do
         full="$org/$repo"
@@ -113,6 +159,11 @@ for org in "${ORGS[@]}"; do
         if ! gh_api_safe "repos/$full/contents/.github/workflows" '.[].path'; then
             echo "WARN: $full: workflows listing failed: $GH_API_BODY" >&2
             emit "$full,error"
+            # STRICT_AUDIT contract: every "$full,error" sentinel emitted must
+            # flag the audit as incomplete. The per-file-fetch error path
+            # below sets this flag too; both branches must stay in sync or
+            # STRICT_AUDIT can exit 0 while error rows are present.
+            audit_incomplete=true
             continue
         fi
         if [[ "$GH_API_STATUS" == "missing" ]] || [[ -z "$GH_API_BODY" ]]; then
@@ -147,6 +198,12 @@ for org in "${ORGS[@]}"; do
             fi
             [[ -z "$content" ]] && continue
             while IFS= read -r line; do
+                # Strip a trailing CR before regex matching. Workflow files
+                # checked out on Windows or fetched through proxies can
+                # carry CRLF endings; without this, BASH_REMATCH[4] would
+                # capture a literal \r at the end of refs like "v4$'\r'",
+                # failing the 40-hex SHA test and inflating violations.
+                line="${line%$'\r'}"
                 # Skip comment lines
                 [[ "$line" =~ ^[[:space:]]*# ]] && continue
                 if [[ "$line" =~ $TAG_OR_BRANCH_RE ]]; then
@@ -167,8 +224,21 @@ for org in "${ORGS[@]}"; do
 
         if [[ "$api_error" == true ]]; then
             emit "$full,error"
+            audit_incomplete=true
         else
             emit "$full,$violations"
         fi
     done
 done
+
+# Fail closed when STRICT_AUDIT is enabled. The default flow remains
+# report-only (exit 0) so existing CI consumers do not break. Accept the
+# common truthy spellings (1, true, yes) case-insensitively rather than the
+# literal "1" alone; "STRICT_AUDIT=true" silently degrading to report-only
+# was a documented portability footgun.
+strict_audit_lower="${STRICT_AUDIT:-0}"
+strict_audit_lower="${strict_audit_lower,,}"
+if [[ "$strict_audit_lower" =~ ^(1|true|yes)$ && "$audit_incomplete" == true ]]; then
+    echo "ERROR: STRICT_AUDIT=1 and the audit was incomplete (API errors and/or REPO_LIMIT saturation). Exiting non-zero." >&2
+    exit 2
+fi

scripts/update-pinned-actions.sh

@@ -131,9 +131,25 @@ resolve_tag_sha() {
     obj_type="${pair%%|*}"
     obj_sha="${pair#*|}"
 
+    # jq emits the literal string "null" under -r when a referenced field
+    # is missing or null. Treat that as a resolution failure so the caller
+    # marks the action SKIPPED rather than writing "actions/checkout@null"
+    # into the workflow file. Emit a stderr WARN so operators can
+    # distinguish "tag doesn't exist" from "malformed GitHub API response."
+    if [[ -z "$obj_type" || -z "$obj_sha" || "$obj_type" == "null" || "$obj_sha" == "null" ]]; then
+        echo "WARN: $repo: $tag: null/empty .object.type or .object.sha; marking SKIPPED" >&2
+        echo ""
+        return
+    fi
+
     if [[ "$obj_type" == "tag" ]]; then
         # Annotated tag: resolve tag object to its target commit SHA
         obj_sha=$(gh api "repos/$repo/git/tags/$obj_sha" --jq '.object.sha' 2>/dev/null) || { echo ""; return; }
+        if [[ -z "$obj_sha" || "$obj_sha" == "null" ]]; then
+            echo "WARN: $repo: $tag: annotated tag dereferenced to null .object.sha; marking SKIPPED" >&2
+            echo ""
+            return
+        fi
     fi
 
     echo "$obj_sha"
@@ -283,7 +299,16 @@ pin_tags_main() {
     while IFS='|' read -r full_action current_tag new_sha latest_tag; do
         local _pat _rep safe_tag safe_action safe_current_tag
         # Escape sed-replacement metacharacters in latest_tag (the replacement
-        # side). Order matters: backslash first.
+        # side, not the pattern). Replacement strings have three specials:
+        #   \   literal backslash (must escape first to avoid double-escape)
+        #   &   the full matched pattern (would splice the ref back in)
+        #   |   only special because the script uses `s|...|...|g`; if the
+        #       delimiter changes to `/` (or any other char), this set must
+        #       be updated to match. The matching pattern-side escape lives
+        #       in escape_sed_pat() below.
+        # Maintenance note: if the sed delimiter ever changes from `|`, both
+        # the replacement-side escape set here AND the pattern-side
+        # escape_sed_pat() function below must be updated together.
         safe_tag="${latest_tag//\\/\\\\}"
         safe_tag="${safe_tag//&/\\&}"
         safe_tag="${safe_tag//|/\\|}"