Skip to content

Commit b573907

Browse files
mdboothmdbooth
committed
Update api-review to do comprehensive test coverage analysis
1 parent ebe535b commit b573907

File tree

3 files changed

+2639
-134
lines changed

3 files changed

+2639
-134
lines changed

‎.claude/commands/api-review.md‎

Lines changed: 71 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -7,175 +7,112 @@ parameters:
77
required: false
88
---
99

10-
# Output Format Requirements
11-
You MUST use this EXACT format for ALL review feedback:
10+
I will assume the role of an experienced software engineer. I will perform a comprehensive review of a proposed OpenShift API change, which may be either a specific GitHub PR or local changes against upstream master.
1211

12+
## Step 1: Pre-flight checks and determine review mode
1313

14-
+LineNumber: Brief description
15-
**Current (problematic) code:**
16-
```go
17-
[exact code from the PR diff]
18-
```
14+
I will directly execute the `pre-flight-checks.sh` script from this command, passing any command arguments as arguments to the script.
1915

20-
**Suggested change:**
21-
```diff
22-
- [old code line]
23-
+ [new code line]
24-
```
16+
The output of this script clearly indicates:
17+
* The changed API files I must review
18+
* Whether the lint check passed
2519

26-
**Explanation:** [Why this change is needed]
20+
Do not continue if:
21+
* An error occurred
22+
* There are no files to review
23+
* The lint check failed
2724

25+
## Step 2: Documentation and Validation Review
2826

29-
I'll run a comprehensive API review for OpenShift API changes. This can review either a specific GitHub PR or local changes against upstream master.
27+
Consult **AGENTS-validations.md** for detailed guidance:
28+
- **Quick Decision Trees** - Fast lookup for validation method selection
29+
- **Best Practices** - Validation patterns, documentation requirements, combining rules
30+
- **Common Errors & Solutions** - Problems to detect and fix during review
31+
- **Acceptable Patterns** - Patterns that should NOT be flagged as issues
32+
- **Test Requirements** - Test coverage rules and requirements
33+
- **Complete Validator Catalog** - Reference for all Format markers and CEL validators
3034

31-
## Step 1: Pre-flight checks and determine review mode
35+
I will now analyze each changed API file according to the requirements in AGENTS-validations.md:
3236

33-
First, I'll check the arguments and determine whether to review a PR or local changes:
37+
### Documentation Requirements (1.3 Documentation Requirements Checklist)
38+
- All fields have documentation comments (2.4 Documentation Requirements)
39+
- Optional fields explain behavior when omitted (2.4 Documentation Requirements, 4.4 "This field is optional")
40+
- Validation rules are documented in human-readable form (2.4 Documentation Requirements, 3.5 Missing Documentation for Validation Constraints)
41+
- Cross-field relationships are both documented AND enforced with XValidation (3.6 Cross-Field Constraints Without Enforcement, 3.8 Cross-Field Validation Placement)
42+
- XValidation rules have accurate message fields (3.9 Inaccurate or Inconsistent Validation Messages)
3443

35-
```bash
36-
# Save current branch
37-
CURRENT_BRANCH=$(git branch --show-current)
38-
echo "📍 Current branch: $CURRENT_BRANCH"
39-
40-
# Check if a PR URL was provided
41-
if [ -n "$ARGUMENTS" ] && [[ "$ARGUMENTS" =~ github\.com.*pull ]]; then
42-
REVIEW_MODE="pr"
43-
PR_NUMBER=$(echo "$ARGUMENTS" | grep -oE '[0-9]+$')
44-
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER"
45-
46-
# For PR review, check for uncommitted changes
47-
if ! git diff --quiet || ! git diff --cached --quiet; then
48-
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review."
49-
echo "Please commit or stash your changes before running the API review."
50-
git status --porcelain
51-
exit 1
52-
fi
53-
echo "✅ No uncommitted changes detected. Safe to proceed with PR review."
54-
else
55-
REVIEW_MODE="local"
56-
echo "🔍 Local review mode: Reviewing local changes against upstream master"
57-
58-
# Find a remote pointing to openshift/api repository
59-
OPENSHIFT_REMOTE=""
60-
for remote in $(git remote); do
61-
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "")
62-
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then
63-
OPENSHIFT_REMOTE="$remote"
64-
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url"
65-
break
66-
fi
67-
done
68-
69-
# If no existing remote found, add upstream
70-
if [ -z "$OPENSHIFT_REMOTE" ]; then
71-
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..."
72-
git remote add upstream https://github.com/openshift/api.git
73-
OPENSHIFT_REMOTE="upstream"
74-
fi
75-
76-
# Fetch latest changes from the OpenShift API remote
77-
echo "🔄 Fetching latest changes from $OPENSHIFT_REMOTE..."
78-
git fetch "$OPENSHIFT_REMOTE" master
79-
fi
80-
```
44+
### Validation Requirements (Section 2: Best Practices)
45+
- Follow validation decision hierarchy (2.1 Validation Decision Hierarchy)
46+
- Prohibited format markers not used (7.1.0 Security: Prohibited Format Markers)
47+
- Cross-field validation placed on parent structs (3.8 Cross-Field Validation Placement)
48+
- Validation messages use exact enum values and JSON field names (3.9 Inaccurate or Inconsistent Validation Messages)
8149

