fix(address-reviews): Add duplicate prevention and response rules - #301
openshift-merge-bot[bot] merged 1 commit into
Conversation
WalkthroughAdds a new CLI script Changes
Sequence DiagramsequenceDiagram
participant User as User / CI
participant Script as check_replied.py
participant GH as GitHub CLI (gh)
participant Output as JSON Result
User->>Script: Invoke CLI (--type, owner, repo, pr_number, id)
Script->>Script: Parse args & select handler
alt review_thread
Script->>GH: gh api (GraphQL) -> fetch review threads (paginated)
GH-->>Script: Threads JSON
Script->>Script: Locate thread by id -> scan comments
else issue_comment
Script->>GH: gh api (REST) -> list issue comments
GH-->>Script: Comments JSON
Script->>Script: Find target comment -> check subsequent comments
else review_comment
Script->>GH: gh api (REST) -> list PR review comments
GH-->>Script: Comments JSON
Script->>Script: Find target comment -> inspect replies
end
Script->>Script: is_bot_reply() checks author signatures or reply signature
Script->>Output: Emit JSON {safe_to_reply, reason, existing_reply?} and exit (0/1/2)
Output-->>User: Receive JSON + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (6 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
f146335 to
2c0af67
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@plugins/utils/scripts/check_replied.py`:
- Around line 131-139: The exception handler in the block that calls run_gh (the
API call that fetches comments) currently returns "safe_to_reply": True on
RuntimeError; change it to return "safe_to_reply": False with reason "api_error"
(and keep the existing message=str(e)) to avoid the bot treating transient API
failures as safe to reply; locate the try/except around run_gh (the comments =
run_gh([...]) call, likely in check_replied) and update the returned dict
accordingly.
- Around line 190-192: The call that fetches review comments via run_gh
(assigning to comments) is missing pagination; update the run_gh invocation that
builds ["api", f"repos/{owner}/{repo}/pulls/{pr_number}/comments"] to include
the "--paginate" flag so all review comments are retrieved (e.g., add
"--paginate" to the argument list passed to run_gh), and keep using the comments
variable as before.
- Around line 189-198: The current except block for the run_gh call in
check_replied.py returns safe_to_reply: True on API failures; change it to
return safe_to_reply: False (and keep reason "api_error" and message=str(e)) so
temporary API outages don't allow replies. Locate the try/except around the
run_gh(["api", f"repos/{owner}/{repo}/pulls/{pr_number}/comments"]) call and
update the except RuntimeError as e return value to safe_to_reply: False to
match the safer handling used in check_issue_comment.
- Around line 277-280: The except block in check_replied.py currently catches a
bare Exception and sets result = {"error": str(e), "safe_to_reply": True,
"reason": "error_fallback"}, which is too broad and unsafe; change it to catch
only expected exceptions (e.g., ValueError, KeyError, json.JSONDecodeError or
specific custom exceptions used by the parsing logic) and handle unexpected
exceptions by re-raising or logging and exiting non-zero; when an expected
handled error occurs set "safe_to_reply": False (not True) and include the full
error details in the "error" field and "reason"; update the exception handler
around the code that produces result (the except block that assigns result and
calls print(json.dumps(result)) / sys.exit(2)) to implement these narrower
catches and fallback behavior.
- Around line 131-133: The script fetches issue and review comments via
run_gh([...]) without pagination, so change the two run_gh calls that request
"repos/{owner}/{repo}/issues/{pr_number}/comments" and
"repos/{owner}/{repo}/pulls/{pr_number}/comments" to include the "--paginate"
flag (e.g., add "--paginate" as an additional argument in both calls) so all
pages of comments are returned rather than only the first 30 items.
🧹 Nitpick comments (2)
plugins/utils/scripts/check_replied.py (2)
35-36: Use unpacking for more idiomatic list construction.♻️ Suggested improvement
- result = subprocess.run( - ["gh"] + args, + result = subprocess.run( + ["gh", *args], capture_output=True, text=True, )
269-272: Remove unreachable code.The
elsebranch on line 269 is unreachable because argparse enforces thechoicesparameter, ensuringargs.typeis always one of the three valid values.♻️ Simplify by removing unreachable branch
if args.type == "review_thread": result = check_review_thread(args.owner, args.repo, args.pr_number, args.comment_id) elif args.type == "issue_comment": result = check_issue_comment(args.owner, args.repo, args.pr_number, args.comment_id) - elif args.type == "review_comment": + else: # args.type == "review_comment" result = check_review_comment(args.owner, args.repo, args.pr_number, args.comment_id) - else: - result = {"error": f"Unknown type: {args.type}"} - print(json.dumps(result, indent=2)) - sys.exit(2)
eca929f to
3f94e24
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/utils/scripts/check_replied.py`:
- Around line 45-54: The is_bot_reply function currently treats any account
whose login endswith("[bot]") as our bot and thus skips replies; remove that
heuristic and only detect bot replies by membership in BOT_SIGNATURES and by the
REPLY_SIGNATURE in the body. Edit is_bot_reply to drop the
login.endswith("[bot]") condition so it returns True only when login is in
BOT_SIGNATURES or when REPLY_SIGNATURE appears in body; reference the
is_bot_reply function and the BOT_SIGNATURES and REPLY_SIGNATURE constants when
making this change.
- Around line 60-79: Update the GraphQL query in check_replied.py to include
pageInfo { hasNextPage endCursor } for both reviewThreads and comments and
implement cursor-based pagination: paginate reviewThreads by repeatedly querying
reviewThreads(first:100, after:$threadsCursor) using the returned endCursor
until hasNextPage is false, and for any thread whose comments may exceed 100,
fetch comments either by including comments(first:100, after:$commentsCursor) in
the same loop or by re-querying that thread with comments pagination using the
comments endCursor until hasNextPage is false; ensure the query variables (e.g.,
$threadsCursor, $commentsCursor) are added and the code that inspects
reviewThreads.nodes and comments.nodes in the function that checks replies
aggregates results from all pages before deciding "safe to reply."
| query = ''' | ||
| query($owner: String!, $repo: String!, $number: Int!) { | ||
| repository(owner: $owner, name: $repo) { | ||
| pullRequest(number: $number) { | ||
| reviewThreads(first: 100) { | ||
| nodes { | ||
| id | ||
| comments(first: 100) { | ||
| nodes { | ||
| id | ||
| author { login } | ||
| body | ||
| createdAt | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "check_replied.py" -type fRepository: openshift-eng/ai-helpers
Length of output: 108
🏁 Script executed:
fd check_replied.pyRepository: openshift-eng/ai-helpers
Length of output: 106
🏁 Script executed:
git ls-files "*check_replied*"Repository: openshift-eng/ai-helpers
Length of output: 106
🏁 Script executed:
cat -n plugins/utils/scripts/check_replied.pyRepository: openshift-eng/ai-helpers
Length of output: 10937
🌐 Web query:
GitHub GraphQL API reviewThreads pagination pageInfo endCursor
💡 Result:
Use cursor-based pagination: request pageInfo (hasNextPage + endCursor) on the PullRequest.reviewThreads connection, then loop supplying pageInfo.endCursor as the after argument until hasNextPage is false.
Example (forward pagination):
query:
{
repository(owner: "OWNER", name: "REPO") {
pullRequest(number: 123) {
reviewThreads(first: 50, after: null) {
nodes {
id
isResolved
comments(first: 10) { nodes { id body } }
}
pageInfo {
endCursor
hasNextPage
}
}
}
}
}
If pageInfo.hasNextPage is true, run the same query with after: "" (and optionally keep first: 50) and repeat until hasNextPage is false. The reviewThreads connection is PullRequestReviewThreadConnection and exposes pageInfo.endCursor/hasNextPage. [1][2]
Sources:
[1] GitHub Docs — Using pagination in the GraphQL API.
[2] GitHub GraphQL schema docs — PullRequest / PullRequestReviewThreadConnection.
🌐 Web query:
GitHub GraphQL API comments pagination nested within reviewThreads first 100 limit
💡 Result:
Short answer: each GraphQL connection (including reviewThreads and the comments connection inside each thread) is limited to first/last ≤ 100 and must be paginated independently — you can’t get “more than 100 comments” for a nested connection in one shot. Paginate the outer reviewThreads (first:100 → use pageInfo.endCursor → after), and for any thread whose comments exceed 100 paginate that thread’s comments connection (comments(first:100, after: ...)) using that thread’s comment cursors or by issuing follow-up queries per thread. The Octokit graphql paginator does not handle nested/parallel pagination for you, so you must drive nested pagination yourself (or request comment pages separately by thread id).
Key references:
- GitHub GraphQL pagination docs (max first/last = 100 and cursor-based paging). [1]
- Octokit plugin note: “This plugin only supports pagination of a single resource — nested pagination is not supported.” [2]
- Community discussion pointing out UX/implementation complexities of reviewThreads → comments and need to paginate threads and comments separately. [3]
If you want, I can sketch a concrete pagination flow or example queries (server-side loop or async per-thread) for your use case.
Add pagination to GraphQL query to handle PRs with >100 review threads or comments.
The reviewThreads(first: 100) and nested comments(first: 100) queries only fetch the first page. On large PRs with many review threads or comments, bot replies beyond the first 100 will be missed, returning false "safe to reply" results.
Add pageInfo { hasNextPage endCursor } to both reviewThreads and comments connections, then implement a pagination loop to fetch all pages. Since nested pagination is not automatically handled, you'll need to paginate reviewThreads with cursor-based queries, and potentially re-query comments for threads that have more than 100 comments.
🤖 Prompt for AI Agents
In `@plugins/utils/scripts/check_replied.py` around lines 60 - 79, Update the
GraphQL query in check_replied.py to include pageInfo { hasNextPage endCursor }
for both reviewThreads and comments and implement cursor-based pagination:
paginate reviewThreads by repeatedly querying reviewThreads(first:100,
after:$threadsCursor) using the returned endCursor until hasNextPage is false,
and for any thread whose comments may exceed 100, fetch comments either by
including comments(first:100, after:$commentsCursor) in the same loop or by
re-querying that thread with comments pagination using the comments endCursor
until hasNextPage is false; ensure the query variables (e.g., $threadsCursor,
$commentsCursor) are added and the code that inspects reviewThreads.nodes and
comments.nodes in the function that checks replies aggregates results from all
pages before deciding "safe to reply."
- Add check_replied.py script to verify bot hasn't already replied - Add response rules to prevent duplicate responses and unwanted code changes - Only match our specific bot accounts, not all [bot] accounts - Add GraphQL pagination for PRs with >100 review threads - Use --paginate flag for REST API calls to handle >30 comments - Return safe_to_reply=False on API errors (fail safe) - Narrow exception handling to specific types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
89d7110 to
8dbce90
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
It's odd that CodeRabbit seems to have wedged itself on this PR, but a manual review looks good to me /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, cblecker 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 |
Summary
check_replied.pyscript to verify bot hasn't already replied before postingChanges
New Script:
plugins/utils/scripts/check_replied.pyA Python script that checks if the bot has already replied to a comment before posting. Supports:
Updated Skill:
plugins/utils/commands/address-reviews.mdAdded duplicate prevention guidelines:
check_replied.pybefore posting any replyRelated
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores