docs: allow approved third-party packages #59
@@ -2,8 +2,26 @@
|
||||
|
||||
## 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 | 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
|
||||
sonnet-review-bot
commented
[NIT] The sentence 'Transitive dependencies of approved packages are automatically allowed' is a policy statement that the check-deps.sh script does NOT currently verify (it only checks direct deps via **[NIT]** The sentence 'Transitive dependencies of approved packages are automatically allowed' is a policy statement that the check-deps.sh script does NOT currently verify (it only checks direct deps via `go list -m`). This is correct behavior—you generally don't want to enumerate all transitive deps—but the wording could be clearer: something like 'Transitive dependencies pulled in by approved packages do not need to be listed here' to make it clear this is intentional, not an oversight.
|
||||
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
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
.PHONY: build test test-integration lint clean coverage
|
||||
.PHONY: build test test-integration lint clean coverage check-deps precommit
|
||||
|
||||
build:
|
||||
go build -o review-bot ./cmd/review-bot/
|
||||
@@ -12,9 +12,15 @@ test-integration:
|
||||
lint:
|
||||
go vet ./...
|
||||
|
||||
check-deps:
|
||||
gpt-review-bot
commented
[NIT] The check-deps script requires Bash 4+. On macOS, default /bin/bash is 3.x; while the script emits guidance, you might consider documenting this requirement alongside the new precommit target for developer setup clarity. **[NIT]** The check-deps script requires Bash 4+. On macOS, default /bin/bash is 3.x; while the script emits guidance, you might consider documenting this requirement alongside the new precommit target for developer setup clarity.
|
||||
@./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
|
||||
[MINOR] The new **[MINOR]** The new `precommit` target runs the dependency check, but unless CI invokes this target, policy enforcement may not occur in CI. Ensure CI runs `make precommit` (or otherwise invokes `check-deps`) to prevent unapproved dependencies from being merged.
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
#!/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
|
||||
|
||||
# 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
|
||||
echo "❌ CONVENTIONS.md not found"
|
||||
exit 1
|
||||
|
gpt-review-bot
commented
[MINOR] Uses 'grep -P' (Perl regex), which is not available on macOS/BSD grep by default. This reduces cross-platform developer usability for the precommit hook. **[MINOR]** Uses 'grep -P' (Perl regex), which is not available on macOS/BSD grep by default. This reduces cross-platform developer usability for the precommit hook.
|
||||
fi
|
||||
|
||||
# Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible)
|
||||
# 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
|
||||
|
gpt-review-bot
commented
[NIT] Parsing the markdown table with **[NIT]** Parsing the markdown table with `grep`/`awk` is somewhat brittle (e.g., relies on backticks around package names and exact column positions). This is acceptable given the documented process, but a brief note in CONVENTIONS.md to preserve formatting would help avoid accidental breakage.
|
||||
pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}')
|
||||
scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}')
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] The filter **[MINOR]** The filter `[[ "$pkg" =~ ^[a-zA-Z] ]]` rejects valid import paths that begin with a digit (e.g., 9fans.net/go). Consider relaxing to `^[[:alnum:]]` or removing the check, since the header row is already excluded by the grep.
|
||||
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
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] The comment says 'POSIX-compatible' for the awk parsing but the outer loop uses a Bash process substitution **[NIT]** The comment says 'POSIX-compatible' for the awk parsing but the outer loop uses a Bash process substitution `< <(...)` which is Bash-specific. The comment is mildly misleading — it means awk itself is POSIX, not the overall approach.
|
||||
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
|
||||
|
sonnet-review-bot
commented
[MINOR] If **[MINOR]** If `go list` returns non-module output (e.g. build errors) the error is captured in `DIRECT_IMPORTS` and the early-exit error message will contain the raw go toolchain output. This works adequately but the error message could be confusing. Minor quality-of-life issue.
|
||||
DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>&1) || {
|
||||
echo "❌ Failed to list dependencies: $DIRECT_IMPORTS"
|
||||
|
sonnet-review-bot
commented
[MINOR] The script checks **[MINOR]** The script checks `go.mod` direct dependencies for allowlist compliance, but the scope enforcement (lines 97-111) checks the full production import graph via `go list -deps`. These two checks are at different granularities and could diverge. For example, a direct dependency might be test-only in practice but `go list -deps ./...` (which doesn't filter test files) would still traverse it. Using `go list -deps -test=false ./...` more explicitly conveys intent, though the current flag-less form already excludes test builds.
|
||||
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
|
||||
[ -z "$import" ] && continue
|
||||
|
||||
if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then
|
||||
VIOLATIONS="${VIOLATIONS} - ${import} (not in allowlist)"$'\n'
|
||||
fi
|
||||
done <<< "$DIRECT_IMPORTS"
|
||||
|
||||
if [ -n "$VIOLATIONS" ]; then
|
||||
echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
|
||||
echo ""
|
||||
echo "The following imports are not in the allowlist:"
|
||||
printf "%s" "$VIOLATIONS"
|
||||
echo ""
|
||||
echo "To add a dependency, update CONVENTIONS.md (requires Aaron's approval)"
|
||||
|
sonnet-review-bot
commented
[MINOR] The test-scope enforcement uses **[MINOR]** The test-scope enforcement uses `go list -deps -f '...' ./... | grep -v '_test'` to identify production imports, but this approach is fragile: it filters by path containing '_test', which catches test packages but the `-deps` flag on non-test packages already excludes test files. More importantly, this will fail to detect a test-only package imported via a non-test file in a sub-package whose path happens not to contain '_test'. Using `go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' $(go list ./...)` (without `-deps`) and then checking against the allowlist directly would be more precise. That said, the current approach is a reasonable heuristic.
gpt-review-bot
commented
[NIT] The **[NIT]** The `grep -v '_test'` in the production imports step is unnecessary because `go list -deps` without `-test` already excludes test-only dependencies. While harmless, it can be removed for clarity.
|
||||
exit 1
|
||||
fi
|
||||
|
[MINOR] Production imports discovery allows go list to fail open (2>/dev/null | ... || true). If go list fails, PROD_IMPORTS becomes empty and the script won’t detect misuse of test-only dependencies, weakening enforcement. Failing closed here would improve robustness. **[MINOR]** Production imports discovery allows go list to fail open (2>/dev/null | ... || true). If go list fails, PROD_IMPORTS becomes empty and the script won’t detect misuse of test-only dependencies, weakening enforcement. Failing closed here would improve robustness.
|
||||
|
||||
# Enforce Scope: test-only packages must not appear in non-test code
|
||||
|
gpt-review-bot
commented
[MINOR] The scope enforcement uses **[MINOR]** The scope enforcement uses `grep -q "^${test_pkg}"` which can produce false positives for modules with a similar prefix (e.g., github.com/google/go-cmppro would match github.com/google/go-cmp). Use a delimiter-aware pattern like `^${test_pkg}(/|$)` to avoid accidental matches.
|
||||
# Get imports used by non-test code only (go list -deps without -test excludes test deps)
|
||||
|
sonnet-review-bot
commented
[MINOR] The regex **[MINOR]** The regex `"^${test_pkg}(/|\$|$)"` has a redundant `$` — `\$` escapes the literal dollar sign for the shell but in the regex context inside `grep -qE`, the trailing `|$)` is a proper end-of-line anchor followed by the closing group, making `\$` (literal `$` character) unreachable in practice. This is harmless but the intent (match exact package or subpackage) is already handled by the `^pkg($|/)` pattern. Consider simplifying to `"^${test_pkg}(/|$)"`.
|
||||
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
|
||||
|
sonnet-review-bot
commented
[MINOR] The test-only production check uses **[MINOR]** The test-only production check uses `grep -q "^${test_pkg}"` without anchoring the end of the pattern. A package path like `github.com/google/go-cmp` would inadvertently match a hypothetical `github.com/google/go-cmp-extended`. Should be `grep -qE "^${test_pkg}(/|$)"` to match only exact or subpackage imports.
|
||||
# Use word-boundary matching: exact match or followed by /
|
||||
|
gpt-review-bot
commented
[MINOR] The grep regex **[MINOR]** The grep regex `^${test_pkg}(/|\$|$)` redundantly includes both `\$` and `$`. It can be simplified to `^${test_pkg}(/|$)` without changing semantics.
|
||||
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[@]}"
|
||||
[NIT] The table lists
github.com/google/go-cmpwith scope 'test only', but the enforcement script (check-deps.sh) does not differentiate between production and test-only scope — it checks all direct go.mod dependencies uniformly. Ifgo-cmpends up in go.mod as a direct dependency (which it will when used in_test.gofiles), it passes the allowlist check regardless of where it's imported. The 'test only' scope annotation is purely documentation with no mechanical enforcement. This is acceptable but worth noting so future maintainers don't assume it's enforced.[NIT] The note 'Transitive dependencies of approved packages are automatically allowed' is a policy statement, but the enforcement script only checks direct module dependencies (via
go list -m ... allwith.Indirectfiltered out). This is correct and intentional, but it's worth confirming the wording matches: transitive deps won't appear as violations, which aligns with the statement.