-
Notifications
You must be signed in to change notification settings - Fork 219
OTA-1862 Enable AI code review for test scripts #1316
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,219 @@ | ||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||
| # | ||||||||||||||
| # Helper script to run code review using Gemini | ||||||||||||||
| # | ||||||||||||||
| # Usage: | ||||||||||||||
| # ./hack/review.sh # Review current branch vs origin/main | ||||||||||||||
| # ./hack/review.sh --pr 1234 # Review specific PR | ||||||||||||||
| # ./hack/review.sh --files file1.go file2.go # Review specific files | ||||||||||||||
|
|
||||||||||||||
| # set -euo pipefail | ||||||||||||||
|
|
||||||||||||||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||||||||||||||
| REPO_ROOT="${SCRIPT_DIR}" | ||||||||||||||
|
|
||||||||||||||
| # Default values | ||||||||||||||
| PR_NUMBER="" | ||||||||||||||
| PR_REMOTE="${PR_REMOTE:-origin}" # Can be overridden via env var (e.g., PR_REMOTE=upstream) | ||||||||||||||
|
|
||||||||||||||
| # Determine the default branch from the remote | ||||||||||||||
| DEFAULT_BRANCH=$(git remote show "${PR_REMOTE}" 2>/dev/null | grep 'HEAD branch' | cut -d' ' -f5) | ||||||||||||||
| if [[ -z "$DEFAULT_BRANCH" ]]; then | ||||||||||||||
| # Fallback to main if remote is not configured or branch not found | ||||||||||||||
| DEFAULT_BRANCH="main" | ||||||||||||||
| fi | ||||||||||||||
| DIFF_RANGE="${PR_REMOTE}/${DEFAULT_BRANCH}...HEAD" | ||||||||||||||
|
|
||||||||||||||
| FILES=() | ||||||||||||||
|
|
||||||||||||||
| # Pre-parse arguments to handle --files | ||||||||||||||
| for arg in "$@"; do | ||||||||||||||
| case "$arg" in | ||||||||||||||
| -f|--files) | ||||||||||||||
| # When reviewing specific files, | ||||||||||||||
| # default to showing local changes against HEAD instead of a branch diff. | ||||||||||||||
| DIFF_RANGE="HEAD" | ||||||||||||||
| break | ||||||||||||||
| ;; | ||||||||||||||
| esac | ||||||||||||||
| done | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| function usage() { | ||||||||||||||
| cat <<EOF | ||||||||||||||
| Usage: $0 [OPTIONS] | ||||||||||||||
|
|
||||||||||||||
| Run automated code review using Gemini agent. | ||||||||||||||
|
|
||||||||||||||
| Options: | ||||||||||||||
| -p, --pr NUMBER Review a specific pull request number | ||||||||||||||
| -f, --files FILE... Review only specific files | ||||||||||||||
| -h, --help Show this help message | ||||||||||||||
|
|
||||||||||||||
| Environment Variables: | ||||||||||||||
| PR_REMOTE Git remote to use for fetching PRs (default: origin) | ||||||||||||||
| Set to 'upstream' if working from a fork | ||||||||||||||
|
|
||||||||||||||
| Examples: | ||||||||||||||
| $0 # Review current changes vs main | ||||||||||||||
| $0 --pr 1234 # Review PR #1234 (uses gh CLI if available) | ||||||||||||||
| PR_REMOTE=upstream $0 --pr 1234 # Review PR from upstream remote | ||||||||||||||
| $0 --files pkg/cvo/cvo.go # Review specific file | ||||||||||||||
|
|
||||||||||||||
| Notes: | ||||||||||||||
| - PR reviews work best with 'gh' CLI installed: https://cli.github.com/ | ||||||||||||||
| - Without 'gh', falls back to git fetch (requires proper remote configuration) | ||||||||||||||
| - When working from a fork, use PR_REMOTE=upstream to fetch from main repository | ||||||||||||||
| EOF | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| # Parse arguments | ||||||||||||||
| while [[ $# -gt 0 ]]; do | ||||||||||||||
| case $1 in | ||||||||||||||
| -p|--pr) | ||||||||||||||
| PR_NUMBER="$2" | ||||||||||||||
| shift 2 | ||||||||||||||
| ;; | ||||||||||||||
| -f|--files) | ||||||||||||||
| shift | ||||||||||||||
| # stip options until next flag or end of args | ||||||||||||||
| while [[ $# -gt 0 ]] && [[ ! $1 =~ ^- ]]; do | ||||||||||||||
| if [[ ! $1 =~ ^vendor/ ]]; then | ||||||||||||||
| FILES+=("$1") | ||||||||||||||
| fi | ||||||||||||||
| shift | ||||||||||||||
| done | ||||||||||||||
| ;; | ||||||||||||||
| -h|--help) | ||||||||||||||
| usage | ||||||||||||||
| exit 0 | ||||||||||||||
| ;; | ||||||||||||||
| *) | ||||||||||||||
| echo "Unknown option: $1" | ||||||||||||||
| usage | ||||||||||||||
| exit 1 | ||||||||||||||
| ;; | ||||||||||||||
| esac | ||||||||||||||
| done | ||||||||||||||
|
|
||||||||||||||
| cd "$REPO_ROOT" || exit 0 | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, check if the file exists and read it
wc -l hack/gemini_review.shRepository: openshift/cluster-version-operator Length of output: 103 🏁 Script executed: # Read the file around line 99 to see the context
cat -n hack/gemini_review.sh | sed -n '90,110p'Repository: openshift/cluster-version-operator Length of output: 765 🏁 Script executed: # Search for other similar error handling patterns in the script
rg 'exit' hack/gemini_review.sh -nRepository: openshift/cluster-version-operator Length of output: 166 🏁 Script executed: # Get broader context - check beginning and end of file to understand purpose
head -30 hack/gemini_review.shRepository: openshift/cluster-version-operator Length of output: 1005 🏁 Script executed: # Also check if there's any documentation or comments about this behavior
grep -A 5 -B 5 'REPO_ROOT' hack/gemini_review.sh | head -50Repository: openshift/cluster-version-operator Length of output: 561 Change the exit status to 1 when directory change fails. Line 99 exits with status 0 on 🛠️ Proposed fix-cd "$REPO_ROOT" || exit 0
+cd "$REPO_ROOT" || exit 1📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| # Get diff based on input | ||||||||||||||
| if [[ -n "$PR_NUMBER" ]]; then | ||||||||||||||
| PR_REF="pull/${PR_NUMBER}/head" | ||||||||||||||
| FETCH_SUCCESS="false" | ||||||||||||||
|
|
||||||||||||||
| # Find the correct remote that has the PR ref by trying all of them. | ||||||||||||||
| for remote in $(git remote); do | ||||||||||||||
| echo "Attempting to fetch PR #${PR_NUMBER} from remote '$remote'..." | ||||||||||||||
| if git fetch "$remote" "$PR_REF" 2>/dev/null; then | ||||||||||||||
| echo "Successfully fetched PR from remote '$remote'." | ||||||||||||||
| PR_REMOTE=$remote # Set the successful remote for the diff operation below | ||||||||||||||
| FETCH_SUCCESS="true" | ||||||||||||||
| break | ||||||||||||||
| fi | ||||||||||||||
| done | ||||||||||||||
|
|
||||||||||||||
| if [[ "$FETCH_SUCCESS" == "false" ]]; then | ||||||||||||||
| echo "" | ||||||||||||||
| echo "Error: Could not fetch PR #${PR_NUMBER} from any of your configured remotes." | ||||||||||||||
| echo "Attempted remotes: $(git remote | tr '\n' ' ')" | ||||||||||||||
| echo "" | ||||||||||||||
| echo "This can happen if:" | ||||||||||||||
| echo " - You are working on a fork and have not configured a remote for the upstream repository." | ||||||||||||||
| 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. Add the main repository as a remote. e.g.:" | ||||||||||||||
| echo " git remote add upstream https://github.com/openshift/cluster-version-operator.git" | ||||||||||||||
| echo " 2. Manually fetch the PR into a local branch." | ||||||||||||||
| exit 1 | ||||||||||||||
| fi | ||||||||||||||
|
|
||||||||||||||
| # Determine the base branch for the diff | ||||||||||||||
| echo "Determining base branch for comparison..." | ||||||||||||||
| # First, update the remote's HEAD branch information locally | ||||||||||||||
| git remote set-head "$PR_REMOTE" --auto &>/dev/null | ||||||||||||||
|
|
||||||||||||||
| # Now, get the HEAD branch name from the updated remote info | ||||||||||||||
| BASE_BRANCH=$(git remote show "$PR_REMOTE" 2>/dev/null | grep 'HEAD branch' | cut -d' ' -f5) | ||||||||||||||
| # Fallback to master for this specific repository if lookup fails | ||||||||||||||
| BASE_BRANCH=${BASE_BRANCH:-master} | ||||||||||||||
| echo "Base branch identified as: ${PR_REMOTE}/${BASE_BRANCH}" | ||||||||||||||
|
|
||||||||||||||
| DIFF=$(git diff "${PR_REMOTE}/${BASE_BRANCH}...FETCH_HEAD" -- . ':(exclude)vendor/*') | ||||||||||||||
| FILES_CHANGED=$(git diff --name-only "${PR_REMOTE}/${BASE_BRANCH}...FETCH_HEAD" -- . ':(exclude)vendor/*') | ||||||||||||||
|
|
||||||||||||||
| echo "Successfully generated diff for PR #${PR_NUMBER}." | ||||||||||||||
|
|
||||||||||||||
| elif [[ ${#FILES[@]} -gt 0 ]]; then | ||||||||||||||
| echo "Reviewing specified files: ${FILES[*]}" | ||||||||||||||
| DIFF=$(git diff "$DIFF_RANGE" -- "${FILES[@]}") | ||||||||||||||
| FILES_CHANGED=$(printf '%s | ||||||||||||||
| ' "${FILES[@]}") | ||||||||||||||
| else | ||||||||||||||
| echo "Reviewing changes in range: $DIFF_RANGE" | ||||||||||||||
| DIFF=$(git diff "$DIFF_RANGE" -- . ':(exclude)vendor/*') | ||||||||||||||
| FILES_CHANGED=$(git diff --name-only "$DIFF_RANGE" -- . ':(exclude)vendor/*') | ||||||||||||||
| 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 <<REVIEW_PROMPT_EOF | ||||||||||||||
| Please perform a code review for the following changes in the ${REPO_NAME} repository. | ||||||||||||||
|
|
||||||||||||||
| As a reviewer, you must follow these instructions: | ||||||||||||||
| 1. **Analyze the provided git diff.** The user has provided the diff of the changes. | ||||||||||||||
| 2. **Apply Go best practices.** Refer to "Effective Go" for guidance on idiomatic Go, including formatting, naming conventions, and. | ||||||||||||||
| 3. **Consider oc-specific conventions.** Since 'oc' is built on 'kubectl', ensure that kubectl compatibility is maintained, OpenShift-specific commands follow existing patterns, and CLI output is consistent. | ||||||||||||||
| 4. **Check for code quality issues.** Look for potential race conditions, resource leaks, and proper context propagation. | ||||||||||||||
| 5. **Verify documentation.** Ensure that exported functions and types have doc comments, and complex logic is explained. | ||||||||||||||
| 6. **Provide a structured summary of your findings.** Organize your feedback clearly. | ||||||||||||||
|
|
||||||||||||||
| Files changed: | ||||||||||||||
| ${FILES_CHANGED} | ||||||||||||||
|
|
||||||||||||||
| Git diff: | ||||||||||||||
| $(diff | ||||||||||||||
| ${DIFF} | ||||||||||||||
| ) | ||||||||||||||
|
Comment on lines
+189
to
+192
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, check if the file exists and read the relevant section
head -n 200 hack/gemini_review.sh | tail -n 30Repository: openshift/cluster-version-operator Length of output: 1413 🏁 Script executed: # Search for where DIFF variable is defined/assigned
rg "DIFF\s*=" hack/gemini_review.sh -nRepository: openshift/cluster-version-operator Length of output: 286 🏁 Script executed: # Let me see the full context around lines 145-192 to understand the flow
sed -n '145,195p' hack/gemini_review.sh | cat -nRepository: openshift/cluster-version-operator Length of output: 2468 🏁 Script executed: # Check if there are any tests or usage patterns that show how REVIEW_PROMPT is used
rg "REVIEW_PROMPT" hack/gemini_review.sh -A 3 -B 3Repository: openshift/cluster-version-operator Length of output: 639 🏁 Script executed: # Let me verify what happens when you try to run diff with multiline content as argument
# Simulate the DIFF content and what $(diff ${DIFF}) would do
python3 << 'EOF'
# Simulate DIFF variable containing actual git diff output
DIFF_CONTENT = """--- a/test.go
+++ b/test.go
@@ -1,3 +1,4 @@
package main
func main() {
+ // new line
fmt.Println("hello")
"""
# What happens when you try diff with this as arguments?
import subprocess
import shlex
# This simulates what bash does when expanding $(diff ${DIFF})
cmd = f"diff {DIFF_CONTENT}"
print("Command that would be executed:")
print(repr(cmd))
print("\nTrying to parse this as shell command would result in:")
print("diff --- a/test.go +++ b/test.go @@ -1,3 +1,4 @@ ...")
print("\nThe 'diff' command expects file paths, not diff content.")
print("This would fail with 'diff: ---: No such file or directory'")
EOFRepository: openshift/cluster-version-operator Length of output: 470 🏁 Script executed: # Verify by checking the actual behavior - see what the script does after creating REVIEW_PROMPT
sed -n '190,210p' hack/gemini_review.sh | cat -nRepository: openshift/cluster-version-operator Length of output: 668
Fix Git diff:
-$(diff
-${DIFF}
-)
+${DIFF}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| Provide your code review based on the instructions above. | ||||||||||||||
| REVIEW_PROMPT_EOF | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| # Save prompt to temp file for gemini CLI | ||||||||||||||
| PROMPT_FILE=$(mktemp) | ||||||||||||||
| echo "$REVIEW_PROMPT" > "$PROMPT_FILE" | ||||||||||||||
|
|
||||||||||||||
| echo "=== CODE REVIEW RESULTS ===" | ||||||||||||||
| echo "" | ||||||||||||||
|
|
||||||||||||||
| # Check if gemini CLI is available | ||||||||||||||
| if command -v gemini &> /dev/null; then | ||||||||||||||
| # Use Gemini CLI if available | ||||||||||||||
| gemini -y "$(cat "$PROMPT_FILE")" | ||||||||||||||
| else | ||||||||||||||
| # Fallback: Print instructions for manual review | ||||||||||||||
| echo "gemini CLI not found." | ||||||||||||||
| echo "" | ||||||||||||||
| echo "To perform the review, copy the following prompt to Gemini:" | ||||||||||||||
| echo "---------------------------------------------------------------" | ||||||||||||||
| cat "$PROMPT_FILE" | ||||||||||||||
| echo "---------------------------------------------------------------" | ||||||||||||||
| fi | ||||||||||||||
|
|
||||||||||||||
| rm -f "$PROMPT_FILE" | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 1073
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 1098
🏁 Script executed:
sed -n '100,125p' hack/gemini_review.shRepository: openshift/cluster-version-operator
Length of output: 1109
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 107
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 129
🏁 Script executed:
Repository: openshift/cluster-version-operator
Length of output: 500
Validate
--prargument before consuming$2.The
--prflag lacks validation and can accept the next flag as a value. If--pris passed without a PR number (e.g.,--pr --files foo.go),PR_NUMBERgets assigned--files, resulting in invalid git operations likegit fetch origin pull/--files/head. This causes misleading error messages and silent failures.🐛 Proposed fix
🤖 Prompt for AI Agents