OTA-1862 Enable AI code review for test scripts#1316
Open
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
Open
OTA-1862 Enable AI code review for test scripts#1316JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds an automated code-review workflow: a new Makefile target, bash review runners (Claude & Gemini variants) that fetch diffs via gh/git, a JSON review-agent config, and multiple documentation and example files describing usage and CI integration. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Makefile
participant Review as hack/review.sh
participant GitCLI as gh/git
participant AgentCLI as Claude/Gemini
participant FS as FileSystem
User->>Makefile: make review-changes [args]
Makefile->>Review: execute with args
alt PR-based
Review->>GitCLI: fetch PR (gh pr diff / git fetch pull/…)
GitCLI-->>Review: return PR diff
else Files-based
Review->>GitCLI: git diff -- <files>
GitCLI-->>Review: return diff
else Range-based
Review->>GitCLI: git diff <range>
GitCLI-->>Review: return diff
end
Review->>FS: collect changed file list
Review->>FS: write temp prompt (repo, files, diff)
Review->>AgentCLI: send prompt -> request JSON review
AgentCLI-->>Review: return JSON review
Review->>FS: write output file / stdout
Review->>FS: cleanup temp files
Review-->>User: display summarized review
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.shtest/review.sh: test/review.sh: openBinaryFile: does not exist (No such file or directory) Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JianLi-RH The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
JianLi-RH
force-pushed
the
test_code_review
branch
3 times, most recently
from
b430587 to
6c08e73
Compare
JianLi-RH
force-pushed
the
test_code_review
branch
from
6c08e73 to
5d41de8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
hack/review.sh (3)
165-165:REPO_NAMEalways reads from theoriginremote, ignoringPR_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 showis 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' ' -f5will silently produce a wrong branch name if the spacing differs (e.g., extra leading spaces). Themainfallback 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" --autoonce 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@v3andactions/upload-artifact@v3can 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
📒 Files selected for processing (9)
AGENTS.mdMakefilehack/review.shtest/CODE_REVIEW_AGENT.mdtest/README.mdtest/REVIEW_SCRIPT_UPDATES.mdtest/code-review-agent.jsontest/example-review-usage.mdtest/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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | ||
| ) |
There was a problem hiding this comment.
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$VARexpansion inside the body. Embed the dynamic values before the heredoc or useprintfto 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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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:** | ||
| ``` |
There was a problem hiding this comment.
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.
5d41de8 to
9d69892
Compare
JianLi-RH
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/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: |
There was a problem hiding this comment.
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
EXITtrap 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 hardcodesorigin.Line 131 and Line 132 still print
originin suggested commands, which can mislead users whenPR_REMOTEis 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.shin 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-evaluateBashtool 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
📒 Files selected for processing (9)
AGENTS.mdMakefilehack/review.shtest/CODE_REVIEW_AGENT.mdtest/README.mdtest/REVIEW_SCRIPT_UPDATES.mdtest/code-review-agent.jsontest/example-review-usage.mdtest/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 | ||
| ;; |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
🧩 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-formatwithtext | json | stream-json
Example:
claude -p "Explain this repo" --output-format jsonSource: 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 aclaude task --type ...CLI subcommand. Community docs show it assubagent_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 aclaude taskcommand. [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:
- 1: https://docs.anthropic.com/en/docs/claude-code/cli-usage
- 2: https://github.com/Cranot/claude-code-guide?utm_source=openai
🏁 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 -100Repository: 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 -20Repository: 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 jsonThe 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.", |
There was a problem hiding this comment.
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 | ||
| └── ... | ||
| ``` |
There was a problem hiding this comment.
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.
9d69892 to
8d82833
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
test/README.md (1)
61-70:⚠️ Potential issue | 🟡 MinorAdd a language identifier for the directory-tree fence (MD040).
The fence at Line 61 should include a language hint (e.g.,
text).📝 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>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test/README.mdaround lines 61 - 70, The fenced directory-tree block in
test/README.md lacks a language identifier; update the opening triple-backtick
for the block that shows the tree (the block starting with "test/") to include a
language hint (e.g., changetotext) so the Markdown linter rule MD040
passes and the tree renders correctly.</details> </blockquote></details> <details> <summary>test/REVIEW_SCRIPT_UPDATES.md (1)</summary><blockquote> `60-123`: _⚠️ Potential issue_ | _🟡 Minor_ **Add language identifiers to output fences (MD040).** The output blocks opened at Line 60, Line 78, Line 97, and Line 109 should specify a language (use `text`). <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text Fetching PR `#1273`... Using 'gh' CLI to fetch PR... Files changed: pkg/cvo/cvo.go pkg/payload/payload.go ... ``` -``` +```text Fetching PR `#1273`... Note: 'gh' CLI not found. Using git fetch as fallback. Install 'gh' for better PR integration: https://cli.github.com/ Fetching pull/1273/head from remote 'origin'... Successfully fetched PR `#1273` ``` -``` +```text Fetching PR `#1273`... Note: 'gh' CLI not found. Using git fetch as fallback. Install 'gh' for better PR integration: https://cli.github.com/ Fetching pull/1273/head from remote 'upstream'... Successfully fetched PR `#1273` ``` -``` +```text Error: Could not fetch PR `#1273` from remote 'origin' ... ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@test/REVIEW_SCRIPT_UPDATES.mdaround lines 60 - 123, Add the missing
language tag "text" to the Markdown fenced code blocks used for CLI output
examples in the review docs: update the four triple-backtick fences that wrap
the PR fetch summary, the two "Note: 'gh' CLI not found..." fallback outputs,
and the "Error: Could not fetch PR..." block so each starts withtext; ensure the exact fenced blocks shown in the diff (the initial "Fetching PR `#1273`..." snippet, the two fallback outputs, and the error handling example) are updated to usetext.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@hack/gemini_review.sh:
- Line 99: The
cd "$REPO_ROOT" || exit 0fallback masks a failure by exiting
with status 0; change the fallback toexit 1so the script fails when thecd
fails (locate thecd "$REPO_ROOT" || exit 0line in hack/gemini_review.sh and
replace theexit 0withexit 1), ensuring subsequent steps don't run when
the repo root change fails.- Around line 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.- Around line 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.In
@test/CODE_REVIEW_AGENT.md:
- Around line 48-54: The fenced code block that starts with ``` in the
test/CODE_REVIEW_AGENT.md documentation lacks a language identifier (MD040);
update that fence to include a language hint (e.g., "text") so the block becomestriple-backtick only and leave the inner content unchanged. --- Duplicate comments: In `@test/README.md`: - Around line 61-70: The fenced directory-tree block in test/README.md lacks a language identifier; update the opening triple-backtick for the block that shows the tree (the block starting with "test/") to include a language hint (e.g., change ``` to ```text) so the Markdown linter rule MD040 passes and the tree renders correctly. In `@test/REVIEW_SCRIPT_UPDATES.md`: - Around line 60-123: Add the missing language tag "text" to the Markdown fenced code blocks used for CLI output examples in the review docs: update the four triple-backtick fences that wrap the PR fetch summary, the two "Note: 'gh' CLI not found..." fallback outputs, and the "Error: Could not fetch PR..." block so each starts with ```text; ensure the exact fenced blocks shown in the diff (the initial "Fetching PR `#1273`..." snippet, the two fallback outputs, and the error handling example) are updated to use ```text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID:
66cc91f3-fe50-4dcf-9bec-f10e6dfb159e📒 Files selected for processing (10)
AGENTS.mdMakefilehack/gemini_review.shhack/review.shtest/CODE_REVIEW_AGENT.mdtest/README.mdtest/REVIEW_SCRIPT_UPDATES.mdtest/code-review-agent.jsontest/example-review-usage.mdtest/review.sh🚧 Files skipped from review as they are similar to previous changes (5)
- test/code-review-agent.json
- hack/review.sh
- test/example-review-usage.md
- AGENTS.md
- test/review.sh
Comment on lines
+73
to
+76
| -p|--pr) | ||
| PR_NUMBER="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
🧩 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.shRepository: 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:
# 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 -20Repository: 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.
| esac | ||
| done | ||
|
|
||
| cd "$REPO_ROOT" || exit 0 |
There was a problem hiding this comment.
🧩 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 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.
Comment on lines
+189
to
+192
| Git diff: | ||
| $(diff | ||
| ${DIFF} | ||
| ) |
There was a problem hiding this comment.
🧩 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
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.
Comment on lines
+48
to
+54
| ``` | ||
| Please review the changes in my current branch using the code-review agent. | ||
| Use the Task tool with the following: | ||
| - subagent_type: general-purpose | ||
| - Load the prompt from test/code-review-agent.json | ||
| - Provide the git diff and changed files as context | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to this fenced block (MD040).
The code fence opened at Line 48 has no language identifier.
📝 Proposed fix
-```
+```text
Please review the changes in my current branch using the code-review agent.
Use the Task tool with the following:
- subagent_type: general-purpose
- Load the prompt from test/code-review-agent.json
- Provide the git diff and changed files as context</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/CODE_REVIEW_AGENT.md` around lines 48 - 54, The fenced code block that
starts with ``` in the test/CODE_REVIEW_AGENT.md documentation lacks a language
identifier (MD040); update that fence to include a language hint (e.g., "text")
so the block becomes ```text and the linter will accept it, ensure you change
the opening triple-backtick only and leave the inner content unchanged.
Contributor
|
@JianLi-RH: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/OTA-1862
How to use:
Summary by CodeRabbit
New Features
Documentation
Chores