Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes
```

## Architecture
Expand Down
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
219 changes: 219 additions & 0 deletions hack/gemini_review.sh
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
;;
Comment on lines +73 to +76
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n hack/gemini_review.sh | sed -n '60,90p'

Repository: openshift/cluster-version-operator

Length of output: 1073


🏁 Script executed:

head -20 hack/gemini_review.sh && echo "---" && rg -n "PR_NUMBER" hack/gemini_review.sh

Repository: openshift/cluster-version-operator

Length of output: 1098


🏁 Script executed:

sed -n '100,125p' hack/gemini_review.sh

Repository: openshift/cluster-version-operator

Length of output: 1109


🏁 Script executed:

# Test shift behavior in bash
bash -c 'set +e; a=1; shift 2; echo "Exit code: $?"; echo "Script continues"'

Repository: openshift/cluster-version-operator

Length of output: 107


🏁 Script executed:

# Test what happens when shift has no arguments
bash -c 'args=("--pr" "--files"); set +e; shift 2 <<< ""; echo "Exit: $?"'
# More realistic test
bash -c 'set +e; set -- "--pr" "--files" "foo.go"; PR_NUMBER="$2"; shift 2; echo "PR_NUMBER=$PR_NUMBER"; echo "Remaining: $@"; echo "Exit: $?"'

Repository: openshift/cluster-version-operator

Length of output: 129


🏁 Script executed:

# Check if PR_NUMBER is validated before use
rg -A 5 "if.*PR_NUMBER" hack/gemini_review.sh | head -20

Repository: openshift/cluster-version-operator

Length of output: 500


Validate --pr argument before consuming $2.

The --pr flag lacks validation and can accept the next flag as a value. If --pr is passed without a PR number (e.g., --pr --files foo.go), PR_NUMBER gets assigned --files, resulting in invalid git operations like git fetch origin pull/--files/head. This causes misleading error messages and silent failures.

🐛 Proposed fix
     -p|--pr)
+      if [[ $# -lt 2 ]] || [[ "$2" =~ ^- ]]; then
+        echo "Error: --pr requires a pull request number" >&2
+        usage
+        exit 1
+      fi
       PR_NUMBER="$2"
       shift 2
       ;;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/gemini_review.sh` around lines 73 - 76, The -p|--pr) branch assigns
PR_NUMBER from $2 without validation; update that branch to first verify $2
exists and is a valid PR identifier (e.g., non-empty and numeric or at least
does not start with '-') before assigning PR_NUMBER and shift 2; if validation
fails print a clear error and exit (refer to PR_NUMBER and the -p|--pr) case
handling) so we never consume the next flag as the PR number.

-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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and read it
wc -l hack/gemini_review.sh

Repository: 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 -n

Repository: 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.sh

Repository: 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 -50

Repository: 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 cd failure, masking the error. Since subsequent operations depend on being in REPO_ROOT, the script should signal failure when the cd command fails.

🛠️ Proposed fix
-cd "$REPO_ROOT" || exit 0
+cd "$REPO_ROOT" || exit 1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cd "$REPO_ROOT" || exit 0
cd "$REPO_ROOT" || exit 1
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/gemini_review.sh` at line 99, The `cd "$REPO_ROOT" || exit 0` fallback
masks a failure by exiting with status 0; change the fallback to `exit 1` so the
script fails when the `cd` fails (locate the `cd "$REPO_ROOT" || exit 0` line in
hack/gemini_review.sh and replace the `exit 0` with `exit 1`), ensuring
subsequent steps don't run when the repo root change fails.


# 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and read the relevant section
head -n 200 hack/gemini_review.sh | tail -n 30

Repository: 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 -n

Repository: 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 -n

Repository: 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 3

Repository: 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'")
EOF

Repository: 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 -n

Repository: openshift/cluster-version-operator

Length of output: 668


REVIEW_PROMPT embeds the diff incorrectly (command substitution bug).

$(diff ${DIFF}) attempts to run the external diff command with diff content as arguments, but diff expects file paths. This causes an error, resulting in a corrupted or empty review payload sent to gemini.

Fix
 Git diff:
-$(diff
-${DIFF}
-)
+${DIFF}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Git diff:
$(diff
${DIFF}
)
Git diff:
${DIFF}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/gemini_review.sh` around lines 189 - 192, REVIEW_PROMPT is embedding the
diff via command substitution using $(diff ${DIFF}) which tries to execute diff
as a program; replace that with the actual diff content by expanding the
variable (e.g. use "${DIFF}" or printf '%s' "$DIFF") or embed it with a heredoc,
and update the code that builds REVIEW_PROMPT to use the expanded content
(reference REVIEW_PROMPT and the current $(diff ${DIFF}) spot) so the review
payload contains the diff text rather than attempting to run an external
command.


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"
Loading