Skip to content

OCPCLOUD-3254: Update api-review to do comprehensive test coverage analysis #2574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPCLOUD-3254: Update api-review to do comprehensive test coverage analysis #2574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 71 additions & 134 deletions .claude/commands/api-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,175 +7,112 @@ parameters:
required: false
---

# Output Format Requirements
You MUST use this EXACT format for ALL review feedback:
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.

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

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

**Suggested change:**
```diff
- [old code line]
+ [new code line]
```
The output of this script clearly indicates:
* The changed API files I must review
* Whether the lint check passed

**Explanation:** [Why this change is needed]
Do not continue if:
* An error occurred
* There are no files to review
* The lint check failed

## Step 2: Documentation and Validation Review

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.
Consult **AGENTS-validations.md** for detailed guidance:
- **Quick Decision Trees** - Fast lookup for validation method selection
- **Best Practices** - Validation patterns, documentation requirements, combining rules
- **Common Errors & Solutions** - Problems to detect and fix during review
- **Acceptable Patterns** - Patterns that should NOT be flagged as issues
- **Test Requirements** - Test coverage rules and requirements
- **Complete Validator Catalog** - Reference for all Format markers and CEL validators

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

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

```bash
# Save current branch
CURRENT_BRANCH=$(git branch --show-current)
echo "📍 Current branch: $CURRENT_BRANCH"

# Check if a PR URL was provided
if [ -n "$ARGUMENTS" ] && [[ "$ARGUMENTS" =~ github\.com.*pull ]]; then
REVIEW_MODE="pr"
PR_NUMBER=$(echo "$ARGUMENTS" | grep -oE '[0-9]+$')
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER"

# For PR review, check for uncommitted changes
if ! git diff --quiet || ! git diff --cached --quiet; then
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review."
echo "Please commit or stash your changes before running the API review."
git status --porcelain
exit 1
fi
echo "✅ No uncommitted changes detected. Safe to proceed with PR review."
else
REVIEW_MODE="local"
echo "🔍 Local review mode: Reviewing local changes against upstream master"

# Find a remote pointing to openshift/api repository
OPENSHIFT_REMOTE=""
for remote in $(git remote); do
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "")
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then
OPENSHIFT_REMOTE="$remote"
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url"
break
fi
done

# If no existing remote found, add upstream
if [ -z "$OPENSHIFT_REMOTE" ]; then
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..."
git remote add upstream https://github.com/openshift/api.git
OPENSHIFT_REMOTE="upstream"
fi

# Fetch latest changes from the OpenShift API remote
echo "🔄 Fetching latest changes from $OPENSHIFT_REMOTE..."
git fetch "$OPENSHIFT_REMOTE" master
fi
```
### Validation Requirements (Section 2: Best Practices)
- Follow validation decision hierarchy (2.1 Validation Decision Hierarchy)
- Prohibited format markers not used (7.1.0 Security: Prohibited Format Markers)
- Cross-field validation placed on parent structs (3.8 Cross-Field Validation Placement)
- Validation messages use exact enum values and JSON field names (3.9 Inaccurate or Inconsistent Validation Messages)

## Step 2: Get changed files based on review mode
## Step 3: Comprehensive Test Coverage Analysis

```bash
if [ "$REVIEW_MODE" = "pr" ]; then
# PR Review: Checkout the PR and get changed files
echo "🔄 Checking out PR #$PR_NUMBER..."
gh pr checkout "$PR_NUMBER"
### Process Reference:

echo "📁 Analyzing changed files in PR..."
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
else
# Local Review: Get changed files compared to openshift remote master
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..."
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')

# Also include staged changes
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
if [ -n "$STAGED_FILES" ]; then
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u)
fi
fi
**CRITICAL:** Execute this for *all* validation markers, not just those with other documentation issues.

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

if [ -z "$CHANGED_FILES" ]; then
echo "ℹ️ No API files changed. Nothing to review."
if [ "$REVIEW_MODE" = "pr" ]; then
git checkout "$CURRENT_BRANCH"
fi
exit 0
fi
```
## Step 4: Generate comprehensive review report

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

