fix: address review feedback on check-deps script

- Accept import paths starting with digit (e.g., 9fans.net/go) by
  relaxing filter from ^[a-zA-Z] to ^[[:alnum:]]
- Fix misleading comment: loop uses Bash process substitution, not POSIX
- Remove redundant \$ from grep regex (literal $ unreachable)
- Capture stderr separately in go list to avoid mixing errors with output
- Clarify CONVENTIONS.md wording on transitive vs direct deps

Reviewed-by: sonnet-review-bot
Reviewed-by: gpt-review-bot

CONVENTIONS.md

@@ -7,18 +7,22 @@
 
 ### Approved Third-Party Packages
 
-| Package | Use Case |
+| Package | Use Case | Scope |
-|---------|----------|
+|---------|----------|-------|
-| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) |
+| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
-| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) |
+| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
 
 **Any import not in this table or the Go standard library is forbidden.**
 
+Only *direct* dependencies (listed in go.mod without `// indirect`) are checked against this allowlist. Transitive dependencies pulled in by approved packages are implicitly allowed.
+
 To request a new dependency:
-1. Open a PR that ONLY updates this table with justification
+1. Open a PR that ONLY updates this table
 2. Requires explicit approval from Aaron
 3. After merge, a separate PR may use the package
 
+*Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
+
 ## Error Handling
 
 - Return errors; never panic.

Makefile

@@ -1,4 +1,4 @@
-.PHONY: build test test-integration lint clean coverage check-deps
+.PHONY: build test test-integration lint clean coverage check-deps precommit
 
 build:
 	go build -o review-bot ./cmd/review-bot/

scripts/check-deps.sh

@@ -1,61 +1,133 @@
-#!/bin/bash
+#!/usr/bin/env bash
 # check-deps.sh - Enforces the strict dependency allowlist from CONVENTIONS.md
 # Exit 1 if any unapproved import is found.
+#
+# Requires: Bash 4+ (for associative arrays), Go toolchain
+# 
+# The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth.
+# Enforces Scope column: "test only" packages cannot appear in non-test code.
 
 set -euo pipefail
 
-# Approved third-party packages (from CONVENTIONS.md)
+# Check bash version
-ALLOWED=(
+if ((BASH_VERSINFO[0] < 4)); then
-    "gopkg.in/yaml.v3"
+    echo "❌ Bash 4+ required (found ${BASH_VERSION})"
-    "github.com/google/go-cmp"
+    echo "   On macOS: brew install bash"
-)
+    exit 1
+fi
 
-# Build regex pattern from allowed list
+CONVENTIONS_FILE="${1:-CONVENTIONS.md}"
-ALLOWED_PATTERN=""
+
-for pkg in "${ALLOWED[@]}"; do
+if [ ! -f "$CONVENTIONS_FILE" ]; then
-    if [ -z "$ALLOWED_PATTERN" ]; then
+    echo "❌ CONVENTIONS.md not found"
-        ALLOWED_PATTERN="$pkg"
+    exit 1
-    else
+fi
-        ALLOWED_PATTERN="$ALLOWED_PATTERN|$pkg"
+
+# Parse approved packages from CONVENTIONS.md table
+# Note: uses Bash process substitution (< <(...)) for the loop
+# Format: | `package` | use case | scope |
+declare -A ALLOWED_PROD=()
+declare -A ALLOWED_TEST=()
+
+while IFS= read -r line; do
+    # Use awk to extract package and scope from table row
+    pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}')
+    scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}')
+    
+    # Accept packages starting with letter or digit (e.g., 9fans.net/go)
+    if [ -n "$pkg" ] && [ "$pkg" != "Package" ] && [[ "$pkg" =~ ^[[:alnum:]] ]]; then
+        if [[ "$scope" == *"test"* ]]; then
+            ALLOWED_TEST["$pkg"]=1
+        else
+            ALLOWED_PROD["$pkg"]=1
+        fi
     fi
-done
+done < <(grep '| `' "$CONVENTIONS_FILE" 2>/dev/null || true)
 
-# Get all imports from go.mod (excluding the module itself and stdlib)
+ALL_ALLOWED=("${!ALLOWED_PROD[@]}" "${!ALLOWED_TEST[@]}")
-IMPORTS=$(go list -m all 2>/dev/null | tail -n +2 | awk '{print $1}' || true)
 
