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 Makefile target and two review runners (Claude + Gemini) that gather diffs via gh/git, produce prompts, optionally call LLM CLIs, and emit structured JSON; includes agent configuration and extensive documentation and examples. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Makefile
participant Review as hack/review.sh / hack/gemini_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 (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
+217
to
+228
| FINDINGS=$(jq -r '.findings[] | | ||
| select(.severity=="high" or .severity=="medium") | | ||
| "**[\(.severity | ascii_upcase)] \(.file):\(.line_or_range)**\n\n\(.message)\n\n\(.suggestion // "")\n"' review.json) | ||
|
|
||
| # Post as PR comment | ||
| gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review | ||
|
|
||
| $FINDINGS | ||
|
|
||
| --- | ||
| _Generated by CVO code review agent_" | ||
| ``` |
There was a problem hiding this comment.
Example 10 posts an empty PR comment when no high/medium findings exist.
If FINDINGS is empty (clean review), gh pr comment still fires and posts a comment body containing only the header and footer — noisy and unhelpful. A guard (or a fallback LGTM message) makes the example more realistic and prevents comment spam.
📝 Proposed fix
-# Post as PR comment
-gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review
-
-$FINDINGS
-
----
-_Generated by CVO code review agent_"
+# Post as PR comment (only if there are findings)
+if [[ -n "$FINDINGS" ]]; then
+ gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review
+
+$FINDINGS
+
+---
+_Generated by CVO code review agent_"
+else
+ echo "No high/medium findings — skipping PR comment."
+fi📝 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
| FINDINGS=$(jq -r '.findings[] | | |
| select(.severity=="high" or .severity=="medium") | | |
| "**[\(.severity | ascii_upcase)] \(.file):\(.line_or_range)**\n\n\(.message)\n\n\(.suggestion // "")\n"' review.json) | |
| # Post as PR comment | |
| gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review | |
| $FINDINGS | |
| --- | |
| _Generated by CVO code review agent_" | |
| ``` | |
| FINDINGS=$(jq -r '.findings[] | | |
| select(.severity=="high" or .severity=="medium") | | |
| "**[\(.severity | ascii_upcase)] \(.file):\(.line_or_range)**\n\n\(.message)\n\n\(.suggestion // "")\n"' review.json) | |
| # Post as PR comment (only if there are findings) | |
| if [[ -n "$FINDINGS" ]]; then | |
| gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review | |
| $FINDINGS | |
| --- | |
| _Generated by CVO code review agent_" | |
| else | |
| echo "No high/medium findings — skipping PR comment." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/example-review-usage.md` around lines 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.
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
+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.
8d82833 to
1a5632b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (18)
hack/gemini_review.sh (2)
99-99:⚠️ Potential issue | 🟡 MinorChange the exit status to 1 when directory change fails.
exit 0masks the failure. Subsequent operations depend on being in the correct directory.🛠️ Proposed fix
-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 script currently uses "cd \"$REPO_ROOT\" || exit 0" which masks failures by exiting with status 0; update the fallback to exit with a non-zero status (e.g., change to "exit 1") so failures to change into REPO_ROOT are reported and stop the script; locate the statement in hack/gemini_review.sh that references the REPO_ROOT variable and replace the exit 0 with exit 1 (or another non-zero code) to ensure calling processes detect the error.
73-76:⚠️ Potential issue | 🟠 MajorValidate
--prargument before consuming$2.The
--prflag reads$2without checking if it exists or if it's another flag. If called as--pr --files foo.go,PR_NUMBERgets assigned--files.🐛 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 --pr case assigns PR_NUMBER from $2 without validation; update the -p|--pr) branch to first check that "$2" exists and does not begin with a hyphen (e.g., [[ -n "$2" && ${2:0:1} != "-" ]]) before assigning PR_NUMBER and shifting; if the check fails, print a clear error and exit with non-zero status so we don't consume the next flag as the PR number. Reference the -p|--pr) case, PR_NUMBER variable, and the subsequent shift 2 logic when making this change.hack/review.sh (4)
127-132:⚠️ Potential issue | 🟡 MinorError message hardcodes
originin the manual fetch suggestion.Line 131 says
git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}but should use${PR_REMOTE}for consistency when the user has overridden it.📝 Proposed fix
- echo " 3. Manually fetch: git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}" + echo " 3. Manually fetch: git fetch ${PR_REMOTE} pull/${PR_NUMBER}/head:pr-${PR_NUMBER}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/review.sh` around lines 127 - 132, The manual fetch suggestion in review.sh hardcodes "origin" instead of using the configured PR_REMOTE variable; update the suggestion string so the fetch command uses ${PR_REMOTE} (refer to PR_REMOTE and the echo that prints "git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}") to keep behavior consistent when a different remote is supplied.
55-86:⚠️ Potential issue | 🟠 MajorValidate option arguments before reading
$2.Lines 58, 62, and 73 read
$2without checking if it exists. Withset -u, calls like--pr(no value) exit abruptly instead of showing a clear usage error.🐛 Proposed fix
case $1 in --pr) + if [[ $# -lt 2 || "$2" == --* ]]; then + echo "Error: --pr requires a PR number" >&2 + usage + exit 1 + fi PR_NUMBER="$2" shift 2 ;; --range) + if [[ $# -lt 2 || "$2" == --* ]]; then + echo "Error: --range requires a git range" >&2 + usage + exit 1 + fi DIFF_RANGE="$2" shift 2 ;;Apply similar validation to
--output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/review.sh` around lines 55 - 86, The option parsing reads positional parameter $2 directly for --pr, --range and --output which fails under set -u when a value is missing; update the case branch for each of these flags (affecting PR_NUMBER, DIFF_RANGE and OUTPUT_FILE assignments) to first verify that a following argument exists and is not another flag (e.g. test that $# -ge 2 and that $2 does not start with --), and if the value is missing call usage and exit non‑zero; apply the same presence/validation logic used in the --files handling and reuse the existing usage() error path so missing values produce a clear message instead of an abrupt unset-variable error.
191-194:⚠️ Potential issue | 🔴 Critical
claude task --type general-purposeis not a valid Claude CLI command.The official Claude Code CLI does not support a
tasksubcommand or--type/--prompt/--outputflags. The documented syntax isclaude -p "prompt" --output-format json. This will fail at runtime.Claude Code CLI "claude task" command documentation 2026🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/review.sh` around lines 191 - 194, The script uses an invalid Claude CLI invocation ("claude task --type general-purpose --prompt "@${PROMPT_FILE}" --output "$OUTPUT_FILE""); replace it with the documented form using the -p flag and --output-format, e.g. call claude -p "@${PROMPT_FILE}" --output-format json and write the output to the OUTPUT_FILE (for example by redirecting stdout to "$OUTPUT_FILE" or using the CLI's output options), updating the invocation that references PROMPT_FILE and OUTPUT_FILE accordingly.
184-186:⚠️ Potential issue | 🟡 MinorAdd a
trapto ensure temp file cleanup on early exit.If the script exits between
mktempandrm, the temp file (containing the full diff) remains on disk.🛠️ Proposed fix
PROMPT_FILE=$(mktemp) +trap 'rm -f "$PROMPT_FILE"' EXIT echo "$REVIEW_PROMPT" > "$PROMPT_FILE"Then remove the explicit
rm -f "$PROMPT_FILE"at line 219.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/review.sh` around lines 184 - 186, The temp file created by PROMPT_FILE=$(mktemp) can be left behind if the script exits early; after creating PROMPT_FILE add a trap that removes it on EXIT (e.g., trap 'rm -f "$PROMPT_FILE"' EXIT) so it is always cleaned up, and then remove the later explicit rm -f "$PROMPT_FILE" invocation to avoid double-deletion.test/CODE_REVIEW_AGENT.md (2)
17-42:⚠️ Potential issue | 🟠 Major
review.shreferences point totest/but the actual script is athack/review.sh.Line 20 lists
review.shas residing intest/, and all Quick Start examples (lines 29, 32, 35, 38, 41) use./test/review.sh. Update these to./hack/review.sh.🤖 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 17 - 42, Update the README references to the review script so they point to the actual location: replace mentions of "review.sh" under the Files list and all Quick Start example invocations that use "./test/review.sh" with "./hack/review.sh"; specifically update the "Files in This Directory" bullet that lists review.sh and each example command (Review your current branch, Review a specific pull request, Review specific commits, Review specific files only, Save output) to use the correct "./hack/review.sh" path so documentation matches the real script location.
48-54:⚠️ Potential issue | 🟡 MinorAdd a language hint to this fenced block (MD040).
The code fence at line 48 has no language identifier.
📝 Proposed fix
-``` +```text Please review the changes in my current branch using the code-review agent.🤖 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 containing the text "Please review the changes in my current branch using the code-review agent." is missing a language hint (MD040); update the opening fence from ``` to ```text (or another appropriate language identifier like ```txt) so it becomes ```text and keep the trailing closing fence unchanged; this will satisfy the linter and preserve the block content.test/README.md (3)
17-24:⚠️ Potential issue | 🟠 MajorScript path references point to
./test/review.shbut the script is at./hack/review.sh.The Quick Start examples at lines 20 and 23 will fail with "file not found". Update all occurrences throughout this file.
🐛 Proposed fix
**Quick Start:** ```bash # Review your current branch -./test/review.sh +./hack/review.sh # Review a specific PR -./test/review.sh --pr 1273 +./hack/review.sh --pr 1273</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 17 - 24, Update all references to the review
script in the README that incorrectly point to "./test/review.sh" so they
instead reference the actual script location "./hack/review.sh"; search for
occurrences of the string "./test/review.sh" (e.g., in the "Quick Start"
examples) and replace each with "./hack/review.sh" ensuring flags like "--pr
1273" are preserved.</details> --- `61-70`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language hint to the directory-tree code fence (MD040).** The fenced block at line 61 lacks a language identifier. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text test/ ├── cvo/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/README.md` around lines 61 - 70, The markdown fenced code block containing the directory tree needs a language hint to satisfy MD040; update the opening fence from ``` to ```text (or ```bash/```none) so the block becomes a labeled code fence for the directory-tree snippet, ensuring the README's directory-tree code fence is annotated with a language identifier.
129-148:⚠️ Potential issue | 🟠 MajorCI example also uses incorrect script path.
Line 143 references
./test/review.shwhich should be./hack/review.sh.🐛 Proposed fix
- name: Review PR - run: ./test/review.sh --output review.json + run: ./hack/review.sh --output review.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/README.md` around lines 129 - 148, Update the GitHub Actions example workflow to use the correct script path: replace the referenced "./test/review.sh" string in the workflow snippet (the run step in the "Review PR" job) with "./hack/review.sh" so the CI step invokes the actual review script.test/example-review-usage.md (3)
217-228:⚠️ Potential issue | 🟡 MinorExample posts an empty PR comment when no high/medium findings exist.
If
FINDINGSis empty,gh pr commentstill fires with only the header/footer. Add a guard.📝 Proposed fix
+if [[ -n "$FINDINGS" ]]; then gh pr comment "$PR_NUMBER" --body "## 🤖 Automated Code Review $FINDINGS --- _Generated by CVO code review agent_" +else + echo "No high/medium findings — skipping PR comment." +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/example-review-usage.md` around lines 217 - 228, The script assigns FINDINGS from jq and always calls gh pr comment, which posts an empty review when no high/medium findings exist; add a guard around the gh pr comment invocation (checking the FINDINGS variable or the jq result) so the PR comment is only posted if FINDINGS is non-empty (e.g., test FINDINGS with a non-empty string or check jq count) and skip or log a no-findings message otherwise; modify the block that defines FINDINGS and the subsequent gh pr comment call to conditionally execute the comment only when FINDINGS contains content.
87-93:⚠️ Potential issue | 🟡 MinorMisleading comment and unused variables in pre-push hook example.
- Line 90 says "Only run for main branch" but the condition
!= "main"runs for every branch except main.REMOTEandURL(lines 87-88) are captured but never used.📝 Proposed fix
-REMOTE="$1" -URL="$2" - -# Only run for main branch +# Only run for non-main branches (skip review when pushing directly to main) CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/example-review-usage.md` around lines 87 - 93, The pre-push hook has a misleading comment and unused variables: either update the branch check or the comment, and remove or use the unused REMOTE and URL variables; specifically, decide whether you want to "Only run for main branch" and if so change the CURRENT_BRANCH check in the conditional from != "main" to == "main" (operate on CURRENT_BRANCH), otherwise update the echo message to reflect that it runs for non-main branches; also remove the unused REMOTE and URL variables (or use them where needed) to eliminate dead variables.
56-61:⚠️ Potential issue | 🟡 MinorUnquoted subshell expansion may produce unexpected behavior when no files match.
If
grepmatches zero files,$MODIFIED_GO_FILESexpands to nothing and--filesreceives no arguments. Add a guard before invoking the script.📝 Proposed fix
MODIFIED_GO_FILES=$(git diff --name-only origin/main...HEAD | grep '\.go$') -./test/review.sh --files $MODIFIED_GO_FILES +if [[ -n "$MODIFIED_GO_FILES" ]]; then + ./hack/review.sh --files $MODIFIED_GO_FILES +else + echo "No Go files changed, skipping review." +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/example-review-usage.md` around lines 56 - 61, The script sets MODIFIED_GO_FILES from a subshell and then calls ./test/review.sh --files $MODIFIED_GO_FILES unquoted, which yields no arguments when grep finds nothing; add a guard that checks MODIFIED_GO_FILES is non-empty (or use a properly quoted/array form) and only invoke ./test/review.sh --files with MODIFIED_GO_FILES when it contains values, otherwise skip the call or run the script without --files to avoid unexpected behavior.test/REVIEW_SCRIPT_UPDATES.md (2)
5-5:⚠️ Potential issue | 🟡 MinorRelative link
[review.sh](review.sh)resolves totest/review.sh, not the actual script.Update to
[review.sh](../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 the markdown points to the actual script location: replace the broken link text [review.sh](review.sh) with [review.sh](../hack/review.sh) so the reference resolves to the real script; ensure you update the exact token "[review.sh](review.sh)" in the file to the new "[review.sh](../hack/review.sh)" form.
60-60:⚠️ Potential issue | 🟡 MinorFenced code blocks at lines 60, 78, 97, and 109 are missing language identifiers (MD040).
Add
textorbashas appropriate to each block.📝 Proposed fix (apply to all four)
-``` +```text Fetching PR `#1273`...🤖 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, Several fenced code blocks in the markdown are missing language identifiers causing MD040; locate the code fences that contain the example lines like "Fetching PR `#1273`..." (and similar blocks at the other reported positions) and update each opening fence from ``` to ```text or ```bash as appropriate (use ```text for plain output examples and ```bash for shell commands) so every code block has a language identifier.test/code-review-agent.json (2)
7-7:⚠️ Potential issue | 🟠 Major
Bashtool grants arbitrary shell execution — confirm this is intentional.Including
"Bash"allows the review agent to run arbitrary shell commands. For CI use, this widens the attack surface (prompt injection via diff content → command execution). Consider restricting to["Read", "Grep", "Glob"]unless shell access is required.🤖 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 currently includes the "Bash" entry which permits arbitrary shell execution; remove "Bash" from the tools list (change ["Read","Grep","Glob","Bash"] to ["Read","Grep","Glob"]) unless you explicitly require shell access, or add a secure opt-in flag/validation around the "tools" configuration to only enable "Bash" when explicitly authorized; update any code that parses the "tools" config to validate and reject/require approval for "Bash".
6-6:⚠️ Potential issue | 🟠 MajorFix invalid JSON in the embedded output schema example.
The prompt contains
"line_or_range": "42" or "42-51"which is not valid JSON. This may cause the agent to produce malformed output.📝 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 JSON example contains an invalid value for the key "line_or_range" ("42" or "42-51") which breaks JSON parsing; replace that fragment in the Output Format example with valid JSON—either a single string example like "line_or_range": "42-51" or an array of examples like "line_or_range": ["42", "42-51"]—so the example remains human-readable but is syntactically valid; update the example under the "Output Format" block (the "line_or_range" key) accordingly.
🤖 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/gemini_review.sh`:
- Around line 12-13: The REPO_ROOT variable is incorrectly set to SCRIPT_DIR
(making it point at hack/) so change REPO_ROOT to the repository root by
resolving the parent of SCRIPT_DIR; update the assignment for REPO_ROOT
(currently referencing SCRIPT_DIR) to compute the parent directory (i.e., cd to
"${SCRIPT_DIR}/.." and pwd) so subsequent git commands run from the repository
root; ensure you modify the REPO_ROOT variable in the same file that defines
SCRIPT_DIR and keep SCRIPT_DIR assignment unchanged.
- Around line 196-198: Add a trap right after creating PROMPT_FILE with mktemp
to ensure the temp file is removed on any EXIT or interrupt; specifically, after
PROMPT_FILE=$(mktemp) register a trap that runs rm -f "$PROMPT_FILE" on EXIT,
INT, TERM so the temp file is cleaned if the script exits early, and then remove
the explicit rm -f "$PROMPT_FILE" at the end of the script since the trap will
handle cleanup; ensure references to PROMPT_FILE and the mktemp call (and any
echo "$REVIEW_PROMPT" > "$PROMPT_FILE") remain unchanged.
In `@test/CODE_REVIEW_AGENT.md`:
- Around line 159-213: Update the CI examples to use the correct script path by
replacing occurrences of "./test/review.sh" with "./hack/review.sh" in both the
GitHub Actions job step (the Run Code Review Agent step that invokes review.sh)
and the GitLab CI job (the code-review script invocation that pipes output to
review.json); ensure any references to the old path (the string "test/review.sh"
or the step name "Run Code Review Agent") are changed so CI runs
"./hack/review.sh" instead.
In `@test/example-review-usage.md`:
- Around line 23-26: Replace all incorrect references to the review script
('./test/review.sh') in test/example-review-usage.md with the correct path
('./hack/review.sh'); scan the file for every occurrence of the string
"./test/review.sh" (multiple examples are present) and update them to
"./hack/review.sh", then run a quick smoke-check by executing one example
command to confirm the corrected path works.
- Around line 325-331: The examples in test/example-review-usage.md incorrectly
use the unsupported --diff-file flag when invoking hack/review.sh; update the
example commands to use supported options (--pr, --range, --files, or --output)
or show a file-based workaround: either change the piped example to call
./hack/review.sh --range (after writing git diff --cached > /tmp/staged.diff) or
remove --diff-file and pipe the diff directly into ./hack/review.sh
--files/--output as appropriate, and mention hack/review.sh by name so reviewers
can find the implementation to verify supported flags.
In `@test/README.md`:
- Around line 150-158: In the "Development Workflow" section update the
incorrect script path by replacing the referenced './test/review.sh' with the
correct './hack/review.sh' (look for the line containing the literal
'./test/review.sh' in the README and change it to './hack/review.sh'); ensure
the step text remains the same otherwise.
In `@test/REVIEW_SCRIPT_UPDATES.md`:
- Around line 48-105: Update the example commands in REVIEW_SCRIPT_UPDATES.md by
replacing every instance of the incorrect script path string './test/review.sh'
with the correct './hack/review.sh' (this affects examples under headings "With
gh CLI (Recommended)" and "Without gh CLI (Git Fallback)" including the
fork/upstream snippets and their outputs so that the commands and output blocks
match the correct script path).
---
Duplicate comments:
In `@hack/gemini_review.sh`:
- Line 99: The script currently uses "cd \"$REPO_ROOT\" || exit 0" which masks
failures by exiting with status 0; update the fallback to exit with a non-zero
status (e.g., change to "exit 1") so failures to change into REPO_ROOT are
reported and stop the script; locate the statement in hack/gemini_review.sh that
references the REPO_ROOT variable and replace the exit 0 with exit 1 (or another
non-zero code) to ensure calling processes detect the error.
- Around line 73-76: The --pr case assigns PR_NUMBER from $2 without validation;
update the -p|--pr) branch to first check that "$2" exists and does not begin
with a hyphen (e.g., [[ -n "$2" && ${2:0:1} != "-" ]]) before assigning
PR_NUMBER and shifting; if the check fails, print a clear error and exit with
non-zero status so we don't consume the next flag as the PR number. Reference
the -p|--pr) case, PR_NUMBER variable, and the subsequent shift 2 logic when
making this change.
In `@hack/review.sh`:
- Around line 127-132: The manual fetch suggestion in review.sh hardcodes
"origin" instead of using the configured PR_REMOTE variable; update the
suggestion string so the fetch command uses ${PR_REMOTE} (refer to PR_REMOTE and
the echo that prints "git fetch origin pull/${PR_NUMBER}/head:pr-${PR_NUMBER}")
to keep behavior consistent when a different remote is supplied.
- Around line 55-86: The option parsing reads positional parameter $2 directly
for --pr, --range and --output which fails under set -u when a value is missing;
update the case branch for each of these flags (affecting PR_NUMBER, DIFF_RANGE
and OUTPUT_FILE assignments) to first verify that a following argument exists
and is not another flag (e.g. test that $# -ge 2 and that $2 does not start with
--), and if the value is missing call usage and exit non‑zero; apply the same
presence/validation logic used in the --files handling and reuse the existing
usage() error path so missing values produce a clear message instead of an
abrupt unset-variable error.
- Around line 191-194: The script uses an invalid Claude CLI invocation ("claude
task --type general-purpose --prompt "@${PROMPT_FILE}" --output
"$OUTPUT_FILE""); replace it with the documented form using the -p flag and
--output-format, e.g. call claude -p "@${PROMPT_FILE}" --output-format json and
write the output to the OUTPUT_FILE (for example by redirecting stdout to
"$OUTPUT_FILE" or using the CLI's output options), updating the invocation that
references PROMPT_FILE and OUTPUT_FILE accordingly.
- Around line 184-186: The temp file created by PROMPT_FILE=$(mktemp) can be
left behind if the script exits early; after creating PROMPT_FILE add a trap
that removes it on EXIT (e.g., trap 'rm -f "$PROMPT_FILE"' EXIT) so it is always
cleaned up, and then remove the later explicit rm -f "$PROMPT_FILE" invocation
to avoid double-deletion.
In `@test/CODE_REVIEW_AGENT.md`:
- Around line 17-42: Update the README references to the review script so they
point to the actual location: replace mentions of "review.sh" under the Files
list and all Quick Start example invocations that use "./test/review.sh" with
"./hack/review.sh"; specifically update the "Files in This Directory" bullet
that lists review.sh and each example command (Review your current branch,
Review a specific pull request, Review specific commits, Review specific files
only, Save output) to use the correct "./hack/review.sh" path so documentation
matches the real script location.
- Around line 48-54: The fenced code block containing the text "Please review
the changes in my current branch using the code-review agent." is missing a
language hint (MD040); update the opening fence from ``` to ```text (or another
appropriate language identifier like ```txt) so it becomes ```text and keep the
trailing closing fence unchanged; this will satisfy the linter and preserve the
block content.
In `@test/code-review-agent.json`:
- Line 7: The "tools" array currently includes the "Bash" entry which permits
arbitrary shell execution; remove "Bash" from the tools list (change
["Read","Grep","Glob","Bash"] to ["Read","Grep","Glob"]) unless you explicitly
require shell access, or add a secure opt-in flag/validation around the "tools"
configuration to only enable "Bash" when explicitly authorized; update any code
that parses the "tools" config to validate and reject/require approval for
"Bash".
- Line 6: The embedded JSON example contains an invalid value for the key
"line_or_range" ("42" or "42-51") which breaks JSON parsing; replace that
fragment in the Output Format example with valid JSON—either a single string
example like "line_or_range": "42-51" or an array of examples like
"line_or_range": ["42", "42-51"]—so the example remains human-readable but is
syntactically valid; update the example under the "Output Format" block (the
"line_or_range" key) accordingly.
In `@test/example-review-usage.md`:
- Around line 217-228: The script assigns FINDINGS from jq and always calls gh
pr comment, which posts an empty review when no high/medium findings exist; add
a guard around the gh pr comment invocation (checking the FINDINGS variable or
the jq result) so the PR comment is only posted if FINDINGS is non-empty (e.g.,
test FINDINGS with a non-empty string or check jq count) and skip or log a
no-findings message otherwise; modify the block that defines FINDINGS and the
subsequent gh pr comment call to conditionally execute the comment only when
FINDINGS contains content.
- Around line 87-93: The pre-push hook has a misleading comment and unused
variables: either update the branch check or the comment, and remove or use the
unused REMOTE and URL variables; specifically, decide whether you want to "Only
run for main branch" and if so change the CURRENT_BRANCH check in the
conditional from != "main" to == "main" (operate on CURRENT_BRANCH), otherwise
update the echo message to reflect that it runs for non-main branches; also
remove the unused REMOTE and URL variables (or use them where needed) to
eliminate dead variables.
- Around line 56-61: The script sets MODIFIED_GO_FILES from a subshell and then
calls ./test/review.sh --files $MODIFIED_GO_FILES unquoted, which yields no
arguments when grep finds nothing; add a guard that checks MODIFIED_GO_FILES is
non-empty (or use a properly quoted/array form) and only invoke ./test/review.sh
--files with MODIFIED_GO_FILES when it contains values, otherwise skip the call
or run the script without --files to avoid unexpected behavior.
In `@test/README.md`:
- Around line 17-24: Update all references to the review script in the README
that incorrectly point to "./test/review.sh" so they instead reference the
actual script location "./hack/review.sh"; search for occurrences of the string
"./test/review.sh" (e.g., in the "Quick Start" examples) and replace each with
"./hack/review.sh" ensuring flags like "--pr 1273" are preserved.
- Around line 61-70: The markdown fenced code block containing the directory
tree needs a language hint to satisfy MD040; update the opening fence from ```
to ```text (or ```bash/```none) so the block becomes a labeled code fence for
the directory-tree snippet, ensuring the README's directory-tree code fence is
annotated with a language identifier.
- Around line 129-148: Update the GitHub Actions example workflow to use the
correct script path: replace the referenced "./test/review.sh" string in the
workflow snippet (the run step in the "Review PR" job) with "./hack/review.sh"
so the CI step invokes the actual review script.
In `@test/REVIEW_SCRIPT_UPDATES.md`:
- Line 5: Update the relative link in test/REVIEW_SCRIPT_UPDATES.md so the
markdown points to the actual script location: replace the broken link text
[review.sh](review.sh) with [review.sh](../hack/review.sh) so the reference
resolves to the real script; ensure you update the exact token
"[review.sh](review.sh)" in the file to the new "[review.sh](../hack/review.sh)"
form.
- Line 60: Several fenced code blocks in the markdown are missing language
identifiers causing MD040; locate the code fences that contain the example lines
like "Fetching PR `#1273`..." (and similar blocks at the other reported positions)
and update each opening fence from ``` to ```text or ```bash as appropriate (use
```text for plain output examples and ```bash for shell commands) so every code
block has a language identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1243150a-f482-40e5-a5a0-ecc76d64f1ad
📒 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 (2)
- Makefile
- test/review.sh
Comment on lines
+12
to
+13
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| REPO_ROOT="${SCRIPT_DIR}" |
There was a problem hiding this comment.
REPO_ROOT is set to SCRIPT_DIR instead of the actual repository root.
SCRIPT_DIR resolves to hack/, so REPO_ROOT will be hack/ as well. This differs from hack/review.sh which correctly uses REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)". Git commands expecting to run from the repo root may behave unexpectedly.
🐛 Proposed fix
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-REPO_ROOT="${SCRIPT_DIR}"
+REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/gemini_review.sh` around lines 12 - 13, The REPO_ROOT variable is
incorrectly set to SCRIPT_DIR (making it point at hack/) so change REPO_ROOT to
the repository root by resolving the parent of SCRIPT_DIR; update the assignment
for REPO_ROOT (currently referencing SCRIPT_DIR) to compute the parent directory
(i.e., cd to "${SCRIPT_DIR}/.." and pwd) so subsequent git commands run from the
repository root; ensure you modify the REPO_ROOT variable in the same file that
defines SCRIPT_DIR and keep SCRIPT_DIR assignment unchanged.
Comment on lines
+196
to
+198
| # Save prompt to temp file for gemini CLI | ||
| PROMPT_FILE=$(mktemp) | ||
| echo "$REVIEW_PROMPT" > "$PROMPT_FILE" |
There was a problem hiding this comment.
Add a trap to ensure temp file cleanup on early exit.
If the script exits unexpectedly between mktemp and the final rm, the temp file (containing the full diff) remains on disk.
🛠️ Proposed fix
PROMPT_FILE=$(mktemp)
+trap 'rm -f "$PROMPT_FILE"' EXIT
echo "$REVIEW_PROMPT" > "$PROMPT_FILE"Then remove the explicit rm -f "$PROMPT_FILE" at the end since the trap handles it.
📝 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
| # Save prompt to temp file for gemini CLI | |
| PROMPT_FILE=$(mktemp) | |
| echo "$REVIEW_PROMPT" > "$PROMPT_FILE" | |
| # Save prompt to temp file for gemini CLI | |
| PROMPT_FILE=$(mktemp) | |
| trap 'rm -f "$PROMPT_FILE"' EXIT | |
| echo "$REVIEW_PROMPT" > "$PROMPT_FILE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/gemini_review.sh` around lines 196 - 198, Add a trap right after
creating PROMPT_FILE with mktemp to ensure the temp file is removed on any EXIT
or interrupt; specifically, after PROMPT_FILE=$(mktemp) register a trap that
runs rm -f "$PROMPT_FILE" on EXIT, INT, TERM so the temp file is cleaned if the
script exits early, and then remove the explicit rm -f "$PROMPT_FILE" at the end
of the script since the trap will handle cleanup; ensure references to
PROMPT_FILE and the mktemp call (and any echo "$REVIEW_PROMPT" > "$PROMPT_FILE")
remain unchanged.
Comment on lines
+159
to
+213
| ## Integration with CI/CD | ||
|
|
||
| ### GitHub Actions Example | ||
|
|
||
| ```yaml | ||
| name: Code Review | ||
| on: [pull_request] | ||
|
|
||
| jobs: | ||
| review: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Run Code Review Agent | ||
| run: | | ||
| ./test/review.sh --range origin/${{ github.base_ref }}...${{ github.sha }} \ | ||
| --output review-results.json | ||
|
|
||
| - name: Post Review Comments | ||
| uses: actions/github-script@v6 | ||
| with: | ||
| script: | | ||
| const fs = require('fs'); | ||
| const review = JSON.parse(fs.readFileSync('review-results.json')); | ||
|
|
||
| // Post findings as PR comments | ||
| for (const finding of review.findings) { | ||
| if (finding.severity === 'high' || finding.severity === 'medium') { | ||
| await github.rest.pulls.createReviewComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number, | ||
| body: `**${finding.severity.toUpperCase()}**: ${finding.message}`, | ||
| path: finding.file, | ||
| line: parseInt(finding.line_or_range) | ||
| }); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### GitLab CI Example | ||
|
|
||
| ```yaml | ||
| code-review: | ||
| stage: review | ||
| script: | ||
| - ./test/review.sh --range origin/main...HEAD --output review.json | ||
| - cat review.json | jq -r '.findings[] | select(.severity=="high") | .message' | ||
| artifacts: | ||
| reports: | ||
| codequality: review.json | ||
| ``` |
There was a problem hiding this comment.
CI/CD examples use incorrect script path.
Lines 177 and 208 reference ./test/review.sh. Update to ./hack/review.sh.
🤖 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 159 - 213, Update the CI examples to
use the correct script path by replacing occurrences of "./test/review.sh" with
"./hack/review.sh" in both the GitHub Actions job step (the Run Code Review
Agent step that invokes review.sh) and the GitLab CI job (the code-review script
invocation that pipes output to review.json); ensure any references to the old
path (the string "test/review.sh" or the step name "Run Code Review Agent") are
changed so CI runs "./hack/review.sh" instead.
Comment on lines
+23
to
+26
| ```bash | ||
| cd /path/to/cluster-version-operator | ||
| ./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 actual script is at ./hack/review.sh. This affects examples at lines 25, 60, 69, 74, 94, 130, 148, 174, 178, 196, 199, 202, 214, 245, 250, 256, 267, 291, 298, 309, 316, and 327.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/example-review-usage.md` around lines 23 - 26, Replace all incorrect
references to the review script ('./test/review.sh') in
test/example-review-usage.md with the correct path ('./hack/review.sh'); scan
the file for every occurrence of the string "./test/review.sh" (multiple
examples are present) and update them to "./hack/review.sh", then run a quick
smoke-check by executing one example command to confirm the corrected path
works.
Comment on lines
+325
to
+331
|
|
||
| # Review staged changes | ||
| git diff --cached | ./test/review.sh --diff-file /dev/stdin | ||
|
|
||
| # If LGTM, commit | ||
| git commit -m "cvo: fix synchronization race condition" | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if --diff-file is implemented in review.sh
rg -n 'diff.file|diff_file|\-\-diff\-file' hack/review.shRepository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Check what flags are actually supported by review.sh
cat hack/review.sh | head -100Repository: openshift/cluster-version-operator
Length of output: 2930
🏁 Script executed:
# Read the specific lines mentioned in the review comment
sed -n '320,350p' test/example-review-usage.mdRepository: openshift/cluster-version-operator
Length of output: 801
--diff-file flag is not supported by review.sh but is used in examples.
The examples at lines 327 and 341 reference --diff-file which does not exist in hack/review.sh. The script only supports: --pr, --range, --files, and --output. Users following these examples will encounter "Unknown option: --diff-file" errors.
Update the examples to use one of the supported options, such as:
git diff --cached > /tmp/staged.difffollowed by./hack/review.sh --range(if using a file-based approach is needed)- Or remove
--diff-fileand pipe directly if the script gains that feature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/example-review-usage.md` around lines 325 - 331, The examples in
test/example-review-usage.md incorrectly use the unsupported --diff-file flag
when invoking hack/review.sh; update the example commands to use supported
options (--pr, --range, --files, or --output) or show a file-based workaround:
either change the piped example to call ./hack/review.sh --range (after writing
git diff --cached > /tmp/staged.diff) or remove --diff-file and pipe the diff
directly into ./hack/review.sh --files/--output as appropriate, and mention
hack/review.sh by name so reviewers can find the implementation to verify
supported flags.
Comment on lines
+150
to
+158
| ## Development Workflow | ||
|
|
||
| 1. **Make changes** to CVO code | ||
| 2. **Run unit tests**: `make test` | ||
| 3. **Review changes**: `./test/review.sh` | ||
| 4. **Fix issues** identified in review | ||
| 5. **Run integration tests** (on test cluster): `make integration-test` | ||
| 6. **Commit** with conventional format: `pkg/cvo: fix synchronization bug` | ||
|
|
There was a problem hiding this comment.
Development workflow example uses incorrect script path.
Line 154 should reference ./hack/review.sh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/README.md` around lines 150 - 158, In the "Development Workflow" section
update the incorrect script path by replacing the referenced './test/review.sh'
with the correct './hack/review.sh' (look for the line containing the literal
'./test/review.sh' in the README and change it to './hack/review.sh'); ensure
the step text remains the same otherwise.
Comment on lines
+48
to
+105
| ## Usage Examples | ||
|
|
||
| ### With gh CLI (Recommended) | ||
| ```bash | ||
| # Install gh first: https://cli.github.com/ | ||
| gh auth login | ||
|
|
||
| # Review a PR | ||
| ./test/review.sh --pr 1273 | ||
| ``` | ||
|
|
||
| **Output:** | ||
| ``` | ||
| Fetching PR #1273... | ||
| Using 'gh' CLI to fetch PR... | ||
| Files changed: | ||
| pkg/cvo/cvo.go | ||
| pkg/payload/payload.go | ||
| ... | ||
| ``` | ||
|
|
||
| ### Without gh CLI (Git Fallback) | ||
|
|
||
| #### From Upstream Repository | ||
| ```bash | ||
| # Works if 'origin' points to github.com/openshift/cluster-version-operator | ||
| ./test/review.sh --pr 1273 | ||
| ``` | ||
|
|
||
| **Output:** | ||
| ``` | ||
| 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 | ||
| ``` | ||
|
|
||
| #### From a Fork | ||
| ```bash | ||
| # Add upstream remote (one time) | ||
| git remote add upstream https://github.com/openshift/cluster-version-operator.git | ||
|
|
||
| # Fetch PR from upstream | ||
| PR_REMOTE=upstream ./test/review.sh --pr 1273 | ||
| ``` | ||
|
|
||
| **Output:** | ||
| ``` | ||
| 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 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Example commands use incorrect script path.
Lines 56, 74, 93, 94, 164, 175, 184, 193 reference ./test/review.sh. Update to ./hack/review.sh.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 97-97: 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` around lines 48 - 105, Update the example
commands in REVIEW_SCRIPT_UPDATES.md by replacing every instance of the
incorrect script path string './test/review.sh' with the correct
'./hack/review.sh' (this affects examples under headings "With gh CLI
(Recommended)" and "Without gh CLI (Git Fallback)" including the fork/upstream
snippets and their outputs so that the commands and output blocks match the
correct script path).
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