```bash
echo "⏳ Running linting checks on changes..."
make lint

if [ $? -ne 0 ]; then
echo "❌ Linting checks failed. Please fix the issues before proceeding."
if [ "$REVIEW_MODE" = "pr" ]; then
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
git checkout "$CURRENT_BRANCH"
fi
exit 1
fi
**CRITICAL:** You MUST use this EXACT format for documentation issues, validation problems, and lint errors (but not test coverage):

echo "✅ Linting checks passed."
+LineNumber: Brief description
**Current (problematic) code:**
```go
[exact code from the PR diff]
```

## Step 4: Documentation validation

For each changed API file, I'll validate:

1. **Field Documentation**: All struct fields must have documentation comments
2. **Optional Field Behavior**: Optional fields must explain what happens when they are omitted
3. **Validation Documentation**: Validation rules must be documented and match markers

Let me check each changed file for these requirements:

```thinking
I need to analyze the changed files to:
1. Find struct fields without documentation
2. Find optional fields without behavior documentation
3. Find validation annotations without corresponding documentation

For each Go file, I'll:
- Look for struct field definitions
- Check if they have preceding comment documentation
- For optional fields (those with `+kubebuilder:validation:Optional` or `+optional`), verify behavior is explained
- For fields with validation annotations, ensure the validation is documented
**Suggested change:**
```diff
- [old code line]
+ [new code line]
```

## Step 5: Generate comprehensive review report
**Explanation:** [Why this change is needed]

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

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

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

## Step 5: Switch back to original branch (PR mode only)

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

```bash
cat > /tmp/api_review_step5.sh << 'STEP5_EOF'
#!/bin/bash
source /tmp/api_review_vars.sh

if [ "$REVIEW_MODE" = "pr" ]; then
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
git checkout "$CURRENT_BRANCH"
echo "✅ API review complete. Back on branch: $(git branch --show-current)"
else
echo "✅ Local API review complete."
fi
STEP5_EOF

bash /tmp/api_review_step5.sh
```

**CRITICAL WORKFLOW REQUIREMENTS:**
Expand Down
96 changes: 96 additions & 0 deletions .claude/commands/pre-flight-checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
#!/bin/bash

# Save current branch
CURRENT_BRANCH=$(git branch --show-current)
echo "📍 Current branch: $CURRENT_BRANCH"

# Check if an argument was provided
if [ $# -gt 0 ]; then
REVIEW_MODE="pr"

# We expect the argument to be either a PR number, or the URL of a PR,
# which will end in the PR number
PR_NUMBER=$(echo "$1" | grep -oE '[0-9]+$')
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER"

# For PR review, check for uncommitted changes
if ! git diff --quiet || ! git diff --cached --quiet; then
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review."
echo "Please commit or stash your changes before running the API review."
git status --porcelain
exit 1
fi
echo "✅ No uncommitted changes detected. Safe to proceed with PR review."

# PR Review: Checkout the PR and get changed files
echo "🔄 Checking out PR #$PR_NUMBER..."
gh pr checkout "$PR_NUMBER"

echo "📁 Analyzing changed files in PR..."
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
else
REVIEW_MODE="local"
echo "🔍 Local review mode: Reviewing local changes against upstream master"

# Find a remote pointing to openshift/api repository
OPENSHIFT_REMOTE=""
for remote in $(git remote); do
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "")
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then
OPENSHIFT_REMOTE="$remote"
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url"
break
fi
done

# If no existing remote found, add upstream
if [ -z "$OPENSHIFT_REMOTE" ]; then
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..."
git remote add upstream https://github.com/openshift/api.git 2>&1 || true
OPENSHIFT_REMOTE="upstream"
fi

# Local Review: Get changed files compared to openshift remote master
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..."
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)

# Also include staged changes
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
if [ -n "$STAGED_FILES" ]; then
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u)
fi
fi

echo "Changed API files:"
echo "$CHANGED_FILES"

if [ -z "$CHANGED_FILES" ]; then
echo "ℹ️ No API files changed. Nothing to review."
if [ "$REVIEW_MODE" = "pr" ]; then
git checkout "$CURRENT_BRANCH"
fi
exit 0
fi

# Count the files
FILE_COUNT=$(echo "$CHANGED_FILES" | wc -l)
echo ""
echo "📊 Total API files changed: $FILE_COUNT"

echo "⏳ Running linting checks on changes..."
make lint

if [ $? -ne 0 ]; then
echo "❌ Linting checks failed. Please fix the issues before proceeding."
if [ "$REVIEW_MODE" = "pr" ]; then
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
git checkout "$CURRENT_BRANCH"
fi
exit 1
fi

echo "✅ Linting checks passed."

# Export variables to restore the environment after review
echo "REVIEW_MODE=$REVIEW_MODE" > /tmp/api_review_vars.sh
echo "CURRENT_BRANCH=$CURRENT_BRANCH" >> /tmp/api_review_vars.sh
Loading