82-
## Step 2: Get changed files based on review mode
50+
## Step 3: Comprehensive Test Coverage Analysis
8351

84-
```bash
85-
if [ "$REVIEW_MODE" = "pr" ]; then
86-
# PR Review: Checkout the PR and get changed files
87-
echo "🔄 Checking out PR #$PR_NUMBER..."
88-
gh pr checkout "$PR_NUMBER"
52+
### Process Reference:
8953

90-
echo "📁 Analyzing changed files in PR..."
91-
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
92-
else
93-
# Local Review: Get changed files compared to openshift remote master
94-
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..."
95-
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
96-
97-
# Also include staged changes
98-
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
99-
if [ -n "$STAGED_FILES" ]; then
100-
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u)
101-
fi
102-
fi
54+
**CRITICAL:** Execute this for *all* validation markers, not just those with other documentation issues.
10355

104-
echo "Changed API files:"
105-
echo "$CHANGED_FILES"
56+
Execute all 7 steps from Section 5.0.1:
57+
1. Extract validation markers from changed API files
58+
2. Categorize using Section 5.0 lookup table
59+
3. Locate test files in `<group>/<version>/tests/<crd-name>/`
60+
4. Map validations to existing tests
61+
5. Verify coverage completeness (minimal vs comprehensive)
62+
6. Identify gaps (missing, insufficient, unnecessary)
63+
7. Report using standard format from 5.0.1
10664

107-
if [ -z "$CHANGED_FILES" ]; then
108-
echo "ℹ️ No API files changed. Nothing to review."
109-
if [ "$REVIEW_MODE" = "pr" ]; then
110-
git checkout "$CURRENT_BRANCH"
111-
fi
112-
exit 0
113-
fi
114-
```
65+
## Step 4: Generate comprehensive review report
11566

116-
## Step 3: Run linting checks on changes
67+
I'll provide a comprehensive report showing:
68+
- ✅ Files that pass all checks
69+
- ❌ Files with documentation issues
70+
- 📋 Specific lines that need attention
71+
- 🧪 Any API validations with insufficient test coverage
72+
- 🧪 Any redundant test coverage
73+
- 📚 Guidance on fixing any issues
11774

118-
```bash
119-
echo "⏳ Running linting checks on changes..."
120-
make lint
121-
122-
if [ $? -ne 0 ]; then
123-
echo "❌ Linting checks failed. Please fix the issues before proceeding."
124-
if [ "$REVIEW_MODE" = "pr" ]; then
125-
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
126-
git checkout "$CURRENT_BRANCH"
127-
fi
128-
exit 1
129-
fi
75+
**CRITICAL:** You MUST use this EXACT format for documentation issues, validation problems, and lint errors (but not test coverage):
13076

131-
echo "✅ Linting checks passed."
77+
+LineNumber: Brief description
78+
**Current (problematic) code:**
79+
```go
80+
[exact code from the PR diff]
13281
```
13382

134-
## Step 4: Documentation validation
135-
136-
For each changed API file, I'll validate:
137-
138-
1. **Field Documentation**: All struct fields must have documentation comments
139-
2. **Optional Field Behavior**: Optional fields must explain what happens when they are omitted
140-
3. **Validation Documentation**: Validation rules must be documented and match markers
141-
142-
Let me check each changed file for these requirements:
143-
144-
```thinking
145-
I need to analyze the changed files to:
146-
1. Find struct fields without documentation
147-
2. Find optional fields without behavior documentation
148-
3. Find validation annotations without corresponding documentation
149-
150-
For each Go file, I'll:
151-
- Look for struct field definitions
152-
- Check if they have preceding comment documentation
153-
- For optional fields (those with `+kubebuilder:validation:Optional` or `+optional`), verify behavior is explained
154-
- For fields with validation annotations, ensure the validation is documented
83+
**Suggested change:**
84+
```diff
85+
- [old code line]
86+
+ [new code line]
15587
```
15688

157-
## Step 5: Generate comprehensive review report
89+
**Explanation:** [Why this change is needed]
15890

159-
I'll provide a comprehensive report showing:
160-
- ✅ Files that pass all checks
161-
- ❌ Files with documentation issues
162-
- 📋 Specific lines that need attention
163-
- 📚 Guidance on fixing any issues
91+
**Test coverage output must follow the Standard Test Coverage Report Format** defined in Section 5.0.2.
16492

