From 4b96231b328ac0de46eb736a7f5b4054d231da89 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 13:43:59 -0700 Subject: [PATCH 1/4] docs: strict dependency allowlist with CI enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit STRICT ALLOWLIST policy: Only packages explicitly listed in CONVENTIONS.md may be imported. No exceptions. ## Changes - Updates CONVENTIONS.md with strict allowlist language - Adds scripts/check-deps.sh to enforce the allowlist - Adds 'make check-deps' and 'make precommit' targets - CI will fail if any unapproved dependency is detected ## Approved packages - gopkg.in/yaml.v3 — YAML parsing - github.com/google/go-cmp — test comparisons ## Process for new dependencies 1. Open a PR that ONLY updates CONVENTIONS.md 2. Requires explicit approval from Aaron 3. After merge, a separate PR may use the package --- CONVENTIONS.md | 16 +++++++++++- Makefile | 8 +++++- scripts/check-deps.sh | 61 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100755 scripts/check-deps.sh diff --git a/CONVENTIONS.md b/CONVENTIONS.md index ce4406a..abfba06 100644 --- a/CONVENTIONS.md +++ b/CONVENTIONS.md @@ -2,8 +2,22 @@ ## Language & Dependencies -- Go standard library only — no external dependencies. - Target the latest stable Go release. +- **STRICT ALLOWLIST:** Only packages listed below may be imported. No exceptions. + +### 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`) | + +**Any import not in this table or the Go standard library is forbidden.** + +To request a new dependency: +1. Open a PR that ONLY updates this table with justification +2. Requires explicit approval from Aaron +3. After merge, a separate PR may use the package ## Error Handling diff --git a/Makefile b/Makefile index 0f2e182..e5dde12 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test test-integration lint clean coverage +.PHONY: build test test-integration lint clean coverage check-deps build: go build -o review-bot ./cmd/review-bot/ @@ -12,9 +12,15 @@ test-integration: lint: go vet ./... +check-deps: + @./scripts/check-deps.sh + clean: rm -f review-bot coverage: go test -coverprofile=coverage.out ./... go tool cover -func=coverage.out + +# Precommit runs all checks required before pushing +precommit: check-deps lint test diff --git a/scripts/check-deps.sh b/scripts/check-deps.sh new file mode 100755 index 0000000..cd1cb05 --- /dev/null +++ b/scripts/check-deps.sh @@ -0,0 +1,61 @@ +#!/bin/bash +# check-deps.sh - Enforces the strict dependency allowlist from CONVENTIONS.md +# Exit 1 if any unapproved import is found. + +set -euo pipefail + +# Approved third-party packages (from CONVENTIONS.md) +ALLOWED=( + "gopkg.in/yaml.v3" + "github.com/google/go-cmp" +) + +# 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" + fi +done + +# 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 [ -z "$IMPORTS" ]; then + echo "✅ No external dependencies" + exit 0 +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) + MATCHED=false + 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" + +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 " 1. Open a PR that ONLY updates CONVENTIONS.md" + echo " 2. Get explicit approval from Aaron" + echo " 3. After merge, use the package in a separate PR" + exit 1 +fi + +echo "✅ All dependencies are approved" -- 2.47.3 From 70267b68f4ded61c5ced43ede5a9f3e131c34ced Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 13:53:55 -0700 Subject: [PATCH 2/4] 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 ' ')" -- 2.47.3 From aeb0c8cb79efaa8df8964f2d6ca676d578e2792c Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 13:57:49 -0700 Subject: [PATCH 3/4] fix: enforce Scope column and improve portability Addresses review feedback: 1. MAJOR - Scope enforcement: Script now parses the Scope column and ensures 'test only' packages don't appear in non-test code. Uses 'go list -deps' to check production imports. 2. MINOR - Portability: Replaced 'grep -P' (GNU-only) with awk-based parsing that works on macOS/BSD. 3. MINOR - Robustness: Table parsing uses awk to split on '|' and extract columns properly, handling whitespace variations. 4. MINOR - Glob safety: Prefix matching now uses parameter expansion instead of glob patterns to prevent metacharacter issues. --- scripts/check-deps.sh | 98 ++++++++++++++++++++++++++++++------------- 1 file changed, 68 insertions(+), 30 deletions(-) diff --git a/scripts/check-deps.sh b/scripts/check-deps.sh index 8e058dd..1d2999c 100755 --- a/scripts/check-deps.sh +++ b/scripts/check-deps.sh @@ -3,6 +3,7 @@ # Exit 1 if any unapproved import is found. # # The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth. +# Also enforces Scope column: "test only" packages cannot appear in non-test code. set -euo pipefail @@ -13,22 +14,53 @@ if [ ! -f "$CONVENTIONS_FILE" ]; then 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 < <(grep -E '^\| `[a-zA-Z]' "$CONVENTIONS_FILE" || true) +# Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible) +# Format: | `package` | use case | scope | +# Output: package:scope (e.g., "gopkg.in/yaml.v3:production") +declare -A ALLOWED_PROD=() +declare -A ALLOWED_TEST=() -if [ ${#ALLOWED[@]} -eq 0 ]; then +while IFS= read -r line; do + # Use awk to extract package and scope from table row + # Split on | and extract backtick-wrapped package from column 1, scope from column 3 + pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}') + scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}') + + if [ -n "$pkg" ] && [ "$pkg" != "Package" ] && [[ "$pkg" =~ ^[a-zA-Z] ]]; then + if [[ "$scope" == *"test"* ]]; then + ALLOWED_TEST["$pkg"]=1 + else + ALLOWED_PROD["$pkg"]=1 + fi + fi +done < <(grep '| `' "$CONVENTIONS_FILE" 2>/dev/null || true) + +ALL_ALLOWED=("${!ALLOWED_PROD[@]}" "${!ALLOWED_TEST[@]}") + +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 (no glob interpretation) + if [ "${import#"$allowed/"}" != "$import" ]; then + return 0 + fi + done + return 1 +} + # 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) || { @@ -44,21 +76,13 @@ if [ -z "$IMPORTS" ]; then exit 0 fi +# Check all direct dependencies are in the allowlist VIOLATIONS="" while IFS= read -r import; do [ -z "$import" ] && continue - # Check if import matches any allowed package (prefix match for subpackages) - MATCHED=false - for allowed in "${ALLOWED[@]}"; do - if [[ "$import" == "$allowed" ]] || [[ "$import" == "$allowed/"* ]]; then - MATCHED=true - break - fi - done - - if [ "$MATCHED" = false ]; then - VIOLATIONS="${VIOLATIONS} - ${import}"$'\n' + if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then + VIOLATIONS="${VIOLATIONS} - ${import} (not in allowlist)"$'\n' fi done <<< "$IMPORTS" @@ -68,17 +92,31 @@ if [ -n "$VIOLATIONS" ]; then 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 "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 +PROD_IMPORTS=$(go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ./... 2>/dev/null | grep -v '_test' || true) + +TEST_ONLY_IN_PROD="" +for test_pkg in "${!ALLOWED_TEST[@]}"; do + if echo "$PROD_IMPORTS" | grep -q "^${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 "" - echo "To add a dependency:" - echo " 1. Open a PR that ONLY updates CONVENTIONS.md" - echo " 2. Get explicit approval from Aaron" - echo " 3. After merge, use the package in a separate PR" + 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 deps: $(echo "$IMPORTS" | wc -l | tr -d ' ')" +echo " Production: ${#ALLOWED_PROD[@]}, Test-only: ${#ALLOWED_TEST[@]}" -- 2.47.3 From 01cde16d47fb11528793f47950421ccb70b54f39 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 14:02:06 -0700 Subject: [PATCH 4/4] fix: validate all deps and improve robustness Addresses GPT review feedback: 1. MAJOR - Test deps now validated: All direct module deps (from go.mod) are checked against the allowlist, whether used in prod or tests. 2. MINOR - Prefix match: Uses grep -E with word boundary (^pkg(/|$|$)) to avoid false positives on similarly-prefixed modules. 3. MINOR - Bash version check: Script now fails early with helpful message if Bash < 4 (macOS default). Added shebang: #!/usr/bin/env bash 4. NIT - Removed redundant grep -v '_test' (go list -deps already excludes test-only deps without -test flag). --- scripts/check-deps.sh | 45 ++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/scripts/check-deps.sh b/scripts/check-deps.sh index 1d2999c..f88efd2 100755 --- a/scripts/check-deps.sh +++ b/scripts/check-deps.sh @@ -1,12 +1,21 @@ -#!/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. -# Also enforces Scope column: "test only" packages cannot appear in non-test code. +# Enforces Scope column: "test only" packages cannot appear in non-test code. set -euo pipefail +# Check bash version +if ((BASH_VERSINFO[0] < 4)); then + echo "❌ Bash 4+ required (found ${BASH_VERSION})" + echo " On macOS: brew install bash" + exit 1 +fi + CONVENTIONS_FILE="${1:-CONVENTIONS.md}" if [ ! -f "$CONVENTIONS_FILE" ]; then @@ -16,13 +25,11 @@ fi # Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible) # Format: | `package` | use case | scope | -# Output: package:scope (e.g., "gopkg.in/yaml.v3:production") declare -A ALLOWED_PROD=() declare -A ALLOWED_TEST=() while IFS= read -r line; do # Use awk to extract package and scope from table row - # Split on | and extract backtick-wrapped package from column 1, scope from column 3 pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}') scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}') @@ -53,7 +60,7 @@ matches_allowlist() { if [ "$import" = "$allowed" ]; then return 0 fi - # Literal prefix match for subpackages (no glob interpretation) + # Literal prefix match for subpackages: must match "pkg/" exactly if [ "${import#"$allowed/"}" != "$import" ]; then return 0 fi @@ -61,22 +68,19 @@ matches_allowlist() { return 1 } -# 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" +# Get direct module dependencies from go.mod +DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>&1) || { + echo "❌ Failed to list dependencies: $DIRECT_IMPORTS" exit 1 } +DIRECT_IMPORTS=$(echo "$DIRECT_IMPORTS" | grep -v '^$' || true) -# Filter out the module itself (first line) and empty lines -IMPORTS=$(echo "$IMPORTS" | tail -n +2 | grep -v '^$' || true) - -if [ -z "$IMPORTS" ]; then +if [ -z "$DIRECT_IMPORTS" ]; then echo "✅ No external dependencies" exit 0 fi -# Check all direct dependencies are in the allowlist +# Check ALL direct dependencies are in some allowlist VIOLATIONS="" while IFS= read -r import; do [ -z "$import" ] && continue @@ -84,7 +88,7 @@ while IFS= read -r import; do if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then VIOLATIONS="${VIOLATIONS} - ${import} (not in allowlist)"$'\n' fi -done <<< "$IMPORTS" +done <<< "$DIRECT_IMPORTS" if [ -n "$VIOLATIONS" ]; then echo "❌ UNAPPROVED DEPENDENCIES DETECTED" @@ -97,12 +101,13 @@ if [ -n "$VIOLATIONS" ]; then fi # Enforce Scope: test-only packages must not appear in non-test code -# Get imports used by non-test code only -PROD_IMPORTS=$(go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ./... 2>/dev/null | grep -v '_test' || true) +# 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 - if echo "$PROD_IMPORTS" | grep -q "^${test_pkg}"; then + # Use word-boundary matching: exact match or followed by / + 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 @@ -118,5 +123,5 @@ if [ -n "$TEST_ONLY_IN_PROD" ]; then fi echo "✅ All dependencies are approved" -echo " Direct deps: $(echo "$IMPORTS" | wc -l | tr -d ' ')" -echo " Production: ${#ALLOWED_PROD[@]}, Test-only: ${#ALLOWED_TEST[@]}" +echo " Direct module deps: $(echo "$DIRECT_IMPORTS" | wc -l | tr -d ' ')" +echo " Production allowlist: ${#ALLOWED_PROD[@]}, Test-only allowlist: ${#ALLOWED_TEST[@]}" -- 2.47.3