review_checks: run heavy scanners when the lease policy fails; add missing-tools row

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

@@ -20,7 +20,7 @@
 # Each tool is timeboxed (default 60s). Exit 0 even with findings; 2 on usage/unresolvable base.
 set -euo pipefail
 
-repo=. base=origin/main tests=0 hist=0 tmo=60 ran="" missing="" lease=ok leased=0 full=0 wr=0
+repo=. base=origin/main tests=0 hist=0 tmo=60 ran="" missing="" gap="" lease=ok leased=0 full=0 wr=0
 while [ $# -gt 0 ]; do
   case "$1" in
     --base) base=${2:?--base needs a ref}; case "$base" in -*) echo "review_checks: --base must not start with '-'" >&2; exit 2 ;; esac; shift 2 ;;
@@ -42,8 +42,8 @@ cd "$(git rev-parse --show-toplevel)"
 
 raw=$(mktemp "${TMPDIR:-/tmp}/review-raw.XXXXXX") out=$(mktemp "${TMPDIR:-/tmp}/review-out.XXXXXX") tsv=$(mktemp "${TMPDIR:-/tmp}/review-tsv.XXXXXX")
 sgh=$(mktemp -d "${TMPDIR:-/tmp}/review-sg.XXXXXX") # semgrep HOME: the macOS agent sandbox denies writes to ~/.semgrep
-rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX") gla=$(mktemp "${TMPDIR:-/tmp}/review-gla.XXXXXX") gli=$(mktemp "${TMPDIR:-/tmp}/review-gli.XXXXXX") # mktemp files are mode 600
-trap 'rm -rf "$raw" "$out" "$tsv" "$sgh" "$rep" "$gla" "$gli"' EXIT INT TERM HUP
+lf=$(mktemp "${TMPDIR:-/tmp}/review-lf.XXXXXX") rep=$(mktemp "${TMPDIR:-/tmp}/review-gl.XXXXXX") gla=$(mktemp "${TMPDIR:-/tmp}/review-gla.XXXXXX") gli=$(mktemp "${TMPDIR:-/tmp}/review-gli.XXXXXX") # mktemp files are mode 600
+trap 'rm -rf "$raw" "$out" "$tsv" "$sgh" "$lf" "$rep" "$gla" "$gli"' EXIT INT TERM HUP
 # Vendored, build and worktree dirs are never scanned (a changed file under them is dropped here, for every tool).
 XRE='(^|/)(node_modules|\.claude/worktrees|vendor|dist|build|\.git|\.venv|venv|target|__pycache__)/'
 if [ "$wr" = 1 ]; then all=$(git -c core.quotePath=false diff --name-only --diff-filter=ACMR "$base" HEAD); else all=$(git -c core.quotePath=false diff --name-only --diff-filter=ACMR "$base"...HEAD); fi
@@ -69,21 +69,30 @@ tbox() {
 pp="$here/../../agentic-delivery/scripts/perun_policy.py"
 lease_take() {
   [ "$leased" = 1 ] && return 0
-  [ "$lease" = ok ] || return 1
+  [ "$lease" = none ] && return 0 # policy unusable (reported once): no lease policy, so the scanner runs
+  [ "$lease" = ok ] || return 1 # full: already waited REVIEW_LEASE_WAIT, stay skipped
   [ -f "$pp" ] && command -v python3 >/dev/null || return 0
   local w=${REVIEW_LEASE_WAIT:-120} r
   while :; do
-    r=0; python3 "$pp" heavy-acquire >/dev/null 2>&1 || r=$?
+    # stdout to a file, not $(...): the lease pid is the caller's parent, which must stay this shell. A full lease is
+    # exit 1 AND stdout "FULL"; python also exits 1 on any uncaught exception (traceback, no FULL), which is a policy failure.
+    r=0; python3 "$pp" heavy-acquire >"$lf" 2>/dev/null || r=$?
+    [ "$r" != 1 ] || [ "$(cat "$lf")" = FULL ] || r=70
     case "$r" in
       0) leased=1; return 0 ;;
       1) if [ "$w" -le 0 ]; then lease=full; return 1; fi; sleep 5; w=$((w - 5)) ;;
-      *) skip lease "policy rc=$r"; lease=none; return 0 ;;
+      *) echo "review_checks: not run: lease (policy rc=$r); heavy scanners run without a lease" >&2; lease=none; return 0 ;;
     esac
   done
 }
 lease_drop() { [ "$leased" = 1 ] && { python3 "$pp" heavy-release >/dev/null 2>&1 || true; leased=0; }; true; }
