review_checks: fail closed on scanner output, guard dash filenames, batch semgrep

.claude/skills/deep-code-review/SKILL.md

@@ -21,6 +21,6 @@ You are a senior reviewer hunting defects that a change introduces. For a small
 4. One finding per distinct mechanism, most severe first, at most 8.
 5. Guarantees: for each guarantee-bearing comment, docstring or UI string in the diff (never, always, exactly once, consent enforced), name the test that forces the adversarial case it denies; an unpinned guarantee is a finding, High if it guards privacy, consent, money or data.
 
-Whole-app security audit or review (not a diff): read `references/security-review-mode.md` first.
+Whole-app security audit (not a diff): read only `references/security-review-mode.md`; it routes the rest.
 
 OUTPUT. Final message: the severity-ranked findings, each as `file:line`, severity, mechanism and failing input in one or two sentences. Report template: `references/full-map-report.md`.

.claude/skills/deep-code-review/scripts/review_checks.sh

@@ -28,8 +28,8 @@ case "$tmo" in ''|*[!0-9]*) echo "review_checks: --timeout must be an integer" >
 git rev-parse --verify -q "$base^{commit}" >/dev/null || { echo "review_checks: unresolvable base '$base'" >&2; exit 2; }
 cd "$(git rev-parse --show-toplevel)"
 
-raw=$(mktemp "${TMPDIR:-/tmp}/review-raw.XXXXXX") out=$(mktemp "${TMPDIR:-/tmp}/review-out.XXXXXX")
-trap 'rm -f "$raw" "$out"' EXIT
+raw=$(mktemp "${TMPDIR:-/tmp}/review-raw.XXXXXX") out=$(mktemp "${TMPDIR:-/tmp}/review-out.XXXXXX") tsv=$(mktemp "${TMPDIR:-/tmp}/review-tsv.XXXXXX")
+trap 'rm -f "$raw" "$out" "$tsv"' EXIT
 files=$(git diff --name-only --diff-filter=ACMR "$base"...HEAD)
 rc=0
 
@@ -53,18 +53,21 @@ run_one() {
   case "$rc" in 124|142) skip "$tool" "timeout ${tmo}s"; return 1 ;; esac
   return 0
 }
+# scan_ok TOOL [OKRC...]: false (with a not-run line) when the last run_one exit code is not 0 or an OKRC (findings), i.e. a crash.
+scan_ok() { local t=$1 c; shift; for c in 0 "$@"; do [ "$rc" = "$c" ] && return 0; done; skip "$t" "exit $rc"; return 1; }
 # Findings parsed from $out by a sed script (TSV columns) land via `add`.
 sh_files=$(changed '\.(sh|bash)$')
 if [ -n "$sh_files" ]; then
   if have shellcheck; then
     while IFS= read -r f; do
-      run_one shellcheck shellcheck -f gcc "$f" || continue
+      run_one shellcheck shellcheck -f gcc "./$f" || continue
+      scan_ok shellcheck 1 || continue
       sed -nE 's/^.*:([0-9]+):[0-9]+: [a-z]+: (.*) \[(SC[0-9]+)\]$/\1\t\3\t\2/p' "$out" |
         while IFS=$'\t' read -r l r m; do add shellcheck "$f" "$l" "$r" "$m"; done
     done <<<"$sh_files"
   fi
   while IFS= read -r f; do
-    run_one bash-n bash -n "$f" || continue
+    run_one bash-n bash -n "./$f" || continue
     [ "$rc" = 0 ] || add bash-n "$f" "$(sed -nE 's/.*line ([0-9]+):.*/\1/p' "$out" | head -n1)" syntax "$(head -n1 "$out")"
   done <<<"$sh_files"
 fi
@@ -73,7 +76,7 @@ py_files=$(changed '\.py$')
 if [ -n "$py_files" ]; then
   if have python3; then
     while IFS= read -r f; do
-      run_one py_compile python3 -c 'import sys;compile(open(sys.argv[1],"rb").read(),sys.argv[1],"exec")' "$f" || continue
+      run_one py_compile python3 -c 'import sys;compile(open(sys.argv[1],"rb").read(),sys.argv[1],"exec")' "./$f" || continue
       [ "$rc" = 0 ] || add py_compile "$f" "$(sed -nE 's/.*[(,] ?line ([0-9]+).*/\1/p' "$out" | head -n1)" SyntaxError "$(tail -n1 "$out")"
     done <<<"$py_files"
   fi
@@ -84,7 +87,8 @@ if [ -n "$py_files" ]; then
   else skip "ruff/pyflakes" "not installed"; ln=; fi
   if [ -n "$ln" ]; then
     while IFS= read -r f; do
-      run_one "$ln" "${lint[@]}" "$f" || continue
+      run_one "$ln" "${lint[@]}" "./$f" || continue
+      scan_ok "$ln" 1 || continue
       sed -nE 's/^.*:([0-9]+):[0-9]+:? ([A-Z]+[0-9]+ )?(.*)$/\1\t\2\t\3/p' "$out" |
         while IFS=$'\t' read -r l r m; do add "$ln" "$f" "$l" "${r% }" "$m"; done
     done <<<"$py_files"
@@ -94,20 +98,20 @@ fi
 js_files=$(changed '\.(js|mjs|cjs)$')
 if [ -n "$js_files" ] && have node; then
   while IFS= read -r f; do
-    run_one node-check node --check "$f" || continue
+    run_one node-check node --check "./$f" || continue
     [ "$rc" = 0 ] || add node-check "$f" "$(sed -nE 's/^.*:([0-9]+)$/\1/p' "$out" | head -n1)" SyntaxError "$(grep -m1 'Error' "$out" || head -n1 "$out")"
   done <<<"$js_files"
 fi
 
 if [ -n "$(changed '\.(ts|tsx)$')" ] && [ -f tsconfig.json ] && have tsc; then
-  if run_one tsc tsc --noEmit -p tsconfig.json; then
+  if run_one tsc tsc --noEmit -p tsconfig.json && scan_ok tsc 1 2; then
     sed -nE 's/^(.*)\(([0-9]+),[0-9]+\): error (TS[0-9]+): (.*)$/\1\t\2\t\3\t\4/p' "$out" |
       while IFS=$'\t' read -r f l r m; do add tsc "$f" "$l" "$r" "$m"; done
   fi
 fi
 
 if [ -n "$(changed '\.go$')" ] && [ -f go.mod ] && have go; then
-  if run_one go-vet go vet ./...; then
+  if run_one go-vet go vet ./... && scan_ok go-vet 1; then
     sed -nE 's/^([^#[:space:]][^:]*\.go):([0-9]+):[0-9]+: (.*)$/\1\t\2\t\3/p' "$out" |
       while IFS=$'\t' read -r f l m; do add go-vet "$f" "$l" vet "$m"; done
   fi
@@ -116,42 +120,55 @@ fi
 json_files=$(changed '\.json$')
 if [ -n "$json_files" ] && have jq; then
   while IFS= read -r f; do
-    run_one jq jq empty "$f" || continue
+    run_one jq jq empty "./$f" || continue
     [ "$rc" = 0 ] || add jq "$f" "$(sed -nE 's/.*line ([0-9]+).*/\1/p' "$out" | head -n1)" invalid-json "$(head -n1 "$out")"
   done <<<"$json_files"
 fi
 
 # Scanner JSON (file $2, may have log noise before the JSON) of kind $1 -> TSV: file, line, rule, message.
+# Fails closed: missing, empty or malformed JSON exits non-zero instead of reading as "no findings".
 sjson() {
-  python3 - "$1" "$2" <<'PY' 2>/dev/null || true
+  python3 - "$1" "$2" <<'PY'
 import json, sys
 kind = sys.argv[1]
 t = open(sys.argv[2], encoding="utf-8", errors="replace").read()
 i = min([x for x in (t.find("{"), t.find("[")) if x >= 0], default=-1)
-d = json.JSONDecoder().raw_decode(t[i:])[0] if i >= 0 else []
+if i < 0:
+    sys.exit("sjson: no JSON in scanner output")
+d = json.JSONDecoder().raw_decode(t[i:])[0]
+# Wrong-shape JSON must not read as clean: gitleaks is a list, semgrep and osv an object with a "results" list.
+if kind == "gitleaks":
+    if not isinstance(d, list):
+        sys.exit("sjson: gitleaks report is not a list")
+elif not isinstance(d, dict) or not isinstance(d.get("results"), list):
+    sys.exit(f"sjson: {kind} output has no results list")
 rows = []
 if kind == "gitleaks":  # Secret/Match fields are deliberately never read
     rows = [(r.get("File"), r.get("StartLine"), r.get("RuleID"), r.get("Description") or "secret detected") for r in d]
 elif kind == "semgrep":
-    rows = [(r["path"], r["start"]["line"], r.get("check_id"), (r.get("extra") or {}).get("message", "")) for r in d.get("results") or []]
+    rows = [(r["path"], r["start"]["line"], r.get("check_id"), (r.get("extra") or {}).get("message", "")) for r in d["results"]]
 else:
-    for r in d.get("results") or []:
+    for r in d["results"]:
         for p in r.get("packages") or []:
             pk = p.get("package") or {}
             rows += [(r["source"]["path"], 0, v.get("id"), f"{pk.get('name')}@{pk.get('version')}: {v.get('summary', '')}")
                      for v in p.get("vulnerabilities") or []]
 for row in rows:
     print("\t".join(str(x).replace("\t", " ").replace("\n", " ") for x in row))
+sys.exit(3 if kind == "semgrep" and d.get("errors") else 0)  # 3 = rows printed, but the scan was partial
 PY
 }
 
 if have gitleaks; then
-  rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX"); trap 'rm -f "$raw" "$out" "$rep"' EXIT
+  rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX"); trap 'rm -f "$raw" "$out" "$tsv" "$rep"' EXIT
   while IFS= read -r f; do
     [ -f "$f" ] || continue
-    run_one gitleaks gitleaks detect --no-git --no-banner --redact --exit-code 0 -f json -r "$rep" -s "$f" || continue
-    sjson gitleaks "$rep" | while IFS=$'\t' read -r _ l r m; do add gitleaks "$f" "$l" "$r" "$m"; done
-    : >"$rep"
+    : >"$rep" # a stale report from the previous file must never be read for this one
+    run_one gitleaks gitleaks detect --no-git --no-banner --redact --exit-code 0 -f json -r "$rep" -s "./$f" || continue
+    scan_ok gitleaks || continue
+    [ -s "$rep" ] || echo '[]' >"$rep" # gitleaks may leave an empty report for a clean file
+    sjson gitleaks "$rep" >"$tsv" || { skip gitleaks "unparseable report for $f"; continue; }
+    while IFS=$'\t' read -r _ l r m; do add gitleaks "$f" "$l" "$r" "$m"; done <"$tsv"
   done <<<"$files"
 fi
 
@@ -160,19 +177,28 @@ if [ -z "$lock_files" ]; then :
 elif [ "${REVIEW_OSV:-}" != 1 ]; then skip osv-scanner "REVIEW_OSV not set; osv-scanner queries the OSV API"
 elif have osv-scanner; then
   while IFS= read -r f; do
