review-agent: fix comment pre-filter to use per-comment reply matching#77742
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReworked bot classification and reply-matching in the hypershift review agent: introduced Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 363-381: The loop currently sets last_actionable for any non-bot
commenter which lets an unauthorized user overwrite a prior authorized
actionable comment; change the assignment so you only update last_actionable
when the comment author passes is_authorized_author(author). Keep existing bot
handling (is_our_bot/is_ignored_bot) and last_our_bot logic, and ensure
subsequent code still derives last_author and author_type from last_actionable
and uses is_approved_bot as before.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f7a8048-c5f8-483e-83e5-5dfb2def2a1a
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
c23c048 to
6a54b93
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 466-467: The current reply detection uses a raw substring check
(replied = any(str(review_id) in b for b in bot_bodies)) which over-matches IDs;
change the logic to only recognize the explicit linkage markers used elsewhere
(e.g., the stable "Re:" or anchored comment markers) by matching the exact,
bounded pattern around review_id—use a regex or string boundary check that looks
for the explicit marker plus the numeric id (not plain containment) when
scanning bot_bodies and update the same pattern usage in the other spots
referenced (the replied assignment, the block around 530-533, and the block
around 844-846) so only exact marker-linked replies are considered matches.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ce26f54-7606-462c-b292-915b0bbf8029
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
6a54b93 to
25a43f0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (1)
527-535:⚠️ Potential issue | 🟠 MajorMatch
html_urlreplies exactly, not by substring.Line 533 still uses raw substring matching for
comment_url. A reply to...#issuecomment-1234also contains...#issuecomment-123, so comment123can be treated as addressed when only1234was answered. Since the prompt now requiresRe: <html_url>on its own line, match that marker exactly here too.Proposed fix
comment_id = str(comment["id"]) comment_url = comment.get("html_url", "") # Match by full URL or by the `#issuecomment-`{id} anchor with word boundary # to prevent `#issuecomment-123` from matching `#issuecomment-1234` anchor_pattern = re.compile(rf"#issuecomment-{re.escape(comment_id)}(?!\d)") + url_pattern = ( + re.compile(rf"(?m)^\s*Re:\s*{re.escape(comment_url)}\s*$") + if comment_url else None + ) replied = any( - anchor_pattern.search(body) or (comment_url and comment_url in body) + anchor_pattern.search(body) or (url_pattern and url_pattern.search(body)) for body in bot_bodies )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 527 - 535, The current replied check treats comment_url as a substring which can falsely match similar URLs; update the replied logic to require the exact reply marker on its own line by checking for a line equal to the expected marker (e.g., "Re: {comment_url}") instead of using substring membership — modify the generator that builds replied (which currently uses anchor_pattern, comment_url and bot_bodies) so the second condition tests each body line-for-line (or via a regex anchored with ^ and $) for the exact "Re: {comment_url}" string rather than using "comment_url in body".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 527-535: The current replied check treats comment_url as a
substring which can falsely match similar URLs; update the replied logic to
require the exact reply marker on its own line by checking for a line equal to
the expected marker (e.g., "Re: {comment_url}") instead of using substring
membership — modify the generator that builds replied (which currently uses
anchor_pattern, comment_url and bot_bodies) so the second condition tests each
body line-for-line (or via a regex anchored with ^ and $) for the exact "Re:
{comment_url}" string rather than using "comment_url in body".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a64b8eef-ed82-481c-b045-7e827a6c5e00
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@bryan-cox: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
This is working as seen here, the original issue documented in the PR description - openshift/hypershift#8211 (comment) |
|
/pj-rehearse ack |
|
@bryan-cox: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
d01d967 to
6810f7f
Compare
6810f7f to
3de6ca9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (1)
467-469:⚠️ Potential issue | 🟠 MajorUse exact linkage-marker matching for flat comments/review bodies.
Line 468 and Line 533 still allow permissive containment matching. That can over-match and suppress legitimate follow-ups (the same class of failure this PR is addressing). Since Line 846-848 already defines stable reply markers, match those markers exactly.
Proposed tightening
+def has_review_reply_marker(body: str, review_id: str) -> bool: + return re.search( + rf"(?m)^\s*Re:\s*review\s+{re.escape(review_id)}\s*$", + body or "", + ) is not None + + +def has_issue_reply_marker(body: str, comment_id: str, comment_url: str) -> bool: + text = body or "" + if comment_url and re.search(rf"(?m)^\s*Re:\s*{re.escape(comment_url)}\s*$", text): + return True + return re.search(rf"(?<!\d)#issuecomment-{re.escape(comment_id)}(?!\d)", text) is not None @@ - replied = any(str(review_id) in b for b in bot_bodies) + replied = any(has_review_reply_marker(b, review_id) for b in bot_bodies) @@ - replied = any( - anchor_pattern.search(body) or (comment_url and comment_url in body) - for body in bot_bodies - ) + replied = any( + has_issue_reply_marker(body, comment_id, comment_url) + for body in bot_bodies + )Also applies to: 530-535, 846-848
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 467 - 469, The current permissive check "replied = any(str(review_id) in b for b in bot_bodies)" should be changed to exact marker matching: construct the exact reply marker string using the same format used when posting replies (the stable reply marker variable used elsewhere, e.g., REPLY_MARKER or reply_marker) and then test equality (e.g., any(b.strip() == reply_marker for b in bot_bodies)) instead of using the "in" containment; update the same pattern in the other occurrences that check bot_bodies/bot comment bodies so all checks use the exact reply marker comparison (refer to variables replied, bot_bodies, review_id and the stable reply marker constant).
🧹 Nitpick comments (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (1)
868-873: Tail truncation can under-report tool activity in the report step.Line 872 keeps only the last 200 lines, but the report parser reads
tool_use/tool_resultevents from this file (seeci-operator/step-registry/hypershift/review-agent/report/hypershift-review-agent-report-commands.sh, Line 195-230). Early tool events will be dropped, making report telemetry incomplete.Safer compacting approach
- if [ -f "/tmp/claude-pr-${PR_NUMBER}-output.json" ]; then - tail -200 "/tmp/claude-pr-${PR_NUMBER}-output.json" > "${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json" - fi + if [ -f "/tmp/claude-pr-${PR_NUMBER}-output.json" ]; then + # Keep compact telemetry-relevant events plus final window for context + { + grep -E '"type":"result"|"type":"tool_use"|"type":"tool_result"|"is_error":true' "/tmp/claude-pr-${PR_NUMBER}-output.json" || true + tail -100 "/tmp/claude-pr-${PR_NUMBER}-output.json" || true + } | awk 'NF' > "${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 868 - 873, The current truncation in hypershift-review-agent-process-commands.sh drops early tool events by tailing only the last 200 lines of /tmp/claude-pr-${PR_NUMBER}-output.json before writing to ${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json; instead, preserve tool telemetry by extracting and saving all lines that contain the report parser markers ("tool_use" and "tool_result") plus a compact summary (e.g., keep the first N and last M lines) so the report step can still read events — modify the block that writes from /tmp/claude-pr-${PR_NUMBER}-output.json to first grep for "tool_use" and "tool_result" (or a combined head/tail strategy) and write those matched lines (and optionally head+tail) into ${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json to avoid losing early tool events while staying within size limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 467-469: The current permissive check "replied =
any(str(review_id) in b for b in bot_bodies)" should be changed to exact marker
matching: construct the exact reply marker string using the same format used
when posting replies (the stable reply marker variable used elsewhere, e.g.,
REPLY_MARKER or reply_marker) and then test equality (e.g., any(b.strip() ==
reply_marker for b in bot_bodies)) instead of using the "in" containment; update
the same pattern in the other occurrences that check bot_bodies/bot comment
bodies so all checks use the exact reply marker comparison (refer to variables
replied, bot_bodies, review_id and the stable reply marker constant).
---
Nitpick comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 868-873: The current truncation in
hypershift-review-agent-process-commands.sh drops early tool events by tailing
only the last 200 lines of /tmp/claude-pr-${PR_NUMBER}-output.json before
writing to ${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json; instead, preserve
tool telemetry by extracting and saving all lines that contain the report parser
markers ("tool_use" and "tool_result") plus a compact summary (e.g., keep the
first N and last M lines) so the report step can still read events — modify the
block that writes from /tmp/claude-pr-${PR_NUMBER}-output.json to first grep for
"tool_use" and "tool_result" (or a combined head/tail strategy) and write those
matched lines (and optionally head+tail) into
${SHARED_DIR}/claude-pr-${PR_NUMBER}-output.json to avoid losing early tool
events while staying within size limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26982c85-bc18-4bf5-80a4-3084f01252df
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (2)
467-468:⚠️ Potential issue | 🟠 MajorMatch review-body replies with the explicit
Re: review <id>marker.Line 468 still treats any bot comment containing the review ID as “replied.” Now that the prompt requires a dedicated
Re: review <id>line, raw containment can still suppress an unaddressed review body if the ID is mentioned incidentally elsewhere in the bot comment.Suggested fix
- replied = any(str(review_id) in b for b in bot_bodies) + review_pattern = re.compile( + rf"(?m)^\s*Re:\s*review\s+{re.escape(review_id)}\s*$" + ) + replied = any(review_pattern.search(body) for body in bot_bodies)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 467 - 468, The current replied check uses simple containment (replied = any(str(review_id) in b for b in bot_bodies)) which falsely counts incidental mentions; change it to match the explicit "Re: review <id>" marker by testing each bot body (bot_bodies) against a precise pattern that includes the "Re:" prefix and the word "review" followed by the review_id (for example using a regex search like r'\bRe:\s*review\s+{review_id}\b' or equivalent) so replied only becomes True when a bot comment includes the exact "Re: review <id>" line.
527-534:⚠️ Potential issue | 🟠 MajorAvoid prefix-matching shorter
html_urlvalues against other issue-comment URLs.Line 533 still uses
comment_url in body. That makes...#issuecomment-123look addressed by a reply that only references...#issuecomment-1234, because the shorter URL is a prefix of the longer one. This can re-hide legitimate flat comments.Suggested fix
comment_id = str(comment["id"]) comment_url = comment.get("html_url", "") # Match by full URL or by the `#issuecomment-`{id} anchor with word boundary # to prevent `#issuecomment-123` from matching `#issuecomment-1234` anchor_pattern = re.compile(rf"#issuecomment-{re.escape(comment_id)}(?!\d)") + url_pattern = re.compile( + rf"(?m)^\s*Re:\s*{re.escape(comment_url)}\s*$" + ) if comment_url else None replied = any( - anchor_pattern.search(body) or (comment_url and comment_url in body) + anchor_pattern.search(body) or (url_pattern and url_pattern.search(body)) for body in bot_bodies )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 527 - 534, The current reply check uses "comment_url in body" which allows shorter URLs to match as prefixes of longer ones; replace that substring check with a regex search that matches the full escaped URL only when not immediately followed by a non-whitespace character. Concretely, where replied is computed (using variables anchor_pattern, comment_url, comment_id, bot_bodies), change the second predicate to something like: if comment_url, use re.search(re.escape(comment_url) + r'(?!\S)', body) instead of "comment_url in body" so only exact/full URL occurrences are treated as replies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 467-468: The current replied check uses simple containment
(replied = any(str(review_id) in b for b in bot_bodies)) which falsely counts
incidental mentions; change it to match the explicit "Re: review <id>" marker by
testing each bot body (bot_bodies) against a precise pattern that includes the
"Re:" prefix and the word "review" followed by the review_id (for example using
a regex search like r'\bRe:\s*review\s+{review_id}\b' or equivalent) so replied
only becomes True when a bot comment includes the exact "Re: review <id>" line.
- Around line 527-534: The current reply check uses "comment_url in body" which
allows shorter URLs to match as prefixes of longer ones; replace that substring
check with a regex search that matches the full escaped URL only when not
immediately followed by a non-whitespace character. Concretely, where replied is
computed (using variables anchor_pattern, comment_url, comment_id, bot_bodies),
change the second predicate to something like: if comment_url, use
re.search(re.escape(comment_url) + r'(?!\S)', body) instead of "comment_url in
body" so only exact/full URL occurrences are treated as replies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: af6bffd5-58da-4c08-a1f5-a427cc547c9e
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
The comment analyzer used a global last_bot_time cutoff to determine
which comments were already addressed. Any bot comment (even unrelated)
posted after a human comment would suppress it. This caused legitimate
review feedback to be skipped when an unrelated bot comment happened to
be more recent.
Fix by matching replies per-comment:
- Issue comments: check if our bot's reply contains the comment URL
or #issuecomment-{id} anchor (requires bot to include the link)
- Review bodies: check if our bot's reply contains the review ID
- Review threads: unchanged (already per-thread via GraphQL)
Also fix bot classification into three categories:
- is_our_bot(): BOT_ACCOUNTS only - counts as "already replied"
- is_approved_bot(): APPROVED_BOTS (e.g. coderabbitai) - triggers
responses, was previously blocked by the catch-all [bot] suffix
check in is_bot()
- is_ignored_bot(): unrecognized [bot] accounts - skipped
Add response rule instructing the agent to include comment URL/ID
references when replying to flat comments for reply linkage.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3de6ca9 to
5e8aeb5
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@bryan-cox: The following test 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. |
|
/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 |
|
/pj-rehearse ack |
|
@bryan-cox: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift#77742) The comment analyzer used a global last_bot_time cutoff to determine which comments were already addressed. Any bot comment (even unrelated) posted after a human comment would suppress it. This caused legitimate review feedback to be skipped when an unrelated bot comment happened to be more recent. Fix by matching replies per-comment: - Issue comments: check if our bot's reply contains the comment URL or #issuecomment-{id} anchor (requires bot to include the link) - Review bodies: check if our bot's reply contains the review ID - Review threads: unchanged (already per-thread via GraphQL) Also fix bot classification into three categories: - is_our_bot(): BOT_ACCOUNTS only - counts as "already replied" - is_approved_bot(): APPROVED_BOTS (e.g. coderabbitai) - triggers responses, was previously blocked by the catch-all [bot] suffix check in is_bot() - is_ignored_bot(): unrecognized [bot] accounts - skipped Add response rule instructing the agent to include comment URL/ID references when replying to flat comments for reply linkage. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
openshift#77742) The comment analyzer used a global last_bot_time cutoff to determine which comments were already addressed. Any bot comment (even unrelated) posted after a human comment would suppress it. This caused legitimate review feedback to be skipped when an unrelated bot comment happened to be more recent. Fix by matching replies per-comment: - Issue comments: check if our bot's reply contains the comment URL or #issuecomment-{id} anchor (requires bot to include the link) - Review bodies: check if our bot's reply contains the review ID - Review threads: unchanged (already per-thread via GraphQL) Also fix bot classification into three categories: - is_our_bot(): BOT_ACCOUNTS only - counts as "already replied" - is_approved_bot(): APPROVED_BOTS (e.g. coderabbitai) - triggers responses, was previously blocked by the catch-all [bot] suffix check in is_bot() - is_ignored_bot(): unrecognized [bot] accounts - skipped Add response rule instructing the agent to include comment URL/ID references when replying to flat comments for reply linkage. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
openshift#77742) The comment analyzer used a global last_bot_time cutoff to determine which comments were already addressed. Any bot comment (even unrelated) posted after a human comment would suppress it. This caused legitimate review feedback to be skipped when an unrelated bot comment happened to be more recent. Fix by matching replies per-comment: - Issue comments: check if our bot's reply contains the comment URL or #issuecomment-{id} anchor (requires bot to include the link) - Review bodies: check if our bot's reply contains the review ID - Review threads: unchanged (already per-thread via GraphQL) Also fix bot classification into three categories: - is_our_bot(): BOT_ACCOUNTS only - counts as "already replied" - is_approved_bot(): APPROVED_BOTS (e.g. coderabbitai) - triggers responses, was previously blocked by the catch-all [bot] suffix check in is_bot() - is_ignored_bot(): unrecognized [bot] accounts - skipped Add response rule instructing the agent to include comment URL/ID references when replying to flat comments for reply linkage. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes the review agent's comment pre-filter which incorrectly skipped legitimate review feedback.
The problem: The
comment_analyzer.pyused a globallast_bot_timetimestamp cutoff to determine which comments were already addressed. Any bot comment posted after a human comment — even a completely unrelated one — would suppress that human comment from being flagged.How it was discovered: On PR #8211, a reviewer left feedback ("verify is failing"), then triggered the job (
/test address-review-comments). The job ran successfully but concluded there were no comments needing attention — because an unrelated bot comment (a test failure analysis) happened to be posted between the reviewer's comment and the/testtrigger.The fix:
Re: <url>) when replying. The pre-filter checks if any bot reply references a specific comment's URL or#issuecomment-{id}anchor.Bot classification is now three-way instead of a single
is_bot():BOT_ACCOUNTS)hypershift-jira-solve-ci[bot]APPROVED_BOTS)coderabbitai[bot]author_type: "approved_bot"IGNORED_ACCOUNTS+[bot]suffix)openshift-ci-robot,openshift-ciThis also fixes a pre-existing issue where
coderabbitai[bot]comments were silently dropped by the oldis_bot()catch-all[bot]suffix check, even thoughAPPROVED_BOTSwas defined.Test plan
comment_analyzer.pyand ran locally against PR Add kubevirt-builder as build base #8211openshift-ci-robot) are filtered outcoderabbitai[bot]comments are flagged withauthor_type: "approved_bot"🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Chores