From 70267b68f4ded61c5ced43ede5a9f3e131c34ced Mon Sep 17 00:00:00 2001
From: Rodin <rodin@foredthought.ai>
Date: Sun, 10 May 2026 13:53:55 -0700
Subject: [PATCH] fix: address review feedback on dependency allowlist

Fixes:
- Single source of truth: script now parses allowlist from CONVENTIONS.md
- Fail closed: script exits non-zero if 'go list' fails
- Direct deps only: uses '-f' flag to exclude transitive deps
- Added 'precommit' to .PHONY in Makefile
- Removed unused ALLOWED_PATTERN variable
- Added Scope column to distinguish test-only vs production deps
- Clarified that transitive deps of approved packages are allowed
- Added note that enforcement script parses the table
---
 CONVENTIONS.md        | 14 ++++++----
 Makefile              |  2 +-
 scripts/check-deps.sh | 61 +++++++++++++++++++++++++++++--------------
 3 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/CONVENTIONS.md b/CONVENTIONS.md
index abfba06..5c3839c 100644
--- a/CONVENTIONS.md
+++ b/CONVENTIONS.md
@@ -7,18 +7,22 @@
 
 ### Approved Third-Party Packages
 
-| Package | Use Case |
-|---------|----------|
-| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) |
-| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) |
+| Package | Use Case | Scope |
+|---------|----------|-------|
+| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
+| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
 
 **Any import not in this table or the Go standard library is forbidden.**
 
+Transitive dependencies of approved packages are automatically 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.
diff --git a/Makefile b/Makefile
index e5dde12..389bad8 100644
--- a/Makefile
+++ b/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/
diff --git a/scripts/check-deps.sh b/scripts/check-deps.sh
index cd1cb05..8e058dd 100755
--- a/scripts/check-deps.sh
+++ b/scripts/check-deps.sh
@@ -1,27 +1,43 @@
 #!/bin/bash
 # check-deps.sh - Enforces the strict dependency allowlist from CONVENTIONS.md
 # Exit 1 if any unapproved import is found.
+# 
+# The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth.
 
 set -euo pipefail
 
-# Approved third-party packages (from CONVENTIONS.md)
-ALLOWED=(
-    "gopkg.in/yaml.v3"
-    "github.com/google/go-cmp"
-)
+CONVENTIONS_FILE="${1:-CONVENTIONS.md}"
 
-# Build regex pattern from allowed list
-ALLOWED_PATTERN=""
-for pkg in "${ALLOWED[@]}"; do
-    if [ -z "$ALLOWED_PATTERN" ]; then
-        ALLOWED_PATTERN="$pkg"
-    else
-        ALLOWED_PATTERN="$ALLOWED_PATTERN|$pkg"
+if [ ! -f "$CONVENTIONS_FILE" ]; then
+    echo "❌ CONVENTIONS.md not found"
+    exit 1
+fi
+
+# Parse approved packages from CONVENTIONS.md table
+# Looks for lines like: | `gopkg.in/yaml.v3` | ...
+ALLOWED=()
+while IFS= read -r line; do
+    # Extract package from markdown table cell: | `package` |
+    pkg=$(echo "$line" | grep -oP '\| `\K[^`]+' | head -1 || true)
+    if [ -n "$pkg" ] && [[ "$pkg" != "Package" ]]; then
+        ALLOWED+=("$pkg")
     fi
-done
+done < <(grep -E '^\| `[a-zA-Z]' "$CONVENTIONS_FILE" || true)
 
-# Get all imports from go.mod (excluding the module itself and stdlib)
-IMPORTS=$(go list -m all 2>/dev/null | tail -n +2 | awk '{print $1}' || true)
+if [ ${#ALLOWED[@]} -eq 0 ]; then
+    echo "⚠️  No approved packages found in $CONVENTIONS_FILE"
+    echo "   (This is fine if you want stdlib-only)"
+fi
+
+# Get DIRECT dependencies only (exclude indirect/transitive)
+# Fail closed: if go list fails, we exit non-zero
+IMPORTS=$(go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all 2>&1) || {
+    echo "❌ Failed to list dependencies: $IMPORTS"
+    exit 1
+}
+
+# Filter out the module itself (first line) and empty lines
+IMPORTS=$(echo "$IMPORTS" | tail -n +2 | grep -v '^$' || true)
 
 if [ -z "$IMPORTS" ]; then
     echo "✅ No external dependencies"
@@ -30,10 +46,9 @@ fi
 
 VIOLATIONS=""
 while IFS= read -r import; do
-    # Skip empty lines
     [ -z "$import" ] && continue
     
-    # Check if import matches any allowed pattern (prefix match for subpackages)
+    # Check if import matches any allowed package (prefix match for subpackages)
     MATCHED=false
     for allowed in "${ALLOWED[@]}"; do
         if [[ "$import" == "$allowed" ]] || [[ "$import" == "$allowed/"* ]]; then
@@ -43,13 +58,20 @@ while IFS= read -r import; do
     done
     
     if [ "$MATCHED" = false ]; then
-        VIOLATIONS="$VIOLATIONS\n  - $import"
+        VIOLATIONS="${VIOLATIONS}  - ${import}"$'\n'
     fi
 done <<< "$IMPORTS"
 
 if [ -n "$VIOLATIONS" ]; then
     echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
-    echo -e "The following imports are not in the allowlist:$VIOLATIONS"
+    echo ""
+    echo "The following imports are not in the allowlist:"
+    printf "%s" "$VIOLATIONS"
+    echo ""
+    echo "Approved packages (from CONVENTIONS.md):"
+    for pkg in "${ALLOWED[@]}"; do
+        echo "  - $pkg"
+    done
     echo ""
     echo "To add a dependency:"
     echo "  1. Open a PR that ONLY updates CONVENTIONS.md"
@@ -59,3 +81,4 @@ if [ -n "$VIOLATIONS" ]; then
 fi
 
 echo "✅ All dependencies are approved"
+echo "   Direct deps: $(echo "$IMPORTS" | wc -l | tr -d ' ')"