Skip to content

OTA-1862 Enable AI code review for test scripts#1316

Open
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
JianLi-RH:test_code_review
Open

OTA-1862 Enable AI code review for test scripts#1316
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
JianLi-RH:test_code_review

Conversation

@JianLi-RH
Copy link
Contributor

@JianLi-RH JianLi-RH commented Feb 12, 2026

https://issues.redhat.com/browse/OTA-1862

How to use:

Usage: ./test/review.sh [OPTIONS]

Run automated code review using Claude Code agent.

Options:
  --pr NUMBER         Review a specific pull request number
  --range RANGE       Git diff range (default: origin/main...HEAD)
  --files FILE...     Review only specific files
  --output FILE       Output file for review results (default: test/review-output.json)
  -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:
  ./test/review.sh                              # Review current changes vs main
  ./test/review.sh --pr 1234                    # Review PR #1234 (uses gh CLI if available)
  PR_REMOTE=upstream ./test/review.sh --pr 1234 # Review PR from upstream remote
  ./test/review.sh --range HEAD~5..HEAD         # Review last 5 commits
  ./test/review.sh --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
 

Summary by CodeRabbit

  • New Features

    • Add an automated code review command that can analyze PRs, specific files, or custom diff ranges and emit structured review output.
  • Documentation

    • Extensive user guides, examples, integration notes, troubleshooting, and configuration docs for setup, usage, CI integration, and best practices for the review workflow.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

Walkthrough

Adds an automated code-review workflow: new Makefile target, a Bash review script (hack/review.sh) retrieving diffs via gh/git and invoking Claude CLI, a JSON agent config, and multiple docs/examples explaining usage and integration.

Changes