+# miss TOOL HINT: queue a tool that could not run (not installed, or its DB is blocked) for the closing "missing tools" row.
+miss() { gap+="${gap:+; }$1 ($2)"; }
 have() {
-  command -v "$1" >/dev/null || { skip "$1" "not installed"; missing+=" $1"; return 1; }
+  command -v "$1" >/dev/null || {
+    skip "$1" "not installed"; missing+=" $1"
+    case "$1" in semgrep) miss "$1" "install: pipx install semgrep" ;; tsc) miss "$1" "install: npm install -g typescript" ;; *) miss "$1" "install: brew install $1" ;; esac
+    return 1; }
   case "$1" in gitleaks|osv-scanner|semgrep|trivy) lease_take || { skip "$1" "heavy lease full"; return 1; } ;; esac
 }
 # add TOOL FILE LINE RULE MSG -> one TSV row.
@@ -325,6 +334,7 @@ lock_files=$(changed '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|go\.su
 if [ -z "$lock_files" ]; then :
 elif [ "${REVIEW_OSV:-}" != 1 ] && [ -z "${REVIEW_OSV_DB_DIR:-}" ]; then skip osv-scanner "REVIEW_OSV not set; osv-scanner queries the OSV API"
 elif [ -n "${REVIEW_OSV_DB_DIR:-}" ] && [ -z "$(find "$REVIEW_OSV_DB_DIR" -type f -name '*.zip' 2>/dev/null | head -n1)" ]; then
+  miss osv-scanner "warm the DB: osv-scanner --offline-vulnerabilities --download-offline-databases"
   skip osv-scanner "NOT CHECKED: no pre-seeded DB in REVIEW_OSV_DB_DIR=$REVIEW_OSV_DB_DIR; warm it once: OSV_SCANNER_LOCAL_DB_CACHE_DIRECTORY=$REVIEW_OSV_DB_DIR osv-scanner --offline-vulnerabilities --download-offline-databases ."
 elif have osv-scanner; then
   # REVIEW_OSV_DB_DIR: pre-seeded DB, offline run (no TLS to api.osv.dev); the cache-dir env is osv-scanner's documented one.
@@ -377,7 +387,7 @@ if [ -n "$files" ] && have trivy; then
     if ! grep -Eqi 'vulnerability db|trivy-db|download.* db( |$)' "$out" && grep -Eqi 'dial |no such host|connection (refused|reset)|unreachable|x509|TLS|failed to download|download.*(bundle|check)|(bundle|check).*download' "$out"; then
       echo "review_checks: trivy: secret-only (misconfig bundle unreachable)" >&2; tk=trivy-secret; ran+=" trivy(secret-only)"
       run_one trivy "${tv[@]}" --skip-check-update --scanners secret .
-    fi; }; } && { scan_ok trivy || { grep -qi 'DB' "$out" && echo "review_checks: trivy: NOT CHECKED, $trivy_hint" >&2; false; }; }; then
+    fi; }; } && { scan_ok trivy || { grep -qi 'DB' "$out" && { echo "review_checks: trivy: NOT CHECKED, $trivy_hint" >&2; miss trivy "$trivy_hint"; }; false; }; }; then
     if sjson "$tk" "$out" >"$tsv"; then
       while IFS=$'\t' read -r f l r m; do printf '%s\n' "$files" | grep -qxF -- "$f" && add trivy "$f" "$l" "$r" "$m"; done <"$tsv"
     else skip trivy "unparseable output"; fi
@@ -412,8 +422,9 @@ while IFS=';' read -r hn hs hm hre _ hh; do
   hf=$(changed "$hre"); [ -n "$hf" ] || continue
   nc() { echo "NOT CHECKED: $hn ($1)" >&2; add "$hn" "" 0 not-checked "$1"; }
   if [ "$hs" != verified ]; then nc "$hs flags: skipped by default"; continue; fi
-  command -v "$hn" >/dev/null || { nc "install: $hh"; missing+=" $hn"; continue; }
+  command -v "$hn" >/dev/null || { nc "install: $hh"; missing+=" $hn"; miss "$hn" "install: $hh"; continue; }
   hubtool=$hn
+  # zizmor, bandit and hadolint are light (single-file static checks), so they are intentionally unleased.
   case $hn in grype|checkov|gosec) lease_take || { skip "$hn" "heavy lease full"; continue; } ;; esac
   [ "$hn" != gosec ] || [ -f go.mod ] || { nc "no go.mod at repo root"; continue; }
   targets=()
@@ -453,3 +464,4 @@ print(json.dumps([{"file": t[1], "line": int(t[2]) if t[2].isdigit() else 0, "ru
 n = sum(len(t) == 5 for t in rows)
 print(f"review_checks: {n} findings; {sys.argv[4]} files excluded; analyzers ran: {', '.join(sys.argv[2].split()) or 'none'}; skipped (not installed): {', '.join(sys.argv[3].split()) or 'none'}", file=sys.stderr)
 PY
+[ -z "$gap" ] || echo "review_checks: missing tools: $gap" >&2

SHA256SUMS

@@ -388,7 +388,7 @@ cdacdfba4a9aff3c20344bfc8a4fe84b6ed24e85e9b5feefb2a08058f8858bda  .claude/skills
 9cf250be85e7c589979ca6a4f2774cf9a692695d94d46f6ded79480dea18fbe5  .claude/skills/deep-code-review/scripts/partition_diff.py
 5d6910164ddf6dfd4ed62dfc365a2f615f49dc07878ed3d1f881aa9518e481c2  .claude/skills/deep-code-review/scripts/post_review.sh
 f1108e2700864195a239744b319b1d04953879589f8a2b83708bc45a080e3d86  .claude/skills/deep-code-review/scripts/reaper_lint.py
-87395b898841a00ac1660d69f51196b92dd01ad9c3c16f31783672086e094fad  .claude/skills/deep-code-review/scripts/review_checks.sh
+0959dd599affc9f35f9d094daaa3b32c235bc9f1d79894e344004bd6ecae4059  .claude/skills/deep-code-review/scripts/review_checks.sh
 8c87c02a050a975e453b52ecacbaf01481c7134466a9c67feaf409d22e36d9f7  .claude/skills/deep-code-review/scripts/review_feedback.py
 859c67d6a6a781c873f47d5662d403da2321da53e6994ca3d0ccdbf74afd39ac  .claude/skills/deep-code-review/scripts/score_review.py
 51c50eef1e4c5f417e56ee89b6804068052333ac8b8764c3b6ffcb2f3b8c185e  .claude/skills/deep-code-review/scripts/security_verify.py

changelog.d/review-checks-lease-policy.md

@@ -0,0 +1,3 @@
+### Fixed
+- `review_checks.sh`: when the heavy-lease policy exits with an unexpected code (rc=2, for example an unusable lease directory), the scanners now run without a lease instead of every later heavy scanner (semgrep, trivy, osv-scanner, gitleaks, grype, checkov, gosec) being skipped with a false "heavy lease full", including under `--full`. The policy failure is reported once on stderr and no longer adds a bogus not-checked row to the scanner that happened to hit it first. A genuinely full lease still skips, but "full" is now an explicit signal (exit 1 and stdout `FULL`); an exit 1 from a Python traceback is a policy failure, not a full lease.
+- `review_checks.sh`: a tool that is not installed, or whose offline DB is missing (osv-scanner, trivy), is now listed in one closing `review_checks: missing tools: <tool> (<hint>); ...` line on stderr, with an install or warm-the-DB hint for each. New test `scripts/test-review-checks-lease-policy.sh`, registered in `scripts/ci-tests.d/`.

scripts/ci-tests.d/review-checks-lease-policy.sh

@@ -0,0 +1,2 @@
+# review_checks.sh heavy lease policy failure (rc=2) runs scanners unleased; closing "missing tools" row.
+bash scripts/test-review-checks-lease-policy.sh

scripts/test-review-checks-lease-policy.sh

@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# review_checks.sh heavy lease: an unusable lease policy (perun_policy.py exits 2) must mean "run without a lease", never a
+# false "heavy lease full" skip for semgrep/trivy, in diff mode and --full. Also covers the closing "missing tools" row (tool + hint).
+set -euo pipefail
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+RC=${RC_UNDER_TEST:-$ROOT/.claude/skills/deep-code-review/scripts/review_checks.sh} # override: run the test against another copy of the script
+T=$(mktemp -d "${TMPDIR:-/tmp}/dcr-test-lease.XXXXXX"); trap 'rm -rf "$T"' EXIT
+pass=0 total=0
+chk() { total=$((total + 1)); if [ "$1" -eq 0 ]; then pass=$((pass + 1)); echo "PASS  $2"; else echo "FAIL  $2"; fi; }
+g() { git -c user.email=t@example.com -c user.name=t "$@"; }
+mkdir "$T/bin" "$T/repo"
+cat >"$T/bin/semgrep" <<'S'
+#!/usr/bin/env bash
+echo ran >>"$SG_LOG"; echo '{"results":[]}'
+S
+cat >"$T/bin/trivy" <<'S'
+#!/usr/bin/env bash
+echo ran >>"$TV_LOG"; echo '{"Results":[]}'
+S
+chmod +x "$T/bin"/*
+export SG_LOG="$T/sg.log" TV_LOG="$T/tv.log"
+cd "$T/repo"
+git init -q . && g commit -q --allow-empty -m base && git branch base
+echo x >app.txt && g add . && g commit -q -m files
+: >"$T/notadir" # a file where the lease dir should be: heavy-acquire raises OSError -> rc=2
+export PERUN_HEAVY_DIR="$T/notadir"
+run() { : >"$SG_LOG"; : >"$TV_LOG"; PATH="$T/bin:$PATH" bash "$RC" --base base "$@" 2>"$T/err" >"$T/out"; }
+
+for mode in "" --full; do
+  run $mode
+  r=0; [ -s "$SG_LOG" ] && [ -s "$TV_LOG" ] || r=1; chk $r "policy rc=2 ${mode:-diff}: semgrep and trivy both ran"
+  r=0; ! grep -q 'heavy lease full' "$T/err" || r=1; chk $r "policy rc=2 ${mode:-diff}: no false 'heavy lease full'"
+done
+r=0; [ "$(grep -c 'not run: lease (policy rc=2)' "$T/err")" = 1 ] || r=1; chk $r "policy failure reported once"
+
+# Stub policy tree: "full" is exit 1 AND stdout FULL; a crash (traceback, exit 1) is a policy failure, not a full lease.
+SK="$T/sk"; mkdir -p "$SK/deep-code-review/scripts" "$SK/agentic-delivery/scripts"
+cp "$RC" "$SK/deep-code-review/scripts/"; cp -R "$(dirname "$RC")/../templates" "$SK/deep-code-review/templates"
+stubrun() { : >"$SG_LOG"; : >"$TV_LOG"; printf '%s\n' "$1" >"$SK/agentic-delivery/scripts/perun_policy.py"
+  PATH="$T/bin:$PATH" REVIEW_LEASE_WAIT=0 bash "$SK/deep-code-review/scripts/review_checks.sh" --base base 2>"$T/err" >"$T/out"; }
+stubrun 'raise ModuleNotFoundError("boom")'
+r=0; [ -s "$SG_LOG" ] && [ -s "$TV_LOG" ] && ! grep -q 'heavy lease full' "$T/err" || r=1; chk $r "policy traceback (exit 1): scanners run, no 'heavy lease full'"
+stubrun 'import sys; print("FULL"); sys.exit(1)'
+r=0; [ ! -s "$SG_LOG" ] && [ ! -s "$TV_LOG" ] && grep -q 'semgrep: heavy lease full\|semgrep (heavy lease full)' "$T/err" || r=1; chk $r "explicit FULL (exit 1): scanners skipped as 'heavy lease full'"
+stubrun 'import os; print(os.getppid())'
+r=0; [ -s "$SG_LOG" ] && [ -s "$TV_LOG" ] || r=1; chk $r "exit 0: scanners run under the lease"
+
+# missing tools: one closing row with a hint per tool (skipped when the host has a real trivy/semgrep on the minimal PATH)
+if ! PATH="/usr/bin:/bin" sh -c 'command -v trivy || command -v semgrep' >/dev/null; then
+  PATH="/usr/bin:/bin" bash "$RC" --base base 2>"$T/err" >/dev/null || true
+  row=$(grep '^review_checks: missing tools:' "$T/err" || true)
+  r=0; [ "$(grep -c '^review_checks: missing tools:' "$T/err")" = 1 ] && [[ $row == *"semgrep (install: "* && $row == *"trivy (install: "* ]] || r=1
+  chk $r "one 'missing tools' row lists semgrep and trivy with hints"
+  r=0; [ "$(tail -n1 "$T/err")" = "$row" ] || r=1; chk $r "'missing tools' row is the last line"
+else chk 0 "missing tools (skipped: real scanner on the minimal PATH)"; fi
+echo "Tests: $pass/$total"; [ "$pass" -eq "$total" ]