fix(utils): improve address-reviews reliability#425
Merged
openshift-merge-bot[bot] merged 1 commit intoApr 20, 2026
Merged
Conversation
openshift-ci
Bot
added
the
approved
bryan-cox
force-pushed
the
improve-address-reviews
branch
from
a0e5bb8 to
61791c6
Compare
Contributor
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/utils/commands/address-reviews.md`:
- Around line 225-231: Add a language identifier (e.g., text) to the two fenced
code blocks that show verification logs: the block containing "✅ Verification
passed (make verify)" and the block containing "❌ Verification failed — fixing
issues before push" so the code fences become ```text ... ``` to satisfy
markdownlint MD040.
- Around line 215-221: The documentation claims a 15-minute timeout for "Run
verification" but doesn't enforce it; update the flow text around the "make
verify 2>&1" example and the retry logic so the verification command is
explicitly executed with a 15-minute timeout wrapper (e.g., shell timeout or
equivalent in the automation runner), ensure failures/timeouts are captured as
errors, and modify the retry handling to treat a timeout as a failed attempt and
include the last timeout/error text in the final message "Verification continues
to fail after 3 attempts. Last error: [error]. Manual intervention needed."
- Around line 136-140: The remote detection using grep
'openshift/hypershift\|upstream' is repo-specific; change the logic around
BASE_REMOTE so it picks a sensible remote in any repo: prefer a remote literally
named "upstream" if present, otherwise prefer "origin", and as a final fallback
pick the first remote returned by git remote (use commands around BASE_REMOTE
and git remote -v/gist of git remote). Update the BASE_REMOTE assignment and the
comment to reflect this new order, keeping the later git fetch "$BASE_REMOTE" &&
git rebase "$BASE_REMOTE/$BASE_BRANCH" unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a82b5a3-4802-4de7-a0a0-9c9131316ab8
📒 Files selected for processing (4)
.claude-plugin/marketplace.jsondocs/data.jsonplugins/utils/.claude-plugin/plugin.jsonplugins/utils/commands/address-reviews.md
…, rebase, and verification Addresses three failures reported by Alberto on hypershift PRs: 1. Comments on stale diff hunks (line=null, original_line≠null) were being incorrectly filtered as "outdated" — now only truly orphaned comments (both null) are filtered out. 2. Repo-level instructions like "please rebase" and "make sure verify passes" had no dedicated handling — adds ACTION_INSTRUCTION category processed before code changes, with dynamic remote/branch detection for rebase. 3. No pre-push verification existed — adds Step 3.5 that detects available verification commands (make verify, make lint, go build, npm lint) and blocks push on failure with max 3 retries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
improve-address-reviews
branch
from
61791c6 to
9251288
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/utils/commands/address-reviews.md (3)
217-217: Clarify if both Go commands must pass.For the Go verification case, two commands are listed (
go build ./...andgo vet ./...). Should both pass, or is either sufficient?📝 Suggested clarification
- - `go.mod` exists → `go build ./...` and `go vet ./...` + - `go.mod` exists → run both `go build ./...` and `go vet ./...` (both must pass)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/utils/commands/address-reviews.md` at line 217, The documentation line that reads "`go.mod` exists → `go build ./...` and `go vet ./...`" is ambiguous about whether both commands must succeed; update the text to explicitly state that both `go build ./...` and `go vet ./...` must pass for the Go verification case so reviewers know to require successful builds and vetting. Locate the sentence containing the exact snippet "`go.mod` exists → `go build ./...` and `go vet ./...`" and modify it to a clear statement such as "Both `go build ./...` and `go vet ./...` must pass" (keeping the surrounding style consistent).
214-219: Clarify how to detect Makefile targets.The verification detection checks for "Makefile with a
verifytarget" but doesn't specify how to programmatically determine if a target exists. Consider adding implementation guidance.📝 Suggested clarification
1. **Detect available verification commands** (check in order, use the first that exists): - - `Makefile` or `makefile` with a `verify` target → `make verify` - - `Makefile` with a `lint` target → `make lint` + - `Makefile` or `makefile` with a `verify` target (check: `make -n verify 2>&1` doesn't output "No rule") → `make verify` + - `Makefile` with a `lint` target (check: `make -n lint 2>&1` doesn't output "No rule") → `make lint` - `go.mod` exists → `go build ./...` and `go vet ./...`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/utils/commands/address-reviews.md` around lines 214 - 219, Clarify how to detect Makefile targets by specifying the detection approach: when checking for a Makefile/makefile, either (a) attempt a safe non-executing make query (e.g., run make -n verify or make -q verify and treat success/exit code as presence of the target) or (b) fall back to parsing the Makefile file contents for a target definition using a regex that matches a target at line start (e.g., ^verify\s*:) to detect the `verify` target; document that the code should prefer the make query method for accuracy and fall back to file content parsing if make is unavailable, and ensure the check is case-insensitive for the filename and robust to whitespace/comments in the Makefile definition.
226-227: Clarify the retry "fix" mechanism.The instructions state "fix the issues, amend the relevant commit, and re-run verification" but don't specify whether fixes are automated (if so, how?) or require manual user intervention. This ambiguity could lead to inconsistent implementations.
📝 Suggested clarification
- - If verification fails: fix the issues, amend the relevant commit, and re-run verification + - If verification fails: analyze the error output, attempt to fix the issues automatically when possible (e.g., formatting issues), or ask the user for guidance on non-trivial failures. Then amend the relevant commit and re-run verification. - Maximum 3 retry attempts. If verification still fails after 3 fix-and-retry cycles, stop and report to the user: "Verification continues to fail after 3 attempts. Last error: [error]. Manual intervention needed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/utils/commands/address-reviews.md` around lines 226 - 227, Update the two bullets "If verification fails: fix the issues, amend the relevant commit, and re-run verification" and "Maximum 3 retry attempts..." to explicitly state whether fixes are manual or automated; if manual, say "manually fix the issues in your working tree, amend the relevant commit (git commit --amend) and re-run verification" and require user confirmation between attempts; if automated fixes are supported, document the exact command/flag (e.g., --auto-fix), what kinds of fixes it will perform, and any limits, and ensure the final stop message includes whether auto-fix was attempted and the last error ("Verification continues to fail after 3 attempts. Last error: [error]. Manual intervention needed. Auto-fix attempted: yes/no").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/utils/commands/address-reviews.md`:
- Line 217: The documentation line that reads "`go.mod` exists → `go build
./...` and `go vet ./...`" is ambiguous about whether both commands must
succeed; update the text to explicitly state that both `go build ./...` and `go
vet ./...` must pass for the Go verification case so reviewers know to require
successful builds and vetting. Locate the sentence containing the exact snippet
"`go.mod` exists → `go build ./...` and `go vet ./...`" and modify it to a clear
statement such as "Both `go build ./...` and `go vet ./...` must pass" (keeping
the surrounding style consistent).
- Around line 214-219: Clarify how to detect Makefile targets by specifying the
detection approach: when checking for a Makefile/makefile, either (a) attempt a
safe non-executing make query (e.g., run make -n verify or make -q verify and
treat success/exit code as presence of the target) or (b) fall back to parsing
the Makefile file contents for a target definition using a regex that matches a
target at line start (e.g., ^verify\s*:) to detect the `verify` target; document
that the code should prefer the make query method for accuracy and fall back to
file content parsing if make is unavailable, and ensure the check is
case-insensitive for the filename and robust to whitespace/comments in the
Makefile definition.
- Around line 226-227: Update the two bullets "If verification fails: fix the
issues, amend the relevant commit, and re-run verification" and "Maximum 3 retry
attempts..." to explicitly state whether fixes are manual or automated; if
manual, say "manually fix the issues in your working tree, amend the relevant
commit (git commit --amend) and re-run verification" and require user
confirmation between attempts; if automated fixes are supported, document the
exact command/flag (e.g., --auto-fix), what kinds of fixes it will perform, and
any limits, and ensure the final stop message includes whether auto-fix was
attempted and the last error ("Verification continues to fail after 3 attempts.
Last error: [error]. Manual intervention needed. Auto-fix attempted: yes/no").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cea90616-5bf5-453c-a2ee-e0d574d6931f
📒 Files selected for processing (4)
.claude-plugin/marketplace.jsondocs/data.jsonplugins/utils/.claude-plugin/plugin.jsonplugins/utils/commands/address-reviews.md
✅ Files skipped from review due to trivial changes (3)
- plugins/utils/.claude-plugin/plugin.json
- docs/data.json
- .claude-plugin/marketplace.json
Collaborator
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 tasks
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
2 participants
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.
Summary
Addresses three failures reported by Alberto on hypershift PRs (PR 8249, PR 7810, PR 8223):
line=null, original_line≠null) were incorrectly filtered out. Now only truly orphaned comments (both null) are discarded.make verify→make lint→go build/vet→npm lint), blocks push on failure, max 3 retries with 15-minute timeout.Also includes
original_linein both first-pass and second-pass API queries, retry bounds on verification, and a note about post-rebase line number staleness.Test plan
/utils:address-reviewson a PR with outdated inline comments (line=null, original_line set) — verify they are fetched and addressedmake verifyfails — verify it blocks push and retries🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features