docs: allow approved third-party packages #59
@@ -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 |
|
||||
sonnet-review-bot
commented
[NIT] **[NIT]** `go-cmp` is a test-only dependency, but the table doesn't distinguish test dependencies from production dependencies. A 'Scope' column (e.g. 'test only' vs 'production') would clarify that `go-cmp` should never appear in non-test code, preventing accidental production use.
|
||||
| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
|
||||
gpt-review-bot
commented
[MINOR] The dependency request process omits updating the enforcement mechanism. If the allowlist remains hard-coded in scripts/check-deps.sh, instructions should explicitly require updating the script alongside the table, or the script should be changed to derive its allowlist from a single canonical source. **[MINOR]** The dependency request process omits updating the enforcement mechanism. If the allowlist remains hard-coded in scripts/check-deps.sh, instructions should explicitly require updating the script alongside the table, or the script should be changed to derive its allowlist from a single canonical source.
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] The table lists **[NIT]** The table lists `github.com/google/go-cmp` with 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. If `go-cmp` ends up in go.mod as a direct dependency (which it will when used in `_test.go` files), 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.
sonnet-review-bot
commented
[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 **[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 ... all` with `.Indirect` filtered 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.
|
||||
**Any import not in this table or the Go standard library is forbidden.**
|
||||
|
sonnet-review-bot
commented
[NIT] The process for adding new dependencies doesn't mention who has approval authority (e.g. maintainers, a specific review process). Adding a brief note like 'requires approval from a maintainer' would make the governance expectation explicit. **[NIT]** The process for adding new dependencies doesn't mention who has approval authority (e.g. maintainers, a specific review process). Adding a brief note like 'requires approval from a maintainer' would make the governance expectation explicit.
|
||||
|
||||
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
|
||||
|
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
|
||||
|
||||
- Return errors; never panic.
|
||||
|
||||
@@ -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
|
||||
|
gpt-review-bot
commented
[MINOR] The new 'precommit' target isn't listed in .PHONY. It should be added to avoid make treating a file named 'precommit' as up-to-date and skipping the target. **[MINOR]** The new 'precommit' target isn't listed in .PHONY. It should be added to avoid make treating a file named 'precommit' as up-to-date and skipping the target.
|
||||
|
||||
build:
|
||||
go build -o review-bot ./cmd/review-bot/
|
||||
|
||||
@@ -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
|
||||
|
||||
[MINOR] The allowlist is duplicated between CONVENTIONS.md and this script ( **[MINOR]** The allowlist is duplicated between CONVENTIONS.md and this script (`ALLOWED` array), creating a risk of drift. A mismatch could either block approved deps or unintentionally allow unapproved ones, weakening policy enforcement. Consider deriving the list from a single source (e.g., parse CONVENTIONS.md or define the list in a single machine-readable file).
gpt-review-bot
commented
[MAJOR] The allowlist is hard-coded in the script (ALLOWED array) while CONVENTIONS.md is presented as the source of truth. The documented process says to open a PR that ONLY updates the table, but the enforcement will still fail later unless this script is updated too. This mismatch will cause policy drift and block subsequent PRs using newly approved dependencies. **[MAJOR]** The allowlist is hard-coded in the script (ALLOWED array) while CONVENTIONS.md is presented as the source of truth. The documented process says to open a PR that ONLY updates the table, but the enforcement will still fail later unless this script is updated too. This mismatch will cause policy drift and block subsequent PRs using newly approved dependencies.
|
||||
# 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
|
||||
|
sonnet-review-bot
commented
[MINOR] The ALLOWED_PATTERN variable is built but never used. The actual checking is done in the loop with direct string comparison against the ALLOWED array. The dead code is harmless but confusing — either use the pattern (e.g., with grep) or remove it. **[MINOR]** The ALLOWED_PATTERN variable is built but never used. The actual checking is done in the loop with direct string comparison against the ALLOWED array. The dead code is harmless but confusing — either use the pattern (e.g., with grep) or remove it.
|
||||
|
||||
|
gpt-review-bot
commented
[NIT] ALLOWED_PATTERN is built but never used. Remove this dead code to reduce confusion and keep the script minimal. **[NIT]** ALLOWED_PATTERN is built but never used. Remove this dead code to reduce confusion and keep the script minimal.
|
||||
# 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` |
|
||||
|
gpt-review-bot
commented
[MAJOR] The script does not enforce the 'Scope' column from CONVENTIONS.md (e.g., allowing github.com/google/go-cmp only in tests). As written, any listed package is allowed in production code, contradicting the documented policy. **[MAJOR]** The script does not enforce the 'Scope' column from CONVENTIONS.md (e.g., allowing github.com/google/go-cmp only in tests). As written, any listed package is allowed in production code, contradicting the documented policy.
gpt-review-bot
commented
[MINOR] The script relies on bash associative arrays ( **[MINOR]** The script relies on bash associative arrays (`declare -A`), which require Bash 4+. On macOS (default Bash 3.2), this will fail locally for contributors running `make precommit`. Consider documenting the requirement, invoking a known shell (e.g., via `env bash` with ensured version), or rewriting to avoid associative arrays.
|
||||
pkg=$(echo "$line" | grep -oP '\| `\K[^`]+' | head -1 || true)
|
||||
if [ -n "$pkg" ] && [[ "$pkg" != "Package" ]]; then
|
||||
|
sonnet-review-bot
commented
[NIT] The comment says 'POSIX-compatible' but **[NIT]** The comment says 'POSIX-compatible' but `declare -A` (associative arrays) is a bash 4+ feature and is not POSIX. The comment should say 'bash 4+' to accurately describe the requirement. macOS ships bash 3.2 by default (GPLv2 licensing constraint), so this script would fail on stock macOS without Homebrew bash. Worth documenting the bash version requirement in a shebang comment or README.
|
||||
ALLOWED+=("$pkg")
|
||||
|
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
|
||||
|
[MINOR] The script fails open if **[MINOR]** The script fails open if `go list -m all` errors (`|| true`), then treats an empty IMPORTS as "no external dependencies" and exits 0. This can bypass enforcement if the Go toolchain is unavailable or the command fails. Prefer failing closed on errors or explicitly erroring when `go list` fails.
|
||||
done
|
||||
done < <(grep -E '^\| `[a-zA-Z]' "$CONVENTIONS_FILE" || true)
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] Table parsing relies on a brittle grep pattern ('^| `[a-zA-Z]') and a single-code-cell extraction. Future formatting changes (leading spaces, additional code spans) or package names starting with non-letters may cause false negatives/positives. **[MINOR]** Table parsing relies on a brittle grep pattern ('^\| `[a-zA-Z]') and a single-code-cell extraction. Future formatting changes (leading spaces, additional code spans) or package names starting with non-letters may cause false negatives/positives.
|
||||
# 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
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `go list -m all` includes indirect/transitive dependencies. If yaml.v3 or go-cmp pull in their own transitive deps, those would appear here and trigger false-positive violations. Consider using `go mod edit -json | jq '.Require[] | select(.Indirect == false) | .Path'` or filtering with `go list -m -mod=mod -f '{{if not .Indirect}}{{.Path}}{{end}}' all` to check only direct dependencies.
|
||||
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)
|
||||
|
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.
|
||||
# Fail closed: if go list fails, we exit non-zero
|
||||
IMPORTS=$(go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all 2>&1) || {
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `-oP` flag for PCRE-style regex (`\K`) is a GNU grep extension and is not available on macOS (BSD grep). This will silently fail to parse the allowlist on macOS developer machines (the `|| true` swallows the error), leaving ALLOWED empty and triggering the warning but not enforcing anything. Consider using `sed` instead: `echo "$line" | sed -n "s/^| \`\([^\`]*\)\`.*/\1/p"`, or document that GNU grep is required.
|
||||
echo "❌ Failed to list dependencies: $IMPORTS"
|
||||
|
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.
|
||||
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)
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `echo -e` is not POSIX-compliant and behavior varies across shells (e.g., on some systems /bin/bash may not support -e with printf semantics). The `\n` in the violations string also won't render on all platforms. Consider using `printf` instead: `printf 'The following imports are not in the allowlist:%b\n' "$VIOLATIONS"`.
|
||||
MATCHED=false
|
||||
for allowed in "${ALLOWED[@]}"; do
|
||||
if [[ "$import" == "$allowed" ]] || [[ "$import" == "$allowed/"* ]]; then
|
||||
|
[MINOR] The prefix match uses , which treats the right side as a glob pattern. While allowed entries are controlled via review, a metacharacter (e.g., *, ?, [) in an approved package could unintentionally broaden matches. Consider a literal prefix check to avoid glob semantics. **[MINOR]** The prefix match uses [[ "$import" == "$allowed/"* ]], which treats the right side as a glob pattern. While allowed entries are controlled via review, a metacharacter (e.g., *, ?, [) in an approved package could unintentionally broaden matches. Consider a literal prefix check to avoid glob semantics.
|
||||
@@ -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
|
||||
|
gpt-review-bot
commented
[MAJOR] The script checks only direct non-test modules via **[MAJOR]** The script checks only direct non-test modules via `go list -m ... all` and never verifies test-only dependencies against the allowlist. As a result, importing any new third-party package in tests (e.g., in *_test.go) will not be flagged unless also used in production code. This violates the documented 'STRICT ALLOWLIST' policy for tests.
|
||||
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:"
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `go list -m` output includes the current module itself on the first line, and the comment says 'Filter out the module itself (first line)'. However, `tail -n +2` strips the first line unconditionally. If `go list -m ... all` ever reorders output or includes a blank first line (e.g., when run in a workspace), this assumption breaks silently. A more robust approach would be to filter by comparing against `$(go list -m)` (the current module path) rather than relying on line position.
|
||||
printf "%s" "$VIOLATIONS"
|
||||
echo ""
|
||||
echo "Approved packages (from CONVENTIONS.md):"
|
||||
|
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.
|
||||
for pkg in "${ALLOWED[@]}"; do
|
||||
echo " - $pkg"
|
||||
|
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.
|
||||
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 ' ')"
|
||||
|
sonnet-review-bot
commented
[NIT] The final line counts direct deps via **[NIT]** The final line counts direct deps via `wc -l` on `$IMPORTS`, but `$IMPORTS` could contain blank lines depending on the `go list` output format when some template expansions produce empty strings. The `grep -v '^$'` earlier helps but the count may still be off by one if there's a trailing newline. Minor cosmetic issue.
|
||||
|
||||
[MINOR] Policy text says 'Only packages listed below may be imported' but the enforcement checks modules via 'go list -m all' and will also block any transitive modules not on the list. Consider clarifying that approving a dependency may entail approving its transitive modules, or adjust the script to allow transitive deps of approved modules automatically.