Cohort / File(s) Summary
Build / entrypoint
Makefile, AGENTS.md
New review-changes Makefile target invoking hack/review.sh; added .PHONY: review-changes and documentation references in AGENTS.md.
Review runner script
hack/review.sh
New Bash script implementing PR-/range-/files-based diff collection, gh CLI primary with git fallback, prompt assembly, Claude CLI invocation (JSON output), optional output file, and temp-file cleanup.
Agent config
test/code-review-agent.json
New JSON configuration defining the automated review agent prompt, CVO-focused checklist, output JSON schema, tooling options, and limits.
Documentation & examples
test/CODE_REVIEW_AGENT.md, test/README.md, test/REVIEW_SCRIPT_UPDATES.md, test/example-review-usage.md
New comprehensive docs covering agent overview, quick-starts, hybrid gh/git fetch strategy, usage scenarios (many examples), CI integration, troubleshooting, and changelog/details for review.sh.
Misc test files
test/review.sh, test/*
Added small helper/test files and usage examples referenced by the docs (new test scripts and guidance files).

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant Makefile
    participant Review as hack/review.sh
    participant CLI as gh/git
    participant Claude as Claude CLI
    participant FS as FileSystem

    User->>Makefile: make review-changes [options]
    Makefile->>Review: execute with args

    alt PR-based
        Review->>CLI: request PR diff (gh pr diff)
        alt gh available
            CLI-->>Review: PR diff
        else
            Review->>CLI: git fetch PR_REMOTE && git diff
            CLI-->>Review: PR diff
        end
    else File(s)-based
        Review->>CLI: git diff -- <files>
        CLI-->>Review: diff
    else Range-based
        Review->>CLI: git diff <range>
        CLI-->>Review: diff
    end

    Review->>FS: collect changed file list
    Review->>FS: write temp prompt (repo, files, diff)
    Review->>Claude: send prompt -> request JSON review
    Claude-->>Review: return JSON review
    Review->>FS: write output file / stdout
    Review->>FS: cleanup temp files
    Review-->>User: display summarized review
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: enabling AI code review capabilities for test scripts through the addition of automation tooling.
Stable And Deterministic Test Names ✅ Passed The PR adds documentation and tooling for a code review agent. Test code examples demonstrate proper test naming conventions with static, deterministic titles containing no dynamic values, generated suffixes, timestamps, or identifiers.
Test Structure And Quality ✅ Passed PR does not add or modify Golang test files; introduces bash script, documentation, and configuration files only.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Shellcheck (0.11.0)
test/review.sh

test/review.sh: test/review.sh: openBinaryFile: does not exist (No such file or directory)
{"comments":[]}


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 12, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JianLi-RH
Once this PR has been reviewed and has the lgtm label, please assign wking for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@JianLi-RH JianLi-RH force-pushed the test_code_review branch 3 times, most recently from b430587 to 6c08e73 Compare February 14, 2026 08:42
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 17

🧹 Nitpick comments (4)
hack/review.sh (3)

165-165: REPO_NAME always reads from the origin remote, ignoring PR_REMOTE.

When PR_REMOTE=upstream, the repository name in the prompt will reflect the fork's name (origin) rather than the upstream project.

♻️ Proposed fix
-REPO_NAME=$(git remote get-url origin | sed -e 's/.*[:/]\([^/]*\/[^/]*\)\.git$/\1/' -e 's/.*[:/]\([^/]*\/[^/]*\)$/\1/')
+REPO_NAME=$(git remote get-url "${PR_REMOTE}" 2>/dev/null || git remote get-url origin | sed -e 's/.*[:/]\([^/]*\/[^/]*\)\.git$/\1/' -e 's/.*[:/]\([^/]*\/[^/]*\)$/\1/')
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` at line 165, The REPO_NAME assignment always uses the
hardcoded "origin" remote, ignoring the PR_REMOTE variable; update the command
that sets REPO_NAME to use git remote get-url "$PR_REMOTE" (and optionally fall
back to origin if PR_REMOTE is empty or unset) so the prompt reflects the chosen
PR remote; keep the existing sed extraction logic but change the remote name in
the git command and ensure PR_REMOTE is referenced safely (e.g.,
"${PR_REMOTE:-origin}") where REPO_NAME is set.

6-9: Script header comments still reference ./test/review.sh (stale copy from the old location).

♻️ Proposed fix
-#   ./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
+#   ./hack/review.sh                    # Review current branch vs origin/main
+#   ./hack/review.sh --pr 1234          # Review specific PR
+#   ./hack/review.sh --range HEAD~3     # Review last 3 commits
+#   ./hack/review.sh --files file1.go file2.go  # Review specific files
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 6 - 9, Update the script header comments in
hack/review.sh to remove the stale references to ./test/review.sh and replace
them with the correct script path ./hack/review.sh (e.g., update the example
usages that currently show "./test/review.sh", "./test/review.sh --pr 1234",
"./test/review.sh --range HEAD~3", and "./test/review.sh --files file1.go
file2.go" so they reference "./hack/review.sh" instead); ensure the examples
remain accurate and any descriptive text in the header reflects the script’s
current location and accepted flags.

135-136: git remote show is a network-bound call with fragile output parsing.

git remote show "$PR_REMOTE" performs a live network round-trip every invocation and outputs human-readable text whose format can vary. cut -d' ' -f5 will silently produce a wrong branch name if the spacing differs (e.g., extra leading spaces). The main fallback on line 136 mitigates the risk, but consider the cheaper, purely-local alternative:

♻️ Proposed fix — local symbolic-ref lookup
-    BASE_BRANCH=$(git remote show "$PR_REMOTE" 2>/dev/null | grep 'HEAD branch' | cut -d' ' -f5)
-    BASE_BRANCH=${BASE_BRANCH:-main}
+    BASE_BRANCH=$(git symbolic-ref "refs/remotes/${PR_REMOTE}/HEAD" 2>/dev/null | sed "s|refs/remotes/${PR_REMOTE}/||")
+    BASE_BRANCH=${BASE_BRANCH:-main}

Run git remote set-head "$PR_REMOTE" --auto once to populate the symbolic ref locally.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 135 - 136, The current BASE_BRANCH assignment
uses a network-bound `git remote show` parse; instead, run `git remote set-head
"$PR_REMOTE" --auto` once (to populate the local symbolic ref) and then read the
HEAD locally with a symbolic-ref lookup to set BASE_BRANCH; replace the `git
remote show ... | cut` pipeline with a local lookup like `git symbolic-ref
--short refs/remotes/"$PR_REMOTE"/HEAD` (falling back to main) and keep
references to the existing variables PR_REMOTE and BASE_BRANCH so the change is
localized.
test/README.md (1)

141-144: GitHub Actions example pins to mutable version tags (@v3), not immutable SHAs.

actions/checkout@v3 and actions/upload-artifact@v3 can be silently updated under the same tag. For a workflow that runs external tooling against PR diffs this is a mild supply-chain risk.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/README.md` around lines 141 - 144, The workflow examples use mutable
tags 'actions/checkout@v3' and 'actions/upload-artifact@v3'; replace those with
the corresponding immutable commit SHAs (e.g., 'actions/checkout@<sha>' and
'actions/upload-artifact@<sha>') to avoid silent updates. Look up the current
stable SHA for each action on the GitHub Actions marketplace or the action
repository's releases, and update the strings in the README example so the
workflow references the exact commit SHA instead of '@v3'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 46: The comment for the make target review-changes is misleading because
review.sh defaults to running git diff origin/main...HEAD which reviews all
changed files, not just tests; update the AGENTS.md description of the make
target (mentioning 'review-changes' and 'hack/review.sh') to state that it runs
the review script against all changed files in the branch (or explicitly mention
the git diff range) instead of saying "Code review for the test files."

In `@hack/review.sh`:
- Line 21: The default OUTPUT_FILE currently uses SCRIPT_DIR
(OUTPUT_FILE="${SCRIPT_DIR}/review-output.json") which writes into the script
directory but the usage string advertises test/review-output.json; fix by making
the default and the usage consistent — either update OUTPUT_FILE to point at the
repo-root test path (e.g. use a REPO_ROOT variable and set
OUTPUT_FILE="${REPO_ROOT}/test/review-output.json") or change the usage string
to say hack/review-output.json; ensure you modify the OUTPUT_FILE assignment and
the usage message together so they match, referencing the OUTPUT_FILE and
SCRIPT_DIR occurrences and the usage text in the script.
- Around line 167-180: The heredoc used to build REVIEW_PROMPT (cat <<EOF ...
EOF) can be truncated if ${DIFF} contains a line that's exactly EOF; change the
construction so the delimiter is quoted and collision-resistant and ensure
variables are still expanded by pre-populating dynamic values: for example,
assemble FILES_CHANGED, REPO_NAME and DIFF into local variables first (or use
printf to build the string), then use a quoted heredoc delimiter (e.g.,
<<'HEREDOC') to prevent accidental termination, or alternatively pick a unique
delimiter name unlikely to appear in diffs and keep variable expansion behavior
consistent with how REVIEW_PROMPT is currently populated.
- Around line 97-105: The gh invocations are redirecting stderr into the
captured variables (DIFF and FILES_CHANGED) via "2>&1", which mixes warnings
into the diff; update the error handling around the gh calls (the DIFF
assignment using gh pr diff and the FILES_CHANGED assignment using gh pr view)
to stop merging stderr into the content—either remove "2>&1" so stderr prints to
the terminal, or capture stderr separately into a temp file/variable (e.g.,
STDERR_DIFF/STDERR_FILES) and only assign stdout to DIFF/FILES_CHANGED; on
failure, include the separate stderr contents in the error messages so you still
surface gh errors without corrupting the diff/file list.
- Around line 118-131: The error message and suggested commands hardcode
'origin' instead of using the PR_REMOTE variable; update the echo text and
example commands in hack/review.sh to interpolate ${PR_REMOTE} (and keep
${PR_NUMBER}) so the lines referencing "from remote 'origin'", "git fetch origin
pull/${PR_NUMBER}/head:pr-${PR_NUMBER}", and the git range example
"origin/main...HEAD" use ${PR_REMOTE} (e.g., "${PR_REMOTE}/main...HEAD") so the
printed guidance matches the actual PR_REMOTE value.
- Around line 183-217: The script creates a temp file via PROMPT_FILE=$(mktemp)
but only removes it at the normal exit path (rm -f "$PROMPT_FILE"), which leaks
the file on early exits (e.g., when the claude command fails); add a trap right
after PROMPT_FILE is created to always remove the file on EXIT (and on INT/TERM
if desired) so cleanup runs unconditionally, then remove the trailing explicit
rm -f "$PROMPT_FILE" to avoid double-removal; update any references around the
claude task invocation and jq printing blocks to rely on the trap rather than
manual cleanup.

In `@test/CODE_REVIEW_AGENT.md`:
- Around line 19-21: Update the incorrect script path: replace references to
"review.sh" in the file table and all examples with the actual location
"hack/review.sh"; specifically edit the "Files in This Directory" list entry
that mentions review.sh and update every Quick Start example block and the
GitHub Actions example that reference test/review.sh so they point to
hack/review.sh (search for the literal "review.sh" and change to
"hack/review.sh" inside CODE_REVIEW_AGENT.md).

In `@test/code-review-agent.json`:
- Line 7: The configuration currently includes the "Bash" entry in the "tools"
list which grants arbitrary shell execution; remove "Bash" from the "tools"
array (leave ["Read","Grep","Glob"]) unless you can justify and document why
shell access is required, or gate it behind an explicit opt-in (e.g., an
environment flag like ENABLE_BASH or a secure runtime-only toggle) and add a
comment explaining the security risk and intended scope; target the "tools" JSON
key and the "Bash" value when making this change.
- Around line 1-9: The script hack/review.sh calls a non-existent CLI invocation
"claude task --type general-purpose" and points at an unused JSON agent file
test/code-review-agent.json; fix by either converting the JSON agent into a
Claude Code agent file under .claude/agents/ with YAML frontmatter (e.g.,
.claude/agents/code-review.md) and updating hack/review.sh to load that file, or
change the script to use a documented claude invocation (e.g., replace the
"claude task --type general-purpose ..." call with a supported form like claude
-p "$(cat .claude/agents/code-review.md)" or claude -f
.claude/agents/code-review.md); update/remove test/code-review-agent.json
accordingly and ensure the script references the new .claude/agents filename
instead of the JSON and the invalid "task --type" syntax.

In `@test/example-review-usage.md`:
- Around line 87-93: The pre-push hook example has a misleading comment and dead
variables: the comment "# Only run for main branch" contradicts the condition in
CURRENT_BRANCH check (if [[ "$CURRENT_BRANCH" != "main" ]]) which actually runs
the review on non-main branches, and REMOTE and URL are captured but never used;
update the comment to accurately reflect the behavior (e.g., "# Skip running on
main branch" or invert the condition to == "main" if you intended to run only on
main), and remove the unused REMOTE and URL variables (or use them if the hook
needs them) so only CURRENT_BRANCH and the actual control flow remain in the
script.
- Around line 217-228: The script builds FINDINGS from review.json and always
calls gh pr comment "$PR_NUMBER" --body "...$FINDINGS..." which posts an empty
header/footer when FINDINGS is empty; modify the logic around FINDINGS and the
gh pr comment invocation so you check if FINDINGS is non-empty (or set a
fallback like "LGTM — no high/medium findings") before calling gh pr comment,
e.g. test the FINDINGS variable and only run gh pr comment "$PR_NUMBER" --body
"## 🤖 Automated Code Review\n\n$FINDINGS\n\n---\n_Generated by CVO code review
agent_" when FINDINGS contains content, otherwise post a concise approval
message or skip posting entirely.
- Around line 56-61: The subshell expansion passed to --files (e.g.,
MODIFIED_GO_FILES used with ./test/review.sh --files $MODIFIED_GO_FILES) can be
empty and cause unexpected behavior; capture the git diff output into a
variable, then guard it before calling review.sh by testing whether the variable
is empty and exiting or skipping the review when no files match; apply the same
fix to the other occurrences that build a modified-files variable and call
./test/review.sh --files (the same pattern repeated at the other locations).
- Around line 325-331: The README examples call an unsupported flag
(--diff-file) and the wrong script path; update the two examples that invoke
./test/review.sh to instead call ./hack/review.sh and replace --diff-file usage
with a supported flag (e.g., --range or --files) that passes the diff to
review.sh (supported flags: --pr, --range, --files, --output, -h/--help), and
also fix the unquoted variable in the second example by adding quotes around the
variable expansion so shell word-splitting is prevented.

In `@test/README.md`:
- Around line 20-23: Update all README references that point to ./test/review.sh
to instead reference the real script at ./hack/review.sh; search for occurrences
of "./test/review.sh" in this file and replace them with "./hack/review.sh" (or
alternatively fix the non-functional wrapper in test/review.sh so the original
references work), ensuring examples like "./test/review.sh --pr 1273" become
"./hack/review.sh --pr 1273".

In `@test/REVIEW_SCRIPT_UPDATES.md`:
- Line 5: Update the relative link in test/REVIEW_SCRIPT_UPDATES.md so it points
to the actual script in hack rather than the file-local test path: change the
Markdown link target `[review.sh](review.sh)` to
`[review.sh](../hack/review.sh)` so all prose referencing hack/review.sh
resolves correctly.
- Line 60: Four fenced code blocks reported by the linter are missing language
identifiers; update each fenced block (the ones pointed out in the review) to
add an appropriate language hint such as ```text for plain output or ```bash for
shell snippets so markdownlint MD040 is satisfied. Locate the fenced blocks in
REVIEW_SCRIPT_UPDATES.md and replace the opening ``` with the correct marker
(e.g., ```text or ```bash) for each occurrence.

In `@test/review.sh`:
- Line 1: The wrapper script "hack/review.sh" is non-functional because it lacks
a shebang and does not forward arguments; update the script to include an
appropriate shebang (e.g., /usr/bin/env bash) at the top and ensure it forwards
all received CLI args to the real review script/command by appending "$@" (or
"$@"-style forwarding) so flags like --pr and --range are preserved when
invoking the underlying hack/review.sh logic.

---

Nitpick comments:
In `@hack/review.sh`:
- Line 165: The REPO_NAME assignment always uses the hardcoded "origin" remote,
ignoring the PR_REMOTE variable; update the command that sets REPO_NAME to use
git remote get-url "$PR_REMOTE" (and optionally fall back to origin if PR_REMOTE
is empty or unset) so the prompt reflects the chosen PR remote; keep the
existing sed extraction logic but change the remote name in the git command and
ensure PR_REMOTE is referenced safely (e.g., "${PR_REMOTE:-origin}") where
REPO_NAME is set.
- Around line 6-9: Update the script header comments in hack/review.sh to remove
the stale references to ./test/review.sh and replace them with the correct
script path ./hack/review.sh (e.g., update the example usages that currently
show "./test/review.sh", "./test/review.sh --pr 1234", "./test/review.sh --range
HEAD~3", and "./test/review.sh --files file1.go file2.go" so they reference
"./hack/review.sh" instead); ensure the examples remain accurate and any
descriptive text in the header reflects the script’s current location and
accepted flags.
- Around line 135-136: The current BASE_BRANCH assignment uses a network-bound
`git remote show` parse; instead, run `git remote set-head "$PR_REMOTE" --auto`
once (to populate the local symbolic ref) and then read the HEAD locally with a
symbolic-ref lookup to set BASE_BRANCH; replace the `git remote show ... | cut`
pipeline with a local lookup like `git symbolic-ref --short
refs/remotes/"$PR_REMOTE"/HEAD` (falling back to main) and keep references to
the existing variables PR_REMOTE and BASE_BRANCH so the change is localized.

In `@test/README.md`:
- Around line 141-144: The workflow examples use mutable tags
'actions/checkout@v3' and 'actions/upload-artifact@v3'; replace those with the
corresponding immutable commit SHAs (e.g., 'actions/checkout@<sha>' and
'actions/upload-artifact@<sha>') to avoid silent updates. Look up the current
stable SHA for each action on the GitHub Actions marketplace or the action
repository's releases, and update the strings in the README example so the
workflow references the exact commit SHA instead of '@v3'.

ℹ️ Review info

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9a401d5 and 5d41de8.

📒 Files selected for processing (9)
  • AGENTS.md
  • Makefile
  • hack/review.sh
  • test/CODE_REVIEW_AGENT.md
  • test/README.md
  • test/REVIEW_SCRIPT_UPDATES.md
  • test/code-review-agent.json
  • test/example-review-usage.md
  • test/review.sh

AGENTS.md Outdated
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
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

Misleading comment — review-changes is not limited to test files.

hack/review.sh defaults to git diff origin/main...HEAD which covers all changed files in the branch, not only test/. The comment "Code review for the test files" will confuse users who want to review non-test changes.

♻️ Proposed fix
-make review-changes     # Code review for the test files
+make review-changes     # AI-assisted code review for current branch changes
📝 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
make review-changes # Code review for the test files
make review-changes # AI-assisted code review for current branch changes
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 46, The comment for the make target review-changes is
misleading because review.sh defaults to running git diff origin/main...HEAD
which reviews all changed files, not just tests; update the AGENTS.md
description of the make target (mentioning 'review-changes' and
'hack/review.sh') to state that it runs the review script against all changed
files in the branch (or explicitly mention the git diff range) instead of saying
"Code review for the test files."

Comment on lines 118 to 131
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
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

Error message hardcodes 'origin' instead of ${PR_REMOTE}.

When a user sets PR_REMOTE=upstream, the failure message still says "from remote 'origin'" and the suggested fix in the message also says origin, which is misleading.

🐛 Proposed fix
-      echo "Error: Could not fetch PR #${PR_NUMBER} from remote '${PR_REMOTE}'"
+      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 "  - Your '${PR_REMOTE}' 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"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 118 - 131, The error message and suggested
commands hardcode 'origin' instead of using the PR_REMOTE variable; update the
echo text and example commands in hack/review.sh to interpolate ${PR_REMOTE}
(and keep ${PR_NUMBER}) so the lines referencing "from remote 'origin'", "git
fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}", and the git range example
"origin/main...HEAD" use ${PR_REMOTE} (e.g., "${PR_REMOTE}/main...HEAD") so the
printed guidance matches the actual PR_REMOTE value.

hack/review.sh Outdated
Comment on lines 167 to 180
REVIEW_PROMPT=$(cat <<EOF
Please perform a code review for the following changes in the ${REPO_NAME} repository.

Files changed:
${FILES_CHANGED}

Git diff:
\`\`\`diff
${DIFF}
\`\`\`

Analyze these changes according to the code review checklist and provide your findings in JSON format as specified in your instructions.
EOF
)
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

Heredoc terminated early if the diff contains a line that is exactly EOF.

The review prompt is built with cat <<EOF ... EOF. If the captured $DIFF contains a line consisting solely of EOF, the shell heredoc terminates at that point, silently truncating REVIEW_PROMPT. Use a quoted delimiter to suppress all expansion and avoid collisions:

🐛 Proposed fix — use a collision-resistant quoted delimiter
-REVIEW_PROMPT=$(cat <<EOF
+REVIEW_PROMPT=$(cat <<'REVIEW_PROMPT_EOF'
 Please perform a code review for the following changes in the ${REPO_NAME} repository.
 ...
-EOF
+REVIEW_PROMPT_EOF
 )

Note: switching to a quoted heredoc (<<'...') suppresses $VAR expansion inside the body. Embed the dynamic values before the heredoc or use printf to construct the prompt instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 167 - 180, The heredoc used to build
REVIEW_PROMPT (cat <<EOF ... EOF) can be truncated if ${DIFF} contains a line
that's exactly EOF; change the construction so the delimiter is quoted and
collision-resistant and ensure variables are still expanded by pre-populating
dynamic values: for example, assemble FILES_CHANGED, REPO_NAME and DIFF into
local variables first (or use printf to build the string), then use a quoted
heredoc delimiter (e.g., <<'HEREDOC') to prevent accidental termination, or
alternatively pick a unique delimiter name unlikely to appear in diffs and keep
variable expansion behavior consistent with how REVIEW_PROMPT is currently
populated.

Comment on lines +20 to +23
./test/review.sh

# Review a specific PR
./test/review.sh --pr 1273
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

All ./test/review.sh references throughout this file point to the wrong location.

The script lives at hack/review.sh. With the wrapper stub in test/review.sh currently non-functional (see that file's review comment), users copy-pasting any of these examples will get an error. Update all occurrences to ./hack/review.sh (or fix the wrapper first).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/README.md` around lines 20 - 23, Update all README references that point
to ./test/review.sh to instead reference the real script at ./hack/review.sh;
search for occurrences of "./test/review.sh" in this file and replace them with
"./hack/review.sh" (or alternatively fix the non-functional wrapper in
test/review.sh so the original references work), ensuring examples like
"./test/review.sh --pr 1273" become "./hack/review.sh --pr 1273".


## Summary

Updated [review.sh](review.sh) to use a **hybrid approach** for PR reviews:
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

Relative link [review.sh](review.sh) resolves to test/review.sh, not the actual script at hack/review.sh.

All prose in this document describes hack/review.sh. The link target should be ../hack/review.sh.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/REVIEW_SCRIPT_UPDATES.md` at line 5, Update the relative link in
test/REVIEW_SCRIPT_UPDATES.md so it points to the actual script in hack rather
than the file-local test path: change the Markdown link target
`[review.sh](review.sh)` to `[review.sh](../hack/review.sh)` so all prose
referencing hack/review.sh resolves correctly.

```

**Output:**
```
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

Fenced code blocks at lines 60, 78, 97, and 109 are missing language identifiers.

markdownlint (MD040) flags these. Add text or bash as appropriate.

♻️ Example fix (apply to all four occurrences)
-```
+```text
 Fetching PR `#1273`...
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 60-60: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/REVIEW_SCRIPT_UPDATES.md` at line 60, Four fenced code blocks reported
by the linter are missing language identifiers; update each fenced block (the
ones pointed out in the review) to add an appropriate language hint such as
```text for plain output or ```bash for shell snippets so markdownlint MD040 is
satisfied. Locate the fenced blocks in REVIEW_SCRIPT_UPDATES.md and replace the
opening ``` with the correct marker (e.g., ```text or ```bash) for each
occurrence.

@JianLi-RH JianLi-RH changed the title [WIP] OTA-1862 Enable AI code review for test scripts OTA-1862 Enable AI code review for test scripts Feb 25, 2026
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2026
@JianLi-RH
Copy link
Contributor Author

JianLi-RH commented Feb 25, 2026

/cc @hongkailiu @jhou1

If you'd like, could you please help review this PR?

I tested it locally, it really helpfull when I review a PR:

[jianl@jianl-thinkpadt14gen4 cluster-version-operator]$ sh hack/review.sh --files test/cvo/cvo.go test/util/util.go
Reviewing specified files: test/cvo/cvo.go test/util/util.go
Files changed:
test/cvo/cvo.go
test/util/util.go

Generating code review...

=== CODE REVIEW RESULTS ===

Claude Code CLI not found.

To perform the review, copy the following prompt to Claude Code:
---------------------------------------------------------------
Please perform a code review for the following changes in the JianLi-RH/cluster-version-operator repository.

Files changed:
test/cvo/cvo.go
test/util/util.go

Git diff:
``diff
diff --git a/test/cvo/cvo.go b/test/cvo/cvo.go
index 19ad0b59c..230383e80 100644
--- a/test/cvo/cvo.go
+++ b/test/cvo/cvo.go
@@ -24,8 +24,12 @@ var _ = g.Describe(`[Jira:"Cluster Version Operator"] cluster-version-operator-t
                o.Expect(true).To(o.BeTrue())
        })
 
+       g.It("should support passing serial tests [Serial]", func() {
+               o.Expect(true).To(o.BeTrue())
+       })
+
        g.It("can use oc to get the version information", func() {
-               ocClient, err := oc.NewOC(logger)
+               ocClient, err := oc.NewOC(ocapi.Options{Logger: logger})
                o.Expect(err).NotTo(o.HaveOccurred())
                o.Expect(ocClient).NotTo(o.BeNil())
 
``

Analyze these changes according to the code review checklist and provide your findings in JSON format as specified in your instructions.
---------------------------------------------------------------
[jianl@jianl-thinkpadt14gen4 cluster-version-operator]$ 

@openshift-ci openshift-ci bot requested review from hongkailiu and jhou1 February 25, 2026 10:10
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

♻️ Duplicate comments (5)
hack/review.sh (2)

185-219: Ensure temp prompt cleanup on all exits.

Line 185 creates a temp file, but cleanup at Line 219 is skipped on early failure paths. Add an EXIT trap immediately after creation.

🧹 Proposed fix
 PROMPT_FILE=$(mktemp)
+trap 'rm -f "$PROMPT_FILE"' EXIT
 echo "$REVIEW_PROMPT" > "$PROMPT_FILE"
@@
-rm -f "$PROMPT_FILE"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 185 - 219, The temp prompt file created in
PROMPT_FILE=$(mktemp) isn't guaranteed to be removed on early exits; immediately
after creating PROMPT_FILE add a trap to cleanup it on EXIT (e.g., trap 'rm -f
"$PROMPT_FILE"' EXIT) so the temporary file is always removed regardless of
which branch or error path runs; ensure the trap is set right after PROMPT_FILE
is assigned and before any commands that might exit, and keep the final rm -f
"$PROMPT_FILE" safe to run (idempotent).

131-132: Fallback guidance still hardcodes origin.

Line 131 and Line 132 still print origin in suggested commands, which can mislead users when PR_REMOTE is set to another remote.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 131 - 132, The fallback guidance echoes hardcode
"origin"; update those echo lines to use the PR_REMOTE variable instead (e.g.,
reference PR_REMOTE) so suggestions reflect the configured remote; change both
occurrences that print `git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}`
and `./hack/review.sh --range origin/main...HEAD` to substitute ${PR_REMOTE}
(and ensure proper quoting) so the printed commands match the runtime PR_REMOTE
value.
test/REVIEW_SCRIPT_UPDATES.md (2)

5-5: Script link target remains ambiguous with current tool location.

Line 5 still links to review.sh in the local directory; previous feedback requested pointing to the actual script path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/REVIEW_SCRIPT_UPDATES.md` at line 5, The README entry in
test/REVIEW_SCRIPT_UPDATES.md still links to "review.sh" in the local dir;
update that link to point to the actual script location instead (replace the
ambiguous "review.sh" text with the real relative repo path to the script, e.g.
the module or tools path where the script lives) so the link resolves correctly;
locate the reference to "review.sh" in test/REVIEW_SCRIPT_UPDATES.md and change
it to the canonical path used in the repo (the real script file name/path) so
reviewers can open the correct script.

60-123: Add language identifiers to fenced code blocks.

The fences starting at Line 60, Line 78, Line 97, and Line 109 are still untyped and can fail MD040 linting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/REVIEW_SCRIPT_UPDATES.md` around lines 60 - 123, Add language
identifiers to the untyped fenced code blocks in test/REVIEW_SCRIPT_UPDATES.md:
mark shell/script examples (e.g., the blocks starting with "./test/review.sh
--pr 1273" and "git remote add upstream ..." ) as bash or sh, and mark command
output blocks (e.g., blocks that begin with "Fetching PR `#1273`..." and the
"Error: Could not fetch PR `#1273`..." block) as console or text so the MD040
linter passes; update each triple-backtick fence accordingly (e.g., ```bash for
commands, ```console or ```text for terminal output).
test/code-review-agent.json (1)

7-7: Re-evaluate Bash tool exposure for this agent profile.

Line 7 still enables "Bash", which broadens execution risk for prompt-influenced reviews.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/code-review-agent.json` at line 7, The tools array in
test/code-review-agent.json exposes "Bash", which increases execution risk;
remove the "Bash" entry from the "tools" list (or replace it with a safer,
limited tool) so the agent no longer has direct bash execution capability;
locate the "tools" JSON array and delete or substitute the string "Bash" to
restrict the agent's execution surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@hack/review.sh`:
- Around line 57-75: The case branches handling flags (--pr, --range, --files,
--output) read positional arguments like $2 without validating they exist;
update each branch to first verify the expected argument is present (e.g., check
${2:-} is non-empty or $# -ge 2) before assigning to PR_NUMBER, DIFF_RANGE,
OUTPUT_FILE or entering the --files loop, and emit a clear usage/error and exit
when missing; also ensure the --files branch still handles multiple files but
stops on next -- prefixed arg without reading beyond available args.
- Line 194: The script currently invokes the unsupported command string 'claude
task --type general-purpose --prompt "@${PROMPT_FILE}" --output "$OUTPUT_FILE"';
replace this with the supported Claude CLI usage by passing the prompt via -p
(e.g., reading the prompt file into the -p argument) and request JSON output
with --output-format json, then write that output to "$OUTPUT_FILE" (e.g.,
redirect or supported output-file option) so the runtime uses a valid claude
invocation.

In `@test/code-review-agent.json`:
- Line 6: The embedded example in the "Output Format" contains invalid JSON at
the "line_or_range" entry ("42" or "42-51"); update that example so it is valid
JSON (e.g., pick one representation such as "line_or_range": "42-51" or use an
array like "line_or_range": ["42","42-51"]) and ensure the surrounding JSON
object remains syntactically correct; locate the example under the "Output
Format" block and edit the "line_or_range" field accordingly.

In `@test/README.md`:
- Around line 61-70: Add a language identifier to the directory-tree fenced code
block in test/README.md: change the opening triple-backtick for the file-tree
block to include a language hint (e.g., ```text) so markdownlint MD040 is
satisfied; update the block that begins with the test/ directory tree (the code
fence containing "test/ ├── cvo/ ...") to use the chosen language hint.

---

Duplicate comments:
In `@hack/review.sh`:
- Around line 185-219: The temp prompt file created in PROMPT_FILE=$(mktemp)
isn't guaranteed to be removed on early exits; immediately after creating
PROMPT_FILE add a trap to cleanup it on EXIT (e.g., trap 'rm -f "$PROMPT_FILE"'
EXIT) so the temporary file is always removed regardless of which branch or
error path runs; ensure the trap is set right after PROMPT_FILE is assigned and
before any commands that might exit, and keep the final rm -f "$PROMPT_FILE"
safe to run (idempotent).
- Around line 131-132: The fallback guidance echoes hardcode "origin"; update
those echo lines to use the PR_REMOTE variable instead (e.g., reference
PR_REMOTE) so suggestions reflect the configured remote; change both occurrences
that print `git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}` and
`./hack/review.sh --range origin/main...HEAD` to substitute ${PR_REMOTE} (and
ensure proper quoting) so the printed commands match the runtime PR_REMOTE
value.

In `@test/code-review-agent.json`:
- Line 7: The tools array in test/code-review-agent.json exposes "Bash", which
increases execution risk; remove the "Bash" entry from the "tools" list (or
replace it with a safer, limited tool) so the agent no longer has direct bash
execution capability; locate the "tools" JSON array and delete or substitute the
string "Bash" to restrict the agent's execution surface.

In `@test/REVIEW_SCRIPT_UPDATES.md`:
- Line 5: The README entry in test/REVIEW_SCRIPT_UPDATES.md still links to
"review.sh" in the local dir; update that link to point to the actual script
location instead (replace the ambiguous "review.sh" text with the real relative
repo path to the script, e.g. the module or tools path where the script lives)
so the link resolves correctly; locate the reference to "review.sh" in
test/REVIEW_SCRIPT_UPDATES.md and change it to the canonical path used in the
repo (the real script file name/path) so reviewers can open the correct script.
- Around line 60-123: Add language identifiers to the untyped fenced code blocks
in test/REVIEW_SCRIPT_UPDATES.md: mark shell/script examples (e.g., the blocks
starting with "./test/review.sh --pr 1273" and "git remote add upstream ..." )
as bash or sh, and mark command output blocks (e.g., blocks that begin with
"Fetching PR `#1273`..." and the "Error: Could not fetch PR `#1273`..." block) as
console or text so the MD040 linter passes; update each triple-backtick fence
accordingly (e.g., ```bash for commands, ```console or ```text for terminal
output).

ℹ️ Review info

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 5d41de8 and 9d69892.

📒 Files selected for processing (9)
  • AGENTS.md
  • Makefile
  • hack/review.sh
  • test/CODE_REVIEW_AGENT.md
  • test/README.md
  • test/REVIEW_SCRIPT_UPDATES.md
  • test/code-review-agent.json
  • test/example-review-usage.md
  • test/review.sh
✅ Files skipped from review due to trivial changes (1)
  • test/review.sh
🚧 Files skipped from review as they are similar to previous changes (3)
  • Makefile
  • test/example-review-usage.md
  • AGENTS.md

Comment on lines +57 to +75
--pr)
PR_NUMBER="$2"
shift 2
;;
--range)
DIFF_RANGE="$2"
shift 2
;;
--files)
shift
while [[ $# -gt 0 ]] && [[ ! $1 =~ ^-- ]]; do
FILES+=("$1")
shift
done
;;
--output)
OUTPUT_FILE="$2"
shift 2
;;
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

Validate option arguments before reading $2.

Line 58, Line 62, and Line 73 read $2 without checking presence. With set -u, calls like --pr (no value) exit abruptly instead of showing a clear usage error.

🐛 Proposed fix
 while [[ $# -gt 0 ]]; do
   case $1 in
     --pr)
+      if [[ $# -lt 2 || "$2" == --* ]]; then
+        echo "Error: --pr requires a PR number"
+        usage
+        exit 1
+      fi
       PR_NUMBER="$2"
       shift 2
       ;;
     --range)
+      if [[ $# -lt 2 || "$2" == --* ]]; then
+        echo "Error: --range requires a git range"
+        usage
+        exit 1
+      fi
       DIFF_RANGE="$2"
       shift 2
       ;;
@@
     --output)
+      if [[ $# -lt 2 || "$2" == --* ]]; then
+        echo "Error: --output requires a file path"
+        usage
+        exit 1
+      fi
       OUTPUT_FILE="$2"
       shift 2
       ;;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` around lines 57 - 75, The case branches handling flags (--pr,
--range, --files, --output) read positional arguments like $2 without validating
they exist; update each branch to first verify the expected argument is present
(e.g., check ${2:-} is non-empty or $# -ge 2) before assigning to PR_NUMBER,
DIFF_RANGE, OUTPUT_FILE or entering the --files loop, and emit a clear
usage/error and exit when missing; also ensure the --files branch still handles
multiple files but stops on next -- prefixed arg without reading beyond
available args.

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

🌐 Web query:

Claude CLI commands flags documentation 2026 "claude task" --type general-purpose --prompt --output

💡 Result:

In Claude Code’s official CLI, the flags you’re looking for are not claude task, --type, --prompt, or --output.

Official equivalents (Claude Code CLI)

  • “prompt” / one-shot mode: use -p / --print
  • “output” format: use --output-format with text | json | stream-json

Example:

claude -p "Explain this repo" --output-format json

Source: official CLI reference (commands + flags). [1]

About "general-purpose" / “task”

  • "general-purpose" appears as an agent/sub-agent type in Claude Code’s agent/task system, not as a claude task --type ... CLI subcommand. Community docs show it as subagent_type="general-purpose" in task/sub-agent contexts. [2]
  • The official CLI reference page lists commands like claude, claude -p, claude agents, etc., and does not document a claude task command. [1]

Sources
[1] Claude Code Docs – CLI reference (commands/flags, -p, --output-format, etc.) (docs.anthropic.com)
[2] Community-maintained Claude Code guide mentioning task/subagent type "general-purpose" (github.com)

Citations:


🏁 Script executed:

# Check if there's any documentation or comments in the repository about Claude CLI usage
fd -e md -e txt -e sh | xargs rg -i "claude.*task|claude.*command|CLI" -B 2 -A 2 | head -100

Repository: openshift/cluster-version-operator

Length of output: 8893


🏁 Script executed:

# Look for any setup, documentation, or config files that might reference the Claude CLI
find . -type f \( -name "README*" -o -name "SETUP*" -o -name "*.md" -o -name ".env*" -o -name "*config*" \) | head -20

Repository: openshift/cluster-version-operator

Length of output: 821


Unsupported Claude CLI syntax will cause runtime failure.

Line 194 uses claude task --type general-purpose --prompt "@file" --output output.json, which is not a valid command according to the official Claude Code CLI reference. The correct equivalent syntax is:

claude -p "prompt" --output-format json

The claude task subcommand and flags like --type, --prompt, and --output are not supported by the current CLI. This will fail at runtime when the script executes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/review.sh` at line 194, The script currently invokes the unsupported
command string 'claude task --type general-purpose --prompt "@${PROMPT_FILE}"
--output "$OUTPUT_FILE"'; replace this with the supported Claude CLI usage by
passing the prompt via -p (e.g., reading the prompt file into the -p argument)
and request JSON output with --output-format json, then write that output to
"$OUTPUT_FILE" (e.g., redirect or supported output-file option) so the runtime
uses a valid claude invocation.

"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.",
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

Fix invalid JSON in the embedded output schema example.

Line 6 includes "line_or_range": "42" or "42-51", which is not valid JSON. This can nudge the agent toward invalid output formatting.

♻️ Proposed fix
-      \"line_or_range\": \"42\" or \"42-51\",
+      \"line_or_range\": \"42-51\",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/code-review-agent.json` at line 6, The embedded example in the "Output
Format" contains invalid JSON at the "line_or_range" entry ("42" or "42-51");
update that example so it is valid JSON (e.g., pick one representation such as
"line_or_range": "42-51" or use an array like "line_or_range": ["42","42-51"])
and ensure the surrounding JSON object remains syntactically correct; locate the
example under the "Output Format" block and edit the "line_or_range" field
accordingly.

Comment on lines +61 to +70
```
test/
├── cvo/
│ └── cvo.go # Main CVO integration test suite
├── oc/
│ ├── api/ # Kubernetes API helpers
│ ├── cli/ # OpenShift CLI utilities
│ └── oc.go # oc command wrappers
└── ...
```
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

Add a language hint to the directory-tree code fence (MD040).

Line 61 opens an untyped fenced block; markdownlint requires a language identifier.

📝 Proposed fix
-```
+```text
 test/
 ├── cvo/
 │   └── cvo.go              # Main CVO integration test suite
 ├── oc/
 │   ├── api/                # Kubernetes API helpers
 │   ├── cli/                # OpenShift CLI utilities
 │   └── oc.go               # oc command wrappers
 └── ...
</details>

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>

[warning] 61-61: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @test/README.md around lines 61 - 70, Add a language identifier to the
directory-tree fenced code block in test/README.md: change the opening
triple-backtick for the file-tree block to include a language hint (e.g.,

the test/ directory tree (the code fence containing "test/ ├── cvo/ ...") to use
the chosen language hint.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 25, 2026

@JianLi-RH: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agnostic-ovn-upgrade-into-change 9d69892 link true /test e2e-agnostic-ovn-upgrade-into-change
ci/prow/e2e-hypershift-conformance 9d69892 link true /test e2e-hypershift-conformance
ci/prow/e2e-agnostic-ovn 9d69892 link true /test e2e-agnostic-ovn
ci/prow/e2e-hypershift 9d69892 link true /test e2e-hypershift
ci/prow/e2e-agnostic-ovn-techpreview-serial 9d69892 link true /test e2e-agnostic-ovn-techpreview-serial

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant