docs: allow approved third-party packages #59
@@ -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) |
|
||||
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`) |
|
||||
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.
|
||||
|
||||
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
|
||||
|
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.
|
||||
|
||||
## Error Handling
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
.PHONY: build test test-integration lint clean coverage
|
||||
.PHONY: build test test-integration lint clean coverage check-deps
|
||||
|
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/
|
||||
@@ -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,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=(
|
||||
|
[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.
|
||||
"gopkg.in/yaml.v3"
|
||||
"github.com/google/go-cmp"
|
||||
)
|
||||
|
||||
# Build regex pattern from allowed list
|
||||
ALLOWED_PATTERN=""
|
||||
|
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.
|
||||
for pkg in "${ALLOWED[@]}"; do
|
||||
|
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.
|
||||
if [ -z "$ALLOWED_PATTERN" ]; then
|
||||
ALLOWED_PATTERN="$pkg"
|
||||
else
|
||||
ALLOWED_PATTERN="$ALLOWED_PATTERN|$pkg"
|
||||
fi
|
||||
|
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.
|
||||
done
|
||||
|
||||
|
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.
|
||||
# Get all imports from go.mod (excluding the module itself and stdlib)
|
||||
|
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.
|
||||
IMPORTS=$(go list -m all 2>/dev/null | tail -n +2 | awk '{print $1}' || true)
|
||||
|
[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.
|
||||
|
||||
if [ -z "$IMPORTS" ]; then
|
||||
|
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.
|
||||
echo "✅ No external dependencies"
|
||||
|
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.
|
||||
exit 0
|
||||
fi
|
||||
|
||||
VIOLATIONS=""
|
||||
while IFS= read -r import; do
|
||||
|
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.
|
||||
# Skip empty lines
|
||||
[ -z "$import" ] && continue
|
||||
|
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.
|
||||
|
||||
|
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.
|
||||
# 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
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all` lists module-level direct dependencies, not package-level imports. This means a module in go.mod that isn't actually imported in any Go source file will still be flagged. More precisely, it will miss the case where someone adds a `go.mod` entry for an unapproved module but the script will catch modules not in go.mod at all (since they wouldn't be listed). This is probably fine for the intended purpose, but the comment 'DIRECT dependencies only' may mislead — it means direct in the go.mod sense, not 'actually used in source'. Consider a brief clarifying comment.
|
||||
if [ "$MATCHED" = false ]; then
|
||||
VIOLATIONS="$VIOLATIONS\n - $import"
|
||||
fi
|
||||
done <<< "$IMPORTS"
|
||||
|
||||
if [ -n "$VIOLATIONS" ]; then
|
||||
echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
|
||||
|
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"`.
|
||||
echo -e "The following imports are not in the allowlist:$VIOLATIONS"
|
||||
echo ""
|
||||
echo "To add a dependency:"
|
||||
|
[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.
|
||||
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"
|
||||
|
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.
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "✅ All dependencies are approved"
|
||||
[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.