-    run_one osv-scanner osv-scanner --format json -L "$f" || continue
-    sjson osv "$out" | while IFS=$'\t' read -r _ l r m; do add osv-scanner "$f" "$l" "$r" "$m"; done
+    run_one osv-scanner osv-scanner --format json -L "./$f" || continue
+    scan_ok osv-scanner 1 || continue # exit 1 = vulnerabilities found
+    sjson osv "$out" >"$tsv" || { skip osv-scanner "unparseable output for $f"; continue; }
+    while IFS=$'\t' read -r _ l r m; do add osv-scanner "$f" "$l" "$r" "$m"; done <"$tsv"
   done <<<"$lock_files"
 fi
 
 if [ -z "${REVIEW_SEMGREP_CONFIG:-}" ]; then skip semgrep "REVIEW_SEMGREP_CONFIG not set; --config auto is never used"
 elif [ ! -e "$REVIEW_SEMGREP_CONFIG" ]; then skip semgrep "REVIEW_SEMGREP_CONFIG must be an existing local path"
 elif have semgrep; then
-  while IFS= read -r f; do
-    [ -f "$f" ] || continue
-    run_one semgrep semgrep --config "$REVIEW_SEMGREP_CONFIG" --json --quiet --metrics=off --disable-version-check "$f" || continue
-    sjson semgrep "$out" | while IFS=$'\t' read -r _ l r m; do add semgrep "$f" "$l" "$r" "$m"; done
-  done <<<"$files"
+  sg=()
+  while IFS= read -r f; do [ -f "$f" ] && sg+=("$f"); done <<<"$files"
+  # One call per 200 files: typically a single run, but bounded argv and a timeout that is not shared by everything.
+  for ((i = 0; i < ${#sg[@]}; i += 200)); do
+    run_one semgrep semgrep --config "$REVIEW_SEMGREP_CONFIG" --json --quiet --metrics=off --disable-version-check -- "${sg[@]:i:200}" || continue
+    scan_ok semgrep || continue
+    e=0; sjson semgrep "$out" >"$tsv" || e=$?
+    if [ "$e" = 0 ] || [ "$e" = 3 ]; then
+      while IFS=$'\t' read -r p l r m; do add semgrep "$p" "$l" "$r" "$m"; done <"$tsv"
+      [ "$e" = 0 ] || skip semgrep "partial scan: semgrep reported errors"
+    else skip semgrep "unparseable output"; fi
+  done
 fi
 
 if [ "$tests" = 1 ]; then

changelog.d/security-folds.md

@@ -0,0 +1,4 @@
+### Fixed
+- `review_checks.sh` scanners now fail closed: malformed, empty, missing or wrong-shape scanner JSON, a semgrep `errors` array (partial scan) and crash exit codes from gitleaks, osv-scanner, semgrep, shellcheck, ruff, pyflakes, tsc and go vet print a `not run` line instead of reading as "no findings". Changed files reach the linters, gitleaks and osv-scanner as `./path` so a leading-dash filename is never read as an option, the gitleaks report is cleared before every run, and semgrep runs once per 200 changed files after `--`.
+- `SKILL.md` routes the whole-app security review reference lazily ("read only", it routes the rest) instead of "first".
+- size-budget-raise: .claude/skills/deep-code-review/SKILL.md 2884→2893 lazy whole-app security route wording

scripts/ci-tests.d/review-checks-security-folds.sh

@@ -0,0 +1,2 @@
+# semgrep batching, fail-closed scanner JSON and dash-filename tests for review_checks.sh.
+bash scripts/test-review-checks-security-folds.sh

scripts/test-review-checks-security-folds.sh

@@ -0,0 +1,108 @@
+#!/usr/bin/env bash
+# Regression tests for review_checks.sh folds: malformed, empty or wrong-shape scanner JSON, scanner/linter crash
+# codes and semgrep `errors` fail closed (a not-run line, never a silent clean); leading-dash filenames are never
+# read as options; semgrep runs once for a normal changed-file set. REVIEW_CHECKS overrides the script under test
+# (used to prove each check fails on the parent commit).
+# shellcheck disable=SC2015,SC2119,SC2120
+set -euo pipefail
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+RC="${REVIEW_CHECKS:-$ROOT/.claude/skills/deep-code-review/scripts/review_checks.sh}"
+T=$(mktemp -d "${TMPDIR:-/tmp}/dcr-test-rcfolds.XXXXXX"); trap 'rm -rf "$T"' EXIT
+fail=0
+ok() { printf 'ok   %s\n' "$1"; }
+bad() { printf 'FAIL %s\n' "$1"; fail=1; }
+
+mkdir "$T/stub" "$T/repo"
+cat >"$T/stub/semgrep" <<'S'
+#!/usr/bin/env bash
+echo "semgrep $*" >>"$SG_LOG"
+[ -z "${SG_EXIT:-}" ] || exit "$SG_EXIT"
+if [ -n "${SG_OUT:-}" ]; then echo "$SG_OUT"; else echo '{"results":[]}'; fi
+S
+cat >"$T/stub/gitleaks" <<'S'
+#!/usr/bin/env bash
+echo "gitleaks $*" >>"$GL_LOG"
+while [ $# -gt 0 ]; do [ "$1" = -r ] && rep=$2; shift; done
+[ -z "${GL_EXIT:-}" ] || exit "$GL_EXIT"
+if [ -n "${GL_FIRST_BAD:-}" ]; then # garbage on the first call only, nothing written afterwards
+  [ -e "$GL_FIRST_BAD" ] && exit 0
+  : >"$GL_FIRST_BAD"; printf 'not json' >"$rep"; exit 0
+fi
+printf '%s' "${GL_OUT-[]}" >"$rep"
+S
+cat >"$T/stub/osv-scanner" <<'S'
+#!/usr/bin/env bash
+echo "osv-scanner $*" >>"$OSV_LOG"
+echo "${OSV_OUT-{\"results\":[]\}}"
+exit "${OSV_EXIT:-0}"
+S
+for tool in shellcheck ruff node; do
+  cat >"$T/stub/$tool" <<S
+#!/usr/bin/env bash
+echo "$tool \$*" >>"\$TOOL_LOG"
+exit "\${EXIT_$tool:-0}"
+S
+done
+chmod +x "$T/stub"/*
+
+cd "$T/repo"
+git init -q . && git -c user.email=t@example.com -c user.name=t commit -q --allow-empty -m base
+git branch base
+printf 'a\n' >a.txt
+printf 'b\n' >b.txt
+printf '#!/bin/sh\necho hi\n' >./-v.sh
+printf 'x = 1\n' >./-p.py
+printf '1;\n' >./-n.js
+printf '{}\n' >./-j.json
+printf '{}\n' >package-lock.json
+git add -- . && git -c user.email=t@example.com -c user.name=t commit -q -m files
+export SG_LOG="$T/sg.log" GL_LOG="$T/gl.log" OSV_LOG="$T/osv.log" TOOL_LOG="$T/tool.log" REVIEW_SEMGREP_CONFIG="$T/rules.yml"
+echo "rules: []" >"$REVIEW_SEMGREP_CONFIG"
+: >"$SG_LOG"; : >"$GL_LOG"; : >"$OSV_LOG"; : >"$TOOL_LOG"
+run() { PATH="$T/stub:$PATH" REVIEW_OSV=1 bash "$RC" --base base "$@" 2>"$T/err" >"$T/out"; }
+errhas() { grep -qF -- "$1" "$T/err"; }
+findings() { jq --arg t "$1" '[.[] | select(.tool==$t)] | length' "$T/out"; }
+
+# Dash filenames: bash -n and jq would read them as options; every tool gets ./ or --.
+run && ok "exit 0" || bad "exit 0"
+jq -e 'any(.[]; .tool=="bash-n" or .tool=="jq")' "$T/out" >/dev/null && bad "dash file not read as option" || ok "dash file not read as option"
+grep -q -- '-s ./-v.sh' "$GL_LOG" && ok "gitleaks gets ./ path" || bad "gitleaks gets ./ path"
+grep -q -- '-L ./package-lock.json' "$OSV_LOG" && ok "osv-scanner gets ./ path" || bad "osv-scanner gets ./ path"
+grep -q 'shellcheck .* ./-v.sh' "$TOOL_LOG" && grep -q 'ruff .* ./-p.py' "$TOOL_LOG" && grep -q 'node --check ./-n.js' "$TOOL_LOG" && ok "linters get ./ path" || bad "linters get ./ path"
+# One semgrep call for the changed files, targets after --.
+[ "$(wc -l <"$SG_LOG")" -eq 1 ] && grep -q -- ' -- .*a.txt' "$SG_LOG" && grep -q -- 'b.txt' "$SG_LOG" && ok "semgrep batched once" || bad "semgrep batched once"
+
+# Malformed scanner output: not-run line, no findings, still exit 0.
+SG_OUT='not json' GL_OUT='not json' OSV_OUT='not json' run && ok "bad json exit 0" || bad "bad json exit 0"
+errhas 'not run: semgrep (unparseable output)' && errhas 'not run: gitleaks (unparseable report' && errhas 'not run: osv-scanner (unparseable output' && ok "bad json fails closed" || bad "bad json fails closed"
+[ "$(findings semgrep)$(findings gitleaks)$(findings osv-scanner)" = 000 ] && ok "bad json no findings" || bad "bad json no findings"
+
+# Wrong-shape JSON (valid JSON, missing results list) is not "clean".
+SG_OUT='{"x":1}' OSV_OUT='{"results":null}' GL_OUT='{"x":1}' run
+errhas 'not run: semgrep (unparseable output)' && errhas 'not run: osv-scanner (unparseable output' && errhas 'not run: gitleaks (unparseable report' && ok "wrong-shape json fails closed" || bad "wrong-shape json fails closed"
+
+# gitleaks: an empty report for a clean file is [], not an error.
+GL_OUT='' run
+errhas 'gitleaks (unparseable' && bad "empty gitleaks report is clean" || ok "empty gitleaks report is clean"
+
+# A bad report for one file must not leak into the next file's read.
+GL_FIRST_BAD="$T/gl-first" run
+[ "$(grep -c 'not run: gitleaks (unparseable report' "$T/err")" -eq 1 ] && ok "stale gitleaks report cleared" || bad "stale gitleaks report cleared"
+rm -f "$T/gl-first"
+
+# semgrep `errors` => partial-scan line, findings still reported.
+SG_OUT='{"results":[{"check_id":"r","path":"a.txt","start":{"line":1},"extra":{"message":"m"}}],"errors":[{"message":"boom"}]}' run
+errhas 'not run: semgrep (partial scan' && [ "$(findings semgrep)" = 1 ] && ok "semgrep errors surfaced" || bad "semgrep errors surfaced"
+
+# Crash codes surfaced; osv exit 1 (vulnerabilities found) is not a crash.
+SG_EXIT=3 run && ok "crash exit 0" || bad "crash exit 0"
+errhas 'not run: semgrep (exit 3)' && ok "semgrep crash surfaced" || bad "semgrep crash surfaced"
+GL_EXIT=2 run
+errhas 'not run: gitleaks (exit 2)' && ok "gitleaks crash surfaced" || bad "gitleaks crash surfaced"
+OSV_EXIT=127 run
+errhas 'not run: osv-scanner (exit 127)' && ok "osv crash surfaced" || bad "osv crash surfaced"
+OSV_EXIT=1 OSV_OUT='{"results":[{"source":{"path":"x"},"packages":[{"package":{"name":"p","version":"1"},"vulnerabilities":[{"id":"GHSA-x","summary":"s"}]}]}]}' run
+errhas 'not run: osv-scanner' && bad "osv exit 1 is findings" || [ "$(findings osv-scanner)" = 1 ] && ok "osv exit 1 is findings" || bad "osv exit 1 is findings"
+EXIT_shellcheck=2 EXIT_ruff=2 run
+errhas 'not run: shellcheck (exit 2)' && errhas 'not run: ruff (exit 2)' && ok "linter crash surfaced" || bad "linter crash surfaced"
+exit "$fail"