fix: enforce Scope column and improve portability
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 14s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 35s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m2s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 14s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 35s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m2s
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.
This commit is contained in:
+68
-30
@@ -3,6 +3,7 @@
|
|||||||
# Exit 1 if any unapproved import is found.
|
# Exit 1 if any unapproved import is found.
|
||||||
#
|
#
|
||||||
# The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth.
|
# 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
|
set -euo pipefail
|
||||||
|
|
||||||
@@ -13,22 +14,53 @@ if [ ! -f "$CONVENTIONS_FILE" ]; then
|
|||||||
exit 1
|
exit 1
|
||||||
fi
|
fi
|
||||||
|
|
||||||
# Parse approved packages from CONVENTIONS.md table
|
# Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible)
|
||||||
# Looks for lines like: | `gopkg.in/yaml.v3` | ...
|
# Format: | `package` | use case | scope |
|
||||||
ALLOWED=()
|
# Output: package:scope (e.g., "gopkg.in/yaml.v3:production")
|
||||||
while IFS= read -r line; do
|
declare -A ALLOWED_PROD=()
|
||||||
# Extract package from markdown table cell: | `package` |
|
declare -A ALLOWED_TEST=()
|
||||||
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)
|
|
||||||
|
|
||||||
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 "⚠️ No approved packages found in $CONVENTIONS_FILE"
|
||||||
echo " (This is fine if you want stdlib-only)"
|
echo " (This is fine if you want stdlib-only)"
|
||||||
fi
|
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)
|
# Get DIRECT dependencies only (exclude indirect/transitive)
|
||||||
# Fail closed: if go list fails, we exit non-zero
|
# Fail closed: if go list fails, we exit non-zero
|
||||||
IMPORTS=$(go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all 2>&1) || {
|
IMPORTS=$(go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all 2>&1) || {
|
||||||
@@ -44,21 +76,13 @@ if [ -z "$IMPORTS" ]; then
|
|||||||
exit 0
|
exit 0
|
||||||
fi
|
fi
|
||||||
|
|
||||||
|
# Check all direct dependencies are in the allowlist
|
||||||
VIOLATIONS=""
|
VIOLATIONS=""
|
||||||
while IFS= read -r import; do
|
while IFS= read -r import; do
|
||||||
[ -z "$import" ] && continue
|
[ -z "$import" ] && continue
|
||||||
|
|
||||||
# Check if import matches any allowed package (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} - ${import}"$'\n'
|
|
||||||
fi
|
fi
|
||||||
done <<< "$IMPORTS"
|
done <<< "$IMPORTS"
|
||||||
|
|
||||||
@@ -68,17 +92,31 @@ if [ -n "$VIOLATIONS" ]; then
|
|||||||
echo "The following imports are not in the allowlist:"
|
echo "The following imports are not in the allowlist:"
|
||||||
printf "%s" "$VIOLATIONS"
|
printf "%s" "$VIOLATIONS"
|
||||||
echo ""
|
echo ""
|
||||||
echo "Approved packages (from CONVENTIONS.md):"
|
echo "To add a dependency, update CONVENTIONS.md (requires Aaron's approval)"
|
||||||
for pkg in "${ALLOWED[@]}"; do
|
exit 1
|
||||||
echo " - $pkg"
|
fi
|
||||||
done
|
|
||||||
|
# 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 ""
|
||||||
echo "To add a dependency:"
|
printf "%s" "$TEST_ONLY_IN_PROD"
|
||||||
echo " 1. Open a PR that ONLY updates CONVENTIONS.md"
|
echo ""
|
||||||
echo " 2. Get explicit approval from Aaron"
|
echo "These packages are marked 'test only' in CONVENTIONS.md"
|
||||||
echo " 3. After merge, use the package in a separate PR"
|
echo "and must only be imported from *_test.go files."
|
||||||
exit 1
|
exit 1
|
||||||
fi
|
fi
|
||||||
|
|
||||||
echo "✅ All dependencies are approved"
|
echo "✅ All dependencies are approved"
|
||||||
echo " Direct deps: $(echo "$IMPORTS" | wc -l | tr -d ' ')"
|
echo " Direct deps: $(echo "$IMPORTS" | wc -l | tr -d ' ')"
|
||||||
|
echo " Production: ${#ALLOWED_PROD[@]}, Test-only: ${#ALLOWED_TEST[@]}"
|
||||||
|
|||||||
Reference in New Issue
Block a user