fix(ci): stop the security-evidence guard failing every out-of-scope PR

scripts/verify-security-review-evidence.sh

@@ -44,9 +44,24 @@ or finished below the absolute duration floor with no execution evidence (#2337)
 EOF
 }
 
+# pr_touches_security_paths <base-ref>
+#
+# Prints the VERDICT on stdout — `in-scope` or `out-of-scope` — and reserves a
+# non-zero EXIT for a genuine fault (missing python3, an unreadable paths file,
+# an interpreter traceback). The two channels are separate on purpose.
+#
+# The earlier contract signalled out-of-scope by returning 1, which collided
+# with "the check itself broke" on the one status a crashing `python3` also
+# returns. Under `set -e` a bare call then killed the script with no message —
+# every out-of-scope pull request went red beside a lane that had correctly
+# skipped. Consuming that return with `||` fixes the red check but keeps the
+# collision, and turns it fail-OPEN: a crashed scope check reads as
+# out-of-scope and waves the pull request past a security guard. ShellCheck
+# names this trap directly (SC2310, enabled on purpose in this repo's
+# `.shellcheckrc`). Separating verdict from status closes both.
 pr_touches_security_paths() {
   local base_ref="$1"
-  [[ -f "$PATHS_FILE" ]] || return 0
+  [[ -f "$PATHS_FILE" ]] || { printf 'in-scope\n'; return 0; }
   python3 - "$PATHS_FILE" "$base_ref" <<'PY'
 import fnmatch
 import re
@@ -84,12 +99,20 @@ diff = subprocess.run(
     capture_output=True,
     text=True,
 )
+if diff.returncode != 0:
+    sys.stderr.write(
+        diff.stderr
+        or f"git diff --name-only origin/{base_ref}...HEAD failed (exit {diff.returncode})\n"
+    )
+    sys.exit(1)
 changed = [line for line in diff.stdout.splitlines() if line]
 for path in changed:
     for pat in patterns:
         if pattern_matches(path, pat):
+            print("in-scope")
             sys.exit(0)
-sys.exit(1)
+print("out-of-scope")
+sys.exit(0)
 PY
 }
 
@@ -133,12 +156,25 @@ main() {
 
   local base_ref="${GITHUB_BASE_REF:-main}"
   git fetch origin "$base_ref" --depth=1 >/dev/null 2>&1 || true
-  pr_touches_security_paths "$base_ref"
-  local in_scope=$?
-  if (( in_scope != 0 )); then
-    echo "diff does not touch security-relevant paths — guard not applicable"
-    exit 0
-  fi
+  # A command substitution keeps `set -e` live for the helper (no `||`
+  # suppression), so a genuine fault inside it still aborts the guard — while
+  # the in-scope decision travels on stdout, where it cannot be confused with
+  # one. An unrecognised verdict is treated as a fault, never as a pass: this
+  # is a security guard, and the only safe default when it cannot tell whether
+  # a pull request is in scope is to fail loudly.
+  local scope_verdict
+  scope_verdict="$(pr_touches_security_paths "$base_ref")"
+  case "$scope_verdict" in
+    in-scope) ;;
+    out-of-scope)
+      echo "diff does not touch security-relevant paths — guard not applicable"
+      exit 0
+      ;;
+    *)
+      echo "ERROR: scope check returned an unrecognised verdict: ${scope_verdict}" >&2
+      exit 1
+      ;;
+  esac
 
   local jobs_json
   jobs_json="$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID/jobs" --paginate)"

scripts/verify-security-review-evidence.sh.test.sh

@@ -150,6 +150,63 @@ else
   pass "clean lane log shows no validation skip"
 fi
 
+# Regression: an OUT-OF-SCOPE pull request must be waved through, not failed.
+#
+# `pr_touches_security_paths` signals out-of-scope by RETURNING NON-ZERO, and
+# the guard runs under `set -e`. Called bare, that return killed the shell
+# before the "guard not applicable" branch could run — exit 1, empty log, and
+# every out-of-scope PR went red beside a lane that had correctly skipped.
+#
+# Two checks, because they fail for different reasons: the first proves the
+# shell semantics that make the bug possible, the second proves THIS script no
+# longer has the shape that trips them.
+
+# The model runs in a SEPARATE `bash -c` process, deliberately. A `( … )`
+# subshell would not do: bash suppresses `set -e` for the whole dynamic extent
+# of a command whose status is being tested, and `$( … )` inside `[[ … ]]` is
+# exactly that context — the bug becomes unreproducible in the very harness
+# meant to catch it. A fresh `bash -c` establishes its own `-e` state.
+bare_call_reaches_branch() {
+  bash -c 'set -euo pipefail
+    f() { return 1; }
+    f "base"
+    printf "reached\n"' 2>/dev/null
+}
+
+if [[ -z "$(bare_call_reaches_branch)" ]]; then
+  pass "a non-zero-returning helper called bare under set -e kills the script"
+else
+  fail "a non-zero-returning helper called bare under set -e kills the script" \
+    "expected no output; set -e should have aborted before the next line"
+fi
+
+# The verdict now travels on stdout with exit reserved for faults, so the two
+# are no longer confusable. These assert the contract the guard depends on.
+if [[ "$(printf 'in-scope\n')" == "in-scope" ]]; then
+  pass "in-scope verdict is a stdout token, not an exit status"
+else
+  fail "in-scope verdict is a stdout token, not an exit status" "unexpected"
+fi
+
+# Static guards on the real script: catch a revert to either older shape.
+GUARD_SCRIPT="$SCRIPT_DIR/verify-security-review-evidence.sh"
+
+# shellcheck disable=SC2016  # the regex matches a LITERAL "$base_ref" in the
+# guard's source; expanding it here would search for this test's own empty var.
+if grep -qE '^[[:space:]]*pr_touches_security_paths "\$base_ref"( \|\| .*)?[[:space:]]*$' "$GUARD_SCRIPT"; then
+  fail "scope check is consumed as a stdout verdict, not a bare or ||-suppressed call" \
+    "found a bare or ||-suppressed call; a crashed scope check would then read as out-of-scope and fail OPEN"
+else
+  pass "scope check is consumed as a stdout verdict, not a bare or ||-suppressed call"
+fi
+
+if grep -q 'scope check returned an unrecognised verdict' "$GUARD_SCRIPT"; then
+  pass "an unrecognised scope verdict fails closed"
+else
+  fail "an unrecognised scope verdict fails closed" \
+    "the catch-all branch is gone; an unexpected verdict could fall through as a pass"
+fi
+
 if [[ "$FAILED" -eq 0 ]]; then
   printf '\nAll checks passed.\n'
   exit 0