OTA-1862 Enable AI code review for test scripts#1316
OTA-1862 Enable AI code review for test scripts#1316JianLi-RH wants to merge 1 commit intoopenshift:mainfrom
Conversation
WalkthroughAdds an automated code-review workflow: new Makefile target, a Bash review script (hack/review.sh) retrieving diffs via gh/git and invoking Claude CLI, a JSON agent config, and multiple docs/examples explaining usage and integration. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Makefile
participant Review as hack/review.sh
participant CLI as gh/git
participant Claude as Claude CLI
participant FS as FileSystem
User->>Makefile: make review-changes [options]
Makefile->>Review: execute with args
alt PR-based
Review->>CLI: request PR diff (gh pr diff)
alt gh available
CLI-->>Review: PR diff
else
Review->>CLI: git fetch PR_REMOTE && git diff
CLI-->>Review: PR diff
end
else File(s)-based
Review->>CLI: git diff -- <files>
CLI-->>Review: diff
else Range-based
Review->>CLI: git diff <range>
CLI-->>Review: diff
end
Review->>FS: collect changed file list
Review->>FS: write temp prompt (repo, files, diff)
Review->>Claude: send prompt -> request JSON review
Claude-->>Review: return JSON review
Review->>FS: write output file / stdout
Review->>FS: cleanup temp files
Review-->>User: display summarized review
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Shellcheck (0.11.0)test/review.shtest/review.sh: test/review.sh: openBinaryFile: does not exist (No such file or directory) Comment |
|
[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 |
b430587 to
6c08e73
Compare
6c08e73 to
5d41de8
Compare
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.
| 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."
| 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
| 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.
| ./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
|
/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
| --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.
| ``` | ||
| 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.
|
@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. |
https://issues.redhat.com/browse/OTA-1862
How to use:
Summary by CodeRabbit
New Features
Documentation