Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Rodin
|
9e15b73a23 | ||
|
Rodin
|
006b7a3b27 |
+1
-19
@@ -2,26 +2,8 @@
|
||||
|
||||
## 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.**
|
||||
|
||||
Only *direct* dependencies (listed in go.mod without `// indirect`) are checked against this allowlist. Transitive dependencies pulled in by approved packages are implicitly allowed.
|
||||
|
||||
To request a new dependency:
|
||||
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
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
.PHONY: build test test-integration lint clean coverage check-deps precommit
|
||||
.PHONY: build test test-integration lint clean coverage
|
||||
|
||||
build:
|
||||
go build -o review-bot ./cmd/review-bot/
|
||||
@@ -12,15 +12,9 @@ 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
|
||||
|
||||
@@ -208,7 +208,7 @@ AI Core handles OAuth token management and deployment discovery automatically. M
|
||||
| `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
|
||||
| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
|
||||
| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
|
||||
| `persona-file` | No | `""` | Path to persona JSON file with custom review focus |
|
||||
| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
|
||||
| `temperature` | No | `0` | LLM temperature (0 = server default) |
|
||||
| `timeout` | No | `300` | LLM request timeout in seconds |
|
||||
| `dry-run` | No | `false` | Print review to stdout instead of posting |
|
||||
@@ -408,32 +408,38 @@ Each persona posts independently with its own sentinel, so reviews don't interfe
|
||||
|
||||
### Custom Personas
|
||||
|
||||
Create a JSON file with your domain-specific review focus:
|
||||
Create a YAML file with your domain-specific review focus:
|
||||
|
||||
```json
|
||||
// .review/personas/trading.json
|
||||
{
|
||||
"name": "trading",
|
||||
"display_name": "Trading Domain Expert",
|
||||
"identity": "You are a trading systems expert reviewing code for correctness.\n\nYour expertise:\n- Order lifecycle and state machines\n- Fill handling and partial fills\n- Position tracking and P&L calculations\n- Event sourcing invariants",
|
||||
"focus": [
|
||||
"Order state machine correctness",
|
||||
"Fill handling edge cases (partial, overfill)",
|
||||
"Position and P&L calculation accuracy",
|
||||
"Event replay determinism",
|
||||
"Decimal precision for money"
|
||||
],
|
||||
"ignore": [
|
||||
"Code style",
|
||||
"General performance",
|
||||
"Documentation formatting"
|
||||
],
|
||||
"severity": {
|
||||
"major": "Bugs that cause incorrect positions, fills, or money calculations",
|
||||
"minor": "Edge cases that could cause issues under unusual conditions",
|
||||
"nit": "Clarity improvements for domain logic"
|
||||
}
|
||||
}
|
||||
```yaml
|
||||
# .review/personas/trading.yaml
|
||||
name: trading
|
||||
display_name: Trading Domain Expert
|
||||
|
||||
identity: |
|
||||
You are a trading systems expert reviewing code for correctness.
|
||||
|
||||
Your expertise:
|
||||
- Order lifecycle and state machines
|
||||
- Fill handling and partial fills
|
||||
- Position tracking and P&L calculations
|
||||
- Event sourcing invariants
|
||||
|
||||
focus:
|
||||
- Order state machine correctness
|
||||
- Fill handling edge cases (partial, overfill)
|
||||
- Position and P&L calculation accuracy
|
||||
- Event replay determinism
|
||||
- Decimal precision for money
|
||||
|
||||
ignore:
|
||||
- Code style
|
||||
- General performance
|
||||
- Documentation formatting
|
||||
|
||||
severity:
|
||||
major: "Bugs that cause incorrect positions, fills, or money calculations"
|
||||
minor: "Edge cases that could cause issues under unusual conditions"
|
||||
nit: "Clarity improvements for domain logic"
|
||||
```
|
||||
|
||||
Use it in CI:
|
||||
@@ -442,17 +448,24 @@ Use it in CI:
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: trading
|
||||
persona-file: .review/personas/trading.json
|
||||
persona-file: .review/personas/trading.yaml
|
||||
...
|
||||
```
|
||||
|
||||
YAML is the recommended format for personas because it supports:
|
||||
- Multi-line strings with `|` blocks (cleaner identity definitions)
|
||||
- Comments for documentation
|
||||
- More readable arrays and nested structures
|
||||
|
||||
JSON is also supported for backwards compatibility—just use `.json` extension.
|
||||
|
||||
|
||||
### Persona vs system-prompt-file
|
||||
|
||||
| Feature | `persona` / `persona-file` | `system-prompt-file` |
|
||||
|---------|---------------------------|----------------------|
|
||||
| Replaces base prompt | Yes | No (appends) |
|
||||
| Structured format | Yes (JSON) | No (freeform) |
|
||||
| Structured format | Yes (YAML/JSON) | No (freeform) |
|
||||
| Focus/ignore lists | Yes | Manual |
|
||||
| Severity calibration | Yes | Manual |
|
||||
| Header display name | Yes | No |
|
||||
|
||||
@@ -1,133 +0,0 @@
|
||||
#!/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
|
||||
fi
|
||||
|
||||
# Parse approved packages from CONVENTIONS.md table
|
||||
# Note: uses Bash process substitution (< <(...)) for the loop
|
||||
# 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
|
||||
pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}')
|
||||
scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}')
|
||||
|
||||
# Accept packages starting with letter or digit (e.g., 9fans.net/go)
|
||||
if [ -n "$pkg" ] && [ "$pkg" != "Package" ] && [[ "$pkg" =~ ^[[:alnum:]] ]]; 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: must match "pkg/" exactly
|
||||
if [ "${import#"$allowed/"}" != "$import" ]; then
|
||||
return 0
|
||||
fi
|
||||
done
|
||||
return 1
|
||||
}
|
||||
|
||||
# Get direct module dependencies from go.mod
|
||||
# Capture stderr separately to avoid mixing error messages with package list
|
||||
GO_LIST_STDERR=$(mktemp)
|
||||
trap 'rm -f "$GO_LIST_STDERR"' EXIT
|
||||
DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>"$GO_LIST_STDERR") || {
|
||||
echo "❌ Failed to list dependencies:"
|
||||
cat "$GO_LIST_STDERR"
|
||||
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)"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Enforce Scope: test-only packages must not appear in non-test code
|
||||
# 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
|
||||
# Match exact package or subpackages (pkg or pkg/...)
|
||||
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[@]}"
|
||||
Reference in New Issue
Block a user