feat(code): add /code:review for automated code review (#107)
Add code review command that: - Reviews staged changes, commit ranges, or GitHub PRs - Checks for debug statements, secrets, error handling - Validates test coverage and documentation updates - Supports --security flag for intensive security scanning - Outputs formatted review with actionable suggestions Migrated from core-claude PR #51. Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
08a57ce60c
commit
5f136dea2a
2 changed files with 216 additions and 0 deletions
29
claude/code/commands/review.md
Normal file
29
claude/code/commands/review.md
Normal file
|
|
@ -0,0 +1,29 @@
|
||||||
|
---
|
||||||
|
name: review
|
||||||
|
description: Perform a code review on staged changes, a commit range, or a GitHub PR
|
||||||
|
args: <range> [--security]
|
||||||
|
---
|
||||||
|
|
||||||
|
# Code Review
|
||||||
|
|
||||||
|
Performs a code review on the specified changes.
|
||||||
|
|
||||||
|
## Usage
|
||||||
|
|
||||||
|
Review staged changes:
|
||||||
|
`/code:review`
|
||||||
|
|
||||||
|
Review a commit range:
|
||||||
|
`/code:review HEAD~3..HEAD`
|
||||||
|
|
||||||
|
Review a GitHub PR:
|
||||||
|
`/code:review #123`
|
||||||
|
|
||||||
|
Perform a security-focused review:
|
||||||
|
`/code:review --security`
|
||||||
|
|
||||||
|
## Action
|
||||||
|
|
||||||
|
```bash
|
||||||
|
"${CLAUDE_PLUGIN_ROOT}/scripts/code-review.sh" "$@"
|
||||||
|
```
|
||||||
187
claude/code/scripts/code-review.sh
Executable file
187
claude/code/scripts/code-review.sh
Executable file
|
|
@ -0,0 +1,187 @@
|
||||||
|
#!/bin/bash
|
||||||
|
# Core code review script
|
||||||
|
|
||||||
|
# --- Result Variables ---
|
||||||
|
conventions_result=""
|
||||||
|
debug_result=""
|
||||||
|
test_coverage_result=""
|
||||||
|
secrets_result=""
|
||||||
|
error_handling_result=""
|
||||||
|
docs_result=""
|
||||||
|
intensive_security_result=""
|
||||||
|
suggestions=()
|
||||||
|
|
||||||
|
# --- Check Functions ---
|
||||||
|
|
||||||
|
check_conventions() {
|
||||||
|
# Placeholder for project convention checks (e.g., linting)
|
||||||
|
conventions_result="✓ Conventions: UK English, strict types (Placeholder)"
|
||||||
|
}
|
||||||
|
|
||||||
|
check_debug() {
|
||||||
|
local diff_content=$1
|
||||||
|
if echo "$diff_content" | grep -q -E 'console\.log|print_r|var_dump'; then
|
||||||
|
debug_result="⚠ No debug statements: Found debug statements."
|
||||||
|
suggestions+=("Remove debug statements before merging.")
|
||||||
|
else
|
||||||
|
debug_result="✓ No debug statements"
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
check_test_coverage() {
|
||||||
|
local diff_content=$1
|
||||||
|
# This is a simple heuristic and not a replacement for a full test coverage suite.
|
||||||
|
# It checks if any new files are tests, or if test files were modified.
|
||||||
|
if echo "$diff_content" | grep -q -E '\+\+\+ b/(tests?|specs?)/'; then
|
||||||
|
test_coverage_result="✓ Test files modified: Yes"
|
||||||
|
else
|
||||||
|
test_coverage_result="⚠ Test files modified: No"
|
||||||
|
suggestions+=("Consider adding tests for new functionality.")
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
check_secrets() {
|
||||||
|
local diff_content=$1
|
||||||
|
if echo "$diff_content" | grep -q -i -E 'secret|password|api_key|token'; then
|
||||||
|
secrets_result="⚠ No secrets detected: Potential hardcoded secrets found."
|
||||||
|
suggestions+=("Review potential hardcoded secrets for security.")
|
||||||
|
else
|
||||||
|
secrets_result="✓ No secrets detected"
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
intensive_security_check() {
|
||||||
|
local diff_content=$1
|
||||||
|
if echo "$diff_content" | grep -q -E 'eval|dangerouslySetInnerHTML'; then
|
||||||
|
intensive_security_result="⚠ Intensive security scan: Unsafe functions may be present."
|
||||||
|
suggestions+=("Thoroughly audit the use of unsafe functions.")
|
||||||
|
else
|
||||||
|
intensive_security_result="✓ Intensive security scan: No obvious unsafe functions found."
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
check_error_handling() {
|
||||||
|
local diff_content=$1
|
||||||
|
# Files with new functions/methods but no error handling
|
||||||
|
local suspicious_files=$(echo "$diff_content" | grep -E '^\+\+\+ b/' | sed 's/^\+\+\+ b\///' | while read -r file; do
|
||||||
|
# Heuristic: if a file has added lines with 'function' or '=>' but no 'try'/'catch', it's suspicious.
|
||||||
|
added_logic=$(echo "$diff_content" | grep -E "^\+.*(function|\=>)" | grep "$file")
|
||||||
|
added_error_handling=$(echo "$diff_content" | grep -E "^\+.*(try|catch|throw)" | grep "$file")
|
||||||
|
|
||||||
|
if [ -n "$added_logic" ] && [ -z "$added_error_handling" ]; then
|
||||||
|
line_number=$(echo "$diff_content" | grep -nE "^\+.*(function|\=>)" | grep "$file" | cut -d: -f1 | head -n 1)
|
||||||
|
echo "$file:$line_number"
|
||||||
|
fi
|
||||||
|
done)
|
||||||
|
|
||||||
|
if [ -n "$suspicious_files" ]; then
|
||||||
|
error_handling_result="⚠ Missing error handling"
|
||||||
|
for file_line in $suspicious_files; do
|
||||||
|
suggestions+=("Consider adding error handling in $file_line.")
|
||||||
|
done
|
||||||
|
else
|
||||||
|
error_handling_result="✓ Error handling present"
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
check_docs() {
|
||||||
|
local diff_content=$1
|
||||||
|
if echo "$diff_content" | grep -q -E '\+\+\+ b/(README.md|docs?)/'; then
|
||||||
|
docs_result="✓ Documentation updated"
|
||||||
|
else
|
||||||
|
docs_result="⚠ Documentation updated: No changes to documentation files detected."
|
||||||
|
suggestions+=("Update documentation if the changes affect public APIs or user behavior.")
|
||||||
|
fi
|
||||||
|
}
|
||||||
|
|
||||||
|
# --- Output Function ---
|
||||||
|
|
||||||
|
print_results() {
|
||||||
|
local title="Code Review"
|
||||||
|
if [ -n "$range_arg" ]; then
|
||||||
|
title="$title: $range_arg"
|
||||||
|
else
|
||||||
|
local branch_name=$(git rev-parse --abbrev-ref HEAD 2>/dev/null)
|
||||||
|
if [ -n "$branch_name" ]; then
|
||||||
|
title="$title: $branch_name branch"
|
||||||
|
else
|
||||||
|
title="$title: Staged changes"
|
||||||
|
fi
|
||||||
|
fi
|
||||||
|
|
||||||
|
echo "$title"
|
||||||
|
echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"
|
||||||
|
echo ""
|
||||||
|
|
||||||
|
# Print checklist
|
||||||
|
echo "$conventions_result"
|
||||||
|
echo "$debug_result"
|
||||||
|
echo "$test_coverage_result"
|
||||||
|
echo "$secrets_result"
|
||||||
|
echo "$error_handling_result"
|
||||||
|
echo "$docs_result"
|
||||||
|
if [ -n "$intensive_security_result" ]; then
|
||||||
|
echo "$intensive_security_result"
|
||||||
|
fi
|
||||||
|
echo ""
|
||||||
|
|
||||||
|
# Print suggestions if any
|
||||||
|
if [ ${#suggestions[@]} -gt 0 ]; then
|
||||||
|
echo "Suggestions:"
|
||||||
|
for i in "${!suggestions[@]}"; do
|
||||||
|
echo "$((i+1)). ${suggestions[$i]}"
|
||||||
|
done
|
||||||
|
echo ""
|
||||||
|
fi
|
||||||
|
|
||||||
|
echo "Overall: Approve with suggestions"
|
||||||
|
}
|
||||||
|
|
||||||
|
# --- Main Logic ---
|
||||||
|
security_mode=false
|
||||||
|
range_arg=""
|
||||||
|
|
||||||
|
for arg in "$@"; do
|
||||||
|
case $arg in
|
||||||
|
--security)
|
||||||
|
security_mode=true
|
||||||
|
;;
|
||||||
|
*)
|
||||||
|
if [ -n "$range_arg" ]; then echo "Error: Multiple range arguments." >&2; exit 1; fi
|
||||||
|
range_arg="$arg"
|
||||||
|
;;
|
||||||
|
esac
|
||||||
|
done
|
||||||
|
|
||||||
|
diff_output=""
|
||||||
|
if [ -z "$range_arg" ]; then
|
||||||
|
diff_output=$(git diff --staged)
|
||||||
|
if [ $? -ne 0 ]; then echo "Error: git diff --staged failed." >&2; exit 1; fi
|
||||||
|
if [ -z "$diff_output" ]; then echo "No staged changes to review."; exit 0; fi
|
||||||
|
elif [[ "$range_arg" == \#* ]]; then
|
||||||
|
pr_number="${range_arg#?}"
|
||||||
|
if ! command -v gh &> /dev/null; then echo "Error: 'gh' not found." >&2; exit 1; fi
|
||||||
|
diff_output=$(gh pr diff "$pr_number")
|
||||||
|
if [ $? -ne 0 ]; then echo "Error: gh pr diff failed. Is the PR number valid?" >&2; exit 1; fi
|
||||||
|
elif [[ "$range_arg" == *..* ]]; then
|
||||||
|
diff_output=$(git diff "$range_arg")
|
||||||
|
if [ $? -ne 0 ]; then echo "Error: git diff failed. Is the commit range valid?" >&2; exit 1; fi
|
||||||
|
else
|
||||||
|
echo "Unsupported argument: $range_arg" >&2
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
|
# Run checks
|
||||||
|
check_conventions
|
||||||
|
check_debug "$diff_output"
|
||||||
|
check_test_coverage "$diff_output"
|
||||||
|
check_error_handling "$diff_output"
|
||||||
|
check_docs "$diff_output"
|
||||||
|
check_secrets "$diff_output"
|
||||||
|
|
||||||
|
if [ "$security_mode" = true ]; then
|
||||||
|
intensive_security_check "$diff_output"
|
||||||
|
fi
|
||||||
|
|
||||||
|
# Print the final formatted report
|
||||||
|
print_results
|
||||||
Loading…
Add table
Reference in a new issue