diff --git a/AGENTS.md b/AGENTS.md index b6f9f9258..cf5aa926f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,6 +43,7 @@ make update make verify # Runs verify-yaml and verify-update make verify-yaml # Validates YAML manifests make verify-update # Ensures generated files are up-to-date +make review-changes # Code review for the test files ``` ## Architecture diff --git a/Makefile b/Makefile index cb3c055f5..e54031edb 100644 --- a/Makefile +++ b/Makefile @@ -45,6 +45,10 @@ verify-update: update git diff --exit-code HEAD .PHONY: verify-update +review-changes: + hack/review.sh +.PHONY: review-changes + clean: rm -rf _output/ .PHONY: clean diff --git a/hack/review.sh b/hack/review.sh new file mode 100755 index 000000000..1541a6c3e --- /dev/null +++ b/hack/review.sh @@ -0,0 +1,217 @@ +#!/usr/bin/env bash +# +# Helper script to run code review using Claude Code's Task tool +# +# Usage: +# ./test/review.sh # Review current branch vs origin/main +# ./test/review.sh --pr 1234 # Review specific PR +# ./test/review.sh --range HEAD~3 # Review last 3 commits +# ./test/review.sh --files file1.go file2.go # Review specific files + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" + +# Default values +DIFF_RANGE="origin/main...HEAD" +PR_NUMBER="" +PR_REMOTE="${PR_REMOTE:-origin}" # Can be overridden via env var (e.g., PR_REMOTE=upstream) +FILES=() +OUTPUT_FILE="${SCRIPT_DIR}/review-output.json" + +function usage() { + cat < /dev/null; then + # Use gh CLI (preferred - shows PR metadata and handles authentication) + echo "Using 'gh' CLI to fetch PR..." + DIFF=$(gh pr diff "$PR_NUMBER" 2>&1) || { + echo "Error: Failed to fetch PR #${PR_NUMBER} using 'gh' CLI" + echo "Output: $DIFF" + exit 1 + } + FILES_CHANGED=$(gh pr view "$PR_NUMBER" --json files -q '.files[].path' 2>&1) || { + echo "Error: Failed to get PR file list" + exit 1 + } + else + # Fallback to git fetch (no gh CLI required) + # Note: This requires 'origin' to be the upstream GitHub repo, not a fork + echo "Note: 'gh' CLI not found. Using git fetch as fallback." + echo "Install 'gh' for better PR integration: https://cli.github.com/" + echo "" + + PR_REF="pull/${PR_NUMBER}/head" + echo "Fetching ${PR_REF} from remote '${PR_REMOTE}'..." + + if ! git fetch "$PR_REMOTE" "$PR_REF" 2>/dev/null; then + echo "" + echo "Error: Could not fetch PR #${PR_NUMBER} from remote '${PR_REMOTE}'" + echo "" + echo "This can happen if:" + echo " - Your 'origin' remote is a fork (not the upstream repo)" + echo " - The PR doesn't exist or is closed" + echo " - You don't have network access to the repository" + echo "" + echo "Possible solutions:" + echo " 1. Install 'gh' CLI (recommended): https://cli.github.com/manual/installation" + echo " 2. Add upstream remote: git remote add upstream https://github.com/openshift/cluster-version-operator.git" + echo " Then use: PR_REMOTE=upstream ./test/review.sh --pr ${PR_NUMBER}" + echo " 3. Manually fetch: git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}" + echo " 4. Use git range instead: ./test/review.sh --range origin/main...HEAD" + exit 1 + fi + + # Get the base branch (usually main) + BASE_BRANCH=$(git remote show "$PR_REMOTE" 2>/dev/null | grep 'HEAD branch' | cut -d' ' -f5) + BASE_BRANCH=${BASE_BRANCH:-main} + + DIFF=$(git diff "${PR_REMOTE}/${BASE_BRANCH}...FETCH_HEAD") + FILES_CHANGED=$(git diff --name-only "${PR_REMOTE}/${BASE_BRANCH}...FETCH_HEAD") + + echo "Successfully fetched PR #${PR_NUMBER}" + fi +elif [[ ${#FILES[@]} -gt 0 ]]; then + echo "Reviewing specified files: ${FILES[*]}" + DIFF=$(git diff "$DIFF_RANGE" -- "${FILES[@]}") + FILES_CHANGED=$(printf '%s\n' "${FILES[@]}") +else + echo "Reviewing changes in range: $DIFF_RANGE" + DIFF=$(git diff "$DIFF_RANGE") + FILES_CHANGED=$(git diff --name-only "$DIFF_RANGE") +fi + +if [[ -z "$DIFF" ]]; then + echo "No changes found to review." + exit 0 +fi + +echo "Files changed:" +echo "$FILES_CHANGED" +echo "" +echo "Generating code review..." +echo "" + +# Create prompt for the agent +REPO_NAME=$(git remote get-url origin | sed -e 's/.*[:/]\([^/]*\/[^/]*\)\.git$/\1/' -e 's/.*[:/]\([^/]*\/[^/]*\)$/\1/') + +REVIEW_PROMPT=$(cat < "$PROMPT_FILE" + +echo "=== CODE REVIEW RESULTS ===" +echo "" + +# Check if claude CLI is available +if command -v claude &> /dev/null; then + # Use Claude Code CLI if available + claude task --type general-purpose --prompt "@${PROMPT_FILE}" --output "$OUTPUT_FILE" + echo "" + echo "Review saved to: $OUTPUT_FILE" + + # Pretty print if jq is available + if command -v jq &> /dev/null && [[ -f "$OUTPUT_FILE" ]]; then + echo "" + echo "=== SUMMARY ===" + jq -r '.summary // "No summary available"' "$OUTPUT_FILE" + echo "" + echo "Verdict: $(jq -r '.verdict // "Unknown"' "$OUTPUT_FILE")" + echo "" + echo "Top Risks:" + jq -r '.top_risks[]? // "None identified"' "$OUTPUT_FILE" | sed 's/^/ - /' + fi +else + # Fallback: Print instructions for manual review + echo "Claude Code CLI not found." + echo "" + echo "To perform the review, copy the following prompt to Claude Code:" + echo "---------------------------------------------------------------" + cat "$PROMPT_FILE" + echo "---------------------------------------------------------------" +fi + +rm -f "$PROMPT_FILE" diff --git a/test/CODE_REVIEW_AGENT.md b/test/CODE_REVIEW_AGENT.md new file mode 100644 index 000000000..f40bfc292 --- /dev/null +++ b/test/CODE_REVIEW_AGENT.md @@ -0,0 +1,334 @@ +# Code Review Subagent for Claude Code + +## Overview + +This directory contains a specialized code review subagent designed for the OpenShift Cluster Version Operator (CVO) repository. The agent leverages Claude Code's Task tool system to provide automated, structured code reviews. + +## What the Agent Does + +The code review agent: +- Analyzes Go code changes for correctness, security, and maintainability +- Applies CVO-specific domain knowledge (release payloads, task graphs, feature gates) +- Runs a comprehensive checklist covering 7 key areas +- Produces structured JSON output with actionable findings +- Assigns severity levels and provides specific line-number references +- Gives a clear verdict: LGTM, Minor, Major, or Reject + +## Files in This Directory + +- **code-review-agent.json** - Agent definition with prompt and configuration +- **review.sh** - Bash wrapper to invoke the agent easily +- **CODE_REVIEW_AGENT.md** - This file + +## Quick Start + +### Method 1: Using the Bash Wrapper (Recommended) + +```bash +# Review your current branch against origin/main +./test/review.sh + +# Review a specific pull request +./test/review.sh --pr 1273 + +# Review specific commits +./test/review.sh --range HEAD~3..HEAD + +# Review specific files only +./test/review.sh --files pkg/cvo/cvo.go pkg/payload/payload.go + +# Save output to custom location +./test/review.sh --output my-review.json +``` + +### Method 2: Direct Invocation in Claude Code + +When working in Claude Code, you can directly invoke the code review agent: + +``` +Please review the changes in my current branch using the code-review agent. +Use the Task tool with the following: +- subagent_type: general-purpose +- Load the prompt from test/code-review-agent.json +- Provide the git diff and changed files as context +``` + +### Method 3: Manual Usage + +1. Get your diff: + ```bash + git diff origin/main...HEAD > /tmp/my-changes.diff + ``` + +2. Copy the prompt template from [code-review-agent.json](code-review-agent.json) + +3. Provide to Claude Code with your diff + +## Review Checklist + +The agent evaluates changes across these dimensions: + +### 1. **Correctness** (Critical) +- Logic errors, nil checks, race conditions +- Proper error handling and propagation +- Edge cases: empty inputs, boundaries, concurrent access +- Kubernetes resource lifecycle handling + +### 2. **Testing** (High Priority) +- Unit test coverage for new logic +- Integration tests for end-to-end flows +- Table-driven test patterns +- Error path testing +- Proper use of fakes/mocks + +### 3. **Go Idioms and Style** (Medium Priority) +- Naming conventions (exported vs unexported) +- Idiomatic error handling (errors.Wrap, fmt.Errorf) +- Context usage and cancellation +- Resource cleanup with defer +- Goroutine and channel best practices + +### 4. **Security** (Critical) +- No secrets or credentials in code/logs +- Command injection prevention +- RBAC permissions correctness +- Input validation +- TLS certificate validation +- Image signature verification + +### 5. **Performance** (Medium Priority) +- Allocation efficiency in hot paths +- Proper data structure selection (maps vs slices) +- API server query optimization (avoid N+1) +- Rate limiting and backoff +- Resource leak prevention + +### 6. **Maintainability** (Medium Priority) +- Function length and complexity +- Clear variable naming +- Comments for complex logic +- Consistent error messages +- Appropriate logging levels (V(2), V(4)) + +### 7. **CVO-Specific** (High Priority) +- Release payload verification +- Task graph dependencies +- ClusterVersion status updates +- Upgrade compatibility +- Metrics naming/cardinality +- Feature gate handling +- Capability filtering + +## Output Format + +The agent produces JSON output: + +```json +{ + "summary": "Brief description of the change", + "verdict": "LGTM | Minor | Major | Reject", + "top_risks": [ + "Risk 1: Nil pointer in edge case", + "Risk 2: Missing integration test", + "Risk 3: Potential race condition" + ], + "findings": [ + { + "file": "pkg/cvo/cvo.go", + "line_or_range": "142", + "severity": "high", + "category": "correctness", + "message": "Potential nil pointer dereference when config.Spec is nil", + "suggestion": "Add nil check: if config.Spec == nil { return nil, fmt.Errorf(...) }" + } + ], + "positive_observations": [ + "Excellent use of table-driven tests", + "Good error wrapping with context" + ] +} +``` + +### Verdict Meanings + +- **LGTM**: Ready to merge. No blocking issues. +- **Minor**: Small fixes needed (typos, comments). Quick iteration. +- **Major**: Significant changes required (logic errors, missing tests). Re-review needed. +- **Reject**: Fundamental issues (security, wrong approach, breaks compatibility). + +## Integration with CI/CD + +### GitHub Actions Example + +```yaml +name: Code Review +on: [pull_request] + +jobs: + review: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + + - name: Run Code Review Agent + run: | + ./test/review.sh --range origin/${{ github.base_ref }}...${{ github.sha }} \ + --output review-results.json + + - name: Post Review Comments + uses: actions/github-script@v6 + with: + script: | + const fs = require('fs'); + const review = JSON.parse(fs.readFileSync('review-results.json')); + + // Post findings as PR comments + for (const finding of review.findings) { + if (finding.severity === 'high' || finding.severity === 'medium') { + await github.rest.pulls.createReviewComment({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + body: `**${finding.severity.toUpperCase()}**: ${finding.message}`, + path: finding.file, + line: parseInt(finding.line_or_range) + }); + } + } +``` + +### GitLab CI Example + +```yaml +code-review: + stage: review + script: + - ./test/review.sh --range origin/main...HEAD --output review.json + - cat review.json | jq -r '.findings[] | select(.severity=="high") | .message' + artifacts: + reports: + codequality: review.json +``` + +## Customization + +### Adjusting the Agent Prompt + +Edit [code-review-agent.json](code-review-agent.json) to: +- Add repository-specific rules +- Adjust severity thresholds +- Include additional checklist items +- Change output format + +### Adding Custom Checks + +For project-specific validation: + +```bash +# Create custom pre-check script +cat > test/custom-checks.sh <<'EOF' +#!/bin/bash +# Add custom validation before code review +./hack/verify-yaml +./hack/verify-generated-code +EOF + +# Modify review.sh to call it +``` + +## Best Practices + +1. **Run locally before pushing**: Catch issues early + ```bash + ./test/review.sh --files $(git diff --name-only HEAD) + ``` + +2. **Review incrementally**: Don't wait for large PRs + ```bash + ./test/review.sh --range HEAD~1..HEAD # Review each commit + ``` + +3. **Combine with linters**: Use alongside golangci-lint, staticcheck + ```bash + golangci-lint run ./... + ./test/review.sh + ``` + +4. **Focus on high/medium severity**: Triage findings by severity + +5. **Iterate**: Use Minor/Major verdicts to guide revision cycles + +## Limitations + +- **Context window**: Very large diffs (>100 files) may need splitting +- **Not a replacement for human review**: Use as a first-pass filter +- **Go-specific**: Optimized for Go code in CVO +- **Requires git**: Relies on git for diff generation + +## Troubleshooting + +### "No changes found to review" +```bash +# Check your diff range +git diff origin/main...HEAD + +# Ensure you've fetched latest +git fetch origin +``` + +### "Claude Code CLI not found" +```bash +# Install Claude Code +npm install -g @anthropic-ai/claude-code + +# Or use manual method +./test/review.sh # Follow printed instructions +``` + +### Agent times out on large diffs +```bash +# Split review by directory +./test/review.sh --files pkg/cvo/*.go +./test/review.sh --files pkg/payload/*.go + +# Or review commit-by-commit +for sha in $(git rev-list origin/main..HEAD); do + ./test/review.sh --range ${sha}^..${sha} +done +``` + +## Contributing + +To improve the code review agent: + +1. Test changes: + ```bash + # Edit code-review-agent.json + vim test/code-review-agent.json + + # Test with a known PR + ./test/review.sh --pr 1273 + ``` + +2. Validate JSON output: + ```bash + jq empty review-output.json # Should exit 0 + ``` + +3. Update documentation in this file + +## Related Resources + +- [CVO Architecture Documentation](../CLAUDE.md) +- [OpenShift Testing Guide](https://github.com/openshift/release/blob/master/ci-operator/README.md) +- [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments) +- [Kubernetes API Conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md) + +## Support + +For issues or questions: +- File an issue in the CVO repository +- Reach out to the OpenShift CVO team +- Consult the [development docs](../docs/dev/) diff --git a/test/README.md b/test/README.md new file mode 100644 index 000000000..51eb80953 --- /dev/null +++ b/test/README.md @@ -0,0 +1,174 @@ +# CVO Test Directory + +This directory contains integration tests and development tools for the Cluster Version Operator (CVO). + +## Contents + +### Integration Tests +- **cvo/** - CVO-specific integration test suites +- **oc/** - OpenShift CLI test utilities and helpers + +These tests are executed as part of OpenShift's test extension framework. See the [main CVO documentation](../CLAUDE.md#testing-commands) for how to run them. + +### Code Review Agent 🤖 + +An automated code review subagent for Claude Code that provides structured analysis of code changes. + +**Quick Start:** +```bash +# Review your current branch +./test/review.sh + +# Review a specific PR +./test/review.sh --pr 1273 +``` + +**Documentation:** +- [CODE_REVIEW_AGENT.md](CODE_REVIEW_AGENT.md) - Full documentation and setup guide +- [example-review-usage.md](example-review-usage.md) - Practical examples and recipes +- [code-review-agent.json](code-review-agent.json) - Agent configuration + +**Key Features:** +- ✅ CVO-specific domain knowledge (task graphs, feature gates, payloads) +- ✅ Comprehensive checklist: correctness, security, performance, tests +- ✅ Structured JSON output with severity levels +- ✅ Integration with CI/CD pipelines +- ✅ Git hook support for pre-push/pre-commit validation + +See [CODE_REVIEW_AGENT.md](CODE_REVIEW_AGENT.md) for complete usage instructions. + +## Running Integration Tests + +Integration tests require a disposable OpenShift cluster with admin credentials: + +```bash +# Set KUBECONFIG to your test cluster +export KUBECONFIG=/path/to/test-cluster-kubeconfig + +# Run all integration tests +make integration-test + +# Or use gotestsum directly +gotestsum --packages="./test/..." -- -test.v -timeout 30m +``` + +**⚠️ Warning:** Never run integration tests against production clusters. These tests modify cluster state and can disrupt operations. + +## Test Organization + +Tests follow OpenShift's extension test framework: + +``` +test/ +├── cvo/ +│ └── cvo.go # Main CVO integration test suite +├── oc/ +│ ├── api/ # Kubernetes API helpers +│ ├── cli/ # OpenShift CLI utilities +│ └── oc.go # oc command wrappers +└── ... +``` + +### Writing New Tests + +When adding integration tests: + +1. **Use Ginkgo/Gomega**: Follow OpenShift testing conventions + ```go + var _ = g.Describe("[Feature:ClusterVersionOperator] CVO", func() { + g.It("should reconcile payloads", func() { + // Test implementation + }) + }) + ``` + +2. **Add test metadata**: Update `.openshift-tests-extension/openshift_payload_cluster-version-operator.json` + +3. **Use helper utilities**: Leverage `test/oc` package for cluster operations + ```go + import "github.com/openshift/cluster-version-operator/test/oc" + + client := oc.NewClient() + cv, err := client.GetClusterVersion(ctx, "version") + ``` + +4. **Clean up resources**: Ensure tests clean up after themselves + ```go + defer func() { + client.Delete(ctx, testNamespace) + }() + ``` + +5. **Mark destructive tests**: Use `[Serial]` for tests that can't run in parallel + ```go + var _ = g.Describe("[Serial] CVO upgrade", func() { ... }) + ``` + +## Test Metadata + +Test metadata for OpenShift CI is in: +- `.openshift-tests-extension/openshift_payload_cluster-version-operator.json` + +This file registers CVO tests with the OpenShift test suite, defining: +- Test names and descriptions +- Resource requirements +- Lifecycle (blocking vs. informing) +- Environment selectors + +Update this file when adding new test scenarios. + +## CI/CD Integration + +### Prow Jobs + +CVO tests run in OpenShift CI via Prow. Test results and artifacts are available at: +- https://prow.ci.openshift.org/ + +Search for "cluster-version-operator" to find relevant jobs. + +### Code Review in CI + +Integrate the code review agent into your CI pipeline: + +```yaml +# Example: .github/workflows/review.yml +name: Code Review +on: [pull_request] +jobs: + review: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Review PR + run: ./test/review.sh --output review.json + - uses: actions/upload-artifact@v3 + with: + name: code-review + path: review.json +``` + +## Development Workflow + +1. **Make changes** to CVO code +2. **Run unit tests**: `make test` +3. **Review changes**: `./test/review.sh` +4. **Fix issues** identified in review +5. **Run integration tests** (on test cluster): `make integration-test` +6. **Commit** with conventional format: `pkg/cvo: fix synchronization bug` + +## Related Documentation + +- [CVO Architecture](../CLAUDE.md) - Main CVO documentation +- [Development Guide](../docs/dev/README.md) - Building and testing CVO +- [OpenShift Testing](https://github.com/openshift/release/blob/master/ci-operator/README.md) + +## Getting Help + +- File issues in the CVO repository +- Check Prow job logs for test failures +- Review code with the automated agent first to catch common issues +- Reach out to the OpenShift CVO team on Slack + +--- + +**💡 Tip:** Start every PR by running `./test/review.sh` to catch issues early! diff --git a/test/REVIEW_SCRIPT_UPDATES.md b/test/REVIEW_SCRIPT_UPDATES.md new file mode 100644 index 000000000..b0f014c87 --- /dev/null +++ b/test/REVIEW_SCRIPT_UPDATES.md @@ -0,0 +1,218 @@ +# Review Script Updates - Hybrid gh/git Approach + +## Summary + +Updated [review.sh](review.sh) to use a **hybrid approach** for PR reviews: +1. **Preferred**: Use `gh` CLI if available (best experience) +2. **Fallback**: Use `git fetch` if `gh` not installed (no extra dependencies) + +## What Changed + +### 1. Smart Detection (Line 94) +```bash +if command -v gh &> /dev/null; then + # Use gh CLI path +else + # Use git fetch fallback +fi +``` + +### 2. Git Fallback Implementation (Lines 106-142) +When `gh` CLI is not available: +- Fetches PR using GitHub's pull request refs: `pull/{PR_NUMBER}/head` +- Auto-detects base branch from remote +- Supports custom remote via `PR_REMOTE` environment variable +- Provides helpful error messages with multiple solutions + +### 3. New Environment Variable: `PR_REMOTE` +```bash +PR_REMOTE="${PR_REMOTE:-origin}" # Default: origin, can override +``` + +Allows fetching PRs from different remotes (useful for forks): +```bash +PR_REMOTE=upstream ./test/review.sh --pr 1234 +``` + +### 4. Improved Error Messages (Lines 120-131) +Now explains: +- Why fetch might fail (fork, closed PR, network) +- 4 different solutions to try +- How to use `PR_REMOTE` environment variable + +### 5. Updated Usage Documentation +- Documents `PR_REMOTE` environment variable +- Explains gh CLI vs git fetch behavior +- Shows fork workflow examples + +## Usage Examples + +### With gh CLI (Recommended) +```bash +# Install gh first: https://cli.github.com/ +gh auth login + +# Review a PR +./test/review.sh --pr 1273 +``` + +**Output:** +``` +Fetching PR #1273... +Using 'gh' CLI to fetch PR... +Files changed: +pkg/cvo/cvo.go +pkg/payload/payload.go +... +``` + +### Without gh CLI (Git Fallback) + +#### From Upstream Repository +```bash +# Works if 'origin' points to github.com/openshift/cluster-version-operator +./test/review.sh --pr 1273 +``` + +**Output:** +``` +Fetching PR #1273... +Note: 'gh' CLI not found. Using git fetch as fallback. +Install 'gh' for better PR integration: https://cli.github.com/ + +Fetching pull/1273/head from remote 'origin'... +Successfully fetched PR #1273 +``` + +#### From a Fork +```bash +# Add upstream remote (one time) +git remote add upstream https://github.com/openshift/cluster-version-operator.git + +# Fetch PR from upstream +PR_REMOTE=upstream ./test/review.sh --pr 1273 +``` + +**Output:** +``` +Fetching PR #1273... +Note: 'gh' CLI not found. Using git fetch as fallback. +Install 'gh' for better PR integration: https://cli.github.com/ + +Fetching pull/1273/head from remote 'upstream'... +Successfully fetched PR #1273 +``` + +### Error Handling Example + +If git fetch fails: +``` +Error: Could not fetch PR #1273 from remote 'origin' + +This can happen if: + - Your 'origin' remote is a fork (not the upstream repo) + - The PR doesn't exist or is closed + - You don't have network access to the repository + +Possible solutions: + 1. Install 'gh' CLI (recommended): https://cli.github.com/manual/installation + 2. Add upstream remote: git remote add upstream https://github.com/openshift/cluster-version-operator.git + Then use: PR_REMOTE=upstream ./test/review.sh --pr 1273 + 3. Manually fetch: git fetch origin pull/1273/head:pr-1273 + 4. Use git range instead: ./test/review.sh --range origin/main...HEAD +``` + +## Comparison: gh CLI vs Git Fallback + +| Feature | gh CLI | Git Fallback | +|---------|--------|--------------| +| **Installation** | Requires gh CLI | No extra tools needed | +| **Authentication** | Handles auth automatically | Uses git credentials | +| **PR Metadata** | Full metadata (title, author, etc.) | Limited to diff | +| **Private Repos** | ✅ Works with auth | ⚠️ Requires git access | +| **Fork Support** | ✅ Automatic | ⚠️ Needs `PR_REMOTE=upstream` | +| **Closed PRs** | ✅ Can fetch | ✅ Can fetch | +| **Error Messages** | Detailed | Basic | + +## Recommendations + +### For Individual Developers +**Install `gh` CLI** - Best experience, handles all edge cases: +```bash +# macOS +brew install gh + +# Linux +sudo apt install gh # Debian/Ubuntu +sudo dnf install gh # Fedora/RHEL + +# Authenticate +gh auth login +``` + +### For CI/CD Environments +- GitHub Actions: `gh` is pre-installed ✅ +- GitLab CI: Use git fallback or install `gh` +- Prow Jobs: Typically has `gh` available + +### For Fork Workflows +```bash +# One-time setup +git remote add upstream https://github.com/openshift/cluster-version-operator.git + +# Create shell alias for convenience +alias review-pr='PR_REMOTE=upstream ./test/review.sh --pr' + +# Use it +review-pr 1273 +``` + +## Testing the Changes + +### Test gh CLI Path +```bash +# Requires gh CLI installed +./test/review.sh --pr 1273 +``` + +### Test Git Fallback Path +```bash +# Temporarily hide gh CLI +alias gh='echo "gh not found" && false' + +# Run review +./test/review.sh --pr 1273 + +# Remove alias +unalias gh +``` + +### Test Custom Remote +```bash +git remote add upstream https://github.com/openshift/cluster-version-operator.git +PR_REMOTE=upstream ./test/review.sh --pr 1273 +``` + +## Files Modified + +1. **[review.sh](review.sh)** - Main script + - Lines 17-18: Added `PR_REMOTE` variable + - Lines 23-45: Updated usage documentation + - Lines 91-142: Hybrid gh/git implementation + - Error handling and messaging improvements + +## Benefits + +1. **No Hard Dependency**: Works without `gh` CLI +2. **Better UX**: Tries `gh` first, falls back gracefully +3. **Fork Support**: `PR_REMOTE` handles fork workflows +4. **Clear Errors**: Helpful messages with actionable solutions +5. **Flexible**: Multiple ways to accomplish the same goal + +## Future Enhancements + +Potential improvements for future versions: +- Cache fetched PRs to avoid re-fetching +- Support PR URLs in addition to numbers +- Add `--pr-remote` flag as alternative to env var +- Integrate with Claude Code's native PR review features diff --git a/test/code-review-agent.json b/test/code-review-agent.json new file mode 100644 index 000000000..d30053340 --- /dev/null +++ b/test/code-review-agent.json @@ -0,0 +1,9 @@ +{ + "name": "code-review", + "version": "1.0.0", + "description": "Automated code review agent for OpenShift Cluster Version Operator", + "type": "general-purpose", + "prompt": "You are an expert senior engineer performing automated code reviews for the OpenShift Cluster Version Operator (CVO) repository.\n\nYour task is to analyze code changes and provide structured, actionable feedback.\n\n## Context About CVO\n\n- CVO manages OpenShift cluster version updates and reconciliation\n- Written in Go, follows Kubernetes controller patterns\n- Critical component - bugs can disrupt cluster operations\n- Must handle edge cases: network failures, partial updates, rollbacks\n- Security-sensitive: handles cluster-wide operations with elevated privileges\n\n## Review Checklist\n\nFor each change, systematically evaluate:\n\n### 1. Correctness\n- Logic errors or off-by-one errors\n- Nil pointer dereferences\n- Race conditions in concurrent code\n- Proper error handling and propagation\n- Edge cases: empty lists, nil values, boundary conditions\n- Kubernetes resource lifecycle (create, update, delete, watch)\n\n### 2. Testing\n- Are unit tests added/updated for new logic?\n- Are integration tests needed for end-to-end flows?\n- Are table-driven tests used appropriately?\n- Are error paths tested?\n- Are there obvious untested edge cases?\n- Do tests use proper mocking/fakes?\n\n### 3. Go Idioms and Style\n- Naming conventions (camelCase for private, PascalCase for exported)\n- Error handling (errors.Wrap vs fmt.Errorf)\n- Context usage and cancellation\n- Resource cleanup (defer for Close, mutex unlocking)\n- Proper use of goroutines and channels\n- Avoid naked returns in long functions\n\n### 4. Security\n- Secrets or credentials in code/logs\n- Command injection risks (exec.Command with user input)\n- RBAC: proper permissions for resources\n- Input validation and sanitization\n- TLS certificate validation\n- Image signature verification\n\n### 5. Performance\n- Unnecessary allocations in hot paths\n- Efficient use of maps vs slices\n- Proper indexing for lookups\n- Avoid N+1 queries to API server\n- Rate limiting and backoff strategies\n- Memory leaks (goroutine leaks, unclosed resources)\n\n### 6. Maintainability\n- Functions > 50 lines (consider refactoring)\n- Cyclomatic complexity\n- Clear variable names\n- Comments for non-obvious logic\n- Consistent error messages\n- Logging at appropriate levels (V(2) for debug, V(4) for trace)\n\n### 7. CVO-Specific Concerns\n- Release payload handling and verification\n- Task graph dependency ordering\n- ClusterVersion status condition updates\n- Compatibility with existing clusters during upgrades\n- Metrics naming and cardinality\n- Feature gate handling\n- Capability filtering\n\n## Output Format\n\nProvide your review as a JSON object with this structure:\n\n```json\n{\n \"summary\": \"Brief 1-2 sentence summary of the change\",\n \"verdict\": \"LGTM | Minor | Major | Reject\",\n \"top_risks\": [\n \"Risk 1: Description\",\n \"Risk 2: Description\",\n \"Risk 3: Description\"\n ],\n \"findings\": [\n {\n \"file\": \"path/to/file.go\",\n \"line_or_range\": \"42\" or \"42-51\",\n \"severity\": \"low | medium | high\",\n \"category\": \"correctness | testing | style | security | performance | maintainability\",\n \"message\": \"Clear description of the issue\",\n \"suggestion\": \"Optional: specific code suggestion or fix\"\n }\n ],\n \"positive_observations\": [\n \"Things done well in this change\"\n ]\n}\n```\n\n## Verdict Definitions\n\n- **LGTM**: Looks good to merge. No blocking issues, only minor nits if any.\n- **Minor**: Needs small changes (typos, comments, minor refactoring). Can merge after quick fixes.\n- **Major**: Requires significant changes (logic errors, missing tests, architectural concerns). Needs another review.\n- **Reject**: Fundamental issues (security vulnerabilities, wrong approach, breaks compatibility).\n\n## Guidelines\n\n- Be specific: Reference exact line numbers and files\n- Be constructive: Explain why something is an issue and how to fix it\n- Be concise: Focus on actionable items, not theoretical concerns\n- Be fair: Acknowledge good practices and clever solutions\n- Prioritize: Focus on correctness and security first, then performance and style\n- Context matters: Consider the scope of the change - don't demand perfection for small fixes\n\nNow analyze the provided code changes and deliver your review.", + "tools": ["Read", "Grep", "Glob", "Bash"], + "max_turns": 15 +} diff --git a/test/example-review-usage.md b/test/example-review-usage.md new file mode 100644 index 000000000..9f10af9b8 --- /dev/null +++ b/test/example-review-usage.md @@ -0,0 +1,376 @@ +# Example: Using the Code Review Subagent + +This document shows concrete examples of how to use the code review subagent in different scenarios. + +## Example 1: Review Current Branch in Claude Code + +When you're in a Claude Code session, simply ask: + +``` +Please review my current changes against origin/main using the code review agent. +``` + +Claude Code will: +1. Load the agent configuration from `test/code-review-agent.json` +2. Get the diff using `git diff origin/main...HEAD` +3. Invoke the Task tool with subagent_type="general-purpose" +4. Return structured findings in JSON format + +## Example 2: Review a Specific PR + +### Using the Shell Script + +```bash +cd /path/to/cluster-version-operator +./test/review.sh --pr 1273 +``` + +**Output:** +``` +Fetching PR #1273... +Files changed: +pkg/cvo/cvo.go +pkg/payload/payload.go +... + +Generating code review... + +=== CODE REVIEW RESULTS === + +=== SUMMARY === +Add feature gate based manifest inclusion with integration tests + +Verdict: LGTM + +Top Risks: + - Risk 1: Feature gate filtering may exclude critical operators if misconfigured + - Risk 2: Integration test coverage for edge cases (empty feature set) could be improved + - Risk 3: Backward compatibility needs verification for clusters without feature gates + +Review saved to: test/review-output.json +``` + +## Example 3: Review Only Modified Go Files + +```bash +# Get list of modified .go files +MODIFIED_GO_FILES=$(git diff --name-only origin/main...HEAD | grep '\.go$') + +# Review only those files +./test/review.sh --files $MODIFIED_GO_FILES +``` + +## Example 4: Incremental Review During Development + +While developing a feature, review each commit: + +```bash +# Review your last commit +./test/review.sh --range HEAD~1..HEAD + +# Review last 3 commits individually +for i in {1..3}; do + echo "=== Reviewing commit HEAD~$((i-1)) ===" + ./test/review.sh --range HEAD~${i}..HEAD~$((i-1)) +done +``` + +## Example 5: Pre-Push Hook Integration + +Create a git pre-push hook to automatically review changes: + +```bash +cat > .git/hooks/pre-push <<'EOF' +#!/bin/bash +# Pre-push hook to run code review + +REMOTE="$1" +URL="$2" + +# Only run for main branch +CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD) +if [[ "$CURRENT_BRANCH" != "main" ]]; then + echo "Running code review for branch: $CURRENT_BRANCH" + ./test/review.sh --range origin/main...HEAD --output /tmp/pre-push-review.json + + # Check verdict + VERDICT=$(jq -r '.verdict' /tmp/pre-push-review.json 2>/dev/null || echo "Unknown") + + if [[ "$VERDICT" == "Reject" ]]; then + echo "❌ Code review FAILED: Reject verdict" + echo "Please address critical issues before pushing." + echo "Review details: /tmp/pre-push-review.json" + exit 1 + elif [[ "$VERDICT" == "Major" ]]; then + echo "⚠️ Code review found MAJOR issues" + echo "Consider addressing before pushing." + jq -r '.top_risks[]' /tmp/pre-push-review.json | sed 's/^/ - /' + fi +fi + +exit 0 +EOF + +chmod +x .git/hooks/pre-push +``` + +## Example 6: CI/CD Integration + +### Prow Job Configuration + +```yaml +# In ci-operator config +tests: +- as: code-review + commands: | + #!/bin/bash + set -euo pipefail + + # Review changes in this PR + ./test/review.sh --range origin/main...HEAD --output ${ARTIFACT_DIR}/review.json + + # Fail job if critical issues found + HIGH_SEVERITY=$(jq '[.findings[] | select(.severity=="high")] | length' ${ARTIFACT_DIR}/review.json) + if [[ $HIGH_SEVERITY -gt 0 ]]; then + echo "Found $HIGH_SEVERITY high-severity issues" + jq -r '.findings[] | select(.severity=="high") | "\(.file):\(.line_or_range) - \(.message)"' ${ARTIFACT_DIR}/review.json + exit 1 + fi + container: + from: src +``` + +## Example 7: Filtering Review Results + +Extract only high-severity security findings: + +```bash +./test/review.sh --output review.json + +jq -r '.findings[] | + select(.severity=="high" and .category=="security") | + "\(.file):\(.line_or_range)\n \(.message)\n Fix: \(.suggestion // "See details above")\n"' \ + review.json +``` + +**Output:** +``` +pkg/cvo/cvo.go:245 + API token logged in debug statement - potential credential leak + Fix: Use klog.V(4).Infof("token: [REDACTED]") instead + +pkg/payload/payload.go:112 + Image signature verification skipped when URL starts with "http://" + Fix: Reject non-HTTPS URLs: if !strings.HasPrefix(url, "https://") { return err } +``` + +## Example 8: Compare Reviews Across Commits + +Track how code quality changes: + +```bash +# Review original version +git checkout v4.15.0 +./test/review.sh --range HEAD~5..HEAD --output review-v4.15.0.json + +# Review current version +git checkout main +./test/review.sh --range HEAD~5..HEAD --output review-main.json + +# Compare findings count +echo "v4.15.0 findings: $(jq '.findings | length' review-v4.15.0.json)" +echo "main findings: $(jq '.findings | length' review-main.json)" + +# Compare severity distribution +jq -r '.findings | group_by(.severity) | + map({severity: .[0].severity, count: length}) | + .[] | "\(.severity): \(.count)"' review-main.json +``` + +## Example 9: Custom Filtering for CVO Components + +Review only changes to specific CVO components: + +```bash +# Review only CVO core operator changes +./test/review.sh --files $(git diff --name-only origin/main...HEAD | grep '^pkg/cvo/') + +# Review only payload-related changes +./test/review.sh --files $(git diff --name-only origin/main...HEAD | grep '^pkg/payload/') + +# Review only library changes +./test/review.sh --files $(git diff --name-only origin/main...HEAD | grep '^lib/') +``` + +## Example 10: Automated PR Comments + +Post review findings as PR comments using `gh`: + +```bash +#!/bin/bash +PR_NUMBER=$1 + +# Run review +./test/review.sh --pr "$PR_NUMBER" --output review.json + +# Get findings +FINDINGS=$(jq -r '.findings[] | + select(.severity=="high" or .severity=="medium") | + "**[\(.severity | ascii_upcase)] \(.file):\(.line_or_range)**\n\n\(.message)\n\n\(.suggestion // "")\n"' review.json) + +# Post as PR comment +gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review + +$FINDINGS + +--- +_Generated by CVO code review agent_" +``` + +## Example 11: Review with Context from Logs + +When reviewing a bug fix, include error logs as context: + +```bash +# Suppose you're fixing a bug from a failing test +ERROR_LOG=$(oc logs -n openshift-cluster-version -l k8s-app=cluster-version-operator | tail -100) + +# Create enhanced prompt +cat > /tmp/review-with-context.txt < batch-review.md +for pr in "${PRS[@]}"; do + echo "## PR #$pr" >> batch-review.md + jq -r '"**Verdict:** \(.verdict)\n\n**Top Risks:**\n\(.top_risks[] | "- \(.)")\n"' \ + "review-pr-${pr}.json" >> batch-review.md +done + +cat batch-review.md +``` + +## Tips and Best Practices + +### 1. Use Incremental Reviews +Don't wait until you have 50 files changed. Review frequently: +```bash +# After each logical change +git add -p # Stage changes +./test/review.sh --files $(git diff --cached --name-only) +``` + +### 2. Combine with Linters +Code review agent complements but doesn't replace static analysis: +```bash +golangci-lint run ./... | tee lint-results.txt +./test/review.sh --output review.json + +# Check both +echo "Lint issues: $(wc -l < lint-results.txt)" +echo "Review findings: $(jq '.findings | length' review.json)" +``` + +### 3. Focus on Your Changes +Use `--files` to review only what you touched: +```bash +MY_FILES=$(git diff --name-only origin/main...HEAD | grep '^pkg/cvo/') +./test/review.sh --files $MY_FILES +``` + +### 4. Archive Reviews +Keep historical reviews for reference: +```bash +TIMESTAMP=$(date +%Y%m%d-%H%M%S) +./test/review.sh --output "reviews/review-${TIMESTAMP}.json" +git add "reviews/review-${TIMESTAMP}.json" +``` + +### 5. Review Before Committing +Make code review part of your pre-commit workflow: +```bash +# Stage changes +git add pkg/cvo/cvo.go + +# Review staged changes +git diff --cached | ./test/review.sh --diff-file /dev/stdin + +# If LGTM, commit +git commit -m "cvo: fix synchronization race condition" +``` + +## Troubleshooting Examples + +### Large Diff Timeout +```bash +# Split by file +for file in $(git diff --name-only origin/main...HEAD); do + echo "Reviewing $file..." + git diff origin/main...HEAD -- "$file" > /tmp/single-file.diff + ./test/review.sh --diff-file /tmp/single-file.diff --output "review-$(basename $file).json" +done +``` + +### No Changes Detected +```bash +# Check your diff +git diff origin/main...HEAD + +# Update origin +git fetch origin + +# Verify branch +git log --oneline origin/main..HEAD +``` + +### JSON Parse Errors +```bash +# Validate review output +jq empty review.json && echo "Valid JSON" || echo "Invalid JSON" + +# Pretty-print for debugging +jq . review.json + +# Extract just the summary if full JSON is malformed +grep -A 5 '"summary"' review.json +``` + +--- + +## Next Steps + +- Read [CODE_REVIEW_AGENT.md](CODE_REVIEW_AGENT.md) for full documentation +- Customize [code-review-agent.json](code-review-agent.json) for your needs +- Integrate into your CI/CD pipeline +- Share feedback with the CVO team diff --git a/test/review.sh b/test/review.sh new file mode 120000 index 000000000..fc77e5609 --- /dev/null +++ b/test/review.sh @@ -0,0 +1 @@ +hack/review.sh \ No newline at end of file