-if [ -z "$IMPORTS" ]; then
+if [ ${#ALL_ALLOWED[@]} -eq 0 ]; then
+    echo "⚠️  No approved packages found in $CONVENTIONS_FILE"
+    echo "   (This is fine if you want stdlib-only)"
+fi
+
+# Helper: check if import matches any package in an associative array (literal prefix, no glob)
+matches_allowlist() {
+    local import="$1"
+    shift
+    local -n allowlist=$1
+    
+    for allowed in "${!allowlist[@]}"; do
+        # Exact match
+        if [ "$import" = "$allowed" ]; then
+            return 0
+        fi
+        # Literal prefix match for subpackages: must match "pkg/" exactly
+        if [ "${import#"$allowed/"}" != "$import" ]; then
+            return 0
+        fi
+    done
+    return 1
+}
+
+# Get direct module dependencies from go.mod
+# Capture stderr separately to avoid mixing error messages with package list
+GO_LIST_STDERR=$(mktemp)
+trap 'rm -f "$GO_LIST_STDERR"' EXIT
+DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>"$GO_LIST_STDERR") || {
+    echo "❌ Failed to list dependencies:"
+    cat "$GO_LIST_STDERR"
+    exit 1
+}
+DIRECT_IMPORTS=$(echo "$DIRECT_IMPORTS" | grep -v '^$' || true)
+
+if [ -z "$DIRECT_IMPORTS" ]; then
     echo "✅ No external dependencies"
     exit 0
 fi
 
+# Check ALL direct dependencies are in some allowlist
 VIOLATIONS=""
 while IFS= read -r import; do
-    # Skip empty lines
     [ -z "$import" ] && continue
     
-    # Check if import matches any allowed pattern (prefix match for subpackages)
+    if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then
-    MATCHED=false
+        VIOLATIONS="${VIOLATIONS}  - ${import} (not in allowlist)"$'\n'
-    for allowed in "${ALLOWED[@]}"; do
-        if [[ "$import" == "$allowed" ]] || [[ "$import" == "$allowed/"* ]]; then
-            MATCHED=true
-            break
-        fi
-    done
-    
-    if [ "$MATCHED" = false ]; then
-        VIOLATIONS="$VIOLATIONS\n  - $import"
     fi
-done <<< "$IMPORTS"
+done <<< "$DIRECT_IMPORTS"
 
 if [ -n "$VIOLATIONS" ]; then
     echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
-    echo -e "The following imports are not in the allowlist:$VIOLATIONS"
     echo ""
-    echo "To add a dependency:"
+    echo "The following imports are not in the allowlist:"
-    echo "  1. Open a PR that ONLY updates CONVENTIONS.md"
+    printf "%s" "$VIOLATIONS"
-    echo "  2. Get explicit approval from Aaron"
+    echo ""
-    echo "  3. After merge, use the package in a separate PR"
+    echo "To add a dependency, update CONVENTIONS.md (requires Aaron's approval)"
+    exit 1
+fi
+
+# Enforce Scope: test-only packages must not appear in non-test code
+# Get imports used by non-test code only (go list -deps without -test excludes test deps)
+PROD_IMPORTS=$(go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ./... 2>/dev/null || true)
+
+TEST_ONLY_IN_PROD=""
+for test_pkg in "${!ALLOWED_TEST[@]}"; do
+    # Match exact package or subpackages (pkg or pkg/...)
+    if echo "$PROD_IMPORTS" | grep -qE "^${test_pkg}(/|$)"; then
+        TEST_ONLY_IN_PROD="${TEST_ONLY_IN_PROD}  - ${test_pkg} (marked 'test only' but used in production code)"$'\n'
+    fi
+done
+
+if [ -n "$TEST_ONLY_IN_PROD" ]; then
+    echo "❌ TEST-ONLY DEPENDENCIES IN PRODUCTION CODE"
+    echo ""
+    printf "%s" "$TEST_ONLY_IN_PROD"
+    echo ""
+    echo "These packages are marked 'test only' in CONVENTIONS.md"
+    echo "and must only be imported from *_test.go files."
     exit 1
 fi
 
 echo "✅ All dependencies are approved"
+echo "   Direct module deps: $(echo "$DIRECT_IMPORTS" | wc -l | tr -d ' ')"
+echo "   Production allowlist: ${#ALLOWED_PROD[@]}, Test-only allowlist: ${#ALLOWED_TEST[@]}"