165-
The review will fail if any documentation requirements are not met for the changed files.
93+
**Do not provide output for anything that does not require corrective action.**
16694

167-
## Step 6: Switch back to original branch (PR mode only)
95+
The review will fail if any documentation or test coverage requirements are not met for the changed files.
96+
97+
## Step 5: Switch back to original branch (PR mode only)
16898

16999
After completing the review, if we were reviewing a PR, I'll switch back to the original branch:
170100

171101
```bash
102+
cat > /tmp/api_review_step5.sh << 'STEP5_EOF'
103+
#!/bin/bash
104+
source /tmp/api_review_vars.sh
105+
172106
if [ "$REVIEW_MODE" = "pr" ]; then
173107
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
174108
git checkout "$CURRENT_BRANCH"
175109
echo "✅ API review complete. Back on branch: $(git branch --show-current)"
176110
else
177111
echo "✅ Local API review complete."
178112
fi
113+
STEP5_EOF
114+
115+
bash /tmp/api_review_step5.sh
179116
```
180117

181118
**CRITICAL WORKFLOW REQUIREMENTS:**

‎.claude/commands/pre-flight-checks.sh‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
#!/bin/bash
2+
3+
# Save current branch
4+
CURRENT_BRANCH=$(git branch --show-current)
5+
echo "📍 Current branch: $CURRENT_BRANCH"
6+
7+
# Check if an argument was provided
8+
if [ $# -gt 0 ]; then
9+
REVIEW_MODE="pr"
10+
11+
# We expect the argument to be either a PR number, or the URL of a PR,
12+
# which will end in the PR number
13+
PR_NUMBER=$(echo "$1" | grep -oE '[0-9]+$')
14+
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER"
15+
16+
# For PR review, check for uncommitted changes
17+
if ! git diff --quiet || ! git diff --cached --quiet; then
18+
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review."
19+
echo "Please commit or stash your changes before running the API review."
20+
git status --porcelain
21+
exit 1
22+
fi
23+
echo "✅ No uncommitted changes detected. Safe to proceed with PR review."
24+
25+
# PR Review: Checkout the PR and get changed files
26+
echo "🔄 Checking out PR #$PR_NUMBER..."
27+
gh pr checkout "$PR_NUMBER"
28+
29+
echo "📁 Analyzing changed files in PR..."
30+
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
31+
else
32+
REVIEW_MODE="local"
33+
echo "🔍 Local review mode: Reviewing local changes against upstream master"
34+
35+
# Find a remote pointing to openshift/api repository
36+
OPENSHIFT_REMOTE=""
37+
for remote in $(git remote); do
38+
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "")
39+
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then
40+
OPENSHIFT_REMOTE="$remote"
41+
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url"
42+
break
43+
fi
44+
done
45+
46+
# If no existing remote found, add upstream
47+
if [ -z "$OPENSHIFT_REMOTE" ]; then
48+
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..."
49+
git remote add upstream https://github.com/openshift/api.git 2>&1 || true
50+
OPENSHIFT_REMOTE="upstream"
51+
fi
52+
53+
# Local Review: Get changed files compared to openshift remote master
54+
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..."
55+
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
56+
57+
# Also include staged changes
58+
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
59+
if [ -n "$STAGED_FILES" ]; then
60+
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u)
61+
fi
62+
fi
63+
64+
echo "Changed API files:"
65+
echo "$CHANGED_FILES"
66+
67+
if [ -z "$CHANGED_FILES" ]; then
68+
echo "ℹ️ No API files changed. Nothing to review."
69+
if [ "$REVIEW_MODE" = "pr" ]; then
70+
git checkout "$CURRENT_BRANCH"
71+
fi
72+
exit 0
73+
fi
74+
75+
# Count the files
76+
FILE_COUNT=$(echo "$CHANGED_FILES" | wc -l)
77+
echo ""
78+
echo "📊 Total API files changed: $FILE_COUNT"
79+
80+
echo "⏳ Running linting checks on changes..."
81+
make lint
82+
83+
if [ $? -ne 0 ]; then
84+
echo "❌ Linting checks failed. Please fix the issues before proceeding."
85+
if [ "$REVIEW_MODE" = "pr" ]; then
86+
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
87+
git checkout "$CURRENT_BRANCH"
88+
fi
89+
exit 1
90+
fi
91+
92+
echo "✅ Linting checks passed."
93+
94+
# Export variables to restore the environment after review
95+
echo "REVIEW_MODE=$REVIEW_MODE" > /tmp/api_review_vars.sh
96+
echo "CURRENT_BRANCH=$CURRENT_BRANCH" >> /tmp/api_review_vars.sh

0 commit comments

Comments
 (0)